Skip to content

Restore durable Codex goal prompt headroom#148

Merged
justin808 merged 8 commits into
mainfrom
codex/issue-123-prompt-headroom
Jul 13, 2026
Merged

Restore durable Codex goal prompt headroom#148
justin808 merged 8 commits into
mainfrom
codex/issue-123-prompt-headroom

Conversation

@justin808

@justin808 justin808 commented Jul 12, 2026

Copy link
Copy Markdown
Member

Closes #123

Summary

  • replace the verbose repeated prompt block with a versioned, self-contained GMCC-v1 line while retaining the full canonical expansion in workflows/pr-processing.md
  • preserve unresolved-current-head review, bounded-watch, merge-authority, PR/ad-hoc closeout, and issue-closure semantics under missing/autoload-failure references
  • raise the Codex early headroom floor from 100 to 300 characters and proactively split route groups when an otherwise-valid prompt drops below it
  • add semantic mutation coverage, exact parity checks, and both-route-group size guards

Prompt pressure replay

  • before: raw headroom 132-133 characters
  • after: raw headroom 337-338 characters
  • one-item fallback: 406-407 characters headroom
  • mixed two-route prompt: 14-15 characters headroom, now deliberately preemptive-split
  • split implementation group: 444-445 characters headroom
  • split QA group: 494-495 characters headroom
  • four-route incident remains over limit and splits as before

Validation

  • MISE_CACHE_DIR=/tmp/aw-d-i123-mise-cache RUBOCOP_CACHE_ROOT=/private/tmp/aw-d-i123-rubocop-cache bin/validate
  • goal completion contract: 24 runs, 245 assertions
  • model routing contract: 16 runs, 625 assertions
  • dispatcher contract: 54 runs, 476 assertions
  • RuboCop: 49 files, no offenses

Review history

  • TDD RED on current 132-133 headroom and under-floor mixed route fixture
  • observed Sol/high precommit review caught two GMCC semantic parity gaps; corrected wording and mutation tests now fail the old contract with seven assertions

Summary by CodeRabbit

  • Documentation

    • Clarified automated workflow completion rules using the versioned GMCC-v1 standard.
    • Improved handling of pending checks, reviews, merge authority, blockers, and automatic merge outcomes.
    • Added clearer prompt-size guidance, including 300-character headroom and route-group splitting.
    • Strengthened dispatch and triage guidance for evidence, state persistence, replay protection, and failures.
  • Tests

    • Expanded validation of completion behavior, prompt consistency, sizing limits, and auto-merge scenarios.
    • Added coverage for unresolved reviews, unknown check results, and PR-only closeout cases.

Observed model and effort provenance

Audited from Codex session turn_context records, worker handoffs, and commit timestamps. These observed routes supersede requested-route labels in earlier handoffs/comments. Git author metadata remains justin808; the table records AI content provenance separately.

Scope Requested route Observed route Result
Issue #123 implementation gpt-5.6-terra/high gpt-5.6-sol/high Requested Terra route was not used by the collaboration subagent.
Independent review gpt-5.6-sol/xhigh gpt-5.6-sol/high Review subagents inherited Sol/high.
Exact-head and current-main QA gpt-5.6-sol/xhigh gpt-5.6-sol/high QA subagent inherited Sol/high.
Coordinator integration gpt-5.6-sol/high gpt-5.6-sol/high Matched.

Commit provenance

Commit Observed provenance
164404d Restore durable Codex prompt headroom Implementation/review integration: gpt-5.6-sol/high.
d40be30 Clarify compact goal closeout semantics Implementation/review integration: gpt-5.6-sol/high.
a3d6a4b Align compact goal fallback guidance Implementation/review integration: gpt-5.6-sol/high.
801f903 Align compact review gate semantics Implementation/review integration: gpt-5.6-sol/high.
7e53a94 Fix prompt sizing list indentation Implementation and independent closeout review: gpt-5.6-sol/high.
bc55840 Fix Output list nesting Implementation and independent closeout review: gpt-5.6-sol/high.
da0bf62 Preserve configured reviewer gate in GMCC Implementation and independent closeout review: gpt-5.6-sol/high.
3656708 Align canonical auto-merge closeout semantics Implementation, independent closeout review, and exact-head QA: gpt-5.6-sol/high.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 32 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dc69b993-0969-4487-ab25-ec2144a16058

📥 Commits

Reviewing files that changed from the base of the PR and between 801f903 and 3656708.

📒 Files selected for processing (6)
  • 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
  • skills/triage/SKILL.md
  • workflows/pr-processing.md

Walkthrough

Codex prompt templates now require 300 characters of headroom and can split route groups earlier. GMCC-v1 completion semantics were standardized across workflow and skill prompts, with expanded checker and test coverage for alignment, readiness, and auto-merge closeout behavior.

Changes

Codex sizing and completion contracts

Layer / File(s) Summary
GMCC-v1 contract surfaces
skills/plan-pr-batch/SKILL.md, skills/pr-batch/SKILL.md, skills/triage/SKILL.md, workflows/pr-processing.md
Completion-state rules, version-key semantics, dispatch requirements, and auto-merge closeout behavior now use the GMCC-v1 contract.
Headroom and route-group sizing
skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md, workflows/pr-processing.md, skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
Codex sizing guidance and simulations enforce 300-character headroom, measure prompt bodies, split route groups, and report per-group metrics.
Prompt checker contract validation
skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
Reusable GMCC-v1 constants and validations verify compact semantics, canonical expansion text, and fallback phrases across prompt sources.
GMCC-v1 invariant tests
skills/pr-batch/bin/goal-completion-contract-test.rb
Tests verify byte-for-byte compact-contract alignment, unresolved review handling, readiness categories, and PR-only or ad-hoc auto-merge closeout.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: restoring Codex goal prompt headroom.
Linked Issues check ✅ Passed The PR adds the 300-char headroom floor and compact GMCC-v1 prompt contract the issue requested, while preserving the referenced behavior.
Out of Scope Changes check ✅ Passed The changes stay focused on prompt headroom, compact contract wording, and related validation/tests; no clear unrelated additions stand out.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/issue-123-prompt-headroom

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread workflows/pr-processing.md Outdated
Comment thread skills/plan-pr-batch/scripts/check_goal_prompt_size.rb Outdated
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review: Restore durable Codex goal prompt headroom (#148)

Overview

This PR raises the Codex goal-prompt headroom floor from 100 to 300 characters, proactively splits route groups when a valid-but-tight prompt drops below that floor, and replaces the verbose, repeated "Goal Mode Completion Contract" paragraph with a compact, versioned GMCC-v1 one-liner that is embedded verbatim in every goal prompt (while the full canonical wording is preserved as reference documentation in workflows/pr-processing.md). It also expands the Ruby self-test suite (check_goal_prompt_size.rb, goal-completion-contract-test.rb) with parity/mutation checks tying the compact and canonical forms together.

This repo relies heavily on natural-language prompt text to drive autonomous agent behavior, so the review focused on semantic precision of that text (since ambiguity there is the equivalent of a logic bug) as well as the Ruby validation scripts.

Findings

  • Semantic drift in the GMCC-v1 compact line (workflows/pr-processing.md, and mirrored in skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md): the canonical contract ties the "unless a real blocker prevents it" exception to both "merged" and "closed out" as a single clause. The compact line splits this into three semicolon clauses, where "target closed out" reads as an unconditional requirement, since the trailing "unless real blocker" grammatically attaches only to the last clause. Agents only ever see the compact line in actual goal prompts, so this could make an agent keep retrying/blocking on target closure even when a legitimate blocker exists, diverging from the documented canonical intent. Left an inline suggestion to reword so the exception unambiguously covers all three clauses.
  • Minor: tautological guard in check_goal_prompt_size.rb. The new if GOAL_PROMPT_MIN_HEADROOM < REQUIRED_GOAL_PROMPT_MIN_HEADROOM check can never trigger, since both constants are literal 300 defined a line apart in the same file. If it is meant as a regression guard for a future edit, a short comment would clarify intent; otherwise it reads as dead code.

Code quality / test coverage

  • The added parity tests (test_goal_prompts_retain_every_completion_invariant_inline, test_compact_prompt_contracts_stay_byte_for_byte_aligned, mutation tests) are a solid way to keep the 5+ verbatim copies of the GMCC-v1 line from drifting, using shared constants and assert_equal across all copies.
  • The split-route-group validation in check_goal_prompt_size.rb now hard-codes the assumption that the mixed-route fallback fixture stays under the Codex limit while breaching the 300-char headroom floor, rather than branching on both overflow and under-headroom cases as before. That matches the PR description stated fixture measurements, but is a bit brittle: an unrelated future doc edit that nudges fixture character counts into genuine overflow will fail with a "must stay under limit while breaching headroom floor" message instead of exercising the intended split-group behavior. Not a blocker, just worth knowing.
  • No other test coverage gaps found; the Ruby validation script is exercised via bin/validate per the PR description, and the diff includes corresponding fixture/mutation coverage for the new behavior.

Security / performance

  • No security-relevant surface (no new external inputs, no code execution changes beyond text templates and a self-test script). Performance is a non-issue since this is a doc/prompt/test-only change.

Overall this is a well-tested, self-contained change with good regression coverage for the character-budget mechanics; the main substantive concern is the ambiguous scope of the blocker exception in the new compact prompt line, since that is the text agents will actually act on.

Comment thread skills/pr-batch/SKILL.md Outdated
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review

Reviewed the diff (gh pr diff 148) plus the two ruby validation scripts in full (skills/plan-pr-batch/scripts/check_goal_prompt_size.rb, skills/pr-batch/bin/goal-completion-contract-test.rb). Note: I could not execute the ruby scripts in this sandbox (interpreter invocation required approval that wasn't available), so the analysis below is static; the PR description's reported validation runs (RuboCop, dispatcher/goal-completion/model-routing contracts) are the authoritative execution evidence.

What this PR does

  • Replaces the verbose, repeated "Goal Mode Completion Contract" prose with a compact, versioned GMCC-v1 line used in all three goal-prompt templates (workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md) and the generated-prompt requirement in skills/triage/SKILL.md, while keeping the full canonical expansion only in workflows/pr-processing.md.
  • Raises GOAL_PROMPT_MIN_HEADROOM from 100 to 300 characters and updates all four docs to require proactive route-group splitting when headroom drops below the new floor (not just on hard overflow).
  • Adds test/validation coverage: exact byte-for-byte parity checks across the four GMCC-v1 copies, semantic-invariant phrase checks, two regression tests for previously-found parity gaps (unresolved-review-thread ordering, PR-only/ad-hoc auto-merge closeout scoping), and realistic split-route-group size fixtures.

Correctness

  • I manually diffed all five verbatim GMCC-v1 occurrences (workflow x2, pr-batch, plan-pr-batch, triage) — they're byte-identical, consistent with the new test_compact_prompt_contracts_stay_byte_for_byte_aligned test.
  • Traced the compact line's semantics against the original verbose contract: the restructuring preserves every original trigger (waiting-on-checks-or-review, CI/review-agent gates, unresolved threads, failures/UNKNOWN, bounded-watch resume) and adds clarity that was previously implicit — splitting "pending/missing/untriaged CI or configured review agents" into CI/reviews/review-agents, and decomposing the auto-merge closeout into PR/target/issue so PR-only and ad-hoc (issue-less) targets aren't incorrectly blocked on "issue closed." This matches the two "semantic parity gaps" the PR description says were caught in precommit review, and the new tests (test_triaged_but_unresolved_current_head_review_thread_is_not_complete, test_auto_merge_closeout_handles_pr_only_and_ad_hoc_targets) directly guard both.
  • The check_goal_prompt_size.rb mixed-route-fallback logic was restructured (not just the constant bump): the old code branched on "already over the hard 4000 limit" vs. "under limit but low headroom," aborting outright in the latter case. The new code asserts the realistic 2-item mixed-route fixture must sit strictly under the hard limit while breaching the new 300-char floor, then validates that splitting into per-route-group prompts brings each back under budget with adequate headroom. Given headroom-floor breach (chars > 3700) always precedes hard overflow (chars ≥ 4000) for the same growing fixture, this is a reasonable tightening (fails fast on floor breach rather than silently tolerating growth all the way to full overflow) — not a correctness regression, just worth knowing if this fixture's owners later add more fields.
  • Anchor links (../../workflows/pr-processing.md#goal-mode-completion-contract) still resolve correctly since the heading name in pr-processing.md didn't change, only the body content.

Minor finding (posted inline)

  • skills/pr-batch/SKILL.md's new one-line explanation of why GMCC-v1 is self-contained uses different wording ("remain executable when the workflow reference cannot autoload") than the equivalent, validated sentences in workflows/pr-processing.md and skills/triage/SKILL.md ("remain normative when the workflow reference is missing or cannot autoload"), and drops the "missing" case. skills/plan-pr-batch/SKILL.md has no equivalent sentence at all. None of this is caught by check_goal_prompt_size.rb's GOAL_MODE_AUTOLOAD_NORMATIVE_PHRASES check, which only validates workflow_text and triage_skill_text. Low severity (the GMCC-v1 line itself is self-explanatory and byte-identical everywhere), but worth tightening given how carefully this PR otherwise keeps the four copies in sync.

Security / performance

No security-relevant surface changed (no new external input handling, no code execution paths beyond the existing internal validation tooling). Performance is a non-issue — all changes are string-length/content checks over small, fixed markdown templates.

Test coverage

Good — the PR adds targeted regression tests for the exact two semantic gaps called out in its own description, plus mutation-style byte-parity assertions that will fail loudly on any future drift between the four GMCC-v1 copies.

Comment thread skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
Comment thread workflows/pr-processing.md Outdated
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review: Restore durable Codex goal prompt headroom (#148)

Overview
This PR is a pure documentation/prompt-engineering change to the agent-workflows repo: it replaces the verbose, repeated Goal Mode Completion Contract: line embedded in every generated Codex/Claude goal prompt with a compact, versioned GMCC-v1: line, while keeping the full canonical expansion only in workflows/pr-processing.md. It also raises the Codex prompt-size 'early headroom' floor from 100 to 300 characters (GOAL_PROMPT_MIN_HEADROOM in skills/plan-pr-batch/scripts/check_goal_prompt_size.rb) and adds preemptive route-group splitting when a prompt is technically under the 4000-char limit but has thin headroom. Most of the diff is new self-consistency checks (check_goal_prompt_size.rb) and unit tests (skills/pr-batch/bin/goal-completion-contract-test.rb) asserting byte-for-byte parity between the compact and canonical forms across all generation surfaces (workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md).

What I checked

  • Read the full diff plus surrounding non-diffed context of every touched file.
  • Grepped the whole repo to confirm no stale copies of the old verbose contract line remain outside the canonical workflows/pr-processing.md source, and that the new GMCC-v1 alignment sentence appears exactly once per generation surface, matching what the tests expect.
  • Traced the Ruby validation script's control flow (headroom math, mixed-route/split-route-group fixtures, phrase/occurrence assertions) for logic errors.
  • Could not execute ruby skills/plan-pr-batch/scripts/check_goal_prompt_size.rb or the minitest suite in this sandbox (tool approval for running ruby wasn't available), so I could not independently reproduce the '24 runs / 245 assertions' etc. numbers in the PR description. This review is static-analysis only.

Code quality / correctness

  • The change is internally consistent: every markdown surface that should carry the new compact GMCC-v1: line does, exactly once, matching the literal hardcoded in both Ruby scripts (verified via grep).
  • Headroom math in assert_prompt_budget (check_goal_prompt_size.rb:289-295) is correct (headroom = LIMIT - chars, < 300 fails) and is deliberately Codex-only, matching the prose that Claude/generic don't apply the strict 4000-char limit.
  • The restructured mixed-route fixture check (check_goal_prompt_size.rb:811-846) now asserts the fixture is under the 4000 limit but breaches the 300-char floor (matching the PR's stated 14-15 char headroom replay value), then validates both split route groups. This ties the test tightly to the current byte-for-byte size of the templates — any future field/line added to the shared goal-prompt template that shifts the mixed-route fixture's headroom past 300 will trip this specific assertion with a fairly generic failure message. Not a bug, just a likely future maintenance trip-wire worth knowing about.
  • Left two inline comments: (1) on check_goal_prompt_size.rb about the compact/canonical contract strings being duplicated verbatim as separate Ruby constants in that file and in skills/pr-batch/bin/goal-completion-contract-test.rb, with nothing asserting the two Ruby copies agree with each other; (2) on workflows/pr-processing.md:954 questioning whether folding 'CI or configured review agents' into 'CI/reviews/review agents' (adding a third 'reviews' bucket not present in the canonical expansion sentence below it) is an intentional semantic addition or accidental overlap with the separately-listed 'review threads'.

Security / performance
No security-relevant surface (no untrusted input handling, no shell/system calls in the touched Ruby, pure string/file processing over the repo's own markdown). Performance is a non-issue given the small in-memory string operations.

Test coverage
Coverage is thorough for what this PR changes: exact parity assertions, semantic-invariant phrase checks, autoload-failure fallback text checks, and new split-route-group size guards — consistent with the PR's stated TDD process and independent adversarial review pass.

Overall this looks like a solid, carefully self-verified change. The two inline items are suggestions/questions rather than blockers.

Comment thread skills/pr-batch/bin/goal-completion-contract-test.rb
Comment thread skills/pr-batch/bin/goal-completion-contract-test.rb
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Summary

What this PR does: Replaces the verbose, repeated "Goal Mode Completion Contract:" prose block (previously pasted byte-for-byte into 4 goal-prompt locations) with a compact, versioned GMCC-v1: line, while keeping the full canonical English expansion in workflows/pr-processing.md for reference. It also raises the Codex prompt headroom floor from 100 to 300 characters in skills/plan-pr-batch/scripts/check_goal_prompt_size.rb, and adds proactive route-group splitting when a prompt is under the hard 4000-char limit but breaches the new floor. Test/validation coverage is expanded significantly: exact byte-for-byte parity checks, phrase-level semantic-invariant checks, mutation-style guards (refute_includes/refute_match against known-wrong compressions), and both-route-group size fixtures.

Code quality / correctness

  • Traced the compact GMCC-v1 line against the canonical Goal Mode Completion Contract: prose it replaces (in the actual dispatched-prompt text, this line is read and acted on by an LLM agent, not just a human, so semantic fidelity matters as much as it would for executable code). The four gating categories (waiting-on-checks-or-review, pending/missing/untriaged CI/review agents, unresolved review threads, failures/UNKNOWN) and the ready-no-merge-authority / auto_merge_when_gates_pass clauses all map cleanly onto the canonical wording — I didn't find a dropped or inverted condition.
  • Verified the closeout expansion (PR merged+closed out when present; target closed out; issue closed where applicable) is deliberately guarded by mutation tests (refute_includes "merged+issue closed", refute_match(/issue closed where applicable unless real blocker/)) so PR-only and ad-hoc lanes can't accidentally require a non-existent issue to close. This is good defensive test design for a prompt whose grammar is otherwise easy to mis-render during future edits.
  • Spot-checked the refactored check_goal_prompt_size.rb fixture logic around the mixed_route_fallback / split_route_groups block (lines ~811-846): the new fixture is designed to land under the hard limit but inside the 300-char floor (per the PR's own "14-15 characters headroom, now deliberately preemptive-split" note), and the assertion (unless fallback_chars < limit && headroom < GOAL_PROMPT_MIN_HEADROOM) correctly encodes that expectation. No stale/dead branch left over from the old >= limit early-return path.
  • Cross-checked all 5 prose surfaces (workflows/pr-processing.md ×2, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md) — the GMCC-v1: line and the GMCC-v1 alignment sentence are present and byte-identical everywhere the tests expect them, and the "300 characters of headroom" wording is consistent between prose and the Ruby constants that check it.
  • bin/validate wires both check_goal_prompt_size.rb and goal-completion-contract-test.rb into CI, so the passing validation run cited in the PR description should be a reliable signal; I wasn't able to execute Ruby directly in this review sandbox to independently re-run them, so I relied on careful static tracing instead.

Minor observations (posted inline, non-blocking)

  • The full GMCC-v1 line is defined as an identical literal string in two separate Ruby files (check_goal_prompt_size.rb and goal-completion-contract-test.rb) in addition to the 5 markdown copies. This matches the repo's existing "duplicate + drift-check" convention (the old contract line had the same shape), so it's not a regression, but it does mean a future wording change touches 7 places to stay green. A one-line comment cross-referencing the sibling constant would help the next editor.
  • test_skill_prose_points_to_canonical_contract_instead_of_pasting_it asserts the compact-contract occurrence count only for skills/pr-batch/SKILL.md, not skills/plan-pr-batch/SKILL.md, even though both carry the same line. The byte-for-byte alignment test elsewhere covers correctness, so this is just a slight asymmetry in intent-documentation between the two "exactly one copy" assertions, not a gap that would let a bug slip through.

Security / performance

No security-relevant surface here — this PR only edits Markdown prompt specs and local Ruby validation/test scripts that read repo files; nothing touches network input, shell execution with untrusted data, or secrets. Performance is a non-issue (small, one-shot string/regex checks over local files run in CI).

Overall this is a well-scoped, heavily self-validated change with good before/after evidence in the PR description. I didn't find any correctness bugs blocking merge.

@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: aw-d-qa; read-only exact-head Sol/xhigh lane
  • Tested at: base 9e160d03fe9dd43267377d4ca0016deb8debc024 through head 801f903632f06929c084236fbc8f758ed5c87f0d
  • Automated: completion 26/272; routing 16/579 with 2 expected skips; dispatcher 54/476; prompt guard; three mutation replays; sanitized bin/validate; hosted checks
  • Manual: baseline headroom 132-133 versus 345-346; both route groups 452-503; exact two-category gate; distinct pending-draft guard; all 7 threads resolved
  • QA status: satisfied; release-blocking: clear

@justin808
justin808 marked this pull request as ready for review July 12, 2026 23:30
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review

This PR replaces the verbose, repeated Goal Mode Completion Contract: block with a compact, versioned GMCC-v1: line embedded in every generated goal prompt (workflow doc, pr-batch, plan-pr-batch, triage), keeps the full canonical prose only in workflows/pr-processing.md, and raises the Codex prompt-size early-headroom floor from 100 to 300 characters (with proactive route-group splitting when a prompt would otherwise dip below that floor).

Verification performed

  • Diffed all six changed files and cross-checked every GMCC-v1: ... occurrence (workflow doc x2, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md) — all are byte-for-byte identical, matching the new test_compact_prompt_contracts_stay_byte_for_byte_aligned assertion.
  • Confirmed the non-prompt GMCC-v1 alignment sentence ("... its inline semantics remain normative when the workflow reference is missing or cannot autoload.") sits outside every goal-prompt fence, so it doesn't consume prompt headroom — consistent with test_non_prompt_gmcc_alignment_sentence_is_exact_on_all_generation_surfaces.
  • Verified the "300 characters of headroom" phrase counts match what check_goal_prompt_size.rb's host_aware_batch_sizing_phrase_checks expects in each file (1 occurrence in pr-processing.md/plan-pr-batch, 2 in triage/SKILL.md).
  • Traced through the restructured mixed-route/split-route-group logic in check_goal_prompt_size.rb (lines ~811-846): the old if mixed_route_fallback_chars >= limit ... else ... branching was collapsed into a single unless chars < limit && headroom < FLOOR guard followed by unconditional split-group validation — this matches the PR's stated intent (deliberately force the mixed two-route fixture into the "under limit but below the new 300-char floor" state, then validate both split groups unconditionally).
  • I could not execute ruby/bin/validate directly in this review sandbox (execution was blocked), so I relied on static/textual verification rather than running the described 96-file rubocop + Minitest suite myself. The PR description reports these all passing; worth confirming green CI before merge.

Minor observations (non-blocking)

  • In check_goal_prompt_size.rb, the new combined abort_with_failure message for the mixed-route preemptive-split check ("must stay under #{limit} while breaching the #{FLOOR}-character headroom floor") conflates two distinct failure modes (prompt overflow vs. sufficient headroom). If this fixture's byte count ever drifts back over the 4000-char limit, the failure message won't clearly say "this is now an overflow, not a headroom problem" — a cosmetic diagnostics nit only, not a functional bug.
  • The compact contract's closeout clause — PR merged+closed out when present; target closed out; issue closed where applicable — packs three closeout categories (PR, generic "target", issue) into a very terse run-on. It's unambiguous once you read the canonical expansion, and tests pin the exact wording, but it may be worth a beat re-reading for a human skimming just the compact line.
  • No CHANGELOG.md entry was added for this change under [Unreleased], unlike most other entries currently there. Not sure if that's required by this repo's convention for every PR — flagging in case it's expected.

Overall

No correctness bugs or security issues found. The change is a well-scoped, heavily self-validating documentation/prompt-engineering refactor (six files, all cross-checked by an expanded Ruby contract/size test suite), and the described "prompt pressure replay" numbers are consistent with the code paths that measure them.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/SKILL.md`:
- Around line 322-328: Adjust the indentation of the two sizing bullet items in
the goal-prompt sizing section so they use the same two-space indentation as
their sibling list items. Preserve their text, ordering, and surrounding
fence/counting instructions.
🪄 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: 7835b423-7ddb-4f7d-a5a7-f6bcb80d44fc

📥 Commits

Reviewing files that changed from the base of the PR and between 9e160d0 and 801f903.

📒 Files selected for processing (6)
  • 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
  • skills/triage/SKILL.md
  • workflows/pr-processing.md

Comment thread skills/plan-pr-batch/SKILL.md
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review

Overview: This PR reworks the "Goal Mode Completion Contract" that's embedded in generated Codex/Claude goal prompts, replacing the verbose inline copy with a compact, versioned GMCC-v1 one-liner, while keeping the full canonical prose in workflows/pr-processing.md. It also raises the Codex prompt-size headroom floor from 100 to 300 chars and adds preemptive route-group splitting when a prompt would otherwise breach that floor. Test coverage (skills/pr-batch/bin/goal-completion-contract-test.rb, skills/plan-pr-batch/scripts/check_goal_prompt_size.rb) is expanded accordingly with byte-for-byte parity checks across all four generation surfaces (workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md).

Verification: I confirmed the GMCC-v1: line is byte-identical across all four surfaces, and the GMCC-v1 alignment sentence appears exactly once in each of the four non-prompt surfaces and is correctly excluded from the fenced goal prompts (matching what the new tests assert).

Findings

  • Minor: two bullet points touched in skills/plan-pr-batch/SKILL.md (around lines 322 and 326) were re-indented from 3 spaces to 2 spaces, while every sibling bullet at that same nesting level under "4. Output" still uses 3 spaces. This is likely an unintentional whitespace slip introduced while editing those lines rather than a deliberate style change — worth normalizing back to 3 spaces for consistent list nesting/rendering (left as an inline comment).
  • The Ruby test/validation changes (goal-completion-contract-test.rb, check_goal_prompt_size.rb) look internally consistent: the compact-contract constants match across files, the preemptive-split logic now always exercises the two-route-group split path (matching the "deliberately preemptive-split" behavior described in the PR body) rather than only on overflow, and the new "both route groups validated" guard (length == 2) is a nice safety check against a silently-skipped branch.
  • No security concerns — this PR only touches documentation/prompt text and Ruby validation scripts used in CI/local bin/validate; no runtime app code, credentials, or external inputs are involved.
  • Nice touch keeping the pending-checks pressure scenario ("A batch with 5 PRs, 3 pending hosted checks...") only in the canonical workflows/pr-processing.md copy rather than duplicating it into every compact prompt — keeps the compact line lean while the tests still assert the underlying semantics via shorter phrase fragments.

Overall this is a well-scoped, well-tested doc/prompt-engineering change. The only actionable item is the stray indentation on two bullets.

Comment thread skills/plan-pr-batch/SKILL.md Outdated
Comment thread skills/plan-pr-batch/SKILL.md Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e53a949e2

ℹ️ 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".

Comment thread workflows/pr-processing.md Outdated
Comment thread workflows/pr-processing.md Outdated
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Summary

This PR reworks the Goal Mode Completion Contract into a versioned, terse GMCC-v1 line (replacing the verbose repeated block across four generation surfaces), raises the Codex headroom floor from 100 → 300 chars, and adds proactive route-group splitting plus a large amount of new test/validation coverage in check_goal_prompt_size.rb and goal-completion-contract-test.rb.

Overview

  • workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md all now carry the identical GMCC-v1: ... line plus a short "version key, not an external-only pointer" alignment sentence.
  • GOAL_PROMPT_MIN_HEADROOM raised 100 → 300; check_goal_prompt_size.rb now proactively validates that mixed-route fixtures under the hard limit but below the new headroom floor get preemptively split into per-route-group prompts, and validates both resulting groups.
  • goal-completion-contract-test.rb was restructured: the old "byte-for-byte" test now compares the compact GMCC-v1 line across surfaces instead of the old verbose line, and several new tests assert invariant ordering/wording (unresolved-review gating, PR-only/ad-hoc/issue closeout, autoload-failure fallback).

Verification performed

  • Statically cross-checked every require_occurrence_count/require_phrases assertion in check_goal_prompt_size.rb against the actual file contents (grep) — all counts and phrases line up (e.g. "300 characters of headroom" appears exactly twice in skills/triage/SKILL.md, the GMCC-v1 alignment sentence appears exactly once per surface, etc.). I could not execute ruby/bin/validate in this sandbox (command execution was blocked), so I relied on manual/static verification of the assertions rather than running the suite.
  • Traced the new headroom-floor logic in check_goal_prompt_size.rb (lines ~811-847): the mixed-route fixture is now required to be under the hard limit and below the 300-char headroom floor (rather than branching on over/under the hard limit as before), then both split route groups are validated and a count check ensures neither is skipped. This matches the PR's described "mixed two-route prompt ... now deliberately preemptive-split" behavior and looks correct.
  • RuboCop's Layout/LineLength is disabled repo-wide, so the new, longer unwrapped lines in the Ruby files won't trigger lint failures (consistent with the claimed "no offenses").

Finding

  • Left an inline comment on workflows/pr-processing.md (~line 958): the new compact GMCC-v1 line encodes more specific closeout semantics (PR-only vs. target vs. issue-closure) than the "canonical" prose it's documented as "expanding to." That canonical text (and the "Pressure checks" bullet restating it) wasn't updated to match, so the two representations of the same contract have drifted apart — worth reconciling so GMCC-v1 is genuinely a compression of the canonical spec rather than a separate, more detailed one.

Other observations (non-blocking)

  • The skills/plan-pr-batch/SKILL.md indentation fix (2-space → 3-space continuation under the Lane Card: bullet) is a good drive-by fix that aligns it with sibling bullets in the same list — not a regression.
  • Phrasing like "ready-no-merge-authority only without merge auth" in the compact line is terser than the canonical "is terminal only when merge_authority does not allow merging" — a bit elliptical, but the PR notes this already went through an independent adversarial semantic-parity pass, so I'm not flagging it as a blocker.

No security or performance concerns — this PR only touches Markdown documentation/prompt templates and Ruby validation/test scripts with no runtime/production code paths.

@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review: Restore durable Codex goal prompt headroom

Overview

This PR compacts the repeated, verbose "Goal Mode Completion Contract" prose into a versioned, self-contained GMCC-v1 line embedded in every generated goal prompt, while retaining the full canonical expansion in workflows/pr-processing.md. It also raises the Codex headroom floor from 100 to 300 characters and adds proactive route-group splitting when a prompt would otherwise dip below that floor, plus a substantial expansion of the self-verifying test/validation scripts.

I verified this statically (grep/manual cross-referencing across all 6 changed files) since the sandbox here blocks executing ruby/gh pr checks. Everything I could check lines up correctly:

  • The GMCC-v1: compact line is byte-for-byte identical across all 5 embed sites (workflows/pr-processing.md ×2, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, skills/triage/SKILL.md).
  • The `GMCC-v1` is a version key... alignment sentence appears exactly once on each of the 4 non-prompt generation surfaces, as the new test enforces.
  • The "300 characters of headroom" / "less than 300 characters of headroom" phrase counts match what check_goal_prompt_size.rb's host_aware_batch_sizing_phrase_checks expects (e.g. triage SKILL.md requires exactly 2 occurrences and has exactly 2).
  • No stale references to the old verbose Goal Mode Completion Contract: prompt-embed line remain outside the canonical workflow doc and the test files that intentionally still assert the full canonical text.

Code quality observations (non-blocking)

  1. Duplicated literal contract strings across two Ruby filesGOAL_MODE_COMPACT_CONTRACT/GOAL_MODE_CANONICAL_EXPANSION in skills/plan-pr-batch/scripts/check_goal_prompt_size.rb and COMPACT_CONTRACT_LINE/CANONICAL_CONTRACT_LINE in skills/pr-batch/bin/goal-completion-contract-test.rb are hand-duplicated, byte-for-byte, in two independent files (in addition to the markdown docs). This is consistent with the repo's existing pattern of validating prose via literal string matches, and the redundancy is presumably intentional (independent scripts, drift-detection-by-design), but it does mean a future contract wording change has to be replicated correctly in 2 Ruby constants + up to 5 markdown embeds by hand, with no single source of truth. Worth a comment noting the "keep these N places in sync" burden if not already documented somewhere central.

  2. skills/plan-pr-batch/scripts/check_goal_prompt_size.rb:814 — the preemptive-split fixture guard reads as a double negative:

    unless mixed_route_fallback_chars < limit && mixed_route_headroom < GOAL_PROMPT_MIN_HEADROOM
      abort_with_failure(...)
    end

    Logic is correct (I traced it against the PR's stated "14–15 chars headroom, now preemptive-split" scenario), but a small breaches_headroom_floor = ...; abort_with_failure(...) unless breaches_headroom_floor rename would make the intent (fixture must be under the hard limit but below the new floor) easier to read at a glance.

  3. Minor drive-by fix: skills/plan-pr-batch/SKILL.md's "Add a compact Lane Card: line" bullet had its indentation corrected from 2 to 3 spaces to match sibling bullets — harmless and arguably an improvement, but unrelated to the PR's stated purpose; worth calling out explicitly in the description if intentional (it is, just noting for the reviewer's own future grep).

Correctness / semantics

The compact GMCC-v1 line drops the illustrative "A batch with 5 PRs, 3 pending hosted checks..." example (retained only in the canonical expansion, which is fine — it's an example, not a rule) and expands "done means merged and closed out" into three explicit closeout clauses (PR merged+closed out when present, target closed out, issue closed where applicable) to correctly scope PR-only and ad-hoc (issue-less) targets. This is a genuine semantic improvement over the original wording (per the PR's own "Review history" note about a caught parity gap), not a regression.

Security / performance

No security-relevant code paths (docs + Ruby validation scripts only, no runtime app code). Performance impact is negligible — this only affects prompt-generation guidance and CI-time validation script execution.

Test coverage

Test coverage is thorough for a prompt/documentation change: mutation tests (test_compact_contract_rejects_configured_reviewer_omission), exact byte-for-byte parity checks across all embed sites, and both-route-group size guards for the new preemptive-split behavior. I was not able to execute the Ruby test suite or bin/validate in this sandboxed review environment to independently confirm the reported "24 runs / 245 assertions" etc. — recommend confirming green CI before merge if that hasn't already happened.

Overall: solid, well-tested change with strong internal consistency. No blocking issues found.

@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review

This PR compresses the verbose Goal Mode Completion Contract line into a versioned GMCC-v1 line (retaining the full canonical expansion in workflows/pr-processing.md), raises the Codex headroom floor from 100 to 300 chars, and adds preemptive route-group splitting plus a good set of new validation/parity tests.

Verification performed (static, since this sandbox blocks executing ruby/grep pipelines for approval):

  • Diffed the GMCC-v1 compact contract line and the GOAL_MODE_CANONICAL_EXPANSION/CANONICAL_CONTRACT_LINE constants in both Ruby files against the actual text in workflows/pr-processing.md, skills/pr-batch/SKILL.md, skills/plan-pr-batch/SKILL.md, and skills/triage/SKILL.md — all byte-for-byte identical across the 5 occurrences, as the new "stay byte-for-byte aligned" test expects.
  • Confirmed the GMCC_ALIGNMENT_SENTENCE ("... is a version key that pins drift ...") appears exactly once per generation surface, and zero times inside the goal-prompt fences (so it doesn't eat into the 4000-char Codex budget) — matches test_non_prompt_gmcc_alignment_sentence_is_exact_on_all_generation_surfaces.
  • Confirmed the "300 characters of headroom" / "less than 300 characters of headroom" phrase counts match what host_aware_batch_sizing_phrase_checks expects per file (2 in skills/triage/SKILL.md, 1 elsewhere).
  • Traced the rewritten mixed-route-fallback branch in check_goal_prompt_size.rb (lines ~812-847): it now unconditionally requires the Codex mixed-route fixture to stay under the 4000 limit while breaching the new 300-char floor, then validates both split route groups individually. This matches the "mixed two-route prompt ... now deliberately preemptive-split" behavior described in the PR summary, and the old branch that specially handled an over-limit mixed-route case was correctly removed (that scenario is still covered separately by the oversized_candidate fixture).
  • The skills/plan-pr-batch/SKILL.md diff includes an unrelated indentation change (2-space → 3-space) on the "Add a compact Lane Card:" bullet under item 4. This isn't a bug — it actually fixes a markdown list-nesting inconsistency (that bullet was under-indented relative to its sibling bullets and the parent 4. ordered item), so it's a welcome incidental cleanup, just worth knowing it's out of the PR's stated scope.

Bugs / security / performance: none found. This PR only touches Markdown documentation and a Ruby text-validation script with no user input or external execution surface, so there's no meaningful security or performance angle here — the main risk category is textual drift/inconsistency between the multiple copies of the contract line, and the new tests specifically close that gap (byte-for-byte alignment checks, mutation tests for the legacy closeout sentence and the configured-reviewer omission).

I wasn't able to execute bin/validate / the RuboCop and Minitest suites myself in this environment (command execution required interactive approval that wasn't available), so I relied on manual text diffing against the Ruby constants instead of re-running the suite the PR description says already passes (24/16/54 runs, 245/625/476 assertions, RuboCop clean). Recommend CI confirms that before merge.

No inline comments — I didn't find any specific lines that need to change.

@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: aw-d-qa; read-only exact-head lane
  • Scope checked: issue Recover Codex goal-prompt headroom: split-fallback prompt at 3823/4000 chars #123 headroom recovery, compact/canonical GMCC parity, configured-review-agent and pending-draft gates, route splitting, host compatibility, and current-head review gates
  • Tested at: base 9e160d03fe9dd43267377d4ca0016deb8debc024 through head 3656708b0211d313980af060d11855c0c1566c21
  • Automated checks: focused completion/routing/dispatcher contracts; prompt-size guard; legacy canonical-closeout negative mutation; sanitized bin/validate; hosted validate, Claude review, and CodeRabbit
  • Manual checks: start/mid/close SHA; raw and split headroom; configured-review-agent and pending-draft guards; canonical auto-merge parity; all review objects and threads
  • Findings: all priority findings fixed; no release-blocking findings
  • QA required: yes
  • QA required rationale: workflow-facing completion and prompt-size contracts require exact-head replay
  • QA lane status: satisfied
  • Release-blocking status: clear
  • Process-gap disposition: script

@justin808
justin808 merged commit 6271fea into main Jul 13, 2026
5 checks passed
@justin808
justin808 deleted the codex/issue-123-prompt-headroom branch July 13, 2026 01:08
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.

Recover Codex goal-prompt headroom: split-fallback prompt at 3823/4000 chars

1 participant