sandbox exec: fix stdout piping and short-id lookup#828
Conversation
Backing up a SQLite file with `miren sandbox exec -i app cat /data/db > backup.db` would print the bytes to the terminal instead of the file. The TTY check only looked at stdin: when stdin was a console we sent its file descriptor as both input and output, silently ignoring shell-level stdout redirection. The server also got winsize and allocated a PTY, which would mangle binary content. Now we probe both stdin and stdout. Only when both are TTYs do we take the interactive path (raw mode, winsize, SIGWINCH). When stdout is redirected we use os.Stdout directly and skip winsize so the server keeps the stream binary-clean. While in there: app run had the identical PTY dance copy-pasted from sandbox exec, and the same bug. Pulled the shared logic into a new setupExecIO helper so both commands go through one path.
`m sandbox exec -i 7g7 ...` would error with "no container found for 7g7" even though `sandbox list` happily showed `7g7` as the short ID. The proxy already resolved the short ID via EAC.Get (EntityServer's index lookup), but forgot to update the local `id` variable to the resolved full entity ID before forwarding to the node-local exec server. So the node searched for a container labeled with literally "7g7", which never exists. The neighboring "app" branch already does `id = found.Id().String()` after its lookup. Mirror that pattern for the "id" branch and the short ID flows through correctly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts interactive terminal and window-resize handling into a reusable setupExecIO helper that returns stdin/stdout, a winUpdates channel, and a cleanup closure. The helper detects TTYs, reads and applies terminal size to shell options, sets stdin to raw mode, and starts a SIGWINCH goroutine to emit WindowSize updates. AppRun and SandboxExec are refactored to call setupExecIO and forward its streams into sec.Exec. The exec proxy is adjusted to use the resolved entity's actual ID when dispatching exec requests. Comment |
There was a problem hiding this comment.
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 `@cli/commands/exec_io.go`:
- Line 44: stdinCon.SetRaw() currently ignores its returned error which can
leave terminal state inconsistent; update the exec_io logic to capture and
handle the error from stdinCon.SetRaw() (call site: stdinCon.SetRaw()), e.g., if
it returns an error log/return the error and avoid assuming raw mode is active,
only call stdinCon.Reset() in the cleanup path when SetRaw succeeded (track with
a boolean like rawEnabled) and ensure any cleanup or defer uses that flag so
Reset() is not invoked erroneously.
🪄 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: 9bd6a450-7580-4a66-a3e3-d5c2aef1f2f0
📒 Files selected for processing (4)
cli/commands/app_run.gocli/commands/exec_io.gocli/commands/sandbox_exec.goservers/exec_proxy/exec_proxy.go
Per CodeRabbit review feedback on #828. SetRaw should not fail on a verified TTY, but if it does we want visibility rather than silently proceeding with potentially inconsistent terminal state.
|
Actionable comments posted: 0 |
Two related papercuts that surfaced while our pal @briancain was trying to back up a SQLite database from a sandbox.
The first one is the headline.
miren sandbox exec -i app cat /data/db > backup.dbwould print the database contents to the terminal instead of redirecting to the file. The TTY check only looked at stdin, so when stdin was a console we sent its file descriptor as both input and output, silently ignoring shell-level stdout redirection. The server would also get winsize and allocate a PTY, which would mangle any binary content even if you worked around the wiring. Now we check both stdin and stdout and only take the interactive path when both are TTYs. When stdout is redirected we useos.Stdoutdirectly and skip winsize so the stream stays binary-clean.While I was in there,
app runhad the identical PTY dance copy-pasted fromsandbox exec(plus the same bug). Pulled the shared logic into a newsetupExecIOhelper so both commands evolve together.The second commit is a separate fix that turned up during testing.
miren sandbox exec -i 7g7 ...would fail with "no container found for 7g7" even thoughsandbox listproudly displays7g7as the short ID. The exec proxy was resolving the short ID viaEAC.Get(which already does short-id index lookup) but then forwarded the original literal string to the node-local exec server instead of the resolved full entity ID. The neighboring "app" branch in the same function already did this correctly; the "id" branch just needed the same one-liner. With this, the short IDs thatsandbox listadvertises actually work withsandbox exec.Closes MIR-1001