ogar-fma/ogar-cpic: address Sonnet review findings (laterality, render_classid delegation, 0x1000 marker, test hardening) - #228
Conversation
Follow-up to the merged reference-surface crates, from a 3-agent review: - ogar-fma: "first metatarsophalangeal joint" was Laterality::Unpaired, but it exists on both feet and a gout flare localizes to one side — a consumer must be able to prompt L/R. Corrected to Paired. (The enum defines Unpaired as single/midline; the MTP joint is neither.) - ogar-fma: render_classid now DELEGATES to ogar_vocab::app::render_classid (the single source of the concept<<16|prefix bit math) instead of re-implementing it — removes the latent-drift risk app.rs's doc warns against. A test pins agreement with the canonical fn. - ogar-fma: the render_classid test used 0x1000 labelled "q2 render skin". 0x1000 is the reserved V3-adoption monitor marker (ports.rs) that can never be a real port prefix, and q2's prefix awaits an operator ruling — so the test no longer bakes it in; it uses the core default 0x0000. - ogar-fma: added an FMA-id provenance note (FMAID xref; the viscera cluster numerically because FMA entered them as one adjacent block — not fabrication). - ogar-cpic: the nars_truth monotonicity test only checked Strong-vs- Optional, leaving Moderate unpinned. Now asserts the full Strong > Moderate > Optional chain on both axes. 12 lib tests + 2 doctests green; fmt + clippy -D warnings clean.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_09992d26-8f6a-4708-bd2e-15c48543b7c5) |
…↔full notes Integration plan Phase 0 + Phase 1 (anatomy axis), consumer-grounding side: - ogar-fma: new FmaPartition::ArticularSurface (routes on ANATOMICAL_STRUCTURE) + three cartilage target surfaces the HEAD-US score reads C from — femoral trochlea, anterior talar dome, anterior distal humeral epiphysis. fma_id left None (honesty fence; resolves via graph:fma, never fabricated). - ogar-fma + ogar-cpic: "Curated subset of the full FMA/CPIC" module notes — the subset↔full contract (curated row = classid address; graph:fma / graph:cpic = reasoned-over corpus). Name only, no license text (commitment #9). 12+ lib tests + doctests green; fmt + clippy -D warnings clean.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_04e6f144-8a3c-4198-8d87-a4913731488a) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b177aae5ad
ℹ️ 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".
| //! what does it chain to?" A pair absent here is not absent from CPIC — it lives | ||
| //! in `graph:cpic` and resolves there. |
There was a problem hiding this comment.
For genuinely unsupported inputs, this promises that every pair omitted from the curated table exists in the full CPIC graph, which is false—for example, the crate itself tests ABCB1/digoxin as absent, and callers may supply arbitrary gene-drug pairs. This also contradicts resolve's contract that None means no actionable guideline. State that an omitted pair may be available in graph:cpic, rather than guaranteeing that it resolves there.
Useful? React with 👍 / 👎.
The subset↔full note over-promised that any pair omitted from the curated table "lives in graph:cpic and resolves there" — false for arbitrary pairs (many gene-drug pairs have no CPIC guideline at all, e.g. the ABCB1/digoxin the tests assert absent) and contradicting resolve()'s None contract. Reworded: an omitted pair *may* have a guideline in graph:cpic, but many resolve in neither; resolve returning None means "no actionable guideline in this curated table," and graph:cpic is the authoritative check.
Follow-up to the merged
ogar-fma+ogar-cpicreference surfaces (#227), applying a 3-agent Sonnet review of the diff.Fixes
"first metatarsophalangeal joint"wasLaterality::Unpaired, but it exists on both feet and a gout flare localizes to one side — corrected toPaired. The enum definesUnpairedas single/midline; the MTP joint is neither.render_classidused0x1000labelled "q2 render skin" in a test.0x1000is the reserved V3-adoption monitor marker (ports.rs) that can never be a real port prefix, and q2's prefix awaits an operator ruling — the test no longer bakes it in; it uses the core default0x0000and pins agreement with the canonical bit-math fn.render_classidnow delegates toogar_vocab::app::render_classid(the single source ofconcept<<16 | prefix) instead of re-implementing it — removes the latent-drift riskapp.rs's own doc warns against.nars_truthmonotonicity test now asserts the fullStrong > Moderate > Optionalchain on both axes (previouslyModeratewas unpinned).Not changed (verified clean by the review)
Bit-packing, domain routing, the Genetics no-mint gate, producer→consumer direction, firewall split, and
ogar-fmavsogar-fma-skeleton(complements, not duplicates) all passed.12 lib tests + 2 doctests green;
cargo fmt+cargo clippy --all-targets -- -D warningsclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Xv5wRqar5sC5QcXVpkn94n
Generated by Claude Code