Activate conformance fixture 158 payload-leak arms - #273
Merged
Conversation
Fixture 158 exercises openarmature's fail-closed behaviour when a Langfuse client is bound to a provider it does not own. Against our detection-capable declaration it asserts five of its eleven cases: the four raise arms plus the ungated opt-out. Five suppress-floor cases are recognized skips selected by requires_capability, and one is deferred by name because it drives a retrieval node this harness cannot yet build. Both the skips and the deferral now surface on a passing run, so a case that did not execute cannot read as coverage. The provider-faithful double gains a payload-bearing classifier. It keys on the flattened metadata attribute a real client writes rather than on a single blob only the double emitted, which had made the harvested error-message channel undetectable on the client mode every case uses. The double now renders metadata the way a real client does, and a new test drives a real SDK client so the double is no longer the only witness for behaviour it exists to model. The log_records assertion matches on the emitting call site rather than on level alone. Construction always logs an unrelated cached-client warning on the same logger, so a level-only match was satisfied by that record while the mandated one went unchecked. 0115 moves to implemented now that both fixtures run.
There was a problem hiding this comment.
Pull request overview
This pull request activates spec conformance fixture 158 for Langfuse payload-leak “fail closed” behavior and updates the conformance harness and fakes so fixture 158’s assertions are observable and correctly classified against both the provider-faithful fake and the real Langfuse SDK path.
Changes:
- Activates conformance fixture
158-langfuse-payload-leak-fail-closedalongside 157 and updates the harness to surface non-executing cases asRecognizedSkipwarnings (capability-gated or per-case deferred). - Aligns the provider-faithful Langfuse fake with real v4 SDK metadata rendering (flattened
langfuse.observation.metadata.<key>attributes) and adds payload-bearing classification utilities for leak assertions. - Updates unit and conformance tests to validate payload-bearing classification against both the fake and a real egress-free Langfuse SDK client, and strengthens log-record assertions by filtering on the emitting call site.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_langfuse_provider_fake.py | Expands unit coverage for payload-bearing classification (provider payload, trace stub vs real state, metadata error_message) and cross-checks behavior against a real Langfuse SDK client. |
| tests/conformance/test_observability.py | Moves fixture 158 from deferred to activated in the Langfuse harness fixture set. |
| tests/conformance/test_observability_langfuse.py | Extends the isolation runner to cover fixture 158, adds per-case deferral reporting via RecognizedSkip, implements payload-bearing leak assertions, and tightens directive allowlists per runner. |
| tests/conformance/harness/langfuse_real_client.py | Adds prime_credential_on() to model “preexisting same-key client” behavior using the real SDK cache. |
| tests/conformance/harness/langfuse_provider_fake.py | Renders metadata in a v4-faithful flattened shape and adds span_is_payload_bearing() / payload_bearing_spans() plus trace-stub detection. |
| conformance.toml | Marks proposal 0115 as implemented and updates proposal notes to reflect fixture 158 activation and the implemented payload-bearing classification rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/conformance/test_observability_langfuse.py:3125
- This checks only that at least one matching decision record exists, but the contract and the comment above require exactly one WARNING. Emitting the warning twice (or emitting an additional level from the same decision site) would still pass. Compare the complete set of records from
_ISOLATION_DECISION_FUNCwith the expected levels so cardinality is enforced too.
matched = [
r for r in log_records if r.levelname == level and r.funcName == _ISOLATION_DECISION_FUNC
]
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Activates spec conformance fixture 158 (
158-langfuse-payload-leak-fail-closed) and flips proposal 0115 frompartialtoimplemented. This completes the conformance side of the 0114-0118 provider-isolation arc: 157 and 158 now both run.Case accounting
Against our
langfuse_bound_provider_detection = truedeclaration, 158 asserts five of its eleven cases: the four raise arms plus the ungated opt-out. Five suppress-floor cases are recognized skips selected byrequires_capability(§5.5), and one case is deferred by name because it drives acalls_reranknode this harness has no retrieval support for at all; it rides the retrieval cluster that 150/151 also wait on.All six non-executing cases now surface as
RecognizedSkipwarnings on a passing run. Previously the per-case deferral was written into a local dict that is only rendered inside an assertion failure message, so a green run looked identical whether the fixture asserted eleven cases or five.What the adversarial review changed
The first version of this passed, and three of its defects were real. Worth recording since two of them defeated the mutation testing that was supposed to catch exactly this:
The error-message channel was undetectable on the real client. The payload classifier read the bare
langfuse.observation.metadataattribute, but a Langfuse v4 client only writes that name for non-dict metadata; a dict flattens tolangfuse.observation.metadata.<key>. We always pass a dict, and every 158 case ismode: credentials, so a span carrying leaked exception text classified payload-free. The only test of that limb drove the fake, which was the one client writing a single blob, so the double both produced the evidence and judged it. The classifier now keys on the flattened attribute (parsing the bare blob for the non-dict case rather than substring-matching, which would false-positive on caller metadata values), the fake renders metadata the way a real client does, and a new test drives a real SDK client through the egress-free harness.The
log_recordsassertion matched the wrong record. Every 158 case primes the credential, sofrom_credentialsalways logs an unrelated cached-client warning on the same logger inside the capture window. Matching on level alone was satisfied by that record, and downgrading only the mandated emitter left the fixture green. It now discriminates on the emitting call site.A case-level directive sat behind an early return.
disable_provider_payloadwas read after theexpected_construction_errorbranch returns, so it never executed for the only case that declares it while_CASE_DIRECTIVESvouched for it as implemented. The OTel observer is now built before the Langfuse construction and the runner asserts the flag reached it.Also: the generic
_run_caserunner had been granted allowlist credit for six keys only the isolation runner reads, which would have converted a loud failure into silent coverage for any future fixture on that path. The allowlists are now per-runner. And theinspect.getsourceguard on the trace-stub shapes was replaced with a behavioural one, since it could not detect a stub gaining a key, which is the exact drift its own comment claimed to catch.Verification
2108 passed, 507 skipped. Ruff, pyright and the conformance-manifest check clean. Conformance and unit suites re-run together for order contamination, which matters here because the raise cases prime the SDK's real per-credential cache.
Each fix is mutation-verified, with the mutation confirmed to have landed before its result was trusted:
The one case that cannot be killed this way is
singleton_preexists_raises_otel_not_suppressing, which is a negative control: construction raises before anything emits, so no conforming run can observe the flag. Spec has since confirmed the case is inert for every adapter under the current contract, because the case-level key is not in the adapter contract at all, and proposal 0120 will add a realotel_observer:directive to carry it.Notes
Two findings from this work went to spec as items 11 and 12 of the consolidated v0.17.0 review, and both were ruled in our favour. Fixture 158's
log_recordscannot pin the WARNING it exists to pin, which is a gap in §6 rather than in the fixture, and case 2 discriminates nothing for anybody. Expect a follow-up rewiring this runner onto 0120's sanctioned spelling once that lands.