From a75a821a2971d43f966820d6d7444a37ebfeb1ff Mon Sep 17 00:00:00 2001 From: Brummel Date: Mon, 6 Jul 2026 14:14:36 +0200 Subject: [PATCH] fix(implement-loop): guard plan-extract against silent truncation on big plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan-extract step pulled every requested task's verbatim block through a single schema-bound agent() response. On a large plan that response self-limits and returns a well-formed PREFIX (commonly just task 1); the only guard rejected an empty list, so a truncated non-empty subset ran to a clean DONE with the missing tasks never attempted, yet the end-report read as a completion. Replace the single-shot dump with an enumerate pass + per-task extraction, guarded by cardinality checks (issue #22 suggested either direction; this does both, plus a backstop the issue did not scope): - plan-index enumerates ids + one-line titles only (bounded output, self-limits far later than a verbatim dump) and reports total_tasks, the whole-plan count computed from the fully-readable plan text independently of the emitted list. - plan-extract: carries one task's verbatim block per agent() call, removing the across-all-tasks output ceiling. Calls fan out via parallel() (which honours the concurrency cap on a big plan); the requested id is bound inside each thunk and results are re-associated by it, never by array position, so the mapping holds regardless of the order parallel() resolves in. - Cardinality guards: (a) the whole-plan enumerate must list exactly total_tasks (the only backstop the no-range path can have without a caller range); (b) a task_range must be well-formed and covered exactly by the plan; (c) every expected id must extract non-empty text. Any breach is a hard BLOCKED with a precise "got N of M" reason, never a silent short run. Known residual (documented in-code): a single task whose own verbatim body overflows its dedicated response still returns an accepted prefix — bounded only by the planner's bite-sized-task invariant, far narrower than the closed vector. Verified by driving the real script through 11 extraction scenarios in a stubbed harness plus an order- and self-id-independence association test, and an adversarial three-lens review of the diff. closes #22 --- docs/agent-template.md | 2 +- implement/workflows/implement-loop.js | 188 ++++++++++++++++++++++++-- 2 files changed, 174 insertions(+), 16 deletions(-) diff --git a/docs/agent-template.md b/docs/agent-template.md index 7d95a9d..262e954 100644 --- a/docs/agent-template.md +++ b/docs/agent-template.md @@ -159,7 +159,7 @@ 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-extract, mini-verify, tree-check, finalize, build/suite verify) +plan-index, plan-extract, 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 diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index 23f1360..b6dd520 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -50,7 +50,7 @@ // • 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-extract, mini-verify, +// extraction/check steps (preflight, plan-index, plan-extract, mini-verify, // tree-check, finalize) run medium. See docs/agent-template.md § effort. // // Carrier — passed as the Workflow `args` object by the orchestrator: @@ -103,26 +103,51 @@ const PREFLIGHT_SCHEMA = { dirty_paths: { type: 'array', items: { type: 'string' } }, }, } -const TASKS_SCHEMA = { +// Plan extraction is a TWO-step, cardinality-guarded operation (issue #22), +// never a single unbounded verbatim dump. TASK_INDEX_SCHEMA carries the cheap +// enumerate pass (ids + one-line titles only — one short line per task, so it +// self-limits far later than a verbatim dump; the `total_tasks` field is a +// self-consistency backstop for the residual). TASK_TEXT_SCHEMA carries one +// task's verbatim body per agent call (per-task extraction removes the +// ACROSS-ALL-TASKS output-budget ceiling that silently truncated big plans to a +// prefix; a single task whose own body overflows one response is bounded only by +// the planner's bite-sized-task invariant — see the block comment below). +const TASK_INDEX_SCHEMA = { type: 'object', additionalProperties: false, - required: ['tasks'], + required: ['total_tasks', 'task_ids'], properties: { - tasks: { + total_tasks: { + type: 'integer', + description: + 'the TOTAL number of tasks the ENTIRE plan defines — count all of them directly (e.g. count the task ' + + 'headers), IGNORING any range filter. This is a self-consistency check: the plan text is fully readable ' + + '(input budget is ample), so this count is authoritative even if the id list below self-limits.', + }, + task_ids: { type: 'array', items: { type: 'object', additionalProperties: false, - required: ['id', 'title', 'text'], + required: ['id', 'title'], properties: { id: { type: 'integer' }, - title: { type: 'string' }, - text: { type: 'string', description: 'verbatim task block from the plan' }, + title: { type: 'string', description: 'one-line title ONLY — never the task body' }, }, }, }, }, } +const TASK_TEXT_SCHEMA = { + type: 'object', + additionalProperties: false, + required: ['id', 'title', 'text'], + properties: { + id: { type: 'integer' }, + title: { type: 'string' }, + text: { type: 'string', description: 'verbatim task block from the plan (this one task only)' }, + }, +} const IMPL_SCHEMA = { type: 'object', additionalProperties: false, @@ -282,16 +307,149 @@ if (mode === 'mini') { }, ] } else { - const extracted = await agent( - `${STANDING}\n\nRead the plan at ${plan_path}. Extract ` + - (task_range ? `tasks ${task_range[0]}..${task_range[1]} (inclusive, 1-based)` : 'every task') + - ', each as its VERBATIM block with the task id and a one-line title. Do not summarise or reorder. Do not edit anything.', - { model: 'sonnet', effort: 'medium', label: 'plan-extract', phase: 'Per-task loop', schema: TASKS_SCHEMA }, + // Plan extraction is split into an enumerate pass + per-task verbatim + // extraction, guarded by cardinality checks (issue #22). The old design + // pulled every requested task's VERBATIM block through ONE schema-bound + // response; on a large plan (many tasks, or a few tasks carrying long code + // blocks) that response self-limits and the agent returns a well-formed + // PREFIX of the tasks — commonly just task 1. The short list was non-empty, + // so the only guard (reject empty) passed, and the loop ran to a clean DONE + // over the truncated subset. The missing tasks were never attempted, yet the + // end-report read as a completion. This fix removes the across-all-tasks + // ceiling AND adds the backstops: + // • Enumerate pass (plan-index) — returns ONLY task ids + one-line titles, + // one short line per task, so it self-limits far later than a verbatim + // dump. It also reports `total_tasks` (the whole plan's task count, + // computed from the fully-readable plan text independently of the emitted + // list) — the self-consistency oracle the whole-plan path otherwise lacks. + // • Per-task extract (plan-extract:) — one agent() per id carries + // exactly one task's verbatim block, so no single response has to hold + // every task's text. Extraction is read-only and order-independent (unlike + // the implement loop, whose tasks carry ordering dependencies), so the + // calls fan out via parallel() (which also honours the concurrency cap on + // a big plan). The requested id is bound INSIDE each thunk and the results + // are re-associated by it — never by array position — so the mapping is + // correct regardless of the order parallel() resolves in. + // • Cardinality guards — (a) the enumerate must list exactly `total_tasks` + // on the whole-plan path; (b) a task_range must be well-formed and covered + // exactly by the plan; (c) every expected id must extract non-empty text. + // Any breach is a hard BLOCKED, never a silent short run. + // Residual (bounded, not closed): a SINGLE task whose own verbatim body + // overflows its dedicated per-task response would return a non-empty prefix + // that part-(c) accepts — there is no per-task round-trip oracle. This is far + // narrower than the closed across-tasks vector and rests on the planner's + // bite-sized-task invariant; it is the one truncation slice left open here. + const index = await agent( + `${STANDING}\n\nRead the plan at ${plan_path}. List the task ` + + (task_range + ? `ids in the range ${task_range[0]}..${task_range[1]} (inclusive, 1-based)` + : 'ids of EVERY task') + + ' that the plan actually defines, each with its one-line title. Return ONLY ids and titles — do NOT include ' + + 'any task body or code block. Do not summarise, invent, reorder, or pad the list. Separately report ' + + '`total_tasks`: the count of ALL tasks the entire plan defines, ignoring any range filter. Do not edit anything.', + { model: 'sonnet', effort: 'medium', label: 'plan-index', phase: 'Per-task loop', schema: TASK_INDEX_SCHEMA }, ) - if (!extracted || !extracted.tasks || extracted.tasks.length === 0) { - return { status: 'NEEDS_CONTEXT', iter_id, reason: `no tasks extracted from ${plan_path}` } + if (!index || !index.task_ids || index.task_ids.length === 0) { + return { status: 'NEEDS_CONTEXT', iter_id, reason: `no tasks found in ${plan_path}` } } - tasks = extracted.tasks + // Dedup the enumerated ids (first title wins); they define the enumerate set. + const titleById = new Map() + for (const t of index.task_ids) if (!titleById.has(t.id)) titleById.set(t.id, t.title) + const enumeratedIds = [...titleById.keys()].sort((a, b) => a - b) + const planTotal = typeof index.total_tasks === 'number' ? index.total_tasks : null + + // Which ids MUST be extracted, and the guard that the enumerate is complete. + let expectedIds + if (task_range) { + // Guard 0 — a well-formed 1-based [from, to]. A degenerate/inverted range + // (from > to, non-integer, < 1) is a caller error surfaced as its own + // BLOCKED, not swallowed into an empty task list that later mis-reports as + // a no-op iteration. + const [from, to] = task_range + if (!Number.isInteger(from) || !Number.isInteger(to) || from < 1 || to < from) { + return { + status: 'BLOCKED', + iter_id, + reason: `invalid task_range [${from}, ${to}]: expected a 1-based [from, to] with 1 <= from <= to`, + } + } + // Guard (b) — the range must be covered EXACTLY by the plan. A range that + // overshoots (e.g. [1,10] on a 5-task plan) is surfaced, not run short. + const wanted = [] + for (let i = from; i <= to; i++) wanted.push(i) + const missing = wanted.filter((i) => !titleById.has(i)) + if (missing.length) { + return { + status: 'BLOCKED', + iter_id, + reason: + `plan-extract cardinality mismatch: range ${from}..${to} asked ${wanted.length} task(s) but ` + + `${plan_path} defines ${planTotal ?? enumeratedIds.length} task(s) (missing in-range id(s): ${missing.join(', ')})`, + } + } + expectedIds = wanted + } else { + // Guard (a) — whole-plan path: the enumerate is the sole authority, so + // cross-check its own count. `total_tasks` is computed from the full plan + // independently of the emitted list; a listed-vs-counted mismatch is the + // enumerate self-limiting to a prefix — issue #22's symptom relocated to the + // (bounded) enumerate pass, and the only backstop this path can have without + // a caller-supplied range. + if (planTotal !== null && enumeratedIds.length !== planTotal) { + return { + status: 'BLOCKED', + iter_id, + reason: + `plan-index truncated: enumerated ${enumeratedIds.length} task(s) but the plan at ${plan_path} ` + + `defines ${planTotal} — the enumerate pass self-limited; re-run, do not proceed over a partial index`, + } + } + expectedIds = enumeratedIds + } + + // Per-task verbatim extraction — read-only and order-independent, so fan out + // via parallel(). The requested id is captured in the closure and returned + // alongside the result, so association is by id, never by resolution order. + // parallel() yields null for a thunk that threw / whose agent died; agent() + // itself returns null on a terminal error — both surface as a missing id below. + const extractedTasks = await parallel( + expectedIds.map((id) => () => + agent( + `${STANDING}\n\nRead the plan at ${plan_path}. Extract EXACTLY task ${id} (1-based) as its VERBATIM block — ` + + 'the full task text as written, including any code blocks. Do not summarise, truncate, reorder, or merge in ' + + 'any other task. Return that one task only. Do not edit anything.', + { model: 'sonnet', effort: 'medium', label: `plan-extract:${id}`, phase: 'Per-task loop', schema: TASK_TEXT_SCHEMA }, + ).then((t) => ({ reqId: id, t })), + ), + ) + + // Guard (c) — every expected id must have produced non-empty verbatim text. + // A null result (thunk threw, or agent died) or empty text is the truncation + // this issue is about; fail loudly with "got N of M". Keyed by the REQUESTED + // id carried in the closure (never the agent's self-reported id, so a + // mislabelled response cannot masquerade as coverage, and never array + // position, so parallel()'s resolution order is irrelevant). + const tasksById = new Map() + for (const slot of extractedTasks) { + if (!slot) continue // parallel() null: the thunk threw + const { reqId, t } = slot + if (t && typeof t.text === 'string' && t.text.trim()) { + const title = (t.title && t.title.trim()) || titleById.get(reqId) || `task ${reqId}` + tasksById.set(reqId, { id: reqId, title, text: t.text }) + } + } + const missingText = expectedIds.filter((id) => !tasksById.has(id)) + if (missingText.length) { + return { + status: 'BLOCKED', + iter_id, + reason: + `plan-extract truncated: got ${expectedIds.length - missingText.length} of ${expectedIds.length} task(s) ` + + `from ${plan_path} (missing id(s): ${missingText.join(', ')}) — re-run; do not proceed over a partial plan`, + } + } + // Restore plan order — expectedIds is already sorted ascending. + tasks = expectedIds.map((id) => tasksById.get(id)) } // ---- Phase 2 — per-task loop (sequential) ------------------------------