Uh oh!
There was an error while loading. Please reload this page.
test(api): decode-fidelity, not blanket non-zero, for the client/admin list contracts - #324
Merged
Merged
Conversation
…n list contracts assertProvisionedClient asserts every ProvisionedClient field decodes non-zero — the silent-drift class. That is right for the three hand-pinned single fixtures (create/adopt/patch), but TestContractListClientsPaginated applied it to every row of a real synced list (edge_device_list.json). status==0 is STATUS_OFFLINE and cluster_id is "Empty on legacy / not-yet-backfilled clients" (client.go), so the next re-sync of a list that includes an offline or unanchored row would fail CI even though decoding is correct. This is the Cursor Bugbot "contract test rejects valid values" finding on #323. Add assertDecodesFrom, used per-row by the list test. It catches BOTH silent drift directions without rejecting legitimate zeros: - struct-tag drift (a Go json tag renamed): the wire still carries the old key with a value while the struct decodes zero -> value mismatch -> caught. - backend rename/drop on re-sync (the wire field renamed, fixture reseeded): the old key vanishes, so a pure value compare reads zero on both sides. A presence assertion on the always-present keys (id/first_name/username/ namespace/status) catches this. cluster_id is presence-optional (legit empty), its rename stays covered by the strict single-fixture tests. status==0 is tolerated (key present, value 0). The single pinned fixtures keep the strict non-zero helper. Also harden TestContractListClientAdmins (surfaced by the same-class sweep): email is a domain-guaranteed identifier -> assert present + wire-match (a renamed email tag decodes "" -> caught); name is a display name that can be legitimately blank -> assert wire-fidelity only, not non-empty. Proven by mutation: struct-tag rename and backend key-rename both fail the list test; an offline/unanchored fixture row passes; a blank admin name passes; an admin email-key rename fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
@BugBot run |
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
👋 Heads-up — Code review queue is at 47 / 30 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b8fb9c4. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Fixes the Cursor Bugbot "contract test rejects valid values" (Medium) finding on #323 — plus one same-class sibling the verification sweep surfaced.
assertProvisionedClientasserts everyProvisionedClientfield decodes non-zero (a valid silent-drift check). That's right for the three hand-pinned single fixtures (create/adopt/patch), butTestContractListClientsPaginatedapplied it to every row of a real synced list (edge_device_list.json, reseeded byscripts/sync-backend-fixtures.sh). Perclient.go,status==0isSTATUS_OFFLINEandcluster_idis "Empty on legacy / not-yet-backfilled clients" — both legitimate. So the next re-sync of a list containing an offline or unanchored row would fail CI even though decoding is correct.Fix
New
assertDecodesFrom, used per-row by the list test, catches both silent-drift directions while tolerating legitimate zeros:json:tag renamed)id/first_name/username/namespace/status) catches itcluster_idis presence-optional (legitimately empty); its rename stays covered by the strict single-fixture tests.status==0is tolerated (key present, value0). The three pinned single fixtures keep the strict non-zero helper.Sibling (same-class, from the sweep):
TestContractListClientAdminsassertedname != ""for every admin row — butnameis a display name that can be legitimately blank;emailis the domain-guaranteed identifier. Hardened to assertemailpresent + wire-match andnamewire-fidelity only.Verification (mutation-proven)
cluster_id→ list test fails ✓namespace→k8s_namespaceon the fixture → list test fails via presence ✓status:0&cluster_id:""→ list test passes ✓name→ admins test passes ✓emailkey-rename → admins test fails ✓go vet,gofmt -s, goimports,file-budgetcleanOnce merged to
develop, #323 (develop → main promotion) inherits it and Bugbot re-clears.The presence-check design was caught by a 3-lens adversarial review of an earlier draft that value-compared all six fields — that draft silently missed the backend-rename-on-resync direction, the primary class these contract tests exist to catch.
Note
Low Risk
Test-only changes in contract tests; no production API or CLI behavior is modified.
Overview
List contract tests no longer require every
ProvisionedClientrow to decode non-zero.assertDecodesFromcompares each decoded list row to the raw fixture JSON: required keys must still be present on the wire (catches backend rename/drop on re-sync), and values must match (catches Gojsontag drift).status == 0and emptycluster_idare allowed on real synced list rows.TestContractListClientsPaginatedusesassertDecodesFromper row (mapped topage1.Results) instead ofassertProvisionedClient. Create/adopt/patch single-fixture tests still use the strict non-zero helper.TestContractListClientAdminsnow assertsemailis non-empty and matches the wire, andnamematches the wire only (blank display names are valid).Reviewed by Cursor Bugbot for commit b8fb9c4. Bugbot is set up for automated code reviews on this repo. Configure here.