Do not send a locally minted run id as a run authorization binding - #3546
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:
📝 WalkthroughWalkthroughAG-UI contexts now mark generated run IDs as non-binding for tool authorization. Remote tool and MCP metadata omit ChangesRun ID authorization binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AGUIHandler
participant ToolExecutionContext
participant RemoteTools
participant RemoteMCP
participant VeryfrontControlPlane
AGUIHandler->>ToolExecutionContext: create run context
ToolExecutionContext->>RemoteTools: provide runId and binding flag
RemoteTools->>RemoteTools: omit runId when flag is false
ToolExecutionContext->>RemoteMCP: provide endpoint and binding flag
RemoteMCP->>VeryfrontControlPlane: send metadata without run_id
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 977b9406ef
ℹ️ 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".
![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/ag-ui/handler.ts`:
- Around line 330-338: In both AG-UI streaming paths, preserve the non-binding
authorization marker when the handler generated the run ID: after selecting
beforeStreamResult?.context as the final context, reapply
runIdBindsToolAuthorization: false whenever clientRunId is absent. Update the
final-context handling near the shown buildStreamContext flow and its
counterpart, while leaving contexts for client-supplied run IDs unchanged.
In `@src/integrations/remote-tools.ts`:
- Line 127: Add focused regression coverage for the authorization contract
around runIdBinds and the request-building flow: verify absent client runId
preserves the generated lifecycle ID while omitting serialized run_id, supplied
runId is serialized, no marker preserves existing behavior, and replacement
context keeps the non-binding marker effective. Make the test fail before
implementation, then update the relevant remote-tools logic to satisfy it
without changing unrelated behavior.
🪄 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: 1a43738c-962e-4105-97fc-24bcf8908c35
📒 Files selected for processing (3)
src/agent/ag-ui/handler.tssrc/integrations/remote-tools.tssrc/tool/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ag-ui/handler.ts`:
- Around line 150-153: Add focused regression tests in handler.test.ts for both
streaming paths and both generated and client-supplied AG-UI run IDs. Cover
direct and injected-tool execution, have the replacement context return a fresh
context without the marker, and assert the final agent or runtime context
contains runIdBindsToolAuthorization: false.
🪄 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: ecea2012-0d59-4dbb-940e-391853128f16
📒 Files selected for processing (1)
src/agent/ag-ui/handler.ts
Review: 58/100 — right diagnosis, incomplete fix, red CIThe core insight is good and the measured A/B evidence is the best part of this PR. Sending a locally minted I also independently confirmed the cloud-runs argument. Below is what blocks merge. P0 — CI is red, and it is this PR
Every stale line is a line-number shift caused by exactly this diff — handler.ts +11, remote-tools.ts +5, types.ts +3. Nothing pre-existing. Fix: P0 — the same bug is unpatched on the MCP path
function buildRunContextMeta(context: ToolExecutionContext | undefined) {
const meta: Record<string, unknown> = {};
if (typeof context?.runId === "string" && context.runId.length > 0) {
meta.run_id = context.runId;
}
...
}That const runContext = {
...(typeof meta.run_id === 'string' ? { runId: meta.run_id } : {}),
...
}
await callIntegrationTool(toolName, toolArgs, ctx, runContext)Same function. Same P1 — the marker is stripped on the nested/delegate path
const REMOTE_TOOL_CREDENTIAL_CONTEXT_KEYS = ["authToken", "runId", "agentId"] as const;
This is the third instance of the same failure mode. Commit P1 — no testsZero test files in the diff. The marker is fail-open by design, and it has already been dropped once during review. Nothing pins:
Those are cheap unit tests against the exact regressions this branch already hit. P1 — the central premise is overstated
That is not what the code does. In const executionUserId = getIntegrationExecutionUserId(toRunAuthorizationActor(ctx), runDeclaredTool)
await executeIntegrationTool({
projectId: runDeclaredTool?.projectId ?? ctx.projectId ?? undefined,
...(executionUserId ? { userId: executionUserId } : {}),
...(runDeclaredTool ? { runDeclaredTool } : {}),
})
That may well be the behaviour you want in local dev. But it should be stated as the deliberate change it is, not as a no-op. P1 — the success case was never observedThis matters because of the point above. The PR reports Reconnect the connection and get one green tool call. Without that, the branch is unverified in the case it exists to fix. P2 — the description contradicts the code
It does not.
That was measured against To be clear, the change in P2 — duplicate
|
veryfront dev mints run_<uuid> in-process. That id is not a control-plane run, so agentRunRepository.getByRunId finds nothing and every integration tool call is denied with "Run context is not authorized for this integration tool". The run id itself must stay: AG-UI RUN_STARTED/RUN_FINISHED, the resume session manager and the run.id trace attribute all depend on it. Only its export as an authorization binding is wrong. buildStreamContext now marks the id non-binding when the client did not supply one, and snapshotToolExecutionContext drops runId on strict === false. Absence of the marker preserves the previous behaviour exactly, so every other context producer is unchanged. Cloud runs are unaffected: runtime-auth-token.ts mints tokens with runId: run.runId on both project-backed branches, and the API computes runId = claimedRunId ?? suppliedRunId, so the seal still engages from the token claim.
Two review findings. An AG-UI run id supplied by the client is an untrusted request field, not proof of a control-plane run. Treating it as binding meant any client that generates its own per-request id still got denied for the same missing-run reason this branch fixes. Hosted durable runs bind through the token claim in createAgUiRuntimeHandler, so createAgUiHandler now marks the id non-binding unconditionally. That also removes the clientRunId branching. beforeStream may return a fresh context object rather than spreading the one it was given. finalContext replaced the whole object and dropped the marker, so snapshotToolExecutionContext read the absent marker as binding and exported the generated id again. The marker is now reapplied to finalContext in both streaming paths.
deno task docs. Line number shifts only, no content changes.
0470b77 to
40119f7
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
Review found the same defect in two more places. remote-mcp.ts buildRunContextMeta exported context.runId as _meta.run_id, which the API reads back into the same callIntegrationTool gate. An integration reached over remote MCP was still denied. It now honours the marker like the REST path. mcp-server-tool-sources.ts copies authToken, runId and agentId from the credential owner into a nested context. The marker was not in that list, so it was dropped when the owner supplied a run id, re-exporting a local id as binding. Added it, and the marker is now cleared when the owner has a real run id and no marker, so a stale nested marker cannot suppress a legitimate binding. Three regression tests: suppression on the marker, run_id still sent when the marker is absent, and suppression only on strict false.
The marker is fail-open, so a truthy, absent or malformed value must still export the run id. Only an explicit false suppresses it.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b8e83358b
ℹ️ 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".
Review: the marker means the run id must not be used as a Veryfront authorization binding, not that it is secret. Suppressing _meta.run_id for every remote MCP server stripped correlation metadata that third-party servers had always received. Suppression is now scoped to endpoints whose origin matches the configured Veryfront API base URL, which is the only handler that reads _meta.run_id back into the integration authorization gate. The existing non-binding test asserted suppression against an endpoint that was not the control plane, so it now sets VERYFRONT_API_BASE_URL to match. Added the converse: a third-party origin still receives run_id. The third-party endpoint is a public address rather than a TEST-NET one because the egress guard classifies TEST-NET as internal and blocks it.
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 `@src/agent/ag-ui/handler.test.ts`:
- Around line 185-207: Update the AG-UI handler behavior so only locally minted
run IDs set runIdBindsToolAuthorization to false; client-supplied IDs must
retain the default authorization binding. Adjust the expectations in
src/agent/ag-ui/handler.test.ts lines 185-207 and 400-401 for the direct-stream
and injected-tool cases, respectively, while preserving the run ID values in the
responses.
In `@src/agent/runtime/mcp-server-tool-sources.test.ts`:
- Around line 597-611: The test around
bindRuntimeRemoteToolSourcesToCredentialOwner must cover owner contexts with a
defined runId and runIdBindsToolAuthorization false, plus an owner context where
the marker is absent. Add assertions for both cases confirming the owner context
replaces the nested run context, including the expected runId and marker
behavior, so the marker-copy and deletion branches execute.
In `@src/tool/remote-mcp.test.ts`:
- Around line 299-300: In src/tool/remote-mcp.test.ts at lines 299-300 and
341-342, replace Deno.env access with the runtime-neutral scoped environment
helper used by the tests; at lines 335-336 and 373-374, restore each scoped
environment in a finally block so cleanup runs on failures. In
src/agent/runtime/mcp-server-tool-sources.test.ts at line 587, register the test
case through describe() and it() imported from `#veryfront/testing/bdd.ts`.
🪄 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: 1d51c0b2-e520-4c77-8dd5-25721f8ac095
📒 Files selected for processing (12)
docs/api-reference/veryfront/agent.mddocs/api-reference/veryfront/integrations.mddocs/api-reference/veryfront/tool.mdsrc/agent/ag-ui/handler.test.tssrc/agent/ag-ui/handler.tssrc/agent/runtime/mcp-server-tool-sources.test.tssrc/agent/runtime/mcp-server-tool-sources.tssrc/integrations/remote-tools.test.tssrc/integrations/remote-tools.tssrc/tool/remote-mcp.test.tssrc/tool/remote-mcp.tssrc/tool/types.ts
💤 Files with no reviewable changes (1)
- docs/api-reference/veryfront/agent.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/integrations/remote-tools.ts
- src/tool/types.ts
- src/agent/ag-ui/handler.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11d6582c71
ℹ️ 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".
remote-mcp.test.ts used Deno.env, which Node and Bun runners can skip, and restoration did not run on failure. Replaced with setEnv/deleteEnv from compat/process and an afterEach restore. mcp-server-tool-sources.test.ts did not exercise the marker copy or the delete branch, because the owner context omitted runId. Added both: an owner run id carrying a non-binding marker replaces the nested context, and an owner run id without a marker clears a stale nested one.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime/mcp-server-tool-sources.test.ts`:
- Line 614: Replace both Deno.test declarations in the existing suite with it()
calls, and import describe and it from `#veryfront/testing/bdd.ts`. Preserve each
test’s name, body, and existing describe grouping so the cases run across all
supported runtimes.
🪄 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: cd531b7e-952b-4b9c-a44b-e61b66302e8a
📒 Files selected for processing (2)
src/agent/runtime/mcp-server-tool-sources.test.tssrc/tool/remote-mcp.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tool/remote-mcp.test.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c27d3c5e78
ℹ️ 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".
|
@codex review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Problem
Veryfront dev creates local AG-UI and eval run IDs that do not identify control-plane runs. Exporting those IDs as integration authorization bindings causes valid local tool calls to be rejected.
Change
For a non-binding local run, integration execution deliberately falls back to the authenticated caller context instead of claiming a run-owned connection. Control-plane runs remain bound by the run ID in their signed token claim.
Validation