Skip to content

Fix: stop repeated skill-script lookup retries with SCRIPT_NOT_FOUND_FATAL - #729

Open
AmaadMartin wants to merge 1 commit into
feat/run-skill-script-error-code-enumfrom
fix/skill-script-not-found-fatal-guard
Open

Fix: stop repeated skill-script lookup retries with SCRIPT_NOT_FOUND_FATAL#729
AmaadMartin wants to merge 1 commit into
feat/run-skill-script-error-code-enumfrom
fix/skill-script-not-found-fatal-guard

Conversation

@AmaadMartin

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):

None.

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

Problem: run_skill_script returns the same SCRIPT_NOT_FOUND response on every failed script lookup. Nothing in that response tells the model to stop, so a model that guesses paths (scripts/setup.js, then scripts/setup.ts, then setup/main.js) gets an identical retry-friendly error each time and can burn a whole invocation. The Python SDK already guards against this in src/google/adk/tools/skill_toolset.py; adk-js does not.

Solution: Count script-lookup misses per invocation and escalate to SCRIPT_NOT_FOUND_FATAL from the second miss onward. The counter is keyed only by invocation id, not by skill or path, so the guard still fires when the model varies the path on each retry. The key is temp:-prefixed so session services strip it before persisting, and the invocation id suffix stops in-memory backends from carrying a count into the next invocation.

Stacked on #356. That PR introduces RunSkillScriptErrorCode and converts the existing literals. This branch adds the SCRIPT_NOT_FOUND_FATAL member and the guard on top of it, so the enum work is not duplicated. Merge #356 first. The diff below is only this change.

Collision check. I listed all 621 open PRs on the fork and read every one whose title or branch mentioned skill, script, not-found, fatal or retry. #356 (feat/run-skill-script-error-code-enum) touches the same three files and is the stack base. #634 (fix/skill-script-wrapper-path-normalization) rewrites the lines just above the if (!script) block but leaves the block body alone, so the two changes are independent. No open PR implements this guard.

Parity vs. local convention. Parity wins for the cross-boundary observables: the state key string, the SCRIPT_NOT_FOUND_FATAL value and the message text are byte-identical to Python, em dash included. Local TypeScript convention wins for the enum, the camelCase errorCode response key and the module layout.

Testing Plan

Unit Tests:

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

npx vitest run --project unit:core core/test/tools/skills/run_skill_script_tool_test.ts — 15 passed.

Five new cases in core/test/tools/skills/run_skill_script_tool_test.ts: first miss stays SCRIPT_NOT_FOUND; second miss on the same path escalates; second miss on a different path also escalates; a new invocation id resets the count; and the new enum member's string value. The existing 'returns error if script not found in skill' test is unchanged and still pins first-miss behaviour. The only edit to existing test code is createMockContext, which gained two optional trailing parameters, so all previous call sites are untouched.

New lines and branches in run_skill_script_tool.ts are at 100% coverage. Whole-file coverage reads 79.32% because this one test file does not reach _getDeclaration, the registry-error catch or getSkillResourceFiles; those gaps are pre-existing.

Mutation results — every new test was proven able to fail:

Mutation Tests that failed Failure message
Delete the counter block escalate-same-path, escalate-different-path, reset-on-new-invocation expected 'SCRIPT_NOT_FOUND' to be 'SCRIPT_NOT_FOUND_FATAL'
failCount > 1 -> failCount > 2 escalate-same-path, escalate-different-path, reset-on-new-invocation expected 'SCRIPT_NOT_FOUND' to be 'SCRIPT_NOT_FOUND_FATAL'
Drop ${toolContext.invocationId} from the key reset-on-new-invocation expected { …(2) } to deeply equal { …(2) } on the state-key assertion
Key the counter by scriptPath escalate-different-path, reset-on-new-invocation expected { …(2) } to deeply equal { …(2) }

Manual End-to-End (E2E) Tests:

No integration test. The guard is in-memory state arithmetic on an existing error path with no service, executor or network dependency, and the unit tests drive it through the tool's public runAsync.

CI on this PR is absent: validation.yaml runs on pull_request: branches: [main], and this PR targets feat/run-skill-script-error-code-enum. Validated locally on commit 14a44f65 instead:

npx vitest run --project unit:core core/test/tools/skills/run_skill_script_tool_test.ts   # 15 passed
npm run build                                                                             # exit 0
npm run lint                                                                              # exit 0
npx prettier --check <the three files>                                                    # clean
npx typedoc --emit none --treatWarningsAsErrors                                           # exit 0

Root npx tsc --noEmit reports 308 errors on this branch and 308 on the base branch, so this change adds none.

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.

Every failed script lookup returned the same retry-friendly
SCRIPT_NOT_FOUND response, so a model guessing script paths could burn
a whole invocation on hallucinated paths. Count the misses per
invocation and escalate to SCRIPT_NOT_FOUND_FATAL from the second miss
onward, matching the adk-python run_skill_script guard.

The counter key is temp:-prefixed so it never reaches durable session
storage, and carries the invocation id so a new invocation starts over.
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