Cleanup and error flow refactor (PR 1 of 2)#26
Merged
Conversation
- Drop the unused AwsParam struct, its isValid() method, and the ParamType/String/SecureString/StringList constants that only existed to support isValid(). Nothing else referenced them. - Drop the empty setPrefixes() method on app.Config. No behavior change.
client.New(*app.Config) now returns (*EspClient, error). An
unsupported backend produces fmt.Errorf("unsupported backend %q",
c.Backend) instead of panicking. The lone caller in cmd/root.go
prints the error and exits 1.
The os.Exit at the call site is interim — commit 5 moves env-var
validation and client construction into PersistentPreRunE so the
error flows back through cobra.
Stop the side-effecting "fmt.Printf + os.Exit" pattern inside the SSM client and surface failures as returned errors: - Service.Init returns error from config.LoadDefaultConfig instead of os.Exit(1). - Service.Save / GetOne / GetMany / Copy / Delete return (..., error). New mapErr helper applies the per-action mapper and falls back to the raw error when the mapper returns nil (which previously caused unrecognized errors to be silently swallowed). - getNextParams propagates errors through recursion. (Replaced by the v2 paginator in a follow-up commit.) - handleAwsErr is deleted; internal/ssm/utils.go drops its fmt and os imports. The Client interface in internal/client/client.go is updated to match the new signatures; EspClient wrapper methods (GetParam, ListParams, Save, Delete, Copy, Move) propagate errors. New now also returns the Init error. cmd subcommands still use Run: in this commit and handle the new errors with fmt.Fprintln(os.Stderr, err) + os.Exit(1) — this scaffolding is replaced by RunE / return err in the next commit.
Flip every subcommand (copy, delete, get, list, move, put, init, version) from Run to RunE returning error. First line of every RunE body is cmd.SilenceUsage = true so cobra prints usage on flag/arg misuse (which fires before RunE) but not on runtime errors from the body. Sweeps away the temporary fmt.Fprintln(stderr, err) + os.Exit(1) scaffolding from the previous commit — those become return err. The args-empty checks in cmd/copy.go that previously printed and called os.Exit become return errors.New(...) per the spec's bug audit. External behavior on the happy path is unchanged. On errors, cobra now prefixes the message with "Error:" before printing — small UX shift accepted in the design.
The legacy initConfig hook ran via cobra.OnInitialize and called
os.Exit on env-var failures (exit 1 for missing AWS_DEFAULT_REGION,
2 for missing AWS_PROFILE, 3 for MarkFlagRequired failure).
Split that into:
- configureLogging (cobra.OnInitialize): just slog setup. No I/O,
no failure path.
- persistentPreRun (rootCmd.PersistentPreRunE): env-var checks,
client.New, viper config read, MarkFlagRequired("env"). All
failures return error so cobra surfaces them as
"Error: <message>" on stderr; Execute() exits 1.
PersistentPreRunE is wired in init() rather than the rootCmd
literal to avoid an initialization cycle (rootCmd ↔ persistentPreRun).
Cobra short-circuits before PersistentPreRunE for --help, so help
output still renders without AWS credentials. Exit code on env-var
failure collapses from {1,2,3} to a uniform 1, matching the spec.
Replace the manual NextToken recursion in GetMany / getNextParams
with awsssm.NewGetParametersByPathPaginator and a standard
for paginator.HasMorePages() { paginator.NextPage(...) } loop.
Cuts the iteration logic in half and uses the v2-idiomatic shape.
External behavior unchanged on the happy path; error handling
still flows through mapErr(GetMany, err).
Reflect the PR 1 refactor in the architecture overview:
- AWS_DEFAULT_REGION / AWS_PROFILE checks now live in
PersistentPreRunE; failures return error → cobra prints
"Error: <msg>" → Execute() exits 1. Note the exit-code
collapse from {1,2} to a uniform 1.
- client.New now returns (*EspClient, error) on unsupported
backend instead of panicking; Client interface methods
return (value, error).
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements PR 1 of the cleanup-and-tests design (
docs/superpowers/specs/2026-05-05-cleanup-and-tests-design.md). Removes dead code and migrates the codebase from ad-hocos.Exit/panicerror handling to idiomatic cobra error flow. PR 2 (pure-logic tests) follows after this merges.Commits (in order)
chore: remove dead code (AwsParam, setPrefixes)— drops unreferenced struct, method, and supportingParamTypeconstants.refactor: client.New returns error instead of panic— unsupported backend now returnsfmt.Errorf("unsupported backend %q", c.Backend).refactor: ssm methods return errors; remove handleAwsErr—Init/Save/GetOne/GetMany/Copy/Deleteall return(value, error). AddsmapErrhelper that falls back to the raw error when the AWS-error mapper returns nil (was previously a silent-swallow bug).client.Clientinterface updated to match.refactor: cmd subcommands use RunE with SilenceUsage— every subcommand flipsRun→RunE, setscmd.SilenceUsage = trueas the first line, and propagates errors viareturn err. The args-empty checks incmd/copy.gobecomeerrors.New(...)per the bug-audit note.refactor: env-var validation moves to PersistentPreRunE—AWS_DEFAULT_REGION/AWS_PROFILEchecks,client.Newconstruction,viper.ReadInConfig, andMarkFlagRequired("env")move intorootCmd.PersistentPreRunE.slogconfig stays incobra.OnInitialize.Execute()exits 1 on any error.refactor: use ssm paginator for GetParametersByPath— replaces the recursivegetNextParamswithawsssm.NewGetParametersByPathPaginatorand the standardfor paginator.HasMorePages()loop.chore: update CLAUDE.md error-flow notes— reflects thePersistentPreRunEflow, the(value, error)interface, and the exit-code collapse.External behavior changes (per spec)
AWS_DEFAULT_REGION/AWS_PROFILEcollapses from1/2to a uniform1.MarkFlagRequiredfailure also collapses from3to1. (These codes were undocumented and look incidental.)Error:on stderr, replacing the barefmt.Printlnstyle. Usage is suppressed for runtime errors viaSilenceUsage.esp --helpcontinues to work without AWS env vars (cobra short-circuits beforePersistentPreRunE, same as it did beforeOnInitialize).Test Plan
go build ./...cleango vet ./...cleango test ./...— all packages reportokor[no test files]esp --helpprints usage with no AWS env vars set, exit 0esp versionwith noAWS_DEFAULT_REGION→Error: AWS_DEFAULT_REGION environment variable is not set, exit 1esp versionwithAWS_DEFAULT_REGIONset but noAWS_PROFILE→Error: AWS_PROFILE environment variable is not set, exit 1