Skip to content

ci: guard the compatibility epoch against silent same-number merges - #3339

Merged
Astro-Han merged 2 commits into
apache:mainfrom
yihanzhu:ci/protocol-epoch-merge-base-guard
Aug 21, 2026
Merged

ci: guard the compatibility epoch against silent same-number merges#3339
Astro-Han merged 2 commits into
apache:mainfrom
yihanzhu:ci/protocol-epoch-merge-base-guard

Conversation

@yihanzhu

@yihanzhu yihanzhu commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Two branches that bump RUNTIME_HOST_COMPATIBILITY_EPOCH write 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

  • The guard intentionally uses HEAD^1, not pull_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.
  • The affected-test planner keeps its existing event-base behavior; only the epoch guard needs the synthetic merge parent.
  • The guard remains coarse by design: every change under packages/runtime-host/src/protocol/, including tolerance-only edits, requires an epoch beyond the current base parent.

Verification

  • Epoch guard tests: 8 pass
  • Real Git graph: sibling A and B both bump 27 → 28; the old fork-point comparison passes, while HEAD^1 correctly rejects B and reports its changed protocol file
  • CI planner and workflow policy tests: 53 pass; Windows inventory tests: 3 pass and inventory current
  • Guard self-check, root lint, and format pass

AI use

Select exactly one:

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

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-by trailer.

Checklist

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

Does this PR entail a change in behavior?

  • Yes — CI rejects a protocol change whose merged result does not move the epoch past the current base parent's value

@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 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.

Comment thread .github/workflows/ci.yml Outdated
- name: Guard the protocol compatibility epoch
if: github.event_name == 'pull_request'
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}

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 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

yihanzhu and others added 2 commits August 21, 2026 08:24
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
@yihanzhu
yihanzhu force-pushed the ci/protocol-epoch-merge-base-guard branch from ea6f2aa to d4d0232 Compare August 21, 2026 12:31
@yihanzhu

Copy link
Copy Markdown
Contributor Author

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 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 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants