diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4ff86a04f..d9e8082cd 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -91,7 +91,7 @@ The server writes `.gstack/browse.json` (atomic write via tmp + rename, mode 0o6 { "pid": 12345, "port": 34567, "token": "uuid-v4", "startedAt": "...", "binaryVersion": "abc123" } ``` -The CLI reads this file to find the server. If the file is missing or the daemon process is dead, the CLI spawns a new server. A process that is alive but not answering `/health` is busy, not dead: the CLI probes for a bounded ~8s, then reports busy with a nonzero exit — only an explicit `--force-restart` kills a live daemon. Process liveness uses signal-0 (`isProcessAlive`, EPERM counts as alive) on every platform, with the health check (GET /health) as the responsiveness signal. Daemon stdout/stderr persists to `/.gstack/browse-daemon.log`. +The CLI reads this file to find the server. If the file is missing or the daemon process is dead, the CLI spawns a new server. A process that is alive but not answering `/health` is busy, not dead: the CLI probes for a bounded ~8s, then reports busy with a nonzero exit — only an explicit `--force-restart` kills a live daemon. Process liveness uses signal-0 (`isProcessAlive`, EPERM counts as alive) on every platform, with the health check (GET /health) as the responsiveness signal. Its `status` covers the daemon and its Chromium connection only; the active tab's main thread is reported separately (`activeTab.responsive`, a probe bounded to 1s), so one page stuck in a long script never makes the CLI refuse commands. Daemon stdout/stderr persists to `/.gstack/browse-daemon.log`. ### Port selection diff --git a/BROWSER.md b/BROWSER.md index 396bb6e9b..3047da8f5 100644 --- a/BROWSER.md +++ b/BROWSER.md @@ -626,6 +626,14 @@ Storage stays intact by default. With explicit approval on a Chromium target, `- | `tab-each [args...]` | Fan out a command across every open tab; returns JSON | | `frame ` | Switch to iframe context (or back to main); clears refs | +**A busy tab is not a busy daemon.** `/health` `status` is daemon liveness +(Chromium connected); the active tab's main thread is reported separately as +`activeTab: { id, responsive }`, probed for at most 1s. While one page is stuck +in a long script, `status`, `tabs`, `tab `, `newtab`, `closetab`, +`state save` and `stop` keep working, and `status` and `tabs` mark the tab as +not responding. A command that needs the stuck tab times out after 30s and +names it, so you can switch away or close it without `--force-restart`. + ### Extraction | Command | Description | diff --git a/CHANGELOG.md b/CHANGELOG.md index abe8cadb7..7cce3cd8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,49 @@ - **…add a bin or a hook:** start it with the isolated shebang, and pass `bun --no-env-file --config=/dev/null` (or `bunIsolationArgs()` in TypeScript) wherever you start Bun. If you don't, the ratchet names the line. - **…relied on a repo `.env` to configure gstack:** export the variable in your shell or set it with `gstack-config`. +## [1.91.156.0] - 2026-10-19 + +**The weekly marathon's office-hours case can run its review formatter again, and its validator now checks cap fixes the way the formatter does.** Both defects came from #3164 and were in the test, not the skill. The marathon fixture copied `bin/gstack-office-hours-review` with a hand-kept list of two library files; #3164 added a third import (`lib/spec-review-cap.ts`), so every formatter command in the fixture failed at module load, the agent correctly declared the review UNREVIEWED, and the case went red in marathon run 39032308051 (#3246). With the module restored, a focused rerun showed the second defect: when the review stops at the round cap and the agent applies the remaining blocking fixes with `--fixes`, the formatter records them as verified, but the test never gave the validator that list, so it expected "fixes unconfirmed" and rejected a correct report. + +### Fixed +- **Office-hours cap fixes:** `observedSpecFixes()` in `test/helpers/office-hours-completion.ts` returns the last valid `--fixes` list the agent saved with Write, and the marathon test passes it to `validateOfficeHoursReviewArtifacts`. A free test in `test/office-hours-completion.test.ts` runs the real `finalize --fixes` at the cap and checks three cases: the observed list validates, no list fails with the marathon rerun's `Spec Review does not match` error, and a list naming a different finding also fails. +- **Office-hours marathon fixture:** `test/helpers/office-hours-formatter-fixture.ts` copies the formatter together with every local module it imports, so a new import can no longer be left out. A free test in `test/office-hours-review.test.ts` loads the copied formatter and fails on a missing module; the old three-file list reproduces the exact `Cannot find module '../lib/spec-review-cap.ts'` error. The case's touchfiles now include `lib/spec-review-cap.ts` and the new helper. +- **Gate integrity:** no threshold, trial count, case kind or judge changed. The marathon assertions are unchanged; only the fixture's file copy changed. + +## [1.91.155.0] - 2026-10-19 + +**A busy page no longer locks up the browse daemon.** When one tab kept its main thread busy (a heavy dev bundle, a big grid render, a runaway script), every `browse` command, including `status`, `tabs`, `tab `, `closetab` and `stop`, failed with "Daemon busy" after about 9.6 s, and the only way out was `--force-restart`, which threw away cookies, tabs and logins. Fixes #3247. From #3250 by @AndreIntelas. + +### Fixed +- **`/health` `status` reports daemon liveness (Chromium connected)**, and the active tab's responsiveness is reported separately as `activeTab: {id, responsive}`. The CLI no longer gates every command on a 2 s `page.evaluate` race against the active tab. +- **`tabs` and `state save` bound their per-tab reads**, so a stuck tab shows as `(not responding)` instead of hanging the command, and one pending probe is shared per stuck tab rather than one per poll. A busy tab keeps its last good storage in saved state. +- **A command that really needs the stuck tab times out with an error naming that tab** and how to get out (switch tabs or close it). The CLI's timeout branch now matches Bun's `TimeoutError`; the old `AbortError` check never fired. + +### Changed +- **Gate integrity:** `browse/test/terminal-agent.test.ts` adds `activeTab` to the exact `/health` keys and updates its stub for the now-synchronous `isHealthy` and the new `probeActiveTab`; its no-token assertions are unchanged. `browse/test/busy-active-tab.test.ts` and `browse/test/command-timeout-report.test.ts` are new. No threshold, trial count or case kind changed. +## [1.91.154.0] - 2026-10-19 + +**/cso can verify Rails repairs in a read-only checkout, its retained Git history no longer brings back credential files the snapshot leaves out, a repair whose external assertions fail is now reported as `ASSERTION_FAILED` instead of a misleading `INCOMPATIBLE_INPUT`, and a security assertion can accept several safe rejection statuses while still requiring the secret to be absent.** The helper keeps `.env`, `master.key`, keys and agent configuration out of the readable tree, but `history.txt` captured `git log -p` in full, so a committed `.env` came back through its diff whenever the value wasn't credential-shaped enough for the redactor. Paid qualification run 39186328569 caught this when the private corpus's planted `.env` value showed up in the judge's helper state. + +### Fixed +- **Rails `verify` on read-only sources:** the helper writes its own test `config/database.yml` (plus an initializer and `storage.yml`) into the execution copy. The copy keeps the source's file modes, so in a read-only checkout (the eval producer seals every source file at 0444, and Nix or Bazel trees are the same) the write failed with `EACCES`. That surfaced as the generic `INVALID_SCHEMA` "unexpected or unsafe input", and no Rails repair could reach `runtime_tested`. Each helper file now replaces the copied file instead of opening it for writing, so it also never writes through a copied symlink. +- **`history.txt`:** every diff section that touches a path the snapshot excludes (`exclusion()`, the same rule the readable tree uses) is replaced with `diff withheld: ()`. This covers either side of a rename and any diff header that can't be parsed. Commit preambles and other files' diffs stay. +- **`verify` with an authenticated witness:** a receipt or observation mismatch is still `INCOMPATIBLE_INPUT`. Receipts that bind correctly but record failed external assertions now raise `ASSERTION_FAILED`, with the phase and the booted, legitimate and security outcome. Nothing new is certified either way. In run 39186328569, three repairs that rejected the attack with a different status than the oracle expected surfaced as `INCOMPATIBLE_INPUT`. + +### Changed +- **A security assertion's `expected.status` may list up to eight safe statuses** (for example `[403, 404]`), so a repair that refuses an attack with a different reasonable rejection still passes. A set is accepted only on a security assertion that also carries a `vulnerable` expectation, and only with `excludes` set, so the response must still withhold the vulnerable marker. The fixed and vulnerable oracles must stay provably mutually exclusive: a set that contains the vulnerable status is accepted only when the vulnerable response's `includes` contains the excluded marker. Legitimate controls and `vulnerable` expectations still take exactly one status, so a patch that answers every route with 403 still fails its controls. A no-op patch, a listed rejection that still leaks the marker, and an unlisted status all stay `inconclusive`. + +## [1.91.153.0] - 2026-10-19 + +**An outside review whose findings are untagged prose no longer reads `clean` because a "no issues" phrase appears somewhere in it, and a clean Codex review that says "no actionable regressions" no longer reads `unverified`.** Reported in #3294 by @SchizasNik, with the false-pass half pointed out by @xg-gh-25. `lib/outside-review-result.ts` matched its no-findings vocabulary anywhere in the reply, so "The parser change has no new issues. However, … drops users_email_idx" passed /ship's structured gate. It also lacked `regressions`, so `codex review --uncommitted`'s "I found no actionable regressions in the staged, unstaged, or untracked changes." was missing coverage. + +### Fixed +- **A no-findings statement counts only as the review's conclusion.** It must open the reply's last sentence or last ", and" clause (or the `because` clause of a review's `Recommendation:` line) and carry no contrast or exception ("but", "however", "except", "other than", "elsewhere"). When other prose comes first, anything after the statement other than a predicate ("were found", "is evident") or a colon explanation must scope it to the change as a whole, so "No issues in the parser." and "No regressions were found by the existing tests." do not conclude the review. Anything else stays `unverified` / `untagged_review`, the same reason code and troubleshooting anchor as before. +- **`regressions` and `defects` join the vocabulary**, and `NO_FINDINGS` written as inline code still counts. + +### Gate integrity +- The structured and review gates' no-findings check (`classifyOutsideReview`) is stricter about position and wider in vocabulary. New free regressions in `test/outside-review-result.test.ts` cover the reporter's exact reply through their CLI invocation, two clean `codex review --uncommitted` replies captured from codex-cli 0.160.0 with gpt-6-sol (both read `unverified` before), fifteen mixed replies that must stay `unverified` under both gates (the two strings from the issue, the phrase before, after and inside untagged findings, exception conclusions, conclusions scoped to one place or to the tests, a quoted `NO_FINDINGS`, a no-issues line inside a code fence), and Recommendation-clause cases including a wrapped clause with "but". A GPT-6.1 Sol outside review probed the classifier; its false passes are among those controls. A known residual, unchanged from before: untagged findings followed by "No issues found in the changed parser." still read `clean`, because "changed" names the change and the parser cannot tell a finding from a summary without tags. All four new tests fail on the previous classifier. Every existing negative control passes unchanged, including the stored Codex clean review that still reads `unverified` ("no actionable correctness issues"). No threshold, trial count or case kind changed. + ## [1.91.152.0] - 2026-10-19 **`/cso` now starts and finishes on ordinary repositories that hold a nested Git repository, a long patch history, or a Docker socket on macOS 27.** Three user reports (#3255, #3248) each stopped a run on a normal repo. diff --git a/VERSION b/VERSION index 8219c59aa..59c1b7f37 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.91.152.0 +1.91.156.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index 942806459..b5dfb1d5f 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.91.152.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.91.156.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 338c6dd6b..7ad480813 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -290,6 +290,44 @@ export async function handleChromiumDisconnect(browser: Browser | null): Promise process.exit(1); } +/** + * How long a tab-independent read waits on one tab's renderer (#3247). A page + * stuck in a long script never answers page.evaluate() or page.title(); before + * this bound, one such tab made /health call the whole daemon unhealthy and + * hung `tabs`. Kept well under the CLI's /health request timeout + * (HEALTH_REQUEST_TIMEOUT_MS in cli.ts) so the CLI reads the answer instead of + * timing out on it. + */ +export const TAB_PROBE_TIMEOUT_MS = 1_000; + +/** + * Budget for saveState()'s per-tab storage read. Longer than the probe: a tab + * that misses it falls back to the last storage it answered with, which may + * be stale, so only a tab that is really stuck takes that path. + */ +const TAB_STORAGE_READ_TIMEOUT_MS = 5_000; + +type TabStorage = { localStorage: Record; sessionStorage: Record }; +type TabReadKind = 'probe' | 'title' | 'storage'; + +function urlOrigin(url: string): string { + return URL.canParse(url) ? new URL(url).origin : ''; +} + +/** + * Resolve with `work`'s value, or with undefined once `ms` passes. The losing + * read settles on its own when the renderer frees up, so callers pass a promise + * that cannot reject. + */ +function withinTabBudget(work: Promise, ms: number): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), ms); + (timer as { unref?: () => void }).unref?.(); + }); + return Promise.race([work, deadline]).finally(() => clearTimeout(timer)); +} + export type { RefEntry }; // Re-export TabSession for consumers @@ -300,7 +338,7 @@ export interface BrowserState { pages: Array<{ url: string; isActive: boolean; - storage: { localStorage: Record; sessionStorage: Record } | null; + storage: TabStorage | null; /** * HTML content loaded via load-html (setContent), replayed after context recreation. * In-memory only — never persisted to disk (HTML may contain secrets or customer data). @@ -360,6 +398,13 @@ export class BrowserManager { private extensionContext = false; /** Pending and recorded navigation-guard verdicts per page (D2). */ private navigationGuards = new WeakMap>; blocked: string | null }>(); + /** One pending read per page and kind (#3247). A read that outlives its + * budget stays queued in the renderer until the page frees up, so a stuck + * tab reuses it instead of collecting a new one on every /health poll, + * `tabs` call or persist tick. */ + private pendingTabReads = new WeakMap>>(); + /** The last storage each page answered with, and the origin it came from. */ + private lastTabStorage = new WeakMap(); // Our own 'disconnected' listeners per browser. Detach only these: removing // every 'disconnected' listener also strips Playwright's internal one that // resolves browser.close(), so each close waited the full race timeout (and @@ -1090,20 +1135,43 @@ export class BrowserManager { this.closeOwnedDisplay(); } - /** Health check — verifies Chromium is connected AND responsive */ - async isHealthy(): Promise { - if (!this.browser || !this.browser.isConnected()) return false; - try { - const page = this.pages.get(this.activeTabId); - if (!page) return true; // connected but no pages — still healthy - await Promise.race([ - page.evaluate('1'), - new Promise((_, reject) => setTimeout(() => reject(new Error('timeout')), 2000)), - ]); - return true; - } catch { - return false; + /** + * Daemon liveness for /health: Chromium is connected. The active tab is NOT + * part of it (#3247): a page stuck in a long script is one busy tab, not a + * dead daemon, and the CLI gates every command on this answer — including + * the tabs, tab, closetab and stop commands that get out of that state. + * probeActiveTab() reports the tab on its own. + */ + isHealthy(): boolean { + return Boolean(this.browser?.isConnected()); + } + + /** + * Does the active tab's main thread answer within `timeoutMs`? Null when + * there is no active page. A rejected evaluate (crashed or closing target) + * also counts as not responsive. + */ + async probeActiveTab(timeoutMs = TAB_PROBE_TIMEOUT_MS): Promise<{ id: number; responsive: boolean } | null> { + const id = this.activeTabId; + const page = this.pages.get(id); + if (!page) return null; + const probe = this.sharedTabRead(page, 'probe', () => page.evaluate('1').then(() => true, () => false)); + return { id, responsive: (await withinTabBudget(probe, timeoutMs)) === true }; + } + + /** Join the page's pending read of this kind, or start one. `read` must not + * reject. */ + private sharedTabRead(page: Page, kind: TabReadKind, read: () => Promise): Promise { + let reads = this.pendingTabReads.get(page); + if (!reads) { + reads = new Map(); + this.pendingTabReads.set(page, reads); } + const pending = reads.get(kind); + if (pending) return pending as Promise; + const started = read().finally(() => reads.delete(kind)); + reads.set(kind, started); + return started; } // ─── Tab Management ──────────────────────────────────────── @@ -1301,17 +1369,23 @@ export class BrowserManager { return released; } - async getTabListWithTitles(): Promise> { - const tabs: Array<{ id: number; url: string; title: string; active: boolean }> = []; - for (const [id, page] of this.pages) { - tabs.push({ + /** + * Every tab with its title. Titles are read in parallel, each bounded by + * TAB_PROBE_TIMEOUT_MS: a tab whose renderer is stuck comes back with + * `busy: true` and an empty title instead of hanging the list (#3247). + */ + async getTabListWithTitles(): Promise> { + return Promise.all([...this.pages].map(async ([id, page]) => { + const title = await withinTabBudget( + this.sharedTabRead(page, 'title', () => page.title().catch(() => '')), TAB_PROBE_TIMEOUT_MS); + return { id, url: page.url(), - title: await page.title().catch(() => ''), + title: title ?? '', active: id === this.activeTabId, - }); - } - return tabs; + busy: title === undefined, + }; + })); } // ─── Session Access ──────────────────────────────────────── @@ -1715,7 +1789,9 @@ export class BrowserManager { // ─── State Save/Restore (shared by recreateContext + handoff) ─ /** * Capture browser state: cookies, localStorage, sessionStorage, URLs, active tab. - * Skips pages that fail storage reads (e.g., already closed). + * Skips the storage of pages that fail the read (e.g., already closed). + * A page whose renderer is stuck past TAB_STORAGE_READ_TIMEOUT_MS does not + * hold up `state save` (#3247); see readTabStorage() for what it gets. */ async saveState(): Promise { if (!this.context) throw new Error('Browser not launched'); @@ -1725,13 +1801,7 @@ export class BrowserManager { for (const [id, page] of this.pages) { const url = page.url(); - let storage = null; - try { - storage = await page.evaluate(() => ({ - localStorage: { ...localStorage }, - sessionStorage: { ...sessionStorage }, - })); - } catch {} + const storage = await this.readTabStorage(page, url); // Capture load-html content so a later context recreation (viewport --scale) // can replay it via setTabContent. Never persisted to disk. @@ -1753,6 +1823,28 @@ export class BrowserManager { return { cookies, pages }; } + /** + * One tab's localStorage/sessionStorage for saveState(). A failed read + * (closed page, opaque origin) is null. A read that times out returns the + * last storage this page gave for the same origin: a periodic + * BROWSE_PERSIST_STATE snapshot must not replace good storage on disk with + * null just because the page is busy. + */ + private async readTabStorage(page: Page, url: string): Promise { + const origin = urlOrigin(url); + const read = await withinTabBudget(this.sharedTabRead(page, 'storage', () => page.evaluate(() => ({ + localStorage: { ...localStorage }, + sessionStorage: { ...sessionStorage }, + })).catch(() => null)), TAB_STORAGE_READ_TIMEOUT_MS); + if (read === undefined) { + const last = this.lastTabStorage.get(page); + return last?.origin === origin ? last.storage : null; + } + if (read) this.lastTabStorage.set(page, { origin, storage: read }); + else this.lastTabStorage.delete(page); + return read; + } + /** * Restore browser state into the current context: cookies, pages, storage. * Navigates to saved URLs, restores storage, wires page events. diff --git a/browse/src/cli.ts b/browse/src/cli.ts index d774d23d5..556947f0d 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -215,39 +215,50 @@ function readState(): ServerState | null { // isProcessAlive is imported from ./error-handling -/** - * HTTP health check — definitive proof the server is alive and responsive. - * Used in all polling loops instead of isProcessAlive() (which is slow on Windows). - */ -export async function isServerHealthy(port: number, timeoutMs = 2000): Promise { - try { - const resp = await fetch(`http://127.0.0.1:${port}/health`, { - signal: AbortSignal.timeout(timeoutMs), - }); - if (!resp.ok) return false; - const health = await resp.json() as any; - return health.status === 'healthy'; - } catch { - return false; - } -} +/** Client timeout for one GET /health. Must stay above the daemon's active-tab + * probe (TAB_PROBE_TIMEOUT_MS in browser-manager.ts): when the two were equal, + * the CLI aborted just before a busy tab's reply arrived and could not tell a + * busy tab from a hung daemon (#3247). */ +export const HEALTH_REQUEST_TIMEOUT_MS = 2000; -/** Best-effort tab count via GET /health (no auth, bounded). Returns null - * when the daemon doesn't answer in time or predates the `tabs` field — - * callers degrade to a countless phrasing, never block on this. */ -async function fetchDaemonTabCount(port: number, timeoutMs = 2000): Promise { +/** GET /health (no auth, bounded). Null when the daemon doesn't answer in + * time or the reply is not a 200. */ +async function fetchDaemonHealth(port: number, timeoutMs = HEALTH_REQUEST_TIMEOUT_MS): Promise | null> { try { const resp = await fetch(`http://127.0.0.1:${port}/health`, { signal: AbortSignal.timeout(timeoutMs), }); if (!resp.ok) return null; - const health = await resp.json() as any; - return typeof health.tabs === 'number' ? health.tabs : null; + return await resp.json() as Record; } catch { return null; } } +/** + * HTTP health check — definitive proof the server is alive and responsive. + * Used in all polling loops instead of isProcessAlive() (which is slow on Windows). + * `status` is daemon liveness only; a busy active tab does not fail it (#3247). + */ +export async function isServerHealthy(port: number, timeoutMs = HEALTH_REQUEST_TIMEOUT_MS): Promise { + return (await fetchDaemonHealth(port, timeoutMs))?.status === 'healthy'; +} + +/** Best-effort tab count via GET /health. Returns null when the daemon + * doesn't answer in time or predates the `tabs` field — callers degrade to a + * countless phrasing, never block on this. */ +async function fetchDaemonTabCount(port: number): Promise { + const tabs = (await fetchDaemonHealth(port))?.tabs; + return typeof tabs === 'number' ? tabs : null; +} + +/** The active tab's id when /health reports its main thread as not + * responding (#3247), else null — including daemons that predate the field. */ +async function fetchBusyActiveTab(port: number): Promise { + const activeTab = (await fetchDaemonHealth(port))?.activeTab as { id?: unknown; responsive?: unknown } | null | undefined; + return activeTab?.responsive === false && typeof activeTab.id === 'number' ? activeTab.id : null; +} + // ─── Process Management ───────────────────────────────────────── async function killServer(pid: number): Promise { if (!isProcessAlive(pid)) return; @@ -420,7 +431,7 @@ async function probeHealthWithBackoff( await Bun.sleep(intervalMs); const remainingMs = deadline - Date.now(); if (remainingMs <= 0) return false; - if (await isServerHealthy(port, Math.min(2000, remainingMs))) return true; + if (await isServerHealthy(port, Math.min(HEALTH_REQUEST_TIMEOUT_MS, remainingMs))) return true; } } @@ -1034,6 +1045,32 @@ export function extractTabId(args: string[]): { tabId: number | undefined; args: return { tabId, args: stripped }; } +/** Did this fetch fail because its AbortSignal fired? Bun rejects an + * AbortSignal.timeout() fetch with TimeoutError, not AbortError — matching + * only AbortError left the timeout reports below unreachable. */ +export function isFetchTimeout(err: unknown): boolean { + const name = (err as { name?: unknown } | null)?.name; + return name === 'AbortError' || name === 'TimeoutError'; +} + +/** + * What to print when a command's reply did not arrive within `timeoutMs`. + * #1781: a timeout on a heavy page usually means busy, not dead, so a live + * daemon is never restarted here. #3247: when /health names the active tab + * as not responding, say which tab and the commands that leave it without + * losing the session. + */ +export async function commandTimeoutReport(state: { pid: number; port: number } | null, timeoutMs: number): Promise { + const after = `[browse] Command timed out after ${timeoutMs / 1000}s`; + if (!state?.pid || !isProcessAlive(state.pid)) return [after]; + const busyTab = await fetchBusyActiveTab(state.port); + if (busyTab === null) return [`${after} (server still alive — busy, not restarting). Retry, or raise load.`]; + return [ + `${after}: tab ${busyTab} is not responding (its main thread is busy).`, + `[browse] The daemon, its other tabs and cookies still work: switch with 'browse tab ', close this one with 'browse closetab ${busyTab}', or retry once the page settles.`, + ]; +} + // ─── Command Dispatch ────────────────────────────────────────── export async function sendCommand(state: ServerState, command: string, args: string[], retries = 0): Promise { // Precedence: CLI --tab-id flag > BROWSE_TAB env var. @@ -1085,19 +1122,14 @@ export async function sendCommand(state: ServerState, command: string, args: str process.exit(1); } } catch (err: any) { - if (command === 'cookie-import-browser' && (['AbortError', 'TimeoutError'].includes(err.name) + if (command === 'cookie-import-browser' && (isFetchTimeout(err) || ['ECONNREFUSED', 'ECONNRESET'].includes(err.code) || err.message?.includes('fetch failed'))) { throw new Error('Cookie import response was lost or timed out. It may have partially completed; inspect the destination before retrying manually. The command was not replayed.'); } - if (err.name === 'AbortError') { - // #1781: a 30s timeout on a heavy page usually means busy, not dead. - // Don't kill a live server (that's what triggered the crash-loop) — report - // and exit so the user can retry rather than losing their (headed) window. - const ts = readState(); - const alive = ts?.pid ? isProcessAlive(ts.pid) : false; - console.error(alive - ? '[browse] Command timed out after 30s (server still alive — busy, not restarting). Retry, or raise load.' - : '[browse] Command timed out after 30s'); + if (isFetchTimeout(err)) { + // Report and exit so the user can retry rather than losing their + // (headed) window — killing a live server triggered the crash-loop. + for (const line of await commandTimeoutReport(readState(), timeoutMs)) console.error(line); process.exit(1); } // Connection error — server may have crashed, OR may just be busy. diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 8cc9e2e6c..af49910a9 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -265,7 +265,7 @@ export async function handleMetaCommand( case 'tabs': { const tabs = await bm.getTabListWithTitles(); return tabs.map(t => - `${t.active ? '→ ' : ' '}[${t.id}] ${t.title || '(untitled)'} — ${t.url}` + `${t.active ? '→ ' : ' '}[${t.id}] ${t.busy ? '(not responding)' : t.title || '(untitled)'} — ${t.url}` ).join('\n'); } @@ -404,12 +404,17 @@ export async function handleMetaCommand( case 'status': { const tabs = bm.getTabCount(); const mode = bm.getConnectionMode(); + const activeTab = await bm.probeActiveTab(); return [ `Status: healthy`, `Mode: ${mode}`, `URL: ${bm.getCurrentUrl()}`, `Tabs: ${tabs}`, `PID: ${process.pid}`, + // #3247: name the busy tab and the ways out that keep the session. + ...(activeTab && !activeTab.responsive + ? [`Active tab ${activeTab.id}: not responding. Switch with: browse tab , or close it: browse closetab ${activeTab.id}`] + : []), ].join('\n'); } diff --git a/browse/src/routes/core.ts b/browse/src/routes/core.ts index 491fc5f76..611448594 100644 --- a/browse/src/routes/core.ts +++ b/browse/src/routes/core.ts @@ -77,13 +77,19 @@ export const coreRoutes: RouteEntry[] = [ // Health check — no auth required, does NOT reset idle timer. NEVER carries // a token in any mode: token bootstrap is POST /extension-token and shell // auth is POST /pty-session. Liveness/status only. + // + // `status` is daemon liveness (Chromium connected); the CLI gates every + // command on it. `activeTab.responsive` is the active tab's main thread, + // probed for at most TAB_PROBE_TIMEOUT_MS and reported beside it: one busy + // page must not read as a dead daemon (#3247). { method: '*', path: '/health', auth: 'none', surfaces: ['local'], handler: async (_req, _r, ctx) => { const { browserManager } = ctx; - const healthy = await browserManager.isHealthy(); + const healthy = browserManager.isHealthy(); return json({ status: healthy ? 'healthy' : 'unhealthy', + activeTab: healthy ? await browserManager.probeActiveTab() : null, mode: browserManager.getConnectionMode(), uptime: Math.floor((Date.now() - ctx.startTime) / 1000), tabs: browserManager.getTabCount(), diff --git a/browse/test/busy-active-tab.test.ts b/browse/test/busy-active-tab.test.ts new file mode 100644 index 000000000..04a15861b --- /dev/null +++ b/browse/test/busy-active-tab.test.ts @@ -0,0 +1,132 @@ +/** + * #3247: one busy tab must not make the daemon look dead. A renderer stuck in + * a long script never answers page.evaluate() or page.title(). /health folded + * that into its `status`, so the CLI refused every command (status, tabs, + * tab, newtab, stop) with "Daemon busy" until --force-restart threw the + * session away, and `tabs` and `state save` hung on the tab as well. + * + * A real Chromium tab is held in a busy loop while a second tab stays idle; + * each surface must answer within its bound and name the busy tab, without + * piling up reads on the stuck renderer or dropping its persisted storage. + */ +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import type { Page } from 'playwright'; +import { BrowserManager, TAB_PROBE_TIMEOUT_MS } from '../src/browser-manager'; +import { HEALTH_REQUEST_TIMEOUT_MS } from '../src/cli'; +import { handleMetaCommand } from '../src/meta-commands'; +import { persistSessionState } from '../src/session-persist'; +import { callRoute, stubRouteContext } from './route-test-harness'; +import { startTestServer } from './test-server'; + +// Outlasts every check below; closing the busy tab at the end stops it sooner. +const BUSY_MS = 30_000; + +let testServer: ReturnType; +let bm: BrowserManager; +let busyId = 0; +let idleId = 0; +let busyPage: Page; +let profileDir = ''; +const originalProfile = process.env.CHROMIUM_PROFILE; + +beforeAll(async () => { + // Per-file profile, set in a hook (see commands.test.ts for why). + profileDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-busy-tab-profile-')); + process.env.CHROMIUM_PROFILE = profileDir; + testServer = startTestServer(0); + bm = new BrowserManager(); + await bm.launch(); + busyId = bm.getActiveTabId(); + await bm.getPage().goto(`${testServer.url}/basic.html`); + idleId = await bm.newTab(`${testServer.url}/basic.html`); + bm.switchTab(busyId, { bringToFront: false }); + busyPage = bm.getPage(); + // The good snapshot a BROWSE_PERSIST_STATE tick takes before the page hangs. + await busyPage.evaluate(() => localStorage.setItem('probe', 'kept')); + await bm.saveState(); + await busyPage.evaluate((ms) => { + setTimeout(() => { const end = Date.now() + ms; while (Date.now() < end) { /* busy */ } }, 50); + }, BUSY_MS); + await Bun.sleep(300); // let the loop start +}, 30_000); + +afterAll(async () => { + try { testServer.server.stop(true); } catch {} + try { await Promise.race([bm?.close(), Bun.sleep(3000)]); } catch {} + if (originalProfile === undefined) delete process.env.CHROMIUM_PROFILE; + else process.env.CHROMIUM_PROFILE = originalProfile; + if (profileDir) fs.rmSync(profileDir, { recursive: true, force: true }); +}); + +describe('busy active tab (#3247)', () => { + test('/health says the daemon is alive and names the busy tab before the CLI gives up', async () => { + // Equal budgets were the bug: the CLI aborted just before the reply landed. + expect(TAB_PROBE_TIMEOUT_MS).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + const ctx = stubRouteContext({ + browserManager: bm, + terminal: { readPort: () => null, grantToken: async () => true, restartSession: async () => true }, + }); + const started = Date.now(); + const body = await (await callRoute('GET', '/health', ctx)).json(); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(body.status).toBe('healthy'); + expect(body.activeTab).toEqual({ id: busyId, responsive: false }); + }); + + test('tabs lists every tab without waiting on the busy one', async () => { + const started = Date.now(); + const out = await handleMetaCommand('tabs', [], bm, async () => {}); + expect(Date.now() - started).toBeLessThan(HEALTH_REQUEST_TIMEOUT_MS); + expect(out).toContain(`[${busyId}] (not responding)`); + expect(out).toContain(`[${idleId}] Test Page - Basic`); + }); + + test('status stays healthy and names the busy tab with a way out', async () => { + const out = await handleMetaCommand('status', [], bm, async () => {}); + expect(out).toContain('Status: healthy'); + expect(out).toContain(`Active tab ${busyId}`); + expect(out).toContain(`closetab ${busyId}`); + }); + + test('a stuck tab gets one pending probe, not one per poll', async () => { + let evaluates = 0; + const evaluate = busyPage.evaluate.bind(busyPage); + Object.assign(busyPage, { evaluate: (...args: Parameters) => { evaluates++; return evaluate(...args); } }); + try { + for (let i = 0; i < 3; i++) expect(await bm.probeActiveTab()).toEqual({ id: busyId, responsive: false }); + } finally { + delete (busyPage as { evaluate?: unknown }).evaluate; // back to the prototype method + } + // The /health check above may already hold the pending probe. + expect(evaluates).toBeLessThanOrEqual(1); + }, 10_000); + + test('a persisted snapshot taken while the tab is busy keeps its last good storage', async () => { + const file = path.join(profileDir, 'session-state.json'); + await persistSessionState(bm, file); + const saved = JSON.parse(fs.readFileSync(file, 'utf8')); + expect(saved.pages).toHaveLength(2); + expect(saved.pages.find((p: { isActive: boolean }) => p.isActive).storage.localStorage).toEqual({ probe: 'kept' }); + expect(saved.pages.find((p: { isActive: boolean }) => !p.isActive).storage).not.toBeNull(); + }, 15_000); + + test('the idle tab still answers', async () => { + bm.switchTab(idleId, { bringToFront: false }); + try { + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + expect(await bm.getPage().title()).toBe('Test Page - Basic'); + } finally { + bm.switchTab(busyId, { bringToFront: false }); + } + }); + + test('closetab closes the busy tab and the session carries on', async () => { + const out = await handleMetaCommand('closetab', [String(busyId)], bm, async () => {}); + expect(out).toContain(`Closed tab ${busyId}`); + expect(bm.getTabCount()).toBe(1); + expect(await bm.probeActiveTab()).toEqual({ id: idleId, responsive: true }); + }, 15_000); +}); diff --git a/browse/test/command-timeout-report.test.ts b/browse/test/command-timeout-report.test.ts new file mode 100644 index 000000000..0505c0737 --- /dev/null +++ b/browse/test/command-timeout-report.test.ts @@ -0,0 +1,66 @@ +/** + * What the CLI prints when a command's reply never arrives (#1781, #3247). + * + * Bun rejects an AbortSignal.timeout() fetch with TimeoutError, so the old + * AbortError-only check skipped the busy report and printed "The operation + * timed out." instead. When the daemon's /health names its active tab as not + * responding, the report names that tab and the commands that leave it + * without a restart; a dead daemon gets the plain line. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { commandTimeoutReport, isFetchTimeout } from '../src/cli'; + +let servers: Array> = []; +afterEach(() => { for (const s of servers) s.stop(true); servers = []; }); + +/** A fake daemon whose /health answers `body`. */ +function fakeHealth(body: unknown): number { + const srv = Bun.serve({ port: 0, hostname: '127.0.0.1', fetch: () => Response.json(body) }); + servers.push(srv); + if (!srv.port) throw new Error('fake /health: no port'); + return srv.port; +} + +/** A pid that no longer runs: a child that has already exited. */ +function deadPid(): number { + return Bun.spawnSync([process.execPath, '-e', '']).pid; +} + +describe('isFetchTimeout', () => { + test('a real fetch whose AbortSignal.timeout fires counts as a timeout', async () => { + const silent = Bun.serve({ port: 0, hostname: '127.0.0.1', fetch: () => new Promise(() => {}) }); + servers.push(silent); + const err = await fetch(`http://127.0.0.1:${silent.port}/`, { signal: AbortSignal.timeout(50) }) + .then(() => null, (e: unknown) => e); + expect(isFetchTimeout(err)).toBe(true); + expect(isFetchTimeout(new TypeError('fetch failed'))).toBe(false); + }); +}); + +describe('commandTimeoutReport', () => { + test('names the busy tab and the way out, never a restart', async () => { + const port = fakeHealth({ status: 'healthy', activeTab: { id: 7, responsive: false } }); + const out = (await commandTimeoutReport({ pid: process.pid, port }, 30_000)).join('\n'); + expect(out).toContain('tab 7'); + expect(out).toContain('closetab 7'); + expect(out).not.toContain('--force-restart'); + }); + + test('a live daemon with a responsive or unreported tab gets the busy-not-restarting line', async () => { + for (const body of [ + { status: 'healthy', activeTab: { id: 1, responsive: true } }, + { status: 'healthy' }, // daemon from before activeTab existed + ]) { + const out = (await commandTimeoutReport({ pid: process.pid, port: fakeHealth(body) }, 30_000)).join('\n'); + expect(out).toContain('still alive'); + expect(out).not.toContain('closetab'); + } + }); + + test('a dead daemon gets only the timeout line', async () => { + const port = fakeHealth({ status: 'healthy', activeTab: { id: 7, responsive: false } }); + const out = await commandTimeoutReport({ pid: deadPid(), port }, 30_000); + expect(out).toHaveLength(1); + expect(out[0]).toContain('timed out after 30s'); + }); +}); diff --git a/browse/test/terminal-agent.test.ts b/browse/test/terminal-agent.test.ts index f066d1a23..c5b247ac1 100644 --- a/browse/test/terminal-agent.test.ts +++ b/browse/test/terminal-agent.test.ts @@ -94,13 +94,16 @@ describe('/health does NOT surface ptyToken', () => { test('/health response body does not include ptyToken', async () => { // It's allowed to include `terminalPort` (a port number, not auth). const ctx = stubRouteContext({ - browserManager: { isHealthy: async () => true, getConnectionMode: () => 'launched', getTabCount: () => 1 } as any, + browserManager: { + isHealthy: () => true, probeActiveTab: async () => ({ id: 1, responsive: true }), + getConnectionMode: () => 'launched', getTabCount: () => 1, + } as any, terminal: { readPort: () => 4242, grantToken: async () => true, restartSession: async () => true }, }); const text = await (await callRoute('GET', '/health', ctx)).text(); const body = JSON.parse(text); expect(body.terminalPort).toBe(4242); - expect(Object.keys(body).sort()).toEqual(['mode', 'status', 'tabs', 'terminalPort', 'uptime']); + expect(Object.keys(body).sort()).toEqual(['activeTab', 'mode', 'status', 'tabs', 'terminalPort', 'uptime']); expect(text).not.toContain('ptyToken'); expect(text).not.toContain('gstack_pty'); }); diff --git a/docs/REMOTE_BROWSER_ACCESS.md b/docs/REMOTE_BROWSER_ACCESS.md index c5565ba0e..902df6efb 100644 --- a/docs/REMOTE_BROWSER_ACCESS.md +++ b/docs/REMOTE_BROWSER_ACCESS.md @@ -81,7 +81,9 @@ Response: (plain text result of the command) ``` #### GET /health -Server status. No auth required. Returns status, tabs, mode, uptime. +Server status. No auth required. Returns status, activeTab, tabs, mode, uptime. +`status` is daemon liveness; `activeTab` (`{ id, responsive }`, or null with no +page) says whether the active tab's main thread answered within 1s. Never carries a token — extension token bootstrap is `POST /extension-token` (local listener only, validates the pinned `chrome-extension://` Origin and a loopback Host; 403 otherwise). Not reachable over the tunnel (404). diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 15094f27e..1553aa67a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -172,9 +172,17 @@ the readiness dashboard and the PR body, and never count it as a pass. review, including the `execution` gate behind `/codex` consult and challenge. A reply whose last `Verdict:` line says `not ready` but carries no readable severity prints `FINDINGS: unspecified` and exits 3 like a blocking finding. -- **No findings.** A review with no tag and no label is `clean` only when it - says so explicitly (`NO_FINDINGS`, "no issues", "did not find any bugs"). - Otherwise it is `unverified` (see below), never `clean`. A `/codex` consult +- **No findings.** A review with no tag and no label is `clean` only when its + conclusion says so: the last sentence (or its last ", and" clause), or the + `because` clause of its `Recommendation:` line, opens with `NO_FINDINGS`, + "No issues found", "I found no actionable regressions" or "I did not find + any bugs", with no "but", "however", "except", "other than" or "elsewhere". + When other prose comes first, anything after that statement except a + predicate ("were found", "is evident") or a colon explanation must scope it + to the change as a whole ("in this change", "in the staged changes"); "No + issues in the parser." does not conclude the review. A "no issues" phrase + anywhere else in the reply does not count. Otherwise the review is + `unverified` (see below), never `clean`. A `/codex` consult answer with no severity labels at all is not a review summary and stays `clean`. - **Design proposals** (the design-direction voices in /design-consultation diff --git a/lib/cso/contracts.ts b/lib/cso/contracts.ts index 1f06b4565..c1bc250af 100644 --- a/lib/cso/contracts.ts +++ b/lib/cso/contracts.ts @@ -180,7 +180,7 @@ export interface HttpAssertion { method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE'; headers?: Record; body?: string; - expected: { status: number; includes?: string; excludes?: string }; + expected: { status: number | number[]; includes?: string; excludes?: string }; vulnerable?: { status: number; includes?: string; excludes?: string }; } export interface Command { @@ -648,16 +648,34 @@ export function validateVerificationObservation(value: unknown): VerificationObs inputHash: v.inputHash, }; } -function assertion(value: unknown, name: string): HttpAssertion { +const MAX_SAFE_STATUSES = 8; +function assertion(value: unknown, name: string, safeStatusSet = false): HttpAssertion { const v = object(value, name), expected = object(v.expected, `${name}.expected`); exact(v, ['name', 'path', 'method', 'headers', 'body', 'expected', 'vulnerable'], name); - const oracle = (x: Record, n: string) => { + const httpStatus = (s: unknown) => Number.isInteger(s) && (s as number) >= 100 && (s as number) <= 599; + const oracle = (x: Record, n: string, statusSet = false) => { exact(x, ['status', 'includes', 'excludes'], n); - if (!Number.isInteger(x.status) || x.status < 100 || x.status > 599) - throw new CsoError('INVALID_SCHEMA', `${n}.status must be an HTTP status`); + if (!Array.isArray(x.status)) { + if (!httpStatus(x.status)) throw new CsoError('INVALID_SCHEMA', `${n}.status must be an HTTP status`); + } else if (!statusSet) throw new CsoError('INVALID_SCHEMA', `${n}.status must be an HTTP status`); + else if ( + !x.status.length || + x.status.length > MAX_SAFE_STATUSES || + !x.status.every(httpStatus) || + new Set(x.status).size !== x.status.length + ) + throw new CsoError( + 'INVALID_SCHEMA', + `${n}.status must be 1..${MAX_SAFE_STATUSES} distinct HTTP statuses`, + ); + else if (typeof x.excludes !== 'string' || !x.excludes) + throw new CsoError( + 'INVALID_SCHEMA', + `${n} with a set of safe statuses must exclude the vulnerable marker`, + ); return { - status: x.status, + status: Array.isArray(x.status) ? [...x.status] : x.status, ...(x.includes === undefined ? {} : { includes: string(x.includes, `${n}.includes`) }), ...(x.excludes === undefined ? {} : { excludes: string(x.excludes, `${n}.excludes`) }), }; @@ -683,7 +701,7 @@ function assertion(value: unknown, name: string): HttpAssertion { method: oneOf(v.method, ['GET', 'POST', 'PUT', 'PATCH', 'DELETE'], `${name}.method`), ...(Object.keys(headers).length ? { headers } : {}), ...(v.body === undefined ? {} : { body: string(v.body, `${name}.body`, 65536) }), - expected: oracle(expected, `${name}.expected`), + expected: oracle(expected, `${name}.expected`, safeStatusSet && v.vulnerable !== undefined), ...(v.vulnerable === undefined ? {} : { vulnerable: oracle(object(v.vulnerable), `${name}.vulnerable`) }), }; } @@ -692,13 +710,13 @@ export function securityAssertions(security: VerificationRequest['security']): H return Array.isArray(security) ? security : [security]; } function securityAssertion(value: unknown): VerificationRequest['security'] { - if (!Array.isArray(value)) return assertion(value, 'security'); + if (!Array.isArray(value)) return assertion(value, 'security', true); if (!value.length || value.length > MAX_SECURITY_ASSERTIONS) throw new CsoError( 'INVALID_SCHEMA', `security must be one assertion or 1..${MAX_SECURITY_ASSERTIONS} assertions`, ); - const assertions = value.map((x, i) => assertion(x, `security[${i}]`)); + const assertions = value.map((x, i) => assertion(x, `security[${i}]`, true)); if (new Set(assertions.map((x) => x.name)).size !== assertions.length) throw new CsoError('INVALID_SCHEMA', 'security assertion names must be distinct'); return assertions; @@ -805,7 +823,9 @@ export function validateVerificationRequest(input: unknown): VerificationRequest 'Verification needs a legitimate control, distinct before/fixed security oracles, existing tests, immutable test files, and boundary files', ); const mutuallyExclusive = ({ expected: secure, vulnerable }: HttpAssertion) => - secure.status !== vulnerable!.status || + (Array.isArray(secure.status) + ? !secure.status.includes(vulnerable!.status) + : secure.status !== vulnerable!.status) || (secure.includes !== undefined && vulnerable!.excludes !== undefined && secure.includes.includes(vulnerable!.excludes)) || diff --git a/lib/cso/history.ts b/lib/cso/history.ts index 86c6159cd..92c8b8082 100644 --- a/lib/cso/history.ts +++ b/lib/cso/history.ts @@ -57,6 +57,29 @@ export function gitDiffHeaderPaths(line: string): [string, string] | undefined { } } +/** + * Replace every diff section touching a path `withheld` names with one marker line, so a file the snapshot keeps + * out of the readable tree cannot come back through retained history. An unparseable diff header is withheld too. + */ +export function withholdHistory(raw: string, withheld: (path: string) => string | undefined): string { + const output: string[] = []; + let skipping = false; + for (const line of raw.split('\n')) { + if (line.startsWith('commit ')) skipping = false; + else if (line.startsWith('diff --git ')) { + const paths = gitDiffHeaderPaths(line)?.map((value) => value.replace(/^[ab]\//, '')), + reason = paths ? paths.map(withheld).find(Boolean) : 'unparseable diff header'; + skipping = Boolean(reason); + if (reason) { + output.push(`diff withheld: ${paths?.[1] ?? 'unknown path'} (${reason})`); + continue; + } + } + if (!skipping) output.push(line); + } + return output.join('\n'); +} + /** Return only exact path hunks, keeping one commit preamble per matching commit. */ export function historyForPath(raw: string, path: string): string | undefined { const expected = new Set([`a/${path}`, `b/${path}`]), diff --git a/lib/cso/preparation-docker.ts b/lib/cso/preparation-docker.ts index e36e0a34d..7874d8762 100644 --- a/lib/cso/preparation-docker.ts +++ b/lib/cso/preparation-docker.ts @@ -1106,6 +1106,25 @@ export async function superviseRegistrySocket(options: { * Requires qualified runtime images to contain the fixed, compiled * /opt/cso/preparation@1.0.0 helper contract. */ +/** + * Write the helper's own files (Rails' test database.yml and friends) into an execution copy. The copy keeps the + * source's modes, and a sealed or read-only checkout makes those files 0444, so each copied file is replaced, never + * opened for writing (or written through). + */ +export function writeOfflineTransformations( + source: string, + transformations: OfflinePreparationRequest['transformations'], +): void { + for (const transformation of transformations) { + if (sha256(transformation.content) !== transformation.sha256) + fail('INCOMPATIBLE_INPUT', 'Offline transformation content hash changed'); + const file = contained(source, transformation.path); + secureDirectory(resolve(file, '..')); + fs.rmSync(file, { force: true }); + fs.writeFileSync(file, transformation.content, { mode: 0o600, flag: 'wx' }); + } +} + export class DockerPreparationSandboxRunner implements PreparationSandboxRunner { readonly qualification; private readonly runtime; @@ -1514,13 +1533,7 @@ export class DockerPreparationSandboxRunner implements PreparationSandboxRunner errorOnExist: false, preserveTimestamps: false, }); - for (const transformation of request.transformations) { - if (sha256(transformation.content) !== transformation.sha256) - fail('INCOMPATIBLE_INPUT', 'Offline transformation content hash changed'); - const file = contained(source, transformation.path); - secureDirectory(resolve(file, '..')); - fs.writeFileSync(file, transformation.content, { mode: 0o600 }); - } + writeOfflineTransformations(source, request.transformations); for (const item of request.metadata) { if (sha256(item.content) !== item.sha256) fail('INCOMPATIBLE_INPUT', `Offline metadata hash changed: ${item.path}`); diff --git a/lib/cso/snapshot.ts b/lib/cso/snapshot.ts index feb41b43e..7899dc0fd 100644 --- a/lib/cso/snapshot.ts +++ b/lib/cso/snapshot.ts @@ -14,6 +14,7 @@ import { snapshotPathId, MAX_OUTPUT, } from './contracts'; +import { withholdHistory } from './history'; import { childEnvironment, executable, git, redact, runProcess } from './process'; import { boundedList, readJson, secureDirectory, writeHelperJson, writeJson } from './state'; import { scan } from '../redact-engine'; @@ -952,7 +953,9 @@ export async function capture( if (!(error instanceof CsoError) || error.code !== 'MISSING_INPUT') throw error; } guard(); - const commits = (patched ?? '').split(/^(?=commit [0-9a-f]{40}(?:[0-9a-f]{24})?$)/m).filter(Boolean), + const commits = withholdHistory(patched ?? '', exclusion) + .split(/^(?=commit [0-9a-f]{40}(?:[0-9a-f]{24})?$)/m) + .filter(Boolean), sizes = commits.map((commit) => Buffer.byteLength(commit)), whole = sizes.reduce((sum, size) => sum + size, 0) <= MAX_OUTPUT; let kept = 0, diff --git a/lib/cso/verification.ts b/lib/cso/verification.ts index 6ccf9ef83..4b4807ff2 100644 --- a/lib/cso/verification.ts +++ b/lib/cso/verification.ts @@ -1219,14 +1219,21 @@ export function certify(params: { beforeReceipt.observationHash !== witnessObservationHash(beforeObservation) || afterReceipt.observationHash !== witnessObservationHash(afterObservation) || params.before.existingTests !== beforeReceipt.diagnosticTestsPassed || - params.after.existingTests !== afterReceipt.diagnosticTestsPassed || - !beforeReceipt.externalAssertionsPassed || - !afterReceipt.externalAssertionsPassed + params.after.existingTests !== afterReceipt.diagnosticTestsPassed ) throw new CsoError( 'INCOMPATIBLE_INPUT', 'Authenticated assertion witness receipts do not match the verifier observations', ); + for (const [phase, receipt] of [ + ['before', beforeReceipt], + ['after', afterReceipt], + ] as const) + if (!receipt.externalAssertionsPassed) + throw new CsoError( + 'ASSERTION_FAILED', + `External assertions failed in the ${phase} phase: booted ${params[phase].booted}, legitimate ${params[phase].legitimate}, security ${params[phase].security}`, + ); const stableExecutions = (receipt: AssertionWitnessReceipt) => receipt.executions.map(({ outputHash: _, ...execution }) => execution); if (canonical(stableExecutions(beforeReceipt)) !== canonical(stableExecutions(afterReceipt))) diff --git a/lib/cso/verifier.ts b/lib/cso/verifier.ts index b51551e2a..beb51a4e5 100644 --- a/lib/cso/verifier.ts +++ b/lib/cso/verifier.ts @@ -18,7 +18,7 @@ interface Config { } function matches(status: number, body: string, oracle: HttpAssertion['expected']): boolean { return ( - status === oracle.status && + (Array.isArray(oracle.status) ? oracle.status.includes(status) : status === oracle.status) && (oracle.includes === undefined || body.includes(oracle.includes)) && (oracle.excludes === undefined || !body.includes(oracle.excludes)) ); diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index e9871427d..8409c73b2 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -135,8 +135,56 @@ const SEVERITY_LABELS = [ function severityWords(text: string): Severity[] { return SEVERITY_LABELS.flatMap(re => [...text.matchAll(re)].map(m => SEVERITY_WORDS[m[1]!.toLowerCase()]!)); } -/** An explicit conclusion that the review found nothing to report. */ -const NO_FINDINGS = /\bNO_FINDINGS\b|\bno (?:actionable |significant |new |concrete )?(?:bugs?|issues?|findings?|problems?)\b|\b(?:did not|didn't|cannot|can't|could not|couldn't) (?:find|identify) any (?:actionable |new |concrete )?(?:bugs|issues|findings|problems)\b/i; +/** + * A no-findings statement opening a sentence: NO_FINDINGS (its underscore is + * gone after plainReview), "No issues found", "I found no actionable + * regressions", "I did not find any bugs". Never a phrase inside another + * clause ("renaming the flag is no problem for callers, but ..."). + */ +const FINDING_NOUN = '(?:(?:actionable|significant|new|concrete)\\s+)?(?:bugs?|issues?|findings?|problems?|regressions?|defects?)\\b'; +const NO_FINDINGS_LEAD = '(?:(?:overall|in summary|in short|summary|conclusion|result)[,:]?\\s+|(?:after|having|on|upon)\\s+(?:reviewing|reading|checking|inspecting|reviewed|read|checked|inspected)\\b[^,.;]{0,80},\\s+)?'; +const NO_FINDINGS = new RegExp(`^${NO_FINDINGS_LEAD}(?:NO_?FINDINGS\\b|(?:(?:i|we)\\s+(?:found|see|saw|identified)\\s+|there\\s+(?:are|were|is)\\s+)?no\\s+${FINDING_NOUN}|(?:i|we)\\s+(?:did not|didn't|could not|couldn't|cannot|can't)\\s+(?:find|identify|see)\\s+any\\s+${FINDING_NOUN})`, 'i'); +/** A contrast or exception qualifies the claim, so the sentence does not conclude that nothing was found. */ +const NO_FINDINGS_EXCEPTION = /(? sentence.trim()).filter(Boolean); + const total = sentences.length + (because ? 1 : 0); + return [sentences.at(-1), because].some(conclusion => { + const statement = conclusion?.match(NO_FINDINGS)?.[0]; + if (!conclusion || !statement || NO_FINDINGS_EXCEPTION.test(conclusion)) return false; + const qualifier = conclusion.slice(statement.length).replace(NO_FINDINGS_PREDICATE, '').replace(/[\s.!]+$/, '').trim(); + return total === 1 || !qualifier || qualifier.startsWith(':') || WHOLE_CHANGE_SCOPE.test(qualifier); + }); +} /** Formatting the requested marker in bold, inline code, or a list does not invalidate a completed review. */ function plainReview(text: string): string { @@ -159,7 +207,7 @@ function withoutCode(text: string): string { } if (marker) { fence = marker; return ''; } return line.replace(/`([^`]+)`/g, (span, inner: string, at: number) => - /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at)) ? inner : ' '); + inner.trim() === 'NO_FINDINGS' || /^\[?(?:P[0-3]|critical|high|medium|low)\]?$/i.test(inner.trim()) && CODE_LABEL_BEFORE.test(line.slice(0, at)) ? inner : ' '); }).join('\n'); } @@ -218,7 +266,7 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC const notReady = !findings.highest && verdictNotReady(plainCode); if (blocking || notReady) return { ...result('findings'), ...(unparsed ? { unparsed } : {}) }; if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed }; - if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !NO_FINDINGS.test(input.text)) { + if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !concludesNoFindings(code)) { return result('unverified', 'untagged_review', 'missing severity or explicit no-findings conclusion'); } return result('clean'); diff --git a/package.json b/package.json index 53f7c72f7..ccb208ff2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.91.152", + "version": "1.91.156", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/test/cso-contracts.test.ts b/test/cso-contracts.test.ts index a3434a54f..3f8f452be 100644 --- a/test/cso-contracts.test.ts +++ b/test/cso-contracts.test.ts @@ -18,8 +18,8 @@ const finding:any={title:'Cross-tenant read',rootCause:'Tenant query omits calle const request=(before:string,after:string)=>({findingId:'a'.repeat(32),runtimeProfile:'node',port:3456,start:{executable:'/usr/local/bin/node',args:['app.js']},legitimate:[{name:'health',path:'/health',method:'GET',expected:{status:200,includes:'ok'}}],security:{name:'tenant isolation',path:'/user?id=2',method:'GET',expected:{status:403},vulnerable:{status:200,includes:'tenant-b'}},existingTests:[{executable:'/usr/local/bin/node',args:['test.js']}],testFiles:['test.js'],fixtures:{'tenant.json':'{}'},boundaryFiles:['app.js'],changes:[{path:'app.js',beforeSha256:sha256(before),after,effect:'source'}],review:{reviewer:'independent-reviewer',independent:true,rootCauseRepaired:true,featurePreserved:true,boundaryMocks:false,rationale:'Predicate now binds the caller tenant and health behavior is preserved',reviewedPatchHash:''}}); // Structural signed fixture for bundle-reader mutation tests. Production // issuance uses AssertionWitnessSession in verifyRepair instead. -function authenticatedBundleFixture(certified:ReturnType,params:any,outcomes?:Record<'before'|'after',{executedTests:number;passingTests:number}>):any{ - const verification=structuredClone(certified.manifest),identityRequest=params.identityRequest??params.request,pair=crypto.generateKeyPairSync('ed25519'),publicKey=pair.publicKey.export({format:'der',type:'spki'}).toString('hex'),keyId=sha256(Buffer.from(publicKey,'hex')),createdAt=verification.createdAt,runner={testToolchain:params.testToolchain,startPlanHash:verification.startPlanHash,testPlanHash:verification.testPlanHash,commandsHash:sha256(canonical(params.request.existingTests)),minimumPassingTestsHash:sha256(canonical(params.request.existingTests.map(()=>1)))},make=(phase:'before'|'after')=>{const observation=verification[phase],counts=outcomes?.[phase]??{executedTests:1,passingTests:1},binding:any={schemaVersion:1,protocol:'gstack-cso-assertion-witness-v1',nonce:sha256(`${phase}:${createdAt}`),phase,issuedAt:createdAt,expiresAt:new Date(Date.parse(createdAt)+300_000).toISOString(),runId:params.runId,findingId:identityRequest.findingId,policyHash:verification.policyHash,auditPolicyHash:verification.auditPolicyHash,runtime:{image:params.runtime.image,verifierImage:params.verifier.image,platform:params.runtime.platform,profile:params.runtime.id},runner,sourceHash:phase==='before'?verification.beforeSourceHash:verification.afterSourceHash,dependencyHash:phase==='before'?verification.beforeDependencies:verification.afterDependencies,configurationHash:phase==='before'?verification.beforeConfiguration:verification.afterConfiguration,requestHash:verification.requestHash,patchHash:verification.patchHash,harnessHash:verification.harnessHash,assertionHash:sha256(canonical({legitimate:identityRequest.legitimate,security:identityRequest.security})),fixturesHash:verification.fixturesHash},executions=params.request.existingTests.map((command:any)=>({commandHash:sha256(canonical(command)),exitCode:0,outputHash:sha256(`structural signed fixture ${phase}`),minimumPassingTests:1,...counts,reportedPassed:counts.passingTests>=1})),unsigned:any={schemaVersion:1,binding,keyId,publicKey,observationHash:witnessObservationHash(observation),externalAssertionsPassed:true,diagnosticTestsPassed:executions.every((execution:any)=>execution.reportedPassed),executions};return{...unsigned,signature:crypto.sign(null,Buffer.from(canonical(unsigned)),pair.privateKey).toString('hex')}};const witness={before:make('before'),after:make('after')};verification.assertionAssurance='authenticated_out_of_process';verification.testCompletionAssurance='self_reported';verification.witnessHash=assertionWitnessPairHash(witness);verification.result='runtime_tested';verification.id='';const id=verificationIdentity(verification);verification.id=id; +function authenticatedBundleFixture(certified:ReturnType,params:any,outcomes?:Record<'before'|'after',{executedTests:number;passingTests:number}>,externalAssertionsPassed:Record<'before'|'after',boolean>={before:true,after:true}):any{ + const verification=structuredClone(certified.manifest),identityRequest=params.identityRequest??params.request,pair=crypto.generateKeyPairSync('ed25519'),publicKey=pair.publicKey.export({format:'der',type:'spki'}).toString('hex'),keyId=sha256(Buffer.from(publicKey,'hex')),createdAt=verification.createdAt,runner={testToolchain:params.testToolchain,startPlanHash:verification.startPlanHash,testPlanHash:verification.testPlanHash,commandsHash:sha256(canonical(params.request.existingTests)),minimumPassingTestsHash:sha256(canonical(params.request.existingTests.map(()=>1)))},make=(phase:'before'|'after')=>{const observation=verification[phase],counts=outcomes?.[phase]??{executedTests:1,passingTests:1},binding:any={schemaVersion:1,protocol:'gstack-cso-assertion-witness-v1',nonce:sha256(`${phase}:${createdAt}`),phase,issuedAt:createdAt,expiresAt:new Date(Date.parse(createdAt)+300_000).toISOString(),runId:params.runId,findingId:identityRequest.findingId,policyHash:verification.policyHash,auditPolicyHash:verification.auditPolicyHash,runtime:{image:params.runtime.image,verifierImage:params.verifier.image,platform:params.runtime.platform,profile:params.runtime.id},runner,sourceHash:phase==='before'?verification.beforeSourceHash:verification.afterSourceHash,dependencyHash:phase==='before'?verification.beforeDependencies:verification.afterDependencies,configurationHash:phase==='before'?verification.beforeConfiguration:verification.afterConfiguration,requestHash:verification.requestHash,patchHash:verification.patchHash,harnessHash:verification.harnessHash,assertionHash:sha256(canonical({legitimate:identityRequest.legitimate,security:identityRequest.security})),fixturesHash:verification.fixturesHash},executions=params.request.existingTests.map((command:any)=>({commandHash:sha256(canonical(command)),exitCode:0,outputHash:sha256(`structural signed fixture ${phase}`),minimumPassingTests:1,...counts,reportedPassed:counts.passingTests>=1})),unsigned:any={schemaVersion:1,binding,keyId,publicKey,observationHash:witnessObservationHash(observation),externalAssertionsPassed:externalAssertionsPassed[phase],diagnosticTestsPassed:executions.every((execution:any)=>execution.reportedPassed),executions};return{...unsigned,signature:crypto.sign(null,Buffer.from(canonical(unsigned)),pair.privateKey).toString('hex')}};const witness={before:make('before'),after:make('after')};verification.assertionAssurance='authenticated_out_of_process';verification.testCompletionAssurance='self_reported';verification.witnessHash=assertionWitnessPairHash(witness);verification.result='runtime_tested';verification.id='';const id=verificationIdentity(verification);verification.id=id; return{schemaVersion:3,runId:params.runId,id,createdAt,expiresAt:new Date(Date.parse(createdAt)+30*86400_000).toISOString(),requiredInputs:{sourceHash:params.manifest.executionHash,originalHash:params.manifest.originalHash,runtimeImage:params.runtime.image,platform:params.runtime.platform,archives:params.archives,...(params.dependencyClosures?{dependencyClosures:params.dependencyClosures}:{})},request:identityRequest,verification,transformations:params.manifest.entries.filter((entry:any)=>entry.transformation),...(params.preparation?{preparation:params.preparation}:{}),witness}; } @@ -170,6 +170,14 @@ describe('tested repair certificate gate',()=>{ const params:any={runId:'run',manifest:{originalHash:'d'.repeat(64),executionHash:treeHash(before),entries:[]},request:req,runtime:{id:'node',image:'runtime@sha256:'+'a'.repeat(64),platform:'linux/amd64'},verifier:{image:'runtime@sha256:'+'a'.repeat(64)},before:{booted:true,legitimate:true,security:'intended_failure',existingTests:true,output:'before',inputHash:''},after:{booted:true,legitimate:true,security:'pass',existingTests:true,output:'after',inputHash:''},beforeRoot:before,afterRoot:after,policyHash:'c'.repeat(64),auditPolicyHash:'e'.repeat(64),archives:[],testToolchain:'runtime'},unsigned=certify(params),witness=authenticatedBundleFixture(unsigned,params,{before:{executedTests:2,passingTests:2},after:{executedTests:2,passingTests:1}}).witness; expect(()=>certify({...params,witness})).toThrow('existing-test execution count or outcome'); }); + test('a witness whose external assertions failed is an assertion failure, and a mismatched observation stays incompatible input',()=>{ + const root=tmp(),before=path.join(root,'before'),after=path.join(root,'after');fs.mkdirSync(before);fs.writeFileSync(path.join(before,'app.js'),'vulnerable\n');fs.writeFileSync(path.join(before,'test.js'),'one registered test\n');const raw:any=request('vulnerable\n','fixed\n');raw.review.reviewedPatchHash=patchHash(raw);const req=validateVerificationRequest(raw);preparePatchedSource(before,after,req); + const params:any={runId:'run',manifest:{originalHash:'d'.repeat(64),executionHash:treeHash(before),entries:[]},request:req,runtime:{id:'node',image:'runtime@sha256:'+'a'.repeat(64),platform:'linux/amd64'},verifier:{image:'runtime@sha256:'+'a'.repeat(64)},before:{booted:true,legitimate:true,security:'intended_failure',existingTests:true,output:'before',inputHash:''},after:{booted:true,legitimate:true,security:'inconclusive',existingTests:true,output:'after',inputHash:''},beforeRoot:before,afterRoot:after,policyHash:'c'.repeat(64),auditPolicyHash:'e'.repeat(64),archives:[],testToolchain:'runtime'},unsigned=certify(params); + const failed=authenticatedBundleFixture(unsigned,params,undefined,{before:true,after:false}).witness; + expect(()=>certify({...params,witness:failed})).toThrow(expect.objectContaining({code:'ASSERTION_FAILED',message:'External assertions failed in the after phase: booted true, legitimate true, security inconclusive'})); + const mismatched=authenticatedBundleFixture({...unsigned,manifest:{...unsigned.manifest,after:{...unsigned.manifest.after,output:'a different observation'}}},params,undefined,{before:true,after:false}).witness; + expect(()=>certify({...params,witness:mismatched})).toThrow(expect.objectContaining({code:'INCOMPATIBLE_INPUT'})); + }); test('canonical Python starts use direct prepared launchers and reject root framework shadows',()=>{ const pin=(name:string)=>`${name}==1.0.0 --hash=sha256:${'a'.repeat(64)}\n`; const flask=tmp();fs.writeFileSync(path.join(flask,'requirements.txt'),pin('Flask'));fs.writeFileSync(path.join(flask,'app.py'),'from flask import Flask\napp = Flask(__name__)\n');const flaskCommand=canonicalStartPlan(flask,'python',3456).command;expect(flaskCommand.executable).toBe('/work/.venv/bin/python');expect(flaskCommand.args.slice(0,3)).toEqual(['-I','-S','-c']);expect(flaskCommand.args[3]).toContain('from flask.cli import main');expect(flaskCommand.args.slice(4)).toEqual(['--app','app:app','run','--host','127.0.0.1','--port','3456']);fs.writeFileSync(path.join(flask,'flask.py'),'raise RuntimeError("shadow")\n');expect(()=>canonicalStartPlan(flask,'python',3456)).toThrow('root import shadow'); @@ -186,6 +194,17 @@ describe('tested repair certificate gate',()=>{ const runDir=tmp(),snapshot=path.join(runDir,'snapshot');fs.mkdirSync(snapshot);const files:Record={'package.json':JSON.stringify({scripts:{test:'jest'}}),'package-lock.json':'{}\n','app.js':'server.listen(3456)\n','app.test.js':'test("ok",()=>{})\n'};for(const [name,body] of Object.entries(files))fs.writeFileSync(path.join(snapshot,name),body);const plan=canonicalTestPlan(snapshot,'node'),raw:any=request(files['app.js'],'server.listen(3456)\n');raw.start={executable:'/usr/local/bin/node',args:['app.js']};raw.existingTests=plan.commands;raw.testFiles=plan.files;raw.changes=[{path:'package-lock.json',beforeSha256:sha256(files['package-lock.json']),after:'{"changed":true}\n',effect:'dependency'}];raw.review.reviewedPatchHash=patchHash(raw);const entries=Object.entries(files).map(([path,body])=>({path,originalHash:sha256(body),executionHash:sha256(body),bytes:Buffer.byteLength(body),mode:0o600})),manifest:any={version:3,createdAt:'2026-01-01T00:00:00Z',expiresAt:'2026-01-08T00:00:00Z',root:'/repo',headCommit:'a'.repeat(40),originalHash:'b'.repeat(64),executionHash:treeHash(snapshot),entries};let executed=false;await expect(verifyRepair({runId:'run',runDir,manifest,rawRequest:raw,runtime:{id:'node',stack:'node',image:'runtime@sha256:'+'a'.repeat(64),platform:'linux/amd64'} as any,verifier:{id:'node',stack:'node',image:'runtime@sha256:'+'a'.repeat(64),platform:'linux/amd64'} as any,policyHash:'c'.repeat(64),archives:[],executor:{observe:async()=>{executed=true;throw new Error('must not run');}},persist:false})).rejects.toMatchObject({code:'PREREQUISITE'});expect(executed).toBe(false); }); test('overlapping security oracles and no-op repairs cannot be certified',()=>{const overlapping:any=request('vulnerable\n','fixed\n');overlapping.review.reviewedPatchHash='pending';overlapping.security.vulnerable={...overlapping.security.expected};expect(()=>validateVerificationRequest(overlapping)).toThrow('mutually exclusive');const noop:any=request('vulnerable\n','vulnerable\n');noop.review.reviewedPatchHash='pending';expect(()=>validateVerificationRequest(noop)).toThrow('material patch');}); + test('a security expectation may list several safe statuses only while it excludes the vulnerable marker',()=>{ + const make=(edit:(raw:any)=>void)=>{const raw:any=request('vulnerable\n','fixed\n');raw.review.reviewedPatchHash='pending';edit(raw);return raw;}; + expect(validateVerificationRequest(make(raw=>{raw.security.expected={status:[403,404],excludes:'tenant-b'};})).security).toMatchObject({expected:{status:[403,404],excludes:'tenant-b'}}); + expect(validateVerificationRequest(make(raw=>{raw.security.expected={status:[200,403],excludes:'tenant-b'};})).security).toMatchObject({expected:{status:[200,403]}}); + expect(()=>validateVerificationRequest(make(raw=>{raw.security.expected={status:[200,403],excludes:'unrelated'};}))).toThrow('mutually exclusive'); + expect(()=>validateVerificationRequest(make(raw=>{raw.security.expected={status:[403,404]};}))).toThrow('must exclude the vulnerable marker'); + for(const status of [[],[403,403],[99,403],[400,401,403,404,405,409,410,422,429]])expect(()=>validateVerificationRequest(make(raw=>{raw.security.expected={status,excludes:'tenant-b'};}))).toThrow('distinct HTTP statuses'); + expect(()=>validateVerificationRequest(make(raw=>{raw.security.vulnerable={status:[200,206],includes:'tenant-b'};}))).toThrow('security.vulnerable.status must be an HTTP status'); + expect(()=>validateVerificationRequest(make(raw=>{raw.legitimate[0].expected={status:[200,403],excludes:'denied'};}))).toThrow('legitimate[0].expected.status must be an HTTP status'); + expect(()=>validateVerificationRequest(make(raw=>{raw.security=[{...raw.security,expected:{status:[403,404],excludes:'tenant-b'}},{...raw.security,name:'second',expected:{status:[403,404]}}];}))).toThrow('security[1].expected with a set of safe statuses'); + }); test('review is a separate content-bound but explicitly self-attested artifact',()=>{const raw:any=request('vulnerable\n','fixed\n');raw.review.reviewedPatchHash=patchHash(raw);const req=validateVerificationRequest(raw),artifact=makeReviewArtifact('run',req,'producer-session');raw.review.artifactId=artifact.id;const bound=validateVerificationRequest(raw);expect(validateReviewArtifact(artifact,'run',bound)).toMatchObject({producer:'producer-session',assurance:'self_attested'});expect(()=>makeReviewArtifact('run',req,req.review.reviewer)).toThrow('distinct');expect(()=>validateReviewArtifact({...artifact,rationale:'changed'},'run',bound)).toThrow('does not bind');}); test('executor observations have a strict helper-owned shape',()=>{expect(()=>validateVerificationObservation({booted:true,legitimate:true,security:'pass',existingTests:true,output:'ok',inputHash:'',tested:true})).toThrow('Unexpected');expect(()=>validateVerificationObservation({booted:'yes',legitimate:true,security:'pass',existingTests:true,output:'ok',inputHash:''})).toThrow('booleans');}); test('secret-bearing fixtures cannot enter a replayable verification bundle',async()=>{const runDir=tmp(),snapshot=path.join(runDir,'snapshot');fs.mkdirSync(snapshot);fs.writeFileSync(path.join(snapshot,'app.js'),'vulnerable\n');const raw:any=request('vulnerable\n','fixed\n');raw.review.reviewedPatchHash=patchHash(raw);raw.fixtures={'secret.txt':CREDENTIAL_CANARY};const manifest:any={entries:[{path:'app.js',transformation:undefined}]};const runtime:any={id:'node',image:'runtime@sha256:'+'a'.repeat(64),platform:'linux/amd64'};expect(verifyRepair({runId:'run',runDir,manifest,rawRequest:raw,runtime,verifier:runtime,policyHash:'c'.repeat(64),archives:[],executor:{observe:async()=>{throw new Error('must not execute');}},persist:false})).rejects.toMatchObject({code:'REDACTION_FAILED'});}); diff --git a/test/cso-git-hardening.test.ts b/test/cso-git-hardening.test.ts index 567b1e1f7..cbea74b7a 100644 --- a/test/cso-git-hardening.test.ts +++ b/test/cso-git-hardening.test.ts @@ -92,6 +92,20 @@ describe('CSO Git metadata hardening',()=>{ expect(history).not.toContain('replacement-history'); }); + test.skipIf(process.platform==='win32')('retained history withholds diffs of credential files the snapshot excludes',async()=>{ + const {repo,runDir,git}=fixture(); + fs.writeFileSync(path.join(repo,'.env'),'UPSTREAM_TOKEN=planted-history-value\n');fs.writeFileSync(path.join(repo,'tracked.ts'),'export const tracked = false\n'); + git('add','.env','tracked.ts');git('commit','-qm','configure upstream'); + + await capture(repo,runDir); + const history=fs.readFileSync(path.join(runDir,'history.txt'),'utf8'); + + expect(history).not.toContain('planted-history-value'); + expect(history).toContain('diff withheld: .env (credential or execution configuration)'); + expect(history).toContain('+export const tracked = false'); + expect(history).toContain('Subject: configure upstream'); + }); + test.skipIf(process.platform==='win32')('rejects repository config includes before Git can consume them',async()=>{ const {root,repo,runDir}=fixture(),included=path.join(root,'included.conf'); fs.writeFileSync(included,'[core]\n\tworktree = /tmp/cso-decoy\n'); diff --git a/test/cso-history.test.ts b/test/cso-history.test.ts index 0e32b107c..71cde803b 100644 --- a/test/cso-history.test.ts +++ b/test/cso-history.test.ts @@ -1,5 +1,5 @@ import { describe,expect,test } from 'bun:test'; -import { gitDiffHeaderPaths,historyForPath } from '../lib/cso/history'; +import { gitDiffHeaderPaths,historyForPath,withholdHistory } from '../lib/cso/history'; describe('CSO retained Git history path filtering',()=>{ test('matches exact paths without leaking a prefix sibling hunk',()=>{ @@ -25,6 +25,38 @@ diff --git a/foo-extra b/foo-extra expect(historyForPath(raw,'café file.ts')).toContain('+unicode-only'); }); + test('withholds every diff section touching an excluded path, keeping commit preambles and other files',()=>{ + const raw=`commit ${'c'.repeat(40)} +Subject: add config +diff --git a/.env b/.env +--- /dev/null ++++ b/.env ++PLANTED_VALUE=excluded-file-body +diff --git a/app.ts b/app.ts +--- a/app.ts ++++ b/app.ts ++kept-line +commit ${'d'.repeat(40)} +Subject: rename +diff --git a/config/old.txt b/config/master.key +rename from config/old.txt +rename to config/master.key ++renamed-into-excluded +diff --git "a/foo b/foo ++unparseable-body +`; + const kept=withholdHistory(raw,(path)=>/(^|\/)(\.env|master\.key)$/.test(path)?'credential or execution configuration':undefined); + expect(kept).not.toContain('excluded-file-body'); + expect(kept).not.toContain('renamed-into-excluded'); + expect(kept).not.toContain('unparseable-body'); + expect(kept).toContain('diff withheld: .env (credential or execution configuration)'); + expect(kept).toContain('diff withheld: config/master.key (credential or execution configuration)'); + expect(kept).toContain('diff withheld: unknown path (unparseable diff header)'); + expect(kept).toContain('+kept-line'); + expect(kept).toContain('Subject: add config'); + expect(kept).toContain('Subject: rename'); + }); + test('rejects malformed quoted headers instead of broadening the match',()=>{ expect(gitDiffHeaderPaths('diff --git "a/foo b/foo')).toBeUndefined(); expect(historyForPath('diff --git "a/foo b/foo\n+secret\n','foo')).toBeUndefined(); diff --git a/test/cso-preparation-container.test.ts b/test/cso-preparation-container.test.ts index eb8b42581..6cde0dc9a 100644 --- a/test/cso-preparation-container.test.ts +++ b/test/cso-preparation-container.test.ts @@ -3,8 +3,9 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { spawnSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; import { createPreparedExport, recordNpmArchiveEntry } from '../lib/cso/preparation-container'; -import { materializePreparedExport } from '../lib/cso/preparation-docker'; +import { materializePreparedExport, writeOfflineTransformations } from '../lib/cso/preparation-docker'; import { heldApplicationReady, validateSingleContainerProcessOutput } from '../lib/cso/docker'; const roots: string[] = []; @@ -68,3 +69,28 @@ describe('CSO preparation container archive limits', () => { expect(heldApplicationReady('PID COMMAND\n1 sh\n')).toBe(false); }); }); + +describe('offline transformations in an execution copy', () => { + test.skipIf(process.platform === 'win32')('replace a read-only copied file under a read-only directory instead of writing through it', () => { + const source = temporary(), config = path.join(source, 'config'), database = path.join(config, 'database.yml'), outside = path.join(temporary(), 'outside.yml'); + fs.mkdirSync(config); + fs.writeFileSync(database, 'production: sealed\n'); + fs.chmodSync(database, 0o444); + fs.chmodSync(config, 0o555); + fs.writeFileSync(outside, 'untouched\n'); + const content = 'test:\n adapter: sqlite3\n', sha256 = createHash('sha256').update(content).digest('hex'); + writeOfflineTransformations(source, [ + { path: 'config/database.yml', content, sha256, reason: 'helper test configuration' }, + { path: 'config/initializers/zzzz_cso_test.rb', content, sha256, reason: 'helper test configuration' }, + ]); + expect(fs.readFileSync(database, 'utf8')).toBe(content); + expect(fs.statSync(database).mode & 0o777).toBe(0o600); + expect(fs.readFileSync(path.join(config, 'initializers', 'zzzz_cso_test.rb'), 'utf8')).toBe(content); + expect(() => writeOfflineTransformations(source, [{ path: 'config/database.yml', content, sha256: 'f'.repeat(64), reason: 'tampered' }])).toThrow('Offline transformation content hash changed'); + fs.rmSync(database); + fs.symlinkSync(outside, database); + writeOfflineTransformations(source, [{ path: 'config/database.yml', content, sha256, reason: 'helper test configuration' }]); + expect(fs.lstatSync(database).isSymbolicLink()).toBe(false); + expect(fs.readFileSync(outside, 'utf8')).toBe('untouched\n'); + }); +}); diff --git a/test/cso-verifier.test.ts b/test/cso-verifier.test.ts index 48ab1e1b9..95cd725c0 100644 --- a/test/cso-verifier.test.ts +++ b/test/cso-verifier.test.ts @@ -65,6 +65,19 @@ describe('CSO verifier security judgement',()=>{ try{expect((await judgeSecurity('before',payloads,disproved.port)).security).toBe('pass');}finally{disproved.server.stop(true);} }); + test('a set of safe statuses passes any listed rejection that withholds the marker and nothing else',async()=>{ + const dotfile:HttpAssertion={name:'dotfile',path:'/.env',method:'GET',expected:{status:[403,404],excludes:'DOTFILE'},vulnerable:{status:200,includes:'DOTFILE'}}; + const answer=async(status:number,body:string)=>{ + const server=Bun.serve({port:0,hostname:'127.0.0.1',fetch:()=>new Response(body,{status})}); + try{return [(await judgeSecurity('before',dotfile,Number(server.port))).security,(await judgeSecurity('after',dotfile,Number(server.port))).security];}finally{server.stop(true);} + }; + expect(await answer(404,'not found')).toEqual(['pass','pass']); + expect(await answer(403,'denied')).toEqual(['pass','pass']); + expect(await answer(200,'DOTFILE=1')).toEqual(['intended_failure','inconclusive']); + expect(await answer(403,'denied DOTFILE=1')).toEqual(['inconclusive','inconclusive']); + expect(await answer(400,'bad request')).toEqual(['inconclusive','inconclusive']); + }); + test('a single assertion keeps its original before and after outcomes',async()=>{ const vulnerable=serve(new Set()),fixed=serve(ALL); try{ diff --git a/test/helpers/office-hours-completion.ts b/test/helpers/office-hours-completion.ts index 27b15b170..4ae26efe0 100644 --- a/test/helpers/office-hours-completion.ts +++ b/test/helpers/office-hours-completion.ts @@ -1,7 +1,7 @@ import { isDeepStrictEqual } from 'node:util'; import * as path from 'node:path'; import { renderOfficeHoursReviewerPrompt, validateOfficeHoursReview, renderOfficeHoursReview, extractOfficeHoursReviewBlock, parseOfficeHoursVerdictReceipt, verifyOfficeHoursVerdictReceipt, officeHoursDesignChanges, verifyOfficeHoursCitations, type OfficeHoursReview } from '../../lib/office-hours-review'; -import { verifySpecFixesAtCap, type SpecFix } from '../../lib/spec-review-cap'; +import { parseSpecFixes, verifySpecFixesAtCap, type SpecFix } from '../../lib/spec-review-cap'; export type OfficeHoursFile = { path: string; content: string | null }; /** The helper's captured diff before a later round; a missing snapshot fails closed. */ @@ -32,6 +32,21 @@ export interface OfficeHoursCompletionEvidence { fixes?: SpecFix[] | null; } +/** + * The cap fixes the agent saved for the finalizer: the last observed Write whose + * content is a valid `--fixes` list. Null when it saved none. The validator + * recomputes the cap record from this list, so a list that was not the one + * passed to finalize still produces a mismatch. + */ +export function observedSpecFixes(toolCalls: OfficeHoursCompletionEvidence['toolCalls']): SpecFix[] | null { + for (const call of toolCalls.toReversed()) { + if (call.tool !== 'Write') continue; + try { return parseSpecFixes(JSON.parse(String(call.input?.content ?? ''))); } + catch { /* not a fixes list */ } + } + return null; +} + export interface OfficeHoursReviewEvidence { verdict: string; concerns: string; diff --git a/test/helpers/office-hours-formatter-fixture.ts b/test/helpers/office-hours-formatter-fixture.ts new file mode 100644 index 000000000..0d11b3ea2 --- /dev/null +++ b/test/helpers/office-hours-formatter-fixture.ts @@ -0,0 +1,31 @@ +/** + * Copy bin/gstack-office-hours-review and every local module it imports into a + * fixture, so the copied formatter loads the same modules as the checkout. + * Marathon 39032308051 copied a hand-kept file list that missed + * lib/spec-review-cap.ts, and every formatter command failed at module load. + */ +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +const ROOT = path.resolve(import.meta.dir, '../..'); + +export function copyOfficeHoursFormatter(fixtureRoot: string): string { + const pending = ['bin/gstack-office-hours-review']; + const copied = new Set(); + while (pending.length) { + const relative = pending.pop()!; + if (copied.has(relative)) continue; + copied.add(relative); + const source = path.join(ROOT, relative); + const target = path.join(fixtureRoot, relative); + fs.mkdirSync(path.dirname(target), { recursive: true }); + fs.copyFileSync(source, target); + for (const [, specifier] of fs.readFileSync(source, 'utf8').matchAll(/\b(?:from|import)\s+'(\.\.?\/[^']+)'/g)) { + const resolved = path.relative(ROOT, path.resolve(path.dirname(source), specifier!)); + pending.push(path.extname(resolved) ? resolved : `${resolved}.ts`); + } + } + const formatter = path.join(fixtureRoot, 'bin/gstack-office-hours-review'); + fs.chmodSync(formatter, 0o755); + return formatter; +} diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 597bbda32..c8fee27dd 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -1099,7 +1099,7 @@ export const E2E_TOUCHFILES: Record = { 'test/skill-e2e-arm-benchmark.test.ts', 'ship/SKILL.md', 'test/helpers/e2e-gate.ts'], - 'office-hours-section-loading': [ 'office-hours/**', 'bin/gstack-office-hours-review', 'lib/office-hours-review.ts', 'lib/fs-atomic.ts', 'scripts/resolvers/review-dashboard.ts', 'scripts/resolvers/plan-gates.ts', 'scripts/resolvers/spec-review.ts', 'scripts/resolvers/outside-voice-steps.ts', 'scripts/resolvers/review-scope.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/carve-guards.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/office-hours-completion.ts', 'test/helpers/llm-judge.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-office-hours-section-loading.test.ts', 'test/helpers/agent-sdk-runner.ts', 'test/helpers/auq-native-capture.ts', 'test/helpers/auto-decision-state.ts', 'test/helpers/autoplan-artifact-digest.ts', 'test/helpers/autoplan-artifact-permission.ts', 'test/helpers/autoplan-artifact-recorder.ts', 'test/helpers/capture-parity-baseline.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/pty/**', 'test/helpers/dx-selected-navigation.ts', 'test/helpers/e2e-gate.ts', 'test/helpers/eng-cache-writer-decision.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/helpers/native-auto-decide.ts', 'test/helpers/owned-claude-transcript.ts', 'test/helpers/parity-harness.ts', 'test/helpers/plan-count-artifacts.ts', 'test/helpers/plan-count-file-permission.ts', 'test/helpers/plan-count-fixture.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/helpers/plan-count-pending-question.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-floor-review.ts', 'test/helpers/plan-floor-target.ts', 'test/helpers/plan-scope-selection.ts', 'test/helpers/plan-seed-submission.ts', 'test/helpers/plan-skill-question-events.ts', 'test/helpers/plan-skill-question-hook-scope.ts', 'test/helpers/plan-skill-questions.ts', 'test/helpers/pty-screen.ts', 'test/helpers/pty-trust-dialog.ts', 'test/helpers/skill-census.ts'], + 'office-hours-section-loading': [ 'office-hours/**', 'bin/gstack-office-hours-review', 'lib/office-hours-review.ts', 'lib/fs-atomic.ts', 'lib/spec-review-cap.ts', 'test/helpers/office-hours-formatter-fixture.ts', 'scripts/resolvers/review-dashboard.ts', 'scripts/resolvers/plan-gates.ts', 'scripts/resolvers/spec-review.ts', 'scripts/resolvers/outside-voice-steps.ts', 'scripts/resolvers/review-scope.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/carve-guards.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/office-hours-completion.ts', 'test/helpers/llm-judge.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-office-hours-section-loading.test.ts', 'test/helpers/agent-sdk-runner.ts', 'test/helpers/auq-native-capture.ts', 'test/helpers/auto-decision-state.ts', 'test/helpers/autoplan-artifact-digest.ts', 'test/helpers/autoplan-artifact-permission.ts', 'test/helpers/autoplan-artifact-recorder.ts', 'test/helpers/capture-parity-baseline.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/pty/**', 'test/helpers/dx-selected-navigation.ts', 'test/helpers/e2e-gate.ts', 'test/helpers/eng-cache-writer-decision.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/helpers/native-auto-decide.ts', 'test/helpers/owned-claude-transcript.ts', 'test/helpers/parity-harness.ts', 'test/helpers/plan-count-artifacts.ts', 'test/helpers/plan-count-file-permission.ts', 'test/helpers/plan-count-fixture.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/helpers/plan-count-pending-question.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-floor-review.ts', 'test/helpers/plan-floor-target.ts', 'test/helpers/plan-scope-selection.ts', 'test/helpers/plan-seed-submission.ts', 'test/helpers/plan-skill-question-events.ts', 'test/helpers/plan-skill-question-hook-scope.ts', 'test/helpers/plan-skill-questions.ts', 'test/helpers/pty-screen.ts', 'test/helpers/pty-trust-dialog.ts', 'test/helpers/skill-census.ts'], 'office-hours-design-draft': [ 'office-hours/**', 'lib/office-hours-review.ts', 'scripts/resolvers/review-dashboard.ts', 'scripts/resolvers/plan-gates.ts', 'scripts/resolvers/spec-review.ts', 'scripts/resolvers/outside-voice-steps.ts', 'scripts/resolvers/review-scope.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/carve-guards.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/office-hours-completion.ts', 'test/helpers/llm-judge.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-office-hours-design-draft.test.ts', 'test/helpers/agent-sdk-runner.ts', 'test/helpers/auq-native-capture.ts', 'test/helpers/auto-decision-state.ts', 'test/helpers/autoplan-artifact-digest.ts', 'test/helpers/autoplan-artifact-permission.ts', 'test/helpers/autoplan-artifact-recorder.ts', 'test/helpers/capture-parity-baseline.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/pty/**', 'test/helpers/dx-selected-navigation.ts', 'test/helpers/e2e-gate.ts', 'test/helpers/eng-cache-writer-decision.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/helpers/native-auto-decide.ts', 'test/helpers/owned-claude-transcript.ts', 'test/helpers/parity-harness.ts', 'test/helpers/plan-count-artifacts.ts', 'test/helpers/plan-count-file-permission.ts', 'test/helpers/plan-count-fixture.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/helpers/plan-count-pending-question.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-floor-review.ts', 'test/helpers/plan-floor-target.ts', 'test/helpers/plan-scope-selection.ts', 'test/helpers/plan-seed-submission.ts', 'test/helpers/plan-skill-question-events.ts', 'test/helpers/plan-skill-question-hook-scope.ts', 'test/helpers/plan-skill-questions.ts', 'test/helpers/pty-screen.ts', 'test/helpers/pty-trust-dialog.ts', 'test/helpers/skill-census.ts'], 'dx-error-contract-review': ['test/skill-e2e-dx-error-contract.test.ts', 'test/fixtures/dx-error-contract-case.ts', 'lib/dx-error-contract.ts', 'scripts/resolvers/dx.ts', 'scripts/resolvers/index.ts', 'scripts/gen-skill-docs.ts', 'plan-devex-review/**', 'bin/gstack-autoplan-snapshot.ts', 'lib/autoplan-prompts.ts', 'lib/autoplan-reconcile.ts', 'lib/coordinator-contract.ts', 'test/helpers/session-runner.ts', 'test/helpers/eval-store.ts', 'test/helpers/eval-budgets.ts', 'lib/claude-bin.ts', 'lib/eval-model.ts'], 'plan-devex-peer-comparison-classification': [ diff --git a/test/office-hours-completion.test.ts b/test/office-hours-completion.test.ts index 156ff7dd3..cf73e5eaa 100644 --- a/test/office-hours-completion.test.ts +++ b/test/office-hours-completion.test.ts @@ -4,7 +4,7 @@ import { describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; -import { validateOfficeHoursCompletion, validateOfficeHoursDesignDraft, validateOfficeHoursReviewerHandoffs, validateOfficeHoursReviewArtifacts, validateOfficeHoursReviewPreservation, type OfficeHoursCompletionEvidence } from './helpers/office-hours-completion'; +import { observedSpecFixes, validateOfficeHoursCompletion, validateOfficeHoursDesignDraft, validateOfficeHoursReviewerHandoffs, validateOfficeHoursReviewArtifacts, validateOfficeHoursReviewPreservation, type OfficeHoursCompletionEvidence } from './helpers/office-hours-completion'; import { E2E_TOUCHFILES } from './helpers/touchfiles-data'; import { selectTests } from './helpers/test-selection'; import { expectMentions } from './helpers/prompt-structure'; @@ -954,6 +954,42 @@ Next: /plan-eng-review after observing the workflow. The user declined launching } finally { fs.rmSync(dir, { recursive: true, force: true }); } }); + test('cap fixes the agent saved and passed to finalize validate; without them the report mismatches (marathon rerun 2026-10-19)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'office-hours-cap-fixes-')); + try { + const { evidence, artifacts } = JSON.parse(JSON.stringify(structured()) + .replaceAll('/tmp/office-hours-fixture', dir.replaceAll('\\', '/'))); + evidence.toolCalls.filter((call: { output?: string }) => call.output?.startsWith('OFFICE_HOURS_VERDICT ')) + .forEach((call: { output?: string }, i: number) => { call.output = officeHoursVerdictReceipt(i + 1, artifacts[i].path, artifacts[i].content); }); + const applied = 'An absent mapped column stops the import and names the missing column.'; + const fixes = [{ id: 'R2-1', applied_text: applied }]; + const reviewDir = path.dirname(artifacts[0].path); + const fixesPath = path.join(reviewDir, 'fixes.json'); + const reportPath = path.join(dir, 'REPORT.md'); + fs.mkdirSync(path.dirname(evidence.designPath), { recursive: true }); + fs.writeFileSync(evidence.designPath, evidence.designContent.replace('# Design: RosterCheck\n', `# Design: RosterCheck\n\n${applied}\n`)); + fs.writeFileSync(reportPath, evidence.output); + fs.mkdirSync(reviewDir, { recursive: true }); + for (const artifact of artifacts) fs.writeFileSync(artifact.path, artifact.content); + const snapshots = snapshotsIn(reviewDir); + for (const snapshot of snapshots) fs.writeFileSync(snapshot.path, snapshot.content); + fs.writeFileSync(fixesPath, JSON.stringify(fixes)); + const result = Bun.spawnSync([process.execPath, path.resolve(import.meta.dir, '../bin/gstack-office-hours-review'), + 'finalize', '--design', evidence.designPath, '--report', reportPath, '--fixes', fixesPath, ...artifacts.map((artifact: { path: string }) => artifact.path)], + { cwd: dir, timeout: 5000 }); + expect(result.exitCode, result.stderr.toString()).toBe(0); + expect(JSON.parse(result.stdout.toString()).cap.verified).toEqual(['R2-1']); + evidence.output = fs.readFileSync(reportPath, 'utf8'); + evidence.designContent = fs.readFileSync(evidence.designPath, 'utf8'); + evidence.toolCalls.push({ tool: 'Write', input: { file_path: fixesPath, content: JSON.stringify(fixes) } }); + expect(observedSpecFixes(evidence.toolCalls)).toEqual(fixes); + expect(() => checkArtifacts({ ...evidence, fixes: observedSpecFixes(evidence.toolCalls) }, artifacts, snapshots)).not.toThrow(); + expect(() => checkArtifacts(evidence, artifacts, snapshots)).toThrow('Spec Review does not match'); + expect(() => checkArtifacts({ ...evidence, fixes: [{ id: 'R2-2', applied_text: applied }] }, artifacts, snapshots)).toThrow('Spec Review does not match'); + expect(observedSpecFixes(evidence.toolCalls.filter((call: { input?: { file_path?: string } }) => call.input?.file_path !== fixesPath))).toBeNull(); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } + }); + test('allows a Markdown separator after the exact block, without hiding extra outcome prose', () => { const { evidence, artifacts } = structured(); evidence.output = evidence.output.replace('## Handoff', '---\n\n## Handoff'); diff --git a/test/office-hours-review.test.ts b/test/office-hours-review.test.ts index 7694d2218..f0a44d82b 100644 --- a/test/office-hours-review.test.ts +++ b/test/office-hours-review.test.ts @@ -9,6 +9,7 @@ import { } from '../lib/office-hours-review'; import { parseSpecFixes, verifySpecFixesAtCap, renderSpecCap } from '../lib/spec-review-cap'; import { expectMentions } from './helpers/prompt-structure'; +import { copyOfficeHoursFormatter } from './helpers/office-hours-formatter-fixture'; const cli = path.resolve(import.meta.dir, '../bin/gstack-office-hours-review'); function review(round = 1, count = 1, severity: 'blocking' | 'minor' = 'blocking'): OfficeHoursReview { @@ -638,3 +639,17 @@ describe('office-hours review CLI', () => { } })); }); + +describe('office-hours formatter fixture copy', () => { + test('the copied formatter loads every local module it imports (marathon 39032308051)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'office-hours-formatter-copy-')); + try { + const formatter = copyOfficeHoursFormatter(dir); + const result = Bun.spawnSync([process.execPath, formatter], { cwd: dir, stdout: 'pipe', stderr: 'pipe', timeout: 5000 }); + expect(result.stderr.toString()).not.toContain('Cannot find module'); + expect(result.stderr.toString()).toContain('gstack-office-hours-review: usage:'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts index 5d3a2ab7e..572b90f30 100644 --- a/test/outside-review-result.test.ts +++ b/test/outside-review-result.test.ts @@ -65,6 +65,67 @@ describe('classifyOutsideReview: separate execution, findings and verdict', () = }); }); +// #3294: "clean" comes only from a no-findings statement that concludes the reply, +// never from a "no X" phrase among untagged findings anywhere in it. +describe('a no-findings statement counts only as the reply\'s conclusion (#3294)', () => { + const REPORTER = 'I found no actionable regressions in the staged, unstaged, or untracked changes.'; + const FINDING = 'The fallback path now swallows ENOENT and returns an empty config.'; + + test("the reporter's exact codex review reply reads clean through the reporter's CLI invocation", () => { + expect(classifyOutsideReview({ text: REPORTER, gate: 'structured' }).verdict).toBe('clean'); + const r = cli(['--label', 'probe', '--exit', '0', '--stderr', file(''), 'structured', file(REPORTER)]); + expect([r.status, r.stdout]).toEqual([0, 'VERDICT: clean\nFINDINGS: none\n']); + }); + + test('clean conclusions, with the added regressions/defects vocabulary and the requested marker formatted', () => { + for (const text of ['No regressions found.', 'I did not find any defects.', 'There are no actionable regressions.', + 'NO_FINDINGS', '**NO_FINDINGS**', '`NO_FINDINGS`', '- NO_FINDINGS', + `The change adds a retry cap and its tests.\n${REPORTER}`, 'Reviewed the retry helper.\n\nNo issues found in this change.', + 'After reviewing the patch, I found no issues.', 'I found no issues in this read-only change.', + 'Summary of the retry helper\nNo issues found.', 'I found no actionable regressions in the staged\nchanges.', + // Clean `codex review --uncommitted` replies captured from codex-cli 0.160.0 with gpt-6-sol. + 'The change validates the retry limit before execution. No actionable regression was found.', + 'The change validates `max` before retrying. No affected callers or tests are present in the repository, and no actionable regression is evident.']) { + expect({ text, verdict: classifyOutsideReview({ text, gate: 'structured' }).verdict }).toEqual({ text, verdict: 'clean' }); + } + }); + + test('mixed replies: untagged findings beside a "no X" phrase are unverified, never clean', () => { + for (const text of [ + 'The parser change has no new issues. However, db/002_drop_idx.sql drops users_email_idx, which will regress login latency under load.', + 'Renaming the flag is no problem for callers, but the fallback path now swallows ENOENT and returns an empty config.', + `No issues in the parser.\n\n${FINDING}`, + `${FINDING}\n\nNo issues in the parser.`, + `${FINDING} No regressions in the existing tests.`, + 'No regressions in the existing tests, but the fallback path now swallows ENOENT.', + `${FINDING}\nI found no issues other than the fallback path.`, + `${FINDING}\nNo other issues.`, + 'No issues found, except that the fallback path now swallows ENOENT.', + 'No issues in the parser, and the fallback path now swallows ENOENT.', + 'Missing configuration now silently disables authentication. No regressions were found by the existing tests.', + 'Missing configuration now silently disables authentication. No issues are apparent in the parser.', + 'The fallback path now swallows ENOENT, and no issues elsewhere.', + 'The diff adds a NO_FINDINGS check, but the regex misses lowercase input.', + `${FINDING}\n\`\`\`\nNo issues found.\n\`\`\``, + ]) { + for (const gate of ['structured', 'review'] as const) { + const r = classifyOutsideReview({ text: gate === 'review' ? `${text}\nRecommendation: ship because it looks fine.` : text, gate }); + expect({ text, gate, verdict: r.verdict, reason: r.reason }).toEqual({ text, gate, verdict: 'unverified', reason: 'untagged_review' }); + } + } + }); + + test("a review's Recommendation because-clause concludes under the same rules", () => { + const verdict = (text: string) => classifyOutsideReview({ text, gate: 'review' }).verdict; + expect(verdict('Recommendation: ship because I found no regressions in this change.')).toBe('clean'); + expect(verdict(`The change adds a retry cap.\nRecommendation: ship because no issues remain.`)).toBe('clean'); + expect(verdict('No issues in the parser.\nRecommendation: fix the fallback because it swallows ENOENT.')).toBe('unverified'); + expect(verdict(`${FINDING}\nRecommendation: ship because no regressions in the existing tests.`)).toBe('unverified'); + expect(verdict(`${FINDING}\nRecommendation: ship because no issues remain, but the fallback needs a follow-up.`)).toBe('unverified'); + expect(verdict('Recommendation: address the migration because no regressions were found in the parser change,\nbut the migration drops users_email_idx.')).toBe('unverified'); + }); +}); + describe('validateOutsideReview keeps the text-only shape for direct importers', () => { test('existing shapes are unchanged', () => { expect(validateOutsideReview('', 'review')).toEqual({ completed: false, reason: 'empty response' }); diff --git a/test/skill-e2e-office-hours-section-loading.test.ts b/test/skill-e2e-office-hours-section-loading.test.ts index dcdfc1aec..c50b70747 100644 --- a/test/skill-e2e-office-hours-section-loading.test.ts +++ b/test/skill-e2e-office-hours-section-loading.test.ts @@ -14,7 +14,8 @@ import * as path from 'node:path'; import { describeE2ETier } from './helpers/e2e-gate'; import { setupSkillDir, skillFromWorktree, captureSectionReads } from './helpers/auq-sdk-capture'; import { CARVE_GUARDS } from './helpers/carve-guards'; -import { validateOfficeHoursCompletion, validateOfficeHoursReviewerHandoffs, validateOfficeHoursReviewArtifacts, validateOfficeHoursReviewPreservation } from './helpers/office-hours-completion'; +import { copyOfficeHoursFormatter } from './helpers/office-hours-formatter-fixture'; +import { observedSpecFixes, validateOfficeHoursCompletion, validateOfficeHoursReviewerHandoffs, validateOfficeHoursReviewArtifacts, validateOfficeHoursReviewPreservation } from './helpers/office-hours-completion'; const describeMarathon = describeE2ETier('marathon'); const runId = `office-hours-section-loading-${process.env.EVALS_RUN_ID ?? 'local'}`; @@ -42,15 +43,9 @@ describeMarathon('/office-hours full section-loading workflow (marathon)', () => const planDir = setupSkillDir({ skillName: guard.skill, skillMd, sectionsFrom, tmpPrefix: 'gstack-office-hours-secload-', }); - // Copy the real formatter and its small dependency set into the fixture. + // Copy the real formatter and its local imports into the fixture. // This exercises production code while keeping all outputs in the sandbox. - for (const relative of ['bin/gstack-office-hours-review', 'lib/office-hours-review.ts', 'lib/fs-atomic.ts']) { - const target = path.join(planDir, relative); - fs.mkdirSync(path.dirname(target), { recursive: true }); - fs.copyFileSync(path.resolve(__dirname, '..', relative), target); - } - const formatter = path.join(planDir, 'bin/gstack-office-hours-review'); - fs.chmodSync(formatter, 0o755); + const formatter = copyOfficeHoursFormatter(planDir); // Two rounds keep the live run inside its budget: round 1 (full) and round 2 (delta) run here; round 3 and CONVERGENCE are covered by free validator tests. const reviewRounds = 2; const capture = await captureSectionReads({ @@ -83,6 +78,7 @@ Completion delivery, after the full workflow and design approval: const reviewEvidence = validateOfficeHoursReviewArtifacts({ ...capture, designPath, designContent: fs.existsSync(designPath) ? fs.readFileSync(designPath, 'utf-8') : null, + fixes: observedSpecFixes(capture.toolCalls), }, artifacts, snapshots, reviewRounds); validateOfficeHoursReviewerHandoffs({ ...capture, designPath,