fix(build): forward tokenjuice-treesitter + gate feature-forwarding drift in CI (#4918, #4919)#4934
Conversation
…4901) The Tauri shell embeds the core in-process (tinyhumansai#1061), so its dependency declaration is what actually ships. It has carried `default-features = false` since the in-process move, back when the core crate had no `[features] default` list at all — a harmless no-op. tinyhumansai#4833 then made `voice` a default-ON compile-time feature so slim builds could drop it. That silently collided with the pre-existing `default-features = false`: the shipped app has been built WITHOUT the voice domain ever since, so the `#[cfg(feature = "voice")]` controllers in `src/core/all.rs` were never registered and every `openhuman.voice_*` RPC answered "unknown method". Evidence: `hound` (reachable only via `voice = ["dep:hound", ...]`) was absent from the shell's dependency graph while present in the root crate's. Sentry confirms in production — `voice_status` (46,735 events / 56 users) and `voice_server_status` (46,592 / 52) both unknown-method, first seen 2026-07-14, hours after tinyhumansai#4833 landed, matching first bad release 0.58.19. - Cargo.toml: opt `voice` back in explicitly, and document that every future core default must be re-added here. - lib.rs: assert `VOICE_COMPILED_IN` at compile time. Cargo features are per-crate, so the shell cannot `#[cfg]` on the core's features; this const assert is the only way to make a voice-less desktop binary fail the build instead of failing silently at runtime. Verified red/green. - sttClient.ts: the "restart to pick up the latest core sidecar" copy was written for tinyhumansai#1289 (a stale bundled sidecar, removed in tinyhumansai#1061) and cannot ever resolve a compile-time gate. Replaced with an accurate reason and a real action, keeping the "unavailable in this build" substring that MicComposer matches to suppress the pointless retry loop. Tests asserting the misleading restart hint were pinning the bug; they now assert the accurate message and that restart advice is not reintroduced. Fixes tinyhumansai#4901
Conflict in app/src-tauri/Cargo.toml: the media feature gate (tinyhumansai#4804/tinyhumansai#4840) landed on main and hit the same trap as tinyhumansai#4901 — `default-features = false` silently dropping a default-ON core gate from the shipped desktop build. It forwarded `media`; this branch forwards `voice`. Both are required, so the resolution keeps both rather than either side. Merged the two comments into one rule ("every default-ON gate must be forwarded explicitly, a missing gate vanishes silently") and noted that `tokenjuice-treesitter` is the remaining un-forwarded default (tinyhumansai#4918). Verified after merge: `cargo tree -e features -i openhuman` resolves both `feature "media"` and `feature "voice"`; shell `cargo check` green (so the VOICE_COMPILED_IN assert passes); 212 related Vitest tests pass; fmt clean.
…t of mod.rs Addresses review feedback on tinyhumansai#4917. Two of three findings were valid. i18n (valid): `MicComposer` interpolated the raw English error into the translated `mic.transcriptionFailed` template ("Transcription failed: {message}"), so non-English locales rendered a half-translated sentence. CI could not catch this — `i18n:english:check` scans locale files, not service code. - sttClient now throws a typed `VoiceNotCompiledError` carrying a stable `VOICE_NOT_COMPILED_CODE`. The English message is kept deliberately: it is what reaches debug logs and Sentry. - `isVoiceNotCompiledError` duck-types on `code` rather than `instanceof` — the error crosses async/retry boundaries where prototypes can be stripped. - `MicComposer` branches on the code and renders `mic.voiceNotCompiled`. - Key added to all 14 locales with real translations (no em dashes, per AGENTS.md). `i18n:check` and `i18n:english:check` both pass. mod.rs (valid): AGENTS.md requires mod.rs be export-only. Moved `VOICE_COMPILED_IN` to a sibling `compile_status.rs`, deliberately ungated (it must exist in both gate states — reporting which state we are in is its purpose) and re-exported. Tests pin the const to `cfg!(feature = "voice")` rather than a literal, so they hold for both builds. transport provenance (skipped): not reachable. `setActiveCoreTransport` has exactly one occurrence in the tree — its own definition — so `_activeTransport` is always null and every `callCoreRpc` hits the local core. The `unknown method` rewrite condition is also pre-existing, unchanged by tinyhumansai#4917. Threading provenance through the shared RPC client for a dead path is not minimal; it belongs with whatever work activates the iOS/tunnel transport. Also fixes a latent test defect: MicComposer.test.tsx hand-listed its sttClient mock exports, so any newly imported export silently resolved to undefined and threw a TypeError at the call site. Now spreads importOriginal(). Verified: compile_status tests pass with voice ON and OFF (2/2 each); shell cargo check green (const assert resolves via the re-export); 78 voice tests pass; i18n checks, tsc, ESLint, Prettier, cargo fmt all clean; locale diff is 27 insertions / 0 deletions.
…rift in CI Closes tinyhumansai#4918 and tinyhumansai#4919 — the remaining un-forwarded gate, and the check that stops the next one from slipping through. tinyhumansai#4918 — the desktop app has never shipped with tree-sitter. The shell declares `openhuman_core` with `default-features = false`, so `tokenjuice-treesitter` (default-ON in the core) was silently dropped and TokenJuice fell back to the brace-depth heuristic instead of the AST-aware path. Unlike tinyhumansai#4901 this failed *soft*: no error, no Sentry signal, just quietly worse compression, which is why it went unnoticed since the gate was added in tinyhumansai#4123. The exclusion was NOT deliberate. tinyhumansai#4575 ("fix tinyjuice build wiring") set out to "forward the correct TinyJuice tree-sitter feature name" and named broken `tokenjuice-treesitter` resolution as its root cause — the intent was clearly for this to work. The shell's separate `tinyjuice = { default-features = false }` is version alignment for the 0.2.1 patch, not a veto. Cost: 5 crates (tree-sitter + Rust/TS/Python grammars, C build); the core already pays it. Verified with the probe that diagnosed tinyhumansai#4901: `cargo tree -i tree-sitter` from app/src-tauri went from "did not match any packages" to resolving via tinyjuice -> openhuman -> OpenHuman. Shell `cargo check` green. tinyhumansai#4919 — nothing enforced that the shell's forwarded list matches the core's `default` list. Two of three gates have now been dropped, by two different authors, one of whom knew about the trap and documented it in a comment. A comment is documentation, not enforcement. - scripts/lib/feature-forwarding.mjs — narrow TOML reader + drift diff. No TOML dep exists for Node, and only two well-known shapes are needed, so it is scanner-based like the sibling checklist-parser.mjs. - scripts/ci/check-feature-forwarding.mjs — CLI + INTENTIONALLY_NOT_FORWARDED allow-list (empty by design). An intentional exclusion must carry a reason, so "deliberate" and "forgotten" stop looking identical — that ambiguity is what let tinyhumansai#4918 sit. - New always-on CI lane, wired into the PR CI Gate. Deliberately not filtered on changed paths: drift can come from either manifest, and a skipped job counts as a pass in the gate. - AGENTS.md documents the rule next to the gate docs. The checker refuses to pass vacuously: parsing zero core defaults exits 2 rather than reporting success. That caught a real bug in my own parser during development (`\s` matches newlines, so `^\s*\[features\]` anchored early and sliced the section to nothing). Verified red/green on the real manifests: green as committed; dropping voice and tokenjuice-treesitter fails with exit 1 naming both. 18 unit tests, including regression cases reproducing tinyhumansai#4901 and tinyhumansai#4918, and a case proving a brand-new default gate is covered with no per-gate wiring. Full `pnpm test:scripts` suite passes (103 tests).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR forwards default-on core feature gates through the Tauri shell, adds CI drift detection, exposes compile-time voice availability, and replaces stale-sidecar voice errors with typed, localized frontend handling. ChangesFeature gating and voice availability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MicComposer
participant sttClient
participant DesktopShell
participant openhuman_core
participant Localization
MicComposer->>sttClient: transcribeWithFactory()
sttClient->>DesktopShell: voice_stt_dispatch RPC
DesktopShell->>openhuman_core: invoke voice domain
openhuman_core-->>DesktopShell: unknown method when voice is absent
DesktopShell-->>sttClient: RPC error
sttClient-->>MicComposer: VoiceNotCompiledError
MicComposer->>Localization: t('mic.voiceNotCompiled')
Localization-->>MicComposer: localized unavailable message
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/features/human/voice/sttClient.test.ts (1)
100-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed compile-gate contract.
These tests only pin message text, so a regression to a generic
Errorwould pass whileMicComposerno longer selects its localizedisVoiceNotCompiledErrorpath. AssertVOICE_NOT_COMPILED_CODE(orisVoiceNotCompiledError) for both RPC paths.
app/src/features/human/voice/sttClient.test.ts#L100-L105: assert the rejected cloud error has the stable voice-not-compiled code.app/src/features/human/voice/sttClient.test.ts#L169-L174: assert the rejected factory error has the same code.🤖 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 `@app/src/features/human/voice/sttClient.test.ts` around lines 100 - 105, Update both cloud and factory error tests in app/src/features/human/voice/sttClient.test.ts (lines 100-105 and 169-174) to assert the rejected error has the stable VOICE_NOT_COMPILED_CODE, or satisfies isVoiceNotCompiledError, in addition to the existing message assertion. Use the same typed compile-gate assertion for both RPC paths.app/src/features/human/MicComposer.tsx (1)
579-586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated log line for the compile-gate branch.
The branch correctly detects
VoiceNotCompiledErrorand shows localized copy without retry, matching the upstream contract and tests. However, unlike other branches in this function (which use[session:%d] ...prefixes), this one relies solely on the earlier generictranscribe failed: %slog, making it harder to grep specifically for compile-gate hits.♻️ Suggested addition
if (isVoiceNotCompiledError(err)) { + composerLog('[session:%d] voice not compiled — showing localized error', sessionIdRef.current); onError?.(t('mic.voiceNotCompiled')); return; }As per coding guidelines, "New or changed flows should log entry and exit, branches, external calls, retries/timeouts, state transitions, and errors using stable grep-friendly prefixes and correlation fields."
🤖 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 `@app/src/features/human/MicComposer.tsx` around lines 579 - 586, Add a dedicated stable, grep-friendly log entry in the isVoiceNotCompiledError branch, including the existing session correlation prefix used by neighboring branches, before invoking onError and returning. Keep the localized error handling and no-retry behavior unchanged.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 `@app/src/features/human/MicComposer.tsx`:
- Around line 579-586: Add a dedicated stable, grep-friendly log entry in the
isVoiceNotCompiledError branch, including the existing session correlation
prefix used by neighboring branches, before invoking onError and returning. Keep
the localized error handling and no-retry behavior unchanged.
In `@app/src/features/human/voice/sttClient.test.ts`:
- Around line 100-105: Update both cloud and factory error tests in
app/src/features/human/voice/sttClient.test.ts (lines 100-105 and 169-174) to
assert the rejected error has the stable VOICE_NOT_COMPILED_CODE, or satisfies
isVoiceNotCompiledError, in addition to the existing message assertion. Use the
same typed compile-gate assertion for both RPC paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d9e7925-d044-42d9-9e42-fcdfbfd60b85
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/ci-lite.ymlAGENTS.mdapp/src-tauri/Cargo.tomlapp/src-tauri/src/lib.rsapp/src/features/human/MicComposer.test.tsxapp/src/features/human/MicComposer.tsxapp/src/features/human/voice/sttClient.test.tsapp/src/features/human/voice/sttClient.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsscripts/__tests__/feature-forwarding.test.mjsscripts/ci/check-feature-forwarding.mjsscripts/lib/feature-forwarding.mjssrc/openhuman/voice/compile_status.rssrc/openhuman/voice/mod.rs
# Conflicts: # app/src-tauri/Cargo.toml # app/src/features/human/voice/sttClient.ts
|
Maintainer review changes (merge-conflict resolution only — no logic changes to the PR's own diff): Merged current
No changes to the PR's substantive additions (the checker scripts, CI lane, AGENTS.md rule). |
|
Re CodeRabbit's 2 nitpicks on |
|
Maintainer sync: merged current main (includes #4949, the fix for the red-main typecheck failure tracked in #4948). The Frontend Checks failure on this PR was inherited from main and not caused by this PR's changes. Merge was clean — no conflicts. This PR's Rust feature-gate changes are untouched; the merge only brings in the repaired main. |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approving — both merge conditions are genuinely met.
CI: fully green, 18/18 lanes pass, zero pending, zero failing — including every lane that matters for this change:
Feature Forwarding Gate (shell forwards core defaults)— the PR's own new lane, passing against the merged tree.Rust Quality (fmt, clippy),Rust Feature-Gate Smoke (gates off),Rust Core Coverage,Rust Tauri Coverage— the Tauri lane actually compiles the shell with the new feature set.Frontend Checks— passes. This lane previously failed on this PR purely by inheriting the redmain(issue #4948, fixed by #4949); the re-sync onto the repaired main cleared it. The failure was never caused by this PR.PR CI Gate— the rollup, green.
Review state: no CHANGES_REQUESTED. CodeRabbit's review is COMMENTED; its two 🔵-trivial nitpicks were on sttClient.test.ts, which is not in this PR's net diff (carried transiently from the stacked, already-merged #4917) — correctly declined in-thread rather than silently dropped.
What was verified beyond CI:
- Two merge conflicts against
mainwere resolved to the union:app/src-tauri/Cargo.tomlnow forwardsmedia,tokenjuice-treesitter,voice,web3(main'sweb3bullet + this PR'stokenjuicebullet kept; main's now-false "un-forwarded" paragraph dropped).sttClient.tstook main's stricter type-guard refinement from #4917. - The resolution was checked with the PR's own gate:
node scripts/ci/check-feature-forwarding.mjs→OK: every default-ON core gate is forwarded (4/4).
Why this is worth having: it restores AST-aware compression to the shipped desktop build and adds durable CI enforcement against the exact drift class behind two prior incidents — a checker that is dependency-free, fails closed, and has its own passing lane. The fix and the guard against its recurrence land together.
Not merging — that is the maintainer's call.
|
@coderabbitai review Requesting a re-review of the current head. Your earlier review on this PR was dismissed, and the tree has since moved on: the branch was re-synced onto current |
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Summary
default-features = falsesilently dropped the default-ONtokenjuice-treesittergate, so TokenJuice fell back to the brace-depth heuristic instead of the AST-aware path (AST-aware code compression silently disabled in desktop builds (tokenjuice-treesitter dropped) #4918).Problem
app/src-tauri/Cargo.tomldeclaresopenhuman_corewithdefault-features = false(set in #1061, back when the core had nodefaultlist at all — a harmless no-op then). The shipped app therefore does not inherit the core's default gates; each must be forwarded by hand. Nothing enforced that.Two of three gates have been dropped, by two different authors:
voicemediatokenjuice-treesitterThe
mediaauthor knew about the trap and wrote it down, andvoicestill shipped broken. That is the argument for enforcement over documentation.#4918 is not a deliberate exclusion. #4575 ("fix tinyjuice build wiring") explicitly set out to "forward the correct TinyJuice tree-sitter feature name" and named broken
tokenjuice-treesitterresolution as its root cause — the intent was clearly that this works. The shell's separatetinyjuice = { default-features = false }is version alignment for the vendored 0.2.1 patch, not a veto.Reproduction (the probe that diagnosed #4901):
Solution
app/src-tauri/Cargo.toml— forwardtokenjuice-treesitter. Cost: 5 crates (tree-sitter+ Rust/TS/Python grammars, C build); the core crate already pays it.scripts/lib/feature-forwarding.mjs— narrow TOML reader + drift diff. Node has no TOML dependency here and only two well-known shapes are needed, so it is scanner-based in the same spirit as the siblingchecklist-parser.mjs.scripts/ci/check-feature-forwarding.mjs— CLI plusINTENTIONALLY_NOT_FORWARDED, empty by design. An intentional exclusion must carry a reason, so "deliberate" and "forgotten" stop looking identical — precisely the ambiguity that let AST-aware code compression silently disabled in desktop builds (tokenjuice-treesitter dropped) #4918 sit unnoticed..github/workflows/ci-lite.yml— new always-on lane wired intoPR CI Gate. Deliberately not filtered on changed paths: drift can be introduced from either manifest, and a skipped job counts as a pass in the gate. It is a few seconds of dependency-free Node.AGENTS.md— the rule, next to the gate docs.The checker refuses to pass vacuously: parsing zero core defaults exits 2 rather than reporting success. That caught a real bug in my own parser during development —
\smatches newlines, so^\s*\[features\]anchored several lines early and sliced the section to nothing. Without that guard it would have shipped as a green rubber stamp.Submission Checklist
pnpm test:scripts(which CI runs) passes 103 tests.N/A: build-configuration + CI tooling; no feature rows added/removed/renamed## RelatedN/A: no user-facing behaviour change; TokenJuice compression quality is internal.Closes #NNNin the## RelatedsectionImpact
Runtime/platform: desktop (macOS/Windows/Linux). Restores AST-aware code compression to the shipped app. No user-visible behaviour change — TokenJuice already worked, just worse.
Build cost: adds
tree-sitter,tree-sitter-language,tree-sitter-python,tree-sitter-rust,tree-sitter-typescriptto the shell's Cargo world (the core already builds them). C grammar compilation, so the shell's cold build grows modestly.Verification
cargo tree -i tree-sitterin shellcargo checkwith tree-sittervoice+tokenjuice-treesitterdroppedpnpm test:scripts(full suite CI runs)ci-lite.ymlneeds+ gateresultsmap all wiredCompatibility / migration: none. Build configuration only.
Related
Closes #4918
Closes #4919
tokenjuice-treesitteras a core default; never forwardedN/A — no coverage-matrix rows affected (build config + CI tooling)Summary by CodeRabbit
Bug Fixes
Localization
Reliability