v2 overhaul: surface UI, workflows, templates, teams, Slack - #259
Merged
Conversation
# Conflicts: # packages/web/src/api/client.ts
- Validate schedule name (trim, reject blank) and input (must be an object) instead of persisting junk into every scheduled run's trigger payload. - Delete a workflow's schedules when the workflow itself is deleted, matching the existing webhook cleanup. - Scope the schedule DELETE route's ownership check to a WHERE predicate instead of an org-wide scan-and-.some(). - Dedupe the webhook URL builder (webhookUrl in actions.ts duplicated workflowWebhookUrl in webhook-service.ts); the HTTP routes now return the same absolute url the agent-facing tool already computed, with the request's own origin as a fallback when no public origin is configured.
PATCH/DELETE on a personal (user-owned) subscription now 404 for any caller but its creator, matching the cross-owner "not found" convention used elsewhere. Previously any org member could enable/disable or delete a colleague's personal subscription through the org-scoped routes. An org-owned subscription stays mutable by any org member.
dev-seed-linear.ts passed PGlite straight to PgCredentialStore, whose PgQueryable contract requires query() to return rowCount — PGlite's result type doesn't have one, so this broke the repo's `tsc --build`. Adapts through a thin query wrapper instead. dev-events-smoke.ts probed three guessed shapes for the webhook mint response (hookId/secret/parsed-from-url); imports the real WorkflowWebhookResponse type instead.
New reusable pieces, extracted after the events+triggers+teams panels independently reinvented the same patterns: - OwnerPicker: the caller-or-team select used by the workflow and skill editors. Filters to teams the caller can see AND is a member of (callerRole !== null) — the prior two copies each offered every team an org admin's useTeams() returns, including ones they aren't a member of, which the create routes then 404 on. - SelectMenu: the trigger-button-plus-option-list pattern behind the event feed's service/key filters and the subscription dialog's workflow picker (three near-identical DropdownMenu blocks before this). - useCopyToClipboard: extracted from tool-shell's CopyButton (which now uses it too) so the workflow webhook section doesn't hand-roll its own copy-state timer, and so both handle a denied/unavailable clipboard. - TabBar: adds the ARIA tabs keyboard contract (arrow keys move focus and selection with roving tabindex, Home/End jump to the ends) and an aria-controls/tabPanelId pairing for consumers to wire a real tabpanel.
- new-workflow-dialog and skill-editor now share OwnerPicker instead of each keeping its own copy-pasted <select>, and route their create errors through errorText instead of showing raw error.message. - Webhook rotate/delete: the ConfirmDialogs now receive the mutation error (previously invisible, rendered below the modal overlay) and the initial "Create webhook URL" failure has its own error line. useMintWorkflowWebhook seeds the query cache from its response so a rotate doesn't keep showing the just-revoked URL until the refetch lands; useDeleteWorkflowWebhook invalidates onSettled so a failed delete still reconciles. - Schedule delete now confirms (it was one click, unlike every other destructive control added this pass) and surfaces its error; the empty-state message no longer renders alongside the error message on a failed fetch; useDeleteWorkflowSchedule invalidates onSettled.
mutations by owner, close remaining error/reset gaps - Feed's two filter dropdowns and the create dialog's workflow picker now use SelectMenu instead of three copies of the same DropdownMenu block. Added a truncation notice when the feed is at the server's 50-event page size (no pagination yet) instead of silently looking complete. The load-failure message now names the Refresh button as the corrective action. - listEvents built its query string with a URLSearchParams.size check — unsupported by some engines, which would silently drop the service/key filters; switched to .toString(). - The subscriptions list disables the enable/disable switch and hides the actions menu for a subscription the caller isn't allowed to mutate (mirrors the new server-side gate), and the switch surfaces a failed PATCH instead of silently snapping back. - The create dialog now resets its form and error state on any close path (Cancel, overlay, Escape), not just a successful submit — it stays mounted between opens. - teams-panel: dropped a dead Spinner import and routed its two raw error.message displays through errorText. Also adds the ARIA tabpanel half of the /events tab strip (role, aria-labelledby) to match TabBar's new aria-controls wiring.
…ent-native run status Splits the 522-line node inspector into one form file per node type (mirrors the tool-renderer registry pattern). Consolidates four duplicate date formatters onto lib/format-when.ts and three hand-rolled clipboard handlers onto lib/use-copy.ts (upgraded with the execCommand fallback one of them had, so nothing regresses). Upgrades the run-detail checkpoint list from a plain-text status list to color/glyph-coded rows sharing the canvas's own NodeRunStatus vocabulary, with failed nodes auto-expanding their result and everything else collapsed by default. Gives the pending- approval card real visual prominence instead of a plain bordered box. Demotes the editor toolbar's least-used action (version history) into an overflow menu, cutting the primary row from 4 buttons to 3.
engineToWireParts previously dropped thinking parts alongside attachment/error. Mirror them onto the wire and render a collapsed-by-default disclosure in the transcript.
Consolidates markdown-fenced code onto one component: selective Prism language registration (a few KB, not the ~290-language bundle) and CSS-token-based coloring so blocks track the app's light/dark theme instead of a fixed prebuilt Prism theme. Also removes @fontsource/newsreader, dead since the brand-refresh swapped the display font family; corrects that spec's earlier (wrong) claim that the font was never loaded.
Stamp turnStartedAt in the stream store on the idle -> non-idle transition (cleared on turn_end/init), and render it as a live ticking counter on the agent status badge while a turn is running.
Reuses the existing OwnerPicker component (already used for workflow ownership) so a skill source can be imported as team-owned, with a badge on the row showing which team owns it.
createWorkflowSchedule always wrote ownerType "user" regardless of who actually owned the workflow being scheduled. Derive it from the workflow's real owner instead (team ownership maps to org, since the schedule owner enum has no team value).
Team orchestrators were structurally real (workflow dispatch could already wake one) but had no way to view them: nothing created the app-row on first access, and the read routes only checked direct session ownership. Adds POST /api/teams/:id/orchestrator (get-or-create, reusing the existing ensureOrchestratorSession helper) and widens GET /api/sessions/:id plus the messages/threads/decisions routes and the WS handshake to also allow a caller who can view the owning team. Read-only and team-scoped — no mutating session route changes, no org- level access. TeamsPanel gets an "Assistant" button that opens the team's orchestrator session, visible to any member.
A team-owned session's `agent_sessions.userId` records whoever opened the team's assistant first — `ensureOrchestratorSession` stamps the actor. The model, pause and delete routes read that stamp as ownership, so the first member to open a shared agent could pause or delete it, and every other member got a 404. Adds `canAdministerSession` (team-owned => team admin or org admin) as the deliberate mirror of `canViewSession`, and extracts `canAdministerTeam` so the team mutation routes and session administration share one definition. Auto-title had the same filter, which left a member's threads permanently untitled. Titling is part of prompting, so it moves to `canViewSession`.
A team's assistant was reachable only at Settings > Organization > Teams, behind an org-admin gate most members fail. Teams are not a setting, so each team you belong to now gets a permanent row in the /chat sidebar beside your own assistant, addressed by `?team=`. The session id is derived client-side, so browsing the rail creates nothing; /chat ensures the session only when you open the conversation. A strip above the composer names who else can read what you type. Also fixes the header titling a team assistant with the VIEWER's own assistant name, and hides the lifecycle controls the API now refuses to plain members. The settings button stays as a cross-link, not a second door. A dashboard card covers the collapsed-sidebar case; the usage card's "Team" heading becomes "Organization", which is what it always listed. Nothing renders for a caller with no teams.
Workflows, skill cards, skill docs and skill repositories each resolved the owning team's name for themselves. Four copies had drifted into four looks, and the skill card never resolved the name at all — it rendered the literal word "Team" and never called useTeams, a bug skill-doc and the skill detail page both inherited through the same helper. The badge links to that team's assistant, so the flat lists people already open become a second way in. The skill card's whole-card link becomes a stretched overlay so the badge stays clickable without nesting anchors.
A subtraction pass over the whole product. Every candidate was searched for independently and then argued against before it was cut, so roughly forty proposals were talked back down — the /primitives showcase is the brand-refresh verification surface, the /orchestrator redirect still serves bookmarks from the deployed client, and the pause control's 409 only fires on the docker dev backend. What went, ~620 lines: - The legacy prebuild/image-catalog wire block, whose routes were unmounted long ago. docs/api-reference.md still advertised three of them; two service doc comments and an e2e header named others. - WorkflowRunDetail.signals, fetched on every five-second run-detail poll and read by nobody. - The EditorModel class and applyAutoLayout; the sidebar drag-to-resize machinery (its pseudo-element also drew the aside's only border, now a real one); nine Hono RPC type aliases with no RPC client. - SessionViewVariant, a three-value enum branched on twice for the same value, now a boolean. - Header count strips on Skills and Integrations, the model picker's keyboard legend, a read-only Created row, and a run status rendered twice beside its own badge. Also folds two copies of the blank workflow definition into one. The surviving copy seeds node positions, which the live one did not. Deferred deliberately: the app messages table (its migration forces a local database wipe), the sdk analytics export (published, so an out-of-repo consumer could break), and repo-wide noUnusedLocals.
`badge.tsx` styled two of its variants with `bg-success-500/15` and `bg-danger-500/15`. Those scales are raw `oklch(...)` strings in the Tailwind config, so the slash modifier produced NO rule at all — checked against the running stylesheet, neither class exists — and both badges rendered as bare coloured text with no pill. theme.css already documents this trap and the way out: bake the alpha into a pre-mixed wash token. The `accent` variant was fixed that way; these two were missed. Adds --success-wash, --danger-wash and their dark values, and points the variants at them. Adds a `warning` variant on the same footing. Amber is the palette's documented "waiting" colour, so it carries the states that are blocked on a person — which the run-state work needs and would otherwise hand-roll.
`handleDecisionGate` returned early unless the gate came from a child session, so a gate raised in a standalone session or in a user or team orchestrator routed no attention at all. The session blocked on a person and told nobody, indefinitely. The child-to-parent routing was right and is unchanged — a child has no independent audience. It was just not the only case. Every other session now notifies its own owner, the same way `handleSubmissionStuck` directly above already did. A child whose parent row is missing falls back to its own owner: a weaker audience beats silence. The replaced test asserted the old behavior as if it were correct.
The Sessions list could only report the row's lifecycle (active, hibernated), never what a session was DOING. Five surfaces were each about to invent their own word for "working". Adds `SessionRunState` to the wire — needs_you > working > failed > sleeping > idle, one precedence, derived in one pure module and used by the list, the detail routes and the WS init frame so no two surfaces can disagree. `RunStateBadge` renders it; "Needs you" names who is blocking rather than the mechanism, and takes the palette's waiting colour. The list stays at three round trips whatever the session count — one cross-session read of unsettled submissions, grouped by id. A per-row query would have made an ordinary list cost one query per session. Two limits are real and documented rather than papered over: a pending gate is read from the `blocked_on_decision_gate` submission status because no cross-session gate read exists, and `failed` only shows while a failed turn is terminalizing — settled outcomes are readable one session at a time, so a session whose failure already settled reads idle. `initialPrompt` shipped on the wire documented as "server enqueues immediately after creation" and was read nowhere. It now enqueues through the same path the messages route uses, extracted rather than duplicated. A failed enqueue does not fail the create: the row exists and the caller has its id.
The engine already accepts a mid-turn message; the composer refused to send one. It now sends in every state, and the label comes from the live queue mode rather than an assumption. That distinction matters more than it looks. `EngineHost` gives a user's own orchestrator the `steer` mode and every other principal `followup` — team orchestrators included — so a fixed "Steer" label would promise an interrupt that never happens on exactly the team assistants the sidebar just made prominent. The steer copy is literal. A steer admission supersedes the running submission and aborts the live run, so it says the turn stops and the new message starts, not that it joins the turn in progress. Also keeps `ev.error` on a settled submission. The wire carried the reason and the store dropped it, leaving a grey "failed" with nothing behind it.
A background agent that blocks on a person and cannot reach them is this product's core failure mode: the work stops, and the only way to find out is to go looking. The bell already collected these, but a badge helps nobody who tabbed away — which is the entire point of a background agent. Only the kinds that block on a person ring: question, escalation, approval. A general update goes to the bell and the title, never the speaker. A sound that fires when nothing is needed is a sound people switch off, and then the one that mattered is silent too. It also stays quiet when you are already looking at the page the notification points to — the decision card is on screen and a chime tells you nothing you cannot see. One ping per burst, and nothing on first load, so a refresh does not replay the backlog as fresh alarms. The tone is synthesised rather than shipped as an asset: two short sine tones rising a fifth, low gain, smooth envelope. Browsers block audio until a real gesture, so the context resumes on the first interaction and the document title carries the count regardless — that is the signal that survives a muted tab, blocked autoplay, or headphones out. The device-local toggle lives in Settings, on by default. Whether a machine may make noise depends on where it is, not on who signed in. Also clears a close-on-blur timer in the new-session combobox on unmount. It set state on an unmounted tree and threw after teardown, which had been raising a permanent error in the suite.
Creating the Slack app by hand means transcribing scopes, events and two request URLs into a form, where one wrong entry fails silently later. `GET /api/org/slack` returns a manifest an org admin pastes into Slack's app-creation form instead. Slack's `apps.manifest.create` needs an app-configuration token this deployment does not hold, so the flow is paste-in rather than API-driven. The scopes, the bot events and the ingress path live in one module because three call sites must agree or the integration breaks with no visible error: this route hands them to the operator, the credential save checks the installed app really granted them, and the webhook route only ever receives the events declared here. A scope in the manifest but not in the save-time check surfaces weeks later as an opaque `missing_scope` from one API method. The manifest targets Slack's current agent messaging experience. The feature key is `agent_view`; the older `assistant_view` is deprecated and its nested description is renamed. `messages_tab_read_only_enabled` is forced false — Slack's own scaffolding defaults it true, which disables the composer so the user cannot type to the agent, and nothing reports why. A deployment with no public URL gets a Socket Mode manifest, because there is no URL Slack could reach. The request URL is composed from the same constants `app.ts` mounts, so what an operator installs cannot drift from what the api serves. The response also reports connection state and any requested scope the install withheld, read from the scope list recorded at connect time rather than a fresh call to Slack.
Every way of misconfiguring Slack fails silently and late. A wrong signing secret only shows up as 401s on an unauthenticated webhook nobody watches. A user token pasted instead of a bot token posts as that person. A workspace that never granted `assistant:write` fails on the first `assistant.threads.*` call, hours after the operator left the settings page. `PUT /api/credentials/slack?scope=org` now pays one round trip to `auth.test` and turns each of those into an error the operator reads immediately, each naming the fix. A user-scoped Slack credential is a personal token for the action plugin and is left alone. The same call records the identity the rest of the integration needs. The workspace id is the security boundary the webhook route enforces: a Slack app's signing secret is valid for every workspace that installs the app, so a valid signature alone does not prove an update is ours. The bot user id is what the transport suppresses its own echo on. The granted scopes are stored so the setup route can report missing ones without calling Slack again. A missing scope header is treated as unknown rather than as nothing granted, so a proxy that strips it cannot break every save.
The host derived a conversation key from a thread key by assuming the
whole address fits in the thread key: `${channelType}:dm:${rest}`. That
holds for Telegram, where the chat id is the address. It is false when
the key also names a workspace, and the cost is silent: every gate
prompt, command result and attention DM for such a transport is
addressed with a key the transport never minted, so a reply posts
somewhere plausible instead of failing.
The host now asks the transport for both directions of the mapping when
it owns them, and stops rather than guess when the transport disowns a
key. The inbound half uses the same hook, so the two cannot drift apart.
Attention DMs get the same treatment. A sender id is not always an
address — Slack's `U…` names a person while `D…` names the DM with them —
so the host asks the transport to open the direct conversation when it
knows the difference.
A transport factory may now reject a credential it cannot serve. That
rejection is contained per transport: one bad credential must not stop
the transports after it, nor the outbound queue. The message names the
fix and lands in the startup log.
`surface_opened` ends without a drop-log row. Someone opened the
conversation and said nothing, so there is no turn to start, but a row
per DM open would bury the reasons that matter.
The engine publishes `text_delta` through `publishEphemeral`: live-only, no offset, no replay. A provider stream is the opposite — a long-lived object with no documented timeout. Between those two facts sits the ugly case. If the api dies between start and stop, nothing in the event log says a stream was ever open, and the reader keeps a message that shimmers until they give up on it. `channel_active_streams` holds one row per stream the api has started and not yet stopped, so the next boot can close what a previous one left open. The deltas are not recoverable and this does not pretend otherwise: `message_end` is durable and the web session shows the full answer. The table is keyed on the provider's own message id, which is unique per conversation, so a conflicting insert is a retry rather than a second stream and overwrites instead of failing the turn. Nothing here is Slack-specific — any transport that implements the start/append/stop triple is swept by the same code. The store is an interface with a Postgres and an in-memory implementation, so the bridge's own tests need no database.
Replies arrived as one finished message after the whole turn. On a surface that renders text as it is produced, the reader watches nothing happen for as long as the model runs. `ChannelStreamBridge` subscribes to the engine bus and drives the provider's stream instead: open on the first `message_start`, append batched text, close on `message_end`. Most of the code is the other paths, because each one ends with a real person looking at a message that never stops shimmering. The turn fails or is aborted. The reader presses stop. The api restarts mid-stream. The provider rate-limits an append. The turn parks on an approval gate. Every abnormal ending closes the stream with a line saying what happened and what to do next. Two engine facts shape the design. `text_delta` carries no messageId, so the active message is tracked off the preceding `message_start`. And deltas are ephemeral — live-only, no replay — so an open stream is recorded durably and swept at boot. Deltas are batched behind a cadence floor rather than posted one per call, which would breach the provider's one-per-second guidance within the first second of a turn. A flush stops at a clean markdown boundary so the reader does not watch an unterminated code fence, but only up to a bound: text that renders untidily for one interval beats text that never arrives. Long answers roll over to a continuation message before the provider's per-message ceiling turns into an error with no way back. Failures are classified, never guessed. A rate limit keeps every character and waits. A dead stream drops the row and finishes through ordinary messages. A reader pressing stop aborts the turn too, because continuing spends tokens on output with nowhere to go. Anything unclassified is fatal for the stream and the remaining text still goes out. Whatever happens, the answer reaches the reader by some route. Two sweeps cover what the normal path cannot. The boot sweep closes streams a previous api left open. The orphan sweep catches rows this process wrote and then lost, and never touches a stream it is still writing to. A close that keeps failing keeps its row, because the row is the only record the message is open — bounded so a permanently failing close does not leave an immortal row. The host skips its own delivery of a streamed message, or the reader sees the same answer twice. It also closes the stream before an approval card, so the card lands after the text that led to it.
Slack delivers everything an app receives to one app-level URL: Events API envelopes as JSON, interactivity as a form-encoded field. The generic `/:channelType/webhook` route cannot serve it, because one delivery has two consumers and the secret is provider-issued rather than host-minted. This route verifies the signing-secret HMAC once and fans each update out to both: the channel host for the agent surface and approval callbacks, and the trigger definitions plus ingest for workflow subscriptions. It is mounted before the auth middleware because the caller is Slack, and before `channelsRouter` so the more specific path wins. Slack expects a response inside three seconds and redelivers when it does not get one. A turn takes far longer, so the fan-out runs after the response. A redelivery is logged and then processed like any other delivery: Slack redelivers because the first attempt produced no 2xx, so dropping one would turn every transient failure on this route into a message the user typed and Valet silently lost. Processing twice is safe because both consumers dedupe durably on ids Slack repeats across retries. A valid signature is not enough on its own. A Slack app's signing secret is valid for every workspace that installs the app, so an update must also name the workspace recorded at connect time; anything else is dropped. A half-configured org is acked rather than 401'd, so Slack does not retry against an endpoint that will keep failing. The handshake echo answers before verification, because that is how the endpoint is enabled before any credential exists. Ingest gains match-gated persistence for keys marked ephemeral. `slack.message` is a firehose, and subscribing is now what turns it on — an unsubscribed key never touches the events table.
…verhaul # Conflicts: # Makefile # docs/specs/2026-07-13-assistant-centered-web-ui-design.md # docs/specs/2026-07-16-workflows-overhaul-design.md # packages/api/src/routes/messages.ts # packages/api/src/routes/orchestrator.ts # packages/api/src/routes/plugins.test.ts # packages/api/src/routes/plugins.ts # packages/api/src/routes/workflows.test.ts # packages/api/src/routes/workflows.ts # packages/api/src/wire/types.ts # packages/api/src/workflows/service.ts # packages/engine/src/plugin-catalog.ts # packages/web/src/api/client.ts # packages/web/src/components/session/composer.test.tsx # packages/web/src/components/session/message-item.tsx # packages/web/src/components/session/session-header.test.tsx # packages/web/src/components/session/session-header.tsx # packages/web/src/components/session/thread-tree.tsx # packages/web/src/components/session/tool-renderers/tool-shell.tsx # packages/web/src/components/workflows/editor/inspector.tsx # packages/web/src/routes/-integrations.test.tsx # packages/web/src/routes/-workflows.runs.$runId.test.tsx # packages/web/src/routes/workflows.$workflowId.tsx # packages/web/src/routes/workflows.index.tsx # packages/web/src/routes/workflows.runs.$runId.tsx # packages/web/src/stores/stream.ts
…feat/v2-overhaul # Conflicts: # packages/api/src/providers/node.ts # packages/api/src/routes/workflows.ts # packages/api/src/wire/types.ts # packages/api/src/workflows/actions.ts # packages/api/src/workflows/service.ts # packages/web/src/api/workflows.ts # packages/web/src/components/workflows/editor/inspector.tsx # packages/web/src/routes/-workflows.runs.$runId.test.tsx # packages/web/src/routes/workflows.index.tsx # packages/web/src/routes/workflows.runs.$runId.tsx # packages/workflow/src/dag/nodes.ts # packages/workflow/src/interpreter.ts # packages/workflow/src/local-host.ts
The workflows page offered nothing but an empty state. Templates are the starting points, ported from the V1 gallery that was removed from main. Architecture, not code, came across. V1 was Workers, D1 and IntegrationPackage with a draft/publish split; v2 is Hono, Postgres and ValetPlugin with one definition row and immutable version snapshots. What survives is the shape: a plugin owns the templates for the integrations it owns, so enabling a plugin adds its templates and disabling one removes them, and a template is installed by the same create path a user's own workflow takes. Eight templates, each drawn from a use case the team actually runs: a nightly memory sweep, a daily triage digest, weekly meeting prep, one instruction over a batch of rows, a daily development digest, a nudge on quiet pull requests, an inbox sweeper, and weekly issue triage. Install is atomic. Everything after the definition row sits in a rollback, so a validation failure or a trigger collision leaves nothing behind, and webhook paths carry a uuid-derived suffix because they are globally unique. A template whose integration is not connected says so instead of offering an install that would fail. WorkflowInputDefinition regains label, placeholder and hidden, so a template declares each input once in the trigger schema and the run form is derived from it rather than duplicated. Verified by running, not by reading: two templates were installed through the real route on an isolated api and driven to settled/completed, and all 52 template paths across every shipped template were resolved against the real template context. Six templates remain unproven at run time because they need live GitHub, Gmail, Linear or Slack credentials. web 808 passing, workflow 303, engine 39, plugin suites 132, api template suites 155, typecheck clean.
…/v2-overhaul # Conflicts: # packages/api/src/routes/teams.ts # packages/web/src/components/settings/teams-panel.tsx # packages/web/src/routes/-settings.organization.test.tsx
…overhaul # Conflicts: # packages/api/src/channels/host.ts
…v2-overhaul # Conflicts: # packages/api/src/channels/host.ts # packages/api/src/routes/workflows.ts # packages/api/src/wire/types.ts # packages/engine/src/session.ts # packages/web/src/api/queries.ts # packages/web/src/components/session/tool-renderers/tool-shell.tsx
Sixth branch into the overhaul. Templates were built off dev-v2 before the five-branch consolidation, so they arrive as their own merge. Two components were invented in parallel on both sides: both pulled the brand-colour table and monogram tile out of integration-row into their own file. They are not one superseding the other — the colour table, fallback palette and hash are byte-identical. ServiceIcon is the superset (real brand marks for 13 services with a monogram fallback, three tones, and the slug threaded from the wire), and the connect dialog already depends on its slug shape, so the template gallery moves onto it and ServiceMark is deleted rather than left as a second way to draw the same thing. A tool node now carries both onError and the approval-gate fields. They answer different questions: onError decides what a node FAILURE does to the rest of the run, onDeny and approvalTimeout decide what a policy gate's refusal or expiry does. A denial is not an error, so one field cannot serve both. Two files merged without a conflict and still needed fixing: the workflows index test kept both sides' halves and produced a type annotation missing the owner fields, and two new tests rendered the page bare while a team row now needs a tooltip provider. api, web, workflow and engine typecheck clean; web 1092 passing, workflow 332, api template suites 155.
Three changes to the template surface and its tests. The gallery is scanned, not read, and each card said the same thing three times: the service chain drew the marks, a footer line spelled the same services out in text, and a sentence above the button repeated the button's own label. Two of those go. A card now carries the marks, the name, two lines of description, the cadence, and one control; the full text and the step list stay in the install dialog, where the reader has already chosen. A template whose services are not connected still cannot be installed, and the button names the service to connect. Every template also carried an icon field holding an emoji. That field was dead — not on the WorkflowTemplate type, never mapped to the wire, never read by the client — so it is deleted rather than swapped for an icon name. No emoji remain in the client. Separately, vitest 4's jsdom environment provides window, document and a real origin but no localStorage. The workspace scope persists there and its provider reads it during the first render, so about forty tests failed before asserting anything. The setup file already shims four other jsdom gaps; this is the fifth. Three tests asserted the removed card strings. They now assert the affordance that survived rather than being deleted. Web suite 1092 passing across 123 files, run sequentially. Under file parallelism a varying handful fail and each passes in isolation — pre- existing cross-file pollution, the reason the vitest config already documents reverting isolate: false.
This was referenced Aug 16, 2026
![valet-valet-turnkey-dev[bot]](https://avatars.githubusercontent.com/u/104513330?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diff is enormous (446 files, 37k+ additions) but is architecturally coherent and the test coverage is substantial. A handful of real issues are worth calling out.
-
packages/api/src/assistants/service.ts:262 —createAssistantretry path has a silent data-loss windowWhen the first
onConflictDoNothinginsert loses the default-slot race it retries withisDefault: false. But the retry itself uses plain.insert(...).values({...row, isDefault: false}).returning()with noonConflictDoNothing. If two concurrent callers both tried to create the very first assistant for a principal, one wins theisDefault: trueslot, and the other's retry insertsisDefault: false— fine. But if a third concurrent call (or a fast retry loop) races the retry on the same id (row.idis pre-generated), the second insert will throw a primary-key conflict rather than return cleanly. The error propagates as an unhandled exception instead of surfacing a useful row.- The retry should also use
.onConflictDoNothing()and then re-read on an empty result, same pattern asresolveDefaultAssistant.
- The retry should also use
-
packages/api/src/channels/stream-bridge.ts:~590 —closeRowsdrops the durable row afterSTREAM_ABANDON_AFTER_MSeven whentryStopreturned"retry"The loop is:
const outcome = await this.tryStop(...); if (outcome === "done") { closed += 1; } else if (this.now() - row.startedAt < STREAM_ABANDON_AFTER_MS) { continue; } else { console.error(...); } await this.deleteRecordFor(...); // always reached when not continue'd
A row older than 24 h that still fails
stopStream(e.g. Slack is down) gets its durable row deleted withoutclosedincrementing and withoutstopStreamsucceeding. The shimmer is then permanent: no row left for future sweeps, no successful close. Thecontinueshould encompass the give-up branch too, or the delete should be guarded byoutcome === "done". -
packages/api/src/routes/events.ts:~370 — double-canMutateSubscriptiondefinition / dead comment blockTwo consecutive JSDoc blocks appear at the same position (lines ~37-63 in the patch) — an unused first comment followed immediately by the real one. The first (
"A subscription is mutable by...") was not removed when the second was written. No runtime impact but worth cleaning to avoid confusion about which comment describes the function. -
packages/api/src/channels/host.ts:~800 —ensureDefaultAssistantSessioncalled inhandleGateCallbackwithout eviction guardIn the gate-resolution path, the code calls
ensureDefaultAssistantSessionto get the session, relying on the invariant that channel-keyed threads only exist on the default assistant's session. The comment acknowledges this invariant but also notes it would fail safe if violated. However,ensureDefaultAssistantSessioncreates the assistant row and session on first use. If a user's default was promoted to a different assistant after a gate was opened, the callback resolves a different (newly created?) default, andresolveDecisionthrows on a gate id that doesn't exist — caught by the outer try/catch but silently swallowed, leaving the gate permanently pending. This is acknowledged as a known edge case but worth a// TODOat minimum, since it produces a hung approval with no user-visible error. -
packages/api/scripts/dev-seed-linear.ts:45 —DEV_WEBHOOK_SECRETexported from a scriptexport const DEV_WEBHOOK_SECRET = "dev-webhook-secret";
Scripts under
scripts/are dev-only but theexportkeyword means this constant can be imported by application code. Given the constant is hardcoded to"dev-webhook-secret", the risk is low, but the export is unnecessary for a dev seed script and should be removed or the file should not be importable from application paths.
Created on behalf of Xiangan He xiangan@turnkey.io
Brings the triggers UI and tabbed Workflows hub, DinD sandboxes, and per-profile sandbox sources onto the consolidated branch. The hub and our template gallery both put tabs on /workflows. Upstream's hub primitive wins — it keeps tab state in the URL — and the gallery is mounted as a fourth tab inside it rather than as a second tab system. Our zero state made the gallery the whole page with no tabs; Runs and Triggers must stay reachable with no workflows, so the tabs always render and the gallery shows inline when the list is empty. Two files git merged without a conflict and produced broken behaviour. Upstream added GET /workflows/runs in a second router that mounts first, so its unfiltered handler silently shadowed ours and our filters, the 404 on an unreadable workflow, and the cursor validation all stopped working; the two are now one handler that also stamps the workflow name upstream's Runs tab needs. A router mock in the workflows index test ended up with a duplicate key, so the second won and returned an empty search forever, disabling upstream's tab tests. Two schedule UIs also survived the merge — a panel in the editor drawer and a strip below the canvas, two lists over one resource. The drawer now holds upstream's TriggerList, which does everything the removed section did and more. api 3001 passing with the five environmental failures, web 1109 across 125 files run sequentially, four typechecks clean.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
plugin-slack declared hono and @hono/node-server as devDependencies and imported neither — nothing in the package references either name. They pulled undici 8 into the install, which replaced the global fetch that the api's own tests use, and undici rejected the oversized content-length the two body-cap tests deliberately send. Both 413 tests failed inside the test's own fetch call, before reaching any route. This is the workspace trap CLAUDE.md already documents: a new dependency edge silently changing a shared singleton. It was read as a lockfile drift and treated by running an install, which recorded the new dependency instead of removing it. dev-v2 CI is green on the same commit, which is what identified this as ours rather than pre-existing. Running the suite on this machine could not have found it — Node 24 fails those tests locally either way. api back to 3001 passing with only the five environmental failures, web 1109 across 125 files.
Both packages declared the Kubernetes client as "latest", so every fresh install resolved whatever was newest rather than what the lockfile pinned. A recent install floated it from 1.4.0 to 2.0.0, which brought undici 8. undici 8 replaced the global fetch the api's own tests use, and it rejects the oversized content-length the two body-cap tests deliberately send, so both 413 tests failed inside the test's own fetch call before reaching a route. pi-ai pins undici 7; nothing else wanted 8. This was latent on every branch, not introduced by this one. Any install would have tripped it, and a lockfile is no defence when the range is "latest" — it only records which install got there first. Pinned to ^1.4.0, the version dev-v2 resolves and the deploy has been tested against. Zero undici 8 edges remain.
Applies the audit of packages/web against the Web Interface Guidelines. The shared fixes carry the most weight. ErrorRow renders through a live region, so a load failure that arrives after paint is announced instead of reading as an empty list — the case that made a 500 on the threads route look like a workspace with nothing in it. Three memory routes rendered a second main landmark inside the shell's; they are plain containers now. Nine native selects set a colour as well as a background, without which Windows dark mode paints unreadable text. Destructive actions ask first. Restoring a version overwrites the live definition, cancelling a run stops it where it stands, and deleting a workflow takes its versions, schedules, subscriptions and webhook with it; each now says so and each surfaces its server error beside the button that caused it rather than behind the dialog. Two findings did not reproduce and were reverted rather than applied. A Radix dialog already blocks the scroll chain through react-remove-scroll, so an overscroll rule there was inert. The tab bar never suppressed its focus outline, so adding a ring would have swapped a working indicator for a differently coloured one — a restyle, not a compliance fix. Left deliberately: focus-visible:ring-accent-500/40 on inputs and dialogs emits no rule, because an opacity modifier on a raw oklch value produces nothing. Tailwind's default ring colour still paints, so the ring is visible but generic blue rather than brand. Fixing it changes the ring on every input and dialog, which is past this pass. web 1121 passing across 125 files, typecheck clean.
A quick tunnel changes hostname on every restart, so an App's webhook URL goes stale and inbound events stop with nothing to see. GitHub exposes PATCH /app/hook/config under App-JWT auth, which this service already mints, so the instance can keep the URL current itself. It only ever touches an app this instance owns. loadAppConfigWithSource already separates an app created through this instance's manifest flow from one supplied by GITHUB_APP_*, and the second is how a deployment or a shared team app arrives. Syncing that one would hand the team's app to whoever booted last, and two developers would take it from each other on every start. The rest is restraint. It reads before it writes, so a matching URL costs one request and no change. It never clears: an instance with no public URL leaves the app alone rather than unsetting a URL some deployment set deliberately. It cannot throw, because it runs at boot and inside the manifest redirect, where a GitHub outage must not stop the server starting or strand an admin mid-flow. This fixes half the symptom. GitHub has no App-JWT endpoint for callback_urls, so user OAuth still points at the host that existed at creation and still needs a hand edit when the tunnel moves. The commit does not pretend otherwise. api typecheck clean; 453 passing across the github-app route and service suites, including a test that asserts an environment-sourced app receives no request at all.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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
1 participant
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.
What
One branch consolidating six v2 workstreams that were previously separate PRs (#216, #229, #233, #236, #251, #254). Each is merged in full; nothing was dropped to make the merge compile.
onError, completion events at settle, and a reallistRunsendpoint replacing an N+1.valet.yaml.Why one PR
These six branches touched the same files repeatedly —
wire/types.ts, the workflows routes, the integrations surface, and0000_app.sql. Reviewed separately, each conflicts with the others and the reviewer sees a shape that will not exist after the next merge. Consolidated, the diff is what actually ships.Notable merge decisions
origincolumn replaces that for both config-owned and group-owned teams.onErrorand the approval-gate fields. They answer different questions:onErrordecides what a node failure does to the run,onDeny/approvalTimeoutdecide what a policy gate's refusal or expiry does. A denial is not an error.integration-row.ServiceIconis the superset and the connect dialog depends on its slug shape, so the template gallery moved onto it and the duplicate was deleted.Test plan
pnpm typecheckclean across all packagesworkspace-prep*.docker,k8s-builder.cluster) and reproduce on a cleandev-v2checkoutRun the web suite with
--no-file-parallelism. Under parallelism a varying handful fail and each passes in isolation — pre-existing cross-file pollution, whichvitest.config.tsalready documents.Migration
0000_app.sqlgains columns from several branches.rm -rf ~/.valet/pgis required after checkout.Not covered
identity-links.tsmints codes for Telegram only), so a Slack DM stops atunlinked_sender. Everything downstream is built and tested.