Merge remote-tracking branch 'origin/main' into claude/issue-17176-oauth-resource-seed

Author: claude

.changeset/blank-node-condition-refused-at-registration.md

@@ -0,0 +1,104 @@
+---
+"@objectstack/service-automation": minor
+---
+
+fix(service-automation)!: a whitespace-only `config.condition` is refused at `registerFlow`, the rule the edge door has carried since #15807 (#17322)
+
+<!-- adr-0087: not-required (already-registered flow-edge-condition-evaluated-slot-source-required) this is a second face of the decision that entry already carries — an evaluated slot requires a non-blank `source`, refused with EVALUATED_EXPRESSION_SOURCE_REQUIRED — applied to the other structural condition slot by importing the same schema rather than by deriving a second rule; no key is renamed, retired or given a new meaning here. ⚠️ That entry's `surface` and `acceptanceCriteria` name only `edges[].condition`, so they need widening to `config.condition` for a consumer replaying the chain; that file is in packages/spec, outside this card's package, and is filed as a follow-up rather than edited here. -->
+
+**BREAKING** in the accept-set sense, landing in the launch window as `minor`
+(the lockstep convention: `major` is refused by `check-changeset-no-major`, and
+breaking-ness is carried by this banner plus the ADR-0087 disposition): a flow
+node's `config.condition` — a `decision` node's predicate, and on a `start` node
+the **trigger gate** — is now refused at `registerFlow` when its source is blank
+after trimming, where it used to register clean and answer a **silent `false`**
+at every evaluation.
+
+Two doors, the same authored value, two fates until now. `FlowEdgeSchema.condition`
+composes `EvaluatedExpressionInputSchema` (#15807), so `'   '` on an edge is
+refused at `FlowSchema.parse`, by name. A node's `config` is an open
+`z.record(z.string(), z.unknown())`, so the same value passed through verbatim,
+reached `AutomationEngine.evaluateCondition`'s empty-source arm — `exprStr.trim()
+=== ''` — and returned `false`, under a comment that names that arm as being for
+an **unauthored** condition. `'   '` was authored. The branch never ran, forever,
+with nothing said at any layer.
+
+```yaml
+nodes:
+  - { id: gate,   type: start,    config: { objectName: lead, triggerType: record-after-update, condition: '   ' } }  # the flow was gated shut
+  - { id: branch, type: decision, config: { condition: { dialect: cel, source: '   ' } } }                            # the same blank, through the envelope key
+```
+
+> An expression in an evaluated slot needs a non-blank `source`: the expression
+> engine evaluates `source` (the canonical persisted form of phase M9.1) and
+> cannot evaluate `ast` alone, so an envelope carrying only `ast`, or a `source`
+> that is blank after trimming, would validate and register and then fault at
+> run time. Write `{ dialect: 'cel', source: '…' }`.
+
+- **The rule is imported, not re-derived.** `registerFlow`'s structural pass runs
+  the condition's source through `EvaluatedExpressionInputSchema` itself, so the
+  node door and the edge door cannot drift into two notions of "blank" or two
+  sentences for it — the property the #15662 campaign built the shared refusal
+  for. Nothing is exported from this package to carry it, and no new export was
+  added.
+- **Applied to the SOURCE, not to the whole value**, deliberately: the union
+  would also refuse an envelope with no `dialect` or with a dialect outside its
+  enum, and this slot admits both (`structuralConditionRefusal`'s docblock,
+  #4336). The narrowing is exactly the blank population and nothing else — a
+  `cron` envelope with a real source still earns its own pre-existing verdict,
+  and a bare string with a `{…}` brace trap still earns #1491's.
+- **`evaluateCondition` is unchanged and still answers `false`.** It is the
+  shared evaluator and a public method on an exported class, so its throw
+  behaviour is itself a contract; and a stored flow reaches it whatever the
+  producer refuses. This change is at the producer only.
+- **`structuralConditionRefusal` is unchanged.** A string is still a well-shaped
+  condition; the new refusal sits behind the shape one and in front of the CEL
+  one, and answers the evaluated-slot sentence rather than
+  `STRUCTURAL_CONDITION_SHAPE_REFUSAL`.
+
+**What an author does with a refused condition.** A whitespace-only condition was
+never a predicate — the engine answered `false`, so the branch never fired, and on
+a `start` node the flow never triggered. **Remove the `condition` key** if the node
+was meant to be unconditional, or **write the expression** if it was meant to
+branch. ⚠️ Those two are not interchangeable: a refused condition never fired,
+while an absent `condition` on a decision node is an unconditional branch that
+always fires and an absent one on a start node is a gate that always opens.
+Deleting the key to clear the refusal inverts the node rather than preserving it.
+Every condition with a non-blank source is unchanged, and nothing is renamed or
+retired.
+
+**A flow ALREADY STORED in `sys_metadata` stops running entirely — the whole flow,
+not just the branch.** Stored flows are deliberately not canonicalized by
+`applyConversionsToStoredItem` (`spec/src/conversions/stored.ts`, and the same
+skip in `metadata/src/loaders/database-loader.ts`'s `rowToData`); they canonicalize
+at `registerFlow`, and each of the three boot paths in
+`service-automation/src/plugin.ts` wraps that call in `try`/`catch`, logs one
+`warn` naming the flow, and continues. So a node condition that used to answer a
+silent `false` while the rest of the flow ran now takes the flow down with it: it
+is never registered, its trigger is never armed, and the announcement is that one
+warn line — `[Automation] failed to register flow` at boot, `[Automation]
+cold-boot flow bind: failed to register flow` at the kernel:ready bind,
+`[Automation] flow re-sync: failed to register flow` on a re-sync. The warn line
+is also the locator: the refusal names the node and the slot, e.g. `node 'gate'
+(start) condition`. A stack authored in config files has a second door,
+`objectstack validate` — see the note below for what that door does **not** yet
+say.
+
+**A repo-wide census on this branch found zero authored `config.condition` values
+of this shape**, against a lit control: a textual probe over all 8,123 tracked
+source files found **461** non-blank `condition:` string literals and **zero**
+blank-after-trim ones in any authored flow (the four blank hits are two prose
+examples inside #15807's own changeset and two `packages/lint` test fixtures).
+There is nothing in this repository to rewrite.
+
+⚠️ **Two follow-ups this change does not carry, both outside this card's package.**
+(1) The ADR-0087 D3 entry named above,
+`flow-edge-condition-evaluated-slot-source-required`, registers the decision this
+change is a second face of — an evaluated slot requires a non-blank `source` — but
+its `surface` and `acceptanceCriteria` name only `edges[].condition`. They need
+widening to `config.condition` so a consumer replaying the chain is told to sweep
+the node key too; that file is in `packages/spec`.
+(2) `@objectstack/lint`'s `validate-expressions` applies only
+`structuralConditionRefusal` to a structural condition, so `objectstack validate`
+still reports nothing for a blank `config.condition` that `registerFlow` now
+refuses — the two doors disagree until that rule is rebound as well.

.changeset/import-protocol-implementor-typed.md

@@ -0,0 +1,13 @@
+---
+"@objectstack/plugin-auth": patch
+---
+
+fix(plugin-auth): let `ImportProtocolLike` type the admin import protocol's members (#17422)
+
+`admin-import-users.ts` is the only hand-written in-repo implementor of the runner's `ImportProtocolLike`, and it annotated all three required members `args: any`. An explicit parameter annotation wins over the contextual type, so #16952's newly declared request dialect held every implementor except this one — the one with a demonstrated history: before #16950 this file read `args?.query?.$filter ?? {}`, the runner moved to the canonical spelling, the read went `undefined`, and the `?? {}` default degraded the import's duplicate probe into match-everything, so `POST /api/v1/auth/admin/import-users` updated the wrong users without a sound.
+
+The three annotations are deleted, so `findData` / `createData` / `updateData` are typed by the contract they implement. Measured: with the annotations gone, reading a retired wire alias (`args.query?.$filter`) is `TS2339 Property '$filter' does not exist on type 'QueryInput'`; with `args: any` restored the identical probe type-checks at exit 0.
+
+`FindDataRequest` declares `query` optional, so `findData` now states its refusal in code — a thrown `Error` carrying the already-registered `INVALID_REQUEST` code — instead of relying on an incidental `TypeError` from a property read on `undefined`. No `??` fallback and no optional chaining were added: both spell match-everything, which is the defect this closes.
+
+No API, request body, response shape or exported signature changes. A caller that reaches `findData` through `runImport` always supplies `query`, so no supported call moves; only a protocol call that was already failing now fails with a code attached.

packages/plugins/plugin-auth/src/admin-import-users.test.ts

@@ -1,12 +1,18 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { describe, it, expect, vi } from 'vitest';
+import { readFileSync } from 'node:fs';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
 import { assertEngineUpdateDispatch } from '@objectstack/objectql';
 import { runAdminImportUsers, IMPORT_USERS_MAX_ROWS, type IdentityImportDeps } from './admin-import-users.js';
 import type { AdminActor } from './admin-user-endpoints.js';
 
 const ACTOR: AdminActor = { id: 'admin-1', email: 'admin@example.com' };
 
+const HERE = dirname(fileURLToPath(import.meta.url));
+const IMPORT_USERS_SOURCE = readFileSync(resolve(HERE, 'admin-import-users.ts'), 'utf8');
+
 function makeRequest(body: unknown): Request {
   return new Request('http://localhost/api/v1/auth/admin/import-users', {
     method: 'POST',
@@ -624,3 +630,33 @@ describe('runAdminImportUsers — CSV payloads', () => {
     expect(m.createUser.mock.calls.map((c) => c[0].body.email).sort()).toEqual(['c1@x.co', 'c2@x.co']);
   });
 });
+
+/**
+ * [#17422] The IMPLEMENTOR half of #16952's contract.
+ *
+ * `ImportProtocolLike` types the three required members, but an EXPLICIT
+ * parameter annotation wins over a contextual type — so `findData(args: any)`
+ * opts this file back out of the contract while `tsc --noEmit` stays green.
+ * Measured on #17422 in this package: with the annotation restored, a probe
+ * reading the retired wire alias (`args.query?.$filter` — the pre-#16950 read
+ * whose `?? {}` default degraded the duplicate probe into match-everything)
+ * type-checks at exit 0; with the annotation gone the same probe is
+ * `TS2339 Property '$filter' does not exist on type 'QueryInput'`.
+ *
+ * ⇒ Nothing else in the repo can see that difference. The behavioural upsert
+ * tests above discriminate the CONSEQUENCE (ablated to the historical read,
+ * two of them go red) but not the opt-out itself: re-annotating the parameter
+ * leaves every one of them green and every gate green. This is that guard.
+ */
+describe('[#17422] the import protocol literal is typed BY `ImportProtocolLike`', () => {
+  it('binds the literal to the exported contract', () => {
+    expect(IMPORT_USERS_SOURCE).toContain('const protocol: ImportProtocolLike = {');
+  });
+
+  for (const member of ['findData', 'createData', 'updateData'] as const) {
+    it(`leaves \`${member}\`'s parameter unannotated, so the contract types it`, () => {
+      expect(IMPORT_USERS_SOURCE).toContain(`async ${member}(args) {`);
+      expect(IMPORT_USERS_SOURCE).not.toMatch(new RegExp(`async\\s+${member}\\s*\\(\\s*args\\s*:`));
+    });
+  }
+});

packages/plugins/plugin-auth/src/admin-import-users.ts

@@ -362,17 +362,34 @@ export async function runAdminImportUsers(
     // to `{}` stops constraining anything, so the duplicate probe matches rows
     // it was given no key for and the upsert updates the WRONG user. One
     // dialect, read straight — a request that arrives without a `query` is a
-    // caller defect and costs a loud TypeError, not a silent match-everything.
-    async findData(args: any) {
-      const where = args.query.where;
-      const limit = args.query.limit;
+    // caller defect and is refused loudly, not softened into match-everything.
+    //
+    // [#17422] The parameter is deliberately UNANNOTATED: `ImportProtocolLike`
+    // types it, and an explicit annotation here would win over that contextual
+    // type and opt this implementor back out of the contract (the runner's own
+    // docblock says so). `FindDataRequest` declares `query` OPTIONAL, so the
+    // contract makes this file write its refusal down instead of leaving it as
+    // an incidental TypeError from a property read on `undefined`.
+    // ⛔ Not `args.query ?? {}` and ⛔ not `args.query?.where`: both spell
+    // match-everything, which is the exact regression this protocol's history
+    // is about.
+    async findData(args) {
+      const query = args.query;
+      if (!query) {
+        throw Object.assign(
+          new Error('import-users: findData was called without a query — refusing to match every user'),
+          { code: 'INVALID_REQUEST' },
+        );
+      }
+      const where = query.where;
+      const limit = query.limit;
       return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any);
     },
 
     // One better-auth create per row — hashing + credential sys_account.
     // Deliberately NO createManyData: there is no safe bulk primitive for
     // identities, and scrypt dominates the cost anyway.
-    async createData(args: any) {
+    async createData(args) {
       const data: Record<string, any> = args?.data ?? {};
       const email: string = typeof data.email === 'string' && data.email.length > 0
         ? data.email
@@ -431,7 +448,7 @@ export async function runAdminImportUsers(
 
     // Upsert updates touch PROFILE fields only — never email, never anything
     // credential- or system-managed. An empty filtered patch is a no-op.
-    async updateData(args: any) {
+    async updateData(args) {
       const patch: Record<string, any> = {};
       for (const [k, v] of Object.entries(args?.data ?? {})) {
         if (UPDATE_ALLOWED_FIELDS.has(k) && v !== undefined && v !== null && v !== '') patch[k] = v;

packages/services/service-automation/src/engine.ts

@@ -35,6 +35,13 @@ import { predicateSlotRefusal, resolveFlowNodeExpressions, structuralConditionRe
 // ({@link AutomationEngine.valueEnvelopeRefusals}), so a flow that registers can
 // never be refused at run time and vice versa.
 import { AssignmentValueSchema, ASSIGNMENT_VALUE_ENVELOPE_REFUSAL } from '@objectstack/spec/automation';
+// [#17322] The EVALUATED-slot rule, IMPORTED rather than re-derived. It is the
+// rule `FlowEdgeSchema.condition` already composes since #15807, so a node's
+// `config.condition` — which no schema stands in front of — is held to the same
+// notion of "blank" and answers the same published sentence. A second,
+// hand-written `trim()` here is exactly the drift the #15662 campaign built the
+// shared refusal to prevent. See `checkStructuralCondition` in `registerFlow`.
+import { EvaluatedExpressionInputSchema, EVALUATED_EXPRESSION_SOURCE_REQUIRED } from '@objectstack/spec';
 import { applyConversionsToFlow, type ConversionNotice, type ConversionConflictNotice } from '@objectstack/spec';
 // [ADR-0126 §7.3] "Does a code package ship this flow?" for the subflow guard.
 // Routed through the local precedence module rather than importing
@@ -8060,14 +8067,47 @@ export class AutomationEngine implements IAutomationService {
          * node's trigger gate is read from. Same severity as a malformed
          * predicate (this throws): the reject set of registration and the reject
          * set of evaluation must be one set.
+         *
+         * [#17322] SECOND gate, after the shape one and before the CEL one: the
+         * source must be non-blank. `structuralConditionRefusal` admits every
+         * string by design, so a whitespace-only `config.condition` passed here
+         * and landed on `evaluateCondition`'s empty-source arm — a SILENT
+         * `false`, i.e. a branch that never runs, forever, under a comment that
+         * names that arm as being for an UNAUTHORED condition. `'   '` was
+         * authored, and on a `start` node that key is the trigger gate. Since
+         * #15807 the EDGE door refuses exactly this value at
+         * `FlowSchema.parse`, and 带治理的一侧胜出,另一侧改绑: the node door,
+         * which has no schema in front of it, aligns to the governed side HERE,
+         * at the producer.
          */
+        const evaluatedSourceRefusal = (raw: unknown): { message: string; source: string } | undefined => {
+            // Reached only after `structuralConditionRefusal` cleared the value,
+            // so `raw` is bare text or an envelope carrying a string `source`.
+            const source = typeof raw === 'string' ? raw : (raw as { source?: unknown }).source;
+            if (typeof source !== 'string') return undefined;
+            // The rule and its sentence both come from the edge door's own
+            // schema. Applied to the SOURCE rather than to the whole value on
+            // purpose: the union would also refuse an envelope with no
+            // `dialect` or a dialect outside its enum, both of which this slot
+            // admits (`structuralConditionRefusal`'s docblock, and #4336) — and
+            // refusing them would widen this narrowing past what was ruled.
+            const verdict = EvaluatedExpressionInputSchema.safeParse(source);
+            if (verdict.success) return undefined;
+            return { message: verdict.error.issues[0]?.message ?? EVALUATED_EXPRESSION_SOURCE_REQUIRED, source };
+        };
+
         const checkStructuralCondition = (where: string, raw: unknown): void => {
             if (raw == null) return;
             const shapeRefusal = structuralConditionRefusal(raw);
             if (shapeRefusal) {
                 failures.push(`  • ${where}: ${shapeRefusal.message}\n      source: \`${shapeRefusal.source}\``);
                 return;
             }
+            const blankRefusal = evaluatedSourceRefusal(raw);
+            if (blankRefusal) {
+                failures.push(`  • ${where}: ${blankRefusal.message}\n      source: \`${blankRefusal.source}\``);
+                return;
+            }
             check(where, raw);
         };