Skip to content

test(e2e): stage the WSL inline-image payload in a file - #29633

Merged
nwparker merged 2 commits into
mainfrom
fix/wsl-inline-images-short-command
Oct 16, 2026
Merged

nwparker merged 2 commits into
mainfrom
fix/wsl-inline-images-short-command

Conversation

@nwparker

@nwparker nwparker commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​15 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​12
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 0

ELI5

The "real WSL terminal" CI check sometimes failed on PRs that have nothing to do with terminals. In those runs the blue test image was missing. The images were fine. The test typed one very long command into the Linux shell before the shell was ready to read it, and Linux silently threw away most of that line. Now the test saves the image data to a file and types a short command that prints the file.

What Changed

  • Before: the WSL part of terminal-inline-images-windows.spec.ts typed a single 24,495-character command (printf '<base64 payload>' | base64 -d). It did this the moment the terminal got its PTY, before WSL bash was showing its prompt. About 1 in 15 WSL runs, the printed output was missing part of the Kitty image data. Then either the blue image never appeared ("All three protocol images must appear in the rendered screenshot") or the rest of the output disappeared ("Terminal did not contain WINDOWS_INLINE_IMAGES_DONE").
  • After: the test writes the exact same output bytes to a temporary file in the test repo. The terminal's working directory is that repo. The test then types cat <file>; rm -f <file>; printf 'WSL_IMAGE_PROTOCOL=…', which is under 150 characters. The terminal receives byte-for-byte the same image output as before, so the rendering assertions are unchanged. The native Windows (PowerShell) variant is unchanged.
  • Mechanism: until bash's line editor starts, the Linux terminal is in "cooked" (line-buffered) mode. The kernel holds at most 4095 bytes of an unfinished line and drops the rest. Whether bash's line editor starts before the long input arrives is a timing race, which is why the failure came and went.
  • CI routing: this spec only runs in the real WSL lane, but changing it did not trigger that lane. config/scripts/pr-e2e-source-routing.mjs now routes the spec to the lane, and config/scripts/wsl-e2e-lane-contract.test.mjs checks that.

Why

Evidence I gathered:

Alternatives I rejected:

  • Waiting for the prompt before typing. This still races: bash returns to cooked mode between commands, so any later long command has the same hazard.
  • Retrying or skipping. That hides the cause.

The product is not at fault. Real users pasting a long line into a shell that hasn't started yet hit the same kernel limit in any terminal.

Linked Issue

N/A (internal CI flake; seen on #29597 and #29610)

Visual Proof

N/A: this is a test-only change. Failing screenshots (blue image missing) are in the windows-wsl-terminal-traces artifacts of runs 38884043896 and 38887618271.

Testing

  • I manually tested these changes locally: I validated them on the Windows WSL runner with windows-wsl-e2e.yml dispatch runs, because WSL isn't available on macOS.
  • Automated tests added/updated, or explained why not below: the spec itself is the test. I added a route case to wsl-e2e-lane-contract.test.mjs.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Test and CI routing only, with no product code.
  • Only the WSL variant changes. Native Windows, macOS and Linux are unaffected.
  • SSH, mobile and backwards compatibility are not affected.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 090f853d-654c-490e-ae9b-b10bd8699689
📥 Commits

Reviewing files that changed from the base of the PR and between e7d7b1a2316c15e2e916ccd0e0d7bd3ea250d5f9 and bd6ea00.

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
📝 Walkthrough

The WSL E2E route now matches the Windows inline-images spec, and its routing contract test includes that spec. During WSL runs, the test writes the raw image payload and marker to a unique repository file, then cats and removes the file before reporting the protocol. Native Windows continues to send the base64-encoded payload through the Node terminal command.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to ddd3a

This changes the Windows inline-images E2E path, not product runtime behavior. The leftover file’s concrete effect on later tests is unestablished, leaving minimal identified merge risk.

Pre-merge checks: Passed 5
Check name Status Explanation
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the main change: staging the WSL inline-image payload in a file to avoid long-command input loss.
Description check Passed The description is complete and aligned with the template. It explains the failure, mechanism, fix, testing, routing changes, scope, and visual-proof rationale. The full validation command checklist r…
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files.

Full details: Description check

Explanation

The description is complete and aligned with the template. It explains the failure, mechanism, fix, testing, routing changes, scope, and visual-proof rationale. The full validation command checklist remains unchecked, but this is non-critical because the description states that CI will provide coverage.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments: 1
tests/e2e/terminal-inline-images-windows.spec.ts: 1

81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clean up the payload file if the test fails before rm -f runs.

The test writes payloadFile into testRepoPath and relies on the typed rm -f command to delete it. If execInTerminal is never reached or fails, the file stays in the repository. The fixture documents testRepoPath as a disposable repository per worker. This limits the impact, but the file can affect later tests in the same worker. A try/finally with rmSync(..., { force: true }) removes this risk.

Proposed fix
+import { rmSync, writeFileSync } from 'node:fs'
-import { writeFileSync } from 'node:fs'

Then wrap the steps from execInTerminal to the end of the test in try { ... } finally { rmSync(path.join(testRepoPath, payloadFile), { force: true }) }.

Source: Learnings


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 09761014-3dc2-4c4f-a503-ef39cd6994c2
📥 Commits

Reviewing files that changed from the base of the PR and between 2d2bccf and ddd3aed067eeb96b980a372b9fc8878a4a908fad.

📒 Files selected for processing: 3
  • config/scripts/pr-e2e-source-routing.mjs
  • config/scripts/wsl-e2e-lane-contract.test.mjs
  • tests/e2e/terminal-inline-images-windows.spec.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@nwparker
nwparker force-pushed the fix/wsl-inline-images-short-command branch from ddd3aed to e7d7b1a Compare October 16, 2026 11:44
@nwparker
nwparker force-pushed the fix/wsl-inline-images-short-command branch from e7d7b1a to bd6ea00 Compare October 16, 2026 11:55
@nwparker
nwparker merged commit d3cafe2 into main Oct 16, 2026
65 of 66 checks passed
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