docs: feedback for the database project, and two corrections to our own assumptions

docs/superpowers/analysis/2026-08-10-database-feedback.md

@@ -0,0 +1,121 @@
+# Feedback for the smartbotic-database project
+
+Date: 2026-08-10
+Server: 2.11.0 on zeus (`zeus.fsociety.hu:9004`)
+Client on mulan: `libsmartbotic-db-client-dev` 2.9.0-2
+CLI on mulan: reports 2.9.0-2 to dpkg but offers the 2.11.0 relation commands
+
+Everything below was reproduced against the live 2.11.0 instance today. Where I
+could not reproduce something, I say so rather than passing on a rumour.
+
+## Confirmed problems
+
+### 1. `relations` lists nothing while a relation exists
+
+```
+$ smartbotic-db-cli --address zeus.fsociety.hu:9004 relations
+no relations declared
+
+$ smartbotic-db-cli --address zeus.fsociety.hu:9004 relation smartbotic-automation:executions_workflow
+smartbotic-automation:executions_workflow:
+  child:              smartbotic-automation:executions
+  child_field:        workflowId
+  parent:             smartbotic-automation:workflows
+  on_delete:          cascade
+  validate_on_write:  no
+```
+
+The relation is real - it backfilled 2,136 rows and `describe-delete` enforces it
+correctly. Only the listing cannot see it.
+
+`ListRelationsRequest.project` is documented in `proto/database.proto:985` as
+"Empty lists every project (operator/CLI use)", so a CLI sending an empty project
+should have listed it. Either the CLI sends something other than empty, or the
+empty case does not do what the comment says.
+
+This matters more than a cosmetic listing bug: `relations` is how an operator
+answers "what is enforced on this database right now", and it currently answers
+"nothing" on a database with enforcement active. Somebody could reasonably
+conclude a declaration failed and declare it twice.
+
+### 2. `relation-create` blames the wrong argument
+
+The relation **name** must be project-qualified, which is documented
+(`proto/database.proto:928`). Passing an unqualified name gives:
+
+```
+$ ... relation-create executions_workflow \
+      smartbotic-automation:executions workflowId smartbotic-automation:workflows cascade false
+error: relation, child and parent must be in one project (no transaction spans two project envs)
+```
+
+Child and parent were both already qualified and both in one project. The message
+names the two arguments that were correct and not the one that was wrong. Naming
+the offending argument would have saved a round trip.
+
+### 3. Unique constraints are unreachable from the CLI
+
+`index-create <collection> <field>` takes no `unique` flag, so the headline
+unique-constraint feature of 2.11.0 cannot be declared by an operator at all -
+only from client code. We have three constraints we want (`users.username`,
+`users.email`, `(projectId, name)` on `workflows`) and cannot declare any of them
+until our client is upgraded, even though the server supports them today.
+
+### 4. `find` has no limit or offset
+
+`find <collection>` returns a fixed 100 with no way to page. Clearing 3,869
+session documents took 39 rounds of find-then-remove. A `[limit] [offset]`, or a
+`remove-where`, would turn routine cleanup from a scripted loop into one command.
+
+### 5. Fields named `password` are encrypted in version snapshots but not in live reads
+
+This one cost real debugging time and is not documented anywhere I could find.
+
+A workflow whose node config contains a field named `password` reads back
+plaintext from `get`, and `$ENC$zEiFjkEXD+...` from **every** version, including
+version 1. The same ciphertext appears in all versions. `$ENC$` does not appear
+anywhere in our source tree, and the client exposes only a collection-level
+`encrypted` flag which we do not set - so this is the daemon applying a
+field-name-based rule at the version-history layer.
+
+The visible consequence for us: our editor asks "does this workflow differ from
+its published version?" by comparing the live document against the stored
+version. That comparison is now plaintext against ciphertext, so it is **always**
+different, and the editor permanently claims unpublished changes for any workflow
+with such a field. A badge that is always on is a badge people stop reading.
+
+Whatever the intent, it needs to be either documented prominently or made
+symmetric - a version read that returns what a live read returns would remove the
+whole class of problem.
+
+## Not your bugs - corrections to things I previously believed
+
+Recorded so nobody wastes time chasing them.
+
+- **Server-side projection is in the client** and has been since 2.7.1
+  (`client.hpp:425`). I had it noted as "on the wire but not in the client". Our
+  adapter still filters after fetching and its comment still says the old thing;
+  that is our bug to fix, and it is the cheap fix for our biggest read cost.
+- **`ListCollectionsRequest` gained a `project` field in 2.8.0**
+  (`proto/database.proto:528`). I had it noted as "not project-scoped and cannot
+  be". Our adapter still verifies every bare collection name with a separate
+  `getCollectionInfo` call, which is now unnecessary work on every listing.
+- **A page of large documents returning empty rather than erroring**: I could not
+  reproduce this today. 200 execution documents came back intact. Our own read
+  path goes through a server-side summary view, which may be masking it. Not
+  reported as current, only noted.
+
+## Questions
+
+1. Does `relation-check` scan the whole child collection, or use the reverse
+   index? We would like to run it periodically against `executions` (2,136 rows
+   today, growing) and need to know whether that is cheap or a full scan.
+2. Is there a way to declare a relation whose child field is nested inside an
+   array of objects? Two references in our data cannot currently be expressed:
+   a workflow node's `config.credentialId` and its `type`, both living inside a
+   `nodes[]` array. If that is out of scope, saying so plainly would let us stop
+   looking for it.
+3. `validate_on_write` hard-rejects an empty-string reference. Is there a way to
+   allow "no parent" while still validating non-empty references? We have a
+   legitimate case - a webhook run that records no workflow id - and today the
+   choice is all-or-nothing per relation.

docs/superpowers/analysis/2026-08-10-database-relations.md

@@ -0,0 +1,206 @@
+# Database relations (v2.11.0): what we hand-roll today, and what to declare
+
+Written 2026-08-10. Read-only analysis, no code changed. Context: the upstream
+`smartbotic-database` daemon on zeus is 2.11.0 (relations, unique constraints);
+our client on mulan is still 2.9.0 with no relation API, so nothing here can be
+used yet. This is groundwork for when the client upgrade lands.
+
+Sources read in full before writing this: `/data/smartbotic-database/proto/database.proto`
+(from line 919) and `/data/smartbotic-database/client/include/smartbotic/database/client.hpp`
+(from line 800).
+
+## 1. Where we hand-roll referential integrity today
+
+| # | Delete site | Cleans up | Mechanism | Complete? |
+|---|---|---|---|---|
+| 1 | `workflow_controller.cpp:673-696` `deleteWorkflow` | its own scheduled triggers, then `executions` (workflowId match) and its private `wf_<id>` collection and its files | `updateScheduledTriggers(id,false)`, then `retention_.retire(id)` (`retention_service.cpp:42-50`) queues an async job: restamp matching executions/documents/files to a short TTL, drop the `wf_` collection after | **No.** `retire()` restamps only executions matching `workflowId` at the moment of the call, via an async worker. Anything that failed to restamp (worker not running yet, restart mid-sweep, or - the actual history here - workflows deleted before this retention machinery existed) is never revisited. Confirmed by the hand sweep below. |
+| 2 | `project_controller.cpp:303-349` `deleteProject` | nothing directly; instead **refuses** the delete while `workflows`, `credentials` or `workflow_groups` still carry that `projectId` (`ownedCollections()`, `project_controller.cpp:20-25`) | manual restrict-style check, `page_size=1000` per collection | Complete for its stated job (never deletes a project holding work), but it is a hand-written `restrict` relation over exactly the three collections in `ownedCollections()` - nothing else with a `projectId` (e.g. a future collection) is covered without remembering to add it here. |
+| 3 | `user_controller.cpp:183-227` `deleteUser` | sessions (`auth_store.cpp:247-252` -> `invalidateAllSessions`, `auth_store.cpp:427-438`, deletes each `sessions` row for the user); the user's personal project, but **only if** it holds no workflows/credentials/workflow_groups (`user_controller.cpp:203-221`) | explicit per-row `storage_.remove("sessions", ...)`; explicit query + conditional `storage_.remove("projects", ...)` | **Leaves orphans.** Sessions are the only thing cleaned. Not touched: `credentials.sharedWith` arrays containing the deleted user's id (verified no code path strips it - see `credential_controller.cpp:107-148`, only add/remove-by-request exist); `ownerId`/`createdBy` on workflows, credentials, workflow_groups, projects (dangling attribution, arguably intentional - see relation table); `collection_permissions` and `api_keys` are unused in this codebase today (see Part 3), so nothing references a user there yet. |
+| 4 | `workflow_group_controller.cpp:320-379` `deleteGroup` | with `force=true`: reassigns every child workflow's `groupId` to `""` and every child subgroup's `parentId` to the deleted group's own parent (`workflow_group_controller.cpp:334-368`); without `force`, refuses if `hasChildren(id)` | manual `set_null`-style reassignment | Complete for its own children. This is exactly what a `set_null` relation with `child_field=groupId`/`parentId` would do automatically. |
+| 5 | `credential_controller.cpp:383-408` `deleteCredential` | **nothing** | none | **Leaves orphans.** No check that a workflow node still references the credential's id (`config.credentialId`, embedded inside `workflow.nodes[]`), and no cleanup after delete. A workflow keeps pointing at a `credentialId` that resolves to nothing; the node then fails at run time with whatever "credential not found" error the node author wrote (if any). |
+| 6 | `node_controller.cpp:399-416` `deleteNode` | **nothing** | none | **Leaves orphans.** No check that any workflow still has a node of that type. Note: this reference cannot be expressed as a v2.11.0 relation at all - see the "not relation-able" note under Part 2. |
+
+**The orphan sweep.** Commit `a0cf96e` (2026-08-09, "feat: clear orphaned data,
+refuse dropping a collection that holds documents") is the hand sweep referenced
+in the task. Its own numbers: **8,538 of 10,159** execution records belonged to
+workflows that no longer existed ("mostly test workflows deleted over the past
+days"), plus 67 entirely-null rows from writes that never completed;
+`executions` went from 481 MB to 74 MB. The commit message states the direct
+cause: retention's cascade only finds executions whose `workflowId` matches a
+*current* delete call, so a workflow deleted before `retire()` existed (the
+whole per-workflow retention feature landed 2026-08-08/09, commits `8a52564`
+through `802d30c`) has no mechanism that will ever revisit it. The same commit
+also found four `watch_cursors` that looked orphaned by field-matching but
+were not (`watch_cursors` is keyed by the collection it watches, not by a
+workflow id) - a caution about guessing a "child field" instead of reading the
+schema.
+
+Two more things worth flagging that are not deletions but sit right next to
+this problem:
+
+- `execution_controller.cpp:113-131` `canSeeExecution` explicitly handles both
+  a deleted-workflow execution ("The workflow has been deleted. Its history
+  outlives it...") and an execution with an **empty** `workflowId` ("Some runs
+  record no workflow id - the webhook path is one"). The empty-`workflowId`
+  case was an active bug until **2026-08-08** (see Part 2's hazard section) -
+  the comment is now a defensive leftover for historical rows, not a live
+  data path, but those historical rows still exist.
+- `webserver_service.cpp:661-706` `reconcile` (runner restart handling)
+  proactively closes `executions` rows stuck in `status: running` on a runner
+  that no longer claims them, logging them as orphans it repairs rather than
+  ignores. This is the one piece of "orphan" handling in the codebase that is
+  actually complete for its scope.
+
+## 2. Proposed relation table
+
+| Child collection | Child field | Parent collection | `on_delete` | `validate_on_write` | Why |
+|---|---|---|---|---|---|
+| `executions` | `workflowId` | `workflows` | `no_action` | **off** | This is the one place `cascade`/`set_null` looks tempting and is wrong twice over: (a) it is exactly the relation whose *child* (`executions`) already carries a per-row TTL restamped after the fact by `retention_service.cpp` - see the TTL hazard below, this time from the child side, since a cascade delete has to walk the child's rows while the TTL sweep may be mutating them concurrently; (b) `validate_on_write` would hard-reject the empty-string `workflowId` that historical rows carry and that the webhook path produced until 2026-08-08 (`followups-from-tier-2` memory: "FIXED 2026-08-08... An execution recorded an empty workflowId whenever the run used the published version"). Recommend `no_action` and leave the existing app-level `retire()` sweep as the cleanup path, or move to `restrict`-then-manual-retire if the team wants delete-time visibility. **Do not use `cascade`.** |
+| `workflows` | `projectId` | `projects` | `restrict` | on | Replaces the hand-rolled check in `project_controller.cpp:320-337`. `validate_on_write` is safe: nothing found that inserts a workflow with an empty or dangling `projectId` (workflow creation always assigns one, `workflow_controller.cpp:357` sets `ownerId`, and `projectId` comes from the same request). |
+| `workflow_groups` | `projectId` | `projects` | `restrict` | on | Same reasoning as above; currently covered by the same `ownedCollections()` check. |
+| `credentials` | `projectId` | `projects` | `restrict` | on | Same; currently covered by `ownedCollections()`. |
+| `workflows` | `groupId` | `workflow_groups` | `set_null` | on | Matches the existing manual behaviour in `deleteGroup` force-path (`workflow_group_controller.cpp:334-345`) exactly - a declared relation would let that hand-written loop be deleted. |
+| `workflow_groups` | `parentId` | `workflow_groups` (self) | `set_null` | on | Matches `deleteGroup`'s subgroup reparenting (`workflow_group_controller.cpp:347-367`). Self-referential relations are not called out as unsupported in the proto/client docs, but neither are they described as tested - **verify with upstream before relying on this one**, flagged as a soft caution rather than a hard unsafe. |
+| `credentials` | `sharedWith` | `users` | `cascade` (array field, so this pulls the id and keeps the credential - equivalent to `set_null` here per the proto note on array fields) | on | Closes gap #3/#5 above: today a deleted user is never stripped from `credentials.sharedWith`, leaving a dangling id in the array forever. `validate_on_write` is safe as long as sharing continues to check `auth_store_.getUser` first, which `credential_controller.cpp:100-116` already does. |
+| `sessions` | `userId` | `users` | `cascade` | on | Would let `AuthStore::deleteUser`'s explicit per-row loop (`auth_store.cpp:247-252`, `427-438`) be deleted entirely - the relation does the same thing atomically with the parent delete. `validate_on_write` is safe: a session is only ever created from an authenticated login against an existing user (`auth_store.cpp:282-320`). |
+| `workflows` / `credentials` / `workflow_groups` / `projects` / `nodes` | `ownerId` / `createdBy` | `users` | `no_action` | off | These are attribution fields, not ownership in the referential-integrity sense - the app already treats a dangling owner as fine (e.g. `canSeeExecution`'s deleted-workflow branch). Declaring anything stronger than `no_action` would be a behavior change nobody asked for: deleting a user would either destroy their workflows (`cascade`, clearly wrong) or silently blank out who made them (`set_null`, loses history). Leave alone; `no_action` only adds the reverse index for future `describeDelete()` queries, which is a pure upside. |
+| `api_keys` | *(n/a)* | `users` | - | - | **Not a real candidate today.** `api_keys` is a reserved system-collection name (`database_controller.cpp:300`, `auth_store.hpp`/`system_collections.hpp:43`) with no controller, no writer, and (checked live) zero documents and not even listed among the instance's existing collections. Nothing to declare a relation over until the feature is built. |
+| `collection_permissions` | *(n/a)* | `users` | - | - | **Not a real candidate.** It is a single settings document keyed `"settings"` (`collection_permissions.cpp:39` `storage_.get("collection_permissions", "settings")`), not a per-user row. No child field to point at a user. |
+| workflow node `config.credentialId` | - | `credentials` | *(not expressible)* | - | **Cannot be declared as a v2.11.0 relation at all.** `child_field` is a dot-path that resolves to a single id or an array of ids on the *child document itself*. A workflow's credential references live inside `nodes[].config.credentialId`, i.e. one string per element of an array of *objects*, not an array of ids. The relation contract has no path syntax for "the `credentialId` field of every element of this array." This is exactly the gap behind hand-rolled cleanup gap #5 (`deleteCredential` cleans up nothing) - relations do not fix it; only app code walking `workflow.nodes` can. |
+| workflow node `type` | - | `nodes` | *(not expressible)* | - | Same shape of problem as above, same conclusion: not relation-able, still needs app code if `deleteNode` (gap #6) is ever to check for in-use node types. |
+
+**Unsafe / flagged candidates, summarized:**
+
+1. **`executions.workflowId -> workflows` with `cascade` or `set_null` is
+   unsafe and not recommended.** `executions` carries retention TTLs that
+   `retention_service.cpp` actively restamps (`restamp()`, `retention_service.cpp:187-283`,
+   called from both `apply()` and `retire()`). The upstream warning is about a
+   race hitting a `cascade`/`set_null` relation *whose parent* carries a
+   renewed-after-expiry TTL; here the shoe is on the other foot - `executions`
+   is the *child*, and it is the child rows that are being TTL-renewed out
+   from under a delete-time cascade walk over the parent (`workflows`). Either
+   direction is the same underlying hazard: a delete-time relation walk and an
+   independent TTL-renewal sweep touching the same rows concurrently. Recommend
+   `no_action`, keep `retire()` as-is (or route it through `describeDelete`
+   once the client has it, for visibility), and rely on periodic sweeps like
+   `a0cf96e` for anything `retire()` misses.
+2. **`validate_on_write` on `executions.workflowId` is currently safe going
+   forward** (the empty-string bug was fixed 2026-08-08) **but would reject
+   re-inserting or re-processing any of the historical empty-`workflowId` rows**
+   that predate the fix, and would reject the row shape `canSeeExecution` still
+   defends against. Recommend leaving `validate_on_write` off for this field
+   until a migration confirms no code path can still produce `workflowId: ""`
+   (see Part 4 - the `listPending` path pulls raw executions and would be
+   worth auditing at the same time).
+3. **`workflow_groups.parentId` self-relation** - not proven unsafe, just
+   unverified against a self-referential case; test against a real 2.11.0
+   instance before relying on it for the group hierarchy.
+
+## 3. Unique constraint candidates
+
+Checked against the live instance at `http://localhost:8090` (admin/admin) on 2026-08-10.
+
+| Field | Candidate collection | Live duplicates found | Declarable today? |
+|---|---|---|---|
+| `username` | `users` | 0 (only 1 user exists: `admin`) | Yes, trivially - but with only one row this is a weak signal. Re-check once the instance has more than one account. |
+| `email` | `users` | 0 (same caveat) | Yes, same caveat. |
+| `refreshToken` | `sessions` | **19** (given - task states this is already known) | **No.** Needs a cleanup pass (dedupe or drop stale sessions) before `unique=true` can be declared; the server will refuse it while duplicates exist. |
+| `id` | `nodes` | 0 across 86 node definitions | Moot - `id` is already the document's own primary key (`_id`), so a `unique` index is redundant, not a new guarantee. |
+| `(projectId, name)` | `credentials` | **1 pair**: two credentials named `noreply@shopcall.ai` in project `prj_89c532cf-...` (`cred_53c04169-...` and `cred_a668d3b5-...`) | **No.** Needs the duplicate renamed or merged first. Only 7 credentials total in this instance, so the fix is a two-minute manual rename, not a migration. |
+| `(projectId, name)` | `workflows` | 0 across 12 workflows | Yes, declarable today. |
+
+No `slug`-style field was found anywhere in the schema (workflows, projects,
+credentials, node definitions all key by generated id, not a human slug), so
+there is nothing to check there.
+
+## 4. The fat execution document
+
+**What it looks like today.** `ExecutionResult::toJson()` (`workflow_engine.cpp:210-273`)
+embeds every node's output as `nodeExecutions[]` directly inside the execution
+document, plus an optional full `workflowSnapshot` for the pinning feature.
+Two truncation regimes already exist as a stopgap: a finished execution's node
+outputs are truncated by `truncateLargeValues` (`workflow_engine.cpp:251`), but
+a **`Waiting`** execution keeps every node output whole and untruncated
+(`workflow_engine.cpp:234-240`) - deliberately, because a later resume seeds
+itself from that stored state. That is precisely the shape of document that
+gets largest.
+
+**Where the current pain actually shows up**, confirmed by reading the read
+paths rather than assuming:
+
+- The list endpoint (`GET /executions`, `execution_controller.cpp:286`) does
+  **not** suffer this today - it already queries a server-side view,
+  `executions_summary` (`webserver_service.cpp:460-493`), that projects out
+  `nodeExecutions` and `workflowSnapshot` entirely at the database, listing
+  only ten summary fields. This view is the reason listing 336 KB documents
+  doesn't currently cost what it could.
+- `ExecutionController::listPending` (`execution_controller.cpp:545-573`,
+  `GET /executions/pending`), by contrast, queries the **raw `executions`
+  collection** with a `status=waiting` filter and **no field projection and no
+  page size cap**. Every waiting execution is exactly the untruncated,
+  full-node-output case described above. This is the most likely candidate for
+  "a page of large documents can come back empty because the gRPC message
+  limit is hit silently" - it is the one remaining raw, unprojected,
+  unpaginated read of full execution documents in the controller layer.
+- Single-execution `GET /executions/:id` and the resume path
+  (`workflow_engine.cpp:1029`, `1060-1111`) legitimately need the full
+  document - resume literally reconstructs `node_results` by reading
+  `record["nodeExecutions"]` back off the stored row.
+
+**What a split would cost.**
+
+- *Schema*: new `execution_node_results` (or similar) collection, one row per
+  `(executionId, nodeId)` (or one row per execution holding the array - either
+  way the relation is `execution_node_results.executionId -> executions`).
+  Given the TTL hazard identified in Part 2 item 1, this new child collection
+  would sit in the exact configuration the upstream implementor warned about:
+  its parent (`executions`) is TTL-bearing and is actively restamped after
+  creation by `retention_service.cpp`. **A `cascade` or `set_null` relation
+  here inherits the same race** the executions-to-workflows relation was
+  flagged for above, except now it is the primary, intended use of the split,
+  not an edge case - so it would need `no_action` plus an explicit sweep
+  (mirroring what already exists for workflow deletion) rather than relying on
+  the relation to keep the two collections in sync.
+- *Write path*: `WorkflowEngine::execute` and every node-completion callback
+  (`workflow_engine.cpp:1921, 1955, 1981, 2001, 2023`, and the loop-body
+  variants) would need to write node results as a second collection call
+  instead of building one in-memory `ExecutionResult` and serializing it once.
+  That is a real increase in write volume and a new failure mode (execution
+  row written, node-results write fails or vice versa - no longer atomic
+  without a transaction the storage client doesn't offer across collections).
+- *Read paths that would have to change*: `GET /executions/:id` (join in the
+  child rows), resume (`workflow_engine.cpp:1060-1111`, currently reads
+  `record["nodeExecutions"]` directly off the stored execution - would need a
+  second query), the pinning/diff feature that reads `workflowSnapshot` and
+  node outputs together, `listPending` (would become the thing that benefits
+  most, since it could stop pulling full documents), and the retention
+  preview/measure code (`retention_service.cpp:114-159`) which would need to
+  measure and restamp two collections in lockstep instead of one.
+- *Migration*: every existing row in `executions` (2,125 documents live today,
+  74 MB post-sweep) would need its `nodeExecutions` array split out and
+  reinserted as child rows, or the split would only apply going forward and
+  every read path would need an `if (has child rows) else (read inline)`
+  branch indefinitely. Given how young this collection already is (rebuilt by
+  the 2026-08-09 sweep) a clean cutover is realistic, but it is still a
+  one-way migration with no simple rollback once child rows are written and
+  the inline field is dropped.
+
+**Recommendation: not yet, and only partly for the reason originally deferred.**
+The wait was "for relations" so the split has referential integrity to lean on
+- but Part 2 and this section both land on the same conclusion: the relation
+this split would actually use has a TTL-bearing parent, which is on the
+upstream implementor's own list of races to avoid for `cascade`/`set_null`.
+Declaring the relation as `no_action` (the safe choice) removes the main thing
+relations were expected to buy here - automatic child cleanup - leaving a
+second collection to keep in sync by hand, which is the same kind of sweep
+`a0cf96e` just had to run once already. The listing-side problem the split was
+meant to solve is already half-fixed by the `executions_summary` view; the
+concrete remaining offender (`listPending`) is a much smaller, much cheaper fix
+on its own - add a page size cap and a field projection to that one query -
+and would remove most of the "gRPC limit hit silently" risk without touching
+the schema, the write path, or every read path at once. Revisit the full split
+once (a) the client is actually on 2.11.0, (b) `describeDelete`/`checkRelation`
+give an operator a way to see what a `no_action` relation is quietly leaving
+dangling, and (c) `listPending`-style raw reads have been audited across the
+codebase to confirm this is the only one - if it is not, the case for the split
+gets stronger.