Log tool-surfacing outcome on every tools/list call#21
Conversation
The tools/list handler only logged on failure paths (connect.backend-error, tools-list.fetch-failed), so the success and pre-auth early-return paths were silent. That left the most common question — "why don't my tools appear?" — unanswerable from the log: you couldn't tell whether the dynamic surface was empty because of a missing server URL, missing token, a backend blip, or a backend that genuinely returned nothing. Emit one structured tools-list.served line on every return path with the static count, the surfaced dynamic tool names, and a reason tag (unconfigured | unauthenticated | connect-error | fetch-failed | fetched). Because the allow-list only ever drops tools outside our fixed set, a missing allow-listed name (e.g. chat) means the backend never returned it. Only tool names, counts, and the reason are logged — never argument values. No change to which tools are served.
eshwar-sundar-glean
left a comment
There was a problem hiding this comment.
Nice! We should make sure all these works are folding into feedback tool
| // outside our fixed set, so a missing allow-listed name (e.g. `chat`) means | ||
| // the backend never returned it. Only tool *names*, counts and the reason | ||
| // tag are logged — never argument values, which can carry PII/secrets. | ||
| const serve = (reason: string): { tools: Tool[] } => { |
There was a problem hiding this comment.
Is it fair to call this reason, this is more of "state"
There was a problem hiding this comment.
Good call — renamed reason → state in d1f9bb3.
|
@swarup-padhi-glean - Before merging, just check this, you are mutating tools array by adding new tools, previously this was not the case. I am thinking what happens if the state keeps changing, so there will be duplicate copies. I think this might happen please do check before merge. This should probably not happen since tools array is in tools/list call, but just check once |
Per review: build the served list as a fresh [...staticTools, ...dynamic] in the serve() helper instead of pushing into the per-call tools array, so the handler never mutates a shared array (cachedRemoteTools is only reassigned or spread, never mutated). Rename the log field reason -> state. No behavior change to which tools are served on any path.
|
@eshwar-sundar-glean thanks for the careful read on both points 🙏 Mutation / duplicate copies: Folding into the feedback tool: these lines all land in |
What
Enhance logging on the
tools/listpath so the most common question — "why don't my tools appear?" — is answerable from~/.glean/glean-server.logalone.Before, the handler only logged on the two failure paths (
connect.backend-error,tools-list.fetch-failed). The success path and both pre-auth early returns (no server URL, no token) were silent, so you couldn't tell whether an empty dynamic surface was caused by missing config, missing auth, a backend blip, or a backend that genuinely returned nothing.Change
Emit one structured
tools-list.servedline on every return path of the handler, via a smallserve(reason)helper:static— count of always-present local tools (find_skills,run_tool,setup)dynamic/names— the remote tools actually surfaced (freshly fetched, or served from cache on a blip)reason— which path produced this result:unconfiguredunauthenticatedconnect-errorfetch-failedfetchedThe existing
connect.backend-error/tools-list.fetch-failederror lines (which carry the errormsg) are kept — the new line complements them with counts + outcome.Because the allow-list only ever drops tools outside our fixed 7-tool set, a missing allow-listed name (e.g.
chatabsent fromnames) means the backend never returned it — so the name list alone distinguishes "backend didn't serve it" from "auth/connectivity problem."Privacy: only tool names, counts, and the reason tag are logged — never argument values (consistent with
summarizeCall).No behavior change to which tools are served on any path.
Note / tradeoff
tools-list.servedfires on everytools/list, and hosts poll that on idle cycles — so it's chatty on a healthy plugin (this is consistent with the already-always-on error lines). It's exactly what you want when debugging tool-surfacing. If the noise is unwanted in steady state, an easy follow-up is to gate thereason === "fetched"case behind a debug env flag while keeping the diagnostic paths always-on. Happy to do that if preferred.Test plan
npm run typecheck✓npm test✓ (154 passed)npm run build✓bash scripts/check-version-bump.sh origin/main✓ (0.2.30 → 0.2.31, all three manifests aligned)Version bumped to
0.2.31across all host manifests;plugins/glean/dist/index.jsrebuilt by the pre-commit hook.