8000
test(e2e): let the session's shell wait for a resize before it reports stty size by bryan-minimal · Pull Request #3178 · gominimal/minimal · GitHub
You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test(e2e): let the session's shell wait for a resize before it reports stty size - #3178
The terminal-relay proof in scripts/session-e2e.sh resizes the attached pty to 41x117 and, half a second later (the helper's fixed pause), types the line that prints stty size in the session. The resize travels client → ssh → daemon → the session's pty, and on a loaded host that relay takes longer than the pause, so the line printed the old size and the proof read as "a resize of the attached terminal did not reach the session's 'stty size'". The typed line now polls stty size for up to ten seconds until it shows the new size, then prints it. The assertion is unchanged: the size the session reports is the resized one; a resize that never arrives still fails.
Why now
The lab's pre-push lane saw this beat fail on nine runs of unrelated branches (two tonight, within two hours, with the host at a load of about 11), each time with every other e2e beat green. It is the lane's most frequent false red.
Verification
Pre-push (CI's exact commands, lab lane): green at 1cb2796. Its native-daemon-e2e step runs this beat.
Smoke: OK: e2e-resize-wait-1cb2796c: 1 expectation(s) on ubuntu-24.04: 000-baseline:pass
min-regress batch e2e-resize-wait-1cb2796c: base rel-0bb305b9 vs the candidate cand-e2e-resize-wait-1cb2796c-1daa6559, per distro:
ubuntu-24.04: 000 {'pass': 1} -> {'pass': 1}
Lab evidence (min-regress batch e2e-resize-wait-1cb2796c on the lab box, ubuntu-24.04)
Head: 1cb2796ca13c641c7aea86d283d5c9fe7f00a0ca at 1cb2796 (lab name cand-e2e-resize-wait-1cb2796c-1daa6559). Base: rel-0bb305b9 (rel-0bb305b9).
scenario
distro
base
head
000-baseline: baseline (CPU + install sanity; not a bug)
Fixed since base: 0. Still reproducing on both: 0. Regressed: 0. Passing on both: 1. A verdict links to the run's trace in Cloud Trace (project access needed); the scenario link is its source with the # issue: it reproduces.
Updated terminal-session checks to wait for the shell to become ready before continuing and to allow more time for terminal-size updates to appear. This makes automated verification of session startup and resizing more consistent, including when those updates take longer to arrive.
…s stty size
The terminal-relay proof resizes the attached pty and, half a second later,
types the line that prints `stty size` in the session. The resize travels
client -> ssh -> daemon -> the session's pty, and on a loaded host that relay
takes longer than the helper's pause, so the line printed the old size and the
proof read as a failed resize (nine lab pre-push runs on unrelated branches,
two of them tonight). The typed line now polls `stty size` for up to ten
seconds until it shows the new size, then prints it. The assertion is the
same: the size the session reports is the resized one.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Essentials
Run ID: 24219835-8e41-492f-a6eb-5f0cf8684c73
Commits
Reviewing files that changed from the base of the PR and between 29e4daf and 34c7269.
Files selected for processing: 1
scripts/session-e2e.sh
No actionable comments were generated in the recent review.
Walkthrough
The PTY driver can wait for a configured startup marker before quiet detection. The terminal-relay resize proof sends a readiness marker and polls for the resized terminal dimensions.
Changes: PTY readiness and resize proof
Layer / File(s)
Summary
Readiness marker and quiet detection scripts/e2e-attach-pty.py
The driver reads the optional E2E_PTY_READY setting. While its encoded marker is absent from captured output, the quiet counter stays at zero.
Terminal resize proof scripts/session-e2e.sh
The proof sends RELAY_READY, then polls stty size for up to 10 seconds and prints progress dots before reporting the result. The expected-size assertion is unchanged.
gominimal/minimal#3155: The issue reports an intermittent failure in the terminal-relay step; this change adds synchronization to the resize proof but does not claim to fix the missing-session symptom.
Possibly related PRs
gominimal/minimal#2835: That PR changes attach handling for terminal window adjustments during outbound redraws; this PR tests resize propagation through the attached session relay.
Suggested reviewers:norrietaylor
Merge Risk: Minimal · previous review 29e4d
No merge-blocking risk was identified in the readiness and resize changes.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly identifies the main change: the session shell waits for the resized terminal dimensions before reporting stty size. It is concise, specific, and follows the required Conventional C…
Description check
The description explains what changed, why the change was needed, and how it was verified. It includes detailed test results and regression evidence. It uses ## What, ## Why now, and `## Verificat…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title clearly identifies the main change: the session shell waits for the resized terminal dimensions before reporting stty size. It is concise, specific, and follows the required Conventional Commit format.
Full details: Description check
Explanation
The description explains what changed, why the change was needed, and how it was verified. It includes detailed test results and regression evidence. It uses ## What, ## Why now, and ## Verification instead of the template's ## Summary, ## Testing, and ## Checklist headings, and it omits the checklist, but the required information is mostly present.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (1 skipped: 1 too large.)
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit waits beside the screen, For RELAY_READY to appear. The quiet counter holds its place, While stty size is checked with care. The resized rows and columns cheer.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Essentials
Run ID: ce79931d-4e6a-4772-a251-1940eb1b264e
Commits
Reviewing files that changed from the base of the PR and between 620a1ce and 29e4daf.
Files selected for processing: 2
scripts/e2e-attach-pty.py
scripts/session-e2e.sh
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 1 remain after this review.
Reviewed: no findings; approval withheld, 2 threads unresolved at 34c7269bd9e8.
APPROVE withheld: 2 unresolved threads — norrietaylor scripts/session-e2e.sh, norrietaylor scripts/session-e2e.sh. Resolving the last thread releases the approval on this head.
Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required.
The helper sends the detach chord once the pty has been quiet for two
one-second drains, so a wait that printed nothing was cut off at about two
seconds and RELAY_SIZE never printed: the 10 s budget was unusable. Each
poll now prints a dot, and the size line follows on its own line.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Under load the sandbox can take longer than two seconds to mint, so the
helper's quiet counter fired before the shell ran a line and the resize
beat detached an empty attach. With E2E_PTY_READY set, the stream is not
quiet until the shell printed the marker; the resize beat passes RELAY_READY.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The relay beat's ready check searched the stream for RELAY_READY, and the
pty echoes the typed `echo RELAY_READY` back, so the marker could match
the echo before the shell had run anything and quiet counting resumed
early. The typed line is now `echo RELAY_"READY"`: its echo never
contains the marker, only the shell's output does. The check also no
longer copies the buffer on every pass.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Scope matches the title. The PR body does not describe the second commit, which adds E2E_PTY_READY to the helper. Update the body to describe it. Clobber: none. The diff only adds to proof_tty_relay and the helper's quiet counter. Correctness: the poll loop prints a dot on each pass, which keeps the stream busy. This fixes the problem from the first round, where the detach chord cut the wait at about 2 s. A resize that never arrives still prints the old size, and the unchanged assertion fails on it. seq and fractional sleep occur in many other session-side loops in this script. The E2E_PTY_READY guard does not protect against the slow mint that it targets. The marker is also the typed text, and the echo of the typed text satisfies the check (see the finding). The open bot thread on e2e-attach-pty.py:309-311 raises the same point. This finding adds the cooked-mode echo path and a committable fix. Not checked: a run of the e2e, which needs a build and a VM.
The reason will be displayed to describe this comment to others. Learn more.
medium · The ready marker is also the typed text, so the guard is satisfied before the shell runs a line. The helper writes echo RELAY_READY at 1.5 s, while min session attach can still be connecting. The local pty is then in cooked mode with ECHO on, and the kernel echoes the line into buf at once. The remote pty echoes it again before the shell reads it. READY_BYTES not in bytes(buf) searches the whole transcript, so it matches the echo. Quiet counting then starts at once, and the slow mint that the second commit targets still gets the second stage and the chord too early. The helper's default stream already avoids this: e2e-attach-pty.py:99-103 builds TOOL_%s_BEFORE with printf so the marker appears only as output. Use the same shape here. Also add one sentence to the E2E_PTY_READY docstring that says the marker must not appear in the typed text. This is the only E2E_PTY_READY caller, so the fix is local.
The reason will be displayed to describe this comment to others. Learn more.
Taken, in 29e4daf, in the shape you gave: the beat types printf "RELAY_%s\n" READY, so neither the local echo nor the remote one carries the marker, and the E2E_PTY_READY docstring now says the marker must never appear in the typed text and why. The earlier push had the same effect through quoting (echo RELAY_"READY"); printf is the clearer of the two, so that is what stays.
The relay beat typed the marker with echo and quoting, which kept it
out of the pty's echo but read as a trick. The typed line now builds it
with printf, the shape the helper's own default stream uses, and the
helper's E2E_PTY_READY docstring says the marker must never appear in
the typed text, since the pty echoes what is typed into the transcript
before the shell runs it.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Scope matches the title. The diff changes the relay resize beat in scripts/session-e2e.sh and adds E2E_PTY_READY to scripts/e2e-attach-pty.py, and session-e2e.sh:8007 is its only caller. Clobber none. git log on both files shows only this PR's commits above 65e83ca3. Correctness The poll loop prints a dot every 0.25 s. That keeps quiet below 2, so the chord cannot cut the 10 s wait short. The marker is built with printf "RELAY_%s\n" READY, so neither pty echo carries RELAY_READY. The assertion at session-e2e.sh:8016 matches RELAY_SIZE[41 117]. The echoed typed line holds only RELAY_SIZE[%s] and "41 117", so the echo cannot satisfy it. A shell that never prints the marker holds quiet at 0. That wait has a bound: the helper's 240 s DEADLINE, or EOF when the attach ends. All three earlier threads are answered and taken at head. Not checked I did not run the e2e, because it needs a build and a session.
The reason will be displayed to describe this comment to others. Learn more.
nit · The comment block tells the story of the PR's review rounds. Examples: "the chord then left before a line ran" (past tense, about the old failure) and "as the helper's own default stream does". It also restates the E2E_PTY_READY docstring in e2e-attach-pty.py:29-38. Keep the two facts the reader of this line needs: the dots keep the pty busy, and the marker is built with printf so the echo does not contain it. Point to the helper docstring for the rest.
The reason will be displayed to describe this comment to others. Learn more.
Trimmed in 34c7269 to the two facts the line needs, the dots keep the pty busy so the quiet-stream detach does not fire, and the marker is built with printf so the echo never contains it, with a pointer to the E2E_PTY_READY docstring for the rest.
The dots keep the pty busy so the helper's quiet-stream detach does not
fire while the shell waits, and the ready marker is built with printf so
the pty's echo never contains it; the helper's docstring has the rest.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
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.
What
The terminal-relay proof in
scripts/session-e2e.shresizes the attached pty to 41x117 and, half a second later (the helper's fixed pause), types the line that printsstty sizein the session. The resize travels client → ssh → daemon → the session's pty, and on a loaded host that relay takes longer than the pause, so the line printed the old size and the proof read as "a resize of the attached terminal did not reach the session's 'stty size'". The typed line now pollsstty sizefor up to ten seconds until it shows the new size, then prints it. The assertion is unchanged: the size the session reports is the resized one; a resize that never arrives still fails.Why now
The lab's pre-push lane saw this beat fail on nine runs of unrelated branches (two tonight, within two hours, with the host at a load of about 11), each time with every other e2e beat green. It is the lane's most frequent false red.
Verification
native-daemon-e2estep runs this beat.min-regress batch
e2e-resize-wait-1cb2796c: baserel-0bb305b9vs the candidatecand-e2e-resize-wait-1cb2796c-1daa6559, per distro:Lab evidence (min-regress batch
e2e-resize-wait-1cb2796con the lab box, ubuntu-24.04)Head:
1cb2796ca13c641c7aea86d283d5c9fe7f00a0caat 1cb2796 (lab namecand-e2e-resize-wait-1cb2796c-1daa6559). Base:rel-0bb305b9(rel-0bb305b9).Fixed since base: 0. Still reproducing on both: 0. Regressed: 0. Passing on both: 1. A verdict links to the run's trace in Cloud Trace (project access needed); the scenario link is its source with the
# issue:it reproduces.🤖 Generated with Claude Code
Summary by CodeRabbit