fix(catalog): name the better search tier when a search finds nothing - #3872
Conversation
The hint that tells a scripted caller the on-device tier exists was wired to the branch where the search succeeded, and was silent on the branch where it found nothing. The caller with the most reason to hear it was the only one who never did. It also only ever reached stderr, so a --json caller never saw it at all. The hint is now a returned sentence pushed into the same warnings array the zero-result envelope already serializes, and printed from that one value, so the terminal and the envelope cannot drift. Withheld when the query parsed to no searchable tokens, where the advice is already to search in English, and when a warning has already explained why the tier cannot run here. The consent gate is untouched: nothing downloads, and the sentence still asks the caller to check with a person before enabling it.
Deriving the status inside the hint helper re-read and re-hashed the on-device model from disk to answer a question the caller had already answered a few lines earlier. It now takes the resolved value. Both guards below the status check were invisible to the suite: every other case pinned a status that already returned null, so deleting either guard passed everything. Two tests now hold them, and each fails for its own mutation.
The CLI skill said that under --json nothing about the offline tier is printed at all. An agent following that sentence would never read the warnings array this branch writes the tier sentence into, so the fix would land in the code and never reach the caller it was written for. It also described warnings as firing only when a tier was asked for and could not run. That is now too narrow: a zero-result search emits the hint with no tier requested. Regenerate skills-manifest.json for the changed content hash.
jrusso1020
left a comment
There was a problem hiding this comment.
Head reviewed: f08eb5f
Independent pass. The three findings already listed as unapplied in the description — the hit-path --json gap, the truthiness gating, the matching test gap — are not repeated here.
Strengths
catalog.ts:675-679— returning the sentence instead of printing it is the right shape for the actual bug. The old helper could not have fed the envelope from any call site, so this is a structural fix rather than an added call in one more place.catalog.test.ts:506-528— the two tests added for the!unsearchableandwarnings.length === 0guards, with the comment explaining that every other "no hint" case pins a status that already short-circuits the helper, so deleting either guard passed the whole suite. That is mutation-survivor reasoning done properly, and it reproduces: each of those guards is killed by exactly one test.- Swapping
localModelStatus()for the caller'seffectiveStatusis safe.applySearchonly readsstatus.status === "ready"and pushes a warning; it never writes the stored status, so the snapshot cannot go stale between resolve and use.
important — the hint fires when the tag filter, not the search, emptied the list
catalog.ts:261-268. The guard is query && !unsearchable && warnings.length === 0. It never asks whether there was anything to search. When --tag matches nothing, tagged is [], applySearch ranks an empty array, and the zero-result branch tells the caller that a better tier is one consented download away — for a search where no tier can return anything.
Run in this suite's own harness with modelStatus: "not-asked":
--query "count up" --tag nonexistent -> warnings: ["consent"], shown 0, total 0
--query "count up" --tag nonexistent -> shown 0, total 0 [control: modelStatus "ready", ranking healthy and covering the query]
--query "count up" -> shown 1 [control: no tag filter — the move is right there]
The middle line decides it. With the tier fully on, the answer is identically empty, so the sentence asks someone to approve a 33 MB download that provably cannot change the result. SKILL.md now tells agents to act on exactly this array — "read that array and put the decision to the user yourself" — so the wrong ask reaches a human rather than dying in stderr.
This is the same shape as the unsearchable case the change already withholds for: the catalog has not been shown to be missing anything, and the advice points away from the real cause. The envelope already carries the discriminator, since total is tagged.length and reads 0 on exactly these runs:
const tierHint =
query && !unsearchable && tagged.length > 0 && warnings.length === 0
? localModelHint(json, effectiveStatus)
: null;Not gating: before this change a --json caller got nothing at all, so it is strictly better either way. But it is new advice in a case where it is wrong, and it is one clause.
nits
catalog.ts:200-204— pre-existing, raised because this change is what makes the envelope load-bearing. When--typeempties the registry before any search, a--jsonrun with a query prints bare[]— notier, noreport_gap, nowarnings. The note atcatalog.test.ts:180("both envelope-shaped emit sites are insideif (query)branches and both set it") is true of the two it means, but this third zero-result exit emits no envelope at all.catalog.ts:676— the TTY guard is the one branch the suite cannot reach:process.stdout.isTTYis falsy under vitest, so every test runs the non-interactive side and deleting the guard outright still passes all 39. Correct as written and unchanged from the old helper, but unpinned.catalog.ts:262—query &&is redundant, sinceeffectiveStatusis only assigned whenqueryis non-empty andlocalModelHintalready returns null without it. Harmless; noting it because it survives deletion for that reason rather than for a missing test.
What I verified rather than took
Mutation pass against the new code, run locally: dropping the warnings.push, the console.error, the !unsearchable guard, or the warnings.length === 0 guard is each caught by exactly one test — the description's table reproduces. Dropping the not-asked guard is caught by two. The query and TTY guards survive, for the reasons above.
skills-manifest.json is genuinely in sync: gen:skills-manifest regenerates 06b31056db460774 and reports no change. SKILL.md and the code agree at this head — the precise bullet ("when a search returned nothing and a better tier is still waiting on someone's consent") matches the not-asked gate exactly.
catalog.test.ts is 39/39 green here. Across src/commands + src/registry I get 1010 passed / 0 failed; 8 suites did not load in my worktree for want of a built @hyperframes/studio-server, which is my environment and not this change.
Verdict: APPROVE
Reasoning: The structural fix is right and the tests behind it are real — I reproduced the mutation table rather than taking it. The one finding is a narrow false-positive in new code with a one-clause fix, not something that should hold up the series.
— Rames Jusso
Summary
A scripted caller that searched the catalog and found nothing was told how to report a gap, but never that a second, better search tier exists. The sentence that says so was wired to the branch where the search succeeded, so the caller with the most reason to hear it was the only one who never did.
First of seven independently landable changes from a plan covering the dead ends the CLI puts in front of scripted callers. The rest — the real option surface on a rejected flag, conventional flag spellings, deprecation visible where items are chosen, and a lint rule plus registry sweep for a seek-safety defect class — land separately.
What changed
The hint is now a returned sentence pushed into the same
warningsarray the zero-result envelope already serializes, and printed from that one value. Before, it only ever reached stderr, so a--jsoncaller never received it at all — the two channels could not have drifted, because one of them was never written.It stays silent in three cases: the query parsed to no searchable tokens, where the advice is already to search in English; an earlier warning has already explained why the tier cannot run; and the caller has already answered the consent question.
The consent gate is untouched. Nothing downloads. The sentence still asks the caller to check with a person before turning the tier on.
While here: the helper derived model status itself, which reads and hashes the on-device model from disk. It now takes the status the caller already resolved a few lines earlier.
Session-settled decisions carried from planning: teach the caller that the better tier exists rather than enabling it on their behalf (user-directed, over auto-enabling for detected runtimes).
Test plan
Five scenarios from the plan, plus two added in review. Every behaviour claim is pinned by a test observed failing before the fix.
Four deliberate mutations were run to prove the tests are not vacuous, each caught by exactly one test:
warnings--jsonenvelope test!unsearchableguardwarnings.length === 0guardThe last one matters most: without that guard the same sentence lands twice in the array this change exists to make trustworthy, and before these tests that mutation passed the whole suite.
1134 tests pass across the CLI's commands and registry suites. oxlint, oxfmt and tsc clean.
Unapplied review findings
packages/cli/src/commands/catalog.ts:337— a--jsoncaller on the hit path still never receives the tier hintThe same hint is computed for a search that found results, but that code sits after the
if (json)block returns, so it is unreachable for exactly the caller this change targets. Three reviewers found it independently. It is pre-existing: the call already sat there, equally unreachable, before this change. Not fixed here because it means deciding whether a successful search should also prompt about a better tier — a product call, and this change was scoped to a failed search. If wanted: hoist the computation above theif (json)branch and push intowarningsthe way the zero-result branch now does.packages/cli/src/commands/catalog.ts:263— both call sites gate on truthiness rather than!== nullInert today, since the message producer returns a hardcoded non-empty string. Flagged because it is the same silent-pass shape as the bug being fixed: a future edit to that producer could reintroduce silence with no compiler or test signal.
--jsonrun with consent unanswered. That is where the first finding lives, so the coverage gap and the behaviour gap are the same gap.Review: five reviewers (correctness, testing, agent-native, adversarial, project standards). Three findings, one applied. The adversarial pass ran locally; no code was sent off the machine.