Refactor pc config to support key-based configuration and commands#90
Refactor pc config to support key-based configuration and commands#90austin-denoble wants to merge 12 commits into
pc config to support key-based configuration and commands#90Conversation
…mands Replaces the per-setting subcommands (get-api-key, set-api-key, set-color, set-environment) with a generic, registry-driven interface modeled after `gh config` and `aws configure`. A new keyDescriptor in registry.go holds each setting's getter, setter, validator, side-effect hook, short/long descriptions, and sensitivity flag. Adding a new key is supported through a registry entry rather than a command file. Adds `pc config get|set|unset|list|describe <key>`, with --json and --reveal flag support for sensitive values like API keys. The four legacy commands are maintained as hidden, deprecated aliases to maintain functionality for existing users.
`pc config unset` now invokes the onChange hook when clearing a key changes its value, fixing a bug where `unset environment` on staging silently resets to production without clearing the staging-tied OAuth session, API key, or target org/project. `pc config set` now passes the canonical stored value to onChange via a post-set getStr() read, rather than the raw user input. Hooks like environment's now see "production" instead of "prod" when the input is normalized during setStr.
Extract business logic from cobra Run blocks into run* functions that accept a ConfigService interface, enabling unit testing without touching the real viper config store. Add --json output to set and unset commands for machine-readable use. Add a Hidden field to keyDescriptor to suppress internal keys (environment) from list output and tab completion while keeping them accessible by name.
… onChange: add specific fields to keyDescriptor for validateStr, persistStr, and onChange. make sure config set|unset call validateStr, onChange, and then persistStr to avoid onChange failing after persistence has already occurred
…ually checked in ValidateStr
…t reading from viper and bypassing any configured env setting
… state.AuthedUser appropriately if configuring an API key
- Output the normalized stored value rather than the raw user input in pc config set --json (e.g. true instead of on for color aliases) - Include onChange side-effect messages in JSON output for both set and unset so environment changes (logout, key/target clear) are visible to machine consumers, not silently dropped - Treat an unreadable OAuth token record as no active session in the environment onChange hook rather than blocking the operation entirely, unblocking users who authenticate via API key
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
There are 7 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4759118. Configure here.
| // Re-read the stored value so the output reflects what was actually | ||
| // persisted (e.g. "true" rather than the user-supplied alias "on"). | ||
| storedValue, _, _ := svc.Get(keyName) | ||
| fmt.Fprintln(os.Stdout, text.IndentJSON(setOutput{Key: keyName, Value: storedValue, Messages: lines})) |
There was a problem hiding this comment.
Set output ignores stored config
Medium Severity
runSetCmd treats ConfigService.Get as the persisted value for --json success output and for ErrNoChange messaging, but Get returns effective values (including PINECONE_* env overrides). Environment no-op detection uses GetStored, so users can see the wrong value—or a value that contradicts what they just set—when overrides are in play.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4759118. Configure here.
| }, | ||
| persistStr: func(value string) { | ||
| secrets.DefaultAPIKey.Set(value) | ||
| }, |
There was a problem hiding this comment.
Api-key unset ignores file value
Medium Severity
The new Unset path uses getStoredStr for environment so env overrides do not trigger a no-op incorrectly, but api-key has no getStoredStr and still compares secrets.DefaultAPIKey.Get() (which honors PINECONE_* env vars). pc config unset api-key can run onChange, report success, and rewrite auth context even when the secrets file is already empty and only an env var supplies the key.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4759118. Configure here.
| case "false", "off": | ||
| return "false", nil | ||
| default: | ||
| return "", fmt.Errorf("invalid value %q for color; must be one of: true, false, on, off", value) |
There was a problem hiding this comment.
Color set rejects one alias
Low Severity
The registry color validator accepts true, false, on, and off, but not 1, while the deprecated hidden set-color command still treats 1 as enabled. Users moving to pc config set color 1 get a validation error instead of the behavior the legacy alias still provides.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4759118. Configure here.
| lines = append(lines, fmt.Sprintf("You have been logged out; to login again, run %s", style.Code("pc login"))) | ||
| } else { | ||
| lines = append(lines, fmt.Sprintf("To login, run %s", style.Code("pc login"))) | ||
| } |
There was a problem hiding this comment.
OAuth may survive environment switch
Medium Severity
The environment onChange hook decides whether to call oauth.Logout() from the result of oauth.Token, but it discards that function’s error. oauth.Token can return an error while refresh or claim parsing fails even though OAuth tokens are still stored locally, so the hook skips Logout, continues the environment change, and leaves the previous session on disk.
Reviewed by Cursor Bugbot for commit 4759118. Configure here.


Problem
The
pc configcommand exposed configuration through a flat set of verb-noun subcommands —get-api-key,set-api-key,set-color,set-environment— with one Cobra command file per setting. The pattern had a few problems:listequivalent.gh,aws,npm,stripe) handle config, raising the learning curve for new users.This becomes more painful as the set of configurable values grows.
Solution
Refactor
pc configto a generic, registry-driven interface modeled aftergh config. A singlekeyDescriptorininternal/pkg/cli/command/config/registry.goholds each setting's getter, setter, clear function, optionalonChangeside-effect hook, short and long descriptions, sensitivity flag, and valid values. Theget/set/unset/list/describecommands are generic and dispatch through the registry — adding a new config key is now a single registry entry, no new command file required.New commands
Registry-driven extensibility
Each setting is one entry in
configRegistry. Adding a new key — say, a default output format — would look like:No new command file. No changes to
root.go's skip-auth list. It just works acrossget,set,unset,list, anddescribe.Side-effect hooks
The
environmentkey needs to clear OAuth state, the API key, and the target org/project when it changes. That used to live inline inset-environment. It now lives in anonChangehook on the registry entry and fires consistently from bothsetandunset:Flags
--jsononget,list,describefor machine-readable output.--revealonget,list,describeto unmask sensitive values (defaults to masking head/tail forapi-key).Backwards compatibility
The four legacy commands (
get-api-key,set-api-key,set-color,set-environment) are preserved as hidden, deprecated aliases. They continue to function and print Cobra's standard deprecation noticepointing at the new equivalent:
Existing scripts and docs keep working; new users land on the new pattern.
Type of Change
The legacy commands are kept as hidden aliases, so this is non-breaking for existing users. Docs referencing
pc config set-api-keyetc. should be updated to usepc config set api-key <value>going forward.Test Plan
just test-unitpassespc config list— confirms all three keys render with descriptionspc config get api-key— confirms<not set>placeholder for empty valuespc config get api-key --reveal— confirms full value shown when revealedpc config set environment staging— confirms OAuth/API key/target wipe firespc config unset environment(from staging) — confirms same wipe fires (regression fix)pc config set environment prod— confirms canonical "production" is stored and reportedpc config describe api-key/environment/color— confirms long description rendered when present, omitted when blankpc config get-api-key/set-api-key/set-color/set-environment— confirms each still works and prints deprecation notice--jsononget,list,describe— confirms structured outputNote
Medium Risk
Changes how API keys and environment are written and can clear OAuth, targets, and auth context; legacy commands still behave differently for API key auth until users migrate.
Overview
pc configis reworked around a centralconfigRegistryso oneget/set/unset/list/describeflow covers all keys (api-key,color,environment) instead of separate subcommands per setting. New commands support--jsonand--revealfor sensitive values;listanddescribeexpose descriptions, valid values, and long help text.Registry entries drive validation, persistence, and
onChangeside effects (e.g.environmentlogs out OAuth, clears API key and target org/project—now onsetandunset).api-keyset/unsetreconcilesAuthContextvia the registry; the oldset-api-keypath did not.ConfigProperty.GetStored()compares file-only values so env overrides do not block no-op detection.Legacy
get-api-key,set-api-key,set-color, andset-environmentremain as hidden, deprecated aliases. Root skip-auth lists the five new config subcommands. Minor copy fix in auth configure help (“API key”).Reviewed by Cursor Bugbot for commit 4759118. Bugbot is set up for automated code reviews on this repo. Configure here.