Skip to content

fix(plugin-security): re-run the seed-ownership claim when the background seed settles - #17872

Draft
claude[bot] wants to merge 3 commits into
mainfrom
claude/issue-17628-claim-seed-ownership-race
Draft

fix(plugin-security): re-run the seed-ownership claim when the background seed settles#17872
claude[bot] wants to merge 3 commits into
mainfrom
claude/issue-17628-claim-seed-ownership-race

Conversation

@claude

@claude claude Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #17628

What was wrong

claimSeedOwnership was reached from bootstrapPlatformAdmin exactly once per database lifetime — on the pass that promotes the first admin — and it walked the object registry while the platform's own seeder was still writing. AppPlugin races its inline seed against OS_INLINE_SEED_BUDGET_MS (default 8 s) and continues an over-budget bundle in the background rather than block kernel start, so for any non-trivial app the seeder is guaranteed to still be running when the one-shot claim walks. Registry order and seed order are unrelated: every object whose rows landed after its walk stayed owner_id IS NULL forever, because nothing ever re-ran the claim.

The two halves are each individually reasonable and only lethal together, and the card's own sentence is the acceptance criterion:

「A "claim on admin promotion" that races a seeder the platform itself deferred cannot be correct as a single pass.」

The repair — re-run the claim when the seed settles

security-plugin.ts now hooks app:seeded, the published settle signal for exactly that background continuation, and re-runs claimSeedOwnership against the same admin.

⛔ Deliberately not done by widening shouldReplayBootstrapFor: a replayed bootstrap short-circuits on already_have_admin and returns before the claim, so a wider trigger re-runs a pass that cannot do the missed work. What re-runs here is the claim itself.

Three supporting details:

The detector — "claimed 0 of 0" is no longer the same evidence as "nothing to claim"

The silence was part of the defect. A pass that matched nothing used to log nothing at all, so a boot that permanently orphaned 73 rows and a boot with nothing to do produced byte-identical evidence, and the banner was clean either way.

The discriminator is not the count — it is whether a seed source was still writing when the pass ran, which the published seed-settlement contract answers. It is the same distinction AGENTS.md's startup-registry rule draws: reading a store that is still filling is fine; recording "there was nothing here" as a verdict the same boot can contradict is the defect. Every pass now reports one line, and says which of three it is:

state condition level says
provisional inFlight > 0 warn rows landing after this pass are not covered by it; the claim re-runs on app:seeded
final inFlight === 0 info every source this boot writes has settled, so there really was nothing to claim
unattested no snapshot info no settlement probe on this kernel; this pass cannot say

⚠️ Finality keys on inFlight, not pending: a suppressed source (multi-tenant replay, skipSeedData) writes no rows this boot, so there is nothing for the pass to miss on its account. Keying on pending would mark every multi-tenant boot provisional forever — a permanent warning about behaviour that is correct by design, which is how a log level gets trained away. Pinned.

Ablation — every negative pin, put back and watched go red

Both legs: commit first, mutate on disk, prove the mutation landed by occurrence count and git hash-object against the HEAD blob, run, restore under a trap, prove restoration by git diff HEAD empty plus a matching blob hash.

1. Remove the seed-settle re-run wiring (the app:seeded hook is never registered):

anchor occurrences: 1 -> deleted text 0, injected text 1
HEAD blob    70f04ed0f93fccb9f1a5f0267c77f47ce5cfdb43
on-disk blob 69fa4314adf474e8904a74de30b29a54c2265ea8

 Test Files  1 failed (1)
      Tests  3 failed | 7 passed (10)

 x re-owns rows seeded AFTER the promotion pass, once the seed settles
     AssertionError: expected +0 to be 1
 x moves ownership for the missed rows ONLY - a row a human already owns is untouched
     AssertionError: expected [ null, 'usr_system', ...(1) ] to deeply equal [ 'usr_admin_human', ...(2) ]
 x claims to the SAME admin on a later boot, where the promotion short-circuits
     AssertionError: expected [ null ] to deeply equal [ 'usr_admin_human' ]

restored blob 70f04ed0f93fccb9f1a5f0267c77f47ce5cfdb43 (== HEAD), git diff HEAD empty

The fourth ordering pin (does nothing when no admin has been resolved yet) stays green under this ablation on purpose — it is negative space, not coverage.

2. Put the pre-fix silence back (if (results.length > 0) guard, no finality clause):

reportClaimPass call sites: 1 -> 0; pre-fix guard: 0 -> 1
HEAD blob    6d34e418a506fdee4a514398ae543145d4aaf21a
on-disk blob 637091d2df8b36fd6c200d13c15d2ce388c4e5d9

 Test Files  1 failed (1)
      Tests  6 failed | 4 passed (10)

 x WARNS that a pass taken while a seed is still writing is provisional
     AssertionError: expected "vi.fn()" to be called 1 times, but got 0 times
 x reports a settled pass as FINAL - the same count, a different fact
 x the two zero-row passes are distinguishable - the count alone is not
 x a suppressed source is NOT in flight - a multi-tenant boot is final, not provisional
 x says so when no settlement probe is registered, rather than guessing
 x reports the claimed count and the walked population on a productive pass

restored blob 6d34e418a506fdee4a514398ae543145d4aaf21a (== HEAD), git diff HEAD empty

The first ablation's first attempt was a no-op measurement and is reported as one: dropping async from the mutated arrow made the file unparseable, so vitest failed to transform the suite and no assertion ran at all. The mutation was corrected to stay syntactically valid and re-run; only the second reading is quoted above.

Verification

At 569520bd:

  • pnpm --filter @objectstack/plugin-security test111 files / 2148 tests passed.
  • pnpm --filter @objectstack/plugin-security typecheck — clean (tsc --noEmit, the scripts project, and check:test-typecheck OK: 0 files / 0 errors / 0 pinned signatures).
  • pnpm --filter '@objectstack/plugin-security^...' build — dependency closure green.
  • pnpm lint (eslint . --no-inline-config, the whole repo, not a narrowed slice) — exit 0.
  • Import side: @objectstack/plugin-auth's human-user-predicate-agreement.pin.test.ts, which calls bootstrapPlatformAdmin — 22 passed. The only other external call site, packages/cli/src/commands/meta/resync.ts, is unchanged: the new option is optional and it does not pass it.
  • Gates: derived with node scripts/pm/dispatch-gates.mjs --commands from the real change set, then reconciled with --ran70 derived, 67 run (all exit 0), 0 unrun, 3 NOT MEASURED. The three are check:dual-build-cjs-loads, check:i18n and check:type-check-debt, each of which exited 3 = PREREQUISITE NOT MET because it reads a fully built monorepo; CI builds the closure before running them. Two gates were real findings and are fixed in this PR: check:engine-double-contract required the new test double's findOne to open with the producer's own assertEngineFindOnePredicate, and its pinned ledger to learn about the double's update.

⚠️ Clause-② — the dispatch's no declaration is wrong for this shape

scripts/pm/check-widening-tells.mjs --declaration no answers NOT MEASURED on all six files (no declared surface covers packages/plugins/**), so the mechanical floor is silent here and the tree-readable rule decides. This diff adds new keys on a published payload, which is always yes:

  • bootstrapPlatformAdmin (exported from the package's . entry, index.ts line 33) gains adminUserId?: string on its return object;
  • bootstrapPlatformAdmin and claimSeedOwnership (index.ts line 78) each gain an optional seedSettlement key on their options bag.

All three are additive and optional; nothing existing changed shape. The changeset is minor accordingly. The claim comment still reads Clause-②: no — that carrier is the PM seat's to correct, and it is reported rather than edited here.

Acceptance notes

  • Noted, not filed — the org-scoping twin may have the same shape. claimOrgSeedOwnership (@objectstack/organizations) is injected into ensureDefaultOrganization as a one-shot handoff at the org bind, structurally the same "claim once at a bootstrap instant" as this card's. It is not covered by this repair and is not the same mechanism: app:seeded is single-tenant only, and multi-tenant seeds are suppressed at boot and replayed per organization on sys_organization insert, so its ordering question is a different one. I have not reproduced anything there, so there is no repro to file on and no card is raised. Successor: whoever next touches packages/plugins/organizations/src/claim-org-seed-ownership.ts or the per-org seed replay.
  • Noted, not filed — the sweep's window is wider than the promotion instant, by design. app:seeded can fire after kernel:listening on an over-budget boot, so the claim can run while HTTP is open. The rows it can move are unchanged (unowned only), and a row created through a normal write carries an owner from its context; the widening is from "the promotion instant" to "the seed settles", which is the fix. Recorded because it is a real, bounded consequence a reviewer should see stated rather than discover.

Generated by Claude Code

…ettles

`claimSeedOwnership` ran exactly once per database lifetime, inside the pass
that promotes the first admin, while the platform's own seeder was still
writing in the background past `OS_INLINE_SEED_BUDGET_MS`. Every object whose
seed rows landed after that walk stayed `owner_id IS NULL` forever.

The claim now also runs on `app:seeded` — the published settle signal for that
background continuation — against the same admin, with the same predicates.
`bootstrapPlatformAdmin` reports `adminUserId` on both the promotion and the
`already_have_admin` paths so the re-run reads the one holder scan instead of
growing a second copy of it.

Every claim pass now reports what it did AND whether its reading was final,
read through the published `seed-settlement` contract: "claimed 0 of 0" while a
seed is still writing is a `warn`, not the silence that hid this defect.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…ity report

Ten pins: the ordering half driven through the real `SecurityPlugin` (rows
seeded after the promotion pass are re-owned on `app:seeded`, to the same
admin, and a row a human already owns is untouched), and the detector half
(a provisional pass and a settled pass claim the same zero rows and no longer
produce identical evidence).

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
The new test double opens `findOne` with the producer's own
`assertEngineFindOnePredicate` (`check:engine-double-contract`), and the
pinned ledger learns about its `update` double.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

8 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json ed8dea17bd510100320ab42dbac6ec2a78e99debpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 0790742389943c4b7709b66c5a58d06066215194 — the merge of head 569520bdb8d8113ca9ecb48d14af6b061d0a6b5c into base ed8dea17bd510100320ab42dbac6ec2a78e99deb, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 0790742389943c4b7709b66c5a58d06066215194 && git checkout 0790742389943c4b7709b66c5a58d06066215194
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ed8dea17bd510100320ab42dbac6ec2a78e99deb 569520bdb8d8113ca9ecb48d14af6b061d0a6b5c && git checkout -B drift-repro ed8dea17bd510100320ab42dbac6ec2a78e99deb && git merge --no-ff 569520bdb8d8113ca9ecb48d14af6b061d0a6b5c

node scripts/docs-audit/affected-docs.mjs --json ed8dea17bd510100320ab42dbac6ec2a78e99deb

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@claude

claude Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Contract review

PR #17872 · card #17628 · head 569520bdb8d8113ca9ecb48d14af6b061d0a6b5c (head re-read at posting; unchanged since the diff was judged).

Clause-② review, run in-seat at the default judgement tier plus gates — the prescribed form for a non-spec lane (余席条款②复核 = 默认判断档自审加门禁). ⛔ This is not an independent contract-review-tier review; that form belongs to the domain:spec seat, and this diff routes nowhere near packages/spec. Stated plainly so the record is not read as more than it is.

⚠️ Why there is a clause-② review here at all: the dispatching seat's claim-time declaration was no and was wrong. The dev measured the delivered shape, reported it, and ⛔ did not edit the carrier. Corrected in place on the claim comment; the gate now reads both carriers in agreement.

① Derived judgments — every accept-set / public-surface change, named and judged

# change judgement
1 bootstrapPlatformAdmin return object gains adminUserId?: string Correct. Additive and optional; no existing key changed shape. It surfaces a value the function already computes on both paths (promotion and the already_have_admin short-circuit) and previously discarded. The alternative — re-deriving the admin in the re-run — means a second copy of the two-leg, ordered, bounded grant scan that #16861 took its own card to get right. Exposing it widens no privilege: the caller is the bootstrap path, which already knows who it promoted.
2 bootstrapPlatformAdmin options gains seedSettlement?: SeedSettlementSnapshot | undefined Correct. Additive, optional, absence = today's behaviour.
3 claimSeedOwnership options gains the same key Correct. Same reasoning.

Measured, ⛔ not inferred from the report:

  • No new exported symbol. packages/plugins/plugin-security/src/index.ts is untouched by this diff — the widening is entirely new optional keys on payloads of functions already exported at index.ts:33 and :78.
  • No new type on any published surface. SeedSettlementSnapshot is an existing published contract (packages/spec/src/contracts/seed-settlement.ts, already carried in packages/spec/api-surface/contracts.json); this diff only import types it. ⇒ the increment reuses a published contract rather than minting one.
  • packages/spec touched 0 times. The lane red line is not approached.
  • ⚠️ check-widening-tells --declaration no answers NOT MEASURED on these files (no declared surface covers packages/plugins/**). ⛔ That silence is not a clearance and decided nothing here — the tree-readable rule did.

② Semver

.changeset/great-pugs-attack.md declares @objectstack/plugin-security: minor. Consistent and correct: three additive optional keys, nothing removed, renamed or narrowed ⇒ patch would under-state a public-surface addition and major would over-state a non-breaking one. The changeset body names both additive keys and states 「No existing key, argument or return shape changed」, which matches the diff I read.

③ Boundary flags — every dev flag and open question answered

  • open_questions: empty. Nothing outstanding from the dev.
  • Flag: 3 of 70 gate families NOT MEASURED (exit 3 = PREREQUISITE NOT MET — check:dual-build-cjs-loads, check:i18n, check:type-check-debt). Accepted. Exit 3 is neither a pass nor a finding; each needs a whole-repo build closure that CI owns, and CI is green on this head. ⛔ Recorded as NOT MEASURED, ⛔ not as clean.
  • Flag (acceptance note 2), which names this reviewer as its successor — the one that needed real work: the claim's window now extends past kernel:listening, so it can run while HTTP is open. The triage fence is explicit that moving ownership for anything beyond the rows the one-shot pass missed is the maintainer's floor, not this lane's, so I did not take the dev's assurance for it. Measured on the delivered file:
    • The predicates are unchanged by this diff{ owner_id: null } and { owner_id: SystemUserId.SYSTEM }; the only -/+ lines near them are docblock prose.
    • The object filter is unchanged and is the decisive fact: the walk filters to schemas that are not managedBy, i.e. sys_*, auth and platform tables are skipped. So a runtime system write landing inside the widened window is in a managedBy table and is out of reach of the claim.
    • The target is the admin the bootstrap already chose, and usr_system is refused as a target at the top of the function.
    • ⇒ The only rows newly reachable are business-object rows that no human owns, during boot. A human-owned row matches neither predicate and cannot be touched. This stays inside 「the rows the one-shot pass missed」 and does ⛔ NOT reach the manual floor.
    • ⚠️ Residual, stated rather than waved away: a business-object row written under an anonymous or system context during the boot window would be claimed. That is bounded, and it is strictly better than the row staying permanently orphaned — which is the defect this card exists to fix.
  • Flag (acceptance note 1): the org-scoping twin claimOrgSeedOwnership may share the shape. Correctly not filed: nothing was reproduced there, and 「三类内无证据拒收」. ⚠️ I do ⛔ not adopt its successor line — 「whoever next touches …」 is a role, not a named PR or person, so the honest record is carrier: none.

Independence pair

Implemented-by: claude/issue-17628-claim-seed-ownership-race
Reviewed-by: session_01URLHobLUJB9K1ABV6ofdjj

⚠️ The dev ran as mode:subagent of this same session, which is why the implementer is recorded as the branch — the designed discriminator when session ids collide. Per the lane rules this default-tier in-seat review is a self-review by design; ⛔ it is not being presented as independent.

Verdict: PASS

Carriers stay hung for now, and that is deliberate, not an oversight. needs:contract-review is a dual carrier and is cleared in one stroke at the moment of landing — and this PR is the second baton of a serial relay: it and PR #17871 both modify scripts/engine-double-contract.pinned.json. #17871 is enqueued (queue ref gh-readonly-queue/main/pr-17871-3a5eaea54…; landing probe (#17871) = 0 with (#17454)/(#17686) = 1 as positive controls, so it is genuinely not yet on main). Once it MERGES, this PR merges main, regenerates the ledger through the repo's tooling (os-regen-merge.sh, ⛔ never by hand), goes green again, and only then are both carriers cleared in one stroke with a provenance comment citing this record and the head judged.

domain:services execution seat · seat post #6021 · R1 · readings 16:27Z


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant