Repository navigation
fix(ui): the plan run's child gets no console on stdin (M14-02) - #291
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sandgraal
marked this pull request as ready for review
October 19, 2026 07:02
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
sandgraal
added a commit
that referenced
this pull request
Oct 19, 2026
…-72; M14-09 pointers (#293) ## What Conductor docs lane (ADR-0013 amendment, 2026-10-17): `docs/BACKLOG.md` only. - Ticks M14-02 (#288), M14-05 (#286), M14-06 (#287), M14-08 (#290). - M14-02: a dated note on the Windows stdin follow-up (#291, 826ce3d): why `DEVNULL` made the child's `isatty()` True on Windows, what changed, and that Windows on `main` at 826ce3d is green. - M14-08: a dated note from the #290 reviews (shipped-table colours vs the ticket's "from the model" wording, which is an owner item; the `aria-live` region; the `page.css` exemption in the colour grep test). - M14-09: pointers from the #290 code review (the `loadGear` refactor, the intro paragraph and allowlist test that change, the default-flavor question, the `aria-live` narrowing). - New tickets: M13-71 (`guard_bash.py`: `--input` bodies and the substring allowlist match, from the #289 review; conductor lane) and M13-72 (`char list --json` carries every full record under the Character tab, and the colour grep's `page.css` exemption; from the #290 code review, off that branch). ## Proof - `grep -c '^## \[' docs/BACKLOG.md` → 87 headings; the four ticks are the only `[ ]` → `[x]` changes. - Pre-commit hooks green on the commit (merge-conflict check, end-of-file). - Rebased on `main` at 7415e07 (#289); the one conflict (M13-70 and M13-71 both appended after M13-69) was resolved by keeping both. ## Reviewed by `code-reviewer` CLEAN at cec1ea3 (every claim checked against main and the CI history; the one nit, a blank line before M13-72, fixed in the follow-up commit). session: 284d1153-c891-4b84-8e47-150842fac4ba 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- 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
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.
Ticket
M14-02 follow-up (#288, merged c8d6f68). The UI server's plan-run child saw a terminal on stdin on Windows; the Windows run on
mainat c8d6f68 was red for this case (https://github.com/sandgraal/wowlab/actions/runs/39206897102).Acceptance — with proof
UiServer._startnow passesstdin=subprocess.PIPEand closes the writing end right afterPopen(before the suspended Windows child is resumed), instead ofDEVNULL, whose NUL device is a character device on Windows and reportsisatty()True.test_ui_confirm.py::test_a_plan_run_never_carries_yes_and_runs_with_stdin_closedis kept as it was (tty False, stdin reads ""), with a comment on why; it passes locally.windows.ymlrun https://github.com/sandgraal/wowlab/actions/runs/39209908024 success on 2f553eb.lab/core/tests/reviewandtests/scripts/test_readme.py: 963 passed, 8 skipped.make lintclean.make cigreen: it ran in full on the M14-02 branch before feat(ui): the confirm flow on snap gc and doctor --yes (M14-02) #288; CI on this PR is the confirmation.luadata.py,guard.pyor another parser changed? No.Invariants (AGENTS.md)
Judgment calls
Claims for reviewers to test:
cli._interactive()would have counteddoctoras interactive under the UI on Windows. This was latent because every UI run carries--json. The closed pipe gives the same guarantee (no terminal, EOF on read) on POSIX and Windows.Popenand before the suspended Windows child is resumed, so the child can never observe an open pipe with a live writer.Reviews
Reviewed by: code-reviewer (narrow) CLEAN, no findings: "stdin closed" is now true on Windows, subprocess rules hold,
_watchuses reader threads and nevercommunicate(); the test failed on #288's Windows run 39206897102 (head d73111e, DEVNULL) for both[snap gc]and[doctor]ontty is Falseand passes on run 39209908024 (6763 passed, 437 skipped).security-reviewer (narrow) CLEAN, no findings: the close cannot block (writer closed before the child has written anything and before the suspended Windows child is resumed); no descriptor leak on success or Popen failure (probe 50+50 starts, fd count unchanged); timeout, tree kill, output cap, env scrub,
sys.executableargv andshell=Falseuntouched;uv run --frozen pytest -q lab/core/tests/test_ui*.py379 passed, 1 skipped.session: 284d1153-c891-4b84-8e47-150842fac4ba
🤖 Generated with Claude Code