From e688c3f5d4b3399648e700ad7bc55581bded0b5b Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sun, 9 Aug 2026 10:07:23 +0200 Subject: [PATCH] chore(ship): fix stale drift-bind test, add ad hoc ChatGPT consultation procedure skills/ship/tests/workflow-contract.test.mjs asserted against the OLD flat for-loop shape of decompose-and-implement-loop.workflow.mjs (pre-PR #651) -- PR #651's wave-based rewrite (sequential waves, parallel() only within an explicitly-independent multi-sub-task wave) broke 3 of its regex assertions and left a 4th (bare `model: 'sonnet'`) only coincidentally passing. Rewrote the assertions to pin the current structure and intent: wave-ordered iteration, the solo-vs-parallel branch, a fresh Sonnet agent on both paths, and a dead-agent halt at all three call sites (solo, parallel wave, integration). Confirmed pre-existing on origin/main via git stash before this fix. Also documented a third ChatGPT touchpoint for /ship, alongside the two formal pass-counted loops: an ad hoc, coordinator-only consultation for a genuinely hard judgment call internal review can't resolve, using the same manual Chrome-driving technique already sanctioned for stalled-generation recovery and post-pass-cap continuation (review-loops.md) -- no new script/plumbing. Cross-referenced from per-issue-cycle.md's root-cause circuit breaker and internal review budget sections, and from SKILL.md's ChatGPT review loops operating rules. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- skills/ship/SKILL.md | 4 +++ skills/ship/references/per-issue-cycle.md | 9 +++++-- skills/ship/references/review-loops.md | 27 ++++++++++++++++++++ skills/ship/tests/workflow-contract.test.mjs | 25 ++++++++++++------ 4 files changed, 55 insertions(+), 10 deletions(-) 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 () => {