fix(acp): recognise structured ENOENT codes and broader not-found phrasings#26439
fix(acp): recognise structured ENOENT codes and broader not-found phrasings#26439frozename wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
…asings AcpFileSystemService.normalizeFileSystemError classified read failures as file-not-found by checking the message against four exact substrings: 'Resource not found', 'ENOENT', 'does not exist', 'No such file'. JSON-RPC error responses also carry a structured 'code' field, but it was ignored — and common phrasings like 'not_found' (snake_case) or 'file not found' (no capitalisation) didn't match any substring. The downstream effect is severe: the write-file tool calls readTextFile first to check existence; a non-ENOENT-shaped error short-circuits the write attempt with 'unreadable file' rather than 'file is new'. The agent reasoning loop then commonly hallucinates 'the tool succeeded' and reports success in the final stop_reason: end_turn response — a silent failure pattern that's hard to detect from outside the CLI. This patch: - prefers the structured `err.code === 'ENOENT'` signal when present; - relaxes the legacy substring matcher to also accept 'not_found' and 'file not found' variants. Discovered while investigating gemini-cli ACP write reliability against a local stdio JSON-RPC server (penumbra/agentchat). Server emits 'fs/read_text_file not_found: file not found: <path>' for missing files — neither the original four substrings catch it, so every gemini-cli write silently wrote nothing. After this patch a single-file write smoke against the same server goes from 9m19s with 3 server restarts (recovered by chance) to 36s clean. Test added: 3 new it-block paths covering snake_case 'not_found', the 'file not found' phrasing, and a structured `code: 'ENOENT'` ACP error.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of file system error normalization within the ACP service. By introducing support for structured error codes and broadening the range of recognized error message phrasings, it prevents silent failures where file-not-found conditions were previously misclassified, ensuring that downstream tools correctly identify missing files. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances error normalization in AcpFileSystemService by introducing support for structured error objects containing an ENOENT code and expanding substring matching to include 'not_found' and 'file not found' patterns. Unit tests were added to cover these new cases. Feedback suggests refining the structured error handling to correctly extract the error message, ensuring detailed errors are logged for debugging, and utilizing acp.RequestError with specific JSON-RPC codes to align with repository standards.
| if (err && typeof err === 'object' && 'code' in err) { | ||
| const code = (err as { code?: unknown }).code; | ||
| if (code === 'ENOENT') { | ||
| const newErr = new Error(errorMessage) as NodeJS.ErrnoException; | ||
| newErr.code = 'ENOENT'; | ||
| throw newErr; | ||
| } | ||
| } |
There was a problem hiding this comment.
If err is a plain object, errorMessage will be initialized as '[object Object]' at line 37. When a structured error is detected, we should extract the message property. Additionally, per repository rules, we should log the detailed error for debugging and throw acp.RequestError with an appropriate JSON-RPC error code instead of a generic Error.
| if (err && typeof err === 'object' && 'code' in err) { | |
| const code = (err as { code?: unknown }).code; | |
| if (code === 'ENOENT') { | |
| const newErr = new Error(errorMessage) as NodeJS.ErrnoException; | |
| newErr.code = 'ENOENT'; | |
| throw newErr; | |
| } | |
| } | |
| if (err && typeof err === 'object' && 'code' in err) { | |
| const errObj = err as { code?: unknown; message?: unknown }; | |
| console.error('Detailed error:', err); | |
| if (errObj.code === 'ENOENT') { | |
| const msg = typeof errObj.message === 'string' ? errObj.message : errorMessage; | |
| throw new acp.RequestError(-32602, msg); | |
| } | |
| } |
References
- When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
- When handling JSON-RPC requests, throw acp.RequestError with an appropriate JSON-RPC error code instead of a generic Error for invalid parameters.
…ject
JSON-RPC clients commonly surface error responses as plain objects
(`{ code, message, data }`) rather than Error instances. The previous
`String(err)` fallback collapsed these to '[object Object]', losing the
diagnostic. Resolve a useful message by reading `err.message` when the
rejection is a plain object that has it, before falling back to String().
Adds a test for the non-Error structured-error case.
|
@gemini-code-assist Thanks — pushed Applied: the message-extraction fix. Pushed back on: swapping the throw to
I left out the |
|
Thank you for the detailed explanation and for addressing the message-extraction issue. Your reasoning regarding the contract of |
gemini-cli
Bot
added
the
status/need-issue
gemini-cli
Bot
removed
the
status/need-issue
1 participant
Problem
AcpFileSystemService.normalizeFileSystemErrorclassifies a read failure asfile-not-found by string-matching the message against four exact substrings:
JSON-RPC error responses also carry a structured
codefield, but it wasignored. Common phrasings that don't match any of those four substrings —
notably
not_found(snake_case) andfile not found(no capitalisation, no"No such") — fall through to the generic
throw errbranch.The downstream effect is severe. The
write-filetool callsreadTextFilefirst to check existence, and a non-ENOENT-shaped error short-circuits the
call site with
error: { code: FILE_WRITE_FAILURE }rather thanfileExists: false(seepackages/core/src/tools/write-file.ts).The agent's reasoning loop then commonly hallucinates "the tool succeeded"
and reports success in the final
stop_reason: end_turnresponse — asilent failure pattern that's hard to detect from outside the CLI.
Fix
This PR makes
normalizeFileSystemError:code: 'ENOENT',recognise it as ENOENT regardless of message wording. Cheapest signal,
matches what well-behaved JSON-RPC servers should emit.
not_foundandfile not foundphrasings. These are common in JSON-RPC servers in thewild and appear in the agent-client-protocol ecosystem in particular.
The existing four substrings are preserved for backward compatibility.
Tests added
In
packages/cli/src/acp/acpFileSystemService.test.ts:it.eachover the two new message variants (not_foundsnake_case andfile not foundlowercase).Object.assign(new Error('opaque server message'), { code: 'ENOENT' })and the matcher recognises it.npm run test:ci -- src/acp/acpFileSystemService→ 13 passed (3 new + 10 existing).How this surfaced
Discovered while running gemini-cli in ACP mode against a third-party stdio
JSON-RPC server. The server raised
fs/read_text_file not_found: file not found: <path>for a non-existentfile. None of the four current substrings matched, so every gemini-cli
write to that worktree silently produced no file changes. After this patch
a single-file write smoke against the same server goes from ~9 minutes
with three server restarts (recovered by chance) to ~36 seconds clean.
Compatibility
not_found,file not found) and the structuredcode === 'ENOENT'path are strictly additive — they only converterrors that previously fell through to
throw errinto ENOENT-codederrors, never the reverse. No risk of regression for existing servers
that already produce one of the four recognised messages.
Fixes #26448.