feat(net): HTTP contract — HttpError, HttpClient, ResponseBody + net-http-mock (Slice 0 PR 2)#60
Merged
Merged
Conversation
Final-review Minor: lib.rs doc listed error/client but not service/body, and the header still said the data plane 'lands in later slices' after PR 2 added it. Refs #59
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds HTTP data-plane contracts to Changesnet-http contract and mock harness
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/adapter/net/http/mock/src/body.rs (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
try_from/unwrap_oris defensive but effectively dead code.
usizenever exceedsu64on any Rust-supported target, so theunwrap_or(u64::MAX)fallback is unreachable. A plainf.len() as u64would be simpler with identical behavior.♻️ Optional simplification
- .map(|f| u64::try_from(f.len()).unwrap_or(u64::MAX)) + .map(|f| f.len() as u64)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/adapter/net/http/mock/src/body.rs` around lines 50 - 57, The `size_hint` implementation in `Body` uses an unreachable `try_from`/`unwrap_or(u64::MAX)` fallback when summing `frames`; simplify it by using a direct `u64` conversion for each frame length and keep the existing exact total behavior. Update the logic in `size_hint` only, and leave `SizeHint::with_exact(total)` unchanged.crates/adapter/net/http/mock/src/client.rs (2)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: accept
impl Into<Bytes>forok.Slightly nicer ergonomics for callers passing
&'static [u8]orVec<u8>literals without an explicitBytes::from_static/Bytes::from.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/adapter/net/http/mock/src/client.rs` around lines 29 - 32, Update the mock HTTP client helper `MockResponse::ok` to accept `impl Into<Bytes>` instead of requiring a `Bytes` argument, so callers can pass `Vec<u8>` or `&'static [u8]` directly. Keep the behavior the same by converting the input to `Bytes` inside `ok` before calling `Self::new`, and leave the `new` constructor unchanged.
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe the poison-recovery lock pattern.
Same
.lock().unwrap_or_else(PoisonError::into_inner)sequence appears twice. Could extract a small private helper, but the duplication is trivial and low-risk here.Also applies to: 56-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/adapter/net/http/mock/src/client.rs` around lines 38 - 40, The poison-recovery lock sequence is duplicated in the mock HTTP client, so dedupe the repeated .lock().unwrap_or_else(PoisonError::into_inner) pattern to keep `Client` cleaner. Extract a small private helper in `crates/adapter/net/http/mock/src/client.rs` and use it from the affected methods around the shared lock access in the `Client` implementation, including the other occurrence noted in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-01-net-http-contract.md`:
- Line 174: The module-list doc entry for the new `error` module has a malformed
inline code span around `HttpError`, which markdownlint flags and will render
incorrectly. Update the `//! - [`error`] — ...` line in the `lib.rs` module-list
documentation so the `HttpError` name is wrapped with a valid inline code span
and the markdown syntax is balanced.
---
Nitpick comments:
In `@crates/adapter/net/http/mock/src/body.rs`:
- Around line 50-57: The `size_hint` implementation in `Body` uses an
unreachable `try_from`/`unwrap_or(u64::MAX)` fallback when summing `frames`;
simplify it by using a direct `u64` conversion for each frame length and keep
the existing exact total behavior. Update the logic in `size_hint` only, and
leave `SizeHint::with_exact(total)` unchanged.
In `@crates/adapter/net/http/mock/src/client.rs`:
- Around line 29-32: Update the mock HTTP client helper `MockResponse::ok` to
accept `impl Into<Bytes>` instead of requiring a `Bytes` argument, so callers
can pass `Vec<u8>` or `&'static [u8]` directly. Keep the behavior the same by
converting the input to `Bytes` inside `ok` before calling `Self::new`, and
leave the `new` constructor unchanged.
- Around line 38-40: The poison-recovery lock sequence is duplicated in the mock
HTTP client, so dedupe the repeated
.lock().unwrap_or_else(PoisonError::into_inner) pattern to keep `Client`
cleaner. Extract a small private helper in
`crates/adapter/net/http/mock/src/client.rs` and use it from the affected
methods around the shared lock access in the `Client` implementation, including
the other occurrence noted in the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 903c4af7-a58b-4127-842a-12c357a38fb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mdCargo.tomlcrates/adapter/net/http/api/Cargo.tomlcrates/adapter/net/http/api/src/body.rscrates/adapter/net/http/api/src/client.rscrates/adapter/net/http/api/src/error.rscrates/adapter/net/http/api/src/lib.rscrates/adapter/net/http/mock/Cargo.tomlcrates/adapter/net/http/mock/src/body.rscrates/adapter/net/http/mock/src/client.rscrates/adapter/net/http/mock/src/lib.rscrates/adapter/net/http/mock/src/timer.rsdocs/superpowers/plans/2026-07-01-net-http-contract.md
- Fix malformed inline code span in the plan doc module-list note (MD038). - Simplify MockBody::size_hint to a direct `as u64` sum (usize→u64 never truncates; the try_from/unwrap_or(u64::MAX) fallback was unreachable). - Accept `impl Into<Bytes>` in MockClient::ok for nicer call-site ergonomics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`Sleep::poll` registered a fresh waker on every pending poll, so a `Sleep` polled more than once before its deadline (e.g. inside a `select!` woken by a sibling future — the pattern the ADR-0033 WS resilience layer will use) stacked duplicate waiter entries and woke the task once per entry. Guard the registration on `(deadline, will_wake)`: skip when this exact waker is already queued for this deadline. Deadline alone is not identity (unrelated futures can share one), so both must match — a re-poll of the same future is a no-op while a distinct future still registers its own waker. Add a regression test asserting one registration and exactly one wake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Extract a private `lock()` helper in the mock crate for the `.lock().unwrap_or_else(PoisonError::into_inner)` poison-recovery pattern, replacing its six repetitions across MockClient and MockTimer. - Collapse ResponseBody::poll_frame's Buffered arm from a four-branch `Poll` match to a single `Poll::map` that maps the `Infallible` body error away. No behavior change; /simplify cleanup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Applies the CodeRabbit nitpick that was missed in 8eb0e4a: usize→u64 never truncates, so the try_from/unwrap_or(u64::MAX) fallback was unreachable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
This was referenced Jul 3, 2026
This was referenced Jul 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 2 of Slice 0 (the net-http foundation) — gives
oath-adapter-net-http-apiits HTTP data-plane contracts and ships the standaloneoath-adapter-net-http-mocktest harness. Builds on #58.HttpError— one concrete transport/middleware error +HasErrorKind. Model A: HTTP 4xx/5xx statuses are not error-ified — they pass through asOk(Response)with the body intact for the adapter to classify (ADR-0030 §5); the resilience layers will peekstatus()/Retry-After.HttpClient— a blanket-impl'dServicesub-trait withsendsugar; any backendServiceof the right shape isHttpClientfor free (RPITIT, nodyn).ResponseBody<B>— apin-project-litebuffer-xor-stream enum that forwards all threeBodymethods (poll_frame/is_end_stream/size_hint) to the active arm — a defaultedsize_hint/is_end_streamwould silently mis-size a caller's.collect()and defeat any max-size guard.BufferMode(Copy).oath-adapter-net-http-mock— standalone harness:MockBody(frame-controllable body),MockTimer(virtual clock with a waker registry —advancedrivesnowand wakes due sleepers; lock-drop-before-wake, no deadlock/lost-wake),MockClient(canned-responseServiceleaf that records requests). Consumed downstream via[dev-dependencies]only — no dev-dep cycle.Out of scope (later slices/PRs)
AuthSource/Auth/Guarded(PR 3),RateKey/coverage (PR 4), the resilience layers +stack/buildassembly + hyper leaf (later slices), theCircuitOpenHttpErrorvariant (lands with the CircuitBreaker layer).Verification
just cigreen (fmt, fmt-toml, typos, lint, check, test, deny, doc, machete, gitleaks, actionlint, shellcheck), 31 tests.net-http-apistays runtime-free — notokio/hyper/reqwest/serdein its production graph (tokio/http-body-utilare dev-deps). Nounsafe; nounwrap/expectin non-test code (Mutexpoison recovered viaPoisonError::into_inner).Process
Subagent-driven: 4 tasks, each spec + code-quality reviewed (one Important test-gap on
ResponseBody'sis_end_streamforwarding was caught and fixed), plus a clean whole-branch review (one doc-list omission fixed).Closes #59
🤖 Generated with Claude Code
Summary by CodeRabbit
HttpErrorfor unified transport/middleware failures (with error-kind mapping).HttpClientwithsendconvenience, plusResponseBodyandBufferModeto support buffered vs streaming HTTP response bodies.oath-adapter-net-http-mocktest harness:MockClient,MockBody, and deterministicMockTimer.CHANGELOG.mdand added an implementation-plan document for the net-http HTTP contract work.