feat: fold glean-vnext runtime MCP server into the glean plugin - #3
Conversation
Add sources/glean-vnext/ as a source plugin and merge it into the existing
`glean` emitted plugin (from: array) for claude/cursor/codex. The shipped
`glean` plugin now coexists with the portable skills: it bundles a local
stdio MCP server (glean-local) exposing find_skills/run_tool/setup, the
glean_run skill, and an auto-approve PreToolUse hook. A user may still
connect a remote Glean MCP server separately; the local server is named
glean-local to avoid clashing with a remote glean.
Source plugin layout (sources/glean-vnext/):
- plugin.pluginpack.json: files map (dist/index.js, start.mjs, package.json)
- .mcp.json: base config (glean-local, \${CLAUDE_PLUGIN_ROOT}/start.mjs)
- targets/codex/.mcp.json: codex override (./start.mjs + cwd ".")
- src/ (15 TS files) + build.mjs: server source + esbuild bundler
- tests/ (14 vitest files): server unit tests
- skills/glean_run, hooks/, start.mjs, package.json (runtime)
Build wiring:
- build:bundle runs esbuild (src -> sources/glean-vnext/dist/index.js)
- prebuild now chains sync-changelog && build:bundle before pluginpack build
- test:bundle / typecheck:bundle run the server suite scoped to the source dir
- deps: @modelcontextprotocol/sdk, yaml (runtime); esbuild, tsx, typescript,
vitest, @types/node (dev); overrides pin hono/esbuild/vite for CVEs
config (pluginpack.config.ts):
- claude glean: from += glean-vnext, components += hooks
- cursor glean: from += glean-vnext, components += hooks
- codex glean: from += glean-vnext (no hooks; matches existing codex shape)
Hook: server env lookup now checks mcpServers["glean-local"] first, falls back
to the legacy "glean" key.
Tests: npm test (build+validate) green (69/65/56 managed files);
npm run test:bundle 189/189; typecheck clean.
Requires @gleanwork/pluginpack@^0.8.0 (per-target MCP override + plugin-root
files features). Until published, use npm link @gleanwork/pluginpack against
the pluginpack branch.
…es; needs 0.8.1+)
0f08d41 to
8508c28
Compare
Relocated to a personal docs folder (Desktop/docs/pluginsUnification) alongside the pluginpack-dataflow companion doc; it is a handoff/testing artifact, not product content that belongs in the plugin source repo.
Match the pluginpack review rename (gleanwork/pluginpack#11): the plugin-root files map key is now `additionalFiles`. npm test green (69/65/56).
…tures) pluginpack 0.9.0 shipped the per-target MCP override + additionalFiles features (gleanwork/pluginpack#11). Bump the dep off ^0.7.0 so `npm ci` in publish.yml installs 0.9.0 and the pluginpack-action builds each target's payload WITH the vnext runtime files/MCP. Removes the local npm-link workaround entirely. npm test green against the published version (69/65/56).
The pluginpack-action runs `npx pluginpack diff/build` directly, not `npm run build`, so the repo prebuild (esbuild) never fires. Since sources/glean-vnext/dist/index.js is gitignored, add an explicit build:bundle step in the publish job or pluginpack's additionalFiles source read fails for every target. (The validate job already builds it via npm test.)
Release CI verified end-to-end (build → per-target payload → sync PRs)Dispatched Workflow run: https://github.com/gleanwork/agent-plugins/actions/runs/30330444116
Payload verified in the emitted output
Both pluginpack 0.9.0 features exercised for real
Note: each sync PR also carries pending updates to the existing — sent via Glean Pi |
Drop glean-vnext from the cursor glean plugin's `from:` and remove the now-orphaned `hooks` component. Cursor ships only the portable skills; claude and codex still bundle the local runtime server + hooks + glean_run skill. npm test green (cursor 62, claude 65, codex 56).
swarup-padhi-glean
left a comment
There was a problem hiding this comment.
Thorough review pass. Grouping: 3 I'd block on, 6 medium (inline where anchorable), low-severity at the bottom.
Correction first, to save cycles — Codex is not ungated. codex-mcp-client advertises capabilities.elicitation in its initialize request (verified in openai/codex codex-rs/codex-mcp/src/rmcp_client.rs). So on Codex hitlEnabled && toolMeta?.requires_approval && !isCursorClient && getClientCapabilities()?.elicitation is all true — the plugin's own elicitation fires, and readOnlyHint:true suppresses Codex's native prompt. The HITL gate holds on Codex. (The other host gaps below are real; this one isn't.)
On the separate targets/codex/.mcp.json (my earlier question) — it's mechanically required, not redundant. Codex does no variable expansion on plugin .mcp.json args (it spawns the process directly), so node "${CLAUDE_PLUGIN_ROOT}/start.mjs" would exec the literal string and die MODULE_NOT_FOUND — Codex only sets CLAUDE_PLUGIN_ROOT for hook processes, never for MCP args. The override's cwd:"." works because Codex rewrites a relative cwd against the installed plugin root, so ./start.mjs then resolves. Claude/Cursor expand the var; Codex can't. Resolving that thread.
[Medium] A shipped skill now states the opposite of what this PR does (skills/connect-glean/SKILL.md:8, unchanged file so not inline-anchorable). It says "The Glean plugin ships skills but does not bundle an MCP server." This PR folds glean-local into the same emitted glean plugin, so the model reads a false premise while troubleshooting "no Glean tools." Update connect-glean (and the README) to reflect the bundled local server and how it coexists with a remote one.
Low-severity (not blocking):
skills/glean_run/SKILL.md:6—allowed-tools: Read(path="//**/glean-skills-cache/**")isn't valid permission-rule syntax (Claude Code takes the glob directly:Read(//**/glean-skills-cache/**)), so the intended pre-approval never matches and every cache read prompts.src/index.ts:182— thefind_skillstool description still says an XML index "with usage instructions is returned", but the per-response<instructions>block was removed in this PR. Drop "with usage instructions".src/tools/remote-passthrough.ts:13— descriptor/PR text say tools are "(find_skills, run_tool, setup)", but after setup the server also promotes 7 allowlisted remote tools (search, read_document, chat, memory, memory_schema, user_activity, employee_search) as first-class — the source of duplicate-bare-name ambiguity when a remote Glean server is also connected. Document + add preference guidance in using-glean/connect-glean.src/index.ts:529—advanceSetupsuccess doesn't emittools/list_changedwhen tokens were already valid, so recovered dynamic tools stay hidden until the host re-lists (setup reply claims they're usable).src/remote-client.ts:158— two overlappingsetupcalls can double-exchange the single-use auth code; the loser's catch runsinvalidateCredentials("all"), wiping the tokens the winner just saved.src/remote-tools-cache-store.ts:38&src/skill-writer.ts:91— non-atomic writes to caches shared across sessions (esp. whenCLAUDE_PLUGIN_DATAunset →~/.glean); write to a temp file + rename. skill-writer's rm-then-write can also fail the wholefind_skillscall on a lost race.sources/glean-vnext/tsconfig.json—typecheck:bundleincludes onlysrc/, so the 14 test files are never typechecked (vitest transpiles without checking). A signature drift like this PR'srunToolAnnotations2→3-arg change wouldn't fail typecheck.build.mjs:16— comment says the bundle is "checked into git";dist/is gitignored here and rebuilt in CI (leftover from the old repo).- runtime
package.json— three conflicting Node floors (>=22 <26vs root>=24vs esbuildnode20); the<26cap already excludes Node 26, which runs the bundle fine.start.mjshas no version guard.
| { | ||
| "mcpServers": { | ||
| "glean-local": { | ||
| "command": "node", | ||
| "args": ["./start.mjs"], | ||
| "cwd": ".", | ||
| "env": { | ||
| "ENABLE_HITL": "true", | ||
| "HITL_TIMEOUT_MS": "300000" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why need a separate one for codex when it does it automatically for the 3 harnesses?
There was a problem hiding this comment.
The base .mcp.json launches the server via ${CLAUDE_PLUGIN_ROOT}/start.mjs — that variable is Claude-specific; Codex doesn't expand it. pluginpack copies the .mcp.json content per target verbatim (it doesn't rewrite launcher paths/placeholders per harness), so Codex needs the targets/codex/.mcp.json override that uses ./start.mjs + cwd: "." (Codex resolves the server's cwd to the plugin dir). Without the override, Codex would try to run a literal ${CLAUDE_PLUGIN_ROOT}/start.mjs and fail to find it. Claude/Cursor share the base file since both honor CLAUDE_PLUGIN_ROOT.
| // (already shown before this call) be the single approval. | ||
| if ( | ||
| hitlEnabled && | ||
| toolMeta?.requires_approval && |
There was a problem hiding this comment.
[High] Approval gate fails open when the tool's cached JSON is missing.
The gate keys on toolMeta?.requires_approval, and findToolJson (line 168-188) swallows missing-dir / missing-file / JSON-parse errors and returns null. When metadata is absent, toolMeta?.requires_approval is falsy, this block is skipped, and execution falls straight through to callRemoteTool — while the native prompt is already suppressed (hook allow + readOnlyHint:true). So a write tool runs with zero approval exactly when the metadata that would mark it requires_approval is what's missing.
Reachable: evictStaleSkills(…, ONE_WEEK_MS) runs at every startup (index.ts:837), so any tool cached >1 week is gone; also the model calling run_tool from memory/CLAUDE.md without a fresh find_skills, or a corrupt JSON file.
Fix: when ENABLE_HITL=true and the client supports elicitation, treat missing/unparseable metadata as requires_approval=true (or refuse with "call find_skills first"). Never execute a downstream tool whose approval requirement is unknown after the native prompt has been surrendered.
There was a problem hiding this comment.
Fixed in #6 (commit b48d6c5). The gate now fails closed: requiresApproval is true unless the tool JSON explicitly says requires_approval: false, so missing/unparseable metadata (evicted, from-memory, or corrupt) elicits rather than executing ungated. Added tests: elicits on missing JSON (accept executes / decline does not).
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "mcp__.*glean.*run_tool", |
There was a problem hiding this comment.
[High] Matcher is broader than this plugin's own server.
mcp__.*glean.*run_tool (and the script guard toolName.includes("glean") in auto-approve-run-tool.mjs:37) matches any MCP server whose name contains "glean". The code comment says "scope strictly to this plugin's run_tool" — it doesn't. If a user separately connects a remote Glean MCP server that exposes run_tool (the PR body says they may), this hook auto-approves that server's run_tool too, suppressing its native prompt — while glean-local's elicitation never runs for a different server's process. Net: the remote server's run_tool executes ungated.
Fix: match the exact bundled server name (mcp__glean-local__run_tool / the plugin-prefixed form) rather than any string containing "glean".
There was a problem hiding this comment.
Fixed in #6 (commit b48d6c5). Matcher is now mcp__.*glean-local__run_tool and the script guard is /(?:^|__)glean-local__run_tool$/ — scoped to the exact bundled server segment. Added tests: a remote mcp__glean__run_tool is not auto-approved; a host/plugin-prefixed ...__glean-local__run_tool still matches.
| if ( | ||
| hitlEnabled && | ||
| toolMeta?.requires_approval && | ||
| !isCursorClient(mcpServer) && |
There was a problem hiding this comment.
[High] Auto-approve hook is shipped to the Cursor target, contradicting this Cursor-only design.
This block deliberately skips elicitation for Cursor and omits readOnlyHint (see runToolAnnotations) so that Cursor's native prompt is the single gate — the comment above says exactly this. But pluginpack.config.ts adds hooks to the cursor target's components (line 66), shipping the auto-approve hook whose entire purpose is to suppress that native prompt.
Cursor uses "permission":"allow" rather than Claude's hookSpecificOutput.permissionDecision, but Cursor docs state it "supports loading hooks from third-party tools like Claude Code" via a compat layer. If that layer maps permissionDecision:allow (now or in a future version), Cursor loses both gates → ungated writes. Security shouldn't hinge on Cursor's compat layer not understanding the field.
Fix: make hooks a claude-only component; drop it from the cursor target.
There was a problem hiding this comment.
Already resolved: cursor's target components exclude hooks (and we dropped glean-vnext from cursor's from entirely in v3.2.0), so the auto-approve hook is claude-only and never ships to cursor. Kept it that way as the durable fix.
| return; | ||
| } | ||
|
|
||
| if (expectedState !== undefined) { |
There was a problem hiding this comment.
[Medium] OAuth state CSRF check is dead code. This validates state only when expectedState !== undefined, but no state parameter is ever generated in the authorize URL, so expectedState is always undefined and the check never runs. Loopback-only bind + PKCE make this low-exploitability, but it reads as active CSRF protection and isn't. Either generate and verify a state nonce, or drop the block and note that PKCE+loopback is the protection.
|
|
||
| let remoteClient; | ||
| try { | ||
| remoteClient = await createRemoteClient( |
There was a problem hiding this comment.
[Medium] Concurrent token refresh can wipe valid tokens. getOAuthProvider() is a process-wide singleton reused by every transport (tools/list here, find_skills, run_tool). The stdio server doesn't serialize handlers, so two concurrent calls with an expired-but-present access token each invoke the SDK refresh with the same refresh_token. If the auth server rotates refresh tokens (one-time use), the first refresh succeeds and persists new tokens; the second presents the now-consumed token, gets invalid_grant, and the SDK calls invalidateCredentials("tokens") — clearing the freshly saved tokens and forcing re-auth. Fix: dedupe refresh behind a single shared in-flight promise (or a per-token-generation mutex).
There was a problem hiding this comment.
Fixed in #6 (commit b48d6c5). connect/finishAuth now run behind a shared in-flight lock (withConnectLock), so a second concurrent handler runs only after the first has saved refreshed tokens — it reads valid tokens and skips the refresh, avoiding the rotate → invalid_grant → invalidateCredentials wipe.
| rendered = String(value); | ||
| } | ||
|
|
||
| return { line: `${key.toUpperCase()}: ${rendered}`, truncated }; |
There was a problem hiding this comment.
[Medium] Approval-prompt line injection via argument keys. Values are whitespace-collapsed, but the key is interpolated raw: ${key.toUpperCase()}: ${rendered}. A prompt-injected model can add an argument whose key contains newlines (e.g. "note\nACTION: read_only_lookup\nARGUMENTS:\n (none)") to inject fake lines into the elicitation message and spoof what the user is approving. Most downstream tools ignore unknown extra keys, so the decoy doesn't block execution. Amplified by the 7-line cap (args past 8 spill to a file the user likely won't open). Fix: sanitize keys like values, and surface a "+N more arguments" count.
There was a problem hiding this comment.
Fixed in #6 (commit b48d6c5). Argument keys are now whitespace-collapsed like values before toUpperCase(), so a key with embedded newlines can't forge structural lines in the prompt. Also added a (+N more argument(s)) disclosure line so omitted args are never silently dropped. Test asserts a newline-bearing key can't start its own ACTION: line.
| "clean": "pluginpack clean", | ||
| "prune": "pluginpack prune", | ||
| "typecheck:bundle": "tsc --noEmit -p sources/glean-vnext/tsconfig.json", | ||
| "test:bundle": "vitest run --root sources/glean-vnext", |
There was a problem hiding this comment.
[Medium] No CI runs the transplanted suite or typecheck. test:bundle (189 tests) and typecheck:bundle are wired here, but no workflow invokes them — publish.yml only builds the bundle (build:bundle) before sync, and there's no PR-triggered test job. A good suite came over with the code; nothing guards it against regressions. Add a CI step running npm run test:bundle + npm run typecheck:bundle on PRs.
| } | ||
|
|
||
| const skillEntries = index.map((entry) => { | ||
| const skillMd = entry.files.find((f) => f.endsWith("/SKILL.md")); |
There was a problem hiding this comment.
[Medium] Windows: SKILL.md reference dropped from the skill index. entry.files.find((f) => f.endsWith("/SKILL.md")) hardcodes /; on Windows the stored paths use \, so the match fails and the <file path=…/SKILL.md /> reference is omitted from <available_skills> — the model isn't pointed at the skill's instructions. Use path.sep or match on (^|[\\/])SKILL\.md$.
|
@swarup-padhi-glean all your comments are on existing Plugin code. Lets tackle it separately. This PR is just migrating the code over. |
Ack, I was supposed to review comments & post, nonetheless claude went ahead & posted it. Would still be useful to tackle as separate issues. |
…nd_tools Rename the local server's exposed discovery tool to discover_skills_and_tools (name, dispatch, agent-facing descriptions, glean_run skill, plugin description, hook-test example). The downstream/backend tool the server calls (callRemoteTool(..., "find_skills")) is unchanged \u2014 the Glean backend still exposes find_skills. typecheck + 189 bundle tests green; emitted server tools/list confirms discover_skills_and_tools.

Summary
Moves the glean-vnext runtime MCP server out of
glean-experimental-pluginsinto this repo as a source plugin (sources/glean-vnext/) and folds it into the existinggleanemitted plugin via thefrom:array for claude/cursor/codex. The shippedgleanplugin now coexists with the portable skills: it bundles a local stdio MCP server (glean-local) exposingdiscover_skills_and_tools/run_tool/setup, theglean_runskill, and an auto-approve PreToolUse hook. Users may still connect a remote Glean MCP server; the local server is namedglean-localto avoid a name clash.Draft PRs
What changed
sources/glean-vnext/— new source plugin:plugin.pluginpack.json(additionalFilesmap),.mcp.json(base),targets/codex/.mcp.json(codex override),start.mjs, runtimepackage.json,src/(15 TS),build.mjs(esbuild),tests/(14 vitest),skills/glean_run,hooks/.pluginpack.config.ts—glean-vnextadded to eachgleanfrom:;hooksadded to claude/cursorcomponents.package.json—build:bundlechained intoprebuild;test:bundle/typecheck:bundle; server deps + esbuild/vitest/tsx/typescript; CVE overrides.mcpServers["glean-local"]first (fixes prod bug from rename).Tests
npm testgreen (69/65/56 managed files).npm run test:bundle189/189.typecheck:bundleclean.Requires pluginpack ^0.9.0
Depends on gleanwork/pluginpack#11 (
additionalFiles+ per-target.mcp.jsonoverride), published as0.9.0. This repo now depends on^0.9.0directly — nonpm linkneeded.Release CI verified end-to-end
Dispatched
publish.ymlon this branch in sync mode. All three matrix jobs passed (Install → Build server bundle → Resolve pinned pluginpack version (0.9.0) → Mint token → Sync), andgleanwork/pluginpack-actionopened one machine-ownedpluginpack/sync-<target>PR per output repo (never pushed to their default branches).Latest sync run: https://github.com/gleanwork/agent-plugins/actions/runs/30428683189 (all three jobs ✓)
hooks/.mcp.json,dist/index.js,start.mjs,package.json,skills/glean_run/, plushooks/.glean-vnextfrom itsfrom:and thehookscomponent).componentsconfig working as designed..mcp.json→args: ["./start.mjs"]+"cwd": "."; claude →args: ["${CLAUDE_PLUGIN_ROOT}/start.mjs"]. Local server namedglean-localin both.Testing doc
The manual test workflow (Claude/Cursor/Codex) is maintained as a separate handoff artifact outside this repo (not product content), alongside the pluginpack dataflow doc.
Sequence diagrams
Build time — vnext folded into the
gleanpluginsequenceDiagram participant Dev as developer participant PB as npm test participant Pre as prebuild<br/>build:bundle (esbuild) participant PP as pluginpack build participant Merge as glean = from[glean-lib, shared, <target>, glean-vnext] Dev->>PB: npm test PB->>Pre: esbuild src/ -> sources/glean-vnext/dist/index.js Pre-->>PB: bundle ready PB->>PP: pluginpack build (per target) PP->>Merge: merge source plugins into one glean plugin Note over Merge: skills: 18 portable + glean_run<br/>files: dist/index.js, start.mjs, package.json<br/>.mcp.json: glean-local (codex override = ./start.mjs + cwd)<br/>hooks: claude/cursor only Merge-->>PP: emitted glean plugin PP-->>Dev: dist/{claude 65, cursor 69, codex 56} + Validation passedRuntime — local server in the host (Claude/Cursor/Codex)
sequenceDiagram actor User participant Host as Host (Claude Code) participant Launch as start.mjs participant Srv as dist/index.js<br/>glean-local MCP server participant Hook as auto-approve hook participant Glean as Glean backend Host->>Launch: node ${CLAUDE_PLUGIN_ROOT}/start.mjs Launch->>Srv: import dist/index.js (stdio) Srv-->>Host: tools: discover_skills_and_tools, run_tool, setup (state: unconfigured) User->>Host: "set up Glean, email me@acme.com" Host->>Srv: setup({email}) Srv->>Glean: resolve Server URL -> OAuth (browser) -> fetch tool catalog Glean-->>Srv: tokens + remote tools Srv-->>Host: configured (dynamic tools > 0) User->>Host: "find a skill / run it" Host->>Srv: discover_skills_and_tools -> run_tool Host->>Hook: PreToolUse(run_tool) Note over Hook: ENABLE_HITL=true -> auto-approve<br/>(HITL elicitation is the single gate) Hook-->>Host: allow (no native prompt) Srv->>Glean: execute tool Glean-->>Host: result