Skip to content

fix: make HTTP body capture completion-aware - #800

Open
magurotuna wants to merge 12 commits into
mainfrom
fix/request-sampler-completion
Open

fix: make HTTP body capture completion-aware#800
magurotuna wants to merge 12 commits into
mainfrom
fix/request-sampler-completion

Conversation

@magurotuna

Copy link
Copy Markdown
Member

Summary

  • synchronize body sampling and record complete, incomplete, or aborted state
  • emit a full-body SHA only when the body is complete
  • preserve denied and partial-read request captures
  • reject fixtures built from incomplete, lossy, redacted, or transformed bodies
  • show partial capture state in the dashboard

Why

An early response could snapshot a body while it was still being written. Fixture export could also treat a partial or changed body as the original request.

Tests

  • go test ./...
  • focused race tests
  • dashboard tests, format, lint, and build

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

🚀 Preview deployed: https://demo.clawpatrol.dev/pr-preview/pr-800/

Built from ee5ea9e915ad6c8cc32bdb8f60f53c9324b8e633 at 2026-08-17T09:35:03Z.

@magurotuna
magurotuna requested a review from avocet-bot August 17, 2026 09:29
@avocet-bot

avocet-bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Avocet review status

State: Completed
Target: fcf9cf50156f
Requested by: magurotuna
Model: Claude Opus
Attempt: 1 of 3
Updated: 2026-08-17T09:50:03.744Z
Formal review: #800 (review)

The formal GitHub review is the authoritative result. This comment only reports operational status.

@magurotuna
magurotuna marked this pull request as ready for review August 17, 2026 09:35

@avocet-bot avocet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review model: Claude Opus

⚠️ Review of an earlier PR revision.

Reviewed base: b2a84fae0ec02bb1ec3ee7bf28f8487dfe8ad5d6
Reviewed commit: fcf9cf50156f188f6fe58e14697522a8806b08d8
Current PR base: e0350a79a20197f0fba894080f4ecad8a1950e7a
Current PR head: ee5ea9e915ad6c8cc32bdb8f60f53c9324b8e633
Verdict for the reviewed commit: APPROVE (informational only)

This review applies only to the reviewed comparison above.
Newer commits and their integration with the current base were not reviewed.
Re-request review to review the current PR revision.

No blocking findings. The body-capture lifecycle feature is correct, cohesive, and thoroughly tested.

Verified in depth:

  • SQL column/placeholder/value counts balance across Sink.drain (29), loadAction (28), readTailEvents (25), and dev_seed.go (25); req_transformed correctly relies on its DEFAULT 0 where omitted.
  • Migrations 0020/0021 are sound — 0020 omits an explicit _schema insert but the migrate runner's catch-all (db.go:132) records the version, so ordering and idempotency hold.
  • The mutex-guarded sampler state machine keeps aborted terminal, gates SHA to complete && n>0, and fails closed on short-EOF, read-error, close-before-EOF, and content-length overrun. Read errors in bufferHTTPBodyForMatchResult now surface truncated=true+readErr instead of the previous fail-open nil,false.
  • validateHTTPFixtureBodyCapture rejects every unfaithful capture (incomplete/aborted/legacy-unknown/truncated/content-encoded/binary/decoded-truncated/redacted/transformed/non-UTF8); happy path and each rejection branch are covered.

Non-blocking suggestions:

  • web.go:3086 — captureState()'s default maps unknown states to complete (fail-open). If a fourth samplerState is ever added, prefer an exhaustive switch defaulting to incomplete to stay consistent with the PR's fail-closed intent.
  • web.go:3010 — the bodyTruncatedMarker doc points to HttpBody for marker stripping, but that logic moved to splitBodyCapture in dashboard/src/lib/bodyCapture.ts; update the cross-reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants