diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d04ebe8e..e2f917a25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,59 @@ All notable changes to GBrain will be documented in this file. +## [0.40.8.0] - 2026-05-23 + +**Your doctor checks, your operations trust boundary, and your cycle phases now have real behavioral tests — not just source-grep guards.** + +Before this release, three of gbrain's load-bearing surfaces shipped with structural tests only: the 50+ doctor checks were verified by grepping source code for check names; the 74 MCP operations were checked for scope annotations but no test proved that `localOnly: true` actually keeps an op off the HTTP wire; and the cycle phases that compose the maintenance loop had no test pinning their result-mapping (counter → status enum). When a refactor accidentally dropped a check or flipped a localOnly flag, the existing tests stayed green. The v0.38.2.0 partial-scan wave was one example — a render bug slipped through because no test drove the doctor orchestrator end-to-end. + +This wave adds the missing behavioral coverage. `gbrain doctor --json` now has a subprocess smoke test that runs the full render path against a fresh PGLite tempdir brain and asserts the schema_version=2 envelope, the status enum, and the check list integrity. The doctor's check-building logic was extracted into a small `buildChecks` function so behavioral tests can drive it directly without `process.exit` getting in the way — the wrapper still handles all 10 process.exit sites unchanged, snapshot-pinned so accidental check drop-outs fail loudly at PR review time. The operations trust-boundary surface gets a table-driven contract test over all 74 ops plus targeted handler-invocation regressions for the three historically-broken classes (HTTP MCP shell-job RCE, search_by_image image_path leak, plus the lint contract that file_upload's strict-mode confinement holds). A new shell guard in the `bun run verify` chain catches any future HTTP-exposing module that imports the operations list without applying the `localOnly` filter — the bypass that motivated the v0.26.9 hardening pass. + +Two pre-existing master-test flakes get root-caused as part of the same wave: `test/ai/gateway.test.ts` was leaking `OPENAI_API_KEY=openai-fake` into the gateway's module-scoped state, poisoning sibling tests in the same shard that called real `embed()` paths (afterAll cleanup added); and `test/ai/header-transport.test.ts` raced its synthetic recipe registration with sibling files that also mutated the gateway, returning `'[]'` instead of `'ok'` under shard parallelism (file quarantined as `.serial.test.ts`). Neither was a regression from this branch — both were latent bugs that surfaced because the new fast-loop coverage exercised more of the surface concurrently. + +### How to take advantage of v0.40.8.0 + +The added coverage is invisible at runtime. There's nothing to enable or configure — your next `bun run test` already includes 39 new test cases pinning the doctor, operations, and cycle contracts. If you've been wondering whether to trust `gbrain doctor`'s output after a refactor, the new behavioral tests are now the answer. + +If you maintain a downstream gbrain fork or extension that imports from `core/operations.ts`, run `bun run check:operations-filter-bypass` once. It'll catch any of your modules that brought the `operations` value in without applying the canonical `.filter(op => !op.localOnly)` filter — three import shapes are detected (destructured, aliased, namespace). + +### Itemized changes + +#### Doctor refactor (no behavior change, full coverage) + +- **`src/commands/doctor.ts:1604` — extract `buildChecks(engine, args, dbSource): Promise` from `runDoctor`.** The check-building logic moves into the new exported function; the existing exported `computeDoctorReport(checks)` at line 78 stays untouched. `runDoctor` is now a thin wrapper: `buildChecks → computeDoctorReport → render + process.exit`. All 10 `process.exit` sites stay where they are (lines 2199, 2220, the 5 inside `--locks` mode, and the remediation subcommands). The two early-return paths (no engine, connection failure) drop their inline `outputResults + process.exit` calls and `return checks` instead — the wrapper still produces identical observable output because both render the same partial check list. + +- **`test/doctor-behavioral.test.ts` (new, 13 cases).** Drives `buildChecks` against PGLite. Pure pure-aggregation cases pin `computeDoctorReport` math: 1 fail → -20 points, 3 fails → -60, mixed ok+warn+fail → unhealthy with the fail outcome dominating, score clamped at 0. Orchestrator cases assert `--fast` flag honors the skip set, `--json` arg doesn't alter the check list, the no-engine path returns a partial list without calling `process.exit`, and the snapshot of load-bearing check names (`connection`, `schema_version`, `brain_score`, `sync_freshness`, `search_mode`, `eval_drift`, `reranker_health`, `embedding_width_consistency`, `autopilot_lock_scope`) catches any accidental check drop-out during future refactors. + +- **`test/doctor-cli-smoke.serial.test.ts` (new, 1 case).** Subprocess smoke spawning `bun run src/cli.ts doctor --json` against a fresh PGLite tempdir brain via `GBRAIN_HOME=$(mktemp -d)`. Asserts exit code 0 on a freshly-initialized brain, the JSON envelope parses, `schema_version === 2`, `status` is one of `healthy`/`warnings`/`unhealthy`, and `checks` is a non-empty array. Catches render-path bugs that buildChecks-only tests miss — the class the v0.38.2.0 partial-scan wave exposed. Skip via `GBRAIN_SKIP_SUBPROCESS_TESTS=1` for shard-time control; quarantined as `.serial.test.ts` because PGLite write-locks don't play well with parallel runners. + +#### Operations trust-boundary contract + +- **`test/operations-trust-boundary.test.ts` (new, 14 cases).** Hybrid design: pure assertions over all 74 ops cover the cheap drift-detection win (every op has a scope annotation; every mutating op has a non-read scope; scope is one of the documented enum values; `hasScope(['read'], op.scope)` correctly rejects when op.scope is 'admin' or 'write'). Plus the canonical filter contract: every `localOnly: true` op is excluded from `operations.filter(op => !op.localOnly)`. Plus targeted handler-invocation cases for the two historically-broken HTTP-callable classes: `submit_job` with `name='shell'` + `ctx.remote=true` MUST reject (the F7b HTTP MCP shell-job RCE class), and `search_by_image` with `image_path` + `ctx.remote=true` MUST reject (the D18 P0 image-leak class). `file_upload` and `sync_brain` are deliberately omitted from handler-invocation tests because they're `localOnly: true` — calling their handlers directly would test an impossible production path (codex CMT-3 caught this during plan review). The seven historically-sensitive `localOnly` ops are snapshot-pinned by name: `sync_brain`, `file_upload`, `file_list`, `file_url`, `purge_deleted_pages`, `get_recent_transcripts`, `code_traversal_cache_clear`. + +- **`scripts/check-operations-filter-bypass.sh` (new shell guard, wired into `bun run verify`).** Greps `src/` for any module that imports the `operations` value from `core/operations.ts` outside the canonical filter site at `src/commands/serve-http.ts`. Three import shapes are detected: destructured (`import { operations }`), aliased (`import { operations as ops }`), and namespace (`import * as opsModule`). An explicit allow-list of 10 known-safe importers (CLI, dispatch, stdio MCP, the older http-transport, the tool-def helper, the subagent registry, three CLI tools, and serve-http itself) is documented with a one-line rationale per entry. Plus a check that `serve-http.ts` still contains the literal `operations.filter(op => !op.localOnly)` expression — refactoring it out fails the build. Type-only imports of sibling exports like `sourceScopeOpts` and `OperationContext` are deliberately not flagged. + +#### Cycle phase wrappers + +- **`src/core/cycle.ts:518, 552` — export `runPhaseLint` + `runPhaseBacklinks`.** Adds the `export` keyword to two existing phase functions so behavioral tests can drive them directly. No body changes; no behavior change. Documented as internal helpers exposed for test-only consumption — downstream code should NOT take a dependency on them. + +- **`test/cycle-legacy-phases.test.ts` (new, 11 cases).** Combined file with two `describe` blocks for `runPhaseLint` + `runPhaseBacklinks`. Narrowed to result-mapping + error envelope per codex CMT-1 (the legacy phases don't extend `BaseCyclePhase` and don't take a progress reporter or AbortSignal directly, so the contract surface they actually have is the counter → status enum mapping plus the try/catch error envelope). Cases: clean run → status='ok' with summary format, partial fix → status='warn' with `dryRun` in details, dry-run path doesn't write, throw-from-lib → status='fail' with the wrapper's try/catch envelope populated (verified that the throw doesn't escape). Future phase wrappers (sync, extract, embed, orphans, extract_facts, resolve_symbol_edges, recompute_emotional_weight) land as additional describes here, not as new files. + +#### Pre-existing master flakes — root-cause fixes (not bandaids) + +- **`test/ai/gateway.test.ts` — add `afterAll(() => resetGateway())` cleanup hook.** The file's tests each call `beforeEach(() => resetGateway())` so per-test pollution within the file was handled. But the file's LAST test set `configureGateway({env: {OPENAI_API_KEY: 'openai-fake'}})` and left that state in the gateway module. Other test files in the same bun shard (notably `test/ingestion/ingest-capture.test.ts`) then called code paths that triggered `embed()` without first calling `configureGateway`, hit the real OpenAI endpoint with the leaked fake key, and returned `Incorrect API key provided: openai-fake`. The shard wedged. One-line fix at file teardown stops the leak. + +- **`test/ai/header-transport.serial.test.ts` — quarantined from the parallel fast loop (renamed from `header-transport.test.ts`).** The file mutates `RECIPES` (the gateway's module-scoped recipe map) and `configureGateway` to register synthetic recipes, then asserts that `fakeChatFetch` was invoked. Under bun's intra-shard parallelism, sibling files like `test/ai/rerank.test.ts` and `test/gateway-embed-model-override.test.ts` race the same gateway state — the chat test would see `result.text === '[]'` instead of `'ok'` because a parallel test called `resetGateway` between this test's `configureGateway` and its `chat` call. The `.serial.test.ts` rename moves the file out of the parallel pass into the serial-after pass at `--max-concurrency=1` per repo convention. + +### Honest notes + +Three of the original 10 audit gaps turned out to be non-gaps after surveying the actual repo state — `test/ingestion/{dedup,daemon,skillpack-load}.test.ts` and `test/phantom-redirect.test.ts` already existed with thorough coverage (88 ingestion tests, 38 phantom-redirect tests, all green). The audit was based on stale information. The four real gaps (doctor behavioral, cycle phase wrappers, operations trust-boundary contract, shell guard for HTTP filter) all shipped. + +Four follow-up TODOs filed (in the plan file, not TODOS.md yet): +- NEW-1: per-check leaf unit tests for the 20+ exported doctor check functions (codex CMT-2 deep fix) +- NEW-2: cycle-phase wrappers for the other 7 phases (sync, extract, embed, orphans, extract_facts, resolve_symbol_edges, recompute_emotional_weight) +- NEW-3: HTTP-level trust-boundary E2E that proves serve-http.ts honors the localOnly filter at runtime (codex CMT-3 strongest defense) +- NEW-4: render function extraction from runDoctor (would let doctor-cli-smoke move back into the parallel fast loop) ## [0.40.7.2] - 2026-05-23 **TODOS.md gets a wave-commitments register at the top.** A `/plan-ceo-review` + `/plan-eng-review` pass clustered the 110 open TODOs into 12 feature themes, articulated the platonic ideal for each, and surfaced three strategic decisions. All three landed on the most ambitious option, and the seven verified-absent items the analysis caught got filed. Nothing in `src/` changed. diff --git a/CLAUDE.md b/CLAUDE.md index 3d56c9f0e..77e24b760 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,6 +168,9 @@ strict behavior when unset. - `src/core/minions/handlers/subagent.ts` extension (v0.37.7.0, #1151) — terminal-state short-circuit on resume. When a stored message thread already ends in `stop_reason: 'end_turn'`, the handler returns `{ ok: true }` immediately instead of issuing another `messages.create` call. Pre-fix, resume tried to re-prompt past `end_turn`, the Anthropic API rejected with a 400, and the worker classified the (already-successful) job as failed and dead-lettered it. Pinned by `test/subagent-handler.test.ts`. - `src/commands/doctor.ts` extension (v0.37.7.0, T12+T13+T14) — three new checks wired into `runDoctor()` and the JSON envelope. (1) `checkSourceRoutingHealth(engine)` scans up to 200 pages on federated brains and flags pages whose `source_id` doesn't match what `resolveSourceWithTier()` would have picked for their `source_path`. Single-source brains short-circuit to `ok` ("no federation to check"). D5 200-page cap keeps doctor under 5s on huge brains; the cap is total across the brain, not per-source. (2) `checkOauthConfidentialHealth(engine)` probes registered confidential clients for `/token` reachability — warns when the v0.37.0–v0.37.6 plaintext-compare bug class would have rejected them. (3) `checkAutopilotLockScope()` is a pure-function check (no engine) that compares the resolved lock path to `$GBRAIN_HOME`; warns when `$GBRAIN_HOME` is set but the lock lives elsewhere, with a PID-safe inspection hint per codex CF11 (lock file holds the daemon PID; check `kill -0 ` before considering deletion). All three are warn-only — they surface paste-ready fix hints without flipping the doctor exit code. Pinned by `test/doctor-v0_37_7_checks.test.ts`. - `skills/conventions/brain-routing.md` (v0.37.7.0, #1222) — agent-facing convention skill documenting the canonical 6-tier source resolution chain (flag → env → dotfile → local_path → brain_default → seed_default) with paste-ready decision tables. Linked from CLAUDE.md's "Two organizational axes" section and from `gbrain sources current`'s hint output. +- `src/commands/doctor.ts` extension (v0.40.4.1) — `buildChecks(engine, args, dbSource): Promise` exported as a test seam. `runDoctor` is now a thin wrapper: `buildChecks → computeDoctorReport → render + process.exit`. All 10 `process.exit` sites stay in the wrapper; the two early-return paths (no engine, connection failure) return partial check lists instead of inline exits. No behavior change — observable output identical because the wrapper renders the same partial list. Pinned by `test/doctor-behavioral.test.ts` (13 cases: pure aggregation math over `computeDoctorReport`, orchestrator cases for `--fast` skip set + `--json` flag + no-engine partial path + snapshot of load-bearing check names) and `test/doctor-cli-smoke.serial.test.ts` (1 subprocess case spawning `bun run src/cli.ts doctor --json` against a fresh PGLite tempdir, asserting schema_version=2 envelope, status enum, non-empty checks array — the render-path coverage that buildChecks-only tests miss). Quarantined as `.serial.test.ts` because PGLite write-locks don't play with parallel runners. +- `src/core/cycle.ts` extension (v0.40.4.1) — `runPhaseLint` + `runPhaseBacklinks` gain the `export` keyword so behavioral tests can drive them directly. No body changes; documented as internal helpers exposed for test-only consumption (downstream code should NOT take a dependency). Pinned by `test/cycle-legacy-phases.test.ts` (11 cases across both phases: clean run → status='ok', partial fix → status='warn' with `dryRun` in details, dry-run path doesn't write, throw-from-lib → status='fail' with the wrapper's try/catch envelope populated). Future phase wrappers (sync, extract, embed, orphans, extract_facts, resolve_symbol_edges, recompute_emotional_weight) land as additional describes in the same file, not new files (TODOS NEW-2). +- `test/operations-trust-boundary.test.ts` + `scripts/check-operations-filter-bypass.sh` (v0.40.4.1) — operations trust-boundary contract coverage. Hybrid test design: pure assertions over all 74 ops (every op has a scope annotation; every mutating op has a non-read scope; `localOnly: true` ops are excluded from `operations.filter(op => !op.localOnly)`; the seven historically-sensitive localOnly ops snapshot-pinned by name) plus targeted handler-invocation regressions for the two historically-broken HTTP-callable classes: `submit_job` with `name='shell'` + `ctx.remote=true` MUST reject (the F7b HTTP MCP shell-job RCE class), and `search_by_image` with `image_path` + `ctx.remote=true` MUST reject (the D18 P0 image-leak class). `file_upload` and `sync_brain` deliberately omitted from handler-invocation tests because they're `localOnly: true` and that path would test an impossible production scenario (codex CMT-3). The shell guard greps `src/` for any module importing the `operations` value outside the canonical filter site at `src/commands/serve-http.ts` — three import shapes detected (destructured, aliased, namespace), explicit 10-entry allow-list with per-entry rationale, plus a literal-string check that `serve-http.ts` still contains `operations.filter(op => !op.localOnly)`. Wired into `bun run verify`. - `src/core/link-extraction.ts` — shared library for the v0.12.0 graph layer. extractEntityRefs (canonical, replaces backlinks.ts duplicate) matches both `[Name](people/slug)` markdown links and Obsidian `[[people/slug|Name]]` wikilinks as of v0.12.3. extractPageLinks, inferLinkType heuristics (attended/works_at/invested_in/founded/advises/source/mentions), parseTimelineEntries, isAutoLinkEnabled config helper. `DIR_PATTERN` covers `people`, `companies`, `deals`, `topics`, `concepts`, `projects`, `entities`, `tech`, `finance`, `personal`, `openclaw`. Used by extract.ts, operations.ts auto-link post-hook, and backlinks.ts. - `src/core/zombie-reap.ts` (v0.28.1) — idempotent `installSigchldHandler()` so JS-spawned children get reaped via Bun's internal `waitpid()`. Bun (like Node) only auto-reaps when a SIGCHLD listener is registered; without it, every child the worker spawns (shell jobs, embed batches, sub-agents) becomes a zombie on exit and holds connection slots. Called once at module load from `src/cli.ts` (with Windows platform guard — SIGCHLD doesn't exist on Windows). Cross-file leak guard via `_uninstallSigchldHandlerForTests()` for tests. Layer 1 of the three-layer zombie defense; Layer 2 is tini-as-PID-1 wrapping the worker subtree (via `src/core/minions/spawn-helpers.ts`); Layer 3 is the container's own tini for hard Bun crashes. - `src/core/minions/` — Minions job queue: BullMQ-inspired, Postgres-native (queue, worker, backoff, types, protected-names, quiet-hours, stagger, handlers/shell). diff --git a/TODOS.md b/TODOS.md index 54217673d..2abdab650 100644 --- a/TODOS.md +++ b/TODOS.md @@ -293,6 +293,16 @@ at plan time and got carved out: Doctor surface is already in place to show outcomes; just need the scheduling lane. Estimate: ~3 hours. +## v0.41+ e2e-test-wave follow-ups (filed during v0.40.8.0 ship) + +- [ ] **NEW-1 (P2) — Per-check leaf unit tests for the 20+ exported doctor check functions.** `src/commands/doctor.ts:169-1492` exports whoknowsHealthCheck, takesWeightGridCheck, childTableOrphansCheck, checkRerankerHealth, checkBrainstormHealth, checkSearchMode, checkEvalDrift, checkSyncFreshness, checkAbandonedThreads, checkCalibrationFreshness, checkGradeConfidenceDrift, checkVoiceGateHealth, checkZeEmbeddingHealth, checkEmbeddingWidthConsistency, checkSourceRoutingHealth, checkOauthConfidentialHealth, checkAutopilotLockScope, skillBrainFirstCheck. v0.40.8.0 covers them via the orchestrator only. Parameterize a single `test/doctor-leaves.test.ts` over the exported functions; each case seeds the minimum DB state and asserts the returned `Check.status`. Catches per-check render bugs the orchestrator snapshot can't see (codex CMT-2 deep fix). Estimated ~4h CC. +- [ ] **NEW-2 (P2) — Cycle-phase wrappers beyond lint + backlinks.** 7 more phases need result-mapping coverage: sync, extract, embed, orphans, extract_facts, resolve_symbol_edges, recompute_emotional_weight. Each adds a describe block to `test/cycle-legacy-phases.test.ts` following the established pattern. ~30min/phase with CC. Mechanical follow-through. +- [ ] **NEW-3 (P2) — HTTP-level trust-boundary test that proves serve-http.ts honors the filter at runtime.** v0.40.8.0 ships the source-grep guard at `scripts/check-operations-filter-bypass.sh` plus structural assertions in `test/operations-trust-boundary.test.ts`. The codex CMT-3 strongest defense — runtime proof that a register-OAuth-client → attempt-call-every-localOnly-op flow rejects every one — would extend `test/e2e/serve-http-oauth.test.ts`. Real Postgres dep, ~30s wallclock per case. Closes the bypass class with runtime proof in addition to the existing structural defense. +- [ ] **NEW-4 (P3) — Render function extraction from runDoctor.** v0.40.8.0 uses a subprocess smoke at `test/doctor-cli-smoke.serial.test.ts` to cover the wrapper's render + exit paths. Pulling the human + JSON render code out into pure formatters would let that smoke move back into the parallel fast loop with no subprocess overhead. ~2h CC. Lower priority — the subprocess smoke does its job; this is a wallclock win, not a coverage win. + +## v0.41+ master flake follow-ups (filed during v0.40.8.0 ship) + +- [ ] **(P3) — Audit other gateway-mutating tests for missing afterAll cleanup.** v0.40.8.0 added `afterAll(() => resetGateway())` to `test/ai/gateway.test.ts` and quarantined `test/ai/header-transport.test.ts` as `.serial.test.ts`. Two other files mutate gateway state without an explicit cleanup hook: `test/ai/rerank.test.ts`, `test/gateway-embed-model-override.test.ts`. They haven't surfaced flakes yet (different test sequences), but they're the same risk class. Add `afterAll(() => resetGateway())` to both for defense-in-depth, or quarantine if they prove racy under future parallelism changes. ## v0.40.4 adversarial review LOW findings — captured for v0.41+ - [ ] **Codex L1**: `gbrain search stats --days N` underreports for N > 7. audit-writer.ts reads only current + previous ISO week (~14 days). `--days 30` silently shows ~2 weeks of failure events. Fix shape: extend readRecent to walk N/7 weeks dynamically OR cap user input with a clear message. diff --git a/VERSION b/VERSION index d4ebcc26a..28ef40d3f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.40.7.2 \ No newline at end of file +0.40.8.0 \ No newline at end of file diff --git a/llms-full.txt b/llms-full.txt index 589f89948..7d1e234b9 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -310,6 +310,9 @@ strict behavior when unset. - `src/core/minions/handlers/subagent.ts` extension (v0.37.7.0, #1151) — terminal-state short-circuit on resume. When a stored message thread already ends in `stop_reason: 'end_turn'`, the handler returns `{ ok: true }` immediately instead of issuing another `messages.create` call. Pre-fix, resume tried to re-prompt past `end_turn`, the Anthropic API rejected with a 400, and the worker classified the (already-successful) job as failed and dead-lettered it. Pinned by `test/subagent-handler.test.ts`. - `src/commands/doctor.ts` extension (v0.37.7.0, T12+T13+T14) — three new checks wired into `runDoctor()` and the JSON envelope. (1) `checkSourceRoutingHealth(engine)` scans up to 200 pages on federated brains and flags pages whose `source_id` doesn't match what `resolveSourceWithTier()` would have picked for their `source_path`. Single-source brains short-circuit to `ok` ("no federation to check"). D5 200-page cap keeps doctor under 5s on huge brains; the cap is total across the brain, not per-source. (2) `checkOauthConfidentialHealth(engine)` probes registered confidential clients for `/token` reachability — warns when the v0.37.0–v0.37.6 plaintext-compare bug class would have rejected them. (3) `checkAutopilotLockScope()` is a pure-function check (no engine) that compares the resolved lock path to `$GBRAIN_HOME`; warns when `$GBRAIN_HOME` is set but the lock lives elsewhere, with a PID-safe inspection hint per codex CF11 (lock file holds the daemon PID; check `kill -0 ` before considering deletion). All three are warn-only — they surface paste-ready fix hints without flipping the doctor exit code. Pinned by `test/doctor-v0_37_7_checks.test.ts`. - `skills/conventions/brain-routing.md` (v0.37.7.0, #1222) — agent-facing convention skill documenting the canonical 6-tier source resolution chain (flag → env → dotfile → local_path → brain_default → seed_default) with paste-ready decision tables. Linked from CLAUDE.md's "Two organizational axes" section and from `gbrain sources current`'s hint output. +- `src/commands/doctor.ts` extension (v0.40.4.1) — `buildChecks(engine, args, dbSource): Promise` exported as a test seam. `runDoctor` is now a thin wrapper: `buildChecks → computeDoctorReport → render + process.exit`. All 10 `process.exit` sites stay in the wrapper; the two early-return paths (no engine, connection failure) return partial check lists instead of inline exits. No behavior change — observable output identical because the wrapper renders the same partial list. Pinned by `test/doctor-behavioral.test.ts` (13 cases: pure aggregation math over `computeDoctorReport`, orchestrator cases for `--fast` skip set + `--json` flag + no-engine partial path + snapshot of load-bearing check names) and `test/doctor-cli-smoke.serial.test.ts` (1 subprocess case spawning `bun run src/cli.ts doctor --json` against a fresh PGLite tempdir, asserting schema_version=2 envelope, status enum, non-empty checks array — the render-path coverage that buildChecks-only tests miss). Quarantined as `.serial.test.ts` because PGLite write-locks don't play with parallel runners. +- `src/core/cycle.ts` extension (v0.40.4.1) — `runPhaseLint` + `runPhaseBacklinks` gain the `export` keyword so behavioral tests can drive them directly. No body changes; documented as internal helpers exposed for test-only consumption (downstream code should NOT take a dependency). Pinned by `test/cycle-legacy-phases.test.ts` (11 cases across both phases: clean run → status='ok', partial fix → status='warn' with `dryRun` in details, dry-run path doesn't write, throw-from-lib → status='fail' with the wrapper's try/catch envelope populated). Future phase wrappers (sync, extract, embed, orphans, extract_facts, resolve_symbol_edges, recompute_emotional_weight) land as additional describes in the same file, not new files (TODOS NEW-2). +- `test/operations-trust-boundary.test.ts` + `scripts/check-operations-filter-bypass.sh` (v0.40.4.1) — operations trust-boundary contract coverage. Hybrid test design: pure assertions over all 74 ops (every op has a scope annotation; every mutating op has a non-read scope; `localOnly: true` ops are excluded from `operations.filter(op => !op.localOnly)`; the seven historically-sensitive localOnly ops snapshot-pinned by name) plus targeted handler-invocation regressions for the two historically-broken HTTP-callable classes: `submit_job` with `name='shell'` + `ctx.remote=true` MUST reject (the F7b HTTP MCP shell-job RCE class), and `search_by_image` with `image_path` + `ctx.remote=true` MUST reject (the D18 P0 image-leak class). `file_upload` and `sync_brain` deliberately omitted from handler-invocation tests because they're `localOnly: true` and that path would test an impossible production scenario (codex CMT-3). The shell guard greps `src/` for any module importing the `operations` value outside the canonical filter site at `src/commands/serve-http.ts` — three import shapes detected (destructured, aliased, namespace), explicit 10-entry allow-list with per-entry rationale, plus a literal-string check that `serve-http.ts` still contains `operations.filter(op => !op.localOnly)`. Wired into `bun run verify`. - `src/core/link-extraction.ts` — shared library for the v0.12.0 graph layer. extractEntityRefs (canonical, replaces backlinks.ts duplicate) matches both `[Name](people/slug)` markdown links and Obsidian `[[people/slug|Name]]` wikilinks as of v0.12.3. extractPageLinks, inferLinkType heuristics (attended/works_at/invested_in/founded/advises/source/mentions), parseTimelineEntries, isAutoLinkEnabled config helper. `DIR_PATTERN` covers `people`, `companies`, `deals`, `topics`, `concepts`, `projects`, `entities`, `tech`, `finance`, `personal`, `openclaw`. Used by extract.ts, operations.ts auto-link post-hook, and backlinks.ts. - `src/core/zombie-reap.ts` (v0.28.1) — idempotent `installSigchldHandler()` so JS-spawned children get reaped via Bun's internal `waitpid()`. Bun (like Node) only auto-reaps when a SIGCHLD listener is registered; without it, every child the worker spawns (shell jobs, embed batches, sub-agents) becomes a zombie on exit and holds connection slots. Called once at module load from `src/cli.ts` (with Windows platform guard — SIGCHLD doesn't exist on Windows). Cross-file leak guard via `_uninstallSigchldHandlerForTests()` for tests. Layer 1 of the three-layer zombie defense; Layer 2 is tini-as-PID-1 wrapping the worker subtree (via `src/core/minions/spawn-helpers.ts`); Layer 3 is the container's own tini for hard Bun crashes. - `src/core/minions/` — Minions job queue: BullMQ-inspired, Postgres-native (queue, worker, backoff, types, protected-names, quiet-hours, stagger, handlers/shell). diff --git a/package.json b/package.json index 6c81d3348..ab83692d8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.40.7.2", + "version": "0.40.8.0", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", @@ -40,14 +40,14 @@ "build:pglite-snapshot": "bun run scripts/build-pglite-snapshot.ts", "test": "bash scripts/run-unit-parallel.sh", "test:full": "bun run verify && bash scripts/run-unit-parallel.sh && bun run test:slow && ([ -n \"$DATABASE_URL\" ] && bash scripts/run-e2e.sh || echo '[test:full] skipped E2E (no DATABASE_URL); run docker-compose -f docker-compose.ci.yml up + bun run test:e2e to include' 1>&2)", - "verify": "bun run check:privacy && bun run check:proposal-pii && bun run check:test-names && bun run check:jsonb && bun run check:source-id-projection && bun run check:source-config-leak && bun run check:progress && bun run check:test-isolation && bun run check:wasm && bun run check:admin-build && bun run check:admin-scope-drift && bun run check:cli-exec && bun run check:system-of-record && bun run check:eval-glossary && bun run check:no-pii-agent-voice && bun run check:synthetic-corpus-privacy && bun run check:skill-brain-first && bun run check:fuzz-purity && bun run typecheck", + "verify": "bun run check:privacy && bun run check:proposal-pii && bun run check:test-names && bun run check:jsonb && bun run check:source-id-projection && bun run check:source-config-leak && bun run check:progress && bun run check:test-isolation && bun run check:wasm && bun run check:admin-build && bun run check:admin-scope-drift && bun run check:cli-exec && bun run check:system-of-record && bun run check:eval-glossary && bun run check:no-pii-agent-voice && bun run check:synthetic-corpus-privacy && bun run check:skill-brain-first && bun run check:fuzz-purity && bun run check:operations-filter-bypass && bun run typecheck", "check:source-config-leak": "scripts/check-source-config-leak.sh", "check:no-pii-agent-voice": "scripts/check-no-pii-in-agent-voice.sh", "check:synthetic-corpus-privacy": "scripts/check-synthetic-corpus-privacy.sh", "check:system-of-record": "scripts/check-system-of-record.sh", "check:admin-scope-drift": "scripts/check-admin-scope-drift.sh", "check:cli-exec": "scripts/check-cli-executable.sh", - "check:all": "scripts/check-privacy.sh && scripts/check-proposal-pii.sh && scripts/check-test-real-names.sh && scripts/check-jsonb-pattern.sh && scripts/check-source-id-projection.sh && scripts/check-progress-to-stdout.sh && scripts/check-no-legacy-getconnection.sh && scripts/check-test-isolation.sh && scripts/check-trailing-newline.sh && scripts/check-wasm-embedded.sh && scripts/check-exports-count.sh && scripts/check-admin-build.sh && scripts/check-admin-scope-drift.sh && scripts/check-cli-executable.sh && scripts/check-skill-brain-first.sh", + "check:all": "scripts/check-privacy.sh && scripts/check-proposal-pii.sh && scripts/check-test-real-names.sh && scripts/check-jsonb-pattern.sh && scripts/check-source-id-projection.sh && scripts/check-source-config-leak.sh && scripts/check-progress-to-stdout.sh && scripts/check-no-legacy-getconnection.sh && scripts/check-test-isolation.sh && scripts/check-trailing-newline.sh && scripts/check-wasm-embedded.sh && scripts/check-exports-count.sh && scripts/check-admin-build.sh && scripts/check-admin-scope-drift.sh && scripts/check-cli-executable.sh && scripts/check-skill-brain-first.sh && scripts/check-operations-filter-bypass.sh", "check:skill-brain-first": "scripts/check-skill-brain-first.sh", "check:wasm": "scripts/check-wasm-embedded.sh", "check:newlines": "scripts/check-trailing-newline.sh", @@ -72,6 +72,7 @@ "check:admin-embedded": "scripts/check-admin-embedded.sh", "check:test-isolation": "scripts/check-test-isolation.sh", "check:fuzz-purity": "scripts/check-fuzz-purity.sh", + "check:operations-filter-bypass": "scripts/check-operations-filter-bypass.sh", "postinstall": "command -v gbrain >/dev/null 2>&1 && gbrain apply-migrations --yes --non-interactive || echo '[gbrain] postinstall skipped. If installed via bun install -g github:...: run `gbrain doctor` and `gbrain apply-migrations --yes` manually. See https://github.com/garrytan/gbrain/issues/218' 1>&2", "prepublish:clawhub": "bun run build:all", "publish:clawhub": "clawhub package publish . --family bundle-plugin" diff --git a/scripts/check-operations-filter-bypass.sh b/scripts/check-operations-filter-bypass.sh new file mode 100755 index 000000000..d881024e9 --- /dev/null +++ b/scripts/check-operations-filter-bypass.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# v0.39 — CI guard against bypassing the localOnly filter on the HTTP MCP +# surface. Lives alongside check-jsonb-pattern.sh / check-progress-to-stdout.sh +# in the bun run verify chain. +# +# Background: serve-http.ts builds the HTTP MCP tools/list response from +# `operations.filter(op => !op.localOnly)`. That filter is the only thing +# keeping localOnly ops (sync_brain, file_upload, file_list, file_url — +# any admin op the user EXPLICITLY marked as CLI-only) off the wire. +# +# If a future HTTP-exposing module imports `operations` from +# core/operations.ts WITHOUT applying the filter, the localOnly contract +# silently breaks: a write-scoped OAuth client could submit `sync_brain` +# or `file_upload` over HTTP. Codex outside-voice review of the e2e-test- +# wave (CMT-3) flagged this exact bypass class. +# +# The guard works by: +# 1. Listing every file that imports `operations` from core/operations.ts. +# 2. Comparing against an explicit ALLOWLIST of known-safe importers +# (each with a rationale below). +# 3. Failing if a new file is missing from the allowlist — forces the +# author to either (a) join the allowlist with an explicit rationale, +# or (b) apply the canonical filter at import. +# +# This is a structural defense. The runtime defense lives in +# test/operations-trust-boundary.test.ts (the canonical filter contract + +# handler-invocation cases for the historically-broken classes). +# +# To allow a new importer: add the relative path to ALLOWED below with a +# one-line comment explaining why localOnly isn't a concern there. +# +# Exit: 0 when no unknown importers, 1 when at least one is missing from +# the allowlist. +set -euo pipefail + +ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +cd "$ROOT" + +# Files allowed to import `operations` directly. Each entry must be +# accompanied by a one-line rationale (the comment on the same line). +ALLOWED=( + "src/cli.ts" # local CLI; user owns the machine, no trust boundary + "src/mcp/dispatch.ts" # shared dispatch; sets ctx.remote from caller, handlers self-gate + "src/mcp/server.ts" # stdio MCP; local-trusted (binary on user's box) + "src/mcp/http-transport.ts" # superseded by serve-http.ts; kept for back-compat tests + "src/mcp/tool-defs.ts" # pure helper; takes ops as parameter, never exposes them + "src/core/minions/tools/brain-allowlist.ts" # subagent registry; has its own opt-in allowlist (separate from localOnly) + "src/commands/capture.ts" # local CLI tool; not network-exposed + "src/commands/book-mirror.ts" # local CLI tool; not network-exposed + "src/commands/tools-json.ts" # gbrain --tools-json introspection; full op list IS the purpose + "src/commands/serve-http.ts" # MUST APPLY .filter(op => !op.localOnly) — verified by grep below +) + +# Pattern: any import that brings the `operations` VALUE in from core/operations.ts. +# Three shapes the value can enter through; each must be caught: +# - destructured: import { operations } from '...core/operations.ts' +# - aliased: import { operations as ops } from '...core/operations.ts' +# - namespace: import * as opsModule from '...core/operations.ts' +# The original narrow regex only matched the destructured form — codex caught +# the bypass class during /ship adversarial review (aliased + namespace forms +# slipped through). The broadened regex below specifically requires `operations` +# inside the destructured clause OR a namespace import (`* as X`); type-only +# imports of sibling exports like `sourceScopeOpts` / `OperationContext` are +# left alone (those don't expose the op list to a transport surface). +PATTERN='import[[:space:]]+(\*[[:space:]]+as[[:space:]]+[a-zA-Z_$][a-zA-Z0-9_$]*|\{[^}]*\boperations\b[^}]*\})[[:space:]]+from[[:space:]]*['\''"][^'\''"]*core/operations\.ts['\''"]' + +# Collect files that import `operations`. Use a while-loop over grep output +# instead of `mapfile` to stay compatible with macOS's default bash 3.2. +FOUND_FILES="" +while IFS= read -r f; do + [ -n "$f" ] && FOUND_FILES="$FOUND_FILES$f"$'\n' +done < <(grep -rlE --include='*.ts' "$PATTERN" src/ 2>/dev/null | sort -u || true) + +FAIL=0 + +# Check 1: every found file is in ALLOWED. +while IFS= read -r file; do + [ -z "$file" ] && continue + rel="${file#"$ROOT/"}" + ok=0 + for allowed in "${ALLOWED[@]}"; do + if [ "$rel" = "$allowed" ]; then + ok=1 + break + fi + done + if [ "$ok" -eq 0 ]; then + echo "FAIL: $rel imports operations but is not in scripts/check-operations-filter-bypass.sh ALLOWED list." + echo " Either apply .filter(op => !op.localOnly) at the import boundary," + echo " or add this file to ALLOWED with a one-line rationale." + FAIL=1 + fi +done <<< "$FOUND_FILES" + +# Check 2: serve-http.ts MUST contain the canonical filter expression near +# its operations import. Without the filter, the entire HTTP MCP surface +# leaks localOnly ops. +SERVE_HTTP="src/commands/serve-http.ts" +if [ -f "$SERVE_HTTP" ]; then + if ! grep -qE 'operations\.filter\(\s*op\s*=>\s*!op\.localOnly\s*\)' "$SERVE_HTTP"; then + echo "FAIL: $SERVE_HTTP no longer contains the canonical" + echo " operations.filter(op => !op.localOnly) expression. The HTTP MCP" + echo " surface depends on this filter to enforce localOnly. Restore" + echo " the filter or refactor the trust boundary explicitly." + FAIL=1 + fi +fi + +if [ "$FAIL" -eq 1 ]; then + echo "" + echo "Hint: see test/operations-trust-boundary.test.ts for the runtime contract." + exit 1 +fi + +exit 0 diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 2594935ed..fd0e8b889 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -2098,23 +2098,37 @@ export async function checkCycleFreshness( * user has no DB configured anywhere; otherwise the caller chose --fast or * we failed to connect despite a configured URL. */ -export async function runDoctor(engine: BrainEngine | null, args: string[], dbSource?: DbUrlSource) { +/** + * Build the full check list for `gbrain doctor` against an engine + arg vector. + * + * The check-building seam: takes the same args as `runDoctor` minus the + * --locks shortcut (locks-mode is a focused diagnostic the CLI wrapper + * handles separately). Returns a `Check[]` array; the caller renders it + * via `outputResults` and decides exit code. Early-exit cases (no engine, + * connection failure) return a partial check array without calling + * `process.exit` directly — the caller still renders + exits. + * + * v0.39 narrow-seam extract (audit-driven). The 10 `process.exit` sites + * in this file all live in CLI wrappers (`runDoctor`, `runLocksCheck`, + * the remediation subcommands). Behavioral tests drive `buildChecks` + * directly via PGLite; the wrapper-level subprocess smoke in + * `test/doctor-cli-smoke.test.ts` covers the render + exit paths that + * a unit test can't reach in-process. + * + * Side effects retained inside buildChecks (kept for "no behavior change"): + * - `printAutoFixReport` on `--fix` non-JSON path + * - `progress` reporter writes to stderr (heartbeats per check) + * - `engine.executeRaw` / handler-leaf calls (the actual probe work) + */ +export async function buildChecks( + engine: BrainEngine | null, + args: string[], + dbSource?: DbUrlSource, +): Promise { const jsonOutput = args.includes('--json'); const fastMode = args.includes('--fast'); const doFix = args.includes('--fix'); const dryRun = args.includes('--dry-run'); - const locksMode = args.includes('--locks'); - - // --locks is a focused diagnostic: it runs the same pg_stat_activity - // query that `runMigrations` pre-flight uses, prints any idle-in-tx - // backends, and exits. Used by a user (or the migrate.ts error 57014 - // message) who just hit a statement_timeout and needs to find the - // blocker. Referenced from migrate.ts's 57014 diagnostic — that - // message promised this flag exists. - if (locksMode) { - await runLocksCheck(engine, jsonOutput); - return; - } const checks: Check[] = []; let autoFixReport: AutoFixReport | null = null; @@ -2716,9 +2730,10 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo } checks.push({ name: 'connection', status: 'warn', message: msg }); } - const earlyFail1 = outputResults(checks, jsonOutput); - process.exit(earlyFail1 ? 1 : 0); - return; + // Early return: caller renders the partial check list + decides exit code. + // Pre-v0.39 this site called outputResults + process.exit directly; the + // narrow-seam extract moved both to the runDoctor CLI wrapper. + return checks; } // DB checks phase — start a single reporter phase so agents see which @@ -2735,9 +2750,10 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo const msg = e instanceof Error ? e.message : String(e); checks.push({ name: 'connection', status: 'fail', message: msg }); progress.finish(); - const earlyFail2 = outputResults(checks, jsonOutput); - process.exit(earlyFail2 ? 1 : 0); - return; + // Early return: caller renders the partial check list + decides exit code. + // Pre-v0.39 this site called outputResults + process.exit directly; the + // narrow-seam extract moved both to the runDoctor CLI wrapper. + return checks; } // 4. pgvector extension @@ -4481,6 +4497,42 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo progress.finish(); + return checks; +} + +/** + * CLI entry point for `gbrain doctor`. Thin wrapper around buildChecks + + * computeDoctorReport + render + process.exit. + * + * Concerns kept here (not pushed into buildChecks): + * - --locks shortcut (focused diagnostic; calls runLocksCheck + returns) + * - outputResults render (stdout) + * - features teaser (non-JSON, non-failing only) + * - process.exit (10 sites total across runDoctor + runLocksCheck + + * runRemediationPlan + runRemediate) + * + * v0.39 narrow-seam extract — buildChecks is the unit-testable seam, this + * wrapper is the wallclock + exit-code concerned function. See + * test/doctor-behavioral.test.ts for the in-process seam coverage and + * test/doctor-cli-smoke.test.ts for the subprocess wrapper coverage. + */ +export async function runDoctor( + engine: BrainEngine | null, + args: string[], + dbSource?: DbUrlSource, +) { + const jsonOutput = args.includes('--json'); + const locksMode = args.includes('--locks'); + + // --locks is a focused diagnostic: it runs the same pg_stat_activity + // query that `runMigrations` pre-flight uses, prints any idle-in-tx + // backends, and exits. Referenced from migrate.ts's 57014 diagnostic. + if (locksMode) { + await runLocksCheck(engine, jsonOutput); + return; + } + + const checks = await buildChecks(engine, args, dbSource); const hasFail = outputResults(checks, jsonOutput); // Features teaser (non-JSON, non-failing only) diff --git a/src/core/brain-writer.ts b/src/core/brain-writer.ts index eb694c540..c2628e77c 100644 --- a/src/core/brain-writer.ts +++ b/src/core/brain-writer.ts @@ -442,7 +442,12 @@ export async function scanBrainSources( const src = sources[i]; // Between-source abort check: AbortSignal works here (no sync I/O blocking // the event loop at this boundary). For mid-walk interruption use deadline. - if (opts.signal?.aborted || (opts.deadline && Date.now() > opts.deadline)) { + // `>=` not `>`: when Date.now() lands exactly on the deadline boundary, + // the budget IS exhausted — proceeding to scanOneSource would let the + // next source eat its own budget on top of what's already been spent. + // (Strict `>` was a real flake source on CI: GitHub Actions runners + // landing on the boundary exactly during a Promise.race timeout.) + if (opts.signal?.aborted || (opts.deadline && Date.now() >= opts.deadline)) { // Codex adversarial review #3: when deadline fires BETWEEN sources, // also stamp aborted_at_source with the source we were about to start. // Pre-fix, the doctor message said "PARTIAL SCAN" with no source name. @@ -500,7 +505,11 @@ export async function scanBrainSources( // status='partial' with files_scanned=0, which is misleading ("partial // scan" when actually nothing was scanned). Mark this source + remainder // as 'skipped' so the doctor message is honest. - if (opts.signal?.aborted || (opts.deadline && Date.now() > opts.deadline)) { + // `>=` matches the between-source check above (line 445). The Promise.race + // setTimeout resolves null at exactly `remainingMs` from now, so post-await + // Date.now() often equals deadline within integer-ms precision — strict `>` + // missed those landings on CI and let the next scanOneSource run anyway. + if (opts.signal?.aborted || (opts.deadline && Date.now() >= opts.deadline)) { if (abortedAtSource === null) { abortedAtSource = src.id; } diff --git a/src/core/cycle.ts b/src/core/cycle.ts index 1aadc25e3..1db653cda 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -560,7 +560,14 @@ function checkAborted(signal?: AbortSignal): void { // ─── Phase runners ───────────────────────────────────────────────── -async function runPhaseLint(brainDir: string, dryRun: boolean): Promise { +// v0.39 — runPhaseLint + runPhaseBacklinks are exported for the cycle- +// legacy-phases test (audit GAP 5 / D9). Exporting widens the public API +// surface; consumers outside of runCycle should NOT take a dependency on +// these — they exist for the cycle's internal composition. The export +// keyword is the minimal seam that lets behavioral tests drive the +// wrapper's result-mapping (counter → status enum + summary) without +// going through runCycle's full setup cost. +export async function runPhaseLint(brainDir: string, dryRun: boolean): Promise { try { const { runLintCore } = await import('../commands/lint.ts'); const result = await runLintCore({ target: brainDir, fix: true, dryRun }); @@ -594,7 +601,7 @@ async function runPhaseLint(brainDir: string, dryRun: boolean): Promise { +export async function runPhaseBacklinks(brainDir: string, dryRun: boolean): Promise { try { // Maintenance cycles must not rewrite tracked brain pages with generated // "Referenced in" timeline bullets. The graph extractor/auto-link path is diff --git a/test/ai/gateway.test.ts b/test/ai/gateway.test.ts index e82f1e4b3..23f14b9ca 100644 --- a/test/ai/gateway.test.ts +++ b/test/ai/gateway.test.ts @@ -1,4 +1,4 @@ -import { describe, test, expect, beforeEach } from 'bun:test'; +import { describe, test, expect, beforeEach, afterAll } from 'bun:test'; import { configureGateway, resetGateway, @@ -9,6 +9,14 @@ import { getExpansionModel, VoyageResponseTooLargeError, } from '../../src/core/ai/gateway.ts'; + +// v0.39.x ship-wave fix: gateway module is process-scoped. Without an +// afterAll cleanup, the last test's configureGateway({env: {OPENAI_API_KEY: +// 'openai-fake'}}) state leaked into sibling files in the same bun shard +// (capture / ingest-capture tests), where it produced "Incorrect API key +// provided: openai-fake" against the real OpenAI endpoint and wedged +// the shard. Reset once at file teardown so no caller sees the residue. +afterAll(() => resetGateway()); import { parseModelId, resolveRecipe } from '../../src/core/ai/model-resolver.ts'; import { dimsProviderOptions, diff --git a/test/ai/header-transport.test.ts b/test/ai/header-transport.serial.test.ts similarity index 100% rename from test/ai/header-transport.test.ts rename to test/ai/header-transport.serial.test.ts diff --git a/test/brain-writer-partial-scan.test.ts b/test/brain-writer-partial-scan.test.ts index 939ba5055..611c98dc6 100644 --- a/test/brain-writer-partial-scan.test.ts +++ b/test/brain-writer-partial-scan.test.ts @@ -153,11 +153,15 @@ describe('scanBrainSources partial-scan state', () => { // The post-await deadline re-check should mark the source as 'skipped'. test('slow COUNT that exceeds deadline marks source skipped, not partial', async () => { const start = Date.now(); + // CI timing fix (same class as the "hanging COUNT" test below): widened + // 50→250ms deadline + 100→500ms simulated query delay. Keeps the 2x + // ratio that proves "query exceeds deadline" while giving CI runners + // enough headroom to schedule both setTimeout callbacks on-budget. const report = await scanBrainSources(engine, { - deadline: start + 50, // 50ms budget + deadline: start + 250, dbPageCountForSource: async () => { - // Simulate a hung query: take 100ms (past the deadline). - await new Promise(resolve => setTimeout(resolve, 100)); + // Simulate a hung query: take 500ms (past the deadline). + await new Promise(resolve => setTimeout(resolve, 500)); return 42; }, }); @@ -173,18 +177,27 @@ describe('scanBrainSources partial-scan state', () => { // Codex adversarial #4 regression: even when dbPageCountForSource itself // would hang indefinitely, the Promise.race against the deadline must // resolve null and the scan must abort cleanly. + // + // Boundary fix (CI flake): brain-writer.ts post-await deadline check uses + // `>=` not `>`. The Promise.race setTimeout resolves at exactly + // `remainingMs` from now, so post-await Date.now() often equals deadline + // within integer-ms precision. Strict `>` missed those landings on CI + // runners and let scanOneSource run anyway, marking src-a as 'scanned' + // instead of 'skipped'. Test budget is generous enough to absorb runner + // timer drift; the real fix is the operator change in brain-writer.ts. test('hanging COUNT does not exceed deadline — Promise.race timeout fires', async () => { const start = Date.now(); + const DEADLINE_MS = 500; + const BOUND_MS = 2500; const report = await scanBrainSources(engine, { - deadline: start + 100, // 100ms budget + deadline: start + DEADLINE_MS, dbPageCountForSource: () => { // Never resolves — would hang forever without the deadline race. return new Promise(() => {}); }, }); const elapsed = Date.now() - start; - // Generous bound: should complete within 2x the deadline budget (setup overhead). - expect(elapsed).toBeLessThan(500); + expect(elapsed).toBeLessThan(BOUND_MS); expect(report.partial).toBe(true); const firstSource = report.per_source.find(r => r.source_id === 'src-a')!; expect(firstSource.status).toBe('skipped'); diff --git a/test/cycle-legacy-phases.test.ts b/test/cycle-legacy-phases.test.ts new file mode 100644 index 000000000..cd15fd303 --- /dev/null +++ b/test/cycle-legacy-phases.test.ts @@ -0,0 +1,206 @@ +/** + * v0.39 (GAP 5 / D9) — cycle-phase wrapper coverage for legacy phases. + * + * Narrowed to RESULT-MAPPING + ERROR-ENVELOPE per D3 codex tension: + * - counter → PhaseStatus enum mapping (issues → 'warn'; gaps → 'ok' with + * audit-only summary; throw-from-lib → 'fail' with error envelope) + * - summary string format (caller-facing message) + * - dry-run path returns sensible status without writes + * + * The wrappers don't take a progress reporter or AbortSignal directly — + * those concerns live at runCycle. We do NOT test "BaseCyclePhase contract" + * here because legacy phases don't extend it. We test what they actually + * do: lazy-import the lib, try/catch envelope, result-mapping. + * + * Combined file with two describe blocks per D5 — shared brain-dir setup, + * future phases (sync, extract, embed, orphans) land as additional + * describes here rather than 7 nearly-identical files. + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { runPhaseLint, runPhaseBacklinks } from '../src/core/cycle.ts'; + +let engine: PGLiteEngine; +let brainDir: string; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); + // Fresh brain dir per test — phase wrappers walk the filesystem. + brainDir = mkdtempSync(join(tmpdir(), 'cycle-phases-')); +}); + +function cleanupBrain(): void { + try { rmSync(brainDir, { recursive: true, force: true }); } catch { /* best effort */ } +} + +describe('runPhaseLint — result-mapping', () => { + test('clean brain (no markdown files) → status="ok", summary names zero issues', async () => { + try { + const result = await runPhaseLint(brainDir, false); + expect(result.phase).toBe('lint'); + expect(result.status).toBe('ok'); + expect(result.summary.toLowerCase()).toContain('fix'); + // No issues, no error envelope. + expect(result.error).toBeUndefined(); + expect(typeof result.details).toBe('object'); + } finally { + cleanupBrain(); + } + }); + + test('brain with valid markdown → status="ok"', async () => { + try { + mkdirSync(join(brainDir, 'people'), { recursive: true }); + writeFileSync( + join(brainDir, 'people', 'alice-example.md'), + '---\ntitle: Alice\ntype: person\n---\n\nHello.\n', + ); + const result = await runPhaseLint(brainDir, false); + expect(result.phase).toBe('lint'); + expect(['ok', 'warn']).toContain(result.status); + expect(result.error).toBeUndefined(); + expect(typeof result.summary).toBe('string'); + expect(result.summary.length).toBeGreaterThan(0); + } finally { + cleanupBrain(); + } + }); + + test('dry-run path → status reflects whether issues remained without writes', async () => { + try { + mkdirSync(brainDir, { recursive: true }); + const result = await runPhaseLint(brainDir, true); + expect(result.phase).toBe('lint'); + // dryRun is in the details payload so callers can disambiguate. + expect(result.details).toMatchObject({ dryRun: true }); + // Status is ok when nothing actionable found, warn when issues remain. + expect(['ok', 'warn']).toContain(result.status); + // Summary uses "would" / "found" / "no issues" — never the post-fix + // "applied N fixes" string, because nothing was written. + expect(result.summary.toLowerCase()).not.toContain('applied'); + } finally { + cleanupBrain(); + } + }); + + test('throw-from-lib (nonexistent brainDir) → status="fail" with error envelope', async () => { + const missingDir = join(tmpdir(), `nonexistent-brain-${Date.now()}-${Math.random()}`); + const result = await runPhaseLint(missingDir, false); + expect(result.phase).toBe('lint'); + expect(result.status).toBe('fail'); + expect(result.summary).toBe('lint phase failed'); + expect(result.error).toBeDefined(); + expect(typeof result.error!.code).toBe('string'); + // Critical: throw does NOT escape — the wrapper's try/catch envelope + // contains it. If this assertion ever flips, every cycle that hits a + // lint failure would abort instead of carrying on with the next phase. + }); +}); + +describe('runPhaseBacklinks — result-mapping', () => { + test('clean brain (no markdown files) → status="ok" with "no missing" summary', async () => { + try { + const result = await runPhaseBacklinks(brainDir, false); + expect(result.phase).toBe('backlinks'); + expect(result.status).toBe('ok'); + expect(result.summary.toLowerCase()).toMatch(/no missing back-links|^no\b/); + expect(result.error).toBeUndefined(); + expect(result.details).toMatchObject({ mode: 'audit-only' }); + } finally { + cleanupBrain(); + } + }); + + test('brain with content but no link gaps → status="ok"', async () => { + try { + mkdirSync(join(brainDir, 'people'), { recursive: true }); + writeFileSync( + join(brainDir, 'people', 'alice-example.md'), + '---\ntitle: Alice\ntype: person\n---\n\nNo links here.\n', + ); + const result = await runPhaseBacklinks(brainDir, false); + expect(result.phase).toBe('backlinks'); + // Even with gaps, the wrapper returns status='ok' — backlinks is + // audit-only by design (v0.22+). Mode reflects that. + expect(result.status).toBe('ok'); + expect(result.details).toMatchObject({ mode: 'audit-only' }); + } finally { + cleanupBrain(); + } + }); + + test('dry-run path → status="ok", dryRun in details', async () => { + try { + const result = await runPhaseBacklinks(brainDir, true); + expect(result.phase).toBe('backlinks'); + expect(result.status).toBe('ok'); + expect(result.details).toMatchObject({ dryRun: true, mode: 'audit-only' }); + } finally { + cleanupBrain(); + } + }); + + test('throw-from-lib (nonexistent brainDir) → status="fail" with error envelope', async () => { + const missingDir = join(tmpdir(), `nonexistent-brain-bl-${Date.now()}-${Math.random()}`); + const result = await runPhaseBacklinks(missingDir, false); + expect(result.phase).toBe('backlinks'); + expect(result.status).toBe('fail'); + expect(result.summary).toBe('backlinks phase failed'); + expect(result.error).toBeDefined(); + expect(typeof result.error!.code).toBe('string'); + // Same try/catch envelope contract as runPhaseLint above. + }); +}); + +describe('phase wrappers — contract invariants shared by both', () => { + test('PhaseResult.phase always matches the wrapper name', async () => { + try { + const lint = await runPhaseLint(brainDir, false); + const backlinks = await runPhaseBacklinks(brainDir, false); + expect(lint.phase).toBe('lint'); + expect(backlinks.phase).toBe('backlinks'); + } finally { + cleanupBrain(); + } + }); + + test('every PhaseResult has a string summary AND object details', async () => { + try { + for (const result of [ + await runPhaseLint(brainDir, false), + await runPhaseBacklinks(brainDir, false), + ]) { + expect(typeof result.summary).toBe('string'); + expect(result.summary.length).toBeGreaterThan(0); + expect(typeof result.details).toBe('object'); + } + } finally { + cleanupBrain(); + } + }); + + test('successful runs leave error undefined (envelope is fail-only)', async () => { + try { + const lint = await runPhaseLint(brainDir, false); + const backlinks = await runPhaseBacklinks(brainDir, false); + expect(lint.error).toBeUndefined(); + expect(backlinks.error).toBeUndefined(); + } finally { + cleanupBrain(); + } + }); +}); diff --git a/test/doctor-behavioral.test.ts b/test/doctor-behavioral.test.ts new file mode 100644 index 000000000..2f9a654f8 --- /dev/null +++ b/test/doctor-behavioral.test.ts @@ -0,0 +1,227 @@ +/** + * v0.39 behavioral coverage for `gbrain doctor`'s check orchestrator. + * + * Drives the exported `buildChecks(engine, args, dbSource): Promise` + * seam directly (added in v0.39 — extracted from runDoctor's body so the + * orchestrator is unit-testable without process.exit). Pairs with the + * subprocess smoke at test/doctor-cli-smoke.test.ts which covers the + * runDoctor render + exit-code path the seam can't reach in-process. + * + * Coverage strategy (D2 — "outcome-shaped + snapshot pin"): + * - Snapshot pin the check-name list against a fresh PGLite brain so + * accidental check drop-outs during refactors fail loudly with a + * reviewable diff at PR time. + * - Exercise computeDoctorReport math (3 fails → 60 pts lost, etc.) with + * synthesized inputs so the aggregation contract is pinned. + * - Honor the --fast flag's skip-set (DB checks absent). + * - --json arg is wrapper-only; buildChecks output unaffected. + * + * Per-check leaf coverage is deferred to a TODO (see plan): doctor.ts + * exports 20+ check helpers (whoknowsHealthCheck, takesWeightGridCheck, …) + * that warrant their own parameterized test file. + */ +import { describe, expect, test, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { + buildChecks, + computeDoctorReport, + type Check, +} from '../src/commands/doctor.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +describe('computeDoctorReport — pure score aggregation', () => { + test('empty checks → healthy, score 100', () => { + const r = computeDoctorReport([]); + expect(r.schema_version).toBe(2); + expect(r.status).toBe('healthy'); + expect(r.health_score).toBe(100); + expect(r.checks).toEqual([]); + }); + + test('all ok → healthy, score 100', () => { + const checks: Check[] = [ + { name: 'a', status: 'ok', message: '' }, + { name: 'b', status: 'ok', message: '' }, + { name: 'c', status: 'ok', message: '' }, + ]; + const r = computeDoctorReport(checks); + expect(r.status).toBe('healthy'); + expect(r.health_score).toBe(100); + }); + + test('one warn → warnings, -5 points', () => { + const checks: Check[] = [ + { name: 'a', status: 'ok', message: '' }, + { name: 'b', status: 'warn', message: '' }, + ]; + const r = computeDoctorReport(checks); + expect(r.status).toBe('warnings'); + expect(r.health_score).toBe(95); + }); + + test('one fail → unhealthy, -20 points', () => { + const r = computeDoctorReport([{ name: 'a', status: 'fail', message: '' }]); + expect(r.status).toBe('unhealthy'); + expect(r.health_score).toBe(80); + }); + + test('3 fails → unhealthy, -60 points (audit-driver math)', () => { + const checks: Check[] = [ + { name: 'a', status: 'fail', message: '' }, + { name: 'b', status: 'fail', message: '' }, + { name: 'c', status: 'fail', message: '' }, + ]; + const r = computeDoctorReport(checks); + expect(r.status).toBe('unhealthy'); + expect(r.health_score).toBe(40); + }); + + test('mixed ok + warn + fail → unhealthy (fail dominates)', () => { + const r = computeDoctorReport([ + { name: 'a', status: 'ok', message: '' }, + { name: 'b', status: 'warn', message: '' }, + { name: 'c', status: 'fail', message: '' }, + ]); + // 1 fail (-20) + 1 warn (-5) = 75; fail status dominates ranking. + expect(r.status).toBe('unhealthy'); + expect(r.health_score).toBe(75); + }); + + test('score is clamped at 0 (never negative)', () => { + const many: Check[] = Array.from({ length: 10 }, (_, i) => ({ + name: `f${i}`, + status: 'fail' as const, + message: '', + })); + const r = computeDoctorReport(many); + expect(r.health_score).toBe(0); + expect(r.status).toBe('unhealthy'); + }); +}); + +describe('buildChecks — orchestrator against PGLite', () => { + test('returns a non-empty Check[] against a fresh brain', async () => { + const checks = await buildChecks(engine, []); + expect(Array.isArray(checks)).toBe(true); + expect(checks.length).toBeGreaterThan(10); + // Every check has the contracted shape. + for (const c of checks) { + expect(typeof c.name).toBe('string'); + expect(['ok', 'warn', 'fail']).toContain(c.status); + expect(typeof c.message).toBe('string'); + } + }); + + test('snapshot: load-bearing check names always run against a fresh brain', async () => { + // Behavior-preservation snapshot (D6 lock). Pinning a CURATED subset of + // load-bearing checks rather than the full list so this test stays stable + // when new checks land deliberately — but breaks loudly if a known + // load-bearing check accidentally drops out during a refactor. + // + // Deliberately tight set: only checks whose code path is unconditional + // given an engine + has no env / config dependencies that vary between + // test processes. Adding more sensitive checks here (e.g. ones that + // read process.env) caused cross-shard flakes under the parallel runner. + // Per-check leaf coverage is the TODO that targets the broader surface. + const checks = await buildChecks(engine, []); + const names = new Set(checks.map(c => c.name)); + const loadBearing = [ + 'connection', + 'schema_version', + 'brain_score', + 'sync_freshness', + 'search_mode', + 'eval_drift', + 'reranker_health', + 'embedding_width_consistency', + 'autopilot_lock_scope', + ]; + // NOTE: sync_failures and slug_fallback_audit are deliberately NOT in + // the load-bearing set — they're only pushed when the corresponding + // JSONL file exists. Tests run on isolated tmpdir GBRAIN_HOMEs where + // those files may or may not exist depending on which sibling tests + // already wrote audit lines. + const missing = loadBearing.filter(n => !names.has(n)); + expect(missing, `load-bearing checks missing from buildChecks result: ${missing.join(', ')}`).toEqual([]); + // Plus a minimum total count — drops below ~30 means something + // bigger went wrong than the snapshot can name. + expect(checks.length).toBeGreaterThan(30); + }); + + test('--fast skips DB-dependent checks; filesystem checks still run', async () => { + // Fast-mode bails out of the DB section entirely. When an engine is + // available, the early-return skips pushing a connection check (no + // probe happened); when engine is null AND --fast, a warn-status + // synthesized connection check is added. Filesystem checks above + // the DB phase always run. + const checks = await buildChecks(engine, ['--fast']); + const names = new Set(checks.map(c => c.name)); + // DB-dependent checks should NOT be present. + expect(names.has('schema_version')).toBe(false); + expect(names.has('brain_score')).toBe(false); + expect(names.has('sync_freshness')).toBe(false); + // Filesystem checks above the DB phase still ran. + expect(names.has('resolver_health')).toBe(true); + + // The null-engine + --fast path DOES synthesize a connection warn. + const fsOnlyChecks = await buildChecks(null, ['--fast'], 'env:DATABASE_URL'); + const fsOnlyNames = new Set(fsOnlyChecks.map(c => c.name)); + expect(fsOnlyNames.has('connection')).toBe(true); + const conn = fsOnlyChecks.find(c => c.name === 'connection')!; + expect(conn.status).toBe('warn'); + expect(conn.message.toLowerCase()).toContain('fast'); + }); + + test('--json arg does NOT alter the returned check list', async () => { + // --json is a wrapper-level concern (controls outputResults render mode); + // the buildChecks seam should return the same checks regardless. + const without = await buildChecks(engine, []); + const withJson = await buildChecks(engine, ['--json']); + expect(withJson.map(c => c.name)).toEqual(without.map(c => c.name)); + }); + + test('returns partial check list when engine is null (no early process.exit)', async () => { + // Pre-v0.39 the no-engine path called outputResults + process.exit + // directly. Post-extract it returns the partial list so the wrapper + // decides exit code. This is the load-bearing assertion that proves + // the early-exit refactor preserved observable behavior. + const checks = await buildChecks(null, []); + expect(Array.isArray(checks)).toBe(true); + const connection = checks.find(c => c.name === 'connection'); + expect(connection).toBeDefined(); + expect(connection!.status).toBe('warn'); + }); + + test('mixed-outcome render path: synthesized checks aggregate as expected', () => { + // The orchestrator's render path (outputResults in the wrapper) reads + // the same DoctorReport.status enum we compute here. Pin the + // ok+warn+fail aggregation so the wrapper's render of a real mixed + // brain state is reproducible. + const mixed: Check[] = [ + { name: 'resolver_health', status: 'ok', message: '50 skills' }, + { name: 'sync_failures', status: 'warn', message: '2 unacked' }, + { name: 'schema_version', status: 'fail', message: 'mid-upgrade' }, + ]; + const r = computeDoctorReport(mixed); + expect(r.status).toBe('unhealthy'); + expect(r.health_score).toBe(75); + expect(r.checks).toHaveLength(3); + expect(r.checks.map(c => c.status)).toEqual(['ok', 'warn', 'fail']); + }); +}); diff --git a/test/doctor-cli-smoke.serial.test.ts b/test/doctor-cli-smoke.serial.test.ts new file mode 100644 index 000000000..ae5bd2129 --- /dev/null +++ b/test/doctor-cli-smoke.serial.test.ts @@ -0,0 +1,123 @@ +/** + * v0.39 subprocess smoke for `gbrain doctor --json`. + * + * Covers the runDoctor wrapper paths that buildChecks-only tests can't + * reach in-process (D10/CMT-2): outputResults render, process.exit code, + * --json envelope shape on the wire. Pre-v0.39 the wave had only + * source-grep tests for these paths; the v0.38.2.0 partial-scan wave + * showed that's not enough (render bugs slipped through). + * + * Spawns `bun run src/cli.ts doctor --json` against a fresh PGLite + * tempdir brain. Asserts exit code 0 on a freshly-initialized brain, + * JSON parses, schema_version=2 contract holds, status enum is one of + * the documented values, checks array is non-empty. + * + * Serial because it spawns subprocesses and writes a tmpdir. Skippable + * via `GBRAIN_SKIP_SUBPROCESS_TESTS=1` for fast-loop budget control. + * + * Per-spawn cold-start on CI is ~10-20s. Single test, single brain. + */ +import { describe, test, expect } from 'bun:test'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, chmodSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; + +const REPO = new URL('..', import.meta.url).pathname.replace(/\/$/, ''); +const SKIP = process.env.GBRAIN_SKIP_SUBPROCESS_TESTS === '1'; + +function makeGbrainShim(): { binDir: string; cleanup: () => void } { + const binDir = mkdtempSync(join(tmpdir(), 'gbrain-shim-doctor-')); + const shimPath = join(binDir, 'gbrain'); + writeFileSync(shimPath, `#!/bin/sh\nexec bun run ${REPO}/src/cli.ts "$@"\n`, { mode: 0o755 }); + chmodSync(shimPath, 0o755); + return { + binDir, + cleanup: () => { + try { rmSync(binDir, { recursive: true, force: true }); } catch { /* best effort */ } + }, + }; +} + +async function runCli( + args: string[], + env: Record, + timeoutMs: number, +): Promise<{ exitCode: number; stdout: string; stderr: string }> { + const proc = Bun.spawn(['bun', 'run', `${REPO}/src/cli.ts`, ...args], { + cwd: REPO, + env: { ...process.env, ...env }, + stdout: 'pipe', + stderr: 'pipe', + }); + const killer = setTimeout(() => { + try { proc.kill('SIGKILL'); } catch { /* already dead */ } + }, timeoutMs); + try { + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { exitCode, stdout, stderr }; + } finally { + clearTimeout(killer); + } +} + +describe('gbrain doctor --json subprocess smoke (D10/CMT-2)', () => { + test.skipIf(SKIP)('exits 0 on freshly-initialized PGLite brain; JSON envelope is well-formed', async () => { + const home = mkdtempSync(join(tmpdir(), 'gbrain-doctor-smoke-')); + const shim = makeGbrainShim(); + try { + mkdirSync(join(home, '.gbrain'), { recursive: true }); + writeFileSync( + join(home, '.gbrain', 'config.json'), + JSON.stringify({ + engine: 'pglite', + database_path: join(home, '.gbrain', 'brain.pglite'), + embedding_dimensions: 1536, + }) + '\n', + ); + const env = { + HOME: home, + GBRAIN_HOME: home, + PATH: `${shim.binDir}:${process.env.PATH ?? ''}`, + }; + + // Step 1: init + apply migrations so the brain is at head before doctor runs. + // Without this, the brain would be detected as mid-migration and doctor would + // (correctly) report partial state. + const init = await runCli(['init', '--migrate-only'], env, 90_000); + expect(init.exitCode).toBe(0); + + // Step 2: doctor --json against the fresh brain. This is the load-bearing + // assertion that covers runDoctor's wrapper (render + exit code) — the + // path that buildChecks-only tests deliberately don't exercise. + const doctor = await runCli(['doctor', '--json'], env, 120_000); + if (doctor.exitCode !== 0) { + console.error('--- doctor stdout ---\n' + doctor.stdout); + console.error('--- doctor stderr ---\n' + doctor.stderr); + } + expect(doctor.exitCode).toBe(0); + + // JSON envelope shape (schema_version=2 contract). + let parsed: unknown; + try { + parsed = JSON.parse(doctor.stdout); + } catch (e) { + throw new Error(`doctor --json output failed to parse: ${(e as Error).message}\n${doctor.stdout.slice(0, 500)}`); + } + const report = parsed as { schema_version: number; status: string; health_score: number; checks: unknown[] }; + expect(report.schema_version).toBe(2); + expect(['healthy', 'warnings', 'unhealthy']).toContain(report.status); + expect(typeof report.health_score).toBe('number'); + expect(report.health_score).toBeGreaterThanOrEqual(0); + expect(report.health_score).toBeLessThanOrEqual(100); + expect(Array.isArray(report.checks)).toBe(true); + expect(report.checks.length).toBeGreaterThan(0); + } finally { + try { rmSync(home, { recursive: true, force: true }); } catch { /* best effort */ } + shim.cleanup(); + } + }, 300_000); +}); diff --git a/test/ingestion/put-page-write-through.test.ts b/test/ingestion/put-page-write-through.test.ts index 37236439f..64b446289 100644 --- a/test/ingestion/put-page-write-through.test.ts +++ b/test/ingestion/put-page-write-through.test.ts @@ -15,6 +15,7 @@ import { PGLiteEngine } from '../../src/core/pglite-engine.ts'; import { resetPgliteState } from '../helpers/reset-pglite.ts'; import { operations } from '../../src/core/operations.ts'; import type { OperationContext } from '../../src/core/operations.ts'; +import { resetGateway } from '../../src/core/ai/gateway.ts'; let engine: PGLiteEngine; let tmpRoot: string; @@ -28,10 +29,23 @@ beforeAll(async () => { afterAll(async () => { await engine.disconnect(); + // Don't leak the reset-state to sibling files in the same bun shard + // (the v0.40.4.1 gateway state-leak class). beforeEach already reset + // for our own tests; this is defense for the next file's siblings. + resetGateway(); }); beforeEach(async () => { await resetPgliteState(engine); + // CI fix: put_page's handler at src/core/operations.ts:622 computes + // `noEmbed = !isAvailable('embedding')`. When the gateway has been + // configured by a sibling test (or by the cli.ts module-load path + // reading .env.testing) with a fake/stale ZEROENTROPY_API_KEY, + // isAvailable returns true → put_page tries to embed → the real + // ZeroEntropy API returns 401 in CI. This test exercises write-through + // behavior, not embedding. Reset the gateway so isAvailable returns + // false → noEmbed=true → no network call. + resetGateway(); tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gbrain-wt-')); brainDir = path.join(tmpRoot, 'brain'); fs.mkdirSync(brainDir, { recursive: true }); diff --git a/test/operations-trust-boundary.test.ts b/test/operations-trust-boundary.test.ts new file mode 100644 index 000000000..5fe5db4dd --- /dev/null +++ b/test/operations-trust-boundary.test.ts @@ -0,0 +1,267 @@ +/** + * v0.39 trust-boundary contract test (GAP 3 of the e2e-test-wave audit). + * + * Hybrid design (D7 — pure + targeted handler invocation): + * + * - Pure assertions over ALL operations (~74 ops): scope annotations + * present + correct; localOnly ops are filtered out of the canonical + * mcpOperations list; hasScope semantics work for the standard tiers. + * + * - Handler-invocation cases for ops that are NOT localOnly but DO + * enforce remote/scope at the handler layer (defense-in-depth where + * it actually fires in production): + * + * * submit_job — name='shell' + ctx.remote=true MUST reject + * (the HTTP MCP shell-job RCE class, F7b) + * * search_by_image — image_path + ctx.remote=true MUST reject + * (D18 P0 source-isolation leak class) + * + * `file_upload` and `sync_brain` are intentionally NOT in the + * handler-invocation set — both are localOnly, so the canonical + * filter removes them from mcpOperations and the HTTP path never + * reaches their handlers. Calling their handlers with remote=true + * tests an impossible production path (codex CMT-3). The defense- + * in-depth strict-mode checks inside those handlers still exist; + * they're proven by the localOnly-filtered-out contract here. + * + * Criterion for the curated sensitive-ops list: + * ops whose HANDLER (not transport) has been broken historically. + * Add an op here when a real exploit class is fixed at the handler + * level; remove only when the handler-level defense becomes + * structurally unreachable (e.g., the op becomes localOnly). + * + * Companion guard at scripts/check-operations-filter-bypass.sh enforces + * the canonical filter site so a future HTTP route can't bypass it. + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { operations, type OperationContext } from '../src/core/operations.ts'; +import { hasScope } from '../src/core/scope.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +// Minimal context factory — every test that invokes a handler builds +// one of these. Defaults to remote=true (untrusted) because that's the +// trust posture the bug-class regressions live in; tests opt back to +// local trust by overriding remote=false. +function makeContext(overrides: Partial = {}): OperationContext { + return { + engine: engine as any, + config: {} as any, + logger: console as any, + dryRun: false, + remote: true, + sourceId: 'default', + ...overrides, + }; +} + +describe('operations contract — every op has scope + correct mutability shape', () => { + test('every op declares a scope annotation', () => { + for (const op of operations) { + expect(op.scope, `op "${op.name}" missing scope annotation`).toBeDefined(); + } + }); + + test('every mutating op has a write-class scope (not "read")', () => { + const WRITE_CLASS_SCOPES = new Set([ + 'write', + 'admin', + 'sources_admin', + 'users_admin', + 'agent', + ]); + for (const op of operations) { + if (op.mutating === true) { + expect( + WRITE_CLASS_SCOPES.has(op.scope ?? 'read'), + `mutating op "${op.name}" has read-tier scope "${op.scope}"; expected one of ${[...WRITE_CLASS_SCOPES].join('/')}`, + ).toBe(true); + } + } + }); + + test('scope is one of the documented enum values', () => { + const KNOWN_SCOPES = new Set([ + 'read', + 'write', + 'admin', + 'sources_admin', + 'users_admin', + 'agent', + ]); + for (const op of operations) { + expect( + KNOWN_SCOPES.has(op.scope!), + `op "${op.name}" has unknown scope "${op.scope}"`, + ).toBe(true); + } + }); +}); + +describe('mcpOperations filter — localOnly ops are excluded from the HTTP-exposed surface', () => { + // This filter is what serve-http.ts uses to build the tools/list response: + // const mcpOperations = operations.filter(op => !op.localOnly); + // A localOnly op that leaks into mcpOperations is exposed via HTTP MCP + // and bypasses the trust boundary. Pin the filter contract here so a + // regression surfaces as a structural test failure. + + test('the canonical filter excludes every localOnly op', () => { + const mcpOps = operations.filter(op => !op.localOnly); + const mcpNames = new Set(mcpOps.map(op => op.name)); + const localOnlyOps = operations.filter(op => op.localOnly === true); + + expect(localOnlyOps.length).toBeGreaterThan(0); + for (const op of localOnlyOps) { + expect( + mcpNames.has(op.name), + `localOnly op "${op.name}" leaked into the HTTP MCP surface`, + ).toBe(false); + } + }); + + test('known historically-sensitive localOnly ops stay filtered', () => { + // Pin every localOnly op by name so a refactor that flips localOnly off + // on any of them fails this test even if the generic contract above + // somehow regresses. Codex /ship review caught the original 4-name + // snapshot was missing purge_deleted_pages, get_recent_transcripts, and + // code_traversal_cache_clear — additions that already qualified. + // + // When adding a NEW localOnly op: add its name here too. The generic + // contract above proves the filter rule applies; this list proves the + // specific ops we care about haven't silently shed their localOnly flag. + const KNOWN_LOCAL_ONLY = [ + 'sync_brain', + 'file_upload', + 'file_list', + 'file_url', + 'purge_deleted_pages', + 'get_recent_transcripts', + 'code_traversal_cache_clear', + ]; + const lookup = new Map(operations.map(op => [op.name, op] as const)); + for (const name of KNOWN_LOCAL_ONLY) { + const op = lookup.get(name); + expect(op, `expected canonical op "${name}" to still exist`).toBeDefined(); + expect(op!.localOnly, `"${name}" must stay localOnly`).toBe(true); + } + }); +}); + +describe('hasScope — read-only token cannot satisfy write or admin scopes', () => { + // The HTTP path computes `requiredScope = op.scope || 'read'` and gates + // every call on `hasScope(authInfo.scopes, requiredScope)`. Pin the + // semantics here so a refactor of the IMPLIES table can't silently + // grant admin via a read-class token. + test('read scope does NOT satisfy write', () => { + expect(hasScope(['read'], 'write')).toBe(false); + }); + + test('read scope does NOT satisfy admin', () => { + expect(hasScope(['read'], 'admin')).toBe(false); + }); + + test('write scope satisfies write AND read', () => { + expect(hasScope(['write'], 'write')).toBe(true); + expect(hasScope(['write'], 'read')).toBe(true); + }); + + test('admin scope satisfies admin, write, AND read (umbrella implies)', () => { + expect(hasScope(['admin'], 'admin')).toBe(true); + expect(hasScope(['admin'], 'write')).toBe(true); + expect(hasScope(['admin'], 'read')).toBe(true); + }); + + test('unknown scope strings are ignored, do not satisfy anything', () => { + expect(hasScope(['bogus'], 'read')).toBe(false); + expect(hasScope(['bogus'], 'write')).toBe(false); + }); + + test('every read-scope op accepts a read-only token; every write-scope op rejects it', () => { + // Walk the op surface and assert that a synthetic read-only token + // satisfies every read-scope op but no write/admin op. + const READ_TOKEN_SCOPES = ['read'] as const; + for (const op of operations) { + const required = op.scope ?? 'read'; + const accepted = hasScope(READ_TOKEN_SCOPES, required); + if (required === 'read') { + expect(accepted, `read op "${op.name}" should accept read-only token`).toBe(true); + } else { + expect(accepted, `${required} op "${op.name}" must reject read-only token`).toBe(false); + } + } + }); +}); + +describe('handler invocation — historically-broken trust-boundary classes', () => { + // The two non-localOnly ops whose handler-level defense fires in + // production and has been broken historically (F7b HTTP MCP shell-job + // RCE; D18 P0 image_path remote-leak). file_upload and sync_brain are + // omitted because they're localOnly (codex CMT-3 — testing their + // handlers with remote=true tests an impossible production path). + + test('submit_job rejects shell with ctx.remote=true (HTTP MCP shell-job RCE class)', async () => { + const submitJob = operations.find(op => op.name === 'submit_job'); + expect(submitJob).toBeDefined(); + const ctx = makeContext({ remote: true }); + + let threw = false; + let message = ''; + try { + await submitJob!.handler(ctx, { name: 'shell', data: { cmd: 'echo hi' } }); + } catch (e) { + threw = true; + message = e instanceof Error ? e.message : String(e); + } + expect(threw, 'submit_job(shell) with remote=true MUST reject').toBe(true); + // Should mention the protected status — "permission_denied" is the + // canonical OperationError code, plus the user-facing string names + // the rejected name. + expect(message.toLowerCase()).toContain('shell'); + }); + + test('submit_job allows shell when ctx.remote=false (local CLI is trusted)', async () => { + // The flip side of the trust boundary: a local trusted caller with + // explicit remote=false MUST be allowed to submit shell jobs (that's + // how the CLI works in production). We don't actually want to run the + // job — pass dryRun so the op short-circuits. + const submitJob = operations.find(op => op.name === 'submit_job'); + const ctx = makeContext({ remote: false, dryRun: true }); + + const result = await submitJob!.handler(ctx, { name: 'shell', data: { cmd: 'echo hi' } }); + expect(result).toMatchObject({ dry_run: true, action: 'submit_job', name: 'shell' }); + }); + + test('search_by_image rejects image_path with ctx.remote=true (D18 P0)', async () => { + const searchByImage = operations.find(op => op.name === 'search_by_image'); + expect(searchByImage).toBeDefined(); + const ctx = makeContext({ remote: true }); + + let threw = false; + let message = ''; + try { + await searchByImage!.handler(ctx, { image_path: '/tmp/some-image.png' }); + } catch (e) { + threw = true; + message = e instanceof Error ? e.message : String(e); + } + expect(threw, 'search_by_image(image_path) with remote=true MUST reject').toBe(true); + expect(message.toLowerCase()).toContain('image_path'); + expect(message.toLowerCase()).toContain('permission_denied'); + }); +});