From 2ef6d8b959b181465f5121448b337152dcf9c5b4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 20 Jun 2026 16:13:55 -0400 Subject: [PATCH 01/35] Add explicit credential storage schema Closes #356 --- docs/init-config-surface.md | 47 +- internal/cmd/credentialcmd/credentialcmd.go | 2 +- .../cmd/credentialcmd/credentialcmd_test.go | 347 +++-------- internal/cmd/reviewcmd/reviewcmd_test.go | 12 + internal/config/config.go | 547 +++++++++++------- internal/config/config_test.go | 512 +++++++++------- internal/credentials/credentials_test.go | 10 +- 7 files changed, 758 insertions(+), 719 deletions(-) diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 7bb578d3..3661ac50 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -42,20 +42,7 @@ intentionally unsupported. | Path | Interactive handling | Scripted owner | Defaults and second-run prepopulation | Mutation semantics | Helper/command owner and expected tests | |------|----------------------|----------------|---------------------------------------|--------------------|-----------------------------------------| | config.default_profile | Core profile wizard lets the user preserve, set to the selected profile, or choose another existing profile. | Existing `cr config default set`; `cr init --set-default` owns the scripted "make this profile default" case during init. | First init sets the created profile as default. Existing value is pre-populated and preserved by default. | Overwrite only to an existing profile. Profile rename updates this field when it points at the renamed profile. | #177 shared profile rename/default helper. #180 tests create, preserve, set, and rename updates. | -| config.keyring.backend | Configure secrets management now keeps `keyring.backend` only as an explicit legacy-compatibility row inside the secrets-management workflow. | `cr init --keyring-backend` and `--reset-keyring-backend` remain the scripted durable surface. Existing global `--backend` stays runtime-only and still conflicts with persisted defaults. | Empty means backend auto/env selection. Existing value is pre-populated in the legacy compatibility editor. | Preserve by leaving the compatibility row unchanged. Overwrite validates through credential backend parsing. Clearing the selection resets the field to auto/env behavior. Persistent `op`, `op-connect`, and `op-desktop` values remain rejected here; 1Password must be configured through a named secrets-management profile instead. | Existing keyring validation plus #184/#187 tests cover set, reset, backend conflict, legacy runtime persistence, and unrelated config preservation. #297 adds the interactive legacy-compatibility row. | -| config.secrets.default_profile | Secrets-management workflow can stage or clear the configured default named secrets-management profile while creating or editing a profile. | `cr config secrets-profile default get/set/unset`. Not owned by legacy `keyring.backend`. | Empty means there is no configured default named secrets-management profile. Legacy configs instead project a read-only `legacy-default` compatibility profile in presentation/helpers. Existing default state is pre-populated in the editor. | Overwrite only to an existing named secrets-management profile id. `legacy-default` is reserved/projected and cannot be set. Global runtime `--backend` still conflicts with a configured named default during the current init invocation. | #293 establishes schema, validation, and compatibility projection. #294 adds scripted default get/set/unset. #297 adds interactive staging. | -| config.secrets.profiles..label | Secrets-management workflow can create and edit human-friendly secrets-management profile labels directly. | `cr config secrets-profile set --label/--clear-label`. | Optional. Existing values are pre-populated. New profiles start from a backend-derived default label so the UI never opens on a blank unknown item. | Preserve on skip. Overwrite trims whitespace. Must remain a single line. Duplicate labels are allowed; stable ids remain authoritative and are deconflicted separately for new profiles by slugging the chosen label and appending `-2`, `-3`, and so on when needed. | #293 establishes schema/validation plus safe display summaries. #294 adds scripted create/update/clear-label behavior. #297 adds flat interactive editing. | -| config.secrets.profiles..backend.kind | Secrets-management workflow can create a named profile for any surfaced backend and edit an existing profile's durable backend choice inline. | `cr config secrets-profile set --backend`. | Required for an explicit named secrets-management profile. Existing values are pre-populated. Legacy configs with no explicit `secrets.profiles` continue to project `legacy-default` from `keyring.backend` or auto/env resolution without persisting a synthetic entry. | Preserve on skip. Overwrite validates against recognized credential-store backend names. Backend-specific 1Password metadata is edited in the same flat workflow and cleared automatically when switching away from 1Password. | #293 establishes schema/validation, projected legacy inventory, and config-show summaries. #294 adds scripted create/update/remove/default management. #297 adds interactive inventory and flat editing. | -| config.secrets.profiles..backend.onepassword.timeout | Secrets-management workflow edits the non-secret 1Password timeout inline whenever the selected backend kind needs it. | `cr config secrets-profile set --op-timeout`. | Empty input normalizes to the current service-account/desktop default timeout when that backend family requires one. Existing value is pre-populated. | Preserve on skip. Overwrite validates as a Go duration string. Switching away from 1Password clears the field from config. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.vault_id | Secrets-management workflow edits the non-secret 1Password vault selection inline. | `cr config secrets-profile set --op-vault-id`. | Required for `op`, `op-connect`, and `op-desktop`. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Switching away from 1Password clears the field from config. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.item_title_prefix | Secrets-management workflow edits the optional 1Password item naming override inline. | `cr config secrets-profile set --op-item-title-prefix`. | Empty means use the underlying keyring library default. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Switching away from 1Password clears the field from config. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.item_tag | Secrets-management workflow edits the optional 1Password item tag override inline. | `cr config secrets-profile set --op-item-tag`. | Empty means use the underlying keyring library default. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Switching away from 1Password clears the field from config. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.item_field_title | Secrets-management workflow edits the optional 1Password item-field title override inline. | `cr config secrets-profile set --op-item-field-title`. | Empty means use the underlying keyring library default. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Switching away from 1Password clears the field from config. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.connect_host | Secrets-management workflow edits the non-secret 1Password Connect endpoint inline when `op-connect` is selected. | `cr config secrets-profile set --op-connect-host`. | Required for `op-connect`. Ignored for other backend kinds. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Backend switches clear it when no longer relevant. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.connect_token_env | Secrets-management workflow edits the non-secret env-var name for the 1Password Connect token inline when `op-connect` is selected. | `cr config secrets-profile set --op-connect-token-env`. | Defaults to `OP_CONNECT_TOKEN` for `op-connect`. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Backend switches clear it when no longer relevant. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.service_token_env | Secrets-management workflow edits the non-secret env-var name for the 1Password service-account token inline when `op` is selected. | `cr config secrets-profile set --op-service-token-env`. | Defaults to `OP_SERVICE_ACCOUNT_TOKEN` for `op`. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Backend switches clear it when no longer relevant. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.secrets.profiles..backend.onepassword.desktop_account_id | Secrets-management workflow edits the optional explicit 1Password desktop account id inline when `op-desktop` is selected. | `cr config secrets-profile set --op-desktop-account-id`. | Optional for `op-desktop`; empty means the runtime may fall back to `OP_DESKTOP_ACCOUNT_ID`. Existing value is pre-populated. | Preserve on skip. Overwrite trims whitespace. Backend switches clear it when no longer relevant. | #296 adds schema, validation, runtime mapping, and command coverage. #297 adds interactive editing. | -| config.profiles..secrets_profile | Review-profile editing now exposes a selection-only `Secrets management` field that chooses an existing named secrets-management profile or clears back to the built-in/default runtime path. It must not configure new backends inline. | Manual YAML plus the interactive selector. Still not owned by legacy `keyring.backend`. | Empty means the review profile follows legacy/default runtime resolution: use `secrets.default_profile` when configured, otherwise fall back to legacy `keyring.backend`/env/auto behavior. Existing explicit values are pre-populated when valid. Missing/deleted refs are surfaced as repairable recovery selections during interactive init. | Overwrite only to an existing named secrets-management profile id. `legacy-default` is reserved/projected and must not be persisted directly. Clearing the field restores legacy/default runtime resolution rather than forcing a named profile. Interactive init may temporarily open configs with `ErrSecretsProfileNotFound` so the user can repair the dangling ref, but final validation/save still rejects unresolved missing refs. | #295 introduces the schema plus runtime credential routing through this field. #298 adds the user-facing selector and missing-reference recovery UX. | +| config.secrets.stores | Configure secrets storage creates and edits user-configured credential stores. The built-in OS credential store is projected as `local-os` and is not persisted here. | Interactive main-menu secrets-storage flow only. Scripted credential writes choose an existing store explicitly. | Omitted means only the built-in OS credential store is available. Existing stores are pre-populated in the inventory. | Store ids must not be `local-os`. Store metadata is non-secret. Creating, editing, and deleting configured stores never deletes credential values. | #356 establishes explicit stores and removes ambient/default credential-store selection. | | config.repository_profiles[] | Route editor opens directly with existing namespace and repo routes prefilled so the user can edit them in place or blank the field to remove all routes for the profile. | Existing `cr config route set` and `cr config route unset`; #186 documents route commands. #187 must not add a nested multi-route init grammar. | Empty or omitted means default-profile fallback. Existing routes are prefilled on later runs. | Leaving the prefilled text unchanged preserves routes. Mutations must converge through shared route helpers and preserve unrelated routes. Blanking the field removes the profile's routes. | #177 extracts route helpers. #185 tests list/edit/remove, route canonicalization, and preservation. | | config.repository_profiles[].profile | Route wizard selects the profile that a route resolves to. | Existing `cr config route set --profile ...`. | No default inside an entry; required when a route exists. Existing profile is pre-populated. | Overwrite only to an existing profile. Profile rename updates matching route entries. | #177 profile rename/route reference helper. #185 tests rename updates and invalid profile rejection. | | config.repository_profiles[].match.host | Route wizard derives from selected profile host or pasted PR URL. | Existing `cr config route set --host`. | Required for a route. Existing host is pre-populated and normalized. | Must match the target profile's `git.host`. Host edits on a profile with routes are blocked or reconciled by #185. | #177 route helper, #185 PR URL derivation and host/profile validation tests. | @@ -64,17 +51,20 @@ intentionally unsupported. | config.profiles. | Core profile wizard selects existing profile, creates a new profile, or renames an existing profile. | Existing global `--profile` with `cr init --non-interactive` owns scripted create/replace. Scripted rename is intentionally unsupported by init and belongs to future profile-management command design. | Empty global `--profile` means `default` during current init. Existing profile names are pre-populated. | Create requires a valid profile body. Rename preserves credential refs by default, updates default_profile and routes, and does not delete old keyring entries. | #177 profile rename helper. #180 tests create/select/rename and validation. | | config.profiles..git.host | Core profile wizard edits Git host. | Existing `cr init --git-host`; existing route commands own route-safe scripted changes. | Current init defaults to `github.com`. Existing value is pre-populated. | Set validates non-empty normalized host. If routes reference the profile and reconciliation is not selected, block or defer the edit. | #180 blocks/defer route-unsafe host edits. #185 implements reconciliation tests. | | config.profiles..git.auth_mode | Core profile wizard chooses `pat` or `github_app`; `oauth_device` remains reserved. | `cr init --git-auth-mode`, parallel to existing reviewer auth mode. Current init otherwise defaults user Git auth to PAT. | Existing value is pre-populated. New profile defaults to `pat`. | Overwrite only to supported v1 modes. Switching auth modes re-plans credential keys but does not delete old secrets. | #179 credential-ref/key-spec planner. #180 auth-mode prompt tests. | -| config.profiles..git.credential_ref | Credential-ref planner chooses ref for user Git auth before any secret ingress. | Existing `cr init --git-credential-ref`; `cr set-credential` writes secrets. | New profile defaults to `codereview/`. Existing ref is pre-populated and preserved by default. | Flattened interactive init shows the effective Git storage label inline. Leaving that field at the effective default means "follow the selected Git scope's default label" across later scope changes; entering a different value makes it a preserved custom override. Clear is invalid because Git credentials are required. | #179 planner tests default/custom refs and required state. #181 secret ingress tests. | +| config.profiles..git.credential.store | User Git auth chooses an existing credential store before any secret ingress. | Interactive credential-writing flows and scripted `cr set-credential --store`. | New profiles start with `local-os` selected. Existing store id is pre-populated. | Must be `local-os` or a configured store id. Store setup is not available inline from this flow. | #356 explicit destination tests. | +| config.profiles..git.credential.name | User Git auth names the credential bundle before any secret ingress. | Interactive credential-writing flows and scripted `cr set-credential --name`. | New profile defaults to `codereview/`. Existing name is pre-populated and preserved by default. | Must use the `codereview/` credential grammar. It must differ from other credentials in the same profile when the store also matches. | #356 explicit credential-location tests. | | config.profiles..git.identity_cache | Not shown as an editable init field. Preserve only. | Intentionally unsupported for init and config mutation. Runtime identity refresh owns it. | Existing cache is preserved. New profiles omit it. | Preserve unless a future explicit cache invalidation ticket owns behavior. Profile rename does not rewrite cache contents. | Preserve-only regression in #177/#180. | | config.profiles..reviewer_credentials | Optional reviewer credential section. Wizard supports skip, preserve, enable, edit, or clear reviewer config. | Existing `cr init --reviewer-credential-ref` and `--reviewer-auth-mode` own enable/edit. `cr init --disable-reviewer` owns scripted removal. | Omitted means posting uses Git credentials. Existing section is pre-populated. | Clear removes the whole section. Enable requires auth mode and credential ref. Ref must differ from Git and LLM refs. | #179 planner and #180 optional section tests. #181 secret ingress tests. | | config.profiles..reviewer_credentials.auth_mode | Reviewer wizard chooses PAT or GitHub App; `oauth_device` remains reserved. | Existing `cr init --reviewer-auth-mode`. | Current init defaults reviewer mode to `pat` when reviewer credentials are requested. Existing value is pre-populated. | Overwrite only to supported v1 modes. Switching modes re-plans key specs and preserves old secrets unless explicit overwrite/migration occurs. | #179 credential bundle tests for PAT and GitHub App. | -| config.profiles..reviewer_credentials.credential_ref | Credential-ref planner chooses reviewer Git ref. | Existing `cr init --reviewer-credential-ref`; `cr set-credential` writes secrets. | New reviewer section defaults to `codereview/-reviewer`, or `codereview/