Summary
The stable-sort assertion in buildJobQuery's test suite passes vacuously. It has never verified the ordering property it names, because the term it probes for can never appear in the array it searches.
Found independently by two reviewers on unrelated PRs (#853 and the now-closed #854), each of whom noticed the same test-input change and traced it back to the same root cause. It reproduces on main today.
The defect
src/lib/job-search/query-builder.test.ts:414-424 on main:
it("preserves résumé order within the canonical and unrecognized tiers (stable sort)", () => {
const parsed = baseParsed({
skills: ["go", "rust", "underwater basket weaving", "competitive juggling"],
});
const query = buildJobQuery(parsed);
// Canonical tier keeps its own relative order (go before rust)...
expect(query.skills.indexOf("go")).toBeLessThan(query.skills.indexOf("rust"));
...
});
The go dictionary entry at src/lib/jd-match/skills.ts:80 is:
{ id: "go", aliases: ["golang", "go lang"] },
"go" is not among its own aliases, and compileIndex registers aliases only — never the id. So the résumé skill "go" does not canonicalize; it falls through to the unrecognized tier and is title-cased to "Go".
query.skills therefore never contains the lowercase string "go", and indexOf("go") returns -1. Since -1 is less than every valid array index, expect(-1).toBeLessThan(<any index>) holds no matter how buildJobQuery orders its output. The first assertion cannot fail.
The second assertion in the same test (the unrecognized tier, Underwater Basket Weaving before Competitive Juggling) is real and does exercise stable sort. Only the canonical-tier half is dead.
Why it matters
Stable ordering within the canonical tier is a real product property — query.skills feeds searchPhrase() and primaryKeyword() (src/lib/job-search/providers/keywords.ts:27,37), and searchPhrase takes query.skills.slice(0, 3), so which skills land in the first three positions determines the outbound keyword string. A regression that reordered the canonical tier would change what we send to Jobicy / Remotive / Arbeitnow and this test would stay green.
Fix
Use a probe term that actually canonicalizes. Both closed PRs converged on the same shape independently:
skills: ["golang", "rust", "underwater basket weaving", "competitive juggling"],
// ...
expect(query.skills.indexOf("Go")).toBeLessThan(query.skills.indexOf("Rust"));
Note the expected values are the display labels, not the ids — which is the other half of why the original looked plausible.
Verify the fix is not itself vacuous: assert both probes are present before comparing, e.g. expect(query.skills).toContain("Go"). A toBeLessThan against a possible -1 is the exact trap this issue is about, and the repaired test should not be able to fall into it a second time.
Acceptance criteria
- The canonical-tier ordering assertion fails when
buildJobQuery's canonical-tier sort is deliberately made unstable, and passes when it is restored. Demonstrate the fail-before.
- Neither probe term resolves to
-1; the test asserts their presence explicitly before comparing indices.
- Sweep the rest of
query-builder.test.ts and extract-jd-terms.test.ts for the same shape — any indexOf(...) fed to toBeLessThan / toBeGreaterThan without a presence check — and either repair or note each.
- No change to
skills.ts data. Whether "go" should be an alias of the go entry is a separate question (see below) and must not be smuggled in here; changing it would alter recognition and outbound egress.
Out of scope, but noticed while filing
{ id: "c", aliases: ["c language"] } (skills.ts:82) has the same id-is-not-an-alias shape: a résumé listing a bare C will not canonicalize through jd-match. That may well be deliberate — a bare c is a noisy token and the entry looks written to avoid it — but it is worth writing the reasoning down, and it is adjacent to the single-letter-skill work in #832. Not part of this issue; file separately if it turns out to be unintended.
Credit
Surfaced by @shubhransh-gupta (#853) and @qtjg (#854), independently and on the same day.
Summary
The stable-sort assertion in
buildJobQuery's test suite passes vacuously. It has never verified the ordering property it names, because the term it probes for can never appear in the array it searches.Found independently by two reviewers on unrelated PRs (#853 and the now-closed #854), each of whom noticed the same test-input change and traced it back to the same root cause. It reproduces on
maintoday.The defect
src/lib/job-search/query-builder.test.ts:414-424onmain:The
godictionary entry atsrc/lib/jd-match/skills.ts:80is:"go"is not among its own aliases, andcompileIndexregisters aliases only — never the id. So the résumé skill"go"does not canonicalize; it falls through to the unrecognized tier and is title-cased to"Go".query.skillstherefore never contains the lowercase string"go", andindexOf("go")returns-1. Since-1is less than every valid array index,expect(-1).toBeLessThan(<any index>)holds no matter howbuildJobQueryorders its output. The first assertion cannot fail.The second assertion in the same test (the unrecognized tier,
Underwater Basket WeavingbeforeCompetitive Juggling) is real and does exercise stable sort. Only the canonical-tier half is dead.Why it matters
Stable ordering within the canonical tier is a real product property —
query.skillsfeedssearchPhrase()andprimaryKeyword()(src/lib/job-search/providers/keywords.ts:27,37), andsearchPhrasetakesquery.skills.slice(0, 3), so which skills land in the first three positions determines the outbound keyword string. A regression that reordered the canonical tier would change what we send to Jobicy / Remotive / Arbeitnow and this test would stay green.Fix
Use a probe term that actually canonicalizes. Both closed PRs converged on the same shape independently:
Note the expected values are the display labels, not the ids — which is the other half of why the original looked plausible.
Verify the fix is not itself vacuous: assert both probes are present before comparing, e.g.
expect(query.skills).toContain("Go"). AtoBeLessThanagainst a possible-1is the exact trap this issue is about, and the repaired test should not be able to fall into it a second time.Acceptance criteria
buildJobQuery's canonical-tier sort is deliberately made unstable, and passes when it is restored. Demonstrate the fail-before.-1; the test asserts their presence explicitly before comparing indices.query-builder.test.tsandextract-jd-terms.test.tsfor the same shape — anyindexOf(...)fed totoBeLessThan/toBeGreaterThanwithout a presence check — and either repair or note each.skills.tsdata. Whether"go"should be an alias of thegoentry is a separate question (see below) and must not be smuggled in here; changing it would alter recognition and outbound egress.Out of scope, but noticed while filing
{ id: "c", aliases: ["c language"] }(skills.ts:82) has the same id-is-not-an-alias shape: a résumé listing a bareCwill not canonicalize through jd-match. That may well be deliberate — a barecis a noisy token and the entry looks written to avoid it — but it is worth writing the reasoning down, and it is adjacent to the single-letter-skill work in #832. Not part of this issue; file separately if it turns out to be unintended.Credit
Surfaced by @shubhransh-gupta (#853) and @qtjg (#854), independently and on the same day.