From fa1d4f67b8c5623b4cff7f3b883d74b2361bb480 Mon Sep 17 00:00:00 2001 From: Colin Neilens Date: Wed, 23 Sep 2026 01:48:56 -0700 Subject: [PATCH] Replace vacuous Windows shell tests and make the anti-drift guard structural Three tests were executing in CI while being structurally incapable of failing, and the guard meant to prevent exactly that could not see the regression it was built to catch. UpdateOfferDialog.zig asserted `expectEqual(Action.later, .later)` -- an enum compared to itself, invoking no production code at all, under a name claiming to cover "install unavailable". The real contracts lived inside windowProc and createButton, unreachable from a unit test. Extract them into `buttons` and `actionForCommand`, have windowProc consume those instead of open-coding the same decisions, and assert the actual behaviour: Install is presented disabled and maps to install_unavailable, the two honourable actions stay enabled, every command id routes to its declared action, unknown commands return null, ids are distinct, and dismissal defers rather than implying an install. Because windowProc now reads the same declarations the tests assert on, the two cannot drift. QuickChats.zig had zero importers anywhere in the repo. Its Availability enum had a single variant and Controller returned it unconditionally, so "quick chat operations are available through the daemon controller" could not fail without a compile error, while protocolGapMessage emitted "unavailable" strings the Controller could never produce. Its one genuine assertion covered Wire.commandName, so move that into Wire.zig -- the file that owns commandName -- and delete the dead scaffold. The anti-drift guard (issue #424) compared test-bearing sources against a hand-maintained list rather than against the zig test invocations it claimed to describe. Deleting an invocation while leaving its name in the list silently stopped running those tests and still passed: verified by removing the Navigation.zig invocation, which the old guard reported as PASS. Derive the wired set from the script's own invocations so the guard observes reality instead of a description of it, and fail loudly if derivation yields nothing rather than vacuously passing. Evidence (pinned Zig 0.15.2): UpdateOfferDialog 4/4, Wire 61/61, App 233/233. Six mutations of the new dialog assertions all caught; mutating Wire.commandName caught by the relocated assertions; removing an invocation, typo'ing one, dropping a file from a multi-file invocation, and breaking derivation are all now caught, where the first previously passed. Signed-off-by: Colin Neilens Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Tools/windows/Tests/WindowsShell.Tests.ps1 | 78 ++++--------- graphcode-windows/src/QuickChats.zig | 53 --------- graphcode-windows/src/UpdateOfferDialog.zig | 123 +++++++++++++++----- graphcode-windows/src/Wire.zig | 5 + 4 files changed, 117 insertions(+), 142 deletions(-) delete mode 100644 graphcode-windows/src/QuickChats.zig diff --git a/Tools/windows/Tests/WindowsShell.Tests.ps1 b/Tools/windows/Tests/WindowsShell.Tests.ps1 index 669b7915..cb1776b1 100644 --- a/Tools/windows/Tests/WindowsShell.Tests.ps1 +++ b/Tools/windows/Tests/WindowsShell.Tests.ps1 @@ -562,10 +562,6 @@ Invoke-Native "Sidebar navigation executable tests" { Push-Location $shellRoot try { & $zig test src\Navigation.zig } finally { Pop-Location } } -Invoke-Native "Quick chats executable tests" { - Push-Location $shellRoot - try { & $zig test src\QuickChats.zig } finally { Pop-Location } -} Invoke-Native "Workspace controls executable tests" { Push-Location $shellRoot try { & $zig test src\WorkspaceControls.zig } finally { Pop-Location } @@ -669,60 +665,28 @@ Invoke-Native "App shell executable tests" { } # Structural anti-drift guard (issue #424): every graphcode-windows\src\*.zig file -# that declares at least one `test "..."` block must be executed by one of the -# `zig test` invocations above. Add the new file's name here as part of wiring it -# in; forgetting either step (the invocation or this list) fails this guard -# instead of letting the tests silently never run. This check runs last, after -# every other invocation above, so a real regression in an individual file's -# tests is reported before this contract-only failure short-circuits the run. +# that declares at least one `test "..."` block must actually be executed by one of +# the `zig test` invocations above. This check runs last, after every other +# invocation, so a real regression in an individual file's tests is reported before +# this contract-only failure short-circuits the run. # -# GraphContextMenu.zig and MainWindow.zig were wired by in-flight issue #418 -# (PR #422, merged as 06e092e) after this guard was first added here; #422 -# added the zig test invocations and the source-list entries above but never -# touched this list, since it did not exist on main when #422 was authored. -# Listed here after rebasing onto main so the guard reflects reality post-merge. +# The wired set is DERIVED from this script's own `zig test` invocations rather than +# from a hand-maintained list. A hand-maintained list is a second source of truth +# that can drift from the invocations it claims to describe: deleting an invocation +# while leaving its name in the list would silently stop executing those tests and +# still pass the guard -- exactly the regression #424 exists to prevent. Deriving the +# set from the invocations themselves makes the guard observe reality instead of a +# description of it, and removes the second place to forget when wiring a new file. +$guardScriptText = Get-Content -LiteralPath $PSCommandPath -Raw $wiredTestFiles = @( - "Wire.zig", - "Codespaces.zig", - "WindowsCodespaceDialog.zig", - "Forms.zig", - "Win32.zig", - "NativeForms.zig", - "UpdateOfferPresentation.zig", - "GraphContextMenu.zig", - "MainWindow.zig", - "JumpPalette.zig", - "WindowsOnboarding.zig", - "WindowsProductSettings.zig", - "WindowsUpdates.zig", - "FrameBuffer.zig", - "DaemonClient.zig", - "DaemonSupervisor.zig", - "WorkspaceLayout.zig", - "InputRouter.zig", - "TerminalSurface.zig", - "GraphModel.zig", - "CanvasInput.zig", - "GraphCanvas.zig", - "WorktreeStatus.zig", - "DraftAttachments.zig", - "WorktreeDialog.zig", - "Dpi.zig", - "TemplateLibrary.zig", - "WorkspaceLifecycle.zig", - "Navigation.zig", - "QuickChats.zig", - "WorkspaceControls.zig", - "Sidebar.zig", - "WindowsRepositoryDialogs.zig", - "GdiGradient.zig", - "AppFont.zig", - "GdiplusAA.zig", - "UpdateOfferDialog.zig", - "WindowsNativeDialogs.zig", - "Accessibility.zig", - "App.zig" -) + ($guardScriptText -split "`r?`n") | + Where-Object { $_ -match '\$zig test' } | + ForEach-Object { [regex]::Matches($_, 'src\\([A-Za-z0-9_]+)\.zig') } | + ForEach-Object { "$($_.Groups[1].Value).zig" } +) | Sort-Object -Unique +if ($wiredTestFiles.Count -eq 0) { + throw "Windows shell contract: the anti-drift guard derived zero `zig test` invocations from $PSCommandPath, so it cannot verify anything (see issue #424)." +} $missingTestFiles = @( Get-ChildItem -LiteralPath (Join-Path $shellRoot "src") -Filter "*.zig" -File | Where-Object { @@ -735,5 +699,5 @@ if ($missingTestFiles.Count -ne 0) { throw "Windows shell contract: the following src\*.zig files contain test blocks but are not wired into any zig test invocation in WindowsShell.Tests.ps1 (see issue #424): $($missingTestFiles -join ', ')" } -Write-Output "Windows shell scaffold contract: PASS" +Write-Output "Windows shell scaffold contract: PASS ($($wiredTestFiles.Count) source files executed)" exit 0 diff --git a/graphcode-windows/src/QuickChats.zig b/graphcode-windows/src/QuickChats.zig deleted file mode 100644 index 408f8305..00000000 --- a/graphcode-windows/src/QuickChats.zig +++ /dev/null @@ -1,53 +0,0 @@ -const std = @import("std"); -const Wire = @import("Wire.zig"); - -pub const Availability = enum { - available, -}; - -pub const Operation = enum { - create, - open, - rename, - delete, -}; - -pub const Result = union(enum) { - accepted: Availability, -}; - -pub const Controller = struct { - pub fn availability(_: Controller) Availability { - return .available; - } - - pub fn request(_: Controller, _: Operation) Result { - return .{ .accepted = .available }; - } -}; - -pub fn protocolGapMessage(operation: Operation) []const u8 { - return switch (operation) { - .create => "Quick chats: create command unavailable", - .open => "Quick chats: open command unavailable", - .rename => "Quick chats: rename command unavailable", - .delete => "Quick chats: delete command unavailable", - }; -} - -test "quick chat operations are available through the daemon controller" { - const controller = Controller{}; - try std.testing.expectEqual(Availability.available, controller.availability()); - inline for (std.meta.tags(Operation)) |operation| { - const result = controller.request(operation); - try std.testing.expectEqual(Availability.available, result.accepted); - } -} - -test "authoritative wire command vocabulary includes every quick chat operation" { - try std.testing.expectEqualStrings("listQuickChats", Wire.commandName(.list_quick_chats)); - try std.testing.expectEqualStrings("createQuickChat", Wire.commandName(.create_quick_chat)); - try std.testing.expectEqualStrings("openQuickChat", Wire.commandName(.open_quick_chat)); - try std.testing.expectEqualStrings("renameQuickChat", Wire.commandName(.rename_quick_chat)); - try std.testing.expectEqualStrings("deleteQuickChat", Wire.commandName(.delete_quick_chat)); -} diff --git a/graphcode-windows/src/UpdateOfferDialog.zig b/graphcode-windows/src/UpdateOfferDialog.zig index b1d2ac51..ae149841 100644 --- a/graphcode-windows/src/UpdateOfferDialog.zig +++ b/graphcode-windows/src/UpdateOfferDialog.zig @@ -18,9 +18,40 @@ const State = struct { }; const class_name = std.unicode.utf8ToUtf16LeStringLiteral("GraphCodeUpdateOffer"); -const install_id = 9701; -const release_notes_id = 9702; -const later_id = 9703; +const install_id: u16 = 9701; +const release_notes_id: u16 = 9702; +const later_id: u16 = 9703; + +pub const ButtonSpec = struct { + label: []const u8, + id: u16, + x: i32, + width: i32, + enabled: bool, + action: Action, +}; + +const button_row_y: i32 = 190; + +/// The dialog's button row. `windowProc` creates exactly these controls and +/// routes WM_COMMAND through `actionForCommand`, so these specs are the +/// presented behaviour rather than a parallel description of it. +pub const buttons = [_]ButtonSpec{ + .{ .label = "Install", .id = install_id, .x = 18, .width = 140, .enabled = false, .action = .install_unavailable }, + .{ .label = "Release Notes", .id = release_notes_id, .x = 160, .width = 140, .enabled = true, .action = .release_notes }, + .{ .label = "Later", .id = later_id, .x = 470, .width = 110, .enabled = true, .action = .later }, +}; + +/// Action taken when the dialog is dismissed without pressing a button. +pub const dismiss_action: Action = .later; + +pub fn actionForCommand(command: u16) ?Action { + for (buttons) |spec| { + if (spec.id == command) return spec.action; + } + return null; +} + var active = false; var active_state: State = undefined; @@ -98,31 +129,19 @@ fn windowProc(hwnd: c.HWND, message: c.UINT, wparam: c.WPARAM, lparam: c.LPARAM) createStatic(hwnd, active_state.allocator, version_text, 18, 46, 560, 24); createStatic(hwnd, active_state.allocator, "Release Notes opens the verified GraphCode release page.", 18, 76, 560, 24); createStatic(hwnd, active_state.allocator, active_state.reason, 18, 106, 560, 44); - createButton(hwnd, "Install", install_id, 18, 190, false); - createButton(hwnd, "Release Notes", release_notes_id, 160, 190, true); - createButton(hwnd, "Later", later_id, 470, 190, true); + for (buttons) |spec| createButton(hwnd, spec); return 0; }, c.WM_COMMAND => { const command: u16 = @truncate(wparam); - if (command == install_id) { - active_state.action = .install_unavailable; - requestClose(hwnd); - return 0; - } - if (command == release_notes_id) { - active_state.action = .release_notes; - requestClose(hwnd); - return 0; - } - if (command == later_id) { - active_state.action = .later; + if (actionForCommand(command)) |action| { + active_state.action = action; requestClose(hwnd); return 0; } }, c.WM_CLOSE => { - active_state.action = .later; + active_state.action = dismiss_action; requestClose(hwnd); return 0; }, @@ -164,32 +183,32 @@ fn createStatic( AppFont.apply(control, AppFont.control_size, false); } -fn createButton(hwnd: c.HWND, text: []const u8, id: usize, x: i32, y: i32, enabled: bool) void { - const wide = wideZ(std.heap.c_allocator, text) catch return; +fn createButton(hwnd: c.HWND, spec: ButtonSpec) void { + const wide = wideZ(std.heap.c_allocator, spec.label) catch return; defer std.heap.c_allocator.free(wide); - const disabled_style: c.DWORD = if (enabled) 0 else @as(c.DWORD, @intCast(c.WS_DISABLED)); + const disabled_style: c.DWORD = if (spec.enabled) 0 else @as(c.DWORD, @intCast(c.WS_DISABLED)); const style: c.DWORD = @as(c.DWORD, @intCast(c.WS_CHILD | c.WS_VISIBLE | c.WS_TABSTOP | c.BS_PUSHBUTTON)) | disabled_style; const button = c.CreateWindowExW( 0, std.unicode.utf8ToUtf16LeStringLiteral("BUTTON").ptr, wide.ptr, style, - x, - y, - if (id == later_id) 110 else 140, + spec.x, + button_row_y, + spec.width, 30, hwnd, - controlId(id), + controlId(spec.id), c.GetModuleHandleW(null), null, ) orelse return; AppFont.apply(button, AppFont.control_size, false); - _ = c.EnableWindow(button, if (enabled) 1 else 0); + _ = c.EnableWindow(button, if (spec.enabled) 1 else 0); } -fn controlId(value: usize) c.HMENU { +fn controlId(value: u16) c.HMENU { @setRuntimeSafety(false); - return @ptrFromInt(value); + return @ptrFromInt(@as(usize, value)); } fn wideZ(allocator: std.mem.Allocator, value: []const u8) ![]u16 { @@ -202,7 +221,47 @@ fn wideZ(allocator: std.mem.Allocator, value: []const u8) ![]u16 { } test "update offer keeps install unavailable while preserving explicit actions" { - try std.testing.expectEqual(Action.later, .later); - try std.testing.expectEqual(Action.release_notes, .release_notes); - try std.testing.expectEqual(Action.install_unavailable, .install_unavailable); + // The Install button must be presented but not actionable: an in-app + // installer does not exist yet, and offering an enabled control would + // promise behaviour the app cannot deliver. + const install = buttons[0]; + try std.testing.expectEqualStrings("Install", install.label); + try std.testing.expect(!install.enabled); + try std.testing.expectEqual(Action.install_unavailable, install.action); + + // The two actions the app can honour stay enabled. + for (buttons[1..]) |spec| { + try std.testing.expect(spec.enabled); + try std.testing.expect(spec.action != .install_unavailable); + } + + // Exactly one disabled button, so a future edit cannot quietly disable + // Release Notes or Later and still satisfy the assertions above. + var enabled_count: usize = 0; + for (buttons) |spec| { + if (spec.enabled) enabled_count += 1; + } + try std.testing.expectEqual(@as(usize, 2), enabled_count); +} + +test "update offer routes every button command to its declared action" { + for (buttons) |spec| { + try std.testing.expectEqual(spec.action, actionForCommand(spec.id).?); + } + // Unrecognised commands must not resolve to an action; windowProc relies on + // null to fall through to DefWindowProcW. + try std.testing.expectEqual(@as(?Action, null), actionForCommand(0)); + try std.testing.expectEqual(@as(?Action, null), actionForCommand(install_id + 100)); +} + +test "update offer button command ids are distinct" { + for (buttons, 0..) |spec, i| { + for (buttons[i + 1 ..]) |other| { + try std.testing.expect(spec.id != other.id); + } + } +} + +test "dismissing the update offer defers rather than implying an install" { + try std.testing.expectEqual(Action.later, dismiss_action); } diff --git a/graphcode-windows/src/Wire.zig b/graphcode-windows/src/Wire.zig index 3fda2d90..fbeb3f35 100644 --- a/graphcode-windows/src/Wire.zig +++ b/graphcode-windows/src/Wire.zig @@ -1237,6 +1237,11 @@ test "node drafts with attachments encode PromptAttachment-shaped entries" { } test "quick chat commands match shared Codable labels" { + try std.testing.expectEqualStrings("listQuickChats", commandName(.list_quick_chats)); + try std.testing.expectEqualStrings("createQuickChat", commandName(.create_quick_chat)); + try std.testing.expectEqualStrings("openQuickChat", commandName(.open_quick_chat)); + try std.testing.expectEqualStrings("renameQuickChat", commandName(.rename_quick_chat)); + try std.testing.expectEqualStrings("deleteQuickChat", commandName(.delete_quick_chat)); const allocator = std.testing.allocator; const list = try commandListQuickChats(allocator); defer allocator.free(list);