Skip to content

fix(packets): give each observation its own wire bytes in the detail API (port of upstream #2055) - #77

Merged
dborup merged 4 commits into
masterfrom
port/upstream-2055-obs-raw-hex
Sep 26, 2026
Merged

dborup merged 4 commits into
masterfrom
port/upstream-2055-obs-raw-hex

Conversation

@dborup

@dborup dborup commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Port of upstream Kpa-clawbot/CoreScope#2055, which fixes upstream issue #1999.

Problem

/api/packets/{hash} returns the transmission's canonical raw_hex for every observation. Observations of one transmission reach observers along different paths and carry different path bytes, so the packet detail hex view can contradict the path_json shown beside it (for example, three hops in the path but two path bytes in the frame). Upstream measured this on a production DB: of the recent transmissions with more than one observation, 93% hold genuinely different frames.

The fork has the same defect:

Fix

  • cmd/server/db.go: new ObservationRawHexForHash. It returns the stored frame per observation id in one indexed query: transmissions.hash, then observations.transmission_id. It is guarded by hasObsRawHex().
  • cmd/server/routes.go: handlePacketDetail backfills once per request, after the store lock is released.
    • A stored per-observation frame wins, which is required because enrichObsWithTx has already put the canonical frame into every map.
    • Observations with no stored frame fall back to the canonical one. That also fills the DB-fallback path, which previously returned no raw_hex on observations at all.
  • The store's memory optimisation is untouched, and there is no frontend change.

Differences from upstream

  • hasObsRawHex() is a self-healing schema flag in this fork (a method), not a bool field. Code and tests are adapted: hasObsRawHexFlag.forceTrue().
  • The fork has no prepared stmtTxByHash, so the hash lookup uses a plain QueryRow, like the existing DB.GetObservationsForHash.
  • The contradictory upstream comment ("bytes already present win … not overwritten", followed by "a stored frame WINS") is reduced to the part that matches the code.

Tests

cmd/server/obs_raw_hex_test.go comes from upstream, adapted to the flag API. It covers:

  • the per-id mapping, including an observation with no stored frame;
  • the schema-flag guard;
  • two handler regressions, one on the DB-fallback path and one on the in-memory store path. Each asserts that every observation gets its own frame, that the frameless observation gets the canonical fallback, and that at least three distinct frames come back.

Results:

  • Without the routes.go change: both handler regressions fail.
  • With it: all five related tests pass, and the full cmd/server suite passes with -race (165s). go vet is clean, gofmt is clean on the touched files, and git diff --check is clean.

Not live-verified: the committed e2e fixture has no observations.raw_hex column, so the behaviour is verified at handler level only, not in a browser against real per-observation frames. Staging or real data would be the place to see it in the hex view.

Not in scope, same as upstream

  • The load and ingest paths still SELECT o.raw_hex and discard it.
  • fetchResolvedPathForObs still runs one query per observation.
  • Other endpoints (lists, WebSocket) still carry the canonical frame.

Not a hot path: one indexed query per packet-detail request.

🤖 Generated with Claude Code

dborup and others added 3 commits September 22, 2026 16:17
Port of upstream Kpa-clawbot/CoreScope PR Kpa-clawbot#2055 (upstream issue Kpa-clawbot#1999).

The store drops observations.raw_hex (Kpa-clawbot#881) on the belief that one
content hash means one frame. Observations of one transmission carry
different path bytes, so /api/packets/{hash} served the canonical frame
for every observation and the hex view could contradict the path shown
beside it.

- db.go: ObservationRawHexForHash reads the stored frame per observation
  id in one indexed query, guarded by hasObsRawHex().
- routes.go: handlePacketDetail backfills after the store lock is
  released; a stored frame wins, the canonical frame is the fallback
  (also filling the DB-fallback path, which had no raw_hex at all).

Fork adaptations: hasObsRawHex() is a self-healing schema flag here, not
a bool field; the hash lookup uses a plain QueryRow (no stmtTxByHash);
the contradictory "bytes already present win" comment from upstream is
dropped.

Tests: obs_raw_hex_test.go (upstream, adapted to the flag API). The two
handler regressions fail without the routes.go change and pass with it;
full cmd/server suite passes with -race.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dborup

dborup commented Sep 25, 2026

Copy link
Copy Markdown
Owner Author

Review feedback addressed (commit 11dc8c3)

  1. Merged current master into the feature branch with ordinary merge commit 650136a (no rebase or force-push).
  2. Changed ObservationRawHexForHash to return real query/scan/iteration errors while preserving no-error fallback for an absent optional column, empty hash, and unknown transmission.
  3. Packet detail now logs the database failure and returns HTTP 500 instead of silently presenting canonical bytes as observation-specific bytes.
  4. Added regression tests for both the DB error contract and the HTTP 500 handler path.
  5. Fresh verification: focused tests pass 3x under -race, go vet ./... passes, full cmd/server suite passes, and git diff --check is clean.

…text leaks (#77 review)

The review of #77 found the error branches after the transmission lookup in
ObservationRawHexForHash untested: swallowing a failed observation query,
scan or rows.Err left every test green. Cover all three through real SQLite
schema changes made after the store loaded, with no test seam in production
code:

- query:   observations.raw_hex dropped under a stale schema flag;
- scan:    observations replaced by a view whose id is not an integer;
- iterate: a view whose raw_hex fails at runtime on frames after the first
           row, which the driver steps inside Query, so it surfaces from
           rows.Next.

Each case asserts the wrapped error at DB level and a 500 from the packet
detail handler. Both 500 tests now require exactly the generic body and
reject DB error text, SQL/sqlite, table/column names and paths.

Correct the index comment on ObservationRawHexForHash: both lookups are
indexed, but EXPLAIN QUERY PLAN on the migrated e2e fixture shows the planner
using sqlite_autoindex_transmissions_1 and idx_observations_tx_ts rather than
the two indexes the comment named. Comment-only change in db.go.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Review feedback addressed (commit 88dcb6d)

  1. Added TestObservationFrameFailuresAfterTransmissionLookup, which covers the three error branches after the transmission lookup in ObservationRawHexForHash. Each is provoked through a real SQLite schema change after the store has loaded, with no test seam in production code:

    • query: observations.raw_hex is dropped while the schema flag is still true.
    • scan: observations is replaced by a view whose id is not an integer.
    • iterate: a view whose raw_hex fails at runtime on frames after the first returned row. The driver steps the first row inside Query, so the error surfaces from rows.Next / rows.Err.

    Each case asserts the wrapped error at DB level and HTTP 500 from the packet-detail handler.

  2. Tightened both 500 tests through a shared assertGenericFrameFailure. The body must be exactly {"error":"Failed to load observation frames"} and must not contain, case-insensitively, no such column, sql, sqlite, raw_hex, observations, :memory:, .db or /. The generic message itself contains none of these.

  3. Corrected the index comment on ObservationRawHexForHash. Both lookups are indexed. On the migrated e2e fixture, EXPLAIN QUERY PLAN shows sqlite_autoindex_transmissions_1 and idx_observations_tx_ts, not the two indexes the comment named. The comment now describes this without locking in the planner's choice. Only comment lines changed in db.go, with no production logic change.

  4. Extracted the duplicated store-backed router setup into newStoreBackedRouter, reusing the package's existing mustExec.

Mutation check in a scratch copy, run against the focused tests: all 11 mutations fail the suite. These are the previous a–d and h, swallowing the query, scan or iterate error, and returning the DB error text in the body with or without the generic prefix.

Local verification: the focused packet-detail/raw_hex suite passed 3× with -race (21 tests), and the full cmd/server suite passed with -race. go vet ./..., gofmt -l and git diff --check are clean.

Not covered: nothing from the review remains untested.

@dborup
dborup marked this pull request as ready for review September 26, 2026 04:07
@dborup
dborup merged commit fa5bf4f into master Sep 26, 2026
6 checks passed
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