Fix ui send-keys --via send-input silently dropping characters on long text (#657)#667
Conversation
`ui send-keys --via send-input` injected the entire payload in one SendInput call. Long text (e.g. 1000+ chars) overran the target thread's input queue, which silently drops events even though SendInput returns the full count — so the command reported success while only part of the text landed. Split the payload into small self-contained segments (each chord stays atomic; literal text is chunked) and inject one SendInput per segment, pausing briefly between segments so the target can drain its queue. Sleeping only *between* segments means short inputs (a chord or a word) add zero latency. Each segment is balanced, so a short write can only strand its own keys — release just that segment and surface the same precise error. Defaults (16 chars / 15 ms) were tuned empirically: un-throttled dropped 3000→2500 chars on a deterministic Win32 EDIT, while throttled delivered 3000/3000; real Notepad lands 1000/1000. No new CLI flags — throttling is fully automatic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds chunked, throttled SendInput delivery to prevent long text payloads from dropping characters.
Changes:
- Splits text into paced segments while keeping chords atomic.
- Adds segmentation, throttling, and failure-path tests.
- Documents automatic throttling and regenerates skill files.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
KeyboardInput.cs |
Implements segmented, throttled input. |
KeyboardInputTests.cs |
Tests chunking and failure handling. |
ui-automation.md |
Documents throttling behavior. |
.github/.../SKILL.md |
Regenerates Copilot skill documentation. |
.claude/.../SKILL.md |
Synchronizes Claude skill documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Build Metrics ReportBinary Sizes
Test Results✅ 3582 passed, 4 skipped out of 3586 tests in 561.4s (+27 tests, -65.3s vs. baseline) Test Coverage✅ 95.2% line coverage, 89.3% branch coverage · ✅ no change vs. baseline CLI Startup Time47ms median (x64, Updated 2026-07-17 18:11:30 UTC · commit |
zateutsch
left a comment
zateutsch
left a comment
There was a problem hiding this comment.
M1 might be worth a follow up.
PR Review — nmetulev-fix-send-input-drops-characters vs origin/main (1 commit, 5 files, +248/-31 lines)
Summary
Critical: 0 High: 0 Medium: 2 Low: 1
Coverage
security ⚠ 1 finding
correctness ⚠ 1 finding
cli-ux ⚠ 1 finding (merged into M2)
alternative-solution ⚠ 1 finding
necessity-simplicity n/a (no new surface — internal bug fix)
test-coverage ✓ clean
docs-and-samples ✓ clean
packaging ✓ clean
multi-model ⚠ 1 new (High, disputed) · 4/4 specialist findings reconciled · model: gemini-3.1-pro (self-reported "gpt")
regression ⚠ 1 behavior change (loss of single-call atomicity — deliberate tradeoff)
validation ✓ build clean (0 warnings) · 19/19 KeyboardInput tests pass · L1 & message-logic confirmed statically; wrong-window delivery static-only
Findings
M1 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:201-232 security,correctness Paced multi-segment injection drops SendInput atomicity; foreground checked once → focus steal or physical input mid-stream can misdirect/corrupt remaining text
M2 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:224-249 correctness,cli-ux Failure after earlier chunks reports "refused"/"retry" without noting input was already partially applied → misleading, can duplicate text
L1 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:300-321 alternative-solution BuildSendInputBatch is now production-dead, retained only for its own tests
Details
M1 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:201-232
- Severity: medium
- Confidence: medium
- Validation: static-only (needs runtime confirmation) — single foreground check confirmed in code; wrong-window delivery not reproduced
- Domain: security, correctness
- Multi-model: upgrade (Gemini cross-check argues High)
- Finding: The fix trades one atomic SendInput for N paced SendInput calls with 15 ms sleeps between them. Windows guarantees no physical input interleaves within one SendInput call, but not across calls. During a long payload (~1000 chars/sec) a focus steal (notification, popup, accidental click) or a physical keypress can land remaining chunks in the wrong window or corrupt the stream (e.g. a real Shift/Ctrl held mid-stream).
UiSendKeysCommandvalidates the target foreground only once (UiSendKeysCommand.cs:206) beforeKeyboardInput.Send. - Evidence:
SendViaSendInputloopss_sendInput(segment)thens_sleep(s_chunkDelayMs)between segments (KeyboardInput.cs:217-231) with no re-check of the foreground window;foregroundGuard.TryEnsureForeground(...)is called exactly once at UiSendKeysCommand.cs:206. - Note: This is a deliberate tradeoff — the prior single burst silently dropped characters (#657), which is worse. The exposure is real but low-probability.
- Recommendation: Re-assert the target is still foreground before each post-sleep segment (abort with the existing
foreground_not_targeterror on change). That closes the sensitive-text-to-wrong-window case at low cost.
M2 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:224-249
- Severity: medium
- Confidence: high
- Validation: validated (message logic confirmed by reading
BuildSendInputFailure); user-visible duplication is static-only - Domain: correctness, cli-ux
- Multi-model: confirmed
- Finding: On a failure in a later segment,
BuildSendInputFailure(sent, segment.Length)only sees the current segment. A zero-write after earlier chunks succeeded emits "SendInput failed — no interactive desktop / UIPI…" (implying nothing was typed), and the short-write path says "retry the gesture" — yet earlier chunks already landed, so a literal retry duplicates text (e.g. "passpass…word"). - Evidence: The throw at KeyboardInput.cs:227 has no knowledge of
i > 0;BuildSendInputFailure(238-249) branches solely on the current segment'ssent. - Recommendation: Track whether any prior segment was delivered; when
i > 0, prepend "some input was already applied — verify/clear the target before retrying" to both the zero- and short-write messages.
L1 src/winapp-CLI/WinApp.Cli/Helpers/KeyboardInput.cs:300-321
- Severity: low
- Confidence: high
- Validation: validated — grep confirms no production caller
- Domain: alternative-solution
- Multi-model: confirmed
- Finding:
BuildSendInputBatchis no longer called in production (SendViaSendInputusesBuildSendInputSegments); only twoKeyboardInputTestsmethods and one doc-cref reference it. It's dead production code kept alive by tests that only test itself. - Evidence:
rg BuildSendInputBatch→ definition + XML-doc cref + KeyboardInputTests.cs:43,67 only. Build produced 0 warnings, so the compiler doesn't flag it. - Recommendation: Remove
BuildSendInputBatchand port its two tests to assert over the flattened output ofBuildSendInputSegments(a single-chord input yields one segment; text yields chunk segments).
Multi-model independent finding (disputed)
Gemini raised a High: "loss of SendInput atomicity allows physical input interleaving." Root cause is the same as M1. I keep it at Medium, not High: the pre-fix single-call path already lost characters to queue overrun, so the fix is a net improvement, and the interleave window requires the user to actively type/steal focus during automation. Surfaced under M1; the per-segment foreground re-check is the concrete mitigation. Note: the cross-check self-reported "gpt (gpt-4o)" but was dispatched on gemini-3.1-pro-preview — model self-reports are unreliable.
Coverage notes
test-coverage: New tests map to every new branch (chunk split, sleep-between-only, chord atomicity, short-write releases only the stranded segment, short-text no-sleep). sent==0 path covered by pre-existing Send_SendInput_ZeroWriteReportsPreciseReason. Reset restores s_textChunkChars/s_chunkDelayMs. Only untested gap: empty-string TextInput (harmless). Clean.
docs-and-samples: The one throttling bullet is byte-identical across the hand-written fragment and both auto-generated SKILL.md mirrors; docs/ui-automation.md, usage.md, winapp.agent.md already cover send-input + set-value guidance. No schema change (no Commands/ edit). Clean.
packaging: No Commands/, npm wrapper, csproj, NuGet, manifest, or version.json change; generated skills in sync; no Release warnings. Clean.
validation: Built src\winapp-CLI\WinApp.Cli Debug → succeeded, 0 warnings. Ran WinApp.Cli.Tests → 19/19 pass. Could not stage a runtime focus-steal to reproduce M1's wrong-window delivery (needs a second app + timed focus change).
…#657) Follow-up to PR #667 (pr-review). Addresses one HIGH plus four polish items on the issue #657 chunked/throttled send-input fix: - [H1] Re-verify the target owns the foreground (GA_ROOT) before EACH paced SendInput segment and abort on drift, so a focus change mid-injection cannot spray the remaining keystrokes into whatever window took focus. Surfaced as ForegroundLostException and mapped by the command to the existing foreground_not_target error code. No releases are injected on drift: every completed segment is self-balanced (nothing held), and any SendInput after drift would land in the wrong window -- the exact hazard being prevented. The short-write path still releases its one in-flight segment. - [M1] Emit an informational warning when a send-input payload exceeds one chunk, noting the throttling is intentional and that 'ui set-value' is faster for bulk text. - [L1] Consolidate the test-only BuildSendInputBatch onto BuildSendInputSegments so one builder owns the action->INPUT contract. - [M2] Pin the shipping throttle defaults (16 chars / 15 ms) behaviorally so a future default change cannot silently reintroduce #657. - [M3] Document the per-chunk foreground re-check and auto-throttle behavior in docs/ui-automation.md. Tests: +3 helper tests (focus-drift abort, held-throughout happy path, default pinning) and +3 command tests (foreground_not_target mapping, throttle warning, warning boundary). Full suite green (3329 passed, 0 failed). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address two copilot-pull-request-reviewer threads on PR #667: the inter-segment pacing delay and the mid-injection foreground re-check were applied between EVERY SendInput segment, so a short chord sequence like `ctrl+a delete` gained an inter-chord pause and a chord that intentionally moves focus (alt+tab, win+d) could be misread as drift and abort the send. Tag each SendInput segment with ThrottleBefore and pace/re-check only the 2nd..Nth chunk of a single split literal-text run -- the sole place the issue #657 queue overrun occurs. Chords and the first chunk of any text run now inject immediately: no added latency for short input, and a focus-changing chord is never treated as foreground drift. The H1 wrong-window guard is preserved for the long-text scenario it targets, and the sleep->recheck->inject ordering tightens the TOCTOU window. - KeyboardInput: add SendInputSegment(Events, ThrottleBefore); gate delay and foreground re-check on ThrottleBefore; BuildSendInputBatch flattens .Events. - Tests: assert ThrottleBefore tagging; chord->text boundary adds no sleep; drift test re-checks only continuation chunks. - Docs: fragment + ui-automation.md clarify pacing is scoped to one long run (chord sequences add no delay) and note the per-continuation-chunk foreground_not_target abort with the chord exemption. Regenerated plugin + Claude ui-automation skills. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address zateutsch's PR #667 review: after send-input was split into paced chunks, a failure in a LATER segment built its message only from the current segment. A zero write after earlier chunks already landed emitted the "no interactive desktop / UIPI" wording (implying nothing was typed), and the short-write path said "retry the gesture" — but a literal retry re-types the text that already went in (e.g. a password becomes "passpass...word"). Track whether any earlier segment was delivered and thread it into BuildSendInputFailure. When prior input landed, lead both the zero-write and short-write messages with a caveat to verify or clear the target before retrying, and drop the bare "retry the gesture" hint that would invite duplication. Injection path is unchanged — this is a message/UX correctness fix. Tests: extend the later-chunk short-write test to assert the caveat and the dropped retry hint; add a zero-write-after-earlier-chunk test; pin the scoping with negative assertions on first-segment failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolves conflicts after #664/#665/#666/#668 merged to main. - KeyboardInput.cs: keep #657 chunked/throttled SendViaSendInput + per-segment foreground re-check + partial-apply messaging, and apply #664 NormalizeNewlines to the chunking path (normalize once, then chunk). - KeyboardInputTests.cs: keep both #657 segment/throttle/partial-apply tests and #664 newline tests. - UiSendKeysCommand.cs: keep #657 throttle warning; deliver via #666 effectiveHwnd (post-message focused-child retarget; equals target for send-input). - ui-automation skill fragment + generated SKILL.md (.github + .claude): keep #666 focused-child/windowless post-message note and the #657 auto-throttle / foreground-recheck / set-value-for-bulk note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Problem
winapp ui send-keys --via send-inputbuilt oneSendInputarray for the entire payload and injected it in a single call. Long text (e.g."X" * 1000) expands to thousands of key events; injected all at once it overruns the target thread's input queue, which silently drops the excess. BecauseSendInputstill returns the full count, the command reported success (✅ Sent 1 key action(s), exit 0) while only part of the text landed (issue #657 reported ~434 of 1000).Fix (fully automatic — no new CLI flags)
Chunk + throttle the
send-inputinjection:SendInputper segment, with a briefThread.Sleepbetween segments so the target can pump and drain its input queue before the next burst.Defaults (
16chars /15ms) were tuned empirically and chosen for a comfortable queue-drain margin that's robust across both coarse (~15.6 ms) and fine (~1 ms) Windows timer resolutions.Verification
3000 → 2500chars (4/4 runs); throttled delivered 3000/3000 (8/8 runs).KeyboardInputTests.cscover segmentation, chord atomicity, sleep-between-not-after, no-latency-for-short-text, and per-segment release on short write. Existing keyboard tests unchanged and green (19/19). Full suite passes.cli-schema.json(no new surface); docs skill fragment updated and regenerated.Docs
Added a note to the
ui-automationskill fragment that longsend-inputtext is auto-throttled for reliable delivery, and thatset-valueremains preferred for large bulk text (regenerated.github/plugin+.claudeskills follow).Closes #657.