Repository navigation
Conversation
Add src/lib/sheetMakeover.ts exporting roleToNumberFormat and buildMakeoverPlan, which emits only format-only/layout steps from the AUTO_APPLY_LOW_RISK_TOOLS allowlist (header styling, per-column numberFormat with the spec section 3 already-styled guard, auto_fit, and one optional negative-amount emphasis). An in-function assert enforces value-safety. Add an execute_macro branch to buildActionPreview that flattens format_cells steps into format-only CellChange[] (newValue null, newFormula undefined). Covered by new vitest suites.
Add parseProfessionalizePhrase and route general 'make it professional' phrases to a single execute_macro built from buildMakeoverPlan, placed before the narrow format_as_table/style_recipe recipes so specific phrases still defer. Surface the totals row as a separate opt-in data-writing follow-up; the macro itself stays format-only. Veto question-framed requests via execute_macro in DESTRUCTIVE_TOOLS, and defer the local actTemplates fallback (no live profile) to the pipeline path.
…otals test Add the spec section 4 professionalize_sheet capability (kind recipe, tool execute_macro) to shared/capabilities.js for catalog discoverability; it is resolved by the Tier-1 regex parser, not the semantic router, which is left unchanged. Extend applyPosture.test.ts to prove style_recipe and execute_macro stay preview-only and off the auto-apply allowlist.
…ess execute_macro
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
Reviewer's GuideIntroduces a preview-first, value-safe “professionalize my sheet” workflow that builds whole-sheet formatting-only macros from live sheet profiles, preserves existing command routing, keeps totals explicitly opt-in, and verifies zero cell-data writes across unit and integration coverage. Sequence diagram for the value-safe sheet makeover previewsequenceDiagram
actor User
participant AgentParser as AgentParser
participant Parser as parseMessage
participant Makeover as buildMakeoverPlan
participant Preview as handleMacroPreview
participant Response as buildActionPreview
participant Store as executeMacroAction
User->>AgentParser: make this look professional
AgentParser->>Parser: parseMessage(message, sheetContext)
Parser->>Makeover: buildMakeoverPlan(sheet, profile, getComputedValue)
Makeover-->>Parser: format-only steps
Parser-->>AgentParser: execute_macro(steps)
AgentParser->>Preview: handleMacroPreview(call, explanation)
Preview->>Response: buildActionPreview(execute_macro, steps)
Response-->>User: format-only preview with Apply or Reject
alt User chooses Apply
User->>Store: executeMacroAction(steps)
Store-->>User: apply one undo group
else User chooses Reject
User-->>AgentParser: reject preview
end
Flow diagram for professionalize request routing and safety boundariesflowchart TD
A[Professionalize request] --> B{parseProfessionalizePhrase}
B -->|General whole-sheet phrase| C{Live sheet, profile, and value accessor?}
C -->|No| D[Defer to LLM]
C -->|Yes| E[buildMakeoverPlan]
E --> F[execute_macro with format-only steps]
F --> G[Preview first]
G --> H{User confirms?}
H -->|Apply| I[Formatting changes only]
H -->|Reject| J[No changes]
B -->|Narrow recipe phrase| K[Existing format_as_table or style_recipe routing]
K --> L[Totals remain separate and preview-only]
Flow diagram for role-based number formattingflowchart LR
A[ColumnRole plus detectedPurpose] --> B{roleToNumberFormat}
B -->|date| C[date]
B -->|percentage| D[percent]
B -->|quantity| E[number-int]
B -->|amount and budget/invoice/inventory/sales| F[currency]
B -->|amount and generic| G[number]
B -->|text roles| H[No format step]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 12 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/lib/previewBuilders.ts" line_range="166" />
<code_context>
+export function previewExecuteMacro(sheet: SheetData, steps: ActionStep[]): CellChange[] {
+ const changes: CellChange[] = []
+ for (const step of steps) {
+ if (step.tool !== 'format_cells') continue // layout steps have no cell changes
+ const range = step.params?.range
+ if (typeof range !== 'string' || !range.trim()) continue
</code_context>
<issue_to_address>
**Changes apply without a preview**
When an `execute_macro` contains non-formatting steps or only layout steps, `previewExecuteMacro` skips non-formatting steps, while `executeMacroAction` executes them. Layout-only plans also produce no preview, yet `handleMacroPreview` still returns an applicable action. The user can therefore apply changes that the preview does not show, or apply a layout change with no preview.
Make the preview cover every executable step, or prevent the action from being applied when its effects cannot be previewed.
Also at `src/ai/pipeline/stages/agentParser.ts:236-237`.
</issue_to_address>
### Comment 2
<location path="src/ai/pipeline/stages/agentParser.ts" line_range="113-115" />
<code_context>
+ // from buildActionPreview. It never auto-applies and is never executed
+ // inline here; the store's executeMacroAction runs it as one undo group on
+ // Apply.
+ if (parsed.calls.length === 1 && parsed.calls[0].tool === 'execute_macro') {
+ return handleMacroPreview(parsed.calls[0], parsed.explanation)
+ }
+
// ─── Ambiguity clarification (understood but no calls) ──────────────
</code_context>
<issue_to_address>
**Concurrent edits are lost**
When the workbook is edited while a macro runs and a later step fails or the user undoes a successful run, `executeMacroAction` snapshots the whole workbook, so failure restores the old snapshot and discards the concurrent edit; a successful run can include that edit in its undo diff, so undoing the macro also erases it.
Prevent concurrent edits from being included in the macro snapshot, rollback, or undo diff.
</issue_to_address>
### Comment 3
<location path="src/ai/pipeline/stages/agentParser.ts" line_range="113-115" />
<code_context>
+ // from buildActionPreview. It never auto-applies and is never executed
+ // inline here; the store's executeMacroAction runs it as one undo group on
+ // Apply.
+ if (parsed.calls.length === 1 && parsed.calls[0].tool === 'execute_macro') {
+ return handleMacroPreview(parsed.calls[0], parsed.explanation)
+ }
+
// ─── Ambiguity clarification (understood but no calls) ──────────────
</code_context>
<issue_to_address>
**Mid-run sheet switch redirects formatting**
When the user switches the active sheet between awaited makeover steps, `executeMacroAction` creates a step executor that rebuilds its execution context before every step, and the formatting setters target the currently active sheet. If the user switches sheets while the multi-step makeover is running, later steps apply the original sheet’s ranges to the newly active sheet, so the wrong sheet is formatted.
Capture the target sheet ID when Apply begins and make every macro step operate on that sheet, or prevent sheet switching until the macro completes.
</issue_to_address>
### Comment 4
<location path="src/lib/previewBuilders.ts" line_range="175" />
<code_context>
+ const current = sheet.cells[cellId]
+ changes.push({
+ cell: cellId,
+ oldValue: current?.value ?? null,
+ newValue: null, // format-only: value is untouched
+ oldFormula: current?.formula,
</code_context>
<issue_to_address>
**Format preview looks like data deletion**
When a pending makeover formats populated cells, `previewExecuteMacro` sets `newValue` to `null` without representing the requested format change. `GridCell` renders that pending value as empty, and the Apply/Reject diff shows existing values as blank, making a format-only change look like data deletion.
Represent the formatting change in the preview without replacing the existing value or formula.
Also at `src/lib/previewBuilders.ts:176-179`.
</issue_to_address>
### Comment 5
<location path="src/lib/sheetMakeover.ts" line_range="93" />
<code_context>
+ }
+
+ const { headerRow, startCol, endCol, endRow } = range
+ const purpose = profile.detectedPurpose
+ const theme = themeForPurpose(purpose)
+ const dataStartRow = headerRow + 1
</code_context>
<issue_to_address>
**Formats use the wrong table columns**
When the detected table header is below row 0, `buildMakeoverPlan` uses `detectTableRange` for the header row but takes `profile.columns` and `profile.detectedPurpose` from `buildSheetProfile`, which assumes headers are on row 0. The resulting roles and purpose do not describe the detected table, so its data columns get missing or incorrect number formats.
Infer the column roles and purpose from the detected header row and use them for the makeover's format decisions.
Also at `src/lib/sheetMakeover.ts:94`, `src/lib/sheetMakeover.ts:126-132`.
</issue_to_address>
### Comment 6
<location path="src/lib/sheetMakeover.ts" line_range="157-161" />
<code_context>
+
+ if (!headerFullyStyled) {
+ const headerRange = `${colToLetter(startCol)}${headerRow + 1}:${colToLetter(endCol)}${headerRow + 1}`
+ steps.push({
+ tool: 'format_cells',
+ params: {
+ range: headerRange,
+ bold: true,
+ bgColor: theme.headerBg,
+ fontColor: theme.headerFontColor,
+ borders: { bottom: `2px solid ${theme.accentColor}` },
+ },
+ description: `Style the header row (${headerRange}) bold with a ${theme.name.toLowerCase()} fill`,
+ })
+ }
+
</code_context>
<issue_to_address>
**No-op steps roll back formatting**
When no rows need auto-fitting or an amount column has no negative values, `buildMakeoverPlan` still adds the `auto_fit` or negative-emphasis step. `handleAutoFit` and `applyFormatCells` report these no-ops as failures, and `executeMacro` rolls back the earlier formatting, so the makeover fails without leaving its styling in place.
Omit inapplicable steps or treat these expected no-op results as successful.
Also at `src/lib/sheetMakeover.ts:165-180`.
</issue_to_address>
### Comment 7
<location path="src/lib/previewBuilders.ts" line_range="171-173" />
<code_context>
+ if (typeof range !== 'string' || !range.trim()) continue
+ const cellIds = expandRange(range, sheet)
+ if (!cellIds) continue
+ for (const cellId of cellIds) {
+ const current = sheet.cells[cellId]
+ changes.push({
+ cell: cellId,
+ oldValue: current?.value ?? null,
</code_context>
<issue_to_address>
**Preview highlights the wrong cells**
When the negative-emphasis range contains positive or zero amounts, `previewExecuteMacro` expands the whole conditional-format range without evaluating its condition, so the preview marks non-negative cells as highlighted even though Apply formats only matching negative cells.
Evaluate the conditional-format condition when building the preview and include only cells that match.
</issue_to_address>
### Comment 8
<location path="src/lib/previewBuilders.ts" line_range="169-173" />
<code_context>
+ if (step.tool !== 'format_cells') continue // layout steps have no cell changes
+ const range = step.params?.range
+ if (typeof range !== 'string' || !range.trim()) continue
+ const cellIds = expandRange(range, sheet)
+ if (!cellIds) continue
+ for (const cellId of cellIds) {
+ const current = sheet.cells[cellId]
+ changes.push({
+ cell: cellId,
+ oldValue: current?.value ?? null,
</code_context>
<issue_to_address>
**Large ranges stall the preview**
When a makeover formatting range covers a very large or sparse sheet extent, `previewExecuteMacro` expands the range and allocates a `CellChange` for every cell, consuming excessive memory and slowing or stalling the response UI; overlapping ranges also duplicate preview entries.
Bound or compact the preview changes and deduplicate cells across overlapping ranges.
</issue_to_address>
### Comment 9
<location path="src/ai/pipeline/stages/agentParser.ts" line_range="107" />
<code_context>
+ // ─── execute_macro preview/confirm flow ─────────────────────────────
+ // execute_macro (the value-safe sheet makeover) is intentionally NOT in
+ // toolRegistry, so executeToolCalls would see category=undefined, treat it
+ // as a read, and run it inline — where executeTool returns
+ // "Unknown tool: execute_macro". It must instead be returned as an
+ // Apply/Reject action so responseBuilder attaches the format-only preview
</code_context>
<issue_to_address>
**Makeover gets a destructive warning**
When a professionalize request produces an `execute_macro` action, the parser checks `DESTRUCTIVE_TOOLS` before its macro-preview branch, so `handleDestructivePreview` handles the format-only makeover and tells the user it will remove all sheet data.
Keep `execute_macro` out of `DESTRUCTIVE_TOOLS` or route it to the macro-preview branch before destructive-tool handling.
Also at `src/agent/parser.ts:82`.
</issue_to_address>
### Comment 10
<location path="src/lib/sheetMakeover.ts" line_range="63-66" />
<code_context>
+}
+
+/** Does the header-row cell already carry every key the makeover would set? */
+function headerAlreadyStyled(fmt: CellFormat | undefined, theme: (typeof TABLE_THEMES)[string]): boolean {
+ if (!fmt) return false
+ return Boolean(fmt.bold) && fmt.bgColor === theme.headerBg && Boolean(fmt.borders?.bottom)
+}
+
+/**
</code_context>
<issue_to_address>
**Partly styled headers are left unchanged**
When header cells match the target bold and fill but have a different font color or bottom-border style, `headerAlreadyStyled` accepts any bottom border and does not check the requested font color or border styling. `buildMakeoverPlan` therefore skips the header-format step and leaves the headers inconsistently styled.
Have `headerAlreadyStyled` compare every header-format property the makeover sets.
</issue_to_address>
### Comment 11
<location path="shared/spreadsheetPhrases.ts" line_range="338" />
<code_context>
+ }
+
+ // "professionalize (this|my sheet)"
+ if (/\bprofessionali[sz]e\b/.test(lower)) return true
+
+ // "clean up the formatting" / "clean up this sheet"
</code_context>
<issue_to_address>
**Negated requests trigger makeover**
When the message contains “professionalize” in a negated instruction, the bare `professionali[sz]e` match accepts negated or descriptive text such as “don’t professionalize this,” so `parseMessage` builds an `execute_macro` action and presents an Apply/Reject makeover for a request that explicitly says not to apply it.
Reject negated/non-command phrasing before constructing the makeover action.
</issue_to_address>
### Comment 12
<location path="src/lib/sheetMakeover.test.ts" line_range="9" />
<code_context>
+import { buildSheetProfile } from '@/ai/sheetProfile'
+
+// Tools that write cell values/formulas — the makeover macro must emit none.
+const DATA_WRITING_TOOLS = ['set_cell', 'set_range', 'apply_formula', 'style_recipe', 'clear_sheet', 'modify_column']
+
+function makeSheet(cells: Record<string, CellData>): SheetData {
</code_context>
<issue_to_address>
**Header rewrites pass value-safety checks**
When a plan emits `rename_header`, a makeover step using `rename_header` passes the allowlist assertion and is absent from `DATA_WRITING_TOOLS`, so these tests accept a plan that changes a header value. `handleRenameHeader` calls `setCellValue`, making the claimed zero-data-write guarantee untested for this tool.
Add `rename_header` to the data-writing denylist and assert it is never emitted.
Also at `src/agent/parser.makeover.test.ts:17`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 12 findings to address first, and if the makeover chooses inappropriate number formats or styling, applying it changes the sheet’s saved formatting; reverting the code would not restore sheets already changed. The damage is limited to formatting and can be repaired, and the preview/Apply flow keeps the plan from changing cell values or formulas.
Blocking findings: src/lib/previewBuilders.ts:166, src/ai/pipeline/stages/agentParser.ts:115, src/ai/pipeline/stages/agentParser.ts:115, src/lib/previewBuilders.ts:175, src/lib/sheetMakeover.ts:93, and 7 more
| export function previewExecuteMacro(sheet: SheetData, steps: ActionStep[]): CellChange[] { | ||
| const changes: CellChange[] = [] | ||
| for (const step of steps) { | ||
| if (step.tool !== 'format_cells') continue // layout steps have no cell changes |
There was a problem hiding this comment.
🟠 High · Changes apply without a preview
When an execute_macro contains non-formatting steps or only layout steps, previewExecuteMacro skips non-formatting steps, while executeMacroAction executes them. Layout-only plans also produce no preview, yet handleMacroPreview still returns an applicable action. The user can therefore apply changes that the preview does not show, or apply a layout change with no preview.
Make the preview cover every executable step, or prevent the action from being applied when its effects cannot be previewed.
Also at src/ai/pipeline/stages/agentParser.ts:236-237.
Prompt for AI agents
In `src/lib/previewBuilders.ts` at line 166:
**Changes apply without a preview**
When an `execute_macro` contains non-formatting steps or only layout steps, `previewExecuteMacro` skips non-formatting steps, while `executeMacroAction` executes them. Layout-only plans also produce no preview, yet `handleMacroPreview` still returns an applicable action. The user can therefore apply changes that the preview does not show, or apply a layout change with no preview.
Make the preview cover every executable step, or prevent the action from being applied when its effects cannot be previewed.
Also at `src/ai/pipeline/stages/agentParser.ts:236-237`.previewExecuteMacro now preserves each cell value+formula instead of emitting newValue:null, so a format-only makeover no longer looks like data deletion in the Apply/Reject diff (#4). Layout steps (auto_fit/set_column_width/set_row_height) emit a value-preserving anchor change so a layout-only plan still produces a non-empty preview and cannot apply unseen (#1). Cells are deduped across overlapping ranges and capped at MAX_PREVIEW_CELLS=200 with a single preserving summary row (#6). Updated the existing no-data-write tests to the preserve contract and added previewBuilders.makeover.test.ts.
Capture the active sheet id when a makeover Apply begins and thread it through buildExecutionContext so every formatting/layout step targets that sheet even if the user switches tabs mid-run (#3). Scope the macro's undo diff and failure rollback to the target sheet so a concurrent edit on another sheet is neither captured in the macro's single undo entry nor erased on rollback (#2). Preserves one-macro=one-undo-entry, the PR#57 undo-id toast guard, and execute_macro staying off the tool registry.
#8) Export inferRole+detectPurpose from sheetProfile; buildMakeoverPlan re-infers column roles and sheet purpose from detectTableRange's header row instead of the row-0 profile. headerAlreadyStyled now compares every property the header step sets (bold, bgColor, fontColor, exact 2px bottom border). Value-safety assert unchanged.
The FEAT-004 import in shared/spreadsheetPhrases.ts omitted the .js extension, which the client bundler tolerates but the server tsc (moduleResolution nodenext) rejects with TS2835. All other non-test relative imports in shared/ already use .js; this aligns with them.
This branch has not been deployed
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
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.
Summary
Adds a value-safe "professionalize my sheet" makeover: a chat request like "make this look professional" / "clean up the formatting" / "format this like a pro" produces a whole-sheet styling pass that writes zero cell data.
Value-safe split
execute_macropass. The plan builder emits ONLY allowlist tools (format_cells,set_column_width,set_row_height,auto_fit,rename_header— theAUTO_APPLY_LOW_RISK_TOOLSset). It emits NOset_cell/set_range/style_recipe/apply_formulastep, so every emitted step is format-only (preview changes carrynewValue: null/newFormula: undefined).execute_macrois preview-first. It is intentionally not intoolRegistry, so the agent-parser stage special-cases it into an Apply/Reject preview (handleMacroPreview) instead of running it inline.shouldAutoApply('execute_macro', 'auto')isfalse, so the whole-sheet makeover always previews — it is never auto-applied.style_recipe: total_row, which writes a Total label + SUM formulas) is not part of the makeover macro. It is surfaced only as a distinct follow-up suggestion in the response message and is triggered by its own phrase ("add a totals row"). It stays off the auto-apply allowlist, remains inhighImpactTools, and keeps its persistence caveat — no makeover code path can reach it automatically.Role → number-format mapping
A new pure function
roleToNumberFormat(role, purpose)over existinginferRole/detectPurposeoutputs (no inference changes):datedatepercentagepercentquantitynumber-intamountbudget/invoice/inventory/salescurrencyamountgenericnumberid/category/label/description/unknownAn already-styled-sheet guard (spec §3) skips emitting a
format_cellsstep when a cell already carries the relevant format, keeping the makeover "match and extend" and re-runs near-idempotent.Testing
npm run typecheck— ✅ clean (tsc --noEmit)npm run lint— ✅ clean (eslint .)npm run test(vitest) — ✅ 174 files, 2252 tests passedNew/extended tests lock the value-safe invariants: every emitted makeover step is on the low-risk allowlist (no
set_cell/set_range/apply_formula/style_recipe), theexecute_macropreview yields zero changes with a non-null value/formula, and bothexecute_macroandstyle_recipeareshouldAutoApply(..., 'auto') === false.Doc/code discrepancies recorded in plan.md
toolRegistry/AUTO_APPLY_LOW_RISK_TOOLS. Freeze is skipped as out of value-safe allowlist scope and noted as deferred.kind: 'recipe'. Theprofessionalize_sheetcatalog entry (spec §4) is added for discoverability, butsemanticCapabilityRouter.tsstill returnsnullforkind: 'recipe'. The makeover resolves via the Tier-1 regex (which runs before Tier-2), so a high-scoring paraphrase defers to Tier-1/LLM rather than emitting a steps-lessexecute_macro. The router was deliberately left unexpanded (mature-codebase stability); the embedded-but-unrouted entry is a subtle invariant to preserve if the router is ever expanded.Summary by Sourcery
Add a preview-first, value-safe professionalization workflow that formats an entire sheet without changing its data.
New Features:
Bug Fixes:
Enhancements:
Tests:
Review-fix pass: value-safe "professionalize my sheet" makeover
Addresses the 11 review findings against the value-safe sheet-makeover feature on
feat/sheet-makeover. Each finding was verified against current code; valid issues were fixed and test-only findings were hardened. The value-safe contract is preserved throughout: the default makeover writes zero cell data, totals stay opt-in / preview-only,execute_macronever auto-applies, and one macro = one undo entry using the PR#57 undo-id toast-guard pattern.Fixes per cluster
Preview correctness — findings #1 / #4 / #6 (
src/lib/previewBuilders.ts)previewExecuteMacrono longer emitsnewValue: null; it preserves each cell's existing value and formula so a format-only makeover renders as a format change, not a blanking of data, in the Apply/Reject diff.auto_fit/set_column_width/set_row_height) now emit a value-preserving anchor change, so a layout-only plan still produces a non-empty preview and cannot be applied without being shown.MAX_PREVIEW_CELLS = 200with a single value-preserving summary row, bounding memory and UI cost.Macro execution scope — findings #2 / #3 (
src/store/aiExecution.ts)buildExecutionContext, so every formatting/layout step targets that sheet even if the user switches tabs mid-run.Plan building — findings #5 / #8 (
src/lib/sheetMakeover.ts,src/ai/sheetProfile.ts)buildMakeoverPlannow re-infers column roles and sheet purpose fromdetectTableRange's detected header row (via exportedinferRole+detectPurpose) instead of assuming headers on row 0, so detected tables get correct number formats.headerAlreadyStylednow compares every property the header step sets (bold, background color, font color, and the exact 2px bottom border) so inconsistently styled headers are correctly re-formatted.Phrase gating — finding #9 (
shared/spreadsheetPhrases.ts)execute_macromakeover action is constructed, so Apply/Reject is no longer presented for a request that says not to apply it.Trigger ordering & invariants — findings #7 / #10 / #11 (test hardening)
execute_macrois routed to the macro-preview branch before destructive-tool handling and stays offDESTRUCTIVE_TOOLS; pinned withsrc/agent/parser.makeover.test.ts.rename_headerand thatrename_headeris treated as a data-writing tool, so the zero-data-write guarantee is enforced by tests.Preserved value-safe invariants
execute_macronever auto-applies and stays off the tool registry / out ofDESTRUCTIVE_TOOLS.Verification
npm run typecheck— greennpm run lint— greennpm run test— 176 files / 2267 tests passingCommits FEAT-001..FEAT-005:
f6abcff,e3c1180,cc1c38a,9d5b0b0,eeb4911.