evals: promote agent-mount contract to v4-spike - #2635
Conversation
|
There was a problem hiding this comment.
3 issues found across 2 files
Confidence score: 3/5
- In
packages/evals/core/contracts/tool.ts,stagehand_codewas added toToolSurfacebut runtime resolution paths still fall through to "not implemented"/"No default startup profile", which can cause tool runs to fail when that surface is selected — align the union with resolver and default-profile handling. - In
packages/evals/core/contracts/tool.ts, thehandlesvariant documents reserved names (startUrl,task,console) but uses a plainRecord<string, unknown>with no enforcement, so collisions can slip through and break harness assumptions at runtime — add compile-time constraints and/or runtime validation for reserved keys. - In
packages/evals/core/contracts/tool.ts, exported run-tool constants are unused while the harness keeps parallel local constants, creating a quiet drift risk that can desynchronize prompts, allowlists, and MCP wiring — switch harness references to the shared exported constants.
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/evals/core/contracts/tool.ts">
<violation number="1" location="packages/evals/core/contracts/tool.ts:9">
P2: `stagehand_code` is now part of `ToolSurface`, but the runtime resolvers still don't handle it, so selecting that surface reaches thrown "not implemented"/"No default startup profile" paths. Keeping the union aligned with implemented registry/startup branches (or deferring this union member) would avoid a misleading supported-surface contract.</violation>
<violation number="2" location="packages/evals/core/contracts/tool.ts:175">
P3: The new exported run-tool constants are currently unused while the harness keeps parallel local constants, so name/value drift can silently desynchronize prompts, allowlists, and MCP wiring later. Reusing the shared contract constants in the adapter would keep this behavior centralized.</violation>
<violation number="3" location="packages/evals/core/contracts/tool.ts:205">
P2: The new `handles` variant documents an invariant — harness-reserved handle names (`startUrl`, `task`, `console`) may not appear in the map — but nothing enforces it. `handles` is typed as a plain `Record<string, unknown>`, so an implementing surface can silently include a reserved name and overwrite the harness-injected binding, causing confusing agent behavior with no compile-time or runtime error. Consider exposing a small validator (e.g. `assertNoReservedHandles`) alongside `AGENT_RUN_TOOL_RESERVED_HANDLES` and calling it from the harness/runner when the mount is consumed, so the documented contract is actually guaranteed.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Harn as Agent Harness
participant ToolCtrl as CoreTool
participant StartRes as ToolStartResult
participant Mount as AgentMount
participant Evid as captureEvidence
Note over Harn,Evid: NEW: Agent-Mount Contract Flow (v4-spike)
Harn->>ToolCtrl: start(input) via CoreTool interface
activate ToolCtrl
ToolCtrl-->>StartRes: Return ToolStartResult
deactivate ToolCtrl
Note over StartRes: Contains optional agentMount + captureEvidence
alt Native surface (no mount)
Harn->>StartRes: Use session directly
StartRes-->>Harn: Cleanup lifecycle
else Agent-mounted delivery
Harn->>Mount: Access agentMount via
alt via: "handles"
Mount->>Mount: Resolve handles (Record<string, unknown>)
Mount->>Mount: Validate reserved handles (startUrl, task, console)
else via: "mcp"
Mount->>Mount: Load mcpServers config
else via: "cli"
Mount->>Mount: Build command (bin, args, cwd, env)
end
Harn->>StartRes: Use promptInstructions for agent context
end
opt Evidence capture requested
Harn->>Evid: Call captureEvidence()
activate Evid
Evid->>Evid: Swallow per-field failures, never throw
Evid-->>Harn: Return ProbeEvidence (screenshot, url, ariaTree)
deactivate Evid
end
Harn->>StartRes: Call cleanup()
activate StartRes
Note over StartRes: captureEvidence invalid after this
StartRes-->>Harn: Runtime released
deactivate StartRes
alt Compile-time validation
Harn->>Mount: Verify discriminated union fields
Mount-->>Harn: TypeScript ensures via-specific properties
Note over Harn: e.g., CLI mount requires command field
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| export type ToolSurface = | ||
| | "understudy_code" | ||
| | "stagehand_code" |
There was a problem hiding this comment.
P2: stagehand_code is now part of ToolSurface, but the runtime resolvers still don't handle it, so selecting that surface reaches thrown "not implemented"/"No default startup profile" paths. Keeping the union aligned with implemented registry/startup branches (or deferring this union member) would avoid a misleading supported-surface contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/core/contracts/tool.ts, line 9:
<comment>`stagehand_code` is now part of `ToolSurface`, but the runtime resolvers still don't handle it, so selecting that surface reaches thrown "not implemented"/"No default startup profile" paths. Keeping the union aligned with implemented registry/startup branches (or deferring this union member) would avoid a misleading supported-surface contract.</comment>
<file context>
@@ -1,10 +1,12 @@
export type ToolSurface =
| "understudy_code"
+ | "stagehand_code"
| "playwright_code"
| "cdp_code"
</file context>
| * `AGENT_RUN_TOOL_RESERVED_HANDLES` are injected by the harness and may | ||
| * not appear here. | ||
| */ | ||
| handles: Record<string, unknown>; |
There was a problem hiding this comment.
P2: The new handles variant documents an invariant — harness-reserved handle names (startUrl, task, console) may not appear in the map — but nothing enforces it. handles is typed as a plain Record<string, unknown>, so an implementing surface can silently include a reserved name and overwrite the harness-injected binding, causing confusing agent behavior with no compile-time or runtime error. Consider exposing a small validator (e.g. assertNoReservedHandles) alongside AGENT_RUN_TOOL_RESERVED_HANDLES and calling it from the harness/runner when the mount is consumed, so the documented contract is actually guaranteed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/core/contracts/tool.ts, line 205:
<comment>The new `handles` variant documents an invariant — harness-reserved handle names (`startUrl`, `task`, `console`) may not appear in the map — but nothing enforces it. `handles` is typed as a plain `Record<string, unknown>`, so an implementing surface can silently include a reserved name and overwrite the harness-injected binding, causing confusing agent behavior with no compile-time or runtime error. Consider exposing a small validator (e.g. `assertNoReservedHandles`) alongside `AGENT_RUN_TOOL_RESERVED_HANDLES` and calling it from the harness/runner when the mount is consumed, so the documented contract is actually guaranteed.</comment>
<file context>
@@ -146,9 +161,59 @@ export interface ToolStartResult {
+ * `AGENT_RUN_TOOL_RESERVED_HANDLES` are injected by the harness and may
+ * not appear here.
+ */
+ handles: Record<string, unknown>;
+ runTool: AgentRunToolSpec;
+ }
</file context>
| * The MCP server / tool name used when an agent harness wraps handles in a | ||
| * code-execution tool. | ||
| */ | ||
| export const AGENT_RUN_TOOL_SERVER = "stagehand_browser"; |
There was a problem hiding this comment.
P3: The new exported run-tool constants are currently unused while the harness keeps parallel local constants, so name/value drift can silently desynchronize prompts, allowlists, and MCP wiring later. Reusing the shared contract constants in the adapter would keep this behavior centralized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/evals/core/contracts/tool.ts, line 175:
<comment>The new exported run-tool constants are currently unused while the harness keeps parallel local constants, so name/value drift can silently desynchronize prompts, allowlists, and MCP wiring later. Reusing the shared contract constants in the adapter would keep this behavior centralized.</comment>
<file context>
@@ -146,9 +161,59 @@ export interface ToolStartResult {
+ * The MCP server / tool name used when an agent harness wraps handles in a
+ * code-execution tool.
+ */
+export const AGENT_RUN_TOOL_SERVER = "stagehand_browser";
+export const AGENT_RUN_TOOL_NAME = `mcp__${AGENT_RUN_TOOL_SERVER}__run`;
+export const AGENT_RUN_TOOL_RESERVED_HANDLES = ["startUrl", "task", "console"] as const;
</file context>
A uniform declaration of what a tool surface offers a coding agent: code_handles (in-scope objects the agent writes code against, mounted by the harness as a single local-MCP run tool), mcp_server, or cli — plus the LLM_RUN_TOOL_SERVER/LLM_RUN_TOOL_NAME bindings the mount uses. Types and constants only; no surface or adapter changes. Part 1/4 of the #2473 port onto the current SDK generation. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Updates the tool-surface contract to include agent delivery (`AgentMount`) and final-state evidence capture, keeping native `CoreTool.surface` independent from how it’s delivered to the agent. Types + tests only; supports STG-2671. - **New Features** - Types: `AgentMount` (`via: handles | mcp | cli` with `promptInstructions`), `AgentRunToolSpec`; `ToolStartResult` gains optional `agentMount` and `captureEvidence(): Promise<ProbeEvidence>`. - Enums: `ToolSurface` adds `stagehand_code`; `CoreTool.family` adds `stagehand`. - Constants: `AGENT_RUN_TOOL_SERVER` ("stagehand_browser"), `AGENT_RUN_TOOL_NAME` (`mcp__stagehand_browser__run`), `AGENT_RUN_TOOL_RESERVED_HANDLES` (`startUrl`, `task`, `console`). - Contract semantics: delivery is independent of native surface; CLI env merges over harness env; reserved harness bindings; capture-before-cleanup ordering. - Tests: `tool-contract.test.ts` validates delivery independence, handle mounts, and evidence capture. <sup>Written for commit d3ba958. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/browserbase/stagehand/pull/2590?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Miguel <36487034+miguelg719@users.noreply.github.com>
aeebcd2 to
fd74210
Compare
Summary
Corrects the base-target mistake from #2590 by landing its merged agent-mount contract on v4-spike.
This branch is exactly current v4-spike plus the #2590 squash commit:
After merge, #2591 and the remaining stack will be rebased directly onto v4-spike.
Verification
Summary by cubic
Promotes the agent-mount tool-surface contract to
v4-spike, adding type-safe agent delivery bindings and best-effort evidence capture, independent ofCoreTool.surface. Types and tests only; no runtime changes.ToolStartResultadds optionalagentMountandcaptureEvidence(): Promise<ProbeEvidence>.AgentMountunion (via: handles | mcp | cli) withpromptInstructions; plusAgentRunToolSpec.AGENT_RUN_TOOL_SERVER,AGENT_RUN_TOOL_NAME,AGENT_RUN_TOOL_RESERVED_HANDLES. Enums:ToolSurfaceaddsstagehand_code;CoreTool.familyaddsstagehand.packages/evals/tests/core/tool-contract.test.ts) validate surface/delivery independence, handle mounts, and evidence capture.Written for commit fd74210. Summary will update on new commits.