feat(voice): instrument judge STT pre-pass - #880
Conversation
|
Reviewing this so it can land. I have not pushed anything and I have not marked it ready. It is your PR. CI is unblocked and greenThe workflow runs were sitting at I also checked it locally: it rebases cleanly on current 1. The Python span scope is hardcoded where the TypeScript one is gated
On the TS side the span is opt-in. Python has no equivalent gate. Nothing is mislabelled today. Suggestion, mirroring the TS shape: async def transcribe_segments(
recording: VoiceRecording,
provider: Optional[STTProvider] = None,
only_missing: bool = True,
telemetry_scope: Optional[str] = None,
) -> None:Emit the span only when 2. The WARNING log now loses the provider's own messageWorth knowing, not a change request. Before, the outer handler formatted the original exception, so a default-level log read roughly: Now it formats the sanitized and the real detail only exists at DEBUG. This is defensible. It is a faithful copy of the #783 guard in Test coverageGood. Both runtimes, mixed success and failure inside one batch, and the sanitization asserted against the raw key and body rather than just the message shape. That last part is the one people usually skip. To land itMark it ready for review, then get one approving review. The auto-approve workflow skips fork PRs by design, so it will never approve this one. One red check you can ignore: |
32ada9e to
e28991d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughJudge pre-pass STT transcription now supports optional judge telemetry scoping in JavaScript and Python. Instrumented calls record audio and transcript metadata, sanitize provider errors, preserve per-message failures, and retain uninstrumented behavior for default callers. ChangesJudge STT telemetry
Sequence Diagram(s)sequenceDiagram
participant JudgePrePass
participant ScopedTranscription
participant STTProvider
participant Telemetry
JudgePrePass->>ScopedTranscription: request transcription with scope="judge"
ScopedTranscription->>Telemetry: start voice.stt.transcribe span
ScopedTranscription->>STTProvider: transcribe audio segment
STTProvider-->>ScopedTranscription: transcript or provider error
ScopedTranscription->>Telemetry: record metadata or sanitized error
ScopedTranscription-->>JudgePrePass: transcript result per segment
Suggested reviewers: Poem Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for the careful review. I addressed the Python scope asymmetry in e28991d: I also added coverage proving a direct public call still transcribes without emitting a judge span, and updated the judge-boundary call assertions. The branch is rebased onto current |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
javascript/src/voice/__tests__/judge-stt-telemetry.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the test requirements.
Line 1 does not state how to run this test, required dependencies, coverage expectations, or a test example. Add a short header or a link to the project test documentation that provides these details.
As per coding guidelines: “Document testing requirements explaining how to run tests, test coverage requirements, test dependencies, and providing test examples.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/voice/__tests__/judge-stt-telemetry.test.ts` at line 1, Expand the header comment in the judge-stt telemetry test to document how to run it, required test dependencies, expected coverage, and a representative test example, or link to the project’s existing testing documentation that covers all four requirements.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@javascript/src/voice/__tests__/judge-stt-telemetry.test.ts`:
- Line 1: Expand the header comment in the judge-stt telemetry test to document
how to run it, required test dependencies, expected coverage, and a
representative test example, or link to the project’s existing testing
documentation that covers all four requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eaec8783-ea60-40a8-89e9-e9dbcfd01c9d
📒 Files selected for processing (6)
javascript/src/voice/__tests__/judge-stt-telemetry.test.tsjavascript/src/voice/judge-stt.tspython/scenario/judge_agent.pypython/scenario/voice/_transcribe.pypython/tests/test_judge_agent.pypython/tests/voice/test_judge_stt_telemetry.py
langwatch-agent
added
the
hound-checked
langwatch-agent
added
the
ci-green
Human Review BriefMode: Targeted Review. Closes #785, and the PR does what the issue asks: the judge pre-pass STT was absent from Decisions being ratified
Must Check
Ask AuthorIf the opt-in flag is ever set on a direct caller, do the two runtimes emit identical attributes? That is the moment the alignment claim gets tested for real. |
langwatch-agent
left a comment
langwatch-agent
left a comment
There was a problem hiding this comment.
Found one privacy issue in the Python STT failure path. I reviewed this external contribution statically; I did not execute contributor code or install dependencies.
LangWatch-Review: verdict=findings sha=e28991d907daaac970b3d5c30d50954799ba1076 p0=0 p1=0 p2=1 p3=0
| # Provider SDK errors can include response bodies and key fragments. | ||
| # Keep the raw detail local and let telemetry record only a minimal | ||
| # provider-agnostic exception, matching the #783 STT guard. | ||
| logger.debug( |
There was a problem hiding this comment.
P2 — Do not log the raw provider exception in the sanitized path. exc_info=True serializes the original exception message and traceback into debug logs. STT SDK failures can contain provider response bodies or credential fragments—the exact data this change prevents from reaching telemetry—so enabling debug logging reintroduces that disclosure under a second observable path. Log only the exception type (or remove this debug entry) before raising the sanitized RuntimeError.
drewdrewthis
left a comment
drewdrewthis
left a comment
There was a problem hiding this comment.
Automated pr-review pass. Inline findings below; verdict comment upserted separately.
| # provider-agnostic exception, matching the #783 STT guard. | ||
| logger.debug( | ||
| "scenario.voice.transcribe: STT provider error detail", | ||
| exc_info=True, |
There was a problem hiding this comment.
[review-clerk, Fix] This logger.debug(..., exc_info=True) still serializes the raw provider exception (message + traceback) into the scenario.voice DEBUG logger — the exact content this PR sanitizes out of the OTel span. If DEBUG logging is ever enabled for this logger (local debugging, a verbose log level in prod), provider response bodies / key fragments (e.g. sk-...) leak through a second, unsanitized channel, defeating the PR's own stated goal.
This concurs with the still-unresolved langwatch-agent review thread on this same line (posted 2026-08-13T06:52:00Z, isResolved=false) — flagging as a review-clerk finding too so it participates in this review's own gate. Suggest: drop exc_info=True (log only type(exc).__name__, already available at line 115) or move the raw detail behind an explicit opt-in flag, never a bare DEBUG level on this logger.
| expect(warn.mock.calls[0]![0]).toContain("STT provider failed: Error"); | ||
| expect(warn.mock.calls[0]![0]).not.toContain(rawError); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[review-clerk, Fix/New AC] Missing test: nothing here proves a direct (non-judge) transcribeAudioMessages call stays uninstrumented — i.e. emits zero voice.stt.transcribe spans when telemetryScope is omitted.
The Python side has this exact test (test_direct_transcription_is_uninstrumented_by_default in python/tests/voice/test_judge_stt_telemetry.py), and the PR description explicitly claims parity: "Direct TypeScript and Python transcription callers remain unchanged and uninstrumented." Right now only the Python half of that claim is evidenced. Add a JS sibling test calling transcribeAudioMessages (or the underlying non-judge path) without telemetryScope and asserting exporter.getFinishedSpans() has no voice.stt.transcribe span.
drewdrewthis
left a comment
drewdrewthis
left a comment
There was a problem hiding this comment.
See verdict comment below for full details. NOT-READY: 2 new review-clerk findings (DEBUG-log leak of raw provider exception; missing JS parity test) plus 1 pre-existing unresolved thread from langwatch-agent. CI has not actually run on this SHA — see verdict.
Review verdict: NOT-READYReviewed at: CI status — the green on this SHA does not cover the changed code
PR-body gaps (pr-ready-check C5/C6)Body has "## Test plan" and "## How I can prove I was successful" but no "## Human verification" heading, and no explicit backend-only-change declaration under that exact heading — this is an internal telemetry change with no UI surface, so the fix is administrative (rename/add the heading), not a design problem. Blocking — must resolve before this PR is done
Non-blocking
Verdict is prose, not a GitHub approval. Scope: review findings only — READY means no unresolved blocking review threads at this SHA. It is not a merge-readiness signal; that is |
langwatch-agent
left a comment
langwatch-agent
left a comment
There was a problem hiding this comment.
No actionable correctness, security, or integrity finding in the external diff. The judge-only instrumentation leaves public direct transcription unchanged and replaces provider errors before telemetry/logging observes them. I performed static review only; contributor code and dependencies were not executed.
LangWatch-Review: verdict=clean sha=fac25413a1562c1f16b4dcb415cc8df7a056a5a5 p0=0 p1=0 p2=0 p3=0
langwatch-agent
left a comment
langwatch-agent
left a comment
There was a problem hiding this comment.
External static review: no blocking concern found in the current diff. I did not execute branch code, install dependencies, or run contributor-provided scripts. Residual risk: runtime behavior remains covered by the repository CI.
LangWatch-Review: verdict=clean sha=fac25413a1562c1f16b4dcb415cc8df7a056a5a5 p0=0 p1=0 p2=0 p3=0
langwatch-agent
added
the
blocked-with-author
4 participants
Why
The judge pre-pass performs a second STT pass that was absent from
voice.stt.transcribetelemetry in both runtimes, so STT volume and latency were undercounted. Closes #785.What changed
How it works
prepareJudgeInputandJudgeAgentopt into avoice.stt.transcribespan for each audio message or segment. Direct TypeScript and Python transcription callers remain unchanged and uninstrumented.Test plan
python/.venv/Scripts/python.exe -m pytest python/tests/voice/test_judge_stt_telemetry.py python/tests/test_judge_agent.py -q- 28 passedcd javascript && vitest run src/voice/__tests__/judge-stt-telemetry.test.ts- 3 passedcd javascript && eslint src/voice/__tests__/judge-stt-telemetry.test.tscd javascript && tsc --noEmitHuman verification
This is an internal telemetry change with no UI surface. The regression tests verify the exported span attributes, mixed-batch isolation, absence of raw provider error content, and that direct transcription remains uninstrumented by default.