feat(web): key the farm brand palette per farm (#586) - #600
Conversation
Driver verification (Phase 11) — head
|
| Command | Result |
|---|---|
npm run test:coverage |
exit 0 — Test Files 87 passed (87), Tests 1932 passed (1932) |
npm run build |
exit 0 |
npm run verify:sw |
exit 0 |
Suite-count ledger: baseline on 577d94e5 was 1900; head is 1932; +32. No coverage threshold
error, so the src/lib/** lock (100% statements/lines/functions/branches) held.
git diff --stat 577d94e5..HEAD touches exactly the ten allow-listed files, 648 insertions / 39
deletions. grep -rn "MUTANT\|\[DEBUG-" web/src web/public → empty. Working tree clean after every run.
Mutations — 21 rows, 20 driver-verified
Every row below was applied by the driver, the named test observed, then restored and re-run green.
Failures were parsed from FAIL lines only.
| Row | Result |
|---|---|
| M1 | RED — returns null once the tab is bound to a DIFFERENT account (+ the no-binding case; one comparison, both cases, as specified) |
| M2 | RED — returns the slug while the account binding still matches |
| M3 | RED — stores nothing when there is no account to pin the slug to |
| M4 | RED — clearBoundAccount drops the farm binding with the account binding |
| M5 | SURVIVED — see below |
| M6 | RED — writes NOTHING when the tab has no proven farm |
| M7 | RED — applyBrand … caches it |
| M8 | RED — a failed slug write leaves the legacy fallback intact |
| M9 | RED — applyBrand drops a stale cache when the write fails but reads still work |
| M10 | RED — deletes the pre-#586 un-namespaced key once a farm has its own |
| M11 | RED — binds the typed farm code to the tab at login (1 failed, 22 passed) |
| M12 | Implementer-attested, NOT driver-verified — needs a temporary harness extension; the shipped fixture's farm code is already canonical |
| M13 | RED — forgetting a farm removes its palette too |
| M14 | RED — forgetBrandFor removes that farm's palette AND the unattributable legacy key |
| M15 | RED — removes the palette even when the roster itself cannot be read |
| M16 | RED — matches the palette key canonically, like the roster entry |
| M17 | RED — a MALFORMED roster never reaches the legacy key either (the "" case) |
| M18 | RED — branch 1 has NO legacy fallback |
| M19 | RED — branch 2 falls back to the legacy key on a miss |
| M20 | RED — branch 4: two remembered farms and no ?farm= assert NOTHING |
| M21 | RED — an INVALID ?farm= falls through + the parity test |
M5, resolved: the guard is real, the row was wrong
M5 drops the typeof accountId !== "string" half of readBoundFarm's screen. No runtime test reddens,
because getBoundFarmCode's identity comparison rejects a non-string accountId anyway.
That looked like a dead guard — the exact defect class this issue exists to delete — so it was tested
rather than assumed. It is not dead: with the clause removed, npm run build fails with
src/auth/tokenStore.ts(121,12): error TS2322: Type 'string | undefined' is not assignable to type 'string'.
So the clause is pinned by the type checker, not by a test, and M5 named the wrong gate. A follow-up
increment will say so in the code, so nobody later "simplifies" a clause whose only guard is tsc.
Honest notes on the verification itself
- The first automated pass classified rows with a regex that matched passing test lines as well as
failing ones, which made every row look RED. It was caught by cross-checking M11 (real result: 1
failed, 22 passed) and every row was then re-run with a parser reading onlyFAILlines. The table
above is from that second pass. - Two rows initially "survived" against a mutation that did not mutate anything: one inserted a comment
instead of deleting a call, the other matched an identicaltry/catchearlier in the same file. Both
were re-anchored and re-run. Neither was a product finding.
|
@codex please review this branch. Context: it changes how a render-blocking pre-paint script chooses which farm's colour palette to apply on a multi-tenant SPA, and adds a per-farm localStorage key namespace. The isolation-critical parts are |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98277de952
ℹ️ 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".
| var urlSlug = canon(new URLSearchParams(location.search).get("farm")); | ||
| if (urlSlug !== null) { | ||
| b = localStorage.getItem("cluckwork.brand:" + urlSlug); |
There was a problem hiding this comment.
Document the per-farm palette behavior
This changes user-visible sign-in behavior: a farm link or single remembered farm now receives its own cached palette, while forgetting a farm removes that palette. The existing Help and in-app glossary still describe only the farm-wide setting (web/src/i18n/en.ts:2715-2724 and 3067-3070), and this commit includes no corresponding Help/glossary update; document the new persistence and Forget semantics in the SPA Help/in-app glossary and update the product glossary accordingly.
AGENTS.md reference: AGENTS.md:L198-L198
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 7ddb6a15. You were right and the omission was at design time — the design doc, plan and runbook never listed the documentation surfaces, so none of the four hand-briefed reviewers was looking for this.
Updated in the same PR, per AGENTS.md L198:
- SPA Help —
farmPaletteIntronow describes the per-farm, per-device persistence, the?farm=link case, the several-farms-means-default case, and Forget removing the colour with the code. - In-app glossary —
glossaryFarmPaletteDefcarries the short form. - Product glossary —
specs/product/GLOSSARY.md's Farm palette entry gains a paragraph. - es and tl shipped in the same commit, machine-drafted pending native review, per the repo's translate-now policy.
Existing keys were amended rather than added, so catalogParity.test.ts stays green — verified: 87 files, 1930 tests, exit 0 at that commit.
Review round 1 — 4 reviewers on head
|
| Finding | Disposition |
|---|---|
Branch 1 (?farm=) gets no upgrade-day fallback even when the URL names the sole remembered farm |
Moot — the fallback is being removed entirely |
'["a","a"]' (hand-edited) is not recognised as a single farm |
Fails safe, paints nothing. Pre-paint being stricter than the prefill is the safe direction |
| A comment called an alternative clause "unreachable" when it is merely redundant | Being corrected — a false property claim is its own defect in this repo, and this PR exists partly to delete one |
| A catch comment said a failure implies a device that "cannot write" | Being corrected — storage can permit writes and reject removals |
| "Stale" understates the restore-only case, where the cache is absent | Being corrected |
Round 1 yield: 2 confirmed defects in product/guard code. The stop-rule counter is therefore 0
consecutive zero-yield rounds. Fix increment is in flight.
Fix round 1 verified — head
|
| Gate | Result |
|---|---|
npm run test:coverage |
exit 0 — 87 files, 1930 tests |
npm run build |
exit 0 |
npm run verify:sw |
exit 0 |
Ledger: baseline 1900 → 1930 (+30). Lower than the previous 1932 because the fix deleted four
tests that pinned the legacy fallback — behaviour that no longer exists. Thresholds met, no markers,
clean tree.
Mutations, all driver-run
| Row | Mutation | Result |
|---|---|---|
| F1 | re-add the legacy fallback to branch 2 | RED — NEVER reads the pre-#586 un-namespaced key, even for a device with one farm |
| F2 | drop cluckwork.brand from UNSCOPED_KEYS |
RED — purges the pre-#586 un-namespaced farm palette |
| F3 | add /i to one copy of the slug pattern |
RED — the slug pattern is IDENTICAL to farmCodeCache's |
| F4 | empty forgetBrandFor's key list |
RED — forgetBrandFor removes only that farm's palette |
| F5 | add the slug key to UNSCOPED_KEYS |
RED — all three keeps the farm palette tests |
F3 is the one that matters. That exact mutation — a real divergence between the two copies of the
slug pattern — passed silently before this round. The guard now bites.
F5 exists because the fix re-pointed three pre-existing #149 tests at the per-farm key. A
re-pointed test is worth nothing if it now watches a key nothing touches, so F5 proves they still pin
persistence.
One thing the fix increment got wrong, and how it surfaced
Purging cluckwork.brand broke three pre-existing AuthContext.lifecycle tests that assert the farm
palette survives session teardown by reading that key. The fix increment's file list did not include
them — a driver omission. The implementer stopped, diagnosed the root cause correctly, declined to
touch the assertions without authorisation, and asked. It was then authorised to re-point them at
cluckwork.brand:<slug>, preserving the #149 guarantee while moving only its storage location. No
assertion was weakened; F5 is the proof.
|
@codex the legacy-key fallback you flagged is removed in 002e6f1 — the un-namespaced |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 002e6f1c7c
ℹ️ 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".
| const slug = getBoundFarmCode(); | ||
| if (slug === null) return; | ||
| const key = brandKeyFor(slug); | ||
| try { | ||
| localStorage.setItem(KEY, brand); | ||
| localStorage.setItem(key, brand); |
There was a problem hiding this comment.
Preserve farm attribution across asynchronous brand loads
When an account/settings request started under farm A resolves after the user logs out and signs into farm B, this reads B's current binding and caches A's response under cluckwork.brand:B. This is reachable through the post-save load() in SettingsPage.tsx, which awaits at line 352 before calling applyBrand at line 357, and generic API responses are not discarded when the auth generation changes. Capture the initiating farm/account before the request and reject a stale completion, or pass that attribution into the cache write, so a delayed response cannot recreate the cross-farm pre-paint this change is intended to prevent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c3f3eb8e (hardened again in c4b889f0). This was a real cross-farm write and none of my four hand-briefed reviewers found it — thank you.
applyBrand now takes the farm identity the value was fetched for, captured by the caller before its await, and applies nothing — neither attribute nor cache — when it no longer matches. All three call sites updated: FarmContext.refresh, FarmContext's seeded branch, and SettingsPage's post-save read-back. The parameter is required rather than optional, so a future post-await caller cannot skip the guard by omission.
Your follow-up on the null-binding case was right too: a slug alone could not express it, so the parameter is now an opaque token carrying a binding generation counter.
Pinned by mutation H2 — moving the capture to after the await, which is the original bug — reddening does not cache a superseded response under the new farm's key. Driver-verified, as is H1 and the J-series.
Review round 2 — head
|
| Row | Mutation | Result |
|---|---|---|
| G1 | applyBrand deletes the bare key on the unbound path |
RED — an UNBOUND tab leaves the pre-#586 un-namespaced key untouched |
| G2 | logout deletes the derived per-farm key |
RED — keeps the farm palette on logout |
Both of those mutations were invisible to the suite before this round.
Also cleared by mutation: the rewritten parity guard reddens for an /i flag on one side, a body change
on one side, and a rename of either binding; the three other deleted tests pinned only removed behaviour.
Gates at ae6b5bcb, driver-run
npm run test:coverage exit 0 — 87 files, 1931 tests. npm run build exit 0. npm run verify:sw
exit 0. Thresholds met, no markers, clean tree.
Stop-rule counter
Round 1 confirmed two defects in product/guard code. Round 2 confirmed none in product code — its
entire yield was tests. That is the first zero-yield round; a second consecutive one stops the loop.
|
@codex re-review please — head is now |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae6b5bcb5c
ℹ️ 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".
| // its own colour instead of reverting to the default. Since #586 that is no | ||
| // longer a cross-farm hazard — the pre-paint script reads a palette only | ||
| // for a farm this device can NAME (?farm=, or exactly one remembered code), |
There was a problem hiding this comment.
Reset the live palette when logout loses farm attribution
On a device with two remembered farms, logging out from farm A leaves the data-brand applied by FarmProvider on the document; apiLogout() only clears authentication/bindings, and ProtectedRoute performs a client-side redirect, so the pre-paint script cited here never runs again. The anonymous /login screen therefore remains in A's colour even though no farm is selected, and choosing farm B retains A's colour until B's account request completes, contradicting the documented multi-farm default and recreating the cross-farm paint this change is intended to prevent. Reconcile the live attribute with the anonymous farm selection on logout while preserving the per-farm cache.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c4b889f0. Logout now applies what the DEVICE can justify showing rather than leaving the previous farm's colour: the farm's own palette when exactly one is remembered — which preserves #149's deliberate choice for single-farm devices — and the default when several are, because at that point the app cannot say which farm the login screen belongs to.
Pinned by a new test (drops the palette on logout when the device remembers SEVERAL farms) alongside the existing single-farm one, and by mutation J3 (deleting the call reddens it). Driver-verified.
| // removal must not inherit that early return: "forget this farm" still has | ||
| // to leave no colour behind. forgetBrandFor never throws, so ordering it | ||
| // first cannot cost the roster write either. | ||
| forgetBrandFor(code); |
There was a problem hiding this comment.
Remove the forgotten farm's live palette immediately
When the only remembered farm supplied the current pre-paint colour, confirming Forget deletes its storage key and roster entry but never clears document.documentElement.dataset.brand. Because theme-init.js does not rerun during this in-page action, the login screen continues showing the forgotten farm's colour—and keeps it while another farm code is typed—until a reload or successful sign-in. Update the live anonymous palette after forgetting, rather than removing only the cached value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c4b889f0, same mechanism. Forget now recomputes the on-screen palette from the roster that remains, so forgetting the farm whose colour was showing drops it. Covered by a new case in Login.test.tsx.
Round 3 — the automated reviewer found a real cross-farm write. Fixed in
|
| Row | Mutation | Result |
|---|---|---|
| H1 | delete the guard in applyBrand |
RED — a response that outlived its farm applies NOTHING |
| H2 | move the capture to AFTER the await in FarmContext.refresh |
RED — does not cache a superseded response under the new farm's key |
H2 is the original bug, reproduced. It is the row that matters: it fails exactly when the capture
stops happening before the await.
Stop-rule counter: reset to 0
Round 2 yielded no product defect (tests only) — that was the first zero. Round 3 confirmed a product
defect, so the counter resets. The loop is still finding real bugs in code earlier rounds passed, which
is the productive regime, not the polishing one.
|
@codex the superseded-response race you found is fixed in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3f3eb8e83
ℹ️ 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".
| // farm B's key. Synchronous callers pass getBoundFarmCode() and the check is | ||
| // a no-op by construction. | ||
| export function applyBrand(brand: string, boundAt: string | null): void { | ||
| if (boundAt !== getBoundFarmCode()) return; |
There was a problem hiding this comment.
Track session identity when the farm binding is absent
On a fresh tab restored from its refresh cookie, getBoundFarmCode() is null, and an explicit logout leaves it null; therefore a /account or settings read started before logout passes this new equality check when it resolves afterward. React unmounting does not cancel the promise continuation, so FarmContext.refresh() or SettingsPage.load() can still reach applyBrand and repaint farm A's palette on the anonymous login screen. The c3f3eb8e guard needs an account/session-generation attribution in addition to the nullable slug so two different unbound states are not treated as the same session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c4b889f0, and you were right that a slug could not express it — my own reviewer found the same hole independently. applyBrand's guard now compares an opaque token from tokenStore that carries a binding generation counter alongside the slug, so a response that outlived a whole bind-then-unbind cycle no longer reads as current just because both sides are null.
Pinned by a response that outlived a whole bind-then-unbind cycle applies nothing, and by mutation J1: deleting the counter increment reddens exactly that test. Driver-verified.
Round 4 verified, and the review loop is stopped deliberately — head
|
| Row | Mutation | Result |
|---|---|---|
| J1 | delete the generation increment in bindFarm |
RED — a response that outlived a whole bind-then-unbind cycle applies nothing |
| J2 | length !== 1 → < 1 in applyDeviceBrand |
RED — falls back to the default for zero or several farms |
| J3 | delete the applyDeviceBrand call in logout |
RED — drops the palette on logout when the device remembers SEVERAL farms |
| J4 | make applyDeviceBrand write storage |
RED — applyDeviceBrand never writes storage |
J4 survived on its first run, and the fault was a test I wrote. It asserted localStorage.length,
which cannot detect a write to a key already present. Re-pinned on the call itself with a spy; J4 then
reddened. Reported here rather than quietly re-run, because a surviving mutant is a finding.
The loop is stopped, by owner decision, at 4 rounds
Not because the reviewer went quiet — it would not. Yield across the rounds:
| Round | Confirmed in product code |
|---|---|
| 1 | 2 — cross-farm paint via the legacy key; a vacuous parity guard |
| 2 | 0 — two vacuous tests only |
| 3 | 1 — cross-farm write via an async race |
| 4 | 3 — the live-attribute class (all transient, none persisted) |
Three of four rounds found real defects, so this stopped inside the productive regime rather than at
exhaustion. The owner made the call with that stated. No further @codex tag will follow — the tag
is what mints the next round, so withholding it is the mechanism, and the silence from here is
deliberate rather than an unfinished response.
Anything a further round would have found is, on the trend, in the same class as round 4's: transient,
in-session, corrected by a reload.
|
The review loop is deliberately closed at head Checks
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1030a6846
ℹ️ 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".
| // #586 — attribution is gone, so the palette becomes what the DEVICE can | ||
| // justify: this farm's colour if it is the only one remembered (#149's | ||
| // deliberate choice, preserved), the default when several farms are. | ||
| applyDeviceBrand(readFarmCodes()); |
There was a problem hiding this comment.
Reconcile the palette on forced sign-out
The explicit-logout fix does not cover the separate setOnUnauthenticated teardown at lines 69–74. When a refresh is exhausted on a device remembering several farms, that callback redirects to /login without applying the device-default palette, so the authenticated farm's colour remains visible; it also leaves farmBindingToken() unchanged, allowing a late /account response to repaint it after any reconciliation. Apply the same anonymous-palette policy on forced sign-out and invalidate outstanding brand reads without discarding the account binding that refresh adoption relies on.
Useful? React with 👍 / 👎.
Closes #586. Slice T7b of epic #530 (Phase 1.6 — multi-farm tenancy).
What was wrong
cluckwork.brandwas one un-namespaced localStorage key holding one palette id for the whole device,read by
theme-init.js— a render-blocking script that runs before React and before any auth tokenexists. On a device holding several farms, the last farm's palette painted the next farm's login screen.
The GUID namespacing #535 uses is structurally unavailable at pre-paint: a fresh tab with a live refresh
cookie has no token yet. Keying by GUID would have painted the default and snapped to the farm colour on
every cold start, regressing #149's no-flash guarantee for the single-farm majority.
What ships
Pre-paint resolves the farm from what exists before a token does. Three branches, first match wins:
?farm=<slug>present and canonicalcluckwork.brand:<slug>cluckwork.brand:<that slug>The pre-#586 un-namespaced key is never read, and is purged once at startup. Its value cannot be
attributed to any farm, and every rule tried for attributing it failed — see below.
Write side caches only under a slug the current session's login typed.
cluckwork.boundFarmholds{accountId, slug}in sessionStorage as one record, andgetBoundFarmCode()returns the slug only whilethat
accountIdstill equals the tab's bound account. A fresh tab restored from the refresh cookie has nobinding: it paints the palette and caches nothing.
Forget (#587) removes that farm's palette with its roster entry.
The decision that took four review findings to reach
The first design kept the un-namespaced key as a pre-paint fallback, to avoid a cold-start flash on
upgraded devices. Reviewers broke that three times — as
"[]"(an emptied roster is not a single-farmdevice), as a shrinking roster (Forget makes "one remembered farm" stop meaning "one farm"), and finally
through history: a device that used Forget on the old build shrank its roster while leaving the key
untouched, so after the upgrade branch 2 would paint the forgotten farm's colour. Reproduced directly:
No lifetime rule can reach that, because the key's provenance predates every mechanism available to us.
So it is purged and never read — which is what issue #586 originally specified. Keeping it was a
driver decision and it was wrong.
Accepted limitations
the purge, taken deliberately over painting one farm's colour on another's login screen.
absent or stale until someone types a login there. Wrong shade of the right farm; another farm's
colour is unreachable.
remembering one, so "never paints farm A's palette for farm B" is unachievable without a farm
identifier in the URL. The shipped guarantee is: never paints a palette for a farm the device still
remembers.
Verification
Driver-run gates and 21 mutation rows are in the review thread, including the two rows that prove the
fix: one re-adds the legacy fallback (the regression test must redden), one adds an
/iflag to a singlecopy of the slug pattern (the parity guard must redden — it was vacuous before this PR's second round and
silently missed exactly that divergence).