Problem
Three minor hygiene items left by commit 2fcea42 (PR #1161), flagged as non-blocking advisories by its native review:
- The audit-insert deadline test band (
requestStarted.Add(requestAuthAuditInsertTimeout - 25ms) to +250ms) uses asymmetric magic numbers with no named constants or rationale. The asymmetry is intentional (the insert context is created after requestStarted, so the lower bound is tight; the upper bound carries CI latency slack), but nothing in the code says so.
TestRequestAuthDenyReasonClassifiesPrincipalValidationErrors overlaps the mapping-table subtests (token_principal_mismatch, invalid_stored_principal) of TestRequestAuthDeniedAuditReasonMapping.
- The
ErrTokenPrincipalMismatch vs ErrInvalidPrincipal split introduced in 2fcea42 has a real classification contract (mismatch -> token_principal_mismatch; validation failures -> resolver_error) that lives only in test assertions, with no doc comment at the sentinel in internal/cloud/auth/foundation.go.
Evidence
internal/cloud/cloudserver/cloudserver_test.go:1927-1928 (tolerance bounds), :2138 (classification test).
internal/cloud/auth/foundation.go:58 (ErrTokenPrincipalMismatch).
- Review advisories: asymmetric tolerance constants, classification test duplication, mismatch sentinel split contract.
Proposed direction
Name the tolerance bounds as constants with a one-line rationale each, fold the overlapping classification assertions into the mapping-table subtests (keeping one authority), and document the sentinel split contract next to both sentinels.
Scope
Tests and comments only; no behavior change.
Problem
Three minor hygiene items left by commit 2fcea42 (PR #1161), flagged as non-blocking advisories by its native review:
requestStarted.Add(requestAuthAuditInsertTimeout - 25ms)to+250ms) uses asymmetric magic numbers with no named constants or rationale. The asymmetry is intentional (the insert context is created afterrequestStarted, so the lower bound is tight; the upper bound carries CI latency slack), but nothing in the code says so.TestRequestAuthDenyReasonClassifiesPrincipalValidationErrorsoverlaps the mapping-table subtests (token_principal_mismatch,invalid_stored_principal) ofTestRequestAuthDeniedAuditReasonMapping.ErrTokenPrincipalMismatchvsErrInvalidPrincipalsplit introduced in 2fcea42 has a real classification contract (mismatch ->token_principal_mismatch; validation failures ->resolver_error) that lives only in test assertions, with no doc comment at the sentinel ininternal/cloud/auth/foundation.go.Evidence
internal/cloud/cloudserver/cloudserver_test.go:1927-1928(tolerance bounds),:2138(classification test).internal/cloud/auth/foundation.go:58(ErrTokenPrincipalMismatch).Proposed direction
Name the tolerance bounds as constants with a one-line rationale each, fold the overlapping classification assertions into the mapping-table subtests (keeping one authority), and document the sentinel split contract next to both sentinels.
Scope
Tests and comments only; no behavior change.