Repository navigation
fix(cli): use a healthy pinned Sherpa copy beside the CLI - #5095
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Edit accuracy: accurate 2055 (base branch 2055), smooth 1597 of thoseThe gate passes. Quarantined, measured but not gated (0) |
somanshreddy
left a comment
There was a problem hiding this comment.
Review at 11eae121 (full PR, including the merge with main). This is a comment, not an approval.
No blockers.
- Runtime selection is consistent end to end:
selectRuntimetries the pinned copy beside the CLI first, probing it in a child.- It logs one line if that copy is broken, then probes the cache copy.
- The decode worker gets the exact selected entry, so the probe and the decode can't pick different copies.
- The merge with #5103 keeps both behaviours in
transcribeWithSherpa: thestartedandcompletedevents surround the new selection and decode. - The probe's switch from
spawnSyncto asyncspawnhandles each way it can stop: a timeout reports "timed out" (becausestoppedByCancelSignalreturns false when the child waskilled), Ctrl-C or abort becomesDecodeCancelled, and a crash reports its signal.
Low
- Each Parakeet transcription now starts an extra Node child that loads the native runtime before the decode worker loads it again (
sherpa.tstranscribeWithSherpa→selectRuntime). With a broken pinned copy it starts two, and the "did not load" line prints on every run. Under--json, that line now sits among #5103's JSON progress lines on stderr. Consider caching the selection per process. Alternatively, skip the probe in transcribe and let the worker's load error carry the repair message. installedOptionalPackageVersiononly trustsresolution === "entry", but no test pins that. Mutant M8, accepting the"package"resolution as well, survives. Today onlysherpa-onnx-nodereaches the package branch, so it has no effect yet.- The
startedevent fires before runtime selection. A failed selection leaves astartedwith no terminal event. Callers that pair events would need to treat the thrown error as the end.
Mutations: 7 of 8 caught against the 5 changed test files:
- pinned copy ignored;
- no cache fallback after a broken pinned copy;
- any version accepted;
- the package-resolution branch dropped;
- "installed" ignoring the pinned copy;
- a probe timeout counted as healthy;
loadBesideCliloading a package-only resolution.
M8 survived (low 2).
What I ran: the 5 changed test files pass 91/91 (vitest, packages/cli). I didn't exercise real native sherpa or onnxruntime binaries, the same limit the PR body states.
CI at this head: 68 pass, 20 pending, 7 skipped, 0 failing so far.
— Somu
somanshreddy
left a comment
There was a problem hiding this comment.
Re-check at 6692cb1e: the only change from 11eae121 is that docs/contracts/2026-10-05-sherpa-beside-cli.html was removed (GitHub compare: 1 commit ahead, 0 behind, one file removed). The code is identical, so my review at 11eae121 (5423978817) still applies unchanged: no blockers, three lows. This is a comment, not an approval.
— Somu
terencecho
left a comment
There was a problem hiding this comment.
Re-review at 6692cb1e18dd21afd1ded6cae176a9987370bacb after my earlier hold on the unresolved #5103 merge conflict:
- The merge resolution in
packages/cli/src/whisper/sherpa.ts:440–479preserves both #5103'sstarted/completedprogress events and this PR's selected-runtime handoff todecode(wavPath, selected.path, signal). The worker loads that exact path; the added happy-path test pins the event sequence. The conflict that prevented an approval atd69eac0cis resolved. - The pinned adjacent package is exact-version checked and probed in a child, with a healthy cache fallback and cancellation kept distinct from probe failure. I checked the installer, availability reader, and decode worker paths. As Somu noted in the at-head comment, the additional native probe has a per-transcription cost and the
resolution === "entry"installed-version guard lacks a direct test. The live guard is correct; thestarted-without-completedfailure behavior and diagnostics alongside JSON progress match the pre-existing #5103 contract. - From the reviewed
11eae121to this head, GitHub compare shows only removal ofdocs/contracts/2026-10-05-sherpa-beside-cli.html; runtime and test source are unchanged. Somu's 91/91 focused run applies to the unchanged code; I did not rerun native binaries, real audio, or packaged-CLI adjacency locally.
No remaining code-merit blocker found. The required checks visible at review time passed; this approval is not a merge action.
Verdict: APPROVE
Reasoning: The conflict resolution preserves both runtime selection and progress semantics, and the re-pinned head changes only the design document relative to the audited source.
— Review by tai (pr-review)
When a pinned copy shipped beside the CLI is available, Parakeet should use it without requiring another runtime installation. Previously, installation checks and decoding only looked in the cache.
This change shares exact-version package discovery, probes native loading in a cancellable child, and passes the selected entry directly to the decode worker. A broken pinned copy emits one line naming its path and load error, then a healthy cache may be used. If neither loads, transcription reports repair guidance.
Installation JSON retains
runtimeDiras the cache destination and addsruntimePathfor the selected healthy entry.Validation
Validated at
d69eac0cbb7c9da57e84997d92ed8cc134f303aa.No visible change
Runtime loading and command JSON only. No Studio or player UI changes.
Limits
Fixture packages prove routing and child isolation. Real native binaries and speech model decoding are not exercised by these tests.
Small-PR note: one runtime-loading fix spans discovery, health probing, availability, installation reporting and the existing worker handoff so they cannot select different copies.