fix(sandbox): honor --cwd/--env after the box name - #81
Merged
Conversation
urfave/cli stops parsing flags at the first positional argument, so
every flag written after the sandbox name is discarded in silence:
process run <box> --cwd /workspace -- pwd
body: {"cmd":"pwd"} cwd lost
process run --cwd /workspace <box> -- pwd
body: {"cmd":"pwd","cwd":"/workspace"} cwd sent
The process commands already carried raw-argv fallbacks for exactly
this problem (processBoolFlag, processIntFlag, processStringFlag,
rawProcessFlagValue). That is why --pty survived after the box name
and --cwd did not: the cwd, cmd, and env reads never used them.
Wire cwd and cmd to the existing processStringFlag. Add
processStringSliceFlag and rawProcessFlagValues, because --env is
repeatable and needs every occurrence, not just the first.
Both raw readers stop at "--", so a sandbox command can never inject
a flag. A test covers that.
This is the same class of bug as #66, which hand-rolled parsing for
the tunnel command. Two commands still carry it: sandbox sync reads
--local/--remote/--mode with no fallback, and sandbox exec loses
--stream. Left alone here, because the root fix is one argv reorder
before parsing and it touches every command.
Verified against a live sandbox:
old, flag after box: "cwd": "/root"
new, flag after box: "cwd": "/workspace"
new, flag before box: "cwd": "/workspace"
new, no flag: "cwd": "/root"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
urfave/clistops parsing flags at the first positional argument. Every flag written after the sandbox name is discarded, with no error.Captured with
CREATEOS_DEBUG=1:process run <box> --cwd /workspace -- pwd{"cmd":"pwd"}— cwd lostprocess run --cwd /workspace <box> -- pwd{"cmd":"pwd","cwd":"/workspace"}The control plane is not at fault. The CLI never sent the field.
Why
--ptyworked and--cwddid notThe process commands already carry raw-argv fallbacks for exactly this problem:
processBoolFlag,processIntFlag,processInt64Flag,processStringFlag, andrawProcessFlagValue. Bool and int flags were recovered. Thecwd,cmd, andenvreads never used them.The change
cwdandcmdnow read through the existingprocessStringFlag.processStringSliceFlagandrawProcessFlagValues, because--envis repeatable and needs every occurrence, not just the first.cmd/sandbox/process_flags_test.go, 6 tests.Both raw readers stop at
--, so a sandbox command can never inject a flag. One test covers that:Who this unblocks
The Herdr plugin (
createos.sandbox) starts its agent withprocess start <box> --pty --cwd /workspace. The agent landed in/rootand could not see the uploaded worktree. Confirmed with/proc/<pid>/cwd.Verified against a live sandbox
/root/workspace/workspace/rootChecks run:
go build ./...,go vet, fullgo test ./...,golangci-lint 2.11.3(the version pinned in.tool-versions, 0 issues),gosec(no findings in the changed file).Still open, deliberately
This is the same class of bug as #66, which hand-rolled parsing for
tunnel. Two commands still carry it:cmd/sandbox/sync.goreads--local,--remote,--modewith no fallback, sosandbox sync <box> --local ...drops them and falls back to an interactive prompt.sandbox exec <box> --streamloses--stream.The per-command fallbacks are a workaround. The root fix is one argv reorder before
urfave/cliparses, which would remove the need for all of them. That touches every command, so it is left for a separate decision.