From 8fed13d0c80d523fb5ea53b9291cc245dc6eb0d3 Mon Sep 17 00:00:00 2001 From: pseudo Date: Sun, 23 Aug 2026 22:31:39 -0600 Subject: [PATCH] Scope check overrides per protocol, and stop warning about correct ones (#33) porch.checks is one flat map applied to every protocol, and protocols do not declare the same check names. Measured against this skeleton: air build, e2e_tests, pr_exists, tests bugfix build, regression_test, tests maintain build, tests pir build, plan_exists, pr_exists, review_has_*, tests spir/aspir build, e2e_tests, tests, + the artifact checks regression_test is BUGFIX's alone; e2e_tests is absent from BUGFIX, PIR and MAINTAIN. In a repo with no package.json the npm defaults cannot run, so those overrides are required -- and then every porch status on a protocol that does not declare the name printed "Unknown check override". Dropping the override broke the protocol that needed it; keeping it warned on every one that did not. Two halves. The issue offers them as alternatives; they fix different things. The warning now fires only for a name NO protocol anywhere declares. A name another protocol uses is applicable config that this one happens not to use, which is a different statement from "unknown", and the text says so. Without a workspace to check against, the old behaviour stands rather than falling silent -- silence there would turn a real typo into a no-op. And porch.byProtocol..checks states per-protocol overrides directly, merged field-by-field over the flat map with the per-protocol value winning. Wholesale replacement would mean a per-protocol skip silently discarded the global command for the same check. Keyed by canonical protocol name, so spir and spider are one entry rather than two spellings that can disagree. Reading the check names turned out to need both shapes: loadProtocol hoists per-phase check OBJECTS into a top-level map, so a protocol.json read raw from disk carries them under each phase instead. Reading only the top-level `checks` key found nothing at all in any protocol. Note the issue's own example uses `test`, singular. No protocol in this skeleton declares that name -- they all use `tests` -- so a literal `test` override still warns, correctly. Documented in all four porch skill copies. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/porch/SKILL.md | 19 ++ .codex/skills/porch/SKILL.md | 19 ++ codev-skeleton/.claude/skills/porch/SKILL.md | 19 ++ codev-skeleton/.codex/skills/porch/SKILL.md | 19 ++ .../issue-33-checks-by-protocol.test.ts | 220 ++++++++++++++++++ packages/codev/src/commands/porch/config.ts | 57 ++++- packages/codev/src/commands/porch/index.ts | 16 +- packages/codev/src/commands/porch/next.ts | 4 +- packages/codev/src/commands/porch/protocol.ts | 24 +- packages/codev/src/lib/config.ts | 19 ++ packages/codev/src/lib/skeleton.ts | 57 +++++ 11 files changed, 455 insertions(+), 18 deletions(-) create mode 100644 packages/codev/src/commands/porch/__tests__/issue-33-checks-by-protocol.test.ts diff --git a/.claude/skills/porch/SKILL.md b/.claude/skills/porch/SKILL.md index bbc26b6bc..83191e2cc 100644 --- a/.claude/skills/porch/SKILL.md +++ b/.claude/skills/porch/SKILL.md @@ -86,6 +86,25 @@ Checks are bounded at **300 seconds**. A suite that passes in 305s is reported a **Seconds, not milliseconds.** A value that is not a positive number is rejected with a warning and the default bound stays in force; it is never clamped, because a clamp would apply a bound nobody asked for. The `⚠ Check "…" overridden` line reports the bound that actually applied, so a rejected value cannot read as an accepted one. +### `byProtocol` — when one map cannot fit every protocol + +Protocols do not declare the same check names. `regression_test` is BUGFIX's alone; `e2e_tests` is absent from BUGFIX, PIR and MAINTAIN. A flat override for one of those is *required* by the protocol that has it and warned on every `porch status` for every protocol that does not. + +Two things changed. A flat override naming a check that **some** protocol declares no longer warns — it is applicable config that this protocol happens not to use. Only a name no protocol anywhere declares warns, and that one is a typo. + +And per-protocol overrides can be stated directly, merged field-by-field over the flat map with the per-protocol value winning: + +```jsonc +{ + "porch": { + "checks": { "build": { "command": "./infra/render.sh" } }, + "byProtocol": { "bugfix": { "checks": { "regression_test": { "skip": true } } } } + } +} +``` + +Keyed by protocol name or alias; an alias and its canonical name are the same entry, so `spir` and `spider` cannot silently disagree. + Keys are the **check names** from the protocol: `build` / `test` / `e2e_tests` live in phases; `build_succeeds` / `tests_pass` live in `phase_completion`. An override key that matches no check in the protocol prints a warning. Shipped by Spec #550. ## State storage diff --git a/.codex/skills/porch/SKILL.md b/.codex/skills/porch/SKILL.md index bbc26b6bc..83191e2cc 100644 --- a/.codex/skills/porch/SKILL.md +++ b/.codex/skills/porch/SKILL.md @@ -86,6 +86,25 @@ Checks are bounded at **300 seconds**. A suite that passes in 305s is reported a **Seconds, not milliseconds.** A value that is not a positive number is rejected with a warning and the default bound stays in force; it is never clamped, because a clamp would apply a bound nobody asked for. The `⚠ Check "…" overridden` line reports the bound that actually applied, so a rejected value cannot read as an accepted one. +### `byProtocol` — when one map cannot fit every protocol + +Protocols do not declare the same check names. `regression_test` is BUGFIX's alone; `e2e_tests` is absent from BUGFIX, PIR and MAINTAIN. A flat override for one of those is *required* by the protocol that has it and warned on every `porch status` for every protocol that does not. + +Two things changed. A flat override naming a check that **some** protocol declares no longer warns — it is applicable config that this protocol happens not to use. Only a name no protocol anywhere declares warns, and that one is a typo. + +And per-protocol overrides can be stated directly, merged field-by-field over the flat map with the per-protocol value winning: + +```jsonc +{ + "porch": { + "checks": { "build": { "command": "./infra/render.sh" } }, + "byProtocol": { "bugfix": { "checks": { "regression_test": { "skip": true } } } } + } +} +``` + +Keyed by protocol name or alias; an alias and its canonical name are the same entry, so `spir` and `spider` cannot silently disagree. + Keys are the **check names** from the protocol: `build` / `test` / `e2e_tests` live in phases; `build_succeeds` / `tests_pass` live in `phase_completion`. An override key that matches no check in the protocol prints a warning. Shipped by Spec #550. ## State storage diff --git a/codev-skeleton/.claude/skills/porch/SKILL.md b/codev-skeleton/.claude/skills/porch/SKILL.md index 8e2cd9937..987b54e40 100644 --- a/codev-skeleton/.claude/skills/porch/SKILL.md +++ b/codev-skeleton/.claude/skills/porch/SKILL.md @@ -90,4 +90,23 @@ Checks are bounded at **300 seconds**. A suite that passes in 305s is reported a **Seconds, not milliseconds.** A value that is not a positive number is rejected with a warning and the default bound stays in force; it is never clamped, because a clamp would apply a bound nobody asked for. The `⚠ Check "…" overridden` line reports the bound that actually applied, so a rejected value cannot read as an accepted one. +### `byProtocol` — when one map cannot fit every protocol + +Protocols do not declare the same check names. `regression_test` is BUGFIX's alone; `e2e_tests` is absent from BUGFIX, PIR and MAINTAIN. A flat override for one of those is *required* by the protocol that has it and warned on every `porch status` for every protocol that does not. + +Two things changed. A flat override naming a check that **some** protocol declares no longer warns — it is applicable config that this protocol happens not to use. Only a name no protocol anywhere declares warns, and that one is a typo. + +And per-protocol overrides can be stated directly, merged field-by-field over the flat map with the per-protocol value winning: + +```jsonc +{ + "porch": { + "checks": { "build": { "command": "./infra/render.sh" } }, + "byProtocol": { "bugfix": { "checks": { "regression_test": { "skip": true } } } } + } +} +``` + +Keyed by protocol name or alias; an alias and its canonical name are the same entry, so `spir` and `spider` cannot silently disagree. + Keys are the **check names** from the protocol: `build` / `test` / `e2e_tests` live in phases; `build_succeeds` / `tests_pass` live in `phase_completion`. An override key that matches no check in the protocol prints a warning. Shipped by Spec #550. diff --git a/codev-skeleton/.codex/skills/porch/SKILL.md b/codev-skeleton/.codex/skills/porch/SKILL.md index 8e2cd9937..987b54e40 100644 --- a/codev-skeleton/.codex/skills/porch/SKILL.md +++ b/codev-skeleton/.codex/skills/porch/SKILL.md @@ -90,4 +90,23 @@ Checks are bounded at **300 seconds**. A suite that passes in 305s is reported a **Seconds, not milliseconds.** A value that is not a positive number is rejected with a warning and the default bound stays in force; it is never clamped, because a clamp would apply a bound nobody asked for. The `⚠ Check "…" overridden` line reports the bound that actually applied, so a rejected value cannot read as an accepted one. +### `byProtocol` — when one map cannot fit every protocol + +Protocols do not declare the same check names. `regression_test` is BUGFIX's alone; `e2e_tests` is absent from BUGFIX, PIR and MAINTAIN. A flat override for one of those is *required* by the protocol that has it and warned on every `porch status` for every protocol that does not. + +Two things changed. A flat override naming a check that **some** protocol declares no longer warns — it is applicable config that this protocol happens not to use. Only a name no protocol anywhere declares warns, and that one is a typo. + +And per-protocol overrides can be stated directly, merged field-by-field over the flat map with the per-protocol value winning: + +```jsonc +{ + "porch": { + "checks": { "build": { "command": "./infra/render.sh" } }, + "byProtocol": { "bugfix": { "checks": { "regression_test": { "skip": true } } } } + } +} +``` + +Keyed by protocol name or alias; an alias and its canonical name are the same entry, so `spir` and `spider` cannot silently disagree. + Keys are the **check names** from the protocol: `build` / `test` / `e2e_tests` live in phases; `build_succeeds` / `tests_pass` live in `phase_completion`. An override key that matches no check in the protocol prints a warning. Shipped by Spec #550. diff --git a/packages/codev/src/commands/porch/__tests__/issue-33-checks-by-protocol.test.ts b/packages/codev/src/commands/porch/__tests__/issue-33-checks-by-protocol.test.ts new file mode 100644 index 000000000..f6599f554 --- /dev/null +++ b/packages/codev/src/commands/porch/__tests__/issue-33-checks-by-protocol.test.ts @@ -0,0 +1,220 @@ +/** + * Issue #33 — a correct check override that warns on every `porch status`. + * + * `porch.checks` is one flat map applied to every protocol, and protocols do not + * declare the same check names. Measured against the skeleton in this repo: + * + * air build, e2e_tests, pr_exists, tests + * bugfix build, regression_test, tests + * maintain build, tests + * pir build, plan_exists, pr_exists, review_has_*, tests + * spir/aspir build, e2e_tests, tests, + the artifact checks + * + * `regression_test` exists only in BUGFIX; `e2e_tests` is absent from BUGFIX, + * PIR and MAINTAIN. In a repo with no package.json the npm defaults cannot run, + * so those overrides are required — and then every `porch status` on a protocol + * that does not declare the name printed: + * + * ⚠ Unknown check override "regression_test" (not found in protocol) + * + * There was no way to satisfy both. Dropping the override broke the protocol + * that needed it; keeping it warned on every protocol that did not. + * + * (The issue's own example uses `test`, singular. No protocol in this skeleton + * declares that name — they all use `tests` — so a literal `test` override still + * warns, correctly.) + * + * Two halves, offered in the issue as alternatives. They fix different things: + * `byProtocol` lets the config say what it means, and the warning fix stops + * punishing a flat override that is simply not used by this protocol. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { tmpdir } from 'node:os'; +import { getPhaseChecks } from '../protocol.js'; +import { loadCheckOverrides } from '../config.js'; +import { listAllCheckNames } from '../../../lib/skeleton.js'; +import type { Protocol } from '../types.js'; + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..', '..', '..'); + +const spir = { + name: 'spir', + phases: [{ id: 'implement', checks: ['build', 'tests'] }], + checks: { build: { command: 'npm run build' }, tests: { command: 'npm test' } }, +} as unknown as Protocol; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +function captureStderr(): { lines: string[] } { + const lines: string[] = []; + vi.spyOn(process.stderr, 'write').mockImplementation((chunk: unknown) => { + lines.push(String(chunk)); + return true; + }); + return { lines }; +} + +/** A throwaway workspace with a .codev/config.json. */ +function withConfig(config: unknown, fn: (root: string) => void): void { + const root = fs.mkdtempSync(path.join(tmpdir(), 'i33-')); + fs.mkdirSync(path.join(root, '.codev'), { recursive: true }); + fs.writeFileSync(path.join(root, '.codev', 'config.json'), JSON.stringify(config, null, 2)); + try { + fn(root); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +describe('#33: the warning that punished a correct override', () => { + it('does not warn for a name another protocol declares', () => { + // `regression_test` is BUGFIX's alone. SPIR has no such check, and that is + // not an error — the override simply does not apply here. + const { lines } = captureStderr(); + + getPhaseChecks(spir, 'implement', { regression_test: { skip: true } }, REPO_ROOT); + + expect(lines.join('')).toBe(''); + }); + + it('still warns for a name NO protocol declares, which really is a typo', () => { + const { lines } = captureStderr(); + + getPhaseChecks(spir, 'implement', { tset: { command: './run-tests.sh' } }, REPO_ROOT); + + expect(lines.join('')).toContain('"tset"'); + expect(lines.join('')).toContain('not declared by any protocol'); + }); + + it('keeps warning when no workspace is given, rather than falling silent', () => { + // Without a workspace there is nothing to check the name against. Silence + // there would turn a real typo into a no-op, so the old behaviour stands. + const { lines } = captureStderr(); + + getPhaseChecks(spir, 'implement', { regression_test: { skip: true } }); + + expect(lines.join('')).toContain('"regression_test"'); + }); + + it('never warns for a name this protocol does declare', () => { + const { lines } = captureStderr(); + + getPhaseChecks(spir, 'implement', { tests: { command: './run-tests.sh' } }, REPO_ROOT); + + expect(lines.join('')).toBe(''); + }); +}); + +describe('#33: listAllCheckNames', () => { + it('finds names from protocols other than the one being run', () => { + const names = listAllCheckNames(REPO_ROOT); + + expect(names.has('regression_test')).toBe(true); // bugfix only + expect(names.has('e2e_tests')).toBe(true); // air, aspir, spir + expect(names.has('tests')).toBe(true); // most + expect(names.has('build')).toBe(true); // all + }); + + it('includes phase_completion predicates, which porch.checks also overrides', () => { + const names = listAllCheckNames(REPO_ROOT); + + expect(names.has('tests_pass') || names.has('build_succeeds')).toBe(true); + }); + + it('reports nothing rather than throwing when the tiers cannot be read', () => { + expect(() => listAllCheckNames(path.join(tmpdir(), 'i33-does-not-exist'))).not.toThrow(); + }); +}); + +describe('#33: byProtocol lets the config say what it means', () => { + it('applies a per-protocol override that the flat map does not carry', () => { + withConfig( + { porch: { byProtocol: { bugfix: { checks: { regression_test: { command: './run-tests.sh' } } } } } }, + root => { + expect(loadCheckOverrides(root, 'bugfix')?.regression_test?.command).toBe('./run-tests.sh'); + }, + ); + }); + + it('does not leak one protocol’s override into another', () => { + // The whole point. A BUGFIX-only override must not reach SPIR. + withConfig( + { porch: { byProtocol: { bugfix: { checks: { regression_test: { command: './run-tests.sh' } } } } } }, + root => { + expect(loadCheckOverrides(root, 'spir')).toBeNull(); + }, + ); + }); + + it('merges field-by-field over the flat map, per-protocol winning', () => { + // Wholesale replacement would mean a per-protocol `skip` silently discarded + // the global `command` for the same check. + withConfig( + { + porch: { + checks: { build: { command: './render.sh', cwd: 'infra' } }, + byProtocol: { bugfix: { checks: { build: { timeout: 900 } } } }, + }, + }, + root => { + const o = loadCheckOverrides(root, 'bugfix'); + + expect(o?.build).toEqual({ command: './render.sh', cwd: 'infra', timeout: 900 }); + }, + ); + }); + + it('leaves the flat map alone for a protocol with no entry', () => { + withConfig( + { + porch: { + checks: { build: { command: './render.sh' } }, + byProtocol: { bugfix: { checks: { regression_test: { skip: true } } } }, + }, + }, + root => { + expect(loadCheckOverrides(root, 'spir')).toEqual({ build: { command: './render.sh' } }); + }, + ); + }); + + it('treats an alias and its canonical name as the same entry', () => { + // `spider` is spir. Resolving by spelling would make the config depend on + // which one happened to be typed. + withConfig( + { porch: { byProtocol: { spider: { checks: { tests: { skip: true } } } } } }, + root => { + expect(loadCheckOverrides(root, 'spir')?.tests?.skip).toBe(true); + }, + ); + }); + + it('ignores a malformed byProtocol rather than throwing', () => { + withConfig( + { porch: { checks: { build: { command: './x.sh' } }, byProtocol: 'nonsense' } }, + root => { + expect(loadCheckOverrides(root, 'spir')).toEqual({ build: { command: './x.sh' } }); + }, + ); + }); + + it('returns the flat map unchanged when no protocol is named', () => { + // Every existing caller passed no protocol; none of them may change behaviour. + withConfig( + { + porch: { + checks: { build: { command: './render.sh' } }, + byProtocol: { bugfix: { checks: { regression_test: { skip: true } } } }, + }, + }, + root => { + expect(loadCheckOverrides(root)).toEqual({ build: { command: './render.sh' } }); + }, + ); + }); +}); diff --git a/packages/codev/src/commands/porch/config.ts b/packages/codev/src/commands/porch/config.ts index d5fb18ba5..70199ef78 100644 --- a/packages/codev/src/commands/porch/config.ts +++ b/packages/codev/src/commands/porch/config.ts @@ -6,6 +6,7 @@ import { loadConfig } from '../../lib/config.js'; import { resolveLaneComposition, type ConsultMode } from '../../lib/consult-lanes.js'; +import { canonicalProtocolName } from '../../lib/skeleton.js'; import type { CheckOverrides } from './types.js'; /** @@ -48,7 +49,10 @@ export function resolveConsultationModels( * Throws when config exists but cannot be parsed as JSON, * or when the legacy af-config.json is found. */ -export function loadCheckOverrides(workspaceRoot: string): CheckOverrides | null { +export function loadCheckOverrides( + workspaceRoot: string, + protocol?: string, +): CheckOverrides | null { const config = loadConfig(workspaceRoot); if (typeof config.porch !== 'object' || config.porch === null) { @@ -56,9 +60,56 @@ export function loadCheckOverrides(workspaceRoot: string): CheckOverrides | null } const checks = config.porch.checks; - if (typeof checks !== 'object' || checks === null || Array.isArray(checks)) { + const flat = (typeof checks === 'object' && checks !== null && !Array.isArray(checks)) + ? checks as CheckOverrides + : null; + + const perProtocol = protocol ? loadProtocolCheckOverrides(config, workspaceRoot, protocol) : null; + + if (!flat && !perProtocol) return null; + if (!perProtocol) return flat; + if (!flat) return perProtocol; + + // Field-level merge, per-protocol winning (#33). Wholesale replacement would + // mean a per-protocol `skip` silently discarded the global `command` for the + // same check, which is the opposite of what someone writing one line of + // protocol-specific config expects. + const merged: CheckOverrides = { ...flat }; + for (const [name, override] of Object.entries(perProtocol)) { + merged[name] = { ...(flat[name] ?? {}), ...override }; + } + return merged; +} + +/** + * Per-protocol check overrides from `porch.byProtocol..checks` (#33). + * + * `porch.checks` is one flat map applied to every protocol, and protocols do not + * declare the same check names: BUGFIX and AIR have `test`, SPIR does not. In a + * repo with no package.json, overriding `test` is REQUIRED or BUGFIX blocks at + * the fix phase running `npm test` — and then every `porch status` on a SPIR + * project warns that `test` is unknown. There was no way to satisfy both. + * + * Resolved by canonical protocol name, so `spir` and `spider` are one entry + * rather than two spellings that silently disagree. + */ +function loadProtocolCheckOverrides( + config: ReturnType, + workspaceRoot: string, + protocol: string, +): CheckOverrides | null { + const byProtocol = (config.porch as { byProtocol?: unknown } | undefined)?.byProtocol; + if (typeof byProtocol !== 'object' || byProtocol === null || Array.isArray(byProtocol)) { return null; } - return checks as CheckOverrides; + const wanted = canonicalProtocolName(workspaceRoot, protocol); + for (const [name, entry] of Object.entries(byProtocol as Record)) { + if (canonicalProtocolName(workspaceRoot, name) !== wanted) continue; + if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) continue; + const checks = (entry as { checks?: unknown }).checks; + if (typeof checks !== 'object' || checks === null || Array.isArray(checks)) continue; + return checks as CheckOverrides; + } + return null; } diff --git a/packages/codev/src/commands/porch/index.ts b/packages/codev/src/commands/porch/index.ts index 272de5b8b..4d8f67c91 100644 --- a/packages/codev/src/commands/porch/index.ts +++ b/packages/codev/src/commands/porch/index.ts @@ -381,8 +381,8 @@ export async function status( } // Show checks status (apply overrides so display matches what will actually run) - const statusOverrides = loadCheckOverrides(workspaceRoot); - const checks = getPhaseChecks(protocol, state.phase, statusOverrides ?? undefined); + const statusOverrides = loadCheckOverrides(workspaceRoot, state.protocol); + const checks = getPhaseChecks(protocol, state.phase, statusOverrides ?? undefined, workspaceRoot); if (Object.keys(checks).length > 0) { const checkLines = Object.keys(checks).map(name => ` ○ ${name} (not yet run)`); console.log(section('CRITERIA', checkLines.join('\n'))); @@ -421,10 +421,10 @@ export async function check(workspaceRoot: string, projectId: string, resolver?: const state = readState(statusPath); const protocol = loadProtocol(workspaceRoot, state.protocol); - const overrides = loadCheckOverrides(workspaceRoot); + const overrides = loadCheckOverrides(workspaceRoot, state.protocol); const phaseConfig = getPhaseConfig(protocol, state.phase); const phaseCheckNames = phaseConfig?.checks ?? []; - const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined); + const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined, workspaceRoot); if (Object.keys(checks).length === 0 && phaseCheckNames.length === 0) { console.log(chalk.dim('No checks defined for this phase.')); @@ -507,10 +507,10 @@ export async function done(workspaceRoot: string, projectId: string, resolver?: } const protocol = loadProtocol(workspaceRoot, state.protocol); - const overrides = loadCheckOverrides(workspaceRoot); + const overrides = loadCheckOverrides(workspaceRoot, state.protocol); const phaseConfig = getPhaseConfig(protocol, state.phase); const phaseCheckNames = phaseConfig?.checks ?? []; - const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined); + const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined, workspaceRoot); // Scope artifact reads + check cwd to the worktree that owns this status.yaml // (bugfix #676 — see check() for rationale). @@ -874,10 +874,10 @@ export async function approve( // Run phase checks before approving const protocol = loadProtocol(workspaceRoot, state.protocol); - const overrides = loadCheckOverrides(workspaceRoot); + const overrides = loadCheckOverrides(workspaceRoot, state.protocol); const phaseConfig = getPhaseConfig(protocol, state.phase); const phaseCheckNames = phaseConfig?.checks ?? []; - const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined); + const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined, workspaceRoot); if (phaseCheckNames.length > 0) { const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(artifactRoot, state.id, state.title, scopedResolver) }; diff --git a/packages/codev/src/commands/porch/next.ts b/packages/codev/src/commands/porch/next.ts index 0ca437d5a..945782396 100644 --- a/packages/codev/src/commands/porch/next.ts +++ b/packages/codev/src/commands/porch/next.ts @@ -456,7 +456,7 @@ async function handleBuildVerify( resolver?: ArtifactResolver, ): Promise { const verifyConfig = getVerifyConfig(protocol, state.phase); - const overrides = loadCheckOverrides(workspaceRoot); + const overrides = loadCheckOverrides(workspaceRoot, state.protocol); // Determine plan phase context for per_plan_phase protocols const planPhase = isPhased(protocol, state.phase) @@ -492,7 +492,7 @@ async function handleBuildVerify( } // Add check tasks (with overrides applied) - const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined); + const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined, workspaceRoot); // Also show skipped checks as informational tasks const phaseConfig_ = phaseConfig.checks ?? []; for (const name of phaseConfig_) { diff --git a/packages/codev/src/commands/porch/protocol.ts b/packages/codev/src/commands/porch/protocol.ts index 2a1d88e44..ff57faf27 100644 --- a/packages/codev/src/commands/porch/protocol.ts +++ b/packages/codev/src/commands/porch/protocol.ts @@ -8,7 +8,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { Protocol, ProtocolPhase, BuildConfig, VerifyConfig, OnCompleteConfig, CheckDef, CheckOverrides, ContextRefreshConfig } from './types.js'; -import { resolveCodevFile, getSkeletonDir } from '../../lib/skeleton.js'; +import { resolveCodevFile, getSkeletonDir, listAllCheckNames } from '../../lib/skeleton.js'; // ============================================================================ // Protocol Loading @@ -424,7 +424,8 @@ export function resolveCheckTimeoutMs( export function getPhaseChecks( protocol: Protocol, phaseId: string, - overrides?: CheckOverrides + overrides?: CheckOverrides, + workspaceRoot?: string, ): Record { const phase = getPhaseConfig(protocol, phaseId); if (!phase || !phase.checks) { @@ -439,11 +440,19 @@ export function getPhaseChecks( ...Object.keys(protocol.checks ?? {}), ...Object.keys(protocol.phase_completion ?? {}), ]); + // #33: a name this protocol does not declare is not necessarily WRONG. + // `porch.checks` is one flat map applied to every protocol, and protocols do + // not share check names: overriding `test` is required for BUGFIX and AIR in + // a repo with no package.json, and SPIR has no `test`, so a correct and + // necessary override warned on every `porch status`. Warn only for a name no + // protocol anywhere declares — that one really is a typo. + const knownAnywhere = workspaceRoot ? listAllCheckNames(workspaceRoot) : null; for (const name of Object.keys(overrides)) { if (phaseNames.has(name)) continue; // In this phase — normal case if (allProtocolChecks.has(name)) continue; // Valid elsewhere in protocol + if (knownAnywhere?.has(name)) continue; // Valid in a DIFFERENT protocol (#33) process.stderr.write( - `\x1b[33m ⚠ Unknown check override "${name}" (not found in protocol)\x1b[0m\n` + `\x1b[33m ⚠ Unknown check override "${name}" (not declared by any protocol)\x1b[0m\n` ); } } @@ -497,7 +506,8 @@ export function isPhased(protocol: Protocol, phaseId: string): boolean { */ export function getPhaseCompletionChecks( protocol: Protocol, - overrides?: CheckOverrides + overrides?: CheckOverrides, + workspaceRoot?: string, ): Record { const base = protocol.phase_completion ?? {}; if (!overrides) return base; @@ -507,10 +517,14 @@ export function getPhaseCompletionChecks( ...Object.keys(protocol.checks ?? {}), ...Object.keys(protocol.phase_completion ?? {}), ]); + // #33: same rule as getPhaseChecks — a name another protocol declares is + // applicable config, not a typo. + const knownAnywhere = workspaceRoot ? listAllCheckNames(workspaceRoot) : null; for (const name of Object.keys(overrides)) { if (allProtocolChecks.has(name)) continue; + if (knownAnywhere?.has(name)) continue; process.stderr.write( - `\x1b[33m ⚠ Unknown check override "${name}" (not found in protocol)\x1b[0m\n` + `\x1b[33m ⚠ Unknown check override "${name}" (not declared by any protocol)\x1b[0m\n` ); } diff --git a/packages/codev/src/lib/config.ts b/packages/codev/src/lib/config.ts index 0c95b7ba3..da2d0374f 100644 --- a/packages/codev/src/lib/config.ts +++ b/packages/codev/src/lib/config.ts @@ -47,7 +47,26 @@ export interface CodevConfig { }>; porch?: { autoOpenArtifacts?: boolean; + /** + * Check overrides applied to EVERY protocol. Per-protocol overrides live in + * `byProtocol..checks` and win field-by-field over these (#33). + */ checks?: Record; + /** + * Per-protocol porch settings (#33). + * + * `checks` above is one flat map applied to every protocol, and protocols do + * not declare the same check names: overriding `test` is required for BUGFIX + * and AIR in a repo with no package.json, and SPIR has no `test`. There was + * no way to satisfy both — dropping the override broke BUGFIX and AIR, + * keeping it warned on every SPIR `porch status`. + * + * Keyed by protocol name or alias; an alias and its canonical name are the + * same entry. + */ + byProtocol?: Record; + }>; /** * Which lanes run a consultation. Precedence, highest first: * byProtocol[P].modelsByType[T] > byProtocol[P].models > modelsByType[T] > models diff --git a/packages/codev/src/lib/skeleton.ts b/packages/codev/src/lib/skeleton.ts index 2491d72e0..8176fb37d 100644 --- a/packages/codev/src/lib/skeleton.ts +++ b/packages/codev/src/lib/skeleton.ts @@ -321,6 +321,63 @@ export function listConsultTypes(workspaceRoot?: string): Array<{ type: string; .sort((a, b) => a.type.localeCompare(b.type)); } +/** + * Every check name declared by ANY protocol visible at any tier (#33). + * + * `porch.checks` is one flat map applied to every protocol, and protocols do not + * declare the same check names. Overriding `test` is required for BUGFIX and AIR + * in a repo with no package.json; SPIR has no `test`, so every `porch status` on + * a SPIR project warned about a correct and necessary override. The name is not + * unknown — it is simply not used here, and those are different statements. + * + * Union across all four tiers, matching `listProtocolNames`. Includes + * `phase_completion` predicates, which `porch.checks` also overrides. + */ +export function listAllCheckNames(workspaceRoot?: string): Set { + const names = new Set(); + for (const dir of protocolDirs(workspaceRoot)) { + let protocols: fs.Dirent[]; + try { + protocols = fs.readdirSync(dir, { withFileTypes: true }).filter(d => d.isDirectory()); + } catch { + continue; // unreadable tier contributes nothing; it is not this function's error to raise + } + for (const proto of protocols) { + const json = readProtocolJson(path.join(dir, proto.name, 'protocol.json')); + if (!json) continue; + + const collect = (section: unknown): void => { + if (typeof section === 'object' && section !== null && !Array.isArray(section)) { + for (const name of Object.keys(section)) names.add(name); + } + }; + + // Two shapes. `loadProtocol` hoists per-phase check OBJECTS into a + // top-level map and rewrites `phase.checks` to a name list, so a protocol + // read raw from disk usually carries them under each phase instead — + // reading only the top-level `checks` found nothing at all. + collect(json.checks); + collect(json.phase_completion); + + const phases = json.phases; + if (Array.isArray(phases)) { + for (const phase of phases) { + if (typeof phase !== 'object' || phase === null) continue; + const phaseChecks = (phase as { checks?: unknown }).checks; + if (Array.isArray(phaseChecks)) { + for (const name of phaseChecks) { + if (typeof name === 'string') names.add(name); + } + } else { + collect(phaseChecks); + } + } + } + } + } + return names; +} + function readProtocolJson(filePath: string): Record | null { try { return JSON.parse(fs.readFileSync(filePath, 'utf-8')) as Record;