Register format_sheet and set_conditional_format, and teach the spreadsheets skill - #2574
Conversation
The formatting tools existed for a whole epic and nothing could reach them:
no TOOL_MODULES entry, no WRITE_TOOLS gating, no label, and a spreadsheets
skill that still told the model edit_sheet_cells was the only write path.
Registration and gating:
- TOOL_MODULES.sheetsFormat (TOOL_REGISTRY / WORKSPACE_TOOL_NAMES / the
doc-enforced count follow from it; README + getting-started go 81 -> 83).
- WRITE_TOOLS carries both names, so a read-only agent loses them and
cap-step-tool-payloads keeps their results intact.
- Curated labels, and a per-tool-gated SHEET bullet in inline-instructions
so an agent is never told about a tool it does not hold.
- SHEET_WRITE_TOOL_NAMES deliberately unchanged, with a comment: it answers
"can sandbox OUTPUT go into a Sheet", and a formatting tool cannot put
data anywhere.
- The spreadsheets command description now names formatting, dashboards and
"presentable", and format_sheet joins its .some()-gated requiredTools.
Skill body (19,795 / 20,000 chars): regions first, ops as the escape hatch,
a one-call budget-table example, the four lines the schema cannot convey
(ops do not cover rows added later, a region costs nothing per row, the
precedence chain, rule values compare the unformatted value), a short
set_conditional_format section, pitfall 7 reworded as the doorway to display
formatting, and two new pitfalls. Trimmed the function-table notes and the
longest read_sheet bullets to pay for it.
Renderer: SheetFormatRenderer (regions, ops, rules, removed ids, one swatch
per distinct colour) registered for both tools. Not SheetEditRenderer, which
is an address -> value table and formatting has no values.
Tests probe search mode through tool_search('select:...') and execute_tool
dispatch rather than Object.keys, which passes vacuously for non-core tools.
Every claim was mutation-checked by line (13 probes, all red).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5259d85db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…format too Two Codex review findings on #2574. The set_conditional_format card passed every requested rule through as a change, but an append that overlaps the tab lands only the new rules and names the rest by index in `skippedDuplicates`. Those rows now carry an "already present" badge, are excluded from the added count, and the summary says how many were skipped. The spreadsheets skill's requiredTools is .some()-gated, so an agent whose allowlist held set_conditional_format but not read_sheet / edit_sheet_cells / format_sheet could run rules it could not discover the semantics for. Both formatting tools now gate the skill; the registration test pins both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06932d28f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…x two skill facts
Review pass (Codex re-review, local code review, and a simplify sweep).
Renderer
- The execute_tool wrapper returns `{ error }` with no `success` key and the
model's UNVALIDATED parameters as the input; both registry entries now fall
through on it too, and every deref in the card tolerates a missing field, so
a malformed call can never take the message list down.
- Cell rules are described through `describeCondition`, the same helper the
sheet's rule panel uses, so the card omits the operands the executor drops
(none for isEmpty/isNotEmpty/isError, no value2 outside between/notBetween)
and reads "is greater than 1000" like the sheet does.
- The region swatch is the header colour region-format actually paints
(`regionTheme(...).header.background`, slate fallback included), not a hue
strength the sheet never uses.
- Input types come from the tool's own zod schemas (type-only import); the
hand copy had already drifted. Dropped props nothing passed, the useMemos
guarding trivial work, and the twice-listed colour fields.
Skill body (19,784 / 20,000)
- `dataBar` `color` is required: buildRule refuses the whole call without it.
- Restored the never-computed-formula exception on `unformatted` reads that
the rewrite had dropped; MIN/MAX named beside SUM as text-intolerant.
- Trimmed sentences that restated a pinned line to pay for both.
Tests
- Every builtin skill body is now checked for tool names that do not exist
(skill-bodies.test), and every `name: tool(` in a *-tools.ts module must be
a key of the full registry (tool-registry-docs.test) — the class of bug this
epic shipped with. The collision case derives its keysets from
TOOL_REGISTRY instead of a hand list that had drifted.
- The search-mode read-only probe now runs over every registered write tool.
- Gated SHEET line and the destination-phrase non-change moved beside their
owning suites; the fragment no longer names edit_sheet_cells unconditionally.
Also: CHANGELOG entry, CATEGORY_MAP entries, corrected WRITE_TOOLS comment,
and the tool module's stale "not registered anywhere yet" header.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78001f2db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`lands both rules when two calls add different ones concurrently` added two rules that differed only by id. Since d31b789 the store refuses a rule whose content matches one already on the tab, so the second call now throws SheetDuplicateRuleError and the case fails — on the base branch too, whose last green run (7ced967) predates that commit. The case is about the tab lock, not the dedup: the second rule now covers a different range, and both land. Verified against a migrated local test DB: 88/88 in the file, and the case goes red again with the range removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
…l removed Codex round 3. The skill (and the tool description) say `regionMode: "replaceAll"` keeps only the regions in the call, but the nothing-given guard fired before the second guard that already meant to admit an empty replaceAll, so "keep none" was unreachable. An empty replaceAll now plans `setRegions([])`, the way the conditional tool's replaceAll clears every rule; a bare call with no regions, no ops and no mode is still refused. The result carries `regionMode` and the message names the removal. The card previously said "1 region" for a replaceAll that deleted every other region on the tab. It now adds "other regions removed" to the summary and a row naming the removal — and an empty replaceAll is not an empty card. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d62b14257a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round 4, now that format_sheet is reachable. A region declared without an id got a freshly minted one on every call, so retrying the same call after a timed-out response landed a content-identical twin each time. A new region that matches one already on the tab (everything but the id, via the new `regionContentKey` beside the rule's) now reuses that region's id, so the retry is a no-op upsert and reports the id the first call minted. `applyPlanned` logged an activity entry, fired the workflow trigger and broadcast content-updated even when the store reported no rows touched and no tab field changed — which it does for exactly the no-op and retry cases, and bumps no revision for. Those side effects are now gated on the store's own change indicators. The card listed a repeated `removeRuleIds` entry twice (with duplicate React keys) although the executor removes it once; ids are deduplicated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 838bdb9d52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…at calls Codex round 5. Two overlapping executions of a retried format_sheet call are both planned before either commits, so neither snapshot shows the other's region and the snapshot twin-lookup alone still minted two ids. A region declared without an id now gets one derived from its content (sha256 of `regionContentKey`), so both executions send the same id and the store's upsert-by-id under its lock absorbs the second. Random only when that id is already taken by something else. The snapshot lookup stays for regions that already exist under a UI-minted id. A call the store reports as a no-op (nothing written, no revision bumped) still answered with the requested counts, and the card listed "1 op" as though it had landed. The result now carries `changed`, the message says the sheet already had this formatting, and the card shows "already formatted this way" instead of the rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2490fb1ce3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…diff
Codex round 6.
Declaring the same no-id region twice in one call gave the first the
content-derived id and the second a random fallback — an overlapping twin.
It is now refused by name ("regions[1] declares the same region as
regions[0]").
set_conditional_format's replaceAll cleared every rule and re-added the call's
rules under fresh random ids, so an identical retry rewrote the tab, bumped
the revision, fired activity and workflows, and invalidated the ids the first
attempt had returned. It is now a diff: rules on the tab that match one in
the call keep their ids and are reported as already present, only the rest
are removed, and new rules get content-derived ids. A retried replaceAll
plans nothing and reaches the store zero times.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b69671de99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…er ids are given Codex round 7. The conditional replaceAll diff kept existing rules in the tab's order, so [B, A] over a tab holding [A, B] planned nothing although later rules win; and its removals came from the read snapshot, so a rule another writer added before the lock survived a "keep only these". New store op `setConditionalRules`: the whole list, in the caller's order, validated (ids and content unique within the list) and replanned under the tab lock, like `setRegions`. The store already compares the list it stores as JSON, so an identical retry is not a write, bumps no revision and fires nothing — the integration suite pins that end to end — while a reorder or a concurrently added rule is reconciled. The tool's replaceAll builds that list with reused or content-derived ids and refuses a rule sent twice. Regions: the same-call content check now covers every declaration, not only those without an id, so explicit-id + no-id (either order) and two explicit ids over identical content are refused instead of landing a twin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41006e2131
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l as done Codex round 8. With content-derived ids, an overlapping retry of an append sends the SAME id as the attempt that committed first; the store checked the id collision before the content twin, so the retry surfaced as an ordinary refusal instead of the SheetDuplicateRuleError the tool recovers from. The twin check now runs first; same id with different content is still the id collision. A retried append whose removeRuleIds already landed read a tab where the id was gone and was refused before anything else was considered. An absent id is now treated as already removed, with a warning that still lists the ids that exist, so the replay succeeds and reports the state it finds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25ddaf58bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…a retry explains Codex round 9. The store's applyFormatOps result now carries `ruleIdsAdded` and `ruleIdsRemoved`, computed under the tab lock from what the tab held then. The tool's counts, confirmed removals and `changed` flag come from that delta instead of its earlier snapshot, so a rule another writer added between read and write is reported as removed by a replaceAll, and an id the tab never held is never listed as a removal. The append path's retry recovery now re-verifies the requested final state after ANY store refusal under the lock, not only a content twin: an overlapping retry whose removal already landed is reported as the landed retry, provided every rule is present by content and every removal absent. The card takes removals from the result, labels reused replaceAll rules "kept" rather than "already present", and shows a pure reorder as "rules reordered". The formula examples the model sees use a bounded range (`C2:C40`); `C:C` does not parse here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
…rite The earlier concurrent-rule test injected the rule before the read, so the snapshot count and the locked count agreed and a mutation back to the snapshot survived. This one lands the rule inside the store call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b0baab1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed region delta The previous commit left SheetDuplicateRuleError imported but unused, which fails ESLint and therefore the Next build in every CI job. Removed, and the touched files now pass ESLint locally. Codex round 10: - An explicit region id that is not the existing twin's id, over identical content, is refused (pass that id, omit it, or change the region) instead of landing an overlapping region under a second id. - The activity entry's counts (what the workflow trigger sees) are built from the store's locked delta — rules and regions added/removed, cells, tab fields — not from the caller's pre-lock estimate, which is no longer passed. - The store also reports regionIdsAdded/regionIdsRemoved; format_sheet returns removedRegionIds and names the count in its message, and the card shows the removal note only when something was removed, with the number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 055980c421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round 11. A freeze op naming one axis (or a region's freezeHeader) had its other axis filled in by the tool from its pre-lock snapshot, so a change another writer made to that axis in between was written back and lost. `setFrozen` now takes each axis optionally: an omitted axis keeps what the tab holds at plan time — under the store's lock — and a second freeze in the same plan resolves against the first, not the tab. `SheetFormatTarget` carries the current freeze for that. The tool sends only the axis the model named; a region's freezeHeader sends rows only. Both axes missing is still refused as the no-op it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CI enforces 100% line/branch/function coverage on format-request.ts; the setConditionalRules refusals (non-array, over the limit, non-object entry) and regionContentKey (called only by the tool) had no lib test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa
1 participant
Why
Task 5 of 5 on the sheet-formatting epic.
format_sheetandset_conditional_format(#2571) existed but nothing registered them, no gate knew them, and the spreadsheets skill still saidedit_sheet_cellswas the only write path. Until this lands the whole epic is dead code.What
Registration and gating
TOOL_MODULES.sheetsFormatinai-tools.ts—TOOL_REGISTRY,WORKSPACE_TOOL_NAMESand the doc-enforced count follow from it. README and the getting-started doc go 81 → 83 (tool-registry-docs.test.tsenforces this; not weakened).WRITE_TOOLS, so a read-only agent loses them andcap-step-tool-payloadskeeps their results intact.Format Sheet,Conditional Formatting.inline-instructions.ts: a per-tool-gated SHEET line forformat_sheet, matching the existing pattern (naming a tool the agent does not hold produces an unknown-tool call).command-core.ts: thespreadsheetsdescription now names formatting, dashboards and "presentable" (the model's only retrieval signal); bothformat_sheetandset_conditional_formatjoin the.some()-gatedrequiredTools, so an agent holding only the rule tool still discovers the skill (Codex review).Two deliberate non-changes
SHEET_WRITE_TOOL_NAMESinsystem-prompt.tsstays['edit_sheet_cells'], with a comment saying why: it answers "can sandbox OUTPUT go into a Sheet", and a formatting tool cannot put data anywhere. A test pins that an agent holding only the formatting tools is not told a Sheet is a destination.cap-step-tool-payloads.tsuntouched — it is generic and keys offWRITE_TOOLS.Skill body (
spreadsheets.ts, 19,795 / 20,000 chars)opsas the escape hatch, a one-call budget-table example (range, headerRows, currency column, total row, theme), and the lines the schema cannot convey: ops do not cover rows added later / regions do; a region costs nothing per row vsA2:A5000= 4,999 cells; precedencecolumn default < region < cell < conditional rule; rule values compare the computed, unformatted value.set_conditional_formatsection: the four rule kinds,removeRuleIds, andreplaceAllrequiring aread_sheet includeFormatting: truefirst.SUMandwherestill work). "Only write path" claim removed; heading is now "Two write paths". Two new pitfalls (colouring cells instead of a rule; cell-by-cell instead of a region).dataBarcoloris required (the tool refuses without it); the never-computed-formula exception onunformattedreads is restored; MIN/MAX named beside SUM as text-intolerant.region-format.tsdecides what money and a header band look like.read_sheetbullets. No capability content was cut.Renderer
SheetFormatRenderer— regions, ops, rules, removed rule ids, and one swatch per distinct colour (#FFFand#ffffffare one swatch). Registered for both tools inregistry.tsx. NotSheetEditRenderer: that is an address → value table and formatting has no values. A refusal falls through to the generic envelope. Rules the result reports inskippedDuplicates(a retried or overlapping append) render with an "already present" badge and are excluded from the added count (Codex review). Cell rules are described through the sheet's owndescribeCondition, so the card omits the operands the executor drops and reads "is greater than 1000" like the rule panel (Codex review). The region swatch is the header colourregion-formatactually paints, slate fallback included. Both entries also fall through on the execute_tool{ error }envelope, which has nosuccesskey and arrives with the model's unvalidated input (local review). Input types are the tool's own zod-inferred types, imported type-only.Tests
sheet-format-registration.test.ts— registry shape, labels,WRITE_TOOLS(direct and viafilterToolsForReadOnly), the skill'srequiredTools/description, the gated SHEET line, and the destination-phrase non-change.tool_search('select:format_sheet')on a search-mode set built from the real registry (full agent → found, read-only → empty, withread_sheetas the live-probe control), plus anexecute_tooldispatch probe (full agent reaches the tool's auth guard; read-only gets the unknown-tool envelope).Object.keyswould have passed vacuously because neither tool is core.spreadsheets-skill-body.test.ts— no "only write path" line; regions taught before ops; the four schema-invisible lines; worked example; rule kinds, required data-bar colour, replaceAll; pitfall 7; the two new pitfalls; the never-computed-formula clause; MIN/MAX. Pins facts, not wording, so the body can still be trimmed.packages/libgainsregionContentKey(beside the rule key), thesetConditionalRulesop, andruleIdsAdded/ruleIdsRemoved/regionIdsAdded/regionIdsRemovedon theapplyFormatOpsresult (computed under the tab lock), andsetFrozenaxes that may be omitted to keep the current freeze;sheet-format-request.test.tscovers the op (order, empty list, duplicate id/content) and the store integration suite covers the identical-list no-op end to end.skill-bodies.test.ts), and everyname: tool(declared in a*-tools.tsmodule must be a key of the full registry (tool-registry-docs.test.ts, ledger:ask_user). Theai-tools.test.tscollision case now derives its keysets fromTOOL_REGISTRY; the hand list had drifted. The search-mode read-only probe runs over every registered write tool.registry-sheet-format.test.tsx— both tools render the formatting card, counts/ranges/swatch dedupe, refusal fall-through, empty-state message.Mutation probes (by line, all red, restored)
sheetsFormatline removed fromTOOL_MODULES'format_sheet'removed fromWRITE_TOOLS'set_conditional_format'removed fromWRITE_TOOLSformat_sheetlabel removedformat_sheetadded toSHEET_WRITE_TOOL_NAMESformat_sheetremoved fromrequiredTools(lib dist rebuilt)format_sheetregistry entry renamednormalizeHexdedupe removedset_conditional_formatremoved fromrequiredTools{ error }guard removed from the registry entrydescribeConditionhue.midinstead of the header coloursheetsFormatremoved fromTOOL_MODULESreplaceAllno longer exempt from the nothing-given guardregionModenot passed through the registryremoveRuleIdsdedupe removedchangedhard-coded truereplaceAllback to clear-and-rebuildsetConditionalRulescontent-duplicate refusal removedremoveRuleIdsentry refuses againC:Cexample restoredVerification
bun run --filter web test -- sheet-format→ 3 files, 54 passedbun run --filter web test -- skill→ 5 files, 50 passedbun run --filter web test -- tool→ 94 files passed; the 2 failures areactivity-tools.test.tsandagent-tool-surface.integration.test.ts, which fail loud by design without the dockerized test Postgres. Unrelated to this change.bun run --filter web typecheck→ exit 0test.ymlonly triggers on PRs against master, so the full Test Suite was dispatched by hand on this branch (gh workflow run test.yml --ref pu/sheet-tool-registry), the same way the epic base branch got its run.sheet-store.integration.test.ts › lands both rules when two calls add different ones concurrently. It is red onpu/sheet-agent-regionsitself: Add format_sheet and set_conditional_format AI tools #2571'sd31b789made the store refuse a content-identical rule, and that case added two rules differing only by id (the base branch's last green run,7ced967, predates that commit). The second rule now covers a different range. Verified against a migrated local test DB (88/88 in the file; red again with the range removed). Lint/typecheck and E2E passed on the same run.Review threads
Codex round 1 (duplicate rules shown as changes; skill eligibility missing
set_conditional_format) fixed in the second commit. Codex round 2 (data-bar colour is required; card printed operands the executor drops) and the local code-review findings (error-envelope crash, dropped skill clause, MIN/MAX, swatch colour, drifted collision test, unregistered-module guard, changelog) fixed in the third. Codex round 3 (an emptyreplaceAllwas refused by the nothing-given guard although the second guard meant to admit it; the card hid the regions areplaceAlldeleted) fixed in the fifth: an emptyreplaceAllnow clears every region like the conditional tool's does, the result carriesregionMode, and the card names the removal. Codex round 4 (new-region retries minted a twin each time; no-op calls still logged activity and broadcast; repeatedremoveRuleIdsrendered twice) fixed in the sixth: a new region content-identical to an existing one reuses its id (regionContentKeyin lib beside the rule key), side effects are gated on the store's ownrowsTouched/tabFieldsChanged, and removed ids are deduplicated. Codex round 5 (overlapping retries could still mint two ids because each snapshot misses the other's region; a no-op call's card still listed the requested ops) fixed in the seventh: a region declared without an id gets a content-derived id so concurrent executions agree on it and the store's upsert-by-id absorbs the second; the result carrieschangedand the card shows a no-op as unchanged. Codex round 6 (a same-call duplicate region declaration landed a twin; the conditionalreplaceAllre-minted every rule id on retry) fixed in the eighth: the duplicate is refused by name, andreplaceAllis a diff that keeps content-matched rules under their ids, removes only the rest, and derives new rule ids from content — a retried replaceAll reaches the store zero times. Codex round 7 (the replaceAll diff ignored rule order and could not remove a concurrently added rule; same-call duplicate regions with explicit ids slipped past) fixed in the ninth: a new store opsetConditionalRulestakes the whole list in the caller's order and reconciles it under the tab lock (identical list ⇒ not a write, pinned by a store integration test against Postgres), the tool's replaceAll sends that list, and every region declaration is content-checked. Codex round 8 (an overlapping append retry with content-derived ids hit the store's id check before its twin check and surfaced as an unrecoverable refusal; a replayed append whoseremoveRuleIdshad landed was refused) fixed in the tenth: the store compares content before id, and an absent removal id is treated as already removed with a warning. Codex round 9 (overlapping removal retries refused under the lock; replaceAll counted removals from the stale snapshot; unboundedC:Cexamples; the card listed absent ids as removed and reused replaceAll rules as skipped) fixed in the eleventh:applyFormatOpsnow returnsruleIdsAdded/ruleIdsRemovedcomputed under the lock, the tool's counts and confirmed removals come from that, the append recovery re-verifies the requested final state after any store refusal, the examples useC2:C40, and the card shows confirmed removals, "kept" rows and a reorder. Codex round 10 (an explicit id over an existing region's content landed a twin; the activity entry's counts came from the pre-lock snapshot; the card claimed region removals on every changed replaceAll) fixed in the twelfth: the twin is refused by name, activity metadata is built from the store's locked delta (rules and regions), and the store's newregionIdsAdded/regionIdsRemoveddrive a removal note only when something was removed. That commit also fixes an ESLint break (unused import) the previous commit introduced, which had failed all three CI jobs; the touched files now pass ESLint locally. Codex round 11 (a one-axis freeze filled the other axis from the pre-lock snapshot and could revert a concurrent change) fixed in the thirteenth:setFrozentakes each axis optionally and an omitted axis keeps what the tab holds under the lock, tracked across the plan; the tool sends only the axis named. The CI run on the freeze commit failed only@pagespace/lib#test:coverage: that package enforces 100% line/branch/function coverage onformat-request.ts, and the newsetConditionalRulesrefusals andregionContentKeyhad no lib test. Covered in the fourteenth commit (file back at 100/100/100/100 locally). All threads replied to and resolved.🤖 Generated with Claude Code
https://claude.ai/code/session_01X9AGP28YN9qgXonNY3BAZa