fix(ci): accept an amended protocol compatibility declaration - #4944
Conversation
The staged guard read declarations with --diff-filter=A against HEAD, so a later commit on the same branch could only declare its protocol files by adding a second declaration file. The merge-result check is unchanged and still counts only declarations added against the base. Generated-by: Claude Code
d3e7091 to
21b476d
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
复核 21b476dd364ea410f1f93414e57b6d6b3dd0bfc5,未发现有充分证据支持的 P0–P3 问题。
实际改动是 staged 检查接受新增或修改的 JSON 兼容声明,并共用声明校验及更具体的错误说明。scripts/protocol-epoch-check.mjs:198 使用 AM,:162 的整分支检查仍只接受相对 base 新增的声明;.husky/pre-commit 调用 staged 路径,.github/workflows/ci.yml:106 对合并结果的第一父提交执行最终检查。声明目录的 README 不再作为 JSON 解析。
17/17 回归、脚本 lint/format 和 diff 检查通过。用两个真实 Git 提交的夹具对比原脚本,排除 README 干扰后,原脚本在修订已有声明以覆盖第二个协议文件时拒绝,新脚本接受;整分支回归仍拒绝修改已落入 base 的声明来豁免新文件。schema 校验继续限制键集合、epoch、非空理由及协议路径。
边界需要明确:本地 hook 现在可能接受对已落地声明的修改,但 CI 仍拒绝;理由字符串是否真的代表兼容性仍由人审,脚本没有证明 wire 等价。本次没有改握手或运行时兼容 epoch。当前 head 的 hosted test 成功;与 main b06eb02e6 合并树无冲突,未在合并树上重跑测试。
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the change. Approving exact head 21b476dd364ea410f1f93414e57b6d6b3dd0bfc5 following the completed review and Astro-Han’s explicit acceptance of this merge batch. Current checks pass and no review threads remain unresolved.
A newly added compatibility declaration became invisible to staged validation after a later edit. Accept added or modified declarations in the staged path while retaining added-only validation against the target base. Shared parsing retains the existing human-reviewed compatibility contract.
AI assistance: Codex performed the review and final-state verification; Astro-Han authorized approval and merge.
中文
感谢改动。基于已完成的审查和 Astro-Han 对本批次的明确认可,批准当前精确 head;检查通过,讨论已结清。此前说明的验证边界与后续事项保持不变。本次由 Codex 执行审查和状态核对,Astro-Han 授权批准与合并。
Summary
A branch that touches protocol files across more than one commit cannot declare
them in the declaration it already added.
evaluateStagedEpochCheckreadsdeclarations with
--diff-filter=AagainstHEAD, so amending the existing fileto name the newly touched protocol file is a modification, not an addition, and
the pre-commit guard rejects the commit. The only way through is a second
declaration file for the same change — which is what the last few PRs to use this
path ended up carrying.
--diff-filter=AMin the staged path fixes it. The merge-result path keepsAagainst the base, so a declaration that already landed on the base branch still
grants no exemption, whatever a branch edits into it.
Two things came out of the same reading, both in the exemption path this fixes:
files filled in, and the eight distinct ways a declaration can be invalid no
longer share one
Invalid compatible protocol change declaration:string. Astale
epoch— the common one, since the epoch moved 25 times in the last fivedays — now says to re-pin it and re-read the reason against the protocol as it
now stands. The duplicated validation block the two modes each carried is now
one
parseDeclaration.crashed the guard with a JSON parse error on its own directory. Only
.jsonfiles are declarations now.
No change to what the guard admits: every protocol change still needs an epoch
the base has not seen, or a declaration added on the branch that names it.
Verification
node --test scripts/protocol-epoch-check.test.mjs(17 pass),biome lint,biome format,npm run check:asf-headers. Each new test fails against thecurrent script.
A branch that touches
first.tsin one commit andsecond.tsin the next,amending the one declaration to cover both:
The failure a contributor meets when the change is genuinely compatible:
Review focus
The only semantic change is
A->AMin the staged path.refuses a landed declaration edited to cover a new protocol filecovers the casethat would have to break for this to weaken the guard: on the merge result, editing
a declaration that already landed on the base yields no exempt files and the change
is still refused.
That does widen one gap between the two modes, in the safe direction. A branch that
edits a declaration already on the base and re-pins its epoch now satisfies
pre-commit, where before the file it named stayed undeclared and the commit was
refused; the merge result refuses it either way, so the author learns it in CI
rather than at commit time. Closing it locally would mean teaching the pre-commit
hook to resolve the base branch, which is the one thing a fast local hook should
not need. The README states the rule the hook cannot enforce: a landed declaration
is history, and a branch declares by adding its own.
AI use
Select exactly one:
Tool(s) and scope: Claude Code wrote the script change, the tests and the README.
Reviewed and verified by the author.
Checklist
Does this PR entail a change in behavior?