Skip to content

Fix: escalate repeated load_skill_resource misses to RESOURCE_NOT_FOUND_FATAL - #831

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/skill-resource-not-found-fatal-guard
Open

Fix: escalate repeated load_skill_resource misses to RESOURCE_NOT_FOUND_FATAL#831
AmaadMartin wants to merge 4 commits into
mainfrom
fix/skill-resource-not-found-fatal-guard

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

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):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: load_skill_resource returns the same soft RESOURCE_NOT_FOUND on every miss. A model that guesses a resource path gets the same "try again" signal each time, so it can retry until the invocation ends. adk-python already escalates from the second miss (skill_toolset.py, LoadSkillResourceTool.run_async).

Solution: The tool now counts misses per invocation and returns RESOURCE_NOT_FOUND_FATAL from the second miss onward, with a message that tells the model to stop. The counter lives in toolContext.state under temp:_adk_skill_resource_not_found_count_<invocationId>, the same key shape adk-python uses: the temp: prefix keeps it out of persisted session state, and the invocation id resets the count each invocation. The six existing error-code literals move into an exported LoadSkillResourceErrorCode enum; the emitted strings do not change.

Behaviour change: a caller that saw RESOURCE_NOT_FOUND on the 2nd+ miss in one invocation now sees RESOURCE_NOT_FOUND_FATAL. That is the fix, and it matches adk-python.

Parity vs. local convention: parity wins on the wire — error-code strings, the counter key, and the fatal message text (including the em dash) match adk-python byte for byte. Local convention wins in-process — an enum instead of bare literals, and the existing snake_case error_code response key stays as it is in this file.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 720 open PRs. None implements this guard. #761 edits the same two files but in different hunks (the binary-resource branch, not the miss branch) and #729 ports the sibling run_skill_script guard in a different file, so this branches from main.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Commands run on the pushed commit:

npx vitest run --project unit:core core/test/tools/skills/load_skill_resource_tool_test.ts        # 14 passed
npx vitest run --project unit:core core/test/tools/skills/load_skill_resource_guard_run_test.ts   # 1 passed
npx vitest run --project unit:core core/test/tools/skills/skill_registry_test.ts                  # 35 passed (regression)
npm run build          # pass
npm run lint           # pass
npm run format:check   # pass
npm run docs:check     # pass

npm run ts:check reports 287 errors on this branch and 287 on main, none in the files this PR touches. That breakage is pre-existing and unrelated.

Coverage of the new code is 100% of lines and branches, measured with
--coverage.include='**/tools/skill/load_skill_resource_tool.ts': every statement and branch in the new block (source lines 141-162) is hit.

Proof the tests can fail. Each mutation was applied to the source alone, then reverted:

Mutation Result
failCount > 1 -> failCount > 2 4 unit tests fail, plus the agent-run test: expected [ 'RESOURCE_NOT_FOUND', …(1) ] to deeply equal [ 'RESOURCE_NOT_FOUND', …(1) ]
drop ${toolContext.invocationId} from the key resets the counter for a new invocation id fails
drop State.TEMP_PREFIX from the key prefix resets the counter for a new invocation id fails, and the agent-run test fails with expected [ …(2) ] to deeply equal [] — the counter key reaches the stored session state
em dash -> hyphen in the fatal message the two message-text tests fail

Scope note: the plan listed a fifth unit test for the first-miss soft error. The existing test returns error if resource not found already pins that exact response with full-object equality, so I did not add a duplicate.

Manual End-to-End (E2E) Tests:
core/test/tools/skills/load_skill_resource_guard_run_test.ts runs the guard through a real InMemoryRunner, LlmAgent, SkillToolset and InMemorySessionService; only the model is scripted. It asserts the escalation over real invocation ids and that no _adk_skill_resource_not_found_count_* key survives in the stored session.

To reproduce by hand: give an agent a SkillToolset, then in one invocation ask for two resource paths that do not exist. The first tool response carries RESOURCE_NOT_FOUND and the second carries RESOURCE_NOT_FOUND_FATAL. Start a second invocation on the same session; its first miss is RESOURCE_NOT_FOUND again.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 4 commits August 8, 2026 17:06
…ND_FATAL

A model that guesses a skill resource path got the same soft
RESOURCE_NOT_FOUND on every miss, so it could retry until the invocation
ended. Count misses per invocation in temp: state and return
RESOURCE_NOT_FOUND_FATAL from the second miss onward, matching
adk-python. The six existing error-code literals move into an exported
LoadSkillResourceErrorCode enum; the emitted strings are unchanged.
Adds four cases: escalation on the second miss, escalation on a
different path, a running count across the references/, assets/ and
scripts/ prefixes, and a per-invocation reset that also pins the temp:
counter key shape.
Runs the guard through InMemoryRunner, LlmAgent and
InMemorySessionService with a scripted model. Proves the escalation and
the per-invocation reset over real invocation ids, and that the counter
key never reaches the stored session state.
The stripper that keeps this counter out of durable storage matches on
State.TEMP_PREFIX, so the key must be built from the same constant
rather than a literal that can drift.
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.

1 participant