fix(approvals): refuse cross-org targeting on directory-less approver types (ADR-0105 D9)

.changeset/adr-0105-d9-refuse-on-directory-less-types.md

@@ -0,0 +1,26 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): refuse `organization` on directory-less approver types instead
+of silently ignoring it (ADR-0105 D9)
+
+`user`, `field` and `manager` return EARLY in `resolveApproverSpec` — they name
+a person outright rather than expanding a directory. D9's org resolution was
+placed after those returns, so an `organization` declared on one of them never
+reached the check: it was silently INERT.
+
+That is the one behaviour ADR-0105 D9 rules out and the authoring docs
+explicitly promise against ("`organization` on those is refused at runtime").
+The `os lint` rule caught it at author time, but the runtime claim was false —
+and a stored flow that predates the lint, or one assembled programmatically,
+got no signal at all.
+
+Resolution now happens at the top of `resolveApproverSpec`, above every early
+return, so the refusal reaches all three types. The ordinary path is unchanged
+and still costs nothing: with no `organization` declared the resolver returns
+the request's organization without reading anything.
+
+Found by cloud's group-posture dogfood driving a real `group` boot — the
+resolver's own unit tests could not see it, because they call the resolver
+directly and never traverse the early return.

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -784,6 +784,23 @@ export class ApprovalService implements IApprovalService {
         { deprecated: a.type, canonical: type },
       );
     }
+    // [ADR-0105 D9] WHERE this approver is looked up — the request's own
+    // organization unless the spec targets another one in the same group.
+    //
+    // Resolved HERE, above the `user` / `field` / `manager` early returns,
+    // because refusing a declaration on a directory-less type is one of the
+    // things this resolution DOES (those types name a person outright, so
+    // `organization` on them cannot narrow anything and an author who wrote it
+    // misunderstood the field). Resolving it after those returns made the
+    // refusal unreachable and the declaration silently inert — exactly the
+    // "ignored, not refused" behaviour ADR-0105 D9 rules out, and what the
+    // cloud group-posture dogfood caught.
+    //
+    // Costs nothing on the overwhelmingly common path: with no `organization`
+    // declared, the resolver returns the request org without reading anything.
+    const directoryOrg = await this.directoryOrgFor(a, organizationId);
+    const crossOrg = directoryOrg !== organizationId;
+
     if (type === 'user') {
       return this.applyOooDelegation(String(a.value), now, organizationId, substitutions);
     }
@@ -798,12 +815,11 @@ export class ApprovalService implements IApprovalService {
       }
       return out;
     }
-    // [ADR-0105 D9] WHERE this approver is looked up — the request's own
-    // organization unless the spec targets another one in the same group.
-    // Resolution failures propagate: they are routing bugs, and this call is
-    // OUTSIDE the swallowing try below on purpose (see the catch's comment).
-    const directoryOrg = await this.directoryOrgFor(a, organizationId);
-    const crossOrg = directoryOrg !== organizationId;
+    // `directoryOrg` / `crossOrg` were resolved at the TOP of this method, so
+    // the refusal reaches directory-less types too. Resolution failures
+    // propagate: they are routing bugs, and that call sits OUTSIDE the
+    // swallowing try below on purpose (see the catch's comment).
+    //
     // A cross-org slate is filtered to the people who can actually READ the
     // request (D2 union); same-org routing is untouched and does no extra read.
     const bounded = async (users: string[]): Promise<string[]> => (

packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts

@@ -158,6 +158,33 @@ describe('ADR-0105 D9 — cross-org approver targeting through ApprovalService',
     expect(req.pending_approvers).toEqual(['position:cfo']);
   });
 
+  // Regression: `user` / `field` / `manager` return EARLY in
+  // `resolveApproverSpec`, before the graph-expansion branch. D9's resolution
+  // originally sat after those returns, so the declaration on a directory-less
+  // type was silently INERT rather than refused — the one behaviour ADR-0105 D9
+  // and the authoring docs both promise it is not. The resolver's own unit test
+  // could not see it (it calls the resolver directly); only a request opened
+  // through the service reaches the early return. Caught by cloud's
+  // group-posture dogfood.
+  it('refuses `organization` on a directory-less type — the early return must not skip the check', async () => {
+    for (const spec of [
+      { type: 'user', value: 'u_cfo', organization: '$root' },
+      { type: 'field', value: 'owner_id', organization: '$root' },
+      { type: 'manager', organization: '$root' },
+    ]) {
+      await expect(
+        svc.openNodeRequest(openInput([spec]), CTX),
+        `approver type '${spec.type}' must refuse a cross-org declaration`,
+      ).rejects.toThrow(/VALIDATION_FAILED.*no effect/);
+    }
+  });
+
+  it('a directory-less approver with NO declaration is untouched by D9', async () => {
+    // The guard must not cost the ordinary case anything.
+    const req = await svc.openNodeRequest(openInput([{ type: 'user', value: 'u_plant_mgr' }]), CTX);
+    expect(req.pending_approvers).toEqual(['u_plant_mgr']);
+  });
+
   it('refuses a target outside the group — loudly, and no request is created', async () => {
     engine._tables['sys_organization'].push({ id: 'o_rival', slug: 'rival-co', parent_organization_id: null });
     await expect(