Skip to content

fix: harden lifecycle, integrations, and release checks - #35

Merged
junioryono merged 12 commits into
mainfrom
codex/fix-audit-findings
Jul 18, 2026
Merged

fix: harden lifecycle, integrations, and release checks#35
junioryono merged 12 commits into
mainfrom
codex/fix-audit-findings

Conversation

@junioryono

@junioryono junioryono commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

  • harden registration snapshots, constructor identity, multi-interface aliases, build cancellation, scope ownership, and deterministic disposal
  • normalize HTTP adapter options and error paths, sanitize Huma errors without dropping status headers, and add real cross-router Huma composition tests
  • make all Go modules first-class in CI, coverage, linting, security, dependency updates, documentation, benchmarks, and atomic release automation

Compatibility notes

  • prebuilt values are singleton-only; transient void and variadic constructors are rejected
  • repeated Close calls wait for and return the same cleanup result; recursive owner close remains unsupported
  • Echo and Fiber centrally render downstream errors while the request scope is alive, so recovery and error observers belong inside the scope middleware
  • unexpected plain Huma errors are sanitized; explicit status errors and approved response headers are preserved

Migration guidance is included in MIGRATION.md and docs/guides/v4-to-v5.md.

Verification

  • all nine Go modules: build, vet, race tests, tidy checks, and golangci-lint 2.12.2
  • coverage enforcement: core 87.8%; runtime adapters 89.5%-98.8%
  • strict Sphinx build with warnings as errors
  • published integration graphs tested without local replace directives
  • gosec 2.27.1 and govulncheck 1.6.0 across every module; no reachable vulnerabilities
  • full package and comparison benchmark suites at CI settings (1s, count 3)
  • actionlint, immutable Action SHA checks, module/Dependabot inventory checks, release tag-set validation, and release dry runs

Copilot AI review requested due to automatic review settings July 10, 2026 04:09
Copilot AI reviewed Jul 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the type: bug Something isn't working label Jul 10, 2026
junioryono and others added 4 commits July 9, 2026 21:16
@junioryono
fix(ci): resolve dependency and workflow validation failures
bf46006
@junioryono @claude
refactor: align PR with project conventions
6667bb5 
Core:
- extract registerAliases to match sibling registration methods and drop
  the descriptorList type
- collapse the newScope wrapper trio into newScope + newUninitializedScope
- inline single-use descriptorInstanceKey; document analyzer cache retention

Tests:
- restructure the three contract test files into grouped t.Run subtests
  with t.Parallel and the shared testutil fixtures; remove the
  package-level alias constructor counter

Adapters:
- huma: default ErrorMapper is identity (Handle already sanitizes);
  use errors.AsType per codebase convention
- chi: port the scope-disposal test from http for parity
- gin: document abort-on-error behavior on ScopeMiddleware
- integrationtests: align app.Test usage and add a cross-router
  error-sanitization sweep

CI:
- release.yml: drop the push-tag trigger (tag.yml already invokes it)
- test.yml: single-source golangci-lint version from the Makefile, drop
  dead tool env vars, run coverage via check-coverage.sh, build docs via
  make, dedupe the Ubuntu root-test run, and scope
  published-compatibility to non-PR events
- check-coverage.sh: support per-module runs with kept profiles
- check-format.sh: guard the empty file-list case
- add .gitattributes to keep LF normalization

Docs:
- document the As pointer-implements rejection in MIGRATION.md and the
  v4-to-v5 guide

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix(docs): let make docs bootstrap the venv or use a provided sphinx-…
c09f4ce 
…build

The html target never depended on the venv, so make docs failed on any
fresh checkout (and in CI). Bootstrap the venv when using its
sphinx-build, and let CI override SPHINXBUILD to its provisioned
interpreter.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix: address confirmed findings from multi-agent review
f83c2dd 
Core:
- close orphaned constructor results through the disposable identity set
  so a value shared across sibling registrations closes exactly once
  when Close races createInstance; retain disposableSet after Close
- provider.trackDisposable closes (once) instead of leaking a disposable
  registered after provider Close
- ContainsKeyed/RemoveKeyed use value-level comparability so interface-
  wrapped non-comparable keys cannot panic

Adapters:
- normalizeConfig copies the middleware slice instead of compacting the
  caller-owned backing array in place (all five adapters)
- opt-in panic recovery re-panics http.ErrAbortHandler instead of
  converting a deliberately aborted response into a 500 (net/http-backed
  adapters)
- huma: sanitizeControllerError no longer mutates a status error whose
  chain carries headers (previously duplicated header values on every
  request), and headers on sanitized plain errors are preserved

Tests:
- build cancellation tests are constructor-driven instead of racing
  wall-clock deadlines against Build setup
- snapshot isolation test asserts via a removed transient so singleton
  caching cannot mask a regression
- regression tests for the orphan dedup, slice mutation, ErrAbortHandler,
  and huma header fixes

Scripts/docs:
- release notes: hash-first log format survives '|' in subjects; real
  newlines instead of printf %b so subjects with backslashes are intact
- README: correct godi/huma/v5 path in prose

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
ccoVeille
ccoVeille reviewed Jul 10, 2026
View reviewed changes

@ccoVeille ccoVeille left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, there are way too much changes in this PR.

It should be split.

Comment thread Makefile
SHELL := /usr/bin/env bash

GOLANGCI_LINT_VERSION ?= v2.12.2
GOSEC_VERSION ?= v2.27.1

@ccoVeille ccoVeille Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to install gosec while golangci-lint ships it ?

Comment thread huma/huma_test.go Outdated
var se huma.StatusError
require.ErrorAs(t, err, &se)
assert.Equal(t, http.StatusInternalServerError, se.GetStatus())
if model, ok := se.(*huma.ErrorModel); ok {

@ccoVeille ccoVeille Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be reported if OK is false.

It's not checked right now

junioryono and others added 7 commits July 16, 2026 21:48
@junioryono @claude
test(huma): require the sanitized error's concrete type before asserting
f2e58de 
The *huma.ErrorModel assertion was silently skipped when the type
assertion failed, so the empty-details check could pass vacuously.

Addresses review feedback on #35.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix: exclude test modules from the published-compatibility matrix
4c22935 
integrationtests composes the sibling adapters and asserts this branch's
behavior; with replace directives dropped it runs against published
adapter versions and fails on main and releases until those versions
exist. Only integration modules belong in the published check.

Found by Codex review of #35 (make published-check failed).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix: treat typed-nil interface Out fields as unprovided
ebe52ab 
A typed-nil pointer stored in an interface field reports IsNil() ==
false on the interface, so ProcessResultObject cached it as a valid
service and resolution returned a typed nil. Unwrap interfaces before
the nil skip so it matches the directly-nil field semantics.

Found by Codex review of #35.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix: use value-level key comparability in GetKeyed
1ef3fec 
A comparable struct wrapping a non-comparable value in an interface
field passed the type-level check and panicked in the map lookup;
mirrors the earlier ContainsKeyed/RemoveKeyed fix.

Found by Codex review of #35.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
fix(scripts): detect BREAKING CHANGE footers in release notes
84d0264 
The subject-only log format meant the footer check could never match;
inspect each commit's full message instead.

Found by Codex review of #35.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
Merge branch 'main' into codex/fix-audit-findings
0bb3e2f 
Resolve workflow conflicts by keeping the SHA-pinned layout and bumping
actions/checkout pins to the v7 commit from main's Dependabot update.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011toBdwdaHL4T3wbewN3BP9
@junioryono @claude
test: pair contract tests with the files they cover
75ef1c6 
The audit work added build_contract_test.go, lifecycle_contract_test.go,
and registration_contract_test.go, none of which had a corresponding
source file. The root package otherwise pairs every X_test.go with an
X.go, so these broke the convention.

Redistribute each test to the file implementing the behavior it covers:

- Build cancellation/context and all registration tests -> collection_test.go
  (Build and the registration APIs are on *collection)
- Disposable close dedup, concurrent close, close error aggregation, and
  root scope initializers -> provider_test.go (disposal tracking lives in
  provider.trackDisposable)
- Scope cancellation cleanup -> scope_test.go

No test bodies changed and no tests were dropped; the package still
exports the same 71 test functions. Also document the pairing rule in
CONTRIBUTING.md so it does not regress.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013ArexH3Dgxun8wYj4MxQ5T
@junioryono
junioryono merged commit 22b01d1 into main Jul 18, 2026
37 checks passed
@junioryono
junioryono deleted the codex/fix-audit-findings branch July 18, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
@ccoVeille ccoVeille ccoVeille left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@junioryono @ccoVeille