fix(langfuse): reject right-prefix template keys, not just wrong prefixes - #64309
Solitud1nem wants to merge 1 commit into
Conversation
…ixes The placeholder guard from NousResearch#26320 checks only that keys start with pk-lf-/sk-lf-. The catch: our own docs (plugin README, website env-var reference) print the keys as literally `pk-lf-...` — so the most likely copy-paste in existence sails through the guard, the SDK happily constructs a client, and every trace dies at flush time with zero log output. That's why the same silent-failure report kept getting filed after the guard merged: NousResearch#51399 and six duplicates, all post-fix. The validator now looks past the prefix: a remainder that IS a template (`...`, `***`, empty) or CONTAINS an unambiguous template fragment (placeholder, your-, test-key, changeme, example, unset, dummy, xxx) gets the same one-shot warning + short-circuit as a wrong-prefix value. False positives are structurally impossible for issued keys — Langfuse remainders are UUID-shaped (hex + dashes), and every fragment contains at least one non-hex letter. Runtime ingestion-failure surfacing (the other half of what a dead PR in this area attempted) is deliberately out of scope: it's a separate failure mode, and NousResearch#62882 is already working the runtime path. Fixes NousResearch#51399
|
Thanks for isolating the residual right-prefix case. Current main's The PR base has no current-main changes in either touched file, so this is mechanically salvageable without target-region conflict. Automated hermes-sweeper review. |
|
Reviewed in the context of #66960 (another placeholder-key silent-failure report), which #66960 was triaged as a duplicate of #51399. This PR does fix the outstanding class of the bug. The root cause is correctly identified: the guard from #26320 checks only Coverage vs the reports is solid:
Minor things to weigh (none blocking):
Overall: correct root cause, matches the reproduction, well-tested (both the reject and accept directions), and minimal-footprint. LGTM for closing the #51399 / #66960 placeholder-key line once merged. |
Thanks — that's a genuinely careful read, and you caught the two things worth calling out. On On the startup ping — declining it deliberately, and putting that on the record here so it's a decision rather than an oversight. A network call at plugin init buys a narrow win (catching a well-formed but revoked key) for a real cost: latency on every startup, a new failure mode when Langfuse is unreachable or slow, and ambiguity about what to do when the probe times out — none of which the reported misconfigurations need. Every reproduction in #51399, #66960 and the rest of the cluster is a template value copy-pasted from our own docs, and that's detectable offline, which is what this PR does. If someone later wants revoked-key detection, that's a separate change with its own trade-off discussion — #52389 is the thread where real-shaped-but-revoked keys came up, and it would need a live And yes — your read on #66960 matches mine: its literal examples are wrong-prefix values already caught on |
|
Independent verification (swarm dedup campaign, worker t_82825c05) — premise re-verified against current main after the Jul 29 suite-wide test prune (6b81590, 46,820 -> 28,106 test functions):
Post-prune green-light confirmed: this PR is the correct fix for the #51399 cluster's residual right-prefix template gap. |
andrexibiza
left a comment
There was a problem hiding this comment.
Independent verification against current main (2026-08-02, commit dd600d1) — cluster worker for #51399 dedup campaign.
Premise confirmed on current main
_validate_langfuse_key()at plugins/observability/langfuse/init.py:141-142 still returns None for every value with the right prefix. The docs' literal copy-paste templatespk-lf-.../sk-lf-...pass the guard, the SDK client is constructed, and traces die silently at flush time — exactly the #51399 failure mode. The merged #26320 guard only covered wrong-prefix values.
Empirical test evidence (Windows 11, Python 3.12.12, pytest 9.1.1)
- Clean
main(dd600d1): 24 passed, no right-prefix coverage. - 3-way merge of this PR onto current main (
git merge-tree --write-tree origin/main pr64309-head): clean tree, GitHub reports MERGEABLE; the new tests land insideTestPlaceholderKeyDetection. - Merged tree, full plugin suite: 37 passed (24 baseline + 13 new: 10 parametrized right-prefix rejects, 2 UUID-shaped accepts, 1 docs-template end-to-end warn-and-skip).
- The 13 new tests run against the clean-main plugin (PR tests only, no plugin change): 11 failed — the 10 right-prefix template rejections plus
test_docs_template_keys_warn_and_skip— proving the regression tests cover the actual reported hole, not a strawman. The 2 accepts passed as expected.
Notes for the author / reviewer
- The PR's test-file base drifted after suite-wide test prune 6b81590 removed
test_validate_langfuse_key_unknown_name_passes(the anchor the test hunks sit on):git applyof the PR diff fails on the test file, but the 3-way merge is clean and all tests pass in the merged result. A rebase/merge of main is advisable for hygiene but is not merge-blocking. - Matches the keep_open review (teknium1, Jul 16): extension of the existing validator, preserves the one-shot
_INIT_FAILEDshort-circuit, no target-region conflict.
Verdict: green-light. This is the agreed best fix for the #51399 cluster (right-prefix template keys); merge when a maintainer approves.
|
Superseded by #77373 — please merge that one instead. Same change: every added and removed line is identical, and the commit there is mine with authorship intact. Only the context differs, because that branch sits on a post-prune base. Re-verified today on main 82c6aca: the gap is live on a clean tree ('pk-lf-...' still accepted, 24 passed), and in the tree merged with 77373's head the templates are rejected while UUID-shaped keys still pass (37 passed). I will close this once #77373 lands. If a maintainer would rather clear it from the queue now, go ahead — the fix is not lost. |
What does this PR do?
The placeholder guard from #26320 checks only that credentials start with
pk-lf-/sk-lf-. But the plugin README and the website env-var reference both print the keys as literallypk-lf-.../sk-lf-...— and that exact copy-paste passes the prefix check on today'smain: the SDK constructs a client without complaint and every trace dies at flush time with zero log output, which is precisely the silent failure the guard was built to catch. The new regression test proves it (test_docs_template_keys_warn_and_skipfails onmain)._validate_langfuse_key()now looks past the prefix. A remainder that is a template (...,***, empty — i.e. a bare prefix) or contains an unambiguous template fragment (placeholder,your-,test-key,changeme,example,unset,dummy,xxx) gets the same one-shot warning and_INIT_FAILEDshort-circuit as a wrong-prefix value. False positives are structurally impossible for issued keys: Langfuse remainders are UUID-shaped (hex digits and dashes), and every fragment in the list contains at least one non-hex letter — a property the tests pin. A side effect worth noting: the docs' ownpk-lf-...template is now caught, so the copy-paste footgun closes without touching the docs or their translations.Context on the report chain: #51399 has six duplicates filed after #26320 merged. Some of that stream is likely version lag (triage noted "the fix ships once a release tag carries #26320"), so I'm not claiming every duplicate hit this exact hole — but the hole itself is real and test-proven above, and it's the most likely paste value in existence.
Deliberately out of scope, two things: (1) surfacing runtime ingestion failures — #62882 is already reworking that path, and this PR stays clear of it; (2) the adjacent gap Bartok9 described on #52389: a revoked or invalid key with a real UUID shape can't be caught statically at all — that needs a live auth probe (e.g. the SDK's
auth_check()) with its own network/offline tradeoffs, and deserves its own PR.Related Issue
Fixes #51399
Duplicate chain (all marked duplicates of #51399 by triage; canonical original #22763 is closed): #52377, #52389, #57949, #60308, #60961, #63787.
Type of Change
Changes Made
plugins/observability/langfuse/__init__.py—_validate_langfuse_key()gains a remainder check after the existing prefix check: whole-value templates by exact match, conservative fragments by substring, case-insensitive; new_PLACEHOLDER_REMAINDERS/_PLACEHOLDER_FRAGMENTSconstants with the false-positive reasoning documented inlinetests/plugins/test_langfuse_plugin.py— 13 new tests in the existingTestPlaceholderKeyDetectionclass: 10 parametrized right-prefix templates rejected (including the docs' literalpk-lf-...), 2 UUID-shaped real keys still accepted, and an end-to-end_get_langfuse()case proving the docs copy-paste now warns and never constructs a clientHow to Test
HERMES_LANGFUSE_PUBLIC_KEY=pk-lf-...+HERMES_LANGFUSE_SECRET_KEY=sk-lf-...(the literal values from the plugin README), run any traced operation.pk-lf-<uuid>): behavior unchanged.pytest tests/plugins/test_langfuse_plugin.py -q— 61 passed; the 11 new rejection tests fail onmain.Checklist
Code
fix(scope):,feat(scope):, etc.)tests/plugins/test_langfuse_plugin.py— 61 passedDocumentation & Housekeeping
pk-lf-...template is now caught by the guard, closing the copy-paste footgun without churning docs and their translationscli-config.yaml.example— N/A (no config keys)CONTRIBUTING.mdorAGENTS.md— N/AScreenshots / Logs
Fail-before / pass-after on today's
main: