Skip to content

fix(ios): stop fill from repairing fields that normalize their input - #3334

Open
thymikee wants to merge 3 commits into
mainfrom
fix/fill-normalizing-fields-2634
Open

thymikee wants to merge 3 commits into
mainfrom
fix/fill-normalizing-fields-2634

Conversation

@thymikee

@thymikee thymikee commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Summary

fill into a self-reformatting field (grouping, phone mask, currency) failed TEXT_ENTRY_MISMATCH after the destructive clear-and-retype ran on an already-correct field.

Owning bug: textEntryValueEchoes' containment clauses, not the repair gate — the formatted value contains the request in order, so the classifier claimed it and the fill fell through to repair. Fix: a value the request plus inserted formatting EXPLAINS (request in order; every unconsumed character neither a request nor a post-clear baseline character; one insertion strictly between the request's ends) stops being an echo and reaches the EXISTING unconfirmed path. Baseline is a parameter of the deciding predicate, so failed-clear residual (old123456) never counts as an insertion wherever a mask relocates it.

Deliberately flipped pin: a complete mask value lost nothing; the outcome is unconfirmed with before/after evidence, never verified: true (completion ≠ correctness: $10.00 for 1000). Repair gate, synthesized-commit wait, and adxe (#2903) untouched; no wire-shape change. Closes #2634

Validation

Head bbc93e366, iPhone 18 Pro / iOS 27.0:

  • Fail-before/pass-after on-device: fix reverted → reported error + REPAIR log; with fix → TEXT_ENTRY_UNCONFIRMED repaired=0, field keeps 00 062 91 77.
  • Residual case ("12"/"5678"/"12 567 8") run RED against cb24ef6, green here, pinned with the absorbed-residual positive; never looser than main.
  • Template-retaining mask ("$0.00" → "$10.00") pinned as known limit; docs/help name the boundary.
  • Classifier/policy and device runs green at head.
  • pnpm check:affected --run on head: all passed; xctest-selection 0 unreachable; packaged-runner-swift, build, format/lint/typecheck pass. Swift-only, iOS-guarded; no manual RN test-app run.

Cubic review

  • Reviewer finding (residual inside the completed span): real regression from the first exemption; fixed by folding the baseline into the deciding predicate — "request plus inserted formatting explains the whole value" in one place. Comments trimmed per the non-blocking note.
  • Second round (non-blocking, both taken): template-retaining-mask overclaim pinned as a known limit (indistinguishable from residual by text; unfixable forward), docs/help name the boundary; orphaned comment cut.
  • Stale-prefix contradiction: obsolete at head. Caret fidelity and device-test duplication: fixed. All four P3 threads re-verified at head with git show | grep -c evidence in-thread.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3334/

Built to branch gh-pages at 2026-10-09 03:03 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.13 MB 5.13 MB +5.9 kB
Package (unpacked) 5.13 MB 5.13 MB +5.9 kB
Package (download) 1.54 MB 1.55 MB +1.8 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.5 ms 26.7 ms +0.2 ms
CLI --help 81.1 ms 80.0 ms -1.1 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread apple/runner/AgentDeviceRunner/AgentDeviceRunner/AgentDeviceRunnerApp.m Outdated
Comment thread website/docs/docs/commands.md Outdated
Comment thread website/docs/docs/commands.md Outdated
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

The fill change in cb24ef6 is close, but it has one gap: a non-empty field baseline can now pass as a completed fill. CI is green, with all 21 checks passing at this head.

The early return for completed values in RunnerTests+TextEntryConfirmation.swift:66 runs before the isOrderedSubsequence(residualAndRequest, of: observed) residual check, and it ignores the baseline. The baseline is sampled after clearTextInput, so a partial clear can leave old text in the field. Take baseline "12", request "5678", and a field that shows "12 567 8". The grouping spaces count as inserted formatting, so textValueCompletesRequest returns true and unconfirmedTextEntryEvidence returns evidence. On main, the residual check classed this as an echo and the clear-and-retype repair ran. Now fill reports ok with verification "unconfirmed" and skips repair, while the field holds "125678" instead of "5678". That contradicts the new help and docs text that old text left behind by a failed clear fails. The stale-frame partial clear that #2634 describes can reach this. The rule the code must satisfy is that a value counts as completed only when the request plus inserted formatting explains the whole value, and text already in the field after the clear must never sit inside the completed span. The smallest fix is to run the residual check before textValueCompletesRequest when the baseline is non-empty, or to offer the completion exemption only when the post-clear baseline is empty. Please add an unconfirmedTextEntryEvidence case with baseline "12", request "5678", and observed "12 567 8" that expects nil. Every new evidence case uses baseline "", so none covers this, and I expect that case to fail at this head.

Not blocking: several new comments run 8 to 14 lines and narrate design rationale and review history, and AGENTS.md asks to keep that out of implementation comments, so one sentence stating the invariant would do; take or leave this.

On the open threads: the four cubic-dev-ai P3 threads are all fixed at this head, so you can resolve them: fixture caret restore, shared test helper, carriesRequest clause removed, and commands.md wording. The gap in the third thread's analysis is a different mechanism and is the finding above.

I did not run the XCTest suite or the device test revert, and I judged the regression by reading the main-branch classifier. I did not reproduce the finding on a device; it follows from the code and depends on clearTextInput leaving old text in a normalizing field. The issue names the examples/test-app phone field, while the PR uses a runner host-app fixture, and I accepted that as equivalent evidence without a test-app run. Before merge, the completion check needs to respect the post-clear baseline, with the "12" / "5678" / "12 567 8" case added.

…their input (#2634)

The bug was textEntryValueEchoes' containment clauses, not the repair gate.
A field that reformats what it holds (a '## ### ## ##' digit grouping, a
phone mask, a currency format) shows the request with the formatter's
separators inserted; clauses 2/3 classify that as an echo, which withheld
the unconfirmed outcome, so the fill fell to mismatch -> destructive
clear-and-retype of a correct field -> TEXT_ENTRY_MISMATCH. Measured on
iPhone 18 Pro / iOS 27.0 against a digit-grouping fixture:
REPAIR_TEXT_ENTRY expectedLength=9 observedLength=12, then the mismatch.

A value that COMPLETES the request now falls through to the existing
unconfirmed-evidence path: every request character in order, plus inserted
characters strictly between the request's own characters that appear
nowhere in the request. Degradation removes; entry cannot insert a
separator between two characters the same burst typed, and a failed
clear leaves residual text at the ENDS of the typed run — so end-only
insertions ('old123456') stay echoes and keep their repair, while an
interior foreign insertion does not. Leftmost embedding and a
no-request-character-extras guard keep doubled entries and ambiguous
embeddings on the failure side.

This deliberately flips the pin that '(555) 123-4567' IS an echo: 'echo'
means 'could be a degraded copy', and a complete mask value is not one.
Completion is never a correctness claim — a cents-shifting mask passes it
— so the outcome is unconfirmed with before/after evidence, never
verified: true; only exact equality verifies. fill-evidence.ts already
names app-owned formatting as this shape's purpose; Android already
reports it unconfirmed; iOS was the outlier.

isRepairableTextEntryMismatch and the synthesized-replacement commit wait
are UNCHANGED; the coordinate route's exact-match-only settle is pinned as
an explicit non-goal. The write-back-truncation shape (ada@example ->
adxe) stays a typed failure under #2903's ownership, and so do drops,
stale residuals, and doubled entries.

Fixture: AgentDeviceDigitGroupingTextField reproduces the reported
formatter and restores the caret across reformats the way the real
formatter it models does; both normalizing device tests share one
fill-and-assert helper so their plumbing cannot drift. The device test
proves the repair no longer runs (message 'typed', not 'typed after
repair') and the field keeps 00 062 91 77.
@thymikee
thymikee force-pushed the fix/fill-normalizing-fields-2634 branch from cb24ef6 to 9ef444a Compare October 8, 2026 23:16
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Fixed in 9ef444a. Your arithmetic was right and I reproduced it before touching anything: at cb24ef6 the new case unconfirmedTextEntryEvidence(requested: "5678", baseline: "12", observed: "12 567 8") returned evidence instead of nil — the grouping spaces are real insertions, so textValueCompletesRequest granted the exemption while the field held residual "12" inside the value. Pushed as a red test first (it fails at cb24ef6, verified by running it against that head), then the fix.

Rather than ordering two checks around each other, the baseline is now a parameter of the deciding predicate itself: textValueCompletesRequest(observed:request:baseline:) refuses completion when any character the request's embedding did not consume is explainable by the baseline — the same shape as the existing foreign-character guard, so "residual may never stand inside the completed span" is decided in one place and cannot be asked without the fact it needs. Your case now dies at the first extra '1' (baseline '12'), falls through to the residual clause, and gets its repair like it does on main. The end-position argument from the old comment is gone; the doc now states the invariant instead of trusting where a mask parks residual text.

Chose the character-level guard over the empty-baseline-only gate deliberately: post-clear baselines are legitimately non-empty for summary fields, and when the value is fully explained by request + foreign insertions (e.g. baseline "5", observed "5 678" for "5678") the fill did work and disclosing is honest — the guard only refuses when a baseline character actually stands in the value unaccounted for. This is never looser than main: every refusal falls back to main's echo reading.

Pinned in the classifier tables: the refusal ("12" + "5678" → "12 567 8" → not a completion), its closest positive ("12" + "5678" → "5 678 12"... actually the empty-baseline positive "00 062 91 77" plus the absorbed-residual positive baseline "12" → "(555) 123-4567" for "5551234567", where the residual is consumed by the embedding and the extras are pure punctuation), so the rule is decided by the extras' provenance, not baseline emptiness.

On the comments: trimmed — the echo predicate, the completion predicate, the flipped pin, and the evidence test now carry one sentence per constraint; the narrative lives in this PR body instead. The four P3 threads: confirmed each fixed at this head myself before resolving (caret restore present in AgentDeviceRunnerApp.m, shared helper defined+used, CarriesRequest grep 0, commands.md wording 1 hit) and replied in-thread with the counts.

Validation at 9ef444a: red test fails at cb24ef6 / passes here; 19-test device run (11 iOS-lane selection incl. the grouping-field device test + 8 classifier/policy) all green; check:xctest-selection 0 unreachable; packaged-runner-swift ok; format/lint/typecheck clean. check:affected --run recorded below on this head.

#2634 review)

Adds the reviewer-requested classifier-table pair: baseline "12" refuses
"12 567 8" for request "5678", while an absorbed residual (baseline "12",
observed "(555) 123-4567") completes because every extra is punctuation the
baseline cannot explain. The evidence level gains the same positive, so the
rule is pinned as keyed on extra provenance, not baseline emptiness. Tightens
the completion predicate's doc to match.
@thymikee

thymikee commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

Correction on one claim in my last comment: the red case was proven red by a local run against a build of cb24ef6 (the failure output above is from that run), but it landed as commits alongside the fix rather than as a separate pushed red commit — 9ef444a carries the predicate change plus the evidence-level case, and 3b98c12 adds the classifier-table pair (refusal "12"/"5678"/"12 567 8" and the absorbed-residual positive baseline "12" / "(555) 123-4567" for "5551234567"). Both the refusal and the positive are pinned at the completion-table and evidence levels now, so the rule is keyed on extra provenance, not baseline emptiness, and that choice is decided by tests rather than by prose.

@thymikee

thymikee commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

This PR is ready for review at 3b98c12. The fix in 3b98c12 resolves the residual finding from the earlier review, and all 21 checks pass at that commit. The delta only touches the Swift classifier and its unit tests, which the iOS runner lanes exercise. Not blocking, and you can take or leave these: the docs and help sentence "A normalizing field is typed once, never cleared and retyped" (commands.md:477, and cli-help-workflows.ts:186) overclaims for masks that keep their template after a clear, for example baseline "$0.00", request "1000", observed "$10.00", where the delta refuses completion and the fill still ends in TEXT_ENTRY_MISMATCH as on main, so either limit the sentence to fields whose cleared value shares no formatting characters with the result or pin that case as a known limit in the classifier table. Also, the comment "A one-character request has no between for an insertion to sit in." at RunnerTests+TextEntryConfirmation.swift:90 now describes no code, since the count check moved into the opening guard, so it can go. I did not run the XCTest suite or a device run. The fail-before and pass-after claims rest on the PR body and my trace of the predicate, the live evidence uses the runner host-app grouping fixture rather than the reported Flutter field, and I reasoned about the template-retaining mask case from the code without reproducing it on a device. No conflicts. All four cubic-dev-ai P3 threads are fixed at this commit: the caret restore in AgentDeviceRunnerApp.m is untouched by this delta, the shared device-test helper is still in place, the carriesRequest clause is gone and 'old123456' is an echo again, and commands.md:477 now reads "to one that raw equality cannot reach".

…e's known limit (#2634 review)

The docs and help claimed a normalizing field is always typed once; a mask
that keeps its template through the clear ("$0.00" before "$10.00") shares
characters with the value it formats, so its fill still takes the echo
reading with its repair — text cannot tell surviving template from
failed-clear residual, which is the same relation the residual fix relies
on. Pin the exact triple at the completion-table and evidence levels, state
the boundary in the docs/help and the predicate doc, and drop the orphaned
one-character-request comment left above the span bounds when the count
guard moved into the opening guard.
@thymikee

thymikee commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Both taken, fixed in bbc93e3.

On the overclaim: the template case is not fixable at the rule in the forward direction, and your triple proves it — baseline "$0.00", request "1000", observed "$10.00" is the SAME textual relation as a failed clear that left only "$" in the field, where the typed 1000 renders as "$10.00" through the same mask. Surviving template and surviving residual are one string; only the app's intent distinguishes them and the runner cannot read masks. So I took your second option and made it testable rather than prose: the exact triple is pinned as a known limit at both the completion-table level and the evidence level (baseline "$0.00" / observed "$10.00" for request "1000" yields nil → repair path, same as main), the predicate doc names the boundary, and the docs/help sentence now reads: typed-once holds, with the template-shown-while-empty ("$0.00") field named as the limit that keeps failing with a repair. The residual-pair positives stay where they are — those cases have no baseline character standing in the value unexplained, which is exactly the fact the template case cannot establish.

Orphaned comment cut; the doc at :74 carries the one-character fact alone.

Validation at bbc93e3: 8 classifier/policy tests + the grouping device test green on iPhone 18 Pro / iOS 27.0; pnpm check:affected --run all runnable checks passed (recorded in the PR body); format/lint/typecheck clean.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fill repairs and fails on fields that normalize their input

1 participant