fix(implement-loop): guard against a task discarding sibling tasks' uncommitted work
During a multi-task run nothing commits between tasks, so one task's file-level `git checkout -- <file>` / `git restore <file>` silently destroys an earlier task's uncommitted DONE work in a shared file. The loss was structurally invisible: spec- and quality-reviewers are scoped to the current task's footprint, diff_fingerprint only detects same-task repair cycles, and the end-of-iteration tree gate counts files (the surviving changes keep it nonzero). Observed in the wild as a PARTIAL run whose DONE reports did not match the tree. Two layers, both plugin-side (harness-level prevention is ruled out — decision log on the issue): Prose guard — implementer.md Step 8 no longer sanctions file checkout for scope curation: over-reach is undone by editing back, a broken intermediate state is repaired forward or reported BLOCKED (new fourth BLOCKED bucket in the status protocol). A matching Iron Law line in implement/SKILL.md covers all in-loop agents, whole-file and --patch; docs/conventions.md now marks the checkout discard idiom as the orchestrator's, between iterations. Mechanical guard — after every task of a multi-task run a snapshot agent records `git stash create` (a dangling commit; HEAD, index, and tree untouched — semantics verified empirically in a scratch repo) plus the `git diff HEAD --name-only --no-renames` path set. A path that was HEAD-modified at one boundary and gone at the next trips a hard BLOCKED naming the lost paths, the boundary, and the recovery snapshot sha (`git show <sha>:<path>`); the discard verdict outranks per-task outcomes in blocked_detail since the reports and the tree have diverged. Coarse by design, in both directions, and documented as such: a checkout-then-re-edit or a --patch hunk restore escapes the comparison (the snapshot keeps it diagnosable); a legitimate back-to-HEAD edit trips it (the verdict says adjudicate against the snapshot). --no-renames keeps a staged rename from reading as a loss; untracked files are outside the threat model (checkout cannot discard them). Cost: one sonnet/medium call per task, multi-task runs only; single-task and mini runs are unchanged. Verified: node --check on the async-wrapped script and a stub-agent harness — discard trips the hard BLOCKED and stops the loop; an accumulating happy path, a plan-intended deletion, a single-task run, and a dead snapshot agent all pass without a false positive. closes #23
This commit is contained in:
@@ -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 -- <path>` 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 "<label>"` — empty string if it printed nothing (no tracked ' +
|
||||
'modification to snapshot)',
|
||||
},
|
||||
modified_paths: {
|
||||
type: 'array',
|
||||
items: { type: 'string' },
|
||||
description:
|
||||
'every path printed by `git diff HEAD --name-only --no-renames` (tracked modifications and deletions; ' +
|
||||
'`--no-renames` keeps a renamed file\'s old path listed as a deletion; untracked files are invisible ' +
|
||||
'here by design)',
|
||||
},
|
||||
},
|
||||
}
|
||||
const FINALIZE_SCHEMA = {
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
@@ -652,9 +700,50 @@ async function runTask(task) {
|
||||
}
|
||||
|
||||
const results = []
|
||||
// Cross-task discard guard (issue #23) state: the previous task boundary
|
||||
// (snapshot sha + HEAD-modified path set) and, when tripped, the discard
|
||||
// evidence. Only a multi-task run needs the guard — with a single task there
|
||||
// is no sibling work to protect.
|
||||
let prevBoundary = null
|
||||
let discard = null
|
||||
const guardConcerns = []
|
||||
for (const task of tasks) {
|
||||
const r = await runTask(task)
|
||||
results.push(r)
|
||||
if (tasks.length > 1) {
|
||||
const snap = await agent(
|
||||
`${STANDING}\n\nRecord a working-tree snapshot boundary WITHOUT changing anything. Run exactly these two ` +
|
||||
'commands and report their output:\n' +
|
||||
`1. \`git stash create "iter ${iter_id} post-task ${task.id}"\` — prints a sha (a dangling commit; it ` +
|
||||
'touches neither HEAD, nor the index, nor the working tree). Report it as snapshot_sha; empty string if ' +
|
||||
'it printed nothing.\n' +
|
||||
'2. `git diff HEAD --name-only --no-renames` — report every printed path as modified_paths.\n' +
|
||||
'NEVER run bare `git stash` or `git stash push/pop/apply/drop` — those MOVE the changes. Do NOT edit, ' +
|
||||
'stage, or commit anything.',
|
||||
{ model: 'sonnet', effort: 'medium', label: `snapshot:${task.id}`, phase: 'Per-task loop', schema: SNAP_SCHEMA },
|
||||
)
|
||||
if (snap && prevBoundary) {
|
||||
const now = new Set(snap.modified_paths || [])
|
||||
const lost = [...prevBoundary.paths].filter((p) => !now.has(p))
|
||||
if (lost.length) {
|
||||
// A path that was HEAD-modified at the previous boundary is
|
||||
// HEAD-identical now: a file-level checkout/restore discarded
|
||||
// sibling-task work mid-run. Hard stop — the per-task DONE reports
|
||||
// and the tree have diverged, so nothing downstream is trustworthy.
|
||||
discard = {
|
||||
lost_paths: lost,
|
||||
modified_through_task: prevBoundary.taskId,
|
||||
discarded_during_task: task.id,
|
||||
recovery_snapshot: prevBoundary.sha || null,
|
||||
}
|
||||
break
|
||||
}
|
||||
}
|
||||
if (!snap) guardConcerns.push(`discard-guard snapshot after task #${task.id} did not return — that boundary went unchecked`)
|
||||
// A non-returning snapshot agent skips the NEXT comparison (prevBoundary
|
||||
// null) rather than false-positiving against a stale boundary.
|
||||
prevBoundary = snap ? { taskId: task.id, sha: snap.snapshot_sha || null, paths: new Set(snap.modified_paths || []) } : null
|
||||
}
|
||||
if (r.outcome === 'BLOCKED') break // do not skip ahead — task ordering dependencies are unknown
|
||||
}
|
||||
|
||||
@@ -675,6 +764,29 @@ else outcome = 'BLOCKED'
|
||||
let synthBlock = null
|
||||
let noopClean = false
|
||||
|
||||
// Cross-task discard override (issue #23): a tripped discard guard outranks
|
||||
// every per-task outcome — the reports and the tree have diverged, so neither
|
||||
// DONE nor PARTIAL is trustworthy (the reported-DONE work may be the very
|
||||
// content that was destroyed). Hard BLOCKED; the recovery snapshot is a
|
||||
// dangling commit the orchestrator reads the discarded content from
|
||||
// (`git show <sha>:<path>`, `git diff <sha> HEAD -- <path>`).
|
||||
if (discard) {
|
||||
outcome = 'BLOCKED'
|
||||
synthBlock = {
|
||||
task: discard.discarded_during_task,
|
||||
reason:
|
||||
`working-tree discard detected: [${discard.lost_paths.join(', ')}] carried uncommitted edits through ` +
|
||||
`task #${discard.modified_through_task} and reverted to HEAD-identical during task #${discard.discarded_during_task} — ` +
|
||||
'a file-level checkout/restore destroyed sibling-task work (or, rarely, a task legitimately edited the ' +
|
||||
'file back to HEAD content); adjudicate against the recovery snapshot — until then the DONE reports ' +
|
||||
'covering these paths are not trustworthy',
|
||||
detail: discard.recovery_snapshot
|
||||
? `recovery snapshot (dangling commit): ${discard.recovery_snapshot} — read the discarded content via ` +
|
||||
`git show ${discard.recovery_snapshot}:<path>`
|
||||
: 'no recovery snapshot available: the pre-discard boundary printed no sha',
|
||||
}
|
||||
}
|
||||
|
||||
// ---- Phase 2.5 — verify (issue #12, facets 1+2+4) ----------------------
|
||||
// mini mode: independently OBSERVE the handed-off RED test is now GREEN (a no-op
|
||||
// leaves it RED while the implementer can still report DONE). standard mode: the
|
||||
@@ -781,10 +893,12 @@ const aggregate = {
|
||||
tasks_total: tasks.length,
|
||||
tasks_completed: completed.length,
|
||||
task_summaries: results.map((r) => ({ id: r.id, title: r.title, outcome: r.outcome })),
|
||||
concerns: [...results.flatMap((r) => r.concerns || []), ...e2eConcerns],
|
||||
blocked_detail: blocked
|
||||
? { task: blocked.id, reason: blocked.reason, detail: blocked.detail || null }
|
||||
: synthBlock,
|
||||
concerns: [...results.flatMap((r) => r.concerns || []), ...e2eConcerns, ...guardConcerns],
|
||||
// synthBlock (a discard or iteration-level no-op verdict) outranks the
|
||||
// per-task block: when the discard guard broke the loop on a task that
|
||||
// ALSO blocked, the discard is the cause and the task block the symptom.
|
||||
blocked_detail:
|
||||
synthBlock || (blocked ? { task: blocked.id, reason: blocked.reason, detail: blocked.detail || null } : null),
|
||||
}
|
||||
|
||||
const fin = await agent(
|
||||
|
||||
Reference in New Issue
Block a user