Skip to content

feat(find): harden search failure modes and blend ranking by popularity - #2457

Open
lanyouxize wants to merge 1 commit into
vercel-labs:mainfrom
lanyouxize:fix/find-search-hardening
Open

lanyouxize wants to merge 1 commit into
vercel-labs:mainfrom
lanyouxize:fix/find-search-hardening

Conversation

@lanyouxize

Copy link
Copy Markdown

Summary

skills find had four defects. One of them was actively misleading: a failed
search was reported to the user as "No skills found", i.e. a network outage was
presented as a factual search result.

# Defect Impact
1 Failures returned [], same as "no matches" 503 / DNS failure reported as fact
2 fetch(url) had no timeout Could hang indefinitely
3 No response ordering Stale reply could overwrite newer results
4 Client re-sorted by raw installs Server relevance discarded

Changes

1. Typed failure taxonomy. searchSkillsAPI now returns
SearchOutcome = ok | empty | timeout | server-error | network-error.
Only empty means "no match". Everything else prints an actionable message on
stderr and exits non-zero.

Before:

$ skills find paper        # network unreachable
No skills found for "paper"          # <- outage reported as a search result

After:

$ skills find paper
Could not reach the search service (TypeError: fetch failed). This is a network
problem, NOT proof that no matching skill exists. Check connectivity, or browse
https://skills.sh/ directly.
$ echo $?
1

2. Request timeout. Added AbortSignal.timeout(8_000) on the search path
(override with SKILLS_SEARCH_TIMEOUT_MS). The sibling download path already
used a 30s bound; 8s is deliberately tighter because searching is interactive and
the user has already concluded the tool is broken by then.

3. Response ordering + cache. Each request carries a monotonic sequence
number; replies whose sequence is stale are discarded. Debouncing only throttles
the request rate, it does not order responses. Repeated queries within 60s
reuse a TTL cache, so backspacing/retpying no longer re-hits the network.

4. Ranking blend. The API response is already relevance-ordered; the client
was discarding that by re-sorting on raw installs. Ranking is now
0.7 * popularity + 0.3 * relevance.

Popularity is log-compressed (log10(1+n)/log10(1e6)). This matters: install
counts span five orders of magnitude (15 to 452,294), so a raw-weight blend
would be fully swamped by the popularity term and collapse back into the old
pure-installs sort. The log makes the two terms comparable.

Measured on a live q=paper capture:

Metric Before After
Top-10 results with "paper" in name 3 5

Weight choice is data-backed, not arbitrary

The ranking suite sweeps the weight grid:

popularity weight top-10 relevance density unrelated skills in top 3
0.5 5 0
0.7 (default) 5 1
0.8 5 2
1.0 (old behaviour) 3 2

Density saturates at 0.5-0.7; 0.7 is the point where density is maxed while
unrelated promotions stay bounded. 1.0 reproduces the old defect.

Compatibility

  • Success path unchanged: same endpoint, parameters, and field names.
  • No new dependencies.
  • Node: AbortSignal.timeout requires 17.3+; CI matrix is 22.20.0 / 24 / 26.
  • SKILLS_API_URL / SKILLS_SEARCH_TIMEOUT_MS are opt-in overrides.

Testing

29 new cases across two files:

  • search-hardening.test.ts (17) - each failure mode asserted distinct
    from empty; AbortSignal always passed; never throws on network errors;
    sequence-guard ordering; cache TTL / eviction / owner isolation / case-insensitivity.
  • find-ranking.test.ts (12) - score bounds, monotonicity, determinism,
    input immutability, empty & single-element lists, weight-grid calibration on a
    real API capture.

The ranking tests use a real API capture as fixture rather than mocks, so
they encode the actual server response shape.

Test suite was adversarially verified

A green suite proves nothing unless it can go red. Three injected defects, each
caught:

Injected defect Caught by
Weights -> 1.0 / 0.0 (reproduce old bug) expected 3 to be greater than 3
isCurrent() -> always true (guard disabled) expected true to be false
Failure outcomes -> empty (old swallow behaviour) 3 tests failed: server-error / network / timeout

All defects reverted; grep -r "INJECTED BUG" src/ returns nothing.

Zero-regression evidence

Passed Failed
Baseline (upstream, changes stashed) 764 200
With this branch 793 (+29) 200 (unchanged)

Failure count is identical, and the delta is exactly the new tests.

Note on the 200 pre-existing failures: they are an environment artifact on
this machine, not a code issue. test-utils.ts spawns the CLI via
spawnSync(process.execPath, [...]), which fails here with EBUSY at a 3/3
reproduction rate - and for any executable, including cmd.exe /c echo hi -
while invoking node src/cli.ts directly from a shell works fine. This
Windows environment does not allow a Node process to spawn a child process.
The repo's devDependencies include @types/bun, suggesting these are normally
exercised under Bun.

Verified from a clean clone of the branch: pnpm install --frozen-lockfile ->
tsc --noEmit PASS -> 34/34 tests PASS -> live skills find paper returns
ranked results.

`skills find` had four defects that made the command unreliable and, in one
case, actively misleading.

1. Failures were indistinguishable from "no matches". `searchSkillsAPI`
   returned `[]` for a non-2xx response, a JSON parse error and a network
   exception alike. The CLI then printed `No skills found for "<query>"`,
   so a 503 or a DNS failure was reported to the user as a factual search
   result. Failures now return a typed `SearchOutcome`
   (`ok | empty | timeout | server-error | network-error`); only `empty`
   legitimately means "no match". Non-`ok` outcomes print an actionable
   message on stderr, state explicitly that this is NOT proof of no match,
   and exit non-zero so callers can retry.

2. The search request had no timeout. `fetch(url)` could hang indefinitely.
   Sibling code (`download-source.ts`) already used
   `AbortSignal.timeout(30_000)`; the search path now uses an 8s bound
   (override via `SKILLS_SEARCH_TIMEOUT_MS`), which is deliberately tighter
   because searching is interactive.

3. Out-of-order responses could overwrite fresher results. Debouncing throttles
   the request *rate* but does not order the responses; a slow reply for an
   older query could land after and clobber a newer one. Each request now
   carries a monotonic sequence number and stale replies are discarded.
   Repeated queries also reuse a 60s TTL cache, so backspacing and retyping
   no longer re-hit the network.

4. Ranking discarded server relevance. The response is already relevance
   ordered, but the client re-sorted by raw `installs`, which promoted
   unrelated high-popularity skills. On a live `q=paper` capture this left
   3/10 top results with "paper" in the name. Ranking is now a weighted blend
   (0.7 popularity / 0.3 relevance by default). Popularity is log-compressed
   because install counts span five orders of magnitude (15 - 452,294) and
   would otherwise swamp the relevance term entirely, collapsing the blend
   back into the old pure-installs sort. On the same capture the top-10
   relevant density improves from 3 to 5.

Success-path behaviour is unchanged: same endpoint, parameters and field names.
Adds no dependencies.

Tests: 29 new cases covering the outcome taxonomy (each failure mode asserted
distinct from empty), sequence-guard ordering, cache TTL/eviction/owner
isolation, and ranking calibration. The ranking suite is built on a real API
capture and sweeps the weight grid so the 0.7/0.3 default is backed by data
rather than intuition.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant