Return the response on a failed http_request, not just its status code

[sanil-23]

Problem

format_response builds the status line, headers and body — then discards all of it on a non-2xx:

let output = format!("Status: {} {}\nResponse Headers: {}\n\nResponse Body:\n{}", …);
if status.is_success() {
    Ok(ToolResult::success(output))
} else {
    Ok(ToolResult::error(format!("HTTP {status_code}")))   // output dropped
}

Observed in a live agent run as HTTP 403 — twenty characters — eight times in a row. The body that was thrown away said exactly what was wrong:

Request forbidden by administrative rules. Please make sure your request has a
User-Agent header (https://docs.github.com/.../troubleshooting-the-rest-api#user-agent-required)

The caller had nothing to act on, and the failure was read downstream as an authentication problem — which it was not.

A second bug in the same block: the header line printed each name twice rather than name and value —

format!("{}: {:?}", k.as_str(), k.as_str())

so every entry read x-ratelimit-remaining: "x-ratelimit-remaining". The set-cookie redaction sitting beside it only makes sense if values were meant to be shown, which is the reading taken here.

Change

• return the same output on both arms
• print header values; widen the redaction from set-cookie to anything cookie- or authorization-shaped, since a response can hand back credentials

Testing

cargo test -p tinytools-std --lib network:: — 91 passed, 0 failed.

The existing contract test is rewritten against the module's real loopback server, now serving a genuine 403 with a body and a rate-limit header, and asserts all three properties:

assert!(text.starts_with("Status: 403"), "{text}");          // classifiable
assert!(text.contains("User-Agent header"), "{text}");        // the reason survives
assert!(lower.contains("x-ratelimit-remaining: 59"), "{text}"); // values, not names

cargo fmt -p tinytools-std -- --check clean.

Risk

Low, but it is a visible change: failures now carry the status line, headers and body instead of HTTP <code>. That is longer output on an error path, and the previously-pinned contains("HTTP 500") assertion no longer holds — hence the rewritten test. Header values are newly exposed, which is why the redaction list was widened at the same time.

🤖 Generated with Claude Code

[tinysweeper]

Tiny Sweeper review

Tiny Sweeper completed its review; deterministic results follow.

State: Ready for maintainer review
Priority: none
Reviewed head: cebfb6dcab5c
Updated: 1791458693 (Unix time)

Review snapshot

Change surface	Files	Review signal	Count
Production	1	Active findings	0
Tests	1	Noted findings	0
Documentation	0	Resolved findings	12
Configuration	0	Pending checks/questions	0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

No supported behavioral explanation was produced.

Features

• Modified — Full formatted response returned on non-success HTTP status: Callers of the http_request tool now see the status line, headers and body on failures (e.g. GitHub's 403 explaining the missing User-Agent header) instead of a bare string like HTTP 403; previously the only part of the response saying why was dropped. (crates/tinytools-std/src/network/http_request.rs#impl HttpRequestTool {)
• Modified — Response-header values shown only for an explicit safe allowlist: Header values are printed only for content-type, content-length, retry-after and x-ratelimit-* headers; all other response headers (including set-cookie, www-authenticate, proxy-authenticate, authentication-info and arbitrary service-specific names) are redacted as ***REDACTED***. This resolves the security lane's earlier concern that broad value exposure could leak credentials. (crates/tinytools-std/src/network/http_request.rs#impl HttpRequestTool {)
• Modified — Header rendering fixed to print name and value: The header block no longer prints each header name twice (the old (k.as_str(), k.as_str()) formatting); it now prints name: value for allowlisted headers, e.g. x-ratelimit-remaining: 59, with <binary> for unreadable values. (crates/tinytools-std/src/network/http_request.rs#impl HttpRequestTool {)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Resolved this pass

• Redact all credential-bearing response headers
• Redact sensitive response-header values before returning them
• Test the sensitive-header redaction now that values are printed
• Redact all credential-bearing response headers
• Redact sensitive response-header values before returning them
• Test the sensitive-header redaction now that values are printed
• Redact all credential-bearing response headers
• Redact sensitive response-header values before returning them
• Test the sensitive-header redaction now that values are printed
• Redact all credential-bearing response headers
• Redact sensitive response-header values before returning them
• Test the sensitive-header redaction now that values are printed

Before merge

None.

How this fits together

flowchart LR
  n0["HttpRequestTool<br/>changed"]:::changed
  n1["..._is_returned_as_the_error_without_a_retry<br/>changed"]:::changed
  n2["..._success_response_is_reported_as_an_error<br/>changed"]:::changed
  n3["test_tool"]:::impacted
  n4["execute_request"]:::impacted
  n5["format_response"]:::impacted
  n6["serve"]:::impacted
  n7["PaymentHook"]:::impacted
  n8["handle_payment_required"]:::impacted
  n0 -->|uses| n7
  n1 -->|calls| n3
  n1 -->|tests| n3
  n1 -->|calls| n4
  n1 -->|tests| n4
  n1 -->|calls| n6
  n1 -->|tests| n6
  n1 -->|uses| n7
  n1 -->|calls| n8
  n1 -->|tests| n8
  n2 -->|calls| n3
  n2 -->|tests| n3
  n2 -->|calls| n4
  n2 -->|tests| n4
  n2 -->|calls| n5
  n2 -->|tests| n5
  n2 -->|calls| n6
  n2 -->|tests| n6
  n3 -->|uses| n0
  n8 -->|calls| n4
  n8 -->|uses| n7
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff

Loading

Agent review details

critique

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Lane summary: The response-header allowlist, sensitive-value redaction, and non-success response rendering are implemented consistently with the surrounding API, and the added tests cover the changed behavior. The change looks safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

security

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The earlier redaction concerns are fixed: response-header values are restricted to an explicit safe allowlist while non-success diagnostics are preserved; the security lane considers the change safe to merge.
• Lane summary: The response formatting change now preserves non-success diagnostics while restricting response-header values to an explicit safe allowlist. The earlier redaction concerns are fixed, and this change looks safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

tests

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: All three earlier findings are addressed, with the redaction pinned by both a unit test on the allowlist and integration tests asserting secrets are absent; the tests would fail if redaction regressed or the error path dropped the body again. No new issues found.
• Lane summary: The change fixes the header formatting bug, redacts all non-allowlisted response-header values, and returns the full response text on error — all three earlier findings are addressed, with the redaction pinned by both a unit test on the allowlist and integration tests asserting secrets are absent. The behaviour changes are genuinely tested: the tests would fail if redaction regressed or the error path dropped the body again. No new issues found; looks safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

commits

• Conclusion: Neutral
• Scope reviewed: all assigned evidence
• Lane summary: Nothing sensitive found in what this pull request commits.

description

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The description accurately matches the diff: header values are shown only for a small allowlist of diagnostic headers, with unit and loopback tests covering both the safe list and redacted ones, and the error path returns the full formatted response as described.
• Lane summary: The earlier redaction concerns are resolved: header values are now shown only for a small allowlist of diagnostic headers, with unit and loopback tests covering both the safe list and the redacted ones, and the error path now returns the full formatted response as the description says. The description accurately matches the diff; the change looks sound to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

e2e

• Conclusion: Neutral
• Scope reviewed: all assigned evidence
• Lane summary: No end-to-end harness in this repository: no e2e test files and no e2e workflow.

Evidence and run details

• Models: gpt-5.6-luna, glm-5.3-flash
• Spend: $0.010456
• Tokens: 118181 input · 4539 output · 9848 cached · 0 embedding

Head	State	Pass summary
e9765301681c	changes requested	3 active finding(s), 0 resolved finding(s) (at 1791396478)
cebfb6dcab5c	ready for maintainer review	0 active finding(s), 12 resolved finding(s) (at 1791458693)

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 53 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e337a495-7f0c-4067-8bbd-d3429cd9c05f

📥 Commits

Reviewing files that changed from the base of the PR and between 68163b1 and cebfb6d.

📒 Files selected for processing (2)

• crates/tinytools-std/src/network/http_request.rs
• crates/tinytools-std/src/network/http_request_tests.rs

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[tinysweeper]

Requesting changes: 1 lane(s) blocking, worst finding is high.

Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.

             $0.0041 · 82,395 in / 6,317 out · 9,604 cached (12%) · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0013 · 18,528 in / 1,918 out · 2,116 cached (11%) · gpt-5.6-luna
security:    $0.0027 · 49,471 in / 2,516 out · 7,488 cached (15%) · gpt-5.6-luna
tests:       $0.0001 · 5,355 in  / 580 out   · 0 cached (0%)      · glm-5.3-flash
description: $0.0000 · 5,310 in  / 162 out   · 0 cached (0%)      · glm-5.3-flash

[tinysweeper]

The previously-blocking findings are resolved. Clearing the changes request.

             $0.0105 · 118,181 in / 4,539 out · 9,848 cached (8%)  · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0050 · 41,189 in  / 1,313 out · 4,232 cached (10%) · gpt-5.6-luna
security:    $0.0052 · 54,248 in  / 1,103 out · 5,616 cached (10%) · gpt-5.6-luna
tests:       $0.0001 · 6,621 in   / 240 out   · 0 cached (0%)      · glm-5.3-flash
description: $0.0001 · 6,576 in   / 176 out   · 0 cached (0%)      · glm-5.3-flash