Skip to content

feat: #2288 workflow-scoped key-value state (State Get + State Set) - #2368

Open
dickwin2003 wants to merge 1 commit into
KeeperHub:stagingfrom
dickwin2003:feat/issue-2288-workflow-state
Open

feat: #2288 workflow-scoped key-value state (State Get + State Set)#2368
dickwin2003 wants to merge 1 commit into
KeeperHub:stagingfrom
dickwin2003:feat/issue-2288-workflow-state

Conversation

@dickwin2003

@dickwin2003 dickwin2003 commented Sep 8, 2026

Copy link
Copy Markdown

Closes #2288 (part of #2293). Also my submission for the DoraHacks "KeeperHub - The Agent Economy" hackathon bounty track, held to the normal merge bar.

Storage: Postgres, not Redis

Argued from the requirements as the issue asks:

  • Concurrency/durability. lib/redis.ts documents itself as "never a source of truth" (null client when REDIS_HOST is unset, fail-fast commands, loss tolerated). A monitor cursor is exactly the value whose loss is visible - the resolved-alert-reopens failure the issue describes - so it belongs in the durable store the app already operates. Bonus: self-hosted installs need zero new infrastructure, and the user provisions nothing, which is the "serverless-first" spirit of the ticket.
  • Eviction. expires_at (timestamptz, null = no TTL) filtered on read, expired rows deleted on touch. No background job is required for correctness, and an indexed expires_at keeps a future sweep cheap if one is wanted.
  • Visibility. A row is trivially inspectable later; updated_by_execution_id already answers "which run wrote this cursor" from day one.

What ships

  • Schema (lib/db/schema.ts + migration 0152): workflow_state with unique (organization_id, workflow_id, key), value jsonb, version (bumped on every write), nullable expires_at, updated_by_execution_id; FKs to organization and workflows with ON DELETE CASCADE. Migration generated with pnpm db:generate per AGENTS.md.
  • Two system actions, State Get and State Set, registered the compile-enforced way: entries in SYSTEM_ACTION_TYPES, the executor's SYSTEM_ACTIONS dispatch table, SYSTEM_ACTION_EGRESS (both none - in-process DB access, no user-chosen destination, same as the circuit breakers), and lib/mcp/workflow-schema-constants.ts so they surface in validate_workflow / the schemas route. Builder UI: palette entries plus config forms (key, value editor, ttl, advanced expectedVersion field).
  • Isolation. Both steps take (organizationId, workflowId) from _context and never from config - the circuit-breaker steps' rule. No API surface takes a workflow id, so cross-workflow and cross-org access have no path. This also answers the direct-execution question: /api/execute/node has no workflow scope, so the actions are deliberately not in action-resolver.ts and fail with a clear error if run without workflow context.
  • Concurrency. state/set is an atomic upsert (update-then-insert inside a transaction, with ON CONFLICT DO UPDATE covering a concurrent insert of the same key). With expectedVersion it is a compare-and-set: on mismatch the step fails with a structured conflict error naming expected vs current version instead of silently losing the race. state/get returns version to feed it. Both steps set maxRetries = 0 so the durability layer never replays a bump.
  • Limits, enforced: serialized value <= 8 KB (error message points at Database Query/HTTP Request for payloads), 100 live keys per workflow, ttl clamped to [1s, 365 days]. The ceiling only gates writes that create a new row; overwriting an existing key (live or expired) never grows the store. An expired key frees its slot.
  • Deletion semantics. Hard delete cascades. Duplication and export/import need no code: a duplicate gets a new workflow id and therefore starts with empty state (a cloned monitor must not inherit the source's cursor), and state is runtime data so it is not part of export. Soft-deleted (slug-hidden) workflows keep their state, consistent with how soft delete treats the rest of the row.
  • Out of scope, per the issue: no cross-workflow state, no API read path. Code node access: none - the actions are the only path, consistent with the sandbox env allowlist decision in Workflows have no configuration layer: no org-scoped values, and no state that survives a run #2293.

The actions are deliberately not added to the codegen export map (same treatment the circuit-breaker actions shipped with): an exported standalone bundle cannot reach this org table.

Testing

  • New unit suites: step behavior (context-derived scoping including the config-injection case, CAS conflict passthrough, ttl/expectedVersion coercion, no-context failures), store validation (key/value/ttl/CAS-input rules, all enforced before storage), and registration invariants (union + MCP catalog + egress none + not plan-gated + validator acceptance), mirroring the circuit-breaker suites.
  • tests/unit/validate-workflow-structural.test.ts pinned key list extended 7 -> 9.
  • Local verification beyond the mocked unit tests: the store's transactional SQL was exercised against a real Postgres engine (PGlite, repo migrations applied) covering create/overwrite/version bump, CAS apply + stale-version rejection without write, CAS on missing key, isolation between (org, workflow) pairs, TTL expiry + physical eviction, size limit, key-count ceiling at exactly 100/101, overwrite at ceiling, expired key freeing a slot, FK cascade on workflow delete, and editor JSON-text coercion. 19/19 checks passed. Happy to port that harness into the integration suites if you want it in CI - it needs a live Postgres, so I left the committed tests hermetic.
  • pnpm type-check, pnpm type-check:executor, pnpm fix (biome) clean, and pnpm build passes locally (against a .env.local and a socket-served embedded Postgres for static generation).
  • Full pnpm test:unit run: 22,030 passed; the 142 failures all sit in 5 files (code-sandbox runtime, docs markdown, a Windows path-sensitive plugins walk, one slow-import timeout) and were verified to fail identically on the untouched base commit via a clean worktree run, so none are introduced here.

Addresses the design comment on the issue: storage question resolved as above; open question 2 resolved as system action types (that is what surfaces them in the builder/MCP schema with no extra wiring); question 3 resolved by adopting the proposed 8 KB / 100 keys ballparks.

…te Set)

Adds a per-workflow key-value store that survives a run, so monitor
workflows can keep a cursor ("last block scanned", "already alerted on")
instead of re-scanning a rolling window or provisioning an external
database for one value.

- workflow_state table: unique (organization_id, workflow_id, key),
  jsonb value, per-write version, nullable expires_at, cascading FKs;
  migration 0152 generated with db:generate
- State Get / State Set system actions dispatched like Database Query,
  classified egress: none, and listed in the MCP schema catalog
- Scope comes from the execution context, never config (the
  circuit-breaker rule), so cross-workflow and cross-org access have no
  path
- state/set is an atomic upsert and accepts expectedVersion for
  compare-and-set; get returns the version to feed it
- Enforced limits: 8 KB serialized value, 100 keys per workflow, ttl
  clamped to 365 days; expired rows are evicted on touch
- Builder UI: palette entries and config forms for both actions
- Not in the direct-execute node resolver or codegen export: standalone
  node execution has no workflow scope, and exported bundles cannot
  reach this table (same treatment as the circuit-breaker actions)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

@suisuss suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome, and thanks for a diff that arrives with its registration complete - CONTRIBUTING.md and plugins/AGENTS.md carry the conventions, and the four system-action registration points are all here, which is the part most first contributions miss.

What this changes

A workflow_state table (drizzle/0152_tired_silver_sable.sql, model at lib/db/schema.ts), keyed unique on (organization_id, workflow_id, key) with version, expires_at and updated_by_execution_id, cascading from both organization and workflows. A store at lib/workflow/nodes/workflow-state/store.ts with the limits, the pure helpers, and a transactional setWorkflowStateValue that does compare-and-set when expectedVersion is given and an upsert otherwise, gating the 100-key ceiling only when no row exists. Two "use step" wrappers with maxRetries = 0 that take their scope only from input._context. Registration in all four places - SYSTEM_ACTION_TYPES, the executor dispatch table, system-action-capabilities.ts as egress none, and the MCP catalogue. Then the builder surfaces: two entries in the System group and two config forms.

I diffed 0152_snapshot.json structurally against staging's 0151: prevId chains correctly, 72 to 73 tables, and the only delta is public.workflow_state, matching the SQL and the model exactly. The migration is additive and safe - CREATE TABLE locks nothing, the two ADD CONSTRAINT take SHARE ROW EXCLUSIVE on organization and workflows but validate instantaneously against an empty child, and the deployed migrator sets lock_timeout=60000 so it fails rather than queues. Both indexes are built in the same migration on an empty table, so this correctly needs no @requires-db-prep.

Does it match the description

Undersells. The store's own comment and the MCP catalogue both say numbers and booleans are stored as-is, and through the editor they are not - see the fourth blocker.

Blocking

  • components/workflow/config/action-config.tsx:745-758 - the expectedVersion field cannot accept what its own help text tells you to put in it. The help says "Pass the version returned by State Get (as @ reference)", and onChange runs e.target.value.replace(/[^0-9]/g, ""). -> Paste {{@state-get-1:State Get.version}} and the field stores "1", the digit out of the node id. Every run then compares against version 1, so the second write to that key ever fails with "expected version 1, current 2", permanently, with nothing to indicate why. The runtime side is fine - resolveExpectedVersion accepts numeric strings and the template resolves - so this is the sanitizer defeating the documented pattern. Same sanitizer on TTL at :723-726.

  • lib/workflow/nodes/state-get/step.ts:58 - a miss returns {success: true, exists: false, value: null} with no version key at all. The executor stores the step result directly as the node's output (executor.workflow.ts:3523-3525), so a downstream {{@get:State Get.version}} resolves to nothing, recordUnresolved fires with no-path, and assertResolved throws. -> The first execution of the documented State Get then State Set cursor fails before it can create the key. Return version: null on a miss, or say in the catalogue that the field is absent and the config must tolerate it.

  • lib/workflow/editor/template-helpers.ts:209-224 and components/ui/template-autocomplete.tsx:130-148 - neither learned about the new actions, so both fall through findActionById (undefined for a system action) to the terminal default [{ field: "data" }]. -> The only field the @ menu offers for a State node is {{@state-get-1:State Get.data}}, the step result has no data wrapper, and the run aborts with the same TemplateResolutionError. The discoverable path through the UI is a guaranteed failure. Both files special-case For Each and Collect already; add the two here.

  • lib/workflow/nodes/workflow-state/store.ts:143-163 - "numbers and booleans stored as-is" holds for MCP and not for the editor. processTemplates operates on string fields and formatConfigValue stringifies, so value: "{{@trigger:Trigger.blockNumber}}" reaches the step as "4219", and coerceStateValue only JSON-parses strings starting with { or [. -> It is stored as the JSON string "4219", and a downstream Condition comparing it against the same trigger field compares lexically: "100" < "99" is true. Objects and arrays round-trip; scalars do not, and the two authoring paths disagree on the stored type of the same workflow. -> Either coerce a bare numeric or boolean string in coerceStateValue, or correct the claim in the store comment, the config help text and the MCP catalogue.

Mechanical - actionable as-is

  • components/workflow/config/action-config.tsx:653-666 - the Key field is a plain <Input> while its help text says "Use @ to build the key from previous node values". Every other template-capable text field in the file uses TemplateBadgeInput (:208, :414, :482). Raw {{…}} still resolves at runtime, so this degrades rather than breaks, but the affordance the help promises is not wired.

  • lib/workflow/nodes/workflow-state/store.ts:410 - overwriting an expired row takes the existing branch and returns created: false, immediately after State Get reported exists: false. The catalogue defines created as "created the key rather than overwriting it".

  • lib/workflow/nodes/state-set/step.ts:62 - the guard rejects only undefined, so config.value of "" from the editor sets the key to the empty string, and an MCP-authored "value": null reaches an insert against a notNull() jsonb column. I could not verify from the repo whether drizzle's jsonb mapper writes JSON 'null' or SQL NULL there - mark that one UNSURE - but screening both is one line.

  • drizzle/0152_tired_silver_sable.sql - the last thirteen migrations are descriptive (0151_keep_1328_org_circuit_breaker.sql); this one keeps drizzle's generated codename.

  • idx_workflow_state_expires_at indexes a column nothing sweeps. Eviction is lazy-on-touch only, so an abandoned expired key is invisible to reads and to the 100-key count and occupies storage indefinitely, while the index costs write amplification for a sweeper that does not exist.

  • components/workflow/nodes/action-node.tsx:134-166 - getProviderLogo has no case for the new actions, so both render the amber Zap fallback, identical to HTTP Request.

  • Screenshots: the System group of the action picker with both new entries; the State Set config form with Value, TTL and expectedVersion filled; and the State Get form. Those three are what the diff renders.

With the team

  • Whether a lost compare-and-set should be recoverable. stateSetStep.maxRetries = 0 plus a CAS that fails the node means the "re-read with State Get and retry" pattern the catalogue describes is not expressible - there is no loop construct in the builder, so a lost race fails the whole run. I'm weighing allowing a retry on the step against adding a first-class retry-on-conflict output the author can branch on; the tradeoff is an unbounded retry on a write that moves state against a config surface nobody has asked for yet. I'm taking it to the core team and will come back.

  • Whether an org-level ceiling is needed. The only cap is per workflow, every plan has maxWorkflows: -1, and there is no reaper. I'm weighing a per-org key or byte budget against leaving it uncapped until it matters.

Does it match the description: Scope creep

One clean seam. components/workflow/config/action-config.tsx and action-grid.tsx on one side, everything else on the other. The engine half ships and is correct with the UI reverted - the actions stay usable from MCP, the executor dispatches them, egress and validation are right, they are simply absent from the palette. The UI half does not survive alone: it uses bare string literals so it compiles, but validateWorkflowActionConfigs skips only members of SYSTEM_ACTION_TYPES, so a State node fails save with UNKNOWN_ACTION_TYPE. That is a one-way dependency, so the split is available but not required, and everything inside the engine half is genuinely atomic - the union, the egress record and the dispatch table are compile-time coupled, and the schema fails at first execution without the migration. Not scope creep; I am recording it so it is not re-litigated.

Verdict

Changes requested - the version field strips the reference it asks for, a State Get miss omits the field the documented pattern reads, and the only output the @ menu offers for these nodes aborts the run.

@suisuss suisuss added changes-requested Triage: reviewed, changes needed from the contributor decision-needed Blocked on a maintainer decision, not on the contributor requested-evidence Screenshots or video requested from the contributor labels Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Triage: reviewed, changes needed from the contributor decision-needed Blocked on a maintainer decision, not on the contributor requested-evidence Screenshots or video requested from the contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nothing a workflow computes survives the run: no workflow-scoped key-value state

2 participants