Repository navigation
fix: reject truncated and non-success API responses - #60
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 15, 2026, 3:15 AM ET / 07:15 UTC. ClawSweeper reviewWhat this changesThe shared Places and Routes HTTP client rejects oversized responses and final non-2xx statuses, with regression tests and documentation of the resulting errors. Merge readiness⛔ Blocked before merge - 1 item remains This remains a useful, focused fix: current main and v0.4.11 retain both defects, and no replacement implementation was identified. No actionable patch defect was found. Priority: P2 Review scores
Verification
How this fits togetherThe shared HTTP client receives API responses for the Go library and CLI, then passes successful payloads to workflow-specific JSON decoders. Its errors prevent result output and produce CLI exit code 1. flowchart TD
A[Library or CLI request] --> B[Shared HTTP client]
B --> C[API response]
C --> D[Check size and HTTP status]
D -->|Valid response| E[Decode workflow result]
D -->|Rejected response| F[Return diagnostic error]
E --> G[Library result or CLI output]
F --> H[Library error or CLI exit 1]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep response validation centralized, preserve valid responses through exactly 1 MiB, and retain the documented upgrade behavior and before/after CLI coverage. Do we have a high-confidence way to reproduce the issue? Yes, from source: serve valid JSON padded to the old 1 MiB boundary followed by more data, or return JSON with final HTTP 302. Current main can decode these as success; this read-only review did not execute the scenarios. Is this the best way to solve the issue? Yes. Reading one extra byte and validating the final status in the existing shared request path is a narrow repair that avoids duplicating checks across workflows. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against f408ca16e16a. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Reviewed the compatibility note and accepted the documented response validation fix. Decoding final non-2xx statuses as successful Places/Routes results, or silently discarding bytes beyond the existing size cap, was accidental behavior. The shared client now reports these failures consistently, while valid responses through exactly 1 MiB and followed same-origin redirects retain their behavior. Custom endpoints relying on the accidental success behavior should return a valid 2xx response within the existing limit. The before/after built CLI fixtures independently reproduced both defects, the new regression tests pass, and independent review found no actionable P0–P2 defect. CI run https://github.com/openclaw/goplaces/actions/runs/34940613504 passed on |
1 participant
The shared HTTP client silently discarded bytes after its 1 MiB read limit. A valid JSON prefix followed by whitespace and additional data could therefore produce a successful result from a malformed response. It also decoded final HTTP 300/302 bodies as successful place results and reported HTTP 304 as an empty response, losing the upstream status.
Read one extra byte to detect overflow, reject oversized successful responses, and decode results only for final 2xx statuses. Non-2xx responses retain their status in
APIError; oversized error bodies use a concise size diagnostic. This applies to every Places and Routes workflow through the shared request path. The existing payload limit and redirect policy remain in place. Documentation and the Unreleased changelog describe the corrected behavior.Validation:
http.ErrUseLastResponse; all pass afterward.make lint test coveragepassed (93.1% total coverage);go test -race ./...,go vet ./..., andgo build ./...passed.