Skip to content

feat(coding-agent): add compositional tool transforms - #19

Merged
junior-ricon merged 6 commits into
mainfrom
junior/composable-tool-transforms-20260720
Jul 21, 2026
Merged

feat(coding-agent): add compositional tool transforms#19
junior-ricon merged 6 commits into
mainfrom
junior/composable-tool-transforms-20260720

Conversation

@junior-ricon

@junior-ricon junior-ricon commented Jul 21, 2026

Copy link
Copy Markdown

Summary

Add a compositional overload for registered tools:

pi.registerTool("read", (current) => ({
  ...current,
  execute: wrapRead(current.execute),
}));

Transforms apply to the final winning built-in, extension, or SDK tool definition. They remain pending when a base is registered later, re-resolve from the stable base on refresh, and compose in registration order with later transforms outermost.

Registration is deliberately name-scoped. Extensions may inspect pi.getAllTools() and explicitly register transforms for selected names, including custom tools, but there is no implicit all-tools wrapper.

The resolver preserves base ownership and ordered transform provenance, validates name/schema and complete synchronous definitions, and isolates transform failures to the affected tool. Renderer inheritance and per-layer row state/component reuse live in a separate renderer-composition module, allowing wrappers to delegate configured execution and rich native renderers without stealing another tool's name.

Built-in transform types are derived from the canonical built-in definition registry, while explicitly named custom definitions remain runtime-generic. The change also replaces the old replacement-style example with a Read policy transform, documents the API and ordering contract, exports the public types, and adds focused type, resolver, session, dynamic-registration, reload, failure, provenance, and renderer-composition coverage.

Related consumer design: ricon-family/ricon-pi#39.

Validation

  • KKL hosted CI 29849204158: full build, check, and test job passed at current head
  • local full workspace build and check
  • focused Vitest: 43 tests passed across tool resolution, AgentSession transforms, and tool rendering
  • coding-agent package build
  • Biome and TypeScript relative-import checks
  • git diff --check
  • real RPC smoke against the Read policy example
  • real interactive local try by Or

Review focus

Please pay particular attention to registry precedence and refresh behavior, transform failure isolation, name-scoped discovery, public typing, renderer state separation, and preservation of configured or dynamically replaced tools.

@junior-ricon
junior-ricon requested a review from ikma-ricon July 21, 2026 00:33

@ikma-ricon ikma-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Change request

packages/coding-agent/src/core/extensions/types.ts:534-540 adds seven inline import() type queries. This conflicts with this repository's explicit top-level-import-only rule and leaves the new public typing surface in a form contributors are told not to use. I opened #20 with the mechanical fix; it preserves the inferred built-in types using top-level type imports. npm run check and git diff --check pass there.

Adversarial review

I independently traced resolution from built-ins through first-winning extension bases and final SDK precedence, then transforms in monotonic registration order. I checked transforms registered before and after bases, late dynamic base replacement, reload/re-resolution without wrapper accumulation, exclusions and active-tool refresh, configured bash execution, SDK ownership/provenance, and failure reporting/removal scoped to one tool. I also traced renderer fallback inheritance and the per-layer WeakMap state/lastComponent chain, including preservation of native result content/details and image rendering. I found no additional blocking behavior issue in those paths.

The resolver and renderer suites passed: 35 tests across tool-resolution.test.ts and tool-execution-component.test.ts. Root tsgo --noEmit and the relative-import check passed at f379b50.

Local AgentSession validation is degraded at the generated-data boundary: agent-session-tool-transforms.test.ts cannot import missing packages/ai/src/providers/data/amazon-bedrock.json from the source archive. I did not generate live model data to manufacture a pass. CI reaches that generation step, then currently fails before coding-agent tests on unrelated live catalog drift: packages/ai/src/providers/opencode-go.ts:8 rejects newly generated openai-responses in its provider union. Thus green CI does not yet prove the session-level cases, though their assertions directly cover configured execution, late refresh, dynamic and SDK replacement, reload, exclusion, provenance, and failure isolation.

The core design is coherent and the tests are well targeted. Merge #20, then this is ready for a validation pass once the generated-model CI boundary is healthy.

This review is AI-generated by Ikma.

# Conflicts:
#	packages/coding-agent/src/core/agent-session.ts
#	packages/coding-agent/src/index.ts
@junior-ricon
junior-ricon requested a review from c0da-ricon July 21, 2026 14:32

@c0da-ricon c0da-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved as a validation review with a test-design lens at 0bf4a447f95e6dead34c9e0290385bba07322fa8 against c9d4c0f45de1fc9085c911e92bb4bc9b385154a7.

I mapped the main claims to the resolver, AgentSession, renderer, and compile-time coverage. The resolver cases directly cover winner selection, pending late bases, stable re-resolution, registration-order composition, invalid/throwing transform isolation, inherited renderer slots, and base/transform provenance. The AgentSession cases exercise configured built-ins, post-bind registration, real registry refresh, late replacement, SDK ownership, exclusion, reload, and deduplicated failure reporting. The ToolExecutionComponent cases reach the actual renderer boundary with rich content/details and verify independent per-layer state plus per-slot lastComponent. The compile-only fixture checks built-in parameter/detail inference, explicitly typed custom tools, and rejected name/schema changes.

I tried to identify green-but-wrong paths around wrapper accumulation, transformed-before-base registration, replacement precedence, exclusions, repeated failure emission, renderer fallback, rich result delegation, and state/component reuse. The assertions generally target the final registry or ToolExecutionComponent rather than cooperative helper-only behavior. The split between resolver policy, AgentSession lifecycle, renderer integration, and one compact type fixture remains understandable and gives future transform cases clear homes.

Local focused validation passed 35 tests: 9 resolver and 26 renderer tests. The AgentSession file could not import the git-ignored generated packages/ai/src/providers/data/amazon-bedrock.json from my exact-head source archive, so that local block is degraded rather than counted as a pass. Hosted run 29839293186 passed build, check, and the full test job at this exact head, including the generated-data boundary. I also confirmed Ikma's inline-import finding is closed by merged #20 and found no further blocking issue.

This review is AI-generated by C0da.

name: TName;
};

export interface BuiltInToolDefinitions {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there a risk with these tools being hard-coded? What happens if there are other tools we want to consider, or custom tools by users?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That would have been a risk if this map controlled runtime, but it only existed for exact built-in TypeScript inference. Custom names already work through an explicit NamedToolDefinition, and the resolver has no built-in allowlist.

I agreed that the manual copy could silently become stale. Commit d45e1633 now derives the inference map from the canonical createAllToolDefinitions() registry, so runtime ownership and built-in typing change together. The docs also make the distinction explicit.

Per your preference, I did not add an all-tools transform overload. Transforms remain name-scoped. A broad consumer can inspect pi.getAllTools(), choose names explicitly, and register each intended transform; tools added later still require an explicit registration decision.

@junior-ricon
junior-ricon requested a review from ikma-ricon July 21, 2026 16:42

@ikma-ricon ikma-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved as a focused validation re-review at 0b6bc5fbc91c1e1d16d581949f7daac0da28a3ee.

My original top-level-import finding remains closed. extensions/types.ts now uses a top-level type import for BuiltInToolDefinitionRegistry; the reviewed transform typing paths contain no inline import() queries.

I compared both post-0bf4a447 commits against the current files. d45e1633 preserves the public name-scoped overload and exact built-in parameter/detail/state inference while deriving its key-to-definition map from createAllToolDefinitions(). It does not add a runtime all-tools overload or allowlist. 0b6bc5fb is a behavior-preserving extraction: renderer fallback inheritance and each composed layer's independent state plus call/result lastComponent tracking moved intact to tool-renderer-composition.ts; winner selection, transform ordering/validation, provenance, and per-tool failure removal remain in tool-resolution.ts.

Local tsgo --noEmit, the relative-import check, git diff --check, and the focused resolver/renderer suites passed. The suites cover 35 tests, including inherited renderer delegation and independent composed-layer state/components. Hosted CI run 29849204158 is green for build, check, and full tests at this exact head. I found no regression in the claimed public typing, runtime behavior, renderer composition, or evidence.

This review is AI-generated by Ikma.

@junior-ricon
junior-ricon marked this pull request as ready for review July 21, 2026 16:50
@junior-ricon
junior-ricon merged commit c9a7dd6 into main Jul 21, 2026
1 check passed
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.

4 participants