* feat(#3661): make the code-review hook point configurable Add `workflow.code_review_point` (`execute:post` default, or `execute:wave:post`) so a multi-wave phase can run code review once per wave instead of once at the end, scoped to what changed since the phase's prior review. The code-review capability now declares its step at both loop points via a new generic `pointFrom` step field: `pointFrom` names an enum config key, and the step is only active at its own `point` when that key resolves to a matching value. `_resolvePointGate` (capability-activation.cts) is the single shared implementation consumed identically by loop-resolver.cts and capability-state.cts, and capability-validator.cjs enforces that `pointFrom` references an enum key whose values cover the declaring step's own point. code-review.md's manual-invocation gate now reads `workflow.code_review` directly instead of probing registry presence at the hardcoded execute:post point (so manual `/gsd-code-review` keeps working regardless of which automatic point is configured), and its file-scope tiers narrow to what changed since the phase's last review commit when one exists. execute-phase.md's wave-post step dispatch gets a small, precedented carve-out so the code-review skill still receives its required phase argument when dispatched generically (caught by the isolated spec review). Closes #3661 Emitted-Drift-Ack-Growth: code-review.md — #3661 adds a point-aware config gate check and LAST_REVIEW_COMMIT-based incremental scoping to the file-scope tiers. Emitted-Drift-Ack-Growth: execute-phase.md — #3661 adds one carve-out sentence so the wave-post generic step dispatch passes PHASE_NUMBER to the code-review skill. * docs: backfill changeset PR number for #3661 (#4159) * fix: scope tests/io.test.cjs's fs.writeSync fault-injection mocks by fd Five fault-injection mocks in the "bug #1008" describe blocks intercepted every fs.writeSync call regardless of file descriptor, and several threw or truncated unconditionally on the first call. This surfaced as an intermittent macOS CI failure: node:test's own IPC channel back to the parent process (which also goes through fs.writeSync internally) could get a bogus injected error or truncated write if node's internal machinery called it while one of these mocks was active, corrupting the message frame the parent tried to deserialize ("Unable to deserialize cloned data.", location tests/io.test.cjs:1:1, uncaughtException — a whole-file IPC crash, not a test assertion failure). Root cause confirmed by a working counter-example already in the same file: the "#3912 A6" mocks gate on `fd !== 2` before any fault injection and were never implicated. Applied the same fd-scoped pattern to the five unscoped mocks (four output()-targeting tests gate on fd 1, one error()-targeting test gates on fd 2), and added a regression test proving an unrelated fd passes through untouched while the fault-injection mock is active. Found while verifying #3661; unrelated to that change's own diff. --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -299,6 +299,112 @@ describe('activation filter', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── 2b. pointFrom gate (#3661, 50-test-matrix.md Section B) ────────────────
|
||||
|
||||
describe('activation filter: pointFrom gate (#3661, matrix Section B)', () => {
|
||||
test('B1: isActiveUnchangedForWhenOnlyStepTruthy — when-only step, when truthy → active (unchanged)', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }],
|
||||
});
|
||||
const config = { mytool: { on: true } };
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
assert.strictEqual(result.activeHooks.length, 1);
|
||||
});
|
||||
|
||||
test('B2: isActiveUnchangedForWhenOnlyStepFalsy — when-only step, when falsy → inactive (unchanged)', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on' }],
|
||||
});
|
||||
const config = { mytool: { on: false } };
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
assert.strictEqual(result.activeHooks.length, 0);
|
||||
});
|
||||
|
||||
test('B3: isActiveTrueWhenBothGatesPass — when truthy AND pointFrom matching point → active', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }],
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } },
|
||||
});
|
||||
const config = { mytool: { on: true } };
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
assert.strictEqual(result.activeHooks.length, 1);
|
||||
});
|
||||
|
||||
test('B4: isActiveFalseWhenPointFromMismatchesDespiteWhenTrue — AND semantics: pointFrom mismatch wins', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }],
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:post', description: 'x' } },
|
||||
});
|
||||
const config = { mytool: { on: true } };
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
assert.strictEqual(result.activeHooks.length, 0);
|
||||
});
|
||||
|
||||
test('B5: isActiveFalseWhenWhenFalseDespitePointFromMatch — AND semantics: when short-circuits', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }],
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } },
|
||||
});
|
||||
const config = { mytool: { on: false } };
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
assert.strictEqual(result.activeHooks.length, 0);
|
||||
});
|
||||
|
||||
test('B6: resolveLoopHooksSelectsExactlyOneOfTwoPointRegistrations — no cross-point leakage', () => {
|
||||
const byLoopPoint = {};
|
||||
for (const p of CANONICAL_POINTS_FALLBACK) byLoopPoint[p] = { steps: [], contributions: [], gates: [] };
|
||||
byLoopPoint['plan:pre'].steps = [
|
||||
{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' },
|
||||
];
|
||||
byLoopPoint['plan:post'].steps = [
|
||||
{ capId: 'test-cap', point: 'plan:post', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' },
|
||||
];
|
||||
const registry = {
|
||||
byLoopPoint,
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } },
|
||||
};
|
||||
const config = { mytool: { on: true } };
|
||||
const resultPre = resolveLoopHooks({ point: 'plan:pre', registry, config });
|
||||
const resultPost = resolveLoopHooks({ point: 'plan:post', registry, config });
|
||||
assert.strictEqual(resultPre.activeHooks.length, 1, 'point A must have exactly one active hook');
|
||||
assert.strictEqual(resultPost.activeHooks.length, 0, 'point B must have no active hooks — no leakage');
|
||||
});
|
||||
|
||||
test('B7: resolveLoopHooksBothPointsInactiveOnOutOfEnumValue — fail-closed on a typo', () => {
|
||||
const byLoopPoint = {};
|
||||
for (const p of CANONICAL_POINTS_FALLBACK) byLoopPoint[p] = { steps: [], contributions: [], gates: [] };
|
||||
byLoopPoint['plan:pre'].steps = [
|
||||
{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' },
|
||||
];
|
||||
byLoopPoint['plan:post'].steps = [
|
||||
{ capId: 'test-cap', point: 'plan:post', ref: { skill: 'a' }, when: 'mytool.on', pointFrom: 'mytool.point' },
|
||||
];
|
||||
const registry = {
|
||||
byLoopPoint,
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } },
|
||||
};
|
||||
const badConfig = { mytool: { on: true, point: 'bogus' } };
|
||||
const resultPre = resolveLoopHooks({ point: 'plan:pre', registry, config: badConfig });
|
||||
const resultPost = resolveLoopHooks({ point: 'plan:post', registry, config: badConfig });
|
||||
assert.strictEqual(resultPre.activeHooks.length, 0, 'an out-of-enum value must not revive the step at either point');
|
||||
assert.strictEqual(resultPost.activeHooks.length, 0, 'an out-of-enum value must not revive the step at either point');
|
||||
});
|
||||
|
||||
test('B8: isActiveFalseWhenCapabilityInactiveDespitePointFromMatch — capability gate still cascades', () => {
|
||||
const registry = makeRegistry({
|
||||
steps: [{ capId: 'test-cap', point: 'plan:pre', ref: { skill: 'my-skill' }, when: 'mytool.on', pointFrom: 'mytool.point' }],
|
||||
configSchema: { 'mytool.point': { type: 'enum', values: ['plan:pre', 'plan:post'], default: 'plan:pre', description: 'x' } },
|
||||
});
|
||||
const config = { mytool: { on: true } };
|
||||
const capabilityStatesById = new Map([['test-cap', { enabled: true, active: false }]]);
|
||||
const result = resolveLoopHooks({ point: 'plan:pre', registry, config, capabilityStatesById });
|
||||
assert.strictEqual(
|
||||
result.activeHooks.length, 0,
|
||||
'capability-level active=false must suppress the hook regardless of pointFrom match',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── 3. UI pilot integration tests ───────────────────────────────────────────
|
||||
|
||||
describe('UI pilot integration', () => {
|
||||
@@ -358,6 +464,42 @@ describe('UI pilot integration', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── 3b. #3661 — code-review capability.json + generated registry structural
|
||||
// assertions (50-test-matrix.md Section F, rows F2-F3) ────────────────
|
||||
//
|
||||
// Loads the REAL generated registry (built by `npm run build:lib` +
|
||||
// `node scripts/gen-capability-registry.cjs --write`) — no source-grep, this
|
||||
// is the same `realRegistry` object the "UI pilot integration" tests above
|
||||
// already use for behavioral (not text) assertions.
|
||||
|
||||
describe('#3661: code-review capability.json + generated registry (matrix Section F, F2-F3)', () => {
|
||||
test('F2: registryConfigSchemaHasCodeReviewPointEnum', () => {
|
||||
const slice = realRegistry.configSchema['workflow.code_review_point'];
|
||||
assert.ok(slice, 'registry.configSchema must contain workflow.code_review_point');
|
||||
assert.strictEqual(slice.type, 'enum');
|
||||
assert.deepStrictEqual(slice.values, ['execute:post', 'execute:wave:post']);
|
||||
assert.strictEqual(slice.default, 'execute:post');
|
||||
assert.strictEqual(slice.owner, 'code-review');
|
||||
});
|
||||
|
||||
test('F3: registryRegistersCodeReviewStepAtBothPoints', () => {
|
||||
const postSteps = realRegistry.byLoopPoint['execute:post'].steps;
|
||||
const wavePostSteps = realRegistry.byLoopPoint['execute:wave:post'].steps;
|
||||
assert.ok(Array.isArray(postSteps), 'execute:post.steps must be an array');
|
||||
assert.ok(Array.isArray(wavePostSteps), 'execute:wave:post.steps must be an array');
|
||||
|
||||
const postStep = postSteps.find((s) => s.capId === 'code-review' && s.ref && s.ref.skill === 'code-review');
|
||||
const wavePostStep = wavePostSteps.find((s) => s.capId === 'code-review' && s.ref && s.ref.skill === 'code-review');
|
||||
assert.ok(postStep, 'execute:post.steps must contain a code-review step. Got: ' + JSON.stringify(postSteps));
|
||||
assert.ok(wavePostStep, 'execute:wave:post.steps must contain a code-review step. Got: ' + JSON.stringify(wavePostSteps));
|
||||
});
|
||||
|
||||
// F4-F7 (CLI-behavioral: gsd_run loop render-hooks / config-set through the real
|
||||
// subprocess, against a temp git project) live in tests/code-review.test.cjs's
|
||||
// 'CR-CONFIG: config key registration' describe block, alongside the sibling
|
||||
// workflow.code_review / workflow.code_review_depth CLI round-trip tests.
|
||||
});
|
||||
|
||||
// ─── 4. Ordering tests ────────────────────────────────────────────────────────
|
||||
|
||||
describe('hook ordering', () => {
|
||||
|
||||
Reference in New Issue
Block a user