fix(api): seeded demo and simulation records name a real person by mforce · Pull Request #517 · mforce/cluckwork

mforce · 2026-08-12T08:21:55Z

Closes #500.

Seeded audit events carried ActorEmail = "(unresolved)". Both seeders resolved
TenantContext but never ICurrentUser, so every record they wrote fell into
AuditWriter's placeholder fallback — invisible until #494 rendered provenance,
at which point roughly 256 rows across five screens read "Created by
(unresolved)".

What changed

The placeholder is gone, and cannot come back. AuditWriter now throws on an
unresolved actor, symmetric with the tenant guard already beside it. Every
non-HTTP caller declares who it is:

Caller	Actor
`bootstrap-admin`	`(bootstrap-admin)` system actor
`recover-admin`	`(break-glass)` system actor
`seed --profile demo`	the account's Owner
`seed --profile simulation`	the persona who would really have done it

Demo now requires an Owner and exits PrerequisitesMissing naming
bootstrap-admin when there is none. That is a deliberate break with the old
"demo needs nothing but a connection string" contract, re-confirmed by the
owner with the cost on the table: a demo fixture exists to be looked at, looking
requires a login, and a login requires an Owner.

The simulation fixture is attributed per persona. Managers place the flocks
and create the products, categories and expenses; sales staff book the orders; a
rotating worker pool records the daily entries; the Owner does account
administration. About one submission in three is a manager signing off a worker's
draft, so the fixture now carries both #494 provenance shapes — the
"created by X, last changed by Y" case had no fixture at all before this.

The thing that made this harder than it looks

ICurrentUser is an authorization input, not an audit label. FlockScopeGuard
reads Roles and UserId; RecordDailyEntry, SubmitDailyEntry,
RecordFeedUsage and RecordWaterUsage all call it. Two consequences:

The seeders' unresolved actor was a live authorization bypass, which this
removes. The guard's if (!user.IsResolved) return Result.Success(); branch
named "startup/demo seeders" as its justification, and the seeders were in fact
its only callers. It is now unreachable for the two handlers that audit — but
not for the two that do not; see Review below, because getting that wrong
is the most interesting thing in this PR.
Which worker the seeder acts as decides what it is allowed to write. One
worker is restricted to one flock; picking it for another returns
FlockScope.NotAssigned and fails the entire seed. WorkerFor carries the
per-flock eligibility rule that prevents it.

The plan asserted the opposite through three revisions before anyone walked the
consumers. Four source comments still carrying that false model are corrected
here.

Proof

Full suite green — 1654 tests. Against real throwaway databases through the
real CLI, at the production 90-day history default:

demo → 509 audit rows, 0 placeholders: 508 signed by the Owner, 1 by
(bootstrap-admin).
simulation → 442 rows, 0 placeholders, and every persona signs the work it
would really have done:

actor rows

sim-worker-2, sim-worker-3 133 each

sim-manager-1 85

sim-sales-1 49

sim-worker-1 (flock-restricted) 30

Owner 11

(bootstrap-admin) 1
The restricted worker authored on 1 flock; the other two on 2 each —
the eligibility rule, visible in the data.
Both provenance shapes are present: 116 entries submitted by the worker who
recorded them, 60 by a manager.

Against a never-bootstrapped database, demo exits 1 naming the command to run.

Every mutation in the design's table was run before its claim was written —
apply, rebuild, full suite, record every red test, restore, rebuild, re-confirm
green. The recorded run is in docs/plans/500-seeded-audit-actor/05b-mutation-run.md.
That discipline earned its keep twice:

The partial-rerun guard survived the mutation it was written to catch. At
the draft window's day offsets both the correct code and the buggy one pick a
non-restricted worker, just a different one. It now asserts which one.
A mutation written as if (false) failed the build under warnings-as-errors,
so the test run silently used the previous mutant's binary — a false result
that only a build-output check caught.

Review

Two rounds, four reviewers each — codex, pi and two local agents — at the
mid-point and again on the finished diff. Ledgers:
05a-review-slices-1-4.md and 05c-review-final.md.

The final round is the one worth reading. It found a false claim in
FlockScopeGuard's own comment — that the unresolved-actor branch is now
unreachable "because every handler behind this guard also audits". Two of the four
do not: RecordFeedUsage and RecordWaterUsage write no audit row, so an
unresolved caller reaching either is granted account-wide access silently.

This plan's round 1 had already established that, in its audited-action walk.
Round 3 asserted the opposite to justify "unreachable", nothing re-checked it, and
the claim shipped into a comment in an authorization path — which is the exact
failure mode this issue is about. Nothing exploits it today (both seeders declare
an actor before every feed/water call, and every route behind the guard is
authenticated), so it is documented honestly rather than silently narrowed:
closing it flips an authorization default from open to closed, which deserves its
own issue.

Three more fixed, each a claim contradicted by its own code: ResolveSystemActor
said it grants no authorization privilege (a system actor holds no assignment
rows, so the guard treats it as account-wide — more reach than a restricted
worker); DemoDataSeeder fabricated [Roles.Owner] from a literal while its
sibling's comment forbade exactly that; and a cast persona demoted outside the
seeder was accepted into the wrong pool, failing much later on a count mismatch
that named nothing.

One mid-point finding is worth surfacing for anyone touching this area: nesting
Pick inside Pick evaluated the inner fallback eagerly, so at
Simulation:Managers = 0 the seeder logged "provenance is degraded" once per
flock-day while a worker had in fact been selected. A warning that fires when the
thing it warns about did not happen is worse than none.

Three reviewer claims were refuted against the code rather than accepted, each
recorded in the ledger with the evidence.

Not done, deliberately

Existing databases are not repaired. A completed fixture is certified
AlreadySeeded and nothing is rewritten, so the fix applies to a fresh or reset
database only — consistent with the issue, which puts backfilling out of scope.
tools/simulation/reset.sh is the supported way to get a repaired fixture.

Docs updated in the same PR: specs/product/GLOSSARY.md, AGENTS.md,
deploy/README.md, decisions 280 and 283, the break-glass runbook, and the
simulation harness README. The SPA Help page is deliberately unchanged — a real
farm never had placeholder rows and never could, so nothing changes for the
audience that page serves.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36a6207ac7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-12T08:26:31Z

+        (await users.GetUsersInRoleAsync(Roles.Owner))
+            .Where(u => u.AccountId == accountId)
+            .OrderBy(u => u.Id)


Exclude disabled owners from the seed actor

When an Owner has been disabled, DisableUserAsync retains their Owner role and only stamps DisabledAt, so GetUsersInRoleAsync still returns them here. Ordering solely by ID can therefore make either seeder resolve a disabled principal—even when a newer active Owner exists—and permanently attribute every seeded audit row to an account that login rejects; with only a disabled Owner, seeding also reports success although nobody can view the fixture. Filter out users with DisabledAt != null before making the deterministic selection in both seeders.

AGENTS.md reference: AGENTS.md:L54-L54

Useful? React with 👍 / 👎.

Confirmed and fixed in acb77b16.

You're right, and the codebase says so in its own words — IdentityProvider:
"a disabled actor retains its Owner ROLE ROW — only authentication is blocked",
while login rejects DisabledAt != null. Both seeders now exclude disabled
Owners before the deterministic pick.

A local review of that fix caught a second defect in it, worth recording here.
My first patch reworded the message to mention disabled Owners but still told the
operator to run bootstrap-admin. That does not work for this cause:
FirstRunAdminService counts Owner role rows without checking DisabledAt, so it
reports "already provisioned", exits 0 having done nothing, and lands them back on
the same error — an instruction loop with no exit. AdminRecoveryService already
documents that trap; this was the second place it bites. Both seeders now split the
message by cause and name a remedy that actually works.

Verification:

Mutation — removing && u.DisabledAt is null turns both new demo guards red;
restored, rebuilt, green. Full suite 1657 passing.

DemoSeed_PrefersTheEnabledOwner_EvenWhenADisabledOneSortsFirst — ids are
chosen, not generated (00000000-…a1 disabled vs ffffffff-…a2 active), so
the disabled Owner provably sorts first. With random GUIDs this would catch the
bug only about half the time.

DemoSeed_WithOnlyADisabledOwner_FailsClosed — asserts PrerequisitesMissing,
zero flocks written, and that the message does not send the operator to
bootstrap-admin.

SimulationSeed_WithTheOnlyOwnerDisabled_FailsClosedAndDoesNotSayBootstrapAdmin
— added because the fix landed in both seeders but only one had tests, which is
the shape that ships silently.

Each guard runs in its own Postgres container, so none depends on xUnit ordering.

Two related readers deliberately still count disabled Owners, checked rather than
assumed: FirstRunAdminService's idempotency check (auto-minting a second Owner
because the first is disabled is a security decision, not a convenience) and the
simulation manifest's Owners count (an inventory of what was provisioned, not of
who can act).

Codex review of #517. Disabling a user KEEPS their Owner role row and only stamps DisabledAt -- IdentityProvider says so itself, "a disabled actor retains its Owner ROLE ROW, only authentication is blocked" -- while login rejects exactly that flag. So GetUsersInRoleAsync still returned disabled Owners, and ordering by Id could sign a whole fixture with an account nobody can log in as: every History line naming somebody who could never look at the records they supposedly created. With ONLY a disabled Owner the preflight passed and the seed reported success. Both seeders now exclude DisabledAt before the deterministic pick. A local review of that fix then caught a second defect in it: the reworded message told the operator to run bootstrap-admin, which does NOT help for this cause. That verb counts Owner role rows without checking DisabledAt, so it reports "already provisioned", exits 0 having done nothing, and sends them back to the same error -- an instruction loop with no exit. AdminRecoveryService already documents that trap; this was the second place it bites. Both seeders now split the message by cause and name a remedy that works. Guards, each in its own container: only-disabled fails closed and does not say bootstrap-admin; a disabled Owner sorting FIRST still loses to the active one (ids are chosen, not generated, so the ordering is the test's to control -- with random GUIDs it would catch the bug half the time); and the simulation seeder gets its own, since the fix landed in both seeders but only one had tests. Mutation-checked: removing the filter turns the demo guards red; the message assertions fail if the remedy text regresses.

mforce · 2026-08-12T09:08:24Z

@codex the disabled-Owner finding (P2) is addressed in acb77b16.

Both seeders now exclude DisabledAt before the deterministic pick. A local review of that fix caught a second defect in it: the reworded message still pointed at bootstrap-admin, which counts Owner role rows without checking DisabledAt — it reports "already provisioned", exits 0, and loops the operator back to the same error. Both messages now split by cause.

Guards added, each in its own container, all mutation-checked: only-disabled fails closed and does not name bootstrap-admin; a disabled Owner with a deliberately lower id still loses to the active one; and the simulation seeder got its own guard, since the fix landed in both seeders but only one had tests. Full suite 1657 green.

Please re-review — particularly whether any other reader of Owner role rows still has the same blind spot, and whether the new messages name a remedy that actually works in every state they can be printed in.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acb77b1612

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-12T09:12:23Z

CVE-2026-62901

+                  "could view the fixture they supposedly created. `bootstrap-admin` will NOT fix this: it " +
+                  "counts Owner role rows without checking DisabledAt, so it reports 'already provisioned' and " +
+                  "does nothing. Re-enable the Owner from the Users screen, then re-run `seed --profile demo`."


Provide a usable recovery path for disabled-only accounts

When this branch is reached, FindOwnerAsync has proved that the account has no enabled Owner, so “Re-enable the Owner from the Users screen” cannot be followed: the entire Users group requires OwnerOnly authorization (Program.cs lines 396–401), disabled users cannot authenticate, recover-admin refuses disabled targets, and bootstrap-admin no-ops on their retained Owner role row. The equivalent simulation message has the same defect. These messages should identify the actual supported repair, or state that direct database recovery is currently required until an enable-capable CLI path exists.

AGENTS.md reference: AGENTS.md:L54-L54

Useful? React with 👍 / 👎.

Confirmed and fixed in 3a536050. You're right, and I verified each leg rather
than taking it on trust:

/api/v1/users is .RequireAuthorization(AuthPolicies.OwnerOnly) — Program.cs

a disabled user cannot authenticate (IdentityProvider rejects DisabledAt)

recover-admin explicitly refuses a disabled target — AdminRecoveryService

bootstrap-admin no-ops on the retained Owner role row — FirstRunAdminService

So "re-enable from the Users screen" is unreachable from the exact state that
prints it. There is no in-product repair today.

Both messages now say so plainly and name the one thing that works — clearing
DisabledAt directly in the database — with the reason each obvious-looking
alternative fails, so nobody re-derives this the hard way. AdminRecoveryService
already records that #357 is the intended CLI answer.

This is the second correction to the same message: round 1 named a remedy that
loops, round 2 one that is unreachable. So I changed what the tests pin. They
no longer assert a particular sentence — they assert the property that kept being
violated: the remedy named must be performable from the state that printed it.
no in-product repair + directly in the database are the load-bearing clauses,
in both the demo and simulation guards.

Full suite 1657 green. Also rebased onto main now that #518 has merged, so the
Trivy job should go green here too — it was failing on CVE-2026-62901 in the
pinned .NET base image, unrelated to this PR and reproducing on every open PR.

Codex review of #517. Disabling a user KEEPS their Owner role row and only stamps DisabledAt -- IdentityProvider says so itself, "a disabled actor retains its Owner ROLE ROW, only authentication is blocked" -- while login rejects exactly that flag. So GetUsersInRoleAsync still returned disabled Owners, and ordering by Id could sign a whole fixture with an account nobody can log in as: every History line naming somebody who could never look at the records they supposedly created. With ONLY a disabled Owner the preflight passed and the seed reported success. Both seeders now exclude DisabledAt before the deterministic pick. A local review of that fix then caught a second defect in it: the reworded message told the operator to run bootstrap-admin, which does NOT help for this cause. That verb counts Owner role rows without checking DisabledAt, so it reports "already provisioned", exits 0 having done nothing, and sends them back to the same error -- an instruction loop with no exit. AdminRecoveryService already documents that trap; this was the second place it bites. Both seeders now split the message by cause and name a remedy that works. Guards, each in its own container: only-disabled fails closed and does not say bootstrap-admin; a disabled Owner sorting FIRST still loses to the active one (ids are chosen, not generated, so the ordering is the test's to control -- with random GUIDs it would catch the bug half the time); and the simulation seeder gets its own, since the fix landed in both seeders but only one had tests. Mutation-checked: removing the filter turns the demo guards red; the message assertions fail if the remedy text regresses.

Codex re-review of #517, round 2, against my own previous fix. The disabled-only-Owner message said "re-enable the Owner from the Users screen". That cannot be followed from the state that prints it: the users group requires OwnerOnly, a disabled user cannot authenticate, and the only Owner is the disabled one. recover-admin refuses a disabled target and bootstrap-admin no-ops on the retained role row, so there is NO in-product repair for this state today. Both messages now say exactly that and point at the one thing that works: clearing DisabledAt directly in the database, until #357 ships an enable-capable CLI path. This is the second correction to the same message. Round 1 named a remedy that loops; round 2 named one that is unreachable. The tests now pin the property that matters -- the remedy must be performable from the state that printed it -- rather than any particular wording.

mforce · 2026-08-12T15:34:35Z

@codex round 2's finding is addressed in 3a536050.

Verified each leg before changing anything: /api/v1/users is OwnerOnly, a disabled user cannot authenticate, recover-admin refuses a disabled target, and bootstrap-admin no-ops on the retained role row. So there is genuinely no in-product repair for a disabled-only-Owner account, and both messages now say that and point at clearing DisabledAt in the database until #357 lands.

Worth noting what changed in the tests: this was the second correction to the same message — round 1 named a remedy that loops, round 2 one that is unreachable — so the guards no longer pin a sentence. They pin the property that kept being violated: the remedy named must be performable from the state that printed it.

Full suite 1657 green, and rebased onto main now that #518 merged (the Trivy red was CVE-2026-62901 in the pinned .NET base image, unrelated to this PR).

Please re-review.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a53605005

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-12T15:39:59Z

+                  "no in-product repair for this state today, so do not go looking for one: `bootstrap-admin` " +
+                  "counts Owner role rows without checking DisabledAt and reports 'already provisioned'; " +
+                  "`recover-admin` refuses a disabled target; and the Users screen that could re-enable them " +
+                  "is Owner-only, which nobody can now reach. Clear DisabledAt for that user directly in the " +


Repair both disabled-state columns

When the only Owner is disabled, following this instruction literally clears DisabledAt but leaves DisabledBy populated. Fresh evidence beyond the prior thread is the existing invariant in IdentityProvider.EnableUserAsync (lines 859–863), which explicitly treats these columns as one live fact and clears both; Enable_ClearsDisabledBy_NotJustDisabledAt also pins that behavior. The proposed repair therefore restores login while leaving an active user carrying stale disable metadata, so the message should direct operators to clear both fields or reference a complete database-repair procedure.

Useful? React with 👍 / 👎.

Confirmed and fixed in a4f0b638.

EnableUserAsync makes the point itself — "Both columns describe ONE live fact. Leaving DisabledBy behind would be a column that reads as current and is not" — and Enable_ClearsDisabledBy_NotJustDisabledAt pins it. My repair instruction would have restored login and left stale disable metadata on an active user.

Both messages now say to clear both DisabledAt and DisabledBy, and say why, so the reason travels with the instruction.

chatgpt-codex-connector · 2026-08-12T15:39:59Z

+                // operator whose only Owner is DISABLED at `bootstrap-admin`
+                // sends them in a circle, because it counts Owner role rows
+                // without checking DisabledAt and exits 0 having done nothing.
+                var prereqMessage = disabledOwners > 0


Check for an active Owner before giving disabled-only advice

When an enabled Owner and a disabled co-Owner coexist, disabledOwners > 0 is true even though owner is non-null. If a required grade was renamed, MissingBaseDataAsync enters this block because of the grade, but this branch falsely says the account has only disabled Owners and directs the operator to modify DisabledAt instead of repairing the base data. Gate this advice on owner is null and otherwise report the missing base prerequisite; grades are explicitly user-renamable.

AGENTS.md reference: AGENTS.md:L50-L50

Useful? React with 👍 / 👎.

Confirmed and fixed in a4f0b638. This was the sharpest of the three.

You're right that the block is entered for any missing base prerequisite, and the grades are user-renamable (#283) — so the realistic cause is a renamed grade, not the Owner. Gated on owner is null && disabledOwners > 0.

Guarded by SimulationSeed_WithAnEnabledOwnerAndADisabledCoOwner_ReportsTheRealPrerequisite, which builds exactly your scenario: the factory's enabled Owner, a disabled co-Owner beside it, and a renamed Large grade. It asserts the message names the base data and does not contain DisabledAt or no in-product repair.

Mutation-checked: reverting to disabledOwners > 0 turns that guard red on the base data assertion.

chatgpt-codex-connector · 2026-08-12T15:39:59Z

+        // it), so a literal would fabricate a privilege rather than report one —
+        // and it would keep claiming Owner even for a user demoted between the
+        // lookup above and this line.
+        currentUser.Resolve(owner.Id, owner.Email!, [.. await users.GetRolesAsync(owner)]);


Update the SPA help and glossary for seeded actors

This changes the actor users see in the five History surfaces, and the PR updates the product glossary to explain real and system actors, but it makes no corresponding change under web/ to the SPA Help page or in-app glossary. The repository requires both user-facing documentation surfaces to be updated with every user-visible behavior change, so the localized guide will otherwise omit the new seeded/system-actor behavior.

AGENTS.md reference: AGENTS.md:L252-L252

Useful? React with 👍 / 👎.

You're right, and my earlier reasoning was wrong in a way worth naming.

Slice 6 recorded a deliberate decision not to touch the Help page, on the ground that a real farm never had (unresolved) rows. That considered only the five #494 History screens and missed the audit log viewer: (bootstrap-admin) and (break-glass) are written on a real production farm — the first Owner's User.Create and any break-glass reset — and an Owner browsing /audit sees them. So it genuinely was a user-visible change.

Fixed in a4f0b638: a new auditSystemActors entry in the Help page's audit section, explaining both bracketed labels and that a break-glass entry also records the machine and the reason, so an emergency reset is never anonymous. Shipped in en/es/tl per the repo's translate-now policy; catalogParity passes. Guarded by a Vitest case asserting both labels, the accountability clause, and that the reader is not left thinking everything is nameless.

There is no in-app glossary surface to update yet — #71 has not landed.

…tors Codex round 3 on #517, three findings, all real. 1. The simulation branch keyed on `disabledOwners > 0` without checking `owner is null`. That block is entered when ANY base prerequisite is missing, and the egg grades are user-renamable (#283) -- so an account with a perfectly good enabled Owner, one disabled co-Owner and a renamed grade was told to go editing DisabledAt while the real fault was the grade name. Now gated, and guarded by a test that reproduces exactly that combination. 2. Both messages named only DisabledAt. EnableUserAsync clears DisabledBy as well -- "both columns describe ONE live fact" -- so following the instruction literally restored login and left stale disable metadata on an active user. Both messages now name both columns. 3. The SPA Help page had no mention of the system actors. Slice 6 recorded a deliberate decision to skip it, reasoning that a real farm never had "(unresolved)" rows. That considered only the five #494 History screens and missed the audit log viewer: "(bootstrap-admin)" and "(break-glass)" are written on a REAL farm -- the first Owner's creation and any break-glass reset -- and an Owner browsing /audit sees them. Help now explains both labels, in en/es/tl, with a Vitest guard; catalog parity passes. Mutation-checked: reverting the gating fix turns the new co-Owner guard red on its "base data" assertion. Suites green -- 1658 .NET, 1739 web.

mforce · 2026-08-13T01:41:56Z

@codex all three round-3 findings are addressed in a4f0b638, replies on each thread.

Gating — owner is null && added; the disabled-only advice no longer fires when a renamed grade is the real cause. New guard builds exactly that combination (enabled Owner + disabled co-Owner + renamed Large) and is mutation-checked.
Both columns — messages now say to clear DisabledAt and DisabledBy, per EnableUserAsync's own "both columns describe ONE live fact".
Help page — you were right and my slice-6 reasoning was wrong: I considered only the five Show "created by / when" and "last changed by / when" inline on record pages #494 History screens and missed /audit, where (bootstrap-admin) and (break-glass) appear on a real farm. Added in en/es/tl with a Vitest guard; catalog parity green.

Suites: 1658 .NET, 1739 web.

Three rounds, three-for-three on real defects, each against the previous round's fix — that chain has been the most productive part of this PR, so please keep going. One thing I'd particularly like challenged: the new guard asserts the message does not contain DisabledAt, which pins a negative. If you can see a state where that assertion passes while the advice is still wrong, say so.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4f0b638d5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-13T01:45:30Z

+                    : "Simulation seed prerequisites missing: the base data (default account, Admin role, " +
+                      "the saleable Large/Medium/Small egg grades, and an enabled admin in the Owner role) is not " +
+                      "fully present. The account/role/grades ship with the EF migrations (#283); the Owner " +
+                      "admin does not — run `dotnet Cluckwork.Api.dll bootstrap-admin --email <e>` against " +
+                      "this database, then re-run `seed --profile simulation`.";


Direct grade failures to a grade repair

When an enabled Owner, a disabled co-Owner, and a renamed Large grade coexist—the new guard's exact state—the negative DisabledAt assertion passes, but this fallback still instructs the operator to run bootstrap-admin. FirstRunAdminService.ProvisionUnderLockAsync returns AlreadyProvisioned whenever any Owner exists (lines 222–224), so rerunning the seed produces the same failure indefinitely; distinguish the enabled-Owner/base-data case and identify the missing or renamed grade instead.

AGENTS.md reference: AGENTS.md:L50-L50

Useful? React with 👍 / 👎.

Confirmed and fixed in 6b75b2e8 — and you were right that this is the round-3 fix's own remaining defect, which makes it the fourth round running where the finding is against the previous round's fix.

Fixed structurally rather than with a third prose patch. The base-data check and the Owner check are now two separate blocks, which is what DemoDataSeeder has always had — that is precisely why the demo seeder never had this bug. One if serving two unrelated causes had to guess which had failed, and the guess is what kept being wrong: round 2 was a remedy that loops, round 3 a remedy that is unreachable, this one a remedy that no-ops. MissingBaseDataAsync no longer takes the Owner at all.

Guarded by SimulationDisabledCoOwnerTests, which now also asserts DoesNotContain("dotnet Cluckwork.Api.dll bootstrap-admin") — on the invocation form, not the bare word, because the new message is allowed to mention bootstrap-admin in order to rule it out, and does. Mutation-verified: restoring the old instruction turns it red on exactly that assertion.

One thing your finding did not cover, which a local reviewer caught afterwards: two blocks short-circuit, so a database missing both the base data and the Owner now reported only the first, and the operator would repair the grades, re-run, and only then meet the second problem. That was a regression the split itself introduced. The base-data message now carries an appendix naming the Owner remedy too, gated on owner is null, with its own test — which doubles as proof the negative assertion above is not vacuous: same message, opposite condition, that exact string required to appear.

chatgpt-codex-connector · 2026-08-13T01:45:30Z

        var existing = await users.FindByEmailAsync(email);
-        if (existing is not null) return existing.Id;
+        if (existing is null)


Reject disabled simulation cast members

On a partial rerun after a seeded manager, salesperson, or worker has been disabled, FindByEmailAsync still returns that user and disabling retains their role row, so the later role check accepts them and the new actor rotation attributes additional fixture records to them. IdentityProvider.LoginAsync rejects the same user at line 41, meaning the seed can report success while newly created history names a persona who cannot log in; reject cast members with DisabledAt != null before constructing the SimActor.

Useful? React with 👍 / 👎.

Confirmed and fixed in 6b75b2e8. This was the strongest of the three, and the mutation run backs your wording exactly: the seed reports success.

EnsureUserAsync now refuses a persona with DisabledAt != null, before the role check. Deleting that guard flips SimulationDisabledCastMemberTests red on Assert.Equal(SeedStatus.Failed, ...) with an actual Seeded — the fixture is written, the counts validate, the manifest is emitted, and nothing surfaces until a human tries to sign in as the persona a History line names.

Worth recording why nothing downstream catches it, because it is the difference between this finding and the worker one below: ValidateCounts counts users by role bucket, and disabling moves nobody between buckets. So this is genuinely invisible to the fixture's own validation, while a promotion is not.

The advice here is deliberately different from the disabled-Owner message — "re-enable from the Users screen" rather than "repair the database directly". That is reachable in this case and only in this case: SeedCastAsync runs strictly after the owner is null preflight has passed, so an enabled Owner is guaranteed to exist, and the cast never contains an Owner, so the disabled persona is never the one blocking its own repair. A local reviewer checked that chain against Program.cs's OwnerOnly policy on the users group specifically because the same advice was wrong two rounds ago.

chatgpt-codex-connector · 2026-08-13T01:45:30Z

+        // Fail here instead, naming both roles. Workers are exempt because
+        // "Worker" is a pseudo-role CreateUserHandler maps to null: a worker
+        // correctly holds none.
+        if (role != CreateUserValidator.WorkerRole && !roles.Contains(role))


Reject role-bearing users from the worker pool

When an existing sim-worker-* user has been assigned Manager, Sales, or ReadOnly through the Users UI, this exemption accepts them solely because the requested pseudo-role is Worker, even though roles is nonempty. The returned actor then authors worker daily entries under their actual role; a promoted Manager also bypasses FlockScopeGuard, so a rerun can succeed without exercising the restricted-worker authorization shape. Require an existing worker persona to hold no assignable role.

AGENTS.md reference: AGENTS.md:L54-L54

Useful? React with 👍 / 👎.

Real defect, fixed in 6b75b2e8 — but the severity is not what you claimed, and the mutation run is what showed it.

The fix is what you asked for: the worker case is now checked rather than exempted. A worker holds no assignable role by construction, so the correct assertion for a worker is roles is empty, not "skip the check". The test promotes the restricted worker specifically, since that is the one persona whose FlockScopeGuard narrowing the fixture exists to exercise.

Where I disagree: "a rerun can succeed without exercising the restricted-worker authorization shape". It cannot. Removing the guard does not produce a green run — ValidateCounts counts users by role bucket, and a promotion moves one user between two of them:

users.managers: expected 1, got 2; users.workers: expected 3, got 2

So the real defect is a misleading failure late — that check runs in EmitManifestAsync, last, after every durable write — not a silent success. Still worth the guard, for the reason the surrounding comment already gave before this round: failing at the cast names the cause, failing at the manifest names counts.

That correction changed the test rather than just the reply. My first version asserted Status == Failed, which the late count check satisfies on its own — a mutant that dies on an assertion other than the one it is named for. It now asserts DoesNotContain("expected"), which is the only thing separating refused at the cast from refused at the manifest once both produce Failed.

Contrast with the disabled-persona finding above, which I have confirmed is silently green: disabling moves nobody between role buckets, so the same validation is blind to it. Two findings in one round, same file, opposite answers on whether anything downstream notices.

Codex round 4 on PR #517, three findings, all real. Split the combined base-data/Owner preflight into two blocks, matching what DemoDataSeeder has always had. One `if` serving two unrelated causes had to guess which had failed, and that guess is what kept being wrong: a renamed egg grade with a healthy Owner was told to run `bootstrap-admin`, which returns AlreadyProvisioned whenever any Owner exists. Third round in a row on the same defect shape, so this one is structural rather than more prose. Reject a DISABLED cast member on a partial re-run. Disabling keeps every role row and only stamps DisabledAt, so the persona was accepted and signed newly written fixture history. Confirmed silently green by mutation: the manifest's count validation moves nobody between role buckets and cannot see it. Assert, rather than exempt, the worker persona's empty role set. A worker promoted through the Users UI passed as a worker and then bypassed FlockScopeGuard by role. Measured severity is lower than the review claimed: ValidateCounts does catch this, but last, after every durable write, with a message about counts instead of about the cause. A local reviewer then caught a defect the split introduced: with both the base data and the Owner missing, the first block returned before the second ran, so one broken database took two trips to diagnose. The message now names both remedies. Suites: 1661 green.

mforce · 2026-08-13T02:16:30Z

@codex round 4 is addressed in 6b75b2e8. All three findings were real; one was overstated and I have said so on its thread rather than quietly accepting it.

Grade failure told to run bootstrap-admin — fixed structurally, not with a third prose patch. The combined preflight is now two blocks, matching DemoDataSeeder, which is why the demo seeder never had this bug.
Disabled cast member — confirmed silently green by mutation. Deleting the guard yields Seeded, because the manifest count check moves nobody between role buckets and cannot see it.
Promoted worker — real, but not a green run: ValidateCounts catches it with users.managers: expected 1, got 2, last, after every durable write. Misleading-failure-late, not silent-success. The test was rewritten because of this, since my first version asserted only Status == Failed, which that late check satisfies on its own.

A local reviewer then found a defect the fix itself introduced — two blocks short-circuit, so a database missing both the base data and the Owner took two trips to diagnose. Fixed, tested, mutation-verified.

Suites: 1661 green.

Four rounds, four-for-four on real defects, every one against the previous round's fix. Two specific things to attack next:

The dual-failure appendix is gated on owner is null and appended to the base-data message. I believe that is the only interaction between the two blocks. If there is a third state where one block's message is silently incomplete about another's cause, name it.
I deleted a defensive if (owner is null) block on the grounds that C# nullable flow analysis proves it unreachable after the new early return. A local reviewer verified nothing reassigns owner in between and that the build is warning-clean under warnings-as-errors. If you can see a path where that reasoning is wrong, that is the finding I most want.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b75b2e813

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-13T02:18:58Z

+            // after the preflight could disagree with it (an Owner disabled or
+            // reassigned between the two) and the seeder would then act as a
+            // user its own preflight never approved.
+            var (owner, disabledOwners) = await FindOwnerAsync(accountId);


Check base data before querying the Owner role

When the migration-baked Owner role itself is absent—the partially restored schema this preflight explicitly handles—FindOwnerAsync calls UserManager.GetUsersInRoleAsync(Roles.Owner) before MissingBaseDataAsync can detect the missing role. Identity throws because the named role does not exist, so the outer catch returns a generic Failed result rather than the intended PrerequisitesMissing base-data advice, and the new dual-failure appendix is never reached. Run the base-data check first or make the Owner lookup tolerate an absent role.

AGENTS.md reference: AGENTS.md:L50-L50

Useful? React with 👍 / 👎.

The mechanism you describe does not reproduce, but chasing it found a real defect one level down. Both halves are in 0d64ac0a.

What is wrong with the finding: UserManager.GetUsersInRoleAsync does not throw for an absent role. UserStore.GetUsersInRoleAsync returns an empty list when FindRoleAsync misses; throwing on a missing role is AddToRoleAsync/IsInRoleAsync. I wrote the test before the fix specifically to check your claim, and against unfixed code it already passed on:

Assert.Equal(SeedStatus.PrerequisitesMissing, result.Status); Assert.Contains("base data", result.Message); Assert.Contains("migrate", result.Message);

So the base-data check caught the dropped role exactly as intended, and the ordering is not load-bearing for the reason given.

What you were right about: owner is null is hiding more than one state. The empty list makes a schema whose migration-baked roles were dropped indistinguishable from "nobody has been made an Owner yet" — and the dual-failure appendix I added last round therefore appended run bootstrap-admin, which cannot create a role. Wrong advice, in the one state this preflight exists to handle. That is a defect I introduced in round 4, so it is the kind most worth having caught.

FindOwnerAsync now returns RoleExists alongside the Owner and the disabled count, and the appendix is gated on it. Mutation-verified: removing the gate turns SimulationMissingOwnerRoleTests red on the DoesNotContain("bootstrap-admin --email") assertion, which is the one the finding is named for. The three assertions that passed pre-fix are kept deliberately — they are what stops the new one being a bare negative.

DemoDataSeeder needs no equivalent change: it checks base data first and returns, so its Owner block is unreachable with the role missing.

Suites: 1662 green.

…500) Codex round 5 on PR #517. The finding as stated does not reproduce, and the test was written first to find that out: the claim was that GetUsersInRoleAsync throws for an absent role, so the Owner lookup running ahead of the base-data check would surface a generic Failed instead of the prerequisite advice. It returns an empty list instead — throwing is AddToRoleAsync/IsInRoleAsync — so the base-data check already caught it and the status and message assertions passed against unfixed code. The empty list does cause a smaller defect one level down, introduced by round 4. A missing Owner ROLE and "nobody has been made an Owner yet" both present as `owner is null`, so the dual-failure appendix advised running bootstrap-admin, which cannot create a role. FindOwnerAsync now reports whether the role exists and the appendix is gated on it; restoring the base data, which the message already says, is the whole remedy in that case. Suites: 1662 green.

Seeded audit events carried ActorEmail = "(unresolved)". Both seeders resolved TenantContext but never ICurrentUser, so everything they wrote took AuditWriter's placeholder fallback -- invisible until #494 rendered provenance on five screens. AuditWriter now throws on an unresolved actor, symmetric with the tenant guard beside it, and every non-HTTP caller declares who it is: bootstrap-admin and recover-admin declare system actors; both seeders resolve the account's Owner. Demo gains that prerequisite and fails closed without it -- a deliberate break with the old "needs nothing but a connection string" contract. The simulation fixture is attributed per persona: managers place the flocks and create the catalog and expenses, sales staff book the orders, a rotating worker pool records the daily entries under a per-flock eligibility rule, and about one submission in three is a manager signing off somebody else's draft -- so both #494 provenance shapes now exist. ICurrentUser is an authorization input, not an audit label: FlockScopeGuard reads it, so the seeders' unresolved actor was a live authorization bypass and which worker the seeder acts as decides what it may write. A final review round found a false claim in FlockScopeGuard's own comment, already disproved by this plan's round 1: two of the four handlers behind that guard write no audit row, so the unresolved-actor branch is not unreachable for them. Nothing exploits it today; documented rather than silently narrowed, since closing it flips an authorization default. Closes #500.

Codex review of #517. Disabling a user KEEPS their Owner role row and only stamps DisabledAt -- IdentityProvider says so itself, "a disabled actor retains its Owner ROLE ROW, only authentication is blocked" -- while login rejects exactly that flag. So GetUsersInRoleAsync still returned disabled Owners, and ordering by Id could sign a whole fixture with an account nobody can log in as: every History line naming somebody who could never look at the records they supposedly created. With ONLY a disabled Owner the preflight passed and the seed reported success. Both seeders now exclude DisabledAt before the deterministic pick. A local review of that fix then caught a second defect in it: the reworded message told the operator to run bootstrap-admin, which does NOT help for this cause. That verb counts Owner role rows without checking DisabledAt, so it reports "already provisioned", exits 0 having done nothing, and sends them back to the same error -- an instruction loop with no exit. AdminRecoveryService already documents that trap; this was the second place it bites. Both seeders now split the message by cause and name a remedy that works. Guards, each in its own container: only-disabled fails closed and does not say bootstrap-admin; a disabled Owner sorting FIRST still loses to the active one (ids are chosen, not generated, so the ordering is the test's to control -- with random GUIDs it would catch the bug half the time); and the simulation seeder gets its own, since the fix landed in both seeders but only one had tests. Mutation-checked: removing the filter turns the demo guards red; the message assertions fail if the remedy text regresses.

Codex re-review of #517, round 2, against my own previous fix. The disabled-only-Owner message said "re-enable the Owner from the Users screen". That cannot be followed from the state that prints it: the users group requires OwnerOnly, a disabled user cannot authenticate, and the only Owner is the disabled one. recover-admin refuses a disabled target and bootstrap-admin no-ops on the retained role row, so there is NO in-product repair for this state today. Both messages now say exactly that and point at the one thing that works: clearing DisabledAt directly in the database, until #357 ships an enable-capable CLI path. This is the second correction to the same message. Round 1 named a remedy that loops; round 2 named one that is unreachable. The tests now pin the property that matters -- the remedy must be performable from the state that printed it -- rather than any particular wording.

…tors Codex round 3 on #517, three findings, all real. 1. The simulation branch keyed on `disabledOwners > 0` without checking `owner is null`. That block is entered when ANY base prerequisite is missing, and the egg grades are user-renamable (#283) -- so an account with a perfectly good enabled Owner, one disabled co-Owner and a renamed grade was told to go editing DisabledAt while the real fault was the grade name. Now gated, and guarded by a test that reproduces exactly that combination. 2. Both messages named only DisabledAt. EnableUserAsync clears DisabledBy as well -- "both columns describe ONE live fact" -- so following the instruction literally restored login and left stale disable metadata on an active user. Both messages now name both columns. 3. The SPA Help page had no mention of the system actors. Slice 6 recorded a deliberate decision to skip it, reasoning that a real farm never had "(unresolved)" rows. That considered only the five #494 History screens and missed the audit log viewer: "(bootstrap-admin)" and "(break-glass)" are written on a REAL farm -- the first Owner's creation and any break-glass reset -- and an Owner browsing /audit sees them. Help now explains both labels, in en/es/tl, with a Vitest guard; catalog parity passes. Mutation-checked: reverting the gating fix turns the new co-Owner guard red on its "base data" assertion. Suites green -- 1658 .NET, 1739 web.

Codex round 4 on PR #517, three findings, all real. Split the combined base-data/Owner preflight into two blocks, matching what DemoDataSeeder has always had. One `if` serving two unrelated causes had to guess which had failed, and that guess is what kept being wrong: a renamed egg grade with a healthy Owner was told to run `bootstrap-admin`, which returns AlreadyProvisioned whenever any Owner exists. Third round in a row on the same defect shape, so this one is structural rather than more prose. Reject a DISABLED cast member on a partial re-run. Disabling keeps every role row and only stamps DisabledAt, so the persona was accepted and signed newly written fixture history. Confirmed silently green by mutation: the manifest's count validation moves nobody between role buckets and cannot see it. Assert, rather than exempt, the worker persona's empty role set. A worker promoted through the Users UI passed as a worker and then bypassed FlockScopeGuard by role. Measured severity is lower than the review claimed: ValidateCounts does catch this, but last, after every durable write, with a message about counts instead of about the cause. A local reviewer then caught a defect the split introduced: with both the base data and the Owner missing, the first block returned before the second ran, so one broken database took two trips to diagnose. The message now names both remedies. Suites: 1661 green.

…500) Codex round 5 on PR #517. The finding as stated does not reproduce, and the test was written first to find that out: the claim was that GetUsersInRoleAsync throws for an absent role, so the Owner lookup running ahead of the base-data check would surface a generic Failed instead of the prerequisite advice. It returns an empty list instead — throwing is AddToRoleAsync/IsInRoleAsync — so the base-data check already caught it and the status and message assertions passed against unfixed code. The empty list does cause a smaller defect one level down, introduced by round 4. A missing Owner ROLE and "nobody has been made an Owner yet" both present as `owner is null`, so the dual-failure appendix advised running bootstrap-admin, which cannot create a role. FindOwnerAsync now reports whether the role exists and the appendix is gated on it; restoring the base data, which the message already says, is the whole remedy in that case. Suites: 1662 green.

mforce · 2026-08-13T02:47:30Z

The review loop is stopped deliberately at five rounds — not abandoned, and not because the reviewer went quiet. Recording the count so the silence is not misread.

Rounds 1–4 each confirmed a real defect in shipped behaviour, and every one was against the previous round's fix: wrong actor → a remedy that loops → a remedy that is unreachable → advice printed for the wrong cause → the cast held to a weaker standard than the Owner. That escalating chain was the most productive part of this PR and is exactly why it ran this long.

Round 5 is where it turned. Its stated finding did not reproduce — GetUsersInRoleAsync returns an empty list for an absent role rather than throwing — and what it actually surfaced was operator wording in a state that requires someone to have dropped migration-baked roles by hand. Real, fixed, but a corner of the corner round 4 opened. An automated reviewer always has one more level, so "wait for silence" is not a terminating condition; the owner called it here.

Everything in flight is finished: round 5 is fixed in 0d64ac0a, its thread is answered, and the branch is rebased onto main (the AGENTS.md conflict was #505's new bullet landing beside #500's — both kept).

Final state: 1667 .NET tests (145 application, 325 domain, 1197 integration) and 1779 web, all green, typecheck clean.

Anyone picking this up later: the loop stopped by decision, and the open question I would have asked round 6 is in the previous comment — whether the deleted if (owner is null) block is genuinely unreachable under nullable flow analysis. A local reviewer verified it; codex never got to.

mforce mentioned this pull request Aug 12, 2026

EPIC: Phase 1.1 — Operational fill #14

Closed

90 tasks

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

mforce mentioned this pull request Aug 12, 2026

chore(deps): bump the aspnet runtime base image to .NET 10.0.11 #518

Merged

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

mforce force-pushed the fix/500-seeded-audit-actor branch from acb77b1 to a2bf65b Compare August 12, 2026 15:30

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 13, 2026

View reviewed changes

mforce added 6 commits August 12, 2026 19:45

mforce force-pushed the fix/500-seeded-audit-actor branch from 0d64ac0 to 3db184f Compare August 13, 2026 02:47

mforce merged commit a552d08 into main Aug 13, 2026
10 checks passed

cluckwork-lockfix Bot mentioned this pull request Aug 13, 2026

chore(main): release 0.0.4 #461

Merged

mforce deleted the fix/500-seeded-audit-actor branch August 13, 2026 04:53

actor	rows
`sim-worker-2`, `sim-worker-3`	133 each
`sim-manager-1`	85
`sim-sales-1`	49
`sim-worker-1` (flock-restricted)	30
Owner	11
`(bootstrap-admin)`	1

Conversation

mforce commented Aug 12, 2026

What changed

The thing that made this harder than it looks

Proof

Review

Not done, deliberately

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mforce commented Aug 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mforce commented Aug 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mforce commented Aug 13, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mforce commented Aug 13, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment