You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Adarsh-Me (via GitHub)" <gi...@apache.org> on 2026/09/06 20:19:39 UTC
[PR] refactor(runtime): name the tool-call data boundary and share the call-field recipe [maka]
Adarsh-Me opened a new pull request, #4937:
URL: https://github.com/apache/maka/pull/4937
## Summary
Implements #4908 — the call-data slice of the A06 direction in #4726. Behavior-preserving extraction; no public API, event, message, or durable-format changes.
`executeTool()` spelled out the argument-projection policy inline: raw snapshot, permission projection on a private clone, the Computer Use persisted/model views, and two handwritten common-field recipes for the `tool_start` event and the persisted `tool_call` message — all inside a method that also owns admission, dispatch identity, T1/T2 and publication.
The call-data rules now live in `packages/runtime/src/tool-call-snapshot.ts`:
- `snapshotToolArgs` / `snapshotJsonValue` — the recursive, frozen, cycle-rejecting snapshot, moved verbatim.
- `validateDeclaredToolArgs` — schema validation, moved verbatim.
- `buildToolCallArgs()` — one named operation producing the four argument views (`executionArgs`, `permissionArgs`, `persistedArgs`, `modelFacingArgs`) with the existing guards: direct-only rejection skips validation and projection entirely; an unavailable sandbox-boundary surface defers validation while every other rule still applies; validation/projection failures are returned as `permissionArgsError` for the existing refusal path instead of thrown. The Computer Use history-regression rationale (#4908's window_id/windowId record) moved into this function's documentation.
- `buildToolCallCommonFields()` — the one shared recipe for the fields both call records assemble. It is invoked once per record: the event and the message each receive privately owned clones of `args` and `providerOptions`, preserving the isolation the duplicated recipes guaranteed.
The 43 downstream uses of the views (loop gates, managed mutation admission, durable preparation, result projection, sandbox-denial keys, telemetry/artifacts) read the same named values as before; their algorithms are untouched. Synchronous-before-first-await registration is preserved: only the projection point moved into the helper, and admission ordering around it is unchanged.
## Measurements (per #4908's required table)
| Measurement | Before | After |
| --- | --- | --- |
| Common call-field recipes | 2 handwritten | 1 shared recipe (`buildToolCallCommonFields`), invoked once per record |
| Argument-view construction | 1 inline region (L1121–L1194) | 1 named operation (`buildToolCallArgs`); no duplicated projection |
| tool-runtime.ts size | 4,192 physical lines | 4,077 (−115; extraction, not deletion) |
| executeTool() size | 1,096 physical lines | smaller by the moved projection/recipe regions; remaining body is admission/T1/T2/publication flow |
| New internal module(s) | 0 | tool-call-snapshot.ts, 307 lines (incl. license header, moved comments, types) |
| Net physical delta, production code | — | +192 lines (350 added / 158 removed across both files) |
| Tests | existing coverage retained | no test modifications; counts below |
The net addition is documentation and named types: the moved rules kept their explanatory comments, which the issue required preserving, and the module adds the interface declarations that make the four views explicit. No behavior changed, so no new tests were required; the existing isolation suite covers the boundary.
## Verification
- The four named suites, run against the compiled output: `tool-runtime-argument-ownership` (1/1), `tool-runtime-settlement` (12/12), `computer-use-model-loop` (3/3), `computer-use-privacy-boundary` (5/5) — 21/21 pass. All test files unmodified.
- Baseline differential: the same four suites also pass 21/21 against the pre-refactor module (rebuilt from a stashed tree), confirming the extraction changed no outcomes.
- One real defect was caught by that suite during development: wiring both records to a single shared common-fields object leaked one mutable `args` across event and storage, and `tool-runtime-argument-ownership` failed immediately. The per-record invocation fixes it; that failure mode is exactly what the duplicated recipes used to prevent, and the shared recipe now documents it.
- `@maka/runtime` builds clean (`tsc -p tsconfig.json` + filesystem worker, 0 errors); `biome format/lint` clean on both files.
- Not run locally: the full desktop/e2e gates (no Linux/Xvfb here); CI covers them.
## AI use
Select exactly one:
- [x] No generative tool made a substantive contribution
- [ ] Generative tooling made a substantive contribution
Tool(s) and scope:
## Checklist
- [x] Tests cover the change and fail without it — the argument-ownership suite fails if the two records ever share one args object (verified during development); all pre-existing suites pass unmodified.
- [x] Lint, format, typecheck and the affected suites pass locally — see Verification.
Does this PR entail a change in behavior?
- [x] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): extract tool-call argument projections [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on code in PR #4937:
URL: https://github.com/apache/maka/pull/4937#discussion_r4106714734
##########
packages/runtime/src/tool-call-snapshot.ts:
##########
@@ -0,0 +1,310 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * The call-data boundary for one tool execution: the argument views a call
+ * produces and the common fields its `tool_start` event and persisted
+ * `tool_call` message share.
+ *
+ * Extracted from `executeTool()` so the ownership of each rule is named
+ * instead of living inline in a thousand-line method. This module owns
+ * *construction only* — admission, dispatch identity, publication timing and
+ * persistence stay in `executeTool()`.
+ */
+
+import { computerUseModelCallArgs } from '@maka/core/computer-use';
+import type { ToolActivityKind } from '@maka/core/events';
+
+/** The recursively frozen, cycle-rejecting argument snapshot. */
+export function snapshotToolArgs(value: unknown): unknown {
+ return snapshotJsonValue(value, new WeakSet<object>());
+}
+
+function snapshotJsonValue(value: unknown, seen: WeakSet<object>): unknown {
+ if (value === null || typeof value !== 'object') return value;
+ if (seen.has(value)) throw new Error('Tool arguments must not contain cycles');
+ seen.add(value);
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map((entry) => snapshotJsonValue(entry, seen)));
+ }
+ const output: Record<string, unknown> = {};
+ for (const key of Object.keys(value)) {
+ const descriptor = Object.getOwnPropertyDescriptor(value, key);
+ if (!descriptor || !('value' in descriptor)) {
+ throw new Error(`Tool argument ${key} must be a plain data property`);
+ }
+ output[key] = snapshotJsonValue(descriptor.value, seen);
+ }
+ return Object.freeze(output);
+}
+
+/**
+ * Validates call arguments against the tool's declared schema, accepting the
+ * schema shapes the runtime already supports (zod `safeParseAsync`/`safeParse`,
+ * a `validate` callable, or a standard-schema `~standard.validate`). A tool
+ * without a usable schema declares nothing and validates nothing.
+ */
+export async function validateDeclaredToolArgs(parameters: unknown, args: unknown): Promise<void> {
+ if (!parameters || (typeof parameters !== 'object' && typeof parameters !== 'function')) {
+ return;
+ }
+ const schema = parameters as {
+ safeParseAsync?: (
+ value: unknown,
+ ) => PromiseLike<{ success: true; data: unknown } | { success: false; error: unknown }>;
+ safeParse?: (
+ value: unknown,
+ ) => { success: true; data: unknown } | { success: false; error: unknown };
+ validate?: (
+ value: unknown,
+ ) =>
+ | { success: true; value: unknown }
+ | { success: false; error: unknown }
+ | PromiseLike<{ success: true; value: unknown } | { success: false; error: unknown }>;
+ '~standard'?: {
+ validate?: (
+ value: unknown,
+ ) =>
+ | { value: unknown }
+ | { issues: readonly unknown[] }
+ | PromiseLike<{ value: unknown } | { issues: readonly unknown[] }>;
+ };
+ };
+
+ if (typeof schema.safeParseAsync === 'function') {
+ const parsed = await schema.safeParseAsync(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.safeParse === 'function') {
+ const parsed = schema.safeParse(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.validate === 'function') {
+ const parsed = await schema.validate(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema['~standard']?.validate === 'function') {
+ const parsed = await schema['~standard'].validate(args);
+ if ('value' in parsed) return;
+ throw new Error('Tool arguments failed declared schema validation', { cause: parsed.issues });
+ }
+}
+
+/** Permission-projection context handed to a tool's `permissionArgs` hook. */
+export interface ToolPermissionArgsContext {
+ sessionId: string;
+ turnId: string;
+ toolCallId: string;
+}
+
+/** Input to {@link buildToolCallArgs}. */
+export interface ToolCallArgsInput {
+ toolName: string;
+ /** Declared argument schema; validation runs against the execution snapshot. */
+ parameters: unknown;
+ /** Category hint, selecting the Computer Use persisted/model projection. */
+ categoryHint?: string | undefined;
+ /**
+ * The tool's permission-oriented projection. When present, it receives a
+ * private mutable clone of the execution args and its result is
+ * re-snapshotted — exactly as the inline region did.
+ */
+ permissionArgs?: ((args: never, context: ToolPermissionArgsContext) => unknown) | undefined;
+ /** The canonical execution input, snapshotted synchronously at entry. */
+ executionArgs: unknown;
+ sessionId: string;
+ turnId: string;
+ toolCallId: string;
+ /**
+ * Direct-only nested rejection skips validation and permission projection
+ * entirely — the call never reaches the tool.
+ */
+ directOnlyRejected: boolean;
+ /**
+ * An unavailable sandbox-boundary surface retains its current validation
+ * exception, so validation is deferred while every other rule still applies.
+ */
+ validationDeferred: boolean;
+}
+
+/**
+ * The four argument views of one tool call. Each has one owner and one job:
+ *
+ * - `executionArgs` — canonical execution input. Consumers receive private
+ * mutable clones; the view itself stays frozen.
+ * - `permissionArgs` — the tool's permission-oriented projection, also read
+ * by downstream policy/signature logic.
+ * - `persistedArgs` — what the `tool_start` event, the persisted `tool_call`
+ * message and the durable call data record.
+ * - `modelFacingArgs` — what the model reads back as its own call.
+ */
+export interface ToolCallArgs {
+ readonly executionArgs: unknown;
+ readonly permissionArgs: unknown;
+ readonly persistedArgs: unknown;
+ readonly modelFacingArgs: unknown;
+}
+
+export interface ToolCallArgsAndProjectionError extends ToolCallArgs {
+ /**
+ * First validation or permission-projection failure. The caller routes it
+ * to the existing refusal path; the builder never throws for it.
+ */
+ readonly permissionArgsError: unknown;
+}
+
+/**
+ * The named argument-view construction operation: validation, permission
+ * projection, and the persisted/model-facing views, in the order and with the
+ * guards the inline region in `executeTool()` established.
+ *
+ * The args written into the `tool_start` event, the persisted `tool_call`
+ * message and the durable ledger are the record of the call the model reads
+ * back on its next turn (`model-history.ts` replays `event.content.args`).
+ *
+ * Computer Use used the host's approval summary there. That projection exists
+ * to decide and display a permission: it renames `window_id` to `windowId`,
+ * adds `approvalClass` and `rememberForTurnAllowed`, and drops every argument
+ * it does not need. On the real ToolRuntime a model that sent
+ * {action:'press_key', app, window_id, observation_id, element_id,
+ * text:'cmd+s'} read back {action, approvalClass, rememberForTurnAllowed,
+ * app, windowId, observationId} — a key the tool rejects, two fields it never
+ * sent, no element, and a press_key with no key. It then went on calling it
+ * that way.
+ *
+ * The permission prompt still reads the permission view, and the approval
+ * scope key is still computed from the raw call, so the projection only
+ * changes what is written down. `computerUseModelCallArgs` keeps the same
+ * privacy rule — screen-derived and user-typed values are reduced to a shape
+ * — and speaks the tool's own argument names.
+ *
+ * The model-facing view is the same projection as the audit record, since
+ * `computerUseModelCallArgs` became what both are written with. It was
+ * spelled out twice, which meant running it twice per call and leaving two
+ * expressions to drift apart. The two names stay because the roles are
+ * different — one is what the host records, one is what the model reads — and
+ * a divergence would go here.
+ */
+export async function buildToolCallArgs(
+ input: ToolCallArgsInput,
+): Promise<ToolCallArgsAndProjectionError> {
+ let permissionArgs = input.executionArgs;
+ let permissionArgsError: unknown;
+ if (!input.directOnlyRejected) {
+ try {
+ if (!input.validationDeferred) {
+ await validateDeclaredToolArgs(input.parameters, input.executionArgs);
+ }
+ permissionArgs = input.permissionArgs
+ ? snapshotToolArgs(
+ input.permissionArgs(structuredClone(input.executionArgs) as never, {
+ sessionId: input.sessionId,
+ turnId: input.turnId,
+ toolCallId: input.toolCallId,
+ }),
+ )
+ : input.executionArgs;
+ } catch (error) {
+ permissionArgsError = error;
+ }
+ }
+ const persistedArgs =
+ input.categoryHint === 'computer_use'
+ ? snapshotToolArgs(computerUseModelCallArgs(permissionArgs))
+ : permissionArgs;
+ return {
+ executionArgs: input.executionArgs,
+ permissionArgs,
+ permissionArgsError,
+ persistedArgs,
+ modelFacingArgs: persistedArgs,
+ };
+}
+
+/**
+ * Identity facts every tool activity record carries. Mirrors the private
+ * `ToolActivityIdentity` in `@maka/core/events`, which is not exported.
+ */
+export interface ToolActivityIdentityFields {
+ origin?: 'provider' | 'code_mode';
+ modelVisibility?: 'visible' | 'hidden';
+ parentToolCallId?: string;
+ parentOperationId?: string;
+}
+
+/** Input to {@link buildToolCallCommonFields}. */
+export interface ToolCallCommonFieldsInput extends ToolActivityIdentityFields {
+ turnId: string;
+ ts: number;
+ /**
+ * The call id. It is NOT emitted into the common fields: the event carries
+ * it as `toolUseId`, the message as `id`, and the persisted message schema
+ * rejects unknown keys — each record names it at its own site.
+ */
+ toolUseId: string;
+ toolName: string;
+ activityKind?: ToolActivityKind | undefined;
+ displayName?: string | undefined;
+ persistedArgs: unknown;
+ providerOptions?: Record<string, unknown> | undefined;
+ stepId?: string | undefined;
+}
+
+/** The fields both call records share, with privately owned copies. */
+export interface ToolCallCommonFields extends ToolActivityIdentityFields {
+ turnId: string;
+ ts: number;
+ toolName: string;
+ activityKind?: ToolActivityKind | undefined;
+ displayName?: string | undefined;
+ args: unknown;
+ providerOptions?: Record<string, unknown> | undefined;
+ stepId?: string | undefined;
+}
+
+/**
+ * The one recipe for the fields both call records share, replacing the two
Review Comment:
`ed8605106` took this: the module is now 232 lines (was 307), `tool-runtime.ts` is −105/+26, `modelFacingArgs` is gone from this path, and `toolUseId` only feeds `toolCallId`. Your "redundant model-facing alias" and "two callers" points no longer apply at this head.
What is still true: one production caller, and no direct test of the module. Two ways forward — say which you want: keep the extraction as the permission/persist boundary (and I add direct tests for it), or fold it back into `tool-runtime.ts` so the PR is a pure move with no new module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): extract tool-call argument projections [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #4937:
URL: https://github.com/apache/maka/pull/4937
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): extract tool-call argument projections [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4937:
URL: https://github.com/apache/maka/pull/4937#discussion_r4109149664
##########
packages/runtime/src/tool-call-snapshot.ts:
##########
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * The call-data boundary for one tool execution: the argument views a call
+ * produces.
+ *
+ * Extracted from `executeTool()` so the ownership of each rule is named
+ * instead of living inline in a thousand-line method. This module owns
+ * *construction only* — admission, dispatch identity, publication timing and
+ * persistence stay in `executeTool()`.
+ */
+
+import { computerUseModelCallArgs } from '@maka/core/computer-use';
+
+/** The recursively frozen, cycle-rejecting argument snapshot. */
+export function snapshotToolArgs(value: unknown): unknown {
+ return snapshotJsonValue(value, new WeakSet<object>());
+}
+
+function snapshotJsonValue(value: unknown, seen: WeakSet<object>): unknown {
+ if (value === null || typeof value !== 'object') return value;
+ if (seen.has(value)) throw new Error('Tool arguments must not contain cycles');
+ seen.add(value);
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map((entry) => snapshotJsonValue(entry, seen)));
+ }
+ const output: Record<string, unknown> = {};
+ for (const key of Object.keys(value)) {
+ const descriptor = Object.getOwnPropertyDescriptor(value, key);
+ if (!descriptor || !('value' in descriptor)) {
+ throw new Error(`Tool argument ${key} must be a plain data property`);
+ }
+ output[key] = snapshotJsonValue(descriptor.value, seen);
+ }
+ return Object.freeze(output);
+}
+
+/**
+ * Validates call arguments against the tool's declared schema, accepting the
+ * schema shapes the runtime already supports (zod `safeParseAsync`/`safeParse`,
+ * a `validate` callable, or a standard-schema `~standard.validate`). A tool
+ * without a usable schema declares nothing and validates nothing.
+ */
+export async function validateDeclaredToolArgs(parameters: unknown, args: unknown): Promise<void> {
+ if (!parameters || (typeof parameters !== 'object' && typeof parameters !== 'function')) {
+ return;
+ }
+ const schema = parameters as {
+ safeParseAsync?: (
+ value: unknown,
+ ) => PromiseLike<{ success: true; data: unknown } | { success: false; error: unknown }>;
+ safeParse?: (
+ value: unknown,
+ ) => { success: true; data: unknown } | { success: false; error: unknown };
+ validate?: (
+ value: unknown,
+ ) =>
+ | { success: true; value: unknown }
+ | { success: false; error: unknown }
+ | PromiseLike<{ success: true; value: unknown } | { success: false; error: unknown }>;
+ '~standard'?: {
+ validate?: (
+ value: unknown,
+ ) =>
+ | { value: unknown }
+ | { issues: readonly unknown[] }
+ | PromiseLike<{ value: unknown } | { issues: readonly unknown[] }>;
+ };
+ };
+
+ if (typeof schema.safeParseAsync === 'function') {
+ const parsed = await schema.safeParseAsync(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.safeParse === 'function') {
+ const parsed = schema.safeParse(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.validate === 'function') {
+ const parsed = await schema.validate(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema['~standard']?.validate === 'function') {
+ const parsed = await schema['~standard'].validate(args);
+ if ('value' in parsed) return;
+ throw new Error('Tool arguments failed declared schema validation', { cause: parsed.issues });
+ }
+}
+
+/** Permission-projection context handed to a tool's `permissionArgs` hook. */
+export interface ToolPermissionArgsContext {
+ sessionId: string;
+ turnId: string;
+ toolCallId: string;
+}
+
+/** Input to {@link buildToolCallArgs}. */
+export interface ToolCallArgsInput {
+ toolName: string;
Review Comment:
`toolName` is declared here and passed by the one caller (`tool-runtime.ts:1100`), but `buildToolCallArgs` never reads it — grep the module: the name appears only on this line. The same is true of the returned `executionArgs`: the caller destructures three fields and not that one (`tool-runtime.ts:1114-1116`), and `validateDeclaredToolArgs` is exported at line 61 while its only consumer is this module itself (line 207).
None of these change behaviour, and I am not asking for a new consumer. The reason to fix them is that this module's stated purpose is to name a boundary and say who owns what, so a field that nothing reads is the one thing it cannot afford to carry — a reader has to decide whether `toolName` is part of the contract or was left over. Either drop them, or say in the docstring why they are there.
##########
packages/runtime/src/tool-runtime.ts:
##########
@@ -1089,39 +1089,31 @@ export class ToolRuntime {
? `Tool ${tool.name} is direct-only and cannot run inside exec.`
: undefined;
const admissionFailure = directOnlyFailure ?? this.admitToolForStep(tool, stepId);
- let permissionArgs = executionArgs;
- let permissionArgsError: unknown;
- if (directOnlyFailure === undefined) {
- try {
- // An unavailable sandbox-boundary surface rejects the expansion before
- // interpreting it, including legacy shapes whose validation stays deferred.
- const sandboxBoundaryUnavailable =
- tool.name === 'request_sandbox_boundary' &&
- !this.interactionRun() &&
- (!this.input.createSandboxBoundaryRequest || !this.input.settleSandboxBoundaryRequest);
- if (!sandboxBoundaryUnavailable) {
- await validateDeclaredToolArgs(tool.parameters, executionArgs);
- }
- permissionArgs = tool.permissionArgs
- ? snapshotToolArgs(
- tool.permissionArgs(structuredClone(executionArgs) as never, {
- sessionId: this.input.sessionId,
- turnId,
- toolCallId: toolUseId,
- }),
- )
- : executionArgs;
- } catch (error) {
- permissionArgsError = error;
- }
- }
- // Permission args are a policy/UI projection. Persistence and model replay
- // share one canonical tool-dialect projection; Computer Use also strips
- // screen-derived and user-typed values at this boundary.
- const persistedArgs =
- tool.categoryHint === 'computer_use'
- ? snapshotToolArgs(computerUseModelCallArgs(permissionArgs))
- : permissionArgs;
+ // An unavailable sandbox-boundary surface cannot carry the expansion the
+ // tool requests, and that availability contract holds even when an older
+ // caller sends a legacy shape — validation stays deferred for it.
+ const sandboxBoundaryUnavailable =
+ tool.name === 'request_sandbox_boundary' &&
+ !this.interactionRun() &&
+ (!this.input.createSandboxBoundaryRequest || !this.input.settleSandboxBoundaryRequest);
+ const callArgs = await buildToolCallArgs({
+ toolName: tool.name,
+ parameters: tool.parameters,
+ categoryHint: tool.categoryHint,
+ permissionArgs: tool.permissionArgs,
+ // Upstream renamed rawExecutionArgs to executionArgs after this
+ // refactor; buildToolCallArgs passes it through unchanged, and the
+ // entry snapshot above already holds that same value.
Review Comment:
`rawExecutionArgs` does not exist anywhere in this tree, or in the merge base — the local is `executionArgs`, snapshotted at line 1082 and passed unchanged here. A reader cannot tell what was renamed, when, or whether the comment is still load-bearing. The rest of the comment (the value passes through unchanged and the entry snapshot already holds it) is worth keeping; the rename sentence is not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): name the tool-call data boundary and share the call-field recipe [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on PR #4937:
URL: https://github.com/apache/maka/pull/4937#issuecomment-5562366295
The CI failure on `f312477` was real and mine — root-caused and fixed on `c840671`.
The shared common-field recipe emitted `toolUseId` as its own key. The `tool_start` event carries that field, but the persisted `tool_call` message names the same value `id`, and storage validates every appended message against an exact-shape schema that rejects unknown keys (`hasExactShape` in `@maka/core/session`). `appendMessages` therefore threw `Invalid stored message schema` on the first tool call, the turn-level error path generalized it to `Operation failed` (the redaction fallback), and the turn died before reaching the fixture tool. Found by temporarily un-generalizing the model-failure path and rerunning the integration test locally: the raw stack pointed straight at the append-time decode.
The fix keeps the shared recipe but stops it from emitting the call id: the recipe's input still takes `toolUseId` (both records need it), and each record names it at its own site — `toolUseId` on the event, `id` on the message — exactly as the two handwritten recipes did. Documented in the interface so the next consolidation cannot reintroduce it.
Re-verified locally: the previously failing `tui-mcp-remote-integration` test passes, and the four suites named in the issue are green (argument-ownership 1, settlement 12, model-loop 3, privacy-boundary 5 — 21/21). Line counts move by two: tool-runtime.ts 4,078, tool-call-snapshot.ts 310.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): name the tool-call data boundary and share the call-field recipe [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #4937:
URL: https://github.com/apache/maka/pull/4937#discussion_r4027939547
##########
packages/runtime/src/tool-call-snapshot.ts:
##########
@@ -0,0 +1,310 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * The call-data boundary for one tool execution: the argument views a call
+ * produces and the common fields its `tool_start` event and persisted
+ * `tool_call` message share.
+ *
+ * Extracted from `executeTool()` so the ownership of each rule is named
+ * instead of living inline in a thousand-line method. This module owns
+ * *construction only* — admission, dispatch identity, publication timing and
+ * persistence stay in `executeTool()`.
+ */
+
+import { computerUseModelCallArgs } from '@maka/core/computer-use';
+import type { ToolActivityKind } from '@maka/core/events';
+
+/** The recursively frozen, cycle-rejecting argument snapshot. */
+export function snapshotToolArgs(value: unknown): unknown {
+ return snapshotJsonValue(value, new WeakSet<object>());
+}
+
+function snapshotJsonValue(value: unknown, seen: WeakSet<object>): unknown {
+ if (value === null || typeof value !== 'object') return value;
+ if (seen.has(value)) throw new Error('Tool arguments must not contain cycles');
+ seen.add(value);
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map((entry) => snapshotJsonValue(entry, seen)));
+ }
+ const output: Record<string, unknown> = {};
+ for (const key of Object.keys(value)) {
+ const descriptor = Object.getOwnPropertyDescriptor(value, key);
+ if (!descriptor || !('value' in descriptor)) {
+ throw new Error(`Tool argument ${key} must be a plain data property`);
+ }
+ output[key] = snapshotJsonValue(descriptor.value, seen);
+ }
+ return Object.freeze(output);
+}
+
+/**
+ * Validates call arguments against the tool's declared schema, accepting the
+ * schema shapes the runtime already supports (zod `safeParseAsync`/`safeParse`,
+ * a `validate` callable, or a standard-schema `~standard.validate`). A tool
+ * without a usable schema declares nothing and validates nothing.
+ */
+export async function validateDeclaredToolArgs(parameters: unknown, args: unknown): Promise<void> {
+ if (!parameters || (typeof parameters !== 'object' && typeof parameters !== 'function')) {
+ return;
+ }
+ const schema = parameters as {
+ safeParseAsync?: (
+ value: unknown,
+ ) => PromiseLike<{ success: true; data: unknown } | { success: false; error: unknown }>;
+ safeParse?: (
+ value: unknown,
+ ) => { success: true; data: unknown } | { success: false; error: unknown };
+ validate?: (
+ value: unknown,
+ ) =>
+ | { success: true; value: unknown }
+ | { success: false; error: unknown }
+ | PromiseLike<{ success: true; value: unknown } | { success: false; error: unknown }>;
+ '~standard'?: {
+ validate?: (
+ value: unknown,
+ ) =>
+ | { value: unknown }
+ | { issues: readonly unknown[] }
+ | PromiseLike<{ value: unknown } | { issues: readonly unknown[] }>;
+ };
+ };
+
+ if (typeof schema.safeParseAsync === 'function') {
+ const parsed = await schema.safeParseAsync(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.safeParse === 'function') {
+ const parsed = schema.safeParse(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema.validate === 'function') {
+ const parsed = await schema.validate(args);
+ if (parsed.success) return;
+ throw parsed.error;
+ }
+ if (typeof schema['~standard']?.validate === 'function') {
+ const parsed = await schema['~standard'].validate(args);
+ if ('value' in parsed) return;
+ throw new Error('Tool arguments failed declared schema validation', { cause: parsed.issues });
+ }
+}
+
+/** Permission-projection context handed to a tool's `permissionArgs` hook. */
+export interface ToolPermissionArgsContext {
+ sessionId: string;
+ turnId: string;
+ toolCallId: string;
+}
+
+/** Input to {@link buildToolCallArgs}. */
+export interface ToolCallArgsInput {
+ toolName: string;
+ /** Declared argument schema; validation runs against the execution snapshot. */
+ parameters: unknown;
+ /** Category hint, selecting the Computer Use persisted/model projection. */
+ categoryHint?: string | undefined;
+ /**
+ * The tool's permission-oriented projection. When present, it receives a
+ * private mutable clone of the execution args and its result is
+ * re-snapshotted — exactly as the inline region did.
+ */
+ permissionArgs?: ((args: never, context: ToolPermissionArgsContext) => unknown) | undefined;
+ /** The canonical execution input, snapshotted synchronously at entry. */
+ executionArgs: unknown;
+ sessionId: string;
+ turnId: string;
+ toolCallId: string;
+ /**
+ * Direct-only nested rejection skips validation and permission projection
+ * entirely — the call never reaches the tool.
+ */
+ directOnlyRejected: boolean;
+ /**
+ * An unavailable sandbox-boundary surface retains its current validation
+ * exception, so validation is deferred while every other rule still applies.
+ */
+ validationDeferred: boolean;
+}
+
+/**
+ * The four argument views of one tool call. Each has one owner and one job:
+ *
+ * - `executionArgs` — canonical execution input. Consumers receive private
+ * mutable clones; the view itself stays frozen.
+ * - `permissionArgs` — the tool's permission-oriented projection, also read
+ * by downstream policy/signature logic.
+ * - `persistedArgs` — what the `tool_start` event, the persisted `tool_call`
+ * message and the durable call data record.
+ * - `modelFacingArgs` — what the model reads back as its own call.
+ */
+export interface ToolCallArgs {
+ readonly executionArgs: unknown;
+ readonly permissionArgs: unknown;
+ readonly persistedArgs: unknown;
+ readonly modelFacingArgs: unknown;
+}
+
+export interface ToolCallArgsAndProjectionError extends ToolCallArgs {
+ /**
+ * First validation or permission-projection failure. The caller routes it
+ * to the existing refusal path; the builder never throws for it.
+ */
+ readonly permissionArgsError: unknown;
+}
+
+/**
+ * The named argument-view construction operation: validation, permission
+ * projection, and the persisted/model-facing views, in the order and with the
+ * guards the inline region in `executeTool()` established.
+ *
+ * The args written into the `tool_start` event, the persisted `tool_call`
+ * message and the durable ledger are the record of the call the model reads
+ * back on its next turn (`model-history.ts` replays `event.content.args`).
+ *
+ * Computer Use used the host's approval summary there. That projection exists
+ * to decide and display a permission: it renames `window_id` to `windowId`,
+ * adds `approvalClass` and `rememberForTurnAllowed`, and drops every argument
+ * it does not need. On the real ToolRuntime a model that sent
+ * {action:'press_key', app, window_id, observation_id, element_id,
+ * text:'cmd+s'} read back {action, approvalClass, rememberForTurnAllowed,
+ * app, windowId, observationId} — a key the tool rejects, two fields it never
+ * sent, no element, and a press_key with no key. It then went on calling it
+ * that way.
+ *
+ * The permission prompt still reads the permission view, and the approval
+ * scope key is still computed from the raw call, so the projection only
+ * changes what is written down. `computerUseModelCallArgs` keeps the same
+ * privacy rule — screen-derived and user-typed values are reduced to a shape
+ * — and speaks the tool's own argument names.
+ *
+ * The model-facing view is the same projection as the audit record, since
+ * `computerUseModelCallArgs` became what both are written with. It was
+ * spelled out twice, which meant running it twice per call and leaving two
+ * expressions to drift apart. The two names stay because the roles are
+ * different — one is what the host records, one is what the model reads — and
+ * a divergence would go here.
+ */
+export async function buildToolCallArgs(
+ input: ToolCallArgsInput,
+): Promise<ToolCallArgsAndProjectionError> {
+ let permissionArgs = input.executionArgs;
+ let permissionArgsError: unknown;
+ if (!input.directOnlyRejected) {
+ try {
+ if (!input.validationDeferred) {
+ await validateDeclaredToolArgs(input.parameters, input.executionArgs);
+ }
+ permissionArgs = input.permissionArgs
+ ? snapshotToolArgs(
+ input.permissionArgs(structuredClone(input.executionArgs) as never, {
+ sessionId: input.sessionId,
+ turnId: input.turnId,
+ toolCallId: input.toolCallId,
+ }),
+ )
+ : input.executionArgs;
+ } catch (error) {
+ permissionArgsError = error;
+ }
+ }
+ const persistedArgs =
+ input.categoryHint === 'computer_use'
+ ? snapshotToolArgs(computerUseModelCallArgs(permissionArgs))
+ : permissionArgs;
+ return {
+ executionArgs: input.executionArgs,
+ permissionArgs,
+ permissionArgsError,
+ persistedArgs,
+ modelFacingArgs: persistedArgs,
+ };
+}
+
+/**
+ * Identity facts every tool activity record carries. Mirrors the private
+ * `ToolActivityIdentity` in `@maka/core/events`, which is not exported.
+ */
+export interface ToolActivityIdentityFields {
+ origin?: 'provider' | 'code_mode';
+ modelVisibility?: 'visible' | 'hidden';
+ parentToolCallId?: string;
+ parentOperationId?: string;
+}
+
+/** Input to {@link buildToolCallCommonFields}. */
+export interface ToolCallCommonFieldsInput extends ToolActivityIdentityFields {
+ turnId: string;
+ ts: number;
+ /**
+ * The call id. It is NOT emitted into the common fields: the event carries
+ * it as `toolUseId`, the message as `id`, and the persisted message schema
+ * rejects unknown keys — each record names it at its own site.
+ */
+ toolUseId: string;
+ toolName: string;
+ activityKind?: ToolActivityKind | undefined;
+ displayName?: string | undefined;
+ persistedArgs: unknown;
+ providerOptions?: Record<string, unknown> | undefined;
+ stepId?: string | undefined;
+}
+
+/** The fields both call records share, with privately owned copies. */
+export interface ToolCallCommonFields extends ToolActivityIdentityFields {
+ turnId: string;
+ ts: number;
+ toolName: string;
+ activityKind?: ToolActivityKind | undefined;
+ displayName?: string | undefined;
+ args: unknown;
+ providerOptions?: Record<string, unknown> | undefined;
+ stepId?: string | undefined;
+}
+
+/**
+ * The one recipe for the fields both call records share, replacing the two
Review Comment:
[P2] Re-scope this helper to the current single-authority path
After the rebase onto `c87651e2`, `executeTool()` no longer constructs or appends a `ToolCallMessage`; `tool_start` is mapped to the RuntimeEvent ledger and the transcript message is derived later. Consequently this helper now has one invocation (`tool-runtime.ts:1197`), not two, `toolUseId` in its input is never read, and `modelFacingArgs` is assigned at `tool-runtime.ts:1120` but never consumed. The exact parent already has one call-field recipe, so this adds a 310-line module (+196 net production lines) without consolidating the advertised duplicate; it also reintroduces the redundant model-facing alias removed by #4971. The prior commit `b1b4684f` passes the claimed failing `tui-mcp-remote-integration` on this rebased tree, consistent with there being no stored-message construction here. Please remove the stale event/message contract and redundant view, or re-scope the extraction around an actual current production owner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org