improve-codebase-practices

name

improve-codebase-practices

description

Audit Go code against production conventions: error sentinels, wrapping, context, ownership cloning, tests, concurrency, lint suppressions, and public-API contracts. Emits structured findings for a phase-wise checklist. Use when reviewing Go best practices, idioms, test quality, API contracts, or running /improve-codebase-practices. Do not reopen decided product tradeoffs, run perf-review, or delete code for leanness (ponytail).

Improve-codebase — Go practices

Audit conventions that keep a Go library honest. This is not a style pass (gofmt already ran) and not a performance pass.

Do not implement.

Read first: ../references/finding-schema.md. If the module path is gowkhtmltopdf, also read ../references/gowkhtmltopdf.md. Calibrate to .golangci.yml, Makefile, and existing tests before filing.

1. Law vs taste

Law (file if broken):

• make lint (enable-all here; gofumpt/tenv stay disabled).
• %w wrapping. No new nolint:wrapcheck outside tests.
• Canonical sentinels aliased, not re-constructed.
• Public clone-on-intake for maps/slices/bytes the caller still owns.
• Nil context rejected at every exported cancellation-aware entry with the canonical ErrNilContext.
• Tests use errors.Is, t.Parallel(), t.Context() / t.TempDir().
• CGO_ENABLED=0. Direct deps stay on the allowlist.
• Lint suppressions are local and name the reason.

Taste (do not file): line wrapping, comment voice, whether a helper is 8 lines or 12, := vs var unless it hides a nil map.

2. Error and API contracts

Walk exported functions and the engine entries they call.

Pattern	Finding when
Sentinel	Same condition, different errors.New text/value
Prefix	Public errors lose the gowkhtmltopdf: prefix; internal wrap drops %w
OnError / hooks	Preflight fails and the hook is skipped, or the hook swallows the return
Panic vs error	Fluent programmer-error panics are policy; Set/Convert/Run* must not panic on user input
Distinct nils	ErrNilConverter and ErrNilPDFRequest (etc.) collapsed into one value
Validate-before-write	Validate* / Run* writes bytes, then fails

Do not demand a sealed option interface. A compatibility dotted Set plus a typed overlay is a product, not a smell.

3. Context and cancellation

• Thread ctx as a parameter.
• One documented containedctx engine that polls at recursion boundaries is allowed. A second struct that stores ctx is a finding unless it is the same engine.
• ctx.Err() belongs between expensive stages and at load/fetch seams, not on every CSS declaration.
• Adapters named Layout / Paint that call context.Background() are legacy names, not bugs, if *Context variants exist.

4. Ownership and concurrency

• Clone at the public boundary. Prove with a test that mutates the caller's buffer after AddObject / SetBody / Convert.
• Single-goroutine engines do not get a "just in case" mutex. File the mutex as defect if someone added one on Document or layout without a second goroutine that actually writes.
• sync.Pool: no &slice escape; copy-on-put when the buffer is retained. sync.Once for lazy immutable tables is correct.
• Converter is not concurrent. Do not add internal locking to make Convert re-entrant; the docs already say one converter per run.

5. Tests that lie

File defect or friction when a test claims a contract it does not exercise:

• Golden / structural walker treated as a pixel or overlap oracle.
• Semantic oracle that builds a pdf.Document by hand and is sold as HTML→PDF proof.
• Byte-identical PDF assertions (forbidden product claim here).
• Performance budgets run under -race and treated as release numbers.
• Live network inside make test.
• White-box //nolint:testpackage with no reason.
• A matrix Implemented row with no test and no fixture.

Prefer adding the missing assertion over inventing a new test framework.

6. Refuse list (do not nag)

If this is gowkhtmltopdf, the refuse list is ../references/gowkhtmltopdf.md § Do not nag — do not restate it.

In any other repo, refuse: a second settings system, pixel-diff merge gates, mutexes on a documented single-goroutine engine, context on every struct, live network in unit tests, and re-filing a closed ledger row without a current-source regression.

7. Emit findings

Finding-schema records. Cap a solo run at 15. Prefer PRAC- IDs after dedupe. Quote current file:line. Name the sentinel, test, or clone helper that should own the fix.

If invoked by /improve-codebase, return the finding list only.

If invoked alone, continue to skills/phase-wise-checklist/SKILLS.md and write:

plans/reviews/improve-codebase/practices-<YYYY-MM-DD>/phase-wise-checklist.md

Phase order for this lens: security/ACL and output integrity → public sentinel/clone contracts → context → tests that lie → lint suppressions / docs. Closure gates stay in the checklist skill.