Skip to content

feat(engine): MulmoClaude 側で育った engine を持ち帰る(テスト一式と、Excel と違っていた期待値2件の修正つき) - #31

Merged
isamu merged 1 commit into
mainfrom
backport/engine-from-mulmoclaude
Sep 24, 2026
Merged

isamu merged 1 commit into
mainfrom
backport/engine-from-mulmoclaude

Conversation

@isamu

@isamu isamu commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

src/engine をまるごと、MulmoClaude 側で育った engine に入れ替えます(#30)。もとは同じコピーで、向こうだけが進み続けていました。engine 自身のテストも一緒に持ち込みます。

  • engine の入れ替え: 参照の解決は parser.ts から formulaRefs.ts へ移り、集計は cellBuilder / condition / numericCoercion を通り、金融・統計・検索・数学のヘルパーはそれぞれ独立したファイルになっています。
  • src/engine/guards.ts: engine が必要とする実行時ヘルパー3つ(isRecord / isObj / errorMessage)。MulmoClaude の leaf パッケージから写したもので、依存はしていません。gui-chat の plugin がホストのパッケージに依存してはいけないためです。
  • tests/engine: engine の node:test 一式。新しい test:engine script で走り、test:all にも組み込みました。
  • fixture の期待値2件を Excel に合わせました: 最初の利息支払いの符号と、最初の元金支払いの値(PMT - IPMT になっていませんでした)。理由は tests/engine/fixtures/README.md に書いたので、将来また戻されることはありません。

Items to Confirm / Review

  • engine は公開 API ではありません。 exports は . / ./core / ./vue / ./style.css で、core/index.ts は engine を再公開していません。vue/View.vue が使うのは SpreadsheetEngine / columnToIndex / indexToColumn の3つだけで、どれも残っています。利用者向けの破壊的変更はありません。
  • parseCellRef / parseRangeRef / cellRefToA1 は復活させませんでした。 この engine には無く、参照解決は expandRangeOrCell に移っていて、そちらにテストがあります。parser.test.ts の該当ブロックは消し、列変換のテストは残しました(View がまだ使うため)。消してよかったかは見てください。
  • fixture の2件は、こちらの期待値が間違っていました。 IPMT(5%/12,1,24,10000) は Excel では支払いなので負、PPMT は PMT - IPMT です。同じ fixture の PMT はもともと負を期待していたので、中でも食い違っていました。Excel で確かめ直せる方に見てほしい箇所です。
  • run-evaluator-tests.ts の偽コンテキストを直しました。 range を読む関数が単一セルに何も返さないため、MAX(B1, A1:A3, 5) が B1 を黙って落としていました。本物のコンテキストは参照をすべて expandRangeOrCell で解決するので、偽物もそれに合わせています。engine 側は変えていません(MulmoClaude の engine を実際のシートで叩くと Excel と同じ 75 を返します)。
  • type を interface にした箇所が3つあります(calculator.ts ×2、functions/statistical.ts ×1)。このリポジトリの lint 規則がそう要求するためで、中身は同じです。
  • calculator.ts の CellPosition.cells が any[] のままです。 持ち込んだコードのままにしました。直す価値はありますが、この PR では型を触らない方針にしています。
  • vue/View.vue は手を付けていません。 MulmoClaude 側の View は wiki リンクや markdown 向けの処理などホスト固有の依存を持つので、次段にします。

挙動が変わるところ(レビューで両エンジンを走らせて測りました)

fixture の2値以外にも、旧エンジンが黙って空や 0 を返していた3つが値を返すようになります。

  • 小文字の範囲参照(a1:a3)
  • 絶対参照の範囲($A$1:$A$3)
  • 集計関数に素のセル参照を混ぜた形(MAX(B1, A1:A3, 5) の B1)

これらは旧エンジンの取りこぼしで、どれも正しい値を返す方向の変化です。それ以外の入力については、旧エンジンを取り出して並べて走らせ、算術・セル参照の算術・SUM / AVERAGE の範囲・IF・文字列結合・シート跨ぎ参照・DAY(date)・2^3^2 の結合順まで一致を確認しています。

確認したこと

typecheck / lint(エラー0)/ build と、test / test:engine / test:fixtures / test:calculator / test:evaluator / test:functions がすべて通ります。

User Prompt

  • gui-chat-protocol のスプレッドシートの plugin がどこにあるか確認したい。
  • src/plugins/spreadsheet/ はこの plugin をコピーして改良したもの。こちらのほうが進んでいるので、上流に戻したい。
  • 同じようにコピーして改良したものが他にもあるはずなので、それも戻したい。
  • 5組すべて見直す。戻したあと MulmoClaude 側のコピーをどうするかは、今回は決めない。

Closes #30

🤖 Generated with Claude Code

The engine here and the one in MulmoClaude's src/plugins/spreadsheet started as
one copy. The copy kept going: its formula handling was split into small units
with tests of their own, and several Excel behaviours were corrected along the
way. This brings that engine back, with its tests.

What arrives with it:

- src/engine is replaced wholesale. The reference handling moved from parser.ts
  into formulaRefs.ts, the aggregates read through cellBuilder / condition /
  numericCoercion, and the financial, statistical, lookup and math helpers are
  separate units.
- src/engine/guards.ts holds the three runtime helpers the engine needs
  (isRecord, isObj, errorMessage). They come from MulmoClaude's leaf package;
  copied rather than depended on, because a gui-chat plugin must not take a
  dependency on a host's packages.
- tests/engine gains the engine's own node:test suite, run by a new
  `test:engine` script and folded into `test:all`.

Two expected values in the financial fixture were wrong and now match Excel: the
first interest payment had the sign of money coming in rather than going out,
and the first principal payment was not PMT - IPMT. The reason is recorded in
tests/engine/fixtures/README.md so a future change cannot quietly flip them back.

Two test-side repairs, neither of which changes the engine:

- parser.test.ts kept exercising parseCellRef / parseRangeRef / cellRefToA1,
  which this engine does not have; expandRangeOrCell covers that ground and has
  its own tests. Column conversion stays, since the Vue view still uses it.
- run-evaluator-tests.ts built a context whose range readers answered nothing
  for a single cell, so MAX(B1, A1:A3, 5) silently dropped B1. The real context
  resolves every reference through expandRangeOrCell; the fake now does too.

typecheck, lint, build, and every test script pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@isamu

isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Evidence for the two corrected fixture values, from outside this repository (asked for on review):

Microsoft's own IPMT page states the convention — "For all the arguments, cash you pay out, such as deposits to savings, is represented by negative numbers. Cash you receive, such as dividend checks, is represented by positive numbers." — and its worked example returns ($66.67) for the first month's interest on an $8,000 loan, i.e. negative for a positive pv.

So for IPMT(0.05/12, 1, 24, 10000) the interest payment is negative, and since PMT = IPMT + PPMT, the first principal payment is PMT - IPMT:

  • PMT = 10000 · r / (1 - (1+r)^-24) with r = 0.05/12 → about -438.71
  • IPMT(1) = -pv · r → -41.67
  • PPMT(1) = PMT - IPMT → -397.05

Which is what this engine returns, and what the fixture now expects. The old expectation had the interest payment positive while the PMT row in the same fixture was already negative.

Source: https://support.microsoft.com/en-us/office/ipmt-function-5cce0ad6-8402-4a41-8d29-61a0b054cb6f

@isamu

isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

CODEX VERDICT: LGTM

  • No blocking findings.
  • Verified the two financial fixture corrections independently: for IPMT(0.05/12,1,24,10000), PMT is about -438.713897, period-1 IPMT is -41.666667, and PPMT is PMT - IPMT = -397.047231, so the new expected values round correctly.
  • Ran the requested suite locally: yarn test, yarn run test:engine, yarn run test:fixtures, yarn run test:calculator, yarn run test:evaluator, yarn run test:functions, yarn typecheck, yarn lint, yarn build, and yarn run test:all all exit 0. Lint reports warnings only.
Axis Result
Corrected fixture values No findings. The new negative IPMT and PPMT = PMT - IPMT values are right under the standard annuity cash-flow sign convention. The old +$41.67 IPMT and -$480.38 PPMT were the wrong values.
Removed parser helpers No findings. parseCellRef, parseRangeRef, and cellRefToA1 are gone from src/engine/parser.ts; rg shows no use in src/ or demo/. package.json exports only ., ./core, ./vue, and CSS; src/index.ts and src/core/index.ts do not expose engine parser helpers. src/engine/index.ts did re-export parser before, but that subpath is not a package export. The new test_expandRangeOrCell.ts and test_formulaRefs.ts cover range/single-cell expansion, $ refs, lowercase refs, Z->AA, malformed refs, and extraction. The surviving parser.test.ts cases still matter because columnToIndex / indexToColumn remain used by src/vue/View.vue and lookup code.
Behaviour over same inputs No findings. I imported origin/main's engine from a /tmp archive and compared it with this branch by running formulas. Representative old-supported inputs agree: arithmetic, cell arithmetic, SUM/AVERAGE ranges, MAX mixed args, ROUND over SUM/COUNT, IF, string concat, cross-sheet refs, DAY(date), and the known 2^3^2 JS-associativity behaviour. Deliberate differences observed by running: IPMT/PPMT corrected, plus lowercase ranges, absolute ranges, and bare-cell aggregate references now return the spreadsheet value instead of silently empty/0.
src/engine/guards.ts No findings. The copied isRecord, isObj, and errorMessage implementations match the MulmoClaude common package semantics. rg inside this repo finds no existing equivalent outside the new file, so this is not duplicating a local helper.
Tests brought in / CI No findings. .github/workflows/pull_request.yaml runs yarn run test:all, and test:all now runs vitest, yarn run test:engine, and fixtures. Local test:engine reports 951 tests, 228 suites, 951 pass, 0 skipped/todo/fail, so it is not passing vacuously.
Evaluator harness single-cell ranges No findings. The fake context now returning [cell] for a single-cell range matches real collectRangeValues, which calls expandRangeOrCell; expandRangeOrCell("B1") returns one coordinate, and collectRangeValues reads that cell. This specifically prevents MAX(B1, A1:A3, 5) from passing after dropping B1.
API / on-disk compatibility No findings. Fixture input/expected JSON shape is unchanged; only two expected cell display strings and README text changed. Package public exports remain ., ./core, ./vue, and ./style.css; top-level/core public surfaces do not expose engine names. The engine index exports more internals, but the engine subpath is not in package.json exports.
Security No findings. Dynamic arithmetic/comparison/concat evaluation still reaches new Function only after substitution plus character/structure allowlists; string literals are masked for concat validation, refs inside literals are skipped, and condition evaluation no longer executes code. Criteria regexes escape regex metacharacters before applying spreadsheet wildcards. Range expansion remains formula-controlled and can be large, but I did not find a new unbounded-expansion class beyond the existing spreadsheet range model.
Consistency and lint No findings. The type to interface conversions are ordinary object-shape declarations and typecheck cleanly. The dead parser import was removed. any warnings exist in the copied calculator/Vue surface, but yarn lint exits 0 with warnings only; I did not find one that changes behaviour or blocks this PR. git diff --check is clean.

FINDINGS COMPLETE: I read every hunk of the diff and this is every finding I have.

@isamu

isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Codex cross-review — round 1 (tier C): clean

Codex: CODEX VERDICT: LGTM, every axis answered, FINDINGS COMPLETE present. Nothing has been pushed since the head it read, so this is the clean round on the head that would merge.

What it did rather than read:

  • Checked the two corrected fixture values independently: PMT ≈ -438.713897, period-1 IPMT = -41.666667, PPMT = PMT - IPMT = -397.047231. The new expected values round to those; the old +$41.67 / -$480.38 were wrong.
  • Ran both engines side by side — it extracted origin/main's engine into a temp archive and compared outputs. Agreement on arithmetic, cell arithmetic, SUM/AVERAGE ranges, MAX with mixed arguments, ROUND over SUM/COUNT, IF, string concatenation, cross-sheet references, DAY(date), and the 2^3^2 associativity quirk.
  • Found three further deliberate differences I had not stated: lowercase ranges, absolute ranges, and bare-cell aggregate references now return the value instead of silently empty/0. Those are fixes the adopted engine brings, and I have added them to the PR body.
  • Confirmed the removed helpers are unreachable: parseCellRef / parseRangeRef / cellRefToA1 have no use in src/ or demo/, and the package exports only ., ./core, ./vue, ./style.css — the engine subpath was never published.
  • Confirmed the ported suite is not vacuous (951 tests, 228 suites, 0 skipped) and that CI reaches it through test:all.
  • Confirmed the evaluator harness change is faithful: the real context resolves a single-cell reference through expandRangeOrCell, which is what the fake now does — and it is what stops MAX(B1, A1:A3, 5) from passing while dropping B1.

My own sign-off

No MUST-FIX from either reviewer. One note I am deliberately leaving: src/engine/formulaRefs.ts carries a historical comment mentioning parseCellRef, a function this PR removes. It is a why-comment about the behaviour expandRangeOrCell exists to avoid, and the same line is in the other repo's copy — editing it here would diverge the two files for a sentence that misleads nobody into a wrong change. Recorded rather than fixed.

Prompt sent (verbatim)
Review PR #31 at https://github.com/receptron/GUIChatPluginSpreadsheet/pull/31 (branch backport/engine-from-mulmoclaude, base main, 0 behind).

Tier C. This replaces src/engine wholesale with an engine developed in another repository (MulmoClaude's spreadsheet plugin) and CORRECTS TWO EXPECTED VALUES in a test fixture. Every formula this plugin evaluates passes through the replaced code, and a wrong fixture correction bakes a wrong answer into the suite.

── THIS IS THE ONLY ROUND YOU ARE GUARANTEED ────────────────────────────────
A finding held back costs a full extra round. Read the WHOLE diff before posting. Do not stop at the first problem.

Use the gh CLI to inspect the diff, then post inline comments via
  gh api repos/receptron/GUIChatPluginSpreadsheet/pulls/31/comments
or top-level via
  gh pr comment 31

You can run everything: `yarn test`, `yarn run test:engine`, `yarn run test:fixtures`, `yarn run test:calculator`, `yarn run test:evaluator`, `yarn run test:functions`, `yarn typecheck`, `yarn lint`, `yarn build`. Use them.

── AXES — answer EVERY line, even when the answer is 'no findings' ──────────
  the two corrected fixture values: `IPMT(0.05/12,1,24,10000)` now expects a NEGATIVE interest payment and `PPMT` expects PMT - IPMT. Verify against the annuity formulas yourself and say whether the NEW numbers are right. If either is wrong, this PR bakes a wrong answer into the suite.
  what the replacement REMOVES: parseCellRef / parseRangeRef / cellRefToA1 are gone from src/engine/parser.ts and their tests were deleted from tests/engine/parser.test.ts. Confirm nothing in src/ or demo/ used them, that the package's public surface (package.json exports, src/index.ts, src/core/index.ts) never exposed them, and that the ground they covered is covered by the new formulaRefs tests.
  behaviour over the same inputs: the old engine is at `git show origin/main:src/engine/...`. Sample some formulas the old one handled and check the new one agrees, or name where it deliberately differs. Both are importable, so compare by RUNNING rather than by reading.
  src/engine/guards.ts: three helpers copied from the other repo rather than depended on. Check they behave as the originals did and that nothing else in this repo already provides them.
  the tests brought in: ~950 node:test cases now run via the new `test:engine` script, which the CI reaches through `test:all`. Confirm they actually run in CI (read .github/workflows/pull_request.yaml), that they are not passing vacuously, and that `tests/engine/parser.test.ts`'s surviving cases still mean something.
  the evaluator harness change in tests/engine/run-evaluator-tests.ts: its fake context now answers the RANGE readers for a single cell. Confirm that matches how the real context resolves references (src/engine/calculator.ts's collectRangeValues / expandRangeOrCell) and that it does not make a test pass for the wrong reason.
  API / on-disk compatibility: the fixture JSON format, the engine's exported names, anything a consumer of `@gui-chat-plugin/spreadsheet` could reach.
  security: formula evaluation over untrusted input — anything the new engine evaluates that the old one did not (dynamic dispatch, RegExp built from input, unbounded expansion).
  consistency and lint: the three `type` → `interface` conversions, the removed dead import, `any` that arrived with the copy.

An axis with no findings is a RESULT and must be stated.

── EACH FINDING CARRIES FOUR THINGS ─────────────────────────────────────────
1. SEVERITY — P1 / P2 / P3 / N (N = prose-only, does not block the verdict).
2. EVERY SITE — grep and list them all now.
3. THE SMALLEST CHANGE THAT RESOLVES IT.
4. WHAT WOULD CHANGE YOUR MIND.

── ALREADY SETTLED ─────────────────────────────────────────────────────────
| # | iter | finding | disposition | why |
|---|---|---|---|---|
| 0 | 0 | sandbox pre-flight | OK | test / test:engine / test:fixtures / typecheck all exit 0; socket bind 0; api.github.com 200; deletions permitted |
| 1 | pre | the fixture's IPMT sign and PPMT value disagreed with Excel | FIXED | corrected in this PR; Microsoft's IPMT page states payments are negative for a positive pv, and PMT = IPMT + PPMT |

── END WITH ONE TOP-LEVEL COMMENT, IN THIS ORDER ────────────────────────────
  Line 1: 'CODEX VERDICT: LGTM' or 'CODEX VERDICT: CHANGES REQUESTED' + bulleted summary.
  Then the axis table, one row per axis.
  Then: 'FINDINGS COMPLETE: I read every hunk of the diff and this is every finding I have.'
Do not apply any fixes yourself.
Codex stdout (verbatim)
Posted the PR review comment: https://github.com/receptron/GUIChatPluginSpreadsheet/pull/31#issuecomment-5821729039

Verdict was `CODEX VERDICT: LGTM`. I verified the fixture math, old-vs-new behavior by running both engines, CI wiring, guard copies, harness change, API surface, security shape, lint/typecheck/build/tests, and found no blocking issues.
codex exit=0

@isamu
isamu merged commit bf753c6 into main Sep 24, 2026
11 checks passed
@isamu
isamu deleted the backport/engine-from-mulmoclaude branch September 24, 2026 21:50
isamu added a commit that referenced this pull request Sep 24, 2026
refactor(view): ハイライトと矢印キーの純粋部分を持ち帰る(#31 の上に積んでいます)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine が MulmoChat 側のコピーに追い越されている — 持ち帰って入れ替える(fixture の期待値2件も Excel と違う)

1 participant