Repository navigation
[obvious-fleet] fix(substitutions): canonical table offers after hydration and household-allergen screening - #180
Conversation
…able Map PR #114's reviewed substitution pairs onto the base branch's canonical ingredient vocabulary instead of the legacy app ingredient-id space: exact name/alias/fdc-id/USDA-reference evidence only, fail-closed on ambiguous or conflicting matches, self-swaps and duplicate key/role offers dropped, per-key composition map with the compound allergen/diet closure, unknown composition fail-closed to known=false. Regenerated resource: substitutions-v2.json ships 70/279 reviewed pairs covering 126/317 endpoints (the legacy app-id table shipped 35/40) with 50 swaps carrying known substitute composition; 209 pairs drop with a named unmapped-key audit. 32 converter tests; regeneration from the pinned pairs+vocabulary is byte-identical. Co-authored-by: Sam Gu <sgu07966@gmail.com>
…llergen rechecks SubstitutionTable decodes schema-2 canonical-key resources (rejects v1), looks swaps up by canonical original key, and screens the substitute's composition per eater: forbidden classes refuse naming the class, unknown composition fails closed to ask-the-user under any active restriction, unknown restriction ids add no forbids. SubstitutionService loads the v2 resource, translates between app ingredient ids and canonical keys through the catalog identity bridge (legacy id -> canonical keys -> ingredient rows), assembles deterministic offers, and answers the cook-mode recheck from real composition facts — the cooking guide no longer reports unknown for every substitute. substitution-check v2 pins audit counts, canonical-key integrity, recheck/compositions agreement, the all-pairs x all-restrictions sweep, and the reviewer compound-closure examples (yogurt carries dairy, tofu carries soy); 13 XCTest cases cover table semantics on an inline miniature document. Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
@codex review |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 63f8301bcd
ℹ️ 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".
| init(db: DatabaseQueue) { | ||
| self.db = db | ||
| self.userData = UserDataRepository(db: db) | ||
| self.table = Self.loadTable() | ||
| let bridged = Self.loadBridge(db: db) | ||
| self.keysByAppID = bridged.keysByAppID | ||
| self.appIDsByKey = bridged.appIDsByKey |
There was a problem hiding this comment.
The service snapshots recipes_catalog_ingredient_bridge during AppDependencies construction, but AppDatabase.setup() only runs migrations; catalog hydration and RecipeCatalogIngredientBridge.rebuild happen later in warmBundledContentIfNeeded() on a background task. Consequently keysByAppID and appIDsByKey are empty on every normal launch and are never refreshed, so offerableSwaps, hasSubstitutions, and substituteIsAllergen return no offers/rechecks even after the catalog is populated. Load the bridge after hydration or make the service query/refresh it when hydration completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1cf8a83 — the service now reads recipes_catalog_ingredient_bridge per query instead of snapshotting it in init, so offers appear as soon as warmBundledContentIfNeeded() rebuilds the bridge. Regression SubstitutionServiceTests.testOffersAppearAfterCatalogHydrationEvenWhenBuiltBeforeIt (a8a7106) builds the service before hydration, hydrates the shipped bundle, and failed on run 38832423820 without the fix (offers empty) — it passes on run 38833215409 (head 1317e36, 544 tests / 0 failures).
| func substituteIsAllergen(for substituteId: Int64) -> Bool? { | ||
| guard let table else { return nil } | ||
| let restrictions = (try? userData.fetchHealthProfile())?.normalizedDietaryRestrictionIDs ?? [] | ||
| guard !restrictions.isEmpty else { return false } |
There was a problem hiding this comment.
If fetchHealthProfile() throws (for example, a transient database/read error), this try? converts the failure into an empty restriction set and the function returns false, allowing the cooking flow to execute the substitution. The caller explicitly treats nil as the fail-closed/blocked result, so profile-read failures should return nil rather than being treated as a household with no restrictions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1317e36 — a failed fetchHealthProfile() read now returns nil (the caller's blocked verdict) instead of degrading to an empty restriction set and returning false. Regression testHealthProfileReadFailureBlocksTheSubstitute (a8a7106) drops health_profile and failed on run 38832423820 without the fix (got false, wanted nil) — passes on run 38833215409.
| func substituteIsAllergen(for substituteId: Int64) -> Bool? { | ||
| guard let table else { return nil } | ||
| let restrictions = (try? userData.fetchHealthProfile())?.normalizedDietaryRestrictionIDs ?? [] | ||
| guard !restrictions.isEmpty else { return false } | ||
|
|
||
| var sawUnknown = false | ||
| for key in keysByAppID[substituteId]?.sorted() ?? [] { | ||
| guard let composition = table.document.compositions[key] else { | ||
| sawUnknown = true | ||
| continue | ||
| } | ||
| guard composition.compositionKnown else { | ||
| sawUnknown = true | ||
| continue | ||
| } | ||
| let carried = Set(composition.allergenClasses).union(composition.dietClasses) |
There was a problem hiding this comment.
The recheck only reads normalizedDietaryRestrictionIDs; it never reads HealthProfile.parsedAllergenIds, even though onboarding and settings persist the household's explicit allergen ingredient selections there. A user who flags a specific ingredient such as peanut but has no dietary restriction therefore gets false at the early return and can execute a substitute containing that allergen, and the same exact-ingredient allergy is ignored even when a diet restriction is present. Include the persisted allergen IDs in the screening or fail closed when they cannot be resolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1317e36 — the recheck now screens the union of HealthProfile.parsedAllergenIds and dietaryExcludedIngredientIds before the diet early-return, so explicit allergens block their substitutes whether or not a diet is active. Regressions testExplicitAllergenBlocksTheSubstituteEvenWithoutADiet and testDietExcludedIngredientIDsBlockTheirSubstitutes (a8a7106) failed on run 38832423820 without the fix and pass on run 38833215409.
The iOS-only helper above the Linux-mirrored slice was declared -> TasteEvent while constructing a TasteEventRecord, so the macOS test build failed with seven type-mismatch errors the mirror cannot see (the slice region regenerates its own helper from the committed preamble). Match the preamble's return type. Co-authored-by: Sam Gu <sgu07966@gmail.com>
…old allergens, fail-closed rechecks Adds SubstitutionServiceTests covering the three Codex findings on PR #180: offers must survive building the service before warmBundledContentIfNeeded hydrates the catalog (r4289584058); an explicit household allergen and a diet-excluded ingredient must block their substitutes even with no diet active (r4289584073); a failed health-profile read must block the swap instead of reading as no restrictions (r4289584068). Also unblocks the macOS test build: the v2 table tests called .isEmpty on an optional String and shadowed $0 inside an immediately-invoked closure (SubstitutionTableV2Tests:125/:218/:222). Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
Inside an async test method, DatabaseQueue.read resolves to the async overload, so the bridge drain needs await. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
swaps(forCanonical:) returns dictionary-built order, which macOS and Linux decode differently; the evidence run 38832423820 shows macOS returning [binder..., butter...]. Pin the sorted pair ids. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…e-hydration builds AppDependencies builds SubstitutionService before warmBundledContentIfNeeded hydrates the catalog and rebuilds recipes_catalog_ingredient_bridge, so the init-time snapshot left keysByAppID/appIDsByKey empty for the whole launch and no swap was ever offered (Codex r4289584058). Read the small bridge select per query instead. Verified: SubstitutionServiceTests.testOffersAppearAfterCatalogHydrationEvenWhenBuiltBeforeIt fails on run 38832423820 (offers empty with a bridged ingredient present) and passes with this change. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
…te recheck substituteIsAllergen read only normalizedDietaryRestrictionIDs, so a peanut-allergic household with no diet got false and could execute a peanut substitute (Codex r4289584073): screen the union of the persisted allergen ingredient ids and the diet's excluded ingredient ids first, whether or not a diet is active. A failed health-profile read became an empty restriction set through try? and returned false, allowing the swap (Codex r4289584068): return nil — the caller's blocked verdict — instead. Verified on run 38832423820: testExplicitAllergenBlocksTheSubstituteEvenWithoutADiet, testDietExcludedIngredientIDsBlockTheirSubstitutes, and testHealthProfileReadFailureBlocksTheSubstitute all fail without this change and pass with it. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
obvious-autobuild
Bot
changed the title
…to the join Hosted macOS CI run 38838436665 failed the test targets on two more join drifts, both caught only on Apple hosts (the portable suites do not build the app targets): - CookReviewFixRegressionTests expects CookModeViewModel.lastPersistError and the foreign-session abandonment from the #157 review round (927c062, 'abandon foreign sessions on start and surface receipt errors'); the join's merge of startOrResume() kept the pre-review body that silently orphaned a different recipe's still-cooking session. Port the reviewed declaration and abandon block onto the catalog-aware startOrResume() — at most one session cooking, failed persists surfaced (LeChef section 4.2). - CatalogCookFlowTests built a hand-reviewed unknown-substitute offer with the pre-#180 Substitution memberwise init; the canonical-key round added the required substituteKey field, whose documented hand-built value is the empty string. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
fleet-packet: burst9-fridgeluck-fix180-codex
Head:
1317e365onobv/burst6-fridgeluck-subs-canonical(baseobv/burst6-fridgeluck-catalog-recommend). This round fixes the three Codex review findings left on head63f8301(reviewed 2026-10-16T00:17Z) and turnsbuild-and-testgreen.As reviewed, the canonical substitution table never offered a swap in the running app, and a household's explicit allergens never blocked a substitute. Both are fixed, with failing-first regressions.
What changed and why
SubstitutionServicesnapshottedrecipes_catalog_ingredient_bridgeduring init, but the bridge is rebuilt later inwarmBundledContentIfNeeded(), sokeysByAppID/appIDsByKeywere empty on every normal launch and no swap was ever offered. The service now reads the small bridge select per query (1cf8a83a), which is also correct if hydration ever re-runs.substituteIsAllergen(for:)read onlynormalizedDietaryRestrictionIDs, so a peanut-allergic household with no diet gotfalseand could execute a peanut substitute. It now screens the union of the persistedparsedAllergenIdsanddietaryExcludedIngredientIdsbefore composition screening, with or without an active diet (1317e365).try? fetchHealthProfile()turned a read failure into "no restrictions" and returnedfalse(allow the swap); callers treatnilas blocked. The recheck now returnsnilon any profile-read failure (1317e365).SubstitutionServiceTests(a8a71063, one test per finding, built on the real launch order and the shipped bundle), plus compile repairs the macOS test build needed before anything could run:SubstitutionTableV2Tests:125(.isEmptyon an optional String),:218/:222($0shadowed inside an immediately-invoked closure), and the taste-store helper return type cherry-picked from trunk9de29858(c29a6402, same fix as PR feat(ios): weekly horizon for the LeChef plan — one allocator, repeats priced, leftovers carried #178).testLookupByCanonicalKeyassumed JSON decode order, butswaps(forCanonical:)returns dictionary-built order that macOS and Linux decode differently. The test now pins the sorted pair ids (1c7d2d76).Findings → tests → fixes
a8a71063)testOffersAppearAfterCatalogHydrationEvenWhenBuiltBeforeIt1cf8a83atestExplicitAllergenBlocksTheSubstituteEvenWithoutADietfalse≠truewith no diet set1317e365testDietExcludedIngredientIDsBlockTheirSubstitutesfalse≠truefor the vegan dairy set1317e365testHealthProfileReadFailureBlocksTheSubstitutefalse, wantednil1317e365How I verified (commands and results)
e58063fe, fixes not yet applied):gh pr checks 180 --repo SamGu-NRX/FridgeLuck --watch→ build-and-test fail;gh run view 38832423820 --repo SamGu-NRX/FridgeLuck --log-failed→ exactly 5 failures: the four regression tests above plus the pre-existingtestLookupByCanonicalKeyordering assumption. All other 539 tests passed.1317e365):gh pr checks 180 --repo SamGu-NRX/FridgeLuck --watch→ build-and-test success,Executed 544 tests, with 0 failures; all fourSubstitutionServiceTestscases andSubstitutionTableV2Testspass.swiftc -parse-level review only — macOS CI was the real compiler; the first push's compile error (asyncdb.readoverload) and everything after were caught and fixed on CI as expected.Failing without the fix — run 38832423820 (tests only, fixes reverted)
Passing with the fix — run 38833215409 (final head 1317e36)
What is not verified
build-and-testjob was the only compiler and test runner; nothing here type-checked the app target locally. The substituted-swaps UI itself (SubstitutionSheet/CookingGuideView) was not manually exercised on a device or simulator; the service behavior under it is covered by the new tests.dead-code-advisoryand CodeRabbit are advisory/skipped gates for bot drafts; their final state is visible on the checks list.Assumptions
SubstitutionService.swift, substitution tests, and theTasteEventStoreTestshelper.AppDependenciesis untouched — the wiring thread burst8-fridgeluck-goal-path owns it on another branch.true(refuse, shown in the transcript) rather thannil, because the household declared it — composition is settled by the user's own declaration.dietaryExcludedIngredientIdsis the diet's derived exclusion set (e.g. the vegan dairy ids), so the union check makes those substitutes block outright instead of depending on bridge resolution and a composition lookup.Human author: Sam Gu (sgu07966@gmail.com)
Carried forward from the burst6 packet (updated to final head
1317e365):fleet-packet: burst6-fridgeluck-subs-canonical
Head:
1317e365onobv/burst6-fridgeluck-subs-canonical(same commits asobv/subs-canonical-keys-v2, which already carries draft PR #177 — if both appear, close one; this packet ref is the canonical description).Bridges PR #114's reviewed substitution table onto the base branch's canonical ingredient vocabulary (329 entries) instead of the legacy app ingredient-id space, and re-arms the cook-mode allergen recheck with real composition facts.
What changed and why
The shipped substitution resource indexes pairs by legacy app ingredient IDs and offers only 35 of 279 reviewed pairs. The reviewed table no longer depends on the app's id space; the catalog's canonical keys are the stable join. The legacy wiring also made cook mode report every substitute as unknown composition (
{ _ in nil }), so proposal-driven swaps always failed closed — correctly, but uselessly.scripts/substitutions/convert.pymaps pair endpoints to canonical keys via exact evidence only (canonical names, recorded aliases, fdc-id equality, recorded USDA source names), failing closed on ambiguous/conflicting matches. Self-swaps created by canonicalization and duplicate (original, substitute, role) offers are dropped (first pairId wins). Emits a per-key composition map with the compound allergen/diet closure (vocabulary curated classes ∪ mapped facts store); keys nothing covers fail closed tocompositionKnown: falsewith empty class lists. Regeneration is deterministic (byte-identical from the pinned inputs).substitutions-v2.jsonships 70/279 reviewed pairs covering 126/317 endpoints (legacy: 35 pairs / 40 ingredients), 50 swaps with known substitute composition, 209 dropped pairs recorded with named unmapped-key audit.SubstitutionTable(Swift) decodes schema 2 canonical-key resources and rejects non-v2; lookup by canonical original key; screening refuses naming the forbidden class, fails unknown composition to ask-the-user under any active restriction, and unknown restriction ids add no forbids.SubstitutionServiceloads v2, translates app ingredient ids ↔ canonical keys through the catalog identity bridge (legacy id → canonical keys → ingredient rows, min id for determinism), assembles deterministic offers, and answers the cook-modesubstituteIsAllergenrecheck from real composition facts.CookingGuideViewis wired to it.substitution-checktool + XCTest cover the semantics; README documents the v2 regen flow.How I checked (commands and results — at 63f8301)
python3 -m pytest scripts/substitutions/ -q→ 32 passed (canonical evidence, ambiguity/conflict fail-closed, self-swap/dup rejection, composition merge + screening, CLI, generated resource).cmpbyte-identical.bash apps/ios/Tools/substitution-check/run.sh(Linux swiftc, compiles the production table + checker against the real resource) → OK — 70 swaps, 126 mapped endpoints, 329 composition keys, sweep allowed=597 askUser=260 refused=53. Includes: no self-swaps/dups, composition coverage for every endpoint, recheck↔compositions agreement, vegan refusal names dairy (yogurt), soy-free refusal + gluten-free allowance (tofu), egg→banana resolves, schema gate rejects v1, determinism.swiftc -parseclean on all five changed Swift files.63f8301(run 38816535943) failed on the base stack's pre-existingRecipeCatalogRecommendTestserror, not on this PR's files; the green hosted run for this round is 38833215409 (see above).Assumptions (carried forward)
substituteKeyrides along in the legacySubstitutionmodel for display.buttersubstitute; the reviewer dairy example usesyogurt(6 shipped swaps), which carries the same compound closure.Substitution.substituteIdsemantics preserved: the resolved app row id, not a canonical key string.Human author: Sam Gu (sgu07966@gmail.com)
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact