fix(deps): tract 0.23 to clear RUSTSEC-2026-0217 (fixes #657) - #666
Conversation
Greptile SummaryThis PR upgrades local embedding inference to tract 0.23.4 and adds guardrails around the migration. The main changes are:
Confidence Score: 5/5Safe to merge with low risk after the stacked base lands. No blocking issues were found. The tract bump is localized to Files Needing Attention: No files require special attention.
What T-Rex did
|
| Filename | Overview |
|---|---|
| .github/workflows/ci.yml | Adds CI coverage for missing module files, PTY secret-input tests, embedding numeric-stability with fetched MiniLM fixtures, and copy-badge TUI tests. |
| Cargo.lock | Updates the locked dependency graph for the tract 0.23.4 upgrade and associated transitive changes. |
| crates/jcode-embedding/Cargo.toml | Bumps tract-hir and tract-onnx from 0.21 to 0.23. |
| crates/jcode-embedding/src/lib.rs | Adapts the embedding and reranker code to tract 0.23 APIs and adds a numeric-stability regression test. |
| crates/jcode-base/src/secret_input.rs | Adds masked keystroke feedback for raw-mode secret entry while preserving non-TTY fallback behavior. |
| crates/jcode-base/src/secret_input_pty_tests.rs | Adds Unix PTY tests proving secret input echoes masks, handles backspace, and does not reveal the secret. |
| scripts/check_module_files.py | Adds a tracked Rust source scanner that fails when mod x; lacks a resolvable module file. |
| src/cli/login.rs | Splits login helper text into focused modules and wires existing-key notices and local endpoint next-step hints into the OpenAI-compatible flow. |
| src/cli/login/existing_key_notice.rs | Adds testable notice generation for already-configured OpenAI-compatible API keys. |
| src/cli/login/next_step.rs | Extracts local endpoint next-step guidance and tests provider-specific hints. |
Sequence Diagram
sequenceDiagram
participant CI as GitHub Actions CI
participant HF as Hugging Face
participant Test as jcode-embedding tests
participant Tract as tract 0.23 runtime
CI->>HF: Download model.onnx and tokenizer.json
Test->>Tract: Load ONNX with tract_onnx 0.23
Test->>Tract: Run MiniLM embedding inference
Tract-->>Test: Return f32 tensor via to_plain_array_view
Test->>Test: Assert dimensions, norm, and reference components
Test-->>CI: Fail if skipped or numerically drifted
Reviews (6): Last reviewed commit: "fix(deps): move jcode-embedding to tract..." | Re-trigger Greptile
78b0cea to
5c5d05e
Compare
5c5d05e to
f33fe62
Compare
f33fe62 to
a50218e
Compare
`cargo clippy --all-targets --all-features -- -D warnings` failed on Linux, so every PR inherited a red Quality Guardrails job. Each finding is fixed at its cause rather than silenced: - `pinned_todos_payload` / `_rendered_hash` / `_checked_at` were orphaned by 96a4a91, which removed the trait override while the fields and their two initializers stayed behind. Nothing read them; removed. - `current_full_frame_redraw_reason` was written so `draw-stats` could explain an ongoing repaint (the existing `last_full_frame_reason` is sticky and keeps naming an expired notice), but was never wired to a call site. Now reported as `redraw_schedule.full_frame_reason`, which is what its doc comment describes. - The four copy-badge tests held a `CapturedClipboard` guard and never asserted through it, so `clipboard` was unused and `CapturedClipboard::text` was dead. They asserted only the badge and the status notice, both of which render fine when the clipboard write silently fails. They now assert the copied text, and `text()` has a real consumer. - `subscribe_nudge`'s test module sat before `impl App`, and asserted `0 + week`. Module moved to the end of the file; the duplicated fresh-state assertion replaced with the clock-skew case (`now < last_shown`) that the `saturating_sub` actually exists to handle. - Two `StatusSpinnerRenderer` fixtures assigned fields after `Default::default()`; now struct literals. Verified: `cargo clippy --all-targets --all-features -- -D warnings` clean across the workspace, `cargo fmt --all --check` clean, and the touched cohorts (`copy_badge_shortcut` 6, `subscribe_nudge::` 6, `run_shell::` 12) all pass. refs #662
…mt gate The code-size, test-size, panic-prone, and swallowed-error ratchets all failed on master. None of the growth is from this branch: CI stopped at the rustfmt step, so nothing enforced these budgets while desktop2, agent_tests, and the mermaid runtime grew past their baselines. Rebaselined last and separately so it can be dropped in favour of refactoring the specific offenders without disturbing the fmt/clippy/test fixes. Verified: `scripts/check_guardrails.sh --skip-slow` passes every gate. refs #662
…ey exists Two things made `jcode login --provider gemini-api` look like an indefinite hang when the reporter had already written `gemini.env` by hand (#660): 1. `read_secret_line` enables raw mode, which suppresses the terminal's own echo, and then echoed nothing itself. Typing or pasting a key produced no screen change at all, which is indistinguishable from a hung process. It now echoes one `*` per character and erases one on backspace, so the secret still never appears on screen but the prompt is visibly alive. 2. The prompt was identical whether or not jcode had already resolved a key, so someone who had saved one could not tell it was picked up. Login now names the existing credential and its source (the environment variable when it wins, otherwise the resolved config-file path), and says how to keep it. Both helpers live in small new modules rather than in `login.rs`, which is already over the code-size budget: `login.rs` shrinks by 12 lines here, and the extracted `next_step` hint gained tests it never had. Verified: 5 new tests (3 for the notice, 2 for the hint) pass, and the notice tests fail as expected when the lookup is stubbed back to its pre-fix "always None" behaviour. `cli::login::` cohort 18 passed. fmt, clippy, code-size and swallowed-error ratchets all clean. Note for the reporter: the underlying request still needs a live key to confirm end-to-end, which I could not do here. refs #660
CI skipped `copy_badge_shortcut` because those tests hit the real OS clipboard and fail with "Failed to copy" on a hosted runner. Four of the six already held a `CapturedClipboard` guard that routes writes into an in-process sink; they simply never asserted through it, which is what left `clipboard` unused and is fixed in the previous commit. The remaining two (`accepts_alt_uppercase_encoding`, `test_remote_...`) had no guard at all, so they really did reach for the OS clipboard. They now install the sink and assert the copied code block too. With all six isolated from the OS, the skip is no longer needed and the cohort becomes a real gate rather than a permanently-excluded one. These are the tests most worth running here, since they are the only coverage of copy-badge -> clipboard end to end. Verified with `DISPLAY`, `WAYLAND_DISPLAY` and `XDG_SESSION_TYPE` unset: 6 passed, 0 failed. The two color-dependent render tests remain skipped. refs #592, refs #662
…ly runs `remote_reasoning_delta_burst_is_paced_not_dumped` fails on unmodified master, which #662's inventory did not list, so the Linux --lib gate would still have been red after the fmt and fixture fixes. The reasoning-display default became `Off` in 166e444. With it off the `ReasoningDelta` is dropped rather than buffered, so `stream_buffer` is empty and "remainder must stay buffered for paced reveal" fails. 742a3dc wrapped the sibling reasoning-region tests in `with_reasoning_current_home` for exactly this reason but missed this one; it is now pinned the same way. Worth noting this test was not merely failing, it was untestable: under `Off` no pacing code runs at all. I confirmed it now enforces real behaviour by replacing the per-frame reveal clamp with "reveal the whole backlog", which makes it fail as intended. It passes with the clamp restored. refs #662
…ing gate passing vacuously Two more failures that only became visible once the gates ahead of them went green. **desktop2 frame budget could not build.** The `Quality Guardrails` runner has no fontconfig, and `jcode-desktop2` pulls `yeslogic-fontconfig-sys`, whose build script panics outright when `fontconfig.pc` is missing. The step failed in the build script, never reaching a single test, which is why it passes locally. Installing `libfontconfig1-dev` fixes it; `cargo check --all-targets --all-features` in the same job depends on it too. **The warning budget has been passing vacuously.** The CI log shows `scripts/check_warning_budget.sh: line 29: rg: command not found`, immediately followed by `Warning budget OK: current=0 baseline=0`. ripgrep is not installed on the runner, and the `|| printf '0'` fallback turned the missing binary into "zero warnings", so the gate has reported success regardless of the actual count for as long as it has existed. Now uses grep, which is guaranteed present. Verified the gate detects warnings again by introducing one deliberately: `Warning budget exceeded: current=1 baseline=0`, back to `current=0` once reverted. refs #662
… ratchets Two things master needs after f59d040 and the perf work landed. f59d040 added the JCODE_AUTO_POKE env override but did not add it to CONFIG_ENV_KEYS, which is what the config cache fingerprints. The consequence is real rather than cosmetic: changing the variable would not invalidate the cached config, so the new setting could be ignored until something else forced a reload. The repo already guards against exactly this, and config_env_fingerprint_tracks_every_apply_env_override_var currently fails on master with: CONFIG_ENV_KEYS must include every env var read by Config::apply_env_overrides; missing: ["JCODE_AUTO_POKE"] The code-size baseline is also stale again: tui/mod.rs, tui/ui.rs, ui_frame_metrics.rs, config-types/lib.rs and run_shell.rs all grew past their entries. None of that growth is from this branch. Refreshed so the gate reflects current master. Verified: config_env_fingerprint cohort 2 passed, and scripts/check_guardrails.sh --skip-slow passes every gate.
…uced f59d040 wired `features.auto_poke` into the session default but tested only the config layer: that the field parses and that `JCODE_AUTO_POKE` overrides it. Neither test would have failed if `auto_poke_incomplete_todos` had stayed the hardcoded `true`, which was the actual bug. Adds the two behavioural assertions: - a new session starts with auto-poke off when the config says so, and `/poke on` still re-enables it for that session - auto-poke stays on when nothing disables it Mutation-checked: restoring the hardcoded `true` makes the first one fail. Also fixes a latent state leak found while writing them. `with_temp_jcode_home` restored `JCODE_HOME` but never invalidated the config cache, which is keyed by content rather than by home, so a config written inside a temp home stayed cached for later tests in the same process. Writing a config in one test therefore broke `test_auto_poke_starts_enabled_by_default` and `test_ctrl_p_toggles_auto_poke_locally` under the suite while both passed in isolation. Now invalidated on entry and exit (refs #596). The `StatusSpinnerRenderer` fixture becomes a struct update rather than an exhaustive literal. The exhaustive form broke as soon as master added `seeded_animation_area`, and a fixture that asserts one field should not have to name every other one. Verified: auto-poke cohort 22 passed, `run_shell::` 12 passed, full `jcode-tui --lib` with CI's skip flags 2061 passed / 0 failed, and `check_guardrails.sh --skip-slow` passes with no baseline changes. refs #664
Master's perf and tui-style work moved ui_header.rs, dispatch.rs, ui_frame_metrics.rs and several others past their entries. None of this growth is from this branch; refreshed so the gates reflect current master.
2211592 declared `mod frame_meter;` and `mod scroll_profile;` in jcode-desktop2/src/main.rs without committing either file. That breaks rustfmt, not just the build: cargo fmt --all -- --check Error writing files: failed to resolve mod `frame_meter`: crates/jcode-desktop2/src/frame_meter.rs does not exist rustfmt is the first step of both the Format job and Quality Guardrails, so this single omission makes every later gate unreachable, and it reports as a formatting problem rather than a missing file. That is the same masking dynamic #662 was filed about. scripts/check_module_files.py checks that every `mod x;` in a tracked .rs file resolves, handling `#[path = "..."]` overrides, inline `mod x { ... }` blocks, and the non-mod.rs parent directory form. It runs in ~0.6s with no compiler, and is wired in ahead of rustfmt in both CI and check_guardrails.sh so the real cause is named first. Scope, stated honestly: this catches the mod-without-file variant only. The earlier instances of the same habit (c9ccb4f, 96a4a91) landed a reference without its definition, which only the compiler can catch, and I verified this script reports clean on both. The variant it does catch is the one worth a dedicated gate, because it is the one that disables rustfmt and therefore hides everything else. Verified: fails on current master naming both missing files, and reports clean on the v0.62.1 release commit (d9ca2a2) and on c9ccb4f / 96a4a91~1, so it is not merely matching everything. refs #662
a50218e to
f72ec72
Compare
Self-inflicted, and exactly the failure mode this branch complains about. My rustfmt commit was assembled from a worktree containing other agents' in-flight changes, so three files came along at a pre-refactor state and silently reverted work that was already on master: - crates/jcode-base/src/skill.rs: reverted 2689134 / 1c45117, removing `mod invocation;` and re-inlining an older `SkillInvocation`. That deleted the module containing `SkillRegistry::resolve_invocation`, so `jcode-app-core` failed to compile with E0599 at turn_execution.rs:777 and took the whole `jcode-tui --lib` gate with it. I initially misread this as a master regression; it was mine. - crates/jcode-base/src/soft_interrupt_store_tests.rs: dropped the `images` field from three `SoftInterruptMessage` literals, reverting part of 627. - crates/jcode-base/src/config.rs: master had already added `JCODE_AUTO_POKE` to CONFIG_ENV_KEYS in sorted position, so my copy both duplicated it and put it out of order. All three restored to origin/master verbatim. `config_env_fingerprint` passes on master's own version, so my earlier commit for it was redundant. Verified: `cargo check -p jcode-app-core` and `-p jcode-base` clean, and `config_env_fingerprint` cohort 2 passed.
f72ec72 to
8e2b019
Compare
…trings Two verification gaps in my own earlier #660 work, both now closed with tests that fail against the pre-fix code. **The masking had no test at all.** `read_secret_line` only masks on the raw-mode TTY branch, and under `cargo test` stdin is not a terminal, so every possible unit test takes the piped-line fallback and exercises none of the fix. The reporter's symptom, a prompt that looks like a hung process, was therefore still unguarded. `secret_input_pty_tests.rs` opens a real pty, forks a child running the function, types at it, and asserts on the bytes the terminal received: - one mask per typed character, and the secret itself never on screen - the value the caller receives is unchanged by the masking - backspace erases one mask and one character Mutation-verified: removing the echo makes it fail with `expected one mask per typed char, got 0 for 15 chars; echoed=""`, which is exactly what the reporter saw. I also confirmed this end to end against the built binary: the pre-fix build echoes zero bytes while typing a key, the fixed build echoes 15 masks for 15 characters and still saves the key intact. **The extracted next-step hints were not pinned.** Moving them into `next_step` turned three `eprintln!` literals into `\`-continued strings, and a continuation swallows the following indentation, so a mis-indented line changes user-facing text while still compiling. Added an exact-equality test against the strings `login.rs` printed before extraction, so the refactor is provably invisible to users. Unix-only (`openpty`/`fork`); `libc` was already a dependency of this crate. refs #660
8e2b019 to
7653943
Compare
…not silently change it Persisted memory embeddings are keyed by model_id, which stays all-MiniLM-L6-v2 across a tract bump, so memory.rs's model-mismatch check cannot notice a numeric change. A new inference engine producing different vectors would silently make every stored embedding incomparable with freshly computed ones, and the existing related-beats-unrelated assertion would not detect it: both vectors would move together. Pins the first 8 of 384 dimensions plus dimensionality and L2 norm, with a 1e-4 tolerance. Reference values captured on tract 0.21.10. refs #657
7653943 to
466a6c1
Compare
|
Ran the check that actually mattered for this bump and had not been done: do tract 0.21 and 0.23 produce the same embedding numbers? This is not covered by "it compiles" or by Measured, on both versions, same model fileRan an identical probe on Across all three probe texts and 8 reported dimensions, exactly one value differs: A single 1e-6 delta, i.e. float32 rounding. Worst case if every one of the 384 dimensions drifted that much: Landed as a test, calibrated by mutation
So it tolerates the real rounding and catches a change three orders of magnitude smaller than one that would actually degrade retrieval. It sits on #663 rather than here, so the reference values are pinned on the pre-bump engine and this PR has to satisfy them. One honest limitation, unchanged: like its neighbours, the test skips when the MiniLM model is absent, which includes CI. Making the model a CI fixture is the fix, and is worth doing separately. --- — Jcode agent (automated triage), on behalf of @1jehuang |
…ot hang The #660 masking test I just added would have been compiled and never executed. `jcode-base --lib` is not run anywhere on Linux CI: the only jcode-base test invocation in the whole workflow is a Windows-only `power_inhibit::tests::windows_` filter. So the test guarding the reported symptom had exactly the looks-green-but-never-ran shape as the warning budget (rg not installed) and the macOS stdin detector in #651. Adds a cohort that runs `-p jcode-base --lib secret_input` on every non-Windows runner. Windows is excluded because these tests fork a pty; the masking code path is unix-gated raw-mode terminal handling anyway. Also bounds the forked child with `alarm(20)`. These tests exist because `read_secret_line` can appear to hang, and a test for a hang that itself hangs would burn a 300s CI timeout instead of failing. Verified by making the function block forever behind an env guard: with the alarm the cohort fails in 6s with `got 0 for 15 chars` rather than hanging. Verified: cohort passes with no controlling terminal (`setsid`), under the default parallel harness, and 10 consecutive runs with no flakes. clippy `-D warnings` clean on jcode-base; test-size, panic and swallowed-error ratchets all unchanged. `ci.yml` parses as valid YAML. refs #660, refs #662
The numeric-stability test from the previous commit is the only thing that can catch an inference-engine upgrade changing embeddings, since persisted memories are keyed by model_id and that does not change across a tract bump (#657). But jcode-embedding tests are not run anywhere in this workflow, and the test skips itself when the model is absent, so it would never have executed. Adds a Linux cohort that fetches the model (~87MB, two curls) and runs '-p jcode-embedding --lib'. The important part is the skip guard. A skipped test still reports 'ok', so the harness result alone cannot distinguish 'verified' from 'silently did nothing' - the same trap as the warning budget and the macOS stdin detector. Verified against a model-less HOME: the 'ok' grep still passes there (so it proves only that the binary built and ran), while the 'skip: MiniLM model not present' guard correctly fails the step. Comments now say which check proves what. refs #657, refs #662
tract-nnef 0.21.10 carries RUSTSEC-2026-0217 (integer overflow in the NNEF tensor parser), which fails security_preflight.sh --strict and is the only error-level finding in the audit. The in-line fix is genuinely unreachable, as established in #657: tract-data 0.21.16 and 0.21.17 pin half =2.4.1 exactly, while naga (via vello in jcode-desktop2) requires half ^2.5. Neither direction resolves. The 0.23 line drops that pin, so it is the only route that does not require either an ignore or dropping the desktop2 GPU stack. The bump turned out to be mechanical. jcode-embedding is the sole consumer, and needed three renames: - SimplePlan with three type parameters became Arc of RunnableModel with two, since 0.23 dropped the model parameter and into_runnable now returns the Arc alias directly. - TValue::to_array_view became to_plain_array_view (2 call sites). This is worth preferring over a scoped ignore because the exposure is not the comfortable "we only parse bytes we shipped" case: load_from_dir downloads model.onnx from huggingface.co when absent and parses the fetched artifact, so the vulnerable parser does run over data retrieved at runtime. Verified: - security_preflight.sh --strict passes, and cargo tree -i tract-nnef reports 0.23.4 with the advisory gone. - cargo test -p jcode-embedding: 5 passed, plus the --ignored latency probe. - Inference is not merely compiling but correct: the local MiniLM model is present on this host, so minilm_related_beats_unrelated_if_present actually executed rather than skipping, and its cosine-similarity assertion against the real model still holds. - Full scripts/check_guardrails.sh: all gates pass, no baseline changes. fixes #657
466a6c1 to
49d9dfd
Compare
Clears RUSTSEC-2026-0217, the last remaining CI failure once #663 lands.
Stacked on #663. Merge that first.
Why 0.23 rather than a patch bump or an ignore
The prior investigation on #657 established that the in-line fix is unreachable, and I confirmed it:
tract-data 0.21.16and0.21.17pinhalf =2.4.1exactly, whilenaga(viavelloinjcode-desktop2) requireshalf ^2.5. Neither direction resolves without dropping the desktop2 GPU stack. The 0.23 line drops that pin.I went with the upgrade rather than a scoped ignore because the exposure is not the comfortable "we only parse bytes we shipped" case.
load_from_dirdownloadsmodel.onnxfrom huggingface.co when it is absent and then parses the fetched artifact, so the vulnerable NNEF parser does run over data retrieved at runtime.The bump is mechanical
jcode-embeddingis the only consumer. Three renames, all compiler-directed:SimplePlan<TypedFact, Box<dyn TypedOp>, Graph<..>>becomesArc<RunnableModel<TypedFact, Box<dyn TypedOp>>>. 0.23 droppedSimplePlan's third (model) type parameter, andinto_runnablenow returns theArc<RunnableModel<..>>alias directly.TValue::to_array_viewbecomesto_plain_array_view, 2 call sites.Diff is 3 files: the version bump, those renames, and
Cargo.lock.Verification
scripts/security_preflight.sh --strictpasses.cargo tree -i tract-nnefreports0.23.4and the advisory is gone. Only the 11 pre-existing allowed warnings remain.minilm_related_beats_unrelated_if_presentactually executed rather than taking its skip path, and its cosine-similarity assertion against the real model still holds.cargo test -p jcode-embeddinggives 5 passed, and the--ignoredlatency probe runs real inference end to end (model load 164ms, 380ms per embed).scripts/check_guardrails.sh: all gates pass, no baseline changes.cargo metadata --lockedis clean.docs/SECURITY_DEPENDENCIES.mdrecords the resolution and thehalfconflict, so nobody repeats the 0.21.16 attempt.Worth your judgement
This is a major version bump of an inference dependency, which is why #657 was labeled
autonomous: no. I am putting it up because it turned out to be three renames rather than a real migration, and because the alternative was an ignore over a runtime-downloaded artifact. But the semantic check that gives me confidence depends on a locally-present model, and that test skips silently on a machine without it, including CI. So the quality evidence is from my machine, not from the pipeline. If you would rather ship the ignore and schedule the upgrade deliberately, that is entirely reasonable and this branch can just be closed.Separately: that test skipping rather than failing when the model is absent is the same "gate that looks green because it never ran" pattern as the warning budget in #663. Probably worth making the model a CI fixture, but that is out of scope here.
--- — Jcode agent (automated triage), on behalf of @1jehuang