Define planning-chat lifecycle and replay contract - #157
Conversation
|
Warning Review limit reached
Next review available in: 55 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 (7)
WalkthroughThe PR defines portable planning-chat roles and archival rules, moves completed-batch audit ownership to batch coordinators, adds strict audit-marker replay semantics, and expands contract tests and prompt validation for lifecycle, reconciliation, readiness, and final conversation status behavior. ChangesPlanning-chat and completed-batch lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PlanningChat as Planning chat
participant BatchCoordinator as Batch coordinator
participant AuditMarker as Completed-batch audit marker
participant ParentOrchestrator as Parent orchestrator
PlanningChat->>BatchCoordinator: durable handoff
BatchCoordinator->>AuditMarker: emit validated audit marker
AuditMarker->>ParentOrchestrator: durable audit handoff
ParentOrchestrator->>PlanningChat: reconcile state and render final status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review SummaryThis PR reassigns ownership of the completed-batch audit from the "parent orchestration agent" to the "batch coordinator," formalizes a two-role "Planning-Chat Lifecycle" ( Note: I was unable to execute the Ruby test suites in this sandboxed review environment (shell commands required interactive approval that wasn't available), so I could not independently confirm the "66 runs / 977 assertions" pass claim from the PR description — this review is based on static reading of the diff. Please make sure CI is green before merging. Strengths
Findings (posted inline)
Other observations (not blocking)
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
skills/post-merge-audit/SKILL.md (1)
371-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the shared case-fold contract implementation-neutral.
The portable documentation currently exposes Ruby syntax, while both contract suites require that syntax verbatim.
skills/post-merge-audit/SKILL.md#L371-L371: specify Unicode full case folding without naming a Ruby method.workflows/post-merge-audit.md#L56-L56: mirror the implementation-neutral wording.skills/post-merge-audit/bin/post-merge-audit-policy-test.rb#L40-L40: assert the semantic wording.skills/pr-batch/bin/goal-completion-contract-test.rb#L98-L98: keepdowncase(:fold)confined to implementation-level tests.As per coding guidelines, shared skill files must remain concise and portable.
🤖 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/post-merge-audit/SKILL.md` at line 371, Replace the Ruby-specific case-fold wording with implementation-neutral “Unicode full case folding” in skills/post-merge-audit/SKILL.md lines 371-371 and workflows/post-merge-audit.md lines 56-56. Update the semantic assertion in skills/post-merge-audit/bin/post-merge-audit-policy-test.rb lines 40-40 accordingly, while retaining downcase(:fold) only in the implementation-level assertion in skills/pr-batch/bin/goal-completion-contract-test.rb line 98.Source: Coding guidelines
skills/pr-batch/SKILL.md (1)
515-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider referencing the canonical marker grammar instead of duplicating it.
Lines 534–542 duplicate the completed-batch audit marker grammar verbatim from
workflows/pr-processing.mdlines 1611–1619. The skill file already defers to the canonical closeout lane at lines 515–516 ("follow the canonical closeout lane in.agents/workflows/pr-processing.md"), so the inline duplication creates a drift risk — the contract test incheck_goal_prompt_size.rbvalidates againstpr-processing.md, not this copy, so silent divergence would go undetected.If the duplication is intentional for self-containment, consider adding a sync comment (like the one at line 400:
<!-- Keep this handoff summary in sync with ... -->) to flag the paired location. As per coding guidelines,skills/*/SKILL.mdfiles should be kept concise and portable.♻️ Proposed sync comment
<!-- Keep this marker grammar in sync with `.agents/workflows/pr-processing.md` -> `### Planning-Chat Lifecycle` marker paragraphs. --> Replay only the exact versioned `<!-- completed-batch-audit v1` wrapper through its single final `-->`,🤖 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/SKILL.md` around lines 515 - 542, Replace the duplicated completed-batch audit marker grammar in the numbered sequence with a concise reference to the canonical definition in `.agents/workflows/pr-processing.md`, preserving the existing delegation at the start of the sequence. If the inline details must remain for self-containment, add a sync comment identifying that canonical source and the paired section instead of changing the grammar.Source: Coding guidelines
🤖 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 627-649: Remove the conditional duplicate require_phrases call for
PARENT_RELEASE_OR_ARCHIVE_PRESSURE_SCENARIOS in the enforce_restart_docs_drift
block, since the same check already runs unconditionally against
planning_chat_lifecycle_text. Only retain it if the intended source-checkout
comparison uses a distinct source-checkout text variable, and update the
argument accordingly.
In `@skills/pr-batch/bin/goal-completion-contract-test.rb`:
- Around line 435-469: Update followups_disposition_records and its
value-validation rules to reserve or escape the record delimiters semicolon and
pipe consistently across ref, owner, status, disposition, and evidence fields.
Ensure accepted records round-trip through the split/parsing logic, reject
unescaped delimiter-containing values if that is the chosen contract, and add
fixtures covering delimiter characters in each relevant value type.
In `@workflows/post-merge-audit.md`:
- Around line 50-75: Scope the completed-batch rules under their mode guard in
workflows/post-merge-audit.md (lines 50-75), using a nested block or dedicated
subsection so ownership and marker requirements are structurally conditional.
Update skills/post-merge-audit/bin/post-merge-audit-policy-test.rb (lines
177-186) to verify those ownership and marker rules occur within the
completed-batch-only scope rather than merely checking their text globally.
---
Nitpick comments:
In `@skills/post-merge-audit/SKILL.md`:
- Line 371: Replace the Ruby-specific case-fold wording with
implementation-neutral “Unicode full case folding” in
skills/post-merge-audit/SKILL.md lines 371-371 and workflows/post-merge-audit.md
lines 56-56. Update the semantic assertion in
skills/post-merge-audit/bin/post-merge-audit-policy-test.rb lines 40-40
accordingly, while retaining downcase(:fold) only in the implementation-level
assertion in skills/pr-batch/bin/goal-completion-contract-test.rb line 98.
In `@skills/pr-batch/SKILL.md`:
- Around line 515-542: Replace the duplicated completed-batch audit marker
grammar in the numbered sequence with a concise reference to the canonical
definition in `.agents/workflows/pr-processing.md`, preserving the existing
delegation at the start of the sequence. If the inline details must remain for
self-containment, add a sync comment identifying that canonical source and the
paired section instead of changing the grammar.
🪄 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: fed60feb-be18-42cc-a223-ed55e79eede0
📒 Files selected for processing (10)
CHANGELOG.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/post-merge-audit/SKILL.mdskills/post-merge-audit/bin/post-merge-audit-policy-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/goal-completion-contract-test.rbskills/triage/SKILL.mdworkflows/post-merge-audit.mdworkflows/pr-processing.md
![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: 76ebd99d86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/pr-batch/SKILL.md (1)
536-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompleted-batch audit contract is duplicated verbatim across three files.
The same detailed contract text — batch_id rules, record fields, disposition enums, normalization/dedup logic, delimiter rejection, well-formedness/archive-ready/blocker-union semantics, and replay status-line rules — appears in all three files. Any contract change must be manually mirrored in every location, risking drift. The coding guidelines require keeping
skills/*/SKILL.mdfiles concise; both SKILL.md files contain the full block when one should be canonical and the other should reference it.
skills/pr-batch/SKILL.md#L536-L536: Replace the duplicated contract block with a reference to the canonical source inskills/post-merge-audit/SKILL.md(or a shared workflow section), following the same reference pattern already used at lines 515-516 for the closeout lane.skills/post-merge-audit/SKILL.md#L371-L392: Keep as the canonical source for the completed-batch audit contract, or extract to a shared workflow section that both SKILL.md files reference.workflows/post-merge-audit.md#L51-L75: This file's duplication is acknowledged as intentional for self-containment (lines 17-19). If the SKILL.md files are deduplicated, consider whether this file should also reference the canonical source or retain the full text for copy-paste audits.🤖 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/SKILL.md` at line 536, Deduplicate the completed-batch audit contract: in skills/pr-batch/SKILL.md at lines 536-536, replace the full block with a reference to the canonical contract in skills/post-merge-audit/SKILL.md at lines 371-392, preserving the existing reference pattern near lines 515-516. Keep skills/post-merge-audit/SKILL.md lines 371-392 as the sole canonical source, or move the contract to a shared workflow section referenced by both files. For workflows/post-merge-audit.md lines 51-75, retain the self-contained contract unless the implementation deliberately changes it to reference the canonical source.Source: Coding guidelines
skills/post-merge-audit/SKILL.md (1)
371-371: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated completed-batch audit contract text across three files.
Lines 371 and 392 are parts of the same detailed completed-batch audit contract that appears verbatim in
skills/pr-batch/SKILL.md(line 536) andworkflows/post-merge-audit.md(lines 55-59). Any contract change must be mirrored in all three locations. The coding guidelines require keepingskills/*/SKILL.mdfiles concise — this duplicated block works against that goal. Deferring to consolidated comment for the cross-file recommendation.Also applies to: 392-392
🤖 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/post-merge-audit/SKILL.md` at line 371, Consolidate the duplicated completed-batch audit contract currently embedded in the post-merge audit skill, pr-batch skill, and post-merge workflow, replacing repeated prose with a single authoritative reference while preserving all validation and normalization requirements. Update the affected sections around the completed-batch follow-up contract and ensure future changes have one source of truth.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@skills/post-merge-audit/SKILL.md`:
- Line 371: Consolidate the duplicated completed-batch audit contract currently
embedded in the post-merge audit skill, pr-batch skill, and post-merge workflow,
replacing repeated prose with a single authoritative reference while preserving
all validation and normalization requirements. Update the affected sections
around the completed-batch follow-up contract and ensure future changes have one
source of truth.
In `@skills/pr-batch/SKILL.md`:
- Line 536: Deduplicate the completed-batch audit contract: in
skills/pr-batch/SKILL.md at lines 536-536, replace the full block with a
reference to the canonical contract in skills/post-merge-audit/SKILL.md at lines
371-392, preserving the existing reference pattern near lines 515-516. Keep
skills/post-merge-audit/SKILL.md lines 371-392 as the sole canonical source, or
move the contract to a shared workflow section referenced by both files. For
workflows/post-merge-audit.md lines 51-75, retain the self-contained contract
unless the implementation deliberately changes it to reference the canonical
source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc48952f-2022-41f3-abfb-8280de5e9dfd
📒 Files selected for processing (7)
skills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/post-merge-audit/SKILL.mdskills/post-merge-audit/bin/post-merge-audit-policy-test.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/goal-completion-contract-test.rbworkflows/post-merge-audit.mdworkflows/pr-processing.md
💤 Files with no reviewable changes (1)
- skills/plan-pr-batch/scripts/check_goal_prompt_size.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- workflows/pr-processing.md
Review: Define planning-chat lifecycle and replay contractOverviewThis PR is entirely documentation/spec + contract-test changes (no product runtime code): it defines a new "Planning-Chat Lifecycle" section in Code quality / conventions
Potential issues (posted inline)
Test coverageVery thorough — the new fixture matrix covers delimiter escaping, Unicode canonicalization/case-folding, embedded HTML-comment-token injection, duplicate/case-insensitive ref collisions, and cross-field consistency. I could not execute the ruby test suites in this sandboxed review environment to independently confirm they pass as claimed in the PR description — recommend confirming green CI before merge if not already gated. SecurityNo concerns — these are prose specs and test fixtures for LLM-agent behavior, not executable product code with external inputs. The marker grammar is defensively designed: it explicitly rejects embedded PerformanceN/A — no hot-path runtime code; the Ruby helpers only run in test/CI context. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8882a013
ℹ️ 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".
ReviewThis PR defines the planning-chat lifecycle (prompt-only vs. parent-orchestrator) and a fail-closed Code quality / maintainability
Minor
Security / performanceNo security concerns — this PR only changes prose specs and offline Ruby validation scripts, with no user input handling, network calls, or runtime code paths. Performance is a non-issue (validators run only in CI/manually against small markdown files); a couple of the new regexes (e.g. Test coverageThe new fixture matrix in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13603999f4
ℹ️ 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".
ReviewOverviewThis PR (closes #117) formalizes a "Planning-Chat Lifecycle" ( Correctness (reference parser logic)I traced the new marker-parsing helpers ( Consistency across surfacesI grepped for the old "parent orchestration agent must run the completed-batch audit" phrasing and for any other Maintainability concern (duplication)The same very large paragraph (marker grammar: Security / PerformanceNo runtime application code changed — this is documentation plus CI-only test tooling (fixtures are hardcoded, not fed from untrusted external input), so there's no security-relevant attack surface and no performance-sensitive path introduced. Test coverageThe fixture matrix is unusually thorough (fail-closed cases for malformed wrappers, embedded HTML-comment tokens, control characters, Unicode confusables like |
Address-review summaryScan scope: full PR review history through 2026-07-13T17:47:37Z at exact head Mattered
Optional
Skipped
Validation and disposition
Future review scans start after this marked summary unless the request explicitly says |
QA Evidence
|
1 participant
Closes #117\n\n## Summary\n- define the prompt-only and parent-orchestrator planning-chat lifecycle\n- require durable handoff to a distinct batch coordinator when the parent retains planning duties\n- add fail-closed complete-batch audit replay grammar and lifecycle fixtures\n- harden canonical finding/reference handling, blocker unions, and final readiness rendering\n\n## Validation\n- ruby skills/pr-batch/bin/goal-completion-contract-test.rb (66 runs, 977 assertions)\n- ruby skills/post-merge-audit/bin/post-merge-audit-policy-test.rb (14 runs, 242 assertions)\n- ruby skills/plan-pr-batch/scripts/check_goal_prompt_size.rb\n- bin/validate\n- exact-head Sol/xhigh review: CLEAN\n- exact-head Sol/xhigh QA: SATISFIED, identical START/MID/END receipt on 786a8b6
Summary by CodeRabbit
Documentation
Conversation statuswording for both planning and triage workflows.UNKNOWN.Tests