fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298) (#15311)

* fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298)

`declaredPermissionSets`' docblock stated a short-circuit the code did not
have: it pushed the flattened top level and then every package body
unconditionally, collecting each permission set twice on today's additive
artifact. The duplication was unobservable at the sole (private) call site,
which takes the first `isDefault` set, so this corrects a false written
contract on a security-path reader rather than a live defect.

The reader now walks the discipline its docblock claims — start from the
expression the card replaced (`appDefaultPermissionSetName(config.permissions)`)
and consult `packages[]` only where it came back `undefined`.

The condition is the resolved NAME, never the `permissions` container:
branching on the container re-creates the silent loss the card removed one
shape further along, since a flattened level that carries sets but marks none
of them `isDefault` is legal and hand-authorable.

`resolveArtifactPackageOrder` is called before the top level is consulted, so
its ADR-0112 refusal of a malformed `packages` stays unconditional instead of
depending on which level happened to answer first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m

* chore(changeset): record the plugin-security default-permission-set reader correction (#15298)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m

* docs(plugin-security): say why this reader branches on the answer where the sibling branches on the container (#15298)

`resolveStackCollection` (packages/cli/src/utils/stack-collections.ts, #15006)
landed on main branching on the container, and is right to: it returns a whole
collection, so a present top-level key has already answered. This reader
extracts a distinguished element out of the collection, so "present" and
"answers" are different facts here.

Recording the reason in the docblock so the convergence pass reads two readers
that differ AND say why, rather than two that differ while appearing to agree.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m

* chore(changeset): name the #15226 sentence this entry supersedes in the same unreleased batch (#15298)

Both entries are patch bumps on @objectstack/plugin-security and will compile
into one release. Leaving the earlier one as written keeps the record honest;
naming the superseded sentence keeps the compiled notes from carrying a
contradiction.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: hotlong

.changeset/plugin-security-default-set-answer-not-container.md

@@ -0,0 +1,57 @@
+---
+"@objectstack/plugin-security": patch
+---
+
+fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298)
+
+`declaredPermissionSets` carried a docblock stating a short-circuit its code did
+not have:
+
+> The `packages[]` pass only supplies a set where the top level had none — which
+> is precisely the option-B artifact.
+
+The code pushed the flattened top level and then **every** package body
+unconditionally, so on today's additive artifact (flattened level *and*
+`packages[]` both present) every permission set was collected twice. Nothing
+observable came of it — the sole caller is private and takes the first
+`isDefault` set, which the flattened copy still supplied — so this corrects a
+false written contract on a security-path reader, not a live defect. That
+distinction is the point: the sentence was load-bearing, because it was the
+stated reason the reader half was revertible on its own and safe to land before
+the emitter half (#14512), and the next reader would have believed the mechanism
+was there.
+
+⚠️ Release-notes note: this supersedes one sentence of the #15226 entry in this same
+unreleased batch — "The resolution now reads the flattened top level FIRST and then each
+package body". That described #15226 accurately when it landed; after this change the
+`packages[]` pass runs only where the top level named no default. The earlier entry is
+left as written rather than retro-edited, so whoever compiles the notes collapses the two
+deliberately instead of reading a contradiction.
+
+The reader now walks the discipline the docblock claims — start from the
+expression this program replaced, `appDefaultPermissionSetName(config.permissions)`,
+and consult `packages[]` only where it came back `undefined`.
+
+- **The condition is the resolved NAME, never the `permissions` container.**
+  Branching on the container re-creates the silent loss the reader program
+  exists to remove, one shape further along: a flattened level that carries
+  permission sets but marks none of them `isDefault` is legal today and
+  hand-authorable in any `objectstack.config.ts`, and a container-shaped
+  condition (`Array.isArray(flattened)`, with or without `&& length > 0`) shorts
+  it past the whole `packages[]` pass and answers `undefined` — nothing thrown,
+  nothing logged, every member of the app back down to the platform floor alone.
+  Reading the answer also retires the `[]`-is-truthy trap rather than patching
+  around it.
+- **The package order is resolved BEFORE the top level is consulted.**
+  `resolveArtifactPackageOrder` refuses a malformed `packages` — not an array,
+  an entry inlined instead of wrapped under `manifest:`, a duplicate package id
+  — with an ADR-0112 envelope this reader does not catch, and that refusal must
+  not become conditional on whether the flattened level happened to name a
+  default first. An artifact is either loadable or refused; which level answered
+  is not part of that question.
+- **No emitted artifact changes its answer.** Measured, not argued: 26 shapes —
+  the composed additive artifact, its option-B derivative, the collection-zoo
+  fixtures behind the #15004 acceptance pin, every config the unit suite drives,
+  the three malformed-`packages` refusals, and the hand-authored mixed shapes —
+  return byte-identical results before and after, with `@objectstack/plugin-security`
+  rebuilt and the change proven present in `dist/` on each leg.

packages/plugins/plugin-security/src/app-default-permission-set.test.ts

@@ -259,6 +259,56 @@ describe('appSecurityPluginOptions over `packages[]` (ADR-0130 D4, #15007)', ()
     expect(appSecurityPluginOptions({ permissions: [permissionSet('top')] })).toEqual({ fallbackPermissionSet: 'top' });
   });
 
+  /**
+   * [#15007 follow-up] "The top level had none" is the resolved NAME coming
+   * back `undefined` — never the `permissions` CONTAINER being absent or empty.
+   *
+   * Branching on the container re-creates the silent loss this card removed,
+   * one shape further along. A flattened level that carries permission sets but
+   * marks none of them `isDefault` is legal today and hand-authorable in any
+   * `objectstack.config.ts`; a container-shaped condition shorts it past the
+   * whole `packages[]` pass and answers `undefined` — nothing thrown, nothing
+   * logged, every member of the app back down to the platform floor alone.
+   */
+  describe('the `packages[]` pass runs wherever the top level named no default', () => {
+    const corePackage = {
+      manifest: {
+        id: CORE_ID, name: 'Core', version: '1.0.0', type: 'app',
+        permissions: [permissionSet(CORE_PROFILE)],
+      },
+    };
+
+    it('an EMPTY flattened array does not short-circuit it', () => {
+      expect(appSecurityPluginOptions({ permissions: [], packages: [corePackage] }))
+        .toEqual({ fallbackPermissionSet: CORE_PROFILE });
+    });
+
+    it('a NON-EMPTY flattened array that marks no default does not either', () => {
+      // A `permissions.length > 0` guard passes the case above and fails this
+      // one — which is the whole reason the condition is the resolved name.
+      expect(
+        appSecurityPluginOptions({
+          permissions: [{ name: 'core_read_only', label: 'Read only', objects: {} }],
+          packages: [corePackage],
+        }),
+      ).toEqual({ fallbackPermissionSet: CORE_PROFILE });
+    });
+
+    it('a `permissions` key that is not an array at all does not either', () => {
+      expect(appSecurityPluginOptions({ permissions: null, packages: [corePackage] }))
+        .toEqual({ fallbackPermissionSet: CORE_PROFILE });
+    });
+
+    it('and once the top level DOES name one, the packages pass cannot change the answer', () => {
+      expect(
+        appSecurityPluginOptions({
+          permissions: [permissionSet('flattened_wins')],
+          packages: [corePackage],
+        }),
+      ).toEqual({ fallbackPermissionSet: 'flattened_wins' });
+    });
+  });
+
   /**
    * The gate travels with the read: `resolveArtifactPackageOrder` refuses a
    * malformed `packages` with an ADR-0112 envelope, and this reader does not
@@ -293,5 +343,21 @@ describe('appSecurityPluginOptions over `packages[]` (ADR-0130 D4, #15007)', ()
       expect(err.code).toBe('DUPLICATE_ARTIFACT_PACKAGE');
       expect(err.status).toBe(422);
     });
+
+    it('…and refused just the same when the flattened top level already named a default', () => {
+      // The package order is resolved BEFORE the top level is consulted, so an
+      // artifact is either loadable or refused independently of which level
+      // happens to answer. Move that resolution below the early return and this
+      // pair turns into a silent accept: a permission surface resolved out of an
+      // artifact the manifest service refuses moments later.
+      const notAnArray = refusalOf({ permissions: [permissionSet('flattened_wins')], packages: 'nope' });
+      expect(notAnArray.code).toBe('INVALID_ARTIFACT_PACKAGES');
+      expect(notAnArray.status).toBe(422);
+
+      const entry = { manifest: { id: CORE_ID, name: 'Core', version: '1.0.0', type: 'app', permissions: [permissionSet(CORE_PROFILE)] } };
+      const duplicate = refusalOf({ permissions: [permissionSet('flattened_wins')], packages: [entry, entry] });
+      expect(duplicate.code).toBe('DUPLICATE_ARTIFACT_PACKAGE');
+      expect(duplicate.status).toBe(422);
+    });
   });
 });

packages/plugins/plugin-security/src/app-default-permission-set.ts

@@ -86,8 +86,9 @@ export function appDefaultPermissionSetName(permissions: unknown): string | unde
 }
 
 /**
- * [ADR-0130 D4, #15007] Every permission set a stack config DECLARES — from the
- * flattened top level, and from `packages[]`.
+ * [ADR-0130 D4, #15007] The app-declared default permission-set NAME, resolved
+ * from wherever the artifact carries the declaration — the flattened top
+ * level, or `packages[]`.
  *
  * ## What this exists to stop
  *
@@ -114,52 +115,83 @@ export function appDefaultPermissionSetName(permissions: unknown): string | unde
  * this function has to be a superset of the old read rather than a replacement
  * for it: for every artifact the platform emits today the flattened level
  * answers first and this returns exactly what it returned before. The
- * `packages[]` pass only supplies a set where the top level had none — which is
- * precisely the option-B artifact. That is what makes this card revertible on
- * its own and safe to land before the emitter half (#14512).
+ * `packages[]` pass is consulted ONLY where the top level named no default —
+ * which is precisely the option-B artifact. That is what makes this card
+ * revertible on its own and safe to land before the emitter half (#14512).
+ *
+ * ## The condition is the ANSWER, never the container
+ *
+ * "The top level had none" is spelled as `appDefaultPermissionSetName` coming
+ * back `undefined`, and deliberately NOT as the `permissions` array being
+ * absent or empty. Branching on the container re-creates the silent loss this
+ * card exists to remove, one shape further along: a config whose flattened
+ * level carries permission sets but marks none of them `isDefault` — legal
+ * today, and expressible by hand in any `objectstack.config.ts` — would
+ * short-circuit the whole `packages[]` pass and resolve `undefined`, with
+ * nothing thrown and nothing logged. Reading the container also hands back the
+ * `[]`-is-truthy trap for free. The answer is the only condition that cannot
+ * be wrong in either direction, so the answer is what this branches on.
+ *
+ * ⚠️ That is deliberately NOT the shape of the sibling reader's condition, and
+ * the difference is a property of the readers, not an inconsistency to
+ * converge away. `resolveStackCollection` (`packages/cli/src/utils/
+ * stack-collections.ts`, #15006) branches on the CONTAINER — `if
+ * (Array.isArray(top)) return top;` — and is right to: it returns a whole
+ * collection, so a top level that carries the key has, by construction,
+ * already answered, and `composeStacks` flattened that array into the union.
+ * This reader extracts a DISTINGUISHED ELEMENT out of the collection instead,
+ * so "the key is present" and "the key answers" are two different facts here
+ * and one of them is the wrong one to branch on. Same discipline — start from
+ * the expression this program replaced, consult `packages[]` only where it came
+ * back empty — read against what each reader's expression actually returns.
  *
  * ## The order is `resolveArtifactPackageOrder`'s, not the array's
  *
- * `appDefaultPermissionSetName` resolves the FIRST `isDefault` set, so with more
- * than one package declaring one, "first" has to mean the same thing here as it
- * does everywhere else the artifact is read. `resolveArtifactPackageOrder`
+ * The first package body that names a default wins, so with more than one
+ * package declaring one, "first" has to mean the same thing here as it does
+ * everywhere else the artifact is read. `resolveArtifactPackageOrder`
  * (`@objectstack/core`, ADR-0130 D4+D5, #14643) is the ONE place that turns an
  * artifact into its ordered package list — dependency-topological, so a package
  * that extends another is read after it regardless of which array slot it
  * occupies. ⛔ Do not iterate `config.packages` directly here; a second
  * traversal is a second ordering, and the depended-upon package would win or
  * lose by authoring accident.
  *
- * ## Two things it deliberately does NOT do
- *
- *   • It does not look inside the SINGULAR `manifest`. That constraint is
- *     #7001's and it still holds — the harness must not honour a declaration
- *     `serve.ts` ignores. Note this is not a special case bolted on: an
- *     artifact carrying no `packages` key makes `resolveArtifactPackageOrder`
- *     return the caller's own object as the single package body (D4's second
- *     branch, D7's compatibility term), so that branch reads `permissions` from
- *     exactly where the old code read it and nowhere else.
- *   • It does not catch `resolveArtifactPackageOrder`'s refusals. A malformed
- *     `packages` (not an array, an unwrapped entry, a duplicate package id)
- *     raises an ADR-0112 envelope here, the same one the manifest service
- *     raises when it registers that artifact moments later. Swallowing it would
- *     resolve a permission surface out of an artifact the loader refuses to
- *     load — the gate travels with the read.
+ * ## The package order is resolved BEFORE the top level is consulted
+ *
+ * Reading that line as a misplaced statement is the expected mistake, so: it is
+ * placed there on purpose, and moving it below the early return is a behaviour
+ * change. `resolveArtifactPackageOrder` REFUSES a malformed `packages` (not an
+ * array, an unwrapped entry, a duplicate package id) with an ADR-0112 envelope,
+ * and this reader does not catch it — swallowing it would resolve a permission
+ * surface out of an artifact the loader refuses to load. Resolving the order
+ * first is what keeps that refusal unconditional: an artifact is either
+ * loadable or refused, and which answer this reader gives about it must not
+ * depend on whether its flattened level happened to name a default first.
+ *
+ * ## One thing it deliberately does NOT do
+ *
+ * It does not look inside the SINGULAR `manifest`. That constraint is #7001's
+ * and it still holds — the harness must not honour a declaration `serve.ts`
+ * ignores. Note this is not a special case bolted on: an artifact carrying no
+ * `packages` key never reaches the package pass at all, so that branch reads
+ * `permissions` from exactly where the old code read it and nowhere else.
  */
-function declaredPermissionSets(config: unknown): unknown[] {
-  const sets: unknown[] = [];
+function declaredDefaultPermissionSetName(config: unknown): string | undefined {
+  const packages = (config as { packages?: unknown } | null | undefined)?.packages;
+  const bodies =
+    packages === undefined || packages === null ? [] : resolveArtifactPackageOrder(config);
 
   const flattened = (config as { permissions?: unknown } | null | undefined)?.permissions;
-  if (Array.isArray(flattened)) sets.push(...flattened);
-
-  const packages = (config as { packages?: unknown } | null | undefined)?.packages;
-  if (packages === undefined || packages === null) return sets;
+  const fromFlattened = appDefaultPermissionSetName(flattened);
+  if (fromFlattened !== undefined) return fromFlattened;
 
-  for (const body of resolveArtifactPackageOrder(config)) {
+  for (const body of bodies) {
     const declared = (body as { permissions?: unknown } | null | undefined)?.permissions;
-    if (Array.isArray(declared)) sets.push(...declared);
+    const fromPackage = appDefaultPermissionSetName(declared);
+    if (fromPackage !== undefined) return fromPackage;
   }
-  return sets;
+  return undefined;
 }
 
 /**
@@ -191,13 +223,14 @@ function declaredPermissionSets(config: unknown): unknown[] {
  * the result straight through — `new SecurityPlugin(appSecurityPluginOptions(config))`
  * — and a caller cannot get the undefined case subtly wrong.
  *
- * Reads the sets through {@link declaredPermissionSets} — the flattened top
- * level `serve.ts` has always read, and, for a multi-package artifact, the
- * `packages[]` bodies that carry the same declaration under ADR-0130 D4.
+ * Resolves the name through {@link declaredDefaultPermissionSetName} — the
+ * flattened top level `serve.ts` has always read, and, for a multi-package
+ * artifact, the `packages[]` bodies that carry the same declaration under
+ * ADR-0130 D4.
  */
 export function appSecurityPluginOptions(
   config: unknown,
 ): { fallbackPermissionSet: string } | undefined {
-  const name = appDefaultPermissionSetName(declaredPermissionSets(config));
+  const name = declaredDefaultPermissionSetName(config);
   return name ? { fallbackPermissionSet: name } : undefined;
 }