Allow artifacts to be passed as tool arguments#2189
Conversation
🦋 Changeset detectedLatest commit: ae38f8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) resolveArgs() — Missing unit test coverage for core feature
Issue: The resolveArgs() method (ArtifactParser.ts lines 220-254) is the primary new feature enabling artifact-as-argument functionality, but it has no direct unit tests. This method handles: (1) detecting { $artifact, $tool } sentinel objects, (2) calling artifactService.getArtifactFull() to fetch full data, (3) recursive traversal of nested objects/arrays, (4) fallback behavior when resolution fails.
Why: Without tests, several regressions could go undetected: nested artifact refs might not resolve correctly, resolution failures silently return the original sentinel object (tool receives { $artifact: 'x', $tool: 'y' } instead of failing), arrays with mixed refs and values might be corrupted. This is the primary feature of the PR and is called on every tool execution path.
Fix: Add dedicated unit tests covering:
- Top-level artifact ref resolution
- Nested artifact refs in objects
- Artifact refs in arrays
- Resolution failure fallback behavior
- Primitive passthrough
describe('resolveArgs', () => {
it('resolves top-level artifact ref to full data', async () => {
mockArtifactService.getArtifactFull.mockResolvedValue({ data: { title: 'Resolved' } });
const result = await parser.resolveArgs({ $artifact: 'art-1', $tool: 'tool-1' });
expect(result).toEqual({ title: 'Resolved' });
});
it('resolves nested artifact ref in object', async () => {
mockArtifactService.getArtifactFull.mockResolvedValue({ data: { content: 'Full' } });
const result = await parser.resolveArgs({ doc: { $artifact: 'art-1', $tool: 'tool-1' }, other: 'value' });
expect(result).toEqual({ doc: { content: 'Full' }, other: 'value' });
});
it('returns original ref when resolution fails', async () => {
mockArtifactService.getArtifactFull.mockResolvedValue(null);
const ref = { $artifact: 'missing', $tool: 'tool' };
const result = await parser.resolveArgs(ref);
expect(result).toEqual(ref);
});
});Refs:
- ArtifactParser.ts:220-254 —
resolveArgs()implementation - ArtifactParser.typeSchema.test.ts — existing test file (can add
resolveArgstests here)
Inline Comments:
- 🔴 Critical:
artifact-components.mdx:118Incorrect import — SDK exportsfunctionTool, nottool - 🔴 Critical:
artifact-components.mdx:121Function name should befunctionTool - 🔴 Critical:
visual-builder/artifact-components.mdx:134Same incorrect import - 🔴 Critical:
visual-builder/artifact-components.mdx:136Same function name fix
🟠⚠️ Major (3) 🟠⚠️
🟠 1) ArtifactParser.ts:220-235 — Silent failure when artifact resolution fails
Issue: When resolveArgs() fails to resolve an artifact reference (line 231-235), it logs a warning but returns the original { $artifact, $tool } sentinel object unchanged. The tool then receives this sentinel object as its input instead of the expected data shape.
Why: This creates a silent failure mode where tools may crash or behave unexpectedly because they receive { $artifact: "...", $tool: "..." } instead of the actual artifact content. The docs and prompts promise full resolution but don't explain what happens when resolution fails. Debugging requires correlating the warning log with the tool failure.
Fix: Consider either:
- Throw an error that propagates to the tool execution error handler (Agent.ts lines 729-749) which already handles tool errors gracefully
- Return a clearly marked error object like
{ $artifactResolutionFailed: true, reason: 'Artifact not found', ... }that tools can check - Document the behavior and instruct tool authors to handle the fallback case
Refs:
- ArtifactParser.ts:230-235 — fallback return
- Agent.ts:729-749 — tool error handler
🟠 2) agents-docs/ — Documentation doesn't explain sentinel format for developers
files: agents-docs/content/typescript-sdk/structured-outputs/artifact-components.mdx, agents-docs/content/visual-builder/structured-outputs/artifact-components.mdx
Issue: The new "Passing Artifacts to Tools" section documents that tools receive complete artifact data, but it does not explain the agent-side syntax { "$artifact": "...", "$tool": "..." } that triggers this resolution. The sentinel format is only documented in the system prompts seen by LLMs, not in developer-facing docs.
Why: Developers reading this documentation may not understand that artifact passing is automatic/system-resolved. They may wonder how they should configure their tool's inputSchema to accept artifacts, or whether they need to handle artifact resolution themselves.
Fix: Add a clarifying subsection explaining:
- The sentinel format
{ "$artifact": "artifact-id", "$tool": "tool-call-id" }that agents use in tool arguments - That the runtime automatically resolves this to full artifact data before the tool executes
- What happens if resolution fails (per the behavior noted above)
Example addition:
"When the agent decides to pass an artifact to your tool, it uses a special reference format internally. The system automatically resolves this reference and provides the complete artifact data to your execute function. You simply define your inputSchema with the expected shape of the artifact data—no special handling is required."
Refs:
Inline Comments:
- 🟠 Major:
Agent.ts:1856collectProjectArtifactComponents()omits current agent's own artifact components
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
.changeset/petite-ends-trade.md:5Changeset message should use sentence case - 🟡 Minor:
conversations.ts:908Artifact IDs injected into XML without escaping
💭 Consider (5) 💭
💭 1) ArtifactParser.ts:18-22 — StreamPart uses data?: any - weak type safety
Issue: The StreamPart interface uses data?: any which provides no type safety for the artifact data payload. Downstream consumers cannot safely narrow the data field.
Why: Code that expects data.artifactId or data.typeSchema has no compile-time guarantee these fields exist. This is a design choice that trades type safety for flexibility.
Fix: Consider defining a discriminated union for StreamPart with specific data shapes if stronger typing is desired.
Refs: ArtifactParser.ts:18-22
💭 2) types.ts:39 — Naming inconsistency: allProjectArtifactComponents vs artifactComponents
Issue: The new property allProjectArtifactComponents uses a different naming convention than the existing artifactComponents property. The "all" prefix is unusual in this interface.
Fix: Consider projectArtifactComponents for consistency, or clarify the distinction in the comment.
Refs: types.ts:38-39
💭 3) artifact-retrieval-guidance.xml:5-17 — Concept economy: "FOUR WAYS" vs "three modes" mismatch
Issue: The guidance lists "FOUR WAYS TO ACCESS ARTIFACT DATA" but PromptConfig describes "three modes of use" (create, reference, pass). The mismatch may confuse LLMs about the conceptual model.
Fix: Align the framing: "Two ways to ACCESS existing artifact data (reference in text for preview, pass to tool for full) plus get_reference_artifact for explicit retrieval. One way to CREATE new artifacts."
Refs: artifact-retrieval-guidance.xml:5-17
💭 4) Agent.ts:1877-1879, ArtifactParser.ts:269-272 — Empty catch blocks without logging
Issue: Several catch blocks silently swallow errors without logging: collectProjectArtifactComponents() (Agent.ts:1877), parseCreateAttributes() (ArtifactParser.ts:269-272).
Fix: Add debug-level logging to help diagnose issues: catch (error) { logger.debug({ error }, 'Failed to parse, using fallback'); ... }
💭 5) ArtifactParser.ts:217-235 — Sentinel format is first-of-kind convention
Issue: The { "$artifact": "...", "$tool": "..." } sentinel is a novel pattern in the codebase. Existing stream event schemas use discriminated unions with a type field rather than $-prefixed keys. This establishes a precedent.
Fix: Consider documenting why this format was chosen, or adding a constant for the sentinel keys to prevent drift between prompt and runtime.
🚫 REQUEST CHANGES
Summary: This is a well-architected feature that adds valuable capability for artifact-as-argument passing. However, two blocking issues need resolution before merge:
-
Critical docs bug: The code examples import
toolwhich doesn't exist in the SDK (should befunctionTool). Users will get build errors. -
Missing test coverage: The core
resolveArgs()method has no unit tests despite being the primary feature and affecting every tool execution path.
Additionally, the silent failure behavior when artifact resolution fails (returning the sentinel object unchanged) may cause confusing downstream errors and should at minimum be documented, or ideally made more explicit.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
PromptConfig.ts:418-438 |
First-contact legibility - timing rules confusing | Prompt guidance is comprehensive; timing is explained, just detailed |
Agent.ts:2034-2040 |
Updated tool description may confuse LLM | Description is helpful and accurate; negation is common in LLM guidance |
PromptConfig.ts:414-534 |
Extensive prompt instructions may exceed token limits | Token estimation exists; this is a design tradeoff, not a bug |
conversations.ts:894 |
reconstructMessageText exported but could be private |
Function is tested separately, export is intentional |
ArtifactParser.ts:131-145 |
typeSchema property lacks schema definition |
data: z.any() in stream schemas is intentional flexibility |
collectProjectArtifactComponents |
Method returns any[] |
Type looseness is consistent with existing patterns |
buildTypeSchemaMap |
Accepts any[] |
Same pattern as above |
reconstructMessageText |
Accepts any |
Defensive runtime handling is appropriate here |
Reviewers (9)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
3 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
4 | 1 | 0 | 0 | 4 | 0 | 0 |
pr-review-product |
5 | 1 | 1 | 0 | 0 | 0 | 2 |
pr-review-errors |
4 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-architecture |
4 | 0 | 1 | 0 | 0 | 0 | 1 |
pr-review-llm |
5 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-consistency |
7 | 0 | 2 | 0 | 1 | 0 | 3 |
pr-review-types |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 37 | 3 | 5 | 0 | 7 | 0 | 14 |
Note: Multiple reviewers flagged the same issues (silent failure, missing tests) — these were deduplicated in the summary.
| For example, a document processing tool can access the full `content` array from a citation artifact even though `content` is a non-preview field: | ||
|
|
||
| ```typescript | ||
| import { tool } from "@inkeep/agents-sdk"; |
There was a problem hiding this comment.
🔴 CRITICAL: Incorrect import - SDK exports functionTool, not tool
Issue: The code example imports tool from @inkeep/agents-sdk, but this export does not exist. The SDK exports functionTool for creating function tools with execute handlers.
Why: Users copying this example will get a build/runtime error: Module '@inkeep/agents-sdk' has no exported member 'tool'. This blocks developers from implementing the documented feature.
Fix:
| import { tool } from "@inkeep/agents-sdk"; | |
| import { functionTool } from "@inkeep/agents-sdk"; |
Refs:
- packages/agents-sdk/src/index.ts — SDK exports
| import { tool } from "@inkeep/agents-sdk"; | ||
| import { z } from "zod"; | ||
|
|
||
| const summarizeTool = tool({ |
There was a problem hiding this comment.
🔴 CRITICAL: Function name should be functionTool, not tool
Issue: Continuation of the import fix — the function call should also use functionTool.
Why: Consistent with the import fix above.
Fix:
| const summarizeTool = tool({ | |
| const summarizeTool = functionTool({ |
Refs:
- packages/agents-sdk/src/index.ts — SDK exports
| When a tool receives an artifact as an argument, it has access to all fields — not just the preview ones. For example, a document processing tool can read the full `content` array even though `content` is a non-preview field: | ||
|
|
||
| ```typescript | ||
| import { tool } from "@inkeep/agents-sdk"; |
There was a problem hiding this comment.
🔴 CRITICAL: Incorrect import - SDK exports functionTool, not tool
Issue: Same issue as the TypeScript SDK docs — the SDK exports functionTool, not tool.
Why: Users copying this example will get a build error.
Fix:
| import { tool } from "@inkeep/agents-sdk"; | |
| import { functionTool } from "@inkeep/agents-sdk"; |
Refs:
- packages/agents-sdk/src/index.ts — SDK exports
| ```typescript | ||
| import { tool } from "@inkeep/agents-sdk"; | ||
|
|
||
| const summarizeTool = tool({ |
There was a problem hiding this comment.
🔴 CRITICAL: Function name should be functionTool, not tool
Issue: Continuation of the import fix.
Fix:
| const summarizeTool = tool({ | |
| const summarizeTool = functionTool({ |
| } | ||
| } | ||
|
|
||
| private collectProjectArtifactComponents(): any[] { |
There was a problem hiding this comment.
🟠 MAJOR: Method omits current agent's own artifact components
Issue: collectProjectArtifactComponents() iterates over subAgents to gather artifact components but never includes this.artifactComponents. If an agent has artifact components AND its subAgents also have artifact components, the agent's own components will be excluded from allProjectArtifactComponents.
Why: This could cause schema lookup failures when an artifact created by the current agent (whose type is defined in the current agent's artifact components) is passed to a tool — the type won't be found in the type schema map, resulting in "Schema not available" in prompts.
Fix: Consider seeding the all array with this.artifactComponents before iterating subAgents:
private collectProjectArtifactComponents(): any[] {
const project = this.executionContext.project;
try {
const agentDefinition = project.agents[this.config.agentId];
if (!agentDefinition) {
return this.artifactComponents;
}
const seen = new Set<string>();
const all: any[] = [];
// Include current agent's artifact components first
for (const ac of this.artifactComponents) {
if (ac.name && !seen.has(ac.name)) {
seen.add(ac.name);
all.push(ac);
}
}
for (const subAgent of Object.values(agentDefinition.subAgents)) {
// ... rest of iteration
}
return all.length > 0 ? all : this.artifactComponents;
} catch {
return this.artifactComponents;
}
}Refs:
- agents-api/src/domains/run/agents/Agent.ts:2057 — where
allProjectArtifactComponentsis used
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| Allowed Artifacts to be Passed as Tool Arguments |
There was a problem hiding this comment.
🟡 Minor: Changeset message should use sentence case
Issue: The changeset message uses title case ("Allowed Artifacts to be Passed as Tool Arguments") rather than sentence case per CLAUDE.md guidelines.
Why: Consistency with changelog conventions.
Fix:
| Allowed Artifacts to be Passed as Tool Arguments | |
| Allow artifacts to be passed as tool arguments |
Refs:
- CLAUDE.md changelog guidance — "Sentence case: 'Add new feature' not 'add new feature'"
| if (part.type === 'data') { | ||
| try { | ||
| const data = typeof part.data === 'string' ? JSON.parse(part.data) : part.data; | ||
| if (data?.artifactId && data?.toolCallId) { |
There was a problem hiding this comment.
🟡 Minor: Artifact IDs injected into XML without escaping
Issue: The reconstructMessageText function injects artifactId and toolCallId directly into XML-like tags without HTML/XML escaping. If these IDs contain special characters (quotes, angle brackets), they could malform the XML or cause parsing issues.
Why: While artifact IDs are typically safe alphanumeric strings, defensive escaping prevents edge-case bugs if IDs contain unexpected characters.
Fix: Consider escaping the values:
// Helper function
const escapeXml = (s: string) => s.replace(/&/g, '&').replace(/"/g, '"').replace(/</g, '<').replace(/>/g, '>');
// In the function
return `<artifact:ref id="${escapeXml(data.artifactId)}" tool="${escapeXml(data.toolCallId)}" />`;Refs:
|
Need to check claude's comment |
1 participant
Adds support for passing saved artifacts by reference ({ "$artifact": "...", "$tool": "..." }) as tool call arguments. Before execution, ArtifactParser.resolveArgs() recursively walks the tool args and replaces any artifact reference objects with the full artifact data. This prevents the LLM from needing to re-transcribe or inline large artifact payloads when chaining tool calls.
Updated system prompts (both v1 PromptConfig variants) instruct the model to always use the reference object form rather than writing artifact data inline.