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 resolveArgs tests here)

Inline Comments:

• 🔴 Critical: artifact-components.mdx:118 Incorrect import — SDK exports functionTool, not tool
• 🔴 Critical: artifact-components.mdx:121 Function name should be functionTool
• 🔴 Critical: visual-builder/artifact-components.mdx:134 Same incorrect import
• 🔴 Critical: visual-builder/artifact-components.mdx:136 Same 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:

1. Throw an error that propagates to the tool execution error handler (Agent.ts lines 729-749) which already handles tool errors gracefully
2. Return a clearly marked error object like { $artifactResolutionFailed: true, reason: 'Artifact not found', ... } that tools can check
3. 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:

1. The sentinel format { "$artifact": "artifact-id", "$tool": "tool-call-id" } that agents use in tool arguments
2. That the runtime automatically resolves this to full artifact data before the tool executes
3. 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:

• typescript-sdk/artifact-components.mdx:111-138
• visual-builder/artifact-components.mdx:129-168

Inline Comments:

• 🟠 Major: Agent.ts:1856 collectProjectArtifactComponents() omits current agent's own artifact components

🟡 Minor (2) 🟡

Inline Comments:

• 🟡 Minor: .changeset/petite-ends-trade.md:5 Changeset message should use sentence case
• 🟡 Minor: conversations.ts:908 Artifact 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.

Summary: This is a well-architected feature that adds valuable capability for artifact-as-argument passing. However, two blocking issues need resolution before merge:

1. Critical docs bug: The code examples import tool which doesn't exist in the SDK (should be functionTool). Users will get build errors.

2. 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.

PR Review Summary

(1) Total Issues | Risk: Low

🟡 Minor (0) 🟡

No new minor issues in the delta.

💭 Consider (0) 💭

No new considerations in the delta.

🕐 Pending Recommendations (1)

• 🟡 conversations.ts:908 Artifact IDs injected into XML without escaping — defensive escaping prevents edge-case bugs if IDs contain unexpected characters

Summary: Great work addressing the prior review feedback! The critical and major issues have all been resolved:

✅ Tests added — resolveArgs() now has comprehensive unit test coverage (lines 276-333)
✅ Docs fixed — Both artifact-components.mdx files now correctly import functionTool
✅ Agent fix applied — collectProjectArtifactComponents() now seeds with current agent's components first
✅ Changeset corrected — Now uses sentence case per guidelines
✅ Silent failure documented — The fallback behavior is now explicitly tested, confirming it's intentional design

The only remaining item is the XML escaping suggestion (Minor) which is a defensive hardening measure for an unlikely edge case. This can be addressed in a follow-up if desired.

This PR is ready to merge! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

This is a delta re-review scoped to changes since the last automated review (Feb 24), which approved this PR.

Delta Changes Reviewed

Verification

• Type changes: collectProjectArtifactComponents() now correctly uses ArtifactComponentApiInsert[] for return type, local variable, and cast — properly addressing prior review feedback
• Comment cleanup: Excess comment removed from allProjectArtifactComponents in types.ts
• JSDoc fix: reconstructMessageText now accurately describes its purpose (handling multi-part content with artifact refs)
• Changeset: Now uses sentence case per guidelines

🕐 Pending Recommendations (1)

• 🟡 conversations.ts:909 Artifact IDs injected into XML without escaping — defensive hardening for unlikely edge case (can be addressed in follow-up)

💭 Consider (1)

Inline Comments:

• 💭 Consider: Agent.ts:1854-1884 Clarity improvements for collectProjectArtifactComponents() (suggestions provided per @amikofalvy's feedback)

Summary: All prior review feedback has been addressed correctly. The delta changes are type annotation improvements and documentation clarifications with no behavioral impact. The one remaining minor suggestion (XML escaping) is a defensive hardening measure for an unlikely edge case that can be addressed in a follow-up.

This PR is ready to merge! 🚀

I left a few questions for you that need answers before we can ship