Fix #262

[bernalalexis-try]

I'd add a TabStops struct in emu/seq.rs holding one flag per column plus a TabOp, handling HT, HTS, TBC, CHT and CBT, with RIS/DECSTR restoring the every-eight default and set_cols on resize.

In vt100.rs, apply_tabs would read the cursor column and rewrite movement as CSI n G, covered by a new tests/tabs.rs.

[vyncint]

Welcome, and thank you for taking this one on — it is one of the harder issues in the queue, and the part you have written is genuinely good.

Before the change requests, so you know where you stand: your work is correct. I completed the one missing piece locally and ran it. All ten of your tests pass, the full suite is green, clippy is clean in all four feature configurations, MSRV 1.85 builds, and the docs build with warnings denied. The TabStops design, the TabOp split between tracker and emulator, the strictly-left back-tab, the resize rule, the clamping — all of it is right, and the comments explain why in the way this repository asks for. Your CHANGELOG entry is in [Unreleased], which four other open PRs got wrong today.

So this is not "start over". It is "one file did not make it, and the way you uploaded it needs to change".

---

1. The blocker: crates/termlens/src/emu/vt100.rs is not in the PR

Your PR changes five files. The sixth one — the emulator half your docs/DESIGN.md paragraph describes in detail — is missing, so the crate does not compile:

error[E0061]: this function takes 2 arguments but 1 argument was supplied
  --> crates/termlens/src/emu/vt100.rs:72:22
   |
72 |             tracker: SeqTracker::new(capture),
   |                      ^^^^^^^^^^^^^^^--------- argument #2 of type `u16` is missing

error[E0004]: non-exhaustive patterns: `SeqEvent::Tabs(_)` not covered
  --> crates/termlens/src/emu/vt100.rs:230:30

That single failure is why clippy, test, features, msrv and skill are all red. They are not five problems; they are one. (skill compiles the agent guide's examples against the crate, so it fails whenever the crate does.)

If you still have your local vt100.rs, include it and skip to §2 — but please check the ordering note at the end of this section first, because it is the one place this is easy to get subtly wrong. If you need to rebuild it, it is three edits.

Edit 1 — the constructor, around line 72. The tab-stop set is one flag per column, so it needs the width:

tracker: SeqTracker::new(capture, cols),

Edit 2 — set_size, around line 413. The set has to follow a resize:

fn set_size(&mut self, rows: u16, cols: u16) {
    self.parser.screen_mut().set_size(rows, cols);
    self.shadow.set_size(rows, cols);
    // The tab-stop set is one flag per column, so it follows the width.
    self.tracker.set_cols(cols);
    // A resize can push rows into history on its own.
    self.capture_scrolled_rows();
}

Edit 3 — a new arm in the match self.tracker.step(byte) inside process, next to the SeqEvent::SoftReset arm:

SeqEvent::Tabs(op) => {
    // Everything *before* this byte, so the column read is the one the
    // operation is relative to. It matters for plain HT alone: vt100
    // acts on it and would have moved the cursor to its own fixed
    // eight before we looked.
    self.feed_staged(&bytes[fed..i]);
    let (_, col) = self.parser.screen().cursor_position();
    let target = self.tracker.tab_op(op, col);
    self.feed_staged(&bytes[i..=i]);
    fed = i + 1;
    if let Some(target) = target {
        self.feed(format!("\x1b[{}G", target.saturating_add(1)).as_bytes());
    }
    None
}

The ordering note, which is worth the paragraph. My first attempt fed the operation's own byte before reading the cursor column — the obvious reading of "feed what you owe the parser first" — and seven of your ten tests failed. The reason is the case your own DESIGN paragraph singles out: vt100 ignores HTS, TBC, CHT and CBT, but it does act on a plain HT, so by the time that byte has been fed the cursor has already jumped to vt100's fixed eight and the column you read is the wrong base. Reading before feeding is correct for all five operations, because no escape prefix moves the cursor either. Splitting the feed at i rather than i + 1 is the whole fix, and it is invisible until you run the tests.

That is also the best evidence that your tests are doing their job: they caught a real ordering bug in an implementation that compiled fine.

2. cargo fmt — one spot

crates/termlens/src/emu/seq.rs:1151. rustfmt wants this broken across lines:

let ps = if params_empty { 0 } else { self.csi_first_param };

Do not hand-edit it — run cargo fmt --all and it is done. That is the only formatting complaint in the whole PR.

3. Sign-off, commit messages, and the upload workflow

Your four commits are all titled Add files via upload and none carries a Signed-off-by line, so commit-policy fails. Both come from the same cause: GitHub's web upload button cannot sign off a commit and names them for you. It is also almost certainly why vt100.rs was left behind — five files is one too many to shepherd through that dialog.

The fix is to work from a real clone, which §4 walks through. What CI needs at the end:

• One commit, signed off with git commit -s. That appends Signed-off-by: Your Name <your@email> and is how you certify the DCO — see CONTRIBUTING §5. The sign-off email must match the commit author email.
• A Conventional Commit subject, imperative and under 72 characters. For this change: fix(emu): honour the tab stops an application sets.
• A PR title in the same form. This repository squash-merges, so the PR title becomes the commit subject on main. Fix #262 would land there as-is.

4. Step by step, from where you are

Everything you have already written is recoverable — it is sitting on main in your fork. This pulls it onto a proper branch, adds the missing file, and updates this same PR.

# 1. A real clone (the web UI cannot sign a commit).
git clone https://github.com/bernalalexis-try/termlens
cd termlens
git remote add upstream https://github.com/vyncint/termlens
git fetch upstream

# 2. A branch off current upstream main. Working on your fork's `main`
#    is what left you with no way to keep this PR separate from the next.
git switch -c fix/262-tab-stops upstream/main

# 3. Recover the five files you already uploaded.
git checkout origin/main -- CHANGELOG.md README.md docs/DESIGN.md \
    crates/termlens/src/emu/seq.rs crates/termlens/tests/tabs.rs

# 4. Make the three edits from §1 to crates/termlens/src/emu/vt100.rs.

# 5. Format, then run what CI runs.
cargo fmt --all
cargo test --workspace --all-features
cargo clippy --workspace --all-targets --all-features -- -D warnings

# 6. One signed-off commit.
git add -A
git commit -s -m "fix(emu): honour the tab stops an application sets"

# 7. Check the two policy gates before pushing — they take a range.
.github/scripts/check-dco.sh upstream/main..HEAD
.github/scripts/check-no-ai-attribution.sh upstream/main..HEAD

# 8. Update this PR in place.
git push --force-with-lease origin fix/262-tab-stops:main

Step 8 pushes your branch onto your fork's main, which is what this PR tracks, so #273 updates rather than a new one opening. If you would rather start a clean PR, push to fix/262-tab-stops instead and open one from that branch; close this one and say so, and nothing is lost.

Then edit the PR title to fix(emu): honour the tab stops an application sets, and rewrite the description to say what the PR does rather than what you would do — the current text is in the conditional ("I'd add a TabStops struct…"), which reads as a plan rather than a change. Closes #262 in the body links them so the issue closes on merge.

The full gate list is in CONTRIBUTING §1. Everything there is reproducible locally, which is the point of writing it out.

5. Two small things for when the rest is green

• docs/DESIGN.md. Your paragraph says the emulator resolves a TabOp "feeding the bytes it still owes the parser first, so the position it reads is current". After §1 you know that is precise only if "owes" excludes the operation's own byte. One clause naming plain HT as the reason would save the next reader the hour it cost me.
• tests/tabs.rs. Ten cases, and the one gap is the resize rule you documented in three places. I checked it by hand and your implementation is right — a stop set at column 3, then a widen from 24 to 40 columns, keeps the custom stop and gives the new columns the every-eighth pattern. It deserves a test, because it is the rule most likely to be broken by a later change and the only one nothing currently pins.

---

None of this is a reflection on the work. The tracker, the tests and the docs are the hard three-quarters and you got them right; what is left is one file and the mechanics of getting it here. Push when you are ready and I will re-review.

[bernalalexis-try]

Implements custom tab stops for #262. The stop set now lives in seq.rs, and the emulator rewrites every tab movement as CHA, including plain \t. Added eleven tests plus documentation.

Following review, I added the missing vt100.rs changes and fixed formatting, docs, and a resize test.

[vyncint]

This is a big step forward, and you got the hard part right.

The missing vt100.rs is here, and — the part I want to call out — you found the ordering trap on your own and explained it in the doc comment on apply_tabs better than my review did. "Reading first is correct for all five, since no escape prefix moves the cursor either" is exactly the reasoning, and it is the sentence a future reader needs. You also added seven unit tests including the shadow-parser one, and the resize test I asked for.

I ran your branch against current main: cargo fmt, clippy with -D warnings in every feature configuration, the full workspace suite, and MSRV 1.85 all pass, and ten of your eleven tab tests pass. The implementation is finished and correct. What is left is one test line and the git mechanics.

---

1. One failing test, and it is not the behaviour

a_resize_extends_the_stops_and_keeps_the_ones_it_had fails — but look at what it caught:

assertion `left == right` failed: the custom stop survives:
size: 40x4  cursor: 3,4
READY
   a                            b
DONE
  left: Some((2, 3))
 right: Some((1, 3))

The column is 3, which is what the test is about: the custom stop survived the widen. The row is 2 where you expected 1, because the shell echoes the Enter that releases read _, and that echo costs a line. Your resize rule works; the assertion is just pinned to a row it does not care about.

The fix is one you already built. Line 25 of that file has col_of, with a comment saying a column is the only thing these tests assert — and every other test in the file uses it. This is the one place that calls find directly:

// crates/termlens/tests/tabs.rs, in a_resize_extends_the_stops_and_keeps_the_ones_it_had
assert_eq!(col_of(&s, "a"), Some(3), "the custom stop survives:\n{s}");
assert_eq!(
    col_of(&s, "b"),
    Some(32),
    "and column 25 tabs on to the every-eighth stop at 33:\n{s}"
);

I applied exactly that locally and the file goes to 11 passed, 0 failed, and the whole workspace stays green.

Worth keeping the reasoning in mind rather than just the edit: a row number in a PTY test depends on how much the shell echoed before your output, which is not what any of these tests are measuring. Your helper already encodes that judgement. This assertion just did not go through it.

2. The sign-off, which is now the only other blocker

commit-policy still fails. Your six commits are all Add files via upload with no Signed-off-by line, because GitHub's web upload button cannot add one — it writes the message for you and commits as GitHub rather than as you.

Everything else on the PR is green now, so this and §1 are the whole remaining list.

3. main has moved, so a rebase is needed

Six PRs merged today, including four that touched [Unreleased] in CHANGELOG.md. Your entry will conflict there. Nothing else in your PR conflicts — I checked by merging it onto current main file by file.

When you resolve it, put your entry under the existing ### Fixed heading, after the docs.rs one. [Unreleased] now has Added, Changed and Fixed sections; keep that order.

4. Step by step

Your work is all in your fork, so this recovers it, adds the fix, and lands it as one signed-off commit.

# 1. A real clone. The web UI cannot sign a commit, which is the blocker.
git clone https://github.com/bernalalexis-try/termlens
cd termlens
git remote add upstream https://github.com/vyncint/termlens
git fetch upstream

# 2. A branch off current upstream main.
git switch -c fix/262-tab-stops upstream/main

# 3. Recover the six files you uploaded.
git checkout origin/main -- CHANGELOG.md README.md docs/DESIGN.md \
    crates/termlens/src/emu/seq.rs crates/termlens/src/emu/vt100.rs \
    crates/termlens/tests/tabs.rs

# 4. Reopen CHANGELOG.md. Your entry came from a main that no longer
#    exists, so move it under the current "### Fixed", after the docs.rs
#    entry, and delete any duplicated headings step 3 brought with it.

# 5. Make the one-line fix from §1 in crates/termlens/tests/tabs.rs.

# 6. Run what CI runs.
cargo fmt --all
cargo test --workspace --all-features
cargo clippy --workspace --all-targets --all-features -- -D warnings

# 7. One commit, signed off. -s is what adds the Signed-off-by line.
git add -A
git commit -s -m "fix(emu): honour the tab stops an application sets"

# 8. Check the policy gates yourself; both take a range.
.github/scripts/check-dco.sh upstream/main..HEAD
.github/scripts/check-no-ai-attribution.sh upstream/main..HEAD

# 9. Update this PR in place.
git push --force-with-lease origin fix/262-tab-stops:main

Step 9 pushes onto your fork's main, which is the branch this PR tracks, so #273 updates rather than a new one opening.

Then two edits on the PR page itself:

• Title → fix(emu): honour the tab stops an application sets. This repository squash-merges, so the PR title becomes the commit subject on main; Fix #262 would land there verbatim.
• Description → say what the PR does rather than what you would do. It is still in the conditional ("I'd add a TabStops struct…"), which reads as a proposal. A few sentences on what is implemented, plus Closes #262 so the issue closes on merge.

5. One optional thing

docs/DESIGN.md still says the emulator resolves a TabOp "feeding the bytes it still owes the parser first". After what you worked out, you know that is precise only if "owes" stops short of the operation's own byte — and your apply_tabs comment now says exactly why. One clause there pointing at plain HT would make the design note as clear as the code. Not a blocker; take it or leave it.

---

To be clear about where this stands: the emulation, the tests and the docs are done and they are good work. What is between here and merge is one assertion and a git commit -s. Push when you are ready and I will re-review.