Feat: cap stdout/stderr size in the skill script tool responses - #518
Open
AmaadMartin wants to merge 4 commits into
Open
Feat: cap stdout/stderr size in the skill script tool responses#518AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 3 commits
August 2, 2026 04:34
Add a shared truncateMiddle helper and apply it to the stdout and stderr fields of RunSkillScriptTool and RunSkillInlineScriptTool, so a script that emits a large log cannot put its whole output into the next prompt. The cap is configurable per toolset via the new SkillToolset maxOutputChars option and defaults to 30,000 characters per stream.
Add unit coverage for the truncateMiddle helper (boundaries, odd/even split, removed-character count, zero and negative limits, default cap), for both skill script tools (stdout, stderr, independent caps, custom maxOutputChars, no mutation of the executor result), for the new SkillToolset option, and an end-to-end case that runs a real noisy script through UnsafeLocalCodeExecutor and asserts the head, marker and tail.
docs:check runs typedoc with --treatWarningsAsErrors, and a {@link} to
DEFAULT_MAX_OUTPUT_CHARS warns because truncate_utils stays internal and
is not part of the documented public API. Name the constant and its value
inline instead of linking to it.
Address the complexity review: - RunSkillInlineScriptTool read the cap as 'this.toolset?.maxOutputChars ?? DEFAULT_MAX_OUTPUT_CHARS', but toolset is non-optional and maxOutputChars is always assigned, so neither guard could fire. Collapse it to the sibling tool's spelling. - truncateMiddle's 'limit' default was taken by no caller, only by a test of the default itself. Make the parameter required. - With no default left in the helper, the 30,000 cap is skill-toolset policy rather than a property of string truncation, so move the constant beside the option it defaults and leave truncate_utils a generic helper the queued follow-ups can reuse.
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
N/A — no public issue tracks this.
Problem:
RunSkillScriptToolandRunSkillInlineScriptToolhand the code executor'sstdoutandstderrstraight back to the model with no size limit (core/src/tools/skill/run_skill_script_tool.ts,core/src/tools/skill/run_skill_inline_script_tool.ts, both previously ending theirtryblock withreturn result;). A skill script that dumps a build log, a large JSON blob, or a looping stack trace puts its entire output into the prompt on the very next model turn. For a long-running agent that is an unbounded context-window and cost hazard, and it breaks the "bound every accumulator" rule that applies to anything growing per invocation.Solution: Add a small shared helper,
core/src/utils/truncate_utils.ts, and apply it to thestdoutandstderrfields of both tools' success responses.DEFAULT_MAX_OUTPUT_CHARS = 30_000, applied per stream (stdoutandstderrare capped independently, so a largestdoutnever eats into thestderrbudget). The constant lives inskill_toolset.tsbeside the option it defaults, because the cap is skill-toolset policy rather than a property of string truncation;truncate_utils.tsstays a generic, import-free helper that the queued follow-ups can reuse without inheriting a constant named after skill script output.truncateMiddle(text, limit)(both parameters required — no call site omits the limit) returnstextunchanged when it fits, otherwise keepsceil(limit/2)characters of head andlimit - ceil(limit/2)of tail, replacing the elided middle with\n... [truncated <N> characters] ...\nwhere<N>is the number of characters removed. The preserved-content budget is therefore exactlylimit; the returned string is marginally longer because it also carries the marker.new SkillToolset(skills, {codeExecutor, maxOutputChars: 4_000}). The option is surfaced aspublic readonly maxOutputCharsand read by both tools in this same change, so it is not dead config. The model-facingFunctionDeclarations are unchanged — the cap is an operator control, and a model-settable cap would be a model-controlled way to defeat its own context budget.Design notes / where this deviates from
adk-python(deliberate, per the cross-language-parity rule that local convention wins for things that do not cross a wire boundary):adk-python'ssrc/google/adk/tools/environment/_constants.py(MAX_OUTPUT_CHARS = 30_000), and the per-stream application matchesExecuteTool.run_asyncinsrc/google/adk/tools/environment/_execute_tool.py, which truncates each stream separately.adk-python'struncate()(src/google/adk/tools/environment/_utils.py) keeps only the head. The hazard here is specifically a looping stack trace or a failing test suite, where the diagnostically useful content — the failure summary, the innermost frames — is at the end of the stream. Head-only truncation throws exactly that away, so this keeps both ends.adk-pythonskill-tool behaviour:adk-python'ssrc/google/adk/tools/skill_toolset.pyreturns scriptstdout/stderrverbatim too. This borrows the environment toolset's mechanism for the JS skill tools.CodeExecutionResult(object spread) rather than the same object withoutputFilesassigned in place. No caller in the repo depends on that identity — these tworeturnstatements are the only consumers — and a test pins that the executor's own result is no longer mutated.Helper placement:
truncateMiddleis generic string handling, not skill-specific, so it lives incore/src/utils/with a feature-neutral name (truncate_utils.ts), not inlined at the top of either tool file and not prefixed with the feature that happened to need it first. Following thecore/src/utils/file_utils.tsprecedent it stays internal — it is not added tocore/src/common.tsorcore/src/index.ts, and its tests import it by relative path, exactly ascore/test/utils/file_utils_test.tsdoes formaterializeFiles.Collision check (required before implementation):
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 418 open PRs; none truncates skill script output (gh pr diffover every plausibly adjacent PR found notruncat/maxOutputCharshit). Several PRs are textually adjacent — #517 (save output files as artifacts), #410 (output files to the artifact service), #437/#353/#298 (explicit script output directory) all touch the samerunAsynctails and/orSkillToolsetoptions — but each does a different thing, and they conflict with one another as much as with this PR, so stacking on any one of them would be arbitrary. This branch is based on currentmainand its diff is confined to thestdout/stderrfields;outputFileshandling is left byte-for-byte equivalent tomainso those PRs stay mergeable.Deliberately out of scope (not silently dropped): saving the full log to the artifact service (needs an
artifactService-undefined guard, graceful degradation, a collision-resistant filename scheme, and its own error surface — and would collide head-on with #517/#410); capping the base64contentofoutputFiles; cappingLoadSkillResourceTool'scontentfield. The last two have been queued as independent follow-ups that reuse this helper.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.
No existing test was modified, skipped, weakened or deleted; every addition is a new
itor a newdescribeblock.New coverage:
core/test/utils/truncate_utils_test.ts(new file, 9 cases): below/at/one-over the limit, even-limit split asserted as exact head and tail slices, odd-limit split (limit = 7-> 4 head + 3 tail), exact removed-character count on a hand-written input,limit === 0(marker only), negative limit clamped to 0, and the default (DEFAULT_MAX_OUTPUT_CHARS === 30_000, 30,000 chars unchanged / 30,001 truncated).run_skill_script_tool_test.ts: newdescribe('output truncation')— stdout under the cap verbatim, stdout over the cap, stderr over the cap while a short stdout stays verbatim, both streams capped independently in one call, a custommaxOutputChars, and the executor's result not being mutated. Plus one new case for the pre-existingEXECUTION_ERRORpath. This one is a deliberate drive-by, not load bearing for the cap: it exercises thecatchblock, which this diff does not touch, and it exists becauseMockCodeExecutor.shouldThrowwas declared in this file but never used (the inline sibling already had the equivalent case). It is 17 lines of pure addition covering the error path adjacent to thetryblock being restructured; say the word and I will split it out.run_skill_inline_script_tool_test.ts: the equivalentdescribe('output truncation'), driven through the existing confirmed-ToolConfirmationhelper so the calls pass the security gate.skill_toolset_test.ts:maxOutputCharsdefaults toDEFAULT_MAX_OUTPUT_CHARS, and reflects an explicitly passed option.Measured coverage (
--coverage.includelimited to the changed files):core/src/utils/truncate_utils.tsis 100% lines / 100% branches / 100% functions. Every line added to the three modified files is covered; the residual uncovered lines in those files are pre-existing (_getDeclarationbodies, unrelatedSkillToolsetpaths) and untouched here.Proof the tests can fail. Coverage is a floor, not proof, so each new test was run against mutated source and confirmed to fail (all mutations reverted afterwards; the working tree is clean):
truncateMiddlebody ->return text;expected 'abcdef' to be 'abc\n... [truncated 1 characters] ...\nef'Math.ceil(cap / 2)->Math.floor(cap / 2)expected 'abc\n... [truncated 93 characters] ...' to be 'abcd\n... [truncated 93 characters] ...'text.length - cap->text.lengthexpected 'abc\n... [truncated 6 characters] ...' to be 'abc\n... [truncated 1 characters] ...'<=-><expected 'abc\n... [truncated 0 characters] ...\nde' to be 'abcde'Math.max(0, limit)->limitexpected '\n... [truncated 106 characters] ...\n' to be '\n... [truncated 6 characters] ...\n'stderr:line fromrun_skill_script_toolexpected 'eee…' to contain '... [truncated 10 characters] ...'stderr:line fromrun_skill_inline_script_toolexpected 'eee…' to contain '... [truncated 7 characters] ...'SkillToolsetignores the option (always the default)expected '0123456789' to be '01\n... [truncated 6 characters] ...\n89'expected 'yyyyy\n... [truncated 40 characters] ...' to be 'yyyyy…' (50 chars)stdout/stderrthrough untruncated (integration)expected 'SSS…' to contain '... [truncated 110 characters] ...'Complexity review round 1 (three findings, all addressed in
301569f0, no behaviour change — the same 53 unit + 19 integration tests pass unchanged): the inline tool read the cap asthis.toolset?.maxOutputChars ?? DEFAULT_MAX_OUTPUT_CHARS, buttoolsetis non-optional andmaxOutputCharsis always assigned, so both guards were unreachable — collapsed to the sibling'sthis.toolset.maxOutputChars;truncateMiddle'slimitdefault was taken by no caller and existed only for the test that asserted it — made required, and that test became an explicit-limit assertion that the result is exactlylimitcharacters of content plus the marker; and with no default left in the helper,DEFAULT_MAX_OUTPUT_CHARSmoved toskill_toolset.ts. It remains internal:core/src/common.tsre-exportsSkillToolsetby name (export {SkillToolset} from ...), notexport *, so the constant is not published. A new mutation was added for the move — changing the constant to10_000failsskill_toolset > maxOutputChars > defaults to DEFAULT_MAX_OUTPUT_CHARS when the option is omitted— and every mutation in the table above was re-run against the refactored source and still fails.CI on the first pushed commit failed
docs:check(typedoc --emit none --treatWarningsAsErrors) because a{@link DEFAULT_MAX_OUTPUT_CHARS}in theSkillToolsetoption doc resolved to a symbol that is not part of the documented public API. Fixed by naming the constant and its value inline rather than exporting the internal module just to satisfy a doc link.run-testsis now green on ubuntu-latest, macos-latest and windows-latest.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
An automated no-mock end-to-end case was added to
tests/integration/tools/run_skill_script_tool_test.ts(caps stdout and stderr of a real noisy JavaScript skill script): it runs a real script underUnsafeLocalCodeExecutorthat writes 610 characters to stdout and 710 to stderr withmaxOutputChars: 500, and asserts the exact head slice, the exact marker (... [truncated 110 characters] .../... [truncated 210 characters] ...) and the tail sentinelsSTDOUT_END/STDERR_END. This is a deliberate, small departure from "no new integration tests": a 500-character cap needs only ~600 characters of real subprocess output, so the case costs ~150 ms rather than the minutes a 30 KB flood would, and it is what proves the option is wired through to a real executor end to end. Mutation 10 above confirms it fails without the fix.To reproduce by hand:
With a live model: configure an
LlmAgentwith aSkillToolsetwhose skill has a script printing well over 30,000 characters, ask the agent to run it, and confirm the tool response carries the head, the marker with a plausible removed-character count, and the tail — and that the model's follow-up references the truncation rather than reasoning as though it saw the whole log. Repeat withmaxOutputChars: 500to confirm the option is honoured end to end.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.