fix(upgrade): parse Codex App heartbeat TOML - #3913
Conversation
1fcbb61 to
a216497
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
[P1] Standard TOML parsing exposes an unescaped writer path that silently drops valid heartbeat prompts
Exact head: a2164970f9d0286732f1da1fac890c25bf9bbd44
parse_automation_toml() now correctly uses tomllib, but the paired production writer _write_automation_toml() still interpolates the prompt directly into a TOML multiline basic string. Backslashes in basic strings are escape introducers. A legitimate generated prompt containing text such as C:\Users\alice therefore produces TOML that tomllib rejects (\U is not a valid TOML escape). load_codex_app_automation_manifest() catches that decode error and silently skips the file, so the installed heartbeat disappears from upgrade inspection and RRULE resolution. I reproduced this on the exact head by writing such a prompt through _write_automation_toml() and reading it through load_codex_app_automation_manifest(); the manifest returned zero entries. The new round-trip test only uses TOML-insensitive text, so it misses this writer/reader mismatch.
Minimum repair: serialize prompt as a real TOML string (or correctly escape backslashes and every other basic-string-sensitive character before interpolation), then add a writer-to-reader regression containing backslashes/escape-like content and assert exact prompt digest, character count, line count, agent/capability inference, and RRULE resolution. The reader should continue to fail closed on genuinely malformed external TOML.
动机
这个 PR 要修复 Codex App heartbeat automation.toml 的多行 prompt 被逐行解析截断的问题。旧读取器只保留 prompt = """... 的第一行,使 prompt digest/line count、goal/agent 推断、capability 提示以及 RRULE/thread 匹配都可能错误,最终让 upgrade 诊断或 scheduler fallback 把已安装的 heartbeat 报成 stale/unavailable。用标准库 TOML parser 替换手写 parser 是正确方向,也是相比继续扩展逐行 parser 更小、更可维护的修复。
改动思路
parse_automation_toml() 改用 Python 3.11+ 自带的 tomllib.loads() 读取完整文件,从而正确恢复 multiline basic string、布尔值和整数。load_codex_app_automation_manifest() 将 UnicodeError 和 TOMLDecodeError 与原有 OSError 一样视为单文件失败,跳过损坏/不可读配置而不阻断其他 automation 的 upgrade 检查。测试通过实际 writer 写入多行 prompt,再由 manifest loader 和 RRULE resolver 读回,覆盖正向路径。
具体改动
loopx/upgrade.py:导入tomllib;将约 20 行手写 key/value parser 收缩为标准 TOML 解析;在 manifest 遍历中捕获文本编码和 TOML syntax 错误。现有resolve_codex_app_automation_rrule()仍以 goal/agent/thread 筛选唯一 active entry,未改公开回执 schema。tests/control_plane/test_scheduler_fallback_hint.py:新增 writer-to-reader 多行 prompt 回归,检查 goal/agent/thread、RRULE、digest、字符/行数和--available-capability提取,再验证 resolver 的精确回执。- 生产调用链:quota/scheduler follow-up CLI 通过
resolve_codex_app_automation_rrule()观察当前 Codex App automation;因此这不是孤立 upgrade helper,而是 ACK/fallback 路由的实际 host observation 输入。
关键代码讲解
parse_automation_toml(path):输入一个 automation 文件,现在按 TOML 规范一次解析全文;正常情况返回 typed mapping,语法错误由上层 manifest loader 隔离。load_codex_app_automation_manifest(root):遍历automation.toml,仅投影 active heartbeat 的公开字段和 prompt 派生摘要;单文件损坏时继续处理其他条目。问题正是 writer 生成的语法错误也走了这条“静默跳过”分支。resolve_codex_app_automation_rrule(...):从 manifest 按 goal/agent/thread 求唯一匹配,正向返回 RRULE/automation ID;若前置 parser 丢掉文件,它会返回“no matching active Codex App heartbeat RRULE”,后续 scheduler 只能进入 unavailable/fallback 路径。_write_automation_toml(...)(未改但是成对生产入口):直接把 prompt 嵌入prompt = """..."""。标准 reader 上线后,这个写法必须与 TOML escaping 规则对齐,否则 writer 自己就会制造被 reader 拒绝的文件。
对主干的风险
正向路径已验证:实际 writer 生成的普通多行 prompt 能被完整读回,digest/行数和 RRULE resolution 与原值一致。负向路径也能 fail closed:外部损坏 TOML 不会打断整个 upgrade inspection。但当前最强回归场景是“内置 writer 收到含 TOML escape 字符的合法 prompt”:这不是外部损坏输入,却会被新 reader 静默丢弃,影响 upgrade 信息和 scheduler host observation。最小恢复是修正 writer 的 TOML serialization 并增加对应回归,无需退回逐行 parser。
独立验证结果:聚焦测试 8 passed,Ruff 通过,compileall 通过,upgrade-plan-smoke.py 通过,git diff --check 通过,当前 GitHub 检查全绿。额外的 writer/reader 反例稳定返回 entry_count=0,证明正常测试集还没有覆盖实际序列化边界。
我的整体评价
把手写 parser 替换成 tomllib 的方向、变更规模和异常隔离策略都合理,也确实修复了普通多行 prompt 的主问题。变更只有两个文件,对原问题是成比例的,不涉及 default-off 或 actor authority 语义。但读端现在真正遵循 TOML 语义,写端却仍把任意 prompt 当作已转义的 basic string,使一类合法 heartbeat prompt 被静默丢弃。这个 writer-reader 契约缺口必须与 parser 切换一起修复;当前结论:REQUEST_CHANGES。
English verdict: REQUEST_CHANGES on exact head a2164970f9d0286732f1da1fac890c25bf9bbd44. Replacing the line parser with tomllib fixes ordinary multiline prompts and all focused/required checks pass, but the paired writer still embeds arbitrary prompt text into a TOML multiline basic string without escaping. A prompt containing C:\Users\alice is written successfully and then silently omitted by the new reader. Serialize the prompt correctly and add an exact round-trip regression for backslash/escape-like content before merging.
|
Independent cross-check of this PR, complementary to the maintainer's review on the same head — I verified its P1 is accurate and will not repeat it here. My focus is on two consequences of the reader switch that the writer-escaping repair alone does not cover, both reproduced on the exact head First, confirming the direction of the change: replacing the hand-rolled line parser with
Minor, folded into the maintainer's minimum repair as a concrete regression-input note: beyond backslashes, an embedded The scope and proportionality are right, and the focused tests are real. With the writer serialization fix from the maintainer's review plus visibility on what the stricter reader skips, this is a clean swap to the standard-library parser. |
Signed-off-by: yuefengw <60574042+yuefengw@users.noreply.github.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
a216497 to
58751f8
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Maintainer rereview on exact head 58751f85da08739bb6ecc8cced277b7e77d94401 after rebasing onto main/#3989.
Product and architecture verdict: APPROVE. The original move from a partial line parser to tomllib is correct. This refinement closes the paired storage-contract gap by serializing every writer string through a TOML-safe encoder, removes the duplicate regex prompt reader in the RRULE bridge, and makes malformed installed automation visible through bounded content-free diagnostics. Scheduler authority and the successful RRULE resolution envelope are unchanged. A TypeScript migration would be misplaced here because Codex App TOML/SQLite is a Python host adapter, not control-plane domain authority.
Review findings resolved:
- P1 writer escaping: real writer-to-reader coverage now includes
C:\Users\alice, embedded triple quotes, a line-ending backslash, multiline content, exact digest/counts, inferred agent/capability, thread binding, and RRULE resolution. - P2 silent skip: manifest, resolver failure, upgrade summary, and Markdown now carry a stable count, capped 20-item automation-ID/reason list, and completeness bit; invalid UTF-8 and invalid TOML are distinguished without exposing prompt content.
Validation: 18 focused Python tests; 20 Python-CI/Sonar workflow tests; configured Ruff scope; mypy over 21 source files; compileall; upgrade-plan smoke; TS typecheck; 614 TS tests with 613 passed and 1 skipped. Premerge direct checks and public-boundary scan passed; 8/9 catalog canaries passed. The remaining canary was only a local 120-second timeout: install-local-smoke.py passed directly in 149.74 seconds with no assertion failure. No manual holds.
Parallel-CI audit: the earlier 34 local full-suite failures predated the #3989 rebase and reproduce serially. They are local environment effects from a Git wrapper injecting core.hooksPath=/dev/null, system Python 3.9, and a live service on the fixed dashboard test port. The corrected affected module set reached 392 passes, with only that occupied-port branch remaining. #3989 hosted shards are green on main; no sharding change is justified in this PR.
|
@now-ing Both follow-up consequences are addressed on |
|
Final hosted readback on |
- 3f605c3 workspace stories + owner 任务可见性 (loopx-project#3990) - df5c557 codex-app TOML 往返修复 (loopx-project#3913) - 2f4c8f3 todo 延迟原生创建 (loopx-project#3980) 文档融合: repair-patterns.md 以上游英文表为源重建中文版 (184 旧行复用 + 新增 codex_app_automation_toml_contract_gap 手译)。 Co-Authored-By: Claude Code <noreply@anthropic.com>
Summary
Root cause and architecture
The old reader only retained the first line of a multiline prompt. Switching it to
tomllibis the correct fix, but it exposed a paired writer defect: arbitrary prompt text was interpolated directly into a TOML multiline basic string, so values such asC:\Users\alice, embedded""", or a trailing backslash could create TOML that the strict reader rejected. The manifest then silently treated the installed heartbeat as absent.This refinement treats the writer and reader as one host-storage contract. It keeps malformed files fail-closed, but reports only a bounded automation ID and stable reason taxonomy (
unreadable,invalid_utf8,invalid_toml); prompt contents and file contents remain private. This is intentionally a Python host-adapter repair rather than a TypeScript control-plane migration: Codex App TOML and SQLite are host-owned storage surfaces, while the typed scheduler authority remains unchanged.Validation
install-local-smoke.pyexceeded the local 120-second canary budget but passed directly in 149.74 seconds with no assertion failureParallel-CI investigation
The unrelated local full-suite failures were not introduced by the new sharding in #3989. The failing run started from the preceding main head, before #3989 was present, and representative failures reproduced serially. They were caused by local machine state:
/opt/homebrew/bin/gitis a wrapper that injectscore.hooksPath=/dev/null, systempython3is 3.9, and one dashboard test uses a fixed port already occupied by the local LoopX service. With a clean Git executable and the test interpreter, the affected parallel module set produced 392 passes; the only remaining failure was the occupied fixed-port branch. #3989 itself passed both hosted shards on main and is now included by rebase, so no CI-sharding change belongs in this PR.Closes the two review findings without changing scheduler authority, automation activation semantics, or the existing successful RRULE resolution envelope.