feat(web): prefill the farm code from ?farm= and remember it (#535)

[mforce]

Closes #535. Slice T7 of epic #530 (Phase 1.6 — multi-farm tenancy). Frontend only — no API change, nothing under src/.

What ships

Prefill order on the login form, first match wins: ?farm=<slug> (validated) → the device's remembered codes (one prefills, several offer a picker) → empty.

web/src/auth/farmCodeCache.ts (new) — cluckwork.farmCodes, most-recent-first, capped at 10. Deliberately not account-namespaced: this list is the cross-farm roster, so keying it per account would be circular. Written only after apiLogin resolves, which is what makes "a typo is never remembered" true by control flow rather than by a check.

web/src/lib/accountStorage.ts (new) — per-account browser state, namespaced by the account GUID the server minted into the access token (getBoundAccountId()), not by the farm code the user typed. A read with no bound account returns null and deliberately does not fall back to the bare key — falling back is precisely how farm B inherits farm A's state. cluckwork.lastFlockId moves onto it; the pre-namespacing value is purged rather than migrated, because the app cannot attribute it to a farm.

Normalisation before validation. AccountRepository.cs:44 looks the slug up as (slug ?? "").Trim().ToLowerInvariant() and LoginRequestValidator carries no pattern rule by design. So Sunny-Acres and " sunny-acres " sign in successfully — and without normalising first they would have failed the client regex and been silently never remembered, with no error and a green suite. The picker would simply never have appeared for anyone whose keyboard capitalised the first letter.

Client-side slug validation mirrors Account.SlugPattern and is applied to ?farm= and to every value read back out of the cache. Anchored, so an over-long value is ignored rather than truncated. It deliberately does not mirror ReservedSlugs — that list stays server-side, a reserved code is rejected at login, and a success-only cache can never contain one. The claim is bounded to shape, not acceptability.

A visible notice when the code came from the URL. A same-origin ?farm=attacker-farm link would otherwise silently replace the operator's own farm code in a field they did not type, while the password manager autofills for the origin. Naming the farm makes the substitution visible. Not a complete fix — the autocomplete username is still not farm-qualified (#585).

Scope decisions

• No ADR here. Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537 owns the multi-farm ADR and its scope already lists the farm-code-cache disclosure. Writing it here would ship a version Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537 has to reconcile. A comment on Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537 records the two items this slice adds.
• Docs are an amendment. Auth: login by farm code, per-account email identity, immediate suspension #532 already shipped the Farm code glossary term and Help copy; this extends them with the picker/prefill behaviour and the accepted disclosure.
• cluckwork.splashShown needed no change — already tab-scoped and cleared on every login. Audited, recorded, untouched.
• The farm brand was deliberately left alone and split to SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586. theme-init.js is render-blocking and runs before any token exists, so the account GUID is structurally unavailable to it; a GUID-keyed brand would have regressed Per-farm theme — curated accent palettes in farm settings (API + SPA) #149's no-flash guarantee on every cold start for the single-farm majority.
• Follow-ups filed: Auth: farm-qualified autocomplete username on the login form #585 (farm-qualified autocomplete), SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586 (per-farm brand), SPA: let an operator forget a remembered farm code #587 (forget a remembered farm code).

Two comments corrected

brand.ts:5-8 claimed the palette "is cleared on every path that ends a session, so farm A's colour never bleeds into farm B's login screen." Nothing clears it — AuthContext.tsx:118-121 says so explicitly, justified by a "single-farm deployment" premise this epic falsifies. The repo was asserting the exact isolation property this slice was built to provide. Both comments now state the truth.

Verification

Driver-run, not quoted from the implementer — its report was lost to a repetition loop, so nothing here is implementer-attested.

• npm run test:coverage — 86 files, 1857 tests, exit 0, no threshold violations.
• Suite ledger: 1817 → 1857 (+40), 84 → 86 files. Nothing shrank.
• src/lib/accountStorage.ts and src/auth/farmCodeCache.ts both 100/100/100/100 (json-summary), so the src/lib/** high-water lock holds. No coverage floor was moved in either direction.
• npm run build and npm run verify:sw clean.

Mutation checks — 8 run, 8 killed by their named test

#	Mutant	Named test that went red
M1	drop canonicalFarmCode from the ?farm= initialiser	the 6 invalid-?farm= cases, on toHaveValue
M2	remove the urlFarmCode === null gate on the cache read	"never consults the farm-code cache"
M3	picker onClick always fills rememberedCodes[0]	the second-button click case
M4	delete write-side .slice(0, MAX_REMEMBERED)	"write-side cap: most-recent 10 of 12 in raw storage"
M5	delete write-side dedupe .filter	"write-side dedupe + most-recent-first in raw storage"
M6	delete try/catch from readAccountScoped	"bound + getItem throws — spy called with the namespaced key"
M7	revert Daily Entry to the bare un-namespaced key	"reads the namespaced key, never the bare key"
M8	move rememberFarmCode above the await	"writes nothing when apiLogin rejects"

Each restored, touched, and re-run green.

M4 and M5 are the ones worth noting. readFarmCodes() independently re-caps and re-dedupes, so any write-side test that read back through it would have stayed green under both mutants — the cap and dedupe would have been freely deletable. Those two tests assert on the raw stored value specifically for that reason.

Likewise M7: DailyEntryPage.tsx:161 already membership-checks the remembered id against the farm-scoped flock list, so a UI-level assertion cannot see the namespacing at all. Confirmed empirically — all 58 pre-existing Daily Entry tests pass with the change applied. That test asserts on the storage key.

Known, not fixed in this PR

• accountStorage.test.ts calls spy.mockRestore() after its assertions, so a failure in one of the three storage-throws tests leaks a throwing spy into later tests in the file. Harmless while green; produces misleading collateral failures when something breaks.
• brand.ts's replacement comment describes SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586 as "clearing the palette on every session-ending path". SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586's actual approach is slug-keyed namespacing — clearing was rejected because it would break Per-farm theme — curated accent palettes in farm settings (API + SPA) #149's device-persistence.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e7648013a

ℹ️ 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]

Update the in-app glossary for farm-code remembering

This updates the Help text and product glossary for the picker and ?farm= prefill, but the in-app glossaryFarmCodeDef at lines 2865–2868 still describes only typing the code and omits both new behaviors. Update that glossary entry, including its translated counterparts, so the user-facing documentation remains consistent.

AGENTS.md reference: AGENTS.md:L198-L198

Useful? React with 👍 / 👎.

[mforce]

Addressed in 73f6379f. You were right and this was a miss against the repo's own rule, not an oversight in a grey area.

AGENTS.md's docs-in-sync directive covers specs/product/GLOSSARY.md and the SPA Help page and the in-app glossary. This PR updated the first two — the product glossary and the signingInMultiTabResync Help copy — and left glossaryFarmCodeDef describing only typing the code, while quoting that same directive in its own planning notes.

glossaryFarmCodeDef now covers both new behaviours: that the sign-in screen remembers each farm code successfully used on the device and offers them, and that a /login?farm=<code> link fills it in. Mirrored into es.ts and tl.ts with matching meaning, not just matching keys — the parity test enforces presence, not semantics, so the translations were checked by reading them.

[chatgpt-codex-connector]

Serialize farm-roster updates across tabs

When two tabs complete successful logins for different farms concurrently, both can read the same existing roster here and then independently write their own updated array; the last setItem wins and silently drops the other successfully used farm code. Since multi-farm sessions explicitly coexist across tabs, make this read-modify-write operation cross-tab-safe rather than relying on separate atomic localStorage calls.

Useful? React with 👍 / 👎.

[mforce]

Addressed in 73f6379f. Confirmed as a real defect, not mitigated — thank you.

rememberFarmCode now serialises its read-modify-write with the Web Locks API, following the existing AUTH_COOKIE_LOCK precedent at client.ts:402 rather than inventing a mechanism — same degradation stance for browsers without navigator.locks (older Safari, insecure origins), which fall back to the unsynchronised path: no cross-tab guarantee, never worse than before. The roster is re-read inside the lock callback, since a read taken before acquisition is the stale value the lock exists to prevent. It never rejects: AuthContext.login now awaits it after apiLogin has already succeeded, so a storage or lock failure must not turn a completed sign-in into a thrown login.

On testing it honestly: the true cross-tab lost update is not expressible in Vitest — it needs two JS contexts sharing one localStorage, and jsdom has one, where the read-modify-write is synchronous and cannot interleave. A test claiming to reproduce the race would pass with the lock removed. So the guard is structural, and the first draft of it was itself a false green: asserting "storage still holds the old value when the callback is entered" survives hoisting the next computation outside the lock, because with an unchanged seed the pre-computed array and the in-lock re-read are identical.

The shipped test instead injects a concurrent tab's write into localStorage after the lock is acquired but before the callback runs, so only a genuine re-read preserves it.

Verification (driver-run, not attested):

mutation	result
remove the lock entirely, call write() directly	red — "re-reads INSIDE the lock callback: a roster change that lands while the lock is held is not overwritten"
hoist next outside write (lock still held, read now stale)	red — same named test

Both restored, touched, re-run green. Full gate: 86 files / 1866 tests, exit 0; src/auth/** at 98.75 / 94.64 / 100 / 100 against its 98 / 90 / 100 / 100 lock.

[mforce]

Review round 1 — 4 reviewers, 2 confirmed guard defects in shipped code

codex is unavailable (credits exhausted), so this round ran three Claude reviewers scoped to one defect class each, plus a local pi pass. Findings below are dispositioned; fixes are in flight as one increment.

Merge-blocking — both CONFIRMED BY EXECUTION, not by prediction

The false-green reviewer predicted these statically; I applied each mutation and ran the whole suite.

1. purgeUnscopedAccountState()'s call site is untested. Deleting the call at AuthContext.tsx:88 outright leaves 86 files / 1857 tests green. accountStorage.test.ts proves the function works but nothing proves anyone calls it — so the migration behaviour this slice ships is deletable today while that test reads as safety.

purgeLegacyTokens() one line above has the identical untested-call-site shape. That's pre-existing, but it makes this the second instance of one shape, which per AGENTS.md means the method is wrong rather than the instance. One test pins both.

2. The remembered-flock prefill is untested. Deleting

else if (remembered && f.some((x) => x.id === remembered)) retarget(() => setFlockId(remembered));

while leaving the readAccountScoped call in place also leaves all 1857 tests green. The existing guard pins the storage keys — namespaced read, bare key never read, namespaced write — but never asserts the remembered id selects anything, so the feature is gone and the guard stays green.

This is the inverse of the masking problem the slice already handled: the test moved off the UI observable because DailyEntryPage.tsx:161's membership check masks it, and moving to the key-name observable dropped the behaviour claim entirely. The fix asserts both, plus a real cross-account isolation case (bind A, write, rebind to B, assert no inheritance) which was previously only inferred from the key string.

Wrong claims in comments — four, all mine

Stated plainly because it is the same defect class this slice exists to remove: brand.ts shipped a comment asserting an isolation property no line enforced, and in correcting it I introduced four new inaccurate claims.

• AuthContext.tsx:137 and brand.ts:9 both say SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586 will fix this by "clearing the palette on session end". SPA: per-farm brand palette, keyed by slug so it survives pre-paint #586 does the opposite — it keeps a persistent slug-keyed cache and explicitly rejects clearing, because clearing regresses Per-farm theme — curated accent palettes in farm settings (API + SPA) #149's no-flash guarantee. The wording was duplicated across two files; I had only spotted one copy.
• farmCodeCache.ts:6 says the disclosure is "recorded in Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537's ADR". Docs: multi-farm tenancy ADR + AGENTS.md / GLOSSARY sync #537 is open and no multi-farm ADR file exists. The disclosure is in epic EPIC: Phase 1.6 — Multi-farm tenancy #530 decision 9; the ADR is pending.
• AuthContext.tsx:115-117 cites client.ts:143 and client.ts:239; the real lines are 144 and 242. The reasoning is correct, only the pins are stale — which is exactly why this repo's rule prefers "what is pinned by which test" over prose citations.

Test quality — three, all real

• accountStorage.test.ts calls spy.mockRestore() after its assertions, so a failing assertion skips the restore and leaks a throwing spy into later tests in the file. This is why one verification mutant produced two reds instead of one.
• The URL-notice negative assertion is pinned to the English literal /Signing in to farm/ while its positive half uses i18n.t(...). Live today, but a reword of auth:farmFromLink makes it match nothing and the claim becomes unfalsifiable with no failure to announce it.
• The picker's role="group" / aria-labelledby wiring ships with a justifying comment and no assertion; both attributes are deletable with the suite green.

Rejected, with evidence

• pi: "isFarmCode is dead API production never calls." False — canonicalFarmCode calls it at farmCodeCache.ts:44.
• pi: "a mid-session ?farm= change is silently ignored." The mechanism is real (a useState initialiser doesn't re-run), but the only in-app navigations to /login are AppLayout.tsx:61 and ProtectedRoute.tsx:15 and neither carries a query string. An external farm link is a full page load, which remounts. No current trigger.
• pi: "picker poisoning via ?farm=." Requires the victim to complete a valid login at the attacker's farm, i.e. to already hold credentials there. The cache is success-only by construction.

Accepted as follow-up, not fixed here

rememberFarmCode runs only on the explicit-login path, so a session surviving on the refresh cookie while localStorage is purged loses the cached code until the next full login. Narrow and self-healing — and executeRefresh has no typed farm code to remember anyway.

Also confirmed by review

The invariants reviewer walked every localStorage/sessionStorage site in web/src independently rather than trusting the PR's list, found no namespacing violation, and confirmed no window where getBoundAccountId() is null on an authenticated render. It also verified every line citation in the new modules against source — Account.cs:36, AuthEndpoints.cs:203, AccountRepository.cs:44, and LoginRequestValidator's deliberate absence of a shape rule.

Round-1 count for the stop rule: 2 confirmed defects in product code.

[mforce]

Round-1 fixes are written but not yet applied — the loop is paused on infrastructure, not on a decision.

The implementer backend for this slice (a self-hosted endpoint) went unreachable mid-round: the dispatch failed with API call failed after 3 retries: Connection error after 4 API calls and applied nothing. The branch is unchanged at 1e764801 and still green.

The fix increment for every item in the round-1 comment above is written and ready to dispatch; it is queued until that host returns, by owner decision. Nothing is dropped, and this pause should not be read as the round being finished.

Still outstanding, in priority order:

1. Pin purgeUnscopedAccountState()'s call site (confirmed surviving mutant).
2. Pin the remembered-flock prefill and add real cross-account isolation (confirmed surviving mutant).
3. Correct four inaccurate claims in comments.
4. Three test-quality items (spy restoration, i18n-pinned negative assertion, picker a11y assertion).

[mforce]

Round 1 fixes applied (7b1c01cc), plus a round-2 finding against them (82fbb5b5)

Both merge-blocking mutants are now dead — re-verified by me, not attested

Mutant	Before	Now
delete purgeUnscopedAccountState()'s call site	survived (1857 green)	red — "purges pre-namespacing browser state on mount"
delete purgeLegacyTokens()'s call site (pre-existing gap)	survived	red — same test
delete the remembered-flock prefill branch	survived (1857 green)	red — "actually selects the remembered flock"
delete the picker's role="group" / aria-labelledby	survived	red — "labels the recent-farms picker group with its heading"

The four claim corrections landed and were checked against source: both #586 references now say "the per-farm fix is tracked in #586" with no clearing claim, farmCodeCache.ts points at epic #530 decision 9 with the ADR noted as pending in #537, and the two line pins now read client.ts:144 / client.ts:242.

The implementer caught a factual error in my runbook

I specified a fixture asserting isFarmCode("sunny-acres\n") === true, on the reasoning that $ without the m flag matches before a trailing newline. That is Python's semantics, not JavaScript's — in JS, $ without /m matches only at true end of input, so it returns false. Verified:

JS  $ no m flag  → false      JS  $ with m flag → true      Python $ → True

I took the claim from a reviewer and propagated it without checking. Transcribing it verbatim would have committed a permanently-red test. The implementer refused to encode it, pinned the true behaviour both ways (isFarmCode rejects the newline; canonicalFarmCode trims it first, which is why every live caller is safe), and reported the deviation.

Round 2 — one finding, against round 1's own fix

Round 1 replaced the notice test's English literal /Signing in to farm/ with an i18n-pinned string. That was the right instinct and it did not work: removing the urlFarmCode !== null gate so the notice always renders still left the whole suite green.

Cause: Testing Library normalises the rendered text but compares a string matcher literally. i18n.t("auth:farmFromLink", { farmCode: "" }) ends in a trailing space, because the interpolated value is empty — so it can never match anything. Demonstrated:

queryByText("Signing in to farm: ")   → null      (trailing space: never matches)
queryByText("Signing in to farm:")    → found
queryByText(/Signing in to farm:/)    → found

So the assertion went from vacuous-and-obvious to vacuous-and-principled-looking. Both null and "" interpolate to the same string, so the i18n pinning was not the problem — the trailing space was. Fixed by asserting on the .auth-farm-source node, which is immune to text normalisation. The gate-removal mutant now reddens that test by name.

Applied directly rather than routed back: one mechanical line with a verified diagnosis. Driver fix budget: 1 of 2 spent.

Invariant re-enumerated before closing the loop

A clean round says the diff is clean and nothing about untouched code. All 20 localStorage/sessionStorage sites in web/src plus theme-init.js were re-walked and dispositioned: 3 namespaced, 2 deliberately cross-account (the roster, circular to namespace), 6 correctly device/user-scoped, 3 tab-scoped and cleared on login, 4 infrastructure, and one known gap — cluckwork.brand — deferred by owner decision and tracked in #586.

State

npm run test:coverage — 86 files, 1862 tests, exit 0. build and verify:sw clean. Suite ledger 1817 → 1862 (+45); nothing shrank. src/lib/** and src/auth/** still meet their high-water locks with no floor moved in either direction.

Stop-rule tally: round 1 = 2 confirmed product defects; round 2 = 0 (its single finding was a test guard, in round 1's own fix).

[mforce]

Round 3 — 0 product defects. Stop rule fired; this is the last round.

Three reviewers against 1e764801..HEAD (the round-1/2 fix increment, which no earlier round had seen). Findings: two wrong claims in comments, zero defects in product code. Rounds 2 and 3 both yielded nothing but tests and comments, which is the documented stop condition, so I am not tagging for a round 4.

Correction to my own round-2 comment — I had the provenance backwards

I wrote above that "both the original English literal and its i18n-pinned replacement were unfalsifiable". That is wrong, and the same wrong claim is in 82fbb5b5's commit message. Verified against history:

commit	assertion	falsifiable?
1e764801	queryByText(/Signing in to farm/) — a regex, not a literal	yes — regex matchers apply to the normalised text
7b1c01cc (round-1 fix)	i18n.t("auth:farmFromLink", { farmCode: "" })	no — this is what introduced the hole
82fbb5b5 (round-2 fix)	.auth-farm-source node query	yes, and stronger than either

So round 1 did not fail to close a pre-existing hole — it broke a working guard, and round 2 repaired round 1's own regression. The original F4 finding was about future-proofing an assertion that already worked, and the fix I specified converted a hypothetical future vacuity into a real present one.

That is precisely the loop's convergence signal — a round whose findings are breakage the previous round introduced — and it is a second, independent reason to stop here rather than run a round 4.

The other finding: a wrong claim three lines from the wrong claims it was fixing

farmCodeCache.ts:13 says a drift between the JS regex and Account.SlugPattern is "fail-safe, costing a rejected-at-login code (never an accepted-invalid one)". True in one direction only. The pattern gates prefill, the picker filter and the post-login cache write — not what the operator types, since the input carries no pattern attribute:

• JS looser than the server → an over-permissive cached/URL value reaches login and is rejected there. Matches the claim.
• JS stricter than the server → a genuinely valid slug that just logged in successfully is silently never stored and never offered again. The login never fails. The cost is a permanently unofferable farm code, not a rejection.

Found independently by the contract-drift reviewer and by me on re-read. The security half ("never an accepted-invalid one") holds both ways; the stated cost does not.

Both corrections are dispatched as a comment-only increment.

Verified sound by real mutation (not prediction)

The false-green reviewer ran its checks as actual mutations in throwaway worktrees:

• Case A is not a tautology — FLOCK (f1) is Active and first, FLOCK2 spreads it, so find(x => x.status === "Active") ?? f[0] resolves to f1 by default while the test asserts f2.
• The purge test's "namespaced key survives" assertion is live — widening the purge to a blanket wipe reddens it, so a delete-everything purge cannot pass.
• The cross-account isolation test is not redundant — a prefix-scan fallback in readAccountScoped reddens it and nothing else. Its noted limit: it asserts the default, so it stays green if the remembered read returns null for any reason. The positive test three lines above covers selection; neither substitutes for the other.
• vi.restoreAllMocks() is not a no-op — vite.config.ts sets no restoreMocks, and setup.ts calls only unstubAllGlobals. The leak was real and is closed.
• The JS-vs-Python regex correction is right, and adding /m to the pattern reddens the newline test by name.

pi: 3 findings, 0 valid

Claimed the newline assertion was "unfalsifiable by design" (adding /m reddens it); claimed the purge test passes with the call site deleted (that mutation was already run and went red); and raised the Case-A tautology question, which the mutation above settles.

Tally

round	product defects
1	2 (both surviving mutants)
2	0 (one test guard, in round 1's fix)
3	0 (two wrong claims in comments)

Driver fix budget: 1 of 2 — both round-3 corrections were routed back to the implementer rather than hand-applied.

[mforce]

@codex both of your findings are addressed in 73f6379f — please re-review at that head.

P2 (cross-tab roster race) — real, and fixed properly rather than mitigated. rememberFarmCode now serialises its read-modify-write with the Web Locks API, following the existing AUTH_COOKIE_LOCK precedent at client.ts:402 and its degradation stance for browsers without navigator.locks. The roster is re-read inside the lock callback, and the function never rejects because AuthContext.login awaits it after apiLogin has already succeeded.

P1 (in-app glossary) — a genuine miss against AGENTS.md's docs-in-sync directive. glossaryFarmCodeDef now describes the remembered-codes picker and ?farm= prefill, mirrored into es.ts and tl.ts with matching meaning.

Two things worth your attention on re-review, because both are places where a guard could read as safety without being one:

1. The P2 guard is structural by necessity. The true cross-tab lost update is not expressible in Vitest — it needs two JS contexts sharing one localStorage. The first draft of the test was itself a false green: asserting "storage still holds the old value when the callback is entered" survives hoisting the next computation outside the lock, since with an unchanged seed the pre-computed array and the in-lock re-read are identical. The shipped test injects a concurrent tab's write after lock acquisition but before the callback, so only a real re-read preserves it. Both mutations (remove the lock; hoist the read out of it while keeping the lock) were run and both redden that named test.

2. The navigator.locks-absent path is a real fallback, not dead code — it degrades to the unsynchronised write deliberately, so a browser without Web Locks is no worse off than before this PR. If you think that degradation is wrong for a multi-farm device, say so; it is a deliberate choice, not an oversight.

Full gate at 73f6379f: 86 files / 1866 tests, exit 0; src/auth/** at 98.75 / 94.64 / 100 / 100.

---

Process note, since it affects how much weight to give this PR's earlier comments. Your review of 1e764801 was posted at 09:48 and I did not read it until the owner asked whether you had reviewed. Three review rounds ran in the interim, and my round-3 comment above claims "0 product defects" and declares the loop closed — that tally is wrong, because it counted only the reviewers I dispatched myself and never enumerated this PR's review threads. P2 is a product defect. I have corrected the record here rather than editing the earlier comment.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 73f6379f58

ℹ️ 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".

[mforce]

@codex please re-review at 522e59f4. Your clean verdict covered 73f6379f; two test-only commits have landed since.

235a1ef7 and 522e59f4 address two false greens my own review found in the tests that shipped with the lock fix. No production behaviour changed in either.

The one worth your attention

farmCodeCache.ts has a if (locks === undefined) { write(); return; } guard, and nothing pins it — deliberately, now documented in the test file.

The branch is behaviourally redundant with the catch beneath it: with locks undefined the guard writes and returns; delete the guard and locks.request throws a TypeError that the catch answers with the identical write. Same storage, same call count, no externally observable difference. A mutation deleting the guard leaves the whole suite green — verified by running it.

The first attempt to "fix" this shipped a tautology: a vi.fn() that was never wired to anything, asserted not to have been called. It read as safety and could not fail. That is now removed and replaced with a comment saying why no pin exists and that the previous attempt was fake, so nobody writes another one.

The branch stays because it states intent — relying on undefined.request() throwing is accidental control flow, and narrowing the catch later would silently break browsers without navigator.locks. If you think that trade is wrong, or you can see an observable difference I could not, say so: I would rather delete the branch than keep an untestable one on a false premise.

What is genuinely pinned

mutation	result
remove the lock entirely	red — concurrent-write test
hoist the re-read outside the lock (lock still held)	red — same test
change ROSTER_LOCK's value	red by name, in two tests
delete the locks === undefined guard	survives — documented above, not hidden

Gate at 522e59f4: 86 files / 1867 tests, exit 0; src/auth/** 98.75 / 94.64 / 100 / 100 against its 98 / 90 / 100 / 100 lock.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 522e59f4a5

ℹ️ 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".