Skip to content

fix(sse): preserve tool output in safe compression plans - #18818

Merged
diegosouzapw merged 5 commits into
cheaper-inference:release/v3.8.53from
stanleytejakusuma:fix/safe-tool-output-preservation
Nov 4, 2026
Merged

diegosouzapw merged 5 commits into
cheaper-inference:release/v3.8.53from
stanleytejakusuma:fix/safe-tool-output-preservation

Conversation

@stanleytejakusuma

@stanleytejakusuma stanleytejakusuma commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Bug

Default compression could silently clip historical tool results to about 2 KB even when a request had not opted into lossy compression. Disabling tool-result clipping alone was insufficient: Lite still removed trailing spaces and folded blank lines. The Responses WebSocket and offline eval callers also discarded the selected request-scoped pipeline and could execute the stored pipeline instead.

Fix

  • Give safe/default Lite steps request-local settings that disable clipping and preserve unique tool-result whitespace, including RTK fallback, standalone Lite and retained stacked Lite.
  • Pass the full selected plan through the Responses WebSocket and eval callers. Do not mutate shared or stored settings.
  • Retain existing explicit lossy opt-in behavior, including whitespace normalization when compressToolResults is false; leave non-tool processing, caps, dependencies and session deduplication unchanged.
  • Extract the existing per-case eval operation into one private helper to meet the standalone function-length gate. A before-and-after control pins call ordering, gold grading and metering, including exclusion of judge self-test costs.

Regression evidence and fresh verification

Actual failing-then-passing tests were captured during implementation: a 10,800-byte historical result was clipped to 1,937 bytes; after the clipping fix, a whitespace-sensitive 10,800-byte result still became 10,791 bytes. The whitespace regression set failed 74 of 79 tests before its fix and passed the same 79 afterward. Coverage includes 10,800 / 16,000 / 100,000-byte unique results, Chat/Responses layouts, sync/async engines, opt-in controls and unchanged stored settings.

Fresh results after rebasing onto 493101c3f1c82091e316ac3162a8511e17f43f7c:

Check Result
All 12 touched native test files plus 3 adjacent Lite/eval files 213 passed, 0 failed
Vitest 56 files, 588 passed, 0 failed
Changed-file configured ESLint, Prettier and core typecheck Passed
Standalone complexity/length findings over the same 6 production files Base: 3; head: 2; no new length violation
Eval behavior control plus existing eval tests, before and after helper extraction 7 passed in each run

Commands used for focused and static verification:

BASE=493101c3f1c82091e316ac3162a8511e17f43f7c
mapfile -t TESTS < <(git diff --name-only "$BASE"...HEAD -- tests/)
mapfile -t CHANGED < <(git diff --name-only "$BASE"...HEAD -- '*.ts')
mapfile -t SOURCE < <(git diff --name-only "$BASE"...HEAD -- open-sse/ src/)
env -u DATA_DIR DISABLE_SQLITE_AUTO_BACKUP=true node \
  --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts \
  --import ./tests/_setup/isolateDataDir.ts \
  --test --test-force-exit --test-concurrency=4 "${TESTS[@]}" \
  tests/unit/compression/eval-runner.test.ts \
  tests/unit/compression/lite.test.ts tests/unit/compression/lite-current-turn.test.ts
./node_modules/.bin/eslint --suppressions-location config/quality/eslint-suppressions.json "${CHANGED[@]}"
./node_modules/.bin/prettier --check "${CHANGED[@]}"
npm run typecheck:core
node_modules/.bin/eslint --no-config-lookup --config eslint.complexity.config.mjs --format json "${SOURCE[@]}"

The standalone complexity command still exits 1 for two existing cyclomatic violations; the measured finding count decreases from 3 to 2. The same config was also run against each base blob via --stdin --stdin-filename to compare identical paths without changing the checkout. Vitest ran with a separate temporary DATA_DIR and plugin directory, with SQLite automatic backups disabled.

Normal commit hooks ran without bypass or stashing. The private-reference UI-kit snapshot gate self-skipped in this fork environment. At initial draft publication, the full native unit suite, production build and coverage gate had not been rerun locally. Ready-for-review CI has since passed on 383217d4bd179f9cc676be3b2099a7f1c6f5cece (2026-11-03): all 8 unit-test shards, Vitest, Build, Coverage, Quality Ratchet, Lint, Security Tests and both aggregate gates.

The earlier draft-triggered aggregate failures are superseded by those successful runs. Unselected package-artifact and live E2E jobs remain skipped, not certified passes.

Known base status

⚠️ base-red inherited: #18791

The base tracker reports a package-artifact gate timeout and a skipped tarball boot-smoke. This PR does not attempt to fix those base failures.

@stanleytejakusuma
stanleytejakusuma marked this pull request as ready for review November 3, 2026 05:24
# Conflicts:
#	open-sse/services/compression/lossyRequestPolicy.ts
#	tests/unit/chatcore-compression-combo-predicates.test.ts
#	tests/unit/compression/active-combo-dispatch.test.ts
#	tests/unit/compression/active-combo-integration.test.ts
#	tests/unit/compression/compression-header-dispatch.test.ts
#	tests/unit/compression/derive-effective-preview-plan.test.ts
#	tests/unit/compression/derived-pipeline-integration.test.ts
#	tests/unit/compression/lossy-request-policy.test.ts
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

2 participants