refactor(#12269): fail-fast DB adapters — throw ElizaError, drop fabricated defaults - #12602
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
QA design review: fail-fast/fail-closed direction is correct — turning a silently-fabricated fallback (or a silent routing drift) into an observable typed error/refusal is strictly safer than masking a broken pipeline. The one thing the tests must confirm (and do, per the PR) is that no caller relied on the old silent-fallback behavior. Not self-merging: this is behavior-changing and my review worktree was pruned by concurrent worktree contention so I can't run the tests locally; deferring to CI per my consistent policy on untestable behavior changes. Direction LGTM; recommend merge once CI is green. |
|
Follow-up pushed: What changed:
Local verification:
|
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Follow-up pushed: What changed:
Local verification:
|
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…icated defaults Reference PR for the #12182 DB-adapter fallback-slop family. Rewrites catch-return-default methods across plugin-sql, plugin-localdb, plugin-inmemorydb, and plugin-local-storage so a broken database surfaces a typed ElizaError instead of a healthy-looking 0 / [] / false / undefined. - plugin-sql base.ts: deleteAgents, updateAgent, countAgents (+ `|| 0`), createEntities, ensureEntityExists, updateComponent (also removed a server console.error), log, updateMemory, addParticipant, addParticipantsRoom, removeParticipant, createRelationship, getCache, setCache, deleteCache now throw ElizaError({ code, cause, context }); getCachedEmbeddings keeps its input-constraint empty-result as J3. createAgent/deleteAgent/cleanupAgents/ setParticipantUserState/updateRelationship rewritten to ElizaError or no-catch propagation; isReady annotated J4. - plugin-sql stores/*: same treatment for the parallel *.store.ts modules. - pglite/manager.ts: J6-annotated teardown catches (empty catch -> logger.debug), J3-annotated OS/parse probes. - entry points + advanced-memory-storage + schema-transformer: J3/J4 probe annotations; index.browser empty catch removed. - plugin-localdb: J3 ENOENT-first-boot, J5 write-chain link suppression. - plugin-local-storage: J3 ENOENT existence probe. Every kept handler carries a grep-able `// error-policy:J<N>` comment. Adds a real error-path test that closes a live PGlite out from under the adapter and asserts every method throws the right ElizaError code and that an API-style handler returns a structured 5xx (not a fabricated 200/0). Closes #12269 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bad67e2 to
e5c8c6c
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Rechecked current rebased head |
…allers (#12800) The DB-adapter slop sweep (#12602/#12269) correctly made getCache/setCache/ deleteCache fail-fast (throw ElizaError on DB error instead of returning undefined/false). But two best-effort callers were left calling the old return-a-default contract, which the throw now weaponizes: - PromptBatcher.invalidateCache did `void this.runtime.deleteCache(key)` (fire-and-forget, no .catch). A transient DB error during normal cache invalidation is now an UNHANDLED REJECTION — a non-fatal log in guarded long-running agents, but process termination in one-shot CLI (run-main unhandledRejection -> exit 1) and in hosts embedding @elizaos/core without the crash guards (Node 24 default). The in-memory entry is already cleared and a stale DB row is harmless, so this is a genuine J7 best-effort boundary: now .catch -> reportError, never crash. - AutonomyService.getCompactedAutonomyThoughts wrapped a DETERMINISTIC recompute in best-effort getCache/setCache. Post-sweep both throw on a DB blip and abort the (successful) compaction instead of degrading to recompute-and-return. Both cache ops are now try/catch'd (J7) -> reportError + continue; the entry is always returned. Found by an adversarial correctness audit of the merged fail-fast sweeps. Refs #12602 #12269 #12182 Co-authored-by: moon <stupidlybadadvice@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… prober contract, biome drift Repairs the accumulated red on develop so bun run verify passes again (modulo two pre-existing failures noted below): - @elizaos/core: export security/redact from the browser barrel — cloud-shared's logger (bundled into the app UI) imports isSensitiveKeyName/redactLogArgs from the root barrel (#12572 follow-up). - plugin-local-inference: fix two logger.warn(msg, {error}) calls to pino-style ({error}, msg) — introduced in #12546, broke typecheck of every package that type-includes the downloader source. - plugin-sql: carry the !agent.name narrowing across the withRetry closure in AgentStore.create (#12602 follow-up). - plugin-personal-assistant: adapt the website-blocking prober to the shared PermissionState contract (adds required platform field) instead of exposing plugin-blocker's narrower local shape. - plugin-tee: allowImportingTsExtensions (tsconfig is noEmit). - packages/ui: biome-ignore annotations for the two intentional registryVersion useMemo deps; type-collapse cast for the postcss/tailwind dual-major seam in the e2e fixture builder. - Repo-wide biome format normalization emitted by the self-fixing lint scripts (ui, agent, cloud-shared, cloud-ui and dependents). Still red on develop (pre-existing, untouched): @elizaos/app#typecheck (ui/navigation export resolution under tsgo), @elizaos/electrobun#typecheck. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… prober contract, biome drift (#12919) Repairs the accumulated red on develop so bun run verify passes again (modulo two pre-existing failures noted below): - @elizaos/core: export security/redact from the browser barrel — cloud-shared's logger (bundled into the app UI) imports isSensitiveKeyName/redactLogArgs from the root barrel (#12572 follow-up). - plugin-local-inference: fix two logger.warn(msg, {error}) calls to pino-style ({error}, msg) — introduced in #12546, broke typecheck of every package that type-includes the downloader source. - plugin-sql: carry the !agent.name narrowing across the withRetry closure in AgentStore.create (#12602 follow-up). - plugin-personal-assistant: adapt the website-blocking prober to the shared PermissionState contract (adds required platform field) instead of exposing plugin-blocker's narrower local shape. - plugin-tee: allowImportingTsExtensions (tsconfig is noEmit). - packages/ui: biome-ignore annotations for the two intentional registryVersion useMemo deps; type-collapse cast for the postcss/tailwind dual-major seam in the e2e fixture builder. - Repo-wide biome format normalization emitted by the self-fixing lint scripts (ui, agent, cloud-shared, cloud-ui and dependents). Still red on develop (pre-existing, untouched): @elizaos/app#typecheck (ui/navigation export resolution under tsgo), @elizaos/electrobun#typecheck. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
❌ PR title does not match the required pattern. Please use one of these formats:
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Closes #12269
Reference PR for the #12182 DB-adapter fallback-slop family. The flagship batch: a DB adapter that converts every failure into a healthy-looking default, making "database broken" indistinguishable from "no data". This lands the fix pattern the sibling sweeps copy.
What changed
Catch-return-default methods across the four adapters now throw a typed
ElizaError({ code, cause, context }) instead of fabricating0/[]/false/undefined. "No rows" and "DB broken" are now type-distinguishable at every public method.plugin-sql/src/base.ts(live adapter) — rewritten to throw:deleteAgents,updateAgent,countAgents(+ killedresult0?.count || 0),createEntities,ensureEntityExists,updateComponent(also removed a server-sideconsole.error),log,updateMemory,addParticipant,addParticipantsRoom,removeParticipant,createRelationship,getCache,setCache,deleteCache.createAgent/deleteAgent/cleanupAgents/setParticipantUserState/updateRelationshiprewritten toElizaError-with-causeor no-catch propagation.getCachedEmbeddingskeeps its input-length empty result as J3;isReadyannotated J4 (a health probe —falseis the designed answer).plugin-sql/src/stores/*.ts— same treatment for the parallel store modules (agent/cache/entity/participant/relationship/memory/log), including the secondresult[0]?.count || 0/?? 0count fallbacks and an embedding-query swallow inmemory.store.plugin-sql/src/pglite/manager.ts— the 9 empty teardown catches become J6logger.debug(or an annotated J3 for the error-text formatter); OS//procparse probes annotated J3.Entry points / services —
index.ts+index.browser.ts(removed an empty catch) capability probes annotated J4;advanced-memory-storage.tsoptional-service probe J4;schema-transformer.tstype-probe J3.plugin-localdb— J3 first-bootENOENT, J5 write-chain link suppression (the real rejection is observed by the flush caller).plugin-local-storage— J3ENOENTexistence probe.plugin-inmemorydb— already clean (no catches).Every kept handler carries a grep-able
// error-policy:J<N> <reason>comment.Verification
Real error-path test (no mocks — closes a live PGlite out from under the adapter so the query itself faults):
No existing test regressed — the affected real suites still pass end to end:
Error-policy ratchet — zero new empty-catch / server-console in touched files (net decreases):
Grep proof over the four adapter dirs (excluding tests/dist/build.ts): empty catch -> 0, server console.* -> 0, error-policy:J annotations -> 54.
bunx biome checkclean on all changed files.Note: full
bun run verify/ monorepo build is not run here —developfails to build for unrelated cloud-routing reasons — so verification is source-based (real-PGlite vitest + ratchet + biome + grep), per the batch guidance.Generated with Claude Code