Repository navigation
feat(driver): stream external runtime exec and logs - #1388
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (19)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe external host adds command execution and log streaming through runtime RPCs. It retains environment values per workspace for output redaction and adjusts streaming redaction boundaries to avoid splitting complete secret matches. ChangesExternal Runtime Streaming
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant Host
participant RuntimeExec
participant InputReader
participant OutputWriters
Caller->>Host: Run shell or literal-argv command
Host->>RuntimeExec: Start Exec request
Host->>InputReader: Read stdin
InputReader-->>RuntimeExec: Send stdin chunks and close stdin
RuntimeExec-->>Host: Send output frames and terminal exit
Host->>OutputWriters: Write stdout and stderr
Host-->>Caller: Return stream result or command exit error
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was identified; the change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Command and log streaming adds sensitive execution and output paths. Text masking, bounded frames, and cancellation cleanup constrain the change. Binary output intentionally remains unmasked, and secret-retention ownership during future lifecycle integration still needs definition. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 17 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
✨ Simplify code
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 |
✅ Deploy Preview for images-devsy-sh canceled.
|
|
@greptileai review |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
|
@greptileai review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
f95745d to
096a1d7
Compare
|
@greptileai review |
|
@greptileai review |
|
@greptileai review |
|
Final-head review disposition for Read-error recording and host-owned Close are serialized by the same mutex. A non-EOF input failure recorded before owned closure survives successful terminal completion even when completion wins stream cancellation. Readers without Close never acquire the owned-closure marker, so their late failures remain errors. For a closable reader whose pending Read is interrupted by host-owned Close after a valid terminal exit and clean EOF, the cleanup error intentionally preserves that successful exit, regardless of the error type. Concurrent Read and Close use this explicit observation boundary. An arbitrary io.Reader provides no separate provenance signal that would distinguish its underlying return immediately before Close from a return caused by Close. Holding the mutex across a blocking Read would prevent Close from unblocking it. Error-name allowlists cannot establish that ordering either. The deterministic tests cover a read failure before buffered success, both cancellation orderings without owned Close, and six error types released by owned Close. A real os.File early-exit test verifies the successful result and input closure. These boundary and early-exit regressions passed 20 race-enabled repetitions locally, and the final Linux/macOS race suites passed in CI. No additional actionable defect was supplied by this review; the summary concern is dispositioned against the documented ownership contract. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
1 participant
The external runtime host can now execute commands and retrieve logs through Runtime Protocol v1. Exec preserves literal argv, streams bounded binary stdin/stdout/stderr frames concurrently, and requires a terminal exit followed by clean stream completion. Nonzero and signal exits return a typed command error; transport and backend failures retain the existing sanitized runtime error handling.
Cancellation closes interruptible stdin, joins the input pump, reaps the supervised plugin session, and preserves caller deadline errors after cleanup. Read-error recording and owned input closure are serialized: failures observed before host-owned Close survive cancellation and command completion; errors from unblocking a pending Read during owned Close preserve a successful early exit regardless of their type. Readers without Close retain their failures independently of stream cancellation. Text command output uses chunk-safe redaction, including overlapping or repeated secrets at stream end. The host retains per-workspace RunImage environment values across later operations, including prior and failed-request values, using cached immutable operation snapshots and workspace-specific rebuild locks. An immutable byte trie avoids comparing every output byte with every retained secret. RawStdout and argv execution preserve stdout bytes for agent injection. Logs capability is checked before launch and the protocol's merged binary log stream goes to stdout. Documentation spells out stream ownership and the limits of arbitrary blocking readers/writers.
This completes the planned C3 host streaming stage. Workspace factory registration and image-backend integration remain the next C4 stage.
Validation:
c0d692de2, including Linux/macOS race suites, all platform builds, lint, pre-commit, and integration suites. One Podman job needed a targeted retry after its daemon stopped responding during cleanup; that retry passed. Applicable external checks passed.