fix(config): degrade gracefully on a broken config file in PawWork runtime (#1485) - #1500
Conversation
…ntime (#1485) A malformed config file (bad JSONC or a schema violation) is raised as a defect by ConfigParse, which dies the whole instance config load. Because Config.get() runs on nearly every operation and the app polls it, the same failure resurfaced on every call — the reported error storm that also left the desktop app unusable after a non-technical user hand-edited pawwork.json to add an MCP server. Engine: in the PawWork runtime, catch config-parse defects per file, record each one, and fall back to an empty config for just that file so every other valid config source still loads. The plain opencode CLI keeps its fail-fast contract (a broken config still throws). Verified via probe that these throws are defects, not typed failures, so the existing global orElseSucceed never actually caught them — loadGlobal now degrades the same way. Recorded errors are exposed through a new read-only GET /config/errors route (SDK regenerated). Frontend: on bootstrap, after config loads, fetch the recorded errors and surface one readable toast per unseen problem (deduped by directory + file + message so a still-broken file does not re-toast on every reload), reusing the existing ConfigInvalidError formatter and adding a ConfigJsonError branch. Tests: PawWork keeps valid sibling config and records the error for both invalid JSON and an invalid mcp entry; the CLI still fails fast; the /config/errors route returns recorded errors; formatServerError renders ConfigJsonError by file; and surfaceConfigErrors emits once per distinct error and dedupes across bootstraps.
github-actions
Bot
added
app
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds resilient config loading in the opencode config service: invalid JSON or schema-invalid config files are skipped per-file with recorded ConfigLoadError entries instead of failing bootstrap. A new /config/errors API endpoint exposes these errors, and the app surfaces deduplicated toast notifications with localized messages for JSON parse failures. ChangesConfig load resilience with error surfacing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App as App Bootstrap
participant SDK as sdk.config
participant Server as ConfigHttpApi
participant ConfigService as Config.Service
participant Toast as showToast
App->>SDK: errors()
SDK->>Server: GET /config/errors
Server->>ConfigService: getErrors()
ConfigService-->>Server: ConfigLoadError[]
Server-->>SDK: 200 JSON error list
SDK-->>App: ConfigLoadError[]
App->>App: surfaceConfigErrors(directory, errors)
alt unseen error
App->>Toast: emit error toast
else already surfaced
App->>App: skip
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/context/global-sync/bootstrap.test.ts, packages/app/src/context/global-sync/bootstrap.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/utils/server-errors.test.ts, packages/app/src/utils/server-errors.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app/src/context/global-sync/bootstrap.ts (1)
63-93: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded module-level dedup cache.
seenConfigErrorsis a module-levelSetthat entries are only ever added to, never evicted — it lives for the entire app process lifetime. Every distinct(directory, path, message, issues)combination a user ever triggers (e.g. repeatedly editing/fixing/re-breaking a config file, each attempt producing a different parser message/line number) permanently occupies memory, even long after the underlying file is fixed and the directory is closed/removed. It's also awkward for future test isolation since the state is shared across all callers/tests importing this module.Consider scoping this by directory (e.g. clear/replace the per-directory entries when
bootstrapDirectoryruns and the directory's config successfully loads with no errors, or evict entries once a directory is closed) rather than a flat, ever-growing globalSet.♻️ Possible direction
-const seenConfigErrors = new Set<string>() +// Keyed by directory so entries can be cleared/replaced per bootstrap pass instead of growing forever. +const seenConfigErrorsByDirectory = new Map<string, Set<string>>() export function surfaceConfigErrors( directory: string, errors: ConfigLoadError[], translate: (key: string, vars?: Record<string, string | number>) => string, emit: (toast: { variant: "error"; title: string; description: string }) => void = showToast, ) { + const seen = seenConfigErrorsByDirectory.get(directory) ?? new Set<string>() + seenConfigErrorsByDirectory.set(directory, seen) for (const error of errors) { - const key = [directory, error.data?.path ?? "", error.data?.message ?? "", JSON.stringify(error.data?.issues ?? "")].join( + const key = [error.data?.path ?? "", error.data?.message ?? "", JSON.stringify(error.data?.issues ?? "")].join( " | ", ) - if (seenConfigErrors.has(key)) continue - seenConfigErrors.add(key) + if (seen.has(key)) continue + seen.add(key) emit({ ... }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/global-sync/bootstrap.ts` around lines 63 - 93, The module-level dedup cache in surfaceConfigErrors currently grows forever because seenConfigErrors only adds entries and never evicts them. Change the dedup strategy to be directory-scoped, or otherwise clear/remove entries when bootstrapDirectory succeeds for that directory or when the directory is closed, so stale config error keys do not accumulate across the app lifetime. Also make the cache easy to reset for tests by avoiding a single shared global Set that all callers import and mutate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/config/config.ts`:
- Around line 385-388: The `toConfigLoadError` helper is returning
`InvalidError.toObject()` by cast, which can leak extra `issues` fields beyond
the declared `{ message, path }` `ConfigLoadError` shape. Update
`toConfigLoadError` in `config.ts` to normalize `InvalidError` by mapping its
`issues` into only the allowed fields before returning, while keeping the
`JsonError.isInstance` and `InvalidError.isInstance` branches intact. Make sure
the `/config/errors` response matches the SDK contract and does not expose
upstream Zod issue details.
---
Nitpick comments:
In `@packages/app/src/context/global-sync/bootstrap.ts`:
- Around line 63-93: The module-level dedup cache in surfaceConfigErrors
currently grows forever because seenConfigErrors only adds entries and never
evicts them. Change the dedup strategy to be directory-scoped, or otherwise
clear/remove entries when bootstrapDirectory succeeds for that directory or when
the directory is closed, so stale config error keys do not accumulate across the
app lifetime. Also make the cache easy to reset for tests by avoiding a single
shared global Set that all callers import and mutate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5472924e-0c88-41b2-a898-01c52e0b5d1f
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (12)
packages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/bootstrap.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/utils/server-errors.test.tspackages/app/src/utils/server-errors.tspackages/opencode/src/config/config.tspackages/opencode/src/server/routes/instance/httpapi/groups/config.tspackages/opencode/src/server/routes/instance/httpapi/handlers/config.tspackages/opencode/test/config/pawwork-global-config.test.tspackages/opencode/test/server/config-routes.test.tspackages/sdk/openapi.json
…anaged config Address two findings from PR review: - toConfigLoadError kept the JSONC parser's raw debug message, which echoes the offending config text (potentially API keys/PATs from a half-edited file) both as a full dump and per-error context lines. That message is surfaced to the frontend via /config/errors and stored in the dedup key. Keep only the '<code> at line N, column M' summaries; drop echoed source. - The managed-config-dir and macOS managed-preferences branches loaded config bare, so a broken managed file still died the whole load and never reached /config/errors. Wrap both in keepValidConfig so managed deployments degrade like user config (non-PawWork CLI still fail-fasts via the isPawWork gate).
The sanitizer kept lines whose suffix matched ' at line N, column M', but the
parser echoes the offending source as 'Line N: <source>'. A config line that
itself ends with such a suffix (e.g. 'sk-secret at line 2, column 3') would be
mistaken for a summary and leak through /config/errors.
Anchor the filter to the whole line ('^<Code> at line N, column M$') and drop
'Line N:' echo lines explicitly, so no raw config line can masquerade as a
summary. Add a regression test for a source line ending in a fake position.
…ache
Address two CodeRabbit findings:
- toConfigLoadError cast InvalidError.toObject() straight to ConfigLoadError,
so zod issues carried fields beyond the declared { message, path } shape
(issue code, expected/received or raw input values, nested union errors).
Map issues to only { message, path } so no upstream zod detail — including
echoed config values — leaks via /config/errors, state, or the dedup key.
- surfaceConfigErrors used a module-level Set that only ever grew, holding a
key for every distinct config error for the app's lifetime. Scope it per
directory and replace each pass with the errors still present, so a fixed
config drops its entry and a regressed error is surfaced again.
1 participant
Summary
In the PawWork runtime, a config file that fails to load (invalid JSONC or a schema violation) is now skipped per file — its error is recorded and every other valid config source still loads — instead of dying the whole config load. The recorded errors are exposed via a new read-only
GET /config/errorsroute and surfaced once as a readable toast on bootstrap. The plain opencode CLI keeps its fail-fast contract.Why
Reported in #1485: a non-technical user hand-edited
pawwork.jsonto add an MCP server, got the schema wrong, and the desktop app spammed the same config error on every operation until it was unusable.Root cause:
ConfigParseraises parse errors with a plainthrow, which inside an Effect generator becomes a defect, not a typed failure. That defect dies the instance config load, and becauseConfig.get()runs on nearly every operation (and is polled), the same failure resurfaced on every call. A probe confirmed the existing globalorElseSucceednever caught these (it only recovers typed failures), so the "graceful" global path was silently ineffective too.Related Issue
Fixes the error-spam half of #1485. The in-app MCP management GUI (the other half) is a follow-up PR.
Human Review Status
Pending
Review Focus
Runtime.isPawWork()gate inkeepValidConfig(config.ts): desktop degrades, the opencode CLI still fails fast — the existing engine tests encode fail-fast as a contract, so degrading universally would break them and change CLI behavior. This diverges from an earlier suggestion to degrade globally; the divergence is deliberate because the global-path premise was disproven.catchDefect(notcatchAll) is required because the parse errors are defects; non-config defects are re-raised unchanged.Risk Notes
packages/sdk/openapi.jsonandpackages/sdk/js/src/v2/gen/*are regenerated from the new route viabun dev generate+ the SDK build. Expected, in scope.dev:desktopscreenshot walk — the toast reuses the existing toast component and copy is unit-covered. Flagging so a reviewer can request a screenshot if desired.How To Verify
Screenshots or Recordings
Not captured — see Risk Notes (toast copy is unit-covered; no live screenshot walk done yet).
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests