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;