fix(agent): scope hosted child MCP tools to the active project - #3624
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughVeryfront API MCP endpoints now include the active project path. Endpoint resolution changes when the project changes. Remote MCP transport selection supports trusted project-scoped routes and guarded unmatched endpoints. Hosted and runtime tests validate the behavior. ChangesProject-scoped MCP configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HostedAgent
participant RemoteMCPToolSource
participant ProjectResolver
participant FetchProvider
HostedAgent->>RemoteMCPToolSource: request MCP tools
RemoteMCPToolSource->>ProjectResolver: resolve active project
ProjectResolver-->>RemoteMCPToolSource: return project-scoped endpoint
RemoteMCPToolSource->>FetchProvider: select transport for endpoint
FetchProvider-->>RemoteMCPToolSource: return supplied or guarded fetch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7831181608
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Critical review — 32/100 (do not merge as-is)The diagnosis is right: integration tools are only discoverable on the project-scoped MCP route, and the flat BlockingB1 — CI is red, and the PR body says it is green. The body claims "fmt/lint/diff checks pass" and that the focused Deno test is "blocked by known local native-brand/typecheck failures." Neither holds. The focused test passes locally (
The failing case in B2 — The endpoint is captured once at construction; the project is mutable. ( Every neighbouring parameter is a deferred thunk on purpose: defaultProjectId: () => activeProjectId(input.taskContext),
getProjectId: input.getProjectId ?? (() => activeProjectId(input.taskContext)),
getActiveBranchId: ...
This PR calls Before this PR the flat route was project-agnostic and correct under switching. This makes switching worse, not better.
B3 — The run-id authorization classifier was not updated. ( function endpointBindsToolAuthorization(endpoint: string): boolean {
...
const controlPlaneEndpoint = normalizeTrustedEndpoint(`${apiBaseUrl.replace(/\/+$/, "")}/mcp`);
return normalizedEndpoint !== undefined && normalizedEndpoint === controlPlaneEndpoint;
}Exact match against The same exact-match assumption governs the deployment-owned transport allowlist ( Should fix before mergeS1 — Dispatch duplicated at the call sites instead of at the single point that already dispatches. The identical S2 — The third call site was left behind. ( apiMcpUrl: `${bootstrap.apiBaseUrl.replace(/\/+$/, "")}/mcp`,
getProjectId: () => projectId,
S3 — Probed against the real implementation:
The whitespace case is inconsistent with the repo's own convention — S4 — S5 — New exported symbol, no doc comment. Every other export in both touched files carries one ( What holds up
Suggested shapeMove the whole thing into // mcp-server-config.ts — one place, deferred, covers all three call sites
return {
id: server.id ?? input.defaultSourceId ?? "veryfront-mcp",
endpoint: () => createProjectScopedMcpUrl(input.apiMcpUrl, input.getProjectId?.()),
headers: (context) => { /* unchanged */ },
};Then: trim + guard the project id, make the helper idempotent, normalise trailing slashes, teach 32/100. Right problem, right evidence, wrong layer — and shipped without running the suites that assert the invariant it changes. The remedy is roughly a 15-line rewrite; the score reflects the current diff, not the difficulty of fixing it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/agent/hosted/project-remote-tool-source.test.ts`:
- Around line 92-107: Add regression tests in createProjectScopedMcpUrl for an
already project-scoped URL and a whitespace-only project ID, then update
createProjectScopedMcpUrl to preserve already scoped URLs and ignore or reject
blank IDs. Keep existing unscoped and null-project behavior unchanged, and
implement the tests before the helper change.
In `@src/agent/hosted/veryfront-cloud-agent-service.test.ts`:
- Line 149: Extend the tests around createAgentRuntime and onStudioProjectSwitch
to switch from project-1 to project-2, then list or execute a remote tool.
Assert that both the root runtime and its child-fork configuration resolve the
project-2 MCP endpoint, adding this focused regression before changing
implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a87fdebd-7607-491a-9f3a-c596a79cef96
📒 Files selected for processing (3)
docs/api-reference/veryfront/agent.mdsrc/agent/hosted/project-remote-tool-source.test.tssrc/agent/hosted/veryfront-cloud-agent-service.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/api-reference/veryfront/tool.md`:
- Around line 130-131: Regenerate the API reference links for
createRemoteMCPToolSource and createRemoteMCPToolSourceFactoryWithTransport,
updating them to the current declaration locations (984 and 1015) and applying
the same correction to the additionally referenced entry at line 173.
In `@src/agent/service/mcp-server-config.test.ts`:
- Around line 59-79: Replace the Deno.test-based cases with describe() and it()
from `#veryfront/testing/bdd.ts`: convert the createProjectScopedMcpUrl test in
src/agent/service/mcp-server-config.test.ts (lines 59-79) and the API-only MCP
test in src/agent/hosted/project-remote-tool-source.test.ts (lines 908-935).
Preserve each test’s existing assertions and behavior.
In `@src/tool/remote-mcp.ts`:
- Around line 817-831: Update normalizeTrustedEndpoint and the related
trusted-endpoint handling around createProjectScopedMcpUrl to validate only the
parsed URL origin and pathname, while allowing and preserving query parameters
in the returned request target. Ensure query-bearing project-scoped
/projects/{id}/mcp endpoints remain trusted, suppress non-binding runId
behavior, and use requestFetch; add focused authorization and transport tests
covering this endpoint shape.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3781a35e-f92e-4840-a15d-8b107d8539b6
📒 Files selected for processing (11)
docs/api-reference/veryfront/agent.mddocs/api-reference/veryfront/tool.mdsrc/agent/hosted/child-fork-tool-sources.test.tssrc/agent/hosted/child-fork-tool-sources.tssrc/agent/hosted/project-remote-tool-source.test.tssrc/agent/hosted/veryfront-cloud-agent-service.test.tssrc/agent/runtime/mcp-server-tool-sources.test.tssrc/agent/service/mcp-server-config.test.tssrc/agent/service/mcp-server-config.tssrc/tool/remote-mcp.test.tssrc/tool/remote-mcp.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-reference/veryfront/agent.md
- src/agent/hosted/veryfront-cloud-agent-service.test.ts
|
Addressed the review findings in the latest commits:
Local evidence: 115 focused tests pass; API reference generation matches Linux/Deno 2.7.7 byte-for-byte. |
Re-review at
|
kwakayama
changed the title
961dc6a to
26ccb91
Compare
26ccb91 to
dbd9a2c
Compare
Re-review at
|
1353c55 to
3c2a5af
Compare
child-fork-tool-sources.ts gained a line (execution-support.ts changes from #3632), shifting the source link for prepareDefaultHostedChildForkSandboxToolSources from L196 to L197.
2 participants
Problem
Hosted
invoke_agentchild runs discovered first-party tools through the flat/mcpendpoint. That route has no active-project context, so project integrations such as Salesforce were absent even though they were available to the project.Solution
/projects/{active-project-id}/mcp, so root, child-fork, and direct runtime paths follow project switches.Validation
The result restores project-scoped integration discovery for hosted child agents without granting the deployment transport to arbitrary dynamic endpoints.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation