Wave 20: CLI & fidelity correctness — numFmt parity, batch/arg ergonomics, sheet lifecycle, comment/sheetFormatPr round-trip

[arcaputo3]

Burn-down wave 2 of 3 (plan of record 2026-07-22): nine issues across six worktree-isolated TDD clusters, pipelined adversarial review — all six approved with zero rework rounds.

What's in it

• numfmt-parity (streaming: StylePatcher builtin numFmt-id table diverges from NumFmt.builtInId (Decimal/Percent/Currency render wrong formats) #408, display: unify NumFmtFormatter built-in enum arms onto FormatCodeParser (PercentDecimal renders 15.6% vs Excel's 15.60%) #410 — serial): StylePatcher deletes its hand-rolled builtin-id table and delegates to NumFmt.builtInId (Decimal→2, Percent→9, Currency→7 — streamed styles no longer render differently from in-memory ones, including --stream put detected formats from Fix positional put smart type detection #438). NumFmtFormatter built-in arms now render via FormatCodeParser(NumFmt.formatCode(fmt)), property-tested identical to the file-declared path (PercentDecimal now shows Excel's 15.60%); display re-pins each justified against the ECMA-376 code.
• cli-putpath (cli: batch put with values[] silently ignores the op-level format field #416, cli: two arg-shape ergonomics nits — recalc's missing -o reports 'Internal:', lint rejects the positional file form #422 — serial): batch put values[] threads the op-level format into every element with exact single-put semantics (in-memory + streaming); all 40 write verbs report <verb> requires -o <out.xlsx> (or -i to modify in place) instead of Internal:; xl lint <file> positional form accepted (exactly-one-file enforced with hints).
• workbook-lifecycle (workbook: sheet-scoped definedNames beyond modeled print names keep stale localSheetId across sheet insert/remove/reorder #434, ooxml: remove-sheet leaves orphaned chart/drawing parts in the package #417 — serial): removeAt/insertAt/reorder remap DefinedName.localSheetId (removed-sheet scopes dropped, Excel semantics); remove-sheet prunes parts reachable only via the removed sheet's rel closure — chart add → remove-sheet → zero orphaned chart/drawing parts, while a shared-image-across-two-sheets fixture keeps the image.
• comment-fidelity (ooxml: comment rewrite prepends a duplicate author-prefix run and drops run color #433): no second synthesized author-prefix run when the comment already carries one; run colors (indexed/raw rPr payloads) survive rewrite byte-faithfully. (Footprint note: the synthesis lived in XlsxWriter.buildCommentsData, not Comments.scala — the cluster extended scope deliberately and flagged it.)
• sheetformatpr (ooxml: Sheet.defaultRowHeight/defaultColumnWidth have no write path (sheetFormatPr never emitted) #426): <sheetFormatPr> emits on both backends from Sheet.defaultRowHeight/defaultColumnWidth, reads back, and preserves unmodeled attrs; untouched sheets stay byte-stable.
• scripting-ergonomics (scripting: Sheet(name)/put(ref) transparent-inline literal-vs-dynamic split is a field footgun #420): Sheet.named(name): XLResult[Sheet] as the documented dynamic-name path; scripting docs/skills explain the literal-vs-dynamic split (markers say "since 0.17.0").

Gates (all green)

• ./mill __.compile 810/810, 0 non-exhaustive-match warnings
• scalafmt CI form — no drift
• ./mill -i __.test 1028/1028 tasks, 4,908 cases (+72 this wave)
• XL_ROUNDTRIP_MIN_SUCCESS=500 generative law 239/239
• ./scripts/test-examples.sh + ./scripts/verify-skill-snippets.sh --local green; xl.docJar verified

Field-repro replays (branch-built CLI)

• values[] + format:"0.00%" → cells render 100.00% (cli: batch put with values[] silently ignores the op-level format field #416)
• recalc without -o → recalc requires -o <out.xlsx> (or -i to modify in place); xl lint file.xlsx positional works (cli: two arg-shape ergonomics nits — recalc's missing -o reports 'Internal:', lint rejects the positional file form #422)
• --stream put B1 45.5% → styles.xml numFmtId 9 (canonical Percent; was 10) (streaming: StylePatcher builtin numFmt-id table diverges from NumFmt.builtInId (Decimal/Percent/Currency render wrong formats) #408)
• chart add → remove-sheet → chart/drawing parts 3 → 0 (ooxml: remove-sheet leaves orphaned chart/drawing parts in the package #417)
• Excel-authored comment (own Reviewer: prefix run + indexed="81") rewrites with exactly one prefix and color intact (ooxml: comment rewrite prepends a duplicate author-prefix run and drops run color #433)

Integration notes

All six clusters merged conflict-free. Follow-up candidates surfaced by the wave (not filed yet, listed for triage): unknown batch format names are silently ignored (warnings-vector candidate); Workbook.apply/upsert have the same literal-vs-dynamic union as #420; streaming extractStyle doesn't carry raw numFmtId onto CellStyle; lint part-reachability check needs carve-out design (not trivial — #413 comment stands).

Closes #408
Closes #410
Closes #416
Closes #417
Closes #420
Closes #422
Closes #426
Closes #433
Closes #434

🤖 Generated with Claude Code

[claude]

Review: Wave 20 — CLI & fidelity correctness (1/2)

Read the full diff (35 files, +2365/−242) against b2028a7. High-quality work: every change is anchored to an ECMA-376 clause or a cross-checked SSF behavior, the "delete the hand-rolled table, delegate to the canonical one" moves (#408/#410) are the right direction for a library whose thesis is law-governed determinism, and the new specs are unusually good — DefinedNameScopeRemapSpec pinning verbatim print names, SheetFormatPrRoundTripSpec pinning that the identity fast-path does not inject customHeight, and the GH-410 property test comparing every enum arm against Custom(NumFmt.formatCode(fmt)) are all the tests I would have asked for.

Findings most-significant first. Nothing here blocks merge; items 1–3 are the ones worth acting on.

---

1. removalOrphanedParts: an unparseable survivor .rels can cause over-pruning

XlsxWriter.scala:729-748 — targetsOf swallows every failure into Set.empty via Try(...).toOption.flatten.map(...).getOrElse(Set.empty). That default is asymmetric across the two walks:

• deleted closure — empty targets ⇒ fewer candidates ⇒ under-prune ⇒ safe (dead weight rides through, as before).
• survivor walk — empty targets ⇒ a surviving part's references become invisible ⇒ anything it shares with the deleted closure gets pruned ⇒ dangling r:id in a part that ships.

parseOptionalEntry returns None on both a parseSafe failure and a Relationships.fromXml Left, and the reader is deliberately lenient about malformed rels (that is what xl lint exists to report), so this is reachable on a real file, not only on I/O failure. The doc comment promises "a part reachable from ANY surviving part survives" — that is the invariant that breaks.

Suggestion: fail-open rather than fail-closed. Either distinguish "no rels part" (legitimately empty) from "rels present but unreadable" and abandon the prune entirely in the latter case, or have the survivor walk treat an unreadable rels as reaching every manifest part. A fixture with a corrupt xl/worksheets/_rels/sheet1.xml.rels plus a shared image would pin it.

2. removalOrphanedParts reopens the source zip once per part

Same function: targetsOf calls withSourceZip(ctx.content), which for SourceContent.OnDisk constructs a fresh new ZipFile(path.toFile) — re-reading the central directory each time. survivorRoots is every surviving manifest part, so the walk opens the zip once per part that has a .rels sibling (workbook, one per worksheet, one per drawing, one per chart). On a 100-sheet, 50 MB book with drawings that is a few hundred central-directory parses on a single remove-sheet.

targetsCache prevents re-opens for the same part but not across parts. Hoisting one withSourceZip around the whole closure computation and threading SourceZip into targetsOf is a small, contained refactor for a large constant-factor win.

3. NumFmtFormatter.builtInFormats is a hand-maintained list — a new NumFmt variant degrades silently

NumFmtFormatter.scala:26-42 builds the map from a literal List(...), and both lookup sites fall back to formatGeneral / dt.toString with // unreachable comments. Combined with the new catch-all case builtin => (which replaced the previously exhaustive arms), adding a NumFmt case — say Accounting — compiles clean and silently renders as General. That is exactly the failure mode GH-404 documents: "a partial mapping with an unknown → General fallback is what silently un-formatted cells".

NumFmt.builtInId is the good pattern: an exhaustive match, so a new variant is a compile error. DisplaySpec.builtInVariants is a third hand-written copy of the list, so the property test cannot catch the drift either.

Suggestion: one canonical NumFmt.builtIns: List[NumFmt] in the companion (Scala 3 gives no values for enums with parameterized cases, so derive it from an exhaustive match, or keep the list beside builtInId with a law test builtIns.forall(builtInId(_).isDefined) plus a pinned size), read by builtInFormats, formatDateTime, and the spec alike.

4. Read/write author-prefix predicates disagree (#433)

XlsxWriter.hasLeadingAuthorPrefix (new, line 640) accepts the bare author name or author:, with no bold or second-run-newline requirement. XlsxReader.authorPrefixMatches (line 715) requires the colon, and stripAuthorPrefix additionally requires bold plus a newline-leading second run.

The wider writer predicate is right for the bug being fixed (Excel-authored prefixes the reader deliberately leaves alone). But the bare-author-without-colon arm creates a new false positive: a comment by author TODO whose text starts with a run TODO now loses its synthesized attribution line, where before this PR it got one. Requiring the colon (matching the reader) closes it; better still, lift one shared predicate — these are two spellings of "is this run an author prefix", and they will drift.

The fix also makes repeated round-trips of XL-written comments idempotent even when stripAuthorPrefix declines (non-bold first run) — a real side benefit, worth a write-read-write test pinning prefix count at 1 for the non-strippable shapes.

5. NumFmt write-id vs. display-code still diverge for Currency and Date

This wave's thesis is "the id we write and the code we display are the same format". Two of the fourteen built-ins still fail that, and #408 newly propagates them into the streaming path:

variant	builtInId	ECMA-376 §18.8.30 code for that id	formatCode
Currency	7	$#,##0.00_);($#,##0.00)	$#,##0.00
Date	14	mm-dd-yy (en-US Excel renders m/d/yyyy)	m/d/yy

So -1234.56 styled NumFmt.Currency displays -$1,234.56 in xl view/HTML but opens in Excel as ($1,234.56); a NumFmt.Date cell displays 11/21/25 but opens as 11/21/2025. The GH-410 property test cannot see this — it compares the enum arm against Custom(formatCode(fmt)), both sides of the same code.

The missing law is the cross-check: for every built-in, formatCode(fmt) should equal the ECMA code for builtInId(fmt). Since no built-in id carries a plain $#,##0.00, Currency probably wants to be emitted as a declared numFmt entry (like Custom) rather than mapped to 7. Follow-up-sized, but same class as #408/#410 and this wave is the natural place to record it.

[claude]

Review: Wave 20 — CLI & fidelity correctness (2/2)

6. findOrAddNumFmt can allocate a custom id inside the reserved 0–163 range

StylePatcher.scala:319-323 (untouched here, but newly more reachable) computes the next custom id as existingIds.maxOption.map(_ + 1).getOrElse(164). If the source numFmts block declares only low ids, that lands inside the built-in reserved range.

Your own new fixture in StylePatcherSpec.scala:117 is exactly such a file (a single declared entry at numFmtId="10") — adding any other custom code to that workbook allocates id 11, overriding built-in Scientific. And #408's extractNumFmt change (declared entries now become Custom even at built-in ids) routes more files through this path than before.

One-line fix using the constant that already exists for this: clamp with .max(NumFmt.FirstCustomId), and replace the two bare 164 literals in that method with NumFmt.FirstCustomId while you are there.

7. The bare-h 24-hour fix has a wider blast radius than the CHANGELOG suggests

renderDateToken's new twelveHour gate is spec-correct (§18.8.31) and well tested, but it changes applyDateFormat for every custom code containing h/hh, not just the NumFmt.DateTime/Time enum arms — so xl view --eval, toHtml, toSvg, and PNG output of any cell styled "h:mm" all shift from 2:30 to 14:30. Similarly NumFmt.Date on a negative serial now renders ###### instead of a fabricated 1899 date.

Both are corrections, but they are silent visible changes to public rendering, and the CHANGELOG files them only under the #410 "built-in display arms" bullet. A ### Changed entry calling out the custom-code reach would save someone a bisect.

8. requireOutput verb names are 40 hand-written string literals

The #422 fix is a real UX win, but requireOutput("cf add", …), requireOutput("chart add", …), requireOutput("import-md", …) duplicate names that already exist as Opts.subcommand labels, with nothing tying them together — a renamed subcommand leaves a stale error message. Two of the forty are test-pinned (recalc, unfreeze) despite the PR body's "all 40 write verbs" claim.

Either derive the name from the CliCommand case (a total def verbName(cmd: CliCommand): String, which the compiler then keeps exhaustive) or add a table-driven test walking the write commands and asserting each message starts with its expected verb. The latter is cheap and makes the claim enforceable.

9. The missing--o error fires only after the whole workbook is read

runWithOutput(None, false) passes None straight through to run(...), which reads the file before executeCommand reaches requireOutput. So xl -f huge.xlsx recalc parses 100 MB and then prints the usage error. Since #422 is specifically about this interaction being confusing, checking the write-verb/output pairing at the Opts layer (or right after arg parsing, before the read) would make the fix land where users feel it.

10. Smaller notes

• parseFormatName now has three identical call sites (BatchParser.scala:332, 345, 710) computing the op-level format the same way. Worth one private def opFormat(objMap) — especially since the PR already flags "unknown batch format names are silently ignored" as a warnings-vector follow-up, and that fix will now need to land in three places.
• mergeSheetFormatPrElem writes Double.toString: a modeled defaultRowHeight = 15.0 emits defaultRowHeight="15.0" where Excel writes 15. Valid xsd:double, and the identity fast-path keeps preserved elements byte-stable — but the DOM and streaming backends can then disagree byte-for-byte for the same model, since DirectSaxEmitter always passes existing = None and so never hits the fast-path. A small canonicalizer (if h == h.floor then h.toInt.toString else h.toString) closes it.
• mergeSheetFormatPrElem width-only over a preserved element lacking defaultRowHeight: the backfill is gated on existing.isEmpty, so this path emits a sheetFormatPr missing the attribute your own doc comment calls required. Only reachable from a malformed source (use="required"), and "preserved elements are never healed" is defensible — just flagging that the comment and the code disagree about whether that is OK.
• remapDefinedNameScopes does not touch refersTo: removeAt correctly drops sheet-scoped names, but a workbook-scoped name whose formula points at the removed sheet (e.g. Global -> "Alpha!$A$1") rides through dangling; Excel rewrites such names to #REF!. The spec's Global name is "0.08", so this is not exercised. Same family as the documented rename limitation — follow-up, not a blocker.
• CHANGELOG cross-refs: the streaming: StylePatcher builtin numFmt-id table diverges from NumFmt.builtInId (Decimal/Percent/Currency render wrong formats) #408 bullet cites "cli: positional put bypasses smart detection — ISO dates land as text; format:"date" on non-ISO yields Text wearing a Date numFmt #431" where the PR body says Fix positional put smart type detection #438 (--stream put detected formats); the cli: batch put with values[] silently ignores the op-level format field #416 bullet cites "Fix positional put smart type detection #438 explicit-date rejection". One of those is a typo.
• reorder's newOrder.indexWhere cannot return -1 given the permutation guard above it — correct, but a one-line comment saying so would spare the next reader the check.

---

Checked and found correct

• applyScientificPattern: e10 = precision - scale - 1 is an exact floor(log10 |x|) for every BigDecimal shape I tried (1E+2, 0.0100, 100.0) — good call avoiding the Double detour. Engineering snapping via floorDiv, the rounded >= limit renormalization, E--suppresses-plus, and pad-never-truncate all match SSF. lift(-1) returning None keeps the dispatch total.
• formatCustom's by-name dt: => Option[LocalDateTime] means the new case builtin => arm does not eagerly run serialToDateTime for numeric formats, and serialToDateTime range-guards rather than throwing — totality holds.
• NumFmt.Text on a number now routes through selectSection → None → formatGeneral, which is Excel's behavior and strictly better than the old n.toString (1E+3).
• resolveRelTarget: pure segment arithmetic, parent-escape → None, fragment/query stripped, package-absolute handled. Right call not to use java.nio.Path.
• The prune's blocked = deletedRoots correctly stops the survivor walk expanding through removed worksheets while still letting workbook.xml.rels be a survivor root; the deleted worksheets' own .rels siblings are excluded from survivorRoots via withRelsSiblings.
• insertAt/removeAt/reorder are genuinely the complete set of index-shifting mutations (grepped every copy(sheets = ...); the rest are appends and in-place updates).
• Comments.encodeCommentText preserved-rPr-wins matches the established pattern at all five other sites (SharedStrings x2, DirectSaxEmitter, OoxmlCell x2), and the synthesized authorRun correctly carries rawRPrXml = None.
• mergeSheetFormatPrElem is emitted in the correct CT_Worksheet slot on both backends (after sheetViews, before cols).
• lintOpts' position in the orElse chain and fileOpt.orNone do not shadow any other alternative; resolveLintFile's four-case match is total and keeps lint's exit-code-2 convention.
• The batch values[] format threading matches single-put semantics exactly, and the spec pins it against the single-value arm directly rather than restating the expectation — good test design.

Nice wave. 👍