fix(executor): validate writes before disk, wire rollback, emit validation_failed - #402
fix(executor): validate writes before disk, wire rollback, emit validation_failed#402ceilf6 wants to merge 2 commits into
Conversation
…ation_failed 三个缺陷各自的修法： 1. 坏内容先落盘再校验（#387 的直接后果） 在 call_tool 之前新增 validate_content 阶段：resolveWriteContent 解析 「写盘前即可确定的完整内容」（create_file 有 content；apply_patch 只给 补丁片段，故返回 null 走原后置路径），能解析时先跑 guard.validateCode， 不过则直接返回、根本不调用写工具。validateAfterExecution 新增 contentValidatedBeforeWrite 参数，前置已校验过的内容不再重复跑 guard。 2. needsRollback 恒为 false + 回滚从不执行（#387） needsRollback 原表达式为 `!pass && step.validation.some(v => v.required)`， 而 LLM 生成的计划里 validation 常为空数组，该条件几乎恒假；改为 `!postValidation.pass`。新增 rollbackFailedWrite：写后校验失败时按 toolResult.snapshotId 调 rollback 撤销落盘。 3. validation_failed 事件无发射点（#388） ExecutorConfig 新增 emitEvent 出口，agent.ts 接到 this.emit； 校验拦截（前置/写前/写后）与回滚各自上报事件。 注：SecurityManager 把 rollback 归类为「需审批」，非交互运行会拒绝， 自动回滚需 permissions.allow: ['rollback']——见 PR follow-up。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
代码评审报告: fix(executor): validate writes before disk, wire rollback, emit validation_failed
风险等级: 中
处理建议: 请求修改
决策摘要: 方向正确且执行路径上确实生效，但两条链路未达关联 issue 的验收口径：apply_patch（生产上几乎全部走 LLM 全文件替换）仍然先落盘后校验，#387 的首选修法只覆盖了 create_file；同时 validation_failed 现在也会在普通工具失败时触发，直接污染 #388 想要建立的「拦截可计数」指标。两处修复都很小，建议本轮改完再合。
级联分析
- 变更符号:
Executor.executeStep、Executor.validateAfterExecution（私有，新增带默认值参数）、新增私有resolveWriteContent/rollbackFailedWrite、ExecutorConfig.emitEvent（可选，additive）、ExecutorTraceStage.name新增'validate_content'（additive）、FrontAgent构造函数中的 executor 配置。 - 受影响流程: 步骤执行（pre-write 校验 → call_tool → post-write 校验 → 回滚）、agent 事件流、执行 trace 采集、进度强制引擎的中止判定。
- 变更集外调用方 (text search):
packages/core/src/executor/progress-enforcement.ts:42—needsRollback唯一消费者，语义被放宽后行为改变。packages/core/src/executor/phase-runner.ts— 真实 agent 路径（agent.ts:813→executeStepsWithErrorFeedback）根本不读needsRollback，所以放宽只影响Executor.executeSteps的使用者（如benchmarks/frontagent-flow-benchmark.mjs:206）。apps/desktop/src/state/executionReducer.ts:279/286/289— 三个事件的 handler 已存在，形状匹配，无需改动。benchmarks/eval/run-eval.mjs:97+benchmarks/eval/report.mjs:75— 按event.type计数并直接印在评测报告里，受下面第 1 条影响。packages/mcp-file/src/tools/create-file.ts:88/apply-patch.ts:136确实返回snapshotId，agent.ts:243已注册rollback工具映射，回滚链路可达。
- 置信度: medium（无代码图谱输出，结论来自 Read/Grep 全仓核查；私有方法签名变更与 additive 联合类型不构成外部破坏）
问题发现
-
[高]
validation_failed现在也覆盖普通工具失败，#388 想要的「拦截计数」失真- 证据:
executor.ts:228-231对任意!postValidation.pass发事件；而validateAfterExecution（executor.ts:614-623）在result.success === false时就返回pass:false, results:[], blockedBy:[工具报错]。于是run_command、read_file、create_file撞到「文件已存在」等普通失败都会发validation_failed。benchmarks/eval/run-eval.mjs:97按 type 计数、report.mjs:75把这个数字当作「校验是否起作用」的证据；桌面端executionReducer.ts:283会把工具报错渲染成「校验失败: ...」。 - 受影响调用方/流程: 评测报告的 guard 有效性口径、桌面 UI 日志、后续重跑 benchmark 的结论。
- 最小可行修复: 只在失败确由 guard 产生时发事件（例如判
postValidation.results.length > 0，或让validateAfterExecution区分「工具失败」与「guard 拦截」两种返回并只对后者上报）。issue #388 还要求带 check type/path，当前 payload 只有ValidationResult，可顺带确认是否足够。
- 证据:
-
[高]
apply_patch仍是先落盘后校验，#387 的首选修法只覆盖 create 路径- 证据:
resolveWriteContent（executor.ts:564-584）只认toolParams.content，而apply_patch的内容在patches[0].content。生产上这条路几乎全部走 LLM 生成：planner.ts:465固定发patches: []（注释「补丁由 Executor 生成」），提示词plan-generation.ts:423要求 apply_patch 用changeDescription + needsCodeGeneration，executor-skills.ts:278-288随后生成{operation:'replace', startLine:1, endLine:原文件行数, content: 全文件}——这正是「写盘前已完全可知的完整文件内容」，也正是 markdown 围栏幻觉出现的地方。结果：#387 证据里的.tsx围栏若来自修改类任务，仍会先写进磁盘，只能靠回滚兜底；而回滚在无approvalHandler的非交互 CLI 会被tool-call-handler.ts:187-196直接拒绝（评测侧因run-eval.mjs:94自动批准不受影响，PR 描述中「eval harness 会被安全层挡住」这点与仓库现状不符）。 - 受影响调用方/流程: 所有 modify/bugfix/refactor 任务的写盘路径；#387 的验收项「create/patch 步骤的非法内容不得留在磁盘上」未对 patch 成立。
- 最小可行修复: 在
resolveWriteContent中补一条窄分支——apply_patch且patches为单个整文件replace（startLine === 1且endLine >= 原文件行数，原文件可从collectedContext.files取）时返回该 content。附带收益：这条补上后，validateAfterExecution里的 post-write 内容校验分支与contentValidatedBeforeWrite这个 mode flag 都可以整体删掉，少一条并行校验路径。
- 证据:
-
[中]
needsRollback放宽的范围比 PR 描述更大，且无测试- 证据:
executor.ts:244现在对任何!postValidation.pass置 true，其中包含纯工具失败（见 finding 1 的同一分支），不只是「post-write 校验失败」。唯一消费者progress-enforcement.ts:42会把剩余步骤全标skipped并 break，因此在Executor.executeSteps路径上，一次read_file/run_command失败就会中止整个剩余计划。真实 agent 路径走PhaseRunner（不读该标志），所以爆炸半径限于executeSteps的直接使用者，但这与 PR 里「whenever a step fails post-write validation」的描述不一致。 - 受影响调用方/流程:
benchmarks/frontagent-flow-benchmark.mjs:206及任何以executeSteps为入口的嵌入方。 - 最小可行修复: 把标志收窄到校验类失败（与 finding 1 用同一判据），或保留现语义但在 PR 描述与测试中明确覆盖「普通工具失败也会中止后续步骤」。
- 证据:
-
[中] 回滚失败是静默的，UI 会停在「回滚开始」
- 证据:
executor.ts:599-605只在成功时发rollback_completed，失败仅debugWarn（debug关闭时完全无输出）。安全层拒绝时返回的是{success:false, error}，而这里读的是result.message，日志会打成undefined。桌面端已记了rollback_started（executionReducer.ts:287）却永远等不到终止事件。 - 受影响调用方/流程: 非交互运行的可观测性、桌面日志一致性——恰恰是本 PR 想解决的那类盲区。
- 最小可行修复: 失败时也上报一个终止事件（新增
rollback_failed，或复用现有告警事件），并同时读取message ?? error。
- 证据:
行级发现
- [packages/core/src/executor/executor.ts:229] 这里对任何
postValidation失败都发validation_failed，包含validateAfterExecution对纯工具失败返回的pass:false；建议加判据（如postValidation.results.length > 0）只上报 guard 拦截，否则评测计数与 UI 文案都会把工具报错算成校验拦截。 - [packages/core/src/executor/executor.ts:244]
needsRollback: !postValidation.pass把普通工具失败也纳入，progress-enforcement.ts:42会因此中止剩余全部步骤；建议与上一条用同一判据收窄到校验类失败。 - [packages/core/src/executor/executor.ts:577] 只从
toolParams.content取内容，导致apply_patch恒返回 null；而executor-skills.ts:278-288生成的是覆盖整文件的单个replacepatch，内容在写盘前已完全可知，建议补一条整文件 replace 分支，让 #387 的前置校验也覆盖修改路径。 - [packages/core/src/executor/executor.ts:604] 回滚失败只走
debugWarn，且安全层拒绝返回的是{success:false,error}（message为 undefined）；建议补一个终止事件并读取message ?? error，避免 UI 停在「回滚开始」。 - [packages/core/src/executor/executor.test.ts:794] 回滚用例全程 mock
callTool，只断言调用参数，没有验证磁盘上的文件被恢复；#387 的验收项要求的是「文件不留在磁盘上」，建议改成真实SnapshotManager+ 临时目录并断言文件内容已还原。
Karpathy 评审
- 假设: 隐含假设「
apply_patch的完整内容写盘前不可知」——在 codegen 路径上不成立（planner.ts:465+executor-skills.ts:288）。另一处隐含假设是postValidation失败等价于 guard 拦截，实际混入了工具失败。PR 描述中关于 eval harness 无审批通道的说法与run-eval.mjs:94不符。 - 简洁性:
WRITE_ACTIONS常量、resolveWriteContent、rollbackFailedWrite拆分合理，命名与既有风格一致，没有多余抽象。contentValidatedBeforeWrite是一个为兼容旧路径而存在的 mode flag；若 finding 2 落地，这个 flag 与 post-write 内容校验分支可整段删除，属于能真正减少概念数的简化。 - 结构质量: 无错误分层、无重复 helper、无文件规模跨界（
executor.ts847 行）；trace 阶段与配置字段均为 additive，对下游无破坏。 - 变更范围: 5 个文件都服务于三个既定缺陷，无无关重构或格式噪声。范围本身克制，问题在于覆盖不足而非过宽。
- 验证: 新增用例针对性强（pre-write 用例使用真实 guard + 真实失败样本 + 断言磁盘无文件，这点很好）。但回滚用例完全 mock，且测试自身注释（
executor.test.ts:781）承认刻意绕开了 LLM codegen 分支——而那正是生产上 apply_patch 的主路径。
缺失覆盖
apply_patch走 codegen 全文件替换、内容非法时的端到端用例：断言磁盘上不留下（或已还原）非法文件，这是 #387 明确要求的回归测试且当前 patch 路径未覆盖。- 「普通工具失败不应发
validation_failed」的负向断言（配合 finding 1 的修复）。 Executor.executeSteps路径下，一次非校验类工具失败对剩余步骤的影响（finding 3 的行为变更目前无任何测试锁定）。- 非交互配置（无
approvalHandler、无permissions.allow: ['rollback']）下回滚被安全层拒绝时的事件与日志行为。
| ); | ||
|
|
||
| if (!postValidation.pass) { | ||
| this.config.emitEvent?.({ type: 'validation_failed', result: postValidation }); |
There was a problem hiding this comment.
这里对任何 postValidation 失败都发 validation_failed，包含 validateAfterExecution 对纯工具失败返回的 pass:false；建议加判据（如 postValidation.results.length > 0）只上报 guard 拦截，否则评测计数与 UI 文案都会把工具报错算成校验拦截。
| stepResult, | ||
| validation: postValidation, | ||
| needsRollback: !postValidation.pass && step.validation.some((v) => v.required), | ||
| needsRollback: !postValidation.pass, |
There was a problem hiding this comment.
needsRollback: !postValidation.pass 把普通工具失败也纳入，progress-enforcement.ts:42 会因此中止剩余全部步骤；建议与上一条用同一判据收窄到校验类失败。
| } | ||
|
|
||
| const path = (toolParams.path ?? step.params.path) as string | undefined; | ||
| const content = (toolParams.content ?? step.params.content) as string | undefined; |
There was a problem hiding this comment.
只从 toolParams.content 取内容，导致 apply_patch 恒返回 null；而 executor-skills.ts:278-288 生成的是覆盖整文件的单个 replace patch，内容在写盘前已完全可知，建议补一条整文件 replace 分支，让 #387 的前置校验也覆盖修改路径。
| if (result.success) { | ||
| this.config.emitEvent?.({ type: 'rollback_completed', snapshotId }); | ||
| } else { | ||
| this.debugWarn(`[Executor] Rollback failed for snapshot ${snapshotId}: ${result.message}`); |
There was a problem hiding this comment.
回滚失败只走 debugWarn，且安全层拒绝返回的是 {success:false,error}（message 为 undefined）；建议补一个终止事件并读取 message ?? error，避免 UI 停在「回滚开始」。
| }), | ||
| ); | ||
|
|
||
| expect(callTool).toHaveBeenCalledWith('rollback', { snapshotId: 'snap-1' }); |
There was a problem hiding this comment.
回滚用例全程 mock callTool，只断言调用参数，没有验证磁盘上的文件被恢复；#387 的验收项要求的是「文件不留在磁盘上」，建议改成真实 SnapshotManager + 临时目录并断言文件内容已还原。
|
Follow-ups from this PR are now filed as issues:
Both are the same defect class this PR addresses for Still open from the PR description and not filed as an issue, because it is a policy decision rather than a defect: |
|
Correction to my earlier follow-up comment: #405 was a duplicate of #400 (which predates it and already covers Revised follow-up list for this PR:
The #406 finding matters for how this PR should be read: the executor plumbing here blocks writes that the guard flags, and it demonstrably does so (see the before/after probe in the benchmark report PR). But when the checker itself passes the content, the write still lands. Interception rate is now gated on checker strength, not on the executor. |
validation_failed 此前复合了三个来源：执行前结构性拦截、写盘前内容拦截， 以及写盘后校验失败——而最后一类包含纯工具失败，因为 validateAfterExecution 在 `resultObj.success === false` 时返回的是 results 为空的失败结果。 事件名为「校验失败」却把工具失败计入，任何人拿它当拦截数都会被误导。 改为只在「至少一项真实检查判失败」（results 中存在 pass:false 项）时 emit， 指标按构造即为纯拦截信号，不必事后按 results[].type 过滤。 回滚仍只看 !postValidation.pass：工具失败但已产生快照时同样要撤销落盘。 补测试：工具失败（EACCES）不产生 validation_failed。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed The first version of this PR had a metric defect I want to name explicitly, because it would mislead anyone reading the number. The event was emitted from three places, and the third was not an interception at all:
So an Fix: emit only when at least one real check reports New test: Re-verified end to end after the change, through the real |
There was a problem hiding this comment.
代码评审报告: fix(executor): validate writes before disk, wire rollback, emit validation_failed
风险等级: 高
处理建议: 请求修改
决策摘要: 方向正确且证据扎实，但前置校验把 guard 的 block 判定升级成了"写盘否决权"——其中 import_validity 对"后续步骤才创建的相对导入"必然判 block，会让本来能落盘的文件彻底不写；同时 apply_patch（真实计划里全部 modify 步骤）没有拿到任何前置保护，#387 在默认无审批配置下未闭环。建议收窄前置门禁的检查范围并补上 patch 路径后再合。
级联分析
- 变更符号:
Executor.executeStep、Executor.validateAfterExecution（签名 +1 参数）、新增resolveWriteContent/emitValidationFailed/rollbackFailedWrite、ExecutorConfig.emitEvent、ExecutorTraceStage.name联合类型、FrontAgent构造。 - 受影响流程:
executeStep → validate_content → call_tool；Executor.executeSteps → progress-enforcement；phase-runner的失败/恢复回路；agent 事件流 →apps/desktop/src/state/executionReducer.ts；benchmarks/eval/report.mjs对validation_failed的计数口径。 - 变更集外调用方:
packages/core/src/executor/progress-enforcement.ts:42（needsRollback唯一消费者，语义被动过但文件未改）、apps/desktop/src/state/executionReducer.ts:279-290（validation_failed/rollback_started/rollback_completed三个 case 现在首次会真的收到事件）、packages/core/src/skills/executor-skills.ts:278-288（apply_patch 代码生成路径）、packages/core/src/llm/schemas.ts:19-37（决定toolParams实际能带哪些字段）。 - 置信度: medium（本地只读代码 + 文本检索确认调用方，未自行跑 GitNexus；PR 提供的图谱结果与我读到的调用关系一致）
问题发现
-
[高] 前置门禁把
import_validity的 block 判定变成了"不写盘"，会吞掉合法文件- 证据:
executor.ts:181-205调用的是完整guard.validateCode，而validateCode会跑checkAllImports（packages/hallucination-guard/src/guard.ts:196-203）；相对导入解析不到时返回severity: 'block'（packages/hallucination-guard/src/checks/import-validity.ts:84-90），非.//开头的路径别名（@/components/x）会走checkPackageExists判成"未安装的包"（同文件 36-53、96-126）。改动前这些判定发生在写盘后：step 标记失败，但文件仍在磁盘上，等后续步骤把被导入模块建出来后仍可自洽；改动后该文件根本不存在。 - 受影响调用方/流程: 多文件计划中"先建页面、后建组件"的
create_file步骤；随后phase-runner.ts:271-275记录 phaseErrors →runPhaseRecovery。恢复步骤若是apply_patch，会命中Cannot apply patch: file does not exist，而该错误在executor-skills.ts:291-298被判为可跳过 → 记为成功；onPhaseComplete的tsc --noEmit（agent/phase-checks.ts:99-105）也看不到一个"根本不存在的文件"的导入错误，于是phase-runner.ts:369-376会把原本 failed 的步骤翻成 completed。净效果：文件没写、run 报成功。 - 最小可行修复: 前置阶段只用写盘前可靠的检查（
syntax_validity）做否决，import_validity结果保留为非阻塞或仍留在写盘后判定；若要保留 import 门禁，用已有的config.getCreatedModules?.()（executor/types.ts:23）把"计划内待建模块"排除出 block 集合。并补一条回归测试：内容引用同计划后续步骤才创建的相对模块时，文件必须照常写入。
- 证据:
-
[高]
apply_patch全程没有前置保护，#387 的 patch 侧验收未闭环- 证据: PR 描述称"apply_patch 只带补丁片段所以返回 null"，但
packages/core/src/llm/schemas.ts:19-37的STEP_PARAMS_SCHEMA既没有content也没有patches字段，因此 LLM 计划里的 apply_patch 步骤必然走executor-skills.ts:226-289的代码生成分支，产出的是覆盖全文件的单个 replace 补丁（startLine: 1, endLine: originalLines, content: modifiedCode）——写盘后的完整内容在写之前就是已知的。而resolveWriteContent（executor.ts:573-584）只认toolParams.content，对该路径恒返回null。 - 受影响调用方/流程: 所有 modify 类任务。它们只能落到写盘后校验 +
rollbackFailedWrite，而rollback在security.ts:283-292被判为ask/high，非交互运行直接拒绝（PR follow-up 1 已自述）。issue #387 明确要求"a create/patch step whose content is syntactically invalid must not leave the file on disk"，patch 侧在默认配置下仍会留下坏文件——正是评测所处的 headless 配置。 - 最小可行修复: 在
resolveWriteContent里识别"单个 replace 且覆盖collectedContext.files.get(path)全部行"的补丁，直接用patch.content作为前置校验内容；其余补丁形态继续返回 null。
- 证据: PR 描述称"apply_patch 只带补丁片段所以返回 null"，但
-
[中]
needsRollback现在等价于"步骤失败"，语义与字段名脱节- 证据:
validateAfterExecution（executor.ts:626-635）对任何result.success === false都返回pass:false，因此executor.ts:245的!postValidation.pass对EACCES、命令失败等纯工具错误同样为 true——与!stepResult.success完全同义；同时写盘失败时rollbackFailedWrite已经在executor.ts:228-232尝试过回滚，标志位却仍报"需要回滚"。PR 描述把影响表述为"post-write validation 失败时"，实际范围是所有失败。 - 受影响调用方/流程:
progress-enforcement.ts:42（Executor.executeSteps这一公开 API：任何一步失败即把余下步骤全部置skipped）。主链路走executeStepsWithErrorFeedback/phase-runner，不读该标志，所以生产影响有限，但 SDK 消费者的 run shape 变了，且变化面比 PR 描述更宽。 - 最小可行修复: 让该标志表达"有内容已落盘且未被成功撤销"（结合
snapshotId与rollbackFailedWrite的返回值），或明确删除它并让消费者直接读stepResult.success；同时在 PR 描述里更正影响范围。
- 证据:
-
[中] 回滚失败没有任何终态信号，恰好在最需要观测的 headless 场景失声
- 证据:
executor.ts:611-617先发rollback_started，成功才发rollback_completed，失败只走debugWarn。AgentEvent联合（packages/core/src/types.ts:793-795）没有失败态；apps/desktop/src/state/executionReducer.ts:286-290会留下一条永远等不到收尾的"回滚开始"日志。默认非交互配置下rollback被安全层拒绝，正是这条静默分支。 - 受影响调用方/流程: 桌面端执行日志；
benchmarks/eval/report.mjs:74-75依赖事件计数评估 guard 有效性——"拦截到了但没能撤销"与"撤销成功"不可区分。 - 最小可行修复: 补一个终态（扩
AgentEvent加rollback_failed，或至少把回滚失败原因追加进stepResult.error），使"坏文件是否仍在磁盘上"可判定。
- 证据:
-
[中] 纯工具失败也会触发回滚，交互式运行会多出审批弹窗
- 证据:
executor.ts:228-232在postValidation.pass === false时无条件调用rollbackFailedWrite，而该分支包含工具自身报错（executor.ts:626-635）。只要工具返回了snapshotId，就会发起一次rollback，在注册了approvalHandler的桌面/CLI 交互模式下触发snapshot_rollback_requires_approval审批（security.ts:283-292）。行内注释说明这是有意的，但这属于用户可见行为变化，PR 描述未覆盖。 - 受影响调用方/流程: 桌面端与交互式 CLI 的失败路径。
- 最小可行修复: 要么把执行器发起的补偿性回滚单独归类（PR 已把它列为 maintainer 决策，可先只在校验驱动的失败上触发），要么在描述与文档里显式说明新增审批点。
- 证据:
行级发现
- [packages/core/src/executor/executor.ts:183] 这里用完整
validateCode做写盘否决，import_validity的 block 判定（含"后续步骤才创建的相对导入"和路径别名）会导致文件根本不落盘；前置门禁建议只依据syntax_validity，import 检查保留在写盘后或用getCreatedModules()排除计划内模块。 - [packages/core/src/executor/executor.ts:573]
WRITE_ACTIONS含apply_patch，但计划 schema 不产出content，apply_patch 恒返回 null；应识别"覆盖全文件的单个 replace 补丁"并取其content，否则所有 modify 步骤没有前置保护。 - [packages/core/src/executor/executor.ts:245]
!postValidation.pass对纯工具错误同样为 true，使该字段与!stepResult.success同义且与"已在上方尝试过回滚"矛盾；建议收窄为"有落盘且未成功撤销"。 - [packages/core/src/executor/executor.ts:616] 回滚失败只
debugWarn，rollback_started没有终态；在安全层拒绝回滚的 headless 运行里这正是最需要上报的分支，建议补失败事件或把原因写进stepResult.error。 - [packages/core/src/executor/executor.ts:231] 工具自身失败也会发起
rollback，交互模式下会新增一次审批弹窗；请确认该行为并在描述中说明，或只在校验驱动的失败上触发。
Karpathy 评审
- 假设: "apply_patch 只携带补丁片段"这一核心假设与实际实现不符（skill 生成的是全文件 replace），据此得出的"无法前置校验"结论不成立；另一处隐含假设是"guard 的 block 判定足够可靠到可以否决写入"，但 import 检查存在已知误报类别。
- 简洁性: 三个新私有方法职责清晰、体量合适；
validate_content阶段对所有 step（含 read_file）都会记一条空 stage，属可接受的噪声。emitValidationFailed用results.some(r => !r.pass)区分"校验拦截"与"工具失败"是隐式契约，但注释已写明，可接受。 - 结构质量: 无明显退化，没有跨层泄漏或薄 wrapper；
emitEvent作为可选出口与既有onSecurityDecision模式一致。 - 变更范围: 聚焦，
agent.ts仅 3 行接线，无无关重构。 - 验证: 3 个新执行器测试 + 1 个 agent 契约测试方向正确，但只覆盖真阳性；两个关键测试 mock 了 guard，回滚测试还额外授予
permissions.allow: ['rollback']，因此验证的是接线而非默认配置下的结果。新门禁的假阳性代价（合法文件不落盘）完全没有测试。
缺失覆盖
create_file内容引用"同计划后续步骤才创建"的相对模块时，文件必须照常写入（针对发现 1 的回归测试）。- 路径别名导入（如
@/components/Card）不应触发写盘否决。 apply_patch步骤内容语法无效时，磁盘上不应留下坏文件——按 issue #387 的验收口径断言文件状态，而不只断言callTool('rollback', ...)被调用。- 默认（未授予
permissions.allow: ['rollback']）非交互配置下回滚被拒的路径：断言失败可观测，且不会出现只有rollback_started的悬空事件序列。 Executor.executeSteps在纯工具失败（非校验失败）时是否应中止余下步骤——progress-enforcement的新语义目前无测试。
| const writeContent = this.resolveWriteContent(step, toolParams); | ||
| const contentValidation = await trace.withStage('validate_content', () => | ||
| writeContent | ||
| ? this.config.hallucinationGuard.validateCode( |
There was a problem hiding this comment.
这里用完整 validateCode 做写盘否决，import_validity 的 block 判定（含"后续步骤才创建的相对导入"和路径别名）会导致文件根本不落盘；前置门禁建议只依据 syntax_validity，import 检查保留在写盘后或用 getCreatedModules() 排除计划内模块。
| content: string; | ||
| language: 'typescript' | 'javascript' | 'json' | 'yaml'; | ||
| } | null { | ||
| if (!WRITE_ACTIONS.includes(step.action)) { |
There was a problem hiding this comment.
WRITE_ACTIONS 含 apply_patch，但计划 schema 不产出 content，apply_patch 恒返回 null；应识别"覆盖全文件的单个 replace 补丁"并取其 content，否则所有 modify 步骤没有前置保护。
| stepResult, | ||
| validation: postValidation, | ||
| needsRollback: !postValidation.pass && step.validation.some((v) => v.required), | ||
| needsRollback: !postValidation.pass, |
There was a problem hiding this comment.
!postValidation.pass 对纯工具错误同样为 true，使该字段与 !stepResult.success 同义且与"已在上方尝试过回滚"矛盾；建议收窄为"有落盘且未成功撤销"。
| if (result.success) { | ||
| this.config.emitEvent?.({ type: 'rollback_completed', snapshotId }); | ||
| } else { | ||
| this.debugWarn(`[Executor] Rollback failed for snapshot ${snapshotId}: ${result.message}`); |
There was a problem hiding this comment.
回滚失败只 debugWarn，rollback_started 没有终态；在安全层拒绝回滚的 headless 运行里这正是最需要上报的分支，建议补失败事件或把原因写进 stepResult.error。
| if (!postValidation.pass) { | ||
| this.emitValidationFailed(postValidation); | ||
| // 回滚不看事件口径：工具失败但已产生快照时同样要把落盘撤销 | ||
| await this.rollbackFailedWrite(toolResult); |
There was a problem hiding this comment.
工具自身失败也会发起 rollback，交互模式下会新增一次审批弹窗；请确认该行为并在描述中说明，或只在校验驱动的失败上触发。
1 participant
Linked Issue Or Context
validation_failednever fires — guard effectiveness unobservable)Summary
Three defects on the executor's validation path, each with its own fix.
1. Bad content reached disk before it was ever validated.
Validation of generated code only ran in
validateAfterExecution, i.e. after thewrite tool had already run. A new
validate_contenttrace stage now runsbefore
call_tool:resolveWriteContent(step, toolParams)resolves the"complete file content knowable before the write" (
create_filecarriescontent;apply_patchonly carries patch fragments, so it returnsnullandkeeps the old post-write path). When content is resolvable,
guard.validateCoderuns and a failure returns immediately — the write tool is never invoked, so
the bad file never lands.
validateAfterExecutiontakes a newcontentValidatedBeforeWriteargument so pre-validated content is not runthrough the guard a second time (previously every write would have paid for two
guard passes).
2.
needsRollbackwas effectively always false, and nothing ever rolled back.The old expression was
!postValidation.pass && step.validation.some((v) => v.required). LLM-generatedplans routinely emit
validation: [], so the second conjunct was almost alwaysfalse — this is the actual mechanism behind #387. It is now
!postValidation.pass. Separately, a newrollbackFailedWrite(toolResult)reverts a written patch via
toolResult.snapshotIdwhen post-write validationfails.
3.
validation_failedhad no emit site anywhere in the repo.ExecutorConfiggains anemitEventoutlet;agent.tswires it tothis.emit. Validation blocks (pre-execution, pre-write, post-write) androllback now report
validation_failed/rollback_started/rollback_completed.Verified facts (checked against the tree, not assumed)
guard.tshonoursenabledChecksinboth
validateFilePathandvalidateCode. Residual gap: the executornever calls the guard's integrated entry point
validate(); it only callsvalidateFilePath/validateCodeindividually.guard.validate()has zeroproduction callers (only
guard.test.ts), and thesddCompliancecheck livesonly inside
validate()— so SDD compliance checking does not run on thereal execution path at all. Not addressed here; worth its own issue.
default":
needsRollbackhas exactly one consumer in the whole repo(
progress-enforcement.ts:42), and that consumer only marks remaining stepsskippedand breaks — it never rolls anything back.phase-runner.tsdoes notread the flag at all.
Executor.rollback()had zero callers. Rollback wasdead code in both execution engines.
validation_failedappeared only in theAgentEventunion intypes.tsand in acase 'validation_failed'branch inapps/desktop/src/state/executionReducer.ts— the desktop UI has a handler foran event nothing could ever emit.
finding "multi-layer validation never intercepted anything" is likely, in part,
an observability artefact. Validation may well have been firing all along
as ordinary step failures; the event was simply never emitted and the bad file
was left on disk, so it looked like nothing was intercepted. The real numbers
are unknown until the suite is re-run — this PR does not claim a benchmark
delta.
Behavioural change worth reviewer attention
Broadening
needsRollbackmeans more runs now abort their remaining steps: thesingle consumer aborts the phase whenever a step fails post-write validation,
where previously plans with empty
validationarrays continued. This isintended (do not keep building on a file that failed validation), but it is a
real change in run shape, not just a bug fix.
Follow-ups (not in this PR)
SecurityManagerclassifiesrollbackasask/ high risk, andenforceSecuritydenies anyaskwhensecurity.interactiveis false or noapprovalHandleris registered — which is exactly how the eval harness andnon-interactive CLI runs operate. So the newly wired rollback only actually
executes when the user supplies
permissions.allow: ['rollback'](theexisting documented escape hatch, which the new test uses). Deciding whether
an executor-initiated compensating rollback should be classified separately
from a user-requested rollback of an arbitrary snapshot is a maintainer call,
so it is deliberately not changed here.
packages/hallucination-guardis regex/heuristic based.The package intentionally keeps zero runtime dependencies for CLI
distribution, so swapping in a real parser (
@babel/parseror similar) is amaintainer-level decision and was not attempted.
Impact Scope
packages/core/src/executor/executor.ts— newvalidate_contentstage,resolveWriteContent,rollbackFailedWrite,needsRollbacksemantics,validateAfterExecutionsignature.packages/core/src/executor/types.ts—ExecutorConfig.emitEvent,'validate_content'trace stage.packages/core/src/agent/agent.ts— wiresemitEventinto the agent event stream.executor.test.ts(+3),agent.test.ts(+1 contract test).emitEventis optional.GitNexus Impact Summary
agent-core(packages/core/src/agent/agent.ts), with a matching contract test added underpackages/core/src/agent/.detect_changes --scope compare --base-ref developreports 6 files / 17 symbols / 6 affected execution flows at high risk;impact Executor.executeStep --direction upstreamreturns impactedCount 1 at LOW risk, andimpact Executor.rollback --direction upstreamreturns impactedCount 3 where it previously had zero callers. Breakdown below.pnpm quality:precommitgreen;pnpm quality:local(incl.build:verify) green via the pre-push hook.Breakdown:
detect_changes --scope compare --base-ref develop: 6 files, 17 symbols,6 affected execution flows, reported risk high (driven by
ExecuteStep → *andConstructor → *flows).impact Executor.executeStep --direction upstream: impactedCount 1,risk LOW, 1 process / 1 module affected.
impact Executor.validateAfterExecution --direction upstream:impactedCount 2, risk LOW (depth-1
executeStep).impact Executor.rollback --direction upstream: impactedCount 3,risk LOW — previously this symbol had no callers at all; the new
rollbackFailedWriteis what puts it on a live path.Verification
npx vitest run packages/core/src/executor/executor.test.ts— 31 passed(3 new: pre-write block, single-validation path, rollback-on-failure).
npx vitest run packages/core/src/agent/agent.test.ts— 16 passed(1 new: executor
validation_failedsurfaces on the agent event stream).pnpm quality:precommit— exit 0; turbo 25/25 tasks successful;test:workflows34/34 pass.pnpm quality:local— ran automatically on pre-push, includingbuild:verify(14/14 turbo build tasks, CLI + VS Code bundles OK).HallucinationGuardplus a genuinefailure sample from the evaluation set (a markdown fence written into a
.tsx, which producedTS1127), and asserts the write tool was never calledand no file exists on disk.
Environment notes
npx gitnexus analyzeinitially failed(
COPY failed for File: Binder exception … extension is not loaded), and theCLI was resolving queries against a different repo because FrontAgent was
not in the GitNexus registry. Fixed with
npx gitnexus index .; after thatcontract:localre-indexed cleanly (5,742 nodes / 13,801 edges) and allGitNexus results above are from that fresh index.
pnpm agent:bootstrapcould not run: corepack times out fetching pnpm 9.0.0from registry.npmjs.org. Not a repository problem; the local pnpm 9.0.0 was
used directly.
.gitnexus/lbugand.gitnexus/meta.jsonare tracked in git and get rewrittenby any local
gitnexus analyze. They were deliberately kept out of thiscommit; only source files are staged.
Checklist
pnpm quality:precommit, or explained why it could not run.pnpm quality:localfor critical skeleton changes, or explained why it could not run.