v0.7.52: span sanitization, highlight tables and files text to chat, settings UX improvements, security hardening - #6183
Merged
Merged
Conversation
* fix(files): serve rendered documents to attachments and workflow reads Generated documents store their generation source under a .pdf/.docx name and keep the compiled binary in a separate content-addressed artifact store, so any consumer doing a raw read handed out source text under a document name. - Route attachments and readUserFileContent through the servable resolver so they get the compiled artifact, and stop the internal generation-source MIME marker reaching providers as a content type. - Render on read when the artifact is missing. The artifact key is (workspace, source hash), so forking a workspace, moving a file, or editing the source outside a recompiling writer orphaned it permanently and reported "still being generated" forever. Rendering self-heals those and stores the result. - Fall back to serving stored bytes as application/octet-stream when a render fails, instead of failing forever, and remember not to retry those bytes. - Only compile without a workspace context when the file's type positively says it is generation source, so unrelated stored bytes are never executed. - Make the doc-not-ready error opt-in per caller and give it a 409 via HttpError, so output decoration degrades instead of failing completed work. * fix(files): keep render failures retryable and never relabel unrendered bytes Addresses the first review round. - Only memoize a render failure when it is deterministic. A DocCompileUserError means the source will never render, so remembering it is safe; sandbox outages, timeouts, and cancellations are transient and were stranding valid documents for the life of the process. Infra failures now propagate, which also restores DocCompileUserError reaching callers that map it to 409. - Refuse to hand back bytes the resolver could not render. readUserFileContent returns a string, so the resolver's honest application/octet-stream could not travel with it and attachment builders re-inferred a document MIME from the filename — shipping generation source to a provider as a PDF. The file-serve route keeps the graceful passthrough, where a human downloading the bytes is useful. - Normalize the declared type once so a padded or upper-cased source marker cannot pass the resolver gate on one code path and fail it on the other. - Import the doc-not-ready guard lazily. The static import pulled the doc-compile module graph (remote sandbox, task runner, execution limits) into every hydration consumer and broke an unrelated test's module mock in CI. * perf(files): coalesce concurrent renders of the same missing artifact An artifact miss is identical for every concurrent reader — a freshly forked workspace whose document several viewers open at once, or one request whose blocks read the same file — and each was paying for its own compile of the same bytes. Share one in-flight render per (workspace, source, ext) key and drop the entry as soon as it settles, so a later read still re-renders normally. * fix(files): refuse unrendered bytes at the download boundary Addresses the second review round. - Throw UnrenderableDocumentError from downloadServableFileFromStorage instead of returning bytes with an `unrendered` flag. Around 45 call sites (email attachments, cloud uploads, zip entries, provider attachments) receive only a Buffer and re-infer the type from the filename, so a flag they must remember to check is a flag they will not check. Those callers already handled the previous not-ready throw, so failing is the shape they expect. The file-serve route is unaffected — it resolves bytes directly and keeps the graceful passthrough, where a human downloading the file has a use for it. - Surface that failure through hydration: with throwOnDocNotReady set, the caller cannot use a file with no content, so an unrenderable document now reaches it verbatim instead of degrading to null and reporting a misleading "may exceed size limit or no longer accessible". - Stop a shared render inheriting one caller's cancellation. The coalesced run no longer carries any caller's signal; each caller races its own instead, so an aborting reader gives up promptly while the render finishes for the others and still lands in the cache. * fix(files): move the unrenderable error out of the 'use server' module file-utils.server.ts carries 'use server', whose exports must all be async functions, so exporting an error class from it failed the production build with 67 cascading errors. The class now lives in the plain file-utils.ts beside the other shared file helpers, which also lets the hydration path import it directly instead of through a dynamic import. Also bounds how long a failed render is remembered. The isolated-vm engine cannot tell a bad source from a sandbox outage, so a permanent entry let one transient failure block re-rendering that source for the life of the process. Entries now expire after five minutes: long enough to stop a read loop spending a sandbox run per read, short enough that an outage self-heals without a deploy. * fix(files): finish the render cancellation and failure-surfacing edges - Race the E2B render against the caller's signal too. Only the isolated-vm branch did, so an aborted request on the E2B path waited for the sandbox to finish and could return a success the caller no longer wanted. - Attach a terminal handler to the shared render. Every caller races it against its own signal, so all of them can walk away; a later rejection with no waiters left would otherwise surface as an unhandled rejection. - Stop narrowing what throwOnDocNotReady rethrows. readUserFileContent now runs document compiles and can fail in ways this module has no business enumerating; narrowing produced three consecutive review rounds of "this particular failure is still swallowed". The flag means "do not degrade". - Do not mark an unrendered response immutable. The serve route caches versioned responses for a year, which would pin a one-off render failure to that URL long after a later compile succeeds on the same version. * revert(files): drop the concurrent-render coalescing The coalescing was an optional efficiency win — rendering is content-addressed and idempotent, so duplicate concurrent renders produced the same artifact and cost only extra sandbox time on an artifact miss. It bought that at the price of the most intricate code in the change set, and produced three concurrency findings across two review rounds: a shared render inheriting one caller's cancellation, an E2B/isolated-vm asymmetry in how the signal was raced, and orphaned rejections once every caller could race away. Removing it also restores true cancellation on the isolated-vm path: the caller's signal now reaches runSandboxTask again, so an abort cancels the sandbox work rather than only abandoning the wait for it. * fix(review): simplify generated document attachments * fix(files): mock servable downloads in hydration tests * fix(files): preserve rendered attachment semantics * fix(files): preserve cached artifact size * fix(files): refuse unresolved xlsx source * fix(files): resolve execution artifact workspace
…restarts) (#6151) * perf(dev): re-enable the Turbopack dev filesystem cache (5.4x faster restarts) `turbopackFileSystemCacheForDev` has been `false` since #5408 — a landing-page homepage redesign whose description covers hero cards, feature-card aspect ratios, eyebrow chips and a voice-input button color, and never mentions Turbopack, caching, or dev performance. It was collateral, not a decision, and it overrode the Next default (true since v16.1). It is not the flag #6078/#6080 measured. That A/B was `...ForBuild` and its conclusion stands — the build cache is a 3.2x regression and stays off. The two flags look alike and are opposite decisions; both are now commented as such. Measured on `/workspace/[workspaceId]/w`, n=3 per arm, SIGINT between runs: cache OFF 31.4s / 30.1s / 31.9s RSS 9.0-9.8 GB cache ON 5.6s / 5.6s / 5.5s RSS 4.4-5.1 GB 5.4x faster restarts, ~2x less resident memory. Cold compile against an empty cache is unchanged (~32s either way) — the cache only pays back on restart, which is the loop that actually hurts. The cache is unbounded on disk: the abandoned one on this machine had reached 78 GB across 1,848 SST files, and a stale cache is slower to read back, so left alone it erodes the win it exists to provide. `prune-turbopack-cache.ts` runs on `predev` and drops it past a cap (default 20 GB, `SIM_TURBOPACK_CACHE_MAX_GB` to override); `bun run dev:cache:prune` forces it. It never blocks `next dev` on a maintenance failure. Adds a `dev-performance` skill recording the cost model, the reference numbers, and the benchmarking method — including that stopping the server with `kill -9` mid-cache-write discards the cache and makes this exact win read as no win. * improvement(dev): chain the cache prune into dev scripts instead of a predev hook Review read the root `bun run dev` path as bypassing the `predev` hook and so never capping the newly-enabled cache. Turbo does fire `pre*` hooks — verified live, the run prints the prune before `next dev` — but the concern is fair in that the guarantee rested on package-manager lifecycle semantics that are invisible at the call site. Chaining it explicitly removes the question entirely: every `dev` variant now runs `bun run dev:cache:cap && …`, which holds on any invocation path, is visible in the command itself, and drops the three duplicated `predev:*` entries for one shared script. Verified on both paths — direct `bun run dev` and root `turbo run dev`, the latter printing: sim:dev: $ bun run dev:cache:cap && next dev --port 3000 sim:dev: $ bun run ../../scripts/prune-turbopack-cache.ts * docs(dev): document cache-corruption recovery, the cost of enabling the cache Stress-tested the failure mode rather than assuming it: deliberately corrupting an SST block makes Turbopack abort with a FATAL panic — it does not self-heal. FATAL: An unexpected Turbopack error occurred. Cache corruption detected: checksum mismatch in block 4 of 00000221.sst `bun run dev:cache:prune` and restart fixes it; verified the canvas serves 200 again afterwards. Documented in the skill and in the script's header, since the symptom is a hard crash and the remedy is not guessable. This is the honest cost of turning the cache on. It is worth paying — a 5.4x faster restart against a rare, loud, single-command failure — but it should be written down rather than discovered. Worth distinguishing from the adjacent case: an ordinary hard kill does *not* corrupt the cache. Turbopack discards a partially-written cache and rebuilds it silently, which is exactly why a `kill -9`-based benchmark reads as "no cache win" (noted in the benchmarking section). * refactor(dev): drop the dev-performance skill, keep its findings at the code A whole skill was too much for what this is. The parts that are load-bearing — why the two lookalike cache flags are opposite decisions, the measured numbers, the corruption remedy, and the benchmarking trap — now live in the config and script they describe, where someone changing the flag actually reads them. The trap is the piece worth keeping: `next dev` compiles on demand so startup time is meaningless, and stopping the server with `kill -9` makes Turbopack discard a partially-written cache and rebuild silently — which reads as 'the cache does nothing' and is how this flag stayed wrong for a month. Dropped rather than relocated: generic advice that was not specific to this repo (antivirus, Docker-on-macOS, orphaned processes) and a measured no-op (`optimizePackageImports` for lucide-react changed nothing, 31.6s vs 31.7s). * docs(dev): record the measured cost and concurrency behaviour of cache pruning Stress-tested the maintenance path rather than assuming it is free. Cost: the size walk is ~30ms on a real cache and ~85ms at 2,000 files — under 2% of a 4.2s warm restart, and invisible against a cold one. It runs before every dev start, so it needed to be cheap; it is. Concurrency: pruning while a dev server is live (which happens when a second server is started from the same checkout) does not crash it. The running server keeps its in-memory state and kept serving HTTP 200 with zero panics. It does stop persisting for the rest of that session, so its next start is cold once — verified recovering at 23.4s then 4.5s. Worth writing down because the directory silently never reappears mid-session, which looks like a bug if you go looking. The cap is a backstop, not routine: a normal session sits at 1-2 GB against a 20 GB default. * fix(dev): cap every app's Turbopack cache, not just apps/sim `apps/docs` is a Next app too (`next dev --port 3001`) and overrides nothing, so it uses the Next default where the dev filesystem cache is on. It already had an uncapped 1.1 GB cache here, and the root `bun run dev` (`turbo run dev`) starts it — so a teammate using the documented command was accumulating a cache nothing would ever prune. The script now resolves its target from the working directory instead of hardcoding `apps/sim`, and each app chains its own cap. Per-app rather than one sweep on purpose: a single pass would let one app's dev start delete a cache another app is holding open, which costs that session its persistence. Verified both: `apps/sim` and `apps/docs` each report and cap their own 1.1 GB cache, and both dev servers start clean (`Ready in 299ms` / `229ms`, docs serving). * refactor(dev): drop dev:cache:prune in favour of the existing dev:clean `dev:cache:prune` duplicated `dev:clean`, which already existed in `apps/sim` and does strictly more (`rm -rf .next/dev/cache` covers the Turbopack cache plus the fetch and image caches). Two commands for one job is worse than one, and the docs pointed at the newer, narrower of the two. Removes it from both apps and gives `apps/docs` the `dev:clean` that `apps/sim` already had, so the recovery command is the same everywhere. `dev:cache:cap` stays — it is the chained step, used by more than one dev variant, and naming it keeps the relative script path out of each command. Verified `dev:clean` is a real remedy: corrupt a cache block, run it, restart — canvas serves 200 with no panic. Also corrects an overstatement. A damaged cache does not *always* abort Turbopack; whether it panics depends on whether the damaged region is read, so it is not reliably reproducible. Both notes now say "can abort" and give the same remedy either way.
#6152) * perf(tools): move mergeToolParameters into a registry-free leaf module `providers/utils.ts` imports exactly one thing from `@/tools/params`: `mergeToolParameters`. That function performs no tool lookup at all — it merges two plain param objects. But `params.ts` imports `getTool` from `@/tools/utils`, which statically imports the 4,300-entry `@/tools/registry` barrel, so that one-symbol import was dragging the entire tool registry into every module graph that reached it. Measured with a module-graph walk from each entry: tools/params.ts 4,926 modules (registry reachable) providers/utils.ts 4,926 modules (registry reachable) -> 22 modules ✅ tools/merge-params.ts 2 modules (registry NOT reachable) `mergeToolParameters`, `deepMergeInputMapping` and `isNonEmpty` move to `@/tools/merge-params`, which is forbidden from importing `@/tools/utils`, `@/tools/registry` or `@/tools/params`. `params.ts` now imports `isNonEmpty` from there; its `isRecordLike` and `isEmptyTagValue` imports became unused and are dropped. The two consumers (`providers/utils.ts`, `executor/handlers/pi/sim-tools.ts`) import from the new module directly rather than via a re-export, per the no-re-exports rule. This is preparation, not the payoff. The canvas route still reaches the registry through three other edges (block-outputs, serializer, sanitization/validation) — all four are redundant paths and must all be cut before the route's module count moves. Those follow in the metadata-manifest PRs. Behaviour is unchanged: the moved functions are copied verbatim. * refactor(tools): make deepMergeInputMapping module-private It was exported from `@/tools/params` and imported by nothing — a private helper of `mergeToolParameters` that had leaked into the public surface. Since this move created the module, the export goes with it rather than being carried forward. Verified zero consumers repo-wide before dropping it.
* perf(tools): generate serializable tool metadata artifacts
Adds `scripts/sync-tool-metadata.ts`, which projects the executable tool
registry down to the data half nobody needs a closure for, plus typed accessors
over the result. No consumer is rewired yet — that is the next PR.
`@/tools/registry` is a ~9,000-line barrel over 4,366 tools. Each `ToolConfig`
mixes plain data (`params`, `outputs`, `name`) with closures (`request.headers`,
`transformResponse`, `directExecution`, `postProcess`), and those closures reach
every integration's SDK client and parser — which is why reaching the barrel
costs ~4,700 modules. Every client-reachable caller was audited: none of them
need a closure. They need `outputs`, `params`, or an existence check.
Two artifacts, not one. `outputs` is ~4 MB of the ~8 MB and has a single
consumer, so it is emitted separately and exposed from its own module; callers
needing only params never load it.
The data is a JSON string parsed at runtime rather than an imported `.json` or
an object literal. That is not stylistic — with `resolveJsonModule` (enabled
repo-wide) a `.json` import makes TypeScript infer a literal type for all 4,366
entries:
tsc --noEmit, baseline 12.6s
tsc --noEmit, with `.json` imports 8m07s (38x)
tsc --noEmit, with string literals 12.0s
An ambient `declare module` does not short-circuit it (measured: 8m18s), and an
object literal is the same inference work. A single string literal is one cheap
token for the compiler and the bundler, and `JSON.parse` beats evaluating the
equivalent literal at runtime.
The generator refuses to emit any function value, so shipping executable config
to the client fails loudly instead of silently. `hosting` and `schemaEnrichment`
are excluded on those grounds — both hold functions and are server-only.
Also strips empty param entries: the registry has one (`stt_deepgram_v2`, an
`undefined`) which crashes callers that read `param.type` while iterating.
`JSON.stringify` drops `undefined` on its own, so the guard is there for an
explicit `null` — which serializes faithfully and would reach consumers — and to
warn either way.
Wires `tool-metadata:check` into CI alongside the other generated-contract
gates, and ignores the generated directory in biome (it exceeds the 1 MB limit
and was being skipped with a notice on every commit).
Adds a `tool-registry-boundary` skill covering which module to import, the three
non-obvious properties of the artifacts, and how to verify an edge is actually
cut — the canvas route reaches the registry through four redundant paths, so
cutting one alone moves the module count by ~1.
* fix(tools): harden the metadata accessors against inherited keys
Review found two real defects in the generated-metadata layer.
`JSON.parse` returns an object with the normal prototype, so a bare bracket
lookup resolved inherited members: `getToolMetadata('constructor')` returned a
*function* typed as `ToolMetadata`, and `getToolOutputsMetadata('toString')`
likewise — silently violating the accessors' documented "undefined if unknown"
contract. Guarded with `Object.hasOwn`, with a parameterised regression test
over `constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__`.
The generator's no-functions scan also gave up past ten levels of nesting. Param
and output schemas nest arbitrarily, so a deeper closure would have been dropped
silently by `JSON.stringify` while generation reported success — shipping an
incomplete schema and defeating the guarantee the scan exists to provide. The
depth cap is gone; a `WeakSet` handles the cycles that exposes.
* docs(tools): tell tool authors to regenerate the metadata artifacts
A new tool now has a second registration step. Client code reads `params` and
`outputs` from the generated artifacts rather than from the registry, so a tool
added without regenerating them is registered but invisible to the UI — and CI
fails on the stale artifacts.
`add-tools` and `add-integration` are where someone actually adds a tool, so the
step goes in both, next to the registry edit and in each checklist.
* docs(blocks): note when a block change needs tool-metadata regeneration
Adding a block alone needs no regeneration — it references existing tool IDs and
changes no tool's shape. But a change that touches a tool alongside the block
does, and this is where that is easy to miss: a block's `outputs` are authored
to match its tools' outputs, and the UI now reads those from the generated
metadata, so a stale artifact makes the block's declared outputs disagree with
what the panel renders (and fails CI).
Completes the tool-authoring surface alongside add-tools and add-integration.
* docs(tools): cover tool removal in the regeneration guidance
The three tool-authoring skills said to regenerate after adding or changing a
tool, but not after removing one. Removal is equally breaking and equally
guarded: deleting a tool from `tools/registry.ts` without regenerating fails
`tool-metadata:check` (verified — exit 1), so a contributor following the skill
literally would have hit a CI failure the skill never warned about.
…hs (#6155) * perf(tools): read tool metadata instead of the registry on client paths Cuts the last four edges that pulled `@/tools/registry` into the workspace shell. Every workspace route drops ~4,700 modules: route before after /w (canvas) 6,592 1,908 -71% /logs 6,227 1,543 -75% /tables 5,903 1,217 -79% /files 5,996 1,310 -78% workspace layout 5,751 1,063 -82% Dev cold compile of the canvas, n=3, cache cleared between runs: before 32.3s / 31.4s / 30.1s RSS 9.0-12.5 GB after 22.4s / 22.2s / 21.6s RSS 7.8-9.2 GB That lands where the `dev:minimal` escape hatch measured (20.0s / 6.7 GB) without its downside — `dev:minimal` swaps in curated registries that drop ~250 services, whereas this keeps every tool working. Rewired: - `block-outputs` -> `getToolOutputsMetadata` (needed `outputs`) - `serializer` -> `getToolParams` (needed `params`) - `validation` -> `hasToolId` (needed existence only) - `tools/params` -> `getToolMetadata` (needed `params`, `oauth`, `name`) `tools/params.ts` was the stubborn one: `mcp-dynamic-args.tsx` imports only `formatParameterLabel` from it, so the whole registry rode in behind a string helper — the same shape as the `mergeToolParameters` edge cut earlier. Adds a third generated artifact, `tool-ids.ts` (~110 KB). Resolution needs only the key set, so `@/tools/metadata` and `@/tools/metadata-outputs` both resolve through it and stay independent of each other, and an existence check costs ~110 KB instead of ~4 MB. Behaviour preservation was the risk here: `getTool` resolves an unversioned name onto its newest version, and a plain key lookup would have silently reported 246 versioned tools as missing. `resolveToolId` is reproduced against the id set and differentially tested — 4,404 probes (every id, every stripped base name, and an unknown) comparing old vs new resolution and existence: 0 mismatches. `ToolWithParameters.toolConfig` and `SubBlocksForToolInput.toolConfig` narrow from `ToolConfig` to `ToolMetadata`. The only external reader is `tool-input.tsx`, which uses `.name`. * docs(tools): point the boundary skill at the three metadata modules The skill still routed `hasToolMetadata` and `getToolIds` to `@/tools/metadata`, but this PR moved id resolution into `@/tools/tool-ids`. Left as-is it would send the next caller to the 4 MB module for an existence check that costs 110 KB — the exact mistake the skill exists to prevent. Also records the two properties a caller can silently get wrong: lookups guard with `Object.hasOwn` (a bare bracket lookup returns inherited prototype members), and they resolve unversioned names (246 tools are versioned, and a plain lookup reports them missing rather than crashing). * fix(tools): cut the settings-route registry edge and fix serializer test mocks Two findings from review, both real. The settings route still reached the registry: settings/[section]/page.tsx -> settings.tsx -> (dynamic import) ee/access-control/components/access-control.tsx -> group-detail.tsx -> tools/utils.ts -> tools/registry.ts It reads `getTool(id)?.name` — metadata — so it moves to `getToolMetadata`. The earlier audit missed it because it walked only from the canvas route, and the edge hides behind a dynamic `import()` that a static walk skips. Serializer tests mocked the wrong module. `Serializer` now reads params via `getToolParams` from `@/tools/metadata`, but the tests still only mocked `@/tools/utils`, so they controlled nothing and passed because the real generated artifacts happen to agree with the fixtures. Adds `toolsMetadataMock` to `@sim/testing/mocks`, backed by the same `mockToolConfigs` as `toolsUtilsMock` so a test mocking both sees one consistent tool universe, and mocks it in the three serializer suites. Verified the mock is now load-bearing: pointing it at a sentinel param makes the three user-only-required validation tests fail, and restoring it returns all 110 serializer tests to green. Before this they passed either way. * fix(tools): freeze the tool id array handed out by getToolIds `getToolIds()` returned the module's internal array by reference, so a caller doing `getToolIds().sort()` would reorder it in place and silently corrupt every later lookup — the in-place-mutation footgun `.claude/rules/sim-react-performance.md` calls out. Frozen rather than copied: the array is consumed in loops, so copying would allocate on every call. Freezing makes the mutation throw instead of corrupt, and `[...getToolIds()].sort()` still works. Return type is now `readonly string[]`, so the mistake is a compile error rather than a runtime surprise. No caller mutates it today; this is closing the hole, not fixing a live bug. * test(tools): enforce that the two tool-id resolvers never diverge `resolveToolId` now exists twice on purpose — `@/tools/utils` resolves against the live registry (so a tool added before regeneration still resolves at runtime), `@/tools/tool-ids` against the generated id list (so client code resolves without importing 4,300 tools). Nothing structurally kept them in step; a change to versioning logic in one would silently drift from the other. `tool-metadata:check` now asserts they agree across every id, every stripped base name, and an unknown — 4,404 probes — and only after the staleness check passes, so a missing regeneration reports as staleness rather than as drift. Verified it fails: breaking resolution for `gmail*` exits 1; restoring it passes. It cannot live in a vitest suite. `vitest.setup.ts` globally mocks `@/tools/registry` to an empty map, so `getTool` resolves nothing there — a parity test written as a spec passes or fails for the wrong reason. Both facts are recorded where the code is. Both resolvers stay exported. An earlier pass here un-exported the `@/tools/utils` one as dead; `tools/utils.server.ts` imports it through a multi-line import that a grep missed, and `tsc` caught it. Its doc now says which resolver a caller should reach for instead of leaving two identically-named functions unexplained.
* perf(tools): guard the tool-registry client boundary in CI
The registry was 71-82% of every workspace route's module graph, and the two
edges that put it there were invisible at the call site: `providers/utils.ts`
imported `mergeToolParameters`, and `mcp-dynamic-args.tsx` imported
`formatParameterLabel`. Neither import looks remotely like "pull in 4,700
modules of SDK clients", which is why this needs a lint rather than a convention.
`check-tool-registry-boundary.ts` walks the value-import graph (skipping
`import type`, which is erased) from the workspace layout and the four routes
that mount inside it, and fails if `@/tools/registry` is reachable — printing
the exact chain that reintroduced it.
Verified it fails: reintroducing a `getTool` import in `serializer/index.ts`
exits 1 and names the chain through `stores/workflow-diff/store.ts`; removing it
returns to 0.
There is deliberately no allowlist. The fix for a failure is always to move the
symbol the file actually needs into a registry-free module, not to exempt the
route.
Documents the guard in the tool-registry-boundary skill.
* fix(tools): close two edge-detection gaps in the registry boundary guard
Review found the walker missed two forms, both verified against a matrix of
every import/export shape:
export * as ns from '…' namespace re-export — the star branch had no alias
import('…') dynamic import
A dynamic import splits the registry into its own chunk rather than the route's
initial one, so it does not show up in cold-compile time — but it still puts
4,300 tools' worth of executable config on a client path, which is what this
guard exists to prevent. It counts as reaching the registry. No such import
exists today; this is purely closing the hole.
Adding both raised the measured counts (tables 1,217 -> 1,261, files
1,310 -> 1,419) because lazily-loaded modules are now counted. The registry
stays unreachable from all five entries.
Also checked and rejected: side-effect imports (`import '@/x'`) were reported as
missed, but are matched both standalone and after another import — the `from`
clause is already optional.
* fix(tools): resolve extensionful specifiers in the boundary guard
`resolveSpecifier` probed `base + ext` and `base/index + ext` but never `base`
itself, so an already-extensioned specifier resolved to null and its edge
vanished from the walk — `import { tools } from '@/tools/registry.ts'` would
have passed the guard silently.
Not theoretical: `executor/execution/block-executor.ts` already imports
`@/executor/human-in-the-loop/utils.ts` with the extension, so real edges were
being dropped. Counts rise slightly now that they are followed (canvas
2,023 -> 2,029).
Verified: the extensionful import exits 1, and removing it returns to 0.
* fix(tools): discover guard entries instead of listing them
Review caught the guard checking the wrong shell: it named
`app/workspace/layout.tsx` as "the shared shell every route mounts inside", but
that file only wraps `SocketProvider`. The real shell is
`app/workspace/[workspaceId]/layout.tsx`, which pulls in `WorkspaceChrome`, the
loaders and the providers — and it was never checked.
Worse, layouts are composed by Next.js convention rather than imported, so a
page's graph never reaches its layout at all. Walking pages alone left every
layout module outside the guard.
So entries are now discovered: every `page.tsx` and `layout.tsx` under
`app/workspace`, 35 of them instead of a hand-written 5. A list goes stale
silently; discovery cannot. Refuses to pass vacuously if the walk finds none.
Immediately found a real edge the hand-written list had missed — the settings
route reaching the registry through a dynamically-imported access-control panel
(fixed in the previous commit). Full walk takes ~2s.
Also restores the extensionful-specifier fix, which a bad merge had dropped from
this file. Re-verified both directions: an extensionful `@/tools/registry.ts`
import exits 1, removing it returns to 0.
* fix(tools): restore the dynamic-import and namespace-alias edge detection
A bad merge during a rebase reverted this file to a pre-fix revision, silently
dropping `DYNAMIC_IMPORT_RE` and the `export * as ns from` alias branch that
earlier commits on this branch had already added. The guard still passed, which
is the worst way for a lint to break — it simply stopped following edges.
Caught it because the per-route counts fell after the rebase (files
1,424 -> 1,314, logs 1,610 -> 1,545) rather than staying put. A guard that
reports fewer modules after a no-op merge is not passing, it is blind.
Now verified against every bypass form rather than the one I happened to think
of, so a future regression of this kind fails loudly:
CAUGHT extensionful import { tools } from '@/tools/registry.ts'
CAUGHT dynamic import('@/tools/registry')
CAUGHT ns re-export export * as ns from '@/tools/registry'
CAUGHT side-effect import '@/tools/registry'
CAUGHT plain named import { tools } from '@/tools/registry'
clean tree passes
* fix(tools): traverse require() edges in the boundary guard
Review flagged `require()` as an untraversed edge form, and it is not
hypothetical here — this codebase uses lazy `require('@/…')` to break import
cycles, including from a client-reachable file (`tools/params.ts` reaches
`@/blocks` that way). Those edges are as real as static imports; a `require` of
the registry would have walked straight past the guard.
The audit now covers every form a module can be reached by, each verified rather
than assumed:
CAUGHT plain named import { tools } from '@/tools/registry'
CAUGHT side-effect import '@/tools/registry'
CAUGHT extensionful import { tools } from '@/tools/registry.ts'
CAUGHT ns re-export export * as ns from '@/tools/registry'
CAUGHT dynamic import('@/tools/registry')
CAUGHT require require('@/tools/registry')
clean tree passes
No new violations surfaced — the 35 guarded page/layout graphs stay clean with
require edges followed.
* refactor(dev): remove the minimal-registry escape hatch `dev:minimal` existed because the tool registry was 71-82% of every workspace route's module graph and aliasing it away was the only way to make dev bearable. The metadata work removed that reason, so the hatch now buys almost nothing: before this stack 31.7s -> 20.0s cold (-37%) after this stack 22.9s -> 20.7s cold (-10%) A 10% cold-compile win, on the run that happens once — restarts are ~4.2s either way — is not worth what it costs. `tools/registry.minimal.ts` and `blocks/registry-maps.minimal.ts` are 283 lines of hand-curated duplicates of the real registries that **nothing keeps in sync** (no lint, no CI check, no test); they are correct today only because someone remembered. And the mode is actively misleading: it silently drops ~250 services and ~280 blocks, so anything reproduced under it may not reproduce for real. Removes both files, the `SIM_DEV_MINIMAL_REGISTRY` branch from `next.config.ts` (including the whole `webpack()` hook, which existed only for this), and the `dev:minimal` / `dev:full:minimal-registry` scripts. Verified after removal: `tsc` clean, boundary + metadata + skills + monorepo gates pass, and `next dev` starts and serves the canvas at 22.6s cold / HTTP 200. * fix(setup): stop the wizard offering the removed minimal-registry mode The setup wizard prompted for a dev server on machines under 16GB and **defaulted** to `dev:full:minimal-registry` — a script this stack deletes. Anyone running `bun run setup` on a low-RAM machine would have accepted the default and hit "Script not found", which is exactly the contributor the mode existed to help. Repointed at `dev:full:capped`, which still exists and caps Node at 4GB without dropping ~250 integrations — a strictly better answer to the same question. The hints were also stale: they warned the full registry "can use 4-5GB+ on its own", which was true when a dev server sat at 11.5GB. It now sits at ~4GB, so they say that instead. Missed by an earlier sweep because the pattern searched for `dev:minimal` and `registry.minimal`, and this string is `dev:full:minimal-registry` — the two halves reversed. Re-swept across every file type for all spellings: zero references remain. Also audited every script value the wizard can return, so the class of bug is checked, not just this instance.
…#6165) `tailwindcss-animate` adds a `duration-*` utility for `animation-duration`, which collides with core's `transition-duration`. For a named value Tailwind emits both rules, but for an arbitrary value it cannot pick one — so it drops the class entirely and warns. The result is 52 usages across 13 files that produced **no CSS at all**. Elements marked `transition-transform duration-[1700ms] ease-[cubic-bezier(0.22,1,0.36,1)]` had neither a duration nor an easing function: they snapped instead of animating. Most of it is the landing hero. Verified by compiling the stylesheet on both sides: transition-duration: 1700ms before=0 after=1 transition-duration: 420ms before=0 after=1 transition-duration: 175ms before=0 after=1 transition-timing-function: cubic-bezier(0.22,1,0.36,1) before=0 after=1 transition-timing-function: cubic-bezier(0.23,1,0.32,1) before=0 after=1 The stylesheet grows 1,425 bytes — exactly the rules that were missing — and the 16 ambiguity warnings on every dev start go to zero. Every usage is in a transition context (`transition-opacity`, `transition-transform`, `transition-[...]`); none sits alongside an `animate-*` utility, so `transition-duration`/`transition-timing-function` is the right half of the collision in all 52 cases.
… Options Compare (#6164) * feat(library): Agentic AI Coding Tools: What They Are and How the Top Options Compare * feat(library): add generated cover for agentic AI coding tools post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com>
…6000) * fix trace span secret sanitization * sanitize workflow output logs * preserve streaming usage estimates * Fix logging session test after staging merge * secrets sanitization correctness * fix(execution): address review regressions * fix(execution): harden secret trace provenance * fix(execution): preserve functional state during trace projection --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Mac.localdomain> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
…ze (#6168) The SFTP/SSH download routes buffered a remote file into memory with the only size guard being sftp.stat().size — a value the caller-supplied SSH server controls. A server that reports a tiny size and then streams endlessly drove unbounded heap growth until OOM. Read through the existing readNodeStreamToBufferWithLimit limiter, which enforces the cap on bytes actually received and destroys the stream on breach, via a small readSftpFileCapped wrapper in sftp/utils. All four SFTP-reading tool routes now share it — download, download-file, read-file-content, and the append path of write-file-content, which had no cap at all. The wrapper keeps a no-op error listener on the stream for its whole life: ssh2 rejects still-pending SFTP requests when the channel closes, which arrives as a late error event after the limiter has detached its own handlers, and an error event with no listener would take the process down. Too-large responses now return 413 to match how the rest of the routes surface PayloadSizeLimitError, and responses report the actual byte count rather than the server-reported stat size.
…fault (#6169) secureFetchWithPinnedIP only capped a response body when the caller passed maxResponseBytes; with the option absent the body streamed into an unbounded Buffer.concat. Several tool proxies whose target host is user-supplied (Jupyter, ClickHouse, Grafana, 1Password Connect) and the RSS poller called it without that option, so an attacker-controlled server answering with an endless chunked body could grow the shared process heap until it was OOM-killed. Make the cap fail-safe: default to 100MB (and treat a non-positive value as the default) so there is no unlimited mode, then pass tighter explicit caps at the user-supplied-host sites. Responses that carry no body (HEAD, 204, 304) are exempted from the content-length pre-check — they advertise the resource size as metadata, which would otherwise spuriously fail a HEAD probe of a large file or an RSS conditional-GET 304.
…at (#6087) * feat(chat): highlight-to-chat for file and table selections Adds an IDE-style "add to chat" affordance so the Sim agent can reference an exact passage of a file or a specific set of table rows/cells instead of a whole resource. - Two new ChatContext kinds: file_selection (inline selected text + line range) and table_selection (authoritative row ids, optional column ids; the server re-fetches current rows by id). Chip registry, client serializer, boundary contract, and server resolver all extend along their existing switch(kind) seams. - Producers: Monaco context menu, Tiptap bubble menu, and the table grid context menu, all using the Sim Chat block icon. Adding a selection opens the split slideover (chat + resource) with the chip dropped straight into the input; from a standalone Files/Tables page the context is stashed and drained on chat mount. - Cmd+C / Cmd+V: a selection copied from a file/table rides a custom text/x-sim-selection clipboard MIME and pastes into chat as the same reference chip; the chat input round-trips a sole selection chip on copy/cut too. - Guards: selection payloads are length/row/column bounded and truncated within the schema bound; labels carry a deterministic key so distinct same-size selections don't collide; a shared resource tab closes only once no remaining chip references it; stale cell-range column ids drop the context rather than dumping the full table. * fix(chat): widen selection code fence so embedded backticks can't truncate it Cursor: resolveFileSelectionResource wrapped the selected passage in a fixed ``` fence, so a selection that itself contained a fenced code block closed the outer fence early and the agent received a truncated snippet. The fence is now one backtick longer than the longest backtick run in the content (floored at three), matching CommonMark's close rule. * fix(chat): carry the selection chip on a column-header copy Cursor: a column-header Cmd+C always took the async paged clipboard path, which replaces the whole clipboard with text/plain only and can't carry a custom MIME - so pasting a column copy into Chat couldn't rebuild the table_selection chip that Add-to-chat produces for the same selection. When every row is loaded and within the chat-selection cap, the column copy now does a synchronous event write so the scoped table_selection rides alongside text/plain (mirroring the row-'some' and cell-range paths); oversized/partially-loaded columns keep the async plain-text path. * refactor(chat): clean up highlight-to-chat selections Correctness: - Stop reporting fabricated line numbers for rich-markdown selections. `doc.textBetween` counts ProseMirror block boundaries, not markdown source lines, so the chip label and the agent prompt both claimed line ranges that don't exist in the file. Line info is now emitted only by Monaco. - Bound a table_selection's rendered markdown by characters, not just row and column counts — 500 wide rows dwarfed the 20k-char file-selection budget. Rows are emitted until the budget is spent, and the content says what was omitted. - Complete `areContextsEqual` for both selection kinds, so re-adding the same selection dedupes while a different passage of an already-referenced file registers as new. Consistency: - Carry the resource display name on the context instead of recovering it by regex from the chip label, deleting fileNameFromSelectionLabel and tableNameFromSelectionLabel. - Replace the user-visible `#k3f9` hash disambiguator with a readable ordinal applied at insert time (`Sales (3 rows) (2)`), via a shared prepareContextForInsert used by both the add-to-chat and paste paths. - Fold MothershipPendingContextStorage into MothershipHandoffStorage as a chip-only handoff (optional message), removing the parallel storage class, the second drain effect, and its StrictMode guard ref. - Replace `window.location.assign` with `router.push`, matching the existing "Troubleshoot in Chat" handoff. - Collapse three near-duplicate synchronous copy branches in table-grid into shared buildTableSelectionContext / writeLoadedRowsWithChip helpers, also reused by the add-to-chat handler. - Trim multi-paragraph inline comments to TSDoc stating each reason once. Tests: new coverage for the character budget, the label ordinal, selection equality, and chip-only handoff accumulation; each verified to fail when its fix is reverted. * refactor(chat): tighten table copy fallback and helper placement - writeLoadedRowsWithChip now requires a chip to carry; with none it falls through to the canonical paged path (preserving its row loading and truncation notice) instead of doing a bare synchronous write. - Reuse selectedColumnIds in the context-menu memo. - Restore resolveTableSelectionResource's TSDoc, orphaned when renderTableCell was extracted between the doc and its function. * fix(chat): apply chip handoffs as one batch; widen table copy chip path Cursor Bugbot: a multi-context chip handoff dispatched one event per context, and insertContextChip resolved label collisions against selectedContexts read through a ref that only refreshes on render. Each dispatch therefore saw the same stale list, so a second same-label selection was never ordinalized and addContext dropped it while its @token still landed in the text. The event now carries the whole batch and insertContextChips threads each resolved context forward as it goes. Greptile: an explicit multi-row ('some') selection no longer requires every selected row to be loaded before taking the chip-carrying sync path. That gate assumed the paged fall-through would copy more, but its loadRows returns rowsRef.current unchanged for 'some' — the same rows, minus the chip. Renamed the parameter to to say what it actually gates on. Remaining chip-less cases are inherent to the async Clipboard API, which replaces the whole clipboard and cannot hold a custom MIME: a filtered select-all (must page in more rows) and selections past the 500-row chip cap. 'Add to chat' covers both — it is not gesture-bound and drains to the cap. * fix(chat): distinguish a line-less file-selection label from the whole file Cursor Bugbot: a rich-markdown selection labelled itself with the bare file name, which is exactly the whole-file chip's label. Menu-driven inserts reject any context whose label is already taken (isContextAlreadySelected closes the menu silently), so once a markdown selection was attached, mentioning that same file was quietly dropped. One-way: the reverse order works because programmatic inserts ordinalize through prepareContextForInsert. Fallen out of dropping the fabricated line range from this editor — with no range left, the label collapsed onto the file name. Fixed at the single source: buildFileSelectionLabel now returns 'notes.md (selection)' when there is no line range, so it stays honest about location while remaining distinguishable. * fix(chat): don't revive an aged-out chip handoff when accumulating Cursor Bugbot: pendingContexts merged a prior chip-only handoff without checking its age, while store stamps a fresh timestamp on every write. An abandoned handoff that had already passed max-age would therefore ride along on the next 'Add to chat' and fire on the following navigation as if current. Introduced by the accumulation behavior added earlier in this PR. Applied the same freshness bar consume uses, and hoisted the 60s window into a single MAX_AGE_MS constant so the two paths cannot drift. * fix(chat): reference every selected row in a table chip, not just loaded ones Cursor Bugbot: for a 'some' row selection the chip's rowIds came from the loaded-page intersection (currentRows filtered by the selection) rather than rowSel.ids. The chip carries ids and the server re-fetches them via getRowsByIds, so a selected row that simply had not been paged into the grid was silently dropped from the agent's context — select 600 rows with 200 loaded and the agent saw 200. Add to chat had the same loaded-only narrowing through contextMenuRowIds. Both now send the full selection, still bounded by MAX_TABLE_SELECTION_ROWS. Only the pasted text stays limited to loaded rows, which is inherent — there are no cell values to serialize for a row that has not been fetched. * docs(chat): scope the table copy 'complete' comment to the text path It read as though the whole selection were complete when ids are unloaded, which is now only true of the serialized text — the chip deliberately carries every selected id for the server to re-fetch. * fix(chat): compare selection ids as sets, not sequences Cursor Bugbot: sameIds compared rowIds/columnIds by index, but a table selection's ids iterate in click order (they come from a Set), so the same rows picked in a different order — or reached via a cell range rather than the gutter — compared unequal. prepareContextForInsert then added a second ordinalized chip pointing at rows already referenced instead of no-opping. More reachable since the previous commit started sourcing rowIds from rowSel.ids directly, where insertion order tracks the user's clicks. * fix(chat): enforce the table selection budget over the whole rendered content Cursor Bugbot: the budget subtracted only the header and divider before packing rows, then prepended the 'Selected ...' prose and the newlines afterward, so the final content could exceed MAX_TABLE_SELECTION_CONTENT_LENGTH whenever the last accepted row left less slack than the prefix needed. The cap the TSDoc promises was not actually enforced. The prior test passed while missing this: its rows were wide enough that packing stopped far short of the limit, so the boundary was never exercised. Replaced with rows sized to fill the budget almost exactly, asserting both that the content stays within the cap and that it still approaches it (so the assertion can't pass by emitting an empty table). Also capitalize Chat in the three 'Add to Chat' menu labels, matching the constitution's module naming and the existing 'Fix in Chat' / 'Troubleshoot in Chat' UI strings. * fix(chat): don't swallow a paste whose selection chip is already attached Cursor Bugbot: the selection-paste path called preventDefault before prepareContextForInsert, so when that returned null (the same selection is already a chip) the handler returned having claimed the event — no chip inserted and no text/plain pasted either. Cmd+V did nothing. preventDefault now waits until there is a chip to insert; a duplicate falls through to the plain-text paste below, which is the reasonable reading of the gesture. * fix(chat): derive the budget reserve from the same clause it reserves for Cursor Bugbot: worstCaseSizeClause built its own string that omitted the row/rows word the real clause always carries, so the reserve ran ~5 characters short and a tightly packed selection could still exceed MAX_TABLE_SELECTION_CONTENT_LENGTH. A bug in the previous fix, from duplicating the format instead of sharing it. Replaced with one sizeClause(shown, omitted) used for both the up-front reserve and the final prose, so the two cannot describe the count differently. The reserve passes (rows.length, rows.length) — max digits on both counts and the plural forced — which is an upper bound on any real clause. The earlier tight-packing test could not see this: a single cell width leaves whatever remainder it leaves, and 100 left more than 5 characters. Added a sweep over widths 60-75 that collects overflows so a failure names the width; it catches the reported bug at width 74 (20002 vs 20000). * fix(chat): align MothershipChat's onContextRemove with the surface contract The remaining-contexts argument was added to ChatSurfaceContextValue but not to MothershipChat's own prop type, which forwards straight into it. Nothing passes the handler there today so it typechecked (fewer params is assignable), but the two declarations of the same wiring disagreed. Does not change behavior: home still wires the remove handler only to the empty-state surface, as it did before this PR. * refactor(chat): apply cleanup pass findings emcn design: the table context menu built its Add-to-Chat label in the parent and never pluralized it, so right-clicking a single row read 'Add rows to Chat' directly above 'Delete row'. Derive it inside ContextMenu from selectedRowCount like every sibling label, with an addToChatCellScoped boolean mirroring workflowCellScoped. Also more correct: selectedRowCount accounts for a select-all beyond the loaded page, which contextMenuRowIds.length does not. callbacks: drop two useCallback wrappers whose identity nothing observes — both handleAddSelectionToChat handlers feed unmemoized components (one through an inline arrow). buildSelectionContext stays wrapped; it is a real dep of the copy-bridge effect. comments: fold a duplicated rationale paragraph in handleAddSelectionToChat left by two commits stacking, drop a {@link MothershipHandoff} that resolves to nothing (the type is not imported there), and trim a rowIds doc that restated its own type. Skipped, deliberately: the effects pass proposed moving buildContext out of useSelectionCopyBridge's deps behind a latest-ref. buildSelectionContext is already stable, so churn is near-zero, and it would leave two sibling useCallbacks purposeless. * fix(chat): don't attach a selection chip to a copy from a nested input Cursor Bugbot: a copy from a field inside the editor — Monaco's find box being the common one — bubbles to the container while the document still holds a highlight, so the bridge attached the editor selection to text the user never copied. Chat paste then prefers the custom MIME and inserts a reference chip instead of the search term. Skips INPUT targets only. Copying the table grid's INPUT/TEXTAREA guard would have suppressed the chip on the main copy path this hook exists for: Monaco's own editing surface is a hidden textarea, unlike the grid's cell editors, which really are form fields. Tests cover both directions — chip attached from the textarea surface, skipped from a nested input — and were verified to fail against the missing guard and against the INPUT+TEXTAREA variant. * fix(chat): persist the source names a selection chip renders from Cursor Bugbot: fileName/tableName were mapped into the optimistic message and accepted by the API schema, but PersistedMessageContext, buildPersistedUserMessage and toDisplayContexts never carried them. After a reload a file_selection chip fell back to its label for getDocumentIcon — and the label carries a location suffix ('notes.md:12-40'), so extension detection broke. Persists only the two names the display path reads. The rest of the payload (text, rowIds, columnIds, line numbers) stays unpersisted on purpose: it exists to resolve the selection server-side at send time, is never re-read when rendering a past message, and would put a selection-sized blob — up to the 20k char cap — in every stored message. Test asserts both halves of that, and was verified to fail with the mapping removed. * refactor(chat): remove a needless alias and correct an eslint-disable reason Self-audit for shortcuts, prompted by 'nothing hacky': - handlePaste kept `const prepared = preparedSelection`, an alias added only to avoid renaming two downstream lines. Uses the real name now. - The home.tsx drain's eslint-disable claimed handleContextAdd is 'a stable body function'. It is a body function, so it is a NEW value every render — the justification was false. Replaced with the actual reason: it is omitted to keep the drain one-shot, and doing so is harmless because consume() clears atomically, so a re-run would find nothing. Audited the rest of the diff for suppressions, casts and swallowed errors. The three catch blocks are documented graceful degradations with explicit fallbacks (row-drain failure, browsers rejecting a custom clipboard MIME mid-gesture, malformed clipboard JSON); the one double cast is a DataTransfer stub in a test. * fix(chat): bound the sync copy path by the text limit, not the chip cap Cursor Bugbot: writeLoadedRowsWithChip bailed once loaded rows exceeded MAX_TABLE_SELECTION_ROWS, but buildTableSelectionContext already slices rowIds to that cap. So selecting more than 500 loaded rows fell through to the async path, which cannot carry a custom MIME, and silently lost the chip — while Add to Chat on the very same selection still produced a 500-row chip. The two limits govern different things: the chip's cap is how many rows a table_selection can reference, the text's is TABLE_LIMITS.MAX_COPY_ROWS. Gate on the latter. Past it the paged path still takes over, because it owns truncation and the accompanying notice. * refactor(tables): extract selection-to-chip helpers into utils so they are testable Follow-up I owed on the previous round: the copy path's eligibility rule and the context builder were module-private in a ~4,600-line component with no test file, so the last two fixes to them were reasoned rather than covered — and both were wrong on the first attempt. Moves selectedColumnIds and buildTableSelectionContext to the existing table-grid/utils.ts (which already owns RowSelection, DisplayColumn and getColumnId), and extracts the copy decision as canWriteRowsWithChip. writeLoadedRowsWithChip keeps only the clipboard and toast effects, so the pure rule can be tested without a DOM. utils.ts stays free of side effects. New utils.test.ts covers the two limits that were conflated — a selection past the chip cap still qualifies (the context caps its own rowIds), one past MAX_COPY_ROWS defers to the paged path — plus the all-columns collapse and both caps. Verified to fail against the old chip-cap gate. * fix(chat): scope selections to the columns actually picked, and stop under-counting rows Three findings from one Bugbot round. Hidden columns widened cell ranges (reported twice). buildTableSelectionContext and contextMenuColumnIds collapsed a range to an open scope when it covered 'every column', comparing against displayColumns.length — which drops hidden columns AND expands workflow groups, so it never meant 'the whole schema'. Selecting every visible column therefore cleared columnIds and the server re-fetched columns the user had hidden. The collapse is removed rather than re-based on a schema count: no count available to a caller describes the schema, and an explicit column list is what the user actually selected. totalColumnCount is gone from the signature. Add-to-chat label undercounted rows. The menu derived its count from selectedRowCount (loaded rows only) while the chip was built from the full rowSel.ids set, so the label could promise fewer rows than were sent. Both now read one addToChatRowIds memo, with the count passed through explicitly since it legitimately differs from the count the delete/run labels use. Monaco line range was off by one. A full-line highlight ends at column 1 of the FOLLOWING line, so endLineNumber named a line that contributed no text — the chip label and the agent prompt both claimed an extra line. The collapse test I added last round asserted the buggy behavior as correct; it now pins the opposite, and fails if the collapse returns. * fix(tables): cap the Add-to-Chat label at the rows a chip can carry Cursor Bugbot: last round's fix for the label undercounting rows introduced the opposite error. addToChatRowCount passed the raw selection size, but buildTableSelectionContext caps rowIds at MAX_TABLE_SELECTION_ROWS, so a 2,000-row selection advertised 2,000 while the chip referenced 500 — breaking the same invariant the fix claimed to establish. Routes the count through a chipRowCount helper next to the builder that applies the cap, so the label cannot drift from the payload again. utils.test.ts now asserts the invariant directly rather than the formula: across 1, 42, 500, 750 and 50,000 requested rows, chipRowCount equals the rowIds length the context actually carries. Verified to fail if the cap is dropped. * fix(chat): stop a removed chip lingering when its label prefixes another Cursor Bugbot: the mention sync tests each label with a lookahead that rejects only word characters, so '-', ')' and space all let a shorter label match INSIDE a longer token. '@notes.md:12' matches within '@notes.md:12-40', and '@sales (3 rows)' within '@sales (3 rows) (2)'. Deleting the shorter chip left its context attached, and it was still sent with the message. The label class is pre-existing, but this PR made it routine: line ranges and uniqueContextLabel ordinals generate prefix pairs for any two selections of the same file or table. Fixed at the sync rather than by reshaping labels to dodge the prefix — a label format chosen to avoid a matcher bug would just relocate it. Contexts are now tested longest-label-first, and each matched token is blanked before shorter labels are tested, so every context is judged against text its own token owns. Blanked in place, not removed, so the (^|\s) boundary of whatever sits next to it is preserved; prev order is still what's returned. Shared with the workflow copilot input, so tests cover both directions: the two prefix pairs are dropped when only the longer token remains, both survive when both tokens are present, order is preserved, and trailing punctuation after a mention still keeps its chip. * fix(chat): include the line range in file-selection equality Cursor Bugbot: areContextsEqual compared only fileId and text for file_selection, while the comment directly above claimed equality was the selected range. A line that occurs twice in a file — a repeated import, a closing brace — highlighted at both places produced identical text, so prepareContextForInsert called the second a duplicate and dropped its chip, even though the labels (notes.md:12 vs notes.md:50) were plainly different. Comparing startLine/endLine as well makes the code match what the comment promised. The rich-markdown editor omits the range, so both sides are undefined there and identical text still dedupes — correct, since two identical passages are genuinely indistinguishable without line numbers. Tests cover both: distinct ranges stay distinct, an exact repeat still dedupes, and the no-line-number path still dedupes. Verified the first fails against text-only equality. * fix(tables): drain past the cap so exclusions can't shrink a select-all chip Cursor Bugbot: for a gutter select-all the menu count comes from selectedRowCount (capped), but the chip was built by loading exactly MAX_TABLE_SELECTION_ROWS and filtering exclusions AFTER. Any excluded row inside that prefix left the chip short of the advertised count. Third appearance of the same invariant — label vs payload — this time in the select-all path specifically, which the earlier fixes did not touch. Loads the cap plus the exclusion count, which covers the worst case where every exclusion falls inside the prefix, so the filtered result still reaches the cap whenever the table has the rows. Extracted as drainTargetForChip so the compensation is stated and tested rather than an inline arithmetic detail. --------- Co-authored-by: Waleed Latif <walif6@gmail.com>
…redentials to the executing workspace (#6167) * fix(security): validate cloud region/project inputs and bind vertex credentials to the executing workspace Two credential-exposure bugs on the Vertex AI path. 1. `vertexLocation` reached `new GoogleGenAI({ location })` unvalidated. The SDK interpolates that value straight into the API hostname (`https://${location}-aiplatform.googleapis.com/`), so a value like `attacker.tld/x` terminates the authority component and relocates the request — with the workspace's GCP bearer token attached by the auth client — to an arbitrary host. Reachable from agent/router/evaluator blocks and from `POST /api/guardrails/validate` with only a session cookie. `vertexProject` (interpolated into the URL path) and the Bedrock region (interpolated into the endpoint hostname) had the same shape of problem. Adds `validateGoogleCloudLocation` / `validateGoogleCloudProject` to the shared input-validation module and applies them, plus the existing `validateAwsRegion`, at the provider chokepoints. Every path to the SDK goes through `executeRequest`, so no caller can bypass them. `azureEndpoint` already routes through the DNS-pinning SSRF guard. 2. `resolveVertexCredential` enforced only the user↔credential predicate and never the workflow-workspace↔credential predicate that `authorizeCredentialUse` applies on the HTTP path. A credential held in workspace B could be pasted into a workflow in workspace A and consumed by workspace-A principals with no access to B — including on deployed runs, where `enforceCredentialAccess` is false and `ctx.userId` is the workflow owner rather than the trigger caller. Service-account credentials mint a `cloud-platform`-scoped token, so this handed workspace A the use of workspace B's GCP identity. The resolver now takes the executing `workspaceId` and rejects a credential belonging to a different workspace, mirroring `credential-access.ts`. All four executor call sites pass `ctx.workspaceId`. * fix(providers): normalize vertex location case before validating Hostnames are case-insensitive, so a mixed-case location like US-Central1 reaches Google today. Lowercase it before the region check rather than rejecting an input that currently works. * fix(security): accept AWS European Sovereign Cloud regions in validateAwsRegion eusc-de-east-1 is a real Bedrock-enabled region that the existing pattern did not cover, so applying the validator to bedrockRegion would have rejected a config that worked before. Adds the eusc-<country> partition.
…6173) Save is now always rendered (primary, disabled until there is something to save) and Discard only appears once there is something to discard. Previously sandboxes showed no action at all until dirty — so a new sandbox had no visible Create button — while skills always showed a disabled Save and no Discard. - saveDiscardActions moves to @/components/settings and owns the one rule - adds SettingsActionChip / SettingsActionChips so the settings shell and the ReactNode-slot headers render actions through the same chip path - skills, secrets, connected credentials, custom tools and the secrets manager drop their hand-rolled Save chips for the shared helper - credential-detail drafts seed once per credential instead of re-seeding from every refetch, which was acting as an implicit discard
) * fix(realtime): enforce room access continuously, not only at join File-doc, table, and workspace-list rooms authorized once at JOIN and never again, so a member whose workspace access was revoked or downgraded kept live collaborative write access — including durable Yjs document writes — for the whole lifetime of an already-open socket. The access-revalidation sweep explicitly skipped every non-workflow room. - sweep every room type, authorizing each against its own resource - share one membership policy (ROOM_MEMBERSHIP_ACTIONS) between the join check and the sweep, so a file-doc room keeps requiring write in both - gate file-doc document frames and table cell selections on the cached permission, evicting on a confirmed loss of access - re-check the cached decision before a join commits, so a join that authorized just before a revocation cannot re-enter the room - never let a join's own cache write clobber a revocation recorded mid-flight - surface room-access-revoked to clients; the file-doc editor falls back to read-only instead of accepting keystrokes that go nowhere * refactor(realtime): one shared eviction path for revoked room access The sweep and both per-frame gates each open-coded emit + leave + local-state cleanup. Route them all through evictSocketFromRoom so they cannot diverge on what eviction means; workflow keeps its historical access-revoked payload. * fix(realtime): re-check access before workspace-list room joins too The workspace-files / workspace-tables joins committed straight from their authorize result, so a join that authorized just before a revocation could put the socket back in a room the sweep had already evicted it from. Mirrors the guard the file-doc and table joins already had. * fix(realtime): order role-cache writes by read start, not write time Two authorizations can start in one order and finish in the other, so the decision written last can come from the older read. A join that authorized before a revocation but returned after the sweep's denial would bury it, handing the socket another full cache TTL of access. Every writer now takes a monotonic ticket before it queries and yields only to a later-started read. * chore(realtime): drop the test-only unguarded role-cache writer Tests can express the same setup with commitRoomPermission + a read ticket, so the cache has exactly one write path and no export without a production caller. * fix(realtime): keep handler-initiated table eviction retryable Evicting leaves the Socket.IO room synchronously, which is also how the sweep discovers work — so a presence removal failing in the per-frame path could never be retried and left a ghost collaborator until disconnect. Failed (or unconfirmed) removals now hand off to the sweep's existing cleanup lane instead of a second retry loop. * fix(realtime): re-resolve access at join commit instead of peeking the cache The pre-commit recheck peeked the role cache, which reports an EXPIRED entry as unknown and fails open — so a join stalled longer than the cache TTL could re-enter a room the sweep had already evicted it from, including a file-doc room where the next cold-cache frame is accepted as a durable write. All three joins now re-resolve the way the workflow join always has; it is normally a cache hit, since the join's own authorize just warmed it. * fix(realtime): keep the join generation guard after the access re-check The access re-resolve added in the previous commit sat AFTER the generation / superseded guard in the table and workspace-list joins, so a leave or a newer join landing during that await no longer cancelled the stale join — it would go on to leave the room the client had switched to and commit the abandoned one. The guard is now the last thing before the commit in all three handlers, as it already was for file-doc and workflow. * test(realtime): use the shared sleep helper in the new join tests check:utils bans the inline new Promise(setTimeout) form; the two stalled-join tests were the only new offenders. * fix(realtime): leave the prior table room only once the join is certain A table switch left the previous room before the access re-check ran, so a denial there aborted the join and left the client in no table room at all — silently dropped from one it may still be allowed to occupy. The leave now happens after the re-check, matching the file-doc and workspace-list joins. * fix(realtime): close the table join window between re-check and commit Moving the prior-room leave after the access re-check left Redis awaits between that check and socket.join, and superseded() only watches the join generation — so a sweep revocation landing in that window could still put a revoked socket back in the room. A synchronous cache peek immediately before the commit closes it without reintroducing the await; the authoritative resolve moments earlier wrote a fresh entry, so a differing read IS the revocation being guarded.
…ion (#6166) * fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion DocParser handed the raw upload straight to officeparser and then mammoth, both of which inflate every ZIP entry into memory before any app-level size cap applies. The extension is only a routing hint, so a bomb-bearing OOXML archive renamed to .doc selected the one parser that skipped the guard its docx/pptx/xlsx siblings all call. Adds assertOoxmlArchiveWithinLimits to DocParser.parseBuffer, and centrally in file-parsers parseBuffer so a future parser cannot silently opt out. The guard reads the central directory's declared sizes without decompressing, and no-ops for non-ZIP buffers, so legacy OLE .doc files are unaffected. * fix(file-parsers): verify actual inflation, not just declared ZIP sizes The declared uncompressed sizes in a ZIP central directory are attacker- controlled, so a bomb can under-report them and pass the size and ratio checks untouched. officeparser and mammoth only detect the mismatch after inflating the entry in full: a 498 KB archive declaring 1000 bytes per entry drove 559 MB resident through the .doc parser and 538 MB through .docx, then failed. SheetJS and officeparser reject the container earlier, so xlsx/pptx were not affected, but doc and docx both were. Each entry is now inflated during verification under a maxOutputLength bound equal to the size it declared. Node's zlib aborts the moment output would exceed that bound, so a lying entry costs only its declared size and the inflated bytes are discarded immediately; both bomb variants now reject at +0 MB across every extension. Stored entries are checked against their own compressed size, and unsupported compression methods fail closed. Verification walks the contiguous run of central-directory records rather than the EOCD's declared entry count, since that run is what a decompression library allocates per entry — a lied-down count must not hide an entry from verification. Cost is ~0.45 ms per MB of uncompressed content (22 ms for a 50 MB archive), against parse times an order of magnitude larger. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. * fix(file-parsers): require central and local ZIP headers to agree The parsers disagree about which header to trust. JSZip skips the local header outright and decompresses using the central directory's method, while SheetJS's parse_local_file switches on the local header's method and inflates from there. An entry claiming STORED centrally and DEFLATE locally therefore took the guard's stored branch, skipping bounded inflation, and was still expanded downstream — a 398 KB archive hiding a 400 MB deflate payload. Verification now rejects any entry whose two headers disagree on compression method, and on declared sizes when the local header carries them (the data-descriptor flag and ZIP64 sentinels legitimately omit them, and those entries stay covered by the bounded inflate). Caught by Greptile review. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. * fix(file-parsers): charge hidden central-directory entries against the cap sumDeclaredUncompressedSize walked only the entry count the EOCD declares, while verification walks the contiguous run of records. JSZip's readCentralDir loops on the record signature and keeps every entry it finds — a count mismatch is explicitly not an error there — so an archive that under-reported its count could hide honestly-large entries from the total-size cap and still have the parser expand them. The sum now walks the same contiguous run as the verification pass and readZipCentralDirectoryStats, and fails closed when the run is shorter than the declared count. Caught by Cursor Bugbot review.
* fix(realtime): stop read-only members persisting block positions The socket operation ACL granted the `read` role block.updatePosition and blocks.batchUpdatePositions on the premise that they were ephemeral cursor sync. They are not: both are followed by persistWorkflowOperation, which UPDATEs workflow_blocks.positionX/positionY and bumps workflow.updatedAt. A member holding only `read` on a workspace could therefore permanently rewrite the coordinates of every block of every workflow in it — a write across the read/write boundary. Live cursors ride their own `cursor-update` event, and the smooth-drag broadcast is the UNCOMMITTED position path, which returns before persisting and never consults the role table — so the read role needs no grant at all. * test(realtime): assert the role ACL against production, not a fixture The shared ROLE_ALLOWED_OPERATIONS fixture still listed the two position operations for the read role, and three tests compared that fixture against itself — so they certified whatever it said, including the grants this PR removes. They now assert checkRolePermission over the protocol's complete operation list (the fixture's copy omits subblock/variable/admin-only ops), plus one test that pins the fixture to the production ACL so the two cannot drift apart again.
The batch presign endpoint validated its type param against the shared seven-value upload enum while only authorizing knowledge-base, so any authenticated user could mint an S3 presigned PUT into workspace-logos, profile-pictures, execution, mothership, chat and copilot prefixes — objects that are then served unauthenticated from the app origin with a one-year public cache. The single presign endpoint had the same gap for chat, which has no authorization predicate and no client. - Batch presign now accepts only knowledge-base, and always requires a workspaceId the caller has write/admin on before anything is minted - Single presign drops chat from its accepted contexts; every remaining context has a per-context predicate - Both allowlists live in the contract module so the enforced enum and the documented one cannot drift apart again
* fix(copilot): guard document-style extraction against zip bombs extractDocumentStyle handed an attacker-controlled archive straight to JSZip and inflated named parts (word/theme/theme1.xml, word/styles.xml, ppt/presentation.xml, ppt/slideMasters/slideMaster1.xml) with no size bound, reachable from GET /api/workspaces/[id]/files/[fileId]/style and from the workspace VFS. Reading only a few entries is no protection: the bomb just has to live at one of those paths. It now calls assertOoxmlArchiveWithinLimits, the same guard the document parsers use, and the hand-rolled ZIP_MAGIC check is replaced by isZipShaped from that module — zip-guard is already shared this way by lib/uploads/archive.ts and lib/copilot/tools/handlers/upload-file-reader.ts. Checking each entry's size through JSZip instead would have been cheaper, since only a handful of parts are read, but JSZip reports the size the archive declares — the same attacker-controlled field a bomb lies about — so it would need to re-derive the guard's verification to be sound. The guard sits inside the existing try, so a rejection logs and returns null: the route already answers 422 and the VFS already returns null when no summary can be produced, and neither caller changes. * test(copilot): declare the ratio-test expansion instead of carrying it The compression-ratio case built a real 400 MiB string and deflated it synchronously, costing the parallel test runner memory and CPU for no added coverage. The guard reads the total the archive declares, so declaring the expansion exercises the same ratio path with a few-hundred-byte fixture. Still fails when the guard call is removed, and the file now runs in 286 ms instead of seconds. Caught by Greptile review.
* fix(copilot): make tool write gates fail closed The three handler-map management tools (manage_custom_tool, manage_mcp_tool, manage_skill) gated writes with `context.userPermission && perm !== 'write' && perm !== 'admin'`. userPermission is optional on the execution context, so an absent value skipped the check entirely and the write proceeded unguarded — while the server-tool router's equivalent gate is fail-closed. Both paths now share copilotToolCanWrite, built on the canonical permissionSatisfies (null/undefined never satisfies). manage_custom_tool additionally resolved its target as `params.workspaceId || context.workspaceId`, so a model-supplied workspace id won while the permission check was resolved for the context workspace — and upsertCustomTools performs no authz of its own. It now uses the server-set context only, matching its two siblings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CiHhAk2R1NryaS3R8n2yFz * fix(copilot): gate materialize_file writes and enforce the deploy mutation lock materialize_file's save/extract/import all create workspace resources, but the handler-map path has no central permission check, so a read-only member could create files and workflows through the agent. Gate on write access after param validation. assertWorkflowMutable moves into performFullDeploy / performFullUndeploy / performActivateVersion, where performRevertToVersion already had it. The check previously lived only in the deploy routes, so the copilot deploy tools — which call the orchestration functions directly — could deploy, undeploy, and activate versions of a locked workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(copilot): enforce secret-admin on env writes and gate KB indexing on usage Two more paths where the agent is a weaker route to the same write than the UI. upsertWorkspaceEnvVars was a weakened copy of the environment route's write: no per-key secret-admin check, no advisory lock, no audit row. Its only caller is the set_environment_variables copilot tool, which gates on workspace 'write' alone — so any write-level member could have the agent overwrite a workspace secret they do not administer, with nothing in the audit log and a lost-update race against the route's locked transaction. The gate now lives in the function so every caller inherits it, reusing getWorkspaceEnvKeyAdminAccess rather than restating the route's logic. knowledge_base add_file computed a billing attribution and then indexed without calling checkAttributedUsageLimits, which every upload route applies before accepting indexing work — an over-quota workspace could index without limit through the agent. The same file's query operation already gated correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(copilot): return lock denials and stop bootstrapping a legacy secret's ACL Two defects in the previous commits, both found by review. assertWorkflowMutable throws, and the three orchestration entry points awaited it outside any try/catch, so a locked workflow escaped as an exception instead of the { success: false } result the callers consume — performChatDeploy and the copilot deploy tools would have surfaced a generic 500 rather than a lock denial. performRevertToVersion already converted it; the three now do too, through a shared helper. upsertWorkspaceEnvVars derived newKeys for createWorkspaceEnvCredentials from the credential rows rather than the stored variables. A secret written before credential rows existed has no ACL, so overwriting it looked like adding a new key: it minted a credential and made the caller that secret's admin. The environment route derives newKeys from the locked jsonb read for exactly this reason, and now so does this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(env): collapse the merged test import onto one line Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm * fix(env): give the workspace env denial its own write-access message WorkspaceEnvAccessError reported "must be an admin of these secrets" for both denials, so a caller lacking workspace write to ADD a key was told to get secret-admin on a key that does not exist yet. Carry the reason and mirror the route's two messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCXbU36FwJBmtJKaH83BUm --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…rand chrome (#6178) * improvement(logfire): scope block outputs per operation and refresh brand chrome - gate each block output on the operations that actually return it - swap in the official Logfire mark, black tile with brand-magenta bare icon - move host to advanced mode and alphabetize the tool registry entries - add track-logfire-llm-cost and verify-logfire-token-target skills * fix(logfire): honor numeric-string limits and surface token validity fields - accept a numeric-string limit so agent-invoked calls stop silently falling back to Logfire's 100-row default - keep an hour-only UTC offset intact instead of producing +05Z - surface expiresAt and spendingCapReachedAt on Get Token Info - document pending_span as a fourth record kind * chore(logfire): regenerate tool metadata and document the step in the skill - regenerate apps/sim/tools/generated/tool-outputs.ts, which CI's tool-metadata:check requires after a tool output change - add the regeneration step and artifact-diff guidance to the validate-integration skill so the gate stops being missed * chore(skills): sync validate-integration projections
…members (#6180) * improvement(permissions): confine workspace role changes to existing members * improvement(permissions): retry lock timeouts and audit admin upserts truthfully Bounding the row-lock wait with `lock_timeout` made a competing writer raise 55P03, which the retry set excluded — so the bounded wait failed the request where the unbounded one would have waited. 55P03 is now retried like the other contention aborts, the timeout is shortened since each attempt releases its pooled connection, and contention that outlives every attempt answers with a busy conflict instead of a generic server error. The admin member upsert recorded MEMBER_ADDED even when the conflict branch amended an existing membership, putting a join that never happened in the workspace audit trail. It now records a role change, matching the action the response already reported. Adds the missing test coverage for withTransactionRetry. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
* fix(security): bind copilot chat attachment keys to their owner `POST /api/copilot/chat` accepted a client-supplied `fileAttachments[].key` and passed it straight into `trackChatUpload`, which wrote it into the `workspace_files` ownership binding with no permission check and no key-ownership validation. That binding is what `verifyFileAccess` and the Files feature resolve authorization from, so any workspace member — including read-only — could hand in another member's key and have their file re-parented to a private chat: removed from the Files listing, from folders and from download-by-id, and destroyable through the chat-delete FK cascade. The sibling register route already enforced these invariants; the copilot path did not. - `trackChatUpload` now rejects keys that do not address the target workspace, only re-links a chat-upload row the caller already owns (matched by row id, not by raw key), and only mints a new binding when the key has no prior record at all — including soft-deleted ones, which the partial active-key unique index would otherwise let it insert over. - Minting a new binding verifies the object exists in storage, matching `registerUploadedWorkspaceFile`. - `buildCopilotRequestPayload` gates attachment tracking on write/admin, the same grant the upload routes that issue these keys require. * fix(security): make the chat-upload ownership check atomic with its write The ownership lookup and the binding UPDATE were separate statements, and the UPDATE matched on the captured row id alone. A concurrent `materialize_file` sets `context='workspace'` and clears `chatId` on that same row, so the tracking write still matched and dragged the saved file back into chat scope — hiding it from every workspace-file listing and re-exposing it to the chat-delete cascade, with materialize's storage-usage increment left stranded. Re-assert every ownership predicate in the UPDATE so the statement is itself the atomic check. The lookup now only decides UPDATE-vs-INSERT and is no longer load-bearing for authorization, which also makes the existing `updated.length === 0` fail-closed branch correct rather than dead. * fix(uploads): tolerate transient storage probes and reject unowned keys with 403 Follow-ups from a backward-compatibility audit of the attachment-key hardening. The existence probe is hygiene, not authorization — the key-format and no-prior-record guards already carry that, and a binding to a nonexistent object grants nothing readable. But `headObject` rethrows non-404 provider errors, so a transient 5xx or throttle would drop a legitimate attachment. Only a definitive not-found now rejects; a thrown error logs and proceeds on the ownership guards. This path is reached solely by >50MB multipart uploads, the one flow that persists no metadata row at upload time. The stage route mapped an ownership rejection to a 500. It is a client error; return 403 instead. * fix(security): compare-and-swap the chat binding on chat uploads Two overlapping chat requests could both observe the same claimable row with `chat_id IS NULL` and both satisfy the update predicate, so the later write silently moved the upload to its own chat — taking over the delete-cascade lifecycle of a file the first chat had already bound. Scope the update to `chat_id IS NULL OR chat_id = <target>` so the statement is a compare-and-swap: the loser matches zero rows and fails closed. The resolver applies the same rule so the update-vs-insert decision stays coherent. This also makes an upload bind to exactly one chat, matching the 409 the sibling `local-files/stage` route already returns for the same case; verified no client flow relinks a key across chats (drafts are per-chat, every retry path replays under a pinned chat id, forking copies blobs to fresh keys). * refactor(copilot): gate attachment tracking with the shared permission predicate `permissionSatisfies` is the documented single source of truth for permission comparisons and exists to replace hand-written `=== 'admin' || === 'write'` ladders. `userPermission` is typed `string` for legacy reasons, so narrow it with `isPermissionType` first — an unrecognized value fails the gate instead of ranking below every level. Behavior is unchanged for all three levels. Imported from the dependency-free `/predicates` subpath rather than `/workspace`, which would pull `@sim/db` onto the chat request path.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Realtime access no longer trusts join-time checks alone. The access revalidation sweep now covers every room type (workflow, table, file-doc, workspace invalidation), evicts on revocation or downgrade, and adds per-frame gates so mid-session loss of write access stops durable edits and presence updates. Shared room eviction handlers reconcile handler-local state; joins re-resolve permissions before committing so a stale allow cannot override a sweep denial. Read-only workspace members are denied all persisted socket operations (including committed position updates); uncommitted drag sync still relays without persisting. Secrets in logs: docs and user-facing copy now describe execution log protection—successful Tools / client bundle: agent skills and commands document Other highlights (from the broader release): span sanitization for function/agent traces; SSH/SFTP and Docs-only deltas in this diff also refresh Logfire integration tables ( Reviewed by Cursor Bugbot for commit 797f781. Configure here. |
Collaborator
Author
|
@greptile-apps review |
Collaborator
Author
|
@cursor review |
Contributor
Greptile SummaryThis release combines security and authorization hardening with chat selection contexts, execution/logging improvements, generated tool metadata, settings UX updates, and file-processing safeguards.
Confidence Score: 5/5The PR appears safe to merge; no concrete changed-code defect remained after accounting for intentional behavior and base-branch provenance. The reviewed authorization, ownership, bounded-read, parser, execution, metadata, and chat-context changes preserve their surrounding contracts, and the only observed realtime fail-open interval is explicitly documented and tested as an intentional tradeoff.
|
| Filename | Overview |
|---|---|
| apps/realtime/src/handlers/tables.ts | Adds cached per-event table authorization and eviction behavior; the deliberate cache-miss acceptance window is documented and tested. |
| apps/realtime/src/access-revalidation.ts | Adds periodic room-access revalidation so revoked collaborators are evicted from long-lived realtime sessions. |
| apps/sim/app/api/files/presigned/route.ts | Restricts accepted upload contexts and applies context-specific authorization before issuing presigned URLs. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Atomically binds chat uploads to their owner, workspace, context, and chat to prevent cross-request claims. |
| apps/sim/executor/utils/vertex-credential.ts | Binds Vertex credentials to the workspace executing the workflow while retaining actor-level credential checks. |
| apps/sim/lib/copilot/chat/process-contents.ts | Materializes bounded file and table selections into chat context while enforcing workspace containment. |
| apps/sim/tools/metadata.ts | Provides registry-free generated metadata lookup with version resolution and own-property guards. |
| apps/sim/lib/file-parsers/zip-guard.ts | Adds archive expansion limits used to prevent document parsing from exhausting memory. |
| apps/sim/lib/logs/execution/trace-secret-projection.ts | Projects resolved secrets into trace sanitization so function and agent spans do not expose credentials. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
User[Workspace user] --> UI[Files, tables, chat, settings]
UI --> API[Next.js API routes]
UI <--> RT[Realtime service]
API --> Authz[Workspace and ownership checks]
RT --> Revalidation[Continuous room-access revalidation]
API --> Uploads[Bounded and context-authorized uploads]
API --> Copilot[Copilot payload and tools]
Copilot --> Selection[File and table selection context]
API --> Executor[Workflow executor]
Executor --> Sanitization[Trace secret projection and sanitization]
Executor --> Logs[Operation-scoped logs and outputs]
Executor --> Metadata[Generated tool metadata]
Reviews (1): Last reviewed commit: "chore(deps): bump @opentelemetry/core to..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 797f781. Configure here.
* fix(chat): show deployment passwords to admins
* fix(chat): reject whitespace-only passwords
* fix(chat): preserve password visibility on regenerate
* fix(chat): harden password reveal and close deployment lockout paths
Follow-ups from a security review of the password reveal endpoint. The
permission model itself was correct — the reveal is gated on workspace
admin via the canonical resolver, so derived org-admin access is honored.
These address secret handling and validation around it.
- Cap set-path passwords at the same 1024 chars the chat login accepts.
Neither the input nor the schema bounded length, so a longer password
saved fine and then failed the login POST on length before auth ran,
locking every visitor out permanently.
- Discard the revealed password when the field is hidden. It previously
stayed in state and in the input's DOM value with Copy still armed, so
the field read as hidden while still handing out the plaintext.
- Evict the decrypted password from the mutation cache on unmount, and
correct the TSDoc claiming it was never retained — it sat in the
MutationCache for the default five minutes after the modal closed.
- Validate the password inside performChatDeploy, the writer both callers
must use. The copilot deploy_chat tool bypasses the route contract and
could still store a whitespace-only or over-long password, or create a
password-protected chat with no password at all.
- Stop echoing raw decryption errors from the reveal endpoint.
- Only persist a new password when the chat ends up password-protected;
PATCH { authType: 'email', password } used to re-arm the secret that the
auth-type branch had just cleared.
Also replaces the hand-rolled copy state with useCopyToClipboard, which
fixes an unawaited clipboard write that surfaced as an unhandled rejection
and a "Copied" confirmation shown even when the write failed.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(chat): correct password-change confirmation gate and stale reveal error
Addresses both open Bugbot findings.
- shouldConfirmPasswordChange keyed on "a chat exists" rather than "a password
exists", so switching a public chat to password protection for the first time
asked the admin to confirm changing a password that was never set. It now
takes the existing-password signal the component already computes.
- A failed reveal left "Failed to load the current password" on screen while the
admin typed or generated a replacement, because the mutation only drops its
error on the next attempt. Editing or regenerating now resets it.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
Co-authored-by: Claude <noreply@anthropic.com>
…tions (#6187) * fix(resume): stop a failed run-buffer publish from stranding a resumed execution The run buffer is a replay convenience for stream readers; the durable execution record is authoritative. A failed terminal event publish was deciding the outcome of work that had already run: it threw, the resume was marked failed, and the paused execution was left paused forever. That path is also not retryable — the workflow had already executed — so the next resume attempt re-ran its side effects. Degrade instead. Record the terminal status on the stream meta so readers are not left polling an 'active' stream, log the failure, and let the resume settle on its real result. The same treatment applies when the terminal event is never published at all, which previously synthesized an error for the same stranding effect. Removes the now-unreachable TERMINAL_PUBLISH_ERROR constant and its branch. Pre-execution buffer failures stay fatal and retryable, since no work has happened yet. * fix(execution): give per-user Redis budget keys a fixed window The per-user byte budget refreshed its TTL on every accepted write, so for any user who never went a full TTL without writing, the key never expired. The per-execution data it accounted for kept expiring underneath it, so the counter accrued bytes Redis had already dropped and drifted toward the ceiling. On reaching it, every subsequent write for that user was rejected until they stopped writing for a full TTL — and since a rejected write does not refresh the TTL, it recovered on its own and then refilled. User keys now get a fixed window: the TTL is set when the key is created and never extended. Applied to all five Lua scripts that touch the key so the writers cannot drift apart. Execution-scoped keys keep sliding — they are refreshed on the same schedule as the data they account for, so their counter and bytes stay in step. * fix(execution): heal a user budget key that somehow has no expiry The no-op branch of the flush script dropped the user-key EXPIRE outright rather than guarding it like every other site. No current path can create the key without an expiry, but if one ever did, that branch was the one place that would never give it one — and a user counter with no expiry is the unbounded version of the bug this series fixes. Guard it instead, so the branch heals such a key rather than skipping it, and so all five scripts read the same way. * fix(stream): end a replay cleanly when terminal metadata has no terminal event Terminal metadata is the authoritative end-of-run signal — the happy path writes it atomically with the terminal event, and a run whose terminal event could not be buffered records the status on its own. The reader required both, so the degraded case threw and turned a replay that was merely incomplete into a broken one. Metadata without a matching event means the buffer degraded, not that the run is still going. Log it and close the stream: the reader has already received every event that was buffered, and the durable record is unaffected either way.
* feat(zoho-desk): add Zoho Desk integration
Add a full Zoho Desk integration: tools, block, icon, and a webhook trigger.
Tools (tools/zoho_desk): list/get/update tickets, list/add comments,
list/get threads, get contact, list organizations, and download attachments
as UserFiles via an internal route. Registered in tools/registry.ts.
Block (blocks/blocks/zoho-desk.ts): operation dropdown, OAuth credential,
an organization selector backed by GET /organizations, per-operation fields,
and BlockMeta templates. Wires the Zoho Desk trigger.
OAuth (zoho-desk provider): authorize/token at accounts.zoho.com with
access_type=offline + prompt=consent; the Desk REST base is derived from the
token response api_domain and persisted so calls honor data residency instead
of assuming desk.zoho.com. Every call sends Authorization: Zoho-oauthtoken and
the orgId header.
Trigger + webhook handler (triggers/zoho_desk, lib/webhooks/providers/zoho-desk.ts):
Sim creates and tears down the Zoho Desk webhook subscription. Inbound events
are verified with JWT RS256 (X-ZDesk-JWT) against the data-center JWKS, ACKed
via the durable queue to meet Zoho's 5s deadline, and fail loudly on
Free/Standard editions that cannot create webhooks.
* fix(zoho-desk): OAuth PKCE, DC scope-marker parsing, SSRF, and e2e fixes
OAuth: forward code_verifier in the custom getToken (PKCE is enabled, so the
exchange must echo the verifier or Zoho rejects the request with invalid_request).
Surface Zoho's error/error_description, which it returns in the JSON body with
HTTP 200, instead of collapsing every failure into "no access token".
Data-center base parsing: better-auth persists Zoho's scopes comma-joined with no
spaces, so the greedy \S+ marker regex swallowed the whole scope list into the
host. Stop the capture at a comma or whitespace in both read sites (token route
and webhook handler), so apiDomain resolves to the real Desk host.
Attachment SSRF: replace the permissive host regex (which accepted attacker
domains like zoho.attacker.com) with a strict Zoho-apex suffix allowlist.
Block: guard Number() pagination so a non-numeric typo can't send NaN; add the
ignoreSourceId -> sourceId loop-guard header to update_ticket (matching add_comment).
Organizations route: surface fetch/Zoho failures with a real status instead of a
200 with an empty list, so the org selector no longer fails silently.
* fix(zoho-desk): webhook creation, attachment naming, and HTML content handling
Webhook trigger (verified end-to-end against a live Enterprise org):
- Omit ignoreSourceId; Zoho rejects a non-Zoho UUID with INVALID_DATA. Drop
the generateId() fallback and its providerConfig persistence.
- Answer Zoho's create-time notification-URL probe via the existing pending
webhook verification mechanism (GET/HEAD matchers) so subscription creation
no longer 405s.
- mapZohoWebhookError now surfaces Zoho's real errorCode / message / field
errors instead of a catch-all edition message, and attaches an HTTP status so
4xx flow through NonRetryableDeploymentError while 429/5xx stay retryable.
- Propagate the real status through deploy.ts so failed creates don't retry-loop.
get_attachment polish:
- Return the downloaded file's name under `name` (ToolFileData key) instead of
`filename`, and derive it (explicit -> Content-Disposition -> URL segment ->
fallback) so attachments are no longer stored as "untitled".
- Gate the add_comment-only `contentType` param so it isn't sent to get_attachment.
HTML content handling (Zoho content fields emit raw HTML):
- Add a Zoho-local html-to-text converter mirroring the Outlook dual-field
pattern: when contentType is 'html', derive a plain-text `contentText`
alongside the untouched raw `content` + `contentType`; plainText mirrors.
- Apply to comments (list/add), threads (list/get), the ticket description
(descriptionText), and the webhook trigger payload.
Trigger org selector: Organization is now a credential-scoped combobox that
lists the connected account's Zoho Desk organizations.
* fix(zoho-desk): review round - DC-base derivation, org-loader resilience, batched-event visibility
- deriveZohoDeskBaseFromApiDomain: preserve an already-regional desk.zoho.<tld>
api_domain instead of falling back to the US (.com) data center, and map the
DC TLD from any zoho(apis).<tld> host - keeps Desk calls in the right data
center for residency.
- fetchZohoDeskOrganizationOptions: wrap the token/org fetch in try/catch and
degrade to an empty list (the org field is a free-text combobox, so manual
entry still works) instead of hard-failing the selector on token/DC/network
errors.
- formatInput: warn (not silently drop) if Zoho ever delivers more than one
event in a single payload.
* fix(zoho-desk): harden attachment download against redirect-based SSRF/token leak
Replace the raw fetch in the attachment route with secureFetchWithValidation
(the same guarded fetch the copilot file-download tool uses). The download URL
is user/LLM-influenced and Zoho may redirect, so auto-following redirects could
send the OAuth token / orgId to an untrusted or internal host. The guarded fetch
pins the resolved IP, blocks private/reserved targets on every hop, drops the
Authorization header if a redirect leaves the origin (stripAuthOnRedirect), and
enforces the 50MB cap while streaming. The strict Zoho apex allowlist still
gates the initial origin as defense in depth.
* fix(zoho-desk): only add the edition hint when Zoho's error indicates it
mapZohoWebhookError appended the "requires Professional edition or higher"
guidance to every 403, but a 403 can also mean a wrong org, a missing scope, or
a bad token. Gate the hint on Zoho's own errorCode / message matching the
permission/edition pattern instead of the bare status, so unrelated 403s surface
Zoho's real reason without the misleading suffix. Adds a test for the
non-edition 403 path.
* fix(zoho-desk): stop duplicating /api/v1 when resolving a relative attachment href
A relative attachment href that already starts with `api/v1` (as Zoho's hrefs
often do) was concatenated onto getZohoDeskApiBase (which ends in /api/v1),
producing `/api/v1/api/v1/...` and a failing download. Extract a tested
resolveZohoAttachmentUrl helper that uses absolute hrefs as-is and strips a
leading slash + `api/v1/` prefix from relative ones before joining, so the path
is correct for absolute, root-relative, and api/v1-prefixed hrefs alike.
* fix(zoho-desk): reject an empty update_ticket PATCH with a clear error
update_ticket built its PATCH body from optional fields via filterUndefined, so
a call with no fields set sent `{}` and surfaced an opaque Zoho failure. Guard
the body builder to throw an actionable "provide at least one field" error
before the request. Adds a test for the empty and populated body paths.
* fix(zoho-desk): fall back to the credential Desk domain in webhook JWT verify
verifyAuth chose the JWKS host from providerConfig.apiDomain and otherwise
defaulted to the US host (desk.zoho.com), so a non-US webhook row missing
apiDomain would verify against the wrong JWKS and reject legitimate events. When
apiDomain is absent, resolve it from the OAuth credential's __zoho_domain__ scope
marker (mirroring deleteSubscription). The persisted-apiDomain fast path stays
DB-free to respect the 5s delivery deadline. Adds tests for both paths.
* fix(zoho-desk): apply the Zoho host allowlist to the organizations route
The organizations route built its URL from the client-supplied apiDomain and
attached the OAuth token without the https-Zoho-host allowlist the attachment
route already enforced, so a session-access caller could point the server at an
arbitrary origin and leak the token. Extract the shared isZohoHost allowlist and
an assertZohoUrl guard into tools/zoho_desk/utils (two consumers now), guard the
organizations URL before fetching, and refactor the attachment route to reuse
the shared helper. Adds tests for the allowlist and guard.
* fix(zoho-desk): propagate provider 4xx in the stable webhook prepare path
The v2 stable deploy preparation flattened every registration failure (except
path conflicts) to HTTP 500, so a provider-attached permanent 4xx - e.g. Zoho's
edition/validation failures from createSubscription - retried instead of failing
the deploy terminally. Propagate the attached status (`?? 500`), matching the
legacy save path's status-aware mapping so both deploy paths route 4xx through
NonRetryableDeploymentError.
* fix(zoho-desk): make createSubscription config failures non-retryable
createSubscription threw plain Errors (no status) for missing orgId, event type,
or credentials, and for a Zoho success with no webhook id - so the deploy outbox
mapped them to 500 and retried permanent configuration failures. Attach a 4xx
via statusError (400 for missing config/credentials; 422 for the no-id anomaly,
where a retry risks duplicate webhooks) so they fail the deploy terminally like
the mapped Zoho API 4xx responses. Tests assert the 400 status on the guard paths.
* fix(zoho-desk): enrich prevState with contentText symmetrically with payload
formatInput derived plain-text contentText only on payload, so an update event
for a comment/thread left prevState as raw HTML while payload carried
contentText - inconsistent shapes for before/after comparisons. Apply
withDerivedContentText to prevState too. Test asserts both are enriched.
* docs(zoho-desk): regenerate integration docs
Regenerate zoho_desk.mdx from the current tool definitions: removes the stale
add_comment `ignoreSourceId` input row (the field was dropped because Zoho
rejects arbitrary values) and adds the derived `contentText` / `descriptionText`
plain-text fields on comments, threads, and tickets.
* fix(zoho-desk): validate the persisted Desk base against the strict host allowlist
deriveZohoDeskBaseFromApiDomain trusted any host matching `desk.zoho.[a-z.]+`,
so a crafted api_domain like `desk.zoho.com.attacker.com` passed and was
persisted as the credential's `__zoho_domain__` REST base - later receiving the
OAuth token on every Desk tool/webhook call. Gate the derivation on the strict
isZohoHost apex allowlist (which rejects that lookalike), extracted with
assertZohoUrl into a dependency-free host-allowlist module so the auth
token-exchange path validates hosts without pulling in the tool utilities. The
attachment and organizations routes now import the shared guard from there.
Also: formatInput now emits the normalized null trigger shape for an empty/
malformed event array instead of leaking a raw `[]` to downstream steps. Tests
cover the empty-array shape and the lookalike-host rejection.
* fix(zoho-desk): correct API field names, scopes, and host validation
Validation pass against Zoho's published Desk API surfaced six defects that
typecheck, lint, and the existing suite all passed over, because each one fails
silently against the live API rather than erroring.
Wire-name mismatches (Zoho ignores unknown keys, so all three were silent):
- update_ticket sent `customFields`; the ticket PATCH body names it `cf`.
`customFields` exists only as a deprecated alias on other Desk resources and
on the separate validate-field-updates endpoint, so updates reported success
and applied nothing.
- ZOHO_DESK_TICKET_PROPERTIES and ZOHO_DESK_CONTACT_PROPERTIES advertised a
`customFields` output; both resources return `cf`. The declared field always
resolved undefined and the real one was undeclared.
- list_tickets sent `departmentId`; the query param is `departmentIds`, so the
department filter was dropped and every department's tickets came back.
Content handling:
- deriveZohoContentText matched `contentType === 'html'`, but Zoho spells the
discriminator per resource: comments use `html`, threads use the MIME form
`text/html`. Every thread's `contentText` was therefore raw markup - the exact
opposite of the field's purpose. Now normalized across both spellings,
parameterized values, and casing, with regression tests.
Scopes (least privilege):
- Desk.tickets.ALL -> Desk.tickets.READ + Desk.tickets.UPDATE. No tool creates
or deletes a ticket; ALL additionally granted ticket DELETE.
- Dropped Desk.search.READ (no search tool exists) and Desk.webhooks.READ /
.UPDATE (the provider only creates and deletes), plus their orphaned
SCOPE_DESCRIPTIONS entries.
Host validation - the webhook provider was the only token-carrying path not
anchored to the Zoho apex allowlist, including the JWKS fetch, where an
unrecognized host would have stood in as the JWT issuer:
- createSubscription, deleteSubscription, and verifyAuth now route their base
through a shared allowlist check.
- getZohoDeskApiBase validates rather than trusting injection precedence.
- The organizations route uses secureFetchWithValidation with
stripAuthOnRedirect, matching the attachment route it had diverged from.
Block and trigger:
- The trigger's department field is renamed `triggerDepartmentIds`; sharing the
`departmentIds` id let a value typed as a list_tickets filter become the
webhook subscription's filter when switching modes.
- `isPublic` no longer serializes onto all ten operations, matching the existing
gating for `contentType`.
- from/limit reject negatives and fractions instead of forwarding them.
- update_ticket gains description, resolution, and classification (all already
declared as outputs), and a departmentId input so a ticket can be moved.
Accuracy corrections to user-facing text, all against the published parameter
tables: `from` is 0-based (0-4999, default 0), not 1-based; per-endpoint limits
are tickets 1-100/10, comments 1-100/50, threads 1-200/100; sortBy lists Zoho's
actual allowed values; the two `include` sets genuinely differ per endpoint;
status and priority accept comma-separated lists.
Also: path IDs are trimmed via requireZohoDeskId so a pasted trailing space
fails with a clear message instead of a %20 404; comment `commenter` and thread
`status`/`isDescriptionThread`/`visibility`/`canReply` are now declared;
ZOHO_CLIENT_ID/SECRET added to the oauth test env; docs page gains a
MANUAL-CONTENT intro covering capabilities, the Professional-edition webhook
requirement, and the US-data-center limitation.
Not verified from documentation, needs a live account before merge:
- the OAuth scope for the attachment content sub-path (Zoho publishes none, and
there is an unanswered SCOPE_MISMATCH report against it)
- 12 of the 17 offered webhook event ids (5 are confirmed); Ticket_Delete is
documented but not offered
- the ticket `descriptionContentType` key, and the POST /api/v1/webhooks body
shape, neither of which appears in any reachable Zoho reference
* chore(zoho-desk): regenerate tool metadata
The param and description corrections in the previous commit changed the
generated tool surface, so tool-metadata:check failed in CI. Regenerated;
the diff is two Zoho-only lines.
* fix(zoho-desk): stop posting null for untouched update_ticket fields
`filterUndefined` strips only `undefined`, but an untouched subBlock never
arrives as `undefined`: the workflow serializer initializes every subBlock value
to `null` (stores/workflows/utils.ts) and extractBlockParams writes those nulls
straight into tool params, with nothing between the serializer and request.body
filtering them.
Reproduced against the real serializer and block with only `status` set:
basic {"subject":null,"status":"Closed"}
advanced {"subject":null,"status":"Closed","priority":null,...,"cf":null}
`subject` leaks even in basic mode because it declares no `mode`, so
shouldSerializeSubBlock never drops it. Zoho documents subject as a writable
field, so every status-only edit either failed the PATCH or blanked the ticket's
subject; in advanced mode the whole update surface nulled out, including `cf`.
Two things hid this. The empty-PATCH guard was unreachable from the block (the
body always carried at least `subject`), and the existing test called buildBody
with fields *absent* rather than null - the shape the block never produces - so
it could not fail on the real path.
Replaces filterUndefined with a local omitUnset that drops undefined, null, and
'' (a cleared input means "leave unchanged", not "set to empty"). Adds three
tests using the real serializer shape, all verified to fail before the fix.
Also fixes the same null-blindness in the block's param mapping, where
Number(null) === 0 injected from=0 on every operation, and corrects the shared
limit placeholder, which claimed max 100 while list_threads allows 200.
* feat(zoho-desk): add Self Client service-account credential
Adds a second way to connect Zoho Desk, alongside the interactive OAuth flow: a
Zoho Self Client, pasted as client id + client secret + organization id. Built
on the existing client-credential-accounts framework rather than a new credential
path, so it behaves like the Zoom Server-to-Server and Box CCG accounts already
in the repo - a short-lived token minted on demand, no refresh token.
Two Zoho behaviors the generic framework does not cover:
- `scope` must be COMMA-separated on Zoho's token endpoint; a space-separated
list is rejected as an invalid scope. The list comes from
getCanonicalScopesForProvider('zoho-desk'), so the Self Client and the OAuth
flow can never drift apart on scopes.
- Zoho reports OAuth failures in the JSON body, frequently with HTTP 200
(e.g. {"error":"invalid_client"}), so the success body is inspected for an
`error` field before the token is read - a status-only check would accept a
failed mint.
deriveZohoDeskBaseFromApiDomain moves out of auth.ts into the dependency-free
host-allowlist module so the minter and the OAuth path share one derivation
instead of duplicating it, and the mint response's api_domain now flows through
to tools as `apiDomain` (the SA branch of the token route previously returned
none, so SA calls would have assumed desk.zoho.com).
Docs: hand-authored zoho-desk-service-account.mdx following the existing
*-service-account.mdx pages, registered in meta.json and in the generator's
keep-list so stale-page cleanup does not delete it.
Known limitation, documented in the descriptor helpText and the docs page:
webhook triggers still require an OAuth connection. Webhook provisioning resolves
credentials through getCredentialOwner/refreshAccessTokenIfNeeded, which is
OAuth-account-only for every provider in the repo - not a Zoho-specific gap.
Unverified from documentation, needs a live Zoho org before merge:
- the `ZohoDesk.` soid prefix. Zoho documents only the syntax
{servicename}.{zsoid} with a single CRM example; no first-party doc states the
Desk prefix. normalizeZohoDeskSoid passes through any value already containing
a '.', so an operator can paste a corrected full soid without a code change.
- whether zsoid is the same identifier as the Desk orgId header value.
- whether the client-credentials endpoint accepts Desk.webhooks.CREATE/DELETE
for a Self Client.
- whether the mint response populates api_domain for Desk (documented for CRM);
if absent the derivation falls back to the US Desk host.
* fix(zoho-desk): derive descriptionText for ticket-shaped payloads
Cursor Bugbot: webhook ticket events reached workflows as raw HTML with no
plain-text sibling. `withDerivedContentText` only looked at `content` /
`contentType`, but ticket resources carry their body on `description` /
`descriptionContentType`, so trigger output disagreed with get_ticket.
The helper now derives both, which also removed two inconsistencies on the tool
side: get_ticket had its own inline copy of the derivation (now one shared
implementation that cannot drift), and update_ticket returned its PATCH response
raw despite the shared output map declaring descriptionText.
`descriptionContentType` remains the one field name unconfirmed in any Zoho
reference. It degrades safely - an absent key makes deriveZohoContentText return
the value unchanged, so descriptionText mirrors description rather than breaking,
exactly as get_ticket already behaved - and it is now one helper to correct if
Zoho names it differently.
* feat(zoho-desk): let the service account pick its data center
Zoho's accounts server is per region, and the integration pinned every call to
the US host. For the interactive OAuth flow that is currently unavoidable -
better-auth's authorize/token URLs are static per provider - but the service
account mints its own token, so the region can simply be chosen. This makes the
Self Client the only way a non-US Zoho org can connect.
Adds an optional `dataCenter` field to the client-credential framework. Optional
matters: ClientCredentialAccountFieldId and ClientCredentialAccountFields are
shared with Zoom, Box and Salesforce, whose descriptors and minters are
unchanged. Blank keeps the previous behavior (US), so existing credentials are
unaffected.
Only us/eu/in/au are offered - the four regions where both the accounts server
and the Desk REST host are confirmed. CA is deliberately absent: Zoho's accounts
docs say accounts.zohocloud.ca while Zoho's own Desk SDK says accounts.zoho.ca,
and the two cannot both be right. JP/SA/CN/UK lack a confirmed Desk host.
The Desk base is now derived from the selected region rather than inferred from
the mint response, which also removes a dependency on `api_domain` being
populated for Desk (Zoho documents it for CRM only). When `api_domain` IS present
and disagrees with the region, it wins - it is authoritative about where the
token actually works - and the mismatch is logged so a mis-selected region is
diagnosable. deriveZohoDeskBaseFromApiDomain gains a `try` variant returning
undefined so an untrusted api_domain can no longer masquerade as an authoritative
US answer and silently override a correct region.
A wrong region fails loudly rather than silently: the minter runs as verification
on both create and reconnect, so the credential is never persisted in a broken
state. Because Zoho reports it as `invalid_client` - a Self Client only exists on
its own region's accounts server - the operator hint for that code now names the
data center as a candidate cause.
Copy is scoped per path rather than blanket "US only": the OAuth service
description, trigger setup instructions, and the docs intro now say which path
each limitation applies to, and the service-account page documents the four
regions with a sign-in-domain to region-code table.
* fix(zoho-desk): strip ticket description HTML, classify body-reported refresh failures
Final validation pass findings.
descriptionText never stripped anything. It was gated on a
`descriptionContentType` discriminator that Zoho does not send: the Ticket_Add
webhook sample ships `"description": "<div>Description</div>"` with no such key,
and the ticket GET/PATCH response field lists have no content-type sibling
either. So get_ticket, update_ticket, and every webhook ticket payload emitted
descriptionText as a byte-identical copy of the raw HTML, while the declared
output promised stripped text.
The tests did not catch it because they fabricated the shape - both fixtures
constructed `descriptionContentType: 'html'`, a key Zoho never emits, proving the
branch works without proving it is ever taken. Ticket descriptions are HTML by
convention, so the strip is now unconditional (html-to-text is a near-identity on
genuinely plain text), an explicit descriptionContentType is still honored if
Zoho ever adds one, and the fixtures now use Zoho's real shape with no
content-type key anywhere.
A body-reported refresh failure was unclassified. Zoho answers a revoked refresh
token with HTTP 200 and `{"error":"invalid_client"}`; refreshOAuthToken only
checked `data.ok === false` (a Slack-ism), so the request fell through to the
"no access token" guard and returned no errorCode. isTerminalRefreshError could
therefore never recognize invalid_client as terminal, the credential was never
marked dead, and every later execution retried a refresh that cannot succeed -
with the user shown "No access token in refresh response" instead of a reconnect
prompt. The body is now classified before the status is trusted, matching what
the token exchange and the service-account mint already did. That guard also
stopped logging the whole response body, which carries live tokens on a partial
success.
Also: an unrecognized dataCenter now fails with a named error instead of quietly
resolving to US and surfacing as an opaque invalid_client (blank still means US);
the webhook JWKS cache is bounded, since its key derives from a providerConfig
field that SYSTEM_MANAGED_FIELDS protects from diffing but not from being
written; and the attachment `size` output no longer asserts bytes, a unit Zoho
documents as KB.
* feat(zoho-desk): canonical selectors and BlockMeta skills
The block picked its organization with an ad-hoc `combobox` + `fetchOptions`.
Only five blocks in the repo did that, and the other four are core blocks
(agent/credential/function/logs) - no other OAuth integration used it. Every
other resource a user has to identify was a bare short-input taking an opaque
numeric id.
Zoho Desk now uses the same machinery as the other 25 selector providers:
hooks/selectors/providers/zoho-desk/selectors.ts registered in the selector
registry, consumed from the block as basic selector + advanced manual input
sharing one canonicalParamId, for organization, update-ticket department, and
the list-tickets department filter. The trigger's org field moves to the same
selector. zoho-desk-org-options.ts is deleted rather than left beside the new
path, so blocks/ has zero fetchOptions usages outside the core blocks.
Wire params are unchanged (orgId, departmentId, departmentIds, assigneeId,
ticketId, contactId) - this is a UI change, not an API change.
The organizations route now resolves the credential server-side. It previously
had the browser fetch an access token and POST it back, which an earlier audit
flagged as the one place a Zoho token left the server; the new selector-credential
resolver keeps it server-side for both the OAuth and service-account credential
types and re-anchors every outbound host to the Zoho apex allowlist.
No agents selector: the endpoint is documented but its OAuth scope is not, and
the nearest evidence points at Desk.agents.READ, which we do not request. Adding
it would force every existing Zoho Desk user to reconnect for a convenience
field, so assigneeId stays a manual input until the scope can be confirmed
against a live org.
Adds the skills array BlockMeta was missing - 227 of 300 blocks declare one and
this did not. Seven skills, each grounded in a use case Zoho or the ecosystem
actually advertises (auto-triage, SLA escalation, digest, AI draft reply,
customer context, engineering handoff, knowledge-gap report) and each exercising
only tools in tools.access. CSAT surveys, ticket creation, dedup and keyword
search were deliberately left out: the integration has no tool for them, and a
skill implying an unsupported action is worse than a shorter list.
* feat(zoho-desk): agents selector and free-text trigger organization
Three improvements that were previously deferred only to avoid forcing existing
users to reconnect or orphaning saved workflows. This integration is unmerged and
has no users, so the constraint does not apply and the better option wins.
assigneeId was the last field still asking for an opaque numeric id. It is now a
canonical selector pair backed by a new zoho_desk.agents selector, which required
adding the Desk.agents.READ scope - the reason it was skipped before. Route
follows the departments one exactly: auth before parseRequest, host anchored to
the Zoho apex allowlist, secureFetchWithValidation with stripAuthOnRedirect, and
a page drain capped at 20 pages with 204 treated as end-of-list.
Scope caveat: Zoho publishes no explicit scope line for the list-all
GET /api/v1/agents. Every other endpoint in the Agents module documents
Desk.agents.READ (get by id, get by email, roles/{id}/agents), and it is the only
agents-module scope Zoho defines, so that is the basis. Inference across a module
rather than a direct quote - worth one live call before merge, same as the
existing attachment-scope note.
The trigger regained free-text organization entry, lost when the org field became
a selector. The earlier concern - that a manual value would land under its raw
subBlock id and never reach the provider - turned out not to hold: buildProviderConfig
already collapses canonical pairs and writes the active member under the canonical
key. The real gap is narrower and does exist: when canonicalModes pins the group
to basic while only the manual field has a value, the collapse deletes the
canonical key even though the required-field check passes, so the deploy succeeds
and then fails at subscription time. resolveConfigOrgId closes that, with a test.
The block/trigger `orgId` id overlap stays shared, now with a comment. Two earlier
audits disagreed; renaming turns out to be the wrong call. buildCanonicalIndex has
an explicit guard for trigger-mode reuse and blocks.test.ts codifies it as a valid
pattern, orgId means the same portal in both modes (unlike departmentIds, which is
correctly distinct), and a separate triggerManualOrgId would put two advanced
members in one canonical group - getCanonicalValues takes the first non-empty, so
a stale tool-mode value could silently supply the trigger's organization.
* fix(zoho-desk): make the attachment cap reachable, unbreak selector paging
Final audit round.
The 50 MB attachment ceiling could never be hit. This route returns the file as
base64 inside its JSON body, and the executor reads internal tool responses
through readToolResponseBody, capped at 10 MB. Base64 inflates 4/3, so ~7.5 MB
of raw bytes is the real ceiling - and the old limit meant a larger attachment
was downloaded, encoded and serialized in full (peaking near 250 MB of live
allocation, with nothing bounding concurrent downloads) purely to be rejected
afterwards. The cap is now the reachable size, so the limit enforces itself while
the bytes are still streaming, and an overflow returns 413 with the actual
ceiling instead of a generic 500. Raising it properly means uploading in the
route and returning a file reference, as the WhatsApp media route does - not a
bigger constant.
Selector paging assumed a 0-based `from`. Zoho's docs contradict themselves:
the pagination section says "range 0-4999, default 0" while the listing examples
read as 1-based ("from=5 and limit=50 retrieves records 5 to 54"). Under the
1-based reading, stepping by exactly the page size re-fetches the boundary record
and the dropdown shows a duplicate per page. Rather than pick a base that cannot
be confirmed without a live tenant, the department and agent drains dedupe by id,
which is correct under either reading.
The organization list was unpaginated, and Zoho's listing APIs default to ten per
page. An account with more accessible portals silently got a truncated dropdown,
and since every other selector and every tool call is gated on orgId, a missing
portal was unreachable except through the advanced manual field. Both the
selector route and list_organizations now request the documented maximum.
Docs: regenerated so the trigger table includes manualOrgId, and two
service-account claims are hedged to match what the code already says it cannot
verify - that zsoid equals the Desk orgId header value, and that every tool works
under the requested scopes (Zoho publishes no scope for the attachment content
sub-path).
Also: status and priority move out of advanced mode - they are the fields most
often changed on a ticket update; the custom-fields wand prompt now ends with the
required "Return ONLY" clause; and the shared-orgId rationale comment cites the
mechanism that actually applies (buildCanonicalIndex dedupe plus the first-non-
empty rule in getCanonicalValues) rather than a blocks.test.ts branch that never
evaluates this pair.
* fix(zoho-desk): five-audit round - serializer trigger-advanced leak, scopes, paging
Five independent audits (OAuth/scopes, tools-vs-docs, block/selectors,
blast-radius, /validate-trigger). Findings, most severe first.
A trigger-mode field was a live tool-mode required param. `shouldSerializeSubBlock`
excluded `mode: 'trigger'` but not `'trigger-advanced'`, so the trigger's required
`manualOrgId` validated on every tool operation. Reproduced against the real
serializer: with the Organization field pinned to advanced, running
List Organizations failed with "Missing required fields: Organization ID" - a
field that operation does not even render, and which the user could not clear
without switching operations. Fixed in the serializer rather than locally,
because the Google Sheets/Drive/Calendar pollers have the identical shape.
`limit=200` on /organizations was an undocumented parameter I added by
extrapolating from /departments and /agents. Zoho documents NO parameters for
that endpoint and its sample is a bare GET; the other siblings cap at 100 and
Zoho answers out-of-range with 422. Since orgId gates every tool and both other
selectors, a 422 there would have made the whole integration unreachable. Reverted
to Zoho's documented shape.
`descriptionText` was HTML-stripping plain text. The previous round made the strip
unconditional after finding Zoho sends no `descriptionContentType`, but Zoho's REST
samples show plain descriptions while only the webhook payload is HTML - and the
webhook path runs this over contact/account/department bodies too. html-to-text is
not identity on plain text: it decodes entities and deletes tag-shaped content
("a < b > c", XML snippets). Now sniffs for markup first.
`omitUnset` made every documented field-clear impossible. Zoho's own PATCH sample
uses `"classification": ""` and `"productId": ""` to clear. Dropping `''` meant no
scalar field could be cleared. Now drops only undefined/null - the serializer-null
case it was written for - and forwards `''`.
status/priority leaked between operations. One shared subBlock served both the
list_tickets filter and the update_ticket value, and subBlock values survive an
operation switch, so a filter of "Open,On Hold" could be PATCHed onto a ticket and
an update value could silently filter a later list. Split per operation.
Auth: `invalid_code` added to TERMINAL_ERRORS - it is Zoho's code for a revoked
refresh token, so without it the previous round's refresh fix never actually
dead-flagged the credential it was written for. The shared refresh body-error
branch now also requires `!data.access_token`, so no provider can have a
successful refresh misclassified. The token route now uses the validating
`extractZohoDeskBaseFromScope` instead of a private regex with no https/allowlist
check - that value is injected into every tool call. Scope list falls back to the
requested scopes when Zoho omits `scope`, which would otherwise flag every
credential as needing reconnect. The Self Client mint no longer sends
`aaaserver.profile.READ`, a scope that grant never uses.
Trigger: `includePrevState` now set for every *_Update event, not just tickets -
it defaults to false, so prevState was permanently null for contact/agent/task/
article updates while the trigger advertised it. `departmentIds` is only sent for
events Zoho documents as accepting it, and the field is conditioned accordingly.
Empty filters serialize as `null`, matching Zoho's examples, rather than `{}`.
JWKS fetch bounded to 1.5s - jose's default is 5000ms, exactly Zoho's whole
delivery deadline, and Zoho publishes no retry. The create-time validation POST
fallback is now matched by the pending-verification probe. Ticket_Delete added.
All 17 webhook event ids, the POST /api/v1/webhooks body contract, and the JWT
claim/JWKS specifics are now confirmed verbatim against Zoho's webhook
documentation - previously 12 of 17 events and the entire subscription contract
were unverified.
* revert(zoho-desk): back out both shared lib/oauth changes
Reverting two changes to shared OAuth code because their premise is inferred
rather than proven, and neither meets the bar for touching a path every provider
runs.
`refreshOAuthToken` body-error branch. The premise was that Zoho reports refresh
failures with HTTP 200 and an `error` body. That is documented and empirically
confirmed for the authorization-code EXCHANGE (see the comment on getToken in
auth.ts), but I never confirmed it for the REFRESH grant specifically - and if
Zoho returns a proper 4xx there, the existing `!response.ok` path already
classifies it via extractErrorCode, making the branch dead code that every one
of the ~34 providers still executes on each refresh. A shared branch whose only
justification is an unverified inference about one provider is not worth its
blast radius.
`invalid_code` in TERMINAL_ERRORS. Same problem, worse downside: the code is
sourced from a Zoho community post rather than official docs, TERMINAL_ERRORS is
consulted for every provider, and a false positive marks a credential dead for an
hour. Not adding it simply preserves today's behavior (retry rather than
dead-flag), so reverting costs nothing that was previously working.
Both are cheap to reinstate, correctly scoped, once a live Zoho account shows
what a revoked refresh token actually returns.
Kept: the token-redaction on the "no access token" warn, which is an unambiguous
improvement independent of Zoho.
Also kept, deliberately, is the serializer `trigger-advanced` exclusion - that one
rests on a reproduced bug rather than an inference, and it aligns the serializer
with the convention the rest of the codebase already follows (blocks.test.ts
treats `trigger` and `trigger-advanced` identically in six places, as does the
copilot block-metadata tool, and blocks/types.ts documents trigger-advanced as
"the advanced side of a trigger field").
* fix(zoho-desk): carry the stored data center through a credential reconnect
A reconnect rebuilds the service-account secret blob from the submitted fields
only, and the connect modal never prefills - correctly, since for every other
field in this family the stored value is a secret the admin must retype. The
data center is the first non-secret member of that set, so it was being silently
dropped: rotating a client secret on an EU/IN/AU credential moved it back to the
US accounts server, where the next mint fails with an opaque invalid_client.
performUpdateCredential now reads the stored dataCenter out of the existing blob
when the caller does not supply one. The read is failure-tolerant - an
undecryptable or unparseable blob yields undefined rather than throwing, so it
can never block a reconnect, and the provider default applies as before.
Raised independently by three reviewers; I twice argued it was acceptable because
the mint fails loudly rather than corrupting silently. That was true and beside
the point - the operator still had to guess why.
* fix(zoho-desk): delta-audit findings - prevState scope, status leak, HTML sniffer
An audit of the commits the earlier five audits never saw. All four findings are
in code written as fixes for those audits, which is where this branch has
repeatedly introduced new problems.
`includePrevState` was sent for Ticket_Comment_Update. The previous commit gated
it on an `_Update` suffix and claimed Zoho supports it on every update event.
Zoho's webhook doc lists the attribute on Ticket/Contact/Agent/Task/Article update
events but NOT on Ticket_Comment_Update, which documents only `departmentIds`.
That made it an undocumented filter key on a live subscription create - the same
class of risk the same commit reverted `limit=200` for, so it failed that commit's
own stated bar. Now an explicit set rather than a suffix rule.
The status/priority split did not stop the leak it was written for. The mapping
used `operation === 'list_tickets' ? filterValue : updateValue`, whose bare else
covers all eight other operations - so a stale Update Ticket status was forwarded
into get_ticket, list_comments and the rest. Harmless on the wire (those tools
ignore it) but exactly the stale-value pattern the neighbouring gates exist to
prevent. Both fields are now scoped to the two operations that declare them.
The HTML sniffer destroyed plain text. `/<[a-z!\/][^>]*>/` fires on any `<`
followed by a letter with a later `>`, so realistic ticket bodies lost content:
"if x<y then z>0" became "if x0", and "replace <username> with the real name"
lost the placeholder. It now requires a real element - a paired tag, a
self-closing tag, a comment/doctype - or an entity, and the entity arm covers hex
references it previously missed. Regression tests verified by reverting to the
loose pattern and watching them go red.
The reconnect data-center carry-forward is scoped to client-credential providers.
As written it added a DB read plus a decrypt to every service-account reconnect
for every provider - Slack, Atlassian, all token-paste providers - to carry a
field only Zoho has.
Also: the JWKS cache-bound TSDoc had been orphaned onto the wrong constant by an
earlier insertion, and `cooldownDuration` was dropped since it restated jose's
default while only `timeoutDuration` needed justifying.
* test(zoho-desk): cover the webhook subscription filter rules
The subscription filter logic had no test coverage at all, and it is where the
last two rounds both found bugs - includePrevState on an event Zoho does not
document it for, and departmentIds sent to events that accept no filters.
Adds six cases against the real createSubscription: includePrevState is set for
each of the five documented update events and NOT for Ticket_Comment_Update,
departmentIds is kept for a filterable event and dropped for one that is not, and
an event with no filters serializes as null rather than an empty object.
Verified the guard bites: reverting PREV_STATE_EVENTS to the `endsWith('_Update')`
rule turns the Ticket_Comment_Update case red.
The Ticket_Comment_Update assertion checks the with-departments case as well as
the bare one - asserting only `not.toHaveProperty` on the bare filter would pass
vacuously, since that filter is legitimately null.
---------
Co-authored-by: Waleed Latif <walif6@gmail.com>
#6190) Tables and knowledge bases gained folder support, but the chat attachment menu and the embedded resource menu still listed them flat. Both now render the same hierarchy the Tables and Knowledge pages show. Unifies the two near-duplicate workflow/file tree builders into one shared buildResourceFolderTree plus a spec-driven useResourceTreeSections, so all four foldered families go through one code path. Items whose folder no longer resolves now surface at the root instead of vanishing.
* fix(search): disambiguate tables and knowledge bases by folder The Cmd-K search modal listed tables and knowledge bases without the folder breadcrumb workflows and files already showed, and the table, knowledge base, and search-and-replace pickers in the workflow editor rendered bare names -- so two resources sharing a name in different folders were indistinguishable. Extracts the disambiguation the workflow selector already did into shared collectDuplicateNames + disambiguateLabelByFolder, and shares the search row's folder breadcrumb and its memo comparator, which were duplicated between the workflow and file rows. Also routes folder text through filterAndCap's secondary-rank parameter rather than concatenating it into the name, so an exact name match can no longer be outranked by a folder that happens to fuzzy-match. * fix(zoho-desk): use the real Zoho Desk mark on a white tile The icon was a generic headset placeholder drawn in currentColor, so it never resembled Zoho at all. Replaces it with the mark from Zoho's official logo -- wordmark stripped, viewBox set to the mark's own bounding box so it centers -- and moves the tile to white, matching the other brand-mark integrations.
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.