mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
030c4e2a5eec430ea1dbfa3d30502a9819657e1b
4266
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Waleed
|
030c4e2a5e |
refactor(auth): extract connector definitions out of auth.ts (#6203)
auth.ts had grown to 3,927 lines, of which ~2,340 were the genericOAuth
connector list — the OAuth apps a workspace connects tools to, as distinct from
the handful of providers used to sign in to Sim. Adding a connector meant
editing the same file that configures sessions, database hooks and Stripe.
Moves that list to lib/auth/connectors/providers.ts behind
buildConnectorProviders(), and relocates getMicrosoftUserInfoFromIdToken to
lib/oauth/microsoft.ts alongside the three Microsoft helpers it already depends
on. auth.ts drops to 1,489 lines and reads as auth configuration again.
Pure move, verified mechanically: the connector array is token-identical after
stripping whitespace, and all 179 template literals emit byte-identical strings
(the one apparent diff was reindentation inside a ${} expression, not text).
Behavior, evaluation order and log scopes are unchanged; the array is still
built once, when betterAuth() runs.
The explicit GenericOAuthConfig[] return type is required, not cosmetic —
inline, the entries were contextually typed by the config property. Without it
prompt: 'consent' widens to string and every getUserInfo parameter becomes
implicitly any.
|
||
|
Waleed
|
14d9542f2a |
fix(credentials): capture the correct provider identity on connect and rotate (#6201)
* fix(credentials): capture the correct provider identity on connect and rotate Attio OAuth recorded an arbitrary workspace member instead of the authorizing user, so two members connecting under one Sim user collapsed into a single account row via the stale-sibling dedupe. Notion read `profile.person.email`, which never exists on a bot token. Synthetic connector emails were minted on live third-party domains. Google service-account rotation left the credential labeled with the old key's client_email and skipped audit metadata entirely. Box and Salesforce identity lookups failed silently with no logger in either file. Service-account principals are now a single ServiceAccountPrincipal union (user / tenant / lookup_failed / null) mirrored centrally into both audit and stored metadata, so a principal can no longer be captured and forgotten, and "which account is this credential?" is answerable from SQL. * fix(credentials): keep the provider-reported name when identity lookup degrades Box and Salesforce returned early on a missing user id, discarding a `name` or `login` the response did carry and relabeling the credential to the enterprise or host fallback. Only the principal should degrade; the human label still beats an id-derived string. Also notes the Salesforce `openid` scope in the connect help text. The client credentials minter sends no scope parameter — effective scopes come from the customer's Connected App — so without `openid` the userinfo lookup can 403 and the run-as user silently never reaches the audit record. |
||
|
Waleed
|
786c854e04 |
improvement(settings): consolidate resource UI onto shared primitives (#6202)
* improvement(settings): consolidate resource UI onto shared primitives Sandboxes, MCP tools, and MCP servers each modeled their list rows and detail headers differently. Collapse them — and the surfaces they drifted from — onto one set of primitives. SettingsResourceRow now owns the row entirely: tile, title/subtitle tokens, padding and bleed, hover band, hit area, focus ring, and the one navigation chevron. Adds onClick/href (a stretched overlay, so interactive trailing controls keep their clicks), navigable, iconVariant='custom', and a badge slot for decoration that must not swallow row clicks. Rows that open a detail page get a chevron and a whole-row click; flat records keep the "..." menu. Delete moves to the detail header as a destructive chip behind a confirm modal — sandboxes previously deleted with no confirmation at all. Also folds in SettingsField (new), SettingsEmptyState tone='error', RESOURCE_LIST_STACK/GRID, RESOURCE_TILE_PLAIN, and a shared MemberAvatar; deletes DetailIconTile (byte-identical to ResourceTile); and standardizes on the emcn ArrowRight, which is a visibly different glyph from lucide's. * fix(settings): correct row bleed, delete-confirm binding, and avatar seeding Follow-up from review of the consolidation. The sandbox delete confirmation was boolean-only state. Browser Back unmounts the modal without closing it, so opening another sandbox re-opened it already confirmed — against the wrong sandbox. Reset it where the draft guard already handles the same history hazard. RESOURCE_LIST_GRID kept an 8px column gap after the bleed moved onto the row, so neighbouring cards overlapped by exactly the gutter and the right-hand card's stretched hit area won it: clicking between two cards opened the wrong one. Budget the gap for the bleed instead. Also: restore the `group` the template-icon hover outlines depend on; add a `flush` row for headings and overflow boxes; seed MemberAvatar identically on every surface; keep the MCP transport label visible in all row states; give the Delete chip a stable id so it doesn't remount mid-delete; and stop an empty subtitle rendering a phantom line. Docs: widen the rule's path globs to the surfaces it now governs, drop the `text-[14px]` example that contradicted the token rule, and add a Mode C for migrating rows onto the primitive. * revert(settings): keep the dense member roster avatar Consolidating the Teammates/Organization avatar onto the credential member row's was a redesign, not a deduplication. The two encode different things: the roster is a dense list keyed on email where the avatar is a 14px neutral marker, while a member management row carries a name, an email, and a role control and earns a 36px hashed avatar. Merging them made those rows ~70% taller, gave every workspace in a permission group a colour-hashed "avatar" seeded on its name, and cut the Add Members picker from ~7 visible rows to ~5 inside its fixed-height box. Restores both avatars and the containers that owned their bleed, and records in the rule why they stay separate. Keeps only the unrelated fix in that area: the picker row was the one settings row rounded at `sm`. * fix(settings): wire flush, drop the lone skeleton, close review gaps Final review round. Two fixes the previous commit claimed but did not land: `CredentialDetailHeading` never passed `flush`, so every credential detail heading wore list-row padding, and the empty-subtitle guard was never applied. Both were scripted replacements that silently no-op'd. RESOURCE_LIST_GRID also dropped to one column 32px earlier than the grid it replaced — `auto-fit` measures tracks, not margin boxes, so widening the gap for the rows' bleed moved the breakpoint. Track minimum now budgets for it. Removes the BYOK skeleton rather than maintaining a second copy of the row: it was the only skeleton in settings, and it had already desynced from the row it imitates. Its peers render nothing while loading. Also: unify the glyph-tile treatment across MCP/sandboxes/workflow-MCP with custom tools; move decoration out of `trailing` in verified-domains and recently-deleted; convert the last hand-rolled row and empty states in workflow-MCP, api-keys, copilot and group-detail; give copilot's delete the same `...` affordance as api-keys; announce the row description via aria-describedby, which the stretched overlay had silenced; and let SettingsField render its own value so callers stop restating type tokens. Docs: correct claims that predate this PR — `aside` does not exist, the navigation source of truth is under components/, beforeunload mounts in the layouts, `getSettingsSectionMeta` takes two args — and stop asserting a literal-pixel grep returns zero when display type legitimately uses it. |
||
|
Waleed
|
2ccda18f3e |
test(files): collaborative agent-streaming coverage (two-writer, multi-editor, undo, persist round-trip) (#6199)
* test(files): two-writer concurrent-editing regression coverage for agent streaming * test(files): multi-editor + undo + persist round-trip + late-joiner streaming integration coverage * test(files): make full-rewrite two-writer test actually exercise the concurrent peer edit * test(files): assert every peer edit lands (no false-green from a no-op peerInsertNear) |
||
|
Waleed
|
5460133e07 |
fix(files): collapse blank-line runs to markdown standard (no empty-paragraph explosion / reflow) (#6198)
parseMarkdownToDoc now strips ALL top-level empty paragraphs (leading, interior, trailing), not just trailing. A run of blank lines between blocks is insignificant in markdown (CommonMark collapses it), but @tiptap/markdown reconstructs one empty paragraph per blank line — which made the mounted editor render vertical gaps that exist nowhere else the file is viewed (GitHub, download, our own static preview), let a pathological blank run explode into thousands of empty nodes, and caused the visible reflow on open (static preview collapses empty <p>; the live editor gives each a trailing-break line). Collapsing on parse keeps normal one-blank-line spacing, matches every standard renderer, and stays idempotent so the round-trip-safety probe still reaches a fixed point (files stay editable; existing files normalize on next cold-open + save). Serializer is intentionally NOT changed — a global blank-run collapse there would corrupt blank lines inside fenced code blocks. |
||
|
Vikhyath Mondreti
|
746756d8a9 |
improvement(misc): settings organization, chat agent deploy request keys (#6195)
* improvement(misc): settings organization, chat agent deploy request keys * fix subtitle * adjust perms |
||
|
Vikhyath Mondreti
|
b8ec114382 |
improvement(chat): secrets mounting / exposure improvements and controls (#6191)
* fix(copilot): secrets injection into sandbox * improvement(chat): secrets mounting / exposure improvements and controls * fix(secrets): simplify copilot mounting flow * test(secrets): preserve standard tool permissions * fix(copilot): bind workflow tool completions * fix(secrets): preserve own environment keys * fix(copilot): release failed workflow claims * fix(copilot): trust compacted workflow completion |
||
|
Waleed
|
25e609167f |
fix(search): disambiguate tables and knowledge bases by folder (#6192)
* fix(search): disambiguate tables and knowledge bases by folder The Cmd-K search modal listed tables and knowledge bases without the folder breadcrumb workflows and files already showed, and the table, knowledge base, and search-and-replace pickers in the workflow editor rendered bare names -- so two resources sharing a name in different folders were indistinguishable. Extracts the disambiguation the workflow selector already did into shared collectDuplicateNames + disambiguateLabelByFolder, and shares the search row's folder breadcrumb and its memo comparator, which were duplicated between the workflow and file rows. Also routes folder text through filterAndCap's secondary-rank parameter rather than concatenating it into the name, so an exact name match can no longer be outranked by a folder that happens to fuzzy-match. * fix(zoho-desk): use the real Zoho Desk mark on a white tile The icon was a generic headset placeholder drawn in currentColor, so it never resembled Zoho at all. Replaces it with the mark from Zoho's official logo -- wordmark stripped, viewBox set to the mark's own bounding box so it centers -- and moves the tile to white, matching the other brand-mark integrations. |
||
|
Waleed
|
f113b0bc24 |
fix(chat): show table and knowledge base folders in the resource menus (#6190)
Tables and knowledge bases gained folder support, but the chat attachment menu and the embedded resource menu still listed them flat. Both now render the same hierarchy the Tables and Knowledge pages show. Unifies the two near-duplicate workflow/file tree builders into one shared buildResourceFolderTree plus a spec-driven useResourceTreeSections, so all four foldered families go through one code path. Items whose folder no longer resolves now surface at the root instead of vanishing. |
||
|
mzxchandraandWaleed Latif
|
87aeca6f0c |
feat(zoho-desk): add Zoho Desk integration (#6157)
* feat(zoho-desk): add Zoho Desk integration
Add a full Zoho Desk integration: tools, block, icon, and a webhook trigger.
Tools (tools/zoho_desk): list/get/update tickets, list/add comments,
list/get threads, get contact, list organizations, and download attachments
as UserFiles via an internal route. Registered in tools/registry.ts.
Block (blocks/blocks/zoho-desk.ts): operation dropdown, OAuth credential,
an organization selector backed by GET /organizations, per-operation fields,
and BlockMeta templates. Wires the Zoho Desk trigger.
OAuth (zoho-desk provider): authorize/token at accounts.zoho.com with
access_type=offline + prompt=consent; the Desk REST base is derived from the
token response api_domain and persisted so calls honor data residency instead
of assuming desk.zoho.com. Every call sends Authorization: Zoho-oauthtoken and
the orgId header.
Trigger + webhook handler (triggers/zoho_desk, lib/webhooks/providers/zoho-desk.ts):
Sim creates and tears down the Zoho Desk webhook subscription. Inbound events
are verified with JWT RS256 (X-ZDesk-JWT) against the data-center JWKS, ACKed
via the durable queue to meet Zoho's 5s deadline, and fail loudly on
Free/Standard editions that cannot create webhooks.
* fix(zoho-desk): OAuth PKCE, DC scope-marker parsing, SSRF, and e2e fixes
OAuth: forward code_verifier in the custom getToken (PKCE is enabled, so the
exchange must echo the verifier or Zoho rejects the request with invalid_request).
Surface Zoho's error/error_description, which it returns in the JSON body with
HTTP 200, instead of collapsing every failure into "no access token".
Data-center base parsing: better-auth persists Zoho's scopes comma-joined with no
spaces, so the greedy \S+ marker regex swallowed the whole scope list into the
host. Stop the capture at a comma or whitespace in both read sites (token route
and webhook handler), so apiDomain resolves to the real Desk host.
Attachment SSRF: replace the permissive host regex (which accepted attacker
domains like zoho.attacker.com) with a strict Zoho-apex suffix allowlist.
Block: guard Number() pagination so a non-numeric typo can't send NaN; add the
ignoreSourceId -> sourceId loop-guard header to update_ticket (matching add_comment).
Organizations route: surface fetch/Zoho failures with a real status instead of a
200 with an empty list, so the org selector no longer fails silently.
* fix(zoho-desk): webhook creation, attachment naming, and HTML content handling
Webhook trigger (verified end-to-end against a live Enterprise org):
- Omit ignoreSourceId; Zoho rejects a non-Zoho UUID with INVALID_DATA. Drop
the generateId() fallback and its providerConfig persistence.
- Answer Zoho's create-time notification-URL probe via the existing pending
webhook verification mechanism (GET/HEAD matchers) so subscription creation
no longer 405s.
- mapZohoWebhookError now surfaces Zoho's real errorCode / message / field
errors instead of a catch-all edition message, and attaches an HTTP status so
4xx flow through NonRetryableDeploymentError while 429/5xx stay retryable.
- Propagate the real status through deploy.ts so failed creates don't retry-loop.
get_attachment polish:
- Return the downloaded file's name under `name` (ToolFileData key) instead of
`filename`, and derive it (explicit -> Content-Disposition -> URL segment ->
fallback) so attachments are no longer stored as "untitled".
- Gate the add_comment-only `contentType` param so it isn't sent to get_attachment.
HTML content handling (Zoho content fields emit raw HTML):
- Add a Zoho-local html-to-text converter mirroring the Outlook dual-field
pattern: when contentType is 'html', derive a plain-text `contentText`
alongside the untouched raw `content` + `contentType`; plainText mirrors.
- Apply to comments (list/add), threads (list/get), the ticket description
(descriptionText), and the webhook trigger payload.
Trigger org selector: Organization is now a credential-scoped combobox that
lists the connected account's Zoho Desk organizations.
* fix(zoho-desk): review round - DC-base derivation, org-loader resilience, batched-event visibility
- deriveZohoDeskBaseFromApiDomain: preserve an already-regional desk.zoho.<tld>
api_domain instead of falling back to the US (.com) data center, and map the
DC TLD from any zoho(apis).<tld> host - keeps Desk calls in the right data
center for residency.
- fetchZohoDeskOrganizationOptions: wrap the token/org fetch in try/catch and
degrade to an empty list (the org field is a free-text combobox, so manual
entry still works) instead of hard-failing the selector on token/DC/network
errors.
- formatInput: warn (not silently drop) if Zoho ever delivers more than one
event in a single payload.
* fix(zoho-desk): harden attachment download against redirect-based SSRF/token leak
Replace the raw fetch in the attachment route with secureFetchWithValidation
(the same guarded fetch the copilot file-download tool uses). The download URL
is user/LLM-influenced and Zoho may redirect, so auto-following redirects could
send the OAuth token / orgId to an untrusted or internal host. The guarded fetch
pins the resolved IP, blocks private/reserved targets on every hop, drops the
Authorization header if a redirect leaves the origin (stripAuthOnRedirect), and
enforces the 50MB cap while streaming. The strict Zoho apex allowlist still
gates the initial origin as defense in depth.
* fix(zoho-desk): only add the edition hint when Zoho's error indicates it
mapZohoWebhookError appended the "requires Professional edition or higher"
guidance to every 403, but a 403 can also mean a wrong org, a missing scope, or
a bad token. Gate the hint on Zoho's own errorCode / message matching the
permission/edition pattern instead of the bare status, so unrelated 403s surface
Zoho's real reason without the misleading suffix. Adds a test for the
non-edition 403 path.
* fix(zoho-desk): stop duplicating /api/v1 when resolving a relative attachment href
A relative attachment href that already starts with `api/v1` (as Zoho's hrefs
often do) was concatenated onto getZohoDeskApiBase (which ends in /api/v1),
producing `/api/v1/api/v1/...` and a failing download. Extract a tested
resolveZohoAttachmentUrl helper that uses absolute hrefs as-is and strips a
leading slash + `api/v1/` prefix from relative ones before joining, so the path
is correct for absolute, root-relative, and api/v1-prefixed hrefs alike.
* fix(zoho-desk): reject an empty update_ticket PATCH with a clear error
update_ticket built its PATCH body from optional fields via filterUndefined, so
a call with no fields set sent `{}` and surfaced an opaque Zoho failure. Guard
the body builder to throw an actionable "provide at least one field" error
before the request. Adds a test for the empty and populated body paths.
* fix(zoho-desk): fall back to the credential Desk domain in webhook JWT verify
verifyAuth chose the JWKS host from providerConfig.apiDomain and otherwise
defaulted to the US host (desk.zoho.com), so a non-US webhook row missing
apiDomain would verify against the wrong JWKS and reject legitimate events. When
apiDomain is absent, resolve it from the OAuth credential's __zoho_domain__ scope
marker (mirroring deleteSubscription). The persisted-apiDomain fast path stays
DB-free to respect the 5s delivery deadline. Adds tests for both paths.
* fix(zoho-desk): apply the Zoho host allowlist to the organizations route
The organizations route built its URL from the client-supplied apiDomain and
attached the OAuth token without the https-Zoho-host allowlist the attachment
route already enforced, so a session-access caller could point the server at an
arbitrary origin and leak the token. Extract the shared isZohoHost allowlist and
an assertZohoUrl guard into tools/zoho_desk/utils (two consumers now), guard the
organizations URL before fetching, and refactor the attachment route to reuse
the shared helper. Adds tests for the allowlist and guard.
* fix(zoho-desk): propagate provider 4xx in the stable webhook prepare path
The v2 stable deploy preparation flattened every registration failure (except
path conflicts) to HTTP 500, so a provider-attached permanent 4xx - e.g. Zoho's
edition/validation failures from createSubscription - retried instead of failing
the deploy terminally. Propagate the attached status (`?? 500`), matching the
legacy save path's status-aware mapping so both deploy paths route 4xx through
NonRetryableDeploymentError.
* fix(zoho-desk): make createSubscription config failures non-retryable
createSubscription threw plain Errors (no status) for missing orgId, event type,
or credentials, and for a Zoho success with no webhook id - so the deploy outbox
mapped them to 500 and retried permanent configuration failures. Attach a 4xx
via statusError (400 for missing config/credentials; 422 for the no-id anomaly,
where a retry risks duplicate webhooks) so they fail the deploy terminally like
the mapped Zoho API 4xx responses. Tests assert the 400 status on the guard paths.
* fix(zoho-desk): enrich prevState with contentText symmetrically with payload
formatInput derived plain-text contentText only on payload, so an update event
for a comment/thread left prevState as raw HTML while payload carried
contentText - inconsistent shapes for before/after comparisons. Apply
withDerivedContentText to prevState too. Test asserts both are enriched.
* docs(zoho-desk): regenerate integration docs
Regenerate zoho_desk.mdx from the current tool definitions: removes the stale
add_comment `ignoreSourceId` input row (the field was dropped because Zoho
rejects arbitrary values) and adds the derived `contentText` / `descriptionText`
plain-text fields on comments, threads, and tickets.
* fix(zoho-desk): validate the persisted Desk base against the strict host allowlist
deriveZohoDeskBaseFromApiDomain trusted any host matching `desk.zoho.[a-z.]+`,
so a crafted api_domain like `desk.zoho.com.attacker.com` passed and was
persisted as the credential's `__zoho_domain__` REST base - later receiving the
OAuth token on every Desk tool/webhook call. Gate the derivation on the strict
isZohoHost apex allowlist (which rejects that lookalike), extracted with
assertZohoUrl into a dependency-free host-allowlist module so the auth
token-exchange path validates hosts without pulling in the tool utilities. The
attachment and organizations routes now import the shared guard from there.
Also: formatInput now emits the normalized null trigger shape for an empty/
malformed event array instead of leaking a raw `[]` to downstream steps. Tests
cover the empty-array shape and the lookalike-host rejection.
* fix(zoho-desk): correct API field names, scopes, and host validation
Validation pass against Zoho's published Desk API surfaced six defects that
typecheck, lint, and the existing suite all passed over, because each one fails
silently against the live API rather than erroring.
Wire-name mismatches (Zoho ignores unknown keys, so all three were silent):
- update_ticket sent `customFields`; the ticket PATCH body names it `cf`.
`customFields` exists only as a deprecated alias on other Desk resources and
on the separate validate-field-updates endpoint, so updates reported success
and applied nothing.
- ZOHO_DESK_TICKET_PROPERTIES and ZOHO_DESK_CONTACT_PROPERTIES advertised a
`customFields` output; both resources return `cf`. The declared field always
resolved undefined and the real one was undeclared.
- list_tickets sent `departmentId`; the query param is `departmentIds`, so the
department filter was dropped and every department's tickets came back.
Content handling:
- deriveZohoContentText matched `contentType === 'html'`, but Zoho spells the
discriminator per resource: comments use `html`, threads use the MIME form
`text/html`. Every thread's `contentText` was therefore raw markup - the exact
opposite of the field's purpose. Now normalized across both spellings,
parameterized values, and casing, with regression tests.
Scopes (least privilege):
- Desk.tickets.ALL -> Desk.tickets.READ + Desk.tickets.UPDATE. No tool creates
or deletes a ticket; ALL additionally granted ticket DELETE.
- Dropped Desk.search.READ (no search tool exists) and Desk.webhooks.READ /
.UPDATE (the provider only creates and deletes), plus their orphaned
SCOPE_DESCRIPTIONS entries.
Host validation - the webhook provider was the only token-carrying path not
anchored to the Zoho apex allowlist, including the JWKS fetch, where an
unrecognized host would have stood in as the JWT issuer:
- createSubscription, deleteSubscription, and verifyAuth now route their base
through a shared allowlist check.
- getZohoDeskApiBase validates rather than trusting injection precedence.
- The organizations route uses secureFetchWithValidation with
stripAuthOnRedirect, matching the attachment route it had diverged from.
Block and trigger:
- The trigger's department field is renamed `triggerDepartmentIds`; sharing the
`departmentIds` id let a value typed as a list_tickets filter become the
webhook subscription's filter when switching modes.
- `isPublic` no longer serializes onto all ten operations, matching the existing
gating for `contentType`.
- from/limit reject negatives and fractions instead of forwarding them.
- update_ticket gains description, resolution, and classification (all already
declared as outputs), and a departmentId input so a ticket can be moved.
Accuracy corrections to user-facing text, all against the published parameter
tables: `from` is 0-based (0-4999, default 0), not 1-based; per-endpoint limits
are tickets 1-100/10, comments 1-100/50, threads 1-200/100; sortBy lists Zoho's
actual allowed values; the two `include` sets genuinely differ per endpoint;
status and priority accept comma-separated lists.
Also: path IDs are trimmed via requireZohoDeskId so a pasted trailing space
fails with a clear message instead of a %20 404; comment `commenter` and thread
`status`/`isDescriptionThread`/`visibility`/`canReply` are now declared;
ZOHO_CLIENT_ID/SECRET added to the oauth test env; docs page gains a
MANUAL-CONTENT intro covering capabilities, the Professional-edition webhook
requirement, and the US-data-center limitation.
Not verified from documentation, needs a live account before merge:
- the OAuth scope for the attachment content sub-path (Zoho publishes none, and
there is an unanswered SCOPE_MISMATCH report against it)
- 12 of the 17 offered webhook event ids (5 are confirmed); Ticket_Delete is
documented but not offered
- the ticket `descriptionContentType` key, and the POST /api/v1/webhooks body
shape, neither of which appears in any reachable Zoho reference
* chore(zoho-desk): regenerate tool metadata
The param and description corrections in the previous commit changed the
generated tool surface, so tool-metadata:check failed in CI. Regenerated;
the diff is two Zoho-only lines.
* fix(zoho-desk): stop posting null for untouched update_ticket fields
`filterUndefined` strips only `undefined`, but an untouched subBlock never
arrives as `undefined`: the workflow serializer initializes every subBlock value
to `null` (stores/workflows/utils.ts) and extractBlockParams writes those nulls
straight into tool params, with nothing between the serializer and request.body
filtering them.
Reproduced against the real serializer and block with only `status` set:
basic {"subject":null,"status":"Closed"}
advanced {"subject":null,"status":"Closed","priority":null,...,"cf":null}
`subject` leaks even in basic mode because it declares no `mode`, so
shouldSerializeSubBlock never drops it. Zoho documents subject as a writable
field, so every status-only edit either failed the PATCH or blanked the ticket's
subject; in advanced mode the whole update surface nulled out, including `cf`.
Two things hid this. The empty-PATCH guard was unreachable from the block (the
body always carried at least `subject`), and the existing test called buildBody
with fields *absent* rather than null - the shape the block never produces - so
it could not fail on the real path.
Replaces filterUndefined with a local omitUnset that drops undefined, null, and
'' (a cleared input means "leave unchanged", not "set to empty"). Adds three
tests using the real serializer shape, all verified to fail before the fix.
Also fixes the same null-blindness in the block's param mapping, where
Number(null) === 0 injected from=0 on every operation, and corrects the shared
limit placeholder, which claimed max 100 while list_threads allows 200.
* feat(zoho-desk): add Self Client service-account credential
Adds a second way to connect Zoho Desk, alongside the interactive OAuth flow: a
Zoho Self Client, pasted as client id + client secret + organization id. Built
on the existing client-credential-accounts framework rather than a new credential
path, so it behaves like the Zoom Server-to-Server and Box CCG accounts already
in the repo - a short-lived token minted on demand, no refresh token.
Two Zoho behaviors the generic framework does not cover:
- `scope` must be COMMA-separated on Zoho's token endpoint; a space-separated
list is rejected as an invalid scope. The list comes from
getCanonicalScopesForProvider('zoho-desk'), so the Self Client and the OAuth
flow can never drift apart on scopes.
- Zoho reports OAuth failures in the JSON body, frequently with HTTP 200
(e.g. {"error":"invalid_client"}), so the success body is inspected for an
`error` field before the token is read - a status-only check would accept a
failed mint.
deriveZohoDeskBaseFromApiDomain moves out of auth.ts into the dependency-free
host-allowlist module so the minter and the OAuth path share one derivation
instead of duplicating it, and the mint response's api_domain now flows through
to tools as `apiDomain` (the SA branch of the token route previously returned
none, so SA calls would have assumed desk.zoho.com).
Docs: hand-authored zoho-desk-service-account.mdx following the existing
*-service-account.mdx pages, registered in meta.json and in the generator's
keep-list so stale-page cleanup does not delete it.
Known limitation, documented in the descriptor helpText and the docs page:
webhook triggers still require an OAuth connection. Webhook provisioning resolves
credentials through getCredentialOwner/refreshAccessTokenIfNeeded, which is
OAuth-account-only for every provider in the repo - not a Zoho-specific gap.
Unverified from documentation, needs a live Zoho org before merge:
- the `ZohoDesk.` soid prefix. Zoho documents only the syntax
{servicename}.{zsoid} with a single CRM example; no first-party doc states the
Desk prefix. normalizeZohoDeskSoid passes through any value already containing
a '.', so an operator can paste a corrected full soid without a code change.
- whether zsoid is the same identifier as the Desk orgId header value.
- whether the client-credentials endpoint accepts Desk.webhooks.CREATE/DELETE
for a Self Client.
- whether the mint response populates api_domain for Desk (documented for CRM);
if absent the derivation falls back to the US Desk host.
* fix(zoho-desk): derive descriptionText for ticket-shaped payloads
Cursor Bugbot: webhook ticket events reached workflows as raw HTML with no
plain-text sibling. `withDerivedContentText` only looked at `content` /
`contentType`, but ticket resources carry their body on `description` /
`descriptionContentType`, so trigger output disagreed with get_ticket.
The helper now derives both, which also removed two inconsistencies on the tool
side: get_ticket had its own inline copy of the derivation (now one shared
implementation that cannot drift), and update_ticket returned its PATCH response
raw despite the shared output map declaring descriptionText.
`descriptionContentType` remains the one field name unconfirmed in any Zoho
reference. It degrades safely - an absent key makes deriveZohoContentText return
the value unchanged, so descriptionText mirrors description rather than breaking,
exactly as get_ticket already behaved - and it is now one helper to correct if
Zoho names it differently.
* feat(zoho-desk): let the service account pick its data center
Zoho's accounts server is per region, and the integration pinned every call to
the US host. For the interactive OAuth flow that is currently unavoidable -
better-auth's authorize/token URLs are static per provider - but the service
account mints its own token, so the region can simply be chosen. This makes the
Self Client the only way a non-US Zoho org can connect.
Adds an optional `dataCenter` field to the client-credential framework. Optional
matters: ClientCredentialAccountFieldId and ClientCredentialAccountFields are
shared with Zoom, Box and Salesforce, whose descriptors and minters are
unchanged. Blank keeps the previous behavior (US), so existing credentials are
unaffected.
Only us/eu/in/au are offered - the four regions where both the accounts server
and the Desk REST host are confirmed. CA is deliberately absent: Zoho's accounts
docs say accounts.zohocloud.ca while Zoho's own Desk SDK says accounts.zoho.ca,
and the two cannot both be right. JP/SA/CN/UK lack a confirmed Desk host.
The Desk base is now derived from the selected region rather than inferred from
the mint response, which also removes a dependency on `api_domain` being
populated for Desk (Zoho documents it for CRM only). When `api_domain` IS present
and disagrees with the region, it wins - it is authoritative about where the
token actually works - and the mismatch is logged so a mis-selected region is
diagnosable. deriveZohoDeskBaseFromApiDomain gains a `try` variant returning
undefined so an untrusted api_domain can no longer masquerade as an authoritative
US answer and silently override a correct region.
A wrong region fails loudly rather than silently: the minter runs as verification
on both create and reconnect, so the credential is never persisted in a broken
state. Because Zoho reports it as `invalid_client` - a Self Client only exists on
its own region's accounts server - the operator hint for that code now names the
data center as a candidate cause.
Copy is scoped per path rather than blanket "US only": the OAuth service
description, trigger setup instructions, and the docs intro now say which path
each limitation applies to, and the service-account page documents the four
regions with a sign-in-domain to region-code table.
* fix(zoho-desk): strip ticket description HTML, classify body-reported refresh failures
Final validation pass findings.
descriptionText never stripped anything. It was gated on a
`descriptionContentType` discriminator that Zoho does not send: the Ticket_Add
webhook sample ships `"description": "<div>Description</div>"` with no such key,
and the ticket GET/PATCH response field lists have no content-type sibling
either. So get_ticket, update_ticket, and every webhook ticket payload emitted
descriptionText as a byte-identical copy of the raw HTML, while the declared
output promised stripped text.
The tests did not catch it because they fabricated the shape - both fixtures
constructed `descriptionContentType: 'html'`, a key Zoho never emits, proving the
branch works without proving it is ever taken. Ticket descriptions are HTML by
convention, so the strip is now unconditional (html-to-text is a near-identity on
genuinely plain text), an explicit descriptionContentType is still honored if
Zoho ever adds one, and the fixtures now use Zoho's real shape with no
content-type key anywhere.
A body-reported refresh failure was unclassified. Zoho answers a revoked refresh
token with HTTP 200 and `{"error":"invalid_client"}`; refreshOAuthToken only
checked `data.ok === false` (a Slack-ism), so the request fell through to the
"no access token" guard and returned no errorCode. isTerminalRefreshError could
therefore never recognize invalid_client as terminal, the credential was never
marked dead, and every later execution retried a refresh that cannot succeed -
with the user shown "No access token in refresh response" instead of a reconnect
prompt. The body is now classified before the status is trusted, matching what
the token exchange and the service-account mint already did. That guard also
stopped logging the whole response body, which carries live tokens on a partial
success.
Also: an unrecognized dataCenter now fails with a named error instead of quietly
resolving to US and surfacing as an opaque invalid_client (blank still means US);
the webhook JWKS cache is bounded, since its key derives from a providerConfig
field that SYSTEM_MANAGED_FIELDS protects from diffing but not from being
written; and the attachment `size` output no longer asserts bytes, a unit Zoho
documents as KB.
* feat(zoho-desk): canonical selectors and BlockMeta skills
The block picked its organization with an ad-hoc `combobox` + `fetchOptions`.
Only five blocks in the repo did that, and the other four are core blocks
(agent/credential/function/logs) - no other OAuth integration used it. Every
other resource a user has to identify was a bare short-input taking an opaque
numeric id.
Zoho Desk now uses the same machinery as the other 25 selector providers:
hooks/selectors/providers/zoho-desk/selectors.ts registered in the selector
registry, consumed from the block as basic selector + advanced manual input
sharing one canonicalParamId, for organization, update-ticket department, and
the list-tickets department filter. The trigger's org field moves to the same
selector. zoho-desk-org-options.ts is deleted rather than left beside the new
path, so blocks/ has zero fetchOptions usages outside the core blocks.
Wire params are unchanged (orgId, departmentId, departmentIds, assigneeId,
ticketId, contactId) - this is a UI change, not an API change.
The organizations route now resolves the credential server-side. It previously
had the browser fetch an access token and POST it back, which an earlier audit
flagged as the one place a Zoho token left the server; the new selector-credential
resolver keeps it server-side for both the OAuth and service-account credential
types and re-anchors every outbound host to the Zoho apex allowlist.
No agents selector: the endpoint is documented but its OAuth scope is not, and
the nearest evidence points at Desk.agents.READ, which we do not request. Adding
it would force every existing Zoho Desk user to reconnect for a convenience
field, so assigneeId stays a manual input until the scope can be confirmed
against a live org.
Adds the skills array BlockMeta was missing - 227 of 300 blocks declare one and
this did not. Seven skills, each grounded in a use case Zoho or the ecosystem
actually advertises (auto-triage, SLA escalation, digest, AI draft reply,
customer context, engineering handoff, knowledge-gap report) and each exercising
only tools in tools.access. CSAT surveys, ticket creation, dedup and keyword
search were deliberately left out: the integration has no tool for them, and a
skill implying an unsupported action is worse than a shorter list.
* feat(zoho-desk): agents selector and free-text trigger organization
Three improvements that were previously deferred only to avoid forcing existing
users to reconnect or orphaning saved workflows. This integration is unmerged and
has no users, so the constraint does not apply and the better option wins.
assigneeId was the last field still asking for an opaque numeric id. It is now a
canonical selector pair backed by a new zoho_desk.agents selector, which required
adding the Desk.agents.READ scope - the reason it was skipped before. Route
follows the departments one exactly: auth before parseRequest, host anchored to
the Zoho apex allowlist, secureFetchWithValidation with stripAuthOnRedirect, and
a page drain capped at 20 pages with 204 treated as end-of-list.
Scope caveat: Zoho publishes no explicit scope line for the list-all
GET /api/v1/agents. Every other endpoint in the Agents module documents
Desk.agents.READ (get by id, get by email, roles/{id}/agents), and it is the only
agents-module scope Zoho defines, so that is the basis. Inference across a module
rather than a direct quote - worth one live call before merge, same as the
existing attachment-scope note.
The trigger regained free-text organization entry, lost when the org field became
a selector. The earlier concern - that a manual value would land under its raw
subBlock id and never reach the provider - turned out not to hold: buildProviderConfig
already collapses canonical pairs and writes the active member under the canonical
key. The real gap is narrower and does exist: when canonicalModes pins the group
to basic while only the manual field has a value, the collapse deletes the
canonical key even though the required-field check passes, so the deploy succeeds
and then fails at subscription time. resolveConfigOrgId closes that, with a test.
The block/trigger `orgId` id overlap stays shared, now with a comment. Two earlier
audits disagreed; renaming turns out to be the wrong call. buildCanonicalIndex has
an explicit guard for trigger-mode reuse and blocks.test.ts codifies it as a valid
pattern, orgId means the same portal in both modes (unlike departmentIds, which is
correctly distinct), and a separate triggerManualOrgId would put two advanced
members in one canonical group - getCanonicalValues takes the first non-empty, so
a stale tool-mode value could silently supply the trigger's organization.
* fix(zoho-desk): make the attachment cap reachable, unbreak selector paging
Final audit round.
The 50 MB attachment ceiling could never be hit. This route returns the file as
base64 inside its JSON body, and the executor reads internal tool responses
through readToolResponseBody, capped at 10 MB. Base64 inflates 4/3, so ~7.5 MB
of raw bytes is the real ceiling - and the old limit meant a larger attachment
was downloaded, encoded and serialized in full (peaking near 250 MB of live
allocation, with nothing bounding concurrent downloads) purely to be rejected
afterwards. The cap is now the reachable size, so the limit enforces itself while
the bytes are still streaming, and an overflow returns 413 with the actual
ceiling instead of a generic 500. Raising it properly means uploading in the
route and returning a file reference, as the WhatsApp media route does - not a
bigger constant.
Selector paging assumed a 0-based `from`. Zoho's docs contradict themselves:
the pagination section says "range 0-4999, default 0" while the listing examples
read as 1-based ("from=5 and limit=50 retrieves records 5 to 54"). Under the
1-based reading, stepping by exactly the page size re-fetches the boundary record
and the dropdown shows a duplicate per page. Rather than pick a base that cannot
be confirmed without a live tenant, the department and agent drains dedupe by id,
which is correct under either reading.
The organization list was unpaginated, and Zoho's listing APIs default to ten per
page. An account with more accessible portals silently got a truncated dropdown,
and since every other selector and every tool call is gated on orgId, a missing
portal was unreachable except through the advanced manual field. Both the
selector route and list_organizations now request the documented maximum.
Docs: regenerated so the trigger table includes manualOrgId, and two
service-account claims are hedged to match what the code already says it cannot
verify - that zsoid equals the Desk orgId header value, and that every tool works
under the requested scopes (Zoho publishes no scope for the attachment content
sub-path).
Also: status and priority move out of advanced mode - they are the fields most
often changed on a ticket update; the custom-fields wand prompt now ends with the
required "Return ONLY" clause; and the shared-orgId rationale comment cites the
mechanism that actually applies (buildCanonicalIndex dedupe plus the first-non-
empty rule in getCanonicalValues) rather than a blocks.test.ts branch that never
evaluates this pair.
* fix(zoho-desk): five-audit round - serializer trigger-advanced leak, scopes, paging
Five independent audits (OAuth/scopes, tools-vs-docs, block/selectors,
blast-radius, /validate-trigger). Findings, most severe first.
A trigger-mode field was a live tool-mode required param. `shouldSerializeSubBlock`
excluded `mode: 'trigger'` but not `'trigger-advanced'`, so the trigger's required
`manualOrgId` validated on every tool operation. Reproduced against the real
serializer: with the Organization field pinned to advanced, running
List Organizations failed with "Missing required fields: Organization ID" - a
field that operation does not even render, and which the user could not clear
without switching operations. Fixed in the serializer rather than locally,
because the Google Sheets/Drive/Calendar pollers have the identical shape.
`limit=200` on /organizations was an undocumented parameter I added by
extrapolating from /departments and /agents. Zoho documents NO parameters for
that endpoint and its sample is a bare GET; the other siblings cap at 100 and
Zoho answers out-of-range with 422. Since orgId gates every tool and both other
selectors, a 422 there would have made the whole integration unreachable. Reverted
to Zoho's documented shape.
`descriptionText` was HTML-stripping plain text. The previous round made the strip
unconditional after finding Zoho sends no `descriptionContentType`, but Zoho's REST
samples show plain descriptions while only the webhook payload is HTML - and the
webhook path runs this over contact/account/department bodies too. html-to-text is
not identity on plain text: it decodes entities and deletes tag-shaped content
("a < b > c", XML snippets). Now sniffs for markup first.
`omitUnset` made every documented field-clear impossible. Zoho's own PATCH sample
uses `"classification": ""` and `"productId": ""` to clear. Dropping `''` meant no
scalar field could be cleared. Now drops only undefined/null - the serializer-null
case it was written for - and forwards `''`.
status/priority leaked between operations. One shared subBlock served both the
list_tickets filter and the update_ticket value, and subBlock values survive an
operation switch, so a filter of "Open,On Hold" could be PATCHed onto a ticket and
an update value could silently filter a later list. Split per operation.
Auth: `invalid_code` added to TERMINAL_ERRORS - it is Zoho's code for a revoked
refresh token, so without it the previous round's refresh fix never actually
dead-flagged the credential it was written for. The shared refresh body-error
branch now also requires `!data.access_token`, so no provider can have a
successful refresh misclassified. The token route now uses the validating
`extractZohoDeskBaseFromScope` instead of a private regex with no https/allowlist
check - that value is injected into every tool call. Scope list falls back to the
requested scopes when Zoho omits `scope`, which would otherwise flag every
credential as needing reconnect. The Self Client mint no longer sends
`aaaserver.profile.READ`, a scope that grant never uses.
Trigger: `includePrevState` now set for every *_Update event, not just tickets -
it defaults to false, so prevState was permanently null for contact/agent/task/
article updates while the trigger advertised it. `departmentIds` is only sent for
events Zoho documents as accepting it, and the field is conditioned accordingly.
Empty filters serialize as `null`, matching Zoho's examples, rather than `{}`.
JWKS fetch bounded to 1.5s - jose's default is 5000ms, exactly Zoho's whole
delivery deadline, and Zoho publishes no retry. The create-time validation POST
fallback is now matched by the pending-verification probe. Ticket_Delete added.
All 17 webhook event ids, the POST /api/v1/webhooks body contract, and the JWT
claim/JWKS specifics are now confirmed verbatim against Zoho's webhook
documentation - previously 12 of 17 events and the entire subscription contract
were unverified.
* revert(zoho-desk): back out both shared lib/oauth changes
Reverting two changes to shared OAuth code because their premise is inferred
rather than proven, and neither meets the bar for touching a path every provider
runs.
`refreshOAuthToken` body-error branch. The premise was that Zoho reports refresh
failures with HTTP 200 and an `error` body. That is documented and empirically
confirmed for the authorization-code EXCHANGE (see the comment on getToken in
auth.ts), but I never confirmed it for the REFRESH grant specifically - and if
Zoho returns a proper 4xx there, the existing `!response.ok` path already
classifies it via extractErrorCode, making the branch dead code that every one
of the ~34 providers still executes on each refresh. A shared branch whose only
justification is an unverified inference about one provider is not worth its
blast radius.
`invalid_code` in TERMINAL_ERRORS. Same problem, worse downside: the code is
sourced from a Zoho community post rather than official docs, TERMINAL_ERRORS is
consulted for every provider, and a false positive marks a credential dead for an
hour. Not adding it simply preserves today's behavior (retry rather than
dead-flag), so reverting costs nothing that was previously working.
Both are cheap to reinstate, correctly scoped, once a live Zoho account shows
what a revoked refresh token actually returns.
Kept: the token-redaction on the "no access token" warn, which is an unambiguous
improvement independent of Zoho.
Also kept, deliberately, is the serializer `trigger-advanced` exclusion - that one
rests on a reproduced bug rather than an inference, and it aligns the serializer
with the convention the rest of the codebase already follows (blocks.test.ts
treats `trigger` and `trigger-advanced` identically in six places, as does the
copilot block-metadata tool, and blocks/types.ts documents trigger-advanced as
"the advanced side of a trigger field").
* fix(zoho-desk): carry the stored data center through a credential reconnect
A reconnect rebuilds the service-account secret blob from the submitted fields
only, and the connect modal never prefills - correctly, since for every other
field in this family the stored value is a secret the admin must retype. The
data center is the first non-secret member of that set, so it was being silently
dropped: rotating a client secret on an EU/IN/AU credential moved it back to the
US accounts server, where the next mint fails with an opaque invalid_client.
performUpdateCredential now reads the stored dataCenter out of the existing blob
when the caller does not supply one. The read is failure-tolerant - an
undecryptable or unparseable blob yields undefined rather than throwing, so it
can never block a reconnect, and the provider default applies as before.
Raised independently by three reviewers; I twice argued it was acceptable because
the mint fails loudly rather than corrupting silently. That was true and beside
the point - the operator still had to guess why.
* fix(zoho-desk): delta-audit findings - prevState scope, status leak, HTML sniffer
An audit of the commits the earlier five audits never saw. All four findings are
in code written as fixes for those audits, which is where this branch has
repeatedly introduced new problems.
`includePrevState` was sent for Ticket_Comment_Update. The previous commit gated
it on an `_Update` suffix and claimed Zoho supports it on every update event.
Zoho's webhook doc lists the attribute on Ticket/Contact/Agent/Task/Article update
events but NOT on Ticket_Comment_Update, which documents only `departmentIds`.
That made it an undocumented filter key on a live subscription create - the same
class of risk the same commit reverted `limit=200` for, so it failed that commit's
own stated bar. Now an explicit set rather than a suffix rule.
The status/priority split did not stop the leak it was written for. The mapping
used `operation === 'list_tickets' ? filterValue : updateValue`, whose bare else
covers all eight other operations - so a stale Update Ticket status was forwarded
into get_ticket, list_comments and the rest. Harmless on the wire (those tools
ignore it) but exactly the stale-value pattern the neighbouring gates exist to
prevent. Both fields are now scoped to the two operations that declare them.
The HTML sniffer destroyed plain text. `/<[a-z!\/][^>]*>/` fires on any `<`
followed by a letter with a later `>`, so realistic ticket bodies lost content:
"if x<y then z>0" became "if x0", and "replace <username> with the real name"
lost the placeholder. It now requires a real element - a paired tag, a
self-closing tag, a comment/doctype - or an entity, and the entity arm covers hex
references it previously missed. Regression tests verified by reverting to the
loose pattern and watching them go red.
The reconnect data-center carry-forward is scoped to client-credential providers.
As written it added a DB read plus a decrypt to every service-account reconnect
for every provider - Slack, Atlassian, all token-paste providers - to carry a
field only Zoho has.
Also: the JWKS cache-bound TSDoc had been orphaned onto the wrong constant by an
earlier insertion, and `cooldownDuration` was dropped since it restated jose's
default while only `timeoutDuration` needed justifying.
* test(zoho-desk): cover the webhook subscription filter rules
The subscription filter logic had no test coverage at all, and it is where the
last two rounds both found bugs - includePrevState on an event Zoho does not
document it for, and departmentIds sent to events that accept no filters.
Adds six cases against the real createSubscription: includePrevState is set for
each of the five documented update events and NOT for Ticket_Comment_Update,
departmentIds is kept for a filterable event and dropped for one that is not, and
an event with no filters serializes as null rather than an empty object.
Verified the guard bites: reverting PREV_STATE_EVENTS to the `endsWith('_Update')`
rule turns the Ticket_Comment_Update case red.
The Ticket_Comment_Update assertion checks the with-departments case as well as
the bare one - asserting only `not.toHaveProperty` on the bare filter would pass
vacuously, since that filter is legitimately null.
---------
Co-authored-by: Waleed Latif <walif6@gmail.com>
|
||
|
Waleed
|
69289d26e7 |
fix(execution): stop run-buffer failures from stranding resumed executions (#6187)
* fix(resume): stop a failed run-buffer publish from stranding a resumed execution The run buffer is a replay convenience for stream readers; the durable execution record is authoritative. A failed terminal event publish was deciding the outcome of work that had already run: it threw, the resume was marked failed, and the paused execution was left paused forever. That path is also not retryable — the workflow had already executed — so the next resume attempt re-ran its side effects. Degrade instead. Record the terminal status on the stream meta so readers are not left polling an 'active' stream, log the failure, and let the resume settle on its real result. The same treatment applies when the terminal event is never published at all, which previously synthesized an error for the same stranding effect. Removes the now-unreachable TERMINAL_PUBLISH_ERROR constant and its branch. Pre-execution buffer failures stay fatal and retryable, since no work has happened yet. * fix(execution): give per-user Redis budget keys a fixed window The per-user byte budget refreshed its TTL on every accepted write, so for any user who never went a full TTL without writing, the key never expired. The per-execution data it accounted for kept expiring underneath it, so the counter accrued bytes Redis had already dropped and drifted toward the ceiling. On reaching it, every subsequent write for that user was rejected until they stopped writing for a full TTL — and since a rejected write does not refresh the TTL, it recovered on its own and then refilled. User keys now get a fixed window: the TTL is set when the key is created and never extended. Applied to all five Lua scripts that touch the key so the writers cannot drift apart. Execution-scoped keys keep sliding — they are refreshed on the same schedule as the data they account for, so their counter and bytes stay in step. * fix(execution): heal a user budget key that somehow has no expiry The no-op branch of the flush script dropped the user-key EXPIRE outright rather than guarding it like every other site. No current path can create the key without an expiry, but if one ever did, that branch was the one place that would never give it one — and a user counter with no expiry is the unbounded version of the bug this series fixes. Guard it instead, so the branch heals such a key rather than skipping it, and so all five scripts read the same way. * fix(stream): end a replay cleanly when terminal metadata has no terminal event Terminal metadata is the authoritative end-of-run signal — the happy path writes it atomically with the terminal event, and a run whose terminal event could not be buffered records the status on its own. The reader required both, so the degraded case threw and turned a replay that was merely incomplete into a broken one. Metadata without a matching event means the buffer degraded, not that the run is still going. Log it and close the stream: the reader has already received every event that was buffered, and the durable record is unaffected either way. |
||
|
|
13772565a3 |
fix(chat): show deployment passwords to admins (#6177)
* fix(chat): show deployment passwords to admins
* fix(chat): reject whitespace-only passwords
* fix(chat): preserve password visibility on regenerate
* fix(chat): harden password reveal and close deployment lockout paths
Follow-ups from a security review of the password reveal endpoint. The
permission model itself was correct — the reveal is gated on workspace
admin via the canonical resolver, so derived org-admin access is honored.
These address secret handling and validation around it.
- Cap set-path passwords at the same 1024 chars the chat login accepts.
Neither the input nor the schema bounded length, so a longer password
saved fine and then failed the login POST on length before auth ran,
locking every visitor out permanently.
- Discard the revealed password when the field is hidden. It previously
stayed in state and in the input's DOM value with Copy still armed, so
the field read as hidden while still handing out the plaintext.
- Evict the decrypted password from the mutation cache on unmount, and
correct the TSDoc claiming it was never retained — it sat in the
MutationCache for the default five minutes after the modal closed.
- Validate the password inside performChatDeploy, the writer both callers
must use. The copilot deploy_chat tool bypasses the route contract and
could still store a whitespace-only or over-long password, or create a
password-protected chat with no password at all.
- Stop echoing raw decryption errors from the reveal endpoint.
- Only persist a new password when the chat ends up password-protected;
PATCH { authType: 'email', password } used to re-arm the secret that the
auth-type branch had just cleared.
Also replaces the hand-rolled copy state with useCopyToClipboard, which
fixes an unawaited clipboard write that surfaced as an unhandled rejection
and a "Copied" confirmation shown even when the write failed.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(chat): correct password-change confirmation gate and stale reveal error
Addresses both open Bugbot findings.
- shouldConfirmPasswordChange keyed on "a chat exists" rather than "a password
exists", so switching a public chat to password protection for the first time
asked the admin to confirm changing a password that was never set. It now
takes the existing-password signal the component already computes.
- A failed reveal left "Failed to load the current password" on screen while the
admin typed or generated a replacement, because the mutation only drops its
error on the next attempt. Editing or regenerating now resets it.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Waleed
|
797f781855 |
chore(deps): bump @opentelemetry/core to 2.8.0 for CVE-2026-54285 (#6182)
W3CBaggagePropagator.extract() had no size limits on inbound baggage headers. Bumps the OTel core packages to 2.8.0 and the exporter/sdk-node packages to 0.219.0, which pin core 2.8.0 so no vulnerable copy is nested under them. |
||
|
Waleed
|
a76b928771 |
fix(security): bind copilot chat attachment keys to their owner (#6179)
* fix(security): bind copilot chat attachment keys to their owner `POST /api/copilot/chat` accepted a client-supplied `fileAttachments[].key` and passed it straight into `trackChatUpload`, which wrote it into the `workspace_files` ownership binding with no permission check and no key-ownership validation. That binding is what `verifyFileAccess` and the Files feature resolve authorization from, so any workspace member — including read-only — could hand in another member's key and have their file re-parented to a private chat: removed from the Files listing, from folders and from download-by-id, and destroyable through the chat-delete FK cascade. The sibling register route already enforced these invariants; the copilot path did not. - `trackChatUpload` now rejects keys that do not address the target workspace, only re-links a chat-upload row the caller already owns (matched by row id, not by raw key), and only mints a new binding when the key has no prior record at all — including soft-deleted ones, which the partial active-key unique index would otherwise let it insert over. - Minting a new binding verifies the object exists in storage, matching `registerUploadedWorkspaceFile`. - `buildCopilotRequestPayload` gates attachment tracking on write/admin, the same grant the upload routes that issue these keys require. * fix(security): make the chat-upload ownership check atomic with its write The ownership lookup and the binding UPDATE were separate statements, and the UPDATE matched on the captured row id alone. A concurrent `materialize_file` sets `context='workspace'` and clears `chatId` on that same row, so the tracking write still matched and dragged the saved file back into chat scope — hiding it from every workspace-file listing and re-exposing it to the chat-delete cascade, with materialize's storage-usage increment left stranded. Re-assert every ownership predicate in the UPDATE so the statement is itself the atomic check. The lookup now only decides UPDATE-vs-INSERT and is no longer load-bearing for authorization, which also makes the existing `updated.length === 0` fail-closed branch correct rather than dead. * fix(uploads): tolerate transient storage probes and reject unowned keys with 403 Follow-ups from a backward-compatibility audit of the attachment-key hardening. The existence probe is hygiene, not authorization — the key-format and no-prior-record guards already carry that, and a binding to a nonexistent object grants nothing readable. But `headObject` rethrows non-404 provider errors, so a transient 5xx or throttle would drop a legitimate attachment. Only a definitive not-found now rejects; a thrown error logs and proceeds on the ownership guards. This path is reached solely by >50MB multipart uploads, the one flow that persists no metadata row at upload time. The stage route mapped an ownership rejection to a 500. It is a client error; return 403 instead. * fix(security): compare-and-swap the chat binding on chat uploads Two overlapping chat requests could both observe the same claimable row with `chat_id IS NULL` and both satisfy the update predicate, so the later write silently moved the upload to its own chat — taking over the delete-cascade lifecycle of a file the first chat had already bound. Scope the update to `chat_id IS NULL OR chat_id = <target>` so the statement is a compare-and-swap: the loser matches zero rows and fails closed. The resolver applies the same rule so the update-vs-insert decision stays coherent. This also makes an upload bind to exactly one chat, matching the 409 the sibling `local-files/stage` route already returns for the same case; verified no client flow relinks a key across chats (drafts are per-chat, every retry path replays under a pinned chat id, forking copies blobs to fresh keys). * refactor(copilot): gate attachment tracking with the shared permission predicate `permissionSatisfies` is the documented single source of truth for permission comparisons and exists to replace hand-written `=== 'admin' || === 'write'` ladders. `userPermission` is typed `string` for legacy reasons, so narrow it with `isPermissionType` first — an unrecognized value fails the gate instead of ranking below every level. Behavior is unchanged for all three levels. Imported from the dependency-free `/predicates` subpath rather than `/workspace`, which would pull `@sim/db` onto the chat request path. |
||
|
Vikhyath MondretiandClaude
|
e90378a5c1 |
improvement(permissions): confine workspace role changes to existing members (#6180)
* improvement(permissions): confine workspace role changes to existing members * improvement(permissions): retry lock timeouts and audit admin upserts truthfully Bounding the row-lock wait with `lock_timeout` made a competing writer raise 55P03, which the retry set excluded — so the bounded wait failed the request where the unbounded one would have waited. 55P03 is now retried like the other contention aborts, the timeout is shortened since each attempt releases its pooled connection, and contention that outlives every attempt answers with a busy conflict instead of a generic server error. The admin member upsert recorded MEMBER_ADDED even when the conflict branch amended an existing membership, putting a join that never happened in the workspace audit trail. It now records a role change, matching the action the response already reported. Adds the missing test coverage for withTransactionRetry. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Waleed
|
9064039c19 |
improvement(logfire): scope block outputs per operation and refresh brand chrome (#6178)
* improvement(logfire): scope block outputs per operation and refresh brand chrome - gate each block output on the operations that actually return it - swap in the official Logfire mark, black tile with brand-magenta bare icon - move host to advanced mode and alphabetize the tool registry entries - add track-logfire-llm-cost and verify-logfire-token-target skills * fix(logfire): honor numeric-string limits and surface token validity fields - accept a numeric-string limit so agent-invoked calls stop silently falling back to Logfire's 100-row default - keep an hour-only UTC offset intact instead of producing +05Z - surface expiresAt and spendingCapReachedAt on Get Token Info - document pending_span as a fourth record kind * chore(logfire): regenerate tool metadata and document the step in the skill - regenerate apps/sim/tools/generated/tool-outputs.ts, which CI's tool-metadata:check requires after a tool output change - add the regeneration step and artifact-diff guidance to the validate-integration skill so the gate stops being missed * chore(skills): sync validate-integration projections |
||
|
Theodore LiandClaude Opus 5
|
8348592d38 |
fix(copilot): close fail-open write gates in agent tools (#6132)
* fix(copilot): make tool write gates fail closed The three handler-map management tools (manage_custom_tool, manage_mcp_tool, manage_skill) gated writes with `context.userPermission && perm !== 'write' && perm !== 'admin'`. userPermission is optional on the execution context, so an absent value skipped the check entirely and the write proceeded unguarded — while the server-tool router's equivalent gate is fail-closed. Both paths now share copilotToolCanWrite, built on the canonical permissionSatisfies (null/undefined never satisfies). manage_custom_tool additionally resolved its target as `params.workspaceId || context.workspaceId`, so a model-supplied workspace id won while the permission check was resolved for the context workspace — and upsertCustomTools performs no authz of its own. It now uses the server-set context only, matching its two siblings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CiHhAk2R1NryaS3R8n2yFz * fix(copilot): gate materialize_file writes and enforce the deploy mutation lock materialize_file's save/extract/import all create workspace resources, but the handler-map path has no central permission check, so a read-only member could create files and workflows through the agent. Gate on write access after param validation. assertWorkflowMutable moves into performFullDeploy / performFullUndeploy / performActivateVersion, where performRevertToVersion already had it. The check previously lived only in the deploy routes, so the copilot deploy tools — which call the orchestration functions directly — could deploy, undeploy, and activate versions of a locked workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(copilot): enforce secret-admin on env writes and gate KB indexing on usage Two more paths where the agent is a weaker route to the same write than the UI. upsertWorkspaceEnvVars was a weakened copy of the environment route's write: no per-key secret-admin check, no advisory lock, no audit row. Its only caller is the set_environment_variables copilot tool, which gates on workspace 'write' alone — so any write-level member could have the agent overwrite a workspace secret they do not administer, with nothing in the audit log and a lost-update race against the route's locked transaction. The gate now lives in the function so every caller inherits it, reusing getWorkspaceEnvKeyAdminAccess rather than restating the route's logic. knowledge_base add_file computed a billing attribution and then indexed without calling checkAttributedUsageLimits, which every upload route applies before accepting indexing work — an over-quota workspace could index without limit through the agent. The same file's query operation already gated correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(copilot): return lock denials and stop bootstrapping a legacy secret's ACL Two defects in the previous commits, both found by review. assertWorkflowMutable throws, and the three orchestration entry points awaited it outside any try/catch, so a locked workflow escaped as an exception instead of the { success: false } result the callers consume — performChatDeploy and the copilot deploy tools would have surfaced a generic 500 rather than a lock denial. performRevertToVersion already converted it; the three now do too, through a shared helper. upsertWorkspaceEnvVars derived newKeys for createWorkspaceEnvCredentials from the credential rows rather than the stored variables. A secret written before credential rows existed has no ACL, so overwriting it looked like adding a new key: it minted a credential and made the caller that secret's admin. The environment route derives newKeys from the locked jsonb read for exactly this reason, and now so does this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(env): collapse the merged test import onto one line Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm * fix(env): give the workspace env denial its own write-access message WorkspaceEnvAccessError reported "must be an admin of these secrets" for both denials, so a caller lacking workspace write to ADD a key was told to get secret-admin on a key that does not exist yet. Carry the reason and mirror the route's two messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Waleed
|
30820fcbca |
fix(copilot): guard document-style extraction against zip bombs (#6176)
* fix(copilot): guard document-style extraction against zip bombs extractDocumentStyle handed an attacker-controlled archive straight to JSZip and inflated named parts (word/theme/theme1.xml, word/styles.xml, ppt/presentation.xml, ppt/slideMasters/slideMaster1.xml) with no size bound, reachable from GET /api/workspaces/[id]/files/[fileId]/style and from the workspace VFS. Reading only a few entries is no protection: the bomb just has to live at one of those paths. It now calls assertOoxmlArchiveWithinLimits, the same guard the document parsers use, and the hand-rolled ZIP_MAGIC check is replaced by isZipShaped from that module — zip-guard is already shared this way by lib/uploads/archive.ts and lib/copilot/tools/handlers/upload-file-reader.ts. Checking each entry's size through JSZip instead would have been cheaper, since only a handful of parts are read, but JSZip reports the size the archive declares — the same attacker-controlled field a bomb lies about — so it would need to re-derive the guard's verification to be sound. The guard sits inside the existing try, so a rejection logs and returns null: the route already answers 422 and the VFS already returns null when no summary can be produced, and neither caller changes. * test(copilot): declare the ratio-test expansion instead of carrying it The compression-ratio case built a real 400 MiB string and deflated it synchronously, costing the parallel test runner memory and CPU for no added coverage. The guard reads the total the archive declares, so declaring the expansion exercises the same ratio path with a few-hundred-byte fixture. Still fails when the guard call is removed, and the file now runs in 286 ms instead of seconds. Caught by Greptile review. |
||
|
Waleed
|
51dd0df4ca |
fix(files): authorize presigned upload contexts per type (#6175)
The batch presign endpoint validated its type param against the shared seven-value upload enum while only authorizing knowledge-base, so any authenticated user could mint an S3 presigned PUT into workspace-logos, profile-pictures, execution, mothership, chat and copilot prefixes — objects that are then served unauthenticated from the app origin with a one-year public cache. The single presign endpoint had the same gap for chat, which has no authorization predicate and no client. - Batch presign now accepts only knowledge-base, and always requires a workspaceId the caller has write/admin on before anything is minted - Single presign drops chat from its accepted contexts; every remaining context has a per-context predicate - Both allowlists live in the contract module so the enforced enum and the documented one cannot drift apart again |
||
|
Waleed
|
e5d2f7d80a |
fix(realtime): stop read-only members persisting block positions (#6174)
* fix(realtime): stop read-only members persisting block positions The socket operation ACL granted the `read` role block.updatePosition and blocks.batchUpdatePositions on the premise that they were ephemeral cursor sync. They are not: both are followed by persistWorkflowOperation, which UPDATEs workflow_blocks.positionX/positionY and bumps workflow.updatedAt. A member holding only `read` on a workspace could therefore permanently rewrite the coordinates of every block of every workflow in it — a write across the read/write boundary. Live cursors ride their own `cursor-update` event, and the smooth-drag broadcast is the UNCOMMITTED position path, which returns before persisting and never consults the role table — so the read role needs no grant at all. * test(realtime): assert the role ACL against production, not a fixture The shared ROLE_ALLOWED_OPERATIONS fixture still listed the two position operations for the read role, and three tests compared that fixture against itself — so they certified whatever it said, including the grants this PR removes. They now assert checkRolePermission over the protocol's complete operation list (the fixture's copy omits subblock/variable/admin-only ops), plus one test that pins the fixture to the production ACL so the two cannot drift apart again. |
||
|
Waleed
|
ccc2ec9507 |
fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion (#6166)
* fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion DocParser handed the raw upload straight to officeparser and then mammoth, both of which inflate every ZIP entry into memory before any app-level size cap applies. The extension is only a routing hint, so a bomb-bearing OOXML archive renamed to .doc selected the one parser that skipped the guard its docx/pptx/xlsx siblings all call. Adds assertOoxmlArchiveWithinLimits to DocParser.parseBuffer, and centrally in file-parsers parseBuffer so a future parser cannot silently opt out. The guard reads the central directory's declared sizes without decompressing, and no-ops for non-ZIP buffers, so legacy OLE .doc files are unaffected. * fix(file-parsers): verify actual inflation, not just declared ZIP sizes The declared uncompressed sizes in a ZIP central directory are attacker- controlled, so a bomb can under-report them and pass the size and ratio checks untouched. officeparser and mammoth only detect the mismatch after inflating the entry in full: a 498 KB archive declaring 1000 bytes per entry drove 559 MB resident through the .doc parser and 538 MB through .docx, then failed. SheetJS and officeparser reject the container earlier, so xlsx/pptx were not affected, but doc and docx both were. Each entry is now inflated during verification under a maxOutputLength bound equal to the size it declared. Node's zlib aborts the moment output would exceed that bound, so a lying entry costs only its declared size and the inflated bytes are discarded immediately; both bomb variants now reject at +0 MB across every extension. Stored entries are checked against their own compressed size, and unsupported compression methods fail closed. Verification walks the contiguous run of central-directory records rather than the EOCD's declared entry count, since that run is what a decompression library allocates per entry — a lied-down count must not hide an entry from verification. Cost is ~0.45 ms per MB of uncompressed content (22 ms for a 50 MB archive), against parse times an order of magnitude larger. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. * fix(file-parsers): require central and local ZIP headers to agree The parsers disagree about which header to trust. JSZip skips the local header outright and decompresses using the central directory's method, while SheetJS's parse_local_file switches on the local header's method and inflates from there. An entry claiming STORED centrally and DEFLATE locally therefore took the guard's stored branch, skipping bounded inflation, and was still expanded downstream — a 398 KB archive hiding a 400 MB deflate payload. Verification now rejects any entry whose two headers disagree on compression method, and on declared sizes when the local header carries them (the data-descriptor flag and ZIP64 sentinels legitimately omit them, and those entries stay covered by the bounded inflate). Caught by Greptile review. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. * fix(file-parsers): charge hidden central-directory entries against the cap sumDeclaredUncompressedSize walked only the entry count the EOCD declares, while verification walks the contiguous run of records. JSZip's readCentralDir loops on the record signature and keeps every entry it finds — a count mismatch is explicitly not an error there — so an archive that under-reported its count could hide honestly-large entries from the total-size cap and still have the parser expand them. The sum now walks the same contiguous run as the verification pass and readZipCentralDirectoryStats, and fails closed when the run is shorter than the declared count. Caught by Cursor Bugbot review. |
||
|
Waleed
|
5686b7bb83 |
fix(realtime): enforce room access continuously, not only at join (#6170)
* fix(realtime): enforce room access continuously, not only at join File-doc, table, and workspace-list rooms authorized once at JOIN and never again, so a member whose workspace access was revoked or downgraded kept live collaborative write access — including durable Yjs document writes — for the whole lifetime of an already-open socket. The access-revalidation sweep explicitly skipped every non-workflow room. - sweep every room type, authorizing each against its own resource - share one membership policy (ROOM_MEMBERSHIP_ACTIONS) between the join check and the sweep, so a file-doc room keeps requiring write in both - gate file-doc document frames and table cell selections on the cached permission, evicting on a confirmed loss of access - re-check the cached decision before a join commits, so a join that authorized just before a revocation cannot re-enter the room - never let a join's own cache write clobber a revocation recorded mid-flight - surface room-access-revoked to clients; the file-doc editor falls back to read-only instead of accepting keystrokes that go nowhere * refactor(realtime): one shared eviction path for revoked room access The sweep and both per-frame gates each open-coded emit + leave + local-state cleanup. Route them all through evictSocketFromRoom so they cannot diverge on what eviction means; workflow keeps its historical access-revoked payload. * fix(realtime): re-check access before workspace-list room joins too The workspace-files / workspace-tables joins committed straight from their authorize result, so a join that authorized just before a revocation could put the socket back in a room the sweep had already evicted it from. Mirrors the guard the file-doc and table joins already had. * fix(realtime): order role-cache writes by read start, not write time Two authorizations can start in one order and finish in the other, so the decision written last can come from the older read. A join that authorized before a revocation but returned after the sweep's denial would bury it, handing the socket another full cache TTL of access. Every writer now takes a monotonic ticket before it queries and yields only to a later-started read. * chore(realtime): drop the test-only unguarded role-cache writer Tests can express the same setup with commitRoomPermission + a read ticket, so the cache has exactly one write path and no export without a production caller. * fix(realtime): keep handler-initiated table eviction retryable Evicting leaves the Socket.IO room synchronously, which is also how the sweep discovers work — so a presence removal failing in the per-frame path could never be retried and left a ghost collaborator until disconnect. Failed (or unconfirmed) removals now hand off to the sweep's existing cleanup lane instead of a second retry loop. * fix(realtime): re-resolve access at join commit instead of peeking the cache The pre-commit recheck peeked the role cache, which reports an EXPIRED entry as unknown and fails open — so a join stalled longer than the cache TTL could re-enter a room the sweep had already evicted it from, including a file-doc room where the next cold-cache frame is accepted as a durable write. All three joins now re-resolve the way the workflow join always has; it is normally a cache hit, since the join's own authorize just warmed it. * fix(realtime): keep the join generation guard after the access re-check The access re-resolve added in the previous commit sat AFTER the generation / superseded guard in the table and workspace-list joins, so a leave or a newer join landing during that await no longer cancelled the stale join — it would go on to leave the room the client had switched to and commit the abandoned one. The guard is now the last thing before the commit in all three handlers, as it already was for file-doc and workflow. * test(realtime): use the shared sleep helper in the new join tests check:utils bans the inline new Promise(setTimeout) form; the two stalled-join tests were the only new offenders. * fix(realtime): leave the prior table room only once the join is certain A table switch left the previous room before the access re-check ran, so a denial there aborted the join and left the client in no table room at all — silently dropped from one it may still be allowed to occupy. The leave now happens after the re-check, matching the file-doc and workspace-list joins. * fix(realtime): close the table join window between re-check and commit Moving the prior-room leave after the access re-check left Redis awaits between that check and socket.join, and superseded() only watches the join generation — so a sweep revocation landing in that window could still put a revoked socket back in the room. A synchronous cache peek immediately before the commit closes it without reintroducing the await; the authoritative resolve moments earlier wrote a fresh entry, so a differing read IS the revocation being guarded. |
||
|
Waleed
|
bf78c4c1d6 |
improvement(settings): unify save/discard across editable surfaces (#6173)
Save is now always rendered (primary, disabled until there is something to save) and Discard only appears once there is something to discard. Previously sandboxes showed no action at all until dirty — so a new sandbox had no visible Create button — while skills always showed a disabled Save and no Discard. - saveDiscardActions moves to @/components/settings and owns the one rule - adds SettingsActionChip / SettingsActionChips so the settings shell and the ReactNode-slot headers render actions through the same chip path - skills, secrets, connected credentials, custom tools and the secrets manager drop their hand-rolled Save chips for the shared helper - credential-detail drafts seed once per credential instead of re-seeding from every refetch, which was acting as an implicit discard |
||
|
Waleed
|
be8a93de81 |
fix(security): validate cloud region/project inputs and bind vertex credentials to the executing workspace (#6167)
* fix(security): validate cloud region/project inputs and bind vertex credentials to the executing workspace
Two credential-exposure bugs on the Vertex AI path.
1. `vertexLocation` reached `new GoogleGenAI({ location })` unvalidated. The
SDK interpolates that value straight into the API hostname
(`https://${location}-aiplatform.googleapis.com/`), so a value like
`attacker.tld/x` terminates the authority component and relocates the
request — with the workspace's GCP bearer token attached by the auth
client — to an arbitrary host. Reachable from agent/router/evaluator
blocks and from `POST /api/guardrails/validate` with only a session
cookie. `vertexProject` (interpolated into the URL path) and the Bedrock
region (interpolated into the endpoint hostname) had the same shape of
problem.
Adds `validateGoogleCloudLocation` / `validateGoogleCloudProject` to the
shared input-validation module and applies them, plus the existing
`validateAwsRegion`, at the provider chokepoints. Every path to the SDK
goes through `executeRequest`, so no caller can bypass them.
`azureEndpoint` already routes through the DNS-pinning SSRF guard.
2. `resolveVertexCredential` enforced only the user↔credential predicate and
never the workflow-workspace↔credential predicate that
`authorizeCredentialUse` applies on the HTTP path. A credential held in
workspace B could be pasted into a workflow in workspace A and consumed
by workspace-A principals with no access to B — including on deployed
runs, where `enforceCredentialAccess` is false and `ctx.userId` is the
workflow owner rather than the trigger caller. Service-account
credentials mint a `cloud-platform`-scoped token, so this handed
workspace A the use of workspace B's GCP identity.
The resolver now takes the executing `workspaceId` and rejects a
credential belonging to a different workspace, mirroring
`credential-access.ts`. All four executor call sites pass
`ctx.workspaceId`.
* fix(providers): normalize vertex location case before validating
Hostnames are case-insensitive, so a mixed-case location like US-Central1
reaches Google today. Lowercase it before the region check rather than
rejecting an input that currently works.
* fix(security): accept AWS European Sovereign Cloud regions in validateAwsRegion
eusc-de-east-1 is a real Bedrock-enabled region that the existing pattern
did not cover, so applying the validator to bedrockRegion would have
rejected a config that worked before. Adds the eusc-<country> partition.
|
||
|
mzxchandraandWaleed Latif
|
cd50a445fd |
feat(chat): highlight-to-chat — reference file/table selections in Chat (#6087)
* feat(chat): highlight-to-chat for file and table selections
Adds an IDE-style "add to chat" affordance so the Sim agent can reference an
exact passage of a file or a specific set of table rows/cells instead of a whole
resource.
- Two new ChatContext kinds: file_selection (inline selected text + line range)
and table_selection (authoritative row ids, optional column ids; the server
re-fetches current rows by id). Chip registry, client serializer, boundary
contract, and server resolver all extend along their existing switch(kind)
seams.
- Producers: Monaco context menu, Tiptap bubble menu, and the table grid context
menu, all using the Sim Chat block icon. Adding a selection opens the split
slideover (chat + resource) with the chip dropped straight into the input; from
a standalone Files/Tables page the context is stashed and drained on chat mount.
- Cmd+C / Cmd+V: a selection copied from a file/table rides a custom
text/x-sim-selection clipboard MIME and pastes into chat as the same reference
chip; the chat input round-trips a sole selection chip on copy/cut too.
- Guards: selection payloads are length/row/column bounded and truncated within
the schema bound; labels carry a deterministic key so distinct same-size
selections don't collide; a shared resource tab closes only once no remaining
chip references it; stale cell-range column ids drop the context rather than
dumping the full table.
* fix(chat): widen selection code fence so embedded backticks can't truncate it
Cursor: resolveFileSelectionResource wrapped the selected passage in a fixed ```
fence, so a selection that itself contained a fenced code block closed the outer
fence early and the agent received a truncated snippet. The fence is now one
backtick longer than the longest backtick run in the content (floored at three),
matching CommonMark's close rule.
* fix(chat): carry the selection chip on a column-header copy
Cursor: a column-header Cmd+C always took the async paged clipboard path, which
replaces the whole clipboard with text/plain only and can't carry a custom MIME
- so pasting a column copy into Chat couldn't rebuild the table_selection chip
that Add-to-chat produces for the same selection. When every row is loaded and
within the chat-selection cap, the column copy now does a synchronous event
write so the scoped table_selection rides alongside text/plain (mirroring the
row-'some' and cell-range paths); oversized/partially-loaded columns keep the
async plain-text path.
* refactor(chat): clean up highlight-to-chat selections
Correctness:
- Stop reporting fabricated line numbers for rich-markdown selections.
`doc.textBetween` counts ProseMirror block boundaries, not markdown source
lines, so the chip label and the agent prompt both claimed line ranges that
don't exist in the file. Line info is now emitted only by Monaco.
- Bound a table_selection's rendered markdown by characters, not just row and
column counts — 500 wide rows dwarfed the 20k-char file-selection budget.
Rows are emitted until the budget is spent, and the content says what was
omitted.
- Complete `areContextsEqual` for both selection kinds, so re-adding the same
selection dedupes while a different passage of an already-referenced file
registers as new.
Consistency:
- Carry the resource display name on the context instead of recovering it by
regex from the chip label, deleting fileNameFromSelectionLabel and
tableNameFromSelectionLabel.
- Replace the user-visible `#k3f9` hash disambiguator with a readable ordinal
applied at insert time (`Sales (3 rows) (2)`), via a shared
prepareContextForInsert used by both the add-to-chat and paste paths.
- Fold MothershipPendingContextStorage into MothershipHandoffStorage as a
chip-only handoff (optional message), removing the parallel storage class,
the second drain effect, and its StrictMode guard ref.
- Replace `window.location.assign` with `router.push`, matching the existing
"Troubleshoot in Chat" handoff.
- Collapse three near-duplicate synchronous copy branches in table-grid into
shared buildTableSelectionContext / writeLoadedRowsWithChip helpers, also
reused by the add-to-chat handler.
- Trim multi-paragraph inline comments to TSDoc stating each reason once.
Tests: new coverage for the character budget, the label ordinal, selection
equality, and chip-only handoff accumulation; each verified to fail when its
fix is reverted.
* refactor(chat): tighten table copy fallback and helper placement
- writeLoadedRowsWithChip now requires a chip to carry; with none it falls
through to the canonical paged path (preserving its row loading and
truncation notice) instead of doing a bare synchronous write.
- Reuse selectedColumnIds in the context-menu memo.
- Restore resolveTableSelectionResource's TSDoc, orphaned when renderTableCell
was extracted between the doc and its function.
* fix(chat): apply chip handoffs as one batch; widen table copy chip path
Cursor Bugbot: a multi-context chip handoff dispatched one event per context,
and insertContextChip resolved label collisions against selectedContexts read
through a ref that only refreshes on render. Each dispatch therefore saw the
same stale list, so a second same-label selection was never ordinalized and
addContext dropped it while its @token still landed in the text. The event now
carries the whole batch and insertContextChips threads each resolved context
forward as it goes.
Greptile: an explicit multi-row ('some') selection no longer requires every
selected row to be loaded before taking the chip-carrying sync path. That
gate assumed the paged fall-through would copy more, but its loadRows returns
rowsRef.current unchanged for 'some' — the same rows, minus the chip. Renamed
the parameter to to say what it actually gates on.
Remaining chip-less cases are inherent to the async Clipboard API, which
replaces the whole clipboard and cannot hold a custom MIME: a filtered
select-all (must page in more rows) and selections past the 500-row chip cap.
'Add to chat' covers both — it is not gesture-bound and drains to the cap.
* fix(chat): distinguish a line-less file-selection label from the whole file
Cursor Bugbot: a rich-markdown selection labelled itself with the bare file
name, which is exactly the whole-file chip's label. Menu-driven inserts reject
any context whose label is already taken (isContextAlreadySelected closes the
menu silently), so once a markdown selection was attached, mentioning that same
file was quietly dropped. One-way: the reverse order works because programmatic
inserts ordinalize through prepareContextForInsert.
Fallen out of dropping the fabricated line range from this editor — with no
range left, the label collapsed onto the file name. Fixed at the single source:
buildFileSelectionLabel now returns 'notes.md (selection)' when there is no line
range, so it stays honest about location while remaining distinguishable.
* fix(chat): don't revive an aged-out chip handoff when accumulating
Cursor Bugbot: pendingContexts merged a prior chip-only handoff without
checking its age, while store stamps a fresh timestamp on every write. An
abandoned handoff that had already passed max-age would therefore ride along on
the next 'Add to chat' and fire on the following navigation as if current.
Introduced by the accumulation behavior added earlier in this PR. Applied the
same freshness bar consume uses, and hoisted the 60s window into a single
MAX_AGE_MS constant so the two paths cannot drift.
* fix(chat): reference every selected row in a table chip, not just loaded ones
Cursor Bugbot: for a 'some' row selection the chip's rowIds came from the
loaded-page intersection (currentRows filtered by the selection) rather than
rowSel.ids. The chip carries ids and the server re-fetches them via
getRowsByIds, so a selected row that simply had not been paged into the grid
was silently dropped from the agent's context — select 600 rows with 200
loaded and the agent saw 200. Add to chat had the same loaded-only narrowing
through contextMenuRowIds.
Both now send the full selection, still bounded by MAX_TABLE_SELECTION_ROWS.
Only the pasted text stays limited to loaded rows, which is inherent — there
are no cell values to serialize for a row that has not been fetched.
* docs(chat): scope the table copy 'complete' comment to the text path
It read as though the whole selection were complete when ids are unloaded,
which is now only true of the serialized text — the chip deliberately carries
every selected id for the server to re-fetch.
* fix(chat): compare selection ids as sets, not sequences
Cursor Bugbot: sameIds compared rowIds/columnIds by index, but a table
selection's ids iterate in click order (they come from a Set), so the same rows
picked in a different order — or reached via a cell range rather than the
gutter — compared unequal. prepareContextForInsert then added a second
ordinalized chip pointing at rows already referenced instead of no-opping.
More reachable since the previous commit started sourcing rowIds from
rowSel.ids directly, where insertion order tracks the user's clicks.
* fix(chat): enforce the table selection budget over the whole rendered content
Cursor Bugbot: the budget subtracted only the header and divider before packing
rows, then prepended the 'Selected ...' prose and the newlines afterward, so the
final content could exceed MAX_TABLE_SELECTION_CONTENT_LENGTH whenever the last
accepted row left less slack than the prefix needed. The cap the TSDoc promises
was not actually enforced.
The prior test passed while missing this: its rows were wide enough that packing
stopped far short of the limit, so the boundary was never exercised. Replaced
with rows sized to fill the budget almost exactly, asserting both that the
content stays within the cap and that it still approaches it (so the assertion
can't pass by emitting an empty table).
Also capitalize Chat in the three 'Add to Chat' menu labels, matching the
constitution's module naming and the existing 'Fix in Chat' / 'Troubleshoot in
Chat' UI strings.
* fix(chat): don't swallow a paste whose selection chip is already attached
Cursor Bugbot: the selection-paste path called preventDefault before
prepareContextForInsert, so when that returned null (the same selection is
already a chip) the handler returned having claimed the event — no chip
inserted and no text/plain pasted either. Cmd+V did nothing.
preventDefault now waits until there is a chip to insert; a duplicate falls
through to the plain-text paste below, which is the reasonable reading of the
gesture.
* fix(chat): derive the budget reserve from the same clause it reserves for
Cursor Bugbot: worstCaseSizeClause built its own string that omitted the
row/rows word the real clause always carries, so the reserve ran ~5 characters
short and a tightly packed selection could still exceed
MAX_TABLE_SELECTION_CONTENT_LENGTH. A bug in the previous fix, from duplicating
the format instead of sharing it.
Replaced with one sizeClause(shown, omitted) used for both the up-front reserve
and the final prose, so the two cannot describe the count differently. The
reserve passes (rows.length, rows.length) — max digits on both counts and the
plural forced — which is an upper bound on any real clause.
The earlier tight-packing test could not see this: a single cell width leaves
whatever remainder it leaves, and 100 left more than 5 characters. Added a
sweep over widths 60-75 that collects overflows so a failure names the width;
it catches the reported bug at width 74 (20002 vs 20000).
* fix(chat): align MothershipChat's onContextRemove with the surface contract
The remaining-contexts argument was added to ChatSurfaceContextValue but not to
MothershipChat's own prop type, which forwards straight into it. Nothing passes
the handler there today so it typechecked (fewer params is assignable), but the
two declarations of the same wiring disagreed.
Does not change behavior: home still wires the remove handler only to the
empty-state surface, as it did before this PR.
* refactor(chat): apply cleanup pass findings
emcn design: the table context menu built its Add-to-Chat label in the parent
and never pluralized it, so right-clicking a single row read 'Add rows to Chat'
directly above 'Delete row'. Derive it inside ContextMenu from selectedRowCount
like every sibling label, with an addToChatCellScoped boolean mirroring
workflowCellScoped. Also more correct: selectedRowCount accounts for a
select-all beyond the loaded page, which contextMenuRowIds.length does not.
callbacks: drop two useCallback wrappers whose identity nothing observes — both
handleAddSelectionToChat handlers feed unmemoized components (one through an
inline arrow). buildSelectionContext stays wrapped; it is a real dep of the
copy-bridge effect.
comments: fold a duplicated rationale paragraph in handleAddSelectionToChat
left by two commits stacking, drop a {@link MothershipHandoff} that resolves to
nothing (the type is not imported there), and trim a rowIds doc that restated
its own type.
Skipped, deliberately: the effects pass proposed moving buildContext out of
useSelectionCopyBridge's deps behind a latest-ref. buildSelectionContext is
already stable, so churn is near-zero, and it would leave two sibling
useCallbacks purposeless.
* fix(chat): don't attach a selection chip to a copy from a nested input
Cursor Bugbot: a copy from a field inside the editor — Monaco's find box being
the common one — bubbles to the container while the document still holds a
highlight, so the bridge attached the editor selection to text the user never
copied. Chat paste then prefers the custom MIME and inserts a reference chip
instead of the search term.
Skips INPUT targets only. Copying the table grid's INPUT/TEXTAREA guard would
have suppressed the chip on the main copy path this hook exists for: Monaco's
own editing surface is a hidden textarea, unlike the grid's cell editors, which
really are form fields.
Tests cover both directions — chip attached from the textarea surface, skipped
from a nested input — and were verified to fail against the missing guard and
against the INPUT+TEXTAREA variant.
* fix(chat): persist the source names a selection chip renders from
Cursor Bugbot: fileName/tableName were mapped into the optimistic message and
accepted by the API schema, but PersistedMessageContext, buildPersistedUserMessage
and toDisplayContexts never carried them. After a reload a file_selection chip
fell back to its label for getDocumentIcon — and the label carries a location
suffix ('notes.md:12-40'), so extension detection broke.
Persists only the two names the display path reads. The rest of the payload
(text, rowIds, columnIds, line numbers) stays unpersisted on purpose: it exists
to resolve the selection server-side at send time, is never re-read when
rendering a past message, and would put a selection-sized blob — up to the 20k
char cap — in every stored message.
Test asserts both halves of that, and was verified to fail with the mapping
removed.
* refactor(chat): remove a needless alias and correct an eslint-disable reason
Self-audit for shortcuts, prompted by 'nothing hacky':
- handlePaste kept `const prepared = preparedSelection`, an alias added only to
avoid renaming two downstream lines. Uses the real name now.
- The home.tsx drain's eslint-disable claimed handleContextAdd is 'a stable body
function'. It is a body function, so it is a NEW value every render — the
justification was false. Replaced with the actual reason: it is omitted to keep
the drain one-shot, and doing so is harmless because consume() clears
atomically, so a re-run would find nothing.
Audited the rest of the diff for suppressions, casts and swallowed errors. The
three catch blocks are documented graceful degradations with explicit fallbacks
(row-drain failure, browsers rejecting a custom clipboard MIME mid-gesture,
malformed clipboard JSON); the one double cast is a DataTransfer stub in a test.
* fix(chat): bound the sync copy path by the text limit, not the chip cap
Cursor Bugbot: writeLoadedRowsWithChip bailed once loaded rows exceeded
MAX_TABLE_SELECTION_ROWS, but buildTableSelectionContext already slices rowIds
to that cap. So selecting more than 500 loaded rows fell through to the async
path, which cannot carry a custom MIME, and silently lost the chip — while Add
to Chat on the very same selection still produced a 500-row chip.
The two limits govern different things: the chip's cap is how many rows a
table_selection can reference, the text's is TABLE_LIMITS.MAX_COPY_ROWS. Gate on
the latter. Past it the paged path still takes over, because it owns truncation
and the accompanying notice.
* refactor(tables): extract selection-to-chip helpers into utils so they are testable
Follow-up I owed on the previous round: the copy path's eligibility rule and the
context builder were module-private in a ~4,600-line component with no test file,
so the last two fixes to them were reasoned rather than covered — and both were
wrong on the first attempt.
Moves selectedColumnIds and buildTableSelectionContext to the existing
table-grid/utils.ts (which already owns RowSelection, DisplayColumn and
getColumnId), and extracts the copy decision as canWriteRowsWithChip.
writeLoadedRowsWithChip keeps only the clipboard and toast effects, so the pure
rule can be tested without a DOM. utils.ts stays free of side effects.
New utils.test.ts covers the two limits that were conflated — a selection past
the chip cap still qualifies (the context caps its own rowIds), one past
MAX_COPY_ROWS defers to the paged path — plus the all-columns collapse and both
caps. Verified to fail against the old chip-cap gate.
* fix(chat): scope selections to the columns actually picked, and stop under-counting rows
Three findings from one Bugbot round.
Hidden columns widened cell ranges (reported twice). buildTableSelectionContext
and contextMenuColumnIds collapsed a range to an open scope when it covered
'every column', comparing against displayColumns.length — which drops hidden
columns AND expands workflow groups, so it never meant 'the whole schema'.
Selecting every visible column therefore cleared columnIds and the server
re-fetched columns the user had hidden. The collapse is removed rather than
re-based on a schema count: no count available to a caller describes the schema,
and an explicit column list is what the user actually selected. totalColumnCount
is gone from the signature.
Add-to-chat label undercounted rows. The menu derived its count from
selectedRowCount (loaded rows only) while the chip was built from the full
rowSel.ids set, so the label could promise fewer rows than were sent. Both now
read one addToChatRowIds memo, with the count passed through explicitly since it
legitimately differs from the count the delete/run labels use.
Monaco line range was off by one. A full-line highlight ends at column 1 of the
FOLLOWING line, so endLineNumber named a line that contributed no text — the
chip label and the agent prompt both claimed an extra line.
The collapse test I added last round asserted the buggy behavior as correct; it
now pins the opposite, and fails if the collapse returns.
* fix(tables): cap the Add-to-Chat label at the rows a chip can carry
Cursor Bugbot: last round's fix for the label undercounting rows introduced the
opposite error. addToChatRowCount passed the raw selection size, but
buildTableSelectionContext caps rowIds at MAX_TABLE_SELECTION_ROWS, so a
2,000-row selection advertised 2,000 while the chip referenced 500 — breaking
the same invariant the fix claimed to establish.
Routes the count through a chipRowCount helper next to the builder that applies
the cap, so the label cannot drift from the payload again.
utils.test.ts now asserts the invariant directly rather than the formula:
across 1, 42, 500, 750 and 50,000 requested rows, chipRowCount equals the
rowIds length the context actually carries. Verified to fail if the cap is
dropped.
* fix(chat): stop a removed chip lingering when its label prefixes another
Cursor Bugbot: the mention sync tests each label with a lookahead that rejects
only word characters, so '-', ')' and space all let a shorter label match INSIDE
a longer token. '@notes.md:12' matches within '@notes.md:12-40', and
'@Sales (3 rows)' within '@Sales (3 rows) (2)'. Deleting the shorter chip left
its context attached, and it was still sent with the message.
The label class is pre-existing, but this PR made it routine: line ranges and
uniqueContextLabel ordinals generate prefix pairs for any two selections of the
same file or table.
Fixed at the sync rather than by reshaping labels to dodge the prefix — a label
format chosen to avoid a matcher bug would just relocate it. Contexts are now
tested longest-label-first, and each matched token is blanked before shorter
labels are tested, so every context is judged against text its own token owns.
Blanked in place, not removed, so the (^|\s) boundary of whatever sits next to
it is preserved; prev order is still what's returned.
Shared with the workflow copilot input, so tests cover both directions: the two
prefix pairs are dropped when only the longer token remains, both survive when
both tokens are present, order is preserved, and trailing punctuation after a
mention still keeps its chip.
* fix(chat): include the line range in file-selection equality
Cursor Bugbot: areContextsEqual compared only fileId and text for
file_selection, while the comment directly above claimed equality was the
selected range. A line that occurs twice in a file — a repeated import, a
closing brace — highlighted at both places produced identical text, so
prepareContextForInsert called the second a duplicate and dropped its chip,
even though the labels (notes.md:12 vs notes.md:50) were plainly different.
Comparing startLine/endLine as well makes the code match what the comment
promised. The rich-markdown editor omits the range, so both sides are undefined
there and identical text still dedupes — correct, since two identical passages
are genuinely indistinguishable without line numbers.
Tests cover both: distinct ranges stay distinct, an exact repeat still dedupes,
and the no-line-number path still dedupes. Verified the first fails against
text-only equality.
* fix(tables): drain past the cap so exclusions can't shrink a select-all chip
Cursor Bugbot: for a gutter select-all the menu count comes from
selectedRowCount (capped), but the chip was built by loading exactly
MAX_TABLE_SELECTION_ROWS and filtering exclusions AFTER. Any excluded row inside
that prefix left the chip short of the advertised count.
Third appearance of the same invariant — label vs payload — this time in the
select-all path specifically, which the earlier fixes did not touch.
Loads the cap plus the exclusion count, which covers the worst case where every
exclusion falls inside the prefix, so the filtered result still reaches the cap
whenever the table has the rows. Extracted as drainTargetForChip so the
compensation is stated and tested rather than an inline arithmetic detail.
---------
Co-authored-by: Waleed Latif <walif6@gmail.com>
|
||
|
Waleed
|
d89ab4a8ee |
fix(security): bound response bodies in secureFetchWithPinnedIP by default (#6169)
secureFetchWithPinnedIP only capped a response body when the caller passed maxResponseBytes; with the option absent the body streamed into an unbounded Buffer.concat. Several tool proxies whose target host is user-supplied (Jupyter, ClickHouse, Grafana, 1Password Connect) and the RSS poller called it without that option, so an attacker-controlled server answering with an endless chunked body could grow the shared process heap until it was OOM-killed. Make the cap fail-safe: default to 100MB (and treat a non-positive value as the default) so there is no unlimited mode, then pass tighter explicit caps at the user-supplied-host sites. Responses that carry no body (HEAD, 204, 304) are exempted from the content-length pre-check — they advertise the resource size as metadata, which would otherwise spuriously fail a HEAD probe of a large file or an RSS conditional-GET 304. |
||
|
Waleed
|
47e8f1eb5b |
fix(ssh/sftp): cap remote file reads on received bytes, not stat() size (#6168)
The SFTP/SSH download routes buffered a remote file into memory with the only size guard being sftp.stat().size — a value the caller-supplied SSH server controls. A server that reports a tiny size and then streams endlessly drove unbounded heap growth until OOM. Read through the existing readNodeStreamToBufferWithLimit limiter, which enforces the cap on bytes actually received and destroys the stream on breach, via a small readSftpFileCapped wrapper in sftp/utils. All four SFTP-reading tool routes now share it — download, download-file, read-file-content, and the append path of write-file-content, which had no cap at all. The wrapper keeps a no-op error listener on the stream for its whole life: ssh2 rejects still-pending SFTP requests when the channel closes, which arrives as a late error event after the limiter has detached its own handlers, and an error event with no listener would take the process down. Too-large responses now return 413 to match how the rest of the routes surface PayloadSizeLimitError, and responses report the actual byte count rather than the server-reported stat size. |
||
|
|
18214158b9 |
fix(sanitization): secret exposure in function and agent trace spans (#6000)
* fix trace span secret sanitization * sanitize workflow output logs * preserve streaming usage estimates * Fix logging session test after staging merge * secrets sanitization correctness * fix(execution): address review regressions * fix(execution): harden secret trace provenance * fix(execution): preserve functional state during trace projection --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Mac.localdomain> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai> |
||
|
|
3740c62112 |
feat(library): Agentic AI Coding Tools: What They Are and How the Top Options Compare (#6164)
* feat(library): Agentic AI Coding Tools: What They Are and How the Top Options Compare * feat(library): add generated cover for agentic AI coding tools post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Waleed
|
51560390dd |
fix(styles): restore transition timing Tailwind was silently dropping (#6165)
`tailwindcss-animate` adds a `duration-*` utility for `animation-duration`, which collides with core's `transition-duration`. For a named value Tailwind emits both rules, but for an arbitrary value it cannot pick one — so it drops the class entirely and warns. The result is 52 usages across 13 files that produced **no CSS at all**. Elements marked `transition-transform duration-[1700ms] ease-[cubic-bezier(0.22,1,0.36,1)]` had neither a duration nor an easing function: they snapped instead of animating. Most of it is the landing hero. Verified by compiling the stylesheet on both sides: transition-duration: 1700ms before=0 after=1 transition-duration: 420ms before=0 after=1 transition-duration: 175ms before=0 after=1 transition-timing-function: cubic-bezier(0.22,1,0.36,1) before=0 after=1 transition-timing-function: cubic-bezier(0.23,1,0.32,1) before=0 after=1 The stylesheet grows 1,425 bytes — exactly the rules that were missing — and the 16 ambiguity warnings on every dev start go to zero. Every usage is in a transition context (`transition-opacity`, `transition-transform`, `transition-[...]`); none sits alongside an `animate-*` utility, so `transition-duration`/`transition-timing-function` is the right half of the collision in all 52 cases. |
||
|
Waleed
|
03649e934c |
refactor(dev): remove the minimal-registry escape hatch (#6163)
* refactor(dev): remove the minimal-registry escape hatch `dev:minimal` existed because the tool registry was 71-82% of every workspace route's module graph and aliasing it away was the only way to make dev bearable. The metadata work removed that reason, so the hatch now buys almost nothing: before this stack 31.7s -> 20.0s cold (-37%) after this stack 22.9s -> 20.7s cold (-10%) A 10% cold-compile win, on the run that happens once — restarts are ~4.2s either way — is not worth what it costs. `tools/registry.minimal.ts` and `blocks/registry-maps.minimal.ts` are 283 lines of hand-curated duplicates of the real registries that **nothing keeps in sync** (no lint, no CI check, no test); they are correct today only because someone remembered. And the mode is actively misleading: it silently drops ~250 services and ~280 blocks, so anything reproduced under it may not reproduce for real. Removes both files, the `SIM_DEV_MINIMAL_REGISTRY` branch from `next.config.ts` (including the whole `webpack()` hook, which existed only for this), and the `dev:minimal` / `dev:full:minimal-registry` scripts. Verified after removal: `tsc` clean, boundary + metadata + skills + monorepo gates pass, and `next dev` starts and serves the canvas at 22.6s cold / HTTP 200. * fix(setup): stop the wizard offering the removed minimal-registry mode The setup wizard prompted for a dev server on machines under 16GB and **defaulted** to `dev:full:minimal-registry` — a script this stack deletes. Anyone running `bun run setup` on a low-RAM machine would have accepted the default and hit "Script not found", which is exactly the contributor the mode existed to help. Repointed at `dev:full:capped`, which still exists and caps Node at 4GB without dropping ~250 integrations — a strictly better answer to the same question. The hints were also stale: they warned the full registry "can use 4-5GB+ on its own", which was true when a dev server sat at 11.5GB. It now sits at ~4GB, so they say that instead. Missed by an earlier sweep because the pattern searched for `dev:minimal` and `registry.minimal`, and this string is `dev:full:minimal-registry` — the two halves reversed. Re-swept across every file type for all spellings: zero references remain. Also audited every script value the wizard can return, so the class of bug is checked, not just this instance. |
||
|
Waleed
|
452d82a636 |
perf(tools): read tool metadata instead of the registry on client paths (#6155)
* perf(tools): read tool metadata instead of the registry on client paths Cuts the last four edges that pulled `@/tools/registry` into the workspace shell. Every workspace route drops ~4,700 modules: route before after /w (canvas) 6,592 1,908 -71% /logs 6,227 1,543 -75% /tables 5,903 1,217 -79% /files 5,996 1,310 -78% workspace layout 5,751 1,063 -82% Dev cold compile of the canvas, n=3, cache cleared between runs: before 32.3s / 31.4s / 30.1s RSS 9.0-12.5 GB after 22.4s / 22.2s / 21.6s RSS 7.8-9.2 GB That lands where the `dev:minimal` escape hatch measured (20.0s / 6.7 GB) without its downside — `dev:minimal` swaps in curated registries that drop ~250 services, whereas this keeps every tool working. Rewired: - `block-outputs` -> `getToolOutputsMetadata` (needed `outputs`) - `serializer` -> `getToolParams` (needed `params`) - `validation` -> `hasToolId` (needed existence only) - `tools/params` -> `getToolMetadata` (needed `params`, `oauth`, `name`) `tools/params.ts` was the stubborn one: `mcp-dynamic-args.tsx` imports only `formatParameterLabel` from it, so the whole registry rode in behind a string helper — the same shape as the `mergeToolParameters` edge cut earlier. Adds a third generated artifact, `tool-ids.ts` (~110 KB). Resolution needs only the key set, so `@/tools/metadata` and `@/tools/metadata-outputs` both resolve through it and stay independent of each other, and an existence check costs ~110 KB instead of ~4 MB. Behaviour preservation was the risk here: `getTool` resolves an unversioned name onto its newest version, and a plain key lookup would have silently reported 246 versioned tools as missing. `resolveToolId` is reproduced against the id set and differentially tested — 4,404 probes (every id, every stripped base name, and an unknown) comparing old vs new resolution and existence: 0 mismatches. `ToolWithParameters.toolConfig` and `SubBlocksForToolInput.toolConfig` narrow from `ToolConfig` to `ToolMetadata`. The only external reader is `tool-input.tsx`, which uses `.name`. * docs(tools): point the boundary skill at the three metadata modules The skill still routed `hasToolMetadata` and `getToolIds` to `@/tools/metadata`, but this PR moved id resolution into `@/tools/tool-ids`. Left as-is it would send the next caller to the 4 MB module for an existence check that costs 110 KB — the exact mistake the skill exists to prevent. Also records the two properties a caller can silently get wrong: lookups guard with `Object.hasOwn` (a bare bracket lookup returns inherited prototype members), and they resolve unversioned names (246 tools are versioned, and a plain lookup reports them missing rather than crashing). * fix(tools): cut the settings-route registry edge and fix serializer test mocks Two findings from review, both real. The settings route still reached the registry: settings/[section]/page.tsx -> settings.tsx -> (dynamic import) ee/access-control/components/access-control.tsx -> group-detail.tsx -> tools/utils.ts -> tools/registry.ts It reads `getTool(id)?.name` — metadata — so it moves to `getToolMetadata`. The earlier audit missed it because it walked only from the canvas route, and the edge hides behind a dynamic `import()` that a static walk skips. Serializer tests mocked the wrong module. `Serializer` now reads params via `getToolParams` from `@/tools/metadata`, but the tests still only mocked `@/tools/utils`, so they controlled nothing and passed because the real generated artifacts happen to agree with the fixtures. Adds `toolsMetadataMock` to `@sim/testing/mocks`, backed by the same `mockToolConfigs` as `toolsUtilsMock` so a test mocking both sees one consistent tool universe, and mocks it in the three serializer suites. Verified the mock is now load-bearing: pointing it at a sentinel param makes the three user-only-required validation tests fail, and restoring it returns all 110 serializer tests to green. Before this they passed either way. * fix(tools): freeze the tool id array handed out by getToolIds `getToolIds()` returned the module's internal array by reference, so a caller doing `getToolIds().sort()` would reorder it in place and silently corrupt every later lookup — the in-place-mutation footgun `.claude/rules/sim-react-performance.md` calls out. Frozen rather than copied: the array is consumed in loops, so copying would allocate on every call. Freezing makes the mutation throw instead of corrupt, and `[...getToolIds()].sort()` still works. Return type is now `readonly string[]`, so the mistake is a compile error rather than a runtime surprise. No caller mutates it today; this is closing the hole, not fixing a live bug. * test(tools): enforce that the two tool-id resolvers never diverge `resolveToolId` now exists twice on purpose — `@/tools/utils` resolves against the live registry (so a tool added before regeneration still resolves at runtime), `@/tools/tool-ids` against the generated id list (so client code resolves without importing 4,300 tools). Nothing structurally kept them in step; a change to versioning logic in one would silently drift from the other. `tool-metadata:check` now asserts they agree across every id, every stripped base name, and an unknown — 4,404 probes — and only after the staleness check passes, so a missing regeneration reports as staleness rather than as drift. Verified it fails: breaking resolution for `gmail*` exits 1; restoring it passes. It cannot live in a vitest suite. `vitest.setup.ts` globally mocks `@/tools/registry` to an empty map, so `getTool` resolves nothing there — a parity test written as a spec passes or fails for the wrong reason. Both facts are recorded where the code is. Both resolvers stay exported. An earlier pass here un-exported the `@/tools/utils` one as dead; `tools/utils.server.ts` imports it through a multi-line import that a grep missed, and `tsc` caught it. Its doc now says which resolver a caller should reach for instead of leaving two identically-named functions unexplained. |
||
|
Waleed
|
d6e08d38d7 |
perf(tools): generate serializable tool metadata artifacts (#6153)
* perf(tools): generate serializable tool metadata artifacts
Adds `scripts/sync-tool-metadata.ts`, which projects the executable tool
registry down to the data half nobody needs a closure for, plus typed accessors
over the result. No consumer is rewired yet — that is the next PR.
`@/tools/registry` is a ~9,000-line barrel over 4,366 tools. Each `ToolConfig`
mixes plain data (`params`, `outputs`, `name`) with closures (`request.headers`,
`transformResponse`, `directExecution`, `postProcess`), and those closures reach
every integration's SDK client and parser — which is why reaching the barrel
costs ~4,700 modules. Every client-reachable caller was audited: none of them
need a closure. They need `outputs`, `params`, or an existence check.
Two artifacts, not one. `outputs` is ~4 MB of the ~8 MB and has a single
consumer, so it is emitted separately and exposed from its own module; callers
needing only params never load it.
The data is a JSON string parsed at runtime rather than an imported `.json` or
an object literal. That is not stylistic — with `resolveJsonModule` (enabled
repo-wide) a `.json` import makes TypeScript infer a literal type for all 4,366
entries:
tsc --noEmit, baseline 12.6s
tsc --noEmit, with `.json` imports 8m07s (38x)
tsc --noEmit, with string literals 12.0s
An ambient `declare module` does not short-circuit it (measured: 8m18s), and an
object literal is the same inference work. A single string literal is one cheap
token for the compiler and the bundler, and `JSON.parse` beats evaluating the
equivalent literal at runtime.
The generator refuses to emit any function value, so shipping executable config
to the client fails loudly instead of silently. `hosting` and `schemaEnrichment`
are excluded on those grounds — both hold functions and are server-only.
Also strips empty param entries: the registry has one (`stt_deepgram_v2`, an
`undefined`) which crashes callers that read `param.type` while iterating.
`JSON.stringify` drops `undefined` on its own, so the guard is there for an
explicit `null` — which serializes faithfully and would reach consumers — and to
warn either way.
Wires `tool-metadata:check` into CI alongside the other generated-contract
gates, and ignores the generated directory in biome (it exceeds the 1 MB limit
and was being skipped with a notice on every commit).
Adds a `tool-registry-boundary` skill covering which module to import, the three
non-obvious properties of the artifacts, and how to verify an edge is actually
cut — the canvas route reaches the registry through four redundant paths, so
cutting one alone moves the module count by ~1.
* fix(tools): harden the metadata accessors against inherited keys
Review found two real defects in the generated-metadata layer.
`JSON.parse` returns an object with the normal prototype, so a bare bracket
lookup resolved inherited members: `getToolMetadata('constructor')` returned a
*function* typed as `ToolMetadata`, and `getToolOutputsMetadata('toString')`
likewise — silently violating the accessors' documented "undefined if unknown"
contract. Guarded with `Object.hasOwn`, with a parameterised regression test
over `constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__`.
The generator's no-functions scan also gave up past ten levels of nesting. Param
and output schemas nest arbitrarily, so a deeper closure would have been dropped
silently by `JSON.stringify` while generation reported success — shipping an
incomplete schema and defeating the guarantee the scan exists to provide. The
depth cap is gone; a `WeakSet` handles the cycles that exposes.
* docs(tools): tell tool authors to regenerate the metadata artifacts
A new tool now has a second registration step. Client code reads `params` and
`outputs` from the generated artifacts rather than from the registry, so a tool
added without regenerating them is registered but invisible to the UI — and CI
fails on the stale artifacts.
`add-tools` and `add-integration` are where someone actually adds a tool, so the
step goes in both, next to the registry edit and in each checklist.
* docs(blocks): note when a block change needs tool-metadata regeneration
Adding a block alone needs no regeneration — it references existing tool IDs and
changes no tool's shape. But a change that touches a tool alongside the block
does, and this is where that is easy to miss: a block's `outputs` are authored
to match its tools' outputs, and the UI now reads those from the generated
metadata, so a stale artifact makes the block's declared outputs disagree with
what the panel renders (and fails CI).
Completes the tool-authoring surface alongside add-tools and add-integration.
* docs(tools): cover tool removal in the regeneration guidance
The three tool-authoring skills said to regenerate after adding or changing a
tool, but not after removing one. Removal is equally breaking and equally
guarded: deleting a tool from `tools/registry.ts` without regenerating fails
`tool-metadata:check` (verified — exit 1), so a contributor following the skill
literally would have hit a CI failure the skill never warned about.
|
||
|
Waleed
|
9299cee58d |
perf(tools): move mergeToolParameters into a registry-free leaf module (#6152)
* perf(tools): move mergeToolParameters into a registry-free leaf module
`providers/utils.ts` imports exactly one thing from `@/tools/params`:
`mergeToolParameters`. That function performs no tool lookup at all — it merges
two plain param objects. But `params.ts` imports `getTool` from `@/tools/utils`,
which statically imports the 4,300-entry `@/tools/registry` barrel, so that
one-symbol import was dragging the entire tool registry into every module graph
that reached it.
Measured with a module-graph walk from each entry:
tools/params.ts 4,926 modules (registry reachable)
providers/utils.ts 4,926 modules (registry reachable) -> 22 modules ✅
tools/merge-params.ts 2 modules (registry NOT reachable)
`mergeToolParameters`, `deepMergeInputMapping` and `isNonEmpty` move to
`@/tools/merge-params`, which is forbidden from importing `@/tools/utils`,
`@/tools/registry` or `@/tools/params`. `params.ts` now imports `isNonEmpty`
from there; its `isRecordLike` and `isEmptyTagValue` imports became unused and
are dropped. The two consumers (`providers/utils.ts`,
`executor/handlers/pi/sim-tools.ts`) import from the new module directly rather
than via a re-export, per the no-re-exports rule.
This is preparation, not the payoff. The canvas route still reaches the registry
through three other edges (block-outputs, serializer, sanitization/validation) —
all four are redundant paths and must all be cut before the route's module count
moves. Those follow in the metadata-manifest PRs.
Behaviour is unchanged: the moved functions are copied verbatim.
* refactor(tools): make deepMergeInputMapping module-private
It was exported from `@/tools/params` and imported by nothing — a private helper
of `mergeToolParameters` that had leaked into the public surface. Since this move
created the module, the export goes with it rather than being carried forward.
Verified zero consumers repo-wide before dropping it.
|
||
|
Waleed
|
28abbc94c6 |
perf(dev): re-enable the Turbopack dev filesystem cache (5.4x faster restarts) (#6151)
* perf(dev): re-enable the Turbopack dev filesystem cache (5.4x faster restarts) `turbopackFileSystemCacheForDev` has been `false` since #5408 — a landing-page homepage redesign whose description covers hero cards, feature-card aspect ratios, eyebrow chips and a voice-input button color, and never mentions Turbopack, caching, or dev performance. It was collateral, not a decision, and it overrode the Next default (true since v16.1). It is not the flag #6078/#6080 measured. That A/B was `...ForBuild` and its conclusion stands — the build cache is a 3.2x regression and stays off. The two flags look alike and are opposite decisions; both are now commented as such. Measured on `/workspace/[workspaceId]/w`, n=3 per arm, SIGINT between runs: cache OFF 31.4s / 30.1s / 31.9s RSS 9.0-9.8 GB cache ON 5.6s / 5.6s / 5.5s RSS 4.4-5.1 GB 5.4x faster restarts, ~2x less resident memory. Cold compile against an empty cache is unchanged (~32s either way) — the cache only pays back on restart, which is the loop that actually hurts. The cache is unbounded on disk: the abandoned one on this machine had reached 78 GB across 1,848 SST files, and a stale cache is slower to read back, so left alone it erodes the win it exists to provide. `prune-turbopack-cache.ts` runs on `predev` and drops it past a cap (default 20 GB, `SIM_TURBOPACK_CACHE_MAX_GB` to override); `bun run dev:cache:prune` forces it. It never blocks `next dev` on a maintenance failure. Adds a `dev-performance` skill recording the cost model, the reference numbers, and the benchmarking method — including that stopping the server with `kill -9` mid-cache-write discards the cache and makes this exact win read as no win. * improvement(dev): chain the cache prune into dev scripts instead of a predev hook Review read the root `bun run dev` path as bypassing the `predev` hook and so never capping the newly-enabled cache. Turbo does fire `pre*` hooks — verified live, the run prints the prune before `next dev` — but the concern is fair in that the guarantee rested on package-manager lifecycle semantics that are invisible at the call site. Chaining it explicitly removes the question entirely: every `dev` variant now runs `bun run dev:cache:cap && …`, which holds on any invocation path, is visible in the command itself, and drops the three duplicated `predev:*` entries for one shared script. Verified on both paths — direct `bun run dev` and root `turbo run dev`, the latter printing: sim:dev: $ bun run dev:cache:cap && next dev --port 3000 sim:dev: $ bun run ../../scripts/prune-turbopack-cache.ts * docs(dev): document cache-corruption recovery, the cost of enabling the cache Stress-tested the failure mode rather than assuming it: deliberately corrupting an SST block makes Turbopack abort with a FATAL panic — it does not self-heal. FATAL: An unexpected Turbopack error occurred. Cache corruption detected: checksum mismatch in block 4 of 00000221.sst `bun run dev:cache:prune` and restart fixes it; verified the canvas serves 200 again afterwards. Documented in the skill and in the script's header, since the symptom is a hard crash and the remedy is not guessable. This is the honest cost of turning the cache on. It is worth paying — a 5.4x faster restart against a rare, loud, single-command failure — but it should be written down rather than discovered. Worth distinguishing from the adjacent case: an ordinary hard kill does *not* corrupt the cache. Turbopack discards a partially-written cache and rebuilds it silently, which is exactly why a `kill -9`-based benchmark reads as "no cache win" (noted in the benchmarking section). * refactor(dev): drop the dev-performance skill, keep its findings at the code A whole skill was too much for what this is. The parts that are load-bearing — why the two lookalike cache flags are opposite decisions, the measured numbers, the corruption remedy, and the benchmarking trap — now live in the config and script they describe, where someone changing the flag actually reads them. The trap is the piece worth keeping: `next dev` compiles on demand so startup time is meaningless, and stopping the server with `kill -9` makes Turbopack discard a partially-written cache and rebuild silently — which reads as 'the cache does nothing' and is how this flag stayed wrong for a month. Dropped rather than relocated: generic advice that was not specific to this repo (antivirus, Docker-on-macOS, orphaned processes) and a measured no-op (`optimizePackageImports` for lucide-react changed nothing, 31.6s vs 31.7s). * docs(dev): record the measured cost and concurrency behaviour of cache pruning Stress-tested the maintenance path rather than assuming it is free. Cost: the size walk is ~30ms on a real cache and ~85ms at 2,000 files — under 2% of a 4.2s warm restart, and invisible against a cold one. It runs before every dev start, so it needed to be cheap; it is. Concurrency: pruning while a dev server is live (which happens when a second server is started from the same checkout) does not crash it. The running server keeps its in-memory state and kept serving HTTP 200 with zero panics. It does stop persisting for the rest of that session, so its next start is cold once — verified recovering at 23.4s then 4.5s. Worth writing down because the directory silently never reappears mid-session, which looks like a bug if you go looking. The cap is a backstop, not routine: a normal session sits at 1-2 GB against a 20 GB default. * fix(dev): cap every app's Turbopack cache, not just apps/sim `apps/docs` is a Next app too (`next dev --port 3001`) and overrides nothing, so it uses the Next default where the dev filesystem cache is on. It already had an uncapped 1.1 GB cache here, and the root `bun run dev` (`turbo run dev`) starts it — so a teammate using the documented command was accumulating a cache nothing would ever prune. The script now resolves its target from the working directory instead of hardcoding `apps/sim`, and each app chains its own cap. Per-app rather than one sweep on purpose: a single pass would let one app's dev start delete a cache another app is holding open, which costs that session its persistence. Verified both: `apps/sim` and `apps/docs` each report and cap their own 1.1 GB cache, and both dev servers start clean (`Ready in 299ms` / `229ms`, docs serving). * refactor(dev): drop dev:cache:prune in favour of the existing dev:clean `dev:cache:prune` duplicated `dev:clean`, which already existed in `apps/sim` and does strictly more (`rm -rf .next/dev/cache` covers the Turbopack cache plus the fetch and image caches). Two commands for one job is worse than one, and the docs pointed at the newer, narrower of the two. Removes it from both apps and gives `apps/docs` the `dev:clean` that `apps/sim` already had, so the recovery command is the same everywhere. `dev:cache:cap` stays — it is the chained step, used by more than one dev variant, and naming it keeps the relative script path out of each command. Verified `dev:clean` is a real remedy: corrupt a cache block, run it, restart — canvas serves 200 with no panic. Also corrects an overstatement. A damaged cache does not *always* abort Turbopack; whether it panics depends on whether the damaged region is read, so it is not reliably reproducible. Both notes now say "can abort" and give the same remedy either way. |
||
|
Justin Blumencranz
|
273ca06ddb |
fix(files): serve rendered documents instead of source code (#6139)
* fix(files): serve rendered documents to attachments and workflow reads Generated documents store their generation source under a .pdf/.docx name and keep the compiled binary in a separate content-addressed artifact store, so any consumer doing a raw read handed out source text under a document name. - Route attachments and readUserFileContent through the servable resolver so they get the compiled artifact, and stop the internal generation-source MIME marker reaching providers as a content type. - Render on read when the artifact is missing. The artifact key is (workspace, source hash), so forking a workspace, moving a file, or editing the source outside a recompiling writer orphaned it permanently and reported "still being generated" forever. Rendering self-heals those and stores the result. - Fall back to serving stored bytes as application/octet-stream when a render fails, instead of failing forever, and remember not to retry those bytes. - Only compile without a workspace context when the file's type positively says it is generation source, so unrelated stored bytes are never executed. - Make the doc-not-ready error opt-in per caller and give it a 409 via HttpError, so output decoration degrades instead of failing completed work. * fix(files): keep render failures retryable and never relabel unrendered bytes Addresses the first review round. - Only memoize a render failure when it is deterministic. A DocCompileUserError means the source will never render, so remembering it is safe; sandbox outages, timeouts, and cancellations are transient and were stranding valid documents for the life of the process. Infra failures now propagate, which also restores DocCompileUserError reaching callers that map it to 409. - Refuse to hand back bytes the resolver could not render. readUserFileContent returns a string, so the resolver's honest application/octet-stream could not travel with it and attachment builders re-inferred a document MIME from the filename — shipping generation source to a provider as a PDF. The file-serve route keeps the graceful passthrough, where a human downloading the bytes is useful. - Normalize the declared type once so a padded or upper-cased source marker cannot pass the resolver gate on one code path and fail it on the other. - Import the doc-not-ready guard lazily. The static import pulled the doc-compile module graph (remote sandbox, task runner, execution limits) into every hydration consumer and broke an unrelated test's module mock in CI. * perf(files): coalesce concurrent renders of the same missing artifact An artifact miss is identical for every concurrent reader — a freshly forked workspace whose document several viewers open at once, or one request whose blocks read the same file — and each was paying for its own compile of the same bytes. Share one in-flight render per (workspace, source, ext) key and drop the entry as soon as it settles, so a later read still re-renders normally. * fix(files): refuse unrendered bytes at the download boundary Addresses the second review round. - Throw UnrenderableDocumentError from downloadServableFileFromStorage instead of returning bytes with an `unrendered` flag. Around 45 call sites (email attachments, cloud uploads, zip entries, provider attachments) receive only a Buffer and re-infer the type from the filename, so a flag they must remember to check is a flag they will not check. Those callers already handled the previous not-ready throw, so failing is the shape they expect. The file-serve route is unaffected — it resolves bytes directly and keeps the graceful passthrough, where a human downloading the file has a use for it. - Surface that failure through hydration: with throwOnDocNotReady set, the caller cannot use a file with no content, so an unrenderable document now reaches it verbatim instead of degrading to null and reporting a misleading "may exceed size limit or no longer accessible". - Stop a shared render inheriting one caller's cancellation. The coalesced run no longer carries any caller's signal; each caller races its own instead, so an aborting reader gives up promptly while the render finishes for the others and still lands in the cache. * fix(files): move the unrenderable error out of the 'use server' module file-utils.server.ts carries 'use server', whose exports must all be async functions, so exporting an error class from it failed the production build with 67 cascading errors. The class now lives in the plain file-utils.ts beside the other shared file helpers, which also lets the hydration path import it directly instead of through a dynamic import. Also bounds how long a failed render is remembered. The isolated-vm engine cannot tell a bad source from a sandbox outage, so a permanent entry let one transient failure block re-rendering that source for the life of the process. Entries now expire after five minutes: long enough to stop a read loop spending a sandbox run per read, short enough that an outage self-heals without a deploy. * fix(files): finish the render cancellation and failure-surfacing edges - Race the E2B render against the caller's signal too. Only the isolated-vm branch did, so an aborted request on the E2B path waited for the sandbox to finish and could return a success the caller no longer wanted. - Attach a terminal handler to the shared render. Every caller races it against its own signal, so all of them can walk away; a later rejection with no waiters left would otherwise surface as an unhandled rejection. - Stop narrowing what throwOnDocNotReady rethrows. readUserFileContent now runs document compiles and can fail in ways this module has no business enumerating; narrowing produced three consecutive review rounds of "this particular failure is still swallowed". The flag means "do not degrade". - Do not mark an unrendered response immutable. The serve route caches versioned responses for a year, which would pin a one-off render failure to that URL long after a later compile succeeds on the same version. * revert(files): drop the concurrent-render coalescing The coalescing was an optional efficiency win — rendering is content-addressed and idempotent, so duplicate concurrent renders produced the same artifact and cost only extra sandbox time on an artifact miss. It bought that at the price of the most intricate code in the change set, and produced three concurrency findings across two review rounds: a shared render inheriting one caller's cancellation, an E2B/isolated-vm asymmetry in how the signal was raced, and orphaned rejections once every caller could race away. Removing it also restores true cancellation on the isolated-vm path: the caller's signal now reaches runSandboxTask again, so an abort cancels the sandbox work rather than only abandoning the wait for it. * fix(review): simplify generated document attachments * fix(files): mock servable downloads in hydration tests * fix(files): preserve rendered attachment semantics * fix(files): preserve cached artifact size * fix(files): refuse unresolved xlsx source * fix(files): resolve execution artifact workspace |
||
|
Waleed
|
06506bbd38 |
improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path (#6149)
* improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path - Cache the agent-stream ProseMirror↔Yjs binding metadata per session and reuse it across streamed frames instead of rebuilding it (O(doc)) every frame; matches how y-tiptap's own binding maintains the mapping in place. Safe because the shadow doc only ever sees the agent's own reconciles. - Default createFileResponse to a private, no-cache policy so auth-gated bytes are never stored in a shared cache; genuinely-public serve routes opt into public caching explicitly. - Serve content-addressed (key=) embedded images with an immutable private cache to avoid re-downloading them on every doc re-open; fileId= embeds and public shares keep revalidating (the underlying key can change / a share can be revoked). - Drop the dead conflict.version field from the collab-doc persist result and remove the wasted getWorkspaceFile re-read on the conflict path (the relay treats missing and conflict identically and never read version). - Decorate-sort the files/folders lists (compute each sort key once) and index the move-menu subtree build (O(N) vs O(N^2)); ordering is preserved exactly. * fix(files): serve inline images with no-cache so deletion/authorization is enforced per request Embedded images are authenticated content whose backing file can be deleted or have its access revoked at any time. Both key= and fileId= embeds serve private, no-cache, must-revalidate, so every request re-runs the server-side deletion/authorization check instead of serving a possibly-stale image from cache. |
||
|
Waleedandmzxchandra
|
10bfb5d139 |
feat(realtime): shared room spine + live Files/Tables collaboration + Yjs document editing (#5991)
* feat(realtime): add shared room identity + authorization spine (#5929) Introduces the foundation for a unified realtime "room" model spanning the Socket.IO presence server (apps/realtime), the durable SSE event log, and the ephemeral pub/sub fanout — all of which today reinvent their own room identity, naming, and authorization. - @sim/realtime-protocol/rooms: RoomRef { type, id }, ROOM_TYPES, and a roomName/parseRoomName codec. WORKFLOW deliberately maps to the bare id so the ~40 existing io.to(workflowId) callsites and presence state keys are unchanged; every other room type is namespaced so id spaces cannot collide. - @sim/platform-authz/rooms: authorizeRoom(userId, room, action) generalizing the exemplary authorizeWorkflowByWorkspacePermission — one resource->workspace resolver per room type, then the shared resolveEffectiveWorkspacePermission + permissionSatisfies gate. Pure foundation, no behavior change: nothing consumes these yet. Prune graph stays at 14/25 (platform-authz already depended transitively on realtime-protocol via apps/realtime). * refactor(realtime): generalize presence server to multi-room [2/N] (#5930) * refactor(realtime): generalize presence server to multi-room (RoomRef) Generalizes the Socket.IO presence layer from single-workflow-room-per-socket to a domain-neutral, multi-room-per-socket model keyed by RoomRef, so a second domain (workspace files, next PR) can reuse the same membership + presence engine. Behavior-preserving for workflow collaboration. IRoomManager is now domain-neutral (addUserToRoom/removeUserFromRoom/ getRoomForSocket/getRoomUsers/updateUserActivity/... all take a RoomRef). The workflow lifecycle broadcasts (deletion/revert/update/deploy) move out of the manager into WorkflowRoomService, composed over the generic manager. Backward-compat by design (no workflow migration, no regression): - Workflow Socket.IO room name stays the bare workflowId (roomName() maps workflow -> bare id), so the ~40 io.to(workflowId) callsites are untouched. - Workflow Redis presence keys stay workflow:{id}:users/:meta (the type prefix IS "workflow"). Multi-room correctness (from adversarial audit): - socket:{id}:workflow single-value key -> socket:{id}:rooms HASH (type->id). - The SHARED socket:{id}:session key is deleted only when the socket leaves its LAST room (refcount via HLEN) — a leave from one room no longer breaks the other room's handlers. - disconnect enumerates the socket's stored rooms and rebroadcasts presence per room, instead of picking an arbitrary socket.rooms entry. - presence broadcasts use a per-room-type event name (workflow keeps the bare presence-update; others are namespaced). Workflow handlers wrap manager calls with a shared workflowRoom(id) helper; UserPresence.workflowId -> room (the client never reads that field). Tests: existing 112 realtime tests pass unchanged (behavior gate) + 7 new multi-room tests (refcounted session, presence isolation, multi-room disconnect, per-type event names). tsc clean, boundaries + prune (14/25) green. * fix(realtime): harden multi-room disconnect + id-guard room removal Two fixes from an adversarial regression audit of the multi-room refactor: - Disconnect now handles `disconnecting` (where `socket.rooms` is still populated and authoritative) and falls back to the live Socket.IO room set for any room the manager's stored state no longer tracked. This restores reliable presence cleanup + departure broadcast even if the Redis `socket:{id}:rooms` key was evicted or TTL-expired — the one behavioral gap vs the pre-refactor disconnect. - REMOVE_ROOM_SCRIPT now only drops the socket's room mapping (and runs the last-room session cleanup) when the stored id matches the room being removed, matching the memory manager's existing id guard. Prevents a mismatched-room call from wiping a different room's mapping or the shared session. +1 test (id-guarded no-op removal). 120 realtime tests pass, tsc clean. * fix(realtime): only rebroadcast disconnect-fallback rooms whose removal succeeded Greptile 4/5 follow-up: the disconnecting-time fallback ignored removeUserFromRoom's boolean and rebroadcast presence even when the removal reported false. Now it only treats a room as removed (and rebroadcasts) when the manager confirms it — symmetric with removeSocketFromAllRooms, which already only returns rooms it actually removed. * fix(realtime): exclude the disconnecting socket from its farewell broadcast Greptile follow-up (transient-Redis-failure edge): if removeUserFromRoom fails on disconnect, the socket's presence entry can outlive it (room hashes have no TTL) and reappear as a ghost. Disconnect now broadcasts a correction to EVERY room the socket was in (union of the manager's removed rooms and the live Socket.IO membership) and passes the disconnecting socket id as excludeSocketId, so it is never shown as a collaborator regardless of whether the Redis delete succeeded. Any orphaned entry is still reclaimed by the next join's stale-presence sweep. broadcastPresenceUpdate gains an optional excludeSocketId; normal broadcasts are unchanged. +1 test. * fix(realtime): make presence broadcasts liveness-aware (root-cause ghost fix) Presence broadcasts now reconcile the stored list against the live Socket.IO membership (io.in(room).fetchSockets()) before emitting, via a shared filterVisiblePresence helper. This closes the residual behind the earlier disconnect fixes: an entry orphaned by a failed removal (room hashes have no TTL) could reappear in a LATER join's presence snapshot until the 75-min stale sweep. Now such an entry is never emitted, because a non-live socket is filtered out of every broadcast. Combined with excludeSocketId (which handles the disconnecting socket, still momentarily live). Fail-safe: on a fetchSockets throw or an empty result while entries remain, emit the unfiltered list rather than hide live collaborators. Also drops a dead guard in the disconnect union loop (rooms already removed are skipped by the wasInRooms check) and the now-unused isSameRoom import. +1 ghost-guard test. 122 realtime tests pass. * feat(files): live presence avatars + live file tree via realtime rooms (#5932) * refactor(tables): adopt shared durable event-log core (#5934) * fix(realtime): address post-merge review-comment findings (#5937) * fix(realtime): address post-merge review-comment findings A re-audit of every inline review comment on the merged stack surfaced real issues that the thread-resolutions and prior audits missed. Fixes: Presence server (#5930 comments): - connection.ts: snapshot `socket.rooms` SYNCHRONOUSLY before the first await. Socket.IO clears the room set once the synchronous part of a `disconnecting` handler returns, so reading it after `await removeSocketFromAllRooms` saw an empty set — the eviction fallback was dead. (Cursor: "Disconnect fallback misses live rooms".) - workflow-room-service: restore the original managers' final unconditional room-state wipe via a new `deleteRoom(room)` manager method, so a deleted workflow leaves no lingering presence/meta even if a per-socket removal failed or a socket joined mid-teardown. (Cursor: "Deletion skips final room wipe".) Files (#5932 comments): - workspace-file-manager.uploadWorkspaceFile now fans out the live-tree signal (all direct-upload paths: multipart fallback, copilot create, /api/files/upload, v1 files — the presigned path already notified). (Cursor: "Creates miss live tree fan-out".) - use-workspace-files-room: clear the pending retry timer on join success; and a module-scoped intended-room guard defers the unmount `leave` so a rapid remount re-claims the room and skips a stale leave — fixing presence flap + a leave-after-join race. (Cursor: "Retry timer survives join success" + "Remount churns files presence".) - workspace-files handler: roll back a partial join (leave room + remove presence) in the catch, mirroring the workflow join. (Cursor: "Join failure skips membership rollback".) +2 tests (deleteRoom). 127 realtime tests pass, both apps tsc clean, api-validation + boundaries green. * fix(files): scope workspace-files leave to a workspace (deferred-leave safety) Self-review of the deferred-leave guard found a real bug: leave-workspace-files was not workspace-scoped, so after a workspace switch (A->B) the deferred leave from A would evict the socket from its new room B. The leave now carries the workspaceId and the server no-ops if the socket's current files room differs. Also excludes the leaving socket from the leave broadcast (consistent with disconnect). * fix(realtime): close files-room presence leak + validate join payload Architecture-audit findings: - S1 (real Redis leak): the files room inherited the shared manager but not the workflow join's liveness sweep, so an UNGRACEFUL disconnect (pod crash — no `disconnecting` event) left its presence entry in the no-TTL room hash forever. Added a shared `sweepStalePresence(manager, room)` (fetchSockets liveness + remove not-live-AND-stale entries, matching the workflow 75min threshold) and run it on files join; also filter the join ack through `filterVisiblePresence` so a joiner never briefly sees an un-swept ghost. - S2: validate the client-supplied `workspaceId` on files join before it reaches the DB query (matches the /api/workspace-files-changed guard; fails closed). - N2: corrected the notify doc — it is awaited (guaranteed dispatch before a Node route returns) and hard-bounded to NOTIFY_TIMEOUT_MS, not "never block". +1 test (sweepStalePresence keeps live/fresh, reclaims not-live-stale). 128 realtime tests pass, both apps tsc clean, biome clean. * fix(realtime): workflow-deletion always notifies + cleans by socket.io membership Review-round findings on #5937: - Always emit `workflow-deleted` (was guarded by users.length>0), so a socket still in the Socket.IO room after a Redis presence eviction is told the workflow is gone before socketsLeave kicks it — the editor no longer keeps showing a deleted workflow. (Cursor: "Silent kick skips deletion event".) - Clean per-socket state for the UNION of live Socket.IO members and presence-tracked sockets, so an evicted/late-joined socket's room mapping + session are dropped too — not just presence-snapshot sockets. (Greptile: "Room deletion leaves reverse state".) - deleteRoom now logs AND rethrows on Redis failure (like addUserToRoom) so a failed wipe isn't reported as a clean deletion; the request surfaces it. (Greptile: "Room deletion failures are suppressed".) The two "deferred leave drops new membership" P1s were already fixed by the workspace-scoped leave in a prior commit (leave carries { workspaceId }; server no-ops on mismatch). 128 tests pass, tsc + biome clean. * refactor(files): drop module-scoped deferred-leave; rely on workspace-scoped leave Removes the one non-idiomatic construct (a module-level mutable `intendedFilesWorkspaceId` + queueMicrotask). It only guarded a same-workspace CONCURRENT remount, which doesn't occur in production (folder nav is shallow/no remount; list<->detail is sequential) — a dev-StrictMode-only case. The real cross-workspace race is already handled by the workspace-scoped leave: if B's join runs first (auto-leaving A), A's leave no-ops because the socket's current files room is B. Simpler, idiomatic, prod-correct. * feat(realtime): Yjs relay server for collaborative document editing [4/N] (#5941) Server-side Yjs relay for collaborative document editing (live carets + text selection) in the Files rich-markdown editor. Faithful y-websocket-style relay over the existing authenticated Socket.IO connection + shared room abstraction; in-memory Y.Doc + Awareness per file; awareness ownership binding, userId-keyed client-id uniqueness, seeder election with deadline re-election, concurrent-JOIN generation guard. 25 relay tests. Reviewed to Greptile 5/5 + Cursor pass across multiple rounds, plus an independent 4-lens audit (correctness/security/conventions/simplicity) and /simplify + /cleanup passes. * feat(files): collaborative document editing — client provider + editor (#5946) Client Yjs provider (FileDocProvider over the authenticated socket) + TipTap Collaboration/CollaborationCaret wiring for live carets + text-selection in the Files rich-markdown editor. Collaboration is a Files-page-only surface (explicit `collaborative` opt-in), disjoint from agent-streaming. Read-only + autosave-gated until synced+seeded. Merges into the realtime-rooms integration branch. * feat(tables): live collaboration — cell-selection presence + live mutation propagation (#5957) * feat(tables): live cell-selection presence — protocol + server + client hook The realtime spine for Google-Sheets-style table presence (mode A, socket): - @sim/realtime-protocol/table-presence: centralized wire protocol (events + TableCellSelection {anchor, focus, editing} + payloads) so server emits and client subscriptions can't drift. - ROOM_TYPES.TABLE + resolveTableWorkspace registered in ROOM_WORKSPACE_RESOLVERS (tableId -> workspace via userTableDefinitions, honoring archivedAt); roomName / presenceEventName / disconnect cleanup / authorizeRoom all derive automatically. - apps/realtime/src/handlers/tables.ts: join/leave (mirrors workspace-files) + a table-cell-selection relay (mirrors the workflow selection channel), broadcasting via roomName(room) since table rooms are namespaced. UserPresence gains a cell field threaded through the memory + Redis managers (Lua ARGV[7], null clears). - Extracted the duplicated resolveAvatarUrl into handlers/avatar.ts. - use-table-room.ts client hook: joins over the shared socket, tracks the roster (avatars) + patches per-socket cell deltas, exposes a throttled emitCellSelection. Grid UI (avatars + selection overlay) lands next; concurrent cell-value edits (last-write-wins via the durable log) are the follow-up PR. * feat(tables): render live cell-selection presence in the grid Wires the table presence room into the grid UI: - Page (table.tsx): useTableRoom (gated off in embedded/mothership mode) — renders <PresenceAvatars> in the header and passes remoteSelections + emitCellSelection down to the grid. - Grid emits its local selection: an effect resolves the index-based anchor/focus to stable (rowId, columnId) via refs and broadcasts it (with an editing flag for the active cell) through the throttled emitter. - RemoteSelectionOverlay: draws each remote viewer's selection in their color (getUserColor), a darker fill while editing, and name-on-hover — measured from live cell rects in the content wrapper's space (scrolls with the grid), hidden when rows are virtualized off-window, pointer-events-none so it never blocks cell clicks (hover via pointer hit-test). * test(tables): cover the table presence handler Mirrors workspace-files.test.ts: join auth/unavailable/denied/success, plus the cell-selection relay (asserts it persists via updateUserActivity and broadcasts on the namespaced roomName, not the bare id) and leave. * feat(tables): propagate manual cell edits live (last-write-wins) A manual row edit now appends a lightweight 'edit' event to the durable table stream; collaborators refetch the row (via the existing debounced rows-invalidate the job events use) so the winning value shows live. The event carries no value — peers refetch in their own wire format, so there's no auth-specific value translation on the wire, and last-write-wins falls out of the DB's committed order (the Google-Sheets model). Edits that also trigger a dispatch already emit dispatch/cell events; the debounce coalesces the two. * refactor(tables): apply /simplify findings - Drop the dead 'add unknown peer' upsert branch in use-table-room (Socket.IO ordering guarantees a peer is in the roster before their selection delta). - TableCellSelectionBroadcast = TablePresenceUser & { cell } (was a copy-paste). - Make TableGrid's presence props required + drop the unused empty-default/guard (only table.tsx mounts it, always passing both). - Drop the unused rowId from the 'edit' event (the handler invalidates all rows). - Overlay: subscribe scroll/resize/pointer listeners once per scroll element and cache the wrapper origin, so incoming deltas re-measure without re-subscribing and the pointer hit-test never forces a per-move layout read. - Server: cache the immutable socket session so a selection delta no longer reads it from Redis every time. * refactor(tables): apply /cleanup findings - Fix the remote-selection name label contrast: text-white is unreadable on the light-pastel user colors (same bug the Files caret fixed) → fixed dark #1a1a1a. - Re-measure via useLayoutEffect so a moving peer selection updates before paint (no one-frame position lag). - Drop 'mothership' from a comment (constitution copy rule). Six cleanup passes ran (effect, memo/callback, state, react-query, emcn, comment); the rest confirmed clean — all state/memos/callbacks/effects are load-bearing, presence correctly lives in useState (socket-pushed), and the edit→rows-invalidate granularity is right. * feat(tables): propagate every table mutation live (edit + schema signals) Comprehensive live collaboration for all user table mutations, via two value-less durable signals + named helpers (signalTableRowsChanged / signalTableSchemaChanged): - edit (rows refetch): single + batch row create, cell/row update, batch update, delete by id/filter, and upsert. - schema (definition + rows refetch): column add/update/delete, workflow-group add/update/delete, table rename, and CSV import (which can add columns). - Client handles 'schema' by invalidating the table detail (exact) + rows. Execution paths (column run, cancel-runs) and async jobs (delete/import-async, job-cancel) already propagate via cell/dispatch/job events — verified applyJob refetches on terminal. No reorder routes exist. Table archive (route DELETE) is a deliberate follow-up: it needs a table-deleted redirect event, not a refetch signal (which would 404). * refactor(tables): apply comprehensive /cleanup audit findings Holistic + react-query + comment audits over the whole PR: - Security/crash fix: a remote peer's rowId flowed unescaped into the overlay's querySelector — a hostile id ('x"]') threw SyntaxError inside a useLayoutEffect, crashing every other viewer's page. CSS.escape it, and validate + whitelist the untrusted cell payload server-side (shape + 200-char id bound) before it is stored/rebroadcast. - Simplify the CELL_SELECTION relay: the delta attached userId/userName/avatarUrl that the client discarded (identity comes from the roster). Drop them + the getUserSession lookup/cache entirely — the delta is now { socketId, cell }. - React Query: schema handler also invalidates lists() (parity with the local column-mutation set); document that the mutating client self-refetches by design. - Comment tightenings; biome fixed a stale import order in workspace-files.ts. * fix(tables): broadcast single-cell selections (focus falls back to anchor) Cursor High: a normal cell click leaves selectionFocus null (the grid treats it as a one-cell selection via focus ?? anchor), but the presence emit required BOTH anchor and focus to resolve — so the most common selection never broadcast and clicking even cleared a prior remote outline. Mirror the grid's focus ?? anchor semantics. * fix(tables): reviewer + regression + per-LOC audit findings Cursor review round (5 findings) + regression audit + per-LOC audit: - Presence roster snapshot now KEEPS the cell we already hold for a known socket, so a join/leave broadcast can't revert a fresher CELL_SELECTION delta. - Reset the selection throttle on table switch (was unmount-only), so a pending selection for table A can't flush into table B's room after a switch. - Metadata writes (column widths, display) use a new lightweight 'metadata' signal that refetches only the definition — a resize no longer forces peers to refetch rows. - Overlay re-measures on row add/remove/reorder via a tbody childList MutationObserver (a live refetch moves cells without a scroll/resize). - Document the actor self-refetch create caveat (scrolled multi-page insert) accurately. - isCellRef narrows to a partial instead of casting to the full type then re-checking; drop a redundant mount measure() (the layout effect covers it); text-[11px]→text-xs. * fix(tables): drop ineffective metadata propagation + re-measure overlay on column resize Cursor round on |
||
|
Bill LeoutsakosandBill Leoutsakos
|
ecf1d7dbbb |
refactor Instagram integration architecture (#6143)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Waleed
|
19b0312a04 |
feat(managed-agent): add session lifecycle operations (#6140)
* feat(managed-agent): add session lifecycle operations
Adds an operation selector to the Claude Managed Agents block, backed by
nine new tools alongside the existing run-session behavior:
- create session (non-blocking, seeds initial_events)
- send message to an existing session
- get session (surfaces tool calls awaiting approval)
- list events
- update session (title/metadata)
- interrupt session
- respond to tool confirmation (allow/deny)
- archive session
- delete session
Run Session stays the default, so blocks saved before the selector
existed keep their exact behavior and field layout.
* fix(managed-agent): address review findings on session lifecycle ops
- List Events kept the OLDEST slice when capped, dropping the agent's most
recent reply. Paging is exhaustive again and the cap now keeps the newest
N after ordering, with a `truncated` flag so callers know it is a tail.
- listPaginated returned whole pages past maxItems; it now trims to the
exact cap.
- A whitespace-only title passed the update guard and would have cleared an
existing session title. Blank is now treated as not provided.
- Interrupt had no request timeout and could hang; bounded at 15s while
still honoring the workflow signal.
- Custom-tool gates were surfaced by Get Session but could not be answered,
since they need user.custom_tool_result rather than a confirmation. Adds a
Respond To Custom Tool operation and a `kind` on each pending gate so a
workflow routes to the right one.
- Docs rendered a raw ${DEFAULT_EVENT_LIMIT} placeholder for the default.
* fix(managed-agent): correct truncation flag, gate lookup, custom tool result
- `truncated` was true whenever the history size equalled the limit, even
though nothing was dropped. Event reads now report the untrimmed total and
the flag compares against that.
- Pending-gate enrichment capped its read, which keeps the OLDEST events in
page order — the opposite of where blocking gates live. It now filters to
the ids being looked up as pages arrive, which is both correct regardless
of page order and bounded by the id count. Paging continues on the raw
page so a fully-filtered page is not mistaken for the end of the list.
- Respond To Custom Tool applied one result to every id, so multiple pending
tools would all receive the same output. It now answers a single call per
invocation.
* fix(managed-agent): stop fractional event limits reading unbounded
A limit below 1 passed the positivity check and then floored to 0, which
made `slice(-0)` hand back the ENTIRE history flagged as complete — the
opposite of the requested bound. The limit is now floored before it is
validated, so anything that does not resolve to a positive integer falls
back to the default.
Also hardened the library: a zero or negative cap short-circuits to an
empty result instead of falling through to `slice(-0)`, so no future
caller can hit the same trap.
* fix(managed-agent): stop gate lookup scanning the full tool history
The id filter keeps the collected array tiny, so `maxItems` never trips and
the walk continued to the end of a session's tool history even after every
blocking id had been found. `listPaginated` now takes a `stopWhen` predicate
and the gate lookup ends as soon as it has all the ids it came for.
Also makes a blocked-but-unnamed session observable: when a session reports
`requires_action` with no blocking event ids, `requiresAction` stays true —
reporting false would tell a workflow the session is fine while it is parked
indefinitely — and the dead end is logged and documented instead.
* fix(managed-agent): floor event cap and make metadata clearing explicit
- A `maxItems` between 0 and 1 slipped past the zero guard and became
`slice(-0)` — the whole history — because slice truncates its index toward
zero. The cap is now floored at the library boundary, so no caller can hit
it whatever they pass.
- Update Session documented full metadata replacement but could not express
a clear: an empty map normalizes to "absent". Inferring the clear from
emptiness would be worse, since an untouched table is also empty and would
wipe metadata on every title-only update. Adds an explicit `clearMetadata`
instead, and corrects the parameter's documentation.
* test(managed-agent): pin the HTTP shape of every session endpoint
Method, URL, and beta header for all 11 calls, plus the SSE accept header,
the separate memory-store beta (combining the two is a documented 400), and
content-type only on requests that carry a body. These are the details types
cannot catch and that break silently when a path is "tidied".
|
||
|
Justin Blumencranz
|
a5ca810cfe |
fix(chat): drop unparsable special-tag payloads instead of dumping raw JSON (#6142)
* fix(chat): drop unparsable special-tag payloads instead of dumping raw JSON * fix(chat): suppress broken payloads mid-stream; reserve marker rescan for mispaired quotes * improvement(chat): require key-value colon evidence before dropping an unparsable tag body |
||
|
Siddharth Ganesan
|
88f054e185 | fix(auto-model): fix auto model (#6144) | ||
|
Vikhyath Mondreti
|
b77b9ee491 | improvement(workspace): remove tip + simplify modal header text (#6141) | ||
|
Theodore LiandClaude Opus 5
|
c5cc6ce26c |
feat(chat): hide the Chat module when NEXT_PUBLIC_CHAT_DISABLED is set (#6137)
* feat(chat): hide the Chat module when CHAT_ENABLED is unset A self-hosted deployment that skipped the chat key still rendered the full mothership Chat UI, landing on the composer and 401ing on every message. Gate it behind a CHAT_ENABLED / NEXT_PUBLIC_CHAT_ENABLED twin, written by the setup wizard alongside COPILOT_API_KEY and validated by the existing FLAG_TWINS doctor check. The flag resolves at module scope on both render passes, so no chat surface renders then disappears. With Chat off the workspace lands on its first workflow (resolved server-side, behind the cached host-context check so no workflow id leaks to non-members), and the chats list, scheduled tasks, editor Chat panel, and chat CTAs are absent. Routes are gated rather than deleted: /home redirects because it is baked into delivered invitation emails and the accept contract. Also fixes two bugs the gate exposed: a persisted activeTab of 'copilot' left the workflow panel blank from first paint, and the panel's handoff listener claimed MOTHERSHIP_SEND_MESSAGE events outside its own gate, silently swallowing "Fix in Chat" messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * refactor(chat): gate the UI on NEXT_PUBLIC_CHAT_DISABLED, not an opt-in flag CHAT_ENABLED made Chat opt-in, so every existing deployment that already had COPILOT_API_KEY would have lost the module until it set a new variable. Invert to an opt-out so nothing changes for them. That also collapses the twin. The only reason the flag needed a server/client pair was that it projected a secret; NEXT_PUBLIC_CHAT_DISABLED is not one, so getEnv resolves the same value from process.env on the server and window.__ENV in the browser. Gone with it: the FLAG_TWINS entry and its doctor sync check, the two-variable wizard write, and the boot-time throw, whose contradiction (flag on, key absent) can no longer be expressed. Presentation and capability are now separate concerns. NEXT_PUBLIC_CHAT_DISABLED decides whether the surfaces render; COPILOT_API_KEY decides whether the work can run, and gates the paths that need it — the Sim Chat block, prompt-job claims, and inbox access — each failing on its own terms. The wizard writes the opt-out when you skip the chat key, which is the case this started from: a fresh self-host that never configured Chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * feat(setup): prompt for the chat key in k8s mode The dev and compose flows minted a chat key and wrote the Chat opt-out alongside it; k8s did neither, so a cluster install with no COPILOT_API_KEY in its Helm values rendered a Chat module that rejects every message. Prompt with the same flow and feed both values into `app.env`, which the chart already renders as arbitrary container env. Reading the previous release's key matters here in a way it does not for the file-based modes: `helm upgrade` without `--reuse-values` keeps only what this document carries, so a key the user elects to keep has to be re-supplied or it is silently dropped. Splits the release-values read from the secret-reuse check so both the key and the secrets come from one `helm get values` call, and carries the mothership override across for the same mint-here-validate-there reason the other modes document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(setup): write app-behavior flags to every env file the app can start from The wizard wrote the Chat opt-out only to the env file its own mode owns, so choosing compose put it in the root `.env` while `bun run dev` reads `apps/sim/.env` and never saw it. Skipping the chat key appeared to do nothing. Mirror values that change how the app behaves — as opposed to where it connects — across both targets. Connection settings deliberately do not go through this: DATABASE_URL and friends differ between the compose stack and a local dev run, which is why this takes an explicit set of values rather than the whole batch. The mirrored file is written even when absent, since missing is exactly the case that stranded the flag, but with seeding suppressed so a compose run leaves a one-line apps/sim/.env instead of a full .env.example for a stack the user is not running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(compose): forward NEXT_PUBLIC_CHAT_DISABLED to the app container The wizard wrote the flag into the root .env, but compose only passes through variables the service's `environment` block names — and that block listed COPILOT_API_KEY without its companion. Skipping the chat key on a Docker install therefore did nothing: the value sat in .env and never reached the container. Add the passthrough to all four compose files. Reverts the previous commit's mirroring into apps/sim/.env, which treated the symptom — each mode writes only the env file it owns, and that file is now wired correctly. k8s needs no equivalent: its values flow into `app.env`, which the chart renders key by key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(chat): resolve the landing route without blocking on the database Server-resolving the first workflow meant a session lookup, an access check and a query had to finish before anything rendered. A slow or unreachable database left the user on a blank page under a populated sidebar — worse than the instant redirect it replaced, and with no signal that anything was wrong. Redirect straight to `/w` instead and let it pick from the workflow list the layout already prefetches, so the choice costs no round trip and cannot hang. Repoints the sidebar's primary action rather than hiding it: the slot that offered "New chat" now offers "New workflow" and creates one, since with Chat off there is no composer to open but the intent is the same. Sends the CLI key handoff to signup rather than login. It is reached from a terminal — usually the setup wizard standing up a fresh self-host — where the visitor has no account yet. Both auth pages cross-link carrying the callback, so a returning user is one click from login with their destination intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * improvement(chat): address cleanup-pass findings on the Chat gate Effects: the panel's auto-select effect read the copilot chat list while the list query was deliberately skipped, took "empty" for "deleted in another tab", and cleared the user's selection — latching a ref that stopped it ever being restored. Guarded on the same condition as the handoff listener. Memo: `/w` filtered workflows through a useMemo whose array dependency was a fresh `[]` on every render while the query had no data — the exact window the page exists for — so it memoized nothing and re-fired the redirect effect. Keyed on the workflow id instead. Same unstable-default problem on the sidebar's chat list, where it invalidated five downstream memos; given a stable empty constant. Callback: `handleCreateWorkflow` listed the whole mutation object in its deps, which TanStack recreates every render. Harmless until this branch wired it into the top nav, where it defeated `memo(SidebarNavItem)`. React Query: Recently Deleted still fetched archived chats unconditionally and offered restores into routes that now 404. Also surfaces an error state on `/w` — it is the landing route now, so a failed list fetch would otherwise spin forever behind a log line — fixes a spinner using a token undefined in dark mode, and trims comments that restated code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(chat): gate workflow creation on write access, pin the key in schedule tests The zero-workflow landing offered "Create workflow" to every member. Creation navigates optimistically, so a read-only member was sent to a workflow the server had already refused to create, with the failure never surfaced. Gate both entry points — the empty state and the sidebar's "New workflow" row — on the same `canEdit` check the rest of the sidebar uses, and tell read-only members who can make one instead of offering an action that cannot succeed. The schedule-execution tests only passed locally because vitest loads the developer's own `.env`, which supplied COPILOT_API_KEY; CI has none, so the prompt-job claim guard skipped the claims those cases assert on. Pin the key through the env mock so the suite states its own preconditions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(setup): name both variables in the chat-key failure hint The caller writes the Chat opt-out whenever the prompt returns no key, so the hint's "or set COPILOT_API_KEY yourself" restored capability while leaving the module hidden — the one path where following setup's own advice does not work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Waleed
|
0bcf64a505 |
perf(bundle): stop three routes shipping the block and tool registries (#6138)
The public /integrations page downloaded, parsed and executed 19.54 MB of JavaScript, 15.52 MB of which was the tool registry — 4,351 tool configs, to render a catalog of names and icons. It arrived as `<script async>` in the initial HTML, so the browser fetched and ran it on load. Measured on a production build, chunks referenced by the page's own HTML: /integrations 19.54 MB -> 3.55 MB across 41 chunks (-82%) No chunk over 1 MB remains on that route. credit-usage also loses its copy. Three separate import edges, all the same shape: a module mixing pure helpers with registry-backed ones, so importing the pure half dragged in all 282 block configs and the tool registry behind them. - `lib/integrations/index.ts` built POPULAR_WORKFLOWS at module scope via `getAllBlockMeta()` and re-exported registry functions. The landing grid is a client component, so importing anything from that barrel inherited the whole graph. POPULAR_WORKFLOWS moves to its own module — its one real consumer is a server page — and the re-export goes, which .claude/rules/sim-imports.md already forbids and which nothing imported through the barrel. - `blocks/icon-color.ts` mixed pure contrast maths (`getTileIconColorClass`, `isLightTileColor`, used by the landing page) with `getBareIconStyle`, which reads `getAllBlocks()`. Split by dependency into `blocks/brand-icon-style.ts`; all six of its consumers are under app/workspace/** where the registry is already legitimately present. - `credit-usage-view.tsx` imported a date formatter from the logs feature's `utils.ts`, which also exports registry-backed badge components. `formatDateShort` has three consumers across three features, so it moves to `lib/core/utils/date-display.ts` per the repo's utils-location rule. Not addressed: account/settings/[section] still reaches the registry through workspace-permissions-provider -> socket-provider, one of the four known client edges from the module-graph audit rather than a stray import. It is also loaded via `dynamic()`, so it is likely lazy rather than on first paint. That belongs with the registry split, not here. workspace/* keeps its copy, which is correct — the editor needs tool params and outputs. |
||
|
Bill LeoutsakosandBill Leoutsakos
|
c0e7ea9421 |
fix(pi): surface provider errors from agent events (#6133)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Theodore Li
|
648dd345fa |
feat(notifications): email on schedule auto-disable and 100% usage limit (#6038)
* feat(notifications): email on schedule auto-disable and 100% usage limit * fix(notifications): keep partial recipients when one lookup fails |
||
|
Vikhyath Mondreti
|
422b4919f1 | chore(trigger): upgrade version (#6127) | ||
|
Theodore Li
|
b4f027eece |
fix(billing): point self-hosted upgrade CTAs at the hosted app (#6003)
* fix(billing): point self-hosted upgrade CTAs at the hosted app Self-hosted Chat bills against the sim.ai account behind COPILOT_API_KEY, but its 402 upgrade card linked to local billing settings that a self-hosted deployment does not have. Point those CTAs at the hosted app instead, and drop the local workspace-role gate that could hide the CTA from the only person able to act on it. Adds /upgrade, an account-scoped entry for callers that cannot know a workspace id. It delegates to /workspace?redirect=upgrade rather than re-deriving workspace resolution, inheriting local recency, stale-session recovery, and the no-workspace creation policy. * fix(billing): keep the upgrade intent through workspace creation A first-time visitor to /upgrade has no workspace to resolve, so /workspace creates one — and then hardcoded a redirect to home, silently dropping the upgrade intent. Route both exits through one destination helper so the created workspace lands on the plan picker with its reason intact. |
||
|
Waleed
|
e98715da01 |
fix(realtime): noindex the socket server's 404 responses (#6129)
* fix(realtime): noindex the socket server's 404 responses The sockets.* hostnames are served by this server and return a plain JSON 404, which Google Search Console reports as crawl errors. Mark unmatched routes noindex so crawlers drop the hostnames instead of retrying them. * fix(realtime): noindex every response, not just the 404 Setting the header only on the 404 fallback covered the one response that crawlers already drop on status code alone, while /health — the sole route returning 200 with a body, and so the only indexable surface on the socket hostnames — stayed uncovered, with a test pinning it that way. Set it once on the handler instead. Node merges setHeader values into writeHead and no branch sets X-Robots-Tag, so it reaches every response. |