fix(minimal): bound the workspace-upload unpack-error buffer by norrietaylor · Pull Request #928 · gominimal/minimal

norrietaylor · 2026-07-23T05:29:46Z

Bound the workspace-upload unpack-error buffer in `Client.upload_workspace_files`

Replaces unbounded accumulation of daemon error output (err.extend_from_slice(&data)) with append_daemon_error, capping the buffer at DAEMON_ERROR_MAX during workspace-file uploads.
Updates the DAEMON_ERROR_MAX doc comment to reflect that the cap applies to all streaming RPCs that buffer daemon errors, not just diagnostic-bundle downloads.
Adds a unit test verifying the cap and an integration test confirming that daemon-side unpack failures propagate as a client-visible error rather than silent success.

Summary by CodeRabbit

Bug Fixes
- Workspace upload failures are now surfaced clearly instead of being silently ignored.
- Error details from failed uploads are safely bounded to prevent excessive diagnostic data.
- Improved reporting for invalid or expired workspace sessions.

`Client::upload_workspace_files` reads the upload channel to its close (the daemon's completion ack) and relays any unpack failure carried on extended-data stream 1, but it accumulated that stream into an unbounded `Vec`. A daemon spraying extended data could balloon the client's memory. Accumulate through the existing `append_daemon_error` helper instead, so the workspace upload shares the 64 KiB `DAEMON_ERROR_MAX` cap that the diagnostic-bundle download already enforces — the same "server acks first, client drains to close" pattern. Adds a client-side regression test that an upload targeting an unknown session surfaces as `Err` rather than a silent success, and a unit test that the error accumulator is capped. Closes #824 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-07-23T05:29:53Z

📝 Walkthrough

Walkthrough

upload_workspace_files now bounds daemon unpack-error buffering through append_daemon_error, documents the shared cap, and adds unit and integration coverage for capped accumulation and surfaced upload failures.

Changes

Workspace upload error handling

Layer / File(s)	Summary
Bounded daemon error capture `crates/minimal/src/client.rs`	Workspace uploads use the shared daemon-error cap when collecting SSH extended-data failures; the cap documentation and unit test were updated accordingly.
Upload failure regression coverage `crates/minimal/tests/cli.rs`	An integration test verifies that uploading to an invalid session returns an error for a daemon-side unpack failure.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

gominimal/inbox#332 — Covers bounding daemon-error collection and testing unpack failure reporting in Client::upload_workspace_files.
gominimal/minimal#901 — Addresses surfacing daemon unpack errors from the workspace upload path.
gominimal/inbox#309 — Describes bounded daemon unpack-error accumulation and regression coverage.

Possibly related PRs

gominimal/minimal#806 — Implements workspace upload daemon-error collection through SSH extended data.
gominimal/minimal#883 — Updates the daemon-side workspace unpack error sent over SSH extended data.
gominimal/minimal#923 — Also changes client handling of workspace-unpack failure details.

Suggested reviewers: twitchyliquid64

Poem

A bunny bounded errors tight,
And caught unpack trouble in flight.
The buffers stayed small,
Tests watched over all,
So uploads now speak when they’re wrong—what a delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	It includes a useful Summary, but the required Testing and Checklist sections are missing.	Add the template sections for Testing and Checklist, including commands/output and the docs/BREAKING CHANGE checklist items if applicable.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise, conventional, and accurately summarizes the main fix.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/minimal/tests/cli.rs`:
- Around line 314-320: Update the test around upload_workspace_files to assert
that the returned error contains the daemon-side “daemon failed to unpack
project files” prefix from the client error path, rather than only checking
result.is_err(). Preserve the unknown-session scenario and ensure unrelated
preparation or channel I/O errors do not satisfy the assertion.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e2c1ffd-dd09-4c52-a519-f6615acc8366

📥 Commits

Reviewing files that changed from the base of the PR and between 5720bf6 and a6d8264.

📒 Files selected for processing (2)

crates/minimal/src/client.rs
crates/minimal/tests/cli.rs

coderabbitai · 2026-07-23T15:56:50Z

+    let result = client
+        .upload_workspace_files(SessionId::nil(), project.path())
+        .await;
+    assert!(
+        result.is_err(),
+        "upload to an unknown session must fail, not report success: {result:?}"
+    );


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the daemon-side error, not merely any error.

This can pass if upload preparation or channel I/O fails before the daemon reports its unpack error. Assert the daemon failed to unpack project files prefix emitted by crates/minimal/src/client.rs.

Suggested assertion

- assert!( - result.is_err(), - "upload to an unknown session must fail, not report success: {result:?}" - ); + let err = result.expect_err("upload to an unknown session must fail"); + assert!( + err.to_string() + .contains("daemon failed to unpack project files"), + "expected daemon unpack failure, got: {err:#}" + );

📝 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.

Suggested change

let result = client

.upload_workspace_files(SessionId::nil(), project.path())

.await;

assert!(

result.is_err(),

"upload to an unknown session must fail, not report success: {result:?}"

);

let err = result.expect_err("upload to an unknown session must fail");

assert!(

err.to_string()

.contains("daemon failed to unpack project files"),

"expected daemon unpack failure, got: {err:#}"

);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal/tests/cli.rs` around lines 314 - 320, Update the test around upload_workspace_files to assert that the returned error contains the daemon-side “daemon failed to unpack project files” prefix from the client error path, rather than only checking result.is_err(). Preserve the unknown-session scenario and ensure unrelated preparation or channel I/O errors do not satisfy the assertion.

norrietaylor marked this pull request as ready for review July 23, 2026 15:54

evanspearman approved these changes Jul 23, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

norrietaylor merged commit 1e8f271 into main Jul 23, 2026
29 checks passed

norrietaylor deleted the fix/824-upload-ack branch July 23, 2026 15:58

coderabbitai Bot mentioned this pull request Jul 24, 2026

fix(minimal): add idle-progress timeout to upload to prevent indefinite hang #920

Merged

coderabbitai Bot mentioned this pull request Aug 5, 2026

fix(sftp): resolve relative paths to session HOME #1177

Merged

Navigation Menu

Uh oh!

fix(minimal): bound the workspace-upload unpack-error buffer - #928

fix(minimal): bound the workspace-upload unpack-error buffer#928
norrietaylor merged 1 commit into
mainfrom
fix/824-upload-ack

norrietaylor commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

norrietaylor commented Jul 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bound the workspace-upload unpack-error buffer in Client.upload_workspace_files

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

norrietaylor commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Bound the workspace-upload unpack-error buffer in `Client.upload_workspace_files`

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading