diff --git a/.changeset/patient-birds-rest.md b/.changeset/patient-birds-rest.md new file mode 100644 index 000000000..262be4bac --- /dev/null +++ b/.changeset/patient-birds-rest.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3068 +--- +**The composer's load-bearing-fragment guarantee is now enforced, not just documented** — ADR-1671 promised a deterministic gate proving no load-bearing content is dropped or shrunk when context is trimmed to fit a budget; only synthetic unit tests existed. The gate now runs against real declared strategies and fails if it would ever assert over nothing. (#3065) diff --git a/docs/adr/1671-dynamic-context-management-platform.md b/docs/adr/1671-dynamic-context-management-platform.md index af82e968f..c5a773264 100644 --- a/docs/adr/1671-dynamic-context-management-platform.md +++ b/docs/adr/1671-dynamic-context-management-platform.md @@ -186,6 +186,37 @@ Pure Agent Skills (A alone) and pure MCP (D alone) were rejected as the foundati `DEFECT.AGENT-FILE-SIZE-CAP-BREACH` fix-forward), not by `when=` markers. Extending gating to agents would require a per-agent manifest family and a dispatch-time seam to read it; that is a separate decision, not an organic edit, and is not taken here. + + **Amended by #3065 (Phase 7) — the promised contract gate is built, and three records are + corrected.** A post-merge audit of every promise in this ADR against the merged tree found one + mitigation asserted-but-absent and two stale records. + + *The load-bearing contract gate now exists.* The Consequences section below claims, as amended by + #2931, that a deterministic gate proves no load-bearing fragment was omitted or shrunk. Until this + phase only synthetic unit tests of the `composeWithinBudget` primitive existed, over invented + fragments, asserting nothing about real content. `tests/load-bearing-contract-gate.test.cjs` now + derives the load-bearing set from declared `verbatim` strategies rather than a hand-maintained + list, sweeps a descending budget range, and carries both anti-vacuity guards as executable + assertions: an empty load-bearing set fails, and a sweep that never applies pressure fails. + + *Decision item 2 overstated what shipped.* It describes a composer that "selects the needed + fragments and trims by priority to fit that runtime's measured cap". `composeWorkflow` in fact + calls `composeWithinBudget` with `budget: Number.MAX_SAFE_INTEGER` and every fragment + `{kind:'verbatim'}` — non-lossiness is a structural guarantee of the strategy set, not a + large-budget trick, and no per-runtime trimming happens there. The emitted-byte cap is enforced by + a separate measure-and-fail gate, and Windsurf's limit by a bespoke description truncation + (#2931), not by this composer. Per-runtime trimming remains available in the strategy set and + unused; the wording above describes an option, not shipped behavior. + + *`flag:--converge` reaches a terminal state.* #2992 withheld six atoms and deferred them to the + rollout phase. Five were resolved explicitly. `flag:--converge` was resolved in code by reusing + `state:plan-strategy-converge` for `autonomous.md`'s converge sections, but that disposition was + recorded nowhere — the same undocumented-disposition gap #2995 closed for `flag:--verify-only`. + It is recorded here: **not admitted as its own atom; superseded by `state:plan-strategy-converge`.** + + *Open-questions numbering is corrected.* The list enumerates three questions, while two "Resolved + by" blocks below resolve a "Question 4" that was never added to it. Question 4 — index keying, + stable ids vs baked line numbers — is now listed explicitly. - **Budget unit:** bytes for emission caps (matches `lfByteCount`, deterministic, offline-safe); a token estimate for run-time selection. **Corrected by #2931 (Phase 4) — the Windsurf cap was never load-bearing.** The Context @@ -305,6 +336,7 @@ Prototype scope notes: the parser is intentionally self-contained for the exampl 1. Fragment unit: separate files vs in-file section markers? 2. Build-time emission vs run-time assembly as the primary surface during migration (double-write vs per-workflow cutover)? 3. Whether/when to invest in per-runtime native channels (skills, MCP) above the universal file floor. +4. Index keying: stable IDs vs baked `line` numbers? *(Resolved by #2928 — see below.)* **Resolved by #2928 — index keying: stable IDs, with no `line` field at all.** Question 4 asked stable IDs vs baked `line` numbers: `CONTEXT-INDEX.json` stored each predicate's `line`, so `--check` re-drifted on *any* `CONTEXT.md` line shift — a typo fix three sections up failed the gate. Raised by @davesienkowski (#1671, 2026-06-25). The shipped resolution is **stronger than the option originally proposed** (keying the comparison on stable IDs with `line` retained as non-compared metadata): the committed `ContextIndex.predicates` entries carry **no `line` field at all**. Committed-but-uncompared metadata goes silently stale — the same defect class the drift-guard exists to catch, with the alarm removed — so it was dropped from the committed artifact rather than merely excluded from the comparison. `line` is still returned by the live `parsePredicates`/`gsd-tools query context-predicates` result for callers that want to cite a source location; only the committed `docs/CONTEXT-INDEX.json` shape omits it. diff --git a/src/prompt-budget.cts b/src/prompt-budget.cts index 73c13bb9a..8e8979c1f 100644 --- a/src/prompt-budget.cts +++ b/src/prompt-budget.cts @@ -31,8 +31,13 @@ import contextComposer = require('./context-composer.cjs'); const NOTE_RESERVE_TOKENS = 80; -/** floor per plan when proportionally truncating and for the minimum-set check. */ -const MIN_PLAN_BYTES = 1024; +/** + * Floor per plan when proportionally truncating and for the minimum-set + * check. Exported so consumers (notably the load-bearing contract gate, + * issue #3065) never need to re-declare this value locally. + */ +export const PLAN_FLOOR_CHARS = 1024; +const MIN_PLAN_BYTES = PLAN_FLOOR_CHARS; const DEFAULT_NOTE_TEMPLATE = [ '', @@ -158,16 +163,22 @@ function assemblePrompt(parts: { } /** - * Apply a token budget to a set of review prompt sections. - * Returns the trimmed prompt and structured metadata. + * Build the `composeWithinBudget` fragment array for a set of review prompt + * sections, with each fragment's declared trim strategy attached. + * + * Extracted (issue #3065) so the load-bearing contract gate + * (tests/load-bearing-contract-gate.test.cjs) can assert directly over the + * REAL declared strategies used by `applyBudget`, rather than a hand-copied + * duplicate array. A duplicate would let production silently diverge from + * the gate (e.g. flipping `roadmap` from `verbatim` to `drop` would keep the + * gate green if it read from a copy) — exactly the + * `DEFECT.GENERATIVE-FIX` divergence class this extraction exists to + * prevent. Pure; performs no I/O and has no side effects. */ -export function applyBudget({ sections, budget, options = {} }: ApplyBudgetInput): BudgetResult { - const { - safetyMarginPct = 10, - noteTemplate = DEFAULT_NOTE_TEMPLATE, - projectMdHeadLines = 40, - } = options; - +export function buildBudgetFragments( + sections: PromptSections, + projectMdHeadLines: number +): contextComposer.Fragment[] { const { instructions, roadmap, @@ -198,7 +209,7 @@ export function applyBudget({ sections, budget, options = {} }: ApplyBudgetInput required: true, })); - const fragments: contextComposer.Fragment[] = [ + return [ { id: 'instructions', content: instructions, wrapper: '', strategy: { kind: 'verbatim' }, required: true }, { id: 'roadmap', content: roadmap, wrapper: '## Roadmap\n\n', strategy: { kind: 'verbatim' }, required: true }, { @@ -213,6 +224,27 @@ export function applyBudget({ sections, budget, options = {} }: ApplyBudgetInput { id: 'research', content: researchRaw ?? '', wrapper: '## Research\n\n', strategy: { kind: 'drop' } }, { id: 'requirements', content: requirementsRaw ?? '', wrapper: '## Requirements\n\n', strategy: { kind: 'drop' } }, ]; +} + +/** + * Apply a token budget to a set of review prompt sections. + * Returns the trimmed prompt and structured metadata. + */ +export function applyBudget({ sections, budget, options = {} }: ApplyBudgetInput): BudgetResult { + const { + safetyMarginPct = 10, + noteTemplate = DEFAULT_NOTE_TEMPLATE, + projectMdHeadLines = 40, + } = options; + + const { plans } = sections; + + const fragments: contextComposer.Fragment[] = buildBudgetFragments(sections, projectMdHeadLines); + + // Recover the per-plan fragment ids buildBudgetFragments assigned (in + // `plans` declaration order) rather than re-deriving the id-collision + // logic here — a single source of truth for id assignment. + const planIds: string[] = fragments.filter((f) => f.group === 'plans').map((f) => f.id); const composed = contextComposer.composeWithinBudget({ fragments, diff --git a/tests/load-bearing-contract-gate.test.cjs b/tests/load-bearing-contract-gate.test.cjs new file mode 100644 index 000000000..3adcfe8e2 --- /dev/null +++ b/tests/load-bearing-contract-gate.test.cjs @@ -0,0 +1,274 @@ +'use strict'; + +/** + * Deterministic load-bearing fragment contract gate. + * + * ADR-1671 Consequences → Negative/risks (as amended by #2931) claims a + * deterministic gate proves no load-bearing fragment is ever omitted or + * shrunk under budget pressure. Until #3065 that gate did not exist — only + * `tests/context-composer.test.cjs`, synthetic unit tests of the + * `composeWithinBudget` primitive over invented fragments, unconnected to + * any real declared strategy. + * + * This file: + * 1. IMPORTS the real fragment construction from `buildBudgetFragments` + * (src/prompt-budget.cts, exported for issue #3065) rather than + * hand-copying it. A hand-copy is exactly the `DEFECT.GENERATIVE-FIX` + * divergence class this gate exists to guard against: if production + * flips a fragment's strategy (e.g. `roadmap` from `verbatim` to + * `drop`), a copy would keep passing because it computes from stale + * duplicated data instead of the real declaration. Importing means the + * gate can only pass if the REAL declared strategies still hold. + * 2. Derives the load-bearing set mechanically — every fragment whose + * declared `strategy.kind === 'verbatim'` — rather than hand-listing + * ids, so the gate follows automatically if the upstream declarations + * change (test-matrix row 13). + * 3. Drives `composeWithinBudget` directly (not `applyBudget`, whose + * `BudgetMetadata` return type only exposes + * {budget, effectiveBudget, estimatedTokens, omitted, projectMdShrunk, + * planTruncationPct, hardFailed, noteInjected} — it does NOT expose + * `shrunk` (full array), `floored`, `underPressure`, `isolatePrefix`, + * or `hardFailReason`, all of which this gate must assert on). + * + * Module under test: gsd-core/bin/lib/context-composer.cjs + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { composeWithinBudget } = require('../gsd-core/bin/lib/context-composer.cjs'); +const { buildBudgetFragments, PLAN_FLOOR_CHARS } = require('../gsd-core/bin/lib/prompt-budget.cjs'); + +// Same estimator prompt-budget.cts uses (estimateTokens: chars/4, rounded up). +const measure = (text) => (text ? Math.ceil(text.length / 4) : 0); + +/** + * Build realistic, non-trivial input sections (not one-word placeholders) + * and call the REAL `buildBudgetFragments` (src/prompt-budget.cts) to get + * the fragment array, so the sweep below exercises real head-shrink and + * proportional-truncate behavior against production's actual declared + * strategies — not a hand-copied duplicate (issue #3065). + */ +function buildProductionFragments() { + const instructions = [ + '# Review Instructions', + '', + 'You are reviewing a change against the acceptance criteria below. Read every', + 'plan section fully before commenting. Cite file:line for every finding and', + 'never approve a PR missing a changeset entry or docs update. Flag any', + 'unbounded subprocess call, any missing timeout, and any test that asserts on', + 'wall-clock time. Findings must be actionable — no vague "consider improving"', + 'language. Severity is one of BLOCKER, MAJOR, MINOR, or NIT.', + ].join('\n'); + + const roadmap = [ + '## Roadmap', + '', + '1. Phase 1 — land the composer seam and characterize existing behavior.', + '2. Phase 2 — extract the trim ladder into a shared module.', + '3. Phase 3 — wire prompt-budget through the shared seam.', + '4. Phase 4 — add flexReserve and isolate-prefix support.', + '5. Phase 5 — this phase: build the deterministic contract gate.', + ].join('\n'); + + const projectMdLines = []; + for (let i = 0; i < 220; i++) { + projectMdLines.push(`Project note line ${i}: context that matters for review and must survive head-shrink.`); + } + const projectMd = projectMdLines.join('\n'); + + const planContent = (n) => { + const lines = []; + for (let i = 0; i < 45; i++) { + lines.push(`Plan ${n} step ${i}: a concrete, non-trivial implementation detail worth reviewing carefully and at length.`); + } + return lines.join('\n'); + }; + const plan1 = planContent(1); + const plan2 = planContent(2); + + const context = 'Context: ' + 'relevant background prose describing the surrounding subsystem and its invariants. '.repeat(20); + const research = 'Research: ' + 'prior art and precedent gathered before this change was proposed. '.repeat(20); + const requirements = 'Requirements: ' + 'acceptance criteria extracted verbatim from the linked issue. '.repeat(20); + + const sections = { + instructions, + roadmap, + plans: [ + { file: 'plan1.md', content: plan1 }, + { file: 'plan2.md', content: plan2 }, + ], + projectMd, + context, + research, + requirements, + }; + + return buildBudgetFragments(sections, 40); +} + +/** + * Derive the load-bearing set: every fragment whose declared strategy is + * {kind:'verbatim'}. NEVER hardcode this list — it must follow the + * production declarations mechanically (test-matrix row 13). + */ +function deriveLoadBearingSet(fragments) { + return fragments.filter((f) => f.strategy.kind === 'verbatim').map((f) => f.id); +} + +/** + * Anti-vacuity guard (test-matrix row 6 / design.md "Rejected" section). + * A gate that derives an empty load-bearing set proves nothing — it must + * fail loudly rather than silently pass an empty sweep. + */ +function assertLoadBearingSetNonEmpty(set) { + if (!Array.isArray(set) || set.length === 0) { + throw new Error( + 'load-bearing set is empty — a gate asserting over nothing proves nothing (test-matrix row 6)' + ); + } +} + +const composeOptions = () => ({ + safetyMarginPct: 10, + reserve: 80, + charsPerUnit: 4, + // Mirrors the minimumFor in src/prompt-budget.cts:230-234. + minimumFor: (f) => { + if (f.id === 'instructions' || f.id === 'roadmap') return f.content; + if (f.id.startsWith('plan:')) return f.content.slice(0, PLAN_FLOOR_CHARS); + return null; + }, +}); + +const fragments = buildProductionFragments(); +const loadBearing = deriveLoadBearingSet(fragments); +const total = fragments.reduce((sum, f) => sum + measure(f.content), 0); +const budgets = [ + total * 4, + total + 1, + total, + total - 1, + Math.floor(total * 0.75), + Math.floor(total * 0.5), + Math.floor(total * 0.25), +]; + +describe('load-bearing fragment contract gate (ADR-1671, #2931, #3065)', () => { + test('load-bearing set is derived (not hardcoded) and non-empty', () => { + assertLoadBearingSetNonEmpty(loadBearing); + // The production fragment array declares exactly three verbatim + // fragments today: instructions, roadmap, and plans-header (the empty + // "## Plans" section header). ADR-1671's own prose only names + // instructions/roadmap; plans-header is verbatim too and this + // mechanical derivation correctly includes it. + assert.deepEqual(loadBearing, ['instructions', 'roadmap', 'plans-header']); + }); + + test('budget sweep: no load-bearing fragment is ever omitted or shrunk; isolatePrefix is pinned; hardFailed never fires', () => { + let firstIsolatePrefix; + let sawUnderPressure = false; + + for (const budget of budgets) { + const result = composeWithinBudget({ fragments, budget, measure, options: composeOptions() }); + + if (result.metadata.hardFailed) { + assert.fail(`budget ${budget}: hardFailed with reason "${result.metadata.hardFailReason}"`); + } + + for (const id of loadBearing) { + assert.ok(!result.metadata.omitted.includes(id), `budget ${budget}: load-bearing "${id}" was omitted`); + assert.ok(!result.metadata.shrunk.includes(id), `budget ${budget}: load-bearing "${id}" was shrunk`); + } + + // `floored` is a PASS (design.md row 3 / "floored is a success, not a + // finding"): a non-empty `floored` means flexReserve prevented a cut + // that would otherwise have happened. Do NOT assert it is empty. + + // HONEST NOTE (#3065 review): no production `applyBudget` fragment + // currently sets `isolate: true`, so `isolatePrefix` is always `''` + // for the real production fragment set — this equality assertion by + // itself would be decorative (it could never observe a change, + // because there is nothing to observe). It is kept because it is + // still the correct invariant to hold, and its ability to actually + // detect drift is proven separately below (isolate-prefix pinning + // test), which builds a fragment set WITH an isolate fragment. + if (firstIsolatePrefix === undefined) { + firstIsolatePrefix = result.metadata.isolatePrefix; + } else { + assert.equal(result.metadata.isolatePrefix, firstIsolatePrefix, `budget ${budget}: isolatePrefix drifted from the first sampled budget`); + } + + if (result.metadata.underPressure) sawUnderPressure = true; + } + + // Row 7 anti-vacuity: the sweep must have applied real pressure at + // least once, or the assertions above prove nothing. + assert.ok(sawUnderPressure, 'no sampled budget reported underPressure — an always-unpressured sweep proves nothing (test-matrix row 7)'); + }); + + test('isolate-prefix pinning is a REAL check: a fragment set WITH isolate:true produces a non-empty, byte-identical isolatePrefix across budgets', () => { + // Proves the equality assertion in the budget-sweep test above is + // capable of failing, not merely decorative. Production has no + // isolate:true fragment today (see the HONEST NOTE above), so this + // constructs one explicitly. + const isolateFragments = [ + { id: 'canonical-header', content: 'STABLE CANONICAL PREFIX — must never move.', wrapper: '', strategy: { kind: 'verbatim' }, required: true, isolate: true }, + { id: 'instructions', content: 'Body instructions that may be trimmed under pressure. '.repeat(40), wrapper: '', strategy: { kind: 'verbatim' }, required: true }, + { id: 'context', content: 'Droppable context. '.repeat(40), wrapper: '', strategy: { kind: 'drop' } }, + ]; + const isolateTotal = isolateFragments.reduce((sum, f) => sum + measure(f.content), 0); + + const resultRoomy = composeWithinBudget({ fragments: isolateFragments, budget: isolateTotal * 4, measure, options: composeOptions() }); + const resultTight = composeWithinBudget({ fragments: isolateFragments, budget: Math.floor(isolateTotal * 0.3), measure, options: composeOptions() }); + + assert.notEqual(resultRoomy.metadata.isolatePrefix, '', 'isolatePrefix must be non-empty when an isolate:true fragment is present'); + assert.equal(resultRoomy.metadata.isolatePrefix, resultTight.metadata.isolatePrefix, 'isolatePrefix must stay byte-identical across budgets, including under severe pressure'); + assert.equal(resultTight.metadata.isolatePrefix, isolateFragments[0].content, 'isolatePrefix must equal the isolate fragment content verbatim'); + }); + + test('anti-vacuity row 7: a sweep of only budget=total*4 never reports underPressure, proving the guard CAN fail', () => { + const result = composeWithinBudget({ fragments, budget: total * 4, measure, options: composeOptions() }); + assert.equal(result.metadata.underPressure, false); + // If total*4 were the ONLY sampled budget in the sweep above, its + // `assert.ok(sawUnderPressure, ...)` would legitimately fail — this + // test demonstrates that failure condition is real, not vacuous. + }); + + test('row 12 negative space: a drop-strategy fragment IS allowed in `omitted` under severe pressure — that is the ladder working', () => { + const severeBudget = Math.floor(total * 0.25); + const result = composeWithinBudget({ fragments, budget: severeBudget, measure, options: composeOptions() }); + + assert.equal(result.metadata.hardFailed, false, `severe budget ${severeBudget} unexpectedly hardFailed: ${result.metadata.hardFailReason}`); + assert.ok( + result.metadata.omitted.includes('context'), + 'expected the drop-strategy "context" fragment to be omitted under severe pressure (ladder step 6/prompt-budget.cts:212)' + ); + for (const id of loadBearing) { + assert.ok(!result.metadata.omitted.includes(id), `load-bearing "${id}" must never be omitted, even under the same severe pressure that dropped "context"`); + } + }); + + test('anti-vacuity row 6 (executable): an all-non-verbatim fragment set derives an empty load-bearing set, and the guard throws on it', () => { + const noVerbatimFragments = [ + { id: 'projectMd', content: 'x'.repeat(2000), wrapper: '', strategy: { kind: 'head-shrink', maxLines: 10 } }, + { id: 'plan:only.md', content: 'y'.repeat(2000), wrapper: '', strategy: { kind: 'proportional-truncate', floorChars: PLAN_FLOOR_CHARS }, group: 'plans' }, + { id: 'context', content: 'z'.repeat(500), wrapper: '', strategy: { kind: 'drop' } }, + ]; + + const derived = deriveLoadBearingSet(noVerbatimFragments); + assert.deepEqual(derived, []); + assert.throws(() => assertLoadBearingSetNonEmpty(derived), /load-bearing set is empty/); + }); + + test('boundary: budget = total-1 / total / total+1 all hold the contract against the real measured total', () => { + for (const budget of [total - 1, total, total + 1]) { + const result = composeWithinBudget({ fragments, budget, measure, options: composeOptions() }); + assert.equal(result.metadata.hardFailed, false, `budget ${budget} unexpectedly hardFailed: ${result.metadata.hardFailReason}`); + for (const id of loadBearing) { + assert.ok(!result.metadata.omitted.includes(id)); + assert.ok(!result.metadata.shrunk.includes(id)); + } + } + }); +});