fix(core): load artifacts and MCP resources when other tools are call…

[Varun-S10]

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: Artifacts not being loaded by load_artifacts when other tools are called #632

2. Or, if no issue exists, describe the change:

Problem:
When an agent calls load_artifacts alongside other tools in the same turn (such as parallel tool calls or sequential tool calls like load_skill), the artifact content is never sent to the model.

This happens because LoadArtifactsTool only checks the first item parts[0] of the last message contents[contents.length - 1].

1. If another tool runs in parallel before load_artifacts, it is at parts[0], so load_artifacts is ignored.
2. If another tool runs in a later step in the same turn, contents[contents.length - 1] points to the other tool's response, so the earlier load_artifacts response is ignored.

(The same bug exists in LoadMcpResourceTool for MCP resources).

Solution:

1. Updated appendArtifactsToLlmRequest in load_artifacts_tool.ts and appendResourcesToLlmRequest in load_mcp_resource_tool.ts to scan all parts across the active conversation turn.
2. Finds all load_artifacts (or load_mcp_resource) responses in the current turn and injects all requested artifacts/resources into llmRequest.contents.
3. Bounded the search to the active turn so artifacts from previous conversation turns are not re-injected.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Summary of npm test results:

• Added unit tests in core/test/tools/load_artifacts_tool_test.ts covering:
  • Parallel tool execution ([other_tool, load_artifacts])
  • Sequential multi-step execution (load_artifacts followed by other_tool)
  • Turn isolation (ensuring artifacts from prior turns are not re-injected)
• Added unit tests in core/test/tools/mcp/load_mcp_resource_tool_test.ts for parallel and sequential MCP resource loading.
• Ran full core unit test suite: 201 test files passed (2,736 tests passed).
• Ran linter: npm run lint passed with 0 errors.

Manual End-to-End (E2E) Tests:

Tested with an agent executing create_test_artifact, load_artifacts, and load_skill in the same turn:

Before Fix:

Tools called: [create_test_artifact, load_artifacts, load_skill]
{
  msg: '[processLlmRequest] artifact in request?',
  artifactInRequest: null
}
Result: Artifact loaded in LLM Request? false

After Fix:

Tools called: [create_test_artifact, load_artifacts, load_skill]
{
  msg: '[processLlmRequest] artifact in request?',
  artifactInRequest: [
    { text: 'Artifact evaluation_input.csv is:' },
    { text: 'case_id,verdict,code\n1,pass,SECRET-CODE-7Q4Z\n' }
  ]
}
Result: Artifact loaded in LLM Request? true

Checklist

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

Additional context

• Fully backward-compatible with single-tool turns.
• Fixes both LoadArtifactsTool and LoadMcpResourceTool.

[AmaadMartin]

The bug is real and the diagnosis of it is correct — reading only parts[0] of the last content does miss both the parallel and the sequential case.

The turn-boundary rule is where this needs work: one of its two conditions also matches a rewritten sub-agent event, so the fix does not hold in multi-agent apps. Details inline, along with the duplication between the two tools.

On the app_loader_test timeout: that one is a genuine flake, and you are right to raise it — it timed out at 40s on #637 with a diff that could not have caused it. It is unrelated to this change though, so it is better as its own PR; a reviewer of the artifact fix should not have to reason about CI timeouts, and it will read oddly in the changelog.

[AmaadMartin]

Not a nit. A sub-agent event matches this boundary, so the fix does not hold in a multi-agent app.

getContents rewrites every event from another agent through convertForeignEvent (content_processor_utils.ts:206). That flattens each functionCall and functionResponse into prose and emits:

const content: Content = {role: 'user', parts: [{text: 'For context:'}]};

Role user, no functionResponse — exactly this condition. So it reads as a fresh user prompt:

user   : "show me the file"
model  : [functionCall load_artifacts]
user   : [functionResponse load_artifacts]
user   : [text "For context: [sub_agent] said ..."]   <- taken as a new turn
model  : [functionCall other_tool]
user   : [functionResponse other_tool]

startIndex lands on the foreign content, the load_artifacts response falls outside it, and the artifact is dropped again.

Dropping the user clause fixes it: the model-response clause alone still passes your turn-isolation test, where model: [text 'Old artifact was processed.'] is the boundary. The case that clause covers on its own is a previous turn that ended without any model text, which I did not find in the tests.

Same code in load_mcp_resource_tool.ts:111.

[AmaadMartin]

I retract this finding. It is wrong, and I am sorry for the delay it caused.

I traced the multi-agent flows again at b0853a2. A foreign event lands before the current agent's own load_artifacts response, not after it. For a transfer into agent B, the contents are [user, foreign(A), foreign(A), B model call, B response]. The loop stops at the last foreign event, so the scan from startIndex still finds the response. The fix holds.

The boundary also matches the repo's own turn rule. getCurrentTurnContents starts a turn at a user event or at a foreign-agent event (content_processor_utils.ts:136).

Ignore my advice to drop the user clause. That clause is correct.

[AmaadMartin]

Nit. This block is duplicated verbatim in load_mcp_resource_tool.ts:104-134.

The boundary scan and the collection loop are identical in both files. Only two things vary: the tool name, and the response key (artifact_names versus resource_names). That is about 30 lines carried twice, and the bug in my other comment has to be fixed in both places.

One helper in a shared module takes both:

export function collectNamesFromCurrentTurn(
  contents: Content[], toolName: string, responseKey: string,
): string[]

Each tool then calls it and keeps only its own loading loop.

[AmaadMartin]

Nit. The second half of this test is dead, and it would be wrong if it ever fired.

functionResponse.name === this.name ||
functionResponse.name === 'load_artifacts'

The constructor hardcodes the name:

super({name: 'load_artifacts', ...});   // load_artifacts_tool.ts:91

so this.name is always 'load_artifacts' and the literal never adds a match. If someone later subclasses this with a different name, the literal starts matching a different tool's response instead. Keep this.name only.

load_mcp_resource_tool.ts:127 repeats it with 'load_mcp_resource'.

[AmaadMartin]

The branch conflicts with main, so I cannot approve it yet. GitHub reports mergeable: false.

One file conflicts: tests/integration/app_loader/app_loader_test.ts. Commit b0853a2 sets TEST_EXECUTION_TIMEOUT to 80000. main set the same constant to 60000. Please rebase.

That timeout change is not part of this fix. A separate PR for it keeps this one clean.

The fix itself is good. I retracted my earlier blocking comment. Only the two nits remain, and they do not block.

[AmaadMartin]

Approved at b0853a2. All checks pass and no blocking finding stands.

I withdrew my earlier Not a nit. about the sub-agent boundary. It was wrong. Request processors run before the tool loop, so the tool does see convertForeignEvent output, but the converted foreign event always lands before this agent's own load_artifacts response. The scan at load_artifacts_tool.ts:181-203 still finds it, and that matches the turn rule at content_processor_utils.ts:136. The one flow that drops the artifact drops it under the old code too.

One thing to do before merge: the branch conflicts with main in tests/integration/app_loader/app_loader_test.ts. This PR sets the timeout to 80000 and main set it to 60000. The conflict is unrelated to the fix, so please rebase and keep whichever value the suite needs.

Two nits stand and neither blocks: the duplicated block at load_mcp_resource_tool.ts:99-140, and the dead || 'load_artifacts' clause at load_artifacts_tool.ts:189.

[Varun-S10]

Approved at b0853a2. All checks pass and no blocking finding stands.

I withdrew my earlier Not a nit. about the sub-agent boundary. It was wrong. Request processors run before the tool loop, so the tool does see convertForeignEvent output, but the converted foreign event always lands before this agent's own load_artifacts response. The scan at load_artifacts_tool.ts:181-203 still finds it, and that matches the turn rule at content_processor_utils.ts:136. The one flow that drops the artifact drops it under the old code too.

One thing to do before merge: the branch conflicts with main in tests/integration/app_loader/app_loader_test.ts. This PR sets the timeout to 80000 and main set it to 60000. The conflict is unrelated to the fix, so please rebase and keep whichever value the suite needs.

Two nits stand and neither blocks: the duplicated block at load_mcp_resource_tool.ts:99-140, and the dead || 'load_artifacts' clause at load_artifacts_tool.ts:189.

Hi @AmaadMartin, thank you for the review and approval. I have synced with main, resolved the timeout conflict in app_loader_test.ts (keeping 60000), and addressed the nits in load_artifacts_tool.ts and load_mcp_resource_tool.ts.

All checks and integration tests are passing cleanly. Could you please take a look?

[AmaadMartin]

Re-reviewed at 74dd6f8. The fix is intact and correct.

Both tools now scan every function response in the current turn (load_artifacts_tool.ts:180-199, load_mcp_resource_tool.ts:118-136). The three new tests cover the parallel, sequential, and turn-isolation cases. My earlier dead-clause nit is fixed: both tools match on this.name only (load_artifacts_tool.ts:186, load_mcp_resource_tool.ts:124). The duplication nit still stands and does not block.

The merge commit also reformats eight tests/integration/workflows/*/model_responses.json fixtures (array collapse only). That churn is unrelated to the fix. A revert keeps the diff clean, but it is optional.

CI: run-tests passed on ubuntu and macOS. Windows was still running at review time. No blocking finding stands.

[AmaadMartin]

Approve. I re-verified the turn scan against the source at 6b794e5.

• I retract my earlier "Not a nit" about a sub-agent boundary (see that thread). A foreign event lands before the current agent's own load_artifacts response, so the scan still finds the response. The fix holds.
• My dead name-match nit is fixed. Both files now match functionResponse.name === this.name only.
• One nit stays open and does not block: the boundary scan and the collection loop are duplicated in both tools.

The diff adds no any, cast, or suppression. CI is green on this head, including run-tests on all three platforms.