Skip to content

[finding] A package's whole declared permission set is dropped on a set-name collision with no diagnostic and no build gate — the comment says "refuse loudly", the code warns through a doubly-optional logger #17516

Closed

Description

@os-bill

Maintainer-action: rule how a domain:services card obtains the CONTRACT_REVIEW_TIER clause-② verdict its carrier needs — route it to the spec seat (naming how), or rule that this seat's default-judgment-tier review plus the gates suffices — done when node scripts/pm/check-clause2-carriers.mjs --pair 18022 exits 0 with the carrier cleared on both carriers

A package's entire declared permission set is dropped on a set-name collision, and the drop is invisible to the author who caused it.

packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts, the else arm at the skippedForeign branch (anchor by content — out.skippedForeign += 1), on origin/main d9e15877aa read 2026-09-10T18:25Z:

} else {
  // Package-namespaced object api names make set-name collisions a
  // packaging bug, not a merge case — refuse loudly (ADR-0086 D4:
  // a package never writes into a foreign record).
  out.skippedForeign += 1;
  logger?.warn?.('[security] permission set name owned by another package — skipped', {
    name: ps.name, declaredBy: packageId, ownedBy: existing.package_id,
  });
}

Why this is a finding and not a design choice

The comment says "refuse loudly". The code does not refuse loudly. ⚠️ Sharper than it looks: logger?.warn?. is optionally-chained twice, so a caller that passes no logger produces no output at all — the set vanishes with only an internal counter incremented. Nothing is written back where an author can read it: no diagnostic record, no os build check, no os validate check.

Measured absence, with a lit control so the zero means something:

skippedForeign referenced in packages/cli/src or packages/spec/src : 0 files
LIT CONTROL   'permission' in packages/cli/src                     : 27 files

Skipping the set is arguably the correct runtime behaviour (ADR-0086 D4: a package never writes into a foreign record). ⛔ This card does not propose changing that. The defect is that the refusal never reaches the author.

This is a class the repo has already ruled on — one layer over

This is the same failure shape #14553 just inverted for navigationContributions: present, plausible-looking, and silently different from what the author wrote. That fix gave nav a nav_contribution_group_missing diagnostic raised at BOTH doors — runtime (recordNavGroupMiss) and compile (os build / os validate via checkNavContributionGroups) — behind one shared predicate so the two cannot drift. The authorization surface never got the same treatment.

An AI author colliding on a set name is exactly how this fires, which puts it in class (c): metadata that will be stored and re-authored by someone other than its writer, silently dropped at the door.

Second, smaller defect in the same three lines

The comment's stated premise — "Package-namespaced object api names make set-name collisions a packaging bug, not a merge case" — is falsified by ADR-0130 D1, which lets N packages co-own one namespace. ADR-0130 already records this under its "What was NOT decided" section, so ⛔ no separate card is warranted; it belongs in the same change as the fix above.

Dedup

domain:spec open + closed, newest 100 each (⚠️ both at the API page cap, so bounded, not exhaustive). One near miss, not this: #16870 (readScope beside viewAllRecords). The round that surfaced this also searched via MCP and found only closed #9952 and #15298, neither of them this, with a live control hit.

Provenance

Surfaced by the #14488 proposal round, which was dispatched write-restricted (⛔ no card filing) and correctly handed it to the seat rather than filing it or dropping it. Re-verified independently by the seat before filing — including the double optional-chaining, which the round did not report.

⛔ Ungraded — triage sets type and priority. Successor: whoever builds #14488 edits this exact branch.


Generated by Claude Code

Activity

  1. claude commented on Sep 13, 2026

    @claude
    Contributor

    Claim: session_01URLHobLUJB9K1ABV6ofdjj · branch claude/issue-17516-permission-set-collision-diagnostic · domain:services execution seat, R7
    Branch: claude/issue-17516-permission-set-collision-diagnostic
    Clause-②: no

    ⚠️ That declaration is a PREDICTION, to be re-derived from the DELIVERED diff. It flips to yes if a diagnostic constant or predicate ends up re-exported from plugin-security/src/index.ts, or if a published payload gains a key. ⭐ The line above is copied from the template in the fixed spelling and left undecorated — correction 164: this seat wrote Clause-②: **no** in five claims this session and the carrier gate refused one of them, because the predicate reads the spelling, not the meaning.

    🔴 The precedent this card names spans THREE lanes — measured, not assumed

    The card points at #14553's navigationContributions fix as the shape to copy: a diagnostic raised at both doors, runtime and compile, behind one shared predicate so the two cannot drift. Measured where that precedent's parts actually live:

    part file lane
    the diagnostic constant packages/objectql/src/nav-contribution-diagnostics.ts:88 domain:engine (engine.md:7)
    the runtime raiser / registry wiring packages/objectql/src/{core,registry}.ts domain:engine
    the compile-time check packages/cli/src/utils/nav-contribution-groups.ts domain:cli (cli.md:7)
    ✅ the defect itself packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts domain:services — this lane (services.md:10)

    ⇒ this lane owns the defect, ⛔ not the whole shape of the precedent's remedy. packages/spec mentions the nav diagnostic only inside a .describe() docstring (1 file) — ⛔ the constant is not declared there, so a spec edit is likely not owed. ⚠️ "Likely" is a prediction; the delivery measures it.

    Scope

    Deliver the half this lane owns: make the refusal reach the author from plugin-security. The card's core complaint is precise — logger?.warn?. is optionally chained twice, so a caller passing no logger produces no output at all, and the set vanishes with only an internal counter moved. Measured absence with a lit control: skippedForeign referenced in packages/cli/src or packages/spec/src → 0 files; control 'permission' in packages/cli/src → 27 files.

    Also in scope, same three lines: the comment's stated premise — "Package-namespaced object api names make set-name collisions a packaging bug, not a merge case" — is falsified by ADR-0130 D1, which lets N packages co-own one namespace. ADR-0130 records this under "What was NOT decided", so ⛔ no separate card; it rides this change.

    ⛔ Fences


    Generated by Claude Code

  2. claude commented on Sep 13, 2026

    @claude
    Contributor

    os-dev-report

    {
      "issue": 17516,
      "status": "done",
      "branch": "claude/issue-17516-permission-set-collision-diagnostic",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/18022",
      "premise_still_valid": true,
      "summary": "The card's premise held exactly as written, double optional chain included. The runtime door is delivered: the set-name collision refusal now reaches the author through a console-backed default sink (per the #10556 ruling) and travels back as a structured diagnostic record on PermissionSeedOutcome.collisions, so a caller reading no log can still ask which sets were dropped and why. The skip is untouched (ADR-0086 D4) and every test asserts it still happens, still counts, and still leaves the foreign row unwritten. The predicate, wording and formatter live in one new module exported from the package entry so the compile-time door consumes them rather than re-deriving them (#14553's shape). The falsified comment premise is corrected in the same three lines: ADR-0130 D1 lets N packages co-own one namespace, so a collision is a legal configuration that gets more common, not a packaging bug. STOPPED AT A LANE FENCE on the compile-time half - see lane_fence below; nothing was pushed for it.",
      "tests": "REPRO (before, on BASE c45ea8c39, collision seeded, NO logger, all five console channels spied): skippedForeign=1, author-visible console lines=0, collisions on outcome=null. AFTER (same harness): skippedForeign=1 (unchanged), author-visible console lines=1 ('warn: [security] [permission_set_name_collision] 1 declared permission set was NOT materialized - another package already owns that set name (ADR-0086 D4). The declaring package's permissions are NOT in effect.'), collisions=1 full record. DISCRIMINATING CONTROL (non-colliding pass that really seeds and really re-seeds its own row): 0 console lines and collisions undefined, before AND after. ABLATION from the committed state: mutation proven on disk first (wiring marker 1->0, old doubly-optional spelling 0->1), new suite went to '4 failed | 5 passed', restore byte-identical to HEAD (blob c11b5d82fb129ae7d1f631a7d6f3507f1aec4004) with 'git diff HEAD' empty. SUITE at 6a5958acd after merging current main and rebuilding: pnpm --filter @objectstack/plugin-security test = 112 files / 2157 tests / 0 failures; typecheck clean (both legs joined with && so the verdict covers both). GATES: 24 derived families from scripts/pm/dispatch-gates.mjs all exit 0; plus check:dispatcher-error-vocabulary, check:error-code-casing, check:optional-error-sink, check:durability-log-level, check:logger-receiver-detach, check:startup-registry-verdict, check:nul-bytes, check:test-source-alias, check:cross-package-test-inputs, check:dts-closure, check:lean-entry-closure, check:published-files, check:tier-file-adoption, check:swallow-census-controls, check:adr-0087-registration, check:empty-changeset, check:changeset-gate-self-tests, check:type-check-coverage - all exit 0. check:type-check-debt first answered exit 3 (PREREQUISITE NOT MET); the full workspace closure it names was built (turbo 72/72 successful) and it re-ran to a real exit 0 ('5 ledger entries re-measured, 55 raw tsc errors, none above its recorded number'). check:engine-double-contract named the new test fake's update() as a loose double: it now opens with assertEngineUpdateDispatch and the pinned ledger was regenerated with the repo's own --write - 799 rows, 1 added or grown, 0 lost, byte-identical across two consecutive writes, gate then exit 0. check-clause2-carriers --pair 18022 exit 0 ('both carriers agree, no widening tell').",
      "clause_2_redetermination": {
        "claim_predicted": "no",
        "delivered": "yes",
        "method": "grep of each new exported symbol against the built published entry, which package.json exports['.'].types names as ./dist/index.d.ts, after a real build",
        "new_exported_symbols_reachable": {
          "PERMISSION_SET_NAME_COLLISION": 5,
          "permissionSetNameIsForeign": 3,
          "permissionSetNameCollisionDiagnostic": 3,
          "formatPermissionSetNameCollisionDiagnostic": 2,
          "reportPermissionSetNameCollisions": 3,
          "PermissionSetNameCollisionDiagnostic (type)": 6,
          "CollisionReportSink (type)": 3
        },
        "positive_control": "bootstrapDeclaredPermissions = 7 hits, so a hit means something",
        "negative_controls": "SHIPPED_PLATFORM_BUCKET_NAMES = 0, defaultLookup = 0 - both module-private, so a zero discriminates",
        "published_payload_key_added": "PermissionSeedOutcome.collisions? - a new optional key on an already-published payload",
        "consequence": "changeset graded minor (not patch); needs:contract-review hung on BOTH carriers (PR #18022 and card #17516) and read back on each; the PM's claim comment still reads 'Clause-②: no' and is the PM's record to correct, not mine"
      },
      "lane_fence": {
        "stopped": true,
        "door": "the compile-time door (os build / os validate)",
        "package": "packages/cli (domain:cli)",
        "detail": "Nothing was pushed for it. The #14553 precedent raises its diagnostic at both doors behind one shared predicate; only the runtime door is in this lane. The predicate, the diagnostic builder and the formatter are exported from @objectstack/plugin-security precisely so the cli-lane card consumes them instead of re-deriving either, which is the drift the precedent exists to prevent.",
        "packages_spec": "never touched - verified rather than assumed: the delivered diff is 9 files, none under packages/spec. The claim's prediction that no spec edit was owed held.",
        "packages_objectql_and_runtime": "never touched. Measured bonus: because the token is stamped as `event` (a snake_case data value, the same discrimination the sibling position_name_fold_grant already makes in this package) rather than as `code`, check:dispatcher-error-vocabulary passes with NO row owed in packages/runtime/src/dispatcher-error-vocabulary.ts - which is what the nav precedent needed and would have been a second lane fence."
      },
      "mcp_calls": "3 - issue_read(get), issue_read(get_comments), search_issues(dedup). Everything else went through repo-scoped REST or git. CHANNEL SWITCH DECLARED: the REST search endpoint answers 403 in this container ('sessions are bound to their configured repositories'), and the control query was equally dark, so that reading was void; the dedup went through one targeted MCP search_issues instead, whose channel is proven live by #17516 itself returning first.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #18023: bootstrap-declared-capabilities.ts carries the IDENTICAL doubly-optional logger at its own skippedForeign branch - a declared capability is dropped just as silently. Measured with the same harness: skippedForeign=1, author-visible console lines=0, against a lit control of 1 line on the repaired permission-set axis. Not ridden in on this card: it needs its own record type, its own outcome key and its own pins, so it would have widened this card's verification surface. Deduped first (one targeted query, channel proven live); nearest hits #11096 and #4967, neither of them this.",
        "noted, not filed: the ADR-0086 P2 publish materializer in security-plugin.ts returns its own 'permission set name is owned by another package' failure text, which is a second, pre-existing author-visible path for the publish door only. Left exactly as it was. Carrier: PR #18022's acceptance notes."
      ]
    }

    Generated by Claude Code

  3. claude commented on Sep 13, 2026

    @claude
    Contributor

    Clause-②-correction: 5652942907
    Clause-②: yes
    Session: session_01URLHobLUJB9K1ABV6ofdjj

    ⚠️ The claim's prediction was no. The delivered diff is yes, and the flip fired exactly on the condition the claim named — "it flips to yes if a diagnostic constant or predicate ends up re-exported from plugin-security/src/index.ts, or if a published payload gains a key." Both limbs fired, independently:

    (a) new exported symbols reachable from the published entry. src/index.ts now re-exports seven names from permission-set-name-collision.js, counted in the built dist/index.d.ts that package.json exports['.'].types names:

    symbol hits
    PERMISSION_SET_NAME_COLLISION 5
    permissionSetNameIsForeign 3
    permissionSetNameCollisionDiagnostic 3
    formatPermissionSetNameCollisionDiagnostic 2
    reportPermissionSetNameCollisions 3
    PermissionSetNameCollisionDiagnostic (type) 6
    CollisionReportSink (type) 3

    ⭐ Controlled in both directions: positive bootstrapDeclaredPermissions = 7 (so a hit means something), negative SHIPPED_PLATFORM_BUCKET_NAMES = 0 and defaultLookup = 0 — both module-private, so a zero discriminates.

    (b) a new key on an already-published payload: PermissionSeedOutcome.collisions?.

    ⇒ changeset graded minor (⛔ not patch), and needs:contract-review is hung on both carriers — card and PR, one stroke, read back on each.

    ⭐ And the export is the mechanism, ⛔ not an accident. The card's precedent (#14553) turns on one shared predicate so the two doors cannot drift. The compile-time door lives in another lane, so publishing the predicate, the builder and the formatter is precisely what lets that lane consume them instead of re-deriving the wording — which is the drift this card is about, one layer up. A no here would have meant a private predicate and a second copy of the sentence.

    ⛔ No second Claim:. The declaration above supersedes the claim's line; the claim comment was not edited.


    Generated by Claude Code

  4. claude commented on Sep 13, 2026

    @claude
    Contributor

    os-dev-report

    {
      "issue": 17516,
      "status": "done",
      "branch": "claude/issue-17516-permission-set-collision-diagnostic",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/18022",
      "premise_still_valid": true,
      "summary": "REWORK round on the existing open draft PR — no new PR, no new branch, no scope change, and the card's own delivery untouched. The seat's reading of the defect was verified in-seat and held exactly: the `find` double this PR adds treats a top-level combinator as a FIELD NAME, so `{ name: 'x', $or: [...] }` silently returns nothing. It now REFUSES what it does not implement, the remedy the gate's header prescribes for a double that only ever sees scalar equality. The unmeasured tail behind the CI stop was NOT empty: `check:objectql-double-limit` (step 157, the very next gate) graded the same double BLIND to the caller's bound — and that bound is load-bearing (`defaultLookup` reads `limit: organizationId ? 5 : 1`, #10103), so every change to it was green by construction. Both are this PR's own defects in the file it adds; both are fixed in one commit. Head 6a5958acd -> 498f6054345cba3f530f991d7d4781bb5d2d45c5, pushed as a FAST-FORWARD (no rebase, no amend, no force-push). PR left in draft, auto-merge not enabled, nothing merged, no label touched, `packages/spec/**` and `content/docs/releases/**` never touched, no baseline or allow-list grew, no gate weakened, skipped or re-ordered. Branch is ~13 commits behind origin/main (08a363aa2) and was NOT merged forward — `lint.yml` and root `package.json` are unchanged across that range, so the sequence run here IS the current sequence.",
      "tests": "REPRO FIRST, before any edit: `pnpm check:where-matcher` at 6a5958acd reproduced CI run 34756867494 byte-identical — '1 silently wrong', 'line 56 (anonymous): no combinator branch', 'Discovered 408 matcher(s): 407 conforming (269 by refusing)'. DEFECT MEASURED ON THE REAL BYTES, not read off the report: makeQl() was lifted out of the file by brace-matching the real source, transpiled and driven directly (harness at /tmp/claude-0/-home-user-objectstack/45ff97fc-890f-5208-90f4-7b75ea7a88cf/scratchpad/issue-17516/lift-makeql.mjs), handed `{ name: 'crm_sales_rep', $or: [{package_id:'com.example.a'},{package_id:'com.example.z'}] }` against a row satisfying the name AND the first arm — BEFORE (HEAD blob): RETURNED [] (the row a real driver returns, silently dropped); AFTER: THREW 'fake driver: unsupported combinator $or'. GATES AFTER: check:where-matcher exit 0 — 408 discovered, 408 conforming (270 refuse, +1 exactly), 'baseline key set verified against 32a3214: no files added'. check:objectql-double-limit exit 0 — 386 graded, 188 apply the bound or refuse (+1 exactly), 'none new', no files added. FULL GATE SEQUENCE: all 168 executable steps of the `Lint & Repo Gates` job (steps 5-172 of lint.yml's 174; 0-4 are checkout/node/pnpm/cache, 173 is the failure-only tail reporter) re-run IN DECLARED ORDER on the final tree 498f60543 — EVERY ONE EXIT 0, `pnpm lint` over the whole repo included (138s, exit 0). The 5 family-conditional steps were run UNCONDITIONALLY, a superset of CI (which selects 4 run / 1 skip=verify_lock here). Per-step logs: /tmp/claude-0/-home-user-objectstack/45ff97fc-890f-5208-90f4-7b75ea7a88cf/scratchpad/issue-17516/steplogs/. No gate left the tree dirty (`git status --porcelain` empty at 498f60543). SUITE at 498f60543 after building the closure (`pnpm --filter '@objectstack/plugin-security^...' build` exit 0): `pnpm --filter @objectstack/plugin-security test` = 112 files / 2157 tests / 0 failures — IDENTICAL to the numbers the PR body records at 6a5958acd, so the PR body's claim still holds; `typecheck` exit 0 across all three legs (tsc --noEmit, tsconfig.scripts.json, check:test-typecheck = 0 files / 0 errors / 0 pinned signatures); the new file's own suite 9 passed (9). ⇒ neither remedy changed a single test outcome: nothing in the suite hands the double a combinator, and the bound truncates nothing the assertions rest on — both are dormant guards that go red the day someone writes the query they model. NOT MEASURED: nothing — the tail is fully measured. The first locked attempt returned exit 99 (queue-timeout, NOT MEASURED, never acquired after 540s); it was re-taken under the same OS_VERIFY_LOCK_SLOT, which kept the parked place, and acquired in 11s.",
      "mcp_calls": "0 — no MCP GitHub call this round. Every read (card, PR, all comments on both) and every write (PR comment, this report) went through repo-scoped REST with the session token; the branch went through git.",
      "open_questions": [],
      "out_of_scope_findings": [
        "noted, not filed: `node scripts/pm/dispatch-gates.mjs --commands` DOES name both gates for this change set — `pnpm check:where-matcher` and `pnpm check:objectql-double-limit` are both in its derived list at this head. So both were derivable before the first push and were simply not run; neither appears in the earlier round's recorded gate list. No tool defect was found, so no card is warranted. Carrier: PR #18022 comment 5654102888.",
        "noted, not filed: the PR body's `## Checks` section is now incomplete rather than wrong — it is scoped 'All at 6a5958acd' and names neither of these two gates. It was deliberately NOT rewritten (a PR-body write carries measured footer-mutation risk and rewriting is not this round's job); the rework evidence lives in PR comment 5654102888 instead. Carrier: this seat, at landing."
      ]
    }

    Generated by Claude Code

  5. claude commented on Sep 13, 2026

    @claude
    Contributor

    pm:dispatched → pm:awaiting-maintainer

    ⚠️ Correcting this seat's own board defect. PR #18022 has been parked since ~16:3xZ on a question put to the maintainer in chat only — and 「交接即标签:只写交接评论而不同笔挂标 = 空交接,收件箱只认标签」. The board was claiming pm:dispatched, which asserts a dev is working this card. No dev is, and none should be: the work is delivered and the PR is green.

    The blocker is that clearing needs:contract-review requires a verdict whose Served-tier: is exactly CONTRACT_REVIEW_TIER (claude-fable-5-1), which this seat is neither served at nor permitted to commission (SKILL.md 〈模型分档〉; dispatch-gates.mjs --tier names the spec seat). See PR #18022's status comment and seat-post correction 175.

    Maintainer-action: is now on the body, line-anchored and dual-channel. ⚠️ This state has no machine exit: once the maintainer acts, the director seat verifies and strips the label with an evidence comment in the same stroke — ⛔ this seat does not self-clear it.

    ⛔ needs:contract-review stays hung on both carriers — it is a carrier, not a state, and it is genuinely pending.


    Generated by Claude Code

  6. claude commented on Sep 13, 2026

    @claude
    Contributor

    Maintainer has ruled — the Maintainer-action: on this card is answered

    Ruling, verbatim (2026-09-13T22:2xZ): 「你可以直接派fable复核」 — this seat may commission the CONTRACT_REVIEW_TIER clause-② review directly.

    Acted on immediately: a claude-fable-5-1 session (session_01NiQpkfNDNyAJTx3CAhkdhi) is commissioned to render the verdict for PR #18022 and post the record itself, because this seat cannot relay a Served-tier: line it did not read from a transcript.

    ⚠️ The mechanism constraint measured earlier is unchanged and was respected: an in-session subagent does not lift the tier — model: fable is a dispatch parameter, exactly what gate row C7 refuses, and get_session stamps the session rather than the subagent thread. Only a separate session stamps last_served_model: claude-fable-5-1.

    ⛔ This seat is not stripping pm:awaiting-maintainer itself — 「本态无机器出口:维护者完成后由总监席核验摘标 + 证据评论同笔」. This comment is that evidence, for the director seat to act on.

    Next, once the record lands: re-run check-clause2-carriers --pair 18022, strip the carrier from BOTH carriers citing the record, re-run --pair again (correction 174), un-draft, arm auto-merge.


    Generated by Claude Code

  7. claude commented on Sep 13, 2026

    @claude
    Contributor

    Landed — PR #18022 merged c54d8d67b at 23:19:51Z

    ⭐ Serial relay, first baton: discharged and PROVEN, ⛔ not assumed. main's scripts/engine-double-contract.pinned.json re-measured after the merge:

    reading value
    ledger count (sum of list lengths) 1492 — was 1491 before this landed
    this PR's entry (permission-set-name-collision.test.ts) 1 hit
    #18046's entry (admin-import-users-manager-pass.test.ts) — negative control 0 hits, correct

    ⇒ nothing was swallowed. PR #18046 now holds the second baton; its lap must land on 1493.

    ⚠️ pm:awaiting-maintainer stripped by THIS seat, and saying so explicitly rather than leaving it: the rule 「维护者完成后由总监席核验摘标」 puts that on the director seat, but the card is CLOSING and a pm:* state left on a closed card is stale residue the patrol would keep surfacing. The maintainer's action is complete — they ruled 「你可以直接派fable复核」 at 22:2xZ, the at-tier verdict 5656684087 was rendered and posted, the carrier cleared, and the PR merged. ⛔ If the director seat disagrees with this seat stripping it, the record is here to reverse.


    Generated by Claude Code

  8. added 3 commits that reference this issue on Sep 17, 2026
    c54d8d6
    b5cbfef
    8fa6b97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions