fix(cli): Polish eval inference, style labels, rasterizer exit code, number formatting (#210, #211)

[arcaputo3]

Summary

• Eval sheet inference (enhancement(cli): CLI polish — eval sheet inference, style log label, rasterizers exit code #210): Add DependencyGraph.containsUnqualifiedCellReferences so eval with fully-qualified formulas (e.g., =SUM(Revenue!B2:B5)) no longer requires the -s flag. Qualified-only formulas use the first sheet as ambient context since the evaluator resolves cross-sheet refs by name.
• Style log label (enhancement(cli): CLI polish — eval sheet inference, style log label, rasterizers exit code #210): Change "(merge)" → "(additive)" and "(merge, streaming)" → "(additive, streaming)" in style command output.
• Rasterizers exit code (enhancement(cli): CLI polish — eval sheet inference, style log label, rasterizers exit code #210): rasterizers command now always returns ExitCode.Success (informational command, not a failure).
• Number formatting (bug(cli): cell command Formatted field shows scientific notation instead of applying number format #211): Fix NumFmtFormatter.formatGeneral to use n.underlying.stripTrailingZeros.toPlainString instead of n.toString, preventing scientific notation (e.g., 5.809440E-01) in cell command Formatted field.

Test plan

• ./mill xl-evaluator.test — 174/174 pass
• ./mill xl-core.test — 127/127 pass
• ./mill xl-cli.test — 317/317 pass
• ./mill __.checkFormat — all pass

Closes #210
Closes #211

🤖 Generated with Claude Code

[claude]

Code Review: PR #216

This PR addresses four quality-of-life improvements in the CLI. Overall, the implementation is well-structured and thoughtful, with good attention to architectural consistency. Here's my detailed feedback:

---

✅ Strengths

1. Smart Sheet Inference Logic (GH-210)

The new containsUnqualifiedCellReferences method elegantly solves the qualified formula problem. The three-tier branching (needsSheet, hasAnyCellRefs, constant formula) is clean and logically sound:

• Unqualified refs → require -s flag
• Qualified-only refs → use first sheet as ambient context
• Constants → no file needed

Key insight: The distinction between "needs a sheet" vs "has cell references" is architecturally correct—qualified formulas like =SUM(Revenue!B2:B5) genuinely don't need sheet selection.

2. Number Formatting Fix (GH-211)

The fix in NumFmtFormatter.scala:130 is perfect: using .underlying.stripTrailingZeros.toPlainString correctly prevents scientific notation while maintaining precision. This is the canonical way to format BigDecimal in Excel-like contexts.

3. Clear Label Change

"(merge)" → "(additive)" is much clearer for users—eliminates cell merge confusion.

4. Sensible Exit Code

rasterizers returning ExitCode.Success is correct—it's an informational command, not a failure state.

[claude]

⚠️ Code Duplication Concern (High)

Issue: ReadCommands.scala:370-440 and 441-500 contain ~70 lines of near-identical code for dependency graph evaluation. The only difference is line 381 vs 451:

// needsSheet branch
sheet <- SheetResolver.requireSheet(wb, sheetOpt, "eval")

// hasAnyCellRefs branch  
val sheet = sheetOpt.getOrElse(wb.sheets.head)

DRY Principle Violation: This creates maintenance burden—future bug fixes must be applied twice. Given the project's emphasis on purity and law-governed semantics, this duplication is inconsistent with the codebase's high standards.

Recommendation: Extract the common evaluation logic into a private helper that accepts the sheet as a parameter. This would reduce the code to ~15 lines and ensure consistency between both branches.

[claude]

🔍 Missing Test Coverage

Critical Gap: No tests verify containsUnqualifiedCellReferences behavior. The method has complex pattern matching (71 lines) covering:

• Qualified refs (SheetRef, SheetPolyRef, SheetRange)
• Unqualified refs (Ref, PolyRef, RangeRef)
• Binary/unary operators (12+ cases)
• Function calls with recursive traversal

Recommended Test Cases:

// xl-evaluator/test/src/.../graph/DependencyGraphSuite.scala
test("containsUnqualifiedCellReferences - qualified only") {
  val expr = parse("=SUM(Revenue!B2:B5)").toOption.get
  assert(!DependencyGraph.containsUnqualifiedCellReferences(expr))
}

test("containsUnqualifiedCellReferences - mixed qualified/unqualified") {
  val expr = parse("=A1 + Revenue!B2").toOption.get
  assert(DependencyGraph.containsUnqualifiedCellReferences(expr))
}

test("containsUnqualifiedCellReferences - nested aggregates") {
  val expr = parse("=SUM(A1:A10) + SUM(Data!B1:B10)").toOption.get
  assert(DependencyGraph.containsUnqualifiedCellReferences(expr))
}

test("containsUnqualifiedCellReferences - constants only") {
  val expr = parse("=10 + 20").toOption.get
  assert(!DependencyGraph.containsUnqualifiedCellReferences(expr))
}

These tests would validate the core logic and prevent regressions.

[claude]

🔒 Security & Performance

No Concerns Identified:

• ✅ No new unsafe operations or external I/O
• ✅ Bounded dependency extraction (extractDependenciesBounded) already prevents 1M+ cell enumeration
• ✅ BigDecimal operations use safe methods (stripTrailingZeros, toPlainString)
• ✅ Exit code change doesn't introduce privilege escalation risks

---

📋 Minor Observations

1. Nowarn Annotation: DependencyGraph.scala:196 has @nowarn("msg=Unreachable case") on containsUnqualifiedCellReferences. This is correct—exhaustiveness checking can't prove all TExpr cases are covered. Consider adding a comment explaining why.

2. Comment Precision: Line 442-443 states "any sheet works as the ambient context"—this is true because qualified refs resolve by name via the workbook. A subtle point well-documented.

3. Code Style Adherence: All changes follow the project's opaque types, total functions, and Either-based error handling. Scalafmt compliant.

---

🎯 Verdict

Approve with Recommendations:

• ✅ Merge-ready from a functionality perspective—fixes all four issues correctly
• ⚠️ Strongly recommend refactoring the duplicated evaluation logic before merge (reduces future maintenance burden)
• ⚠️ Add unit tests for containsUnqualifiedCellReferences to prevent regressions

Risk Assessment:

• Low risk for immediate merge (tests pass, no breaking changes)
• Medium long-term maintenance risk due to code duplication

---

💡 Suggested Follow-Up

If refactoring in this PR risks delaying the merge, consider:

1. Merge as-is with a follow-up issue: "Refactor ReadCommands evaluation logic duplication"
2. Add // TODO: Extract common dependency evaluation logic comments at lines 370 and 441

Great work on the fixes—especially the qualified formula inference and BigDecimal formatting! 🚀