suggested-binding-loop (a): the isDefault audience-binding suggestion is never surfaced on stock (auto-bound at boot, then skipped)

[huangyiirene]

Symptom

GET /api/v1/security/suggested-bindings → 200 {suggestions:[], synced:{created:0}} and the table is empty on stock, though the permission set and its everyone binding both exist.

Discriminating step: delete that binding row and re-list → synced {created:1} and a PENDING row appears — so the declaration is collected; it is just never surfaced on stock.

Root cause

Located by the run. packages/plugins/plugin-security/src/suggested-audience-bindings.ts syncAudienceBindingSuggestions does if (bound) continue when no row exists yet (~L224), and the "confirmed (observed)" transition only fires for a row already in status pending. But the security plugin auto-binds the app's isDefault set to everyone at boot, before any list call, so stock always takes the continue. This contradicts the module's own docblock. Confirmed present on origin/main (if (bound) continue still at suggested-audience-bindings.ts ~L224).

Reproduction

1. Stock boot with an isDefault permission set that auto-binds to everyone.
2. GET /api/v1/security/suggested-bindings → empty, synced.created:0.
3. Delete the everyone binding row and re-list → a PENDING suggestion appears, synced.created:1.

Suggested fix

Surface a PENDING suggestion for a declared isDefault binding even when the boot auto-bind has already satisfied it (or record it before the auto-bind), so the suggestion table reflects the declaration on stock rather than only after an unbind.

Source

Extracted from the QA run #7637 (framework 92f26f7, console 09987b680).

[os-help]

Claim: PM loop (domain:identity seat, #6022)
Session: session_01BVc1ekPpi6yaWywAUhfzfd
Branch: claude/issue-7677-suggested-binding-surfacing
Worktree: objectstack-issue-7677
Mode & model: M card, mode:subagent (Claude_Code_Remote absent this shift, sanctioned fallback), model: opus.

Scope: packages/plugins/plugin-security/src/suggested-audience-bindings.ts (~L224 if (bound) continue) + tests — a DIFFERENT file from the security-plugin.ts hot-file serial queue; no region conflict with #7505/#7626/#7665. One design point is left to the dev's judgment against the module's own docblock contract, with the reasoning required in the report: whether a declaration already satisfied by the boot auto-bind should surface as PENDING or as confirmed/observed — the card suggests PENDING-or-record-before-autobind, but a row that IS bound arguably belongs in the confirmed state the module already models. Acceptance either way: on stock, the list is non-empty and reflects the isDefault declaration.

---

Generated by Claude Code

[os-help]

{
  "issue": 7677,
  "status": "done",
  "premise_still_valid": true,
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7704",
  "branch": "claude/issue-7677-suggested-binding-surfacing",
  "commits": [
    "70d4fef fix(plugin-security): surface the isDefault audience-binding suggestion on stock",
    "df3fee8 Merge remote-tracking branch 'origin/main' into claude/issue-7677-suggested-binding-surfacing"
  ],
  "files_changed": [
    "packages/plugins/plugin-security/src/suggested-audience-bindings.ts",
    "packages/plugins/plugin-security/src/suggested-audience-bindings.test.ts",
    ".changeset/olive-donkeys-shake.md"
  ],
  "summary": "Premise confirmed on origin/main, including the ordering that causes it: security-plugin.ts binds the composed baseline set to the everyone anchor at boot and its own comment requires that to run BEFORE syncAudienceBindingSuggestions, so 'already bound' is the normal stock case and the reconciler's `if (bound) continue` (L224) meant no row was ever written; the confirmed-observed transition only fired for a row already in pending, which on stock never existed. An already-satisfied declaration is now recorded rather than skipped, as `confirmed` with resolved_at set and resolved_by empty - the same end state the existing pending-to-confirmed branch reaches, without passing through pending. The module docblock was corrected to state the reconciliation it now performs. security-plugin.ts was NOT touched: the fix stayed entirely inside the reconciler, so no boot reordering was needed.",
  "status_choice_rationale": "Chose `confirmed` (observed), not `pending`. No new status value introduced. Consumer audit of the status field, all five pointing the same way: (1) the module's own docblock already specifies 'binding already present -> confirmed (observed)', so confirmed implements the stated contract and pending would contradict the very docblock the card cites; (2) the backing object schema names this exact case in the resolved_by field description - 'Empty on a confirmed row means the binding was observed (e.g. bound at boot or by hand), not confirmed through the prompt' - i.e. confirmed-with-empty-resolver IS the schema's word for a boot-observed binding; (3) the console consumer objectui packages/app-shell/src/components/SuggestedBindingsPanel.tsx lists exactly status=pending and renders nothing when that set is empty, so pending is the actionable-prompt state and a pending row here would prompt an admin to 'accept' a binding that already exists; (4) confirmAudienceBindingSuggestion and dismissAudienceBindingSuggestion both 409 on a non-pending row, so a pending-but-already-bound row would allow a meaningless confirm (no-op write) and a misleading dismiss (records the admin's 'no' while the binding stays in force); (5) the boot comment in security-plugin.ts requires a satisfied baseline to 'never nag', which confirmed honours and pending breaks. Also checked: spec contract packages/spec/src/contracts/security-service.ts (status union pending|confirmed|dismissed, row type deliberately open), the SDK client surface packages/client/src/index.ts security.suggestedBindings, and the REST/runtime routes - none branch on status beyond passing the filter through. Counter choice: the row is counted in `created` only, not in `confirmedObserved`, so each row change is counted exactly once and `synced.created` reflects the creation as the card specifies; confirmedObserved stays what its contract doc says it is, the pending-to-confirmed transition count. DELIBERATE CONSEQUENCE, recorded rather than hidden: on a post-fix stock system the row is created confirmed, so a LATER unbind does not re-open it as pending. That is consistent with how the module treats resolved rows everywhere else (confirmed and dismissed are terminal audit history, never re-opened) and re-opening would nag an admin who had just deliberately unbound the set. The card's discriminating step is preserved in its meaningful form - an unbound declaration with no row still surfaces as PENDING.",
  "tests": "pnpm --filter '@objectstack/plugin-security^...' build (build closure first, toolchain trap 2) then pnpm --workspace-concurrency=2 --filter '@objectstack/plugin-security' test -- --maxWorkers=2 => 'Test Files 46 passed (46) / Tests 955 passed (955)'. typecheck => 'tsc --noEmit' clean, no output. Post-merge with origin/main both re-run: 46/46 files, 955/955 tests, typecheck clean. HTTP surface: packages/runtime carries the /security/suggested-bindings routes (http-dispatcher.ts, domains/security.ts); targeted run of src/http-dispatcher.test.ts + src/domain-handler-registry.test.ts => 'Test Files 2 passed (2) / Tests 288 passed (288)', and the FULL runtime suite => 'Test Files 127 passed (127) / Tests 2018 passed (2018)'. packages/client src/admin-surfaces.test.ts (client.security.suggestedBindings) => 'Test Files 1 passed (1) / Tests 7 passed (7)'. node scripts/check-nul-bytes.mjs => OK (7100 files, no raw control bytes); targeted grep -naP self-scan over the three changed files => no hits. NOTE on a false red worth recording: an initial packages/runtime run showed 78 of 127 files failing before I had built runtime's dependency closure - the AGENTS.md stale/missing-artefact trap, not my change; after pnpm --filter '@objectstack/runtime^...' build all 127 files pass. Also note the vitest file filter does not pass through `pnpm test -- <file>` here (the script already ends in `vitest run --`, so the paths were swallowed and the whole suite ran); targeted runs were done with npx vitest inside the package dir.",
  "ablation": "Reverse-verified by ablation with the direction predicted in advance (new stock-surfacing tests red, existing pins green - the ordinary direction, no inversion expected since the new tests assert a row that only the fix creates). Reverted ONLY packages/plugins/plugin-security/src/suggested-audience-bindings.ts to origin/main via `git checkout origin/main -- <path>` (never git stash - shared refs/stash), keeping the new tests, and confirmed `if (bound) continue` was back at L224. Result matched exactly: 'Test Files 1 failed | 45 passed (46) / Tests 3 failed | 952 passed (955)'. The three reds are precisely the new stock-surfacing tests - 'records an already-bound set as confirmed/observed (the boot baseline) rather than skipping it', 'is idempotent for an already-bound set', 'surfaces the stock isDefault declaration through the list endpoint (#7677)'. All 18 pre-existing pins stayed GREEN, including pending creation, the pending-to-confirmed (observed) transition, pruning on uninstall, dismissed-as-history, the full confirm flow with caller context, and every tenant-admin denial. Restored with `git checkout <branch> -- <path>` and re-verified green.",
  "neighbors": "No conflict. Final diff file list vs origin/main is exactly three files - packages/plugins/plugin-security/src/suggested-audience-bindings.ts, its .test.ts, and .changeset/olive-donkeys-shake.md (+127/-8). Mechanically confirmed that security-plugin.ts is absent from the diff (git diff --name-only origin/main...HEAD | grep -c security-plugin.ts => 0), so there is no overlap with #7505/PR#7697 or #7665. The fix did NOT require touching security-plugin.ts: the boot auto-bind ordering could stay exactly as it is because recording the observed state inside the reconciler is sufficient, so the 'prefer the shape that stays in your file' instruction was satisfiable with no escalation. security-plugin.ts's boot comment ('never generates a redundant pending suggestion') remains accurate under this change, since no pending row is generated. Changeset is a separate new file, no conflict with theirs. git fetch origin main && git merge origin/main ran clean (no conflicts) before the final push, and the suites were re-run after it.",
  "escalations": [],
  "deviations": "One pre-existing test was REPLACED rather than added to, and it is the crux of the card so flagging it explicitly: 'skips a set that is already bound to the anchor (e.g. the boot baseline)' (suggested-audience-bindings.test.ts L104-111) asserted created:0 and zero rows - it pinned the defect itself. It is replaced by a test asserting the recorded confirmed/observed row. Per the local-verification-scope rule I ran the affected packages' own suites plus the implicated surfaces, not the full 55+ gate farm; check:nul-bytes was run as it applies to any edit. No skip-changeset label: this PR ships a changeset (user-visible fix), so the label would be wrong. PR left as DRAFT with auto-merge NOT enabled, as instructed.",
  "changeset": ".changeset/olive-donkeys-shake.md - patch bump on @objectstack/plugin-security, English, describing the empty-on-stock symptom, the boot-ordering cause, the confirmed-observed choice with its reasoning, and the unchanged existing flow."
}

---

Generated by Claude Code

---

Generated by Claude Code

[os-help]

Review: ACCEPT — PR #7704 (PM session session_01BVc1ekPpi6yaWywAUhfzfd; diff reviewed line-by-line).

• Core change is minimal and exact: the if (bound) continue skip becomes a record-in-actual-state insert — confirmed, resolved_at stamped, resolved_by empty, which the object schema itself defines as an observed (not prompt-confirmed) binding. No new status value; docblock corrected to state the reconciliation it now performs.
• The status choice (confirmed, not the card's literal pending) is upheld: the dev's five-consumer audit (module docblock, schema resolved_by description, console panel's status=pending prompt semantics, the 409 guards on confirm/dismiss, the boot never-nag comment) all point one way, and the card's acceptance criterion ("list non-empty, truthfully reflects the declaration") is met more truthfully by confirmed.
• Counter semantics kept honest: the row counts once in created, confirmedObserved stays the pending→confirmed transition count per its contract doc.
• One behavior nuance surfaced for the maintainer (open veto window): post-fix, the stock row is born confirmed, so a LATER manual unbind does not re-open it as pending — consistent with resolved rows being terminal audit history everywhere else in this module, and re-opening would nag the admin who just unbound deliberately. The discriminating step survives in its meaningful form (a declaration with NO row still surfaces as pending, and pending→confirmed still fires). If the product intent is "unbind re-opens the prompt", say so here and it becomes a small follow-up card rather than a rework.
• Replaced test flagged by the dev is legitimate: the old case (created: 0, zero rows) pinned the defect itself.
• Ablation matched the pre-declared direction exactly: 3 new stock-surfacing tests flip red on revert, all 18 existing pins green. Suites: plugin-security 955, runtime full 2018, client admin-surfaces 7 — green on top of merged origin/main (base 21888ab, post-fix(plugin-security): propagate engine faults from readRowById instead of flattening to null #7697).
• Scope: 3 files, security-plugin.ts mechanically confirmed absent from the diff (no overlap with [security] A by-id write is not gated by record visibility — a contributor mutates records they cannot read, when only select-scope RLS is authored #7665/[security] $expand bypasses the CRUD gate and OWD row scope — a contributor reads records they are 403'd from, because the #2850 expand waiver keys on the wrong axis #7626). Docs-drift advisory: package-level fan-out; the change makes the suggested-bindings surface MORE consistent with documented behavior, no doc edits needed.

Next: flipping #7704 ready and enabling auto-merge; the merge queue's full suite is the last gate.

---

Generated by Claude Code