Guard NativeForms modal reentrancy - #420
Merged
coneilen merged 1 commit intoSep 23, 2026
Merged
Conversation
Reject nested NativeForms presentations, expose the active modal state to the app, and retain completed update offers until the current native form closes. Add focused decision and guard tests plus a mutation-sensitive assertion in the Windows shell contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Colin Neilens <coneilen@microsoft.com>
coneilen
force-pushed
the
coneilen-microsoft-modal-reentrancy-guard
branch
from
September 23, 2026 01:17
2c0c270 to
548fba6
Compare
coneilen
added a commit
that referenced
this pull request
Sep 23, 2026
Reject nested NativeForms presentations, expose the active modal state to the app, and retain completed update offers until the current native form closes. Add focused decision and guard tests plus a mutation-sensitive assertion in the Windows shell contract. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen
added a commit
that referenced
this pull request
Sep 23, 2026
Cherry-picked #420's NativeForms/UpdateOfferPresentation harness wiring (commit 775d77b) onto this branch: my branch point (68eabe5) predated that merge, so the reentrancy-guard Assert-Contract, the source-list entry, and the zig test invocation for UpdateOfferPresentation.zig were all absent here even though they exist on main. Restored via cherry-pick rather than a full rebase, per instruction to hold on rebasing until #422 lands. That cherry-pick alone was not sufficient: this PR's own anti-drift guard maintains a second, independent file list (\) that #420 never touched (the guard did not exist on main). Newly restoring the UpdateOfferPresentation.zig invocation without adding it to that list would have made the guard itself flag it as unwired. RED: after cherry-picking 775d77b, the guard's \ array still lacked "UpdateOfferPresentation.zig" -> a manual simulation of the guard's detection logic reported it as missing alongside the two entries correctly reserved for #422. GREEN: added "UpdateOfferPresentation.zig" to \ next to "NativeForms.zig" -> the same simulation now reports exactly and only GraphContextMenu.zig and MainWindow.zig as missing, matching the #422 reservation. REGRESSION: reran the full harness end-to-end locally with pinned Zig 0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 37 invocations execute (including the restored NativeForms 96/96 and UpdateOfferPresentation 1/1), the same 3 pre-existing Sidebar.zig failures are tolerated by name, and the guard still throws on exactly GraphContextMenu.zig, MainWindow.zig. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen
added a commit
that referenced
this pull request
Sep 23, 2026
Cherry-picked #420's NativeForms/UpdateOfferPresentation harness wiring (commit 775d77b) onto this branch: my branch point (68eabe5) predated that merge, so the reentrancy-guard Assert-Contract, the source-list entry, and the zig test invocation for UpdateOfferPresentation.zig were all absent here even though they exist on main. Restored via cherry-pick rather than a full rebase, per instruction to hold on rebasing until #422 lands. That cherry-pick alone was not sufficient: this PR's own anti-drift guard maintains a second, independent file list (\) that #420 never touched (the guard did not exist on main). Newly restoring the UpdateOfferPresentation.zig invocation without adding it to that list would have made the guard itself flag it as unwired. RED: after cherry-picking 775d77b, the guard's \ array still lacked "UpdateOfferPresentation.zig" -> a manual simulation of the guard's detection logic reported it as missing alongside the two entries correctly reserved for #422. GREEN: added "UpdateOfferPresentation.zig" to \ next to "NativeForms.zig" -> the same simulation now reports exactly and only GraphContextMenu.zig and MainWindow.zig as missing, matching the #422 reservation. REGRESSION: reran the full harness end-to-end locally with pinned Zig 0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 37 invocations execute (including the restored NativeForms 96/96 and UpdateOfferPresentation 1/1), the same 3 pre-existing Sidebar.zig failures are tolerated by name, and the guard still throws on exactly GraphContextMenu.zig, MainWindow.zig. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen
added a commit
that referenced
this pull request
Sep 23, 2026
Cherry-picked #420's NativeForms/UpdateOfferPresentation harness wiring (commit 775d77b) onto this branch: my branch point (68eabe5) predated that merge, so the reentrancy-guard Assert-Contract, the source-list entry, and the zig test invocation for UpdateOfferPresentation.zig were all absent here even though they exist on main. Restored via cherry-pick rather than a full rebase, per instruction to hold on rebasing until #422 lands. That cherry-pick alone was not sufficient: this PR's own anti-drift guard maintains a second, independent file list (\) that #420 never touched (the guard did not exist on main). Newly restoring the UpdateOfferPresentation.zig invocation without adding it to that list would have made the guard itself flag it as unwired. RED: after cherry-picking 775d77b, the guard's \ array still lacked "UpdateOfferPresentation.zig" -> a manual simulation of the guard's detection logic reported it as missing alongside the two entries correctly reserved for #422. GREEN: added "UpdateOfferPresentation.zig" to \ next to "NativeForms.zig" -> the same simulation now reports exactly and only GraphContextMenu.zig and MainWindow.zig as missing, matching the #422 reservation. REGRESSION: reran the full harness end-to-end locally with pinned Zig 0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 37 invocations execute (including the restored NativeForms 96/96 and UpdateOfferPresentation 1/1), the same 3 pre-existing Sidebar.zig failures are tolerated by name, and the guard still throws on exactly GraphContextMenu.zig, MainWindow.zig. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen
added a commit
that referenced
this pull request
Sep 23, 2026
…-drift guard (#426) * Wire 17 orphaned Zig test files into WindowsShell.Tests.ps1 and add anti-drift guard Fixes #424. WindowsShell.Tests.ps1 hand-maintained a list of zig test invocations that missed 17 files (Accessibility.zig was fixed separately by #421; GraphContextMenu.zig/MainWindow.zig are reserved for in-flight #418/ #422). Wires all 17 remaining orphaned files with per-file link flags verified against pinned Zig 0.15.2, and adds a structural guard that enumerates graphcode-windows\src\*.zig, detects files containing a est " block, and throws if any is missing from the wired-file list -- so this drift cannot recur silently. First-run triage: - WorktreeDialog.zig: one test used a stale fixture (dirty instead of locked) to exercise armConfirmation()'s fail-closed path; sweepSelectable() intentionally permits dirty rows. Fixed the test fixture, not the code. - App.zig: one test's App struct literal predated three fields (sidebar_state, declared_entry_ids, kept_worktree_paths) added since it last compiled. Fixed the test to match App.init()'s initialization. - Sidebar.zig: 3 real, pre-existing test failures traced to one root cause (layoutFor()/projectSectionHeight() count a recent_projects entry that is also the open project, but appendRows() correctly excludes it from rendered rows, desyncing row/scroll y-math). Per explicit instruction not to modify Sidebar.zig source, these are quarantined at the harness level with an explicit reason string, not fixed or deleted. Reported as a real product bug for separate follow-up. - App.zig transitively reruns the same 3 Sidebar failures (it imports Sidebar.zig); quarantined identically with a note explaining why. RED: WorktreeDialog and App.zig tests failed on first run -> both had stale fixtures, not product bugs; fixed the tests to match current code. GREEN: all 17 newly-wired files now execute; 90/93 Sidebar.zig and 228/231 App.zig tests pass -> the 3 residual failures are quarantined with cause. REGRESSION: full harness run end-to-end with pinned Zig 0.15.2 exits 0 and the anti-drift guard fails loudly on a real mutation removing a wired file -> guard verified functional, not merely asserted. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Run the anti-drift guard last so CI still validates every zig test invocation Moves the structural guard added for #424 to the end of WindowsShell.Tests.ps1, after all zig test invocations, instead of right after Resolve-TestZig. Placed first, the guard's expected failure (reserving GraphContextMenu.zig/MainWindow.zig for #418/#422) short-circuited the whole script in CI before any of the newly-wired tests ever ran on the actual runner, leaving only local verification as evidence. Placed last, CI now executes and reports every invocation for real before the guard's contract check runs, while the guard still fails the job overall until #418/#422 lands. RED: with the guard first, CI failed at the guard on the first push and never exercised a single newly-wired zig test -> no real CI signal existed for the wiring itself, only local runs. GREEN: relocated the guard after every Invoke-Native/Invoke-NativeQuarantined call and reran the full harness locally with pinned Zig 0.15.2 -> all 37 invocations execute (90/93 Sidebar.zig, 228/231 App.zig, everything else 100%), quarantines are tolerated correctly, and the guard still throws last naming exactly GraphContextMenu.zig and MainWindow.zig. REGRESSION: reran the same real-mutation guard check (removing a wired file name) after relocating the block -> guard still throws the identical not-wired message immediately, confirming the guard's detection logic is unchanged, only its position in the script moved. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Point quarantine reason at issue #428 instead of #424 The three Sidebar.zig quarantine entries pointed at #424 (this PR's own tracking issue) with the phrase "first-run finding" as a stand-in for a real bug report, since no dedicated issue existed yet for the Sidebar layout bug when it was first quarantined. The coordinating session filed #428 with the full root-cause writeup (layoutFor()/projectSectionHeight() vs appendRows()'s isProjectOpen skip, the 76px delta, and the hit-testing failure). Repointing the quarantine reason string at #428 so it resolves to the actual bug report instead of this wiring PR. RED: quarantine reason string referenced #424, which is this very PR and not a bug report -> anyone reading the quarantine message would have to guess where the real Sidebar.zig fix should land. GREEN: repointed both Sidebar.zig and App.zig quarantine reason strings to #428 and reran the full harness locally with pinned Zig 0.15.2 -> same 90/93 Sidebar.zig and 228/231 App.zig pass counts, guard still fails last on exactly GraphContextMenu.zig and MainWindow.zig, exit code unchanged. REGRESSION: confirmed Invoke-NativeQuarantined's known-failure matching is by test name string only, independent of the reason text -> the wording change cannot affect which failures are tolerated. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Add UpdateOfferPresentation.zig to the guard's wired-file list Cherry-picked #420's NativeForms/UpdateOfferPresentation harness wiring (commit 775d77b) onto this branch: my branch point (68eabe5) predated that merge, so the reentrancy-guard Assert-Contract, the source-list entry, and the zig test invocation for UpdateOfferPresentation.zig were all absent here even though they exist on main. Restored via cherry-pick rather than a full rebase, per instruction to hold on rebasing until #422 lands. That cherry-pick alone was not sufficient: this PR's own anti-drift guard maintains a second, independent file list (\) that #420 never touched (the guard did not exist on main). Newly restoring the UpdateOfferPresentation.zig invocation without adding it to that list would have made the guard itself flag it as unwired. RED: after cherry-picking 775d77b, the guard's \ array still lacked "UpdateOfferPresentation.zig" -> a manual simulation of the guard's detection logic reported it as missing alongside the two entries correctly reserved for #422. GREEN: added "UpdateOfferPresentation.zig" to \ next to "NativeForms.zig" -> the same simulation now reports exactly and only GraphContextMenu.zig and MainWindow.zig as missing, matching the #422 reservation. REGRESSION: reran the full harness end-to-end locally with pinned Zig 0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 37 invocations execute (including the restored NativeForms 96/96 and UpdateOfferPresentation 1/1), the same 3 pre-existing Sidebar.zig failures are tolerated by name, and the guard still throws on exactly GraphContextMenu.zig, MainWindow.zig. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Wire GraphContextMenu.zig/MainWindow.zig into the guard's list after #422; correct Sidebar quarantine attribution Rebased onto origin/main after #422 (issue #418) merged as 06e092e, which wired GraphContextMenu.zig and MainWindow.zig into the harness's zig test invocations and source-list array. #422 never touched this PR's own \ guard array, because that array did not exist on main when #422 was authored -- it is this PR's own addition. Added both filenames to the list and updated the guard's explanatory comment, which was otherwise now stale (still described the two files as reserved and not-yet-landed). Also corrected the Sidebar.zig quarantine reason: an independent review determined that only 2 of the 3 quarantined failures are the real layoutFor()/appendRows() product bug (#428, fix in flight as #430); the third (sidebar scroll clamps overflow, shrink, and resize, expected 334 found 410) is a separate, stale test expectation -- the 76px delta is the Activity block height that contentBottom/paint() correctly account for and the test's oracle omitted. Left it quarantined (not fixed) since this PR must not modify Sidebar.zig source, but the reason string now attributes each failure accurately instead of lumping all three under one root cause. RED: mechanically diffed this branch's wired-file set against origin/main after rebasing -> main had 22 entries (20 plus #422's 2), this branch still reported only 38 in its own \ guard list, and running the guard's detection logic directly showed it still flagging GraphContextMenu.zig and MainWindow.zig as unwired despite their zig test invocations now existing in the script. GREEN: added both names to \ and reran the same detection logic -> zero missing files reported. REGRESSION: ran the complete harness end-to-end locally with pinned Zig 0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 39 invocations execute including the newly-landed GraphContextMenu.zig (8/8) and MainWindow.zig (6/6, appearing twice via App.zig's transitive import), the same 3 Sidebar.zig failures are tolerated by name with the corrected attribution text rendering intact, and the full script now exits 0 -- the guard no longer fires at all. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Remove Sidebar/App.zig quarantine now that #430 fixed the underlying bug PR #430 (merged as ece5935) fixed the real Sidebar.zig layout/row-count desync structurally via a single shared predicate, projectIsVisibleInSection, so all three previously-quarantined tests now pass unconditionally. Landing the quarantine would have shipped a misleading tolerate-list for an already-fixed bug, so remove it entirely instead: - Delete Invoke-NativeQuarantined and its Get-FailingZigTestNames helper (Sidebar.zig/App.zig were their only consumers). - Delete the sidebarLayoutOpenProjectKnownFailures/-Reason variables. - Convert both call sites to plain Invoke-Native. Verified end-to-end locally with pinned Zig 0.15.2: exit 0, all 93 Sidebar.zig tests and the full 232-test App.zig suite pass unconditionally, zero quarantine output. Re-ran the guard's set-difference check against origin/main (ece5935): empty, with UpdateOfferPresentation.zig and the NativeForms Assert-Contract block both still intact. Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Signed-off-by: Colin Neilens <coneilen@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Fixes #419
NativeForms now rejects reentrant presentations and releases its active flag through a defer on every error and normal return path. The update checker records an offer that completes while a NativeForms modal is active and presents it on a later timer tick after the modal closes.
The parity matrix is unchanged because no validation-status claim changed.
RED: removing the active-state rejection -> named reentrancy test fails with expected FormAlreadyOpen, found void
GREEN: restored guard and deferred-offer decision -> NativeForms 96/96 and focused deferral 1/1 pass on Zig 0.15.2
REGRESSION: pinned Windows shell contract and production build -> PASS without touching GraphCanvas.zig or Sidebar.zig
Validation:
zig build -Dwinghostty-dir=D:\depot\Winghostty-worktrees\host-integrationTools\windows\Tests\WindowsShell.Tests.ps1with pinned Zig 0.15.2 and the real Winghostty include pathnative forms reject reentrant modal acquisition and allow sequential dialogsThis fixes a pre-existing nested-modal risk; it does not claim to explain or fix the previously observed CI hang.