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]

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

[matt-edmondson]

Merged main in to clear the conflict. Three files clashed, and one of them needed a real decision rather than a pick-a-side.

ProjectDirector.cs — the scan loop. This is the one worth reading. While this PR was open, main fixed a credential-leak defect: the owner loop now calls ApplyCredentials, which assigns client.Credentials for every owner, including one with no token of its own, so the previous owner's identity cannot carry into the next scan. This branch had rewritten the same lines to read the owner's token from the secret store via ResolveGitHubCredentials, which returns null when nothing is usable and so only assigned conditionally — exactly the shape of the defect main had just fixed.

Taking either side alone regresses something, so the merge takes both: main's unconditional ApplyCredentials, fed from this branch's TokenStorage.ReadOwnerToken(owner) instead of the plaintext pat field (which no longer exists, since GitHubOwners is now a set of names). There's a comment at the call site recording why the assignment is unconditional, so it doesn't get "simplified" back later.

ResolveGitHubCredentials is kept — it is still used for the account-level resolution and is covered by TokenStorageTests.

ProjectDirectorOptions.cs — DefaultDevDirectory. Both sides made the same fix independently: the PR description flagged the C:\dev default as an unrelated fix the tests needed, and main has since landed its own. Same behavior, different comment wording and line breaks. Took main's version, so the PR no longer carries a duplicate of a fix that is already upstream.

Directory.Packages.props. Took main's newer versions (ktsu.Extensions 1.6.16, the four ktsu.ImGui.* at 3.38.8, ktsu.RunCommand 1.5.24, ktsu.Semantics.* 5.4.3) and kept this branch's new ktsu.CredentialCache 1.3.49 entry, which main has no opinion about.

Verified on the merge result: dotnet build ProjectDirector.sln -c Release clean with 0 warnings, 0 errors, and the suite at 102 passed / 3 skipped / 0 failed (105 total). The three skips are environmental and not from this change — one reads a mode-000 directory anyway because the container runs as root, and two need git-lfs, which isn't installed here. main's ScanCredentialTests and this branch's TokenStorageTests are green together, which is the pair that matters for the resolution above.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud

[matt-edmondson]

The coverage gate is the same blocker as before, not a regression from the merge

SonarCloud went red again on the merge commit — 70.6% new-code coverage, required ≥ 80%. This is the blocker already analysed above, not a new one, and the decision it asks for is unchanged. Adding this only because that comment cites ~73% and the reading is now 70.6%, which would otherwise look like the merge cost coverage.

It didn't. What the merge contributed to ProjectDirector.cs is one executable line:

ApplyCredentials(GitHubClient, owner, TokenStorage.ReadOwnerToken(owner), Options.GitHubLogin, Options.GitHubToken);

It sits in ScanRemoteAccountsForRepos, which was already uncovered, and it calls ApplyCredentials — which main brought in with ScanCredentialTests covering it. The rest of the resolution touched Directory.Packages.props (not code) and took main's already-covered DefaultDevDirectory.

The bulk of the movement is main arriving: ProjectDirector.cs gained 412 lines in this merge (MakeLoadedOptionsSafe, FindDiff, the unsupported-repo handling), alongside roughly 996 lines of main's own new tests. The new-code window this PR is measured over is simply much larger than it was yesterday.

Still nothing to push

The uncovered remainder is what it was: drawing and wiring — popup fields, BeginMenu/MenuItem blocks, opening a popup from Tick — reachable only with a window and a GL context, in the layer CLAUDE.md states is not unit-tested. The five decisions worth extracting were extracted in the earlier round, and ApplyCredentials is itself an example of the pattern working: the rule lives in a plain tested method, and the loop that calls it is the untestable part.

Manufacturing further seams around ImGui.MenuItem purely to move the percentage would widen the PR and make the code worse, so I haven't.

The decision is still yours

Unchanged from the earlier comment — set SONAR_COVERAGE_EXCLUSIONS_EXTRA to something like **/ProjectDirector.cs for this repo (the mechanism the shared workflow documents for exactly a windowed entry point), or accept the gate on this PR. It is a repository setting, so it can't come from a PR, and it changes the bar for every future PR here.

Everything else on the current head is green: tests pass on ubuntu, windows and macOS, CodeQL and github-advanced-security both succeeded, and the merge conflict is cleared. This gate is the only thing outstanding.

---

Generated by Claude Code