From f2f8d6c81f5df6a954c7ed11bcfc9850cfcf141b Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 11 Jul 2026 15:31:05 +0200 Subject: [PATCH] refactor(credentials): extract pure credential-mutation planning from initcmd (477.4) Move plan states/entries, transition classification, staged-key validation, per-store write grouping, the all-profiles active-ref scan, and stale-key calculation into internal/credentials (planning.go). The 'id|source|backend' string map key is replaced by the comparable credentials.StoreIdentity struct via ResolvedSecretsStore.Identity(). Planner and multi-store/shared-ref tests relocated first and pass against the moved logic; initcmd keeps both application paths, preflight, bundle writes, config-save sequencing, key deletion, and rendering. Zero tests deleted (427 -> 432); stale-key policy, overwrite semantics, and error wording unchanged. --- .../init_reviewer_credential_status.go | 10 +- internal/cmd/initcmd/initcmd.go | 393 ++---------------- internal/cmd/initcmd/initcmd_test.go | 225 +--------- internal/credentials/planning.go | 356 ++++++++++++++++ internal/credentials/planning_test.go | 368 ++++++++++++++++ 5 files changed, 768 insertions(+), 584 deletions(-) create mode 100644 internal/credentials/planning.go create mode 100644 internal/credentials/planning_test.go diff --git a/internal/cmd/initcmd/init_reviewer_credential_status.go b/internal/cmd/initcmd/init_reviewer_credential_status.go index 5a14a37..1e41e23 100644 --- a/internal/cmd/initcmd/init_reviewer_credential_status.go +++ b/internal/cmd/initcmd/init_reviewer_credential_status.go @@ -163,7 +163,7 @@ func buildInteractiveInitReviewerCredentialStatuses(opts *root.Options, deps ini func buildInteractiveInitCredentialStatuses(opts *root.Options, deps initDeps, session initSessionDraft, entries []initCredentialPlanEntry, writes map[string]map[string]string, purpose string) []initReviewerCredentialStatus { statuses := make([]initReviewerCredentialStatus, 0, len(entries)) - stores := map[string]initStore{} + stores := map[credentials.StoreIdentity]initStore{} defer func() { for _, store := range stores { if store != nil { @@ -177,7 +177,7 @@ func buildInteractiveInitCredentialStatuses(opts *root.Options, deps initDeps, s } existing := map[string]bool{} unavailable := "" - storeKey := initCredentialStoreKey(entry.SecretsStore) + storeKey := entry.SecretsStore.Identity() store := stores[storeKey] if store == nil { if !canOpenInitStoreForReviewerStatus(deps, entry) { @@ -228,7 +228,7 @@ func interactiveInitReviewerCredentialPlanEntries(session initSessionDraft, plan if currentProfile, ok := session.cfg.Profiles[session.workspace.profileName]; ok { profile = currentProfile } - entries, err := planInitCredentialsWithConfig(session.cfg, session.workspace.previousProfile, profile, plannedWriteKeys) + entries, err := credentials.PlanWithConfig(session.cfg, session.workspace.previousProfile, profile, plannedWriteKeys) if err != nil || !hasInitReviewerCredentialPlanEntry(entries) { entries = append([]initCredentialPlanEntry(nil), session.workspace.credentialPlan...) } @@ -308,8 +308,8 @@ func appendReviewerCredentialPlanEntry(entries []initCredentialPlanEntry, seen m KeySpecs: append([]credentials.KeySpec(nil), specs...), PlannedWriteKeys: append([]string(nil), plannedWriteKeys[ref.Ref]...), } - entry.MissingRequiredKeys = missingRequiredInitCredentialKeys(entry.KeySpecs, entry.PlannedWriteKeys) - entry.State = classifyInitCredentialPlanEntry(entry) + entry.MissingRequiredKeys = credentials.MissingRequiredPlannedKeys(entry.KeySpecs, entry.PlannedWriteKeys) + entry.State = credentials.ClassifyPlanEntry(entry) entries = append(entries, entry) seen[key] = struct{}{} return entries diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index 0f0ea7b..d463423 100644 --- a/internal/cmd/initcmd/initcmd.go +++ b/internal/cmd/initcmd/initcmd.go @@ -441,28 +441,19 @@ type initStore interface { Close() error } -type initCredentialPlanState string +type initCredentialPlanState = credentials.PlanState const ( - initCredentialPlanStateKeepExisting initCredentialPlanState = "keep_existing" - initCredentialPlanStateDefer initCredentialPlanState = "defer" - // #nosec G101 -- init planner state label, not secret material. - initCredentialPlanStateOverwriteRef initCredentialPlanState = "overwrite_ref" - initCredentialPlanStateWrite initCredentialPlanState = "write" - initCredentialPlanStateClearRef initCredentialPlanState = "clear_ref" - initCredentialPlanStateMissingRequired initCredentialPlanState = "missing_required" - initCreateProfileSentinel = "__create__" + initCredentialPlanStateKeepExisting = credentials.PlanStateKeepExisting + initCredentialPlanStateDefer = credentials.PlanStateDefer + initCredentialPlanStateOverwriteRef = credentials.PlanStateOverwriteRef + initCredentialPlanStateWrite = credentials.PlanStateWrite + initCredentialPlanStateClearRef = credentials.PlanStateClearRef + initCredentialPlanStateMissingRequired = credentials.PlanStateMissingRequired + initCreateProfileSentinel = "__create__" ) -type initCredentialPlanEntry struct { - Ref config.CredentialRef - PreviousRef *config.CredentialRef - SecretsStore credentials.ResolvedSecretsStore - KeySpecs []credentials.KeySpec - PlannedWriteKeys []string - MissingRequiredKeys []string - State initCredentialPlanState -} +type initCredentialPlanEntry = credentials.PlanEntry type initCredentialDecisionKind string @@ -472,7 +463,7 @@ const ( ) type initCredentialDecisionKey struct { - Store string + Store credentials.StoreIdentity Purpose string Mode string Ref string @@ -1326,7 +1317,7 @@ func editInteractiveInitReviewerEntityStep(_ *cobra.Command, opts *root.Options, session = mergeReviewerCredentialDraftWrites(session, draft) if session.workspace != nil { workspace := *session.workspace - credentialPlan, err := planInitCredentialsWithConfig(session.cfg, workspace.previousProfile, session.workspace.profile, projectInitPlannedWriteKeys(session.writes)) + credentialPlan, err := credentials.PlanWithConfig(session.cfg, workspace.previousProfile, session.workspace.profile, projectInitPlannedWriteKeys(session.writes)) if err != nil { return initSessionDraft{}, false, cmderr.Config(err) } @@ -3455,7 +3446,7 @@ func buildNonInteractiveInitPlan(cmd *cobra.Command, opts *root.Options, flags i addWrite(writes, llmRef, llmKey, llmSecret) } plannedWriteKeys := projectInitPlannedWriteKeys(writes) - credentialPlan, err := planInitCredentialsWithConfig(cfg, previousProfile, profile, plannedWriteKeys) + credentialPlan, err := credentials.PlanWithConfig(cfg, previousProfile, profile, plannedWriteKeys) if err != nil { return initPlan{}, cmderr.Config(err) } @@ -3533,7 +3524,7 @@ func buildInteractiveInitWorkspace(cmd *cobra.Command, opts *root.Options, flags return initWorkspaceDraft{}, cmderr.Config(err) } - credentialPlan, err := planInitCredentialsWithConfig(working, previousProfile, profile, nil) + credentialPlan, err := credentials.PlanWithConfig(working, previousProfile, profile, nil) if err != nil { return initWorkspaceDraft{}, cmderr.Config(err) } @@ -3631,7 +3622,7 @@ func buildInteractiveInitSessionPlan(opts *root.Options, session initSessionDraf activeRefs[ref.Ref] = true } previousProfile := touchedInteractiveInitPreviousProfile(session, profileName) - entries, err := planInitCredentialsWithConfig(session.cfg, previousProfile, profile, plannedWriteKeys) + entries, err := credentials.PlanWithConfig(session.cfg, previousProfile, profile, plannedWriteKeys) if err != nil { return initSessionPlan{}, cmderr.Config(err) } @@ -3749,8 +3740,8 @@ func standaloneRepositoryAccessPlanEntries(cfg config.File, plannedWriteKeys map KeySpecs: append([]credentials.KeySpec(nil), specs...), PlannedWriteKeys: append([]string(nil), plannedWriteKeys[ref.Ref]...), } - entry.MissingRequiredKeys = missingRequiredInitCredentialKeys(entry.KeySpecs, entry.PlannedWriteKeys) - entry.State = classifyInitCredentialPlanEntry(entry) + entry.MissingRequiredKeys = credentials.MissingRequiredPlannedKeys(entry.KeySpecs, entry.PlannedWriteKeys) + entry.State = credentials.ClassifyPlanEntry(entry) entries = append(entries, entry) } return entries @@ -3826,8 +3817,8 @@ func standaloneReviewerEntityPlanEntries(cfg config.File, plannedWriteKeys map[s KeySpecs: append([]credentials.KeySpec(nil), specs...), PlannedWriteKeys: append([]string(nil), plannedWriteKeys[ref.Ref]...), } - entry.MissingRequiredKeys = missingRequiredInitCredentialKeys(entry.KeySpecs, entry.PlannedWriteKeys) - entry.State = classifyInitCredentialPlanEntry(entry) + entry.MissingRequiredKeys = credentials.MissingRequiredPlannedKeys(entry.KeySpecs, entry.PlannedWriteKeys) + entry.State = credentials.ClassifyPlanEntry(entry) entries = append(entries, entry) } return entries @@ -5352,13 +5343,9 @@ func sameInitCredentialStore(a, b credentials.ResolvedSecretsStore) bool { return a.Equal(b) } -func initCredentialStoreKey(resolved credentials.ResolvedSecretsStore) string { - return fmt.Sprintf("%s|%s|%s", resolved.ID, resolved.Source, resolved.Backend) -} - func initCredentialDecisionMapKey(entry initCredentialPlanEntry, key string) initCredentialDecisionKey { return initCredentialDecisionKey{ - Store: initCredentialStoreKey(entry.SecretsStore), + Store: entry.SecretsStore.Identity(), Purpose: entry.Ref.Purpose, Mode: entry.Ref.Mode, Ref: entry.Ref.Ref, @@ -5397,177 +5384,7 @@ func initCredentialEntryDeferredByDecision(decisions map[initCredentialDecisionK return true } -type initWriteGroup struct { - Resolved credentials.ResolvedSecretsStore - Writes map[string]map[string]string - OverwriteRefs map[string]bool -} - -type initReviewerCredentialCleanupGroup struct { - Resolved credentials.ResolvedSecretsStore - Entries map[string][]initCredentialPlanEntry -} - -func groupInitWritesByStore(entries []initCredentialPlanEntry, writes map[string]map[string]string, overwriteRefs map[string]bool) ([]initWriteGroup, error) { - if len(entries) == 0 { - // An empty credential plan means init is using the legacy OS store path, - // represented by the zero-value unresolved selection. - return []initWriteGroup{{ - Writes: writes, - OverwriteRefs: overwriteRefs, - }}, nil - } - groups := map[string]*initWriteGroup{} - for _, entry := range entries { - bundle := writes[entry.Ref.Ref] - if len(bundle) == 0 { - continue - } - key := initCredentialStoreKey(entry.SecretsStore) - group := groups[key] - if group == nil { - group = &initWriteGroup{ - Resolved: entry.SecretsStore, - Writes: map[string]map[string]string{}, - OverwriteRefs: map[string]bool{}, - } - groups[key] = group - } - group.Writes[entry.Ref.Ref] = bundle - if overwriteRefs[entry.Ref.Ref] { - group.OverwriteRefs[entry.Ref.Ref] = true - } - } - keys := make([]string, 0, len(groups)) - for key := range groups { - keys = append(keys, key) - } - sort.Strings(keys) - out := make([]initWriteGroup, 0, len(keys)) - for _, key := range keys { - out = append(out, *groups[key]) - } - return out, nil -} - -func groupStaleReviewerCredentialCleanupsByStore(cfg config.File, entries []initCredentialPlanEntry) ([]initReviewerCredentialCleanupGroup, error) { - activeByStoreRef, resolvedByStore, err := activeReviewerCredentialEntriesByStoreRef(cfg, entries) - if err != nil { - return nil, err - } - cleanupRefsByStore := map[string]map[string]struct{}{} - for _, entry := range entries { - if !initCredentialEntryUsesActiveReviewerRef(entry) { - continue - } - storeKey := initCredentialStoreKey(entry.SecretsStore) - if initCredentialEntryChangesReviewerModeInPlace(entry) { - if cleanupRefsByStore[storeKey] == nil { - cleanupRefsByStore[storeKey] = map[string]struct{}{} - } - cleanupRefsByStore[storeKey][entry.Ref.Ref] = struct{}{} - } - } - - storeKeys := make([]string, 0, len(cleanupRefsByStore)) - for storeKey := range cleanupRefsByStore { - storeKeys = append(storeKeys, storeKey) - } - sort.Strings(storeKeys) - groups := make([]initReviewerCredentialCleanupGroup, 0, len(storeKeys)) - for _, storeKey := range storeKeys { - group := initReviewerCredentialCleanupGroup{ - Resolved: resolvedByStore[storeKey], - Entries: map[string][]initCredentialPlanEntry{}, - } - refs := make([]string, 0, len(cleanupRefsByStore[storeKey])) - for ref := range cleanupRefsByStore[storeKey] { - refs = append(refs, ref) - } - sort.Strings(refs) - for _, ref := range refs { - activeEntries := activeByStoreRef[storeKey][ref] - if len(activeEntries) == 0 { - continue - } - group.Entries[ref] = activeEntries - } - if len(group.Entries) > 0 { - groups = append(groups, group) - } - } - return groups, nil -} - -func activeReviewerCredentialEntriesByStoreRef(cfg config.File, fallback []initCredentialPlanEntry) (map[string]map[string][]initCredentialPlanEntry, map[string]credentials.ResolvedSecretsStore, error) { - activeByStoreRef := map[string]map[string][]initCredentialPlanEntry{} - resolvedByStore := map[string]credentials.ResolvedSecretsStore{} - if len(cfg.Profiles) == 0 { - for _, entry := range fallback { - if !initCredentialEntryUsesActiveReviewerRef(entry) { - continue - } - addActiveReviewerCredentialEntry(activeByStoreRef, resolvedByStore, entry) - } - return activeByStoreRef, resolvedByStore, nil - } - profileNames := make([]string, 0, len(cfg.Profiles)) - for profileName := range cfg.Profiles { - profileNames = append(profileNames, profileName) - } - sort.Strings(profileNames) - for _, profileName := range profileNames { - profile := cfg.Profiles[profileName] - refs, err := config.CredentialRefs(profile) - if err != nil { - return nil, nil, err - } - for _, ref := range refs { - if ref.Purpose != "reviewer_credentials" || strings.TrimSpace(ref.Ref) == "" { - continue - } - resolved, err := credentials.ResolveCredentialStore(cfg, ref.Store) - if err != nil { - return nil, nil, err - } - specs, err := credentials.KeySpecsForPurpose(ref) - if err != nil { - return nil, nil, err - } - addActiveReviewerCredentialEntry(activeByStoreRef, resolvedByStore, initCredentialPlanEntry{ - Ref: ref, - SecretsStore: resolved, - KeySpecs: append([]credentials.KeySpec(nil), specs...), - State: initCredentialPlanStateKeepExisting, - }) - } - } - return activeByStoreRef, resolvedByStore, nil -} - -func addActiveReviewerCredentialEntry(activeByStoreRef map[string]map[string][]initCredentialPlanEntry, resolvedByStore map[string]credentials.ResolvedSecretsStore, entry initCredentialPlanEntry) { - storeKey := initCredentialStoreKey(entry.SecretsStore) - resolvedByStore[storeKey] = entry.SecretsStore - if activeByStoreRef[storeKey] == nil { - activeByStoreRef[storeKey] = map[string][]initCredentialPlanEntry{} - } - activeByStoreRef[storeKey][entry.Ref.Ref] = append(activeByStoreRef[storeKey][entry.Ref.Ref], entry) -} - -func initCredentialEntryUsesActiveReviewerRef(entry initCredentialPlanEntry) bool { - return entry.State != initCredentialPlanStateClearRef && - entry.Ref.Purpose == "reviewer_credentials" && - strings.TrimSpace(entry.Ref.Ref) != "" -} - -func initCredentialEntryChangesReviewerModeInPlace(entry initCredentialPlanEntry) bool { - if !initCredentialEntryUsesActiveReviewerRef(entry) || entry.PreviousRef == nil { - return false - } - return entry.PreviousRef.Purpose == entry.Ref.Purpose && - entry.PreviousRef.Ref == entry.Ref.Ref && - entry.PreviousRef.Mode != entry.Ref.Mode -} +type initReviewerCredentialCleanupGroup = credentials.ReviewerCredentialCleanupGroup func openInitStoreForEntry(deps initDeps, opts *root.Options, flagSet bool, cfg config.File, entry initCredentialPlanEntry) (initStore, error) { backend := "" @@ -5610,9 +5427,9 @@ func collectInteractiveInitSecrets(_ *cobra.Command, opts *root.Options, deps in if prompter.pasteSecret == nil { prompter.pasteSecret = defaults.pasteSecret } - stores := map[string]initStore{} + stores := map[credentials.StoreIdentity]initStore{} openStore := func(entry initCredentialPlanEntry) (initStore, error) { - key := initCredentialStoreKey(entry.SecretsStore) + key := entry.SecretsStore.Identity() if store := stores[key]; store != nil { return store, nil } @@ -5882,7 +5699,7 @@ func loadInteractiveCredentialPlanStateWithResolver(entries []initCredentialPlan if !needsStore { return entries, nil } - stores := map[string]initStore{} + stores := map[credentials.StoreIdentity]initStore{} defer func() { for _, store := range stores { if store != nil { @@ -5900,7 +5717,7 @@ func loadInteractiveCredentialPlanStateWithResolver(entries []initCredentialPlan normalized = append(normalized, entry) continue } - key := initCredentialStoreKey(entry.SecretsStore) + key := entry.SecretsStore.Identity() store := stores[key] if store == nil { var err error @@ -6202,8 +6019,8 @@ func refreshInteractiveCredentialPlan(entries []initCredentialPlanEntry, planned refreshed = append(refreshed, next) continue } - next.MissingRequiredKeys = missingRequiredInitCredentialKeys(next.KeySpecs, next.PlannedWriteKeys) - next.State = classifyInitCredentialPlanEntry(next) + next.MissingRequiredKeys = credentials.MissingRequiredPlannedKeys(next.KeySpecs, next.PlannedWriteKeys) + next.State = credentials.ClassifyPlanEntry(next) refreshed = append(refreshed, next) } return refreshed @@ -6320,7 +6137,7 @@ func applyInteractiveInitSessionPlan(opts *root.Options, deps initDeps, plan ini } touchedCredentialRefs := map[string]struct{}{} if len(plan.writes) > 0 { - groups, err := groupInitWritesByStore(plan.credentialPlan, plan.writes, plan.overwriteRefs) + groups, err := credentials.GroupWritesByStore(plan.credentialPlan, plan.writes, plan.overwriteRefs) if err != nil { return cmderr.Config(err) } @@ -6346,7 +6163,7 @@ func applyInteractiveInitSessionPlan(opts *root.Options, deps initDeps, plan ini _ = store.Close() } } - cleanupGroups, err := groupStaleReviewerCredentialCleanupsByStore(plan.cfg, plan.credentialPlan) + cleanupGroups, err := credentials.GroupStaleReviewerCredentialCleanupsByStore(plan.cfg, plan.credentialPlan) if err != nil { return cmderr.Config(err) } @@ -6516,7 +6333,7 @@ func applyStaleReviewerCredentialCleanups(opts *root.Options, deps initDeps, bac func applyInitPlan(opts *root.Options, flags initOptions, deps initDeps, plan initPlan) error { var store initStore var primaryResolved credentials.ResolvedSecretsStore - cleanupGroups, err := groupStaleReviewerCredentialCleanupsByStore(plan.cfg, plan.credentialPlan) + cleanupGroups, err := credentials.GroupStaleReviewerCredentialCleanupsByStore(plan.cfg, plan.credentialPlan) if err != nil { return cmderr.Config(err) } @@ -6576,7 +6393,7 @@ func applyInitPlan(opts *root.Options, flags initOptions, deps initDeps, plan in } touchedCredentialRefs := map[string]struct{}{} if len(plan.writes) > 0 { - groups, err := groupInitWritesByStore(plan.credentialPlan, plan.writes, plan.overwriteRefs) + groups, err := credentials.GroupWritesByStore(plan.credentialPlan, plan.writes, plan.overwriteRefs) if err != nil { return cmderr.Config(err) } @@ -6690,132 +6507,6 @@ func projectInitPlannedWriteKeys(writes map[string]map[string]string) map[string return projected } -func planInitCredentials(previousProfile *config.Profile, desiredProfile config.Profile, plannedWriteKeys map[string][]string) ([]initCredentialPlanEntry, error) { - return planInitCredentialsWithConfig(config.File{}, previousProfile, desiredProfile, plannedWriteKeys) -} - -func planInitCredentialsWithConfig(cfg config.File, previousProfile *config.Profile, desiredProfile config.Profile, plannedWriteKeys map[string][]string) ([]initCredentialPlanEntry, error) { - desiredRefs, err := config.CredentialRefs(desiredProfile) - if err != nil { - return nil, err - } - var previousRefs []config.CredentialRef - if previousProfile != nil { - previousRefs, err = config.CredentialRefs(*previousProfile) - if err != nil { - return nil, err - } - } - - previousByPurpose := make(map[string]config.CredentialRef, len(previousRefs)) - for _, ref := range previousRefs { - previousByPurpose[ref.Purpose] = ref - } - - entries := make([]initCredentialPlanEntry, 0, len(desiredRefs)+len(previousRefs)) - for _, ref := range desiredRefs { - resolvedStore, err := credentials.ResolveCredentialStore(cfg, ref.Store) - if err != nil { - return nil, err - } - specs, err := credentials.KeySpecsForPurpose(ref) - if err != nil { - return nil, err - } - writeKeys := append([]string(nil), plannedWriteKeys[ref.Ref]...) - if err := validateInitPlannedWriteKeys(ref, specs, writeKeys); err != nil { - return nil, err - } - entry := initCredentialPlanEntry{ - Ref: ref, - SecretsStore: resolvedStore, - KeySpecs: append([]credentials.KeySpec(nil), specs...), - PlannedWriteKeys: writeKeys, - } - if previousRef, ok := previousByPurpose[ref.Purpose]; ok { - previousCopy := previousRef - entry.PreviousRef = &previousCopy - delete(previousByPurpose, ref.Purpose) - } - entry.MissingRequiredKeys = missingRequiredInitCredentialKeys(entry.KeySpecs, entry.PlannedWriteKeys) - entry.State = classifyInitCredentialPlanEntry(entry) - entries = append(entries, entry) - } - for _, ref := range previousRefs { - if _, ok := previousByPurpose[ref.Purpose]; !ok { - continue - } - resolvedStore, err := credentials.ResolveCredentialStore(cfg, ref.Store) - if err != nil { - return nil, err - } - entries = append(entries, initCredentialPlanEntry{ - Ref: ref, - SecretsStore: resolvedStore, - State: initCredentialPlanStateClearRef, - }) - } - return entries, nil -} - -func validateInitPlannedWriteKeys(ref config.CredentialRef, specs []credentials.KeySpec, plannedWriteKeys []string) error { - if len(plannedWriteKeys) == 0 { - return nil - } - allowed := make(map[string]struct{}, len(specs)) - for _, spec := range specs { - allowed[spec.Key] = struct{}{} - } - for _, key := range plannedWriteKeys { - if _, ok := allowed[key]; ok { - continue - } - return fmt.Errorf("init credential planner: unexpected planned write key %q for %s credential name %q", key, ref.Purpose, ref.Ref) - } - return nil -} - -func missingRequiredInitCredentialKeys(specs []credentials.KeySpec, plannedWriteKeys []string) []string { - if len(plannedWriteKeys) == 0 { - return nil - } - present := make(map[string]struct{}, len(plannedWriteKeys)) - for _, key := range plannedWriteKeys { - present[key] = struct{}{} - } - var missing []string - for _, spec := range specs { - if !spec.Required { - continue - } - if _, ok := present[spec.Key]; ok { - continue - } - missing = append(missing, spec.Key) - } - return missing -} - -func classifyInitCredentialPlanEntry(entry initCredentialPlanEntry) initCredentialPlanState { - if len(entry.MissingRequiredKeys) > 0 { - return initCredentialPlanStateMissingRequired - } - if len(entry.PlannedWriteKeys) > 0 { - return initCredentialPlanStateWrite - } - if entry.PreviousRef == nil { - return initCredentialPlanStateDefer - } - if entry.PreviousRef.Purpose == entry.Ref.Purpose && - entry.PreviousRef.Store == entry.Ref.Store && - entry.PreviousRef.Ref == entry.Ref.Ref && - entry.PreviousRef.Mode == entry.Ref.Mode && - entry.PreviousRef.Provider == entry.Ref.Provider { - return initCredentialPlanStateKeepExisting - } - return initCredentialPlanStateOverwriteRef -} - func shouldWriteInitCredentialHint(entry initCredentialPlanEntry, includeLLM bool) bool { if entry.Ref.Purpose == "llm" && !includeLLM { return false @@ -6972,7 +6663,7 @@ func deleteStaleReviewerCredentialKeys(store initStore, entries []initCredential return false, nil } } - staleKeys := staleReviewerCredentialKeys(entries) + staleKeys := credentials.StaleReviewerCredentialKeys(entries) if len(staleKeys) == 0 { return false, nil } @@ -7001,28 +6692,6 @@ func deleteStaleReviewerCredentialKeys(store initStore, entries []initCredential return deleted, nil } -func staleReviewerCredentialKeys(entries []initCredentialPlanEntry) []string { - current := map[string]struct{}{} - for _, entry := range entries { - for _, spec := range entry.KeySpecs { - current[spec.Key] = struct{}{} - } - } - candidates := []string{ - credentials.GitTokenKey, - credentials.GitHubAppIDKey, - credentials.GitHubAppPrivateKeyKey, - credentials.GitHubAppInstallationIDKey, - } - var stale []string - for _, key := range candidates { - if _, ok := current[key]; !ok { - stale = append(stale, key) - } - } - return stale -} - func sortedRefs(writes map[string]map[string]string) []string { refs := make([]string, 0, len(writes)) for ref := range writes { diff --git a/internal/cmd/initcmd/initcmd_test.go b/internal/cmd/initcmd/initcmd_test.go index db05bb1..c06df0c 100644 --- a/internal/cmd/initcmd/initcmd_test.go +++ b/internal/cmd/initcmd/initcmd_test.go @@ -339,198 +339,13 @@ func TestInitNonInteractiveWritesPiRPCProfile(t *testing.T) { assertFakeStored(t, store, "default", credentials.GitTokenKey, "git-token") } -func TestPlanInitCredentials(t *testing.T) { - t.Run("new git pat defaults to defer", func(t *testing.T) { - desired := basicProfile("work") - entries, err := planInitCredentials(nil, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - if len(entries) != 1 { - t.Fatalf("entries len = %d, want 1", len(entries)) - } - entry := entries[0] - if entry.Ref.Ref != "codereview/work" || entry.State != initCredentialPlanStateDefer { - t.Fatalf("entry = %#v, want git defer codereview/work", entry) - } - if !reflect.DeepEqual(entry.KeySpecs, []credentials.KeySpec{{Key: credentials.GitTokenKey, Required: true}}) { - t.Fatalf("key specs = %#v, want git_token required", entry.KeySpecs) - } - }) - - t.Run("reviewer github app default ref includes bundle keys", func(t *testing.T) { - desired := basicProfile("work") - desired.ReviewerCredentials = &config.ReviewerCredentials{ - AuthMode: config.GitAuthModeGitHubApp, - GitHubApp: &config.GitHubAppConfig{AppID: "12345"}, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, - } - entries, err := planInitCredentials(nil, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - if len(entries) != 2 { - t.Fatalf("entries len = %d, want 2", len(entries)) - } - entry := entries[1] - if entry.Ref.Purpose != "reviewer_credentials" || entry.Ref.Ref != "codereview/work-reviewer" || entry.State != initCredentialPlanStateDefer { - t.Fatalf("reviewer entry = %#v, want deferred reviewer app ref", entry) - } - want := []credentials.KeySpec{{Key: credentials.GitHubAppPrivateKeyKey, Required: true}} - if !reflect.DeepEqual(entry.KeySpecs, want) { - t.Fatalf("key specs = %#v, want %#v", entry.KeySpecs, want) - } - }) - - t.Run("llm providers use provider-specific api keys", func(t *testing.T) { - for _, tt := range []struct { - name string - provider config.LLMProvider - adapter config.LLMAdapter - key string - }{ - {name: "anthropic", provider: config.LLMProviderAnthropic, adapter: config.LLMAdapterAnthropicAPI, key: credentials.AnthropicAPIKeyKey}, - {name: "openai", provider: config.LLMProviderOpenAI, adapter: config.LLMAdapterOpenAIAPI, key: credentials.OpenAIAPIKeyKey}, - } { - t.Run(tt.name, func(t *testing.T) { - desired := basicProfile("work") - desired.LLM = config.LLMConfig{ - Provider: tt.provider, - Auth: config.LLMAuthAPIKey, - Adapter: tt.adapter, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-llm"}, - } - entries, err := planInitCredentials(nil, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - if len(entries) != 2 { - t.Fatalf("entries len = %d, want 2", len(entries)) - } - entry := entries[1] - if entry.Ref.Purpose != "llm" || entry.State != initCredentialPlanStateDefer { - t.Fatalf("llm entry = %#v, want deferred llm credential", entry) - } - want := []credentials.KeySpec{{Key: tt.key, Required: true}} - if !reflect.DeepEqual(entry.KeySpecs, want) { - t.Fatalf("key specs = %#v, want %#v", entry.KeySpecs, want) - } - }) - } - }) - - t.Run("preserve custom refs across rename interactions", func(t *testing.T) { - cfg := config.File{ - Profiles: map[string]config.Profile{ - "work": { - Git: config.GitConfig{ - Host: "github.com", - AuthMode: config.GitAuthModePAT, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-work"}, - }, - ReviewerCredentials: &config.ReviewerCredentials{ - AuthMode: config.GitAuthModePAT, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-reviewer"}, - }, - LLM: config.LLMConfig{ - Provider: config.LLMProviderAnthropic, - Auth: config.LLMAuthAPIKey, - Adapter: config.LLMAdapterAnthropicAPI, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-llm"}, - }, - }, - }, - } - renamed, changed, err := configedit.RenameProfile(cfg, "work", "office") - if err != nil { - t.Fatalf("RenameProfile: %v", err) - } - if !changed { - t.Fatal("RenameProfile changed = false, want true") - } - previous := cfg.Profiles["work"] - desired := renamed.Profiles["office"] - entries, err := planInitCredentials(&previous, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - for _, entry := range entries { - if entry.State != initCredentialPlanStateKeepExisting { - t.Fatalf("entry = %#v, want keep_existing across rename", entry) - } - } - }) - - t.Run("overwrite custom ref without writes is tracked separately", func(t *testing.T) { - previous := basicProfile("work") - previous.Git.Credential = config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-old"} - desired := previous - desired.Git.Credential = config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-new"} - entries, err := planInitCredentials(&previous, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - if entries[0].State != initCredentialPlanStateOverwriteRef { - t.Fatalf("state = %s, want overwrite_ref", entries[0].State) - } - }) - - t.Run("optional refs can clear", func(t *testing.T) { - previous := apiKeyProfile("work", config.LLMProviderAnthropic) - previous.ReviewerCredentials = &config.ReviewerCredentials{ - AuthMode: config.GitAuthModePAT, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, - } - desired := basicProfile("work") - entries, err := planInitCredentials(&previous, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - states := map[string]initCredentialPlanState{} - for _, entry := range entries { - states[entry.Ref.Purpose] = entry.State - } - if states["git"] != initCredentialPlanStateKeepExisting { - t.Fatalf("git state = %s, want keep_existing", states["git"]) - } - if states["reviewer_credentials"] != initCredentialPlanStateClearRef { - t.Fatalf("reviewer state = %s, want clear_ref", states["reviewer_credentials"]) - } - if states["llm"] != initCredentialPlanStateClearRef { - t.Fatalf("llm state = %s, want clear_ref", states["llm"]) - } - }) - - t.Run("github app private key not staged defers credential setup", func(t *testing.T) { - desired := basicProfile("work") - desired.Git.AuthMode = config.GitAuthModeGitHubApp - desired.Git.GitHubApp = &config.GitHubAppConfig{AppID: "12345"} - entries, err := planInitCredentials(nil, desired, map[string][]string{ - "codereview/work": {}, - }) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - entry := entries[0] - if entry.State != initCredentialPlanStateDefer { - t.Fatalf("state = %s, want defer", entry.State) - } - if len(entry.PlannedWriteKeys) != 0 { - t.Fatalf("planned write keys = %#v, want none", entry.PlannedWriteKeys) - } - if len(entry.MissingRequiredKeys) != 0 { - t.Fatalf("missing required = %#v, want none without staged writes", entry.MissingRequiredKeys) - } - }) -} - func TestLoadInteractiveCredentialPlanStateSkipsStoreForSettledEntries(t *testing.T) { previous := basicProfile("work") - keepEntries, err := planInitCredentials(&previous, previous, nil) + keepEntries, err := credentials.Plan(&previous, previous, nil) if err != nil { t.Fatalf("planInitCredentials keep: %v", err) } - writeEntries, err := planInitCredentials(nil, basicProfile("office"), map[string][]string{ + writeEntries, err := credentials.Plan(nil, basicProfile("office"), map[string][]string{ "codereview/office": {credentials.GitTokenKey}, }) if err != nil { @@ -563,13 +378,13 @@ func TestLoadInteractiveCredentialPlanStateSkipsStoreForSettledEntries(t *testin } func TestLoadInteractiveCredentialPlanStatePreservesSettledEntriesWhenMixedPlanNeedsStore(t *testing.T) { - keepEntries, err := planInitCredentials(nil, basicProfile("work"), nil) + keepEntries, err := credentials.Plan(nil, basicProfile("work"), nil) if err != nil { t.Fatalf("planInitCredentials keep seed: %v", err) } keepEntries[0].State = initCredentialPlanStateKeepExisting - writeEntries, err := planInitCredentials(nil, basicProfile("office"), map[string][]string{ + writeEntries, err := credentials.Plan(nil, basicProfile("office"), map[string][]string{ "codereview/office": {credentials.GitTokenKey}, }) if err != nil { @@ -577,7 +392,7 @@ func TestLoadInteractiveCredentialPlanStatePreservesSettledEntriesWhenMixedPlanN } deferredProfile := apiKeyProfile("lab", config.LLMProviderAnthropic) - deferredEntries, err := planInitCredentials(nil, deferredProfile, nil) + deferredEntries, err := credentials.Plan(nil, deferredProfile, nil) if err != nil { t.Fatalf("planInitCredentials deferred: %v", err) } @@ -3498,30 +3313,6 @@ func testInitMultiKeySecretWorkspace() initWorkspaceDraft { } } -func TestPlanInitCredentialsClearsOptionalRefsInStableOrder(t *testing.T) { - previous := apiKeyProfile("work", config.LLMProviderAnthropic) - previous.ReviewerCredentials = &config.ReviewerCredentials{ - AuthMode: config.GitAuthModePAT, - Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, - } - desired := basicProfile("work") - - entries, err := planInitCredentials(&previous, desired, nil) - if err != nil { - t.Fatalf("planInitCredentials: %v", err) - } - - var cleared []string - for _, entry := range entries { - if entry.State == initCredentialPlanStateClearRef { - cleared = append(cleared, entry.Ref.Purpose) - } - } - if !reflect.DeepEqual(cleared, []string{"reviewer_credentials", "llm"}) { - t.Fatalf("cleared purposes = %#v, want reviewer then llm", cleared) - } -} - func TestWriteInitCredentialPlanHintsUsesMissingRequiredKeysOnly(t *testing.T) { var stderr bytes.Buffer entry := initCredentialPlanEntry{ @@ -14000,7 +13791,7 @@ func TestMergeReviewerCredentialDraftWritesDropsStaleKeysWhenAuthModeChanges(t * GitHubApp: &config.GitHubAppConfig{AppID: "12345"}, Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: ref}, } - if _, err := planInitCredentialsWithConfig(config.File{Profiles: map[string]config.Profile{"work": profile}}, nil, profile, projectInitPlannedWriteKeys(session.writes)); err != nil { + if _, err := credentials.PlanWithConfig(config.File{Profiles: map[string]config.Profile{"work": profile}}, nil, profile, projectInitPlannedWriteKeys(session.writes)); err != nil { t.Fatalf("planInitCredentialsWithConfig: %v", err) } }) @@ -14030,7 +13821,7 @@ func TestMergeReviewerCredentialDraftWritesDropsStaleKeysWhenAuthModeChanges(t * AuthMode: config.GitAuthModePAT, Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: ref}, } - if _, err := planInitCredentialsWithConfig(config.File{Profiles: map[string]config.Profile{"work": profile}}, nil, profile, projectInitPlannedWriteKeys(session.writes)); err != nil { + if _, err := credentials.PlanWithConfig(config.File{Profiles: map[string]config.Profile{"work": profile}}, nil, profile, projectInitPlannedWriteKeys(session.writes)); err != nil { t.Fatalf("planInitCredentialsWithConfig: %v", err) } }) @@ -18338,7 +18129,7 @@ func TestStaleReviewerCleanupKeepsKeysRequiredByAnotherActiveModeWithoutWrites(t t.Fatalf("ResolveSecretsStoreForProfile: %v", err) } appEntry.SecretsStore = resolved - groups, err := groupStaleReviewerCredentialCleanupsByStore(cfg, []initCredentialPlanEntry{appEntry}) + groups, err := credentials.GroupStaleReviewerCredentialCleanupsByStore(cfg, []initCredentialPlanEntry{appEntry}) if err != nil { t.Fatalf("groupStaleReviewerCredentialCleanupsByStore: %v", err) } diff --git a/internal/credentials/planning.go b/internal/credentials/planning.go new file mode 100644 index 0000000..ef10d34 --- /dev/null +++ b/internal/credentials/planning.go @@ -0,0 +1,356 @@ +package credentials + +import ( + "cmp" + "fmt" + "maps" + "slices" + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +// PlanState describes the credential transition init must apply. +type PlanState string + +const ( + // PlanStateKeepExisting preserves an unchanged credential reference. + PlanStateKeepExisting PlanState = "keep_existing" + // PlanStateDefer leaves credential setup for a later command. + PlanStateDefer PlanState = "defer" + // PlanStateOverwriteRef replaces a changed credential reference. + // #nosec G101 -- planner state label, not secret material. + PlanStateOverwriteRef PlanState = "overwrite_ref" + // PlanStateWrite writes the staged credential keys. + PlanStateWrite PlanState = "write" + // PlanStateClearRef removes a credential reference no longer desired. + PlanStateClearRef PlanState = "clear_ref" + // PlanStateMissingRequired reports an incomplete staged credential bundle. + PlanStateMissingRequired PlanState = "missing_required" +) + +// PlanEntry describes one pure credential transition. +type PlanEntry struct { + Ref config.CredentialRef + PreviousRef *config.CredentialRef + SecretsStore ResolvedSecretsStore + KeySpecs []KeySpec + PlannedWriteKeys []string + MissingRequiredKeys []string + State PlanState +} + +// StoreIdentity is the comparable identity of a resolved credential store. +type StoreIdentity struct { + ID string + Source config.EffectiveSecretsStoreSource + Backend string +} + +// Identity returns the comparable identity used to group resolved stores. +func (r ResolvedSecretsStore) Identity() StoreIdentity { + return StoreIdentity{ID: r.ID, Source: r.Source, Backend: r.Backend} +} + +// WriteGroup collects staged writes targeting one resolved credential store. +type WriteGroup struct { + Resolved ResolvedSecretsStore + Writes map[string]map[string]string + OverwriteRefs map[string]bool +} + +// ReviewerCredentialCleanupGroup collects reviewer refs requiring stale-key cleanup in one store. +type ReviewerCredentialCleanupGroup struct { + Resolved ResolvedSecretsStore + Entries map[string][]PlanEntry +} + +// Plan compares previous and desired profiles using the built-in store configuration. +func Plan(previousProfile *config.Profile, desiredProfile config.Profile, plannedWriteKeys map[string][]string) ([]PlanEntry, error) { + return PlanWithConfig(config.File{}, previousProfile, desiredProfile, plannedWriteKeys) +} + +// PlanWithConfig compares previous and desired profiles and resolves each target store. +func PlanWithConfig(cfg config.File, previousProfile *config.Profile, desiredProfile config.Profile, plannedWriteKeys map[string][]string) ([]PlanEntry, error) { + desiredRefs, err := config.CredentialRefs(desiredProfile) + if err != nil { + return nil, err + } + var previousRefs []config.CredentialRef + if previousProfile != nil { + previousRefs, err = config.CredentialRefs(*previousProfile) + if err != nil { + return nil, err + } + } + + previousByPurpose := make(map[string]config.CredentialRef, len(previousRefs)) + for _, ref := range previousRefs { + previousByPurpose[ref.Purpose] = ref + } + + entries := make([]PlanEntry, 0, len(desiredRefs)+len(previousRefs)) + for _, ref := range desiredRefs { + resolvedStore, err := ResolveCredentialStore(cfg, ref.Store) + if err != nil { + return nil, err + } + specs, err := KeySpecsForPurpose(ref) + if err != nil { + return nil, err + } + writeKeys := append([]string(nil), plannedWriteKeys[ref.Ref]...) + if err := ValidatePlannedWriteKeys(ref, specs, writeKeys); err != nil { + return nil, err + } + entry := PlanEntry{ + Ref: ref, + SecretsStore: resolvedStore, + KeySpecs: append([]KeySpec(nil), specs...), + PlannedWriteKeys: writeKeys, + } + if previousRef, ok := previousByPurpose[ref.Purpose]; ok { + previousCopy := previousRef + entry.PreviousRef = &previousCopy + delete(previousByPurpose, ref.Purpose) + } + entry.MissingRequiredKeys = MissingRequiredPlannedKeys(entry.KeySpecs, entry.PlannedWriteKeys) + entry.State = ClassifyPlanEntry(entry) + entries = append(entries, entry) + } + for _, ref := range previousRefs { + if _, ok := previousByPurpose[ref.Purpose]; !ok { + continue + } + resolvedStore, err := ResolveCredentialStore(cfg, ref.Store) + if err != nil { + return nil, err + } + entries = append(entries, PlanEntry{ + Ref: ref, + SecretsStore: resolvedStore, + State: PlanStateClearRef, + }) + } + return entries, nil +} + +// ValidatePlannedWriteKeys rejects staged keys outside a credential purpose's key specs. +func ValidatePlannedWriteKeys(ref config.CredentialRef, specs []KeySpec, plannedWriteKeys []string) error { + if len(plannedWriteKeys) == 0 { + return nil + } + allowed := make(map[string]struct{}, len(specs)) + for _, spec := range specs { + allowed[spec.Key] = struct{}{} + } + for _, key := range plannedWriteKeys { + if _, ok := allowed[key]; ok { + continue + } + return fmt.Errorf("init credential planner: unexpected planned write key %q for %s credential name %q", key, ref.Purpose, ref.Ref) + } + return nil +} + +// MissingRequiredPlannedKeys returns required keys absent from a non-empty staged bundle. +func MissingRequiredPlannedKeys(specs []KeySpec, plannedWriteKeys []string) []string { + if len(plannedWriteKeys) == 0 { + return nil + } + present := make(map[string]struct{}, len(plannedWriteKeys)) + for _, key := range plannedWriteKeys { + present[key] = struct{}{} + } + var missing []string + for _, spec := range specs { + if !spec.Required { + continue + } + if _, ok := present[spec.Key]; !ok { + missing = append(missing, spec.Key) + } + } + return missing +} + +// ClassifyPlanEntry returns the transition state for a planned entry. +func ClassifyPlanEntry(entry PlanEntry) PlanState { + if len(entry.MissingRequiredKeys) > 0 { + return PlanStateMissingRequired + } + if len(entry.PlannedWriteKeys) > 0 { + return PlanStateWrite + } + if entry.PreviousRef == nil { + return PlanStateDefer + } + if entry.PreviousRef.Purpose == entry.Ref.Purpose && + entry.PreviousRef.Store == entry.Ref.Store && + entry.PreviousRef.Ref == entry.Ref.Ref && + entry.PreviousRef.Mode == entry.Ref.Mode && + entry.PreviousRef.Provider == entry.Ref.Provider { + return PlanStateKeepExisting + } + return PlanStateOverwriteRef +} + +// GroupWritesByStore groups staged credential bundles by comparable store identity. +func GroupWritesByStore(entries []PlanEntry, writes map[string]map[string]string, overwriteRefs map[string]bool) ([]WriteGroup, error) { + if len(entries) == 0 { + return []WriteGroup{{Writes: writes, OverwriteRefs: overwriteRefs}}, nil + } + groups := map[StoreIdentity]*WriteGroup{} + for _, entry := range entries { + bundle := writes[entry.Ref.Ref] + if len(bundle) == 0 { + continue + } + identity := entry.SecretsStore.Identity() + group := groups[identity] + if group == nil { + group = &WriteGroup{ + Resolved: entry.SecretsStore, + Writes: map[string]map[string]string{}, + OverwriteRefs: map[string]bool{}, + } + groups[identity] = group + } + group.Writes[entry.Ref.Ref] = bundle + if overwriteRefs[entry.Ref.Ref] { + group.OverwriteRefs[entry.Ref.Ref] = true + } + } + identities := slices.Collect(maps.Keys(groups)) + sortStoreIdentities(identities) + out := make([]WriteGroup, 0, len(identities)) + for _, identity := range identities { + out = append(out, *groups[identity]) + } + return out, nil +} + +// GroupStaleReviewerCredentialCleanupsByStore finds in-place reviewer auth changes and protects keys used by every active profile. +func GroupStaleReviewerCredentialCleanupsByStore(cfg config.File, entries []PlanEntry) ([]ReviewerCredentialCleanupGroup, error) { + activeByStoreRef, resolvedByStore, err := activeReviewerCredentialEntriesByStoreRef(cfg, entries) + if err != nil { + return nil, err + } + cleanupRefsByStore := map[StoreIdentity]map[string]struct{}{} + for _, entry := range entries { + if !credentialEntryChangesReviewerModeInPlace(entry) { + continue + } + identity := entry.SecretsStore.Identity() + if cleanupRefsByStore[identity] == nil { + cleanupRefsByStore[identity] = map[string]struct{}{} + } + cleanupRefsByStore[identity][entry.Ref.Ref] = struct{}{} + } + + identities := slices.Collect(maps.Keys(cleanupRefsByStore)) + sortStoreIdentities(identities) + groups := make([]ReviewerCredentialCleanupGroup, 0, len(identities)) + for _, identity := range identities { + group := ReviewerCredentialCleanupGroup{ + Resolved: resolvedByStore[identity], + Entries: map[string][]PlanEntry{}, + } + for _, ref := range slices.Sorted(maps.Keys(cleanupRefsByStore[identity])) { + activeEntries := activeByStoreRef[identity][ref] + if len(activeEntries) > 0 { + group.Entries[ref] = activeEntries + } + } + if len(group.Entries) > 0 { + groups = append(groups, group) + } + } + return groups, nil +} + +// StaleReviewerCredentialKeys returns reviewer bundle keys unused by all active entries sharing a ref. +func StaleReviewerCredentialKeys(entries []PlanEntry) []string { + current := map[string]struct{}{} + for _, entry := range entries { + for _, spec := range entry.KeySpecs { + current[spec.Key] = struct{}{} + } + } + candidates := []string{GitTokenKey, GitHubAppIDKey, GitHubAppPrivateKeyKey, GitHubAppInstallationIDKey} + var stale []string + for _, key := range candidates { + if _, ok := current[key]; !ok { + stale = append(stale, key) + } + } + return stale +} + +func activeReviewerCredentialEntriesByStoreRef(cfg config.File, fallback []PlanEntry) (map[StoreIdentity]map[string][]PlanEntry, map[StoreIdentity]ResolvedSecretsStore, error) { + activeByStoreRef := map[StoreIdentity]map[string][]PlanEntry{} + resolvedByStore := map[StoreIdentity]ResolvedSecretsStore{} + if len(cfg.Profiles) == 0 { + for _, entry := range fallback { + if credentialEntryUsesActiveReviewerRef(entry) { + addActiveReviewerCredentialEntry(activeByStoreRef, resolvedByStore, entry) + } + } + return activeByStoreRef, resolvedByStore, nil + } + for _, profileName := range slices.Sorted(maps.Keys(cfg.Profiles)) { + refs, err := config.CredentialRefs(cfg.Profiles[profileName]) + if err != nil { + return nil, nil, err + } + for _, ref := range refs { + if ref.Purpose != "reviewer_credentials" || strings.TrimSpace(ref.Ref) == "" { + continue + } + resolved, err := ResolveCredentialStore(cfg, ref.Store) + if err != nil { + return nil, nil, err + } + specs, err := KeySpecsForPurpose(ref) + if err != nil { + return nil, nil, err + } + addActiveReviewerCredentialEntry(activeByStoreRef, resolvedByStore, PlanEntry{ + Ref: ref, SecretsStore: resolved, KeySpecs: append([]KeySpec(nil), specs...), State: PlanStateKeepExisting, + }) + } + } + return activeByStoreRef, resolvedByStore, nil +} + +func addActiveReviewerCredentialEntry(active map[StoreIdentity]map[string][]PlanEntry, resolved map[StoreIdentity]ResolvedSecretsStore, entry PlanEntry) { + identity := entry.SecretsStore.Identity() + resolved[identity] = entry.SecretsStore + if active[identity] == nil { + active[identity] = map[string][]PlanEntry{} + } + active[identity][entry.Ref.Ref] = append(active[identity][entry.Ref.Ref], entry) +} + +func credentialEntryUsesActiveReviewerRef(entry PlanEntry) bool { + return entry.State != PlanStateClearRef && entry.Ref.Purpose == "reviewer_credentials" && strings.TrimSpace(entry.Ref.Ref) != "" +} + +func credentialEntryChangesReviewerModeInPlace(entry PlanEntry) bool { + return credentialEntryUsesActiveReviewerRef(entry) && entry.PreviousRef != nil && + entry.PreviousRef.Purpose == entry.Ref.Purpose && + entry.PreviousRef.Ref == entry.Ref.Ref && + entry.PreviousRef.Mode != entry.Ref.Mode +} + +func sortStoreIdentities(identities []StoreIdentity) { + slices.SortFunc(identities, func(a, b StoreIdentity) int { + if n := cmp.Compare(a.ID, b.ID); n != 0 { + return n + } + if n := cmp.Compare(a.Source, b.Source); n != 0 { + return n + } + return cmp.Compare(a.Backend, b.Backend) + }) +} diff --git a/internal/credentials/planning_test.go b/internal/credentials/planning_test.go new file mode 100644 index 0000000..b68839c --- /dev/null +++ b/internal/credentials/planning_test.go @@ -0,0 +1,368 @@ +package credentials + +import ( + "reflect" + "testing" + + "github.com/open-cli-collective/cli-common/credstore" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/configedit" +) + +func TestPlan(t *testing.T) { + t.Run("new git pat defaults to defer", func(t *testing.T) { + desired := planningBasicProfile("work") + entries, err := Plan(nil, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if len(entries) != 1 { + t.Fatalf("entries len = %d, want 1", len(entries)) + } + entry := entries[0] + if entry.Ref.Ref != "codereview/work" || entry.State != PlanStateDefer { + t.Fatalf("entry = %#v, want git defer codereview/work", entry) + } + if !reflect.DeepEqual(entry.KeySpecs, []KeySpec{{Key: GitTokenKey, Required: true}}) { + t.Fatalf("key specs = %#v, want git_token required", entry.KeySpecs) + } + }) + + t.Run("reviewer github app default ref includes bundle keys", func(t *testing.T) { + desired := planningBasicProfile("work") + desired.ReviewerCredentials = &config.ReviewerCredentials{ + AuthMode: config.GitAuthModeGitHubApp, + GitHubApp: &config.GitHubAppConfig{AppID: "12345"}, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, + } + entries, err := Plan(nil, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if len(entries) != 2 { + t.Fatalf("entries len = %d, want 2", len(entries)) + } + entry := entries[1] + if entry.Ref.Purpose != "reviewer_credentials" || entry.Ref.Ref != "codereview/work-reviewer" || entry.State != PlanStateDefer { + t.Fatalf("reviewer entry = %#v, want deferred reviewer app ref", entry) + } + want := []KeySpec{{Key: GitHubAppPrivateKeyKey, Required: true}} + if !reflect.DeepEqual(entry.KeySpecs, want) { + t.Fatalf("key specs = %#v, want %#v", entry.KeySpecs, want) + } + }) + + t.Run("llm providers use provider-specific api keys", func(t *testing.T) { + for _, tt := range []struct { + name string + provider config.LLMProvider + adapter config.LLMAdapter + key string + }{ + {name: "anthropic", provider: config.LLMProviderAnthropic, adapter: config.LLMAdapterAnthropicAPI, key: AnthropicAPIKeyKey}, + {name: "openai", provider: config.LLMProviderOpenAI, adapter: config.LLMAdapterOpenAIAPI, key: OpenAIAPIKeyKey}, + } { + t.Run(tt.name, func(t *testing.T) { + desired := planningBasicProfile("work") + desired.LLM = config.LLMConfig{ + Provider: tt.provider, + Auth: config.LLMAuthAPIKey, + Adapter: tt.adapter, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-llm"}, + } + entries, err := Plan(nil, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if len(entries) != 2 { + t.Fatalf("entries len = %d, want 2", len(entries)) + } + entry := entries[1] + if entry.Ref.Purpose != "llm" || entry.State != PlanStateDefer { + t.Fatalf("llm entry = %#v, want deferred llm credential", entry) + } + want := []KeySpec{{Key: tt.key, Required: true}} + if !reflect.DeepEqual(entry.KeySpecs, want) { + t.Fatalf("key specs = %#v, want %#v", entry.KeySpecs, want) + } + }) + } + }) + + t.Run("preserve custom refs across rename interactions", func(t *testing.T) { + cfg := config.File{Profiles: map[string]config.Profile{"work": { + Git: config.GitConfig{ + Host: "github.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-work"}, + }, + ReviewerCredentials: &config.ReviewerCredentials{ + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-reviewer"}, + }, + LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthAPIKey, + Adapter: config.LLMAdapterAnthropicAPI, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-llm"}, + }, + }}} + renamed, changed, err := configedit.RenameProfile(cfg, "work", "office") + if err != nil { + t.Fatalf("RenameProfile: %v", err) + } + if !changed { + t.Fatal("RenameProfile changed = false, want true") + } + previous := cfg.Profiles["work"] + desired := renamed.Profiles["office"] + entries, err := Plan(&previous, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + for _, entry := range entries { + if entry.State != PlanStateKeepExisting { + t.Fatalf("entry = %#v, want keep_existing across rename", entry) + } + } + }) + + t.Run("overwrite custom ref without writes is tracked separately", func(t *testing.T) { + previous := planningBasicProfile("work") + previous.Git.Credential = config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-old"} + desired := previous + desired.Git.Credential = config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/custom-new"} + entries, err := Plan(&previous, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if entries[0].State != PlanStateOverwriteRef { + t.Fatalf("state = %s, want overwrite_ref", entries[0].State) + } + }) + + t.Run("optional refs can clear", func(t *testing.T) { + previous := planningAPIKeyProfile("work", config.LLMProviderAnthropic) + previous.ReviewerCredentials = &config.ReviewerCredentials{ + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, + } + desired := planningBasicProfile("work") + entries, err := Plan(&previous, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + states := map[string]PlanState{} + for _, entry := range entries { + states[entry.Ref.Purpose] = entry.State + } + if states["git"] != PlanStateKeepExisting { + t.Fatalf("git state = %s, want keep_existing", states["git"]) + } + if states["reviewer_credentials"] != PlanStateClearRef { + t.Fatalf("reviewer state = %s, want clear_ref", states["reviewer_credentials"]) + } + if states["llm"] != PlanStateClearRef { + t.Fatalf("llm state = %s, want clear_ref", states["llm"]) + } + }) + + t.Run("github app private key not staged defers credential setup", func(t *testing.T) { + desired := planningBasicProfile("work") + desired.Git.AuthMode = config.GitAuthModeGitHubApp + desired.Git.GitHubApp = &config.GitHubAppConfig{AppID: "12345"} + entries, err := Plan(nil, desired, map[string][]string{"codereview/work": {}}) + if err != nil { + t.Fatalf("Plan: %v", err) + } + entry := entries[0] + if entry.State != PlanStateDefer { + t.Fatalf("state = %s, want defer", entry.State) + } + if len(entry.PlannedWriteKeys) != 0 { + t.Fatalf("planned write keys = %#v, want none", entry.PlannedWriteKeys) + } + if len(entry.MissingRequiredKeys) != 0 { + t.Fatalf("missing required = %#v, want none without staged writes", entry.MissingRequiredKeys) + } + }) +} + +func TestPlanClearsOptionalRefsInStableOrder(t *testing.T) { + previous := planningAPIKeyProfile("work", config.LLMProviderAnthropic) + previous.ReviewerCredentials = &config.ReviewerCredentials{ + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/work-reviewer"}, + } + desired := planningBasicProfile("work") + + entries, err := Plan(&previous, desired, nil) + if err != nil { + t.Fatalf("Plan: %v", err) + } + + var cleared []string + for _, entry := range entries { + if entry.State == PlanStateClearRef { + cleared = append(cleared, entry.Ref.Purpose) + } + } + if !reflect.DeepEqual(cleared, []string{"reviewer_credentials", "llm"}) { + t.Fatalf("cleared purposes = %#v, want reviewer then llm", cleared) + } +} + +func TestGroupWritesByStoreSeparatesStores(t *testing.T) { + personal := ResolvedSecretsStore{ID: "personal-memory", Backend: string(credstore.BackendMemory), Source: config.EffectiveSecretsStoreSourceConfigured} + work := ResolvedSecretsStore{ID: "work-file", Backend: string(credstore.BackendFile), Source: config.EffectiveSecretsStoreSourceConfigured} + writes := map[string]map[string]string{ + "codereview/home": {GitTokenKey: "home-token"}, + "codereview/work": {GitTokenKey: "work-token"}, + } + + groups, err := GroupWritesByStore([]PlanEntry{ + {Ref: config.CredentialRef{Ref: "codereview/home"}, SecretsStore: personal}, + {Ref: config.CredentialRef{Ref: "codereview/work"}, SecretsStore: work}, + }, writes, map[string]bool{"codereview/work": true}) + if err != nil { + t.Fatalf("GroupWritesByStore: %v", err) + } + if len(groups) != 2 || groups[0].Resolved != personal || groups[1].Resolved != work { + t.Fatalf("groups = %#v, want deterministic personal then work groups", groups) + } + if !reflect.DeepEqual(groups[0].Writes, map[string]map[string]string{"codereview/home": writes["codereview/home"]}) { + t.Fatalf("personal writes = %#v", groups[0].Writes) + } + if !groups[1].OverwriteRefs["codereview/work"] { + t.Fatalf("work overwrite refs = %#v, want work", groups[1].OverwriteRefs) + } +} + +func TestGroupWritesByStoreKeepsSameCredentialNameInDifferentStoresSeparate(t *testing.T) { + ref := "codereview/shared" + writes := map[string]map[string]string{ref: {GitTokenKey: "shared-token"}} + groups, err := GroupWritesByStore([]PlanEntry{ + {Ref: config.CredentialRef{Ref: ref}, SecretsStore: ResolvedSecretsStore{ID: "personal", Backend: "memory", Source: config.EffectiveSecretsStoreSourceConfigured}}, + {Ref: config.CredentialRef{Ref: ref}, SecretsStore: ResolvedSecretsStore{ID: "work", Backend: "file", Source: config.EffectiveSecretsStoreSourceConfigured}}, + }, writes, nil) + if err != nil { + t.Fatalf("GroupWritesByStore: %v", err) + } + if len(groups) != 2 || groups[0].Resolved.ID != "personal" || groups[1].Resolved.ID != "work" { + t.Fatalf("groups = %#v, want same ref grouped once per store", groups) + } + for _, group := range groups { + if !reflect.DeepEqual(group.Writes[ref], writes[ref]) { + t.Fatalf("group %q writes = %#v, want shared bundle characterization", group.Resolved.ID, group.Writes) + } + } +} + +func TestGroupStaleReviewerCredentialCleanupsByStoreProtectsSharedRefAcrossProfiles(t *testing.T) { + appProfile := planningBasicProfile("app") + appProfile.ReviewerCredentials = &config.ReviewerCredentials{ + AuthMode: config.GitAuthModeGitHubApp, + GitHubApp: &config.GitHubAppConfig{AppID: "12345"}, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/shared-reviewer"}, + } + patProfile := planningBasicProfile("pat") + patProfile.ReviewerCredentials = &config.ReviewerCredentials{ + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/shared-reviewer"}, + } + cfg := config.File{Profiles: map[string]config.Profile{"app": appProfile, "pat": patProfile}} + resolved, err := ResolveSecretsStoreForProfile(cfg, appProfile) + if err != nil { + t.Fatalf("ResolveSecretsStoreForProfile: %v", err) + } + entry := PlanEntry{ + Ref: config.CredentialRef{Purpose: "reviewer_credentials", Ref: "codereview/shared-reviewer", Mode: string(config.GitAuthModeGitHubApp)}, + PreviousRef: &config.CredentialRef{Purpose: "reviewer_credentials", Ref: "codereview/shared-reviewer", Mode: string(config.GitAuthModePAT)}, + SecretsStore: resolved, + KeySpecs: []KeySpec{{Key: GitHubAppPrivateKeyKey, Required: true}}, + State: PlanStateKeepExisting, + } + + groups, err := GroupStaleReviewerCredentialCleanupsByStore(cfg, []PlanEntry{entry}) + if err != nil { + t.Fatalf("GroupStaleReviewerCredentialCleanupsByStore: %v", err) + } + if len(groups) != 1 || len(groups[0].Entries["codereview/shared-reviewer"]) != 2 { + t.Fatalf("groups = %#v, want both active shared-ref modes", groups) + } + if got, want := StaleReviewerCredentialKeys(groups[0].Entries["codereview/shared-reviewer"]), []string{GitHubAppIDKey, GitHubAppInstallationIDKey}; !reflect.DeepEqual(got, want) { + t.Fatalf("stale keys = %#v, want %#v", got, want) + } +} + +func TestGroupStaleReviewerCredentialCleanupsByStoreSeparatesConfiguredStores(t *testing.T) { + cfg := config.File{Secrets: config.SecretsConfig{Stores: map[string]config.SecretsStore{ + "personal": {Backend: config.SecretsStoreBackend{Kind: config.SecretsBackendKind(credstore.BackendMemory)}}, + "work": {Backend: config.SecretsStoreBackend{Kind: config.SecretsBackendKind(credstore.BackendFile)}}, + }}} + entries := make([]PlanEntry, 0, 2) + for _, storeID := range []string{"personal", "work"} { + resolved, err := ResolveCredentialStore(cfg, storeID) + if err != nil { + t.Fatalf("ResolveCredentialStore(%s): %v", storeID, err) + } + entries = append(entries, PlanEntry{ + Ref: config.CredentialRef{Purpose: "reviewer_credentials", Store: storeID, Ref: "codereview/shared", Mode: string(config.GitAuthModeGitHubApp)}, + PreviousRef: &config.CredentialRef{Purpose: "reviewer_credentials", Store: storeID, Ref: "codereview/shared", Mode: string(config.GitAuthModePAT)}, + SecretsStore: resolved, + KeySpecs: []KeySpec{{Key: GitHubAppPrivateKeyKey, Required: true}}, + State: PlanStateKeepExisting, + }) + } + + groups, err := GroupStaleReviewerCredentialCleanupsByStore(config.File{}, entries) + if err != nil { + t.Fatalf("GroupStaleReviewerCredentialCleanupsByStore: %v", err) + } + if len(groups) != 2 || groups[0].Resolved.ID != "personal" || groups[1].Resolved.ID != "work" { + t.Fatalf("groups = %#v, want one cleanup group per configured store", groups) + } +} + +func TestResolvedSecretsStoreIdentityIsComparableWithoutStringEncoding(t *testing.T) { + a := ResolvedSecretsStore{ID: "a|b", Backend: "c"}.Identity() + b := ResolvedSecretsStore{ID: "a", Backend: "b|c"}.Identity() + stores := map[StoreIdentity]bool{a: true, b: true} + if len(stores) != 2 { + t.Fatalf("store identities collided: %#v", stores) + } +} + +func planningBasicProfile(profile string) config.Profile { + ref, err := FormatRef(profile) + if err != nil { + panic(err) + } + return config.Profile{ + Git: config.GitConfig{ + Host: "github.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: ref}, + }, + LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + }, + } +} + +func planningAPIKeyProfile(profile string, provider config.LLMProvider) config.Profile { + p := planningBasicProfile(profile) + p.LLM = config.LLMConfig{ + Provider: provider, + Auth: config.LLMAuthAPIKey, + Adapter: config.LLMAdapterAnthropicAPI, + Credential: config.CredentialLocation{Store: config.LocalOSCredentialStoreID, Name: "codereview/" + profile + "-llm"}, + } + if provider == config.LLMProviderOpenAI { + p.LLM.Adapter = config.LLMAdapterOpenAIAPI + } + return p +}