fix(islo): propagate streamed output delivery failures

[steipete]

Summary

Islo's SSE decoder ignored errors returned while writing command stdout and stderr. A run could therefore report success after its output destination rejected the bytes. Return the original writer error through the decoder's existing failure path so the run reports a delivery failure instead of a successful remote exit.

This stays inside Islo's transport adapter. SSE framing, multiline data, comments, final EOF flushing, repeated exit events, stream limits, and successful output bytes are unchanged. Existing read/decode failures already take precedence over a previously observed exit; output delivery now follows that same rule. The provider's existing run finalizer still owns retention and guarded cleanup. Closing a stream is not evidence that its remote workload stopped.

The provider documentation clarifies the completion rule, and Unreleased records the user-visible fix. No configuration, credentials, dependency, pricing, idle/reclaim, heartbeat, or other provider policy changes are included.

Verification

Candidate head: 79ff10914e08cef124d90d562af5160480ae4261, tree: 0b4771dbf1cc204de0e439eeda90df599866f162, based on 4f6a77b16199fb4a31c28a164ede394b9d07e6bd.

• Eight pre-fix regression cases failed: stdout/stderr failures before exit, after exit and during final EOF flush, plus both real read-only output-file cases. All pass after the fix, with the original writer cause preserved.
• Completion-rule controls passed before and after: multiline/comments/EOF, last exit wins, error-plus-exit, and late reader/decode failures.
• Focused parser races, full Islo race tests, Islo vet, internal-link checks across 246 Markdown files and diff checks passed.
• Independent Codex review is scoped-clean at P0; this is not an all-priority certification.

Real-provider verification and integration

The actual before/after comparison passed all 18 CLI commands using two real Islo sandboxes: normal output/exit7 preserved, both quiet-descriptor controls passed, and rejected stdout/stderr changed from silent baseline0 to candidate1. Entry-witness reads avoid cross-channel ordering assumptions. Both owned sandboxes were explicitly stopped, with original-ID deleted tombstones, exact-name HTTP404 and no local claims. All run session handles were retained correctly. This is retained-run plus explicit Stop proof, not automatic deletion-failure or remote-cancellation testing.

Full observed output and scope: #1969 (comment). Independent audit and parent recheck verified the 4,636-entry evidence seal. The audit also caught a task-bound credential daemon recreating an empty temporary directory after initial cleanup; that specific daemon and directory were cleaned, and the original observation remains preserved.

The real execution stays bound to the original ad2/7f7 sources. Current integration includes the separately landed AWS Worker diagnostics change and resolves only the changelog conflict. Both newly built CLIs are byte-identical to those actually executed, and the full 1,497-file Go source scope is unchanged. No hosted replay or incoming AWS feature proof is claimed. Fresh Islo/shared races, docs and independent P0-scoped review passed on this integrated tree.

Current-head CI passed all 11 main jobs: https://github.com/openclaw/crabbox/actions/runs/34158254464. All five code-validation workflows succeeded, including connector smokes and the release check; no reruns were needed. The current-head review accepts the real-provider proof and reports no actionable code findings. The final merge target against main 4f6a77b16199fb4a31c28a164ede394b9d07e6bd is exactly the reviewed/tested integrated tree 0b4771dbf1cc204de0e439eeda90df599866f162. Related lifecycle/heartbeat holds remain separate: #1706 and #1707.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs changes before merge. Reviewed September 7, 2026, 4:16 PM ET / 20:16 UTC.

ClawSweeper review

What this changes

The Islo adapter now propagates stdout/stderr delivery errors, with regression tests and documentation explaining when remote exit codes remain authoritative.

Merge readiness

⛔ Needs changes before merge - 1 item remains

This remains a useful fix: current main and v0.52.0 still discard output-writer errors. The new real-provider evidence resolves the previous proof request, and no blocking correctness defect was found.

Priority: P2
Reviewed head: 79ff10914e08cef124d90d562af5160480ae4261

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused, correct repair with strong real-provider comparisons and useful regression controls.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The new captured terminal results exercise the real CLI → Islo HTTP/SSE decoder → Run finalizer path: failed stdout/stderr now return 1, normal and exit-7 controls remain correct, and retained sessions plus explicit Stop cleanup are observed. The relevant source is unchanged at the reviewed integration, satisfying the earlier rank-up request.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The new captured terminal results exercise the real CLI → Islo HTTP/SSE decoder → Run finalizer path: failed stdout/stderr now return 1, normal and exit-7 controls remain correct, and retained sessions plus explicit Stop cleanup are observed. The relevant source is unchanged at the reviewed integration, satisfying the earlier rank-up request.
Evidence reviewed	8 items	Policy and repository identity: The origin identifies openclaw/crabbox. Root AGENTS.md was read fully; no nested AGENTS.md applies under internal or docs, and no maintainer-notes directory exists. Provider boundaries and regression-coverage guidance informed the review. Fix remains necessary on main: Current main ignores both stdout.Write and stderr.Write errors. The existing flush-error path already returns code 1 for decoder failures, making the proposed propagation a narrow extension of established behavior. Latest release still has the defect: The supplied latest-release commit for v0.52.0 also discards both output-writer errors; this is not an already-shipped fix.
Findings	None	None.
Security	None	None.

How this fits together

Crabbox’s Islo adapter sends commands directly to remote sandboxes and decodes their streamed output into local stdout and stderr. Its result feeds the CLI’s exit status and existing session retention or cleanup handling.

flowchart TD
  A[CLI command] --> B[Islo HTTP execution]
  B --> C[Stream decoder]
  C --> D[Local stdout and stderr]
  D --> E{Delivery succeeded?}
  E -->|Yes| F[Preserve remote exit]
  E -->|No| G[Return delivery failure]
  F --> H[Finalize session and cleanup]
  G --> H

Loading

Before merge

• Complete next step (P2) - Mark the PR ready for review after the author’s stated current-head checks and final merge-target verification are complete.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	Production +6/-2 lines; tests +102 lines	The small production change is justified by propagating discarded errors, with substantially broader regression coverage.

Technical review

Best possible solution:

Keep output-delivery failures in the existing Islo stream-error path while preserving successful remote exits and established session cleanup semantics.

Do we have a high-confidence way to reproduce the issue?

Yes. Current-main source discards writer errors, and the supplied real-provider comparison demonstrates false success with read-only stdout/stderr descriptors; this review did not execute the reproduction.

Is this the best way to solve the issue?

Yes. Returning the original writer error through the existing flush failure path is the narrowest repair, and the supplied live controls demonstrate that healthy output and remote exit codes remain intact.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 4f6a77b16199.

Labels

Label justifications:

• P2: This fixes false-success reporting when an Islo command’s local output destination rejects writes.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The new captured terminal results exercise the real CLI → Islo HTTP/SSE decoder → Run finalizer path: failed stdout/stderr now return 1, normal and exit-7 controls remain correct, and retained sessions plus explicit Stop cleanup are observed. The relevant source is unchanged at the reviewed integration, satisfying the earlier rank-up request.
• proof: sufficient: Contributor real behavior proof is sufficient. The new captured terminal results exercise the real CLI → Islo HTTP/SSE decoder → Run finalizer path: failed stdout/stderr now return 1, normal and exit-7 controls remain correct, and retained sessions plus explicit Stop cleanup are observed. The relevant source is unchanged at the reviewed integration, satisfying the earlier rank-up request.

Evidence

What I checked:

• Policy and repository identity: The origin identifies openclaw/crabbox. Root AGENTS.md was read fully; no nested AGENTS.md applies under internal or docs, and no maintainer-notes directory exists. Provider boundaries and regression-coverage guidance informed the review. (AGENTS.md, 79ff10914e08)
• Fix remains necessary on main: Current main ignores both stdout.Write and stderr.Write errors. The existing flush-error path already returns code 1 for decoder failures, making the proposed propagation a narrow extension of established behavior. (internal/providers/islo/client.go:514, 4f6a77b16199)
• Latest release still has the defect: The supplied latest-release commit for v0.52.0 also discards both output-writer errors; this is not an already-shipped fix. (internal/providers/islo/client.go:514, e32b14fad760)
• Production failure handling: ExecStream returns the decoder result and closes the response body. Run preserves the error cause, uses the shared command-outcome classifier, and executes its existing deferred retention and guarded cleanup logic. The patch does not alter cleanup authorization. (internal/providers/islo/backend.go:294, 79ff10914e08)
• Regression coverage: The added tests cover six writer-failure combinations across stdout/stderr and event positions, two real read-only-file destinations, and six completion-rule controls. Tests were inspected, not executed during this read-only review. (internal/providers/islo/backend_test.go:116, 79ff10914e08)
• New live evidence satisfies the prior request: The captured comment fix(islo): propagate streamed output delivery failures #1969 (comment) reports 18 real Darwin/arm64 CLI commands against two Islo sandboxes. Rejected stdout and stderr changed from baseline exit 0 to candidate exit 1; normal output, remote exit 7, and quiet-descriptor controls were preserved. All 16 run sessions retained the correct lease, and explicit Stops were followed by deleted-ID, name-404, and local-claim checks. This proves retained-run behavior and explicit cleanup, without claiming automatic deletion-failure or remote-cancellation coverage. (7f7b3ae8dd47)

Likely related people:

• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Yossi Eliaz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-09-07T19:29:49.542Z sha 7f7b3ae :: needs real behavior proof before merge. :: none
• reviewed 2026-09-07T20:11:50.122Z sha 79ff109 :: needs real behavior proof before merge. :: none

[steipete]

Observed live comparison for head 7f7b3ae8dd474bbf3325ad849936d53de8bfd1a1, tree 8d31437213ec63a194a6d6f64857177b6a602112, against main ad2cc622d80430f1fc5efaa2b5d315ffdade9125.

Both actual Darwin/arm64 CLIs were built with Go 1.26.5 using -trimpath -buildvcs=false, from separately archived source. All 2,241 Git blobs and modes per side were checked. Baseline binary SHA-256: 3299dd73da74ddf84440e99941286e390caf675bd4f479219a957279b4824037; candidate: 2c3b962eae89b7f17f5e8eb6449e04bfac0de0fec44e8c90519a748e0afef35b.

One real Islo sandbox per version, one active at a time: Ubuntu 26.04, 1 vCPU, 1024 MB memory, 10 GB disk. Fresh run --keep synced a single 913-byte synthetic shell workload; later runs used the captured lease with --no-sync. All runs explicitly used --timing-record off without timing JSON, so rejected diagnostic/timing output could not masquerade as the command-delivery fix.

Actual scenario	Baseline exit	Candidate exit
Normal stdout/stderr	0	0
Genuine remote exit 7	7	7
Read-only stdout, workload silent on stdout	0	0
Read-only stderr, workload silent on stderr	0	0
Workload stdout rejected by read-only descriptor	0	1
Healthy-descriptor stdout entry-witness read	0	0
Workload stderr rejected by read-only descriptor	0	1
Healthy-descriptor stderr entry-witness read	0	0
Explicit normal Stop	0	0

The failed sinks were real O_RDONLY /dev/null descriptors inherited as stdout/stderr, not a fake provider response or a SIGPIPE test. Both quiet controls ran before either failed-delivery case and passed, showing that best-effort progress output alone was not the source of failure. Candidate stdout rejection printed islo run failed: write /dev/stdout: bad file descriptor. The stderr diagnostic was inherently unavailable on its failed descriptor; the evidence there is the changed exit, healthy output, retained session and passing controls—not an invented captured message.

Each failing-output workload wrote a case-specific witness before emitting its output. An unconditional later healthy-FD command verified that witness. This establishes workload entry without assuming stdout/stderr event ordering or claiming that closing the stream stopped the remote command. All 16 run session files identified the correct kept lease and correct fresh/reused state.

All 18 CLI processes completed and were reaped; no capture overflow or guard deadline fired. Baseline cleanup completed before candidate allocation. Both normal Stops returned success; separate read-only API observations returned the original immutable sandbox IDs with status deleted, exact-name HTTP 404, and zero local claims. No task sandbox remains. No recovery, outer retry, adoption, extra allocation, or provider failure injection occurred; SDK-internal HTTP attempts were not separately counted. The fixture supplied its API key only through process environments, not arguments, workload environment or public output. The wrapper exited and its task credential window was retired. Independent cleanup inspection later found that a task-bound credential-helper daemon had recreated an empty temporary directory; that exact daemon was identified by its task paths, terminated, and the directory removed. The earlier removal observations and later correction are retained separately; no undocumented claim is made about the credential tool's internal storage.

This is real CLI → Islo HTTP/SSE → Run finalization → retained-session → explicit Stop proof. It does not test automatic deletion failure, remote process cancellation, or crafted post-exit SSE ordering; source regression tests cover late decoder/writer precedence. Driver SHA-256: a7853de69778b17123de035180e55b70cfe00d3847a5131042d8a07befe656c8.

The executed-source head 7f7b3ae8 passed all 11 main CI jobs: https://github.com/openclaw/crabbox/actions/runs/34155489667. Connector smokes and the release check also passed, with no reruns. Dispatch cancellations/skips are recorded separately, not counted as failed code checks.

Independent inspection verified the raw command results, session files, provider readbacks and cleanup, including the late credential-helper correction. The 4,636-entry evidence seal was independently produced and rechecked; audit SHA-256: c9670c7f48bdd3d1192d72085cecb77744e5a8001ecd55347c6e28a69e0d6bef.

Current integration 79ff10914e08cef124d90d562af5160480ae4261 (tree 0b4771dbf1cc204de0e439eeda90df599866f162) includes main 4f6a77b16199fb4a31c28a164ede394b9d07e6bd and preserves both changelog entries. Separate archive/build checks verified 2,243 Git blobs/modes per side, an unchanged 1,497-file Go source scope, and byte-identical baseline/candidate CLIs. The old hosted execution remains bound to its original sources; it is not a new whole-tree run or proof of the incoming AWS Worker feature. Fresh integrated Islo/shared races, docs and scoped P0 review passed. New-head CI is running at https://github.com/openclaw/crabbox/actions/runs/34158254464; this proof does not claim it has finished.

[steipete]

Merged as 89989f2d07849228e581daded423f5075fbbd6a7. Its raw parent is 4f6a77b16199fb4a31c28a164ede394b9d07e6bd, and its actual tree is exactly the reviewed/tested integration 0b4771dbf1cc204de0e439eeda90df599866f162.

The fix stops silently discarding Islo stdout/stderr writer errors. Successful stream delivery still preserves output and remote exit codes; failed delivery uses the existing stream-error path, with provider-owned retention and guarded cleanup unchanged. The documentation and Unreleased changelog entry are on main.

Verification included eight pre-fix regression failures, focused/full Islo race tests, shared races, Islo vet, internal-link checks across 246 Markdown files, independent scoped P0 review, and exact-head CI with all 11 main jobs passing: https://github.com/openclaw/crabbox/actions/runs/34158254464. Connector and release checks passed without reruns.

The actual two-sandbox, 18-command Islo comparison and its precise limits are recorded at #1969 (comment). Both failed-output cases changed from baseline success to candidate exit1; ordinary exits0/7 and quiet controls stayed correct. Both exact resources were deleted and local claims removed. Independent audit and parent recheck passed after correcting a task-bound credential daemon's late temporary-directory recreation; that history was preserved. The integrated CLI binaries are byte-identical to those actually executed, rather than an assertion that the historical hosted run was replayed on a new tree.

The separate run-admission change #1970 landed immediately afterward. Current main f5dfb62977ce9ba4b87d5d68da37bd110f2d8d74 was pulled clean, and the following post-merge checks passed: Islo 8.323s, shared 3.065s.

GOTOOLCHAIN=go1.26.5 go test -race -count=1 -timeout=180s ./internal/providers/islo ./internal/providers/shared

That final source test is distinct from the earlier hosted binary proof; no claim is made that the newer main CLI was live-replayed. Adjacent lifecycle/heartbeat proof holds remain untouched.