You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
STATUS.md "New in 0.19.1" block; roadmap current-version line
NOTE (procedure): after #487 squash-merges, this branch gets REBUILT on new main (git diff wave..bump | git apply + force-with-lease) before merging — the stacked history diverges otherwise.
Review: chore(release): Bump version to 0.19.1 (#489)
Reviewed the full diff against 8f86e78 (58 files, +1976/-237) — the stacked wave-23b content from #487 plus the bump commit. The bump itself is clean and complete: build.mill, WorkbookMetadata.appVersion, plugin.json, README/QUICK-START/scripting docs/RECIPES/examples and the scripting prelude header are all consistently at 0.19.1, and every remaining 0.19.0 string is a correct historical reference (STATUS "New in 0.19.0", roadmap release line, CHANGELOG, one code comment describing the old bug). The formula-leading-equals retag to (0.19.1) is right.
The engineering in the wave is strong — total comparators, documented Excel semantics, a spec per fix. Notes below, most-severe first.
1. The GH-472 bounds guard will likely refuse insert-cols on most real-world books
Sheet.maxPopulatedIndex counts row/column properties as populated positions, and StructuralEditor.edit throws when maxPopulatedIndex(axis) >= at && max + delta > axisMax:
But XlsxReader.parseColumnProperties expands <col min=… max=…> into one entry per column (XlsxReader.scala:1188: for colIdx <- min to max do builder += …). Files with a sheet-wide <cols> span — <col min="1" max="16384" width="8.43" customWidth="1"/>, which Excel emits whenever a width or default style is applied across all columns, and which is exactly the sheet-wide-body-font mechanism GH-445 documents — therefore carry a columnProperties key at column 16383 (XFD).
Consequence: on any such workbook maxPopulatedIndex(rowAxis = false) == 16383, so everyinsertColumns with delta >= 1 throws XLError.OutOfBounds no matter where the data actually ends. xl -f book.xlsx insert-cols B 1 goes from working to refusing on a large fraction of field books. The row axis has the same shape but is far rarer (needs a <row> element at 1048576).
I could not run the build in this environment, so this is read from code rather than executed — but the chain is mechanical. Repro to confirm:
Suggested fix: keep the hard refusal for carriers that cannot clamp without losing data (cells, comments, drawing anchors, freeze anchor) and treat row/column properties as clamping structures like ranges (GH-428) — drop the entries that fall off the edge in shiftAxis instead of refusing the edit. Cosmetic width/hidden/style metadata on the last column is not worth failing a user insert. A test with a full-width <cols> span plus insert-cols pins it either way; StructuralBoundsSpec covers cells and comments only, which is why this is invisible today.
2. The guard exists only on the StructuralEditor path — core Sheet still overflows
Sheet.insertRows / Sheet.insertColumns are public and exported to users, and still call shiftAxis unguarded — maxPopulatedIndex sits right beside them but is never used there (Sheet.scala:160-175). Column.from0 / Row.from0 are unvalidated (Column.scala:15), so sheet.insertColumns(0, 20) on near-edge data still yields cells at column >= 16384 and an out-of-range <dimension> — the exact file desktop Excel refuses, which the CHANGELOG now claims is prevented. Either route the sheet-level API through the same check, or scope the CHANGELOG/STATUS wording to StructuralEditor.
3. Semver: this reads like a minor, not a patch
CfRule.Preserved gained a third field (ConditionalFormat.scala:126). CfRule is publicly exported via api.scala:80, so any downstream case CfRule.Preserved(xml, priority) => stops compiling — you had to update your own call sites (CfRuleParser.scala:249, ConditionalFormat.scala:200) for exactly that reason. It is also binary-incompatible (constructor/unapply arity), and there is no MiMa gate in build.mill to catch it. Same class of additive-but-public change: FormulaPrinter.printFileForm, ArgPrinter.separator, two new CriteriaMatcher.Criterion cases (downstream exhaustive matches break), SourceContext.definedNamesAsRead. The CHANGELOG line "No features; additive API only where a fix demanded a signal" undersells that. Either ship as 0.20.0, or call the source-breaking Preserved arity change out explicitly so consumers are not surprised by a patch bump. (Keeping a 2-arg unapply on Preserved would also work.)
4. Purity charter: StructuralEditor.insertRows/insertColumns now throw
CLAUDE.md non-negotiable #1 is "no thrown exceptions"; edit throws XLException(XLError.OutOfBounds) from inside wb.sheets.find(...).foreach { … } (StructuralEditor.scala:66-77). It works end to end — the CLI .attempt in Main.scala catches it and the new renderErrorMessage null-guard renders it — but scripting-prelude users calling StructuralEditor.insertRows directly now get an exception from an API whose signature promises totality. Consider an XLResult[Workbook] variant (insertRowsChecked, with the throwing one delegating), and document the new failure mode: neither plugin/skills/xl-scripting/SKILL.md, the CLI skill, nor docs/reference/scripting.md currently mentions that a structural insert can fail.
5. GH-473: defined names with an unqualified refersTo never shift
shiftDefinedNameText short-circuits on !mentionsSheet(formula, editedSheet) and passes shiftLocal = false (StructuralEditor.scala:520-531). Correct for Excel-authored names (always sheet-qualified), but wb.withDefinedName("Foo", "$A$1") / xl name-add Foo A1:B2 produce unqualified refersTo that will silently stay put after an insert — the same bug class GH-473 fixes, for names this library authored. Worth documenting, or shifting unqualified refs when the name is sheet-scoped to the edited sheet.
6. Minor / nits
Text criteria do not trim the operand.parseNumeric trims ("<= 50" -> Compare(Lte, 50)), but the new text branches pass s.drop(1)/s.drop(2) raw, so ">= abc" compares against " abc" and COUNTIF(r, "> m") differs from COUNTIF(r, ">m") (CriteriaMatcher.scala:106-140). Excel is lenient here; cheap to make consistent.
reconciledWith derives a source index from the part number.worksheetPartNumber("xl/worksheets/sheet3.xml") -> 2 (SourceContext.scala:161) assumes part number equals workbook.xml sheet order, which Excel breaks routinely (moving tabs keeps part names stable). It matches the writer existing s"xl/worksheets/sheet${idx + 1}.xml" convention (XlsxWriter.scala:766-779), so it is not a new inconsistency — and since deleteUntracked also sets modifiedMetadata, everything regenerates from the model anyway. Still worth a test for "untracked reduction on a book whose tab order != part order", plus a note that the index is only used for pruning.
Untracked rename is untested.copy(sheets = …) with a renamed sheet trips both the ghost branch and the untracked-addition branch; SheetSetReconcileSpec covers reduce/append/defined-names but not that.
Bounds-guard coverage gaps.maxPopulatedIndex counts drawing anchors, freeze-pane anchor/scroll target and row/col properties, none of which have a spec (comments do). See Add Claude Code GitHub Workflow #1.
Stale test counts.docs/STATUS.md:196 and docs/plan/roadmap.md:14 still say "5,047 tests" (and the CLAUDE.md per-module breakdown is untouched) although this wave adds roughly 150 tests across seven new/extended specs. A release bump is the natural place to refresh them.
printExpr threading. Passing sep positionally through ~25 recursive call sites works but is churn-heavy and easy to miss one; a using context parameter would have been a one-line diff per case. The remaining hardcoded ", " sites (printList, printWithTypes, prettyPrint) are all debug/human-facing, and both custom renderers (FunctionSpec.render, YEARFRAC) were converted, so file-form coverage does look complete.
Reads well
Format.saveSuffix collapsing five identical private clones, and renderErrorMessage extending the temp-to-target rewrite to the failure path with a getMessage-null guard.
matchesLookupExact / normalizeLookupValue replacing two divergent comparators with one total, blank-safe one — the "default 0 means equal" fall-through was a nasty bug class, and the specs pin range-position semantics explicitly.
Mechanically this is a clean bump. I verified the whole pin surface rather than reading the diff alone:
grep -rEoh "com\.tjclp::xl:[0-9A-Za-z.+-]+" plugin/skills/xl-scripting/ → 15 hits, all 0.19.1, so the release.yml:352-359 release gate will pass.
The examples/project.scala ↔ build.mill drift guard in scripts/test-examples.sh:17-29 agrees (both 0.19.1) — and CI confirms it: Skill Verify / snippets and CI / examples are both green.
Every surviving 0.19.0 string is a deliberate backward reference (CHANGELOG headings, StructuralEditor.scala:64 and StructuralCommandSpec.scala:282 describing the old buggy behavior, SKILL.md:109 feature-introduction tag).
No golden fixture or test asserts the old appVersion — grep -rl "0.19.0" across *.xml/*.json/*.svg/*.html is empty, and DocPropsSpec only uses explicit Some("1.0") / None.
Six things below. The first is a straightforward miss; the second is a call worth making explicitly before tagging.
1. Two anticipatory (0.20.0) tags missed — same class the PR set out to retag
docs/reference/scripting.md:253-254:
| `converged` | (0.20.0) `false` iff an iterative run exhausted `maxIter` ... |
| `iterationsUsed` | (0.20.0) iterative rounds actually run ... |
The PR body says the goal included retagging (0.20.0) → (0.19.1) text that #478 wrote anticipating 0.20. These are the only two remaining 0.20.0 strings in the repo (grep -rn "0\.20\.0" --include="*.md" --include="*.scala" --include="*.mill" --include="*.json" ., excluding .claude-pr/). Both describe the #454 fields shipping in this release, and the neighbouring rows use the same convention (excelErrors | (0.14.0) …), so they should read (0.19.1). As-is, the scripting reference points users at a version number that will never exist.
The CLI-side lint help needed no change — Main.scala:597 lists rule names without version tags. Nothing else drifted.
2. Semver: this is a patch release carrying an ### Added section and a case-class field addition
Recalc.scala:93-99 on the merge base:
finalcaseclassRecalcResult(
workbook: Workbook,
evaluated: Map[SheetName, Map[ARef, CellValue]],
errors: Vector[CellEvalError],
converged: Boolean=true, // new in this releaseiterationsUsed: Int=0// new in this release
) derivesCanEqual:
RecalcResult is public API in the published xl-evaluator artifact. Even with defaults, adding parameters changes the generated apply/unapply/copy signatures:
Binary incompatible — downstream code compiled against 0.19.0 gets a NoSuchMethodError on RecalcResult.apply/copy at 0.19.1. There is no MiMa in build.mill, so nothing catches this.
Source incompatible for positional matches — case RecalcResult(wb, ev, errs) => no longer compiles.
Plus seedDataTablesReport(), the IterativeCalc overloads, and FormulaPrinter.printFileForm are new public surface, which is why the CHANGELOG needs an ### Added heading at all. Under the semver the CHANGELOG header explicitly cites, additive public API is a minor bump: 0.20.0. #478 appears to have made that call already (hence the (0.20.0) tags), and this PR reverses it.
I'd either bump to 0.20.0, or — if shipping as a patch is deliberate (pre-1.0, minor reserved for feature waves) — say so in the CHANGELOG preamble so the ### Added section doesn't read as an oversight, and note the RecalcResult bincompat break for anyone upgrading a compiled dependency. It's your call; I just don't think it should be implicit.
3. CHANGELOG lost its [Unreleased] placeholder
## [Unreleased] was renamed to ## [0.19.1] - 2026-08-04 with nothing left behind. .claude/commands/release-prep.md:81-82 specifies "leaving an empty ## [Unreleased] section". The tag-message awk extraction works either way, but the next wave now has to recreate the heading, which is exactly how the section drifts.
4. Test count is stale in three places while STATUS.md claims today's date
docs/STATUS.md:196 reads "5,047 tests (verified via ./mill __.test, 2026-07-29)" — but this PR bumps the same file's header to Last Updated: 2026-08-04 (0.19.1). docs/plan/roadmap.md:13 and CLAUDE.md:399,403 repeat 5,047 with the per-module breakdown. Waves 22, 23, and 23b each added regression tests (#472's bounds repro is right there in StructuralCommandSpec.scala:282), so the number can't still be right. A release PR that already touches STATUS.md is the natural place to re-run and refresh it, along with the per-module split.
5. docs/reference/scripting.md carries 8 version pins that no gate protects
The release gate at release.yml:352-359 only greps plugin/skills/xl-scripting/; test-examples.sh only cross-checks examples/project.scala against build.mill. docs/reference/scripting.md has 8 //> using dep pins and appears in neither — and it's also absent from the file list in .claude/commands/release-prep.md (which stops at item 8, the skill files). This PR bumped it correctly by hand, but nothing would have caught the omission. Cheapest fix is widening the existing gate:
That would have caught #1's class of drift too, if the checklist item covered version tags as well as pins.
6. Two user-facing behavior changes the skill docs don't mention
Not blocking the bump, but this release changes observable behavior and plugin/skills/xl-cli/SKILL.md is what agents read before running these commands:
insert-rows shifts data cells and <dimension> past row 1,048,576 — Excel refuses the file #472: StructuralEditor.edit now returns a typed OutOfBounds error where it previously exited 0. SKILL.md:148-150 and 756-757 document insert-rows/delete-rows with no mention of the new refusal. One clause — "refuses the edit if the shift would push data past row 1,048,576 / column XFD" — saves an agent from retrying a command that will never succeed.
WorkbookMetadata.scala:38 writes appVersion = Some("0.19.1") into docProps/app.xml<AppVersion>. ECMA-376 constrains that element to an XX.YYYY shape (Excel emits 16.0300); a three-component 0.19.1 doesn't match. Excel tolerates it and round-trip is unaffected — XlsxReader.scala:1482 preserves whatever the source had, so the determinism law holds. But for a library whose charter is spec fidelity, it's worth a tracked issue (e.g. derive AppVersion as major.minor zero-padded and keep the full version in <Application>) rather than carrying it forward every release. Worth confirming against the spec text before acting.
Verdict: fix #1 (30-second edit, and the retag was in scope for this PR), and make an explicit decision on #2 before tagging — the tag is the point of no return for the version number. #3-#6 are housekeeping that can ride along or follow.
Nice touch on the CHANGELOG preamble, by the way — reframing waves 23 + 23b as one "field hardening" story with "No features; additive API only where a fix demanded a signal" is exactly the right summary for readers deciding whether to upgrade. That sentence is also, precisely, the argument for #2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #487 (wave 23b). Standard bump set → 0.19.1, plus:
[Unreleased]→[0.19.1] - 2026-08-04with the wave-23b Fixed entries appended (field-hardening framing, both waves)formula-leading-equalslint help/skill text retagged(0.20.0)→(0.19.1)(shipped in Wave 23: field-QC burn-down — CalcPr-honoring seeder, convergence signal, printer parens, JAXP limits, <f> canon, lint FP, -i message #478 anticipating 0.20)NOTE (procedure): after #487 squash-merges, this branch gets REBUILT on new main (
git diff wave..bump | git apply+ force-with-lease) before merging — the stacked history diverges otherwise.🤖 Generated with Claude Code