feat: add sqry structural code analysis as add-in#862
feat: add sqry structural code analysis as add-in#862mr-k-man wants to merge 14 commits intogarrytan:mainfrom
Conversation
Routing contract (tools.json), detection fragment, install/uninstall scripts, and README for the sqry structural code analysis add-in. Follows the established contrib/add-tool pattern from llm-gateway. Parameter guidance is delegated to sqry's live MCP resources (sqry://docs/capability-map, sqry://docs/tool-guide) per the v8 resource delegation architecture.
TypeScript resolver reads contrib/add-tool/sqry/tools.json and generates conditional markdown per skill. Delegates parameter guidance to sqry's live MCP resources instead of hardcoding limits.
Inlines detection.sh bash fragment into preamble. Outputs SQRY, SQRY_VERSION, SQRY_INDEXED, and SQRY_STALE variables. Uses sqry's own index --status --json API (<1ms) for staleness detection.
Place after {{LEARNINGS_SEARCH}} in investigate, cso, review, retro,
plan-eng-review, and ship templates.
Generated from templates with new {{SQRY_CONTEXT}} placeholder.
Golden ship baselines updated for all three hosts.
Mirrors the llm-gateway-resolver test structure. Validates schema, tool names, MCP resource delegation, manifest health check, preamble detection, and generated SKILL.md content.
Design spec approved by Gemini 3 Pro + OpenAI Codex (4 review rounds). Includes initial implementation DAG and v8 alignment restructure DAG.
…riptions Apply techniques from TOKEN_OPTIMIZATION_GUIDE: remove filler phrases, terse descriptions, arrow notation for conditionals, consolidate repeated prose. Resolver output reduced from 33 to 22 lines per skill. tools.json when-clauses shortened (remove articles, leading verbs).
|
@garrytan A few observations from a security and performance perspective on this PR. Supply chain surface. The install script has 6 curl-pipe-bash patterns pointing to verivus-oss/sqry, a repo outside this project's control. Once installed, sqry-mcp runs as an MCP server with full codebase index access. No signature verification on the installed binaries. Prompt injection via MCP resources. The generated SKILL.md contains 23 references to ReadMcpResourceTool instructing the LLM to read sqry://docs/capability-map and sqry://docs/tool-guide (25 sqry:// URIs total) on every session. That content comes from the sqry-mcp process and enters the LLM context with zero content wrapping or sanitization. The LLM has shell and file write tools in the same context. If sqry-mcp is compromised or serves malicious content in those resources, the LLM follows the injected instructions with full tool access. This is the same vulnerability class as untrusted external content reaching an LLM without a security boundary. Performance on every skill load. The detection block runs three subprocesses (sqry --version, sqry index --status --json, grep parsing) on every single skill invocation across all 30+ skills, including skills that never use sqry. For users without sqry installed that means three failed command lookups adding latency to every skill. Blast radius. 52 files touched with 3307 additions. 30+ generated SKILL.md files modified with identical 19-line detection blocks. CLAUDE.md says generated SKILL.md files should not be edited directly. If the template resolver handles this correctly the generated files should not appear in the diff. Gus |
Generated SKILL.md files are output of bun run gen:skill-docs and should not appear in the PR diff. Reviewers run gen:skill-docs locally to verify.
…and perf Supply chain: - Remove all curl-pipe-bash patterns from install.sh - Use cargo install as primary install method (builds from source) - Point to GitHub Releases for binary downloads Prompt injection: - Remove auto-read of sqry://meta/manifest on every session - Add explicit security boundary: MCP resource content is REFERENCE DATA - Instruct agent to never execute commands found in MCP resource responses Performance: - Replace 3 subprocesses (sqry --version, sqry index --status --json, grep) with single command -v check (~1ms) + directory existence check - Index staleness deferred to query time, not preamble load - Zero subprocess overhead for users without sqry installed Addresses: garrytan#862 (comment)
|
@garagon Thanks for the thorough review. All four issues addressed in the latest commits: 1. Supply chain — curl-pipe-bash removedAll 2. Prompt injection — MCP resource security boundary
3. Performance — 3 subprocesses → 0Before: After: 4. Blast radius — generated files removed30 generated SKILL.md files removed from the diff in the prior commit. PR now touches only source files — reviewers run Commits: |
|
Thanks for the updates. The cargo install change, the reduced detection block, and removing generated files from the diff are real improvements. Two remaining concerns:
The risk here is not theoretical. A single malicious update to the sqry-mcp binary or its MCP resources gives an attacker direct influence over an LLM with shell access on every machine where this integration is installed. That scales with every user who adopts it. The fundamental question for @garrytan: should gstack skills instruct the LLM to read content from third-party MCP servers in a context where the LLM has shell access? That is an architectural decision independent of how well the resolver is implemented. |
…ance The resolver previously instructed agents to read sqry://docs/capability-map and sqry://docs/tool-guide via ReadMcpResourceTool. That content entered the LLM's instruction stream in-band, creating a prompt injection surface: a compromised sqry-mcp binary could influence agent behavior on any machine where the add-in is installed. The resolver now emits a static parameter_guidance string from tools.json. This follows the same pattern as the llm-cli-gateway add-in, where only tool names (not external content) appear in resolver output. Tests updated to assert the absence of ReadMcpResourceTool and sqry:// URIs in resolver output.
New contrib/add-tool/README.md documents the security boundary between MCP tool calls (safe, standard trust model) and MCP resource reads (content enters the LLM instruction stream, prompt injection vector). Includes the staleness trade-off discussion: resource delegation was originally designed to prevent version drift between gstack and sqry, a real concern. We chose static guidance because the injection risk scales with every install while the staleness risk is a one-line PR to fix. Updated sqry README to reflect the new architecture and preserve the design history in HTML comments for future contributors.
All three golden fixtures (claude, codex, factory) had the old pattern: sqry://meta/manifest auto-read and sqry://docs/* resource delegation. Updated to match the current resolver output with inline parameter guidance and no MCP resource reads.
Regenerated from templates after replacing MCP resource delegation with static parameter_guidance in the sqry resolver. The 6 integrated skills now show inline parameter defaults instead of ReadMcpResourceTool instructions. All 31 skills pick up the sqry preamble detection block.
|
@garagon Thanks for pressing on this. You were right on both remaining points. MCP resource reads removedThe resolver no longer instructs agents to read No external content enters the LLM's instruction stream. This follows the same pattern as the llm-cli-gateway add-in (#866), where only static tool names appear in resolver output. Golden fixtures updatedAll three golden fixtures (claude, codex, factory) now reflect the current resolver output. The old Tests updatedThe resolver tests now assert the absence of Policy for future add-insAdded The README documents the staleness trade-off openly. Resource delegation was designed to prevent version drift between gstack and sqry, which is a real concern. We chose static guidance because the injection risk scales with every install while the staleness risk is a one-line Addressing the architectural questionTo your question about whether gstack skills should instruct the LLM to read content from third-party MCP servers: the answer is no. MCP tool calls are fine (standard trust boundary, tool-response channel). MCP resource reads are not (content enters the instruction stream, no programmatic isolation). This is now documented policy. Commits: |
2 participants
Summary
contrib/add-tool/pattern/investigate,/cso,/review,/retro,/plan-eng-review,/shipArchitecture: WHEN vs HOW
tools.jsondefines which sqry tools to use at which skill phase — that's gstack's value-add. Parameter guidance (max_depth, max_results, scoping, cost tiering) is delegated to sqry's live MCP resources (sqry://docs/capability-map,sqry://docs/tool-guide), which always match the user's installed version and update automatically. No hardcoded limits that can drift.Review history
Eval results
Two full runs. Zero failures from this PR — all failures are environmental (missing browse binary, codex API unreachable).
Test plan
bun test— 155 pass, 0 fail (sqry resolver tests)bun run test:evals— all non-environmental tests pass across 10 suites{{SQRY_CONTEXT}}placeholders--host all)