fix(cli): structural commands recalculate before writing - #437
Merged
Conversation
…ract) 8c021a9 correctly made structural edits invalidate shifted-formula caches (stale aggregates/ROW() would be silently wrong), which left insert/delete rows/cols output with no cached <v> workbook-wide — blank in cached-value consumers until a manual recalc (the original Delrin field complaint). The four structural commands now end with one global recalculate() before writing, exactly like batch (#352): fresh-correct caches, failing cells stay uncached, the file is written regardless, errors surface in the summary. Spec pins flip to assert recomputed values — including a deliberately-wrong stale cache (999 -> 4) and a shrinking-SUM delete (6 -> 4) that distinguish fresh-correct from both stale-preserved and absent. Refs #427 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review — structural commands recalculate before writing Reviewed the diff against the batch (#352) contract it mirrors, the What's good
Minor considerations (non-blocking)
Overall: correct, well-tested, and consistent with the established contract. LGTM. |
arcaputo3
added a commit
that referenced
this pull request
Aug 7, 2026
* fix(evaluator): refuse structural edits that tear a data-table interior An insert/delete band cutting through a `<f t="dataTable">` interior used to replace the record cell with its cached constant. The record vanished from the model and from the written XML, so WorkbookLint's `data-table-torn` rule could not fire — it builds its facts only from cells that still carry a record, so the loss was invisible by construction. The sibling GH-435 path (an edit that deletes an INPUT cell) already keeps the record and sets del1/del2; the guard was asymmetric. StructuralEditor now refuses the tear up front, matching Excel ("cannot change part of a data table") and the authoring API's V5 validation: the workbook is left untouched and the record survives, so any file xl writes still carries its record for lint to see. Deleting a band that swallows the WHOLE interior stays legal (that deletes the entire table), and a zero-width edit is a no-op instead of a record-destroying "tear" (editIntersects treated delta == 0 as an insert strictly inside, which also silently degraded ArrayFormula kinds). Totality: the edit path is now `XLResult` end-to-end via the new insertRowsChecked / deleteRowsChecked / insertColumnsChecked / deleteColumnsChecked family; the four pre-existing methods keep their signature as the throwing facade (unchanged GH-472 OutOfBounds contract, same messages). Rewrites the two tests that pinned the old degrade-to-constant behavior and adds a round-trip gate: after a refused tear the written sheet XML still contains `<f t="dataTable" ref="D5:F6">` and lints clean. Refs #495 * fix(render): render #### for numbers too wide for their column A numeric or date value wider than its cell was drawn under a per-cell clip-path, which sheared the LEADING digits off: 1,234,567.9 rendered as a clean-looking 4,567.9 — a plausible but wrong number. Right-aligned cells (numbers and dates get HAlign.Right from contentBasedAlignment) never overflow into neighbours, so the clip always won. RenderUtils.hashOverflowText now decides, from the value and its number format, whether the formatted text fits the effective cell width, and substitutes a run of '#' sized to the column when it does not — Excel's behaviour. SvgRenderer and HtmlRenderer both call it with their effective width (merge- and overflow-expanded), so the two renderers agree by construction. Not hashed: text (it bleeds into empty neighbours or clips, losing nothing a reader can misread), wrapped cells (Excel grows the row), values that fit, and values that fit the width they were expanded to by a merge or an overflow span. A column too narrow for even one marker still gets one '#' rather than digits. The "financial model example" HTML test now sets an explicit width on column B: "1000000.00" measures 85px and never fit the 72px default — it was previously rendering sheared, which is the very defect this fixes. Refs #459 * fix(evaluator): one-pass global fixpoint via SCC condensation walk Iterative recalculation split the pass three ways — preOrder / one flat Jacobi over the entire cyclic core / postOrder. An acyclic cell BETWEEN two SCCs is a transitive dependent of the first, so it landed in postOrder and was never evaluated before the fixpoint, yet the second SCC read it during iteration off the loaded caches. `converged` was therefore asserted mid-wavefront and certified only that the Jacobi loop had stopped moving. Replace the split with a single walk of the SCC condensation in dependency-first order: runs of acyclic components evaluate via the existing evalPass, each cyclic component fixpoints via the existing jacobiFixpoint against the threaded temp sheets. Every precedent is a freshly computed value before anything reads it, so no read can fall back to a loaded cache and `converged` now certifies the workbook's GLOBAL fixpoint. - DependencyGraph: additive `Scc` + `qualifiedSccOrder` — Tarjan via the existing foreachSccOf, filtered to dependencies.keySet (constants are visited as singletons and must not leak in), members sorted by (sheet, A1), then canonical Kahn over the condensation with a key-ordered TreeSet frontier so the order is a pure function of the graph's VALUE. qualifiedCyclicNodes untouched. - WorkbookEvaluator: PassState/CalcStep/buildPlan/runPlan/fixpointStep; evalPass takes the clock as a parameter; jacobiFixpoint takes an explicit seed map (Map.empty reproduces the all-zero seeding exactly) and returns FixpointOutcome with the final-round max |delta|. - Per-component budgets: maxIter/maxChange now apply per SCC, so one permanently-oscillating cycle no longer burns an unrelated cycle's budget nor blocks its verdict. Worst-case total work is unchanged. - One volatile generation: the clock is pinned once for the whole iterative walk, fixing a pre-existing split where pre/post-order used the raw clock while the fixpoint used a pinned one. Non-iterative path deliberately unchanged. - Dynamic bucket: when iterating, the INDIRECT/OFFSET deferral closure is taken over the FULL dependents map (the pruned one has the core deleted), so a cyclic component downstream of a dynamic cell cannot be scheduled ahead of it. Closure lifts to whole components. - Reporting: new SccReport (members, converged, rounds, maxDelta, render) on RecalcResult.cycles, plus unconverged/certified. converged and iterationsUsed become the aggregate (forall / max) of the per-component verdicts. Exhaustion stays a data condition, never a CellEvalError. - DataTableSeeder stays cold-seeded (Map.empty) on purpose: under what-if substitution its caches are stale by construction. Non-iterative recalculate() is structurally untouched — same single evalPass over the same order with the caller's clock. Refs #492 * test(evaluator): pin GH-491 idempotence on an interleaved multi-SCC book Regression suite for the condensation walk. The essential shape is an acyclic cell BETWEEN two SCCs: year cycles interleaved with tax-bank SCCs, separated by bridge cells. A single chain of SCCs does not reproduce the failure. Gates (all four fail on the pre-GH-492 engine, verified before the fix): - one pass reaches the global fixpoint from a CACHED input — solve at one driver value, perturb the driver on the solved book, one pass must land where a fresh solve of the perturbed book lands. Pre-fix the bridges stayed frozen for the whole pass and only pass 2/3 caught up. - five successive re-solves of the book's own output stay converged, value-stable, and keep the internal-consistency sentinel (end=beg-pay) in both year cycles. - twin exactness: re-recalculating the fresh result reproduces its workbook, evaluated map and error order bit-for-bit. - a five-SCC chain with every formula cache poisoned reaches the global fixpoint in ONE pass (pre-fix: one SCC of wavefront per pass). - one iterative recalculation is ONE volatile generation across SCC, bridge and SCC. Plus the new-capability gates: per-component budgets and verdicts (an oscillator does not consume a convergent component's rounds), summary fields as exact aggregates, determinism under insertion order and under a seeded Rng, bridge freshness, an INDIRECT cell upstream of a cyclic component, the unchanged non-iterative path, and GH-430 data-table cache pinning under an iterative run. Refs #491 Refs #492 * fix(evaluator): data-table seeder evaluates its precedent cone in both what-if lanes The seeder's what-if lane did not evaluate everything the solve lane evaluates, in two ways that both read as a clean run: - GH-493: the acyclic lane overlaid the axis value and evaluated the source expression against pinned caches. Any cached intermediate between the input cell and the corner answered with its BASE value, so every axis combination collapsed to the same number — a FLAT grid, zero warnings. The CLI `recalc --tables` lane hit this always: its recalc phase caches every precedent before the seed phase runs. - GH-494: an UNCACHED formula inside a range argument is silently DROPPED by the cashflow readers (they shrink the array rather than erroring), so XIRR saw a length mismatch, ISERROR saw TRUE, and a guarded corner banked its "NM " error arm into every interior. The iterated lane made this worse by stripping caches. Both lanes now re-derive the source formula's precedent cone on TEMP sheets, in topological order, writing each value back before its dependents run — the seeder's copy of recalculateImpl's evalPass, including its tolerance (a cone cell that fails to evaluate is left as it was and seeding continues). The cone splits three ways: uncached input-independent precedents resolve ONCE per table; axis-dependent precedents upstream of a cycle resolve after the axis overlay so the Jacobi fixpoint reads substituted values; the rest resolve after the fixpoint folds its members in. Cost is capped by MaxConeSeedBudget (interior cells x axis-dependent precedents), past which the table skips untouched with a budget-naming Skipped rather than seeding stale. Adds SeedTableWarning.ErrorGuardFired: when the source formula's own error guard — IFERROR/IFNA, or IF(ISERROR(x),…) with its ISERR/ISNA siblings — resolves to its ERROR ARM during seeding, the run reports it per table with the count of combinations and the guarded expression. A guarded corner turns a failure into a plausible-looking grid, so banking a fallback must never read as clean. Rendered by `xl recalc --tables` alongside the other warnings. Refs #493 Refs #494 * feat(cli): cache-safe writes — dirty-cone recalc, --no-recalc, calcPr bridge, --strict No CLI lane may silently corrupt a cached book. - GH-468: every write verb's trailing recalculation is now scoped to the edit's dirty dependency cone (changed cells + transitive dependents workbook-wide + always-dirty INDIRECT/OFFSET cells). batch and the four structural verbs stopped calling a whole-book recalculate(); caches no op can have invalidated are never rewritten, and sheets with no cone change are not put back at all, so the surgical writer still preserves their XML verbatim. New global --no-recalc / --preserve-caches flag skips the trailing recalculation outright on every write verb (put/putf/fill/copy/batch/insert-*/delete-*), including the dependent refresh inside a batch copy op. - GH-481: batch (and the structural verbs) recalculate through the same calcPr-honoring path as recalc, so an iterate-declared circular book fixpoints instead of reporting circular errors — and the GH-454 convergence suffix goes live for those verbs. - GH-496: the existing --strict spelling is extended to the write verbs as a global flag. It promotes recalc errors, iterative non-convergence and data-table seed warnings to exit 1 while printing the full summary verbatim (StrictFailure, not an Error:-prefixed crash); the file is still written and the default stays advisory. Refused with --stream (streaming writes never recalculate, so the gate could never fire); recalc rejects --no-recalc as a contradiction. Refs #468 Refs #481 Refs #496 * fix(evaluator): warm-start iterative cycles from their loaded caches Cycle members seeded unconditionally to 0. On a mutually IF(ISERROR(...))-guarded pair sitting at a VALID numeric fixpoint (A3 = B3*0.5+10 = 20, B3 = A3*0.5+10 = 20, both cached 20), round 1 sees 0/0 = #DIV/0!, both guards take the text branch, and "NA " is itself a fixpoint — so the run reported converged with errors and excelErrors both empty while two valid caches were destroyed. Excel seeds iterative calculation from the current cell values. So does xl now: `warmSeed` reads each member's cached value off the THREADED temp sheets (not the original workbook), zero remains the fallback for uncached members, and `IterativeCalc.seedFromCaches = true` (appended, defaulted, inherited by fromCalcPr) is the Excel-parity default. Reading the threaded sheets buys two properties for free: a member whose cache the dynamic INDIRECT/OFFSET bucket stripped correctly seeds 0 (its cache is declared stale for the pass), and no member can be read after being written, since the condensation walk visits each component exactly once and components are disjoint. Any cached VALUE seeds, not just numbers — convergence still requires re-evaluating the formula and reproducing it, which is a genuine fixpoint (GH-344 already blesses this for error values). seedFromCaches = false is the supported cold start for a book whose caches are known to be poisoned; DataTableSeeder keeps passing Map.empty because its caches are stale by construction under what-if substitution. Consequences documented in LIMITATIONS/scripting: for a circular book recalculate(wb) may differ from recalculate(stripCaches(wb)) on a nonlinear multi-fixpoint cycle (Excel's exposure), and re-solving a converged book is bit-exact only under cold seeding — warm seeding still writes round 1's values, which move by strictly less than maxChange, toward the true fixpoint. Fresh (uncached) books are unaffected. Refs #469 * fix(evaluator): fire ErrorGuardFired only for guards on the evaluated path; report unresolvable cone cells Review rework of the GH-493/GH-494 cone fix. Two defects, both in the diagnostics rather than the substitution mechanism (which the review verified end-to-end and which is untouched here). 1. ErrorGuardFired false-positived on guards in UNEVALUATED branches. `errorGuards` harvested every IFERROR/IFNA/IF(ISERROR..) node anywhere in the source AST and probed each protected sub-expression standalone, so a chained ladder `IFERROR(A1*2,IFERROR(1/(A1-A1),0))` and a guard parked in an untaken IF branch both reported a fallback that was never banked — the CLI rendering that as "seeded the ERROR ARM ... verify the grid is not a fallback" over a perfectly live grid. Guards now travel as (protected, fallback) pairs and fire only when the protected expression errors AND the fallback reproduces the value the seeder is about to bank. A clean run that reads as dirty destroys the same signal #494 was filed to protect. 2. A cone cell that could not be re-derived restored the #493 signature silently: `resolveCone` dropped the Left, the cell stayed on its stale cache and the grid went FLAT with zero warnings. Cone resolution now returns the unresolved refs, both lanes accumulate them de-duplicated across axis combinations, and a table with any reports one Skipped. Refs #493 Refs #494 * fix: wave-24 field asks — view hidden lines, streaming numFmt parity, loud evaluator gaps, lookup date plane, lint docs, skill data-table write view (GH-474): hidden rows/columns inside an explicitly requested range now render by default and carry a marker in every data format — markdown trailer, CSV stderr note, JSON `hiddenRows`/`hiddenCols`. `--skip-hidden` restores the old elision and the marker then names what was dropped. html/svg/raster are pictures of the sheet and keep mirroring Excel's display; streaming already rendered every addressed cell. numFmt (GH-475): `StreamingWriteCommands.parseNumFmtSync` was a hand-copied six-name subset ending in `case _ => None`, so a `--stream batch` style op with a custom code reported success and left the cell General; it now delegates to `StyleBuilder.parseNumFmt`. The code test moves into `StyleBuilder.looksLikeFormatCode` (quoted literals and `;` count, so `"Yes ";;"No "` survives the put/putf `format` hint), and a string that is neither a name nor code-shaped warns instead of being dropped/shipped silently. evaluator (GH-476): add SEARCH (case-insensitive, wildcard-aware, #VALUE! on miss), N() and HYPERLINK(); resolve untyped refs reaching operand positions so `=IF(1=1,+S2!G1,0)` no longer dies with "Unresolved SheetPolyRef"; VALUE() of unparseable text is a cached #VALUE! rather than a host error. Registry 109→112. lookups (GH-488): VLOOKUP/HLOOKUP route their key normalization through the shared `normalizeLookupValue`, and `extractNumericForMatch` learns the DateTime→serial case, so date keys resolve over date columns. docs (GH-486): cli.md's lint "What it flags" enumerates all nine LintCategory slugs, with a test that fails when a category is added without documenting it. skill (GH-490): the worked sensitivity-table example writes with `Excel.writeRecalculated` — `seedDataTables` never caches the corner formula. Refs #474 Refs #475 Refs #476 Refs #486 Refs #488 Refs #490 * fix(render): clamp the right text anchor and hash General-aligned overflow Two residual holes in the #### overflow marker. Shear band (SVG only). textAlignment right-anchors at `cellX + cellWidth - CellPaddingX`, but hashOverflowText tests fit against the full available width. Text measuring in (W - 6, W] escaped the marker and was then clipped on its left by the per-cell clip-path — the truncated numeral #459 exists to prevent. Reproduced through the CLI PNG path: 1234567.9 at column width 9.25 rendered ".234567.9"; -$1,234.56 at the same width lost its minus sign and read as a positive amount; the default-width date 11/10/25 (68px in 72px) lost 2px off its leading digit. Subtracting padding from the fit threshold instead would over-hash — a 72px-column date that Excel shows fine gets hashed — so the anchor is clamped to the clip's left edge rather than the threshold tightened to the anchor. Safe because anything wider than the effective width was already replaced by hashes, so the clamp can never push text past the right edge. The HTML path has no CSS padding on data cells (only padding-left for indent) and so has no band; it is untouched. General-aligned numbers. calculateOverflowColspan mapped HAlign.General to Left and bled right, while textAlignment resolves General on a Number to Right via contentBasedAlignment. A bare `A1 -> 123456789012345.0` with a narrow column A and empty B/C/D rendered the full number right-anchored at the end of column C under a 181px clip, so the marker no-opped on the simplest possible sheet. calculateOverflowColspan now resolves General through contentBasedAlignment, matching the renderers: numeric and date content takes the clip-then-hash path and stays in its own column. Follow-ups filed: #500 (Bool/Error clip instead of hashing), #501 (a number under NumFmt @ is hashed though Excel treats it as text), #502 (calculateOverflowColspan measures the raw value, rendering the formatted text). Refs #459 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(evaluator): warm-start iterative cycles from NUMERIC caches only The GH-469 warm start seeded from any cached value, including Excel error and text values. Arithmetic propagates both shapes unchanged (`#DIV/0! * 0.5 + 10` is `#DIV/0!`, `"junk" * 0.5` is `#VALUE!`), so a non-numeric seed is its own fixpoint: a perfectly HEALTHY cycle carrying a stale error cache wedged at the poison in round 1 and reported `converged = true` with `errors` empty and `certified = true` — the exact silent-failure shape GH-469 was filed about, inverted. Pre-0.20.0 the same book healed. `warmSeed` now matches `Some(CellValue.Formula(_, Some(v @ Number(_)), _))` only; every other cached shape falls through to the 0 fallback and heals as it did before. GH-469's reported repro is unaffected (its caches are Number(20)) and its five existing tests stay green. Scaladoc on `IterativeCalc.seedFromCaches` and `RecalcResult`'s seed- dependence note, the LIMITATIONS.md warm-start bullet, and scripting.md are corrected — they previously stated the opposite ("Any cached VALUE seeds"). Refs #469 * test(evaluator): make the GH-491 idempotence gate discriminate The designated GH-491 gate re-solved a book that was ALREADY at its fixpoint. The `interleaved` fixture is a contraction, so the pre-GH-492 split pass operator is stationary on it too and the test passed verbatim against wave-24 sources — it could not have caught a regression. The gate now perturbs the driver (`A1 = 1000 -> 1200`) on the SOLVED, fully cached book before the five successive re-solves, and requires every pass — the first included — to be converged, schedule-consistent, and equal to `interleaved(1200)` solved fresh. That is what exposes the split pass: the bridges C1/E1 are acyclic cells BETWEEN two SCCs, so pre-fix they were evaluated only in post-order and the downstream SCCs iterated against the 1000-generation caches. Verified RED: lifted verbatim into a detached wave-24 worktree it fails at pass 1 with `year-2 end=beg-pay: got 1066.78..., expected ~1283.44...`. Refs #491, #492 * fix(cli): --no-recalc must preserve structural caches; -i strict must not claim a save Rework of the cache-safety cluster after adversarial review. F1 (data loss): --no-recalc destroyed caches on insert-rows/insert-cols/ delete-rows/delete-cols while printing "every existing cached value preserved". StructuralEditor strips cachedValue from every formula that transitively reads the edited sheet before the CLI sees the workbook; the unconditional recalculate() used to repopulate them, and under the new escape hatch nothing did — the written file carried formula cells with no <v> at all, even for a shift that moved nothing. writeStructural now carries the pre-edit caches forward (restorePriorCaches + the inverse axis shift), restoring a cache only when the formula text is byte-identical, so a rewritten (shifted) formula correctly stays uncached. F2 (test overstated its coverage): the insert-rows preservation test's second sheet was self-contained, the only shape where preservation holds. Renamed to the narrow truth and added a test that pins the CURRENT wrong behavior for a cross-sheet reader, marked for #503. F3 (false CI log line): under -i a strict failure discards the temp file but still printed "Saved: <input>". The line now reads "NOT saved (--strict failure): <input> left untouched"; with -o the file really is written, so "Saved:" stands there. Docs: the three global flags (--no-recalc/--preserve-caches/--strict) are now in docs/reference/cli.md and the xl-cli skill, including the note that global flags must precede the verb. Refs #468 Refs #496 Refs #503 Refs #504 * fix(cli): warn on fat-fingered `style --format`, stop `--stream --skip-hidden` from no-oping silently Two rework findings from the wave-24 field-asks review. #475 second ask reached only the batch-JSON path: `xl style A1 --format curency` still wrote numFmtId=164 formatCode="curency" with exit 0 and no diagnostic, because StyleBuilder.numFmtWarning had exactly one call site (BatchParser). New StyleBuilder.warnNumFmt drains it to stderr and is called from both direct `style` handlers (WriteCommands.style and StreamingWriteCommands.style). It is deliberately NOT called from buildCellStyle: the batch path already warns through BatchParser.ParseResult.warnings, and calling it there would double-warn (verified: one warning per batch run, in-memory and streaming). stdout is unchanged, the code is still applied verbatim — Excel remains the authority on what a format code means. #474: `--stream view --skip-hidden` was accepted and did nothing — Main destructured the field as `_`. The streaming reader yields cell values only and never parses row/column properties, so it cannot honour the flag or compute the hidden-line marker; it now says so on stderr (RendererCommon.streamingSkipHiddenNotice) instead of silently ignoring the flag. StreamingReadCommands.view takes `skipHidden` so the behaviour is testable at the command boundary rather than buried in Main's pattern match. cli.md's hidden-lines paragraph documents the limitation and the note. Refs #474 Refs #475 * fix(cli): --no-recalc must never write a provably-wrong cached value Round-1 healed the missing-<v> loss on the structural verbs but restored caches on a single condition — post-edit formula text byte-identical to the pre-edit cell's. That is not sufficient for a cell that RELOCATED or whose referent did: `C30 = =ROW()` cached 30 came back as `C31 v=30` (truth 31), `D30 = =COLUMN()` cached 4 landed at F30 still caching 4 (truth 6), and `F1 = =INDIRECT("A30")*2` kept 10 after the content of A30 moved to A31 (truth 0). A missing <v> is visible; a wrong one is silent, so the rework had traded a loss for a corruption in the one lane whose whole purpose is cache trust. restorePriorCaches now carries a cache forward only when all three hold: the text is unchanged, the cell did not move (preEditRef(r) == r; every sheet but the edited one is unshifted by construction), and the formula bears no dynamic reference (DependencyGraph.dynamicCells, before or after). Everything else is left uncached — Excel's own dirty state. The `--no-recalc` note also stopped being a constant on that path. It claimed "every existing cached value preserved" while an ordinary model (A1:A10 data, B1:B10 =An*2, D1..D3 aggregates) came out of insert-rows 5 1 with 9 of 13 formula cells uncached. The structural arm now prints counted, auditable output — "4 cached value(s) preserved, 9 formula(s) invalidated by the edit left uncached (recalculate externally)" — while the unconditional wording stays on put/putf/fill/copy/batch, where it is true. docs/reference/ cli.md, plugin/skills/xl-cli/SKILL.md and the --no-recalc flag help are corrected to match. Tests: relocated =ROW()/=COLUMN() and a moved-target INDIRECT never come back cached; a contiguous-block insert (not just the row-20 fixture that rewrites nothing) pins the real counts and the new message; the non-structural claim is pinned separately. Refs #468 Refs #481 Refs #496 * fix(render): gate the right-anchor clamp on fit, hash by the real text box Round-2 rework of the #### overflow marker. Two defects, both in the geometry around the marker rather than the marker itself. Right-anchor clamp regressed overflowing text. 563c73e clamped the end anchor unconditionally, on the argument that anything wider than the effective width had already been hashed. That is only true of numbers and dates: hashOverflowText deliberately leaves Text, Bool and Error alone, so those reached the clamp at full width and were left-anchored by it — the renderer showed the HEAD and pushed the tail out of the cell. A right-aligned "Q1 2025 Actuals" in a 6.0-wide column rendered "Q1 202" instead of Excel's "Actuals", and a right-aligned text-number "1,234,567.90" rendered "1,234,5" — itself the plausible-wrong-number shape this issue exists to prevent. It also split the renderer against itself: the RichText branch left-shifts by the run width and kept cutting the head. The clamp is now gated on `textWidth <= effectiveWidth`, which is exactly the shear band it was written for; wider text keeps Excel's tail. Numerals were still cut on the right. Only the end anchor was ever clamped, and the fit test measured against the whole column while left-aligned text starts CellPaddingX (plus indent) inside it. Left-aligned 1234567.9 at width 9.0 spanned [6,82] under a [0,77] clip — five pixels of the trailing digit gone, no marker; at indent 2 in a 12.0 column, 23px (~2.5 digits); at indent 3, 44px, rendering the number as roughly "123". hashOverflowText now takes the cell's style and tests the fit against the box the text really occupies (textBoxWidth: full width when right-anchored, less the pad and indent when left-anchored, less the indent when centred), and the marker run is bounded by that same box so it cannot overflow either. Deriving the box in RenderUtils rather than per-renderer is what keeps HTML and SVG hashing the same cells: HTML's geometry differs (no horizontal padding on data cells, padding-left for indent) but the decision must not. Right-anchored values are unaffected by the tighter fit test — their box is still the full width — so the sixteen existing GH-459 tests are unchanged. Batik lays text out ~2px wider than AWT measures, so at sub-2px slack the clamp can still shave the flag off a leading '1' (rsvg and resvg do not). Recorded at the clamp and filed as #505; a safety margin would only move the shave to the trailing digit. Refs #459 Refs #505 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(evaluator): decide ErrorGuardFired by walking the evaluated path; split the cone warning off Skipped Round-2 review rework of GH-493/GH-494. The #493 cone mechanism (whatIfCone / resolveCone / ConeResolution / CellOutcome / the topological write-back) is untouched — both defects are in the diagnostics layer round 1 introduced. 1. Round 1 traded #494's false positives for FALSE NEGATIVES. `banksFallback` asked whether the fallback's STANDALONE value equalled the whole banked cell value, a proxy wrong in both directions: it went silent for every guard that is not the ROOT of the source formula (`IFERROR(1/A1,0)+5` banks 5, which no arm equals alone; `SUM(IFERROR(1/A1,0),5)` likewise) and for the numeric fallback house shape `IF(ISERROR(x), <cell ref>, x)` (the ref parses as a PolyRef in IF's Any-typed arg slots and does not reproduce the banked value on its own) — six shapes that all warned before the rework. It also still false-positived when an untaken fallback coincidentally equalled the taken path's value (`IFERROR(A1*2,IFERROR(1/(A1-A1),0))` at axis 0). The verdict is now STRUCTURAL: walk the parsed source AST top-down and return the first guard on the path Excel actually took. IFERROR/IFNA fire when their protected half errors and their fallback is never entered; an IF over ISERROR/ISERR/ISNA fires on its argument, otherwise the condition evaluates (Excel truthiness, same evaluator and pinned clock) and only the SELECTED branch is walked; every other node walks all its children. No value is ever compared to the banked cell value. The AST is parsed once per table and kept only when it carries a guard at all. 2. The cone diagnostic rendered a false statement. Reusing `Skipped` made the CLI print "N interior cell(s) left unseeded" over a table whose interiors were ALL seeded (they were merely seeded off a stale precedent). Cone staleness now has its own case, `SeedTableWarning.ConeUnresolved`, carrying the distinct refs rendered 'Sheet'!A1 (sorted, capped at 8) so the warning is actionable, with its own CLI line: seeded, but N precedent cells could not be re-derived. `Skipped`'s wording and meaning (genuinely unseeded cells) are unchanged. Refs #493 Refs #494 * fix(cli): --no-recalc must not re-assert ANY pre-edit cache after a structural edit Round 3 of #468. Rounds 1 and 2 each added a local guard to `restorePriorCaches` (formula text unchanged / cell did not move / no INDIRECT-OFFSET) and each time review found another dependency path where the formula TEXT is unchanged but what it resolves to is not. Two more were reproduced: 1. A static dependent of a dynamic cell. `F1 = INDIRECT("A30")*2` is correctly refused a cache, then `G1 = F1+1` is re-stamped with the value derived from the cache xl just discarded as untrustworthy — an internally inconsistent file carrying a wrong number. 2. A formula reached through a defined name the edit rewrote. `Other!B2 = SUM(MyRange)` over `MyRange -> Data!$A$1:$A$10` keeps its text, its position and its cached 55 while the name shrinks to `Data!$A$1:$A$9` and the answer becomes 52. In the `#REF!` variant it keeps 15 for a formula that has no answer at all. Rather than add a fourth local guard, this drops restoration entirely. The dirty cone `scopedRecalc` uses does not rescue it in either form, and both were measured: - Seeded with `changedRefs`, the cone is sound but VACUOUS. `changedRefs` compares whole cell values and dropping a `cachedValue` IS a difference, so every candidate for restoration is by construction one of the cone's own seeds; the predicate can never fire. Empirically every preservation fixture went to zero. - Seeded on semantic change only (ignoring the cache strip) the cone preserves again but is UNSOUND: with `MyName -> Data!$A$30` degraded to `#REF!`, `FormulaParser` cannot parse the definition, so `TExpr.NameRef` contributes no graph edge, `Other!B2` has no precedents and no seed can reach it. The cone hands back 15. That failure mode is general — any reference the static graph cannot resolve (a `#REF!`-ed name, an unparseable structured reference, an external link) breaks the closure the same way, and a dynamic reference is invisible to it by definition. The only complete test is a recalculation, which is exactly what the flag refuses. So `--no-recalc` now writes the structural edit as `StructuralEditor` produced it and counts what that left uncached: a missing `<v>` any recalculation repairs, a wrong one nothing detects. The flag is not thereby vacuous — `StructuralEditor` only invalidates formulas that transitively read the edited sheet, so an independent sheet's externally-authored caches still ride through byte-identical and are counted as preserved. Also corrects the `WritePolicy.noRecalc` scaladoc, which still claimed "Every cached formula value already in the file survives verbatim" — the sentence the user-facing docs were already corrected for — and realigns cli.md, SKILL.md and the `--no-recalc` help text with the actual behavior. Verified end to end on a real `xl-cli.assembly` via `java -jar`, reading values back out of the written zip: all three escapes come out with no `<v>`, the four original acceptance probes still hold, the #481 calcPr bridge and the #496 strict gate are unchanged. Refs #468 * fix(cli): --no-recalc withdraws caches behind defined names the graph cannot parse Round 4 of #468. The reviewer found a fourth escape of the round-3 class and it falsifies the universal this cluster's own docs wrote one revision earlier. THE ESCAPE. `StructuralEditor.staleCaches` closes over the STATIC dependency graph. A defined name whose refersTo `FormulaParser` REJECTS contributes no `TExpr.NameRef` edge, so a formula reading the edited sheet only through that name has no precedents at all: the seed set never reaches it and its cache rides straight through while the answer changes. Two repros, both ordinary Excel constructs xl itself cannot evaluate — i.e. exactly the externally authored population --no-recalc exists to serve: Multi -> Data!$A$1:$A$3,Data!$A$8:$A$10 (multi-area / Ctrl-click union) Other!B2 =SUM(Multi) <v>33</v>, delete-rows 2 1 -> truth 31, file said 33 Inter -> Data!$A$1:$A$8 Data!$A$5:$A$10 (intersection) Other!B2 =SUM(Inter) <v>26</v>, delete-rows 2 1 -> truth 30, file said 26 The intersection name's TEXT is unchanged by the edit (shiftDefinedNameText does not rewrite the space-separated form either), so a before/after name-text diff is not a sufficient detector and is deliberately not used. THE FIX (--no-recalc structural arm only, before the count). `dropCachesBehindBlindNames` strips `cachedValue` from every formula whose text mentions — whole-token, case-insensitive, outside string literals — a defined name that `FormulaParser` rejects in the PRE- or POST-edit name set. This is not another local "the text didn't change so the value didn't" guard of the kind round 3 correctly rejected; it is that argument's complement. Where the graph is known blind, its silence about a reader is not evidence of independence, and the strip only ever withdraws a claim, never adds one. NOT FIXED HERE, FILED INSTEAD. The escape is PRE-EXISTING: the DEFAULT recalc path writes <v>33</v> too, and so does 07064ed (before --no-recalc existed). #507 staleCaches under-approximates through any unparseable defined name (default path included); shiftDefinedNameText misses intersections #508 cone scoping swallows the out-of-cone unparseable-name error, so --strict now exits 0 on that book #509 <calcPr fullCalcOnLoad="1"/> on --no-recalc structural writes — the design that makes the flag useful again without lying, given that preservation measures 0/14 on any model reading the edited sheet DOCS. The structural paragraph in docs/reference/cli.md and the matching sentence in plugin/skills/xl-cli/SKILL.md no longer claim xl "never writes the wrong one". They now say what is true: xl never RE-ASSERTS a cache the edit invalidated, and a cache rides through only when the pre-edit dependency graph shows no path to the edited sheet — a reference the parser cannot resolve can hide such a path. Round-3's deletion of `restorePriorCaches` stands. Nothing is restored. Refs #468 Refs #481 Refs #496 Refs #507 Refs #508 Refs #509 * fix(cli): close the blind-name set transitively over the defined-name table `dropCachesBehindBlindNames` flagged only names whose OWN refersTo the parser rejects. A name that merely ALIASES a blind one parses fine, so it was never flagged, and the reading formula never mentions the blind name textually — so a cache behind `Outer -> Blind`, `Alias -> SUM(Blind)` or `L2 -> L1 -> Blind` rode straight through while the answer changed. Close the set with a tailrec fixpoint over the same name table, admitting any name whose refersTo mentions an already-blind one. The common book pays nothing: with no unparseable name the seed set is empty and the closure terminates after one trivial iteration. This was NOT GH-507's territory (that is the evaluator's static graph) — it was this function's own blind set being one iteration short, fixable here with data already in hand. The honest residue stays GH-507: structured references and external links in cell text are still not detected, and the DEFAULT recalc path still has the hole. Verified on a freshly built assembly at all three alias depths (each `0 cached value(s) preserved, 1 formula(s) invalidated`, no <v>), with the union/intersection repros still closed and the over-strip fixture still preserving its parseable independent name. Refs #468 * docs: wave 24 release notes — 0.19.2 "Fixpoint" CHANGELOG, roadmap release entry, STATUS and the CLAUDE.md counts (5,424 tests, 112 functions). Integrator-owned files, deliberately untouched by the wave agents to avoid six-way conflicts. * fix(evaluator): narrow structural cache invalidation to what the edit touched GH-503. `StructuralEditor.staleCaches` seeded on EVERY cell of the edited sheet, and every formula on that sheet dropped its cache unconditionally. An insert at row 20 therefore invalidated a reader of A1 — which is why `--no-recalc` preserved almost nothing (0/14 on a 14-formula model) and why `restorePriorCaches` was invented to undo it. Undoing an over-invalidation by re-asserting caches from local syntactic evidence is what four review rounds proved unsound. Not over-invalidating is sound by construction: it withdraws fewer claims and asserts nothing new. Two narrowings: - seeds are the cells the edit MOVED OR REMOVED (index >= at on the edited axis), not the whole sheet, then the usual forward closure with dynamic references still unconditionally dirty; - a formula keeps its cache when its printed text is byte-identical, its record kind is unchanged, it did not relocate, and it is outside that cone. `ref` is a POST-shift address while the cone is keyed on PRE-edit ones, so relocation is tested against the post-edit cut — that is the `=ROW()` case, which keeps its text and changes its answer. Gated behind `preserveUntouchedCaches` (default false) and threaded from the CLI's --no-recalc arm only, so the DEFAULT recalculating path stays byte-identical: it still re-bakes a poisoned cache on an unrelated edit per the GH-352/#437 contract, which shrinking its cone would have broken. Measured on the m2 model: an edit below the data now preserves 14/14 (was 0/14); an edit inside a contiguous block preserves 5/14, splitting exactly where the dependency graph does. =ROW() relocating still drops. Refs #503, #468 * fix(evaluator): gate the seed narrowing too, not just the cache carry Review finding on 30d8132, reproduced and confirmed: the previous commit gated the cache-CARRYING decision behind `preserveUntouchedCaches` but left the seed narrowing unconditional. `staleCaches` also feeds `keepNonParticipant`, which runs on EVERY path, so the DEFAULT recalculating path was NOT byte-identical as that commit claimed. A cross-sheet reader that reaches the edited sheet only through a defined name never spells that sheet in its text, so it rides the non-participant branch. With narrowed seeds it kept its cache, dropped out of `changedRefs`, escaped the dirty cone, and `scopedRecalc` never refreshed it — a verb that promises to recalculate silently resurfaced a poisoned <v>. Measured on Other!B1 = SUM(TotalRange) over Data!$A$1:$A$10 with a spliced 777: baseline writes 55, 30d8132 writes 777, this commit writes 55 again. Named ranges read cross-sheet are the norm in banker models. Off the flag the seeds stay the whole edited sheet, byte-for-byte as before; the --no-recalc arm's output is byte-identical to 30d8132, so the gate costs zero preservation. Also from the review: - pin the default arm (nothing covered it: the existing cross-sheet pin uses `Data!A1*2`, whose text DOES name the sheet, so it takes the parse branch and always dropped); - four StructuralEditorSpec cases so the new public parameter's contract is pinned where it is defined, not only through the CLI — xl-evaluator.test passed with the whole file reverted; - document that volatiles above the cut now keep their caches under --no-recalc. Deliberate: the flag means do not recalculate, and Excel refreshes volatiles on open regardless. Refs #503, #468 * docs: record GH-503 in the 0.19.2 notes; refresh counts (5,430) * fix(evaluator): guard-walk precision, N() error propagation, cone-scoped convergence Review pass over the wave-24 seeder and CLI work (Codex-authored, verified and extended here). Four independent correctness fixes, all of the same family the wave is about — a plausible wrong answer with no signal. - N() returned 0 for an error cell where Excel propagates it, so =N(A1) over #DIV/0! silently became a zero in the arithmetic. It now lifts a carried error onto the Left channel (#476). - The #494 guard walk treated CHOOSE / IFS / SWITCH as ordinary nodes and walked every child, so a guard sitting in an UNSELECTED branch reported as fired. They are selector functions: only the taken branch is on the evaluated path. IFS conditions are materialized the way IFS itself does (range -> array, top-left collapse) and SWITCH compares its target with the same scalar boundary the function uses. - firedPredicate fired when any ISERROR/ISERR/ISNA argument resolved to an error, ignoring which predicate it was. It now evaluates the predicate, preserving the accepted-error sets: ISERR excludes #N/A, ISNA accepts only it. - scopeToCone filtered `evaluated` and `errors` to the dirty cone but left converged/iterationsUsed/cycles at whole-book values, so a cone-scoped write verb inherited a convergence failure from a cycle it never touched — and --strict exited 1 on it. Added here on review: - IFNA coverage was absent repo-wide, and probing it showed why: IFNA has no FunctionSpec at all, so `errorGuardFires`' IFNA arm is unreachable dead code and its scaladoc described behaviour that cannot occur. Scaladoc corrected to mark the arm latent; current behaviour pinned (an IFNA corner seeds NOTHING, silently) and filed as #511. - Formatting (checkFormatAll was failing on DataTableSeederSpec). Anti-rubber-stamp: with the three source files reverted and the tests kept, 8 of the 9 new tests fail; the two that pass are deliberate positive controls against over-correcting (a SELECTED guard must still report, and an unconverged SCC INSIDE the cone must still fail --strict). Refs #494, #476, #496, #511 * feat(evaluator): IFNA / NA / ISNA, and error guards that see a cached error Making the IFNA branch reachable, per review — it was dead code guarding a function the roster did not have. IFNA is IFERROR's discriminating sibling: it swallows only #N/A, so a #DIV/0! propagates instead of being masked. That is precisely why banker books guard lookups with it — a missing key is expected, a division by zero is a bug, and one guard must not hide both. NA() authors the value it catches. ISNA completes the family; `DataTableSeeder.ErrorPredicates` already named it, so the guard walk carried a second unreachable branch. Specs auto-register through the FunctionRegistry macro. 115 functions. Adding them surfaced a larger PRE-EXISTING bug (#512): IFERROR, ISERROR and ISERR matched only a bare CellValue.Error. On any recalculated book a formula cell carries its value in `cachedValue`, so: A1 = Formula("=1/0", Some(Error(Div0))) =ISERROR(A1) -> false WRONG =IFERROR(A1,42) -> the cell WRONG (no fallback taken) Both silently wrong, on the guard idiom these books lean on hardest. `ArrayArithmetic.carriedError` already resolved this and recurses through cached formulas — its scaladoc says it is package-visible so the error guards share the exact matching semantics, and the type-check family simply never used it. All five now do. Zero regressions across the suite. The DataTableSeederSpec pin ("an IFNA corner seeds NOTHING today") flips to real assertions: the corner seeds, and the guard fires on #N/A only. Also records the incremental-compile trap this hit: editing any FunctionSpecs trait invalidates the registry macro's view and Zinc reports `Cyclic reference involving trait TExprReferenceOps` in files you never touched. A clean module compile clears it; it is not your code. Refs #511, #512, #494 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-through from the #436 review discussion: 8c021a9's cache-invalidation policy is correct (stale shrink-aggregates/ROW() must never ship), but it left
insert-rows/delete-rows/insert-cols/delete-colsoutput with no cached<v>workbook-wide — blank in openpyxldata_only/pandas/xl viewuntil a manual recalc, the original field complaint from the Delrin audit.This wires the #352 batch contract into the four structural commands: one global
recalculate()before write — fresh-correct caches, failing cells stay uncached, the file is written regardless, errors surface in the command summary.Pins (StructuralCommandSpec): a deliberately-wrong stale cache (
999→ recomputed4) and a shrinking-SUM delete (SUM(A1:A3)cached6→SUM(A1:A2)re-baked to the true4) — distinguishing fresh-correct from both stale-preserved and absent. Empirical:insert-rowson the audit fixture now ships<f>A4*2</f><v>4</v>, the array record with<v>60</v>, equals-free throughout.Gates: xl-cli 343/343, full suite 1028/1028, scalafmt CI form clean.
Refs #427
🤖 Generated with Claude Code