Feat: add RunSkillScriptErrorCode enum for RunSkillScriptTool - #356
Open
AmaadMartin wants to merge 2 commits into
Open
Feat: add RunSkillScriptErrorCode enum for RunSkillScriptTool#356AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
July 30, 2026 20:02
Replace the seven bare errorCode string literals in RunSkillScriptTool with members of a new exported RunSkillScriptErrorCode enum, mirroring the sibling RunSkillInlineScriptErrorCode. String values are unchanged, so the tool's response contract is byte-identical on the wire. Export the enum from core/src/index.ts so consumers can branch on the failure payload with compiler support instead of magic strings.
…OR path Add an 'error codes' contract test asserting each enum member's string value, mirroring the inline sibling's block. Also cover the executor-throws branch, which had no test at all: the MockCodeExecutor.shouldThrow flag existed but was never set, so the EXECUTION_ERROR return site was unpinned. Existing assertions are left comparing against raw strings, which is what proves the wire values did not move.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue.
Problem:
RunSkillScriptToolandRunSkillInlineScriptToolare sibling tools inSkillToolsetthat return the same shaped failure payload ({error, errorCode}). The inline tool already models itserrorCodevalues as an exported string enum (RunSkillInlineScriptErrorCode,core/src/tools/skill/run_skill_inline_script_tool.ts:21), documented as part of the tool's response contract and re-exported fromcore/src/index.ts. The non-inline tool still scatters bare string literals across its seven failure return sites, so a consumer branching on the payload has to hardcode a magic string with no compiler help and no discoverability in the API reference —result.errorCode === 'SCRIPT_NOT_FUOND'compiles fine and is silently never true.Solution: Declare and export
RunSkillScriptErrorCodeinrun_skill_script_tool.ts, replace all seven literals with its members, and export it fromcore/src/index.ts. This follows the repo guideline "Error Codes Are Enums: declare an enum and use its members; never scatter bare string literals at throw sites."The sibling enum is the sole reference for the shape (JSDoc with a
{@link}back-reference plus an explicit stability statement,export enum,SCREAMING_SNAKE_CASEmembers each explicitly initialised to its own name, declared after the imports and above the@experimentalclass). This is deliberately not an adk-python parity change:adk-python'sskill_toolset.pyuses a different payload key (error_code) and a different code vocabulary (INVALID_ARGUMENTS,TOOL_ERROR), and aligning with it would move wire values.Scope and non-goals:
result.errorCode === 'SKILL_NOT_FOUND'keeps working. Purely additive: one new export, no removals, no renames, no signature changes.NO_CODE_EXECUTOR,EXECUTION_ERROR); a shared enum would expose members invalid for one tool or the other.runAsync's return type staysPromise<unknown>— narrowing it to a discriminated union is a real API-surface change and is out of scope here.core/src/common.ts, matching the inline sibling's placement.Collision check (required by the pipeline, recorded here):
gh pr list --repo AmaadMartin/adk-js --state open --limit 300returned 260 open PRs; none adds aRunSkillScriptErrorCodeenum or any error-code enum for this tool. Two PRs touch the same source file — #353 (fix/skill-script-explicit-output-dir) and #298 (fix/skill-script-output-dir), which appear to be competing implementations of each other. Both are confined to thematerializeFilesoutput-directory call at the bottom ofrunAsyncand the import at the top;gh pr diff 353 --repo ... | grep -iE 'ErrorCode'returns nothing for either. Since their hunks are disjoint from the error-code sites and neither lands this change, this PR branches frommainrather than stacking on a PR that may not merge.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.
Two tests added to
core/test/tools/skills/run_skill_script_tool_test.ts, both pure additions — no existing test was edited, and the existingToolErrorResponse.errorCodestays typedstringwith every legacytoEqualstill comparing against a raw literal. That is deliberate: those untouched assertions are the independent evidence that the wire values did not move, and retyping them to the enum would have deleted exactly that evidence.describe('error codes')→exposes stable string values for the error-code enum, asserting all seven members, mirroringrun_skill_inline_script_tool_test.ts:385-404.returns error if the code executor throws, covering theEXECUTION_ERRORreturn site. This closed a real gap rather than padding coverage:MockCodeExecutor.shouldThrowalready existed in the file but was never set totrue, so thecatchbranch — one of the seven lines this PR rewrites — had no test at all. EveryEXECUTION_ERRORvalue could have been mistyped and the suite would still have been green.Proof the tests can fail (mutation testing, both mutations reverted afterwards):
SCRIPT_NOT_FOUND = 'SCRIPT_NOT_FOUNDX'AssertionError: expected 'SCRIPT_NOT_FOUNDX' to be 'SCRIPT_NOT_FOUND' // Object.is equality. Pre-existing untouched testreturns error if script not found in skill:AssertionError: expected { …(2) } to deeply equal { …(2) }.EXECUTION_ERROR = 'EXECUTION_ERRORX'expected 'EXECUTION_ERRORX' to be 'EXECUTION_ERROR'. New executor-throws test:expected { …(2) } to deeply equal { …(2) }.Each mutation fails both the new contract assertion and a payload assertion, which jointly prove the enum member is pinned and that the return site genuinely references the enum rather than a leftover literal. After reverting: 10/10 passing.
Commands run (targeted only, never the full suite):
The
skill_registry_test.tsandrun_skill_inline_script_tool_test.tsruns are regression checks: the former asserts rawerrorCodestrings for this tool from another file (REGISTRY_ERRORis pinned only there), the latter confirms the sibling enum was not disturbed. Both pass unmodified.npm run ts:checkfails on this branch and fails identically on the base commit — it is a pre-existing repo-wide failure (~308 errors, addressed by other open PRs). Verified rather than assumed: diffing the error sets from a stashed clean tree against this branch, with line/column numbers normalised, yields an empty diff. This PR introduces zero new type errors. The one error reported inside a file I touch is the pre-existingas Filecast at what is now line 227 (was 209 before my insertion), which is not in this diff.No type-checker or linter suppressions were added anywhere —
git diff fork/main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|: any|as never|as unknown as'returns nothing.Manual End-to-End (E2E) Tests:
Verifies the symbol is genuinely reachable from the published package entry point — this is what would break if the
index.tsexport were forgotten. The built entry points were resolved fromcore/package.json(main: ./dist/cjs/index.js,exports["."].import: ./dist/esm/index.js) rather than guessed:Both print, exactly and in declaration order:
A consumer can now write:
Postconditions confirmed:
grep -rn "errorCode: '" core/src/tools/skill/returns nothing, so the wholeskill/directory is now literal-free.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.