fix: fail closed on interactive Keychain use in darwin workers

[shixi-li]

Problem

Non-interactive local/worktree worker clusters on macOS can pop GUI Keychain password prompts (SecurityAgent). buildSpawnEnv spawns workers with the inherited host environment, and task-lib/watcher.js passes it through to every worker grandchild, so a descendant like claude doctor probing Keychain writes via security -i reaches the logged-in user's GUI Keychain session.

Fixes #704

Fix (fail closed, option 1 from the issue)

On darwin only, worker spawn envs now get a managed shim directory (~/.zeroshot/keychain-shim) prepended to the exact PATH key. Its security wrapper:

• fails closed (exit 1) on interactive invocations — -i, -p (implies interactive), bundled global flags like -qi, or a zero-argument implicit interactive session — with a deterministic diagnostic naming the non-interactive worker boundary and pointing at Docker isolation or explicit credential configuration;
• transparently execs /usr/bin/security "$@" for every other subcommand, so provider authentication such as security find-generic-password keeps working (flag scanning stops at the first subcommand token, and exit codes pass through).

ZEROSHOT_ALLOW_INTERACTIVE_KEYCHAIN=1 opts out. If the shim cannot be installed, the spawn fails rather than silently re-exposing the interactive Keychain session.

The managed executable is refreshed through a same-directory temporary file, chmodded before an atomic rename over the live path, and cleaned up on publication failure. PATH handling preserves inherited empty components (the POSIX current-directory entry), distinguishes unset PATH from an explicitly empty PATH, and removes only duplicate shim entries.

Scope guarantees:

• Docker isolation never reaches buildSpawnEnv, so containerized runs are untouched.
• Non-darwin platforms get no shim.
• Worktree tool-bin directories stay ahead of the shim, so repo-managed tool substitutes keep winning.

Tests

• tests/unit/darwin-keychain-boundary.test.js now has 15 cases covering interactive blocking, non-interactive pass-through and exit propagation, opt-out behavior, exact PATH casing when Path also exists, empty/unset PATH semantics, idempotency, atomic publication failure, and non-darwin behavior.
• Both production spawn-env builders expose a narrow boundary-injection seam. Their tests force the darwin path on every CI platform, use real worktree tooling metadata, and launch a worker process whose PATH-resolved security -i grandchild must hit the shim rather than a logging fallback executable.
• Focused boundary + adjacent worktree/runner tests: 24 passing.
• Full unit suite, lint, and typecheck complete successfully.

Notes

A PATH shim intercepts PATH-resolved lookups; descendants invoking /usr/bin/security by absolute path bypass it. The reported probe resolves via PATH, so the defect in the issue is covered; this is a guardrail for well-behaved tools, not a sandbox against adversarial code.

[greptile-apps]

Greptile Summary

Adds a fail-closed macOS Keychain boundary for non-interactive workers.

• Installs an atomic, managed security shim that blocks interactive invocations while forwarding non-interactive subcommands.
• Applies the boundary to both local worker spawn-environment builders while preserving worktree tool precedence and Docker behavior.
• Adds coverage for invocation parsing, PATH semantics, publication failures, cleanup, and worker-grandchild integration.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/darwin-keychain-boundary.js	Implements atomic shim publication, interactive-flag blocking, opt-out handling, and PATH mutation for macOS workers.
src/agent/agent-task-executor.js	Applies the Keychain boundary when constructing non-isolated agent worker environments.
src/claude-task-runner.js	Applies the boundary in the task-runner environment path and cleans Claude settings overlays if setup fails.
src/worktree-tooling-env.js	Prefers the exact uppercase PATH key so worktree tools retain precedence over the managed shim.
tests/unit/darwin-keychain-boundary.test.js	Covers shim behavior, publication safety, PATH edge cases, opt-out behavior, and worker-grandchild integration.
tests/unit/claude-task-runner-worktree-env.test.js	Verifies settings-overlay cleanup when Keychain boundary installation fails.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Build local worker environment] --> B{macOS and no opt-out?}
  B -- No --> C[Preserve inherited environment]
  B -- Yes --> D[Install or refresh managed security shim]
  D --> E[Prepend shim to PATH]
  E --> F[Prepend worktree tool directories]
  F --> G[Spawn worker]
  G --> H{PATH-resolved security invocation}
  H -- Interactive --> I[Block with diagnostic and exit 1]
  H -- Non-interactive --> J[Exec /usr/bin/security]

Loading

_{Reviews (5): Last reviewed commit: "fix(agent): preserve exact worktree PATH..." | Re-trigger Greptile}

[tomdps]

The reported #704 root cause is real, but this boundary is not ready to merge. Please address these concrete issues:

1. src/darwin-keychain-boundary.js:27: Darwin environment keys are case-sensitive. pathKeyForEnv() can select Path when both Path and PATH exist, leaving the actual PATH used by descendants unchanged and allowing security -i to bypass the shim. On Darwin, update the exact PATH key and add a both-keys regression case.

2. src/darwin-keychain-boundary.js:98-102: publish the executable atomically. writeFileSync() truncates the live shim, so concurrent installers or a failed refresh can expose an empty/partial security executable to existing workers. Write and chmod a same-directory temporary file, then rename it over the live path; clean the temporary file on failure.

3. src/darwin-keychain-boundary.js:135-146: preserve inherited empty PATH components and distinguish unset PATH from explicitly empty PATH. Empty POSIX PATH segments mean the current directory; filter only duplicate shim-directory entries before prepending the shim.

4. tests/unit/darwin-keychain-boundary.test.js:217-218: the production-wiring cases take only the non-Darwin branch in required Ubuntu unit CI, while the required macOS install matrix does not run this test. Inject/stub the platform boundary in both production builders and exercise a PATH-resolved security -i grandchild using the returned environment, including real worktree tool-bin ordering. The test must fail if either production applyDarwinKeychainBoundaryToEnv call is removed.

Please also split tests/e2e/helpers/e2e-harness.js cleanup retry into a separate PR; it is reasonable but unrelated to the Keychain boundary squash.

After those updates, rerun the required gates and request re-review.

[shixi-li]

@tomdps Thanks for the concrete review. I rebased the change onto current main and addressed every requested item in c892e4a:

1. Darwin now updates the exact PATH key. A regression case starts with both Path and PATH and verifies only the real PATH is changed.
2. Shim refreshes write and chmod a uniquely named same-directory temporary file, atomically rename it over security, and remove the temporary file on failure. The failure-path test forces renameSync to fail and verifies the existing live shim remains intact with no temporary file left behind.
3. PATH mutation now removes only duplicate shim-directory entries. Empty components are retained, and tests separately cover unset PATH (<shim>) and explicitly empty PATH (<shim>:).
4. Both production spawn-env builders now have a narrow boundary-injection seam. Their tests force the Darwin branch on every CI platform, create real worktree tooling metadata, assert tool-bin -> shim -> inherited PATH ordering, and launch a worker whose PATH-resolved security -i grandchild must be blocked by the shim. Removing either production boundary call makes its corresponding test reach the logging fallback executable and fail.
5. The unrelated tests/e2e/helpers/e2e-harness.js cleanup retry is no longer present in this PR's commits or diff.

Validation after the rebase:

• focused boundary + adjacent worktree/runner tests: 24 passing
• complete npm run test:unit: exit 0
• npm run lint: exit 0 (existing warnings only)
• npm run typecheck: exit 0
• pre-push lint and typecheck gates: exit 0

Could you please re-review when convenient?

[tomdps]

Re-reviewed exact head 936d2eb after the contributor updates and two follow-up reviewer-fix commits. The #704 root cause is confirmed, every prior finding is resolved, the full current diff and direct spawn/PATH/cleanup integrations are clean, regression coverage exercises both production builders, unrelated cleanup is absent, authorship is preserved, and all applicable required gates are green. Approved for squash merge at this unchanged head.

[github-actions]

🎉 This PR is included in version 6.10.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀