fix(devtools): make PR scope CI authoritative - #3848
Conversation
Problem: PR scope and residual Bead work were inferred from prose and did not reach CI or the merge boundary. What changed: add a versioned embedded JSON carrier, command-surface render/check commands, CircleCI enforcement, merge-gate receipt binding, and lane/reviewer instructions. Replace the prose reconciliation probe with typed whole-Bead dispositions and successor validation. Verification: devtools test tests/unit/devtools/test_pr_scope.py tests/unit/devtools/test_merge_gate.py tests/unit/devtools/test_merge_boundary.py (49 passed); devtools verify --quick (success).
Problem: the initial structured PR-scope gate ran the validator from the pull request checkout, allowed CircleCI builds without a pull-request URL to fail before validation, and left merge receipts weaker than the carrier they certified. What changed: resolve pull-request metadata from GitHub REST using the exact checkout head, run the validator from the base revision when available, validate the carrier schema strictly, bind receipts to scope and Bead digests, and make the merge use a matching head commit. Remove the prose-parsing PR state workflow and document the structured CI path. Verification: mypy; devtools test tests/unit/devtools/test_pr_scope.py tests/unit/devtools/test_merge_gate.py tests/unit/devtools/test_merge_boundary.py; devtools verify --quick; git diff --check. Co-Authored-By: Claude <noreply@anthropic.com>
Record the structured CI authority follow-up and make it a prerequisite of the remaining merge-gate status-check bead. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 52 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 (1)
📝 WalkthroughWalkthroughChangesPR scope enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CircleCI quick-gate
participant pr_scope.check_ci
participant GitHub REST API
participant Base-revision validator
CircleCI quick-gate->>pr_scope.check_ci: pass repository and expected head SHA
pr_scope.check_ci->>GitHub REST API: resolve the open PR by exact head
GitHub REST API-->>pr_scope.check_ci: return PR metadata and base SHA
pr_scope.check_ci->>Base-revision validator: validate the PR-scope carrier
Base-revision validator-->>pr_scope.check_ci: return validation status
pr_scope.check_ci-->>CircleCI quick-gate: emit verdict and diagnostic log
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 7
🤖 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 @.circleci/config.yml:
- Around line 74-87: Update the “Structured PR scope carrier” command around
`check-ci` so non-master builds without a resolvable candidate PR do not fail
`quick-gate`. Skip the check or treat the specific no-open-PR result as
successful, while preserving failures for network/API and other genuine
validation errors; continue passing the PR number when `CIRCLE_PULL_REQUEST` is
available.
In @.codex/agents/narrow-worker.toml:
- Around line 30-35: Resolve the publication ownership contradiction in both
.codex/agents/narrow-worker.toml lines 30-35 and .codex/agents/worker.toml lines
38-45: remove rendering, validation, and PR publication responsibilities from
these no-push roles and assign them explicitly to the coordinator, or define an
explicit role permitted to push. Ensure the coordinator owns rendering the
PR-scope carrier, validating it, and publishing the non-draft PR.
In `@devtools/command_catalog.py`:
- Around line 833-835: Fix the workspace merge description string around the
command catalog entry by removing the leftover “auto-records a” fragment after
“merge <PR>” so the concatenated text reads grammatically and contains only the
intended merge-gate receipt wording. Keep the existing structured scope
validation and receipt behavior text unchanged.
In `@devtools/merge_gate.py`:
- Around line 222-235: Move the pr_scope.validate_pr_body call and its
invalid-scope refusal block in the record flow below the local-head and
clean-tree guards. Ensure validation occurs only after the checkout is confirmed
to match the PR head, so the carrier digest is read from and bound to the
verified tree; preserve the existing validation behavior and return code.
In `@devtools/pr_scope.py`:
- Around line 439-441: Update fetch_base_validator_source and its
check_ci_metadata caller to read devtools/pr_scope.py from the checked-out base
object when available instead of fetching it through the GitHub API. Handle an
absent base commit explicitly by fetching it or raising a clear error, and do
not pass optional missing data to sys.executable for execution.
In `@tests/unit/devtools/test_merge_gate.py`:
- Around line 201-207: Add a third row to the parametrization for the mutation
test, using receipt_field "pr_scope_digest", a distinct mutated value
representing a changed body digest, and reason "pr_scope_digest". Keep the
existing pr_scope_beads_digest and pr_scope_assigned_beads cases unchanged.
In `@tests/unit/devtools/test_pr_scope.py`:
- Around line 174-222: Add parameterized coverage for both head-authority guards
in check_ci_metadata: one case with a mismatched checkout_head_sha and one with
a mismatched expected_head_sha while metadata.head_sha remains HEAD_SHA. Assert
exit code 2 and verify fetch_base_validator_source is not called, confirming
refusal occurs before base-validator handling.
🪄 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: 6c0d64c5-9424-4d2b-8dcc-92b59fe840d0
📒 Files selected for processing (21)
.beads/issues.jsonl.circleci/config.yml.claude/agents/lane.md.codex/agents/narrow-worker.toml.codex/agents/reviewer.toml.codex/agents/worker.toml.github/pull_request_template.md.github/workflows/pr-state-guard.ymlCLAUDE.mdCONTRIBUTING.mddevtools/command_catalog.pydevtools/lane_brief.pydevtools/merge_boundary.pydevtools/merge_gate.pydevtools/pr_scope.pydevtools/verify_bead_pr_reconciliation.pydocs/devtools.mddocs/tracker-authority.mdtests/unit/devtools/test_merge_boundary.pytests/unit/devtools/test_merge_gate.pytests/unit/devtools/test_pr_scope.py
💤 Files with no reviewable changes (2)
- .github/workflows/pr-state-guard.yml
- devtools/verify_bead_pr_reconciliation.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
polylogue/devtools/command_catalog.py
Lines 2016 to 2019 in b5fe529
Retain post-merge Bead reconciliation
Removing this command eliminates the only check for Beads that remain open after their referenced PR merges; the carrier is only a pre-merge declaration and neither closes records nor audits them afterward. This commit itself declares polylogue-z7sv3 satisfied while adding its canonical record with status: open, demonstrating that the new mechanism does not replace the deleted reconciliation step and can leave completed work on the ready queue indefinitely.
AGENTS.md reference: AGENTS.md:L286-L299
ℹ️ 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".
| branches: [master] | ||
| types: | ||
| - opened | ||
| - edited |
There was a problem hiding this comment.
After CircleCI records a successful quick-gate status for a head SHA, editing the PR body does not push a commit or start another CircleCI pipeline, so removing the only pull_request.edited trigger leaves that success attached even if the carrier is subsequently removed or altered. The local merge wrapper may detect this, but branch protection or auto-merge can consume the stale CI result; retain a PR-metadata-triggered check or otherwise invalidate the status on body edits.
AGENTS.md reference: AGENTS.md:L381-L389
Useful? React with 👍 / 👎.
| "check", | ||
| "--body-file", | ||
| str(body_path), | ||
| "--head-sha", | ||
| metadata.head_sha, | ||
| "--beads-path", | ||
| str(beads_path.resolve()), |
There was a problem hiding this comment.
When check-ci finds a validator on the base revision, this subprocess invokes check --body-file, whose code hardcodes is_draft=False; consequently, a valid carrier on a draft PR passes this supposedly authoritative path even though validate_carrier explicitly rejects drafts. The bootstrap path correctly passes metadata.is_draft, so the behavior changes after the first landing; preserve or pre-check the fetched draft state before accepting the base-validator result.
Useful? React with 👍 / 👎.
| pattern = re.compile( | ||
| r"\b(?:close(?:s|d)?|fix(?:es|ed)?|resolve(?:s|d)?)\s*:?\s+" | ||
| r"(?:(?:[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)?#\d+\b|" | ||
| r"https://github\.com/[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+/issues/\d+\b)", |
There was a problem hiding this comment.
The structured carrier validates only its embedded JSON and never inspects the PR title or surrounding prose, so deleting this regex removes the sole automated rejection of Fixes #N/Closes #N references. In that scenario, merging the PR can close a GitHub issue even when the carrier records partial or deferred scope; retain this check in an active CI path unless the structured validator gains equivalent protection.
AGENTS.md reference: AGENTS.md:L318-L321
Useful? React with 👍 / 👎.
| ~/.local/bin/uv run devtools workspace pr-scope check-ci \ | ||
| "${PR_ARGS[@]}" \ | ||
| --repo "${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}" \ | ||
| --expected-head-sha "${CIRCLE_SHA1}" |
There was a problem hiding this comment.
Although check-ci eventually downloads the base revision's validator, CircleCI first invokes the devtools dispatcher and pr_scope.main from the untrusted PR checkout. A PR can therefore change this entrypoint to return success without calling fetch_base_validator_source (or simply reroute the command), so the base validator is never reached while the quick-gate job still passes; the trusted launcher itself must come from the base revision or an externally enforced CI configuration.
AGENTS.md reference: AGENTS.md:L381-L389
Useful? React with 👍 / 👎.
| for successor in successors: | ||
| record = records.get(successor) | ||
| if record is None: | ||
| reasons.append(f"{bead_id}: successor {successor} is unknown") | ||
| elif record.get("status") == "closed": | ||
| reasons.append(f"{bead_id}: successor {successor} is closed") |
There was a problem hiding this comment.
For a partial, deferred, or superseded disposition, this loop accepts any existing non-closed Bead ID as the successor without checking that the canonical Bead graph contains a relationship to the assigned Bead. A typo that happens to name an unrelated live issue therefore passes while bd show and graph-based planning cannot trace the residual work back to the original acceptance gap; require a durable dependency/discovered-from relationship rather than existence alone.
AGENTS.md reference: AGENTS.md:L294-L299
Useful? React with 👍 / 👎.
Problem: review found that the structured PR scope check could accept an ambiguous CI state, consult the wrong validator revision, and record receipts before verifying the local tree. Agent publication instructions also assigned carrier ownership inconsistently. What changed: distinguish commits with no open PRs from API failures, prefer the checked-out base validator object, defer scope validation until checkout guards pass, and cover the new failure paths with focused tests. Clarify that the coordinator owns carrier rendering and PR publication. Compatibility/migration: non-master CircleCI builds with no open PR now skip the scope check; network and metadata failures remain blocking. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: residual Bead successors were accepted when they merely existed, and draft pull requests could pass the base-validator subprocess because its body-check path defaulted to non-draft. What changed: require a durable Beads relationship for every residual successor and refuse draft PRs before invoking the authoritative base checker. Added focused acceptance and rejection tests. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
|
Review triage for the structured PR-scope carrier:
The PR body carrier has been regenerated and validated against the exact head SHA. |
Problem: the authoritative checker prefers a validator from the checked-out base object, but that selection was not directly covered. What changed: add a focused test proving a locally available base validator is returned without a GitHub fallback. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the CircleCI scope-gate launcher used shell array syntax even though the job's shell contract is not part of the repository interface. What changed: branch explicitly between PR-number lookup and exact-head lookup, keeping both invocations portable while preserving the same strict arguments. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
Bring the published follow-up branch onto the current master tree so the PR is mergeable. Preserve the hardened structured scope carrier while incorporating the merged blob-liveness and display-label changes. Verification: devtools verify --quick passed at the merged tree. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: CircleCI project metadata is not a stable GitHub owner/repository contract for this project, so the scope gate could fail before reaching its validator. What changed: pass the explicit Sinity/polylogue repository slug to both PR metadata paths in the project-specific CircleCI job. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: CircleCI failures in the early gates were not inspectable without an authenticated Circle account. What changed: tee public-claims and structured-scope output into temporary logs and always upload those logs as job artifacts, preserving the command exit status. Ref polylogue-z7sv3 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: CircleCI validates the carrier against the pushed head, so a body-only carrier update after push leaves the first run with stale metadata.\n\nWhat changed: Record the required order in the repository workflow: create the final commit, render its exact carrier, update the PR body, verify the carrier, then push.\n\nCompatibility/migration: This changes coordinator procedure only.\n\nRef polylogue-z7sv3\n\nCo-Authored-By: Claude <noreply@anthropic.com>
|
Review triage for the refreshed head
The draft-state and resolver-keyword comments remain intentionally triaged as before. Drafts are rejected before base validation; prose-pattern enforcement is not restored because this Beads repository uses the structured carrier and its standing policy rejects natural-language gates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d55851260
ℹ️ 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".
| except NoOpenPullRequestError as exc: | ||
| print(f"pr-scope CI skip: {exc}", file=sys.stderr) | ||
| return 0 |
There was a problem hiding this comment.
When CIRCLE_PULL_REQUEST is absent and the commit-to-PR lookup returns no open match, this path reports success without validating any carrier. The inspected CircleCI configuration enables “Only build pull requests” and skips master before invoking check-ci, so a missing match on this path indicates lookup lag or inconsistent PR metadata rather than a legitimate branch build; returning zero can publish a green quick-gate for a PR whose body and base validator were never checked. Treat this as an error and retry or fail closed.
AGENTS.md reference: AGENTS.md:L390-L392
Useful? React with 👍 / 👎.
|
|
||
| def _successor_is_linked(source_id: str, successor_id: str, records: dict[str, dict[str, Any]]) -> bool: | ||
| """Require a durable Beads relationship between a source and its successor.""" | ||
| for record_id, target_id in ((source_id, successor_id), (successor_id, source_id)): |
There was a problem hiding this comment.
Fresh evidence in this revision is the new symmetric relationship check: when the assigned Bead has a supersedes dependency on an older Bead, this loop accepts that older Bead as its successor even though the relationship means the assigned Bead replaces it. A partial/deferred/superseded disposition can therefore point residual work at an obsolete issue and pass validation; validate direction according to relationship type—for example, a successor must supersede or be discovered from the assigned Bead, not vice versa.
AGENTS.md reference: AGENTS.md:L384-L386
Useful? React with 👍 / 👎.
| if [ "${CIRCLE_BRANCH:-}" = "master" ]; then | ||
| exit 0 |
There was a problem hiding this comment.
When a fork submits a PR from its default branch named master, CircleCI exposes the head branch name through CIRCLE_BRANCH, so this unconditional check exits before inspecting the populated CIRCLE_PULL_REQUEST. That gives the fork PR a green structured-scope step without fetching its body or executing the base validator; the previous ordering only skipped master when no PR URL existed. Restrict this bypass to a confirmed non-PR default-branch build rather than the branch name alone.
AGENTS.md reference: AGENTS.md:L390-L392
Useful? React with 👍 / 👎.
1 participant
Summary
Make the structured PR-scope gate authoritative across CI and merge execution. CircleCI now validates the exact checkout head using the base revision's validator when available, and merge receipts bind the complete carrier state.
Problem
The first carrier implementation ran from the pull request checkout, so a PR could modify the validator used to judge its own scope. CircleCI builds can also lack
CIRCLE_PULL_REQUEST, and the old PR-state workflow enforced a prose convention with a regular expression instead of a structured contract.Solution
devtools.pr_scopenow resolves PR metadata through GitHub REST, supports exact-head PR lookup, validates the carrier schema without accepting unknown fields, and runs the base revision validator in an isolated subprocess. CircleCI callscheck-ciwith repository metadata andCIRCLE_SHA1. Merge receipts bind the scope digest, Bead digest, and assigned Bead IDs, while the merge command uses--match-head-commit. The prose-parsing PR-state workflow is removed, and lane and contributor instructions point at the structured carrier.Verification
mypy: Success: no issues found in 2672 source filesdevtools test tests/unit/devtools/test_pr_scope.py tests/unit/devtools/test_merge_gate.py tests/unit/devtools/test_merge_boundary.py: 66 passeddevtools verify --quick: every step exited 0git diff --check: no outputBead disposition matrix
polylogue-z7sv3commit:a03b02a62,commit:b5fe5291a,commit:a4c0d7ffe,commit:f1085c920,commit:859b54052,commit:1a45ca2c3,commit:e409a5182,commit:14724cc0a,commit:4b686ab21,commit:9d5585126,test:66 focused devtools tests passed,command:mypy,command:devtools verify --quickRef polylogue-z7sv3.
Follow-ups
polylogue-1cbehremains open for a GitHub-visible merge-gate status that can support safe auto-merge. Until that status is wired, the repository's merge boundary requires the local structured carrier and fresh receipt checks. The current CircleCI configuration also cannot establish an external trust root for its own launcher, so the base-validator check is a defense within the existing CI trust boundary and the status-workflow work remains separately tracked.