From fd01e7a12e7183e1327efb08994b46f202034924 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 12 Jun 2026 18:39:57 -0400 Subject: [PATCH] feat(#1132): complete contribution hook prerequisite Closes #1132 --- scripts/gen-capability-registry.cjs | 67 +++++++--- scripts/run-tests.cjs | 8 +- src/loop-resolver.cts | 24 +++- tests/capability-registry.test.cjs | 182 ++++++++++++++++++++++++++++ tests/loop-render-hooks.test.cjs | 30 +++++ tests/run-tests-harness.test.cjs | 24 ++++ tests/tsconfig-noemit.test.cjs | 28 +++++ tsconfig.json | 8 +- 8 files changed, 347 insertions(+), 24 deletions(-) create mode 100644 tests/tsconfig-noemit.test.cjs diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index 5cca28125..c36490a21 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -970,6 +970,22 @@ function validateContribution(contrib, prefix) { errors.push(prefix + '.into must be a string (agent role name)'); } + if (!Array.isArray(contrib.produces)) { + errors.push(prefix + '.produces must be an array'); + } else { + for (const p of contrib.produces) { + if (typeof p !== 'string') errors.push(prefix + '.produces entries must be strings'); + } + } + + if (!Array.isArray(contrib.consumes)) { + errors.push(prefix + '.consumes must be an array'); + } else { + for (const c of contrib.consumes) { + if (typeof c !== 'string') errors.push(prefix + '.consumes entries must be strings'); + } + } + if (typeof contrib.fragment !== 'object' || contrib.fragment === null) { errors.push(prefix + '.fragment must be an object with path or inline key'); } else { @@ -978,6 +994,14 @@ function validateContribution(contrib, prefix) { if (!hasPath && !hasInline) { errors.push(prefix + '.fragment must have a "path" or "inline" key'); } + if (hasInline) { + const inline = contrib.fragment.inline; + if (typeof inline !== 'string') { + errors.push(prefix + '.fragment.inline must be a string'); + } else if (inline === '') { + errors.push(prefix + '.fragment.inline must be a non-empty string'); + } + } // S1: fragment.path traversal guard — must be a relative path with no ".." segments if (hasPath) { const p = contrib.fragment.path; @@ -1440,14 +1464,7 @@ function computeRequiresClosure(id, capMap) { // ─── Topological ordering ───────────────────────────────────────────────────── -/** - * Topologically sort steps at a given point by produces/consumes. - * Capability-id tiebreak for determinism. - * - * @param {{ capId: string, step: object }[]} entries - * @returns {{ capId: string, step: object }[]} - */ -function topoSortSteps(entries) { +function topoSortHookEntries(entries, hookKey, hookKind) { if (entries.length <= 1) return entries; // Build adjacency: entry A must come before entry B if B consumes something A produces @@ -1456,10 +1473,10 @@ function topoSortSteps(entries) { const adj = Array.from({ length: n }, () => []); for (let i = 0; i < n; i++) { - const producesI = new Set(entries[i].step.produces || []); + const producesI = new Set(entries[i][hookKey].produces || []); for (let j = 0; j < n; j++) { if (i === j) continue; - const consumesJ = entries[j].step.consumes || []; + const consumesJ = entries[j][hookKey].consumes || []; for (const artifact of consumesJ) { if (producesI.has(artifact)) { adj[i].push(j); @@ -1497,8 +1514,8 @@ function topoSortSteps(entries) { if (result.length < n) { const sortedIds = entries.map((e) => e.capId).join(', '); throw new Error( - 'produces/consumes cycle detected in steps at point "' + - (entries[0] && entries[0].step ? entries[0].step.point : '?') + + 'produces/consumes cycle detected in ' + hookKind + ' at point "' + + (entries[0] && entries[0][hookKey] ? entries[0][hookKey].point : '?') + '" among capabilities [' + sortedIds + ']: ' + 'a cycle in hook produces/consumes prevents deterministic ordering', ); @@ -1506,6 +1523,21 @@ function topoSortSteps(entries) { return result; } +/** + * Topologically sort steps at a given point by produces/consumes. + * Capability-id tiebreak for determinism. + * + * @param {{ capId: string, step: object }[]} entries + * @returns {{ capId: string, step: object }[]} + */ +function topoSortSteps(entries) { + return topoSortHookEntries(entries, 'step', 'steps'); +} + +function topoSortContributions(entries) { + return topoSortHookEntries(entries, 'contrib', 'contributions'); +} + // ─── ADR-857 Phase 4a: Derived views ───────────────────────────────────────── // FIX 5 (lazy requires): paths are declared at top level but the actual require() @@ -1948,14 +1980,9 @@ function buildRegistry(capMap) { ...e.step, })); - // Contributions: group by into, then capability-id order within group - const contribs = pointContribs.get(point); - contribs.sort((a, b) => { - const intoCompare = a.contrib.into.localeCompare(b.contrib.into); - if (intoCompare !== 0) return intoCompare; - return a.capId.localeCompare(b.capId); - }); - byLoopPoint[point].contributions = contribs.map((e) => ({ + // Contributions: topological sort by produces/consumes, cap-id tiebreak + const sortedContribs = topoSortContributions(pointContribs.get(point)); + byLoopPoint[point].contributions = sortedContribs.map((e) => ({ capId: e.capId, ...e.contrib, })); diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index d83054da1..c10ff2051 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -342,6 +342,9 @@ function main() { const MAX_CMDLINE_CHARS = process.env.RUN_TESTS_MAX_CMDLINE_CHARS ? Number(process.env.RUN_TESTS_MAX_CMDLINE_CHARS) : 28000; // headroom below the 32,767 Windows ceiling + const MAX_FILES_PER_CHUNK = process.env.RUN_TESTS_MAX_FILES_PER_CHUNK + ? Number(process.env.RUN_TESTS_MAX_FILES_PER_CHUNK) + : 180; // node:test does not exit until the event loop drains. A unit test that leaks // an open handle (un-terminated Worker, un-killed child_process, ref'd timer) @@ -362,7 +365,10 @@ function main() { let currentLen = FIXED_OVERHEAD; for (const file of selected) { const add = file.length + 1; // +1 for the inter-arg separator - if (current.length > 0 && currentLen + add > MAX_CMDLINE_CHARS) { + if ( + current.length > 0 && + (currentLen + add > MAX_CMDLINE_CHARS || current.length >= MAX_FILES_PER_CHUNK) + ) { chunks.push(current); current = []; currentLen = FIXED_OVERHEAD; diff --git a/src/loop-resolver.cts b/src/loop-resolver.cts index dd7b40b82..6087698b9 100644 --- a/src/loop-resolver.cts +++ b/src/loop-resolver.cts @@ -231,6 +231,7 @@ interface RawHook { point?: unknown; ref?: unknown; into?: unknown; + fragment?: unknown; produces?: unknown; consumes?: unknown; when?: unknown; @@ -246,6 +247,7 @@ interface ActiveHook { kind: HookKind; ref?: HookRef; into?: string; + fragment?: { inline?: string; path?: string }; when?: string; produces?: string[]; consumes?: string[]; @@ -325,6 +327,15 @@ function resolveLoopHooks(input: ResolveLoopHooksInput): ResolveLoopHooksResult return v.filter((x): x is string => typeof x === 'string'); } + function toFragment(v: unknown): { inline?: string; path?: string } | undefined { + if (!v || typeof v !== 'object' || Array.isArray(v)) return undefined; + const raw = v as Record; + const fragment: { inline?: string; path?: string } = {}; + if (typeof raw.inline === 'string') fragment.inline = raw.inline; + if (typeof raw.path === 'string') fragment.path = raw.path; + return Object.keys(fragment).length > 0 ? fragment : undefined; + } + // Process steps const stepsRaw = entryMap['steps']; const steps: RawHook[] = Array.isArray(stepsRaw) ? (stepsRaw as RawHook[]) : []; @@ -356,12 +367,14 @@ function resolveLoopHooks(input: ResolveLoopHooksInput): ResolveLoopHooksResult if (!isActive(hook)) continue; const capId = typeof hook['capId'] === 'string' ? hook['capId'] : ''; const into = typeof hook['into'] === 'string' ? hook['into'] : undefined; + const fragment = toFragment(hook['fragment']); const when = typeof hook['when'] === 'string' ? hook['when'] : undefined; const produces = toStringArray(hook['produces']); const consumes = toStringArray(hook['consumes']); const onError = typeof hook['onError'] === 'string' ? hook['onError'] : undefined; const active: ActiveHook = { capId, kind: 'contribution' }; if (into !== undefined) active.into = into; + if (fragment !== undefined) active.fragment = fragment; if (when !== undefined) active.when = when; if (produces.length > 0) active.produces = produces; if (consumes.length > 0) active.consumes = consumes; @@ -432,7 +445,12 @@ function renderLoopHooks(resolved: ResolveLoopHooksResult): string { } lines.push(''); } else if (hook.kind === 'contribution') { - lines.push(``); + lines.push(``); + if (hook.fragment?.inline) { + lines.push(hook.fragment.inline); + } else if (hook.fragment?.path) { + lines.push(`_Contribution fragment path is declared but not rendered by loop-resolver: ${hook.fragment.path}_`); + } if (hook.produces && hook.produces.length > 0) { lines.push(`- produces: ${hook.produces.join(', ')}`); } @@ -442,6 +460,10 @@ function renderLoopHooks(resolved: ResolveLoopHooksResult): string { if (hook.when) { lines.push(`- when: \`${hook.when}\``); } + if (hook.onError) { + lines.push(`- onError: ${hook.onError}`); + } + lines.push(''); lines.push(''); } else if (hook.kind === 'gate') { let checkStr = '(none)'; diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 1d2a97c5e..607aca6d5 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -373,6 +373,116 @@ describe('topological step ordering', () => { assert.strictEqual(sorted[0].capId, 'a-cap', 'a-cap should come first (alphabetical tiebreak)'); assert.strictEqual(sorted[1].capId, 'z-cap'); }); + + test('contributions at one point use produces/consumes dependency order', () => { + const capMap = new Map([ + ['a-consumer', { + id: 'a-consumer', + role: 'feature', + title: 'Consumer', + tier: 'full', + requires: [], + skills: [], + agents: [], + hooks: [], + config: {}, + steps: [], + contributions: [{ + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'Consume produced planning note.' }, + produces: [], + consumes: ['PLAN-NOTE.md'], + onError: 'skip', + }], + gates: [], + }], + ['b-producer', { + id: 'b-producer', + role: 'feature', + title: 'Producer', + tier: 'full', + requires: [], + skills: [], + agents: [], + hooks: [], + config: {}, + steps: [], + contributions: [{ + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'Produce planning note.' }, + produces: ['PLAN-NOTE.md'], + consumes: [], + onError: 'skip', + }], + gates: [], + }], + ]); + + const registry = buildRegistry(capMap); + assert.deepEqual( + registry.byLoopPoint['plan:pre'].contributions.map((c) => c.capId), + ['b-producer', 'a-consumer'], + ); + }); + + test('contribution produces/consumes cycle throws a clear error', () => { + const capMap = new Map([ + ['cap-a', { + id: 'cap-a', + role: 'feature', + title: 'A', + tier: 'full', + requires: [], + skills: [], + agents: [], + hooks: [], + config: {}, + steps: [], + contributions: [{ + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'A.' }, + produces: ['A.md'], + consumes: ['B.md'], + onError: 'skip', + }], + gates: [], + }], + ['cap-b', { + id: 'cap-b', + role: 'feature', + title: 'B', + tier: 'full', + requires: [], + skills: [], + agents: [], + hooks: [], + config: {}, + steps: [], + contributions: [{ + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'B.' }, + produces: ['B.md'], + consumes: ['A.md'], + onError: 'skip', + }], + gates: [], + }], + ]); + + assert.throws( + () => buildRegistry(capMap), + (err) => { + assert.ok(err instanceof Error); + assert.match(err.message, /contributions/); + assert.match(err.message, /cycle/); + return true; + }, + ); + }); }); // ─── 4. --check drift detection ────────────────────────────────────────────── @@ -1011,6 +1121,78 @@ describe('S1: fragment.path traversal guard', () => { assert.ok(errors.length > 0, 'Expected rejection for empty path'); assert.ok(errors.some((e) => e.includes('fragment.path'))); }); + + test('non-string fragment.inline is rejected', () => { + const cap = { + ...UI_CAP, + contributions: [ + { + point: 'plan:pre', + into: 'planner', + fragment: { inline: 42 }, + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.some((e) => e.includes('fragment.inline') && e.includes('string'))); + }); + + test('empty fragment.inline string is rejected', () => { + const cap = { + ...UI_CAP, + contributions: [ + { + point: 'plan:pre', + into: 'planner', + fragment: { inline: '' }, + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.some((e) => e.includes('fragment.inline') && e.includes('non-empty'))); + }); + + test('non-array contribution produces is rejected', () => { + const cap = { + ...UI_CAP, + contributions: [ + { + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'Plan with UI context.' }, + produces: 'PLAN-NOTE.md', + consumes: [], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.some((e) => e.includes('produces') && e.includes('array'))); + }); + + test('non-string contribution consumes entry is rejected', () => { + const cap = { + ...UI_CAP, + contributions: [ + { + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'Plan with UI context.' }, + produces: [], + consumes: [42], + when: 'workflow.ui_phase', + onError: 'skip', + }, + ], + }; + const errors = validateCapability(cap, 'ui'); + assert.ok(errors.some((e) => e.includes('consumes entries') && e.includes('strings'))); + }); }); // ─── 8. Security: prototype pollution (S2) ──────────────────────────────────── diff --git a/tests/loop-render-hooks.test.cjs b/tests/loop-render-hooks.test.cjs index 8e75784e9..959497538 100644 --- a/tests/loop-render-hooks.test.cjs +++ b/tests/loop-render-hooks.test.cjs @@ -650,12 +650,42 @@ describe('renderLoopHooks', () => { capId: 'contrib-cap', kind: 'contribution', into: 'planner', + fragment: { inline: 'Apply the project-specific planning guardrails.' }, }], }; const rendered = renderLoopHooks(resolved); assert.match(rendered, /contribution/); assert.match(rendered, /contrib-cap/); assert.match(rendered, /planner/); + assert.match(rendered, /Apply the project-specific planning guardrails\./); + assert.match(rendered, //); + assert.match(rendered, /<\/contribution>/); + assert.doesNotMatch(rendered, /]+\/>/); + }); + + test('resolveLoopHooks preserves contribution fragment data', () => { + const registry = makeRegistry({ + point: 'plan:pre', + contributions: [{ + capId: 'contrib-cap', + point: 'plan:pre', + into: 'planner', + fragment: { inline: 'Use artifact-backed evidence.' }, + produces: ['PLAN-NOTES.md'], + consumes: ['CONTEXT.md'], + when: 'workflow.contrib', + onError: 'halt', + }], + configSchema: { + 'workflow.contrib': { type: 'boolean', default: true, description: 'Enable test contribution.' }, + }, + }); + const resolved = resolveLoopHooks({ point: 'plan:pre', registry, config: {} }); + assert.strictEqual(resolved.activeHooks.length, 1); + assert.deepEqual(resolved.activeHooks[0].fragment, { inline: 'Use artifact-backed evidence.' }); + assert.deepEqual(resolved.activeHooks[0].produces, ['PLAN-NOTES.md']); + assert.deepEqual(resolved.activeHooks[0].consumes, ['CONTEXT.md']); + assert.strictEqual(resolved.activeHooks[0].onError, 'halt'); }); test('gate hook renders check, blocking, onError', () => { diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index 0dbe2c48c..3940afa0d 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -314,6 +314,30 @@ test('ambient GSD workstream vars are stripped by the runner', () => { `expected chunking marker in stderr; STDERR (tail):\n${r.stderr.split('\n').slice(-20).join('\n')}`, ); }); + + test('chunks by file count even when argv length is below the ceiling', () => { + const names = Array.from({ length: 7 }, (_, i) => `tiny-${String(i).padStart(2, '0')}.test.cjs`); + seed(tmpDir, names); + const r = runHarness(tmpDir, [], { + RUN_TESTS_MAX_CMDLINE_CHARS: '100000', + RUN_TESTS_MAX_FILES_PER_CHUNK: '3', + }); + assert.strictEqual( + r.status, + 0, + `expected zero exit; got status=${r.status} signal=${r.signal}\nSTDERR:\n${r.stderr}`, + ); + assert.match( + r.stderr, + /run-tests: chunk 1\/3 — 3 files/, + `expected file-count chunking marker in stderr; STDERR:\n${r.stderr}`, + ); + assert.match( + r.stderr, + /run-tests: chunk 3\/3 — 1 files/, + `expected final file-count chunking marker in stderr; STDERR:\n${r.stderr}`, + ); + }); }); describe('per-chunk timeout + force-exit (windows hang guard, #1051)', () => { diff --git a/tests/tsconfig-noemit.test.cjs b/tests/tsconfig-noemit.test.cjs new file mode 100644 index 000000000..99bc0790d --- /dev/null +++ b/tests/tsconfig-noemit.test.cjs @@ -0,0 +1,28 @@ +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +test('root tsconfig supports the default no-emit typecheck command', () => { + const root = path.join(__dirname, '..'); + const tscBin = path.join(root, 'node_modules', 'typescript', 'bin', 'tsc'); + + const result = spawnSync(process.execPath, [tscBin, '--noEmit'], { + cwd: root, + encoding: 'utf8', + }); + + assert.equal( + result.status, + 0, + [ + 'Expected the default root TypeScript typecheck to pass.', + 'stdout:', + result.stdout, + 'stderr:', + result.stderr, + ].join('\n'), + ); +}); diff --git a/tsconfig.json b/tsconfig.json index c5369eaef..016f3673f 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,4 +1,8 @@ { - "files": [], - "references": [] + "//": "Default editor/CI typecheck config. The emitting publish build stays in tsconfig.build.json.", + "extends": "./tsconfig.build.json", + "compilerOptions": { + "noEmit": true + }, + "include": ["src/**/*.cts"] }