test(cmd): pin the machine-facing JSON output contracts - #66
Open
aaearon wants to merge 7 commits into
Open
Conversation
aaearon
force-pushed
the
test/argument-capture
branch
from
August 16, 2026 07:42
63633cf to
b69451d
Compare
Add assertJSONEqual and one whole-object contract test per machine-facing document: status, list, cloud and group elevation, env credentials and the favorites list. Inline expected JSON, deliberately brittle against added fields so a new field forces a compatibility review. Also add the list -> request submit round trip: the emitted target NAME feeds --target and the emitted roleId feeds --role-id, and both must resolve back to the same eligible target. Fixture values are all distinct and self-describing on purpose; a swap mutation is invisible when both sides hold the same string.
- favorites: DirectoryID is persisted on both the unified-selector and the --type groups path, verified by resolving the saved favorite back through findMatchingGroup against two same-named groups in different directories - favorites: non-default default_provider, --provider precedence over the target CSP, parseFavoritesAddFlags validation, remove arity - list: assert Cobra's actual mutual-exclusion error (the old test passed on an unrelated runtime error), --provider suppresses groups, --refresh - status: directory-name merge precedence, stale session-timestamp cleanup, and an exact "remaining: 45m" instead of a prefix a sixfold error satisfied - request: text and JSON field mappings (target/role columns, created vs updated attribution, timeFrom/timeTo)
All 32 PR5 rows plus OUT-27 reverified with -count=1: mutation applied, test fails, mutation reverted, test passes. Test-name and scope corrections noted in the rows where the plan's placeholder name or fixture did not survive contact (OUT-09/10 fold into the status contract test, OUT-23's fixture, and OUT-25, which covers flag registration rather than a cache bypass). Record the output-contract and distinct-fixture conventions in CLAUDE.md.
… panic Three adversarial-review findings on PR5's output contracts. 1. TestListJSON_RoundTripsToRequestSubmit's second leg stubbed resolveSubmitTargetFn with a textual copy of the resolveSubmitTarget matcher, so it pinned nothing: changing --target to resolve on WorkspaceID instead of WorkspaceName survived the whole cmd suite. Extract matchWorkspaceByName (behaviour-preserving) and call it from both production and the test, so the mutation now fails. 2. TestRequestList_TextFieldMapping guarded len(fields) < 6 while indexing fields[6]; a column-blanking mutation panicked and aborted the entire cmd test binary. Raise the guard to < 7. 3. Narrow pinRemainingSeconds' lower bound from 2600 to 2695 so a whole-minute arithmetic error dies at the pin itself, and label the deliberately impossible favorites provider/CSP fixture as such.
Both machine-facing documents were entirely unpinned: every test unmarshalled into the very output struct under test, so renaming requests, sessionId or outcome survived. outcome is the single classification field callers switch on.
Every fixture populated credentials, workspaceName, username and directory, so dropping omitempty from any of them was invisible. Assert the key is absent, not null or empty.
Name the nine pinned documents instead of claiming blanket coverage, and add OUT-30..38 to the mutation ledger.
aaearon
force-pushed
the
test/output-contracts
branch
from
August 16, 2026 07:44
59776c2 to
c2704f8
Compare
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 5 of 8. Base:
test/argument-capture(#65) — it provides the capture infrastructure. #63 must merge first.Why
--output jsonis the documented machine/LLM-facing surface, and it was barely asserted. All of these shipped green: provider case flipped, duration zeroed,roleId/workspaceIdblanked, cloud-vs-group type swapped, target/role swapped,SecretAccessKey/SessionTokenswapped, andDirectoryIDdropped from group favorites on both write paths.Note the asymmetry that existed: the env text path was pinned by
TestEnvCommand_AWSSuccess; the env JSON path with identical data was not.What
assertJSONEqual— whole-object comparison for one representative contract test per output, where brittleness against added fields is desirable because it forces a conscious compatibility review. Focused tests retained for conditional fields. Fixture values are deliberately distinguishable (a swap of two identical values is undetectable), with a comment saying so.35 mutations, all killed.
One production change, and why the plan's "no production code" rule was overridden
TestListJSON_RoundTripsToRequestSubmitoriginally stubbed its second leg with a textual copy of the production matcher. A copy pins nothing — mutatingrequest_submit.goto resolve onworkspaceIdinstead of the emitted name survived the wholecmdsuite, while the test's name, its comment, and the ledger all claimed that guarantee was proven.Fixed properly:
matchWorkspaceByNameextracted and called from both. The mutation now fails. Recording a guarantee that does not exist is worse than an open row.Honest limits
remainingSecondsderives fromtime.Now(), so it is range-checked then pinned before the whole-object compare. Verified this still catches a dropped field, a renamed tag, a wrong type, and a sixfold unit error.TestListCommand_RefreshFlagRegisteredcovers registration and parsing only — the--refresh→buildCachedListerwiring sits behindbootstrapSCAServiceand is unreachable from unit tests. The test comment and ledger both say so. (--refreshis not a production no-op; that earlier claim was refuted.)len(fields) < 6before readingfields[6]) made a test panic under a real regression, which aborts the wholecmdbinary. Fixed to< 7.Adversarial review performed (Codex credits exhausted; review by a Claude agent). Both blocking findings fixed.