fix(implement-loop): guard plan-extract against silent truncation on big plans
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:<id> 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
This commit is contained in:
@@ -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:<id>) — 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) ------------------------------
|
||||
|
||||
Reference in New Issue
Block a user