From 6ed88d8c81e6ed3854254167ecb68271de8cc6d5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 30 Aug 2026 20:30:26 -0700 Subject: [PATCH] Document four Interchange-boundary findings from the CL-7257 audit Each file records what was checked against the vendored pin a8bc06ae and what the check concluded, so the same ground is not re-derived: - workflow-deploy-source vs workflow_run_launch_spec: opposite recovery models (freeze vs re-resolve), not a duplicate - workflow-freeze vs the sidecar probe gate: composes the same native primitives by identity, not a reimplementation - folded-run mail vs @intx/mailbox: different storage planes, Postgres against a git-substrate mailbox - workflow_definition access: 41 hand-rolled queries categorised, and why routing them through the native store surfaced unrealistic test fixtures --- docs/folded-mail-vs-intx-mailbox.md | 79 ++++++++++++++ docs/workflow-definition-access.md | 76 +++++++++++++ docs/workflow-deploy-source-vs-launch-spec.md | 101 ++++++++++++++++++ docs/workflow-freeze-vs-probe-gate.md | 98 +++++++++++++++++ 4 files changed, 354 insertions(+) create mode 100644 docs/folded-mail-vs-intx-mailbox.md create mode 100644 docs/workflow-definition-access.md create mode 100644 docs/workflow-deploy-source-vs-launch-spec.md create mode 100644 docs/workflow-freeze-vs-probe-gate.md diff --git a/docs/folded-mail-vs-intx-mailbox.md b/docs/folded-mail-vs-intx-mailbox.md new file mode 100644 index 000000000..cce70ef8f --- /dev/null +++ b/docs/folded-mail-vs-intx-mailbox.md @@ -0,0 +1,79 @@ +# `folded-runs/src/mail.ts` vs `@intx/mailbox` + +Analysis for CL-7276. Read against the vendored pin `a8bc06ae`. + +## The claim under test + +CL-7276 was filed on the claim that `folded-runs`' mail layer duplicates published +`@intx/mailbox` surface — noting that `executeSearch` and `executeThread` have zero +callers in this repo — and that `listFoldedMail`'s hand-rolled keyset pagination +should be routed through the native package. + +**That claim is wrong, and the zero-caller signal was misleading.** The two operate +on different storage planes. There is nothing to route. + +## The two planes + +| | `folded-runs/src/mail.ts` | `@intx/mailbox` | +| ------------------- | ----------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | +| Backing store | `session_mail`, a **Postgres** table (`@intx/db/schema/messages.ts:71`) | UID/MODSEQ message store, IMAP-shaped | +| Backings that exist | — | `createInMemoryMailboxStore`; `createSubstrateMailboxStore` (`@intx/workflow-host`), writing into the workflow-run **git repo** | +| Postgres dependency | drizzle + `@intx/db` | **none** — deps are `@intx/crypto`, `@intx/mime`, `@intx/types`, `arktype` | +| Plane | Hub | Sidecar / run substrate | +| Addressing | `(sessionId, createdAt, id)` keyset | `uid`, `modseq`, `uidValidity` | + +`@intx/mailbox` has no Postgres backing and no way to acquire one without a new +adapter. `executeSearch`/`executeThread` take a `MailboxStore`, which +`session_mail` is not and cannot cheaply become — `MailboxStore` is a synchronous +in-memory interface exposing `messages`, `uidNext` and `highestModSeq` as readonly +properties. + +So the hand-rolled keyset walk in `listFoldedMail` is not a reimplementation of +`executeSearch`. It is a Postgres query against a Postgres table, and the native +functions could not serve it. + +## Why the zero-caller signal was misleading + +`executeSearch` and `executeThread` having no callers in this repo is real, but it +means the _sidecar-side_ mailbox surface is unused here — not that we reimplemented +it. Those functions are consumed inside `@intx/workflow-host`'s substrate mailbox, +which the vendored sidecar runs. The hub never holds a `MailboxStore`. + +This is worth recording as a caution for the parent audit (CL-7257): "native export +with zero callers" is a lead, not a finding. Two of the five sub-issues filed off +that signal have now been disproved by reading the backing store. + +## What the send path actually shares + +`sendFoldedMail` already composes native primitives rather than reimplementing +them — `@intx/mime`'s `parseMailToEmail` here, and `assembleMessage` / +`assembleSignedContent` / `createDetachedSignatureFromProvider` in the sibling +delivery paths. MIME assembly and signing are not duplicated. + +The retry wrapper `sendFoldedMailWithRetry` has no native counterpart and should +stay. Its hardening is load-bearing: see the module header of +`packages/webhook-triggers/src/launch.ts` on why a delivery-failed mail must not +throw past an already-202'd webhook route, or a sender retry mints a second run for +one event. + +## `POST /workflows/:runId/mail` + +Native, and real (`vendor/intx/hub-api/src/routes/workflows.ts:846`), but it is an +HTTP route requiring a `workflow-run:` `manage` grant. The hub calling its own +route in-process to write a row it already owns would add an authz round-trip and a +serialization hop to a direct write. Not recommended. + +## Conclusion + +No change recommended. CL-7276 should be closed as "different storage planes". + +The genuine version of this work is **CL-7103** (adopt Interchange per-run mailboxes +and threaded mail in workbench chat). That is a real migration — moving hub chat +onto the run-substrate mailbox — with a product decision and a data migration +attached. It is not a deduplication, and this ticket should not be confused with it. + +## Not verified + +- Whether `session_mail` could be exposed _as_ a `MailboxStore` adapter cheaply + enough to be worth it under CL-7103. The interface's synchronous readonly + `messages` array suggests not, but that is CL-7103's question to answer. diff --git a/docs/workflow-definition-access.md b/docs/workflow-definition-access.md new file mode 100644 index 000000000..8a0dfbf93 --- /dev/null +++ b/docs/workflow-definition-access.md @@ -0,0 +1,76 @@ +# `workflow_definition` access in Workbench + +Analysis for CL-7275. Read against the vendored pin `a8bc06ae`. + +## The finding + +`workflow_definition` is an Interchange-owned table. The pin ships +`createWorkflowDefinitionStore` (`vendor/intx/db/src/workflow-definition-store.ts`) +to read it, and that store has **zero** callers here. Meanwhile **41 direct drizzle +queries** hit `workflowDefinition` / `workflowDefinitionVersion` across 14 non-test +files. + +## Why the store is bypassed + +Not laziness. Its whole surface is the `(assetId, wireHash)` identity selector, +`loadFrozenGrantSnapshot`, `loadFrozenWireProjection`, and `rollback`. The two +`loadFrozen*` helpers **are** used (2 and 6 call sites) — callers reach for the +store when it fits. It simply has no by-id, by-name, by-tenant or by-asset read, +and no update path. + +## What the 41 sites actually need + +| Category | Count | Shape | +| --------- | ----- | ----------------------------------------------- | +| by-id | 26 | `(definitionId, tenantId)` | +| by-tenant | 6 | every deployed definition for a tenant | +| by-name | 5 | `(name, tenantId[, status])` | +| by-asset | 2 | every definition sharing an asset, newest-first | +| update | 2 | patch `description` / `status` | + +Spread: `packages/agent-directory` (7 files), `apps/hub/src/index.ts` (12 sites), +`packages/chat/src/platform-adapter.ts` (7), plus `folded-runs`, +`folded-run-one-shot`, `webhook-triggers`, `evals`, `workflow-freeze`, +`routine-launcher`, `skills-mount`. + +## What a prototype migration surfaced + +A prototype routed five callers (one per category) through a widened store. Doing +so exposed a second-order problem: the store runs rows through +`parseWorkflowDefinitionRow`, which validates the full row shape — status enum, +jsonb columns. Several existing fake-db fixtures stubbed only two or three fields +and failed immediately. + +Those fixtures were asserting against row shapes the database cannot produce. The +hand-rolled queries are not merely duplicative; they let unrealistic test doubles +pass. That is the strongest argument for routing these call sites somewhere typed. + +## Why the prototype is not this PR + +The prototype widened the store **inside** `vendor/intx/db`. That is an edit within +a vendored tree, and it carries re-pin tax: every line must be hand-reapplied at +each future pin (CL-7107), and it grows the vendored delta rather than shrinking +our own re-creation. Interchange origin/main is a read-only reference we vendor or +tag from — the widening cannot go anywhere else, so it has to live somewhere that +survives a re-pin cleanly. + +The prototype is preserved on `cl-7275-vendor-edit-archive` (`8a6c9901`) for +reference. It should not merge in that shape. + +## Options + +1. **A thin `@corbits/*` wrapper** over the native store's existing surface, adding + the five reads above. Zero edits to `vendor/intx/db`, survives a re-pin, gets + callers onto `parseWorkflowDefinitionRow`. Costs one small package we own. +2. **Leave the call sites alone** until a re-pin brings a wider native surface. No + new code and no tax, but the fixture problem above persists. + +Undecided. Option 2 is the smaller re-creation and matches "use as much of the pin +as possible, re-create as little as possible"; option 1 buys type safety at the +call sites now. + +## Consequence for the parent check (CL-7257) + +A `check:*` failing on direct `workflow_definition` access cannot ship before one of +the options above lands. Today it would fail 41 times with no correct alternative +to route to — a debt ledger, not enforcement. diff --git a/docs/workflow-deploy-source-vs-launch-spec.md b/docs/workflow-deploy-source-vs-launch-spec.md new file mode 100644 index 000000000..afb0f6071 --- /dev/null +++ b/docs/workflow-deploy-source-vs-launch-spec.md @@ -0,0 +1,101 @@ +# `workflow_deploy_source` vs `workflow_run_launch_spec` + +Analysis for CL-7271. Read against the vendored pin `a8bc06ae`. + +## The claim under test + +CL-7271 was filed on the claim that `@corbits/workflow-deploy-source` duplicates +Interchange's native `workflow_run_launch_spec`, and should be deleted in favour +of it. + +**That claim is too strong.** The two tables share a key, a purpose statement and +several columns, but they implement _different recovery models_. Deleting one for +the other is a design decision, not a cleanup. + +## What each one actually stores + +| | `workflow_run_launch_spec` (native) | `workflow_deploy_source` (ours) | +| ----------------- | --------------------------------------------------------------------------------- | ----------------------------------------------------------------- | +| Key | `anchor_run_id` (FK → `workflow_run`, cascade) | `anchor_run_id` (no FK) | +| Written by | `prepareExclusiveDeployment` only | `withDeploySourceRecording`, every placement | +| Written when | Inside the same transaction as the anchor `workflow_run` row and its `read` grant | After a deploy resolves | +| Recovery payload | `frozen_approval_bundle` — the whole approval, verbatim | `source` + `entry` + `pin` + `definition_asset_id` + `source_ref` | +| Redeploy strategy | **Replay the freeze.** No re-probe | **Re-resolve from the source.** Re-derives | +| Shared columns | `session_id`, `deployment_domain`, `source_authority_principal_id` | `tenant_id`, `deployment_domain`, `source_authority_principal_id` | +| Secrets | None — offering ids re-resolved at launch | None — inference sources re-resolved at redeploy | + +## Why native is exclusive-only + +`createWorkflowRunLaunchSpecStore` has exactly one writer and one reader, both in +`vendor/intx/hub-sessions/src/workflow-allocation-service.ts`: + +- `prepareExclusiveDeployment` (line 250) writes the spec inside the transaction + that mints the anchor run and its grant. +- `deployReadyAllocation` (line 327) reads it back and rehydrates + `InstallAndApproveResult` from `spec.frozenApprovalBundle` — approved wire hash, + approved grants, projection — then deploys with **no re-probe**. + +It is exclusive-only because it is part of the _exclusive allocation lifecycle_: +it exists to survive an allocation being replaced under a run. A shared-capacity +deploy has no allocation generation to lose, so upstream never needed it there. + +## The real difference: freeze vs re-resolve + +This is the crux, and it is a genuine design fork. + +**Native freezes.** `frozen_approval_bundle` is the approval verbatim. A redeploy +replays exactly what was approved — same wire hash, same grants, same projection. +Nothing is recomputed, so nothing can drift. The cost: an approval frozen against +a credential that later died stays frozen, and the run redeploys against a dead +chain forever. + +**Ours re-resolves.** We store where the bytes came from and re-derive at redeploy +against the tenant's live catalog. A rotated credential is picked up. The cost: the +redeploy is not guaranteed to reproduce what was approved. + +That trade-off is exactly the one CL-6687 ("Rotated API keys never reach live +agents", Done) was fixed in favour of re-resolution, and the same one +`apps/sidecar/src/workflow-host-wiring/index.ts`'s `restoreDeploymentFromRecord` +decides the same way when it returns `"deferred-to-wake"` rather than restoring +from a frozen `sources` snapshot (CL-6648). + +So the two tables encode opposite answers to the same question, and this repo has +already ruled twice in favour of re-resolution. + +## What this means for the ticket + +Adopting `workflow_run_launch_spec` wholesale would mean adopting the freeze model +for every placement, and re-opening CL-6687. That is not a cleanup. + +The defensible options, in order of preference: + +1. **Keep both, narrow ours.** Native owns exclusive-allocation recovery (it + already does). Ours owns shared-placement source recording. Document the split + and stop describing them as duplicates. Cheapest, and preserves both rulings. +2. **Extend native with a re-resolve arm.** Add the source/entry/pin columns to + `workflow_run_launch_spec` as an alternative to `frozen_approval_bundle`, and + raise it upstream. One table, two recovery strategies, chosen per row. Larger, + and needs upstream agreement. +3. **Cut over to freeze.** Rejected — reverses CL-6687. + +Option 1 does not delete any code. That is the honest outcome: the ~350 loc in +`@corbits/workflow-deploy-source` is not redundant, and CL-7271's premise that it +could simply be deleted does not survive reading the native writer. + +## What still stands from CL-7271 + +Two of the original observations survive independently of the above: + +- `workflow_deploy_source.anchor_run_id` has **no foreign key** into + `workflow_run`, where the native table has one with `onDelete: "cascade"`. + That is a real defect and belongs to CL-7258. +- The columns that genuinely have no native counterpart are `entry`, `pin`, + `definition_asset_id` and `source_ref` — the last already carries an in-tree + `WORKBENCH DELTA` comment. These are what option 2 would need to upstream. + +## Not verified + +- Whether any shared-placement deploy could ever lose its allocation the way an + exclusive one can. If it cannot, ours is recording for a recovery that never + happens, which is a separate question worth asking. +- Migration shape for existing `workflow_deploy_source` rows under option 2. diff --git a/docs/workflow-freeze-vs-probe-gate.md b/docs/workflow-freeze-vs-probe-gate.md new file mode 100644 index 000000000..741b11885 --- /dev/null +++ b/docs/workflow-freeze-vs-probe-gate.md @@ -0,0 +1,98 @@ +# `@corbits/workflow-freeze` vs the sidecar probe gate + +Analysis for CL-7273. Read against the vendored pin `a8bc06ae`. + +## The claim under test + +CL-7273 was filed on the claim that `@corbits/workflow-freeze` _reimplements_ the +freeze computation that `vendor/intx/hub-sessions/src/workflow-probe-gate.ts` +owns, and that the two will silently drift. + +**That claim does not survive reading the imports.** `workflow-freeze` does not +reimplement the freeze. It composes the same native primitives, from the same +packages, for an input shape the probe path cannot produce. + +## What `workflow-freeze` actually imports + +Every load-bearing step comes from `@intx/*`: + +| Step | Primitive | Package | +| ------------------------------ | ------------------------------- | ---------------------------------- | +| Reify the definition | `projectLiveToInert` | `@intx/workflow` | +| Walk the grant surface | `walkCapabilities` | `@intx/workflow-deploy` | +| Director registry for the walk | `createDefaultDirectorRegistry` | `@intx/agent` | +| Compute the frozen hash | `computeWireDefinitionHash` | `@intx/types/wire-definition-hash` | +| Persist the freeze | `createDbFrozenApprovalWriter` | `@intx/hub-sessions` | + +The last two are the ones that matter, and they are shared with the probe path by +identity, not by imitation: + +- `workflow-probe-gate.ts:454` calls `persist: createDbFrozenApprovalWriter(args.db)`. + `workflow-freeze` calls the _same exported function_. +- `workflow-probe-gate.ts:40` imports `computeWireDefinitionHash` from the same + module path `workflow-freeze` does, and recomputes with it at line 233. + +So the hash preimage and the all-or-nothing stamp — the two places a drift would +actually cause damage — are one implementation, not two. + +## What genuinely differs + +Two things, both by necessity rather than duplication: + +1. **The input.** The probe path receives a projection produced by a sidecar child + evaluating live code. `workflow-freeze` receives a hub-authored definition that + is _already inert JSON_ (an agent from the Agents page, a template block). There + is no probe round-trip to ride because there is no code to evaluate. +2. **The approval policy.** The probe path gates on the operator approval walk. + `workflow-freeze` self-approves, which the probe gate's own comments document as + the analogue for live-authored definitions — the hub authored the bytes, so + there is no third party whose approval is being assumed. + +## The bug this package fixed + +Before it, hub-authored paths called bare `ensureWorkflowDefinitionForAsset`, which +left `approved_wire_hash` / `grant_snapshot` / `wire_projection` NULL — permanently +unlaunchable rows (CL-6447, CL-6439). The package exists to route those paths +_into_ the native freeze, not around it. + +## Existing test coverage + +`packages/workflow-freeze/src/index.test.ts` already asserts the property CL-7273 +asked for a drift test to establish: + +``` +expect(frozen.wireHash).toBe(await computeWireDefinitionHash(frozen.projection)); +``` + +That pins the hash to the native function over the inert projection, and a +companion assertion pins that hashing the _raw_ JSON produces a different preimage +— which is the actual failure mode worth guarding (freezing a hash no launch-time +reader can recover). The DB half is covered against real Postgres in +`test/freeze.drizzle.test.ts`. + +A cross-path test freezing the same definition through both routes would need a +running sidecar to produce the probe half. Given both routes already call one +`createDbFrozenApprovalWriter` and one `computeWireDefinitionHash`, that test would +be asserting that a function equals itself. + +## Conclusion + +No change recommended. This package is the pattern AGENTS.md holds up as correct +— product owns the composition, the platform owns the mechanism — and is closer to +`packages/approvals` (explicitly cited as clean) than to the `mintRepoGrant` drift +that CL-7256 exists to fix. + +CL-7273 should be closed as "not a reimplementation". + +## What would change this + +If either path stopped calling the shared primitives — a local hash helper, a +hand-rolled stamp — the drift risk returns immediately. That is the thing worth +enforcing, and it belongs in the parent check (CL-7257) as "these two call sites +must import the same freeze primitives", not as a bespoke test here. + +## Not verified + +- Whether the self-approve policy is correct in every case it is reached. This + analysis took the probe gate's own documented analogue at its word rather than + auditing each caller's authority.