test(sca,workflows): pin routes, request bodies and wire tags - #69
Open
aaearon wants to merge 2 commits into
Open
test(sca,workflows): pin routes, request bodies and wire tags#69aaearon wants to merge 2 commits into
aaearon wants to merge 2 commits into
Conversation
Close the SCA / workflows / models rows of the mutation ledger. sca: record route/body/params in mockHTTPClient; assert the exact route for every endpoint and the full POST body contents; replace the tautological TestListSessions_WithCSPFilter with an assertion on the sent csp query param; pin the on-demand target_category and pageSize literals; cover context propagation and the paginated decode errors; extract the X-API-Version guard into its own test; pin the sca/uar ISP service slugs via the constructed BaseURL. workflows: drive a 500 through all six checkResponse call sites; assert the exact finalize route and FinalizationReason; pin the limit query param and defaultPageSize; add logging_client_test.go (including the Authorization redaction guard) and service_config_test.go. models: pin the requestDetails, result, cancelReason and roleId request tags, and decode a populated accessCredentials off the wire through ParseAWSCredentials; assert the parsed values in TestParseAWSCredentials. No production code changed.
…rdicts Twelve PR8 rows named tests that the implementer consolidated away. Point each row at the test that actually exists, and say explicitly where several rows now share one test rather than repeating a name with no note. SCA-18 was recorded as OVERSTATED/refuted on a "not reproducible" verdict that came from a sandbox without loopback TCP. It has now been reproduced on both services (inbound requests = 4, want 1), so it flips to CONFIRMED/test naming the two retry-policy tests as its killers. SCA-15's identical clause is dropped: both X-API-Version mutants were executed and both fail. Record the reusable lesson in the preamble: a bare deletion of sdkclient.DisableTransientRetry orphans the import and is a compile kill, not a test kill; the honest mutation is the assignment form. Summary counts updated. Also: soften two test comments that claimed to replace assertions still present elsewhere, and assert the error message in the credentials "empty string" row so the previously inert empty-string guard is killed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 8 of 8. Base:
test/isolation-harness(#63) — must merge first. Tests and docs only; no production change.The headline
Renaming the
accessCredentialsJSON tag oninternal/sca/models/elevate.gopassed the entire repo suite. It is the single fieldgrant envexists to deliver — rename it and every AWS elevation silently returns nil credentials. The existing tests only ever decoded"accessCredentials": null, and the service tests set the field to nil in Go structs the test itself marshalled, so the tag round-tripped trivially.Now decoded from a raw JSON literal off the wire through
ParseAWSCredentials, asserting all three values.Also
checkResponsecall sites ininternal/workflowscould be deleted undetected — the existing test only called the helper directly. A 500 decoded as an emptyAccessRequest, sogrant request submitprinted a blank request and exited 0. The 500 fixture is deliberately designed to decode cleanly into every response model, so removing a guard yields a genuine silent success rather than an incidental decode error.internal/workflows/logging_client.gohad no test file at all, including its Authorization-redaction path, while the byte-identical SCA one was fully covered. It is on the live path.TestListSessions_WithCSPFilterwas tautological — it asserted its own canned response.grant status --provider azurecould have been filtering nothing.FinalizeRequestcould drop the approver's reason and lose the request ID from the route; the test asserted onlyHasSuffix(route, "/finalize").X-API-Version: 2.0guard lived insideTestNewSCAAccessServiceDisablesTransientRetry, so a retry-motivated rename would have silently deleted it. Now its own named test.A won't-fix that became a fix
The ISP service slug was unpinned because both retry tests overwrite
client.BaseURLbefore issuing a request. I had allowed a documented won't-fix; the implementer pinned it instead, arguing the failure mode is total — every live request to the wrong host. Correct call.Recorded for the next reviewer
Two rows were previously marked "not reproducible" because a Codex sandbox blocked loopback TCP. Both reproduce fine here. The ledger preamble now records that a loopback-blocked sandbox cannot refute a row, and that a bare deletion of
DisableTransientRetryis a compile kill, not a test kill — the honest mutation is_ = sdkclient.DisableTransientRetry. That confusion cost two verification passes.29/30 mutations killed (the survivor pre-existing and inert). Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed — ledger only; no Go file changed.