Fix/auto connect clear context - #90
tanglearncode merged 5 commits into
Conversation
tanglearncode
left a comment
There was a problem hiding this comment.
Code review
The feature itself is sound — get_context is already the session asking the plugin to route, so completing a clear connection there rather than hoping the model issues a second use_context is the right call. The problems are in how clear is defined and in what happens when the write fails.
Three separate paths let a single word through the two-term gate. matched.length >= 2 reads as "two independent query terms agreed," but it isn't:
- a one-token alias short-circuits the gate entirely via
exact; tokenize()expands one hyphenated/underscored run into run+parts and one CJK run into chars+bigrams, so"checkout-api"alone producesmatched.length3;- two incidental hits in the lowest-weight
filesfield count the same as two hits in the hand-written description, even thoughFIELD_WEIGHTSratesfilesat 1 vsaliasesat 5 precisely because filename hits are weak evidence.
Each of these was reproduced against a temp home. Combined with silent re-grounding and extension-server spawning, one stray word can move a session into the wrong context and launch a user process off a BM25 hit.
The one I'd fix before anything else is the unguarded write in autoConnectClearMatch. If applySelection throws, main()'s empty .catch eats the rejection and no JSON-RPC reply is ever written, so get_context hangs forever rather than degrading. On main the same environment still answers. That turns the plugin's only grounding tool from degraded to dead.
On the tests: two of the changes here removed coverage rather than extending it — the shortlist test was flipped to ask so auto-connect wouldn't fire, and both regression guards in routing-unconnected.test.mjs were deleted rather than relocated. That is why the tokenize and near-tie gaps pass CI.
On placement: isConfidentMatch and autoConnectClearMatch are host-agnostic and sit only in the Copilot bridge, while claude-code, kimi, codex and pi keep the old behavior against the same shared ~/.neatcontext. Same query, two different routing behaviors, one shared decision log.
Details inline. Findings are ordered roughly by severity; the first six are the ones I'd want resolved before merge.
🤖 Generated with Claude Code
Review found the clear-match path could connect on evidence far weaker than the menu it replaced: one common term, a filename hit, or a short alias found inside an unrelated sentence. Add an absolute floor beside `assess`, which only ever judged the leader relative to the field and so always said "clear" for a single-context store. A match now needs two agreeing terms the user actually typed, counted as words rather than as index tokens, and terms landing only in `files` do not count. An exact name, or an alias the user wrote, still stands in for the floor -- an alias of one word has to be the whole request, and a longer one has to survive tokenizing as two words and appear contiguously, so `the api` cannot route every sentence containing `api`. Also: - rank, boost, then slice, so decline and familiarity multipliers can promote a context into the visible set instead of being applied to a slice that was already cut - exclude the bridge's own automatic connections from familiarity, so routing cannot teach itself a preference the user never expressed - carry the near-tie note into the menu the bridge renders when it declines - make one pass over the store per call and thread it through, rather than re-reading the selection, the listing and the routing state up to three times for one answer - require a host-published session id, so a keyword hit in one window cannot re-ground a conversation in another - guard the whole ranking and persistence tail: an auto-connection that cannot be made is a missed optimization, and unguarded it left every `get_context` in the session unanswered
|
Thanks for the thorough pass — all 15 are addressed in the latest push. Two places where I did something other than what was suggested, both flagged below. The root problem you found.
Deviation 1 — Deviation 2 — the rank limit. I passed the explicit limit you asked for, and also fixed the cause in Staged rollout. Only the Copilot bridge calls Also in this push: the near-tie note now reaches the menu the bridge renders when it declines; the bridge makes one pass over the store per call instead of re-reading the selection, listing and routing state up to three times; auto-connect requires a host-published session id, so a keyword hit in one window cannot re-ground a conversation in another; and the entire ranking and persistence tail is guarded — unguarded, a home this process could not write to left every The two regression tests I had edited away are restored as their own cases rather than folded into the ones I changed, since that is exactly the mistake: I adjusted the tests that were guarding the faults instead of keeping them.
|
tanglearncode
left a comment
There was a problem hiding this comment.
Second review — 3219921f
All fifteen findings from the previous review are fixed, and fixed at the right depth rather than patched at the call site. The gate moved into shared/core/routing-candidates.mjs and is vendored byte-identically to all six plugin copies; matchedFields was threaded through rank() to make the field distinction possible at all; the writes are guarded; both deleted regression guards are back with homes that still run. routing-confidence, routing-unconnected and copilot-plugin pass 89/89, which I ran.
Two of the fixes are better than what I suggested. On the decision log I proposed keying off requested: false — correctly rejected, since a model calling use_context in auto mode records that too and it was still somebody's decision, hence the separate automatic flag. On the alias floor you caught a follow-on I missed: the api is two words the user typed but tokenizes to one after stopword removal, so a token-count check alone reopens the bypass. Both floors now hold, with tests for the api, our pr, INC-1001, user_id.
What is new
Fourteen findings below. Three were verified by running the code against this head:
- The two-term floor is still bypassable.
agreeingTermscounts distinct query words, and one concept written two ways in the same request supplies two of them. A leader matching onlyuserconnects on "what does user_id mean for a user?". Counting distinct carried tokens instead closes it — small change, same class of hole as the one this commit set out to fix, which is why I'd want it resolved before merge. - Auto-connect is unreachable for CJK.
queryTermssplits on whitespace, so a Chinese or Japanese request is always exactly one term. Aliases cannot rescue it either, sincewordCountis 1 for CJK and that forces whole-query equality. The reasoning behind the change is right — 7 tokens off 4 characters is not 7 agreeing terms — but whitespace runs are the wrong unit for scripts that do not use whitespace, andtokenize()already has dedicated CJK bigram support to build on. - The decision-log fix is half-applied. The read-time filter closes the feedback loop, but automatic entries still occupy the same log capped at
MAX_DECISIONS = 100, so they evict the manual decisions the filter exists to preserve.
The rest are source-verified but not reproduced, and are individually narrower than the originals — mostly places where a fix landed one level shallower than the problem, or where the new single-pass refactor left two states that can disagree.
Comments on shared/core/* apply to all six vendored copies.
🤖 Generated with Claude Code
Fourteen findings, all of them fair. The two that mattered most were the same bug seen from two sides: a single compound word carried enough tokens to clear the agreement floor on its own, and a run of Chinese carried far more. Counting distinct carried tokens does not fix it -- checkout-api carries three by itself. Agreement has to be independent on both sides, so agreeingTerms now computes a maximum matching between the parts of the request and the things they agreed on. Exact rather than greedy, because greedy is order-dependent and rewording a sentence must not change where it routes. A no-space script is one part of the request however long it is, so it can supply at most one pairing and can never reach the floor. That is a real limitation and it is documented and tested rather than papered over: Chinese and kanji-dense Japanese need an exact name match or a whole-request alias to connect unasked. Korean is written with spaces and is unaffected. The decision log could be emptied of the user's own routes by the machine's: automatic entries now cap separately at 20 and merge back in time order, so familiarity still sees what the user actually chose. On the bridge, the routing pass is now the single source for the whole call. Ranking happens once and the shortlist slices it, ties are assessed whether or not auto-connect was ever eligible, a stale selection reads as nothing connected, the connection is recorded before the log that describes it, a cross-session refusal disqualifies rather than discounts, sessionId() is inside the guard that keeps a dead working directory from silencing every answer, and an auto-connection stops the previous context's extensions the way an explicit switch does. Every fix above is pinned by a test that fails without it.
|
Thanks — all fourteen were fair, and all fourteen are fixed in The agreement floor (1 and 2)Counting distinct carried tokens does not close it. That is a maximum bipartite matching, so Your CJK case falls out of the same formulation instead of needing a rule of its own: a no-space run is one part of the request however long it is, so it can supply at most one pairing and can never reach 2. I have taken the option you blessed — documented and tested, not papered over — with one correction: the limitation is no-space scripts, not CJK. Korean separates eojeol with whitespace and routes normally today ( Everything else
On verificationEvery fix above is pinned by a test that I confirmed fails when that specific fix is reverted — I reverted each one individually and checked, rather than trusting that a passing suite means anything. 537 tests pass and all 539 changed lines are covered. Round one I read your comments as a list to work through. That was the mistake, and it is why round two found fourteen more: the bipartite-matching bug and the CJK bug were one bug, and I would have seen that if I had looked for what the findings had in common instead of fixing them one at a time. Thanks for the second pass. |
tanglearncode
left a comment
There was a problem hiding this comment.
Third review — 8f63e5d
Twelve of the fourteen are fixed, and fixed where the problem was rather than at the call site. sessionId() is inside the guard; the extension teardown test reads the child's pid and asserts it is gone, which is the right way to test it; a stale selection reads as nothing connected everywhere; the connection is recorded before the log that describes it; the shortlist slices the pass's ranking instead of running BM25 a second time; the second resolution path is gone and activeContext is synchronous. npm run check passes, including the six-copy byte-identity sweep, and npm test is 537/537, which I ran.
The Korean call is correct, and correcting my "CJK" to "no-space scripts" was the right narrowing. I checked it against a real index rather than the unit fixtures: 주문 지연 connects, 订单延迟 declines. The limitation is documented accurately.
The agreement floor is still open, and the tests do not reach it
The maximum matching is the right idea and the argument for exact over greedy is correct — order-dependence in routing would be its own bug. The problem is not the algorithm, it is what the algorithm is fed.
agreeingTerms reads candidate.matched, which the bridge fills from rank(). When the document carries the compound word — the ordinary case, because that is why the request mentions it — the index carries the run and its parts, so matched carries all of them, and the two spellings pair against two different tokens.
Run against the real buildIndex/rank at this head:
| context description | query | matched |
result |
|---|---|---|---|
"How the user_id column is populated." |
"what does user_id mean for a user?" | [user_id, user, id] |
connects |
"Everything about the checkout-api service." |
"is the api part of checkout-api?" | [api, checkout-api, checkout] |
connects |
"How we run services under docker-compose." |
"how do I run docker in docker-compose" | [run, docker, docker-compose, compose] |
connects |
And end to end, one-context store, ungrounded session, real Copilot bridge:
> get_context {"query": "what does user_id mean for a user?"}
Automatically connected "Identity" for this request.
That is the query the comment above agreeingTerms names as the thing that must not pass.
The unit tests pass because candidate("Users", hit("user")) produces matched: ["user"] — a leader that matched user and not user_id, which needs a description that says user and never says user_id while the user's request does say it. They do fail when the matching is reverted, so they are honest regression guards for that fixture. They just do not cover the fixture rank() produces. Details inline.
Three more, one of them a regression this PR introduces
The session-log one is the same shape as the decision-log finding you just fixed, one file over: capDecisions bucketed the decision log so machine routes could not evict the user's, and the session log — which holds the per-session mode override and the declined list — got no equivalent, while this PR gives it a new writer that fires once per session.
On my own last round
Two corrections to make, both mine.
The cross-session decline: I asked for disqualification rather than a discount and that is what landed, but declineFactor(...) < 1 holds for the full DECLINE_LIFETIME_DAYS = 42, which is past the point the constant's own comment calls the discount negligible. That is my suggestion landing broader than I meant it, and it is worth calibrating rather than leaving as written.
And on tooling: a second reviewer flagged the unguarded sessionId() in withRoutingTools as this PR reintroducing the original hang on the tools/list path. That is wrong and I am not raising it here — withRoutingTools is byte-identical on main and this PR only touched its call site. It is a real hazard and I will open it separately; it is not yours to answer in this PR.
Comments on shared/core/* apply to all six vendored copies.
🤖 Generated with Claude Code
Eight findings from the maintainer's third pass. The confidence floor counted a compound and the parts `tokenize` derived from it as separate evidence. `rank` returns every token the index holds, so a description containing `user_id` carries `user_id`, `user` and `id`, and a request naming that one concept twice cleared a floor of two. The carried set now collapses by derivation before pairing, which closes it without touching the matching itself. The tests that guarded this were fixtured with a shape `rank` never produces, so they are re-fixtured, and a new case runs the whole rule against a real index. An automatic decision no longer creates a session record. Records hold the per-session mode override and what the user declined there, and every writer used to need a person or a model; auto-connect fires about once per new session, so it turned the session cap into a shredder — twenty windows connecting themselves elsewhere evicted a window pinned to manual and it silently started routing again. The near-tie note is assessed over the shortlist it is printed beneath rather than the whole corpus, so it can no longer name contexts the model was never shown. A host that publishes no session id now says so, instead of going quiet in a way that reads as "nothing matched". Manual mode stops ranking for a list neither renderer will read. A refusal bars connecting unasked for one half-life, named, rather than for as long as `declineFactor` stays under 1 — six weeks, by the end of which it is a one-percent discount. A decision whose timestamp will not parse stays where it was instead of being relocated to 1970 on every write. The unwritable-routing-file test used `chmod 0o444`, which denies nothing to root and only sets an attribute on Windows, and it matched on a card stored in the file it was destroying. It now uses a directory and matches on the name.
tanglearncode
left a comment
There was a problem hiding this comment.
Fourth review — 1d6dc92 — approving
All eight are fixed, and the two you pushed back on you were right about. Approving with two notes, one of which is the follow-up you already offered to split out.
The floor
The diagnosis was mine and the fix is better than the one I proposed. I reproduced your table: rows 1b and 3 now decline through the real index, and the controls hold.
I also ran your objection to union-by-shared-derived-token, and it is correct — alpha alpha-beta and alpha-beta alpha both collapse to one group under it, so it would have taken it("pairs the same way whichever order the words arrive in") from true to false in both directions. Order-independently wrong is the right description. Collapsing carried rather than the request is the side that was actually non-independent, and it is the smaller change.
I checked the confluence claim rather than taking the argument for it: 400 random permutations of five token pools, including the CJK bigram set and a mixed compound-plus-unrelated-word set, and the collapsed result is identical every time. The argument holds for the reason you gave — a delimiter-free part tokenizes to itself, so deletion only ever runs compound → part and can never cycle.
And you are right about row 2. I put it in the table as a third demonstration and it was not one: run is in the description and in the request, it is not a stopword, and rank returns it. Dropping it from the query flips the same context to false, which is the test you added. My row 2 was a bad row and the two-word rule was doing exactly what it says.
Re-fixturing
it("holds against \matched` as a real index fills it")is the part I care about most in this commit. Building the index from a description and passingisConfidentMatchwhateverrank` returned is what makes this class of fixture unable to hide the next one, and it is worth more than any single assertion in it.
The session log
Verified rather than read: forty automatic decisions against a window pinned to manual with a declined list, and the record survives, resolveMode still returns manual, declined is intact, and zero session records were created by the machine. Preferring "update but never create" to a second bucket is the right call and your reasoning for it — that a record which has only ever auto-connected holds nothing anybody reads — is the argument I would not have made and should have.
The rest
assess(shortlist) for the renderer with pass.decision kept for the gate is the right split, and your note that the verdict provably cannot move is correct: the shortlist is a prefix of the same ranking, so the leader's score is the same and the filter is the same filter over a prefix.
The shared-window branch says the thing that was missing, and softening the tool description to "can safely auto-connect" was the right accompanying change — I had flagged the sentence and not the mood. Thank you for actually checking a live Copilot CLI process rather than guessing at it; "present on 1.0.79, unknown on older surfaces, and the fallback now announces itself" is exactly the shape that answer should have.
hasLiveDecline at one half-life, named against the existing constant instead of a new number, is better than the floor I suggested. timeKeys/mergeByTime is the right instrument — one note on it inline.
On the manual-mode test: your call, and I agree with it. An equivalence guard is the honest test for a change whose entire justification is that the output is identical, and I would not add a rankContexts seam to the bridge for it either. Saying so in the comment is enough.
npm run check passes including the six-copy sweep, and npm test is 546/546, which I ran.
Two notes, neither blocking
The timestamp one is a narrow residual of the fix, inline. The other is the field-based floor you offered to open separately — I have a demonstration for it now and I still think separately is right; details inline so it does not get lost.
🤖 Generated with Claude Code
|
LGTM |
Summary
get_contextwhen the session is ungrounded and routing mode isautoValidation
npm run checknpm testnpm run coverage