diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index 8bfbdfb8..14b5f458 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -137,6 +137,10 @@ this default beyond which Workflow script gets invoked — both are described th dropped: the default/code loops return `accepted` and `rejected` lists to record; the ChatGPT-author loop's rejections land as `## Review responses` entries in the plan itself, and a `needs_human` outcome there carries the round's raw `findings`. +- Beyond the two formal loops, the coordinator may consult ChatGPT ad hoc for a + genuinely hard judgment call — never pass-counted (it isn't a `chatgpt-review` + invocation, so it doesn't conflict with the single-permitted-invocation rule above), + never a substitute for internal review; see `references/review-loops.md`. ### Output capture diff --git a/skills/ship/references/per-issue-cycle.md b/skills/ship/references/per-issue-cycle.md index dcfe3679..bcbccd01 100644 --- a/skills/ship/references/per-issue-cycle.md +++ b/skills/ship/references/per-issue-cycle.md @@ -175,7 +175,10 @@ Before handing the unit back, verify: - **Security-sensitive** (auth / OAuth / `config.json`): add one focused pass with the `security-review` skill. -Do not run a generic code review plus a multi-agent review over the same diff. +Do not run a generic code review plus a multi-agent review over the same diff. None of +these tiers include asking ChatGPT ad hoc — that's a distinct, coordinator-only escalation +for when internal review genuinely can't resolve a tradeoff (`references/review-loops.md`), +not a substitute for running this budget first. Reviewer prompt: @@ -220,7 +223,9 @@ When a finding exposes a **missing invariant** rather than an isolated bug: If two review rounds find variants of one root cause, revise the invariant map before writing another fix. Fixes that relocate a defect are how a one-pass review becomes -four. +four. If even this process doesn't settle which structural fix is right, +`references/review-loops.md`'s ad hoc consultation procedure is for exactly this — a +genuine tradeoff internal review can't resolve, not routine debugging. ## 4 — Reconcile (before the PR — and before certification) diff --git a/skills/ship/references/review-loops.md b/skills/ship/references/review-loops.md index c3ac4636..fc18048c 100644 --- a/skills/ship/references/review-loops.md +++ b/skills/ship/references/review-loops.md @@ -117,6 +117,33 @@ its task notification; do not poll and do not start other review work meanwhile. Only after ruling out both (or after they fail to actually resolve it) does `SKILL.md`'s FULL STOP apply. +- **Ad hoc consultation for a genuinely hard judgment call.** The two formal loops certify + a complete artifact against a verdict protocol — not the right tool for a single mid-cycle + question. When internal review (the risk-based budget in `per-issue-cycle.md` step 3, or + the coordinator's own judgment) genuinely cannot resolve a concrete tradeoff between two or + more approaches with no clear repo precedent — not a routine implementation choice, and not + a substitute for running that internal review first — the coordinator may ask ChatGPT one + self-contained question outside any pass-counted protocol: + + 1. By step 2.3, a ChatGPT conversation already exists for this unit (at minimum the + plan-review/plan-author session from 2.2) — always continue THAT conversation (or a + further-along code-review session), never open a new one, per "one unit, one ChatGPT + conversation" above. + 2. Drive the existing tab directly (the same technique as the pass-cap and + stalled-generation procedures above). Frame it explicitly as an ad hoc consultation, not + a formal review — no `VERDICT:` line, just the concrete question, the specific approaches + under consideration, and why internal review didn't settle it. + 3. **Verify the answer yourself before acting on it** — the same standing principle as every + formal finding, applied directly by the coordinator (read the real code/tests) since + there is no separate fact-check pass for an ad hoc exchange. + 4. Record the question, the answer, and what was decided in the unit's ship-log entry under + "Decisions taken" (`per-issue-cycle.md` already asks for decisions made under ambiguity + there) — cite the conversation URL. + + Never touches a pass counter (it isn't a `chatgpt-review plan`/`pr` CLI invocation) and is + coordinator-only, same as every other `chatgpt-review` touchpoint. One self-contained + question at a time, not an open-ended back-and-forth. + ## Default plan loop — `plan-review-loop.workflow.mjs` ``` diff --git a/skills/ship/tests/workflow-contract.test.mjs b/skills/ship/tests/workflow-contract.test.mjs index 532be894..2c57db39 100644 --- a/skills/ship/tests/workflow-contract.test.mjs +++ b/skills/ship/tests/workflow-contract.test.mjs @@ -32,20 +32,29 @@ test('the canonical plan path remains the authoring-session identity', async () assert.match(workflow, /--output-file \$\{shellQuote\(runArgs\.planFile\)\}/); }); -test('the decompose-and-implement loop grounds decomposition in the real branch, runs sub-tasks sequentially, and propagates failure', async () => { +test('the decompose-and-implement loop grounds decomposition in the real branch, runs waves in dependency order, parallelizing only within an explicitly-independent wave, and propagates failure', async () => { const source = await fs.readFile(path.join(root, 'references/decompose-and-implement-loop.workflow.mjs'), 'utf8'); assert.doesNotThrow(() => new Function(`return async function workflowSyntaxCheck() {\n${source.replace('export const meta', 'const meta')}\n}`)); // Decomposition must read what's ALREADY committed, not just the plan in the abstract. assert.match(source, /git log --oneline origin\/main\.\.HEAD/); assert.match(source, /model: 'fable', effort: 'high'/); - // Sequential, not parallel — a for-loop with one agent() awaited per iteration, no - // parallel()/pipeline() call fanning sub-tasks out concurrently. - assert.match(source, /for \(const \[index, task\] of decomposition\.subtasks\.entries\(\)\)/); - assert.doesNotMatch(source, /parallel\(/); + // Waves run strictly in dependency order; a solo wave (no independent sibling) runs its + // one sub-task directly, never via parallel() — only a wave the decomposition agent + // explicitly declared independent (size > 1) fans out concurrently, scoped to that one + // wave's own sub-tasks. + assert.match(source, /for \(const \[waveIndex, wave\] of waves\.entries\(\)\)/); + assert.match(source, /if \(wave\.length === 1\)/); + assert.match(source, /parallel\(wave\.map\(/); assert.doesNotMatch(source, /pipeline\(/); - assert.match(source, /model: 'sonnet'/); - // A dead sub-task agent must stop the loop, not silently skip the rest. - assert.match(source, /if \(!result\) return \{ status: 'error'/); + // Each sub-task is implemented by a fresh Sonnet coding agent, on both the solo path + // and the concurrent-wave path. + assert.match(source, /\{ label: task\.id, phase: 'Implement', model: 'sonnet' \}/); + assert.match(source, /\{ label: task\.id, phase: 'Implement', model: 'sonnet', isolation: 'worktree'/); + // A dead sub-task agent must stop the loop at every call site, not silently skip the + // rest: the solo path, the concurrent-wave path, and post-wave integration. + assert.match(source, /if \(!summary\) return \{ status: 'error'/); + assert.match(source, /const diedIndex = waveOutputs\.findIndex\(o => !o\)/); + assert.match(source, /if \(!integration\) return \{ status: 'error'/); }); test('required args are validated and the issue tag is derived for commit messages', async () => {