feat(runtime): rebuild MCP and Skill control plane - #15
Open
boh5 wants to merge 2 commits into
Open
Conversation
Hot-apply user MCP servers across all agents, add strict HTTP and STDIO lifecycle management, and expose MCP controls in Settings. Add five-tier Skill discovery, diagnostics, immutable execution overlays, and the Composer picker. BREAKING CHANGE: MCP server configuration now requires type and enabled, and the legacy url, headers, and timeout shape is rejected.
|
This PR has 13,221 reviewable changed lines after ignored/generated files are excluded, above this repository's 10,000-changed-line automatic review limit. The raw diff is 14,456 lines before ignored/generated files are excluded. Most of the diff comes from:
Comment |
📝 WalkthroughWalkthroughThe pull request replaces legacy MCP management with a live HTTP/STDIO runtime. It adds strict MCP configuration, runtime controls, Skill discovery and snapshots, paginated inventories, slash-command activation, execution metadata, and related server and web interfaces. ChangesMCP runtime and configuration
Skill control plane
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Owner
Author
|
@cubic-dev-ai review this |
@boh5 I have started the AI code review. It will take a few minutes to complete. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-core/src/mcp/errors.ts (1)
40-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake error messages match
reason.When
reasonis"aborted"or"timeout", both messages still state that the operation “failed”.tool-adapter.tsreturns these messages with cancellation or timeout result codes. Generate reason-specific messages for consistent tool feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/errors.ts` around lines 40 - 56, Update the constructors for the MCP connection and tool execution error classes so their messages use the provided reason: report cancellation for "aborted", timeout for "timeout", and retain failure wording for "failed". Preserve the existing server, tool, and cause details while ensuring tool-adapter result messages remain consistent with their reason codes.
🧹 Nitpick comments (14)
packages/agent-core/src/mcp/naming.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Bun’s native
Bun.CryptoHasherAPI instead ofnode:crypto. Preserve the current SHA-256 updates anddigest("hex")output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/naming.ts` at line 1, Replace the node:crypto createHash usage in the naming module with Bun.CryptoHasher, preserving the existing SHA-256 update sequence and digest("hex") output behavior.Source: Coding guidelines
packages/agent-core/src/mcp/tool-adapter.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the MCP input-validation boundary.
ToolRegistry.#prepareInputappliessafeParsetomcpToolInputSchema, but.catchall(z.unknown())accepts any object.aiInputSchemaonly describes inputs for the model; this code does not validatemcpTool.inputSchemabefore dispatch. If the MCP server is the validation boundary, document the local exception to the strict Zod-schema rule and add a regression test for invalid arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/tool-adapter.ts` at line 19, Document that mcpToolInputSchema intentionally accepts arbitrary object arguments because MCP performs validation before dispatch, while aiInputSchema only describes model inputs. Add a regression test covering invalid arguments through ToolRegistry.#prepareInput and confirm they are passed to the MCP tool without local schema rejection.Source: Coding guidelines
packages/agent-core/src/mcp/client.ts (2)
238-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated discovery deadline check.
Lines 263 and 266 run the same
this.now() >= deadlinecheck with only a synchronoustools.pushbetween them. One check is enough.♻️ Proposed simplification
throwIfAborted(signal); if (this.now() >= deadline) throw discoveryTimeoutError(timeoutMs); tools.push(...result.tools.map((tool) => tool as McpToolLike)); - if (this.now() >= deadline) throw discoveryTimeoutError(timeoutMs); if (result.nextCursor === undefined) break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/client.ts` around lines 238 - 278, Remove one of the duplicated this.now() >= deadline checks in listTools, retaining a single discovery deadline validation around the synchronous tools.push and pagination handling. Preserve the existing timeout behavior and error flow through discoveryTimeoutError.
371-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBounded stderr can split a multi-byte UTF-8 sequence.
Line 378 slices the buffer at a fixed byte count and then decodes. A truncated multi-byte character produces a replacement character in the log. Use the same UTF-8-safe truncation helper that the Skill projection code uses, if one is exported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/client.ts` around lines 371 - 383, Update `#attachBoundedStderr` to use the exported UTF-8-safe truncation helper used by the Skill projection code instead of slicing the encoded buffer directly at MAX_STDERR_LOG_BYTES. Preserve the existing stderr conversion, redaction, logger event, and serverName context while ensuring truncation never splits a multi-byte character.packages/protocol/src/types.test.ts (1)
181-195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind the MCP fixtures to their protocol types.
The fixtures use
as constwith no type relationship toMcpServerStatusorMcpServerInventoryResponse. The assertions only compare an object to its own JSON round trip, so this test passes even if the protocol types change or the fixture stops matching them. The new binding test at Line 63 already usessatisfies; apply the same here.💚 Proposed change
- const statuses = { + const statuses = { docs: { state: "ready", toolCount: 1, warningCount: 0, connectedAt: 123 }, local: { state: "disabled", updatedAt: 124 }, - } as const; - const inventory = { + } satisfies Record<string, McpServerStatus>; + const inventory = { servers: { docs: [{ serverName: "docs", name: "search", registryName: "mcp__docs__search", description: "Search docs" }], local: [], }, - } as const; + } satisfies McpServerInventoryResponse;Add
McpServerStatusandMcpServerInventoryResponseto the type imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocol/src/types.test.ts` around lines 181 - 195, Bind the statuses and inventory fixtures in the test using the protocol types: add McpServerStatus and McpServerInventoryResponse to the type imports, then apply satisfies McpServerStatus and satisfies McpServerInventoryResponse alongside as const. Keep the existing serializeRoundTrip assertions unchanged.apps/web/src/components/features/ChatInput.tsx (1)
551-565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep non-option content out of the
listboxcontainer.In the loading state and the "No matching Skills." state, the container keeps
role="listbox"while its only children are<p role="status">elements. Alistboxmust ownoption(orgroup) elements. Screen readers can drop or misreport these paragraphs, and an empty listbox is announced as an empty list.Use the same treatment you already apply to the failed state: render
role="group"when no options exist.♿ Proposed change
- role={skillUseInput !== null && skillInventoryState === "failed" ? "group" : "listbox"} + role={skillUseInput !== null && slashOptionCount === 0 ? "group" : "listbox"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/features/ChatInput.tsx` around lines 551 - 565, Update the role expression on the slash menu container in the ChatInput render so loading and empty “No matching Skills.” states use role="group" when no selectable options exist, matching the existing failed-state treatment. Preserve role="listbox" for states with actual options, and keep the existing option rendering unchanged.Source: Coding guidelines
apps/web/src/components/features/settings-panels.tsx (1)
483-493: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider narrowing the inventory effect dependency.
The effect depends on the
serversstatus record identity. Each MCP status event replaces that object and triggers a newgetMcpInventory()request. During a connect/discovery sequence this produces repeated fetches. A derived key such as a joinedname:statestring reduces the request count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/features/settings-panels.tsx` around lines 483 - 493, Update the inventory-loading useEffect dependency around getMcpInventory so it no longer depends directly on the servers object identity. Derive a stable key from each server’s name and state (for example, joined name:state values) and depend on that key instead, while preserving the existing active, expectedRevision, and runtimeAvailable triggers and mounted guards.packages/agent-core/src/mcp/fixtures/stdio-server.ts (1)
6-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: share the fixture tool definitions with the HTTP fixture.
tool()and both request handlers duplicatepaginatedServer()inpackages/agent-core/src/mcp/transports.integration.test.ts(lines 29-53). Extract the tool list and handler registration into one exported helper, and keep only the transport bootstrap here. The two pagination fixtures then cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/mcp/fixtures/stdio-server.ts` around lines 6 - 27, Extract the shared paginated tool definition and request-handler registration from the stdio fixture and the HTTP fixture’s paginatedServer() into one exported helper. Update both fixtures to call that helper while retaining only their transport-specific server bootstrap, preserving the existing pagination and tool-call behavior.packages/agent-core/src/commands/skill.ts (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
SkillServicethrough the Skills barrel.Line 26 imports from
../skills/service. ImportSkillServicefrom../skillsinstead. This preserves the public module boundary.Proposed change
+import type { SkillService } from "../skills"; - readonly skillService: import("../skills/service").SkillService; + readonly skillService: SkillService;As per coding guidelines, use barrel exports through
index.tsfiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/commands/skill.ts` around lines 25 - 31, Update the validateSkillActivation input type to reference SkillService through the public ../skills barrel export instead of the internal ../skills/service module, preserving the existing type and behavior.Source: Coding guidelines
packages/agent-core/src/agents/factory-types.ts (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the tools barrel for
AnyToolDescriptor.Export
AnyToolDescriptorfrompackages/agent-core/src/tools/index.ts, then import it through../tools.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/agents/factory-types.ts` around lines 6 - 7, Export AnyToolDescriptor from the tools barrel at tools/index.ts, then update the import in factory-types.ts to reference ../tools instead of ../tools/types.Source: Coding guidelines
packages/agent-core/src/skills/package-reader.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
node:cryptowithBun.CryptoHasherin both new hashing sites. Both files importcreateHashfromnode:cryptoalthough Bun provides an equivalent, andpackages/agent-core/src/prompt/compiler.tsalready usesnew Bun.CryptoHasher("sha256").
packages/agent-core/src/skills/package-reader.ts#L1-L1: drop thenode:cryptoimport and build the snapshot digest withBun.CryptoHasher, updatingdigestSnapshot,updateDigestField, andupdateDigestLengthsignatures.packages/agent-core/src/skills/service.ts#L1-L1: drop thenode:cryptoimport and computestableDigestwithBun.CryptoHasher.As per coding guidelines: "Use Bun as the runtime and prefer Bun-native APIs over
node:*imports; usenode:*only when Bun has no suitable alternative."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.ts` at line 1, Replace the node:crypto createHash usage at packages/agent-core/src/skills/package-reader.ts#L1-L1 and packages/agent-core/src/skills/service.ts#L1-L1 with Bun.CryptoHasher. In package-reader.ts, update digestSnapshot, updateDigestField, and updateDigestLength to use the Bun hasher while preserving snapshot digest behavior; in service.ts, compute stableDigest with Bun.CryptoHasher and remove both node:crypto imports.Source: Coding guidelines
229-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
SkillPackageResourcePathErrorfor all path validation failures.Lines 229-236 now throw
SkillPackageResourcePathError, but the depth check and theSKILL.mdcheck still throw a plainError. Callers cannot classify path failures reliably. Convert the remaining two throws.♻️ Proposed change
if (segments.length > SKILL_RESOURCE_MAX_DEPTH) { - throw new Error(`Skill resource depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); + throw new SkillPackageResourcePathError(`Skill resource depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); } if (segments[0]?.toLowerCase() === SKILL_ENTRY_FILE.toLowerCase()) { - throw new Error("SKILL.md is the package entry and cannot be a resource directory"); + throw new SkillPackageResourcePathError("SKILL.md is the package entry and cannot be a resource directory"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.ts` around lines 229 - 244, Update validateResourcePath so the depth-limit and SKILL.md entry-file checks also throw SkillPackageResourcePathError, matching the existing error type used for all other invalid resource paths. Preserve the current validation conditions and messages.packages/agent-core/src/skills/projection.ts (1)
16-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the projection fit search from O(n²) to O(n).
Each iteration re-renders and re-measures every included entry. For a large catalog the loop runs once per omitted entry, so cost grows quadratically with the number of skills. Compute each line once, accumulate prefix byte lengths, then select the largest prefix that fits the budget plus the omission line.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/projection.ts` around lines 16 - 27, Update the projection fit logic around renderProjection to avoid re-rendering prefixes in the includedCount loop. Render each normalized entry once, compute cumulative UTF-8 byte lengths, and use those prefix totals with the omission-line size to select the largest fitting prefix within maxBytes; preserve the existing frozen result shape and overflow error.packages/agent-core/src/skills/service.ts (1)
152-199: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce catalog build cost.
Two problems compound here.
sourceNames.includes(name)runs a linear scan for every name and every source, so candidate assembly is quadratic in catalog size.#discoverCandidateis then awaited serially, so every candidateSKILL.mdread blocks the next one.catalogForAgentruns on the prompt-compile path and on everyskill_listcall.Build one
Setper source before the name loop, and resolve candidate discovery with bounded concurrency instead of a fully serial loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/service.ts` around lines 152 - 199, Optimize catalog construction in the surrounding catalog-building method by precomputing a Set of names for each entry in namesBySource, then use Set.has(name) instead of sourceNames.includes(name). Replace the serial awaits around `#discoverCandidate` with bounded-concurrency processing while preserving candidate order, winner selection, inventory entries, diagnostics, and entries output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/features/settings-panels.tsx`:
- Line 601: Update the stdio arguments handler in the settings panel’s textarea
onChange flow to remove empty lines after splitting the input by newline before
assigning draft.args. Preserve undefined for entirely empty input, and ensure
stored arguments contain only non-empty lines.
In `@apps/web/src/components/features/SettingsDialog.tsx`:
- Around line 50-58: Update the snapshot-change useEffect to clear
preserveSaveErrorRevision.current in both branches, including when the current
revision matches; ensure no recorded revision marker survives after processing
any snapshot change.
In `@packages/agent-core/src/config/runtime-secret-literals.ts`:
- Around line 88-93: Update collectRuntimeSecretLiterals around the HTTP server
branch so server.url is excluded from runtime secret literal collection, keeping
header and environment collection unchanged; add a regression test confirming an
HTTP URL such as http:a follows the same save and runtime validation behavior.
In `@packages/agent-core/src/execution/session-execution-manager.ts`:
- Line 439: Update deleteSession to remove every deleted session ID from
`#executionSkillSnapshots` before completing session cleanup. Ensure suspended and
non-terminal executions are evicted even when terminalization methods are not
invoked, while preserving the existing agent, store, and directory deletion
behavior.
In `@packages/agent-core/src/mcp/runtime-service.ts`:
- Around line 365-368: Update the losing-candidate branch in `#connectAndPublish`
so candidate.handle.retire() is wrapped with the same `#logCloseFailure` handling
used by the error path, preventing retire failures from rejecting reconnect
operations for superseded connections.
- Around line 185-205: Update testServer and its in-flight test state to enforce
a shared maximum number of concurrent draft tests, rejecting new drafts once the
limit is reached while preserving the existing duplicate testKey guard. Ensure
the count is incremented only for accepted tests and decremented in the existing
promise.finally cleanup so slots are released on every completion path.
- Around line 597-602: Update createServerRedactionPolicy to include only
secret-bearing values extracted from config.args for stdio servers, while
preserving header/env handling for HTTP and other transports. Filter argument
candidates through the existing SecretRedactionPolicy eligibility rules so short
ordinary flags such as --stdio are excluded and longer non-secret arguments are
not redacted; add coverage for both cases.
In `@packages/agent-core/src/mcp/transports.integration.test.ts`:
- Around line 162-179: Replace the immediate sessionCloseCount assertions after
runtime.reconnect("local") and runtime.apply(...) with expectHttpSessionClose
waits for the retired first handle, matching the existing usage later in the
test. Keep the active-session and inventory assertions, and retain the
pre-operation close counts so the helper verifies exactly one additional close
on each path.
In `@packages/agent-core/src/runtime-mcp.test.ts`:
- Line 178: Remove the duplicate calls declaration in the test setup, retain
only one boundaries declaration near the affected test, and keep a single
streamText property in the object literal. Update the relevant test code in
runtime-mcp.test.ts without changing the remaining setup or behavior.
In `@packages/agent-core/src/runtime-skill-command.test.ts`:
- Around line 85-146: Install the test LLM adapter before the test accepts any
session messages, using the existing
setLlmAdapterForTest()/installTestLlmAdapter() pattern from main.test.ts. Update
the runtime test setup around createRuntime and acceptSessionMessage so
execution uses the stubbed adapter and cannot make live model calls; preserve
the existing assertions and cleanup.
In `@packages/agent-core/src/runtime.ts`:
- Around line 2282-2290: Restrict the fallback lookup in testMcpServer to own
keys of BUILTIN_MCP_SERVERS by using Object.hasOwn or an explicit built-in name
membership check before indexing. Preserve draft server precedence and continue
throwing the existing configuration error when neither source contains
serverName.
In `@packages/agent-core/src/skills/projection.test.ts`:
- Around line 41-42: Update the omitted-skills rendering logic and its assertion
in the projection test so omittedCount === 1 uses singular “Skill omitted,”
while counts greater than one retain “Skills omitted.”
In `@packages/agent-core/src/store/helpers.ts`:
- Around line 922-931: Update the available schema’s renderedText field
alongside byteLength so persisted text is directly limited to the 8,000-byte
maximum, using the existing boundedUtf8String helper and preserving the current
byteLength validation.
- Around line 280-285: Update the executionSkills schema to pass the imported
SKILL_SOURCE_TIERS constant to z.enum instead of duplicating the five tier
values inline. Keep the existing name, digest, and resolutionRoot validations
unchanged, ensuring execution records and prompt traces share the same
source-tier definition.
In `@packages/agent-core/src/tools/builtins/skill-read.test.ts`:
- Line 14: Update the SkillService setup in the skill-read test suite to pass an
isolated temporary userAgentsSkillsRoot under tmpRoot, ensuring skill_read
resolves only from the test directory. Keep the unknown-skill test independent
of any skills installed in $HOME/.agents/skills.
In `@packages/agent-core/src/tools/registry.ts`:
- Around line 119-132: Extract the descriptor invariant validation currently
performed by register() into a reusable helper, then invoke it from
executeResolved() before calling `#execute`(). Ensure resolved descriptors with
traits.destructive set but no permissions are rejected consistently with
registered descriptors, while preserving the existing tool-name mismatch
handling.
In `@packages/protocol/src/guards.ts`:
- Around line 219-227: Update isExecutionSkillBinding to validate digest as a
64-character hexadecimal string rather than any string, while preserving the
existing field and source checks. Add rejection coverage for short and malformed
digest values.
In `@packages/protocol/src/types.ts`:
- Around line 973-997: Update README.md to document the MCP configuration schema
represented by ConfigMcpServerSettings, including required type and enabled
fields, HTTP and stdio-specific properties, the connectTimeoutMs,
discoveryTimeoutMs, and callTimeoutMs fields replacing timeout, and the
mcp.disabledBuiltins setting.
---
Outside diff comments:
In `@packages/agent-core/src/mcp/errors.ts`:
- Around line 40-56: Update the constructors for the MCP connection and tool
execution error classes so their messages use the provided reason: report
cancellation for "aborted", timeout for "timeout", and retain failure wording
for "failed". Preserve the existing server, tool, and cause details while
ensuring tool-adapter result messages remain consistent with their reason codes.
---
Nitpick comments:
In `@apps/web/src/components/features/ChatInput.tsx`:
- Around line 551-565: Update the role expression on the slash menu container in
the ChatInput render so loading and empty “No matching Skills.” states use
role="group" when no selectable options exist, matching the existing
failed-state treatment. Preserve role="listbox" for states with actual options,
and keep the existing option rendering unchanged.
In `@apps/web/src/components/features/settings-panels.tsx`:
- Around line 483-493: Update the inventory-loading useEffect dependency around
getMcpInventory so it no longer depends directly on the servers object identity.
Derive a stable key from each server’s name and state (for example, joined
name:state values) and depend on that key instead, while preserving the existing
active, expectedRevision, and runtimeAvailable triggers and mounted guards.
In `@packages/agent-core/src/agents/factory-types.ts`:
- Around line 6-7: Export AnyToolDescriptor from the tools barrel at
tools/index.ts, then update the import in factory-types.ts to reference ../tools
instead of ../tools/types.
In `@packages/agent-core/src/commands/skill.ts`:
- Around line 25-31: Update the validateSkillActivation input type to reference
SkillService through the public ../skills barrel export instead of the internal
../skills/service module, preserving the existing type and behavior.
In `@packages/agent-core/src/mcp/client.ts`:
- Around line 238-278: Remove one of the duplicated this.now() >= deadline
checks in listTools, retaining a single discovery deadline validation around the
synchronous tools.push and pagination handling. Preserve the existing timeout
behavior and error flow through discoveryTimeoutError.
- Around line 371-383: Update `#attachBoundedStderr` to use the exported
UTF-8-safe truncation helper used by the Skill projection code instead of
slicing the encoded buffer directly at MAX_STDERR_LOG_BYTES. Preserve the
existing stderr conversion, redaction, logger event, and serverName context
while ensuring truncation never splits a multi-byte character.
In `@packages/agent-core/src/mcp/fixtures/stdio-server.ts`:
- Around line 6-27: Extract the shared paginated tool definition and
request-handler registration from the stdio fixture and the HTTP fixture’s
paginatedServer() into one exported helper. Update both fixtures to call that
helper while retaining only their transport-specific server bootstrap,
preserving the existing pagination and tool-call behavior.
In `@packages/agent-core/src/mcp/naming.ts`:
- Line 1: Replace the node:crypto createHash usage in the naming module with
Bun.CryptoHasher, preserving the existing SHA-256 update sequence and
digest("hex") output behavior.
In `@packages/agent-core/src/mcp/tool-adapter.ts`:
- Line 19: Document that mcpToolInputSchema intentionally accepts arbitrary
object arguments because MCP performs validation before dispatch, while
aiInputSchema only describes model inputs. Add a regression test covering
invalid arguments through ToolRegistry.#prepareInput and confirm they are passed
to the MCP tool without local schema rejection.
In `@packages/agent-core/src/skills/package-reader.ts`:
- Line 1: Replace the node:crypto createHash usage at
packages/agent-core/src/skills/package-reader.ts#L1-L1 and
packages/agent-core/src/skills/service.ts#L1-L1 with Bun.CryptoHasher. In
package-reader.ts, update digestSnapshot, updateDigestField, and
updateDigestLength to use the Bun hasher while preserving snapshot digest
behavior; in service.ts, compute stableDigest with Bun.CryptoHasher and remove
both node:crypto imports.
- Around line 229-244: Update validateResourcePath so the depth-limit and
SKILL.md entry-file checks also throw SkillPackageResourcePathError, matching
the existing error type used for all other invalid resource paths. Preserve the
current validation conditions and messages.
In `@packages/agent-core/src/skills/projection.ts`:
- Around line 16-27: Update the projection fit logic around renderProjection to
avoid re-rendering prefixes in the includedCount loop. Render each normalized
entry once, compute cumulative UTF-8 byte lengths, and use those prefix totals
with the omission-line size to select the largest fitting prefix within
maxBytes; preserve the existing frozen result shape and overflow error.
In `@packages/agent-core/src/skills/service.ts`:
- Around line 152-199: Optimize catalog construction in the surrounding
catalog-building method by precomputing a Set of names for each entry in
namesBySource, then use Set.has(name) instead of sourceNames.includes(name).
Replace the serial awaits around `#discoverCandidate` with bounded-concurrency
processing while preserving candidate order, winner selection, inventory
entries, diagnostics, and entries output.
In `@packages/protocol/src/types.test.ts`:
- Around line 181-195: Bind the statuses and inventory fixtures in the test
using the protocol types: add McpServerStatus and McpServerInventoryResponse to
the type imports, then apply satisfies McpServerStatus and satisfies
McpServerInventoryResponse alongside as const. Keep the existing
serializeRoundTrip assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71239106-38b9-434c-b4a1-da80979a3940
📒 Files selected for processing (168)
AGENTS.mdCHANGELOG.mdapps/server/src/app.test.tsapps/server/src/app.tsapps/server/src/errors.tsapps/server/src/routes/attachments.test.tsapps/server/src/routes/compression.test.tsapps/server/src/routes/config.test.tsapps/server/src/routes/files.test.tsapps/server/src/routes/mcp.test.tsapps/server/src/routes/mcp.tsapps/server/src/routes/messages.test.tsapps/server/src/routes/projects.test.tsapps/server/src/routes/sessions.test.tsapps/server/src/routes/skills.test.tsapps/server/src/routes/skills.tsapps/server/src/server-host.test.tsapps/server/src/server-host.tsapps/web/src/api/config.test.tsapps/web/src/api/config.tsapps/web/src/api/mcp.test.tsapps/web/src/api/mcp.tsapps/web/src/api/skills.test.tsapps/web/src/api/skills.tsapps/web/src/components/composite/ExecutionWorkstream.interaction.tsxapps/web/src/components/features/ChatHeader.test.tsxapps/web/src/components/features/ChatInput.test.tsxapps/web/src/components/features/ChatInput.tsxapps/web/src/components/features/ComposerQueueList.interaction.tsxapps/web/src/components/features/SessionComposerDock.interaction.tsxapps/web/src/components/features/SettingsDialog.interaction.tsxapps/web/src/components/features/SettingsDialog.test.tsxapps/web/src/components/features/SettingsDialog.tsxapps/web/src/components/features/TodoProgressButton.interaction.tsxapps/web/src/components/features/settings-helpers.tsapps/web/src/components/features/settings-panels.tsxapps/web/src/components/ui/Dialog.test.tsxapps/web/src/components/ui/Dialog.tsxapps/web/src/context/global-sse.test.tsxapps/web/src/context/settings-modal.tsxapps/web/src/lib/execution-status-presentation.test.tsapps/web/src/lib/execution-workstream.test.tsapps/web/src/routes/session.test.tsxapps/web/src/store/mcp-status-store.test.tsapps/web/src/store/session-store.test.tsconfig.example.jsondesign-system/pages/settings.mddocs/architecture.mddocs/configuration.mddocs/goals/mcp-skill-control-plane-hard-cut-plan-goal.mddocs/goals/mcp-skill-control-plane-hard-cut-progress.mddocs/integrations.mdpackages/agent-core/src/__arch__/architecture.test.tspackages/agent-core/src/__arch__/tool-output-boundaries.test.tspackages/agent-core/src/__arch__/tool-output-policy-matrix.test.tspackages/agent-core/src/agents/configured-agent-mcp.test.tspackages/agent-core/src/agents/configured-agent.test.tspackages/agent-core/src/agents/configured-agent.tspackages/agent-core/src/agents/definitions/analyst.tspackages/agent-core/src/agents/definitions/build.tspackages/agent-core/src/agents/definitions/definitions.test.tspackages/agent-core/src/agents/definitions/discussion.tspackages/agent-core/src/agents/definitions/explore.tspackages/agent-core/src/agents/definitions/lead.tspackages/agent-core/src/agents/definitions/librarian.tspackages/agent-core/src/agents/factory-types.tspackages/agent-core/src/agents/factory.test.tspackages/agent-core/src/agents/factory.tspackages/agent-core/src/agents/query/loop.test.tspackages/agent-core/src/agents/query/loop.tspackages/agent-core/src/agents/query/provider-secret-redaction.integration.test.tspackages/agent-core/src/agents/query/recovery.test.tspackages/agent-core/src/agents/query/types.tspackages/agent-core/src/agents/session-agent-manager.test.tspackages/agent-core/src/agents/session-agent-manager.tspackages/agent-core/src/agents/types.tspackages/agent-core/src/attachments/read-paths.test.tspackages/agent-core/src/background/tasks/title-generation.test.tspackages/agent-core/src/commands/skill.test.tspackages/agent-core/src/commands/skill.tspackages/agent-core/src/commands/types.tspackages/agent-core/src/config/index.tspackages/agent-core/src/config/mcp.test.tspackages/agent-core/src/config/mcp.tspackages/agent-core/src/config/runtime-secret-literals.test.tspackages/agent-core/src/config/runtime-secret-literals.tspackages/agent-core/src/config/server-config-service.test.tspackages/agent-core/src/config/server-config-service.tspackages/agent-core/src/events/session-event-bridge.test.tspackages/agent-core/src/execution/session-execution-manager.test.tspackages/agent-core/src/execution/session-execution-manager.tspackages/agent-core/src/execution/session-tool-batch-scheduler.test.tspackages/agent-core/src/execution/session-tool-batch-scheduler.tspackages/agent-core/src/index.tspackages/agent-core/src/lead-architecture-flows.integration.test.tspackages/agent-core/src/main.test.tspackages/agent-core/src/mcp/builtin-servers.tspackages/agent-core/src/mcp/client.test.tspackages/agent-core/src/mcp/client.tspackages/agent-core/src/mcp/errors.tspackages/agent-core/src/mcp/fixtures/stdio-server.tspackages/agent-core/src/mcp/index.tspackages/agent-core/src/mcp/manager.test.tspackages/agent-core/src/mcp/manager.tspackages/agent-core/src/mcp/naming.test.tspackages/agent-core/src/mcp/naming.tspackages/agent-core/src/mcp/runtime-service.test.tspackages/agent-core/src/mcp/runtime-service.tspackages/agent-core/src/mcp/tool-adapter.test.tspackages/agent-core/src/mcp/tool-adapter.tspackages/agent-core/src/mcp/transports.integration.test.tspackages/agent-core/src/prompt/compiler.test.tspackages/agent-core/src/prompt/compiler.tspackages/agent-core/src/prompt/live-eval.tspackages/agent-core/src/prompt/types.tspackages/agent-core/src/runtime-automations.integration.test.tspackages/agent-core/src/runtime-automations.test.tspackages/agent-core/src/runtime-data/service.test.tspackages/agent-core/src/runtime-mcp.test.tspackages/agent-core/src/runtime-skill-command.test.tspackages/agent-core/src/runtime.tspackages/agent-core/src/session-input/model-selection.test.tspackages/agent-core/src/session-input/service.test.tspackages/agent-core/src/session-input/service.tspackages/agent-core/src/skills/index.tspackages/agent-core/src/skills/package-reader.test.tspackages/agent-core/src/skills/package-reader.tspackages/agent-core/src/skills/pagination.test.tspackages/agent-core/src/skills/pagination.tspackages/agent-core/src/skills/projection.test.tspackages/agent-core/src/skills/projection.tspackages/agent-core/src/skills/service.test.tspackages/agent-core/src/skills/service.tspackages/agent-core/src/skills/types.tspackages/agent-core/src/store/helpers.test.tspackages/agent-core/src/store/helpers.tspackages/agent-core/src/store/logical-execution.test.tspackages/agent-core/src/store/message-phase-hard-cut.test.tspackages/agent-core/src/store/session-store-manager.test.tspackages/agent-core/src/store/store.test.tspackages/agent-core/src/testing/test-execution-fixtures.tspackages/agent-core/src/testing/test-mcp-runtime.tspackages/agent-core/src/tool-output/artifact-lifecycle.test.tspackages/agent-core/src/tool-output/live-bash.integration.test.tspackages/agent-core/src/tools/builtins/skill-list.test.tspackages/agent-core/src/tools/builtins/skill-list.tspackages/agent-core/src/tools/builtins/skill-read.test.tspackages/agent-core/src/tools/builtins/skill-read.tspackages/agent-core/src/tools/concurrency/partition.test.tspackages/agent-core/src/tools/concurrency/partition.tspackages/agent-core/src/tools/errors.test.tspackages/agent-core/src/tools/errors.tspackages/agent-core/src/tools/index.tspackages/agent-core/src/tools/permission/index.tspackages/agent-core/src/tools/permission/mcp.test.tspackages/agent-core/src/tools/permission/mcp.tspackages/agent-core/src/tools/registry.test.tspackages/agent-core/src/tools/registry.tspackages/agent-core/src/tools/types.tspackages/protocol/src/execution.test.tspackages/protocol/src/execution.tspackages/protocol/src/guards.test.tspackages/protocol/src/guards.tspackages/protocol/src/message-phase-hard-cut.test.tspackages/protocol/src/reduce.test.tspackages/protocol/src/reduce.tspackages/protocol/src/types.test.tspackages/protocol/src/types.ts
💤 Files with no reviewable changes (9)
- apps/server/src/routes/projects.test.ts
- packages/agent-core/src/tools/permission/mcp.ts
- packages/agent-core/src/tools/index.ts
- apps/server/src/routes/sessions.test.ts
- apps/server/src/routes/files.test.ts
- packages/agent-core/src/tools/permission/index.ts
- packages/agent-core/src/tools/permission/mcp.test.ts
- packages/agent-core/src/mcp/manager.ts
- packages/agent-core/src/mcp/manager.test.ts
| <Field label="Transport"><select className={selectClass} value={server.type} onChange={(event) => replaceTransport(event.target.value as "http" | "stdio")}><option value="http">HTTP</option><option value="stdio">STDIO</option></select></Field> | ||
| {server.type === "http" ? <Field label="HTTP URL" error={errors[`mcp.servers.${name}.url`]}><TextInput value={server.url} onChange={(next) => update((draft) => { if (draft.type === "http") draft.url = next; })} /></Field> : <> | ||
| <Field label="Command" error={errors[`mcp.servers.${name}.command`]}><TextInput value={server.command} onChange={(next) => update((draft) => { if (draft.type === "stdio") draft.command = next; })} /></Field> | ||
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter empty argument lines.
event.target.value.split("\n") keeps empty strings. A trailing newline or a blank line produces an empty "" argument that is passed to the STDIO process. Drop empty lines before storing.
🛠️ Proposed fix
-<Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })}
+<Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") { const args = event.target.value.split("\n").filter((line) => line.trim() !== ""); draft.args = args.length > 0 ? args : undefined; } })}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> | |
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") { const args = event.target.value.split("\n").filter((line) => line.trim() !== ""); draft.args = args.length > 0 ? args : undefined; } })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/features/settings-panels.tsx` at line 601, Update the
stdio arguments handler in the settings panel’s textarea onChange flow to remove
empty lines after splitting the input by newline before assigning draft.args.
Preserve undefined for entirely empty input, and ensure stored arguments contain
only non-empty lines.
Comment on lines
50
to
+58
| useEffect(() => { | ||
| setDraft(cloneConfig(snapshot.config)); | ||
| setErrors({}); | ||
| setJsonErrors({}); | ||
| setSaveError(undefined); | ||
| if (preserveSaveErrorRevision.current === snapshot.revision) { | ||
| preserveSaveErrorRevision.current = undefined; | ||
| } else { | ||
| setSaveError(undefined); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear preserveSaveErrorRevision on every snapshot change.
The else branch leaves the marker set. If the reload returns a different revision, the marker survives. A later snapshot that happens to carry the recorded revision then preserves a stale save error. Reset the ref in both branches.
♻️ Proposed fix
if (preserveSaveErrorRevision.current === snapshot.revision) {
preserveSaveErrorRevision.current = undefined;
} else {
+ preserveSaveErrorRevision.current = undefined;
setSaveError(undefined);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| setDraft(cloneConfig(snapshot.config)); | |
| setErrors({}); | |
| setJsonErrors({}); | |
| setSaveError(undefined); | |
| if (preserveSaveErrorRevision.current === snapshot.revision) { | |
| preserveSaveErrorRevision.current = undefined; | |
| } else { | |
| setSaveError(undefined); | |
| } | |
| useEffect(() => { | |
| setDraft(cloneConfig(snapshot.config)); | |
| setErrors({}); | |
| setJsonErrors({}); | |
| if (preserveSaveErrorRevision.current === snapshot.revision) { | |
| preserveSaveErrorRevision.current = undefined; | |
| } else { | |
| preserveSaveErrorRevision.current = undefined; | |
| setSaveError(undefined); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/features/SettingsDialog.tsx` around lines 50 - 58,
Update the snapshot-change useEffect to clear preserveSaveErrorRevision.current
in both branches, including when the current revision matches; ensure no
recorded revision marker survives after processing any snapshot change.
Comment on lines
+88
to
+93
| if (server.type === "http") { | ||
| literals.push({ path: `${prefix}.url`, value: server.url }); | ||
| collectRecordValues(literals, `${prefix}.headers`, server.headers); | ||
| } else { | ||
| collectRecordValues(literals, `${prefix}.env`, server.env); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SecretLiteralRegistry bounds and MCP literal validation.
rg -n -C8 'class SecretLiteralRegistry' packages/agent-core/src/config/runtime-secret-literals.ts
rg -n -C6 'validateMcpSecretLiteralPolicy' packages/agent-core/src/config/server-config-service.tsRepository: boh5/archcode
Length of output: 1813
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- runtime-secret-literals.ts ---'
sed -n '1,125p' packages/agent-core/src/config/runtime-secret-literals.ts
printf '%s\n' '--- server-config-service.ts ---'
sed -n '840,910p' packages/agent-core/src/config/server-config-service.ts
printf '%s\n' '--- related references ---'
rg -n -C4 'SecretLiteralRegistry|runtime secret literal|MCP.*literal|short literal|minimum.*length|MIN.*LENGTH' packages/agent-core/src packages/agent-core/test packages 2>/dev/null | head -300Repository: boh5/archcode
Length of output: 33678
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- secret literal bounds ---'
rg -n -C5 'SECRET_LITERAL_(MIN|MAX)' packages/agent-core/src/security packages/agent-core/src
printf '%s\n' '--- runtime collection tests and activation callers ---'
rg -n -C6 'collectRuntimeSecretLiterals|Runtime secret literal configuration|mcp\.servers\..*\.url|server\.url' packages/agent-core/src --glob '*.{ts,tsx}' | head -400
printf '%s\n' '--- MCP save-validation tests ---'
sed -n '1450,1515p' packages/agent-core/src/config/server-config-service.test.tsRepository: boh5/archcode
Length of output: 24755
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- MCP URL schema and resolution ---'
rg -n -C8 'url:|mcpHttpServerConfigSchema|mcp.*http|http.*server' packages/agent-core/src/config/mcp.ts
printf '%s\n' '--- runtime startup and redaction setup ---'
sed -n '525,560p' packages/agent-core/src/runtime.ts
printf '%s\n' '--- runtime literal tests ---'
sed -n '1,145p' packages/agent-core/src/config/runtime-secret-literals.test.tsRepository: boh5/archcode
Length of output: 8649
🏁 Script executed:
#!/bin/bash
sed -n '268,286p' packages/agent-core/src/config/mcp.ts
node - <<'JS'
const candidates = ["http://a", "http://", "https://a", "h://a", "x:", "http:a"];
for (const value of candidates) {
try {
const parsed = new URL(value);
const accepted = parsed.protocol === "http:" || parsed.protocol === "https:";
console.log(JSON.stringify({ value, bytes: Buffer.byteLength(value, "utf8"), parsed: true, accepted }));
} catch {
console.log(JSON.stringify({ value, bytes: Buffer.byteLength(value, "utf8"), parsed: false, accepted: false }));
}
}
JSRepository: boh5/archcode
Length of output: 1162
Align MCP URL validation with runtime literal collection.
SECRET_LITERAL_MIN_BYTES is 8, but validateMcpSecretLiteralPolicy excludes server.url. The current URL validation accepts http:a (6 bytes), so save succeeds and runtime initialization rejects the same configuration. Exclude server.url from collectRuntimeSecretLiterals, or validate it during save. Add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/config/runtime-secret-literals.ts` around lines 88 -
93, Update collectRuntimeSecretLiterals around the HTTP server branch so
server.url is excluded from runtime secret literal collection, keeping header
and environment collection unchanged; add a regression test confirming an HTTP
URL such as http:a follows the same save and runtime validation behavior.
| readonly #familyControls = new Map<string, SessionFamilyControlState>(); | ||
| readonly #runtimeMutations = new Set<symbol>(); | ||
| readonly #runtimeChangeListeners = new Set<SessionRuntimeChangeListener>(); | ||
| readonly #executionSkillSnapshots = new Map<string, ReadonlyMap<string, SkillPackageSnapshot>>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
#executionSkillSnapshots is not evicted when a Session is deleted.
Entries are removed in #finalizeExecution, #terminalizeSuspendedForInspection, and #terminalizeSuspendedFamily. All three require the execution to reach a terminal boundary. deleteSession (line 2476) disposes agents, removes stores, and deletes session directories, but it leaves cached snapshot maps behind. If a Session with a suspended execution is deleted, its SkillPackageSnapshot map (including the skill body text) stays reachable for the process lifetime.
Delete the cached entries for every removed session id inside deleteSession.
🧹 Proposed eviction on session deletion
for (const id of sessionIds) {
this.#config.sessionAgentManager.dispose(workspaceRoot, id);
this.#config.untrackSession(workspaceRoot, id);
+ const prefix = `${scopedKey(workspaceRoot, id)}\0`;
+ for (const key of this.#executionSkillSnapshots.keys()) {
+ if (key.startsWith(prefix)) this.#executionSkillSnapshots.delete(key);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/execution/session-execution-manager.ts` at line 439,
Update deleteSession to remove every deleted session ID from
`#executionSkillSnapshots` before completing session cleanup. Ensure suspended and
non-terminal executions are evicted even when terminalization methods are not
invoked, while preserving the existing agent, store, and directory deletion
behavior.
Comment on lines
+185
to
+205
| async testServer( | ||
| serverName: string, | ||
| config: ResolvedMcpServerConfig, | ||
| options: { signal?: AbortSignal } = {}, | ||
| ): Promise<McpTestResult> { | ||
| this.#assertOpen(); | ||
| const testKey = `${serverName}:${stableSerialize(config)}`; | ||
| if (this.#testsInFlight.has(testKey)) { | ||
| throw new Error(`MCP server test already in progress for "${serverName}"`); | ||
| } | ||
| const controller = new AbortController(); | ||
| const removeAbortForwarder = forwardAbort(options.signal, controller); | ||
| const flight: DraftTestFlight = { controller }; | ||
| const promise = this.#runDraftTest(serverName, config, flight).finally(() => { | ||
| removeAbortForwarder(); | ||
| if (this.#testsInFlight.get(testKey) === flight) this.#testsInFlight.delete(testKey); | ||
| }); | ||
| flight.promise = promise; | ||
| this.#testsInFlight.set(testKey, flight); | ||
| return await promise; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the number of concurrent draft tests.
testKey includes stableSerialize(config), so the duplicate guard only blocks an identical repeat. A caller that varies any field (URL, header value, args, serverName) starts an unlimited number of parallel drafts. Each draft opens a live HTTP connection or spawns a stdio child process through McpClient, and nothing releases the slot until the connect and discovery timeouts expire. A settings client that submits drafts in a loop can exhaust sockets or process slots for the whole runtime.
Add a maximum in-flight draft count and reject beyond it.
🛡️ Sketch
+ static readonly `#MAX_DRAFT_TESTS` = 4;
+
async testServer(
serverName: string,
config: ResolvedMcpServerConfig,
options: { signal?: AbortSignal } = {},
): Promise<McpTestResult> {
this.#assertOpen();
const testKey = `${serverName}:${stableSerialize(config)}`;
if (this.#testsInFlight.has(testKey)) {
throw new Error(`MCP server test already in progress for "${serverName}"`);
}
+ if (this.#testsInFlight.size >= McpRuntimeService.#MAX_DRAFT_TESTS) {
+ throw new Error("Too many MCP server tests are in progress");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async testServer( | |
| serverName: string, | |
| config: ResolvedMcpServerConfig, | |
| options: { signal?: AbortSignal } = {}, | |
| ): Promise<McpTestResult> { | |
| this.#assertOpen(); | |
| const testKey = `${serverName}:${stableSerialize(config)}`; | |
| if (this.#testsInFlight.has(testKey)) { | |
| throw new Error(`MCP server test already in progress for "${serverName}"`); | |
| } | |
| const controller = new AbortController(); | |
| const removeAbortForwarder = forwardAbort(options.signal, controller); | |
| const flight: DraftTestFlight = { controller }; | |
| const promise = this.#runDraftTest(serverName, config, flight).finally(() => { | |
| removeAbortForwarder(); | |
| if (this.#testsInFlight.get(testKey) === flight) this.#testsInFlight.delete(testKey); | |
| }); | |
| flight.promise = promise; | |
| this.#testsInFlight.set(testKey, flight); | |
| return await promise; | |
| } | |
| static readonly `#MAX_DRAFT_TESTS` = 4; | |
| async testServer( | |
| serverName: string, | |
| config: ResolvedMcpServerConfig, | |
| options: { signal?: AbortSignal } = {}, | |
| ): Promise<McpTestResult> { | |
| this.#assertOpen(); | |
| const testKey = `${serverName}:${stableSerialize(config)}`; | |
| if (this.#testsInFlight.has(testKey)) { | |
| throw new Error(`MCP server test already in progress for "${serverName}"`); | |
| } | |
| if (this.#testsInFlight.size >= McpRuntimeService.#MAX_DRAFT_TESTS) { | |
| throw new Error("Too many MCP server tests are in progress"); | |
| } | |
| const controller = new AbortController(); | |
| const removeAbortForwarder = forwardAbort(options.signal, controller); | |
| const flight: DraftTestFlight = { controller }; | |
| const promise = this.#runDraftTest(serverName, config, flight).finally(() => { | |
| removeAbortForwarder(); | |
| if (this.#testsInFlight.get(testKey) === flight) this.#testsInFlight.delete(testKey); | |
| }); | |
| flight.promise = promise; | |
| this.#testsInFlight.set(testKey, flight); | |
| return await promise; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/mcp/runtime-service.ts` around lines 185 - 205,
Update testServer and its in-flight test state to enforce a shared maximum
number of concurrent draft tests, rejecting new drafts once the limit is reached
while preserving the existing duplicate testKey guard. Ensure the count is
incremented only for accepted tests and decremented in the existing
promise.finally cleanup so slots are released on every completion path.
Comment on lines
+922
to
+931
| available: z.strictObject({ | ||
| includedEntries: z.array(z.strictObject({ | ||
| name: z.string(), | ||
| description: z.string(), | ||
| source: z.enum(SKILL_SOURCE_TIERS), | ||
| })), | ||
| omittedCount: z.number().int().nonnegative(), | ||
| renderedText: z.string(), | ||
| byteLength: z.number().int().nonnegative().max(8_000), | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
byteLength does not bound renderedText.
byteLength is a self-declared number capped at 8,000. renderedText has no bound. A persisted trace can declare byteLength: 6 and still carry an arbitrarily large renderedText, so the cap does not limit the persisted payload. Other bounded strings in this file use boundedUtf8String(...).
Tie the two values together, or bound renderedText directly.
🛡️ Proposed fix
available: z.strictObject({
includedEntries: z.array(z.strictObject({
name: z.string(),
description: z.string(),
source: z.enum(SKILL_SOURCE_TIERS),
})),
omittedCount: z.number().int().nonnegative(),
- renderedText: z.string(),
+ renderedText: boundedUtf8String(8_000),
byteLength: z.number().int().nonnegative().max(8_000),
- }),
+ }).refine(
+ (available) => utf8Bytes(available.renderedText) === available.byteLength,
+ "byteLength must equal the rendered catalog size",
+ ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/store/helpers.ts` around lines 922 - 931, Update the
available schema’s renderedText field alongside byteLength so persisted text is
directly limited to the 8,000-byte maximum, using the existing boundedUtf8String
helper and preserving the current byteLength validation.
| import { formatResolvedSkillResource, SkillReadInputSchema, skillReadTool } from "./skill-read"; | ||
|
|
||
| const tmpRoot = join(import.meta.dir, "__test_tmp__", "skill-read", crypto.randomUUID()); | ||
| const tmpRoot = join(tmpdir(), "archcode-skill-read-tool", crypto.randomUUID()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SkillService test constructions that omit userAgentsSkillsRoot.
rg -nP --type=ts -C2 'new SkillService\(' -g '**/*.test.ts' -g '**/*.ts'Repository: boh5/archcode
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'packages/agent-core/src/tools/builtins/*skill*' 'packages/agent-core/src/**/*SkillService*'
printf '%s\n' '--- SkillService references ---'
rg -n -C3 'SkillService|userAgentsSkillsRoot|userSkillsRoot' packages/agent-core/src
printf '%s\n' '--- test file outline ---'
if [ -f packages/agent-core/src/tools/builtins/skill-read.test.ts ]; then
ast-grep outline packages/agent-core/src/tools/builtins/skill-read.test.ts
fiRepository: boh5/archcode
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- skill-read.test.ts ---'
sed -n '1,90p' packages/agent-core/src/tools/builtins/skill-read.test.ts
printf '%s\n' '--- SkillService implementation candidates ---'
fd -t f -e ts . packages/agent-core/src/skills | sort
printf '%s\n' '--- defaults and tier resolution ---'
rg -n -C5 'userAgentsSkillsRoot|homedir\(\)|agents.*skills|resolve.*Skill|listSkills|readSkill' packages/agent-core/src/skillsRepository: boh5/archcode
Length of output: 12825
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- SkillService catalog/read paths ---'
sed -n '112,230p' packages/agent-core/src/skills/service.ts
sed -n '320,370p' packages/agent-core/src/skills/service.ts
printf '%s\n' '--- skill-read assertions and service constructions ---'
rg -n -C4 'catalog|read|not found|new SkillService|skillReadTool.execute|expect\(.*toEqual|toContain' packages/agent-core/src/tools/builtins/skill-read.test.tsRepository: boh5/archcode
Length of output: 18116
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
service = Path("packages/agent-core/src/skills/service.ts").read_text()
test = Path("packages/agent-core/src/tools/builtins/skill-read.test.ts").read_text()
default = re.search(
r"userAgentsSkillsRoot\s*=\s*resolve\(options\.userAgentsSkillsRoot \?\? join\(homedir\(\), \"\.agents\", \"skills\"\)\)",
service,
)
source_order = re.search(
r'\{ source: "user-archcode".*?\n\s*\{ source: "user-agents".*?root: this\.userAgentsSkillsRoot',
service,
re.S,
)
context = re.search(
r"new SkillService\(\{\s*userSkillsRoot\s*\}\)",
test,
)
unknown_case = re.search(
r'test\("unknown skill name returns structured error".*?makeContext\(\["missing-skill"\]\)',
test,
re.S,
)
print({
"defaults_user_agents_root_to_home": bool(default),
"user_agents_is_a_resolution_tier": bool(source_order),
"skill_read_context_omits_user_agents_root": bool(context),
"unknown_skill_case_depends_on_resolution": bool(unknown_case),
})
if not all((default, source_order, context, unknown_case)):
raise SystemExit("expected evidence was not found")
PYRepository: boh5/archcode
Length of output: 338
Isolate the user-agents skill root in this suite.
Pass a temporary userAgentsSkillsRoot to SkillService. Otherwise, skill_read can resolve skills from $HOME/.agents/skills, making the “unknown skill” test environment-dependent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/tools/builtins/skill-read.test.ts` at line 14, Update
the SkillService setup in the skill-read test suite to pass an isolated
temporary userAgentsSkillsRoot under tmpRoot, ensuring skill_read resolves only
from the test directory. Keep the unknown-skill test independent of any skills
installed in $HOME/.agents/skills.
Comment on lines
+119
to
+132
| executeResolved( | ||
| descriptor: AnyToolDescriptor, | ||
| toolCall: ToolCallLike, | ||
| context: ToolExecutionContext, | ||
| ): Promise<RegistryExecutionOutcome> { | ||
| if (descriptor.name !== toolCall.toolName) { | ||
| return this.settleSystem(toolCall, context, createToolErrorResult({ | ||
| kind: "unknown-tool", | ||
| code: "TOOL_UNKNOWN", | ||
| message: `Resolved tool "${descriptor.name}" does not match call "${toolCall.toolName}"`, | ||
| })); | ||
| } | ||
| return this.#execute(toolCall, context, undefined, descriptor); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate run-local descriptors before execution.
executeResolved() bypasses the checks in register(). A descriptor with traits.destructive: true and no permissions can reach #execute() and execute without a permission decision.
Extract the descriptor invariant checks from register(). Apply them before executing a resolved descriptor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/agent-core/src/tools/registry.ts` around lines 119 - 132, Extract
the descriptor invariant validation currently performed by register() into a
reusable helper, then invoke it from executeResolved() before calling
`#execute`(). Ensure resolved descriptors with traits.destructive set but no
permissions are rejected consistently with registered descriptors, while
preserving the existing tool-name mismatch handling.
Comment on lines
+219
to
+227
| function isExecutionSkillBinding(value: unknown): boolean { | ||
| const binding = record(value); | ||
| return binding !== undefined | ||
| && exact(binding, ["name", "source", "digest", "resolutionRoot"]) | ||
| && isString(binding.name) | ||
| && oneOf(binding.source, ["project-archcode", "project-agents", "user-archcode", "user-agents", "builtin"]) | ||
| && isString(binding.digest) | ||
| && isString(binding.resolutionRoot); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the execution Skill digest format.
isExecutionSkillBinding accepts any string for digest. It therefore accepts malformed claim-time Skill identities. Require the declared 64-character digest format and add rejection coverage for short or malformed values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/protocol/src/guards.ts` around lines 219 - 227, Update
isExecutionSkillBinding to validate digest as a 64-character hexadecimal string
rather than any string, while preserving the existing field and source checks.
Add rejection coverage for short and malformed digest values.
Comment on lines
+973
to
+997
| export interface ConfigMcpTimeoutSettings { | ||
| connectTimeoutMs?: number; | ||
| discoveryTimeoutMs?: number; | ||
| callTimeoutMs?: number; | ||
| } | ||
|
|
||
| export interface ConfigMcpHttpServerSettings<Secret> extends ConfigMcpTimeoutSettings { | ||
| type: "http"; | ||
| enabled: boolean; | ||
| url: string; | ||
| headers?: Record<string, Secret>; | ||
| timeout?: number; | ||
| } | ||
|
|
||
| export interface ConfigMcpStdioServerSettings<Secret> extends ConfigMcpTimeoutSettings { | ||
| type: "stdio"; | ||
| enabled: boolean; | ||
| command: string; | ||
| args?: string[]; | ||
| env?: Record<string, Secret>; | ||
| } | ||
|
|
||
| export type ConfigMcpServerSettings<Secret> = | ||
| | ConfigMcpHttpServerSettings<Secret> | ||
| | ConfigMcpStdioServerSettings<Secret>; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether README.md still documents the removed MCP configuration shape.
rg -n -C5 '"mcp"|mcp\.servers|timeout|disabledBuiltins' README.mdRepository: boh5/archcode
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked documentation files ---'
git ls-files | rg '(^|/)(README\.md|config\.example\.json|configuration\.md)$' || true
printf '%s\n' '--- README references ---'
if [ -f README.md ]; then
rg -n -i -C3 'MCP|mcp|disabledBuiltins|connectTimeoutMs|discoveryTimeoutMs|callTimeoutMs|timeout' README.md || true
else
printf '%s\n' 'README.md not found at repository root'
fi
printf '%s\n' '--- relevant configuration documentation ---'
for f in $(git ls-files | rg '(^|/)(config\.example\.json|configuration\.md)$'); do
printf '\n### %s\n' "$f"
rg -n -i -C2 'MCP|mcp|disabledBuiltins|connectTimeoutMs|discoveryTimeoutMs|callTimeoutMs|timeout' "$f" || true
done
printf '%s\n' '--- changed-file summary ---'
git diff --stat
git diff --name-onlyRepository: boh5/archcode
Length of output: 3861
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- protocol MCP types ---'
sed -n '960,1030p' packages/protocol/src/types.ts
printf '%s\n' '--- README configuration and integration sections ---'
sed -n '125,185p' README.md
printf '%s\n' '--- configuration example MCP section ---'
sed -n '35,70p' config.example.json
printf '%s\n' '--- repository references to legacy and current MCP fields ---'
rg -n '(^|[^A-Za-z])(timeout|connectTimeoutMs|discoveryTimeoutMs|callTimeoutMs|disabledBuiltins)([^A-Za-z]|$)' --glob '*.ts' --glob '*.tsx' --glob '*.md' --glob '*.json' .Repository: boh5/archcode
Length of output: 50369
Update README.md with the MCP configuration schema.
The README does not document the breaking MCP changes: required type and enabled, the three timeout fields replacing timeout, and mcp.disabledBuiltins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/protocol/src/types.ts` around lines 973 - 997, Update README.md to
document the MCP configuration schema represented by ConfigMcpServerSettings,
including required type and enabled fields, HTTP and stdio-specific properties,
the connectTimeoutMs, discoveryTimeoutMs, and callTimeoutMs fields replacing
timeout, and the mcp.disabledBuiltins setting.
Source: Coding guidelines
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
23 issues found across 168 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/agent-core/src/runtime.ts">
<violation number="1" location="packages/agent-core/src/runtime.ts:1384">
P2: Invalid `/skill use ...` commands cannot be retried with the same `clientRequestId`: the first attempt is stored as a completed command, but this specialized replay lookup treats that command receipt as an idempotency conflict. Preserve the generic command-replay path for validation failures or existing command receipts before attempting Skill-message replay.</violation>
</file>
<file name="packages/agent-core/src/config/mcp.test.ts">
<violation number="1" location="packages/agent-core/src/config/mcp.test.ts:303">
P3: The URL-scheme allow-list is now tested only against literal URLs. The previous test that validated the http/https restriction on a scheme produced by env expansion ("rejects non-http URL after env expansion", e.g. url "${MCP_PROTO}://chat" with MCP_PROTO=ws) was removed and not replaced, and the new resolveMcpConfig env-expansion tests only expand to valid http/https schemes. Since resolveMcpConfig expands the URL before validateHttpUrl (expandString then validateHttpUrl), a scheme-injection through an env override is a real security-boundary path that currently has no regression coverage.</violation>
</file>
<file name="packages/agent-core/src/skills/pagination.ts">
<violation number="1" location="packages/agent-core/src/skills/pagination.ts:52">
P2: Empty pages can exceed `maxSerializedBytes` because this fast path bypasses serialization; validate the empty page against the same byte limit (or throw) so the helper preserves its advertised bound.</violation>
</file>
<file name="apps/server/src/routes/mcp.ts">
<violation number="1" location="apps/server/src/routes/mcp.ts:48">
P2: Testing a stale or semantically invalid draft returns `INTERNAL_ERROR` instead of actionable revision/validation errors, so Settings cannot distinguish a fixable draft problem from a runtime failure. Catch `ConfigRevisionConflictError` and `ConfigSemanticValidationError` here and map them through the existing config HTTP errors.</violation>
</file>
<file name="packages/agent-core/src/skills/projection.ts">
<violation number="1" location="packages/agent-core/src/skills/projection.ts:17">
P2: Large Skill directories make prompt construction quadratic despite the 8 KB output cap, delaying or timing out every model call. Build the included prefix incrementally (then account for the omission footer) so projection work remains linear in discovered Skills.</violation>
</file>
<file name="packages/protocol/src/types.ts">
<violation number="1" location="packages/protocol/src/types.ts:29">
P1: Pre-update session files will no longer open because these new persisted fields are required but no load-time migration defaults legacy snapshots. Migrate missing Skill fields before strict validation (including queued messages and prompt traces) or version the session format.</violation>
</file>
<file name="packages/agent-core/src/tools/registry.ts">
<violation number="1" location="packages/agent-core/src/tools/registry.ts:131">
P1: Destructive MCP tools execute without the confirmation guard because `executeResolved` bypasses `register`'s required-permission invariant. Validate resolved descriptors or attach the same permission policy before executing them.</violation>
</file>
<file name="packages/agent-core/src/testing/test-mcp-runtime.ts">
<violation number="1" location="packages/agent-core/src/testing/test-mcp-runtime.ts:55">
P2: Snapshots share the caller-owned descriptor map instead of being run-local; mutations through the supplied `Map` or a snapshot leak into later model boundaries. Return a fresh map while retaining descriptor identities, matching `McpRuntimeService`.</violation>
</file>
<file name="packages/agent-core/src/execution/session-execution-manager.ts">
<violation number="1" location="packages/agent-core/src/execution/session-execution-manager.ts:439">
P2: Deleting a suspended Session leaves its full execution Skill snapshot retained for the manager lifetime because `deleteSession` never removes entries from this map; clear all snapshot keys for deleted session IDs during deletion.</violation>
</file>
<file name="packages/agent-core/src/skills/service.test.ts">
<violation number="1" location="packages/agent-core/src/skills/service.test.ts:91">
P2: The rewritten reserved-builtin test drops the Agent-gating assertions that the replaced test covered: it only checks unshadowability for the first reserved name and no longer verifies that a reserved lifecycle builtin is excluded when the agent's allowedNames don't include it (readForAgent → null) and omitted from listForAgent. That gating is an ownership/security boundary (reserved lifecycle skills must only be reachable by the owning agent), so a regression there would now pass CI. Consider restoring the loop over all RESERVED_BUILTIN_SKILL_NAMES and re-adding the `readForAgent(... ["codemap"])` → null and listing-exclusion assertions.</violation>
</file>
<file name="packages/agent-core/src/skills/package-reader.ts">
<violation number="1" location="packages/agent-core/src/skills/package-reader.ts:187">
P2: Atomic Skill package replacement during capture can produce a snapshot with the old `SKILL.md` and new resources; retain initial root identity and verify it after capture before publishing the execution snapshot.
(Based on your team's feedback about bounded Skill filesystem race defenses.)</violation>
<violation number="2" location="packages/agent-core/src/skills/package-reader.ts:230">
P3: Some invalid resource paths still return `TOOL_SKILL_READ_FAILED` instead of the new invalid-input result; throw `SkillPackageResourcePathError` for the depth and entry-path validation branches too.</violation>
</file>
<file name="packages/agent-core/src/config/server-config-service.ts">
<violation number="1" location="packages/agent-core/src/config/server-config-service.ts:863">
P1: Saving MCP credentials can commit a configuration that prevents Runtime startup after restart because this checks MCP literals in isolation rather than the combined runtime registry. Validate the full persisted runtime literal set (and account for externally supplied literals) before committing.</violation>
</file>
<file name="packages/agent-core/src/mcp/tool-adapter.ts">
<violation number="1" location="packages/agent-core/src/mcp/tool-adapter.ts:100">
P1: A post-dispatch effectful call with a generic SDK/network failure is treated as an ordinary retryable error, although the remote server may already have applied it. Mark every dispatched effectful failure as `unknownResult` so scheduler requires inspection before continuation.</violation>
</file>
<file name="packages/agent-core/src/execution/session-tool-batch-scheduler.ts">
<violation number="1" location="packages/agent-core/src/execution/session-tool-batch-scheduler.ts:1358">
P2: A failed tool-result checkpoint can erase a concurrent event-less batch transition, such as another parallel call becoming blocked, because the event-id guard does not detect state patches. Preserve/rebase later store changes rather than replacing the full state snapshot on persistence failure.</violation>
</file>
<file name="packages/agent-core/src/skills/types.ts">
<violation number="1" location="packages/agent-core/src/skills/types.ts:1">
P2: Keep source tiers in one shared contract; this duplicate tuple can let protocol validation and agent Skill resolution diverge when a tier changes. Re-export the protocol tuple here (and use it for resolution order where applicable).</violation>
</file>
<file name="apps/web/src/components/features/settings-panels.tsx">
<violation number="1" location="apps/web/src/components/features/settings-panels.tsx:558">
P2: Closing Settings or leaving MCP does not cancel an in-flight draft test, so a remote HTTP/STDIO discovery can continue until its timeout; wire an AbortController through `testMcpDraft` and abort it during panel cleanup/inactivation.</violation>
<violation number="2" location="apps/web/src/components/features/settings-panels.tsx:563">
P2: A reconnect can overwrite newer status updates for other MCP servers with its stale snapshot, leaving their displayed state incorrect until another event; merge/version status snapshots rather than replacing the SSE-owned map.</violation>
<violation number="3" location="apps/web/src/components/features/settings-panels.tsx:601">
P2: The STDIO arguments parser currently keeps blank lines as `""`, so a trailing newline or spacer line is persisted as an actual argv entry. Filtering out empty lines before assigning `draft.args` avoids passing unintended empty arguments to the child process.</violation>
</file>
<file name="packages/agent-core/src/mcp/client.ts">
<violation number="1" location="packages/agent-core/src/mcp/client.ts:372">
P1: STDIO stderr can leak configured tokens when a secret spans two data chunks, because each chunk is redacted independently. Buffer a bounded overlap of prior bytes and redact before emitting log output.</violation>
<violation number="2" location="packages/agent-core/src/mcp/client.ts:378">
P2: A continuous STDERR writer can generate unlimited logs: this 4 KiB cap applies per chunk, not per connection. Track a cumulative stderr budget or rate-limit/drop subsequent output after it is exhausted.</violation>
</file>
<file name="packages/agent-core/src/execution/session-execution-manager.test.ts">
<violation number="1" location="packages/agent-core/src/execution/session-execution-manager.test.ts:1133">
P2: The restart test waits for the terminal `failed` status with a fixed 20-iteration `setTimeout(resolve, 0)` microtask poll. The failed record is written and flushed in `#runExecution`'s finally block on a different tick from the reconcile rejection, so the loop is a timing bet that can flake under load. Consider awaiting the execution's terminal settlement deterministically (e.g. expose/await a settled signal) or polling with a real deadline and larger/macrotask spacing so the assertion isn't sensitive to scheduler timing.</violation>
</file>
<file name="apps/web/src/components/features/SettingsDialog.tsx">
<violation number="1" location="apps/web/src/components/features/SettingsDialog.tsx:57">
P2: The snapshot-change effect clears `saveError` in the mismatch branch but leaves `preserveSaveErrorRevision.current` set. That stale marker can match a later snapshot revision and incorrectly preserve an old MCP-apply error. Clearing the ref in both branches keeps error preservation scoped to the intended revision.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| maxSteps: number; | ||
| activeTimeoutMs?: number; | ||
| binding: ExecutionModelBindingSummary; | ||
| executionSkills: ExecutionSkillBinding[]; |
There was a problem hiding this comment.
P1: Pre-update session files will no longer open because these new persisted fields are required but no load-time migration defaults legacy snapshots. Migrate missing Skill fields before strict validation (including queued messages and prompt traces) or version the session format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/protocol/src/types.ts, line 29:
<comment>Pre-update session files will no longer open because these new persisted fields are required but no load-time migration defaults legacy snapshots. Migrate missing Skill fields before strict validation (including queued messages and prompt traces) or version the session format.</comment>
<file context>
@@ -26,6 +26,55 @@ export interface ExecutionStartEvent {
maxSteps: number;
activeTimeoutMs?: number;
binding: ExecutionModelBindingSummary;
+ executionSkills: ExecutionSkillBinding[];
+}
+
</file context>
| message: `Resolved tool "${descriptor.name}" does not match call "${toolCall.toolName}"`, | ||
| })); | ||
| } | ||
| return this.#execute(toolCall, context, undefined, descriptor); |
There was a problem hiding this comment.
P1: Destructive MCP tools execute without the confirmation guard because executeResolved bypasses register's required-permission invariant. Validate resolved descriptors or attach the same permission policy before executing them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/tools/registry.ts, line 131:
<comment>Destructive MCP tools execute without the confirmation guard because `executeResolved` bypasses `register`'s required-permission invariant. Validate resolved descriptors or attach the same permission policy before executing them.</comment>
<file context>
@@ -122,6 +116,21 @@ export class ToolRegistry {
+ message: `Resolved tool "${descriptor.name}" does not match call "${toolCall.toolName}"`,
+ }));
+ }
+ return this.#execute(toolCall, context, undefined, descriptor);
+ }
+
</file context>
| } | ||
| if (literals.length === 0) return; | ||
| try { | ||
| new SecretLiteralRegistry(literals); |
There was a problem hiding this comment.
P1: Saving MCP credentials can commit a configuration that prevents Runtime startup after restart because this checks MCP literals in isolation rather than the combined runtime registry. Validate the full persisted runtime literal set (and account for externally supplied literals) before committing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/config/server-config-service.ts, line 863:
<comment>Saving MCP credentials can commit a configuration that prevents Runtime startup after restart because this checks MCP literals in isolation rather than the combined runtime registry. Validate the full persisted runtime literal set (and account for externally supplied literals) before committing.</comment>
<file context>
@@ -761,6 +841,35 @@ function validateConfig(value: unknown): ArchCodeConfig {
+ }
+ if (literals.length === 0) return;
+ try {
+ new SecretLiteralRegistry(literals);
+ } catch (cause) {
+ if (cause instanceof ConfigSemanticValidationError) {
</file context>
| : reason === "timeout" | ||
| ? "TOOL_MCP_CALL_TIMEOUT" | ||
| : "TOOL_MCP_ERROR"; | ||
| const unknownResult = attempted && !traits.readOnly && (reason === "aborted" || reason === "timeout"); |
There was a problem hiding this comment.
P1: A post-dispatch effectful call with a generic SDK/network failure is treated as an ordinary retryable error, although the remote server may already have applied it. Mark every dispatched effectful failure as unknownResult so scheduler requires inspection before continuation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/mcp/tool-adapter.ts, line 100:
<comment>A post-dispatch effectful call with a generic SDK/network failure is treated as an ordinary retryable error, although the remote server may already have applied it. Mark every dispatched effectful failure as `unknownResult` so scheduler requires inspection before continuation.</comment>
<file context>
@@ -2,100 +2,126 @@ import { jsonSchema } from "ai";
+ : reason === "timeout"
+ ? "TOOL_MCP_CALL_TIMEOUT"
+ : "TOOL_MCP_ERROR";
+ const unknownResult = attempted && !traits.readOnly && (reason === "aborted" || reason === "timeout");
return createMcpErrorResult(
serverName,
</file context>
Suggested change
| const unknownResult = attempted && !traits.readOnly && (reason === "aborted" || reason === "timeout"); | |
| const unknownResult = attempted && !traits.readOnly; |
| } | ||
|
|
||
| #attachBoundedStderr(): void { | ||
| this.#transport.stderr?.on("data", (chunk) => { |
There was a problem hiding this comment.
P1: STDIO stderr can leak configured tokens when a secret spans two data chunks, because each chunk is redacted independently. Buffer a bounded overlap of prior bytes and redact before emitting log output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/mcp/client.ts, line 372:
<comment>STDIO stderr can leak configured tokens when a secret spans two data chunks, because each chunk is redacted independently. Buffer a bounded overlap of prior bytes and redact before emitting log output.</comment>
<file context>
@@ -257,68 +280,171 @@ export class McpClient {
+ }
+
+ #attachBoundedStderr(): void {
+ this.#transport.stderr?.on("data", (chunk) => {
+ const raw = typeof chunk === "string"
+ ? chunk
</file context>
| restartError = error; | ||
| } | ||
| expect(restartError).toMatchObject({ code: "SKILL_PACKAGE_CHANGED" }); | ||
| for (let attempt = 0; attempt < 20 && coldStore.getState().executions[0]?.status !== "failed"; attempt += 1) { |
There was a problem hiding this comment.
P2: The restart test waits for the terminal failed status with a fixed 20-iteration setTimeout(resolve, 0) microtask poll. The failed record is written and flushed in #runExecution's finally block on a different tick from the reconcile rejection, so the loop is a timing bet that can flake under load. Consider awaiting the execution's terminal settlement deterministically (e.g. expose/await a settled signal) or polling with a real deadline and larger/macrotask spacing so the assertion isn't sensitive to scheduler timing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/execution/session-execution-manager.test.ts, line 1133:
<comment>The restart test waits for the terminal `failed` status with a fixed 20-iteration `setTimeout(resolve, 0)` microtask poll. The failed record is written and flushed in `#runExecution`'s finally block on a different tick from the reconcile rejection, so the loop is a timing bet that can flake under load. Consider awaiting the execution's terminal settlement deterministically (e.g. expose/await a settled signal) or polling with a real deadline and larger/macrotask spacing so the assertion isn't sensitive to scheduler timing.</comment>
<file context>
@@ -842,6 +845,301 @@ describe("SessionExecutionManager", () => {
+ restartError = error;
+ }
+ expect(restartError).toMatchObject({ code: "SKILL_PACKAGE_CHANGED" });
+ for (let attempt = 0; attempt < 20 && coldStore.getState().executions[0]?.status !== "failed"; attempt += 1) {
+ await new Promise<void>((resolve) => setTimeout(resolve, 0));
+ }
</file context>
| <Field label="Transport"><select className={selectClass} value={server.type} onChange={(event) => replaceTransport(event.target.value as "http" | "stdio")}><option value="http">HTTP</option><option value="stdio">STDIO</option></select></Field> | ||
| {server.type === "http" ? <Field label="HTTP URL" error={errors[`mcp.servers.${name}.url`]}><TextInput value={server.url} onChange={(next) => update((draft) => { if (draft.type === "http") draft.url = next; })} /></Field> : <> | ||
| <Field label="Command" error={errors[`mcp.servers.${name}.command`]}><TextInput value={server.command} onChange={(next) => update((draft) => { if (draft.type === "stdio") draft.command = next; })} /></Field> | ||
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> |
There was a problem hiding this comment.
P2: The STDIO arguments parser currently keeps blank lines as "", so a trailing newline or spacer line is persisted as an actual argv entry. Filtering out empty lines before assigning draft.args avoids passing unintended empty arguments to the child process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/features/settings-panels.tsx, line 601:
<comment>The STDIO arguments parser currently keeps blank lines as `""`, so a trailing newline or spacer line is persisted as an actual argv entry. Filtering out empty lines before assigning `draft.args` avoids passing unintended empty arguments to the child process.</comment>
<file context>
@@ -406,18 +547,66 @@ export function SettingsMcpPanel({ config, servers, onChange, errors = {}, runti
+ <Field label="Transport"><select className={selectClass} value={server.type} onChange={(event) => replaceTransport(event.target.value as "http" | "stdio")}><option value="http">HTTP</option><option value="stdio">STDIO</option></select></Field>
+ {server.type === "http" ? <Field label="HTTP URL" error={errors[`mcp.servers.${name}.url`]}><TextInput value={server.url} onChange={(next) => update((draft) => { if (draft.type === "http") draft.url = next; })} /></Field> : <>
+ <Field label="Command" error={errors[`mcp.servers.${name}.command`]}><TextInput value={server.command} onChange={(next) => update((draft) => { if (draft.type === "stdio") draft.command = next; })} /></Field>
+ <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field>
+ </>}
+ <Field label="Connect timeout (ms)"><NumberField value={server.connectTimeoutMs} onChange={(next) => update((draft) => { draft.connectTimeoutMs = next; })} /></Field>
</file context>
Suggested change
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") draft.args = event.target.value ? event.target.value.split("\n") : undefined; })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> | |
| <Field label="Arguments (one per line)"><textarea rows={3} value={server.args?.join("\n") ?? ""} onChange={(event) => update((draft) => { if (draft.type === "stdio") { const args = event.target.value.split("\n").filter((line) => line.trim() !== ""); draft.args = args.length > 0 ? args : undefined; } })} className="min-h-20 resize-y rounded-sm border border-border-control bg-bg-base px-3 py-2 font-mono text-[12px] leading-[18px] text-text-primary outline-none transition-colors duration-[var(--motion-hover)] hover:border-text-secondary focus:border-brand focus:ring-2 focus:ring-brand-subtle" /></Field> |
| if (preserveSaveErrorRevision.current === snapshot.revision) { | ||
| preserveSaveErrorRevision.current = undefined; | ||
| } else { | ||
| setSaveError(undefined); |
There was a problem hiding this comment.
P2: The snapshot-change effect clears saveError in the mismatch branch but leaves preserveSaveErrorRevision.current set. That stale marker can match a later snapshot revision and incorrectly preserve an old MCP-apply error. Clearing the ref in both branches keeps error preservation scoped to the intended revision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/features/SettingsDialog.tsx, line 57:
<comment>The snapshot-change effect clears `saveError` in the mismatch branch but leaves `preserveSaveErrorRevision.current` set. That stale marker can match a later snapshot revision and incorrectly preserve an old MCP-apply error. Clearing the ref in both branches keeps error preservation scoped to the intended revision.</comment>
<file context>
@@ -46,12 +45,17 @@ export function SettingsBody({ snapshot, adapterCatalog, servers, onReload, runt
+ if (preserveSaveErrorRevision.current === snapshot.revision) {
+ preserveSaveErrorRevision.current = undefined;
+ } else {
+ setSaveError(undefined);
+ }
setSavedWhileRuntimeUnavailable(false);
</file context>
Suggested change
| setSaveError(undefined); | |
| preserveSaveErrorRevision.current = undefined; | |
| setSaveError(undefined); |
| expect(err).toBeInstanceOf(ConfigEnvExpansionError); | ||
| expect((err as ConfigEnvExpansionError).variableName).toBe("MISSING_TOKEN_ENV"); | ||
| expect((err as ConfigEnvExpansionError).message).not.toContain("secret-sentinel"); | ||
| test("rejects non-HTTP schemes and malformed URLs", () => { |
There was a problem hiding this comment.
P3: The URL-scheme allow-list is now tested only against literal URLs. The previous test that validated the http/https restriction on a scheme produced by env expansion ("rejects non-http URL after env expansion", e.g. url "${MCP_PROTO}://chat" with MCP_PROTO=ws) was removed and not replaced, and the new resolveMcpConfig env-expansion tests only expand to valid http/https schemes. Since resolveMcpConfig expands the URL before validateHttpUrl (expandString then validateHttpUrl), a scheme-injection through an env override is a real security-boundary path that currently has no regression coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/config/mcp.test.ts, line 303:
<comment>The URL-scheme allow-list is now tested only against literal URLs. The previous test that validated the http/https restriction on a scheme produced by env expansion ("rejects non-http URL after env expansion", e.g. url "${MCP_PROTO}://chat" with MCP_PROTO=ws) was removed and not replaced, and the new resolveMcpConfig env-expansion tests only expand to valid http/https schemes. Since resolveMcpConfig expands the URL before validateHttpUrl (expandString then validateHttpUrl), a scheme-injection through an env override is a real security-boundary path that currently has no regression coverage.</comment>
<file context>
@@ -2,532 +2,347 @@ import { afterEach, describe, expect, test } from "bun:test";
- expect(err).toBeInstanceOf(ConfigEnvExpansionError);
- expect((err as ConfigEnvExpansionError).variableName).toBe("MISSING_TOKEN_ENV");
- expect((err as ConfigEnvExpansionError).message).not.toContain("secret-sentinel");
+ test("rejects non-HTTP schemes and malformed URLs", () => {
+ for (const url of ["ftp://files.example.com", "ws://socket.example.com", "not a valid url", "//example.com/rpc"]) {
+ expect(() => resolveMcpConfig({ servers: { docs: { ...HTTP_SERVER, url } } })).toThrow(McpConfigError);
</file context>
| if (resource.includes("\0")) throw new Error("Skill resource path must not contain NUL bytes"); | ||
| if (resource.includes("\\")) throw new Error("Skill resource path must use POSIX separators"); | ||
| if (posix.isAbsolute(resource)) throw new Error("Skill resource path must be relative"); | ||
| if (resource.length === 0) throw new SkillPackageResourcePathError("Skill resource path must not be empty"); |
There was a problem hiding this comment.
P3: Some invalid resource paths still return TOOL_SKILL_READ_FAILED instead of the new invalid-input result; throw SkillPackageResourcePathError for the depth and entry-path validation branches too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent-core/src/skills/package-reader.ts, line 230:
<comment>Some invalid resource paths still return `TOOL_SKILL_READ_FAILED` instead of the new invalid-input result; throw `SkillPackageResourcePathError` for the depth and entry-path validation branches too.</comment>
<file context>
@@ -156,14 +166,74 @@ export function readBuiltinSkillResource(
- if (resource.includes("\0")) throw new Error("Skill resource path must not contain NUL bytes");
- if (resource.includes("\\")) throw new Error("Skill resource path must use POSIX separators");
- if (posix.isAbsolute(resource)) throw new Error("Skill resource path must be relative");
+ if (resource.length === 0) throw new SkillPackageResourcePathError("Skill resource path must not be empty");
+ if (resource.includes("\0")) throw new SkillPackageResourcePathError("Skill resource path must not contain NUL bytes");
+ if (resource.includes("\\")) throw new SkillPackageResourcePathError("Skill resource path must use POSIX separators");
</file context>
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
1 participant
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.
Summary
Breaking change
MCP server configuration now requires an explicit type and enabled field. The legacy url, headers, and timeout-only shape is rejected with no compatibility fallback.
Validation
Review record
The implementation completed four independent review iterations. Fixes from those reviews include operation-level MCP discovery deadlines, cursor-cycle rejection, cross-project live-update coverage, complete real HTTP/STDIO lifecycle coverage, and graceful remote HTTP session termination.
Summary by CodeRabbit
/skill useactivation.