mirror of
https://github.com/simstudioai/sim.git
synced 2026-10-31 03:47:12 +08:00
95d08d2a49e1ebc8429cee896570eff0052538bc
6456
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Waleed
|
95d08d2a49 |
fix(ci): stop the migration safety audit from passing on a branch it never read (#7022)
* fix(ci): stop the migration safety audit from passing on a branch it never read
The zero-downtime audit reports the same empty file list for 'this branch adds
no migrations' and 'I could not diff against the base', and the second prints
as `✓ No new migrations to check` with exit 0. Reproduced on this checkout:
$ bun run scripts/check-migrations-safety.ts origin/does-not-exist-branch
✓ No new migrations to check. exit=0
`changedMigrationFiles` returned `[]` whenever `git diff` failed, with a comment
deferring the decision to the caller — but the caller only recognised a missing
git binary (`git rev-parse HEAD === null`), never an unusable ref.
CI supplied exactly that input. `git fetch --depth=1 … 2>/dev/null || true` hid a
failed fetch, leaving `origin/<base>` absent, so a PR adding a destructive
`DROP COLUMN` would clear the only guard on production DDL with a green check.
Two halves:
- The audit now distinguishes the cases. Absent git is still the one legitimate
skip and is checked before the diff; a diff that fails with git present raises
`BaseRefUnusableError` and exits 1.
- The fetch is its own step with no `|| true`, so a failure fails the job. Depth
stays 1: without a merge-base the audit diffs the two tips, which under
`--diff-filter=AM` is exactly the migrations new on the branch.
Covered by a test that runs the script end to end, since the defect was in the
exit code rather than in any function's return value. Verified it fails when the
throw is reverted to `return []`.
* fix(ci): fetch the base ref once, and stop swallowing the failure
The same `git fetch --depth=1 … 2>/dev/null || true` appeared in both base-ref
audits. Fixing only the migration one would have left the identical defect a few
steps above it.
Neither audit can tell an absent base ref apart from a branch that changed
nothing. The block-registry check at least degrades to a visible
`⚠ Could not diff against base ref — skipping`; the migration audit printed
`✓ No new migrations to check` and exited 0.
Both now share one fetch step that fails the job when it fails.
|
||
|
Waleed
|
cc087498ae |
refactor(utils): add slugify and adopt it at the eight sites that hand-rolled it (#7018)
The same three-step derivation — lowercase, collapse each non-alphanumeric run to a hyphen, strip the leading and trailing one — sat in eight files. Two of them carried a TSDoc line whose only job was to warn that they mirrored a third (`instance-org.ts`: "Derives a slug the same way the admin organization API does"; `consolidate-users-into-organization.ts`: "Mirrors the slug derivation used by POST /api/v1/admin/organizations"). A comment asserting two implementations agree is the shape duplication takes when it cannot be checked. All eight were semantically identical. Two anchored the strip with `-+` rather than `-`, and one followed it with a `--+` collapse, but `[^a-z0-9]+` has already collapsed every run by that point, so neither could ever match more than the single-hyphen form. Nothing changes. Truncation stays at the call sites. Four of them bound the result — at 24, 64 and 80 — and only `copy-chats.ts` strips again afterwards, because slicing can land mid-run and leave a trailing hyphen the earlier strip never saw. Folding a `maxLength` into the helper would have had to pick one of those behaviors and silently impose it on the others. `artifact-stylesheet.ts` keeps its copy: it lives inside the `SIM_ARTIFACT_SHELL` template literal and runs in the viewer's browser, where there is no import to resolve. |
||
|
Waleed
|
49593b3191 |
refactor: replace hand-rolled utilities and dead code with the shared forms (#7021)
* refactor: replace hand-rolled utilities and dead code with the shared forms Each of these has a mandated helper or an established accessor in the repo that the site predates or missed. All are behavior-preserving: - `omit()` for the three `Object.fromEntries(Object.entries(x).filter(...))` block-input filters, which also recovers the `Omit<T, K>` typing that `Object.fromEntries` erases to an index signature. - `getErrorMessage()` for the inline `instanceof Error` message ternary. - `getBlock()` for two `getAllBlocks().find((b) => b.type === x)` scans, one of them inside a loop over selected tools. The same file already resolves the same values through `getBlock`. - A memoised `Map` for three `.find()`-by-id scans over the workspace skill list, one of them inside a render `.map()`. - `SELECTOR_SEARCH_STALE` for three copy-pasted `15 * 1000` literals. They are deliberately shorter than `SELECTOR_STALE`, so this is a new named constant rather than a fold into the existing one. - Tailwind classes for the static half of two duplicated anchor styles, keeping only the genuinely dynamic `left`/`top` inline. - Dropped the unused `catch` bindings on three intentional JSON-parse swallows. `panel.tsx`'s run-button gate loses a `TODO`-stubbed `hasValidationErrors = false` and the `isWorkflowBlocked` term built on it. That term was dead twice over: it reduced to `isExecuting`, and the enclosing expression is already guarded by `!isExecuting`. * fix: guard the registry lookups, and scope the search-stale doc to its callers `getBlock` normalizes its argument with `type.replace(...)`, so it throws on `undefined` where the `getAllBlocks().find(...)` it replaced returned `undefined` harmlessly. Both call sites can be reached without a type: `tool-input` reads `state.blocks[blockId]?.type`, which is undefined once the block is deleted while the panel is mounted — and `Record` indexing hides that from the compiler, so it would have thrown during render. `agent-handler`'s `tool.type` is optional and the compiler did catch it. Also index the skill lookup in `resolveSkillsLabel`, which runs a `.find()` inside a `.map()` for every block on the canvas — the case the memoised map in `skill-input` addressed for one component while leaving the hot path. `providers/utils.ts` keeps its `getAllBlocks().find(...)`: it takes the registry as an injected dependency precisely so a client-reachable module never imports it, and reaching for `getBlock` there would cross that boundary. The new constant's doc claimed search-backed selectors take a shorter window. Several still sit on `SELECTOR_STALE`, so it now describes the value its three callers share rather than asserting a rule the tree does not follow. * fix: guard the second registry lookup in tool-input `selectedTools` validates only `value[0]?.type` and then casts the whole array, so a persisted workflow whose later rows lost their `type` yields `undefined` here — the cast is what makes the compiler believe otherwise. `getBlock` normalizes with `type.replace`, so that throws during render. |
||
|
Waleed
|
fc7aa66a39 |
fix(api): withhold internal failure messages from internal route responses (#7015)
An orchestration result carrying `errorCode: 'internal'` holds whatever text the fault happened to have — `workflow-lifecycle.ts` catch-alls return `toError(error).message`, which is the driver's failed SQL. Three application helpers projected that straight into an `OrchestrationError`, and the internal route policy rendered its message into a 500 body, so raw SQL reached clients. The v2 envelope already scrubbed the same failures; internal routes did not. `messageForOrchestrationError` already encoded the rule and two sites honored it. The three that hand-rolled it disagreed, and `workflow-vfs` disagreed with itself: it defaulted the code with `?? 'internal'` but compared the raw `errorCode` against `'internal'`, so an uncoded failure was classified internal and still rendered its own message. Pair the two in `throwOrchestrationFailure` so a code and its message cannot disagree, and scrub at the internal route boundary as well, matching v2 — no call site authors a curated `internal` message, so nothing legitimate is masked, and site N+1 cannot reopen this by forgetting the rule. |
||
|
Waleed
|
a20a5465ef |
perf(db): optimize recurring query paths (#7014)
* perf(db): optimize recurring query paths * perf(logs): batch keyset export reads * fix(workspaces): type nullable member count targets * fix(db): harden query performance changes * fix(logs): guard export stream cancellation |
||
|
Vikhyath Mondreti
|
9cecf0b837 |
improvement(provenance): aggregate and attribute unrecorded durable reads (#7017)
Fail-open on unrecorded durable provenance rests on one compensating control: the audit entry telling the people who own the secrets that a read proceeded unvouched. An audit of all four surfaces found the control incomplete in exactly the places this closes, and confirmed the policy itself sound — so nothing here changes what any read or write does, only what gets recorded about it. Knowledge was the one surface with no audit trail at all: the per-record import reports without a workspace, and the report skips the audit row when it cannot name one, so fail-open knowledge reads emitted one error log line per record and zero audit entries. Both importers now count unrecorded records while the surface is open and report once per read with the workspace, actor, and count — the shape memory and tables already use. The search read reports once across chunks and rendered metadata, and only when the registry did not latch, since a latched read never reaches a model. Fault returns stay silent; those reads fail closed. Memory had the one silent local degrade: a record whose canonical hash outgrows its bounds, or whose entries fail normalization, was stored unknown with nothing logged anywhere — the table writer logs its equivalent. The binding now logs the cause at error where it is decided. An incoming unknown stays silent; its producer already reported. The memory list contract gains the page ceiling every other list already has (max 1000, matching the table convention); no caller in the repo passes a limit at all, and the route is internal-auth only. Workspace-file audit rows now carry the acting user where the caller already holds one — copilot vfs, the agent and mothership handlers, and the provider attachment filter. Everywhere else, including principals with no user to name, the actor stays null, which the report type has always permitted. Two comments catch up with the code: the file sidecar stores three statuses since the absence/taint split, and the mounted-file scanner's scan-overflow-to-taint is deliberate where the registry scan over-approximates — that scan only narrows an already-sound candidate set, while this one decides whether egress redaction would suffice for bytes the same matcher just failed on. |
||
|
Waleed
|
1cb9c868ce |
improvement(chat): speed up conversation navigation (#7011)
* improvement(chat): speed up conversation navigation * fix(chat): prefetch direct navigation intent * fix(chat): preserve quick-click prefetch intent |
||
|
Waleed
|
b44d285607 |
fix(files): download a markdown file as a zip only when it really has assets (#7009)
* fix(files): download a markdown file as a zip only when it really has assets A document that merely mentions an embed URL in prose or an inline code span counted as having attachments, so any document about the files API downloaded as a zip whose assets/ folder was empty. - Detect embeds with the markdown lexer instead of scanning raw text, so only real image embeds count: prose, code spans, fenced samples, and links no longer do - Choose the export format after resolving assets rather than from the candidate count, so a missing, unreadable, or oversized embed falls back to the plain document instead of an empty zip - Move the document scan out of the copilot tool tree into lib/uploads/server, where both file routes already live, and drop two pass-through wrappers - Share one <img> src reader between the clipboard handlers and the scan - Walk tokens explicitly: marked's walkTokens concatenates per token and costs O(n^2), measuring 5.4s on a 254KB document against 14ms here, on a path anonymous public-share traffic reaches * fix(files): keep an embed id spelled as the document spells it Decoding the id let a percent-encoded embed resolve and bundle its asset while the rewrite, which searches the document for that id, found nothing — the zip kept an API URL that renders as a broken image offline. Keys stay decoded; they are matched against stored keys, not against document text. * fix(files): resolve an export asset by its stored id, rewrite by its spelling An embed carries two representations and they are not interchangeable: metadata resolves by the stored id, while the rewrite finds the embed by searching the document for the spelling it used. Using one for both either drops a percent-encoded asset or bundles it behind a link still pointing at the API. * fix(files): resolve an embed by its stored id wherever one is read from a document The export bundler decoded an embed's spelling before looking it up, but the file-agent's embeddability warning did not, so a percent-encoded embed the export resolves and bundles could still be reported as one that will not survive an export. Both now share one helper. Request-supplied ids are untouched: their route contracts already constrain them to the plain id charset, so there is no spelling to decode. |
||
|
Vikhyath Mondreti
|
ed335d4137 | improvement(pricing): list Sandboxes as a Max and Enterprise feature (#7013) | ||
|
Waleed
|
f3867694b8 |
fix(files): allowlist the schemes a markdown link may target (#7012)
`normalizeLinkHref` rejected only `file` for a `scheme://` target, so any other scheme was returned unchanged. `scheme://` is well-formed for every scheme, so the check let through spellings that are not navigable targets at all. - Keep a scheme only when it is http(s), ftp(s), mailto, or tel; drop the rest - Leave an existing link alone when a committed target normalizes away, rather than unsetting it — the editor seeds that field with the current href, so committing an untouched one previously removed the link Detection is unchanged for relative, anchor, protocol-relative, and bare-domain targets. A document's stored markdown is untouched: normalization runs on the render and edit paths, never on parse or serialize, so a target that is refused still round-trips verbatim. |
||
|
Waleed
|
465bdbd12b | fix(settings): keep billing header stable (#7010) | ||
|
Theodore Li
|
d831c0937c |
fix(security): isolate rejected OTP attempts (#7008)
* fix(security): isolate rejected OTP attempts * fix(security): make OTP requests non-enumerating * fix(security): defer OTP delivery work |
||
|
Vikhyath MondretiandClaude Opus 5
|
33feaa158a |
fix(inbox): stop an unattributed sender inheriting owner write authority (#7006)
* fix(inbox): stop an unattributed sender inheriting owner write authority resolveInboxExecutionActor refuses to name a raw-secret actor when the sender matches no workspace member, then hands the run ws.ownerId for everything else. That identity also supplies userPermission, which is what executeTool gates on, so the owner's admin satisfied every requiredPermission check. In headless mode the client-routed workflow tools fall back to their registered server handlers (see the comment in tool-executor/executor.ts), so create_workflow, edit_workflow and run_workflow — all requiredPermission 'write' — were reachable. runWorkflowFromCopilot then executes with enforceCredentialAccess and the owner as actor, which resolves the owner's workspace and personal secrets. An allowlisted external correspondent could therefore reach, through a workflow it had the agent build and run, exactly what the null secret actor refuses for a direct mount. Cap the run's tool permission at read when no member owns the message. An attributed message is unchanged and still uses the sender's own permission, so a read-only member emailing the inbox still cannot run or edit anything. Read rather than none because answering an external correspondent from workspace context is the point of the inbox; only mutation and execution are withheld. The owner identity itself stays: billing attribution and workspace reads need a real user. This separates that need from the authority that came with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(copilot): bar the headless client-tool fallback below write Client-routed tools carry no catalog requiredPermission because the browser runs them through the workflow APIs, which authorize the caller's own session. The headless fallback in executeTool has no session and runs under the request's principal instead, with nothing standing in for that check. So the read cap from the previous commit did not reach run_workflow, run_workflow_until_block, run_block or run_from_block: all four are route 'client' with no requiredPermission, unlike create_workflow and edit_workflow. An unattributed inbox sender could therefore still run an existing workflow, which executes with enforceCredentialAccess under the workspace owner and resolves the owner's workspace and personal secrets. Derive the requirement at the gate instead: a client-routed tool taking the headless fallback requires write. Interactive callers never reach this branch, so the browser path is unaffected. The catalog itself is generated from the copilot contracts repo and cannot carry this rule, which only applies to the fallback. Also corrects the inboxToolPermission doc, which claimed run_workflow gates on requiredPermission 'write'. It does not; it is gated here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Waleed
|
66a9a18d1f |
improvement(perf): eight verified cuts to workspace cold-load JavaScript (#6996)
* improvement(perf): eight verified cuts to workspace cold-load JavaScript Second round of load-time work, adversarially verified for strict behaviour preservation before implementation. Each item is an import-graph fix — none changes what renders, when it renders, or any data path: - knowledge/[id] imported one modal through the [documentId] components barrel, which also exports the chunk editor and therefore js-tiktoken (~2.5 MB gzip of BPE tables) on a route that never edits chunks. Deep import. - prepareBlockState moved out of stores/workflows/utils.ts into its own module. It is the only function there needing the block registry and the generated tool-outputs artifact (~476 KB gzip), and utils.ts is reached by the persistent shell — so every workspace route paid for a canvas-only helper, including a module-scope JSON.parse of a 5.4 MB string. - ExecutionSnapshot (the frozen-canvas modal) is now React.lazy behind its interaction gates, per the code-splitting procedure in sim-imports.md: deep import, dead barrel re-export deleted, sibling imports in log-details deepened to break the parent->child barrel cycle, local Suspense at both render sites. Takes ~7.6 MB of source off logs hydration. - The api contracts barrel no longer re-exports ./tools, ./selectors, ./v1, or ./demo-requests (~58 KB gzip of Zod schema construction on every route). Zero importers used the barrel path for any of them. - createCsvParser (streaming csv-parse, a Node Transform) moved to a server-only module so its stream polyfill leaves client bundles. Deliberately not re-exported from the lib/table barrel. - jszip is dynamically imported at both remaining static call sites (skill zip extraction, pptx parsing) — both already-async, user-triggered paths, mirroring the existing pattern in workflow import-export. - The desktop local-filesystem tool executor is dynamically imported in use-chat; a chunk-load failure now reports an error completion so the server-side tool call settles instead of hanging. Production build, JS downloaded before the load event, vs the previous release: /home 4.44 -> 3.87 MB /logs 4.44 -> 3.64 MB /knowledge 4.22 -> 3.68 MB /tables 4.17 -> 3.61 MB /files 4.68 -> 4.10 MB /w/[id] 4.80 -> 4.67 MB /home total after idle prefetch: 8.15 -> 5.52 MB The lazy snapshot was exercised end-to-end: its chunk loads when a log detail opens (off the route's cold path, warm before the View Snapshot click) and the modal renders without errors. Boundary baseline retightened. * improvement(logs): contain snapshot chunk-load failures and settle the local-fs tool on recovery failure Review round: wrap both lazy ExecutionSnapshot render sites in a small error boundary (Suspense handles the lazy import's pending state, not its rejection — a failed chunk load would have unwound to the route boundary and replaced the logs page over an optional modal; mirrors PreviewErrorBoundary), and contain rejections inside the local-filesystem executor's load-failure recovery so a failed completion report degrades to a log instead of an unhandled rejection. * fix(logs): recover cleanly from snapshot chunk failures * fix(logs): preserve snapshot modal while loading |
||
|
Waleed
|
5a1602e961 |
fix(copilot): enforce the chat's delegated workspace scope across model-steerable server tools (#6995)
* fix(copilot): enforce delegated workspace scope in query_logs and set_environment_variables
A model-supplied workspaceId (or a workflowId in another workspace) could
steer both tools to any workspace the acting principal can reach, bypassing
the asserted-vs-context workspace comparison the rest of the Copilot tool
surface enforces. Both now resolve through requireCopilotWorkspace — moved
to a shared module — so an asserted workspace may only re-state the chat's
execution workspace, and the default-workspace fallback is removed so a
missing scope fails closed.
* fix(copilot): apply the workspace-scope guard across every model-steerable copilot surface
Extends requireCopilotWorkspace to the remaining copilot tools that resolved
their target workspace from model-supplied arguments: get_credentials (a
workflowId could steer the credential listing to any workspace the user can
access) and publish_custom_block (a workflowId could deploy/undeploy custom
blocks from another workspace's workflow). The handlers already protected
downstream by the application adapter (create workflow, generate API key,
list/create workspace MCP servers) now use the same guard so a mismatch is
rejected uniformly at the surface, and the getDefaultWorkspaceId fallback is
deleted entirely — no copilot path picks a workspace for the model anymore.
* refactor(copilot): classify both workspace-scope guard branches and drop call-site boilerplate
requireCopilotWorkspace now accepts an undefined context and throws a
classified OrchestrationError for the missing-workspace branch too, so every
caller drops the 'context ?? {}' and '|| undefined' coercions and one
instanceof covers the guard. query_logs inlines its now-one-line wrapper,
get_credentials drops the workspace-less special case (a workflow with no
workspace asserts nothing), and publish_custom_block handles the guard
locally instead of widening its catch-all — keeping its deliberate
assume-not-published guidance for unrelated failures.
* chore(copilot): drop call-site comments that restate the workspace-scope guard's TSDoc
* test(copilot): type the new query-logs scope tests instead of casting to any
|
||
|
Vikhyath MondretiandClaude Opus 5
|
43aba98527 |
fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL (#7005)
* fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL The desktop connect launcher passed better-auth a same-origin path as its callbackURL. Better Auth stores that value verbatim in the OAuth state, and the callback's credential-draft reader parsed it with a bare `new URL()`, which rejects a path. That throw happened inside the `account.create.before` database hook, which better-auth's OAuth callback does not guard, so the provider redirect landed on a 500 after authorization had already succeeded. Send an absolute URL from the connect page, matching the workspace-scoped branch and every other connect surface, and accept a path-absolute callback URL in the draft reader so the shape can never fail the callback again. Protocol-relative and malformed values still throw, keeping an unreadable binding loud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(desktop): compose the connect completion URL through the URL API Concatenating `getBaseUrl()` with the completion path leaves the result dependent on how the deployment spelled `NEXT_PUBLIC_APP_URL`: the helper only adds a missing protocol, so a trailing slash produced `//desktop/connect/complete`, a pathname that matches no route. The completion page is what bounces the OAuth result to the desktop app's loopback, so that typo would have stranded the flow just past the callback it was meant to fix. Both callback URLs in the page — the launcher's and the workspace-scoped authorize redirect's — now go through one helper that resolves the path against the base with `new URL`, matching how the same function already builds the authorize URL, with coverage for a trailing-slash base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(urls): give base URLs the no-trailing-slash form their call sites assume `getBaseUrl()` returned `NEXT_PUBLIC_APP_URL` as the operator spelled it, while almost every consumer builds `${base}/path`. A base configured with a trailing slash therefore produced a `//path` pathname that matches no route, and broke the `startsWith(`${base}/`)` prefix checks that decide whether a redirect target is our own — the OAuth authorize route rejected its own completion callback and fell back to the workspace page, so the desktop handoff never ran on those deployments. The previous commit fixed one such URL; this fixes the reason it was wrong, for the ~30 concatenation sites that share the assumption. `normalizeBaseUrl` now strips trailing slashes alongside the protocol it already added, which is the invariant SITE_URL has always documented. A path-prefixed base keeps its path. `getInternalApiBaseUrl` gets the same treatment, since its callers concatenate identically. `@sim/testing`'s urls mock is a hand-written mirror of this module, so it moves in step. `internal-api-base-url.test.ts` now unmocks the module it names — otherwise it asserts against that mirror and any drift between the two passes unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(urls): stop claiming path-prefixed base URLs are supported The previous commit's doc and test said a path-prefixed base keeps its path. That reads as support for a deployment shape the app does not have: there is no Next `basePath`, so routes are served at the origin root and such a value could not address them however the base were normalized. Every documented example is origin-only. Says only what is true — trailing slashes are the one spelling absorbed — and reframes the test as pinning the trim's shape rather than asserting a path-prefixed deployment works. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Theodore Li
|
bbf408bf30 |
fix(security): harden public auth rate limits (#6997)
* fix(security): harden public auth rate limits * fix(security): fail closed without client IP * fix(security): backstop public OTP requests * fix(security): preserve independent rate-limit backstops |
||
|
Vikhyath MondretiandClaude Opus 5
|
81ff24a2fc |
improvement(secrets): gate Copilot code mounting at use level (#7004)
* improvement(secrets): gate Copilot code mounting at use level Mounting a saved secret into Copilot code required credential-admin on that key, while a workflow Function block resolves the same secret for the same person at use level through getPersonalAndWorkspaceEnv. Copilot reaches that path itself — edit_workflow plus run_workflow — so the admin bar contained nothing. It redirected a Credential Member through a detour that mutates a persisted workflow, while the direct path is ephemeral and files a usage row. The inconsistency was also internal to Copilot: the secret names advertised to the model come from getAccessibleEnvCredentials and getPersonalAndWorkspaceEnv, both role-agnostic, so Copilot listed every secret the caller could use and then refused to mount all but the admin ones. Widen the workspace and shared-personal predicates to any active grant, and drop the matching role filter from the query. Workspace write is still required, revoked and pending grants are still refused, and a caller with no grant still gets nothing. The view gate stays where Copilot cannot route around it: values remain masked under Settings, and See usage remains admin-only, so a member's use is recorded for whoever can rotate the key. Model-egress projection is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(secrets): stop implying Personal secrets are shareable The Copilot code-execution paragraph listed "any secret shared with you as a Credential Member or Credential Admin" among what mounts, which reads as though a Personal secret can be shared. It cannot through any product surface: CredentialMembersSection renders only for workspace secrets and OAuth credentials, and the personal-credential sync only ever grants the owner. Narrow the sentence to Workspace grants. The comparison table's "Only you can use" row for Personal was correct and is left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Vikhyath MondretiandSim Pi Agent
|
9ff1d77306 |
feat(library): Best No-Code and Low-Code AI Agent Builders in 2026 (#7003)
Co-authored-by: Sim Pi Agent <pi@sim.ai> |
||
|
Theodore Li
|
8104c33bba |
Fix knowledge connector sync follow-up (#6927)
* Fix knowledge connector sync follow-up * Fix connector sync pause race * fix(knowledge): surface connector sync dispatch failures * fix(knowledge): make connector sync recovery durable * fix(knowledge): deduplicate connector sync dispatches * fix(knowledge): preserve pending connector syncs * fix(knowledge): lock connector sync snapshot |
||
|
Vikhyath MondretiandClaude Opus 5
|
763f3aa1b5 |
fix(deployments): stop superseded activations from dead-lettering (#6522)
* fix(deployments): stop superseded activations from dead-lettering 29 workflow.deployment.prepare.v2 events dead-lettered with "Webhook registration operation is stale", every one at attempts = max_attempts. A full retry budget means the failure is deterministic, which rules out the preparation path: an attempt superseded while preparing is marked superseded, so its next attempt short-circuits at the top of the handler and completes. The branch a retry re-enters is the other one. isTerminalNonActiveOperation covers failed and superseded but not active, so an attempt that activated and was then superseded by the next deploy keeps its own active status, re-enters post-activation work on every retry, and re-fails the same generation fence until the event dies. The fence it fails is correct — it takes the same workflow row lock the generation bump takes, and compares generations exactly — so nothing about the detection is racy; only the reaction to it was wrong. Reaching it needs a handler timeout, which parks the row for the 10-minute reaper instead of the 2s/4s/8s backoff, opening a window wide enough for a redeploy to land. Gate the resume branch on the operation still owning the current generation, matching the sibling cleanup that already does this, and complete the event as a no-op when it does not. The newer generation adopts the leftover work anyway: it collects every retired registration below its own fence. Also reverse the post-activation order. The audit entry, analytics event, socket notification, and workspace event describe a cutover that is already durable, and each is separately checkpointed, but they ran behind retiring the previous generation's external subscriptions — one provider call per retired row, and by far the most failure-prone step there. A single flaky provider silently cost the deploy its audit trail and left clients on the old version until something else refreshed them. Both call sites now share one helper so the order cannot drift apart again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(deployments): pin staging's active-resume tests to the current generation Two tests that staging added alongside durable PostHog delivery drive the `operation.status === 'active'` resume branch this PR now gates, and neither overrides `mockIsDeploymentOperationCurrent` — the suite's `beforeEach` defaults it to `false`. Under the new gate they read as superseded, so the handler short-circuits: the delivery test sees a resolve where it asserts a rejection, and the unconsumed workflow row leaks into the next test. Both are still-current resumes by intent, so they say so explicitly. The checkpoints, not the generation gate, remain what keeps analytics from being captured twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analytics): stop PostHog delivery from failing a deploy `deliverOutboxServerEvent` awaited `client.flush()` before letting the deployment outbox checkpoint advance, so an unreachable PostHog failed the event, retried it, and eventually dead-lettered it — while holding the socket notification, the workspace event, and retired-subscription cleanup behind a third party. `flush()` also drains the whole shared client queue, so an unrelated event's network error surfaced here as a failed deploy. It bought no durability the process did not already have: the outbox handler runs in the long-lived app container, where the client flushes on its own 10s interval and again from the `SIGTERM`/`SIGINT` hook in `instrumentation-node.ts`. The helper had one caller and arrived inside an unrelated squashed PR (#5273) with no rationale, against 161 fire-and-forget `captureServerEvent` call sites. Deleted it and restored `captureServerEvent`, whose contract is already "never throws". `insertId` still collapses retried captures. That contract was untested, which is why this regressed unnoticed, so `server.test.ts` now pins it — spying on the real client, since the lazy `require` defeats `vi.mock` and a disabled client would pass every assertion vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(deployments): fence the cleanup, not the notifications The resume guard sat at the top of the `active` branch, so it skipped every post-activation step whenever `isDeploymentOperationCurrent` went false. That predicate goes false as soon as any newer generation row exists — including one still `preparing` or already `failed` — and in that window this activation is still the live cutover. Nothing newer would ever adopt its audit entry, analytics event, socket notification, or workspace event, so the guard permanently dropped them and completed the outbox event as if they were owed to someone else. Only the two cleanups are generation-fenced. `cleanupInactiveDeploymentsForOperation` already gated itself on that exact predicate and returned quietly; the retired webhook cleanup was the one that instead let the store's `assertCurrentOperation` throw. It now carries the same guard, on the same fence the store asserts — `deploymentVersionId` and `statuses: ['active']` included, so passing the gate actually implies passing the assert. The notifications run unconditionally, still idempotent through their checkpoints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Vikhyath Mondreti
|
d4195ff682 |
fix(media): stop the ffmpeg output format from choosing its own path (#7001)
`convert` and `extract_audio` built their output path as
`path.join(dir, `out.${format}`)`, and `format` arrives from the tool as an
unconstrained `{type: 'string'}` — no pattern, no enum, and the handler passes it
through untouched. A format of `../../../../../../../../tmp/x.mp4` resolves to
`/tmp/x.mp4`, so FFmpeg writes attacker-influenced media wherever the traversal
points. Verified against a real binary: the run produced a 44,078-byte MP4
outside the temp directory, which the `rm -rf` cleanup then never saw, because
the file was never inside the directory being removed.
Output extensions are now letters and digits only. The pattern admits no `.` and
no separator, so `out.${ext}` is always a single path segment and containment
follows from the validation itself rather than from a second check that could
drift away from it.
`trim` and `fade` build their paths from `extFromMime`, which returns
`mime.split('/')[1]` — that can never contain a separator, so those stay inside
the temp dir and keep accepting values like `x-msvideo` that this stricter
pattern would wrongly reject.
|
||
|
Justin Blumencranz
|
bb0792564a |
fix(chat): copy workspace resources as portable links (#6956)
* fix(chat): copy workspace resources as portable links * refactor(chat): simplify portable resource copying |
||
|
Justin Blumencranz
|
61c405afb6 | fix(chat): copy orchestrator responses as markdown (#6903) | ||
|
Vikhyath Mondreti
|
e406b8a277 |
fix(charts): confine untrusted chart options to canvas render paths (#6998)
A `.chart` document is untrusted input — any workspace member authors one, and a public share link renders it to anonymous visitors on the app origin — but `parseChartSpec` forwarded `option` to `setOption` unchanged. ECharts draws through canvas with two exceptions: a `tooltip` left in its default `renderMode: 'html'` assigns its content to `innerHTML`, and a string `formatter` is that content's template verbatim (only substituted values are escaped), while `toolbox` assigns `dataView.lang` to `innerHTML` and fills a `saveAsImage` popup with `document.write`. Force the render mode and drop the toolbox so the document has no DOM sink, rather than filtering the values that flow through one. The walk is deep: `baseOption`, `media[].option` and timeline `options[]` each carry their own tooltip, a tooltip declared only under `media` still instantiates the component in HTML mode, and a `media` entry can override a top-level `renderMode`. Also escape the series name in the pptx chart renderer's bubble tooltip — it comes from the uploaded document and is interpolated into a hand-built `innerHTML` string, where ECharts escapes only the markup it builds itself. |
||
|
Bill LeoutsakosandBill Leoutsakos
|
6f919ef192 |
fix(terminal): keep running-command tooltips concise (#6991)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Vikhyath Mondreti
|
cbf3aad569 |
fix(workflows): scope the canonical sub-block index to the active surface (#6990)
* fix(workflows): scope the canonical sub-block index to the active surface A block that is both an action and a trigger holds one `subBlocks` array — its own fields plus its trigger's, spread in after them. The two sets routinely share a `canonicalParamId` under different ids, so indexing them together collapses a trigger field into an action pair whose `basicId` it can never be. Every group-relative question about that field then answers for the dormant surface. The serializer was never affected: `shouldSerializeSubBlock` drops the inactive surface before the canonical collapse reads it, so it resolves against a value map the dormant surface cannot appear in. Every other caller resolves against the block's full value map, so the scoping has to live in the index. - add `getCanonicalSubBlocksForSurface` / `buildCanonicalIndexForSurface`, and move the three sites that already had the filter inline onto them - `getCardSubBlocks` derives its own index instead of accepting one; it already took `triggerMode`, and accepting an index is what let all three callers pass one built for the other surface - scope the remaining consumers that resolve against a full value map: the canvas card, autolayout, both preview surfaces, the dependsOn gate, the canonical value hook, reactive conditions, and the copilot selector lint - keep a canonical group with no advanced member out of the legacy `advancedMode` path, which deleted its basic member and republished nothing - merge legacy type-scoped tool modes as a baseline under index-scoped ones, so the first re-toggle stops reverting the ids the user has not touched * fix(workspace-forking): scope the canonical gates to the block's active surface `createCanonicalModeGates` indexed a block's whole `subBlocks` array, so on a mixed action/trigger block a trigger field sharing a `canonicalParamId` with an action pair was read as a member of THAT pair. Being neither its `basicId` nor in its `advancedIds`, `isDormantMember` answered true the moment the shared mode resolved to advanced — and a fork acts on that by clearing the value, so a configured trigger field was silently wiped on fork/sync. Reachable without any explicit toggle: a block configured as an action with a manual id and then switched to trigger mode leaves the pair's value heuristic resolving to advanced on its own. - `createCanonicalModeGates` takes the surface and scopes its index - thread `triggerMode` through `RemapForkContext`, `SubBlockTransform`, `clearDependentsOnRemap`, `collectClearedDependents`, the reference scanners, and the promote cleared-ref collectors - nested tool params and the dependent scan are unchanged: a tool is always the action surface, and the dependent scan already narrows its configs * fix(workspace-forking): keep the dormant surface classified as it was before scoping Surface scoping decides canonical membership for the ACTIVE surface. Applying it to every key also re-classified the dormant surface's own values: they stopped being dormant members, which meant the remap no longer cleared them AND started detecting them as references — turning a stale action selector on a trigger-mode block into a mapping requirement that can block promote/sync. The gates now pick the index per key: the scoped one for anything the active surface defines (the fix — a trigger field gets its own group instead of being read as a stranded member of an action pair), the whole array for everything else, which is byte-for-byte the pre-scoping behavior. Also adds `check:canonical-index`, an audit that fails any call building a canonical index off a config's whole `subBlocks`, or calling the fork gates without a surface, unless annotated with why. This defect shipped three times in three subsystems; the 14 sites that legitimately mean one fixed surface now say so at the call. * fix(audits): stop the canonical-index guard flagging its own source The audit holds `buildCanonicalIndex(` and `createCanonicalModeGates(` as string literals to search for, and its own regex matched them — the arg-count rule then fired on the literal. It passed locally only because the file was still untracked when it ran, so `git ls-files` did not list it; committing it made the audit scan itself and fail CI on the first run. Exempts the audit's own source alongside the module that defines the primitives. Verified the guard still fails on both regression shapes after the exemption. |
||
|
Vikhyath MondretiandClaude Opus 5
|
91e927442b |
fix(media): bound the ffmpeg tool's child processes, inputs and scale targets (#6989)
* fix(media): bound the ffmpeg tool's child processes, inputs and scale targets The copilot ffmpeg tool shells out to FFmpeg in the Sim app process — not in a sandbox — and nothing bounded the run. runCommand had no timer, no kill and no signal, so a transcode ran until it finished and survived the request that asked for it: a stopped copilot turn left the encode pinning both cores of a shared instance. Every operation now shares one 10-minute wall-clock deadline, and both the deadline and the caller's cancellation SIGKILL the child. The budget is per-operation rather than per-command because concat runs a full re-encode per input, so a per-command timeout would let N inputs multiply into N timeouts. Cancellation is wired to context.abortSignal, whose every abort reason is an explicit user stop — the copilot lifecycle tracks a passive client disconnect separately and does not abort on it — so an encode dies when the user asks and not before. (userStopSignal, which assertServerToolNotAborted reads, has no producer on this path.) ffprobe had the same shape in miniature: fluent-ffmpeg's static ffprobe hands back no process handle, so a timeout could only race the callback and leave a wedged prober alive, and concat probes once per input. It now runs through execFile, which takes timeout, killSignal and signal natively. Scale targets reached the filter graph unvalidated, where libavfilter sizes its per-frame buffers from them — scale=30000:30000 is ~2.7 GB a frame, allocated in a child that shares the instance's memory. Dimensions are now rejected outside 16-4096 with a message the model can act on, plus an area cap. concat's targets come from the source container rather than a caller assertion, so those are clamped instead. Two further holes: readOut buffered the output with no ceiling, and CRF-18 re-encodes routinely exceed their input, so the input budget did not bound it; and the handler accepted unlimited input files. Both are capped, and the byte check now runs against the recorded size before the download rather than after. FFMPEG_LIMITS is the single source for the four numbers the Go tool catalog mirrors into its schema, pinned by ffmpeg-schema-parity.test.ts so the model is never told a ceiling the executor does not enforce. Companion: simstudioai/copilot#PENDING Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(media): fit concat inside the shared area budget and stop input prep on cancel Two review findings, both real. concat clamped each probed axis independently, so a 4096x4096 source produced a normalization target of exactly that — nearly double the area budget scale_pad enforces through the same scale=/pad= graph. The pair is now scaled down together, so aspect ratio survives and one ceiling governs both entry points. Dimensions come back even too, which yuv420p requires and per-axis rounding did not guarantee. Preparing the inputs is itself the expensive half of a many-file call — up to the whole byte budget in storage reads — and the abort signal was only consulted after the loop, so an explicit stop was observed only once every download had already finished. The loop now checks it per input. Also corrects the rationale comments on the mirrored limits. maxItems/minimum/ maximum do not reach the model: copilot's NormalizeToolParameters allowlists type/properties/items/description/enum/required, so the bounds travel only the contract path, where Sim's router validates against them. That is still worth having — it turns an out-of-range argument into a structured rejection before any storage read or child process — but the model learns the limits from the parameter descriptions, and the comments now say so instead of claiming the schema teaches it. * fix(media): floor the scaled concat axes so the area bound actually holds The scale factor lands both axes on a product of exactly the budget, so an axis allowed to round up can put the pair back over it — and when both round up and both land even, nothing pulls them back. A brute force over every dimension pair in 16..4096 finds 219,280 that violate the bound under round-to-even, worst 2694x3520 -> 2688x3512, 3072 pixels over. Under floor-to-even: none. Flooring keeps each axis at or below its exact target, so the product cannot exceed the budget. Probed dimensions are already integers, so this changes only the scaled path. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Vikhyath Mondreti
|
2c61b2989c |
fix(analytics): stop dropping client events captured before PostHog init (#6988)
* fix(analytics): stop dropping client events captured before PostHog init posthog.capture is a no-op until init() has run — its body sits behind if (this.__loaded) with no buffer and no warning. PostHogProvider reaches init through two dynamic imports while a caller needs only one, so mount-time events and error-boundary crash reports during first paint landed in that dead window and were discarded silently. Also routes the workflow canvas error boundary through captureException so its crashes reach error tracking with a parsed stack. * fix(analytics): drop unactionable browser noise from error tracking ResizeObserver loop notices were 92% of everything client-side exception autocapture reported. They are specified behaviour, not faults — the observer defers the remaining notifications to the next frame — and they carry no stack that points anywhere. Alongside opaque cross-origin "Script error." reports and cancellation signals (fetch AbortError, Monaco's CancellationError), they bury the handful of real crashes. Filters them in before_send so they never leave the browser. Fails open on anything unrecognized: a wrong guess here would delete product analytics, not just over-report. * fix(analytics): match cancellations on error name, not coerced type Two holes in the exception filter, both found reading PostHog's coercers rather than trusting the shape. The DOMException coercer always reports type "DOMException" and folds the name into the value as "AbortError: signal is aborted without reason", so matching AbortError on type alone never fired for a real aborted fetch — the filter was dead code for exactly the events it was written for. Match the error name wherever the coercer put it. The filter also applied to deliberate captureException reports, which our error boundaries make. Only exceptions the browser raised itself (mechanism.handled === false) are now eligible; a report we chose to send is never dropped. * test(analytics): make posthog client tests order-independent Readiness is module-level and settles once, so the capture and exception cases silently depended on an earlier case having settled the gate and failed when run in isolation. Walk the lifecycle in one case instead. Moves the disabled-analytics case to its own file, where vitest's per-file isolation supplies an unsettled gate — dropping the vi.resetModules() and dynamic import the repo's testing conventions rule out. Replaces the hand-rolled setTimeout flush with vi.waitFor. |
||
|
Waleed
|
b654bc11cd |
improvement(navigation): eliminate hidden workspace requests (#6979)
# Conflicts: # apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx |
||
|
Waleed
|
ab35ff4c8c |
fix(resources): make context menus selection-aware (#6980)
* fix(resources): make context menus selection-aware * fix(resources): correct bulk action scopes * fix(knowledge): avoid overstating select-all scope * fix(knowledge): keep select-all actions consistent |
||
|
Waleed
|
e46fc1d1f5 |
improvement(settings): reduce navigation and section load latency (#6992)
* improvement(settings): reduce navigation and section load latency * improvement(settings): seed a section's first-paint data on the server A settings section paid three serial hops: the route payload, its lazily loaded chunk, and only then its own queries. The first two are addressed; this starts on the third, which is what fills in after the heading paints. Generalises the existing general-settings prefetch into a per-section registry, awaited before dehydration and started as soon as workspace access is established, so the body renders populated the moment its chunk lands instead of blanking again while it fetches. Adds credential-groups as the first section beyond general/billing/admin. It qualifies on both counts the registry requires: `listCredentialGroupSettings` is a `defineAuthorizedWorkspaceUseCase`, so prefetching through it applies the same authorization and entitlement checks a client request would, and it returns exactly what the hook caches under `credentialGroupKeys.list`. The registry is deliberately sparse. An audit of all 29 sections found that most authorize in their route handler rather than their data layer, so prefetching them means first lifting that check out of the route — and two must never be prefetched naively at all, because the route is also what redacts the response: SSO strips the OIDC client secret before it leaves the server, and MCP withholds upstream credential headers from read-only members. Reaching past either would ship secrets into the dehydrated payload. * fix(settings): keep optional prefetch failures nonfatal * fix(settings): surface organization load failures |
||
|
Waleed
|
b1759f3545 |
improvement(menus): band by what the action acts on, not by verb (#6994)
Replaces "at most one separator" with the test that actually earns a rule: does the next group stop acting on the thing the user clicked. Most row menus have only the destructive transition and keep their one rule. Three menus have a second, and now show it — the logs row menu between this log and the page's filters, and the table row and column menus where they stop acting on the clicked cell or column and start creating siblings. Count follows content rather than a fixed number. The workflow panel menu had six items across four scopes and no separator at all, so "Delete workflow" sat flush against "Duplicate workflow"; it now carries the destructive rule every other menu has. Two label fixes: the logs menu wrote 'Retrying...' with ASCII dots two lines above 'Stopping…' with the character, and "Import CSV…" was the only ellipsis anywhere in the workspace menu surface while the same action reads "Import CSV" in the list menu. |
||
|
Waleed
|
37590bdf73 | fix(csv): neutralize formula-leading exports (#6993) | ||
|
|
c26529a82e |
feat(bitbucket): add repository webhook triggers (#6934)
* feat(bitbucket): add repository webhook triggers * fix(bitbucket): harden webhook trigger delivery * fix(bitbucket): address final trigger review * chore(bitbucket): address review conventions * fix(bitbucket): harden triggers and connector sync --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Theodore Li
|
7173a3fb37 |
feat(status): surface major service incidents (#6987)
* feat(status): surface major service incidents * fix(status): isolate status endpoint failures |
||
|
Vikhyath Mondreti
|
7ea8692e7d |
fix(files): read a file nobody vouched for, as an untracked one already is (#6986)
* fix(files): read a file nobody vouched for, as an untracked one already is A workspace file whose secret provenance could not be recorded was refused by every model and runtime boundary at once — attachments, mounts, and the tool routes that parse, transcribe or describe a file. A file that was never tracked at all returned exact-empty and worked fine. Both say exactly as much about their contents: nothing. So the refusal was a permanent penalty for having tried to record provenance and failed, with no way back, since nothing rewrites a file's provenance but another content write. Workspace files now do what the other durable surfaces do — proceed and record an audit entry naming the surface and the count, so the people who own the secrets are told. The state is distinguished from the four that really are unknown (missing row, version mismatch, stale binding, malformed), which stay refused; only a current sidecar that says so reads as unrecorded. Absence survives a merge rather than dropping through to exact, so a derived file cannot launder an unrecorded input into a positive claim. Script migration 0007 clears the files already stranded in the old state, taking the parent lock first in id order to match the live writer, with the unknown status re-checked under it so a concurrent write is never undone. Nothing changes for a legacy file: no sidecar row still means untracked, still reads exact-empty. The enforcement flag stays in place. * fix(files): narrow an initialization to a status the column accepts The union carries three states; the sidecar's CHECK constraint accepts two. Only the replace path discriminated — initialize forwarded the status verbatim, so an 'unrecorded' would have reached the database as a value it rejects, aborting the enclosing transaction rather than writing a bad row. Nothing passes one today, which is why it belongs here and not in a caller. Also names the storage mismatch on the type itself: a stored 'unknown' reads back as 'unrecorded', not as the union's 'unknown'. Same word, two layers, different meanings. * fix(provenance): repair only what the reader calls an absence The repair selected on status alone, which is wider than the branch it was meant to undo. The reader answers 'unrecorded' only for a sidecar that is version 1, bound to the file's current bytes, and holding a well-formed entries array; anything else is a fault it refuses and no policy relaxes. Clearing a fault was not a smaller claim but a larger one. Deleting the sidecar sets secret_provenance_version to NULL, and a NULL version reads as exact-empty — so a file refused because its provenance described bytes it no longer holds would have come back positively vouched for, silently, with no audit entry. Four such rows exist in production today. Both the candidate query and the delete now carry every condition, so a concurrent write cannot lose the distinction between an absence and a fault. Stale, unversioned and malformed sidecars stay refused exactly as the surface refuses them, and recover as they always have, on the next content write. * fix(provenance): store an absence apart from a refusal Relaxing a stored `unknown` assumed it always meant "nobody recorded this". It did not. The sidecar collapsed two opposite claims into one value: bytes nobody recorded, and bytes a writer refused on purpose — a child of an archive whose parent provably held secrets, a generated asset whose safety decision came back false, a transcode whose scanner knew secrets were present and could not locate them. Relaxing the second would have walked known-secret-bearing content through every model boundary at once. The decision type already told them apart: a registry that never ran is `safe: true`, while every refusal is `safe: false`. Only storage lost it. Writers now persist `unrecorded` and `unknown` separately, readers relax the first and refuse the second, and the constraint is widened to accept it. Only workspace files store this distinction, deliberately. Every other durable surface produces a non-exact sidecar from one condition — an incomplete incoming bundle or registry — which is always an absence. Files are the only surface that derives one stored object from another, so they are the only one that can refuse on purpose. The shared policy is unchanged and uniform: read an absence and audit it, refuse a taint. Also aligns the metadata batch classifier, which answered `unknown` where the single-file reader answers `unrecorded`, leaving two classifiers describing one policy differently. * test(provenance): assert the enforcement switch on a row it can act on The test that pins "closing the surface refuses an unrecorded file again" used a stored unknown, which the split had just turned into a taint. A taint is refused whatever the flag says, so the assertion held with enforcement off too and proved nothing about the switch this posture rests on. It now uses a recorded absence, and fails if the flag is ignored. * fix(files): copy a recorded absence as an absence The copy path required an exact source, so a stored unrecorded one fell in with the refusals and the target was written as a taint the source never carried. A workspace fork or a chat file copy therefore turned a readable file into a permanently refused one — the pathology this surface exists to undo, reached by copying — and nothing rewrites a file's provenance but another content write. Safe past the scope checks it now precedes: those exist to stop one workspace's secret entries landing in another's file, and a recorded absence has no entries to carry. The legacy Function export marker keeps writing unknown. Its record had resolved secret names in scope and an unreadable bundle, so whether that is an absence is not something this change can establish, and guessing in the readable direction is what caused this round. |
||
|
Justin Blumencranz
|
fb8f0d66d1 | improvement(files): improve file sharing UI (#6983) | ||
|
Vikhyath Mondreti
|
8c7a2f1df0 |
fix(uploads): require an explicit byte ceiling on workspace-file downloads (#6985)
* fix(uploads): require an explicit byte ceiling on workspace-file downloads Workspace files are admitted at 5 GB because they stream straight to object storage, but a tool that pulls one back to hand it to a third party buffers the whole thing in the shared app process. maxBytes was optional on every download helper, so 51 call sites had silently inherited "unbounded". Make maxBytes required on all five entry points so a new call site cannot inherit it again, and give each existing site a ceiling: the destination's own documented limit where the route already declared one, otherwise the 100 MB this codebase already uses for buffered work. Multi-attachment routes were the worse case — Gmail, Outlook, SendGrid and SMTP downloaded every attachment via Promise.all and only summed the sizes once they were all resident, so their pre-check on declared sizes protected nothing. Add downloadServableFilesWithinBudget, which walks the list against a shrinking budget, and use the same running budget in Slack, Jira, Discord and Quiver. * fix(uploads): bound Sim-page asset inlining before the bytes are resident The ceiling on the rendered page checked the finished document, by which point renderSimPageDocumentWithAssets had already downloaded every referenced image concurrently with no per-download limit and base64-inlined them — so the allocation the check exists to prevent had already happened. Pick the inline set from recorded sizes before fetching anything, against a per-document budget as well as the existing per-image one, and give each download its own ceiling in case a row understates its object. An image that does not fit keeps its URL reference, exactly as an oversized one already did. * improvement(uploads): charge the page-render budget by delivered bytes Planning the inline set from recorded sizes left the aggregate ceiling resting on metadata being accurate, and needed a paragraph explaining why that was safe. Downloading one image at a time and subtracting what each download actually returned needs no such argument: the budget cannot be exceeded whatever a row says, and the peak is one image rather than the sum of them. Also drop two rough edges the first pass left behind — an SFTP total-size check that became unreachable once the download carried the remaining budget, and a Dataverse error helper whose optional size argument existed only to paper over one caller that had not passed it. |
||
|
Vikhyath MondretiandClaude Opus 5
|
28cfa4361a |
fix(deploy): resolve subblock values to their configuration on both sides of change detection (#6984)
* fix(deploy): resolve subblock values to their configuration on both sides of change detection Focusing a deployed trigger block flipped the deploy button from Live to Update for a change the user never made, and redeploying could not clear it. Deploy materializes every declared `defaultValue` into `webhook.providerConfig` (`getConfigValue`). Opening a trigger block's panel reads that derived artifact back into live state through a non-persisting `setValue`, so the block gains keys the DB draft — which deploy snapshots — does not have. The comparison then reported a difference for a field nobody set. Because deploy snapshots the draft, the next deployment lacked the keys too: a fixpoint. Measured on production: 89 of 89 recent `generic_webhook` deployments were missing `acceptOtherMethods`, and 1,046 of 2,139 active webhooks carry at least one `providerConfig` key their block has no entry for. This is the tenth instance of one failure mode — |
||
|
Vikhyath Mondreti
|
b861062370 |
fix(provenance): clear the rows that went unknown after the first repair (#6982)
* fix(provenance): clear the rows that went unknown after the first repair 0005 is finished and will not run again — the runner records a name in script_migrations and never offers it back, which is the contract a run-once repair wants. But it cleared the backlog that existed at the instant it ran, and the writer that produced that backlog kept running until the fix in this branch. Nothing heals such a row in place, so each one goes on reporting on every later read; a few dozen of them account for thousands of log lines a week. A second entry rather than deleting the first's tracking row: the registry is append-only, and a repair that ran twice should say so twice. It shares 0005's walk rather than restating it — the parent-first lock ordering and the status re-check under that lock are subtleties worth having once — and is idempotent, so it costs one empty query if there is nothing left to repair. * chore(provenance): record the deploy ordering the second pass depends on promote-images needs migrate, so a script migration runs while the previous image is still serving. A row an old instance creates between this walk and the end of the rollout sits behind the cursor, and the name is recorded on success, so it is never offered again. Widening the walk would not help — the exposure is the minutes after it returns, not the milliseconds during — so the requirement is to ship it in a release after the writer fix is already promoted, which the file now says. |
||
|
Vikhyath Mondreti
|
a721a76044 |
fix(provenance): stop requiring a projection of roots no model sees (#6981)
* fix(provenance): stop requiring a projection of roots no model sees A secretProvenance selection is the opposite mechanism to a projected model input: the value reaches an internal API unchanged, with its provenance alongside it in the private bundle, precisely so nothing has to be substituted. selectBlockBoundaryPaths marked those roots required to project anyway. That made a projection failure fatal for tools with no way to project. createStructuredModelProjection rescues only a mode: 'project' tool with an applyProjected, so for the twenty-odd secretProvenance-only tools it returns undefined on its first check. table_insert_row declares no modelInput at all and posts row data to the table API; when the Table block's params threw on its projected data the whole run's registry latched, costing provenance for every later boundary — including the table write that prompted it, whose rows were then stored unknown and re-reported on every later read. Track those paths, require none of them. A root is required to project when a model will see it, which is what modelInput declares. Also separate a crossing that carried no provenance from one that was rejected, so a run that failed before producing any stops reporting a by-design state as an originating fault; and carry the first guard's location into the diagnostics a refusal reports, so a downstream reporter names where rather than only what. * fix(provenance): report where the guard tripped on a refusal too The registry retained the first guard's location, but the refusal reporter copies named fields out of the diagnostics and never picked it up — so the line an operator actually reads when a projection is refused still named only the reason. Nested rather than spread flat: the guard's `inputPath` names where it tripped and the refusal's names where the refusal happened, which differ whenever a latch travels, so flattening would overwrite one with the other. |
||
|
Theodore Li
|
6a5e2502c3 | improvement(ship): skip confirmation prompts (#6977) | ||
|
Bill LeoutsakosandBill Leoutsakos
|
be508b0789 |
fix(utils): prevent randomInt power-of-two hangs (#6955)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Bill LeoutsakosandBill Leoutsakos
|
87ceaf2e20 |
fix(auth): recover from SSO provider lookup errors (#6966)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Waleed
|
4b4ab2479f |
improvement(menus): one separator per menu, before the destructive action (#6974)
* improvement(menus): one separator per menu, before the destructive action Menus banded themselves into semantic groups — navigation, status, edit, copy, destructive — behind two to four separators each. No toolbar in the app renders a divider: every header is a flat gap-1 chip row and every bulk action bar a flat gap-[5px] run. The bands therefore taught a taxonomy the user met on no other surface, and because each band is conditional, the same action landed in a different group depending on which siblings happened to be visible. Pin sat alone in one caller of the shared workflow menu and beside Duplicate in another. Every menu now carries at most one rule, immediately before the destructive group. Order is untouched, so the toolbar-mirroring the ordering rule requires is unaffected. Two separator bugs fixed. The logs row menu had two unconditional separators above conditional items, so a log already filtered by its workflow with no active filters ended on a dangling rule. The shared workflow menu guarded its destructive rule on showLeave alone while the Leave item required showLeave && onLeave, so a caller passing showLeave from a permission check with a conditional onLeave would trail a rule under the last item; every term in both guards is now the exact render condition of the item it stands for. Removes groupNonDestructiveActions and separateNavigationAction. Between them they moved one separator for one caller, four of six branches were unreachable, and separateNavigationAction had no observable effect anywhere in the repo. The separator matrix was previously untested, which is how the showLeave asymmetry survived; it now has invariants including a flag sweep. * fix(menus): build every separator guard from its items' exact conditions Review caught two places where the grouping rule and the code disagreed. The tables row menu guarded its rule on `onMove` while the Move submenu needs a non-empty `moveOptions`, so a table whose other actions were all absent and whose move list was empty would draw the rule with nothing above it — the exact looseness the rule warns about. The logs row menu puts its one rule after Retry and Cancel Run rather than before a destructive group, which the rule as written did not cover. Retry is the primary action on a failed run and belongs at the top; the rule now describes the separator as fencing the consequential group at whichever end it sits, and names the logs menu as the one place that group leads. Also aligns three empty-space menu labels with the header chips they mirror: "Add document" and "Create chunk" were the only create actions not matching their toolbar, and the files menu said "Upload file" where its header says "Upload". Run order in the two column run menus now matches the action bar and the row menu — incomplete before all, not all before incomplete. * fix(menus): apply the one-rule grouping to the folder context menu The folder row menu kept a separator at its canEdit permission boundary plus one before Delete — the same shape already corrected in the file row menu, missed because the sweep that found it did not cover this file. Open and Pin above are unconditional, so the surviving rule is always backed on both sides. Also records the standing exception the sweep surfaced: the text editor, terminal, and browser page menus emulate native OS menus, whose banding the user learns outside Sim. That is the ordering rule's own principle — mirror the surface they already read — so those keep their banding while our own resource and row menus, whose toolbars are flat, take one rule. * fix(menus): keep empty-row actions together |
||
|
Waleed
|
d1b01849a8 |
improvement(perf): cut server-only and unused code out of the workspace client bundles (#6975)
* improvement(perf): cut server-only and unused code out of the workspace client bundles Every workspace route shipped JavaScript it never executes. Four independent import edges, each fixed by moving a symbol rather than changing behaviour: - js-tiktoken's BPE rank tables (5.4 MB source / 2.5 MB wire) reached the workflow editor because the tokenization barrel re-exported the exact counters alongside the character heuristics. Split into lib/tokenization/accurate.ts, which the barrel no longer re-exports. - crypto-browserify (~105 KB gzip, all 26 workspace routes) came from the Salesforce and Gong triggers importing webhook provider modules that reach node:crypto through @sim/security. The two symbols they actually needed are now in crypto-free modules. - tables, files and knowledge each imported one dependency-free hook from the sidebar-hooks barrel, whose other exports reach the 5 MB generated tool-metadata artifact. Deep-imported per the code-splitting rule in sim-imports.md. - lib/workflows/subblocks/display.ts imported a string constant from a React module under app/, inverting the app/lib layering. Moved to lib/. Also adds a loading boundary to chat/[chatId]. Without one, a dynamic route is prefetched as nothing, so clicking a chat held the previous chat on screen for the whole server round trip. Measured on a production build, JS downloaded before the load event: /w/[workflowId] 7.38 MB -> 4.80 MB (-35%) /logs 4.57 MB -> 4.44 MB /knowledge 4.35 MB -> 4.22 MB /home 4.57 MB -> 4.44 MB crypto-browserify no longer appears in any shipped chunk. The tool-registry boundary baseline is retightened so the reclaimed graph weight cannot silently regress. * improvement(sidebar): move useContextMenu to shared hooks Review flagged the four workspace routes deep-importing `useContextMenu` from the sidebar's hooks barrel as a barrel-convention violation. Fair — the code-splitting exception in sim-imports.md is written for `lazy()` splits, and these are static imports. The hook was in the wrong place to begin with. It is entirely generic — no sidebar-specific references, just right-click state and positioning — and nine consumers across tables, files, knowledge, home, the terminal and the preview editor already reached across features to get it. Moved to `@/hooks`, the repo's shared-hooks location, and every consumer including the sidebar's own now imports it from there. This satisfies the barrel convention rather than making an exception to it, and keeps the graph win: tables, knowledge and files stay off the sidebar barrel's path to `stores/workflow-diff -> serializer -> tools/metadata`, unchanged at 20.19 / 20.89 / 21.37 MB of reachable source. |
||
|
Waleed
|
3a04426f37 |
improvement(settings): make workspace settings navigation feel instant (#6964)
* improvement(settings): make settings section navigation feel instant
Every settings tab switch ran four sequential round-trips with no visual
feedback: a cold RSC request, then the panel render, then the section's
lazy chunk, then its queries. The heading was pushed up from the section
body, so the most static thing on the page arrived last and visibly
blanked between sections.
- resolve the section heading in the route layout from its navigation
entry, so it paints with the shell instead of after the body's chunk
- add loading.tsx to all four settings planes, so a click commits the
navigation immediately instead of holding the outgoing section
- give every code-split section a shared skeleton fallback, reused by the
route boundary and the in-page Suspense boundary
- prefetch the route payload on sidebar hover/focus; the rows are buttons
for the unsaved-changes guard, so they never got Next's <Link> prefetch
- scope the general-settings server prefetch to the three sections that
read it, instead of blocking all 28 on it
- set staleTimes so returning to a tab reuses the client router cache
rather than re-running the access gate
- warm workspace credentials under the type the secrets panel queries;
the previous warm wrote a different cache entry and never landed
* improvement(settings): drop the skeleton and the app-wide router cache change
Follow-up review of the diff against the rest of the platform.
- The 4-row body skeleton had no precedent at this layer. Every route-level
fallback in the app renders real chrome over empty content instead:
ResourceChromeFallback renders its header and column headers with rows={[]},
and the credit-usage fallback renders its real title and description over
nothing. Skeleton is only ever used for in-component sub-regions. The
loading boundaries now render an empty body, so the heading is what signals
arrival and nothing shifts when the real body lands. This also removes the
shared dynamic() options module, leaving all four section renderers
untouched by this PR.
- Remove the experimental.staleTimes block. static: 180 silently downgraded
Next 16's own default of 300, and dynamic: 30 is an app-wide change to
client router cache reuse that this PR does not need: the segment cache
already floors prefetch entries at 30s, so hover prefetch pays off without
it. It deserves its own PR and its own measurement.
- Restore the six section chunk warms that existed before this PR. Dropping
them alongside the 28-section map was an unintended regression; they were
already in the module graph, so warming them costs nothing.
* improvement(settings): fix header regressions found in review
Six independent review passes over the diff. Three real defects, all
introduced by the header-meta fallback or the loading boundary.
- A denied organization section rendered the section's catalog heading,
description and Docs link above a "you do not have access" body, because
SettingsUnavailable renders its own centred heading and registers nothing.
It now claims an empty header, which is how a body opts out of the meta
fallback. Releasing the header is an explicit null rather than an
EMPTY_CONFIG sentinel, so "no body owns this" is stated instead of implied.
- The account credit-usage route resolves to its parent billing section, so
the shell painted "Billing" in the server frame before hydration swapped in
"Credit usage". The shell now only supplies meta for a section's own route,
not for detail routes beneath it.
- Adding loading.tsx put the page inside a Suspense boundary, where
notFound() and redirect() can no longer set the response status: a legacy
or unknown settings URL loaded directly answered 200 and redirected in a
second round trip instead of 307/404. Segment-level routing moved into the
layout, above the boundary, which is also where it belonged.
Also from review:
- Parallelize two pairs of independent awaits in the access gate. Every await
there sits in front of the section body, so this shortens the exact wait the
PR is about.
- Note in settings-header that the layout effect is load-bearing: a passive
effect would let the previous section's title show for a frame.
- Correct the loading.tsx docs, which claimed the empty body matched every
other route-level fallback. It is the only null one; the accurate statement
is that the shell above it already renders the chrome.
- Tests: cover resolveSettingsSection's alias table (previously untested on
either side of the move) and the general-settings prefetch gate. Strengthen
the wholesale-substitution test, which passed against a field-merge
implementation because SettingsPanel always emits a description key. All
four verified against mutants.
* improvement(settings): scope the change to the workspace settings plane
Two review findings, both from extending the mechanism to the account,
organization and self-host planes without extending the fixes with it.
- The loading boundary softened 404s on those three planes. Segment
validation was moved above the boundary for the workspace plane only, so a
direct load of an unknown or legacy segment on the others answered 200 and
soft-404ed after hydration.
- SettingsUnavailable's opt-out registers in a layout effect, which does not
run during SSR. A direct load of a denied organization section still painted
the denied section's catalog title, description and Docs link until
hydration — the exact caption the opt-out was added to prevent.
Feeding the header server-side on those planes needs per-section access at
layout level, which is a routing change well beyond this PR. So the three
standalone loading boundaries, the standalone shell's meta, the
SettingsUnavailable opt-out and the standalone sidebar's route prefetch are
all reverted: without a loading boundary in the subtree the scheduler skips
the segment request, so that prefetch bought nothing on its own.
What ships is the workspace settings plane, where the same mechanism is
correct end to end: segment validation and the heading both resolve in the
layout, above the boundary, and a denied section redirects rather than
rendering an unavailable body under a header.
* improvement(settings): stop re-resolving credential-group availability
The access gate asked `isCredentialGroupsAvailable` for an answer the host
context had already derived from the same owner billing one await earlier, so
every workspace-section navigation paid a second feature-flag lookup to learn
what was already on `hostContext.features`.
* improvement(settings): resolve only the entitlements the gate can act on
The access gate fanned out four entitlement lookups for every workspace
section and then built the whole navigation list to ask whether one section
was in it.
- `inbox` and `sandboxes` feed only `locked`, which marks a section as needing
an upgrade rather than hiding it. The gate reads membership alone, so those
two billing round-trips could never change the outcome for any section.
Removing them is behaviour-identical, not a narrowing.
- `forks` is read only by the `forks` entry, so every other section was
resolving a lineage check it could not act on.
`permissionConfig` is deliberately left alone: its keys hide sections, so
skipping the lookup for a section that turns out to be config-gated would
reveal it. That fails open, where the other two fail closed.
Opening a section such as secrets or byok now awaits nothing beyond the
already-conditional permission-group read.
Also corrects the chunk-warmer rationale. Measurement showed the cost is the
boundary audit counting `import()` as a graph edge, not parsed JS — each
section is already `dynamic()`-imported by the panel — and that code-splitting
the sidebar moves exactly one module, so it cannot unlock warming the rest.
|
||
|
Waleed
|
8937bb3550 |
fix(kb): let the server say a connector sync is queued (#6968)
* fix(kb): let the server say a connector sync is queued The connector chip inferred "a sync is coming" from `createdAt` inside a 2-minute window, because nothing on the row distinguished a queued sync from an idle connector until a worker took the lock. The guess was wrong under queue backlog and under client clock skew, and it forced a pile of client state to stand in for it. Adds `pending`, written as the sync is handed to the queue and cleared when a worker takes the lock or the hand-off is found to have been lost. It is a phase of the same lock `syncing` holds, so it opens the lease and takes an ownership token the same way — the lease is what the scheduler ages a stranded queue entry against (`updatedAt` cannot serve: a pending connector is still editable, so any unrelated write would renew the recovery it should trigger), and the token is what proves a late release belongs to this dispatch. Deletes the 2-minute window, the in-flight id sets, the 5-minute cooldown timers and the forced re-render they needed. The cooldown lived in a ref inside a modal, so it evaporated whenever the modal closed; the disable now comes from durable server state and is shared across tabs. Also fixes, all found while tracing the lifecycle: - An on-demand sync on a paused or disabled connector silently resumed it for good. Nothing could put the pause back: success writes `active`, a lost queue entry writes `error`, and the due-sweep keeps syncing that. Refused. - A failed hand-off no longer advances the connector's auto-disable breaker. A queue outage would otherwise increment every connector in the fleet until they all disabled themselves for a fault that was never theirs. - Manual sync on an established connector gave no feedback at all: the poll only ran while the predicate matched, which it never did. - Four over-broad invalidations that refetched every cached chunk page and chunk search in a base when one connector document was excluded. - The dead-process reporter re-sent a PATCH per stale document on every poll. * fix(kb): refuse to start a queued run on a paused connector The queue outlives the decision to sync. Pausing a connector after its run was queued cleared the queue entry's token but left the task itself alive, and the lock CAS accepted any row that was not already `syncing` — so the worker took the paused row and wrote its own terminal `active` over the pause. Moves the rule to the two points that can enforce it: an explicit `LOCKABLE_CONNECTOR_STATUSES` allowlist on the lock acquisition, and the same allowlist on `markSyncPending`, which closes the mirror race where a dispatch already in flight rewrites a just-paused row back to `pending`. Queueing and starting now agree on one rule, and a skipped hand-off is reported as its own outcome rather than a concurrency conflict. Also patches the connector detail cache alongside the list on an optimistic status write, so an already-expanded card starts its own sync poll instead of showing stale history behind the list's spinner. * fix(kb): make a queued sync prove it is the run that was queued `markSyncPending` minted an ownership token but only `releaseFailedDispatch` checked it, so the worker could consume a queue entry that was not its own. A task delayed past its lease is reclaimed and replaced; the status check alone let that stale task take the replacement's entry and run superseded options — a plain sync where the user had just asked for a full resync — while the replacement was turned away as `sync_in_progress`. Carries the token in the task payload and matches it at lock acquisition, the same discipline `holdsSyncLockToken` already applies to the `syncing` phase, extended to the phase before it. A superseded run is now reported as such rather than as a concurrency conflict. The payload field is optional for the rollout window only: tasks already in the queue carry no token, and stranding them would be worse than letting them fall back to the status check for one deploy. * fix(kb): report a paused connector as paused, not superseded Pausing a queued connector releases its token, so testing ownership before status reported every pause-while-queued — the common case — as a superseded dispatch. The mismatch is the symptom there; the status is the reason. * fix(kb): stop a status update landing on a run that already started The update's guards ran against a row read moments earlier and the write carried no compare-and-set, so a worker taking the lock in between meant the write landed on a `syncing` row — overwriting the run's status and, because leaving `pending` also clears the lock columns, wiping the token its heartbeat and terminal write match on. That stranded a sync that had already begun. The write is now conditional on the status the request was authorized against, and a lost race is reported as a conflict rather than "not found". Also restores the in-flight guard on the pause control. The optimistic status flip relabels it Pause -> Resume immediately, so a second click could send `active` before the first pause settled and resume a connector the user meant to pause. Read from the mutation's own pending state rather than the local id set this PR removed — React Query already knows which row is in flight. |
||
|
Waleed
|
8689237b4f |
improvement(files): match the preview toolbar's height to the tab strip above it (#6973)
The toolbar had no height of its own — `py-1` around a 30px chip came to 39px against the tab strip's 41px. Two stacked bars two pixels apart read as a mistake rather than as two different bars, and the toolbar's chips still stood taller than the 26px tabs. It now takes the same 40px content box over a 1px border, so the chips centre in the same band the tabs do. Covers all four previews that mount it — pdf, docx, pptx and the zoomable image/svg surface — none of which override its height. |