fix(#12274): deep fallback-slop sweep slice 0/1 — fail-fast over fabricated defaults

[lalalune]

Deep fallback-slop sweep — app plugins B, slice 0/1 (#12274)

Residual deep sweep over the app-plugins-B slice (plugin-{wallet,vision,music,agent-skills,browser,workflow,training}) after the primary #12274 sweep already merged as #12825. This pass re-derived the full suspect list, converted the clear slop that survived, kept + annotated genuine J-categories, and deferred the ambiguous tail for a per-site judgment pass.

Scope per the parent convention (#12182 / foundation #12263): the unambiguous slop categories only — empty catch, fabricated-healthy-on-error, promise-swallow. Ambiguous return null/[]/false and ?? <lit> defaults are explicitly deferred (they need per-site judgment; over-removing a legitimate not-found→empty is a bug).

Per-category tally

Category	Converted	Notes
Fabricated-healthy (return {} on read failure)	4 sites	agent-skills preference/acknowledgment loaders — DB read failure masked as "no data" (data-loss bug, see below). Fixed in the exported helpers and the duplicated local copies in skills-routes.ts.
Empty catch (catch {})	1 site	browser-service target-resolution failover scan → kept as designed failover, annotated // error-policy:J4, now logs the exclusion.
Promise-swallow	0	Surviving .catch(()=>{}) in the slice are J6 best-effort temp-file teardown / J3 parse-invalid — left for the annotation pass, not masking-a-failure.
Annotated justified (J1–J7)	1	// error-policy:J4 on the browser failover scan.

The data-loss fix (fabricated-healthy)

loadSkillPreferences / loadSkillAcknowledgments returned {} when the cache read threw, conflating "DB read failed" with "nothing persisted yet". The callers read-modify-write these maps and save them back:

const prefs = await loadSkillPreferences(state.runtime); // threw → returned {}
prefs[skillId] = true;
await saveSkillPreferences(state.runtime, prefs);         // overwrites ALL other prefs

So a transient cache-read error silently wiped every other skill's saved preference. Now the read failure propagates to the route boundary (which surfaces it), while a genuinely-empty cache still returns {} via the existing ?? {}. This is the parent's canonical "not loaded must never read as empty" rule.

The empty-catch fix (browser-service)

The target-resolution scan swallowed an unhealthy candidate's available() error with a bare catch {}. This is legitimate failover — the other registered targets must still be selectable (unlike the pinned-preferredId path, which correctly throws BROWSER_TARGET_UNAVAILABLE). Kept the failover, annotated // error-policy:J4, and now logger.debugs the exclusion so a systemically broken target is observable instead of silently vanishing.

Ratchet (diff-scoped empty-catch, touched files)

plugin-agent-skills/src/api/skill-discovery-helpers.ts: emptyCatch 2->2
plugin-agent-skills/src/api/skills-routes.ts:            emptyCatch 3->3
plugin-browser/src/browser-service.ts:                  emptyCatch 1->0   ← down
no new fallback-slop in touched files

Empty-catch count goes down (browser-service 1→0) or stays equal — never up. Remaining empty catches in the agent-skills files are pre-existing comment-only J3 parse sites left for the annotation pass.

Tests

• New plugin-agent-skills/src/api/skill-discovery-helpers.test.ts — asserts a cache read failure now throws (would have resolved {} against the pre-change code), and that a genuinely-empty/absent cache still returns {}. 7 cases.
• plugin-browser/src/__tests__/browser-service.test.ts — new failover-scan case: a target whose available() throws is excluded, a healthy target still resolves, and the exclusion is logged.

plugin-agent-skills  test: 9 files, 42 passed
plugin-browser       test: 27 files, 148 passed

Deferred (ambiguousLeft — not touched this pass)

The slice's large residual tail is genuine J-categories and judgment-heavy sites: URL/JSON/base58 parse-invalid → null/false/[] (J3), executable/model-file existence probes (accessSync/fs.access → false), provider health/availability degrades (J4), best-effort temp-file unlink().catch(()=>{}) teardown (J6), and ?? <lit> load defaults. Notably the PaddleOCR describe() → {blocks:[]}-on-error path is a coordinated J7 vision-loop / OCR-service-family decision (no runtime handle at the leaf; all sibling OCR services share the shape) — left for a dedicated pass rather than a risky partial conversion. These need per-site judgment and are the next wave, not clear slop.

Evidence

• Backend behavior proven by the new unit tests driving the real helpers (real getCache throw → real propagation; no mock-swallow of the thing under test). Real-LLM trajectory / rendered-UI evidence: N/A — this is a server-side error-handling refactor with no new user-facing surface; the observable behavior is "a failed cache read becomes a 500 instead of a silent preference wipe", asserted by the tests.

Refs #12274

🤖 Generated with Claude Code

[greptile-apps]

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

[coderabbitai]

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: 50f238d1-ef59-47bb-afb0-798df12a5ca4

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/12274-deep-s0

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[lalalune]

Reviewed (adversarial): #12274 slop sweep: skill preference/ack loaders rethrow read failures (fixes a real read-modify-write data-loss bug where a transient getCache error would overwrite all prefs) while still empty-for-genuinely-empty. All throws contained (HTTP 500 wrap + boot try/catch). Tested. No failing CI. Queuing auto-merge on green.