mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-15 17:32:37 +00:00
Garry TanandClaude Fable 5
83a4a94c38
* fix(cycle,locks): fenced lock identity + steal-abort — the cycle lock is actually refreshed in production (W0 Tier-1 #1) The 2026-08-14 audit (CONFIRMED by adversarial verification) found the cycle DB lock was effectively never refreshed: lock.refresh() was reachable only through buildYieldDuringPhase, three of five pass sites handed phases the raw caller hook, and NO production caller (jobs.ts, autopilot.ts) sets yieldDuringPhase at all — so with the 5-minute TTL against 35-minute subagent waits, every long cycle lost its lock mid-run and a second cycle could start against the same source. Fixes, per the fix-wave plan (D5.10/D5.11/D5.6): - db-lock: refresh() and release() predicates now require the acquisition fence (id, holder_pid, acquired_at::text) captured at acquire time, so a PID-reuse impostor or a stolen handle can never refresh or delete a successor's row. refresh() returns true only while owned; a fenced miss is distinguished from transient DB errors (which still throw and retry). - cycle: runCycle owns a SERIALIZED background refresher (6x per TTL window, GBRAIN_CYCLE_LOCK_REFRESH_MS escape hatch) for the cycle lock only — Minion job-lock renewal stays on the phase-boundary hooks per the cycle.ts:618 decision. A detected steal aborts an internal controller; the combined signal reaches every existing checkAborted() boundary, and the five long phases (synthesize, extract_atoms, patterns, synthesize_concepts, consolidate) race their awaits against it since their opts cannot carry a signal yet. The three raw yieldDuringPhase pass sites are now wrapped. - A steal returns a structured partial report (reason 'lock_stolen') instead of throwing; completed phases' writes are durable, the freshness stamp is skipped, and the fenced release leaves the successor's row intact. - supervisor: a fenced refresh returning false is CERTAIN lock loss, not a blip — exit LOCK_LOST immediately instead of resetting the failure counter. - withRefreshingLock: stops its heartbeat and reports loudly when the fenced refresh proves the lock gone. Closes TODO-OPS-2 (refresh had no rows-affected check, so lock loss was undetectable). Tests: db-lock-fencing (fence round-trip, steal → refresh false, fenced release no-op, refresher abort/serialization/transient-vs-steal, yield hook steal reporting), cycle-lock-steal.serial (end-to-end mid-run steal → partial/lock_stolen report, no further phases, successor row intact + steal-free regression guard). All pre-existing lock suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(minions): reset started_at on every automatic re-run path (W0 Tier-1 #7) handleWallClockTimeouts anchors on now() - started_at, but only the manual `jobs retry` path cleared started_at — its own docstring documented the bug. The four automatic paths (failJob's delayed branch, handleStalled's requeue, promoteDelayed, and releaseLeaseFullJob — the fourth site surfaced by adversarial verification) preserved the FIRST claim's timestamp, so an exponential-backoff job burned its wall-clock budget while parked in 'delayed' and could be dead-lettered before executing a single line of its retry attempt. All four paths now clear started_at; claim()'s COALESCE re-stamps per attempt. Terminal failures (failed/dead) keep started_at for duration accounting. Pinned end-to-end: a job whose first attempt ran an hour survives the sweep on its fresh attempt, and the negative control proves the sweep still kills genuinely overrunning attempts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(minions): shared killJobs tail — stall-death notifies parents; reapers use parents-first lock order (W0 Tier-1 #4) handleStalled's dead-letter branch set status='dead' and emitted NOTHING: no child_done inbox row, no aggregator unblock. A child that died via max-stall stranded its parent in 'waiting-children' forever — the exact hang the v0.15 comment says was fixed for timeouts (resolveParent has no periodic caller; the worker only logs counts). Meanwhile handleTimeouts and handleWallClockTimeouts carried two verbatim copies of the ~45-line notify-and-unblock block. - One private killJobs(tx, rows, outcome, errorText) now owns the child_done insert + waiting-children unblock; all three reapers route through it. handleStalled's dead branch emits outcome 'dead' / 'max stalled count exceeded' (distinct from 'timeout' so consumers can tell stall-death from overrun). - Deadlock safety (Codex eng-review D5.12): failJob locks the parent BEFORE touching the child, while the reapers previously updated children first — opposite lock order. All three reapers now discover candidates with a plain read, lock parents in ascending-id order via lockParentsOrdered(), then transition children under a re-checked FOR UPDATE SKIP LOCKED subselect in the same transaction. Pinned: stall-exhausted child → child_done(dead) + parent flips to waiting; budget-remaining stall requeues without touching the parent; all three reapers' outcome/error strings asserted through the shared tail (D5.5). Full minions e2e suite green (187 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(embed): carry modality through every re-embed path — one shared field list (W0 Tier-1 #3) CONFIRMED in the audit + adversarial verification: preserveCodeMetadata (commands/embed.ts) rebuilt ChunkInputs without `modality`, and upsertChunks overwrites that column from EXCLUDED — so every CLI re-embed path (embedPage, embed --all, embed --stale, including the autopilot-reachable stale loop) flipped image chunks to modality='text'. The image search arm filters cc.modality='image', so image retrieval silently went to zero while keyword search started returning raw OCR text. The minion twin in core/embed-stale.ts carried modality correctly and its comment documented this exact hazard — the two hand-copied field lists had diverged. carryChunkMetadata (core/embed-stale.ts) is now the single carry list; preserveCodeMetadata delegates to it, killing the divergence class at the root (the full loop merge lands in W6). embedding_image stays deliberately un-carried (COALESCEd by the upsert; getChunks returns pgvector strings). Pinned: the carry preserves modality + all 8 code-metadata fields; an image chunk round-trips the stale-merge intact; and the write-side contract test documents WHY the carry is load-bearing (omission demonstrably resets to 'text'). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(import): throw typed ImportAbortError instead of process.exit — MCP server survives failed preflights (W0 Tier-1 #5) runImport called process.exit(1) at five preflight/argv sites (deferred-setup sentinel, missing embedding credentials, invalid --workers, missing dir, unreadable dir). Correct for the CLI — but runImport is invoked IN-PROCESS by the sync_brain MCP op (via performFullSync), the autopilot daemon, and the minion sync handler, so a first/forced sync against a brain with unusable embedding credentials terminated the stdio MCP server mid-tool-call with no error envelope (verified reachable in adversarial review; daemon/worker paths are partially shielded by noEmbed defaults, the MCP path was not). The five sites now throw ImportAbortError (exitCode, alreadyReported) AFTER printing their user-facing messages exactly as before; the CLI dispatch case maps the error to process.exit(exitCode) — byte-identical CLI behavior. The in-process callers get a normal error: the MCP op returns an error envelope, the job handler fails the job, the daemon logs and continues. Pinned: three abort classes throw typed (not exit), and the calling process demonstrably survives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(lint): single scan for --fix — true fixed count, half the work (W0 Tier-1 #14) runLint ran its own full read+lint+fix loop for human output, then called runLintCore a second time for the summary line. Every page was linted twice, and because the first pass had already written the fixes, the second pass's total_fixed counted against already-fixed content — `gbrain lint --fix` printed "0 auto-fixed." after fixing N issues. runLintCore now exposes per-page hooks (onPageScanned for the progress bar, onPageIssues with the applied fix count); the CLI streams its human detail from the same single pass that produces the canonical counts. Pinned: two pages scan as exactly two ticks, total_fixed matches the page-level fix count, the fix lands on disk, and a second run reports 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): confirm prompts resolve on EOF and refuse non-TTY in-prompt (W0 Tier-1 #15) Port-ledger note: since the audit, both destructive-command callers (pglite-repair, reinit-pglite) gained caller-side non-TTY guards ('Non-TTY environment requires --yes'), so the original always-hangs case is already blocked upstream. The residual: a TTY session whose stdin hits EOF mid-prompt still parked forever — pglite-repair's readline had no 'close' handler and reinit-pglite's raw data-listener had no 'end' path (and its prompt wrote to stdout, polluting --json output). Both prompts now: refuse non-TTY in-prompt (defense-in-depth, safe default false), resolve(false) on EOF/close, prompt on stderr, and clean up their listeners. Decline paths and --yes/-y escape hatches unchanged. No new test: exercising EOF-mid-TTY needs a PTY harness — the W5 prompt canonicalization (core/prompt.ts) picks that up when all seven prompt copies converge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): guard self-test harness — a guard that cannot fail is not coverage (W0 Tier-1 #11) The audit proved scripts/check-no-double-retry.sh had been PERMANENTLY GREEN since it shipped: its `[^)]*` regex could not cross the `)` in `() =>`, so the canonical banned shape `withRetry(() => engine.addLinksBatch(...))` was invisible, and its multi-line fallback was gated on pcregrep — installed neither locally nor in CI. check-jsonb-pattern.sh carried the same nested-paren hole. Two more structural findings: package.json's `check:all` was a second, stale, hand-synced guard registry (the exact disease this fix-wave exists to cure), and three guards were reachable ONLY from it — i.e. never run anywhere. - Both regexes fixed; the no-double-retry multi-line pass now uses perl (always present) instead of pcregrep (never present). Real tree verified clean under the fixed patterns. - scripts/guards-manifest.tsv is THE single guard registry: all 45 guards classified (scanner / buildfresh / repostate, per Codex D5.14 — build and freshness guards are exempt-with-reason, not fixture-tested). - scripts/guard-self-test.sh runs every selftest=yes scanner against known-bad (must fail) and known-good (must pass) fixture trees via the GBRAIN_GUARD_ROOT seam, enforces manifest completeness for new guards, and carries a runtime budget (D4.5) so guard sprawl surfaces here first. Wired into `bun run verify`; adding a self-test = flip a manifest flag + two fixture files. - `check:all` deleted; its three orphaned guards (newlines, exports-count, no-legacy-getconnection) verified green and wired into the real registry. The bad fixtures are the exact shapes the old regexes missed — the harness fails loudly on the pre-fix scripts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test-infra): PGLite snapshot default-on for `bun run test` — idempotent, shard-safe, handler-aware (W0 Tier-1 #16) 500+ test files each cold-boot PGLite and replay all 126 migrations, but the snapshot fixture that skips that was enabled ONLY inside scripts/ci-local.sh — the everyday `bun run test` loop paid full cold-init on every file (measured: 1.63s → 0.91s per PGLite-booting file with the fixture). - run-unit-parallel.sh (the `bun run test` entrypoint) builds + exports the snapshot BEFORE its shard fan-out. Opt out: GBRAIN_NO_SNAPSHOT=1. - build-pglite-snapshot.ts is now idempotent: hash short-circuit exits in ~40ms when fresh, and REBUILDS stale snapshots — the old build-if-missing guard left a stale-but-present snapshot permanently on the warn+slow path. ci-local.sh now calls it unconditionally. - Shard/workspace concurrency safety (Codex D5.8): atomic mkdir lock with takeover-on-stale; tar written first, version file last, so a crash can never leave a fresh-looking torn fixture. - Hash soundness (Codex D5.13 / #4): 19+ migrations carry executable `handler` code with empty sql — invisible to the sql-only hash, so editing a handler reused a stale snapshot. The handler SOURCE now folds into the hash via Function.prototype.toString. Migration-replay coverage is unchanged: the replay canary tests clear GBRAIN_PGLITE_SNAPSHOT themselves and migrate.test.ts exercises runMigrations directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test-infra): snapshot bakes the pinned test embedding shape; loader refuses shape mismatches (W0 follow-through) Turning the snapshot default-on exposed a latent poisoning class: the build script ran with an UNCONFIGURED gateway, so initSchema fell back to the shipped default (1280-d zembed columns) — while bunfig's preload pins every `bun test` file to the legacy OpenAI 1536-d shape. The moment tests loaded the fixture, every embedding write failed with "expected 1280 dimensions, not 1536" (115 suite failures from one root cause). - The pinned shape now lives ONCE in test/helpers/legacy-embedding-config.ts; both the bunfig preload and the snapshot build script consume it (no hand-copied twins — the exact disease this wave cures). The build also isolates GBRAIN_HOME so ambient machine config can't leak in. - The version file records dims= and model= alongside the schema hash; the loader resolves its own would-be shape through the same gateway-or-default fallback initSchema uses and REFUSES a shape-mismatched snapshot (falls back to cold init with a rebuild hint). Pre-W0 hash-only version files read as stale. A test that reconfigures the gateway to a different shape now correctly bypasses the fixture instead of writing into wrong columns. - The build's freshness short-circuit checks all three lines. - Rephrased a guard comment that spelled a batch-call token literally — check-system-of-record scans scripts/ comments (the prose-bleed class, third occurrence this month). put-page-provenance: 9 fail → 0 under the fixture, still 3x faster than cold init. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(eval): fix-wave baseline metrics — measure the 10x claim (W0, D4.13) Records the series' starting numbers: god-file line counts (the registry waves' targets), guard census (47 guards / 3 self-tested / single registry), and the measured snapshot speedup (1.63s → 0.91s per PGLite test file). Each wave PR appends its row; the deltas are the receipt. The retrieval- quality canary (eval gate on a non-production brain) is documented as the mandatory pre-W1 step — W0 touches no search paths and the production brain is single-writer-held by the live serve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cycle,ci): duck-type-tolerant signal combining + contract-test updates (W0 follow-through) - anyAbortSignal no longer uses AbortSignal.any: CycleOpts.signal has always been duck-typed in practice (test stubs pass { aborted: false } and flip the flag; pre-W0 the raw object flowed straight into checkAborted). AbortSignal.any threw ERR_INVALID_ARG_TYPE and broke the autopilot-cycle handler suite. Manual fan-in: real signals propagate via listener, listener-less stubs are polled at 50ms, and the RETURNED signal is a genuine AbortSignal so phases can hand it to fetch/timers. - cycle-abort.test.ts source-contract tests updated to the cycleSignal truth (boundaries now check the combined external+steal signal) and additionally pin that the combine folds BOTH sources. - Restored the `typecheck` entry an errant edit dropped from run-verify-parallel's CHECKS array (caught by its own contract test — the registry pinning working as designed). - De-flaked the refresher steal test: poll to a 5s deadline instead of a fixed 120ms sleep (shard-load timer starvation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: pre-landing review fixes — 5 specialists + coverage audit findings (W0 ship pass) Specialist review (testing/maintainability/security/performance/data-migration, 5 parallel fresh-context reviewers + ship coverage audit at 92%) on the W0 diff. Every accepted finding fixed in-line: - db-lock: fence rendered as extract(epoch from acquired_at)::text — THREE specialists independently flagged timestamptz::text as GUC-fragile (the fence is captured on the acquire pool but compared on the direct pool; a TimeZone/DateStyle divergence would turn every refresh into a false steal and loop the supervisor through LOCK_LOST). Epoch text is session-invariant. - cycle: anyAbortSignal returns {signal, dispose}; runCycle disposes in its finally — the forward listener lives on the CALLER's signal and the autopilot daemon reuses one shutdown signal across every tick, so undisposed combines accumulated listeners + captured controllers for the daemon's lifetime (MaxListenersExceededWarning within ~10 ticks). Stub poll timers clear on dispose too. Helper moved out of the import block and behaviorally tested (5 cases incl. the daemon-leak class). - queue: retroactive stranded-parent sweep on every handleStalled tick — the per-kill unblock was forward-only, so parents stranded by PRE-upgrade stall-deaths (children already 'dead') never healed. Idempotent NOT-EXISTS UPDATE; pinned with stranded-heals + live-child-stays tests. - build-pglite-snapshot: the stale-lock takeover could NEVER acquire (mkdirSync on an existing dir always throws), so one crashed builder left every future rebuild waiting the full deadline then proceeding UNLOCKED forever. Takeover now removes the stale dir first; lock timeout is env-tunable; hermetic setup moved into main() (ESM hoisting made the module-scope placement illusory) and the temp home is cleaned up. - check-no-double-retry.sh: the perl multi-line pass exited 1 from clean batches — under pipefail, xargs's 123 would override grep's verdict the moment src/ outgrows one batch (a future silent miss of the exact class this guard just got cured of; repro'd by the reviewer). Output-presence now decides; multi-line bad fixture added so the pass self-tests. - check-jsonb-pattern.sh: the widened greedy pattern false-positived a SAFE ::text::jsonb line followed by a paren-bearing ${expr()}::jsonb on the same line (proven by repro); bracket-bounded [^}]* pattern can't span interpolations — good fixture now pins the multi-interpolation shape. - check-engine-dynamic-import.ts: also matches require() calls (the new snapshot-loader require was invisible to the guard, its marker decorative); 4 pre-existing lazy requires in tryLoadSnapshot marked with their existing justification. - Coverage gaps closed: supervisor fenced-false → immediate LOCK_LOST test; snapshot shape/hash guard tests (pre-W0 version files refused, dims/model mismatch refused, handler-edit changes the hash); anyAbortSignal behavior suite; steal-test window widened 200ms → 1.5s (shard-load starvation). - lint: tree walked once (onPagesCollected sizes the progress bar; the CLI's extra collectPages walk removed); stale docstrings corrected (hook fires AFTER the fix attempt; carry list includes modality). - Suite hygiene: 3 fresh-brain-premise tests opt out of the default-on snapshot; the check:all contract test now pins the single CHECKS registry. - TODOS.md: 6 fix-wave deferrals filed (each individually decided in review); TODO-OPS-2 marked CLOSED by this wave. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: red-team findings — unfenced file-lock half, 5th started_at path, sync-phase steal coverage (W0 ship pass 2) The post-specialist red team found what five specialists and the coverage audit all missed — two of them critical: - cycle (CRITICAL): the PGLite composite lock's FILE half was rewritten unconditionally even when the fenced DB refresh reported a steal — the losing holder clobbered the successor's file lock with its own pid on the very tick it detected the loss, after which its pid-checked file release DELETED the successor's only host-local protection mid-run (single-writer violation). The file half now rewrites only while the DB fence says owned. - queue (CRITICAL): fifth path of the started_at class — every waiting-children→waiting parent unblock (killJobs, completeJob resolve, failJob remove_dep/ignore, cancelJob, resolveParent, the new retroactive sweep: 7 sites) preserved the parent's attempt-1 anchor, so an aggregator whose children ran >5 minutes was wall-clock dead-lettered on re-claim — orphaning the exact child_done results the W0 parent-unblock fix just delivered. All 7 unblock sites now clear started_at; pinned by a parked-parent-survives-the-sweep test. - cycle: the sync phase — production's LONGEST await (resumable imports can run hours) — was the one long phase outside steal coverage. Now raced like the other five (sync checkpoints, holds its own per-source lock, and its stall watchdog bounds the dangling import). - build-pglite-snapshot: takeover verifies lock-dir mtime staleness before rmdir (two exhausted waiters could steal each other's LIVE lock); hermetic temp home created only past the short-circuit (was leaking one dir per `bun run test`). - Stale docs: jobs.ts import-handler comment claimed a process.exit that no longer exists; CLAUDE.md's engine-dynamic-import exception list now names the snapshot loader's require() cluster (build:llms regenerated in this commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: bump version and changelog (v0.45.15.0) W0 verified-bug hotfix wave of the code-smell fix-wave series. All six version locations synced (VERSION, package.json, openclaw.plugin.json, runbook stamp, template stamp, lockfile). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: update project documentation for v0.45.15.0 - docs/TESTING.md: tiers table now documents the default-on PGLite schema snapshot for `bun run test` (GBRAIN_NO_SNAPSHOT opt-out); new "PGLite schema snapshot" + "Guard registry and self-test" sections (build/loader contract, GBRAIN_SNAPSHOT_LOCK_TIMEOUT_MS, guards-manifest.tsv, GBRAIN_GUARD_ROOT); removed the deleted `check:all` tier; added the nine new W0 test suites to the unit-test inventory. - docs/architecture/KEY_FILES.md: current-state refresh for db-lock.ts (fenced handles, boolean refresh, LockStolenError), cycle.ts (dedicated serialized lock refresher, GBRAIN_CYCLE_LOCK_REFRESH_MS, steal-abort with reason lock_stolen, composed DB+file lock semantics; dropped the closed TODO-OPS-2 residual), minions/queue.ts (shared killJobs tail, lockParentsOrdered, stranded-parent sweep, started_at resets), supervisor.ts (fenced miss exits LOCK_LOST immediately), embed.ts (carryChunkMetadata shared field list), import.ts (typed ImportAbortError), lint.ts (single-pass --fix), pglite-repair.ts (EOF-safe stderr confirm prompts), check-no-double-retry.sh (arrow-paren-crossing pattern, perl fallback); new entries for guards-manifest.tsv + guard-self-test.sh and build-pglite-snapshot.ts; swept stale check:all references. - CONTRIBUTING.md: verify check count refreshed; check:all replaced with the guard-registry + self-test workflow. llms bundles verified fresh (bun run build:llms — no byte changes; test/build-llms.test.ts green). CHANGELOG/TODOS/VERSION already current from the ship pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: cross-model doc-review fixes for v0.45.15.0 (Codex pass) - CHANGELOG 0.45.15.0: the image-chunk recovery command is `gbrain backfill modality` (flipped chunks are not stale, so an embed --stale re-run cannot restore them — doctor names the same fix); upgrade note now also covers jobs supervisor/worker restarts; the prompt-hang fix names its two commands instead of implying all destructive prompts; guard self-test claim scoped to self-tested scanners. - KEY_FILES: cycle entry counts all 23 ALL_PHASES (was 9); raced-wait nuance for the 5 long phases (in-flight work runs to its bounded timeout); snapshot-lock last-resort unlocked path + version-file-not-tar gate scope; guards manifest registers/classifies but does not schedule (CHECKS array stays the execution list). - TESTING.md: same snapshot-lock last-resort honesty. - CONTRIBUTING.md: self-test scope (selftest=yes rows), stale ~85s inner-loop figure and 19+ check count refreshed. - FIX_WAVE_BASELINES.md: two W0 line counts refreshed per the doc's own method (post-ship-pass HEAD). llms bundle rebuilt (no byte changes); guards + build-llms test green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: re-bump to v0.45.16.0 (version queue collision with #4125) The sibling jobs fix wave (PR #4125, open) claims v0.45.15.0; per the user's call this PR advances past it. All six version locations re-synced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: withEnv() for snapshot opt-out in embedding-dim fresh-brain case (test-isolation guard) The W0 ship-pass fix used a manual save/delete/restore of GBRAIN_PGLITE_SNAPSHOT, which check-test-isolation rule R1 flags on CI (the local ship verify ran before this file gained the mutation). withEnv() scopes the opt-out to the connect() call with identical behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>