Skip to content

test: enforce internal, domain, and crypto coverage gates - #125

Open
csnitker-godaddy wants to merge 2 commits into
mainfrom
fix/coverage-gates
Open

csnitker-godaddy wants to merge 2 commits into
mainfrom
fix/coverage-gates

Conversation

@csnitker-godaddy

Copy link
Copy Markdown
Member

make test-cover now enforces the documented coverage requirements separately: at least 90% for internal/, 100% for internal/domain, and 95% for internal/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, and make test-race pass 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.

Signed-off-by: Connor Snitker <csnitker@godaddy.com>

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.

🟡 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")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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
kperry-godaddy previously approved these changes Sep 21, 2026

@kperry-godaddy kperry-godaddy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 package github.com/agentnameservice/ans/scripts (the import path has no trailing slash), so it now lands in -coverpkg, go vet and 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_InvalidEndpoint passes an empty displayName, trips MISSING_DISPLAY_NAME, and never reaches eps.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 asserting ENDPOINT_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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

Suggested change
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

test: enforce the documented internal, domain, and crypto coverage gates

3 participants