lint: flag dangling external-workbook ordinals — the cross-workbook transplant corruption class (GH-525)

[arcaputo3]

Fixes #525. Detection half of #526 (adoption remap API).

The incident

A production agent transplanted four sheets between workbooks via the scripting API (dst.copy(sheets = dst.sheets ++ srcSheets)). Formula text carries external-workbook ordinals [N] that index the source book's <externalReferences> table; the destination declared 1 entry where the source had ≥5. The written file carried 935 formulas referencing [3]/[4]/[5]:

• xl lint (main, 2c3fbcb): clean, exit 0
• Excel: repair dialog; strips all 935 formulas + calcChain (Removed Records: Formula from /xl/worksheets/sheetNN.xml part)

Exactly the lint's charter (GH-397): a corruption class Excel repairs loudly that lenient readers accept silently.

The rule

New external-ref-dangling category: any <f> (and any <definedName>) whose external ordinal [N] exceeds the workbook's <externalReference> count. One finding per part per dangling ordinal — formula count + first offending cell — ordinal-ascending.

Scanner changes (DOM/SAX parity preserved): the SAX scanner previously peeked at only the first character of a cell's first <f> (GH-456). It now accumulates the full text (bounded by Excel's formula-length limit — streaming stays O(1) in rows) and derives both leadingEquals and the external ordinals from it, matching the DOM scanner's Elem.text exactly. Facts fold per part; findings materialize at workbook level where the declared count is known.

Ordinal extraction is false-positive-averse by construction:

• double-quoted string literals skipped ("" escape) — "see [3]" never counts
• quoted sheet names only yield an ordinal immediately after the opening apostrophe ('' escape); Excel forbids [/] in sheet names so no other bracket occurs inside
• a bracket directly preceded by an identifier char or ] is a structured reference — skipped to its matching close, nesting-aware, so Table1[3] / Table1[[#This Row],[3]] (a column literally named "3") and R1C1-style text never count
• non-numeric brackets in reference position ([Book1.xlsx]Sheet1!A1 file-name form) are skipped: lint is not a syntax validator
• ordinal 0 is the self-workbook

Verification

• Incident file (broken): 6 findings — [3]×1, [4]×3, [5]×160 on the first transplanted sheet; [5]×309/231/231 on the other three — summing to exactly the 935 formulas Excel's repair removed (pinned by forensic diff of broken vs repaired packages)
• Excel-repaired copy of the same file: clean, exit 0
• 7 new spec tests (extraction precision incl. every false-positive form; flagged/clean/no-table fixtures; defined names; streaming equivalence) + 4 new SAX/DOM parity fixtures
• Full suite: ./mill __.test → 1028/1028 SUCCESS

Out of scope

Ordinals that happen to resolve in the destination silently rebind to a different external workbook — statically undetectable from one file. That half needs the #526 adoption/remap API.

🤖 Generated with Claude Code

[claude]

Review: external-ref-dangling (GH-525)

Strong PR. The incident write-up, the forensic pinning of exactly 935 formulas against Excel's repair log, and the decision to fold facts per part and materialize findings at workbook level (where the declared count lives) are all the right calls. Making the SAX scanner accumulate the full <f> text also strictly improves the GH-456 parity story rather than bolting the new rule on beside it.

One correctness issue worth fixing before merge, plus bounding/perf/coverage notes.

---

1. False positive: unqualified structured refs to numeric-named columns (WorkbookLint.scala:1126)

externalOrdinals classifies a [ as a structured reference only when the preceding character is an identifier char (isIdentChar, line 1071). That covers the qualified form Table1[3] and the nested form Table1[[#This Row],[3]] (both tested), but not the unqualified form Excel stores for formulas written inside a table:

SUM([2024])            prev = '('         -> not ident -> ordinalAt -> "2024" all digits -> ordinal 2024
[2024]-[2023]          prev = None / '-'  -> ordinals 2024 and 2023
SUBTOTAL(109,[2025])   prev = ','         -> ordinal 2025

Excel preserves unqualified structured refs verbatim in <f> when the formula lives in the same table, and purely numeric column headers (years, month numbers) are ubiquitous in exactly the financial models this library targets. [@2024] is safe ("@2024" is not all digits), but the whole-column form is not. Result: xl lint exits 1 on a healthy workbook and tells the user Excel is about to delete their formulas, which is the failure mode this rule's design notes work hardest to avoid.

Suggested fix, which kills the class rather than patching a case: require the ordinal bracket to be followed by a sheet-name run and a !. Every legitimate external form has one:

form	after ]
[5]Data!B2	Data then !
[2]!ExtName	! immediately
'[3]Sheet'!A1	! after skipQuotedName
[1]S1:S3!A1	S1:S3 then !
[1]#REF!A1	#REF then ! (worth remembering when picking the charset)

None of [2024]-, [2024]), or [2024] at end-of-text do. It is a few lines in both the '[' branch and the '\'' branch, and it makes the rule positively specified ("this looks like an external reference") instead of negatively specified ("this does not look like anything else"), which is the more durable posture for a false-positive-averse lint.

Worth adding the three forms above to the externalOrdinals spec test alongside the existing Table1[3] cases.

2. Defined-name findings are unbounded (WorkbookLint.scala:1038)

definedNameExternalRefFindings emits one Finding per (name, ordinal), while every other rule in this file aggregates: leading-equals is one finding per part with a 5-cell sample plus a total count, and the new formula rule is one per part per ordinal. A transplanted workbook usually carries the source book's external defined names too, so the population that triggers this rule is the same population that makes it verbose; a book with 300 dangling names emits 300 findings.

Mirroring LeadingEqualsFacts (aggregate per ordinal, carry a first-N name sample plus count) would keep the rule consistent with its neighbours. The same idea applies more weakly to externalRefFindings: bounded by distinct ordinals, which is small for real files but scales with cell count for a garbled part. A top-K cap with an "and N more" tail would close it.

3. The streaming O(1) claim is not enforced (WorkbookLint.scala:823, 893)

Bounded by Excel's formula-length limit, so the streaming mode stays O(1) in the row count.

Nothing enforces that bound: characters() appends unconditionally, so a malformed or hostile part with one giant <f> grows the builder without limit. That matters more here than elsewhere because --stream exists precisely for files too large to hold in memory, and --max-size 0 is a documented escape hatch. A hard cap on the append (say 32 KB, 4x Excel's 8192-char limit) makes the comment true at negligible cost; truncation can only lose ordinals inside text Excel itself would reject. If you do cap, cap the DOM side identically so the parity fixtures keep meaning what they say.

4. Per-formula allocation cost

externalOrdinals now runs over every formula's full text on every lint, where streaming mode previously looked at one character. Three cheap wins, in rough order of payoff:

• Fast path: if formula.indexOf('[') < 0 then Set.empty skips the char loop for the overwhelming majority of formulas.
• prev: Option[Char] (line 1110) allocates a Some per character scanned. A plain Char sentinel (one that isIdentChar rejects) removes that allocation entirely with no behaviour change.
• new java.lang.StringBuilder per <f> (line 871): one reusable builder reset with setLength(0) at each formula start avoids an allocation per formula cell.

On the 1M-formula workbooks --stream is built for, these are the difference between free and noticeable.

5. Test coverage gaps (small)

• declaredPhrase's plural branch (only N <externalReference> entries, N >= 2) is never exercised; every fixture declares 0 or 1.
• The ordinal clamp path (digits.toLongOption overflow to Int.MaxValue) has no test; [99999999999999999999] would cover it.
• No fixture has a formula cell without r=, so the first.fold("<f>") locator branch is untested. The leading-equals rule documents that case explicitly; this one does not.
• The unqualified structured-ref forms from section 1.

6. Nits

• first.orElse(obs.ref) (line ~995) makes "first at X" mean first offender that carries an r attribute, not first offender. Harmless, but LeadingEqualsFacts documents the analogous case and this one does not.
• Map[Int, (Long, Option[ARef])] reads awkwardly in the fold; a two-field case class (Use(count, first)) would match how RecordFacts / LeadingEqualsFacts are written.
• externalOrdinals is the only member widened to private[lint]; a one-line "widened for spec-level unit tests" note would save the next reader a lookup.
• docs/STATUS.md:23 mentions the lint rules for Scripting formula writer emits leading '=' inside <f> (CLI putf writes clean) #456 and was not updated. Optional, since it is not an exhaustive category list.

---

Docs, CLI help text, SKILL.md rule list, and CHANGELOG are all updated consistently, and the four new SAX/DOM parity fixtures are exactly the right ones to have added. Section 1 is the only item I would consider blocking.

Note: I reviewed by reading the diff and tracing externalOrdinals by hand. The build was not runnable in this environment, so the section 1 trace comes from the source rather than an executed test.