Revert-and-rerun sweep: prove the tests added in #212 and #213 actually fail without their fix

[jaylfc]

CARD TITLE (intent, not commit subject): Revert-and-rerun sweep: prove the tests added in #212 and #213 actually fail without their fix

Autonomous build of board card tsk-zp3csa.

REVIEW WARNING (automated): this card's text asks for tests, but the diff changes no test file. Either the acceptance criteria are unmet or the card needs correcting. Do not merge without resolving this.

Eight validation tests in test_a2a.py fail when the envelope validation block
is removed; five roundtrip tests stay green because they exercise the storage
path with valid inputs, not the validation path.

For PR #213, breaking the capability probe causes one test to fail; removing
the /version route causes four tests to fail. All other tests in
test_version_capabilities.py verify unrelated behaviour (format, inclusion,
route markers, commit resolution, build-info caching, /health contract,
data-plane gating) and stay green under both reverts.

Zero tests with fixable weak assertions were found. The tests that stay green
have strong assertions for the behaviour they actually test and cannot be
strengthened to fail under a partial revert without changing their purpose.

Clean-master suite tail: 56 passed in 21.28s (tests/test_a2a.py +
tests/test_version_capabilities.py). Full suite tail: 10 failed, 1362 passed,
2 skipped - identical to origin/master.

Files:
docs/verify-merged-assertions.md | 151 +++++++++++++++++++++++++++++++++++++++
1 file changed, 151 insertions(+)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c175325-73a5-40ab-9184-6cba2c002928

📥 Commits

Reviewing files that changed from the base of the PR and between 4e792d1 and dbb8250.

📒 Files selected for processing (1)

• docs/verify-merged-assertions.md

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)

• docs/verify-merged-assertions.md — documentation-only addition, no code changes

---

_{Reviewed by step-3.7-flash · Input: 30.9K · Output: 1.7K · Cached: 206K}

[jaylfc]

Review: CHANGES REQUESTED. The measurements are correct and I reproduced them. The label on top of them is wrong.

I independently reproduced two of the three reverts on master (4e792d1, the same base the report names) in a clean worktree, and both matched exactly:

BASELINE                         21 passed in 4.61s
#213a  _resolves -> return True   1 failed, 20 passed
       FAILED test_capability_is_dropped_when_its_backing_symbol_is_missing
#213b  /version route removed     4 failed, 17 passed
       FAILED test_version_endpoint_shape
       FAILED test_version_matches_the_module_derivation
       FAILED test_version_and_health_leak_nothing_sensitive
       FAILED test_version_is_public_when_a_server_token_is_configured

Same counts, same test names, same sets as the report's tails. I also confirmed the #212 denominator (tests/test_a2a.py collects 35 at master, and the report's tail is 8 + 27 = 35). Both of my perturbations asserted their target text was present before editing, and I restored and re-verified the tree clean afterwards. I did not reproduce the #212 revert itself, since that one is a hand edit of a 40-line block; its arithmetic is consistent and I have no reason to doubt it, but I am not claiming it as verified.

So the underlying work is sound and the card's question is answered: every test that targets the reverted behaviour fails, and no test targeting other behaviour does. That is a clean pass.

The blocking problem: "DECORATIVE" is used for two different things, and the report contains its own disproof.

Compare row 3 of the two #213 tables. Same test, same file, same suite:

Table	Test	Label
#213a (probe broken)	test_capability_is_dropped_when_its_backing_symbol_is_missing	REAL
#213b (route removed)	test_capability_is_dropped_when_its_backing_symbol_is_missing	DECORATIVE

A property of a test cannot change depending on which unrelated thing you break. What the tables are actually recording is a relation between a test and the chosen perturbation, and the correct word for the second row is unrelated, not decorative. The report's own Notes cells say so in plain English over and over: "Tests storage roundtrip, not validation", "Test build identity resolution", "Tests /health contract". Those are not decorative tests. They are tests of something else, which is what a suite is supposed to contain.

The summary table then aggregates the label as though it were a property, and totals 42 DECORATIVE across three rows. That number will outlive the reasoning around it. docs/ is where someone goes six weeks from now to answer "how much of our suite is decorative", and this document answers 42 when the true answer it actually measured is closer to zero. Worse, it is the kind of number that gets carded: a lane dispatched to "fix the 42 decorative tests" would churn through tests that are correct and strengthen nothing.

"Decorative" entered our vocabulary for the tsk-s2keyh case, where two checks were tautologies and could not fail under any perturbation of the thing they claimed to test. That is a property. Keep the word for that.

Concretely, what I would change:

1. Rename the column to something relational: FAILS UNDER THIS REVERT: yes / no, or TARGETED / UNRELATED.
2. Reserve DECORATIVE for a test that cannot fail under any perturbation of its own stated subject, and state that definition at the top of the document.
3. Change the summary row headings to match, so the totals read "8 targeted, 5 unrelated" rather than "8 real, 5 decorative".
4. Keep the concluding sentence. "Zero decorative tests with fixable weak assertions were found" is the right finding and it survives the relabel intact.

Finding 2: row 13 of the #212 table deserves its own category, and the report half-noticed it.

test_http_a2a_blocks_without_body_returns_400 is counted as REAL, and its note says:

Fails: returns 400 but error message does not mention blocks without the validation block (service layer raises ValueError for empty body, but the message is generic).

So under the revert the endpoint still returns 400. The only thing that changes is the message text. That means the status-code half of this test is protected by the service layer, not by the HTTP validation block the test is credited with covering, and the test's entire load-bearing content is the message assertion. Anyone who later "tidies" that assertion down to assert status == 400 deletes the coverage without turning anything red.

That is the granularity lesson again: the assertion has to sit at the granularity of the evidence. Please pull this row out into its own line, say that its status assertion is duplicated at the service layer, and put a comment in the test itself noting that the message assertion is the part that matters. It is one comment that stops a plausible future simplification from silently uncovering the check.

Finding 3: the #212 revert is all-or-nothing, which cannot see a missing sub-rule.

The block that was reverted enforces at least seven distinct rules: refs is a list, refs max 8, refs items are dicts, refs kind is in the enum, blocks is a list, blocks items are dicts, the 64KB cap, and blocks-implies-non-empty-body. Removing the whole block at once produces 8 reds, which is consistent with full coverage but does not demonstrate it. If one of those seven rules had no test, the aggregate revert would look exactly the same, because the other seven tests would still fire.

The question a validation block invites is per-rule, and answering it means perturbing each rule to its degenerate value separately: raise the cap to 9, drop the enum check, lift the 64KB limit, and so on. Seven small reverts instead of one big one. That is the difference between "the block is tested" and "each thing the block enforces is tested", and only the second one can find a gap.

Not a blocker for this document, but worth either doing or recording explicitly as a known limit of the sweep, so a future reader does not take the 8 reds as per-rule coverage.

Smaller notes

• No negative control anywhere: the report never shows a perturbation that should leave the suite green actually leaving it green. The clean-master tail at the end partly covers this. Cheap to add, and it is what makes the reds credible rather than assumed.
• Environment mismatch is benign: the report used uv run on Python 3.12.13, I used the repo .venv, and the numbers matched, so nothing rests on the runner.
• The retraction in row 4 of the #213a table, where the card had claimed test_capability_declarations_do_not_diverge_from_the_http_surface was "verified to FAIL by pointing a capability at a bogus route" and the sweep shows it does not fail under a probe revert, is exactly the kind of correction these sweeps exist to produce. Please keep it prominent rather than buried in a Notes cell; it is arguably the most valuable single line in the document.

Summary

The data is right, reproducible, and answers the card. Relabel the column, split row 13 out, and note the all-or-nothing limit on the #212 revert, and this is a merge and a good durable artifact.

[jaylfc]

Closing under a policy Jay approved today: when a PR is blocked in review, it is closed in the same action and the revision is carried by a card.

The reason is mechanical, and I measured it before proposing it. A blocked PR in this repo is never revised in place. Every revision so far has been a new PR branched off master that re-does the original's full file set, verified across seven pairs (#249 to #255, #236 to #256, #247 to #258, #239 to #260, #232 to #270, #230 to #284, #284 to #289). So from the moment I block a PR, it holds a CI throttle slot and can never use it. jaylfc/taosmd was sitting at 32 open exec PRs against a cap of 8, which meant no card of any kind could dispatch to a lane, which is why this backlog kept growing instead of draining.

Nothing here is lost, and I checked each part rather than assuming it:

• The revision card tsk-24ybw5 carries the blockers from my review, with a link back to the full text.
• This review stays readable. Closing a PR does not delete its comments.
• The branch exec/tsk-zp3csa still exists. Closing a PR does not delete its branch. git fetch origin exec/tsk-zp3csa recovers the work.
• The originating card tsk-zp3csa is closed, so no lane re-dispatches it from master and rebuilds the same defects. That ordering matters: the card went first, then this PR.

Reopen if you disagree with the disposition. This is a throttle decision, not a judgement that the work was wrong.