diff --git a/.wiki/Decisions-and-Roadmap.md b/.wiki/Decisions-and-Roadmap.md index a0a3a6c3..53e872fe 100644 --- a/.wiki/Decisions-and-Roadmap.md +++ b/.wiki/Decisions-and-Roadmap.md @@ -95,7 +95,8 @@ Two roadmap tracks are current: - **Refactor/umbrella track — #593** ("Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter") sequences the ADR-0004 follow-through into one ordered `/ship` execution - plan, one PR per phase with a human merge gate between phases: Phase 1 + plan, phase by phase (shipped under the pre-2026-08-05 per-phase-PR flow; + `/ship` now integrates units onto one branch/PR per run): Phase 1 #586 (`SurfaceLifecycle` + docked right-inspector slot, unblocks #488), Phase 2 #587 (side-panel registry, unblocks #487, salvages `feat/nav-section-registry-487p2`/PR #573), Phase 3 #591 (fail-closed diff --git a/.wiki/Development-Workflow.md b/.wiki/Development-Workflow.md index bd48cd6f..bcd34170 100644 --- a/.wiki/Development-Workflow.md +++ b/.wiki/Development-Workflow.md @@ -52,9 +52,10 @@ verification. - Preserve user-owned dirty-tree changes. - Track planned work in GitHub issues, not internal files under published `docs/`. - File high-signal out-of-scope bugs with the `inbox` label. -- In attended `/ship`, ask for explicit merge approval, then the same session may - merge the PR. In explicit unattended mode, do not ask: auto-merge only after a - clean third ChatGPT pass at the current head and green required checks. +- `/ship` never asks to merge on success: it auto-merges once a certified ChatGPT + review exists at the exact PR head with required checks green. It stops for a + human decision only when a review loop exhausts its passes (5 for plans, 3 for + code) or another merge proof condition fails. - Save genuinely surprising environment/test friction as project memory. Canonical source: [`CLAUDE.md`](../CLAUDE.md), diff --git a/.wiki/Project-Skills.md b/.wiki/Project-Skills.md index a937ad55..06835f46 100644 --- a/.wiki/Project-Skills.md +++ b/.wiki/Project-Skills.md @@ -5,12 +5,15 @@ Back to [[Home]]. Related: [[Development-Workflow]], [[Operations-Memory]]. The canonical project skills are tracked under the repo-root **`skills/`** directory (promoted from `.claude/skills` 2026-07-30, #b4cd83b): -- `ship` — plan, implement, test, review, reconcile, and open a PR for one issue. - Attended runs ask before merging and may perform the approved merge; unattended - runs have no merge prompt and auto-merge only after a clean third ChatGPT pass - at the current head plus green CI. Multi-phase attended work uses one PR per - phase; the old separate `ship-phase` skill now just forwards to `/ship`. -- `ship-phase` — deprecated alias that forwards to `/ship unattended`. +- `ship` — autonomously deliver one or more issues/phases: a coordinator spawns a + fresh worker per unit, iterates each unit's plan through a ChatGPT review loop + to approval (max 5 passes), integrates the implementation onto one branch, + opens one PR, and iterates a ChatGPT code review loop to certification (max 3 + passes). It auto-merges without asking when every proof condition holds + (certified head at the exact PR SHA, green required checks, branch protection + permits) and stops for a human decision only when a review loop exhausts its + passes or a merge proof fails. The old attended/unattended split and the + separate `ship-phase` alias were removed 2026-08-05. - `sql-browser-dashboard` — turns an already-known SQL/result-column investigation into a validated `PortableBundleV2` Dashboard bundle and publishes it through the `save_dashboard` MCP tool (or leaves it as a @@ -33,13 +36,14 @@ copies: ``` Keep `skills/` canonical. Edit each skill once there; do not replace the -symlinks with copies. `ship`/`ship-phase` can mutate git/GitHub and are only to -be invoked when explicitly requested. +symlinks with copies. `ship` can mutate git/GitHub and is only to be invoked +when explicitly requested. The skill instructions also require isolation for concurrent shipping, full unit and build gates, explicit read-only boundaries for review helpers, and reconciliation -of roadmap/ADR/changelog. Automatic merge is limited to explicit unattended runs -that satisfy the three-pass review and current-head CI proof. +of roadmap/ADR/changelog. `/ship` merges automatically only when a certified +ChatGPT review exists at the exact PR head with required checks green; any +failed proof condition halts for a human decision instead. ## Local-only development skills @@ -51,7 +55,7 @@ Some skills are **vendored dev tools kept local, not committed** to the repo: docs are tracked. Install it locally to run `/impeccable`; nothing in CI or the shipped artifact depends on it. -Rule: `ship`, `ship-phase`, `chatgpt-review`, and `sql-browser-dashboard` +Rule: `ship`, `chatgpt-review`, and `sql-browser-dashboard` (project workflow skills) are committed under `skills/`. Large general-purpose skills like `impeccable` stay local and are documented here. The old global instruction-only `chatgpt-review` is retained as a recoverable timestamped diff --git a/skills/ship-phase/SKILL.md b/skills/ship-phase/SKILL.md deleted file mode 100644 index 3149f979..00000000 --- a/skills/ship-phase/SKILL.md +++ /dev/null @@ -1,25 +0,0 @@ ---- -name: ship-phase -description: Deprecated alias — the multi-issue/multi-phase coordinator now lives in /ship. Invoke `/ship ISSUE unattended` or `/ship ISSUE1,ISSUE2 unattended` instead. Invoking this simply forwards to /ship. ---- - -# /ship-phase — moved into /ship - -This skill has been folded into `/ship` so the per-issue cycle has exactly one definition. -It used to duplicate `/ship`'s steps 1–5 by reference, and the two drifted. - -Translate the invocation and continue there: - -| Old | New | -|---|---| -| `/ship-phase 7` (a phase of one issue) | `/ship unattended` | -| `/ship-phase 424,425,426` (several issues) | `/ship 424,425,426 unattended` | - -Invoke the `ship` skill with the translated argument now, and follow it — in particular -`references/unattended.md`, which holds the coordinator and wave rules that used to live here. - -Note the behaviour change worth knowing: `/ship ` **without** `unattended` no longer -ships the whole issue in one go. It ships the next unshipped **phase**, opens one PR, and asks -for merge approval; after approval, the same session may merge it — one session per phase. -Unattended mode has no merge prompt and auto-merges only after a clean third ChatGPT pass at -the current head plus green CI. diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index dd65a644..a5b838d8 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -1,18 +1,33 @@ --- name: ship -description: Ship an altinity-sql-browser roadmap issue or phase end-to-end — resolve scope, plan against an invariant map, implement code and tests, run a budgeted readiness review, open a PR, obtain one clean exact-head ChatGPT review, and merge after approval (attended) or after strict certification (unattended). Invoke as `/ship ISSUE`, `/ship ISSUE.PHASE`, `/ship ISSUE unattended`, or `/ship ISSUE1,ISSUE2 unattended`. +description: Ship altinity-sql-browser roadmap issues or phases end-to-end, autonomously — resolve scope, spawn a fresh worker per unit, iterate each unit's plan through a ChatGPT review loop to approval (max 5 passes), implement code and tests, open one PR, iterate a ChatGPT code review loop to certification (max 3 passes), and merge automatically when every proof condition holds. Stops for a human only when a review loop exhausts its passes or a merge proof fails. Invoke as `/ship ISSUE`, `/ship ISSUE.PHASE`, or `/ship ISSUE1,ISSUE2`. --- -# /ship — deliver an altinity-sql-browser issue +# /ship — deliver altinity-sql-browser issues autonomously Use only in the `altinity-sql-browser` repository. Otherwise stop and say so. -Follow `CLAUDE.md` (hard rules 1–5 and Working discipline) throughout. In attended mode, -act as coordinator and implementer. In unattended mode, act only as coordinator and use -fresh workers for implementation units. +Follow `CLAUDE.md` (hard rules 1–5 and Working discipline) throughout. -Proceed autonomously except at points marked `STOP`. Unattended mode has no approval -stops, but it must skip ambiguous or blocked work rather than invent decisions. +You are the **coordinator**. You do not implement units yourself. You plan waves, spawn +workers and reviewers, verify their output with your own commands, run every ChatGPT +review loop, integrate commits, and own everything git-remote-facing. A **unit** is a +phase or a whole issue; units run in dependency order. + +**Why workers are mandatory:** there is no human to `/clear` between units, so the +context bound comes from structure — every unit's implementation runs inside a fresh +subagent with its own context window, and only a summary returns. Your context grows by +~1–2k per unit, not by a full transcript. Never inline a unit's implementation into your +own turn to save a spawn. + +Proceed autonomously. There are exactly two human stops, both failure stops: + +1. a unit's plan is not approved after **5** review passes (step 2.2); +2. a merge proof condition fails at the gate — including no certified head after **3** + code review passes (step 3.6). + +Everywhere else, an ambiguous or blocked unit is **skipped and reported**, never guessed +at — this is a settled-architecture project; don't invent decisions. ## Operating rules @@ -23,10 +38,9 @@ stops, but it must skip ambiguous or blocked work rather than invent decisions. - Bare `gh issue view` and `gh pr edit` error on this repo: read with `--json`, and edit PR/issue bodies with `gh api -X PATCH` and `-F body=@`. Never build a body with `$(cat …)` inside a quoted heredoc — write it to a file. -- Never force-push or mutate `main` unless explicitly authorized. +- Never force-push or mutate `main` directly. - One `/ship` run owns one working directory. Parallel runs require separate worktrees - (`claude --worktree `); parallelize only dependency-independent units with - disjoint expected file footprints. + (`claude --worktree `). - After any agent batch returns, verify actual state with `git diff`, `git log`, and `gh pr list` — never trust a self-report. @@ -38,13 +52,26 @@ State the boundary in every prompt: - no Edit or Write - no git or gh mutation - no task or memory mutation +- no `chatgpt-review` invocation - return analysis only -Use fresh, non-forked agents for independent review (a fork inherits this in-progress -mutating workflow). Pick the model per subtask: inherited model for planning/review and -high-risk work, a cheaper capable model for mechanical searches and edits. The -coordinator alone owns remote GitHub mutations, reconciliation, PR creation, -review-session coordination, approval handling, and merge. +Use fresh, non-forked agents (a fork inherits this in-progress mutating workflow). Pick +the model per subtask: inherited model for high-risk work, `sonnet` for ordinary units +and reviews. The coordinator alone owns remote GitHub mutations, reconciliation, PR +creation, every `chatgpt-review` invocation, and the merge. + +### ChatGPT review loops + +- Agent Chrome is a **single session**: the coordinator runs every `chatgpt-review` + invocation itself and serializes them — workers and reviewers never invoke the skill, + and parallel units take their review passes one at a time. +- Both loops use a **verdict protocol**: the question file instructs the reviewer to end + with exactly one line — `VERDICT: APPROVED` or `VERDICT: REVISE` for a plan, + `VERDICT: SHIP` or `VERDICT: REVISE` for a PR. A missing or malformed verdict counts + as REVISE (fail-closed), and the pass still counts against the cap. +- **Verify every substantive finding against the real repository before trusting it** — + a second opinion, not a source of truth. Every finding ends in exactly one state: + accepted-and-fixed, rejected-with-reason, or unresolved. Never silently drop one. ### Output capture @@ -55,34 +82,33 @@ accidental context sink in a run. ## 0 — Resolve invocation -| Invocation | Mode | Scope | -|---|---|---| -| `/ship 447` | attended | next unshipped phase, or whole issue if unphased | -| `/ship 447.2` | attended | phase 2 of #447, forced | -| `/ship 447 unattended` | unattended | all remaining phases of #447 | -| `/ship 424,425 unattended` | unattended | several whole issues | - -Attended is the default. **Unattended requires the literal word** — never infer it. +| Invocation | Scope | +|---|---| +| `/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 | -For unattended, Read `references/unattended.md` now and follow it; it replaces steps 1, -6, 7 and 8 below and reuses steps 2–5 as the worker contract. +The legacy word `unattended` is accepted and ignored — this is the only mode. -## 1 — Orient and assemble the delivery contract +## 1 — Orient and assemble the delivery contracts -**Read `references/repo-footguns.md` now** — repo-specific operational knowledge that -applies across steps 2–8. +**Read `references/repo-footguns.md` and `references/per-issue-cycle.md` now.** The +footguns apply across every step; the cycle is the worker contract you will hold every +unit to — one source of truth, quoted, never paraphrased. -Load the issue body to a file, not into context: +Load each issue body to a file, not into context: ```sh gh issue view --json body -q .body > "$TMPDIR/issue-.md" ``` +The bodies are the spec and are deliberately self-contained; never rely on chat history. + Detect phases in this order of precedence: 1. a `## Phases` checklist of `- [ ] N — title` rows; 2. `### Phase N` headings (typically under `## Delivery phases`); -3. neither → single-phase; ship the issue whole. +3. neither → single-phase; the issue is one unit. Find the authoritative `` comment — never infer phase state from PR titles (they go stale when phase counts are re-scoped mid-flight): @@ -92,149 +118,197 @@ gh api repos/{owner}/{repo}/issues//comments --paginate \ --jq '.[] | select(.body | startswith("")) | .id' > "$TMPDIR/logid" ``` -The target is the first phase not marked `shipped`, unless the invocation forces one. -If no log exists, the target is phase 1. +The units are every phase not marked `shipped` (or the one phase the invocation forces). +If no log exists, start at phase 1. -**Read only what this unit needs** from the issue file: the header (depends-on / +**Read only what each unit needs** from the issue file: the header (depends-on / supersedes / owner decisions), the phase list, the target phase's own section, its matching `### Phase N` subsection under `## Tests` if the issue splits tests that way, the global `## Acceptance criteria`, and `## Non-goals`. Skip other phases' detail. **The delivery contract** is the union of: the phase's implement list, its per-phase `## Tests` subsection, any acceptance-gate blockquote in the phase section, the subset -of global acceptance criteria this phase claims (name that subset explicitly, so the +of global acceptance criteria the phase claims (name that subset explicitly, so the remainder is visibly deferred rather than silently dropped), and `## Non-goals`. Missing the per-phase `## Tests` subsection because it lives outside the phase heading is the most likely way to under-deliver a phase — check every time. -`STOP` — check dependencies for *this unit*, not just the issue: the `Depends on:` -header and anything the target phase's own section references. Blocked → say what -blocks it and offer the highest unblocked unit instead. - -### Branch model (once per issue, recorded in the ship log) - -> Can each phase land on `main` green **and** self-consistent — additive, behind a -> flag, or pure-logic-then-wire? - -- **Yes → one branch and PR per phase off fresh `main`** (the default): - `git fetch origin && git checkout main && git pull && git checkout -b /-p` -- **No → one integration branch for the whole issue** off `origin/main`, one PR at the - end. Say so explicitly and record it in the ship log. - -Single-phase issue: branch `/-` off `main` — or off the dependency -branch if it builds on unmerged work. When a phase is subtractive, call out the revert -cost (backing a deletion out of `main` alone is expensive) — a reason to raise it, not -to silently switch models. - -Run `npm ci` after updating `main` when dependencies may have changed or `node_modules` -is absent. - -## 2–5 — The per-issue cycle - -**Read `references/per-issue-cycle.md` now**, before writing any plan or code, and -follow it. It holds plan → implement → readiness review → reconcile, including the risk -classification, the invariant map, the review budgets, and the exact local gate. It is -the same contract unattended workers are held to — one source of truth, so the two -paths cannot drift. - -## 6 — Create the PR - -Performed by **this session only** — never delegate commit/push/PR-create. - -- Commit with the repo footer convention (Co-Authored-By + Claude-Session), then - `git push -u origin `. -- `gh pr create --base main`, title + body per `.github/PULL_REQUEST_TEMPLATE.md`. -- Title: `(#): `; append ` (phase )` for phased work. Never a - running count like `(2/3)` — the ship log owns the count and totals change. -- `Closes #` only when this PR completes the whole issue; earlier phases use - `Part of #`. -- Body covers: contract coverage, invariant verification, sabotage cases, tests, build, - and e2e results. Tick the template checklist. Report the PR URL. - -## 7 — ChatGPT exact-head review - -Performed by this session only. Read `skills/chatgpt-review/SKILL.md`, write a focused -question file to the approved temporary directory containing: the unit contract and -acceptance subset, the invariant map, compatibility requirements, tests and sabotage -cases, and the behaviors that need adversarial review. Then: - -```sh -node skills/chatgpt-review/scripts/chatgpt-review.mjs pr --question-file -``` - -Retain the returned JSON: status, opaque `session` handle, reviewed SHA, and public -comment URL. **Verify every substantive finding against the real repository before -trusting it** — a second opinion, not a source of truth. Every finding ends in exactly -one state: accepted-and-fixed, rejected-with-reason, or unresolved. Never silently drop -one. - -Fix pass: apply accepted findings, run the full local gate, commit, push, wait for -green CI at the new head, then rerun with `--session ` so ChatGPT -reassesses every earlier finding in the same conversation. Each accepted-and-fixed pass -gets its own pushed commit and separately labelled public review comment. - -### Certification rule - -A **certified head** is a completed ChatGPT review pass at the exact current PR head -with no accepted actionable findings remaining. - -- Pass 1 clean at the current head → certified; stop reviewing. -- Never exceed three total passes. Three is a **failure ceiling, not a ritual** — never - run an extra pass over an already-certified head. -- **After certification, push nothing.** Any push voids the certification and burns - another pass — which is why all reconcile commits (step 5) land *before* the PR and - certification. Ship-log comment edits are fine; comments are not commits. -- **Unattended:** certification is a hard merge requirement. No certified head after - three passes → leave the PR open and report the failed proof condition. -- **Attended:** certification is the goal, but the human at the merge gate is the - authority. If ChatGPT is unreachable (agent Chrome down, network denied), disclose - the skip in the gate summary and continue. If the ceiling is reached with findings - still contested or unresolved, continue to the gate with them listed — explicit human - approval overrides. - -## 8 — `STOP` Attended merge gate - -Ask for explicit approval to merge. Do not merge merely because implementation, CI, and -review are green. Summarize: - -- shipped behavior, PR URL, current head SHA -- CI state -- whether the head is certified; ChatGPT comment URLs -- accepted, rejected, and **unresolved** findings — the human rules on unresolved ones -- manual testing instructions - -Serve the app with `npm run local` for manual verification (see footguns for port/PID -discipline). If approval is withheld, leave the PR open and report the exact -outstanding condition. - -After approval: - -1. Re-read the PR head SHA — approval must not race a new push. -2. Confirm the certified SHA still matches, or restate the disclosed skip/override. -3. Confirm required checks are green at that head. -4. `gh pr merge --merge --delete-branch` (the repo's merge-commit convention). -5. Verify the PR reports `MERGED` and `origin/main` contains the merge. -6. Update the ship-log row to `shipped`. - -If phases remain, end with this instruction in substance: - -> Phase `` was merged as PR ``. Start a **fresh session** — `/clear`, then -> `/ship ` — for phase ``. Don't resume this one with `--continue`/ -> `--resume`; that restores this context instead of clearing it. - -Context does not reset on its own; running `/ship` again in this session stacks a -second phase's plan, tests, and reviews on top of this one, which auto-compaction then -eats unpredictably. The fresh session is safe only because step 5 wrote the handoff to -the issue first — this conversation is not the record. - -## 9 — Final report - -Report: unit status, PR and merge URLs, final SHA, CI state, tests/build/e2e results, -sabotage verification, ChatGPT comment URLs with accepted/rejected/unresolved findings, -skipped units, and ship-log updates. - -## 10 — Friction → memory +**Dependencies:** check the `Depends on:` header and anything each unit's own section +references. A blocked unit → skip it (leave its commits out), continue the rest, and +list it in the final report — the same policy as for units needing an unrecorded +decision. + +### Wave plan and integration branch + +- Sequence the dependency spine. Parallelize only units whose planned file footprints + are disjoint — when in doubt, serialize; a merge conflict costs more than lost + parallelism. Phases of one issue are almost always a spine, not a wave. +- **One integration branch for the whole run, one PR at the end**, off the remote + default: `git fetch origin && git checkout -b / origin/main`. Push + immediately so CI runs from the start. (In a worktree, local `main` is stale — see + footguns.) +- Run `npm ci` when dependencies may have changed or `node_modules` is absent. + +## 2 — Per unit (repeat per wave) + +### 2.1 Spawn the worker — plan first, no code + +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). +`model: "sonnet"` unless the wave plan marks the unit high-risk, in which case omit +`model` to inherit yours. Parallel workers get `isolation: "worktree"`; a solo worker +may use the main tree on `wip/-` off the current integration HEAD. + +The worker prompt must contain, explicitly: + +- the issue number, the phase (if any), and the instruction to load the body with + `gh issue view --json body -q .body` and treat the **assembled delivery + contract** as the definition of done — implement list + `## Tests` subsection + + acceptance gate + named subset of global criteria; +- **the mutation boundary**: Edit/Write + local `git commit` on its own branch only — + no push, no PR, no `gh` mutations, no issue edits, no ship-log writes, no memory + writes, no `CHANGELOG.md` beyond its own entry, no TaskCreate/TaskUpdate, and never + invoke `chatgpt-review` (the coordinator owns all review sessions); +- the instruction to follow `skills/ship/references/per-issue-cycle.md` steps 1–3 and + the CHANGELOG part of step 4 — **the ship log is yours, not the worker's** — plus + `references/repo-footguns.md`; +- **its first deliverable is the plan only**: write the plan (cycle step 1) to the + exact path `$TMPDIR/plan-p.md` (or `$TMPDIR/plan-.md` unphased), + return the plan summary and that path **without writing any code**, then wait for + plan approval via `SendMessage`; +- commit message `(#): ` + the repo footer convention; +- what to return after implementing: invariant map, files touched, gate/e2e output + tail, sabotage-case results, and the contract checklist with each item ticked or + explained. + +A worker that reports the unit ambiguous or dependent on an unrecorded decision → +skip the unit, report the missing decision in the final report, move on. + +### 2.2 Plan review loop — coordinator-run, every unit, max 5 passes + +The plan file **path** is the review-session identity — the worker revises the same +file in place; never move or rename it mid-loop (footguns). + +1. Write a context file to `$TMPDIR`: the unit contract and acceptance subset, focused + questions, and the verdict protocol — "End your review with exactly one line: + `VERDICT: APPROVED` or `VERDICT: REVISE`." +2. Run (plan mode never publishes): + + ```sh + node skills/chatgpt-review/scripts/chatgpt-review.mjs plan --question-file + ``` + + Later passes: the same command plus `--session ` so the reviewer reassesses + in the same conversation. +3. `VERDICT: REVISE` → verify each finding against the repo; send the accepted ones to + the worker (`SendMessage` — **check out its branch first**, footguns); the worker + revises the plan file in place; rerun. +4. `VERDICT: APPROVED` → record the pass count and conversation URL for the ship log; + proceed to 2.3. + +**Cap: 5 total passes per unit** (skill-enforced; the script caps only `pr` mode). +Not approved after 5 → **FULL STOP — human decision needed.** Present the latest plan, +the contested points, and the conversation URL, and ask the human: approve the latest +plan, redirect, or skip the unit. Write no code for this unit before that decision. + +### 2.3 Implement + +`SendMessage` the worker: plan approved — implement now (cycle steps 2–3 plus its +CHANGELOG entry). + +### 2.4 Verify, review, integrate, log + +- **Verify yourself** — never trust the self-report. `git log` / `git diff` the worker + branch, rerun the full local gate in that tree. +- **Internal review budget** — the risk-based budget of cycle step 3: no reviewer for + low-risk units; one targeted read-only reviewer (`model: "sonnet"`, boundary stated) + for medium/high, prompted with the unit's contract + CLAUDE.md hard rules. Real + findings → back to the worker (`SendMessage`, branch checked out first) or a bounded + fix agent; re-verify. Do not add generic per-unit review passes on top. +- **Integrate**: merge the worker branch into the integration branch (you resolve + conflicts — you are its only writer), rerun the gate, push. Key every CI wait on the + head SHA (footguns) and check it before the next dependent wave. Red CI stops the + line until fixed. +- **Log**: append the unit's handoff block to the `` comment now, + status `in review` until the single PR merges. Include the plan-review outcome + (passes, conversation URL). + +After every batch, regardless of what agents reported: `git diff`, `git log`, +`gh pr list`. An instruction in a prompt is not an enforced tool restriction, and +review agents on this repo have edited files despite an explicit report-only boundary. + +## 3 — Finish: PR, code review loop, gate + +1. **Whole-branch review** — only if the run contains high-risk work or interacting + units: one targeted read-only pass over the full branch diff at high effort (plus + the `security-review` skill if anything touched auth/config). Do not repeat the + per-unit reviews. Apply real findings via a fix agent under the worker boundary; + re-verify; push. +2. Confirm required CI checks are green at the head. The e2e signal is layered: + workers ran Chromium + WebKit locally per the cycle; PR CI adds its Chromium e2e + run (#564); Firefox comes only from the CI jobs that provide it. +3. **Reconcile per cycle step 4** — CHANGELOG entries are per-unit already, so dedupe + and resolve conflicts only; close superseded issues; tick any `## Phases` checklist + in one edit. All reconcile commits land **now, before the PR** — nothing may be + pushed after certification. +4. **One PR** (`gh pr create --base main`), title `(#): `, body + per `.github/PULL_REQUEST_TEMPLATE.md` covering contract coverage, invariant + verification, sabotage cases, tests, build, and e2e results; a per-unit summary + table; `Closes #` per fully completed issue (`Part of #` for partial or + skipped); the repo PR footer. +5. **Code review loop — max 3 passes.** Write a question file to `$TMPDIR`: the unit + contracts and acceptance subsets, the invariant maps, compatibility requirements, + tests and sabotage cases, the behaviors that need adversarial review, and the + verdict protocol — "End your review with exactly one line: `VERDICT: SHIP` or + `VERDICT: REVISE`." Then: + + ```sh + node skills/chatgpt-review/scripts/chatgpt-review.mjs pr --question-file + ``` + + Retain the returned JSON: status, opaque `session` handle, reviewed SHA, and public + comment URL. + + Fix pass: apply accepted findings, run the full local gate, commit, push, wait for + green CI at the new head, then rerun with `--session ` so ChatGPT reassesses + every earlier finding in the same conversation. Each fix pass gets its own pushed + commit and separately labelled public review comment. + + A **certified head** is a completed pass whose reviewed SHA equals the current PR + head, verdict `SHIP`, and no accepted actionable findings remaining. + + - First clean pass at the current head → certified; stop reviewing. Never re-review + an already-certified head — three is a failure ceiling, not a ritual (and the + script enforces the cap for `pr` mode). + - **After certification, push nothing.** Any push voids the certification and burns + another pass — which is why all reconcile commits landed in step 3.3. Ship-log + comment edits are fine; comments are not commits. +6. **The gate.** Merge automatically — no prompt — only when ALL hold at one exact head: + + - certified head (step 3.5); + - reviewed SHA equals the current PR 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 every included 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 STOP — human decision needed.** Do not merge. Summarize the PR URL, head + SHA, CI state, certification state, and every accepted, rejected, and **unresolved** + finding with its comment URL, then ask the human to rule: merge anyway, leave the + PR open, or direct further work. Their decision governs. + +## 4 — Final report + +Report: PR and merge URLs, per-unit shipped/skipped status and why, plan-loop pass +counts, every ChatGPT conversation/comment link, findings +accepted/rejected/unresolved, final head SHA, CI state, and ship-log updates. + +## 5 — Friction → memory If anything needed retries or surprised you (test / env / scope), save a concise memory so the next `/ship` doesn't repeat it. Record stable process knowledge, not transient -review chronology. This session does the saving, never a subagent. +review chronology. The coordinator writes it, never a subagent. diff --git a/skills/ship/references/per-issue-cycle.md b/skills/ship/references/per-issue-cycle.md index dc7a8ff2..8143ed2e 100644 --- a/skills/ship/references/per-issue-cycle.md +++ b/skills/ship/references/per-issue-cycle.md @@ -1,14 +1,16 @@ -# The per-issue cycle — steps 2–5 +# The per-unit cycle -Steps 2–5 of `/ship`. This is the **single source of truth** for how one unit of work -(a whole issue, or one phase of one) gets built. The attended flow follows it directly; -unattended workers are held to it verbatim. Change it here, never in a copy. +The **single source of truth** for how one unit of work (a whole issue, or one phase of +one) gets built. Workers are held to it verbatim; the coordinator flow that wraps it — +worker spawning, both ChatGPT review loops, integration, and the merge gate — is +`SKILL.md`. Change it here, never in a copy. -Where this file says "the unit", read the delivery contract assembled in step 1: the -implement list, the per-phase `## Tests` subsection, the acceptance gate, and the named -subset of global acceptance criteria. `references/repo-footguns.md` applies throughout. +Where this file says "the unit", read the delivery contract assembled in `SKILL.md` +step 1: the implement list, the per-phase `## Tests` subsection, the acceptance gate, +and the named subset of global acceptance criteria. `references/repo-footguns.md` +applies throughout. -## 2 — Plan +## 1 — Plan **Always write the plan — nothing skips this, however small or well-specified.** Produce it before touching code. State: @@ -25,10 +27,10 @@ Produce it before touching code. State: - **risk classification** (below); - **invariant map** for medium- and high-risk work (below). -`STOP` — if the unit is ambiguous, under-specified, or needs a decision not already -recorded (issue body / ADRs / CLAUDE.md), stop and ask. This is a settled-architecture -project — don't invent decisions. (Unattended: skip the unit and report the missing -decision instead.) +If the unit is ambiguous, under-specified, or needs a decision not already recorded +(issue body / ADRs / CLAUDE.md), do not invent one — return the missing decision +instead of a plan; the coordinator skips the unit and reports it. This is a +settled-architecture project. ### Risk classification @@ -67,35 +69,19 @@ source of truth", "exhaustive", "never", or "exactly once" without showing how i enforced — a claim without enforcement is exactly what review rounds later discover as a defect, one relocation at a time. -### Plan review budget +### Plan review -Do not stack overlapping plan reviews. +Every plan — regardless of risk — goes through the coordinator-run ChatGPT plan review +loop (`SKILL.md` step 2.2: exit on `VERDICT: APPROVED`, max 5 passes). The worker's +part of the loop: -- **Low:** none. -- **Medium:** exactly one `chatgpt-review` plan-mode pass (procedure below), run by - default — cheap enough that a worker can invoke it directly even in unattended mode, - and it catches architecture gaps a solo planning pass misses. Skip only if ChatGPT is - unreachable (agent Chrome down, network denied); disclose the skip in the plan and - continue — same escape hatch High already has. -- **High:** exactly one independent plan review. Choose **one**: - 1. a read-only internal `Plan` agent (boundary stated), or - 2. `chatgpt-review` in plan mode (procedure below). +- write the plan to the exact file path the coordinator assigned, and return it; +- on REVISE findings the coordinator relays, revise that file **in place** — the path + is the review-session identity (footguns) — and return again; +- never invoke `chatgpt-review` yourself, and write no code before the coordinator + reports the plan approved. - Prefer ChatGPT plan review for especially uncertain, externally constrained, or - unattended work; prefer the internal Plan agent when repository context suffices. - -**`chatgpt-review` plan-mode procedure:** write the complete plan to a file under the -approved temporary directory, the contract/acceptance subset/focused questions to a -context file, then -`node skills/chatgpt-review/scripts/chatgpt-review.mjs plan --question-file `. -It uploads exactly the plan file and cannot request a GitHub write, so a worker may run -it directly even in unattended mode. Verify every substantive claim against the real -repo before folding it in. - -For attended high-risk work, `STOP`: post the revised plan and wait for approval -(reviewed on mobile — keep it self-contained). - -## 3 — Implement (inner loop) +## 2 — Implement (inner loop) Write the code **and its tests in the same change** (hard rule 1). @@ -143,14 +129,14 @@ A sabotage that everything survives means the invariant is unenforced — fix th enforcement, not the test. Restore sabotaged uncommitted files from saved bytes, never with `git checkout --` (it deletes uncommitted fixes). -## 4 — Author-side readiness review +## 3 — Author-side readiness review -Internal review prepares the PR for external review. It is not a ritual and must not -duplicate the ChatGPT review. +Internal review prepares the branch for external review. It is not a ritual and must +not duplicate the ChatGPT code review. ### Readiness checklist -Before opening a PR, verify: +Before handing the unit back, verify: 1. Every contract item maps to code or a test. 2. Every invariant has enforcement and a sabotage case. @@ -211,12 +197,13 @@ If two review rounds find variants of one root cause, revise the invariant map b writing another fix. Fixes that relocate a defect are how a one-pass review becomes four. -## 5 — Reconcile (before the PR — and before certification) +## 4 — Reconcile (before the PR — and before certification) -Every commit this step produces must land **now**: a post-certification commit voids -the exact-head review and burns another pass. +Every commit this step produces must land **before the PR is certified**: a +post-certification commit voids the exact-head review and burns another pass. -- Update `CHANGELOG.md` under `[Unreleased]` when required. +- Update `CHANGELOG.md` under `[Unreleased]` when required (the worker writes its own + unit's entry; the coordinator dedupes across units). - Update the relevant ADR addendum for architecture changes. - Reconcile the issue's Goal/Acceptance text when implementation deliberately changed it. - Close or reconcile superseded issues — when the unit that owns that result lands, not @@ -224,12 +211,12 @@ the exact-head review and burns another pass. - Out-of-scope bug or footgun spotted → a **separate** issue labelled `inbox` (file:line + why deferred); never fold it into this PR. -### The ship log — write it BEFORE the approval/merge stage +### The ship log — coordinator-owned, written before the PR -For a multi-phase issue this is the **handoff**, not bookkeeping. The next phase runs -in a cleared session that knows nothing about this one — decisions taken under -ambiguity, deviations, why a test is shaped the way it is. If that only lives in the -conversation, `/clear` destroys it and the next phase re-derives it wrong. +Workers never touch it. For a multi-phase issue this is the **handoff**, not +bookkeeping: later units run in fresh worker contexts that know nothing about this one +— decisions taken under ambiguity, deviations, why a test is shaped the way it is. If +that only lives in a conversation, it is lost and the next unit re-derives it wrong. Maintain exactly one comment on the issue, in this format: @@ -237,8 +224,6 @@ Maintain exactly one comment on the issue, in this format: ## Ship log — # -Branch model: one PR per phase off `main` - | Phase | Status | PR | |---|---|---| | 1 — Hard removal and Variables foundation | shipped | #452 | @@ -251,6 +236,7 @@ Branch model: one PR per phase off `main` ` - comment exactly as `SKILL.md` step 1 describes; take every phase not `shipped`. - Issue list: `gh issue view --json body` each one — the bodies are the spec and - are deliberately self-contained; never rely on chat history. -- Load each body to `$TMPDIR/issue-.md` and Read phase-scoped, as in step 1. -- **Derive the wave plan.** Sequence the dependency spine. Parallelize only units - whose planned file footprints are disjoint — when in doubt, serialize; a merge - conflict costs more than lost parallelism. Phases of one issue are almost always a - spine, not a wave. -- **Branch:** one integration branch for the whole run, off the remote default: - `git fetch origin && git checkout -b / origin/main`. Push immediately so - CI runs from the start. (In a worktree, local `main` is stale — see footguns.) - - Unattended uses **one branch and one PR** even when units would be independently - mergeable, because no human sits at an intermediate gate. - -## 2 — Per unit (repeat per wave) - -**Spawn a worker** — a 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). `model: "sonnet"` unless the wave plan marks the unit -high-risk, in which case omit `model` to inherit yours. Parallel workers get -`isolation: "worktree"`; a solo worker may use the main tree on `wip/-` -off the current integration HEAD. - -The worker prompt must contain, explicitly: - -- the issue number, the phase (if any), and the instruction to `gh issue view` it and - treat the **assembled delivery contract** as the definition of done — implement list - + `## Tests` subsection + acceptance gate + named subset of global criteria; -- **the mutation boundary**: Edit/Write + local `git commit` on its own branch only — - no push, no PR, no `gh` mutations, no issue edits, no ship-log writes, no memory - writes, no `CHANGELOG.md` beyond its own entry, no TaskCreate/TaskUpdate; -- the instruction to follow `skills/ship/references/per-issue-cycle.md` steps 2–4 and - the CHANGELOG part of 5 — **the ship log is yours, not the worker's** — plus - `references/repo-footguns.md`; -- commit message `(#): ` + the repo footer convention; -- what to return: plan summary, invariant map, files touched, gate/e2e output tail, - sabotage-case results, and the contract checklist with each item ticked or explained. - -**Verify yourself** — never trust the self-report. `git log` / `git diff` the worker -branch, rerun the full local gate in that tree. - -**Review budget per unit** — same risk-based budget as `per-issue-cycle.md` step 4: no -reviewer for low-risk units, one targeted read-only reviewer (`model: "sonnet"`, -boundary stated) for medium/high, prompted with the unit's contract + CLAUDE.md hard -rules. Real findings → back to the worker (`SendMessage`, but **check out its branch -first** — see footguns) or to a bounded fix agent; re-verify. Do not add generic -per-unit review passes on top. - -**Integrate**: merge the worker branch into the integration branch (you resolve -conflicts — you are its only writer), rerun the gate, push. Each push is a CI signal; -key the wait on the head SHA (footguns) and check it before the next dependent wave. -Red e2e stops the line until fixed. - -**Log**: append the unit's handoff block to the `` comment now, -status `in review` until the single PR merges. - -After every batch, regardless of what agents reported: `git diff`, `git log`, -`gh pr list`. An instruction in a prompt is not an enforced tool restriction, and -review agents on this repo have edited files despite an explicit report-only boundary. - -## 3 — Finish - -1. **Whole-branch review** — only if the run contains high-risk work or interacting - units: one targeted read-only pass over the full branch diff at high effort (plus - the `security-review` skill if anything touched auth/config). Do not repeat the - per-unit reviews. Apply real findings via a fix agent under the worker boundary; - re-verify; push. -2. Confirm CI fully green — unit + e2e, all engines. -3. **Reconcile per `per-issue-cycle.md` step 5** — CHANGELOG entries are per-unit - already, so dedupe and resolve conflicts only; close superseded issues; tick any - `## Phases` checklist in one edit. All reconcile commits land **now, before the - PR** — nothing may be pushed after certification. -4. **One PR** (`gh pr create --base main`), body per the template, one `Closes #` - per fully completed issue (`Part of #` for partial or skipped), a per-unit - summary table, and the repo PR footer. -5. **Certification** — run the ChatGPT review exactly as `SKILL.md` step 7 specifies, - with the same rule: **stop at the first clean completed pass whose reviewed SHA - equals the current PR head**; rerun (same `--session`) only after accepted findings - are fixed, gated, pushed, and green in CI; never exceed three total passes. Three - is a failure ceiling, not a ritual — never re-review an already-certified head. -6. **Automatic merge — no gate.** Merge only when ALL hold at one exact head: - - ChatGPT status is `completed` with no accepted actionable finding remaining; - - reviewed SHA equals the current PR head; - - required CI checks are green at that head; - - branch protection permits the merge. - - Then `gh pr merge --merge --delete-branch`, verify the PR reports `MERGED`, - fetch `origin/main` and verify the merge, and flip every included ship-log row to - `shipped`. - - If ChatGPT is unavailable, a pass is incomplete, the SHA differs, a finding remains - after three passes, CI is red or pending, or branch protection refuses — **leave - the PR open and report the failed proof condition.** That is a safety failure, not - an approval gate; there is no attended-style human override in this mode. -7. **Final report**: PR and merge URLs, per-unit shipped/skipped status and why, every - ChatGPT comment link, findings accepted/rejected/unresolved, final head SHA, CI state. -8. Friction → memory. The coordinator writes it, never a subagent.