fix(ios): report an unobserved text commit instead of a partial success - #1924
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Adversarial review pass (Fable). No blocker, but two deterministic false-failure windows in my own wait, one overclaim in the PR framing, and an untested branch. All addressed in Fixed1. The deadline was checked before observing. A commit landing during the final poll sleep was condemned as 2. Placeholder collision. 3. The refusal branch was untested. The whole fix was one ternary that no test reached. It is now 4. Message asserted a field state never read. Under both windows above the field may hold the complete value, so "committed only part" was wrong. Now "could not confirm the typed text committed before its deadline". Corrected framing — this does not catch #1844's signaturesI overclaimed. Both #1844 values classify as
Only the strict-prefix stall (#1874's signature) becomes a failure. #1844's lost/reordered prefix is a different mechanism and this PR does not address it. The docs sentence has been narrowed to match, and no longer implies every text-entry route verifies its result. Known, unchanged
Refuted — checked and sound
|
|
[P1] Make the placeholder regression cover the production normalization boundary. |
|
[P1] addressed in Confirmed first. The fix lived at Now. The raw-exact/normalized-prefix choice is XCTAssertEqual(
Self.commitObservation(rawValue: "0.00", expectedText: "0.00", normalizedValue: { "" }),
"0.00")Red-before proved by running both implementations against the assertion set: Three more cases pin the other direction, so the helper cannot be "fixed" by always returning the raw value: a non-exact match keeps the normalized reading ( One deviation from your suggestion. I did not take the fixture-element option — it would put this on the simulator lane, where the host lane now covers it in 12ms per PR. The old test is deleted rather than kept alongside: its only other assertion (an exact match settles without polling) is already
|
|
P1: Do not use placeholder equality as commit evidence.
Please keep this state indeterminate/non-success (or obtain a signal that distinguishes placeholder rendering from a committed value) and add a regression where the raw placeholder already equals the requested text before any commit; it must not report success. |
40c70f5 to
7cedfc5
Compare
|
P1 addressed in You are right, and the direction matters: my fix for a false failure created a false success, which is strictly worse and is the exact defect this PR exists to remove. Confirmed the mechanism before changing anything. Empty field, placeholder No disambiguating signal exists.
The refusal is now up front, not after the deadline. Since no read can resolve the ambiguity, spending 3 seconds discovering that is pure latency. Regression, as you asked — raw placeholder already equals the requested text before any commit, must not report success: XCTAssertTrue(
Self.placeholderMakesCommitUnobservable(placeholder: "0.00", expectedText: "0.00"))
XCTAssertEqual(
Self.textEntryFailure(forCommitOutcome: .notObserved), .commitNotObserved)Red-before proved against the previous revision on the zero-characters-committed scenario: A second test keeps the guard narrow — Two knock-on corrections. The failure message no longer says "before its deadline" (this refusal never waits), and the docs now name both routes to the code — deadline expiry, and placeholder-identical text. The eager-vs-lazy question from my last comment is moot — the helper it applied to is gone. Rebased onto |
|
[P1] Keep the placeholder guard scoped to an empty baseline. |
|
P1 addressed in Right again — I had made the guard baseline-blind, so it refused the very case where the read is trustworthy. Value The baseline is the deciding input, so it is now an input.
Only
Both directions proved, so neither P1 regresses the other: The regression you asked for reaches the observation path rather than asserting on the classifier alone:
Docs updated to name the empty-field condition rather than implying any placeholder-identical text refuses.
|
|
[P1] A non-empty baseline is not post-dispatch commit evidence. |
awaitSynthesizedFirstResponderCommit returned Void, so its three exits were indistinguishable to the caller: the expected text committed, the app transformed the input, or the 3s deadline expired with a strict prefix still outstanding. The caller returned dispatched-with-no-failure in all three, and `type` answered ok with textEntryRoute synthesized-first-responder over a field holding part of the requested text. The wait now returns a SynthesizedTextCommitOutcome and an expired deadline becomes TEXT_INPUT_COMMIT_NOT_OBSERVED, whose hint points at fill rather than a type retry — type appends, so retrying it concatenates onto whatever committed. The tail is still not re-synthesized: #1676 rejected that because a stalled prefix cannot be told apart from a suffix still queued, so repair double-posts. Reporting is what the runner does instead. typeIntoCurrentTarget loses its `dispatched` flag, which was exactly `failure == nil` and could not express the new state — characters posted, commit unconfirmed, command must refuse. Failure is now the single discriminator. The decision moves behind an injected clock/observer so the deadline branch runs in the macOS host lane on every PR instead of needing a simulator. Refs #1874, #1844
Adversarial review found two deterministic false failures in the wait added by the previous commit, plus a message that asserted a field state never read. The deadline was checked before observing, so a commit landing during the final poll sleep was condemned as never observed — under exactly the loaded-host timing the wait exists for. The check now runs after an observation, so the last thing before condemning is a read. `treatingPlaceholderAsEmpty` maps a value equal to the field's placeholder to "", a prefix of every expected value. `type "0.00"` into a field placeheld "0.00" committed instantly, read as pending for the full 3s, and failed. The observation now settles on an exact raw match; the normalized read still drives the prefix walk. The outcome-to-failure mapping moves to textEntryFailure(forCommitOutcome:) so the branch the command refuses on is pinned by a test rather than living only in a ternary. `.unobservable` staying a success is what keeps `type "...\n"` working, and it now has an assertion. Message and hint no longer claim the field holds a partial value: under both fixed windows it may hold all of it. The docs sentence no longer implies every text-entry route verifies its result — the replacement and keyboard-visible routes have no resolvable element to observe and are unchanged. Refs #1874, #1844
Review [P1]: testValueEqualToThePlaceholder… injected an observe closure that already returned "0.00", so it never supplied the normalized "" that causes the failure. The raw-value short-circuit lived in the production observe closure, which that test bypassed entirely — reverting the fix left it green. The raw-exact/normalized-prefix choice moves into commitObservation, and the test drives it with (raw: "0.00", normalized: "", expected: "0.00"). Reverting commitObservation to always return the normalized reading now fails the exact-match assertion. normalizedValue is a closure rather than a value so an exact match still costs one accessibility read instead of two, on a path that polls every 20ms for up to three seconds; a second test pins that laziness. The old test is deleted rather than kept: its remaining assertion (an exact match settles without polling) is already covered by testSynthesizedCommitStopsAtTheFirstSettledObservation. Refs #1874
Review [P1]: an empty text field renders its placeholder AS its accessibility value, which is why editableTextValue(treatingPlaceholderAsEmpty:) classifies that value as empty. The previous revision's raw-exact short-circuit therefore matched BEFORE anything committed whenever the requested text was the placeholder: `type "0.00"` into a field placeheld "0.00" settled on the first read and returned ok with zero characters delivered — reintroducing the success-misdescribes-the-device failure this PR exists to remove. The state is structurally indeterminate. element.value is identical whether the placeholder is rendering or the committed text happens to equal it, and placeholderValue does not disambiguate, so no read resolves it and waiting the deadline out discovers nothing. placeholderMakesCommitUnobservable detects it up front and reports the commit unobserved, which the caller refuses on. commitObservation is deleted rather than narrowed: the raw match was only ever consulted in this exact case, and in this exact case it is not evidence. The failure message drops its deadline reference — this refusal never waits. Refs #1874
Review [P1]: the guard took only the placeholder and the expected text, so it
refused any append whose result happened to equal the placeholder. Value "0" +
`type ".00"` against placeholder "0.00" was refused before a single read, even
though the non-empty pre-dispatch value proves the placeholder is not what is
rendering and a later "0.00" is genuine commit evidence.
The baseline is what decides it, so it is now an input. placeholderCommitEvidence
returns three states rather than a boolean:
normalRead expected differs from the placeholder; the placeholder never
enters into the observation
indistinguishable expected IS the placeholder and the field was empty, so the
placeholder was what rendered and no read can resolve it
rawValueIsEvidence expected IS the placeholder but the field held content, so a
raw match is real
Only .indistinguishable refuses, and it still refuses before the wait, since no
read resolves it. .rawValueIsEvidence reaches the observation and settles on the
raw match, which the normalized read would otherwise hide.
commitObservation returns for that third state, now scoped by evidence rather
than applied unconditionally as in the revision that made raw equality a
false success. Both readings stay closures, so normalRead — the ordinary case —
never pays for the raw read.
Refs #1874
afe99bd to
12612b5
Compare
|
P1 addressed in |
|
Re-reviewed exact head a584102: the clear-after-dispatch P1 is resolved. Placeholder-equal expected values are now conservatively indeterminate before polling, and the new regression proves the old non-empty-baseline/raw-match path would report success over an empty field. The obsolete evidence/raw-read branch was removed and the docs match the conservative contract. Completed checks are green; one iOS Smoke job is still running. |
thymikee
added
the
ready-for-human
|
Exact head a584102 is now fully green (30/30) and mergeable/CLEAN. The conservative placeholder-equality fix and its regression remain review-clean. Merge-ready. |
1 participant
Refs #1874, #1844.
Summary
Stops the iOS synthesized bare-
typeroute from reporting success when the app has not observably committed all requested text.TEXT_INPUT_COMMIT_NOT_OBSERVEDwith recovery guidance to read the field and preferfillover appending anothertype..unobservableremains non-failing).This fixes #1874's strict-prefix signature (
"h"remaining from"hardware-keyboard"). It does not claim to fix #1844's reordered/non-prefix text, which follows the existing transformed-input path. The sibling synthesized replacement and keyboard-visible routes remain outside this observation mechanism because they do not retain a resolvable element.Validation
nilvsTEXT_INPUT_COMMIT_NOT_OBSERVED).TEXT_INPUT_COMMIT_NOT_OBSERVEDand one forbidden post-dispatch observation.TEST BUILD SUCCEEDED;pnpm check:xctest-selectionreports 185 declared tests and 0 unreachable.pnpm check:affected --runpassed ata58410266; GitHub-authoritative Swift, coverage, and device lanes are running on that exact head.Docs were updated because this changes when
TEXT_INPUT_COMMIT_NOT_OBSERVEDis returned and how callers should recover. No skill guidance changed; the command reference owns this behavior.