Skip to content

test(pi): sandbox review-server tests from the user's config - #1624

Merged
backnotprop merged 1 commit into
mainfrom
test/pi-server-isolate-data-dir
Sep 28, 2026
Merged

backnotprop merged 1 commit into
mainfrom
test/pi-server-isolate-data-dir

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Problem

apps/pi-extension/server.test.ts (Pi review server) and packages/server/review-workspace.test.ts boot real review servers, which read config.json from the data dir. The root bunfig.toml preload (tests/setup/feedback-archive-off.ts, #1473) points PLANNOTATOR_DATA_DIR at a temp dir, but bun only loads that bunfig when it runs from the repo root. When it runs from the package directory (apps/pi-extension's own bun test script, or bun test inside packages/server), the tests read the contributor's real ~/.plannotator/config.json.

With {"reviewAnalysis":{"semanticDiff":false}} in that file, 6 tests fail in each file:

  • Pi: forced GitButler review, superseded switch, both semantic-diff cwd tests, "hides semantic diff", workspace prefixed paths
  • review-workspace: superseded switch, both semantic-diff cwd tests, sem probe caching, "hides semantic diff", workspace integration

CI passes only because CI has no config file. This breaks the CLAUDE.md Testing Rules: never read the real user config; sandbox under a temp PLANNOTATOR_DATA_DIR.

Fix

Each file gets a beforeEach that points PLANNOTATOR_DATA_DIR at a fresh temp dir. The file's existing afterEach already restores the variable and removes temp dirs. Tests that set their own data dir still override it. Nothing changes at module scope.

Proof

I ran each check with a temp HOME whose .plannotator/config.json is {"reviewAnalysis":{"semanticDiff":false}}, and with PLANNOTATOR_DATA_DIR unset. The real ~/.plannotator was not touched.

Run (from package dir) Before After
apps/pi-extension: bun test server.test.ts 23 pass / 6 fail 29 pass / 0 fail
packages/server: bun test review-workspace.test.ts 6 fail 54 pass / 0 fail
  • I compared full-package sweeps (all of apps/pi-extension, all of packages/server) with the fake-config HOME against a clean HOME. These two files were the only config-caused failures.
  • After the fix, the two files write nothing into the fake HOME's .plannotator.
  • Running from the repo root still passes: 83/83, with the fake HOME and with the normal env.
  • bun run typecheck is clean.

Out of scope

Some other test files, when run from a package directory, still write drafts, history, and similar files into $HOME/.plannotator. Their results do not depend on the config, so this PR leaves them alone.

apps/pi-extension/server.test.ts and packages/server/review-workspace.test.ts
boot real review servers that read config.json from the data dir. The root
bunfig preload points PLANNOTATOR_DATA_DIR at a temp dir, but bun only loads
it from the repo root; run from the package directory (apps/pi-extension's
own `bun test` script, or `bun test` inside packages/server) the tests read
the contributor's real ~/.plannotator/config.json. With
reviewAnalysis.semanticDiff: false there, 6 tests fail in each file.

Point PLANNOTATOR_DATA_DIR at a fresh temp dir in a beforeEach; the existing
afterEach already restores it.
@backnotprop
backnotprop merged commit 3943ebe into main Sep 28, 2026
28 checks passed
@backnotprop
backnotprop deleted the test/pi-server-isolate-data-dir branch September 28, 2026 15:03
YeKc1M pushed a commit to YeKc1M/plannotator that referenced this pull request Oct 1, 2026
…prop#1624)

apps/pi-extension/server.test.ts and packages/server/review-workspace.test.ts
boot real review servers that read config.json from the data dir. The root
bunfig preload points PLANNOTATOR_DATA_DIR at a temp dir, but bun only loads
it from the repo root; run from the package directory (apps/pi-extension's
own `bun test` script, or `bun test` inside packages/server) the tests read
the contributor's real ~/.plannotator/config.json. With
reviewAnalysis.semanticDiff: false there, 6 tests fail in each file.

Point PLANNOTATOR_DATA_DIR at a fresh temp dir in a beforeEach; the existing
afterEach already restores it.
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