v0.7.50: custom sandboxes, auto model, tables currency type, perf improvements - #6120
Merged
Conversation
…the v2 table surface (#6067)
…6096) * fix(sidebar): stop the workspace switcher stranding a phantom hover The switcher's keyboard cursor is set from `onMouseMove` and only cleared when the menu closes, and it paints in `--surface-active` — the same token hover uses. So the last row the pointer crossed keeps a background indistinguishable from hover long after the pointer has gone, sitting alongside the equally-`--surface-active` current workspace as a second phantom-hovered row. It shows without any hovering too: the cursor is seeded to row 0 on open, so any user whose current workspace is not first sees two marked rows immediately. Only bites above the 3-workspace search threshold, which is why it went unnoticed. Paint the cursor only while the user is actually navigating by keyboard. Arrow keys enter that mode, any pointer motion leaves it, so in pointer mode the sole mark is real CSS :hover — which follows the pointer and leaves with it. `highlightedId` still tracks the pointer, so Enter keeps targeting the row last touched; only whether it is drawn changes. This is the pattern emcn's own popover already uses (`isKeyboardNav`, commented "prevent dual highlights") and that `tag-dropdown` consumes, and it matches how Headless UI models a combobox: one modality-driven focus marker, separate from the selected value. The list carries no `aria-activedescendant`/`aria-selected`, so the highlight is purely visual and nothing in the a11y contract changes. * test(sidebar): assert the keyboard cursor on a row that isn't selected Review round 1: the keyboard-positive assertions landed on the current workspace, which carries its own `isActive` fill, so they held whether or not the cursor was painted — the tests could not have caught deleting keyboard-cursor rendering. Navigate with ArrowUp instead, wrapping from the seeded first row to the last, and assert on that row. Verified both directions now: removing the modality gate reddens three tests, removing keyboard painting reddens two. * fix(sidebar): do not arm Enter without a visible target Review round 1: gating the cursor's paint on keyboard mode left Enter still acting on the seeded first row while that row was unmarked. The search field takes focus on open, so Enter could switch workspace with nothing shown as the target — emcn's popover instead holds its selection at -1 and ignores Enter until keyboard nav begins. Enter now acts only once a cursor is on screen, and typing counts as keyboard intent so the common "filter, then Enter" flow lands on a visible top result. Every path to Enter therefore has a marked target. --------- Co-authored-by: Waleed Latif <waleed@simstudio.ai>
* Add sim-auto: automatic model routing for the agent block (hosted) - 'Auto' model option (Sim wordmark icon, hosted-only, new-block default; runtime fallback for unset models stays claude-sonnet-5) - Resolver (lib/model-router): classifies each execution via mothership's /api/model-router and routes over two ladders — text: fireworks/glm-5.2 -> fireworks/kimi-k3 (new static hosted Fireworks catalog entries on the platform FIREWORKS_API_KEY); attachments: claude-haiku-4-5 -> gpt-5.5 (Fireworks OSS endpoints reject images). Trivial tasks skip the router; 5-min decision cache; 2s timeout; never fails the workflow - Hidden identity preamble on every auto run (English by default, don't volunteer the underlying model) - Fireworks executor: wire-name map for catalog ids (glm-5.2 -> glm-5p2), pricing keyed on the full catalog id - Billing: routing cost applied to non-streaming output cost only when mothership marks the call billable (bill-model-router flag, default off) - sim-auto special-cases: API-key condition, serialization tool lookup, edit-workflow validation (hosted), VFS model options projection * auto model * agent auto model * fix lint
… authenticate (#6102) * fix(integrations): show family service accounts on every product they authenticate An Atlassian API token authenticates Jira, Jira Service Management, and Confluence alike, so it is modeled as an `atlassian` pseudo-provider whose only service is named "Atlassian Service Account". Every credential display surface resolved through `getServiceConfigByProviderId`, which walks OAUTH_PROVIDERS in declaration order — so the credential resolved to that pseudo-service instead of to any product. The result: adding a service account from the Jira page, through a modal titled "Add Jira service account", produced a credential that appeared under neither Jira, JSM, nor Confluence, was titled "Atlassian Service Account" on its detail page, and lost its brand tile and category on the list. The same bug hid a Google service account everywhere except Gmail. - match credentials with `credentialProviderMatchesService`, which accepts a service's OAuth id or its service-account id - add `lib/integrations/credential-display.ts` as the single resolver for catalog join, mark, and copy, replacing three duplicated lookups that keyed the catalog by OAuth service *display name* — the reason the pseudo-service fell off the map - derive "family service account" from the catalog (a service-account id serving >1 integration) rather than hardcoding vendors, so a new integration joining a family needs no edit - title service-account detail pages by credential name, subtitle them with their reach, and state that reach up front on the connect form - keep the service description as the detail subtitle for every non-family credential, unchanged No schema, migration, contract, or persisted value changes; resolution is computed at render time from static config. Coverage for all 22 service-account provider ids is pinned in tests, including that the index and the predicate the Connected list filters on cannot drift apart. * chore(icons): use Atlassian's gradient marks for Jira and Confluence Replaces the flat #1868DB Jira and Confluence marks with Atlassian's gradient versions, matching the Atlassian mark added alongside them. - gradient ids go through `useId()` rather than the source SVGs' static ids, which would collide wherever two of these icons render on one page — the integrations list and the landing loops both do - pads the Atlassian viewBox so its artwork fills ~78% of the box, matching the inset Atlassian ships on the Jira and Confluence marks; without it the mark renders ~30% heavier than its siblings in the same tile Visual-only, but these marks render in ~60 files, so it is split from the credential fix to stay independently revertable. * fix(integrations): route the editor's service-account setup modal through the shared target The workflow editor's credential selector passed the OAuth service's own name and icon straight to ConnectServiceAccountModal, so opening the setup form from a Jira block titled it "Add Jira service account" while the integrations page and the chat — both of which already resolve through `useServiceAccountConnectTarget` — titled the same form "Add Atlassian service account". That is the exact confusion this branch set out to remove, surviving on the one surface that bypassed the shared resolver. * docs(atlassian): correct the service-account setup path and cover all three products The setup section could not be followed. It sent readers to a "Settings → Integrations tab" that does not exist (Integrations is a top-level workspace module) and told them to search the integrations list for "Atlassian Service Account", which matches no catalog entry — the catalog lists Jira, Jira Service Management, and Confluence. The page also described the credential as covering "Jira and Confluence" while listing Jira Service Management scopes, and the product now spells the coverage out in the connect form. - correct the path: Integrations -> Jira/JSM/Confluence -> Add to Sim -> Add service account - name all three products consistently, and state that one service account covers them - match the real button label ("Add service account")
* fix(desktop): keep page headers clear of the traffic lights In the desktop app, collapsing the sidebar hands the top-left of the screen to the content pane — the macOS traffic lights and the sidebar expander then sit over page content rather than over the sidebar. Every top-of-page header bar drew straight underneath them, so back buttons and the integrations/skills switcher were unreadable and unclickable. The reason it hit all of them at once is that the bar's geometry was copied verbatim into seven files, so nothing could reserve that lane in one place. Extract it as PAGE_HEADER_BAR and fold the lane into its top padding via `--workspace-content-title-bar-inset`, the variable the content pane already publishes for exactly this. That variable is 0px everywhere except the macOS desktop app with the sidebar collapsed, so this is inert on the web and on an expanded sidebar, where the lights sit over sidebar chrome that already reserves its own lane. Covers the settings shell, the credential/skill detail layout, the integrations and skills switcher, integration block detail and its fallback, and both upgrade headers. A guard test fails with the file named if any page re-derives the bar instead of composing it. * fix(desktop): cover the Resource header bar too The first pass found its surfaces by grepping one exact class string, which turned up the minority header. `ResourceHeader` is the same bar written as `px-4 py-[8.5px]`, and it is the one logs, files, tables, knowledge and scheduled tasks use — plus every loading fallback. Those all still drew under the traffic lights, and on the breadcrumb surfaces the occluded element is an interactive popover button, not just a title. Share the lane math as TITLE_BAR_LANE_PT and compose it from both bar geometries, which genuinely differ (the Resource bar is bordered and has a min height). Nothing nests one bar inside the other, so no reset is needed: the only out-of-pane render is the landing tables preview. Move the variable's `0px` default to `:root` alongside the other desktop-title-bar vars, so it is defined for bars outside `.workspace-content-shell` — the standalone settings shell and that landing preview — where an undefined var() inside calc() would be invalid at computed-value time and drop padding-top entirely. That replaces the per-call fallback, leaving one default instead of two that can drift. Fold the assertions into the existing desktop title-bar surface audit rather than a second audit file with its own conventions. The standalone guard is gone: it keyed on one spelling of the geometry, so it was blind to `px-4 py-[8.5px]` — the very re-derivation that made this pass necessary. * fix(desktop): cover fullscreen routes, spare embedded panels Review round 2 found the two arrangements the collapsed-sidebar selector alone gets wrong. A fullscreen route (/upgrade) slides the sidebar to zero width without collapsing it, so `data-sidebar-collapsed` is absent and the lane stayed zero while the pane was in fact sitting under the traffic lights. The pane owns the lane whenever the sidebar is not there to own it, so the selector now matches a new `data-content-fullscreen` as well. The mirror error: the variable is inherited, and the mothership panel is the right half of the pane — never under the lights — yet it embeds whole pages (KnowledgeBase and friends) whose header bars reserve the lane. Those bars were gaining ~38px inside the panel. The panel now zeroes the variable for its subtree. I had checked for exactly this nesting and concluded it did not exist, having looked only at the settings pages that import `Resource` types without rendering `Resource.Header`. `resource-content.tsx` renders the knowledge page itself, which does. An assertion each, both verified to fail when the fix is reverted. * fix(desktop): size the peek card to its content, soften the overlay shadow The floating sidebar pinned both its top and bottom edges, so it always drew at full window height. On a short surface — the settings list — that left a tall empty slab of card hanging below the last entry. The card now hugs its content and caps at the pane height less the traffic-light lane and the bottom gutter. Dropping the bottom pin is most of it: the four `h-full` rules down the chain resolve against an auto-height parent and collapse to content on their own. But nothing would then bound the sidebar's own `flex-1 overflow-y-auto` region, so a long workflow list would be clipped by the card's `overflow-hidden` instead of scrolling. The card is therefore a capped flex column, and the shell is allowed to shrink inside it, which restores a definite height for the chain to resolve against. That rule is scoped to `[data-peek]` and is inert while docked, where the shell is not a flex item. Also eases `--shadow-overlay` in both themes (alpha ~27% lighter, bloom pulled in from 48px to 30px), keeping it clearly above `--shadow-medium` so the scale still reads in order. The peek card drops the shadow entirely and separates on the same `--border` hairline the content pane beside it uses. * fix(desktop): stop the login page scrolling, drop the pane border at the window edge Two reports against the desktop window chrome. The login page scrolled by exactly the traffic-light lane. `.desktop-title-bar-page` reserved the lane with `margin-top` plus a `calc(100vh - lane)` height, which sums to the viewport on its own — but `body` carries `min-height: 100vh`, and body is a plain block box with no padding, border, or BFC, so that top margin had nothing to collapse against and collapsed through, displacing body itself. The document came out one full lane taller than the viewport while the shell's `calc` saving was re-inflated underneath it. Reserving the lane with padding *inside* the box removes both the collapse and the `calc`: global `box-sizing: border-box` keeps the padding within the `100vh`. Measured in the Electron renderer over CDP — 40px of overflow before, 0 after, with the logo still clear of the lane. Collapsing the sidebar also left a hairline outline traced just inside the window. The shell drops to `p-0` there, but the content pane kept its border and 8px radius, so both drew flush against the square window frame. The pane now drops them exactly when it is flush. Keyed off the ancestor attributes rather than React state, because the title-bar attribute is written pre-paint and a state-driven rule would flash the border before hydration settles. * docs(desktop): correct the peek card chrome comment The card's TSDoc still listed `shadow-overlay` as part of its chrome after the shadow was deliberately dropped, so the comment contradicted the code. It now records that the card is unshadowed on purpose, and documents the content-hugging height and the flex-column cap that make the sidebar's scroll region bound itself. --------- Co-authored-by: Waleed Latif <waleed@simstudio.ai>
… dead surface (#6105) * fix(integrations): keep the Atlassian coverage hint visible, and trim comments Cursor Bugbot: `ChipModalField` hides a `hint` whenever that field shows an `error`, so the multi-product coverage sentence vanished the moment the domain format check fired — exactly when someone mid-form most needs it. The sentence also describes the token, not the domain, so it read as domain guidance. Moves it to the API token field, which surfaces its errors through `ChipModalError` at the bottom rather than its own `error` prop, so the hint cannot be displaced. Also drops rationale comments that restated the code they sat above. * chore(integrations): trim the credential-display barrel to what consumers use * fix(emcn): let a custom modal field associate its hint with the control it wraps `ChipModalField` computes `aria-required`/`aria-invalid`/`aria-describedby` from its own state, but `type='custom'` returned its children untouched — so the `hint` and `error` text it renders was visible and never announced. The field cannot apply the ARIA itself here: a custom child may be a bare input or a wrapper several levels above one, which is the same reason `associatesLabel` already excludes custom from the label's `htmlFor`. Adds a function form for custom children that receives the ARIA, so the consumer — which knows where focus lands — attaches it. Existing `ReactNode` children are unaffected; all five current custom-field call sites keep working untouched. Uses it for the Atlassian API token field, whose coverage hint this branch had just relocated onto a custom field.
* fix(invitations): report every reason a batch of invites failed
The invite modal collapsed a multi-failure batch to `N invitations
failed.` plus the first reason and discarded the rest. The batch endpoint
rejects per email and each reason names its own address, so inviting
several people who are all ineligible for the chosen membership — the
common multi-failure case, since External requires every invitee to
already be on a paid plan — named one of them and hid the others. The
failed addresses are reseeded into the field as ordinary chips carrying
no error state, so nothing else on screen identified them either.
`buildInviteFailureMessage` now lists the distinct reasons, collapses
identical ones, and reports the denominator ("2 of 5 invitations could
not be sent") the way add-people-modal already did for the same partial
-failure shape. Past three reasons the tail is counted rather than
listed, so a large batch cannot bury the modal in near-identical
sentences.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): name the membership field for the org it acts on
The dropdown grants organization standing — Member and Admin join the
org and take a seat, External does not — but the label said only
"Membership", which reads as membership of the workspace being invited
to, the thing the field directly above it already controls. The EE
access-control modal has its own "Membership" field for permission-group
membership, so the bare word was ambiguous across surfaces too.
Sentence case matches the sibling "Workspace access" in the same form.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
* feat(sandboxes): workspace dependency sets for Function blocks
Named package sets a Function block can import from. The server
canonicalizes and hashes the list; E2B prebuilds a content-addressed
template per set, Daytona installs per execution. Create/edit is gated to
Max or Enterprise via the shared workspace entitlement check; execution is
deliberately ungated, so a downgraded workspace keeps running what it
already built.
Also on this branch:
- Extract the duplicated dropdown/combobox option-fetch lifecycle into
use-fetched-options. Only combobox had the dependency-change reset, so
every dropdown with dependsOn + fetchOptions cleared its list and never
repopulated until reopened.
- Collapse the repeated Max-tier entitlement check onto one
hasMaxTierWorkspaceAccess, shared by inbox, live sync, and sandboxes.
- Resolve a personal payer's block state through getEffectiveBillingStatus
in getBillingEntityBlockStatus, so the client-side Max gates agree with
the server-side ones when blockOrgMembers' fan-out is stale.
- Carve the Daytona dependency install out of the caller's execution
budget instead of stacking on top of it.
Co-Authored-By: Claude <noreply@anthropic.com>
* chore(db): regenerate the sandboxes migration as 0273
Staging claimed 0271 and 0272 while this branch was out, so the hand-authored
0271_workspace_sandboxes was dropped before the merge and regenerated on top
of the merged schema. Same DDL; drizzle emits plain CREATE TABLE/INDEX rather
than the hand-added IF NOT EXISTS, which matches the repo default — that
idempotent form is only needed for files with CONCURRENTLY ops below an
embedded COMMIT. Regenerating also restores the meta snapshot the
hand-authored migration never had.
Co-Authored-By: Claude <noreply@anthropic.com>
* chore(db): drop the sandboxes migration ahead of the staging merge
Staging independently claims idx 0273, so remove ours before merging to
avoid an add/add conflict on the drizzle migration index. Regenerated at
the next free index once the merge lands.
Co-Authored-By: Claude <noreply@anthropic.com>
* chore(db): regenerate the sandboxes migration as 0275
Staging took 0273 and 0274, so the sandboxes DDL lands at the next free
index. The emitted SQL is byte-identical to the dropped 0273.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(billing): consolidate the Max-tier entitlement onto one predicate
The Max tier was spelled five ways. The odd one out — `isMax`, defined as
`isPro(plan) && credits >= 25000` — excluded both `team_25000` and
`enterprise`, and it was the sole input to the personal-workspace cap. A
delinquent Max-for-Teams org admin got 1 personal workspace while a
delinquent Max individual got 10. Only free/pro_6000/pro_25000 were tested,
so the two broken tiers were unpinned.
Separately, the server gate and the client `hasUsableMaxAccess` were
independent copies of the same rule. The settings sidebar renders Sandboxes
and Sim Mailer from the client one while the API answers 403 from the
server one, so any drift renders a feature unlocked that the API refuses.
- `MAX_TIER_CREDITS` is derived from the `CREDIT_TIERS` table; `isMaxTier`
in plan-helpers is now the single definition, shared by the server gates,
the client derivation, `getPlanTypeForLimits`, `plan-view`, and the cap
- `hasWorkspaceTierAccess(id, predicate, { intent, onMissingWorkspace })`
becomes the one org-vs-personal payer fork. `intent: 'active-use'` means
active and not billing-blocked; `'retention'` means active/past_due with
block state ignored, so the inbox teardown guard keeps its fail-open
semantics instead of implying them through a duplicated fork
- `isWorkspaceOnEnterprisePlan`'s personal branch now applies the status and
block checks its own org branch always had, and its TSDoc names its real
consumer (copilot BYOK, not Access Control)
- the client live-sync gate gained the server's `isHosted` branch, so a
self-hosted deploy with billing on no longer locks an interval the API
accepts. It reads both flags directly rather than taking one as a
parameter the callers sourced from the same module
- `sqlIsPro`/`sqlIsTeam` escape the `_` LIKE wildcard, matching the already
correct hand-rolled filter in seat-drift
- deletes the `TERMINAL_SUBSCRIPTION_STATUSES` and `ENTITLED_STATUSES`
shadow constants, and corrects three test mocks that asserted `trialing`
was entitled or usable
`max-tier-parity.test.ts` asserts the client and server answers match for
every plan name. Both new guards were checked against the old code: the
parity test fails 3 assertions with the previous predicate, and the
self-hosted test fails without the `isHosted` branch.
Co-Authored-By: Claude <noreply@anthropic.com>
* chore(db): drop the sandboxes migration ahead of the staging merge
Staging has claimed 0275 (table_views) and 0276 (drop_legacy_folder_tables)
since the last merge, so our 0275_workspace_sandboxes collides on the index.
Dropping ours first — the .sql, meta/0275_snapshot.json, and the journal
entry — leaves packages/db/migrations byte-identical to the merge-base, so
the merge sees no add/add conflict at all. Regenerated on the far side.
Ours is the droppable side: plain additive DDL with no hand edits, which
drizzle reproduces exactly. Staging's migrations are hand-written and must
survive.
Co-Authored-By: Claude <noreply@anthropic.com>
* chore(db): regenerate the sandboxes migration as 0277
Staging claimed 0275 (table_views) and 0276 (drop_legacy_folder_tables), so
the sandboxes migration dropped before the merge comes back on top as 0277.
The emitted SQL is byte-identical to what was dropped — the original had no
hand edits, so there is nothing to reapply. It is purely additive: two enums,
sandbox_image and workspace_sandbox, their two FKs and six indexes. That it
regenerated unchanged also confirms the schema.ts auto-merge was correct —
had it lost staging's legacy-folder-table drops, drizzle would have emitted
CREATE TABLE for them here.
Snapshot chain is continuous (0273 -> 0277, each prevId matching the previous
id) and the table counts track the DDL: 100 -> 101 (table_views) -> 99
(legacy folder tables dropped) -> 101 (the two sandbox tables).
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(sandboxes): gate on the enterprise feature flags, drop the rollout switch
Sandboxes shipped behind `custom-sandboxes`, an AppConfig rollout flag falling
back to a `CUSTOM_SANDBOXES` secret. That made it the only Max-gated surface
with no self-hosted path: `INBOX_ENABLED` can force Sim Mailer on for an
operator running their own billing, and `ENTERPRISE_ENABLED` turns on the
other nine features at once, but neither reached sandboxes. A self-hoster had
to find a separately-named variable that was not part of that family, and one
running with billing enabled could not enable it at all.
Sandboxes now joins the enterprise feature set and the rollout flag is gone:
- `sandboxes` is an `EnterpriseFeature` with `SANDBOXES_ENABLED` and its
`NEXT_PUBLIC_` twin, so the master switch and the per-feature override both
reach it like every sibling
- `hasWorkspaceSandboxAccess` takes the inbox's shape exactly — the override
wins, then a deployment without billing is unrestricted, then the workspace
payer needs usable Max or Enterprise
- the settings nav gains `selfHostedOverride`, so the section resolves through
the same path as Sim Mailer instead of a second entitlement AND-ed in
- `custom-sandboxes`, the `CUSTOM_SANDBOXES` secret, the now-unreachable
`SANDBOXES_UNAVAILABLE` 403 copy, and the route's kill-switch branch are
deleted
Its legacy default is `true`, matching `inbox`: the gate already returns true
whenever billing is off, so `false` would leave the nav override disagreeing
with the gate that answers the request. Self-hosted builds run on the
operator's own E2B/Daytona credentials, so there is no Sim-side cost to
withhold — the docs now say so, since enabling the feature without a provider
configured is the obvious trap.
The new gate tests run with billing enabled on purpose; the `!isBillingEnabled`
bail would otherwise answer every case and hide whether the override is wired.
Verified by deleting the override line — exactly the one assertion fails.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): let the language menu match its trigger width
`matchTriggerWidth={false}` exists for the opposite case — a narrow trigger
whose option labels would truncate, letting the menu grow past it. The language
field is a full-width form control with two short labels, so the override
shrank the menu to "JavaScript" and pinned it to the right edge instead.
The default (`true`) is correct here. Every other consumer passing `false` is a
genuinely narrow trigger — a role picker in a member row, a table filter chip.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): re-queue a build when resolution finds the image unusable
`ensureSandboxImage` only ran when a sandbox was saved, so resolution treated
an unusable image as terminal and told the user to go fix a definition that was
never wrong. Three states stuck permanently until someone re-saved in Settings:
- a build that failed
- a build whose worker died mid-flight, stranding the row in `building`
- every sandbox created while the deployment ran a `runtime` provider, after a
switch to a `prebuilt` one — `runtime` writes no image rows at all, so the
whole fleet resolved to "no completed build" with nothing to repair it
Resolution now re-queues through the registry's existing idempotent entry point
before failing, and says a build is on its way instead of pointing at Settings.
The conflict guard already claims only a `failed` row or a stale `pending`/
`building` one, so executions arriving during a healthy build enqueue nothing —
no thundering herd from a hot workflow.
The registry is imported dynamically for the same reason `sandboxDb` is: it
pulls `@sim/db` into the static graph, which this module keeps out of the
executor bundle. That also avoids a cycle, since the registry imports
`invalidateSandboxResolution` from here. A repair that itself fails is logged
and swallowed — it must never replace the build error naming the sandbox.
Verified by deleting the repair call: exactly the three new assertions fail.
Co-Authored-By: Claude <noreply@anthropic.com>
* improvement(sandboxes): let the picker show just the sandbox name
The label read "Test · Python · 1 package". The block's own list is already
scoped to the language its sibling `language` subblock selects, so the language
repeated on every row said nothing, and the package count is decoration next to
the name that identifies the sandbox.
The language stays for the one caller that cannot filter — agent tool-input
renders this field under a synthetic id where the sibling `language` value is
unreachable, so its list spans both languages and the name alone is ambiguous.
That is the same missing value which disables filtering, so `showLanguage` is
derived from it directly rather than passed independently and left to drift.
A failed build is still marked: that suffix is the difference between a
selection that runs and one that does not.
Passing the flag also means dropping `.map(toSandboxOption)` for an explicit
arrow — `Array.map` hands the index to the second parameter.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): show the sandbox name on the block card, not its uuid
The card printed "443f4934-26ab-44ab-8...". `resolveDropdownLabel` only reads a
subblock's static `options` array, and the sandbox picker is a `combobox` whose
options load asynchronously, so its array is empty and the raw stored id fell
through to the label.
Resolved the same way skills and tools already are: a `resolveSandboxLabel` in
the display layer, fed from the shared sandbox list query — the same cache entry
the picker reads, so this adds no request.
Two deliberate scopings:
- the query is subscribed only for the sandbox row. `SubBlockRow` is memoized
per subblock, and the list query polls while a build is in flight, so an
unconditional hook would re-render every row on the canvas on each poll tick
- the resolver matches the field id, not just the type. There is no dedicated
subblock type for it, and matching `combobox` alone would relabel unrelated
pickers
An id with no matching sandbox resolves to null rather than a guess, so a
deleted sandbox falls through to the caller's placeholder. The template preview
surface is left alone: it is explicitly hook-free and passes empty lists for
tools and skills too.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): hide the Sandboxes section with no provider configured
Entitlement decides whether a workspace may author sandboxes; nothing decided
whether anything could run one. A self-hosted deployment with SANDBOXES_ENABLED
but no E2B or Daytona credentials got a fully functional tab whose output no
Function block could select — the picker is gated on the provider vars, the tab
was not.
Both navigation planes now drop the section when neither
NEXT_PUBLIC_SANDBOX_ENABLED nor the pre-Daytona NEXT_PUBLIC_E2B_ENABLED is set —
the same pair the picker's `showWhenEnvSet` reads, so the two cannot disagree.
Dropped rather than locked: an upgrade does not conjure a provider.
The unified plane drops it in `buildUnifiedSettingsNavigation` rather than in the
sidebar's filter, because the sidebar's `selfHostedOverride` short-circuit runs
before its `requiresMax` check and would have revealed the tab anyway. It reads
the browser twins, not the server's `isRemoteSandboxEnabled`, since this module
renders on both sides.
The predicate is a function, not a module constant, because the constant form was
untestable and ambient: the env mock falls through to `process.env`, and
`apps/sim/.env` (gitignored, so absent on CI) sets NEXT_PUBLIC_E2B_ENABLED=true.
The nav tests passed locally and failed 6 assertions with the flag cleared. They
now pin both flags, so the suite is identical with and without a local env file —
verified by running it both ways.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(sandboxes): correct three claims the code no longer makes
The Sandboxes section described behavior two commits on this branch changed, and
led with an internal detail no reader needs.
- entitlement is no longer Max/Enterprise only: self-hosted deployments unlock
sandboxes with SANDBOXES_ENABLED, and the section is hidden outright when a
deployment has no sandbox provider, which is the state a self-hoster is most
likely to hit and least likely to diagnose
- a build that is not Ready is no longer terminal. It is queued again on the next
run, so the advice is to wait and re-run, not to go edit a package list that
was never wrong
- deleting a sandbox frees its build once nothing else references it. Builds are
shared by content, so this is the one place a reader could reasonably assume
deletion is immediate
Dropped the `ModuleNotFoundError` aside: what the old code did instead is not
something a reader needs to know to use the feature.
The page is hand-written — `function` has category 'blocks' and is absent from
`NATIVE_RESOURCE_BLOCK_TYPES`, so generate-docs skips it and these edits will not
be overwritten.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(sandboxes): release the provider image when nothing references it
Deleting a sandbox only removed its row, leaving the built template in E2B until
the 30-day retention sweep — up to a month of paying to store an image nothing
could select. Editing a package list had the same effect on the old content
address, which is the more common case since every edit re-points the sandbox.
`releaseSandboxImage(specHash)` now deletes the provider image and its row from
both paths. It reuses the sweep's provider call and its ordering: image first,
row second, so a refused delete leaves the row for the sweep to retry rather than
orphaning a remote template nothing points at.
Two guards make eager deletion safe:
- builds are keyed by content, not by workspace, so two workspaces declaring the
same package list share one image. The release no-ops while any sandbox still
references the hash — otherwise one workspace's delete would break the other's
- an in-flight build is left alone rather than raced; the sweep collects it once
it settles
Called detached from both routes. The row is already committed by then, so the
user's action has succeeded whatever the provider says, and awaiting would hold a
UI delete open on a remote call the sweep would retry anyway. Every failure inside
is logged and swallowed for the same reason.
E2B's delete verified against their API reference: DELETE /templates/{templateID}
with X-API-Key, 204 on success. The existing implementation already matched, so
this commit only adds the call sites and the guards.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): rate-limit the automatic rebuild, drop the one-off status dot
Two follow-ups to the resolution repair.
The repair had no rate limit. `ensureSandboxImage` re-claims a `failed` row on
sight, and a bad package name fails in seconds, so the in-flight guard never
closed the window: a workflow on a one-minute schedule would enqueue a build a
minute against a package list that will never resolve, each one real provider
build compute. Before the repair existed resolution simply threw, so this was
introduced with it.
The two callers want different things, so the cooldown is opt-in. A save is a
person explicitly asking for another attempt and still retries immediately;
resolution passes `FAILED_BUILD_RETRY_COOLDOWN_MS` and gets at most one attempt
per window no matter how often the workflow runs. Ten minutes: long enough that
per-minute runs cannot drive per-minute builds, short enough that a transient
registry outage clears within the hour.
The status line loses its colour dot. `size-[6px] rounded-full` appeared in
exactly one file in the repo, so it was a new primitive rather than a pattern,
and it duplicated state the text colour already carries — the label now turns
`--text-error` on a failed build, which is what every other status row in
settings does. `ChipTag` was the wrong home for this: its variants are
`mono`/`invite`, with no semantic tone, so a status version would have meant
overriding its chrome from the consumer.
Also corrects the docs line this changes: a failed build is retried periodically,
and saving is the way to retry now, so "wait a moment and run again" no longer
describes it.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): claim the image row and its reference check in one statement
Greptile P1. Reading references in one statement and deleting in another left a
window — a wide one, since a provider delete is a network call — where a second
workspace could declare the same package list, inherit the `ready` row, and have
its next run fail against a template already on its way out. Content addressing
is what makes that reachable: the image is shared, so one workspace's delete can
strand another's sandbox.
The reference check now lives in the conditional DELETE itself, so winning the
delete is the proof that nothing referenced the hash. A workspace that adopts the
hash first makes the delete match nothing and the release becomes a no-op.
Claiming the row before the provider call would otherwise strand a template
nothing points at if the provider then refused, so that path puts the row back
and the retention sweep inherits the retry — the same property the previous
ordering had.
The sweep is deliberately left as it is: its equivalent window needs a hash
unreferenced AND unused for 30 days, and its provider-first ordering encodes the
documented retry-on-refusal behaviour this path now reproduces explicitly.
No transaction is opened. The provider call sits between discrete statements
rather than inside one, so no pooled connection is held across it — which is why
this uses a conditional delete instead of the repo's `pg_advisory_xact_lock`
pattern, whose lock only releases at commit.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): route the retention sweep through the same image claim
Cursor and Greptile both flagged the sweep as still carrying the interleaving
just fixed in releaseSandboxImage, and they are right — the reason given for
leaving it alone last round does not survive scrutiny.
That reason was that provider-first ordering encodes retry-on-refusal, so making
the claim atomic would trade a race for an orphaned template. The release path
already answers that: claim the row, and put it back if the provider refuses. The
sweep can have both properties too.
The rarity argument was also weaker than stated. The sweep nominates up to 200
candidates and then works through them eight network deletes at a time, so its
check-to-delete gap is seconds to minutes — wider than the window that was just
closed, not narrower.
Both callers now share `claimAndDeleteImage`, which owns the whole contract: the
unreferenced check lives inside the DELETE, the provider call runs only after the
claim succeeds, and a refusal restores the row. Having written that ordering twice
is what let the two paths drift, so it exists once now.
The sweep's query becomes a nomination step only. Its retention cutoff is passed
into the claim rather than trusted from the earlier read, so a candidate that
stops qualifying mid-sweep fails its claim and is skipped instead of losing its
image.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): rebuild a hash adopted while its image was being deleted
Greptile's third pass on this path, and a case the previous two did not cover:
the adopter starting a *fresh build* rather than inheriting a ready row.
Claiming removes the registry row, so between that and the provider delete
finishing, a workspace can declare the same package list, get a new row, and start
a build under the same content-derived imageRef — which the in-flight delete then
removes.
The window itself is inherent. The registry row and the provider template are two
systems with no shared transaction, so it can be narrowed but not closed. A Redis
lock would not close it either: acquireLock returns true when Redis is absent, so
it cannot be a correctness guarantee for self-hosted. Holding a Postgres advisory
lock would, but only by pinning a pooled connection for the length of a provider
call, which is a worse trade.
What was avoidable is the adopter finding out the slow way. Its row is new and
healthy-looking, so nothing noticed: resolution only repairs a row that is missing
or failed, and a failed one waits out the retry cooldown first. The release path
now re-checks after the delete and re-enqueues, so the rebuild starts immediately
instead of one failed run plus a cooldown later. A build already in flight is left
to the conflict guard, since it may still outlive the delete.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): reclaim a ready row whose image was deleted underneath it
Greptile found the hole the previous commit left, and it is the case that made the
claim in that commit's message wrong: this one is permanent, not transient.
If a re-adopted hash reaches `ready` before the in-flight provider delete lands —
plausible, since E2B layer caching can rebuild an identical spec in seconds — the
row looks healthy while its imageRef points at nothing. Resolution repairs a row
that is missing or failed, never one claiming to be ready, so nothing recovers it.
The sandbox stays broken until someone re-saves it by hand.
`rebuildIfReadopted` called `ensureSandboxImage` with no options, whose conflict
guard reclaims only a failed or stale in-flight row, so it silently did nothing in
exactly that case.
The release path now passes `imageKnownGone`, which widens the re-claim to any
settled row rather than only a failed one. It is the one caller that knows the
image is gone regardless of what the row says. An in-flight build is still left
alone: it either recreates the template it was building or fails into the normal
repair path, and resetting it would only add a duplicate build.
The three ways a settled row may be re-claimed now sit in one `settledRebuildBranch`
helper — any settled row when the image is known gone, a failed one after the
cooldown for an automatic caller, a failed one immediately for a person — because
inlining the third case is what hid the gap.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): let a same-spec save retry a failed build
Cursor Bugbot. `scheduleSandboxBuild` sat inside the changed-hash branch, so a save
that did not alter the package list never reached the registry. The comment above
it described the opposite — that an unchanged spec finds a ready row and enqueues
nothing — which is what `ensureSandboxImage` does, but only if it is called.
That made the docs wrong too. They tell a reader to save the sandbox again to retry
a failed build immediately, and this branch is exactly why that did nothing: the
only way to retry was to edit the package list into a different hash, which is not
what someone recovering from a transient registry failure wants to do.
The call is now unconditional and the registry decides what a save costs, which is
what its conflict guard is for: a ready or in-flight row is left alone, a failed one
is re-claimed at once. Releasing the previous image stays behind the hash check,
since only a changed hash orphans one. Cache invalidation is unchanged —
`scheduleSandboxBuild` already does it, which is why the else branch existed.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(sandboxes): correct the image cache's staleness invariant
Cursor Bugbot found that a released image can still be served from another
replica's cache. The finding is real, and the reason it went unnoticed is that the
cache documented an invariant which eager release quietly broke.
It claimed a `ready` row is terminal for its spec hash, so a cached hit could not
go stale in a way that matters. That held while the only ways a row changed were an
edit (new hash) or a delete (caught by the `workspace_sandbox` read). Releasing an
image eagerly made a `ready` row disappear with the hash unchanged, so the premise
no longer holds and the comment was actively misleading to the next reader.
No behaviour change here — the exposure is bounded at IMAGE_TTL_MS on replicas
other than the one that ran the release, and it self-heals once the entry expires
and the row read finds nothing. Closing it properly needs cross-replica
invalidation or a provider-error path that invalidates on "template not found",
both of which are larger than a review fix; the comment now says so instead of
implying the problem cannot exist.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(sandboxes): note that a JavaScript sandbox needs an import to apply
Cursor Bugbot pointed out that `useRemoteSandbox` keys on detected static
import/require and never on the selected sandbox, so JavaScript without one runs
locally and the selection has no effect.
Keeping the behaviour: honouring the selection would force those blocks remote,
and the large-value-ref guard immediately below would then reject code that runs
fine today. Documenting it instead, next to the picker, since a selection that
silently does nothing is only surprising if nothing says so.
Python is unaffected — it always runs remotely, so its sandbox always applies.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): stop create mode surviving a return to an open sandbox
Cursor Bugbot. Create mode and having a sandbox open are mutually exclusive, but
nothing enforced it, so both could be set at once — and the screen then lied about
which sandbox its Delete pointed at.
With `isCreating` true and `selectedId` restored, `baseline` is null, so the editor
renders an empty "New sandbox" form, while the Delete action is built from
`selected` and still targets the restored sandbox. An admin looking at a blank
create form could delete a sandbox it never named.
Two ways in, both closed:
- Browser Forward after starting a new sandbox restores `selectedId` without going
through `closeEditor`. The render-time sync that already drops a stale draft now
also leaves create mode, which is the same class of correction and the reason
that block exists.
- "New sandbox" set `isCreating` without clearing `selectedId`, so the same
contradiction was reachable without touching history at all. It now clears the
selection, with `history: 'replace'` because switching mode is not a destination.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(ci): pin the sandbox flag in the second nav catalog test, bump the chart
Two CI failures, both mine.
`app/workspace/[workspaceId]/settings/navigation.test.ts` asserts the unified
catalog and was left on ambient env. Dropping the Sandboxes section without a
sandbox provider made it 26 items instead of 27 on CI, which has no
`apps/sim/.env` — the same trap already fixed in the sibling
`components/settings/navigation.test.ts`, in the one file that was missed.
Fixing it needs `vi.hoisted` rather than the sibling's `beforeEach`, because this
file reads `allNavigationItems`, built once at module load; a hook would run after
the value it is trying to influence already exists.
The chart gate is separate: this branch adds sandbox settings to
`helm/sim/values.yaml`, and the workflow requires a Chart.yaml bump whenever
`helm/sim/**` changes. Additive config, so 1.3.0 -> 1.4.0 by SemVer.
Verified by running the whole suite with the flags forced off, not just the two
navigation files — no other test depends on a local env file.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): keep the row restore to a refused delete only
Cursor and Greptile, independently, on the same code. `deleteImage` and
`rebuildIfReadopted` shared one try/catch, so a rebuild failure after a *successful*
provider delete was handled as if the provider had refused: the catch put the
claimed row back, `ready` status and all, pointing at a template that no longer
exists.
That is the one state resolution cannot repair — it fixes a row that is missing or
failed, never one claiming to be ready — so it reintroduced the permanent breakage
an earlier commit had just closed, through the error path rather than the happy one.
Restoring now belongs strictly to a refused delete. Once the template is gone the
row stays gone, and the rebuild runs past that catch. The rebuild also swallows its
own failures: it follows a delete that already succeeded, so it must not be reported
as a failed release, and inside the sweep it must not reject the rest of its chunk.
The adopter's next run still reaches the normal repair path.
The regression test drives a rebuild failure and asserts no row is restored. It
fails against the original shape — rebuild inside the shared try, no inner catch —
which is what the two reviewers were describing.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): drop the dead row when a re-adopt rebuild cannot be scheduled
Greptile, one layer under the previous fix. Making the post-delete rebuild swallow
its own failures kept it from being reported as a failed release, but left the
adopter's row claiming a `ready` image whose template is already deleted — the one
state resolution cannot repair, since it rebuilds a row that is missing or failed
and never one that says ready.
So the row is now dropped when the rebuild does not take. That turns the adopter
into the missing-row case, which the next execution repairs on its own, instead of
a sandbox that stays broken until someone re-saves it by hand. A failure to drop it
is logged at error, because at that point two writes in a row have failed and there
is nothing further this path can do.
Also gives the release tests a default "nothing re-adopted" select. Without it the
rebuild threw on an unstubbed mock and the cleanup delete overwrote the predicate
the claim assertions read, so two of them were passing on the wrong statement.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(sandboxes): repair a missing image at create, where the truth is observable
Six review rounds narrowed the window between deleting a shared template and
another workspace adopting its content hash, and each fix exposed the next facet.
They all share a cause: the registry row and the provider template are two systems
with no shared transaction, so any scheme that keeps them in step is guessing.
Create is the one step that does not have to guess. It either gets a sandbox or it
does not, so a `ready` row pointing at a deleted template now corrects itself the
first time it is used, rather than needing someone to re-save the sandbox.
- `SandboxImageBuilder.isMissingImage` asks the provider to classify its own
failure. Prebuilt-only, because a runtime provider has no image to miss
- E2B answers it off `NotFoundError`, which the SDK maps from a 404. The only
resource a create names is the template, and the two subclasses that describe
other calls — a missing file, an exited sandbox — are excluded. The classifier
stays deliberately narrow: treating auth or rate-limit failures as a missing
image would turn a provider outage into a build storm
- `repairMissingSandboxImage` invalidates the cache, rebuilds with
`imageKnownGone` (no cooldown, since this observed the image is gone rather than
inferring it), and returns copy telling the author to run again
- `ResolvedSandbox` carries `specHash` so the failing execution can name what to
rebuild
This subsumes the open facets rather than adding another guard beside them: the
stale per-replica cache, an adopter left `ready` against a deleted ref, and a
rebuild that never took all end at the same place — the next run repairs itself.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(sandboxes): key the build trigger by attempt, not by spec
Cursor Bugbot. The Trigger.dev idempotency key was the content address alone, so a
second attempt at the same spec was deduped against the first: the SDK returns the
finished run instead of starting one, and the row that `ensureSandboxImage` just
flipped to `pending` sits there with no worker. Nothing can re-claim a `pending`
row until it goes stale, so a retry inside the 5-minute TTL did nothing for the
next half hour.
That silently disabled every repair path — save-to-retry, which the docs name
explicitly, and both the resolution and create-time rebuilds.
The key's own comment already said it exists "to collapse concurrent saves of the
same spec into one build, not to suppress a retry after one failed". The conditional
update above it is what actually collapses concurrent saves: only one caller gets a
row back, so only one ever reaches the trigger. Keying by the claim's `updatedAt`
keeps that property and makes each genuine attempt distinct, while a duplicate
delivery of one attempt still collapses.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(sandboxes): create a sandbox from the picker, and fix three UI papercuts
The Function block's sandbox field now pins a "Create Sandbox" row above its
options, matching the "Create Skill" / "Create Tool" rows it sits beside, so
authoring a package list no longer means leaving the workflow for Settings. The
row is declared by the field (`createAction`) rather than hardcoded by id;
block configs are read by the serializer and executor, so the name maps to a
modal in the picker rather than carrying a component.
Two things the modal has to get right. It seeds the new sandbox's language from
the sibling the list is scoped by, or a sandbox created off a JavaScript block
would land in the Python list and vanish. And the created option is held locally
until a real fetch carries it, or the field would sit on a raw uuid until
hydration answered.
Also:
- The Sandboxes icon was the Logs block's icon (`blocks/blocks/logs.ts`), in
both the settings nav and the list rows. It is the Function block's now.
- "Default image (no extra packages)" claimed something untrue: E2B and Daytona
base images both ship with packages installed.
- A new sandbox opened in Python while the Function block defaults to
JavaScript. The test pins the two together rather than the literal.
Draft shape and helpers moved out of the editor component into `utils.ts` —
three consumers now, and it makes the defaults testable without a DOM.
* feat(settings): one Max-plan wall, and give the create modal the same one
The create-sandbox modal answered a non-Max workspace with a red line under a
form it could never submit, and no way to act on it. It now renders the same
wall the Settings > Sandboxes tab does — heading, one sentence on what the plan
unlocks, and an Upgrade to Max chip — instead of the fields.
That wall existed twice already (sandboxes and Sim Mailer), so this extracts it
rather than adding a third copy. `SettingsUpgradeNotice` owns the copy rhythm
and the route, and `compact` trades the page's full-height centering for a
modal's. Both settings consumers now compose it; neither keeps its own markup.
The action lands on billing, which `resolveSettingsHref` already redirects to
the plan-comparison page for a member who cannot manage billing — so it is a
route to explore plans, never a dead end. The chip stays hidden for non-admins,
exactly as the settings pages had it.
A non-admin on an entitled workspace gets the muted reason rather than the
upgrade wall: buying a plan is not what is in their way.
---------
Co-authored-by: Claude <noreply@anthropic.com>
…6106) * feat(tables): add currency column type on a new column-type registry Adds a `currency` column type, and consolidates the per-type knowledge it would otherwise have been scattered across. **Currency.** Stores a plain number and carries an ISO 4217 `currencyCode` as display metadata. That split is what keeps it cheap: filtering, sorting, uniqueness and CSV export all reuse the numeric paths unchanged, changing a column's currency rewrites no rows, and the public row output stays a number rather than a locale-formatted string consumers would have to reparse. Input accepts the shapes an amount actually arrives in — `$1,234.56`, `1 234,56 €`, `(12.00)` — so pastes, CSV imports and tool writes land as numbers instead of being nulled. **The registry.** Adding this type initially required edits in ~40 places: 32 switch arms under `lib/table`, ~26 UI branches, two hand-maintained icon maps, and a coercion implementation duplicated four times. Every one of those failed silently when missed — a missing `jsonbCastForType` arm compares numbers as text; a missing compatibility arm blocks all conversions. `lib/table/column-types/` now holds one file per type carrying its label, icon, badge colour, storage cast, filter operators, coercion, validation, compatibility and formatting. `Record<ColumnType, …>` on both registries is the completeness gate: adding a type to the union is a compile error naming exactly the two files to fill in, and the interface then requires every field. The 32 switch arms are down to 3. Two duplicates collapse as a consequence: - The client no longer mirrors the server's select id-resolution. Those helpers lived in `validation.ts`, which imports drizzle, so anything reaching them became server-only and the grid hand-rolled its own copy. Extracting them to `select-options.ts` lets both sides share one implementation, so the optimistic cache can no longer disagree with what gets persisted. - The two icon maps become one registry read. It also fixes a live inconsistency it surfaced: currency got a numeric keypad in the grid's inline editor but a plain text field in the row modal. Behaviour-neutral by construction: all 1046 tests in the touched areas pass unchanged, with no test edits. * test(tables): guard the column-type registry's invariants Property tests for the registry itself rather than any one type: entries key by their own id, COLUMN_TYPES stays derived, an unknown type degrades to string instead of throwing, only opaque-id types restrict filter operators, only configuration-free types are CSV-inferable, and every type that can reject a draft has a message to show. Plus the metadata-ownership matrix, which pins the generic ownership check to the same answers the hardcoded per-type rules gave. These target the registry's silent-failure class — a wrong jsonbCast or a stray operator whitelist used to be invisible until a filter failed in SQL. Both are verified to fail under mutation. * fix(tables): read exponent-form amounts and reject bad currency PATCHes up front Two P1s from review. Scientific notation lost magnitude. `String()` emits exponent form past 1e21, so a stored amount round-trips through the editor as `1e+21` — and the sanitizer treated the `e` as decoration to strip, reading it back as 121. An untouched cell silently lost 19 orders of magnitude on its next edit. Exponent form is now taken at face value, but only when the string is wholly a numeric literal once symbols are removed, so `12 EUR` (whose `E` survives the strip) still parses through the separator path. A failed currency PATCH left a partial rename. `renameColumn` commits in its own transaction before the currency write, so a `currencyCode` the service would reject — an unsupported code, or any code on a non-currency column — errored only after the rename had stuck. Both are now caught before the first write, matching the guard the route already applies to unique-on-select for exactly this reason. * refactor(tables): finish the registry migration and drop the dead config Audit pass over every consumer, closing the gaps the first cut left. Functional gap: the copilot agent had no currency support at all — it could create a currency column with no code and could never re-denominate one. `add_column` and `update_column` now accept `currencyCode`, with the same up-front validation and the same code-only routing as the HTTP routes. Config that consumers were still restating, now read from the registry: - `supportsUnique` replaces the unique-on-select guard stated in three places (service, both column routes, the copilot tool). - `editor === 'toggle'` replaces seven `type === 'boolean'` checks in the grid and expanded popover, all of which meant the same thing. - `defaultMetadata` replaces the per-type stamping in `addTableColumn` and `updateColumnType`. - `sampleValue` replaces the per-type example values in the LLM prompt scaffolding. - `storesOpaqueIds` replaces the select filter in the find-row matcher. Dead config removed: `getTypeBadgeVariant` had zero callers (already dead on staging), and it was the only reader of `badgeVariant` — so the field, its union, and all seven values went with it. `inferFromCsv` was read by nothing but a comment; CSV inference is an ordered heuristic a boolean cannot express, so it is gone too and `InferredCsvColumnType` is no longer exported. Fixes a latent crash found on the way: unique-constraint checking normalized a cell keyed on its RUNTIME type but reconstructed it keyed on the column's DECLARED type, so a unique `date` column stored a bare `2024-01-01` and then threw `SyntaxError` parsing it back. Both directions now go through JSON unconditionally. Pre-existing, unrelated to currency. Adds the `/add-column-type` skill and a Tables section in CLAUDE.md/AGENTS.md pointing at it, so the next type is one file plus two registry entries. * fix(tables): run the column PATCH guards ahead of the rename, not after it Greptile was right and my previous reply was wrong. The guards were added in the right shape but the wrong place — below `renameColumn`, which is the first write and commits in its own transaction. A PATCH combining a rename with an invalid currency therefore still committed the rename and then returned 400, exactly the counterexample reported. Moved the column lookup and all three pre-flight guards above every write. This also closes the same latent hole for the pre-existing unique-on-select guard, which sat in the same position. Adds route tests that assert `renameColumn` was never called on each rejection path, and that a valid combined rename + currency change still targets the new name. Verified to fail against the previous ordering. * fix(tables): make the retype gate and the write path share one parser A simplify pass over the registry found two real defects and several places the abstraction was being worked around. Silent data loss on conversion. `isCompatibleWith` was hand-written per type and had already drifted from `coerce`, despite the interface promising they could not: `boolean` accepted '1'/'0'/0/1 in the gate but only 'true'/'false' in the write path, so converting a column holding "1" reported zero incompatible rows and then nulled every one of them. `date` drifted the other way. `isCompatibleWith` is now optional and defaults to `coerce(...).ok`, so the two are the same code; only `select` overrides, because its rules are about the column (cleared-vs-required, cardinality) not the value. `isColumnType` used `in`, which matches inherited keys — `isColumnType('toString')` was true and `columnTypeById('toString')` returned `Function.prototype.toString`, which the validator would then call `.validateDefinition()` on. Now `Object.hasOwn`. `defaultMetadata` only ran on the currency arm of a retype, so a future type would get its defaults on create but silently not on conversion. It now runs for every non-select target, carrying forward only metadata the TARGET type declares it owns — a currency→text conversion no longer strands a currencyCode. The index doc claimed the registry is kept out of the `@/lib/table` barrel so 44 server modules don't pull `@sim/emcn/icons`. That was false: `constants.ts` re-exported `COLUMN_TYPES` from the icon-carrying `registry.ts`, and the barrel re-exports `constants`. `COLUMN_TYPES` now lives in the icon-free `types.ts`; verified with an import tracer that both are icon-free again. Also: 5 no-op `validateDefinition`s and 4 duplicated formatters collapsed into registry defaults; `CURRENCY_OPTIONS` was an eager module-load IIFE costing ~8ms of ICU work on every table API route for a list only the config sidebar reads, now built on first call; and the skill's validation grep claimed 'should return nothing' when it returns 8 legitimate hits — it now explains how to tell a leak from a genuine special case. * fix(tables): reject a non-leading sign so dates don't parse as amounts Found by Cursor Bugbot. `parseCurrencyInput` dropped every `-` as decoration, so an ISO date's hyphens vanished and its digit groups joined: `2024-01-01` read as 20240101. With the gate now sharing the write path's parser, a date → currency conversion reported zero incompatible rows and silently turned every cell into a huge number. A sign is only meaningful at the front; an interior one means the string is not a single amount. Leading signs, accounting parentheses, symbols, ISO codes, grouping separators, and exponent form all still parse — covered by the existing cases plus new ones, verified to fail without the fix. * fix(tables): use getErrorMessage in the columns route test mock `check:utils` bans the inline `e instanceof Error ? e.message : fallback` form; the mock for `rootErrorMessage` used it. * fix(tables): rename the column last so a failed write leaves it untouched Greptile's remaining concern: the pre-flight guards read a schema snapshot, so a column-type change landing concurrently can still make a later write fail — and with the rename running first, that failure returned an error with the rename already committed. Guards cannot close that window; each write is its own locked transaction and only the write itself sees the authoritative state. Ordering can. The rename is the one write that is purely cosmetic, so it now runs last: a failed typed write leaves the column entirely untouched, and a failed rename leaves the typed change applied under the old name — the recoverable half. The typed writes target the column's current name, since no rename has happened yet. Tests cover both directions: a typed write rejected mid-flight must not rename, and a successful one must rename strictly after. Verified to fail under the previous ordering. * fix(tables): write back coerced values on every conversion Round 4 findings, all real. A conversion is allowed exactly when the target type's `coerce` accepts the value — and `coerce` frequently TRANSFORMS it. Only `select` and `currency` wrote the transformed value back, so a conversion to any other transforming type left the cell holding its old bytes under the new type. Converting a number column to `date` accepted epoch values, stored them unchanged, and then `(data->>'col')::timestamptz` failed on EVERY query against that column. I opened this myself by defaulting `isCompatibleWith` to `coerce(...).ok`. Fixed at the class rather than the instance: the compatibility scan now records whatever `coerce` produced whenever it differs from what is stored, and one generic write-back applies it. That subsumes the currency-specific migration entirely, so it and its helpers are gone. `select` keeps its own id↔name migrations, which are not coerce-expressible in the outbound direction. The post-conversion column definition is built once, before the scan, so the coercion reads the same metadata the stored value is later validated against. Exponent parsing was ambiguous when followed by text: `1e5 EUR` read as 15. An `e` with a digit on both sides is an exponent marker, so if the string is not a clean numeric literal it is refused rather than guessed — the digit on both sides is what keeps the `E` inside `12 EUR` parsing normally. A failed rename could still leave a typed change committed. The one rename failure a caller can cause — a name already taken — is now rejected up front, leaving only the concurrent-collision race, which no pre-flight check can close without spanning all writes in one transaction. * fix(tables): stop a blank cell blocking an optional type conversion Found by Cursor Bugbot. `''` is incompatible with every numeric type, and the compatibility scan counted it as a hard blocker regardless of whether the target was optional — so a text column with a single empty cell could not be converted to a number at all, and the error said 'to a required ...' either way. An unreadable-but-empty cell is not a conversion failure. The write path already turns an unreadable value into null on an optional column, so the conversion now does the same and records null for it. A required target still reports it, which the existing guard above already does with the message that actually fits. Also pins the two intentional divergences from the pre-registry behavior. A differential run of the registry against the pre-refactor implementations (55 values x 7 column shapes) found ZERO coercion differences and exactly two compatibility differences, both deliberate: boolean now rejects the '1'/'0' conversions the old gate accepted and then nulled, and date now accepts the epoch numbers its write path always accepted. Tests pin both so neither can be silently reverted or widened. * fix(tables): refuse conversions that would invent or destroy values Final adversarial scan found two data-corrupting conversions, both opened by defaulting the retype gate to the write path's parser. number → date destroyed every value. `date.coerce` reads a number as epoch milliseconds, which is right for one deliberate write and catastrophic applied to a whole column: 1, 5, 42 became three timestamps in January 1970, and a Unix-seconds column landed in 1970 rather than the year it meant. Irreversible. `date` now overrides the gate to reject numbers, restoring the pre-refactor behavior, and the contract states the rule the override obeys: a gate may be STRICTER than `coerce`, never looser. Stricter refuses a bulk conversion while single writes still work; looser is the direction that corrupts. string → currency invented values. The parser stripped every non-digit and joined what was left, so `01/02/2024` read as 1022024, `Room 101` as 101, and `0.1.2` as 12 — a column of SKUs or phone numbers converted with zero reported incompatibilities. What remains after removing symbols, spacing and an ISO code must now be only digits and separators, and grouping must be well-formed (a first group of 1-3 digits, the rest exactly 3). Every legitimate form still parses, including all the locale variants. Also generifies the last three metadata leaks: `buildConvertedColumn` strips and carries back by iterating the key list rather than naming keys (naming them meant a future type's metadata rode onto a target that rejects it, failing that column's validation on every later write), `normalizeColumn` forwards metadata through a shared `typeMetadataOf`, and `filterOperatorsFor` moved onto the definition — it was a per-type branch inside the registry's own accessor, the one thing the registry exists to forbid. Skill corrected: it claimed COLUMN_TYPES derives from the registry (backwards), promised exactly two compile errors (four once a type owns metadata), used a grep that missed half the real branches, and never mentioned `import.ts`'s second coercion path, whose silent default arm is the costliest miss available. Differential re-run vs the pre-refactor implementations: 0 coercion differences, 1 intentional compatibility difference (boolean no longer accepts the 0/1 conversions the old gate accepted and then nulled). * fix(tables): let the row modal accept formatted amounts again Found by Cursor Bugbot. I unified the row modal's input type with the grid's `inputMode` last round, but in the wrong direction: mapping `inputMode: 'decimal'` to `<input type="number">` made the modal reject $1,234.56, 1.234,56 and (12.00) — the exact formats `parseCurrencyInput` exists to accept, and which the grid's inline editor takes fine. A native number input and a numeric keypad are different things. Types whose parser accepts formatted text now say so, and get a text field with `inputMode='decimal'` — the shape the grid already uses. A plain number keeps the native input, its spinner, and its validation. * fix(tables): fold a rename into the write it accompanies Closes the last partial-update window, properly rather than by pre-checking around it. A rename is metadata-only — `renameColumn`'s own comment says so: rows, metadata, and workflow-group refs all key on the stable column id, so it is a pure schema write. Nothing forced it to be its own transaction. Running it separately is what created the window: whichever half committed first survived a failure in the other, and no pre-flight guard can close a concurrent collision because only the write itself sees authoritative state. The four column writes now accept an optional `newName` and apply it through one shared `applyPendingRename`, which validates the name shape and checks the collision against the very schema snapshot that write is landing in. A combined request rides the rename on whichever write runs last, so both halves commit together or neither does — a concurrent claim on the name now aborts the whole transaction instead of leaving the other change applied. The routes also address every write by the column's stable id rather than its name, so folding a rename into one write cannot break the next one's lookup. A rename with nothing to ride on still runs standalone. What remains partial is a type write followed by a failing constraints write — two independently locked transactions, pre-existing, and untouched by this PR. * fix(tables): migrate scalar cells when converting a column to select Found by Cursor Bugbot. `resolveSelectOptionId` stringifies a number or boolean before matching, so a `number` column whose values equal option NAMES passes the compatibility gate — but `migrateCellsToSelectIds` only rewrote JSONB `string` and `array` cells. Those cells stayed raw numbers inside a select column, where they render as nothing and fail option membership on the next write. `data->>key` yields the text form for every scalar, so the existing lookup already worked; the predicate was simply too narrow. Widened to cover `number` and `boolean`. The outbound migration is unchanged — cells leaving a select column are option ids, always strings or arrays. Pre-existing on staging (both the resolver's scalar handling and the migration SQL predate this branch), but it lives in a file this PR creates. Tests pin the resolver behavior the predicate depends on, so narrowing either one without the other now fails. * fix(tables): validate a retype's unique against the values it writes Validated the last partial-update seam with a focused investigation rather than assuming. The answer was split. `required` is already safe: `updateColumnType` runs the same `countEmptyCells` against the constraint the request is about to set, which is why that check exists. `unique` was not, and the reachable case commits the unrecoverable half. A text column holding "5" and "5.0", PATCHed with {type: number, unique: true}: the conversion succeeds and coerces both to 5, then the separate constraint write finds duplicates and 400s — with the column already numeric and "5.0" irreversibly rewritten. A pre-scan of the raw text finds nothing; the conversion is what manufactures the duplicate. The retype now carries `unique` and checks it after the write-back, against the values it just wrote. Constraint changes on a workflow-output column were the same shape — rejected by the constraint write, after a type change had committed. Now rejected in the route's pre-flight block, before any write. The duplicate scan is extracted and shared between both paths for the same reason `countEmptyCells` is: two copies of one rule is the drift that produced the original required-check bug. Deliberately NOT merging `updateColumnType` and `updateColumnConstraints`. They assert different lock levels (destructive vs schema-only) and only the retype needs the full row scan, so merging would either force a constraints-only toggle to materialize every row or reintroduce the branching it was meant to remove. With both reachable failures pre-validated, what remains at the seam is concurrent races no in-process check can close. * fix(tables): don't drop a rename when the write it rides on no-ops Found by Cursor Bugbot — a bug I introduced folding the rename in. `updateColumnCurrency` returns early when the code is unchanged, and that return sat ahead of the rename, so PATCH {name, currencyCode} with the column's current code answered 200 with the rename silently discarded. Both early returns now treat a pending rename as work: the currency path only no-ops when the code is unchanged AND no rename is riding along, and the retype path applies a rename-only write when the type is unchanged. `applyPendingRename` signals "nothing to do" by returning the same reference, which is what lets both detect it cleanly. Also extracts `persistColumns` — five sites were repeating the same schema-write-and-return. * fix(tables): make a combined column PATCH a single transaction Finishes the fold-in rather than pre-validating around the seam. A retype now APPLIES the constraints it already validates against — it checks empty cells for `required` and post-conversion duplicates for `unique`, so it was doing the work without persisting the result — and the route skips the separate constraint write when the type changed. A request combining a rename, a retype and constraint changes is now one locked transaction: no half of it can commit while another fails. The separate constraint write remains for requests that do not change type, which is the only case that still needs it. Deliberately still NOT merging the two service functions. They assert different lock levels (destructive vs schema-only) and only the retype needs the full row scan into memory, so a merged function would force a constraints-only toggle to materialize every row or reintroduce the branching it was meant to remove. Folding the payload in gets atomicity without either cost. * fix(tables): reject a flattened list as an amount; fold constraints into every typed write Two findings from round 11. Multi-select converted to nonsense amounts. `selectValueForConversion` flattens a multi cell to its comma-joined option names, and the parser read that as a formatted number: options 12 and 34 became 12.34, and 100 and 200 became 100200. No real amount puts whitespace after a separator, but a delimited list does — so a separator followed by whitespace is now refused. Every legitimate form still parses, including space-grouped locales. Combined options-or-currency + constraints could still commit partially. Those two writes now carry constraints the same way the retype does, through one shared `applyConstraints` that validates (workflow-output, empty cells for required, supportsUnique and duplicates for unique) and applies them. The separate constraint write now runs only when no typed write does. Three copies of those rules is the drift that produced the original required-check bug, so they live in one place. * fix(tables): validate constraints after the migrations that rewrite cells Self-caught while reviewing my own previous commit, which introduced both. `updateColumnOptions` ran the shared `applyConstraints` BEFORE its cell migrations. Those migrations rewrite stored values — a single<->multi toggle changes the shape, removing an option clears cells — so a `unique` scan read the pre-migration values, passed, and the rewrite could then produce the duplicates the scan was meant to prevent. Moved to after the migrations, which is where `updateColumnType` already had it. The same commit also left the options path running `required`'s empty-cell check twice: once in the shared helper and once in its original inline block, whose comment still described a separate constraint write that no longer runs. Removed the duplicate — one query, one rule, which is the whole point of the shared helper. Also routes the options path through `persistColumns` like the others. * fix(tables): stop inventing amounts from identifiers; fix the copilot retype Adversarial pass over the final state, seven real findings. Two destroyed data. The copilot `update_column` still used the two-transaction pattern the HTTP routes were fixed for: `unique` was never forwarded to the typed write, so a retype+unique committed the conversion and then failed the constraint — the same irrecoverable half. It now rides the typed write, and the separate constraint write only runs when no typed write did. And the parser's three-letter strip removed ANY three letters, not an ISO code: `SKU400` parsed as 400, `ABC1234` as 1234. Converting a column of part numbers to currency rewrote every cell with an invented value — while the comment two lines above claimed a SKU was exactly what it prevented. The rule is now that a letter touching a digit means identifier, not amount; a currency marker is always separated by a space or a symbol. That same change fixed a class the review surfaced: the pinned currencies could not parse their own conventional notation. `R$ 1.234,56`, `1 234,56 kr`, `1234,56 zł`, `CHF 1’234.56` and Indian lakh grouping (`₹12,34,567.89`) all work now — these are what Intl emits, so a paste from a spreadsheet was being rejected. `updateColumnConstraints` was a fourth copy of the constraint rules the shared helper exists to unify, and had already drifted: it hardcoded `type === 'select'` where the helper asks the registry, so a future type declaring `supportsUnique: false` would have been ignored on that path. It now uses the helper. `updateColumnType`'s unchanged-type early return silently discarded every field except the rename. Callers gate on the type changing, but from a read taken before the lock — so a concurrent change could land there with real work pending and answer success. It now throws. Also: `UpdateColumnCurrencyData` was missing `required`, which only compiled because the routes pass it through a spread; a missing column returns 404 instead of a 400 reading "of type undefined"; and the comments describing the old two-transaction architecture are gone. Verified NOT a bug: CSV export of a currency column writes the raw number, so export/import round-trips losslessly. * fix(tables): read the negative and RTL forms Intl actually emits An Intl sweep across 24 locales found two forms the parser rejected, both from an ordinary spreadsheet paste. `Intl` emits U+2212 MINUS SIGN rather than the ASCII hyphen for negatives in several locales, so `−12,50 kr` read as null instead of -12.5. And it wraps RTL-locale output in invisible bidi control marks, so `1,234.56 ₪` carried characters that are not part of the amount. Both are now normalized away. 24 locales x 6 amounts now round-trip, up from 99/100 when the sweep started — and the test generates them from `Intl` rather than listing them by hand, so a parser change cannot quietly regress a locale nobody remembered to write down. Locales that format with their own numeral systems (Arabic-Indic) are still rejected, and now say so in the docstring. That is a safe failure — null rather than a wrong value — and supporting them is a wider decision than this type, since it would also touch `number`, display, and sorting.
…, and enumerate them in CI (#6109) * fix(desktop): clear the traffic lights on every auth-shell surface Only /login reserved the macOS traffic-light lane, so signup drew its logo underneath the lights — and so did reset-password, sso, verify, the CLI auth handoff, and the invite pages. The pre-paint script marks the lane on every desktop route, so any surface that did not reserve it overlapped. Ownership moves to `AuthShell`, which is the single source of truth for the frame all of these wear, and it now reserves unconditionally. Per-route gating was the wrong shape rather than merely incomplete: `/invite/[id]` is a dynamic segment, so no route list could have covered it. `supportsDesktopTitleBar` therefore drops its pathname argument — the caller mounting the controller is the signal, and only `AuthShell` mounts it. Workspace routes never render it and keep their existing `WorkspaceChrome`-owned listener, so the two never contend for the attribute. Off the desktop shell `--desktop-title-bar-height` is `0px`, so the reservation and the drag strip collapse to nothing and `.desktop-title-bar-page` is exactly the `min-h-screen` these surfaces had before — web is unchanged. With the prop gone the client auth layout was a bare passthrough, so the route layout renders the shell directly and the passthrough is deleted. Measured in the Electron renderer over CDP across /signup, /login, /reset-password and /cli/auth: lane 40px, logo top 56px, zero overflow on each. /invite/[id] redirects to login when signed out and was not measured directly. The surface audit gains a `stripComments` helper that every negative assertion runs through. These files document the shapes they avoid, so a bare `not.toContain` was matching the prose explaining the fix and failing on correct code. * fix(desktop): cover the remaining traffic-light overlaps, and enumerate them in CI Fixing signup by hand would have been the fourth time this bug was found by a person hitting it. The audit now enumerates instead of listing what to inspect: it walks every `.tsx` outside workspace chrome, flags each full-viewport root, and fails unless that root either composes `.desktop-title-bar-page` or appears in an allowlist with a written reason. A brand-new page that fills the viewport fails on arrival — verified by adding one, and by reverting each fix below. Running it found three more surfaces already overlapping: - `/oauth-error` is Better Auth's `onAPIError.errorURL`, which is precisely where desktop OAuth failures land, so the one page a user sees when sign-in breaks drew its content under the lights. - `/f/[token]` public file view, same shell family, same origin, reachable in the window. - The signup and reset-password Suspense fallbacks are viewport-tall *inside* the lane-reserving shell, so the page overflowed by the lane while the split chunk loaded. A placeholder needs no viewport height. Four surfaces are allowlisted with reasons: the two landing shells (the desktop shell boots to /login or a workspace and has no path to marketing routes), the dev-only playground, and the embedded resume interface. Measured over CDP: /oauth-error reserves 40px with zero overflow. * fix(desktop): cover the shells behind the allowlist, and make the guard fail Greptile was right on both counts, and the first one is worse than reported. `LogoShell` was allowlisted as "marketing chrome, not reachable in the desktop shell". That claim was simply false: it is the frame for `not-found`, the interfaces shell, the desktop handoff shell, and the public-file access gates — so the password, email, and SSO gates for `/f/[token]` all still drew under the traffic lights. The allowlist existed to make risk visible and instead hid four surfaces behind one unverified sentence. It now carries two entries, both checked: the landing shell (every consumer lives under `app/(landing)/`) and the playground (calls `notFound()` unless `NEXT_PUBLIC_ENABLE_PLAYGROUND` is set). The lane's two halves also travelled separately, so `/oauth-error` and the public-file view reserved the space without the drag strip — clearing the lights but leaving the window with no title bar on those pages. `DesktopTitleBarLane` now ships both together and the audit enforces the pairing. Both new checks were unfailable when first written, and mutation testing is the only reason that surfaced: - the pairing check matched `DesktopTitleBarLane` anywhere in the file, so the import line satisfied it after the JSX was deleted; - the coverage check matched `LogoShell` anywhere, so a shell's own definition file self-certified as covered. Both now match JSX usage (`/<LogoShell\b/`). Reverting either fix, and deleting either half of the lane, now fails. Measured over CDP: /oauth-error and the LogoShell-based 404 each reserve 40px, carry the drag strip, and overflow by zero, with the logo at 56px. * test(desktop): strip comments at read time so positive assertions can fail Cursor caught the mirror of a trap this file already documents. `stripComments` was applied to negative assertions only, so a positive like `toContain('desktop-title-bar-page')` still ran on raw source — and `AuthShell`'s TSDoc names that class, so deleting it from the markup left the assertion passing on a broken lane reservation. Stripping now happens in `read`, so every audit constant is comment-free and no assertion in either direction can match prose. Verified: deleting the class from the markup while leaving the TSDoc intact now fails two tests, where it previously failed none. * fix(desktop): audit workspace routes too, and cover the three that bypass the chrome Greptile was right that the blanket `workspace/` exclusion was load-bearing in the wrong direction. It assumed every workspace route reaches the lane through `WorkspaceChrome`, and three do not: the workspace landing route, the access-denied early return, and the workflow error boundary. The exclusion is gone, so workspace files are audited like everything else and `WorkspaceChrome` joins the lane-aware shells — the normal route passes through its layout, and the three exceptions had to be dealt with on their merits: - the landing route (status card and spinner) and access-denied now reserve the lane; - the error boundary is allowlisted, verified: it renders `<Sidebar>`, which already owns the workspace lane and its drag region, so padding that root would double it. Their content was centred, so the lights were never covering text — the real gap was that none of them rendered a drag strip, leaving the window immovable on those screens. The guard's granularity is per file, not per JSX root: `workspace/page.tsx` holds two full-viewport roots and still passes if only one reserves the lane. Verified by mutation and documented rather than papered over — catching it needs an AST pass, and the check's job is to stop a whole surface being forgotten, which is how every instance of this bug has actually shipped. * fix(desktop): teach the audit about nesting, and stop the resume skeleton double-reserving Four findings, all correct, and the first is a bug this PR introduced. The resume loading skeleton reserved the lane while already rendering inside `(interfaces)/layout.tsx` -> `InterfacesShell` -> `LogoShell`, which reserves it too. Two lots of padding, two drag strips, two controllers. It came from adding the lane there before `LogoShell` became lane-aware and never reconciling the two. The skeleton now reserves nothing and is no longer viewport-tall either — nesting a viewport-tall root inside a viewport-tall shell overflowed even before this PR. The public-file header pinned `sticky top-0`, which parks it inside the reserved lane and under the lights. It now sticks below the lane, inert on web where the variable is `0px`. Both audit gaps were real: - The check was file-local, so it could not see the doubling above. It now resolves ancestor layouts: a root counts as covered when it reserves OR sits inside a layout that does, and reserving on both levels is its own failure. That also stops the check demanding a second reservation from chat and the workspace overlays, which correctly inherit theirs. - Detection only matched `min-h-screen`/`h-screen`, so `fixed inset-0` roots never entered it. Now included. With nesting understood, that addition resolved to a single genuinely uncovered file rather than the ten it flagged beforehand. Two allowlist entries added, both reasoned rather than assumed: the landing prefix (dozens of files, one justification), and the desktop update gate — it centres its content, and under `hiddenInset` macOS draws the lights above the web contents, so web UI cannot cover them. This bug class is app chrome sitting under the lights, never the reverse. Verified by mutation: reintroducing the double reservation fails the new check. * test(desktop): do not credit inherited coverage across a layout's early return Ancestor resolution is static, so it credits any file under a layout that mentions a lane-aware shell. That is wrong when the layout returns the surface *instead of* its chrome: `workspace/[workspaceId]/layout.tsx` returns `<WorkspaceAccessDenied />` at the top and only reaches `<WorkspaceChrome>` far below, so at runtime the denied page has no chrome at all. The page does reserve the lane today, but a regression would have read as inherited and passed. `SessionExpired` is deliberately not listed: it renders as a sibling within the chrome tree, so its inherited coverage is real. The distinction is which side of the early return the surface sits on, not which directory it lives in. Verified by reverting the access-denied page exactly as described — it now fails. * test(desktop): count the lane class itself as a viewport claim Cursor caught the audit failing to watch exactly the files this PR converted. Detection keyed on `min-h-screen`/`h-screen`/`fixed inset-0`, but converting a surface to `.desktop-title-bar-page` removes those tokens — the class supplies `min-height: 100vh` itself. So `/oauth-error`, the public-file view and `AuthShell` dropped out of the check entirely, and a nested class-only reservation could ship green. That also means the doubled-reservation check had never actually fired. The mutation I used to "verify" it removed the lane component as well, so the pairing check caught it and the doubled check was never exercised. It now fires on Cursor's exact scenario: a nested class-only reservation, correctly paired, inside a lane-aware shell. Pulling those files back in exposed a second-order bug: a shell's own definition file sits under the layout that renders it, so ancestor resolution called `AuthShell` nested inside itself. Shell definitions are excluded from inheritance. One limit stays, documented rather than papered over: a root is in scope because of how it claims the viewport, so deleting the reservation outright drops the file from the check. That regression is loud, not silent — the surface stops being full height. Closing it properly means treating every route entry point as a window root, which pulls in seven account/organization/selfhost pages needing individual assessment. Worth doing separately; allowlisting them on assumptions is the mistake that produced the `LogoShell` hole. * fix(desktop): a fixed root escapes ancestor padding — chat drew under the lights Cursor's sharpest catch, and the audit was actively hiding the bug rather than missing it. `position: fixed` resolves against the viewport, not the parent, so a lane-aware shell's `padding-top` never moves it. The chat surfaces sit inside `LogoShell` and still painted at viewport top, under the traffic lights, while the check reported them covered — and adding the correct reservation would then have tripped the nested-reservation check, so the audit pushed toward the wrong answer. Roots matching `fixed inset-0` no longer inherit coverage and are exempt from the doubled check. That reclassified seven surfaces, each decided on evidence: - chat, its loading boundary, the loading state and the voice interface are full-window roots at `z-[100]` with their own top chrome — all now reserve; - the file viewer wraps a full-bleed `<iframe>` whose content starts at viewport top — now reserves; - session-expired centres its content with nothing in the lane, and the search modal's `fixed inset-0` is only its scrim (the panel sits at `top-[15%]`) — both allowlisted with that reasoning rather than an assumption. Verified by reverting chat to a bare `fixed inset-0` inside `LogoShell`: it is flagged now and was called covered before. * fix(desktop): stop the lane controller clobbering a mode another owner set A regression I introduced last round. `DesktopTitleBarController` seeded `inset` unconditionally on mount, before its own `getState()` resolved. That was harmless while only `AuthShell` mounted it — but giving the file viewer a lane put a controller inside workspace for the first time, exactly where this PR's own comments say `WorkspaceChrome` owns the mode. Opening a file during native fullscreen therefore snapped the traffic-light lane back on and jumped the content, and left it wrong permanently if `getState()` rejected, since the rejection is swallowed. It now seeds only when no owner has established a mode. The pre-paint script sets the marker before first paint and `WorkspaceChrome` maintains it, so the unconditional write was never the thing making the lane correct — it was only ever able to make it wrong. Adds a controller test covering the case directly: mount during `fullscreen` with a `getState` that never settles, and the mode survives. Verified it fails against the previous behaviour. * test(desktop): assert the lane on LogoShell directly, not via the sweep Cursor found the one shell the enumeration could go silent on. `LogoShell` sits under the `app/(landing)/` prefix allowlist, and this PR replaced its `min-h-screen` with `desktop-title-bar-page` — so stripping the reservation also strips its last viewport token and the sweep stops watching it. Doubly exempt, and it is the shell behind not-found, the interfaces shell (chat, resume), the desktop handoff and the public-file gates. `AuthShell` already had a dedicated assertion; `LogoShell` now has the same. Verified by stripping its reservation: caught now, silent before. This is the documented enumeration limit made concrete rather than a new class of problem — a root leaves the sweep when it stops claiming the viewport. The general fix is still to treat route entry points as window roots, which pulls in seven unassessed pages and belongs in its own change. * docs(desktop): record why access-denied reserving is not a double reservation Review read `WorkspaceHostProvider` as rendering the denied page inside the chrome. It is the other way round: the provider wraps `<WorkspaceChrome>` in the layout, and its 403 branch returns the denied page instead of its children, so the chrome never mounts. The server-side early return does the same thing higher up. Both paths therefore bypass the chrome and the page's own reservation is the only one. Recording that next to the entry, since the file lives under a workspace path where inherited coverage is the norm and the reading is an easy one to repeat.
…pace (#6023) * feat(custom-blocks): log and bill child runs in the publisher's workspace * fix(custom-blocks): sanitize every boundary failure and classify it for consumers * fix(custom-blocks): surface a cancelled child as cancelled, not a generic failure * fix(custom-blocks): share one large-value id list so nested blocks propagate * fix(custom-blocks): add a durable cancel backstop to the child bridge * fix(tools): carry Sim's own status through the tool-response boundary * fix(custom-blocks): stop forwarding the publisher's personal quota to consumers * fix(custom-blocks): correlate agent-tool runs to the real invoking execution * fix(custom-blocks): plumb the invoking execution id and abort signal to agent tools * fix(agent): forward the execution id through the provider payload * fix(executor): stop adopting an upstream target's HTTP status as our own * fix(custom-blocks): drain child log finalization when the parent is cancelled * fix(custom-blocks): require curated outputs instead of exposing the whole result * fix(custom-blocks): track the whole child run, not just its finalization
* fix(emcn): stop the tab strip scrolling vertically by a pixel The active tab extends one pixel past the strip so it covers the bottom border and reads as joined to the panel below. That pixel came from `-mb-px` on the tab itself, which meant it overflowed the row containing it — and that row is a scroll container, because `overflow-x: auto` computes a visible `overflow-y` to `auto` as well. The strip was therefore scrollable on the y axis by exactly one pixel, which is enough for a trackpad to nudge the tabs out of view. Moving the overlap onto the row keeps the tabs flush inside it, so the scroll container has nothing to scroll, while the row itself still hangs the pixel over the border. The strip is unchanged visually. Measured in a real renderer with the component's own classes: the row was scrollHeight 30 against clientHeight 29, and is now 30 against 30. A first attempt to reproduce it with hand-written inline styles showed no overflow at all — the real class output was needed to see it. Shared by the browser and terminal panels, so both stop scrolling. * test(emcn): pin the invariant, not the class placement Review pointed out the guard read source strings rather than rendered geometry, so it could pass while the overflow returned. Correct, and worth being precise about the compromise: this package's vitest runs in `node` with no browser mode, and the repo's only Playwright harness drives Electron against static HTML fixtures, so nothing here can lay out a React tree against the compiled CSS. Adding that capability is real infrastructure, not a line in this fix. What the guard can do is pin the invariant instead of two particular class strings: no negative bottom margin anywhere inside the scrolling row. That now catches the regression wherever a descendant reintroduces it, rather than only on the tab button it came from — verified against both shapes. The rendered behaviour stays measured rather than asserted, and the numbers are recorded next to the guard.
…iguity (#6111) * fix(tables): refuse scale suffixes and resolve the lone-separator ambiguity A verification pass over the previous commit found that my own fix opened a new hole of the class it closed. Widening the currency marker to 1-3 letters made scale suffixes parse: `1.2 M` read as 1.2, so a column of `1.2 M` / `3.4 M` — an ordinary spreadsheet paste — converted cleanly and rewrote every cell a millionfold too small. Before the widening those were rejected and the data was safe. Now refused explicitly, while `kr` and `zł` still parse despite starting with the same letters. Stripping the marker also newly routed formatted zero-decimal amounts into the lone-separator branch, where a single dot was always decimal: `1.235 ¥` read as 1.235 rather than 1235. That is a wrong number where there used to be a refusal, which is the worse failure. A lone separator followed by three digits is now resolved by two signals — a marker means a formatter produced it, and formatters group; a currency carrying three decimals (KWD, TND) reads them as decimals. `coerce` passes the column's code, so the parser can ask. Bare typed input keeps the decimal reading. Also: the unchanged-type throw fell through to a 500 for every type except currency, whose message happened to contain the word; it now maps to 400. And the last three comments describing the removed two-transaction architecture are gone — the previous commit claimed they were and two survived. Documented, and failing closed rather than guessing: markers written flush against the digits (`Rp12,00`) stay rejected, because a letter touching a digit is the only thing distinguishing a currency marker from a part number, and reading `SKU400` as 400 invents a value where refusing merely inconveniences. * fix(tables): fit the whole column-type list in the New column dropdown Adding currency made the list nine items (295px with its separator and padding), past the shared dropdown's 240px cap — so Select and Workflow sat below a scrollbar. Sized on this dropdown rather than on the shared `DropdownMenuContent`, which every other menu in the app relies on. Also gave the sidebar's type combobox headroom, since it gained the same entry and was within 4px of its own cap. * fix(tables): read a lone separator against the column's currency Review of the previous commit found the marker signal alone was too coarse. The evidence: a currency with decimal places ALWAYS formats with both separators — `1.234,56 €`, `$1,234.56`, `1.234,000 KWD`. Only a zero-decimal currency emits a lone one (`1.235 ¥`). So a marker does not imply grouping; the currency's own decimal places decide, and a three-decimal currency's trailing three digits are decimals however the value arrived. That matters most for CSV import, which passed no currency at all: a KWD column importing `0,500` read as five hundred rather than a half. The column's code is now forwarded, since nothing in a CSV carries a marker to fall back on. A bare typed `1.234` still reads as decimals. * fix(tables): let the separator decide a lone-separator amount, not the marker Greptile blocked on `$1.234` reading as 1234, and offered the alternative of stating the input contract explicitly. Looking for a precedent settled it against me: a spreadsheet with a USD-formatted cell stores a typed `1.234` as 1.234 and displays $1.23. Reinterpreting it as one thousand two hundred thirty-four is a thousandfold surprise, and my defense of it was wrong. The rule now keys on the separator, tempered by what the currency can express: - A dot is the decimal point in the notation most people type, so it stays a decimal — except for a currency with no decimal places, where `1.235 ¥` cannot be a fraction of a yen and is the one lone-separator form a formatter actually emits. - A comma is grouping by convention (`1,500` is fifteen hundred), except for a three-decimal currency, where `0,500` is a half. A currency with one or two decimal places always formats with both separators, so no formatter output is decided here at all. This also drops the `hadMarker` signal entirely — once the separator and the currency answer the question, where the string came from stops mattering. * fix(tables): check conversions against the target column, not a rebuilt stub Bugbot flagged that the conversion gate calls `isValueCompatibleWithType`, which hand-builds a column from loose arguments and never sets `currencyCode` — while the coercion a few lines later reads the real one. The reported impact does not occur. Whether an amount parses does not depend on the currency, only which number it yields, so the gate and the coercion always agree on accept/reject; and the value written back is already coerced against the real column, so no cell is stored wrong. I checked this rather than argued it: across nine currency codes and every separator shape in the parser's repertoire, acceptance never diverged. The stub is still worth removing. It is correct only because of an invariant nobody wrote down, and `convertedColumn` — sitting right there, whose own comment already says it exists so the scan reads the same option set and currency — is what the gate should have used all along. So the gate now takes the target column itself. The loose-argument form moves into the test file as local marshalling, leaving production with only the whole-column shape, which cannot silently drop a key a future type adds. No behavior change, so no test changes: a currency-divergence regression test could not fail today, and pinning an invariant this removes the need for would only block a legitimate future change. * fix(tables): vet a currency marker against an allowlist, not a scale denylist Bugbot found that `SCALE_SUFFIX` misses `mio`, `mrd`, `bio`, `tsd`, `mln` and `md`. Each is one to three letters, so each matched the currency-marker pattern and was stripped: `1,2 mio` read as 1.2, a millionfold too small, which is the same failure `1.2 M` had. Adding the six words would leave the next six. The denylist is the defect — it has to enumerate every magnitude abbreviation in every language, and each one it misses silently rescales a value rather than refusing it. So the check is inverted. A bare letter token is stripped only when it names a currency: an ISO 4217 code the runtime can enumerate, or one of the common non-ISO markers people type (`kr`, `zł`, `Kč`). Anything else is left in place, fails the amount-shape check, and is refused. `12 units` and `12 pcs` now fall out of the same rule the magnitude words do, without naming either. A marker carrying a currency symbol skips the check — no magnitude abbreviation contains one — so `R$`, `CHF`, `1 234,56 kr` are untouched. This rests on ISO codes and magnitude words being disjoint, which is true for all 162 codes the runtime knows, and is now pinned by a test so a future collision fails loudly instead of silently. `SCALE_SUFFIX` survives only for a runtime without `Intl.supportedValuesOf`, which cannot enumerate codes and so keeps the old permissive behaviour; rejecting every letter marker there would break `USD 12.50`. It no longer has to be complete, since every modern runtime rejects by non-recognition. Verified the new tests fail when the vetting is reverted. * fix(tables): group a lone dot only when a zero-decimal amount came formatted Greptile found that a bare `1.235` typed into a JPY column stored 1235. The zero-decimal branch read the dot as grouping without asking where the value came from, so typing, pasting, a tool write, or an import all inflated a thousandfold in silence. The rule was inverted relative to its own evidence. Grouping was justified as "the one lone-separator form a formatter emits" — and a formatter always emits its marker too. The marker is exactly what separates formatter output from someone typing, and I had removed it as redundant two commits ago. So a lone dot groups only for a zero-decimal currency AND only when a marker came with it. `1.235 ¥` and `JPY 1.235` still read as 1235; a bare `1.235` now stores 1.235 and displays `¥1` — wrong in a way the writer can see and correct, rather than a silent thousandfold error. This does not walk back the earlier fix. For a currency with one or two decimal places a formatter always emits BOTH separators, so a lone dot there never came from one and stays a decimal whether or not a marker is present — `$1.234` is still 1.234. The marker only carries information in the zero-decimal case, which is the only case that now consults it. The two overlapping lone-separator tests are replaced by three that each state one rule. Verified the new one fails when the marker requirement is reverted.
…mples (#6115) * feat(library): What Is an AI Agent? Definition, How It Works, and Examples * chore(library): generate cover for the AI agent post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com>
…#6116) * improvement(security): isolated-vm env construction (v8s escape case) * upgrade main node to v24
…mizer (#6117) * perf(tests): mock the tool registry globally, drop the dead deps optimizer The test suite spent far more time importing modules than running them: on the full suite, `import` was 1,399s aggregate against 88s of actual tests. Per-file import cost showed exactly where it came from — lib/core, which touches no registry, runs at 0.09s/file, while every area that reaches the tool registry runs 12x-79x that (blocks 7.12s/file, providers 3.65, executor 3.07, tools 2.05, app/api 1.05). The tool registry is 4,351 entries pulling ~5,907 modules, and almost nothing under test needs the real thing. `@/blocks/registry` was already globally mocked for this reason; this does the same for `@/tools/registry`. Full suite, same commit, same machine: baseline Duration 166.21s (transform 141.80s, import 1399.17s) after Duration 91.56s (transform 61.98s, import 617.08s) 45% faster, import -56%, transform -56%. Identical results either way: 1252 files / 16873 tests pass, plus one failure that reproduces on unmodified staging (cloud-review-tools.test.ts cannot find `rg` from its spawned python3 locally; CI installs ripgrep and it passes there). Four test files genuinely assert tool registration or tool params, so they opt out with `vi.unmock('@/tools/registry')` rather than being weakened or deleted — outlook, azure_devops, and the two search-replace suites. No coverage is lost. Also removes `deps.optimizer.web`, which was dead config: it only applies to client environments (jsdom/happy-dom) and 985 of 1,219 files declare `@vitest-environment node`. Measured both ways to be sure — removing it is a no-op (19.33s -> 19.23s), and switching it to the correct `ssr` side with an include list for the heavy provider SDKs was also a no-op (19.19s). The cost is first-party module graph, which the optimizer does not touch, so the honest move is to delete it rather than leave config that reads as if it does something. Adds the missing `getBlockRegistry` accessor to the existing `@/blocks/registry` mock. #6083 renamed that export and the mock was never updated, so any test reaching those three consumers would have hit "getBlockRegistry is not a function". Nothing exercises them today. Not done, deliberately: `isolate: false` is ~20% faster but leaks state between files and broke two doc-servable tests on the first run. * docs(tests): explain why the search-replace registry opt-outs are load-bearing Review read the missing direct import of @/tools/registry as evidence the vi.unmock calls were no-ops. They are not: the dependency is transitive — the search-replace planner resolves tool input params through real subblock configs — and removing both opt-outs fails 8 tests across the two suites. Comment now says that, so the next reader does not delete them.
* improvement(admin): uupdate defaults for better UX * refactor: address review nits on the admin/invitation lock work Correct the attach lock-order comment. The order matches admin move, and what makes it mandatory is invitation acceptance: it holds `workspace-invitations:<id>` while waiting for the workspace row, so row-locking first (as this did) deadlocks against it. The previous ownership-transfer justification did not hold — that path takes the organization lock before its workspace rows too, so the two agree on order rather than inverting. Drop `cancelInvitation`. The `revokeInvitationAsAdmin` extraction left it with no callers, and an unlocked, unauthorized `status = 'cancelled'` flip sitting next to the fenced replacement is easy to reach for by mistake. Drop the unused `executor` parameters from `hasWorkspaceAdminAccess` and `isOrganizationAdminOrOwner`. No caller threads a transaction through either, and the former goes back to delegating to `checkWorkspaceAccess` instead of re-deriving the same permission itself. Import `chunkArray` from `@sim/utils/helpers` everywhere and remove the re-export from `batch-delete.ts`, so the symbol has one source rather than a non-barrel shim plus the package. Restore the bounded attachability check in `addDashboardOrganizationMember`: scope the query to the selected ids instead of listing every attachable workspace and scanning that array per selection. Resolve the credential-creation permission through `getEffectiveWorkspacePermission` rather than a second copy of the org-admin derivation ladder, so the rule cannot drift from the shared resolver. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(invitations): drop dead code left by the revocation extraction `revokeInvitationWorkspaceGrant` lost its only caller when the DELETE route moved to `revokeInvitationAsAdmin`, leaving a locked wrapper nothing invoked. Remove it and fold its documentation into `revokeInvitationWorkspaceGrantTx`, which direct grants and scoped revocation still call. The grant-revocation test now drives the transactional form directly, so the sibling-grant and final-grant-cancels behaviour it covers stays under test. `isSameOrgMember` has had no caller since before this branch — direct grant resolves membership through `getUserOrganization` inside its own transaction — so it and its tests go too. `getWorkspaceMembership` is no longer imported outside its module now that credential creation reads `getCredentialCreationWorkspaceContext`; make it module-private rather than leave it on the public surface. Co-Authored-By: Claude <noreply@anthropic.com> * refactor: remove four uncalled billing and large-value helpers Each was checked by hand across every file type, including barrel re-exports and string references, rather than taken from a static analyzer. `isUserMemberOfOrganization` has no reference anywhere. `reapplyPaidOrgJoinBillingForExistingMember` only ever ran from two lock-ordering tests. The transaction-enlisted form it delegated to is what the subscription webhooks call and what those tests actually assert on, so they now drive it directly. The assertions are unchanged: the wrapper contributed a transaction, an organization lock and a membership existence check, none of which appear in the recorded operations. `replaceLargeValueReferences` and `replaceLargeValueReferencesWithClient` are both thin wrappers over `replaceLargeValueReferenceKeysWithClient`, which execution logging, human-in-the-loop resume and the trace backfill all still call. The single test covering a wrapper now composes the key collection itself and targets that live helper. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Tables gain safer mutation paths: column PATCH folds renames into the last typed write so partial commits are harder; async delete and other filter consumers downgrade predicates to the legacy shape and reject hybrid group+leaf filters instead of widening deletes. Export streaming stops only when Function execution accepts Desktop (macOS) reserves the traffic-light lane on every full-viewport surface via Docs and agent tooling cover sandboxes, Atlassian service accounts across Jira/JSM/Confluence, and the add-column-type registry workflow. isolated-vm workers must use an explicit env allowlist ( Reviewed by Cursor Bugbot for commit 413784e. Configure here. |
github-actions
Bot
added
the
requires-mothership-merge
|
…g-meta-folderid for GCS CORS (#6121) * fix(uploads): set Content-Type once on presigned PUTs; document x-goog-meta-folderid in the GCS CORS example XMLHttpRequest.setRequestHeader appends on repeated calls (values join with a comma), and GCS is the only provider whose signed uploadHeaders include Content-Type — so single-shot GCS uploads sent 'x, x', which fails V4 signature verification with 403 (headers canonicalize to a comma-separated value that must match what was signed; multipart part PUTs are unaffected since part URLs don't sign Content-Type). The client now sets its default Content-Type only when the server's signed headers don't already carry one, with regression tests for both paths. Also adds x-goog-meta-folderid to the documented GCS CORS responseHeader list — workspace uploads now sign a folderId metadata header, and GCS CORS matches preflight request headers against that list exactly (no wildcards), so the missing entry blocked browser uploads into folders. * chore(uploads): drop inline comment
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
4 participants
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.
Uh oh!
There was an error while loading. Please reload this page.