Full app review (unverified, 2/2): security, CLI, release-engineering findings #32
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: KofTwentyTwo/gclo#32
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.Security
[P1] Release pipeline ships unsigned binaries; update trust rests entirely on one GitHub account
.github/workflows/release.yml:105The
vpk packstep (release.yml:105) has no--signParams/--azureTrustedSignFile, and no signtool step exists anywhere in the pipeline, so Setup.exe, the portable package, delta packages, and gclo-cli-win-x64.zip are all unsigned Authenticode-wise. Velopack verifies downloaded packages only against the SHA in the releases.{channel}.json feed, which lives in the same GitHub release — so the entire chain of trust for silent auto-updates (UpdateService.DownloadAndApplyAsync applies and restarts with no further verification, gclo/Services/UpdateService.cs:101-114) is TLS plus the integrity of the KofTwentyTwo GitHub account. A compromised maintainer account or leaked PAT with repo write can publish a release that every installed user applies as arbitrary code. Users also eat SmartScreen/Defender friction on every install, and the winget submission (release.yml:157-164) publishes that unsigned installer org-wide.Proposed fix: Sign all shipped binaries: add Azure Trusted Signing (or an OV cert) via
vpk pack --signParams/--azureTrustedSignFileso both the app EXEs and the Velopack Setup are Authenticode-signed; sign the CLI exe before zipping. Additionally emit build provenance with actions/attest-build-provenance so releases are attributable to the workflow, not just the account.[P2] Release integrity depends on unpinned, unverified tooling executed with secrets in scope
.github/workflows/release.yml:89dotnet tool install -g vpk(release.yml:89) installs whatever the latest vpk on nuget.org is at run time — vpk is the tool that constructs the installer and update packages users receive, so a compromised or malicious vpk release becomes a compromised gclo release with no diff in this repo. Likewise release.yml:161 doesInvoke-WebRequest https://aka.ms/wingetcreate/latest -OutFile wingetcreate.exeand immediately executes it with WINGET_TOKEN (and the rest of the job env) available — an unpinned binary fetched through a redirector, with no hash verification. Both run in the job whose env carries NUGET_API_KEY and WINGET_TOKEN (see separate finding).Proposed fix: Pin versions and verify:
dotnet tool install -g vpk --version <x.y.z>(let Dependabot-style PRs bump it deliberately), and download a specific wingetcreate release asset by version URL followed by aGet-FileHashcheck against a committed SHA-256 before executing it.[P2] NUGET_API_KEY and WINGET_TOKEN are exposed to every step of the release job
.github/workflows/release.yml:28release.yml:25-29 sets NUGET_API_KEY and WINGET_TOKEN as job-level env, so they sit in the environment of all 15 steps — including
dotnet build/dotnet test(which execute NuGet-restored analyzers, MSBuild tasks, and the test suite), the unpinnedvpktool, and the downloadedwingetcreate.exe. Any code execution inside a transitive build/test dependency can read$env:NUGET_API_KEYand publish malicious gclo.Engine packages to nuget.org, even though only the final two steps legitimately need either secret. This directly contradicts the least-exposure pattern GitHub documents for secrets.Proposed fix: Delete the job-level env block; pass
NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}only on the 'Push gclo.Engine to nuget.org' step andWINGET_TOKENonly on the winget step (guarding theif:withsecrets.* != ''directly or a cheap pre-step output). Consider a GitHub 'release' environment with required reviewers for these secrets.[P2] All GitHub Actions are pinned to mutable tags, not commit SHAs
.github/workflows/ci.yml:27Every workflow references actions by major tag: actions/checkout@v7, actions/setup-dotnet@v5, actions/upload-artifact@v7 (ci.yml:27,31,75), github/codeql-action@v4 (codeql.yml:44,53), actions/dependency-review-action@v5 (dependency-review.yml:28), and the same in release.yml:33,36 — the workflow that holds contents:write plus NUGET_API_KEY/WINGET_TOKEN. Tags are repointable: the tj-actions/changed-files compromise (March 2025) exfiltrated CI secrets exactly this way. Dependabot's github-actions ecosystem is already configured (dependabot.yml:24-35), so SHA pins would be maintained automatically at zero ongoing cost — the repo is one
pinpass away from closing this.Proposed fix: Pin each
uses:to a full 40-character commit SHA with the version as a trailing comment (e.g.actions/checkout@<sha> # v7.x.y); Dependabot updates SHA pins natively. Prioritize release.yml, then the workflows with security-events/pull-requests write.[P2] Dev-channel (beta) installs can never discover updates, including security fixes
gclo/Services/UpdateService.cs:127UpdateService hardcodes
new GithubSource(RepoUrl, null, false)— prerelease=false — so CheckForUpdatesAsync only enumerates non-prerelease GitHub releases. But release.yml routes everyv*-beta.*tag to a PRERELEASE release carrying the 'dev' Velopack channel assets (release.yml:55, 101-102, 116-117). A user who installed a beta Setup.exe is on the dev channel; their update check looks for releases.dev.json among stable releases only, finds nothing, and MainWindow reports 'You are up to date' (MainWindow.xaml.cs:711-716) forever. Given SECURITY.md's policy that only the latest release receives security fixes with no backports, stranded beta users silently never receive patches.Proposed fix: Derive the prerelease flag from the installed channel/version instead of hardcoding false — e.g.
bool pre = manager.CurrentVersion?.IsPrerelease ?? false;(or read the channel via VelopackLocator) and pass it to GithubSource. Add a UI test or unit seam asserting a dev-channel install queries prereleases.[P3] SyncRequest's generated ToString() prints the PAT — in a type shipped publicly on NuGet
gclo.Engine/SyncRequest.cs:8public sealed record SyncRequest(string Organization, string Token, ...)gets a compiler-generated ToString()/PrintMembers that emits the raw token, e.g.SyncRequest { Organization = acme, Token = ghp_xxxx... }. Nothing in this repo calls it today, but gclo.Engine is packed and pushed to nuget.org (release.yml:141-146), so any consumer (or future gclo code) that logs the request, string-interpolates it into an exception, or hits it with a debugger/serializer leaks the credential — precisely the class of leak SECURITY.md:42 declares a vulnerability. Secrets held in types with value-printing semantics are a known foot-gun; the mitigation is one line.Proposed fix: Override ToString() (and implement PrintMembers) on SyncRequest to redact Token (
Token = ***); consider the same review for any future record carrying a secret, or introduce a smallSecretStringwrapper whose ToString() is constant.[P3] Editing an account re-materializes the vault token into the UI when a sentinel would do
gclo/MainWindow.xaml.cs:396MainWindow.xaml.cs:396 fetches the plaintext token from Credential Manager every time the edit wizard opens and seeds it into the PasswordBox (AccountWizardDialog.xaml.cs:48-51); WorkspacePage's edit flyout does the same on every open (WorkspacePage.xaml.cs:161-163). The only purpose is the 'unchanged token' detection in AccountWizardViewModel.TokenChanged (AccountWizardViewModel.cs:228-229). This copies the secret into additional long-lived managed strings and into an editable control — where interacting with the box can expose it via the PasswordBox reveal affordance during screen shares or an unlocked desktop — for a flow that never needs to display it. The workspace VM legitimately needs the token to sync; the edit dialogs do not.
Proposed fix: Seed the edit UIs with a constant sentinel (e.g. leave the box empty with placeholder 'unchanged — enter to replace') and treat 'user typed nothing' as 'leave the vault alone'; only call ITokenVault.TryRetrieve on paths that actually transmit the token (sync/org lookup).
[P3] SECURITY.md's token-lifecycle contract is stale — it denies the on-disk persistence accounts mode performs
SECURITY.md:38SECURITY.md:38 states 'The PAT is kept in memory only… never written to disk, settings, or logs', and line 42 invites reports if 'a token can end up on disk'. Since accounts mode shipped, CredentialManagerVault.Store deliberately persists tokens to disk (DPAPI-protected, via CredWriteW with CRED_PERSIST_LOCAL_MACHINE, CredentialManagerVault.cs:33-66), readable by any process running as the same user. The published security policy is the document a reporter and a triager both rely on; as written it would classify the product's own vault feature as a vulnerability, and it fails to disclose the real trust boundary (same-user processes can read stored tokens via CredRead).
Proposed fix: Update the 'How gclo handles your GitHub token' section: ad-hoc workspace tokens are memory-only; saved accounts persist the token in Windows Credential Manager (per-user, DPAPI, non-roaming), never in accounts.json/settings.json/logs; state the same-user-process trust boundary explicitly.
[P3] Updates are only checked when the user opens Help → Check for updates
gclo/MainWindow.xaml.cs:692The sole call sites of UpdateService.CheckAsync are inside CheckForUpdatesMenuItem_Click (MainWindow.xaml.cs:692-740); there is no startup or periodic check. Combined with the support policy that only the latest release receives security fixes (SECURITY.md:5-10) and no backports, a user who never opens the Help menu runs vulnerable builds indefinitely with a credential-holding app — the auto-update machinery (Velopack) is already installed and paying its complexity cost without delivering its main security benefit.
Proposed fix: On startup (and, say, every 24h), run CheckAsync in the background and surface a non-modal InfoBar ('gclo vX.Y is available — Update and restart') that reuses the existing DownloadAndApplyAsync confirmation flow; keep it silent on failure as UpdateService already guarantees.
CLI
[P1] --json contract silently loses path-sanitization outcomes; 'Sanitized' note leaks to stderr in JSON mode
gclo.Cli/CliJson.cs:9SyncJsonResult (CliJson.cs:9-16) has no representation of sanitization: sanitized repos are folded into 'cloned' (SyncCommand.cs:216) with no per-repo record, and skipped paths (case-only collisions that are NOT materialized on disk) are invisible to machine consumers — an automation pipeline mirroring an org cannot detect that a 'cloned' repo is an incomplete working tree. Worse, the only place the information exists in --json mode is a human-formatted stderr line: TrySanitizeAsync writes 'repo Sanitized N renamed, N skipped: ...' unconditionally (SyncCommand.cs:337-339), violating the documented '--json: Print no progress lines; end with one line of JSON' contract (SyncCommand.cs:46-48). Similarly, invalid-path failures in JSON carry only prose: failures[].error embeds just the first 3 offending paths (InvalidRepositoryPathsException.cs:22-25), while human mode gets 10 structured 'path (reason)' lines on stderr (SyncCommand.cs:461-474) — JSON consumers get strictly less information than humans, on the one feature (path recovery) that most needs scripted follow-up.
Proposed fix: Extend SyncJsonResult with a 'sanitized' array ([{"repo","renamed":N,"skipped":N,"skippedPaths":[...]}]) and give SyncFailure an optional 'invalidPaths' array ([{"path","reason","suggestedName"}]) sourced from RepoProgress.InvalidPaths. Suppress (or keep but document) the stderr Sanitized line when --json is active. Update the --json schema shown in help text and docs/CLI.md.
[P2] No repo enumeration, filtering, or dry-run: the CLI is all-or-nothing while the GUI's core workflow is list-select-sync
gclo.Cli/SyncCommand.cs:78The GUI lists repositories first (WorkspaceViewModel.LoadReposAsync, WorkspaceViewModel.cs:463), shows archived/branch metadata, lets the user select a subset, retry only failures (line 630), and resolve one repo's paths without re-running anything (line 657). The CLI can do none of this: there is no 'gclo repos' listing command, no --include/--exclude/--skip-archived filter, and no --dry-run — a nightly mirror job cannot exclude archived repos (RepoDescriptor.IsArchived already exists, RepoDescriptor.cs:8) and recovering one repo's invalid paths requires re-running sync over the entire org with --sanitize-paths (docs/CLI.md:106-108). The engine already exposes exactly the needed seam: OrgSyncEngine.SyncAsync(request, IReadOnlyList repositories, ...) (OrgSyncEngine.cs:49) is public and is what the GUI calls — the CLI just never uses it.
Proposed fix: Add 'gclo repos --org [--json]' (name, defaultBranch, archived) and sync filters '--include ', '--exclude ', '--skip-archived'; implement by listing descriptors, filtering, and calling the existing subset SyncAsync overload. '--include one-repo --sanitize-paths' then also gives targeted path recovery, closing the GUI parity gap.
[P2] The CLI's documented scripting contract (parsing, exit codes, JSON schema) has zero test coverage
gclo.Engine.Tests/gclo.Engine.Tests.csproj:1No test project references gclo.Cli (gclo.Engine.Tests.csproj has no Cli reference; gclo.UiTests is FlaUI against the GUI). docs/CLI.md promises a stable contract — exit codes 0/1/2 (docs/CLI.md:234-243), a specific --json shape (docs/CLI.md:120), token-source precedence and mutual exclusion (TokenOptions.cs:52-69), '--name=value' parsing with the '--'-prefixed-value rejection rule (CommandLine.cs:68-84), and account-override semantics (SyncCommand.cs:148-165) — yet none of it is executable-verified. OptionReader, TokenOptions, ProgressPrinter, and the SyncCommand account/target seeding are pure logic, trivially unit-testable, and exactly the code a future refactor will silently break; a renamed record property would change the published camelCase JSON schema with no failing test. For a repo advertising 201 unit tests, the entire scripting surface being untested is the largest quality asymmetry.
Proposed fix: Add a gclo.Cli.Tests project (or InternalsVisibleTo from the existing test project): unit tests for OptionReader edge cases, TokenOptions precedence/mutual-exclusion/file-trimming, ProgressPrinter dedup and failure capture, account seeding/override logic, and golden-string tests serializing SyncJsonResult/AccountSummary through CliJsonContext to pin the schema.
[P2] Exit code 2 conflates caller bugs with transient runtime failures; exit 1 conflates partial failure with Ctrl+C
gclo.Cli/Program.cs:63Program.cs:63-83 maps usage errors (unknown flag, missing --org), rejected tokens, GitHub rate limiting, and unknown orgs all to exit 2 — the listers fold rate-limit and auth failures into the same InvalidOperationException path (SyncCommand.cs:191-195, OrgsCommand.cs:77-82). A retry-with-backoff wrapper (the natural pattern for a nightly org mirror hitting secondary rate limits) cannot distinguish 'my invocation is wrong, never retry' from 'transient, retry later' without parsing stderr prose, which is not a stable contract. Likewise exit 1 means both 'completed but N repos failed' and 'user pressed Ctrl+C' (SyncCommand.cs:250, help text lines 66-70); in --quiet cron use these are different alerting outcomes and only --json's wasCanceled disambiguates. Also undocumented: with --json, fatal (exit 2) runs emit nothing on stdout, so 'gclo ... --json | jq' fails on empty input unless the script checks the code first.
Proposed fix: Split the taxonomy while it is still pre-1.0: keep 2 for usage, add 3 for auth/permission (never retry), 4 for transient (rate limit, network), and consider 130 or a dedicated code for cancellation. Document in help/docs/CLI.md that exit-2 --json runs produce no stdout document.
[P3] No machine-readable progress stream: --json emits a single document only at the end of a potentially very long run
gclo.Cli/SyncCommand.cs:236For the app's stated purpose — large organizations, hundreds of repos — a sync can run for many minutes to hours. In --json mode stdout is completely silent until the final one-line document (SyncCommand.cs:236); the only progress signal is the human 'name status' lines, whose format is informal (double-space separated, free-text error suffix) and explicitly not a documented contract. CI wrappers, dashboards, or the GUI-parity 'live table' experience cannot be built on the CLI without scraping human output. The plumbing already exists: ProgressPrinter receives every RepoProgress transition and already dedups repeats (ProgressPrinter.cs:42-66).
Proposed fix: Add '--json-lines' (NDJSON): one {"repo","status","error"?} object per status transition on stdout, terminated by the existing summary object (distinguishable by a "type" or by shape). Serialize via a new entry in CliJsonContext to stay reflection-free.
[P3] Account lifecycle is GUI-only: the CLI can consume accounts but never create, edit, or delete them
gclo.Cli/AccountsCommand.cs:22Accounts are the CLI's best scripting feature ('gclo sync --account work' with the token in Credential Manager), yet they can only be created in the WinUI desktop app (AccountsCommand.cs help text lines 20-23, docs/CLI.md:139-141 'a CLI flag for creating them is planned'). On a headless Windows build/mirror server — the exact place a scheduled 'gclo sync --account' belongs — the MSIX-packaged GUI is typically not installable, so accounts are unusable precisely where they add the most value. The recovery message for a missing vault entry (SyncCommand.cs:277-281) even directs users back to the desktop app. Additionally 'gclo accounts --json' omits fields scripts need to reason about a run: id (needed to correlate with the 'gclo:account:' credential), createOrgSubfolder, maxConcurrency, and the recorded last-run summary line (CliJson.cs:23-27 exposes only name/organization/targetRoot/lastSync).
Proposed fix: Add 'gclo accounts add --name --org --target [--parallel N] [--org-subfolder] --token-stdin' (plus edit/remove), reusing AccountsStore and CredentialManagerVault which the CLI already references; token entry via the existing no-echo stdin path. Extend AccountSummary with id, createOrgSubfolder, maxConcurrency, and lastSyncSummary.
[P3] --parallel is unbounded in the CLI while the GUI clamps concurrency to 1..64
gclo.Cli/SyncCommand.cs:113SyncCommand.cs:110-119 validates only 'parsed < 1'. The GUI and settings layer treat 64 as the hard ceiling (AppSettings.cs:14-16 MinConcurrency/MaxConcurrency, clamped on load/save at AppSettings.cs:86), reflecting a deliberate product decision about GitHub abuse/secondary-rate-limit behavior — but 'gclo sync --parallel 100000' bypasses it and will open thousands of concurrent clone connections, tripping GitHub's secondary rate limits and burning the very token the tool went to lengths to protect. The two heads of the same engine disagree on an operational safety limit.
Proposed fix: Clamp (or reject with a clear message) values above AppSettings.MaxConcurrency in the --parallel parse branch, and state the 1..64 range in the sync help text and docs/CLI.md.
[P3] 'gclo help sync' silently prints root help, ignoring the command argument
gclo.Cli/Program.cs:103Program.cs:103-105 handles 'help' as a bare alias for --help and discards everything after it, so 'gclo help sync' — the first thing many users type, and a form the root help's own closing line invites confusion with ('Run gclo --help') — prints the root usage instead of sync's detailed help, with no error. The user gets a plausible-looking but wrong answer rather than either the right help page or a complaint.
Proposed fix: In the 'help' case, if rest is non-empty dispatch to the named command with ['--help'] (reusing the existing switch), and error with exit 2 on an unknown command name.
[P3] Unexpected exceptions print a bare message with no type, stack, or activity-log pointer, and share exit code 2 with usage errors
gclo.Cli/Program.cs:79The top-level catch-all (Program.cs:79-83) prints only ex.Message. An internal bug — say a NullReferenceException — yields exactly 'Object reference not set to an instance of an object.' on stderr and exit 2, indistinguishable from a typo'd flag and useless in a bug report. The full exception (with stack) does reach the daily activity log for failures inside SyncCommand/OrgsCommand (FileActivityLog.cs:56-58), but the console never tells the user that log exists or where it is (FileActivityLog.CurrentLogFilePath is available), and exceptions thrown outside those try blocks (command dispatch, log construction) are never logged at all.
Proposed fix: In the catch-all, distinguish expected Cli*Exceptions from everything else: for unexpected types print 'Unexpected error (): ' plus 'Details: ' (writing the exception there from Main), and consider a distinct exit code (e.g. 70, EX_SOFTWARE) so scripts can separate tool bugs from caller mistakes.
Release engineering / repo
[P1] Dev-channel self-update is broken for every install shipped to date
gclo/Services/UpdateService.cs:127UpdateService creates
new GithubSource(RepoUrl, null, false)— the third argument is Velopack'sprereleaseflag, and its GithubSource filters with.Where(x => includePrereleases || !x.Prerelease), excluding GitHub prereleases entirely. Dev-channel releases are published as prereleases (release.yml:116-117 passes --pre), so a dev-channel install can never see a release containing releases.dev.json: CheckForUpdatesAsync returns null and Help > Check for updates reports 'up to date' forever. This is live: all 9 published releases (v0.1.0-beta.2 through beta.10) are prereleases on the dev channel, so 100% of current installs have dead self-update — directly contradicting README.md:22 and docs/RELEASING.md:30-33 ('Users who installed from a dev release get every prerelease you tag').Proposed fix: Pass prerelease: true. Channel isolation is unaffected: Velopack selects the release by locating the releases.{channel}.json asset for the installed channel, and stable releases carry only releases.stable.json, so stable installs still never pick up prereleases. Add a UI-test or unit seam asserting the source is constructed with prerelease enabled, and verify with a beta.11 tag that an installed beta.10 sees it.
[P1] CONTRIBUTING's CI-gate claims are false for the branch PRs actually target: dev has no required checks and no PR rule
CONTRIBUTING.md:62CONTRIBUTING.md:15-18 directs all PRs to dev and CONTRIBUTING.md:62 states 'Every pull request must pass' Build/Format/Dependency review/CodeQL. The live 'protect-dev' ruleset (id 18480003) contains only deletion and non_fast_forward rules — no required status checks and no pull_request rule — so a contributor PR to dev can be merged red, and direct pushes to dev are allowed (and are the norm: CI history shows repeated
push devevents). The four checks only bind at the dev→main promotion (ruleset 18480002). Additionally, main's pull_request rule has required_approving_review_count: 0 and require_code_owner_review: false, so CONTRIBUTING.md:16's 'only moves via reviewed PRs' is not enforced either. For a repo whose quality story is its gates, the documentation and the enforcement diverge exactly where contributions land.Proposed fix: Add a required_status_checks rule to protect-dev requiring 'Build and test (x64)', 'Format (style gate)', and 'Dependency review' (CodeQL only runs against main, see separate finding), plus a pull_request rule if PR-only flow is intended for outside contributors. Alternatively, reword CONTRIBUTING to say checks are advisory on dev and enforced at main promotion — but enforcing is the 10/10 answer for a repo accepting forks.
[P2] Documented bad-release recovery (delete the tag) is blocked by the active tag ruleset
docs/RELEASING.md:164RELEASING.md:154-165 says the only fix for a broken published release is to delete the GitHub Release and the tag (
git push origin :refs/tags/v1.2.3), and the earlier draft-failure path (line 130) also assumes tag reuse after cleanup. But the live 'protect-release-tags' ruleset (id 18484691) applies deletion, update, and non_fast_forward rules to refs/tags/v* with bypass_actors: [] — rulesets bind admins too, so the documented command fails for everyone, including the owner, at exactly the moment a broken release needs pulling. The runbook has never been exercised against the actual configuration.Proposed fix: Either add a Repository Admin bypass actor to protect-release-tags (keeps day-to-day immutability, permits deliberate recovery), or update RELEASING.md to document the real procedure: temporarily disable/edit the ruleset in Settings → Rules before deleting the tag, then re-enable. Test the runbook once against a throwaway tag.
[P2] Release job exposes NUGET_API_KEY and WINGET_TOKEN to every step, including unpinned binaries fetched at runtime
.github/workflows/release.yml:28NUGET_API_KEY and WINGET_TOKEN are set as job-level env (release.yml:25-29), so they are present in the environment of all ~15 steps — including
dotnet tool install -g vpkwith no --version pin (line 89) andInvoke-WebRequest https://aka.ms/wingetcreate/latest -OutFile wingetcreate.exefollowed by execution (lines 161-164), an unpinned binary downloaded over a redirecting vanity URL at release time. A compromised or merely broken new vpk/wingetcreate release runs with publish credentials for nuget.org and a GitHub PAT in its environment, inside the workflow that also holds contents:write. The unpinned vpk also makes releases non-reproducible: a vpk behavior/naming change lands silently on the next tag push (asset names like gclo-stable-Setup.exe are load-bearing for the winget step at line 163).Proposed fix: Move NUGET_API_KEY to step-level env on the nuget push step only, and WINGET_TOKEN to the winget step only (use
if: ...against secrets via a small preceding check step or vars). Pin the tool:dotnet tool install -g vpk --version <x.y.z>(let Dependabot-style PRs bump it deliberately), and download wingetcreate from a versioned GitHub release URL, ideally with a checksum check.[P2] CodeQL never scans dev — findings batch up and block the release train at the dev→main promotion PR
.github/workflows/codeql.yml:11codeql.yml triggers on push to [main] and PRs targeting [main] only, while all development happens on dev and contributor PRs target dev (CONTRIBUTING.md:15). Result: code merges into dev for days/weeks with zero static analysis, and the first CodeQL run over accumulated changes happens on the dev→main PR — where 'CodeQL' is a required status check on main (ruleset 18480002). Any finding then blocks the promotion/release PR with a pile of unrelated commits to bisect, the worst possible place to discover it. It also makes CONTRIBUTING.md:69's claim that CodeQL gates PRs false for the normal PR flow.
Proposed fix: Change triggers to
push: branches: [main, dev]andpull_request: branches: [main, dev](the 120-minute Windows build is the cost; the weekly scheduled run already accepts it). Findings then surface on the PR that introduced them.[P2] Nothing enforces that a release tag points at a main commit — a stable tag on a dev commit publishes to users and winget
.github/workflows/release.yml:44The release pipeline derives everything from the tag name (release.yml:40-59) and runs on any v* tag push. RELEASING.md:92-98 instructs tagging from main, but this is convention only: the protect-release-tags ruleset restricts deletion/update, not creation, so a v1.0.0 accidentally created on a dev commit (easy from an IDE tag dialog or a mistyped
git push --tagswhile on dev) builds, publishes an immutable stable GitHub Release, updates every installed stable-channel app, and submits a winget manifest — from code that never passed the main promotion gate. With immutable releases, recovery requires burning the version number (RELEASING.md:154-160). All 9 releases so far happen to be on main, but that is discipline, not enforcement.Proposed fix: Add a guard step after checkout:
git fetch origin mainthengit merge-base --is-ancestor $env:GITHUB_SHA origin/main || exit 1(for stable tags at minimum; ideally all v* tags). Five lines that make the runbook's central assumption enforced.[P3] UI-test job has no failure diagnostics, undermining its own plan to earn required-check status
.github/workflows/ci.yml:103ci.yml:82-84 says the FlaUI job is 'not in the branch ruleset's required checks yet ... so this job earns required status by proving itself stable first.' But when it fails on a hosted runner there is nothing to diagnose with: gclo.UiTests captures no screenshots (no Capture/Screenshot usage anywhere in gclo.UiTests/), the
dotnet teststep produces no TRX logger, and the job uploads no artifacts (unlike build-test, which uploads coverage at ci.yml:73-79). Hosted-runner UIA failures are notoriously environmental (session resolution, timing, first-frame rendering); without a screenshot at failure time each flake is unattributable, so the 'prove itself stable' promotion decision can never be made with evidence.Proposed fix: Add
--logger "trx;LogFileName=uitests.trx" --results-directory TestResultsto the test step and anif: failure()(or always) upload-artifact step for TestResults plus %LOCALAPPDATA%\gclo\logs. In AppSession/test dispose, captureCapture.Screen().ToFile(...)on test failure into the results directory.[P3] README download instructions name an asset (gclo-stable-Setup.exe) that exists in no release, and the GUI app README has zero screenshots
README.md:18README.md:18 tells users to 'download and run gclo-stable-Setup.exe', but all 9 published releases are dev-channel prereleases whose assets are gclo-dev-Setup.exe / gclo-dev-Portable.zip — a user following the README today finds no such file (and GitHub's releases page shows the latest prerelease). Meanwhile the five screenshot slots (README.md:11-12, 39-41, 46, 54, 62) are HTML comments, and issue #10 tracking them is labeled 'deferred' — for a desktop GUI app, a README with no picture of the product is the single biggest conversion gap. The three badges (README.md:5-7) are static shields; there is no live CI status badge despite CI being a headline feature of the repo's quality story.
Proposed fix: Until the first stable tag ships: add one line to Download & install noting current builds are beta ('dev' channel) and the installer is gclo-dev-Setup.exe. Land at least the hero screenshot (un-defer issue #10 — it is 15 minutes with the app running). Add
[](...)next to the static badges.[P3] gclo.Engine NuGet package will ship without deterministic-build metadata or symbols
gclo.Engine/gclo.Engine.csproj:3The release pipeline packs gclo.Engine for nuget.org (release.yml:141-146), but the pack is missing the standard publish-quality trio: (1) no ContinuousIntegrationBuild=true in CI, so SourceLink embeds non-normalized local paths and the package fails nuget.org's 'deterministic' health indicator despite Directory.Build.props setting Deterministic; (2) no IncludeSymbols/SymbolPackageFormat=snupkg and the push step only globs *.nupkg, so consumers get no debugger step-through into the engine; (3) the csproj sets license/readme/repo URL but no PackageProjectUrl or PackageTags, weakening the listing. For a package whose whole pitch is 'reuse the engine', these are the visible quality markers every NuGet health checker (and staff reviewer) looks at first.
Proposed fix: In gclo.Engine.csproj add
<PublishRepositoryUrl>true</PublishRepositoryUrl>,<IncludeSymbols>true</IncludeSymbols>,<SymbolPackageFormat>snupkg</SymbolPackageFormat>, PackageProjectUrl and PackageTags; pass-p:ContinuousIntegrationBuild=trueon the pack step (or set it when CI env var is present); change the push glob to include *.snupkg (dotnet nuget push pushes the snupkg alongside automatically when named consistently).[P3] No NuGet restore caching across four Windows CI jobs
.github/workflows/ci.yml:30All three ci.yml jobs (build-test, ui-tests, format) plus the CodeQL job run on windows-latest and each performs a cold
dotnet restoreof the full dependency graph (Windows App SDK, LibGit2Sharp native binaries, Octokit, FlaUI, test SDK) on every run — there is no setup-dotnetcache: true, no actions/cache on the NuGet packages folder, and no packages.lock.json anywhere in the repo to key one on. The repo saw ~30 CI runs on 2026-07-04 alone; at 4 cold restores per push on 2x-billed Windows minutes this is the largest recurring waste in the pipeline, and it adds a network-flakiness surface (transient nuget.org failures fail otherwise-green runs).Proposed fix: Set
<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>in Directory.Build.props (commit the lock files), then addcache: trueandcache-dependency-path: '**/packages.lock.json'to every actions/setup-dotnet step. Lock files also pin the restore graph, which tightens the NuGetAudit gate's reproducibility.🤖 Generated with Claude Code
Note: the security finding about the PAT in
SyncRequest.ToString()is fixed (beta.12), as is the dev-channel update-discovery bug. Remaining security/CLI/release-engineering items still need a verification pass before action. (CLI is now at 100% test coverage as of beta.13, which touches several of the CLI items here.)Verification pass done on 2026-10-09: every finding was re-checked against the current code before acting. All 28 are addressed on
devexcept the one that needs a signing identity (split out as #57); this issue closes with the nextdev → mainPR.gh attestation verify <file> -R KofTwentyTwo/gclo) and SECURITY.md says plainly that binaries are not yet signedvpkinstalled at a pinned version;wingetcreate.exefetched from a pinned release asset and verified against a committed SHA-256 before it runsNUGET_API_KEY/WINGET_TOKENare step-level env on the two publishing steps onlyuses:pinned to a commit SHA with the version as a comment; Dependabot maintains the pins--jsonloses sanitization; stderr leak in JSON modesanitized[]andfailures[].invalidPathsin the document; the stderr note suppressed in JSON modesgclo repos,--include/--exclude/--skip-archived,--dry-run; sync lists first and uses the engine's subset overloadGitHubAccessException.Kinddrives it; documented that fatal exits print no JSON--json-lines(NDJSON progress + summary)gclo accounts add/edit/remove;accounts --jsonadds id, description, createOrgSubfolder, maxConcurrency, lastSyncSummary--parallelunboundedgclo help syncprints root helpdevand enforced atmain; enforcing them ondevwould block the maintainer's direct pushes and is left as the owner's call