fix(intent): refuse any non-cargo program - #321
Conversation
🦋 Changeset detectedLatest commit: f9bd2e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b11a33d0c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "cargo-hauler": patch | ||
| --- | ||
|
|
||
| `hauler exec` and `hauler request` now refuse a program that is not cargo, such as `hauler exec -- ls -la`, with `program must be cargo, got ls` and exit code 2. Before, the daemon or the local passthrough ran it and reported it as a cargo run. |
There was a problem hiding this comment.
Correct the request exit code in the release note
In the documented hauler request -- ls scenario, the daemon rejection is converted to a regular error by runTicketEffect, so the routed CLI exits with code 1; only hauler exec maps a bad-intent response to code 2. Claiming that both commands exit 2 gives scripts consuming the release note the wrong contract, so distinguish the two exit codes or change the request command accordingly.
Useful? React with 👍 / 👎.
b11a33d to
f9bd2e7
Compare
Why
hauler exec -- ls -laandhauler request -- lsran the realls. The result card then called it a cargo run.parseCargoArgvinsrc/internal/cargo/intent.tsrefused only a path-shaped first word or a program after a peeledenvprefix. A bare word such aslsbecame the cargo subcommand, whilebuildCommandinsrc/internal/cargo/execution/executor.tsspawnedargv[0]as the program. The two decisions drifted.A second path ran it too.
localQueryReasontreats any parse error as "cargo resolves this locally", and every client passthrough spawns argv in place without parsing. Rejecting in the parser alone would have movedlsfrom the daemon to the client.Scope
parseCargoArgvnow throwsProgramNotCargoErrorwith the existingprogram must be cargo, got Xmessage for any first word that is not a cargo executable. The only exception is a recognizedbash -c/sh -cwrapper, whose zero or several cargo statements stay accepted with subcommandbashas before. This one branch replaces the two narrower ones.runExecCommandinsrc/scripts/hauler.tsis the only caller ofrunExecClient. It now parses the argv once and refusesProgramNotCargoErrorwith exit 2 before the client picks a local query, direct, or daemon-unreachable passthrough. Other parse errors keep their current handling.No supported caller submits argv without a leading cargo. The hook rewrite in
src/internal/host-hooks/inspect.tsinsertshauler exec --immediately before the cargo word. The PATH shim passes the absolute real cargo.hauler_requestandhauler execforward the user argv as given. The only in-repo argv without cargo was a test convenience intests/unit/cargo/intent.test.ts, now prefixed withcargo.Blast Radius
hauler exec -- bash script.shis now refused. It was never a wrapper the parser modeled. Scripts passed asbash -c '...'are unchanged.env NAME=value cargo ...and absolute cargo paths are unchanged.hauler request -- lsis refused by the daemon withbad-intent. The routed request CLI exits 1 for any daemon rejection, which this PR does not change.Verification
Failing first.
parseCargoArgv(['ls', '-la'])failed withexpected [Function] to throw an error.run(['exec', '--', 'ls', '-la'])returned code 0 with empty output instead of code 2.After the fix, from the built CLI with an isolated state dir:
pnpm run checkpassed. Two earlier fullrstestruns each hit a different integration timeout indaemon-ticket-log,daemon-fold-trailers, anddaemon-reattach. The three files pass 28/28 alone and touch no code in this diff.Principles
Fix Root Causes. The parser owns the program decision, and the exec entry consults it once instead of each passthrough mode guarding separately.
Test Behavior, Not Implementation. Both tests call the parser and the
haulerscript entry with literal argv and assert the literal message and exit code.Laziness Protocol. One parser branch replaces two, and the client check is one parse at the single exec entry.