Repository navigation
fix(transcription): honor supplied whisper.cpp environment - #554
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesPass the supplied environment to whisper.cpp availability checks and transcription subprocesses. Example: config.env.PATH contains /opt/whisper/bin, where whisper-cli is installed, and ambient PATH is absent.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR fixes a current environment-propagation bug with a focused patch; no actionable defects remain. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe core transcription adapter receives media and resolved environment settings from CLI or library callers, launches whisper.cpp through the shared process wrapper, and returns transcript text. flowchart TD
A[CLI config and process environment] --> B[Resolved environment]
B --> C[Core transcription adapter]
D[Media file] --> C
C --> E[Tracked whisper.cpp children]
E --> F[Transcript text]
Technical reviewBest possible solution: Land the two environment overlays in the existing core adapter with the accompanying regression coverage. Do we have a high-confidence way to reproduce the issue? Current-main source shows the missing spawn environment, and the contributor's exact-base CLI capture demonstrates the failure; no target code was executed during this read-only review. Is this the best way to solve the issue? The patch repairs the existing core owner directly, preserves ambient inheritance for partial environments, and avoids new configuration or parallel execution paths. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e1f209e1a9bd. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed November 2, 2026, 1:29 AM ET / 06:29 UTC (Revision 2). |
|
Added the captured built-CLI proof requested in the review, from the exact base and PR head. Both source checkouts stayed clean.
The capture below includes the recording executable source, each fresh config, the environment allowlist, normalized CLI invocation, child arguments, actual stdout/stderr, and exit status. The model and audio are deliberately synthetic: this proves subprocess environment propagation, without claiming speech inference. Local paths were normalized as stated in the log. No credentials or service endpoints are included. Captured terminal log, 2026-11-02 UTC |
|
The captured CLI setup and output are attached above and linked from the PR body. All six cases passed at the unchanged head @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Summary
whisper.cpp binary/model selection used the resolved environment, but its availability and transcription subprocesses inherited only the ambient process environment. Config-only variables and an explicitly supplied PATH could be lost at execution.
Pass the inherited environment with supplied values overlaid at both launch sites. Partial environments retain ambient variables, supplied values take precedence, and process.env is not mutated. Binary/model selection and provider ordering are unchanged.
Behavior proof
Captured built-CLI terminal log shows the exact base and PR commits, recording executable, fresh config, environment allowlist, invocation, child arguments, stdout/stderr, and exit status. Config-only values reach both PR child launches; the base drops them. A conflicting process value wins on both builds. Config-only PATH discovery fails on the base and succeeds on the PR.
The audio/model fixtures are synthetic. This verifies child-process environment propagation without speech inference.
Validation
pnpm run buildpnpm run check: 3,888 passed, 43 optional/live skipsExisting shell exports and installed daemon environment snapshots already worked. No new configuration fields or dependencies.
Maintainer validation
Validated the five environment regressions and
pnpm run checkon Linux (3,882 passed, 49 optional skips). Independent P0–P2 review is clean. Added the credited Unreleased entry; release versions are unchanged.