feat(critique): surface skills-ordering guidance in résumé review + rewrite lane (#544)

[s-annam]

Summary

Surfaces skills-ordering guidance in the résumé review + rewrite lane. A high-signal
skill sitting far down the list relative to the candidate's title/target now
triggers a coaching call-out (never a hard error), and the user can apply a
relevance-first reorder via the existing section-rewrite confirm/undo pattern.

The finding is a plain heuristic (title/target word-overlap + canonical-index
tie-break), independent of the on-device LLM — it renders inside
SkillTermGuidance, the existing non-LLM-gated heuristic surface, so it's visible
on every parse rather than only after a user opts into downloading the on-device
model.

Apply is scoped to the flat (uncategorized) skills list; a categorized résumé shows
the coaching note with a pointer at the existing manual drag controls instead,
since SkillsSection renders order from category grouping, not the flat array.

Verification

• 15 new unit tests: heuristic (skills-order.ts), the confirm/undo controller
  (useSkillsReorder), and the edit-hook reorder path (useEditableParse).
• npm run typecheck / npm run lint clean.

Stack position

Layer 3 of 4. Base: #872. Depends on #872.

Adversarial review

Two round-1 blocking findings, both fixed and verified in round 2:

• reorderSkills resurrected deleted skills — it overwrote the removed-skills
  set instead of merging with it, so a skill deleted before a reorder came back at
  the front of the list. Fixed to merge via Set union; regression test added.
• The heuristic was invisible without WebGPU — it originally mounted only
  inside the LLM-gated critique panel, contradicting its own "independent of the
  on-device LLM" docblock. Moved to SkillTermGuidance (single mount, no LLM gate);
  verified no double-mount and the panel's self-hide guard doesn't hide it.

Two nits remain, not blocking: the panel heading can promise a list it doesn't
render in the ordering-only edge case, and Apply-then-Undo leaves the résumé's
edit-dirty flag permanently set (autosave already fired on Apply, so low impact).

Closes #544

[cloudflare-workers-and-pages]

Deploying offlinecv with    Cloudflare Pages

Latest commit:	3faf2d5
Status:	✅  Deploy successful!
Preview URL:	https://7505af75.offlinecv.pages.dev
Branch Preview URL:	https://gh-batch-03-skills-order-cri.offlinecv.pages.dev

View logs

[Samhit21]

PR Review: feat(critique): surface skills-ordering guidance in résumé review + rewrite lane

Summary

The heuristic is well-shaped for what it is — pure, dependency-free, stable tie-break on original index, and conservative gating (5-skill floor, top-score-only buried, front window). I paid particular attention to reorderSkills, since the round-1 blocking bug lived there, and the fix holds up under the interactions that would break it: a skill deleted before an Apply stays deleted (its key survives the Set union while order is a permutation of the current list), Undo re-runs the same path and preserves the deletion, and delete-after-Apply still works because removeSkill filters added as well as recording in removed. The canApply gate on a categorised résumé matches how SkillsSection actually renders, so the withheld button isn't arbitrary.

One finding, in the relevance matcher.

Findings

1. skills-order.ts:70 — the 4-char prefix rule scores JavaScript against a Java title, and since a loose match can only add score it can become maxScore and drive the reorder; separately, the docblock's lead/leadership example can't fire because lead is a stopword. Both verified by running the function. [Suggestion]

PR-level: the stack claim doesn't match the branch

The body says "Layer 3 of 4. Base: #872. Depends on #872." As configured this PR targets main, carries a single commit, and contains neither #871's docs/scoring.md nor #872's render-findings.ts — it is standalone. I checked for a real coupling and found none (no file overlap with #872, no import of anything it introduces), so the base looks correct and the body looks stale. Worth fixing the body rather than the base: a reviewer told to read #872 first will go looking for a dependency that isn't there, and a merge-queue operator may sequence on it needlessly.

Checked, not raised

useSkillsReorder.ts uses the \u0000 join idiom at three sites and all three are the six-character escape rather than literal bytes, so it passes the #787 gate. And suggestedOrder re-sorting the whole list initially looked like a mismatch with the "move the relevant skills to the front" aria-label, but the tie-break on original index makes it a stable sort — unrelated skills keep their relative order, so the label is accurate.

Verdict

Action: COMMENT

Reviewed by: Claude Opus 5 (1M context)

[Samhit21]

PR Review (round 2): feat(critique): surface skills-ordering guidance

Summary

Fixed in c1329b6, and the negative test is the right one — asserting undefined rather than a score means it pins "no relevance signal at all", which is what actually protects against the wrong skill being promoted. The unreachable lead/leadership example is gone from the docblock too.

Verified by running it

• skills-order.test.ts 8, useSkillsReorder.test.tsx 5, useEditableParse.test.tsx 50, SkillTermGuidance.test.tsx 13, ResultDetail.test.tsx 23 — 99 passed
• npm run typecheck clean; npx eslint clean on the three changed source files

One consequence worth recording, not blocking

Requiring the full shorter token as a prefix (rather than a shared 4-char prefix) narrows the rule in a second way beyond java/javascript. Running both versions side by side:

pair	old	new
engineer / engineering	✅	✅
design / designer	✅	✅
data / database	✅	✅
java / javascript	✅	❌ (the fix)
developer / development	✅	❌
analytics / analytical	✅	❌
engineers / engineering	✅	❌

The last three are legitimate stem variants that the old rule caught: a "Software Developer" title no longer boosts a "Full-Stack Development" skill. I'm not treating it as a defect, because the two failure directions are not symmetric — over-matching promotes the wrong skill to the top of someone's résumé, while under-matching just misses a signal and leaves the order alone. For a feature that rewrites the user's document, that is the correct side to err on, and this was my suggestion so the tradeoff is mine as much as yours.

The docblock now says the rule matches engineer/engineering without noting what it stopped matching. A clause naming the tradeoff ("a stem that diverges before the end — developer/development — no longer matches; under-matching is the safe direction here") would save the next person rediscovering it. Cosmetic.

Still open from round 1: the stack claim

The body still reads "Layer 3 of 4. Base: #872. Depends on #872." This PR targets main, carries one commit, and contains neither #871's nor #872's work — it is standalone, and I found no import or file overlap creating a real dependency. The base is right; the body is stale. Not blocking, but worth a two-word edit before the queue so nobody sequences on a dependency that isn't there.

Verdict

Action: APPROVE

The heuristic is conservative in the ways that matter (5-skill floor, top-score-only buried, stable tie-break), and reorderSkills survives the three interactions that would reopen the round-1 bug — delete-before-Apply, Undo, and delete-after-Apply.

Reviewed by: Claude Opus 5 (1M context)

[s-annam]

Addressed the round-2 "one consequence worth recording" note in 4aeb29e.

The suggested fix (require one token to be a full prefix of the other, capped by length delta) correctly blocked java/javascript but also cost the three legitimate stem pairs the table called out — developer/development, analytics/analytical, engineers/engineering — since none of those are a full-prefix relationship; they diverge on both sides of a shared root.

Replaced it with a common-prefix-length bound: looselyMatches now finds the longest shared prefix and requires it to be ≥4 chars, then caps each token's divergence from that shared stem at 4 chars (rather than capping the difference between the two token lengths). Verified by hand against the full table from the review:

pair	old (round 1)	round-2 fix	this fix
java / javascript	✅ (bug)	❌	❌
engineer / engineering	✅	✅	✅
design / designer	✅	✅	✅
data / database	✅	✅	✅
developer / development	✅	❌	✅
analytics / analytical	✅	❌	✅
engineers / engineering	✅	❌	✅

Added a test (still matches stem pairs that diverge on both sides of a shared root) pinning the three restored pairs, alongside the existing java/javascript negative test. npm run typecheck, npm run test (6010 passed) both green.

[Samhit21]

PR Review (round 3): feat(critique): surface skills-ordering guidance

Re-approving — my previous approval was auto-dismissed by the push to 3faf2d5.

Summary

This is a better rule than the one I suggested, and it resolves the tradeoff I'd written off as acceptable rather than fixable. Bounding the divergence on both sides of a shared stem, instead of requiring one token to be a prefix of the other, recovers exactly the pairs the narrower shape lost — because in every one of them neither token is a prefix of the other (develop + er/ment, analytic + s/al, engineer + s/ing). I ran all three rule versions side by side:

pair	4-char prefix (orig)	prefix-of (r2)	common-stem (r3)
engineer / engineering	✅	✅	✅
design / designer	✅	✅	✅
data / database	✅	✅	✅
developer / development	✅	❌	✅
analytics / analytical	✅	❌	✅
engineers / engineering	✅	❌	✅
java / javascript	❌ (the bug)	✅	✅
communication / community	❌	✅	✅

(✅ in the last two rows means "correctly rejected".) So r3 is strictly better than both predecessors: it keeps every true stem pair the original caught, and rejects communication/community as well, which the original matched and nobody had flagged.

The test is well-built too — placing the stem-pair skill at index 3 of a 5-skill list puts it past the front window, so buried is asserted rather than merely defined, and the comment records that the prefix-of shape was tried and why it was rejected. That is the part that keeps someone from "simplifying" it back in six months.

Verified by running it

• skills-order.test.ts, useSkillsReorder.test.tsx, SkillTermGuidance.test.tsx — 27 passed
• npm run typecheck clean; npx eslint src/lib/heuristics/skills-order.ts clean
• Re-derived the rule independently in a scratch script against all three versions (table above)

Still open, non-blocking

The body still reads "Layer 3 of 4. Base: #872. Depends on #872." This PR targets main, carries one commit, and contains neither #871's nor #872's work — I checked for an import or file overlap creating a real dependency and found none. The base is correct; the body is stale. A two-word edit before the queue so nobody sequences on a dependency that isn't there.

Verdict

Action: APPROVE

Reviewed by: Claude Opus 5 (1M context)