Skip to content

feat(tracing): add retrieval evidence child spans - #599

Merged
rodaddy merged 4 commits into
mainfrom
feat/569-retrieval-evidence-spans
Aug 6, 2026
Merged

feat(tracing): add retrieval evidence child spans#599
rodaddy merged 4 commits into
mainfrom
feat/569-retrieval-evidence-spans

Conversation

@rodaddy

@rodaddy rodaddy commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Part of #569

Summary

  • adds an async-local child-span API to the existing installMcpTracing wrapper; retrieval handlers can emit nested observations only while a traced MCP call is active
  • keeps tracing disabled as a direct no-op and applies the existing shared masking pass to every child input, output, and metadata field before the sink receives it
  • instruments both serving trees (server/tools for the local rewrite runtime and src/tools for the core01 runtime) across full retrieval execution, embedding generation, vector/keyword/graph/qmd candidate queries, fallback dedupe, RRF/federated ranking, and citation filtering
  • records resolved namespace values, database row IDs, bounded 300-character candidate previews, cosine distance/similarity, lexical and ranking scores, selected/filtered status, filter names, and stage counts without changing retrieval return values
  • bounds one tool call's collected child-span payload at 256 KiB; crossing the bound converts all child evidence to counts-only summaries and records the degradation reason in each retained span

Verification

  • bunx tsc --noEmit --project /Volumes/ThunderBolt/_tmp/open-brain/_worktrees/issue-569-retrieval-evidence/tsconfig.json
  • full bun test with /Users/rico/.config/open-brain/env.release-test: 3656 pass, 35 skip, 0 fail across 237 files
  • database-backed tests actually ran, including graph derivation, source sync, maintenance queue, embedding repair, chunk write, and rewrite source-registry/decompose/promotion PostgreSQL suites
  • pre-push hook: TypeScript passed; Bun completed 3272 pass, 509 skip, 0 fail without the release-test DB environment; branch pushed successfully
  • focused tracing suite: 64 pass, 0 fail, 143 assertions, including a real registered src/tools/search-brain call, two concurrent calls, throwing summarizer, payload degradation, transform equivalence, duplicate-qmd-path classification, and child-observation cleanup
  • regression-test proof: temporarily suppressed the parent span.end() in the child-observation exporter; the new test failed with Expected: 1 / Received: 0, then the implementation was restored and the focused tracing suite passed

Retrieval behavior

Instrumentation only. Existing retrieval tests and the full suite pass unchanged. No retrieval bug was found while instrumenting the pipeline.

Downstream rollout

Not applicable: this does not change MCP tool names, schemas, response shapes, auth/namespace semantics, transport behavior, migrations, Python client behavior, or agent-facing guidance. It adds operator-only Langfuse observations.

Critical Self-Review

  • Highest-risk behavior: Moving ranking and fallback computation inside tracing wrappers could accidentally alter ordering or selected rows; before/after equivalence tests cover rrfMerge and both fallback-merge implementations with empty and all-equal-score inputs, and the full suite passed.
  • Assumptions that could be wrong: Langfuse SDK v4 child observations created with parent.startObservation will render as nested observations under the post-call tool trace; the fake sink proves emitted shape and cleanup, but this PR does not perform a live hosted Langfuse canary.
  • Missing/weak tests: No live Langfuse OTLP egress assertion for parent/child rendering; functional coverage otherwise drives a real registered src search tool, concurrency, sink child failures, and the retrieval transform boundaries.
  • Security/permission risk: Candidate previews and namespace values widen observability payloads; previews are capped at 300 characters, rank inputs carry only IDs/counts, total child evidence degrades to counts-only past 256 KiB, and every retained dynamic field still passes through shared detector/key masking. Permission and namespace predicates are unchanged.
  • Migration/deploy risk: No migration. Deployment only activates the new observations where the existing OPENBRAIN_TRACING_* gate is enabled.
  • Downstream client/runtime risk: None identified; public MCP contracts and retrieval results are unchanged.
  • Rollback/cleanup concern: Reverting this commit removes the child observations without database cleanup because no local state is persisted.
  • Fixes made before PR: Installed tracing in the core01 src/index.ts server factory; bounded evidence payloads and precompiled masking regexes; made auth/status metadata authoritative; removed the unreachable candidate-dedupe span; moved ranking work inside spans; corrected duplicate-qmd and pagination classifications; mirrored execute/fallback spans into src; returned fallback classifications from the computation; and guaranteed parent observation cleanup.
  • Known residual risk: Child observations are materialized when the completed tool trace is emitted, so native observation timing is post-call; measured per-stage duration remains in metadata. The payload-size guard and cleanup paths are unit-tested, but live Langfuse parent/child rendering remains the rollout canary.
  • SME review-memory update: [x] docs/sme/ updated (correctness, security, adversarial — commit 5c08455 on this branch, provenance pull/599#issuecomment-5202342752)

Review Gate

Co-Authored-By: Claude <noreply@anthropic.com>
@rodaddy

rodaddy commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Review swarm findings (posted before fixes, per program mandate)

Opposite-family swarm: 3 Claude Opus lanes (correctness / security / adversarial, SME-injected from docs/sme/) over head f2a42aa, plus an independent re-verification of the PR's failure-proof claim. Findings below are deduplicated; lane attribution noted. Fixes follow in subsequent commits — none have been made yet at the time of this comment.

HIGH

H1 (correctness): the entire src/ half of the instrumentation is dead code — installMcpTracing is never installed on the tree that serves core01.
installMcpTracing is called only from server/main.ts:162; src/index.ts (which core01 runs per its own header comment and package.json start) has zero tracing references. So activeMcpTrace.getStore() is always undefined in src/tools/search-brain.ts, search-all.ts, brain-answer.ts, and src/candidate-dedupe.ts — roughly half the PR's added lines can never emit a span in production. The "instruments BOTH serving trees" claim is WRITTEN, not RUNNING, and no test catches it because the tracing tests drive a synthetic in-test span, not a real retrieval file. Fix: install the wrapper in src/index.ts before registerAllTools (same ordering rule as installMcpAudit) and add an end-to-end test asserting a real src/tools search emits the expected span names.

H2 (adversarial, measured): unbounded span payloads — one traced search duplicates full row content 4-5x; measured 47.6 MB serialized and 528 ms of synchronous masking on the request path.
rowEvidence() embeds raw content_preview, which is NOT a preview — src/tools/table-constants.ts:33 defines it as the full t.content (max 856 KB in the live dogfood DB). With limit=250 × HYBRID_FETCH_MULTIPLIER=3, one call serializes the same rows into vector/keyword/graph query spans, AGAIN into rank_rrf.input, again into rank_rrf.output, and again into execute.output. Masking runs synchronously inside emitTrace on the tool's own path and recompiles all 19 detector regexes per string. Fix: bound content_preview in the evidence projection (the repo already uses a 300-char bound at src/tools/search-brain.ts:344), carry ids/counts only in rank_rrf.input, hoist detector regex compilation to module scope, and consider a total-bytes cap on active.spans.

MEDIUM

M1 (security + adversarial, found independently by both lanes): handler-supplied trace metadata spreads LAST in buildToolTraceBody (langfuse-tracing.ts:630) and can silently overwrite the auth-derived identity fields and status. A future setActiveMcpTraceMetadata({ caller_role, status }) call would make the trace lane disagree with the audit lane exactly where it matters. Fix: spread ...traceMetadata FIRST so server-owned fields always win; add a regression where a handler stamps {caller_role: "admin", status: "success"} and then throws, asserting the emitted body keeps the token-derived role and status: "exception".

M2 (correctness + adversarial): retrieval.candidate_dedupe can never fire on ANY tree. runCandidateDedupe's only production caller is the dream-rem maintenance handler / CLI scripts — never inside an MCP tool handler, so the AsyncLocalStorage store is always absent. This belongs to the background-worker tracing mechanism (the sibling feat/569-background-worker-tracing lane), not per-tool-call AsyncLocalStorage. Fix: drop the span + cross-tree import from this PR and remove candidate-dedupe from the instrumented list.

M3 (correctness): src-tree rank_rrf/federated_rank wrap run: () => alreadyComputedValue (src/tools/search-brain.ts:1109, both search-all.ts files) — duration_ms is structurally always ~0 and the exception path can never attribute a ranking failure, while the server tree computes inside run. Same span name, two meanings. Fix: move the computation inside run, matching server/tools/search-engine.ts:545.

M4 (adversarial): chosen-vs-filtered evidence is wrong for qmd rows in federated_rank — qmd rows never carry id, so the selection key collapses to qmd:<path> (or literal qmd: when path is absent); chunks sharing a path are ALL reported chosen: true even when dropped. The evidence answers "why wasn't this returned" with an affirmative falsehood. Also filtered_by conflates pagination-offset drops with ranking-window drops. Fix: key selection by array index tagged before sorting; distinguish pagination_offset from federated_rank_window.

M5 (correctness): two stages traced in the server tree have no src-tree counterpartretrieval.execute (around executeSearch, src/tools/search-brain.ts:1222) and retrieval.fallback_dedupe (around mergeFallbackSearchRows, :1449) are absent from the tree that serves core01; fallback_dedupe is precisely the chosen-vs-filtered span this issue is about. Fix: add both, mirroring server/tools/search-engine.ts:702-825.

LOW

  • L1 (correctness): server fallback_dedupe output re-derives classification from raw inputs instead of what run actually did — keys on bare row.id where the stage keys on fallbackDedupeKey(row), and collapses distinct drop reasons into fallback_limit. Have run return the classification alongside the rows.
  • L2 (adversarial): a child startObservation/child.end() that throws mid-loop in the sink (langfuse-tracing.ts:1199) skips span.end() — parent span leaks unended. Tool call is unaffected (outer try/catch holds). Wrap the child loop in try/finally.
  • L3 (security): span payloads amplify per-request masking cost on a caller-controlled limit (subsumed by H2's fixes).

INFO

  • The child-span export path (langfuse-tracing.ts:1196-1204) has zero test coverage; no test drives a REAL registered search tool under installMcpTracing (which is exactly why H1/M2/M5 were invisible to the suite). No two-concurrent-calls span-attribution test, though the reviewer traced the AsyncLocalStorage usage and believes attribution is correct. No throwing-summarizer test for the instrumentation_error guard, and no before/after equivalence test pinning "retrieval results unchanged".
  • The cross-namespace dedupe span (M2) is today safe only by execution context, not by authority — if it's ever reachable from a tool handler it would attach one tenant's merge pairs to another tenant's trace. Record that constraint wherever dedupe evidence eventually lands.

Failure-proof re-verification: CONFIRMED

Independently reproduced in a detached scratch worktree at head f2a42aa: green 54/54 first; a one-line suppression of recordTraceSpan's push turned exactly 1 test red with the exact claimed text (expected retrieval.vector_query/retrieval.rank_rrf, received []); revert restored 54/54; worktree removed. Caveat worth keeping: the red run fails on the FIRST assertion in the chain, so the masking assertions in the same test are not independently proven by that mutation — a collects-but-skips-masking mutation would be needed to exercise them.


Verdict: not mergeable as-is. H1 defeats the purpose of the PR on the production tree; H2 is a measured request-path cost. Fix lane launching next; fixes will be verified as a delta against these findings before the Review Gate box is checked.

rodaddy and others added 3 commits August 6, 2026 05:01
Wire tracing into the core01 entrypoint, bound retrieval payloads, preserve authoritative metadata, correct ranking and fallback evidence, and remove the unreachable candidate-dedupe span.

Co-Authored-By: Claude <noreply@anthropic.com>
Exercise the real src search tool, bounded span degradation, metadata precedence, fallback classifications, duplicate qmd paths, fail-open summarizers, transform equivalence, concurrent attribution, and parent observation cleanup.

Co-Authored-By: Claude <noreply@anthropic.com>
H1 (dead-tree instrumentation), M3 (precomputed-value spans) -> correctness;
M1 (metadata spread order vs auth-derived identity) -> security;
H2 (unbounded evidence payloads), M4 (collapsing selection keys) -> adversarial.
Provenance: pull/599#issuecomment-5202342752.
@rodaddy

rodaddy commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Fix-delta verification receipt (controller-posted)

Independent opposite-family verification of f2a42aa..3ccbc95 against the findings above, performed against the CODE (fix lane's report treated as untrusted). Every HIGH/MEDIUM/LOW: FIXED_VERIFIED. Verdict: mergeable.

Non-vacuity was established by mutation, not by reading assertions: seven independent mutations (revert the 300-char cap; restore metadata-last spread; restore id/path selection keying; strip the child-loop try/finally; collapse the fallback drop reasons; swap ALS for a module-global store; suppress span collection) each turned exactly one named test red (63/1) and green on restore.

Honestly reported non-reds, carried as residual:

  • NEW LOW (parked): the H1 install wiring in src/index.ts:242 is itself untested — deleting the install block keeps the full suite green, because the e2e test installs tracing on its own fake server. The code is present and mirrors the reviewed server/main.ts ordering, so this is a test-coverage gap, not a live defect. Same gap exists on the server tree. Follow-up shape: a test asserting createApp's serverFactory wraps registerTool when a sink is supplied.
  • L1's key-choice sub-claim rests on code inspection (behaviorally equivalent under the fixture); the drop-reason half is mutation-pinned.
  • INFO: payload degradation is deliberately all-or-nothing per call (one oversized span degrades earlier small ones — recorded in span metadata, on the record here so it's a known trade-off). INFO: retrieval.graph_query exists only in the src tree because the server engine has no graph arm — pre-existing capability difference, do not write a "same stage list in both trees" assertion.

New-defect sweep of the delta itself: payload bound provably cannot change retrieval results; src-tree install neither reorders registration nor double-wraps (runtime is the per-process singleton, wrapper is per-server-instance by design); shutdown drainage is deadline-bounded and never rethrows on any exit path.

State per LAW 0: verified WRITTEN and suite-passing at 3ccbc95 (tsc clean; 3272 pass / 0 fail; Postgres suites skip without the DB env — the tracing tests use fake pools and did run). Nothing here is RUNNING until deployed; live Langfuse rendering remains the rollout canary before #569 closes.

Review gate now satisfied: findings posted before fixes, MEDIUM+ captured into docs/sme/ (commit 5c08455), fix delta independently verified. Checking the Review Gate box; merging on green CI at the exact head.

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.

1 participant