diff --git a/docs/agent-template.md b/docs/agent-template.md index 262e954..25fb355 100644 --- a/docs/agent-template.md +++ b/docs/agent-template.md @@ -159,8 +159,8 @@ pin exists to remove. Effort follows the model split: Workflow scripts pin a third tier inline: schema-bound extraction/verification stages that author no code (preflight, -plan-index, plan-extract, mini-verify, tree-check, finalize, build/suite verify) -run `medium` via the `agent()` call's `effort:` option. As with +plan-index, plan-extract, snapshot, mini-verify, tree-check, finalize, +build/suite verify) run `medium` via the `agent()` call's `effort:` option. As with `model:`, every `agent()` call passes `effort:` explicitly — whether frontmatter effort propagates through an `agentType` dispatch is undocumented, so the scripts do not rely on it (see the policy header diff --git a/docs/conventions.md b/docs/conventions.md index 2d83294..14905bb 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -130,8 +130,14 @@ too. - **main HEAD is sacrosanct — below the session anchor.** Nobody runs `git reset` / `git revert` on user-ratified main history (or any protected branch). main moves forward only via orchestrator commits; a - wrong agent diff is discarded with `git checkout -- ` / - `git stash`, never by rewinding ratified history. + wrong agent diff is discarded — by the orchestrator, between + iterations — with `git checkout -- ` / `git stash`, never by + rewinding ratified history. That discard idiom is the orchestrator's + alone: an agent running INSIDE a multi-task `implement` iteration + never checkouts/restores a file — nothing commits between tasks, so + it would silently destroy sibling tasks' uncommitted work; in-loop + recovery is forward-editing or `BLOCKED` (`implement/SKILL.md` Iron + Law, issue #23). **One narrow `/boss` exception — the rollback sandbox:** inside an autonomous run the orchestrator anchors on the `main` HEAD it started from (the last user-ratified state) and MAY `git reset --hard` its diff --git a/implement/SKILL.md b/implement/SKILL.md index cc7b2cd..98188ae 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -101,6 +101,7 @@ regression around the RED-first gate. ``` THE LOOP NEVER COMMITS. CODE EDITS, TESTS, AND THE STATS FILE LIVE IN THE WORKING TREE AS UNSTAGED CHANGES UNTIL THE ORCHESTRATOR COMMITS THEM. WITHIN THIS LOOP, MAIN HEAD IS SACROSANCT — NO RESET, NO REVERT; THE LOOP ONLY EVER WRITES UNSTAGED CHANGES. MAIN MOVES FORWARD ONLY VIA ORCHESTRATOR COMMITS. (THE `/boss` ROLLBACK SANDBOX WINDS BACK ALREADY-COMMITTED AUTONOMOUS WORK — A BOSS-LEVEL RECOVERY, NOT A STEP IN THIS LOOP.) +NO IN-LOOP AGENT EVER DISCARDS WORKING-TREE STATE VIA `git checkout` / `git restore` — WHOLE-FILE OR `--patch`: NOTHING COMMITS BETWEEN TASKS, SO A SHARED FILE CARRIES EARLIER TASKS' UNCOMMITTED DONE WORK, AND A CHECKOUT SILENTLY DESTROYS IT (issue #23). RECOVERY IS FORWARD — EDIT BACK, OR REPORT `BLOCKED`. (THE ORCHESTRATOR'S OWN END-OF-ITERATION `git checkout -- .` DISPOSITION HAPPENS OUTSIDE THE LOOP.) PER-TASK PHASES ARE SEPARATE AGENT CALLS IN THE WORKFLOW SCRIPT — implementer, then spec-compliance, then quality. SPEC COMPLIANCE IS GATED BEFORE QUALITY. TASKS RUN SEQUENTIALLY; A BLOCKED TASK STOPS THE LOOP — NO SKIP-AHEAD (TASK ORDERING DEPENDENCIES ARE UNKNOWN). NEVER PUSH PAST `BLOCKED` BY HAND. @@ -155,6 +156,32 @@ ground truth, never a self-report** — a self-report must never be able to The E2E phase's `status` is also read now (not just its fixtures): a non-`DONE` E2E status or a zero-fixture run surfaces as a concern. +**Cross-task discard guard** (issue #23). In a multi-task run nothing +commits between tasks, so one task's file-level `git checkout`/`git +restore` can silently destroy an earlier task's DONE work in a shared +file — invisible to the reviewers (scoped to the current task's +footprint) and to the tree-count gate (the surviving changes keep the +count nonzero). The loop therefore records a snapshot boundary after +every task (`git stash create` — a dangling commit; HEAD, index, and +working tree untouched) and hard-`BLOCK`s when a path that was +HEAD-modified at one boundary is HEAD-identical at the next: the +end-report names the lost paths, the boundary, and the recovery +snapshot sha the discarded content can be read from +(`git show :`). Coarse by design, in both directions: a +checkout followed by re-edits of the same file, or a `--patch`-level +restore of a single hunk, escapes the comparison (the path never +leaves the diff) — the snapshot still keeps any loss diagnosable and +recoverable; conversely a task that legitimately edits a shared file +back to byte-identical HEAD content trips the guard, so a tripped +verdict means *adjudicate against the snapshot*, not proven malice. +Renames do not trip it (the boundary diff runs `--no-renames`, so a +renamed file's old path stays listed as a deletion). Untracked files +sit outside the threat model (`git checkout -- ` cannot discard +them). Cost: one extra sonnet/medium snapshot call per task of a +multi-task run; single-task and mini runs skip the guard. The +detection details live in `workflows/implement-loop.js`, the single +source. + ## The Process — orchestrator side ### Step 1 — Run the `implement-loop` workflow diff --git a/implement/agents/implementer.md b/implement/agents/implementer.md index 35dde9f..e3ec492 100644 --- a/implement/agents/implementer.md +++ b/implement/agents/implementer.md @@ -162,8 +162,17 @@ substitute. Law from `tester.md` applies to your tests too. 8. Self-review: re-read `git diff HEAD`. Did it match the task text? Did you do anything not in the task text? - If yes, undo that part with `git checkout -- ` or - by editing back — controller curates scope, not you. + If yes, undo that part BY EDITING BACK — never with + `git checkout` / `git restore`, whole-file or + `-p`/`--patch`; the same holds for recovering from a + broken intermediate state: repair forward by editing, + or report `BLOCKED`. + Nothing commits between the tasks of an iteration, so a + file you touched may also carry EARLIER tasks' + uncommitted, already-reviewed work — `git diff HEAD` + shows those sibling chunks, and a file-level checkout + silently destroys them along with yours. Controller + curates scope, not you. Did the test you wrote actually fail before the GREEN code, or did you write it after? If after, delete the production code and start over. (TDD is letter-and- @@ -196,6 +205,10 @@ End every report with exactly one of: design ledger forbids) - hypothesis-space exhaustion (≥ 3 implementation strategies failed — architecture is wrong, escalate) + - unrecoverable intermediate state (the tree broke + mid-task and forward repair by editing failed — + checkout/restore is not an option: it destroys + sibling tasks' uncommitted work, Step 8) Never push past BLOCKED by hand. ## Output format diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index 18ba68b..543a46d 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -39,6 +39,28 @@ // implementer's call, not enforced in code (the script cannot tell a // cosmetic hold from an ignored bug) — hence the neutral concern label // and the orchestrator backstop, same trust placed in its other reports. +// • Cross-task discard guard (issue #23): nothing commits between tasks, +// so a task's file-level `git checkout`/`git restore` can silently +// destroy an EARLIER task's uncommitted DONE work in a shared file — +// invisible to the footprint-scoped reviewers and to the tree-count +// gate (the surviving changes keep the count nonzero). In a multi-task +// run the loop records a snapshot boundary after every task +// (`git stash create`: a dangling commit — HEAD, index, and tree +// untouched) and hard-BLOCKs when a path that was HEAD-modified at one +// boundary is HEAD-identical at the next, naming the lost paths and the +// recovery snapshot sha. Coarse by design, in BOTH directions: a +// checkout-then-re-edit of the same file or a `--patch`-level hunk +// restore escapes the comparison (the path never leaves the diff) — the +// snapshot still keeps any loss diagnosable; conversely a task that +// legitimately edits a shared file back to byte-identical HEAD content +// trips the guard, so the verdict reads "adjudicate against the +// snapshot", not proof of a discard. `--no-renames` keeps a staged +// rename's old path listed (as a deletion) rather than reading as a +// loss. The snapshot agent itself is prompt-bound to the read-only +// `stash create` (a bare `git stash` would move the tree; the following +// boundary would catch that wipe, but only after the fact). Untracked +// files sit outside the threat model (`git checkout -- ` cannot +// discard them). // • The script has no filesystem or shell access of its own — every // working-tree mutation (code, stats.json, BLOCKED.md) happens inside // an agent() call. @@ -50,8 +72,9 @@ // • Effort policy mirrors the model policy: every agent() call pins an // explicit effort — never the session inherit. Code-writing and review // steps run high, the opus quality gate runs xhigh, and schema-bound -// extraction/check steps (preflight, plan-index, plan-extract, mini-verify, -// tree-check, finalize) run medium. See docs/agent-template.md § effort. +// extraction/check steps (preflight, plan-index, plan-extract, snapshot, +// mini-verify, tree-check, finalize) run medium. See +// docs/agent-template.md § effort. // // Carrier — passed as the Workflow `args` object by the orchestrator: // mode: 'standard' | 'mini' @@ -319,6 +342,31 @@ const TREE_SCHEMA = { }, }, } +// Cross-task discard guard (issue #23): the per-boundary snapshot verdict. +// `git stash create` writes a dangling commit capturing the tracked +// working-tree state without touching HEAD, the index, or the tree; the +// HEAD-modified path set is the comparison key across boundaries. +const SNAP_SCHEMA = { + type: 'object', + additionalProperties: false, + required: ['snapshot_sha', 'modified_paths'], + properties: { + snapshot_sha: { + type: 'string', + description: + 'the sha printed by `git stash create "