lint: flag defined names Excel repairs — fold-duplicate and definitely-illegal names (GH-528, GH-529)

[arcaputo3]

Fixes #528. Fixes #529. Stacked on #527 (same lint module) — retarget to main after #527 merges.

The incident (second Excel-repair corruption in two days)

A deliverable assembled from a June template carrying a 25,602-name legacy fossil corpus triggered Excel's repair dialog: Removed Records: Named range from /xl/workbook.xml part (Workbook). Forensics (broken vs repaired diff):

• Excel removed 9 named ranges. The decisive three collide with surviving neighbors under Excel's case-, width- and kana-size-insensitive name comparison: g/ｇ, html/ＨＴＭＬ, ぁ/あ. The repaired file holds exactly zero fold-duplicate groups — that is Excel's rule.
• The lint: dangling external-workbook ordinals pass clean — Excel repairs the file by removing every such formula #525 incident file turns out to carry 25 more collision groups (HTML/ＨＴＭＬ across many scopes, f/ｆ, g/ｇ) which Excel removed without logging them — the repair log is not a complete inventory.
• Six more removals are 186-char mixed-script garbage fossils (†, €, U+193A).

Origin forensics — this is inherited, not written by us: the session's own probe of the raw input workbook shows html+HTML+ＨＴＭＬ all present before any xl processing; xl's definedNames regeneration round-trips every removed record byte-identically (verified via a name add probe); no openpyxl saves exist in either session. These are JP-Excel-97-era fossils that banker templates have carried for years — which is exactly why the lint must catch them at the ship gate (both sessions ran xl lint; it passed).

The rules (calibrated false-positive-averse)

New defined-name-invalid category:

1. Fold-duplicate names, same scope — NFKC (width) + ROOT lowercase (case) + small-kana→base-kana table. Scope-aware: the same folded name on different scopes is legal shadowing.
2. Definitely-illegal names — longer than 255 chars, whitespace, control characters.

Deliberate non-goal: emulating Excel's full name-character table. Calibration against the 25k surviving names shows Excel accepts ¤, ・, ･, – while rejecting †, € — legacy Windows NLS classification, not Unicode categories. A category-based rule produces 4 false positives on this single corpus. The 6 garbage fossils therefore stay undetected in v1 (documented in code); the fold-duplicate rule alone already fails both incident files.

Also fixes #529: whitelists the suffix-less Microsoft externalLinkPath rel-type variants (xlStartup, xlLibrary, xlAltStartup) — previously 7 wrong-rel-type false positives on externalLink parts of a file Excel opens without complaint.

Verification

• Incident file: 3 findings, exit 1 (g/ｇ, html/ＨＴＭＬ, ぁ/あ), and the 7 rel-type false positives gone; its Excel-repaired copy: clean, exit 0
• lint: dangling external-workbook ordinals pass clean — Excel repairs the file by removing every such formula #525 incident file: 25 additional collision findings — every one matches a name Excel silently removed; its repaired copy: clean
• 7 new spec tests (collision, kana pair, scope-shadowing negative, 255-limit, whitespace, exact-dup degenerate, suffix-less variants) + 2 new SAX/DOM parity fixtures
• Full suite: ./mill __.test → 1028/1028 SUCCESS

🤖 Generated with Claude Code

[claude]

Review — defined-name-invalid lint (GH-528) + suffix-less rel-type variants (GH-529)

Read the whole diff plus the surrounding lint machinery. The forensics in the description are excellent — "calibrated against 25k surviving names, the repaired copy holds exactly zero fold-duplicate groups" is the right standard of evidence for a heuristic like this, and the explicit non-goal (no Windows-NLS character-table emulation) is the correct call. The check lands in lintSource on the always-DOM workbook part, so SAX/DOM parity is free, and the parity fixtures pin it anyway. Locale.ROOT on the lowercase is exactly right (Turkish-i would otherwise bite).

A handful of things I'd want addressed before merge, one of them a real false-positive source in a PR whose whole thesis is false-positive aversion.

1. NFKC over-folds: it is compatibility decomposition, not width folding (WorkbookLint.scala:1078)

The doc comment says "NFKC supplies the width folding", but NFKC applies the full compatibility decomposition (<circle>, <super>, <square>, <compat>, <noBreak>, …), not just <wide>/<narrow>. So these distinct, Excel-legal name pairs fold together and get reported as collisions:

A	B	folds to	why
Area1	Area①	area1	U+2460 → 1 (<circle>)
M2	M²	m2	U+00B2 → 2 (<super>)
m2	㎡	m2	U+33A1 → m2 (<square>)
No1	№1	no1	U+2116 → No (<compat>)
I	Ⅰ	i	U+2160 → I (<compat>)
ff	ff	ff	U+FB00 → ff (ligature)

Circled digits (①②③) and squared units (㎡ ㎏ ㎜) are extremely common in exactly the JP-legacy corpora this rule targets — 面積① alongside 面積1 is an ordinary sheet, not a fossil. That is a false positive at the ship gate, which is the one outcome the PR is built to avoid.

The narrow fix is to fold only the width/kana classes rather than everything NFKC knows about — e.g. map U+FF01–FF5E → ASCII (c - 0xFEE0), U+FF61–FF9F halfwidth katakana → fullwidth (the voiced-mark composition is the only fiddly part), and leave every other codepoint alone. That keeps ＨＴＭＬ/html, ｇ/g and the kana table while dropping the compatibility-decomposition blast radius. Cheaper interim option: keep NFKC but apply it per-character, only to characters whose Character.UnicodeBlock is HALFWIDTH_AND_FULLWIDTH_FORMS.

Either way, re-run the 25k calibration corpus afterwards — the numbers to confirm are that the surviving-name set still yields zero groups and that no ①/㎡-bearing pair has appeared.

2. Collision-finding order is hash-ordered on ties (WorkbookLint.scala:1108)

.sortBy((names, _) => names.headOption.getOrElse("")) sorts on the group's first name only, and sortBy is stable, so ties retain groupBy(...).toVector's order — i.e. immutable-HashMap iteration order. Ties are reachable: a book carrying g + ｇ at workbook scope and g + ｇ on localSheetId="0" produces two groups both headed "g", and their relative order in the output is whatever the hash layout says (and can shift when an unrelated key enters the map, or on a Scala version bump). Given deterministic output is a stated non-negotiable, make the key total:

.sortBy((names, scope) => (names.headOption.getOrElse(""), scope.getOrElse("")))

3. Messages are unbounded on the collision path

illegal truncates names to .take(40); collisions does not. On the incident corpus those two facts combine badly: a group of n fold-equal 186-char fossil names emits one message containing all n of them in full. Worth capping symmetrically — first ~5 names, each .take(40), plus +N more. Separately, .take(40) has no truncation marker, so Defined name "…40 chars…" is 256 characters long reads like a contradiction; appending … when truncated fixes it.

4. Control characters are echoed raw to the terminal

LintCommands.renderText (LintCommands.scala:19) interpolates f.message straight to stdout, and the contains control characters finding puts the offending name inside that message. So linting a hostile or merely broken third-party workbook can emit ESC sequences to the user's terminal — from the one finding whose entire premise is "this string contains control characters". JSON output is safe (ujson escapes). Suggest escaping non-printables to \uXXXX when building the message (belt-and-braces: in renderText too). Low severity, but this is a "run it on files you did not write" tool.

5. Smaller things

• Missing/empty name (:1102): getOrElse("") means two <definedName> elements with no name attribute report as a collision of ""/"", while a single empty name — which Excel also refuses — goes unflagged. An explicit "has no name" finding reads better than folding it into the collision path.
• kanaBaseFold construction (:1069): grouped(2).map(p => p(0) -> p(1)) throws StringIndexOutOfBoundsException during object initialization if that literal ever ends up odd-length, surfacing as ExceptionInInitializerError from WorkbookLint rather than an XLResult. The 25 pairs read correctly today (ぁあ … ヶケ, both scripts covered), but given the totality charter an explicit Map('ぁ' -> 'あ', …) — or .grouped(2).collect { case s if s.length == 2 => s(0) -> s(1) } — removes the trap for the next editor.
• One finding per problem (:1128): a name that is both over-long and whitespace-bearing yields two findings for one name. Defensible, but neither documented nor tested; joining the problems with ; into a single finding may read better at the ship gate.
• Plural agreement: "$n defined names collide … — Excel repairs the file by removing the named range" — singular tail, plural subject. "by removing all but one of them" is both grammatical and more accurate about what Excel actually does.
• name.length vs code points (:1122): counts UTF-16 units, so 200 astral characters trip the 255 cap. Almost certainly matches Excel (UTF-16 internally too), but worth a word in the comment since it is a deliberate choice.

6. GH-529: consider a prefix predicate over an enumerated set

externalLinkPathTypes (:519) is now seven literals, and this PR exists partly because the previous four did not cover the wild. Every Microsoft member is …/relationships/xlExternalLinkPath/<something>, and the test is only ever "is this an acceptable externalBook path type" — a startsWith on that family prefix (plus the ECMA type) would have pre-empted #529 and the next variant too. That means widening RelRef.expectedTypes from Set[String] to a predicate, so it is a follow-up rather than a blocker — but enumerate-one-per-incident is going to keep costing field incidents. If the set stays, a one-line comment on why both xlStartupPath and xlStartup exist (which producer emits which?) would help the next reader.

7. Test coverage

The seven new tests hit the important shapes, and including a scope-shadowing negative is the right instinct. Gaps I would fill:

• The NFKC boundary — a negative test pinning Area1 vs Area① (and m2 vs ㎡) as not a collision. That is the calibration claim itself; without it, item 1 can regress silently in either direction.
• Halfwidth katakana — ｱ vs ア is the headline width claim for kana, but the only width test uses fullwidth ASCII ｇ. Add a halfwidth-kana pair, ideally a voiced one, since that is where per-character width folding gets interesting.
• Control characters — documented in cli.md, the Main.scala help text and the CHANGELOG; untested.
• Multi-scope determinism — the item-2 case (same head name, two scopes) asserting a fixed finding order.
• A >2-member group — the message says "$n defined names"; only n = 2 is exercised.
• Whitespace flavors — U+00A0 is not Char.isWhitespace in Java, so it slips the check today, while NFKC would have mapped it to a plain space. Whichever behavior you want, pin it; right now it is accidental.

Open question

Windows' Japanese string comparison also has NORM_IGNOREKANATYPE (hiragana ≡ katakana). Does the 25k corpus contain a surviving あ/ア pair in the same scope? If it does, that is good evidence Excel's name comparison does not ignore kana type and the current fold is complete — worth recording in the comment either way, since it is the obvious next question a reader will have about the fold's shape.

Verdict

The rule is the right rule, the calibration methodology is exemplary, and the docs/CHANGELOG/skill/help updates are complete and consistent (the 0.19.2 section placement matches the stacked #525 entry, and plugin/skills/xl-cli/SKILL.md is in sync). Item 1 is worth fixing before this ships — a lint that flags 面積① next to 面積1 gets switched off, and then it catches nothing at all. Items 2–4 are quick. The rest is polish or follow-up.

---

🤖 Reviewed with Claude Code

[claude]

Review: defined-name-invalid lint (GH-528) + suffix-less MS rel-type variants (GH-529)

(Comment is deliberately ASCII-only - my tooling rejected the fullwidth/kana literals, so characters are named by code point.)

Strong PR. The forensic write-up in the scaladoc - why the fold is NFKC + ROOT-lowercase + an explicit kana table, and why the character-class rule is a deliberate non-goal - is exactly the kind of provenance this codebase should carry, and calibrating against a 25k surviving-name corpus before shipping a rule is the right instinct. The scope-aware grouping (shadowing is legal) with a negative test for it, the SAX/DOM parity fixture, and the fact that the docs anti-drift gate (LintCommandSpec GH-486) is satisfied are all good. The GH-529 whitelist addition is minimal and correctly placed: those three constants are consumed only by externalLinkPathTypes, so there is no second site to keep in sync.

Findings roughly by importance. Nothing here is a blocker except (1).

Note: I reviewed statically - I could not run ./mill in this sandbox, so I am taking the stated __.test result on trust and leaving verification to CI.

---

1. CHANGELOG.md leaks a client name

CHANGELOG.md:24 - "Excel removed the Camco ones without even logging them". This is the only occurrence of that string in the repo, and CHANGELOG.md ships to a public repo and to Maven Central. Every other incident reference in this file is anonymous ("the cross-workbook sheet-transplant class", "field incident"). Suggest "the #525 file's" or "the earlier incident's".

2. NFKC is much wider than width folding - a false-positive vector the design explicitly wants to avoid

WorkbookLint.scala:1079 uses full NFKC for the width fold. NFKC compatibility mappings go well past halfwidth/fullwidth:

input	NFKC output
U+FB01 LATIN SMALL LIGATURE FI	fi
U+00B2 SUPERSCRIPT TWO	2
U+2161 ROMAN NUMERAL TWO	II
U+2116 NUMERO SIGN	No
U+2121 TELEPHONE SIGN	TEL

So a workbook holding both Area_m2 and Area_m + U+00B2 (or a ligature spelling next to its ASCII spelling) is reported as an Excel-removal collision that Excel does not actually make. The PR's own calibration shows Excel accepting U+00A4, U+30FB and U+2013 in surviving names, so its acceptance set is clearly wider than the documented rule and superscript/ligature names are not hypothetical. A false positive at a ship gate is expensive: it blocks a deliverable that is fine.

The narrow fix keeps the corpus behaviour intact and cannot touch ASCII-only or Latin-only books: apply NFKC only to names that actually contain a compatibility-width or halfwidth-kana character, i.e. guard with s.exists(c => (c >= 0xFF01 && c <= 0xFF9F) || c == 0x3000) before normalizing (NFKC is still what composes the halfwidth voiced marks in U+FF61..U+FF9F), or fold those ranges arithmetically. Either way, a regression test pinning the ligature/ASCII pair as not a collision would lock the decision down.

3. Possible false negative: hiragana vs katakana

That U+3041 (small a) folds to U+3042 (a) is telling - small-vs-large kana is a tertiary-level difference, so Excel is almost certainly doing a Windows linguistic comparison at primary strength (the NORM_IGNORECASE | NORM_IGNOREWIDTH | NORM_IGNOREKANATYPE family). If so, hiragana equals katakana too (U+3042 == U+30A2), and the current table - which maps small kana only within each script - misses that class silently. Worth one Excel experiment: a book carrying the hiragana and the katakana spelling of the same syllable in workbook scope. If Excel repairs it, the fold needs a hiragana-to-katakana step (+0x60) as well. Not asking you to guess, just flagging that the evidence points that way, and a miss here is the same silent-removal class the PR exists to catch.

(Please do not reach for java.text.Collator at PRIMARY strength as the general fix - it folds accented Latin letters onto their base letters and would blow up Latin corpora. The explicit table is the right shape.)

4. Collision-group ordering is not fully deterministic

WorkbookLint.scala:1113 sorts by names.headOption only, over groupBy(...).toVector (unspecified Map iteration order). Two groups can share a first name - e.g. Rate/RATE at workbook scope and Rate/rate at localSheetId=0 - and then finding order falls back to hash order. Determinism is non-negotiable #3 in CLAUDE.md and lint output is consumed by tooling, so .sortBy((names, scope) => (names.headOption.getOrElse(""), scope.getOrElse(""))) (or sorting on the fold key, which is unique per group) is worth the one line.

5. One bad name can produce three findings

WorkbookLint.scala:1126-1141: problems.map { ... } emits a separate Finding per problem, so a 300-char name containing a space is 2 findings and a summary reading "3 findings" implies 3 bad names. One finding per name with problems.mkString("; ") reads truer and matches how externalRefFindings aggregates (one finding per ordinal, carrying a count).

Same block: name.take(40) truncates with no ellipsis, so the evidence for an over-long name looks like a complete name.

6. Length check counts UTF-16 code units, not characters

Option.when(name.length > maxDefinedNameLength) over-counts by one per supplementary-plane character. Not academic for this corpus specifically: CJK Extension B ideographs (U+20000+) are surrogate pairs and turn up in exactly these JP legacy books, so a 130-character name could be reported as 260. name.codePointCount(0, name.length) is a one-line fix, and it is also what "Excel's limit is 255 characters" means in the message.

7. Nameless <definedName> folds to the empty string

XmlUtil.getAttrOpt(dn, "name").getOrElse("") feeds the empty string into the collision grouping, so two <definedName> elements missing name produce 2 defined names collide under Excel's ... comparison ("", "", workbook scope), which reads as a lint bug to whoever hits it. Meanwhile a single nameless one - itself a record Excel repairs away - passes clean. Suggest filtering the empty name out of the collision path, and either flagging it as its own defined-name-invalid or noting in a comment that it is knowingly out of scope.

8. Message wording

"3 defined names collide ... Excel repairs the file by removing the named range" - Excel removes N-1 of N, so "removing all but one" is both more accurate and more actionable (the reader needs to know one name survives). Minor: sheet scope localSheetId=0 could resolve to the sheet name, which is what the user needs in order to go fix it.

9. Test coverage gaps

The 7 new tests cover the happy paths well. Missing:

• control characters - the third illegal branch has no test at all.
• a calibration guard - a name containing the corpus-surviving symbols (U+00A4, U+30FB, U+2013) or the Excel-rejected ones (U+2020, U+20AC) must not be flagged. That deliberate non-goal is the single most likely thing for a future contributor to "fix" into existence; a negative test is what makes the scaladoc binding.
• halfwidth kana (U+FF71 vs U+30A2) - NFKC is what buys this, and it is the width case most specific to the incident corpus.
• ordering - a fixture with two collision groups, or a 3-member group, asserting exact finding order; that is what would catch XLSX Reader + OOXML Quality & Spec Compliance #4.

10. Follow-up worth an issue, not this PR

The forensics justify lint-only for the inherited case, but xl name add can itself mint a fold-duplicate: add HTML to a book already carrying the fullwidth spelling and xl will happily write a workbook Excel repairs. Catching it at the ship gate is a good backstop; refusing to create it is better.

---

Overall the rule is well-chosen and the false-positive discipline is right. (2) and (4) are the two I would want addressed before merge - (2) because it undercuts the PR's own calibration promise, (4) because determinism is a stated invariant - plus (1) for the client name. Everything else is polish.