Repository navigation
feat(accounts): add Account.Slug (farm code), suspend/reactivate, list-accounts verb (#531) - #560
Conversation
…t-accounts verb (#531)
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
@codex please review. Slice T1 of #530: adds Account.Slug (farm code, unique index, md5-derived backfill), Suspend()/Reactivate() with Version++ concurrency token, and a read-only list-accounts OneShot CLI verb (IgnoreQueryFilters cross-account read on #536's allow-list). Migration AddAccountSlug backfills default→'default-farm', others→'farm-<12 hex md5(id)>'. Please scrutinise the migration backfill (data-corruption class), the Version++ race guards, and #394 write-contract caller coverage for the new required Account.Create slug param. |
![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: b001231b71
ℹ️ 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".
|
|
||
| foreach (var account in accounts) | ||
| await Console.Out.WriteLineAsync( | ||
| $"{account.Slug}\t{account.Name}\t{(account.IsActive ? "active" : "suspended")}"); |
There was a problem hiding this comment.
This slice makes the farm code user-visible through list-accounts and introduces it as a new account concept, but it updates neither specs/product/GLOSSARY.md nor the SPA Help page/in-app glossary. Add the required documentation in this change so operators and users have a canonical explanation of the code, its format, and its role.
AGENTS.md reference: AGENTS.md:L191-L191
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 346e4c4. Added Farm code (account slug, #531) and Account status — active / suspended (#531) to specs/product/GLOSSARY.md (the concept, its format, immutability, the reserved list, default-farm, and list-accounts).
On rule (2) — SPA Help page + in-app glossary — deferred deliberately, not skipped: this slice has no SPA surface (issue #531 is explicit: "no tenant-facing endpoint reads or writes the slug yet; no SPA change"; list-accounts is an operator CLI verb). AGENTS.md:191 rule (2) is scoped to "user-visible behavior" through the SPA, which the farm code first gains at #532 (login by farm code) — that is the PR that adds the Help/in-app-glossary text, in the same change as the UI. Adding SPA Help copy now would either describe a screen that does not exist or expand this slice past its stated scope.
|
Review round 1 — 0 confirmed product defects.
Both reviewers independently flagged the same non-defect: the backfill slug Driver-side full verification: |
|
@codex review — the farm-code documentation P1 is addressed in 346e4c4: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 346e4c4e31
ℹ️ 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".
|
|
||
| foreach (var account in accounts) | ||
| await Console.Out.WriteLineAsync( | ||
| $"{account.Slug}\t{account.Name}\t{(account.IsActive ? "active" : "suspended")}"); |
There was a problem hiding this comment.
When a farm admin submits a name containing tabs, newlines, or ANSI escape characters through PUT /settings, the current name validator accepts it and this interpolation writes it verbatim. Running list-accounts can therefore produce ambiguous rows or allow one tenant's name to erase or rewrite terminal output, obscuring another farm's code or status; escape control characters or emit a structured/encoded format before printing the name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dcec20e. Confirmed the finding first: the account name's write validator (UpdateFarmSettingsValidator) bounds only length — no control-char rejection — so a name can carry CR/LF/tab/ANSI, and list-accounts printed it verbatim.
ListAccountsCliCommand now runs the name through SanitizeForDisplay before printing — the same char.IsControl→space strip ClientErrorEndpoints already applies to its rendered fields against log forging. Slug is control-char-free by its regex and IsActive is a bool, so only Name needs it; sanitising at render (not rejecting at write) matches the repo's existing precedent and keeps this slice from touching the shipped settings validator.
Tests: a unit theory over SanitizeForDisplay (tab/CR-LF/ANSI → space) and a subprocess test that writes row-a\u001b[31m\trow-b\nFORGED-ROW straight to the column and asserts the output carries no raw ESC and no forged standalone line. Mutation-checked: dropping the SanitizeForDisplay wrapper reddens the subprocess test (the unit theory stays green, so the two are isolated). Build 0/0, list-accounts suite 7/7.
|
@codex review — the P2 control-character finding is fixed in dcec20e: |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
…r code (#732) Review round 1, F4. TryValidateSlug's description quotes the rejected value, which is raw argv: `--new-slug "bad<newline>code"` printed a two-line stderr, breaking the verb family's one-line contract and letting the second line forge whatever an operator's terminal reads next. The value now goes through ListAccountsCliCommand.SanitizeForDisplay -- the same char.IsControl strip #560 applied to the tenant-controlled farm name. Scope is this verb only: provision-account carries the same older defect and is deliberately left alone rather than widened into this diff.
## Why `docs/images/dashboard.png` is the one README image #865 could not refresh. Its capture fails the spec's own guard (`screenshots.spec.ts`, bar heights must vary) because on the simulation fixture every day in the window is a **partial** day: the fixture seeds ~100 catalog flocks for the picker (#627) which are placed, active and never file, so every day owes a count nobody filed, `DayStripData.max` is null, and all fourteen bars render as the 2% floor stub. #865 states that gap and leaves the image at its 2026-09-02 state, which predates #781's bar strip and #791. The product rule is right and the fixture's counts are pinned by the picker-paging specs, k6 and the e2e suite. So the fixture stays as it is and the **capture** moves: the sim stack now carries a second, small farm seeded with the demo profile, and the dashboard image is taken from that. Closes nothing — no issue exists. It follows from #865's stated gap. ## Scope **`seed --profile demo --farm-code <slug>`** (`SeedCliCommand`, `DemoDataSeeder`). The code is resolved by slug exactly as `rename-account` and the lifecycle verbs resolve theirs (`AccountSlugLookup`), after the migrate and before the seed; an unknown code exits 1 naming `list-accounts`. `DemoDataSeeder.SeedAsync` takes the target account as an explicit `Guid?` parameter, never ambient state — `TenantContext` is single-assignment, so a seeder reading the tenant instead of setting it could only run where somebody else had already resolved one. Default behaviour is unchanged: every existing caller passes nothing and gets `SeedDefaults.AccountId`. `--profile simulation` **refuses** the flag: its manifest, its cast emails and the counts k6 and the e2e suite pin are all default-farm facts, so honouring it would need a second decision, not a parameter. Two things came along with that file. Every stderr path in the verb now routes through one sanitizing sink (#560) instead of two of five, because the shape where only the messages that quote argv get fixed is the shape `rename-account` was corrected out of. And the demo seeder's prerequisite messages no longer say "the default account", which stopped being true. **The sim harness** (#370 — all three files considered, and it says so below). `reset.sh` provisions `readme-farm` with `provision-account`, rotates its Owner off the printed one-time password onto a stable one, demo-seeds it, and preflights that the farm is signable and holds the demo fixture's three flocks. The timezone passed is `Simulation__TimeZoneId`, not a literal, so the two farms on one stack cannot end up on different clocks. - The stable password is generated in `bootstrap.sh` beside `SIM_ADMIN_PASSWORD` and read back by `reset.sh`, **mirroring the existing pattern** rather than minting one in `reset.sh`. Generating it in `reset.sh` would produce a new credential on every reset and leave `.sim-cast.json` describing the previous one. - It lands in `.sim-cast.json` under a top-level `readmeFarm` key, outside the `cast` array, because every entry there signs into `default-farm` and a driver iterating the cast must not have to ask which farm each member belongs to. - The rotation block is now **one shell function with two callers** rather than two copies of a credential-rotation block. - Re-running converges. `reset.sh`'s own flow never reaches the already-exists branch (`down -v` ran at the top), but `provision-account`'s duplicate behaviour is *not* a no-op like `bootstrap-admin`'s — it exits 1 with `Provision.SlugTaken*` and prints no password — so the branch checks the stable credential still signs in and carries on, and fails loudly on any other failure. - `verify-harness.sh` fails closed on a missing or blank `README_*` value and on a cast file that predates the `readmeFarm` key. - **`docker-compose.sim.yml` needs no change**, and that is a considered answer rather than an omission: the `README_*` vars carry no `__`, exactly like `SIM_ADMIN_*`, so they are script-level values `reset.sh` greps out of `.env.sim` and never app configuration. Nothing new reaches the container's `environment:` block. For the same reason there is no `src/Cluckwork.AppHost/Program.cs` change under #565 — no new required config key exists. **The e2e suite.** `cast.ts` exposes `readmeFarmOwner()`, whose return type widens `farmCode` from optional to required; `signIn` and the API sign-in helper take the code from the member, falling back to `default-farm`, so every persona written before this one is untouched. Only the dashboard capture uses it. **Docs.** `AGENTS.md`, `tools/simulation/README.md` ("Two farms on this stack" + the `.env.sim` parameter row + the `reset.sh` chain), `tools/simulation/ui/README.md`, and the dev-database runbook. **No GLOSSARY or Help change**, deliberately: no user-visible concept changed — the flag is an operator CLI argument and the second farm exists only inside the sim harness. ## Blast Radius `seed --profile demo` with no flag behaves exactly as before, which is what every existing caller does. `--profile simulation` gains one refusal on an argument nothing passes today. Per #394 the write contract is unchanged, so no caller under `tools/simulation/k6/` or `specs/` needed a change; the one Playwright caller that did (`signIn`) is in this diff, and `session-races.spec.ts`'s own hardcoded `default-farm` is correct as written because it drives a sim-cast persona. The `readme-farm` account exists only in a throwaway `cluckwork-sim` database. A regenerated `.env.sim`/`.sim-cast.json` is required — run `bootstrap.sh --force`, then `reset.sh`; `verify-harness.sh` says so by name if you forget. Two capture fixes ride along. The #780 readout assertion moved **below** the screenshot, because focusing a day leaves a focus ring and a readout balloon that `capture()`'s blur does not dismiss, and the first capture published both. And the `1280x1180` frame is now held open by the **Owner's sidebar** (its content ends at 1164px, measured on the rendered page) rather than by the main column, which on this farm ends at 700px — anything shorter clips the navigation mid-list. The comment in `playwright.screenshots.config.ts` says so, because the visible empty space below the panels otherwise invites a shrink that breaks the sidebar. ## Verification Everything below ran in the worktree, against the real stack. - `dotnet build Cluckwork.sln` — 0 warnings, 0 errors. - `bash tools/simulation/bootstrap.sh --force` then `bash tools/simulation/reset.sh` — up, migrated, both farms seeded, all four preflights green, and the temporary password redacted on both provisioning paths (checked in the log). - `cd tools/simulation/ui && npm ci && npm run screenshots` — **4 passed**, including the dashboard capture whose guard fails on the simulation fixture. `npm run typecheck` clean. - `SeedCommandTests` (8, up from 5) plus `SimulationSeedCommandTests` — 11 passed. Registry readers found by grepping `CliDispatcher.Commands|ProcessRoles.OneShotVerbs` under `tests/` rather than from memory: `CliDispatcherTests`, `OneShotVerbMinimalConfigTests`, `ProcessRoleRegistryTests`, run with `DemoSeedTests` and `DemoSeedActorTests` — 29 passed. - Both `PostgresImagePin_IsOneIdenticalString*` guards run before the markdown was committed — 2 passed. - **Mutation-checked, not asserted.** Reverting the CLI's account routing to `SeedAsync()` turns `SeedCommand_Demo_WithFarmCode_SeedsThatFarmAndLeavesTheDefaultEmpty` red. Deleting `readmeFarm` from the cast file, and blanking its password, each fail `verify-harness.sh` with exit 1; so does a blank `README_OWNER_EMAIL` in `.env.sim`. - The new seed test runs against **its own Postgres**, not the class fixture's, and that is the assertion rather than tidiness: "nothing landed under the default farm" is only meaningful on a database no sibling `[Fact]` has demo-seeded, and xUnit guarantees no order within a class. - `dotnet test Cluckwork.sln` — result in a comment below. The `RealSourceTree_AllBypassesAreAllowListed` guard fired on the `SeedAsync` signature change, which is #632's registry working: the entry is keyed by the enclosing symbol including its parameters, so adding one demanded a re-read. The justification is re-written rather than re-pinned — the `AccountId` predicate on those pre-`tenant.Resolve` queries is now the caller's account rather than always `SeedDefaults.AccountId`, and the bypass is still what lets the preflight see the target farm at all. ## Two judgment calls worth a reviewer's eye 1. **The function is `readmeFarmOwner()`, not `readmeFarm()`.** It returns a persona, like `owner()` and `restrictedWorker()` beside it, and `readmeFarm()` reads as though it returns the farm. 2. **The farm name was truncated and is now fixed.** "Meadowlark Farm" ellipsised to "Meadowlark F…" at the sidebar's 244px; commit 71f95ad names the farm "Meadowlark" (`README_FARM_NAME` in `bootstrap.sh`) and recaptures the image after a full reset. The comment below carries the new capture. ## Screenshot Before and after below. Same screen, same 1280x1180 frame; the before is the committed image this PR replaces.   <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - The demo seed command can now target a specific farm with `--farm-code <slug>`. - Unknown farm codes return a clear error and guidance. - Simulation profiles explicitly reject the farm-selection option. - **Bug Fixes** - Sign-in and simulation tooling now correctly support members of non-default farms. - **Documentation** - Updated runbooks and simulation guidance describe multi-farm seeding and screenshot workflows. - **Tests** - Added coverage for targeted seeding, invalid farm codes, and unsupported simulation options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: mforce <cleyva@clvc.net>
Slice T1 of epic #530 (Phase 1.6 — multi-farm tenancy). Adds a stable, typeable
farm code and the suspend/reactivate lifecycle; ships a read-only
list-accountsCLI verb so the first upgraded deployment can discover its farm code before #532
makes it mandatory. No SPA change, no tenant-facing API change.
What
Account.Slug— lowercase^[a-z0-9][a-z0-9-]{1,30}[a-z0-9]$, storedalready-normalized so a plain unique index suffices (no
lower("Slug")expression index). Reserved-slug list rejected at creation. Invalid/reserved
slug is an invariant violation (
Account.Createthrows, likeFlock.Create).Account.Suspend()/Account.Reactivate()— each bumpsVersion(the EFconcurrency token), so a concurrent writer loses with a
DbUpdateConcurrencyExceptionrather than a silent overwrite.IsActiveenforcement lands in Auth: login by farm code, per-account email identity, immediate suspension #532; this slice adds the mutations only.
AddAccountSlug— add nullable → backfill →SET NOT NULL→unique
IX_Accounts_Slug. Default account →default-farm(documented, Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537);every other pre-existing row →
farm-<12 hex of md5(id)>. Deliberately not anid-prefix literal: the default and
SimulationDataSeeder.SecondAccountIdsharetheir first 8 id hex, so a prefix slug would collide. The unique index is the
fail-loud uniqueness assertion.
list-accounts— run-then-exitICliCommand,ProcessRole.OneShot(auto-classified via
CliDispatcher.Commands), not environment-gated. Readsacross accounts with a deliberate
IgnoreQueryFilters()(Cross-tenant isolation hardening: enumerating IgnoreQueryFilters guard + two-farm end-to-end matrix #536 allow-list).Tests
hyphen, uppercase, reserved word),
Version++on suspend/reactivate.conflict); migration backfill (default →
default-farm, a pre-existing secondaccount → a distinct valid slug, unique index rejects a duplicate);
list-accountsprints every account across the tenant filter.Account.Creategained a requiredslug; every caller(seeders, fixtures, domain + integration tests) updated.
MigrationSecurityReviewTests,CliDispatcherTests,OneShotVerbMinimalConfigTestsupdated/green;docs/schema/regenerated.Verify
dotnet build Cluckwork.sln·dotnet test Cluckwork.sln·tools/schema-docs/generate.sh --checkCloses #531.