v0.7.52: span sanitization, highlight tables and files text to chat, settings UX improvements, security hardening - #6183
Open
waleedlatif1 wants to merge 27 commits into
Open
v0.7.52: span sanitization, highlight tables and files text to chat, settings UX improvements, security hardening#6183waleedlatif1 wants to merge 27 commits into
waleedlatif1 wants to merge 27 commits into
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.
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.