fix(sandbox): protect daemon token file

[PierrunoYT]

Summary

Protect the remote daemon's file-backed bearer credential from sandboxed commands, in-process file tools, and pre-mutation session checkpoints.

Fixes #677.

The configured filename is treated as literal pathname data, including whitespace. Both its configured spelling and startup-resolved target remain protected, and the daemon passes the opened file's stable identity to workers. File tools check the actual opened object, so permission grants, hard-link aliases, and rotation do not bypass the credential boundary. Reads and mutations use rooted filesystem operations.

Shell behavior

Linux permits the existing safe placement on a filesystem separate from shell-writable roots, provided the credential has no additional hard links and its protected pathname still names the startup object. Rotation that renames the live token and replaces or removes its pathname now fails admission; restoring the original object restores admission. This sequential lifecycle check does not introduce cross-process locking around rotation and sandbox startup.

macOS and Windows require the inline ZERO_DAEMON_REMOTE_TOKEN for sandboxed shell execution with remote authentication. In-process file tools retain their file-token protections. The existing CLI help documents the Linux and macOS restrictions.

Latest review fixes

• Check Linux shell admission against the captured startup file identity before applying the link-count/filesystem exception.
• Exclude protected credentials from write_file and edit_file checkpoint targets. The checkpoint reader independently opens through the workspace root and checks the same handle it reads, preventing a rotated alias from being copied into an unprotected checkpoint blob.
• Carry disambiguated no-prefix rename/copy paths into subsequent hunk-header validation. Spaced filenames with content changes work while contradictory headers remain rejected.
• Isolate the whitespace planner test's home, configuration, cache, credential overrides, and token handoff markers.

The branch contains current main (c1937dfa).

Regression evidence

New tests were run with the fixes and against the original implementation in an isolated Linux copy. Without the fixes:

• Linux rotation admission returned no error instead of refusing the replaced startup object.
• Checkpoints captured the protected alias into a content-addressed blob; both the exec recorder and TUI callback tests failed before their denied mutation.
• Rename and copy hunks failed with ---/+++ paths disagree with diff --git paths from line 1.
• The original whitespace planner test created the caller's .config/zero; the fixed test left it absent.

Coverage includes missing/restored token paths, rotated hard-link aliases, both mutation callbacks, reading persisted session files through read_file, and ordinary checkpoint/rewind tests in the existing suite.

Validation

• make fmt-check (Linux) and git diff HEAD --check: pass; new callback test files also checked directly with gofmt.
• go vet ./...: pass.
• go test ./... on Linux and go test ./... -count=1 on Windows: pass. Windows validation clears the inherited ZERO_PROVIDER override for the test process.
• Focused regression tests on Windows: pass.
• Focused -race regression tests on Linux: pass, including both checkpoint callbacks. The broader sandbox and sessions race suites also pass.
• go run ./cmd/zero-release build and smoke on Linux and Windows: pass.
• make vulncheck on Linux and Windows: no vulnerabilities found.
• Advisory make lint-static: four existing Linux findings and seven existing Windows findings, all in unchanged files.

Additional validation limits: the broader race suite exposes a race in TestWebFetchUnicodeProxyHostnameIsStillTheProxy (web_fetch_proxy_test.go:126/133), reproduced on the unchanged PR head. Windows -race could not build with the installed cgo toolchain. Linux rotation coverage exercises admission; native Bubblewrap execution was unavailable on this host. No native macOS enforcement test was run locally.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The daemon token file is canonicalized before remote serving, added to mandatory sandbox protections, excluded from search and file tools, and removed from spawned command environments. Patch parsing now fails closed for ambiguous paths. Tests cover platform enforcement and path edge cases.

Changes

Daemon token protection

Layer / File(s)	Summary
Token file canonicalization internal/remotetoken/*, internal/daemon/remote/*, internal/cli/daemon*	Token-file paths preserve meaningful whitespace, resolve symlinks, persist configured and resolved identities, and fail closed when the selected file cannot resolve.
Sandbox credential protection internal/sandbox/pathlists.go, internal/sandbox/profile.go, internal/sandbox/engine.go, internal/sandbox/*test.go	The selected daemon token is a mandatory read-deny path. Allow rules, disabled policies, aliases, case variants, and directory traversal cannot expose or modify it.
Platform enforcement and runtime hardening internal/sandbox/linux_helper.go, internal/sandbox/manager.go, internal/sandbox/runner.go, internal/sandbox/filesystem_*	Bubblewrap validates mandatory paths and rejects unsafe symlinks. Command planning rejects linkable token paths. Seatbelt adds targeted write denials and scrubs all daemon token environment variables.
Patch path safety internal/sandbox/risk.go, internal/tools/apply_patch.go, internal/tools/mutation_targets.go, internal/tools/*patch*test.go	Patch paths preserve whitespace and undergo shared Git metadata validation. Ambiguous or malformed patches fail before mutation.
Tool and MCP integration internal/tools/list_directory.go, internal/tools/read_exclusions.go, internal/mcp/*, internal/tools/*test.go	Directory, search, file, patch, and MCP operations apply protected credential exclusions while retaining ordinary files and nested allowed reads.

Estimated code review effort: 5 (Critical) | ~100 minutes

Suggested reviewers: gnanam1990, anandh8

Merge Risk: 🟠 High · up to 6e716

This PR strengthens daemon-token protection across child processes and file tools, but concurrent filesystem changes can still expose or overwrite the token during protected reads and writes. The security boundary is therefore not safe to merge until those race conditions are addressed.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #677 by scrubbing token-file variables, protecting selected paths, enforcing denial across tools and sandboxes, and adding regression tests.
Out of Scope Changes check	✅ Passed	The changes remain focused on daemon token protection, enforcement boundaries, platform behavior, path handling, and related regression coverage.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: protecting the daemon token file across sandbox and tool execution paths.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR closes a sandbox escape where ZERO_DAEMON_REMOTE_TOKEN_FILE could be inherited by sandboxed commands (allowing them to locate and read the daemon bearer token file under the read-all posture). It scrubs the pointer env var across platforms and extends the existing “credential deny-read” profile logic to also deny reads of the referenced token file where deny-read enforcement is supported.

Changes:

• Scrub ZERO_DAEMON_REMOTE_TOKEN_FILE from sandbox command environments (in addition to the inline token env var).
• Extend credentialDenyReadPaths to include the path named by ZERO_DAEMON_REMOTE_TOKEN_FILE (alongside GOOGLE_APPLICATION_CREDENTIALS) and plumb this through the pure helper.
• Add/extend regression tests covering env scrubbing and permission-profile deny-read construction (skipping the deny-read assertion on Windows per existing platform limitations).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
internal/sandbox/runner.go	Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to the sandbox env scrub list.
internal/sandbox/runner_test.go	Extends env scrubbing regression test to ensure the pointer env var is removed.
internal/sandbox/profile.go	Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to default credential deny-read path construction and updates helper signature/docs.
internal/sandbox/manager_test.go	Updates credential deny-read tests for the new parameter and adds a profile-level regression test for daemon token file denial (non-Windows).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Deny writes to the daemon token file on macOS as well
  internal/sandbox/profile.go:176
  The new target enters DenyRead, but the Seatbelt backend translates that only into file-read* and unlink denials. Its broad file-write* allowance still covers every workspace root and the default temporary roots. Therefore, when ZERO_DAEMON_REMOTE_TOKEN_FILE names a file under /tmp or another writable root, a sandboxed command can discover the filename from its parent directory and overwrite or truncate the bearer-token file. This makes the remote bridge unavailable and can replace its credential on a restart/reload. Add a write denial for credential DenyRead files in the Seatbelt profile (and a macOS regression case for a token under a writable temporary root).

[anandh8x]

Re-reviewing against commit 2248aca8 (head). The macOS Seatbelt fix (patch 2/2) is the right primitive: a DenyRead file that's also under a writable root was overwritable/truncatable because the prior profile only emitted file-read* and file-write-unlink, not file-write*. Denying the full write direction for every DenyRead path is correct, the ordering (deny after the broad allow) is correct, and TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot covers both the rule presence and the ordering. The TestSeatbeltProfileProtectsMetadataAndDenyOrdering extension covers the general case.

LGTM.

Cross-PR note: #685 depends on the credentialDenyReadPathsIn signature change from #681 (daemon token file as a parameter) and the scrubSensitiveEnv plumbed sensitiveEnvKeys from #682. Recommend rebasing #685 onto #681 + #682 in that order.

[gnanam1990]

Local review: built and ran go test ./internal/sandbox on darwin/arm64; all pass. The deny-write-for-DenyRead fix is a genuine security improvement (closes the truncate/overwrite bypass under a writable root). One integration note.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Protect the configured symlink pathname as well as its target
  internal/sandbox/profile.go:200
normalizeProfilePaths resolves ZERO_DAEMON_REMOTE_TOKEN_FILE through symlinks before it is added to DenyRead. If the configured pathname is a symlink under a writable root such as /tmp, the new deny rules protect only its current referent; a sandboxed command can unlink the writable symlink and recreate a regular file at the configured pathname. On the next remote-daemon start, TokenFromEnv reads that replacement pathname and accepts the attacker-chosen bearer token (or fails, causing a denial of service). Preserve and deny the lexical configured path in addition to its resolved target, and add a symlink-replacement regression test.

[Vasanthdev2004]

Approving clean security hardening. Scrubbing ZERO_DAEMON_REMOTE_TOKEN_FILE from child envs and adding its target to the credential deny-read set closes a real hole (a sandboxed command could otherwise resolve the pointer and read the daemon bearer-token file under the read-all posture), and extending the macOS seatbelt profile to file-write*-deny every DenyRead path is the right fix: denyReadRules only blocked read and unlink, leaving a credential file under a writable root overwritable/truncatable. I checked the Linux bubblewrap path and it already bind-mounts DenyRead targets read-only, so this just brings macOS to parity. One thing to be aware of: the write-deny now covers all DenyRead paths (~/.aws, ~/.azure, etc.), so no sandboxed command can update cloud creds consistent with the existing unlink-deny and fine under the current threat model, just calling it out.

[coderabbitai]

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/sandbox/profile.go`:
- Around line 320-328: Keep normalizeProfilePath purely lexical by removing its
filepath.EvalSymlinks resolution and returning the result of
normalizeProfilePathLexical unchanged. Resolve symlinks only within
normalizeProfilePathVariants while retaining both the configured lexical path
and resolved target for deny-policy expansion, and add a regression test
covering a writable denied symlink.

🪄 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: 974aa02c-d6a1-45e8-ae0b-c2df72771e98

📥 Commits

Reviewing files that changed from the base of the PR and between 8533492 and 5619a29.

📒 Files selected for processing (4)

• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/sandbox/runner.go
• internal/sandbox/runner_test.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Do not pass a lexical symlink to Bubblewrap's deny mount
  internal/sandbox/profile.go:200
  For an existing ZERO_DAEMON_REMOTE_TOKEN_FILE symlink, the new variant list includes the symlink pathname as well as its target. The Linux backend then emits --ro-bind /dev/null <symlink> for that pathname; Bubblewrap rejects a symlink mount destination before the command starts (Can't create file at .../daemon-token: No such file or directory). Thus configuring the supported token-file option through a symlink makes every Linux sandboxed command fail to launch. Materialize/protect that pathname with a Bubblewrap-safe mechanism (or avoid adding it to the Linux deny-mount list) and add a Linux regression test.

• [P1] Resolve the token-file path in the daemon's context, not each worker's
  internal/sandbox/profile.go:195
TokenFromEnv accepts relative token paths, and serve-remote reads one before it starts workers. The daemon then preserves ZERO_DAEMON_REMOTE_TOKEN_FILE for workers whose cmd.Dir is the per-session spec.Cwd; normalizeProfilePathLexical consequently turns token into a path beneath that session instead of the daemon startup directory that contains the actual bearer-token file. The real file is left outside DenyRead under the read-all posture, so a sandboxed command that can infer its location can read it. Normalize the value at the daemon boundary (or pass an already-absolute protected path) and cover a remote worker whose session CWD differs from the daemon CWD.

[Vasanthdev2004]

Following up on my earlier approve, which I am pulling back from for now. jatmn's latest P1 is a real one: the symlink-protection commit adds the ZERO_DAEMON_REMOTE_TOKEN_FILE symlink pathname itself, not just its resolved target, to the Linux deny-mount list, and Bubblewrap rejects a symlink as a mount destination, so every sandboxed command on Linux fails to launch when that option points at a symlink. I am on Windows and cannot reproduce the bwrap behavior here, but jatmn tested it on Linux with the exact "Can't create file ... daemon-token" error and the mechanism is sound. The target protection and the macOS write-deny are still the right hardening. This just needs the Linux side to protect that pathname without ro-binding the symlink itself (materialize it, or keep the symlink pathname off the Linux deny-mount list). Not re-approving until that is closed.

[coderabbitai]

Actionable comments posted: 2

🤖 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/sandbox/linux_helper.go`:
- Around line 319-324: Add the same lexical-symlink guard used in the DenyRead
path to appendReadOnlyLinuxPathArgs, checking the mount path with os.Lstat and
returning the existing args unchanged when it is a symlink. Keep the current
handling for non-symlink paths unchanged.

In `@internal/sandbox/profile.go`:
- Line 325: The FileSystemPolicy initializers in PermissionProfileFromPolicy and
seatbeltCompatibilityPermissionProfile must preserve both lexical and resolved
paths for user deny policies. Replace single-path normalization for
policy.DenyRead and policy.DenyWrite with normalizeProfilePathVariants, while
leaving normalizeProfilePath unchanged for other uses.

🪄 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: cbb0b9b3-3559-4c77-bb5a-2c1692650e7a

📥 Commits

Reviewing files that changed from the base of the PR and between 5619a29 and 5cd8009.

📒 Files selected for processing (7)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/sandbox/runner_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/sandbox/runner_test.go
• internal/sandbox/manager_test.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve the resolved target for user-configured DenyRead symlinks
  internal/sandbox/profile.go:104
normalizeProfilePath is now lexical-only, while this initializer still uses normalizeProfilePaths for policy entries. On Linux, appendUnreadableLinuxPathArgs then skips that symlink mount destination and no resolved target is present (unlike the credential-path branch). Thus a policy such as denyRead: [link], where link points to a secret, produces no deny mount under the read-all profile and the sandboxed command can read the target. Keep both variants for deny paths (and update the macOS compatibility initializer) so the Bubblewrap-safe target is actually denied.

• [P1] Do not use lexical paths for ordinary sandbox roots
  internal/sandbox/profile.go:324
  This changed the shared normalizer used for workspaceRoot, AllowWrite, and DenyWrite, not just the new credential deny variant. A workspace opened through a symlink now reaches Linux Bubblewrap as --bind <link> <link>; Bubblewrap rejects a symlink mount destination, so every sandboxed command fails before it starts. I reproduced the failure with a symlinked workspace. Restore resolved normalization for ordinary roots and keep lexical-plus-resolved handling scoped to deny-path expansion.

• [P1] Do not leave a writable token-file symlink unprotected on Linux
  internal/sandbox/linux_helper.go:319
  Skipping the lexical symlink avoids Bubblewrap's invalid mount destination, but only its original target is masked. If ZERO_DAEMON_REMOTE_TOKEN_FILE is a symlink under a writable root such as /tmp, a sandboxed command can replace it with a link to another host-readable file and read through the replacement; it can also corrupt the daemon's token path. The test currently asserts the unsafe omission. Protect or materialize the lexical pathname with a Bubblewrap-safe mechanism rather than simply dropping its deny rule.

• [P1] Handle symlinked parent directories before emitting a deny mount
  internal/sandbox/linux_helper.go:319
  The Lstat check catches only a final-component symlink. For a supported token path such as /tmp/linkdir/token, where linkdir is a symlink, Lstat(token) reports a regular file and the helper emits a deny mount through the symlinked parent. Bubblewrap rejects that destination and every Linux sandbox launch fails. Detect path traversal through a symlink (or omit the lexical variant after retaining the resolved target) and add a regression case for this layout.

[coderabbitai]

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/sandbox/linux_helper.go`:
- Around line 323-349: The Linux path argument helpers currently abort on
lexical symlinks instead of skipping them when their resolved target is also
protected. Update the profile-processing flow around appendReadOnlyLinuxPathArgs
and appendUnreadableLinuxPathArgs to recognize lexical symlink entries whose
resolved targets exist in the same deny set, skip those entries, and continue
enforcing the target; retain the existing error behavior when no enforceable
target is present. Update the related test to assert successful sandbox startup
and target enforcement.

🪄 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: baa5d0ce-25e0-42a5-8752-15ae141e7d1d

📥 Commits

Reviewing files that changed from the base of the PR and between 5cd8009 and a9da4ff.

📒 Files selected for processing (7)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/sandbox/runner.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/cli/daemon.go
• internal/sandbox/runner.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Keep the remote token excluded from in-process file tools
  internal/sandbox/profile.go:104
  The new daemon-token path is added only to PermissionProfile.FileSystem.DenyRead, which protects wrapped shell commands. Built-in tools do not consume that profile: read_file reads scoped files directly, and grep/glob exclusions are built from Policy.DenyRead. If the token file is inside a remote session workspace (for example, a daemon started with a relative token-file path from that workspace), a remote-controlled agent can use read_file to exfiltrate the bridge bearer token. Apply the automatic credential exclusion to the in-process read/search tool boundary as well, and cover this with an end-to-end tool test.

• [P1] Preserve inline-token precedence when a token-file variable is stale
  internal/cli/daemon.go:480
TokenFromEnv intentionally returns a nonempty ZERO_DAEMON_REMOTE_TOKEN before consulting ZERO_DAEMON_REMOTE_TOKEN_FILE, but this new preflight resolves the file first. Consequently, a valid inline token plus an inherited missing or dangling token-file variable now makes daemon serve-remote exit instead of starting. Only canonicalize the file when it is the selected source (or otherwise leave an ignored file pointer from changing the result), and add the both-variables regression case.

• [P1] Do not make symlink-backed credential paths disable every Linux sandbox command
  internal/sandbox/linux_helper.go:344
  The profile now deliberately retains both lexical and resolved forms of every credential/deny path, but the Linux argument builder aborts whenever either form has a symlink component. This makes common configurations such as GOOGLE_APPLICATION_CREDENTIALS=/var/run/... (where /var/run is commonly a symlink to /run) fail plan construction for every sandboxed command; the pre-PR profile kept only the resolved target. Preserve the denial of the resolved target while using a Bubblewrap-safe treatment for the lexical path instead of turning a valid credential configuration into a global sandbox-startup failure.

[coderabbitai]

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/sandbox/engine.go`:
- Around line 57-75: Update withAutomaticDenyRead to recompute automaticDenyRead
from the current effective policy before merging it with policy.DenyRead, rather
than reusing the constructor-time list. Ensure credential paths allowed through
session or turn permission profiles are removed from the automatic deny set
while preserving deduplication.

🪄 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: 059619b5-dbbc-4812-a361-6fad61cca69c

📥 Commits

Reviewing files that changed from the base of the PR and between 2a0e63e and 4db4c6f.

📒 Files selected for processing (6)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/engine.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/tools/read_exclusions_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/cli/daemon.go
• internal/sandbox/linux_helper.go

[PierrunoYT]

PierrunoYT pushed the review fixes in 682e23ff. The branch also includes the current upstream main fetched for this update.

Review findings addressed

• Agent-loop apply_patch preflight now derives PatchPaths through the same preparation/parser helper used by execution. Ordinary patches allow; protected metadata patches prompt rather than deny; malformed input fails closed. The request also carries the paths into risk classification.
• The unified executor now rejects contradictory diff --git, rename/copy, and unified headers, with matching-header controls and isolated Git fixture configuration.
• Formatters operate on detached private staging files, inherit the centralized scrubbed environment, and publish through the protected rooted-write primitive. Post-write tracker/preview reads and production diagnostics use credential-checked opened handles rather than unrestricted path reopens.
• Startup authentication bytes and stable protection identity are captured from the same opened token file. The identity is carried to workers and checked against consumed handles, including Windows volume/file identity, so an alias of the startup object remains denied after atomic pathname replacement.

Regression evidence

• Without the preflight fix, the producer-side test returned deny with "patch paths were not supplied by the apply_patch executor" for both ordinary and protected metadata paths.
• With header-agreement enforcement disabled, the rename, copy, update, create, and delete disagreement regressions failed by accepting contradictory targets; matching controls remain executable.
• Removing startup identity persistence reproduced read_file and MCP disclosure through the retained startup hard-link alias.
• Unfixed post-write/diagnostics regressions reproduced token bytes reaching diagnostics and the LSP checker. Fixed tests cover symlink/hard-link swaps, formatter publication, tracker/preview exclusion, child-environment scrubbing, and ordinary-file controls.

Validation

Passed: make fmt-check; go vet ./...; go test ./...; affected-package race tests for tools, agent, sandbox, MCP, and remote authentication; release build and smoke; make lint-static (0 issues); make vulncheck (no vulnerabilities); git diff HEAD --check. Windows/macOS amd64 cross-compilation passed; native platform execution remains for CI. POSIX external-formatter fixtures skip on Windows, while pure-Go alias tests are not blanket-skipped there.

Behavior note

Private staging preserves the original formatter working directory, but formatters that discover settings solely from the input file’s ancestor directories may resolve configuration differently. This tradeoff is documented in the formatter implementation; formatting and diagnostics are not globally disabled when a token is configured.

[jatmn]

@Vasanthdev2004 lgtm

[Vasanthdev2004]

Re-reviewed at 682e23ff. My three findings are closed, and I drove each one. One thing new in this head needs to change before it goes in.

Closed. sandboxRequest now derives PatchPaths with the executor's own parser, and TestSandboxRequestApplyPatchPreflight is the producer-side test I asked for: an agent-shaped request through the real engine allows notes.txt, prompts for .agents/notes.md, and the prompt is offered for approval. Dropping the field population fails it. The header cross-check is back in parseUnifiedPatch, stricter than before, with the --no-prefix ambiguity resolved through the extended headers; I ran prefixed, no-prefix, spaced and plain forms and they all parse, and the three contradiction tests refuse what they should. Forcing the ---/+++ comparison to match fails the git-header and executor-path ones. The identity pinning for the token file reads correctly to me, including the Windows handle path, and the read-side consumers all go through the handle check now.

Format-on-write no longer formats to the project's style. The hardening added since my review stages the formatter's input in a temp directory outside the workspace and hands the formatter that path. Most formatters in formatterCommands resolve their configuration from the input file's location upward: prettier, ruff, rustfmt, clang-format, shfmt, ktlint, swiftformat. Staged under the temp directory, none of them can see .prettierrc, pyproject.toml, rustfmt.toml, .clang-format or .editorconfig, so they format with their built-in defaults. I drove it: a workspace with .clang-format setting IndentWidth: 8, then write_file of a .c file with ZERO_FORMAT_ON_WRITE=1:

through write_file at this head     two-space indent    clang-format's default LLVM style
clang-format on the real path       eight-space indent  what the project asks for

The feature's stated purpose is that output lands in project-canonical style and never fails a CI format check it cannot see. At this head it does the opposite for those projects: it rewrites bytes the model wrote into a style the project's CI will reject, and nothing tells anyone. The doc comment on maybeFormatWrittenFile already says settings from the file's ancestors may differ; that sentence is the finding.

The fix keeps the security property and drops the staging file entirely: feed the content on stdin and read stdout, passing the destination path only as the filename hint each formatter provides for exactly this purpose (prettier --stdin-filepath, ruff format --stdin-filename, clang-format --assume-filename, shfmt --filename, stylua --stdin-filepath, swiftformat --stdinpath, dart format --stdin-name, ktlint --stdin; gofmt, rustfmt, zig fmt --stdin, gleam format --stdin and terraform fmt - read stdin natively). The formatter then never opens the destination pathname at all, which is stronger than the temp copy, and configuration resolves from the real location. Publish the stdout through writeRootedFile as now, and pin it with a test like the one above: a project config the default style would not produce.

CI 6 of 6 at head. Locally the packages pass apart from two stale-base cases: the serve symlink test that #1042 fixed on main after this branch last merged it, and the eager schema budget test, whose accounting main changed in #1017 (the schemas here total the same 3653 tokens main reports). The branch conflicts with main as well.

[jatmn]

I found four issues that need to be addressed before this is ready.

Reviewed head: 8c01bb9b35c252ad89add0ba74a9293836e34e31. The branch contains the captured current main (c1937dfac72e6ad0e5ade6e48e2d9c17d9c3e5d6), is conflict-free, and has passing hosted checks. The findings below concern this head.

Findings

• [P1] Preserve the live startup credential in Linux shell admission
  internal/sandbox/manager.go:362

  The startup-identity request is not fully addressed for shell execution. This preflight checks the link count and filesystem of the file currently at the configured/resolved pathname, but never checks that it is the object identified by ZERO_INTERNAL_DAEMON_REMOTE_TOKEN_FILE_IDENTITY.

  Start the daemon with a single-link token on a dedicated filesystem, outside every shell-writable root and broader credential-directory deny. Then rename token to token.old and create a new regular token. Both files have one link, so the Linux separate-filesystem exception permits the next shell. The read-all root exposes token.old, while the exact /dev/null bind masks only the replacement token. NewAuthenticatorFromEnv keeps accepting the original bytes, as its replacement test explicitly verifies. The shell can therefore read a bearer that still controls the running daemon.

  This is a sequential rotation case, not a demand for cross-process locking. Please keep shell execution fail-closed when the startup object is no longer covered—for example, when the current pathname no longer matches its captured identity—while preserving the safe unrotated placement. Add the rotation lifecycle to the Linux admission coverage. A native fixture should use a separate mount outside /dev; the synthetic /dev makes /dev/shm unsuitable for proving the read-back behavior.

• [P1] Keep pre-mutation checkpoints inside the token boundary
  internal/tools/mutation_targets.go:25

  The new protection-aware target filter is only in the apply_patch branch; write_file and edit_file still return an in-workspace token here. agent/loop.go invokes OnToolCall before executeToolCall; the CLI recorder and TUI callback then call the session checkpoint code. SnapshotForCheckpoint reads the target with os.ReadFile and stores its bytes in a new content-addressed blob before the new tool gate can refuse the mutation. Remote daemon workers use the stream-JSON exec path that enables this recording.

  With an active token, I verified that direct token reads and both subsequent mutations are denied, yet both checkpoint paths store the bearer. A later read_file with access to the session-data directory returns it from the blob, whose pathname and inode are outside the protected set. That read-back requires access to the session directory; it does not require permission to read the token itself. Forking a session also copies its checkpoint blobs.

  The checkpoint reader predates this PR, so this is an incomplete file-tool protection fix, not a newly introduced storage bug. Please prevent credential bytes from entering checkpoints, binding the decision to the object actually read so an alias swap cannot defeat an early pathname filter. Cover both writer callbacks, a denied mutation, and subsequent blob visibility, while retaining ordinary checkpoints and rewind behavior.

• [P2] Retain the disambiguated rename/copy paths for hunk validation
  internal/tools/unified_patch.go:287

  For an unquoted --no-prefix rename such as old name.txt to new name.txt, DiffGitPaths returns an ambiguous result. The extended headers correctly resolve it here by checking their exact concatenation, but diffOldPath and diffNewPath remain empty. When the patch also changes content, the following ---/+++ pair reaches startFile, which compares those paths against the empty pair and rejects the valid patch.

  A real git diff --cached --no-prefix -M fixture with that rename and one changed line fails with ---/+++ paths disagree with diff --git paths from line 1; the same fixture is accepted at the base. Header-only rename tests miss this case, and copies use the same branch. Since preparation feeds the agent gate and executor, this prevents the actual tool call. Please carry the validated pair into the following consistency check, preserving rejection of contradictory headers, and add rename/copy cases with hunks.

• [P2] Isolate the whitespace planner test from developer configuration
  internal/sandbox/protected_credentials_test.go:333

  TestProtectedCredentialFilenameWhitespaceReachesOSSandbox creates its token under t.TempDir, but builds the rest of the profile from the real process environment. Its call to mustBuildLinuxBwrapFilesystemPlan reaches ensureLinuxDenyReadDirs, which creates the process's Zero config directory when absent. The test never redirects or cleans that directory. It runs on both Linux and macOS.

  Running only this test with a previously absent, isolated XDG_CONFIG_HOME leaves its zero directory behind after the test passes. Without that external isolation, the same effect targets the developer's configuration. This violates the explicit hermetic-test requirement in AGENTS.md. Please isolate all environment-derived roots the planner can materialize, or construct an isolated profile, while retaining the exact-whitespace assertion.

The environment-pointer fix in merged #818 does not supersede this PR’s broader file/object protections. Open #1011 overlaps the Linux masking implementation but addresses SSH/GPG discovery and does not close these daemon-token paths.

Focused race tests, formatting, vet, and builds passed, including Windows/macOS cross-builds. The broader local socket-dependent suite and native platform enforcement were not fully exercised; passing hosted checks do not cover the failure paths above.

[jatmn]

I found issues that need to be addressed before this is ready.

Merge readiness

• [P1] Rebase onto current main and resolve merge conflicts
  GitHub reports mergeable: false against main. A merge-tree of this head onto current origin/main conflicts in workflow checkout steps (for example .github/workflows/pr-auto-review.yml and release-artifacts.yml adding persist-credentials: false). Rebase or merge main, resolve those conflicts, and re-run CI on the updated head before merge.

Findings

• [P1] Apply the handle-bound protected-credential read gate to every in-process read tool this PR leaves on pathname-only opens
  Attribution: Incomplete claimed fix. Merge-base had no credential gate; this PR introduces ProtectedReadOpen / FileHandleExcluded across read_file, grep, MCP resources/read, diagnostics, and checkpoints, but two registry read tools still read through os.ReadFile / os.Open after path confinement only.
  Stated contract: PR body — protect the credential from "in-process file tools"; internal/tools/protected_credentials.go — mandatory engine-independent boundary for direct file tools; internal/sandbox/pathlists.go — tool reads must bind the exclusion to the opened handle, not a second pathname resolution.
  Root cause: the new handle-bound read contract is not wired through every direct file-read entry point. read_file and grep were updated; lsp_navigate and view_image were not.
  What fails: when the protected token file lives inside the workspace (the layout used throughout daemon_token_* tests), read_file refuses with "never readable", but lsp_navigate still loads file bytes via readWorkspaceFile → os.ReadFile, and view_image still loads bytes via imageinput.LoadFile → os.Stat + os.Open. A concurrent symlink/hard-link swap between confinement and open can follow the same TOCTOU class this PR closes elsewhere; with a language server available, lsp_navigate also passes those bytes into NavRequest.Text.
  In this PR (must close together):
  • internal/tools/lsp_navigate.go (readWorkspaceFile, Run) — not gated
  • internal/tools/view_image.go (Run → imageinput.LoadFile) — not gated
  • internal/imageinput/imageinput.go (LoadFile) — pathname open without FileHandleExcluded
  • Regression tests: extend TestDaemonTokenProtectionMatrix / MCP daemon-token tests to cover lsp_navigate and view_image (same workspace-token fixture as read_file)
    Unchanged on main (do not edit in this PR): unrelated tools that do not read arbitrary workspace files.
    Required correction: open through tools.ProtectedReadOpen (or equivalent sandbox.ProtectedCredentialExclusions(...).FileHandleExcluded on the consumed handle) before reading content in both tools; keep path confinement as-is. Add matrix rows that fail if either tool serves token bytes or succeeds where read_file is denied.
    Author fix: close the root cause on every "In this PR" row in one pass — implement the handle-bound gate in both tools and their shared loader, then extend the matrix/MCP tests. Do not patch only lsp_navigate or only view_image.
    Out of scope: rebuilding LSP/image subsystems, macOS/Windows shell inode-alias limitations already documented for sandboxed shell, or protecting a stale on-disk token file when inline ZERO_DAEMON_REMOTE_TOKEN is intentionally selected (CLI already documents that case).

[jatmn]

@Vasanthdev2004 lgtm

[Vasanthdev2004]

Re-reviewed at 62d97eaf. My finding is closed. With a .clang-format that sets IndentWidth: 8, write_file of a .c file now lands with eight-space indents, the same bytes main produces. That is the real clang-format, not the helper. The commits since my last pass (8c01bb9b, d519cd31, 6954e2f9, 62d97eaf) read fine to me, and tools, sessions, imageinput, mcp, sandbox, cli and daemon all pass here on Windows.

The stdin route opened two things up, though, and both are as much on me as on you: I suggested that route and mentioned neither guard. Both reproduce at this head and not on main.

1. A formatter that prints nothing and exits 0 empties the file. maybeFormatWrittenFile publishes whatever stdout held. clang-format does exactly that for a path matched by .clang-format-ignore: in place it leaves the file alone, over stdin with --assume-filename it prints nothing and returns 0. Through write_file with ZERO_FORMAT_ON_WRITE=1, in a workspace whose .clang-format-ignore holds vendor/*:

                          main                     this head
create vendor/lib.c       26 bytes, as written     0 bytes, "Created vendor/lib.c (0 lines)."
overwrite vendor/big.c    26 bytes, as written     0 bytes

The tool reports success both times. Any formatter that answers "not mine" with silence does the same, and a stub that reads stdin and exits 0 reproduces it without clang-format installed. Right after formatted := stdout.Bytes(): if the output is blank and the input was not, return unformatted. No notice needed, nothing is wrong in that case. Please pin it with the silent stub and assert the bytes on disk.

2. The deadline no longer bounds the call when the formatter is a shim. Stdout is a pipe now, so Run waits for every holder of that pipe, not only the process the deadline kills. On Windows every npm-installed formatter is a .cmd shim: the deadline kills cmd.exe, node.exe keeps the pipe, and the tool call sits there until node exits. Modelled with a .bat hosting a nine second ping, deadline shortened to 500ms:

main         write_file returns after 500ms
this head    write_file returns after 8.1s

Main never had a stdout pipe, so it never waited. The package already has the fix: hardenProcessLifetime(formatter) right after the command is built gives it the tree kill and the WaitDelay that bash uses.

With both changes in locally the ignored file keeps its 26 bytes, the silent stub leaves the content alone, the shim case returns in 700ms with the timeout notice still firing, and the project-config case still formats to eight spaces.

CI is 9 of 9 at head.

[PierrunoYT]

Both fixed in 31fe0e4.

1. When the formatter's stdout is blank and the written content isn't, the written bytes stay as they are, with no notice. TestFormatOnWriteKeepsContentWhenTheFormatterPrintsNothing runs a stub that reads stdin, prints nothing and exits 0, through write_file for both create and overwrite, and checks the bytes on disk. On the unfixed code the file ends up "" in both cases.
2. hardenProcessLifetime(formatter) now runs right after the command is built. TestFormatOnWriteDeadlineBoundsAShimmedFormatter uses a .bat whose ping child holds stdout for about 9s, with the deadline set to 500ms. It returns in under a second and reports the timeout. On the unfixed code it took 9.1s.

Thanks for spelling out both guards.

[Vasanthdev2004]

Both are in, and both bite. With the blank-output guard taken out, TestFormatOnWriteKeepsContentWhenTheFormatterPrintsNothing fails on create and on overwrite with the file emptied on disk. Without hardenProcessLifetime(formatter), TestFormatOnWriteDeadlineBoundsAShimmedFormatter takes 9.1s against a 500ms deadline, and 0.7s with it. The other new commit makes the foreground server test poll for readiness instead of assuming it, which reads fine.

internal/tools passes here on Windows and CI is 9/9 at 90c043bd. Approving.

[jatmn]

LGTM

[euxaristia]

The protected-credential mechanism is the pattern AGENTS.md asks for: the identity check happens on the opened handle (root open then Stat on the same handle), both the configured spelling and the startup-resolved target are protected, Windows file identity uses volume-serial plus file-index, and unreadable preimages fail closed. Two asks before merge: the fallback in protectedReadOpen that roots at the parent directory when the scope walk fails deserves its own deny-probe test since it is a second resolution shape; and the diff collides heavily with #941 and #988 in the same tool files, so sequence deliberately. Self-declared untested halves: no native macOS enforcement test and no bwrap run on the author host.

[Vasanthdev2004]

Re-reviewed at 35757a9e, which adds one test on top of what I approved at 90c043bd. It covers @euxaristia's first ask. TestProtectedReadOpenParentFallback checks that the workspace walk refuses the path, so the read really takes the parent-directory fallback, then reads the ordinary file and swaps it for an alias of the bridge token. With the FileHandleExcluded check in protectedReadOpen turned off, the hard-link case fails here on Windows with "fallback returned a protected handle", so that check is what refuses it. The two symlink cases skip on this machine because I can't create symlinks without the privilege, so for those I'm relying on the Linux and macOS test runs. CI is 9/9 at head. Approving.

On the second ask: this overlaps #941 in six files (file_commit.go, write_file.go, edit_file.go and the format-on-write files) and #988 in write_file.go, so whichever of them lands later will need a rebase. That's a merge-order call, not a change for this PR.

[anandh8x]

LGTM. Verified the token file is protected by opened-handle identity rather than pathname — configured spelling and startup-resolved object both covered, hard links and rotation don't bypass, checkpoints and engine-less paths included. The new deny-probe coverage for the parent-root fallback and the mandatory deny-read failure mode are the right shape.

Note the diff collides with #941 and #988 in the tool files, so merge order is worth a deliberate call.