From eb051ea69636a5696523d47d09fe5360bc462762 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 12 Jun 2026 16:51:02 -0400 Subject: [PATCH] feat(#1123,#1124): enforce duplicate-producer invariant + fail-loud loadCentralConfigKeys in gen-capability-registry (#1131) Closes #1123 Closes #1124 Refs #857 --- ...y-registry-duplicate-producer-invariant.md | 5 + ...istry-loadcentralconfigkeys-parse-error.md | 6 + docs/adr/894-capability-declaration-format.md | 1 + scripts/gen-capability-registry.cjs | 75 +++++- tests/capability-registry.test.cjs | 227 +++++++++++++++++- 5 files changed, 298 insertions(+), 16 deletions(-) create mode 100644 .changeset/1123-capability-registry-duplicate-producer-invariant.md create mode 100644 .changeset/1124-capability-registry-loadcentralconfigkeys-parse-error.md diff --git a/.changeset/1123-capability-registry-duplicate-producer-invariant.md b/.changeset/1123-capability-registry-duplicate-producer-invariant.md new file mode 100644 index 000000000..06c13864a --- /dev/null +++ b/.changeset/1123-capability-registry-duplicate-producer-invariant.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 1131 +--- +**`gen-capability-registry` now rejects duplicate artifact producers at the same Loop Extension Point** — if two capability `steps` declare `produces: []` at the same point, the generator throws at gen time naming the artifact, the point, and the producing capability ids, instead of letting the topological sort pick a winner silently (which left ADR-857 Decision #6's data-flow contract undefined). The check counts distinct `(capId, stepIdx)` producer steps, so a single step listing an artifact twice does not false-positive. ADR-894 §4's enumerated cross-capability invariant list gains the artifact-production-uniqueness rule. (#1123) diff --git a/.changeset/1124-capability-registry-loadcentralconfigkeys-parse-error.md b/.changeset/1124-capability-registry-loadcentralconfigkeys-parse-error.md new file mode 100644 index 000000000..714474192 --- /dev/null +++ b/.changeset/1124-capability-registry-loadcentralconfigkeys-parse-error.md @@ -0,0 +1,6 @@ +--- +type: Changed +pr: 1131 +--- + +**`loadCentralConfigKeys` now fails loud on a malformed central config-schema instead of silently returning an empty Set** — `ENOENT` (the schema legitimately absent) still returns an empty Set silently, but a JSON parse error or any other read failure now writes a prominent `stderr` warning naming the schema file and throws `ExitError(1)`. Previously a single `catch (_)` swallowed parse errors too, so a merge-conflict marker or truncated write in `config-schema.manifest.json` made every capability config key look non-central — the config-key collision / `pending-migration` gate fired zero warnings and `--check` passed clean, defeating the gate invisibly. (#1124) diff --git a/docs/adr/894-capability-declaration-format.md b/docs/adr/894-capability-declaration-format.md index 5b4eba5fa..45ae203ff 100644 --- a/docs/adr/894-capability-declaration-format.md +++ b/docs/adr/894-capability-declaration-format.md @@ -132,6 +132,7 @@ Two generated artifacts, both following `gen-inventory-manifest`'s `--write`/`-- - `requires` exist, acyclic, **tier-monotone**; - hooks valid against the host contract (§3); - config-key ownership **exclusive AND complete** — a federated key must be owned by exactly one capability *and absent from the central `config-schema`* (presence in both = collision = a mid-flight migration; finish the move); + - artifact-production **unique per Loop Extension Point** — no two capability steps may `produce` the same artifact at the same Loop Extension Point (ambiguous data-flow resolution per Decision #6 — rejected at gen time); - emits the registry (§5). **`tier` is the source of profile/cluster membership.** Install profiles (`core`/`standard`/`full`) and surface clusters are **generated** from capability `tier` + the requires-closure — collapsing ADR-857's dual/triple toggle systems. `/gsd:surface` will operate on capabilities. (This generation lands in the phase-4 install integration; ADR-894 fixes the contract.) diff --git a/scripts/gen-capability-registry.cjs b/scripts/gen-capability-registry.cjs index f1d9f13b8..5cca28125 100644 --- a/scripts/gen-capability-registry.cjs +++ b/scripts/gen-capability-registry.cjs @@ -93,16 +93,42 @@ for (const entry of LOOP_HOST_CONTRACT) { * Loads the set of keys from the central config-schema manifest. * Returns a Set. Used for collision detection. * - * TODO: distinguish file-not-found (ok, return empty Set) from JSON-parse-error - * (should warn — a parse error means the schema is broken, not just absent). + * Contract: + * - ENOENT (file not found): returns empty Set silently — legitimate absent case. + * - Any other read error OR JSON parse error: writes a prominent warning to stderr + * naming the schema path and the underlying error, then throws ExitError(1). + * A parse error clearly states the schema is broken (not merely absent). + * + * @param {string} [schemaPath] Path to the config-schema manifest. Defaults to + * CONFIG_SCHEMA_PATH (the real production path). + * Overridable for unit testing with fixture paths. + * @returns {Set} */ -function loadCentralConfigKeys() { +function loadCentralConfigKeys(schemaPath = CONFIG_SCHEMA_PATH) { + let raw; try { - const manifest = JSON.parse(fs.readFileSync(CONFIG_SCHEMA_PATH, 'utf8')); - return new Set(Array.isArray(manifest.validKeys) ? manifest.validKeys : []); - } catch (_) { - return new Set(); + raw = fs.readFileSync(schemaPath, 'utf8'); + } catch (err) { + if (err.code === 'ENOENT') { + return new Set(); + } + process.stderr.write( + ' ERROR Failed to read config-schema manifest at ' + schemaPath + ': ' + err.message + '\n', + ); + throw new ExitError(1, 'could not read config-schema manifest'); } + + let manifest; + try { + manifest = JSON.parse(raw); + } catch (err) { + process.stderr.write( + ' ERROR Config-schema manifest at ' + schemaPath + ' is broken (JSON parse error): ' + err.message + '\n', + ); + throw new ExitError(1, 'config-schema manifest JSON is malformed'); + } + + return new Set(Array.isArray(manifest.validKeys) ? manifest.validKeys : []); } // ─── Config-slice validation ────────────────────────────────────────────────── @@ -1146,8 +1172,38 @@ function validateConsumesGlobal(capMap) { } } - // TODO: duplicate-producer invariant — if two capability steps produce the same artifact - // at the same point, that's ambiguous. Detect and reject as a follow-up. + // Duplicate-producer invariant: two capability steps may not produce the same artifact + // at the same Loop Extension Point. Same-point dual production makes data-flow resolution + // ambiguous and is rejected at gen time (Decision #6). + for (const artifact of Object.keys(capHookProducers)) { + if (artifact === '__proto__' || artifact === 'constructor' || artifact === 'prototype') continue; + const producers = capHookProducers[artifact]; + // Group by pointIdx + const byPoint = Object.create(null); + for (const entry of producers) { + if (!byPoint[entry.pointIdx]) byPoint[entry.pointIdx] = []; + byPoint[entry.pointIdx].push(entry); + } + for (const pointIdxStr of Object.keys(byPoint)) { + const group = byPoint[pointIdxStr]; + // Count distinct (capId, stepIdx) producer steps — a single step listing the same + // artifact twice in its produces array pushes duplicate entries but represents only + // ONE producer step and must not false-positive the cross-step gate. + const distinctProducers = new Set(group.map((e) => e.capId + ' ' + e.stepIdx)); + if (distinctProducers.size >= 2) { + const pointIdx = Number(pointIdxStr); + const pointName = POINT_ORDER[pointIdx]; + const capIds = [...new Set(group.map((e) => e.capId))].sort().join(', '); + throw new Error( + 'duplicate-producer invariant violated: artifact "' + artifact + '" is produced by ' + + 'two or more capability steps at the same Loop Extension Point "' + pointName + '" ' + + '(capabilities: ' + capIds + '). ' + + 'Two capability steps producing the same artifact at the same Loop Extension Point ' + + 'makes data-flow resolution ambiguous and is rejected at gen time.', + ); + } + } + } // Now check every hook step's consumes. // Self-consume rule: a step H cannot satisfy its own consumes[A] from its own produces[A]. @@ -2238,6 +2294,7 @@ module.exports = { validateConsumesGlobal, validateCrossCapability, classifyCrossErrors, + loadCentralConfigKeys, loadAndValidate, buildRegistry, serializeRegistry, diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 3cc92014f..1d2a97c5e 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -17,6 +17,8 @@ const path = require('node:path'); const { spawnSync } = require('node:child_process'); +const { cleanup } = require('./helpers.cjs'); + const { validateCapability, validateAgainstContract, @@ -50,6 +52,7 @@ const { VALID_EXTENDED_HOOK_EVENTS, VALID_PERMISSION_WRITERS, validateRuntimeBody, + loadCentralConfigKeys, } = require('../scripts/gen-capability-registry.cjs'); const ROOT = path.resolve(__dirname, '..'); @@ -1427,14 +1430,20 @@ describe('FIX 1: self-consume rejection in validateConsumesGlobal', () => { }); // (this test follows the series above) - test('a step produces:["SELF.md"] and consumes:["SELF.md"] and another capability produces SELF.md at the SAME point is accepted (different hook)', () => { + // Updated fixture: producer-cap produces SELF.md at plan:pre; self-cap CONSUMES (not produces) + // SELF.md at plan:pre — a single producer at that point satisfies the consume. + // The prior fixture had BOTH caps producing SELF.md at plan:pre, which now violates the + // duplicate-producer invariant added in Issue #1123. The consume-satisfiability logic being + // tested here is unaffected — the key assertion remains: a step consuming an artifact that + // a DIFFERENT cap produces at the SAME point is satisfied. + test('a step consumes:["SELF.md"] and another capability produces SELF.md at the SAME point is accepted (different cap)', () => { const producerCap = { id: 'producer-cap', role: 'feature', title: 'Producer', description: 'Produces SELF.md', tier: 'standard', requires: [], skills: ['producer-skill'], agents: ['gsd-producer-agent'], hooks: [], config: {}, steps: [ { - point: 'plan:pre', // same point as self-cap + point: 'plan:pre', ref: { skill: 'producer-skill' }, produces: ['SELF.md'], consumes: [], @@ -1443,22 +1452,22 @@ describe('FIX 1: self-consume rejection in validateConsumesGlobal', () => { ], contributions: [], gates: [], }; - const selfCap = { - id: 'self-cap', role: 'feature', title: 'Self', description: 'Self consume test', + const consumerCap = { + id: 'self-cap', role: 'feature', title: 'Self', description: 'Consume test', tier: 'standard', requires: [], skills: ['self-skill'], agents: ['gsd-self-agent'], hooks: [], config: {}, steps: [ { - point: 'plan:pre', // same point — different hook (producer-cap) satisfies it + point: 'plan:pre', // same point — producer-cap (different cap) satisfies the consume ref: { skill: 'self-skill' }, - produces: ['SELF.md'], + produces: [], consumes: ['SELF.md'], onError: 'skip', }, ], contributions: [], gates: [], }; - const capMap = new Map([['producer-cap', producerCap], ['self-cap', selfCap]]); + const capMap = new Map([['producer-cap', producerCap], ['self-cap', consumerCap]]); const errors = validateConsumesGlobal(capMap); const selfErrors = errors.filter((e) => e.includes('SELF.md') && e.includes('self-cap')); assert.deepEqual( @@ -3726,3 +3735,207 @@ describe('ADR-857 phase 5f: cross-field consistency gate rejection tests (DEFECT assert.deepEqual(errors, [], 'Valid fixture must produce no errors, got: ' + JSON.stringify(errors)); }); }); + +// ─── Change A: loadCentralConfigKeys ENOENT vs parse-error distinction ──────── + +describe('loadCentralConfigKeys — ENOENT vs parse-error (Issue #1124)', () => { + test('ENOENT: nonexistent path returns empty Set without throwing or writing stderr', () => { + const stderrWrites = []; + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (msg, ...rest) => { stderrWrites.push(msg); return origWrite(msg, ...rest); }; + let result; + try { + const nonexistent = path.join(os.tmpdir(), 'cfgkeys-nonexistent-' + Date.now() + '.json'); + result = loadCentralConfigKeys(nonexistent); + } finally { + process.stderr.write = origWrite; + } + assert.ok(result instanceof Set, 'should return a Set'); + assert.strictEqual(result.size, 0, 'Set should be empty for ENOENT'); + const warnings = stderrWrites.filter((m) => typeof m === 'string' && m.length > 0); + assert.deepEqual(warnings, [], 'No stderr output expected for ENOENT, got: ' + JSON.stringify(warnings)); + }); + + test('malformed JSON: throws and writes stderr containing the file path', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cfgkeys-')); + const badFile = path.join(tmpDir, 'bad-schema.json'); + fs.writeFileSync(badFile, '<<<<<<< HEAD\nnot json\n>>>>>>> main', 'utf8'); + const stderrWrites = []; + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (msg, ...rest) => { stderrWrites.push(msg); return origWrite(msg, ...rest); }; + let thrownErr; + try { + loadCentralConfigKeys(badFile); + } catch (err) { + thrownErr = err; + } finally { + process.stderr.write = origWrite; + } + // Clean up + cleanup(tmpDir); + assert.ok(thrownErr !== undefined, 'Expected loadCentralConfigKeys to throw on malformed JSON'); + assert.strictEqual(thrownErr.name, 'ExitError', 'thrown error must be an ExitError, got: ' + thrownErr.name); + assert.strictEqual(thrownErr.code, 1, 'ExitError must have code 1, got: ' + thrownErr.code); + const combined = stderrWrites.join(''); + assert.ok( + combined.includes(badFile), + 'stderr should include the file path, got: ' + combined, + ); + }); + + test('valid file: returns Set containing the declared keys', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cfgkeys-')); + const goodFile = path.join(tmpDir, 'good-schema.json'); + fs.writeFileSync(goodFile, JSON.stringify({ validKeys: ['a', 'b'] }), 'utf8'); + let result; + try { + result = loadCentralConfigKeys(goodFile); + } finally { + cleanup(tmpDir); + } + assert.ok(result instanceof Set, 'should return a Set'); + assert.ok(result.has('a'), "Set should contain 'a'"); + assert.ok(result.has('b'), "Set should contain 'b'"); + assert.strictEqual(result.size, 2, 'Set should have exactly 2 entries'); + }); + + test('non-ENOENT read error (path is a directory) throws ExitError and warns', () => { + // fs.readFileSync on a directory throws EISDIR (code !== 'ENOENT'), which must + // hit the non-ENOENT read-error branch: throw ExitError(1) and write stderr. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cfgkeys-dir-')); + const stderrWrites = []; + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (msg, ...rest) => { stderrWrites.push(msg); return origWrite(msg, ...rest); }; + let thrownErr; + try { + loadCentralConfigKeys(tmpDir); + } catch (err) { + thrownErr = err; + } finally { + process.stderr.write = origWrite; + } + cleanup(tmpDir); + assert.ok(thrownErr !== undefined, 'Expected loadCentralConfigKeys to throw on EISDIR (directory path)'); + assert.strictEqual(thrownErr.name, 'ExitError', 'thrown error must be an ExitError, got: ' + thrownErr.name); + assert.strictEqual(thrownErr.code, 1, 'ExitError must have code 1, got: ' + thrownErr.code); + const combined = stderrWrites.join(''); + assert.ok( + combined.includes(tmpDir), + 'stderr should include the directory path, got: ' + combined, + ); + }); +}); + +// ─── Change B: duplicate-producer invariant at gen time (Issue #1123) ───────── + +describe('duplicate-producer invariant — same artifact same point (Issue #1123)', () => { + // Minimal base capability clone helper (deep-clone UI_CAP then override key fields) + function makeFeatureCap(overrides) { + return Object.assign(JSON.parse(JSON.stringify(UI_CAP)), overrides); + } + + test('REJECTION: two caps each producing DUP.md at plan:pre → throws with artifact/point/ids in message', () => { + // Note: omit 'when' and use config:{} so there are no config-key validation errors. + // Caps must pass per-capability validation to enter capMap and trigger the global check. + const capA = makeFeatureCap({ + id: 'dup-a', + skills: ['dup-a-skill'], + agents: ['gsd-dup-a-agent'], + config: {}, + steps: [ + { + point: 'plan:pre', + ref: { skill: 'dup-a-skill' }, + produces: ['DUP.md'], + consumes: ['CONTEXT.md'], + onError: 'skip', + }, + ], + gates: [], + contributions: [], + }); + const capB = makeFeatureCap({ + id: 'dup-b', + skills: ['dup-b-skill'], + agents: ['gsd-dup-b-agent'], + config: {}, + steps: [ + { + point: 'plan:pre', + ref: { skill: 'dup-b-skill' }, + produces: ['DUP.md'], + consumes: ['CONTEXT.md'], + onError: 'skip', + }, + ], + gates: [], + contributions: [], + }); + // The duplicate-producer check fires during loadAndValidate (in validateConsumesGlobal) + // or during buildRegistry — test wraps the entire build flow. + const capDir = makeTempCapDir({ 'dup-a': capA, 'dup-b': capB }); + let threw = false; + let errorMsg = ''; + try { + const { capMap } = loadAndValidate(new Set(), capDir); + buildRegistry(capMap); + } catch (err) { + threw = true; + errorMsg = err.message || String(err); + } + assert.ok(threw, 'Expected build flow to throw for duplicate producers at the same point'); + assert.ok(errorMsg.includes('DUP.md'), 'Error message should mention DUP.md, got: ' + errorMsg); + assert.ok(errorMsg.includes('plan:pre'), 'Error message should mention the point, got: ' + errorMsg); + assert.ok( + errorMsg.includes('dup-a') && errorMsg.includes('dup-b'), + 'Error message should mention both cap ids, got: ' + errorMsg, + ); + }); + + test('PASSING: same artifact at DIFFERENT points does not trigger duplicate-producer error', () => { + // cap-diff-a produces SAME.md at plan:pre; cap-diff-b produces SAME.md at execute:pre + // Different pointIdx → must not throw the duplicate-producer error. + const capDiffA = makeFeatureCap({ + id: 'diff-a', + skills: ['diff-a-skill'], + agents: ['gsd-diff-a-agent'], + config: {}, + steps: [ + { + point: 'plan:pre', + ref: { skill: 'diff-a-skill' }, + produces: ['SAME.md'], + consumes: ['CONTEXT.md'], + onError: 'skip', + }, + ], + gates: [], + contributions: [], + }); + const capDiffB = makeFeatureCap({ + id: 'diff-b', + skills: ['diff-b-skill'], + agents: ['gsd-diff-b-agent'], + config: {}, + steps: [ + { + point: 'execute:pre', + ref: { skill: 'diff-b-skill' }, + produces: ['SAME.md'], + consumes: [], + onError: 'skip', + }, + ], + gates: [], + contributions: [], + }); + const capDir = makeTempCapDir({ 'diff-a': capDiffA, 'diff-b': capDiffB }); + const { capMap, errors } = loadAndValidate(new Set(), capDir); + assert.deepEqual(errors, [], 'Expected no validation errors for different-point fixtures, got: ' + JSON.stringify(errors)); + // buildRegistry must NOT throw — if the duplicate-producer bug regressed it would throw here + assert.doesNotThrow( + () => buildRegistry(capMap), + 'Should NOT throw duplicate-producer error for same artifact at DIFFERENT points', + ); + }); +});