feat: keep GitHub tokens in the OS secret store [minor]

[matt-edmondson]

Fixes #411. Completes the three-repo cluster with ktsu-dev/OAICLI#42 and ktsu-dev/BuildMonitor#278.

The exposure

ProjectDirectorOptions carried a PAT per configured owner plus an account-level one, in plain fields with no [JsonIgnore]. AppData<T> serializes the whole object, so every debounced save wrote those tokens to an unencrypted settings file alongside window state and divider positions.

They now go to the platform-native secret store through ktsu.CredentialCache, under a service name scoped to ktsu.ProjectDirector.

Design

Personas are derived, not stored — a versioned namespace plus the login or the owner name, hashed to a stable GUID. The /v1/ is there because changing the derivation would orphan every token already in the store, so a future change has to be deliberate.

GitHubOwners becomes a set of names. It was doing double duty: the registry of configured owners and the map to their tokens. Splitting those is what lets the settings file keep tracking which owners are configured while holding nothing credential-shaped. This is the public-shape change the issue anticipated.

Migration, not re-entry. The issue's caveat assumed users would re-enter their PAT because there is nothing in the OS store to migrate from — but there is something to migrate to, and the plaintext copy needs clearing regardless. So startup moves both the account token and every owner token, registers the owner names, and empties the old fields. Order is load-bearing: written to the store first, so a store that throws cannot lose the token; if the write is refused the old copy is deliberately left alone, because clearing it would destroy the only copy the user has.

One thing the issue did not cover

There was no way to enter a token at all. Owners were seeded with GitHubToken.Create<GitHubToken>(string.Empty) and nothing ever populated them — the only way to set a PAT was editing the settings JSON by hand. Moving storage without adding an input path would have left new setups with no way whatsoever, so this adds File > Set GitHub Owner Token next to the existing Add New GitHub Owner, following the deferred-open pattern the codebase already uses (a popup cannot be opened from inside BeginMenu).

I could not exercise the ImGui layer here — this container is headless, and per CLAUDE.md the ImGui layer is not unit-tested in this repo either. The storage and migration underneath it are fully covered, which is the split CLAUDE.md asks for.

No secret store available. Same decision as BuildMonitor and for the same reason: a desktop app cannot throw out of a token read without taking down the render loop, and these reads happen per owner on every scan. So it reads as "no token", and TokenStorage records the reason for ProjectDirector to drain into its log exactly once. There is no plaintext fallback.

One unrelated fix, which the tests needed

DevDirectory defaulted to the literal C:\dev, which AbsoluteDirectoryPath rejects off Windows — so new ProjectDirectorOptions() threw on Linux and macOS, and no test could construct the type at all. It now picks a valid path per platform, unchanged on Windows. Flagging it rather than burying it: it is not part of the issue, but without it none of the migration tests below can run, and it is latent today only because no existing test touches the options type.

Tests

ProjectDirector.Test/TokenStorageTests.cs, 14 new tests on top of the existing 38. Proven to fail without the fix, by three mutations:

Mutation	Result
Migration stops emptying the plaintext fields	4 failures, incl. MigrationEmptiesThePlaintextFields, SerializedOptionsCarryNoToken
Migration empties them even when the store refused the write	1 failure: MigrationKeepsTheLegacyTokenWhenTheStoreRefuses
The account token is persisted to the settings file again	1 failure: SerializedOptionsCarryNoToken

SerializedOptionsCarryNoToken builds its serializer the way AppDataStorage does, with RoundTripStringJsonConverterFactory registered — without it a semantic string serializes as a char array and a substring assertion for the token silently never matches.

All 52 pass, and the solution builds clean with no warnings.

Adjacent, not touched

ProjectDirectorOptions.OpenAIToken is the same shape of secret in the same file, but its only consumer is a commented-out line, so it is dead rather than exposed. Left alone as outside this issue — worth its own decision about whether it should exist at all.

🤖 Generated with Claude Code

https://claude.ai/code/session_018AnVpPWKjVtXnAvd4bnzUL

---

Generated by Claude Code

[matt-edmondson]

CI status: github-advanced-security is not this PR's failure

The "Code scanning AI findings" agent failed before analysing anything, on a billing quota:

_t [SessionModelError]: You have exceeded your monthly quota
  errorType: 'quota',
  statusCode: 402,

That is an account-level Copilot quota, not a finding about this diff. It failed identically, within the same few minutes, on two unrelated PRs in other repositories — ktsu-dev/OAICLI#44 and ktsu-dev/BuildMonitor#285 — which is the reproduction: those three share nothing but the account.

There is no fix to port into this PR: a 402 clears when the quota resets or is raised, and nothing in a diff changes it. I also cannot re-run it to confirm — the dynamic workflow is not retryable, and GitHub answers 403 This workflow run cannot be retried.

Flagging it so the red mark is not mistaken for a security finding against a change that is specifically about moving secrets out of a plaintext file. The .NET Workflow and CodeQL runs are still in flight; I'll keep watching and will act on anything that is actually this PR's.

---

Generated by Claude Code

[matt-edmondson]

Checked this one rather than applying it, and I'm leaving the loop as it is: the rule does not hold here, and the suggested rewrite would introduce a real defect.

ImGui.MenuItem(owner) is not a predicate. It draws the menu item, and its return value reports whether that item was clicked during this frame. This is Dear ImGui's immediate-mode idiom: the call is the rendering, and the bool is the event. The analysis reads it as a filter because it sits in an if at the top of a loop body, but there is nothing being filtered — every owner is drawn on every frame, and at most one of them was clicked.

Moving it into .Where(owner => ImGui.MenuItem(owner)) would make drawing the menu a side effect of evaluating a LINQ predicate. That happens to render correctly today only because foreach enumerates the sequence exactly once, eagerly, to completion. It breaks the moment anything treats that sequence as a sequence:

• .Any() or .First() short-circuits, and the owners after the first click stop being drawn;
• .Count() or a second enumeration draws every item twice in one frame, which Dear ImGui reads as duplicate IDs;
• evaluating the enumerable in a debugger watch window draws menu items as a side effect of inspecting it.

Each of those is a frame-dependent UI bug, and none would be caught by a test. Trading that for an explicit .Where(...) is not worth it when the if already says exactly what is happening.

On the second half of the suggestion — adding using System.Linq; — that is already unnecessary: this file uses LINQ a few lines above, in OwnersInDisplayOrder(...), and System.Linq is an implicit global using under this SDK.

Happy to revisit if the intent is a broader convention about immediate-mode loops, but as a change to this loop it makes the code less correct rather than more.

---

Generated by Claude Code

[matt-edmondson]

SonarCloud new-code coverage: what I did, and the one thing I can't do from here

The quality gate failed on new-code coverage, and I've taken it as far as it honestly goes. 62.6% → 67.6% → ~73% across two pushes. It will not reach 80%, and I don't think it should be made to.

What was actually uncovered

TokenStorage and ProjectDirectorOptions were already fully covered. Every uncovered new line is in ProjectDirector.cs, which has no tests at all — and CLAUDE.md says why:

Tests live in ProjectDirector.Test… the ImGui layer is not unit-tested. That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display.

So I followed that rule rather than arguing with the gate. Five decisions were still tangled up with drawing, and each is now a plain method with a test:

Extracted	Why it was worth pulling out
ResolveGitHubCredentials	The owner-token-shadows-account-token rule — what makes a private repo in another org reachable — was an inline ternary with nothing pinning it. Writing the test also improved it: it now returns null rather than building credentials around a blank secret, so a misconfigured owner makes an anonymous request instead of authenticating as nobody.
PrepareTokens	Pins that migration runs before the account token is read. Reversed, a user upgrading from a settings-file token starts that session unauthenticated and only picks it up next launch. Verified: swapping the two lines fails StartupMigratesBeforeResolvingCredentials.
ApplyOwnerToken	The popup closes whether or not the secret store took the token, so a silent refusal would look exactly like success.
OwnersInDisplayOrder	The owner registry became a set in this branch, and a set promises no order — the owner panels and the token menu could otherwise disagree with each other and between runs. That was a bug this branch introduced.
DescribeTokenMigration	The startup log line.

12 tests added for those; 61 pass.

What's left, and why I stopped

48 uncovered lines, all of them drawing and wiring: declaring popup fields, ImGui.BeginMenu/MenuItem blocks, opening a popup from Tick, assigning to GitHubClient.Credentials, and the foreach that calls SyncGitHubOwnerInfo. There is no rule left in any of them, and reaching them needs a window and a GL context. Extracting further would mean inventing seams around ImGui.MenuItem purely to move a number, which makes the code worse.

This PR is also unusually exposed to that gate: it had to add UI. There was previously no way to enter a token at all short of hand-editing the settings JSON, so moving storage without adding an input path would have left new setups with no way whatsoever.

The mechanism already exists — it just isn't set for this repo

The shared .NET Workflow anticipated precisely this, and says so:

A file that cannot be executed rather than one nobody has got round to testing — a windowed entry point, say — is excluded per repository through the SONAR_COVERAGE_EXCLUSIONS_EXTRA variable, so this workflow stays identical everywhere instead of accumulating one repository's paths for all the others to carry.

ProjectDirector.cs is that windowed entry point. The intended remedy is the repository variable SONAR_COVERAGE_EXCLUSIONS_EXTRA, set to something like **/ProjectDirector.cs.

That is a repository setting, not a code change, so I can't do it from a PR — and I wouldn't want to anyway: it changes the coverage bar for every future PR in this repo, which is a maintainer's call, not mine. So this needs a decision:

1. Set SONAR_COVERAGE_EXCLUSIONS_EXTRA for this repo. My recommendation — it is the mechanism the org designed for exactly this file, and it makes the gate measure what the repo actually tests.
2. Accept the gate on this PR and leave the variable alone.

I've deliberately not tried to route around it. Everything else on the PR is green except github-advanced-security, which is the account-level Copilot quota 402 covered above.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate failed

Failed conditions
69.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud