feat(vnext): notify on setup policy, and print one tool list in setup - #19
Merged
Merged
Conversation
Ports gleanwork/glean-plugins-vnext#54 and #55. #54 -- the tools/list_changed notification was suppressed whenever the label said "tools/list", on the reasoning that there the response IS the update: the host asked and is about to receive the filtered surface, so notifying would make it ask again and notify -> list -> resolve -> notify is a cycle. The reasoning holds; the label does not identify the case. `setup` fetches the catalog through the same fetchAllowedRemoteTools helper and so carries the same label, while the host is receiving setup's TEXT. A policy that changed the reachable surface during setup therefore left the host holding a stale list with nothing to prompt a refresh. Observed against a real remote rather than reasoned about: an experimental QE pod flipped toolPromotion false -> true, setup picked it up and the plugin would have executed the promoted tools, but they stayed invisible until a later unrelated list. Suppression now takes an explicit hostReceivingList flag, passed by the tools/list handler alone; setup and every tools/call take the default and notify. Defaulting to notify is the safe direction -- a new call site has to opt into silence rather than inherit it. #55 -- setup printed the remote's whole catalog ("Remote tools: search, chat, ...") and then closed with "You can now use find_skills_and_tools, run_tool". The two disagreed the moment policy withheld anything, and the closing pointed at the catalog with "any of the listed remote tools", so a model was handed names it may call that were neither advertised nor callable. The catalog is gone and setupClosingLine() in enforce.ts is the single authoritative list, scoped to naming tools: deactivation status and the remote's upgrade message stay with policySummary(), which prints them a few lines above. It needs no deactivation branch, because evaluate() already reports every feature as false when deactivated. Port notes. session.ts and remote-passthrough.ts were identical to vnext and copy over whole. enforce.ts differed only by META_TOOL_NAMES naming find_skills_and_tools, and setupClosingLine derives its names from that set rather than repeating them, so the function ports unchanged. The test files differed from vnext by exactly two substitutions, established by diffing before porting: the tool name, and `/plugin update glean-vnext` -> `/plugin update glean`. tests/setup-output.test.ts is new. It asserts the ASSEMBLED setup text, because the duplication both PRs touch was a property of policySummary() and the closing together and no unit test on either half could see it. Mocking version.js is what makes the deactivation lines reachable under vitest at all -- deactivation is gated on version provenance. Verified here, not just upstream: typecheck, 333 tests, check:no-shell, build, and validate on all three targets. Mutation-checked in this repo -- restoring the label check fails exactly the setup-notification test, and ignoring toolPromotion fails six across both the unit and composition layers.
eshwar-sundar-glean
approved these changes
Aug 20, 2026
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.
Ports glean-plugins-vnext#54 and #55, both merged upstream.
#54 — notify the host when
setuplearns a surface-changing policyThe
tools/list_changednotification was suppressed whenever the label saidtools/list. The reasoning was sound — there the response is the update, so notifying would only make the host ask again, andnotify → list → resolve → notifyis a cycle — but the label does not identify that case.setupfetches the catalog through the samefetchAllowedRemoteToolshelper, so it records policy under the same label, while the host is receiving setup's text.tools/listhandlertools/listhostReceivingList: truesetuptools/listtools/calltools/callFound against a real server, not by reading code: an experimental QE pod flipped
toolPromotionfalse → true,setuppicked the new policy up — the plugin would have executed the promoted tools — but they stayed invisible to the host until a later unrelatedtools/list. Enforcement was never wrong; the call-time gate tracked it immediately. Defaulting to notify is the safe direction: a new call site has to opt into silence rather than inherit it.#55 — one authoritative tool list in
setupsetupprinted the remote's catalog and then closed with a different list:Seven tools named, none usable —
toolPromotionwas off. The closing then pointed at that catalog with "any of the listed remote tools", so a model was handed names that were neither advertised nor callable.find_skills_and_tools, run_tool, any of the listed remote toolsYou can now use find_skills_and_tools, run_tool, search, chat, …toolPromotion: falsefind_skills_and_tools, run_toolYou can now use find_skills_and_tools, run_tool.You can now use .No tools are available beyond \setup`.`No tools are available beyond \setup`.`The closing is scoped to naming tools; deactivation status and the remote's
upgradeMessagestay withpolicySummary()a few lines above. Saying them in both places meant a deactivated install was told twice — and when the remote supplied its own wording, its specific remedy was immediately followed by our vaguer generic one. It needs no deactivation branch:evaluate()already reports every feature as false when deactivated, so the empty case is reached without asking.Port fidelity
Diffed before porting rather than eyeballed:
session.ts,remote-passthrough.ts— identical to vnext, copied whole.enforce.ts— differed only byMETA_TOOL_NAMESnamingfind_skills_and_tools.setupClosingLinederives its names from that set instead of repeating them, so the function ports unchanged./plugin update glean-vnext→/plugin update glean.index.ts— two call sites. Worth noting: my first anchor for thetools/listsite matched twice, because the setup handler assignscachedRemoteToolsidentically; re-anchored onserve("fetched", …).No
dist/or version changes, matching #18 — this repo builds at release time. The build's changelog-sync side effect was reverted.Verification
Run in this repo, not inherited from upstream:
typecheck:bundle, 333 tests,check:no-shell,build, andvalidateon all three targets.Mutation-checked here too, so the ported tests are not passing vacuously:
toolPromotion→ fails six, across both the unit and composition layerstests/setup-output.test.tsis new: it asserts the assembled setup text, because the duplication was a property ofpolicySummary()and the closing together and no unit test on either half could see it. Mockingversion.jsis what makes the deactivation lines reachable under vitest at all — deactivation is gated on version provenance.