[efegokdemir]

Problem

A code review identified repeated validation, duplicated response parsing, redundant data copies and several small cleanup opportunities across the application.

Changes

• Simplify routing, error handling, configuration defaults and retry parsing.
• Consolidate OAuth secret cleanup and management API fields.
• Use HTTP status constants and remove an unnecessary storage dependency.
• Propagate terminal response status directly to execution logging and the gateway, and avoid redundant model-list cloning.
• Correct the stream terminal failure type and add regression coverage.

Two checks are intentionally retained:

• http.Transport.Dial is still inspected to avoid overriding a caller-supplied legacy dialer. The deprecated access is documented and regression-tested.
• Configuration validation remains at the environment loader and independent constructors so direct callers cannot bypass validation. The boundaries are documented and tested.

Validation

• just format --check passed.
• just lint passed.
• just test passed with race detection and shuffled execution.
• git diff --check passed.
• Added regression tests for transport compatibility, model snapshots, terminal statuses and direct configuration validation.

Closes #52

[deyna256]

Summary: the change covers #52 and leaves the API contract unchanged, but four points need fixing first: a nil-pointer trap the new *Failure signature introduces, two comments that give a reason that does not exist, missing doc comments on the two newly exported types, and lint directives that suppress nothing in the linter this project is adopting.

Blocking, all inline: internal/codex/stream.go:127, internal/app/app.go:64 with internal/app/config.go:47, internal/management/routes.go:48, and internal/codex/client.go:101 with internal/codex/client_test.go:41.

Checked (at 53b0d16):

• just format --check, just lint, just deps (no file changes) and just test pass.
• GET /api/openapi.json byte-compared with the base commit: the only difference is the order of required in the create-client-key response schema, so the published contract is unchanged.
• Making ListInput unexported again drops limit, offset and q from the schema and makes GET /api/client-keys?limit=1 answer 422, so the new exports are required.
• Blank and whitespace login names return the same 422 body before and after, so removing the handler check is safe.
• The status sweep is complete: the diff removes exactly the 26 numeric statuses #52 counted. The DefaultStatus: 204/201 literals in internal/management/routes.go were never part of that count, so they are out of scope here.
• I did not review Codex request and SSE payload handling beyond the changed lines.

[deyna256]

finish runs errors.As(err, &failure) and then writes failure.RetryAfter, so a nil *Failure carried in a non-nil error panics; I reproduced it. This branch is the only thing preventing that and nothing here says so, which makes the obvious cleanup to s.finish(failure) a crash on every successful stream.

Every finish call site already passes a *Failure (safeFailure, responseFailure, &Failure{...}), so taking one removes the trap instead of documenting it:

func (s *Stream) finish(failure *Failure) {
	s.ended = true
	var err error
	if failure != nil {
		failure.RetryAfter = s.retryAfter
		err = failure
	}
	s.err = err
	// unchanged from here
}

This branch then becomes s.finish(failure). I ran go vet and go test -race for codex, execution and gateway with that change and they pass.

[deyna256]

newApplication has one production caller, Run at app.go:48, and it always passes a loadConfig result, so no direct caller skips validation today; only tests construct a config by hand. #52 accepts a kept check when the reason is recorded, and this reason does not hold as written.

Either drop the duplicate validation as the issue asks, or state what the check really guards (a constructor invariant that only tests exercise) here and in the matching comment at config.go:47.

[deyna256]

ListInput and KeyView are exported now, and the reason is invisible: huma skips unexported embedded structs. I made this type unexported again to check, and limit, offset and q disappear from the schema while GET /api/client-keys?limit=1 answers 422.

Every other exported type under internal/ carries a doc comment. Please give these two one sentence each, including that constraint, so the exports are not undone later.

[deyna256]

golangci-lint does not act on //lint:ignore. With golangci-lint 2.13.2 and staticcheck enabled I get SA1019 at client.go:102 and client_test.go:42 with both directives in place; replacing them with //nolint:staticcheck // <reason> silences both. As written the suppression will not survive #53, which is the reason it was added.

Standalone staticcheck reads the opposite directive, so if that tool is also planned, say which one this project follows.

[deyna256]

Nit: which fields need a deep copy is now stated here and in cloneModels. A cloneModel(Model) Model used by both keeps it in one place; otherwise a third slice field on Model takes two edits, and missing the second one shares cached slices with the snapshot.

[deyna256]

Nit: accounts.go loses this same check in this PR while createKey keeps it, and this one is equally unreachable: minLength:"1" pattern:"\\S" makes huma answer 422 before the handler runs, which I confirmed with {"name":" "}. Remove it as well, or keep both and note why.

[deyna256]

Summary: every point from the previous review is addressed, the API contract is still unchanged and the suppression now actually suppresses. Approving.

Nit: the new commit is fix: address PR #68 review feedback, while CONTRIBUTING asks for #52: <change>. The squash merge takes the PR title, so nothing of it reaches main.

Checked (at a492efc):

• just format --check, just lint, just deps (no file changes) and just test pass; codex, execution and gateway also pass -race -count=5 -shuffle=on.
• golangci-lint 2.13.2 with staticcheck enabled no longer reports SA1019 for client.go or client_test.go. The three findings left in the package (one ST1013, two QF1001) are on older lines this PR does not touch.
• finish now takes a *Failure, so a caller cannot rebuild the nil-pointer case, and Next passes the value through unchanged.
• GET /api/openapi.json compared with the base commit again: still only the order of required in the create-client-key response, so the new doc comments did not leak into the schema.
• Dropping the blank-name check in createKey leaves the answers identical: {"name":""} and {"name":" "} return the same 422 body as on the base commit.
• cloneModel is now the single place that says which fields need a deep copy, and cloneModels uses it.
• One correction to my own review: the reasons for the two retained checks were already recorded in #52 before I asked for them. I should have read the issue's comments first.