diff --git a/test/carve-section-loading.test.ts b/test/carve-section-loading.test.ts index 75a787cbd..cd1983efd 100644 --- a/test/carve-section-loading.test.ts +++ b/test/carve-section-loading.test.ts @@ -22,7 +22,7 @@ import { test, expect } from 'bun:test'; import { CAPTURE_LONG_MS } from './helpers/eval-budgets'; import { describeE2ETier } from './helpers/e2e-gate'; -import { setupSkillDir, skillFromWorktree, captureSectionReads } from './helpers/auq-sdk-capture'; +import { setupSkillDir, skillFromWorktree, captureSectionReads, LONG_SECTION_CAPTURE_MS } from './helpers/auq-sdk-capture'; import { CARVE_GUARDS } from './helpers/carve-guards'; const describeE2E = describeE2ETier('periodic'); @@ -78,7 +78,7 @@ describeE2E('carve behavioral section-loading (periodic, SDK capture)', () => { // their required section reads inside 60s but need 300-450s of // wall clock to finish the report on slower sandboxes — a timeout // there reads as a loading failure when the carve invariant held. - timeout: 480_000, + timeout: LONG_SECTION_CAPTURE_MS, }); const missing = guard.requiredReads.filter((s) => !readSections.has(s)); diff --git a/test/ceo-section-loading-fixture.test.ts b/test/ceo-section-loading-fixture.test.ts new file mode 100644 index 000000000..0d936baa2 --- /dev/null +++ b/test/ceo-section-loading-fixture.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, test } from 'bun:test'; +import { + CACHE_READ_WRITE_SKETCH, + CEO_SECTION_CACHE_PLAN, + hasStaleFillRaceFinding, +} from './helpers/ceo-section-loading-fixture'; + +describe('CEO section-loading cache fixture', () => { + test('the exact proposed wrapper retains a reproducible stale-fill race', async () => { + let releaseRead!: (value: string) => void; + let stored = 'old'; + const cache = new Map(); + const repository = { + read: () => new Promise((resolve) => { releaseRead = resolve; }), + write: async (_key: string, value: string) => { stored = value; return value; }, + }; + // Execute the same sketch the live reviewer receives, not a second model + // of its ordering. Holding the old read exposes the intended interleaving. + const { readProfile, writeProfile } = new Function('cache', 'repository', + CACHE_READ_WRITE_SKETCH + '\nreturn { readProfile, writeProfile };')(cache, repository); + const pending = readProfile('tenant:profile'); + await writeProfile('tenant:profile', 'new'); + releaseRead('old'); + await pending; + expect(stored).toBe('new'); + expect(await readProfile('tenant:profile')).toBe('old'); + expect(CEO_SECTION_CACHE_PLAN).toContain(CACHE_READ_WRITE_SKETCH); + expect(hasStaleFillRaceFinding(CEO_SECTION_CACHE_PLAN)).toBe(false); + }); + + test.each([ + // Actual finding in the unchanged fixture's successful 38 KB live report. + '**Missing: What happens to in-flight requests during invalidation?** If a write invalidates a key and 10 requests are simultaneously loading it (cache miss, in-flight DB fetch), all 10 will cache the same value after the invalidation. The invalidated key may get re-populated with a stale value if any of those fetches started before the write. No mention of this race.', + 'P1: An in-flight read can repopulate stale data after a committed write invalidates the key. Guard fills with a generation token.', + '| Cache fill race | An older value fetched before the write is inserted after eviction, so the next read is stale. | Add a per-key epoch. |', + '**Invalidation race:** the pending fetch stores an outdated snapshot after cache.delete. Serialize the fill with mutation.', + ])('recognizes the actual ordering defect: %s', (report) => { + expect(hasStaleFillRaceFinding(report)).toBe(true); + }); + + test.each([ + 'The full review is complete. No issues found.', + 'A stale value expires after 30 seconds. The LRU byte cap is adequate.', + 'If invalidation throws after a write, the cache retains stale data. Log the failure and bypass the cache.', + 'Read and write concurrency is covered. No stale data can be returned.', + '| Reads | Coalesced concurrent misses |\n| Writes | Invalidation failure leaves stale data |', + '```javascript\n// An in-flight read can cache stale data after invalidation.\n```', + '> An in-flight read can cache stale data after invalidation.', + ])('rejects completion, unrelated text, and quoted source: %s', (report) => { + expect(hasStaleFillRaceFinding(report)).toBe(false); + }); +}); diff --git a/test/helpers/auq-sdk-capture.ts b/test/helpers/auq-sdk-capture.ts index 05603b9fa..993976e6c 100644 --- a/test/helpers/auq-sdk-capture.ts +++ b/test/helpers/auq-sdk-capture.ts @@ -20,6 +20,14 @@ import { runSkillTest, type SkillTestResult } from './session-runner'; const ROOT = path.resolve(__dirname, '..', '..'); +/** + * Existing long section-loader work budget (v1.71): complete workflows can + * load their sections quickly, then need 300–450s to generate the full report. + * Keep 120s of the CAPTURE_LONG_MS outer budget for setup and reporting. + * Ordinary captureSectionReads callers retain the 300s default below. + */ +export const LONG_SECTION_CAPTURE_MS = 480_000; + /** The 7 decision-brief format elements graded on the captured AUQ text. */ export const AUQ_FORMAT_ELEMENTS: Array<{ field: string; re: RegExp }> = [ { field: 'ELI10:', re: /ELI10\s*:/i }, @@ -217,6 +225,22 @@ This is a capture test, not an interactive session. Skip any system-audit / envi * resolves to, so Read/Grep/Glob/Write is all the agent needs (no Bash → it cannot * `find /` its way out, nor run git/gh mutations). */ +export function hasDisabledOutsideReview(output: string): boolean { + const headings = [...output.matchAll(/^## GSTACK REVIEW REPORT[ \t]*\r?$/gm)]; + const heading = headings.at(-1); + if (!heading) return false; + const section = output.slice(heading.index! + heading[0].length).split(/^##[ \t]+/m, 1)[0]; + const plain = (cell: string) => cell.replace(/[*_`]/g, '').trim().replace(/\s+/g, ' ').toLowerCase(); + for (const line of section.split('\n')) { + if (!line.trimStart().startsWith('|')) continue; + const cells = line.split('|').map(plain); + if (cells[1] === 'outside review') { + return /^disabled(?:$|\s|[(:—–-])/.test(cells[5] ?? ''); + } + } + return false; +} + export async function captureSectionReads(opts: { planDir: string; skillName: string; @@ -230,9 +254,19 @@ export async function captureSectionReads(opts: { model?: string; maxTurns?: number; timeout?: number; + /** Measure native section loading with the documented extra-review opt-out. */ + nativeReviewOnly?: boolean; }): Promise<{ readSections: Set; reportProduced: boolean; toolCalls: SkillTestResult['toolCalls']; output: string }> { const outFile = path.join(opts.planDir, opts.reportFile ?? 'REPORT.md'); const skillPath = path.join(opts.planDir, opts.skillName, 'SKILL.md'); + // Outside-review dispatch has separate behavioral coverage. Native-only + // captures use the real supported control in state owned by this call; + // never mutate the operator's or another capture's gstack configuration. + const stateDir = opts.nativeReviewOnly + ? fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-section-state-')) : null; + const nativeReviewRule = stateDir + ? `\n- Read ${path.join(stateDir, 'config.yaml')}, the isolated gstack configuration for this capture. It sets codex_reviews: disabled. Follow that documented control: skip the entire extra outside-review step, including its native fallback, and report outside coverage as disabled. Complete all native review sections and the full required report.` + : ''; const prompt = `You are running an automated skill-execution test. No human is present, so AskUserQuestion is unavailable. The ONLY skill file you may read is this absolute path: ${skillPath}. Do NOT Glob/find/search for any other SKILL.md anywhere — especially nothing under ~/.claude or /Users. Read ${skillPath} and EXECUTE its workflow for this scenario: @@ -244,18 +278,26 @@ Rules for this run: - At any decision point that would call AskUserQuestion, silently pick the skill's recommended option and continue. Do NOT stop to ask. - This skill's body has been carved into on-demand sections/. When the skill gives a STOP-Read directive (for example "Read \`.../sections/\` and execute it in full"), you MUST actually Read that sections/ file with the Read tool BEFORE doing the work it covers. Do not work from memory. - Do NOT run git, gh, commit, push, or any mutating command. -- When the workflow is complete, write the skill's final output (the full review report / ship plan, including any required report table) to ${outFile}.`; +- When the workflow is complete, write the skill's final output (the full review report / ship plan, including any required report table) to ${outFile}.${nativeReviewRule}`; - const result = await runSkillTest({ - prompt, - workingDirectory: opts.planDir, - allowedTools: ['Read', 'Grep', 'Glob', 'Write'], - maxTurns: opts.maxTurns ?? 25, - timeout: opts.timeout ?? 300_000, - testName: opts.testName, - runId: opts.runId, - model: resolveEvalModel('capture', opts.model), - }); + let result: SkillTestResult; + try { + if (stateDir) fs.writeFileSync(path.join(stateDir, 'config.yaml'), 'codex_reviews: disabled\n'); + result = await runSkillTest({ + prompt, + workingDirectory: opts.planDir, + allowedTools: ['Read', 'Grep', 'Glob', 'Write'], + tools: ['Read', 'Grep', 'Glob', 'Write'], + maxTurns: opts.maxTurns ?? 25, + timeout: opts.timeout ?? 300_000, + testName: opts.testName, + runId: opts.runId, + model: resolveEvalModel('capture', opts.model), + ...(stateDir ? { env: { GSTACK_HOME: stateDir, GSTACK_STATE_ROOT: stateDir } } : {}), + }); + } finally { + if (stateDir) fs.rmSync(stateDir, { recursive: true, force: true }); + } const readSections = new Set(); for (const c of result.toolCalls) { @@ -267,7 +309,8 @@ Rules for this run: let output = ''; try { output = fs.readFileSync(outFile, 'utf-8'); } catch { output = result.output ?? ''; } - const reportProduced = opts.reportMarker ? opts.reportMarker.test(output) : output.trim().length > 0; + const reportProduced = result.exitReason === 'success' + && (opts.reportMarker ? opts.reportMarker.test(output) : output.trim().length > 0); return { readSections, reportProduced, toolCalls: result.toolCalls, output }; } diff --git a/test/helpers/ceo-section-loading-fixture.ts b/test/helpers/ceo-section-loading-fixture.ts new file mode 100644 index 000000000..bc131200d --- /dev/null +++ b/test/helpers/ceo-section-loading-fixture.ts @@ -0,0 +1,93 @@ +/** + * Section loading needs a complete, bounded plan, not a finding-count fixture. + * The former two-bullet plan made a successful review invent key isolation, + * error, concurrency, observability, rollout, and test contracts in a 38 KB + * report. Those surrounding contracts are explicit here; the read/write sketch + * still permits an old in-flight read to refill a key after write invalidation. + */ +export const CACHE_READ_WRITE_SKETCH = `async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const value = await repository.read(key); + cache.set(key, value); + return value; +} + +async function writeProfile(key, update) { + const saved = await repository.write(key, update); + cache.delete(key); + return saved; +}`; + +export const CEO_SECTION_CACHE_PLAN = `# Plan: cache profile summaries in one process + +## Measured problem and accepted scope +The existing profile-summary service has one active process. A one-week trace +shows repeated reads of about 900 hot keys: DB CPU is 70%, with read p95 120 ms. +Add a process-local LRU wrapper to the existing repository. Acceptance targets +are at least 60% cache hits, DB CPU below 50%, and read p95 below 60 ms, with the +existing error-rate and correctness SLOs unchanged. This is an internal backend +change with no UI, API, schema, pricing, or developer onboarding change. + +## Existing contracts retained +- All reads and writes use this repository in the same process; there are no + external DB writers. Multi-process operation remains unsupported and startup + rejects that configuration while caching is enabled. +- Authentication and authorization run before repository access. Keys encode + the authenticated tenant ID and validated profile ID without ambiguity. + Values are immutable profile-summary DTOs; secrets and cache keys are never + logged. Cached results cannot bypass authorization. +- The existing LRU adapter supports 1000 entries, a 16 MiB byte cap, and a + 30-second TTL. Recorded hot data fits those limits. Absent records use a + distinct sentinel with a 10-second TTL; undefined means a cache miss. +- Cache operations are synchronous and atomic in the single JS event loop. + On any cache failure the existing adapter bypasses the cache until an empty + cache is reinitialized; repository errors keep the current typed API error + mapping. The existing per-key + single-flight wrapper coalesces simultaneous misses and releases on failure. +- A read already in progress when a write commits may return its earlier DB + snapshot to that caller. Every read begun after that write completes must + observe the committed version. TTL expiry is not a substitute for this rule. + +## Proposed wrapper integration +Keep the current read-through repository interface and shared adapters. These +are the complete new read/write ordering rules; no additional version checks or +coordination between a cache fill and a write are proposed: + +\`\`\`javascript +${CACHE_READ_WRITE_SKETCH} +\`\`\` + +## Verification and rollout +Existing repository contract tests cover tenant isolation, key validation, +absence, DB failures, and authorization. New wrapper tests cover hit/miss, +eviction and byte limits, TTL, adapter-failure fallback, successful-write +invalidation, failed-write preservation, and concurrent-miss coalescing. +The rollout uses the existing runtime feature flag: enable for 10% of keys, +then 50%, then all keys after one healthy hour at each stage. Monitor hit/miss, +eviction, cache bytes, fallback errors, DB CPU, and read p95 without raw IDs. +On error-rate or latency regression, disable the flag immediately; both reads +and writes bypass the cache while disabled, and enabling creates an empty cache. +Cold starts remain within the existing DB capacity. The service owner monitors +the rollout and records the results against the acceptance targets. + +## Out of scope +Distributed caching, cross-process coherence, prewarming, changing consistency +semantics, or adding new product surfaces. The repository interface preserves a +future replacement path without introducing a general cache framework now. +`; + +/** Recognize the seeded concurrency finding across ordinary report wording. */ +export function hasStaleFillRaceFinding(report: string): boolean { + // Ignore quoted source: a copied plan or code sketch is not a finding. + const prose = report.replace(/```[\s\S]*?```/g, '').replace(/^\s*>.*$/gm, ''); + const blocks = prose.split(/\n\s*\n|\n(?=\s*\|)/); + return blocks.some((block) => { + const text = block.replace(/[*_`]/g, '').replace(/\s+/g, ' ') + .replace(/\bno\s+(?:stale|outdated)\b[^.!?]*(?:[.!?]|$)/gi, ''); + return /\b(?:stale|outdated)\b|\bold(?:er)?\s+(?:value|data|result|version)\b/i.test(text) + && /\b(?:race|racing|concurrent|concurrency|in[- ]flight|pending)\b/i.test(text) + && /\b(?:read|fetch|fill|refill|repopulat|insert|store|set)\w*\b/i.test(text) + && /\b(?:invalidat|evict|write|commit)\w*\b/i.test(text); + }); +} diff --git a/test/helpers/session-runner.ts b/test/helpers/session-runner.ts index 2bbababea..6c316b413 100644 --- a/test/helpers/session-runner.ts +++ b/test/helpers/session-runner.ts @@ -132,7 +132,10 @@ export async function runSkillTest(options: { prompt: string; workingDirectory: string; maxTurns?: number; + /** Approval allowlist; does not restrict which tools the model can see. */ allowedTools?: string[]; + /** Optional built-in tool availability. Omit to preserve the CLI defaults. */ + tools?: string[]; timeout?: number; testName?: string; runId?: string; @@ -197,6 +200,10 @@ export async function runSkillTest(options: { '--max-turns', String(maxTurns), '--allowed-tools', ...allowedTools, ]; + // --allowed-tools controls approval, including when permissions are skipped; + // only --tools removes unrelated built-ins such as Agent, Bash, and Skill. + // Keep this opt-in: existing workflow evals intentionally use CLI defaults. + if (options.tools !== undefined) args.push('--tools', options.tools.join(',')); // Hermetic children get zero MCP servers (no --mcp-config is passed). // Gated on the same call-time check as the env scrub so EVALS_HERMETIC=0 // restores operator MCP along with the operator env. @@ -426,9 +433,9 @@ export async function runSkillTest(options: { if (resultLine.subtype === 'success' && resultLine.is_error) { // claude -p can return subtype=success with is_error=true (e.g. API connection failure) exitReason = 'error_api'; - } else if (resultLine.subtype === 'success') { + } else if (resultLine.subtype === 'success' && exitCode === 0 && !timedOut) { exitReason = 'success'; - } else if (resultLine.subtype) { + } else if (resultLine.subtype && resultLine.subtype !== 'success') { // Preserve known subtypes like error_max_turns even if is_error is set exitReason = resultLine.subtype; } diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 5611167ca..b991d8533 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -147,13 +147,13 @@ export const E2E_TOUCHFILES: Record = { 'tpa-broken': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], 'tpa-absent-darwin': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], 'tpa-apple-ban': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], - 'ship-section-loading': ['ship/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-ship-section-loading.test.ts'], - 'plan-ceo-section-loading': ['plan-ceo-review/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts'], + 'ship-section-loading': ['ship/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/skill-e2e-ship-section-loading.test.ts'], + 'plan-ceo-section-loading': ['plan-ceo-review/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts'], // Data-driven behavioral guard for the 'plan'/'prompt' carves (eng, design, // devex, office-hours + future PR2 carves). One file iterating CARVE_GUARDS; // the selector sets GSTACK_CARVE_SKILL= to scope cost to the changed // skill (D-CODEX A). Touching the registry/helper or sections.ts runs all. - 'carve-section-loading': ['design-html/**', 'design-shotgun/**', 'qa/**', 'browse/**', 'retro/**', 'autoplan/**', 'spec/**', 'setup-gbrain/**', 'review/**', 'codex/**', 'land-and-deploy/**', 'plan-eng-review/**', 'plan-design-review/**', 'plan-devex-review/**', 'office-hours/**', 'document-release/**', 'design-consultation/**', 'cso/**', 'test/helpers/carve-guards.ts', 'scripts/resolvers/sections.ts', 'scripts/resolvers/redact-doc.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/carve-section-loading.test.ts'], + 'carve-section-loading': ['design-html/**', 'design-shotgun/**', 'qa/**', 'browse/**', 'retro/**', 'autoplan/**', 'spec/**', 'setup-gbrain/**', 'review/**', 'codex/**', 'land-and-deploy/**', 'plan-eng-review/**', 'plan-design-review/**', 'plan-devex-review/**', 'office-hours/**', 'document-release/**', 'design-consultation/**', 'cso/**', 'test/helpers/carve-guards.ts', 'scripts/resolvers/sections.ts', 'scripts/resolvers/redact-doc.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/carve-section-loading.test.ts'], 'autoplan-chain-pty': ['autoplan/**', 'plan-ceo-review/**', 'plan-design-review/**', 'plan-eng-review/**', 'plan-devex-review/**', 'test/fixtures/plans/ui-heavy-feature.md', 'test/helpers/claude-pty-runner.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/autoplan-setup-question.ts', 'test/autoplan-setup-question.test.ts', 'test/skill-e2e-autoplan-chain.test.ts'], // Per-finding AskUserQuestion count + review-report-at-bottom assertion. diff --git a/test/session-runner-tools.test.ts b/test/session-runner-tools.test.ts new file mode 100644 index 000000000..527007e46 --- /dev/null +++ b/test/session-runner-tools.test.ts @@ -0,0 +1,289 @@ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { captureSectionReads, hasDisabledOutsideReview, LONG_SECTION_CAPTURE_MS } from './helpers/auq-sdk-capture'; +import { CAPTURE_MS, CAPTURE_LONG_MS } from './helpers/eval-budgets'; +import { getHermeticDirs } from './helpers/hermetic-env'; +import { runSkillTest } from './helpers/session-runner'; + +interface Observed { + args: string[]; + prompt: string; + stateHome: string; + stateRoot: string | null; + outsideDisabled: boolean; +} + +describe('outside-review disabled status in a completed report', () => { + const report = (status: string) => [ + '## GSTACK REVIEW REPORT', '', + '| Review | Trigger | Why | Runs | Status | Findings |', + '| Outside Review | codex_reviews | Independent review | 0 | ' + status + ' | None |', + ].join('\n'); + + test.each(['disabled', 'Disabled', '**disabled**', '`disabled`', + 'disabled (codex_reviews)', '__Disabled__ — configured opt-out'])('accepts semantic status %s', (status) => { + expect(hasDisabledOutsideReview(report(status))).toBe(true); + }); + + test.each(['', 'unavailable', 'completed', 'not disabled', 'disabledness'])('rejects status %s', (status) => { + expect(hasDisabledOutsideReview(report(status))).toBe(false); + }); + + test('requires the Outside Review row in the final report section', () => { + expect(hasDisabledOutsideReview('Outside coverage: disabled')).toBe(false); + expect(hasDisabledOutsideReview(report('disabled') + '\n\n' + report('completed'))).toBe(false); + expect(hasDisabledOutsideReview('## GSTACK REVIEW REPORT\n\n## Other context\n' + + '| Outside Review | codex_reviews | Independent review | 0 | disabled | None |')).toBe(false); + }); +}); + +const FAKE_CLAUDE = String.raw` + const fs = require('node:fs'); + const path = require('node:path'); + const prompt = await Bun.stdin.text(); + const stateHome = process.env.GSTACK_HOME; + let config = ''; + try { config = fs.readFileSync(path.join(stateHome, 'config.yaml'), 'utf8'); } catch {} + const observed = { + args: process.argv.slice(2), prompt, stateHome, + stateRoot: process.env.GSTACK_STATE_ROOT ?? null, + outsideDisabled: /^codex_reviews:\s*disabled\s*$/m.test(config), + }; + fs.writeFileSync('observed.json', JSON.stringify(observed)); + console.log(JSON.stringify({ type: 'system', subtype: 'init' })); + if (fs.existsSync('fail-cli')) { + const failure = fs.readFileSync('fail-cli', 'utf8'); + const report = '## GSTACK REVIEW REPORT\nA report written before the run failed.\n'; + fs.writeFileSync('REPORT.md', report); + console.log(JSON.stringify({ type: 'result', + subtype: failure === 'error' ? 'error_during_execution' : 'success', + is_error: failure !== 'nonzero', result: report, + })); + process.exitCode = failure === 'is_error' ? 0 : 1; + } else { + const file_path = path.join(process.cwd(), 'plan-ceo-review', 'sections', 'review-sections.md'); + console.log(JSON.stringify({ type: 'assistant', message: { content: [ + { type: 'tool_use', name: 'Read', input: { file_path } }, + ] } })); + if (fs.existsSync('wait-for-report')) { + fs.writeFileSync('work-ready', ''); + while (!fs.existsSync('release-report')) await Bun.sleep(5); + } + fs.writeFileSync('REPORT.md', '## GSTACK REVIEW REPORT\nFull native review fixture.\nOutside review: ' + + (observed.outsideDisabled ? 'disabled' : 'default') + '\n'); + console.log(JSON.stringify({ type: 'result', subtype: 'success', result: JSON.stringify(observed) })); + } +`; + +function flagValue(args: string[], flag: string): string | undefined { + const at = args.indexOf(flag); + return at < 0 ? undefined : args[at + 1]; +} + +async function withFakeClaude(run: (dir: string, readObserved: () => Observed) => Promise): Promise { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'session-tools-')); + const shimDir = path.join(dir, 'shim'); + const script = path.join(dir, 'fake-claude.ts'); + fs.mkdirSync(shimDir); + fs.writeFileSync(script, FAKE_CLAUDE); + const quote = (value: string) => "'" + value.replace(/'/g, "'\\''") + "'"; + // Explicit POSIX shim: Windows CreateProcess does not interpret /bin/sh. + fs.writeFileSync(path.join(shimDir, 'claude'), + `#!/bin/sh\nexec ${quote(process.execPath)} ${quote(script)} "$@"\n`, { mode: 0o755 }); + const saved = Object.fromEntries(['PATH', 'EVALS_HERMETIC', 'GSTACK_HOME', 'GSTACK_STATE_ROOT'] + .map((key) => [key, process.env[key]])); + process.env.PATH = `${shimDir}${path.delimiter}${process.env.PATH ?? ''}`; + process.env.EVALS_HERMETIC = '1'; + try { + await run(dir, () => JSON.parse(fs.readFileSync(path.join(dir, 'observed.json'), 'utf8'))); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +describe.skipIf(process.platform === 'win32')('session-runner explicit tool availability', () => { + test('long section work fits the existing long capture tier with reporting headroom', () => { + expect(LONG_SECTION_CAPTURE_MS).toBeGreaterThan(CAPTURE_MS); + expect(CAPTURE_LONG_MS - LONG_SECTION_CAPTURE_MS).toBe(120_000); + }); + + test('a long capture can finish after the ordinary deadline; the default still times out', async () => { + // Run the controlled clock in a subprocess: it cannot affect sibling tests + // or hooks in Bun's shared shard. The fake CLI blocks after a real Read + // event until the worker advances time and allows report completion. + for (const long of [false, true]) { + await withFakeClaude(async (dir) => { + fs.writeFileSync(path.join(dir, 'wait-for-report'), ''); + const worker = path.join(dir, 'budget-worker.ts'); + const captureModule = path.resolve(import.meta.dir, 'helpers/auq-sdk-capture.ts'); + fs.writeFileSync(worker, ` + import * as fs from 'node:fs'; + import { captureSectionReads, LONG_SECTION_CAPTURE_MS } from ${JSON.stringify(captureModule)}; + const realNow = Date.now; + const realSetTimeout = globalThis.setTimeout; + const realClearTimeout = globalThis.clearTimeout; + let advanced = 0; + const timers = new Map(); + Date.now = () => realNow() + advanced; + globalThis.setTimeout = ((callback, delay = 0, ...args) => { + const id = realSetTimeout(() => { + timers.delete(id); + callback(...args); + }, delay); + timers.set(id, { callback, args, delay, due: Date.now() + delay }); + return id; + }); + globalThis.clearTimeout = ((id) => { timers.delete(id); realClearTimeout(id); }); + const pause = () => new Promise(resolve => realSetTimeout(resolve, 5)); + let pending; + let finished = false; + try { + pending = captureSectionReads({ + planDir: process.cwd(), skillName: 'plan-ceo-review', scenario: 'Complete the full review', + testName: 'section-budget', ${long ? 'timeout: LONG_SECTION_CAPTURE_MS,' : ''} + }); + const waitStarted = realNow(); + while (!fs.existsSync('work-ready') || ![...timers.values()].some(timer => timer.delay > 200_000)) { + if (realNow() - waitStarted > 5_000) throw new Error('Fake CLI did not enter the work phase'); + await pause(); + } + // 360s is beyond the ordinary 300s work limit, inside the existing + // 480s long-workflow limit. No real-time deadline is relaxed. + advanced = 360_000; + for (const [id, timer] of [...timers]) { + if (timer.due <= Date.now()) { + globalThis.clearTimeout(id); + timer.callback(...timer.args); + } + } + fs.writeFileSync('release-report', ''); + const result = await pending; + finished = true; + console.log(JSON.stringify({ + reportProduced: result.reportProduced, + readSection: result.readSections.has('review-sections.md'), + })); + } finally { + // A failed handshake must still terminate the fake CLI through + // the runner's own scoped timeout, not orphan it behind the worker. + if (!finished) { + for (const [id, timer] of [...timers]) { + if (timer.delay >= 10_000) { + globalThis.clearTimeout(id); + timer.callback(...timer.args); + } + } + await pending?.catch(() => {}); + } + for (const id of timers.keys()) realClearTimeout(id); + Date.now = realNow; + globalThis.setTimeout = realSetTimeout; + globalThis.clearTimeout = realClearTimeout; + } + `); + const result = Bun.spawnSync([process.execPath, worker], { + cwd: dir, env: process.env, stdout: 'pipe', stderr: 'pipe', timeout: 15_000, + }); + expect(result.exitCode, result.stderr.toString()).toBe(0); + expect(JSON.parse(result.stdout.toString())).toEqual({ reportProduced: long, readSection: true }); + }); + } + }, 35_000); + + test('passes available tools separately from the approval allowlist and preserves stdin', async () => { + await withFakeClaude(async (dir, observed) => { + const prompt = 'Read the literal text: `$(do not execute)` and "quotes".\nSecond line.'; + const result = await runSkillTest({ + prompt, workingDirectory: dir, allowedTools: ['Read'], tools: ['Read', 'Write'], + model: 'capture-model', maxTurns: 7, timeout: 5_000, + }); + expect(result.exitReason).toBe('success'); + expect(observed().prompt).toBe(prompt); + expect(flagValue(observed().args, '--allowed-tools')).toBe('Read'); + expect(flagValue(observed().args, '--tools')).toBe('Read,Write'); + expect(flagValue(observed().args, '--model')).toBe('capture-model'); + expect(flagValue(observed().args, '--max-turns')).toBe('7'); + }); + }); + + test('omitting tools preserves default tool availability for existing callers', async () => { + await withFakeClaude(async (dir, observed) => { + await runSkillTest({ prompt: 'Default tools', workingDirectory: dir, allowedTools: ['Read'], timeout: 5_000 }); + expect(observed().args).not.toContain('--tools'); + expect(flagValue(observed().args, '--allowed-tools')).toBe('Read'); + }); + }); + + test('an explicit empty list uses the CLI no-tools argument', async () => { + await withFakeClaude(async (dir, observed) => { + await runSkillTest({ prompt: 'No tools', workingDirectory: dir, tools: [], timeout: 5_000 }); + expect(flagValue(observed().args, '--tools')).toBe(''); + }); + }); + + test('section capture exposes only its declared tools without opting out of outside reviews by default', async () => { + await withFakeClaude(async (dir, observed) => { + const result = await captureSectionReads({ + planDir: dir, skillName: 'plan-ceo-review', scenario: 'Review the full plan', + testName: 'section-tools-default', timeout: 5_000, + }); + expect(flagValue(observed().args, '--tools')).toBe('Read,Grep,Glob,Write'); + expect(observed().stateHome).toBe(getHermeticDirs().gstackHome); + expect(observed().prompt).not.toContain('codex_reviews: disabled'); + expect(result.readSections.has('review-sections.md')).toBe(true); + expect(result.reportProduced).toBe(true); + }); + }); + + test('native-only capture gives its child real isolated disabled config and cleans only that state', async () => { + await withFakeClaude(async (dir, observed) => { + const hostState = path.join(dir, 'host-state'); + fs.mkdirSync(hostState); + const hostConfig = 'codex_reviews: enabled\nmarker: preserve\n'; + fs.writeFileSync(path.join(hostState, 'config.yaml'), hostConfig); + process.env.GSTACK_HOME = hostState; + process.env.GSTACK_STATE_ROOT = hostState; + const sharedConfigPath = path.join(getHermeticDirs().gstackHome, 'config.yaml'); + const sharedBefore = fs.existsSync(sharedConfigPath) ? fs.readFileSync(sharedConfigPath, 'utf8') : null; + const result = await captureSectionReads({ + planDir: dir, skillName: 'plan-ceo-review', scenario: 'Review the full plan', + testName: 'section-tools-native', nativeReviewOnly: true, timeout: 5_000, + }); + const child = observed(); + expect(child.outsideDisabled).toBe(true); + expect(child.stateRoot).toBe(child.stateHome); + expect(child.stateHome).not.toBe(hostState); + expect(child.stateHome).not.toBe(getHermeticDirs().gstackHome); + expect(child.prompt).toContain(path.join(child.stateHome, 'config.yaml')); + expect(child.prompt).toContain('Complete all native review sections and the full required report.'); + expect(child.prompt).toContain('report outside coverage as disabled'); + expect(fs.existsSync(child.stateHome)).toBe(false); + expect(fs.readFileSync(path.join(hostState, 'config.yaml'), 'utf8')).toBe(hostConfig); + expect(fs.existsSync(sharedConfigPath) ? fs.readFileSync(sharedConfigPath, 'utf8') : null).toBe(sharedBefore); + expect(result.reportProduced).toBe(true); + expect(result.output).toContain('Outside review: disabled'); + }); + }); + + test.each(['nonzero', 'is_error', 'error'])('a %s capture cannot pass using a report left behind before failure', async (failure) => { + await withFakeClaude(async (dir, observed) => { + fs.writeFileSync(path.join(dir, 'fail-cli'), failure); + const result = await captureSectionReads({ + planDir: dir, skillName: 'plan-ceo-review', scenario: 'Review the full plan', + testName: 'section-tools-failed', nativeReviewOnly: true, timeout: 5_000, + reportMarker: /^## GSTACK REVIEW REPORT\s*$/m, + }); + expect(observed().outsideDisabled).toBe(true); + expect(fs.existsSync(observed().stateHome)).toBe(false); + expect(result.reportProduced).toBe(false); + // Preserve useful diagnostics; failed completion does not erase output. + expect(result.output).toContain('## GSTACK REVIEW REPORT'); + }); + }); +}); diff --git a/test/skill-e2e-plan-ceo-review-section-loading.test.ts b/test/skill-e2e-plan-ceo-review-section-loading.test.ts index 6d59bd730..4e53ab655 100644 --- a/test/skill-e2e-plan-ceo-review-section-loading.test.ts +++ b/test/skill-e2e-plan-ceo-review-section-loading.test.ts @@ -31,7 +31,10 @@ import { setupSkillDir, skillFromWorktree, captureSectionReads, + hasDisabledOutsideReview, + LONG_SECTION_CAPTURE_MS, } from './helpers/auq-sdk-capture'; +import { CEO_SECTION_CACHE_PLAN, hasStaleFillRaceFinding } from './helpers/ceo-section-loading-fixture'; const describeE2E = describeE2ETier('periodic'); const runId = `plan-ceo-section-loading-${process.env.EVALS_RUN_ID ?? 'local'}`; @@ -39,22 +42,6 @@ const runId = `plan-ceo-section-loading-${process.env.EVALS_RUN_ID ?? 'local'}`; // Sections every plan-ceo-review run must consult after Step 0. const REQUIRED_SECTIONS = ['review-sections.md']; -const PLAN_MD = [ - '# Plan: add an in-memory cache layer', - '', - '## Context', - 'Reads hit the DB on every request. Add a process-local LRU cache in front of', - 'the read path to cut DB load.', - '', - '## Approach', - '- Wrap the read repository in a cache that stores the last 1000 keys.', - '- Invalidate on write.', - '', - '## Out of scope', - 'Distributed cache, cross-process coherence.', - '', -].join('\n'); - describeE2E('/plan-ceo-review section-loading E2E (periodic, SDK capture)', () => { test( 'a real review Reads the carved section before producing the report', @@ -64,7 +51,7 @@ describeE2E('/plan-ceo-review section-loading E2E (periodic, SDK capture)', () = skillName: 'plan-ceo-review', skillMd, sectionsFrom, - fixtures: { 'PLAN.md': PLAN_MD }, + fixtures: { 'PLAN.md': CEO_SECTION_CACHE_PLAN }, tmpPrefix: 'gstack-ceo-secload-', }); @@ -74,9 +61,16 @@ describeE2E('/plan-ceo-review section-loading E2E (periodic, SDK capture)', () = scenario: 'Review the plan in PLAN.md. Hold the current scope (HOLD SCOPE mode) — do not challenge or expand scope. Run the full CEO review and produce the review report.', requiredSections: REQUIRED_SECTIONS, - reportMarker: /GSTACK REVIEW REPORT|COMPLETION SUMMARY|review/i, + reportMarker: /^## GSTACK REVIEW REPORT\s*$/m, testName: 'plan-ceo-section-loading', runId, + // This external carve missed the generic loader's v1.71 budget fix: + // a full 11-section review needs its long work budget, not the helper's + // ordinary 300s default. The outer CAPTURE_LONG_MS remains unchanged. + timeout: LONG_SECTION_CAPTURE_MS, + // This case measures native section loading and report completion; + // outside-provider dispatch is covered by the cross-harness evals. + nativeReviewOnly: true, }); const missing = REQUIRED_SECTIONS.filter(s => !readSections.has(s)); @@ -87,6 +81,13 @@ describeE2E('/plan-ceo-review section-loading E2E (periodic, SDK capture)', () = }); // Guard against an empty pass: the report must have real content. expect(output.trim().length).toBeGreaterThan(200); + expect(output).toMatch(/^\|\s*Review\s*\|\s*Trigger\s*\|\s*Why\s*\|\s*Runs\s*\|\s*Status\s*\|\s*Findings\s*\|/m); + expect(output).toMatch(/^\|\s*CEO Review\s*\|/m); + // A native capture must not invent an outside dispatch or claim coverage. + expect(hasDisabledOutsideReview(output)).toBe(true); + // Loading a section and producing a table alone must not hide an empty + // review: the complete fixture still contains a real ordering defect. + expect(hasStaleFillRaceFinding(output)).toBe(true); }, CAPTURE_LONG_MS, ); diff --git a/test/touchfiles.test.ts b/test/touchfiles.test.ts index 67dcddec3..1bec835a6 100644 --- a/test/touchfiles.test.ts +++ b/test/touchfiles.test.ts @@ -132,6 +132,14 @@ describe('selectTests', () => { expect(result.reason).toContain('global'); }); + test('section-capture tool isolation regression selects only its three capture workflows', () => { + const result = selectTests(['test/session-runner-tools.test.ts'], E2E_TOUCHFILES); + expect(result.selected.sort()).toEqual([ + 'carve-section-loading', 'plan-ceo-section-loading', 'ship-section-loading', + ]); + expect(result.reason).toBe('diff'); + }); + test('gen-skill-docs.ts is a scoped touchfile, not global', () => { const result = selectTests(['scripts/gen-skill-docs.ts'], E2E_TOUCHFILES); // Should select tests that list gen-skill-docs.ts in their touchfiles, not ALL tests