Skip to content

refactor(runtime): extract tool call argument boundary - #4971

Open
testikun wants to merge 2 commits into
apache:mainfrom
testikun:codex/issue-4908-call-data-v2
Open

testikun wants to merge 2 commits into
apache:mainfrom
testikun:codex/issue-4908-call-data-v2

Conversation

@testikun

@testikun testikun commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Refs #4908
Refs #4909

Summary

This is the pre-#4879 call-data slice of A06.

It extracts the existing tool-call argument snapshot, declared-schema validation, permission projection, and Computer Use model-facing projection into one small internal module. The public ToolRuntime API and execution pipeline remain unchanged.

The slice intentionally does not touch transcript/message ownership or common-field construction. In particular, it does not change appendMessage, ToolCallMessage, ToolResultMessage, RuntimeEvent schemas, T1/T2, or the session_messages publication path. This keeps the change independent of #4879, which is converging ordinary transcript facts on RuntimeEvent plus a projector.

What is preserved

  • Recursive frozen snapshots and cycle/non-data-property rejection.
  • Synchronous direct-only and step registration before the first await.
  • Direct-only nested calls skip validation and permission projection.
  • Ordinary admission failures keep the existing projection order.
  • Unavailable sandbox-boundary surfaces retain deferred validation behavior.
  • Separate execution, permission, persisted, and model-facing argument ownership.
  • Existing Computer Use field-name and privacy projections.

Measurements

  • tool-runtime.ts: 4,197 baseline lines on current main; 4,086 after extraction.
  • computer-use-tools.ts: unchanged at 2,871 lines.
  • New argument module: 172 lines.
  • The reduction in tool-runtime.ts is reported as extraction; no unsupported net-deletion claim is made.
  • Common-fields consolidation is intentionally deferred until the final refactor(runtime): derive Session transcripts from RuntimeEvents #4879 writer shape is known.

Verification

Using Node 24.19.0:

  • core, storage, and runtime TypeScript builds passed;
  • Biome check passed;
  • git diff --check passed;
  • 22 targeted Runtime tests passed, including the argument-ownership and async snapshot regressions, settlement, Computer Use model-loop, and Computer Use privacy-boundary suites.

The full workspace test suite was not run because this PR is intentionally limited to the Runtime call-data seam.

@testikun
testikun force-pushed the codex/issue-4908-call-data-v2 branch 2 times, most recently from 4a6b357 to ef9da46 Compare September 7, 2026 09:37
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 7, 2026
@testikun
testikun force-pushed the codex/issue-4908-call-data-v2 branch 2 times, most recently from 66f2b36 to 36feaa7 Compare September 7, 2026 09:52
@testikun

testikun commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Why this extraction exists

This change is not intended to create four independently transformed or redacted copies of every tool argument. The four names describe different consumers of the same call data:

  • executionArgs: the stable snapshot used by the implementation or managed transform;
  • permissionArgs: the projection used by permission and policy logic;
  • persistedArgs: the call shape recorded in RuntimeEvent/durable data;
  • modelFacingArgs: the call shape replayed to the model, currently identical to persistedArgs.

For ordinary tools these values may be identical. They diverge when a tool has a permission projection, and Computer Use additionally applies its existing privacy and accepted-field-name projection before persistence/model replay.

The extraction gives that existing relationship one internal owner. Previously it lived inline in executeTool(), alongside admission, client preparation, managed mutation, T1/T2, execution and publication. Because every view is typed as unknown, using the wrong view would normally compile and surface later as a permission, replay or privacy regression. tool-call-snapshot.ts owns only snapshot/validation/projection construction; it does not own admission, dispatch identity, durability, transcript publication or execution.

A separate module is used because this is a pure data boundary with no ToolRuntime state. Keeping the helpers in the 4,197-line lifecycle module would shorten executeTool() but leave the rule without an independently readable and testable owner. The boundary remains deliberately small: no full call object, no runId/operationId, no service classes, and no common-field abstraction.

This is also intentionally independent of #4879. It does not change appendMessage, ToolCallMessage, ToolResultMessage, RuntimeEvent schemas or the transcript path. #4879 may remove one consumer, but RuntimeEvent, durable preparation and model replay still need the same argument views.

Measured production change: tool-runtime.ts 4,197 -> 4,086 lines, plus a 172-line internal module, for a temporary net +61 production lines. The extraction is not presented as net deletion. Test changes are kept on the real settleToolCall() entry point; the new async case proves the provider input is snapshotted before schema validation yields, and it fails if the entry snapshot is removed.

@testikun

testikun commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han could you please review this specifically against the direction of #4879?

The intended boundary is entirely before transcript publication: it extracts the existing argument snapshot, declared validation, permission projection and persisted/model-facing projection, while leaving appendMessage, call/result messages, RuntimeEvent construction and T1/T2 unchanged. The main question is whether this remains a clean precursor to #4879, or whether any part of the argument-view ownership should wait for the RuntimeEvent-only transcript cutover.

@testikun
testikun force-pushed the codex/issue-4908-call-data-v2 branch from 36feaa7 to 1f2cecd Compare September 8, 2026 03:01
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/L Under 1000 readable lines labels Sep 8, 2026
Merge main with RuntimeEvent-only transcript support. Remove unused exports and intermediate aliases while retaining the distinct argument roles.

Generated-by: OpenAI Codex
@testikun

Copy link
Copy Markdown
Contributor Author

Automated follow-up by OpenAI Codex on behalf of testikun (not an independent human review). Merged main containing #4879; the RuntimeEvent-only transcript dependency is no longer pending. Full build:test, full typecheck and lint pass. 95 focused ToolRuntime/SQLite/model-history tests pass; full Runtime suite: 3,349 passed, 13 skipped, 0 failed. Simplification retained after verification: removed unused exported implementation types/helper and redundant intermediate aliases. Admission, persistence and lifecycle ownership remain in ToolRuntime. Please review the latest head.

Astro-Han

This comment was marked as duplicate.

@Astro-Han
Astro-Han dismissed their stale review September 12, 2026 15:20

Withdrawing my approval pending the overall simplification assessment explained in the follow-up review. Behavior preservation alone does not establish the value of this extraction.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining the boundary and doing the verification. I want to correct my earlier approval: I checked behavior preservation, but gave too much weight to making tool-runtime.ts smaller and not enough to whether the repository becomes simpler overall. That was my mistake.

For this refactor, I would like the acceptance criterion to be a reduction in overall complexity: fewer duplicated rules, concepts, intermediate representations, layers, and maintenance points. Moving an already centralized implementation into another file and adding input/output interfaces does not, by itself, meet that goal. Line count is a useful signal, not the sole criterion, but the current net growth needs a concrete simplification benefit beyond a smaller source file.

I am comfortable with a larger PR or a broader refactor if it removes the underlying complexity coherently. I would prefer that over a small extraction that leaves the same responsibilities in place and adds another interface to maintain. This is not a request to split the work into more wrappers or facade layers.

Could you revisit the scope with that goal? Start from the smallest complete argument-processing path, remove unnecessary pass-through fields and redundant representations where the actual consumers allow it, and show which rules or maintenance points disappear. Keep the execution/permission/privacy contracts and the existing regression coverage. If the extraction cannot demonstrate that tradeoff, keeping the logic in place is a reasonable outcome too.

I am withdrawing my approval for a87f1b263 while that design question is revisited. The earlier behavior checks still stand; this is a correction to my assessment of the refactor's value, not a newly discovered runtime defect. Thanks for taking another look.

中文

我想纠正之前的批准:我确认了行为保持,但过于看重单个文件缩短,没有充分判断仓库整体是否更简单,这是我的判断失误。

我希望重构减少重复规则、概念、中间表示、层级和维护点。可以接受更大规模的 PR 和重构,只要它连贯地消除原有复杂度;不能仅把已有集中逻辑搬走,再增加输入输出接口。行数不是唯一指标,但净增长必须换来具体收益。

请按这个目标重新考虑范围,保留执行、权限和隐私契约,删去没有必要的透传和重复表示,说明最终消除了哪些维护点。如果抽取本身无法带来这样的收益,保留原地实现也可以。我会撤回当前批准,待设计取舍重新收敛后复审;这不是新发现的运行时缺陷。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants