ci: guard the compatibility epoch against silent same-number merges - #3339
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for adding an interim guard at the existing CI seam. I reviewed exact head ea6f2aae18db64c63efa9b781d165588a00d7be9. The collision described in #3313 is real, and checking the synthetic merge result is the right direction. One P2 revision-selection issue currently prevents the guard from catching its primary case.
AI-assisted review disclosure: Codex performed the exact-head analysis, and an independent reviewer agent constructed and checked the sibling-branch Git history adversarially. I verified the live merge ref, base snapshot, source path, and final severity.
| - name: Guard the protocol compatibility epoch | ||
| if: github.event_name == 'pull_request' | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} |
There was a problem hiding this comment.
Thanks for putting this guard on the PR merge result. [P2] pull_request.base.sha is the PR’s older base snapshot, not the first parent of the checked-out synthetic merge. If sibling branches A and B both fork from epoch 27 and bump to 28, then A lands first, B is still evaluated as 27 → 28 and passes even though the current base and merged result are both 28. The live merge ref here already has a newer first parent than the event’s base snapshot. Could the guard compare against HEAD^1 and add a real Git-graph regression covering two sibling same-number bumps?
There was a problem hiding this comment.
Agreed. The guard now compares the checked-out synthetic merge with HEAD^1, while the affected-test planner keeps its existing event-base behavior. I also added a real sibling-branch graph where A and B both bump 27 to 28: the old fork-point comparison is shown to pass, while the current first-parent comparison rejects B and reports its protocol file. The guard tests (8/8), lint, and format pass. Updated in d4d0232. — Posted with Codex assistance.
Two branches that bump RUNTIME_HOST_COMPATIBILITY_EPOCH write the same text to the same line, so git merges them without a conflict and two incompatible protocols advertise one epoch. Add a merge-base guard that runs on the PR merge result and fails when protocol files changed while the epoch still equals the merge base's, or when the epoch moves backward. This is the interim check from apache#3313; the derive-the-epoch question stays open for the dev list. Refs apache#3313 Generated-by: Claude Code (Fable 5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use the synthetic merge's current-base parent instead of the PR's stale base snapshot, and cover two sibling same-number bumps with a real Git graph. Generated-by: Codex
ea6f2aa to
d4d0232
Compare
|
The review fix is pushed at d4d0232. GitHub created the new CI run with action_required and no jobs, so it needs maintainer workflow approval before checks can start: https://github.com/apache/maka/actions/runs/32482323535. The epoch guard, real Git-graph regression, CI policy tests, Windows inventory, lint, and format are green locally. — Posted with Codex assistance. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tightening the guard around the actual synthetic merge. Exact head d4d02321e now compares the checked-out merge result against HEAD^1, so a sibling protocol change that already landed on the current base cannot be hidden by the PR’s older base snapshot. The real Git-graph regression reproduces the two sibling 27 -> 28 bumps and proves that the old comparison passes while the first-parent comparison rejects the collision; all 8 focused guard tests pass.
The original finding is resolved, and I found no remaining P0–P3 issues on this head.
AI-assisted review disclosure: OpenAI Codex reviewed the exact-head workflow revision, Git history semantics, sibling-branch regression, focused test result, and live GitHub state. I verified the evidence and made the final approval decision.
Summary
Two branches that bump
RUNTIME_HOST_COMPATIBILITY_EPOCHwrite the same text to the same line, so git's three-way merge resolves them without a conflict and two incompatible protocols end up advertising one epoch (#3313).This adds the interim merge-result guard from that issue. On a pull request it compares the checked-out synthetic merge with
HEAD^1, its current-base first parent, and fails when protocol files changed while the epoch stayed equal or moved backward. The derive-the-epoch-from-the-contract question stays open for the dev list.Refs #3313
Review focus
HEAD^1, notpull_request.base.sha: the event value is the PR's older base snapshot and misses a sibling same-number bump that has since landed on main.packages/runtime-host/src/protocol/, including tolerance-only edits, requires an epoch beyond the current base parent.Verification
HEAD^1correctly rejects B and reports its changed protocol fileAI use
Select exactly one:
Tool(s) and scope: Claude Code (Fable 5) — original implementation and tests; Codex — review analysis, rebase, first-parent revision fix, real Git-graph regression, and verification. Material commits carry a
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?