Skip to content

fix(logging): prefer the pipeline over the raw bodies in call-log storage and detail enrichment - #13147

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/call-log-artifact-bodies-first
Sep 11, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/call-log-artifact-bodies-first

Conversation

@maxmad64bis

@maxmad64bis maxmad64bis commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ base-red inherited: #12732

Summary

A call log stores the same exchange twice: the raw client bodies, and pipeline — both sides translated, plus what the provider actually replied. Two places picked the wrong copy.

Past the size budget, the ladder dropped pipeline first, keeping a prompt it already had a copy of and losing the upstream answer. resolvePreviousResponseState (src/lib/db/responsesContinuationStore.ts:91) rebuilds previous_response_id history out of pipeline, so a long agentic conversation crossing the budget also lost server-side continuation and had to resend everything. The bodies go first now, and only when there's a pipeline to keep in exchange.

In the request-detail panel, maybeEnrichCompletedDetail reads the pipeline then falls back to responseBody — but it decided which sides were empty before reading anything, so the fallback overwrote what the pipeline had just supplied. responseBody is one value used for both sides, so the panel could show a provider payload as the client response. It's re-checked per side now. It ships here because the first fix makes it worse: the size-limit placeholder is a non-empty string, so it would have replaced a good payload with "[omitted: ...]".

The ladder was five copy-pasted stringify/measure/return blocks over two functions; it's an ordered list and one loop now.

Related Issues

Related to #12420 (overflow on large tool results), which stays open: the reporter says the request itself fails, and an oversized artifact never fails a request (writeCallArtifact catches and returns null). This changes which payload survives the cap, not the cap.

Same shape as #12095, which kept the error alive through this ladder.

Validation

  • Change type: DB
  • Focused tests and category gates from the golden path
  • npm run lint
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR

71/71 — 10 new, 61 existing across call-log-{size-limit-error,cap,stream-debug,artifact-worker,save-drain,oom-unbounded-5618}, log-export-runner and responses-continuation-store, each suite run on its own. typecheck:core and lint exit 0, on the tip of release/v3.8.51.

Tests Added Or Updated

call-log-artifact-bodies-first.test.ts — 5 cases: body-heavy overflow keeps pipeline.providerResponse; pipeline-heavy keeps today's shape; both-heavy falls through to today's minimal; no pipeline is stored exactly as before; a stored body is detectable as a placeholder, not merely truthy.

completed-detail-pipeline-precedence.test.ts — 3 cases, written first and red on the base (1 and 3 failing, 2 passing: an inverted precedence, not a broken fallback): a body doesn't overwrite a side the pipeline filled; it still fills a side no pipeline covered; a half-filled pipeline keeps its side.

Coverage Notes

Old and new serialization replayed over every fixture in the existing suites: same bytes except on the one exercising the new rung, so no existing expectation needed touching.

Reviewer Notes

The tradeoff: on a large 200 (big response body next to a big pipeline, e.g. agentic tool loops) the stored shape flips from bodies-kept/pipeline-dropped to bodies-dropped/pipeline-kept — only the pre-translation client payload is lost. Cost is one extra JSON.stringify on the oversize path, and only for artifacts with a pipeline; without one the new rung isn't in the list at all.

Left alone on purpose: the loop still stops as soon as either side is filled. No test proves that wrong and it predates this change.

@maxmad64bis
maxmad64bis force-pushed the fix/call-log-artifact-bodies-first branch 7 times, most recently from 6dc40dd to e72beb0 Compare September 10, 2026 02:03
@maxmad64bis maxmad64bis changed the title fix(call-logs): keep upstream response when artifact exceeds size budget fix(logging): prefer the pipeline over the raw bodies in call-log storage and detail enrichment Sep 10, 2026
@maxmad64bis
maxmad64bis force-pushed the fix/call-log-artifact-bodies-first branch from e72beb0 to d2674b3 Compare September 10, 2026 02:06
…rage and detail enrichment

An oversized call-log artifact evicted `pipeline` first and kept
`requestBody`. That traded the whole upstream exchange for a raw client
prompt the pipeline already holds a translated copy of, and it cost more
than readability: `resolvePreviousResponseState` rebuilds
`previous_response_id` history from `pipeline.clientRawRequest` /
`pipeline.clientResponse` and returns null for a pipeline-omitted
artifact, so a long agentic conversation that crossed the budget lost
server-side continuation and the client had to resend full history.

Evict the bodies one stage earlier when there is a pipeline to keep in
exchange. Dropping both is still reached when the bodies alone are not
enough, so the previous stored shape survives for artifacts this stage
cannot rescue.

`maybeEnrichCompletedDetail` decided which sides were still empty once,
before reading anything, so its `responseBody` fallback overwrote the
`pipeline.providerResponse` / `pipeline.clientResponse` it had just
recovered -- and `responseBody` is a single coarse value assigned to
both sides, so the panel could show a provider payload as the client
response. Emptiness is re-checked per side now, after the pipeline has
had its turn. The same inversion would have carried the size-limit
placeholder over a real payload once bodies are evicted first, since the
marker is a non-empty string; both call sites of that marker share one
predicate.

The fallbacks were five hand-rolled stringify/measure/return blocks
across two functions, so adding a stage meant copying the block and
re-deriving its position. They are one ordered list evaluated by a
single loop now; the last-resort error-only payload is written once
instead of twice.
@maxmad64bis
maxmad64bis force-pushed the fix/call-log-artifact-bodies-first branch from d2674b3 to cecd26a Compare September 10, 2026 02:07
@diegosouzapw
diegosouzapw merged commit cc4f7ed into diegosouzapw:release/v3.8.51 Sep 11, 2026
8 of 16 checks passed
Githab-capibara added a commit to Githab-capibara/OmniRoute that referenced this pull request Sep 17, 2026
…rage and detail enrichment (diegosouzapw#13147)

Both halves are right, and shipping them together is justified: the ladder dropping `pipeline` first threw away the upstream answer while keeping a prompt it already had a copy of, and `resolvePreviousResponseState` rebuilds continuation history out of exactly that field. The detail-panel fix has to ride along because the size-limit placeholder is a non-empty string, so fixing the ladder alone would let it overwrite a good payload. Re-checking emptiness per side after reading is the actual bug — `responseBody` being one value for both sides is what let a provider payload show as the client response.

---

Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them.

- `typecheck:core` clean; `check:changelog-integrity` OK
- complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline
- 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests
- `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on diegosouzapw#13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch.

⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs.

Thanks @maxmad64bis.
@maxmad64bis
maxmad64bis deleted the fix/call-log-artifact-bodies-first branch September 24, 2026 21:12
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…rage and detail enrichment (diegosouzapw#13147)

Both halves are right, and shipping them together is justified: the ladder dropping `pipeline` first threw away the upstream answer while keeping a prompt it already had a copy of, and `resolvePreviousResponseState` rebuilds continuation history out of exactly that field. The detail-panel fix has to ride along because the size-limit placeholder is a non-empty string, so fixing the ladder alone would let it overwrite a good payload. Re-checking emptiness per side after reading is the actual bug — `responseBody` being one value for both sides is what let a provider payload show as the client response.

---

Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them.

- `typecheck:core` clean; `check:changelog-integrity` OK
- complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline
- 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests
- `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on diegosouzapw#13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch.

⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs.

Thanks @maxmad64bis.
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