[travsr-cli] Embed UX consistency and honesty + reindex --rebuild - #772
Conversation
F1: an interactive "Full" CPU choice now resolves to an explicit 100 percent, so it overrides ambient embed.capacity config instead of deferring to it. The parse is split into a pure parse_cpu_choice so it is testable without a TTY. F4: clamp the three embed status percentages to 100 via pct_display; the raw done/total counts stay honest. ETA: remove the projected ETA everywhere it appeared (embed status fmt_eta and the live reindex bar) and keep measured elapsed. Removes the now-unused eta helper, its warm-up constants, and the eta-only tests. Add embed reindex --rebuild, which invokes the sidecar --reembed full-rebuild mode through the existing banner, progress, and cancel plumbing.
…nd floor F6: write_model_descriptor now merges onto an existing model.toml, preserving keys embed init does not own (for example a hand-set macos_engine) instead of clobbering the whole file from a closed field set. Add the --reembed spawn argument to run_parallel_reindex (CLI-only; the daemon auto-spawns pass false), and raise EMBED_MIN_VERSION to v1.6.0 along with every catalog version_fallback, so a pre-v1.6.0 sidecar hits the actionable floor refusal instead of a raw unknown-argument error (RFC-025). ADR-019 records the re-embed contract and the release ordering. Regenerates plugin-hashes.lock for the travsr-plugin-host src change.
Benchmark comparison (master → PR)Threshold: p95 regression ≥ +10% fails this check. |
raj-rkv
left a comment
raj-rkv
left a comment
There was a problem hiding this comment.
Reviewed against a fresh worktree off origin/master (4579359 vs 0343e08, 0 commits behind). Built and ran the test suites locally.
The shape of this is good: the ADR is clear, the F1/F4/F6 fixes each come with a pure, TTY-free test, and dropping the projected ETA in favour of measured elapsed is the right call. Three things to resolve.
1. Merge gate (expected, per ADR-019). cargo test -p travsr-cli --bin travsr fails locally on declared_floor_never_exceeds_latest_release:
travsr-embed: declared floor 1.6.0 is above the latest release 1.5.0 - users cannot satisfy it
test result: FAILED. 251 passed; 1 failed; 1 ignored
That matches all three test jobs red in CI. ADR-019 orders the sidecar release first, so this is a sequencing gate: Travsr-com/travsr-embed#24 has to merge and v1.6.0 has to be published before this lands. Everything else is green (travsr-plugin-host --lib 169 passed, clippy pass).
2. --rebuild --phase1 N silently discards the phase 2 tier. Details inline. Reachable from the shipped CLI, no validation anywhere on the path.
3. The CPU-budget fallback messages now contradict the F1 fix. Details inline.
Also flagging for follow-up, not a change request here: once F6 lands, the macos_engine doc comment in travsr-embed src/model.rs ("Not durable against the CLI ... reverts to auto on the next embed init") becomes stale and should be updated in that repo.
Not flagged: em-dashes in the new comments. They are pervasive in the existing Rust sources (roughly 2900 across 180 files), so this PR is following the local convention.
…est fallbacks, ADR Add clap conflicts_with between --rebuild and --phase1 on embed reindex. The sidecar clears the whole model before re-embedding, so --rebuild --phase1 N deleted every vector and refilled only the phase 1 tier, silently dropping the phase 2 index from a command that reads as safe. clap now rejects the combination; a test covers it. This pairs with the sidecar-side guard in travsr-embed #24. Fix the two CPU-budget fallback messages in prompt_cpu_budget. After F1, parse_cpu_choice returns None to mean fall through to config/env/default, not Full. Both error arms printed using Full while returning None, so a user with embed.capacity = auto was told Full and got auto, the exact F1 bug on the error paths. Reword them to say the configured budget is used, and correct the stale falls through to Full comment on the test. Document the version-floor blast radius in ADR-019 Consequences. The v1.6.0 floor is a single host constant on resolve_backend, so it refuses all embedding for anyone at or below v1.5.0, including background reindexing, until they reinstall, even though --reembed is opt-in. Record this as an accepted trade-off for RFC-025 consistency, with the per-operation split noted as the escape hatch. Also extend the ADR crash-safety note to cover the HNSW index removal added in #24.
raj-rkv
left a comment
raj-rkv
left a comment
There was a problem hiding this comment.
Approving the code. Re-reviewed 8745239 merged onto current origin/master (9b9a02f); the PR was 12 commits behind, all of them travsr-vscode and travsr-daemon work, and the merge is clean with no interaction with the embed paths.
All three findings are resolved, and the third in the way I was hoping for rather than by capitulating to it.
--rebuild/--phase1: closed at the clap layer withconflicts_with, and the test asserts the actual parse outcome (ErrorKind::ArgumentConflict) rather than the attribute's presence. With travsr-embed#24's matching sidecar refusal, a data-destroying combination is now blocked at both layers.- CPU-budget fallback wording: both arms now say "using the configured budget", matching what
Noneactually does, and the stale test comment is fixed. - Floor blast radius: you kept the blunt floor for RFC-025 consistency and wrote the cost into Consequences, naming the narrower per-operation gate as the escape hatch. That is the right resolution for a deliberate trade-off.
Verified locally on the merged tree: 252 passed in travsr-cli --bin travsr (the new rebuild_and_phase1_conflict among them), 169 passed in travsr-plugin-host --lib.
One gate remains, and it is not in this diff. declared_floor_never_exceeds_latest_release is still red:
travsr-embed: declared floor 1.6.0 is above the latest release 1.5.0 - users cannot satisfy it
travsr-embed#24 is merged and its main carries version = "1.6.0", but no v1.6.0 tag or GitHub release exists yet, and that repo's Release workflow is tag-triggered. Pushing the v1.6.0 tag is the last step before this goes green and can merge. I have not created it, since publishing a release is your call.
2 participants
Implements the travsr-embed UX consistency and honesty fixes from EMBED_UX_AUDIT (F1, F2, F4, F6, F3, F8, plus ETA removal).
Track A: capacity and progress honesty (travsr-cli)
embed initnow resolves to an explicit 100 percent, so it overrides ambientembed.capacityconfig instead of silently deferring to it. The parse is split into a pureparse_cpu_choicefor testing without a TTY.embed statuspercentages are clamped to 100 viapct_display(a lagging tier denominator could print103%). Rawdone/totalcounts stay honest.embed statusand the live reindex bar); measured elapsed is kept (it is a fact, not a projection). Removes the now-unusedetahelper, its warm-up constants, and the eta-only tests.Track B: preserve model.toml keys on re-init (travsr-plugin-host)
write_model_descriptornow merges onto an existingmodel.toml, preserving keysembed initdoes not own (for example a hand-setmacos_engine) instead of rewriting the whole file from a closed field set.Track C: real re-embed path + honest remedy (cross-repo)
travsr embed reindex --rebuild, which invokes the sidecar's new--reembedfull-rebuild mode (paired PR: [travsr-embed] Add --reembed full-rebuild mode (v1.6.0) travsr-embed#24). CLI-only; the daemon auto-spawns passfalse.EMBED_MIN_VERSIONto v1.6.0 and every catalogversion_fallback, so a pre-v1.6.0 sidecar hits the actionable floor refusal instead of a raw unknown-argument error (RFC-025).Release ordering (please read before merging)
This PR raises the embed sidecar floor to v1.6.0. The RFC-025 honesty test
declared_floor_never_exceeds_latest_releasefetches the latest publishedtravsr-embedtag and will be red until travsr-embed v1.6.0 is released (Travsr-com/travsr-embed#24). That is expected and is the deliberate release ordering:(Offline CI runs skip that network test, so it is only red where CI has egress.)
Testing
cargo clippy -p travsr-cli -p travsr-plugin-host -- -D warnings: clean.cargo test -p travsr-plugin-host: all green.cargo test -p travsr-cli: all green except the one network honesty test noted above.embed statusshows 100 percent with no "remaining"/"eta";embed reindex --helpshows--rebuild;embed reindex --rebuildagainst an installed 1.5.0 sidecar refuses cleanly with the v1.6.0 floor message.parse_cpu_choice,pct_display, the progress-frame no-eta check, and thewrite_model_descriptormerge-preserve test.