Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
| Batch QA Lane:<owner/scope+evidence|none+rationale> | ||
| Scope:titles/deps/exclusions/owners;STAGE_DEPENDENCY_PLAN_PATH=<p>,STAGE_DEPENDENCY_PLAN_ID=<id>,live=<replay/ref>;ft=refs/paths/create/delete/rename/collisions/owner/serial/UNKNOWN | ||
| Batch QA Lane:<owner|none> | ||
| Scope:titles/deps/exclusions/owners;STAGE_DEPENDENCY_PLAN_PATH=<p>,STAGE_DEPENDENCY_PLAN_ID=<id>,live=<replay/ref>,expected-terminal=<terminal>;ft=refs/paths/create/delete/rename/collisions/owner/serial/UNKNOWN |
There was a problem hiding this comment.
Spec gap: expected-terminal=<terminal> is added to this compact handoff Scope: line (and mirrored in skills/plan-pr-batch/SKILL.md:836), but its semantics are never defined anywhere in the repo:
- No doc says what values
<terminal>may take, or who is supposed to populate it. - The executing skill that actually consumes this handed-off prompt,
skills/pr-batch/SKILL.md(line ~560), deliberately keeps the old Scope line without this field — confirmed by the new testtest_planning_goal_prompts_carry_expected_terminal_only_on_planning_surfacesinskills/pr-batch/bin/goal-completion-contract-test.rb. So there is no consumer logic anywhere for this field. - Contrast with its neighbors on the same line:
STAGE_DEPENDENCY_PLAN_PATH/STAGE_DEPENDENCY_PLAN_IDandlive=<replay/ref>are fully specified under "Stage-Typed Dependency Gate" earlier in this file (~lines 155-184). - It's also modeled as a single batch-level value, while the repo elsewhere tracks per-target terminal state (see this file's own "per-target terminal state" requirement around line 2187) — a heterogeneous batch can't express differing expected outcomes per target with one field.
Failure scenario: a coordinator produces this prompt, has to fabricate/guess a value for <terminal> since nothing defines it, and the downstream executor has no rule telling it what to do with that value (or the field simply becomes inert dead data).
Recommend either defining expected-terminal's value set/consumer explicitly (e.g. alongside the Stage-Typed Dependency Gate section), or dropping it until that's designed.
| PLANNING_STAGE_DEPENDENCY_SCOPE_LINE = "Scope:titles/deps/exclusions/owners;" \ | ||
| "STAGE_DEPENDENCY_PLAN_PATH=<p>,STAGE_DEPENDENCY_PLAN_ID=<id>," \ | ||
| "live=<replay/ref>,expected-terminal=<terminal>;" \ | ||
| "ft=refs/paths/create/delete/rename/collisions/owner/serial/UNKNOWN" |
There was a problem hiding this comment.
Simplification: PLANNING_STAGE_DEPENDENCY_SCOPE_LINE (lines 130-133) copy-pastes the entire STAGE_DEPENDENCY_SCOPE_LINE literal (lines 134-137) — same prefix Scope:titles/deps/exclusions/owners;STAGE_DEPENDENCY_PLAN_PATH=<p>,STAGE_DEPENDENCY_PLAN_ID=<id>,live=<replay/ref> and same suffix ;ft=refs/paths/create/delete/rename/collisions/owner/serial/UNKNOWN, only inserting ,expected-terminal=<terminal> in between. A third near-duplicate, TRIAGE_STAGE_DEPENDENCY_SCOPE_LINE, already exists a few lines below.
Since these are independent string literals (not built from a shared sub-constant the way most other lines in this file reuse #{OTHER_LINE} interpolation), a future edit to the shared prefix or suffix in one constant can silently diverge from the others; require_occurrence_count would only report an opaque "missing phrase" failure rather than surfacing the actual drift.
Consider deriving the prefix/suffix once and building the two (or three) variants via interpolation, e.g.:
STAGE_DEPENDENCY_SCOPE_PREFIX = "Scope:titles/deps/exclusions/owners;STAGE_DEPENDENCY_PLAN_PATH=<p>,STAGE_DEPENDENCY_PLAN_ID=<id>,live=<replay/ref>"
STAGE_DEPENDENCY_SCOPE_SUFFIX = ";ft=refs/paths/create/delete/rename/collisions/owner/serial/UNKNOWN"
STAGE_DEPENDENCY_SCOPE_LINE = "#{STAGE_DEPENDENCY_SCOPE_PREFIX}#{STAGE_DEPENDENCY_SCOPE_SUFFIX}"
PLANNING_STAGE_DEPENDENCY_SCOPE_LINE = "#{STAGE_DEPENDENCY_SCOPE_PREFIX},expected-terminal=<terminal>#{STAGE_DEPENDENCY_SCOPE_SUFFIX}"|
🤖 Codex Closing this stale stacked implementation without merging. Its The useful acceptance cases have been transferred to issue #477 as a typed per-target design requirement. The branch remains available as reference; no #714 or #477 completion is implied. |
Fixes #715
Depends on #714.
Summary:
expected-terminalto the human-readable planning prompt scope lines.Verification:
ruby skills/pr-batch/bin/goal-completion-contract-test.rbAGENT_WORKFLOWS_SOURCE_CHECKOUT=1 ruby skills/plan-pr-batch/scripts/check_goal_prompt_size.rbgit diff --checkbin/lint(rubocop and markdownlint passed; blocked by missingyamllint)