fix(devtools): stabilize PR scope attestation - #3946
Conversation
Keep v2 scope intent stable in the PR body while merge-gate binds it to the current head and canonical Bead records. Validate the full Bead mutation set and support explicitly self-contained PRs.
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR introduces v2 PR-scope carriers with Bead and self-contained scopes. It validates declared mutations against base revisions, adds head/base-bound attestation sync, and binds merge-gate receipts to current scope and Bead state. ChangesPR-scope v2 workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant pr_scope
participant Beads
participant merge_gate
GitHub->>pr_scope: Fetch PR head and base SHAs
pr_scope->>Beads: Validate base-to-checkout mutations
Beads-->>pr_scope: Return mutated Bead records
pr_scope-->>GitHub: Emit scope attestation
merge_gate->>GitHub: Read current PR scope and attestation state
merge_gate->>merge_gate: Compare state with verification receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/merge_gate.py`:
- Around line 281-282: Update cmd_check to verify the local checkout before
calling _scope_verdict or pr_scope.validate_pr_body: require HEAD to match the
PR head and the working tree to be clean, matching the existing cmd_record
guard. Reject the check with the established error handling when either
condition fails, preventing local Bead state from being used for another
revision.
- Around line 544-548: Extract the repeated baseRefOid narrowing into a shared
helper in devtools/merge_gate.py, returning the string value from info when
present and None otherwise. Replace the occurrences in cmd_record, cmd_check,
and the surrounding attestation-digest flow with this helper so all digest
calculations use identical normalization.
In `@devtools/pr_scope.py`:
- Around line 866-868: Update the exception tuple in the check path’s handler to
include RuntimeError, matching the sync and check-ci handlers so failures from
resolve_repository or fetch_pr_metadata print the existing refusal message and
return exit code 2.
- Around line 346-349: Update the self-contained validation around
ScopeKind.SELF_CONTAINED and the mutated_beads comparison so a self-contained PR
may declare Bead mutations and pass carrier validation when changed Bead records
are present. Continue rejecting assigned_beads and dispositions, while
preserving mutation-set validation for non-self-contained scopes or otherwise
explicitly bypassing it for self-contained scopes.
- Around line 208-213: Harden the base revision handling in _bead_records_at by
validating base_sha as a hexadecimal Git object name before constructing the git
show argument, rejecting values beginning with “-” or containing invalid
characters. Preserve the existing error behavior for unreadable revisions and
avoid treating the list-based subprocess invocation as shell injection.
- Around line 359-371: Filter bound_ids to IDs present in the checkout records
before calling canonical_beads_digest, while retaining assigned IDs and existing
successor handling. Update the logic around load_bead_records and
canonical_beads_digest so deleted mutated Beads are excluded from the digest
without changing changed_bead_ids validation.
- Around line 214-223: Extract the shared JSONL record-parsing loop into a
helper that accepts an iterable of lines and returns the records mapping,
preserving the existing _type and id filters and duplicate-ID ValueError format.
Update both load_bead_records and the shown base-record loading path to call
this helper so changed_bead_ids compares identically parsed results.
- Around line 686-687: Replace the source-text check in the argument-building
flow around metadata.base_sha with a behavioral capability probe against the
fetched validator interface. Determine whether the validator accepts the
--base-sha option without relying on formatting-sensitive source bytes, then
append --base-sha and metadata.base_sha when supported; ensure probe failures do
not silently omit validation without an explicit signal.
In `@tests/unit/devtools/test_pr_scope.py`:
- Around line 189-213: Add test cases in the existing PR scope test covering a
Bead present at the base revision but deleted in the checkout, including the
expected changed ID and unresolved canonical digest path, and a self_contained
carrier where .beads/issues.jsonl differs from the base commit and includes a
real mutation. Exercise each through pr_scope.main validation and assert the
corrected outcomes, preserving the existing edited-record case.
- Around line 234-238: Add an assertion in the attestation test alongside the
existing head_sha check to verify attestation["base_sha"] equals the configured
base SHA value ("b" repeated 40 times). Keep the existing digest and head SHA
assertions unchanged.
- Around line 163-179: Update the git commit invocation in the test setup to
isolate it from host configuration by explicitly disabling commit signing and
external hooks via git -c options, alongside the existing user.name and
user.email settings. Keep the argument-list subprocess execution unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fbb93f9-905e-489a-8d8d-6c2bbe4b0690
📒 Files selected for processing (8)
.github/pull_request_template.mdCLAUDE.mddevtools/command_catalog.pydevtools/merge_gate.pydevtools/pr_scope.pydocs/devtools.mdtests/unit/devtools/test_merge_gate.pytests/unit/devtools/test_pr_scope.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 558be67734
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e14b3c22f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b67f5dcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f8c23d7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devtools/merge_boundary.py (1)
611-624: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the real cause when the scope verdict fails.
The condition mixes two causes. If
scope.okis false, the code prints "no fresh merge-gate receipt", then callsmerge_gate.cmd_record, which refuses with exit code 2 for the invalid carrier. The first message names the wrong cause, so an operator reads a receipt-freshness message for a carrier problem.Separate the two branches so each prints its own cause.
🐛 Proposed fix
- if not scope.ok or not _receipt_is_fresh_for_scope( + if not scope.ok: + print( + f"PR #{pr} has an invalid structured pr-scope carrier -- see the reasons below", + file=sys.stderr, + ) + if not scope.ok or not _receipt_is_fresh_for_scope( pr, head_sha=head_sha, scope=scope, base_sha=merge_gate._base_sha(info), max_age_s=max_age_s, ):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devtools/merge_boundary.py` around lines 611 - 624, Separate the combined condition in the merge-gate handling around _scope_verdict and _receipt_is_fresh_for_scope into distinct branches. When scope.ok is false, report the invalid scope/carrier cause and handle recording accordingly; only print the “no fresh merge-gate receipt” message and invoke merge_gate.cmd_record for a valid scope with a stale or missing receipt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.codex/agents/reviewer.toml:
- Line 32: Update the reviewer instruction at the line containing “missing
current head-bound attestation” to require running both `devtools workspace
pr-scope check --pr <N>` and `devtools workspace pr-scope sync --pr <N>`.
Require reviewers to reject the change when the sync-produced current-head
attestation is missing or invalid, while retaining validation of the embedded
carrier.
In `@devtools/pr_scope.py`:
- Around line 242-251: Update the git fetch subprocess call in the base-revision
validation flow to include a finite timeout consistent with sibling remote
calls, such as 120 seconds. Preserve the existing nonzero-return error handling,
and allow subprocess.TimeoutExpired to propagate so the existing
validate_carrier and main exception handlers can report the stalled fetch.
In `@tests/unit/devtools/test_pr_scope.py`:
- Around line 243-270: The repeated Git fixture setup in the affected tests
should be centralized in a helper. Add a shared isolation-flag constant and a
helper such as _committed_bead_repository that writes all records, initializes
and commits the repository, resolves HEAD, and returns the repository path, Bead
file path, and base SHA; replace all four duplicated setup blocks with calls to
it.
---
Outside diff comments:
In `@devtools/merge_boundary.py`:
- Around line 611-624: Separate the combined condition in the merge-gate
handling around _scope_verdict and _receipt_is_fresh_for_scope into distinct
branches. When scope.ok is false, report the invalid scope/carrier cause and
handle recording accordingly; only print the “no fresh merge-gate receipt”
message and invoke merge_gate.cmd_record for a valid scope with a stale or
missing receipt.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80755877-d92c-4cc9-8353-740a9b106e71
📒 Files selected for processing (9)
.claude/agents/lane.md.codex/agents/reviewer.tomldevtools/lane_brief.pydevtools/merge_boundary.pydevtools/merge_gate.pydevtools/pr_scope.pytests/unit/devtools/test_merge_boundary.pytests/unit/devtools/test_merge_gate.pytests/unit/devtools/test_pr_scope.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d0518add
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7566a7ee81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e08e4d1228
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba40d1ada9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for successor in entry.get("successors", []) | ||
| if isinstance(entry.get("successors"), list) and isinstance(successor, str) |
There was a problem hiding this comment.
When a malformed carrier sets successors to null or another non-iterable value, this comprehension begins iterating entry.get("successors", []) before its later isinstance(..., list) guard can run. validate_pr_body therefore raises TypeError, which the CLI does not catch, so the required scope check emits a traceback instead of a structured BLOCK; validate the value before using it as the comprehension iterable.
Useful? React with 👍 / 👎.
| if base_sha is not None and successor_ids: | ||
| try: | ||
| disposition_records = _bead_records_at(base_sha) |
There was a problem hiding this comment.
When the target branch deletes an assigned Bead after this PR forks and the PR leaves that record unchanged, the mutation set correctly omits the ID and a normal merge preserves the target deletion. Because prospective records are loaded only when the disposition names successors, a satisfied disposition with no successors is validated against the stale PR-head record and passes, leaving the merged carrier assigned to a nonexistent Bead; build the prospective state for assigned IDs as well and reject missing assigned records.
AGENTS.md reference: AGENTS.md:L384-L390
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Keep PR scope intent stable across commits. The v2 carrier leaves exact head and canonical Bead bindings to the existing merge-gate receipt, adds complete tracker-mutation scope, and supports explicitly self-contained PRs.
Problem
The v1 PR-body carrier embeds
head_shaandbeads_digest. Each commit therefore requires a body edit before CircleCI can validate the pushed head, even though GitHub metadata and merge-gate receipts already identify the current commit. The carrier also had no structured self-contained form and could not declare all changed Bead records.Ref
polylogue-pr-scope-contract.Solution
devtools/pr_scope.pyaccepts v1 during transition only when the Bead mutation set is empty, then renders v2 stable intent for all tracker changes.sync --prreports a current head-bound attestation without editing the PR body.merge_gate.pynow refuses an unrelated or dirty checkout, andmerge_boundary.pyrefreshes receipts when the attestation changes. Mutation comparison uses the PR merge base, not a moved target tip. The PR template, command catalog, generated reference, CLAUDE.md, and lane guidance document the v2 publication flow.The PR body uses a v1 carrier only for this transition because the current base-revision validator is v1. Once this PR is on master, new PRs can use v2 without body churn.
Verification
devtools test tests/unit/devtools/test_pr_scope.py tests/unit/devtools/test_merge_gate.py tests/unit/devtools/test_merge_boundary.py:126 passed in 2.34s.mypy devtools/pr_scope.py devtools/merge_gate.py:Success: no issues found in 2 source files.git diff --check: no output.The fresh
devtools verify --seed-testmon --skip-slowdiscovery run was stopped after it made no test progress; its completed static, render, documentation, and policy steps are retained in the local receipt. It is not claimed as a passing baseline.Bead disposition matrix
polylogue-pr-scope-contractcommit:558be6773,commit:3e14b3c22,commit:f9e39e8a6,commit:a6b67f5dc,commit:c8f8c23d7,commit:6b6ea6767,commit:f2d0518ad,commit:7566a7ee8,commit:d6211df13,commit:e08e4d122,test:126 focused tests,command:mypypolylogue-inygw