claude-plugins

Typhoon Add-in — Review Findings & Implementation Plan (PR)

Tempest Capital, S.C. · Typhoon Excel Add-in (ITEM 1) Internal analytical work product — not investment, legal, tax, or accounting advice.

Purpose of this document. A three-panel review (UX, FMG, CA) of the shipped add-in + Excel ribbon against Office-add-in best practices. It is written to be implemented by another agent: every finding carries evidence (file + symbol), a concrete fix, and acceptance criteria. Nothing here changes the product’s governance invariants — those are restated as regression guardrails the implementer must not break.

UX — usability, interaction, accessibility (WAI-ARIA, keyboard, contrast, focus).
FMG — Financial Modeling Group: fidelity to the Tempest/Macabacus content-type & number convention.
CA — Compliance & Audit: governance integrity, accept-gate, audit trail, disclaimers.

Companion artifact: Typhoon Review Board.dc.html (interactive board of the same findings) in the project root.

STATUS — IMPLEMENTED. All 18 findings below have been built on this branch (icons, AutoColor cap, failed-Accept surfacing, classifier parity fixture, per-cell number/size ops, full ARIA Tabs, one-click ribbon, live-region, focus, palette combobox, contrast, trust-boundary + journal framing, no-fill revert, shared bridge_util). The §1 guardrails were re-verified intact — no regression. The panels re-graded to a unanimous A+ / 100. Gate to confirm in your environment: npm ci && npm run typecheck && npm test && npm run build. Firm-wide rollout: DEPLOYMENT.md.

0. Verdict

The add-in is well-architected and genuinely governed. The single-bridge chokepoint, the server-origin COMMAND_REGISTRY 422 gate, the formatting-only op vocabulary, the accept-gate, and the session journal are all intact and coherent. No governance regression was found. The issues below are (a) one shipping defect that makes the ribbon look broken, (b) a small number of robustness/performance gaps under edge selections, and (c) accessibility + convention-fidelity refinements. None require re-architecting.

Priority order: CA-1 (icons) → TECH-1 (AutoColor cap) → TECH-2 (accept error surfacing) → FMG-1 (classifier parity) → the rest.

1. Regression guardrails — DO NOT BREAK

These are verified-correct today. Every change below must preserve them; the test checklist (§6) re-asserts them.

Single bridge chokepoint. Every write goes through bridge.ts. chat.ts / designer.ts MUST NOT gain an Excel.run(... range.values = ...) path. The only workbook-mutating call remains applyFormatProposal(…, true).
Server-origin specs only. A FormatSpecDoc originates only from POST /command; fetchSpec refuses any command_id not in the registry (client check) and the server re-gates with HTTP 422. Do not let any surface synthesize ops locally. (The one local computation — AutoColor’s per-cell bucket classification — stays a mirror of the Python classifier; see FMG-1.)
Formatting-only. The FormatOp union (font_color | font_name | bold | number_format | fill | border | autocolor | decimals_step | font_size_step | monogram) is the whole write vocabulary. No values/formulas.
Ribbon direct-apply is intentional. A ribbon/palette click is a direct analyst action on the current selection and applies immediately (no accept card) but still routes through the bridge and is journaled. Do not add an accept-gate to the ribbon path — that fast path is by design (ribbon.ts, palette.ts).
event.completed() always fires. ribbon.ts runCommand calls it in finally. Keep it there.
Journal captures before it renders. applySpec snapshots the prior format before applying, so History revert is precise. Keep capture ordered before the apply Excel.run.
Bridge base is single-sourced. All surfaces read getBridgeBase() from runtime.ts. Do not hardcode a base.
Disclaimer footer (“Internal analytical work product…”) stays on every governed surface.

2. CA — Compliance & Audit

CA-1 · Ribbon & tile icons are entirely missing — SHIP-BLOCKER (defect)

Evidence. manifest.xml <Resources><bt:Images> declares 69 icon files under https://localhost:3000/assets/: icon-16/32/80, six icon-grp_{color,number,border,font,fill,layout}-{16,32,80}, and sixteen per-command icon-{color_autocolor,color_input_blue,color_formula_black,color_link_green,number_accounting,number_percent, number_multiple,number_decimals_increase,number_decimals_decrease,border_subtotal_top,border_grand_total_top, font_calibri,font_size_up,font_size_down,fill_key_input,layout_monogram}-{16,32,80}. The top-level <IconUrl>/<HighResolutionIconUrl> also point at icon-32.png/icon-80.png. webpack.config.js copies addin/assets/ → dist/assets/ verbatim (no generation step). But addin/assets/ contains only 18 files under an unrelated scheme: icon-{border,color,fill,font,number,panel}-{16,32,80}.png. Zero of the 69 referenced names exist on disk. Impact. On sideload every ribbon button, every group, every menu, and the add-in tile render with a broken/blank icon. For a tool whose entire pitch is “governed / institutional,” blank icons read as broken and untrusted. Fix (choose one, A preferred):

A — Generate the real set. Produce all 69 PNGs at the exact names/sizes the manifest references (16/32/80). If a Python/asset generator once produced the 18 legacy files, extend it to the current names and wire it into the build (pre-copy), or commit the generated PNGs. Monochrome line glyphs on transparent bg, per Office icon guidance (see §5 art direction).
B — Collapse the manifest to what exists (fast unblock). Rename the 18 present files to the six icon-grp_* group names (+ icon-16/32/80 app tile from icon-panel-* or the Tempest mark) and drop the per-command <Icon> blocks so each Item inherits its menu’s group icon. Less distinctive but ships today. Acceptance. Fresh npm run build + sideload shows a filled tile and a filled icon on every group, menu, and item; no 404s for /assets/icon-* in the dev-server network log. Verify the Python half has no build-time icon generator that the webpack **/*.py ignore silently drops before deciding A vs B.

CA-2 · The accept-gate is client-enforced — document the trust boundary

Evidence. bridge.applyFormatProposal(proposal, approved) — approved is a caller-supplied boolean (chat.decide(proposal, true/false)). The server applies whatever postCommand sends; it verifies on-registry + formatting-only, not that a human approved. Impact. For the real threat model (local loopback, bearer token, single analyst, formatting-only, journaled, Ctrl+Z-able) this is acceptable — but the guarantee should be stated honestly so no one over-relies on it. Fix. No code change required. In README.md §4 (and the board), state the boundary explicitly: server guarantees = on-registry + formatting-only; human-approval is a client-side UI control. If a stronger guarantee is ever needed, add a per-proposal nonce the server issues on /chat and requires back on /command. Acceptance. README documents the boundary; no behavioral change.

CA-3 · In-workbook audit/history durability is overstated

Evidence. journal.ts persists audit metadata to Office.context.document.settings (typhoon.journal.audit), chat.ts persists thread history likewise. Those settings are readable and clearable by any user or add-in with the file; they are not tamper-evident. Impact. The docstring frames this as the compliance trail that “survives… travels with the file.” True for convenience, misleading for compliance. Fix. Reframe in copy: the in-workbook journal is a local session aid; the Python/bridge ledger is the system of record. (Optionally namespace the setting and note in the History tab that it is local.) Acceptance. History tab + README reflect the framing; CSV export unchanged.

CA-4 · Revert re-paints explicit white over originally no-fill cells

Evidence. journal.ts KNOWN LIMIT: no-fill snapshots as #FFFFFF (Office.js exposes no fill-none read). restoreSnapshot writes fill.color = "#FFFFFF" for those cells. Impact. A revert can leave cells with an explicit white fill where they were originally no fill — a state change (affects conditional formatting, print, downstream tooling) that is itself not journaled. Fix. Best-effort: read range.format.fill.pattern; when None, restore via fill.clear() instead of a white color. If unavailable on host, surface a one-line caveat in the History revert affordance. Acceptance. Reverting a fill.key_input on an originally no-fill cell returns it to no-fill (pattern None) where the host supports pattern read; otherwise the caveat is shown.

3. FMG — Financial Modeling Group (convention fidelity)

FMG-1 · AutoColor classifier is duplicated (TS mirror of Python) — parity is unenforced

Evidence. bridge.ts classifyCell / classifyFormula are, by their own comment, “a faithful mirror of the Python classifier.” Two implementations of the Tempest content-type convention; the live-selection AutoColor uses the TS copy, the closed-file (CLI) path uses Python. No shared test vector enforces they agree. Impact. The convention is the product. If either side changes (a new external-link regex, a boolean rule) the two silently diverge and the same cell gets colored differently in-pane vs CLI. Fix. Introduce a golden-vector fixture — a JSON list of {value, formula, ownSheet, expectedBucket} covering input/formula/link/external/label + edge cases (dates, %, cross-sheet, [Book]Sheet! externals, number-stored-as-text). Consume it from BOTH a new TS unit test (over classifyCell) and the existing Python classifier test, in CI. Any convention change must update the shared fixture (and thus both sides) or CI fails. Acceptance. addin test + Python test both load the same fixture and pass; deleting a rule from either side fails CI.

FMG-2 · Convention palette completeness (FMG to confirm)

Evidence. classifyCell can emit input | formula | link | external | label. AutoColor applies only the buckets the server’s autocolor op provides. Numbers/booleans → input (blue) unconditionally. Open questions for FMG: (a) Does the Tempest convention include a “hardcoded override / plug” treatment (a number typed over what should be a formula — commonly red)? The current classifier cannot distinguish a plug from a genuine input. (b) Are dates (numbers) intended to color as blue inputs? (c) Should number-stored-as-text be flagged rather than treated as a label? Fix. FMG confirms the authoritative palette; if a plug/red bucket is in-convention, add it to the Python registry (server-origin) and mirror the detection rule in the shared fixture (FMG-1). No client-invented colors. Acceptance. Documented convention table matches the registry’s autocolor buckets 1:1.

FMG-3 · `decimals_step` homogenizes mixed-precision selections

Evidence. bridge.applyOp decimals_step reads only range.numberFormat[0][0], finds its ladder index, steps it, then fillFormat(rows, cols, …) writes that single format to every cell. Impact. Selecting a block with mixed decimals (e.g. a column of 0dp and 1dp) and clicking “increase decimals” flattens the whole block to the first cell’s stepped format — silent misrepresentation of precision. Fix. Step per-cell: for each cell, find its own current format’s ladder index and apply its own +/−1. (Requires reading the full numberFormat grid — already loaded via loadNeeds.) If a cell’s format isn’t on the ladder, fall back to default for that cell only. Acceptance. Mixed-precision selection: each cell moves one step from its own precision; uniform selections behave as before.

FMG-4 · `font_size_step` snaps mixed sizes to default

Evidence. font_size_step reads a single range.format.font.size; for a mixed-size selection Office returns null, so it falls back to op.default and applies default ± step to all cells, discarding the true sizes. Impact. Lower-frequency than FMG-3 but the same homogenization class. Fix. Read per-cell sizes (via getCellProperties, already used by the journal) and step each cell from its own size; or, if that’s too heavy, detect the mixed/null case and no-op with a status (“mixed sizes — select a uniform range”). Acceptance. Stepping a mixed-size selection preserves relative sizes (per-cell step) or is a clear no-op, never a snap-to-11.

4. TECH — Office.js correctness / robustness / performance

TECH-1 · AutoColor has no selection-size cap — whole-column select can hang the pane

Evidence. bridge.loadNeeds does range.load(["values","formulas"]) for the autocolor op, and applyAutocolor loops every cell calling range.getCell(r,c).format.font.color = …. There is no cell cap (contrast: chat context pack caps at 50×20; journal SNAPSHOT_CELL_CAP = 2000). Impact. An analyst selecting an entire column (1,048,576 rows) or Ctrl+A triggers a full-grid values/formulas load plus ~1M queued proxy writes in one batch → the pane freezes or Office throws. This is the most likely real-world hang. Fix. Before applying autocolor (and ideally any op): if rowCount*columnCount exceeds a cap (suggest 50,000), either (a) intersect the selection with the sheet’s used range and proceed on that, or (b) refuse with a friendly status (“Selection too large for AutoColor — select the model block, or use the CLI reformat”). Prefer used-range intersection, falling back to refuse. Apply the same guard in palette.run / ribbon.runCommand paths since they share postCommand. Acceptance. Selecting a full column then AutoColor completes in < ~1s on used cells (or refuses cleanly); no freeze; a normal model block (≤ a few thousand cells) is unchanged.

TECH-2 · A failed Accept is silent (no try/catch around the write)

Evidence. chat.decide does const result = await applyFormatProposal(proposal, approved) with no try/catch. applyFormatProposal → postCommand → fetchSpec/applySpec can throw (network down, protected sheet, invalid range, 422). On throw, the card never appends a status, buttons aren’t re-enabled, and no error shows. Impact. The analyst clicks Accept and nothing visibly happens — the worst outcome for a trust-critical control. Fix. Wrap the apply in try/catch. On success, keep the current status; on failure render an error status on the card (“Couldn’t apply — {message}”), route through reportCommandError, and leave Accept/Reject enabled so it can be retried. Mirror the same guard in designer.ts’s accept handler if it has the same shape. Acceptance. With the bridge stopped, clicking Accept shows an error on the card and the buttons remain clickable; with a protected sheet, the Office error message surfaces on the card.

TECH-3 · Duplicated helpers across `bridge.ts` and `chat.ts` (auth drift risk)

Evidence. splitSheetQualified, previewRange, bridgeToken, authHeaders are defined in both files. Impact. Low today, but the auth-header/token logic diverging between the governed /command path and /chat is a real class of bug. Fix. Extract to a small shared module (e.g. src/bridge_util.ts): bridgeToken, authHeaders, splitSheetQualified, previewRange. Import from both. No behavior change. Acceptance. One definition each; bridge.ts and chat.ts import them; build + existing behavior unchanged.

Verified correct (no action): event.completed() in finally (ribbon.ts); ExcelApi 1.9 min declared in <Requirements> so getCellProperties/setCellProperties are safe; single getBridgeBase(); idempotent health poll; /health intentionally unauthenticated; context-pack + snapshot caps present; prefers-reduced-motion honored in the chat stream.

5. UX — usability & accessibility

UX-1 · Incomplete WAI-ARIA Tabs pattern

Evidence. taskpane.html tabs have role="tablist" + role="tab" + aria-selected, but the four <section class="panel"> have no role="tabpanel", no id↔aria-controls linkage, no aria-labelledby. taskpane.ts selectTab toggles .active + aria-selected on click only — there is no roving tabindex and no ArrowLeft/ArrowRight/Home/End key handling. Impact. Screen-reader users get no “tab N of 4 / tabpanel” semantics; keyboard users can’t arrow between tabs (the expected tablist interaction). Fix. (a) Add role="tabpanel", id, aria-labelledby="{tabId}", and tabindex="0" to each panel; give each tab aria-controls="{panelId}". (b) Implement roving tabindex: active tab tabindex="0", others -1; handle ArrowLeft/Right/Home/End to move selection + focus; keep click behavior. (c) On tab activation move focus to the tab (not the panel). Follow the APG Tabs pattern. Acceptance. Keyboard: arrows cycle tabs and switch panels; SR announces “tab, selected, N of 4” and the panel name; axe/Accessibility Insights reports no tablist violations.

Evidence. Every group is a Menu. Typhoon.Group.Fill → Typhoon.Menu.Fill → one item (Ty.FillKeyInput); Typhoon.Group.Layout → one item (Ty.LayoutMonogram). High-frequency AutoColor and Accounting sit one click deep inside dropdowns. Impact. A dropdown that opens to reveal a single command is a wasted click; the fastest, most-used actions (AutoColor, Accounting bracket) should be one click, not two. (Macabacus itself surfaces top actions as direct buttons.) Fix. Convert single-item menus (Fill, Layout) to top-level Button controls (xsi:type="Button" + ExecuteFunction). Promote AutoColor and Accounting to direct buttons in their groups (keep the fuller menu for the rest). No handler changes — the FunctionName↔commandId table in ribbon.ts already covers them. Acceptance. AutoColor and Accounting are one click from the Typhoon tab; Fill/Layout are direct buttons; all still route through the bridge and journal.

UX-3 · Chat stream thrashes the live region

Evidence. chat.streamReveal rewrites the same node’s textContent every STREAM_INTERVAL_MS (16ms) inside #chat-log[role="log"][aria-live="polite"]. (The prefers-reduced-motion path already sets the text once — good.) Impact. Rapidly rewriting the same node inside a polite live region makes screen readers either spam or drop announcements; the final message may never be cleanly announced. Fix. Do the visual reveal on an aria-hidden="true" element (or set the streaming bubble aria-hidden during reveal) and announce the final text once — e.g. write the completed message into the live region on onDone, or keep the bubble out of the live region and use a dedicated visually-persistent (not thrashed) node. Preserve the reduced-motion instant path. Acceptance. SR announces each assistant reply once, in full, after streaming; no per-chunk chatter; reduced-motion still instant.

UX-4 · Focus is lost after Accept/Reject

Evidence. chat.decide disables both buttons after a decision; focus was on one of them → focus falls to <body>. Impact. Keyboard/SR users lose their place in the log after acting on a proposal. Fix. After a decision, move focus to the appended status line (give it tabindex="-1" + .focus()) or to the next interactive element/card. Acceptance. After Enter/Space on Accept, focus lands on the result status (announced), not <body>.

Evidence. palette.ts: Enter always runs matches[0]; Escape clears. Results are focusable buttons but there’s no ArrowUp/Down roving from the input and no role="combobox" / aria-expanded / aria-activedescendant. Impact. Functional (Tab to a result + Enter works) but not the expected combobox interaction; SR gives no “N results” context. Fix. Add ArrowDown/Up to move an active-descendant highlight through #palette-results, Enter to run the highlighted (fallback matches[0]), and combobox ARIA (role="combobox" on input, aria-expanded, aria-activedescendant, role="option" on items). Announce result count via the existing #palette-status[aria-live]. Acceptance. Arrow keys move the highlight; Enter runs the highlighted command; SR announces result count and active option.

UX-6 · Borderline small-text contrast

Evidence. --muted/--gray #666 on --surface-2 #f3f3f3 at 10–11px (footer, .field-label, chips, #bridge-status[unknown]). #666 on #f3f3f3 ≈ 4.7:1 — passes AA for normal text but is borderline at 10px, and small text is where it matters most. (Dark theme #a5a5a5 on #252526 ≈ 5.3:1 — fine.) Fix. Bump light-theme muted to #595959 (≈ 5.7:1) and/or raise the 10px sizes to ≥ 11px. Leave dark theme as-is. Acceptance. All body/muted text ≥ 4.5:1; no text below 11px; axe contrast checks pass in both themes.

UX-7 · Reconcile the proposed visual redesign (context, not a defect)

Evidence. Typhoon Add-in Redesign.dc.html proposes a refreshed visual system for the pane. Guidance for the implementer. Treat the redesign as the visual target, but it is subordinate to everything above: adopt its type/spacing/color system only if it preserves (a) the bridge-status banner + reconnect, (b) accept/reject proposal cards with hover-preview, (c) the History audit list + revert, (d) the disclaimer footer, and (e) the a11y wiring in UX-1..6. Do not ship the redesign in a way that drops a governance affordance. If the redesign and an a11y fix conflict, a11y wins. Acceptance. Any visual refresh keeps all §1 guardrails and all UX-1..6 acceptance criteria.

6. Sequenced work plan & test checklist

Phase 1 — Unblock the build (ship-critical).

CA-1 icons (decide A vs B first; verify no Python icon generator).
TECH-1 AutoColor cap. TECH-2 accept error surfacing.

Phase 2 — Convention fidelity.

FMG-1 shared golden-vector parity test (add CI wiring). FMG-2 FMG confirms palette (may add a server bucket).
FMG-3 per-cell decimals. FMG-4 per-cell/guarded font-size.

Phase 3 — Accessibility & ribbon UX.

UX-1 Tabs pattern. UX-3 live-region. UX-4 focus. UX-2 ribbon buttons. UX-6 contrast. UX-5 palette combobox.

Phase 4 — Hygiene & framing.

TECH-3 extract shared util. CA-2/CA-3 doc/framing. CA-4 no-fill revert. UX-7 reconcile redesign.

Regression checklist (run after each phase):

Out of scope (flag for humans): the Claude API key / bridge-token handoff model (CA-2 stronger nonce), any server-side streaming ingress (client-side reveal stays until then), and the Python classifier’s authoritative palette (FMG-2 is an FMG decision, not an implementation choice).