Skip to content

Revise PR #310: the banner tests assert against a copy of the logic they re-implement, and three are shadowed duplicates - #319

Merged
jaylfc merged 1 commit into
masterfrom
exec/tsk-6xrv7d
Aug 17, 2026
Merged

Revise PR #310: the banner tests assert against a copy of the logic they re-implement, and three are shadowed duplicates#319
jaylfc merged 1 commit into
masterfrom
exec/tsk-6xrv7d

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): Revise PR #310: the banner tests assert against a copy of the logic they re-implement, and three are shadowed duplicates

Autonomous build of board card tsk-6xrv7d.

Reuse the PR #310 production change as-is in taosmd/http_server.py: serve()
now prints an "A2A registry auth mode: OFF|ENFORCE|WARN (verify-and-warn)"
line, and strips "(no auth)" from the where line only when a registry_url
is configured with enforcement on.

The previous banner tests re-implemented serve()'s mode/where logic inside
the test body and asserted on their own copy, so all six passed against
unfixed master. Three were also shadowed duplicates (defined twice with the
first binding silently discarded).

Rewrite the banner tests as subprocess probes that capture serve()'s real
stdout, guarded by a PROVENANCE marker confirming the exercised module. Six
cases cover the three mode states (OFF, ENFORCE, WARN) and the three LAN-bind
where-line cases. Verified RED against unfixed http_server.py (6 failed: the
"A2A registry auth mode:" line was absent), then GREEN after the banner
change (6 passed). Full suite: 1441 passed, 12 skipped, 0 failed.

Files:
changelog.d/tsk-6xrv7d-a2a-banner-tests.md | 7 ++
taosmd/http_server.py | 11 ++
tests/test_http_server.py | 156 +++++++++++++++++++++++++++++
3 files changed, 174 insertions(+)

…() output

Reuse the PR #310 production change as-is in taosmd/http_server.py: serve()
now prints an "A2A registry auth mode: OFF|ENFORCE|WARN (verify-and-warn)"
line, and strips "(no auth)" from the where line only when a registry_url
is configured with enforcement on.

The previous banner tests re-implemented serve()'s mode/where logic inside
the test body and asserted on their own copy, so all six passed against
unfixed master. Three were also shadowed duplicates (defined twice with the
first binding silently discarded).

Rewrite the banner tests as subprocess probes that capture serve()'s real
stdout, guarded by a PROVENANCE marker confirming the exercised module. Six
cases cover the three mode states (OFF, ENFORCE, WARN) and the three LAN-bind
where-line cases. Verified RED against unfixed http_server.py (6 failed: the
"A2A registry auth mode:" line was absent), then GREEN after the banner
change (6 passed). Full suite: 1441 passed, 12 skipped, 0 failed.
@qodo-code-review

Copy link
Copy Markdown

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

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 54 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: 03fcdbbc-5f69-4139-9405-318885a21458

📥 Commits

Reviewing files that changed from the base of the PR and between 568bd33 and 2fdde51.

📒 Files selected for processing (3)
  • changelog.d/tsk-6xrv7d-a2a-banner-tests.md
  • taosmd/http_server.py
  • tests/test_http_server.py

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

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

@gitar-bot

gitar-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

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

kilo-code-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • taosmd/http_server.py
  • tests/test_http_server.py
  • changelog.d/tsk-6xrv7d-a2a-banner-tests.md

Reviewed by step-3.7-flash · Input: 86.4K · Output: 38K · Cached: 1.6M

@jaylfc

jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

Review: MERGE

This fixes what #310 was blocked on, and it fixes it at the root rather than by patching the symptom. Every claim below was measured on this branch at 2fdde51.

The carry-forward is intact, checked as a deliverable in its own right

#310's production diff was correct and verified at the time, so the card told this revision to reuse it unchanged. That instruction is invisible in this PR's own diff, so it has to be checked against the branch being revised:

git diff origin/exec/tsk-v53vta 2fdde51 -- taosmd/   ->  (empty)

Byte-identical. The mode banner and the (no auth) strip are carried forward exactly, nothing was silently dropped or re-derived.

The red control, reproduced independently

This is the whole point of the card, so I did not take the PR body's word for it. I took the merged tree, reverted only taosmd/http_server.py to master's version, left the new tests in place, and confirmed the feature was genuinely absent first:

grep -c 'A2A registry auth mode' taosmd/http_server.py   ->  0

6 tests against unfixed production   ->  6 failed
6 tests against the fixed tree       ->  6 passed

That is the exact inverse of #310, where the same six cases passed against unfixed master and therefore measured nothing. These tests have real coverage.

The reason they work is that they no longer re-implement anything. _serve_banner runs serve() in a real subprocess and reads its actual stdout, and the assertions compare against literal expected strings:

assert _mode_line(lines) == (
    "A2A registry auth mode: OFF (no registry_url: senders are self-claimed)"
)

If production changes that string, the test fails. That is what was missing before.

_require_provenance is a good addition and worth naming: each subprocess prints PROVENANCE <module __file__> and the test asserts it equals http_server.__file__. That closes the standing trap where standing in a worktree does not bind the interpreter to it, which has produced false passes in this repo more than once.

The shadowed duplicates are gone, and the gate confirms it independently

#310 defined three test functions twice, so three of nine were silently discarded. The six tests here are renamed and unique. Rather than eyeball that, I ran the duplicate-definition gate that merged an hour ago in #316, which exists precisely to catch this and fires on #310's branch naming all three:

scripts/normalise_handle_gate.py on this merged tree  ->  clean, exit 0

The rest

Suite on a trial merge with fc44ec6: 1479 passed, 12 skipped. Baseline is 1473 and this adds 6 tests, so the count is exactly accounted for. The PR body reports 1441, which was correct against its own base 0bf8c9f (1435 + 6) and is stale only because master has moved twice since.

No conflict markers. Deleted-symbols gate clean from inside the merged tree.

One non-blocking note for whoever touches this next: _serve_banner stops reading when it sees a line starting with Admin (admin token required). If that banner line is ever renamed, these tests will not fail fast, they will each read until the 15 second timeout. Worth a sentinel that does not depend on unrelated banner text, but not worth holding this.

Merging.

@jaylfc
jaylfc merged commit 09afa91 into master Aug 17, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant