Skip to content

Feat: warn when a skill's resource payload exceeds the recommended size limit (adk-python parity) - #453

Open
AmaadMartin wants to merge 2 commits into
mainfrom
feat/skill-payload-size-warning
Open

Feat: warn when a skill's resource payload exceeds the recommended size limit (adk-python parity)#453
AmaadMartin wants to merge 2 commits into
mainfrom
feat/skill-payload-size-warning

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 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):

No existing issue.

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

Problem: getSkillResourceFiles() ships a skill's whole references/, assets/ and scripts/ payload into a code executor on every script call. The TypeScript SDK measures none of it, so an oversized skill fails only later, inside the executor, with an opaque message. adk-python accounts for the same payload and warns above a 16 MB soft limit.

Solution: Sum the byte length of every resource that is actually shipped, and log one warning when the total exceeds MAX_SKILL_PAYLOAD_BYTES. The limit stays soft, as in Python: nothing is rejected, truncated or thrown, and the returned File[] is unchanged. The accumulator runs after the existing fileContent === undefined guard, so a skipped resource contributes 0 bytes.

Deliberate deviations from the reference:

  1. The count uses Buffer.byteLength, not String.prototype.length. Python's len() over a str counts code points, so the reference undercounts multi-byte text by up to 3x. Parity here is the behaviour, and a byte count is the correct implementation of it. No adk-python change is made.
  2. Python names the constant _MAX_SKILL_PAYLOAD_BYTES. This repo forbids the leading underscore, so it is MAX_SKILL_PAYLOAD_BYTES. The value and the message wording are identical.
  3. Buffer.byteLength accepts a string and a Buffer and returns the byte length of each, so one call replaces a Buffer.isBuffer ternary.

The constant is not added to index.ts or common.ts. This is not new public API.

Testing Plan

Unit Tests:

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

Six cases in a new describe('payload size accounting', ...) in core/test/tools/skills/run_skill_script_tool_test.ts. No existing test is modified, removed or weakened.

npx vitest run --project unit:core core/test/tools/skills/run_skill_script_tool_test.ts
# Test Files 1 passed (1) / Tests 14 passed (14)

Coverage of the new code is 100% of lines and branches, read from coverage-final.json. The file's remaining uncovered lines (200-201, 216-217) predate this change. No coverage suppression was added.

Mutation proofs. I mutated one source line per row, re-ran the targeted suite, then restored the source:

Mutation Test that fails Failure message
Buffer.byteLength(fileContent) -> fileContent.length measures multi-byte text in UTF-8 bytes, not UTF-16 code units expected "warn" to be called 1 times, but got 0 times
totalBytes > -> totalBytes >= does not warn when skill resources are exactly at the limit expected "warn" to not be called at all, but actually been called 1 times
delete the logger.warn call the four warning cases expected "warn" to be called 1 times, but got 0 times
totalBytes += -> totalBytes = sums bytes across resource types expected "warn" to be called 1 times, but got 0 times

The first mutation is why the multi-byte case exists. Every single-byte test passes against a String.length count, so those tests give no signal on the defect this feature guards against. The fixture is 8,388,609 'é' characters: under the limit by String.length, over it by UTF-8 byte length.

This change adds no error path. getSkillResourceFiles does not throw and has no errorCode. The negative path is "no warning", pinned by the under-limit and at-limit cases.

Manual End-to-End (E2E) Tests:

No E2E test is included. The change alters neither the executor payload nor any tool response, so there is no cross-component contract to exercise. To check the log by hand, set the ADK log level to warn and run a skill script whose resources exceed 16 MiB. The warning renders once:

WARN: [ADK] Skill 'research-helper' resources total 17825834 bytes, exceeding the recommended limit of 16777216 bytes.

Repo gates on the pushed commit: npm run build passes, npx eslint on both touched files is clean, and npx prettier --check reports All matched files use Prettier code style!. npm run ts:check is already red on main (280 errors). This diff adds zero new errors: the count is 280 before and after, and the one error in the touched test file is the pre-existing as File cast in a test I did not change.

Collision check. I listed all 697 fork PRs. Ten open PRs touch run_skill_script_tool.ts (#634, #564, #556, #518, #517, #481, #410, #356, #353, #298), but none adds payload accounting: grep -icE 'byteLength|PAYLOAD_BYTES|payload size' over each diff returns 0. The overlap is textual only, in runAsync, so this branches from main rather than stacking.

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.

Amaad Martin added 2 commits August 5, 2026 23:02
A skill's references/, assets/ and scripts/ are shipped wholesale into a
code executor by getSkillResourceFiles(). The Python SDK size-accounts
that payload and warns above 16 MB; the JS SDK did not account for it at
all, so a developer whose skill grew to hundreds of megabytes of assets
got no signal until the executor itself failed or silently degraded far
downstream.

Accumulate the raw byte length of every resource that is actually shipped
and log a single warning when the total exceeds MAX_SKILL_PAYLOAD_BYTES.
The limit stays soft, matching the reference implementation: nothing is
rejected, truncated or thrown, and the returned File[] is unchanged.
The four existing payload tests all use single-byte content, so they pass
unchanged when the accumulator measures String.length instead of
Buffer.byteLength. That is the exact defect this feature exists to avoid:
a skill of multi-byte text is undercounted by up to 3x and never warns.

Add a reference of 8,388,609 'é' characters. It is under the limit by
String.length and over it by UTF-8 byte length, so it fails against a
code-unit count. Add a second case that splits the payload across
references and assets, so no single resource exceeds the limit but the
total does.
@AmaadMartin
AmaadMartin force-pushed the feat/skill-payload-size-warning branch from ff66d94 to f775659 Compare August 6, 2026 06:09
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