Skip to content

fix(#12265): fallback-slop sweep (packages/agent) — media-store fast-fail + page-scoped-context reported degrades - #12766

Merged
lalalune merged 1 commit into
developfrom
fallback/12265-sweep
Jul 4, 2026
Merged

lalalune merged 1 commit into
developfrom
fallback/12265-sweep

Conversation

@lalalune

@lalalune lalalune commented Jul 4, 2026

Copy link
Copy Markdown
Member

Part of #12182 (fallback-slop sweep). Batch: packages/agent (#12265). Base: develop @ c0c2ddf374.

This is a precision slice of the ~444-suspect-site batch, not a completeness pass: it converts the two highest-confidence data-loss slop sites the issue calls out by name (media-store I/O) and upgrades the page-scoped-context provider's silent degrades to reported degrades, then annotates every justified handler it keeps. The high-risk boot path (runtime/eliza.ts) and the long ?? <lit> tail are intentionally deferred (sitesLeft).

Re-derived suspect counts (packages/agent, this tree)

pattern count
empty catch 4
promise swallow (.catch(()=>{})) 40
return-default catch 200
?? <lit> 760
|| <lit> 24

Site table — verdicts (2 files touched)

site pattern verdict
api/media-store.ts readStoredMediaBytes try/catch → return null CONVERT — absence→null kept; real read error → throw ElizaError("MEDIA_STORE_READ_FAILED", {cause}) (exemplar #2)
api/media-store.ts writeStoredMediaFile try/catch → return false CONVERT — traversal→false kept; real write error → throw ElizaError("MEDIA_STORE_WRITE_FAILED", {cause})
api/media-store.ts maybeEvict (×3) empty / log-warn KEEP J6 best-effort maintenance
api/media-store.ts gcUnreferencedMedia (×2) empty / log-warn KEEP J6 best-effort GC
api/media-store.ts listMediaFiles (×2) empty / log-warn KEEP J6 scan / J4 empty-list degrade
api/media-store.ts touchOnServe empty KEEP J6 LRU hint
api/media-store.ts persistDataUrl catch → null KEEP J3 untrusted data-URL sanitize
api/media-store.ts readBackgroundPins catch → [] KEEP J3 absent/corrupt ledger → no pins
api/media-store.ts pinBackgroundMedia log-warn KEEP J6 best-effort pin write
api/media-store.ts resolveMediaFile decode catch → 400 KEEP J3 malformed path → explicit 400
api/media-store.ts resolveMediaFile stat catch → 404 KEEP J4 missing asset → explicit 404
api/media-store.ts persistImageThumbnail / ensureThumbnailForStoredFile catch → null KEEP J4 optional thumbnail degrade
api/media-store.ts persistAttachmentUrlIfInline log-warn+return url KEEP J4 falls back to functional inline URL
api/media-store.ts deleteMediaFile catch → false LEFT (boolean-delete judgment call) — sitesLeft
providers/page-scoped-context.ts fetchLocalJson catch {} KEEP J4 port failover (exhaustion → caller renders explicit "unavailable"); empty catch removed
providers/page-scoped-context.ts renderBrowserLiveState catch → null CONVERT — add runtime.reportError (J4 degrade, now agent-visible)
providers/page-scoped-context.ts renderAutomationsLiveState catch → null CONVERT — add runtime.reportError (J4 degrade, now agent-visible)
providers/page-scoped-context.ts provider get log-only + return EMPTY_RESULT CONVERTlogger.errorruntime.reportError (J1 boundary)

Per-category tally

  • Converted (slop → fast-fail / reported): 5 — 2 media-store throws + 3 page-scoped reportError upgrades.
  • Annotated (justified, kept): 17 — J1 ×1, J3 ×3, J4 ×6, J6 ×8 (2 of the J4 also live on converted sites).
  • Left (sitesLeft): the rest of the batch's ~444 suspect + 760 ?? <lit> sites, incl. deleteMediaFile and the entire runtime/eliza.ts boot path (high-risk — deferred, not touched).

Exemplar before → after (readStoredMediaBytes)

// BEFORE — EACCES/EIO indistinguishable from "no such media"; a backup silently drops bytes
try {
  return fs.existsSync(filePath) ? fs.readFileSync(filePath) : null;
} catch {
  return null;
}

// AFTER — absence is null; a real read failure throws to the export boundary
if (!fs.existsSync(filePath)) return null;
try {
  return fs.readFileSync(filePath);
} catch (err) {
  // error-policy:J2 context-adding rethrow
  throw new ElizaError(`media read failed for ${fileName}`, {
    code: "MEDIA_STORE_READ_FAILED", cause: err, context: { fileName },
  });
}

Its test (media-store.test.ts, real fs — no mocks): a directory placed where the bytes should be makes readFileSync throw EISDIR; the test asserts an ElizaError with code MEDIA_STORE_READ_FAILED and a preserved cause, while absence still returns null. The write path is proven with a real read-only store dir (chmod 0o555, skipped as root) asserting MEDIA_STORE_WRITE_FAILED. The provider test drives pageScopedContextProvider.get with a throwing getTasks / getRoom and asserts runtime.reportError fires with the right scope while the provider still degrades (brief-only / empty context).

Verification

  • bun run --cwd packages/agent test src/api/media-store.test.ts src/providers/page-scoped-context.test.ts src/services/agent-export.media.test.ts src/services/agent-export.roundtrip.test.ts70 passed (63 pre-existing green + 7 new error-path).
  • bun run audit:error-policy-ratchetpasses: page-scoped-context.ts emptyCatch 1→0, media-store.ts 5→5 (all annotated J6), zero server-console added.
  • bun run --cwd packages/agent typecheck → my four files clean. Two pre-existing, unrelated failures remain in untouched packages (plugins/plugin-discord missing @elizaos/plugin-meetings; plugins/plugin-local-inference logger typing) — not introduced here.
  • Logger-only preserved; logger import dropped from page-scoped-context (top-level catch now uses runtime.reportError).

Evidence

Refs #12265

🤖 Generated with Claude Code

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c61dfe23-48d1-4cf9-89cd-f4116d37853b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fallback/12265-sweep

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lalalune

lalalune commented Jul 4, 2026

Copy link
Copy Markdown
Member Author

Local verification from my side:

  • bun run --cwd packages/agent test src/api/media-store.test.ts src/providers/page-scoped-context.test.ts src/services/agent-export.media.test.ts src/services/agent-export.roundtrip.test.ts passed: 4 files, 70 tests.
  • bun run audit:error-policy-ratchet passed.
  • bunx @biomejs/biome check packages/agent/src/api/media-store.test.ts packages/agent/src/api/media-store.ts packages/agent/src/providers/page-scoped-context.test.ts packages/agent/src/providers/page-scoped-context.ts exited 0, with one fixable info in page-scoped-context.ts for an unnecessary continue.

Not merging from this pass because GitHub checks are still queued/unstable and the new packages/agent/src/providers/page-scoped-context.test.ts starts with // comments instead of the required top-file /** ... */ prose header from the repo comment-cleanup contract.

@lalalune
lalalune force-pushed the fallback/12265-sweep branch from 4d77cfb to a9d5f9c Compare July 4, 2026 04:32

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@lalalune
lalalune force-pushed the fallback/12265-sweep branch from a9d5f9c to 4da82d3 Compare July 4, 2026 04:34

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@lalalune
lalalune force-pushed the fallback/12265-sweep branch from 4da82d3 to 63e7140 Compare July 4, 2026 04:40

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

…, annotate justified handlers

media-store: readStoredMediaBytes / writeStoredMediaFile no longer swallow a
real fs error into a fabricated "media not found" / "restored fewer files".
Absence still returns null/false; a genuine read/write failure (EACCES/EIO/
ENOSPC) throws a typed ElizaError (MEDIA_STORE_READ_FAILED / _WRITE_FAILED) so
it surfaces at the export/import boundary instead of silently corrupting a
backup. The file's remaining best-effort/parse/degrade handlers are annotated
J3/J4/J6.

page-scoped-context: the browser + automations live-state subsections and the
provider boundary now report their failures via runtime.reportError (feeding
RECENT_ERRORS / owner escalation) while still degrading gracefully; the
fetchLocalJson port-failover empty catch is annotated J4 (exhaustion already
renders an explicit "unavailable" line).

Real-fs error-path tests induce the failures without mocking the thing under
test; existing media-store + agent-export tests stay green.

Refs #12265

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lalalune
lalalune force-pushed the fallback/12265-sweep branch from 63e7140 to 6896667 Compare July 4, 2026 04:41

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@lalalune
lalalune merged commit 68d0114 into develop Jul 4, 2026
14 of 56 checks passed
@lalalune
lalalune deleted the fallback/12265-sweep branch July 4, 2026 04:41
lalalune pushed a commit that referenced this pull request Jul 4, 2026
…/fabricated-default/promise-swallow to fail-fast

Deep re-sweep of the remaining fallback-slop in packages/agent after the
headline files were handled by #12766 / #12921. Empty-catch count in this slice
is already 0; this pass targets the log-and-return-empty catches that make a
broken pipeline read as a legitimately-empty result, plus fail-closed swallows.

Converted (behavior-changing, fail-fast; each with a real error-path test):
- providers/recent-conversations.ts + relevant-conversations.ts: on a recall
  failure both providers returned the IDENTICAL empty context as a legit-empty
  recall (the banned "not loaded reads as empty" conflation). Now
  runtime.reportError surfaces the broken pipeline via RECENT_ERRORS while the
  provider still degrades to empty (annotated error-policy:J4). Removed the
  now-unused logger imports (logger-only rule still holds; reportError logs).
- security/access.ts hasPrivateAccess: a throw from the core private-access
  check was silently swallowed to `return false` — fail-closed is correct, but
  a broken role/world-resolution pipeline would deny forever with no signal.
  Now reports via runtime.reportError and stays fail-closed (error-policy:J4).

Annotated (evaluated, kept — not slop):
- api/views-registry.ts ×2 indexView `.catch(() => {})`: indexView self-degrades
  (its own catch logs and falls back to keyword search); the call-site catch only
  suppresses a stray pre-embed rejection so a background task cannot crash the
  loop (error-policy:J5).

Deferred (ambiguous absence-vs-failure, per this wave's scope): bare
`return null/[]/false` catches and `?? <lit>` defaults where legitimate-absence
cannot be cleanly distinguished from masking-a-failure.

Tests: 6 new fast-fail assertions across 3 files (recent-conversations,
relevant-conversations.faildast, access) — all green; each asserts reportError
fires on the real failure AND that a legit-absence path does NOT report.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants