chore(pkg/cmd): medium/low findings from the 2026-07-28 full review #1613

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Medium and low findings from the 2026-07-28 full review of cmd/ (all 40 files, non-test read in full) and the assigned pkg/ packages (common, concurrency, config, errors, logging, provider, retry, scorer). Everything was read via git show origin/main:<path> and git grep origin/main; the shared working tree was not touched.

The critical and high findings from the same review are filed separately. Two further medium findings were posted as comments on existing issues instead: CSV numeric parsing on #1327, and CSV Term / PaymentOption validation on #379.

Scope note: there is no MCP server in this repository at origin/main (git ls-tree -r --name-only origin/main | grep -i mcp is empty), so the MCP stdout-purity item from the review brief had no target here and was not performed.

Each item below is independently actionable; they are grouped only to keep the backlog readable.

• MEDIUM: determineCSVCoverage treats an explicit --coverage 80 as "not set" and silently buys 100%

  Where: cmd/multi_service_csv.go:18-26, called from cmd/multi_service.go:315.

  What: the function compares cfg.Coverage == 80.0 against the flag's default literal to infer "the user did not override". A user who explicitly types the default value is indistinguishable from one who typed nothing. The codebase already knows the correct idiom and uses it a few files over: validateTargetCoverage detects an explicit --coverage via cmd.Flags().Changed("coverage"), and its comment at cmd/validators.go:113-116 spells out this exact hazard ("so a user who happens to set --coverage 80 (which equals the default) still sees the notice").

  Failure scenario: cudly --input-csv approved.csv --coverage 80 --purchase. The operator intends to buy 80% of each CSV row's count. determineCSVCoverage returns 100.0, filterAndAdjustRecommendations skips sizing entirely (cmd/multi_service.go:463, since TargetCoverage == 0 and csvModeCoverage < 100 is false), and 100% of every row is purchased. The log line at cmd/multi_service.go:469 never prints, so there is no on-screen hint that the flag was ignored.

  Fix direction: thread the *cobra.Command, or a precomputed coverageExplicitlySet bool, into determineCSVCoverage and branch on Flags().Changed("coverage").

• MEDIUM: on the CSV path, account filters are applied before AccountName is populated

  Where: ordering inside runToolFromCSV: filterAndAdjustRecommendations (which calls applyFilters) at cmd/multi_service.go:328, populateAccountNames at cmd/multi_service.go:350. The filter is shouldIncludeAccount(rec.AccountName, cfg) (cmd/multi_service_filters.go:98, :194-213). The non-CSV path gets this right: populateRecommendationAccountNames runs at cmd/multi_service_helpers.go:597, one line before applyRegionFilters at :598.

  What: in CSV mode AccountName is only whatever the file's AccountName column held (cmd/multi_service_csv.go:110). The Organizations lookup that would fill it in from the Account ID runs 20 lines too late to matter.

  Failure scenario: a CSV exported with Account (12-digit ID) but no AccountName column, so every rec has AccountName == "". The operator runs --input-csv recs.csv --exclude-accounts sandbox --purchase. shouldIncludeAccount returns len(Include) == 0 && len(Exclude) == 0 (cmd/multi_service_filters.go:196-198), which is false for every row, so all recommendations are dropped and the run prints "No recommendations to process after filtering". The symmetric case is worse: with --include-accounts prod the same all-empty rows are also dropped, so an operator who believes they scoped the run to prod silently buys nothing and may re-run without the filter.

  Fix direction: move populateAccountNames(ctx, recs, accountCache) above filterAndAdjustRecommendations, which requires loading the AWS config first, or filter on rec.Account when rec.AccountName is empty.

• MEDIUM: shouldIncludeAccount drops every unnamed recommendation as soon as any account filter is set

  Where: cmd/multi_service_filters.go:194-198.

  What: if accountName == "" { return len(cfg.IncludeAccounts) == 0 && len(cfg.ExcludeAccounts) == 0 }. For an exclude-only filter the correct answer is "keep it", since an unnamed account cannot match an exclusion, but the code returns false and drops it.

  Failure scenario: a payer account's own Cost Explorer recommendations come back with an empty Account field, so populateRecommendationAccountNames leaves AccountName empty (cmd/multi_service_helpers.go:452-456 only fills it when Account != ""). The operator runs --exclude-accounts sandbox to skip one linked account. Every payer-level recommendation is silently dropped alongside it, and because dimension-filter drops are deliberately not counted in the drop summary (cmd/multi_service_filters.go:62-66), nothing in the output says so.

  Fix direction: for an empty accountName, return len(cfg.IncludeAccounts) == 0, since exclusions cannot match an unnamed account, and count these as a named drop reason.

• MEDIUM: account filters do substring matching, but the flag help and the conflict validator assume exact names

  Where: accountMatchesFilter at cmd/multi_service_filters.go:241-244 (filterLower == accountLower || strings.Contains(accountLower, filterLower)); flag help at cmd/main.go:122-123 ("Only include recommendations for these account names"); conflict check at cmd/validators.go:208 -> validateNoConflicts (cmd/validators.go:234-248), which compares with == only.

  What: substring matching is undocumented and asymmetric with the include/exclude conflict validation, which only rejects identical strings.

  Failure scenario: an org with accounts prod-eu, prod-us, non-prod-sandbox. The operator runs --include-accounts prod --exclude-accounts non-prod. validateNoConflicts passes, since "prod" is not equal to "non-prod". Then checkIncludeList matches non-prod-sandbox because it contains "prod", and checkExcludeList also matches it, so the exclusion happens to win. But --exclude-accounts prod alone would silently exclude non-prod-sandbox too, an account the operator never named. On a purchase run that is a silent scope change in either direction.

  Fix direction: switch to exact match. Delete the strings.Contains arm from accountMatchesFilter, leaving the == compare that validateNoConflicts already uses, so the two agree by construction rather than by a second matcher kept in step.

  Not documenting the substring semantics instead: that blesses substring-for-equality on an account filter, which is the defect class this repo has shipped six times, and it is the more dangerous direction on --exclude-accounts specifically (excluding prod would also exclude nonprod-staging). Not adding glob or prefix syntax either: no reported use needs partial account matching, so that is new surface for a case nobody has demonstrated. Add it if someone asks.

  Remedy simplified (2026-08-03): the "document the substring semantics" option and the glob/prefix syntax were dropped, leaving the exact match. Making the validator match a substring matcher keeps both in the codebase; deleting the substring arm removes the asymmetry and the defect class together.

• MEDIUM: cmd/cleanup-lambda can never delete a purchase execution, and has no status guard if it ever does

  Where: cmd/cleanup-lambda/main.go:63 (dry-run count) and :91 (delete), both DELETE FROM purchase_executions WHERE expires_at < $1.

  What: PurchaseExecution.ExpiresAt is *time.Time (internal/config/types.go:888), and git grep -n "\.ExpiresAt = \|ExpiresAt: *&" origin/main -- internal/ finds no non-test writer for it; the hits are ApprovalTokenExpiresAt, API-key expiry, and PurchaseSuppression.ExpiresAt, a different struct. The column is therefore always NULL, and NULL < $1 is NULL, so the predicate never matches. Separately, the delete has no status filter, so if ExpiresAt is ever populated it would hard-delete pending / scheduled / running rows alongside terminal ones.

  Failure scenario (today): the scheduled cleanup Lambda runs on its cron, logs "Deleted 0 expired executions" forever, and reports executionsDeleted: 0 in its response. Operators reading that see a healthy job while purchase_executions grows without bound. Nothing alerts, because 0-deleted is indistinguishable from "nothing was due".

  Failure scenario (once wired): an execution stuck in running past its grace window is deleted. The per-recommendation idempotency token is derived from the execution ID (DeriveIdempotencyToken(executionID, recIndex), pkg/common/tokens.go:64-67), so a re-drive after the row is gone necessarily uses a new execution ID and a new token. The AWS-side dedupe (ClientToken / IdempotencyTagKey) no longer recognises the earlier attempt, and the purchase can be made twice.

  Fix direction: decide the intended retention rule and implement it explicitly, either by setting ExpiresAt at execution creation or by changing the predicate to a completed_at < $1 AND status IN (<terminal statuses>) form. Either way, add a terminal-status guard before any DELETE.

• MEDIUM: configure-azure / configure-gcp write credentials into ListSecrets(...)[0], a prefix match over an unordered, unpaginated list

  Where: cmd/configure_azure.go:145-166 (storeAzureCredentials) and cmd/configure_gcp.go:116-131 (storeGCPCredentials); the lookup is AWSSecretsStore.ListSecrets (cmd/secrets_store.go:32-56), which builds a FilterNameStringTypeName filter and does no pagination.

  What: Secrets Manager's name filter is a prefix/contains match, not an exact match, and the returned order is unspecified. The code takes arns[0] unconditionally when the list is non-empty and then UpdateSecrets that ARN with the full credential payload. A ListSecrets error is swallowed by the err == nil && guard (cmd/configure_azure.go:152) and falls back to writing by bare name.

  Failure scenario: an account has cudly-AzureCredentials and a leftover cudly-AzureCredentials-2026-rotation-backup from a prior rotation. Both match the filter "cudly-AzureCredentials". If AWS returns the backup first, cudly configure-azure writes the live service-principal client secret into the backup secret and leaves the real one stale. The operator sees "Azure credentials stored successfully" and CUDly keeps authenticating with the old secret, while a live credential now sits in a secret nobody is watching.

  Fix direction: select the ARN whose Name equals secretName exactly, paginating with NextToken; error out when more than one exact match or zero matches is found; and surface the ListSecrets error instead of discarding it.

• MEDIUM: BuildReservationName drops its uniqueness suffix first, so long-SKU services get fully deterministic reservation names

  Where: pkg/common/reservation_name.go:105-125. tail := []string{paymt, ts, randHex} and the loop for keep := len(tail); keep >= 0; keep-- drops from the right: the random suffix first, then the timestamp, then the payment option. Live at six call sites (providers/aws/services/{ec2,elasticache,memorydb,opensearch,rds,redshift}/client.go).

  What: the random suffix is the only unconditional uniqueness source, and the 60-character cap (pkg/common/reservation_name.go:18) removes it before anything else. The function's own docstring example ("opensearch-us-east-1-r6gd-large-search-3x-1yr-allup-...") is 76 characters unabbreviated, so the documented example is itself a case where both the random suffix and the timestamp are dropped.

  Failure scenario: OpenSearch, us-east-1, SKU r6gd.large.search, all-upfront, 1yr. The required-plus-payment prefix is opensearch-us-east-1-r6gd-large-search-3x-1yr-allup at 50 characters; adding the 16-character -20260728T120000 timestamp exceeds 60, so both the timestamp and the random suffix are dropped. Every purchase of that pool, in any run on any day, produces the identical ReservationName. For services where the customer-supplied ID must be unique (RDS ReservedDBInstanceId), a legitimate second purchase of the same pool then fails with an already-exists fault. Where it need not be unique, two distinct commitments become indistinguishable in the console and in any downstream reconciliation that keys on the name.

  Fix direction: treat randHex as required rather than optional: drop paymt and ts before it, or shorten the timestamp or the SKU so the 8-character suffix always survives.

• MEDIUM: engineNameMap is duplicated verbatim between cmd/helpers.go and pkg/common/engine.go, and the pkg/common copy is dead

  Where: cmd/helpers.go:727-760 (engineNameMap, normalizeEngineName) and pkg/common/engine.go:8-41 (engineNameMap, NormalizeEngineName), byte-for-byte identical map contents and fallback. pkg/common/matches.go:20 is the only consumer of the exported copy, and git grep -n "common\.Matches(" origin/main finds no non-test caller of Matches, so the entire pkg/common copy is unreachable.

  What: the live logic is the private cmd copy, used to build the dedupe key in buildExistingCommitmentsMap and getEngineFromRecommendation (cmd/helpers.go:637, :702-722). The pkg/common exported version exists as the "shared" one but nothing shares it.

  Failure scenario: someone adds "Db2": "db2" to pkg/common/engine.go, the obvious exported package-documented location, while the CLI dedupe keeps using the un-updated cmd map. The recommendation-side key and the commitment-side key then diverge for any mapping that is not a pure lowercase, for example adding "Aurora Serverless v2" to only one map. adjustSingleRecommendation (cmd/helpers.go:667-690) finds no existing commitment for a pool that is covered and re-purchases it.

  Fix direction: delete the cmd/helpers.go copy and call common.NormalizeEngineName / common.EngineFromDetails; then either wire common.Matches into the dedupe path or delete it.

• MEDIUM: common.Matches ignores Deployment, Term and PaymentOption, a latent duplicate-suppression bug for whoever wires it up

  Where: pkg/common/matches.go:7-21.

  What: the doc claims "OfferingClass and Term are not compared because Commitment has no such fields", but Commitment.Deployment does exist (pkg/common/types.go:399) and carries the same "single-az" / "multi-az" vocabulary as DatabaseDetails.AZConfig. Its field comment (pkg/common/types.go:383-389) says it was added specifically so "pool-key matching can collapse both sides", yet Matches never reads it. Term and PaymentOption are likewise absent from the comparison.

  Failure scenario: if Matches is adopted for dedupe, which is the shape it is written for, a Single-AZ db.r7g.xlarge/postgres recommendation would match an existing Multi-AZ commitment for the same instance class and engine and be suppressed as a duplicate. Multi-AZ list price is roughly 2x Single-AZ, so the two are entirely different commitments. The same applies to a 1yr recommendation matching a 3yr commitment.

  Fix direction: compare Deployment (normalised) and Term in Matches, or delete the function, since it has no callers.

• MEDIUM: --max-instances and --min-count are unit-mismatched for Savings Plans, which always report Count: 1

  Where: SP recommendations are constructed with Count: 1 (providers/aws/recommendations/parser_sp.go:373); the count-based gates are ApplyInstanceLimit (cmd/helpers.go:505-526) and scorer.filterReason's MinCount check (pkg/scorer/scorer.go:88-90).

  What: an SP commitment is dollar-denominated (SavingsPlanDetails.HourlyCommitment, pkg/common/types.go:578). pkg/common/types.go:237-239 even documents that RecommendedCount is "zero for SPs since the SP commitment is dollar-denominated rather than count-denominated". The instance-count cap therefore counts a $500/hr Compute SP as "1 instance".

  Failure scenario: cudly --services savingsplans --purchase --max-instances 5 reads as a conservative first run. It permits 5 Savings Plans of arbitrary hourly commitment, potentially millions of dollars over a 3-year term, while the same flag would cap an RI run at 5 instances. Symmetrically, --min-count 2 silently filters out every Savings Plans recommendation, with the reason rendered as "count 1 below minimum 2".

  Fix direction: make ApplyInstanceLimit skip or separately budget IsSavingsPlan recs, since a dollar cap is the right unit, and have scorer skip MinCount for SPs rather than filtering them all out.

• MEDIUM: ApplyCoverage passes Savings Plans through unscaled when the Details type assertion fails

  Where: cmd/helpers.go:151-162, the else branch, which logs a warning and then does result = append(result, adjusted) with adjusted == rec, meaning the full unscaled commitment.

  What: the comment argues that "a missing-Details record is a logged anomaly, not a reason to erase coverage from the run", but the chosen alternative is to buy 100% of a commitment during a run the operator sized to a fraction. The project's standing rule is to fail loud rather than degrade silently on money paths.

  Failure scenario: a Compute SP recommendation arrives with Details as a value SavingsPlanDetails rather than a pointer, or nil, for example after a JSONB round-trip where DecodeServiceDetailsFor returned (nil, nil) for an unrecognised service slug (pkg/common/service_details_codec.go:94-97). The operator runs --coverage 25 --purchase. Every other rec is sized to a quarter; this one is purchased at AWS's full recommended hourly commitment. The only signal is a WARNING: line in the middle of stdout, and the CSV row shows the unscaled cost as if that were the intended purchase.

  Fix direction: drop the rec into the DropSummary and exclude it from the purchase set, or return an error that aborts the run, rather than silently promoting it to 100%.

• MEDIUM: the Savings-Plan summary silently discards unknown plan types and every canonical (non-legacy) service type

  Where: categorizeSPRecommendations (cmd/multi_service_stats_helpers.go:20-46) and collectSPSavings (:100-120) switch on the bare string literals "Compute" / "EC2Instance" / "SageMaker" / "Database" with no default. collectRISavings (:129-146) enumerates only ServiceEC2, ServiceRDS, ServiceElastiCache, ServiceMemoryDB and ServiceRedshift, not the canonical ServiceCompute / ServiceRelationalDB / ServiceCache / ServiceDataWarehouse constants those same recs can carry (pkg/common/types.go:73-86).

  What: both are lookup tables that return the zero value for unrecognised input, the silent-default anti-pattern the project's own guidance calls out. The consequence here is not a wrong purchase but a wrong recommendation to the operator.

  Failure scenario: calculateComparisonOptions (cmd/multi_service_stats_helpers.go:159-183) computes Option2Savings = riSavings - risByService.EC2RISavings + BestComputeSP. If the EC2 recs carry ServiceCompute instead of the legacy ServiceEC2, EC2RISavings is 0 while riSavings includes them, so Option 2 adds the Compute SP savings on top of the EC2 RI savings it is meant to replace, double-counting them. determineBestOption (:209-237) then prints "RECOMMENDATION: Use Option 2" on inflated arithmetic. Likewise a new AWS plan type, or a PlanType of "", contributes to no bucket and vanishes from the printed totals with no warning.

  Fix direction: add the canonical constants (ServiceCompute, ServiceRelationalDB, ServiceCache, ServiceDataWarehouse) alongside the legacy ones in collectRISavings, replace the bare string literals with the typed constants, and give each switch a default: arm that errors on an unrecognised value rather than logging and counting it. This is a money-path total; a value that reaches no bucket must not be summarised, and an unknown PlanType including "" is exactly the case that produced the double-count.

  Not adding a canonical-versus-legacy normaliser: it would make ServiceCompute and ServiceEC2 compare equal, and the double-count in the failure scenario above is arithmetic over two buckets that must stay distinct. Enumerating both sets is explicit, is the same number of lines, and fails visibly when a new constant is added instead of quietly folding it into an existing bucket.

  Remedy simplified (2026-08-03): the normaliser was dropped in favour of enumerating the canonical constants, and the default: arm was changed from log-and-count to error, matching the repo's fail-loud rule on money paths.

• LOW: configure-azure's help text advertises passing the client secret on the command line

  Where: cmd/configure_azure.go:101 (the Long help string) and the flag at cmd/configure_azure.go:117.

  What: the first documented usage example is cudly configure-azure --stack-name my-cudly --tenant-id xxx --client-id xxx --client-secret xxx .... Command-line arguments are visible to any local user via ps and /proc/<pid>/cmdline, and are written to shell history. The code already has the safe path: --interactive reads the secret with term.ReadPassword (cmd/configure_azure.go:278) and zeroes the in-memory copy afterwards (cmd/configure_azure.go:204-205), which shows the secret is treated as sensitive everywhere except in its own help text.

  Failure scenario: an operator copy-pastes the documented command on a shared bastion. The Azure service-principal client secret lands in ~/.bash_history and is readable from ps for the duration of the run.

  Fix direction: lead the help with the --interactive form, add an AZURE_CLIENT_SECRET-style env-var path, and mark --client-secret as discouraged in its usage string.

• LOW: queryRDSInstancesInRegion paginates without a page cap or a context check, unlike its fix(aws/recommendations): paginate CE recommendation + RDS engine-version describe (single-page truncation) #692-hardened sibling

  Where: cmd/multi_service_engine_versions.go:132-157. Compare fetchMajorEngineVersionsForEngine (:232-267), which checks ctx.Err() at the top of every page and enforces maxEngineVersionPages (:94, added for fix(aws/recommendations): paginate CE recommendation + RDS engine-version describe (single-page truncation) #692).

  What: the RDS-instance paginator loops on output.Marker with no bound and never consults ctx.

  Failure scenario: a region whose DescribeDBInstances returns a non-advancing Marker, or simply a very large fleet, spins that region's goroutine indefinitely. Because queryRDSInstancesInRegions blocks on wg.Wait() (:127), the whole CLI hangs before it fetches a single recommendation, and Ctrl-C is the only exit since the goroutine ignores context cancellation.

  Fix direction: apply the same ctx.Err() check and page cap the sibling paginator already has.

  Related: ctx.Canceled silently swallowed in queryMajorEngineVersionsWithClient (surfaced by PR #1225) #1325 covers context cancellation being swallowed in queryMajorEngineVersionsWithClient and asks for a sweep of sibling fan-out loops. This is a different function and a different shape (an unbounded paginator rather than a swallowed error), so it will not be caught by that sweep's grep, but it is the same file and cheapest to fix in the same change.

• LOW: CheckAuditLogWritable creates the audit log with 0600, defeating the 0644 the writer documents as required

  Where: cmd/helpers.go:558 opens with os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600; pkg/common/audit.go:28 opens the same path with 0644, and the comment at pkg/common/audit.go:23-26 states 0644 is intentional because "the audit log is consumed by ops tooling and reconciled against purchase_history; restricting to 0600 would break that workflow".

  What: the writability pre-check runs first (cmd/multi_service.go:108) and creates the file, so O_CREAT in WriteAuditRecord is a no-op and its mode argument never applies. Every audit log created by the CLI is 0600.

  Failure scenario: the ops reconciliation job runs as a different service user and gets permission denied reading cudly-audit.jsonl, the exact outcome the 0644 comment says it is avoiding.

  Fix direction: use the same mode in both places, picking one deliberately, or have the pre-check open without O_CREATE and treat a missing file as writable if the directory is.

• LOW: RATE_LIMIT_DOMAIN is interpolated into a SQL LIKE pattern without escaping wildcards

  Where: cmd/lambda/clear-rate-limit/main.go:40-42, with the value from getDomain() (:15-20).

  What: the query is properly parameterised, so this is not injection, but % and _ inside the env var are LIKE metacharacters, so the deletion scope is wider than the variable name implies.

  Failure scenario: RATE_LIMIT_DOMAIN=% makes the pattern EMAIL#%@%#ENDPOINT#forgot_password, which deletes the forgot-password rate-limit rows for every domain rather than one, removing brute-force protection across all tenants. A typo containing _, for example leaner_cloud.com, silently widens the match by one character position.

  Fix direction: reject or escape % and _ in getDomain(), or match on an exact suffix column instead of a constructed LIKE pattern.

  Related: sec(auth): forgot-password rate limit comment mismatch and no per-IP cap cloud-commitments-platform#13 covers the forgot-password rate limit's configuration and missing per-IP cap. This is a different mechanism (an operational tool that can clear the limits wholesale), but the two share a blast radius.

• LOW: retry.Do's doc claims the per-attempt context is "independent of ctx"; it is derived from it

  Where: pkg/retry/exponential.go:36-38 and :96-98 both say the per-attempt context is "independent of the outer ctx". runAttempt (:145) builds it with context.WithTimeout(ctx, cfg.PerAttemptTimeout), which is a child of ctx.

  What: the behaviour (fail fast per attempt, keep the retry budget) is fine; the doc overstates it. A caller reading it would expect an outer cancellation not to abort an in-flight attempt, when in fact it does.

  Failure scenario: a future caller relies on the documented independence to let an in-flight money-path attempt finish after the parent context is cancelled, for example to observe whether a purchase landed. The attempt is cancelled instead, and the caller cannot tell a cancelled-before-send from a cancelled-after-send.

  Fix direction: reword to "bounded independently of the outer deadline but still cancelled with it", or use context.WithoutCancel(ctx) if independence is genuinely wanted.

• LOW: logging.SetOutput writes defaultLogger.output unsynchronised while With reads it

  Where: pkg/logging/logger.go:272-277 assigns defaultLogger.output = w; (*Logger).With reads l.output at :129. Only Logger.level is atomic (:37); output is a plain field with no mutex.

  What: the struct doc at :31-35 explains that level was made atomic because the logger is used "from many goroutines". The same reasoning applies to output, which was left unprotected. The underlying log.Logger.SetOutput is itself mutex-guarded, so only the Logger.output mirror races.

  Failure scenario: a test calls logging.SetOutput(buf) in t.Cleanup while a background goroutine from the code under test calls logging.With("k", v). go test -race reports a data race on Logger.output: an intermittent CI failure unrelated to the code being tested, which is the kind that gets retried rather than diagnosed.

  Fix direction: store output in an atomic.Value, guard it with the same mutex pattern, or drop the mirror field and read it back from the log.Logger.

• LOW: pkg/errors and pkg/config have no importers

  Where: pkg/errors/errors.go (300 lines, 7 error types) and pkg/config/{load,types}.go (about 490 lines).

  What: git grep -ln "CUDly/pkg/errors" origin/main and git grep -n "pkg/config" origin/main, excluding the packages themselves, both return nothing. Both are fully tested and look production-ready, which makes them attractive to a reader looking for the project's error and config conventions.

  Failure scenario: a contributor adopts pkg/config.Load for a new entry point and inherits the DryRun-inversion bug (filed separately from this review as its own issue) without any existing caller having exercised it. Similarly, pkg/errors' documented type-level Is semantics (field-insensitive matching, pkg/errors/errors.go:5-11) are surprising enough that first adoption is likely to misuse them.

  Fix direction: either wire them in, fixing applyYAMLBase first, or delete them so the tree reflects what actually runs.

Verified-clean areas (no findings)

Recorded so a later pass does not re-derive them: pkg/common/tokens.go (MaskToken fully redacts inputs of 8 characters or fewer rather than echoing a prefix that would be the whole value, :82-84; DeriveIdempotencyToken fits the 64-character AWS ClientToken limit exactly; IdempotencyGUID validates hex before formatting and returns "" below 32 characters; DeriveMuteToken / VerifyMuteToken fail closed on an empty key and compare with hmac.Equal); pkg/common/drop_summary.go (Add / Total / IsEmpty are all nil-receiver-safe, which matters because several call sites pass a nil *DropSummary); pkg/concurrency (leaf-only acquisition correctly documented and implemented; Acquire returns ctx.Err() on cancellation and no-ops with no semaphore attached); pkg/provider/registry.go (factories snapshotted under RLock and invoked lock-free, so a factory doing network I/O cannot block the registry); cmd/rekey/main.go (env-gated, refuses to run when the real key equals the dev key, per-row transactions, plaintext never logged); and cmd/gen-permissions/main.go (deterministic sorted output, no injection surface).

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A10-004 (high)

A fourth account-filter item, upstream of the three already listed. GetAccountAlias (cmd/helpers.go:91-94) caches and returns the numeric account ID as the account name when organizations:DescribeAccount fails, and shouldIncludeAccount then filters on that stand-in (cmd/multi_service_filters.go:105, :156-175). So cudly --purchase --exclude-accounts production against account 123456789012 finds no substring match and buys in the account the operator explicitly ruled out; --include-accounts silently narrows the same way. The bad value is cached for the process lifetime, so one transient Organizations throttle poisons every rec for that account. This is the same class as the 'unnamed recommendation' item but a different cause: the name is present and wrong rather than absent. Fix direction: return (string, error) and fail the run when an account filter is in force and the alias cannot be resolved, and do not cache the failure. Finding A10-004.

A09-014 (medium)

Two facts to fold into the pkg/errors / pkg/config bullet, since they change the cost of leaving pkg/config in place. It is the only importer of github.com/spf13/pflag and gopkg.in/yaml.v3 anywhere in the pkg module (load.go:12-13 plus its test), and both sit in pkg/go.mod's direct require block at lines 13 and 16, so every consumer of the shared module inherits two dependencies for unreachable code. Deleting the package lets pflag go entirely; yaml.v3 becomes indirect rather than droppable, since testify pulls it in. Separately, pkg/config defines its own DefaultScorerConfig that no caller can reconcile against the live scorer configuration, which is a second trap for the first adopter alongside the DryRun inversion already filed as LeanerCloud/cloud-commitments-go#56. (audit finding A09-014)

A09-018 (medium)

The (nil, nil) return that the ApplyCoverage bullet uses as its failure scenario is worth listing as its own defect. DecodeServiceDetailsFor (pkg/common/service_details_codec.go:95) returns (nil, nil) before ever looking at raw whenever newDetailsForService has no case for the service slug (:121-167), so a row persisted by a newer deployment with a fully populated Details blob decodes to nothing on an older reader and the purchase proceeds with nil details instead of failing. The in-code comment calls this 'suspicious but tolerated'. Returning (nil, nil) only when raw is empty, and an error naming the service when a payload exists for a slug with no typed shape, would surface version skew instead of dropping purchase parameters. No current slug loses data this way (memorydb's nil details are deliberate at :154-161), so this is low severity today. Audit finding A09-018.

A07-029 (low)

A second latent mismatch on the same matcher. providers/aws/services/ec2/client.go:59-61 returns common.ServiceCompute ("compute") from GetServiceType, while GetExistingCommitments at :98 stamps Service: common.ServiceEC2 ("ec2") on every commitment it builds. common.Matches compares rec.Service to c.Service at pkg/common/matches.go:14, so an EC2 recommendation tagged compute can never match an EC2 commitment tagged ec2. Same inconsistency at ladder/commitments.go:85. Like the Deployment/Term/PaymentOption gap, this is latent because Matches has no caller, but whoever wires it up inherits both. Building the commitment from c.GetServiceType() removes the ability for the client to describe itself two ways. Finding A07-029.

A09-030 (low)

There is a second defect in this file worth pairing with the doc fix, and this one is behavioural rather than documentation. backoffFor (pkg/retry/exponential.go:168-186) applies the plus-or-minus 25 percent jitter factor after both MaxDelay clamps, so the documented cap does not hold: with MaxDelay = 30s the actual sleep reaches 37.5s. Clamping the post-jitter delay to MaxDelay is a one-line change and makes the documented bound true, which matters for the same class of caller the context wording misleads, one sizing a retry budget against the stated numbers. Audit finding A09-030.

A10-030 (low)

One more configure-wizard item for this tracker, on the same two files. The prompt at cmd/configure_azure.go:481 reads "[R]un, [S]kip?" with no default marked, but the guard skips only when the choice is none of "r", "run" or "", so pressing Enter falls through to createAzureServicePrincipal at :513: an Entra ID application, a password credential and a "Reservations Administrator" assignment at /subscriptions/. Undoing it means deleting the application by hand, since the wizard's rollback fires only on failure. The GCP wizard shares the shape with case "r", "run", "": at cmd/configure_gcp.go:666 and :700, so the same Enter creates a service account and grants it compute.admin. Making Skip the empty-input default on the resource-creating steps, and leaving Run-on-Enter only for the read-only listing prompts, fits alongside the help-text and ListSecrets items already here. Finding A10-030.

Scope after the split

Per the monorepo split's path mapping: root-level cmd/*.go files stay with this repo, pkg/ and providers/ moved to cloud-commitments-go, and cmd/ subdirectories (cleanup-lambda, lambda, server, rekey, gen-permissions) moved to cloud-commitments-platform.

• The cmd/cleanup-lambda and cmd/lambda/clear-rate-limit findings went to chore(pkg/cmd): medium/low findings from the 2026-07-28 full review [platform part] cloud-commitments-platform#357 (cmd/ subdirectories).
• The pkg/common, pkg/scorer, pkg/retry, pkg/logging, pkg/errors, pkg/config, and providers/aws findings went to chore(pkg/cmd): medium/low findings from the 2026-07-28 full review [go part] cloud-commitments-go#123.
• Two findings span both repos (engineNameMap duplication between this repo's cmd/helpers.go and the go repo's pkg/common/engine.go; the Savings-Plans count-unit mismatch between this repo's ApplyInstanceLimit and the go repo's scorer.filterReason). Both are quoted in full in both issues with a cross-reference, so fixing either repo's half in isolation is visible as incomplete.

This issue is now scoped to only the findings whose fix location is root-level cmd/*.go in this repo, re-verified against the current tree (2026-09-28: all referenced files still present at their original paths, content not individually re-diffed given the volume - re-verify line numbers before changing code per the issue's own standing disclaimer):

MEDIUM (remaining in this repo)

• determineCSVCoverage treats an explicit --coverage 80 as "not set" and silently buys 100% (cmd/multi_service_csv.go:18-26, cmd/multi_service.go:315).
• On the CSV path, account filters are applied before AccountName is populated (cmd/multi_service.go:328,350, cmd/multi_service_filters.go:98,194-213).
• shouldIncludeAccount drops every unnamed recommendation as soon as any account filter is set (cmd/multi_service_filters.go:194-198).
• Account filters do substring matching, but the flag help and the conflict validator assume exact names (cmd/multi_service_filters.go:241-244, cmd/main.go:122-123, cmd/validators.go:208,234-248).
• configure-azure / configure-gcp write credentials into ListSecrets(...)[0], a prefix match over an unordered, unpaginated list (cmd/configure_azure.go:145-166, cmd/configure_gcp.go:116-131, cmd/secrets_store.go:32-56).
• engineNameMap is duplicated verbatim between this repo's cmd/helpers.go:727-760 and cloud-commitments-go's pkg/common/engine.go:8-41 (cross-repo item, see above).
• --max-instances and --min-count are unit-mismatched for Savings Plans on this repo's side: ApplyInstanceLimit (cmd/helpers.go:505-526) treats every SP as "1 instance" (cross-repo item, see above).
• ApplyCoverage passes Savings Plans through unscaled when the Details type assertion fails (cmd/helpers.go:151-162).
• The Savings-Plan summary silently discards unknown plan types and every canonical (non-legacy) service type (cmd/multi_service_stats_helpers.go:20-46,100-146,159-183,209-237).

LOW (remaining in this repo)

• configure-azure's help text advertises passing the client secret on the command line (cmd/configure_azure.go:101,117).
• queryRDSInstancesInRegion paginates without a page cap or a context check, unlike its fix(aws/recommendations): paginate CE recommendation + RDS engine-version describe (single-page truncation) #692-hardened sibling (cmd/multi_service_engine_versions.go:132-157).
• CheckAuditLogWritable creates the audit log with 0600, defeating the 0644 pkg/common/audit.go (go repo) documents as required (cmd/helpers.go:558).

From the 2026-09-02 audit (remaining in this repo)

• A10-004 (high): GetAccountAlias (cmd/helpers.go:91-94) caches and returns the numeric account ID as the account name when organizations:DescribeAccount fails, and shouldIncludeAccount then filters on that stand-in. cudly --purchase --exclude-accounts production against an account whose alias failed to resolve finds no substring match and buys in the account the operator explicitly ruled out. The bad value is cached for the process lifetime. Fix: return (string, error) and fail the run when an account filter is in force and the alias cannot be resolved; do not cache the failure.
• A10-030 (low): the configure wizards' "[R]un, [S]kip?" prompts have no default marked, but pressing Enter falls through to the resource-creating branch (cmd/configure_azure.go:481,513; cmd/configure_gcp.go:666,700), creating an Entra ID application / GCP service account with elevated grants on a blank input. Fix: make Skip the empty-input default on resource-creating steps.

All other findings from the original review body above are superseded by the sibling issues linked here.