Close #11, #38: merge history + remove plaintext CredentialService#41
Merged
Conversation
Closes #38. Adds MergeHistoryService persisting up to 50 recent merge runs in AppData (timestamp, directory, filename, diff style, batch name if any, exit code). The bound evicts the oldest on overflow. Direct `ktsu merge` invocations record every run (success and failure both, as the issue defaults to); batch-dispatched runs only record on success since a failed batch usually signals a stale config rather than a useful "what path did I try" entry. `ktsu merge-history` renders most-recent first; `ktsu merge-history --clear` truncates idempotently.
…alCache Closes #11 (acceptance bullet 2: "removed in favor of using ktsu.CredentialCache directly from feature services"). The previous CredentialService persisted secrets as a Dictionary<string,string> through AppDataStorage — readable by anyone with FS access. No feature service consumed ICredentialService, so the abstraction was unused dead weight covering a security footgun. This commit: - Deletes ICredentialService, CredentialService, and CredentialStore. - Removes the DI registration. - Adds ktsu.CredentialCache 1.2.3 to Directory.Packages.props and pulls it into KtsuTools.Core so feature code can use CredentialCache.Instance directly when needed. Migration path for users with an existing plaintext store: delete and re-prompt. The old %AppData%\ktsu\KtsuTools\Core\CredentialStore.json file (if present) is no longer read; users will have to re-enter PATs when the first feature that needs them lands. Note: the package's default IPersistenceProvider<string> still writes JSON to AppData (encryption is the persistence-provider author's responsibility). When a feature actually needs credentials, the call site can plug in a DPAPI/Keychain/libsecret-backed provider via CredentialCache.ConfigurePersistenceProvider before first use.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two issues + two audit comments posted on existing issues.
Summary
ktsu merge-history+--clear. NewMergeHistoryServicerecords every directktsu mergeinvocation (success and failure both — default per the issue's open question) and successful batch dispatches (failed batches usually signal a stale saved config rather than a useful entry). Bounded to 50 entries with oldest-first eviction.merge-history --clearis idempotent. Stored alongside batches in AppData.ICredentialServicehad zero callers and persisted secrets as aDictionary<string,string>through AppData. Took option 2 of the issue's acceptance ("removed in favor of using ktsu.CredentialCache directly from feature services"): deletedICredentialService/CredentialService/CredentialStore, addedktsu.CredentialCache 1.2.3so the package is on hand when a feature actually needs it. Migration: delete the oldCredentialStore.jsonand re-prompt.Caveats
ktsu.CredentialCache's default persistence provider still writes JSON to AppData — encryption is the provider author's responsibility. When a feature first needs credentials, the call site should plug in a DPAPI/Keychain/libsecret-backedIPersistenceProvider<string>viaCredentialCache.ConfigurePersistenceProvider. I did not write that provider in this PR.ktsu.AppDataStorage↔ktsu.Semantics.Pathsversion pin plus missing host-runtime package on this distro). Relying on the windows-latest CI build to verify.Test plan
MergeHistoryServiceTests— 4 tests (round trip, MaxEntries cap, idempotent clear, failed-run inclusion)ktools merge ./repos *.ymlfollowed byktools merge-historyshows the run withExit = 0ktools merge --batch nonexistentfollowed byktools merge-historydoes not show the failed batchktools merge-history --clearthenktools merge-historyshows "No merge runs recorded yet."Closes #38, #11.
Generated by Claude Code