fix: address Low-severity findings from the whole-codebase review - #199
Merged
Conversation
TokenSource.Invalidate now reports whether the next Credentials call can produce fresh credentials. StaticTokenSource returns false, so an auth failure with plain credentials no longer doubles the failing request against the flood gate; SessionTokenSource returns true and keeps the refresh-and-retry behaviour.
context.Canceled / DeadlineExceeded from the HTTP round trip is the caller's decision to stop, not a transient server condition; doOnce now returns it without the retryable marker instead of burning a backoff sleep before failing on the same ctx again.
…failure Three write-path fixes from the review: the destructive [y/N] prompt goes to stderr so a redirected stdout cannot swallow it; refused and declined destructive attempts leave an audit record (outcome=refused / declined); and a dispatched write whose audit sink fails keeps its true exit classification (KAS fault stays exit 2, a successful write renders its result before the audit error surfaces as exit 1).
DecodeResponse rejects a <return> value that is not 40 alphanumeric characters before it gets cached and persisted (the error reports only the length, never the content). EncodeRequest range-checks Lifetime against the documented 1..30000 session_lifetime bound (0 = server default).
Resolve failures were string-only errors; callers that need to branch now have errors.Is-able sentinels, per the stable-error-identifier rule.
…/usage Four modules exported their Caller as API while the other ten keep it unexported as c; unified on the unexported form (constructed via NewClient everywhere, no caller used the exported field). cronjob also gains a FieldID constant replacing three hardcoded cronjob_id literals.
get_database, get_directoryprotections, and get_softwareinstalls are not real KAS actions (the fixtures dispatch get_databases / get_directoryprotection / get_softwareinstall); the files now carry the real action name plus a variant suffix (_single for the filtered variant, _all for the unfiltered list), and the three doc.go endpoint lists no longer name the non-actions.
The #125 placeholder modules ship ~25 captured fault fixtures that no test referenced; a one-line AssertFaultFixtures test per package binds them to the KAS contract until the write slices land.
info@example1.org and /example-new.com/ were redactions pointing at registrable domains; normalised to example.org / example.net.
Two corrections from the post-fix re-review: the ctx-cancellation non-retry guard now keys on ctx.Err() instead of errors.Is(err, context.DeadlineExceeded) — a per-attempt HTTPClient.Timeout error also matches the latter but is a transient slow-server condition that must stay retryable (pinned by a new test); the same guard now also covers the body-read path. runWriteE emits the 'audit record not fully written' warning on the refusal and render-error paths too, matching the dispatch path.
…w corrections Bad user input (positional-args failures, unknown root subcommand) now exits 1 instead of 2; 5xx responses carrying a SOAP fault pass through to the decoder instead of being retried blindly; AssertFaultFixtures rejects dead want-keys and the shared fault fixtures gain a dedicated anchor; config.Resolve wraps ErrUnknownProfile on the nil-config --profile path; session.Store.Load treats a zero expires_at as expired; cronjobs/ftpusers update bind their own flag sets; --dry-run help says "write command"; stale doc.go/comment claims corrected; mis-captured get_topleveldomains request fixture and out-of-convention dns/cronjob fixture names fixed.
…contract to the shipped state README no longer claims write paths are pending and lists the full CI gate set; the destructive-writes prompt example matches the real single-line stderr prompt and the audit-trace scope excludes the sessions delete / config use-profile session logout explicitly; ROADMAP gains mail lists get and the real <ddns-login> placeholder; CLAUDE.md's repository-state paragraph reflects the landed write slices.
…review pass Group commands invoked with an unknown subcommand exited 0 (help) — cli.Finalize now rejects them as user errors and registers the lazy completion command before the exit-code walkers, so its args errors exit 1 as well. Create actions with a server-generated identifier record it as created_id in the success audit record, a failed confirmation prompt read is audited as outcome=aborted, the transport 5xx fault sniff also recognises the prefix-less default-namespace Fault form (with an end-to-end api-layer HTTP-500 fault test), the sambausers update command binds its own replacement-flag set, and the remaining get_<singular> fixtures were renamed to the plural KAS action their content embeds.
CHANGELOG umbrella entry for the High/Med/Low fixes, the CLAUDE.md fixture-naming example now uses a conforming plural-action name, the destructive-writes contract documents the new aborted audit outcome and the created_id field on success records of server-generated-ID creates, and the stale pre-rename fixture reference in the unreleased cronjob entry is corrected.
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.
Fixes the fifteen Low-severity findings from the 2026-07-18 whole-codebase review. Stacked on #198 (base:
fix/review-med-findings); after #198 is merged this PR retargetsmain.Invalidate() bool)[y/N]prompt on stderr; refused/declined attempts leave an audit record; audit-sink failure no longer masks the true exit classificationsession_lifetimerange (1..30000) validatedErrUnknownProfile/ErrMissingCredentialssentinelsCallerfield unexported in dns/directoryprotection/server/usage;cronjob.FieldIDconstant