fix(core): avoid duplicating stdout content in sandbox summary#1142
fix(core): avoid duplicating stdout content in sandbox summary#1142omeraplak merged 2 commits intoVoltAgent:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 4887248 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a multi-line stream formatter with a single-line summary formatter in the sandbox toolkit, changing per-stream outputs (evicted, inline, empty) to compact single-line summaries and updating call sites to push one summary string per stream. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Added a changeset in this branch to satisfy release/versioning checks:\n- .changeset/tidy-summary-no-dup.md (: patch)\n\nNo code-path changes beyond the original fix. |
|
Correction to my previous note (shell ate backticks): Added changeset file: No runtime behavior changes in this follow-up commit; this is release metadata only. |
|
Hey @SergioChan , |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/workspace/sandbox/toolkit.ts`:
- Around line 244-256: formatStreamSummary currently only shows eviction error
when info.evicted is true, but evictStream() sets info.error on fallback results
with evicted: false so errors get dropped and the message falls back to
"captured inline"; update formatStreamSummary to preserve and display
failed-eviction metadata: if info.error exists (regardless of info.evicted)
include the attempted save path (info.path if present) and the eviction error
note, and when evicted is false but path exists and error exists, render a
message like "<label>: attempted save to <path> failed (eviction error: <error>,
<bytes> bytes...)" so failed write attempts are distinguishable from normal
inline captures; modify the logic in formatStreamSummary (which consumes
StreamEvictionResult returned by evictStream) to check info.error before falling
back to inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a42d1a3a-1e4c-43b2-9040-5415c5f17551
📒 Files selected for processing (1)
packages/core/src/workspace/sandbox/toolkit.ts
| const formatStreamSummary = (label: string, info: StreamEvictionResult): string => { | ||
| if (info.evicted && info.path) { | ||
| const note = info.truncated ? " (truncated)" : ""; | ||
| lines.push(`${label}: saved to ${info.path} (${info.bytes} bytes${note})`); | ||
| if (info.error) { | ||
| lines.push(`${label} eviction error: ${info.error}`); | ||
| } | ||
| return lines; | ||
| const note = info.truncated ? ", truncated" : ""; | ||
| const errorNote = info.error ? `, eviction error: ${info.error}` : ""; | ||
| return `${label}: saved to ${info.path} (${info.bytes} bytes${note}${errorNote})`; | ||
| } | ||
|
|
||
| if (info.content) { | ||
| lines.push(`${label}:`); | ||
| lines.push(info.content); | ||
| return lines; | ||
| const note = info.truncated ? ", truncated" : ""; | ||
| return `${label}: captured inline (${info.bytes} bytes${note})`; | ||
| } | ||
|
|
||
| lines.push(`${label}: (empty)`); | ||
| return lines; | ||
| return `${label}: (empty)`; |
There was a problem hiding this comment.
Preserve failed-eviction metadata in the summary.
evictStream() only sets info.error on fallback results with evicted: false, so the errorNote here never renders. When a write fails, this formatter falls through to "captured inline" and drops both the attempted save path and the eviction failure, which makes that case indistinguishable from a normal inline capture.
Suggested fix
const formatStreamSummary = (label: string, info: StreamEvictionResult): string => {
+ const note = info.truncated ? ", truncated" : "";
+
if (info.evicted && info.path) {
- const note = info.truncated ? ", truncated" : "";
- const errorNote = info.error ? `, eviction error: ${info.error}` : "";
- return `${label}: saved to ${info.path} (${info.bytes} bytes${note}${errorNote})`;
+ return `${label}: saved to ${info.path} (${info.bytes} bytes${note})`;
+ }
+
+ if (info.path && info.error) {
+ return `${label}: captured inline after save to ${info.path} failed (${info.bytes} bytes${note}, eviction error: ${info.error})`;
}
if (info.content) {
- const note = info.truncated ? ", truncated" : "";
return `${label}: captured inline (${info.bytes} bytes${note})`;
}
return `${label}: (empty)`;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const formatStreamSummary = (label: string, info: StreamEvictionResult): string => { | |
| if (info.evicted && info.path) { | |
| const note = info.truncated ? " (truncated)" : ""; | |
| lines.push(`${label}: saved to ${info.path} (${info.bytes} bytes${note})`); | |
| if (info.error) { | |
| lines.push(`${label} eviction error: ${info.error}`); | |
| } | |
| return lines; | |
| const note = info.truncated ? ", truncated" : ""; | |
| const errorNote = info.error ? `, eviction error: ${info.error}` : ""; | |
| return `${label}: saved to ${info.path} (${info.bytes} bytes${note}${errorNote})`; | |
| } | |
| if (info.content) { | |
| lines.push(`${label}:`); | |
| lines.push(info.content); | |
| return lines; | |
| const note = info.truncated ? ", truncated" : ""; | |
| return `${label}: captured inline (${info.bytes} bytes${note})`; | |
| } | |
| lines.push(`${label}: (empty)`); | |
| return lines; | |
| return `${label}: (empty)`; | |
| const formatStreamSummary = (label: string, info: StreamEvictionResult): string => { | |
| const note = info.truncated ? ", truncated" : ""; | |
| if (info.evicted && info.path) { | |
| return `${label}: saved to ${info.path} (${info.bytes} bytes${note})`; | |
| } | |
| if (info.path && info.error) { | |
| return `${label}: captured inline after save to ${info.path} failed (${info.bytes} bytes${note}, eviction error: ${info.error})`; | |
| } | |
| if (info.content) { | |
| return `${label}: captured inline (${info.bytes} bytes${note})`; | |
| } | |
| return `${label}: (empty)`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/workspace/sandbox/toolkit.ts` around lines 244 - 256,
formatStreamSummary currently only shows eviction error when info.evicted is
true, but evictStream() sets info.error on fallback results with evicted: false
so errors get dropped and the message falls back to "captured inline"; update
formatStreamSummary to preserve and display failed-eviction metadata: if
info.error exists (regardless of info.evicted) include the attempted save path
(info.path if present) and the eviction error note, and when evicted is false
but path exists and error exists, render a message like "<label>: attempted save
to <path> failed (eviction error: <error>, <bytes> bytes...)" so failed write
attempts are distinguishable from normal inline captures; modify the logic in
formatStreamSummary (which consumes StreamEvictionResult returned by
evictStream) to check info.error before falling back to inline.
2 participants
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
execute_commandtool responses can duplicate large payloads becausesummaryincludes fullstdout/stderrcontent that is already returned in structured fields.What is the new behavior?
summaryis now metadata-only for stream outputs:Exit code,Duration, etc.)captured inline,saved to <path>, or empty)summaryfixes #1140
Notes for reviewers
Validation:
pnpm -C packages/core test src/workspace/sandbox/toolkit.spec.tsnode_modulesmissing /vitestnot found).I kept the diff scoped to
packages/core/src/workspace/sandbox/toolkit.tsonly.Summary by cubic
Prevent duplication of STDOUT/STDERR content in sandbox execution summaries to reduce payload size and make responses clearer. The summary now reports stream metadata only, not the full content. Fixes #1140.
Written for commit 4887248. Summary will update on new commits.
Summary by CodeRabbit