Skip to content

Stop the test harness from mutating real machine state - #487

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/test-host-isolation
Sep 3, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/test-host-isolation

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes the confirmed-live finding from the 2026-09-02 adversarial review: the #451 headless harness boots the real App, so every local dotnet test run fired real startup side effects inside the test host — it rewrote HKCU's .sqlplan association and DefaultIcon to the test executable, loaded/cleared/polluted the developer's real appsettings.json (fixture paths evicted real Recent Plans entries; the saved open-tab list was destroyed), started ~36 never-stopped named-pipe servers and GitHub update checks per run, and could bind the real MCP port.

One seam, not scattered env sniffs: AppRuntimeMode.IsTestHost, set by a [ModuleInitializer] in the test assembly before the first App boot (and before any test can touch settings, harness user or not). Under it: the association write, pipe server, startup update check, and MCP start are skipped (each with a comment naming the side effect it prevents), the clipboard guard latches once per process instead of stacking per test dispatch, and AppSettingsService redirects to a per-run temp directory. Nothing in the product sets the gate or calls the redirect — with it unset, every path keeps its static-constructor default byte for byte.

The pinning test asserts flags set by the launch methods themselves, not by the gate, so a future launch path that sidesteps the gate still fails the test.

How was this tested?

Full suite twice in a row (per-run isolation proof): identical green results, each run creating its own temp settings dir while the real appsettings.json mtime never moved and the registry was not rewritten. At dev tip: 425 tests, 424 passed, 1 platform skip, 0 failed. No existing test needed its assumptions changed — the suite was already clean-profile shaped.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n

The harness (#451) boots the REAL App so MainWindow can resolve styles
from the application XAML, which meant the real startup side effects ran
inside the test host on every local dotnet test - all confirmed live:
HKCU's .sqlplan association and DefaultIcon were rewritten to point at
PlanViewer.Core.Tests.exe, ~36 MainWindow constructions each loaded the
real appsettings.json, restored the user's tabs and then destroyed the
saved open-tab list, fixture paths evicted real Recent Plans entries,
every test window seized the machine-wide named-pipe slot without ever
releasing it, hit GitHub with an update check, and could bind a real MCP
port; TextBoxClipboardGuard stacked a fresh set of process-wide class
handlers per test dispatch.

One explicit seam instead of scattered hacks: AppRuntimeMode.IsTestHost,
set only by the harness's module initializer before anything else in the
test assembly runs. Under it the app skips the file-association write,
StartPipeServer, the startup update check, and StartMcpServer. Settings
get an internal RedirectStorageForTestHost that points AppSettingsService
at a temp directory unique to each test RUN (not per test - the restore
tests deliberately exercise save/load continuity), leaving the real path
byte-for-byte the static-constructor default when never called. The
clipboard guard now latches once per process, which is a no-op for the
real app's single call.

Nothing in the product ever sets the gate, so with it off every check is
a constant false and real behavior is unchanged.

TestHostIsolationTests pins both seams: the effective settings path lives
under the run-scoped temp root and a save lands there, and a harness-built
MainWindow launches none of the three services - asserted via flags the
launch methods set themselves, so a new launch path still trips the test.

Full suite in Release twice (per-run isolation proof): 389 passed,
0 failed, 1 skipped both runs; each run created its own settings
directory and the real profile file's timestamp never moved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, well-scoped fix — no plan-XML parsing or T-SQL generation is touched, so the untrusted-input and SQL-injection concerns don't apply here.

Checked specifically:

  • Gate placement: AppRuntimeMode.IsTestHost correctly gates FileAssociationService.RegisterForCurrentExecutable, StartPipeServer, CheckForUpdatesOnStartupAsync, and StartMcpServer at exactly their prior unconditional call sites in App.axaml.cs/MainWindow.axaml.cs — order of operations in the constructor is otherwise unchanged.
  • Ordering guarantee: [ModuleInitializer] on HeadlessUi.EnterTestHostMode is the right tool — it's guaranteed to run before any other static member access in the assembly, including the Session Lazy<T> that boots the real App, and before any test touches AppSettingsService directly (which the harness setup approach couldn't guarantee). Only one test assembly exists (PlanViewer.Core.Tests), so the module-initializer scope is sufficient — there's no second test project that would boot the real App without this seam applying.
  • Settings redirection: AppSettingsService's SettingsDir/SettingsPath/OldFormatSettingsPath going from readonly to mutable, gated by a single internal redirect method, is a reasonable test-seam trade-off. Verified Load/Save/MigrateFormatSettings all route through the same mutable fields — no hardcoded path bypasses the redirect.
  • Test pinning design: PipeServerStarted/StartupUpdateCheckStarted/McpServerStartAttempted are set by the launch methods themselves rather than by the gate, so TestHostIsolationTests actually fails if a future change starts one of these through a new path instead of just re-asserting the gate's own value.
  • TextBoxClipboardGuard latch: applies unconditionally (not gated by IsTestHost), which is correct — it's a general idempotency fix for repeated App boots in the same process, harmless in production where Register() already only runs once.

No warnings introduced, no version-bump or Blazor-linked-file conventions apply (no Directory.Build.props/PlanViewer.Ssms/PlanViewer.Core files touched). Nothing further to flag.

@erikdarlingdata
erikdarlingdata merged commit d0939cb into dev Sep 3, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/test-host-isolation branch September 3, 2026 09:40
rferraton pushed a commit to rferraton/PerformanceStudio that referenced this pull request Oct 1, 2026
…rikdarlingdata#490)

Session restore had two gaps, both from writing the list only at clean
close (erikdarlingdata#468's original scope): any abnormal exit -- crash, task kill, an
OS "shut down anyway" past the dirty-tab prompt -- restored zero tabs,
and a file-backed plan or query detached into its own window was never
written down at all, because SaveOpenPlans walked MainTabControl alone.

Membership changes now write the list as they happen, debounced one
second so a burst (restore, Close All) lands as one write. The trigger
rides the erikdarlingdata#481 TabContentWatcher rather than the call sites, for exactly
that commit's reason: a persist remembered at sixteen call sites gets
forgotten at the seventeenth. The two changes the strip cannot show get
explicit calls -- the detached register (a window closing changes no
tab) and SaveQueryToPath (a scratch gaining its first file changes no
membership).

Detached windows join the collected set through a second register next
to erikdarlingdata#473's: the prompt register is query-sessions-only because only an
edit can be lost, while persistence needs every FILE-backed detached
window, plans included. Detached entries append after the docked tabs;
on the next start they come back as ordinary docked tabs, deliberately
not re-detached windows.

The crash-loop defense is kept and sharpened: RestoreOpenPlans clears
and saves the empty list BEFORE the first open (it used to clear after
the loop, which only defended against crashes after restore finished),
and each file that opens successfully re-enters through the debounced
writer, which restore flushes synchronously at its end. Net invariant:
a file that crashes the app during load never persists -- it died before
its own re-add -- while everything that opened does. A crash mid-restore
still loses the tabs opened before it (their re-add was pending, the UI
thread never flushed it); accepted, and said so in the code.

OnClosed keeps its save as the final authoritative write, now with the
debounce timer stopped first so no tick lands in a torn-down window.
Under the test host no real timer is armed at all -- the suite shares
one dispatcher, and a stray tick would write one test's tabs over
another's staged state -- so tests drive the flush through a
deterministic seam and assert the redirected settings file (erikdarlingdata#451/erikdarlingdata#487).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
rferraton pushed a commit to rferraton/PerformanceStudio that referenced this pull request Oct 1, 2026
…#496)

erikdarlingdata#495 made the open-tab list survive abnormal exits, which brought back
every tab with a file behind it. The remaining loss was the tab that
never had one: a scratch query - typed, never saved - lost its content
to any crash, task kill, or OS "shut down anyway". Every interactive
discard route already prompts (erikdarlingdata#462/erikdarlingdata#469/erikdarlingdata#473/erikdarlingdata#477), so the design
center is the gap the prompts cannot cover: a buffer the user CHOSE to
discard dies; a buffer they NEVER GOT TO CHOOSE about survives.

Storage: one file per scratch buffer under a scratch/ directory beside
the settings file, named by a stable per-session GUID minted at first
persist and carried on the session object (QuerySessionControl
.ScratchBufferId), written through AtomicFile. The directory rides
AppSettingsService's test-host redirection (erikdarlingdata#487/erikdarlingdata#451), pinned in
TestHostIsolationTests.

Session list: scratch tabs enter the erikdarlingdata#495 open_tabs list as inline
scratch:<guid> entries IN STRIP ORDER among the plain paths - no second
list, no version field. Compatibility is pinned as a string property
the way erikdarlingdata#494's sentinel lesson taught: the colon in the prefix means an
old build's File.Exists guard skips the entry silently, and a new build
reading an old list sees only paths and behaves exactly as before.
Restore routes three ways: scratch entry -> dirty query tab recreated
from its buffer with the same GUID; path -> OpenFileByExtension as
always; unparseable -> treated as a path.

Content cadence: a 2s idle debounce, deliberately separate from erikdarlingdata#495's
1s membership debounce (keystroke-scale vs click-scale), hooked through
DirtyStateChanged for sessions that are scratch at CreateTab time, and
drained at every erikdarlingdata#495 flush point (end of restore, OnClosed before the
final list write, PersistSessionForRestart, the membership flush) plus
its own tick, which chains the membership flush so a buffer and its
entry land together. No real timer under the test host (shared
dispatcher, same reasoning as erikdarlingdata#495); FlushPendingScratchPersistForTests
is the deterministic seam. SCOPE FENCE: only scratch content persists -
file-backed tabs' unsaved edits stay guarded by prompts alone.

Delete-on-choice, hooked at the resolution rather than the dialog: the
two near-twin choice switches (docked/detached) collapse into
ResolveCloseChoiceAsync, where Don't Save drops the buffer; a
successful SaveQueryToPath retires it (the real file owns the content
now); closing a clean scratch tab or window sheds any stale buffer;
Cancel changes nothing. A clean scratch is by construction an empty
one, so after a clean close zero buffers remain - every buffer was
chosen about. Orphan sweep at startup deletes unreferenced files
(stranded buffers and AtomicFile .tmp strays alike), and a buffer that
fails to load during restore is skipped, never re-added, and swept -
the erikdarlingdata#495 poison invariant mirrored.

Size cap ~1MB: past it the buffer is removed rather than left stale,
and that one tab behaves pre-erikdarlingdata#496. Detached scratch windows persist
like docked ones - the subscription and pending set are keyed on the
session, which detach moves intact - and Don't Save at a detached
prompt deletes the same way.

Tests: 13 new (content-without-closing, restore continuity, interleaved
order, Don't Save docked and detached, save conversion, clean-close
zero buffers, emptied-tab shed, orphan sweep, poison parity, size cap,
old-format list, prefix compat pin) plus the scratch-directory redirect
pin. Suite: 484 total, 483 passed, 1 platform skip, run twice.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
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