From 8040a6bac088140c6aa044dedf440c06164502f4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 18 Jun 2026 08:50:22 -0400 Subject: [PATCH] chore(#1417): add resolution-provenance CI guard (Resolution Provenance P4) (#1428) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds scripts/lint-resolution-provenance.cjs — a registry + ratchet CI guard that locks in the agent-skills configured_empty/not_configured contract tests so they cannot be silently removed, and establishes a registration point for future config-interpreting read verbs (ADR-1411 P4). Design rationale: - REGISTRY (one entry: agent-skills → src/init.cts → tests/agent-skills.test.cjs) is the canonical registration site; new verbs are added here. - For each registered verb, the guard asserts its test file contains BOTH a `configured_empty` assertion AND a `not_configured` assertion — proving the configured-empty-vs-not-configured contract is explicitly tested. - NOT a universal static detector (intractable / false positives) — mirrors the no-adhoc-markdown-parsing grandfather pattern. - Uses scripts/lib/allowlist-ratchet.cjs (assertWithinAllowlist) so stale allowlist entries fail (ratchet-down) and novel offenders always fail. - checkRegistry() is factored as a pure exported function tested in tests/lint-resolution-provenance.test.cjs without shelling out. - Wired into lint:ci (package.json) and lint step name updated in test.yml. - CONTEXT.md ### Resolution Convention extended with P4 guard sentence. - Allowlist starts empty ([]) — agent-skills already has its tests. Closes #1417 Part of #1411 Co-authored-by: Claude Opus 4.8 --- .github/workflows/test.yml | 2 +- CONTEXT.md | 2 +- package.json | 2 +- .../lint-resolution-provenance.allowlist.json | 1 + scripts/lint-resolution-provenance.cjs | 192 ++++++++++++++++++ tests/lint-resolution-provenance.test.cjs | 175 ++++++++++++++++ 6 files changed, 371 insertions(+), 3 deletions(-) create mode 100644 scripts/lint-resolution-provenance.allowlist.json create mode 100644 scripts/lint-resolution-provenance.cjs create mode 100644 tests/lint-resolution-provenance.test.cjs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 24d44cbde..468889813 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -108,7 +108,7 @@ jobs: # eslint invocation home; the --cache flag inside it is a no-op in CI # (node_modules/.cache is never restored) but still speeds local runs. # Each sub-lint prints its own banner, so a failure identifies itself. - - name: Lint — all (ESLint, skill deps, test-file count, command contract, PR checks, legacy name, regression-test names) + - name: Lint — all (ESLint, skill deps, test-file count, command contract, PR checks, legacy name, regression-test names, resolution-provenance) run: npm run lint:ci test: diff --git a/CONTEXT.md b/CONTEXT.md index efc1cb5a2..212fbb7b7 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -98,7 +98,7 @@ Module owning projection from project/workstream context to concrete `.planning` Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. ### Resolution Convention -Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P3, #1416). Config-interpreting read verbs expose `Resolution { value, configured, reason, warnings }` (`src/resolution.cts`); agent-skills is the first adopter, where `value = { block, skills_count }` and `source`/`degraded` remain config-provenance extras outside the envelope. Other read verbs expose at least `warnings[]` (e.g. capability-state `{ runtimeConfigDir, capabilities, warnings? }`) without `configured`/`reason`, which are meaningful only for config-interpreting verbs. Mutation verbs expose `warnings[]` (advisory) PLUS `errors[]` (operation-not-applied), e.g. capability-writer `{ capabilities, warnings, errors }`. The shared seam across all shapes is `warnings: string[]`; a single generic `Resolution` across read+write verbs was rejected by the deletion test (`configured`/`reason` are meaningless for capability verbs; `errors[]` cannot fold into `warnings[]`) — ADR-1411 P3 amendment. Recurrence prevention is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`), not by a shared envelope. +Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P3, #1416). Config-interpreting read verbs expose `Resolution { value, configured, reason, warnings }` (`src/resolution.cts`); agent-skills is the first adopter, where `value = { block, skills_count }` and `source`/`degraded` remain config-provenance extras outside the envelope. Other read verbs expose at least `warnings[]` (e.g. capability-state `{ runtimeConfigDir, capabilities, warnings? }`) without `configured`/`reason`, which are meaningful only for config-interpreting verbs. Mutation verbs expose `warnings[]` (advisory) PLUS `errors[]` (operation-not-applied), e.g. capability-writer `{ capabilities, warnings, errors }`. The shared seam across all shapes is `warnings: string[]`; a single generic `Resolution` across read+write verbs was rejected by the deletion test (`configured`/`reason` are meaningless for capability verbs; `errors[]` cannot fold into `warnings[]`) — ADR-1411 P3 amendment. Recurrence prevention is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`), not by a shared envelope. A CI guard (`scripts/lint-resolution-provenance.cjs`, wired into `lint:ci`) enforces that every registered config-interpreting read verb keeps a `configured_empty`/`not_configured` contract test; the registry in that script is the registration point for future verbs (ADR-1411 P4 / #1417). ### Worktree Safety Policy Module CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult`. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps)` (a projection over `resolveWorktreeContext`) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. diff --git a/package.json b/package.json index d71ef4417..4c0ea18f2 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ "pretest:coverage": "npm run build:lib && npm run lint:skill-deps", "lint": "eslint . --cache --cache-location node_modules/.cache/eslint/", "lint:fix": "eslint . --fix", - "lint:ci": "npm run lint && npm run lint:skill-deps && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-windows-test-portability.cjs && node scripts/lint-allow-test-rule-refs.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-windows-test-portability.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:windows-test-portability": "node scripts/lint-windows-test-portability.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", diff --git a/scripts/lint-resolution-provenance.allowlist.json b/scripts/lint-resolution-provenance.allowlist.json new file mode 100644 index 000000000..fe51488c7 --- /dev/null +++ b/scripts/lint-resolution-provenance.allowlist.json @@ -0,0 +1 @@ +[] diff --git a/scripts/lint-resolution-provenance.cjs b/scripts/lint-resolution-provenance.cjs new file mode 100644 index 000000000..a7920c757 --- /dev/null +++ b/scripts/lint-resolution-provenance.cjs @@ -0,0 +1,192 @@ +#!/usr/bin/env node +'use strict'; + +/** + * lint-resolution-provenance.cjs — CI guard for Resolution Provenance contracts. + * + * ## Purpose (ADR-1411 P4 / #1417) + * + * This is a REGRESSION-LOCK and REGISTRATION RATCHET, NOT a universal static + * detector (which is intractable given false positives from config-reading + * helpers that don't consume a `reason`). + * + * The guard maintains a REGISTRY of config-interpreting read verbs that MUST + * carry provenance — each entry names the verb, its source file, and its test + * file. For every registered verb, the guard asserts that its test file + * contains BOTH a `configured_empty` assertion AND a `not_configured` + * assertion, proving that the configured-empty-vs-not-configured contract is + * explicitly tested (not silently open-to-defaults). + * + * ## Registration protocol + * + * When adding a NEW config-interpreting read verb: + * 1. Add an entry to REGISTRY below: { verb, sourceFile, testFile }. + * 2. Add a `configured_empty` test and a `not_configured` test to testFile. + * 3. If the test coverage cannot land in the same PR, add the verb to + * scripts/lint-resolution-provenance.allowlist.json to grandfather it — + * but the allowlist MUST shrink over time (stale entries fail). + * + * See docs/adr/1411-resolution-provenance.md and #1417. + */ + +const fs = require('fs'); +const path = require('path'); +const { assertWithinAllowlist } = require('./lib/allowlist-ratchet.cjs'); +const { ExitError, runMain } = require('./lib/cli-exit.cjs'); + +const ROOT = path.join(__dirname, '..'); +const ALLOWLIST_PATH = path.join(__dirname, 'lint-resolution-provenance.allowlist.json'); + +/** + * Registry of config-interpreting read verbs that must carry provenance. + * Each entry: { verb, sourceFile, testFile } + * + * - verb: Short stable name for this verb (used in error messages and the + * allowlist). + * - sourceFile: Path (relative to ROOT) to the source implementation. + * - testFile: Path (relative to ROOT) to the test file that MUST contain + * both a `configured_empty` assertion and a `not_configured` + * assertion. + * + * Seed: agent-skills (P2/P3 fix, #1415/#1416) is the founding member. + */ +const REGISTRY = [ + { + verb: 'agent-skills', + sourceFile: 'src/init.cts', + testFile: 'tests/agent-skills.test.cjs', + }, +]; + +// Markers that MUST appear in every registered verb's test file. +const MARKER_CONFIGURED_EMPTY = 'configured_empty'; +const MARKER_NOT_CONFIGURED = 'not_configured'; + +/** + * Pure check logic — factored out for unit testing without I/O. + * + * @param {object} opts + * @param {Array<{verb: string, sourceFile: string, testFile: string}>} opts.registry + * The REGISTRY to check (or an injected subset for tests). + * @param {string[]} opts.allowlist + * Array of verb names to grandfather (stale entries fail). + * @param {function(string): string} opts.readFile + * Reads a file path and returns its content. Injected for testability; + * callers pass `(p) => fs.readFileSync(p, 'utf8')`. + * @param {function(string): void} opts.fail + * Callback invoked with a descriptive failure message. + * @returns {{ ok: boolean }} + */ +function checkRegistry({ registry, allowlist, readFile, fail }) { + const allowlistSet = new Set(allowlist); + const offenders = []; // verbs that ARE failing (for ratchet: stale check) + let anyFail = false; + + for (const entry of registry) { + const { verb, testFile } = entry; + const resolvedTestFile = path.isAbsolute(testFile) ? testFile : path.join(ROOT, testFile); + + // Grandfathered? Check markers anyway to detect when it's been fixed. + let content; + try { + content = readFile(resolvedTestFile); + } catch (err) { + fail( + `[resolution-provenance] Cannot read test file for verb "${verb}" (${testFile}): ${err.message}\n` + + ` Register the verb's test file correctly, or remove the registry entry.` + ); + anyFail = true; + offenders.push(verb); + continue; + } + + const hasConfiguredEmpty = content.includes(MARKER_CONFIGURED_EMPTY); + const hasNotConfigured = content.includes(MARKER_NOT_CONFIGURED); + + if (!hasConfiguredEmpty || !hasNotConfigured) { + offenders.push(verb); + + if (allowlistSet.has(verb)) { + // Grandfathered — tolerate but don't report. + continue; + } + + const missing = []; + if (!hasConfiguredEmpty) missing.push(`\`configured_empty\``); + if (!hasNotConfigured) missing.push(`\`not_configured\``); + + fail( + `[resolution-provenance] verb "${verb}" (${testFile}) is missing contract test marker(s):\n` + + ` Missing: ${missing.join(', ')}\n` + + ` Each registered config-interpreting read verb must have both a\n` + + ` \`configured_empty\` assertion and a \`not_configured\` assertion in its\n` + + ` test file to prove the configured-empty-vs-not-configured contract is\n` + + ` tested (ADR-1411 P4 / #1417).\n` + + ` Add the missing test(s) or grandfather the verb in\n` + + ` scripts/lint-resolution-provenance.allowlist.json.` + ); + anyFail = true; + } + } + + // Ratchet: stale allowlist entries (verb is compliant but still grandfathered) + // must be pruned so the allowlist only ever shrinks. + const offenderSet = new Set(offenders); + const staleEntries = []; + for (const v of allowlistSet) { + if (!offenderSet.has(v)) { + staleEntries.push(v); + } + } + + // Also verify stale entries via assertWithinAllowlist for consistent messaging. + const ratchetFailures = []; + assertWithinAllowlist({ + label: 'resolution-provenance', + current: offenders, + known: allowlist, + fail: (msg) => ratchetFailures.push(msg), + pruneHint: 'edit scripts/lint-resolution-provenance.allowlist.json', + }); + + // Only report stale entries from the ratchet (novel offenders are already + // reported above with more actionable messages). + if (staleEntries.length > 0) { + for (const msg of ratchetFailures) { + // Only surface the stale-entry message (it contains "stale" or "no longer"). + if (msg.includes('stale') || msg.includes('no longer')) { + fail(msg); + anyFail = true; + } + } + } + + return { ok: !anyFail }; +} + +function main() { + const allowlist = JSON.parse(fs.readFileSync(ALLOWLIST_PATH, 'utf8')); + + const failures = []; + const { ok } = checkRegistry({ + registry: REGISTRY, + allowlist, + readFile: (filePath) => fs.readFileSync(filePath, 'utf8'), + fail: (msg) => failures.push(msg), + }); + + if (!ok) { + for (const msg of failures) process.stderr.write(`${msg}\n`); + throw new ExitError(1); + } + + console.log( + `ok lint-resolution-provenance: ${REGISTRY.length} registered verb(s), all carry configured_empty + not_configured contract tests` + ); +} + +module.exports = { checkRegistry, REGISTRY }; + +// Only run the CLI check when executed directly, not when imported by tests +// (keeps the unit tests hermetic — importing checkRegistry must not run main). +if (require.main === module) runMain(main); diff --git a/tests/lint-resolution-provenance.test.cjs b/tests/lint-resolution-provenance.test.cjs new file mode 100644 index 000000000..c9962be6f --- /dev/null +++ b/tests/lint-resolution-provenance.test.cjs @@ -0,0 +1,175 @@ +'use strict'; + +/** + * Tests for scripts/lint-resolution-provenance.cjs — the registry-ratchet CI + * guard that locks in configured_empty / not_configured contract tests for + * every registered config-interpreting read verb (ADR-1411 P4 / #1417). + * + * Tests the PURE check logic (checkRegistry) directly, injecting fixture + * content rather than shelling out, so the suite is fast and hermetic. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); + +// Import the exported check function. +const { checkRegistry } = require('../scripts/lint-resolution-provenance.cjs'); + +/** + * Run checkRegistry with synthetic content injected as testFileContent so + * the test never reads from the real repo. Returns { ok, failures }. + * + * @param {object} opts + * @param {Array<{verb: string, sourceFile: string, testFile: string}>} opts.registry + * @param {string[]} opts.allowlist + * @param {string} opts.testFileContent - text that represents the test file's content + */ +function runCheck({ registry, allowlist, testFileContent }) { + const failures = []; + const { ok } = checkRegistry({ + registry, + allowlist, + // Inject a content-reader so the test is I/O-free. + readFile: (_filePath) => testFileContent, + fail: (msg) => failures.push(msg), + }); + return { ok, failures }; +} + +// ── suite ──────────────────────────────────────────────────────────────────── + +describe('lint-resolution-provenance: checkRegistry pure logic', () => { + const BOTH_MARKERS = ` + test('configured_empty: agent_skills[X]=[] ...', () => { + assert.strictEqual(ir.reason, 'configured_empty'); + }); + test('not_configured: agent not in map ...', () => { + assert.strictEqual(ir.reason, 'not_configured'); + }); + `; + + const MISSING_CONFIGURED_EMPTY = ` + test('not_configured: agent not in map ...', () => { + assert.strictEqual(ir.reason, 'not_configured'); + }); + `; + + const MISSING_NOT_CONFIGURED = ` + test('configured_empty: agent_skills[X]=[] ...', () => { + assert.strictEqual(ir.reason, 'configured_empty'); + }); + `; + + const sampleVerb = { verb: 'agent-skills', sourceFile: 'src/init.cts', testFile: 'tests/agent-skills.test.cjs' }; + + test('ok: registered verb whose test has both markers passes', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: [], + testFileContent: BOTH_MARKERS, + }); + assert.strictEqual(failures.length, 0, `Unexpected failures: ${failures.join('\n')}`); + assert.ok(ok); + }); + + test('fail: missing configured_empty marker → fails with actionable message', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: [], + testFileContent: MISSING_CONFIGURED_EMPTY, + }); + assert.ok(!ok); + assert.ok(failures.length > 0, 'Expected at least one failure'); + assert.match(failures.join('\n'), /configured_empty/); + assert.match(failures.join('\n'), /agent-skills/); + }); + + test('fail: missing not_configured marker → fails with actionable message', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: [], + testFileContent: MISSING_NOT_CONFIGURED, + }); + assert.ok(!ok); + assert.ok(failures.length > 0, 'Expected at least one failure'); + assert.match(failures.join('\n'), /not_configured/); + assert.match(failures.join('\n'), /agent-skills/); + }); + + test('fail: missing both markers → fails mentioning both', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: [], + testFileContent: '// no relevant markers here', + }); + assert.ok(!ok); + const msg = failures.join('\n'); + assert.match(msg, /configured_empty/); + assert.match(msg, /not_configured/); + }); + + test('tolerated: allowlisted verb with missing markers is skipped', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: ['agent-skills'], + testFileContent: MISSING_CONFIGURED_EMPTY, + }); + assert.strictEqual(failures.length, 0, `Unexpected failures: ${failures.join('\n')}`); + assert.ok(ok); + }); + + test('fail: stale allowlist entry (verb now compliant) must be pruned', () => { + const { ok, failures } = runCheck({ + registry: [sampleVerb], + allowlist: ['agent-skills'], + testFileContent: BOTH_MARKERS, + }); + assert.ok(!ok); + assert.match(failures.join('\n'), /agent-skills/); + assert.match(failures.join('\n'), /stale|prune|no longer/i); + }); + + test('ok: empty registry always passes', () => { + const { ok, failures } = runCheck({ + registry: [], + allowlist: [], + testFileContent: '', + }); + assert.strictEqual(failures.length, 0); + assert.ok(ok); + }); + + test('ok: multiple verbs all compliant passes', () => { + const verb2 = { verb: 'config-read', sourceFile: 'src/config.cts', testFile: 'tests/config.test.cjs' }; + const { ok, failures } = runCheck({ + registry: [sampleVerb, verb2], + allowlist: [], + testFileContent: BOTH_MARKERS, + }); + assert.strictEqual(failures.length, 0, `Unexpected failures: ${failures.join('\n')}`); + assert.ok(ok); + }); +}); + +describe('lint-resolution-provenance: real repo baseline', () => { + test('repo baseline passes (real registry vs real agent-skills.test.cjs)', () => { + // Run the actual check against the real registry and real test file. + // This is the regression lock: if agent-skills.test.cjs loses its markers + // the guard catches it here too. + const { checkRegistry: check, REGISTRY } = require('../scripts/lint-resolution-provenance.cjs'); + const failures = []; + const { ok } = check({ + registry: REGISTRY, + allowlist: [], + readFile: (filePath) => fs.readFileSync(filePath, 'utf8'), + fail: (msg) => failures.push(msg), + }); + assert.strictEqual( + failures.length, + 0, + `Real repo baseline failed:\n${failures.join('\n')}` + ); + assert.ok(ok); + }); +});