fix(cli): report partial output only when present - #359
Conversation
Signed-off-by: Mr-Neutr0n <harikp2002@gmail.com>
ting-hong-shieh
left a comment
ting-hong-shieh
left a comment
There was a problem hiding this comment.
Reviewed at 79e9fe1a569696160fe5d2e46d73f600e9155bc8. The new empty/non-empty directory cases work, but the same misleading retained-output claim remains when the path no longer exists.
Validation snapshot:
- Exact head and a clean synthetic merge onto current
main(216212b70a421b0bf30319d241de47a64050ec0f) both pass the two existing focused tests, TypeScript checking, formatting, andgit diff --check. - The same missing-path fixture fails on both trees. Its structured result is
exit 2 / claimed=true,SIGINT 130 / claimed=true, andSIGTERM 143 / claimed=true. - On the current-main merge, treating
ENOENTas no retained output makes all three cases pass. The PR's empty/non-empty test, TypeScript checking, formatting, and whitespace check remain clean.
I left one blocking inline finding. No model or provider call, API request, credential, or external service was used.
| } catch { | ||
| // Keep the path in the diagnostic when it disappeared or cannot be read. | ||
| // Only suppress the message when emptiness was confirmed. | ||
| return true; |
There was a problem hiding this comment.
Handle ENOENT as no retained output
Both this helper and hasPartialOutputSync turn every readdir failure into true, including ENOENT. If the path is removed after onOutputDirReady reports it, ordinary failure, SIGINT, and SIGTERM still print Partial output was kept at <missing path>. That is the same false claim this PR is meant to suppress, and #191 asks to show it only when the directory is non-empty.
At exact head 79e9fe1a, and again after a clean merge onto main 216212b7, the same fixture produced {exit: 2, claimed: true}, {signal: SIGINT, exit: 130, claimed: true}, and {signal: SIGTERM, exit: 143, claimed: true}. Returning false for ENOENT in both helpers makes all three cases pass while preserving the PR's empty/non-empty test and the static checks. Please handle the missing-path case and add ordinary plus signal-path regression coverage.
|
Security 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79e9fe1a56
ℹ️ 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".
|
|
||
| async function hasPartialOutput(path: string): Promise<boolean> { | ||
| try { | ||
| return (await readdir(path)).length > 0; |
There was a problem hiding this comment.
When a failed or interrupted scan leaves a large number of top-level entries, both readdir() here and readdirSync() in the companion helper materialize the entire directory merely to determine whether one entry exists. This can substantially delay failure reporting or exhaust memory; the synchronous variant also blocks signal handling during interruption. Use a directory iterator and stop after the first entry so this optional diagnostic cannot hold up CLI termination.
AGENTS.md reference: sdk/typescript/AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| if (failed) { | ||
| const partialOutput = scanDir !== null && (await hasPartialOutput(scanDir)); |
There was a problem hiding this comment.
When --max-cost is exceeded before any artifact is written, this correctly computes partialOutput as false, but ScanCostLimitExceededError subclasses ScanInterruptedError and hardcodes “partial output remains at …” in its message. The early return for interrupted errors therefore still prints and returns that misleading path while the verbose diagnostic says partial_output=false; build the displayed message from the computed state or otherwise handle the cost-limit subtype before that return.
Useful? React with 👍 / 👎.
3 participants
Summary
Partially addresses #191 by making the CLI verify that a registered scan output directory contains entries before reporting partial output. Empty directories no longer produce a misleading path, while directories that contain entries, disappear, or cannot be inspected retain the existing diagnostic.
Verification
pnpm exec bun test --timeout 30000 ./tests-ts/cli.test.ts --test-name-pattern "does not claim partial output"pnpm run typespnpm run format