Additional write roots: --add-dir flag, global config key, /add-dir TUI command - #162
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vior - validate: prefer ViolationSymlinkTraversal over ViolationOutsideWorkspace when all roots deny; return original requestedPath in violation; append --add-dir hint only on outside_workspace results; extend doc comment to state that a symlink resolving inside any root is allowed. - normalizePrefixForRoot: dedupe insideRoot check via pathWithinRoot (Fix 3); widen EvalSymlinks comment to cover jump-over case; add POSIX-only note. - WorkspaceRoot: add doc comment noting immutability/lock safety (Fix 4). - Tests: pin multi-root traversal-preferred behavior, deterministic alias test (TestValidateResolvesAliasedPathPrefixes), Add symlink normalization test, and tilde-expansion error path test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Thread *Scope through EngineOptions/Engine so Evaluate and Classify use multi-root scope validation instead of single-root validateWorkspacePaths, letting extra write roots (--add-dir / /add-dir) be honoured by the policy gate and risk classifier. Remove now-dead validateWorkspacePaths helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…roots Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add PathScope interface and scoped resolver helpers to workspace.go, then thread a scope field through all 8 file tools (read_file, list_directory, glob, grep, write_file, edit_file, apply_patch, bash). Each existing constructor delegates to a new NewScoped* variant; nil scope is byte-identical to the original behavior. Registry gains CoreReadOnlyToolsScoped, CoreWriteToolsScoped, CoreShellToolsScoped, and CoreToolsScoped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rsal denial - resolveScopedPath and resolveScopedTargetPath now return the absolute path as the second value when the matched root is an extra (non-workspace) root, eliminating ambiguity in ChangedFiles, cwd meta, and display summaries downstream - Add TestScopedWriteRefusesSameRootSymlinkTraversal to pin that same-root symlink traversal within an extra root is denied (pre-existing behavior) - Add TestScopedWriteReportsAbsolutePathForExtraRoot to enforce the new absolute-path contract for ChangedFiles on extra-root writes - Extend doc comments on PathScope, ChangedFiles, changedFilesFromPatch, and both scoped resolvers to capture the workspace-vs-absolute invariant Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes for the --add-dir dispatch commit: - Add TestRunAddDirDispatchForwardsGrantIntoExecScope: drives runWithDeps through both the "exec" and "-p" dispatch shapes with a provider that calls write_file inside the extra root, asserting the grant reaches the exec sandbox scope (and a negative control without --add-dir is denied fail-closed). Mutation-verified: dropping addDirFlagArgs forwarding from either case fails the test. - Remove help/version from the --add-dir allowlist so any non-TUI/ non-exec leading --add-dir hard-errors, matching the stated requirement; extend the rejection test with those cases. - Fail loud when --add-dir is hidden behind a stray non-flag arg on the --skip-permissions-unsafe path instead of silently dropping the grant. - Reject flag-like values in the inline --add-dir= spelling to match the space form (a directory named -foo stays reachable as ./-foo). - Move TestExecScopeReRegistrationSwapsCoreToolsByName out of the parse test file into the new exec_scope_test.go alongside the e2e test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ack root Review fixes for the Task8 observability range: - runSandboxPolicyEffective JSON payload gains writeRootsError (omitempty) so --json consumers see the same fail-soft signal as the text write_roots_error line instead of a misleading workspace-only writeRoots list. - The fail-soft fallback now derives its write roots from a workspace-only Scope (which cannot fail) so the error path renders the same symlink-resolved root as the success path. - TestRunSandboxPolicyEffectiveWriteRootsFailSoft gains a --json leg asserting writeRootsError names the stale root with a workspace-only fallback; the configured-roots test asserts the key is absent for valid roots. - SandboxPlanSnapshot doc comment reworded to match the converter: no current builder populates WriteRoots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # internal/tools/file_tools_test.go # internal/tools/workspace.go
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughAdds repeatable CLI flag ChangesAdditional write roots feature
Sequence Diagram (high-level flow) sequenceDiagram
participant User
participant CLI
participant Engine
participant Runner
participant HostOS
User->>CLI: launch zero/exec with --add-dir
CLI->>Engine: construct EngineOptions{Scope}
CLI->>Engine: register scoped tools
Engine->>Runner: BuildCommandPlan(writeRoots)
Runner->>HostOS: create sandbox (bubblewrap/seatbelt) with extra binds/write rules
HostOS-->>Runner: runtime enforcement (allow/deny)
Runner-->>Engine: execution result
Engine-->>CLI: return exit/status
CLI-->>User: display success/denial (actionable hint)
Estimated code review effort: Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…batim NormalizePrefixForRoot's POSIX component walk mangled Windows drive paths (C:\Users -> C:Users), which downstream single-root checks treated as relative and joined under the workspace — failing the policy gate OPEN on Windows. Volume-qualified paths now bypass the walk entirely (the macOS /var alias problem it solves has no Windows equivalent) and windows-gated tests pin both the verbatim pass-through and the engine denial. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…olution The previous commit disabled NormalizePrefixForRoot on Windows, which broke it the other direction: a workspace created under an 8.3 short path (C:\Users\RUNNER~1\...) resolves via EvalSymlinks to its long form (runneradmin), so raw short-form requests escaped the long-form root and legitimate extra-root writes were denied. This is the same alias problem as macOS /var -> /private/var, which the helper already solves. Start the component walk from the volume root (C:\ or //host/share/) instead of "/" so it resolves the prefix on Windows too; POSIX VolumeName is empty so the walk reduces to the original "/"-rooted behavior byte-for-byte. Windows test now pins allow-inside-root + deny-outside both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 (3)
internal/tools/bash.go (1)
31-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the bash schema to mention granted directories.
The tool now accepts absolute
cwdvalues inside extra granted roots, but both the tool description and thecwdparameter still say “workspace”. That mismatch will steer the model away from the new feature and make valid extra-root paths look unsupported.🤖 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/tools/bash.go` around lines 31 - 42, The bash tool description and its "cwd" parameter text in NewScopedBashTool incorrectly say only "workspace" even though cwd may be an absolute path inside extra granted roots; update the baseTool.description and the "cwd" PropertySchema Description to mention that commands may run in the workspace root or in granted extra roots (e.g., "workspace root or any extra granted directories"), and adjust any guidance text that references only "workspace" to reflect both workspace and granted directories so the schema and description align with the new feature.internal/tools/grep.go (1)
30-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh grep's schema so it matches the new scope rules.
grepnow accepts granted extra roots, but the description andpathhelp still read as workspace-only. That mismatches the actual contract and makes scoped searches harder to invoke correctly.Suggested wording
- description: "Search file contents with a regular expression inside the workspace.", + description: "Search file contents with a regular expression inside the workspace or an explicitly granted extra root.", ... - "path": {Type: "string", Description: "Directory or file to search. Defaults to workspace root.", Default: "."}, + "path": {Type: "string", Description: "Directory or file to search. Relative paths stay in the workspace; use an absolute path to search a granted extra root. Defaults to workspace root.", Default: "."},🤖 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/tools/grep.go` around lines 30 - 40, Update the grep tool's user-facing schema in NewScopedGrepTool/grepTool/baseTool.parameters so it accurately reflects the new scope rules: change the top-level description to mention that searches may span the workspace root and any granted extra roots, and update the "path" PropertySchema to note that it can be a workspace-relative path, an absolute path, or a granted-root-relative path (and that the default remains "." meaning the effective root for the current scope). Ensure the "glob" and "pattern" descriptions remain unchanged but are consistent with the expanded scope wording.internal/tools/apply_patch.go (1)
22-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the schema text to describe scoped
cwdbehavior.The tool is now scope-aware, but it still advertises workspace-only patching. That makes the new
--add-dir//add-dirflow hard to discover and easy to misuse: relativecwdstays in the workspace, while granted extra roots need an absolute path.Suggested wording
- description: "Apply a unified diff patch inside the workspace.", + description: "Apply a unified diff patch inside the workspace or an explicitly granted extra write root.", ... - "cwd": {Type: "string", Description: "Directory where the patch should be applied. Defaults to workspace root.", Default: "."}, + "cwd": {Type: "string", Description: "Directory where the patch should be applied. Relative paths stay in the workspace; use an absolute path to target a granted extra write root. Defaults to workspace root.", Default: "."},🤖 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/tools/apply_patch.go` around lines 22 - 31, Update the schema description for the "cwd" parameter in NewScopedApplyPatchTool (applyPatchTool/baseTool) to reflect scope-aware behavior: explain that relative paths remain inside the workspace root, while when using additional granted roots (e.g., via --add-dir or /add-dir) an absolute path is required to target those extra directories; mention that the default "." means workspace root and clarify the difference between relative and absolute cwd when PathScope grants extra roots.
🤖 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 `@docs/superpowers/plans/2026-06-10-additional-write-roots.md`:
- Line 15: The heading "Task 1: `sandbox.Scope` core type" is H3 and creates a
jump from the document's H1, causing markdownlint violation; edit that line to
be an H2 (i.e., change the leading "###" to "##") or insert a new H2 above it so
heading depth increments cleanly and preserves the section title referencing
sandbox.Scope.
- Line 1059: The markdown line containing '---### Task 6: CLI wiring
(`--add-dir` on `zero` and `zero exec`)' is malformed; replace it with two lines
so the horizontal rule is on its own line ('---') followed by a separate heading
line starting with '### Task 6: CLI wiring (`--add-dir` on `zero` and `zero
exec`)' to ensure proper rendering.
- Around line 1215-1218: The fenced code block missing a language token triggers
MD040; update the triple-backtick fence to include an appropriate language
identifier (e.g., "bash" or "text") for the shown snippet and make the identical
change in the writeExecHelp function's flag section so both code fences match
surrounding formatting; locate the block containing "--add-dir <path> Allow
writes in an extra directory (repeatable)" and the writeExecHelp symbol and add
the same language token to each opening ``` fence.
In `@internal/sandbox/engine.go`:
- Around line 34-45: The Engine is created with workspaceRoot left empty when
only EngineOptions.Scope is provided, causing later failures; update the Engine
constructor logic (where EngineOptions.Scope is read and Engine{workspaceRoot:
...} is returned) to populate workspaceRoot from the provided scope by taking
the first root from scope.Roots() (and passing it through
normalizeWorkspaceRootBestEffort) or, if scope has no roots, return an error to
fail fast; alternatively explicitly reject the combination of a non-nil
EngineOptions.Scope with an empty WorkspaceRoot in NewEngine and document that
behavior. Also add a regression test for NewEngine(EngineOptions{Scope: scope})
that ensures workspaceRoot is derived or that creation fails as expected.
In `@internal/tools/glob.go`:
- Around line 69-70: The glob function currently calls
resolveScopedPath(tool.workspaceRoot, tool.scope, cwd) and emits matches
relative to root, which causes incorrect resolution when cwd resolves outside
the workspace; update glob to capture the resolved display root returned by
resolveScopedPath and, if that display root is not the same as
tool.workspaceRoot (i.e., cwd resolved to an extra-root), convert each match to
an absolute path by joining it with the resolved root (use filepath.Join/ Clean
semantics) before returning, while still preserving the display root for UI;
keep existing relative behavior when the resolved root equals
tool.workspaceRoot.
In `@internal/tools/workspace.go`:
- Around line 218-223: The scopedRoots function must fail closed when given a
non-nil PathScope whose Roots() is empty: change scopedRoots to validate that
scope != nil implies len(scope.Roots()) > 0, build a roots slice with
workspaceRoot as the first element followed by scope.Roots(), and return an
error (not an empty slice) if the contract is violated; update callers to handle
the error so helpers never silently accept an empty Roots() and thus never allow
Cmd.Dir == "" or other escapes.
---
Outside diff comments:
In `@internal/tools/apply_patch.go`:
- Around line 22-31: Update the schema description for the "cwd" parameter in
NewScopedApplyPatchTool (applyPatchTool/baseTool) to reflect scope-aware
behavior: explain that relative paths remain inside the workspace root, while
when using additional granted roots (e.g., via --add-dir or /add-dir) an
absolute path is required to target those extra directories; mention that the
default "." means workspace root and clarify the difference between relative and
absolute cwd when PathScope grants extra roots.
In `@internal/tools/bash.go`:
- Around line 31-42: The bash tool description and its "cwd" parameter text in
NewScopedBashTool incorrectly say only "workspace" even though cwd may be an
absolute path inside extra granted roots; update the baseTool.description and
the "cwd" PropertySchema Description to mention that commands may run in the
workspace root or in granted extra roots (e.g., "workspace root or any extra
granted directories"), and adjust any guidance text that references only
"workspace" to reflect both workspace and granted directories so the schema and
description align with the new feature.
In `@internal/tools/grep.go`:
- Around line 30-40: Update the grep tool's user-facing schema in
NewScopedGrepTool/grepTool/baseTool.parameters so it accurately reflects the new
scope rules: change the top-level description to mention that searches may span
the workspace root and any granted extra roots, and update the "path"
PropertySchema to note that it can be a workspace-relative path, an absolute
path, or a granted-root-relative path (and that the default remains "." meaning
the effective root for the current scope). Ensure the "glob" and "pattern"
descriptions remain unchanged but are consistent with the expanded scope
wording.
🪄 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 Plus
Run ID: e4cbd36d-1628-4233-a9a2-669fea3de92c
📒 Files selected for processing (44)
README.mddocs/PRD.mddocs/superpowers/plans/2026-06-10-additional-write-roots.mddocs/superpowers/specs/2026-06-10-additional-write-roots-design.mdinternal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_parse_add_dir_test.gointernal/cli/exec_scope_test.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/paths.gointernal/sandbox/risk.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/scope.gointernal/sandbox/scope_test.gointernal/sandbox/scope_windows_test.gointernal/sandbox/seatbelt_integration_darwin_test.gointernal/tools/apply_patch.gointernal/tools/bash.gointernal/tools/edit_file.gointernal/tools/file_tools_test.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/list_directory.gointernal/tools/read_file.gointernal/tools/registry.gointernal/tools/types.gointernal/tools/workspace.gointernal/tools/write_file.gointernal/tui/add_dir.gointernal/tui/add_dir_test.gointernal/tui/commands.gointernal/tui/image_attach.gointernal/tui/model.gointernal/zerocommands/sandbox_snapshots.gointernal/zerocommands/sandbox_snapshots_test.go
💤 Files with no reviewable changes (1)
- internal/sandbox/paths.go
# Conflicts: # internal/cli/app.go # internal/cli/app_test.go
Resolve the CHANGES_REQUESTED review on PR #162: - engine: derive workspaceRoot from scope.Roots()[0] when NewEngine is given a Scope without an explicit WorkspaceRoot, so Evaluate's EnforceWorkspace/classification guards no longer silently skip (+ regression test). - tools/workspace: scopedRoots now fails closed (returns an error) when a non-nil PathScope exposes empty Roots(), instead of returning success with an empty path; threaded through the scoped resolve/recheck helpers and resolveGrepRoot. - tools/glob: emit absolute matches when cwd resolves into an extra root so results can't be fed back and resolve to a same-named workspace file (+ regression test). - tools/bash,grep,apply_patch: schema descriptions mention granted extra roots (relative -> workspace, absolute -> granted root). - docs/plan: fix markdownlint heading increment, malformed rule+heading, and bare code fence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # README.md
There was a problem hiding this comment.
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)
1159-1163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid resetting chat scroll on empty submit.
Line 1159 resets
chatScrollOffsetbefore thecommandEmptyearly return, so pressing Enter on an empty composer jumps the user to the bottom even though nothing was submitted.Suggested fix
m.rememberInput(input) m.clearComposer() m.clearSuggestions() - m.chatScrollOffset = 0 switch command.kind { case commandEmpty: return m, nil + default: + m.chatScrollOffset = 0 case commandHelp:🤖 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 1159 - 1163, The reset of m.chatScrollOffset is happening before the early return for an empty submission; move the reset so it only occurs for real submissions (i.e., do not reset when command.kind == commandEmpty). Concretely, in the function where m.chatScrollOffset is set and the switch on command.kind occurs, ensure you check command.kind != commandEmpty (or handle the commandEmpty case before resetting) and only assign m.chatScrollOffset = 0 for non-empty command kinds (reference m.chatScrollOffset and the command.kind / case commandEmpty in model.go).
♻️ Duplicate comments (1)
internal/tools/grep.go (1)
102-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve absolute file identity for extra-root grep results.
Line 102 resolves extra-root targets, but the display root is discarded and
collectGrepMatchesstill reportsmatch.filerelative toresolvedRoot. A grep against a granted root outside the workspace can therefore returnfoo.txt, and feeding that back intoread_file/edit_filewill resolve under the workspace instead of the granted root. Mirror theglobfix here: carry the extra-root signal through and emit absolute file paths forcontentandfiles_with_matcheswhenever the search target resolves outside the workspace.Possible fix shape
- target, _, err := resolveScopedPath(tool.workspaceRoot, tool.scope, targetPath) + target, displayRoot, err := resolveScopedPath(tool.workspaceRoot, tool.scope, targetPath) if err != nil { return errorResult("Error running grep: " + err.Error()) } @@ - matches := collectGrepMatches(resolvedRoot, files, compiled) + matches := collectGrepMatches(resolvedRoot, filepath.IsAbs(displayRoot), files, compiled)-func collectGrepMatches(resolvedRoot string, files []string, compiled *regexp.Regexp) []grepMatch { +func collectGrepMatches(resolvedRoot string, absolutePaths bool, files []string, compiled *regexp.Regexp) []grepMatch { @@ - matches = append(matches, grepMatch{ - file: relative, + fileLabel := relative + if absolutePaths { + fileLabel = filepath.ToSlash(resolvedPath) + } + matches = append(matches, grepMatch{ + file: fileLabel, line: index + 1, text: strings.TrimRight(line, "\r"), hits: len(lineMatches), })🤖 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/tools/grep.go` around lines 102 - 114, The grep flow currently discards the resolved display root when a target resolves outside the workspace causing collectGrepMatches to emit relative paths (match.file) that get re-resolved under the workspace; update the logic that calls resolveScopedPath/resolveGrepRoot and collectGrepMatches so it preserves an "extra-root" signal (e.g., whether resolvedRoot != tool.workspaceRoot) and when true emit absolute file paths for match.file, content and files_with_matches instead of rel paths—propagate this flag into collectGrepMatches (or adjust its return handling) and use resolvedRoot as the base for Rel/Path computations so reads/edits use the granted root. Ensure symbols mentioned: resolveScopedPath, resolveGrepRoot, collectGrepMatches, match.file, files_with_matches, content are updated accordingly.
🤖 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.
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 1159-1163: The reset of m.chatScrollOffset is happening before the
early return for an empty submission; move the reset so it only occurs for real
submissions (i.e., do not reset when command.kind == commandEmpty). Concretely,
in the function where m.chatScrollOffset is set and the switch on command.kind
occurs, ensure you check command.kind != commandEmpty (or handle the
commandEmpty case before resetting) and only assign m.chatScrollOffset = 0 for
non-empty command kinds (reference m.chatScrollOffset and the command.kind /
case commandEmpty in model.go).
---
Duplicate comments:
In `@internal/tools/grep.go`:
- Around line 102-114: The grep flow currently discards the resolved display
root when a target resolves outside the workspace causing collectGrepMatches to
emit relative paths (match.file) that get re-resolved under the workspace;
update the logic that calls resolveScopedPath/resolveGrepRoot and
collectGrepMatches so it preserves an "extra-root" signal (e.g., whether
resolvedRoot != tool.workspaceRoot) and when true emit absolute file paths for
match.file, content and files_with_matches instead of rel paths—propagate this
flag into collectGrepMatches (or adjust its return handling) and use
resolvedRoot as the base for Rel/Path computations so reads/edits use the
granted root. Ensure symbols mentioned: resolveScopedPath, resolveGrepRoot,
collectGrepMatches, match.file, files_with_matches, content are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 523981dc-9d97-4c87-bac6-3322d783ab63
📒 Files selected for processing (15)
docs/superpowers/plans/2026-06-10-additional-write-roots.mdinternal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/setup.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/tools/apply_patch.gointernal/tools/bash.gointernal/tools/file_tools_test.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/workspace.gointernal/tui/commands.gointernal/tui/model.go
✅ Files skipped from review due to trivial changes (2)
- internal/cli/setup.go
- docs/superpowers/plans/2026-06-10-additional-write-roots.md
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/tools/bash.go
- internal/cli/exec.go
- internal/tui/commands.go
- internal/tools/apply_patch.go
- internal/tools/file_tools_test.go
- internal/cli/app.go
- internal/tools/workspace.go
- internal/cli/app_test.go
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 `@README.md`:
- Around line 79-89: The commands reference table is missing the `/add-dir`
entry; update the markdown table (the block listing commands like `/image`,
`/resume`, `/rewind`, etc.) to include a new row for `/add-dir` so users can
discover it—add the `/add-dir` label and a short description like "add a local
directory to the agent's workspace / attach multiple files" next to `/image` (or
in the same grouping) so the command documented at line ~192 and mentioned under
"Safe by default" is visible in the quick-reference table.
🪄 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 Plus
Run ID: a86d719c-9492-4320-955b-7f24cb303bd2
📒 Files selected for processing (1)
README.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@README.md`:
- Around line 79-89: The commands reference table is missing the `/add-dir`
entry; update the markdown table (the block listing commands like `/image`,
`/resume`, `/rewind`, etc.) to include a new row for `/add-dir` so users can
discover it—add the `/add-dir` label and a short description like "add a local
directory to the agent's workspace / attach multiple files" next to `/image` (or
in the same grouping) so the command documented at line ~192 and mentioned under
"Safe by default" is visible in the quick-reference table.
🪄 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 Plus
Run ID: a86d719c-9492-4320-955b-7f24cb303bd2
📒 Files selected for processing (1)
README.md
🛑 Comments failed to post (1)
README.md (1)
79-89:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
/add-dirto the TUI commands table.The
/add-dircommand is documented at line 192 and mentioned in the "Safe by default" feature at line 45, but it's missing from this reference table. Users scanning the table won't discover the command.📋 Suggested table entry
Add a row after
/imageor in the appropriate grouping:| `/model` `/provider` | switch model or provider mid-session (searchable picker) | | `/spec` `/plan` | spec-mode drafting and live plan view | | `/image` | attach images for vision models | +| `/add-dir` | grant extra write directories (session-only), or list current write roots | | `/resume` `/rewind` | time-travel across sessions |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.| | | |---|---| | `/model` `/provider` | switch model or provider mid-session (searchable picker) | | `/spec` `/plan` | spec-mode drafting and live plan view | | `/image` | attach images for vision models | | `/add-dir` | grant extra write directories (session-only), or list current write roots | | `/resume` `/rewind` | time-travel across sessions | | `/compact` `/context` | manage the context window | | `/permissions` `/tools` | inspect what the agent can touch | | `/theme` `/style` | make it yours | | `/doctor` `/usage` `/config` | health, cost, and config without leaving the chat |🤖 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 `@README.md` around lines 79 - 89, The commands reference table is missing the `/add-dir` entry; update the markdown table (the block listing commands like `/image`, `/resume`, `/rewind`, etc.) to include a new row for `/add-dir` so users can discover it—add the `/add-dir` label and a short description like "add a local directory to the agent's workspace / attach multiple files" next to `/image` (or in the same grouping) so the command documented at line ~192 and mentioned under "Safe by default" is visible in the quick-reference table.
Vasanthdev2004
requested changes
Jun 11, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Request changes
I found one still-valid behavior issue plus the current main conflict.
[P1] grep still returns extra-root matches as relative paths. The PR fixed this class for glob, but grep has the same problem: when searching an explicitly granted extra root, output like report.go:1: ... can be fed back into read_file/edit_file and resolve against the workspace instead of the extra root. Please carry whether the selected grep root is outside the workspace and emit absolute paths for extra-root matches in both content and files_with_matches modes.
Merge blocker: this PR currently conflicts with latest main in README.md.
Validation on the PR head:
go test ./...passedgo vet ./...passedgo run ./cmd/zero-release buildpassedgo run ./cmd/zero-release smokepassed (zero.exe smoke check passed (0.1.0))
The rest of the additional write-root plumbing looks solid from this pass, and the outdated CodeRabbit glob.go finding is already fixed.
- grep now emits absolute, symlink-resolved paths for matches in a granted extra (non-workspace) root, mirroring the glob fix. A bare workspace-relative name like "report.go" otherwise resolves back under the workspace when fed to read_file/edit_file and hits the wrong file when the name exists in both roots. Covers content and files_with_matches output. - tui: only reset chatScrollOffset for a real submission, not an empty Enter (a no-op), so the viewport is no longer yanked to the bottom when nothing was submitted. Real submissions still snap back. - glob: schema description now mentions granted extra roots, matching the bash/grep/apply_patch wording so the --add-dir/-add-dir flow is discoverable. Regression tests: TestScopedGrepReturnsAbsoluteMatchesForExtraRoot, TestEmptySubmitKeepsChatScrollOffset.
Collaborator
Author
|
Pushed fixes addressing the review feedback (head
Validation: @coderabbitai review |
|
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check open review comments and PR status
gh pr view 162 --repo Gitlawb/zero --json state,reviewDecision,statusCheckRollup,reviews | jq .
echo "--- Unresolved review threads ---"
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 162) {
reviewThreads(first: 50) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
path
line
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == false) | {path: .comments.nodes[0].path, line: .comments.nodes[0].line, author: .comments.nodes[0].author.login, body: (.comments.nodes[0].body | .[0:200])}'Length of output: 35795
Fix those two and the PR should be clean to approve. |
The command was documented further down but missing from the scannable commands table, so users browsing the table wouldn't discover the additional-write-roots flow.
Collaborator
Author
|
Pushed
So both flagged items are now resolved. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify README /add-dir row in table
echo "=== README table area ==="
grep -n "add-dir\|/image\|/resume\|/rewind" README.md | head -30
echo ""
echo "=== bash.go cwd description ==="
grep -n "Relative paths\|granted extra\|workspace" internal/tools/bash.go | head -10
echo ""
echo "=== grep.go path description ==="
grep -n "Relative paths\|granted extra\|workspace" internal/tools/grep.go | head -10
echo ""
echo "=== apply_patch.go cwd description ==="
grep -n "Relative paths\|granted extra\|workspace" internal/tools/apply_patch.go | head -10
echo ""
echo "=== glob.go description ==="
grep -n "Relative paths\|granted extra\|workspace" internal/tools/glob.go | head -10Length of output: 5491
No remaining open items. Approving. [approve] |
Collaborator
Author
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
Vasanthdev2004
approved these changes
Jun 11, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE. I reviewed the additional write roots flow across sandbox.Scope, CLI/TUI wiring, exec forwarding, config merge rules, scoped file/shell tools, and the policy snapshot output. I did not find a blocking issue. Validation: go test ./... passed locally in D:\tmp\zero-review-162.
# Conflicts: # internal/sandbox/runner.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tools/file_tools_test.go (1)
555-562: 💤 Low valueConsider reordering assertions to check non-empty before iterating.
The loop at lines 555-559 iterates over
res.ChangedFileschecking absolute paths, but the non-empty check at lines 560-562 comes after. IfChangedFilesis empty, the loop succeeds vacuously before the non-empty assertion fails. The current order still catches the bug, but checking non-empty first is clearer intent.♻️ Optional: Reorder assertions
if res.Status != StatusOK { t.Fatalf("status=%s output=%s", res.Status, res.Output) } + if len(res.ChangedFiles) == 0 { + t.Fatal("expected ChangedFiles to record the extra-root write") + } for _, changed := range res.ChangedFiles { if !filepath.IsAbs(changed) { t.Fatalf("ChangedFiles=%v — extra-root entries must be absolute, got relative %q", res.ChangedFiles, changed) } } - if len(res.ChangedFiles) == 0 { - t.Fatal("expected ChangedFiles to record the extra-root write") - }🤖 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/tools/file_tools_test.go` around lines 555 - 562, Reorder the assertions so the non-empty check runs before iterating: first assert len(res.ChangedFiles) != 0 (use the existing t.Fatal message) and only then loop over res.ChangedFiles to validate each entry with filepath.IsAbs; update the code around res.ChangedFiles and the existing t.Fatal/t.Fatalf calls accordingly so the emptiness check fails fast before the absolute-path checks.internal/sandbox/seatbelt_integration_darwin_test.go (1)
111-111: 💤 Low valueShell script construction is fragile with embedded single quotes.
The script embeds
contentandtargetinside single quotes without escaping. If either value contained a single quote, the script would break or behave unexpectedly. In this test the values are controlled literals, so this is safe today, but the pattern is fragile for future maintenance.♻️ Optional: Use printf %s with separate argument to avoid quoting issues
- script := "printf %s '" + content + "' > '" + target + "'" + // Use heredoc or env var to avoid quote escaping issues in shell + script := "printf '%s' " + shellQuote(content) + " > " + shellQuote(target)Or pass content via stdin:
script := "cat > " + shellQuote(target) // Then use command.Stdin = strings.NewReader(content)🤖 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/seatbelt_integration_darwin_test.go` at line 111, The test builds a fragile shell command into the script variable by embedding content and target inside single quotes; change it to avoid placing content in the command string. For example, construct script only to redirect into the quoted target (e.g., script := "cat > " + shellQuote(target) or use printf with the target only) and pass content via the command's stdin (e.g., set command.Stdin = strings.NewReader(content)), or alternately use exec arguments so content is not in single quotes; update the code that sets script and the command invocation accordingly (referencing script, content, target and any shellQuote helper).
🤖 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.
Nitpick comments:
In `@internal/sandbox/seatbelt_integration_darwin_test.go`:
- Line 111: The test builds a fragile shell command into the script variable by
embedding content and target inside single quotes; change it to avoid placing
content in the command string. For example, construct script only to redirect
into the quoted target (e.g., script := "cat > " + shellQuote(target) or use
printf with the target only) and pass content via the command's stdin (e.g., set
command.Stdin = strings.NewReader(content)), or alternately use exec arguments
so content is not in single quotes; update the code that sets script and the
command invocation accordingly (referencing script, content, target and any
shellQuote helper).
In `@internal/tools/file_tools_test.go`:
- Around line 555-562: Reorder the assertions so the non-empty check runs before
iterating: first assert len(res.ChangedFiles) != 0 (use the existing t.Fatal
message) and only then loop over res.ChangedFiles to validate each entry with
filepath.IsAbs; update the code around res.ChangedFiles and the existing
t.Fatal/t.Fatalf calls accordingly so the emptiness check fails fast before the
absolute-path checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 832b4728-c903-4718-b30c-2e17424de12f
📒 Files selected for processing (9)
internal/cli/exec.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/seatbelt_integration_darwin_test.gointernal/tools/apply_patch.gointernal/tools/bash.gointernal/tools/file_tools_test.gointernal/tools/grep.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/cli/exec.go
- internal/tui/model.go
- internal/tools/bash.go
- internal/tools/apply_patch.go
- internal/sandbox/runner_test.go
- internal/sandbox/runner.go
- internal/tools/grep.go
gnanam1990
added a commit
that referenced
this pull request
Jun 11, 2026
Resolve add/add collision on internal/sandbox/scope.go and scope_test.go: PR #162 added a same-named scope.go for the multi-root write Scope (--add-dir). Keep that file as-is and move the grant-scoping code (DeriveScope/resolveScopeAbs/ grantCovers/ScopeKind) into grant_scope.go; its tests live in grant_scope_test.go. engine.go/engine_test.go auto-merged — both features coexist.
gnanam1990
added a commit
that referenced
this pull request
Jun 11, 2026
The design spec named internal/sandbox/scope.go as the source of truth, but DeriveScope/resolveScopeAbs/grantCovers live in grant_scope.go (scope.go is the unrelated write-roots Scope from #162). Fix the heading and the test reference (grant_scope_test).
gnanam1990
added a commit
that referenced
this pull request
Jun 11, 2026
* permissions: show grant scope on the permission card and decided rows The permission card only named the tool, so 'always' read as a blind tool-wide yes. Derive a concise scope from the call's args (the file path, directory, or working dir it touches) and show it on the card and the persisted decision row, so a user can see exactly what an allow covers. Phase 0 of the permission-UX work: presentation only, no storage change. permissionScope + scope plumbing through PermissionEvent/PermissionRequest; covered by TestPermissionScope. * permissions: spec for Phase 1 scope-enforced grants * permissions: enforce grant scope so "always allow" covers only what the card showed Phase 0 displayed the scope a tool call touches but the persisted grant was still tool-wide: an "always allow" on a write to one file silently authorized every write. This scopes the grant to exactly that file or directory. sandbox/scope.go centralizes scope derivation (DeriveScope), absolute-path resolution (resolveScopeAbs, anchored to the workspace so a grant never leaks across projects), and matching (grantCovers: file = exact, dir = subtree, empty = tool-wide; a tool-wide request is never covered by a narrower grant). Grants are now stored per-tool as a list (schema v2, with v1 files migrated as tool-wide grants). Lookup takes the request's absolute scope and applies deny-wins / most-specific-allow precedence. engine.Grant anchors a relative scope to the workspace; engine.Decide derives and matches the request scope. agent.persistPermissionGrant forwards the scope, and permissionScope now shares DeriveScope so the card and the stored grant can never diverge. Canonicalizing tool keys on read also closes the whitespace-padded-key lookup miss. Covered by sandbox scope/grant/engine tests and the agent/TUI always-allow tests. * sandbox: fix Windows-only TestResolveScopeAbs by using truly-absolute paths A leading separator (\proj\a) is rooted but not absolute on Windows (filepath.IsAbs requires a volume like C:), so the absolute-passthrough case was treated as relative and anchored onto the workspace root, producing a doubled path. Build the root via filepath.Abs so the test exercises a genuinely-absolute path on every platform. * test: isolate session store in exec/tui tests so go test stops writing to $HOME TestRunExecStreamJSONRunStartUsesResolvedAPIModel, TestRunExecReadsStreamJSONPromptFromStdin, and TestPromptSubmitInjectsLiveSessionModelContext ran real exec sessions but only isolated cwd (t.TempDir + getwd), not the session store, so the default store resolved sessions.DefaultRoot() off the real $HOME and persisted into ~/.local/share/zero/sessions on every run. Set XDG_DATA_HOME to a per-test temp dir (the existing convention in exec_test.go / exec_scope_test.go) so each run is fully isolated. * permissions: address review — clean root-equiv scopes, label+fit scope rows - DeriveScope: filepath.Clean the path before the root check so ./, ./., and a/.. collapse to the workspace root and surface as tool-wide instead of a narrower directory grant (which re-prompted inconsistently). Adds regression cases. - renderPermissionRow: prefix the scope segment as scope:<value> across all three branches and fitStyledLine the allow branch (it returned unfit and could overflow narrow terminals). - TestGrantReplacesSameScope: assert the setup store.Grant errors so a setup failure fails at the real cause, not a stale-state assertion. * docs: point permission-scope spec at grant_scope.go The design spec named internal/sandbox/scope.go as the source of truth, but DeriveScope/resolveScopeAbs/grantCovers live in grant_scope.go (scope.go is the unrelated write-roots Scope from #162). Fix the heading and the test reference (grant_scope_test).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Zero's sandbox confined all writes to the workspace root with no escape hatch — asking the agent to save a file anywhere else failed with raw
Operation not permittederrors (or silent empty results). This PR adds user-grantable extra write roots, threaded through all four enforcement layers via one shared, thread-safesandbox.Scope, so a mid-session grant takes effect immediately everywhere.sandbox.Scope(internal/sandbox/scope.go) — workspace root + extra roots; strict grant normalization (~-expansion, absolutize, symlink-resolve, must-exist directory, filesystem root rejected); multi-root validation that keeps per-root symlink-traversal protection and prefers traversal violations in reporting.Evaluateand risk classification are scope-aware; per-request override roots deliberately do NOT inherit extra roots; denials are now actionable:"...is outside the workspace (use /add-dir or --add-dir to allow writes there)".(subpath …)per root; Linux bubblewrap binds extra roots rw at their real paths (with extra-root cwd chdir handling); command cwd validates against the scope. Profiles rebuild per command plan, so/add-dirwidens the next bash command without restart.ChangedFilesentries are absolute to avoid ambiguity; checkpoint/rewind stays workspace-only.--add-dironzeroandzero exec(forwarded across all dispatch paths, hard-error on unsupported subcommands);sandbox.additionalWriteRootshonored from the global user config only (union merge; project config deliberately excluded so a cloned repo cannot grant itself write access);/add-dirTUI command (bare form lists roots; grants are session-only).zero sandbox policy --effectivelistswrite_roots(text + JSON, with fail-softwrite_roots_errorfor stale config entries);SandboxPlanSnapshot.WriteRoots.Test Plan
go test ./...— all 52 packages green;-raceon sandbox/tools/cli/tuigo vet ./...,gofmt -lcleansandbox-execfor real): write inside extra root succeeds, outside all roots denied by the OS, workspace control passes..relative escapes,/grants, project-config injection, override-root isolation, mid-session grant visibility — all fail closed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation