test: enforce internal, domain, and crypto coverage gates - #125
csnitker-godaddy wants to merge 2 commits into
Conversation
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
There was a problem hiding this comment.
🟡 Changes recommended
internal/crypto/deployment_validation_test.go cannot compile as written because it accesses unexported helpers from package crypto.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds separate coverage gates for internal, domain, and crypto code, plus related regression tests and defensive-branch documentation.
Changes:
- Adds AWK-based coverage thresholds and exclusions.
- Wires coverage validation into
make test-cover. - Adds domain/crypto regression tests and synthetic coverage tests.
File summaries
| File | Summary |
|---|---|
scripts/coverage_test.go |
Tests coverage thresholds, exclusions, missing packages, and duplicate observations. |
scripts/check-coverage.awk |
Implements coverage aggregation and thresholds. |
Makefile |
Runs the coverage checker. |
internal/domain/agent_test.go |
Adds endpoint host-mismatch coverage. |
internal/crypto/proofinput.go |
Documents defensive branches. |
internal/crypto/jws.go |
Documents defensive branches. |
internal/crypto/jws_test.go |
Tests tampered RSA payload rejection. |
internal/crypto/jwk.go |
Documents defensive key-encoding branches. |
internal/crypto/deployment_validation_test.go |
Adds crypto validation regressions, but currently references unexported helpers from another package and fails compilation. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| func TestJWSSigningRejectsMalformedInputsAndSignerOutput(t *testing.T) { | ||
| km := newMemKM(t, "key") |
There was a problem hiding this comment.
Both jws_test.go and deployment_validation_test.go declare package crypto_test, so these helpers are in the same test package. No visibility change is needed.
kperry-godaddy
left a comment
There was a problem hiding this comment.
Approving. The awk gate does what #121 asks: statement-weighted floors over internal/, once-per-block union across the cross-package profile, and a hard failure when domain or crypto is missing from the profile. The motivation holds up on the numbers: the base passes the old aggregate check at 90.7% total while domain sits at 99.84% and crypto at 94.14%, and the new gate fails that profile and passes this branch at 91.02 / 100.00 / 98.45. All nine SAFETY/NOTE annotations trace to the guard they cite.
Nothing here blocks. Three things worth tidying while the files are open: one inline with a suggestion, two below because they sit outside the diff.
Outside this diff
Makefile:83— not a blocker:grep -v -e '/scripts/'no longer excludes the new test-only root packagegithub.com/agentnameservice/ans/scripts(the import path has no trailing slash), so it now lands in-coverpkg,go vetand lint, and the comment at 71-82 describes an exclusion that no longer happens.-e '/scripts'matches both the package and its children; the comment could also name the three floors instead of "the 90% gate".internal/domain/agent_test.go:277-291— not a blocker:TestNewRegistration_InvalidEndpointpasses an empty displayName, tripsMISSING_DISPLAY_NAME, and never reacheseps.Validate, which is why that block was uncovered before this PR. The new table row at 95-97 is the real test; deleting the old one (or giving it a displayName and assertingENDPOINT_HOST_MISMATCH) removes a test that claims coverage it does not provide.
While we're in here: the internal floor comes from COVERAGE_THRESHOLD while 100 and 95 are literals in the awk, repeated in the FAIL text and in the test's expected string. Either pass all three from the Makefile or write the 90 beside the 95 so there is one place to read. The single "90% gate" wording in README.md, CLAUDE.md:212 and ci.yml can follow in the same change or a named follow-up.
| statements[$1] = $2 | ||
| if ($3 > 0) hit[$1] = 1 | ||
| } | ||
| END { |
There was a problem hiding this comment.
Not a blocker: minimum is never validated, so an empty, unset or non-numeric value coerces to 0 in the comparison at line 30 and the internal floor passes at any coverage while still printing "Coverage thresholds passed." Today the Makefile always supplies it, so the trigger is make test-cover COVERAGE_THRESHOLD= or a later edit that drops the -v; failing closed costs four lines:
| END { | |
| END { | |
| if (minimum !~ /^[0-9]+(\.[0-9]+)?$/ || minimum + 0 <= 0) { | |
| print "FAIL: minimum must be a positive percentage (pass -v minimum=N)" | |
| exit 1 | |
| } |
A row in TestCoverageGate that omits -v minimum and expects exit 1 pins it.
There was a problem hiding this comment.
The gate now rejects missing, empty, nonnumeric, nonpositive, and greater-than-100 thresholds. The package exclusion and misleading endpoint test are corrected, and the documentation names all three coverage floors.
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
make test-covernow enforces the documented coverage requirements separately: at least 90% forinternal/, 100% forinternal/domain, and 95% forinternal/crypto. Command wiring remains excluded from the denominator, missing required packages fail, and repeated observations of a coverage block are counted once.Added domain and crypto regressions and documented the defensive crypto branches that cannot be reached through the preceding guards. Synthetic coverage profiles verify each threshold, exclusions, missing packages, and duplicate observations.
Validation:
make build,make check, andmake test-racepass on this branch. Coverage is internal 91.01%, domain 100.00%, crypto 98.45%.First PR in the deployment-fix stack. No public API or wire-format changes.
Fixes #121.
AI assistance
Assisted-by: Codex (GPT-6), under Connor Snitker's direction.