feat(cli): search the catalog by meaning, on this machine - #3089
Conversation
29a2470 to
0eed6a8
Compare
14d4326 to
8c434a2
Compare
391d30a to
1edf729
Compare
09bdbfc to
45c4873
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
What it does
Adds a third catalog-search tier: hyperframes catalog --query "..." --on-device ranks the whole catalog by embedding cosine against a shipped 384-dimension vector set produced by bge-small-en-v1.5, run through onnxruntime-node with a hand-rolled WordPiece tokenizer. The default tier is still shared-vocabulary word matching; the hosted-endpoint tier is dropped. Both the tier and every reason it could not run are named in the --json envelope. Ships the vector index + the build/coverage scripts, and enforces artifact-vs-registry coverage in CI and pre-commit.
What I checked
Read every changed file at head 0ce31a2 (31 files, +7540 / -27). Focus lenses: consent state machine across runs and terminals, model-download integrity, first-run concurrency, vector-artifact freshness, tier fallback semantics, JSON envelope backward compat, downstream consumers, CI coverage gate, install-size impact, and the new hand-rolled tokenizer's parity harness.
Findings
P2 — --on-device silently overrides a prior explicit decline
File: packages/cli/src/commands/catalog.ts:63-89
If a user previously answered no to the interactive prompt (consent === false, status "declined"), a later hyperframes catalog --query ... --on-device on a TTY takes this path:
- Line 63:
!opts.assumedYes && status === "not-asked"→ false, prompt skipped. - Line 79:
localModelConsent() !== false→ false, runtime guard skipped. - Line 89:
recordLocalModelConsent(true)fires unconditionally. - Line 90:
ensureLocalModel()downloads 33 MB.
No prompt, no stderr line, no confirmation. The PR's headline promise "Declining means declining" holds within a single run (the returned false branch is correct and tested), but not across runs when --on-device is later present. Concrete failure: a user declines interactively; an agent or shared shell alias later invokes with --on-device; 33 MB downloads silently, contradicting the user's earlier stated preference.
Fix options: (a) re-prompt when a prior decline exists; (b) log on-device: overriding previous decline before recording; (c) require --yes in addition to --on-device to override an existing false. At minimum, document in the --on-device flag description that it supersedes a prior decline.
There is no test for this two-run sequence — the existing decline test only asserts single-run behavior.
P2 — First-run concurrency can permanently corrupt the cached model
File: packages/cli/src/utils/download.ts:52-115 used from packages/cli/src/registry/localModel.ts:120-132
Two parallel hyperframes catalog --query ... --on-device invocations both call downloadFile(url, localModelPath()). Both open ${dest}.tmp for write; process B's createWriteStream(tmp) truncates while A is still streaming; A's renameSync(tmp, dest) moves an interleaved file to dest; B's later renameSync fails with ENOENT.
Result: dest holds a byte-corrupt ONNX file. On the next run, isLocalModelReady() returns true (both files exist), ort.InferenceSession.create() fails at load, applySearch's catch reports "on-device search did not run", falls back to words. This loops forever — the user must rm ~/.hyperframes/models/*.onnx by hand to recover.
The fetchLocalVectors sibling has a similar shape but the pair-agreement check refuses truncated fetches before writing, so it is safer.
Fix: unique .tmp suffix per PID/UUID, or a file lock on ~/.hyperframes/models/.lock, or verify+integrity after rename.
P2 — No integrity check on the model download
File: packages/cli/src/registry/localModel.ts:56-65
MODEL_REVISION = "ea104dacec..." is pinned in the URL path, which is good, but the fetched bytes are trusted on HTTPS + HF's CDN alone. There is no expected_sha256 compared after the write.
downloadFile follows up to 10 redirects and streams whatever the CDN returns into place. Failure modes I can think of: HF-side origin corruption; a CDN-cached partial-200 (rare but possible on some CDNs); a domain change on Xenova's HuggingFace org that silently reserves the path. On success, dimensions === 384 check at inference time gates gross wrong-model bytes but not subtle corruption (a mis-quantized model with the same tensor shape produces plausible embeddings that rank worse — the exact failure mode this feature keeps citing as the reason for other invariants).
Fix: add expectedSha256 to MODEL_FILES entries, verify after write and before rename, unlinkSync on mismatch. Standard for local-ML packages (e.g. transformers.js pins its hashes).
P2 — --on-device in a non-TTY pipeline triggers a silent 33 MB download
File: packages/cli/src/commands/catalog.ts:199
assumedYes: args.yes === true || !process.stdout.isTTY || json sets assumedYes true whenever stdout is redirected. This is intentional for agents/CI (nonInteractiveConsentMessage documents the contract), but it also fires for a human running hyperframes catalog --query X --on-device | tee log.txt or > results.json — no prompt, no confirmation, 33 MB downloads.
Fix: when !isTTY but stdin.isTTY (i.e. the user is watching, they just piped stdout), still ask; or unconditionally print a one-line Downloading 33 MB on-device model... to stderr before the fetch begins so the human notices.
P2 — fetchLocalVectors uses raw fetch() with no timeout
File: packages/cli/src/registry/localSemantic.ts:79
const response = await fetch(`${base}/catalog-artifact/${file}`);No AbortSignal.timeout(...). If the registry hangs mid-response, the CLI waits forever with no output. The model download at least has a 30s idle timeout via downloadFile.
Fix: fetch(url, { signal: AbortSignal.timeout(30_000) }) or equivalent.
P3 — Offer message is misleading when the offer fires from --on-device on first run
File: packages/cli/src/registry/localModel.ts:149-155, used at catalog.ts:65
downloadOfferMessage(0) returns "No matches from word search. Download a 33 MB search model..." — but when prepareOnDeviceTier invokes it (line 65), word search has not run yet. The user asked for on-device directly; being told "no matches from word search" is inaccurate. Split the copy: one message for the "word search came up thin" offer at offerLocalModel (line 552), a different one for the --on-device first-run consent at line 65.
P3 — onnxruntime-node is a hard prod dep for a feature only some users invoke
File: packages/cli/package.json:43
onnxruntime-node@1.21.1 pulls native binaries (~200 MB installed footprint depending on platform) into every install. Users who never pass --on-device still pay. Also risks a platform mismatch (arm64 Alpine, exotic distros) making npm i -g hyperframes fail hard.
The rationale to include it (line 32-46 in localEmbedder.ts) is defensible — a lazy require would still fail without it on disk. But optionalDependencies with a graceful "install onnxruntime-node to use --on-device" would trade a smaller install for a slightly rougher first-use path. Worth considering as a follow-up; not blocking.
CodeQL notes
The three CodeQL findings on localSemantic.ts are worth acknowledging:
- Insecure temp file — flags
tmpdir()usage in the test only; the production path uses~/.hyperframes/catalog/with 0o700/0o600. Acceptable. - File data in outbound request / Network data written to file — the URL is composed from
config.registry(user's own configured registry base) and lands in the user's own cache. Given the pair-agreement + dimension guards, and 0o600 file mode, this is the correct shape for fetching a first-party artifact. Acceptable false-positive.
Positives
- The
returnafterrecordLocalModelConsent(false)— the code AND the comment above it earn their keep. The single-run decline path is correct and covered by two tests. vectorPairAgreesbefore writing is the right shape: refuse truncated / wrong-model responses before they land, so a broken cache never poisons subsequent runs.- Dimension mismatch throws with the actual numbers, at both
loadLocalVectorsandloadLocalEmbedder, so a shape drift is loud rather than silent. MODEL_REVISIONpinned to a specific commit SHA on HF, with a comment naming why (silent weight change would mis-rank against cached vectors).- Wordpiece implementation avoids the standalone-tokenizer pkg (359 MB installed) with a ~200-line hand roll, backed by a real parity corpus (
wordpiece-reference.json, 4419 rows) rather than a handful of by-eye assertions. The\p{S}symbol-exclusion comment naming a specific past bug is the kind of comment I want to see near ML preprocessing. - CLS pooling vs mean pooling and the query-instruction prefix — both bge-specific — are asserted in tests. Getting either wrong degrades quietly, which is what the parity discipline exists to catch.
- Tier is a stable machine token (
"on-device"|"words") separated from the human-readabletierDetail. Downstream JSON consumers get the invariant surface they need. - JSON envelope backward-compat: no-query still returns the pre-existing array shape.
check-artifact-coverage.tsruns in CI (per-changed-paths filter) AND thecatalog-indexlefthook pre-commit re-embeds + re-stages on registry-item edits — with exit code 3 distinguishing "contributor lacks the model" from "build failed". That is the right shape for keeping the index honest without gating contributors on a 33 MB opt-in.- The score is exposed on the
on-devicetier and deliberately omitted on thewordstier — the two are not comparable and the code documents why. unindexedanddroppedmeasured against unfilteredregistryNames, not the post-filter set, with two tests specifically pinning that under--type.README.mdinregistry/catalog-artifact/documents the "reproduce by rebuilding" as the only real provenance check, and calls out the batch-size-changes-the-quantization gotcha. This is the kind of hazard note that survives a maintainer handoff.
Verdict
Approve with nits. None of the P2s are shippers — they are ergonomic robustness gaps rather than data-loss / privacy defects — but the consent-override and download-integrity items are worth addressing before this settles as the shipping contract, since the design goes out of its way to promise both.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 0ce31a237.
The overall shape is right and the docblocks are the best kind: the ones that name the failure mode they defend against. Three tiers cleanly separated (catalog.ts:192-204, 459-497), decision tree matches the mermaid the PR ships (catalog.ts:196-203 for the on-device gate, :459-488 for the fallback), tier NAMED in output and JSON envelope (:265-268 tier and tier_detail), sticky decline persisted (localModel.ts:69-75, catalog.ts:63-69), non-interactive path documented as flag-is-consent (localModel.ts:141-146). WordPiece implementation is fixture-driven at scale (wordpiece.test.ts:135-146 diffs 4,419 reference cases and fails on any mismatch), embedder normalizes for cosine (localEmbedder.ts:97, 113-116, cosine at :118-122 is pure dot product), determinism preserved in the build (build-local-vectors.ts:91 sorts names lex; BATCH = 16 invariant named at :39-44 because padding-within-a-batch changes quantized output). Vector byte-length validated before use (localSemantic.ts:53-64) — the class of "artifact truncated / dimensions drifted" is closed.
Three concerns worth naming before landing, plus a batch of smaller ones.
Concerns
Word tier drops the tags field the mermaid promises. catalog.ts:496 — searchByWords(items, query, (item) => \${item.name} ${item.title} ${item.description}`). The flowchart the PR ships names four searchable fields ("name, title, description, tags") and the tier-name string at catalog.ts:515reads as "local word match" over all of them, but tags never enter the string the ranker scores. Concrete failure: an item whose only relevance to--query transitionis a"tags": ["transition"]entry (no "transition" in name/title/description) scores 0 and drops out. This is the tier that fires on decline, on failure, and by default when--on-deviceis absent — the fallback that most users hit — so the field mismatch shows up on common queries, not exotic ones. Cheap fix: template in${(item.tags ?? []).join(" ")}alongside the other three, or a smallsearchableText(item)` helper so the shape is documented in one place.
No integrity check on the downloaded model. localModel.ts:120-132 — ensureLocalModel gates on existsSync(file.dest()) and isLocalModelReady(), both of which are pure filesystem-existence checks. No SHA is computed against model_quantized.onnx or tokenizer.json after download and no known-good hash lives anywhere in the code. Revision-pinning at MODEL_REVISION = "ea104dacec62c0de699686887e3f920caeb4f3e3" defends against HuggingFace moving main, but not against MITM against a client with a mis-configured TLS trust store (corporate intercept CAs are the common case) or against the origin serving different bytes for the pinned revision. The failure mode is exactly the one Miga's own docblock at localModel.ts:37-46 names: "a model that silently changes under a cached vector set would return confident nonsense, because the catalog vectors were produced by a specific set of weights." The catalog artifact itself carries payload_sha256 (scripts/catalog/catalog-artifact.ts:218) — extending that pattern to the two model files (known SHA-256 pinned in code, verified post-download, delete + refuse on mismatch) closes the class.
No max-bytes cap on downloadFile. packages/cli/src/utils/download.ts:90 (introduced in #3148 base) pipes res into the destination file with only the per-request stall timeout (30 s) as a guard. A misbehaving or hostile origin streaming at just above the stall threshold can fill disk indefinitely — the 33 MB the docblock quotes is documentary, never enforced against the response body. Adding a max-bytes counter on the pipeline (fail + removePartialFile when exceeded, with the max derived from Content-Length or a per-caller cap like 1.5 × LOCAL_MODEL_SIZE_MB * 1024 * 1024) is a small change with a wide safety win. This one arguably belongs on #3148 rather than #3089, but the on-device feature is what makes the exposure realistic — before this feature the download path was for one-off large models (background removal) that maintainers explicitly chose to run.
Smaller concerns
Persisted decline is silently overwritten by any later --on-device. catalog.ts:63, 79, 89 — localModelStatus() === "declined" skips the prompt at :63, the runtime check at :79 short-circuits (localModelConsent() !== false is false), then :89 calls recordLocalModelConsent(true) unconditionally. Passing --on-device after a decline flips the persisted "no" to "yes" without re-asking. The flowchart's "declining stops here" doesn't obviously imply this override — arguably intentional as a "change your mind" gesture, but nothing tells the user. Cheap fix: on declined status with --on-device, print one line ("Enabling on-device search — previously declined") and proceed, so the state transition is visible.
Non-TTY auto-consent triggers more broadly than the flag alone. catalog.ts:199 — assumedYes: args.yes === true || !process.stdout.isTTY || json. Piping stdout to tee, less, or jq in an interactive shell flips isTTY false and auto-consents to a 33 MB download without a prompt. The design comment at localModel.ts:141-146 names "the flag is the human's yes" — but the non-TTY branch is a second consent path, and the user piping through tee didn't pass a flag. Consider gating auto-consent on args.yes === true || json only, and letting a non-TTY session without --yes fail with "cannot prompt in this context, pass --yes or --on-device to consent."
On-device tier is silently missing on any registry item added after the last artifact rebuild. localSemantic.ts:169-175 ranks only items in set.names; catalog.ts:462-478 maps ranked names to filter-hits via pickByName; items with no vector are absent entirely. The summary unindexed count is surfaced at catalog.ts:290-305 and gated in CI by check-artifact-coverage.ts, so the class is caught at merge time — but a local git add between rebuilds hides the new item from every on-device query, silently. The word tier picks up the new item so results aren't empty, but a user searching for the newly-added block by meaning gets nothing. Naming this in the CLI output (per-item, not just aggregate) would let the user know their query missed a new item.
Runtime semantic errors bypass the JSON warnings envelope. catalog.ts:484-487 — the on-device fallback catch logs to stderr but doesn't append to warnings[]. Consumers of --json see tier: "words" with no explanation for why the on-device tier fell through at inference time. prepareOnDeviceTier's pre-run warnings do flow to warnings (as the docblock at :37-45 claims); the runtime-error path is the asymmetric one. Push error.message into a warnings.push(...) before falling through.
Nits
-
--jsonenvelope shape depends on query presence (catalog.ts:216-228, 260-277vs a bare array with no query at:254-259). Documented via back-compat but worth naming in the JSON docs so an agent doesn't parse the wrong shape. -
On-device tier caps at 25 hardcoded, word tier uncapped.
catalog.ts:472ranked.slice(0, 25). No--limitflag. Cap the word tier and add a flag, or document the asymmetry. -
Config schema has no type guard for
localEmbeddingEnabled.telemetry/config.ts:659passes it through as-is. A hand-edited"maybe"reads as consent becausecatalog.ts:79checks!== false. Surrounding fields use=== trueguards — align. -
ONNX
InferenceSessionnever released.localEmbedder.ts:52. Fine for one-shot CLI; if a future long-lived caller (test harness, agent) reusesloadLocalEmbedder, native handles leak. -
No
localEmbedder.test.ts. WordPiece parity is directly tested (wordpiece.test.ts:135-146), but the embedder pipeline (padding invariant, mask correctness, CLS pooling, truncation to 512 tokens) has only integration coverage via the parity test. A direct test for the pad-width-changes-output invariant would pin the docblock claim atbuild-local-vectors.ts:39-44. -
CI
catalog-index-coveragefilter is name-only..github/workflows/ci.yml:53-56triggers onregistry/registry.jsonandregistry/catalog-artifact/**andscripts/catalog/check-artifact-coverage.ts. Edits to a searchable field inside an existingregistry/blocks/foo/registry-item.jsonnever touchregistry.json(unless a maintainer regenerates the manifest), so the vector goes stale and no gate catches it. Content-hash-per-item would close it. -
catalog-indexlefthook exit-3 semantics are correct.lefthook.yml:31—bun scripts/catalog/build-local-vectors.ts || { [ $? -eq 3 ] && exit 0; exit 1; }. Bash$?inside the||RHS reflects the LHS exit; exit 3 → true →exit 0; anything else non-zero →exit 1. Naming here so the next contributor doesn't second-guess.
What lands cleanly
- Three-tier structure with the mermaid faithfully implemented at each branch, tier surfaced in both terminal (
catalog.ts:289) and JSON output (:216-228, 260-277). - WordPiece implementation with 4,419-case parity fixture, driven end-to-end in one test (
wordpiece.test.ts:135-146). - Vector byte-length validation before use (
localSemantic.ts:53-64) — protects against the "artifact drifted, dimensions changed" class. - Deterministic build via lex-sorted names + fixed batch size (
build-local-vectors.ts:91, 39-44). - Coverage gate script with symmetric detection (
check-artifact-coverage.ts:45-46) — both missing-vector and stale-vector directions are detected. - Consent-per-invocation flag (
--on-device) that doubles as the human's yes in non-interactive contexts — matches the documented framing. - Docblocks that name the trap they close (
localModel.ts:37-46,build-local-vectors.ts:39-44,check-artifact-coverage.ts:1-16) rather than describing what the code does.
The three named concerns above are the ones worth deciding on before landing; the smaller items are follow-up-track.
1edf729 to
92d4e3a
Compare
0ce31a2 to
4818dd0
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 4818dd07 (delta from 0ce31a237).
All three R1 concerns closed, plus every smaller item I named. The delta is broad and each fix reads correctly at its site.
Word tier now covers tags — catalog.ts:507-511:
const words = searchByWords(
query,
items,
(item) => `${item.name} ${item.title} ${item.description} ${(item.tags ?? []).join(" ")}`,
);Concrete failure I named (item with only tag "transition" misses --query transition) is fixed. Nil-tags handled via ?? []. The docblock above at :504-506 names why word matching is by shared vocabulary, not substring — good provenance.
Model integrity check is real — localModel.ts:69, :75 pin known-good SHA-256s per file (6c9c6101… for the model, d241a60d… for the tokenizer), :101 verifies via createHash("sha256").update(readFileSync(path)).digest("hex") === file.sha256, and isLocalModelReady() at :107-109 requires BOTH artifacts pass. ensureLocalModel() at :142-160 calls modelFileIsValid() post-download and unlinkSync(dest) + returns false on mismatch — corrupted download self-cleans and the "on-device search skipped" path fires downstream. Closes the exact class the docblock at :37-46 warned about ("a model that silently changes under a cached vector set would return confident nonsense").
Byte cap enforced end-to-end — localModel.ts:149 passes { maxBytes: file.bytes } into downloadFile; the Transform at download.ts:43-55 (from #3148) fails the pipeline on received > maxBytes. Per-file caps at :72, :78 are documented as "exact size caps" — meaning any origin drift past the pinned bytes fails closed rather than just at the SHA check. Slow-drip disk-fill class closed.
Smaller items from R1, also fixed:
- Sticky decline is now visible.
catalog.ts:60-64—if (!opts.assumedYes && status.status === "declined")prints"on-device search skipped: the model download was previously declined. Re-run with --yes to consent."and returns. The state transition is surfaced inwarningsand stderr rather than silently overwritten. Design intent is now readable:--on-devicealone respects a prior decline,--on-device --yesexplicitly reconsents. - Non-TTY auto-consent decoupled.
catalog.ts:210-211:Piping toassumedYes: args.yes === true, canPrompt: process.stdout.isTTY === true && !json,
tee/less/jqnow falls into the!opts.canPromptwarn-and-return branch at:67-70, not into a silent consent. My concrete failure (interactive shell with| teetriggering a 33 MB download unprompted) is closed. - Runtime error → JSON warnings.
catalog.ts:494-499— the on-device fallbackcatchnowwarnings.push(warning)alongsideconsole.error(warning), so--jsonconsumers seetier: "words"with the reason attached. Symmetric with the pre-run warning path Miga's docblock names. - Vector-fetch timeout.
localSemantic.ts:82—signal: AbortSignal.timeout(CATALOG_ARTIFACT_TIMEOUT_MS)on the fetch.AbortSignal.timeoutfires once and can't be reused across retries, but the fetch here isn't retried, so single-shot is correct for this callsite. - Pre-search prompt now names the tier gap.
localModel.ts:180-188—downloadOfferMessage(matchCount)renders"No matches from word search."/"Only N matches from word search."/"Use on-device meaning search."based on how many items the word tier already returned. The prompt communicates the actual "why offer this" rather than a generic ask.
Still open (nits, not blockers)
- On-device cap at 25 hardcoded, word tier uncapped, no
--limitflag.catalog.ts:485ranked.slice(0, 25). Same as R1; deferring as a follow-up is fine, but naming it here so it doesn't drop off. ONNX InferenceSessionnever released.localEmbedder.ts:52. Fine for one-shot CLI; long-lived callers would leak native handles.- No
localEmbedder.test.ts. Padding invariant, mask correctness, CLS pooling, truncation at 512 — covered only indirectly through the wordpiece fixture. Follow-up. catalog-index-coverageCI filter is name-only..github/workflows/ci.yml:53-56— edits to searchable fields inside an existingregistry-item.jsondon't touchregistry/registry.jsonand so don't fire the gate; vectors go stale silently. Content-hash-per-item would close it. Follow-up.- On-device tier silently misses registry items added between artifact rebuilds —
localSemantic.ts:169-175+catalog.ts:462-478. Per-item mention rather than only aggregateunindexedwould let the user know their query missed a new item. Follow-up.
The named blockers and smaller concerns from R1 are all delivered. The follow-ups are polish and defensibility for future contributors; nothing on the critical path.
Clean; ready from where I sit — stamp routing per standing rule.
vanceingalls
left a comment
There was a problem hiding this comment.
Delta verified
R2 max-effort delta-verify at head 4818dd07. Read the exact code at every R1-cited file, traced consent flow end-to-end, checked the new tests. All P2s FIXED; P3-1 FIXED; P3-2 UNCHANGED by author intent.
P2-1 consent-override across runs — FIXED. packages/cli/src/commands/catalog.ts:59-65 now hard-gates declined behind explicit --yes:
const status = localModelStatus();
if (!opts.assumedYes && status.status === "declined") {
warn(
"on-device search skipped: the model download was previously declined. Re-run with --yes to consent.",
);
return warnings;
}Two-run scenario walked end-to-end: Run 1 --on-device on TTY, user declines → recordLocalModelConsent(false) at line 78, status persists as declined. Run 2 --on-device (no --yes) → assumedYes=false, status matches declined, early return with an actionable message. The R1 defect ("silently overrode without re-prompt") is closed. The recordLocalModelConsent(true) call at line 98-100 is now guarded by the early return above, so it can only fire when assumedYes=true (documented re-consent) or when status flipped through the prompt at line 72-86. Explicitly asserted by the new test at catalog.test.ts:352-366 ("keeps a decline sticky until explicit --yes consent") — declines twice with no --yes, downloads exactly zero times, then re-runs with --yes and downloads once.
P2-2 concurrent tmp-file race — FIXED. packages/cli/src/utils/download.ts:74:
const tmp = `${dest}.${process.pid}.${randomUUID()}.tmp`;Per-download unique tmp path composed from PID + UUID → no two concurrent downloads can byte-interleave into the same file. Rename to final dest is still last-write-wins, but both writers land the same bytes (subsequently SHA-verified in localModel.ts), so the corruption path R1 flagged is closed. download.test.ts:53-64 covers concurrent downloads to the same dest and asserts the result is one of the sources whole (not a byte-mix).
P2-3 no SHA256 on model — FIXED, exceeds ask. packages/cli/src/registry/localModel.ts:64-77 pins both artifacts:
export const LOCAL_MODEL_ARTIFACTS: ReadonlyArray<ModelFile> = [
{ url: ..., dest: () => localModelPath(), bytes: 34_014_426, sha256: "6c9c6101a956d62dfb5e7190c538226c0c5bb9cb27b651234b6df063ee7dbfe4" },
{ url: ..., dest: () => localTokenizerPath(), bytes: 711_396, sha256: "d241a60d5e8f04cc1b2b3e9ef7a4921b27bf526d9f6050ab90f9267a1f9e5c66" },
];Exact numeric byte caps (not "reasonable" limits), pinned hex SHA256 per artifact. modelFileIsValid (line 97-105) computes SHA on disk and returns false on mismatch; ensureLocalModel (line 150-153) unlinkSyncs and returns false on mismatch after download, so a corrupt/tampered file is removed and the next call refetches. Model revision is also pinned via MODEL_REVISION (line 47). Tests at localModel.test.ts:113-144 cover both mismatch-clears and both-digests-must-match. Miguel pinned the tokenizer as well as the model — that was implicit in my ask but explicit here.
P2-4 non-TTY silent download — FIXED. assumedYes is now args.yes === true at catalog.ts:210 — no longer inferred from !process.stdout.isTTY. canPrompt (line 211) is only consulted to decide whether to prompt or emit the non-interactive message; it never grants consent. Non-TTY without --yes hits catalog.ts:67-70 and warns via nonInteractiveConsentMessage() with no download. The mixed case (stdout piped, stdin TTY) is treated the same as fully non-TTY — safe conservative default, and consistent with canPrompt gating on process.stdout.isTTY && !json. catalog.test.ts:368-375 asserts downloads === 0 && consentRecorded === [] on non-interactive.
P2-5 vector-fetch timeout — FIXED. packages/cli/src/registry/localSemantic.ts:28,81-83:
const CATALOG_ARTIFACT_TIMEOUT_MS = 30_000;
...
const response = await fetch(`${base}/catalog-artifact/${file}`, {
signal: AbortSignal.timeout(CATALOG_ARTIFACT_TIMEOUT_MS),
});AbortSignal.timeout covers the whole fetch (headers + body — Node 22 supports this natively, per package.json engines). Timeout errors are caught by the outer try/catch at line 98-100 which returns false, so the offline tier degrades cleanly rather than hanging. Test at localSemantic.test.ts:32-34 asserts signal: expect.any(AbortSignal) is passed to fetch.
P3-1 misleading pre-search prompt — FIXED. localModel.ts:176-184:
export function downloadOfferMessage(matchCount?: number): string {
const found =
matchCount === undefined
? "Use on-device meaning search."
: matchCount === 0
? "No matches from word search."
: `Only ${matchCount} match${matchCount === 1 ? "" : "es"} from word search.`;
...
}Overloaded on argument presence: pre-search call site (catalog.ts:74, downloadOfferMessage()) gets the neutral phrasing; post-search offer (catalog.ts:253 / catalog.ts:567) passes matchCount and gets the "No/Only N matches" phrasing which is now accurate for the code path.
P3-2 onnxruntime-node hard prod dep — UNCHANGED (author intent). packages/cli/package.json:43 still lists "onnxruntime-node": "1.21.1" under dependencies. Not in Miguel's fix list; no code change here. Noted as UNCHANGED per the R1 framing (author intent — every install pays the native binary weight even without on-device search).
Extra claims verified
- Tag-aware word fallback.
catalog.ts:510composes searchable text as`${item.name} ${item.title} ${item.description} ${(item.tags ?? []).join(" ")}`, andcatalog.test.ts:275-284asserts a search for"transition"finds a block tagged["transition"]on the word tier. Sane. - JSON runtime warnings.
applySearchreturnswarnings: string[](catalog.ts:498-501), which propagate into the JSON envelope at line 236 / 284 (...(warnings.length ? { warnings } : {})). Test atcatalog.test.ts:286-293asserts an on-device runtime failure surfaces aswarnings: ["on-device search did not run: model could not load"]on the JSON path. Sane.
New findings
None at P1/P2/P3. Adversarial checks passed:
- SHA verification +
unlinkSyncon mismatch is self-healing (next call refetches). fetchLocalVectorswrites both files only ifvectorPairAgreespasses → cannot land a truncated pair.AbortSignal.timeoutis Node 17.3+;package.jsonengines is>=22— safe.- Two concurrent
ensureLocalModelcalls converge on identical bytes via SHA-verified downloads and unique tmp paths. enforceByteLimituses strict>against the exact size, so exactly-sized files pass.
Positives
Test coverage is unusually deep for this size of change: catalog.test.ts (376 lines, new), localModel.test.ts (129 lines, new), localSemantic.test.ts (70 lines, new), download.test.ts (+66 lines) — each R1 fix has a targeted assertion. Prose comments consistently explain the "why not the alternative" rather than restating the code, which made the delta-verify tractable in one pass. Tokenizer SHA-pinning + model revision pin + separate size cap per artifact is more defense-in-depth than I asked for.
APPROVE.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 2e6bfb3d6 (delta from 4818dd072).
Right shape and end-to-end coverage. The two follow-up nits I named on R2 — "content-hash-per-item would close the coverage gate" and "on-device tier silently misses new registry items" — are both closed by the same primitive: a deterministic corpus revision that becomes the identity of the vector artifact.
Deterministic revision derivation. catalog-artifact.ts:131-147 — localVectorRevision(model, modelRevision, dimensions, entries) sorts entries by name via localeCompare, then hashes JSON.stringify({model, modelRevision, dimensions, batchSize, rows}). batchSize is inside the digest, which matches the "padding within a batch changes quantized output" invariant Miga's docblock at build-local-vectors.ts:44 (now LOCAL_VECTOR_BATCH_SIZE, exported) has always named. Direct determinism tests at catalog-artifact.test.ts:151-183: order-independence, corpus-sensitivity, model-sensitivity, dimension-sensitivity, model-revision-sensitivity — each proves a single dimension of the revision by pinning the other four.
Publish path wires revision into three places. build-local-vectors.ts:100-138 — the build script writes:
local-vectors.jsongetsmodelRevision+revisionalongsidemodel/dimensions/names.registry/registry.jsongetscatalogArtifact.revisionmatching.- The console output line at
:141names the revision alongside dimensions and payload — trivial provenance for the next contributor.
Schema-level enforcement at packages/core/schemas/registry.json:24-34: the new catalogArtifact property requires revision matching ^[a-f0-9]{64}$ (SHA-256 hex). Symmetric copy in docs/schema/registry.json.
Client refresh matches the specification. localSemantic.ts:80-101 — the download-first-then-check pattern is intact:
vectorPairAgreesat:80-93:bin.byteLength === names.length * LOCAL_MODEL_DIMENSIONS * 4anddimensions === LOCAL_MODEL_DIMENSIONS.vectorRevisionAgreesat:87-101:parsed.revision === expectedRevision, undefined-expected is permissive for pre-revision manifests.- Both checks fire BEFORE either file lands (
:127-129), so a bad refresh leaves the previous complete pair in place. Directly test-locked atlocalSemantic.test.ts:74-90: mismatched-revision fetch → returns false → cache still holds the previous pair unchanged.
Standing opt-in refresh, no re-prompt. catalog.ts:106-115:
const revisionStale = opts.artifactRevision !== undefined && cachedLocalVectorRevision() !== opts.artifactRevision;
if (!hasLocalVectors() || revisionStale || countUnindexed(...) > 0) {
await fetchLocalVectors(opts.registry, { expectedRevision: opts.artifactRevision });
}Reaches the refresh path only through prepareOnDeviceTier, which the caller enters via routineUpdate = ... status === "ready" && revisionDrift at catalog.ts:220-223. Consent is not asked again — the recordLocalModelConsent(true) at :106 runs only from declined/not-asked, so a ready cache updates silently. Test at catalog.test.ts:317-326 locks the ready→refresh path; test at :328-345 covers the model-revision-change fallback direction.
Model revision refresh path is unchanged. localModel.ts:46, 66, 72 — the pinned LOCAL_MODEL_REVISION is now exported (so the vector build script hashes it into the revision), but the runtime path is byte-for-byte identical: SHA-256 verify each artifact, unlink and re-download on mismatch. Bumping the model revision constant changes the URL AND the vector revision AND the SHAs, so both are re-fetched on next catalog invocation under existing consent.
Fallback message on failed refresh. catalog.ts:125-130:
} else if (opts.artifactRevision !== undefined && cachedLocalVectorRevision() !== opts.artifactRevision) {
warn("on-device search is using the previous catalog vectors because the update failed");
}Matches the claim ("keeps the previous complete pair and reports that fallback"). Not silent, not fatal — the tier still ranks against last-known-good.
Coverage gate now catches same-name drift. check-artifact-coverage.ts:60-99. The old name-only detection stays (still catches add/remove), the new revision comparison catches everything else:
artifactRevisionMatches = artifact.revision === expectedRevisionregistryRevisionMatches = registry.catalogArtifact?.revision === expectedRevision- Fails when either is missing or drifted.
- The docstring change at
:1-11names the honest new state: pre-commit hook rebuilds when the model is available, CI verifies via revision when it isn't.
Concrete: editing registry/blocks/foo/registry-item.json to change the title without regenerating vectors → corpus changes → expectedRevision changes → both artifact.revision and registry.catalogArtifact.revision are still the pre-edit hash → gate fails. This is exactly the class my R2 nit named.
Nits
revisionfield inlocal-vectors.jsonis optional in the parsing atlocalSemantic.ts:87-101. Undefined-expected returns true, and pre-revisionlocal-vectors.jsonfiles (without arevisionkey) load fine. This is deliberate for the transition window; naming it so a future contributor doesn't tighten it without checking. Follow-up when all published artifacts carry a revision.- Coverage gate reads
packages/cli/src/registry/localModel.tsfromscripts/catalog/. The dependency direction (script depends on CLI package internals) has been fine historically, but the newLOCAL_MODEL_REVISIONexport is now load-bearing for the artifact identity. Worth naming in the export site's docstring so a future contributor doesn't move it back to non-exported. Small. - On-device cap at 25 hardcoded, no
--limit. Still the same nit from R2, not touched here. Genuinely follow-up.
Every named claim in the PR body is delivered, with direct test locks for each. Both my R2 follow-up nits ("content-hash-per-item on the coverage gate", "on-device tier silently misses new items") are closed as a byproduct of the revision primitive.
Clean; ready from where I sit — stamp routing per standing rule.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 2e6bfb3d6 (delta from 4818dd072).
R3 adversarial delta-verify. Miguel's claim on the delta was:
"Deterministic vector-artifact revision + already-opted-in client compares that revision with its cached metadata and refreshes only the small vector pair when stale. Fetched pair must match the advertised revision and structural contract before replacement; offline/invalid keeps previous pair and reports fallback. Model revision change refreshes the SHA/size-pinned model under standing opt-in without a new consent prompt. Model-free repository gate catches same-name title/description/tag drift."
All 8 sub-claims land at the exact code paths. Independent trace below.
R3 delta verified
1. Deterministic revision computation — VERIFIED. scripts/catalog/catalog-artifact.ts:131-147 — localVectorRevision(model, modelRevision, dimensions, entries) sorts entries via localeCompare, JSON.stringify over a fixed key order (model, modelRevision, dimensions, batchSize, rows), then sha256Hex. LOCAL_VECTOR_BATCH_SIZE=16 is exported at :20 and mixed into the hash — matches the "padding within a batch changes quantized output" invariant. Determinism tests at scripts/catalog/catalog-artifact.test.ts:151-183 pin each contributing dimension independently (order, text, model id, model revision, dimensions).
2. Stale-detection client compare — VERIFIED. packages/cli/src/registry/localSemantic.ts:186-196 — cachedLocalVectorRevision returns undefined on missing/corrupt pair (via hasLocalVectors gate + try/catch) or an absent revision field, otherwise the string. packages/cli/src/commands/catalog.ts:220-223:
searchContext?.status.status === "ready" &&
artifactRevision !== undefined &&
cachedLocalVectorRevision() !== artifactRevisionFirst-run (no cache) is handled by the parallel !hasLocalVectors() branch at :110.
3. Refreshes only vectors on vector staleness — VERIFIED. packages/cli/src/commands/catalog.ts:106-115 — ensureLocalModel() is a no-op when isLocalModelReady() is true (SHA-pinned artifacts already on disk), so the model isn't re-downloaded on a pure-vector refresh. Then only fetchLocalVectors(registry, {expectedRevision}) runs — that hits /catalog-artifact/local-vectors.json and /catalog-artifact/local-vectors.bin only (localSemantic.ts:111-117). Tokenizer/model files aren't touched.
4. Structural contract before replacement — VERIFIED. packages/cli/src/registry/localSemantic.ts:104-133. The pattern is: full download into fetched: Array<[string, Buffer]> in memory, then TWO gates:
vectorPairAgreesat:66-80:dimensions === LOCAL_MODEL_DIMENSIONSANDbin.byteLength === names.length * dim * 4.vectorRevisionAgreesat:82-96:parsed.revision === expectedRevision.
Both fire on line :122 — BEFORE the writeFileSync loop at :127-129. Test at packages/cli/src/registry/localSemantic.test.ts:73-90 locks it: a mismatched-revision fetch returns false AND the pre-existing "old"/"old" pair is unchanged on disk after the call.
5. Offline/invalid keeps previous pair + reports fallback — VERIFIED. Two disjoint paths, both non-destructive:
- Fetch throws (offline):
tryblock catches atlocalSemantic.ts:131-133, returns false, previous files untouched. - Fetch succeeds but content invalid: contract gates at
:122return false pre-write, previous files untouched.
Warning surfaced at catalog.ts:125-130: "on-device search is using the previous catalog vectors because the update failed" when cachedLocalVectorRevision() !== artifactRevision post-fetch. Test at packages/cli/src/commands/catalog.test.ts:347-357 locks the failed-fetch + warning path. In JSON mode the warning rides into envelope.warnings, in TTY it goes to stderr — same message, both surfaces.
6. Model revision refresh under standing opt-in, no new prompt — VERIFIED. packages/cli/src/registry/localModel.ts:107-110 — isLocalModelReady() verifies BOTH artifacts by SHA-256. A LOCAL_MODEL_REVISION bump changes both artifact URLs AND both pinned SHAs, so an on-disk file from the old revision fails SHA, isLocalModelReady() → false, consent still true → localModelStatus() === "unavailable" at :131.
routineUpdate in catalog.ts:218-222 catches status === "unavailable" → enters prepareOnDeviceTier → the recordLocalModelConsent(true) at :103-105 only runs for declined/not-asked, NEVER for unavailable → ensureLocalModel() re-downloads silently. No prompt fires (all three prompt branches are gated on not-asked). Test at catalog.test.ts:328-345 locks it: state.modelStatus = "unavailable" → tier === "on-device", downloads === 1, consentRecorded === [].
7. Declined stays declined even on model-rev change — VERIFIED. routineUpdate requires status === "unavailable" OR status === "ready" && revisionDrift. declined matches neither → no auto-prepare. Only reachable with an explicit --on-device, and there the declined && !assumedYes branch at catalog.ts:65-70 warns + returns before any download. --yes remains the only override, which is the same R1-agreed semantic. Verified against the R1-cited consent-loop tests still in place at catalog.test.ts (existing declined branches).
8. Model-free repository gate catches drift — VERIFIED. scripts/catalog/check-artifact-coverage.ts:40-88 now:
- Reads the registry from disk via
catalogFromRegistry(title, description, tags — same fields the build uses). - Recomputes
expectedRevision = localVectorRevision(...)— no model, no network. - Compares to BOTH
artifact.revision(in the vectors metadata) andregistry.catalogArtifact.revision(in the registry manifest). - Fails when either is missing or drifted, printing expected/artifact/registry side-by-side.
Concrete drift case: edit registry/blocks/foo/registry-item.json title without regenerating → catalogFromRegistry reads new title → expectedRevision changes → artifact.revision still points at pre-edit hash → gate exits 1. The old name-only unindexed check is preserved for the add/remove case.
Schema wired symmetrically at packages/core/schemas/registry.json:24-34 and docs/schema/registry.json:24-34 (^[a-f0-9]{64}$), and typed at packages/core/src/registry/types.ts:131-134.
Adversarial checks
- Consent bypass under standing opt-in path — traced; no bypass.
recordLocalModelConsent(true)is gated ondeclined/not-asked, never fires on theunavailable(standing-opt-in-with-missing-artifacts) branch. The three prompt branches all requirenot-askedAND!assumedYes. - Contract-validation-bypassed replacement — both agrees-checks are pre-write; writes are unconditional-order behind a single guard. No path replaces on a partial-agree pair.
- Race between refresh calls introducing corruption — the write pattern is
writeFileSyncper file with no tmp/rename, so two concurrent CLIs at the SAME revision produce byte-identical writes (safe). Two concurrent CLIs at DIFFERENT revisions could interleave a mixed.json/.binpair on disk — but this is pre-existing (also true at R2), not R3-delta. TheexpectedRevisionguard on the fetch response tightens the door on stale server responses but doesn't atomize the local writes. Filing as follow-up-only, not blocking. - Silent-fallback masking real errors — the "using previous catalog vectors because the update failed" warning surfaces on stderr AND in
--jsonenvelope; not silent. - New hardcoded URLs / new dependencies — none. HF URLs still build off
LOCAL_MODEL_REVISION; vector URLs still off the configured registry base.
Nits (P3, non-blocking)
localeCompareinlocalVectorRevisionis locale-sensitive without an explicit locale arg (catalog-artifact.ts:143). All current catalog names are ASCII, so no practical drift. Future non-ASCII catalog names could produce different hashes on different maintainer locales. Cheap defensive fix:.sort()(byte-order) or.localeCompare(other, "en"). CI runs in a controlled locale so this is a maintainer-machine-only risk.revisionis optional in the parser — deliberate for the transition window (pre-revision manifests still load). Named for a future contributor not to tighten without checking that all published artifacts carry a revision.LOCAL_MODEL_REVISIONis now load-bearing across the script boundary — a docstring note at the export site would help a future contributor not un-export it. Trivial.
Verdict rationale
All 8 named claims land at the code paths Miguel described, with direct test locks for each. Both R2 follow-up nits (content-hash-per-item on the coverage gate, on-device tier silently missing new items) are closed as byproducts of the deterministic-revision primitive. No new P1/P2. The concurrent-write race is pre-existing, not introduced by R3.
Stamping.
— Review by Via
2e6bfb3 to
6c0042a
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 6c0042a2b (delta from 2e6bfb3d6).
Narrowest possible delta, correct primitive. catalog-artifact.ts:137-139 swaps localeCompare for JS string </> code-unit comparison:
const rows = [...entries.entries()].sort(([left], [right]) =>
left < right ? -1 : left > right ? 1 : 0,
);Locale-independent by construction — < on strings compares UTF-16 code units, which are stable across every Node runtime, every OS locale, and every ICU version. localeCompare's per-environment behavior class (e.g. Swedish locale ä sorting after z, German phonebook locale sorting ä equivalent to ae, ICU version differences on collation weights) is now closed as an input to the artifact identity — the corpus revision cannot silently drift because a CI runner upgraded its ICU tables.
Regression test at catalog-artifact.test.ts:166-188 locks the behavior directly with a mixed-character corpus (ä-item, a-item, z-item) and asserts the resulting rows are [a, z, ä] — the code-unit order (97, 122, 228 in UTF-16), not any Latin-collation order. Anyone reintroducing localeCompare on a machine with a locale that sorts ä between a and b will see this test fail deterministically.
ASCII revision holds. Verified: registry/registry.json and registry/catalog-artifact/local-vectors.json both still carry 3e0b7c14… at this head — bit-identical to the R3 head 2e6bfb3d6. Consistent with the claim that the sort primitive change is a no-op for ASCII names, so the shipped vectors don't need a rebuild.
Nothing else changed. Clean; ready from where I sit — stamp routing per standing rule.
vanceingalls
left a comment
There was a problem hiding this comment.
R4 delta-verify — APPROVE
Amendment 2e6bfb3d6 → 6c0042a2b (both share parent 4818dd072; three-dot compare noise aside, direct file diffs show the delta is exactly two hunks):
scripts/catalog/catalog-artifact.ts:137—localeCompareswapped for explicit code-unit compare (left < right ? -1 : left > right ? 1 : 0). NolocaleCompareleft in any catalog script.scripts/catalog/catalog-artifact.test.ts— newuses locale-independent code-unit orderingtest pinsa-item → z-item → ä-itemand asserts the full sha256 over the expectedrows, so a regression to locale sort fails both order and hash.- Published ASCII revision
3e0b7c14…5558238unchanged inregistry/registry.jsonandregistry/catalog-artifact/local-vectors.json— the current corpus is code-unit-equal under both algorithms, hash preserved.
Preflight + WIP checks green. Nothing else to raise — clean P3 close.
— Via
The base branch was changed.
Browsing the registry means matching names and tags, which fails whenever the author's wording differs from yours. "make the pace feel faster" finds nothing when the move is described as "velocity-driven blur". This ranks by meaning instead. Three tiers, and the command always says which one answered: words shared vocabulary, free, offline, no account on-device bge-small, free, offline, one opt-in download hosted Gemini, free for signed-in HeyGen users The tier is stated because a quietly worse answer looks exactly like a good one. --json carries it as a token alongside dropped, shown, total and top_score, so an agent reads provenance as data rather than matching English that is written to be reworded. Two consents, asked once each, and never conflated. Sending a query is a privacy question, so the prompt says the query is sent. Downloading a model is a disk and bandwidth question, so that prompt talks about size. Neither fires without a terminal: an unattended run sends nothing and downloads nothing unless a flag records that a person agreed. The catalog is derived from registry-item.json rather than from a separate document, so the set that is ranked and the set that can be installed are the same object by construction. Only the on-device vectors are committed; the hosted vectors are nine megabytes and belong on the server. top_score is reported and never acted on. A "nothing matched" threshold looked clean on long briefs and collapsed on the short queries people type: "a logo appears" scores 0.6181 and keyboard mash scores 0.6417, so any cut that catches the noise rejects the real query. The measurement is in the evals directory rather than in this branch. Not covered here. The published recall figures were measured against a separate hand-written document, not against registry text, so they should not be quoted for this catalog until re-measured. The offline tier needs a normal install: a single-file build cannot load the native ONNX runtime, which the command now reports instead of silently degrading. And the drop-detection path has never been observed firing outside its author's tests.
Three things `bun run lint` and `fallow audit --base origin/main` rejected. CI runs both, so none of this branch would have gone green. Found by running them, not by reading the diff. process.exit in catalog.ts, twice: an invalid --type and a cancelled picker. check:cli-process-ownership reserves that for cli.ts, and the rule is not cosmetic — process.exit tears the process down where it stands, so anything cli.ts has queued to run on the way out is dropped. finishCommand throws a CliResultSignal that cli.ts turns into the exit code, which is what init.ts already does for a cancelled prompt. Three exports with no consumers. normalize keeps its body and loses its export; localEmbedder is the only caller. modelsDirectory goes entirely, having no caller inside its file or out. The WordPieceConfig re-export goes, and with it the import it existed to forward: the type is exported from wordpiece.ts, where its consumers already take it from. Complexity. prepareOnDeviceTier is lifted out of run(), which took run from 64 cyclomatic and CRAP 948 to 54 and 684. That block is one decision — can the offline tier run, and if not, why not — and its only product is a list of warnings, so it reads and tests as a unit, which it could not do inline. The rest is suppressed rather than refactored, each with its reason on the line above. Finishing run() means extracting its three output paths, and that is a refactor of a command this branch already changes for other reasons: a separate initiative, not something to absorb here. Every suppression says what shape the function has and why; a bare marker on a function nobody can justify is how a threshold stops meaning anything. Verified: `bun run lint` exits 0, fallow reports no issues across 27 changed files, and 2540 CLI tests pass.
Search now has two tiers, both local: shared-vocabulary word matching, and the opt-in on-device model. The hosted tier, which sent the query to a HeyGen endpoint and ranked it with a hosted model, is removed. This is a scope decision, not a defect. The endpoint works and its own change is reviewed and green; it is simply not what we want to ship first. Landing local only means the feature has no backend dependency, no auth requirement, and nothing leaves the machine unless someone opts into downloading a model. Gone: registry/smartSearch.ts and its test, the --smart and --no-smart flags, the outcome plumbing through the command, the remote branch of applySearch, the remote tier, and the hosted-only JSON fields (ranking, catalog_version, top_score). Also the smartSearchEnabled consent field in telemetry config, which was the persisted storage behind the hosted consent and would otherwise have been left as dead configuration surface. Kept exactly as they were: both local tiers, the --on-device and --yes flags, the download consent prompt, and the runtime check that happens before the download rather than after it. The --json envelope still reports query, tier, tier_detail, shown, total, dropped, warnings and results, so an agent can still tell which tier answered and why. tierToken now distinguishes on-device from words. Verified: lint exits 0, fallow reports no issues, 2522 CLI tests pass, and the command was exercised directly. A query answers on the on-device tier where the model is installed and falls back to word matching where it is not, reporting that fallback in warnings rather than silently. An unknown --type still exits 1 with a readable message, and --smart is now rejected as an unknown flag.
The dropped count was computed against the list left after the user's own --type and --tag filters, so every move the user excluded was reported as one the registry is missing. Filtering made the number go up: the same query reported 277 unfiltered and 302 with --type block. The count exists so a caller can tell "nothing matched your words" apart from "the ranker suggested things this project cannot install". Conflating it with user filtering destroys exactly that signal, and worse, genuine index skew and a self-inflicted filter printed a byte-identical line with opposite remedies -- one means refresh the shelf, the other means drop a flag, and refreshing does nothing. Now counted against the registry rather than the filtered view. The manifest is already fetched whole and narrowed in memory, so keeping the unnarrowed name set costs no extra request, and item loading still runs only on the filtered subset. Verified against ground truth rather than by eye: the vector artifact holds 411 names, the registry holds 168 installable items, and 134 of those names exist in both, so 277 are genuinely uninstallable. The count now reads 277 unfiltered, 277 under --type block, 277 under --type component and 277 under --tag, and the skew it reports is real -- the artifact predates dropping the UI primitives and still ranks moves that are no longer on the shelf. Reported by Vance Ingalls, who also noted this closes an item the status doc listed as unverified. Two earlier sweeps could not make the count fire because neither combined a filter with a query. Tests pin the three cases: a genuinely absent name counts, a filter-excluded name does not, and a fully installable ranking reports zero.
The on-device index was fetched once and never revalidated: the only freshness check was two existsSync calls. A move added after that fetch was invisible to meaning search permanently, not down-ranked but absent from the candidate set. The registry manifest on the same command carries a 24h TTL, so the two halves of one feature disagreed about staleness. The dropped count reported over-coverage only, names the index has that the registry lacks. Under-coverage was never computed, so the harmless direction was instrumented and the costly one was silent. Reproduced with an index truncated to 120 of 168 moves: dropped read 0, perfect health, while 48 moves were unreachable. Counts under-coverage from the name list the artifact already carries, so no extra request. Warns only when non-zero, and names the remedy. The remedy had to be made true: --on-device could not refresh a stale index because hasLocalVectors short-circuited the fetch. That flag now refetches when the index is absent or no longer covering. Two defects the reproduction surfaced. A failed refresh reported the tier unavailable while the old vectors were still on disk and still ranking. And the fetch wrote its two files one at a time, so failing between them paired a new name list with an old matrix, a hard load error rather than stale data. It now writes both or neither, which matters more once refresh runs on staleness. top_score returns, scoped to the on-device tier and set to the score of the best result actually shown rather than the ranking head, which can describe a row the caller never received. Also: scripts/ is now typechecked. It never was, which is how a build script that crashes after the paid embedding call, and two scripts whose imports do not resolve at all, went unnoticed. 43 errors fixed, no suppressions. And the docs stop describing a --smart hosted tier that was deleted, an item that does not exist, and a registry refresh that cannot fix a stale vector index.
The catalog vector artifact is regenerated by hand. Nothing in CI, in package.json or in a hook rebuilds it, because embedding needs the 32 MB model. So adding a registry item silently makes it invisible to meaning search until someone remembers to regenerate. The failure is asymmetric, which is what makes it easy to miss. Removing an item is self-healing: the ranker still scores the dead vector, then filters the name before display, so a user is never offered something they cannot install. Adding one is not: the item is absent from the candidate set entirely, not ranked low. Comparing the two name lists needs neither the model nor a network call, so the gate runs in seconds. CI checks rather than fixes, for the same reason it cannot regenerate. Scoped to blocks and components. Examples are starter projects a user scaffolds, never something catalog ranks, and the artifact carries no vector for them, so demanding one would keep this gate permanently red and it would be ignored within a week. Verified in both directions rather than assumed: adding an unindexed item exits 1 and names it, restoring the registry exits 0.
build-local-vectors.ts read registry/catalog-artifact/catalog.json, a file no script in this repo writes and which is not committed, so the documented regeneration command failed on a missing path. That is why the index could drift from the registry with nothing to run to fix it. It now reads registry/blocks/* and registry/components/* through catalogFromRegistry, the existing helper that already produced the right shape but had no caller. Rebuilding reproduces the shipped 168 rows byte for byte. A lefthook catalog-index command regenerates and re-stages both artifact files whenever a staged registry-item.json changes, mirroring the skills-manifest pattern, so adding or removing an item keeps the index in sync without anyone remembering to. Verified end to end: staging a new item took the artifact 168 to 169 rows and staged it in 0.80s.
The two artifact files have to agree on how many rows there are, and until now nothing checked that before writing them. A truncated or wrong-model response landed in the cache and only failed at load, on every later search, until someone cleared it by hand. The pair is now checked first and refused as a unit, and the cache is created 0o700 with 0o600 files rather than inheriting the umask of a directory the caller may have pointed anywhere. Also lifts the capture setup the two preview generators had drifted into sharing into scripts/preview-capture.ts, and splits the vector builders batching and packing out of main. Both were findings the audit attributed to this branch.
Restacking took the base package.json wholesale, which dropped the vitest dependency and the scripts/catalog run this PR adds. Both belong here rather than under it.
Answering no to the on-device download offer recorded no and warned, then carried on. The guard below it is localModelConsent() !== false, which the decline had just made false, so it was skipped rather than taken: control reached recordLocalModelConsent(true), overwrote the answer with yes, and fetched the 32 MB model the user had refused. Next run it never asked again. No test could catch it. The stub pinned localModelStatus to ready, so the prompt never fired, and recordLocalModelConsent was a no-op that recorded nothing. Two tests now cover the offer, and they need three things the old stubs did not model: the run has to look like a terminal, because off one the command treats --on-device as the consent and never asks; the ONNX probe has to answer true, or an accepted offer returns at the runtime guard before it can download; and the status has to follow the recorded answer, or the second offer later in the run fires as well. Removing the return makes the decline test fail.
The pre-commit hook rebuilds the search index, and rebuilding needs the 32 MB embedding model. An outside contributor adding a registry item does not have it, so their commit died inside the ONNX loader on an ENOENT naming a path they never set, and the CI gate then told them to run the command that had just crashed. The model is an opt-in for search, not a build dependency, so nobody is charged for it to contribute. The builder checks first and explains itself, exiting 3 for cannot as distinct from 1 for failed. The hook treats 3 as skip and lets the commit through. The gate now names both paths: regenerate if you have the model, leave it if you do not and a maintainer will. Verified both ways: with no model the builder explains and the hook exits 0; with the model it still regenerates byte-identically.
…a generated file Two defects, one of them the reason 64 stale entries survived in registry.json. The checklist told contributors to add their item to registry/registry.json. That file is generated from the item directories, so an entry added by hand survives until the next regeneration and then vanishes, and one left behind for a directory that no longer exists is worse: hyperframes add resolves the name and then fails on missing files. Both CONTRIBUTING.md and the agent-facing skill reference now run the generator instead. Nothing said contribution was maintainer-only, but nothing said it was not either, and two steps do need assets an outside contributor has no reason to install. Those are now named in a table with what happens if you do not have them, matching how the preview image was already handled. The search index is the new one: the model behind it is a 32 MB opt-in for search, not a build dependency.
6c0042a to
07d3f60
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at d721928c5 (delta from 6c0042a2b).
Correct fix. localSemantic.test.ts:10-13 — the shared join(tmpdir(), \hf-vec-${process.pid}`)is replaced with a per-testmkdtempSync(join(tmpdir(), "hf-vec-"))inbeforeEach. mkdtemp is atomic on POSIX and creates a unique random suffix, closing both the symlink-race class CodeQL flags and any risk of two files-worth of tests colliding on the same directory. Cleanup pathway (afterEach→rmSync(dir, ...)) is unchanged and still runs against the newly-created path. vi.unstubAllGlobals()` cleanup unchanged.
Change is confined to the one test file, seven cases in this suite still cover the fetch-pair-agrees / revision-agrees / bad-refresh-preserves-previous flow — no test coverage was walked back to close the CodeQL alert.
Nothing else changed at this head. Clean; ready from where I sit — stamp routing per standing rule.
vanceingalls
left a comment
There was a problem hiding this comment.
R5 delta-verify (6c0042a → d721928): CodeQL temp-dir fix in packages/cli/src/registry/localSemantic.test.ts verified — process.pid-derived path replaced with mkdtempSync(join(tmpdir(), "hf-vec-")) in beforeEach, rmSync(dir, { recursive: true, force: true }) cleanup preserved in afterEach. Prefix has trailing hyphen so the random suffix yields readable paths. No new findings; the two unchanged registry/cache data-flow alerts are the trust-boundary false positives already triaged in R1 (tip commit only touches this one test file, so they cannot have moved). Rest of the compare is mechanical restack on new master.\n\n— Via
What
Catalog search that runs on the user's machine, in three named tiers: word matching by default, an opt-in on-device meaning tier, and neither when the query is absent. Ships the vector index the meaning tier reads, the scripts that build it, and the gate that keeps it covering the registry.
Stacked on #3150.
Why
Word matching cannot connect "make the pace feel faster" to a whip pan, because they share no words. The hosted endpoint can, but it needs an account and a network, which rules it out for the two cases this is for: an agent picking a block, and anyone offline.
The hosted tier is dropped rather than kept alongside. Two ranking paths that answer differently for the same query is a support burden, and only one of them works without an account.
How
Three tiers, named in the output. The default ranks by name, title, description and tags.
--on-deviceadds meaning, and every reason it could not run is both printed and carried in--json, so an agent sees the same explanation a terminal does. A declined download, a single-file build with no ONNX runtime, and a failed fetch are distinct messages rather than one silent fallback.Declining means declining. The offer's decline branch returns. Without that it recorded no, then fell past a guard that reads
localModelConsent() !== false(skipped precisely because the answer was now no), reachedrecordLocalModelConsent(true), and downloaded the 32 MB model it had just been refused. Two tests cover the offer, and removing the return makes the decline one fail.The index is checked before it is trusted.
fetchLocalVectorsrefuses a pair whose metadata and matrix disagree on row count, so a truncated download is discarded rather than cached. Writing first and discovering it at load meant every later search failed until someone cleared the cache by hand. The directory is0o700and the files0o600, because the caller can point it anywhere.Regenerating the index is one command and mostly automatic.
build-local-vectors.tsreads the registry directly. It previously read acatalog.jsonthat no script in the repo writes, so the documented command failed on a missing path, which is why the index was allowed to drift. A lefthook hook regenerates and re-stages it whenever a stagedregistry-item.jsonchanges.CI fails when the index stops covering the registry. Comparing two name lists needs neither the model nor a network, so the gate stays seconds long. It checks rather than fixes, because regenerating does need the model.
Coverage is reported, not hidden. A name the vectors carry that this registry cannot install is dropped from results and counted in
dropped; a registry item with no vector is counted inunindexedand named in a warning. A non-zerodroppedmeans the index and the registry are different generations.How it works, without a database
Three questions come up every time, so they are answered here rather than in review comments.
"Where is the search server?"
There isn't one. Every tier is a file on the user's disk and a loop over it.
flowchart TD Q["hyperframes catalog --query 'make the pace feel faster'"] --> HasQ{query given?} HasQ -->|no| LIST["list the catalog<br/>no ranking"] HasQ -->|yes| OD{--on-device?} OD -->|no| WORDS["tier: words<br/>match name, title,<br/>description, tags"] OD -->|yes| READY{model and vectors<br/>on disk?} READY -->|no| GATE["ask once, then fetch<br/>declining stops here"] READY -->|yes| MEAN GATE -->|accepted| MEAN["tier: on-device<br/>embed the query,<br/>cosine against every row"] GATE -->|declined or failed| WORDS WORDS --> OUT["ranked results<br/>tier named in the output"] MEAN --> OUT style OUT fill:#1f6f3f,color:#fff style GATE fill:#7a4a00,color:#fffThe meaning tier is 168 rows of 384 floats. Ranking is a
forloop computing cosine against each row and a sort: no index structure, no service, nothing to keep running. At that size the loop is faster than a query would be."Where does the corpus live?"
In three places, each with one owner. Nothing is stored server-side per user.
flowchart LR subgraph repo["this repo, reviewed like code"] RI["registry/*/*/registry-item.json<br/>title, description, tags"] ART["registry/catalog-artifact/<br/>local-vectors.json + .bin<br/>168 rows x 384 dims"] end subgraph net["fetched once, on opt-in"] HF["huggingface<br/>bge-small-en-v1.5<br/>quantized ONNX, pinned revision"] REG["the registry URL<br/>serves catalog-artifact/"] end subgraph disk["the user's machine"] MOD["~/.hyperframes/models/<br/>the embedding model"] VEC["~/.hyperframes/catalog/<br/>a copy of the vectors"] end RI -->|"build-local-vectors.ts<br/>embeds title + description + tags"| ART ART --> REG REG -->|"only when --on-device"| VEC HF -->|"only after the offer is accepted"| MOD MOD --> RANK["query embedded locally"] VEC --> RANK RANK --> RES["ranked names"] style disk fill:#12263a,color:#fff style RES fill:#1f6f3f,color:#fffThe item's searchable text is its
title,descriptionandtags. The name is deliberately excluded: it is what the query is trying to find, so folding it in would reward items whose name echoes the wording over items that do what was asked."I add a component. When can users find it?"
The index is built from the registry and shipped with it, so the two travel together. Three checks stop them separating.
sequenceDiagram participant M as maintainer participant H as lefthook participant CI as CI participant U as a user's CLI M->>H: commit a new registry-item.json H->>H: rebuild vectors, re-stage them Note over H: identical inputs re-embed to<br/>identical bytes, so an unrelated<br/>edit leaves no diff M->>CI: open the PR CI->>CI: every searchable item has a vector? alt an item has no vector CI-->>M: fail, naming the items else covered CI-->>M: pass end Note over U: word tier: the live registry,<br/>24h cache. New item is findable<br/>as soon as it publishes. U->>U: --on-device with a stale index U->>U: count items with no vector alt any are missing U-->>U: warn, and refetch the vectors endSo the two tiers answer differently and say which one answered. Words sees a new item as soon as the registry serves it. Meaning sees it once the user's copy of the vectors is refreshed, which the CLI does itself when it notices items the index has no vector for. A
droppedcount in--jsonmeans the opposite skew: the index holds names this registry cannot install, so the two are different generations."I do not have the model. Can I still contribute a component?"
Yes, and nothing asks you to install anything. The index is the one part of a contribution a contributor cannot produce, so it is the one part they are not asked for.
flowchart TD C["contributor adds<br/>registry-item.json"] --> HOOK{"embedding model<br/>on this machine?"} HOOK -->|"no, the usual case"| SKIP["hook exits 3 and says so<br/>the commit goes through"] HOOK -->|yes| BUILD["rebuild the vectors<br/>and re-stage them"] SKIP --> PR["open the pull request"] BUILD --> PR PR --> GATE{"CI: does every item<br/>have a vector?"} GATE -->|"yes, they rebuilt it"| MERGE["ready to merge"] GATE -->|"no"| NAME["fail, naming the items,<br/>and saying a maintainer<br/>regenerates before merge"] NAME --> MAINT["a maintainer runs<br/>the one command"] MAINT --> MERGE style SKIP fill:#12263a,color:#fff style MERGE fill:#1f6f3f,color:#fff style NAME fill:#7a4a00,color:#fffThe model is a 32 MB opt-in for search, not a build dependency, so a blocked commit would be charging every contributor for a feature they may never use. The hook exits 3 to mean "cannot", which the hook treats as skip, as distinct from 1 meaning "failed". Until a maintainer regenerates, the new item is findable by word search and not by meaning, which is the same state as any item published after a user last refreshed their vectors.
"How big is it, and who keeps it fresh?"
Measured, not estimated. One vector is 384 floats, so an item costs
384 x 4 = 1536bytes plus its name.local-vectors.bin--on-devicelocal-vectors.json.binThe index reaches about 1.5 MB at a thousand items. The model dwarfs it and is fetched exactly once, so growth in the catalog is not what costs the user anything.
flowchart TD RUN["catalog --query ... --on-device"] --> HAVE{"vectors on disk?"} HAVE -->|no| FETCH HAVE -->|yes| COVER{"any registry item<br/>with no vector?"} COVER -->|"no, index covers the catalog"| RANK["rank, no network"] COVER -->|"yes, the catalog moved on"| FETCH["refetch both files<br/>252 KB, not the model"] FETCH --> CHECK{"do the two files agree<br/>on how many rows?"} CHECK -->|no| KEEP["discard, keep the old pair<br/>a truncated download<br/>never reaches the cache"] CHECK -->|yes| WRITE["replace the pair<br/>0o700 dir, 0o600 files"] WRITE --> RANK KEEP --> RANK style RANK fill:#1f6f3f,color:#fff style KEEP fill:#7a4a00,color:#fffNobody is asked to redownload anything. There is no TTL and no update prompt. The CLI compares the registry's item names against the names its own index carries, and refetches only when the catalog has items the index cannot see. That is a local read of a 4 KB name list, so the check itself costs nothing and happens on every on-device run.
A refresh that fails is not an error either: the previous vectors are still on disk and still rank, so the search continues and the output says what is stale about them rather than claiming the tier is unavailable.
The model is never refetched. It is pinned to one revision, and the vectors shipped in this repo are built with that same revision, which is what keeps a query and the rows it is compared against in the same space.
Test plan
2559 CLI tests pass, covering the tokenizer against a reference fixture, the ranking, the consent offer in both directions, and the refusal paths.
bun run test:scriptsandtypecheck:scriptsexit 0, lint and format are clean, and the coverage gate reports 168 registry items and 168 vectors.Regenerating the artifact from the registry reproduces the shipped rows byte for byte. Batch size is part of that: the same text embedded alone differs at cosine 0.9969, because padding within a batch changes the quantized result.
Size. Of the diff, 272 KB is the generated vector artifact and 292 KB is the tokenizer reference fixture. The hand-written surface is about 1,600 lines in the files under review, and about 2,700 counting every added line outside those two blobs.
Not covered. The embedding model is downloaded at first use rather than bundled, so the meaning tier's end-to-end path is not exercised in CI; the tokenizer is tested against a stored reference instead.
catalog.tscarries two complexity suppressions, and the one onrunsays in its own comment that splitting it is its own change, so that is deferred work rather than a settled design. The model file itself is written without a hash or size check, unlike the vectors, which are pair-validated.