chore: phi integration and mcp client improvements#14
Conversation
Fixes CVE-2026-0621 (ReDoS) flagged by Trivy. Signed-off-by: assagman <ahmetsercansagman@gmail.com>
Avoid writing tool args/config secrets to disk and cap log line size. Signed-off-by: assagman <ahmetsercansagman@gmail.com>
- Add phi.ts module for phi-specific functionality - Add prepare-phi-extension.ts build script - Extend CLI with phi setup command support - Improve MCP client with enhanced argument handling - Add safe-regex2 type definitions - Add regex validation and catalog improvements - Update tests for new functionality Signed-off-by: assagman <ahmetsercansagman@gmail.com>
Summary of ChangesHello @assagman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration for the phi agent, mirroring the existing pi integration, and brings several significant improvements across the codebase. The changes include adding a phi setup command to the CLI, creating a new phi.ts entrypoint, and refactoring the setup logic to be more generic.
I'm impressed with the numerous enhancements to security and robustness. The addition of safe-regex2 to prevent ReDoS, redaction of sensitive data in logs, and more robust error handling in the MCP client (ensuring connections are closed on failure) are all excellent improvements. The new inheritProcessEnv option also provides better control and security for local server processes.
The code is generally well-structured, and the addition of new tests for the new functionality is great. I've left a couple of comments regarding code duplication between the older OpenCode plugin implementation (src/plugin.ts) and the newer shared src/runtime.ts. Addressing this would further improve the maintainability of the project. Overall, this is a high-quality contribution.
| const rawLimit = args.limit ?? config.settings?.defaultLimit ?? 5; | ||
| const searchLimit = Math.max(1, Math.min(rawLimit, 50)); |
There was a problem hiding this comment.
This search logic, including parameter validation like clamping the limit, is duplicated in src/runtime.ts (inside createAgentsboxRuntime). The AgentsboxPlugin appears to be re-implementing logic that is already encapsulated in the AgentsboxRuntime, which is used by other integrations like phi.
To improve maintainability and ensure consistent behavior across all integrations, consider refactoring AgentsboxPlugin to create and use an instance of AgentsboxRuntime. This would centralize the core logic for searching, execution, and status reporting.
| const underscoreIndex = fullName.indexOf("_"); | ||
| if (underscoreIndex === -1) return null; | ||
| if (underscoreIndex <= 0 || underscoreIndex === fullName.length - 1) return null; | ||
|
|
||
| return { | ||
| serverName: fullName.substring(0, underscoreIndex), | ||
| toolName: fullName.substring(underscoreIndex + 1), | ||
| }; | ||
| const serverName = fullName.substring(0, underscoreIndex); | ||
| const toolName = fullName.substring(underscoreIndex + 1); | ||
|
|
||
| if (!serverName || !toolName) return null; | ||
|
|
||
| return { serverName, toolName }; |
There was a problem hiding this comment.
This function can be simplified. The check if (!serverName || !toolName) return null; is redundant because the preceding condition if (underscoreIndex <= 0 || underscoreIndex === fullName.length - 1) already covers cases where serverName or toolName would be empty.
Additionally, this function is duplicated as parseToolName in src/plugin.ts. To improve maintainability, the logic should be centralized here, and src/plugin.ts should import and use this function.
| const underscoreIndex = fullName.indexOf("_"); | |
| if (underscoreIndex === -1) return null; | |
| if (underscoreIndex <= 0 || underscoreIndex === fullName.length - 1) return null; | |
| return { | |
| serverName: fullName.substring(0, underscoreIndex), | |
| toolName: fullName.substring(underscoreIndex + 1), | |
| }; | |
| const serverName = fullName.substring(0, underscoreIndex); | |
| const toolName = fullName.substring(underscoreIndex + 1); | |
| if (!serverName || !toolName) return null; | |
| return { serverName, toolName }; | |
| const underscoreIndex = fullName.indexOf("_"); | |
| if (underscoreIndex <= 0 || underscoreIndex === fullName.length - 1) { | |
| return null; | |
| } | |
| return { | |
| serverName: fullName.substring(0, underscoreIndex), | |
| toolName: fullName.substring(underscoreIndex + 1), | |
| }; |
There was a problem hiding this comment.
Pull request overview
Adds first-class phi integration and hardens MCP/regex/logging behavior, including new setup workflows and safety checks.
Changes:
- Introduces
src/phi.tsplus build prep (prepare-phi-extension.ts) and CLI support foragentsbox setup phi. - Improves safety: regex ReDoS rejection, searchable text bounding, toolId parsing validation, and sensitive-log redaction.
- Improves MCP robustness: clears timeout handles and closes clients on connect/listTools failures; adds
inheritProcessEnvoption for local servers.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/regex.test.ts | Adds coverage for rejecting unsafe regex patterns (ReDoS). |
| test/unit/mcp-client.test.ts | Adds tests ensuring clients are closed on initialization failures. |
| test/unit/local-client.test.ts | Adds test for inheritProcessEnv=false minimal environment propagation. |
| test/unit/cli-setup-pi.test.ts | Updates expected skill symlink destination to shared skills dir. |
| test/unit/cli-setup-phi.test.ts | New tests for setup phi dry-run planning behavior. |
| src/types/safe-regex2.d.ts | Adds local TS typings for safe-regex2. |
| src/search/regex.ts | Adds ReDoS-oriented regex safety gate via safe-regex2. |
| src/runtime.ts | Tightens toolId parsing and clamps search limits to a safe range. |
| src/plugin.ts | Adds underscore validation for server names; improves redaction and avoids logging tool arguments verbatim. |
| src/phi.ts | New phi extension entrypoint with runtime wiring and output truncation. |
| src/mcp-client/types.ts | Adds inheritProcessEnv to local server config type. |
| src/mcp-client/manager.ts | Clears timeout timers and ensures clients are closed on connect/listTools failure. |
| src/mcp-client/local.ts | Implements optional minimal env inheritance for local MCP servers. |
| src/config/schema.ts | Adds inheritProcessEnv to zod schema with default true. |
| src/cli.ts | Adds setup phi and refactors setup planning into shared agent setup; uses shared skills dir. |
| src/catalog/catalog.ts | Caps searchable text to mitigate worst-case regex/runtime behavior. |
| scripts/prepare-phi-extension.ts | New script to generate portable dist/phi-extension/ shim without symlinks. |
| scripts/build.ts | Builds src/phi.ts and runs phi extension preparation. |
| package.json | Bumps MCP SDK and adds safe-regex2 dependency. |
| bun.lock | Lockfile updates for new/updated dependencies. |
| agentsbox.schema.json | Documents inheritProcessEnv in generated JSON schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reject patterns likely to cause catastrophic backtracking (ReDoS) | ||
| if (!safeRegex(searchPattern)) { | ||
| return { | ||
| error: { | ||
| code: "invalid_pattern", | ||
| message: "Unsafe regex pattern (potential ReDoS)", | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
safe-regex2 can throw on invalid/unsupported patterns depending on its parser; since this call is outside a try/catch, an invalid pattern could crash the search path before the existing RegExp compilation try/catch runs. Wrap the safeRegex call in try/catch and return the same invalid_pattern error shape on exceptions (or fall back to the existing regex compilation error handling).
| const SENSITIVE_LOG_KEYS = new Set([ | ||
| "authorization", | ||
| "cookie", | ||
| "set-cookie", | ||
| "token", | ||
| "accessToken", | ||
| "refreshToken", | ||
| "apiKey", | ||
| "apikey", | ||
| "secret", | ||
| "password", | ||
| "headers", | ||
| "arguments", | ||
| "environment", | ||
| "command", | ||
| "commandString", | ||
| "url", | ||
| ]); |
There was a problem hiding this comment.
Redaction is currently case-sensitive, which can miss common variants like "Authorization", "Cookie", or "Set-Cookie". Consider normalizing keys during comparison (e.g., compare on k.toLowerCase()) and storing sensitive keys in lowercase only, while preserving the original key casing in the output object.
| const out: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(value)) { | ||
| if (SENSITIVE_LOG_KEYS.has(k)) { | ||
| out[k] = "[REDACTED]"; | ||
| continue; | ||
| } | ||
| out[k] = redactForLog(v, depth + 1); | ||
| } | ||
| return out; |
There was a problem hiding this comment.
Redaction is currently case-sensitive, which can miss common variants like "Authorization", "Cookie", or "Set-Cookie". Consider normalizing keys during comparison (e.g., compare on k.toLowerCase()) and storing sensitive keys in lowercase only, while preserving the original key casing in the output object.
| const extraJson = extra ? JSON.stringify(redactForLog(extra)) : ""; | ||
| const maxExtraBytes = 2048; | ||
| const extraStr = extraJson | ||
| ? ` ${extraJson.length > maxExtraBytes ? `${extraJson.slice(0, maxExtraBytes)}…` : extraJson}` | ||
| : ""; |
There was a problem hiding this comment.
The log helper comment says "Never blocks or throws", but JSON.stringify can throw (e.g., circular structures or BigInt values). Wrap the stringify step in a try/catch and fall back to a safe placeholder (e.g., "[unserializable extra]") to preserve the "never throws" guarantee.
| const text = parts.join(" "); | ||
|
|
||
| // Bound match input size to mitigate worst-case regex/runtime behavior. | ||
| // (Regex DoS can happen even with short patterns if the input is huge.) | ||
| const MAX_SEARCHABLE_TEXT = 4000; | ||
| return text.length > MAX_SEARCHABLE_TEXT ? text.slice(0, MAX_SEARCHABLE_TEXT) : text; |
There was a problem hiding this comment.
MAX_SEARCHABLE_TEXT is reallocated on every call. Since it's a constant policy value, consider moving it to module scope (or a shared constants module) to avoid repeated allocation and to make the limit easier to reuse/tune.
| function truncateHead( | ||
| text: string, | ||
| options: { maxBytes?: number; maxLines?: number } = {}, | ||
| ): TruncationResult { |
There was a problem hiding this comment.
The function name truncateHead suggests removing content from the end, but the implementation/comment indicate it removes from the head and keeps the tail. Consider renaming to something like truncateToTail/keepTail (or adjusting naming/comment) to reduce ambiguity for callers and maintainers.
| // Truncate from head (keep tail) | ||
| let keptLines = lines; | ||
| if (originalLines > maxLines) { | ||
| keptLines = lines.slice(-maxLines); | ||
| } |
There was a problem hiding this comment.
The function name truncateHead suggests removing content from the end, but the implementation/comment indicate it removes from the head and keeps the tail. Consider renaming to something like truncateToTail/keepTail (or adjusting naming/comment) to reduce ambiguity for callers and maintainers.
| // Symlink skill into pi's skill discovery path (~/.pi/agent/skills/agentsbox). | ||
| // Pi discovers skills from ~/.pi/agent/skills/**/SKILL.md recursively. | ||
| // Symlink skill into shared skills dir (~/.agents/skills/agentsbox). | ||
| // As of Feb 2026, all coding agents use this unified location. |
There was a problem hiding this comment.
Time-anchored comments like "As of Feb 2026" can become stale quickly and are harder to evaluate later. Consider rephrasing to a timeless rationale (e.g., "All supported agents use this unified location") or linking to the source of truth (docs/spec) if available.
| // As of Feb 2026, all coding agents use this unified location. | |
| // All supported coding agents use this unified location for shared skills. |
1 participant
Summary
Changes