Skip to content

fix(runs): preserve oneof fields when decoding revision spec - #621

Open
trtyr wants to merge 1 commit into
chaitin:mainfrom
trtyr:fix/decode-revision-spec-preserve-oneof
Open

fix(runs): preserve oneof fields when decoding revision spec#621
trtyr wants to merge 1 commit into
chaitin:mainfrom
trtyr:fix/decode-revision-spec-preserve-oneof

Conversation

@trtyr

@trtyr trtyr commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Problem

GetProject returns a project spec whose driver field loses its runtime config oneof. A GetProject → PatchProject round-trip fails with:

driver requires exactly one runtime config

Repro: apply a project whose agent has driver: {docker: {}}, then GetProject (includeSpec) → PatchProject the returned spec. The persisted spec_json is correct ({"name":"docker","docker":{}}), but the returned spec drops docker: {}, leaving only {"name":"docker"}.

Root cause

DecodeRevisionSpec unmarshals the persisted spec into the generated proto message using encoding/json:

json.Unmarshal(normalizedData, &spec)

encoding/json does not understand proto oneof fields. DriverSpec.Config is an isDriverSpec_Config interface with no json tag, so the docker key is ignored and Config stays nil. This silently drops every oneof field, not just driver config.

Fix

Switch to protojson.Unmarshal (with DiscardUnknown so the legacy canonical workspace fields — still restored separately by restoreCanonicalRevisionWorkspaces — remain tolerated):

opts := protojson.UnmarshalOptions{DiscardUnknown: true}
opts.Unmarshal(normalizedData, &spec)

protojson understands oneof and enum values, so DriverSpec.Config (and other oneofs) round-trip correctly.

Test

  • New regression test TestDecodeRevisionSpecPreservesDriverRuntimeConfig asserts the driver oneof survives decoding.
  • go test ./pkg/runs/ → 221 passed
  • go test ./pkg/agentcompose/api/ ./pkg/agentcompose/app/ → 321 passed

DecodeRevisionSpec used encoding/json to unmarshal the persisted
project revision spec into the generated proto message. encoding/json
does not understand proto oneof fields, so DriverSpec.config (and any
other oneof) was silently dropped. A GetProject -> PatchProject
round-trip then failed with "driver requires exactly one runtime
config" because the returned driver carried only its name.

Switch to protojson.Unmarshal with DiscardUnknown so oneof fields decode
correctly while the legacy canonical workspace fields (still handled by
restoreCanonicalRevisionWorkspaces) are tolerated.
@monkeyscan

monkeyscan Bot commented Aug 21, 2026

Copy link
Copy Markdown

PR Title: fix(runs): preserve oneof fields when decoding rev...

Commit: c0f3da3

本次变更将 DecodeRevisionSpec 中用于解析项目 revision spec 的 encoding/json.Unmarshal 替换为 protojson.Unmarshal(UnmarshalOptions{DiscardUnknown: true}),并新增回归测试验证 driver 的 oneof 运行时配置被保留。

修复动机正确:encoding/json 无法给 protobuf 生成结构体的 oneof 接口字段赋值,导致 GetProject→PatchProject 往返中 driver config oneof 丢失,PatchProject 校验报 "driver requires exactly one runtime config"。protojson 是 protobuf 官方的 JSON 编解码器,能正确解析 oneof、enum 名称、64 位整数等 proto3 JSON 规范表示;测试输入(camelCase 且 oneof 成员直接作为键)也证实存储格式为 protojson 形式,因此新读取器与写入器一致,整体方向正确,未发现高置信度功能性缺陷。

需要注意的回归覆盖缺口:该替换影响所有已存储 revision spec 的解码路径,而 protojson 对 well-known 类型要求 proto3 JSON 规范表示、对重复键直接报错,与 encoding/json 的结构体字段表示存在差异;新增测试仅覆盖 driver 的 docker oneof 单一路径,未覆盖完整往返与既有存储数据兼容性,故提交一条低严重度发现建议补充往返与兼容性测试。

Comment thread pkg/runs/preparation.go
return nil, fmt.Errorf("decode project revision spec: %w", err)
}
if err := json.Unmarshal(normalizedData, &spec); err != nil {
opts := protojson.UnmarshalOptions{DiscardUnknown: true}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protojson 解码切换缺少对既有存储 spec 兼容性与完整往返的回归覆盖

将 encoding/json.Unmarshal 替换为 protojson.Unmarshal 后,所有已存储的 project revision spec(revision.SpecJSON)的解码语义都发生了变化。encoding/json 按 protobuf 生成结构体的字段标签(camelCase + Go 字段结构)解析,而 protojson 遵循 proto3 JSON 规范:well-known 类型必须使用其规范表示(如 google.protobuf.Timestamp 为 RFC3339 字符串而非生成的 Go 结构体 {"seconds":...,"nanos":...}、Any 使用 @type),并且对同一对象的重复键直接报错而非 last-wins。若存在任何以非 protojson 规范序列化的历史 spec(例如旧写入路径使用 encoding/json.Marshal 产生的结构字段表示),切换后 DecodeRevisionSpec 将解码失败,进而导致 PrepareProjectRun 与 GetProject 中断;而 DiscardUnknown 又会使未知/无法识别的字段被静默丢弃。新增测试仅覆盖 driver 的 docker oneof 这一条最小路径(不含 workspaces/closed-sets 等经过 normalizeRevisionClosedSetsJSON 与 restoreCanonicalRevisionWorkspaces 处理的字段),既未验证 GetProject→PatchProject 的完整往返(解码后重新 protojson.Marshal 再被 PatchProject 接受),也未覆盖与既有存储数据格式的兼容性,回归风险面未被充分覆盖。

Problem code:

Changed code at pkg/runs/preparation.go:154-155

Recommendation:
补充覆盖完整往返与兼容性的测试:1) 构造一个包含 driver oneof、workspace、closed-sets 等字段的 protojson 序列化 spec,经 DecodeRevisionSpec 解码后重新 protojson.Marshal 再解码,断言 oneof 与其余字段在往返中保持(等价于 GetProject→PatchProject 场景);2) 覆盖 well-known 类型(若 ProjectSpec 含 Timestamp/Any 等)的 protojson 规范表示解码,以及非规范结构字段表示的报错/丢弃行为符合预期,防止解码器切换对既有存储数据造成静默数据丢失或运行中断。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant