Repository navigation
fix(acp): reject fractional readTextFile limits #6704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -320,7 +320,8 @@ function isTrustedArtifactToolUpdate( | |||||||||||||||||||||
| */ | ||||||||||||||||||||||
| function preserveFsErrorOverAcp(err: unknown): never { | ||||||||||||||||||||||
| if (isFsErrorShape(err)) { | ||||||||||||||||||||||
| throw new RequestError(-32603, err.message, { | ||||||||||||||||||||||
| const code = err.kind === 'parse_error' ? -32602 : -32603; | ||||||||||||||||||||||
| throw new RequestError(code, err.message, { | ||||||||||||||||||||||
| errorKind: err.kind, | ||||||||||||||||||||||
| ...(err.hint !== undefined ? { hint: err.hint } : {}), | ||||||||||||||||||||||
| ...(err.status !== undefined ? { status: err.status } : {}), | ||||||||||||||||||||||
|
|
@@ -1879,6 +1880,26 @@ export class BridgeClient implements Client { | |||||||||||||||||||||
| if (typeof params.limit === 'number' && params.limit <= 0) { | ||||||||||||||||||||||
| return { content: '' }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if ( | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The guard condition
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||
| typeof params.limit === 'number' && | ||||||||||||||||||||||
| params.limit > 0 && | ||||||||||||||||||||||
| !Number.isSafeInteger(params.limit) | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The new inline guard correctly throws Consider having — qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, addressed. I updated preserveFsErrorOverAcp() so FsError('parse_error') maps to JSON-RPC Invalid Params (-32602) while preserving the existing data.errorKind, data.status, and data.hint fields. Other filesystem error kinds still use the existing internal-error mapping. |
||||||||||||||||||||||
| throw RequestError.invalidParams( | ||||||||||||||||||||||
| undefined, | ||||||||||||||||||||||
| `\`limit\` must be a positive integer, got ${params.limit}`, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if ( | ||||||||||||||||||||||
| typeof params.line === 'number' && | ||||||||||||||||||||||
| params.line > 0 && | ||||||||||||||||||||||
| !Number.isSafeInteger(params.line) | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The — qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| throw RequestError.invalidParams( | ||||||||||||||||||||||
| undefined, | ||||||||||||||||||||||
| `\`line\` must be a positive integer, got ${params.line}`, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| // BSA0E: cap the file size we'll buffer into RSS at 100 MiB so a | ||||||||||||||||||||||
| // request like `{ line: 1, limit: 10 }` against a 500 MB log | ||||||||||||||||||||||
| // doesn't cost the daemon 500 MB of memory just to return 10 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed in the latest push. I added the symmetric positive-integer guard for line in the inline fallback, so fractional line values now fail before reaching sliceLineRange(), matching the limit validation shape.