Replace the vacuous no-redirect test in tests/test_ref_fetch.py with one that asserts the header on the wire

[jaylfc]

CARD TITLE (intent, not commit subject): Replace the vacuous no-redirect test in tests/test_ref_fetch.py with one that asserts the header on the wire

Autonomous build of board card tsk-ljvoh7.

Files:
tests/test_ref_fetch.py | 150 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 115 insertions(+), 35 deletions(-)

[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: 53 minutes

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: 22757681-2cd7-409e-ba01-79a32974325d

📥 Commits

Reviewing files that changed from the base of the PR and between ac8f714 and 9b55ff5.

📒 Files selected for processing (1)

• tests/test_ref_fetch.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

• 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]

CRITICAL: test_local_path_passes_data_dir_to_ref_fetch is incomplete — assertions were accidentally removed

The method body ends at ref = {...} with no call to svc.fetch_by_ref and no assertions. It will pass trivially without verifying that data_dir is forwarded or that the result is base64-encoded correctly. The removed lines were:

result = asyncio.run(svc.fetch_by_ref(ref, agent="test", data_dir="/tmp"))
assert captured["data_dir"] == "/tmp"
assert result["bytes"] == "aGVsbG8="

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	1
WARNING	0
SUGGESTION	0

Issue Details (click to expand)

CRITICAL

File	Line	Issue
tests/test_ref_fetch.py	378	test_local_path_passes_data_dir_to_ref_fetch is incomplete — assertions were accidentally removed

Files Reviewed (1 file)

• tests/test_ref_fetch.py - 1 issue

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 136.3K · Output: 25K · Cached: 335K}

[jaylfc]

Reviewed. This does what the card asked and I verified it by running the full red/green cycle, not by reading it. One required one-line change, which I have already tested for you.

The card bar is met

The vacuous test_fetcher_uses_no_redirect_opener is deleted - good, that was the point. The replacement stands up two real local servers, points the product fetch path at origin A, and asserts on the Authorization header recorded at origin B, which is the assertion the card demanded: about the header on the wire, not about the class of an opener.

And it genuinely defends the guard. Removing only _NoRedirect from the opener in service.py:384:

GREEN as shipped:                          1 passed
RED with build_opener(_NoRedirect) -> build_opener():   1 failed

That is a real red, which is exactly what the previous test could not produce.

Required change: the red is right, but it fails for the wrong reason

In the red state the failure is:

E  taosmd.ref_fetch.HashMismatchError: sha256 mismatch
   taosmd/ref_fetch.py:121

not the leak assertion. Once the redirect is followed, origin B returns b"ok", which does not match the expected sha256(b"hello world"), so HashMismatchError propagates out of asyncio.run(...) before assert len(auth_keys) == 0 is ever evaluated. The test is red by incidental exception, and the message points a future reader at test data rather than at a credential leak.

One-line fix, verified end to end:

-                self.wfile.write(b"ok")
+                self.wfile.write(b"hello world")

With that, in the red state the failure becomes the assertion itself:

E  AssertionError: Authorization header leaked to origin B with _NoRedirect! Keys found: [Authorization]
E  assert 1 == 0

and restoring _NoRedirect goes green again with the same payload. Full cycle proven both directions.

Two non-blocking notes

1. Part 2 does not exercise the product code. It hand-builds urllib.request.build_opener() with its own Request and a hardcoded Authorization: Bearer test-token, so what it proves is that urllib default openers follow redirects and forward headers - a property of the standard library, not of our fetch path. It is a useful sanity check that origin B can record a header at all, but the real defence is Part 1. Worth a comment saying so, otherwise it reads as the control when it is not; with the one-line fix above, Part 1 is the control.
2. time.sleep(0.05) after each serve_forever and a hardcoded data_dir="/tmp" - both fine in practice, both mild flake/portability risks if this test ever moves.

Recommending merge once the payload line is changed. Nothing else blocks it.