Files
zero/internal/cli/exec_scope_test.go
KRATOSandClaude Fable 5 e381d8eab1 Additional write roots: --add-dir flag, global config key, /add-dir TUI command (#162)
* docs: spec for additional write roots (--add-dir / /add-dir)

Co-Authored-By: Claude Fable 5 <[email protected]>

* docs: implementation plan for additional write roots

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: add Scope type for multi-root write access

Co-Authored-By: Claude Fable 5 <[email protected]>

* docs: write roots honored from global config only, union merge semantics

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: fix multi-root violation reporting and pin prefix-alias behavior

- 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 <[email protected]>

* sandbox: make engine path validation and risk scope-aware

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 <[email protected]>

* sandbox: pin override-root scope isolation, tidy Evaluate scope handling

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: widen seatbelt/bubblewrap profiles and command cwd to scope roots

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: pin extra-root chdir behavior and document bind invariants

Co-Authored-By: Claude Fable 5 <[email protected]>

* tools: resolve paths against the shared write scope

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 <[email protected]>

* tools: restore fail-closed write-target symlink checks, share prefix normalization

* tools: report absolute paths for extra-root changes, pin scoped traversal 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 <[email protected]>

* config: add sandbox.additionalWriteRoots (global config only, union merge)

* cli: add repeatable --add-dir flag wiring scope into registry and engine

Co-Authored-By: Claude Fable 5 <[email protected]>

* cli: forward --add-dir across dispatch paths, pin scope wiring with tests

Co-Authored-By: Claude Fable 5 <[email protected]>

* cli: pin --add-dir dispatch forwarding end-to-end, close guard gaps

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 <[email protected]>

* tui: add /add-dir command for session write-root grants

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: surface write roots in zero sandbox status and plan snapshots

Co-Authored-By: Claude Fable 5 <[email protected]>

* cli: surface write-roots error in --effective --json, normalize fallback 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 <[email protected]>

* sandbox: seatbelt integration test for extra write roots

Co-Authored-By: Claude Fable 5 <[email protected]>

* docs: document --add-dir and /add-dir write-root grants

Co-Authored-By: Claude Fable 5 <[email protected]>

* sandbox: pass volume-qualified paths through prefix normalization verbatim

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 <[email protected]>

* sandbox: make prefix normalization volume-aware for Windows alias resolution

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 <[email protected]>

* sandbox: address PR review findings for additional write roots

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) <[email protected]>

* tools/grep, tui: address PR review for additional write roots

- 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.

* docs: add /add-dir to the TUI commands quick-reference table

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.

---------

Co-authored-by: Claude Fable 5 <[email protected]>
2026-06-11 15:17:20 +05:30

178 lines
6.5 KiB
Go

package cli
import (
"bytes"
"context"
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
"github.com/Gitlawb/zero/internal/config"
"github.com/Gitlawb/zero/internal/sandbox"
"github.com/Gitlawb/zero/internal/tools"
"github.com/Gitlawb/zero/internal/zeroruntime"
)
// TestRunAddDirDispatchForwardsGrantIntoExecScope pins the dispatch seam
// end-to-end: a leading --add-dir on the ROOT argv (both the "exec" and the
// "-p" dispatch shapes in runWithDeps) must reach exec's sandbox scope so a
// write_file call inside the extra root succeeds. The negative control runs
// the identical provider WITHOUT --add-dir and must be denied (fail-closed).
// If addDirFlagArgs forwarding were dropped from either dispatch case, the
// positive runs here would fail while every parser/registry unit test stayed
// green.
func TestRunAddDirDispatchForwardsGrantIntoExecScope(t *testing.T) {
for _, tc := range []struct {
name string
args func(extra string) []string
}{
{
name: "exec subcommand",
args: func(extra string) []string {
return []string{"--add-dir", extra, "exec", "--skip-permissions-unsafe", "write the note"}
},
},
{
name: "prompt flag",
args: func(extra string) []string {
return []string{"--add-dir", extra, "-p", "write the note", "--skip-permissions-unsafe"}
},
},
} {
t.Run(tc.name, func(t *testing.T) {
t.Setenv("XDG_DATA_HOME", t.TempDir())
cwd := t.TempDir()
extra := t.TempDir()
target := filepath.Join(extra, "granted.txt")
exitCode, stderr := runExecAddDirWriteProbe(t, cwd, tc.args(extra), target)
if exitCode != exitSuccess {
t.Fatalf("exit = %d, want %d; stderr = %q", exitCode, exitSuccess, stderr)
}
content, err := os.ReadFile(target)
if err != nil {
t.Fatalf("expected write inside --add-dir root to land, read %s: %v", target, err)
}
if string(content) != "granted" {
t.Fatalf("content = %q, want %q", string(content), "granted")
}
// Negative control: the SAME run without --add-dir must be denied —
// the grant is the only thing that widens the scope.
denied := filepath.Join(extra, "denied.txt")
deniedArgs := tc.args(extra)[2:]
deniedExit, deniedStderr := runExecAddDirWriteProbe(t, cwd, deniedArgs, denied)
if deniedExit != exitSuccess {
t.Fatalf("denied-run exit = %d, want %d (tool denial is not fatal); stderr = %q", deniedExit, exitSuccess, deniedStderr)
}
if _, err := os.Stat(denied); !os.IsNotExist(err) {
t.Fatalf("write outside the workspace without --add-dir must not land, stat err=%v", err)
}
})
}
}
// runExecAddDirWriteProbe dispatches the given root argv through runWithDeps
// with a provider that calls write_file on target, then answers. It returns
// the exit code and stderr.
func runExecAddDirWriteProbe(t *testing.T, cwd string, args []string, target string) (int, string) {
t.Helper()
arguments, err := json.Marshal(map[string]string{"path": target, "content": "granted"})
if err != nil {
t.Fatalf("marshal write_file arguments: %v", err)
}
var stdout bytes.Buffer
var stderr bytes.Buffer
exitCode := runWithDeps(args, &stdout, &stderr, appDeps{
getwd: func() (string, error) {
return cwd, nil
},
resolveConfig: func(_ string, _ config.Overrides) (config.ResolvedConfig, error) {
return execResolvedConfig(), nil
},
newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) {
return toolCallingExecProvider{
toolCallID: "call_write_extra",
toolName: "write_file",
arguments: string(arguments),
answer: "done",
}, nil
},
newSandboxStore: func() (*sandbox.GrantStore, error) {
return sandbox.NewGrantStore(sandbox.StoreOptions{FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json")})
},
})
return exitCode, stderr.String()
}
// TestExecScopeReRegistrationSwapsCoreToolsByName pins the mechanism runExec
// relies on: a nil-scope core registry is built first (so --list-tools and
// tool-filter validation work before config resolve), then once the run scope
// is known the scoped core tools are re-registered and Registry.Register
// replaces the earlier instances BY NAME. The before/after write_file probes
// prove both the overwrite and the scoped enforcement it ships.
func TestExecScopeReRegistrationSwapsCoreToolsByName(t *testing.T) {
root := t.TempDir()
extra := t.TempDir()
inside := filepath.Join(extra, "inside.txt")
registry := newCoreRegistry(root)
// Before re-registration the tools carry a nil scope, so an absolute path
// inside the extra root (but outside the workspace) must be denied.
denied := registry.RunWithOptions(context.Background(), "write_file", map[string]any{
"path": inside,
"content": "too early",
}, tools.RunOptions{PermissionGranted: true})
if denied.Status == tools.StatusOK {
t.Fatalf("nil-scope registry must deny extra-root write, got ok: %s", denied.Output)
}
if _, err := os.Stat(inside); !os.IsNotExist(err) {
t.Fatalf("expected inside file to remain absent before re-registration, stat err=%v", err)
}
// Re-register exactly like exec.go does once the run scope is resolved.
scope, err := sandbox.NewScope(root, []string{extra})
if err != nil {
t.Fatalf("NewScope: %v", err)
}
for _, tool := range tools.CoreToolsScoped(root, scope) {
registry.Register(tool)
}
allowed := registry.RunWithOptions(context.Background(), "write_file", map[string]any{
"path": inside,
"content": "granted",
}, tools.RunOptions{PermissionGranted: true})
if allowed.Status != tools.StatusOK {
t.Fatalf("scoped registry must allow extra-root write, got %s: %s", allowed.Status, allowed.Output)
}
content, err := os.ReadFile(inside)
if err != nil {
t.Fatalf("read %s: %v", inside, err)
}
if string(content) != "granted" {
t.Fatalf("inside content = %q, want %q", string(content), "granted")
}
// A path outside every scope root must still be denied after re-registration.
outside := filepath.Join(t.TempDir(), "outside.txt")
deniedOutside := registry.RunWithOptions(context.Background(), "write_file", map[string]any{
"path": outside,
"content": "never",
}, tools.RunOptions{PermissionGranted: true})
if deniedOutside.Status == tools.StatusOK {
t.Fatalf("write outside all roots must fail, got ok: %s", deniedOutside.Output)
}
if !strings.Contains(deniedOutside.Output, "must stay inside the workspace") {
t.Fatalf("expected outside-workspace denial, got %q", deniedOutside.Output)
}
if _, err := os.Stat(outside); !os.IsNotExist(err) {
t.Fatalf("expected outside file to remain absent, stat err=%v", err)
}
}