Fix off-by-one that dropped the 16th SGR trigger parameter

[semx]

Two small fixes in sources/VT100/iTermParser.h. The first one is user-visible; the second is latent.

1. The 16th SGR trigger parameter was dropped

iTermParserAddCSIParameter bailed out once count reached 15:

if (csi->count + 1 >= VT100CSIPARAM_MAX) {   // count >= 15
    return;
}

so it filled only 15 of the 16 slots in CSIParam.p. The terminal's own CSI parser accepts 16 (VT100CSIParser.m:239, param->count < VT100CSIPARAM_MAX), so the trigger and the terminal disagreed about the same parameter string.

The only caller is SGRTrigger.performAction (sources/Triggers/SGRTrigger.swift:136), which then attaches subparameters with iTermParserAddCSISubparameter(&csi, csi.count - 1, sub). So when the 16th parameter was discarded, count didn't advance and its subparameters landed on the 15th parameter — a 24-bit color's components got applied to the wrong parameter.

I verified this by building the real VT100CSIParser.m and the real header helpers into a standalone harness, running the same parameter string through both the terminal parser and a mirror of the SGRTrigger loop:

SGR parameter string "1;2;3;4;5;6;7;8;9;10;11;12;13;14;15;38:2:255:0:0"
  terminal parser  count=16  p=[1,...,15,38] subs: p15.s0=2 p15.s1=255 p15.s2=0 p15.s3=0
  SGRTrigger       count=15  p=[1,...,15]    subs: p14.s0=2 p14.s1=255 p14.s2=0 p14.s3=0
  => MISMATCH

After the fix both sides produce identical output for 15 parameters, 16 parameters, and the subparameter case above. Writing p[15] is in bounds (p has VT100CSIPARAM_MAX = 16 entries), and the 17th parameter is still discarded.

2. iTermParserGetCSISubparameter ignored subparameter_index

i started at 0, the loop returned on the first matching parameter because i == 0, and the i-- after it was unreachable — so the function returned the first subparameter for every requested index. It has no callers today, so nothing is broken by it; I fixed it as a separate commit so it can be dropped independently if you'd rather leave it alone. Verified with the same standalone harness: before the change, indices 0/1/2 all returned the first value; after, they return 11/22/33 as documented.

Notes

• Release note added to docs/notes-3.7.txt for the first fix.
• I could not run tools/run_tests.expect — I don't have a full build of the app on this machine (submodules + make paranoid-deps), so the verification above is from a standalone harness that compiles the real parser sources rather than from the test suite. Happy to add a ModernTests case for this if you'd like one.

[semx]

Update: I've pushed a third commit that makes this a complete fix, and I built the app to check it.

The same corruption existed in the terminal's own parser, not just in triggers. My original patch only moved the boundary: with 17 parameters the 17th is discarded, but VT100CSIParser.m still attached its subparameters to count - 1, so they landed on the 16th parameter. That is remotely triggerable — not trigger-specific — because it happens in the CSI parser itself:

CSI 1;2;3;4;5;6;7;8;9;10;11;12;13;14;15;4;3:5 m
  before: count=16  p[15]=4  with subparameter 5 attached  → dashed underline
  after:  count=16  p[15]=4  no subparameters             → plain underline, as intended

I measured this by compiling the real VT100CSIParser.m and VT100Token.m into a standalone harness and running the same parameter string through both the terminal parser and a mirror of the SGRTrigger loop, before and after the change. Both agree now, at 15, 16 and 17 parameters, with and without subparameters.

The fix: parameters past the limit set an overflow flag, so their subparameters are discarded with them, and iTermParserAddCSIParameter returns whether it stored the value so SGRTrigger can do the same. Discarding the overflow atomically also matches what kitty does.

Testing. I added five regression tests to iTerm2XCTests/VT100CSIParserTest.m, next to the existing subparameter test. I could not execute them: that target does not link from the command line, and it fails identically on a clean origin/master, so it looks pre-existing and unrelated to this change:

Undefined symbols for architecture arm64:
  "_OBJC_CLASS_$_IT2ChannelDisconnect", referenced from: ...
  "_OBJC_CLASS_$_IT2Runner", referenced from: ...

(tools/run_tests.expect can't reach it either — iTerm2XCTests isn't a member of the ModernTests scheme.) Happy to move the tests to ModernTests instead if you'd prefer them somewhere they run today.

make Development succeeds with this branch: ** BUILD SUCCEEDED **, no errors or new warnings.

[gnachman]

Thank you, @semx, for the careful fix and the standalone-harness verification writeup. This is exactly the kind of subtle parser off-by-one that is easy to miss, and pairing it with tests plus a release note made it easy to review. Merged. 🙏

[semx]

Thanks for catching the blank-16th case — that was a real hole in my patch and I should have covered it. I reproduced it against my merged commit and verified your fix with the same standalone harness I used originally, compiling VT100CSIParser.m at 2729db2 and at fd151a4 and running the same input through both:

CSI 1;…;15;;:5 m
  2729db2 (mine): count=16 p=[…,15,0] subs=0            ← subparameter lost
  fd151a4 (fix):  count=16 p=[…,15,0] subs=1: p15.s0=5  ← kept

I then ran the whole boundary set against your version to make sure nothing I'd fixed regressed:

input	result
4:3	p0.s0=3 — ordinary subparameter unaffected
16 numeric parameters	all 16 stored
16th carries 38:2:255:0:0	4 subparameters on p15
17 parameters, 17th is 3:5	no subparameters on p15 — the original fix holds
blank 16th with :5	kept on p15
blank 17th with :5	dropped

The last row is the one your new test doesn't cover — a doubled semicolon past the limit, where the blank parameter genuinely can't be stored and its subparameter must go with it. It passes today; if you'd like it pinned down, the assertion is iTermParserGetNumberOfCSISubparameters(csi, 15) == 0 for CSI 1;…;15;;;:5 m. Happy to send it as a one-test PR, or feel free to take the line as-is.