mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 23:40:28 -10:00
* 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]>
178 lines
6.5 KiB
Go
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)
|
|
}
|
|
}
|