From 24a3cd14960e902c76a9931c7bd6201afe214331 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:23:46 -0700 Subject: [PATCH 01/15] Run real-docker on every pull request and pin CI's Docker excludes Remove the Agent isolation workflow's path filter and run it on pushes to the same branches as CI, so `real-docker` can be a required check: a required check that a path filter skips never reports. Stop excluding test/agent-adapter.test.ts from the CI `test` job. It has never started Docker or a vendor CLI: since D4 (#47) its cases test pure parsers and budgets or refuse before anything is allocated, and it passes with no Docker binary on PATH. It was never on vitest.config.ts's Docker list. The excludes now equal that list, and a test fails if they differ or if real-docker gains a path filter. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/agent-isolation.yml | 39 ++------------------------- .github/workflows/ci.yml | 7 ++--- test/vitest-config.test.ts | 36 +++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 40 deletions(-) diff --git a/.github/workflows/agent-isolation.yml b/.github/workflows/agent-isolation.yml index f8d00633..838004a6 100644 --- a/.github/workflows/agent-isolation.yml +++ b/.github/workflows/agent-isolation.yml @@ -1,44 +1,9 @@ name: Agent isolation +# Runs on every pull request and on pushes to the same branches as CI, so `real-docker` can be a required check. on: push: - branches: [main] - paths: - - 'agents/**' - - 'git/clone.ts' - - 'test/agent-*.test.ts' - - 'runner/question-*.ts' - - 'runner/questions.ts' - - 'runner/planning*.ts' - - 'runner/workspace.ts' - - 'runner/runner-repository.ts' - - 'runner/execution.ts' - - 'core/run-audit.ts' - - 'runner/review.ts' - - 'runner/store.ts' - - 'git/history.ts' - - 'test/runner-workspace.test.ts' - - 'test/vitest-config.test.ts' - - 'vitest.config.ts' - - '.github/workflows/agent-isolation.yml' + branches: [main, 'codex/**'] pull_request: - paths: - - 'agents/**' - - 'git/clone.ts' - - 'test/agent-*.test.ts' - - 'runner/question-*.ts' - - 'runner/questions.ts' - - 'runner/planning*.ts' - - 'runner/workspace.ts' - - 'runner/runner-repository.ts' - - 'runner/execution.ts' - - 'core/run-audit.ts' - - 'runner/review.ts' - - 'runner/store.ts' - - 'git/history.ts' - - 'test/runner-workspace.test.ts' - - 'test/vitest-config.test.ts' - - 'vitest.config.ts' - - '.github/workflows/agent-isolation.yml' permissions: contents: read jobs: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a7709445..2c382283 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,8 +16,9 @@ jobs: cache: npm - run: npm ci --ignore-scripts - run: npm run typecheck - # The Docker agent suites run one file at a time in the Agent isolation workflow; running them here - # would put them in parallel against the same image tag and daemon. - - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-network.test.ts --exclude test/agent-adapter.test.ts --exclude test/agent-supervisor.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-question.test.ts --exclude test/agent-planning.test.ts --exclude test/runner-workspace.test.ts + # The Docker-backed files run one at a time in the Agent isolation workflow; running them here would put them in + # parallel against the same image tag and daemon. Exclude exactly the dockerTests list in vitest.config.ts; + # test/vitest-config.test.ts fails if the two differ. + - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-network.test.ts --exclude test/agent-planning.test.ts --exclude test/agent-question.test.ts --exclude test/agent-supervisor.test.ts --exclude test/runner-workspace.test.ts - run: npx playwright install --with-deps chromium - run: npm run test:browser diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index d1851b4e..64abdfad 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -1,6 +1,12 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; +import { parse } from 'yaml'; import config from '../vitest.config.ts'; +const ROOT = fileURLToPath(new URL('..', import.meta.url)); + const DOCKER_TESTS = [ 'test/agent-container.test.ts', 'test/agent-gate.test.ts', @@ -29,3 +35,33 @@ describe('Vitest scheduling', () => { }); }); }); + +const workflow = (name: string) => + parse(readFileSync(join(ROOT, '.github/workflows', name), 'utf8')) as { + on: Record; + jobs: Record }> }>; + }; +const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); + +describe('CI workflows', () => { + it('excludes exactly the Docker-backed files from the CI test job', () => { + const ci = workflow('ci.yml'); + const commands = runs(ci.jobs.test!.steps).filter(run => run.startsWith('npm test')); + expect(commands).toHaveLength(1); + const words = commands[0]!.split(/\s+/); + const excluded = words.flatMap((word, index) => word === '--exclude' ? [words[index + 1]] : []); + expect(excluded.toSorted()).toEqual(DOCKER_TESTS); + expect(words.filter(word => word.startsWith('-')).every(word => word === '--' || word === '--exclude')).toBe(true); + }); + + it('runs the complete Vitest suite in real-docker on every pull request and on the CI push branches', () => { + const ci = workflow('ci.yml'), isolation = workflow('agent-isolation.yml'); + expect(Object.keys(isolation.on).toSorted()).toEqual(['pull_request', 'push']); + // No path filter: a required check that a filter skips never reports, and the pull request cannot merge. + expect(isolation.on.pull_request).toBeNull(); + expect(ci.on.pull_request).toBeNull(); + expect(isolation.on.push).toEqual(ci.on.push); + expect(isolation.on.push).toEqual({ branches: ['main', 'codex/**'] }); + expect(runs(isolation.jobs['real-docker']!.steps)).toEqual(['npm ci --ignore-scripts', 'npm run typecheck', 'npm test']); + }); +}); From 315af94306b64e51d02a492d51b744d9f43fe61e Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:24:05 -0700 Subject: [PATCH 02/15] Add a nightly and manual Live probes workflow for the Claude startup probes The workflow runs on workflow_dispatch and a nightly schedule, never on a pull request or push, so it is not a required check. It reads the repository secret CLAUDE_CODE_OAUTH_TOKEN and fails at once when the secret is empty. It runs only the four live Claude probes, selected by a shared "(live Claude)" title suffix, through the docker Vitest project so the files run one at a time. Vitest exits 0 when it skips every test, so scripts/live-probes.ts then checks the JSON report: every listed probe must pass and no other test may run. Tests pin the triggers, the command, the secret guard, the probe list against the test files, and the report check. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/live-probes.yml | 43 +++++++++++++++++ scripts/live-probes.ts | 67 +++++++++++++++++++++++++++ test/agent-container.test.ts | 2 +- test/agent-supervisor.test.ts | 4 +- test/vitest-config.test.ts | 77 ++++++++++++++++++++++++++++++- 5 files changed, 189 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/live-probes.yml create mode 100644 scripts/live-probes.ts diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml new file mode 100644 index 00000000..9a740a30 --- /dev/null +++ b/.github/workflows/live-probes.yml @@ -0,0 +1,43 @@ +name: Live probes +# Runs the live Claude startup probes against the real Claude service. It is not a pull request check: it runs +# nightly on the default branch and when started by hand. See docs/implementation/ci.md. +on: + workflow_dispatch: + schedule: + - cron: '23 6 * * *' +permissions: + contents: read +concurrency: + group: live-probes + cancel-in-progress: false +jobs: + live-probes: + runs-on: ubuntu-latest + # Covers the 10 min image build, up to 11 min per probe and teardown. + timeout-minutes: 60 + steps: + # Fail at once when the secret is missing, so a missing secret never looks like a passing run. + - name: Require the Claude token + env: + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + run: | + if [ -z "$CLAUDE_CODE_OAUTH_TOKEN" ]; then + echo "::error::The repository secret CLAUDE_CODE_OAUTH_TOKEN is not set. Add it to run the live probes." + exit 1 + fi + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: '26.7.0' + cache: npm + - run: npm ci --ignore-scripts + # The docker project runs these files one at a time, as in the Agent isolation workflow. The filter keeps only + # the tests whose titles end in "(live Claude)". + - name: Run the live Claude probes + env: + CODEBOOST_RUN_AUTH_PROBES: '1' + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + run: npx vitest run --project docker -t '\(live Claude\)$' --reporter=default --reporter=json --outputFile=live-probes.json test/agent-container.test.ts test/agent-question.test.ts test/agent-supervisor.test.ts + # Vitest exits 0 when it skips every probe, so check that each probe passed and nothing else ran. + - name: Check that every probe passed + run: node scripts/live-probes.ts check live-probes.json diff --git a/scripts/live-probes.ts b/scripts/live-probes.ts new file mode 100644 index 00000000..9454c340 --- /dev/null +++ b/scripts/live-probes.ts @@ -0,0 +1,67 @@ +// The live Claude startup probes, and the check that the Live probes workflow ran all of them. +// +// node scripts/live-probes.ts check +// +// The probes call the real Claude service, so they run only when CODEBOOST_RUN_AUTH_PROBES=1 and +// CLAUDE_CODE_OAUTH_TOKEN are set. Without those, Vitest skips them and still exits 0. This check reads the +// Vitest JSON report and exits 1 unless every probe below passed and nothing else ran. A renamed, removed or +// skipped probe therefore fails the workflow instead of passing it silently. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { pathToFileURL } from 'node:url'; + +// Every probe title ends with this suffix. The workflow selects the probes with `-t` and this text. +export const LIVE_PROBE_MARKER = '(live Claude)'; + +export const LIVE_PROBES = [ + { file: 'test/agent-container.test.ts', + name: 'real Docker agent isolation runs the authenticated Claude startup path with only its OAuth token (live Claude)' }, + { file: 'test/agent-question.test.ts', + name: 'Ask in the agent container answers from a file it can only read in /work (live Claude)' }, + { file: 'test/agent-supervisor.test.ts', + name: 'container invocation supervisor runs the production Claude adapter and parses its bounded envelope (live Claude)' }, + { file: 'test/agent-supervisor.test.ts', + name: 'container invocation supervisor returns a schema-constrained Claude planning answer as bare JSON from the ' + + 'mounted schema (live Claude)' }, +] as const; + +export const LIVE_PROBE_FILES = [...new Set(LIVE_PROBES.map(probe => probe.file))]; + +type Report = { + success?: unknown; numFailedTests?: unknown; + testResults?: Array<{ name?: unknown; assertionResults?: Array<{ fullName?: unknown; status?: unknown }> }>; +}; + +// Returns one line per problem; an empty list means every probe passed and nothing else ran. +export function checkLiveProbeReport(report: Report, root = process.cwd()): string[] { + const problems: string[] = []; + if (report.success !== true) problems.push('Vitest did not report success.'); + if (report.numFailedTests !== 0) problems.push(`Vitest reported ${String(report.numFailedTests)} failed test(s).`); + const passed = new Set(); + for (const file of report.testResults ?? []) { + const name = typeof file.name === 'string' ? file.name : ''; + for (const test of file.assertionResults ?? []) { + if (test.status !== 'passed') continue; + const key = `${name} > ${String(test.fullName)}`; + passed.add(key); + if (!LIVE_PROBES.some(probe => `${resolve(root, probe.file)} > ${probe.name}` === key)) + problems.push(`A test that is not a live probe ran: ${key}`); + } + } + for (const probe of LIVE_PROBES) + if (!passed.has(`${resolve(root, probe.file)} > ${probe.name}`)) + problems.push(`Live probe did not pass: ${probe.file} > ${probe.name}`); + return problems; +} + +if (process.argv[1] && import.meta.url === pathToFileURL(resolve(process.argv[1])).href) { + const [command, path] = process.argv.slice(2); + if (command !== 'check' || !path) { + console.error('Usage: node scripts/live-probes.ts check '); + process.exit(2); + } + const problems = checkLiveProbeReport(JSON.parse(readFileSync(path, 'utf8')) as Report); + for (const problem of problems) console.error(problem); + if (problems.length) process.exit(1); + console.log(`All ${LIVE_PROBES.length} live Claude probes passed.`); +} diff --git a/test/agent-container.test.ts b/test/agent-container.test.ts index 48d809ed..7b6ef613 100644 --- a/test/agent-container.test.ts +++ b/test/agent-container.test.ts @@ -2590,7 +2590,7 @@ describe('real Docker agent isolation', () => { }, 60_000); if (process.env.CODEBOOST_RUN_AUTH_PROBES === '1') { - it('runs the authenticated Claude startup path with only its OAuth token', async () => { + it('runs the authenticated Claude startup path with only its OAuth token (live Claude)', async () => { const data = fixture(), token = process.env.CLAUDE_CODE_OAUTH_TOKEN; if (!token) throw new Error('CLAUDE_CODE_OAUTH_TOKEN is required.'); // Questions answer in plain text; a planning command would also carry the schema as --json-schema. diff --git a/test/agent-supervisor.test.ts b/test/agent-supervisor.test.ts index 7c5cc5c4..7d1bf7b4 100644 --- a/test/agent-supervisor.test.ts +++ b/test/agent-supervisor.test.ts @@ -641,7 +641,7 @@ describe('container invocation supervisor', () => { // formatting fails the probe. const schemaValue = (output: string) => output.replace(/\r?\n$/, ''); - it('runs the production Claude adapter and parses its bounded envelope', async () => { + it('runs the production Claude adapter and parses its bounded envelope (live Claude)', async () => { const data = fixture(), token = process.env.CLAUDE_CODE_OAUTH_TOKEN; if (!token) throw new Error('CLAUDE_CODE_OAUTH_TOKEN is required.'); // Questions answer in plain text; planning answers are schema-constrained (the next probe). @@ -655,7 +655,7 @@ describe('container invocation supervisor', () => { expect(schemaValue(result.stdout)).toBe('codeboost-adapter-schema-marker'); }, 8 * 60_000); - it('returns a schema-constrained Claude planning answer as bare JSON from the mounted schema', async () => { + it('returns a schema-constrained Claude planning answer as bare JSON from the mounted schema (live Claude)', async () => { // `const` makes the schema itself enforce the marker. Claude can also read the mounted file, so the marker alone // does not prove the flag: the strict structured_output decode does, since without --json-schema there is none. const schema = JSON.stringify({ type: 'object', additionalProperties: false, required: ['marker'], diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 64abdfad..1b9cee3e 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -1,8 +1,10 @@ -import { readFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { readdirSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import { parse } from 'yaml'; +import { checkLiveProbeReport, LIVE_PROBE_FILES, LIVE_PROBES } from '../scripts/live-probes.ts'; import config from '../vitest.config.ts'; const ROOT = fileURLToPath(new URL('..', import.meta.url)); @@ -64,4 +66,77 @@ describe('CI workflows', () => { expect(isolation.on.push).toEqual({ branches: ['main', 'codex/**'] }); expect(runs(isolation.jobs['real-docker']!.steps)).toEqual(['npm ci --ignore-scripts', 'npm run typecheck', 'npm test']); }); + + it('runs only the live Claude probes, by hand or nightly, never on a pull request or push', () => { + const live = workflow('live-probes.yml'); + expect(Object.keys(live.on).toSorted()).toEqual(['schedule', 'workflow_dispatch']); + const job = live.jobs['live-probes']!; + expect(job.env).toBeUndefined(); + const vitest = job.steps.filter(step => step.run?.startsWith('npx vitest')); + expect(vitest).toHaveLength(1); + const words = vitest[0]!.run!.split(/\s+/); + // The docker project runs the files one at a time; the filter keeps only titles that end in "(live Claude)". + expect(vitest[0]!.run!.startsWith("npx vitest run --project docker -t '\\(live Claude\\)$' ")).toBe(true); + expect(words.filter(word => word.startsWith('test/'))).toEqual(LIVE_PROBE_FILES); + expect(vitest[0]!.env).toEqual({ CODEBOOST_RUN_AUTH_PROBES: '1', + CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }); + expect(runs(job.steps).at(-1)).toBe('node scripts/live-probes.ts check live-probes.json'); + for (const file of LIVE_PROBE_FILES) expect(DOCKER_TESTS).toContain(file); + }); + + it('fails the live probe job at once when the token secret is empty', () => { + const guard = workflow('live-probes.yml').jobs['live-probes']!.steps[0]!; + expect(guard.env).toEqual({ CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }); + const run = (token: string | undefined) => spawnSync('bash', ['-e', '-c', guard.run!], { encoding: 'utf8', + env: { PATH: process.env.PATH, ...(token === undefined ? {} : { CLAUDE_CODE_OAUTH_TOKEN: token }) } }); + for (const token of [undefined, '']) { + const result = run(token); + expect(result.status).toBe(1); + expect(result.stdout).toContain('::error::The repository secret CLAUDE_CODE_OAUTH_TOKEN is not set'); + } + expect(run('placeholder').status).toBe(0); + }); + + it('names every live probe exactly as its test file titles it', () => { + // Every "(live Claude)" title in the probe files is listed, and no other test file is gated on live credentials. + for (const file of LIVE_PROBE_FILES) { + const source = readFileSync(join(ROOT, file), 'utf8'); + const titles = [...source.matchAll(/'([^']*\(live Claude\))'/g)].map(match => match[1]); + expect(titles.length).toBe(LIVE_PROBES.filter(probe => probe.file === file).length); + for (const title of titles) + expect(LIVE_PROBES.some(probe => probe.file === file && probe.name.endsWith(` ${title}`))).toBe(true); + } + const gated = readdirSync(join(ROOT, 'test')).filter(name => name.endsWith('.test.ts') && name !== 'vitest-config.test.ts') + .filter(name => readFileSync(join(ROOT, 'test', name), 'utf8').includes('CODEBOOST_RUN_AUTH_PROBES')) + .map(name => `test/${name}`); + expect(gated).toEqual(LIVE_PROBE_FILES); + }); +}); + +describe('live probe report check', () => { + const report = (tests: Array<[string, string, string]>, failed = 0) => ({ + success: failed === 0, numFailedTests: failed, + testResults: [...new Set(tests.map(([file]) => file))].map(file => ({ name: join(ROOT, file), + assertionResults: tests.filter(([testFile]) => testFile === file).map(([, fullName, status]) => ({ fullName, status })) })), + }); + const all = LIVE_PROBES.map(probe => [probe.file, probe.name, 'passed'] as [string, string, string]); + + it('accepts a report in which every probe passed and nothing else ran', () => { + expect(checkLiveProbeReport(report([...all, [LIVE_PROBES[0].file, 'other test', 'skipped']]), ROOT)).toEqual([]); + }); + + it('refuses a report in which the probes were skipped, as when the probe switch is off', () => { + const skipped = all.map(([file, name]) => [file, name, 'skipped'] as [string, string, string]); + expect(checkLiveProbeReport(report(skipped), ROOT)).toHaveLength(LIVE_PROBES.length); + expect(checkLiveProbeReport({}, ROOT).length).toBeGreaterThan(LIVE_PROBES.length); + }); + + it('refuses a missing probe, a failure or a test that is not a probe', () => { + expect(checkLiveProbeReport(report(all.slice(1)), ROOT)).toEqual( + [`Live probe did not pass: ${LIVE_PROBES[0].file} > ${LIVE_PROBES[0].name}`]); + expect(checkLiveProbeReport(report(all, 1), ROOT)).toEqual( + ['Vitest did not report success.', 'Vitest reported 1 failed test(s).']); + expect(checkLiveProbeReport(report([...all, [LIVE_PROBES[0].file, 'other test', 'passed']]), ROOT)).toEqual( + [`A test that is not a live probe ran: ${join(ROOT, LIVE_PROBES[0].file)} > other test`]); + }); }); From 5c6fba1133815d2ae3cf178919aadd6a6726c04f Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:36:13 -0700 Subject: [PATCH 03/15] Document the CI workflows, required checks and what remains for T9 Add docs/implementation/ci.md: which workflow and job runs which tests on which triggers, the required checks and secret a repository administrator must set, the live probe job, and how to record the planning fixtures again. Link it from the README, agent isolation and planning acceptance, and update the T9 status in the plan. T9 stays unticked. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- docs/designs/codeboost-plan-indexed-review.md | 4 +- docs/implementation/agent-isolation.md | 29 ++-- docs/implementation/ci.md | 130 ++++++++++++++++++ docs/implementation/planning-acceptance.md | 10 +- 5 files changed, 160 insertions(+), 15 deletions(-) create mode 100644 docs/implementation/ci.md diff --git a/README.md b/README.md index 8aa4776f..f83204f2 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ npm run typecheck npm test ``` -Tests create disposable local repositories and exercise command checks in disposable credential-free containers. They do not invoke model providers or access GitHub. +Tests create disposable local repositories and exercise command checks in disposable credential-free containers. They do not invoke model providers or access GitHub. [CI](docs/implementation/ci.md) lists the workflows, the required checks and the live Claude probes. ## Guarded GitHub merge diff --git a/docs/designs/codeboost-plan-indexed-review.md b/docs/designs/codeboost-plan-indexed-review.md index f7d283bc..1ce0aa2d 100644 --- a/docs/designs/codeboost-plan-indexed-review.md +++ b/docs/designs/codeboost-plan-indexed-review.md @@ -1922,7 +1922,7 @@ Read each row left to right: finish and validate step 1 before step 2 within tha | C — guarded merge gate | **C1.** Required-check and branch-rule reads (T12). **C2.** Snapshot/evidence blockers, with unavailable T6 execution evidence blocking merge. **C3.** Head-pinned, base-protected merge and refusal handling (T7). **C4.** Review UI, race regressions and final #21 / PR #23 review. | Before C1, record B0 evidence for the foundation contracts C consumes; existing work must supply that evidence before C4 completion. Continue existing work rather than restarting implemented steps. Release shared runner/UI files after C4 merges. | | D — agent isolation | **D1.** Invocation contract and isolated task clone (T1). **D2.** Pinned, restricted container and startup self-test (T1). **D3.** Vendor-only egress and phase/tool enforcement (T2). **D4.** Claude/Codex adapters, cancellation settlement and bounded output. **D5.** Full real-Docker and hostile-input gate for this boundary (T9). | Can run alongside C and E. F requires D5 merged; G's production invocation requires D5. Add regressions with each step; D5 integrates them rather than postponing testing. | | E — planning logic | **E1.** Audit existing T18 schema/parser/prompt behavior and remaining #6 gaps. **E2.** Read-only authoring-provider contract and safe prompt/response handling. **E3.** Identity/revision-bound suggestion orchestration using the existing store interface. **E4.** Import, replay, malformed-response and hostile-input acceptance fixtures (T18). | Can run alongside C and D with injected providers. G consumes E4; live invocation waits for D5. Shared schema/store fixes must go through the assigned integration owner. | -| F — runner and pre-merge automation | **F1.** Before implementation, publish and review the lifecycle/state-holder contract: pending, running, completed, failed, cancelled, stale and closing; legal transitions; ownership and settlement for persisted records, in-memory jobs, subprocesses, admitted HTTP requests and rendered UI; guarded retry; reject-admission → drain requests → cancel/await jobs → close storage. Then implement it and the feedback-event contract under the AGENTS.md async rules. **F2.** Per-item execution, review/reject rounds, pre-PR already-fixed checks, PR opening and hostile-issue eval (build step 5; T9). **F3.** Trusted rebase and ledger mapping (remaining T3). **F4.** Foreign-commit conflict handling (T11). **F5.** Post-rebase attribution/approval refresh and head-bound command execution (T6). **F6.** Required-check refresh, already-fixed check, guarded merge handoff and #22 integration regressions. F owns common CI after C: integrate every T9 suite (Docker, adapter, hostile-input/issue, recorded-output, unit and browser) into required CI, coordinating D's dedicated workflow. T9 remains incomplete until the combined head demonstrably runs and passes every suite. | Starts after C4 and D5 merge and B0 evidence is handed off for F's consumed contracts. Recheck that evidence against merged main before F1; existing T4/T5/T10 behavior is reused rather than rebuilt. F1 owns planning persistence/API additions needed by G. F2's working reject loop supplies the learning dependency. Ask already uses D's invocation contract (`runner/question-container.ts`), so F reuses that path rather than adding a second one. | +| F — runner and pre-merge automation | **F1.** Before implementation, publish and review the lifecycle/state-holder contract: pending, running, completed, failed, cancelled, stale and closing; legal transitions; ownership and settlement for persisted records, in-memory jobs, subprocesses, admitted HTTP requests and rendered UI; guarded retry; reject-admission → drain requests → cancel/await jobs → close storage. Then implement it and the feedback-event contract under the AGENTS.md async rules. **F2.** Per-item execution, review/reject rounds, pre-PR already-fixed checks, PR opening and hostile-issue eval (build step 5; T9). **F3.** Trusted rebase and ledger mapping (remaining T3). **F4.** Foreign-commit conflict handling (T11). **F5.** Post-rebase attribution/approval refresh and head-bound command execution (T6). **F6.** Required-check refresh, already-fixed check, guarded merge handoff and #22 integration regressions. F owns common CI after C: integrate every T9 suite (Docker, adapter, hostile-input/issue, recorded-output, unit and browser) into required CI, coordinating D's dedicated workflow. T9 remains incomplete until the combined head demonstrably runs and passes every suite. *F6c status:* the workflows are in place (`docs/implementation/ci.md`); required-check settings, a passing run of both required jobs on one head and a green `Live probes` run remain. | Starts after C4 and D5 merge and B0 evidence is handed off for F's consumed contracts. Recheck that evidence against merged main before F1; existing T4/T5/T10 behavior is reused rather than rebuilt. F1 owns planning persistence/API additions needed by G. F2's working reject loop supplies the learning dependency. Ask already uses D's invocation contract (`runner/question-container.ts`), so F reuses that path rather than adding a second one. | | G — planning screen | **G1.** Import and plan display UI. **G2.** Authoring and suggestion cards. **G3.** Revision-bound Apply and draft/attachment preservation. **G4.** Real provider/store integration and complete T18 browser/adapter acceptance. | G1 starts after E4 and C4 merge; G1–G3 may use fixtures. G4 waits for D5 and F1's production planning API/persistence contract. Release shared web files after G4. | | H — issue prioritization | **H1.** Decide and record ranking policy. **H2.** Issue retrieval/normalization. **H3.** Deterministic ranking with reasons and failure/stale states. **H4.** Issue-list UI and end-to-end checks (build step 8). | H1–H3 can run alongside F/G after the issue-access contract is inspected. H4 waits for G4 to release shared web files. No existing T-ID covers this entire milestone. | | I — queue, schedule and recovery | **I1.** Queue admission and persisted transitions. **I2.** Run-window scheduling and cancellation. **I3.** Restart recovery, stale attempts and shutdown draining. **I4.** UI integration and controlled race acceptance (build step 7). | Starts after F6; owns shared runner/store files. UI work waits for G/H to release its exact files. No existing T-ID covers this entire milestone. | @@ -1981,7 +1981,7 @@ F, G and H ran together within these ownership boundaries. G and H are done; for Built from this review's findings. Each task comes from a specific decision above. Run with Claude Code or Codex, and tick each one as you ship it. Effort ratios assumed: features about 30x, tests about 50x, architecture about 5x. -**Status (checked against `main` on 2026-09-26, lane B0).** T1, T2, T4, T5, T10, T13 and T14 meet their Verify lines and are ticked, with evidence under each (T1, T2 and T14 re-checked 2026-09-26 after lane D merged). T9 stays open until lane F6 runs every suite in required CI. Ask calls the `agents/` boundary from `runner/question-container.ts`; it has no host adapter. The planned files `core/segments`, `core/choices` and `core/attribution` were never created. That logic lives in `core/linking.ts` (segments and ledger attribution) and `core/approvals.ts` (approvals and duplicate-segment choices). The Files lines below now name the real files. +**Status (checked against `main` on 2026-09-26, lane B0).** T1, T2, T4, T5, T10, T13 and T14 meet their Verify lines and are ticked, with evidence under each (T1, T2 and T14 re-checked 2026-09-26 after lane D merged). T9 stays open. Lane F6 (common CI) has added the workflows in `docs/implementation/ci.md`: `test` and `real-docker` run on every pull request with no path filter, and `Live probes` runs the live Claude probes nightly and by hand. T9 can be ticked when a repository administrator requires `test` and `real-docker` on `main`, both pass on the same head, and a `Live probes` run passes with the `CLAUDE_CODE_OAUTH_TOKEN` secret. Ask calls the `agents/` boundary from `runner/question-container.ts`; it has no host adapter. The planned files `core/segments`, `core/choices` and `core/attribution` were never created. That logic lives in `core/linking.ts` (segments and ledger attribution) and `core/approvals.ts` (approvals and duplicate-segment choices). The Files lines below now name the real files. These `T` IDs are requirement identifiers, not the build-order numbers. Current merge-gate work is **build step 4, increment 1 (#21)** and spans parts of T6, T7, and T12; it is unrelated to the numbering of T4. See “Build step 4: scope and progress” for the current increment and remaining milestone criteria. An increment must not mark a broader requirement complete while any of its acceptance criteria remain deferred. diff --git a/docs/implementation/agent-isolation.md b/docs/implementation/agent-isolation.md index c3858887..3760e1af 100644 --- a/docs/implementation/agent-isolation.md +++ b/docs/implementation/agent-isolation.md @@ -15,13 +15,16 @@ npm test `vitest.config.ts` finishes the ordinary test project first, then runs the Docker-backed files one at a time because they share one image tag and daemon. Use `npx vitest run --project docker` to rerun only that serial project. The `Agent isolation` -workflow runs the complete gate. The main `CI` workflow skips the Docker project so that -it can cover the ordinary project independently. +workflow's `real-docker` job runs the complete gate on every pull request. The main `CI` +workflow's `test` job skips the Docker-backed files so that it can cover the ordinary +project independently. [CI](ci.md) describes both jobs. -The live vendor probes need real credentials, so CI does not run them. To run them, -set `CODEBOOST_RUN_AUTH_PROBES=1` and `CLAUDE_CODE_OAUTH_TOKEN`. Codex has no live -probe while it is refused (#93). Do not put credentials in an issue, a pull request -or chat. +The live vendor probes need real credentials, so pull requests do not run them. The +`Live probes` workflow runs them every night and when started by hand, with the +repository secret `CLAUDE_CODE_OAUTH_TOKEN` ([CI](ci.md#live-probes)). To run them +locally, set `CODEBOOST_RUN_AUTH_PROBES=1` and `CLAUDE_CODE_OAUTH_TOKEN`. Codex has no +live probe while it is refused (#93). Do not put credentials in an issue, a pull +request or chat. ## What the gate proves @@ -407,10 +410,16 @@ against real Docker, including two reviews with different Ask owners on one daem ## Limits of this gate -- CI does not run the live vendor probes. Run them locally with credentials before - a release that changes the image, the adapters or the prompts. Codex has no live - probe while it is refused in every phase (#93). -- T9 as a whole is complete only when lane F runs every suite in required CI (F6). +- Pull requests do not run the live vendor probes. The `Live probes` workflow runs them + every night and by hand, and fails if the `CLAUDE_CODE_OAUTH_TOKEN` secret is missing. + A pull request can therefore merge before a probe shows that it broke the Claude + startup path. Start the workflow by hand, or run the probes locally, before a release + that changes the image, the adapters or the prompts. Codex has no live probe while it + is refused in every phase (#93). +- T9 as a whole is not complete yet. Lane F6 has added the workflows ([CI](ci.md)). What + remains: a repository administrator must require `test` and `real-docker` on `main`; + both jobs must pass on the same head after that; and a `Live probes` run must pass + once the secret exists. - Gitlink mounts and the pre-launch check apply to execute and fix only. In planning, questions and review, `/work` is a read-only volume, so nothing can be written beneath a gitlink there; that includes `check` attempts, which run as review. Running an item's `cmd` in a writable task filesystem is not built yet; when it is, each run must call diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md new file mode 100644 index 00000000..eb84de1b --- /dev/null +++ b/docs/implementation/ci.md @@ -0,0 +1,130 @@ +# Continuous integration + +This page describes which GitHub Actions workflows run which tests, and when. It +also lists the settings that only a repository administrator can make. These +settings are not stored in the repository. + +## Workflows + +| Workflow file | Job (check name) | Runs on | What it runs | +| --- | --- | --- | --- | +| `.github/workflows/ci.yml` | `test` | every pull request; pushes to `main` and `codex/**` | Type check; every Vitest file except the Docker-backed files; the Playwright browser suite | +| `.github/workflows/agent-isolation.yml` | `real-docker` | every pull request; pushes to `main` and `codex/**` | Type check; the complete Vitest suite against real Docker | +| `.github/workflows/live-probes.yml` | `live-probes` | started by hand (`workflow_dispatch`); every night at 06:23 UTC | The four live Claude startup probes only | + +`test` and `real-docker` have no path filters. They run on every pull request, +so both can be required checks. A required check that a path filter skips +never reports, and GitHub then blocks the merge. + +### `test` + +`test` runs the Vitest files that do not need Docker, and then the browser +suite (`npm run test:browser`). It excludes the Docker-backed files with +`--exclude`. The Docker-backed files are the `dockerTests` list in +`vitest.config.ts`. The excludes must equal that list: +`test/vitest-config.test.ts` fails if they differ. + +When a test file starts to use Docker, add it to `dockerTests` in +`vitest.config.ts`, to `DOCKER_TESTS` in `test/vitest-config.test.ts` and to the +excludes in `ci.yml`. + +`test/agent-adapter.test.ts` runs in `test`. It does not start Docker or a +vendor CLI: its cases test parsers and budgets, or refuse a request before +anything is allocated. It passes with no Docker binary on `PATH`. + +### `real-docker` + +`real-docker` runs `npm test` with no excludes. `vitest.config.ts` runs the +ordinary files first. Then it runs the Docker-backed files one at a time, +because they share one image tag and one Docker daemon. This job is the T9 +Docker gate. [Agent isolation](agent-isolation.md#what-the-gate-proves) lists +what it proves. + +### Recorded outputs and the hostile-issue eval + +These files are ordinary Vitest files, so both `test` and `real-docker` run +them. They do not call a vendor or GitHub. + +- Recorded Claude planning output: `test/fixtures/planning/recorded/`, replayed + by `test/planning-recorded.test.ts`. +- GitHub (`gh`) responses: fixtures inside `test/merge.test.ts` and the other + merge and publishing tests. +- Hostile issue texts: `test/fixtures/hostile-issues.json`, used by + `test/execution-prompt.test.ts`. + +### `live-probes` + +The live probes call the real Claude service. They are not a pull request +check, and they do not run on a pull request or a push. GitHub runs scheduled +workflows only from the default branch, so the nightly run starts after this +workflow is on `main`. + +The job: + +1. Fails at once if the repository secret `CLAUDE_CODE_OAUTH_TOKEN` is empty or + missing. A missing secret never gives a passing run. +2. Runs the Docker Vitest project with `CODEBOOST_RUN_AUTH_PROBES=1`. It selects + only the tests whose titles end in `(live Claude)`, in + `test/agent-container.test.ts`, `test/agent-question.test.ts` and + `test/agent-supervisor.test.ts`. The files run one at a time. +3. Runs `node scripts/live-probes.ts check` on the Vitest JSON report. Vitest + exits with success when it skips every test, so this step fails unless each + probe listed in `scripts/live-probes.ts` passed and no other test ran. + +If you add, rename or remove a probe, update `LIVE_PROBES` in +`scripts/live-probes.ts`. `test/vitest-config.test.ts` fails if the list and the +test titles differ. + +Codex has no probe, because Codex is refused in every phase (#93). + +## Settings for a repository administrator + +The repository cannot make these settings itself. + +1. **Required checks on `main`.** In the branch protection rule or ruleset for + `main`, require the status checks `test` and `real-docker`. Do not require + `live-probes`. +2. **Live probe secret.** Add the repository secret `CLAUDE_CODE_OAUTH_TOKEN` + (Settings, then Secrets and variables, then Actions). Make the token with + `claude setup-token`. Until the secret exists, every `live-probes` run fails + at its first step. + +Never put a token in an issue, a pull request, a commit or chat. + +## Record the planning fixtures again + +`test/planning-recorded.test.ts` fails when a recording no longer matches +today's request: for example, when the prompt, the schema, the Claude command, +the image tag or the CLI version changes. To record again, run this on a +machine with Docker: + +```sh +CODEBOOST_RUN_AUTH_PROBES=1 CLAUDE_CODE_OAUTH_TOKEN=... node scripts/record-planning.ts +``` + +Read each file in `test/fixtures/planning/recorded/` before you commit it. No +workflow records fixtures. [Planning acceptance](planning-acceptance.md#recorded-vendor-output) +describes the recordings. + +## Run the same checks locally + +```sh +npm run typecheck +npm test # all Vitest files; needs Docker +npx vitest run --project parallel # only the files that do not need Docker +npx vitest run --project docker # only the Docker-backed files +npm run test:browser +``` + +To run the live probes locally, set `CODEBOOST_RUN_AUTH_PROBES=1` and +`CLAUDE_CODE_OAUTH_TOKEN`, then run the `vitest` command from +`live-probes.yml`. + +## What remains before T9 is complete + +T9 in the [plan](../designs/codeboost-plan-indexed-review.md) stays open until +all of these are true: + +1. A repository administrator requires `test` and `real-docker` on `main`. +2. Both required jobs pass on the same head after this setup is merged. +3. The `CLAUDE_CODE_OAUTH_TOKEN` secret exists and a `live-probes` run passes. diff --git a/docs/implementation/planning-acceptance.md b/docs/implementation/planning-acceptance.md index 3f23eb24..e177b4f1 100644 --- a/docs/implementation/planning-acceptance.md +++ b/docs/implementation/planning-acceptance.md @@ -86,5 +86,11 @@ The Store guarantees revision/snapshot binding and pending-only settlement acros processes, and E3 requires the caller's snapshot identity before admission. G4 now composes the browser with production planning setup, lane D's provider boundary, F's HTTP lifecycle and Store persistence in `plans-production.spec.ts`. The exact-head -real-Docker planning gate also passes, completing T18. T9 remains incomplete until -its remaining F6 common-CI and all-suite gate passes. +real-Docker planning gate also passes, completing T18. + +T9 is still incomplete. F6 runs the recordings in both required jobs, `test` and +`real-docker`, and the live Claude probes in the nightly `Live probes` workflow +([CI](ci.md)). No workflow records fixtures: when a recording goes stale, run +`scripts/record-planning.ts` again as above. T9 can be ticked when a repository +administrator requires `test` and `real-docker` on `main`, both pass on the same head, +and a `Live probes` run passes with the `CLAUDE_CODE_OAUTH_TOKEN` secret. From 88dcbf5afe553d81fc756bbe1e2228d73371ed51 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:36:24 -0700 Subject: [PATCH 04/15] Size the Live probes timeout from the probes' own limits Co-Authored-By: Claude Opus 5.5 --- .github/workflows/live-probes.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index 9a740a30..4bd31a38 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -13,8 +13,9 @@ concurrency: jobs: live-probes: runs-on: ubuntu-latest - # Covers the 10 min image build, up to 11 min per probe and teardown. - timeout-minutes: 60 + # The probes' own time limits total 33 min (6, 8, 8 and 11), and each of the three files may spend up to 10 min + # building the image. + timeout-minutes: 75 steps: # Fail at once when the secret is missing, so a missing secret never looks like a passing run. - name: Require the Claude token From acb744a7412d0da2a134d5a4a353dcc8b2198d18 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:39:58 -0700 Subject: [PATCH 05/15] Check every test the live probe switch enables, and correct the build-time note The probe-name check now finds every test inside a CODEBOOST_RUN_AUTH_PROBES gate and requires the "(live Claude)" suffix and a LIVE_PROBES entry, so an unsuffixed gated test cannot be silently skipped by the workflow's -t filter. Unrecognized gate forms fail the check. The workflow comment now says only agent-container and agent-supervisor build the image in beforeAll (53 min of limits, under 75). Co-Authored-By: Claude Opus 5.5 --- .github/workflows/live-probes.yml | 5 ++-- test/vitest-config.test.ts | 40 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index 4bd31a38..998b8802 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -13,8 +13,9 @@ concurrency: jobs: live-probes: runs-on: ubuntu-latest - # The probes' own time limits total 33 min (6, 8, 8 and 11), and each of the three files may spend up to 10 min - # building the image. + # The probes' own time limits total 33 min (6, 8, 8 and 11). agent-container and agent-supervisor may each spend + # up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. That + # is 53 min, plus setup. timeout-minutes: 75 steps: # Fail at once when the secret is missing, so a missing secret never looks like a passing run. diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 1b9cee3e..f4c711aa 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -45,6 +45,38 @@ const workflow = (name: string) => }; const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); +// The titles of every test that runs only when CODEBOOST_RUN_AUTH_PROBES is set. Each use of the switch must be an +// `if (...) { ... }` block or an `it.runIf(...)('title', ...)` call; any other form fails, so a new gate cannot hide +// a test from this check. +function gatedTestTitles(source: string): string[] { + const titles: string[] = []; + const gate = "process.env.CODEBOOST_RUN_AUTH_PROBES === '1'"; + for (const match of source.matchAll(/CODEBOOST_RUN_AUTH_PROBES/g)) { + const start = match.index - 'process.env.'.length; + const runIf = /\b(?:it|test)\.runIf\($/.exec(source.slice(0, start)); + if (runIf && source.startsWith(`${gate})(`, start)) { + const title = /^\(\s*'([^']*)'/.exec(source.slice(start + gate.length + 1)); + if (!title) throw new Error(`Gated test without a single-quoted title at offset ${start}.`); + titles.push(title[1]!); + continue; + } + if (!source.slice(0, start).endsWith('if (') || !source.startsWith(`${gate}) {`, start)) + throw new Error(`Unrecognized use of CODEBOOST_RUN_AUTH_PROBES at offset ${start}.`); + const open = start + gate.length + 2; + let depth = 0, end = open; + for (; end < source.length; end += 1) { + if (source[end] === '{') depth += 1; + else if (source[end] === '}' && --depth === 0) break; + } + const body = source.slice(open, end); + const calls = [...body.matchAll(/\b(?:it|test)(?:\.\w+)*\(/g)].length; + const named = [...body.matchAll(/\b(?:it|test)(?:\.\w+)*\(\s*'([^']*)'/g)].map(call => call[1]!); + if (calls !== named.length) throw new Error(`Gated block has a test without a single-quoted title at offset ${start}.`); + titles.push(...named); + } + return titles; +} + describe('CI workflows', () => { it('excludes exactly the Docker-backed files from the CI test job', () => { const ci = workflow('ci.yml'); @@ -105,6 +137,14 @@ describe('CI workflows', () => { expect(titles.length).toBe(LIVE_PROBES.filter(probe => probe.file === file).length); for (const title of titles) expect(LIVE_PROBES.some(probe => probe.file === file && probe.name.endsWith(` ${title}`))).toBe(true); + // Every test the probe switch enables must carry the suffix, or the workflow's -t filter never runs it. + const gatedTitles = gatedTestTitles(source); + expect(gatedTitles.length, file).toBeGreaterThan(0); + for (const title of gatedTitles) { + expect(title, file).toMatch(/ \(live Claude\)$/); + expect(titles, file).toContain(title); + } + expect(gatedTitles.length, file).toBe(titles.length); } const gated = readdirSync(join(ROOT, 'test')).filter(name => name.endsWith('.test.ts') && name !== 'vitest-config.test.ts') .filter(name => readFileSync(join(ROOT, 'test', name), 'utf8').includes('CODEBOOST_RUN_AUTH_PROBES')) From 9e5b959597f04e6c44673e119eeffd523585f817 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:48:08 -0700 Subject: [PATCH 06/15] Pin live probe full names and the CI test command Parse each live probe file with the Vite (oxc) parser instead of counting braces. The probe switch may only gate an if block of tests and call-free declarations, or it.runIf; the gated tests must match LIVE_PROBES by Vitest full name. The CI test job's npm test command may hold only --exclude pairs after --. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 10 ++- test/vitest-config.test.ts | 154 ++++++++++++++++++++++++++----------- 2 files changed, 115 insertions(+), 49 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index eb84de1b..fa1a6ade 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -71,9 +71,13 @@ The job: exits with success when it skips every test, so this step fails unless each probe listed in `scripts/live-probes.ts` passed and no other test ran. -If you add, rename or remove a probe, update `LIVE_PROBES` in -`scripts/live-probes.ts`. `test/vitest-config.test.ts` fails if the list and the -test titles differ. +If you add, rename or remove a probe, or rename a `describe` block around one, +update `LIVE_PROBES` in `scripts/live-probes.ts`. `test/vitest-config.test.ts` +parses each probe file. It fails unless the tests that +`CODEBOOST_RUN_AUTH_PROBES` enables have exactly the full names in the list: the +`describe` titles and the test title, joined with spaces, as Vitest names them. +The switch may only be the condition of an `if` block that holds only tests and +declarations, or of `it.runIf`. Any other use fails the test. Codex has no probe, because Codex is refused in every phase (#93). diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index f4c711aa..76cfdba5 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -3,6 +3,7 @@ import { readdirSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; +import { parseSync } from 'vite'; import { parse } from 'yaml'; import { checkLiveProbeReport, LIVE_PROBE_FILES, LIVE_PROBES } from '../scripts/live-probes.ts'; import config from '../vitest.config.ts'; @@ -45,36 +46,101 @@ const workflow = (name: string) => }; const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); -// The titles of every test that runs only when CODEBOOST_RUN_AUTH_PROBES is set. Each use of the switch must be an -// `if (...) { ... }` block or an `it.runIf(...)('title', ...)` call; any other form fails, so a new gate cannot hide -// a test from this check. -function gatedTestTitles(source: string): string[] { - const titles: string[] = []; - const gate = "process.env.CODEBOOST_RUN_AUTH_PROBES === '1'"; - for (const match of source.matchAll(/CODEBOOST_RUN_AUTH_PROBES/g)) { - const start = match.index - 'process.env.'.length; - const runIf = /\b(?:it|test)\.runIf\($/.exec(source.slice(0, start)); - if (runIf && source.startsWith(`${gate})(`, start)) { - const title = /^\(\s*'([^']*)'/.exec(source.slice(start + gate.length + 1)); - if (!title) throw new Error(`Gated test without a single-quoted title at offset ${start}.`); - titles.push(title[1]!); - continue; +type Node = { type: string; [key: string]: unknown }; +type ProbeFileTest = { name: string | undefined; gated: boolean }; +const PROBE_SWITCH = 'CODEBOOST_RUN_AUTH_PROBES'; +const isNode = (value: unknown): value is Node => + typeof value === 'object' && value !== null && typeof (value as { type?: unknown }).type === 'string'; +const children = (node: Node) => Object.values(node).flatMap(value => Array.isArray(value) ? value : [value]).filter(isNode); +const mentionsSwitch = (node: Node): boolean => (node.type === 'Identifier' && node.name === PROBE_SWITCH) + || (node.type === 'Literal' && node.value === PROBE_SWITCH) || children(node).some(mentionsSwitch); +const isIdentifier = (node: unknown, name: string) => isNode(node) && node.type === 'Identifier' && node.name === name; +const isMember = (node: unknown, object: (node: unknown) => boolean, property: string): boolean => + isNode(node) && node.type === 'MemberExpression' && node.computed === false && object(node.object) + && isIdentifier(node.property, property); +// Exactly `process.env.CODEBOOST_RUN_AUTH_PROBES === '1'`. +const isSwitchOn = (node: unknown) => isNode(node) && node.type === 'BinaryExpression' && node.operator === '===' + && isMember(node.left, env => isMember(env, process => isIdentifier(process, 'process'), 'env'), PROBE_SWITCH) + && isNode(node.right) && node.right.type === 'Literal' && node.right.value === '1'; +const literalText = (node: unknown) => !isNode(node) ? undefined + : node.type === 'Literal' && typeof node.value === 'string' ? node.value + : node.type === 'TemplateLiteral' && (node.expressions as unknown[]).length === 0 + ? (node.quasis as Array<{ value: { cooked: string } }>)[0]!.value.cooked : undefined; +// A call whose callee chain starts at `it`, `test` or `describe`, such as `it.runIf(...)('title', ...)`: the kind, the +// methods and arguments of the inner calls, and the outer call's arguments. +function suiteCall(node: Node) { + if (node.type !== 'CallExpression') return undefined; + const methods: string[] = [], innerArgs: Node[][] = []; + let callee = node.callee as Node; + for (;;) { + if (callee.type === 'CallExpression') { innerArgs.unshift((callee.arguments as Node[])); callee = callee.callee as Node; } + else if (callee.type === 'MemberExpression' && callee.computed === false && isNode(callee.property) + && callee.property.type === 'Identifier') { methods.unshift(callee.property.name as string); callee = callee.object as Node; } + else break; + } + if (callee.type !== 'Identifier' || !['it', 'test', 'describe'].includes(callee.name as string)) return undefined; + return { kind: callee.name as string, methods, innerArgs, args: node.arguments as Node[] }; +} +// Throws unless the node has no call outside a function body: a gated block may declare helpers for its tests, but +// any call at block level could register a test that this check does not see. +function assertNoCall(node: Node) { + if (['ArrowFunctionExpression', 'FunctionExpression'].includes(node.type)) return; + if (['CallExpression', 'NewExpression', 'TaggedTemplateExpression', 'AwaitExpression'].includes(node.type)) + throw new Error(`A gated block runs a ${node.type} outside a test at offset ${String(node.start)}.`); + children(node).forEach(assertNoCall); +} + +// Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and +// whether CODEBOOST_RUN_AUTH_PROBES enables it. The switch may appear only as the exact condition of an +// `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a +// declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Any other use throws, so +// no gate can hide a test from this check. +function probeFileTests(file: string, source: string): ProbeFileTest[] { + const { program, errors } = parseSync(file, source); + if (errors.length) throw new Error(`${file} does not parse: ${errors[0]!.message}`); + const tests: ProbeFileTest[] = []; + const record = (describes: string[], titleNode: unknown, gated: boolean) => { + const title = literalText(titleNode); + if (gated && title === undefined) throw new Error(`Gated test without a literal title in ${file}.`); + tests.push({ name: title === undefined ? undefined : [...describes, title].join(' '), gated }); + }; + const visit = (node: Node, describes: string[]): void => { + if (node.type === 'IfStatement' && mentionsSwitch(node.test as Node)) { + const block = node.consequent as Node; + if (!isSwitchOn(node.test) || block.type !== 'BlockStatement' || node.alternate !== null) + throw new Error(`Unrecognized ${PROBE_SWITCH} condition at offset ${String(node.start)} in ${file}.`); + for (const statement of block.body as Node[]) { + const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; + if (call && call.kind !== 'describe' && call.methods.length === 0) { + record(describes, call.args[0], true); + call.args.slice(1).forEach(arg => visit(arg, describes)); + } else if (statement.type === 'VariableDeclaration') { + assertNoCall(statement); + visit(statement, describes); + } else throw new Error(`A gated block holds a statement that is not a test at offset ${String(statement.start)} in ${file}.`); + } + return; } - if (!source.slice(0, start).endsWith('if (') || !source.startsWith(`${gate}) {`, start)) - throw new Error(`Unrecognized use of CODEBOOST_RUN_AUTH_PROBES at offset ${start}.`); - const open = start + gate.length + 2; - let depth = 0, end = open; - for (; end < source.length; end += 1) { - if (source[end] === '{') depth += 1; - else if (source[end] === '}' && --depth === 0) break; + const call = suiteCall(node); + if (call) { + const gate = call.kind !== 'describe' && call.methods.length === 1 && call.methods[0] === 'runIf' + && call.innerArgs.length === 1 && call.innerArgs[0]!.length === 1 && isSwitchOn(call.innerArgs[0]![0]); + if (!gate) call.innerArgs.flat().forEach(arg => visit(arg, describes)); + if (call.kind === 'describe') { + const title = literalText(call.args[0]); + call.args.forEach(arg => visit(arg, title === undefined ? describes : [...describes, title])); + return; + } + record(describes, call.args[0], gate); + call.args.forEach(arg => visit(arg, describes)); + return; } - const body = source.slice(open, end); - const calls = [...body.matchAll(/\b(?:it|test)(?:\.\w+)*\(/g)].length; - const named = [...body.matchAll(/\b(?:it|test)(?:\.\w+)*\(\s*'([^']*)'/g)].map(call => call[1]!); - if (calls !== named.length) throw new Error(`Gated block has a test without a single-quoted title at offset ${start}.`); - titles.push(...named); - } - return titles; + if ((node.type === 'Identifier' && node.name === PROBE_SWITCH) || (node.type === 'Literal' && node.value === PROBE_SWITCH)) + throw new Error(`Unrecognized use of ${PROBE_SWITCH} at offset ${String(node.start)} in ${file}.`); + children(node).forEach(child => visit(child, describes)); + }; + visit(program as unknown as Node, []); + return tests; } describe('CI workflows', () => { @@ -82,10 +148,13 @@ describe('CI workflows', () => { const ci = workflow('ci.yml'); const commands = runs(ci.jobs.test!.steps).filter(run => run.startsWith('npm test')); expect(commands).toHaveLength(1); - const words = commands[0]!.split(/\s+/); - const excluded = words.flatMap((word, index) => word === '--exclude' ? [words[index + 1]] : []); - expect(excluded.toSorted()).toEqual(DOCKER_TESTS); - expect(words.filter(word => word.startsWith('-')).every(word => word === '--' || word === '--exclude')).toBe(true); + // `npm test --` and then only `--exclude ` pairs: any other argument, such as a file filter, would narrow + // the job. + const [npm, test, separator, ...rest] = commands[0]!.trim().split(/\s+/); + expect([npm, test, separator]).toEqual(['npm', 'test', '--']); + expect(rest.length % 2).toBe(0); + expect(rest.filter((_, index) => index % 2 === 0).every(word => word === '--exclude')).toBe(true); + expect(rest.filter((_, index) => index % 2 === 1).toSorted()).toEqual(DOCKER_TESTS); }); it('runs the complete Vitest suite in real-docker on every pull request and on the CI push branches', () => { @@ -130,21 +199,14 @@ describe('CI workflows', () => { }); it('names every live probe exactly as its test file titles it', () => { - // Every "(live Claude)" title in the probe files is listed, and no other test file is gated on live credentials. + // The tests that the probe switch enables are exactly the listed probes, by Vitest full name, and every one ends in + // the suffix the workflow's -t filter selects. No other test in the file carries the suffix. for (const file of LIVE_PROBE_FILES) { - const source = readFileSync(join(ROOT, file), 'utf8'); - const titles = [...source.matchAll(/'([^']*\(live Claude\))'/g)].map(match => match[1]); - expect(titles.length).toBe(LIVE_PROBES.filter(probe => probe.file === file).length); - for (const title of titles) - expect(LIVE_PROBES.some(probe => probe.file === file && probe.name.endsWith(` ${title}`))).toBe(true); - // Every test the probe switch enables must carry the suffix, or the workflow's -t filter never runs it. - const gatedTitles = gatedTestTitles(source); - expect(gatedTitles.length, file).toBeGreaterThan(0); - for (const title of gatedTitles) { - expect(title, file).toMatch(/ \(live Claude\)$/); - expect(titles, file).toContain(title); - } - expect(gatedTitles.length, file).toBe(titles.length); + const tests = probeFileTests(file, readFileSync(join(ROOT, file), 'utf8')); + const gated = tests.filter(test => test.gated).map(test => test.name!); + expect(gated.toSorted(), file).toEqual(LIVE_PROBES.filter(probe => probe.file === file).map(probe => probe.name).toSorted()); + for (const name of gated) expect(name, file).toMatch(/ \(live Claude\)$/); + expect(tests.filter(test => !test.gated && test.name?.endsWith('(live Claude)')), file).toEqual([]); } const gated = readdirSync(join(ROOT, 'test')).filter(name => name.endsWith('.test.ts') && name !== 'vitest-config.test.ts') .filter(name => readFileSync(join(ROOT, 'test', name), 'utf8').includes('CODEBOOST_RUN_AUTH_PROBES')) From 465b007124bb9e2781409d1511a812c36986c450 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:52:17 -0700 Subject: [PATCH 07/15] Declare vite and close probe-switch gaps in the CI config test Declare vite 8.3.0 as a direct dev dependency, since the test imports parseSync from it. Count every textual occurrence of the probe switch in each probe file and require each to be a recognized gate; treat a template literal without expressions as a mention. Scan every .ts file under test/, not only *.test.ts, for the switch. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 7 ++++++- package-lock.json | 42 +++----------------------------------- package.json | 1 + test/vitest-config.test.ts | 32 +++++++++++++++++++---------- 4 files changed, 31 insertions(+), 51 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index fa1a6ade..c7738825 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -77,7 +77,12 @@ parses each probe file. It fails unless the tests that `CODEBOOST_RUN_AUTH_PROBES` enables have exactly the full names in the list: the `describe` titles and the test title, joined with spaces, as Vitest names them. The switch may only be the condition of an `if` block that holds only tests and -declarations, or of `it.runIf`. Any other use fails the test. +declarations, or of `it.runIf`. The test counts every occurrence of the name +`CODEBOOST_RUN_AUTH_PROBES` in each probe file's text, comments and strings +included, and fails unless each one is such a gate. It also fails if any other +`.ts` file under `test/`, helpers and fixtures included, names the switch. It +cannot catch a variable name built at runtime from pieces, such as +`process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`. Codex has no probe, because Codex is refused in every phase (#93). diff --git a/package-lock.json b/package-lock.json index fde54e92..9884dd3e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -19,6 +19,7 @@ "@playwright/test": "1.63.0", "@types/node": "26.6.2", "typescript": "7.0.2", + "vite": "8.3.0", "vitest": "5.0.1" }, "engines": { @@ -77,7 +78,6 @@ "integrity": "sha512-rDS5/31E9HfPl/CIzGrn0DOlvBbXFseQ5URJ9sYMfstbKLD/c6Gm9vmRzRGDdAXyOIL4zmO37lc9RIwYqVruZw==", "dev": true, "license": "MIT", - "peer": true, "funding": { "url": "https://github.com/sponsors/oxc-project" } @@ -111,7 +111,6 @@ "os": [ "android" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -129,7 +128,6 @@ "os": [ "android" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -147,7 +145,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -165,7 +162,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -183,7 +179,6 @@ "os": [ "freebsd" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -201,7 +196,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -222,7 +216,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -243,7 +236,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -264,7 +256,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -285,7 +276,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -306,7 +296,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -327,7 +316,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -345,7 +333,6 @@ "os": [ "openharmony" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -363,7 +350,6 @@ "os": [ "win32" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -381,7 +367,6 @@ "os": [ "win32" ], - "peer": true, "engines": { "node": "^20.19.0 || >=22.12.0" } @@ -391,8 +376,7 @@ "resolved": "https://registry.npmjs.org/@rolldown/pluginutils/-/pluginutils-1.0.1.tgz", "integrity": "sha512-2j9bGt5Jh8hj+vPtgzPtl72j0yRxHAyumoo6TNfAjsLB04UtpSvPbPcDcBMxz7n+9CYB0c1GxQFxYRg2jimqGw==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@types/chai": { "version": "5.2.3", @@ -849,7 +833,6 @@ "integrity": "sha512-Btj2BOOO83o3WyH59e8MgXsxEQVcarkUOpEYrubB0urwnN10yQ364rsiByU11nZlqWYZm05i/of7io4mzihBtQ==", "dev": true, "license": "Apache-2.0", - "peer": true, "engines": { "node": ">=8" } @@ -941,7 +924,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } @@ -970,7 +952,6 @@ "integrity": "sha512-WkUDrojuJs0xkgGf2udWxa3yGBRxPtxUkB79i6aCZLRgc7PM8fZe9TosfPDcvEpQZbuFASnHYmRLBLUbmLOIIA==", "dev": true, "license": "MPL-2.0", - "peer": true, "dependencies": { "detect-libc": "^2.0.3" }, @@ -1008,7 +989,6 @@ "os": [ "android" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1030,7 +1010,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1052,7 +1031,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1074,7 +1052,6 @@ "os": [ "freebsd" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1096,7 +1073,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1121,7 +1097,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1146,7 +1121,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1171,7 +1145,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1196,7 +1169,6 @@ "os": [ "linux" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1218,7 +1190,6 @@ "os": [ "win32" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1240,7 +1211,6 @@ "os": [ "win32" ], - "peer": true, "engines": { "node": ">= 12.0.0" }, @@ -1271,7 +1241,6 @@ } ], "license": "MIT", - "peer": true, "bin": { "nanoid": "bin/nanoid.cjs" }, @@ -1298,8 +1267,7 @@ "resolved": "https://registry.npmjs.org/picocolors/-/picocolors-1.1.1.tgz", "integrity": "sha512-xceH2snhtb5M9liqDsmEw56le376mTZkEX/jEb/RxNFyegNul7eNslCXP9FDj/Lcu0X8KEyMceP2ntpaHrDEVA==", "dev": true, - "license": "ISC", - "peer": true + "license": "ISC" }, "node_modules/picomatch": { "version": "4.0.7", @@ -1363,7 +1331,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "nanoid": "^3.3.18", "picocolors": "^1.1.1", @@ -1388,7 +1355,6 @@ "integrity": "sha512-hx/Pv0N1haXRb11qkfnK5MXB/iqr7i0yjWQqmO9uHqZpBgQSqzc8UsSnEpalsh+j1I8qQ2CkXAkJC8Br3dKSlg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@oxc-project/types": "=0.150.0", "@rolldown/pluginutils": "^1.0.0" @@ -1430,7 +1396,6 @@ "integrity": "sha512-UXWMKhLOwVKb728IUtQPXxfYU+usdybtUrK/8uGE8CQMvrhOpwvzDBwj0QhSL7MQc7vIsISBG8VQ8+IDQxpfQA==", "dev": true, "license": "BSD-3-Clause", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -1534,7 +1499,6 @@ "integrity": "sha512-lhZBVvEHefgE+HQZC9O7EBJgCU/nVzFNl7vkS4RE0APtWLP02/8QVIkQtzBxPquh7lq5/78NHipTj7ODQ6XuyQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "lightningcss": "^1.33.0", "picomatch": "^4.0.7", diff --git a/package.json b/package.json index b1691203..28588ea9 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "@playwright/test": "1.63.0", "@types/node": "26.6.2", "typescript": "7.0.2", + "vite": "8.3.0", "vitest": "5.0.1" } } diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 76cfdba5..f6f6dbb2 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -1,6 +1,6 @@ import { spawnSync } from 'node:child_process'; import { readdirSync, readFileSync } from 'node:fs'; -import { join } from 'node:path'; +import { join, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import { parseSync } from 'vite'; @@ -52,8 +52,6 @@ const PROBE_SWITCH = 'CODEBOOST_RUN_AUTH_PROBES'; const isNode = (value: unknown): value is Node => typeof value === 'object' && value !== null && typeof (value as { type?: unknown }).type === 'string'; const children = (node: Node) => Object.values(node).flatMap(value => Array.isArray(value) ? value : [value]).filter(isNode); -const mentionsSwitch = (node: Node): boolean => (node.type === 'Identifier' && node.name === PROBE_SWITCH) - || (node.type === 'Literal' && node.value === PROBE_SWITCH) || children(node).some(mentionsSwitch); const isIdentifier = (node: unknown, name: string) => isNode(node) && node.type === 'Identifier' && node.name === name; const isMember = (node: unknown, object: (node: unknown) => boolean, property: string): boolean => isNode(node) && node.type === 'MemberExpression' && node.computed === false && object(node.object) @@ -66,6 +64,9 @@ const literalText = (node: unknown) => !isNode(node) ? undefined : node.type === 'Literal' && typeof node.value === 'string' ? node.value : node.type === 'TemplateLiteral' && (node.expressions as unknown[]).length === 0 ? (node.quasis as Array<{ value: { cooked: string } }>)[0]!.value.cooked : undefined; +// The switch as an identifier, a string literal or a template literal without expressions. +const isSwitchName = (node: Node) => (node.type === 'Identifier' && node.name === PROBE_SWITCH) || literalText(node) === PROBE_SWITCH; +const mentionsSwitch = (node: Node): boolean => isSwitchName(node) || children(node).some(mentionsSwitch); // A call whose callee chain starts at `it`, `test` or `describe`, such as `it.runIf(...)('title', ...)`: the kind, the // methods and arguments of the inner calls, and the outer call's arguments. function suiteCall(node: Node) { @@ -93,12 +94,14 @@ function assertNoCall(node: Node) { // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and // whether CODEBOOST_RUN_AUTH_PROBES enables it. The switch may appear only as the exact condition of an // `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a -// declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Any other use throws, so -// no gate can hide a test from this check. +// declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the +// switch's name in the source text, comments and strings included, must be one of these gates, or this throws. A +// variable name built at runtime from pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`, is not caught. function probeFileTests(file: string, source: string): ProbeFileTest[] { const { program, errors } = parseSync(file, source); if (errors.length) throw new Error(`${file} does not parse: ${errors[0]!.message}`); const tests: ProbeFileTest[] = []; + let gates = 0; const record = (describes: string[], titleNode: unknown, gated: boolean) => { const title = literalText(titleNode); if (gated && title === undefined) throw new Error(`Gated test without a literal title in ${file}.`); @@ -109,6 +112,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { const block = node.consequent as Node; if (!isSwitchOn(node.test) || block.type !== 'BlockStatement' || node.alternate !== null) throw new Error(`Unrecognized ${PROBE_SWITCH} condition at offset ${String(node.start)} in ${file}.`); + gates += 1; for (const statement of block.body as Node[]) { const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; if (call && call.kind !== 'describe' && call.methods.length === 0) { @@ -125,7 +129,8 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { if (call) { const gate = call.kind !== 'describe' && call.methods.length === 1 && call.methods[0] === 'runIf' && call.innerArgs.length === 1 && call.innerArgs[0]!.length === 1 && isSwitchOn(call.innerArgs[0]![0]); - if (!gate) call.innerArgs.flat().forEach(arg => visit(arg, describes)); + if (gate) gates += 1; + else call.innerArgs.flat().forEach(arg => visit(arg, describes)); if (call.kind === 'describe') { const title = literalText(call.args[0]); call.args.forEach(arg => visit(arg, title === undefined ? describes : [...describes, title])); @@ -135,11 +140,14 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { call.args.forEach(arg => visit(arg, describes)); return; } - if ((node.type === 'Identifier' && node.name === PROBE_SWITCH) || (node.type === 'Literal' && node.value === PROBE_SWITCH)) + if (isSwitchName(node)) throw new Error(`Unrecognized use of ${PROBE_SWITCH} at offset ${String(node.start)} in ${file}.`); children(node).forEach(child => visit(child, describes)); }; visit(program as unknown as Node, []); + const occurrences = source.split(PROBE_SWITCH).length - 1; + if (occurrences !== gates) + throw new Error(`${file} names ${PROBE_SWITCH} ${String(occurrences)} time(s) but has ${String(gates)} recognized gate(s).`); return tests; } @@ -208,10 +216,12 @@ describe('CI workflows', () => { for (const name of gated) expect(name, file).toMatch(/ \(live Claude\)$/); expect(tests.filter(test => !test.gated && test.name?.endsWith('(live Claude)')), file).toEqual([]); } - const gated = readdirSync(join(ROOT, 'test')).filter(name => name.endsWith('.test.ts') && name !== 'vitest-config.test.ts') - .filter(name => readFileSync(join(ROOT, 'test', name), 'utf8').includes('CODEBOOST_RUN_AUTH_PROBES')) - .map(name => `test/${name}`); - expect(gated).toEqual(LIVE_PROBE_FILES); + // Every TypeScript file under test/, helpers and fixtures included, since a probe file could import a gate. + const named = readdirSync(join(ROOT, 'test'), { recursive: true, encoding: 'utf8' }) + .map(name => `test/${name.split(sep).join('/')}`) + .filter(file => file.endsWith('.ts') && file !== 'test/vitest-config.test.ts') + .filter(file => readFileSync(join(ROOT, file), 'utf8').includes(PROBE_SWITCH)); + expect(named.toSorted()).toEqual(LIVE_PROBE_FILES.toSorted()); }); }); From be37122b32e3b80e4aacbf7880ee10341e740d15 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 00:56:46 -0700 Subject: [PATCH 08/15] Refuse skipped or soft-failing gate jobs and dead probe gates Assert that the required test and real-docker jobs and the live-probes job carry no if or continue-on-error at job or step level. Count a probe switch gate only at module load or in a describe callback. Scan every JS/TS source extension under test/ for the switch. Document the 60-day schedule disable. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 19 ++++++++++--- test/vitest-config.test.ts | 55 +++++++++++++++++++++++++++++--------- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index c7738825..11e41e43 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -57,7 +57,10 @@ them. They do not call a vendor or GitHub. The live probes call the real Claude service. They are not a pull request check, and they do not run on a pull request or a push. GitHub runs scheduled workflows only from the default branch, so the nightly run starts after this -workflow is on `main`. +workflow is on `main`. The repository is public, so GitHub disables this +scheduled workflow after 60 days without repository activity. To turn it on +again, open the Actions tab, select the `Live probes` workflow and select +**Enable workflow**. The job: @@ -71,16 +74,26 @@ The job: exits with success when it skips every test, so this step fails unless each probe listed in `scripts/live-probes.ts` passed and no other test ran. +`test/vitest-config.test.ts` fails if the `live-probes` job, or the `test` job +in `ci.yml` or the `real-docker` job in `agent-isolation.yml`, or any step of +these jobs, has an `if` condition or `continue-on-error`. Either one could skip +a step or let it fail while the job passes. + If you add, rename or remove a probe, or rename a `describe` block around one, update `LIVE_PROBES` in `scripts/live-probes.ts`. `test/vitest-config.test.ts` parses each probe file. It fails unless the tests that `CODEBOOST_RUN_AUTH_PROBES` enables have exactly the full names in the list: the `describe` titles and the test title, joined with spaces, as Vitest names them. The switch may only be the condition of an `if` block that holds only tests and -declarations, or of `it.runIf`. The test counts every occurrence of the name +declarations, or of `it.runIf`. Such a gate must be where Vitest registers tests +as it loads the file: at the top level of the file or in a `describe` callback. +A gate in any other function, such as a helper that is never called, makes the +test fail. The test counts every occurrence of the name `CODEBOOST_RUN_AUTH_PROBES` in each probe file's text, comments and strings included, and fails unless each one is such a gate. It also fails if any other -`.ts` file under `test/`, helpers and fixtures included, names the switch. It +source file under `test/` names the switch: any `.ts`, `.tsx`, `.mts`, `.cts`, +`.js`, `.mjs` or `.cjs` file, helpers and fixtures included, except +`test/vitest-config.test.ts` itself. It cannot catch a variable name built at runtime from pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`. diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index f6f6dbb2..948269aa 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -42,15 +42,18 @@ describe('Vitest scheduling', () => { const workflow = (name: string) => parse(readFileSync(join(ROOT, '.github/workflows', name), 'utf8')) as { on: Record; - jobs: Record }> }>; + jobs: Record & { env?: unknown; + steps: Array & { name?: string; run?: string; env?: Record }> }>; }; const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); type Node = { type: string; [key: string]: unknown }; type ProbeFileTest = { name: string | undefined; gated: boolean }; const PROBE_SWITCH = 'CODEBOOST_RUN_AUTH_PROBES'; +const SOURCE = /\.(?:ts|tsx|mts|cts|js|mjs|cjs)$/; const isNode = (value: unknown): value is Node => typeof value === 'object' && value !== null && typeof (value as { type?: unknown }).type === 'string'; +const FUNCTIONS = ['ArrowFunctionExpression', 'FunctionExpression', 'FunctionDeclaration']; const children = (node: Node) => Object.values(node).flatMap(value => Array.isArray(value) ? value : [value]).filter(isNode); const isIdentifier = (node: unknown, name: string) => isNode(node) && node.type === 'Identifier' && node.name === name; const isMember = (node: unknown, object: (node: unknown) => boolean, property: string): boolean => @@ -85,14 +88,16 @@ function suiteCall(node: Node) { // Throws unless the node has no call outside a function body: a gated block may declare helpers for its tests, but // any call at block level could register a test that this check does not see. function assertNoCall(node: Node) { - if (['ArrowFunctionExpression', 'FunctionExpression'].includes(node.type)) return; + if (FUNCTIONS.includes(node.type)) return; if (['CallExpression', 'NewExpression', 'TaggedTemplateExpression', 'AwaitExpression'].includes(node.type)) throw new Error(`A gated block runs a ${node.type} outside a test at offset ${String(node.start)}.`); children(node).forEach(assertNoCall); } // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and -// whether CODEBOOST_RUN_AUTH_PROBES enables it. The switch may appear only as the exact condition of an +// whether CODEBOOST_RUN_AUTH_PROBES enables it. A gate counts only where Vitest registers tests while it loads the +// file: at the module's top level or in a `describe` callback, never in any other function body, so a gate in a +// helper that is never called throws. The switch may appear only as the exact condition of an // `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a // declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the // switch's name in the source text, comments and strings included, must be one of these gates, or this throws. A @@ -107,9 +112,13 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { if (gated && title === undefined) throw new Error(`Gated test without a literal title in ${file}.`); tests.push({ name: title === undefined ? undefined : [...describes, title].join(' '), gated }); }; - const visit = (node: Node, describes: string[]): void => { + // `loading` is true where code runs as Vitest loads the file: outside every function body except `describe` callbacks. + const visit = (node: Node, describes: string[], loading: boolean): void => { + if (FUNCTIONS.includes(node.type)) { children(node).forEach(child => visit(child, describes, false)); return; } if (node.type === 'IfStatement' && mentionsSwitch(node.test as Node)) { const block = node.consequent as Node; + if (!loading) + throw new Error(`A ${PROBE_SWITCH} gate outside module load or a describe callback at offset ${String(node.start)} in ${file}.`); if (!isSwitchOn(node.test) || block.type !== 'BlockStatement' || node.alternate !== null) throw new Error(`Unrecognized ${PROBE_SWITCH} condition at offset ${String(node.start)} in ${file}.`); gates += 1; @@ -117,10 +126,10 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; if (call && call.kind !== 'describe' && call.methods.length === 0) { record(describes, call.args[0], true); - call.args.slice(1).forEach(arg => visit(arg, describes)); + call.args.slice(1).forEach(arg => visit(arg, describes, loading)); } else if (statement.type === 'VariableDeclaration') { assertNoCall(statement); - visit(statement, describes); + visit(statement, describes, loading); } else throw new Error(`A gated block holds a statement that is not a test at offset ${String(statement.start)} in ${file}.`); } return; @@ -129,22 +138,29 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { if (call) { const gate = call.kind !== 'describe' && call.methods.length === 1 && call.methods[0] === 'runIf' && call.innerArgs.length === 1 && call.innerArgs[0]!.length === 1 && isSwitchOn(call.innerArgs[0]![0]); + if (gate && !loading) + throw new Error(`A ${PROBE_SWITCH} gate outside module load or a describe callback at offset ${String(node.start)} in ${file}.`); if (gate) gates += 1; - else call.innerArgs.flat().forEach(arg => visit(arg, describes)); + else call.innerArgs.flat().forEach(arg => visit(arg, describes, loading)); if (call.kind === 'describe') { const title = literalText(call.args[0]); - call.args.forEach(arg => visit(arg, title === undefined ? describes : [...describes, title])); + const inner = title === undefined ? describes : [...describes, title]; + // Vitest calls a describe callback while it loads the file, so the callback's body keeps `loading`. + for (const arg of call.args) { + if (FUNCTIONS.includes(arg.type)) children(arg).forEach(child => visit(child, inner, loading)); + else visit(arg, inner, loading); + } return; } record(describes, call.args[0], gate); - call.args.forEach(arg => visit(arg, describes)); + call.args.forEach(arg => visit(arg, describes, loading)); return; } if (isSwitchName(node)) throw new Error(`Unrecognized use of ${PROBE_SWITCH} at offset ${String(node.start)} in ${file}.`); - children(node).forEach(child => visit(child, describes)); + children(node).forEach(child => visit(child, describes, loading)); }; - visit(program as unknown as Node, []); + visit(program as unknown as Node, [], true); const occurrences = source.split(PROBE_SWITCH).length - 1; if (occurrences !== gates) throw new Error(`${file} names ${PROBE_SWITCH} ${String(occurrences)} time(s) but has ${String(gates)} recognized gate(s).`); @@ -193,6 +209,18 @@ describe('CI workflows', () => { for (const file of LIVE_PROBE_FILES) expect(DOCKER_TESTS).toContain(file); }); + it('never skips or soft-fails a gating job or any of its steps', () => { + // A condition or continue-on-error on the required checks or on the live probe job could skip a step or let it + // fail while the job still passes. + for (const [name, job] of [['ci.yml', 'test'], ['agent-isolation.yml', 'real-docker'], ['live-probes.yml', 'live-probes']] as const) { + const jobConfig = workflow(name).jobs[job]!; + for (const part of [jobConfig, ...jobConfig.steps]) { + expect(Object.keys(part), `${name} ${job}`).not.toContain('if'); + expect(Object.keys(part), `${name} ${job}`).not.toContain('continue-on-error'); + } + } + }); + it('fails the live probe job at once when the token secret is empty', () => { const guard = workflow('live-probes.yml').jobs['live-probes']!.steps[0]!; expect(guard.env).toEqual({ CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }); @@ -216,10 +244,11 @@ describe('CI workflows', () => { for (const name of gated) expect(name, file).toMatch(/ \(live Claude\)$/); expect(tests.filter(test => !test.gated && test.name?.endsWith('(live Claude)')), file).toEqual([]); } - // Every TypeScript file under test/, helpers and fixtures included, since a probe file could import a gate. + // Every JavaScript or TypeScript source file under test/ except this one, helpers and fixtures included, since a + // probe file could import a gate. const named = readdirSync(join(ROOT, 'test'), { recursive: true, encoding: 'utf8' }) .map(name => `test/${name.split(sep).join('/')}`) - .filter(file => file.endsWith('.ts') && file !== 'test/vitest-config.test.ts') + .filter(file => SOURCE.test(file) && file !== 'test/vitest-config.test.ts') .filter(file => readFileSync(join(ROOT, file), 'utf8').includes(PROBE_SWITCH)); expect(named.toSorted()).toEqual(LIVE_PROBE_FILES.toSorted()); }); From e080b5e21efaac4cb86b08fdf2a1676707432faa Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:03:39 -0700 Subject: [PATCH 09/15] Refuse unmodelled describes around probes and pin the required check jobs - Throw when a describe around a gated probe has a non-literal title or a modifier such as .each or .for, instead of computing a wrong full name. - Derive the live probe -t filter and the suffix checks from LIVE_PROBE_MARKER. - Pin the test and real-docker jobs: one job per workflow, keys runs-on, timeout-minutes and steps only (no name, no matrix), exact steps. - Give the CI test job a 45 min timeout (longest recent run: 20 min) and check that all three workflows use the same Node version. - Document that renaming either required job breaks the required checks. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 3 ++ docs/implementation/ci.md | 14 +++++++- scripts/live-probes.ts | 3 +- test/vitest-config.test.ts | 68 ++++++++++++++++++++++++++++++++------ 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c382283..a731a63a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,9 @@ permissions: jobs: test: runs-on: ubuntu-latest + # The longest recent run took 20 min, about half of this limit. A hung browser suite would otherwise hold the + # runner for GitHub's default of 360 min. + timeout-minutes: 45 steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 11e41e43..99eaa6b0 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -16,10 +16,19 @@ settings are not stored in the repository. so both can be required checks. A required check that a path filter skips never reports, and GitHub then blocks the merge. +A check takes its name from the job id, or from the job's `name` when it has +one, and a matrix adds its values to the name. So do not rename the `test` or +`real-docker` job, and do not give either one a `name` or a `strategy` (matrix): +the required check would then never report, and no pull request could merge. +`test/vitest-config.test.ts` fails if either job has any key other than +`runs-on`, `timeout-minutes` and `steps`, if its steps differ from the ones +this page lists, or if either workflow has another job. + ### `test` `test` runs the Vitest files that do not need Docker, and then the browser -suite (`npm run test:browser`). It excludes the Docker-backed files with +suite (`npm run test:browser`). Its time limit is 45 minutes; its longest +recent run took 20 minutes. It excludes the Docker-backed files with `--exclude`. The Docker-backed files are the `dockerTests` list in `vitest.config.ts`. The excludes must equal that list: `test/vitest-config.test.ts` fails if they differ. @@ -84,6 +93,9 @@ update `LIVE_PROBES` in `scripts/live-probes.ts`. `test/vitest-config.test.ts` parses each probe file. It fails unless the tests that `CODEBOOST_RUN_AUTH_PROBES` enables have exactly the full names in the list: the `describe` titles and the test title, joined with spaces, as Vitest names them. +Each `describe` around a gated test must be a plain `describe(...)` with a +literal title. A title with expressions, or `describe.each`, `describe.for`, +`describe.skip` or any other modifier, makes the test fail. The switch may only be the condition of an `if` block that holds only tests and declarations, or of `it.runIf`. Such a gate must be where Vitest registers tests as it loads the file: at the top level of the file or in a `describe` callback. diff --git a/scripts/live-probes.ts b/scripts/live-probes.ts index 9454c340..9547a76a 100644 --- a/scripts/live-probes.ts +++ b/scripts/live-probes.ts @@ -10,7 +10,8 @@ import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; import { pathToFileURL } from 'node:url'; -// Every probe title ends with this suffix. The workflow selects the probes with `-t` and this text. +// Every probe title ends with a space and this suffix. The workflow's `-t` filter is this text escaped as a regular +// expression and anchored at the end; test/vitest-config.test.ts fails if the two differ. export const LIVE_PROBE_MARKER = '(live Claude)'; export const LIVE_PROBES = [ diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 948269aa..7241a62a 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -5,7 +5,7 @@ import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import { parseSync } from 'vite'; import { parse } from 'yaml'; -import { checkLiveProbeReport, LIVE_PROBE_FILES, LIVE_PROBES } from '../scripts/live-probes.ts'; +import { checkLiveProbeReport, LIVE_PROBE_FILES, LIVE_PROBE_MARKER, LIVE_PROBES } from '../scripts/live-probes.ts'; import config from '../vitest.config.ts'; const ROOT = fileURLToPath(new URL('..', import.meta.url)); @@ -45,10 +45,17 @@ const workflow = (name: string) => jobs: Record & { env?: unknown; steps: Array & { name?: string; run?: string; env?: Record }> }>; }; +// The `-t` pattern that selects the titles ending in the marker. +const MARKER_FILTER = `${LIVE_PROBE_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}$`; +const NODE_VERSION = '26.7.0'; +const SETUP = [{ uses: 'actions/checkout@v4' }, + { uses: 'actions/setup-node@v4', with: { 'node-version': NODE_VERSION, cache: 'npm' } }]; const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); type Node = { type: string; [key: string]: unknown }; type ProbeFileTest = { name: string | undefined; gated: boolean }; +// An enclosing `describe`: its literal title, or why its title is not known. +type Suite = { title: string } | { problem: string }; const PROBE_SWITCH = 'CODEBOOST_RUN_AUTH_PROBES'; const SOURCE = /\.(?:ts|tsx|mts|cts|js|mjs|cjs)$/; const isNode = (value: unknown): value is Node => @@ -95,7 +102,9 @@ function assertNoCall(node: Node) { } // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and -// whether CODEBOOST_RUN_AUTH_PROBES enables it. A gate counts only where Vitest registers tests while it loads the +// whether CODEBOOST_RUN_AUTH_PROBES enables it. Every `describe` around a gated test must be a plain `describe(...)` +// with a literal title, or this throws: a title with expressions, or `describe.each`, `.for`, `.skip` or any other +// modifier, would make the full name differ from the one this computes. A gate counts only where Vitest registers tests while it loads the // file: at the module's top level or in a `describe` callback, never in any other function body, so a gate in a // helper that is never called throws. The switch may appear only as the exact condition of an // `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a @@ -107,13 +116,16 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { if (errors.length) throw new Error(`${file} does not parse: ${errors[0]!.message}`); const tests: ProbeFileTest[] = []; let gates = 0; - const record = (describes: string[], titleNode: unknown, gated: boolean) => { + const record = (describes: Suite[], titleNode: unknown, gated: boolean) => { const title = literalText(titleNode); if (gated && title === undefined) throw new Error(`Gated test without a literal title in ${file}.`); - tests.push({ name: title === undefined ? undefined : [...describes, title].join(' '), gated }); + const unknown = describes.find(suite => 'problem' in suite); + if (gated && unknown) throw new Error(`Gated test in ${file} inside a describe ${unknown.problem}.`); + const titles = describes.map(suite => 'title' in suite ? suite.title : '?'); + tests.push({ name: title === undefined ? undefined : [...titles, title].join(' '), gated }); }; // `loading` is true where code runs as Vitest loads the file: outside every function body except `describe` callbacks. - const visit = (node: Node, describes: string[], loading: boolean): void => { + const visit = (node: Node, describes: Suite[], loading: boolean): void => { if (FUNCTIONS.includes(node.type)) { children(node).forEach(child => visit(child, describes, false)); return; } if (node.type === 'IfStatement' && mentionsSwitch(node.test as Node)) { const block = node.consequent as Node; @@ -144,7 +156,10 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { else call.innerArgs.flat().forEach(arg => visit(arg, describes, loading)); if (call.kind === 'describe') { const title = literalText(call.args[0]); - const inner = title === undefined ? describes : [...describes, title]; + const suite: Suite = call.methods.length > 0 + ? { problem: `with the modifier .${call.methods.join('.')} at offset ${String(node.start)}` } + : title === undefined ? { problem: `without a literal title at offset ${String(node.start)}` } : { title }; + const inner = [...describes, suite]; // Vitest calls a describe callback while it loads the file, so the callback's body keeps `loading`. for (const arg of call.args) { if (FUNCTIONS.includes(arg.type)) children(arg).forEach(child => visit(child, inner, loading)); @@ -189,7 +204,34 @@ describe('CI workflows', () => { expect(ci.on.pull_request).toBeNull(); expect(isolation.on.push).toEqual(ci.on.push); expect(isolation.on.push).toEqual({ branches: ['main', 'codex/**'] }); - expect(runs(isolation.jobs['real-docker']!.steps)).toEqual(['npm ci --ignore-scripts', 'npm run typecheck', 'npm test']); + expect(isolation.jobs['real-docker']!.steps).toEqual( + [...SETUP, { run: 'npm ci --ignore-scripts' }, { run: 'npm run typecheck' }, { run: 'npm test' }]); + }); + + it('runs exactly the type check, the non-Docker Vitest files and the browser suite in the CI test job', () => { + expect(workflow('ci.yml').jobs.test!.steps).toEqual([...SETUP, { run: 'npm ci --ignore-scripts' }, { run: 'npm run typecheck' }, + { run: `npm test -- ${DOCKER_TESTS.map(file => `--exclude ${file}`).join(' ')}` }, + { run: 'npx playwright install --with-deps chromium' }, { run: 'npm run test:browser' }]); + }); + + it('keeps the required check names: one job per workflow, with no name and no matrix', () => { + // A check is named after its job id, or after its `name` when it has one; a matrix adds the matrix values to the + // name. Either change would leave the required check `test` or `real-docker` never reporting. The test job's + // timeout is about twice its longest recent run (20 min). + const ci = workflow('ci.yml').jobs, isolation = workflow('agent-isolation.yml').jobs; + expect(Object.keys(ci)).toEqual(['test']); + expect(Object.keys(isolation)).toEqual(['real-docker']); + expect(ci.test).toMatchObject({ 'runs-on': 'ubuntu-latest', 'timeout-minutes': 45 }); + expect(isolation['real-docker']).toMatchObject({ 'runs-on': 'ubuntu-latest', 'timeout-minutes': 45 }); + for (const job of [ci.test!, isolation['real-docker']!]) + expect(Object.keys(job).toSorted()).toEqual(['runs-on', 'steps', 'timeout-minutes']); + }); + + it('uses the same Node version in every workflow', () => { + const versions = ['ci.yml', 'agent-isolation.yml', 'live-probes.yml'].flatMap(name => Object.values(workflow(name).jobs) + .flatMap(job => job.steps.filter(step => String(step.uses).startsWith('actions/setup-node@')) + .map(step => (step.with as Record | undefined)?.['node-version']))); + expect(versions).toEqual([NODE_VERSION, NODE_VERSION, NODE_VERSION]); }); it('runs only the live Claude probes, by hand or nightly, never on a pull request or push', () => { @@ -200,8 +242,8 @@ describe('CI workflows', () => { const vitest = job.steps.filter(step => step.run?.startsWith('npx vitest')); expect(vitest).toHaveLength(1); const words = vitest[0]!.run!.split(/\s+/); - // The docker project runs the files one at a time; the filter keeps only titles that end in "(live Claude)". - expect(vitest[0]!.run!.startsWith("npx vitest run --project docker -t '\\(live Claude\\)$' ")).toBe(true); + // The docker project runs the files one at a time; the filter keeps only titles that end in the marker. + expect(vitest[0]!.run!.startsWith(`npx vitest run --project docker -t '${MARKER_FILTER}' `)).toBe(true); expect(words.filter(word => word.startsWith('test/'))).toEqual(LIVE_PROBE_FILES); expect(vitest[0]!.env).toEqual({ CODEBOOST_RUN_AUTH_PROBES: '1', CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }); @@ -241,8 +283,12 @@ describe('CI workflows', () => { const tests = probeFileTests(file, readFileSync(join(ROOT, file), 'utf8')); const gated = tests.filter(test => test.gated).map(test => test.name!); expect(gated.toSorted(), file).toEqual(LIVE_PROBES.filter(probe => probe.file === file).map(probe => probe.name).toSorted()); - for (const name of gated) expect(name, file).toMatch(/ \(live Claude\)$/); - expect(tests.filter(test => !test.gated && test.name?.endsWith('(live Claude)')), file).toEqual([]); + for (const name of gated) { + expect(name.endsWith(` ${LIVE_PROBE_MARKER}`), name).toBe(true); + expect(name, file).toMatch(new RegExp(MARKER_FILTER)); + } + expect(tests.filter(test => !test.gated && test.name !== undefined && new RegExp(MARKER_FILTER).test(test.name)), file) + .toEqual([]); } // Every JavaScript or TypeScript source file under test/ except this one, helpers and fixtures included, since a // probe file could import a gate. From 2225d0c3d5a19cafcb10c3497ea9a5ab227af647 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:09:00 -0700 Subject: [PATCH 10/15] Pin workflow keys and live-probes steps; reject nested probe gates - Allow-list each workflow's top-level keys and the live-probes job keys, and pin the live-probes steps exactly. - Accept a probe gate only as a statement directly in the module body or in a describe callback body, not inside another if, block or expression. - Docs: say "variable declarations", list the pinned steps of each job. - live-probes.yml: include the teardown hooks in the timeout arithmetic. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/live-probes.yml | 5 ++- docs/implementation/ci.md | 26 ++++++++++--- test/vitest-config.test.ts | 63 +++++++++++++++++++++++-------- 3 files changed, 70 insertions(+), 24 deletions(-) diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index 998b8802..c96fd3a7 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -14,8 +14,9 @@ jobs: live-probes: runs-on: ubuntu-latest # The probes' own time limits total 33 min (6, 8, 8 and 11). agent-container and agent-supervisor may each spend - # up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. That - # is 53 min, plus setup. + # up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. The + # teardown hooks may take 9 min: agent-container 2 after each test and 2 at the end, agent-supervisor 3 and + # agent-question 2. That is 62 min, plus setup. timeout-minutes: 75 steps: # Fail at once when the secret is missing, so a missing secret never looks like a passing run. diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 99eaa6b0..f3a115c9 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -21,8 +21,21 @@ one, and a matrix adds its values to the name. So do not rename the `test` or `real-docker` job, and do not give either one a `name` or a `strategy` (matrix): the required check would then never report, and no pull request could merge. `test/vitest-config.test.ts` fails if either job has any key other than -`runs-on`, `timeout-minutes` and `steps`, if its steps differ from the ones -this page lists, or if either workflow has another job. +`runs-on`, `timeout-minutes` and `steps`, or if either workflow has another +job. It also fails if a workflow has a top-level key other than the ones it has +now (`name`, `on`, `permissions`, and `concurrency` in `live-probes.yml` +only), so no workflow can add `defaults` or `env` for every job. + +The test pins each job's steps exactly, in this order: + +| Job | Steps | +| --- | --- | +| `test` | `actions/checkout@v4`; `actions/setup-node@v4` with Node 26.7.0 and the npm cache; `npm ci --ignore-scripts`; `npm run typecheck`; `npm test` with one `--exclude` for each Docker-backed file; `npx playwright install --with-deps chromium`; `npm run test:browser` | +| `real-docker` | `actions/checkout@v4`; `actions/setup-node@v4` with Node 26.7.0 and the npm cache; `npm ci --ignore-scripts`; `npm run typecheck`; `npm test` | +| `live-probes` | the token check; `actions/checkout@v4`; `actions/setup-node@v4` with Node 26.7.0 and the npm cache; `npm ci --ignore-scripts`; the live probe Vitest run; `node scripts/live-probes.ts check live-probes.json` | + +The `live-probes` job also may have only the keys `runs-on`, +`timeout-minutes` and `steps`, so it has no `defaults`, `env` or `strategy`. ### `test` @@ -97,10 +110,11 @@ Each `describe` around a gated test must be a plain `describe(...)` with a literal title. A title with expressions, or `describe.each`, `describe.for`, `describe.skip` or any other modifier, makes the test fail. The switch may only be the condition of an `if` block that holds only tests and -declarations, or of `it.runIf`. Such a gate must be where Vitest registers tests -as it loads the file: at the top level of the file or in a `describe` callback. -A gate in any other function, such as a helper that is never called, makes the -test fail. The test counts every occurrence of the name +variable declarations, or of `it.runIf`. Such a gate must be where Vitest always +registers tests as it loads the file: a statement directly in the body of the +file, or directly in the body of a `describe` callback that is itself such a +statement. A gate inside any other block or expression, such as another `if`, a +loop or a helper that is never called, makes the test fail. The test counts every occurrence of the name `CODEBOOST_RUN_AUTH_PROBES` in each probe file's text, comments and strings included, and fails unless each one is such a gate. It also fails if any other source file under `test/` names the switch: any `.ts`, `.tsx`, `.mts`, `.cts`, diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 7241a62a..871ca9de 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -42,6 +42,7 @@ describe('Vitest scheduling', () => { const workflow = (name: string) => parse(readFileSync(join(ROOT, '.github/workflows', name), 'utf8')) as { on: Record; + permissions: unknown; jobs: Record & { env?: unknown; steps: Array & { name?: string; run?: string; env?: Record }> }>; }; @@ -104,11 +105,12 @@ function assertNoCall(node: Node) { // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and // whether CODEBOOST_RUN_AUTH_PROBES enables it. Every `describe` around a gated test must be a plain `describe(...)` // with a literal title, or this throws: a title with expressions, or `describe.each`, `.for`, `.skip` or any other -// modifier, would make the full name differ from the one this computes. A gate counts only where Vitest registers tests while it loads the -// file: at the module's top level or in a `describe` callback, never in any other function body, so a gate in a -// helper that is never called throws. The switch may appear only as the exact condition of an +// modifier, would make the full name differ from the one this computes. A gate counts only where Vitest always +// registers its tests while it loads the file: as a statement directly in the module body or directly in the body of a +// plain `describe` callback that is itself such a statement. A gate inside any other block or expression, such as +// another `if`, a loop or a helper that is never called, throws. The switch may appear only as the exact condition of an // `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a -// declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the +// variable declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the // switch's name in the source text, comments and strings included, must be one of these gates, or this throws. A // variable name built at runtime from pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`, is not caught. function probeFileTests(file: string, source: string): ProbeFileTest[] { @@ -124,13 +126,14 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { const titles = describes.map(suite => 'title' in suite ? suite.title : '?'); tests.push({ name: title === undefined ? undefined : [...titles, title].join(' '), gated }); }; - // `loading` is true where code runs as Vitest loads the file: outside every function body except `describe` callbacks. + // `loading` is true only for a statement that always runs as Vitest loads the file, and for its expression: a statement + // directly in the module body or directly in the body of a `describe` callback that is itself such a statement. const visit = (node: Node, describes: Suite[], loading: boolean): void => { if (FUNCTIONS.includes(node.type)) { children(node).forEach(child => visit(child, describes, false)); return; } if (node.type === 'IfStatement' && mentionsSwitch(node.test as Node)) { const block = node.consequent as Node; if (!loading) - throw new Error(`A ${PROBE_SWITCH} gate outside module load or a describe callback at offset ${String(node.start)} in ${file}.`); + throw new Error(`A ${PROBE_SWITCH} gate that does not always run as Vitest loads the file at offset ${String(node.start)} in ${file}.`); if (!isSwitchOn(node.test) || block.type !== 'BlockStatement' || node.alternate !== null) throw new Error(`Unrecognized ${PROBE_SWITCH} condition at offset ${String(node.start)} in ${file}.`); gates += 1; @@ -138,10 +141,10 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; if (call && call.kind !== 'describe' && call.methods.length === 0) { record(describes, call.args[0], true); - call.args.slice(1).forEach(arg => visit(arg, describes, loading)); + call.args.slice(1).forEach(arg => visit(arg, describes, false)); } else if (statement.type === 'VariableDeclaration') { assertNoCall(statement); - visit(statement, describes, loading); + visit(statement, describes, false); } else throw new Error(`A gated block holds a statement that is not a test at offset ${String(statement.start)} in ${file}.`); } return; @@ -151,29 +154,34 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { const gate = call.kind !== 'describe' && call.methods.length === 1 && call.methods[0] === 'runIf' && call.innerArgs.length === 1 && call.innerArgs[0]!.length === 1 && isSwitchOn(call.innerArgs[0]![0]); if (gate && !loading) - throw new Error(`A ${PROBE_SWITCH} gate outside module load or a describe callback at offset ${String(node.start)} in ${file}.`); + throw new Error(`A ${PROBE_SWITCH} gate that does not always run as Vitest loads the file at offset ${String(node.start)} in ${file}.`); if (gate) gates += 1; - else call.innerArgs.flat().forEach(arg => visit(arg, describes, loading)); + else call.innerArgs.flat().forEach(arg => visit(arg, describes, false)); if (call.kind === 'describe') { const title = literalText(call.args[0]); const suite: Suite = call.methods.length > 0 ? { problem: `with the modifier .${call.methods.join('.')} at offset ${String(node.start)}` } : title === undefined ? { problem: `without a literal title at offset ${String(node.start)}` } : { title }; const inner = [...describes, suite]; - // Vitest calls a describe callback while it loads the file, so the callback's body keeps `loading`. + // Vitest calls a describe callback while it loads the file, so the statements directly in the callback's body keep + // `loading`. Everything else in the arguments does not. for (const arg of call.args) { - if (FUNCTIONS.includes(arg.type)) children(arg).forEach(child => visit(child, inner, loading)); - else visit(arg, inner, loading); + const body = arg.body as Node | undefined; + if (!FUNCTIONS.includes(arg.type) || body?.type !== 'BlockStatement') { visit(arg, inner, false); continue; } + (arg.params as Node[]).forEach(param => visit(param, inner, false)); + (body.body as Node[]).forEach(statement => visit(statement, inner, loading)); } return; } record(describes, call.args[0], gate); - call.args.forEach(arg => visit(arg, describes, loading)); + call.args.forEach(arg => visit(arg, describes, false)); return; } if (isSwitchName(node)) throw new Error(`Unrecognized use of ${PROBE_SWITCH} at offset ${String(node.start)} in ${file}.`); - children(node).forEach(child => visit(child, describes, loading)); + // Only the module body and an expression statement pass `loading` on to their direct children. + const keep = loading && (node.type === 'Program' || node.type === 'ExpressionStatement'); + children(node).forEach(child => visit(child, describes, keep)); }; visit(program as unknown as Node, [], true); const occurrences = source.split(PROBE_SWITCH).length - 1; @@ -227,6 +235,17 @@ describe('CI workflows', () => { expect(Object.keys(job).toSorted()).toEqual(['runs-on', 'steps', 'timeout-minutes']); }); + it('has no workflow keys other than the ones it uses now', () => { + // A workflow-level `defaults` or `env`, such as `defaults: { run: { shell: 'true {0}' } }`, would change every + // `run:` step of every job, for example into a no-op that passes. + const top = (name: string) => Object.keys(parse(readFileSync(join(ROOT, '.github/workflows', name), 'utf8')) as object); + expect(top('ci.yml')).toEqual(['name', 'on', 'permissions', 'jobs']); + expect(top('agent-isolation.yml')).toEqual(['name', 'on', 'permissions', 'jobs']); + expect(top('live-probes.yml')).toEqual(['name', 'on', 'permissions', 'concurrency', 'jobs']); + for (const name of ['ci.yml', 'agent-isolation.yml', 'live-probes.yml']) + expect(workflow(name).permissions, name).toEqual({ contents: 'read' }); + }); + it('uses the same Node version in every workflow', () => { const versions = ['ci.yml', 'agent-isolation.yml', 'live-probes.yml'].flatMap(name => Object.values(workflow(name).jobs) .flatMap(job => job.steps.filter(step => String(step.uses).startsWith('actions/setup-node@')) @@ -237,8 +256,20 @@ describe('CI workflows', () => { it('runs only the live Claude probes, by hand or nightly, never on a pull request or push', () => { const live = workflow('live-probes.yml'); expect(Object.keys(live.on).toSorted()).toEqual(['schedule', 'workflow_dispatch']); + expect(Object.keys(live.jobs)).toEqual(['live-probes']); const job = live.jobs['live-probes']!; - expect(job.env).toBeUndefined(); + // No `defaults`, `env`, `strategy` or `container`: any of them could change what the steps run. + expect(Object.keys(job).toSorted()).toEqual(['runs-on', 'steps', 'timeout-minutes']); + expect(job).toMatchObject({ 'runs-on': 'ubuntu-latest', 'timeout-minutes': 75 }); + const token = { CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }; + expect(job.steps.map(step => Object.keys(step).toSorted())).toEqual([['env', 'name', 'run'], ['uses'], ['uses', 'with'], + ['run'], ['env', 'name', 'run'], ['name', 'run']]); + expect(job.steps.slice(1, 4)).toEqual([...SETUP, { run: 'npm ci --ignore-scripts' }]); + expect(job.steps[0]).toMatchObject({ name: 'Require the Claude token', env: token }); + expect(job.steps[4]).toEqual({ name: 'Run the live Claude probes', env: { CODEBOOST_RUN_AUTH_PROBES: '1', ...token }, + run: `npx vitest run --project docker -t '${MARKER_FILTER}' --reporter=default --reporter=json ` + + `--outputFile=live-probes.json ${LIVE_PROBE_FILES.join(' ')}` }); + expect(job.steps[5]).toEqual({ name: 'Check that every probe passed', run: 'node scripts/live-probes.ts check live-probes.json' }); const vitest = job.steps.filter(step => step.run?.startsWith('npx vitest')); expect(vitest).toHaveLength(1); const words = vitest[0]!.run!.split(/\s+/); From 125f5fbcd30c99ab5620d06a1cfc6dfdb442e5f1 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:15:45 -0700 Subject: [PATCH 11/15] Tighten gated-block declarations, pin the token step and correct the teardown comment - A gated if block may declare only const or let names with a function expression or a constant value; a var or an assignment such as (on = true) now fails the config test. - Pin the live-probes token step's run text exactly. - Say the teardown figures are Vitest's limits, that the afterAll docker calls have no timeout of their own, and that the job limit caps the run. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/live-probes.yml | 6 ++++-- docs/implementation/ci.md | 14 +++++++++--- test/vitest-config.test.ts | 36 ++++++++++++++++++++----------- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index c96fd3a7..78e18b52 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -15,8 +15,10 @@ jobs: runs-on: ubuntu-latest # The probes' own time limits total 33 min (6, 8, 8 and 11). agent-container and agent-supervisor may each spend # up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. The - # teardown hooks may take 9 min: agent-container 2 after each test and 2 at the end, agent-supervisor 3 and - # agent-question 2. That is 62 min, plus setup. + # teardown hooks have Vitest limits that total 9 min: agent-container 2 after each test and 2 at the end, + # agent-supervisor 3 and agent-question 2. That is 62 min, plus setup. These are only Vitest's limits: the afterAll + # hooks in agent-container and agent-question call docker through spawnSync with no timeout of their own, and + # Vitest's hook timer cannot fire while such a call blocks. The 75 min job limit caps the run. timeout-minutes: 75 steps: # Fail at once when the secret is missing, so a missing secret never looks like a passing run. diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index f3a115c9..3b8f9bfa 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -26,7 +26,9 @@ job. It also fails if a workflow has a top-level key other than the ones it has now (`name`, `on`, `permissions`, and `concurrency` in `live-probes.yml` only), so no workflow can add `defaults` or `env` for every job. -The test pins each job's steps exactly, in this order: +The test pins each job's steps exactly, in this order, including each step's +`run` text. It also runs the `live-probes` token check with an empty, a missing +and a set token. | Job | Steps | | --- | --- | @@ -109,8 +111,14 @@ parses each probe file. It fails unless the tests that Each `describe` around a gated test must be a plain `describe(...)` with a literal title. A title with expressions, or `describe.each`, `describe.for`, `describe.skip` or any other modifier, makes the test fail. -The switch may only be the condition of an `if` block that holds only tests and -variable declarations, or of `it.runIf`. Such a gate must be where Vitest always +The switch may only be the condition of an `if` block without `else`, or of +`it.runIf`. Each statement in such an `if` block must be an `it` or `test` call +with a literal title, or a `const` or `let` declaration of plain names. Each +name must have a plain value: a function expression, or a constant. A constant +is a string, number or boolean literal, a template literal whose expressions are +constants, or a binary operation on constants. Any other value, such as a call +or an assignment like `(on = true)`, makes the test fail, and so does a `var`, +because it is visible outside the block. Such a gate must be where Vitest always registers tests as it loads the file: a statement directly in the body of the file, or directly in the body of a `describe` callback that is itself such a statement. A gate inside any other block or expression, such as another `if`, a diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 871ca9de..0837964c 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -93,13 +93,21 @@ function suiteCall(node: Node) { if (callee.type !== 'Identifier' || !['it', 'test', 'describe'].includes(callee.name as string)) return undefined; return { kind: callee.name as string, methods, innerArgs, args: node.arguments as Node[] }; } -// Throws unless the node has no call outside a function body: a gated block may declare helpers for its tests, but -// any call at block level could register a test that this check does not see. -function assertNoCall(node: Node) { - if (FUNCTIONS.includes(node.type)) return; - if (['CallExpression', 'NewExpression', 'TaggedTemplateExpression', 'AwaitExpression'].includes(node.type)) - throw new Error(`A gated block runs a ${node.type} outside a test at offset ${String(node.start)}.`); - children(node).forEach(assertNoCall); +// A value that a gated block may give a variable: a function expression, which runs only when called, or a constant: a +// string, number or boolean literal, a template literal whose expressions are constants, or a binary operation on +// constants. Nothing else is allowed: a call could register a test this check does not see, and an assignment or +// update, such as `(on = true)`, could turn on a test outside the gate. +const isConstant = (node: Node): boolean => (node.type === 'Literal' && ['string', 'number', 'boolean'].includes(typeof node.value)) + || (node.type === 'TemplateLiteral' && (node.expressions as Node[]).every(isConstant)) + || (node.type === 'BinaryExpression' && isConstant(node.left as Node) && isConstant(node.right as Node)); +const isPlainValue = (node: Node) => ['ArrowFunctionExpression', 'FunctionExpression'].includes(node.type) || isConstant(node); +// Throws unless the statement is a `const` or `let` declaration of plain names, each with a plain value. A `var` is +// refused because it is visible outside the block. +function assertPlainDeclaration(statement: Node, file: string) { + const declarations = statement.declarations as Array<{ id: Node; init: Node | null }>; + if (!['const', 'let'].includes(statement.kind as string) || !declarations.every(declaration => + declaration.id.type === 'Identifier' && declaration.init !== null && isPlainValue(declaration.init))) + throw new Error(`A gated block declares a variable that is not a const or let with a plain value at offset ${String(statement.start)} in ${file}.`); } // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and @@ -110,9 +118,10 @@ function assertNoCall(node: Node) { // plain `describe` callback that is itself such a statement. A gate inside any other block or expression, such as // another `if`, a loop or a helper that is never called, throws. The switch may appear only as the exact condition of an // `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a -// variable declaration without a call, or of `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the -// switch's name in the source text, comments and strings included, must be one of these gates, or this throws. A -// variable name built at runtime from pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`, is not caught. +// `const` or `let` declaration of plain names with plain values (see `isPlainValue`), or of +// `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the switch's name in the source text, +// comments and strings included, must be one of these gates, or this throws. A variable name built at runtime from +// pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`, is not caught. function probeFileTests(file: string, source: string): ProbeFileTest[] { const { program, errors } = parseSync(file, source); if (errors.length) throw new Error(`${file} does not parse: ${errors[0]!.message}`); @@ -143,7 +152,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { record(describes, call.args[0], true); call.args.slice(1).forEach(arg => visit(arg, describes, false)); } else if (statement.type === 'VariableDeclaration') { - assertNoCall(statement); + assertPlainDeclaration(statement, file); visit(statement, describes, false); } else throw new Error(`A gated block holds a statement that is not a test at offset ${String(statement.start)} in ${file}.`); } @@ -265,7 +274,10 @@ describe('CI workflows', () => { expect(job.steps.map(step => Object.keys(step).toSorted())).toEqual([['env', 'name', 'run'], ['uses'], ['uses', 'with'], ['run'], ['env', 'name', 'run'], ['name', 'run']]); expect(job.steps.slice(1, 4)).toEqual([...SETUP, { run: 'npm ci --ignore-scripts' }]); - expect(job.steps[0]).toMatchObject({ name: 'Require the Claude token', env: token }); + expect(job.steps[0]).toEqual({ name: 'Require the Claude token', env: token, + run: 'if [ -z "$CLAUDE_CODE_OAUTH_TOKEN" ]; then\n' + + ' echo "::error::The repository secret CLAUDE_CODE_OAUTH_TOKEN is not set. Add it to run the live probes."\n' + + ' exit 1\nfi\n' }); expect(job.steps[4]).toEqual({ name: 'Run the live Claude probes', env: { CODEBOOST_RUN_AUTH_PROBES: '1', ...token }, run: `npx vitest run --project docker -t '${MARKER_FILTER}' --reporter=default --reporter=json ` + `--outputFile=live-probes.json ${LIVE_PROBE_FILES.join(' ')}` }); From fd850313b0c74cbf9acea6d762f479a59be485e6 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:21:18 -0700 Subject: [PATCH 12/15] Pin the Vitest projects exactly and refuse option objects and computed titles in probe files The scheduling test now compares the whole Vitest config, so an extra key such as an exclude fails it. Gated tests and the describes around them must take exactly a literal title, a function and an optional timeout. Every it/test/describe title in a probe file must be a literal, so the marker check sees each one. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 20 +++++++---- test/vitest-config.test.ts | 71 ++++++++++++++++++++++---------------- 2 files changed, 56 insertions(+), 35 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 3b8f9bfa..5a6a7a91 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -60,7 +60,10 @@ anything is allocated. It passes with no Docker binary on `PATH`. `real-docker` runs `npm test` with no excludes. `vitest.config.ts` runs the ordinary files first. Then it runs the Docker-backed files one at a time, -because they share one image tag and one Docker daemon. This job is the T9 +because they share one image tag and one Docker daemon. `test/vitest-config.test.ts` +pins the whole of `vitest.config.ts`, so it fails if either project gets any +other key, such as an `exclude`, `testNamePattern` or `passWithNoTests` that +could drop files or tests from both jobs. This job is the T9 Docker gate. [Agent isolation](agent-isolation.md#what-the-gate-proves) lists what it proves. @@ -108,12 +111,17 @@ update `LIVE_PROBES` in `scripts/live-probes.ts`. `test/vitest-config.test.ts` parses each probe file. It fails unless the tests that `CODEBOOST_RUN_AUTH_PROBES` enables have exactly the full names in the list: the `describe` titles and the test title, joined with spaces, as Vitest names them. -Each `describe` around a gated test must be a plain `describe(...)` with a -literal title. A title with expressions, or `describe.each`, `describe.for`, -`describe.skip` or any other modifier, makes the test fail. +Every `it`, `test` and `describe` call in a probe file must have a string +literal or a template literal without expressions as its title, or the test +fails. So the test sees every title, and fails if a test that the switch does +not enable has a title that ends with the marker. Each `describe` around a +gated test, and each gated `it` or `test` call, must have exactly a literal +title, a function expression and optionally a numeric timeout. +`describe.each`, `describe.for`, `describe.skip` or any other modifier, or an +options object such as `{ skip: true }`, makes the test fail. The switch may only be the condition of an `if` block without `else`, or of -`it.runIf`. Each statement in such an `if` block must be an `it` or `test` call -with a literal title, or a `const` or `let` declaration of plain names. Each +`it.runIf`. Each statement in such an `if` block must be a plain `it` or `test` +call, or a `const` or `let` declaration of plain names. Each name must have a plain value: a function expression, or a constant. A constant is a string, number or boolean literal, a template literal whose expressions are constants, or a binary operation on constants. Any other value, such as a call diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 0837964c..aa789acc 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -22,19 +22,15 @@ const DOCKER_TESTS = [ describe('Vitest scheduling', () => { it('finishes ordinary tests before running Docker-backed files one at a time', () => { - const projects = (config as { test?: { projects?: Array<{ test?: Record }> } }).test?.projects; - expect(projects).toHaveLength(2); - expect(projects?.[0]?.test).toMatchObject({ - name: 'parallel', - include: ['test/*.test.ts'], - exclude: DOCKER_TESTS, - sequence: { groupOrder: 0 }, - }); - expect(projects?.[1]?.test).toMatchObject({ - name: 'docker', - include: DOCKER_TESTS, - fileParallelism: false, - sequence: { groupOrder: 1 }, + // The whole config, exactly: any other key, such as an `exclude`, `testNamePattern` or `passWithNoTests` in a + // project, could drop files or tests from both CI jobs while every job still passes. + expect(config).toEqual({ + test: { + projects: [ + { test: { name: 'parallel', include: ['test/*.test.ts'], exclude: DOCKER_TESTS, sequence: { groupOrder: 0 } } }, + { test: { name: 'docker', include: DOCKER_TESTS, fileParallelism: false, sequence: { groupOrder: 1 } } }, + ], + }, }); }); }); @@ -54,7 +50,7 @@ const SETUP = [{ uses: 'actions/checkout@v4' }, const runs = (steps: Array<{ run?: string }>) => steps.flatMap(step => step.run === undefined ? [] : [step.run]); type Node = { type: string; [key: string]: unknown }; -type ProbeFileTest = { name: string | undefined; gated: boolean }; +type ProbeFileTest = { name: string; gated: boolean }; // An enclosing `describe`: its literal title, or why its title is not known. type Suite = { title: string } | { problem: string }; const PROBE_SWITCH = 'CODEBOOST_RUN_AUTH_PROBES'; @@ -101,6 +97,13 @@ const isConstant = (node: Node): boolean => (node.type === 'Literal' && ['string || (node.type === 'TemplateLiteral' && (node.expressions as Node[]).every(isConstant)) || (node.type === 'BinaryExpression' && isConstant(node.left as Node) && isConstant(node.right as Node)); const isPlainValue = (node: Node) => ['ArrowFunctionExpression', 'FunctionExpression'].includes(node.type) || isConstant(node); +// A timeout: a number literal, or a binary operation on numbers, such as `22 * 60_000`. +const isNumber = (node: Node): boolean => (node.type === 'Literal' && typeof node.value === 'number') + || (node.type === 'BinaryExpression' && isNumber(node.left as Node) && isNumber(node.right as Node)); +// Exactly (literal title, function expression[, timeout]). An options object, such as `{ skip: true }`, could skip or +// change the test, so it is refused. +const isPlainSuiteArgs = (args: Node[]) => (args.length === 2 || (args.length === 3 && isNumber(args[2]!))) + && literalText(args[0]) !== undefined && ['ArrowFunctionExpression', 'FunctionExpression'].includes(args[1]!.type); // Throws unless the statement is a `const` or `let` declaration of plain names, each with a plain value. A `var` is // refused because it is visible outside the block. function assertPlainDeclaration(statement: Node, file: string) { @@ -111,15 +114,19 @@ function assertPlainDeclaration(statement: Node, file: string) { } // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and -// whether CODEBOOST_RUN_AUTH_PROBES enables it. Every `describe` around a gated test must be a plain `describe(...)` -// with a literal title, or this throws: a title with expressions, or `describe.each`, `.for`, `.skip` or any other -// modifier, would make the full name differ from the one this computes. A gate counts only where Vitest always +// whether CODEBOOST_RUN_AUTH_PROBES enables it. Every `it`, `test` or `describe` call in the file, with or without a +// modifier, must have a string literal or a template literal without expressions as its title, or this throws, so +// that the caller can check every title for the marker. Every `describe` around a gated test must be a plain +// `describe(...)` with exactly a literal title, a function expression and optionally a numeric timeout, or this +// throws: a modifier such as `describe.each`, `.for` or `.skip`, or an options object such as `{ skip: true }`, could +// skip the test or make its full name differ from the one this computes. A gated test call must have the same +// arguments. A gate counts only where Vitest always // registers its tests while it loads the file: as a statement directly in the module body or directly in the body of a // plain `describe` callback that is itself such a statement. A gate inside any other block or expression, such as // another `if`, a loop or a helper that is never called, throws. The switch may appear only as the exact condition of an -// `if (...) { ... }` block without `else`, whose statements are each an `it`/`test` call with a literal title or a +// `if (...) { ... }` block without `else`, whose statements are each a plain `it`/`test` call or a // `const` or `let` declaration of plain names with plain values (see `isPlainValue`), or of -// `it.runIf(...)`/`test.runIf(...)` with a literal title. Every occurrence of the switch's name in the source text, +// `it.runIf(...)`/`test.runIf(...)`. Every occurrence of the switch's name in the source text, // comments and strings included, must be one of these gates, or this throws. A variable name built at runtime from // pieces, such as `process.env['CODEBOOST_' + 'RUN_AUTH_PROBES']`, is not caught. function probeFileTests(file: string, source: string): ProbeFileTest[] { @@ -127,13 +134,15 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { if (errors.length) throw new Error(`${file} does not parse: ${errors[0]!.message}`); const tests: ProbeFileTest[] = []; let gates = 0; - const record = (describes: Suite[], titleNode: unknown, gated: boolean) => { - const title = literalText(titleNode); - if (gated && title === undefined) throw new Error(`Gated test without a literal title in ${file}.`); + const record = (describes: Suite[], args: Node[], gated: boolean, offset: unknown) => { + const title = literalText(args[0]); + if (title === undefined) throw new Error(`A test without a literal title at offset ${String(offset)} in ${file}.`); + if (gated && !isPlainSuiteArgs(args)) + throw new Error(`A gated test whose arguments are not a literal title, a function and a timeout at offset ${String(offset)} in ${file}.`); const unknown = describes.find(suite => 'problem' in suite); if (gated && unknown) throw new Error(`Gated test in ${file} inside a describe ${unknown.problem}.`); const titles = describes.map(suite => 'title' in suite ? suite.title : '?'); - tests.push({ name: title === undefined ? undefined : [...titles, title].join(' '), gated }); + tests.push({ name: [...titles, title].join(' '), gated }); }; // `loading` is true only for a statement that always runs as Vitest loads the file, and for its expression: a statement // directly in the module body or directly in the body of a `describe` callback that is itself such a statement. @@ -149,7 +158,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { for (const statement of block.body as Node[]) { const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; if (call && call.kind !== 'describe' && call.methods.length === 0) { - record(describes, call.args[0], true); + record(describes, call.args, true, statement.start); call.args.slice(1).forEach(arg => visit(arg, describes, false)); } else if (statement.type === 'VariableDeclaration') { assertPlainDeclaration(statement, file); @@ -168,9 +177,12 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { else call.innerArgs.flat().forEach(arg => visit(arg, describes, false)); if (call.kind === 'describe') { const title = literalText(call.args[0]); + if (title === undefined) throw new Error(`A describe without a literal title at offset ${String(node.start)} in ${file}.`); const suite: Suite = call.methods.length > 0 ? { problem: `with the modifier .${call.methods.join('.')} at offset ${String(node.start)}` } - : title === undefined ? { problem: `without a literal title at offset ${String(node.start)}` } : { title }; + : !isPlainSuiteArgs(call.args) + ? { problem: `whose arguments are not a literal title, a function and a timeout at offset ${String(node.start)}` } + : { title }; const inner = [...describes, suite]; // Vitest calls a describe callback while it loads the file, so the statements directly in the callback's body keep // `loading`. Everything else in the arguments does not. @@ -182,7 +194,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { } return; } - record(describes, call.args[0], gate); + record(describes, call.args, gate, node.start); call.args.forEach(arg => visit(arg, describes, false)); return; } @@ -321,16 +333,17 @@ describe('CI workflows', () => { it('names every live probe exactly as its test file titles it', () => { // The tests that the probe switch enables are exactly the listed probes, by Vitest full name, and every one ends in - // the suffix the workflow's -t filter selects. No other test in the file carries the suffix. + // the suffix the workflow's -t filter selects. No other test in the file carries the suffix: every title in the file + // is a literal, or probeFileTests throws, so this sees each one. for (const file of LIVE_PROBE_FILES) { const tests = probeFileTests(file, readFileSync(join(ROOT, file), 'utf8')); - const gated = tests.filter(test => test.gated).map(test => test.name!); + const gated = tests.filter(test => test.gated).map(test => test.name); expect(gated.toSorted(), file).toEqual(LIVE_PROBES.filter(probe => probe.file === file).map(probe => probe.name).toSorted()); for (const name of gated) { expect(name.endsWith(` ${LIVE_PROBE_MARKER}`), name).toBe(true); expect(name, file).toMatch(new RegExp(MARKER_FILTER)); } - expect(tests.filter(test => !test.gated && test.name !== undefined && new RegExp(MARKER_FILTER).test(test.name)), file) + expect(tests.filter(test => !test.gated && new RegExp(MARKER_FILTER).test(test.name)), file) .toEqual([]); } // Every JavaScript or TypeScript source file under test/ except this one, helpers and fixtures included, since a From 9a69e87565fe29413f68d2e7ca51c09b72db0db2 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:25:51 -0700 Subject: [PATCH 13/15] Refuse probe-file test calls that reach it/test/describe through unmodeled links A call such as it.each`table`('title', fn) has a tagged template in its callee chain, so the title check skipped it. suiteCall now throws when the chain reaches it, test or describe through a tagged template, computed member, parentheses or a TypeScript wrapper. it.each([...])(title, fn) stays modeled with the literal title check. The comment and ci.md now state exactly what is enforced. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 9 +++++++-- test/vitest-config.test.ts | 34 +++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 5a6a7a91..847fb755 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -113,8 +113,13 @@ parses each probe file. It fails unless the tests that `describe` titles and the test title, joined with spaces, as Vitest names them. Every `it`, `test` and `describe` call in a probe file must have a string literal or a template literal without expressions as its title, or the test -fails. So the test sees every title, and fails if a test that the switch does -not enable has a title that ends with the marker. Each `describe` around a +fails. The call must reach `it`, `test` or `describe` through plain calls and +plain property names only, such as `it.skip(...)` or `it.each([...])(...)`. +Any other form, such as ``it.each`table`(...)``, `it['skip'](...)` or +`(it as T)(...)`, makes the test fail. A call through another name, such as +`const t = it; t(...)`, is not checked. So the test sees the title of every +such call, and fails if a test that the switch does not enable has a title that +ends with the marker. Each `describe` around a gated test, and each gated `it` or `test` call, must have exactly a literal title, a function expression and optionally a numeric timeout. `describe.each`, `describe.for`, `describe.skip` or any other modifier, or an diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index aa789acc..4688b1d6 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -74,9 +74,17 @@ const literalText = (node: unknown) => !isNode(node) ? undefined // The switch as an identifier, a string literal or a template literal without expressions. const isSwitchName = (node: Node) => (node.type === 'Identifier' && node.name === PROBE_SWITCH) || literalText(node) === PROBE_SWITCH; const mentionsSwitch = (node: Node): boolean => isSwitchName(node) || children(node).some(mentionsSwitch); -// A call whose callee chain starts at `it`, `test` or `describe`, such as `it.runIf(...)('title', ...)`: the kind, the -// methods and arguments of the inner calls, and the outer call's arguments. -function suiteCall(node: Node) { +// Links that can lead from a callee to `it`, `test` or `describe` but that `suiteCall` does not model, such as the tag +// in ``it.each`table`(...)``, a computed member such as `it['skip']`, a non-null assertion or parentheses. +const UNMODELED_LINKS: Record = { TaggedTemplateExpression: 'tag', MemberExpression: 'object', + CallExpression: 'callee', ChainExpression: 'expression', ParenthesizedExpression: 'expression', + TSNonNullExpression: 'expression', TSAsExpression: 'expression', TSSatisfiesExpression: 'expression', + TSTypeAssertion: 'expression', TSInstantiationExpression: 'expression' }; +// A call whose callee chain starts at `it`, `test` or `describe` through plain calls and non-computed members only, such +// as `it.runIf(...)('title', ...)` or `it.each([...])('title', ...)`: the kind, the methods and arguments of the inner +// calls, and the outer call's arguments. Throws if the chain reaches `it`, `test` or `describe` through any other link +// (see `UNMODELED_LINKS`), because the caller could not check that call's title. +function suiteCall(node: Node, file: string) { if (node.type !== 'CallExpression') return undefined; const methods: string[] = [], innerArgs: Node[][] = []; let callee = node.callee as Node; @@ -86,7 +94,13 @@ function suiteCall(node: Node) { && callee.property.type === 'Identifier') { methods.unshift(callee.property.name as string); callee = callee.object as Node; } else break; } - if (callee.type !== 'Identifier' || !['it', 'test', 'describe'].includes(callee.name as string)) return undefined; + if (callee.type !== 'Identifier' || !['it', 'test', 'describe'].includes(callee.name as string)) { + for (let link: unknown = callee; isNode(link); link = link[UNMODELED_LINKS[link.type] ?? '']) { + if (link.type === 'Identifier' && ['it', 'test', 'describe'].includes(link.name as string)) + throw new Error(`A call that reaches ${String(link.name)} through ${callee.type} at offset ${String(node.start)} in ${file}.`); + } + return undefined; + } return { kind: callee.name as string, methods, innerArgs, args: node.arguments as Node[] }; } // A value that a gated block may give a variable: a function expression, which runs only when called, or a constant: a @@ -114,9 +128,11 @@ function assertPlainDeclaration(statement: Node, file: string) { } // Every test in a probe file with its Vitest full name (describe titles and the test title, joined with spaces), and -// whether CODEBOOST_RUN_AUTH_PROBES enables it. Every `it`, `test` or `describe` call in the file, with or without a -// modifier, must have a string literal or a template literal without expressions as its title, or this throws, so -// that the caller can check every title for the marker. Every `describe` around a gated test must be a plain +// whether CODEBOOST_RUN_AUTH_PROBES enables it. Every call whose callee chain reaches `it`, `test` or `describe` must +// reach it through plain calls and non-computed members only, such as `it.skip(...)` or `it.each([...])(...)`, or this +// throws: ``it.each`table`(...)``, `it['skip'](...)` and `(it as T)(...)` throw. Each such call must have a string +// literal or a template literal without expressions as its title, or this throws, so that the caller can check every +// title for the marker. A call through an alias, such as `const t = it; t(...)`, is not seen. Every `describe` around a gated test must be a plain // `describe(...)` with exactly a literal title, a function expression and optionally a numeric timeout, or this // throws: a modifier such as `describe.each`, `.for` or `.skip`, or an options object such as `{ skip: true }`, could // skip the test or make its full name differ from the one this computes. A gated test call must have the same @@ -156,7 +172,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { throw new Error(`Unrecognized ${PROBE_SWITCH} condition at offset ${String(node.start)} in ${file}.`); gates += 1; for (const statement of block.body as Node[]) { - const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node) : undefined; + const call = statement.type === 'ExpressionStatement' ? suiteCall(statement.expression as Node, file) : undefined; if (call && call.kind !== 'describe' && call.methods.length === 0) { record(describes, call.args, true, statement.start); call.args.slice(1).forEach(arg => visit(arg, describes, false)); @@ -167,7 +183,7 @@ function probeFileTests(file: string, source: string): ProbeFileTest[] { } return; } - const call = suiteCall(node); + const call = suiteCall(node, file); if (call) { const gate = call.kind !== 'describe' && call.methods.length === 1 && call.methods[0] === 'runIf' && call.innerArgs.length === 1 && call.innerArgs[0]!.length === 1 && isSwitchOn(call.innerArgs[0]![0]); From f615b25525e146156078606538b4152efc03c1cd Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:29:09 -0700 Subject: [PATCH 14/15] Pin the package scripts both gating jobs call Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 4 +++- test/vitest-config.test.ts | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 847fb755..83382ddd 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -63,7 +63,9 @@ ordinary files first. Then it runs the Docker-backed files one at a time, because they share one image tag and one Docker daemon. `test/vitest-config.test.ts` pins the whole of `vitest.config.ts`, so it fails if either project gets any other key, such as an `exclude`, `testNamePattern` or `passWithNoTests` that -could drop files or tests from both jobs. This job is the T9 +could drop files or tests from both jobs. It also pins the `test`, `typecheck` +and `test:browser` scripts in `package.json` that the jobs call, so a filter +added there fails it too. This job is the T9 Docker gate. [Agent isolation](agent-isolation.md#what-the-gate-proves) lists what it proves. diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 4688b1d6..1201b4ab 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -283,6 +283,13 @@ describe('CI workflows', () => { expect(workflow(name).permissions, name).toEqual({ contents: 'read' }); }); + it('runs the package scripts the gating jobs call exactly as written here', () => { + // Both required jobs call these scripts; a filter, -t, --project or --passWithNoTests here would narrow them silently. + const scripts = (JSON.parse(readFileSync(join(ROOT, 'package.json'), 'utf8')) as { scripts: Record }).scripts; + expect({ test: scripts.test, typecheck: scripts.typecheck, 'test:browser': scripts['test:browser'] }) + .toEqual({ test: 'vitest run', typecheck: 'tsc --noEmit', 'test:browser': 'playwright test' }); + }); + it('uses the same Node version in every workflow', () => { const versions = ['ci.yml', 'agent-isolation.yml', 'live-probes.yml'].flatMap(name => Object.values(workflow(name).jobs) .flatMap(job => job.steps.filter(step => String(step.uses).startsWith('actions/setup-node@')) From bdd9bac5a0c9deb59daad4c5855e4725dad47f37 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 18 Oct 2026 01:35:10 -0700 Subject: [PATCH 15/15] Refuse test.only in browser specs and pin the Playwright config Set forbidOnly: true in playwright.config.ts and pin the whole config in test/vitest-config.test.ts, so a committed .only, a grep or a narrower testDir fails the test job. In ci.md, state that only top-level test/*.test.ts files run, and that the static probe-title check does not see a marker added by a .each/.for placeholder (the live-probes report check refuses that test). Co-Authored-By: Claude Opus 5.5 --- docs/implementation/ci.md | 23 ++++++++++++++++++++--- playwright.config.ts | 4 +++- test/vitest-config.test.ts | 19 +++++++++++++++++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docs/implementation/ci.md b/docs/implementation/ci.md index 83382ddd..b10e663f 100644 --- a/docs/implementation/ci.md +++ b/docs/implementation/ci.md @@ -8,7 +8,7 @@ settings are not stored in the repository. | Workflow file | Job (check name) | Runs on | What it runs | | --- | --- | --- | --- | -| `.github/workflows/ci.yml` | `test` | every pull request; pushes to `main` and `codex/**` | Type check; every Vitest file except the Docker-backed files; the Playwright browser suite | +| `.github/workflows/ci.yml` | `test` | every pull request; pushes to `main` and `codex/**` | Type check; every top-level `test/*.test.ts` file except the Docker-backed files; the Playwright browser suite | | `.github/workflows/agent-isolation.yml` | `real-docker` | every pull request; pushes to `main` and `codex/**` | Type check; the complete Vitest suite against real Docker | | `.github/workflows/live-probes.yml` | `live-probes` | started by hand (`workflow_dispatch`); every night at 06:23 UTC | The four live Claude startup probes only | @@ -48,6 +48,19 @@ recent run took 20 minutes. It excludes the Docker-backed files with `vitest.config.ts`. The excludes must equal that list: `test/vitest-config.test.ts` fails if they differ. +Vitest runs only the top-level `test/*.test.ts` files: the `include` of both +projects in `vitest.config.ts` is `test/*.test.ts` or the Docker-backed list, so +a test file in a subfolder of `test/` does not run in any job. +`test/vitest-config.test.ts` pins the whole Vitest config, so a change to the +`include` makes it fail. + +The browser suite runs every spec in `test/browser/`. `playwright.config.ts` +sets `forbidOnly: true`, so Playwright refuses to run when a spec has a +`test.only` or `describe.only`, locally as well as in CI. Without it, a committed +`.only` would make the job run only that test and pass. `test/vitest-config.test.ts` +pins the whole Playwright config, so it fails if `forbidOnly` is removed or a +`grep`, `testMatch`, `testIgnore` or narrower `testDir` is added. + When a test file starts to use Docker, add it to `dockerTests` in `vitest.config.ts`, to `DOCKER_TESTS` in `test/vitest-config.test.ts` and to the excludes in `ci.yml`. @@ -120,8 +133,12 @@ plain property names only, such as `it.skip(...)` or `it.each([...])(...)`. Any other form, such as ``it.each`table`(...)``, `it['skip'](...)` or `(it as T)(...)`, makes the test fail. A call through another name, such as `const t = it; t(...)`, is not checked. So the test sees the title of every -such call, and fails if a test that the switch does not enable has a title that -ends with the marker. Each `describe` around a +such call, and fails if a test that the switch does not enable has a literal +title that ends with the marker. The test reads a `.each` or `.for` title before +Vitest fills its placeholders, so it does not see a marker that a placeholder +adds, as in `it.each(['(live Claude)'])('sneaky %s', ...)`. The `-t` filter of +the `live-probes` job selects such a test, and the report check then fails the +job, because a test that is not a probe ran. Each `describe` around a gated test, and each gated `it` or `test` call, must have exactly a literal title, a function expression and optionally a numeric timeout. `describe.each`, `describe.for`, `describe.skip` or any other modifier, or an diff --git a/playwright.config.ts b/playwright.config.ts index 08f130b7..b0132b41 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -1,4 +1,6 @@ import { defineConfig } from '@playwright/test'; // The review server runs in the test process. Its review loads no longer block that process, so test and assertion // timeouts now include the server's real Git work (a merge click runs several review loads); allow for it. -export default defineConfig({ testDir: './test/browser', fullyParallel: false, workers: 1, timeout: 60_000, expect: { timeout: 15_000 }, use: { browserName:'chromium', viewport:{width:1512,height:982}, trace:'retain-on-failure' } }); +// `forbidOnly` makes Playwright refuse a `test.only` (or `describe.only`) left in a spec, which would otherwise run +// only that test and let the CI `test` job pass. `test/vitest-config.test.ts` pins this whole config. +export default defineConfig({ testDir: './test/browser', forbidOnly: true, fullyParallel: false, workers: 1, timeout: 60_000, expect: { timeout: 15_000 }, use: { browserName:'chromium', viewport:{width:1512,height:982}, trace:'retain-on-failure' } }); diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index 1201b4ab..d010c604 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -6,6 +6,7 @@ import { describe, expect, it } from 'vitest'; import { parseSync } from 'vite'; import { parse } from 'yaml'; import { checkLiveProbeReport, LIVE_PROBE_FILES, LIVE_PROBE_MARKER, LIVE_PROBES } from '../scripts/live-probes.ts'; +import playwrightConfig from '../playwright.config.ts'; import config from '../vitest.config.ts'; const ROOT = fileURLToPath(new URL('..', import.meta.url)); @@ -35,6 +36,18 @@ describe('Vitest scheduling', () => { }); }); +describe('Playwright config', () => { + it('runs every browser spec and refuses a committed test.only', () => { + // The whole config, exactly: a `grep`, `testMatch`, `testIgnore` or narrower `testDir` could drop specs from the + // CI test job while it still passes, and without `forbidOnly` a committed `test.only` would run only that test. + // defineConfig adds one symbol-keyed marker; Playwright reads only string keys. + expect(Object.getOwnPropertySymbols(playwrightConfig).map(String)).toEqual(['Symbol(defineConfigWasUsed)']); + expect(Object.fromEntries(Object.entries(playwrightConfig))).toEqual({ testDir: './test/browser', forbidOnly: true, fullyParallel: false, workers: 1, + timeout: 60_000, expect: { timeout: 15_000 }, + use: { browserName: 'chromium', viewport: { width: 1512, height: 982 }, trace: 'retain-on-failure' } }); + }); +}); + const workflow = (name: string) => parse(readFileSync(join(ROOT, '.github/workflows', name), 'utf8')) as { on: Record; @@ -356,8 +369,10 @@ describe('CI workflows', () => { it('names every live probe exactly as its test file titles it', () => { // The tests that the probe switch enables are exactly the listed probes, by Vitest full name, and every one ends in - // the suffix the workflow's -t filter selects. No other test in the file carries the suffix: every title in the file - // is a literal, or probeFileTests throws, so this sees each one. + // the suffix the workflow's -t filter selects. No other test in the file carries the suffix in its literal title: + // every title in the file is a literal, or probeFileTests throws, so this sees each one. A `.each` or `.for` title is + // read before Vitest fills its placeholders, so `it.each(['(live Claude)'])('sneaky %s', ...)` passes here; the + // live-probes job's report check (`checkLiveProbeReport`) then refuses that test, because it ran and is not a probe. for (const file of LIVE_PROBE_FILES) { const tests = probeFileTests(file, readFileSync(join(ROOT, file), 'utf8')); const gated = tests.filter(test => test.gated).map(test => test.name);