fix delegated remote tool namespace handling - #3123
Conversation
kwakayama
left a comment
There was a problem hiding this comment.
Critical review: 58/100
Verdict: not merge-ready. The core provenance path is directionally sound and the new child runtime success case passes, but the change introduces a blocking remote-tool allowlist bypass.
Score
- Correctness and authorization safety: 17/35
- Test quality and regression protection: 14/25
- Linked-issue completeness: 8/20
- Maintainability and repository fit: 14/15
- Verification evidence: 5/5
Blocking finding
A provenance-marked configured remote tool is accepted before allowedRemoteToolNames is enforced. Fresh reproduction on this head returned both gmail__list_emails and denied gmail__delete_email from getAvailableTools(), then executed gmail__delete_email despite an allowlist containing only gmail__list_emails. The inline comment has the exact remediation.
Spec coverage
Against veryfront-api#4150:
- AC1 is covered: the child reaches its first model event.
- AC4 remains covered: ordinary inline local
__tools are rejected. - AC2 is unverified: the test starts only one child.
- AC3 is partial: the tool is advertised but never invoked.
- AC5 is not implemented:
fork-runtime-part-mapper.tsstill falls back toFramework stream failedwhenevent.errorTextis absent. - AC6 is partial: the test constructs the remote source inside the framework and does not span API request construction into framework child assembly.
Evidence
- Local focused suite: 23 passed (77 steps), 0 failed
git diff --check: clean- GitHub lint, format, typecheck, unit, integration, coverage, and CodeQL checks: green
After the allowlist bypass is fixed, strengthen the regression to execute the Gmail tool and preserve the linked issue's remaining criteria as explicit follow-up work if they are intentionally outside this PR.
cb415d9 to
1684e0c
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Critical re-review: 80/100
Verdict: not merge-ready. The previous allowlist bypass is fixed: marked tools are now checked against canonical remote identity for both advertisement and execution, negative alias cases are covered, repeated child runtimes execute the Gmail tool, and concrete fork errors are preserved.
Blocking finding
The default child path now fails closed for an authorized alias. getForkRuntimeAllowedToolNames() and __vfAllowedRemoteTools carry provider-visible keys such as upload_attachment, while getAvailableTools() compares that list to canonical provenance such as gmail__upload_attachment. My fresh reproduction yielded keys/allow ['upload_attachment'] but no advertised tools. Preserve aliases for provider calls, but derive the runtime authorization list from canonical provenance, and add a positive alias regression through the actual child-runtime path.
Score
- Correctness and authorization safety: 27/35
- Tests: 20/25
- Linked issue/spec completeness: 15/20
- Maintainability: 13/15
- Verification: 5/5
Other review notes
Framework-level acceptance criteria 1–5 are substantially covered. The explicit API-to-framework boundary remains deferred to veryfront-api#4150. Low-severity standards issues remain: the private symbol does not follow the repository's uppercase constant convention, and exported hasRemoteToolProvenance() currently has no callers. The changelog instruction appears stale because this repository has no changelog file.
Verification evidence
- Focused suites: 24 passed (90 steps), 0 failed
deno task verify:quick: passedgit diff --check: clean- All required GitHub checks: green
- Prior denied-canonical/alias behavior independently reproduced as fixed
Submitted as a comment review because the authenticated account is the PR author and GitHub does not allow self-requested changes.
0d1cd17 to
b232ecb
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Critical re-review: 87/100
Verdict: not merge-ready yet. The previous authorized-alias blocker is fixed correctly at b232ecbb, and the original canonical allowlist bypass remains closed. One narrower listing/execution mismatch remains for integration-style aliases under source policy.
Score
- Correctness and authorization safety: 31/35
- Tests and regression protection: 21/25
- Linked issue/spec completeness: 16/20
- Maintainability and repository standards: 14/15
- Verification: 5/5
Standards
95/100. The constant casing and unused provenance predicate findings are resolved. No new hard code-level standards violations were found. Two low-severity items remain: the documented changelog requirement cannot currently be satisfied because the repository has no changelog artifact, and canonical authorization identity is reconstructed in several places, creating some drift risk in this security-sensitive invariant.
Spec
The framework-level behavior is substantially complete: two child runtimes advertise and execute the authorized provider alias, canonical allowlist/source policy checks reject denied aliases, ordinary unmarked local __ tools remain rejected, and concrete upstream error strings are preserved. The exact API-request-to-framework-child regression remains explicitly open under veryfront-api#4150.
The remaining blocker is the inline source-policy parity bug: an allowed canonical remote tool exposed under an integration-style alias is filtered from advertisement by the final alias-based policy pass even though execution authorizes it by canonical provenance.
I also reproduced that provenance is cooperative metadata rather than an unforgeable same-process security boundary: callers that already provide executable RemoteToolSource code can copy a marked tool. Given that authority, this is a trust-model/documentation concern rather than a new privilege escalation.
Verification evidence
- Focused suite: 24 passed (90 steps), 0 failed
deno task verify:quick: passedgit diff --check: clean- CI/CD and CodeQL workflows: passed; unit, integration, browser/binary E2E, npm smoke, coverage, lint, format, and typecheck jobs green
- Fresh parity reproduction: advertised
[]; execution invokedgmail__list_emails
Submitted as a comment review because the authenticated account is the PR author and GitHub does not allow self-requested changes.
b232ecb to
76389b1
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Critical re-review: 95/100
Verdict: merge-ready for the veryfront-code scope. No blocking correctness or security findings remain. Keep veryfront-api#4150 open until the explicitly deferred cross-repository regression is added.
Score
- Correctness and authorization safety: 35/35
- Tests and regression protection: 23/25
- Linked issue/spec completeness: 17/20
- Maintainability and repository standards: 15/15
- Verification: 5/5
Standards
96/100. The new authorization-name map is justified and focused: it preserves canonical identity only through the final provider-alias policy pass, and getConfiguredToolAuthorizationName() removes the earlier execution/listing duplication. Constant casing and the unused provenance helper are fixed.
Remaining process-level notes are non-blocking: CONTRIBUTING.md requires a changelog for fixes even though the repository has no changelog artifact, and the PR/commit title does not use the documented fix: Conventional Commit form. A minor judgment-call duplication remains between single-tool and fork-list canonical-name fallback, but the two call sites serve different shapes.
Spec
No incorrect in-scope implementation found. The exact previous failure now passes: canonical gmail__list_emails exposed as integration-style alias mail__list is advertised under the alias and executes the canonical tool. Alias-only allowlisting and source-policy-denied canonical tools remain closed. Two child runtimes execute the authorized alias, genuine local __ tools remain rejected, and concrete upstream error strings are preserved.
One issue-level criterion remains missing: “Add a regression test spanning API runtime request construction and framework child tool assembly.” Current evidence begins with a synthetic framework RemoteToolSource, so actual API request construction, parent invoke_agent dispatch, and parent-visible terminal projection remain cross-repository follow-up work.
Verification evidence
- Focused suite: 24 passed (91 steps), 0 failed
deno task verify:quick: passedgit diff --check: clean- Fresh positive/negative integration-style alias reproductions: passed
- CI/CD and CodeQL: passed; unit, integration, browser/binary E2E, npm smoke, coverage, lint, format, and typecheck jobs green
Axis summary: Standards has two low process notes and one minor smell; worst is the stale/unavailable changelog requirement. Spec has one missing cross-repository criterion; that is the only remaining issue-level gap.
Submitted as a comment review because the authenticated account is the PR author and GitHub does not allow self-approval.
Description
Preserve the canonical identity of remote MCP tools when they are materialized, traced, aliased, and rematerialized for delegated child runtimes.
A remote tool such as
gmail__upload_attachmentwas converted into a concrete runtime tool before a child fork started. The runtime then treated that concrete object as a local tool and rejected its integration-style ID with the reserved-namespace guard, surfacing asFramework stream failed/INVOKE_AGENT_FAILED.Remote provenance records the canonical remote tool name on an internal symbol. Runtime assembly uses that canonical identity for both:
allowedRemoteToolNamesand source-integration policy, including when a denied remote tool is exposed through an allowed-looking alias.Provider-facing aliases remain unchanged. At the child-runtime boundary, selected aliases are mapped back to canonical provenance only for the internal authorization list. During listing, the provider alias remains associated with that canonical authorization identity through the final source-policy filter. This allows authorized aliases, including integration-style aliases, to advertise and execute without making the alias itself an authorization credential.
Ordinary inline local tools using
__remain rejected. A remote tool cannot use provenance or aliasing to bypass the existing authorization policy.The fork-stream error mapper also preserves the concrete upstream error string when
errorTextis absent instead of replacing it with the genericFramework stream failedmessage.Related Issue(s)
Acceptance criteria
__remains valid after remote-to-host materialization.allowedRemoteToolNamesis enforced against the canonical remote name during both advertisement and execution.errorTextis absent.veryfront-apirequest-construction-to-framework boundary is covered cross-repository. This remains tracked in veryfront-api#4150; this PR changes no API code.Type of Change
Test evidence
Red before implementation:
The authorization regression tests also failed before hardening: denied canonical tools were advertised/executed when provenance or an allowed-looking alias was present.
Green after implementation:
Additional verification:
deno task verify:quickChecklist