fix(api): print the farm code from bootstrap-admin (#589) - #594
Conversation
…#589) `tenant-bypass-allowlist.json` pins `ProvisionUnderLockAsync`'s exact symbol signature, and #589 added an `accountSlug` parameter to it, so the enumerating guard from #536 went red. The entry now matches; the justification is unchanged because the reason for the bypass is unchanged (first-run bootstrap runs at an unresolved tenant by design, #283). This was a defect in the runbook, not in the implementation: the file was left out of the permitted-edit list because the driver authored a signature change without grepping for readers of that symbol. The implementer correctly refused to touch a file outside its allow-list and flagged the decision instead. Verified the entry was corrected rather than the guard widened: renaming the parameter reddens TenantBypass again (1 failed / 15 passed); restoring it is green (16 passed). Full suite 1942 passed, 0 failed.
|
@codex review — please review at Two things worth your attention, both places a guard could read as safety without being one:
Also: the slug is read from the account row rather than hardcoded, deliberately — the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 759fd3fa7b
ℹ️ 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".
| .Where(a => a.Id == SeedDefaults.AccountId) | ||
| .Select(a => a.Slug) | ||
| .SingleAsync(); | ||
| Assert.Contains($"on farm {defaultAccountSlug} ", stdout); |
There was a problem hiding this comment.
This assertion still passes if production replaces outcome.Slug with the literal "default-farm", because every database used by this test retains that same migration-seeded value. Reading the expected value dynamically therefore does not prove the CLI reads the row as claimed; mutate the test account to a different valid slug before invoking the command so the hardcoded-literal regression turns the guard red.
AGENTS.md reference: AGENTS.md:L139-L145
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 439077d2. Correct, and sharper than my own runbook instruction — thank you.
My runbook required the expected value be read from the database rather than a literal, and it was. That was not enough, for exactly the reason you give: every test database carries the same migration-seeded default-farm, so the assertion could not distinguish "read the row" from "printed a constant". Reading dynamically from a fixture that never varies is not a discriminating assertion.
The test now updates the account's Slug to varied-farm-x7q2 before invoking the command and asserts on that value.
Verified by mutation, run by me rather than quoted: replacing {outcome.Slug} with the literal "default-farm" now reddens FirstRun_CreatesOwner_PrintsPasswordOnlyToStdout_AndSetsMustChangePassword (1 failed / 5 passed). Before this fix that same mutant stayed green, which is precisely your point.
On fixture isolation, since varying shared state is its own hazard: the class uses IClassFixture<CluckworkWebApplicationFactory> — one container, one database shared across the cases. The slug change is made via raw SQL and restored in a finally in both mutating tests, so it cannot leak into the others. The full 11-test group passes.
| // provisioning happened, so no "on farm" line may appear. This is the | ||
| // mutant that keeps the AlreadyProvisioned() branch honest — if that | ||
| // branch ever started populating Slug and printing it, this reds. | ||
| Assert.DoesNotContain(" on farm ", second.Stdout, StringComparison.Ordinal); |
There was a problem hiding this comment.
This absence assertion only bans the exact text " on farm ", so a broken re-run that emits Farm code: default-farm still passes. It also does not red when AlreadyProvisioned() merely starts populating Slug, because BootstrapAdminCliCommand.RunAsync returns from its fixed-message branch before reading that field; resolve the actual account slug and assert that value is absent from the complete re-run output.
AGENTS.md reference: AGENTS.md:L139-L143
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 439077d2. Both halves of this were right, and the second half caught a false claim of mine.
The phrase ban. The assertion now resolves the account's real slug and asserts that value is absent from the whole re-run output, rather than banning the single phrase " on farm ". Your example is the proof it matters: the mutation I used emits nothing to do (farm varied-farm-x7q2)., which the old phrase-ban could not have caught.
The mutation that could not fire. You are correct that populating Slug in AlreadyProvisioned() cannot redden anything, because BootstrapAdminCliCommand.cs:52-61 returns from its fixed-message branch before reading that field. I had published that mutation in the PR body as verified-red. It is not — I ran it after your comment and it survives (6 passed, 0 failed). I had repeated the implementer's report without executing it. That row is corrected in a comment on the PR rather than quietly edited away.
The replacement mutation is one that can actually fire: make the already-provisioned branch print the slug and populate it. That reddens Rerun_WhenAlreadyProvisioned_DoesNotPrintTheFarmCode (1 failed / 5 passed), verified by me. Restored, 11 passed.
Correction: one mutation row in the PR body is false. I did not run it.The body claims: | make That is wrong. The mutant survives. I ran it: It cannot redden, for the reason codex gives: I published that row as driver-verified. It was not — I repeated the implementer's report without executing it, which is exactly the thing I claim to guard against. The other mutation rows I did run, and they stand; this one did not. Both codex findings are real. Verified.P2 #1 — vary the stored slug. Correct, and sharper than my own runbook instruction. I required the expected value be read from the database rather than a literal, and it is — but every test database carries the same migration-seeded P2 #2 — ban the value, not one phrase. Correct on both counts: the assertion bans only the exact text These two share one root cause worth naming: both guards are aimed at a fixture that never varies. That is what let a "read it from the DB" instruction still produce a test that cannot fail. Also being addressed, from the invariants review
Fixes for all of the above are in progress. Treat the branch as not-ready until they land and every reviewer has answered on the same head. |
…nst a varied slug (#589)
|
@codex both of your findings are addressed in P2 #1 (vary the stored slug) — the test now updates the account's P2 #2 (ban the value, not the phrase) — the re-run test now resolves the real slug and asserts that value is absent from the whole output. And you were right that the mutation named for it could not fire: Also in this commit, from a second reviewer: Two things worth your attention on re-review:
Full suite at this head: 1943 passed, 0 failed. Build |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…rrect a guard comment (#589)
|
@codex re-review at
Same commit corrects a comment of mine on Worth your attention:
Suite at this head: 1943 passed, 0 failed. Build 0 Warning(s) 0 Error(s). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a892f62a36
ℹ️ 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".
| await Console.Out.WriteLineAsync( | ||
| $"First-run admin provisioned: {outcome.Email} (account {outcome.AccountId})."); | ||
| $"First-run admin provisioned: {outcome.Email} on farm {outcome.Slug} " | ||
| + $"(account {outcome.AccountId})."); |
There was a problem hiding this comment.
This changes operator-visible behavior, but the required documentation surfaces remain stale: web/src/i18n/en.ts's signingInFirstRun says the setup command prints only a temporary password, its first-run/break-glass glossary text omits the printed code, and specs/product/GLOSSARY.md likewise describes these commands without this output. Update the SPA Help, in-app glossary/localized packs, and product glossary to cover the farm code emitted by both provisioning and recovery.
AGENTS.md reference: AGENTS.md:L198-L198
Useful? React with 👍 / 👎.
|
@codex re-review at Your P1 (stale docs surfaces) — you were right, and it corrected a call I had made wrongly. I had argued for skipping the Help copy on the grounds that Two more false claims, both introduced by this PR while fixing other false claims:
One production change worth your eye: Suites at this head: .NET 1943 passed / 0 failed; web 1867 passed; both builds Note on the roster: pi's verdict this round is discarded, not counted — its working directory's parent held my own fix runbooks and it read them, so it restated my findings rather than reaching them. That is my setup error, and I am recording it as a non-verdict rather than as a fourth confirmation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9454d23b3
ℹ️ 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".
| signingInFirstRun: | ||
| "<strong>First sign-in on a brand-new farm.</strong> There is no default password — an operator runs a " | ||
| + "one-time setup command that prints a temporary one. Sign in with it and you'll immediately land on a " | ||
| + "one-time setup command that prints a temporary password and the farm code. Sign in with both and " | ||
| + "you'll immediately land on a " |
There was a problem hiding this comment.
At f9454d23, this edit fixes only the first-run Help copy; fresh evidence is that HelpPage still renders the unchanged auditSystemActors text (en.ts lines 2629-2634, with corresponding es/tl entries), which describes break-glass without its newly printed farm code, while glossaryFarmCodeDef (lines 2865-2872 and translations) still omits both CLI discovery/recovery sources. Consequently the new recover-admin behavior remains undocumented in Help and the required in-app glossary surface was not updated; update both keys in en/es/tl.
AGENTS.md reference: AGENTS.md:L198-L198
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half accepted in 351d47c3, half rejected with evidence. Taking them separately because they are different claims.
glossaryFarmCodeDef — accepted, but not as suggested. The gap is real: a user who does not know their farm code gets no guidance from the entry that defines it. But the fix you propose — document the "CLI discovery/recovery sources" — is wrong for this surface. The in-app glossary is read by SPA users, who have no shell; they cannot run list-accounts or see bootstrap-admin's output. Telling them to would be a new false implication, which is the exact defect class this PR has spent three rounds removing.
The sentence added instead: "If you do not know your farm's code, ask your administrator — they can look it up." Mirrored into es and tl with matching meaning. The operator-facing side of that answer is already documented where operators read it — both runbooks now name list-accounts.
auditSystemActors — rejected. That copy explains what audit-log actor names mean: (break-glass) is the emergency command, and the entry records the machine and the reason given. It makes no claim about what any command prints to stdout, so nothing in it became false when recover-admin gained an output line. Adding CLI output details to an audit-log explainer would put information where nobody is looking for it, and would be a change made to satisfy a completeness check rather than to correct an error.
If you think there is a specific sentence in auditSystemActors that is now untrue, quote it and I will fix it — I have been wrong about a doc call on this PR already, when I argued for skipping the Help copy on reachability grounds and you were right that truth, not reachability, is the test. This one I think is different: the copy is accurate, just silent about something outside its subject.
Web suite at this head: 86 files / 1867 tests, build clean.
…589) codex asked for the in-app glossary to document the CLI discovery and recovery sources for the farm code. Half accepted. The gap is real: a signed-out user who does not know their farm code gets no guidance from the glossary entry that defines it. But the suggested fix is wrong for this audience — the in-app glossary is read by SPA users, who have no shell and cannot run `list-accounts` or read `bootstrap-admin` output. Pointing them at CLI verbs would be a new false implication, which is the exact defect class this PR has spent three rounds removing. The sentence added instead points them at the person who can look it up. Mirrored into es and tl with matching meaning. The other half of the finding is rejected with evidence, on the thread: `auditSystemActors` explains what audit-log actor names mean and makes no claim about what those commands print, so nothing in it became false.
|
@codex re-review at Your last P1 is half addressed, half rejected with evidence; both are answered on that thread. Summary: This is the last round I plan to run, so if anything is still wrong, this is the moment. Four rounds have produced two product defects (the
Suites at this head: .NET 1943 passed / 0 failed; web 1867 passed; both builds |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 351d47c36f
ℹ️ 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".
| // it (the read-only `list-accounts` verb can recover it later if lost); | ||
| // the account GUID is diagnostic only. Without this line the command ends | ||
| // with "log in with this now" and they cannot: #532 changed the login | ||
| // contract (farm code became a required input) and this caller was | ||
| // missed. |
There was a problem hiding this comment.
This comment still overstates the dependency on this output: without the new line the operator can recover the code with list-accounts, as the preceding sentence itself explains, so the user is not unable to sign in. The farm code is also not necessarily a field they must type because the SPA can fill it from a remembered farm or /login?farm=<code>. Rephrase this as avoiding an extra recovery step rather than being the only workable sign-in path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fcce1041. You were right, and the contradiction was in a single comment block four lines apart.
It said list-accounts can recover the code, and then that without this output line the operator "cannot" sign in. Both cannot hold. Rephrased to what it actually is: printing the code here removes a recovery step, it is not the only route. Your second point is also recorded — the SPA can supply the code from a remembered value or a /login?farm= link once either exists, which on a first run neither does, so this output remains the practical source without being the exclusive one.
Worth stating plainly since it bears on how much weight to give the rest of this comment: this is the third correction to this one block. The first fixed a mis-scoped AGENTS.md #394 citation, the second removed a "no other way to discover" claim — and each introduced the overstatement the next one caught. Three rounds of my prose, on a change whose actual behaviour was correct from the first commit.
Suite at this head: 11 passed on the two affected classes; full suite 1943 passed / 0 failed; build 0 Warning(s) 0 Error(s).
This is the last planned change to this PR — the review loop was bounded to one final round by the owner, and that round is complete.
codex P3. The comment said `list-accounts` can recover the farm code and then, four lines later, that without this output line the operator "cannot" sign in. Both cannot be true. Rephrased as what it actually is: printing the code here removes a recovery step, it is not the only route. Also notes that the SPA can supply the code from a remembered value or a /login?farm= link once either exists — which on a first run neither does, so this output is still the practical source. This is the third correction to this one comment block. The first two each fixed an overstatement and introduced another.
2 participants
Closes #589. A first-run operator could not sign in:
bootstrap-adminprinted the email and temporary password, said "Log in with this now", and never printed the farm code — which #532 made a required login input.What ships
FirstRunAdminServicereads the default account's slug and carries it on the outcome;bootstrap-adminprints it:The slug is read from the account row, never hardcoded. The existing existence check already queried that table —
— so it now selects the slug instead. Same round trip, and a null result still takes the existing
Bootstrap.AccountMissingpath. The"default-farm"literal stays only in20260818235944_AddAccountSlug.cs; a second copy would drift.Two corrections to the issue, made before implementing
Both are on the issue with an amendment note; the body is left as written for history.
"No test asserts
bootstrap-admin's stdout" — false.BootstrapAdminCommandTests.cshas a stdout harness and five cases, and already assertedContains(email)andContains("Temporary password:"). The real defect is sharper: those are positive-Containsassertions with no completeness requirement, so when #532 added a required login input, nothing had reason to go red. Observed, not argued — the five existing tests pass whether or not the farm code is printed."Mirror
provision-account's shape" — the records are not mirrors. Diffed field by field:FirstRunAdminOutcomebool WasAlreadyProvisioned, string? Email, Guid? AccountId, string? TemporaryPassword— all nullable, has a no-op pathAccountProvisionOutcomeGuid AccountId, string Slug, string OwnerEmail, string TemporaryPassword— all non-null, no no-op pathSo
Slugis added asstring?. Copying the non-nullable version across would have made theAlreadyProvisioned()branch unrepresentable.A defect in the runbook, and the guard that caught it
Adding
accountSlugtoProvisionUnderLockAsyncbroketenant-bypass-allowlist.json:148, which pins that method's exact symbol signature — the enumerating guard from #536. The signature change was authored without grepping for readers of the symbol, so the allow-list was not in the implementer's permitted-edit set. The implementer correctly refused to touch it and flagged the decision rather than acting unilaterally.The entry now matches; its justification is unchanged, because the reason for the bypass is unchanged (#283: first-run bootstrap runs at an unresolved tenant by design).
Verified the entry was corrected rather than the guard widened: renaming the parameter reddens
TenantBypassagain (1 failed / 15 passed), restoring is green (16 passed).Verification — driver-run
dotnet build Cluckwork.sln→0 Warning(s) 0 Error(s)BootstrapAdminCommandTests5 → 6Mutations — one per path the spec names
on farm {outcome.Slug}from the CLI outputNot found: "on farm default-farm "AlreadyProvisioned()populateSlugso the idempotent path prints itaccountSlugparameterTenantBypassguardNo survivors. Each restored and rebuilt green.
Note what the first mutation also shows: the pre-existing assertions stay green under it. That is the hole this issue documents, visible in the run.
Docs — deliberately no change, with the reason
AGENTS.mdand the first-run decision record carry no sample output to update.The SPA Help page is deliberately not touched.
HelpPagesits insideProtectedRoute(App.tsx:43,:70), so in-app Help is unreachable until after sign-in — and the farm code is what you need in order to sign in. Adding it there would help nobody at the moment of need. The concept is already defined inspecs/product/GLOSSARY.mdand the in-app glossary (#535), and this slice introduces no new concept: it changes operator-facing CLI output, not SPA copy. Recording this so it is not re-raised as an oversight.Scope
Rebased onto
58d30568, which independently fixed two pre-existingSchemaDocsimage-pin failures the implementer had correctly identified as not caused by this change.