feat(fleet): usable read-only shell for scouts/reviewers (#5426) - #5428
Conversation
…5356 follow-up) Parked while 0.9.8 publishes. Split classifier + shell gate + git -C/pipeline exec paths are in; test run pending. Do not merge as-is.
ShellPolicy::ReadOnly gated every concrete bash call through is_parallel_readonly_command — the classifier built for the parent's parallel auto-approve chunks, whose tightness (no pipes, no globs, no git -C, no find) is load-bearing there but left the #5356 bounded shell behaviorally dead: live scouts concluded 'bash is entirely denied' and crawled with read/grep or spawned worker children just to run git. Split the classifiers. is_agent_readonly_shell_command keeps the mutation-proof charset (no separators, redirects, expansions, subshells, env prefixes) but admits pipelines where every segment must itself pass the shared token logic, git -C/--no-pager preambles that re-enter the existing per-subcommand option tables, find without mutating primaries, sed -n <range>p, npm view|show|info, and the pure filters sort/uniq/cut/tr/comm. head/tail -N numeric shorthands are recognized as -n N (digit-only form, line-count commands only). Execution: single commands keep the direct-argv hardening (now -C-aware); pipelines exec through set -o pipefail with every segment pre-validated; workspace operand confinement still applies to the whole string. The parent's parallel auto-approve path is unchanged. Tests: 4 new classifier tables (admissions incl. CJK-adjacent pipeline shapes; injection/mutation denials incl. find -exec, sed w-scripts, npm install, env prefixes, empty segments; per-segment pipeline denials; parallel-classifier non-regression), plus the existing 60 command_safety, 113 shell, 42 fleet-exact, and 34 readonly-tagged suites green.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @Hmbown's task in 6m 12s —— View job Review
Careful, well-documented work on the classifier itself (the segment-based pipeline validation, Correctness / Security bugs1. Pipeline + pre-existing if argv.first().is_some_and(|program| program == "gh") {
return Ok(());
}This short-circuit was safe before this PR because passes Fix direction: split on 2. fn is_agent_readonly_sed(tokens: &[String]) -> bool {
if tokens.len() < 3 || tokens[1] != "-n" { return false; }
let script = tokens[2].as_str();
...
}Only Fix direction: require exactly 3 tokens (or reject any token after index 2 that starts with 3. Advertised glob ( Reuse / design (good choices)
Tests
SecurityFindings #1 and #2 above are the security-relevant ones — both are workspace-confinement/mutation-policy bypasses for the Fix these → |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36ec61342e
ℹ️ 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".
| "find" => is_agent_readonly_find(&tokens), | ||
| "sed" => is_agent_readonly_sed(&tokens), | ||
| "npm" => is_agent_readonly_npm(&tokens), | ||
| "sort" | "uniq" | "cut" | "tr" | "comm" => true, |
There was a problem hiding this comment.
When a read-only Scout or Reviewer inherits Agent mode's workspace-write sandbox, unconditional acceptance allows sort input -o input to rewrite a workspace file. GNU sort --help documents -o, --output=FILE as writing the result to a file, and uniq likewise accepts an output-file operand, so these commands need command-specific option and arity allowlists rather than blanket admission.
Useful? React with 👍 / 👎.
| if tokens.len() < 3 || tokens[1] != "-n" { | ||
| return false; |
There was a problem hiding this comment.
In the same workspace-write context, sed -n 1p -i file passes this predicate because only tokens[1] and tokens[2] are validated and all later tokens are ignored. GNU sed --help identifies -i as the in-place option; this example rewrites the file to its first line despite ShellPolicy::ReadOnly, so the remaining tokens must be parsed as operands while rejecting every additional option.
Useful? React with 👍 / 👎.
| let piped = format!("set -o pipefail; {command}"); | ||
| CommandSpec::shell(&piped, work_dir.clone(), Duration::from_millis(timeout_ms)) |
There was a problem hiding this comment.
For any admitted pipeline, this raw-shell branch skips hardened_readonly_argv, so a command such as git show HEAD | head runs without the injected --no-textconv and --no-ext-diff flags. Git's configuration catalog includes diff.<driver>.textconv, diff.<driver>.command, and diff.external; a repository-local diff driver selected by .gitattributes can therefore execute an arbitrary helper in the supposedly read-only agent context. Build the pipeline from individually resolved and hardened argv segments instead of interpolating the original command.
Useful? React with 👍 / 👎.
| loop { | ||
| match rest.first().map(String::as_str) { | ||
| Some("--no-pager") => rest = &rest[1..], | ||
| Some("-C") if rest.len() >= 2 => rest = &rest[2..], | ||
| _ => break, |
There was a problem hiding this comment.
When the workspace contains sub/link as a symlink to an external directory but no top-level link, git -C sub -C link status is admitted here. The operand guard checks both sub and link against the original effective cwd, while Git resolves the second relative -C from sub, so the nonexistent top-level operand evades canonicalization and Git reads outside the workspace. Reject repeated -C options or validate them in the same cumulative order Git applies them.
Useful? React with 👍 / 👎.
| // A pipeline is admitted only when every segment is: `a | b` is two | ||
| // read-only commands, while `a | | b`, `a |`, and `||` all carry an empty | ||
| // segment and reject. Quoted pipes inside an argument mis-split here, | ||
| // which only ever makes a segment fail classification (fail closed). | ||
| trimmed.split('|').all(is_agent_readonly_segment) |
There was a problem hiding this comment.
When the configured network policy denies GitHub, gh pr view 123 | head is nevertheless admitted by this per-segment classifier. enforce_readonly_github_network_policy asks is_github_readonly_command about the complete string, but that predicate delegates to the parallel classifier, which rejects every |; the guard therefore returns without evaluating api.github.com, and the Agent-mode sandbox still permits network access. Detect admitted gh segments individually before dispatch.
Useful? React with 👍 / 👎.
| tokens | ||
| .iter() | ||
| .skip(1) | ||
| .all(|token| !MUTATING_PRIMARIES.contains(&token.as_str())) |
There was a problem hiding this comment.
When the workspace contains a directory symlink to an external location, find -L . -type f and the equivalent -follow form pass this denylist and traverse that external tree. GNU find --help exposes both -L and -follow, and the operand guard only canonicalizes the starting . rather than paths encountered during traversal, so this bypasses the stated workspace-read boundary. Reject symlink-following modes or constrain every traversed result.
Useful? React with 👍 / 👎.
| let piped = format!("set -o pipefail; {command}"); | ||
| CommandSpec::shell(&piped, work_dir.clone(), Duration::from_millis(timeout_ms)) |
There was a problem hiding this comment.
On Unix systems where $SHELL is absent and the dispatcher falls back to /bin/sh (commonly dash), every newly admitted pipeline fails before executing because /bin/sh reports set: Illegal option -o pipefail; the same prefix is also not valid for the dispatcher’s cmd.exe and PowerShell paths. Select syntax by ShellKind or execute a fixed POSIX-compatible pipeline implementation instead of injecting a Bash-only preamble into the generic shell dispatcher.
Useful? React with 👍 / 👎.
| let spec = if let Some(workspace) = readonly_workspace { | ||
| let (program, args) = hardened_readonly_argv(command)?; | ||
| let program = resolve_readonly_program(&program, workspace)?; | ||
| CommandSpec::program( | ||
| program | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("read-only executable path is not valid UTF-8"))?, | ||
| args, | ||
| work_dir.clone(), | ||
| Duration::from_millis(timeout_ms), | ||
| ) | ||
| if command.contains('|') { |
There was a problem hiding this comment.
A command such as the newly tested ls -la *.md contains no pipe, so it takes the direct-program branch and receives *.md as a literal argv element; no shell performs the advertised glob expansion, and ls fails unless a file is literally named *.md. Handle validated glob expansion in the direct execution path as well as in pipelines.
Useful? React with 👍 / 👎.
| // A pipeline is admitted only when every segment is: `a | b` is two | ||
| // read-only commands, while `a | | b`, `a |`, and `||` all carry an empty | ||
| // segment and reject. Quoted pipes inside an argument mis-split here, | ||
| // which only ever makes a segment fail classification (fail closed). | ||
| trimmed.split('|').all(is_agent_readonly_segment) |
There was a problem hiding this comment.
For an admitted command such as gh pr view 123 | cat /etc/passwd, the workspace operand guard returns immediately merely because the first argv token is gh, so it never examines the absolute operand belonging to the later cat segment. The raw pipeline then reads that file under Agent mode, whose workspace-write sandbox does not restrict reads outside the workspace. Apply the gh exemption only to that segment and validate every remaining segment independently.
Useful? React with 👍 / 👎.
| fn is_agent_readonly_npm(tokens: &[String]) -> bool { | ||
| matches!( | ||
| tokens.get(1).map(String::as_str), | ||
| Some("view" | "show" | "info") | ||
| ) |
There was a problem hiding this comment.
A read-only agent can now run npm view, but the only shell network guard recognizes GitHub CLI commands, so npm traffic never reaches NetworkPolicyDecider. npm also honors a workspace .npmrc registry setting, allowing an untrusted repository to redirect this command to an arbitrary denied or prompt-only host while the Agent-mode sandbox permits network access. Pin and evaluate the effective registry host before admitting these npm reads.
Useful? React with 👍 / 👎.
The classifier tests landed above the original use super::*, leaving a second, unused copy that -Dwarnings turns fatal in CI's lib-test build.
|
Claude finished @Hmbown's task in 2m 43s —— View job Review
This PR is still Correctness / security (blocking)1. 2. 3. Pipelines skip Git diff/textconv hardening 4. Network policy is never evaluated for pipelines (and never exists for 5. Advertised glob support doesn't work for non-pipeline commands, and where it does work it bypasses operand confinement Reuse (good)
TestsSolid coverage of I did not run Fix these → |
… never widens authority Acceptance 2 of Hmbown#5426, made mechanical and written down: - docs/SUBAGENTS.md: delegation moves work, never authority. A read-only role delegating to a write-capable role is an intended escape hatch for work capacity, but the child clamps against the delegating parent's live posture, not the operator's — read-only is transitive through any delegation chain, and a delegated child cannot obtain canonical Bash, so spawning for shell is mechanically useless. The scout's own bounded read-only shell (PR Hmbown#5428) is the only shell path a read-only parent has. - fleet/exact.rs: role-isolation test a_read_only_parents_delegation_never_widens_authority pins the end state: a write-capable builder member dispatched from a read-only scout lands write=false, shell=ReadOnly, posture_role=scout, raw shell + Bash + mutating tools denied, while the delegation budget (max_depth) survives — the escape hatch stays open for capacity, never for authority.
… shell (Hmbown#5426) The first live dogfood against a freshly built Hmbown#5428 binary denied a scout all three canonical inspection commands (git -C … log, find … | head, npm view) at posture_permits_tool: the Required branch demanded ShellPolicy::Full, and the relaxed is_agent_readonly_shell_command classifier was unreachable from the gate — it only guards the BashTool::execute ReadOnly branch, which the gate precedes. The catalog admitted canonical bash; the gate then refused to dispatch it. Admit the bounded carve-out at the gate: allows_bounded_readonly_bash(name) plus the same agent_readonly_bash_input predicate execute enforces, so the gate can never widen past the execute-time refusal. Legacy `Bash` stays raw-shell-denied (exact-name match), mutating commands still refuse, and Planner keeps the same bounded admission. Tests: scout_posture_gate_admits_agent_readonly_bash_commands pins the gate↔classifier agreement with the exact dogfood command shapes and the negative controls (touch denied, legacy Bash denied). Suites green: subagent 524, shell 108, command_safety 64.
1 participant
Fixes #5426. Splits the read-only shell classifier so
ShellPolicy::ReadOnlyagents keep the #5356 bounded shell usable, without touching the parent's parallel auto-approve posture.is_agent_readonly_shell_command: pipelines (every segment validated), globs,git -C/--no-pager,find(mutating primaries denied),sed -n <range>p,npm view|show|info,sort/uniq/cut/tr/comm.head/tail -Nrecognized as-n N(digit-only, line-count commands only) — shared, still read-only.-C-aware splice); pipelines exec viaset -o pipefailover pre-validated segments; workspace operand confinement unchanged.Merge AFTER the v0.9.8 publish completes (release lane frozen at tag).