Assorted fixes: SilenceErrors, input validation, tests, and hardening

CLAUDE.md

@@ -1,5 +1,12 @@
 # Code Review & Improvement Plan for imscli
 
+## Preferences
+
+- Do not include "Generated with Claude Code" or similar attribution lines in commits or PRs.
+- Always include file path and line numbers when referencing code (e.g., `ims/config.go:34`).
+
+---
+
 ## Summary
 
 Full review of all Go source files in the `imscli` project — a CLI tool for Adobe's
@@ -46,22 +53,6 @@ return parsedURL.Scheme != "" && parsedURL.Host != ""
 
 ## 3. Structural Improvements
 
-### 3.1 Extract IMS client creation into a helper
-
-Every method in the `ims` package repeats:
-
-```go
-httpClient, err := i.httpClient()
-// ...
-c, err := ims.NewClient(&ims.ClientConfig{URL: i.URL, Client: httpClient})
-```
-
-This 8-line boilerplate appears in ~10 files. Extract into:
-
-```go
-func (i Config) newIMSClient() (*ims.Client, error) { ... }
-```
-
 ### 3.2 Deduplicate validate subcommands
 
 All four files in `cmd/validate/` have identical `RunE` logic — only the flag binding
@@ -94,13 +85,6 @@ The private key is read into a `[]byte` and passed to `ExchangeJWT`, but the byt
 slice is never zeroed after use. Key material persists in memory until GC. Should
 add `defer func() { for i := range key { key[i] = 0 } }()`.
 
-#### 6.1.8 No mutual exclusion for token fields in validate/invalidate
-
-**Files:** `ims/validate.go:26-48`, `ims/invalidate.go:23-48`
-
-If multiple token fields are populated simultaneously, only the first match in the
-switch wins silently. No validation checks that exactly one token is provided.
-
 #### 6.1.12 PKCE flag mutation persists on shared Config pointer
 
 **File:** `cmd/authz/pkce.go:30`
@@ -114,25 +98,10 @@ switch wins silently. No validation checks that exactly one token is provided.
 Set in `jwt_exchange.go:65`, `exchange.go:73`, `refresh.go:67` with wrong calculations,
 but no caller ever reads the field. Dead code with active bugs.
 
-#### 6.2.3 Missing input validation in `AuthorizeService` and `AuthorizeClientCredentials`
-
-`ims/authz_service.go` and `ims/authz_client.go` are the only two operation methods
-without a `validateXxxConfig()` function.
-
-#### 6.2.5 `browser.Stdout = nil` has global side effects
-
-**File:** `ims/authz_user.go:123`
-
-Modifies a package-level variable in `pkg/browser`. Should save/restore the original.
-
 #### 6.2.6 Inconsistent flag shorthands across commands
 
 `-s` and `-a` mean different things in different commands.
 
-#### 6.2.8 `cmd/profile.go:41` — default API version outdated
-
-Profile API version defaults to `"v1"` but latest is `"v3"`.
-
 #### 6.2.9 Hardcoded serviceCode whitelist in profile decoding
 
 **File:** `ims/profile.go:108-111`
@@ -155,18 +124,11 @@ Hardcoded `v1`-`v6` whitelist requires code changes for new versions.
 
 | Finding | File | Description |
 |---------|------|-------------|
-| No test for unicode/special chars | `cmd/pretty/json_test.go` | Missing edge case coverage |
 | `RawURLEncoding` rejects padded base64 | `ims/decode.go:53` | Some JWT impls include `=` padding |
-| No JWE support (5-part tokens) | `ims/decode.go:43` | Only 3-part JWTs supported |
-| Gzip `io.Copy(io.Discard)` unnecessary | `ims/profile.go:147` | JSON decoder already consumed stream |
 | `PersistentPreRunE` can be overridden | `cmd/root.go:31` | If a subcommand defines its own, parent's is lost |
 | Duplicate port defaults (cobra + const) | `cmd/authz/user.go:44` + `ims/authz_user.go:27` | Maintenance risk |
 | `cmd/refresh.go:36-39` uses `map[string]interface{}` | `cmd/refresh.go` | No guaranteed field order in JSON; use struct |
-| `viper.Unmarshal` ignores unknown keys | `cmd/params.go:69` | Config typos silently ignored |
-| Listener not closed on panic path | `ims/authz_user.go:112-130` | Missing `defer listener.Close()` |
 | Cascading/ClientSecret unconditionally sent | `ims/invalidate.go:91-97` | Should be conditional on token type |
-| `version` variable has no default | `main.go:22` | Shows empty if ldflags not set |
-| Double error printing from cobra | `main.go:27-29` | Root cmd doesn't SilenceErrors; leaf cmds do |
 
 ---
 
@@ -178,17 +140,13 @@ Hardcoded `v1`-`v6` whitelist requires code changes for new versions.
 |---|--------|-------|
 | 33 | Deduplicate validate subcommands with factory | `cmd/validate/*.go` |
 | 34 | Deduplicate invalidate subcommands with factory | `cmd/invalidate/*.go` |
-| 38 | Remove empty `internal/output/` directory | directory |
 | 40 | Merge `pkce.go`/`user.go` into factory | `cmd/authz/` |
-| 42 | Update default profile API version to v3 | `cmd/profile.go` |
 
 ### Testing
 
 | # | Change | Files |
 |---|--------|-------|
-| 43 | Add unit tests for all validators | `ims/*_test.go` (new) |
-| 44 | Add unicode/special char tests for pretty | `cmd/pretty/json_test.go` |
-| 46 | Add integration tests for flag/config/env precedence | `cmd/*_test.go` (new) |
+| 46 | Integration tests for flag/config/env precedence (requires mock IMS server) | `cmd/*_test.go` (new) |
 
 ---
 
@@ -231,6 +189,18 @@ The following items have been implemented and merged:
 - **N3** Fix "decodification" → "decoding" (#69)
 - **2.1** Rename `ProfileApiVersion` → `ProfileAPIVersion`, `OrgsApiVersion` → `OrgsAPIVersion` (#70)
 - **2.5** Replace C-style `/* */` block comments with `//` line comments (#70)
+- **3.1** Extract `newIMSClient()` helper, deduplicate 13 call sites (#70)
+- **N17** Move `SilenceErrors` to root command, remove from 21 leaf commands (#70)
+- **6.2.3** Add input validation for `AuthorizeService` and `AuthorizeClientCredentials` (#70)
+- **6.2.5** Save/restore `browser.Stdout` around `OpenURL` call (#70)
+- **N16** Default `version` variable to `"dev"` for local builds (#70)
+- **N14** Add `defer listener.Close()` in OAuth flow (#70)
+- **43** Add unit tests for all validators (`ims/config_test.go`) (#70)
+- **44** Add unicode/special char tests for pretty (`cmd/pretty/json_test.go`) (#70)
+- **38** Remove empty `internal/output/` directory — already gone after #68 refactor
+- **45** Parallel test safety for `output/output_test.go` — resolved by #68 (output package removed, `cmd/pretty` tests a pure function)
+- **N15** Unicode/special char tests — completed as part of item 44 (#70)
+- **6.1.8** Reject multiple tokens in `resolveToken()` (`ims/config.go:67`) (#70)
 
 ### Skipped (not applicable)
 
@@ -239,3 +209,7 @@ The following items have been implemented and merged:
 - **6.1.10** Default metascopes — covered by 1.9
 - **3.8** `MarkFlagRequired` — incompatible with viper config/env workflow
 - **6.1.13** Missing Config `String()` — Config is never printed in the CLI
+- **N8** No JWE support — decode command is for plain JWTs, JWE is a different feature
+- **N9** Gzip `io.Copy(io.Discard)` — intentionally kept as an example pattern
+- **N13** `viper.Unmarshal` ignoring unknown keys — `UnmarshalExact` would break forward compatibility
+- **6.2.8 / 42** Default profile API version v1 — intentional for backward compatibility

README.md

@@ -34,6 +34,12 @@ imscli authorize user help
 
 The complete documentation of the project is available in the [DOCUMENTATION.md](DOCUMENTATION.md) file.
 
+## Development Notes
+
+### PersistentPreRunE and subcommands
+
+The root command defines a `PersistentPreRunE` that loads configuration from flags, environment variables, and config files (see `cmd/root.go`). In cobra, if a subcommand defines its own `PersistentPreRunE`, it **overrides** the parent's — the root's `PersistentPreRunE` will not run for that subcommand or its children. If you need to add a `PersistentPreRunE` to a subcommand, you must explicitly call the parent's first.
+
 ## Contributing
 
 Contributions are welcomed! Read the [Contributing Guide](CONTRIBUTING.md) for more information.

cmd/admin/organizations.go

@@ -27,7 +27,7 @@ func OrganizationsCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:    "Requests the specified user organizations using the admin API and a service token.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.GetAdminOrganizations()
 			if err != nil {
@@ -38,7 +38,7 @@ func OrganizationsCmd(imsConfig *ims.Config) *cobra.Command {
 		},
 	}
 	cmd.Flags().StringVarP(&imsConfig.Guid, "guid", "g", "", "User ID.")
-	cmd.Flags().StringVarP(&imsConfig.AuthSrc, "authSrc", "s", "", "Authorization source.")
+	cmd.Flags().StringVarP(&imsConfig.AuthSrc, "authSrc", "A", "", "Authorization source.")
 	cmd.Flags().StringVarP(&imsConfig.ClientID, "clientID", "c", "", "IMS client ID.")
 	cmd.Flags().StringVarP(&imsConfig.ServiceToken, "serviceToken", "t", "", "Service token.")
 	cmd.Flags().StringVarP(&imsConfig.OrgsAPIVersion, "orgsApiVersion", "a", "v5", "Admin organizations API version.")

cmd/admin/profile.go

@@ -25,7 +25,7 @@ func ProfileCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:  "Requests the specified user profile using the admin API and a service token.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.GetAdminProfile()
 			if err != nil {
@@ -36,7 +36,7 @@ func ProfileCmd(imsConfig *ims.Config) *cobra.Command {
 		},
 	}
 	cmd.Flags().StringVarP(&imsConfig.Guid, "guid", "g", "", "User ID.")
-	cmd.Flags().StringVarP(&imsConfig.AuthSrc, "authSrc", "s", "", "Authorization source.")
+	cmd.Flags().StringVarP(&imsConfig.AuthSrc, "authSrc", "A", "", "Authorization source.")
 	cmd.Flags().StringVarP(&imsConfig.ClientID, "clientID", "c", "", "IMS client ID.")
 	cmd.Flags().StringVarP(&imsConfig.ServiceToken, "serviceToken", "t", "", "Service token.")
 	cmd.Flags().StringVarP(&imsConfig.ProfileAPIVersion, "profileApiVersion", "a", "v1", "Admin profile API version.")

cmd/authz/client.go

@@ -25,7 +25,7 @@ func ClientCredentialsCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:    "Perform the 'Client Credentials Authorization Flow' to negotiate an access token for a service.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.AuthorizeClientCredentials()
 			if err != nil {

cmd/authz/jwt.go

@@ -24,7 +24,7 @@ func JWTCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:  "Perform the 'Assertion Grant Type Flow' to request a token.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.AuthorizeJWTExchange()
 			if err != nil {
@@ -36,9 +36,9 @@ func JWTCmd(imsConfig *ims.Config) *cobra.Command {
 	}
 
 	cmd.Flags().StringVarP(&imsConfig.ClientID, "clientID", "c", "", "IMS Client ID.")
-	cmd.Flags().StringVarP(&imsConfig.ClientSecret, "clientSecret", "s", "", "IMS Client secret.")
+	cmd.Flags().StringVarP(&imsConfig.ClientSecret, "clientSecret", "p", "", "IMS Client secret.")
 	cmd.Flags().StringVarP(&imsConfig.Organization, "organization", "o", "", "IMS Organization.")
-	cmd.Flags().StringVarP(&imsConfig.Account, "account", "a", "", "Technical Account ID.")
+	cmd.Flags().StringVarP(&imsConfig.Account, "account", "A", "", "Technical Account ID.")
 	cmd.Flags().StringVarP(&imsConfig.PrivateKeyPath, "privateKey", "k", "", "Private Key file.")
 	cmd.Flags().StringSliceVarP(&imsConfig.Metascopes, "metascopes", "m", []string{}, "Metascopes to request.")
 

cmd/authz/pkce.go

@@ -26,9 +26,8 @@ func UserPkceCmd(imsConfig *ims.Config) *cobra.Command {
 			"returned IMS token. Public and private clients are supported.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
-			imsConfig.PKCE = true
-			resp, err := imsConfig.AuthorizeUser()
+
+			resp, err := imsConfig.AuthorizeUserPKCE()
 			if err != nil {
 				return fmt.Errorf("error in user authorization: %w", err)
 			}

cmd/authz/service.go

@@ -24,7 +24,7 @@ func ServiceCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:  "Perform the 'Authorization Code Exchange' to negotiate an access token for a service.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.AuthorizeService()
 			if err != nil {
@@ -37,7 +37,7 @@ func ServiceCmd(imsConfig *ims.Config) *cobra.Command {
 
 	cmd.Flags().StringVarP(&imsConfig.ClientID, "clientID", "c", "", "IMS client ID.")
 	cmd.Flags().StringVarP(&imsConfig.ClientSecret, "clientSecret", "p", "", "IMS client secret.")
-	cmd.Flags().StringVarP(&imsConfig.AuthorizationCode, "authorizationCode", "a", "", "Permanent authorization code.")
+	cmd.Flags().StringVarP(&imsConfig.AuthorizationCode, "authorizationCode", "x", "", "Permanent authorization code.")
 
 	return cmd
 }

cmd/authz/user.go

@@ -26,7 +26,7 @@ func UserCmd(imsConfig *ims.Config) *cobra.Command {
 			"IMS token. Without PKCE only private clients are supported.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.AuthorizeUser()
 			if err != nil {

cmd/dcr.go

@@ -26,7 +26,7 @@ func registerCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:  `Register a new OAuth client using Dynamic Client Registration.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.Register()
 			if err != nil {
@@ -39,7 +39,6 @@ func registerCmd(imsConfig *ims.Config) *cobra.Command {
 		},
 	}
 
-	cmd.Flags().StringVarP(&imsConfig.RegisterURL, "url", "u", "", "Registration endpoint URL.")
 	cmd.Flags().StringVarP(&imsConfig.ClientName, "clientName", "n", "", "Client application name.")
 	cmd.Flags().StringSliceVarP(&imsConfig.RedirectURIs, "redirectURIs", "r", []string{}, "Redirect URIs (comma-separated or multiple flags).")
 

cmd/decode.go

@@ -26,7 +26,7 @@ func decodeCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:    "Decode a JWT token and display the header and payload as prettified JSON.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			decoded, err := imsConfig.DecodeToken()
 			if err != nil {

cmd/exchange.go

@@ -25,7 +25,7 @@ func exchangeCmd(imsConfig *ims.Config) *cobra.Command {
 		Long:    "Perform the 'Cluster Access Token Exchange Grant' to request a new token with a new user ID or IMS Org ID.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
-			cmd.SilenceErrors = true
+
 
 			resp, err := imsConfig.ClusterExchange()
 			if err != nil {