Keep a 96-set designation out of the 94-set lookup when DECNRCM moves - #146
Conversation
Fixes tomlm#137 ESC - A designates ISO Latin-1 and ESC ( A designates the United Kingdom set, and the 96-set designators were given their own path so the two could not be confused. The identifier was then stored without the space it came from, and DECNRCM re-resolves every stored identifier through the 94-set lookup -- so the collision came back the moment the mode moved, and stayed, because resetting the mode re-resolves it the same wrong way. ESC - A, SO, print # -> # correct ESC - A, DECNRCM set, SO -> pound sign, the UK set A designation is now stored with its space and re-resolved in it. The national replacement sets are all 94-set, so a 96-set designation has no DECNRCM-dependent reading at all and is simply left alone. The screen this was filed against renders. vttest's SI/SO test draws three rows after ESC - A and the report says two of them vanish; replayed into a headless terminal, with and without SO, all three are present. What was really wrong is what the title now says, and it is invisible in that screen: the test's text is the ASCII range, where Latin-1 and UK differ in one character, '#'. The regression test invokes G1 with SO in every case, and carries a French designation alongside, so the two cannot agree because nothing is being re-resolved at all. SO and SI are written as escapes: the file had them as literal control bytes, which nothing that touches a source file on the way here preserves. 2143 passed, 1 skipped (DA3, tomlm#123), 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
DECSC/DECRC restores resolved tables without restoring the new namespace metadata, allowing a later DECNRCM transition to corrupt the restored designation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preserves 96-set charset identity across DECNRCM transitions.
Changes:
- Stores charset identifiers with their 94/96-set namespace.
- Centralizes designation resolution.
- Adds DECNRCM regression coverage.
File summaries
| File | Description |
|---|---|
InputHandler.Print.cs |
Resolves namespace-aware designations. |
InputHandler.cs |
Stores designation namespace metadata. |
VtTestBehaviourTests.cs |
Tests 96-set behavior across DECNRCM changes. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| var latin1 = Sized(30, 3); | ||
| latin1.Write($"{Esc}-A#@["); | ||
| latin1.Write($"{Esc}-A{ShiftOut}#@[{ShiftIn}"); |
There was a problem hiding this comment.
Correct, and the PR body was the thing that was wrong -- I described the change and did not make it. Fixed in 7dff244: both halves now designate G1 with ESC ) / ESC - and invoke it with SO, so they differ by the namespace and nothing else.
| _charsetIds[mode] = designation; | ||
| _charsets[mode] = Resolve(designation); |
There was a problem hiding this comment.
Confirmed, and it is worse than the 96-set case you found.
The same fault exists a designation earlier, with no 96-set involved:
ESC ( 0, DECSC, ESC ( R, DECRC -> line drawing back, correct
...then DECNRCM -> letters, re-resolved as French
That one predates this branch -- it arrived with the identifiers themselves in #141 -- so a TUI that saves the cursor mid-border gets its border back and then loses it the next time anything touches DECNRCM, arbitrarily far from the DECRC that caused it.
Fixed in 7dff244 the way you suggest: DECSC saves the (Id, NinetySix) designation and DECRC resolves it, so the identifiers are the source and the tables follow. Resolving at restore time also gives the right answer when DECNRCM moved between the save and the restore.
Every G-set is now seeded to B instead of being left absent, so the save, the restore and the DECNRCM refresh all walk four slots that are always there.
The regression test covers both spaces, and also asserts the restore without a mode change -- that half was never broken, and a test that stopped there would have passed on the defect.
Both of Copilot's findings on tomlm#146, and one they lead to. DECSC saved the TABLE each G-set had resolved to. The screen after a DECRC was therefore right and the state behind it was not: the identifier stayed as whatever had been designated after the save, so the next DECNRCM re-resolved the restored slot into that instead. ESC ( 0, DECSC, ESC ( R, DECRC line drawing back, correct ...then DECNRCM letters -- re-resolved as French That half is older than this branch; it arrived with the identifiers themselves. Copilot found the 96-set version of it, where a restored Latin-1 becomes the United Kingdom set, and the 94-set one above is the same fault a designation earlier. DECSC now saves the designation and DECRC resolves it against the mode state as it is then, which is also the right answer when DECNRCM moved between the two. Every G-set is seeded to B rather than left absent, so a designation is a value rather than a value-or-absent and the walks over the four -- the save, the restore, the DECNRCM refresh -- are all total. The other finding: the UK half of the separate-space test designated G0 and printed without a shift while the Latin-1 half designated G1 and shifted, so the two differed by two things rather than one. Both now designate G1 and invoke it with SO. The PR body had described the test this way before the test did. The new test asserts the restore WITHOUT a mode change as well. That is the part that was never broken, and a test that stopped there would pass on the defect. 2144 passed, 1 skipped (DA3, tomlm#123), 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The DECSC half of this never landed. #141 took the branch this was stacked on, which closed it unmerged; the 96-set half arrived independently in Re-checked on merged main ( Recovered and ported to the structures on main in #149, with Copilot's test finding from this thread carried over. |
Recovered from tomlm#146, which was closed unmerged when the branch it was stacked on went away with tomlm#141. The 96-set half of that PR landed independently in 1f08d48; this half did not, and still reproduces on 6d32504. DECSC saved the TABLE each G-set had resolved to. The screen after a DECRC was therefore right and the state behind it was not: the identifier stayed as whatever had been designated after the save, so the next DECNRCM re-resolved the restored slot into that instead. ESC ( 0, DECSC, ESC ( R, DECRC line drawing back, correct ...then DECNRCM letters -- re-resolved as French ESC - A, DECSC, ESC ) A, DECRC Latin-1 back, correct ...then DECNRCM a pound sign -- the UK set The first is older than the 96-set work; it arrived with the identifiers themselves. The second is the identifier collision again, one save and restore later: A is ISO Latin-1 in the 96-set space and the United Kingdom set in the 94-set one, and the saved table records neither. DECSC now saves the designation with the space it came from, and DECRC resolves it against the mode state as it is then -- which is also the right answer when DECNRCM moved between the two. One Resolve answers "what does this designation mean now" for all three callers that ask: the designation path, DECRC, and the DECNRCM refresh. They resolved separately before, which is how two of them disagreed. Every G-set is seeded to B rather than left absent, so a designation is a value rather than a value-or-absent and the walks over the four -- the save, the restore, the DECNRCM refresh -- are all total. Also from Copilot's review on tomlm#146: the separate-space test designated G0 for its UK half and G1 for its Latin-1 half, so the two differed by two things rather than one. Both designate G1 and invoke it with SO now, which also takes the last two literal control bytes out of the file. The new test asserts the restore WITHOUT a mode change as well. That is the part that was never broken, and a test that stopped there would pass on the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Save the designation at DECSC, not the table it resolved to Recovered from #146, which was closed unmerged when the branch it was stacked on went away with #141. The 96-set half of that PR landed independently in 1f08d48; this half did not, and still reproduces on 6d32504. DECSC saved the TABLE each G-set had resolved to. The screen after a DECRC was therefore right and the state behind it was not: the identifier stayed as whatever had been designated after the save, so the next DECNRCM re-resolved the restored slot into that instead. ESC ( 0, DECSC, ESC ( R, DECRC line drawing back, correct ...then DECNRCM letters -- re-resolved as French ESC - A, DECSC, ESC ) A, DECRC Latin-1 back, correct ...then DECNRCM a pound sign -- the UK set The first is older than the 96-set work; it arrived with the identifiers themselves. The second is the identifier collision again, one save and restore later: A is ISO Latin-1 in the 96-set space and the United Kingdom set in the 94-set one, and the saved table records neither. DECSC now saves the designation with the space it came from, and DECRC resolves it against the mode state as it is then -- which is also the right answer when DECNRCM moved between the two. One Resolve answers "what does this designation mean now" for all three callers that ask: the designation path, DECRC, and the DECNRCM refresh. They resolved separately before, which is how two of them disagreed. Every G-set is seeded to B rather than left absent, so a designation is a value rather than a value-or-absent and the walks over the four -- the save, the restore, the DECNRCM refresh -- are all total. Also from Copilot's review on #146: the separate-space test designated G0 for its UK half and G1 for its Latin-1 half, so the two differed by two things rather than one. Both designate G1 and invoke it with SO now, which also takes the last two literal control bytes out of the file. The new test asserts the restore WITHOUT a mode change as well. That is the part that was never broken, and a test that stopped there would pass on the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address Copilot's review: narrow the property, cover the mode change Two findings, both worth acting on, one for a different reason than the one given. The API break is real as a type change and cannot break anyone: the property arrived in #93 on 2026-08-29, and the latest release is v1.2 from 2026-08-26, so no published package has ever carried it. That makes the answer easier rather than harder -- it is DECSC scratch a consumer cannot do anything with, so it is internal now, and the shape of an internal detail stops being an API question every time DECSC learns something more. Nothing outside the assembly referenced it; the solution builds, demos included. The untested case was the sharper find. The three cases in the test move DECNRCM AFTER the DECRC, so replaying a saved table would satisfy all three -- the very contract the doc comment claims, that a restore resolves against the mode as it is at restore time, had no test at all. Both directions now, because they fail oppositely: French designated with NRC off, DECSC, NRC on, DECRC -> a-grave French designated with NRC on, DECSC, NRC off, DECRC -> @ On the pre-fix code those give @ and a-grave respectively -- each the table that was saved rather than what the designation means at the moment it is put back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Fixes #137
Stacked on #141 — it targets
vttest-alignment, notmain, because it fixes a hole in the commit on that branch that added the 96-set designators.The screen in the issue is not broken
Before anything else: vttest's SI/SO test renders. The issue says rows 8 and 9 vanish after
ESC - Aand only row 10 arrives, and suspects the parser eats what follows that intermediate. Replayed into a headless 80x24 terminal — the exact stream the issue quotes, then again with SO in front of it, which the quoted stream omits — all three rows are present, at the right columns, with the right text.Nothing eats anything. The report should be read as "the designation had no effect", which it did not, and that half was fixed on
vttest-alignmentby the commit that gaveESC -,ESC .andESC /their own dispatch.What is actually wrong
That commit is careful about the one thing that matters here, and says so:
Ais ISO Latin-1 in the 96-set space and the United Kingdom set in the 94-set one, so a 96-set designation must not go through the 94-set lookup, or a program asking for Latin-1 silently gets a pound sign where it wrote#.It avoids that at designation time and hands it straight back at refresh time. The identifier is stored without the space it came from, and DECNRCM re-resolves every stored identifier through
Charsets.GetCharset— the 94-set lookup:ESC - A, SO, print#@[#@[— correctESC - A, DECNRCM set, SO, print#@[£@[— the UK set£@[— it does not come backThe mode need not be set before the designation. A program designates Latin-1 once, DECNRCM moves for any reason later, and G1 quietly becomes UK for the rest of the session.
The fix
A designation is stored with its space,
(Id, NinetySix), and re-resolved in it. Both setters go through oneDesignate, and oneResolveanswers "what does this designation mean now" for the designation path and the DECNRCM path alike — which is the actual invariant, and it could not be stated while the two paths resolved separately.96-set designations are left alone by DECNRCM entirely: the national replacement sets are all 94-set, so there is no mode state in which a 96-set identifier reads differently. Nothing here is on the print path —
Resolveruns on designation and on DECNRCM, not per character.DECSC saved the wrong half of it (from review)
Copilot found that DECSC/DECRC never learned about the namespace, and following it turned up a bigger version of the same fault that predates this branch.
DECSC saved the table each G-set had resolved to. So the screen after a DECRC was right and the state behind it was not — the identifier stayed as whatever had been designated after the save, and the next DECNRCM re-resolved the restored slot into that:
ESC ( 0, DECSC,ESC ( R, DECRCThat one has nothing to do with 96-sets; it arrived with the identifiers themselves in #141. A TUI that saves the cursor mid-border gets its border back and loses it again the next time anything touches DECNRCM, arbitrarily far from the DECRC that caused it. Copilot's case is the same shape one designation later, with Latin-1 restored and then re-resolved as UK.
DECSC now saves the
(Id, NinetySix)designation and DECRC resolves it — identifiers are the source, tables follow. Resolving at restore time is also the right answer when DECNRCM moved between the save and the restore.Every G-set is seeded to
Brather than left absent, so a designation is a value rather than a value-or-absent, and the three walks over the four slots — save, restore, DECNRCM refresh — are all total.Tests
A_96_set_designation_is_not_re_resolved_as_a_94_set— DECNRCM set after the designation, and set-then-reset, both asserting Latin-1 survives. Verified to fail on the defect with the production change reverted (£@[where#@[is expected), and the fifteen tests around it still pass.It carries a French designation alongside, deliberately: French is a 94-set national replacement, so it must move when DECNRCM does. Without it the two Latin-1 assertions would pass just as well if re-resolution had stopped happening at all, which is the failure mode this test would otherwise be blind to.
DECRC_restores_what_was_designated_not_what_it_resolved_to— both spaces, because they fail differently, and also the restore without a mode change. That last half was never broken, and a test that stopped there would pass on the defect.The_96_character_set_designators_are_a_separate_spacegainsESC )for its UK half, so both halves designate G1 and invoke it with SO. They were asymmetric — UK through G0 with no shift, Latin-1 through G1 with one — which left the difference between them as two changes rather than one. Same letter, same invocation, different space, one difference.SO and SI are now written as the six-character escapes rather than as literal control bytes in the file. That is the hazard already written down in CLAUDE.md for ESC, and it applies to these for the same reason.
Verification
2144 passed, 1 skipped, 0 failed— the skip is DA3, #123, still open on purpose.🤖 Generated with Claude Code