diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index 082a7de9..8bfbdfb8 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -1,6 +1,6 @@ --- name: ship -description: Ship altinity-sql-browser roadmap issues or phases end-to-end, autonomously — resolve scope into dependency-ordered units (a phase or a whole issue), then for each unit in turn author and approve its plan with the selected Fable or ChatGPT planner workflow (max 5 review passes), implement code and tests, open that unit's own PR, iterate a ChatGPT code review loop to certification (max 3 passes), and merge automatically when every proof condition holds — auto-chaining to the next unit off the just-merged origin/main with no re-prompting. Stops the whole run only when a unit's plan or merge proof exhausts its review passes; stops only a gated unit's own spine — every other independent unit or spine still ships in the same run — when the issue explicitly gates further phases on a new decision. Invoke as `/ship ISSUE [--planner fable|chatgpt]`, `/ship ISSUE.PHASE`, or `/ship ISSUE1,ISSUE2`. +description: Ship altinity-sql-browser roadmap issues or phases end-to-end, autonomously — resolve scope into dependency-ordered units (a phase or a whole issue), then for each unit in turn author and approve its plan with the selected ChatGPT (default) or Fable planner workflow (max 5 review passes), implement code and tests, open that unit's own PR, iterate a ChatGPT code review loop to certification (max 3 passes), and merge automatically when every proof condition holds — auto-chaining to the next unit off the just-merged origin/main with no re-prompting. Stops the whole run only when a unit's plan or merge proof exhausts its review passes; stops only a gated unit's own spine — every other independent unit or spine still ships in the same run — when the issue explicitly gates further phases on a new decision. Invoke as `/ship ISSUE [--planner chatgpt|fable]`, `/ship ISSUE.PHASE`, or `/ship ISSUE1,ISSUE2`. --- # /ship — deliver altinity-sql-browser issues autonomously @@ -98,6 +98,16 @@ This split is wired into `references/plan-review-loop.workflow.mjs`, `references/chatgpt-plan-author-loop.workflow.mjs`, and `references/code-review-pass.workflow.mjs`; keep it there when editing those scripts. +**Planner default.** `--planner` defaults to `chatgpt`: ChatGPT authors and revises +every plan draft privately, and Fable/high owns the read-only approval decision (see +"Coding vs. planning model split" just above). Pass `--planner fable` to opt back into +the original mode — a Fable/high planner writes and revises while ChatGPT reviews — +for a unit where you specifically want Fable's own authorship (e.g. matching an +existing plan-authoring style already established earlier in one issue's spine, or a +unit small/well-specified enough that ChatGPT's extra planning depth isn't worth its +slower per-pass wall-clock). Nothing about which loop runs at step 2.2 changes based on +this default beyond which Workflow script gets invoked — both are described there. + ### ChatGPT review loops - The selected plan loop and the code loop run as **Workflow scripts** — @@ -142,11 +152,12 @@ accidental context sink in a run. | `/ship 447` | all remaining phases of #447, or the whole issue if unphased | | `/ship 447.2` | phase 2 of #447 only, forced | | `/ship 424,425` | several whole issues | -| `/ship 447 --planner chatgpt` | same scope, with ChatGPT authoring/revising and Fable/high approving the plan | +| `/ship 447 --planner fable` | same scope, with a Fable/high planner authoring/revising and ChatGPT reviewing the plan | Parse the invocation with `references/parse-invocation.mjs`. `--planner` accepts -`fable` or `chatgpt` and defaults to `fable`, so every existing invocation remains -behavior-compatible. The legacy word `unattended` is accepted and ignored. +`chatgpt` or `fable` and defaults to `chatgpt` — every bare invocation now gets +ChatGPT-authored plans unless `--planner fable` opts back into the original mode. The +legacy word `unattended` is accepted and ignored. ## 1 — Orient and assemble the delivery contracts @@ -267,9 +278,9 @@ For every planner mode, assign the exact path `$TMPDIR/plan-p.md` (or `$TMPDIR/plan-.md` unphased). The path is part of the review-session identity; never move, rename, or substitute it during the loop. -**Default `fable` planner:** spawn the plan-only agent below. **ChatGPT planner:** do -not spawn this initial planner; step 2.2's dedicated workflow owns every draft and -revision. +**Default `chatgpt` planner:** do not spawn this initial planner; step 2.2's dedicated +workflow owns every draft and revision. **`--planner fable`:** spawn the plan-only +agent below. Fresh agent (`subagent_type: "general-purpose"`, **never `fork`** — a fork inherits this in-progress mutating workflow and can conclude it should finish the whole job). @@ -308,24 +319,13 @@ loops. The plan file **path** is the review-session identity; never move or rena mid-loop (footguns). 1. Write a context file to `$TMPDIR`: the issue URL, unit contract and acceptance - subset, and focused questions. For the default planner, also include the verdict + subset, and focused questions. For `--planner fable`, also include the verdict protocol — "End your review with exactly one line: `VERDICT: APPROVED` or - `VERDICT: REVISE`." + `VERDICT: REVISE`." (the default `chatgpt` planner's CLI supplies its own strict + READY/BLOCKED authoring protocol instead — do not add the VERDICT line to its + context file). 2. Launch exactly one selected loop as a Workflow and wait for its task notification. - For the default `fable` planner: - - ``` - Workflow { - scriptPath: "skills/ship/references/plan-review-loop.workflow.mjs", - args: { planFile: "", contextFile: "", unitLabel: "# phase " } - } - ``` - - Inside, each pass runs one serialized `chatgpt-review plan` call, verifies every - finding with parallel read-only agents, and folds accepted findings into the plan - file in place (rejected ones become `## Review responses` rebuttals). The 5-pass - cap is a loop bound in the script, not an instruction. - For `--planner chatgpt`: + For the default `chatgpt` planner: ``` Workflow { @@ -344,6 +344,21 @@ mid-loop (footguns). `## Review responses`. The workflow performs at most five Fable review passes. ChatGPT alone owns drafts, revisions, and verifying findings against them; Fable/high alone owns approval. + For `--planner fable`: + + ``` + Workflow { + scriptPath: "skills/ship/references/plan-review-loop.workflow.mjs", + args: { planFile: "", contextFile: "", unitLabel: "# phase " } + } + ``` + + Inside, each pass runs one serialized `chatgpt-review plan` call, verifies every + finding with parallel read-only agents, and folds accepted findings into the plan + file in place (rejected ones become `## Review responses` rebuttals). The 5-pass + cap is a loop bound in the script, not an instruction. Here Fable/high authors and + revises while ChatGPT reviews and approves via the `VERDICT:` protocol — the + reverse of the default mode's ownership split above. 3. `status: "approved"` → record the pass count and conversation URL for the ship log; proceed to 2.3. 4. `status: "blocked"` → skip the unit and report the concrete missing decision; do @@ -508,8 +523,15 @@ per the table in `references/review-loops.md`: pushed commit and separately labelled public review comment. - `no-accepted-findings` → append the rebuttals to the question file and re-invoke (spends a pass). -- `fix-failed`, `needs_human`, `error` → treat as a failed proof condition at the gate - (2.7). +- `session-cap-exhausted` → do NOT re-invoke this workflow with this session; switch + immediately to the manual continuation in `references/review-loops.md` (driving the + existing tab directly) — this is expected tooling behavior, not a failed proof + condition. +- `needs_human` → first rule out the two recoverable causes in + `references/review-loops.md`'s "Recovering a stalled or hung generation" (a stuck + live generation; a complete response the runner failed to recognize) — only then + treat it as a failed proof condition at the gate (2.7). +- `fix-failed`, `error` → treat as a failed proof condition at the gate (2.7). A **certified head** is a `certified-pending-proofs` return (completed pass, verdict `SHIP`, no accepted findings) whose reviewed SHA equals this unit's current PR head. @@ -530,9 +552,23 @@ Merge automatically — no prompt — only when ALL hold at one exact head: - required CI checks green at that head; - branch protection permits the merge. -Then `gh pr merge --merge --delete-branch` (the repo's merge-commit convention), -verify the PR reports `MERGED`, fetch `origin/main` and verify the merge, and flip this -unit's own ship-log row to `shipped`. +Then `gh pr merge --merge --delete-branch` (the repo's merge-commit convention). +`--delete-branch` makes `gh` check out the base branch (`main`) locally and try to +fast-forward it to match — this can fail with "Diverging branches can't be +fast-forwarded" even though the REMOTE merge already succeeded, if local `main` has +ever drifted (e.g. one pre-existing unrelated commit from before this run started; a +`/ship` run never advances local `main` itself, since every unit branches straight off +`origin/main`). Treat that error as informational, not a failed merge: verify with +`gh pr view --json state,mergedAt` before assuming anything went wrong. Then, +regardless of whether that error appeared, explicitly check local `main` is not +diverged — `git fetch origin && git rev-list --count main ^origin/main` — and if it's +nonzero, confirm with `git status`/`git diff --stat` that there are no uncommitted +tracked changes and the divergent commit(s) are unrelated pre-existing history +(`git log --oneline main ^origin/main`), then repair the local ref with +`git reset --hard origin/main` (a local-only ref fix, not the "never force-push or +mutate main directly" remote rule above — the remote is untouched either way). Only +after that, verify the merge landed on `origin/main` and flip this unit's own ship-log +row to `shipped`. **Any condition fails** — no certified head after 3 passes, ChatGPT unreachable or a pass incomplete, SHA drift, CI red or pending, branch protection refusal — → **FULL diff --git a/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs b/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs index 5b8c44d0..7812b246 100644 --- a/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs +++ b/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs @@ -17,13 +17,14 @@ if (!runArgs || !runArgs.issueUrl || !runArgs.planFile || !runArgs.contextFile) const AUTHOR_SCHEMA = { type: 'object', additionalProperties: false, - required: ['completed', 'planStatus', 'session', 'conversationUrl', 'blocker'], + required: ['completed', 'planStatus', 'session', 'conversationUrl', 'blocker', 'lastResponsePreview'], properties: { completed: { type: 'boolean' }, planStatus: { type: 'string', enum: ['READY', 'BLOCKED', 'INVALID'] }, session: { type: ['string', 'null'] }, conversationUrl: { type: ['string', 'null'] }, blocker: { type: ['string', 'null'] }, + lastResponsePreview: { type: ['string', 'null'], description: 'if completed=false, the last ~500 chars of the FINAL attempt\'s response_text (even if empty/partial) — diagnostic only, so a future incomplete-with-real-content-present case (observed once on #630 phase 8, never root-caused) is easier to tell apart from a genuinely empty/stuck generation without re-deriving it from scratch. null when completed=true.' }, }, } const REVIEW_SCHEMA = { @@ -55,13 +56,13 @@ for (let pass = 1; pass <= 5; pass++) { const authored = await agent( 'Run the private ChatGPT plan-author command below in Bash IN THE FOREGROUND with the Bash timeout set to 580000, redirect stdout to a JSON file under $TMPDIR, and never use run_in_background:\n\n' + `node skills/chatgpt-review/scripts/chatgpt-review.mjs plan-author ${shellQuote(runArgs.issueUrl)} --output-file ${shellQuote(runArgs.planFile)} --question-file ${shellQuote(authorContextFile)} --timeout 540${sessionFlag}\n\n` + - 'Read the JSON. A complete result has status=completed and plan_status=ready or blocked. For timed_out, rate_limited, invalid_response, or any other incomplete result, retry the same command with --session from the JSON for up to 4 total attempts; wait 90 seconds before a rate_limited retry using a small-increment loop. Never start a new conversation after a session handle exists. Map the last JSON to the schema: completed=true only for a complete ready/blocked protocol; planStatus from plan_status uppercased, otherwise INVALID; retain session, conversation_url, and blocker. The command is private and must never receive publication flags. ' + RUNNER_BOUNDARY, + 'Read the JSON. A complete result has status=completed and plan_status=ready or blocked. For timed_out, rate_limited, invalid_response, or any other incomplete result, retry the same command with --session from the JSON for up to 4 total attempts; wait 90 seconds before a rate_limited retry using a small-increment loop. Never start a new conversation after a session handle exists. Map the last JSON to the schema: completed=true only for a complete ready/blocked protocol; planStatus from plan_status uppercased, otherwise INVALID; retain session, conversation_url, and blocker; lastResponsePreview = the last ~500 characters of the FINAL attempt\'s response_text field (even if it looks empty, partial, or truncated) when completed=false, otherwise null — this is diagnostic only, to help a future incomplete-with-real-content-present case get root-caused instead of just re-observed. The command is private and must never receive publication flags. ' + RUNNER_BOUNDARY, { label: `author plan ${pass}`, phase: 'Author', schema: AUTHOR_SCHEMA, model: 'sonnet' }, ) if (!authored) return { status: 'error', reason: 'plan-author runner agent died', pass, session, conversationUrl } session = authored.session ?? session conversationUrl = authored.conversationUrl ?? conversationUrl - if (!authored.completed) return { status: 'needs_human', reason: 'plan authoring remained incomplete after retries', pass, session, conversationUrl } + if (!authored.completed) return { status: 'needs_human', reason: 'plan authoring remained incomplete after retries', pass, session, conversationUrl, lastResponsePreview: authored.lastResponsePreview } if (authored.planStatus === 'BLOCKED') { return { status: 'blocked', reason: authored.blocker ?? 'ChatGPT identified an unrecorded decision', pass, session, conversationUrl } } diff --git a/skills/ship/references/code-review-pass.workflow.mjs b/skills/ship/references/code-review-pass.workflow.mjs index b344c8b6..31059933 100644 --- a/skills/ship/references/code-review-pass.workflow.mjs +++ b/skills/ship/references/code-review-pass.workflow.mjs @@ -28,7 +28,7 @@ if (!runArgs || !runArgs.prUrl || !runArgs.questionFile || !runArgs.pass || !run const PASS_SCHEMA = { type: 'object', additionalProperties: false, - required: ['completed', 'verdict', 'session', 'conversationUrl', 'reviewedSha', 'commentUrl', 'findings'], + required: ['completed', 'verdict', 'session', 'conversationUrl', 'reviewedSha', 'commentUrl', 'findings', 'attemptsUsed', 'sessionExhausted'], properties: { completed: { type: 'boolean' }, verdict: { type: 'string', enum: ['SHIP', 'REVISE'] }, @@ -36,6 +36,8 @@ const PASS_SCHEMA = { conversationUrl: { type: ['string', 'null'] }, reviewedSha: { type: ['string', 'null'] }, commentUrl: { type: ['string', 'null'] }, + attemptsUsed: { type: 'integer', description: 'total real chatgpt-review CLI calls made this pass, including retries — a single /ship-labelled pass can spend more than 1 of the session\'s 3 total pr-mode slots' }, + sessionExhausted: { type: 'boolean', description: 'true only if a chatgpt-review call itself returned status invalid_request with "permit at most three total passes" (the session\'s hard cap was already spent before this pass could get a real review) — distinct from a generic incomplete/timeout' }, findings: { type: 'array', items: { @@ -91,12 +93,15 @@ const review = await agent( `Instead, run this command with Bash IN THE FOREGROUND, with the Bash call's own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to EXACTLY this file — do not substitute a generic name. This filename is unique to this PR and pass on purpose: a long /ship run reuses the same $TMPDIR across many PRs and passes over many hours, and a generic filename risks a LATER pass silently reading a stale file left over from an EARLIER one instead of its own real result. It publishes a PR comment:\n\n` + `node skills/chatgpt-review/scripts/chatgpt-review.mjs pr ${runArgs.prUrl} --question-file ${runArgs.questionFile} --timeout 540${sessionFlag} > ${outputFile}\n\n` + '--timeout 540 caps the script\'s OWN internal wait at 9 minutes — safely inside the Bash tool\'s 10-minute ceiling — so the process exits cleanly with valid JSON instead of being killed. A "status" of "timed_out" is EXPECTED and NORMAL here, not a failure: the script persists its session handle and conversation URL even on a timeout.\n' + - 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: SHIP" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status, retry immediately. Either way, retry the SAME chatgpt-review command, but REPLACE whatever `--session`/`--seed-from-session` flag it had with `--session ` using the "session" field from the JSON (never keep --seed-from-session, and never pass both flags — the CLI rejects that) (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt (it may already have published the comment). Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + + 'COUNT every real invocation of the chatgpt-review command below (the first one plus every retry) as you go — you will report this total as attemptsUsed. This matters because the CLI enforces a HARD 3-total-calls-per-session cap for pr mode across this session\'s entire life, not per /ship-labelled pass: if an earlier pass already needed a retry, this pass can start with fewer than 3 slots left, and if a call in THIS pass returns status "invalid_request" with a message containing "permit at most three total passes" (or the CLI exits nonzero before producing any response_text), that means the session\'s cap was already fully spent — STOP immediately, do not retry (retrying a call that already told you the session is exhausted will fail identically every time and only wastes time), set sessionExhausted=true and completed=false, and skip straight to mapping the output schema below.\n' + + 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: SHIP" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status (other than the session-exhausted case above), retry immediately. Either way, retry the SAME chatgpt-review command, but REPLACE whatever `--session`/`--seed-from-session` flag it had with `--session ` using the "session" field from the JSON (never keep --seed-from-session, and never pass both flags — the CLI rejects that) (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt (it may already have published the comment). Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + 'Then map the final JSON to the output schema:\n' + - '- completed: true if response_text contains a real, parseable, single well-formed trailing VERDICT line — regardless of the literal "status" field; false only if no such line exists after all attempts;\n' + - '- verdict: the trailing "VERDICT: " line of response_text — SHIP only for a single well-formed "VERDICT: SHIP"; anything absent, duplicated, or malformed is REVISE (fail-closed);\n' + - '- findings: every concrete actionable finding in the response, one entry each, claim self-contained;\n' + - '- session, conversationUrl, reviewedSha, commentUrl: from the returned JSON.\n' + + '- completed: true if response_text contains a real, parseable, single well-formed trailing VERDICT line — regardless of the literal "status" field; false if no such line exists after all attempts OR the session was exhausted;\n' + + '- verdict: the trailing "VERDICT: " line of response_text — SHIP only for a single well-formed "VERDICT: SHIP"; anything absent, duplicated, or malformed is REVISE (fail-closed); if sessionExhausted, still return "REVISE" (required by the schema, but the coordinator will not act on it as a real review verdict);\n' + + '- findings: every concrete actionable finding in the response, one entry each, claim self-contained; if sessionExhausted, return a single synthetic finding here too describing exactly what happened (claim + where), so the information survives even if the coordinator only looks at findings;\n' + + '- attemptsUsed: the total real CLI calls you counted above;\n' + + '- sessionExhausted: as determined above (false in the normal case);\n' + + '- session, conversationUrl, reviewedSha, commentUrl: from the returned JSON (null/unavailable fields stay null).\n' + READ_ONLY, // effort intentionally NOT 'low': this agent must genuinely wait out a real // 10-25 minute external process. 'low' effort was observed capping the agent's @@ -106,8 +111,16 @@ const review = await agent( ) if (!review) return { status: 'error', reason: 'review-runner agent died', session: runArgs.session ?? null } const session = review.session ?? runArgs.session ?? null +log(`Pass ${runArgs.pass}: ${review.attemptsUsed ?? '?'} real CLI call(s) used this pass`) +if (review.sessionExhausted) { + // The CLI's 3-total-passes-per-session cap was already spent before this pass could + // even get a real review — usually because an EARLIER pass needed an internal retry + // that silently consumed an extra slot. Distinct from a generic incomplete/timeout: + // re-invoking this workflow with the same session will fail identically every time. + return { status: 'session-cap-exhausted', reason: 'session\'s 3 total pr-mode passes already spent', session, conversationUrl: review.conversationUrl, findings: review.findings, attemptsUsed: review.attemptsUsed } +} if (!review.completed) { - return { status: 'needs_human', reason: 'review pass incomplete after one retry', session, conversationUrl: review.conversationUrl, commentUrl: review.commentUrl } + return { status: 'needs_human', reason: 'review pass incomplete after all retries', session, conversationUrl: review.conversationUrl, commentUrl: review.commentUrl, attemptsUsed: review.attemptsUsed } } const verified = (await parallel(review.findings.map((f, i) => () => @@ -123,7 +136,7 @@ const accepted = verified.filter(v => v.accepted) const rejected = verified.filter(v => !v.accepted) log(`Pass ${runArgs.pass}: verdict ${review.verdict} — ${accepted.length} accepted, ${rejected.length} rejected of ${review.findings.length} findings`) -const meta_ = { session, conversationUrl: review.conversationUrl, reviewedSha: review.reviewedSha, commentUrl: review.commentUrl, accepted, rejected } +const meta_ = { session, conversationUrl: review.conversationUrl, reviewedSha: review.reviewedSha, commentUrl: review.commentUrl, accepted, rejected, attemptsUsed: review.attemptsUsed } if (review.verdict === 'SHIP' && accepted.length === 0) { // Certification is still the coordinator's call: SHA match, green CI, branch protection. diff --git a/skills/ship/references/parse-invocation.mjs b/skills/ship/references/parse-invocation.mjs index cd230b29..6abe23b7 100644 --- a/skills/ship/references/parse-invocation.mjs +++ b/skills/ship/references/parse-invocation.mjs @@ -1,6 +1,6 @@ export function parseShipInvocation(input) { const tokens = String(input).trim().replace(/^\/ship(?:\s+|$)/, '').split(/\s+/).filter(Boolean); - let planner = 'fable'; + let planner = 'chatgpt'; let scope = null; let plannerSeen = false; for (let index = 0; index < tokens.length; index += 1) { diff --git a/skills/ship/references/per-issue-cycle.md b/skills/ship/references/per-issue-cycle.md index 4215a84d..dcfe3679 100644 --- a/skills/ship/references/per-issue-cycle.md +++ b/skills/ship/references/per-issue-cycle.md @@ -81,9 +81,10 @@ a defect, one relocation at a time. ### Plan review Every plan — regardless of risk — goes through the selected plan Workflow (`SKILL.md` -step 2.2, `references/review-loops.md`, max 5 review passes). In the default mode a -Fable/high planner writes and revises while ChatGPT reviews. With `--planner chatgpt`, -ChatGPT writes and revises while Fable/high approves. The worker's part in default mode: +step 2.2, `references/review-loops.md`, max 5 review passes). By default (`--planner +chatgpt`), ChatGPT writes and revises the plan privately while Fable/high approves it +read-only. With `--planner fable`, a Fable/high planner writes and revises while +ChatGPT reviews. The worker's part in `--planner fable` mode: - write the plan to the exact file path the coordinator assigned, and return it — self-contained, because the loop's revise agent (not you) folds review findings into @@ -93,9 +94,9 @@ ChatGPT writes and revises while Fable/high approves. The worker's part in defau - never invoke `chatgpt-review` yourself, and write no code before the coordinator reports the plan approved. -In ChatGPT mode the author workflow fulfills the first bullet. The fresh implementation -worker still re-reads the approved canonical plan and observes the same no-code-before- -approval and no-`chatgpt-review` boundaries. +In the default ChatGPT mode the author workflow fulfills the first bullet. The fresh +implementation worker still re-reads the approved canonical plan and observes the same +no-code-before-approval and no-`chatgpt-review` boundaries. ## 2 — Implement (inner loop) @@ -190,7 +191,15 @@ Concentrate on: detect drift. 5. Check whether each fix removes the defect or merely relocates it. 6. Check compatibility, accessibility, cancellation, cleanup, and error paths. -7. Report only concrete actionable issues. +7. If this diff touches or adds a prefilter gating an expensive real-parser/ + architecture check (e.g. anything in `build/check-boundaries.mjs` or + `build/lib/check-legacy-owners.mjs`), check every OTHER prefilter in the same file + for the identical unsound pattern (a bare `source.includes(name)` with no escape + awareness, or a resolved-path comparison with no `fs.realpathSync` symlink + canonicalization) — don't stop at the one being modified. This exact bug class + recurred at least 4 times across issue #630, twice on the same guard within one + phase, because each fix addressed only the flagged instance. +8. Report only concrete actionable issues. Read-only. Do not edit files or mutate git, GitHub, tasks, or memory. ``` diff --git a/skills/ship/references/repo-footguns.md b/skills/ship/references/repo-footguns.md index 9d010659..c039663e 100644 --- a/skills/ship/references/repo-footguns.md +++ b/skills/ship/references/repo-footguns.md @@ -91,6 +91,21 @@ repo; when one bites anyway, update this file in the same change. - A stale server already serving `dist/` picks up a fresh build per request — usually no restart is needed at all. +## Architecture guards (`build/check-boundaries.mjs`) + +- A prefilter gating an expensive real-parser check (`findModuleSpecifiers`) must be + escape-aware (`mightReferencePackage`-style: any backslash routes the file through + the real parser) — a bare `source.includes(name)` prefilter silently skips the + parser for an escaped specifier that decodes to the same banned name. This recurred + at least 4 times across issue #630, twice on the *same* guard within one phase: + fixing the drift (production and its test mirror sharing one prefilter helper) is a + different fix from that shared helper actually being sound — don't conflate the two. +- Any check comparing a resolved filesystem path against a forbidden directory prefix + must canonicalize with `fs.realpathSync` first: `node_modules/` + is a real symlink (`package-lock.json`'s `"link": true` entries confirm which), and + a relative import routed through it resolves to a path that never matches the + forbidden prefix lexically even though it's the same file on disk. + ## Issue and phase state - PR titles with phase counts (`(2/3)`) go stale when a phase count is re-scoped diff --git a/skills/ship/references/review-loops.md b/skills/ship/references/review-loops.md index 4970b544..c3ac4636 100644 --- a/skills/ship/references/review-loops.md +++ b/skills/ship/references/review-loops.md @@ -9,8 +9,8 @@ and a crashed run resumes with `resumeFromRunId` instead of re-orienting. The scripts live next to this file and are invoked by `scriptPath` — never paste their bodies inline, and change them here so there is one copy: -- `plan-review-loop.workflow.mjs` — default mode: Fable/high authors and ChatGPT reviews. -- `chatgpt-plan-author-loop.workflow.mjs` — `--planner chatgpt`: ChatGPT authors and Fable/high approves. +- `chatgpt-plan-author-loop.workflow.mjs` — default mode: ChatGPT authors and Fable/high approves. +- `plan-review-loop.workflow.mjs` — `--planner fable`: Fable/high authors and ChatGPT reviews. - `code-review-pass.workflow.mjs` — exactly one PR review pass per call, scoped to one unit's own PR (SKILL.md step 2.6). `pass`/`session` reset to `1`/`null` for every unit — never carried over from a previous unit's loop, even on the same spine. @@ -91,6 +91,32 @@ its task notification; do not poll and do not start other review work meanwhile. `--seed-from-session`) is correct only when starting a genuinely new unit that has never had one. +- **Recovering a stalled or hung generation.** Across every phase of issue #630 from + plan authoring onward, ChatGPT's own live tool calls (GitHub/repo investigation) + repeatedly stalled mid-turn for 5-10+ minutes — a different failure from the + plan-author completion-detection bug fixed in PR #650 (that one is a response that + already finished but wasn't recognized; this one is a response still actively + "generating" with no new content appearing). **Before treating any loop's + `needs_human`/`error`/incomplete return as `SKILL.md`'s FULL STOP, rule out these two + usually-recoverable causes first** — most `needs_human` returns observed across + #630 were one of these, not a genuine plan/review impasse requiring the human's + judgment: + 1. **A stalled generation.** Symptom: the tab shows a still-"generating" state (a + stop control present, e.g. `document.querySelector('[data-testid="stop-button"]')` + truthy) with no growth in the last assistant turn's content for several minutes. + Recovery: drive the existing tab directly (same technique as the pass-cap + paragraph above) — click the stop control, then send a short nudge in the SAME + conversation ("continue without further tool calls" or similarly specific), and + wait again. Never open a new session for this; it is not a conversation problem. + 2. **A complete response already sitting in the DOM that a scripted runner's + completion check failed to recognize.** Read the last assistant turn's full text + directly from the tab before concluding nothing usable exists — a well-formed + verdict/plan-status line can be present even when the runner reported + incomplete. + + Only after ruling out both (or after they fail to actually resolve it) does + `SKILL.md`'s FULL STOP apply. + ## Default plan loop — `plan-review-loop.workflow.mjs` ``` @@ -141,7 +167,10 @@ finding's evidence lives for this loop, since there is no `rejected` return valu loop stops at the first Fable `APPROVED`, skips the unit on a concrete `BLOCKED`, and returns `needs_human` (with the last round's raw `findings`, not an accepted/rejected split) after five non-approved Fable passes or an authoring response that remains -incomplete after bounded same-session retries. +incomplete after bounded same-session retries — the latter case also carries +`lastResponsePreview` (the final attempt's last ~500 raw characters) purely for +diagnosis; before treating either as `SKILL.md`'s FULL STOP, check the two recoverable +causes in "Recovering a stalled or hung generation" above. ## Code review pass — `code-review-pass.workflow.mjs` @@ -163,10 +192,20 @@ Coordinator loop per pass (max 3; the CLI errors on pass 4): | `certified-pending-proofs` | `VERDICT: SHIP`, no accepted findings | proceed to the gate (step 2.7) — still check SHA match, green CI, branch protection yourself | | `fixed-await-push` | accepted findings fixed, gate green, local commits made | verify the diff yourself, push, wait for green CI keyed on the head SHA, re-invoke with `pass+1` and the returned `session` | | `no-accepted-findings` | REVISE, but nothing survived verification | append the rebuttals to the question file; re-invoke (spends a pass) — or go to the gate's FULL STOP if this repeats | +| `session-cap-exhausted` | the session's 3 total `pr`-mode CLI calls were already spent before this pass could get a real review (an earlier pass's own internal retry silently ate an extra slot) | **do not re-invoke this workflow with this session** — switch immediately to the manual continuation below | | `fix-failed` | fix agent died or could not reach a green gate | **FULL STOP** at the gate with the gate tail | -| `needs_human` | pass still incomplete after 4 resume attempts | **FULL STOP** at the gate | +| `needs_human` | pass still incomplete after 4 resume attempts | rule out the two recoverable causes in "Recovering a stalled or hung generation" below FIRST; only **FULL STOP** at the gate if neither applies | | `error` | runner agent died | inspect the journal; the pass may have published — check the PR before re-invoking | The workflow never pushes: `fixed-await-push` commits stay local until the coordinator has diffed and pushed them. Certification remains the coordinator's judgment at the gate; `certified-pending-proofs` is necessary, not sufficient. + +**A `/ship`-labelled pass can consume more than one of the session's 3 total `pr`-mode +slots**, since the review-runner's own up-to-4-attempt retry logic (for a stalled or +incomplete generation) makes real CLI calls that count against the same cap the +workflow's `pass` argument does not track. Do not assume 3 successful `/ship pass` +invocations are guaranteed — `session-cap-exhausted` (or a plain CLI `invalid_request` +rejection, if driving `chatgpt-review` directly) is an expected possible outcome of +*any* pass, not only a literal 4th call. `attemptsUsed` in every return tells you how +many real slots that pass actually spent, for the ship-log record. diff --git a/skills/ship/tests/invocation.test.mjs b/skills/ship/tests/invocation.test.mjs index 8a1a355b..3d605c03 100644 --- a/skills/ship/tests/invocation.test.mjs +++ b/skills/ship/tests/invocation.test.mjs @@ -2,15 +2,15 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { parseShipInvocation } from '../references/parse-invocation.mjs'; -test('existing /ship invocations retain the fable planner default', () => { - assert.deepEqual(parseShipInvocation('/ship 447'), { scope: '447', planner: 'fable' }); - assert.deepEqual(parseShipInvocation('447.2 unattended'), { scope: '447.2', planner: 'fable' }); - assert.deepEqual(parseShipInvocation('/ship 424,425'), { scope: '424,425', planner: 'fable' }); +test('existing /ship invocations default to the ChatGPT planner', () => { + assert.deepEqual(parseShipInvocation('/ship 447'), { scope: '447', planner: 'chatgpt' }); + assert.deepEqual(parseShipInvocation('447.2 unattended'), { scope: '447.2', planner: 'chatgpt' }); + assert.deepEqual(parseShipInvocation('/ship 424,425'), { scope: '424,425', planner: 'chatgpt' }); }); -test('the ChatGPT planner is selected explicitly', () => { - assert.deepEqual(parseShipInvocation('/ship 447 --planner chatgpt'), { scope: '447', planner: 'chatgpt' }); - assert.deepEqual(parseShipInvocation('447 --planner fable'), { scope: '447', planner: 'fable' }); +test('the Fable planner is selected explicitly (opt-out from the ChatGPT default)', () => { + assert.deepEqual(parseShipInvocation('/ship 447 --planner fable'), { scope: '447', planner: 'fable' }); + assert.deepEqual(parseShipInvocation('447 --planner chatgpt'), { scope: '447', planner: 'chatgpt' }); }); test('invalid planner and scope arguments fail closed', () => {