* feat(secrets): let workspace secrets opt out of redaction
* fix(secrets): certify no sandbox exemptions once the registry is incomplete
* feat(secrets): carry visible secret values on the v2 list and document visibility
* fix(secrets): read visible values by own property so prototype-named secrets cannot poison the list
`check-utils-enforcement.ts` scanned line by line, and every idiom it bans is a
multi-token expression the formatter wraps at 100 columns. So it printed
`✓ No banned patterns found` while eleven files carried the wrapped form of
e instanceof Error ? e.message : fallback
which CLAUDE.md mandates `getErrorMessage` for. The same class as the two blind
spots already fixed in check-react-query-patterns.
Patterns now run against the whole file, with match offsets mapped back to line
numbers by binary search over the line-start table — verified against every
offset of a multi-line fixture.
Eight of the eleven are now `getErrorMessage(error, fallback)`.
`auto-layout-utils` collapses a redundant `instanceof ApiClientError` arm on the
way, since that class extends `Error`; `upgrade.ts` keeps its `rawBody ?? message`
arm, which the helper cannot express, and only its tail collapses.
The other three stay, because the helper genuinely does not fit, and they carry a
`// utils-lint-allow: <reason>` annotation — the same escape hatch
check-react-query-patterns already has, which this gate lacked:
- the two auth routes return the message to an unauthenticated caller, so a
non-Error throw must surface the fixed copy rather than its own text.
`getErrorMessage` passes a thrown string straight through, which is the
disclosure shape #7015 closed.
- `e2b.ts` probes E2B's own error shape — a record-like carrying `message` or
`value` — which has no equivalent.
An annotation with no reason does not suppress, so the hatch cannot be used to
silence a finding without saying why.
Also corrects the header, which claimed Biome's `noRestrictedImports` covers
"crypto named imports". It lists only `nanoid` and `uuid`. Named crypto imports
pass both gates deliberately — server code building cipher IVs wants node's
crypto, not the cross-context wrapper — and the comment asserting otherwise would
mislead the next person auditing this.
Verified the gate can fail in both directions: reintroducing a wrapped ternary
reports it, and emptying an annotation's reason reports it too.
* refactor(auth): drop the isProduction prop nothing on the signin path reads
Same shape as the `isWorkflowRunning` removal: declared, required, threaded
through every layer, and never read at the end of the chain.
`SocialLoginButtons` declares `isProduction: boolean` as a REQUIRED prop and
never reads it, so every caller had to produce and forward a value that was
discarded. Neither `login-form` nor `signup-form` reads it either — each only
declares it, destructures it, and passes it down. `signup-form` forwards it twice,
through its own inner `SignupFormContent` hop.
With the chain gone, `getOAuthProviderStatus` has no consumer for the
`isProduction: isProd` it returned: the pages destructured it only to forward it,
and `/api/auth/providers` already takes just the three availability flags. So the
return value and its `isProd` import go too.
`isProduction` stays alive where it is genuinely used — `verify-content.tsx`
branches on it and hands it to `useVerification`, and imports `isProd` directly
rather than through this helper. That path is untouched.
Found by the rule enabled in #7037: it was the only `.tsx` unused-parameter
warning in `apps/sim`.
(cherry picked from commit 331c2ed2ed)
* refactor: drop two more props declared, threaded, and never read
Same shape as the two already in this PR, found by sweeping the rest of the
unused-parameter list for params callers actively compute and pass.
`FieldItem.level` is the worse of the two. It is a required `level: number` that
the component never reads, and `FieldTreeNodes` exists to thread it: declared,
destructured, handed to `FieldItem`, and incremented on every recursion
(`level={level + 1}`) from a `level={0}` seed. So a depth counter was carried
through an arbitrarily deep tree to feed a component that ignores it. Indentation
comes from the nested wrapper divs (`ml-1.5 pl-2.5`, `ml-3 pl-2.5`), not from the
counter — removing it changes no rendering.
`useMentionMenu`'s `onContextSelect` is a required prop carrying the TSDoc
"Callback when a context is selected". The hook never invokes it, so that
contract is unimplemented and a future caller would reasonably rely on it.
Only the dead hand-off goes there. `addContextNotified` stays: the caller invokes
it directly at five sites, and the ref sinks behind it keep its identity stable
for those. Context selection has always worked because the caller does the work
itself, not because the hook calls back.
(cherry picked from commit 110ba76df3)
* refactor: drop two more dead prop chains in the sub-block editor
`GroupedCheckboxList` declares `title` (required) and `maxHeight` and reads
neither. It renders its own hardcoded copy instead — `Select PII Types to Detect`
for the header and `PII types` for the field label — so a block author who sets
`title` on a `grouped-checkbox-list` subBlock gets silence, and the
`maxHeight = 400` default implies a scroll ceiling that is never applied. Both
props go, along with the two values `sub-block.tsx` was passing.
`flatTagList` was threaded through the recursive tag renderers to a dead end:
declared on `NestedTagRendererProps`, inherited by `FolderContentsProps`,
destructured in both, forwarded once more, and read by neither. Its real consumer
is `flatTagIndexMap`, built from it at the top level and documented "Map from tag
string to index for O(1) lookups" — so the array was being carried alongside its
own index through arbitrary nesting depth. The top-level memo and its length
checks stay; only the descent goes.
Note the component's copy is PII-specific while its name and props present as
generic. Renaming it is a separate call, not made here.
Both removals were caught mid-flight by `tsc`: my line patterns also matched a
live `flatTagList` on `KeyboardNavigationHandler` and a live `title` on `Switch`,
which is exactly why the type-check runs before the commit and not after.
(cherry picked from commit 5db44f347b)
* refactor(custom-blocks): drop the workspaceId three mutation hooks never use
`usePublishCustomBlock`, `useUpdateCustomBlock` and `useDeleteCustomBlock` each
take `workspaceId?: string` and never read it. `custom-block-detail.tsx` passes it
to all three.
The parameter looks like it was meant to narrow the invalidation to
`customBlockKeys.list(workspaceId)`, but `lists()` is the level CLAUDE.md's
targeted-invalidation rule actually prescribes, and it is a correct superset. So
the invalidation is right as written and the parameter is simply vestigial —
removing it is the honest fix, and narrowing the key would be a separate call
with its own risk of under-invalidating.
Worth recording that these three were reported to me as having zero callers and
therefore being dead exports. They are not: the search that produced that claim
omitted `apps/sim/ee`, where all three are used.
(cherry picked from commit 2d0854a587)
Removing only the unused binding and keeping the prop was half a fix. The views
declared it, the app passed it, and nothing read it — so the prop was dead, and
dead code does not become live by being documented.
Its TSDoc claimed it "holds every block's action swell open". That behavior does
not exist in either view. Keeping the prop on the chance someone wants it later
is the speculative-generality smell: if the toolbar should pin open during a run,
that gets implemented deliberately and the prop comes back with logic behind it.
Removed from both view interfaces and from both call sites. The store
subscription stays — `workflow-block.tsx` and `subflow-node.tsx` each passed the
same value twice, once to the dead view prop and once to `ActionBar`, which has
28 real reads and is what the surrounding TSDoc is actually describing when it
says the flag "only swaps Run for Stop and disables mutations". `workflow-edge-view`
uses it too and is untouched.
The renderer test that passed it loses the argument. Worth noting it set the flag
to stage a workflow run, and since the view ignored it those two cases were never
exercising the run state they name.
* improvement(provenance): attribute stored-envelope display reads to their execution
A display materialization of an execution log imports the row's stored
provenance envelopes into throwaway registries, and each import of an
incomplete envelope re-emitted the registry's own summary — per envelope,
per view, carrying counts and a workspace but never the execution id. A
reader repeatedly materializing the same stored rows produced hundreds of
identical lines that could not say which executions to go look at, and
the volume scaled with views of a state that was fully recorded when the
run wrote it.
Verified against production before changing anything: essentially no new
incomplete envelopes are being stored since the writer fix shipped, and
no data drains exist — the stream is bounded re-reads of old rows through
the display paths, not a live producer.
The display registries are now staged — the existing concept for a
registry that filters one value for a caller that reports against the
real boundary — and each display function reports once per
materialization with the execution id, workflow, workspace, and the
parts that could not be vouched for. Severity is preserved: an
incomplete stored envelope stays at warn, a malformed one stays at
error. Projection behavior is unchanged everywhere — incomplete and
malformed envelopes still fail their values closed exactly as before;
only the reporting moves to the boundary that knows the execution.
* improvement(provenance): fold the incomplete-envelope predicate and pin dual-site reporting
Review pass over the previous commit: one helper instead of three copies
of the incomplete-envelope check, the staged TSDoc generalized to cover
both of its uses, and the block-outputs entry point's two-site reporting
of one run envelope documented and pinned rather than left implicit.
* improvement(provenance): classify every unusable stored envelope at the display boundary
Review findings from the first round, both accepted: a present-but-
malformed block or run envelope was withheld with no attributed line,
and a complete envelope whose entries fail decryption latched the
staged registry with only the unattributed entry-level error.
Fault classification moves into the one import helper the display
paths share, which now returns the registry and the fault together:
absent is not a fault, unparseable is malformed, unable-to-vouch is
incomplete, and a complete envelope whose registry latched during
import — entry decryption is the only latch on that trusted path —
is undecryptable. Every consumer reports through the same table,
severity per kind, so the exact-value loop stops being the only site
that could name a malformed envelope. Withholding behavior is
unchanged at every site.
Three rules were off, so nothing enforced them. Measured, fixed the sites, and
enabled them where the cost is bounded.
`noAccumulatingSpread` — 2 violations, both real O(n²) reducers, both now
`Object.fromEntries`. One duplicates a block's subBlocks on every block
duplication; the other rebuilds a Record from every workspace env var. Enabled
repo-wide.
`noUnusedVariables` / `noUnusedFunctionParameters` — 633 repo-wide, but only 6
under `packages/`. Fixed those 6 and enabled both at error for `packages/**` via
an override, which permanently covers 979 files. `apps/sim`'s remaining 627 are
left deliberately: that is a sweep of its own, and a rule enabled with 627
outstanding warnings teaches people to ignore it.
This is the class of rule whose absence let the dead code in #7019 accumulate —
eleven unread loggers, a whole unimported file, write-only locals — none of which
any gate could see.
Two of the six were in `workflow-renderer`, where the fix is narrower than it
looks. `isWorkflowRunning` is destructured-but-unread in both the block and
subflow views, and the app passes it from `workflow-block.tsx` and
`subflow-node.tsx`. Its TSDoc claimed it "holds every block's action swell open";
nothing reads it, so that behavior does not exist. Removing the prop breaks the
callers and implementing it is a UX decision — there is adjacent logic
deliberately not pinning the toolbar during a handoff. So only the unused binding
goes, and the TSDoc now says what is true.
Not enabled: `noDocumentCookie` (3 sites, and its fix is the CookieStore API,
which is a browser-support call) and `useExhaustiveDependencies` (384 errors).
Push builds fail the migration audit:
✗ Migration safety check could not run.
Cannot diff against 'HEAD~1'.
`actions/checkout` sets no `fetch-depth`, so it defaults to 1 — a single-commit
clone in which `HEAD~1` does not resolve. Both diff-based audits named `HEAD~1`
as their push base, so neither has ever had a base to read. The migration audit
answered that with `✓ No new migrations to check` and exit 0, so it had never
run on a push build at all; #7022 made it say it could not run instead, which is
what surfaced this. The block-registry check reports `⚠ … skipping` on the same
input — visible, and equally never run.
`HEAD~1` was the wrong base regardless. It names the last commit, so a push
carrying several commits audits the tip and lets every earlier commit through:
3-commit push, HEAD~1 base: mig3.sql
3-commit push, before base: mig1.sql mig2.sql mig3.sql
The base is now `github.event.before` — the tip the branch had before the push,
which is what GitHub provides for exactly this. It is fetched by SHA at depth 1;
the audits diff two tips and need no common ancestry between them. Resolved once
in a step both audits read, so the two cannot drift apart.
`HEAD~1` survives only as the fallback for an all-zero `before` (a new branch,
with no predecessor to diff), which is what `fetch-depth: 2` now covers.
Verified: both audits accept a raw SHA base and pass; the multi-commit case above
is a real reproduction, not a description.
* fix(ci): require a default export before treating a file as a route entry
Follow-up to #7026, which added `error.tsx` to the entry filenames and with it
picked up `[workspaceId]/components/error/error.tsx` — named like a boundary,
and not one. It exports `ErrorShell` and `ErrorState` for the thirteen real
boundaries to use; Next would reject it as a boundary for having no default
export. Counting it inflated the coverage number and would have recorded a
shared component in the graph-weight baseline as though it were a route.
The filename was never the right test. Every convention-composed entry must
default-export the thing Next renders, so that is the discriminator now. Entry
count goes 60 → 59, and all thirteen real `error.tsx` boundaries still walk.
Also adds `template.tsx` and `default.tsx`. Neither exists under
`app/workspace` today, so this changes nothing now — but the enumeration claims
to cover what Next composes, and leaving two out makes that claim false the day
someone adds one.
Both raised in review on #7026 (Cursor and Greptile respectively); I merged
before reading them, so this lands separately.
* fix(ci): count every form that declares a default export
`export { default } from './page'` is a valid Next entry and the regex required
`as default`, so such an entry would have dropped out of the walk and skipped
both the registry gate and the graph-weight ratchet — silently, which is the
dangerous direction for a discriminator to fail in.
Latent rather than live: the form appears once under `app/workspace`, in a
barrel, not in an entry filename.
Four forms now count — `export default …`, `export { default } from`,
`export { default, … } from`, and `export { X as default }`.
`export { default as X }` still does not: it re-exports another module's default
under a name and leaves this one without one. Verified all ten variants,
including that last distinction.
Raised by both Cursor and Greptile on #7028.
`totalRoutes` sits at 1162 and the repo has exactly 1162 routes, so the next
route fails CI whether or not it is contract-backed:
API validation audit failed:
- route count increased from 1161 to 1162
The invariant worth holding is that every route has a contract, and
`nonZodRoutes` states exactly that. It is 0, and it rises the moment a route
ships without one — `zodRoutes === totalRoutes` today, so the total adds no
information the other two counters do not already carry.
What it adds instead is a habit. The only way past it is editing the number, and
this file holds seven other baselines that work only while nobody bumps a
baseline casually.
The total is still printed; it is no longer a failure. Verified both directions:
a compliant new route passes where it previously failed, and a route without a
contract still fails through `nonZodRoutes`.
The tool-registry guard collected `page.tsx` and `layout.tsx`, and Next composes
three more entries by convention: `error.tsx`, `loading.tsx`, `not-found.tsx`.
Twenty-six exist under `app/workspace` and none was walked. `error.tsx` is
always a Client Component — Next requires it — so a registry edge there reaches
the browser bundle exactly as one from a page does.
Coverage goes from 34 entry graphs to 60. Nothing new is reported: the hole was
unexploited, and closing it costs nothing.
The root deliberately stays at `app/workspace`. Widening it to `app` reports
`(interfaces)/resume/[workflowId]/[executionId]/page.tsx`, a Server Component
(`runtime = 'nodejs'`, `force-dynamic`) whose `PauseResumeManager` import
resolves server-side and never reaches a client bundle. The guard cannot
distinguish server from client entries, so it stays where its premise holds.
`check-react-query-patterns.ts` reported a clean strict zone while never
looking at part of it. Two gaps in one regex:
`\buseQuery\s*\(` does not match `useQuery<Row[]>({ ... })` — a type argument
sits between the name and the paren. Twenty query calls carry one, ten of them
inside the zero-tolerance zone, so that zone's "0 violations" was partly a
statement about what the scan could see.
`useQueries` was absent from both the call pattern and the file pre-filter,
where `\buse(Query|...)\b` rejects it on the trailing `s`. All sixteen call
sites were unscanned, and its options nest one level deeper — inside a
`queries` array — so it needs its own pass per entry rather than one that reads
the wrapper and takes a single `staleTime` anywhere inside as covering them all.
With both closed, three real violations surfaced:
- `knowledge-base-selector` served `knowledgeKeys.detail(id)` with an inline
`60 * 1000` while `useKnowledgeBaseQuery` serves the same cache key from
`KNOWLEDGE_BASE_DETAIL_STALE_TIME`. The two agree only by coincidence, and
TanStack resolves staleTime per observer, so tuning the constant would have
left this component on the old window for the same entry.
- The same call dropped the `AbortSignal`, which `fetchKnowledgeBase` accepts.
- `use-permission-config` gave `staleTime` as a literal with no named constant.
The new `stale-time-literal` category makes the second half of the CLAUDE.md
rule enforceable — it required a named constant, and only the presence of
`staleTime` was ever checked. `0` is exempt: it is the sentinel for "always
refetch", not a window anyone keeps in step with a prefetch.
Verified the new rules can fail by reverting each fix and watching the audit
report it, then restoring.
Two places where a failure is reported as something smaller than it is.
**A run that is never billed logs as a notification problem.** The usage
safety net re-records billing when an earlier step threw before the single
record call, and its own failure went into a bare `catch {}`. With a degraded
database the user lookup throws first, the re-record hits the same database and
is swallowed, and the only line emitted reads "Usage threshold notification
check failed (non-fatal)" — which is true of the outer failure and badly wrong
about the inner one. It now logs at error with the execution and workflow ids,
and says the run may be unbilled. The outer warn still covers the email path it
was written for.
**google-docs can ask Drive for a negative page.** `remaining` was
`maxDocs - previouslyFetched` unclamped, where its google-slides twin carries
`Math.max(0, …)` under the comment "Last-page precision". Both then run
`if (documents.length > remaining) documents = documents.slice(0, remaining)`,
and a negative `remaining` makes that guard true for any non-empty page while
`slice` counts from the end — keeping the leading documents and dropping the
trailing ones, where the cap says to keep none. Reachable when `maxDocs` is
lowered while a sync cursor persists. google-drive guards the same case with an
early return; google-docs had neither.
The PATCH catch answered `{ success: true }` with 200, so a failed upsert was
indistinguishable from a saved one.
`useUpdateGeneralSetting` is optimistic: `onMutate` writes the new value into
the cache and calls `syncThemeToNextThemes`, and `onError` restores the previous
settings. `requestJson` only throws on a non-2xx, so `onError` could never run —
the rollback and its theme re-sync were unreachable code. A user toggling a
consent-shaped setting (telemetry, email opt-out) saw it applied and it was not
saved, until a later refetch quietly reverted it.
The catch now returns 500, which is what the mutation was already written to
handle.
Left alone deliberately: GET still falls back to `defaultUserSettings` on error.
Failing it would take the settings page down on a transient read, and the value
of changing it is a separate judgement from this one.
Covered by a route test that drives the failure through the real handler.
Verified it fails when the 200 is put back.
`biome.json:101-102` turns off `noUnusedVariables` and
`noUnusedFunctionParameters`, so none of this was ever going to be flagged.
Everything here was confirmed by grepping the symbol across `apps/` and
`packages/` and finding only its own declaration; `tsc --noEmit` then proves
each deleted binding was unread, since a read one fails to compile.
- Eleven module-scope loggers that nothing logs through, with the now-orphaned
`createLogger` import each left behind.
- `execute-platform-context-use-case.ts` — the whole file. No importer, no
barrel, and neither export is named anywhere.
- `routeToolCall` and, once it goes, `ToolRoute` and `ToolRouteTarget` with it.
The catalog accessors around them stay live.
- `processPastChat`, superseded by `processPastChatFromDb`. It carried the last
`boundary-raw-fetch` exemption in the file.
- `withMessageId`, pasted into three server tools and called in none.
- Write-only locals: `activeSubagent` (assigned twice, read never — the scoped
maps replaced it), `resolvedReadPath`, `workflowPath`, and `workflow` in an
execution-core destructure.
- `ACCEPTED_AUDIO_TYPES` / `ACCEPTED_VIDEO_TYPES`, never wired to an accept
attribute the way their live sibling is.
- Unused `catch` bindings in `error-extractors.ts` and `defaults.ts`.
`diff-engine.ts` drops a `proposedSubKeys.includes(key)` guard that the
`!proposedSub` check three lines down already covers: a key absent from the
proposed block reads back `undefined` there, and so does a key present with a
nullish value. Same answer on every input, without the O(n) scan per iteration.
* fix(ci): stop the migration safety audit from passing on a branch it never read
The zero-downtime audit reports the same empty file list for 'this branch adds
no migrations' and 'I could not diff against the base', and the second prints
as `✓ No new migrations to check` with exit 0. Reproduced on this checkout:
$ bun run scripts/check-migrations-safety.ts origin/does-not-exist-branch
✓ No new migrations to check. exit=0
`changedMigrationFiles` returned `[]` whenever `git diff` failed, with a comment
deferring the decision to the caller — but the caller only recognised a missing
git binary (`git rev-parse HEAD === null`), never an unusable ref.
CI supplied exactly that input. `git fetch --depth=1 … 2>/dev/null || true` hid a
failed fetch, leaving `origin/<base>` absent, so a PR adding a destructive
`DROP COLUMN` would clear the only guard on production DDL with a green check.
Two halves:
- The audit now distinguishes the cases. Absent git is still the one legitimate
skip and is checked before the diff; a diff that fails with git present raises
`BaseRefUnusableError` and exits 1.
- The fetch is its own step with no `|| true`, so a failure fails the job. Depth
stays 1: without a merge-base the audit diffs the two tips, which under
`--diff-filter=AM` is exactly the migrations new on the branch.
Covered by a test that runs the script end to end, since the defect was in the
exit code rather than in any function's return value. Verified it fails when the
throw is reverted to `return []`.
* fix(ci): fetch the base ref once, and stop swallowing the failure
The same `git fetch --depth=1 … 2>/dev/null || true` appeared in both base-ref
audits. Fixing only the migration one would have left the identical defect a few
steps above it.
Neither audit can tell an absent base ref apart from a branch that changed
nothing. The block-registry check at least degrades to a visible
`⚠ Could not diff against base ref — skipping`; the migration audit printed
`✓ No new migrations to check` and exited 0.
Both now share one fetch step that fails the job when it fails.
The same three-step derivation — lowercase, collapse each non-alphanumeric run
to a hyphen, strip the leading and trailing one — sat in eight files. Two of
them carried a TSDoc line whose only job was to warn that they mirrored a third
(`instance-org.ts`: "Derives a slug the same way the admin organization API
does"; `consolidate-users-into-organization.ts`: "Mirrors the slug derivation
used by POST /api/v1/admin/organizations"). A comment asserting two
implementations agree is the shape duplication takes when it cannot be checked.
All eight were semantically identical. Two anchored the strip with `-+` rather
than `-`, and one followed it with a `--+` collapse, but `[^a-z0-9]+` has
already collapsed every run by that point, so neither could ever match more than
the single-hyphen form. Nothing changes.
Truncation stays at the call sites. Four of them bound the result — at 24, 64
and 80 — and only `copy-chats.ts` strips again afterwards, because slicing can
land mid-run and leave a trailing hyphen the earlier strip never saw. Folding a
`maxLength` into the helper would have had to pick one of those behaviors and
silently impose it on the others.
`artifact-stylesheet.ts` keeps its copy: it lives inside the `SIM_ARTIFACT_SHELL`
template literal and runs in the viewer's browser, where there is no import to
resolve.
* refactor: replace hand-rolled utilities and dead code with the shared forms
Each of these has a mandated helper or an established accessor in the repo that
the site predates or missed. All are behavior-preserving:
- `omit()` for the three `Object.fromEntries(Object.entries(x).filter(...))`
block-input filters, which also recovers the `Omit<T, K>` typing that
`Object.fromEntries` erases to an index signature.
- `getErrorMessage()` for the inline `instanceof Error` message ternary.
- `getBlock()` for two `getAllBlocks().find((b) => b.type === x)` scans, one of
them inside a loop over selected tools. The same file already resolves the
same values through `getBlock`.
- A memoised `Map` for three `.find()`-by-id scans over the workspace skill
list, one of them inside a render `.map()`.
- `SELECTOR_SEARCH_STALE` for three copy-pasted `15 * 1000` literals. They are
deliberately shorter than `SELECTOR_STALE`, so this is a new named constant
rather than a fold into the existing one.
- Tailwind classes for the static half of two duplicated anchor styles, keeping
only the genuinely dynamic `left`/`top` inline.
- Dropped the unused `catch` bindings on three intentional JSON-parse swallows.
`panel.tsx`'s run-button gate loses a `TODO`-stubbed `hasValidationErrors =
false` and the `isWorkflowBlocked` term built on it. That term was dead twice
over: it reduced to `isExecuting`, and the enclosing expression is already
guarded by `!isExecuting`.
* fix: guard the registry lookups, and scope the search-stale doc to its callers
`getBlock` normalizes its argument with `type.replace(...)`, so it throws on
`undefined` where the `getAllBlocks().find(...)` it replaced returned
`undefined` harmlessly. Both call sites can be reached without a type:
`tool-input` reads `state.blocks[blockId]?.type`, which is undefined once the
block is deleted while the panel is mounted — and `Record` indexing hides that
from the compiler, so it would have thrown during render. `agent-handler`'s
`tool.type` is optional and the compiler did catch it.
Also index the skill lookup in `resolveSkillsLabel`, which runs a `.find()`
inside a `.map()` for every block on the canvas — the case the memoised map in
`skill-input` addressed for one component while leaving the hot path.
`providers/utils.ts` keeps its `getAllBlocks().find(...)`: it takes the
registry as an injected dependency precisely so a client-reachable module never
imports it, and reaching for `getBlock` there would cross that boundary.
The new constant's doc claimed search-backed selectors take a shorter window.
Several still sit on `SELECTOR_STALE`, so it now describes the value its three
callers share rather than asserting a rule the tree does not follow.
* fix: guard the second registry lookup in tool-input
`selectedTools` validates only `value[0]?.type` and then casts the whole array,
so a persisted workflow whose later rows lost their `type` yields `undefined`
here — the cast is what makes the compiler believe otherwise. `getBlock`
normalizes with `type.replace`, so that throws during render.
An orchestration result carrying `errorCode: 'internal'` holds whatever text
the fault happened to have — `workflow-lifecycle.ts` catch-alls return
`toError(error).message`, which is the driver's failed SQL. Three application
helpers projected that straight into an `OrchestrationError`, and the internal
route policy rendered its message into a 500 body, so raw SQL reached clients.
The v2 envelope already scrubbed the same failures; internal routes did not.
`messageForOrchestrationError` already encoded the rule and two sites honored
it. The three that hand-rolled it disagreed, and `workflow-vfs` disagreed with
itself: it defaulted the code with `?? 'internal'` but compared the raw
`errorCode` against `'internal'`, so an uncoded failure was classified
internal and still rendered its own message.
Pair the two in `throwOrchestrationFailure` so a code and its message cannot
disagree, and scrub at the internal route boundary as well, matching v2 — no
call site authors a curated `internal` message, so nothing legitimate is
masked, and site N+1 cannot reopen this by forgetting the rule.
Fail-open on unrecorded durable provenance rests on one compensating
control: the audit entry telling the people who own the secrets that a
read proceeded unvouched. An audit of all four surfaces found the
control incomplete in exactly the places this closes, and confirmed the
policy itself sound — so nothing here changes what any read or write
does, only what gets recorded about it.
Knowledge was the one surface with no audit trail at all: the
per-record import reports without a workspace, and the report skips the
audit row when it cannot name one, so fail-open knowledge reads emitted
one error log line per record and zero audit entries. Both importers
now count unrecorded records while the surface is open and report once
per read with the workspace, actor, and count — the shape memory and
tables already use. The search read reports once across chunks and
rendered metadata, and only when the registry did not latch, since a
latched read never reaches a model. Fault returns stay silent; those
reads fail closed.
Memory had the one silent local degrade: a record whose canonical hash
outgrows its bounds, or whose entries fail normalization, was stored
unknown with nothing logged anywhere — the table writer logs its
equivalent. The binding now logs the cause at error where it is
decided. An incoming unknown stays silent; its producer already
reported.
The memory list contract gains the page ceiling every other list
already has (max 1000, matching the table convention); no caller in
the repo passes a limit at all, and the route is internal-auth only.
Workspace-file audit rows now carry the acting user where the caller
already holds one — copilot vfs, the agent and mothership handlers,
and the provider attachment filter. Everywhere else, including
principals with no user to name, the actor stays null, which the
report type has always permitted.
Two comments catch up with the code: the file sidecar stores three
statuses since the absence/taint split, and the mounted-file scanner's
scan-overflow-to-taint is deliberate where the registry scan
over-approximates — that scan only narrows an already-sound candidate
set, while this one decides whether egress redaction would suffice for
bytes the same matcher just failed on.
* fix(files): download a markdown file as a zip only when it really has assets
A document that merely mentions an embed URL in prose or an inline code span
counted as having attachments, so any document about the files API downloaded
as a zip whose assets/ folder was empty.
- Detect embeds with the markdown lexer instead of scanning raw text, so only
real image embeds count: prose, code spans, fenced samples, and links no
longer do
- Choose the export format after resolving assets rather than from the
candidate count, so a missing, unreadable, or oversized embed falls back to
the plain document instead of an empty zip
- Move the document scan out of the copilot tool tree into lib/uploads/server,
where both file routes already live, and drop two pass-through wrappers
- Share one <img> src reader between the clipboard handlers and the scan
- Walk tokens explicitly: marked's walkTokens concatenates per token and costs
O(n^2), measuring 5.4s on a 254KB document against 14ms here, on a path
anonymous public-share traffic reaches
* fix(files): keep an embed id spelled as the document spells it
Decoding the id let a percent-encoded embed resolve and bundle its asset while
the rewrite, which searches the document for that id, found nothing — the zip
kept an API URL that renders as a broken image offline. Keys stay decoded;
they are matched against stored keys, not against document text.
* fix(files): resolve an export asset by its stored id, rewrite by its spelling
An embed carries two representations and they are not interchangeable: metadata
resolves by the stored id, while the rewrite finds the embed by searching the
document for the spelling it used. Using one for both either drops a
percent-encoded asset or bundles it behind a link still pointing at the API.
* fix(files): resolve an embed by its stored id wherever one is read from a document
The export bundler decoded an embed's spelling before looking it up, but the
file-agent's embeddability warning did not, so a percent-encoded embed the
export resolves and bundles could still be reported as one that will not
survive an export. Both now share one helper.
Request-supplied ids are untouched: their route contracts already constrain
them to the plain id charset, so there is no spelling to decode.
`normalizeLinkHref` rejected only `file` for a `scheme://` target, so any other
scheme was returned unchanged. `scheme://` is well-formed for every scheme, so
the check let through spellings that are not navigable targets at all.
- Keep a scheme only when it is http(s), ftp(s), mailto, or tel; drop the rest
- Leave an existing link alone when a committed target normalizes away, rather
than unsetting it — the editor seeds that field with the current href, so
committing an untouched one previously removed the link
Detection is unchanged for relative, anchor, protocol-relative, and bare-domain
targets. A document's stored markdown is untouched: normalization runs on the
render and edit paths, never on parse or serialize, so a target that is refused
still round-trips verbatim.
* fix(inbox): stop an unattributed sender inheriting owner write authority
resolveInboxExecutionActor refuses to name a raw-secret actor when the sender
matches no workspace member, then hands the run ws.ownerId for everything else.
That identity also supplies userPermission, which is what executeTool gates on,
so the owner's admin satisfied every requiredPermission check.
In headless mode the client-routed workflow tools fall back to their registered
server handlers (see the comment in tool-executor/executor.ts), so create_workflow,
edit_workflow and run_workflow — all requiredPermission 'write' — were reachable.
runWorkflowFromCopilot then executes with enforceCredentialAccess and the owner as
actor, which resolves the owner's workspace and personal secrets. An allowlisted
external correspondent could therefore reach, through a workflow it had the agent
build and run, exactly what the null secret actor refuses for a direct mount.
Cap the run's tool permission at read when no member owns the message. An
attributed message is unchanged and still uses the sender's own permission, so a
read-only member emailing the inbox still cannot run or edit anything. Read rather
than none because answering an external correspondent from workspace context is
the point of the inbox; only mutation and execution are withheld.
The owner identity itself stays: billing attribution and workspace reads need a
real user. This separates that need from the authority that came with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(copilot): bar the headless client-tool fallback below write
Client-routed tools carry no catalog requiredPermission because the browser runs
them through the workflow APIs, which authorize the caller's own session. The
headless fallback in executeTool has no session and runs under the request's
principal instead, with nothing standing in for that check.
So the read cap from the previous commit did not reach run_workflow,
run_workflow_until_block, run_block or run_from_block: all four are route
'client' with no requiredPermission, unlike create_workflow and edit_workflow.
An unattributed inbox sender could therefore still run an existing workflow,
which executes with enforceCredentialAccess under the workspace owner and
resolves the owner's workspace and personal secrets.
Derive the requirement at the gate instead: a client-routed tool taking the
headless fallback requires write. Interactive callers never reach this branch,
so the browser path is unaffected. The catalog itself is generated from the
copilot contracts repo and cannot carry this rule, which only applies to the
fallback.
Also corrects the inboxToolPermission doc, which claimed run_workflow gates on
requiredPermission 'write'. It does not; it is gated here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* improvement(perf): eight verified cuts to workspace cold-load JavaScript
Second round of load-time work, adversarially verified for strict behaviour
preservation before implementation. Each item is an import-graph fix — none
changes what renders, when it renders, or any data path:
- knowledge/[id] imported one modal through the [documentId] components
barrel, which also exports the chunk editor and therefore js-tiktoken
(~2.5 MB gzip of BPE tables) on a route that never edits chunks. Deep
import.
- prepareBlockState moved out of stores/workflows/utils.ts into its own
module. It is the only function there needing the block registry and the
generated tool-outputs artifact (~476 KB gzip), and utils.ts is reached by
the persistent shell — so every workspace route paid for a canvas-only
helper, including a module-scope JSON.parse of a 5.4 MB string.
- ExecutionSnapshot (the frozen-canvas modal) is now React.lazy behind its
interaction gates, per the code-splitting procedure in sim-imports.md:
deep import, dead barrel re-export deleted, sibling imports in log-details
deepened to break the parent->child barrel cycle, local Suspense at both
render sites. Takes ~7.6 MB of source off logs hydration.
- The api contracts barrel no longer re-exports ./tools, ./selectors, ./v1,
or ./demo-requests (~58 KB gzip of Zod schema construction on every
route). Zero importers used the barrel path for any of them.
- createCsvParser (streaming csv-parse, a Node Transform) moved to a
server-only module so its stream polyfill leaves client bundles.
Deliberately not re-exported from the lib/table barrel.
- jszip is dynamically imported at both remaining static call sites (skill
zip extraction, pptx parsing) — both already-async, user-triggered paths,
mirroring the existing pattern in workflow import-export.
- The desktop local-filesystem tool executor is dynamically imported in
use-chat; a chunk-load failure now reports an error completion so the
server-side tool call settles instead of hanging.
Production build, JS downloaded before the load event, vs the previous
release:
/home 4.44 -> 3.87 MB /logs 4.44 -> 3.64 MB
/knowledge 4.22 -> 3.68 MB /tables 4.17 -> 3.61 MB
/files 4.68 -> 4.10 MB /w/[id] 4.80 -> 4.67 MB
/home total after idle prefetch: 8.15 -> 5.52 MB
The lazy snapshot was exercised end-to-end: its chunk loads when a log
detail opens (off the route's cold path, warm before the View Snapshot
click) and the modal renders without errors. Boundary baseline retightened.
* improvement(logs): contain snapshot chunk-load failures and settle the local-fs tool on recovery failure
Review round: wrap both lazy ExecutionSnapshot render sites in a small error
boundary (Suspense handles the lazy import's pending state, not its
rejection — a failed chunk load would have unwound to the route boundary and
replaced the logs page over an optional modal; mirrors PreviewErrorBoundary),
and contain rejections inside the local-filesystem executor's load-failure
recovery so a failed completion report degrades to a log instead of an
unhandled rejection.
* fix(logs): recover cleanly from snapshot chunk failures
* fix(logs): preserve snapshot modal while loading
* fix(copilot): enforce delegated workspace scope in query_logs and set_environment_variables
A model-supplied workspaceId (or a workflowId in another workspace) could
steer both tools to any workspace the acting principal can reach, bypassing
the asserted-vs-context workspace comparison the rest of the Copilot tool
surface enforces. Both now resolve through requireCopilotWorkspace — moved
to a shared module — so an asserted workspace may only re-state the chat's
execution workspace, and the default-workspace fallback is removed so a
missing scope fails closed.
* fix(copilot): apply the workspace-scope guard across every model-steerable copilot surface
Extends requireCopilotWorkspace to the remaining copilot tools that resolved
their target workspace from model-supplied arguments: get_credentials (a
workflowId could steer the credential listing to any workspace the user can
access) and publish_custom_block (a workflowId could deploy/undeploy custom
blocks from another workspace's workflow). The handlers already protected
downstream by the application adapter (create workflow, generate API key,
list/create workspace MCP servers) now use the same guard so a mismatch is
rejected uniformly at the surface, and the getDefaultWorkspaceId fallback is
deleted entirely — no copilot path picks a workspace for the model anymore.
* refactor(copilot): classify both workspace-scope guard branches and drop call-site boilerplate
requireCopilotWorkspace now accepts an undefined context and throws a
classified OrchestrationError for the missing-workspace branch too, so every
caller drops the 'context ?? {}' and '|| undefined' coercions and one
instanceof covers the guard. query_logs inlines its now-one-line wrapper,
get_credentials drops the workspace-less special case (a workflow with no
workspace asserts nothing), and publish_custom_block handles the guard
locally instead of widening its catch-all — keeping its deliberate
assume-not-published guidance for unrelated failures.
* chore(copilot): drop call-site comments that restate the workspace-scope guard's TSDoc
* test(copilot): type the new query-logs scope tests instead of casting to any
* fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL
The desktop connect launcher passed better-auth a same-origin path as its
callbackURL. Better Auth stores that value verbatim in the OAuth state, and
the callback's credential-draft reader parsed it with a bare `new URL()`,
which rejects a path. That throw happened inside the `account.create.before`
database hook, which better-auth's OAuth callback does not guard, so the
provider redirect landed on a 500 after authorization had already succeeded.
Send an absolute URL from the connect page, matching the workspace-scoped
branch and every other connect surface, and accept a path-absolute callback
URL in the draft reader so the shape can never fail the callback again.
Protocol-relative and malformed values still throw, keeping an unreadable
binding loud.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(desktop): compose the connect completion URL through the URL API
Concatenating `getBaseUrl()` with the completion path leaves the result
dependent on how the deployment spelled `NEXT_PUBLIC_APP_URL`: the helper only
adds a missing protocol, so a trailing slash produced `//desktop/connect/complete`,
a pathname that matches no route. The completion page is what bounces the OAuth
result to the desktop app's loopback, so that typo would have stranded the flow
just past the callback it was meant to fix.
Both callback URLs in the page — the launcher's and the workspace-scoped
authorize redirect's — now go through one helper that resolves the path against
the base with `new URL`, matching how the same function already builds the
authorize URL, with coverage for a trailing-slash base.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(urls): give base URLs the no-trailing-slash form their call sites assume
`getBaseUrl()` returned `NEXT_PUBLIC_APP_URL` as the operator spelled it, while
almost every consumer builds `${base}/path`. A base configured with a trailing
slash therefore produced a `//path` pathname that matches no route, and broke
the `startsWith(`${base}/`)` prefix checks that decide whether a redirect target
is our own — the OAuth authorize route rejected its own completion callback and
fell back to the workspace page, so the desktop handoff never ran on those
deployments. The previous commit fixed one such URL; this fixes the reason it
was wrong, for the ~30 concatenation sites that share the assumption.
`normalizeBaseUrl` now strips trailing slashes alongside the protocol it already
added, which is the invariant SITE_URL has always documented. A path-prefixed
base keeps its path. `getInternalApiBaseUrl` gets the same treatment, since its
callers concatenate identically.
`@sim/testing`'s urls mock is a hand-written mirror of this module, so it moves
in step. `internal-api-base-url.test.ts` now unmocks the module it names —
otherwise it asserts against that mirror and any drift between the two passes
unnoticed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(urls): stop claiming path-prefixed base URLs are supported
The previous commit's doc and test said a path-prefixed base keeps its path.
That reads as support for a deployment shape the app does not have: there is no
Next `basePath`, so routes are served at the origin root and such a value could
not address them however the base were normalized. Every documented example is
origin-only.
Says only what is true — trailing slashes are the one spelling absorbed — and
reframes the test as pinning the trim's shape rather than asserting a
path-prefixed deployment works. No behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* improvement(secrets): gate Copilot code mounting at use level
Mounting a saved secret into Copilot code required credential-admin on that
key, while a workflow Function block resolves the same secret for the same
person at use level through getPersonalAndWorkspaceEnv. Copilot reaches that
path itself — edit_workflow plus run_workflow — so the admin bar contained
nothing. It redirected a Credential Member through a detour that mutates a
persisted workflow, while the direct path is ephemeral and files a usage row.
The inconsistency was also internal to Copilot: the secret names advertised to
the model come from getAccessibleEnvCredentials and getPersonalAndWorkspaceEnv,
both role-agnostic, so Copilot listed every secret the caller could use and
then refused to mount all but the admin ones.
Widen the workspace and shared-personal predicates to any active grant, and
drop the matching role filter from the query. Workspace write is still
required, revoked and pending grants are still refused, and a caller with no
grant still gets nothing.
The view gate stays where Copilot cannot route around it: values remain masked
under Settings, and See usage remains admin-only, so a member's use is
recorded for whoever can rotate the key. Model-egress projection is untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(secrets): stop implying Personal secrets are shareable
The Copilot code-execution paragraph listed "any secret shared with you as a
Credential Member or Credential Admin" among what mounts, which reads as though
a Personal secret can be shared. It cannot through any product surface:
CredentialMembersSection renders only for workspace secrets and OAuth
credentials, and the personal-credential sync only ever grants the owner.
Narrow the sentence to Workspace grants. The comparison table's "Only you can
use" row for Personal was correct and is left alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(deployments): stop superseded activations from dead-lettering
29 workflow.deployment.prepare.v2 events dead-lettered with "Webhook
registration operation is stale", every one at attempts = max_attempts. A full
retry budget means the failure is deterministic, which rules out the
preparation path: an attempt superseded while preparing is marked superseded,
so its next attempt short-circuits at the top of the handler and completes.
The branch a retry re-enters is the other one. isTerminalNonActiveOperation
covers failed and superseded but not active, so an attempt that activated and
was then superseded by the next deploy keeps its own active status, re-enters
post-activation work on every retry, and re-fails the same generation fence
until the event dies. The fence it fails is correct — it takes the same
workflow row lock the generation bump takes, and compares generations exactly
— so nothing about the detection is racy; only the reaction to it was wrong.
Reaching it needs a handler timeout, which parks the row for the 10-minute
reaper instead of the 2s/4s/8s backoff, opening a window wide enough for a
redeploy to land.
Gate the resume branch on the operation still owning the current generation,
matching the sibling cleanup that already does this, and complete the event as
a no-op when it does not. The newer generation adopts the leftover work
anyway: it collects every retired registration below its own fence.
Also reverse the post-activation order. The audit entry, analytics event,
socket notification, and workspace event describe a cutover that is already
durable, and each is separately checkpointed, but they ran behind retiring the
previous generation's external subscriptions — one provider call per retired
row, and by far the most failure-prone step there. A single flaky provider
silently cost the deploy its audit trail and left clients on the old version
until something else refreshed them. Both call sites now share one helper so
the order cannot drift apart again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(deployments): pin staging's active-resume tests to the current generation
Two tests that staging added alongside durable PostHog delivery drive the
`operation.status === 'active'` resume branch this PR now gates, and neither
overrides `mockIsDeploymentOperationCurrent` — the suite's `beforeEach`
defaults it to `false`. Under the new gate they read as superseded, so the
handler short-circuits: the delivery test sees a resolve where it asserts a
rejection, and the unconsumed workflow row leaks into the next test.
Both are still-current resumes by intent, so they say so explicitly. The
checkpoints, not the generation gate, remain what keeps analytics from being
captured twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(analytics): stop PostHog delivery from failing a deploy
`deliverOutboxServerEvent` awaited `client.flush()` before letting the
deployment outbox checkpoint advance, so an unreachable PostHog failed the
event, retried it, and eventually dead-lettered it — while holding the socket
notification, the workspace event, and retired-subscription cleanup behind a
third party. `flush()` also drains the whole shared client queue, so an
unrelated event's network error surfaced here as a failed deploy.
It bought no durability the process did not already have: the outbox handler
runs in the long-lived app container, where the client flushes on its own
10s interval and again from the `SIGTERM`/`SIGINT` hook in
`instrumentation-node.ts`. The helper had one caller and arrived inside an
unrelated squashed PR (#5273) with no rationale, against 161 fire-and-forget
`captureServerEvent` call sites.
Deleted it and restored `captureServerEvent`, whose contract is already
"never throws". `insertId` still collapses retried captures. That contract
was untested, which is why this regressed unnoticed, so `server.test.ts` now
pins it — spying on the real client, since the lazy `require` defeats
`vi.mock` and a disabled client would pass every assertion vacuously.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(deployments): fence the cleanup, not the notifications
The resume guard sat at the top of the `active` branch, so it skipped every
post-activation step whenever `isDeploymentOperationCurrent` went false. That
predicate goes false as soon as any newer generation row exists — including one
still `preparing` or already `failed` — and in that window this activation is
still the live cutover. Nothing newer would ever adopt its audit entry,
analytics event, socket notification, or workspace event, so the guard
permanently dropped them and completed the outbox event as if they were owed to
someone else.
Only the two cleanups are generation-fenced. `cleanupInactiveDeploymentsForOperation`
already gated itself on that exact predicate and returned quietly; the retired
webhook cleanup was the one that instead let the store's `assertCurrentOperation`
throw. It now carries the same guard, on the same fence the store asserts —
`deploymentVersionId` and `statuses: ['active']` included, so passing the gate
actually implies passing the assert. The notifications run unconditionally,
still idempotent through their checkpoints.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`convert` and `extract_audio` built their output path as
`path.join(dir, `out.${format}`)`, and `format` arrives from the tool as an
unconstrained `{type: 'string'}` — no pattern, no enum, and the handler passes it
through untouched. A format of `../../../../../../../../tmp/x.mp4` resolves to
`/tmp/x.mp4`, so FFmpeg writes attacker-influenced media wherever the traversal
points. Verified against a real binary: the run produced a 44,078-byte MP4
outside the temp directory, which the `rm -rf` cleanup then never saw, because
the file was never inside the directory being removed.
Output extensions are now letters and digits only. The pattern admits no `.` and
no separator, so `out.${ext}` is always a single path segment and containment
follows from the validation itself rather than from a second check that could
drift away from it.
`trim` and `fade` build their paths from `extFromMime`, which returns
`mime.split('/')[1]` — that can never contain a separator, so those stay inside
the temp dir and keep accepting values like `x-msvideo` that this stricter
pattern would wrongly reject.
Waleed