Repository navigation
gui: Correct a read-back mismatch in fixed boxes - #109
BenWestgate wants to merge 3 commits into
Conversation
|
nit: It should also lock from editing the groups that are already correct. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1027e599cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start = next((i for i in range(same) if old[i] != new[i]), same) | ||
| tail = next((i for i in range(same - start) if old[-1 - i] != new[-1 - i]), same - start) |
There was a problem hiding this comment.
When an editable group contains adjacent identical symbols, this first-difference calculation cannot determine which copy the operator deleted and ignores the GTK cursor position already captured by _reformat. For example, deleting the first B from ABBC produces ABC? with the cursor after the remaining B, so typing X yields ABXC rather than the intended AXBC. This breaks the promised in-place correction for valid cards containing repeated characters; use the actual edit/cursor location when padding the deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d17822a550
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._dropped, self._locked = "", frozenset() | ||
| self.set_text(PREFIX) |
There was a problem hiding this comment.
When a read-back page is left or confirmation succeeds, clear() replaces only the GTK text and unlocks the entry, but _shown still references the complete recovery card last assigned by _reformat. set_text(PREFIX) merely schedules another idle reformat, so the card remains reachable after the page cleanup—and indefinitely if that idle callback does not run—contrary to the recovery-text clearing guarantee in docs/security/model.md. Reset _shown to PREFIX directly in clear() as part of the synchronous cleanup.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f96aea5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while start and len(new) - tail > cursor and new[-1 - tail] == old[-1 - tail]: | ||
| start, tail = start - 1, tail + 1 |
There was a problem hiding this comment.
When a selection is replaced with text sharing a suffix with the old group, this adjustment only handles inferred edit boundaries to the right of GTK's cursor. For example, replacing a selected editable QQQQ group with WQQQ leaves the text correct but infers only W as typed and moves the cursor after W instead of after the replacement; subsequent typing then edits the wrong position. Fresh evidence beyond the earlier deletion case is that the current fix still fails for multi-character replacements whose trailing symbols match the old text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think once the user has submitted their text, correct groups should be locked and only individual boxes with mistakes remain editable.
characters should no longer shift between boxes at that point, and tab can navigate between the remaining wrong boxes. We preserve the left most groups as correct when an extra or missing character falls between two groups. We also prefer the interpretation that leaves fewer red groups and more correct and locked groups.
There was a problem hiding this comment.
Agreed. Current head still uses one shifting Gtk.Entry plus cursor/suffix inference, so it does not implement this requested post-submit model. Rework the mismatch state into fixed editable boxes/groups with matched boxes locked and Tab navigation; that removes the repeated-symbol cursor ambiguity instead of extending the heuristic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 728db2b835
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if first // GROUP in self._locked or len(old) - tail > end: | ||
| return old, start |
There was a problem hiding this comment.
When two or more consecutive groups are highlighted, selecting those groups and pasting their corrected text changes only editable groups, but this condition compares the edit against the end of the first group and restores the entire old card whenever the edit continues into the next group. For example, an eight-character replacement spanning mismatched groups 2 and 3 is rejected even though neither group is locked, forcing the operator to edit each group separately despite the UI saying all highlighted groups can be changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We should prevent pasting as a security measure. It makes it easier to not backup the cards.
There was a problem hiding this comment.
Agreed. Current a458f9e still accepts pasted replacement text—indeed the latest commit explicitly lets one edit span adjacent open groups—so this requested behavior is not implemented. Disable paste in the post-submit correction state and keep edits confined to the remaining open boxes.
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: NACK a458f9e. The synchronous secret-cache cleanup is fixed, but the remaining single-entry cursor heuristics still conflict with the maintainer's requested mismatch UX: matched boxes should become fixed/locked, only wrong boxes editable with Tab navigation, and paste disabled in that state. The existing repeated-symbol/cursor findings should be resolved by that rework rather than more inference. No additional finding.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at a458f9e: existing P2s remain in the post-submit correction UI. Repeated-symbol edits still depend on cursor/suffix inference, paste remains accepted across open groups, and the requested fixed/locked mismatch boxes with Tab navigation are not implemented.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04aeb87633
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.add_css_class("card-entry") | ||
| self.set_input_hints(Gtk.InputHints.NO_SPELLCHECK | Gtk.InputHints.NO_EMOJI) | ||
| refuse_paste(self) | ||
| self.connect("changed", lambda _entry: GLib.idle_add(self._capitals)) |
There was a problem hiding this comment.
When the operator selects text in a highlighted mismatch box—for example, before replacing the group—GroupEntry lacks the notify::selection-bound and notify::cursor-position hooks used by Codex32Entry to clear GTK's primary selection. The selected recovery fragment therefore remains published long enough for a clipboard manager to persist it, rather than being reclaimed on the next main-loop turn as promised by the GUI security model; reuse the same unpublishing behavior for these new entries.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
| if not result.accepted: | ||
| groups = result.mismatched_groups | ||
| where = f"Group {min(groups)}" if groups else "What you typed" | ||
| _say(status, f"{where} does not match. Check it against your card.", "error") | ||
| correct(reading.aligned(card.text.upper(), entered)) | ||
| update() |
There was a problem hiding this comment.
On every failed retry, this globally realigns the concatenated box contents and rebuilds all boxes, so an edit can move into—and unlock—a previously matched group. For example, after groups 1 and 3 are open while group 2 (NAME) is locked, correcting group 1 to MS12N causes the next retry to redraw group 1 as locked MS12 and group 2 as editable NNAME; the extra character moved across the supposedly fixed boundary. Fresh evidence beyond the earlier single-entry cursor reports is that the new fixed-box implementation discards its boundaries here; retries should realign only existing editable boxes while retaining locked labels.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
Correction boxes did not take their selection back from the primary clipboard (Codex review on #109). The field that did watched only the selection bounds, but GTK also republishes the selection on focus and when the same range is selected again, so a copy could stay published. Both entries now share PrivateEntry, which watches the primary clipboard while the entry is realized and takes back any local copy made while it holds a selection. GTK only publishes from realized text, so nothing is missed, and the handler is dropped on unrealize. Security: narrows the window in which a clipboard manager can persist selected card text, for the read-back field, its boxes and the restore fields. Validation: new primary-clipboard test passed 20 of 20 runs (the old selection watch failed it about a third of the time); full suite normal and -O (998 passed); ruff, mypy. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
A failed retry realigned the joined text of every box and rebuilt them, so an extra character typed into an open box could move across into a locked group and unlock it (Codex review on #109). A retry now keeps every box where it is and only rechecks the open ones against their own groups; the cross-group alignment runs once, on the first submission from the single field. Its regression test, test_a_retry_keeps_every_box_where_it_is, landed in the previous commit by mistake and passes from this one. Validation: the retry test fails before and passes after; full suite normal and -O (998 passed). Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6505f47b02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retried = tuple( | ||
| (box.get_text().upper(), box.get_text().upper() == want) for box, want in zip(boxes, groups) | ||
| ) |
There was a problem hiding this comment.
When multiple boxes remain open and the operator fixes one using spaces (which the page explicitly says do not matter), confirm() evaluates the whitespace-stripped value, but this raw comparison still marks that box as wrong. For example, entering AB CD for an expected ABCD group while another group remains incorrect causes the repaired group to stay highlighted and editable on the retry. Compare each box using the same whitespace/case normalization used for confirmation while preserving the fixed group boundaries.
Useful? React with 👍 / 👎.
| ) | ||
| CORE_ADAPTER = "codex32._bitcoin_core" | ||
| BUDGET = 2000 | ||
| BUDGET = 2050 |
There was a problem hiding this comment.
Raising the enforced budget to 2,050 leaves the repository's review contracts stale: docs/developer/gui.md still states that this test enforces 2,000 lines, while docs/developer/api.md still records a separate 1,800-line GUI budget. Future reviewers therefore cannot tell which limit was authorized, and changes between 2,000 and 2,049 lines silently pass despite the current GUI documentation. Update the matching durable documentation together with this enforcement change.
Useful? React with 👍 / 👎.
The page says spaces and capitals do not matter, and confirm() strips them, but a retry compared each box's raw text with its group, so a group typed as "AB CD" stayed highlighted while another was still wrong (Codex review on #109). Boxes are now compared as confirm() reads them. Validation: new retry test fails before, passes after; GUI tests, ruff, mypy. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
gui.md still said 2,000 and api.md still said 1,800 after the enforced budget rose to 2,050 for the card read-back rework (Codex review on #109). Both now state the enforced figure, and gui.md notes why it rose. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
This comment has been minimized.
This comment has been minimized.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI current-head review: ACK c57dd55. The fixed-box mismatch model removes the earlier cursor/repeated-symbol ambiguity, retry comparison stays within the existing group boundaries, whitespace is normalized before a repaired group locks, and PrivateEntry reclaims primary-selection contents for both the original entry and correction boxes. Paste/drop paths are disabled and the GUI budget documentation matches 2,050. The older unresolved threads are against superseded implementations; I found no current-head correctness issue.
|
Could you squash your commits according to CONTRIBUTING.md before human code review? |
Formatting only. Calls and signatures that were wrapped one argument per line now sit on one line where they fit within the 110-character limit, which frees review lines under the GUI budget for the read-back rework that follows. The syntax trees of all four files are unchanged. Validation: ast.dump of each file is identical before and after; ruff check and ruff format --check pass. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
Selecting text in the codex32 field hands it to the primary selection, where a clipboard manager could copy the card into a history file on disk. The field took the copy back when the selection bound or cursor moved, but GTK publishes the selection again on focus and on other events that move neither, so a selected card was often left on the primary clipboard. Move the guard into a PrivateEntry base class that watches the primary clipboard itself while the entry is realized and drops the local content whenever the entry has a selection. Selecting still works; only the copy that leaves the program is taken back. The read-back group boxes that follow reuse this class. Validation: the new test fails on 4 of 5 runs before this change and passed 8 of 8 after it; ruff, mypy and the GUI tests pass. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
c57dd55 to
243c627
Compare
BenWestgate
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 243c627380
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| field.set_visible(True) | ||
| state = reading.read(reading.normalize(typed()), length=len(card.text)) if boxes else field.reading() | ||
| # A missing or extra character may be submitted: its group is then the one highlighted. | ||
| accept.set_sensitive(abs(len(state.text) - state.expected) <= reading.SLACK) |
There was a problem hiding this comment.
When B, I, O, or 1 is typed instead of a card symbol, _reformat removes it while retaining the specific lookalike warning; this new length-only condition nevertheless enables submission because the normalized value is only one character short. check() then aligns that normalized value, permanently discarding the offending glyph and replacing its useful “probably 8/J/L/0” diagnostic with the generic mismatch message. Require the lookalike fault to be clear before enabling confirmation, while still allowing genuine missing or extra symbols.
Useful? React with 👍 / 👎.
| 2,000 until the card read-back rework of 2026-10-07, which added fixed | ||
| correction boxes, group alignment and paste refusal. |
There was a problem hiding this comment.
This note records the read-back rework, but the review map immediately above still lists reading.py and entry.py at 131 and 73 logical lines; using the test's counting rule they are now 154 and 111. It also still says only the first three modules make decisions even though entry.py now owns the primary-selection and paste-refusal security behavior, so reviewers following the documented order can under-review the newly added boundary logic. Update the table, totals, and review guidance alongside the budget rationale.
Useful? React with 👍 / 👎.
When a card was read back with a mistake, the page named only the first wrong group and left the whole card in one field, where an extra or missing character shifted every later group and a fix in one place could disturb the groups already right. Enter now submits a read-back of about the right length, within the same slack as the card field, so a missing or extra character can be submitted and located. A B, I, O or 1 still has to be fixed first: it is named as the lookalike it probably is, never charged to a group as an ordinary mismatch. On a mismatch the typed text is aligned with the card's groups: a missing or extra character between two groups is charged to the right-hand group, keeping the left one correct, and of the alignments the one with the fewest wrong groups is chosen. The page then shows one fixed box per group. Right groups are locked labels; only wrong groups are editable entries, Tab moves only between them, and characters never move from one box to another. A retry rechecks each box in place, ignoring spaces and case as the card field does, and clearing the field puts the single field back. Pasting, middle-click pasting and dropping text are refused in the read-back field and in every box, so a card is typed from the paper rather than copied from elsewhere, which would defeat the check that the backup was written correctly. The rework adds about 80 logical lines (1,964 to 2,045), so the GUI budget is raised from 2,000 to 2,050 and the developer docs record why. The review map in gui.md gets current line counts and now points reviewers at entry.py's clipboard and paste guards. Security: the read-back compares against the card already generated; no share generation, recovery or checksum code changes. Refusing paste and keeping the boxes on the primary-clipboard guard reduce copies of the card outside the program. Validation: ruff check, ruff format --check, mypy on src/codex32 and src/codex32_gui; full suite 1000 passed under python -m pytest and python -O -m pytest (xvfb). Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
243c627 to
62e1a08
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: code ACK 62e1a08. Retry boxes retain matched-group boundaries, normalize spaces, reject look-alike faults, and share the primary-selection guard. The review map now covers the new security-sensitive entry code. Before human review: satisfy the requested atomic commit cleanup and exercise GTK read-back on Tails.
This branch has not been deployed
2 participants
Requested by Ben · project thread
What
On the card read-back page (Bails's pinned GUI,
1938b60):Why
Ben tested this in Bails and found the re-entry help far weaker than the CLI's, and that Enter didn't submit. The current GUI line (
gui-reviewability-v1) already does the highlighting and Enter. Repinning Bails to it would drop restore's recorded-fingerprint check, so this ports those behaviours onto the pin. Ben then asked for matched groups to be locked, for fixed boxes with Tab navigation, and for paste to be refused.Commits
Squashed per CONTRIBUTING.md into three commits, each passing on its own:
gui: Join calls that fit on one line: formatting only; each module's AST is identical.gui: Watch the primary clipboard, not the cursor:PrivateEntrybase class, with a test that fails on 4 of 5 runs before it.gui: Correct a read-back mismatch in fixed boxes:reading.aligned()(no toolkit),GroupEntry,refuse_paste(), the page rework and its tests. The GUI budget rises from 2000 to 2050 at Ben's request; the GUI is now 2045 lines, andgui.md's review map has current counts. Confirmation is still the library's positional comparison; the alignment only chooses what to highlight.After merge: tag the result (like
bails-recovery-1938b60) and repin Bails'sinstall-codex32andopen-codex32to it.Validation
python -O.tests/test_gui_read_back.pycovers Enter, lookalikes, locked and open boxes, a missing plus an extra character, correcting in place, Tab order, refused paste, retries and clearing.tests/test_gui_reading.pycovers the boundary and fewest-groups rules;tests/test_gui_entry.pythe primary clipboard.src/codex32,src/codex32_gui) are clean.AI-written (Claude); requires responsible-human review per
docs/developer/AI_POLICY.md.🤖 Generated with Claude Code
https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q