feat(#1123,#1124): enforce duplicate-producer invariant + fail-loud loadCentralConfigKeys in gen-capability-registry (#1131)
Closes #1123 Closes #1124 Refs #857
This commit is contained in:
Tom Boucher
@@ -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',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user