diff --git a/CHANGELOG.md b/CHANGELOG.md index ffb2391..485de4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). The - **Adaptive call-level retry: per-attempt request override** (proposal 0095, llm-provider §7.1, spec v0.91.0). The LLM-completion call-level retry loop gains an opt-in per-attempt request override. A new `LlmRetryConfig` (the llm-provider-scoped superset of the generic `RetryConfig`, exported from `openarmature.llm`) carries a `per_attempt_override`: a schedule of `RuntimeConfig` partials applied to retries. Attempt 0 uses the caller's base `config` unchanged; retry `i` merges `per_attempt_override[i]` onto the base (the override's non-None fields replace; a None or unspecified field inherits the base, per the §6 null-skip semantics), and the last entry carries forward when the schedule is shorter than the retry count. The canonical use is an escalating temperature schedule that breaks the "temperature 0 replays the same output" determinism trap on a retried structured-output call. `complete()` never mutates the caller's `config` (each attempt config is a fresh copy), and a plain `RetryConfig` preserves the existing byte-identical replay. The per-attempt OTel span carries a new `openarmature.llm.retry_reason` attribute (`transient`) on retries, absent on the base attempt. This is the first half of proposal 0095; the structured-output reask half follows. Spec v0.91.0 is beyond the current v0.88.0 pin, so the behavior ships ahead of the pin (unit-tested); the conformance fixtures 061-066 ride the v0.17.0 pin bump. - **Adaptive call-level retry: structured-output reask** (proposal 0095, llm-provider §7.1, spec v0.91.0). The second half of 0095. `LlmRetryConfig` gains an opt-in `reask` builder (`Callable[[StructuredOutputInvalid], str]`). When present, a `structured_output_invalid` failure becomes retryable for that call (a call-level convenience, not a classifier change; without a builder it stays non-transient and raises on the first occurrence). On each such failure the loop appends two messages to a working transcript, the model's raw output as an `assistant` message and the builder's returned correction as a `user` message, so the retry is informed rather than a byte-identical replay. OA authors no prompt of its own (the caller owns every word beyond the model's output); the builder receives the raised `StructuredOutputInvalid` (its `raw_content` and `failure_description`). The transcript accumulates reask pairs across reask retries and consumes the `max_attempts` budget; a transient retry interleaved in a reask loop re-sends the accumulated transcript unchanged. `complete()` never mutates the caller's `messages` (each reask replaces the transcript with a fresh list rather than appending in place). The retry span's `openarmature.llm.retry_reason` is `reask` on a reask retry, `transient` otherwise. A reask always appends the model output as a fresh `assistant` message (never continues a trailing one): §3 requires the last message before a call to be `user`/`tool`, so the transcript never ends in `assistant`. Ships ahead of the pin (unit-tested); fixtures 062-066 ride the pin bump. - **Langfuse observer: credentials-in construction with tracer-provider isolation** (proposals 0114 + 0116 + 0117 + 0118, observability §6 / §8.9 / §8.4, spec v0.108.0 / v0.110.0 / v0.111.0 / v0.112.0). The Langfuse observer gains a second construction mode alongside today's caller-supplied client: `LangfuseObserver.from_credentials(public_key=..., secret_key=..., host=...)` (over the lower-level `LangfuseSDKAdapter.from_credentials(...)`) builds an OA-owned `Langfuse` client on a dedicated `TracerProvider` by default, so its observations no longer bind the global provider and leak onto the application's OTel backend. A Langfuse v4 client constructed with no `tracer_provider=` attaches its span processor to the globally-registered provider, so in any service that registers a global provider (the standard app-tracing setup) attaching the Langfuse observer silently exported every observation, prompts and completions included, to the app backend. Because the Langfuse SDK caches one client per `public_key`, a dedicated provider takes effect only when OA is the first constructor for that credential; OA reuses one isolated provider per credential and reads the actual binding back after construction. The invariant covers every payload OA harvests from the runtime -- the provider payload (`disable_provider_payload`), the Trace-level state input/output (`disable_state_payload` and the `trace_input_from_state` / `trace_output_from_state` hooks), and a failed Tool / Embedding / Retriever / LLM observation's `error_message` -- but not the dimensions the caller deliberately attaches (`correlation_id` / `session_id` / `userId` / trace name / caller metadata), which stay verbatim as cross-backend join keys. When any construction-determinable channel is live and OA establishes the client is bound to a provider it did not isolate, construction fails loud with a categorized `LangfuseProviderIsolationUnavailable` before any observation is emitted, rather than leaking payloads to a shared backend; where OA cannot establish the binding at all (a future SDK), it suppresses every channel and logs a warning. A failed observation's `error_message` is harvested exception text, so `disable_provider_payload` governs it for every failure category on all four provider observations: with payloads off it is not rendered, and the error category still rides as the status message where the event carries one. A Tool failure has no category, so its status message is null rather than falling back to the exception string. `error_type` is a classification token rather than harvested content and is never gated, which matters most for a Tool failure where it is the only remaining discriminator; it is optional, so it is emitted only where the failure event supplies one. A single `accept_shared_provider=True` opt-out turns the whole thing into a warn-and-proceed onto the shared provider. With no channel live (the default privacy posture), an un-isolatable client neither raises nor warns. The existing caller-supplied path (mode a) is unchanged and never mutated: a caller who builds their own client stays responsible for isolating its `tracer_provider`, and OA documents the remedy rather than reaching into the supplied client. The `secret_key` is accepted as a `pydantic.SecretStr`, masked in OA's own reprs and logs with the plaintext read only at the SDK call (`public_key` and `host` stay plain strings), and a blank credential is rejected at the boundary rather than falling through to the SDK's ambient `LANGFUSE_*` environment fallback. A `sample_rate` passed for the client is applied to the isolated provider, since the SDK only honors it on a provider it builds itself. `accept_shared_provider` binds the provider the application already registered rather than letting the SDK construct and globally register one of its own, which would capture OTel's single-assignment global slot. The new `LangfuseProviderIsolationUnavailable` derives from an `ObservabilityError` base, a fourth hierarchy alongside the graph-engine, llm-provider, and checkpoint ones. The behavior shipped ahead of the pin (unit-tested) and the pin has since advanced to v0.112.0. Fixture 159 now runs, and fixtures 098 / 137 / 138 are un-deferred and reconciled to the post-0118 shape. Fixtures 157 / 158 stay deferred: they need the conformance-adapter provider-faithful Langfuse fake and the `langfuse_client` construction directive, which are not yet built, and the source behavior is covered meanwhile by the unit suite. -- **Failed-observation `error_message` byte cap, and the `openarmature_` reserved namespace** (proposal 0119, observability §5.5.5 / §8.7 / §3.4, spec v0.116.0). Closes the two error-channel edges 0118 left open. **The cap:** a failed observation's `error_message` is now subject to the §5.5.5 per-value byte cap, on all four mapped provider observations (Generation, Embedding, Tool, Retriever). It takes the contract's *direct-application* arm rather than its *inheritance* arm: the OTel surface defines no `error_message` span attribute, so the value arrives untruncated and the observer that writes it applies its own `payload_byte_cap`, rather than inheriting a cap from an upstream OTel truncation that never happened. Re-applying a second cap to an already-truncated value would move the marker and misreport its byte total, which is why the two arms source the cap differently. A failed Tool observation renders the same harvested string twice, once in `metadata.error_message` and once as the observation's `statusMessage`, and **both** copies are capped: §5.5.5 governs payload-classified values rather than payload-classified fields, so capping one surface while the other still carried the whole exception would defeat the cap. The remaining `statusMessage` writes take the error *category*, a classification token, and stay uncapped. Under the default posture (`disable_provider_payload=True`) the field is absent entirely per 0118, so the cap is observable only where payloads are enabled, and the omission arm is unchanged: a withheld message still leaves `statusMessage` null on a Tool failure rather than substituting the message. **The reserved keys:** `openarmature_` joins `openarmature.` and `gen_ai.` as a reserved caller-metadata *namespace* prefix, and four exact names (`error_type`, `error_message`, `token_budget`, `token_budget_exceeded`) join the reserved set, which grows from 29 to 33. With `error_message` absent under the default posture, an unreserved caller key of that name would otherwise land unopposed in the very field 0118 requires to be absent, reintroducing through the metadata channel the leak the gate closes. Pre-1.0 behavioral change: a caller passing `invocation_metadata` with a key beginning `openarmature_`, or with any of those four names, is now rejected at the `invoke()` boundary with `ValueError` where it previously passed. Spec v0.116.0 is beyond the current v0.112.0 pin, so the behavior ships ahead of the pin (unit-tested); the `conformance.toml` entry and the fixtures ride the pin bump. No fixture drives the Tool arm today: the conformance adapter exposes no tool-calling node, so that arm is unit-tested only and the gap is reported to spec. **The maintenance check 0119 asks for lands with it:** a test that statically scans the Langfuse mapping for every top-level metadata key it writes and fails when one is neither reserved by name nor covered by a reserved namespace. It discovers the metadata bags from what is passed to a `metadata=` argument rather than hardcoding one name, because the observer builds five of them and a scan of the obvious one would miss four; 0119 asks for this specifically, since a sweep that looked at only a subset is what missed nine keys before. The check found three keys on the failure-isolation marker span (`error_category`, `failure_isolation_event_name`, `failure_isolation_node`) that a caller key still overwrites, because that handler merges caller metadata last. Those are held rather than reserved unilaterally: the marker is a graph-mechanism span that no mapping table covers, so whether the reserved set should reach an unmapped span is a spec question, and it is raised as one. **Truncation is also now surrogate-safe:** the cap runs on the failure path, where the value is harvested exception text and is the likeliest string in the observer to carry a lone surrogate (an `OSError` naming a surrogateescape-decoded path). Encoding one raises, an observer that raises is only warned about rather than logged, and the cap runs before the observation is created, so an unguarded encode would have deleted the very observation reporting the failure. A malformed message now degrades that one field and still respects the cap. +- **Failed-observation `error_message` byte cap, and the `openarmature_` reserved namespace** (proposal 0119, observability §5.5.5 / §8.7 / §3.4, spec v0.116.0). Closes the two error-channel edges 0118 left open. **The cap:** a failed observation's `error_message` is now subject to the §5.5.5 per-value byte cap, on all four mapped provider observations (Generation, Embedding, Tool, Retriever). It takes the contract's *direct-application* arm rather than its *inheritance* arm: the OTel surface defines no `error_message` span attribute, so the value arrives untruncated and the observer that writes it applies its own `payload_byte_cap`, rather than inheriting a cap from an upstream OTel truncation that never happened. Re-applying a second cap to an already-truncated value would move the marker and misreport its byte total, which is why the two arms source the cap differently. A failed Tool observation renders the same harvested string twice, once in `metadata.error_message` and once as the observation's `statusMessage`, and **both** copies are capped: §5.5.5 governs payload-classified values rather than payload-classified fields, so capping one surface while the other still carried the whole exception would defeat the cap. The remaining `statusMessage` writes take the error *category*, a classification token, and stay uncapped. Under the default posture (`disable_provider_payload=True`) the field is absent entirely per 0118, so the cap is observable only where payloads are enabled, and the omission arm is unchanged: a withheld message still leaves `statusMessage` null on a Tool failure rather than substituting the message. **The reserved keys:** `openarmature_` joins `openarmature.` and `gen_ai.` as a reserved caller-metadata *namespace* prefix, and four exact names (`error_type`, `error_message`, `token_budget`, `token_budget_exceeded`) join the reserved set, which grows from 29 to 33. With `error_message` absent under the default posture, an unreserved caller key of that name would otherwise land unopposed in the very field 0118 requires to be absent, reintroducing through the metadata channel the leak the gate closes. Pre-1.0 behavioral change: a caller passing `invocation_metadata` with a key beginning `openarmature_`, or with any of those four names, is now rejected at the `invoke()` boundary with `ValueError` where it previously passed. Spec v0.116.0 is beyond the current v0.112.0 pin, so the behavior ships ahead of the pin (unit-tested); the `conformance.toml` entry and the fixtures ride the pin bump. Fixture 160 carries no Tool case, so that arm is unit-tested here; the gap is in the fixture rather than in the harness, since the conformance adapter already drives a failing tool with a caller-chosen message (fixture 098 case 2 does exactly that), and a cap case is writable against it. **The maintenance check 0119 asks for lands with it:** a test that statically scans the Langfuse mapping for every top-level metadata key it writes and fails when one is neither reserved by name nor covered by a reserved namespace. It discovers the metadata bags from what is passed to a `metadata=` argument rather than hardcoding one name, because the observer builds five of them and a scan of the obvious one would miss four; 0119 asks for this specifically, since a sweep that looked at only a subset is what missed nine keys before. The check found three keys on the failure-isolation marker span (`error_category`, `failure_isolation_event_name`, `failure_isolation_node`) that a caller key still overwrites, because that handler merges caller metadata last. Those are held rather than reserved unilaterally: the marker is a graph-mechanism span that no mapping table covers, so whether the reserved set should reach an unmapped span was raised as a spec question rather than settled here. Spec has since ruled that it does not, and committed the forthcoming failure-isolation mapping to carry the three keys; see the precedence fix under Fixed. **Truncation is also now surrogate-safe:** the cap runs on the failure path, where the value is harvested exception text and is the likeliest string in the observer to carry a lone surrogate (an `OSError` naming a surrogateescape-decoded path). Encoding one raises, an observer that raises is only warned about rather than logged, and the cap runs before the observation is created, so an unguarded encode would have deleted the very observation reporting the failure. A malformed message now degrades that one field and still respects the cap. ### Changed @@ -39,9 +39,11 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). The ### Fixed +- **The Langfuse Tool observation now carries caller-supplied invocation metadata** (observability §8.4.2). It was the one provider observation that dropped it. The LLM handlers pick the caller set up through the shared typed-event metadata builder and the embedding and rerank handlers apply it directly, but the tool handler did neither, so a caller filtering Langfuse by their own key (a tenant id, a request id) saw every observation from an invocation except the tool calls. The §8.4.2 mapping maps the caller set to `observation.metadata.` on every Observation, and the unscoped wording is deliberate: the same table scopes its other rows explicitly where it means to, for example `fan_out_item_count` to the fan-out node Span only. The OTel observer's tool span had carried the set all along, so the two bundled observers disagreed about the same event. The caller set is merged before the OA-emitted keys, matching the other handlers, though on this observation precedence is unobservable either way: every key it writes is reserved, the `openarmature_*` pair by prefix and `error_type` / `error_message` by name, so a colliding caller key is rejected at the `invoke()` boundary and never reaches the merge. +- **A caller metadata key no longer overwrites the failure-isolation marker's own fields** (observability §3.4). The Langfuse `openarmature.failure_isolated` span merged caller-supplied invocation metadata *last*, with a plain per-key assignment and no collision check, which made it the only handler where a colliding caller key won: the LLM, embedding and rerank handlers all merge the caller set before writing their own keys. Three of the marker's top-level keys (`error_category`, `failure_isolation_event_name`, `failure_isolation_node`) are not in the reserved set, so `invoke()` does not reject a caller key of the same name at the boundary and the collision reached the observer. A caller passing `error_category` silently replaced the marker's only failure discriminator, on the failure path, with nothing logged. The caller set now merges first, so the OA-emitted values win, and a non-colliding caller key still comes through unchanged. The OTel observer needs no equivalent change: every attribute it writes is `openarmature.`-prefixed and caller keys land under `openarmature.user.*`, both covered by a reserved prefix, so a colliding key is rejected at the boundary and never reaches the merge. Langfuse metadata is flat, which is why the reserved *name* set exists alongside the prefixes. Read the new ordering as the lesser harm rather than a settled precedence rule: OA-wins still drops the caller's value silently, and §3.4 rejects a reserved collision precisely because silent resolution in either direction loses information. The real fix is reservation, and it arrives with the span's mapping: spec ruled that §3.4's reserved set does not reach a span no mapping table covers, and committed the forthcoming failure-isolation mapping to carry these three keys. Found by the proposal 0119 maintenance check. - **A detached fan-out instance no longer opens a second root that abandons the first** (observability §4.4). A detached fan-out's per-instance root is stored under `prefix + (instance index)`, but the ancestor walk that decides whether to open one tested the bare `prefix`, so the test never matched and the arm fired once per inner **node event** rather than once per instance. Each additional inner node re-opened the root, and the second open replaced both the root and its detached invocation span in the observer's state, so the first pair was never ended and never exported. The result for a caller: three traces where there should be two, one instance's inner nodes split across two of them, and spans carrying a parent id that nothing emitted. It needs two or more nodes in the instance subgraph to see, which is why it went unnoticed: with a single node there is one event and one open, and every existing test used that shape. The guard now tests the key the root is actually stored under. The neighbouring bare-prefix test is kept for depth and is documented as redundant rather than load-bearing, since the detached-subgraph path is already guarded a line earlier. -- **A dispatch span synthesized from a wrapper-issued call no longer loses its subgraph identity or its caller metadata** (observability §5.4 / §5.5 / §5.6, graph-engine §6). When a fan-out's `instance_middleware` (or a parallel branch's) issues a provider or tool call and then returns without calling `next_call`, no inner node event is ever emitted, so the dispatch span the observer synthesizes from that call had nothing to repair it. Two attributes went missing and stayed missing. `openarmature.subgraph.name` was empty despite a declared `subgraph_identity`, because the identity reached an observer only through an inner node event; `fan_out_config` now carries an optional `subgraph_identity` alongside its four required keys, and the observer caches it from the fan-out node's own `started` event, which always precedes its instances. The cross-cutting `openarmature.user.*` set was absent whenever a `FailureIsolatedEvent` was the event that synthesized the span, which happens when a wrapper sets metadata and then raises without making a provider call first; that event now carries the same optional `caller_invocation_metadata` the provider events carry, populated in the engine task, because the metadata is per-async-context per §3.4 and an observer resolves on the serial delivery queue where a live read sees the wrong context. The same change puts the cross-cutting set on the `openarmature.failure_isolated` marker span in both observers, which previously carried none. That is a consistency choice rather than a conformance one: `openarmature.failure_isolated` appears nowhere in the observability spec, and §5.6's scope sentence enumerates the spans it reaches without including it. The failure-isolation event is mandated by pipeline-utilities; the span emitted from it is ours and currently has no mapping, which spec is drafting. Both event-surface additions are additive: graph-engine §6 requires `fan_out_config` to present all four of its keys and does not close the set, and `caller_invocation_metadata` is already an optional field on the provider events. They ship ahead of any spec text, unit-tested, with standardization requested so a second implementation matches. +- **A dispatch span synthesized from a wrapper-issued call no longer loses its subgraph identity or its caller metadata** (observability §5.4 / §5.5 / §5.6, graph-engine §6). When a fan-out's `instance_middleware` (or a parallel branch's) issues a provider or tool call and then returns without calling `next_call`, no inner node event is ever emitted, so the dispatch span the observer synthesizes from that call had nothing to repair it. Two attributes went missing and stayed missing. `openarmature.subgraph.name` was empty despite a declared `subgraph_identity`, because the identity reached an observer only through an inner node event; `fan_out_config` now carries an optional `subgraph_identity` alongside its four required keys, and the observer caches it from the fan-out node's own `started` event, which always precedes its instances. The cross-cutting `openarmature.user.*` set was absent whenever a `FailureIsolatedEvent` was the event that synthesized the span, which happens when a wrapper sets metadata and then raises without making a provider call first; that event now carries the same optional `caller_invocation_metadata` the provider events carry, populated in the engine task, because the metadata is per-async-context per §3.4 and an observer resolves on the serial delivery queue where a live read sees the wrong context. The same change puts the cross-cutting set on the `openarmature.failure_isolated` marker span in both observers, which previously carried none. That is a consistency choice rather than a conformance one, because `openarmature.failure_isolated` appears nowhere in the observability spec, so §5.6 cannot reach a span the spec never defines. Not because §5.6's list of span kinds excludes it: spec has since ruled that list illustrative rather than exhaustive, which means the obligation arrives with the mapping. The failure-isolation event is mandated by pipeline-utilities; the span emitted from it is ours and currently has no mapping, which spec is drafting. Both event-surface additions are additive: graph-engine §6 requires `fan_out_config` to present all four of its keys and does not close the set, and `caller_invocation_metadata` is already an optional field on the provider events. They ship ahead of any spec text, unit-tested, with standardization requested so a second implementation matches. - **A fan-out inside a parallel branch no longer returns a sibling branch's results** (pipeline-utilities §9 / §11). **Correctness fix; a graph could return data it never computed.** The engine keyed a fan-out's in-flight execution state by its namespace, node name, and enclosing fan-out-instance lineage. A parallel branch contributes to none of those: branch names never enter the namespace, and a branch descent adds only `null` entries to the fan-out-instance chain. So two sibling branches whose subgraphs each held a fan-out node of the same name built an identical key, and the second branch found the first's instances already `completed` and rolled its results forward. It executed none of its own item bodies and emitted no inner node events, so nothing in a trace showed the work had not happened. Where the two branches resolved different item counts the collision surfaced instead as a `CheckpointRecordInvalid` raised on a fresh run with no checkpointer attached, complaining about a saved record that did not exist. The state key now also carries the enclosing branch lineage, so sibling branches are distinct. The defect was schedule-dependent, and a single `await` in an item body was enough to hide it. Note for checkpointed graphs: a branch-nested fan-out now re-runs every instance on resume rather than skipping completed ones, because the checkpoint record has no field that distinguishes sibling branches and applying one branch's skips to another would be worse than re-running. Resume stays correct for this shape and saves no work; lifting that needs a record-format change, and is under discussion. diff --git a/src/openarmature/observability/langfuse/observer.py b/src/openarmature/observability/langfuse/observer.py index 173cbb7..814bda1 100644 --- a/src/openarmature/observability/langfuse/observer.py +++ b/src/openarmature/observability/langfuse/observer.py @@ -955,14 +955,14 @@ def _handle_failure_isolated(self, event: FailureIsolatedEvent) -> None: fan_out_index_chain=event.fan_out_index_chain, branch_name_chain=event.branch_name_chain, ) - # The caught exception's MESSAGE is deliberately absent: this marker is a - # graph-mechanism span, which no §8.4.x table maps, so writing harvested - # exception content onto it is non-conforming over-emission (0118). Like - # the node Span, it carries only the error category; the full exception - # reaches the OTel span via record_exception on OA's private provider. - metadata: dict[str, Any] = { - "failure_isolation_event_name": event.event_name, - } + # No exception message: no §8.4.x table maps this span, so harvested + # content on it is over-emission. It rides the OTel span instead. + # + # Caller set first because the three keys below are unreserved: a + # colliding caller key survives the §3.4 boundary and would win. + metadata: dict[str, Any] = {} + _apply_caller_metadata(metadata, event.caller_invocation_metadata) + metadata["failure_isolation_event_name"] = event.event_name if event.namespace: metadata["failure_isolation_node"] = event.namespace[-1] if event.caught_exception.category is not None: @@ -970,17 +970,6 @@ def _handle_failure_isolated(self, event: FailureIsolatedEvent) -> None: correlation_id = current_correlation_id() if correlation_id is not None: metadata["correlation_id"] = correlation_id - # The cross-cutting caller set. It carried none until - # `FailureIsolatedEvent` gained the field, because there was nothing to - # read; omitting it now would leave the two observers disagreeing about - # the same marker, which is worse than both lacking it. - # - # NOT a §5.6 obligation, though an earlier version of this comment said - # so. `openarmature.failure_isolated` appears nowhere in the - # observability spec: the EVENT is mandated by pipeline-utilities, the - # span is ours and unmapped. Cross-observer consistency is the reason - # this is here, not conformance. - _apply_caller_metadata(metadata, event.caller_invocation_metadata) handle = self.client.span( trace_id=inv_state.trace_id, name="openarmature.failure_isolated", @@ -2139,7 +2128,21 @@ def _handle_tool_call(self, event: ToolCallEvent | ToolCallFailedEvent) -> None: calling_branch_name_chain=event.branch_name_chain, ) # §8.4.6 metadata: tool name always, tool_call_id when present. - metadata: dict[str, Any] = {"openarmature_tool_name": event.tool_name} + # Caller metadata FIRST, same ordering as every other handler, so an + # OA-emitted key wins a collision. It goes on at all because §8.4.2 maps + # the caller set to `observation.metadata.` on EVERY Observation: + # the table scopes its other rows explicitly ("fan-out node Span + # observation only"), so the unscoped wording is deliberate. + # + # This handler carried NO caller metadata until now, which made the Tool + # observation the only one of the four provider observations to drop it, + # and left the two observers disagreeing about the same event: the OTel + # `_handle_tool_call` has applied it all along. The LLM handlers get it + # via `_typed_event_metadata`, and embedding / rerank apply it directly, + # so this was the one path with neither. + metadata: dict[str, Any] = {} + _apply_caller_metadata(metadata, event.caller_invocation_metadata) + metadata["openarmature_tool_name"] = event.tool_name if event.tool_call_id is not None: metadata["openarmature_tool_call_id"] = event.tool_call_id input_value: Any = None diff --git a/src/openarmature/observability/otel/observer.py b/src/openarmature/observability/otel/observer.py index 5f8523f..c7789ef 100644 --- a/src/openarmature/observability/otel/observer.py +++ b/src/openarmature/observability/otel/observer.py @@ -2390,14 +2390,26 @@ def _handle_failure_isolated(self, event: FailureIsolatedEvent) -> None: # The cross-cutting caller set. It carried none until the event gained # the field, because there was nothing to read. # - # NOT a §5.6 obligation, though an earlier version of this comment said - # so. §5.6's scope sentence enumerates the spans it reaches -- invocation, - # node, subgraph, fan-out instance, LLM provider, retry attempt -- and - # `openarmature.failure_isolated` is not among them. It appears nowhere - # in the observability spec at all: the failure-isolation EVENT is - # mandated by pipeline-utilities, but the span we emit from it is ours - # and has no mapping. Spec is drafting one (coord release-v0.17.0/47); - # until it lands this is our own consistency choice, not conformance. + # NOT a §5.6 obligation, though two earlier versions of this comment got + # the reason wrong in opposite directions. + # + # The reason is that `openarmature.failure_isolated` appears nowhere in + # the observability spec: the failure-isolation EVENT is mandated by + # pipeline-utilities, but the span we emit from it is ours and no §8.4.x + # table maps it. §5.6 cannot mandate attributes on a span the spec never + # defines. + # + # It is NOT that §5.6's list of span kinds excludes this one. A previous + # version argued exactly that, and spec has since ruled the list is + # illustrative rather than exhaustive: the rule reaches every span + # emitted during the invocation, because a framework span missing + # `openarmature.correlation_id` is broken in the way that attribute + # exists to prevent. So do not resurrect the enumeration argument. + # + # The practical consequence: once the span IS mapped, §5.6 reaches it and + # this stops being a consistency choice and becomes required. Spec has + # committed to that mapping (coord release-v0.17.0/52, /54); until it + # lands this is ours. _apply_caller_metadata(attrs, _event_caller_metadata(event)) span = self._tracer.start_span( name="openarmature.failure_isolated", diff --git a/tests/unit/test_failure_isolation_middleware.py b/tests/unit/test_failure_isolation_middleware.py index f9ae9b2..140101e 100644 --- a/tests/unit/test_failure_isolation_middleware.py +++ b/tests/unit/test_failure_isolation_middleware.py @@ -734,10 +734,12 @@ async def test_langfuse_failure_isolated_marker_carries_caller_metadata() -> Non # none until `FailureIsolatedEvent` gained `caller_invocation_metadata`, # because there was nothing to read. # - # This pins a CONSISTENCY choice, not a §5.6 obligation. §5.6 enumerates the - # spans it reaches and this marker is not among them; the span appears - # nowhere in the observability spec, only the event does. Spec is drafting a - # mapping. If it lands differently, this assertion changes with it. + # This pins a CONSISTENCY choice, not a §5.6 obligation. The reason is that + # the span appears nowhere in the observability spec, only the event does, so + # §5.6 cannot reach a span the spec never defines. It is NOT that §5.6's list + # of span kinds excludes this marker: spec has ruled that list illustrative + # rather than exhaustive. Spec has committed to a mapping; when it lands this + # becomes required rather than optional, and this assertion stays either way. # # Asserted on the LANGFUSE side specifically. The OTel half was covered # first, and covering only that leaves the two observers free to disagree @@ -787,3 +789,67 @@ async def _sets_then_raises(_s: _DocState) -> Mapping[str, Any]: assert marker.metadata.get("from_wrapper") == "yes", ( f"caller metadata must reach the Langfuse marker; got {marker.metadata}" ) + + +async def test_oa_keys_win_over_a_colliding_caller_key_on_the_marker() -> None: + # Precedence on the failure-isolation marker. Three of its top-level metadata + # keys are UNRESERVED, so §3.4 does not reject a caller key of the same name + # at the `invoke()` boundary and the collision reaches the observer. + # + # This handler used to merge the caller set LAST, making it the only one + # where the caller won: the LLM, embedding and rerank handlers all merge the + # caller set before writing their own keys. So a caller passing + # `error_category` silently replaced the marker's only failure discriminator, + # on the failure path, with no warning. + # + # OA-wins is the interim, not a settled precedence rule (spec ruled on it in + # coord release-v0.17.0/54): it still drops the caller's value silently. The + # real fix is reservation, which arrives when the span is mapped. + from openarmature.observability.langfuse.client import InMemoryLangfuseClient + from openarmature.observability.langfuse.observer import LangfuseObserver + from openarmature.observability.metadata import set_invocation_metadata + + async def _sets_then_raises(_s: _DocState) -> Mapping[str, Any]: + # `error_category` is the dangerous one: on a marker with no error + # category of its own it is the only discriminator a reader has. + set_invocation_metadata( + error_category="caller_wins_would_be_a_bug", + failure_isolation_node="not_the_real_node", + benign="kept", + ) + raise _TransientError("provider down") + + graph = ( + GraphBuilder(_DocState) + .add_node( + "extract", + _sets_then_raises, + middleware=[ + FailureIsolationMiddleware( + degraded_update={"note": "degraded"}, + event_name="extract_failed", + ) + ], + ) + .add_edge("extract", END) + .set_entry("extract") + .compile() + ) + client = InMemoryLangfuseClient() + observer = LangfuseObserver(client=client) + graph.attach_observer(observer) + + await graph.invoke(_DocState()) + await graph.drain() + + trace = next(iter(client.traces.values())) + marker = next(o for o in trace.observations if o.name == "openarmature.failure_isolated") + # The OA-emitted values survive the collision. + assert marker.metadata.get("error_category") != "caller_wins_would_be_a_bug", ( + "a caller metadata key overwrote the marker's failure discriminator" + ) + assert marker.metadata.get("failure_isolation_node") == "extract" + assert marker.metadata.get("failure_isolation_event_name") == "extract_failed" + # Non-colliding caller keys still come through: the fix is precedence on a + # collision, not dropping the caller set. + assert marker.metadata.get("benign") == "kept" diff --git a/tests/unit/test_observability_langfuse.py b/tests/unit/test_observability_langfuse.py index 8cb447a..992c7d3 100644 --- a/tests/unit/test_observability_langfuse.py +++ b/tests/unit/test_observability_langfuse.py @@ -2764,3 +2764,69 @@ async def test_a_long_surrogate_bearing_message_is_still_capped() -> None: f"the sanitize path bypassed the cap: {len(rendered.encode('utf-8'))} bytes" ) assert "[truncated," in rendered + + +async def test_the_tool_observation_carries_caller_metadata_like_the_others() -> None: + # §8.4.2 maps the caller-supplied invocation metadata to + # `observation.metadata.` on EVERY Observation. The table scopes its + # other rows explicitly ("fan-out node Span observation only"), so the + # unscoped wording is deliberate rather than loose. + # + # The Tool observation was the one provider observation that dropped it: the + # LLM handlers pick it up via `_typed_event_metadata`, embedding and rerank + # apply it directly, and this handler did neither. The OTel observer's tool + # span carried it all along, so the two observers disagreed about the same + # event, which is the divergence class the marker-span consistency choice + # exists to prevent. + from openarmature.graph.events import ToolCallEvent + from openarmature.observability.correlation import ( + _reset_invocation_id, + _set_invocation_id, + ) + + client = InMemoryLangfuseClient() + observer = LangfuseObserver(client=client) + + token = _set_invocation_id("inv-tool-meta") + try: + await observer( + ToolCallEvent( + invocation_id="inv-tool-meta", + correlation_id=None, + node_name="run_tool", + namespace=("run_tool",), + attempt_index=0, + fan_out_index=None, + branch_name=None, + call_id="cc-tool-meta", + tool_name="lookup", + tool_call_id="call_1", + arguments={"q": "x"}, + result={"a": 1}, + latency_ms=1.0, + caller_invocation_metadata={"tenant": "acme"}, + ) + ) + finally: + _reset_invocation_id(token) + + obs = next(o for o in client.traces["inv-tool-meta"].observations if o.type == "tool") + assert obs.metadata.get("tenant") == "acme", ( + f"the Tool observation dropped the caller metadata; got {obs.metadata}" + ) + # The OA-emitted key is still there: the caller set is merged first, not + # instead. + assert obs.metadata.get("openarmature_tool_name") == "lookup" + # Note on what this does NOT pin. Merging the caller set LAST here is + # behaviourally identical and no test catches it, which is correct rather + # than a coverage gap: every OA key this handler writes is reserved, the two + # `openarmature_*` ones by prefix and `error_type` / `error_message` by name, + # so a colliding caller key is rejected at the `invoke()` boundary and never + # reaches the merge. Precedence is unobservable on this observation by + # construction. Merge-first is for consistency with the other handlers, where + # it IS load-bearing. + # + # Worth knowing that this was reachable before 0119: `error_type` was + # unreserved then, so a caller key of that name merged last would have + # replaced the only discriminator a failed Tool observation carries. 0119 + # closed it from the reservation side.