[pull] main from danny-avila:main - #153
Merged
Merged
Conversation
* 🧭 fix: Anchor Summary Coverage to a Source Message ID A persisted summary's reach was inferred from where its content block sits in the payload: everything before that position was dropped. But the block is emitted on the assistant message that *follows* compaction, while the recency tail retained by `retainRecent` sits earlier. Positional trimming could not tell the retained tail apart from covered history, so on the next run the tail was discarded — messages the summary never covered, and that no longer appeared anywhere in the prompt. Summaries now declare their own extent. `createSummarizeNode` records the last refined message that carries a source ID as `coverage.throughMessageId`, and `formatAgentMessages` resolves that ID against the payload, trimming through the covered message and leaving everything after it verbatim. Blocks without resolvable coverage — written before this field existed, or covering a message no longer in the payload — keep the positional reading unchanged, so existing compacted conversations are unaffected. * 🧭 fix: Skip Coverage IDs That Straddle the Recency Boundary A steer expands one source message into pre-steer, steer, and post-steer messages that all carry its ID, and `splitAtRecencyBoundary` splits on any human-type message — including the steer. The newest refined message could therefore carry an ID whose remaining messages are in the retained tail, and declaring it would make the next run drop the half this run kept. Coverage now skips IDs still present in the tail and declares the newest fully covered one instead, falling back to positional semantics when every refined ID straddles the boundary. * 🧭 fix: Anchor Coverage to the First Retained Message Addresses the codex review on 2b6012c. Anchoring coverage to the last *covered* message left two states with no correct value. A steer expands one source message into pre-steer, steer, and post-steer entries sharing its ID and the recency split lands between them, so the newest covered ID could be half-retained; when every refined ID straddled, coverage was omitted and the reader read that as legacy, dropping the tail it was meant to protect. Separately, a reducer-stamped synthetic at the end of the head could be persisted as an ID matching no payload entry. Anchoring to the first *retained* message removes all three: a straddling message is the anchor and survives whole, "nothing fully covered" becomes an ordinary anchor rather than an absent one, and the anchor is read from the retained tail instead of the synthetic-prone head. The boundary is now exclusive, and the straddle-skipping set is gone. Coverage mode also no longer skips token adjustment for the entry holding the block. `formatAssistantMessage` filters that summary part out while its text is accounted separately as `summary.tokenCount`, so charging the entry for both inflated the prompt estimate and pruned early. Summary text length is captured during the existing scan, since it nests under `content[]` where `contentPartCharLength` does not reach. * 🧭 fix: Measure Tool Calls and Skip Injected Context Addresses the second codex review, on 1854731. `contentPartCharLength` reads only top-level fields, so a tool call — whose args and output nest under `tool_call` — measured as zero characters. An entry holding a summary plus a tool call therefore scaled to the summary's share alone and collapsed to 1 token, while `formatAssistantMessage` still emitted the call and its ToolMessage: a 3000-token entry reported 1, hiding the payload from the pruner. Tool-call content is now measured, and the discount is skipped outright when retained parts retain no measurable length, so an unrecognized shape over-counts rather than vanishing. Coverage anchors also skip injected context. `convertInjectedMessages` builds hook and skill-body entries as marked HumanMessages that the reducer stamps with a UUID no payload entry carries; anchoring there looked resolvable at write time and silently degraded to positional trimming on read. * 🧭 fix: Narrow the Injected-Context Check to Spare Steers Addresses the third codex review, on 51376bb. The previous guard treated every non-null `additional_kwargs.source` as in-run context. Steers carry `source: 'steer'` but are replayed by `formatAgentMessages` from a payload entry and stamped with its ID, so they are valid anchors. When compaction lands after a retained steer but before any post-steer message exists, the steer is the only retained entry — the guard skipped it, left coverage undefined, and the next run fell back to positional trimming and dropped the retained steer this coverage exists to protect. The check is now `isMeta === true || source === 'hook'`, the two unambiguous in-run markers. Skill bodies already carry `isMeta` and no source ID. Skipping too little only costs an unresolvable anchor and a positional fallback; skipping too much loses history, so the check errs narrow. The straddle fixtures also now carry the real steer marker. They passed against the broken predicate because their post-steer message repeated the same ID, which masked the skip — fixture drift from the shape production actually produces. * 📝 docs: Record the Unstamped-Payload-Entry Limitation Addresses the fourth codex review, on 2e26c1d — documentation only. A payload entry that omits messageId is never stamped, so the reducer's UUID is recorded as the anchor and cannot resolve on the next run. There is no write-time fix: such an entry has no stable ID in the next payload either, so no anchor can name it. The reader's positional fallback is what main already does, so the anchor degrades to current behavior rather than misleading. * 🧭 fix: Require Every Retained Part to Be Measurable Addresses the fifth codex review, on fb2ed1d. The measurability backstop was an aggregate check: it allowed the discount whenever the retained characters totalled more than zero. An entry retaining both a short caption and an image therefore passed it — the caption contributes characters, the image contributes none but still costs its image token estimate — so the scale charged the entry for the caption alone. A 1100 token entry reported 22. The check is now per-part: any retained part the char heuristic cannot see skips the discount entirely, so an unrecognized shape over-counts and prunes early rather than sending an over-context request. The round-two test covered an image alone, where the aggregate check already held, so it never exercised the mixed case. * 🧭 fix: Separate Zero-Cost Parts From Unmeasurable Ones Addresses the sixth codex review, on c8127e4. Two refinements of tradeoffs taken in earlier rounds, both of which the reviewer showed were tighter than they needed to be. The measurability check conflated a part that costs nothing with a part the char heuristic cannot see. An empty text block is filtered by `formatAssistantMessage` and contributes no prompt tokens, so its zero length is accurate — but it disabled the discount and restored the double count. Parts dropped by formatting are now excluded from the requirement, so only genuinely unreadable shapes suppress the adjustment. The injected-context check was a denylist of one label, which missed hook output carrying `source: 'skill'` or `'system'` without `isMeta` — `InjectedMessage` leaves the flag optional for every source. Inverted to an allowlist: `steer` is the only label that is source-backed, because a steer is replayed from a payload entry and stamped with its ID. Everything else marked with a source is created in-run. * ♻️ refactor: Discount Summary Tokens by Subtraction, Not Char Ratio Addresses the seventh codex review, on 6ac48d9 — and retires the approach that produced four of its findings. Coverage mode was scaling the summary-bearing entry's token count by the share of characters that survive formatting. Characters are the wrong proxy for token cost, so each round found another shape the heuristic could not see: tool-call payloads (round two), media (round five), a measurable part beside an unmeasurable one (round five), zero-cost versus unreadable parts (round six), and now media nested inside a tool output. Enumerating shapes cannot converge. The summary block already records its own token count, so the entry is now discounted by subtracting it. No measurement of the retained content is involved, which removes the whole class: media, tool calls, and nesting are irrelevant to a subtraction. The guard is a value comparison — skip when the summary claims at least the entry's tokens — rather than a shape predicate. Also drops the injected-context filter. Provenance is not recorded in `additional_kwargs`: an in-run injected entry and a replayed payload steer can carry identical markers, and only the reader holds the payload needed to decide. Filtering on markers cost a real bug in round three (skipping `source: 'steer'` dropped retained steers) and bought nothing, because in-run context is always appended — `[...toolMessages, ...injected]` — so a tail beginning with injected context has no payload-backed message later to reach. An unresolvable anchor falls back to positional trimming, which is what main does for every summary today. `contentPartCharLength` keeps its nested tool-call measurement: that is an independent accuracy fix for the positional path, which still scales by chars. * 🧭 fix: Skip Synthetic Context When Anchoring Coverage Addresses the eighth codex review, on 20b392d. The reviewer produced the counter-example I asked for, and my reasoning was wrong. I removed this filter in the previous commit on the grounds that in-run context is always appended, so a tail beginning with synthetic context could never have a resolvable message behind it. That holds for ToolNode and StandardGraph injections but not for formatAgentMessages, which reconstructs skill bodies inside its own payload loop and keeps processing payload entries afterwards. An unstamped skill body — a reducer UUID by the time compaction sees it — is therefore followed by stamped messages, and anchoring on the UUID resolves to nothing on the next run and drops the retained tail. The filter is restored. `isMeta` catches the reconstructed skill body; `steer` stays exempt because a replayed steer is stamped from its payload entry and skipping it dropped retained steers when this check once rejected every marked source. The residual ambiguity is documented rather than papered over: `InjectedMessage` also permits `source: 'steer'`, and an in-run injected steer is unstamped, so the exemption accepts a UUID for it. Nothing in `additional_kwargs` separates the two. Both directions bottom out in the reader's positional fallback, so the exemption favours replayed steers as the common case. * 🧭 fix: Record Injected Provenance at the Source Addresses the ninth codex review, on 649626d, by taking its first suggestion: stamp injected messages rather than infer their origin downstream. `InjectedMessage` leaves both `isMeta` and `source` optional, so a bare injected turn carried only its role and the coverage anchor read its reducer UUID as source-backed. `convertInjectedMessages` now records `injected: true` on everything it builds, which is the one place that knows. Kept separate from `isMeta` deliberately: that flag has UI and prompt-cache meaning of its own (`isMetaOrSkillMessage` in messages/cache.ts), so setting it unconditionally would change cache placement for callers that had left it unset. `injected` has no other consumers. This also resolves the injected-steer case documented as unfixable in the previous commit. Those go through the same funnel, so they now carry the marker and are skipped, while a steer replayed from a payload entry — stamped with its ID, never through this funnel — still anchors. The remaining `isMeta`/`source` checks stay for the constructors that build synthetic entries directly rather than through `convertInjectedMessages`: hook context in ToolNode and StandardGraph, handoff cues, and reconstructed skill bodies. * 🧮 fix: Discount Summaries in the Reader's Own Token Units Addresses the tenth codex review, on 688e7f2. The subtraction introduced two commits ago mixed token spaces. `tokenCount` is the injection budget: provider output-token space whenever usage was reported, plus the wrapper added later at injection time, and for reasoning models the provider figure can include thinking blocks that `extractResponseText` drops. `indexTokenCountMap` is kept in the consumer's own tokenizer. Subtracting one from the other over-subtracted — a 1000-token entry fell to 100 in test — which undercounts retained content and can send an over-context request. The summary block now records `rawTokenCount` alongside it: the summary text measured with the consumer's `tokenCounter`, no wrapper, which is the space `indexTokenCountMap` uses. The reader subtracts only that. Blocks without the field get no discount, so nothing written before this change is touched. Also stamps multi-agent routing prompts. The handoff and fan-in `HumanMessage`s in MultiAgentGraph carried no provenance at all, so a retained routing prompt was read as source-backed and its UUID persisted as an anchor that cannot resolve. They are built in-run and never persisted as standalone payload entries, so they are marked synthetic at construction — the same fix as the injected-message funnel, applied to the constructors that bypass it. * 🧹 fix: Stop Discounting the Summary-Bearing Entry Addresses the eleventh codex review, on 5dcc4d6, by removing the adjustment rather than attempting it a fourth way. The reviewer showed that a token count recorded at write time is in the writing run's units: `Run.create` derives its counter from the model in play, and a consumer may supply its own, so a conversation continued on a different model subtracts across encodings. The reader cannot recover the right figure — this function receives no tokenizer, and has no reliable identity for one. That is the third approach-level failure for this one adjustment: the wrong proxy (characters for tokens), then the wrong space (provider for local), now the wrong tokenizer. Each fix relocated the error rather than removing it, because the quantity is not available at the reader without a consumer-facing change, which this PR otherwise avoids entirely. A `provider`-equality guard would only be a fourth proxy. Coverage mode therefore leaves the entry at its full count. The consequence is that the summary's tokens are counted twice — once in the entry, once as `summary.tokenCount` — which over-states the prompt and prunes earlier than strictly necessary. That is the safe direction; every version of the discount risked under-counting and an over-context request instead. The reasoning is recorded at the call site, and passing the reader a tokenizer remains the way to fix it properly. The positional path is untouched: it slices content and scales by the characters that survive, entirely within one run's units. * 🛟 fix: Require Measurable Retained Parts on the Positional Path Addresses the twelfth codex review, on 7665fdd. The positional path scales a summary-bearing entry by the share of characters after the slice, and atomic media contributes none. An entry retaining only an image after its summary therefore collapsed to 1 token while the image still cost its fixed provider price, which can exceed the context window on replay. This is a pre-existing fault, not one this branch introduced: verified against unmodified main, `[text, summary, image]` takes a 1200-token entry to 1. What this branch did was widen the set of shapes that reach it — measuring nested tool-call content made `[tool_call, summary, image]` pass the `totalCharLen > 0` guard that previously skipped the adjustment by accident. Both are fixed by the discipline the coverage path already learned: every retained part must be represented before scaling. Parts that formatting drops are exempt, since their zero length is accurate rather than a blind spot. Unmeasurable retained content now keeps the original count, over-counting and pruning early instead of under-counting and overflowing. * 🔒 fix: Gate the Positional Ratio on an Allowlist Addresses the thirteenth codex review, on 10945d1. The previous guard rejected retained parts that measured zero characters, which missed media nested inside `tool_call.output`: serializing that output gives the image a nonzero length, so the guard read it as measurable while the token counter charges its fixed media cost. A long text run before the summary and a short image URL nested after it took a 4000-token entry to 68. Rejecting unmeasurable shapes case by case has now missed a nesting level twice, so the predicate is inverted. Only `TEXT` and `THINKING` — the two shapes whose characters `contentPartCharLength` actually reads — are eligible for the ratio. Everything else, at any depth and including content types added later, is ineligible by default and the entry keeps its original count. The common case is unaffected: a summary block mid-message is normally followed by assistant text, so all ten pre-existing positional tests still proportion as before. Only entries retaining media, tool calls, or resources lose the discount, in the safe direction. Removing the positional adjustment outright was the alternative, as was done for coverage mode, but it is not equivalent: positional slices content out of the message, so without it the entry would be charged for everything the slice removed — a large over-count for legacy conversations rather than a small one. * 🔒 fix: Cancel the Positional Ratio on Any Ineligible Part Addresses the fourteenth codex review, on 68d2fba. The previous commit validated only the retained side. A removed tool payload carrying a base64 image serializes to a very large denominator while the token counter charges that media a fixed estimate, so the ratio dragged retained text far below its real cost — the mirror image of the retained-side collapse, and equally able to exceed the window. Both sides are now required to be ratio-eligible: a single ineligible part cancels the discount wherever it sits. Excluding unmeasurable removed parts from the denominator was the narrower option, but it discards legitimate text — a removed tool input of plain JSON really is text whose cost tracks its length — and separating that from a media-bearing payload means recursing into arbitrary nested output, which has already missed a level twice here. This changes pre-existing behaviour: a legacy entry containing any tool call now keeps its full count rather than being proportioned, so it over-counts by the sliced-away payload and prunes earlier. Two tests that encoded the old behaviour are reframed accordingly. The exchange is deliberate — over-counting degrades quality, under-counting sends a request the provider rejects — and entries of plain text and reasoning, the common shape, still proportion as before.
* 🧵 feat: Previous-Headers Continuity for Activity Labels Threads committed headers from earlier batches into the activity-label prompt so consecutive same-activity batches extend the run's story instead of restating a line already on screen. - types/activityLabel.ts: `previousLabels?: string[]` on RunActivityLabelOptions — run order, most recent last; hosts pass only COMMITTED labels (a pending slot's text is empty and a dropped fill never surfaced to the user). - prompts/activityLabel.ts: renders the list as the prompt's FIRST section, capped at MAX_PREVIOUS_LABELS (3); dropped wholesale under any active tool-output redaction policy — per-agent overlays mean an earlier header may have been generated under ANOTHER agent's weaker policy, and a handoff must not leak that phrasing into this trace. - run.ts: generateActivityLabel passes the field through to the builder. Evidence (eval corpus replaying captured production payloads, claude-haiku-4-5, 28 steps x 3 samples): continuity context eliminated restatement incidents, produced correct setup-then-payoff pairs on consecutive same-activity batches (the production failure this addresses), and prevented premature-conclusion labels on setup batches; +12% label-call cost from the larger prompt. Consumer: LibreChat threads committed labels at request-build time via its PostToolBatch hook accumulator (follow-up to LibreChat#14391). * 🛡️ fix: Bound and Flatten Previous Labels in the Prompt Codex round-1 findings (both P2, same root cause: previous labels are the one input that RE-ENTERS the prompt on every later batch of a run, so one malformed label persistently steers unrelated later labels instead of affecting a single request). - prompts/activityLabel.ts: `sanitizePreviousLabel` collapses whitespace and clips at PREVIOUS_LABEL_LIMIT (200) before interpolation. Sections here are delimited by blank lines, so an embedded newline could otherwise forge an apparent `Tool calls:` or `Label:` section — from ordinary noncompliance or from injection surfacing through a tool result. The clip matches how `lastAssistantText` and reasoning excerpts are already bounded: oversized headers must not inflate later requests past the fast model's window and starve the run of labels. - Labels that sanitize to nothing are filtered, and the section is omitted entirely when none survive, so no bare bullet implies a missing header. - run.ts: `extractLabel` collapses whitespace at the source. A header renders as one row, and hosts feed committed labels back as continuity context, so a multi-line result must not carry line breaks forward. Tests: multi-line label yields exactly one header bullet with the injected framing inert inside it (and exactly one real `Tool calls:` / `Label:` section survives); a 5,000-char label is clipped, not inlined; an all-blank list omits the section.
shapeRootSpan reduces a root span's observation input/output to the last user question and assistant answer (item 2 of the Langfuse-team feedback in #288/#316). For chain/agent roots that is pure presentation — the child generation still records the complete prompt. But a generation that IS the trace root (a bare model.invoke with no wrapping chain, i.e. the activity-label path) has no child: the reduction discarded the SystemMessage — the entire label instruction — from the only place it is traced, making the instruction impossible to A/B against production traffic. Skip the observation-level reduction when the root is itself a generation; trace-level input/output still reduce, so the trace list keeps showing question and answer. Titles and the main stream are unaffected (their roots are chains). Verified end-to-end with an OTLP capture probe against the built dist: the exported label generation now carries both messages, trace input still reduces to the user prompt.
Ports the host fallback builder's entries framing onto the live path: the entries heading becomes "What it called, and what came back (do not restate these):" and the terminal cue becomes "Header:". LibreChat's fallback has always framed the list this way — its runtime.ts documents that without it the model "hands back a transcription" of the list — but the guard had never been on the SDK path, which sent a bare "Tool calls:" … "Label:". Measured before porting, via the eval harness (agents #360, ported from LibreChat #14527): across three independent 3-sample sweeps the guard framing had fewer template-redundancy flags (pooled 9 vs 18 for the control) and fewer length violations in every arm, with no per-case regressions by eye. A post-change sweep against this builder on a local merge with the harness confirms the as-shipped rendering: the host-shipped arm scored its best length-violation count of any run (8 vs 12-16 for prior controls) with zero tool-name echoes. The section-forging defense test now attacks the new markers; the sanitizer rationale comment follows the rename.
…ng Docs (#361) * 📐 docs: Langfuse trace-shaping invariants and module map in AGENTS.md Documents what "well-shaped" means for Langfuse traces (stable span names, observation types, root input/output reduction, control-flow and usage normalization, redaction, identity propagation, deterministic trace ids, self-contained trace identity), where the shaping code lives, and how to verify changes against the specs and a live Langfuse project. * 🧭 fix: Detach Langfuse Roots From Foreign Ambient Spans Production traces surfaced three shaping gaps, all traced to root observations inheriting identity from a host's ambient OpenTelemetry span (HTTP auto-instrumentation on the global provider) that is never exported to Langfuse: - Trace roots arrived with dangling parents, so `shapeRootSpan` never fired and trace input/output stayed null. - An agent run and the previous turn's title run fired inside one request context inherited the same foreign trace id and merged into a single trace with racing names (AgentRun vs TitleRun) and unioned tags (`agent` + `title`). - `deterministicTraceId` was silently bypassed: the seeded id generator only runs for true roots. Fixes: - `RoutingLangfuseSpanProcessor` registers spans it exports in a WeakSet registry; `ScopedLangfuseCallbackHandler` detaches foreign ambient spans from the OTEL context for detached runs (roots, or starts whose `parentRunId` the handler never tracked — the base handler's runMap-miss fallback would otherwise parent them on the ambient span). Langfuse-managed ambient spans still parent root observations so hosts can group runs deliberately. - `generateTitle` seeds its runtime scope (`title-<runId>`) mirroring the activity-label path, so an inherited parent seed can no longer collapse the title into the run's trace. - The outer workflow node span is named with the bare agent id; ephemeral ids (`endpoint__model___sender[____index]`, LibreChat's `encodeEphemeralAgentId` format) now reduce to their stable sender name so switching models no longer renames the span. Verified against live Langfuse cloud ingestion: separate AgentRun and TitleRun traces at their deterministic ids, trace input/output populated, root typed `agent`, `Probe Agent` span name, and nothing on the foreign trace id. * 🛡️ fix: Address Codex Review — Destination-Aware Detach, Stricter Id Parse P1: a Langfuse-managed ambient span is only a safe parent for runs exporting to the SAME destination. The span registry now records each span's destination key (moved the processor params/key derivation into langfuseSpanRegistry, with the credential helpers relocated to langfuseConfig and re-exported from langfuse for API stability), and detachment compares the ambient span's destination against the starting run's — a tenant-B run inside tenant-A's managed span now starts its own trace instead of dangling in B's project with A's trace id. P2: `extractEphemeralAgentSender` now validates the full encoded-id format — the `endpoint__model` prefix must contain both segments and no whitespace — and root observations are never rename candidates, so display names that merely embed `___` (e.g. `LibreChat Agent: Ops___EU`) are left alone. * 🎯 fix: Address Codex Round 2 — Metadata-Gated Rename, Pure Destination Identity - Gate ephemeral-id decoding to actual workflow-agent nodes: the span must carry `langgraph_node` observation metadata equal to its own name (verified against production span attributes; `@langfuse/tracing` flattens string metadata raw), so ordinary chains shaped like `pipeline__stage___EU` are never renamed. - Successful decodes now become `agent` observations, matching the inner `agent=<id>` node shaping. - Destination identity excludes processor policy: `toolOutputTracing` stays in the processor cache key (redaction is baked per processor) but no longer splits destination keys, so a host grouping span and a nested run with different redaction settings still parent correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )