Repository navigation
feat(eggs): make cracked and dirty eggs sellable stock via condition grades (#396) - #407
Conversation
…ities (#396) Task 1 of the plan, domain half. Carries the plan and design docs from the closed #397 so the reviewed rules land with the code that implements them. EggGrade gains DailyEntryKind (Manual/Cracked/Dirty) — an IDENTITY for which counter feeds a grade, deliberately not a name match: a farm can rename "Cracked" to "Segunda" through the ordinary grade endpoint, and a name-matching resolver would then detach the farm's own counter from its own grade. Immutable after creation, for a stronger reason than GradeType's: an official entry snapshots the id its counter resolved to, so re-pointing a kind afterwards would restate what a past day produced. Update takes no kind parameter, which is the enforcement. DailyEntry gains CrackedGradeId/DirtyGradeId, written once at the first official transition and never re-resolved. Null means "resolved to nothing" — a durable record that the condition was a loss — not "not yet known". The domain does not resolve them (it has no catalog); the handler passes what it resolved, having required both Active and IsSaleable. That both-flags rule is pinned at its source by Deactivate_leaves_saleability_alone: EggGrade.Deactivate() flips Active and leaves IsSaleable set, so inactive-but-saleable is an ordinary reachable state. A saleability-only resolver would mint lots under a grade the farm has retired from capture — the defect #397's review round 3 caught in prose, now caught by a test. Mutation-checked, baseline green (303/303, solution builds 0 warnings): dropping the Submit assignment turns Submit_records_the_resolved_quality_grades red; making ManagerAdjust clear the snapshot turns Adjust_keeps_the_original_snapshot_and_never_re_resolves red. Still to come in Task 1: EF configuration, the hand-carried InitialCreate columns/partial index/seed defaults, and the two export projections.
Task 1, persistence half. Hand-carried into InitialCreate per #245 — no second migration. The delta was generated with `ef migrations add` against a scratch, read, folded in, and the scratch deleted; both model snapshots were rebuilt so the model and the single migration still agree. Schema: EggGrades."DailyEntryKind" (varchar(16), default 'Manual', stored as a string like GradeType so an enum reorder cannot repoint existing rows), and DailyEntries."CrackedGradeId"/"DirtyGradeId" (nullable uuid). The snapshots take no FK deliberately — they must survive their grade being deleted, exactly as they must survive it being renamed. Index: IX_EggGrades_AccountId_FarmId_DailyEntryKind, unique and PARTIAL (WHERE "DailyEntryKind" <> 'Manual'). One Cracked and one Dirty per farm, since the entry has exactly one counter for each and a second claimant would make resolution silently pick one. The filter is what leaves the many ordinary Manual grades unconstrained, which is also why the column default must stay 'Manual'. Seed: Cracked and Dirty are now saleable by default and carry their kinds; Discarded stays a loss on 'Manual' — binding it would make discarded eggs resolvable to stock. Statement and row counts are unchanged (12 INSERTs, 21 rows), the grades guard stays whole-set and blind to "Name", so MigrationSecurityReviewTests holds. The existing base-data test would have passed with every new flag wrong — it counts ten grades and checks the name index. Two tests added, both asserting through observable behaviour rather than names: - the two condition grades are saleable and bound, Discarded is neither, and the other eight are Manual - a second Cracked is refused by the partial index WHILE an eleventh Manual grade is still accepted — the half a filterless index would break Mutation-checked, baseline green (13/13 migration, 303/303 domain, 0 warnings): seeding Cracked non-saleable turns exactly one test red; dropping the index filter turns the whole class red (a filterless unique index cannot even seed eight Manual grades).
…ports (#396) Completes Task 1. An export is the farm's own copy of its records, and without these columns it states that a day had N cracked eggs while omitting whether they became saleable stock or a loss — which is NOT recoverable afterwards, since the snapshot exists precisely because the grade catalog may have changed since. daily-entries gains crackedGradeId/dirtyGradeId next to the counters they explain; egg-grades gains dailyEntryKind, the only field naming which counter a grade serves and the only one that survives a rename. ExportTests previously asserted dataset names and content types only, so both projections could have been left out silently. The new test checks the header AND a value — a header-only assertion passes against a column wired to the wrong property. Mutation-checked, baseline green (6/6 export, 13/13 migration, 0 warnings): dropping dailyEntryKind from the projection turns exactly that test red.
) Task 2, first slice — the hole this feature opens, closed before the code that opens it. Making Cracked and Dirty saleable makes them pass the manual-line eligibility check, which asks "active and saleable" with no kind restriction. So a direct or stale API client could name a Cracked id as a graded line; #394's exact-total check does not catch it, because that check is only a sum and the sum still reconciles. Submission would then create the manual lot AND the counter-backed quality lot for the same grade — two lots for one grade on one day, double-counting stock and breaking the one-lot-per-grade assumption lot reconciliation depends on. Enforced in the handlers, not a validator (#395/#394): the demo and simulation seeders construct commands and call HandleAsync directly, skipping validators entirely, so a validator-only rule would let a fixture build exactly this state. ConditionGradeGuard is deliberately NOT folded into either handler's existing allow-set. The two ask different questions, and adjust's set additionally unions the lines already on the entry, so a guard sharing it could be talked past. Tests assert the durable state, not the status code: a refused record leaves no entry at all, and a refused adjust leaves status, version and lot count byte-identical — adjust is the path that has already generated lots, so a partial failure there is worse than on record. Mutation-checked with a rebuild after each restore (a --no-build confirmation re-runs the mutant and can report a false green): removing the guard from record turns both record tests red, removing it from adjust turns the adjust test red. Baseline green — 34/34 daily-entry integration, 114/114 application, 0 warnings. One comment corrected rather than left overstated: the guard reads the whole catalog, and I had written that as though it closed a reachable gap. It does not today — an inactive grade is already refused upstream, so narrowing it would change the error code, not the outcome. The comment now says that plainly and says why the shape is still worth keeping.
…lots (#396) Completes Task 2. Submission is the one point at which the Cracked and Dirty counters resolve to a grade, and the resolution requires BOTH Active and IsSaleable. Neither flag implies the other. EggGrade.Deactivate() leaves IsSaleable set, so "inactive but saleable" is an ordinary reachable state — an owner retiring a condition grade without first clearing its saleability — and resolving on saleability alone would mint stock under a grade the farm has already removed from capture. That was #397's review round 3 finding, stated there as prose; it is now pinned by a test that goes red when the Active half is dropped. Unresolved is recorded as NULL, which is a durable outcome ("these eggs were a loss") rather than a gap to be filled in later. The lot loop reads the entry's own SNAPSHOT rather than the ids just resolved, so this loop and every later reader agree by construction and an adjustment — which never re-resolves — takes the identical path. A zero counter still snapshots but mints no lot: EggLot.Create rejects a zero quantity, and "the farm was selling cracked eggs that day" remains a fact worth recording. Snapshot and lot are separate decisions, and that test is what proves they are. Worth recording about the tests themselves: the two "resolves to nothing" cases passed VACUOUSLY before this commit, because nothing resolved at all. They only became meaningful once resolution existed, which is why each was mutation-checked against the specific rule it covers rather than trusted for being green. Mutation-checked, rebuilding after every restore, baseline green (44/44 integration, 303/303 domain, 114/114 application, 0 warnings): - resolving from the full catalog instead of the active slice → the inactive-but-saleable test alone goes red - dropping the IsSaleable requirement → the non-saleable test alone goes red - dropping the zero-counter skip → the zero-counter test alone goes red
…manual ones (#396) A defect introduced by the previous commit in this branch, found by writing the adjustment tests rather than by reasoning about the change. The reconciler drives EVERY lot linked to the entry toward a target built from entry.Grades. Condition lots are deliberately absent from entry.Grades (a condition grade is never a manual line), so they had no target — and a lot with no target does not get skipped, it reads as target ZERO. Any adjustment to an entry carrying quality stock would therefore have emptied that stock silently, including adjustments that had nothing to do with the condition counters. Targets now include both snapshots, read from the ENTRY rather than re-resolved from the catalog: a farm that turns Cracked non-saleable after submitting must still be able to correct that day, and the day keeps the meaning it was recorded with. The existing new-lot loop then covers the other direction — a counter that goes from zero to positive gains a lot dated and linked like the rest. Mutation results, reported in full because one is not a clean pass: - removing the condition targets → the grow test and the manual-lot-untouched test go red - removing the zero-quantity skip in the new-lot loop → SURVIVED on the first attempt. Every existing test submitted a positive counter, so the zero-with-no-lot path was never exercised and the guard was an untested claim. Adjusting_a_day_that_never_had_cracked_eggs_still_succeeds now covers it — an ordinary good day has a snapshot and no lot, and EggLot.Create rejects a zero quantity, so without the skip the adjustment throws. Re-run: killed. Also worth recording: Adjusting_a_condition_counter_to_zero_empties_its_lot passed BEFORE the fix, because the bug and the correct behaviour agree at zero. It was green for the wrong reason and only became meaningful afterwards. Baseline green: 66/66 across the daily-entry, quality-egg and race suites, 0 warnings. Version handling is unchanged — no new aggregate mutation path — so the existing parallel submit/adjust race coverage still applies and passes.
#396) Owner decision: keep Sellable at its existing meaning and show the condition figure BESIDE it, rather than folding the two together. For a 1000-egg day with 900 hand-graded, 60 cracked, 30 dirty and 10 discarded, the report showed "Sellable: 900" while the farm could actually sell 990. The new FromCounts column carries the 90 — the eggs that became stock without passing through the Grading pane. Deliberately a separate figure, not a widened Sellable. The two answer different questions ("how many did we grade" vs "how many can we sell"), were only ever equal because conditions used to be losses, and Sellable is also the target Daily Entry counts down to and that #394 requires the grade lines to reconcile against exactly — moving it would have silently changed what the capture screen validates. Only SNAPSHOT-BACKED conditions count. A null snapshot records that those eggs were a loss that day, so summing the raw counters would invent stock the farm never had — most visibly for entries recorded before this feature, whose conditions were never saleable at all. The test fixture makes Cracked saleable and Dirty NOT, so the naive rule reports 9 where the answer is 6. Mutation-checked, baseline green (7/7 reports, 0 warnings): summing the raw counters turns exactly that test red. Still to come: the Reports and History screens need the column rendered, and the label wants deciding — "From counts" is used here rather than "Ungraded", because those eggs do carry a grade (Cracked/Dirty) and appear in the report's own grade breakdown, so "ungraded" would contradict the table beside it.
Owner-named column: "Condition" — the term specs.md and the glossary already use for Cracked and Dirty. Deliberately not "Ungraded" (those eggs do carry a grade, shown in the same screen's by-grade breakdown), not "Seconds" (a real seeded grade name, so the column and the breakdown would disagree), and not "Bad eggs" (in food that reads as spoiled, and these are being sold — the app already has Discarded for eggs thrown away). Reports gains the column beside Sellable, never folded into it: Sellable stays the hand-graded remainder and the figure Daily Entry counts down to. History gains the same column per entry. History reads each entry's own SNAPSHOT, never the current grade catalog. A farm that switches Cracked off today must still see past days as they were recorded, and one that switches it on must not see past losses become stock. A draft shows an em dash rather than 0 — nothing has resolved yet, and 0 would assert "these were a loss", a different fact that is not yet true. API: DailyEntryResponse gains crackedGradeId/dirtyGradeId (null on a draft and on a resolved-to-loss entry alike; readers distinguish by Status). The ADJUST REQUEST deliberately does not carry them — an adjustment never re-resolves — and a test now pins that, after a broad fixture edit of mine wrongly added them to that assertion and the suite caught it. Help page and GLOSSARY.md updated in the same change, per the standing rule. The glossary gains "Condition grade" (the kind binding, the snapshot, the both- flags rule, and why a condition grade can never be a manual line) and "Condition" as the column. All three catalogs carry the strings; es/tl are machine-drafted pending native review (#182). Assertions are cell-indexed, not text-matched: "0" and the em dash both occur in sibling cells, so a text query passes against the wrong column. That is not hypothetical — the first draft of both tests did exactly that and had to be tightened. Mutation-checked, baseline green (1300/1300 vitest, typecheck clean, 0 warnings): summing the raw counters in History turns 2 tests red; dropping the draft em dash turns 1 red; wiring the Reports column to sellable turns 3 red.
Completes Task 3. EggGradeResponse gains dailyEntryKind ("Manual" | "Cracked" |
"Dirty"); the SPA cannot infer it from name (renameable) or gradeType (a farm
can have many Quality grades, only one of which is the Cracked counter's).
Both grading panes filtered on saleability alone, and this feature makes Cracked
and Dirty saleable — so both would have offered them as grade fields. Daily
Entry's catalog filter and History's panelGrades now also require the Manual
kind. History excludes it from the CATALOG half only: an existing line stays
correctable whatever it names.
This is an affordance, not the enforcement. The server refuses a manual line
naming a condition grade regardless (ConditionGradeGuard, 7370dce); what this
removes is a control whose only possible outcome is a rejection.
Both tests had to be tightened after failing for the right reason. "Cracked" is
a deliberately ambiguous label on these screens — Egg counts has a Cracked
COUNTER, which must stay — so an unscoped query matches the counter and passes
whatever the grading pane does. The Daily Entry test now scopes to the Grading
section and additionally asserts the counter is still present in Egg counts;
the History test asserts the dialog shows exactly ONE "Cracked" control, the
counter, so a grade field would make two.
Mutation-checked, baseline green (1302/1302 vitest, 16/16 egg-grade + quality
integration, typecheck clean, 0 warnings): dropping the kind check from either
filter turns exactly its own test red.
…ixtures (#396) Closes out #396's non-CI-caller obligation (AGENTS.md #395/#394): a change to what submit writes has to be carried into every automated caller in the same PR, and coverage over those callers is not uniform. What actually broke, and what did not — established by reading each caller's payload rather than by running it, since k6's `dailyEntryScreen` counts a tolerated 422 as a passing check and would have stayed green either way: * `SimulationDataSeeder` — BROKEN, and it would have failed closed. Its manifest asserts every count exactly, and `EggLots` was derived from `GradesPerDailyEntry` alone. Submit now also mints a lot per resolved condition counter, and this seeder's deterministic math keeps both counters strictly positive on every entry (`cracked = 4 + d % 3`, `dirty = 2 + d % 2`), so every submitted entry gained exactly two lots. `ValidateCounts` would have reported `Failed` on a correct seed. Added `ConditionLotsPerDailyEntry` beside the existing constant rather than merging the two into a `5`: they answer different questions (grade LINES vs counter-backed lots) and drift for different reasons. * Its grade lines name Large/Medium/Small explicitly, so `ConditionGradeGuard` never fires — checked, not assumed. * `DemoDataSeeder` — sound as written, same explicit grade lines. * `tools/simulation/k6/bundles.js` — unaffected. It posts no `grades` array at all and zeroes both counters, and it records without ever submitting, so neither the guard nor condition resolution is reachable. * Playwright — unaffected. `worker.spec.ts`'s `GRADES` list is exactly the five Manual saleable grades. `getByLabel("Cracked")` stays unambiguous for a reason worth recording: Cracked was previously non-saleable and so absent from the grading pane; it is now saleable but excluded by kind, so the counter remains the only match. `manager.spec.ts` submits with cracked=1 and now silently exercises condition-lot minting, then adjusts and voids it — all three paths hold. Both fixtures also gained a product mapped to a condition grade. Without one the feature reads as stock appearing that nothing can ever sell — ~180 lots of dead inventory in the simulation fixture — and #406 (stock reconciliation) does not exist yet. Priced below Small, which is the point of the feature. Also pins a guarantee that was implicit and unproven: voiding empties the condition lots too. `VoidDailyEntryHandler` reverses whatever `GetByDailyEntryLockedAsync` returns — grade-agnostic, so condition lots are covered by construction rather than by a second code path. Narrowing that query to the entry's grade lines (the exact shape this PR already fixed in `AdjustDailyEntryHandler`) would leave a voided day carrying live, sellable cracked and dirty stock with no entry behind it. Mutation-checked: baseline 12/12 green, the narrowed-query mutant leaves the cracked lot at 10 and fails the test, restored and rebuilt back to 12/12. Verified: 1253/1253 dotnet (114 application + 303 domain + 836 integration), 1340/1340 vitest across 69 files, typecheck clean, 0 build warnings.
![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: f8a3b409ba
ℹ️ 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".
| FromCounts = g.Sum(e => | ||
| (e.CrackedGradeId != null ? e.CrackedEggs : 0) | ||
| + (e.DirtyGradeId != null ? e.DirtyEggs : 0)), |
There was a problem hiding this comment.
For every official entry whose cracked or dirty snapshot is non-null, these lines add the eggs to FromCounts, but the gradeTotals query below still aggregates only DailyEntryGrades. Condition-backed production creates EggLot rows and never creates DailyEntryGrade rows, so the report's “By grade” breakdown systematically omits all cracked/dirty stock even though the same response reports it as produced stock. Aggregate the snapshot-backed counters into their snapshotted grade IDs as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 082aa15 — this was a real defect, and the reasoning is exactly right.
Verified before accepting: GradeTotals came from DailyEntryGrades alone, and condition-backed production creates an EggLot but never a DailyEntryGrade row — and never can, because ConditionGradeGuard refuses a manual line naming a condition grade precisely so one day cannot produce two lots for one grade. So sum(GradeTotals) equalled TotalSellable while the stock that exists is TotalSellable + TotalFromCounts.
The fix aggregates the snapshot-backed counters into their snapshotted grade ids and merges them in. Three details that were not obvious:
- Keyed on the snapshot, not the live catalog — so turning a grade off later still cannot rewrite a past period, which is the same invariant
FromCountsalready relied on. - Gated on
counter > 0, mirroringSubmitDailyEntryHandler's own lot condition. A snapshot is recorded even on a zero-cracked day, and counting those would put a grade in the breakdown the farm holds no stock under — reading as an empty grade rather than as absence. - Accumulates rather than assigns, so a farm that pointed both kinds at one catalog row gets correct arithmetic instead of one counter silently dropped.
Test asserts the arithmetic a reader of the screen actually does: sum(GradeTotals) == TotalSellable + TotalFromCounts. The fixture already had the ideal shape (Cracked saleable, Dirty not), so it also pins the other half — Dirty must stay absent, since folding the counters in must not resurrect conditions the entry recorded as losses.
Mutation-checked, and the first attempt was a false pass worth reporting honestly: an if (false) mutant failed to compile under warnings-as-errors (CS0162), so dotnet test --no-build ran the stale binary and reported 7/7 green while testing nothing. Redone with a mutant that compiles — baseline 7/7 green, merge removed → red, restored and rebuilt → 7/7 green.
No SPA change needed: ReportsPage maps whatever gradeTotals the API returns.
Full run: 1253/1253 dotnet, 1340/1340 vitest, 0 build warnings.
Codex review of #407, P1 — a real defect and mine. `GetProductionAsync` returns both `TotalFromCounts` (eggs that became stock without being hand-graded) and `GradeTotals` (the By-grade breakdown), and the breakdown was built from `DailyEntryGrades` alone. Condition-backed production creates an `EggLot` but never a `DailyEntryGrade` row — and never can, because `ConditionGradeGuard` refuses a manual line naming a condition grade precisely so one day cannot produce two lots for one grade. So the same response reported N cracked eggs as produced stock while the By-grade table omitted every one of them: `sum(GradeTotals)` equalled `TotalSellable` when the stock that exists is `TotalSellable + TotalFromCounts`. Folds the snapshot-backed counters in under the grade each entry actually resolved to, keyed on the SNAPSHOT rather than the live catalog, so turning a grade off later still cannot rewrite a past period. Two queries rather than one: the counters sit in different columns under different snapshot keys, which a single GROUP BY cannot express without a UNION EF would not translate. Gated on `counter > 0`, mirroring SubmitDailyEntryHandler's own lot condition — a snapshot is recorded even on a zero-cracked day, and counting those would put a grade in the breakdown the farm holds no stock under, reading as an empty grade rather than as absence. The merge accumulates rather than assigns, so a farm that pointed both kinds at one catalog row gets correct arithmetic instead of a silently dropped counter. The assertion that states the guarantee is the arithmetic a reader of the screen actually does: `sum(GradeTotals) == TotalSellable + TotalFromCounts`. The existing fixture already had the ideal shape (Cracked saleable, Dirty not), so the test also pins the other half — Dirty must stay absent, since folding the counters in must not resurrect conditions the entry recorded as losses. Mutation-checked, and the first attempt was a false pass worth recording: an `if (false)` mutant failed to COMPILE under warnings-as-errors (CS0162), so `dotnet test --no-build` ran the stale binary and reported 7/7 green while testing nothing. Redone with a mutant that compiles: baseline 7/7 green, the merge removed fails the test, restored and rebuilt back to 7/7. No SPA change needed — ReportsPage maps whatever `gradeTotals` the API returns. Also folds in the #396 specification section from the closed #397, which was the only place it existed; specs.md on main had no section for this behaviour at all. Corrected one sentence that had gone stale against the single-migration rule: it described migrating "existing official entries" with null snapshots, but a virgin database is the only starting state, so there is no such population. Restated as a property of the columns (never backfilled, null is the durable "was a loss that day" answer) rather than as a migration step. Verified: 1253/1253 dotnet, 1340/1340 vitest across 69 files, 0 build warnings.
|
@codex the P1 on You were right, and it was mine: Fixed by aggregating the snapshot-backed counters under their snapshotted grade ids: keyed on the snapshot rather than the live catalog, gated on Same push also folds in the #396 specification section from the closed #397 (specs.md on main had none) and corrects one sentence in it that had gone stale against the single-migration rule. Please re-review — particularly the report change and the seeder-count math in |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
…396) Until now a schema change was hand-folded into `InitialCreate` (#245). That was only safe while the application had never been deployed: no database had applied that file yet, so rewriting it was free. This PR is the last change permitted to do it — from the commit that merges it, a database exists which has already applied `InitialCreate`, EF will never re-run it, and a hand-folded column is a column that silently does not exist. That failure mode is why this is worth a rule rather than a note: it surfaces as broken behaviour, not as a migration error. #399's amendment did exactly this to the dev database and presented as a failing login. The load-bearing part is NOT the prose. `MigrationSecurityReviewTests` asserted `Assert.Single(AllMigrations())`, so the first post-cutover PR to add a migration would have gone red no matter what the docs said. Documenting the cutover without moving that fence would have been a no-op. Replaced it with a guard that still has teeth, rather than deleting it: `InitialCreate` must stay FIRST and keep its recorded id (`20260801190854_InitialCreate`). Regenerating it mints a new timestamp, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database — now a red test instead of a failed deploy. Ordering is asserted on the migration ID, never on list position: `AllMigrations()` is built from `Assembly.GetTypes()`, whose order is explicitly unspecified, so a positional check would pass or fail on reflection ordering. Relaxing that one test is safe because every other assertion in the file already fans out across the whole migration history (`AllInsertDataOperations` / `AllSqlOperations`) — the credential and reference-data guarantees extend to post-cutover migrations for free. They were written that way from the start. Mutation-checked: pointing the pinned id at a different timestamp fails the test; restored and rebuilt back to green. Docs updated in the three places the rule is actually stated or implied: - AGENTS.md's migration bullet — the canonical rule. - README gains a "Changing the database schema" section, which did not exist: the rule lived only in AGENTS.md, so a human following the README had nothing telling them to add a migration. Includes the #318 fail-closed design-time connection and its loopback opt-out, since `ef migrations add` needs both. - Both note that a dev database created BEFORE this merges predates the folded-in columns and must be recreated. `deploy/README.md`'s #364 line is deliberately untouched — it describes what was true of that release at the time, not a live rule. Verified: 1253/1253 dotnet, 0 build warnings.
|
@codex new commit It does something the earlier commits did not: this PR is now the migration cutover. Until now a schema change was hand-folded into The part worth reviewing hardest is the test change. Specifically worth challenging:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c84403996
ℹ️ 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".
| Assert.NotEmpty(ordered); | ||
| Assert.Equal("InitialCreate", ordered[0].Migration.GetType().Name); | ||
| Assert.Equal(InitialCreateMigrationId, ordered[0].Id); |
There was a problem hiding this comment.
When a later schema PR edits InitialCreate.cs and its designer/snapshot instead of adding a second migration, these assertions remain green because the class name and migration attribute do not change. The remaining tests only inspect credential/reference-data inserts and four expression indexes; none detects a new AddColumn, CreateTable, or other ordinary schema operation folded into InitialCreate. A database that already recorded this ID would therefore skip the edit and run code against a missing schema element, so add an immutable operation fingerprint or equivalent guard for InitialCreate while still allowing later migration IDs.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1c81df1. You found the hole in my own fence, and the reasoning is exactly right.
I pinned InitialCreate's class name and migration id — which proves it is still first, but says nothing about what it contains. The regression the cutover exists to prevent is precisely an edit to its contents, and that edit changes neither the class name nor the MigrationAttribute. So the guard passed on exactly the change it was written to stop.
Now fingerprinted: a pinned operation count plus a digest over a structural description of every UpOperation. Any added, removed, or retyped column/table/index/raw-SQL statement fails with a message naming the fix (add a migration) rather than inviting a re-baseline.
Two choices worth flagging, since both cost something the other way:
- The description is structural, not a serialization of EF's operation objects. A serialization would break on an EF version bump that touched an internal property nobody edited — training people to re-baseline the constant on sight, which is how a fence like this dies quietly.
- The count is pinned alongside the digest, not instead of it.
Describe()collapses unusual operation types to their type name, so two differentDropColumns would describe identically; the count catches an added or removed operation even when the description is coarse. That limit is written into the test rather than papered over.
Mutation-checked with the real regression rather than a proxy — folding MutantFoldedColumn into an existing CreateTable in InitialCreate. It failed; restored and rebuilt back to green.
One result from that run is worth reporting, because it sharpens your finding: the operation count stayed at 114 and only the digest moved. The column went inside an existing CreateTable, so a count-only guard would have passed too. Both halves are load-bearing.
1254/1254 dotnet, 0 build warnings.
…ntity (#396) Codex review of #407, P1 — correct, and it found the hole in my own fence. The cutover commit pinned InitialCreate's class name and migration id. That proves the migration is still FIRST, but says nothing about what it CONTAINS — and the regression the cutover exists to prevent is precisely an edit to its contents. A later PR folding a column into InitialCreate.cs changes neither the class name nor the MigrationAttribute, and the other tests in this file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes. An ordinary AddColumn/CreateTable edit was invisible to all of them, so the guard passed on exactly the change it was written to stop. Fixed by fingerprinting the operations: a pinned count plus a digest over a structural description of every UpOperation. Any added, removed, or retyped column/table/index/raw-SQL statement fails the test with a message naming the fix (add a migration), rather than inviting a re-baseline. Two deliberate choices, both of which cost something if taken the other way: * The description is STRUCTURAL, not a serialization of EF's operation objects. A serialization would break on an EF version bump that touched an internal property nobody edited — training people to re-baseline the constant on sight, which is how a fence like this dies quietly. Describe() reads only the properties that define what the migration does. * The count is pinned ALONGSIDE the digest rather than instead of it. Describe() collapses unusual operation types to their type name, so two different DropColumns would describe identically; the count catches an added or removed operation even when its description is coarse. That limit is stated in the test rather than papered over. Mutation-checked with the real regression rather than a proxy: folding `MutantFoldedColumn` into an existing CreateTable in InitialCreate fails the test; restored and rebuilt back to green. Worth recording what that run showed — the operation COUNT stayed at 114 and only the digest moved, because the column went inside an existing CreateTable. A count-only guard would have passed. That is the finding's point, demonstrated rather than asserted. AGENTS.md's migration bullet updated to describe both halves of the fence and why identity alone was insufficient. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings.
|
@codex the P1 on You were right: pinning the class name and migration id proves Now fingerprinted — pinned operation count plus a digest over a structural description of every Mutation-checked with the real regression: folding a column into an existing Please re-review, particularly:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c81df1e6f
ℹ️ 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".
| CreateTableOperation t => | ||
| $"CreateTable({t.Name}:{string.Join('|', t.Columns.Select(c => $"{c.Name}:{c.ColumnType}:{c.IsNullable}"))})", |
There was a problem hiding this comment.
Changing an existing CreateTable operation can still leave both checks unchanged: this description omits column defaults and annotations as well as the table's nested primary keys, foreign keys, unique constraints, and delete behaviors. For example, changing AspNetUsers.CredentialEpoch from defaultValue: 1 to 0, or changing a foreign key's onDelete, preserves the operation count and every emitted description while materially altering the schema, so the freeze test remains green. Include all schema-defining properties of the currently present operation shapes (especially CreateTableOperation's columns and nested constraints) in the structural digest.
AGENTS.md reference: AGENTS.md:L46-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 4889567. Right again — and this was the third miss of the same kind, which is the part worth acting on.
Your CredentialEpoch example is the one that stings: defaultValue: 1 -> 0 is a value #364 declares permanently retired, and it left the operation count and the entire digest unchanged.
The prior digest used a hand-written switch listing "the properties that matter". Enumerating harder is the bet that had already lost twice, so I changed the approach rather than extending the list. Describe() now walks every public readable property of an operation, recursing into nested column definitions, constraints, foreign keys, annotations and sequences. A schema-defining property is included because it exists, not because someone remembered it — adding one to an operation type cannot silently escape the digest.
Details that turned out to be load-bearing:
- Properties ordered by name, never reflection order.
Type.GetProperties()is unspecified — the same hazard the ordering test already avoids withGetTypes(). A flaky fence is worse than none, so I verified determinism empirically: three consecutive runs, byte-identical digests. - Collection order preserved, not sorted — a composite key's column order is itself schema, so sorting would hide a reordering.
- Annotations described, not skipped — Npgsql's
ValueGenerationStrategyrides there, so skipping them would drop identity/serial changes. - A property that throws is recorded as having thrown, rather than falling back to
null, which would be a silent hole.
Mutation-checked against both cases you named, rather than a proxy:
| mutation | before | after |
|---|---|---|
CredentialEpoch defaultValue: 1 -> 0 |
green (missed) | RED |
FK onDelete Cascade -> Restrict |
green (missed) | RED |
Both restored and rebuilt back to green, with the migration file confirmed byte-identical to HEAD afterwards.
The EF-version-churn concern that motivated the original hand-written approach still applies and I am accepting it deliberately: this reads EF's public operation model, not internals, and a genuine change there is worth a human look. The failure message says re-baselining is almost never the right response.
1254/1254 dotnet, 0 build warnings.
…cked list (#396) Codex review of #407 round 2, P1 — right again, and the third miss of the same kind, which is the actual lesson. The previous digest described operations with a hand-written switch: for a column, name + type + nullability. It omitted the column's DEFAULT, and a CreateTable's nested primary key, foreign keys, unique constraints and delete behaviours. So the schema could change materially with the operation count and every emitted description identical, leaving the freeze test green. Codex's example is the one that stings: changing AspNetUsers.CredentialEpoch from `defaultValue: 1` to `0` — a value #364 declares PERMANENTLY RETIRED, the whole point of that design — was invisible. Enumerating harder is the same bet that had already lost twice, so this changes the approach instead. Describe() now walks every public readable property of an operation, recursing into nested column definitions, constraints, foreign keys, annotations and sequences. A schema-defining property is included because it EXISTS, not because someone remembered it; adding one to an operation type cannot silently escape the digest. Details that are load-bearing rather than incidental: * Properties are ordered by NAME, never reflection order. Type.GetProperties() is explicitly unspecified — the same hazard the ordering test above already avoids with GetTypes(). A digest built on reflection order would churn between runs, and a flaky fence is worse than none. * Collection ORDER is preserved, not sorted. For a composite key or an index's columns the order is itself schema, so sorting would hide a reordering. * Annotations are described, not skipped — Npgsql's ValueGenerationStrategy rides there, so skipping them would drop identity/serial changes. * A property that throws is recorded as having thrown rather than falling back to null, which would be a silent hole. * Determinism verified empirically, not assumed: three consecutive runs produce byte-identical digests. The EF-version-churn concern that motivated the original hand-written approach still applies and is accepted deliberately. This reads EF's PUBLIC operation model — the documented shape of a migration — not internals, and a genuine change there is worth a human look. The failure message says plainly that re-baselining is almost never the right response. Mutation-checked against BOTH cases codex named, not a proxy for them: A. CredentialEpoch defaultValue 1 -> 0 -> RED (was green before) B. a foreign key's onDelete Cascade -> Restrict -> RED (was green before) Both restored and rebuilt back to green, with the migration file confirmed byte-identical to HEAD afterwards. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings.
|
@codex the P1 on nested constraints and defaults is addressed in You were right, and it was the third miss of the same kind — a hand-written list of "the properties that matter". So I changed the approach instead of extending the list: Mutation-checked against both cases you named — Please re-review. Specific things I would rather you attack than take on trust:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4889567bbc
ℹ️ 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 members = value.GetType() | ||
| .GetProperties(BindingFlags.Public | BindingFlags.Instance) | ||
| .Where(p => p.CanRead && p.GetIndexParameters().Length == 0) | ||
| .OrderBy(p => p.Name, StringComparer.Ordinal) | ||
| .Select(p => $"{p.Name}={DescribeValue(SafeGet(p, value), depth + 1)}"); |
There was a problem hiding this comment.
EF exposes an operation's annotations through GetAnnotations(), not through a public non-indexed property, so this reflection walk never reaches the IAnnotation branch above. Fresh evidence is the two Npgsql:ValueGenerationStrategy annotations already present in InitialCreate.cs: changing or removing either leaves the operation count and every reflected property unchanged, allowing a material identity-generation change to pass the freeze test. Explicitly enumerate each IReadOnlyAnnotatable's annotations, sorting them by name for determinism, before walking its properties.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6b3ee09. Correct, and this is the worst of the four — the previous commit did not merely miss something, it shipped a comment claiming coverage the code did not have.
GetAnnotations() is a method, the walk enumerates public non-indexed properties, so the IAnnotation branch I wrote to handle them was unreachable dead code — sitting directly under a comment saying annotations "are described rather than skipped". That sentence was false when written.
Verified as a before/after on the identical mutation rather than argued:
Npgsql:ValueGenerationStrategy IdentityByDefaultColumn -> IdentityAlwaysColumn |
|
|---|---|
4889567 (previous) |
GREEN — hole confirmed |
6b3ee09 (this) |
RED |
Same mutation, same migration; only the fingerprint changed between the two runs.
Annotations are now enumerated explicitly from IReadOnlyAnnotatable and emitted first, so every annotatable in the graph contributes — the operation itself and each AddColumnOperation nested inside a CreateTable. Sorted by name, since IReadOnlyAnnotatable promises no order; determinism re-verified across three consecutive runs.
The pattern across these four rounds is the part I have taken away, and AGENTS.md now records it: every version of this fence shipped a comment asserting coverage it did not have, and every one was caught by review rather than by the test suite. The standard for changing this fingerprint is a mutation that goes red, not an argument that it should.
1254/1254 dotnet, 0 build warnings, migration byte-identical to HEAD afterwards.
…perty walk cannot (#396) Codex review of #407 round 3, P1 — correct, and the worst of the four, because the previous commit did not merely miss something: it shipped a comment claiming coverage the code did not have. EF exposes an operation's annotations through GetAnnotations(), a METHOD. The reflection walk enumerates public non-indexed PROPERTIES, so it never reached them — and the IAnnotation branch written to handle them was unreachable dead code sitting under a comment that said annotations "are described rather than skipped". That sentence was false when written. Proven rather than argued, and the cleanest before/after available: with the previous commit in place, flipping InitialCreate's Npgsql:ValueGenerationStrategy from IdentityByDefaultColumn to IdentityAlwaysColumn — a material identity-generation change — left the freeze test GREEN. The SAME mutation against this commit fails it. Only the fingerprint changed between those two runs; the mutation and the migration were identical. Annotations are now enumerated explicitly from IReadOnlyAnnotatable and emitted FIRST, so every annotatable in the graph contributes — the operation itself and each AddColumnOperation nested inside a CreateTable. Sorted by name, because IReadOnlyAnnotatable promises no order and an unsorted digest would churn between runs; determinism re-verified across three consecutive runs. The pattern across these four rounds is the finding worth keeping, and AGENTS.md now records it: each version of this fence shipped a comment asserting coverage it did not have, and each was caught by review rather than by the test suite. The standard for changing this fingerprint is a mutation that goes RED, never an argument that it should. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings. Migration file confirmed byte-identical to HEAD after the mutation runs.
|
@codex the P1 on operation annotations is addressed in You were right, and this was the worst of the four: the previous commit shipped an Before/after on the identical mutation — Annotations now enumerated explicitly from Please re-review. Given this fence has now been wrong four times in a row, I would rather you attack it than confirm it:
|
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
… on (#396) CI failure on 6b3ee09, and it was mine — not a flake. CI: e4d0e30a8d59a2c7c75730d1f3203f0256424a662d20df1c90be22b7609f3c39 local: 320493300753dc57278ec990ff678e8fd7fed1a82ac438ea8563acda65756efc count: 114 on both ColumnOperation.ClrType is a System.Type, and the reflection walk recursed into it — out through Assembly.Location / Module.FullyQualifiedName / CodeBase, which are ABSOLUTE PATHS to System.Private.CoreLib.dll. The digest therefore encoded where the .NET runtime happens to live: a /nix/store path here, something else on the runner. Same migration, different answer. It also made the described text ~168 MB, because Type -> Assembly -> Module cycles back into the reflection graph until MaxDescribeDepth cuts it off. The frozen test took 2s; it now takes 17ms. Fixed by treating a System.Type as a LEAF, described by FullName. That is the schema-relevant part — a column's CLR type matters, the file its metadata lives in does not. The process failure is worth more than the bug. I verified determinism by running the test three times ON ONE MACHINE and reported it as verified. Same-run determinism is not portability, and the check I ran could not have detected this defect in principle. CI caught what my verification was structurally incapable of catching. So the missing check is now a test rather than a habit: InitialCreate_Description_ContainsNoEnvironmentSpecificData asserts the described text names no assembly file and contains neither AppContext .BaseDirectory nor the runtime directory, plus a size ceiling as a cheap canary for the same recursion blowing up again. A future property that leaks environment fails there by name instead of failing as a mystery digest mismatch in CI. Re-verified after the fix, not assumed to still hold: * annotation mutant (ValueGenerationStrategy IdentityByDefault -> IdentityAlways) -> RED * default-value mutant (CredentialEpoch defaultValue 1 -> 0) -> RED * migration file byte-identical to HEAD afterwards * 3 consecutive local runs green (necessary, and — as established above — not sufficient on its own; the environment test is what covers the rest) AGENTS.md records both the Type-leaf rule and the lesson that repeated runs on one machine prove nothing about portability. Verified: 1255/1255 dotnet (114 application + 303 domain + 838 integration), 0 build warnings.
|
@codex please review It fixes a defect CI caught and I did not: the operation digest was machine-dependent.
Worth stating plainly: I had reported determinism as "verified" on the strength of three runs on one machine. That check could not have detected this in principle — same-run determinism is not portability. CI is now the evidence: the digest computes identically on NixOS and the runner, and both fence tests pass there. Re-verified after the fix rather than assumed to still hold — annotation mutant RED, default-value mutant RED, migration byte-identical to HEAD. This is the sixth change to this fence and the previous five each had a real finding, so please be adversarial rather than confirmatory:
|
…fiedName (#396) Comment only — no behaviour change, digest unchanged, 11/11 green. Pre-empts the obvious review suggestion, which would be a regression rather than a fix: AssemblyQualifiedName carries the assembly VERSION, so a routine dependency bump would move this digest with no schema change — the same false-red churn the Assembly.Location leak caused, arriving by a tidier route. Checked rather than asserted: every ClrType InitialCreate actually uses is a BCL primitive (Guid, string, int, long, decimal, bool, DateTime, DateTimeOffset, DateOnly), all from one assembly, so the FullName collision this trades away cannot occur here.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Adds coverage measurement for the four backend test projects. **Report only: there is no threshold, floor, or gate anywhere in this PR**, which is #776's explicit instruction and the order `web/vite.config.ts` followed for the SPA — measure first, decide whether to floor it later, from actuals. Closes #776 ## What lands `tools/coverage/collect.sh` builds once, runs each test project under coverlet, merges the per-project cobertura output through ReportGenerator into per-project and combined reports, and writes `coverage-out/SUMMARY.md`. The script is table-driven: one `ROWS` array of `name|csproj|meaning`, so a fifth test project is one row and nothing else. `--project <name>` runs one project alone for local iteration. `tools/coverage/coverlet.runsettings` carries exactly one interpretive filter, the EF migrations exclusion. Everything else stays at coverlet's default deliberately, so this first measurement is untuned. `.github/workflows/coverage.yml` runs it Mondays at 05:00, on `workflow_dispatch`, and on a `pull_request` scoped to the coverage tooling's own paths. `ci.yml` is untouched. `coverlet.collector` is versioned centrally per #684 with a bare reference in `tests/Directory.Build.props`, and the ReportGenerator tool joins the existing `dotnet-ef` manifest entry. All four `packages.lock.json` files are regenerated in the same commit as the manifest change. ## First measurement, 2026-09-12 | Project | Assemblies | Line | Branch | |---|---|---|---| | Domain | 1 | 81.4% (1480/1817) | 74.9% (651/869) | | Application | 3 | 13.9% (1666/11901) | 3.6% (114/3129) | | Integration | 4 | 93% (15698/16866) | 76.4% (3814/4989) | | AppHost | 1 | 100% (30/30) | 100% (2/2) | | Combined | 5 | 94% (15890/16896) | 79.5% (3968/4991) | Integration's four assemblies split unevenly: `Cluckwork.Api` 92.3%/72.8%, `Cluckwork.Application` 95.4%/81.7%, `Cluckwork.Domain` 83.6%/66.2%, `Cluckwork.Infrastructure` 94.9%/84.4%. ## Two readings these numbers invite, and both are wrong **The unit suites are not redundant, whatever the overlap says.** Domain and Application together add 162 covered lines beyond what Integration and AppHost already reach, over an identical 16,896-line denominator. Read naively that says 781 unit tests are nearly free to delete. Coverage cannot tell a test that asserts a domain invariant from one that happens to execute the same line while asserting an HTTP status, and #771 already found three fully-covered guards that survived the exact mutations they were named for. The redundancy question needs mutation testing; this is not it, and the decision record says so. **Application's 13.9% is not "the Application layer is 14% tested."** Integration measures `Cluckwork.Application` at 95.4%. The 13.9% is what the `Application.Tests` project's own 290 tests, largely validators and architectural guards, reach across the three assemblies they incidentally touch. ## The three decisions #776 asked to be made explicitly Per-project is the primary view and combined is secondary, because a single figure averages a unit-tested Domain against an end-to-end suite. Collection runs on a schedule rather than on every PR, because `Build and test` is already the ~570s critical path that #775 exists to shorten. Integration's EXECUTED-not-ASSERTED caveat is printed in the report itself rather than left for a reader to infer. All three are recorded in `docs/decisions/776-backend-coverage.md`. ## The one filter, and how it was checked `src/Cluckwork.Infrastructure/Persistence/Migrations` is 39,088 of the 76,977 lines of C# under `src/`. It is EF-generated, #407 freezes it, and every integration test executes all of it on container boot regardless of what that test asserts, so including it would swamp the measurement in both directions. Verified against the run's output, not assumed from the config: the Integration report lists zero `*.Migrations.*` classes, the raw cobertura XML contains zero `Persistence/Migrations` paths (so the exclusion drops files before cobertura records them, not just from the rendered report), and all 33 files in that directory are EF-generated — 16 timestamped migrations as `.cs`/`.Designer.cs` pairs plus `AppDbContextModelSnapshot.cs` — so nothing hand-written was dropped. ## Reviewing this The measurement reproduces: `tools/coverage/collect.sh`, or `tools/coverage/collect.sh --project Domain` for a fast single-project check. The `pull_request` path filter means this PR runs the new workflow on itself, so the CI run here is the proof the tooling works rather than a description of it. `ci.yml` is not modified and no security gate is touched. No user-visible behaviour changes, so no screenshots and no glossary or Help page update. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automated backend test coverage reporting across the test suite. - Coverage reports include per-project and combined summaries in HTML, Markdown, and text formats. - Reports can be generated for the full suite or an individual test area. - **CI/CD** - Coverage runs weekly, manually on demand, and for pull requests affecting coverage tooling. - Coverage results are report-only and do not block builds or releases. - **Documentation** - Added guidance explaining coverage measurement, scope, exclusions, and limitations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Review of #985 found the branch's own documentation contradicting what it enforces, plus two missing artifacts the repo's rules require. - .editorconfig's header and block comment still called using placement "a recorded preference at silent severity, not a gate" and said "nothing fails if it does not". Both are false at this head. Rewritten to state that any `:warning` entry in this file is a build error. - Dropped a dead `block_scoped:silent` from the fixture exemption. IDE0161 never reports on a compilation unit declaring more than one namespace, so the line configured nothing. Verified by building without it. - Corrected the migrations exemption's reason. It is there because `dotnet ef migrations add` scaffolds block-scoped namespaces, so it must keep working for migrations nobody has written yet, not because #407 freezes the existing ones. The old comment also claimed migrations are "never hand-edited", which AddBusinessRecordChronology contradicts. - AGENTS.md paragraph and docs/decisions/985-csharp-style-gate.md, both required for a build-gating convention. The record keeps the measurement that the gate's effective set is exactly {IDE0065, IDE0161}, and the correction that outside placement is not inherently safer. - move-usings-outside.py: the mixed-file branch collapsed blank lines across the whole file while a comment below it said a global pass is churn the change has no business making. Narrowed to the seam, shared with the other branch. The docstring also said 586 files and claimed mixed files are skipped; both were wrong. Re-running the fixed script on main reproduces all 589 files byte for byte.
Closes #396.
Cracked and dirty eggs are real, sellable stock on most farms — they go out at a discount rather than into the bin. Today the Daily Entry counts them, subtracts them from sellable, and then loses them: they never become a lot, never appear in stock, and can never be sold. This makes them stock.
The shape of the change
The counters bind to grade identities, not to grade names.
EggGradegains aDailyEntryKind(Manual/Cracked/Dirty). The two condition grades ship active and saleable on a new installation, and a partial unique index allows exactly one of each kind per farm while leaving Manual grades unconstrained. Renaming "Cracked" to "Segunda" therefore does not detach the counter — which a name match would.The entry snapshots what each counter resolved to, at the moment it first became official (
DailyEntry.CrackedGradeId/DirtyGradeId, written once at submit and never re-resolved). Two consequences that are the whole point:nullis a durable outcome ("resolved to nothing"), not a missing value. The snapshot columns deliberately carry no FK, so deleting a grade cannot rewrite history.Resolution requires the grade to be both active and saleable. Those are genuinely independent —
EggGrade.Deactivate()leavesIsSaleableset — so inactive-but-saleable is reachable and is treated as a loss.Enforcement is server-side, not a UI affordance
Excluding condition grades from the Grading pane is a convenience. The enforcement has to be in the handler, because the eligibility check a manual line passes is "active and saleable" with no kind restriction — and this feature makes Cracked and Dirty saleable.
So a stale or direct API client could name a Cracked id as a manual line, pass the exact-total check (it is only a sum), and produce two lots for one grade on one day.
ConditionGradeGuardrefuses it in the record and adjust handlers. Per #395 this belongs in the handler and not in a validator: the seeders callHandleAsyncdirectly and never runValidateAsync, so a validator-only rule is invisible to all five seeder tests.A bug in this PR's own first draft
AdjustDailyEntryHandlerbuilt its target set fromentry.Grades— the grade lines. Condition lots are not grade lines, so they read as target zero and would have been silently emptied on any adjustment.Worth flagging because of how it hid:
Adjusting_a_condition_counter_to_zero_empties_its_lotpassed before the fix. At zero the bug and the correct behaviour agree. The tests that actually distinguish them are the upward and unchanged cases.The mutation check also caught a surviving mutant on the zero-quantity new-lot guard — every existing test submitted a positive counter, so that path was never exercised and the guard was an untested claim.
Adjusting_a_day_that_never_had_cracked_eggs_still_succeedscloses it.Reports and History
A new Condition column: eggs that became stock without being hand-graded, counted only where that entry resolved them to a grade (
e.CrackedGradeId != null ? e.CrackedEggs : 0) — reading the counter alone would report stock for days recorded as a loss.Shown beside Sellable, not folded into it. Sellable is the hand-graded remainder and the figure grading counts down to; the two answer different questions and are added to get everything the day produced that can be sold. History shows
—for Draft entries, since nothing has resolved yet.Non-CI callers (#395)
Verified by reading each caller's payload, not by running anything — k6's
dailyEntryScreencounts a tolerated 422 as a passingcheck(), so a broken write workload would have shown a 100% green baseline.SimulationDataSeederEggLotscame fromGradesPerDailyEntryalone. Its counter math is structurally positive (cracked = 4 + d % 3,dirty = 2 + d % 2), so every submitted entry gained exactly two lots and a correct seed would have reportedFailed. Fixed with a separateConditionLotsPerDailyEntry.DemoDataSeedertools/simulation/k6/bundles.jsgradesarray, zeroes both counters, and records without ever submitting.worker.spec.ts'sGRADESis exactly the five Manual saleable grades;getByLabel("Cracked")stays unambiguous because Cracked was previously non-saleable (absent from the pane) and is now saleable but excluded by kind.manager.spec.tssubmits with cracked=1 and now silently exercises minting, adjust and void.Both fixtures gained a product mapped to a condition grade. Without one the feature reads as stock appearing that nothing can sell — ~180 lots of dead inventory in the simulation fixture — and stock reconciliation (#406) does not exist yet.
One guarantee that was implicit
Voiding empties the condition lots too.
VoidDailyEntryHandlerreverses whateverGetByDailyEntryLockedAsyncreturns — grade-agnostic, so condition lots are covered by construction rather than by a second code path. That is exactly why it was worth pinning: narrowing that query to grade lines (the shape this PR fixed in adjust) would leave a voided day carrying live, sellable stock with no entry behind it, silently, since nothing looks at a voided entry again.Docs
specs/product/GLOSSARY.mdgains Condition grade and Condition (Reports / History column); the SPA Help page and the en/es/tl catalogs are updated in the same PR. Spanish and Tagalog are machine-drafted pending native review, per #182.Verification
Deliberately out of scope
#406 (stock reconciliation for unaccounted cracked eggs) — filed separately and added to epic #14, gated to
AuthPolicies.AdminOnly. Raised while reviewing this work; it is a new write path, not part of making condition eggs into stock.This PR is the migration cutover
Added after the feature work, because merging this is the moment the #245 rule stops being true.
Until now a schema change was hand-folded into
InitialCreate. That was only safe while the application had never been deployed: no database had applied that file yet, so rewriting it was free. This PR is the last change permitted to do it. From the commit that merges it, a database exists which has already appliedInitialCreate, EF will never re-run it, and a hand-folded column is a column that silently does not exist.Worth stating why that deserves a rule rather than a note: it surfaces as broken behaviour, not as a migration error. #399's amendment did exactly this to the dev database and presented as a failing login.
The load-bearing part is not the prose.
MigrationSecurityReviewTestsassertedAssert.Single(AllMigrations())— so the first post-cutover PR to add a migration would have gone red regardless of what any document said. Documenting the cutover without moving that fence would have been a no-op.It is replaced by a guard that still has teeth rather than deleted:
InitialCreatemust stay first and keep its recorded id20260801190854_InitialCreate. Regenerating it mints a new timestamp, which reorders it behind the migrations that depend on it and desynchronises__EFMigrationsHistoryon every deployed database — now a red test instead of a failed deploy. Ordering is asserted on the migration ID, never on list position:AllMigrations()comes fromAssembly.GetTypes(), whose order is explicitly unspecified, so a positional check would pass or fail on reflection ordering.Relaxing that one test is safe because every other assertion in the file already fans out across the whole history (
AllInsertDataOperations/AllSqlOperations) — the credential and reference-data guarantees extend to post-cutover migrations for free.Docs updated where the rule is actually stated or implied:
AGENTS.md— the canonical bullet.README.mdgains a "Changing the database schema" section, which did not previously exist. The rule lived only in AGENTS.md, so a human following the README had nothing telling them to add a migration. Includes Fail closed when the design-time migration connection is unset or insecure #318's fail-closed design-time connection and its loopback opt-out, sinceef migrations addneeds both.deploy/README.md's #364 line is deliberately untouched — it describes what was true of that release at the time, not a live rule.Mutation-checked: pointing the pinned id at a different timestamp fails the test; restored and rebuilt back to green.
Verified after the feature work, by execution
Beyond the unit/integration suites, all three non-CI callers were actually run:
complete=true, 880 egg lots = exactly 5.0 per submitted entry (3 graded + 2 condition) against real Postgres at full 90-day depth. Pre-fix this reportedFailed.manager.spec.tssubmitting with cracked=1 then adjusting and voiding.unexpected_status: 0;sales_create_order/sales_add_itemboth 201 against the 5-product catalog.DAILY_ENTRY_WRITE_PROB = 0.12and never fired in the 20s smoke, and a 422 sits in the tolerated set, sounexpected_statuswould have stayed 0 while the workload silently stopped landing. The DB snapshot is what proves it, not the k6 exit code.