fix(learnings-search): rank by whole-word relevance so an exact match cannot be truncated away (#2762) - #2799
Draft
szsunyuan wants to merge 7 commits into
Draft
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
…aming (garrytan#2762) `--query` gets worse as the query gets more specific. The filter is token-OR over substrings, so every word admits its own set of entries; the union is then ranked by confidence alone and cut to --limit (10 by default). Nothing errors and nothing comes back empty, so a caller searching for something specific receives ten confident, well-formed, unrelated entries and reads them as a complete answer. That is a false absence, which is the dangerous direction to fail in. Recall and relevance are now answered separately. RECALL is unchanged: substring containment across key, insight and files. Every entry that matched before still matches -- verified set-identical to the base binary across 72 query-and-store combinations, with the no-query and --type paths byte-identical across 40 more. RELEVANCE is new, and is counted over DISTINCT query tokens found as WHOLE WORDS. It has two tiers, because the two obvious single-tier designs each fail: - Scoring key, insight and files together lets verbosity beat quality. Keys and file paths are long and descriptive by convention, so a thin entry whose name or path happens to carry the query words outranks an insight that answers it. - Scoring only the insight overcorrects. An entry named exactly after the query then scores zero and loses to incidental prose, which is the same truncation the fix exists to prevent. So what an entry SAYS decides first, and what it is ABOUT (key and file paths) only orders entries that already tie on substance. Confidence breaks a remaining tie, then recency. Whole-word matching is what makes a hit mean the concept is present: a recall net that accepts 'bug' inside 'debug' or 'cause' inside 'because' is right for finding candidates and useless for ordering them. Word characters are identified by case-folding rather than an ASCII range, so accented letters do not fake a word boundary while scripts without case keep an embedded Latin term findable. Internal ranking fields are stripped from every row at parse time. They live in an underscore namespace and gstack-learnings-log re-serializes whatever it is handed, so without the strip a stored _field would arrive as a live sort key on the gstack-skill-start --limit 3 call that runs in every session. Stripping the namespace once covers fields added later, rather than relying on each new one to remember to initialize itself. When a query matched more than the limit shows, the summary line states the fraction once ('LEARNINGS: 10 of 13 matched'), following the phrasing already used in gstack-retro-metrics. It is gated on a non-empty query because the preamble call must not grow a line, and written to stdout because this block redirects its own stderr to /dev/null. Every behavioural claim above is pinned by a test that fails when the corresponding line is reverted; all nine such mutants were verified to die. Fixes garrytan#2762 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GgbYViR1AYn2RqfZaTYd7o
szsunyuan
force-pushed
the
fix/learnings-search-token-hit-ranking
branch
from
September 4, 2026 14:32
4c84d6f to
2233f24
Compare
…utes on Windows The suite invoked the binary directly. gstack-learnings-search is a shebang bash script, and Windows has no shebang handling, so every test in this file died at `execFileSync` with "Executable not found in $PATH" before reaching an assertion -- all 22 of them, including the 16 this file gained for garrytan#2762. They did not fail visibly on a green machine; they failed on the platform nobody was reading the log for, so the ranking behaviour this file exists to pin was unverified there. The file is not in the windows-free-tests.yml curated lane either, so nothing else was covering it. Passing the script to `bash` explicitly is the pattern already used by browse/test/learnings-injection.test.ts for the same reason, and it is a no-op on Linux and macOS where the shebang would have been honoured anyway. All 22 pre-existing tests pass unchanged once they can run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…coring isWordChar counted `_` as a word character, so a snake_case key was one long word and matched none of its parts. The function's own comment says hyphens, dots and slashes are separators "so hyphenated keys and file paths yield their individual words" -- underscore belongs in that list on the same reasoning. It is not a rare shape. gstack-learnings-log's key regex is /^[a-zA-Z0-9_-]+$/, so snake_case keys are supported on the write side, and file paths carry underscores constantly. The result was the same entry ranked on its separator rather than its content: with a store of thirteen rows and the query "preflight project line", the key preflight-project-line ranks 1st and preflight_project_line ranks 13th -- off the page at the default --limit 10, which is the truncation garrytan#2762 exists to close. Recall is untouched: the filter matches substrings and never consulted isWordChar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…flections Whole-word scoring matched the exact form only, so a token was blind to the form prose actually uses. English inflects and the shipped queries do not: "test" could not see "tests", "merge" could not see "merged", "fix" could not see "fixes". That reintroduced the very truncation this feature exists to close, one tier down. Under the shipped `--query "<keyword>" --limit 5` shape used by /investigate, /qa and ship's adversarial section, an entry whose insight opens "Tests interleave when the runner is parallel" scores ZERO for the query "test", while six unrelated rows each score one naming hit for carrying a ui/panelN.test.ts path. The answer is pushed off a five-row page by six filenames -- an exact match truncated away, which is the failure this change was written to prevent. A token now also scores against its regular inflections. The list holds only suffixes that are true suffixes of the base form; 'flaky' is not flake+y and 'policies' is not policy+ies, so reaching those needs stem rewriting rather than a longer list, and adding them bare would widen the false-positive surface while matching neither. Over-reach is bounded by the same word-boundary check that already applied: 'orbital' still scores nothing for 'orbit'. Recall is unchanged -- the filter never consulted this function -- and the no-query path stays byte-identical, both re-verified against the base binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hole store
The formatter groups rows into a plain `{}` keyed by each row's own `type`. When a
row's type names a property every object inherits -- constructor, toString, valueOf,
hasOwnProperty, __proto__ -- the truthiness guard sees the inherited value, the
array is never created, and .push throws on a function.
The bun stage ends `2>/dev/null || exit 0`, so the throw becomes empty stdout at
exit 0. The caller cannot distinguish it from "this project has no learnings": one
malformed row silently blanks every other row in the store, on every query.
The object is pre-existing, but relevance ranking is what makes it reachable. The
malformed row used to sit wherever confidence put it, usually below the --limit cut
that hid it; scoring floats it onto the page. On a store of ten confident rows plus
one row typed "constructor", the query that matches them all returns ten rows
before this change and zero after it -- so the containment belongs with the ranking
that exposed it.
Object.create(null) has no inherited properties, so a type only collides with a
type. Verified against all five reachable poison types.
gstack-learnings-log rejects these types on write, so a row like this arrives by
hand-edit, an older writer, another tool, or a cross-project store -- not remotely
exploitable, but a total-silence path either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ance ranking Every row in the garrytan#2762 fixture is `user-stated`, which is exempt from confidence decay, and the diff contained no `observed` or `inferred` row anywhere. That was deliberate -- a decaying fixture drifts with wall clock and turns into a date-dependent flake -- but it left the interaction between decay and the new sort order completely uncovered. The interaction is worth stating out loud, because relevance now sorts ahead of confidence and decay only feeds confidence: a row that has decayed to zero still outranks a current row when it matches more of the query. That follows from the ordering garrytan#2762 asked for, so these tests pin it as intended behaviour rather than assert against it -- and the second test shows decay is still live underneath, demoting the stale row as soon as the hit counts tie. Both fixtures are dated far enough back that Math.max pins them to the 0 floor, so they are stable forever instead of drifting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`e._tokenHits = 0` was never read. The sort reads _insightHits and _contextHits, which this line does not initialize, and no other reader exists. Its six-line comment is the reason to remove it rather than leave it: it credits the line with preventing a stored _field from becoming a live sort key on the no-query preamble call. That defense is real, but it is provided by the underscore-namespace strip at parse time, which runs on every row on every path. A maintainer trusting the comment could delete the strip and keep this line, believing the control was still in place -- the test planting _insightHits, _contextHits and a name that does not exist yet still passes with the line gone and fails with the strip gone. Removing it leaves output byte-identical on every path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why (in your own words)
If you search your learnings store for something specific, gstack can tell you the thing isn't there when it is.
gstack-learnings-search --queryfilters with token-OR over substrings, so every word admits its own set of entries, and the union is then ranked by confidence alone and cut to--limit(10 by default). Nothing errors, nothing comes back empty — you get ten confident, well-formed entries that simply aren't the one you asked for, and you read that as a complete answer. The search gets worse the more precisely you describe what you want: adding a discriminating word widens the candidate set, and the entry matching every word you typed can be pushed off the end by entries matching just one.That's a false absence, which is the bad direction to fail in. A caller who already has reason to believe a learning exists gets a plausible list back and concludes it doesn't.
What this changes
Recall and relevance were the same computation. They are now two questions answered separately.
Recall is unchanged. The filter predicate is still substring containment across key, insight and files. Every entry that used to come back still comes back — verified set-identical to the base binary across nine query shapes on a real 95-entry store, and byte-identical on the no-query and
--typepaths.Relevance is new, and deliberately stricter: the number of distinct query tokens present as whole words, counted in two tiers. What an entry says (its insight) decides first; what it is about (its key and file paths) only orders entries that already tie on substance. Confidence breaks a remaining tie, then recency.
Four properties of that definition carry the weight, and each exists because the looser version ranks badly on gstack's own shipped queries:
buginsidedebug,causeinsidebecause, andfixinsidefixtureis right for finding candidates and useless for ordering them./investigateships the querydebug investigation root cause hypothesis bug fix; under substring scoring, prose that merely says "debug output ran because the fixture was parallel" collects three free hits and outranks a real insight about root causes.testcould not seetests,mergecould not seemerged. A token now also scores against-s -es -d -ed -ing. The list holds only suffixes that are true suffixes of the base form (flakyis notflake+y), so reaching further would take stem rewriting rather than a longer list, and over-reach is bounded by the same word-boundary check that already applied:orbitalstill scores nothing fororbit.Word boundaries are found by case-folding rather than an ASCII range, so accented letters do not fake a boundary while scripts without case keep an embedded Latin term findable. Hyphens, dots, slashes and underscores all separate, so a
snake_casekey yields its individual words exactly as akebab-caseone does —gstack-learnings-log's key regex admits_, and file paths carry it constantly.The score is initialized in the parse loop beside
_effectiveConfidenceand_crossProject, never inside the query filter. The filter runs only when a query was given, so a score assigned there would leave the no-query path reading whateverJSON.parseproduced — andbin/gstack-learnings-logre-serializes unknown keys, so a stored value would reach the sort on thegstack-skill-start --limit 3call that runs in every session. Every underscore-prefixed key is stripped at parse time, which immunizes fields added later rather than relying on each new one to remember to initialize itself.When a query matched more than the limit shows, the summary line now states the part and the whole, following the phrasing already used in
bin/gstack-retro-metrics(showing 300 of %d). It is gated on a non-empty query because that preamble call runs every session and must not grow a line, and written to stdout because the block redirects its own stderr to/dev/null.One pre-existing defect is fixed here because this change is what exposes it. The formatter groups rows into a plain
{}keyed by each row's owntype. A row whose type names an inheritedObjectproperty —constructor,toString,valueOf,hasOwnProperty,__proto__— finds that key already truthy, never builds the array, and throws on.push. The block ends2>/dev/null || exit 0, so the throw becomes empty stdout at exit 0: one malformed row silently blanking every other row in the store, on every query, indistinguishable from "this project has no learnings". Relevance ranking is what makes it reachable, floating that row past the--limitcut that used to hide it. The map is now prototype-free.Live evidence
Self-contained reproduction — one target entry whose key and insight contain all three query tokens at confidence 8, plus twelve confidence-10 decoys that contain
lineonly as a substring (guideline, pipeline, deadline, …):Before —
upstream/mainat0d1bd561. The key contains all three tokens, in that order, and the third query is the one that loses it:After — this branch:
The shipped single-keyword shape.
--query "<keyword>" --limit 5is whatinvestigate/SKILL.md,qa/SKILL.mdandship/sections/adversarial.mdactually run. Store: one answer at confidence 6 whose insight opens "Tests interleave when the runner is parallel", plus six confidence-10 rows whose only claim on the word is aui/panelN.test.tspath:Confidence-only ranking loses it, and so does scoring the exact form only — the insight says tests and the query says test. Matching a word together with its regular inflections is what finds it.
Same entry, different separator. Thirteen-row store,
--query "preflight project line":A malformed row can no longer blank the store. One row typed
constructoramong ten healthy rows:Verified across all five reachable poison types.
Recall is untouched. Per-token match counts on a real 95-entry store, base vs branch:
Set-identity holds for whole queries too — the matched key sets are identical across nine shapes including all three shipped multi-token queries. The always-on preamble path is byte-identical:
Thirty-two tests, up from six on
main. Every behavioural claim is pinned by a test that dies when the corresponding line is reverted — each mutant applied in place and the suite re-run:Every commit in the series passes on its own, so the history is genuinely bisectable:
Full free suite:
What this deliberately does not do
It does not narrow recall. Substring matching still admits
prinside provider, reports, and approach, which is why one broad token can pull in half a store. Narrowing the filter would change which entries are findable at all, for every query in every skill; that is a semantics decision deserving its own issue, and this change is careful to leave it alone. What changes is only that such an entry no longer outranks a real match.The truncation notice is gated on a query, so a
--type-only truncation stays silent.--type pitfall --limit 5with no--querycan still truncate without saying so. No shipped call site does that today, and widening the gate would risk the no-query preamble line, so I left it.The formatter still groups by type, so the global rank order is visible within a type group rather than across the whole list. The limit is applied to the globally sorted list, so the exact match is no longer truncated away; but with mixed types it may not print first. Changing the output shape is a bigger call than this fix.
Relevance sorts ahead of confidence, and decay only feeds confidence. A row that has decayed to zero therefore still outranks a current row when it matches more of the query. That follows directly from the ordering #2762 asks for, so it is pinned as intended behaviour rather than asserted against, with a companion test showing decay still demotes the stale row as soon as the hit counts tie. Flagging it because it is a real consequence worth agreeing on, not a side effect to discover later.
Two pre-existing defects were found and left alone, both verified identical on
mainand neither caused by this change:Bun.stdin.text()throws above exactly 65536 bytes and the same|| exit 0turns it into the same false-absence signature — so a store large enough to need the new truncation notice returns nothing at all there, and--cross-projectcrosses that threshold much sooner. The fix is different plumbing (spool and redirect rather than pipe), not ranking, so it wants its own issue.--cross-projecta foreign store whose rows restate the query can outrank local ones.mainhas no locality key either — this adds a second lever rather than the first — but whether locality should be structural is a product call, not a quiet one-liner here.Diagnosis, mechanism breakdown, and both implementation traps — gate the notice on a non-empty query, and write it to stdout — are the issue author's from #2762.
Scope
Changed:
bin/gstack-learnings-search(+109/-5),test/gstack-learnings-search.test.ts(+401/-2). NoVERSION, noCHANGELOG, no generated files, no templates.Verified live by: the temp-fixture reproduction above against both the base binary and this branch; matched-key set identity across nine query shapes on a real 95-entry store; byte-identical output on the no-query and
--typepaths; thirty-two tests, with every behavioural claim mutation-verified by reverting the line it pins and confirming the test dies; each commit in the series run independently green; the full free suite green on all six shards (8,685 tests, exit 0);bin/gstack-redactclean on both files;bun run slop:diff upstream/mainreporting no new findings.bash -nclean, and no$, backtick, backslash or double-quote character was added anywhere inside the double-quotedbun -eblock.Did NOT test: CI itself — fork PRs don't receive eval secrets, and this is a deterministic bash/JS change with no model in the path, so no paid evals were run. The 95-entry recall measurement came from one real store on one machine, so treat the per-token counts as one data point rather than a distribution.
scripts/free-test-durations.jsonstill records the pre-existing 282 ms for this file; the added cases make it slower, and I left the shared durations seed alone rather than fold an unrelated regeneration into this diff.On Windows: the suite in this file previously could not execute at all —
gstack-learnings-searchis a shebang script and Windows has no shebang handling, so all 22 tests died atexecFileSyncwith "Executable not found in $PATH" before reaching an assertion. They now run (0 pass / 22 fail→32 pass / 0 fail, verified locally on Windows 10) via the explicitbashinvocation already used bybrowse/test/learnings-injection.test.ts, which is a no-op where the shebang would have been honoured anyway. This does not change CI coverage:scripts/test-free-shards.tsexcludes the file from the windows lane by scanning its content forpath.join(ROOT, 'bin', …), and that pattern still matches regardless of whether the invocation is now correct. Making the heuristic reflect actual runnability is a change to the shared runner and belongs in its own PR.Liveness proof (required)
GSTACK PRtyped live into a real surface (not edited onto the image)Checklist
GSTACK PRtyped live into a real surface (not edited onto the image)Fixes #2762