Skip to content

settings: refuse test writes to the account's real stores - #468

Merged
m4ttheweric merged 6 commits into
mainfrom
settings-test-write-guard
Sep 25, 2026
Merged

m4ttheweric merged 6 commits into
mainfrom
settings-test-write-guard

Conversation

@m4ttheweric

@m4ttheweric m4ttheweric commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

On 2026-09-25 around 09:03 a test run wrote into Matt's real settings stores: rt.apiPort=50489, rt.repoRoots=[] in the machine store, chat.viewerUrl="", chat.humanHandle="matt", and notification toggles. The daemon then bound 50489 instead of 9401 and the tray lost it. This PR makes that write impossible from a test run.

What changed

  • packages/rt-client/src/test-isolation.ts: assertNotRealStoreInTest(path) throws when a test run targets anything under the account's real ~/.mattstack. The rule is two pure functions (testRunSignal, realStoreRefusal), each with its own tests. The error names the signal that tripped (NODE_ENV=test, VITEST set, or entry module <Bun.main>).
  • setSetting/unsetSetting call it before touching a store, so no dirs get created and no file gets edited.
  • The other settings-store writers outside rt-client call it before any side effect: rt team create (hand-writes settings.team.jsonc), rt team join (its clone brings settings.team.jsonc), and the home-repo init seam createRealExecSeam (its clone brings the user and machine stores).
  • lib/__tests__/no-daemon-sync-exec.test.ts allowlists test-isolation.ts: its one spawn (id -P) only runs in a test run.
  • AGENTS.md footgun and docs/settings-architecture.md: bun reads bunfig.toml only from the cwd, plus the two escapes that happen with the preload loaded. Nested bunfigs can't make the preload location-independent (bun never walks up), so this is documented instead.
  • rt-client 0.31.2, dist rebuilt.

Detection signal

  • A test run means NODE_ENV=test, OR Bun.main is a test file, OR VITEST is set. bun test sets NODE_ENV=test only when NODE_ENV is unset, so Bun.main (the running test file) covers a preset NODE_ENV. NODE_ENV also reaches children, including Bun.spawn children that get the startup env. bun sets no other marker (no BUN_* var, and the globals are identical).
  • The account home comes from the user database (id -P on macOS, getent passwd elsewhere). It's memoized and only looked up during a test run. A child-process test pins this: a NODE_ENV=test child started with a scratch HOME writes its own store, and fails if the account home follows HOME. Under bun, os.userInfo().homedir and os.homedir() both return the HOME the process started with. Using them would refuse an e2e child spawned with HOME=<temp> and the inherited NODE_ENV when it writes its own temp store.
  • Only <account home>/.mattstack is refused, so a scratch HOME nested inside the account home still works.

Root cause

  • The traced 08:58 pair (bun test --parallel=2 lib/daemon/__tests__/api-server.test.ts lib/__tests__/cd.test.ts from eored's root) doesn't reproduce. I ran it on its commit (e8e9af4) with a fake account home, inside a sandbox that denies writes to the real ~/.mattstack, and every write landed in the preload's scratch HOME. lib/__tests__/cd.test.ts doesn't exist on that commit, so only api-server.test.ts ran. The full unit suite under --parallel=4 on that commit is clean too. --parallel resets the env and re-runs the preload for every file.
  • The same checkout has an undefined/ tree written at 09:02:32 by a broader run (bundle, secrets, team join, chat and notifier tests). So HOME was corrupted mid-run there: the RT-302 failure mode, fixed on main at 10:43. The leaked keys come from several files (api-server*, chat-handlers, chat-delivery, notifier*, *repo-root*). That fits a stretch of a run where HOME resolved to the account, not a single test.
  • I confirmed three ways a run reaches the account's stores on bun 1.4.2:
    1. No preload, because bun reads bunfig.toml only from the cwd. On e8e9af4, running the two api-server tests from another directory rewrote the fake account's store (their unset deleted its rt.apiPort).
    2. HOME unset: process.env.HOME ?? homedir() falls back to bun's frozen startup HOME, which is the real one, even with the preload loaded.
    3. Bun.spawn/Bun.spawnSync without env: the child gets the startup env (real HOME, no RT_TEST_FORBID_SOCKS), though it does get NODE_ENV=test.
  • Without the 09:02 run's exact command, I couldn't pin which of the three produced the 09:03 state. The guard refuses all three at the write seam. The RT-302 hooks already cover the relative "undefined" variant.

Tests

Targeted runs only:

  • packages/rt-client suite, dist freshness included: 398 pass.
  • no-daemon-sync-exec, team join, team create, home init-exec, api-server, api-server-bind, api-server-cors-ws, settings-paths-parity, test-home-isolation, home-guard: 166 pass.
  • Regression test "refuses the write a test makes after deleting HOME": it runs a child whose startup HOME is a scratch account home. Without the guard, that child wrote the store.
  • End to end: I ran the two api-server files from a directory without the repo preload, with a fake account home. The old code rewrote the store. This branch refuses (4 tests fail loudly) and leaves the store untouched.

Publishing

This bumps @mattstack/rt-client to 0.31.2. Publishing is a separate release-class step, done from main after merge (see AGENTS.md). The version is shared, so whoever merges second renumbers.

🤖 Generated with Claude Code

m4ttheweric and others added 5 commits September 25, 2026 11:24
setSetting and unsetSetting now throw before touching a store under the
account's real ~/.mattstack when the process is a test run (NODE_ENV=test,
VITEST, or a test file as Bun.main). The account home comes from the user
database, because bun's os.homedir() and os.userInfo().homedir both report
the HOME the process started with.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 46 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 85 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c45847c5-e251-4d21-a370-e960601af306

📥 Commits

Reviewing files that changed from the base of the PR and between 30d45d0 and e5dfe5e.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • AGENTS.md
  • docs/settings-architecture.md
  • lib/__tests__/no-daemon-sync-exec.test.ts
  • lib/home/__tests__/init-exec.test.ts
  • lib/home/init-exec.ts
  • lib/team/__tests__/create.test.ts
  • lib/team/__tests__/join.test.ts
  • lib/team/create.ts
  • lib/team/join.ts
  • packages/rt-client/package.json
  • packages/rt-client/src/settings/__tests__/write.test.ts
  • packages/rt-client/src/settings/write.ts
  • packages/rt-client/src/test-isolation.ts
  • packages/rt-client/test/test-isolation.test.ts

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

The refusal now says which signal made the process a test run. Team join
(its clone brings settings.team.jsonc) and the home-repo init seam (its
clone brings the user and machine stores) refuse the account's real
~/.mattstack in a test run, like team create. A child-process test proves
the account home is not read from HOME, and the daemon sync-exec gate
allowlists test-isolation.ts, whose one spawn only runs in a test run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@m4ttheweric
m4ttheweric merged commit dc73775 into main Sep 25, 2026
7 checks passed
@m4ttheweric
m4ttheweric deleted the settings-test-write-guard branch September 25, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant