google-gemini / google-gemini/gemini-cli

formatTruncatedToolOutput: negative maxChars silently inflates output ~2x

Open Beginner friendly
#28,620 2 comments 0 reactions 0 assignees View on GitHub
area/core effort/small kind/bug priority/p1 status/bot-triaged
Dominant language
TypeScript
Stars
107k
Forks
14.6k
Avg merge
2d 3h
Merged PRs (30d)
45

Description

Title: `formatTruncatedToolOutput` returns ~2x larger output when `maxChars` is non-positive (missing guard)

---

## Description

`formatTruncatedToolOutput` in `packages/core/src/utils/fileUtils.ts` (line 661) has no `maxChars > 0` guard. When it's called with a non-positive `maxChars`, the function does not return the content unchanged or an obviously truncated prefix — it returns a string that is roughly **twice** the size of the input.

The function is part of the public surface of `@google/gemini-cli-core`: `src/index.ts:91` does `export * from './utils/fileUtils.js'`, so any tool, extension, or MCP server that imports the package can call it directly. The relevant code:

```ts
export function formatTruncatedToolOutput(
contentStr: string,
outputFile: string,
maxChars: number,
): string {
if (contentStr.length <= maxChars) return contentStr; // false for any non-empty input when maxChars <= 0

const headChars = Math.floor(maxChars * 0.2); // negative when maxChars < 0
const tailChars = maxChars - headChars; // can be 0

const head = contentStr.slice(0, headChars); // negative end index -> offset-from-end -> ~whole string
const tail = contentStr.slice(-tailChars); // slice(-0) === slice(0) -> WHOLE string
const omittedChars = contentStr.length - headChars - tailChars;

return `Output too large. Showing first ${headChars.toLocaleString()} and last ${tailChars.toLocaleString()} characters. For full output see: ${outputFile}\n${head}\n\n... [${omittedChars.toLocaleString()} characters omitted] ...\n\n${tail}`;
}
```

Why it inflates instead of truncating:

1. The early-return `contentStr.length <= maxChars` is `false` for any non-empty content when `maxChars <= 0`, so execution falls through.
2. `headChars = Math.floor(maxChars * 0.2)` goes negative, and `contentStr.slice(0, headChars)` treats a negative end index as an offset from the end — so it returns almost the entire string.
3. `tailChars` can evaluate to `0`, and `contentStr.slice(-0)` is identical to `contentStr.slice(0)`, which returns the **whole** string.
4. The two are then concatenated inside the "Output too large" wrapper, so the result is roughly `head (~whole) + tail (whole)` ≈ 2x the input.

Head's-up on reachability: the two in-tree call sites in `packages/core/src/scheduler/tool-executor.ts` (around lines 220 and 259) both guard the call with `if (threshold > 0 && ...)`, using the value from `getTruncateToolOutputThreshold()`. So stock shell-tool / MCP-tool output is **not** affected today. What's missing is the invariant inside the exported function itself. Two consequences worth calling out separately:

- **(a) `maxChars = 0` as "disable truncation"** does not work. Today `0` does not short-circuit; it wraps the full content in a misleading "Output too large ... characters omitted" header instead of passing the content through. A caller that passes `0` intending "no limit" gets corrupted output.
- **(b) a dynamically computed budget going negative** (e.g. a "remaining context window" calculation, or any caller that doesn't pre-clamp the threshold) lands in the ~2x inflation path silently. The function never errors — it just returns something larger than what it was handed.

## To Reproduce

This drives the real exported function. It adds a throwaway vitest test under `packages/core`, so it imports the actual source rather than a copy.

```bash
git clone https://github.com/google-gemini/gemini-cli.git
cd gemini-cli
npm ci

cat > packages/core/src/utils/repro_maxchars.test.ts <<'EOF'
import { describe, it, expect } from 'vitest';
import { formatTruncatedToolOutput } from './fileUtils.js';

describe('formatTruncatedToolOutput / non-positive maxChars', () => {
it('negative maxChars: output is larger than input (synthetic data)', () => {
const input = 'A'.repeat(50000); // synthetic
const out = formatTruncatedToolOutput(input, '/tmp/out.txt', -1000);
// eslint-disable-next-line no-console
console.log(`maxChars=-1000 input=${input.length} output=${out.length}`);
expect(out.length).toBeGreaterThan(input.length); // bug: should be <= input
});

it('maxChars=0: should disable truncation and return content unchanged (synthetic data)', () => {
const input = 'A'.repeat(50000); // synthetic
const out = formatTruncatedToolOutput(input, '/tmp/out.txt', 0);
// eslint-disable-next-line no-console
console.log(`maxChars=0 input=${input.length} output=${out.length}`);
expect(out).toBe(input); // bug: currently wraps full content in a truncation header
});
});
EOF

npx vitest run --root packages/core src/utils/repro_maxchars.test.ts
```

Both assertions fail on current `master`. Numbers I see (synthetic 50,000-char input):

| `maxChars` | input chars | output chars | ratio |
|-----------:|------------:|-------------:|------:|
| `-1000` | 50,000 | 99,136 | 1.98x |
| `-1` | 50,000 | 100,130 | 2.00x |
| `0` | 50,000 | 50,130 | whole content wrapped in "Output too large" header |

## Expected

- For `maxChars <= 0`, treat truncation as disabled and return `contentStr` unchanged. This matches the convention the in-tree callers already rely on (they only truncate when `threshold > 0`). A one-line guard at the top is enough:

```ts
if (maxChars <= 0 || contentStr.length <= maxChars) return contentStr;
```

- If `0` is meant to carry a different meaning than "disable", that contract should be documented at the function. Right now neither `0` nor any negative value does anything sensible.

## Actual

- `maxChars = -1000` with a 50,000-char synthetic input returns **99,136** chars — nearly 2x the input — instead of a shorter (or unchanged) string.
- `maxChars = 0` returns the full content embedded in an "Output too large ... characters omitted" wrapper (50,130 chars), so a caller passing `0` to disable truncation does not get the content back unchanged.
- No error is thrown in either case; the inflation is silent.

## Anything else we need to know?

- Reproduced against `master` at `packages/core/src/utils/fileUtils.ts:661`; the function is re-exported from `packages/core/src/index.ts:91`.
- This is a library-level repro (direct call against the exported function), not a CLI-session issue, so I haven't attached `/about` output. Happy to add it if it's useful.
- Suggested fix is the one-liner above; happy to send a PR with a test covering `maxChars <= 0` if that's welcome.

Contributor guide

Open the contributing guide

Research direction

Start in packages/core/src/utils/fileUtils.ts at formatTruncatedToolOutput, then check its export through packages/core/src/index.ts and the guarded call sites in packages/core/src/scheduler/tool-executor.ts. Add regression coverage for non-positive maxChars and run the packages/core Vitest command; done means non-positive limits no longer inflate or wrap the content unexpectedly.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
cli, tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.