[codex] Diagnose desktop client settings read failures#3432
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository 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)
Comment |
ApprovabilityVerdict: Approved This PR adds diagnostic warning logs when desktop client settings fail to read due to unexpected errors (not missing file). The actual behavior is unchanged - errors still result in returning None - but now unexpected failures are logged with the settings path for debugging purposes. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
f596e87 to
46386c2
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
46386c2 to
295cff2
Compare
Desktop client settings intentionally fall back to
Nonewhen the settings file is missing or unusable, but the loader previously implemented that behavior withEffect.optionandEffect.orElseSucceed. Those broad fallbacks also hid permission failures, other filesystem failures, and malformed settings documents without leaving any diagnostic evidence.This change keeps the existing nonfatal API while making unexpected failures observable. A normalized
PlatformErrorwhose reason isNotFoundremains an expected silent absence. Other filesystem failures produce a warning annotated withsettingsPathand retain the original platform error in the log arguments. Schema failures likewise produce a path-annotated warning with the originalSchemaErrorbefore returningNone. Both recovery points use tagged catch semantics.The diagnostics tests use an isolated test file because the established client-settings test is currently touched by feature work. They verify that missing files remain silent, permission failures remain nonfatal but observable, and malformed documents remain nonfatal but observable.
Validation:
vp test apps/desktop/src/settings/DesktopClientSettings.diagnostics.test.tsvp check(passes with pre-existing repository warnings only)vp run typecheckNote
Low Risk
Behavior for callers (
getstill returnsOption) is unchanged except that some previously hidden failures may now surface as Effect failures or logs; no auth or data-mutation paths.Overview
Desktop client settings still return
Nonewhen the file is absent or unusable, but unexpected problems are no longer swallowed silently.readClientSettingsreplaces broadEffect.option/orElseSucceedfallbacks with tagged handling:NotFoundstays a quietNone; otherPlatformErrorcases emit a warning (withsettingsPath) and returnNone;SchemaErroron decode gets the same treatment. Errors outside those tags are no longer converted toNone.Adds
DesktopClientSettings.diagnostics.test.tsto assert missing file → no logs, permission/read failure → warning + path, malformed JSON → decode warning + path.Reviewed by Cursor Bugbot for commit 295cff2. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Diagnose desktop client settings read failures with targeted warning logs
Nonesilently (no log) inreadClientSettings.settingsPathinstead of silently returningNone.settingsPathinstead of silently returningNone.PlatformError/SchemaErrornow propagate as failures rather than being swallowed.Macroscope summarized 295cff2.