fix(plugin-security): declaredPermissionSets' docblock states a short-circuit the code does not have — it concatenates both copies

Follow-up to #15007, which landed as PR #15226 (merged 08:59Z). Filed as a fresh card because that PR is merged and a merged PR cannot carry follow-up work.

The divergence, on `main` today

packages/plugins/plugin-security/src/app-default-permission-set.ts, the declaredPermissionSets docblock:

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).

The code does not do that:

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;

for (const body of resolveArtifactPackageOrder(config)) {
  const declared = (body as { permissions?: unknown } | null | undefined)?.permissions;
  if (Array.isArray(declared)) sets.push(...declared);   // ← unconditional, no de-duplication
}

The flattened copy goes in first, then every package body unconditionally. On today's additive artifact (flattened top level and packages[] both present) every permission set is collected twice. There is no short-circuit and no de-duplication.

Impact: no behaviour change today — this is a false written contract, not a live defect

Stated plainly so it is not re-triaged as a P0:

the only consumer is appSecurityPluginOptions in the same file, which calls appDefaultPermissionSetName, which returns the first isDefault set;
the flattened copy is pushed first, so "first" is still the one the old code found;
declaredPermissionSets is private (not exported, no second caller).

So the duplication is unobservable, #15007's four-step measurement and its ablation stand, and the B2 · plugin-security … ledger row it deleted was genuinely earned.

Why it still needs fixing

The docblock sells the short-circuit as the reason the card was revertible on its own and safe to land before the emitter half. That reasoning is sound (the observable result really is unchanged), but the mechanism it names does not exist in the code. The next reader will believe it does.
That next reader is already scheduled: the convergence pass that collapses the program's private packages[] walks onto one reader will read this docblock.
This program exists because readers silently lost things. Leaving a reader whose written self-description is false, in a security path, is the wrong direction.

The fix (preference, not a mandate)

Make the code match the docblock, so #15006's stack-collections.ts, this file, and the runtime reader carry one discipline — start from the expression you replaced, consult packages[] only where it came back falsy:

const flattened = (config as { permissions?: unknown } | null | undefined)?.permissions;
if (Array.isArray(flattened) && flattened.length > 0) return flattened;

⚠️ length > 0 is required, not fastidiousness. The empty-array truthiness trap #15006 measured runs the other way here: Array.isArray(flattened) alone would let a top-level permissions: [] short-circuit the whole packages[] pass, re-creating the silent loss #15007 just fixed in a different shape. With length > 0 the return value is byte-identical to today's on every shape the platform emits, and the docblock becomes true.

The other route is also acceptable: keep the concatenation and fix the docblock — stating that the duplication is unobservable at the sole call site because it takes the first isDefault, and that this is deliberately a different discipline from the runtime reader's identity-claim de-duplication. Two readers that differ are fine; two readers that differ while reading as though they agree are not.

Acceptance

⛔ appSecurityPluginOptions' return value must be byte-identical on every shape the platform emits today, shown by measurement rather than argued.
A positive test for the trap above: a top-level permissions: [] plus a package body carrying an isDefault set must still resolve that set.
The existing 18 unit tests stay green.
⛔ Do not touch packages/cli/test/option-b-reader-acceptance.pin.test.ts — OPTION_B_LOSSES, its set-equality assertion, and the row fix(plugin-security): the app default permission set resolves from packages[] (#15007) #15226 deleted all stay exactly as they are; the pin stays green.
⛔ Never edit content/docs/releases/.
No changeset level change is owed: declaredPermissionSets is private, so the published surface does not move.

Provenance

Found by the isolated adversarial contract review of PR #15261, which flagged it as an out-of-scope note while verifying that PR's dependency claims. Relayed to #15226 as comment 5537872132 before it merged; auto-merge was disarmed for it, and it was merged anyway by an actor with merge rights, which is their call — the finding simply travels to this card instead.

Sibling context: #15005, #15006, #15007, #15229, #15232. Related: #15293 (the packages guard split across the same three readers).

Claiming this card.

session: session_01UHvF5hyiZjnCyExFnfQB8m
branch: claude/issue-15298-declared-permission-sets-docblock (branched from origin/main 9690d114b, which carries fix(plugin-security): the app default permission set resolves from packages[] (#15007) #15226)

Not touching claude/issue-15007-plugin-security-permissions — that branch's PR is merged.

Generated by Claude Code

fix(plugin-security): the app default permission set resolves from th…

chore(changeset): record the plugin-security default-permission-set r…

docs(plugin-security): say why this reader branches on the answer whe…

chore(changeset): name the #15226 sentence this entry supersedes in t…

os-dev-report

{
  "issue": 15298,
  "status": "done",
  "branch": "claude/issue-15298-declared-permission-sets-docblock",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/15311",
  "premise_still_valid": true,
  "summary": "Took route 1 (code aligns to docblock), but NOT with the dispatched snippet: I branch on the resolved ANSWER (appDefaultPermissionSetName back undefined) rather than the permissions CONTAINER. Measurement, not preference — the dispatched `Array.isArray(flattened) && flattened.length > 0` moves 3 of 26 corpus shapes, because a flattened level carrying sets but marking none isDefault (legal, hand-authorable) would short-circuit the whole packages[] pass and answer undefined. resolveArtifactPackageOrder is also called BEFORE the early return, which is what keeps its ADR-0112 refusal of a malformed packages unconditional; that choice is what holds 2 more of the 26 rows identical. Docblock now also records why this reader's condition differs from resolveStackCollection's container-shaped one (#15006 landed on main mid-task): that reader returns a whole collection so 'present' == 'answers', this one extracts a distinguished element so they are different facts. Branch was re-cut from main after the re-dispatch and merged up to origin/main 3a4373f4c; PR #15311 body fully rewritten, PM's own correction section preserved.",
  "tests": "All at HEAD 274ab06bc (merged to origin/main 3a4373f4c); every exit code captured before any pipe. (1) ACCEPTANCE — differential corpus over 26 shapes (4 emitted incl. the #15004 collection-zoo fixtures, 14 unit-suite configs, 3 malformed-packages refusals recorded as thrown code+status, 5 hand-authored mixed), 3 legs each rebuilding @objectstack/plugin-security and proving dist/: leg A HEAD src 0ab95e53 == HEAD blob, preflight 'marker present in 4 built files'; leg B pre-change file from 9690d114b, src 727c8455 == that blob, on-disk sets.push x2 / new name x0, preflight 'sets.push present in 2 built files' + 'new name absent from all 6'; leg C back to HEAD, 0ab95e53 == HEAD blob. diff before after -> empty exit 0. diff after after2 -> empty exit 0, so the restore leg is proven by RE-MEASUREMENT, not by a checkout exit code. (2) ABLATIONS on the same tree, each mutation confirmed on disk (injected marker 1, replaced anchor 0, hash vs HEAD blob) and each restore proven by hash equality + marker 0: control 23 passed; container-shaped WITH length>0 (the dispatched snippet) 1 failed | 22 passed -- 'a NON-EMPTY flattened array that marks no default does not either'; container-shaped WITHOUT length>0 2 failed | 21 passed -- adds 'an EMPTY flattened array does not short-circuit it' (the card's mandated positive test); restored 23 passed. No rebuild needed for this ablation and I say why: the unit suite imports './app-default-permission-set' relative inside its own package, so vitest resolves SOURCE; the dist discipline is owed by the CLI pin and paid in (1) and (3). (3) SUITES — plugin-security unit 23 passed (18 that were on main + 5 new); packages/cli/test/option-b-reader-acceptance.pin.test.ts 7 passed against a dist/ proven to carry the change (file untouched, OPTION_B_LOSSES and the #15226-deleted row exactly as they were). (4) GATES — 46 families derived by scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack; re-derived after the merge because the first derivation WARNED it had read stale copies of its own inputs, and it had: 45 -> 46, gaining check:cli-test-child-env. All 46 green. Two honest non-results first: check:dual-build-cjs-loads and check:i18n exited 3 = PREREQUISITE NOT MET ('nothing was measured'), read as NOT MEASURED rather than red; after turbo run build over ./packages/* and ./packages/*/* (71/71 successful) both re-ran green. check:type-check-debt genuinely re-measured (14 ledger entries, 601.6s, 153 raw errors, none above recorded).",
  "mcp_calls": "4 — 1 claim comment, 1 PR body read, 1 PR body update, 1 PR read-back (this report comment makes 5). Card body and the #15226 relay comment were read through the zero-quota public-repo payload channel, not MCP.",
  "open_questions": [
    {
      "question": "The #15226 changeset is on main, unreleased, and this PR falsifies one of its sentences: 'The resolution now reads the flattened top level FIRST and then each package body.' Both entries are patch bumps on the same package and compile into the same release.",
      "options": [
        "A — leave #15226's entry as written (it described #15226 accurately when it landed) and have my changeset NAME the superseded sentence, so whoever compiles the notes collapses the two deliberately. This is what I shipped.",
        "B — retro-edit #15226's changeset so the compiled notes describe the shipped behaviour once."
      ],
      "recommendation": "A, and it is already in the PR. B rewrites a record of what another change actually did, which is the same defect class this card exists to close, one layer out. A leaves both records true and hands the collapse decision to the release compiler. Flagging rather than deciding silently because release-notes compilation is yours, not mine."
    }
  ],
  "out_of_scope_findings": []
}

Generated by Claude Code

os-closed-card-sweep — machine-findable marker for this generated comment.

Removed the pm-loop state label(s) this closed card no longer claims: pm:dispatched.

Closing pull request: fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298) #15311, merged.
Closing commit c64e65fb17, merged into main.
Left untouched: domain:spec, priority:p3 — ownership, priority and outcome are not state claims.
The label set was read back after the write and matched.

A state label claims work is in flight. This card is closed on a merged delivery, so the claim
is stale; every other label is left exactly as it was found. Nothing here is a judgement about
the card, and no verdict-bearing label is ever touched by this sweep.

posted by half-state-patrol run 34005012908 · trigger schedule

Generated by Claude Code

fix(plugin-security): the app default permission set resolves from th…

Navigation Menu

Repository navigation

fix(plugin-security): `declaredPermissionSets`' docblock states a short-circuit the code does not have — it concatenates both copies #15298

The divergence, on `main` today

Impact: no behaviour change today — this is a false written contract, not a live defect

Why it still needs fixing

The fix (preference, not a mandate)

Acceptance

Provenance

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

fix(plugin-security): declaredPermissionSets' docblock states a short-circuit the code does not have — it concatenates both copies #15298

Description

The divergence, on main today

Impact: no behaviour change today — this is a false written contract, not a live defect

Why it still needs fixing

The fix (preference, not a mandate)

Acceptance

Provenance

Activity

hotlong commented on Sep 4, 2026

hotlong commented on Sep 4, 2026

github-actions commented on Sep 6, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

fix(plugin-security): `declaredPermissionSets`' docblock states a short-circuit the code does not have — it concatenates both copies #15298

The divergence, on `main` today