Repository navigation
🤖 fix: report an unreadable config.json instead of null getInfo and corrupt-file errors - #5777
Conversation
…orrupt-file errors
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877a25e3ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d603446106
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Readiness record for head
Generated with |
Summary
When
config.jsoncannot be read or parsed, the backend now reports that cause.workspace.getInforejects with the load error instead of returningnull. An edit refused after an unreadable read says the file could not be read and no longer tells the user to move a corrupt file aside. Later edits, sends and stream resumes give the same cause instead of "Workspace not found". When the file is readable again, the nextgetInfoand edit succeed and the load error clears, with no restart.Fixes #5757
Background
A failed load returns the defaults view, which has no workspaces. So every lookup missed, and callers reported "not found" (or
null) for workspaces that are still registered on disk. The edit gate also had one message for every failed load, and that message was written for a corrupt file.Implementation
Configrecords two in-memory fields in the existing process-scoped load-failure map:unreadable(no bytes were read) anderrorMessage. Nothing new is persisted.Config.getConfigLoadError()returns a message for the last failed load, or null. It is a map lookup with no I/O. It reports a failure only when thisConfiginstance's own last load wrote it. The map is shared by path, and short-lived instances (for example inruntimeFactory) must not make a healthy instance report their failure.handleConfigLoadFailurestores the state object it wrote in a new private field,observedLoadFailure, andgetConfigLoadError()reports the map entry only when it is that same object. That check is one identity compare: still no fs call, stat orloadConfigOrDefault.readConfigOrDefaultand the snapshot clear/keep rules from 🤖 perf: keep the config snapshot while it still matches the file #5750 are unchanged, andsnapshot.test.tsis unchanged and passes. For a parse failure without a confirmed.corrupt-backup, the message does not point to a backup.assertConfigWritablerefuses an unreadable file with "config.json could not be read ()...". The corrupt-file text stays for parse failures.workspace.getInforoute handler calls the unchangedWorkspaceService.getInfo. When that returnsnullandgetConfigLoadError()is set, the handler throwsORPCError("SERVICE_UNAVAILABLE")with the load error. The route does this check itself, so the PR adds no newWorkspaceServicemethod.getInfokeeps returningnullfor internal callers.WorkspaceService, the 26Err("Workspace not found")returns becomeErr(this.workspaceNotFoundError()). The private helper returns the load error when one is set, else "Workspace not found". Each site is a one-line change. The two "Workspace not found. It may have been deleted." guards insendMessageandresumeStreamprefer the load error the same way.No change to the config snapshot,
loadConfigOrDefault,buildWorkspaceMetadata,getWorkspaceMetadataById,workspaceIndexorlatestUserText.Callers of
workspace.getInfoand what they do with a rejectionWorkspaceContext.getWorkspaceInfosrc/browsercalls it.chatCommands.forkWorkspace/forkcommand) wrap it in try/catch and show the message in the fork error popover or a toast.chatCommands.createNewWorkspace/newcommand calls it, inside try/catch, and shows a toast./newcommand (handleNewCommand)/plan opencommandacp/agent.ts,configOptions.ts,sessionFork.ts,sessionResume.ts)null. Now they throw the real cause. ACP/compactused?.on the result, so it now fails with the load error instead of running with default settings.No renderer change is needed: no caller crashes, and none shows "workspace removed".
Follow-up
workspace.listand thesubscribeMetadatasnapshot still answer an empty list after a failed load, so the sidebar shows no workspaces and no error. That is the startup flow and needs its own renderer handling, so it is not in this PR.assertConfigWritable) still reads the failure state shared by allConfiginstances, so another instance's failure can block edits after the file is readable again. This existed before this PR, and the fix changes the corrupt-file safety gate, so it gets its own review.Validation
chmod 000:workspaceService.configLoadError.test.tscalls the realworkspace.getInforoute throughcreateRouterClientandupdateTitleon a realWorkspaceServicewithconfig.jsonatchmod 000. On main,getInforeturnednulland both edits returned "Workspace not found". The test then restores the mode and checks thatgetInfo, the next edit andgetConfigLoadError()recover.sendMessageandresumeStreamreport the load error.config.test.ts: with a warm snapshot, an edit to an unreadable file was refused with "the existing corrupt config has no confirmed backup yet". A parse failure is reported as "could not be parsed", and without a confirmed backup the message does not mention one. A secondConfigwhose read fails does not change what the warm instance reports.src/node/config(includingsnapshot.test.ts),src/node/orpcand allworkspaceService*.test.tspass.Risks
Low. The changes only touch paths where the config load already failed. In the normal case,
getConfigLoadError()returns null and every message and return value stays the same.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$42.55