Skip to content

fix(cli): stop escalating read-only git commands in the sandbox - #14222

Merged
marius-kilocode merged 7 commits into
mainfrom
fix/sandbox-git-readonly-classifier
Sep 17, 2026
Merged

marius-kilocode merged 7 commits into
mainfrom
fix/sandbox-git-readonly-classifier

Conversation

@marius-kilocode

@marius-kilocode marius-kilocode commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

When the OS sandbox is enabled, the bash tool runs the git mutation classifier on every parsed command and forces an unsandboxed-run prompt on any match. The classifier treated several read-only forms as mutations, so users were prompted to leave the sandbox just to run git remote -v, git stash list, or git branch -v. This change teaches the classifier the read-only forms of remote, stash, branch, tag, config, worktree, reflog, ls-remote, and notes. Every writing form still mutates (git branch -d, git branch <name>, git stash push/pop/drop, git remote add/set-url, git config <key> <value>, git tag <name>, git worktree add), unknown subcommands still default to mutate, and git fetch, git pull, git push, and git clone stay mutating because they write refs or need network.

This classifier is a convenience layer that reduces noisy escalation prompts. It is not a security boundary: a misclassified command still runs inside the sandbox, and command execution stays governed by the normal bash permission rules. It is scheduled for replacement when generic escalation lands, so the per-subcommand read/write sets are intentionally temporary.

Newly non-mutating forms:

  • git remote, git remote -v, git remote --verbose, git remote show, git remote get-url
  • git stash list, git stash show
  • git branch list forms: -v, -vv, -a, -r, -l, --list, --all, --remotes, --verbose, --show-current, --contains, --merged, --no-merged, --points-at, --format, --sort, --column
  • git tag list forms: -l, -v, -n, --list, --contains, --points-at, --merged, --no-merged, --format, --sort, --column
  • git config --get, --get-all, --get-regexp, --list, and the other existing read flags
  • git worktree list
  • git reflog, git reflog show, git reflog exists
  • git notes, git notes list, git notes show, git notes get-ref
  • git ls-remote

Composed forms are handled too: mutating flags win over list flags (git branch -v -d feature, git tag -d v1 --column), option values are consumed (git branch foo -v still mutates; git notes --ref <name> list stays read-only), and short-flag clusters are expanded (git branch -av, git tag -ln5).

Manual test

Ran in an isolated dev-mode VS Code instance on macOS, with a disposable git workspace configured with sandbox.enabled: true and permission.bash: "allow" so the only prompt under test is the sandbox escalation.

  • Read-only: asked the agent to run git remote -v via the bash tool. It ran and returned (no output); no sandbox_escalation card appeared.
  • Mutating: asked the agent to run git branch sandbox-selftest. The sandbox_escalation card appeared before execution: "Allow Git operation outside the sandbox? / Create sandbox-selftest branch / git branch sandbox-selftest". Rejected it, and the branch was not created (git branch --list still showed only main).

Automated coverage: bun test ./test/kilocode/sandbox/git.test.ts (table-driven read/write cases), bun test ./test/kilocode/tool/shell-unparsed.test.ts (real shell scanner path), bun run typecheck, and bun run script/check-opencode-annotations.ts --worktree.

Comment thread packages/opencode/src/kilocode/sandbox/git.ts Outdated
Comment thread packages/opencode/src/kilocode/sandbox/git.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review of commit a06a36d7 (documenting the expand() short-flag helper). The only change is a three-line comment above expand() in packages/opencode/src/kilocode/sandbox/git.ts. The explanation is accurate: -avv expands to -a -v -v, -n5 is narrowed to -n by stripping the attached numeric value, and the code paths it describes match the current implementation. No behavioral change and no new issues in the changed lines.

Files Reviewed (1 files)
  • packages/opencode/src/kilocode/sandbox/git.ts
Previous Review Summaries (5 snapshots, latest commit 91aea98)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 91aea98)

Status: No Issues Found | Recommendation: Merge

Incremental review of commit 91aea98d27 (simplifying the expand() short-flag cluster helper). The change is behavior-preserving: Array.from("ln", (char) => -${char}) produces the same -l/-n tokens as the previous split("").map(...), and the /^-[A-Za-z]{2,}\d*$/ guard plus trailing-digit strip still expands git tag -ln5 to read-only -l/-n. The earlier masked-mutation (scan() checks write flags before read flags) and option-value (verb() skips NOTES_FLAGS) findings remain fixed. No new issues in the changed line.

Files Reviewed (1 files)
  • packages/opencode/src/kilocode/sandbox/git.ts

Previous review (commit 367a9cd)

Status: No Issues Found | Recommendation: Merge

Incremental review of commit 367a9cdb57 (expanding clustered git short flags with numeric suffixes). It resolves the previous finding: expand() now matches /^-[A-Za-z]{2,}\d*$/ and strips trailing digits, so git tag -ln5 expands to -l/-n and stays read-only. The earlier masked-mutation and option-value verb() findings are also fixed in current HEAD: scan() checks write flags before read flags, and verb() skips value-taking flags via NOTES_FLAGS. No new issues found in the changed lines.

Files Reviewed (2 files)
  • packages/opencode/src/kilocode/sandbox/git.ts
  • packages/opencode/test/kilocode/sandbox/git.test.ts

Previous review (commit 32aaae2)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1

Fix these issues in Kilo Cloud

Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/opencode/src/kilocode/sandbox/git.ts 156 expand() handles letter-only clusters (-avv) and single-letter-plus-digit flags (-n5) but not clustered short flags with a numeric suffix, so git tag -ln5 loses its read flag and escalates to an unsandboxed prompt again
Files Reviewed (2 files)
  • packages/opencode/src/kilocode/sandbox/git.ts - 1 issue
  • packages/opencode/test/kilocode/sandbox/git.test.ts - no issues

Previous review (commit 2da7e2b)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/src/kilocode/sandbox/git.ts
  • packages/opencode/test/kilocode/sandbox/git.test.ts

Previous review (commit f90dc84)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 1

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
packages/opencode/src/kilocode/sandbox/git.ts 118 .some() over the widened BRANCH_READ/TAG_READ sets lets a list flag mask a mutation flag, so git branch -v -d feature / git tag -v -d v1.0.0 now skip the sandbox-escalation prompt

SUGGESTION

File Line Issue
packages/opencode/src/kilocode/sandbox/git.ts 93 verb() treats an option value as the subcommand (git notes --ref refs/notes/x list escalates); git notes get-ref also over-escalates
Files Reviewed (4 files)
  • .changeset/sandbox-git-readonly-classifier.md - no issues
  • packages/opencode/src/kilocode/sandbox/git.ts - 2 issues
  • packages/opencode/test/kilocode/sandbox/git.test.ts - no issues
  • packages/opencode/test/kilocode/tool/shell-unparsed.test.ts - no issues

Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

Comment thread packages/opencode/src/kilocode/sandbox/git.ts Outdated
Comment thread packages/opencode/src/kilocode/sandbox/git.ts Outdated

@WebReflection WebReflection left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I think latest change makes sense, thanks!

@marius-kilocode

Copy link
Copy Markdown
Collaborator Author

Added a quick comment to make it clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants