[codex] Fix PR-batch Goal-mode completion contract - #52
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughIntroduces a "Goal Mode Completion Contract" in workflows/pr-processing.md, propagated into pr-batch and plan-pr-batch skill prompts and execution rules. Adds a Ruby test verifying contract consistency across sources, extends prompt-size validation with headroom checks, and wires the new test into bin/validate. ChangesGoal Mode Completion Contract
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as GoalCompletionContractTest
participant Workflow as pr-processing.md
participant PrBatch as pr-batch/SKILL.md
participant PlanPrBatch as plan-pr-batch/SKILL.md
Test->>Workflow: read_repo_file, extract contract phrases
Test->>PrBatch: extract_goal_prompt_template, check contract link
Test->>PlanPrBatch: extract_goal_prompt_template, check contract line
Test->>Test: assert_text_includes required phrases
Test->>Test: assert byte-for-byte contract equality
Test->>Test: assert terminal-state wording (ready-no-merge-authority, auto_merge_when_gates_pass)
Related Issues: None specified Related PRs: None specified Suggested labels: documentation, skills, testing Suggested reviewers: None specified 🐰 A rabbit hops through prompts and threads, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Review SummaryReviewed the Goal Mode Completion Contract addition (bin/validate, skills/plan-pr-batch/SKILL.md, skills/plan-pr-batch/scripts/check_goal_prompt_size.rb, skills/pr-batch/SKILL.md, skills/pr-batch/bin/goal-completion-contract-test.rb, workflows/pr-processing.md). Overall: the substance of the rule (don't call a batch complete while any target is still waiting-on-checks-or-review, has untriaged CI/review state, or is UNKNOWN) is sound and closes a real gap. No security issues, no consumer-repo hardcoding introduced, no shell-injection concerns in the new Ruby test or bin/validate line. Main issue (left as inline comments): the canonical contract text is pasted into 4 places (workflows/pr-processing.md, skills/pr-batch/SKILL.md x2, skills/plan-pr-batch/SKILL.md x1) with three different exact wordings, rather than having the skill files link back to the canonical section. This repo already has an explicit convention for this - see the 'Canonical batch handoff copy... should point here instead of duplicating this section' comment guarding 'Batch Handoff Format' in workflows/pr-processing.md - which this change doesn't follow for the non-prompt-template copy in pr-batch/SKILL.md. The new goal-completion-contract-test.rb only checks that specific substrings are present in each copy, so the wordings can keep drifting (e.g. already: 'configured review agents' vs 'review agents') without any test failing. I verified textually (couldn't execute ruby/bin/validate in this sandbox - execution was blocked pending approval) that all required substrings for goal-completion-contract-test.rb and check_goal_prompt_size.rb are currently present in the three source files, and that the goal-prompt template body is about 3.8k of the 4k character budget after this change, leaving little headroom for future growth. Details in inline comments. |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
0cda8e9 to
d5b18e7
Compare
![chatgpt-codex-connector[bot]](https://avatars.github.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: d5b18e7838
ℹ️ 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 ReviewThis PR adds a canonical Goal Mode Completion Contract to I ran 8 finder angles (correctness, removed-behavior, cross-file, reuse, simplification/efficiency, altitude, conventions) and verified each surviving candidate. Two flagged "Ruby regex needs a multiline flag" correctness concerns and a "lost
No portability violations found (no consumer-repo commands/labels/branches hardcoded in the changed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4cbe0f6c0
ℹ️ 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".
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/pr-batch/bin/goal-completion-contract-test.rb (1)
1-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCross-skill test placed inside a single skill's
bin/folder.This suite validates consistency across
workflows/pr-processing.md,skills/pr-batch/SKILL.md, andskills/plan-pr-batch/SKILL.md, so it isn't scoped to pr-batch alone. As per coding guidelines,skills/*/bin/**scripts should live "in the skill folder that invokes them, unless the helper is explicitly repo-wide." Consider moving this to a repo-wide location (e.g. top-levelbin/) to make its cross-skill scope explicit and discoverable from either skill.🤖 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 `@skills/pr-batch/bin/goal-completion-contract-test.rb` around lines 1 - 115, The new Ruby contract test is cross-skill and should not live under a single skill’s bin folder. Move the test out of GoalCompletionContractTest’s current location into a repo-wide or otherwise clearly shared bin location, and keep the test logic intact so it still validates workflows/pr-processing.md, skills/pr-batch/SKILL.md, and skills/plan-pr-batch/SKILL.md from one shared place.Source: Path instructions
🤖 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 `@skills/plan-pr-batch/scripts/check_goal_prompt_size.rb`:
- Around line 155-156: The required prompt phrase in check_goal_prompt_size.rb
is missing the opening backtick, so the phrase won’t match the canonical Goal
Mode Completion Contract text. Update the string in required_prompt_phrases to
include the full quoted phrase exactly as it appears in
workflows/pr-processing.md, using the existing required_prompt_phrases array as
the reference point.
---
Nitpick comments:
In `@skills/pr-batch/bin/goal-completion-contract-test.rb`:
- Around line 1-115: The new Ruby contract test is cross-skill and should not
live under a single skill’s bin folder. Move the test out of
GoalCompletionContractTest’s current location into a repo-wide or otherwise
clearly shared bin location, and keep the test logic intact so it still
validates workflows/pr-processing.md, skills/pr-batch/SKILL.md, and
skills/plan-pr-batch/SKILL.md from one shared place.
🪄 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: e682c363-c4ba-4e5c-ba27-ec661f94e99b
📒 Files selected for processing (6)
bin/validateskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/goal-completion-contract-test.rbworkflows/pr-processing.md
ReviewReviewed the diff (6 files, +171/-21). This PR adds a canonical Goal Mode Completion Contract to I traced every phrase/assertion in the new test against the actual file contents (execution was blocked in this sandbox, so this was done by manual string comparison):
No correctness bugs found. One minor (non-blocking) cleanup note: Duplicated fence-extraction logic — Everything else (prose compression in |
Review summaryThis PR adds a canonical Goal Mode Completion Contract to Portability: clean. The new contract text and pressure-check bullets are fully generic (placeholder PR/issue counts, no org/repo names, labels, or branches). Verification: the three copies of the contract line ( Findings (posted inline)
Minor/optional (not posted inline, low confidence)
No security or performance concerns — this is a docs/prompt + test-only change with no new executable surface beyond the reviewed Ruby helpers. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 907acc007d
ℹ️ 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".
Review summaryReviewed the diff focusing on code quality, bugs, security, performance, portability (shared skill/workflow genericity), and shell/Ruby helper safety. Overall: this is a well-scoped, low-risk docs + test change. Findings:
No functional bugs found. I wasn't able to execute the Ruby test suite directly in this sandbox (script execution required approval that wasn't available), so this is a static review; the PR description's own |
Review summaryThis PR adds a canonical Goal Mode Completion Contract to Portability / AGENTS.md editing rules: no violations found — the new contract text and test are fully generic (no consumer-repo commands, labels, branches, or paths hardcoded), and the doc/anchor cross-references ( Shell/Ruby helper safety ( Findings posted inline (3):
Nothing else rose to a blocking level — the byte-for-byte alignment tests, section-boundary regexes, and anchor links in the new/changed Ruby all check out correctly against the actual heading structure of |
Review summaryThis PR adds a "Goal Mode Completion Contract" paragraph to Findings posted as inline comments, most notable first:
No security or portability issues found — no consumer-repo-specific hardcoding was added (the pre-existing |
ReviewReviewed HEAD Note: this PR has already been through many automated review rounds today (8 prior Substance: sound and closes a real gap — Portability (AGENTS.md editing rules): clean. The new contract text, pressure checks, and test file are fully generic — no consumer-repo commands/labels/branches/paths hardcoded. The pre-existing Shell/Ruby helper safety: Verified directly against HEAD:
Non-blocking observation: the contract sentence is pasted as literal text in multiple files rather than referenced from one canonical source — necessary here since goal prompts must be self-contained text blobs handed to a worker LLM, not links. The new byte-for-byte alignment test is the correct mitigation for drift risk given that constraint, so this isn't a defect, just worth remembering that all 4 copies need a matching edit if the wording changes again. Could not execute No blocking issues found. |
…h-merge-readiness-route * origin/main: [codex] Make plan-pr-batch goals Codex-pasteable (#50) [codex] Add host adapter and Codex plugin metadata [codex] Add pause restart prompt skill [codex] Add batch titles and audit defaults [codex] Fix PR-batch Goal-mode completion contract (#52) Add strict trust mode to preflight (#48) [codex] Add generic PR-batch continuation prompt (#49)
1 participant
Summary
This PR fixes the PR-batch Goal-mode closeout contract so a batch cannot be reported as complete while any target is still waiting on checks/review, unknown, or otherwise non-terminal.
What changed
Goal Mode Completion Contracttoworkflows/pr-processing.md.$pr-batchand generatedplan-pr-batchgoal prompts.Why
Batch closeout could previously report a useful per-target state such as
waiting-on-checks-or-reviewwhile still letting the overall Goal-mode task look complete. The new contract makes that state explicitly non-terminal for the overall goal unless the run is status-only/timeboxed or there is a real blocker with exact resume instructions.Validation
ruby skills/pr-batch/bin/goal-completion-contract-test.rbruby skills/plan-pr-batch/scripts/check_goal_prompt_size.rbbin/validatecodex review --uncommittedwas attempted in the clean worktree, but this local Codex CLI recursively spawned anothercodex review; I stopped the review process. A prior independentcodex review --uncommittedon the same diff reported no discrete correctness or blocking maintainability issues, and the clean worktree validation above passed.Merge authority
merge_authority: auto_merge_when_gates_passper the maintainer request to get this PR to merge.Summary by CodeRabbit
New Features
Bug Fixes
Tests