fix: preserve opencode auto-capture errors - #258
Conversation
|
Remote validation for exact commit |
|
Fresh validation after updating this branch to cumulative latest |
|
Happy to take a look at the PR, we're (at least I am) honestly not using the plugin any more so it'll be awkward to maintain it, feel free to take control after the 2 PRs |
There was a problem hiding this comment.
Review: preserve opencode auto-capture errors
Call: SHIP. Change is correct, minimal, tested, fail-closed. Nothing blocks merge.
What the change does
Six lines in generateSummary (src/services/auto-capture.ts). When the opencode structured-output path throws and no manual external-API fallback is configured, the real provider error is now rethrown and surfaced to the user instead of being masked by the generic "External API not configured for auto-capture" toast. When memoryModel + memoryApiUrl are present, the manual fallback runs exactly as before — the provider error stays in the logs.
Why it is safe
- Scoped tight. The rethrow only fires in the no-fallback branch. The manual path is unchanged.
- Failure direction is closed. Error now surfaces instead of being swallowed. The only remaining swallow is the intended one: opencode fails → manual fallback runs → manual error reported, first error in logs. That is a fallback chain, not a mask.
- Error type is safe. Every throw site in
generateStructuredOutputproducesError(verified:new Error(...), zodZodError, fetchTypeError). Even if a non-Error slipped through, all three catch points in the caller useinstanceof Error ? .message : String(e)— no unhandled rejection, toast always renders.
Test is real, not decorative
runProviderFailureScenario() spawns a real bun subprocess and asserts failedAttempts=1, released=true, exactly one toast, message containing the provider root cause, and not the old mask. Each assertion eliminates a distinct wrong path — early returns, silent swallows, claim failure all break one of them. Genuine regression test, not an inert mock.
Not blockers
user-memory-learning.ts:697(analyzeUserProfile) carries the same error-mask in the opencode path — same bug class, different service. It deserves the same fix on its own PR; this ones contract is auto-capture. Recommend filing a follow-up so it does not rot.- Stale log at
src/services/auto-capture.ts:578("falling back to external API"when no fallback exists) — pre-existing, slightly more visible now. Nit, fix on next touch. - Body claims
408/408; current main is411/411. Stale count, CI green, immaterial.
Scope note
This review is static analysis only — code read, test logic traced, error paths checked. Actual runtime behavior (subprocess spawn, toast rendering, provider interaction) relies on the PR author owning the change; I did not run the full suite against a live provider.
Merge. File the user-memory-learning twin as a follow-up.
|
Thanks for the detailed review and the maintenance handoff. I'm happy to take a bounded lane covering Turso/vector-search reliability, auto-capture/profile-learning reliability, issue triage, and CI/release readiness. I'll continue requiring independent review for my own changes. I filed #265 for the |
Summary
memoryModelandmemoryApiUrlare availableWhy
Issue #253 shows the underlying provider error (
Thinking mode does not support this tool_choice) followed by a misleading final toast (External API not configured for auto-capture). The OpenCode path currently catches every provider error, then overwrites it when the optional manual fallback is absent.This patch does not alter OpenCode/provider wire payloads or hard-code a model list. Forced
tool_choicecompatibility belongs to the upstream provider layer; this repository should preserve that actionable error instead of masking it.Refs #253.
Verification
bun test tests/auto-capture.test.ts(2/2)bun test(408/408)bun run typecheckbun run format:checkbun run buildExternal API not configured for auto-captureand makes the new regression fail