feat(net): AuthSource + Auth/SetHeaders layers + Guarded body (Slice 0 PR 3)#66
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Guarded<B>, the permit-carrying http_body::Body wrapper that releases an optional async_lock::SemaphoreGuardArc concurrency permit at the earlier of stream-end (poll_frame's terminal None) or drop. Forwards is_end_stream/size_hint to the inner body per the wrapper-transparency rule. Adds the async-lock workspace dependency.
Replace all four #[allow(clippy::significant_drop_tightening)] with #[expect] and concise justifications. Verify via clippy -D warnings that each suppression is load-bearing — all four lint-fire cases were confirmed and retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds the HTTP auth seam and header layers, a Guarded response-body wrapper with async-lock permit handling, public API re-exports, and related ADR, spec, plan, and changelog updates. ChangesAuth/SetHeaders/Guarded construction surface
Suggested labels: 🚥 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 (2)
docs/superpowers/specs/2026-06-30-net-http-construction-surface-design.md (1)
465-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the branch name from the ADR-numbering note.
The
docs/adr-ws-transport-stackreference is a local workflow detail and will age out quickly. Keep the 0034 reservation, but generalize the WebSocket note so the spec stays stable after the branch disappears.♻️ Suggested edit
- ADR-0034** that records them and the construction-surface decisions? (The repo has - landed 0029–0031 append-only; leaning a standalone ADR. **Note:** 0032/0033 are taken by - the WebSocket transport pair on branch `docs/adr-ws-transport-stack`, so this workstream's - ADR is **0034**, not 0032.) + ADR-0034** that records them and the construction-surface decisions? (The repo has + landed 0029–0031 append-only; leaning a standalone ADR. 0032/0033 are reserved by the + WebSocket transport pair, so this workstream's ADR is **0034**, not 0032.)🤖 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 `@docs/superpowers/specs/2026-06-30-net-http-construction-surface-design.md` around lines 465 - 468, The ADR-numbering note in the spec includes a transient branch name that will age out and should be removed. Update the wording around the ADR reservation so it still states that the workstream is ADR-0034 and that 0032/0033 are already taken by the WebSocket transport pair, but generalize the WebSocket reference without mentioning docs/adr-ws-transport-stack. Locate the note in the spec section discussing ADR-0034 and revise that sentence only.crates/adapter/net/http/api/src/body.rs (1)
141-150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winError terminal frame doesn't release the permit eagerly.
poll_frameonly clearspermitwhenframe.is_none(). ASome(Err(_))frame also practically ends the stream but leaves the permit held untilDrop. Not a leak, just a delayed release for error paths — worth aligning with the "earliest of stream-end or drop" intent.♻️ Proposed fix
let this = self.project(); let frame = ready!(this.inner.poll_frame(cx)); - if frame.is_none() { + if !matches!(frame, Some(Ok(_))) { // Eager release at stream-end: a fully-read but still-held body // must not keep hold of one of the venue's concurrency slots. // Dropping the guard is synchronous and runtime-free. *this.permit = None; } Poll::Ready(frame)🤖 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/api/src/body.rs` around lines 141 - 150, The eager-release logic in `Body::poll_frame` only drops `permit` on `None`, so an `Some(Err(_))` terminal frame keeps the slot until `Drop`. Update the `poll_frame` path to also clear `this.permit` when `this.inner.poll_frame(cx)` returns an error frame, while preserving the existing behavior for `None`, so `Body` releases the permit at the earliest stream termination.
🤖 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 `@crates/adapter/net/http/api/src/body.rs`:
- Around line 105-111: Guarded currently only releases its semaphore permit from
poll_frame, so if the wrapped body is already ended at construction time the
permit can remain held until Drop. Update Guarded::new and/or the Guarded body
flow in body.rs to eagerly release the permit when inner.is_end_stream() is
already true, using the existing Guarded and SemaphoreGuardArc handling. Add a
test covering the already-ended body case to verify the permit is released
without requiring any polling.
---
Nitpick comments:
In `@crates/adapter/net/http/api/src/body.rs`:
- Around line 141-150: The eager-release logic in `Body::poll_frame` only drops
`permit` on `None`, so an `Some(Err(_))` terminal frame keeps the slot until
`Drop`. Update the `poll_frame` path to also clear `this.permit` when
`this.inner.poll_frame(cx)` returns an error frame, while preserving the
existing behavior for `None`, so `Body` releases the permit at the earliest
stream termination.
In `@docs/superpowers/specs/2026-06-30-net-http-construction-surface-design.md`:
- Around line 465-468: The ADR-numbering note in the spec includes a transient
branch name that will age out and should be removed. Update the wording around
the ADR reservation so it still states that the workstream is ADR-0034 and that
0032/0033 are already taken by the WebSocket transport pair, but generalize the
WebSocket reference without mentioning docs/adr-ws-transport-stack. Locate the
note in the spec section discussing ADR-0034 and revise that sentence only.
🪄 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: 9a1514dd-0ff3-4195-b8e1-03aeb61c9c10
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdCargo.tomlcrates/adapter/net/http/api/Cargo.tomlcrates/adapter/net/http/api/src/auth.rscrates/adapter/net/http/api/src/body.rscrates/adapter/net/http/api/src/lib.rsdocs/adr/0030-http-transport-contract-wire-bytes-streaming-composition.mddocs/adr/0031-http-resilience-venue-pacing.mddocs/adr/0034-http-construction-surface-auth-guarded-boot-coverage.mddocs/superpowers/plans/2026-07-02-net-http-auth-guarded.mddocs/superpowers/specs/2026-06-30-net-http-construction-surface-design.md
Addresses CodeRabbit review on #66: - release the concurrency permit at construction when the wrapped body is already ended (a consumer may observe is_end_stream() and never poll); - release on an error terminal frame too, not only on None; - drop the transient WS branch name from the spec's ADR-numbering note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai all three findings validated and addressed in d374704 (
Both new tests were RED-verified (they fail without the fix). |
|
Tip For best results, initiate chat on the files or code changes.
Good call on making (=^･ω･^=) ﾉ゛ |
…arded # Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml
1 participant
Closes #64
Slice 0 PR 3 of the net-http construction surface (spec: docs/superpowers/specs/2026-06-30-net-http-construction-surface-design.md; plan: docs/superpowers/plans/2026-07-02-net-http-auth-guarded.md).
What's here
HttpError::Auth,Guardedreplacing 0031 §3'sPermitenum, boot-time total pacing coverage, §4 HTTP-status passthrough).AuthSource(RPITIT,Send-bounded) +NoAuth+ theAuthlayer — credentials stamped on the final request innermost, so a futureRetry(Slice 1) re-signs per attempt; authorize failures classify asErrorKind::Authand short-circuit before the inner service.SetHeaders— static default headers just outsideAuth; dynamic credentials win on collision (caller-set < static defaults <Auth).Guarded<B>— the concurrency permit rides the response body (async-lock, runtime-neutral), released at the earlier of stream-end or drop; forwardsis_end_stream/size_hint(a defaultedsize_hintwould make a max-size guard fail open).Notes
net-http-apistays free of any async runtime — the only added lib dep isasync-lock(runtime-agnostic). Notokio/hyper/reqwest/serde.poll+ realasync_lock::Semaphore), proving runtime-neutrality; both permit-release paths (eager terminal-frame + early drop) are pinned.just cigreen. Delivered subagent-driven TDD with per-task + whole-branch review.Next: PR 4 (
RateKey+RateLimitConfig<K>+ boot-time coverage validation) closes Slice 0.🤖 Generated with Claude Code
Summary by CodeRabbit
AuthSourcecredential-stamping seam, per-attempt freshness, and header precedence viaSetHeaders.Guardedstreaming response body that holds a concurrency permit until terminal frame read or early drop.async-lockworkspace dependency.