fix(#12265): deep fallback-slop sweep slice 0/1 — providers + access fail-fast - #13271
Conversation
|
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 |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…/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>
c6fd8a8 to
3513018
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Validated rebased head 3513018 locally. I also fixed the new access test so it drives the real core role path instead of using a Checks run:
|
|
Reviewed (adversarial): #12265 slop sweep: recent/relevant-conversations providers + security/access hasPrivateAccess now reportError on failure while preserving degrade (empty context / fail-closed false). reportError is never-throwing per core types. Tested. No failing CI. Queuing auto-merge on green. |
#13336) Two of the five wave-2 fallback-slop-sweep PRs left real reds on develop: - #13278: database-rows-compat-routes.test.ts failed 5 OWNER-gate tests in the full app-core suite. app-core runs vitest with isolate:false, so a preceding suite's cached ./auth/sessions mock leaked into the real ensureRouteMinRole. Fix: vi.resetModules() in the hoisted block (the same immunization ensure-route-min-role.test.ts already uses). - #13277: useWhatsAppPairing.test.tsx had a TS2556 — the zero-arg onWsEvent vi.fn spread unknown[]. Fix: type the mock with a rest-param signature. Findings #13271, #13270, #13287 were already green on current develop and are left unchanged. No fail-fast/reportError conversion reverted. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…gagement) (#13335) * test(#12775): author SR slice of persona pack E1 (low-activation-reengagement) Completes the scenario-runner slice of LifeOps persona pack E1 — low-activation-reengagement (persona tara_low / P6, #12775), mirroring the A1 (#13286) and B1 (#13312) pack conventions. Coverage gate: E1 2/28 -> 28/28 authored (22 verified). Authored 10 new SR scenarios on top of the 2 pre-existing E1 scenarios: pr-deterministic (keyless, real lifeops_scheduler tick, no LLM): - lowact-quiet-streak-softens-next-nudge — three check-ins are fired + terminally expired through REAL ticks so the production state-log lays down a checkin/expired streak; the next reminder times out and the quiet-streak softener (#12779/#13237) steps intensity normal->minimal, read back off the persisted task (noReplyState.quietStreakSoftened=true, quietStreakDays>=3, appliedReminderIntensity=minimal, emptied ladder). - lowact-morning-single-priority-fires-once — one gentle high-priority morning pick fires once inside quiet hours while a low-value whole-list ping is HELD (quiet_hours gate); single-delivery finalCheck. - lowact-values-anchored-activity-fires-in-window — a during_window values-anchored evening activity fires inside the seeded eveningWindow and defers outside it. - lowact-micro-step-deferred-not-dropped — a captured one-small-step is parked (snooze override), does not resurface early, and resurfaces exactly once (scheduled_override_due) at the promised gentle time. Added to EXPECTED_PR_DETERMINISTIC_SCENARIO_IDS in the same commit (G1). live-only (status: authored, live-verify deferred to #12781): - lowact-lapse-return-triage-no-guilt (definitionCountDelta + shame-free judge) - lowact-quiet-user-reengagement-tone - lowact-crisis-language-safe-reengagement - lowact-make-whole-list-smaller-bulk-shrink - lowact-celebration-without-infantilizing - lowact-cannot-choose-single-option (definitionCountDelta{delta:1}) Each has effect-reading finalChecks, non-echo-satisfiable, personas-as-data in turns[].text (never promptInstructions). Crisis-boundary handling: #12780 crisis guard is CLOSED/NOT_PLANNED, so no scenario asserts a crisis-guard/988 effect. lowact-crisis-language-safe- reengagement takes the #12280 crisis-adjacent premise and asserts only the SAFE behavior per the A1 adhd-task-initiation convention: warm non-clinical stand-down, NO productivity push (definitionCountDelta{delta:0} proves no task/schedule created against her "not right now"), and does NOT assert a 988/crisis-guard side-effect fires. Catalog: 10 new surface:scenario-runner rows + 16 registered surface:lifeops-bench Python ids (append-only; no lifeops-bench/** file touched). 12 SR + 16 bench = 28 = target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: repair wave-2 fallback-sweep regressions (test mocks + typecheck) (#13336) Two of the five wave-2 fallback-slop-sweep PRs left real reds on develop: - #13278: database-rows-compat-routes.test.ts failed 5 OWNER-gate tests in the full app-core suite. app-core runs vitest with isolate:false, so a preceding suite's cached ./auth/sessions mock leaked into the real ensureRouteMinRole. Fix: vi.resetModules() in the hoisted block (the same immunization ensure-route-min-role.test.ts already uses). - #13277: useWhatsAppPairing.test.tsx had a TS2556 — the zero-arg onWsEvent vi.fn spread unknown[]. Fix: type the mock with a rest-param signature. Findings #13271, #13270, #13287 were already green on current develop and are left unchanged. No fail-fast/reportError conversion reverted. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: exhaustive-lane matrix proof + vacuous-green guard, GitHub-native turbo cache foundation, Linux Bun cache (advances #12342/#12341/#12338) (#13332) * ci(#12342): add exhaustive-lane matrix proof + vacuous-green guard Additive proof infrastructure for the exhaustive develop lane. Nothing here touches a PR-critical workflow or renames a required status check. - packages/scripts/ci-lane-manifest.json: committed source of truth enumerating every expected exhaustive lane plus plan floors. - packages/scripts/ci-full-matrix-proof.mjs: cross-checks the manifest against test.yml (job present, not pinned pull_request-only) and against `run-all-tests.mjs --plan=json` (task/package floors, required core packages, non-empty per-script lanes). Fails on a missing lane, a lane pointed at a nonexistent glob, or a whole script lane collapsing to zero. Emits a GitHub step summary enumerating every lane. - run-all-tests.mjs: `--min-tasks`/`MIN_TEST_TASKS` vacuous-green guard. A filter/shard/glob that collapses a lane to (near-)zero tasks, or a run whose every task skips (no test files), now exits 3 instead of green. Strictly additive: default 0 preserves historical behaviour. - .github/workflows/ci-full-matrix-proof.yml: un-cancellable (cancel-in-progress: false) scheduled proof job, twice daily + workflow_dispatch, plus a path-scoped PR trigger on its own inputs only. - Tests: negative cases for the proof (missing lane, PR-only pin, each plan-floor breach) and the guard (collapsed filter, env parity, usage error, historical exit preserved), all deterministic and dependency-free. Advances #12342 (mechanism only). The DoD's "≥ twice daily for 7 consecutive days without cancellation" is observation-gated and requires post-merge live-CI sampling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(#12341): add pinned GitHub-native turbo cache shim + migration contract Lays the additive foundation for the move off the Vercel remote Turbo cache without doing the risky removal across ~13 workflows in this change. - .github/actions/turbo-cache-github/action.yml: pinned composite action that restores/saves the local .turbo cache via actions/cache (SHA-pinned) keyed on the deterministic turbo-cache-key.mjs hash. No SaaS, no secrets. Nothing adopts it yet — migration of individual workflows is proven safe one at a time under #12341. - packages/scripts/ci-turbo-cache-contract.mjs: static contract. (1) the shim exists, is composite, keys off turbo-cache-key, pins actions/cache to a full SHA, and carries no SaaS env; (2) no workflow that ADOPTS the shim also wires TURBO_TOKEN/TURBO_TEAM/TURBO_CACHE: remote:rw — re-adding SaaS env to a migrated workflow fails the contract. Deliberately silent about not-yet-migrated workflows; the existing dedup contract still pins the SaaS wiring that remains live (nightly/release). - Wired the contract into test.yml's `changes` job (one additive step beside the existing dedup contract) and the new proof workflow. - Negative tests: clean adopter passes; SaaS re-adder fails; floating (unpinned) actions/cache fails; SaaS-in-shim fails. Advances #12341 (mechanism + contract). Removing SaaS env from live workflows and moving PR lanes to `turbo --affected` is deferred — it edits PR-critical workflows and its "cache hit rate >= baseline" DoD is observation-gated on representative post-merge PR runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(#12338): cache the Bun install store on Linux too, pin the cache action The shared setup action skipped the Bun install-store cache on Linux (`if: runner.os != 'Linux'`), so every Linux job re-downloaded the whole dependency tree cold. #12338 explicitly asks to enable Linux Bun store caching in the shared setup action; this does exactly that and pins the cache action by SHA (it was floating `@v5`) to kill the drift the issue targets. Additive and low-risk: a cache miss is just no speedup and a cache write failure is non-fatal, so a cold Linux runner behaves exactly as before while warm runners skip the re-download. The cache key already includes this action's own content, so entries rotate correctly on this edit. Advances #12338 (this DoD item: "Linux jobs restore the Bun store cache"). The "cache logs show Linux Bun store restore" verification is observation-gated on a real post-merge Linux run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(#12342): capture the plan via a file, not a pipe; add spawn-test timeouts The matrix-proof's default plan discovery spawned run-all-tests and read its stdout through a pipe. The plan JSON is >64KB and the runner calls process.exit(0) immediately after writing it, so a piped stdout gets truncated mid-flush (SyntaxError: Unterminated JSON at byte 65536). Redirect the runner's stdout to a temp file instead — a file descriptor is flushed on close, so capture is lossless. This mirrors how the CI workflow already invokes it (`> plan.json`, which was never affected). Also give the runner-spawning tests explicit 60s timeouts. They do whole-repo workspace discovery and were tripping bun's default per-test timeout on a cold/contended runner, producing a flaky red. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: cover remote deep-link profile registry sync * style: format CI lane manifest --------- Co-authored-by: Shaw <shawgotbags@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test: verify A1 task initiation live proof --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Shaw <shawgotbags@gmail.com>
What
Deep re-sweep of the remaining fallback-slop in
packages/agent(issue #12265, slice 0/1), after the headline files (media-store.ts,page-scoped-context.ts, boot empty catches) were handled by the already-merged #12766 / #12921. Empty-catch count in this slice is already 0, so this pass targets the residual log-and-return-empty catches that make a broken pipeline read as a legitimately-empty result, plus a fail-closed security swallow. Uses the #12263 foundation (runtime.reportError→ERROR_REPORTED→RECENT_ERRORSprovider).Per-category tally
providers/recent-conversations.ts,providers/relevant-conversations.ts,security/access.tsapi/views-registry.ts×2 (J5),providers/relevant-conversations.tsinner room-lookup (J4)return null/[]/falsecatches +?? <lit>where absence-vs-failure can't be cleanly told apartConverted (behavior-changing, fail-fast)
providers/recent-conversations.ts+relevant-conversations.ts— on a recall failure both providers returned the identical empty context ({ text: "", values: {}, data: {} }) as a legit-empty recall — the banned "not loaded reads as empty" conflation. Nowruntime.reportError(...)surfaces the broken pipeline to the agent (RECENT_ERRORS / owner escalation) while the provider still degrades to empty rather than aborting the turn (// error-policy:J4). Removed the now-unusedloggerimports (reportErrorlogs).security/access.tshasPrivateAccess— a throw from the core private-access check was silently swallowed toreturn false. Fail-closed is correct (a missing world returnsnullupstream, not a throw — so a throw here is a broken role/world-resolution pipeline), but a silently-denying broken check would deny forever with no signal. Now reports viaruntime.reportErrorand stays fail-closed (// error-policy:J4).Kept + annotated (not slop)
api/views-registry.ts×2 —void viewSearchIndex.indexView(entry, runtime).catch(() => {}).indexViewself-degrades (its own catch logs at debug and falls back to keyword search), so the call-site catch only suppresses a stray rejection from a synchronous pre-embed throw so a backgroundsetImmediatetask cannot crash the loop →// error-policy:J5. Converting toreportErrorhere would double-report a designed degrade — deliberately not converted.Deferred (this wave = clear-slop only)
Bare
return null/[]/falsecatches (e.g.network-policy.tsIPv6 parse → J3,update-checker.tsnet-fail → null,whichSync→ null) and?? <lit>/|| <lit>defaults where legitimate-absence cannot be cleanly distinguished from masking-a-failure. These need per-site judgment.Ratchet —
bun run audit:error-policy-ratchetemptyCatch in touched files: 2 → 2 (equal, never up; the views-registry
.catch(() => {})pair is now J5-annotated).Tests — all green
6 new fast-fail assertions across 3 files, each asserting
reportErrorfires on the real induced failure (stubbed collaborator throws a real error) AND that a legit-absence path does not report:providers/recent-conversations.test.ts(new) — 2 testsproviders/relevant-conversations.faildast.test.ts(new) — 2 testssecurity/access.test.ts(new) — 2 testsTypecheck: 0 errors in the 4 touched source files (remaining
packages/agenttypecheck errors are pre-existing trimmed-tree module-resolution noise:@elizaos/auth,drizzle-orm, etc. — unrelated).Evidence
runtime.reportError", proven by the 6 fail-fast unit tests that drive the real provider/access code with a throwing collaborator (no mock stands in for the code under test).Refs #12265 · parent #12182 · foundation #12263.
🤖 Generated with Claude Code