Skip to content

refactor: clarify tool input coercion - #380

Open
posva wants to merge 2 commits into
mainfrom
refactor/tool-input-positional-args
Open

refactor: clarify tool input coercion#380
posva wants to merge 2 commits into
mainfrom
refactor/tool-input-positional-args

Conversation

@posva

@posva posva commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Move tool-input conversion into a browser-safe shared module with explicit RPC and command adapters while retaining the old internal helper as a deprecated compatibility shim.

This is subjective but I found this to be clearer

Copilot AI lite review requested due to automatic review settings September 10, 2026 09:26
@coldtea-pr-lens

coldtea-pr-lens Bot commented Sep 10, 2026

Copy link
Copy Markdown

◈ PR Lens

🟢 +0 new · 🟠 ~3 changed · 🔴 -0 removed · 2 flows · 8 files · commit d3993ce


Architecture

Architecture diagram for devframes/devframe at d3993ce

3 components touched across 3 lanes.

Open the interactive canvas


Data flow

Data flow diagram for devframes/devframe at d3993ce

Executing a browser WebMCP tool · Executing a Hub command tool

Open the interactive canvas


The other flows — 1 sequence

Executing a Hub command tool

Sequence diagram of Executing a Hub command tool in devframes/devframe

View

  • Architecture lens
  • Data flow lens
  • Expand every detail

Tip

Run npx skills add coldteadotai/pr-lens, then tell your coding agent: "Diagram the change you just made with PR Lens and attach it to the pull request."

🪧 More tips
  • Run npx @coldtea/pr-lens-cli analyze --base origin/main on a branch, then npx @coldtea/pr-lens-cli render .pr-lens/graph.json. Same lenses, your own model key, before the pull request exists.
  • Untick Architecture lens or Data flow lens under View to hide a diagram, or tick Expand every detail to open every section. The comment redraws in a few seconds.
  • Click the link under each diagram to open it on a canvas you can zoom, pan and step through.
  • The diagrams are links. Click one to open it on the canvas, then press W or click play to walk through the change.
  • Open a diagram on the canvas, then press W or click play to walk through the change one step at a time.
  • The CLI's render reads .github/pr-lens.yml and applies your renames, exclusions and lane pins at draw time.
  • Set github.comment.collapsed: true in .github/pr-lens.yml to fold the comment behind one View architecture and data flow row. Drawing still runs on every push.
  • Add .github/workflows/pr-lens.yml with coldteadotai/pr-lens/packages/action@v0 and your model provider's key as its api-key to run PR Lens from your own CI. Any /chat/completions endpoint works.
  • Push a commit and the comment redraws for the new head. A slow older run never overwrites a newer one.
  • Switch GitHub to dark mode and the diagrams follow. The moving dots are this pull request's data in motion.

Thanks for using PR Lens! It's built by Coldtea, free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
devframe Skipped Skipped Sep 10, 2026 9:35am UTC

Copilot AI 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.

🟡 Changes recommended

collectPositionalArgs() currently mishandles an explicit argumentCount = 0 (common when definitions use args: []), which can lead to incorrect coercion behavior and should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors tool-input coercion by moving positional-argument extraction into a shared, browser-safe module and wiring separate adapters for RPC-backed tools vs command-backed tools, while keeping the previous helper as a deprecated shim for compatibility.

Changes:

  • Introduces packages/devframe/src/tool-input.ts with toolInputToRpcArgs() / toolInputToCommandArgs() and deprecates coerceAgentPositionalArgs() / AgentArgsFallback.
  • Switches node-side and browser-side call sites (hub command execution, agent host RPC invoke, WebMCP execution) to the new conversion helpers.
  • Replaces the old node-only coercion module/tests with new shared tests and updates internal API snapshots/exports.
File summaries
File Description
tests/snapshots/tsnapi/devframe/internal.snapshot.js Updates internal snapshot exports to include toolInputToCommandArgs.
tests/snapshots/tsnapi/devframe/internal.snapshot.d.ts Marks legacy coercion types/functions as deprecated and adds toolInputToCommandArgs typing to the internal snapshot.
packages/hub/src/node/host-commands.ts Uses toolInputToCommandArgs when invoking tool-backed commands via the hub.
packages/devframe/src/tool-input.ts Adds the shared, browser-safe tool-input → positional-args conversion utilities and the deprecated compatibility shim.
packages/devframe/src/node/host-agent.ts Uses toolInputToRpcArgs for invoking RPC tools from the node-side agent host.
packages/devframe/src/node/agent-args.ts Removes the old node-only coercion helper (superseded by shared module).
packages/devframe/src/node/tests/agent-args.test.ts Removes old coercion tests (replaced by shared tool-input tests).
packages/devframe/src/internal/index.ts Re-exports the new internal helper and re-exports legacy helpers from the new module with deprecation.
packages/devframe/src/client/webmcp.ts Uses toolInputToRpcArgs to keep WebMCP tool execution aligned with node-side behavior.
packages/devframe/src/tests/tool-input.test.ts Adds tests for the new shared coercion behavior.
Review details
  • Files reviewed: 9/10 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/devframe/src/tool-input.ts Outdated
Comment thread packages/devframe/src/__tests__/tool-input.test.ts
Copilot AI review requested due to automatic review settings September 10, 2026 09:35

Copilot AI 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.

🔵 Needs a closer look

The new shared coercion helper uses prototype-chain key checks (in) for argN detection, which should be switched to own-property checks to avoid surprises with untrusted tool input.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/devframe/src/tool-input.ts:22

  • collectPositionalArgs uses the in operator to detect argN keys. Because in walks the prototype chain, a crafted input like Object.create({ arg0: ... }) (or a Proxy with a has trap) could be treated as having positional args even when they’re not own properties. Prefer checking own-properties only to avoid prototype-chain surprises when tool input is untrusted.
  • Files reviewed: 9/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants