Skip to content

Nice-to-have bundle from the 2026-07-18 whole-codebase review #200

Open
Open
Nice-to-have bundle from the 2026-07-18 whole-codebase review#200
Assignees
chmmou

Description

@chmmou
chmmou
opened on Jul 18, 2026

Grouped Nice-to-have findings from the whole-codebase review (Med findings: #198, Low findings: #199). None of these block anything; pick up opportunistically.

  • internal/transport/client.go (flood gate) — the gate enforces a delay floor but does not serialise concurrent Do calls; irrelevant for the single-flight CLI today, worth revisiting only if concurrent calls ever appear.
  • internal/cli/audit.go (redaction) — the exact key session is in auditSecretParams, but a future session_id-style parameter would only be caught by the substring fragments if it contains one of them; purely latent.
  • internal/cli/run.go (dry-run) — --dry-run requires a resolvable credential profile although no API call is made; a preview without credentials could be allowed.
  • internal/auth/errors.go:17 — CodeGotNoLoginData is the only exported code without an Is… helper (asymmetric with the other codes).
  • testdata/mailinglist/update_mailinglist_request.xml:16 — trailing comma makes the request fixture invalid JSON; harmless today, but it is the declared request contract.
  • testdata/ various — get_<singular>-named fixtures (e.g. get_account_*, get_domain_*, get_mailaccount_*) encode filter-views of plural actions; optional rename to get_<plural>_*_single.xml for consistency with the fix: address Low-severity findings from the whole-codebase review #199 renames.
  • internal/kasread / internal/kaswrite — soap.Value is used as a deliberate cross-boundary DTO; recorded here so a future review does not reclassify it as a layering violation. No action.

Second-pass additions (2026-07-18 re-review)

The get_<singular> fixture-rename bullet above also applies to testdata/{cronjob,ddns,ftpuser,sambauser}/get_<singular>_* — note these files are referenced by test paths, so a rename must be paired with the test updates.

New Nice-to-have findings from the re-review:

  • internal/cli/audit.go:83 — the 256-byte elision threshold will elide legitimate long single-line values (DNS TXT/DKIM records) in the --dry-run preview once the DNS write slice lands; revisit the threshold or key it on secrecy then.
  • internal/cli/audit.go:19 + docs/usage/destructive-writes.md (audit-log example) — the canonical examples reference add_dns_settings/delete_dns_settings, which are not wired yet; switch to a wired action.
  • testdata/**/*_request.xml (~81 files) — no request fixture is referenced by any test; builder<->fixture alignment was verified manually once but is not anchored. Optionally add a round-trip pin per write module, or declare request fixtures documentation-only.
  • internal/session/store.go:113 — the withLock comment says flock.LockContext but the code uses TryLockContext (polling).
  • internal/soap/envelope.go:69 — "soap: empty document" is misleading for non-empty non-SOAP payloads; also compares err == io.EOF instead of errors.Is, inconsistent with the auth decoder.
  • internal/soap/value.go:248 — classifyType("") maps to KindNil, a dead branch (UnmarshalXML has no KindNil case); returning KindUnknown would be honest.
  • internal/api/errors.go + internal/auth/errors.go — IsCode(err, "") returns true for any non-fault error; guard code != "".
  • internal/kasread/kasread.go:46,80 — a comment calls the api fault predicates "transport-error helpers"; Get errors (not-found / ambiguous) carry no sentinels (latent — no caller branches on them yet).
  • docs/usage/destructive-writes.md — does not mention that the write-success output honours --output (it renders through the same pipeline as the read commands).
  • CHANGELOG.md — the released [0.1.0-alpha.1] block contains duplicated section headers; decide whether to consolidate or keep as history.

Activity

  1. chmmou
    added this to kasapi-cli v0.2.0-alpha.1on Jul 18, 2026
  2. github-project-automation
    moved this to Todo in kasapi-cli v0.2.0-alpha.1on Jul 18, 2026
  3. chmmou
    self-assigned this
    on Jul 18, 2026

  4. chmmou commented on Jul 18, 2026

    @chmmou
    chmmou
    on Jul 18, 2026
    OwnerAuthor

    Third-pass additions (2026-07-18 re-review)

    Nice-to-have findings from the third review pass, recorded here per the review loop; none block the current fix branch:

    • internal/transport/client.go — a fault body delivered with HTTP 4xx still bypasses the typed-fault path (asymmetry with the 5xx fault pass-through). No captured KAS fixture shows a 4xx-wrapped fault, so this stays theoretical.
    • internal/api/errors.go — IsAuthFailure includes kas_access_forbidden, so a genuine permission denial triggers a full re-auth + one-shot retry that cannot succeed. Documented as intended; flagged for reconsideration.
    • internal/testutil/testutil.go — the dead-want-key message says "renamed or deleted?" even when a file of that name exists but is not a fault fixture (e.g. a success fixture); the hint could distinguish the two.
    • ROADMAP.md — the mail forwards get placeholder shows <address> while the actual command Use line says get <mail-forward>.
    • CLAUDE.md — "The CI gate … runs on every push and pull request" slightly over-claims: OpenSSF Scorecard runs only on push-to-main and weekly.
    • internal/auth — a --otp value is reused verbatim on the auth-refresh retry; the retry only succeeds while the TOTP is still valid. Inherent to one-shot OTPs, no code action, noted for the docs backlog.

  5. chmmou commented on Jul 18, 2026

    @chmmou
    chmmou
    on Jul 18, 2026
    OwnerAuthor

    Nice-to-have findings from the fourth whole-codebase review pass (2026-07-18, post-#199/#197; grouped here per the review-skill convention):

    1. internal/cli/dryrun.go:82 — when rendering the dry-run preview fails, no audit record is written at all, a hole in the "exactly one record per outcome" invariant (purely local infra error, nothing was dispatched). Direction: emit the dry-run record before/regardless of the render result.
    2. internal/cli/run.go:188 — on double failures the exit-code priority is dispatch > render > audit but is documented nowhere. Direction: one sentence in the runWriteE doc comment or destructive-writes.md.
    3. internal/kaswrite/kaswrite.go:53 — the nil-response error has no errors.Is-able sentinel, unlike ErrUnexpectedReturnString. Direction: add one for symmetry.
    4. internal/transport/client.go:14 — transport imports soap only for MaxResponseBytes, inverting the dependency direction (no cycle, architecture smell). Direction: move the constant to a neutral home or duplicate it with a sync note.
    5. internal/soap/envelope.go:186 — DecodeFault discards structured child XML inside a fault <detail>; KAS has so far only sent plain text. Direction: keep, but note the limitation in the doc comment.
    6. internal/transport/client.go:46 — the exported Now/Sleep hooks are read without a lock; a "set before first use" doc note is missing. Direction: one-line doc comment.
    7. internal/cli/ddnsusers.go:207,236 — update/delete use the <dyndns-login> placeholder while get uses <ddns-login>. Direction: pick one spelling (the get filter param is ddns_login).
    8. internal/cli/databases.go:151 — update --allowed-hosts omits the empty-value=wildcard semantics although an explicitly empty value widens access to all hosts (add explains it). Direction: mirror add's note.
    9. internal/cli/output.go:104 — a Tabular implementation error (per its doc "a kasapi-cli bug") is classified as UserError exit 1 rather than an internal error. Direction: document or reclassify.
    10. Fault fixtures of already-shipped write slices (cronjob, directoryprotection, mailforward, mailinglist, …) are not anchored by mapping tests — same class as the fixture-coverage item already tracked in this issue; listed for completeness.
    11. internal/cli/root.go (RejectUnknownSubcommands) — would unconditionally overwrite a future non-runnable group's own Args validator; no such group exists today (latent). Direction: only set Args when nil, or leave as is until one appears.

    Addendum from the diff review of the fourth-pass fix branch (#201), cosmetic only:

    • internal/cli/root.go (rejectUnknownHelpTopics) — the RunE override calls target.Help() without flowing the context down first, unlike cobra's stock help Run; no functional impact with the default help templates.
    • internal/cli/ddnsusers.go:175 — the --dual-stack update help phrases the bool as "Y when set, --dual-stack=false sends N" while the other bool flags use "pass --…=false to disable"; arguably clearer, so keep-or-align is a coin toss.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions