Fix private-marker stripping that misroutes CSI commands

[Copilot]

• Inspect CSI command dispatch and identify the private-marker aliasing hazard
• Make CSI command lookup explicit for private and non-private identifiers so unmapped private sequences stay unknown
• Add focused tests covering private-sequence dispatch invariants and known regressions
• Run targeted tests to validate the change
• Scan changed files for secrets and run CodeQL review before finalizing

• Fixes CSI private-marker stripping silently misroutes private sequences to their non-private handler #49

[github-actions]

Perf comparison

3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.

corpus	bytes/char	gen0/Mchar	ns/char	Δ time	noise	gate
scroll-ascii	0.00 → 0.00	0.00 → 0.00	3.38 → 3.36	-0.6%	±1%	5%
sgr-churn	0.00 → 0.00	0.00 → 0.00	8.81 → 8.85	+0.4%	±1%	5%
truecolor	0.00 → 0.00	0.00 → 0.00	9.77 → 9.72	-0.6%	±1%	5%
alt-redraw	0.00 → 0.00	0.00 → 0.00	13.61 → 13.71	+0.7%	±3%	9%
unicode	7.66 → 7.66	0.45 → 0.45	33.67 → 34.03	+1.1%	±9%	28%
flood	0.00 → 0.00	0.00 → 0.00	94.34 → 93.68	-0.7%	±1%	5%

Each corpus is gated at max(5%, 3 × its own noise). A wide noise column means this runner was busy and the timing half of the table should be read as advisory; the allocation half is exact either way.

assemblies measured

• base: XTerm.NET 2.0.0.0 mvid:2e4ad242-678c-4e4b-9059-939f89f90b2f
• head: XTerm.NET 2.0.0.0 mvid:2aea67c9-c3d7-4df9-a768-04f7adc5d61b

[tomlm]

@claude can you take over this issue/PR? It's now yours

[claude]

Claude finished @tomlm's task in 11m 15s —— View job

---

Took over the PR — finished and pushed

Pushed 2a3eaec to copilot/fix-private-marker-misroute.

• Read the PR context and the partial work on the branch
• Finish the CSI private-marker dispatch fix
• Add tests covering both the misroutes and the private sequences that must keep working
• Document the fix in FIXES.md
• Push
• ⚠️ Build + test run — not done locally, see below

---

What the bug actually was

ToCsiCommand trimmed a leading ? or > and looked up what was left. For h, l, n and $p that's correct — the private and non-private forms really are the same handler with a flag. For every other final character it's a coincidence, and the aliasing ran the wrong command on input ordinary programs emit at startup:

Sequence	What it is	What ran instead
CSI ? Pi;Pa;Pv S	XTSMGRAPHICS	SCROLL UP — the screen jumped
CSI > 4;2 m	XTMODKEYS	SGR 4;2 — underline + dim on everything after
CSI > 1 u / CSI ? u	Kitty keyboard protocol	RESTORE CURSOR
CSI ? Pm s	XTSAVE (private modes)	SAVE CURSOR — clobbered the real saved cursor
CSI ? Pm r	XTRESTORE	SET SCROLLING REGION with the mode number as a row, then home
CSI > Ps q	XTVERSION	DECSCUSR — changed the cursor shape
CSI > Ps t	XTSMTITLE	XTWINOPS — CSI > 2 t minimised the window

The branch as I found it fixed the lookup but only carried the one already-known symptom (XTSMGRAPHICS) into a test. The remaining six were live and untested.

What I changed on top of the existing work

• CommandExtensions.cs — kept the exact-identifier match, listed the private entries explicitly (?J DECSED, ?K DECSEL, ?S XTSMGRAPHICS, ?h, ?l, ?n, >c, ?$p), added StringComparer.Ordinal, and documented why the strip-then-match approach was wrong.
• Dropped ?c. CSI ? c isn't a sequence; answering it as a secondary DA was an artefact of the stripping, not a decision. >c (DA2) stays.
• CsiCommand.cs — moved GraphicsAttributes next to ScrollUp, where it belongs, and noted why it's a separate command rather than a variant.
• PrivateCsiDispatchTests.cs (new) — one test per misroute above, all driven through Terminal.Write rather than HandleCsi directly, because the parser is what builds the identifier and that's where the bug lived. Plus tests that the implemented private sequences still reach their handlers (?25h/?25l, ?J, ?K, >c, ?6n, ?2026$p, ?S) and that plain CSI S still scrolls.
• CsiCommandExtensionsTests.cs — expanded to pin the mapping in both directions, including <u and =c staying unknown.
• Removed the ad-hoc ?u test that had been dropped into InputHandlerTests.cs; it's covered properly in the new file.
• Updated two now-stale comments that still described the marker as being stripped (InputHandler.GraphicsAttributes, GraphicsAttributesTests).
• Added a FIXES.md section, matching the repo's per-branch convention.

What I could not verify

dotnet isn't in this job's allowed tools, so I could not build or run the test suite — every dotnet build / dotnet test invocation was refused, as was git fetch (the checkout is shallow, so origin/main...HEAD has no merge base; I diffed trees instead and confirmed the branch is otherwise in sync with main). The change is verified by reading only: I traced each sequence through EscapeSequenceParser.DispatchCsi to confirm the identifier it builds, and checked each assertion against the handler it lands in (SetScrollRegion homes the cursor, DECRQM only answers for mode 2026, EraseInDisplay(2) fills in place, XTWINOPS 2 needs WindowOptions.MinimizeWin). CI on this push is the real check — if anything goes red, ping me and I'll fix it.

To re-enable local validation, add Bash(dotnet:*) to --allowedTools.
• branch copilot/fix-private-marker-misroute

[tomlm]

@claude I have enabled dotnet tool and when you are done, change title to remove [WIP] and remove Draft status

[claude]

Claude finished @tomlm's task in 2m 51s —— View job

---

Verified green — but I still can't run dotnet or edit PR metadata from this job

• Read PR context and the work already on the branch
• Verify the build
• Verify the test suite
• Review the diff end-to-end
• ⚠️ Run dotnet locally in this job — still blocked, see below
• ⚠️ Remove [WIP] from the title and undraft — I can't, see below

---

The code is verified — via CI, not locally

dotnet is still refused in this job, so I couldn't build here. Instead I pulled the real CI result for the current head of this PR (0f6f546, checked out as refs/remotes/pull/62/merge, i.e. the merge with main):

BuildAndRunTests → success

Test Run Successful.
Total tests: 1265
     Passed: 1265
 Total time: 4.2890 Seconds

Build succeeded.
    0 Warning(s)
    0 Error(s)

The new files ran as part of it — PrivateCsiDispatchTests and CsiCommandExtensionsTests account for 52 of those passing tests. So the "not verified" caveat from my previous comment is now resolved: the change builds clean and the whole suite passes. Claude Code Review also came back green; PerfCompare was still in progress when I looked, and the earlier perf run on this branch showed no regression (allocations dropped on sgr-churn and truecolor).

I also re-read the diff rather than trusting the earlier pass:

• CommandExtensions.cs — exact-identifier Dictionary<string, CsiCommand> with StringComparer.Ordinal; private forms present only where they're genuinely the same handler (?J, ?K, ?h, ?l, ?n, ?$p), plus ?S → GraphicsAttributes and >c → DA2. Everything else private falls through to Unknown, which is the correct no-op for an unimplemented sequence.
• InputHandler.cs — the isPrivate branch inside case ScrollUp: is gone, replaced by a real case GraphicsAttributes:. Dispatch now reflects the mapping instead of patching it after the fact.
• CsiCommand.cs — GraphicsAttributes added next to ScrollUp with a note on why it's a separate command.

Why dotnet is still blocked

The repo has the right rules in .claude/settings.json:

{ "permissions": { "allow": ["Bash(dotnet:*)", "Bash(git:*)", "Bash(xargs grep:*)"] } }

…but project-level settings aren't applied to the action's session — every dotnet invocation (--version, build, test) came back This command requires approval. The permissions have to be passed to the action itself, in .github/workflows/claude.yml:

      - name: Run Claude Code
        uses: anthropics/claude-code-action@v1
        with:
          anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
          claude_args: '--allowedTools "Bash(dotnet:*),Bash(gh pr:*)"'

I'm unable to make that edit myself — GitHub App permissions don't allow modifying files under .github/workflows. One line in that workflow and future runs can build and test locally instead of leaning on CI.

Why I can't retitle or undraft

Same wall from the other side: gh isn't in this job's allowed tools (gh pr view and gh pr edit were both refused), and the job's own token is pull-requests: read anyway, so even with the tool allowed the edit would be rejected. Adding Bash(gh pr:*) to claude_args and raising the job to pull-requests: write would let me do it next time.

Until then it's two clicks for you — the work itself is finished and CI is green:

• Title → Fix private-marker stripping that misroutes CSI commands
• Ready for review to drop the draft status

No code changes were needed this round, so nothing new was pushed; the branch is exactly as it was at 0f6f546.
• branch copilot/fix-private-marker-misroute

[tomlm]

@claude I fixed permissions for gh

[claude]

Claude finished @tomlm's task in 1m 15s —— View job

---

Done — retitled, undrafted, and verified locally this time

• Check that gh is actually usable from this job
• Re-verify the branch builds and the suite passes
• Remove [WIP] from the title
• Mark the PR ready for review

---

Both tools work now, so the caveats from my last two comments are gone.

Local build + test (dotnet test src/XTerm.NET.slnx, on this branch, not CI):

Passed!  - Failed: 0, Passed: 1265, Skipped: 0, Total: 1265, Duration: 3 s

This is the first run I've done in-job rather than reading CI's result. It matches what CI reported on 0f6f546 — same 1265, all passing.

One warning came out of the build, and it is not from this PR:

SelectionTests.cs(109,17): warning xUnit1013: Public method
'SelectionText_UsesLineFeedLineEndings' should be marked as a Fact.

SelectionTests.cs isn't among this PR's changed files — that's a pre-existing public method on a test class with no [Fact], so it's a test that silently never runs. Worth a separate look, but I've left it alone rather than widen this PR. Fix this →

PR state now:

{"number":62,"title":"Fix private-marker stripping that misroutes CSI commands","isDraft":false,"state":"OPEN"}

No code changes were needed this round, so nothing was pushed — the branch is still at b38fe72. The fix itself is unchanged from what I described earlier: ToCsiCommand now matches the full CSI identifier instead of stripping the leading ?/>, so the seven private sequences that were aliasing onto unrelated handlers (XTSMGRAPHICS→SCROLL UP, XTMODKEYS→SGR, Kitty keyboard→RESTORE CURSOR, XTSAVE/XTRESTORE→cursor save & scroll region, XTVERSION→DECSCUSR, XTSMTITLE→XTWINOPS) fall through to Unknown, while the private forms that genuinely share a handler stay mapped.

It's out of draft and ready for your review.
• branch copilot/fix-private-marker-misroute

[tomlm]

@claude fix conflicts

[claude]

Claude finished @tomlm's task in 10m 54s —— View job

---

Conflicts resolved and pushed — one command left for you

• Work out what actually conflicts
• Resolve it, including the semantic overlap with the Kitty keyboard protocol
• Update the tests that the merge invalidated
• Build + run the full suite
• Push — fa60fa6
• ⚠️ Record the merge parent — I can't, git merge/git fetch aren't in this job's tools. One command for you, below.

---

What conflicted

main moved to 7e86849 (PR #48, the Kitty keyboard protocol) after this branch last synced. Three files overlap, and the overlap isn't only textual — #48 touches the exact lookup this PR rewrites:

• Common/CommandExtensions.cs — Implement the Kitty keyboard protocol (CSI u) #48 added =u, ?u, >u, <u to _commandMap and added an exact-match attempt in front of the strip-then-match fallback, as a targeted fix for ?u landing on RESTORE CURSOR. This PR deletes the fallback entirely, so the exact match is the only lookup. I kept Implement the Kitty keyboard protocol (CSI u) #48's four identifiers and grouped them with u, where the other private entries live.
• Common/CsiCommand.cs — Implement the Kitty keyboard protocol (CSI u) #48's four KittyKeyboard* members now sit alongside this PR's GraphicsAttributes.
• InputHandler.cs — kept Implement the Kitty keyboard protocol (CSI u) #48's Kitty and left/right-margin work in full; the isPrivate branch inside case ScrollUp: is still replaced by a real case GraphicsAttributes:.

Everything else from main (KittyKeyboard.cs, KittyKeyboardState.cs, TerminalBuffer.cs, Terminal.cs, TerminalMode.cs, TerminalOptions.cs, the three new test files, the workflow changes) came across untouched.

Tests the merge invalidated

CsiCommandExtensionsTests asserted ?u and >u were Unknown and that <u was unrecognised. That was right when nothing implemented Kitty and wrong the moment #48 landed, so I moved all four to the mapped-private theory pinning them to KittyKeyboardSet / Query / Push / Pop, and put <c, =S, <m in the "never recognised" theory in <u's place. The point the old assertions made — that ?u/>u must not reach RESTORE CURSOR — is now made by them reaching the Kitty handler instead, which is stronger. Kitty_keyboard_sequences_do_not_restore_the_cursor in PrivateCsiDispatchTests still holds unchanged and still means what it said. FIXES.md records why the Kitty forms are the clearest case for matching exactly.

Verified locally, not read off CI:

dotnet build src/XTerm.NET.slnx   →  Build succeeded. 0 Errors
dotnet test  src/XTerm.NET.slnx   →  Failed: 0, Passed: 1507, Skipped: 0

(1507, up from 1265 — the 242 new ones are #48's Kitty and left/right-margin tests, now running on this branch.) The one warning, xUnit1013 in SelectionTests.cs, is pre-existing on main and untouched here.

The one thing left, and why

git fetch, git merge, git checkout and git replace are all outside this job's allowed tools, and the checkout is shallow with main grafted, so there's no reachable merge base. I did the merge by hand — reading each file out with git show origin/main:<path> and resolving the three overlaps — but I have no way to record main as a second parent, so GitHub still computes the merge base as 9293e5b and still reports CONFLICTING.

The tree is already correct. I checked it directly:

$ git diff --stat origin/main HEAD
 FIXES.md                                        |  65 +++++-
 .../Common/CsiCommandExtensionsTests.cs         |  83 +++++++
 .../Graphics/GraphicsAttributesTests.cs         |  10 +-
 src/XTerm.NET.Tests/PrivateCsiDispatchTests.cs  | 247 +++++++++++++++++++++
 src/XTerm.NET/Common/CommandExtensions.cs       |  59 +++--
 src/XTerm.NET/Common/CsiCommand.cs              |  12 +-
 src/XTerm.NET/InputHandler.cs                   |  20 +-

Nothing of main's is missing or reverted — that diff is exactly this PR's intended delta. Which makes the fix a single command:

git fetch origin
git checkout copilot/fix-private-marker-misroute
git merge -s ours --no-edit origin/main
git push origin copilot/fix-private-marker-misroute

-s ours keeps this branch's tree verbatim and records main as the second parent. It's normally the wrong tool because it throws away the other side — here the other side is already in the tree, which is what the diff above establishes. Don't use the web "Resolve conflicts" editor instead: it would ask you to hand-pick hunks in CommandExtensions.cs, and the correct resolution there isn't either side's text.

Until that lands, Files changed will show #48's Kitty work as part of this PR. That's the same stale merge base, and it clears up the moment the parent is recorded.

To stop this recurring: Bash(git fetch:*),Bash(git merge:*) in claude_args in .github/workflows/claude.yml — I can't edit that file myself, GitHub App permissions exclude .github/workflows.
• branch copilot/fix-private-marker-misroute

[tomlm]

@claude fix merge

[claude]

Claude encountered an error after 2m 18s —— View job

---

Recording the merge parent

• Check whether git fetch/git merge are usable this run
• Confirm the tree already contains all of main
• Record main as a merge parent
• Build + test
• Push
  • branch copilot/fix-private-marker-misroute

[tomlm]

@claude please fix the merge confclit

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[tomlm]

Review: exact-identifier CSI lookup

Reviewed at 9ecd342 (after the merge of main landed mid-review — the earlier head fa60fa6 was ~40 commits behind and I have re-checked everything against the current one).

The core change is right, and it is structural rather than a patch of the three known cases. ToCsiCommand is now a single GetValueOrDefault against a map keyed on the whole identifier the parser built. There is no fallback, so an unlisted private form resolves to Unknown by construction — the next CSI ? X cannot alias onto X because there is no code path that could make it. That is what #49 asked for.

Findings

Medium

• CommandExtensions.cs:71 — { "q", SelectCursorStyle } is the one entry left that is keyed on the final character alone, and it collides with DECLL. Verified: CSI 0 q flips the cursor from steady to blinking. Same defect class as the PR's subject, on the intermediate-byte axis instead of the marker axis, and it contradicts the summary this PR added two lines above the map.
• FIXES.md:731 — "What is mapped now" omits >q, and FIXES.md:742 claims the dispatcher no longer re-decides which command it holds, which InputHandler.cs:880 contradicts.
• InputHandler.cs:876 — the comment (and the switch's implicit default arm) guards against CSI ? Ps q reaching the DECSCUSR case. After this change it cannot; ?q is not in the map.

Low

• CsiCommand.cs:119 — the reworded doc calls CSI c / CSI > c the replies. They are the requests.
• CsiCommandExtensionsTests.cs:65 — the "stays Unknown" theory is a hand-written list, not derived from the map.

Notes — what I checked that is fine

• Enumerated the whole map, not spot-checked. Every private form main currently reaches via TrimStart was compared against the new exact table. The eight legitimate shared-handler forms are all listed (?J DECSED, ?K DECSEL, ?h DECSET, ?l DECRST, ?n DEC DSR, ?$p private DECRQM, >c DA2, plus ?S now split off to its own command). Everything else that changes is a form that had no business resolving at all: ?s, ?r, >m, >n, >t, >T, ?c, =c, and the ?<letter> combinations for @ A B C D E F G H I L M P T X Z b d f g. None of those is a sequence this terminal implements. No legitimate private form is lost.
• Conflict with #48: none remaining. #48's exact-match-first is subsumed — the exact match is now the only lookup — and the four Kitty identifiers are carried over verbatim.
• Hot path / allocation: measured, not reasoned about. 1.3M ToCsiCommand calls over a mix of 13 plain and marked identifiers allocate 0 bytes on this branch against 45.6 MB on main (main still runs TrimStart for every marked identifier that misses the exact map — ?h, ?l, >c, ?$p, >q, ?S, >t, ?r all miss today). This is strictly better than #48, not a reintroduction. new(StringComparer.Ordinal) keeps the non-randomised string fast path.
• The >q entry is load-bearing and covered: I removed it against the merged tree and VersionReportTests goes red on three tests. XTVERSION survives the merge.
• dotnet test src/XTerm.NET.slnx → 1597 passed, 0 failed at 9ecd342.
• The dispatch tests are real, not vacuous: each one fails if the map entry is reverted (>4;2m sets underline, >0q sets CursorBlink, >2t raises WindowMinimized, ?1049r homes the cursor). XtSmTitle_... and Xtsmgraphics_... also assert the unmarked twin still works, which is the half that is easy to forget.
• IsPrivateMode still returns false for < and = while its summary says "represents a DEC private mode sequence" — the map now uses both markers. Harmless today (<u/=u route to commands that never consult the flag) and pre-existing, but the summary overpromises. PrivateMarker is the one to reach for.
• Moving GraphicsAttributes up in the CsiCommand enum renumbers eleven members. Checked: nothing casts CsiCommand to or from an integer, and it is not persisted. Safe.