Full app review (unverified, 1/2): engine, XAML, test-quality findings #31
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: KofTwentyTwo/gclo#31
Reference in new issue
Block a user
Finder output from the same full review whose verification pass did not run (agent budget); treat each claim as plausible-but-unconfirmed and re-check the code before acting. Three claims were spot-checked by hand and confirmed: the two already fixed on dev (PAT ToString, dev-channel updates) and the 404 public-repos fallback below. The
viewmodelsandcommentsdimensions never ran at all and still need a pass.Engine
[P1] 404 fallback silently lists only public repos when a private org is invisible to the token
gclo.Engine/GitHubRepositoryLister.cs:62- spot-checked, confirmedWhen GET /orgs/{name}/repos returns 404, the code assumes 'name' is a user account and falls back to GetAllForUser(organization) (lines 48-72). But GitHub also returns 404 for a real private organization the token cannot see — the default situation for a fine-grained PAT whose owner forgot to grant that org, or a classic PAT without SSO authorization. The /users/{login}/repos endpoint answers 200 for organizations too, returning only PUBLIC repos. Failure scenario: user syncs org 'acme' (400 repos, 388 private) with an ungranted fine-grained PAT -> org endpoint 404s -> fallback returns the 12 public repos -> sync completes green with Total=12. A mirror/backup tool reporting success while silently omitting 97% of the org is the worst failure mode it can have; nothing in the summary or log distinguishes this from a genuinely small account.
Proposed fix: Before falling back, resolve the account type: call client.User.Get(organization) and check Type == AccountType.Organization. If it is an organization (or the /users call itself 404s), throw an actionable InvalidOperationException ('organization exists but the token cannot access it — grant the PAT access / authorize SSO') instead of listing public repos. Only take the GetAllForUser path when the account is genuinely a user.
[P2] Upstream default-branch rename makes a repo report 'Updated' forever while receiving no commits
gclo.Engine/LibGit2GitClient.cs:201FetchAndPull fetches without FetchOptions.Prune (lines 147-156), so when the org renames a default branch (master -> main, a routine GitHub migration), the deleted remote branch's tracking ref refs/remotes/origin/master survives at its last SHA. The up-to-date check at line 208 (repo.Head.Tip?.Sha == tracked.Tip.Sha) then short-circuits every subsequent sync as 'already up to date', and the engine counts the repo Updated. Even with pruning, tracked?.Tip is null at line 202 returns silently. Failure scenario: org migrates 200 repos master->main; gclo reports every sync as fully successful indefinitely while every local mirror is frozen at the rename date. For a tool whose one job is keeping org mirrors current, this is silent staleness with a green status.
Proposed fix: Set FetchOptions.Prune = true, and after fetch, if the current branch's tracked ref no longer exists (or origin/HEAD points at a different branch than HEAD tracks), either re-target the local branch to the remote's current default branch (safe here: fast-forward-only tool, and AdvanceHeadToTrackedTip already establishes the no-local-work invariant for managed repos) or throw an InvalidOperationException naming the renamed branch so the repo surfaces as Failed instead of silently Updated.
[P2] Cancellation cannot abandon an in-flight git operation; one wedged repo blocks SyncAsync completion indefinitely
gclo.Engine/OrgSyncEngine.cs:105The only cancellation checkpoints inside a running git operation are libgit2 callbacks: OnTransferProgress (LibGit2GitClient.cs:61-76, 150) and OnCheckoutNotify. Those callbacks fire only while pack data flows. During DNS resolution, TCP connect, TLS handshake, ref negotiation, and proxy auth, nothing is invoked, so the token is never observed; the LongRunning thread blocks inside native code with timeouts (WinHTTP connect/receive) entirely outside the app's control. Parallel.ForEachAsync always awaits in-flight bodies before propagating cancellation, so SyncAsync at lines 105/111 cannot return until the wedged call gives up. Failure scenario: one repo's connection blackholes mid-negotiation; user clicks Cancel; every other repo drains, then the UI sits in 'Syncing' with no Canceled reports and no summary for however long the native transport takes — minutes, or unbounded behind a misbehaving proxy.
Proposed fix: In the ForEachAsync body, await the git task with .WaitAsync(token) (both the FetchAndPullAsync and CloneAsync awaits). On cancellation this abandons the orphaned LongRunning thread (its own catch/TryDeleteDirectory still runs when it eventually completes, and the next run's IsValidRepository/partial-clone cleanup already handles leftovers), letting cancellation semantics — 'stops scheduling and marks unprocessed repos Canceled' — hold within milliseconds regardless of transport state.
[P2] Secondary rate limit (403) misclassified as missing read:org scope — org list silently truncated to the personal account
gclo.Engine/GitHubOrganizationLister.cs:34The degraded-mode filter
catch (ForbiddenException ex) when (ex is not RateLimitExceededException)is too wide. Verified against the shipped Octokit 14.0.0 assembly: SecondaryRateLimitExceededException, AbuseException, and LoginAttemptsExceededException all derive from ForbiddenException, so all of them match this catch and return just [userLogin]. Failure scenario: bursty API use (e.g., the SyncAll flow listing several accounts back-to-back) trips GitHub's secondary rate limit on /user/orgs -> instead of the explicit rate-limit error the code produces for the primary limit at line 47, every org silently vanishes from the account picker and the user is shown only their personal login as a sync target. A transient throttle is converted into what looks like a permanent scope problem, with no error anywhere.Proposed fix: Narrow the filter:
when (ex is not RateLimitExceededException and not SecondaryRateLimitExceededException and not AbuseException)— and translate the secondary-rate-limit case into the same actionable InvalidOperationException used at line 47 (SecondaryRateLimitExceededException carries RetryAfter via AbuseException.RetryAfterSeconds).[P2] gclo-recovery.json write is non-atomic and load throws raw JsonException, permanently failing every subsequent pull
gclo.Engine/LibGit2GitClient.cs:513SaveRecovery uses File.WriteAllText (line 513) — truncate-then-write, not atomic. LoadRecovery (line 518) deserializes with no handling for malformed content; the
?? throwonly covers a literal 'null' document, so truncated/partial JSON throws System.Text.Json.JsonException. Because the recovery file check is the FIRST branch of the pull path (lines 170-179), a corrupt file makes every future FetchAndPullAsync for that repo throw before doing anything. Failure scenario: process killed / power loss during SaveRecovery -> from then on the repo fails every sync with a raw JsonException message ('is an invalid start of a value...') that names nothing actionable; the only remedy is knowing to delete a file the app never mentions, buried under .git. This defeats the exception-surface discipline the rest of the file maintains (typed InvalidRepositoryPathsException, translated UserCancelledException).Proposed fix: Write atomically: serialize to
<file>.tmpthen File.Move(tmp, file, overwrite: true) (atomic on NTFS same-volume). In LoadRecovery, catch JsonException and throw InvalidOperationException naming the file path and remediation ('delete it and re-run recovery'), or treat a corrupt file as absent-with-pending-marker so the normal recovery flow re-offers itself.[P2] Recovery materialization never deletes files removed or renamed upstream — recovery-managed working trees accumulate stale files
gclo.Engine/LibGit2GitClient.cs:341ApplyRecoveryCore is the checkout substitute for recovery-managed repos, but its write loop (lines 341-359) only creates/overwrites the current tip's effective entries; there is no deletion pass. A real checkout removes files that left the tree — this path never does. Failure scenario: repo is recovery-managed; upstream deletes src/old-module/ (or renames a directory, changing every effective path under it); the next pull advances HEAD (line 176) and writes the new layout, but the old files remain on disk indefinitely. Developers build/grep against sources that no longer exist upstream; for a directory rename the tree contains BOTH layouts. Tests (PathRecoveryTests.cs) cover renames/skips/merges but never upstream deletion, so the gap is unguarded. In a tool whose contract is mirroring, the working tree silently diverging from the tip it claims to represent is a correctness failure of the feature's core promise.
Proposed fix: Persist the previously materialized effective-path list in RecoveryDocument (it is already being rewritten every apply). At the start of ApplyRecoveryCore, diff the previous manifest against the new effective set and delete files (and newly empty directories) that dropped out, before writing the new entries.
[P3] Recovery-managed repos rewrite their entire working tree on every sync, even when already up to date
gclo.Engine/LibGit2GitClient.cs:171In FetchAndPull,
if (File.Exists(recoveryFile))(line 171) runs before — and instead of — the up-to-date short-circuit at line 208, and unconditionally calls ApplyRecoveryCore, which re-validates the full tree, rewrites EVERY blob with FileMode.Create (line 357), and re-saves the JSON, regardless of whether the tip moved. Failure scenario: an org contains one recovery-managed 3 GB repo; a scheduled/hourly sync rewrites 3 GB of disk per run with zero remote changes, bumps every file's mtime (invalidating incremental build caches, indexers, and backup dedup for the whole tree), and holds one of the MaxConcurrency slots for minutes — every single sync, forever, since a recovery-managed repo stays recovery-managed. Nothing in the design requires this: the no-local-work invariant means an unchanged tip needs no re-materialization.Proposed fix: Record the materialized tip SHA in RecoveryDocument when ApplyRecoveryCore succeeds. In the pull path, after AdvanceHeadToTrackedTip, skip ApplyRecoveryCore when repo.Head.Tip.Sha equals the recorded SHA (and no new incoming recovery is being applied).
[P3] Clone's checkout phase is not cancellable — the only checkout call missing the OnCheckoutNotify cancellation hook
gclo.Engine/LibGit2GitClient.cs:110Every other working-tree operation wires cancellation through the checkout notify callback: the ff-merge (lines 231-232), MaterializeUnbornHead (lines 287-288), and CompletePendingCheckout (lines 468-469) all set CheckoutNotifyFlags.Updated + OnCheckoutNotify returning !ct.IsCancellationRequested. The fresh-clone checkout at lines 110-113 sets neither, so once the fetch completes the token is never observed until every file is written. Failure scenario: user cancels while an 8-repo batch of large monorepos is in the checkout phase (transfer shows 100%); each clone runs its full multi-minute checkout to completion before the body observes cancellation — the engine then reports those repos Done, not Canceled, and 'Cancel' appears to do nothing. The surrounding catch at line 126 already maps UserCancelledException to OperationCanceledException, so the plumbing exists; only the hook is missing.
Proposed fix: Add
CheckoutNotifyFlags = CheckoutNotifyFlags.Updated, OnCheckoutNotify = (_, _) => !ct.IsCancellationRequestedto the CheckoutOptions at line 110 (the existing existedBefore/TryDeleteDirectory logic already cleans up the aborted clone correctly).[P3] Repository folder name is never validated against the engine's own Windows path rules
gclo.Engine/OrgSyncEngine.cs:97Path.Combine(request.TargetRoot, repo.Name) trusts the repo name as a folder name. GitHub permits repository names that are reserved Windows device names (repos literally named 'aux', 'con', 'nul' exist and are a well-known Windows clone failure class), yet WindowsPathValidator — which explicitly classifies those names as invalid (WindowsPathValidator.cs:18-23) — is only ever applied to tree CONTENTS, never to the repo name itself. Failure scenario: org contains a repo named 'aux' -> Directory/Repository creation fails with an opaque OS/libgit2 error string reported as a plain Failed status, bypassing the structured InvalidRepositoryPathsException -> recovery UX the app built precisely for this class of problem. Secondary defense-in-depth gap: the public SyncAsync(request, repositories, ...) overload feeds caller-supplied RepoDescriptor.Name straight into Path.Combine — a name containing '', '/', or a rooted path escapes TargetRoot with no validation.
Proposed fix: At the top of the per-repo body (or when building
repos), run WindowsPathValidator.ValidatePaths([repo.Name]) and additionally reject names that are not a single path segment (contain separators, '..', or are rooted); fail that repo with InvalidRepositoryPathsException so it flows into the existing structured-failure and recovery reporting.[P3] Stateless credentials handler turns a revoked/expired token into 'too many redirects or authentication replays' on every repo
gclo.Engine/LibGit2GitClient.cs:526MakeCredentialsProvider returns the same credentials on every invocation. On a 401, libgit2 re-invokes the credentials callback to allow retry; supplying the identical bad PAT loops until libgit2 aborts with LibGit2SharpException 'too many redirects or authentication replays'. The listers translate API 401s into a clear message (GitHubRepositoryLister.cs:75-79), but the primary UI flow calls the SyncAsync overload with pre-listed descriptors (WorkspaceViewModel.cs:569), so a token revoked/expired between listing and syncing (or between scheduled runs) never passes through that translation. Failure scenario: PAT expires -> user syncs -> all 300 repos fail in parallel, each with the cryptic replay message, burning several extra authentication round trips per repo; nothing tells the user to fix the token.
Proposed fix: Track invocation count in the handler closure; on the second call for the same operation, throw an InvalidOperationException('GitHub rejected the token...') — LibGit2Sharp marshals callback exceptions out of the native call — so each repo fails fast with the same actionable message the listers produce.
XAML / WinUI
[P1] Workspace layout is broken at the enforced minimum window size
gclo/WorkspacePage.xaml:467MainWindow.xaml.cs:33-34 enforces a 700x520 minimum 'at which the workspace toolbar ... and repo table remain usable', but the arithmetic does not hold. With the NavigationView pane open (PaneDisplayMode="Left", OpenPaneLength=240, MainWindow.xaml:50-53 — this mode never auto-collapses), content width at 700px is 460px; minus the page's Padding=20 (WorkspacePage.xaml:12) and the hero card's Padding=12 (line 271) that leaves ~396px. The table header/row grids reserve 400px of fixed columns (40+160+120+80, lines 468-474 and 590-596), so the 2* Name column collapses to zero width — repo names disappear entirely — and the Archived column clips. The toolbar row (lines 286-392) needs ~600px (SelectorBar with 4 items + selection summary + Options/Refresh/Sync with MinWidth=120), so the accent Sync button — the primary action — is pushed past the window edge. There is no VisualStateManager/AdaptiveTrigger anywhere in the app, so nothing reflows.
Proposed fix: Either raise MinWindowWidth to what the layout actually needs (~900 logical px with the pane open), or make the layout adaptive: PaneDisplayMode="Auto" so the pane collapses at narrow widths, AdaptiveTrigger-driven visual states that drop the Branch/Archived columns and collapse the selection summary, and let the Sync/Refresh group wrap or move into a CommandBar overflow.
[P2] Mica-only backdrop with a Windows 10 minimum target and no fallback background
gclo/MainWindow.xaml:13The window unconditionally declares (MainWindow.xaml:12-14), but gclo.csproj:5 sets TargetPlatformMinVersion=10.0.17763.0 — Windows 10 1809, where Mica does not exist (it requires Windows 11 / MicaController.IsSupported()). When the backdrop is unsupported, Window.SystemBackdrop is silently ignored and the root Grid declares no Background, so on every supported Windows 10 machine the app renders on the default un-themed window surface. The entire visual system is built for Mica layering — LayerFillColorDefaultBrush cards (WorkspacePage.xaml:29, 145, 272) and ControlFillColorDefaultBrush chips are semi-transparent brushes designed to sit on a Mica base — so the shipped 'card on layer' hierarchy degrades on the OS floor the app claims to support.
Proposed fix: Implement the documented fallback chain in MainWindow's constructor: if MicaController.IsSupported() use MicaBackdrop, else if DesktopAcrylicController.IsSupported() use DesktopAcrylicBackdrop, else set the root Grid's Background to {ThemeResource ApplicationPageBackgroundThemeBrush}. Alternatively raise TargetPlatformMinVersion to a Windows 11 build and state Win11-only support.
[P2] Status colors and badges are pinned to the startup theme; title bar never follows the in-app theme
gclo/StatusFormat.cs:47StatusFormat.BrushFor and MainWindow.CreateDotBadge resolve SystemFillColor*Brush via Application.Current.Resources, whose ThemeDictionaries selection follows Application.RequestedTheme — which is fixed at startup (App.xaml.cs:52-60) and cannot change afterward. Changing the theme in Settings only sets root.RequestedTheme (MainWindow.xaml.cs:130), so all ThemeResource references flip but every function-bound status foreground/icon keeps returning the old theme's brush. Concrete failure: start with OS light, switch Settings to Dark — rows show light-theme #0F7B0F/#C42B1C on near-black cards (unreadable) until restart. The comment at App.xaml.cs:48-51 claiming 'status brushes adopt them as row statuses update' is factually wrong: re-invoking BrushFor still resolves against the pinned app theme. The same staleness hits an OS light/dark flip while running in System mode. Additionally, nothing themes the standard title bar (no ExtendsContentIntoTitleBar, no AppWindowTitleBar.PreferredTheme), so a forced Dark app on a light OS shows a white caption bar over dark Mica content.
Proposed fix: Resolve status brushes against the element theme instead of app resources: pass the row's ActualTheme into BrushFor (x:Bind can pass a FrameworkElement or the page can expose ActualTheme and re-evaluate on ActualThemeChanged), or replace the function binding with ThemeResource-based per-status styles selected via a template. Set AppWindow.TitleBar.PreferredTheme (WinAppSDK 1.6+) in ApplySettings alongside root.RequestedTheme.
[P2] Active-repo strip clips chips at out-of-box defaults
gclo/WorkspacePage.xaml:400The ActiveStrip ItemsControl caps at MaxHeight=64 with a VariableSizedWrapGrid of ItemHeight=30/ItemWidth=208 and no ScrollViewer. At the default 1000px window (MainWindow.xaml.cs:100) the wrap width is ~696px = 3 chips per row; the default parallelism is 8 (AppSettings.cs:16), so 8 active chips need 3 rows (90px) and the third row is clipped to a 4px sliver — two syncing repos are invisible with everything at defaults. Parallelism is user-configurable to 64 (WorkspacePage.xaml:330), where the vast majority of active chips silently vanish. This contradicts the strip's stated purpose ('exactly the repos cloning/pulling right now', line 394).
Proposed fix: Make the strip honest about overflow: size MaxHeight to whole rows (60/90), and either wrap the ItemsControl in a vertical ScrollViewer or cap visible chips and append a '+N more' summary chip bound to ActiveRepos.Count.
[P2] Connect form duplicated wholesale in four places, with drift already shipped
gclo/WorkspacePage.xaml:200The token/org/target/subfolder/preview form exists as two ~60-line verbatim copies (connect card, lines 41-104; Edit flyout, lines 200-259), plus partial copies in the Options flyout (lines 326-345) and AccountWizardDialog step 4 (lines 110-135). Drift is already observable: the card's controls carry AutomationIds (ConnectTokenBox, ConnectOrgBox, ConnectTargetBox, ConnectBrowseButton) while the flyout copies have none, so the FlaUI suite (which locates everything ByAutomationId, gclo.UiTests/SmokeTests.cs) cannot target the edit path at all — a whole user flow is untestable because a copy diverged. Any future fix (placeholder text, token help URL, validation affordance) must be applied 3-4 times.
Proposed fix: Extract a ConnectionFormControl UserControl exposing the WorkspaceViewModel (or a narrow interface) plus an AutomationId prefix, and reuse it in the card, the flyout, and the wizard's destination step; give the flyout instance its own AutomationIds in the process.
[P3] Sort-header buttons and table column widths are copy-pasted with no shared style/resource
gclo/WorkspacePage.xaml:483Four ~20-line sort-header buttons (lines 483-562) are identical except for the column name — each repeats HorizontalAlignment/Background="Transparent"/BorderThickness/Padding and the same StackPanel+FontIcon body with three per-column function bindings. Separately, the column widths 40|2*|160|120|80 are declared twice, in the header grid (468-474) and the row DataTemplate (590-596), with only a comment ('mirrored by the row template below') holding them in sync — WinUI has no SharedSizeGroup, so editing one silently misaligns header from cells.
Proposed fix: Define one 'ColumnHeaderButtonStyle' resource for the chrome, and declare the widths once as GridLength resources referenced by both grids (or generate the header from the same width source in code). This turns a 5-place edit into a 1-place edit for any column change.
[P3] Account wizard dialog resizes and re-centers on every step change
gclo/AccountWizardDialog.xaml:21The four step panels overlay in a Grid with only MinWidth=400 and no height constraint; collapsed panels are skipped at measure, so the dialog sizes to the visible step alone. Step 2 (token + hint + validation row + error, ~260px) is far taller than step 3 (caption + one ComboBox, ~110px), so pressing Next shrinks the dialog and ContentDialog re-centers it — the Next/Back buttons jump vertically under the pointer mid-flow, and the error TextBlocks (lines 35-41, 82-88) appearing later cause a second resize while the user is reading.
Proposed fix: Give the step host Grid a MinHeight sized to the tallest step (e.g. MinHeight="280") so the dialog footprint is stable across steps and validation-error appearance.
[P3] About dialog attribution list is seven duplicated TextBlock patterns
gclo/AboutDialog.xaml:49Lines 49-69 repeat the identical 'CaptionTextBlockStyle + SemiBold Run + license Run' block seven times, one per component. Adding or updating a dependency (which Dependabot already does in this repo) means hand-editing markup and hoping the pattern is copied exactly; the natural representation is data.
Proposed fix: Replace with an ItemsControl bound to a static IReadOnlyList of (Name, License) records and one DataTemplate; new attributions become a one-line data change and the visual treatment stays uniform by construction.
Test quality
[P1] gclo.Cli has zero test coverage across ~1,100 lines of contract-bearing logic
gclo.Cli/SyncCommand.cs:78No test project references gclo.Cli, its types are all internal with no InternalsVisibleTo, and CI has no gate on it — yet it carries the app's scriptable contracts: (1) the documented exit-code mapping 0/1/2 (Program.cs:59-83, SyncCommand.cs:250); (2) the --json output schema scripts parse (CliJson.cs, SyncCommand.cs:231-237); (3) OptionReader's '--name value'/'--name=value' parsing including the rule that a following '--' token counts as a missing value (CommandLine.cs:68-84); (4) TokenOptions.Resolve's mutual-exclusion and env/file/stdin fallback ladder (TokenOptions.cs:52-69); (5) --account seeding precedence via ??= and the CreateOrgSubfolder target composition (SyncCommand.cs:148-155); (6) the --sanitize-paths pipeline: BuildRecovery's skip-wins/last-segment-substitution composition (SyncCommand.cs:365-385), SyncProgressHandler's collect-then-drain state machine (SyncCommand.cs:428-476), and the post-hoc summary rewrite 'Failed - sanitized' (SyncCommand.cs:216). A regression in any of these — e.g. BuildRecovery materializing a path that also had a suggestion-less problem, or a renamed JSON property — ships silently; the engine tests cannot catch it because all of this logic lives above IGitClient/IRepositoryLister. This is the single largest untested blast radius in the repo.
Proposed fix: Add gclo.Cli.Tests (or extend the existing test project): add InternalsVisibleTo, table-test OptionReader/TokenOptions/BuildRecovery/SyncProgressHandler directly, and pin the --json schema with a serialization round-trip. For end-to-end exit codes, extract SyncCommand's inline 'new LibGit2GitClient()/new GitHubRepositoryLister()' (SyncCommand.cs:174-175) behind an injectable factory so RunAsync can run against the existing fakes, then assert exit codes for success/partial-failure/cancel/fatal.
[P2] LibGit2GitClient's cancellation machinery is untested, and the one cancellation test is vacuous
gclo.Engine.Tests/LibGit2GitClientTests.cs:85Clone_TokenAlreadyCanceled_ThrowsOperationCanceledAndCreatesNothing passes a pre-canceled token to Task.Factory.StartNew(..., cancellationToken, ...) (LibGit2GitClient.cs:30-33), so the TPL transitions the task to Canceled without ever invoking Clone(): the OperationCanceledException comes from the scheduler and 'creates nothing' is vacuously true. The code that actually implements the documented 'Ctrl+C cancels gracefully' contract has zero coverage: OnTransferProgress returning false on cancellation (LibGit2GitClient.cs:75, 150), the UserCancelledException-to-OperationCanceledException translation (126-129, 158-161, 235-238, 291-294, 472-475), and partial-directory cleanup when a clone is canceled mid-transfer (121-124). Failure scenario: a LibGit2Sharp upgrade changes the exception surfaced when a transfer callback aborts — Ctrl+C mid-clone then reports repos as Failed (raw LibGit2SharpException) instead of Canceled, OrgSyncEngine counts them into Failed, the CLI exits 1 with wrong failure lines, and a half-cloned directory survives to be mistaken for a valid repo on the next run. No FetchAndPull cancellation test exists at all.
Proposed fix: Replace the vacuous test with one that exercises Clone(): use a forged fixture with enough objects to guarantee transfer callbacks (or cancel from within the onProgress callback the client forwards) and assert OperationCanceledException plus target-directory cleanup. Add the mirror test for FetchAndPullAsync canceled between fetch and merge. If local-transport callbacks prove unreliable, extract the catch/translate/cleanup block into an internal method and unit-test it by throwing UserCancelledException with a canceled vs. non-canceled token.
[P2] GitHub listers (157 lines of pagination, fallback, and error-translation logic) are untestable and untested
gclo.Engine/GitHubRepositoryLister.cs:16GitHubRepositoryLister and GitHubOrganizationLister construct GitHubClient inline, so nothing covers: manual pagination termination at batch.Count < 100 (GitHubRepositoryLister.cs:26-37), the three-way org-404 fallback (own account with private repos vs. other user vs. double-404 error, lines 48-73) keyed on a case-insensitive login comparison, the 401/rate-limit translations users actually see (75-84), the boundary-duplicate DistinctBy (91), and the read:org 403 degraded mode in GitHubOrganizationLister.cs:34-41 (including its 'ForbiddenException that is not RateLimitExceededException' filter). Failure scenario: Octokit changes GetAllForOrg's not-found exception subtype or the ForbiddenException hierarchy on a dependency bump — the fallback ladder silently stops matching, every user-account sync starts failing with a raw Octokit message, and CI stays green because the only tests of 'lister errors' script InvalidOperationException through fakes. This is the one seam between the well-tested engine and the well-tested CLI/VMs that nothing exercises.
Proposed fix: Inject the client: both classes can take a Func<string, IGitHubClient> (Octokit ships the interface) defaulting to the current construction. Then unit-test the pagination loop (exactly-100-item page, multi-page), each fallback branch of the 404 ladder, the login-case-differs path, and every exception translation with a mocked IGitHubClient.
[P2] CI coverage gate measures only the gclo.Engine package — ViewModels and Cli regressions are invisible to it
.github/workflows/ci.yml:58The gate filters the cobertura report to the package named 'gclo.Engine' and enforces 60% on that alone. gclo.ViewModels — the largest tested surface (871-line WorkspaceViewModel, AccountsStore, wizard, coordinator, ~2,300 lines of tests) — is present in the same coverage report but never checked, and gclo.Cli is neither tested nor gated. Failure scenario: a PR that deletes WorkspaceViewModelTests.cs wholesale, or adds hundreds of untested ViewModel lines, passes CI with no signal; the substantial investment in the ViewModels suite has no enforcement backstop. The 60% engine threshold is also far below what the suite actually achieves, so it will not catch meaningful engine coverage erosion either.
Proposed fix: Extend the gate to also parse the gclo.ViewModels package node from the same coverage.cobertura.xml and enforce a threshold reflecting current reality (measure, then set slightly below, e.g. 80%+). Ratchet the engine threshold up from 60% for the same reason, and add gclo.Cli to both test run and gate once finding #1 is addressed.
[P3] Debounce test depends on a 120 ms wall-clock window and will flake on loaded CI runners
gclo.Engine.Tests/WorkspaceViewModelTests.cs:1058TokenChange_RapidEdits_OnlyNewestLookupLands sets vm.Token twice on consecutive lines and requires the second assignment to land inside the 120 ms Task.Delay debounce window (WorkspaceViewModel.cs:325). A GC pause, thread-pool stall, or noisy-neighbor CI scheduler gap of more than 120 ms between lines 1064 and 1065 lets the 'AAAA' lookup fire first: _orgs.Calls becomes 2 and/or Organizations[0] is 'AAAA', failing Assert.Equal(1, _orgs.Calls) and Assert.Equal("BBBB", ...) intermittently. Every other async test in this suite is gate-based and deterministic; this is the one remaining wall-clock race, and intermittent failures here will erode trust in an otherwise flake-free suite.
Proposed fix: Make the debounce injectable as time rather than duration: have WorkspaceViewModel take a TimeProvider (Task.Delay(debounce, timeProvider, ct)) and drive the test with Microsoft.Extensions.TimeProvider.Testing's FakeTimeProvider — set token A, set token B, advance 120 ms once, assert exactly one lookup with B. Alternatively gate the lookup start (TaskCompletionSource in the FakeOrganizationLister) so the test controls interleaving instead of racing the clock.
[P3] UI tests prove 'nothing happened' with fixed 300 ms sleeps — regressions slower than the sleep pass silently
gclo.UiTests/SmokeTests.cs:115Two tests use negative assertions synchronized only by Thread.Sleep(PostInvokeSettle=300ms): AddAccountWizard_NextWithEmptyName (line 114-117) clicks Next, sleeps 300 ms, then asserts the step-1 name box is still present — but WaitForElement succeeds if the box exists at any instant, so if a validation regression advances the dialog after ~350 ms (dialog deferrals on a slow hosted runner routinely exceed 300 ms), the test passes against broken code. AddAccountWhileAboutDialogOpen (line 155-159) has the same shape: Assert.Null(WizardNameBox) after 300 ms passes wrongly if the colliding dialog opens late — which is precisely the crash (0xc000027b) this regression test exists to guard. Compounding this, ci.yml (line 81-84) documents the ui-tests job as not a required check, so even hard failures here do not block merges.
Proposed fix: Anchor each negative assertion to a positive oracle instead of elapsed time: for (6), wait for the step-1 validation error text to appear (deterministic signal that the Next handler ran and rejected), then assert the step title is unchanged; for (8), wait for a signal that the nav command was processed (e.g. About dialog still focused after invoking, or an app-side test hook/log line), then assert no wizard. Promote ui-tests to a required check once stable — the job has existed long enough to have a track record.
[P3] WorkspaceViewModel tests never dispose the view model and clean up only on the success path; Dispose itself has no direct test
gclo.Engine.Tests/WorkspaceViewModelTests.cs:227Roughly twenty tests end with 'Directory.Delete(vm.TargetFolder, recursive: true)' as the last statement rather than in a finally/IDisposable — any assertion failure strands the temp directory, and unlike OrgSyncEngineTests/PathRecoveryTests (which clean via Dispose + TryDeleteDirectory) this class has no backstop, so red runs accumulate junk under %TEMP%\gclo-tests on dev machines and CI. The same class also never calls vm.Dispose() (SyncAllCoordinatorTests.cs:21-28 does), which means WorkspaceViewModel.Dispose (WorkspaceViewModel.cs:295-302) — a real lifecycle API that cancels the in-flight org lookup and the lifetime CTS — has no direct coverage at all: no test asserts that disposing mid-lookup cancels cleanly, is idempotent, or that a lookup completing after Dispose cannot mutate state. That is exactly the kind of teardown path that regresses unnoticed until the app throws on window close.
Proposed fix: Make the test class IDisposable: track created view models and target folders in fields, dispose/TryDeleteDirectory them in Dispose (mirroring SyncAllCoordinatorTests), and delete the inline success-path Directory.Delete calls. Add two Dispose tests: dispose while a gated org lookup is in flight (assert the lookup's token is canceled and no state mutates when the gate releases) and double-dispose is a no-op.
🤖 Generated with Claude Code
Note: two items surfaced by this review's finder pass were hand-verified and already fixed on dev — the
SyncRequest.ToString()PAT leak and the dev-channel update-discovery bug (both beta.12). The engine/XAML/test-quality items here still need a verification pass. The viewmodels and comments review dimensions never ran (agent budget) and should be re-run when convenient.Verification pass done on 2026-10-09: every finding was re-checked against the current code before acting. All 25 are addressed on
dev; this issue closes with the nextdev → mainPR.IGitHubGateway.GetAccountKindAsync; an invisible organization is aGitHubAccessException(Forbidden)with grant/SSO guidance, an unknown login gets a spelling hint, only real users take the user-repos pathWaitAsync(token)InvalidOperationExceptionnaming the fileRepositoryPathResolver(containment); this wave adds the Windows rules: a repo named like a device fails its own row with a plain reason, before any disk or git accessTitleBar.PreferredThemefollowsConnectionFormControlwith an AutomationId prefix replaces the two full copies; the wizard's destination step is a different view model and staysColumnHeaderButtonStylealready existed; widths are nowGridLengthresources used by header and rowsMinHeight="280"IGitHubGatewayseam)ObjectDisposedExceptionThe
viewmodelsandcommentsdimensions the original review never ran: the view-model layer was reworked and re-tested under #30/#40/#43 this week (every public member has tests, 100% line coverage). A dedicated comment-accuracy pass was not part of this wave.