Skip to content

[ON HOLD — upstreamed as microsoft/conductor#510] PDA-35: Fix: discover interactive input treats each newline (including pasted multi-line text) as a separate prompt - #5

Open
throup wants to merge 4 commits into
mainfrom
PDA-35-pandora
Open

throup wants to merge 4 commits into
mainfrom
PDA-35-pandora

Conversation

@throup

@throup throup commented Sep 2, 2026

Copy link
Copy Markdown

Important

On hold — do not merge yet. This fix has been proposed upstream, and the
outcome there decides what happens to this PR.

Accepted upstream → take it via an upstream sync, close this without merging.
Rejected or stalled → merge this and maintain it as a fork-local patch.

The upstream branch is not byte-identical to this one, and one AC in PDA-35
does not describe upstream behaviour — see
this comment.


Ticket: https://toogoodtogo.myjetbrains.com/issue/PDA-35

Automated delivery via the Pandora workflow.

Review notes

Review — PDA-35

Panel verdict: pass
Min score across lenses: 9
Guard decision: accept (after 1 round(s))

Verification (real exit codes, run by the pipeline)

Passed: True

cmd 1: PASS;cmd 2: PASS;cmd 3: PASS;cmd 4: PASS;

Outstanding issues

…lines in human.py) and wired the terminal dialog gate to use it with a /send sentinel so pasted multi-line blocks and mid-paste EOF are handled as one turn instead of fragmenting/dismissing; added regression tests in both test files (lint/format clean; two pre-existing unrelated typecheck errors in claude_agent_sdk.py confirmed present before this change).
@throup
throup requested a review from joaomena September 2, 2026 15:21
@throup

throup commented Sep 2, 2026

Copy link
Copy Markdown
Author

Independent review — head 2641a18bf15e4868136115f1ffc8db934a9285d6

Reviewed pinned to that SHA in a local worktree; head had not moved. The core idea is right and the human.py extraction is faithful, but the tty branch drops the two non-/send exits, and both are verified regressions against main.

Blocking

1. Ctrl-D on a tty no longer dismisses — it hot-spins forever. src/conductor/gates/dialog.py:777-782

On the tty main-turn path read_multiline_lines catches EOFError and returns the accumulated text. When the paste is already submitted, the next turn's read EOFs immediately and returns "", which the new if user_input == "": continue guard (line 291) sends straight back round the loop. _get_user_input can no longer return None on this path, so the user_input is None dismissal branch (line 286) is unreachable for the main turn.

This is exactly the "guard that turns a would-be dismissal into an infinite continue" case. Real input() on an EOF stdin returns instantly rather than blocking, so this burns CPU rather than parking:

5000 EOFErrors in 0.005s

Scenario: user finishes the grill and presses Ctrl-D to exit, as works on main. Once stdin is at EOF it stays at EOF, so every subsequent read EOFs and the dialog never exits. Typing a dismiss keyword still works, but Ctrl-D — the documented EOF exit named in the reader's own hint text (or Ctrl-D) — does not.

Verified by driving handle_dialog with isatty=True and input always raising EOFError:

Evidence
branch 2641a18: RESULT: INFINITE LOOP: >200 input() calls, never exited
origin/main 5b06b92: RETURNED dismissed= True   (input() call count: 1)

2. KeyboardInterrupt on a tty now crashes the workflow instead of dismissing. src/conductor/gates/dialog.py:777-782

The new branch returns before the try that catches (EOFError, KeyboardInterrupt), and read_multiline_lines catches only (EOFError, StopIteration). So Ctrl-C during a main turn propagates out of read_on_daemon_thread and unwinds handle_dialog. The docstring added in this PR states None is returned "on genuine no-input (non-tty EOF or KeyboardInterrupt)" — the code does not do this on the tty path.

Evidence — same harness, input raising KeyboardInterrupt
branch 2641a18: unhandled KeyboardInterrupt propagating through
  dialog.py:284 handle_dialog -> dialog.py:781 _get_user_input
  -> human.py:120 read_on_daemon_thread -> human.py:67 read_multiline_lines
origin/main 5b06b92: RETURNED dismissed= True

Both are unnoticed because the three new dialog tests only cover paths that end in an explicit "done"; none asserts that EOF-with-no-content or Ctrl-C dismisses. A regression test for each would have caught them.

Non-blocking

3. except StopIteration is production code shaped around a test double. src/conductor/gates/human.py:64-70 (non-blocking)

The comment's factual claims check out — real input() never raises it, and I confirmed StopIteration does cross the read_on_daemon_thread boundary intact and is caught (both direct and threaded calls returned 'a' from an exhausted side_effect=["a"]). But it makes an exhausted side_effect silently indistinguishable from a deliberate EOF, so a test that under-supplies inputs passes while looking like it exercised EOF. Finding 1 is a case where a test with a too-short side_effect list would look fine. Preferring side_effect=[..., EOFError()] in the tests and dropping StopIteration would keep the production path honest.

4. /send is undocumented and unhinted. (non-blocking)

No hit for /send anywhere in docs/, README.md or CHANGELOG.md, and _display_dialog_start (line 648) still only advertises done / /done. Users learn the new submit key solely from the reader's runtime hint. A CHANGELOG.md [Unreleased] entry looks warranted given the section is actively maintained and this changes a user-facing interaction contract.

Verified as claimed / no issue found

  • human.py extraction is behaviour-preserving. Compared against origin/main line by line: hint text, MULTILINE_SENTINEL default, line.strip() == sentinel, lines.append, and "\n".join(lines).rstrip("\n") are all identical. Only delta is the added StopIteration. Internal blank lines preserved, trailing stripped, padded sentinel still submits (' /send ' -> ''). Human gate's 40 tests pass.
  • Web path unaffected — confirmed by patching the reader to raise if called; the web dialog completed normally.
  • Non-tty path unchanged — with isatty=False and Prompt.ask raising EOFError, dismissal still occurs and the new reader is never reached (verified by patching it to raise).
  • The "" guard is genuinely reachable and correct for its stated case: a bare /send, or only blank lines, both yield "".
  • Commit's typecheck claim is accurate. Both call-non-callable errors in claude_agent_sdk.py:1418 and :1439 are present on origin/main too, and that file is byte-identical between branches (matching md5). make lint is clean. Note main reported 6 diagnostics vs the branch's 2 — the extra 4 are unused-ignore-comment warnings from a differing local SDK install, not a branch effect.
  • uv run --frozen pytest tests/test_gates -q -> 69 passed.

Not verified

Full tests suite not run (scoped to tests/test_gates for time). No manual check in a real interactive terminal — all tty behaviour above was established by patching sys.stdin.isatty and builtins.input, plus one unmocked input() check against a closed pipe for the EOF-timing claim.

🤖 Generated with Claude Code

The new tty branch in _get_user_input returned before the try/except that
catches (EOFError, KeyboardInterrupt), so the dialog's main turn lost both
of its non-sentinel exits:

- Ctrl-D no longer dismissed. read_multiline_lines converts EOF into a
  returned string, so an empty read yielded "", which the `user_input == ""`
  guard sent back round the loop. _get_user_input could never return None on
  this path, leaving the dismissal branch unreachable and spinning on
  repeated EOF rather than exiting the grill.
- Ctrl-C propagated out of handle_dialog uncaught, crashing the run. The
  docstring added alongside it claimed None was returned on
  KeyboardInterrupt; the code did not do that.

Both were regressions against main, where every path ran through the handler
and returned None.

read_multiline_lines now returns (text, hit_eof) so a caller can tell a paste
that ended at EOF from a deliberate Ctrl-D at an empty prompt. The dialog gate
dismisses on (hit_eof and not text) and re-wraps the await in the existing
except clause; a paste terminated by EOF still submits its content, so AC4
holds. HumanGateHandler._read_multiline unpacks and returns a plain str, so
the human gate's contract is unchanged.

The three existing dialog tests all ended with an explicit "done", which is
why neither exit path was covered. The two new tests use a bare
side_effect=EOFError()/KeyboardInterrupt() rather than a list, so a future
regression hangs or raises instead of passing on an exhausted mock; with the
fix reverted the Ctrl-D test hangs, and pytest is killed before printing a
summary.

Verified: tests/test_gates 72 passed (was 69); full suite 7624 passed,
3 failed, all three failing identically on clean origin/main (chmod 0o000
permission tests that do not hold on this filesystem). make lint clean;
make typecheck reports the same 2 pre-existing diagnostics in
claude_agent_sdk.py and none in the touched files.

Addresses the two blocking findings from the review on PR #5. The /send
discoverability gap (the Dialog Mode banner still advertises only done//done)
is left unaddressed here.
@throup

throup commented Sep 2, 2026

Copy link
Copy Markdown
Author

Both blocking findings fixed in ef9f417

Root cause of both: the tty branch in _get_user_input returned before the try/except (EOFError, KeyboardInterrupt), so the main dialog turn lost both of its non-sentinel exits. On main, every path ran through that handler and returned None.

A second contributing factor for finding 1: read_multiline_lines collapsed "submitted via /send" and "hit EOF" into the same bare string, so the caller could not tell a paste that ended at EOF from a deliberate Ctrl-D.

The fix:

  • read_multiline_lines now returns (text, hit_eof).
  • The dialog gate dismisses on hit_eof and not text, and re-wraps the await in the existing except clause. A paste terminated by EOF still submits its accumulated content, so AC4 is preserved rather than traded away.
  • HumanGateHandler._read_multiline unpacks and returns a plain str, so the human gate's contract is unchanged.

On the test-coverage gap you identified — correct, and it was the reason both regressions shipped. The two new tests use a bare side_effect=EOFError() / KeyboardInterrupt() rather than a list, so a future regression hangs or raises instead of quietly passing on an exhausted mock.

Verification

Confirming the new tests are genuine regression tests: with only the dialog fix reverted (keeping the tuple-returning reader), pytest -k 'ctrl_d or ctrl_c' printed no summary line at alltimeout 120 killed pytest, because the Ctrl-D test hangs in the infinite continue loop. With the fix restored: 2 passed in 0.23s.

  • tests/test_gates: 72 passed (was 69).
  • Full suite: 7624 passed, 3 failed, 80 skipped. All three failures reproduce identically on clean origin/main (5b06b92) — they are chmod 0o000 permission tests that do not hold on this filesystem, and the skip guard only covers root and Windows.
  • make lint: clean (423 files formatted).
  • make typecheck: 2 diagnostics, both pre-existing in claude_agent_sdk.py, none in the touched files. This also confirms the original commit's pre-existing-typecheck claim.

Not addressed here: the /send discoverability gap. The per-turn hint does render Finish with '/send' on its own line (or Ctrl-D)., but the Dialog Mode banner at dialog.py:648 still advertises only done//done. Left out of this commit to keep it scoped to the two blocking regressions.

🤖 Generated with Claude Code

@throup

throup commented Sep 2, 2026

Copy link
Copy Markdown
Author

Re-review — head ef9f417230bd9fbed3f90d67c2eebb27753259f7

Reviewed pinned to that SHA in a local worktree; confirmed against gh pr view that the head had not moved. Both blocking findings are resolved, and the fix introduces nothing blocking. The tuple return is the right shape for the problem — it gives the caller the one bit it was missing rather than trading AC4 away to get the exit back.

Original finding 1 — Ctrl-D hot-spin — RESOLVED

dialog.py:786-789. Driving the real handle_dialog with isatty=True and input raising EOFError forever now exits on the first read:

Ctrl-D at empty prompt (EOF forever)   inputs=1  EXITED dismissed=True turns=0

Against the >200-call non-exit recorded at 2641a18. The hit_eof and not text guard fires before the == "" continue, so the dismissal branch at dialog.py:286 is reachable again.

Original finding 2 — Ctrl-C propagating — RESOLVED

dialog.py:783-784. Same harness with input raising KeyboardInterrupt: inputs=1 EXITED dismissed=True, no traceback. I also confirmed both arms of the new except are live — KeyboardInterrupt from input() returns None, and an EOFError escaping the reader by any other route also returns None (the reader swallows input()'s own EOFError, so that arm is defensive, matching the single-line path below it — correct, not dead-weight).

AC4 is preserved, not traded away

The concern was that fixing the exit would re-break paste-ending-EOF. It does not. Real handle_dialog:

AC4: 3-line paste ended by EOF, then Ctrl-D to leave
   user turns delivered: ['line one\nline two\nline three']   provider_calls=1
AC4: paste ended by EOF, then a second turn via /send, then done
   user turns delivered: ['para A\npara B', 'second turn', 'done']  provider_calls=2

The pre-existing test_eof_mid_paste_submits_content_not_dismissal still passes, so AC4 keeps a guard of its own.

The new tests are genuine regression tests — verified, not taken on trust

I reverted only the dialog guard in a scratch worktree at ef9f417 (keeping the tuple-returning reader) and ran the two tests under timeout:

Evidence
both tests together:                          EXIT=124 (killed by timeout, no pytest summary printed)
test_ctrl_d_at_empty_prompt_dismisses:        EXIT=124 (hangs)
test_ctrl_c_dismisses_rather_than_propagating: EXIT=2  (KeyboardInterrupt aborts the pytest session)

With the fix in place: tests/test_gates -> 72 passed in 0.42s.

The author's claim about the hang is accurate. Using a bare side_effect=EOFError() rather than a list is the right call — a future regression hangs loudly instead of passing on an exhausted mock, which is exactly the failure mode that let these two ship.

The human gate really is unchanged

Not just inspected — differential-tested. I imported origin/main's human.py alongside the branch's and ran both _read_multiline implementations over every input sequence up to length 3 drawn from ["a", "", " ", ".", " . ", "..", "/send"], each terminated by both EOFError and the sentinel:

total=800  mismatches=0

Return type is still str. And read_multiline_lines has exactly one other caller — dialog.py:784, which was updated. grep across src/, tests/ and docs/ finds no third caller, so there is no latent TypeError from a missed unpack.

Every exit path re-traced — no path can loop

All eleven scenarios exit; none reaches the 400-call loop tripwire.

Full trace
Ctrl-D at empty prompt (EOF forever)      inputs=  1  EXITED dismissed=True turns=0
Ctrl-C                                    inputs=  1  EXITED dismissed=True turns=0
dismiss keyword 'done' then /send         inputs=  2  EXITED dismissed=True turns=1
dismiss '/done' then /send                inputs=  2  EXITED dismissed=True turns=1
bare /send (empty) then Ctrl-D            inputs=  2  EXITED dismissed=True turns=0
blank lines + /send, then Ctrl-D          inputs=  4  EXITED dismissed=True turns=0
paste then EOF (content submitted)        inputs=  4  EXITED dismissed=True turns=1
content+/send, then done+/send            inputs=  4  EXITED dismissed=True turns=2
WHITESPACE-only line then EOF forever     inputs=  3  EXITED dismissed=True turns=1
WHITESPACE-only + /send, then EOF         inputs=  3  EXITED dismissed=True turns=1
tab-only line then EOF forever            inputs=  3  EXITED dismissed=True turns=1

No blocking findings

Nothing here blocks. Two small notes below, neither worth holding the PR for.

Non-blocking

1. A whitespace-only line then EOF sends a whitespace turn to the agent. human.py:84, dialog.py:788 (non-blocking, cosmetic)

You asked specifically about the rstrip boundary. rstrip("\n") strips newlines only, not spaces, so [" ", EOFError] gives text=" ", hit_eof=True — truthy, so hit_eof and not text does not fire and one whitespace-only turn is dispatched before the next EOF dismisses:

["   ", EOFError] -> text='   '   hit_eof=True  -> dialog returns '   ' (one wasted turn)
["",    EOFError] -> text=''      hit_eof=True  -> None (DISMISS)   <- correct

The user still escapes on the next read (verified above, inputs=3), so this is one wasted round-trip in a contrived case, not a trap. Worth noting that not text.strip() would close it, but a genuine paste of indented code whose first line is blank-but-indented would then be dismissed instead of sent — so the current not text is arguably the safer of the two. Fine as-is; flagging only because it was the boundary under scrutiny.

2. /send is still undocumented, and the Dialog Mode banner still advertises only done//done. dialog.py:648 (non-blocking, carried over)

Acknowledged as out of scope in the fix write-up, which is reasonable for a commit scoped to two regressions. Re-recording it so it is not lost: no hit for /send in docs/, README.md or CHANGELOG.md, and CHANGELOG.md's [Unreleased] section is actively maintained while this PR touches no non-code file. A follow-up ticket rather than a change here.

Finding 3 from the previous review (except StopIteration shaped around a test double) still stands as written and is still non-blocking; the fix moved that line but did not change the argument either way.

What I ran

  • uv run --frozen pytest tests/test_gates -q -> 72 passed in 0.42s.
  • The two new tests against a scratch worktree with only the dialog guard reverted -> exit 124 / exit 2, as above.
  • 800-case differential test of _read_multiline against origin/main -> 0 mismatches.
  • 11-scenario exit-path trace through the real handle_dialog.
  • make lint -> clean (All checks passed!, 423 files formatted).
  • make typecheck -> 2 diagnostics, both pre-existing call-non-callable in claude_agent_sdk.py:1418/:1439; none in the four touched files. Exits non-zero for that pre-existing reason alone.
  • git diff origin/main...HEAD (three-dot) -> 4 files, +257/-22, code and tests only.

Not verified

  • No manual check in a real interactive terminal — all tty behaviour above comes from patching sys.stdin.isatty and builtins.input.
  • Full tests suite: I started it but it stalled at ~59% on this machine and was killed by my own timeout without printing a summary, so I am not claiming the author's 7624 passed / 3 failed figure as re-verified either way. The stall is not attributable to this PR — it is well past tests/test_gates, which covers all four touched files and passes in 0.42s. Anyone wanting that number confirmed should re-run it.

🤖 Generated with Claude Code

@throup

throup commented Sep 2, 2026

Copy link
Copy Markdown
Author

Correction to my re-review above — full suite did complete, and it passes

In my review I said the full suite "stalled at ~59% ... and was killed by my own timeout", and declined to verify the 7624 passed / 3 failed figure. That was wrong. The suite had not stalled — it was simply slow, and it finished after I'd posted:

3 failed, 7624 passed, 80 skipped, 25 warnings in 374.02s (0:06:14)

That matches the number in the fix write-up exactly, so consider it re-verified rather than unverified. I misread a slow stretch of dots as a hang and should have waited for the process to exit before characterising it.

The 3 failures are the known pre-existing ones, and I checked that directly rather than inheriting the claim — run against the clean origin/main clone at 5b06b92 with a clean tree, all three fail identically in 0.15s:

FAILED tests/test_plugins/test_registry.py::TestUnreadableTrees::test_unreadable_skill_subdirectory_is_reported
FAILED tests/test_skills/test_path_entries.py::TestUnreadableParent::test_unreadable_parent_is_reported_not_raised_raw
FAILED tests/test_skills/test_path_entries.py::TestSkillsRootDiagnostics::test_mis_cased_skill_md_is_reported

Neither file references gates/dialog.py, gates/human.py or read_multiline_lines, and both rely on chmod — environmental, unrelated to this PR.

Everything else in the review stands unchanged: both original blocking findings resolved, no blocking findings, two non-blocking notes. The only remaining unverified item is the lack of a manual check in a real interactive terminal.

🤖 Generated with Claude Code

The multi-line turn requires /send to submit, but the opening banner still
told users only to "type your responses below" and to say done//done when
finished -- so the first instruction a user reads omitted the one keystroke
a turn now needs. The per-turn hint from read_multiline_lines did name it,
but the banner is read first.

The sentinel is passed to styled() as a positional argument alongside
agent.name, so the banner cannot drift from DIALOG_SUBMIT_SENTINEL.

The new test renders the panel for real and asserts against the constant
rather than a literal, matching the approach in
tests/test_cli/test_markup_injection.py, which renders this same banner to
catch interpolation that a mocked Panel would hide.

Verified: tests/test_gates and tests/test_cli/test_markup_injection.py
116 passed; make lint clean. Rendered at width 76 to confirm the wrapped
text reads correctly.

Addresses the non-blocking documentation finding carried across both reviews
on PR #5.
@throup

throup commented Sep 3, 2026

Copy link
Copy Markdown
Author

Review of 84b9777 — third round

Reviewed at head 84b9777. Scope: the new commit only; the conclusions of the two prior rounds still hold — 84b9777 touches nothing they covered (its only src delta vs ef9f417 is the banner string; both except (EOFError, KeyboardInterrupt) handlers from ef9f417 are intact at this head).

84b9777 is sound. Nothing blocking.

The three claims in the commit message check out:

  • Interpolation is safe. The sentinel goes through styled() as a positional argument, which inserts values verbatim, so it is not re-parsed as markup. Rendered for real: an agent named reviewer[bold] survives character-for-character, so this does not reintroduce the deleted-name class of bug that test_dialog_opening_renders_the_agent_name_in_body_and_title guards. That test still passes at this head.
  • The new test constrains the banner. Both mutations behave as claimed — renaming the constant to /transmit keeps it green (it tracks the constant, not a literal), and dropping the sentinel from the banner fails it.
  • The wording is accurate on the tty path. "Can span multiple lines", /send on its own line, and done//done to finish all match behaviour; done still dismisses through the multi-line reader, since _is_dismiss strips the collected text.

Non-blocking

The banner advertises /send on the non-tty path, where it does not apply. _display_dialog_start is called unconditionally on the terminal path (dialog.py:248), but the multi-line reader is gated on prompt_text is None and sys.stdin.isatty() (dialog.py:783). With a non-tty stdin the banner still says the response "can span multiple lines" and to "send it with /send on its own line", while input actually goes through the single-line Prompt.ask branch — where a response cannot span lines and /send is not the submit mechanism. The per-turn hint from read_multiline_lines is correctly absent there, so the banner is the only instruction shown, and it is wrong for that path.

Low impact: the realistic non-tty dialog consumer is the web dashboard, which returns early via _web_handle_dialog and never renders this banner, so this is mostly CI and piped-stdin runs where nobody is reading. Worth a follow-up rather than a change here — the fix is a branch on sys.stdin.isatty() in the banner, which is more than this commit should carry.

What I ran
uv run --frozen pytest tests/test_gates tests/test_cli/test_markup_injection.py -q
  -> 116 passed (matches the commit message)

make lint
  -> All checks passed! / 423 files already formatted

uv run --frozen pytest -q --ignore=tests/test_skills --ignore=tests/test_plugins
  -> 6991 passed, 69 skipped in 313s (exit 0)

Rendering_display_dialog_start via make_console(file=buf, no_color=True) at widths 40/60/76/100. Wraps sensibly, panel intact at every width. At width 40:

╭──────────── Dialog Mode ─────────────╮
│ Agent 'reviewer[bold]' would like to │
│ discuss its output with you.         │
│ Type your response below; it can     │
│ span multiple lines. Send it with    │
│ /send on its own line. Say done or   │
│ /done when finished.                 │
╰──────────────────────────────────────╯

Mutation testing of the new test — the constant change was run against a git archive copy of 84b9777 with PYTHONPATH pointed at the mutated tree, after confirming conductor.gates.human.__file__ resolved there (a first attempt via a plain directory copy silently loaded the original worktree's venv and produced a false result):

Mutation Test outcome
DIALOG_SUBMIT_SENTINEL = "/transmit" passes — banner rendered /transmit, tracking the constant
banner reverted to the pre-84b9777 text fails — assertion not satisfied

done on the tty multi-line path, with isatty patched True and input() driven from a list:

Input Result
done, /send dismissed, provider not called
done, EOF dismissed, provider not called
/done, /send dismissed, provider not called
hello, world, /send, done, /send one turn dispatched, then dismissed

Non-tty path, driving handle_dialog with a non-tty stdin: banner shown (Dialog Mode present, mentions /send), Prompt.ask called twice — once for the engagement select, once for the turn with choices=None — and the read_multiline_lines hint absent, confirming the single-line branch.

Cumulativegit diff origin/main...HEAD --stat: 4 files, +282/-24, confined to gates/dialog.py, gates/human.py and their tests. Nothing unrelated picked up.

Not verified: make typecheck (2 pre-existing claude_agent_sdk.py diagnostics also on main) and the test_skills/test_plugins chmod permission tests, which are known pre-existing failures.

🤖 Generated with Claude Code

84b9777 added the /send hint to the Dialog Mode banner unconditionally, but
_display_dialog_start is called for every terminal dialog (dialog.py:248)
while the multi-line reader is gated on sys.stdin.isatty(). Off a tty the
turn falls back to the single-line Prompt.ask branch, where the sentinel does
nothing -- so the banner told the user to type something with no effect, and
it was the only instruction shown there (read_multiline_lines' per-turn hint
is correctly absent).

This was a regression introduced by 84b9777: on main the banner named only
done//done, which is accurate on both paths.

The hint is now built behind the same sys.stdin.isatty() condition as the
reader, with the [bold] markup kept in the styled() template -- styled()
inserts *values* verbatim precisely so they cannot be parsed as markup, so
interpolating pre-marked-up text would render a literal "[bold]".

The test is now parametrised over isatty and asserts presence-iff-tty rather
than mere presence, so neither direction can regress unnoticed; it also
asserts no literal "[bold]" survives. Confirmed it fails when the gate is
removed (the isatty=False case) and passes with it.

Low impact -- the realistic non-tty consumer is the web dashboard, which
returns via _web_handle_dialog (dialog.py:217) and never renders this banner
-- but the gate costs three lines and the condition already existed.

Verified: tests/test_gates and tests/test_cli/test_markup_injection.py
117 passed; make lint clean; rendered both paths at width 76. Separately
confirmed this panel emits no bold ANSI even on main, so the flattening of
nested [bold] inside [dim] is pre-existing and not introduced here.

Addresses the non-blocking finding from the third review on PR #5.
@throup

throup commented Sep 3, 2026

Copy link
Copy Markdown
Author

Fourth review — head 2b00340

2b00340 is sound, and the three prior rounds' conclusions still hold at this head. Nothing blocking.

The gate is the right one: both _display_dialog_start (dialog.py:647) and the reader branch (dialog.py:796) read sys.stdin.isatty() through the same module-global sys, so they cannot disagree. The reader's condition is prompt_text is None and sys.stdin.isatty(), and the main turn (dialog.py:284) passes no prompt_text, so for the turn the banner describes it reduces to exactly the banner's condition. The Continue? prompt (dialog.py:363) does pass prompt_text and so always takes Prompt.ask, but the banner makes no claim about it.

The styled() reasoning in the commit message checks out. Interpolating the pre-marked-up fragment as a value does render a literal [bold] — confirmed by running it — so keeping the markup in the template and passing only the sentinel as a value is correct. Rendered output contains a real bold span over /send, not literal brackets.

Non-blocking

Nothing worth changing. Two observations, neither a defect:

  • if True:-style asymmetry is avoided cleanly, but the hint_args: tuple[object, ...] annotation sits on the first branch's assignment while the else branch assigns unannotated. That is idiomatic and typechecks fine; noting only that the annotation's placement is load-bearing for the empty-tuple case.
  • The template is assembled with + around the conditional fragment. It is correct in both branches (verified below), but it is the kind of join that a future edit to the adjacent literals can silently break, since the trailing " " lives on the concatenated segment rather than inside the fragment.
Evidence

Testsuv run --frozen pytest tests/test_gates tests/test_cli/test_markup_injection.py -q117 passed in 1.27s.

Lintmake lintAll checks passed!, 423 files already formatted.

Typecheckmake typecheck reports exactly the 2 pre-existing claude_agent_sdk.py call-non-callable diagnostics (also on main); nothing from the new code.

The parametrised test constrains both directions. Mutated in an isolated git archive copy (asserted conductor.gates.dialog.__file__ pointed into the mutated tree); PR worktree left clean.

Mutation Result
(a) hint made unconditional ...[False] FAILS('/send' in rendered) is False
(b) hint removed entirely ...[True] FAILS('/send' in rendered) is True

Both fail in the correct direction, so neither over- nor under-advertising can regress unnoticed.

Rendering, both branches at widths 40/60/76/100/300. No literal [bold] at any width in either branch. Plain text and spans:

isatty=True   "Agent 't' would like to discuss its output with you.\n
               Type your response below. It can span multiple lines;
               send it with /send on its own line. Say done or /done when finished."
   spans: (0,9,bold) (53,175,dim) (120,125,bold)->/send (147,151,bold) (155,160,bold)

isatty=False  "Agent 't' would like to discuss its output with you.\n
               Type your response below. Say done or /done when finished."
   spans: (0,9,bold) (53,111,dim) (83,87,bold) (91,96,bold)

No doubled or missing space at the join in either branch (" " in plainFalse both times). The tty branch reads below. It can span…own line. Say done; the non-tty branch reads below. Say done.

The f-string alternative really would have broken. Value-interpolating the marked-up fragment gives:
'Type your response below. send it with [bold]/send[/bold] on its own line.' — literal brackets present, confirming the template-side markup is the correct choice.

Prior rounds undisturbed. 2b00340 touches only _display_dialog_start and its one test (16/3 and 13/4 lines). ef9f417's Ctrl-D/Ctrl-C work is present verbatim at HEAD (except (EOFError, KeyboardInterrupt) ×2, if hit_eof and not text: ×1, read_on_daemon_thread ×2 — same counts as at ef9f417). No test present at ef9f417 is missing at HEAD; the only test-name delta is the rename of the banner test to test_opening_banner_advertises_the_sentinel_only_on_a_tty.

Cumulative branch (git diff origin/main...HEAD) — 4 files, +304/-24: gates/dialog.py, gates/human.py and their two test files. Nothing outside the dialog/human gate surface.

sys is imported at dialog.py:14.

🤖 Generated with Claude Code

@throup

throup commented Sep 7, 2026

Copy link
Copy Markdown
Author

On hold — proposed upstream instead

This repository is a fork of microsoft/conductor, and this defect is in upstream code rather than anything we added. Rather than carry a permanent fork-local patch, the fix has been raised upstream:

This PR stays open and unmerged until that is resolved.

  • Upstream accepts → take the fix via an upstream sync and close this PR without merging.
  • Upstream rejects, or stalls → merge this PR and maintain the fix as a fork-local patch.

The upstream version is not byte-identical to this branch

Rebased onto upstream main (ff4b312), with four differences that matter if we end up reconciling the two:

  1. Adds import sys to dialog.py. This branch inherited that import from the PDA-13 dialog work, which upstream does not have — without it the patch is a NameError there. This was the one genuine portability defect.
  2. The dialog regression tests are restructured. Ours anchor on a PDA-13-era test that does not exist upstream, so the patch would not apply.
  3. Internal references removed — AC numbering in docstrings, and "grill".
  4. Adds CHANGELOG.md, docs/workflow-syntax.md and AGENTS.md entries this branch does not have.

One claim in the ticket does not hold upstream

PDA-35's AC(4) says exhausting a paste ends the dialog. Verified against upstream main by driving the real DialogHandler: the empty trailing line becomes an additional turn dispatched to the model, not a dismissal — "" is not in DISMISS_KEYWORDS. That premature-exit symptom comes from this fork's PDA-13 changes, which accept dismiss keywords at more points.

So AC(4) is a valid requirement for our fork and not a description of the upstream defect. Issue microsoft#509 describes the measured behaviour instead.

Also worth noting

The /send sentinel is flagged upstream as an unsettled decision with three alternatives, since . already terminates multi-paragraph free text in questions without complaint. If a maintainer prefers . for both surfaces, the sentinel is a single constant and this branch would need the same change before merging.

Verified against upstream: tests/test_gates 69 passed; full suite 8308 passed, with three failures reproducing identically on a clean checkout (chmod 0o000 and case-sensitivity tests that do not hold on macOS APFS).

🤖 Generated with Claude Code

@throup throup changed the title PDA-35: Fix: discover interactive input treats each newline (including pasted multi-line text) as a separate prompt [ON HOLD — upstreamed as microsoft/conductor#510] PDA-35: Fix: discover interactive input treats each newline (including pasted multi-line text) as a separate prompt Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants