Skip to content

fix(daemon): require directory trust before booting - #1260

Open
yetuge wants to merge 3 commits into
Nano-Collective:mainfrom
yetuge:fix/daemon-directory-trust
Open

yetuge wants to merge 3 commits into
Nano-Collective:mainfrom
yetuge:fix/daemon-directory-trust

Conversation

@yetuge

@yetuge yetuge commented Sep 11, 2026

Copy link
Copy Markdown

Fixes #1245

Root cause

nanocoder daemon start is a fast path in source/cli.tsx that branches off before the interactive app and goes straight to runDaemonCli("start")launchDaemonentry.ts. Nothing along that path — or anywhere under source/daemon/ — consults preferences.trustedDirectories: the trust check only lives in the interactive TUI (useDirectoryTrust) and the plain shell (source/plain/shell.ts).

Once booted, the daemon dispatches subagents in headless mode, which skips tool confirmations for execute_bash, write_file, string_replace, diff_edit and MCP tools, and OS-level sandboxing is opt-in (unsupported on Windows). So a daemon started in a directory the user never trusted arms every .nanocoder/ skill subscription for unattended execution.

Fix

Add a non-interactive trust gate with the same rules the plain shell already enforces (ensureDirectoryTrust in source/plain/shell.ts), applied at both layers that can boot a daemon:

  • start() (shared helper in the new source/daemon/trust.ts): refuses with a user-facing message unless the project root is already in preferences.trustedDirectories, the caller passes --trust-directory, or NANOCODER_TRUST_DIRECTORY=1 is set. Both explicit mechanisms record standing trust (like the plain shell's env-var path), because the daemon boots detached and can boot again via autostart without the CLI — ephemeral trust would not survive the spawn.
  • entry.ts: the boot itself re-checks the same rule before standing up the pipeline, so autostart boots (launchd/systemd, set up by daemon install) that never run the CLI are covered too. The CLI gate is still needed: it fails before spawning, with a message the user actually sees, whereas a boot-gate-only refusal would land in daemon.log.

--trust-directory is now honored for daemon start (previously documented as applying only to nanocoder run); for a daemon it records standing trust rather than "this run only", since the process it arms outlives the invoking shell. daemon install itself is deliberately left ungated — it only writes the autostart registration, and the boot gate now covers the path it arms.

Tests

New regression tests in source/daemon/cli.spec.ts (run against unpatched main with only the tests applied, then with the fix):

  • start refuses to spawn when the project directory is not trusted — asserts exit 1, a /not trusted/ message, no launcher invocation and no trust writes. Failed on main (the daemon spawned regardless).
  • start marks the directory trusted and proceeds when NANOCODER_TRUST_DIRECTORY=1 — asserts standing trust is persisted. Failed on main.
  • start records standing trust when --trust-directory is passedfailed on main (the flag was ignored).
  • start proceeds when the directory is already trusted — guards the happy path; passes before and after.

Red→green: on main (537ee6c) the new tests report 3 tests failed; with the fix, pnpm exec ava source/daemon/cli.spec.ts12 tests passed. Also green: source/daemon/lockfile.spec.ts + source/daemon/install.spec.ts (30), source/plain/shell.spec.ts (22), source/cli.spec.ts (54). pnpm test:types and biome check are clean on the touched files.

Fixes #1245

The daemon fast path (`nanocoder daemon start`) never touched the
directory trust machinery: cli.tsx branched off before App.tsx, and
nothing in source/daemon/ checked preferences.trustedDirectories. Once
booted, headless mode skips tool confirmations for execute_bash,
write_file, string_replace, diff_edit and MCP tools, so a daemon started
in an untrusted repo armed every .nanocoder/ skill subscription for
unattended execution (issue Nano-Collective#1245).

Add a non-interactive trust gate with the same rules the plain shell
already enforces, applied at both layers that can boot a daemon:

- start(): refuse with a user-facing message unless the project root is
  in preferences.trustedDirectories, the caller passes
  --trust-directory, or NANOCODER_TRUST_DIRECTORY=1 is set. Both
  explicit mechanisms record standing trust, because the daemon boots
  detached and can boot again via autostart without the CLI.
- entry.ts: the boot itself re-checks, so launchd/systemd autostart
  boots (set up by `daemon install`) that never run the CLI are
  covered too.

Regression tests in cli.spec.ts cover refuse-when-untrusted (no spawn,
no trust writes), trusted-via-preferences (no rewrite), and both
explicit trust mechanisms (persisted, daemon spawned).
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 4 important, 2 nits

@yetuge — there is a blocking item below.

The PR correctly ports the directory-trust gate from the plain shell into both the daemon start CLI and the daemon boot (entry.ts), with a shared helper and four regression tests that fail on main. The core design (gate in two layers, the boot-gate covering autostart) matches the issue. Two blocking concerns: PR #1259 (fix/daemon-trust-gate, addyCooks) appears to solve the same problem and should be reconciled before either lands, and the production wiring through cli.tsx is not covered by the new tests, so a regression in the daemon fast-path's --trust-directory arg handling would not be caught. The main --help text and example for --trust-directory also still say it is only valid with run and does not modify preferences, which the PR turns into a lie.

Possible duplicate of #1259 — worth checking before going further.

🔴 blocking · duplicate · source/daemon/cli.ts:162

PR #1259 (fix/daemon-trust-gate, author addyCooks, title "fix(security): gate daemon start on directory-trust disclaimer") is solving the same problem against the same issue (#1245) and modifies the same files (source/daemon/cli.ts, the daemon boot path, and the test for it). The two PRs will conflict, and whichever lands first obsoletes the other. Coordinate with addyCooks (e.g., agree on a single approach in one of the PRs, or merge one into the other) before this can land.

🟠 important · tests · source/daemon/cli.spec.ts:102

The four new tests exercise runDaemonCli directly with an injected trustDirectory: true option, not through source/cli.tsx's daemon fast path. The production wiring added in cli.tsx (trustDirectory: args.slice(2).includes('--trust-directory'), near line 68) is part of the user-facing contract — nanocoder daemon start --trust-directory is exactly the spell the PR promotes — and it currently has no test. A regression that drops the flag in the daemon fast path (so only NANOCODER_TRUST_DIRECTORY and persisted trust work) would not be caught by this suite. Add at least one test that spawns cli.tsx (or invokes runDaemonCli with trustDirectory derived from an --trust-directory token) for the flag-on-args path.

🟠 important · completeness · source/cli.tsx:165

The --help text now lies about what the PR just changed. Lines 165–166 still say:

  --trust-directory   Skip the first-run directory trust prompt for this run only.
                      Only valid with the "run" command. Does not modify the preferences file.

After this PR, --trust-directory is also valid with nanocoder daemon start, and does modify the preferences file (standing trust), per the PR description's own contract change. The example on line 193 (nanocoder --trust-directory run ...) is now incomplete as a quickstart. Update the help text and examples to reflect the new semantics — this is exactly the CLI-flag contract the project rubric calls out.

🟠 important · tests · source/daemon/entry.ts:56

The boot-time gate added in entry.ts is the only thing covering autostart-launched daemons (the whole point of the PR's second layer), but there is no test that exercises it. The new test file covers only the CLI-level gate in cli.ts. A regression that removed the ensureDirectoryTrust(...) block from entry.ts would silently re-open the original issue on autostart boots; the four green tests would not notice. Add at least one test that imports entry.ts (or a thin re-export of main) with stubbed loadPreferences/savePreferences and asserts it process.exit(1)s for an untrusted project root.

🟠 important · scope · source/daemon/trust.ts:38

ensureDirectoryTrust for the daemon diverges from the plain-shell helper in one substantive way: when the --trust-directory flag is passed, the daemon writes to preferences, the plain shell does not. That asymmetry is intentional per the PR description (the daemon survives the spawn), but it means the two helpers have different observable behaviour under the same flag, and the divergence is silent — nothing in trust.ts says so. Either factor the asymmetry into two helpers (isDirectoryTrusted vs trustDirectory) so the name documents the side effect, or add a one-line note in source/plain/shell.ts's ensureDirectoryTrust referencing the daemon helper so the two implementations stay in sync over time. Right now a refactor of one is easy to apply to the wrong place.

⚪ nit · changeset

This is a user-facing security change (nanocoder daemon start now refuses to boot in untrusted directories and accepts --trust-directory/NANOCODER_TRUST_DIRECTORY=1), so the PR needs a .changeset/*.md entry describing the new gate. None is present in the changed-files list. Without it the changelog for the next release will be missing this user-visible behavior change.

⚪ nit · correctness · source/cli.tsx:68

The new wiring is trustDirectory: args.slice(2).includes('--trust-directory'). slice(2) only inspects the trailing args (after daemon <sub>), so the flag must come after the subcommand: nanocoder daemon start --trust-directory works, nanocoder daemon --trust-directory start does not. The CLI parser lower in the file (around line 366) recognises --trust-directory in any position because the daemon fast path short-circuits before it runs. Consider parsing the flag position-agnostically here too — args.includes('--trust-directory') — to match how the rest of the CLI handles it.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Sep 11, 2026
Second review pass on the daemon trust gate, addressing the nc-review
findings:

- Read the flag position-agnostically through a shared
  `hasTrustDirectoryFlag()` helper, so `nanocoder --trust-directory daemon
  start` no longer quietly degrades to "untrusted" while the general parser
  accepts the same spelling.
- Move the boot rule into `checkDaemonBootTrust()` and have `entry.ts` call
  it. `entry.ts` runs `main()` on load, so a spec cannot import it; the rule
  is testable where it now lives.
- Correct the `--trust-directory` help text, its example, and the matching
  row in docs/getting-started, which still claimed the flag was valid only
  with `run` and never touched the preferences file.
- Document the deliberate persistence asymmetry against the plain shell on
  both sides of the two helpers.
- Add the changeset the user-facing security change was missing.

New spec file `source/daemon/trust.spec.ts` (9 tests) covers flag position,
the persistence asymmetry, the boot gate refusing/permitting a directory,
and that the boot never derives trust from the invoking argv - autostart
boots carry no argv at all.
@yetuge

yetuge commented Sep 12, 2026

Copy link
Copy Markdown
Author

Addressed in b27380a — thanks, this round was useful.

nit · correctness (cli.tsx:68) — the daemon fast path no longer looks at args.slice(2). The flag is read through hasTrustDirectoryFlag() in source/daemon/trust.ts, shared with the boot gate and position-agnostic, so nanocoder --trust-directory daemon start no longer silently degrades to "untrusted". Specs cover flag-before-subcommand, flag-after-subcommand, absent, and a lookalike (--no-trust-directory).

important · completeness (cli.tsx:165) — the --help text now says the flag is valid with run (that run only) and with daemon start (which also records standing trust, so the detached boot and any autostart boot skip the gate), and a nanocoder daemon start --trust-directory example was added. The same incorrect claim lived in docs/getting-started/index.md, so that row was updated too.

important · tests (entry.ts:56) — the boot rule moved into checkDaemonBootTrust() in source/daemon/trust.ts and entry.ts now calls it. A spec cannot import entry.ts as it stands: it runs main() at module load, so importing it would chdir and boot a real daemon. The rule is now testable where it lives, with specs for untrusted → refused with the message naming the directory, already-trusted → passes, NANOCODER_TRUST_DIRECTORY=1 → passes and persists, plus one asserting the boot never derives trust from the invoking argv (autostart boots carry none). The honest gap that remains: nothing asserts entry.ts and cli.tsx call these helpers — that needs either a spawn harness around cli.tsx or splitting the entry.ts bootstrap so main can be imported with stubbed deps. Happy to do either in this PR if you want it here rather than as a follow-up.

important · scope (trust.ts:38) — took the documentation branch rather than splitting into isDirectoryTrusted / trustDirectory. The asymmetry is now spelled out on ensureDirectoryTrust in source/daemon/trust.ts, with the reason (the daemon outlives the CLI process and can be re-booted by autostart, so run-scoped trust would drop the user back to "untrusted" on the next boot), and plain/shell.ts carries the reciprocal pointer. If you'd prefer the split so the name carries the side effect, it's a small change — say the word.

nit · changeset — added .changeset/daemon-boot-trust-gate.md describing the new gate, the daemon start semantics of --trust-directory, and the autostart coverage.

blocking · duplicate (#1259) — flagging this instead of acting on it, since it isn't mine to resolve. #1259 was opened first (10:19Z) and covers the same issue. The one capability this PR has that I can't see in #1259's file list is the second layer: entry.ts re-checks the rule inside the daemon process, which is the only path covering daemon install's launchd/systemd autostart, because those boots never run the CLI and so have no argv to read a flag from. Options as I see them — (a) land one and port the boot layer into the other as a follow-up, (b) leave both open and let a maintainer pick, (c) I close this one so the queue stays clean. No preference between (a) and (b); happy to rebase onto whichever you'd rather take. For transparency, I opened this before spotting that #1245 was already assigned to @addyCooks — I'd rather raise the overlap than leave two competing PRs sitting in the queue.

source/daemon/trust.spec.ts is new (9 specs, green); the existing four gate specs in source/daemon/cli.spec.ts still pass. pnpm test:types, biome check, the daemon suite, source/cli.spec.ts (54) and source/plain/shell.spec.ts (22) are clean locally; the only local failures are the pre-existing Windows ipc socket specs, which fail identically on main.

@github-actions github-actions Bot added the area:docs Documentation label Sep 12, 2026
Keeps the branch mergeable after Nano-Collective#1261 (feature/new-ui) landed on main: the
PR was reported as conflicting with the base branch.

The only conflict was the `--trust-directory` row of the CLI flag table in
docs/getting-started/index.md - main rewrote the neighbouring `--alt-screen`
rows while this branch rewrites the trust row. Resolved by keeping this
branch's trust wording and main's alt-screen wording.

source/cli.tsx auto-merged: the daemon fast path still reads the flag through
hasTrustDirectoryFlag() and the help text keeps the corrected wording.

Post-merge checks: tsc --noEmit clean; daemon trust + cli specs and
plain/shell specs 43 passed.
@yetuge

yetuge commented Sep 12, 2026

Copy link
Copy Markdown
Author

Follow-up: merged main into the branch (773d34bd) — the PR had gone stale against the base once #1261 landed. The only conflict was the --trust-directory row of the CLI flag table in docs/getting-started/index.md (main rewrote the neighbouring --alt-screen rows), resolved by keeping this branch's trust wording and main's alt-screen wording. Nothing under source/daemon/ changed in the merge; tsc --noEmit is clean and the trust / daemon-cli / plain-shell specs are green (43 passed) afterwards.

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself is sound, and the response to the previous round was thorough. I fetched the branch, merged current main in (clean), and ran tsc --noEmit, biome check on the touched files, and the trust / daemon-cli / plain-shell / cli specs: all clean, 100 tests pass. The two-layer design checks out, runDaemonCli is the only production caller of launchDaemon, the plist and systemd unit set only NANOCODER_PROJECT_ROOT so autostart really does depend on persisted trust, and the boot gate sits before createLLMClient and startDaemon.

Requesting changes on the following.

Blocking, not yours to fix: #1259 is still open and #1245 is still assigned to @addyCooks, so a maintainer needs to pick a lane before either lands. Separately, the required pr-checks workflow has never run on this PR (only changeset-check and label did), so the green ticks above mean less than they look. The branch is also 128 commits behind main.

Changeset is factually wrong. .changeset/daemon-boot-trust-gate.md says NANOCODER_TRUST_DIRECTORY=1 "works for a single boot". It doesn't: the env-var path calls markTrusted too, so it persists standing trust exactly like the flag. The behaviour is right (it mirrors plain/shell.ts), only the text is wrong, and that text ships in the changelog.

The --trust-directory split deserves one more look. Same flag, opposite persistence semantics: ephemeral for run, a permanent global trust grant for daemon start. You documented it well, but documenting a footgun doesn't disarm it, and nanocoder daemon start --trust-directory in a throwaway or shared build directory permanently trusts that path. Worth considering: have daemon start --trust-directory propagate NANOCODER_TRUST_DIRECTORY=1 into the spawned child's env rather than persisting. The boot gate passes without a permanent write, and autostart, the one case that genuinely needs persistence, is already behind the explicit daemon install.

Docs still walk users into the refusal. docs/features/skills.md:173 and docs/features/scheduler.md:74 both tell people to run nanocoder daemon start with no mention of the new gate. The flag table in getting-started was updated, but those are the two pages where people actually meet the daemon. Related: daemon install stays ungated by design, so installing in an untrusted directory succeeds and then every autostart boot fails into daemon.log where nobody looks. A warning at install time closes that cheaply.

Silent savePreferences failure. savePreferences swallows write errors and returns void, and markTrusted returns true regardless. On a failed write the CLI prints "Marked X as trusted. Daemon started" while the child's own gate refuses and waitForLockfile times out. Rare, and it fails safe, but the message is untrue.

Test gap: please take your own offer here rather than deferring it. Nothing asserts that cli.tsx and entry.ts actually call the helpers, so a regression deleting either call site passes the suite. For a security gate that's the regression most worth catching.

Minor: the gate runs before readLiveLockfile, so daemon start in an untrusted directory that already has a daemon running reports "not trusted" instead of "already running".

@github-actions

Copy link
Copy Markdown
Contributor

Hi @yetuge, thanks for this PR! It looks like a maintainer has left feedback
or review activity and there are still some outstanding items to wrap up.

Whenever you get a chance, could you take a look at the open comments?
If anything is unclear or you'd like a hand, just reply here and we'll help you get it across the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings area:docs Documentation stale:nudged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] nanocoder daemon start has no directory-trust check; headless mode permits unattended command execution

2 participants