Tools: add PR batch merge ledger#3996
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces ChangesPR Merge Ledger Tool and Workflow Enforcement
Sequence Diagram(s)sequenceDiagram
participant Coordinator as Coordinator Agent
participant Script as script/pr-merge-ledger
participant GH as gh api graphql
participant Fixture as Fixture JSON
Coordinator->>Script: run --strict --changelog-classification X --finding-dispositions Y <PR>
alt fixture mode
Script->>Fixture: load JSON
Fixture-->>Script: raw PR data
else live mode
Script->>GH: GraphQL: PR files
GH-->>Script: files nodes
Script->>GH: GraphQL: review threads (paginated)
GH-->>Script: thread nodes + comments
Script->>GH: GraphQL: reviews (paginated)
GH-->>Script: review nodes
Script->>GH: GraphQL: issue comments (paginated)
GH-->>Script: comment nodes
end
Script->>Script: normalize + analyze (threads, reviews, findings, changelog, lockfile)
Script->>Script: assemble violations + unknown_fields
Script-->>Coordinator: JSON ledger (complete_allowed, violations, unknown_fields)
alt complete_allowed: false or UNKNOWN fields
Coordinator->>Coordinator: block "complete" reporting
else complete_allowed: true, no violations
Coordinator->>Coordinator: proceed to merge/handoff
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Code Review — PR #3996: Tools: add PR batch merge ledgerOverviewAdds Strengths
IssuesBug / accuracy gap — multiple same-severity findings from one source are collapsed to one entry
The fix is to include a per-line index or hash in the dedup key, or to emit one finding per review/comment source rather than per line: # Option A: dedup by (url, severity, line_index) to preserve distinct findings
unique_findings = findings.each_with_index.uniq { |(finding, idx), _| [finding.fetch("url"), finding.fetch("severity"), idx] }.map(&:first)
# Option B: simpler — one finding per (source id, severity) which is what the current
# disposition model already assumes (disposition is per source, not per line)
unique_findings = findings.uniq { |f| [f.fetch("id"), f.fetch("severity")] }Option B aligns with the disposition model (you waive a review/comment, not an individual line) and avoids double-counting. Missing test cases
Minor concerns
SummaryThe overall implementation quality is high. The core concern worth addressing before merge is the finding deduplication key ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
justin808
force-pushed
the
jg-codex/3907-pr-merge-ledger
branch
from
f90bd3e to
7a8e5e2
Compare
justin808
added
full-ci
full-ci-no-benchmarks
|
Addressed current-head review feedback in 7a8e5e2: distinct same-severity findings from one source now remain separate via source line/excerpt, missing |
![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: 7a8e5e2464
ℹ️ 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".
Code Review — PR Batch Merge LedgerSummaryThis PR introduces Overall quality is high. The class-based architecture, clean GitHubCollector/PrMergeLedger separation, explicit UNKNOWN propagation, and paginated live-fetch with truncation detection are all well-executed. The test coverage is thorough for the happy and failure paths it covers. Issues below are mostly correctness edge-cases and one schema design point — none are blockers, but a few are worth addressing. Issues1. 2. 3. 4. Cross-source finding deduplication gap (script/pr-merge-ledger:377–387) 5. Schema Minor notes
VerdictApprove after addressing items 1–2 (small, clear fixes) and optionally items 3–5. The tool is well-designed and the test corpus is production-quality. |
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge; purely internal process tooling with no runtime or package changes, and all four comments are non-blocking quality improvements. The script is well-structured and thoroughly tested. The disposition-per-source issue is the most substantive concern: a single disposition entry silently clears every finding in a multi-finding review body, which could allow a ledger to report complete_allowed: true while genuine findings remain open. The remaining comments cover a missing KeyError rescue, inconsistent body storage for issue comments vs reviews, and the false-positive risk from negation sentences that start with a severity label. None of these block the intended workflow. script/pr-merge-ledger — the disposition lookup and finding-pattern sections warrant a second look before the tool is used in production batch workflows. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([script/pr-merge-ledger]) --> B{--fixture?}
B -- yes --> C[load_fixture JSON]
B -- no --> D[GitHubCollector.fetch via GraphQL]
D --> D1[metadata + files paginated]
D --> D2[reviewThreads paginated]
D --> D3[reviews paginated]
D --> D4[comments paginated]
C --> E[ledger_for dataset]
D1 & D2 & D3 & D4 --> E
E --> F[normalize_pull_request]
E --> G[normalize_review_threads]
E --> H[normalize_reviews]
E --> I[normalize_issue_comments]
F & G & H & I --> J[finding_dispositions\nextract P0/P1/P2/MUST-FIX lines]
J --> K[disposition_for URL or ID lookup]
F --> L[unknown_fields_for]
G --> L
J --> L
L --> M[violations_for]
M --> N[build_report JSON]
N --> O{--strict AND\ncomplete_allowed: false?}
O -- yes --> P([exit 1\nstderr: ledger violations])
O -- no --> Q([exit 0\nstdout: JSON report])
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@script/pr-merge-ledger`:
- Around line 242-254: The `normalize_pull_request` function at lines 242-254 in
script/pr-merge-ledger assigns nil to "head_sha" when raw_pr["headRefOid"] is
missing, which violates the schema requirement that pr.head_sha must be a
required string. Apply the same fallback pattern used for "review_decision"
(with the || UNKNOWN operator) to the "head_sha" assignment so that when
headRefOid is omitted, it defaults to UNKNOWN instead of nil, keeping the output
schema-valid. This same fix must also be applied to the corresponding code at
lines 496-503 in the same file.
- Around line 165-168: The `load_dispositions` function accepts any JSON root
type (arrays, scalars, etc.) but the `disposition_for` method later assumes it
can index the result with string keys. If the JSON file contains an array or
scalar value instead of a JSON object, this causes an unhandled TypeError. Add
validation in `load_dispositions` after `JSON.parse(File.read(path))` to ensure
the parsed result is a Hash/object; if it is not, either return an empty hash or
raise an appropriate error so that the exception handling in the `run` method
can properly catch and handle it.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd394096-3111-43e1-a1fe-a45e731df33f
📒 Files selected for processing (7)
.agents/skills/pr-batch/SKILL.md.agents/workflows/pr-processing.mdRakefilereact_on_rails/spec/react_on_rails/fixtures/pr_merge_ledger/pr_3613.jsonreact_on_rails/spec/react_on_rails/pr_merge_ledger_script_spec.rbscript/pr-merge-ledgerscript/pr-merge-ledger.schema.json
Code ReviewOverall: Well-structured addition of a machine-checkable merge-readiness ledger. The architecture is clean, the fixture-based test harness is solid, and the documentation integration is consistent across both workflow files. The 16 RSpec examples cover the important edge cases. A few issues worth addressing before merge. Bug:
|
justin808
force-pushed
the
jg-codex/3907-pr-merge-ledger
branch
from
7a8e5e2 to
a71f6af
Compare
justin808
force-pushed
the
jg-codex/3907-pr-merge-ledger
branch
from
a71f6af to
050baca
Compare
Code ReviewGood PR overall — the test coverage is thorough and the ledger design is well thought-out. Three findings worth addressing before merge: P1 — 429 is incorrectly marked as non-retryable
The pattern 401/403/404 are genuinely non-retryable (auth errors, missing resource) and should stay. P2 —
|
Code Review — PR #3996:
|
Review: PR 3996 - Add PR batch merge ledgerWell-structured internal tooling with thorough test coverage (104 RSpec examples). The ledger logic, disposition scanning, review-thread heuristics, and JSON schema are solid. One P1 bug and a few smaller notes below (also posted as inline comments). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96bd0cf4f1
ℹ️ 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".
Merge Gate BlockCurrent head: Local and GitHub validation are otherwise clean:
Blocked action: this PR needs a qualifying independent finalizer/approval for the current head, or an explicit maintainer-approved path for handling the blank GitHub aggregate review decision before it can be merged under the accelerated-RC rules. |
Maintainer-Delegated Merge AcknowledgementCurrent head: Final waiver set for this maintainer-delegated human merge path:
Immediate merge decision: proceed only if the final live re-fetch still shows all checks pass or expected-skip, unresolved review threads are 0, no current-head This is not an unattended accelerated-RC auto-merge; it is a maintainer-authorized merge path with criteria documented in the PR description. |
* origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) Add RSC FOUC acceptance coverage (#4033) Keep plan-pr-batch goal prompts under 4000 chars (#4025) docs(skills): file-collision check + goal-prompt size discipline for plan-pr-batch (#3914) Verify React 19.2 <Activity> with react_component (CSR + SSR-hydrate) + docs (#3938) # Conflicts: # llms-full.txt
* origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) Add RSC FOUC acceptance coverage (#4033) Keep plan-pr-batch goal prompts under 4000 chars (#4025) docs(skills): file-collision check + goal-prompt size discipline for plan-pr-batch (#3914) Verify React 19.2 <Activity> with react_component (CSR + SSR-hydrate) + docs (#3938) # Conflicts: # CHANGELOG.md # llms-full.txt
…ter-slice * origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) Add RSC FOUC acceptance coverage (#4033) Keep plan-pr-batch goal prompts under 4000 chars (#4025) docs(skills): file-collision check + goal-prompt size discipline for plan-pr-batch (#3914) Verify React 19.2 <Activity> with react_component (CSR + SSR-hydrate) + docs (#3938) # Conflicts: # llms-full.txt
…e-demo-cost-docs * origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) # Conflicts: # llms-full.txt
…ce-maps * origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) Add RSC FOUC acceptance coverage (#4033) Keep plan-pr-batch goal prompts under 4000 chars (#4025) docs(skills): file-collision check + goal-prompt size discipline for plan-pr-batch (#3914) Verify React 19.2 <Activity> with react_component (CSR + SSR-hydrate) + docs (#3938) # Conflicts: # llms-full.txt
…c-streaming * origin/main: Expose React 19 root error callbacks (rootErrorHandlers) + hydration-mismatch debugging guide (#3933) Add post-merge audit scope resolver (#4029) Document continuous agent-run evaluation loop (#4028) Tools: add PR batch merge ledger (#3996) Add RSC FOUC acceptance coverage (#4033) # Conflicts: # llms-full.txt
1 participant
Summary
script/pr-merge-ledgerplusscript/pr-merge-ledger.schema.jsonto produce a machine-checkable PR closeout ledger from GitHub PR state..agents/workflows/pr-processing.mdand$pr-batchcloseout guidance before merge readiness or batch handoff.UNKNOWNchangelog classifications are rejected,ghsubprocess timeouts avoid RubyTimeout.timeoutaroundOpen3.capture3, and rate-limit/secondary-limit responses stay on the retry path.Closes #3907.
Validation
Current head SHA:
fba964e48f09ca7a61258103cdad77f001087f7aruby -c script/pr-merge-ledger->Syntax OK.cd react_on_rails && bundle exec rspec spec/react_on_rails/pr_merge_ledger_script_spec.rb-> 105 examples, 0 failures.cd react_on_rails && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop ../script/pr-merge-ledger spec/react_on_rails/pr_merge_ledger_script_spec.rb-> no offenses.git diff --check-> passed.pnpm start format.listDifferent-> passed with the repo's existing Node dependency bin path.script/ci-changes-detector origin/main-> full suite recommended; no benchmarks indicated.Acceptance dry run:
script/pr-merge-ledger --fixture react_on_rails/spec/react_on_rails/fixtures/pr_merge_ledger/pr_3613.json --changelog-classification not_user_visible --strict --pretty > /tmp/pr-3613-merge-ledger-fba964e48.json-> strict exit 1.complete_allowed=falsewithreview_decision_changes_requested,unresolved_current_head_review_thread, andunknown_priority_finding_dispositionviolations.Self-ledger:
/tmp/pr-3996-finding-dispositions-after-fba964e48-complete.json.script/pr-merge-ledger 3996 --repo shakacode/react_on_rails --changelog-classification not_user_visible --finding-dispositions /tmp/pr-3996-finding-dispositions-after-fba964e48-complete.json --strict --pretty > /tmp/pr-3996-merge-ledger-final-fba964e48.json-> strict exit 1 only forunknown_review_decision.complete_allowed=false; unresolved current-head review threads: 0; unknown priority dispositions: 0; unused disposition keys: 0.Current GitHub checks snapshot:
claude-reviewpassed, CodeRabbit skipped, Cursor Bugbot neutral/skipping, benchmark confirmation jobs skipped.Codex Decision Log
UNKNOWNchangelog classification handling, 4xx retry handling, 429 retry handling, 403 secondary/rate-limit retry handling, and subprocess timeout cleanup.main.$schemaURI and rely on emittedschema_version: pr-merge-ledger/v1for this initial internal gate.conservative_non_outdatedevidence is a string.comment_head_shain the same ledger thread object..locksuffix detection.Agent Merge Confidence
Mode: accelerated-rc
Current head SHA:
fba964e48f09ca7a61258103cdad77f001087f7aScore: 8/10 for maintainer-delegated human merge
Auto-merge recommendation: no; the independent accelerated-RC finalizer is not present, so this is not an unattended auto-merge path
Maintainer-delegated merge recommendation: yes, per current-session maintainer instruction: "You can merge if you are confident. Just document your criteria in the pr description."
Affected areas: CI/process tooling, root Ruby script, JSON schema, RSpec coverage
CI detector:
script/ci-changes-detector origin/main-> full suite recommended; labels remainfull-ci, full-ci-no-benchmarksValidation run:
ruby -c script/pr-merge-ledger->Syntax OKcd react_on_rails && bundle exec rspec spec/react_on_rails/pr_merge_ledger_script_spec.rb-> 105 examples, 0 failurescd react_on_rails && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop ../script/pr-merge-ledger spec/react_on_rails/pr_merge_ledger_script_spec.rb-> no offensespnpm start format.listDifferent-> passed with the repo's existing Node dependency bin pathgit diff --check-> passedreviewDecisionUNKNOWN; unresolved current-head review threads, unknown priority dispositions, and unused disposition keys are 0Review/check gate:
fba964e48f09ca7a61258103cdad77f001087f7a; 44 pass and 7 expected skipsclaude-reviewcompleted successfully; Cursor Bugbot neutral/skipping; CodeRabbit skippedCHANGES_REQUESTEDreview object remains; all actionable current-head findings were fixed, replied to, or dispositionedKnown residual risk: GitHub aggregate
reviewDecisionis blank/UNKNOWN; this is explicitly waived for this maintainer-delegated human merge because concrete review-thread, review-object, finding-disposition, and check evidence is otherwise complete for the current headFinalized by: maintainer-delegated merge authority from
justin808in the Codex session on 2026-06-15; not an independent accelerated-RC auto-merge finalizerMerge Criteria
Current head SHA:
fba964e48f09ca7a61258103cdad77f001087f7aRelease mode is
accelerated-rcfrom #3823. This PR is mergeable only through the maintainer-delegated human merge path, not unattended accelerated-RC auto-merge. I am comfortable merging when all of these are true immediately before merge:fba964e48f09ca7a61258103cdad77f001087f7a.AGENTS.md.claude-reviewhas completed for the current head without a confirmed blocker.unknown_review_decision, caused by GitHub returning a blank aggregatereviewDecisionwhile concrete review-object/thread evidence is clean.reviewDecisionis explicitly waived for this maintainer-delegated human merge because no current-headCHANGES_REQUESTEDreview object remains, all current-head threads are resolved, all priority findings are dispositioned, and all checks are green or expected skips.Current merge decision: mergeable by maintainer-delegated human path after one final live re-fetch. This remains not an unattended accelerated-RC auto-merge because no independent finalizer is present.
Labels:
full-ci, full-ci-no-benchmarks. This PR changes merge-gate/process tooling and specs, so it needs broad CI coverage but no runtime benchmark signal.Note
Medium Risk
Changes merge-gate and batch-closeout policy for agents; incorrect ledger logic could block merges or miss real blockers, though scope is tooling/docs plus extensive fixture tests rather than runtime app code.
Overview
Introduces a machine-checkable merge closeout gate via
script/pr-merge-ledger(withscript/pr-merge-ledger.schema.json): it pulls GitHub PR state (reviews, threads, comments, lockfiles), emits versioned JSON withcomplete_allowed, violations, andUNKNOWNfields, and exits non-zero under--strictwhen changelog classification or P0/P1/P2/Must-Fix dispositions are missing or merge blockers remain.Agent workflow docs (
pr-processing.md,pr-batchskill) now require running the strict ledger at merge readiness and coordinator closeout, forbid marking targetscompletewhile the ledger hasUNKNOWNorcomplete_allowed: false, and expect ledger JSON in batch handoffs.Repo tooling: root RuboCop covers
script/pr-merge-ledger; a large RSpec suite plus a #3613 fixture exercise review-decision/thread/finding edge cases and liveghbehavior (fixtures, pagination, retries).Reviewed by Cursor Bugbot for commit fba964e. Bugbot is set up for automated code reviews on this repo. Configure here.