Release v1.24.0 - #500
Merged
Merged
Conversation
…447) (#481) Compare Plans came back disabled after running two queries, on a build containing the fix that was supposed to have sorted that out. #449 fixed the enablement rule and left the refresh: a plan produced by executing a query lands by having an existing tab's Content replaced, and #449 subscribed to the tab collection, which says nothing about that. The same shape sits at window level in Get Actual Plan on a file tab. Both tab controls now go through TabContentWatcher, which reports collection changes and content replacement, so the next path that produces a plan is correct without its author knowing the watcher exists. The five hand-written refreshes in Plans.cs go with it - there is one place that decides now. The owner lookup moves off TopLevel.GetTopLevel and onto the logical tree. A TabControl realises the selected tab and nothing else, so a session in a background tab could not see its own window, and a query left running while the user works elsewhere lands its plan in exactly that state - the fallback then reinstated the original bug. Erik's own table had Query Store down as broken; it is not. Its two plan-producing sites go through AddPlanTab, which did refresh. What was broken is both execution paths and the window-level actual plan. The tests now drive those paths rather than the file path they avoided last time. What still needs a SQL Server is producing plan XML, and only that. Claude-Session: https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… copy paths (#482) (#483) #467 substituted at Copy Query Text, Open in Query Editor and the statements grid, and the reporter came straight back with "still shows parametrized in Human and Robot Advice". Both advice buttons, the HTML export, the comparison report and every MCP tool read their statement text out of ResultMapper, so that is where the substitution belongs. StatementResult now carries both forms. StatementText is runnable; parameterized_statement_text carries the plan's own record, present only when something was substituted. get_repro_script reads the parameterized form on purpose — it wraps that body in sp_executesql with a parameter list read out of the same plan, and a body with the literals already inlined would declare parameters it never uses. Statement pairing was checked first and does not key on text: ComparisonFormatter matches on QueryHash and falls back to position, so two runs of one query with different values still pair. A test pins that. Substitution also grew an assignment-target check. compile_memory_exceeded_plan is "SELECT @job_name = name, @owner_sid = owner_sid" with both compiled values NULL, and writing the value over the target gave "SELECT NULL = name" — a clipboard wart under #467, but this change would have made it the default rendering in advice, exports and MCP. Claude-Session: https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#456's StoredProc/UDF descents called ParseStatementAndChildren without the depth argument, so recursion depth silently reset to zero at every procedure boundary. The MaxParseDepth guard - which exists so a maliciously deep plan throws a catchable error instead of an uncatchable StackOverflowException - could then never fire across StoredProc/UDF nesting: a crafted .sqlplan alternating StmtSimple > StoredProc > Statements a few thousand levels deep (about sixty bytes per level) killed the whole process, from any plan-open route that reaches the parser. ParseStatement now takes the caller's depth and both descents pass depth + 1, so the guard sees the true nesting. The same pass closes the sibling gap: synchronous Parse had no document-size ceiling at all, while ParseAsync capped at 16MB via XmlReaderSettings.MaxCharactersInDocument. Parse is the path the app's PlanViewerControl, the web viewer, and the analysis pipeline actually use, so it now enforces the same MaxParseCharacters limit with a length check (the input is already a string; the limit is characters, not bytes) thrown as the parser's usual catchable InvalidOperationException. Tests generate the plan XML instead of shipping a fixture - a depth bomb is three elements repeated 1,100 times. The bomb parses on a deliberately large-stack thread with nesting just past the guard, so both outcomes are deterministic: fixed, the guard fires at depth 1,001; regressed, the parse completes in the headroom and the test fails on a null ParseError instead of killing the test host (verified against the unfixed parser). A 50-level companion pins that legitimate nesting still parses every level, and an oversized well-formed document pins the sync size cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Carry the parse depth through proc and UDF descent
An adversarial review of the unsaved-changes work turned up three data-loss routes that all bypassed the guards #462 and #473 built: - The About window's Velopack "Restart Now" called ApplyUpdatesAndRestart, which exits the process without ever raising Closing. The unsaved-changes walk never ran, so dirty edits were discarded without a question - and OnClosed's session save never ran either, so with the saved tab list already cleared at startup the updated app relaunched empty-handed. The walk now lives in ConfirmAllUnsavedWorkAsync, reused by the close path and run by the About window before the restart; a Cancel aborts the restart with the update still downloaded, and PersistSessionForRestart writes the open tabs down after the walk (a Save answer can give a scratch tab a file worth restoring). - SaveQueryToPath wrote the user's file with a plain truncate-then- write, so a save that died halfway - disk full, crash - destroyed the only copy of the file it was trying to update. It now stages through AtomicFile like the settings writers already did: sibling .tmp, then rename over the top, so a failed save leaves the original bytes on disk and the session dirty. - "Open in Query Editor" pasted a plan's statement over the editor unconditionally - the one wholesale overwrite that skipped the dirty tracking entirely. It now confirms (Replace/Cancel) when the editor holds typed-but-unsaved work; a clean or empty editor replaces without a prompt, exactly as before. ConfirmationDialog rather than the three-button UnsavedChangesDialog, because a Save answer would need the save pipeline that lives on MainWindow; the dialog's fixed button width became a minimum so the Replace caption is not clipped. Tests pin the walk (a clean window answers yes without a prompt, a dismissed prompt refuses with everything intact), the restart persistence, save-in-place round-tripping with no staging file left behind, a save that cannot stage leaving the original untouched, and all three editor-replacement paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
#456 taught the parser and analyzer to descend into stored procedure and UDF bodies via the shared PlanStatements.EnumerateAll, but four consumers kept walking batch.Statements and quietly saw a different plan: - BenefitScorer.ScoreCancellable never visited body statements, so their warnings had no MaxBenefitPercent and no wait-stat scoring — the UI sorts unquantified warnings last, burying every finding of an EXEC plan. - PlanAnalyzer.ApplySeverityOverrides skipped body statements, so a user's severity override silently did not apply inside a procedure. (MarkLegacyWarnings was checked and is fine: it runs per-statement from inside the analyzer's EnumerateAll loop.) - The desktop statements grid and the MCP session registration showed one row (the EXEC's synthetic root) and near-zero counts while Human/Robot Advice discussed warnings the UI could not display or navigate to. The same mismatch lived in McpQueryStoreTools.CaptureSession, whose counts now come from the analysis summary stored on the same session, and in ParsedPlan.AllMissingIndexes, which feeds both registrations. - The web viewer's statement tabs index result.Statements (EnumerateAll order) but ActiveStmtPlan mapped that index into the outer-only batch list, so clicking a body statement's tab rendered no operator tree. Body statements need a visible home, so the traversal grew a context- carrying form, EnumerateAllWithContainer, that pairs each statement with the module path it lives in ("dbo.Proc", "dbo.Outer > dbo.Inner"); EnumerateAll is defined on top of it so the two cannot diverge. The grid prefixes body rows with that path for display only — copy and open-in-editor still hand out the statement text exactly as recorded. Tests: body warnings carry benefit scores (fixture) and wait-stat warnings (synthetic nested plan); severity overrides reach body statements; container paths are asserted; and a headless-UI test drives the real PlanViewerControl over the exec_stored_procedure fixture, asserting the grid row count matches EnumerateAll, body rows carry the module prefix, and selecting a body statement renders its operators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
#431 made AnalysisJson the one place that knows how deep a serialized AnalysisResult goes (MaxDepth 1024 against the System.Text.Json default of 64, two JSON levels per operator), precisely because inline options keep getting rebuilt without it. Review found three more such sites, all on the web share path, so a deep-but-realistic plan (~30 nested operators) analyzed fine and then failed to Share — or shared and failed to load — with an "object cycle" message pointing at the wrong cause: - PlanShareService.ShareAsync serialized the upload envelope with default options; - PlanShareService.LoadAsync parsed the share with default JsonDocumentOptions and deserialized the result at the default 64 again; - server/PlanShare's /api/share parsed the uploaded body with default JsonDocumentOptions, turning a legitimate deep upload into 400 "Invalid JSON" before ttl_days was ever read. AnalysisJson grows a Wire options set (default formatting, only the ceiling raised — shares already in the database were written unindented with nulls, and the fix is the ceiling, not a wire-format change) and a Document counterpart for JsonDocument.Parse call sites. The web project links AnalysisJson.cs the way it links the rest of Core's sources. The server cannot reference PlanViewer.Core, so it mirrors the constant as a literal with a comment naming AnalysisJson as the source of truth — a shared constant only helps call sites that reference it; this one cannot. The Core depth test now says so too, as the tripwire for anyone changing the number. Tests: the exact share envelope shape ({result, text, ttl_days} → JsonDocument → GetRawText → Deserialize) round-trips a 100-operator chain through the shared options, and the default reader is shown to reject the same payload so the options are provably load-bearing. The Web and server call sites themselves are out of this suite's reach (Blazor WASM project not referenced; server references nothing), so they are verified by inspection and the contract is pinned here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…losed Two findings from the gate review of the restart guard: - Every branch of UpdateLink_Click awaits (dialog, unsaved-work walk, download) with the link still clickable, and the walk made the window between click and restart arbitrarily long - a second click started a concurrent copy of whichever step was in flight. One latch at the top now covers all branches. - The walk was gated on 'Owner is MainWindow', which fails OPEN: shown with any other owner, the guard silently vanishes and the route is back to discarding dirty edits. The main window is now resolved through the application lifetime with Owner as the fast path; if no main window exists at all there are no sessions to lose, so restarting without a walk is genuinely safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…tion Finish #456's descent: score, override, display, and share proc-body statements
Same reentrancy class the previous commit fixed in AboutWindow: the Replace confirmation put an await between the dirty check and the assignment, so two back-to-back Open in Query Editor clicks stacked two prompts over the same buffer. Not data loss - the assignment stays gated on an explicit Replace - but the first click now wins and the second is a no-op while the prompt is up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Close three ways the app could quietly lose a user's work
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
Stop the test harness from mutating real machine state
…riable columns Three verified findings from the adversarial review, all in the Core analysis path. "Implicit Conversion" is both rule 29's legacy-listed type and what the parser stamps on the engine's own PlanAffectingConvert record (#436's Source split), and both the legacy pass and TryOverrideSeverity matched by type name alone. The engine's record rendered "[SQL Server] [legacy]" - a migration badge for OUR un-migrated rules, on a warning that is not ours - and a user's rule-number severity override landed on engine warnings the rule never produced, with the Contains matching spreading it wider (every engine Spill variant onto rule 7, "Memory Grant" onto rule 9). Both passes now skip anything stamped Source=SqlServer. ColumnReferenceRegex excluded @ from the first bracket part to keep variables ([@p]) from reading as columns, but a table-variable COLUMN renders [@tv].[col] - so a real column-side CONVERT_IMPLICIT on one lost its Non-SARGable warning. The "].[" sequence is what a bare variable can never have, so it alone draws the line now. ParameterSubstitution's assignment guard (#482) missed three shapes that produced misleading copied SQL: SELECT TOP (1) @A = col (the back-scan met TOP's ")" and gave up, substituting @A into "NULL = col"), EXEC @rc = proc and the FIRST named argument of EXEC dbo.p @debug = @debug (what precedes them is a procedure name, not a keyword; inside EXEC grammar "=" only ever means assignment, so a statement leading with EXEC/EXECUTE settles it), and FETCH ... INTO @A, @b (assignment with no "=" at all - the INTO leading the list answers it, for every member of the list). Reads keep substituting: WHERE @A = 1, UPDATE ... SET col = @p, TOP (@n), positional EXEC arguments and IN lists are all pinned by tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
The defect #452 fixed at the session-level Query Store site, fixed at the five sites that still had it: two fetch paths, the metric refresh, the database check, and the time slicer all cut exception text to 60-80 characters + "..." before handing it to StatusText. The interesting half of a SQL error - the login failure, the firewall hint - is rarely in its first 60 characters, and the cut threw it away for good. The sites now pass the full message, and the strip mirrors whatever it shows into its tooltip, which is #452's recovery path. One PropertyChanged subscription in the constructor rather than a tooltip set at each error site: this control writes StatusText from a dozen places across five partials, and a tip set only on errors would go stale the moment "Fetching plans..." overwrote the text but not the tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
… saves Two file-fidelity findings from the review. PerformanceStudio.exe query.sql showed "The XML is not valid" where the query should have been: the constructor's argv handling was the one path still hard-wired to LoadPlanFile, while the second-instance pipe, drag-and-drop, and session restore all routed through OpenFileByExtension - whose .sql consequence RestoreOpenPlans' own doc comment spells out verbatim. The argv path now takes the same router, split out as OpenFromStartupArgs so a test can hand it an argv of its choosing. Opening a .sql read its BOM correctly (File.ReadAllText honors it) and the first save wrote UTF-8 without one - a UTF-16 file from SSMS was silently transcoded in place, every byte changed, nothing asked. The mark's encoding is captured at open onto the session next to SourceFilePath and handed to AtomicFile at save. A BOM sniff rather than StreamReader.CurrentEncoding, which cannot tell a UTF-8-BOM file from a plain one and whose default instance would stamp marks onto files that never had them. BOM-less files and scratch queries keep the UTF-8-without-BOM every save always wrote, pinned by test alongside the UTF-16 round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Detaching a session never recomputed its Compare button: #447 made the count window-wide, the sub-tab watcher only fires on sub-tab changes, and a detach makes none - so the torn-off window kept offering a comparison whose second plan it could no longer reach, until the next plan landed. DetachTabToWindow now runs the session's own recompute, which with no MainWindow above it counts the session's own plans - the only honest answer in a detached window. Redock needs no twin call; adding the tab back fires the window's collection watcher. MainWindow.OnClosing latches _closeConfirmed only after the unsaved-work walk answers yes, so a second close arriving mid-walk (a double-clicked X, an Alt+F4 behind a Save As picker) started a second concurrent walk: duplicate prompts about the same tabs, and a Cancel one walk never heard. One in-progress latch, the same reentrancy class and the same shape as the About window's update link (#485 review). DetachedWindowHelper had the same gap - its closeConfirmed also latches only on a yes, and the Save As its prompt can raise is not modal to the window - so it carries the same latch. CreateTab subscribed a close-glyph refresher onto the session and nothing ever took it off; the session outlives its tab on the detach path, so every detach/redock cycle pinned one more dead TabItem (closure over its close button, visual tree and all) to the session for good. The unhook is remembered per-tab in a ConditionalWeakTable - a Dictionary would be its own leak for tabs that close normally - and detach removes exactly its own subscription; redock re-subscribes through CreateTab, and the glyph still tracks dirty state afterwards, by test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…xth truncation site The gate review proposed narrowing assignsThroughEquals to the argument list's own nesting, citing reads like EXEC dbo.p @Flag = CASE WHEN @x = 1. That statement cannot compile: T-SQL restricts an EXEC argument's value to a literal, a variable, NULL, or DEFAULT - expressions are a syntax error - so no legal compiled statement puts a read to the right of a named argument, and the only @name = pairs in the region the flag governs are assignments. Written into StatementLeadsWithExec's doc so the question is settled where the next reader will ask it. A crafted plan file can hold anything, but its author controls the whole statement text, so per-token precision against it protects nothing. Also picks up the SIXTH error pre-truncation site the sweep noticed out of scope (PlanViewerControl.Interaction.cs SavePlan catch) with the same mirror the other five got: full message out, tooltip carries the clip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Sweep the review's minor findings: analyzer honesty, error display, lifecycle warts
#488 gave DetachedWindowHelper the same walk-in-progress latch MainWindow.OnClosing got, for the same gap, but only the MainWindow half was pinned. Deleting the detached closeGuardPending check outright left the suite green — which is how a latch gets tidied away by someone who reasonably believes the tests are watching it. Same shape as the existing test, against the detached window: close once and the question comes up, close again and no second prompt stacks, dismiss it and the window stays with the latch cleared, close once more and a fresh question starts. Proved red against the latch's absence before it went green. Claude-Session: https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#456 taught the parser to read StoredProc/UDF sub-plan bodies, but that descent lives in ParseStatement - and a StmtCursor's operation statements never pass through ParseStatement. They are built in the cursor branch of ParseStatementAndChildren straight from CursorPlan > Operation > QueryPlan, so a function called by the cursor's query carried its whole body in the XML (the Operation element holds the UDF sub-plan beside its QueryPlan) and the parser dropped every statement of it: not enumerated, not analyzed, not counted. Same failure mode as #455 - well-formed, plausible, and silently incomplete output. Fix (#491): extract ParseStatement's UDF/StoredProc reads into a shared ParseSubPlans helper and call it from the cursor branch on each Operation element, attaching the bodies to that operation's statement. That is the whole integration: PlanStatements.EnumerateAll already walks UdfPlans/StoredProcPlan on every statement it yields, and since #486 every consumer (analyzer, scorer, result mapper, statements grid, web viewer) reads that traversal, so the bodies flow through analysis and counts with no consumer changes. The caller's depth carries into the new descent unchanged, preserving #484: a generated bomb alternating cursor and procedure shapes past MaxParseDepth throws the catchable depth error on the big-stack thread, and was verified to fail cleanly (parse completes, assert reports the miss) against a simulated depth reset at the cursor boundary. Tests use generated XML on the depth-limit tests' precedent - a cursor wrapping a UDF sub-plan is three nested element shapes, stated more clearly by a minimal document than a captured fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…bbering settings (#489) Two full instances each hold a whole-file AppSettings snapshot and Save writes the whole file, so the instance that exits second silently overwrites the other's open_tabs and every other setting - AtomicFile prevents torn writes, not lost updates. Launches with a file argument already forwarded to the running instance over the SQLPerformanceStudio_OpenFile pipe; a bare second launch ran a full instance and was exactly the clobber case. Program.Main now claims a named mutex (SQLPerformanceStudio_SingleInstance, default per-user-session scope, held for the process lifetime - the previous mutex attempt died because its handle was disposed on return). A launch that finds the slot taken hands its work to the owner over the existing pipe - the file path it was given, or a reserved ::activate:: sentinel meaning "surface your main window" - and exits. The sentinel is deliberately unrepresentable as a Windows file name: a pre-#489 receiver File.Exists-guards every pipe line, so an old running build reads it, finds no such file, and drops it without harm. The with-file pipe probe stays first and unchanged, which also keeps version skew safe in the other direction (an old running build answers its pipe but holds no mutex). If the owner never answers after ~2s of retries (wedged, or still booting before its pipe server is up), the launch proceeds as a full instance: losing the user's double-clicked file, or the app simply appearing, is worse than a rare second instance. That is also the honest residue of the startup race - two simultaneous bare launches can both proceed if the loser's retries run out before the winner's pipe exists; the remainder falls back to the old last-write-wins behavior as the accepted floor. --new-instance skips the single-instance check for users who run two on purpose. It is scrubbed from argv both in Program.Main and inside OpenFromStartupArgs, because MainWindow consumes the raw Environment.GetCommandLineArgs() itself - so "PerformanceStudio.exe --new-instance file.sqlplan" still opens the file. The receiver's line dispatch is extracted into a testable seam (SingleInstance.Classify + MainWindow.DispatchPipeMessage): sentinel surfaces (UI-thread marshal, restore from minimized, Activate), an existing path opens exactly as the SSMS extension has always relied on - and now also restores a minimized window instead of loading into the taskbar - and garbage is ignored as before. Headless tests pin the grammar, the old-receiver degradation property, and the argv scrubbing; the mutex/exit flow is process-level, out of the harness's reach (it boots App directly, never Main), and is verified by inspection and commented in Program.cs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
) Session restore had two gaps, both from writing the list only at clean close (#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 #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 #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 (#451/#487). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Descend into UDF and procedure bodies inside cursor plans
…string property Two gate findings. The version-skew-proof comment overclaimed: it holds for with-file launches only, and a bare launch beside a pre-mutex build deliberately runs fully rather than probing first - an old receiver drops the sentinel while delivery reports success, so probe-first would exit having surfaced nothing, worse than one transient window of the pre-#489 status quo. Documented beside the same-version race instead of solved; a real fix needs an acknowledged surfacing protocol. The sentinel test was vacuous on the ubuntu CI runner, where colons are legal in file names and File.Exists only proved the test CWD was clean. The property that carries the old-Windows-receiver guarantee is the colon itself, so it is now pinned as a string property (plus the actual invalid-chars check where the guarantee lives), which catches a tidied path-representable token on every platform. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…inuity Persist the session continuously so restore survives the exits it exists for
…him in CI The gate's third finding: if the named mutex throws wholesale on some platform, the catch-all's run-fully degradation would silently no-op single-instancing and #489's clobber would be back with no symptom pointing here. Two signals now exist. A unit test drives the named create and second-open paths on whatever platform the suite runs on - the ubuntu runner on every push - so a shim that throws fails CI loudly before it degrades invisibly in the field. And the catch-all says so on stderr, which a Windows GUI launch loses harmlessly while the Unix platforms most likely to hit it are exactly where terminal launches are common. macOS has no CI leg; the bare-double-launch smoke there is a release-checklist item. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…-launch Surface the running instance on a bare second launch instead of clobbering settings
Move the quarterly maintenance skill out of the personal ~/.claude/skills directory into .claude/skills so it travels with the repo to other machines and to cloud sessions, which do not read the local personal skills directory. The skill covers both PerformanceMonitor and PerformanceStudio, and project skills only load inside their own repo, so it is committed to both. .gitignore now un-ignores .claude/skills only. settings.local.json, CLAUDE.md, and worktrees/ stay ignored. The file was named skill.md; renamed to SKILL.md so it resolves on case-sensitive filesystems. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbYty5EYQwd4YGTDcJfEkE
#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 (#462/#469/#473/#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 (#487/#451), pinned in TestHostIsolationTests. Session list: scratch tabs enter the #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 #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 #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 #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 #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 #495 poison invariant mirrored. Size cap ~1MB: past it the buffer is removed rather than left stale, and that one tab behaves pre-#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
Add the maintenance skill as a project skill
…e test The Avalonia sweep caught the one PutAwayMainWindow call that omitted the session, unlike its two siblings. On an assertion failure before the Don't Save click lands the scratch tab is still dirty, and a PutAway that skips MarkClean raises the #462 walk's modal during teardown - the leaked-window session poisoning #474's helper exists to prevent, biting exactly while masking the real failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
The session variable was declared inside the try, so referencing it from finally did not compile - the exact reason the test hoists window above the try, unmirrored. The broken intermediate commit shipped because a piped 'dotnet test | tail' reports the pipe's exit code, not the test run's; the repo documentation warns about precisely this, and this run was verified by its real exit code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…rash flows The code-review gate found three ways the orphan sweep or a drop could destroy scratch content the user never chose to discard — the exact invariant #496 exists to uphold. Blocking: a cold-start file-argument launch (Explorer double-click, SSMS with Studio closed) took an either/or branch that SKIPPED restore, so the continuous writer overwrote the saved list without the previous session's scratch entries and the next start's sweep deleted their buffers. OpenFromStartupArgs now restores first and opens the file on top, with the fallback scratch tab suppressed when a file is coming. Major: a mid-restore crash leaves the poison-cleared list empty, so the next start's sweep would delete every buffer including bystanders a different entry's crash stranded. The sweep is now age-gated (3-day grace) — a fresh orphan lingers as a recoverable .sql instead of dying; chosen deletions (Don't Save, Save) never come through the sweep, so they are unaffected. Major: the shutdown force-close drop could delete a buffer minted during the close walk (a detached scratch typed into while another window's modal prompt was up), leaving a dangling entry. Gated on !IsShuttingDown — OnClosed's final flush has already made every keep-or-drop decision by then. Minor: the trailing-edge debounce had no ceiling, deferring persistence indefinitely under continuous typing; a 10s max-latency cap now forces a write while keeping ordinary 2s debouncing once typing pauses. Two new tests (fresh-vs-aged sweep, file-arg cold start restoring the scratch and opening the file); the orphan test now backdates past the grace period, deriving the offset from the store's own constant. Also fixes the test-hygiene finding from the Avalonia sweep: the Don't Save test now puts its session away with its window like its siblings, so a failed assertion cannot leak a modal into later tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
…stence Persist scratch query buffers so a crash cannot take never-saved work
App, SSMS manifest, and SSMS AssemblyInfo together - the extension's two hand-bumped sites are the drift that bit releases before 1.14.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
v1.24.0 release prep: version bump
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes dev to main for the v1.24.0 release: the September hardening line. Fifteen merges since v1.23.0 — the crafted-plan crash fix (#484, live in v1.22/v1.23), the data-loss cluster (#485, #494, #495, #498: update-restart guard, atomic saves, single-instance, continuous session persistence, scratch-buffer survival), proc/cursor-body analysis integration (#486, #493), the web share depth fix completing end to end via the Pages deploy, the minor-findings sweep (#488), test-host isolation (#487), and the version bump (#499).
On merge: release.yml cuts the tag and assets, deploy-web ships the viewer to plans.erikdarling.com, and deploy-planshare re-deploys the API from main.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n