fix: increase output limits for truncating collector #575
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Deferring to @bolinfest as these limits were added intentionally. |
|
Also please remove the package-lock.json, the project now uses pnpm. And thank you in general for the contribution, it's super appreciated! |
tibo-openai
changed the title
|
The old values often ended up wasting a lot of context window for things like |
|
updated the pr with requested changes. it now has the defaults set in config and takes them from the config file if set, just like how other settings are being propagated. test cases included. thank you 🙏 |
package.json
Outdated
| @@ -16,6 +16,7 @@ | |||
| "husky:add": "husky add" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/node": "^22.14.1", | |||
There was a problem hiding this comment.
I used npm instead of pnpm earlier, and it was showing some linting errors, now that I'm using pnpm its no longer needed, and removed it from the PR also.
codex-cli/src/utils/config.ts
Outdated
| @@ -139,6 +145,10 @@ export type AppConfig = { | |||
| saveHistory: boolean; | |||
| sensitivePatterns: Array<string>; | |||
| }; | |||
| output?: { | |||
There was a problem hiding this comment.
I think we need a different name and maybe a level of two of depth.
That is, this not configuring the general "output" of Codex CLI. (To me, this name implies the "output" that I see as a user.) It is configuring something extremely specific: truncation parameters for the shell tool call.
This makes me wonder if should have a top level config entry named "tools" where keys are tool names that point to dictionaries of arbitrary properties for configuring that tool, so the JSON would look like:
"tools": {
"shell": {
"maxBytes": 12410,
"maxLines": 256
}
}There was a problem hiding this comment.
I changed the config to have these keywords now instead.
| @@ -144,8 +145,20 @@ export function exec( | |||
|
|
|||
| return new Promise<ExecResult>((resolve) => { | |||
| // Collect stdout and stderr up to configured limits. | |||
| const stdoutCollector = createTruncatingCollector(child.stdout!); | |||
| const stderrCollector = createTruncatingCollector(child.stderr!); | |||
| const config = loadConfig(); | |||
There was a problem hiding this comment.
I don't think we should be loading the config at arbitrary places in the code. That means the config could change over the course of the run, which in some cases could be helpful, but in other cases I think could be quite dangerous because it could unintentionally change the behavior of a long-running agent.
Today, loadConfig() is called once in cli.tsx (as it should be) and then it should be threaded through from there.
There was a problem hiding this comment.
You are absolutely right, we should only load config once, to be more predictable, now the config is properly being threaded until raw_exec.ts.
Please let me know if I'm missing anything.
config goes from cli.tsx to app.tsx to TerminalChat to AgentLoop and then to handle-exec-command.ts all these have config already including handleExecCommand function. but with in this file theres a helper function execCommand where config is not passed anymore, I have introduced a new config param for this helper, and while calling this helper function im passing config every in the file handle-exec-command.ts
the helper function execCommand uses either execApplyPatch or exec functions further downstream.
So I handled exec function in exec.ts file to have config and which further uses macos_seatbelt and raw_exec or just raw_exec.
I have threaded the config variable to all these downstream functions that need them, according to the flow I mentioned as far as I am aware.
Please let me know if I'm missing any other flows. I think other flows like cli-singlepass TerminalChatPastRollout flows don't need this particular config propogation.
|
Also, taking a step back: is this going to help the fundamental problem you are trying to solve? Do you really want to increase these limits for all commands or just specific ones? I just took a look at #509 and now I'm wondering if you were using a version of Codex between when I introduced this behavior: and this fix: Because the empty output suggests that might have been the case, in which case the real fix is to ensure you are on the latest version of Codex! |
- Introduced DEFAULT_SHELL_MAX_BYTES and DEFAULT_SHELL_MAX_LINES for shell output limits. - Updated loadConfig and saveConfig to handle new shell settings. - Modified exec functions to utilize shell output limits from config. - Added tests to verify loading and saving of custom shell configurations.
- Introduced effectiveWorkdir to manage the working directory more robustly. - Updated logging to reflect the effective working directory used during command execution. - Added a new test suite for handleExecCommand to ensure proper functionality and error handling.
- Added comprehensive tests for handleExecCommand to cover various execution paths, including sandbox execution, user permission prompts, and command rejection scenarios. - Introduced mocks for exec and execApplyPatch to simulate command execution outcomes. - Improved test structure and clarity, ensuring robust coverage of edge cases and expected behaviors.
- Removed commented-out assertions and unnecessary test structure to enhance clarity and focus on essential test cases. - Adjusted type assertions for better readability and maintainability in the tests for handleExecCommand.
syllogismos
force-pushed
the
increase-output-limits
branch
from
4a62e8f to
85debd0
Compare
Yeah I think you are right. You already fixed with above PRs. But one of the comments suggested these values should be part of the config here #423 (comment), So I continued with my implementation. |
|
I added test cases for for the config settings, and also tried to write test cases for Along with the config propagation, I wrote additional test cases for that file. Please let me know if I followed the test conventions for this repo esp regarding mocking. I wanted to know if I'm in the right track before writing more tests for this file. |
| let { workdir } = execInput; | ||
| if (workdir) { | ||
| // Use a separate variable to track the effective working directory | ||
| let effectiveWorkdir = execInput.workdir; |
There was a problem hiding this comment.
This fix is unrelated to config propagation, but I think its a bug, because earlier, we are changing workdir to process.cwd() if existing workdir is not accessible or found, but we are not passing it downstream.
There was a problem hiding this comment.
we were passing it to execApplyPatch but not to exec earlier. it is now fixed. please let me know if this behaviour is wrong, I will remove these changes and the corresponding test case.
This Pull Request addresses an issue where the output of commands executed in the raw-exec utility was being truncated due to restrictive limits on the number of lines and bytes collected. The truncation caused the message [Output truncated: too many lines or bytes] to appear when processing large outputs, which could hinder the functionality of the CLI.
Changes Made
Increased the maximum output limits in the createTruncatingCollector utility:
Bytes: Increased from 10 KB to 100 KB.
Lines: Increased from 256 lines to 1024 lines.
Installed the @types/node package to resolve missing type definitions for NodeJS and Buffer.
Verified and fixed any related errors in the createTruncatingCollector implementation.
Issue Solved:
This PR ensures that larger outputs can be processed without truncation, improving the usability of the CLI for commands that generate extensive output. #509