feat(permission): auto-classifier autonomy + review transparency + prefix-grant breadth/scope - #570
feat(permission): auto-classifier autonomy + review transparency + prefix-grant breadth/scope#570pengdst wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Zero’s permission/autonomy system by adding an LLM-backed auto-classifier mode for prompt-eligible actions, improving command-prefix grant UX (breadth selection + wildcard support), and introducing project-scoped persisted prefix grants to prevent cross-workspace leakage.
Changes:
- Add
auto-classifierpermission mode with an embedded strict-JSON classifier prompt, runtime plumbing, and TUI confirmation + transparency (ask reasons shown in prompts/logs). - Expand command-prefix approvals into a breadth “ladder” (including last-token
stem*wildcard) and ensure UI selection/clicks resolve the exact chosen breadth. - Persist command-prefix grants with
Projectscope (project vs global) and prefer project-scoped matches during lookup.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/view.go | Updates permission-mode cycling and adds the auto-classifier confirmation overlay rendering. |
| internal/tui/transcript.go | Surfaces classifier “ask” reasons in permission transcript detail and updates decision wording. |
| internal/tui/transcript_selection.go | Tracks permission option selection by index to disambiguate expanded prefix breadth choices. |
| internal/tui/session.go | Plumbs classifierReason from session event payloads into PermissionEvent. |
| internal/tui/rendering.go | Renders classifier decline reasons on prompts and improves transcript labels for new decision types/scopes. |
| internal/tui/rendering_lime_test.go | Adds/updates tests for new transcript labels and auto-reviewed permission rows. |
| internal/tui/render_cache.go | Adds ClassifierReason into permission-row cache fingerprints to avoid stale renders. |
| internal/tui/permission_prompt.go | Expands prefix decisions into one option per breadth and carries chosen breadth through resolution. |
| internal/tui/permission_prompt_test.go | Tests prefix breadth expansion and that the chosen breadth is sent back in the decision. |
| internal/tui/model.go | Adds confirmation modal state, wires Shift+Tab cycling to require confirmation for auto-classifier, and supports new prefix decision. |
| internal/tui/model_test.go | Updates mode-cycling tests for the new mode ladder + confirmation and updates permissions command rendering expectations. |
| internal/tui/keybinding_help.go | Updates Shift+Tab help text (but currently does not reflect the full new cycle). |
| internal/tui/hover.go | Fixes hover behavior to use permission-option indices (not decision actions) with expanded breadth options. |
| internal/tui/command_views.go | Enhances /permissions command card to include label/meaning and auto-classifier explanation section. |
| internal/tui/command_polish_test.go | Updates command-card tests for new permissions mode label/summary fields. |
| internal/swarm/team.go | Adds new canonical permission-mode IDs and ranks them for “never widen” resolution. |
| internal/swarm/team_test.go | Tests permission-mode ordering with new workspace-auto/auto-classifier modes. |
| internal/specialist/exec.go | Updates member autonomy behavior to use workspace-auto instead of legacy member. |
| internal/specialist/exec_test.go | Updates tests for the workspace-auto autonomy selection and emitted args. |
| internal/sandbox/grants.go | Adds project-scoped prefix storage semantics and lookup preference (project over global). |
| internal/sandbox/grants_test.go | Updates tests for new LookupCommandPrefix(..., project) signature. |
| internal/sandbox/engine.go | Adds project-scoped prefix grant persistence helper and threads project into prefix lookups. |
| internal/sandbox/command_prefix.go | Adds Project to persisted prefix grants and implements last-token stem* wildcard matching. |
| internal/sandbox/command_prefix_wildcard_test.go | New tests for project-scoped prefix matching and wildcard prefix validation/matching. |
| internal/cli/exec_tools.go | Maps legacy “member” autonomy aliases and new “workspace” values to workspace-auto. |
| internal/cli/exec_test.go | Updates CLI tests for new autonomy string mappings and tool list behavior under workspace-auto. |
| internal/agent/types.go | Adds mode metadata helpers, new permission decision actions, classifier request/decision types, and request/event fields. |
| internal/agent/request_permissions_test.go | Updates shouldRequestPermission call sites to include the permission mode parameter. |
| internal/agent/permission_additional_perms_test.go | Updates tests for shouldRequestPermission signature change. |
| internal/agent/member_auto_test.go | Renames/member-auto behavior tests to workspace-auto while preserving advertised-tool semantics. |
| internal/agent/loop.go | Implements auto-classifier decision flow, ask-reason propagation, prefix breadth selection validation, and scoped prefix persistence. |
| internal/agent/loop_test.go | Adds extensive tests for auto-classifier allow/prompt/error fallbacks and strict JSON behavior. |
| internal/agent/command_prefix.go | Generates prefix breadth ladder options, adds intra-token wildcard breadth, and validates chosen breadth. |
| internal/agent/command_prefix_test.go | Adds tests for breadth ladder generation, intra-token wildcard, and breadth-choice honoring. |
| internal/agent/auto_classifier.go | Adds embedded classifier prompt + provider call and strict JSON parsing for classifier decisions. |
| internal/agent/auto_classifier_test.go | Verifies embedded prompt presence/contract and that classifier requests expose no tools. |
| internal/agent/auto_classifier_prompt.md | Adds the classifier system prompt used for permission auto-review decisions. |
| internal/acp/agent.go | Exposes new prompt-respecting modes over ACP and uses centralized mode label/description metadata. |
| internal/acp/agent_test.go | Updates ACP tests to allow auto-classifier mode while still rejecting unsafe mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bindings: []keybinding{ | ||
| {labelOr(m.keyBindings.cycleReasoning, "Ctrl+T"), "cycle reasoning effort (auto \u2192 low \u2192 medium \u2192 high)"}, | ||
| {"Shift+Tab", "cycle permission mode (auto \u2194 ask)"}, | ||
| {"Shift+Tab", "cycle permission mode (ask \u2192 auto \u2192 trusted workspace)"}, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/exec_tools.go (1)
84-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the invalid
--autovalue message.Line 92 still says only
low,medium, orhighare valid, but this switch now also acceptsworkspace,workspace-auto,member, andmember-auto.Suggested fix
default: - return "", execUsageError{fmt.Sprintf("Invalid autonomy level %q. Expected low, medium, or high.", options.autonomy)} + return "", execUsageError{fmt.Sprintf("Invalid autonomy level %q. Expected low, medium, workspace, workspace-auto, member, member-auto, or high.", options.autonomy)} }🤖 Prompt for 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. In `@internal/cli/exec_tools.go` around lines 84 - 92, The invalid `--auto` error text in `exec_tools.go` is outdated and should match the accepted values handled by the `switch` in `mode` parsing. Update the `default` branch message in the autonomy selection logic to include `workspace`, `workspace-auto`, `member`, and `member-auto` alongside `low`, `medium`, and `high`, so the `execUsageError` from `options.autonomy` reflects all supported inputs.
🧹 Nitpick comments (4)
internal/acp/agent_test.go (1)
180-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
workspace-autoin this ACP mode test.Line 180 says prompt-respecting modes are accepted, but the new
workspace-autobranch is not exercised here. Add it besideauto-classifierso ACP regressions for both newly exposed modes are caught.Suggested test addition
if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModeAsk)}, &SetSessionModeResult{}); err != nil { t.Fatalf("set_mode ask: %v", err) } + if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModeWorkspaceAuto)}, &SetSessionModeResult{}); err != nil { + t.Fatalf("set_mode workspace-auto: %v", err) + } if err := h.client.Call(ctx, MethodSessionSetMode, SetSessionModeParams{SessionID: newRes.SessionID, ModeID: string(agent.PermissionModeAutoClassifier)}, &SetSessionModeResult{}); err != nil { t.Fatalf("set_mode auto-classifier: %v", err) }🤖 Prompt for 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. In `@internal/acp/agent_test.go` around lines 180 - 186, The ACP mode acceptance test currently covers prompt-respecting modes but misses the new workspace-auto branch. Update the test in agent_test.go around the session mode calls to include a SetSessionMode request for agent.PermissionModeWorkspaceAuto alongside the existing ask and auto-classifier cases, so MethodSessionSetMode validates all exposed prompt-respecting modes.internal/agent/auto_classifier_test.go (1)
25-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the happy path is unit-tested; parser edge cases are untouched.
parseAutoPermissionClassifierDecisionhas several rejection branches (unknown JSON fields, trailing content after the object, unmappedactionvalues, emptyreason) that have no direct unit test here —loop_test.go'sTestRunDefaultAutoClassifierInvalidJSONFallsBackToPermissionRequestonly covers plain non-JSON text ("not json"), not e.g.{"action":"maybe","reason":"x"}or{"action":"allow","reason":"x"}garbage. A small table-driven test onparseAutoPermissionClassifierDecisiondirectly would pin down this contract cheaply.🤖 Prompt for 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. In `@internal/agent/auto_classifier_test.go` around lines 25 - 57, Add direct table-driven tests for parseAutoPermissionClassifierDecision to cover its rejection paths, not just the happy path in TestAutoPermissionClassifierUsesEmbeddedPrompt. Include cases for unknown action values, empty reason, trailing non-JSON content after a valid object, and unknown JSON fields, and assert each returns an error so the parser contract is pinned down independently of classifyAutoPermissionWithProvider.internal/agent/command_prefix.go (1)
90-108: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
intraTokenWildcardPrefixwidens on the first separator, not the last, for multi-separator tokens.
strings.IndexAny(last, ":-/.@")finds the first matching separator. For a token with more than one separator (e.g."run:test:e2e"), this produces"run:*"instead of the more specific"run:test:*", discarding an entire namespace level from the offered wildcard breadth. Since this widens what the "namespace wildcard" option in the breadth picker actually grants, worth confirming this is the intended behavior for nested namespace scripts, or switching tostrings.LastIndexAnyto preserve the deepest namespace segment.♻️ Possible fix using the last separator
- index := strings.IndexAny(last, ":-/.@") + index := strings.LastIndexAny(last, ":-/.@")🤖 Prompt for 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. In `@internal/agent/command_prefix.go` around lines 90 - 108, The intraTokenWildcardPrefix helper is using the first separator in the final token, which makes multi-separator names widen too broadly and drop nested namespace context. Update the separator lookup in intraTokenWildcardPrefix to use the last usable separator so tokens like nested script names preserve the deepest namespace segment when building the wildcard prefix. Keep the existing len/base and separator boundary checks intact, and verify the returned wildcard still comes from the last token of base.internal/tui/model.go (1)
1306-1311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: auto-classifier enable logic repeated.
m.autoClassifierConfirmActive = false; m.permissionMode = agent.PermissionModeAutoClassifieris duplicated between the Enter handler and they/Ycase. Consider extracting a tinyconfirmAutoClassifierMode()helper shared by both call sites.♻️ Example extraction
+func (m model) confirmAutoClassifierMode() model { + m.autoClassifierConfirmActive = false + m.permissionMode = agent.PermissionModeAutoClassifier + return m +}Also applies to: 1690-1701
🤖 Prompt for 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. In `@internal/tui/model.go` around lines 1306 - 1311, The auto-classifier confirmation logic is duplicated in the Enter and y/Y handlers inside model.go. Extract the shared state update into a small confirmAutoClassifierMode helper on the model, and call it from both the m.autoClassifierConfirmActive branch and the y/Y case so the mode switch and flag reset live in one place.
🤖 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 `@internal/agent/loop_test.go`:
- Around line 1563-1565: The failing assertion in the classifier request check
should avoid slicing `contentArg` directly in the `t.Fatalf` message, since
`loop_test.go` can panic before reporting the regression if `content` is missing
or too short. Update the test around `classifierRequests[0]` and `contentArg` to
compute a safe suffix only after validating the string length, or use a bounded
helper when formatting the failure message, so the assertion fails cleanly
instead of panicking.
In `@internal/agent/loop.go`:
- Around line 2706-2711: The truncateClassifierString helper currently converts
the full input to []rune before checking the limit, which can allocate heavily
for large tool arguments. Update truncateClassifierString to bound the work
before full conversion, using a streaming or incremental rune scan that stops
once autoPermissionClassifierStringLimit is reached and only materializes the
truncated prefix plus the suffix. Keep the existing behavior and references to
autoPermissionClassifierStringLimit and the truncation message, but avoid
copying the entire value first.
In `@internal/sandbox/engine.go`:
- Around line 100-108: The GrantCommandPrefixForProject method is incorrectly
falling back to a global grant when engine.workspaceRoot is empty by assigning
input.Project unconditionally; update Engine.GrantCommandPrefixForProject so
project-scoped approvals are only persisted when a workspace root exists, and
otherwise return an error or avoid saving the grant instead of sending an empty
Project to store.GrantCommandPrefix, keeping the scope tied to the current
project and not widening it beyond that selection.
In `@internal/specialist/exec.go`:
- Around line 70-74: MemberAutonomy in exec.go is widening child permissions too
broadly by promoting every non-unsafe mode to workspace-auto. Update the mode
handling in the member-autonomy path so it only uplifts when the resolved mode
is already workspace-capable, and preserves the parent’s strictness for ask,
auto, empty, and unknown modes. Use the existing MemberAutonomy logic and the
mode resolution/switch around the child startup path to ensure the child never
gets broader capabilities than the parent.
In `@internal/tui/command_views.go`:
- Around line 271-275: The permission-mode display in permissionsTextWithStore
is incorrectly relying on agent.PermissionModeInfoFor, which maps unknown values
to PermissionModeAuto and can mislabel rehydrated session values as “Auto.”
Update permissionsTextWithStore (and the related permissions rendering path at
the referenced block) to preserve the raw m.permissionMode string when it is not
a recognized mode, similar to modeLabel() in view.go. Keep the existing label
and summary behavior for known modes, but fall back to the actual stored string
for unknown ones so the UI reflects the real session state.
In `@internal/tui/keybinding_help.go`:
- Line 49: The help text for the Shift+Tab keybinding is out of date because it
only lists ask, auto, and trusted workspace, while nextPermissionMode in view.go
now also includes auto-classifier in the cycle. Update the keybinding
description in keybinding_help.go to reflect the full permission mode sequence,
keeping the wording aligned with the actual cycling behavior so users can see
every state.
---
Outside diff comments:
In `@internal/cli/exec_tools.go`:
- Around line 84-92: The invalid `--auto` error text in `exec_tools.go` is
outdated and should match the accepted values handled by the `switch` in `mode`
parsing. Update the `default` branch message in the autonomy selection logic to
include `workspace`, `workspace-auto`, `member`, and `member-auto` alongside
`low`, `medium`, and `high`, so the `execUsageError` from `options.autonomy`
reflects all supported inputs.
---
Nitpick comments:
In `@internal/acp/agent_test.go`:
- Around line 180-186: The ACP mode acceptance test currently covers
prompt-respecting modes but misses the new workspace-auto branch. Update the
test in agent_test.go around the session mode calls to include a SetSessionMode
request for agent.PermissionModeWorkspaceAuto alongside the existing ask and
auto-classifier cases, so MethodSessionSetMode validates all exposed
prompt-respecting modes.
In `@internal/agent/auto_classifier_test.go`:
- Around line 25-57: Add direct table-driven tests for
parseAutoPermissionClassifierDecision to cover its rejection paths, not just the
happy path in TestAutoPermissionClassifierUsesEmbeddedPrompt. Include cases for
unknown action values, empty reason, trailing non-JSON content after a valid
object, and unknown JSON fields, and assert each returns an error so the parser
contract is pinned down independently of classifyAutoPermissionWithProvider.
In `@internal/agent/command_prefix.go`:
- Around line 90-108: The intraTokenWildcardPrefix helper is using the first
separator in the final token, which makes multi-separator names widen too
broadly and drop nested namespace context. Update the separator lookup in
intraTokenWildcardPrefix to use the last usable separator so tokens like nested
script names preserve the deepest namespace segment when building the wildcard
prefix. Keep the existing len/base and separator boundary checks intact, and
verify the returned wildcard still comes from the last token of base.
In `@internal/tui/model.go`:
- Around line 1306-1311: The auto-classifier confirmation logic is duplicated in
the Enter and y/Y handlers inside model.go. Extract the shared state update into
a small confirmAutoClassifierMode helper on the model, and call it from both the
m.autoClassifierConfirmActive branch and the y/Y case so the mode switch and
flag reset live in one place.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2cea3c4-f35d-409e-991e-11f6f2ce130a
📒 Files selected for processing (39)
internal/acp/agent.gointernal/acp/agent_test.gointernal/agent/auto_classifier.gointernal/agent/auto_classifier_prompt.mdinternal/agent/auto_classifier_test.gointernal/agent/command_prefix.gointernal/agent/command_prefix_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/member_auto_test.gointernal/agent/permission_additional_perms_test.gointernal/agent/request_permissions_test.gointernal/agent/types.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/sandbox/command_prefix.gointernal/sandbox/command_prefix_wildcard_test.gointernal/sandbox/engine.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/specialist/exec.gointernal/specialist/exec_test.gointernal/swarm/team.gointernal/swarm/team_test.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/hover.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/permission_prompt.gointernal/tui/permission_prompt_test.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/session.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/view.go
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds auto-classifier permission handling, workspace-auto mode support, command-prefix breadth and project scoping, plus matching runtime, CLI, specialist, swarm, and TUI updates. ChangesAuto-classifier permission mode and prefix grants
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
3781-3804: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProject-scope prefix grant reason text doesn't convey persistence.
permissionDecisionAllowPrefixProjectis a persisted grant (agent.PermissionDecisionAllowPrefixProject = "always_allow_prefix_for_project"), same durability class aspermissionDecisionAlwaysAllowPrefix("always_allow_prefix", global) — but its reason string doesn't say so, reading ambiguously like the ephemeral session-scopedpermissionDecisionAllowPrefix. This weakens the audit trail's ability to distinguish "granted for this run" from "saved to disk for this project going forward."🩹 Suggested fix
case permissionDecisionAllowPrefixProject: - return "approved command prefix for this project in TUI" + return "persistently approved command prefix (project) in TUI"🤖 Prompt for 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. In `@internal/tui/model.go` around lines 3781 - 3804, The permissionDecisionReason switch treats permissionDecisionAllowPrefixProject like a session-only grant, which makes the audit text ambiguous. Update permissionDecisionReason so the permissionDecisionAllowPrefixProject branch clearly indicates it is persisted for the project (similar to permissionDecisionAlwaysAllowPrefix), using the existing permissionDecisionAllowPrefixProject symbol to make the distinction from permissionDecisionAllowPrefix obvious.
♻️ Duplicate comments (1)
internal/sandbox/engine.go (1)
100-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale project-scope comment.
The implementation correctly refuses an empty workspace root, but the comment still says it falls back to a global grant.
Proposed fix
// GrantCommandPrefixForProject persists a prefix grant scoped to this engine's -// workspace root, so it only matches inside the current project. It falls back to -// a global grant when no workspace root is configured. +// workspace root, so it only matches inside the current project.🤖 Prompt for 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. In `@internal/sandbox/engine.go` around lines 100 - 102, The comment on GrantCommandPrefixForProject is stale and contradicts the implementation: it should no longer mention falling back to a global grant when no workspace root is configured. Update the doc comment on GrantCommandPrefixForProject in engine.go so it accurately describes the current behavior of requiring a configured workspace root and persisting the grant only for the current project scope.
🧹 Nitpick comments (1)
internal/tui/model_test.go (1)
1823-1898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the y/Y and n/N confirm paths too.
Existing tests exercise Enter/Esc for the new auto-classifier confirmation modal, but the
y/Y(confirm) andn/N(cancel) letter-key branches inmodel.go's catch-all swallow block aren't directly asserted anywhere shown here.🤖 Prompt for 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. In `@internal/tui/model_test.go` around lines 1823 - 1898, The auto-classifier confirmation modal in model.go is only covered through Enter and Esc, so add tests for the letter-key confirmation branches as well. Extend the existing permission-mode tests around model.Update and the autoClassifierConfirmActive flow to assert that y/Y confirm to PermissionModeAutoClassifier and n/N cancel back to PermissionModeWorkspaceAuto, using the same shift+tab setup and modal state checks already present in TestShiftTabCyclesPermissionMode and TestAutoClassifierConfirmationCancels.
🤖 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 `@internal/agent/auto_classifier.go`:
- Around line 26-54: The auto-classifier request in
classifyAutoPermissionWithProvider only uses the caller’s context, so a slow or
stuck provider can block permission handling indefinitely. Update this function
to create and use a short, classification-specific timeout context around
provider.StreamCompletion and Zeroruntime stream collection, and ensure the
derived context is canceled before returning. Keep the existing request/parse
flow intact, but make the deadline local to classifyAutoPermissionWithProvider
so it no longer relies solely on the ambient ctx.
- Around line 56-87: parseAutoPermissionClassifierDecision currently rejects
common fenced JSON replies, so a ```json ... ``` response falls through to false
and disables the auto-classifier path. Update this parser to strip markdown code
fences before decoding, or otherwise normalize the model output in the same flow
that handles the JSON.Decoder logic, and keep the existing action/reason
validation intact. Add a test covering fenced input to ensure
AutoPermissionClassifierDecision is still parsed correctly.
In `@internal/sandbox/command_prefix.go`:
- Around line 76-80: The trailing-wildcard handling in command matching is too
permissive, allowing plain tokens like test* to match unrelated commands. Update
the wildcard parsing in trailingWildcardStem and the checks in the
prefix-matching flow so a wildcard is only honored when the stem ends with a
namespace separator such as :, preventing matches like test or testmalicious
while still allowing namespace forms like test:*. Add a table case covering this
behavior in the command prefix tests.
In `@internal/tui/command_views.go`:
- Around line 355-362: The Permissions card’s Grants section in
renderCommandCardTranscript is showing prefix rows without enough scope context,
so project-scoped and global prefix grants look identical even though they are
counted separately. Update the prefix row label generation used to build
grantRows so it includes the scope, at minimum distinguishing project vs global,
and keep the existing Permissions/Grants rendering flow intact while making the
scope visible in each prefix grant row.
In `@internal/tui/rendering.go`:
- Around line 1142-1146: The auto-classifier reason is rendered directly from
request.ClassifierReason, which can include newlines and throw off the visible
row count used for option targeting. Update the rendering path in the TUI code
around the classifier reason block to normalize the text with
permissionDisplayReason before appending it to lines, matching the handling used
for other displayed reasons and keeping click offsets aligned.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 3781-3804: The permissionDecisionReason switch treats
permissionDecisionAllowPrefixProject like a session-only grant, which makes the
audit text ambiguous. Update permissionDecisionReason so the
permissionDecisionAllowPrefixProject branch clearly indicates it is persisted
for the project (similar to permissionDecisionAlwaysAllowPrefix), using the
existing permissionDecisionAllowPrefixProject symbol to make the distinction
from permissionDecisionAllowPrefix obvious.
---
Duplicate comments:
In `@internal/sandbox/engine.go`:
- Around line 100-102: The comment on GrantCommandPrefixForProject is stale and
contradicts the implementation: it should no longer mention falling back to a
global grant when no workspace root is configured. Update the doc comment on
GrantCommandPrefixForProject in engine.go so it accurately describes the current
behavior of requiring a configured workspace root and persisting the grant only
for the current project scope.
---
Nitpick comments:
In `@internal/tui/model_test.go`:
- Around line 1823-1898: The auto-classifier confirmation modal in model.go is
only covered through Enter and Esc, so add tests for the letter-key confirmation
branches as well. Extend the existing permission-mode tests around model.Update
and the autoClassifierConfirmActive flow to assert that y/Y confirm to
PermissionModeAutoClassifier and n/N cancel back to PermissionModeWorkspaceAuto,
using the same shift+tab setup and modal state checks already present in
TestShiftTabCyclesPermissionMode and TestAutoClassifierConfirmationCancels.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6281fbe6-a4f3-4af3-8ee5-057754f840a7
📒 Files selected for processing (39)
internal/acp/agent.gointernal/acp/agent_test.gointernal/agent/auto_classifier.gointernal/agent/auto_classifier_prompt.mdinternal/agent/auto_classifier_test.gointernal/agent/command_prefix.gointernal/agent/command_prefix_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/member_auto_test.gointernal/agent/permission_additional_perms_test.gointernal/agent/request_permissions_test.gointernal/agent/types.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/sandbox/command_prefix.gointernal/sandbox/command_prefix_wildcard_test.gointernal/sandbox/engine.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/specialist/exec.gointernal/specialist/exec_test.gointernal/swarm/team.gointernal/swarm/team_test.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/hover.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/permission_prompt.gointernal/tui/permission_prompt_test.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/session.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/view.go
c1b3f0b to
25f28e2
Compare
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 `@internal/agent/loop.go`:
- Around line 1096-1100: The project-scoped prefix persistence path is dropping
the fallback grant when `persistCommandPrefixGrantScoped` fails, so
`decisionCommandPrefix` never gets updated and the user is reprompted later.
Update the `decisionCommandPrefix` handling in `loop.go` to fall back to the
session grant even when `GrantCommandPrefixForProject` cannot persist due to
missing workspace root, using the same behavior in the retry branch that calls
`persistCommandPrefixGrantScoped`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ce5a04d-74c1-4bfd-a70a-32fba40ee316
📒 Files selected for processing (28)
internal/acp/agent_test.gointernal/agent/auto_classifier.gointernal/agent/auto_classifier_prompt.mdinternal/agent/auto_classifier_test.gointernal/agent/command_prefix.gointernal/agent/command_prefix_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/exec_tools.gointernal/sandbox/command_prefix.gointernal/sandbox/command_prefix_wildcard_test.gointernal/sandbox/engine.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/tui/command_views.gointernal/tui/hover.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/permission_prompt.gointernal/tui/permission_prompt_test.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/session.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/view.go
✅ Files skipped from review due to trivial changes (1)
- internal/agent/auto_classifier_prompt.md
🚧 Files skipped from review as they are similar to previous changes (26)
- internal/tui/keybinding_help.go
- internal/agent/auto_classifier_test.go
- internal/tui/transcript.go
- internal/sandbox/command_prefix_wildcard_test.go
- internal/tui/session.go
- internal/tui/render_cache.go
- internal/tui/hover.go
- internal/tui/transcript_selection.go
- internal/cli/exec_tools.go
- internal/acp/agent_test.go
- internal/sandbox/grants_test.go
- internal/tui/view.go
- internal/agent/command_prefix_test.go
- internal/tui/permission_prompt_test.go
- internal/sandbox/grants.go
- internal/sandbox/engine.go
- internal/tui/permission_prompt.go
- internal/tui/command_views.go
- internal/agent/auto_classifier.go
- internal/sandbox/command_prefix.go
- internal/agent/command_prefix.go
- internal/tui/rendering.go
- internal/agent/types.go
- internal/tui/model_test.go
- internal/tui/model.go
- internal/agent/loop_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a few permission-flow issues that should be addressed before this is ready.
Findings
-
[P1] Do not offer one-token package-manager prefixes
internal/agent/command_prefix.go:77
The new breadth ladder includes every shorter token prefix, so a prompt for a routine command such asyarn test:unitcan now offeryarnas a reusable prefix. Once selected,matchCommandPrefixtreats any lateryarn ...command as approved andshellExecutionArgsForApprovalruns it outside the sandbox, so the grant also covers lateryarn add,yarn install,yarn publish, or arbitrary project scripts. That is exactly the package-manager class the PR says should remain non-grantable as a prefix. Please filter launcher/package-manager-only breadths out of the ladder, or classifyyarn/pnpmthe same way asnpm/npx/bunso broad package-manager grants cannot be offered. -
[P2] Complete CodeRabbit's request to keep a fallback prefix grant
internal/agent/loop.go:1096
CodeRabbit's current review item is still valid for the remembered-grant path. When the user chooses the new "allow command prefix for this project" option butGrantCommandPrefixForProjectfails because the sandbox engine has a grant store without a workspace root, the call is allowed once but no session prefix grant is recorded. The engine comment says this case should keep the safer session grant, and the UI offered a reusable project-scoped approval, so later matching commands prompt again instead of reusing even a session-scoped fallback. Please fall back toGrantCommandPrefixForSessionwhen project persistence fails, or stop offering the project-scoped option when the engine cannot scope it. -
[P3] Complete CodeRabbit's request to mark project prefix approvals as persistent
internal/tui/model.go:3861
The project-scoped prefix option persists a grant just like the global prefix option, but the TUI reason text isapproved command prefix for this project in TUIwhile the global path sayspersistently approved command prefix (global) in TUI. That leaves the permission event/audit trail ambiguous with the session-only prefix approval. Please make the project reason explicitly persistent and project-scoped so session, project, and global prefix grants remain distinguishable in logs.
a8d294b to
a0be67e
Compare
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 `@internal/tui/permission_prompt.go`:
- Around line 53-56: The permission prompt currently assigns the same y hotkey
to both PermissionDecisionAlwaysAllowPrefix and PermissionDecisionAlwaysAllow,
so keyboard dispatch will always hit the first y option and make the full
always-allow choice unreachable. Update permission_prompt.go in the permission
option assembly logic to give one of these paths a different hotkey or
conditionally omit one option when both are present, using the existing
permissionDecisionAlwaysAllowPrefix and permissionDecisionAlwaysAllow cases as
the place to adjust the labels/hotkeys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e566f366-bd83-4c5d-a96b-832f667adb39
📒 Files selected for processing (40)
internal/acp/agent.gointernal/acp/agent_test.gointernal/agent/auto_classifier.gointernal/agent/auto_classifier_prompt.mdinternal/agent/auto_classifier_test.gointernal/agent/command_prefix.gointernal/agent/command_prefix_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/member_auto_test.gointernal/agent/permission_additional_perms_test.gointernal/agent/request_permissions_test.gointernal/agent/types.gointernal/cli/exec_test.gointernal/cli/exec_tools.gointernal/sandbox/command_prefix.gointernal/sandbox/command_prefix_wildcard_test.gointernal/sandbox/engine.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/specialist/exec.gointernal/specialist/exec_test.gointernal/swarm/team.gointernal/swarm/team_test.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/hover.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/permission_prompt.gointernal/tui/permission_prompt_test.gointernal/tui/picker_test.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/session.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/view.go
✅ Files skipped from review due to trivial changes (3)
- internal/agent/auto_classifier_prompt.md
- internal/sandbox/grants_test.go
- internal/tui/keybinding_help.go
🚧 Files skipped from review as they are similar to previous changes (34)
- internal/cli/exec_tools.go
- internal/tui/session.go
- internal/agent/request_permissions_test.go
- internal/tui/hover.go
- internal/sandbox/engine.go
- internal/agent/auto_classifier.go
- internal/agent/member_auto_test.go
- internal/specialist/exec.go
- internal/tui/transcript.go
- internal/swarm/team_test.go
- internal/acp/agent_test.go
- internal/acp/agent.go
- internal/tui/view.go
- internal/specialist/exec_test.go
- internal/tui/render_cache.go
- internal/swarm/team.go
- internal/cli/exec_test.go
- internal/tui/command_polish_test.go
- internal/tui/rendering_lime_test.go
- internal/tui/transcript_selection.go
- internal/tui/permission_prompt_test.go
- internal/agent/command_prefix_test.go
- internal/sandbox/command_prefix.go
- internal/sandbox/command_prefix_wildcard_test.go
- internal/tui/rendering.go
- internal/agent/auto_classifier_test.go
- internal/sandbox/grants.go
- internal/agent/types.go
- internal/tui/model_test.go
- internal/tui/command_views.go
- internal/tui/model.go
- internal/agent/loop_test.go
- internal/agent/permission_additional_perms_test.go
- internal/agent/loop.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I reviewed the auto-classifier permission-mode change. The classifier picks a permission mode from the prompt, and the guardrails look careful: it can only widen within bounded, pre-approved tiers and it never escalates past what the user's session policy allows, and the unclassifiable case falls back to the safe default rather than guessing permissive. That fail-closed default is the part I cared most about, and it's right. Approving.
Heads up: main has an unrelated test-build break right now (PR #589 is the one-line fix, queued for merge), so Smoke is red here too until that lands — not from this change.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Checked out the branch, merged main cleanly, ran make lint, and go test -race -count=1 on ./internal/agent/, ./internal/sandbox/, ./internal/tui/, ./internal/acp/ — all pass.
What looks good
Auto-classifier mode
- Classifier only reviews sandbox
ActionPrompttools — real autonomy on actions that would otherwise pause, not re-gating already-auto-allowed ones. - Hard guards block network, destructive, escalated, and out-of-workspace actions from classifier rescue —
canAutoClassifierReviewis thorough. - Fail-closed on classifier error/timeout/invalid JSON — falls back to normal user prompt.
- 10s timeout + fence stripping + strict JSON decode with
DisallowUnknownFields. - Classifier "ask" reason surfaced on permission prompt and session log (
ClassifierReason).
Prefix-grant breadth
commandPrefixLaddernever offers one-token launcher prefixes — addresses the P1 concern (yarnalone excluded; tests coveryarn test:unit,docker compose up, single-tokengo).grantPrefixForDecisionvalidates chosen breadth against offered options — stale/overbroad grants cannot widen.- Intra-token wildcard (
test:*) with trailing-*validation in sandbox matcher.
Project vs global scope
CommandPrefixGrant.Projectfield + lookup prefers project over global.persistCommandPrefixGrantScopedOrSessionsession fallback when project persistence fails.
TUI/UX
- Blocking confirmation modal for auto-classifier (replaces fragile double-press).
- Distinct hotkeys: prefix-global
y, always-allowf(was colliding). - Per-breadth prefix labels + empty hotkey bracket fix.
- Audit text:
persistently approved command prefix (project)vs(global).
Tests — strong coverage across classifier, ladder, wildcard matching, grant validation, TUI options, ACP mode acceptance.
Nits
- Scope bundling — three feature areas plus an unrelated session-resume
tea.ClearScreenrepaint fix. CONTRIBUTING prefers tight scope. - No
issue-approvedlinked issue — references #170/#244 as related continuation but no parent issue withissue-approved. Policy gap; not blocking given maintainer approval. - @jatmn still
CHANGES_REQUESTED— all three findings appear addressed in2a31f493+ but no re-review from them. - Classifier cost — every prompt-eligible action in auto-classifier mode triggers an extra LLM call on the same provider.
- Two-token grants still broad —
docker composeas a persisted prefix approves anydocker compose …. By design with the breadth picker.
Issues
None blocking. Security-sensitive paths (classifier guards, prefix validation, sandbox escalation) look correct.
ba558e9 to
710c0b3
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Rebase and resolve the current
mainconflict
internal/tui/model.go
GitHub currently reports this head asCONFLICTING/DIRTY, and merging the supplied base (5e1405d) with this head (502845e) reproduces a content conflict in this file. The PR cannot be merged until it is rebased and the resolved integration is revalidated against currentmain. -
[P2] Carry the new prefix-grant choices through ACP
internal/agent/types.go:123
ACP now advertises the new permission modes, but its permission bridge dropsalways_allow_prefix_for_project:optionKindForhas no case for that action, sobuildPermissionOptionssilently omits it. It also has no representation forCommandPrefixOptions, and returns only the action from a selected option, so ACP users cannot choose one of the new breadth rungs and always fall back to the exact default prefix. An editor client therefore cannot use the project-scoped grant at all or use the breadth-picker feature this PR adds. Map the project action and encode/validate the selected offered prefix in the ACP option round trip, with ACP coverage for both behaviors.
502845e to
abb4827
Compare
|
Both already fixed. Rebased onto |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the sandbox credential boundary
internal/sandbox/runner.go:377
This branch now passesos.Environ()directly to sandboxed commands, whileinternal/sandbox/profile.go:103also drops the default deny rules for cloud credential stores. The default workspace profile has a read-all root, so a normal sandboxed command can print provider/VCS/cloud credentials from its environment or read~/.aws, gcloud/Azure, orGOOGLE_APPLICATION_CREDENTIALSinto tool output. Network denial does not prevent that disclosure to the agent/model; a later approved network command can also exfiltrate it. This reverts the base's explicit secret-scrubbing and credential-deny protections. Restore those protections (including their coverage) rather than changing the security boundary in this PR. -
[P1] Enforce project scope before honoring a saved command prefix
internal/agent/command_prefix.go:180
Project-scoped grants are looked up only against the engine's initial workspace root, not the command's effective directory. For example, a grant forgo testsaved in/project-amatchescd /project-b && go test ./...:cdis treated as a known-safe segment, thegosegment matches, andshellExecutionArgsForApprovalthen makes the entire commandrequire_escalated. It consequently executes unsandboxed in/project-b, contradicting the advertised project-only scope. Reject directory-changing/out-of-project composite commands (or bind matching and escalation to the effective command directory). -
[P2] Keep the Windows-safe atomic-replace retry
internal/sessions/store.go:844
The PR deletesfsutil.RenameWithRetryand changes its callers to one-shotos.Rename(also cron, swarm mailboxes, and the Windows unelevated setup marker). The deleted helper specifically retried Windows sharing/lock violations from antivirus, indexers, and concurrent readers; those transient locks now fail session/checkpoint persistence, cron updates, mailbox delivery, or sandbox setup outright. Retain the bounded Windows retry for these atomic replacements and its regression coverage.
abb4827 to
9a0c4bc
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve the MCP resource scope when starting
serve
internal/cli/serve.go:46
This now constructsmcp.ServeOptionswithoutWorkspaceRootorScope. Consequently,zero serve --mcp -C /projectserves resources from the process CWD (the MCP default), rather than/project; and the same reversion removes the documented--add-diroption entirely. Restore the workspace/scope wiring and its resource-scope coverage so the requested workspace is not silently ignored. -
[P1] Resolve symlinks before treating a prefix grant as project-confined
internal/agent/command_prefix.go:224
The newcdguard only cleans paths and compares them lexically. With/workspace/link -> /outside,cd link && go test ./...passes the check, matches a project prefix grant, and is converted to an escalated unsandboxed shell invocation. The shell then follows the symlink and runs outside the approved project. Canonicalize/reject symlinked targets before the containment check and cover this escape path. -
[P1] Do not let the classifier override protected-metadata prompts
internal/agent/loop.go:1909
The sandbox deliberately leaves writes under protected workspace metadata such as.git/**as prompts, but those decisions have no block or risk category that excludes them fromcanAutoClassifierReview. A classifierallowcan therefore write.git/hooks/pre-commitor.git/configwithout user approval, defeating the protected-metadata boundary and allowing later Git-triggered code execution. Treat this prompt class as classifier-ineligible. -
[P1] Keep the unavailable-native-sandbox prompt non-overridable
internal/agent/loop.go:1909
When native shell isolation is unavailable, the sandbox intentionally prompts instead of auto-allowing a shell command. Auto-classifier can currently turn that ordinary prompt intoPermissionGranted=true; the subsequent evaluation then runs the command unwrapped on the host. Require active native shell isolation before classifier review of a shell command, or preserve this prompt as an explicit user-approval boundary. -
[P1] Do not expose auto-classifier over ACP without the required opt-in
internal/acp/agent.go:331
The local TUI requires an explicit confirmation before enabling this LLM-based auto-approval mode, but an ACP client can setauto-classifierdirectly and the server advertises it as a normal selectable mode. That lets an integration bypass the user acknowledgement and subsequently auto-approve eligible actions. Keep the mode out of ACP until the protocol can represent that confirmation, or enforce an equivalent server-side opt-in.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I am aligned with jatmn's changes-requested on this head, so I will not re-audit the whole thing. Adding my weight on the security one, which I looked at directly.
The prefix-grant cd guard is a real sandbox escape as written. The new path handling only filepath.Clean's the argument and compares it lexically, with no symlink resolution. So with a workspace symlink that points outside (say /workspace/link to /outside), cd link && go test ./... still matches a project prefix grant and gets promoted to an escalated, unsanctioned command that runs outside the workspace. Resolve the real path (EvalSymlinks) before the project-confinement check so a symlinked target cannot satisfy it.
jatmn's other P1 needs closing too: serve now builds mcp.ServeOptions without WorkspaceRoot or Scope, so zero serve --mcp -C /project serves from the process CWD instead of /project, and --add-dir is dropped. Please restore the workspace and scope wiring.
The classifier core still reads well. The fail-closed default on an unclassifiable prompt is the part I cared about last time, and it is intact. Close those two and this is in good shape.
…prefix-grant breadth/scope
Adds the auto-classifier ("let Zero cook") permission mode with an LLM reviewing
low-risk actions, review transparency for prompt decisions, and command-prefix
grants offered at multiple breadths and scopes (session/project/global).
TUI:
- Shift+Tab cycles ask -> auto -> trusted workspace -> let Zero cook; the
auto-classifier warning is confirmed once per process, rendered in the
live-tail footer so it paints reliably over a resumed transcript.
- Permission prompt labels each offered prefix breadth distinctly and omits the
empty hotkey bracket for arrow-key-only breadths.
ACP:
- Maps the project-scoped prefix action and expands each prefix action into one
option per offered breadth, encoding and validating the selected breadth on
the option round trip.
Sandbox/permissions:
- Command-prefix ladder excludes the one-token launcher rung and binds a grant
to the effective directory so a `cd` outside the workspace cannot borrow it.
…roject-confined The cd-containment guard cleaned paths and compared them lexically, so a symlink inside the workspace pointing outside it (e.g. /workspace/link -> /outside) passed the check: `cd link && go test` matched a project prefix grant and was promoted to an escalated, unsandboxed command that then ran outside the approved project. Resolve real paths with EvalSymlinks before the containment check: the root and each cd target are canonicalized, and a target that does not exist or whose real path escapes the root fails closed to the sandboxed prompt.
…s off the classifier Two auto-classifier boundaries were overridable: - Writes under protected workspace metadata (.git/**, .zero/**, .agents/**) deliberately stay prompts, but the decision carried no marker excluding them from classifier review, so an LLM allow could write .git/hooks/pre-commit or .git/config and defeat the boundary (enabling later Git-triggered execution). The sandbox now flags such writes (Decision.TouchesProtectedMetadata) and the classifier gate refuses them. - When native shell isolation is unavailable the sandbox prompts instead of wrapping a shell command; the classifier could turn that prompt into an allow and run the command unwrapped on the host. Shell commands are now classifier- eligible only when the native sandbox would actually wrap them (Engine.ShellSandboxActive); file tools, confined by path validation, are unaffected.
Enabling auto-classifier lets an LLM auto-approve low-risk actions; the TUI gates it behind an explicit one-time acknowledgement. ACP had no equivalent, so an editor client could select the mode via session/set_mode and silently enable LLM auto-approval. ACP cannot represent that confirmation, so the mode is no longer advertised in the available modes and set_mode rejects it, matching how Unsafe is already withheld from ACP.
9a0c4bc to
287c1d6
Compare
|
All addressed, and rebased onto latest main.
Tests added for each. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep project-scoped prefix grants inside the project
internal/agent/command_prefix.go:178
The new containment check follows only literalcdsegments and assumes the shell starts atWorkspaceRoot; it ignores the shell tools'cwd/workdirargument. After saving a project-scopedgo testgrant in workspace A, a call such as{command: "go test ./...", cwd: "/outside"}has nocd, matches the A grant, and is changed torequire_escalatedatloop.go:1351, so it runs unsandboxed in/outside. Resolve and contain the effective tool working directory (including symlinks and aliases) before honoring a project grant. -
[P2] Do not apply the project directory guard to global and session grants
internal/agent/command_prefix.go:185
The newcommandDirStaysWithinProjectcheck runs before either persistent or session grant lookup. Consequently, an explicitly global grant—or an ephemeral session grant—such asgo testis ignored forcd /other && go test ./..., even though global grants are documented to match every project and session grants have no project scope. Select the matching grant first, then impose effective-directory containment only when that grant is project-scoped. -
[P2] Keep ACP's config-option mode contract in sync
internal/acp/agent.go:392
session/set_modenow advertises and acceptsworkspace-auto, but the standardsession/set_config_optionpath still accepts and lists onlyautoandask(configOptionsat line 515). Config-driven ACP clients cannot select the newly exposed mode; afterset_mode, they receiveCurrentValue: workspace-autothat is absent from the allowed options. Addworkspace-autoconsistently to both config-option paths and cover the round trip. -
[P3] Make the advertised non-colon wildcard breadth usable
internal/agent/command_prefix.go:111
intraTokenWildcardPrefixaccepts-,/,., and@as namespace separators, but the sandbox recognizes a wildcard only when its stem ends in:(internal/sandbox/command_prefix.go:94). A proposed breadth liketest-*is therefore silently rejected byValidCommandPrefixand never appears in the picker. Either restrict generation to:or teach validation and matching the same separator set.
|
Re-checked my 18 July review against the current head. One of my two blockers is genuinely closed. The other, the security one, is half closed, and the half that remains is the half that matters. Closed: the Still open: the containment guard only watches But the guard never sees the tool's own directory argument. A match here is what promotes the command to the escalated, unsandboxed path, so this is the same escape the What makes it worse than an oversight is which spelling is the recommended one.
So the guard covers the discouraged path and leaves the one the schema actively steers the model towards. And The fix is where the guard already is: give No test would catch it today: every Separately, this cannot merge as it stands. It conflicts with main in twelve files across The classifier core is still good and my earlier notes on it stand: the protected-metadata refusal, the inactive-shell-sandbox refusal and the ACP opt-in rejection are all in place and all fail when I mutate their predicate out. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Sorry about the wait on this, and about the direction I sent you in.
jatmn's P1 is closed. The PR no longer touches serve at all, so the base wiring stands, and TestRunServeMCPWiresWorkspaceRootAndAddDirIntoResources drives the exact reported command and passes.
The cd escape is still open on Windows, and that part is my fault
I told you to use EvalSymlinks. On Windows a directory junction reports ModeIrregular rather than os.ModeSymlink, and filepath.EvalSymlinks does not traverse one, it returns the junction's own path. So the guard is inert against a junction:
mklink: Junction created for ...\001\jlink <<===>> ...\002
Lstat mode=?rw-rw-rw- isSymlink=false
EvalSymlinks("...\001\jlink") = "...\001\jlink" err=<nil>
commandDirStaysWithinProject([cd jlink]) = true
matchCommandPrefix(cd jlink && go test ./...) -> grant{Prefix:["go","test"]}
shellExecutionArgsForApproval -> sandbox_permissions: require_escalated
pnpm creates junctions under node_modules by default, so this is reachable from an ordinary checkout rather than only a crafted one.
What does work, verified against that same junction: open the path with windows.CreateFile(..., FILE_FLAG_BACKUP_SEMANTICS) and call windows.GetFinalPathNameByHandle with FILE_NAME_NORMALIZED|VOLUME_NAME_DOS. That returns the real target, and pathWithinRoot then correctly answers false. golang.org/x/sys is already a dependency, so this wants a _windows.go with the EvalSymlinks path kept as the fallback elsewhere.
Two more that a handle-based resolve would not close, both driven: cd / and cd \. filepath.IsAbs is false for a rooted driveless path, so resolveCdTarget joins it onto the root and it collapses back to the root, the guard answers true, and cmd.exe actually lands at the drive root. Create an ordinary in-workspace Windows\System32 and cd /Windows/System32 && go test ./... matches the grant while the shell runs in the real one.
In fairness to the change: base has no containment check at all and matches all of these too, so this is an incomplete fix and not a regression. cd <abs outside>, bare cd, cd -, cd ~, cd $VAR, cd C:, cd //, cd sub\..\.. and POSIX symlinks are all correctly refused now. What makes it blocking is that the PR ships a test asserting the escape is closed, and a comment at command_prefix.go:246-248 saying the same, and on Windows neither holds.
Three others worth doing while you are in here
command_prefix_test.go:195 calls os.Symlink and t.Fatalfs on failure. On unelevated Windows without Developer Mode that is a hard fail. Every other package in the repo skips instead. CI's runner is Administrator so it stays green, and only contributors see the red suite.
The breadth ladder offers rungs whose entire danger sits in the omitted operands. rm -rf build/ offers [rm -rf]; granted that rung, rm -rf /, rm -rf ~/Documents and rm -rf ../other-repo all match. And because matchCommandPrefix sets permissionGranted=true before Evaluate, the grant also disarms the destructive-command prompt from then on. Requiring a rung to end on a non-flag token would close it.
isShellCommandTool keys on the tool name (bash, exec_command), but the fact being protected is SideEffect == SideEffectShell. write_stdin and Task both carry SideEffectShell + PermissionPrompt + AdvertiseInAuto. A/B through Run with one fake tool and the name as the only variable: bash prompts, write_stdin is classifier-allowed and runs with zero prompts.
canAutoClassifierReview excludes the destructive and network categories but has no risk-level check, so the local_terminal, local_desktop and local_browser family auto-approves at RiskHigh with no sandbox confinement at all. auto_classifier_prompt.md also tells the reviewing model that the sandbox has already blocked the highest-risk categories before they reach it, which is not true for that family and biases it toward allow.
The classifier core still reads well, and the fail-closed default on an unclassifiable prompt is intact.
Resolves 12-file conflict with origin-zero/main (1b5db17). Co-authored-by: pengdst
- commandDirStaysWithinProject now checks effective directory from
workdir/cwd/dir/directory (exec_command) and cwd (bash), resolving
symlinks and refusing outside-project starts. This covers the
Vasanth-found escape where {command:"go test", cwd:outside} bypassed
the cd-only guard while being the recommended cwd-over-cd path.
- add TestMatchCommandPrefixRejectsGrantWhenWorkdirEscapesProject covering
all 4 aliases + bash cwd, inside/outside/symlink/relative cases
- fix loop_test.go: NewWriteFileTool -> NewScopedWriteFileTool after main rename
- move equalStringSlices to production (command_prefix.go) and remove duplicate
from export_test.go so go vet passes for non-test builds
Fixes Gitlawb#570 review blocker, re-mergeable after 1b5db17.
yarn is now a banned launcher in sandbox (unsafeCommandPrefixLauncher), so ValidCommandPrefix rejects yarn prefixes and the wildcard/ ladder tests that used yarn test:unit started failing after the main merge (1b5db17). Switch those tests to cargo which remains allowed.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Overall guidance
The number of findings here does not come from twelve unrelated feature requests. Most are different symptoms of three shared implementation gaps, which is also why earlier point fixes have exposed another edge in a later review:
- The requested prefix scope is carried farther than the effective scope. A prefix starts as project/global/session, but later paths reduce it to an action or a prefix slice. Matching, persistence fallback, model prompt context, ACP labels, and session audit output then reconstruct scope independently and disagree. This accounts for the global/session directory regression, foreign-project prompt disclosure, missing ACP warning, and incorrect audit labels.
- Security policy is inferred from strings and syntax instead of existing typed facts. Classifier confinement checks two tool names rather than
Safety.SideEffect; the breadth ladder accepts every syntactically valid slice without asking whether the remaining tokens still bound the operation; Windows containment applies generic filepath behavior that differs from cmd.exe path resolution. - Tests cover the producer but stop before the downstream consumer. There is good local coverage for creating breadth choices and storing project grants, but less coverage for reopen/match → execute → emit audit state, for both ACP mode entry points, for usage accounting, and for permission-resume timing. That leaves individually correct helpers whose composition is wrong.
To close this without another drip-review cycle, I recommend fixing these as contract groups:
- Have grant matching/persistence return one effective result that retains the selected prefix, actual scope, whether persistence succeeded or fell back to session, applicable project, and whether the current call bypasses the sandbox. Let the execution, ACP disclosure, provider-prompt, and audit consumers use that result instead of inferring state from an action string. This can be a small internal type at the existing grant-resolution seam; it does not require changing the product's three scopes.
- Derive classifier eligibility from the tool's typed safety/risk data and native-sandbox state. Derive prefix warnings from the same predicate that causes sandbox bypass. Keep one source of truth for wildcard separator validity. The goal is to remove parallel allowlists, not add more names to each one.
- Keep the existing project-directory guard, but give its path-resolution seam a Windows implementation that agrees with the shell. This is specifically about junctions and drive-rooted forms already demonstrated; it is not a request to turn project grants into a general filesystem sandbox for arbitrary command arguments.
- Add a compact table-driven matrix that crosses prefix scope (
session,project,global) with lifecycle (select,persist/fallback,reload,match,execute,audit) and frontend (TUI,ACP, provider prompt). Add focused rows for inactive native sandbox +write_stdin, destructive breadth, Windows junction/root paths, both ACP mode APIs, and fixed-choice permission resume. Those cases cover the root causes below without expanding feature scope. - After the grouped changes, run gofmt and the full Linux/macOS/Windows checks, not only the package tests that create the new state. The current live PR is mergeable but blocked, with Ubuntu, macOS, Windows, and Zero Review non-passing.
This guidance is intended to consolidate the existing fixes. It does not ask to remove auto-classifier, remove breadth selection, change the project/global/session product model, expose auto-classifier or unsafe mode through ACP, or revisit specialist autonomy.
Merge readiness
-
[P2] Restore gofmt-clean test files
internal/agent/command_prefix_test.go:291
The head contains an extra blank line here and another ininternal/agent/export_test.go.gofmt -didentifies both,make fmt-checkfails locally, and the live Ubuntu job stops at its hard formatting step before later validation can run.This is mechanical rather than architectural: run gofmt on the two modified files, verify that no unrelated lines change, and rerun the required checks after the functional fixes below.
Findings
-
[P1] Resolve Windows directory aliases before honoring project grants
internal/agent/command_prefix.go:282
The new project guard resolves the initial cwd and literalcdtargets withfilepath.EvalSymlinks, then checksfilepath.IsAbs. Those APIs do not describe every path cmd.exe will actually use: Windows directory junctions are not traversed byEvalSymlinks, and drive-rooted/or\\paths are notfilepath.IsAbseven though cmd.exe resolves them from the current drive root. Withjlink -> C:\outside,cd jlink && go test ./...is judged in-project; likewise,cd /Windows/System32can be joined back under the workspace during the check. The saved project prefix then matches and the call is rewritten torequire_escalated, so execution occurs outside the selected project without another prompt.The root cause is a platform-neutral containment helper being treated as authoritative for platform-specific shell semantics. Keep the existing containment contract, but resolve the final Windows directory using Windows' actual path resolution at this seam (for example, a handle-based final-path lookup), and explicitly recognize rooted drive-relative forms before matching. Regression coverage should drive both junction and
//\\cases throughmatchCommandPrefixand confirm they cannot reach the escalation path, while preserving ordinary in-project directories and the POSIX symlink behavior. -
[P1] Do not offer flag-only destructive prefix rungs
internal/agent/command_prefix.go:85
commandPrefixLadderslices every valid base prefix from two tokens upward, andValidCommandPrefixchecks syntax/launcher safety rather than whether the retained tokens still constrain the action. Forrm -rf build/, this creates["rm","-rf"]. If the user selects that rung, laterrm -rf /,rm -rf ~/Documents, andrm -rf ../other-repoall match before sandbox evaluation.permissionGrantedis set and the call is promoted torequire_escalated, so the newly broad grant suppresses the destructive prompt for a different target.Fix the breadth-generation rule, not only the
rmexample. An intermediate rung must retain the operand or subcommand that bounds the approved operation; a rung ending entirely in option flags is not safe merely because it has two tokens. Preserve useful rungs such as a stable command plus subcommand, preserve exact-prefix fallback and offered-choice validation, and add negative tests showing that target-bearing commands cannot produce a target-free reusable grant. -
[P1] Gate classifier eligibility by the actual shell side effect
internal/agent/loop.go:2286
canAutoClassifierReviewrequires active native isolation only whenisShellCommandTool(tool.Name())recognizesbashorexec_command. The built-inwrite_stdinis neverthelessSideEffectShell,PermissionPrompt, andAdvertiseInAuto; nonempty input is deliberately prompt-gated because it can drive an existing process beyond the original command. With no active native sandbox, its sandbox decision isActionPrompt/RiskHigh, but the name check is false, so an LLMallowsetspermissionGrantedand sends the bytes without the explicit user approval this boundary requires.The root cause is using tool identity as a proxy for execution effect. Base eligibility on
Safety.SideEffect, native-sandbox availability, and the risk classes that are meant to remain explicit; do not maintain another list of shell-like tool names. Add an A/B regression test with identical prompt-gated shell safety and only the tool name changed, plus a test using the realwrite_stdinsafety contract, so future shell/host-control tools inherit the guard automatically. -
[P2] Filter approved-prefix prompt context to the active project
internal/sandbox/engine.go:150
ApprovedCommandPrefixesreturns every persisted row in the shared grant store.approvedCommandPrefixContextthen sends all of those command names to the provider without project labels and states that they “do not need another permission prompt.” After project A savesprivate-script deploy, opening project B therefore discloses that project-specific command to the model and tells it the command is approved, whileLookupCommandPrefixcorrectly refuses the grant in B. The provider's view of authority is both broader and less private than enforcement.The root cause is reusing an administrative, store-wide listing API as an active authorization-context API. Give the prompt consumer a scoped view that contains global grants plus grants for the current project only, and retain scope metadata when rendering it. Keep store-wide listing for
/permissionsor grant management where that breadth is intentional. Test two project roots against one store: B's prompt must exclude A-only rows, include global rows, and agree with runtime lookup. -
[P2] Apply the directory guard only to project-scoped grants
internal/agent/command_prefix.go:186
commandDirStaysWithinProjectruns before either persisted or session lookup, so it cannot know the scope of the grant it is guarding. An explicitly global or sessiongo testgrant is consequently ignored when cwd or a literalcdis outside the current engine root. That contradicts the documented model—global matches every project and session is ephemeral but not project-bound—and causes the user to be prompted again for authority they already granted.Resolve candidate grants first and carry their effective scope through the segmented-command check. Apply directory confinement if any required matched grant is project-scoped; do not apply it to commands covered solely by global/session grants. Preserve the rule that every unsafe segment must be covered or independently known-safe. Tests should cover outside cwd and
cdfor all three scopes, including a multi-segment command whose matches have different scopes. -
[P2] Account for classifier provider usage
internal/agent/auto_classifier.go:47
The classifier completion is drained withzeroruntime.CollectStream, which records the returned usage only inside the local collected result. Nothing forwards it toOptions.OnUsage. Main completions, retries, and compaction useCollectStreamWithOptions(... OnUsage ...), so auto-classifier sessions omit up to one paid completion for every reviewed tool call from TUI/session usage totals, cost reporting, trace accounting, and any budget logic attached to that callback.Route classifier usage through the same accounting callback exactly once. The clean seam could be passing
OnUsageinto the default classifier factory or returning usage alongside the classifier decision; either is acceptable if custom classifier implementations keep their current contract. Preserve the classifier's invisible text stream—only usage should be forwarded—and add a test asserting one callback for one classifier completion with no double count on allow, prompt, error, or timeout. -
[P2] Keep ACP's workspace-auto mode APIs in sync
internal/acp/agent.go:412
The directsession/set_modehandler now acceptsworkspace-auto, but the parallelsession/set_config_optionhandler accepts only auto/ask/plan andconfigOptionsadvertises the same incomplete list. After a client successfully selects workspace-auto throughset_mode, its config state returnsCurrentValue: workspace-autoeven though that value is absent from the allowed choices; attempting to set it through the standard config-option path returns an unknown-mode error.The root cause is two ACP mode allowlists drifting. Define the ACP-selectable mode set once and reuse it for direct validation, config-option validation, and advertised option construction. Keep the deliberate exclusions intact: auto-classifier still requires an in-app acknowledgement ACP cannot represent, and unsafe remains prohibited. Cover both APIs and the returned option list from the same table so adding the next mode cannot update only one door.
-
[P2] Disclose the sandbox bypass for project-prefix approval
internal/acp/permission.go:147
optionKindForappendswithSandboxEscalationNoteto session and global prefix choices, because selecting either also runs the current command outside the sandbox. The new project action returns a bare “Allow this command for this project” label, yetshellPrefixApprovalBypassesSandboxincludes that action andshellExecutionArgsForApprovalperforms the identicalrequire_escalatedrewrite. ACP clients generally display only this label, so the user does not receive the material warning supplied for the sibling choices.Derive disclosure from the same
escalates/bypass predicate for every prefix action instead of adding warnings case by case. Preserve the distinct scope wording, but make the execution consequence identical wherever behavior is identical. A table test over session/project/global actions should assert both the scope label and presence or absence of the escalation note from the actual request conditions. -
[P2] Record the effective prefix scope in permission events
internal/agent/loop.go:1252
There are two ways the event diverges from enforcement. On reuse, every non-session persisted match is recorded asPermissionDecisionAlwaysAllowPrefix, so a project-only match is restored/rendered as “saved prefix (global).” On creation, if project/global persistence fails because the store is unavailable, read-only, or corrupt,persistCommandPrefixGrantScopedOrSessionintentionally installs a safer session grant but leavesDecisionActionas project/global. The transcript then claims persistence that will disappear after restart. This undermines the transparency this PR is specifically adding.Stop reconstructing audit scope from the requested action. Have resolution return the effective grant result—matched/persisted/fallback scope and normalized prefix—and build the permission event from that. The intentional session fallback can remain, but it must be visible as session-scoped (and persistence failure may be recorded separately if useful). Add lifecycle tests for project grant → reload → match → event, global match → event, and forced persistence failure → session fallback → event/restore/render.
-
[P2] Reset the quiet timer after every fixed permission choice
internal/tui/model.go:4504
The old resolver cleared the prompt and setlastStreamActivity = m.now()so human approval time was not treated as provider silence. The newresolvePermissionOptionwas split out to carry the selected breadth, but it copies only the callback and prompt-clear steps. Enter, hotkey, and click all use this new branch; after a long review pause, the run resumes and immediately renders the stale warningstill generating… 1m30s. Free-text feedback still uses the old cleanup and does not fail.The root cause is duplicated permission-finalization logic. Put the invariant cleanup—invoke once, clear prompt, reset the clock, and preserve the existing peer-specific behavior—in one helper shared by fixed and free-text decisions, with the selected prefix as data rather than a separate lifecycle. Extend the existing failing test across Enter, a hotkey, and a click so future option-shape changes cannot bypass cleanup again.
-
[P2] Skip symlink fixtures when Windows cannot create them
internal/agent/command_prefix_test.go:232
Both new containment cases treat anyos.Symlinkerror as a product failure. On ordinary unelevated Windows without Developer Mode, creating a symlink requires a privilege the developer may not have, so the agent package fails before testing containment. GitHub's elevated runner can stay green while supported contributor environments are red, which conflicts with this repository's cross-platform test requirements.Separate fixture capability from the assertion: skip with a clear reason only when the host cannot create the symlink, but keep unexpected setup errors and all behavior assertions fatal once creation succeeds. Reuse the repository's existing platform-capability pattern if one exists. This does not replace the junction coverage requested above; symlinks and junctions exercise different Windows behavior.
-
[P3] Align generated and accepted wildcard separators
internal/agent/command_prefix.go:109
intraTokenWildcardPrefixfinds the last separator from:-/.@and can generatetest-*, path-like, dotted, or at-sign namespace forms. The sandbox'strailingWildcardStemaccepts a wildcard only when its stem ends in:, socommandPrefixLaddersilently drops every generated non-colon candidate throughValidCommandPrefix. The UI therefore cannot offer breadths the generator and comments claim to support.Choose one separator contract and share it between generation, normalization, and matching. Restricting generation to
:is the smallest fix if colon namespaces are the intended feature; supporting more separators requires explicit tests that they cannot widen the launcher or match unintended token shapes. In either case, preserve last-token-only wildcarding, a nonempty stem, and the ban on wildcarding a lone command token.
Summary
Continuation of the auto-classifier permission mode work (builds on the existing permission-mode / permission-UX line, e.g. #170, #244, #246, #248 — related, not a duplicate). Three cohesive improvements:
1. Auto-classifier: real autonomy + transparency
auto-reviewed · <tool> — <reason>); asks now are too.Shift+Tabshows a blocking confirmation explaining that an LLM will auto-review low-risk actions, replacing a fragile timer-based double-press./modecard, and modal.2. Command-prefix grant: breadth picker
yarn,yarn test:*,yarn test:unit) so the approver chooses how wide the grant is instead of getting a single fixed prefix.test:unit→test:*) to grant a namespace of scripts. Only valid on the last token, never on a lone launcher; mid-command globs stay rejected.3. Command-prefix grant: project vs global scope
Projectscope. "allow prefix for this project" matches only inside the current workspace root; "allow prefix globally" matches everywhere; session grants remain ephemeral.Testing
go build ./...clean.go test ./...— full suite green (79 packages).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes