Skip to content

fix(ci): accept an amended protocol compatibility declaration - #4944

Merged
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/staged-epoch-guard-amended-declaration
Sep 7, 2026
Merged

fix(ci): accept an amended protocol compatibility declaration#4944
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/staged-epoch-guard-amended-declaration

Conversation

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor

Summary

A branch that touches protocol files across more than one commit cannot declare
them in the declaration it already added. evaluateStagedEpochCheck reads
declarations with --diff-filter=A against HEAD, so amending the existing file
to 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=AM in the staged path fixes it. The merge-result path keeps A
against 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:

  • The failure now prints a paste-ready declaration with the epoch and changed
    files filled in, and the eight distinct ways a declaration can be invalid no
    longer share one Invalid compatible protocol change declaration: string. A
    stale epoch — the common one, since the epoch moved 25 times in the last five
    days — 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.
  • The directory glob accepted any file, so the README added here would have
    crashed the guard with a JSON parse error on its own directory. Only .json
    files 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 the
current script.

A branch that touches first.ts in one commit and second.ts in the next,
amending the one declaration to cover both:

$ git commit -m 'commit 2: amend the declaration to cover second.ts'
Protocol epoch guard: Protocol files changed but RUNTIME_HOST_COMPATIBILITY_EPOCH
is still 121, the current base parent's value. [...] Changed files without a
compatible-change declaration:
  packages/runtime-host/src/protocol/second.ts

$ git commit -m 'commit 2: amend the declaration to cover second.ts'
Protocol epoch guard: Protocol added a declared compatible extension at epoch 121.

The failure a contributor meets when the change is genuinely compatible:

Protocol epoch guard: Protocol files changed but RUNTIME_HOST_COMPATIBILITY_EPOCH
is still 121 [...] Changed files without a compatible-change declaration:
  packages/runtime-host/src/protocol/codec.ts

Protocol epoch guard: Protocol files changed but RUNTIME_HOST_COMPATIBILITY_EPOCH
is still 121 [...] Changed files without a compatible-change declaration:
  packages/runtime-host/src/protocol/codec.ts

If the wire provably cannot observe this change, declare it instead of bumping:
add one packages/runtime-host/protocol-compatible-changes/<slug>.json, described
by the README in that directory, holding
{
  "epoch": 121,
  "files": [
    "packages/runtime-host/src/protocol/codec.ts"
  ],
  "reason": "<why the wire cannot observe this change>"
}

Review focus

The only semantic change is A -> AM in the staged path.
refuses a landed declaration edited to cover a new protocol file covers the case
that 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:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code wrote the script change, the tests and the README.
Reviewed and verified by the author.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 7, 2026
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
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/staged-epoch-guard-amended-declaration branch from d3e7091 to 21b476d Compare September 7, 2026 02:04

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

复核 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 授权批准与合并。

@Astro-Han
Astro-Han merged commit 73f61bb into apache:main Sep 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants