v0.8.10: security hardening, file sharing, Bitbucket triggers, UI performance improvements - #7007
Merged
Conversation
…ools (#6946) Co-authored-by: Sim Pi Agent <pi@sim.ai>
…#6942) * fix(billing): separate Enterprise reporting periods from Stripe terms * fix(billing): reconcile accepted legacy intents * fix(billing): keep accepted legacy intents fail-closed * fix(billing): reconcile accepted retired intents * fix(billing): retire invalid legacy intents
* feat(cli): add chat command * fix(cli): harden chat command execution
…edentials (#6941) * fix(security): stop redirects replaying request bodies and leaking credentials `secureFetchWithPinnedIP` passed its options straight into the redirect recursion, so a 301/302/303 replayed the original method and body — delivering a non-idempotent write twice — and forwarded `Authorization` and every other caller header to whatever origin the upstream named. `followRedirectsGuarded`, a hundred lines above it in the same file, already had the correct RFC 9110 rules. The two had drifted, and the drift is the bug. Both now route through one `resolveRedirectHop`: - 303, and 301/302 on POST, degrade to a bodyless GET and drop the entity headers that described the removed body. - A cross-origin hop drops every caller header, not just `Authorization`. - A cross-origin hop that would forward a body is refused. `stripAuthOnRedirect` still narrows same-origin hops for endpoints that redirect to a target carrying its own signed URL. Verified by stashing the fix and re-running: 4 of the 6 new tests fail against the old code. The 2 that pass either way cover same-origin behaviour that was already correct. * fix(api): preserve HTTP redirect compatibility
…xes around them (#6943) * fix(delivery): stable provider idempotency tokens, and correctness fixes around them Six independent fixes found while investigating a Slack transport failure. None of them are that failure; all of them are live. **Money writes could be delivered twice.** Square (8 tools), Brex (5) and Outlook Calendar minted their provider idempotency token with `generateId()` at request-build time. That is stable inside the transport retry loop — `prepareToolRequest` runs once above it — but a BLOCK-level retry re-enters the handler and mints a fresh one, defeating the provider's dedupe. A builder ticking "retry" on a Square block turned a committed write into a second card charge, and a Brex one into a second money transfer. Tokens now come from `deriveDeliveryKey`, a pure function of execution + block + tool + invocation, so every retry layer derives the same value. Stripe joins them: all ~50 tools previously sent no `Idempotency-Key` at all. The comment on `brex/create_transfer.ts` claimed a fresh key per transfer *prevents* duplicate money movement. That is inverted — fresh per *attempt* is what permits it — and is corrected here. **`invocationId` is required, not optional.** Deriving from `executionId` alone would be worse than the bug: five loop iterations paying five invoices would share one token, the provider would honour the first and silently drop four real payments, and it would look like five successes. Also: - `INTERNAL_API_BASE_URL` is now ignored on Trigger.dev workers. It names a route that resolves only inside the app container, and several modules run in both runtimes — `guardrails/mask-client.ts` says so in its own TSDoc — so setting it produced `PII redaction failed: Unable to connect` on every worker-side redaction. Mirrored into `packages/testing`'s urls mock, which reimplements the function and would otherwise have diverged. - `engines.bun` raised to >=1.3.14. Measured on 1.2.15, which the old floor permitted: a fully-delivered POST is silently replayed and the caller sees 200. - CloudWatch `put-metric-data` pinned to `maxAttempts: 1`. The AWS SDK default of 3 already replayed it, and `PutMetricData` aggregates rather than overwrites, so a duplicate silently corrupts the customer's metric series and alarm thresholds. - `webhookIdempotency` given a bounded in-progress lease. An untimed run held a SEVEN DAY lease while concurrent duplicates polled it once a second. Verified: `tsc --noEmit` clean, 30,100 tests pass. * fix(ci): restore staging files the branch split had reverted, and format Three problems, all from assembling this branch by checking paths out of a WIP branch built on an older `staging`. Anything `staging` changed since that base came back as a revert. - Root `package.json` had lost the `opentype.js` / `@types/opentype.js` dependencies `staging` added, which desynced `bun.lock` and failed `bun audit`. It also carried a `check:outbound-delivery` script belonging to other work. Every `package.json` is now taken from `staging` with only the `engines.bun` line re-applied. - `executor/utils/block-data.test.ts` — a 77-line file `staging` added — was deleted outright. Restored. - `executor/handlers/generic/generic-handler.test.ts` had lost a test `staging` added. Restored, with only the one `blockId` assertion re-applied. Also formats `keyed-invocation-identity.test.ts` and sorts imports in `internal-api-base-url.test.ts`, which is what `lint:check` failed on. * fix(providers): thread the model's tool-call id into keyed tool execution `prepareToolExecution` accepted an `invocationId` but no provider supplied one, so a keyed tool invoked through an agent always hit the incomplete-context fallback and minted a fresh token — leaving Stripe, Square, Brex and Outlook Calendar writes able to double-deliver under the hosted-key retry layer even though the block path was fixed. The id is now a positional parameter rather than another optional field on `request`, so a provider that cannot supply one fails to compile instead of silently falling through. 22 of the 27 call sites already had the OpenAI-shaped `toolCall` in scope; `tsc` identified the other five, of which Anthropic (`toolUse.id`) and Bedrock (`toolUse.toolUseId`) name it differently. Gemini is left deliberately unthreaded and documented: its function-call parts carry no model-supplied identifier — the streaming loop has to synthesize a local one — and a positional index would not survive the model re-emitting the call. A token that only looks stable is worse than the loud fallback, which names the missing fields. * fix(executor): keep execution order monotonic across a resume `executionOrder` is not carried in the pause snapshot, so a resumed run restarted the counter at 0 and a loop or parallel body executing on both sides of a pause could reuse a pre-pause value. That was cosmetic while the number only ordered logs. It stops being cosmetic once identity is derived from it: a `keyed` tool takes its provider idempotency token from this value, so two distinct writes would present the same token and the provider would silently drop the second. Suppressing a real payment is worse than the duplicate the token exists to prevent, because it looks like success. The counter is now seeded from the highest `executionOrder` among the restored block logs rather than from a new snapshot field, so snapshots written before this change are repaired on resume instead of needing a migration. * fix(providers): thread the tool-call id through the streaming loops too The previous commit only reached call sites written as a single-line three-argument call. The streaming loops are formatted across lines, so openai-compat (Groq, DeepSeek and everything else routing through it), Anthropic and Bedrock still omitted the id and kept falling back to a fresh token. Both Gemini paths now pass `part.functionCall?.id` — the RAW model id, not the `ensureToolCallId` value used for stream events. That helper allocates an execution-local id when Gemini supplies none, and it is freshly allocated per attempt: passing it would complete the keyed context, silencing the "could not derive" warning, while leaving the token just as unstable. Gemini frequently omits the id, in which case this is `undefined` and the loud fallback stands. All 27 call sites are now covered. * fix(providers): make the tool-call id argument required, not optional The TSDoc claimed a provider that cannot supply an id would fail to compile, but the parameter was declared `toolCallId?: string` — so a new call site could omit it entirely, typecheck, and silently take the unstable-token path the positional parameter exists to close. The comment promised a guarantee the type did not enforce. It is now `string | undefined`: required in position, nullable in value. A provider with no model-supplied id must pass `undefined` explicitly and take the loud fallback, rather than being able to forget the argument. All 27 existing call sites already pass it, so this is enforcement only. Verified by deleting the argument at one site: `tsc` rejects it.
…and the workbook preview (#6945) * fix(tables,knowledge): recover abandoned dispatches and bound the sweep Three defects measured in production this afternoon. A dispatcher killed by an OOM left `table_run_dispatches` at `dispatching` forever. Every terminal transition on that table is user- or flow-initiated, so nothing reclaimed the row: four dispatches were stranded in one afternoon, pinning each table's "X running" overlay and blocking re-runs, with no way to clear them from the product. The `table_run_dispatches_watchdog_idx` index has existed for this sweep since the table was created, unused. Liveness comes from a new `heartbeat_at`, stamped by the per-window writes that already advance `cursor` and `processed_count`, so a slow-but-live dispatch is spared however long it runs — the in-process path has no duration ceiling, so ageing from `requested_at` would reclaim live self-hosted work. The sweep reads `COALESCE(heartbeat_at, requested_at)` so rows written before the column stay reclaimable rather than NULL-false forever, and runs as the last arm of the existing stale-execution cron at the same 95-minute window its table-job sibling uses. Rows are cancelled, not completed: the scope never finished. The OOM itself is not a leak. Peak RSS is a flat plateau — 457 MB at 20-45s and 461 MB past 200s, so ten times the duration buys four megabytes — that has crept about two percent per release for a month, from 446 MB in late July to 545 MB, past the 512 MiB `small-1x` ceiling. CPU peaks at 0.19, so the larger preset is bought for RAM alone. `maxAttempts` never covered the kill either: Trigger.dev retries `TASK_PROCESS_OOM_KILLED` only when `retry.outOfMemory.machine` names a preset, and all four runs recorded `attempt_count = 1` while the docstring claimed they resumed from the persisted cursor. The connector stuck-document sweep dispatched without a bound. Its chunk size paced the loop but the candidate query had no limit, so one connector enqueued 2,959 documents in fifteen seconds onto the queue every workspace shares. Nothing was double-billed — those documents were genuinely unindexed — but one connector monopolized the queue, and each dispatch mints a fresh requestId, so the idempotency key differs every pass and none of it deduplicates. Candidates are now taken oldest-first and capped per sync; a deeper backlog is deferred to the next sync rather than dropped. * fix(file-parsers): read officeparser's entry point across module systems `officeparser` is CommonJS — `main: officeParser.js`, no `type`, no `exports` map — so what `await import('officeparser')` yields depends on who built the code. Node and webpack synthesize named exports from `module.exports`, so `.parseOfficeAsync` is there. esbuild, which builds the Trigger.dev worker bundle, puts `module.exports` on `.default` and leaves the named export undefined, and the package is in neither `build.external` nor `additionalPackages`, so it is bundled. Reading the named export directly therefore worked everywhere except the worker, where calling it threw `TypeError: parseOfficeAsync is not a function`. All four parsers treat that as "the library failed" and answer with a scrape of the archive, which returns `degraded: true`, and the document pipeline rejects a degraded parse outright. The visible result was every `.pptx` and legacy `.doc` reporting "No text could be extracted from this file — it may be scanned, image-only, or password-protected", naming a cause that had nothing to do with the fault. 118 pptx and 14 doc failures landed in a single burst when one connector's sync first succeeded after ten consecutive crashes. Resolved in one shared loader rather than per bundler: externalizing the package has to be repeated in every build config this code runs under and regresses silently the day one is missed. The shape handling is split into a pure `resolveParseOfficeAsync` because the failing shape cannot be reproduced by mocking the specifier — Vitest's module-namespace proxy throws on a missing export rather than yielding the `undefined` a real bundle produces, so a test going through `import` can only assert the shape that already worked. That is also why the existing parser suites never caught this: each mocks `officeparser` with a fabricated named export, which presupposes the interop being broken here. * fix(knowledge): bound the workbook preview to the rows it emits `sheet_to_json` allocates from a worksheet's DECLARED `!ref` range rather than its populated cells, and Excel routinely writes an inflated range from stray formatting. The 1,000-row preview cap was applied to the result, so it bounded the emitted string while the allocation it was meant to bound had already happened. An 880 KB workbook exhausted an 8 GB worker; the same content exhausted 16 GB when this ran inside the connector sync. No machine size fixes that, because the allocation scales with a number the file declares about itself — fleet p99 for this task is 691 MB against 8 GB, so this is a cliff, not pressure. Passing the window into the conversion is what makes the cap real. `defval` goes with it: defaulting every cell in the range made each row dense, so allocation scaled with columns x declared rows rather than with populated cells, and because no row was left empty it silently defeated the `blankrows: false` beside it. Reported totals still come from the declared range, so bounding the conversion does not change what the metadata says the workbook holds. The eleven documents killed this way recorded `attempt_count = 1`: `maxAttempts` does not cover `TASK_PROCESS_OOM_KILLED`, which Trigger.dev retries only when a larger preset is named. Adding that escalation is a safety net rather than the fix, and the same gap the dispatcher had. Also corrects the machine comment, which claimed `large-1x` was 2 vCPU / 2 GB. It is 4 vCPU / 8 GB, and believing the stale figure makes a resize look like the answer when the parser is what is unbounded. * fix(tables): keep a cancelled dispatch cancelled when a step claims it `dispatcherStep` reads the dispatch, then awaits the table load before writing `dispatching`. Keying that write on the id alone resurrected a dispatch cancelled inside that window — a Stop-all, or now the stale-dispatch sweep — and the fresh heartbeat it writes would then buy the resurrected row another full window before the sweep could reclaim it again. The race predates the sweep, but the sweep is a new writer of `cancelled` that no user action drives, so it is newly reachable without anyone touching Stop. Re-asserting the status the step already read is the whole fix. * fix(tables,knowledge): spare a live window, and restore the truncation notice A lease needs its heartbeat interval to sit well under its TTL. The dispatch heartbeat is stamped between windows, not during them, and `batchTriggerAndWait` checkpoints the loop for the whole window — so the interval is really "one window", which nothing bounds: the window ends when its cells do, and the in-process path has no ceiling at all. A window outliving the stale threshold had its dispatch cancelled while it was plainly alive. Its cells carry the signal the checkpointed parent cannot — `updatedAt` on every in-flight row execution, written by the cell tasks themselves. Both signals must be stale before a dispatch is reclaimed, so a slow window is spared for as long as its cells keep reporting while a run with nothing beating and nothing executing is still collected. The subquery rides the partial `(table_id, status)` index that already covers exactly those three statuses. Bounding the workbook conversion also made its truncation notice unreachable: the converted length can no longer exceed the window it was compared against, so every sheet larger than the preview cap silently stopped reporting that it had been cut. Compared against the declared row count instead, which is what the comparison meant before the conversion was bounded. * fix(tables,knowledge): act on the claim outcome and scope liveness to the dispatch Three defects, two of them created by the previous round's fixes. Guarding the pending-to-dispatching claim without reading its outcome was the worse half of a fix. When a Stop-all or the stale sweep won the race the row correctly stayed `cancelled`, while the step went on to announce `dispatching`, stamp cells and enqueue a window for it — and an empty window would then reach the unguarded `markDispatchComplete` and overwrite `cancelled` with `complete`. The step now ends when it did not claim the row. The cell-liveness probe was table-scoped, and `table_row_executions` carries no dispatch column, so a live dispatch's cells vouched for an abandoned dispatch beside it and the abandoned row was never reclaimed — turning the stuck overlay this sweep exists to clear into a permanent one. Narrowed to the dispatch's own groups, which it already stores. Two active dispatches over the same groups can still mask each other, but that is the state `markActiveDispatchesCancelled` already prevents. Truncation asks whether the window cut the sheet short — a question about the declared range against the cap. Comparing the converted length to the cap made it unreachable once the conversion was bounded; comparing the declared count to the converted length then reported truncation for any sheet merely containing blank rows, which are now skipped rather than defaulted into existence. * fix(tables): scope dispatch liveness to its rows, not just its groups The previous round narrowed the cell-liveness probe to the dispatch's groups on the reasoning that two active dispatches over the same groups cannot coexist, because starting a run cancels prior work on its scope. That reasoning was wrong. `cancelPriorRuns` in `workflow-columns` requires `isManualRun`, so auto-fired runs never cancel anything, and the per-row path is explicitly a no-op for dispatch cancellation. Same-group coexistence is ordinary. A dispatch that names rows now only accepts liveness from those rows, which covers the auto-fired and row-scoped runs that reach this state. What remains is two table-wide dispatches over the same groups, where nothing in the row execution says whose work it is; closing that needs a `dispatch_id` column on `table_row_executions` threaded through six write sites, including the shared cell-write path every cell task uses. That residue is a delay rather than a permanent mask — the live dispatch's cells stop updating when it finishes, and the next sweep after a quiet window reclaims the abandoned row. * refactor(tables): name the dispatch liveness predicate and bound its fan-out Extracts the cell-activity check into `hasRecentCellActivity`, so the stale predicate reads as its two conditions — nothing beating, nothing executing — rather than a twenty-line SQL blob nested inside an `and()`. No behaviour change; this is the code three review rounds found defects in, and being able to read it is what makes those defects findable. Bounds the terminal-event fan-out with `mapWithConcurrency`, matching how the scheduler already fans out. The sibling cancel paths emit over one table's dispatches; this sweep can carry a whole tick's worth across many tables, and each event is its own write. Also repairs the test that covers it. `collectChunks` walks into the `tableRowExecutions` table object the fragment interpolates, so every column name appears in the chunks whether the predicate references it or not — the group, row, table and timestamp assertions all passed with their predicates deleted. Matching the literal SQL instead makes them fail, which mutating each clause now confirms. * fix(tables): make the row bypass NULL-safe and guard the post-wait completion `jsonb_typeof(scope -> 'rowIds') <> 'array'` was the table-wide bypass, but a table-wide dispatch has no `rowIds`: the extraction is SQL NULL, `jsonb_typeof` returns NULL, and `NULL <> 'array'` is UNKNOWN rather than TRUE. The bypass never fired, so no live cell could satisfy the probe and the sweep reclaimed exactly the long-running table-wide dispatches the row filter was added to protect — inverting it. `IS DISTINCT FROM` is the NULL-safe form, and the same pitfall is already handled with `coalesce` in `markActiveDispatchesCancelled`. `completeDispatch` also wrote through the unguarded `markDispatchComplete`. Both its callers run AFTER the window's wait, so a Stop-all or the sweep landing during that wait leaves the row `cancelled` and the write overwrote it with `complete`, publishing a completion event after the cancellation one. The claim guard cannot cover this — the cancel arrives long after the claim. It now goes through `completeDispatchIfActive`, which already exists for exactly this, and emits nothing when the transition does not land. * fix(knowledge): give connector sync logs a retention pass Nothing pruned `knowledge_connector_sync_log`, so it grew by one row per sync run forever — a connector on a fifteen-minute interval writes about 35,000 rows a year by itself. That cost lands on `loadPreviousListingObservation`, which reads the newest `completed` row per connector through an index covering `connector_id` alone, so every retained row makes the sort behind the deletion-safety corroboration slower. Added as another arm of the cleanup cron, batched the same way as its two sibling prunes. Two `exists` guards are load-bearing rather than defensive: the newest row per connector always survives, and so does the newest `completed` one, because that is the row `loadPreviousListingObservation` reconstructs the previous listing from — and that reconstruction decides whether a suspect listing is corroborated, i.e. whether reconciliation may delete documents. Pruning it would silently change deletion behaviour. `started` rows are never eligible; they are in flight or waiting on the scheduler's own sweep. * fix(tables): funnel every post-claim completion through the guarded write The empty-window exit still wrote through the unguarded `markDispatchComplete`, and it runs after the claim like the other two — so a cancel landing during its window query was overwritten with `complete`. Shorter window than the two post-wait exits, same defect, and leaving one of three unguarded is how this came back twice already. All three now route through `completeDispatch`, so the guard lives in one place and covering it once covers every exit. The redundant test for this path went with it: it could not be made to fail against the mock, and a test that cannot fail is worse than none — the guard is held by the test on the shared funnel. * fix(tables): bound how long cell activity may spare a dispatch The liveness probe cannot tell whose cells it is looking at when two table-wide dispatches share a group, because `table_row_executions` carries no dispatch column. On a quiet table that is only a delay — the neighbour finishes and the next sweep reclaims — but a busy table with continuous auto-fired work can keep an abandoned dispatch masked indefinitely, which is the stuck overlay this sweep exists to clear. A ceiling bounds it: past a day without a heartbeat, a dispatch is reclaimed whatever its cells are doing. That is safe because a live dispatch stamps its heartbeat between windows regardless of cell activity, so only a single window outliving the ceiling could be reclaimed wrongly, and no window lasts a day on any path — the Trigger.dev run ceiling is ninety minutes. The real fix is a `dispatch_id` on the executions row. Threading it through the patch layer and the upserts underneath it is a change to the hottest write path in tables and belongs in its own review, not on the sixth round of this one. * refactor(tables): give the stale predicate one definition of "last beat" `COALESCE(heartbeat_at, requested_at)` was written twice — once for the stale threshold and again for the absolute ceiling — so the two could drift into disagreeing about what proof of life means. One `lastBeat` fragment, one `notBeatingSince(cutoff)` helper, both cutoffs expressed through it. Also corrects the ceiling's comment: it triggers a day past the stale threshold, not a day past now. * fix(tables): delete the unguarded completion rather than guard it a fourth time The two pre-claim exits — table missing, no target groups — still wrote through `markDispatchComplete`. Last round I argued they run before the claim, "where forcing a terminal state is the intent". That was wrong twice over: the table lookup is awaited, so a cancel lands in that window like any other, and a dispatch cancelled mid-lookup has not completed its scope any more than one cancelled mid-window has. Routing them through `completeDispatchIfActive` left `markDispatchComplete` with no callers, so it is gone. That is the part worth having: this is the fourth place the same defect appeared, each time because an unguarded writer was sitting there to be reached. With it deleted, `completeDispatchIfActive` is the only way to complete a dispatch and the class cannot recur. * fix(tables): re-read the dispatch before committing a window Several round trips separate the claim from the enqueue — the window query, the executions prefetch, the tombstone filter — and nothing rechecked the dispatch across them. A Stop-all or the stale sweep landing in that gap had the step stamp cells and run a whole window for a dispatch already recorded as cancelled; the existing recheck sits after the window, which is too late to prevent it. Mirrors that existing check on the other side of the enqueue. It narrows the gap to a single statement rather than closing it — a cancel arriving after this read still races the enqueue, and no check can fix that. The cell-level `cancellationGuard` and the `isExecCancelledAfter` tombstone filter are what catch the remainder.
* fix(workflows): deduplicate generated workflow names * fix(workflows): retry deduplicated name races (#6935) - recompute generated names after workflow-name conflicts - preserve exact-name and unrelated constraint behavior - cover the concurrent-create retry path
* feat(workflows): generate short machine-nature names * fix(workflows): remove vehicle name terms * feat(workflows): expand generated name vocabulary
Replace the hand-rolled resource tab bar with the shared TabStrip primitive, and generalize the strip where this caller needed more than it offered.
…con (#6951) The Logs family flooded the `@` picker with up to 50 near-identical rows, named after their workflow and drawn with the workflow icon, so they read as workflow snapshots and buried every other family. - Preview the 5 most recent runs while the query is empty; typing still searches the full fetched set. The cap spans the workspace rather than one workflow, so a few background runs cannot evict a run just started - Draw the row with the Logs icon, matching the sidebar, the search palette, and the chip the selection turns into - Trail the row with relative time, and with the dot `Badge` draws at `sm` for a run that did not simply succeed, so runs of one workflow are told apart at a glance - Make `@logs` reach the family, which nothing in a row's text names Mentioning a log also resolved to nothing: a log row is keyed by `id` but its run is addressed by `execution_id`, and the picker sent the former where the server resolves the latter. The run id now rides on the resource and every menu builds that resource through one helper, rather than eight inline literals that each silently dropped it.
#6947) * feat(secrets): show where a secret is referenced, beside its usage log "See usage" answered who has run something with a key. It could not answer the question a rotation actually starts from — where is this wired in — because a secret four blocks depend on but nothing has executed yet has no usage rows at all, so the panel read "This secret has not been used yet" for a live key. The usage view now carries two tabs. Logs is the existing trail, unchanged and still the default, since that is what the header action has always opened. References is new: the blocks that name the secret as {{KEY}}, grouped under their workflow, then the custom tools and MCP servers whose own bodies carry it. Detection is the workspace-fork remapper's. remapSubBlocks already walks nested tool-input params, resolves canonical basic/advanced pairs, and skips dormant and condition-hidden members, so calling it per block inherits every rule a fork already obeys. Only the aggregation is new: scanWorkflowReferences collapses its output to unique (kind, sourceId) pairs and discards the workflow — right for building a mapping table, wrong for locating a key. Nothing under ee/workspace-forking changed. - Candidates come from strpos(sub_blocks::text, name) > 0, deliberately not LIKE: `_` is a LIKE single-character wildcard and nearly every env key contains one, so SB_ACTION_ROUTER_SECRET would match text it does not occur in. The prefilter can over-match but never under-match; the scanner decides. The plan is an index scan on workflow by workspace, nested-looped into workflow_blocks, so cost tracks the workspace rather than the table. - Scope gates the read but does not narrow it. A {{KEY}} names a key, not a scope, so the same sites answer for a workspace secret and the personal one it shadows; narrowing here would report a personal secret as unreferenced the moment a workspace variable of the same name existed. - References reports one field per block, not a list. The remapper dedupes a block's references by (kind, sourceId), so a block naming the secret twice yields one entry — the type says so and a test pins it, because the row renders that field as its whole description. - Reads live state, not deployed: a draft workflow referencing the key must show. Blocks are capped and the cap is reported as `truncated` rather than silently trimming the list. - Authorization is the existing usage gate, renamed requireSecretTrailReadAccess and shared verbatim, so the two tabs can never disagree about who may look. UI is existing primitives only — ChipModalTabs for the strip, DetailSection per workflow over RESOURCE_LIST_STACK rows, IntegrationTile for the block glyph so a block reads here as it does on an integrations row, SettingsEmptyState for the gates. No new component, no new class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): close the reference-scan scope bypass and bound its output Review round 1. - use-cases.ts: `scope` was a caller-controlled assertion the reference scan never narrowed by, so `scope=personal` returned from the shared gate before any check and handed any workspace member the admin-gated reference map for any workspace secret. The usage trail can trust that scope because it filters the read by `secretOwnerUserId`; a name-based workspace-wide scan cannot. References now authorize on what the NAME resolves to — a workspace secret under that name is admin-gated outright, and absent one the caller must actually hold a personal secret of that name, which also stops a member enumerating arbitrary names. `scope` is dropped from the input, the contract, the hook and the query key rather than merely ignored: a parameter that does not exist cannot be asserted. The trail gate keeps its old name and a note saying why only a scope-narrowed read may reuse it. - scan.ts: the prefilter matched the bare name, so `API_KEY` also read every block holding `{{API_KEY_TEST}}` or the words "the API_KEY value" — and those false positives counted against the row cap, so on a workspace with enough of them genuine references sorted later were never read at all. It now matches the reference syntax (`{{name}}`, with the whitespace ENV_REF_PATTERN allows), so a candidate is a real occurrence and the cap means what it says. A name outside the env-key charset short-circuits, which is also what makes it safe to inline into the regex unescaped. Verified against a real workspace: the exact key still returns its 16 blocks, its prefix now returns 0 where it previously matched all 16, and a metacharacter name touches no query. - scan.ts: capping tool and server ROWS did not bound the output — one MCP server emits an entry per matching header plus one for its url, so 200 rows could expand past the contract's 400-entry bound and make the route reject its own response, turning a successful scan into a 500 and the tab into "Could not load references." Emission now stops at the bound and reports `truncated`. - secret-references-panel.tsx: the empty-state early return preceded the truncation banner, so a capped scan that filtered everything out claimed the secret was unreferenced. Both paths now share one note, and silence from a capped scan reads as absence of evidence rather than evidence of absence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): cover unicode whitespace, legacy keys, and the shadowed-personal tab Review round 2 — all three follow from round 1's own fixes. - scan.ts: the syntax prefilter anchored on `[[:space:]]`, but the two engines disagree about what whitespace is. `ENV_REF_PATTERN`'s `\s` accepts U+00A0, U+202F and U+3000; Postgres `[[:space:]]` matches only the ASCII set. So a value pasted with a non-breaking space inside the braces is a reference the executor resolves and the prefilter silently dropped — the one failure direction this feature must never take, since the answer it gives is "unused, safe to delete". Anchoring on `[^[:alnum:]_]` instead accepts every whitespace encoding while still rejecting a longer key on either side, and needs no code-point list that could drift. It can admit a non-reference like `{{-NAME-}}`; that costs one candidate row, and the scanner re-checks every candidate regardless. Erring loose here is deliberate. (Greptile's `{{\tAPI_KEY\t}}` example was already handled — tab is ASCII — but the unicode half of the finding was real.) - use-cases.ts: the gate read `keyAccess.knownKeys` as "a workspace secret exists under this name", but that set only covers names with an `env_workspace` credential row. A legacy value written before the ACL existed has no row and still wins at run time, so it fell through to the personal branch and handed a non-admin the reference map for exactly the oldest keys. It now reads the authoritative `workspace_environment.variables` map through a new `hasWorkspaceEnvValue`, which is documented against `knownKeys` so the two are not confused again. `getWorkspaceEnvKeyAdminAccess` keeps its existing contract — its `knownKeys` still answers the ACL question its other callers ask. - secret-references-panel.tsx: a personal secret shadowed by a same-named workspace variable could open the view (its owner may read their own Logs) but References always hit the workspace refusal and rendered a generic load error — a tab offered in a state where it cannot succeed. The refusal is correct; the tab now states the shadowing instead of asking for a map it will be denied, reusing the wording the detail page already shows. No request is made in that state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): re-check the reference gate's volatile input after the scan Review round 3. The name-resolution gate reads whether a workspace value exists, then scans. A workspace secret created between the two makes the map now in hand admin-gated, so a personal owner could receive it without workspace-secret administration. The window is small and the data is derivable — a workspace member can already open every workflow and read its `{{KEY}}` references — but the gate's stated contract is that references follow the same predicate as revealing the value, and a point-in-time check that can be overtaken does not honour that. An advisory lock or a snapshot transaction would serialize a read-only view against secret writes for it, which is the wrong trade. Instead the one volatile input is re-read after the scan and the request fails closed if it flipped. `requireSecretReferencesReadAccess` now reports which branch authorized: an `admin` grant holds however the name resolves and pays nothing, while a `personal` grant — the only one resting on absence — is re-checked. A non-admin loses nothing they were entitled to keep; the request is refused the way it would have been a moment later. Adds a `listSecretReferencesUseCase` suite covering both denial paths, the legacy value, the personal owner, the admin short-circuit, and the race itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): accept JSON-escaped whitespace in the reference prefilter Review round 4. The prefilter reads a `::text` rendering of a JSON column, and `jsonb::text` renders a real tab inside a string value as the literal pair `\` `t`. `t` is alphanumeric, so `[^[:alnum:]_]` could not consume it and the row was discarded before `ENV_REF_PATTERN` ever ran — the References tab omitting a live reference and reporting `truncated: false` while doing it. Round 3's fix was verified against a raw text value rather than the JSON rendering, which is exactly why it looked correct: `E'{{\tAPI_KEY\t}}'` matches, `jsonb_build_object('v', E'{{\tAPI_KEY\t}}')::text` does not. The gap between `{{` and the name now accepts three encodings at once — raw characters (covering every Unicode space, which Postgres `[[:space:]]` misses), JSON two-character escapes, and `\uXXXX` (how a vertical tab survives the same rendering). Verified against the real jsonb rendering: tab, newline, carriage return, vertical tab and form feed all recover, U+00A0 / U+3000 / space / plain keep matching, and `{{API_KEY_TEST}}`, `{{MY_API_KEY}}` and prose are still rejected — so the row cap keeps meaning what it says. Plain-text columns (`custom_tools.code`, `mcp_servers.url`) carry no JSON escaping, but tool code is JavaScript source and can contain the same escape sequences literally, so the one predicate is right for every column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(secrets): land the References link on the block, and name its field Feedback round. - Logs leads the tab strip. It was already the default tab; the order now says so. - The usage view drops its resource heading for a plain "Usage" title. The back chip already names the secret, so the tile and the subtitle underneath were saying it a second time. `CredentialDetailLayout` gains an optional `title` that renders the same element, class and column position the settings shell gives `SettingsPanel` — which is how the sibling Forks "Activity" view titles itself. Existing callers pass nothing and are unchanged. - A block row now lands on the block instead of the workflow's default framing. The editor had no URL params at all, so `?block=` is its first: read once on arrival, acted on, and stripped. It is a navigation signal rather than canvas state — the carve-out in sim-url-state.md is about pan, zoom, selection and drag, which are socket-synced or high-frequency; this is neither, and it rides in the link so a middle-click or reload keeps it where an in-memory handoff could not. The consuming effect mirrors the note-search reveal in the same file, including the three details that make that one work: read from `displayNodes` so a target arriving before its node mounts is retried on the mounting commit, route selection through `resolveSelectionConflicts`, and latch in a ref. It also claims `userFocusedWorkflowIdRef` the way a node click does, because `onInit` re-reads that inside its own rAF and would otherwise `fitView` over the camera — and that ref is reset by exactly the `workflowIdParam` change a deep link causes. The panel opens for free: `syncPanelWithSelection` already follows selection. `useSearchParams` needs a Suspense boundary and the editor's ancestry has none, so the read lives in a leaf under its own `fallback={null}` rather than wrapping the editor and adding a `loading.tsx` to its mount path. `next build` passes. - A tool-input reference showed `tools-tool-0-code`. Those `{subBlockId}-tool-{index}-{paramId}` keys are documented as an ephemeral, client-only projection of the canonical `tool-input` value and are not meant to be persisted, but older rows carry them — so the scanner reported whichever the record yielded last. They are dropped before scanning, which is right even where the two disagree: `tool.params` is what executes, so a mirror the canonical no longer matches describes a reference that no longer runs. - The row now shows the field's label from the block config rather than its storage id — "API Key", "Tools", "Code", "Bot Token" — falling back to the id when the block or field is unregistered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): make the reference prefilter exactly as tight as the authority Review round 6. The gap between `{{` and the name accepted any non-word character, so `{{-API_KEY-}}` and `{{"API_KEY"}}` matched in SQL while `ENV_REF_PATTERN` rejects them. The previous commit called that free — "costs a candidate row and nothing else" — which was wrong: a candidate row is a slot under BLOCK_SCAN_LIMIT, so enough near-misses sorted earlier exhaust the cap before a genuine reference is read, and the tab reports a live key as unused. That is the same failure the tightening in round 1 was meant to remove, reintroduced by the round 4 loosening that fixed JSON-escaped whitespace. The gap now enumerates exactly the whitespace `\s` accepts, in each encoding it can arrive in: `[[:space:]]` for raw ASCII, `\\[tnrf]` and `\\u000[bB]` for the JSON escapes, and an explicit class for the Unicode spaces Postgres emits verbatim but `[[:space:]]` does not match. That class is generated from a code-point table rather than written literally. Writing it by hand put a run of invisible characters in the source — a reviewer cannot check them, and a formatter or editor can silently mangle them. The table is the readable form and `toPgEscape` renders it. Verified against the real jsonb rendering, 17 cases: raw space, tab, newline, carriage return, vertical tab, form feed, U+00A0, U+202F, U+3000 and an embedded reference all match; `{{-API_KEY-}}`, `{{"API_KEY"}}`, `{{API_KEY_TEST}}`, `{{MY_API_KEY}}`, prose and an across-braces span all do not. Every candidate the SQL admits is now a real occurrence, so the cap counts references and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): cap the reference scan on results, not candidates Review round 7. The prefilter now matches reference syntax exactly, but `remapSubBlocks` filters further on semantics SQL cannot see: it drops dormant canonical members and condition-hidden fields. So a block whose only `{{KEY}}` sits in a hidden field is a genuine candidate that yields nothing, and with the cap counting candidates, enough of those sorted earlier displaced active references out of the answer. Unlike the previous two rounds this is not fixable by tightening the prefilter — no SQL predicate can evaluate canonical modes or field conditions. So the cap moves to what it should have counted all along: blocks REPORTED. Candidates are read a page at a time up to a ceiling far above the result limit, so filtered rows are absorbed as extra reads instead of taking result slots. Paging rather than one large read because the alternative is holding every candidate block's `sub_blocks` in memory at once; peak memory is now one page. `blockId` joins the ordering as a final tiebreak, since OFFSET paging over a non-unique sort can repeat or skip rows across pages — which here would double-report a block or silently lose one. This does not make the scan unconditionally complete, and the ceiling says so: bounded work and guaranteed completeness cannot both hold, so the only real choice is where the bound sits and whether it counts something the reader can see. It now counts results. Query plan re-checked with the OFFSET in place: still an index scan on workflow by workspace, nested-looped into workflow_blocks. Test added that pins the fix — 2,500 prose candidates sorted ahead of one real reference, which the previous cap dropped entirely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): drop the paging that caused drift, and stop false truncation Review round 8. - Paging removed. It bought headroom and paid with drift: `OFFSET` is positional, so a block renamed, inserted or deleted between page queries shifts the result set, and the scan skips a live reference or reports one twice. That is a worse failure than the one paging was added to fix, and it was self-inflicted last round. Candidates are read in one statement again — one statement is one snapshot, so neither skew nor duplication is possible — with the ceiling lowered to 4,000 so a single read stays a sane amount of memory. Result-capping survives, which was the actual point: filtered rows are still absorbed as extra reads rather than taking result slots. - `truncated` no longer fires on an exact landing. The block path now uses the limit-plus-one read and strict `>` the resource paths already used, so a scan that ends precisely on a bound reports complete instead of warning about references that were all returned. - The deep-link target is released when its block does not exist. It was cleared only on a match, so a link to a since-deleted block left the id set with the param already stripped: the effect re-checked on every canvas update forever and shadowed a later link to the same block. Once any node has mounted the canvas is populated, so an id still absent is gone and the target is dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): gate the deep-link release on the workflow being ready Review round 9. Round 8 released a deep-link target once `displayNodes` was non-empty, reading that as "the canvas is populated, so a missing id is deleted". It is not: arriving from another workflow the store still holds that graph, so nodes are present while the linked workflow is still hydrating — and a valid `?block=` target was dropped before its own blocks ever mounted. The file already had the right predicate. `isWorkflowReady` pins `hydration.phase === 'ready'`, `hydration.workflowId === workflowIdParam` and `activeWorkflowId === workflowIdParam`, which is exactly "the graph now loaded is this workflow's". Absence is only conclusive under that, and a node count never was — it says something mounted, not whose. This is the second fix to this release condition in two rounds, both from guessing at a readiness signal instead of using the one the component already computes for the same question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s reach consumer traces (#6950) * feat(custom-blocks): let a publisher decide whether their block's runs reach consumer traces Joining a custom block's child run into its caller's trace shipped on by default, gated at read time by whether the person reading could already open the source workspace. That gate is doing the wrong job: a custom block's whole point is that consumers need no access to the source, so the check refuses exactly the readers the feature exists for, and it makes the answer depend on who is looking rather than on what the block's owner agreed to publish. The decision moves to the party whose data it is. `custom_block.trace_child_runs` is set by the publisher in Settings, applies org-wide, and is the entire policy — nothing downstream re-checks a caller. `getCustomBlockAuthority` already resolves per invocation and is the one lookup both the canvas handler and the Agent-tool runner pass through, so one column covers both surfaces and no consumer input can assert it. It defaults to FALSE. With the viewer check gone, an opted-in block publishes the source workflow's block names, inputs, outputs, and prompts to anyone who can read a consuming workflow's log. That is the same boundary curated outputs and redacted errors hold, so it opens by an affirmative act of the publisher or not at all — never as the residue of a column default on rows nobody revisited. Closed means the handle is withheld outright rather than persisted behind a flag: with no `childExecutionId` there is nothing for a reader, a migration, or a later refactor to join. What replaces it is a `_childTraceDisabled` marker, because a boundary span with no children renders exactly like a leaf block and an untraced run would otherwise read as one that did nothing. The consumer-facing failure `ref` is untouched either way — it is the only thing that makes an untraced failure reportable. Custom blocks invoked as Agent tools now join too. The child's handle already reached the agent's persisted `toolCalls[].result` (`postProcessToolOutput` strips only `__`-prefixed keys); nothing lifted it onto the tool span. Both span builders lift and strip it, and `hydrateChildTraces` needs no change — its boundary walk already recurses. The same handle is stripped from the model-facing copy of the tool result in `executeProviderTool`, the single point where the raw and model copies diverge: an opaque execution id in a tool result reads to a model like data the tool returned. The live SSE stream keeps one condition beyond the policy: an identified consumer. Not an authorization check — no workspace query — but chat deployments and the public API leave `liveTraceViewerUserId` unset because their consumer may be anonymous, and opting into org-wide tracing is not consent to stream a publisher's raw agent tokens to the internet. Copilot deliberately cannot set the field; exposing a team's internals org-wide is a human decision, not one an agent makes while publishing on their behalf. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(custom-blocks): read the publisher's trace policy at read time, not from the handle's presence Treating a persisted `childExecutionId` as proof of publisher consent is only true for handles this PR's writer produced. Every handle written before it meant something else — "a child ran; authorize the reader" — and the rows carrying them outlive the migration, so removing the reader check turned them into an open door: a consumer could open an old parent log and receive the source workflow's block names, inputs, outputs, and prompts from a block whose publisher never opted in. `hydrateChildTraces` now resolves the policy live, per boundary, from `custom_block.trace_child_runs`. The child log row's `workflowId` is the key — publish enforces one block per workflow — which also covers an Agent-tool boundary, whose span carries no block type to look up. A workflow with no block row (never published, or since deleted) has no publisher left to consent and stays shut, as does a failed policy read. This is not redundant with the write-time withholding. The handler still emits no handle for a block that was closed when the run executed, so such a run stays closed forever even if the block is opened later; this check decides whether the runs that DO carry a handle may still be shown. Turning the policy off therefore also closes what is already recorded, which is what a governance switch has to do to mean anything. Reported by Greptile on #6950. Also drops `any` from the trace-policy tests: outputs read through `Record<string, unknown>` (the handler's declared return does not name these internal keys) and failures narrow through `ChildWorkflowError.isChildWorkflowError`, which pins the failure type as well as its fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(logs): sum the child-trace drop counters from the struct, not a hand-listed set `totalDropped` re-listed four of the five counters, so a read whose only drops were policy refusals computed zero and skipped the log entirely. That is the commonest drop there is now — every handle written before the publisher policy existed refuses at that gate — so the one signal telling an operator the live check is closing joins went silent exactly when it started mattering. Summed from the struct instead. A hand-maintained list beside a struct is stale the moment a field is added, which is precisely how `policyClosed` was left out. Reported by Cursor Bugbot on #6950. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(db): renumber the custom-block trace migration around a 0299 collision Staging landed its own 0299 (`table_run_dispatches.heartbeat_at`) while this branch was open. The two migrations are independent — different tables, no shared statement — so only the number and drizzle's snapshot chain collided. Regenerated rather than hand-merged: a drizzle snapshot is a full-schema dump whose `prevId` links it to its parent, so editing one by hand to sit after a migration it was not generated against is how the chain silently stops matching the database. Staging's 0299 and its snapshot are taken verbatim; this is 0300, generated against them, and its SQL is byte-identical to what it replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) * improvement(tabs): give the resource tabs a quieter floating look Adds a `floating` variant to the shared TabStrip and uses it for the mothership resource tabs. Only the active tab carries a shape; the rest are bare labels divided by a hairline, sized to their content up to a cap. The browser and terminal strips keep the attached look, which stays the default. * improvement(tabs): align the floating tab ramp and icon size with the platform Each surface token now does the job it is named for: a bare tab hovers to --surface-hover instead of the Button variant's --surface-4, and a selected tab takes the rung between hover and --surface-active. Tab icons match the action icons beside them at 16px.
* fix(selectors): respect trigger credentials in trigger mode * fix(selectors): isolate trigger selector context * fix(selectors): isolate action credential fallback * fix(selectors): scope fork reconfigs to trigger mode --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
…#6954) * feat(tables): return only selected columns from the Table block query * fix(tables): size row batches by stored bytes and surface stale picks on empty lists * fix(tables): bound projected batches by the widest stored row seen
… fades (#6967) Porting the tabs onto the shared strip had taken the header from 43px to 34px, which moved the overlaid collapse toggle from 6.5px below the panel's top edge to 2px while its right inset stayed at 16px — the corner read lopsided. The header goes to 40px: still shorter than it was, with the toggle back to 5px. The toggle also gets its 8px radius back, dropped in that port for no reason anyone asked for. Selecting a partly-hidden tab scrolled it flush against the container edge, which is exactly where the fade gradient sits, so it arrived half-faded and still looked cut off. Reveal now insets by the fade width and clamps at the scroll extremes, where no gradient is drawn. Floating tabs cap at 160px so no single tab dominates the row.
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local>
… chip (#6971) The scroll-edge fade was a gradient div tinted with the surface colour, laid over the tabs. Tabs paint their own fills, so at an edge that washed a pill toward the surface colour instead of dissolving it, and it was only correct while whatever sat behind the strip was exactly that colour. A mask fades pill and label together to real transparency — the way the command palette fades its results, and the way every other horizontal fade in the app is drawn. The ramp goes to 24px, close to the palette's, since a short one reads as a cut. The tab chip drops to 26px, leaving 7px above and below instead of 5px. The collapse toggle and the action buttons stay 30px: a tab paints a fill so its box is visible and wants air, where those are bare glyphs whose box only shows on hover. The close button now centres itself rather than encoding (band - 24) / 2, which would have put it off-centre the moment the band changed.
…st (#6972) Menu rows composed their own gap (8px) instead of the platform's chipContentGap (6px), so a menu row and the chip it opens over spaced their icon/label pairs differently. Surface padding was 6px against an 8px row radius and a 12px surface radius, so a first or last row's rounding cut across the corner it sat in; 4px makes them concentric. Separators carried a 13px gutter against a 0px gap between rows in the same group, which read as the groups floating apart. DropdownMenuLabel padded to an uncontrolled, font-dependent height and sat two type steps below its rows in a colour heavier than them; it now composes the shared row height at one step down in --text-muted. The @ mention list showed every integration. The per-family preview cap defaulted to uncapped, and integrations are 300+ near-identical rows sorted first, so the unfiltered list was the whole catalogue and no other family was reachable without scrolling past all of it. Capping is now the default, families are labelled, the menu is capped shorter than the action-menu default since it floats over the chat input, and the integrations shown are curated popular ones rather than whatever sorts first alphabetically. Both hand-rolled menus that render plain buttons for Radix focus reasons now compose the exported dropdownMenuRowClass instead of re-deriving row chrome.
…ip 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.
* 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.
#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.
…ce 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.
…tion (#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
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local>
…face (#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.
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local>
…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.
* fix(chat): copy workspace resources as portable links * refactor(chat): simplify portable resource copying
…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.
) * 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>
* 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
…7003) Co-authored-by: Sim Pi Agent <pi@sim.ai>
* 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>
* 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
Contributor
|
Too many files changed for review (725 files, 100 file limit). |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Auth and secrets. Public OTP/SSO/password/contact paths no longer treat a missing client IP as a shared bucket; they fail closed or keep independent resource/email backstops. Chat and file OTP always return a generic “code sent” and send mail after the response, so allow-list and rate-limit outcomes are not enumerable. Copilot code mounts secrets on use grants (not raw-view), inbox unattributed senders get no secret actor, and credential admins can list where a key is wired. Share passwords require 15+ characters. CSV exports neutralize formula-leading cells. Integrations and jobs. Bitbucket gains managed webhooks (push, PRs, comments, builds, fork/update) and needs webhook OAuth scopes. Knowledge connectors expose Workspace. Resource tabs, menus (separators by what is acted on), settings nav, file sharing, and client bundles get latency and layout work. PostHog queues events until init and drops unactionable exceptions. Agent skills and list-ordering rules are tightened accordingly. Reviewed by Cursor Bugbot for commit d831c09. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
…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>
…el-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
…ipt (#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
…ity (#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>
* fix(security): isolate rejected OTP attempts * fix(security): make OTP requests non-enumerating * fix(security): defer OTP delivery work
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d831c09. Configure here.
|
|
||
| logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`) | ||
| return createSuccessResponse({ message: 'Verification code sent' }) | ||
| return otpRequestAccepted() |
There was a problem hiding this comment.
Verify OTP still leaks allow-list
High Severity
POST now always returns a generic acceptance for allow-listed and rejected emails, but PUT still distinguishes No verification code found from Invalid verification code. After a short wait for afterResponse, a dummy verify reveals whether deliverOtp stored a code — and thus whether the address was allowed (or rate-limited). That undoes the anti-enumeration hardening this change aims for on both chat and public-file OTP.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d831c09. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.