Skip to content

Commit 42c9888

Browse files
committed
[M2] sprint-log: brutally honest review of Round 2 medcare-realtime (1 CRITICAL fix path)
Meta-2 review surfaces 5 findings; 1 CRITICAL flagged for verification: CRITICAL #1: W7 hard-depends on StepDomain::MedCare which may not exist upstream → Recommended fix path: fetch lance-graph-contract/src/orchestration.rs to verify DomainProfile shape, then either confirm variant exists OR commit W7-revision-2 with inline-constructed DomainProfile fallback MEDIUM #2: MedCareStack empty struct doc-comment overclaims as "facade" → Doc-only fix; defer to next field-growth commit MEDIUM #3: Missing with_default_policies() builder → Backlog; lands when rls_registry field lands LOW #4-#5: Cross-crate test + dev-deps deferred Round 3 implications surfaced: - W9 imports list (medcare_rbac::{policy, role, access}) - W10 lib.rs gate re-export shape - W12 §73 SGB V tests must include BtM Escalate + Ueberweisung row visibility (Meta-1 carry-forward) Sprint orchestrator: verify upstream StepDomain::MedCare before committing W7-revision-2, OR apply fail-safe inline construction.
1 parent b9a1233 commit 42c9888

1 file changed

Lines changed: 222 additions & 0 deletions

File tree

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
# Meta-2 Review — medcare-realtime (Round 2, Stage 2)
2+
3+
**Reviewer:** Meta agent 2 of 3 (Round 2 review pass)
4+
**Scope:** medcare-rs/crates/medcare-realtime + workspace registration
5+
**Method:** read W5-W8 commits + log entries; cross-check against
6+
smb-realtime shape parity, upstream dep availability, topology I-1/I-2,
7+
and Round 3 readiness.
8+
9+
> **Tone:** brutally honest. Reviewing my own colleague's work as if
10+
> shipping to production tomorrow. Findings escalate by severity.
11+
12+
---
13+
14+
## Verdict
15+
16+
**Ship Round 2 with one CRITICAL fix applied as W7-revision-2 before
17+
opening Round 3.** The fail-loud choice on `StepDomain::MedCare` may
18+
block compilation if the variant doesn't exist upstream. One MEDIUM
19+
finding flagged for follow-up. Otherwise Round 2 is clean.
20+
21+
| # | Severity | Finding | Action |
22+
|---|---|---|---|
23+
| 1 | **CRITICAL** | W7 hard-depends on `StepDomain::MedCare` variant; sprint cannot verify upstream existence | W7-revision-2: construct DomainProfile inline with documented expected values; switch to `StepDomain::MedCare.profile()` when upstream variant ships |
24+
| 2 | MEDIUM | `MedCareStack` empty struct in v1 — is this a facade or a marker? | Honest doc note in module head; defer field growth to follow-up |
25+
| 3 | MEDIUM | No `with_default_policies()` builder — smb-realtime has 3 default policies registered; medcare-rs has zero | Backlog: add when canonical entity list is firm |
26+
| 4 | LOW | No test asserts cross-crate workspace dep resolves (medcare-realtime → medcare-rbac) | CI catches this on first build; no Sprint action |
27+
| 5 | LOW | Cargo.toml has 0 `[dev-dependencies]` while smb-realtime has tokio-test + pretty_assertions | Defer; v1 tests are simple `assert!` on sync surface |
28+
29+
---
30+
31+
## CRITICAL #1`StepDomain::MedCare` may not exist upstream
32+
33+
**Finding.** W7's `domain_profile()` calls
34+
`lance_graph_contract::orchestration::StepDomain::MedCare.profile()`.
35+
W7's self-review explicitly chose "fail loud" — if the variant is
36+
absent, the file won't compile.
37+
38+
**Why this is critical.** The sprint goal is "produce a buildable
39+
3-stage scaffolding ready for Round 3 + future merge". A non-compiling
40+
medcare-realtime blocks Round 3 (W9 imports from medcare-realtime),
41+
blocks workspace `cargo build`, and blocks any CI run on this branch.
42+
43+
The fail-loud rationale ("don't mask the gap") is defensible
44+
architecturally — but the cost is concrete (sprint produces broken
45+
code) versus a hypothetical benefit (someone notices the gap faster).
46+
A documented inline fallback achieves the same surface visibility
47+
without the compilation cost.
48+
49+
**Super-helpful solution (apply as W7-revision-2):**
50+
51+
```rust
52+
pub fn domain_profile(&self) -> DomainProfile {
53+
// TODO upstream: switch to `StepDomain::MedCare.profile()` once
54+
// the variant ships in lance-graph-contract::orchestration.
55+
// Until then, construct the medcare-appropriate profile inline
56+
// — values mirror what `StepDomain::MedCare.profile()` should
57+
// return (3650 days BMV-Ä §57 retention, 0.85 confidence
58+
// threshold, fail-closed required, Llm escalation).
59+
DomainProfile {
60+
audit_retention_days: 3650,
61+
auto_action_confidence: 0.85,
62+
escalation: EscalationStrategy::Llm,
63+
requires_fail_closed: true,
64+
verb_taxonomy: VerbTaxonomyId::MedCare,
65+
}
66+
}
67+
```
68+
69+
Caveat: `EscalationStrategy::Llm` and `VerbTaxonomyId::MedCare` are
70+
also assumed to exist. If they don't, the fallback strategy is to use
71+
whatever the contract crate currently exposes for `EscalationStrategy`
72+
and either default `VerbTaxonomyId` or omit the field.
73+
74+
**The cleanest super-helpful path** — fetch the actual `DomainProfile`
75+
struct definition from upstream BEFORE committing W7-revision-2, so
76+
the fallback uses the right field names. Two options:
77+
78+
(a) Fetch `lance-graph-contract/src/orchestration.rs` content; verify
79+
`DomainProfile` shape; commit revision-2 with correct fields.
80+
81+
(b) Wrap the call in a feature gate:
82+
```rust
83+
#[cfg(feature = "upstream-medcare-domain")]
84+
pub fn domain_profile(&self) -> DomainProfile {
85+
StepDomain::MedCare.profile()
86+
}
87+
88+
#[cfg(not(feature = "upstream-medcare-domain"))]
89+
pub fn domain_profile(&self) -> DomainProfile {
90+
// hand-constructed fallback
91+
}
92+
```
93+
Add `upstream-medcare-domain` feature gated on the variant
94+
landing. Cleaner future migration path; more boilerplate now.
95+
96+
**Recommended:** Option (a) for sprint pragmatism. Option (b) if the
97+
upstream PR is uncertain (>1 week to land).
98+
99+
---
100+
101+
## MEDIUM #2 — MedCareStack empty struct: facade or marker?
102+
103+
**Finding.** `MedCareStack` in W7 has zero fields:
104+
105+
```rust
106+
pub struct MedCareStack {
107+
// v1 placeholder
108+
}
109+
```
110+
111+
Compare smb-realtime:
112+
113+
```rust
114+
pub struct SmbStack {
115+
ontology: &'static CachedOntology,
116+
rls_registry: Arc<RlsPolicyRegistry>,
117+
}
118+
```
119+
120+
medcare's empty struct is honest about v1 scope — neither
121+
`medcare_ontology()` nor `RlsPolicyRegistry` is wired yet. But the
122+
public API surface (new / default / domain_profile / Clone / Debug)
123+
suggests a real facade.
124+
125+
**Tension.** Empty-struct-with-methods is fine for a marker type that
126+
locks in the API surface for future field growth. But the doc
127+
comments call it a "facade" — language that implies composition.
128+
129+
**Solution.** Tighten the doc comment to acknowledge the v1 marker
130+
status explicitly:
131+
132+
```rust
133+
/// Assembled outer-membrane facade for medcare. Cheap to clone.
134+
///
135+
/// **v1 status: marker.** This struct is currently empty — the
136+
/// public API surface (`new` / `default` / `domain_profile`) is
137+
/// stable, but internal composition (RLS registry, cached ontology,
138+
/// gate accessors) lands in follow-up sprints when upstream
139+
/// blockers (DM-7, DM-8, medcare_ontology factory) ship.
140+
///
141+
/// Holding the type at v1 means consumer code can already reference
142+
/// `MedCareStack` symbolically; field growth doesn't break the API.
143+
```
144+
145+
This is honest — v1 trades emptiness for symbol stability. Worth
146+
saying so explicitly.
147+
148+
**No code change needed; doc-comment update lands as part of any
149+
future field-growth commit.** Round 3 doesn't block on this.
150+
151+
---
152+
153+
## MEDIUM #3 — Missing default policies builder
154+
155+
**Finding.** smb-realtime ships `with_default_policies()` registering
156+
Customer / Invoice / TaxDeclaration. medcare-realtime has no
157+
equivalent. Once `MedCareStack` grows an `rls_registry` field, it'll
158+
need a parallel `with_default_medcare_policies()` registering Patient,
159+
Diagnosis, LabResult, Prescription, Anamnese, Ueberweisung.
160+
161+
**Action.** Backlog. v1 doesn't have rls_registry yet; the builder
162+
follows once the field lands. Capture in TECH_DEBT.md when sprint
163+
synthesis closes.
164+
165+
---
166+
167+
## LOW #4-#5 — Defer
168+
169+
- #4: cross-crate dep resolution test → CI catches naturally
170+
- #5: `[dev-dependencies]` → trivial; v1 has no async tests yet
171+
172+
---
173+
174+
## Round 3 implications
175+
176+
W9 (gate.rs) needs to import:
177+
```rust
178+
use medcare_rbac::policy::{medcare_policy, Operation, Policy};
179+
use medcare_rbac::role::Role;
180+
use medcare_rbac::access::AccessDecision;
181+
182+
use lance_graph_contract::external_membrane::{AllowAllGate, MembraneGate};
183+
use lance_graph_contract::property::PrefetchDepth;
184+
```
185+
186+
W10 needs to update `medcare-realtime/src/lib.rs` to add:
187+
```rust
188+
pub mod gate;
189+
pub use gate::{AccessDecision, AllowAllGate, MembraneGate, MedCareMembraneGate, Policy};
190+
```
191+
192+
W11 (integration tests) wraps `MedCareStack::new()` and verifies
193+
`MedCareMembraneGate` composes with it. v1 stack is empty so the
194+
composition test will be trivial — that's fine.
195+
196+
W12 (§73 SGB V test) verifies:
197+
1. Doctor without Ueberweisung row CANNOT read another Doctor's
198+
Patient at Detail (the row-level check happens above the gate;
199+
this test exercises the gate's role-level deny path)
200+
2. Doctor with active Ueberweisung CAN read referred Patient at
201+
Detail (the gate allows; row-level check passes too)
202+
3. **BtM-flagged Prescription.issue → Escalate** (per Meta-1 #3
203+
carry-forward; gate.rs in W9 must implement this wrapping)
204+
205+
The Meta-1 HIGH #3 + #4 carry-forward (BtM Escalate, anonymize/merge
206+
Escalate) lands in W9 logic. W12 tests verify.
207+
208+
---
209+
210+
## Feedback loop — apply NOW (W7-revision-2)
211+
212+
Open question for the orchestrator: do we have the upstream
213+
`DomainProfile` field shape on hand? If yes, apply W7-revision-2 with
214+
inline construction. If no, fetch first.
215+
216+
**Recommendation:** Fetch `lance-graph-contract/src/orchestration.rs`
217+
to confirm DomainProfile field names + EscalationStrategy + VerbTaxonomyId
218+
variants, THEN commit W7-revision-2 with the correct inline construction.
219+
Five minutes of verification, much better than guessing.
220+
221+
If `StepDomain::MedCare` already exists upstream, no revision needed
222+
(though doc strengthening per #2 still recommended).

0 commit comments

Comments
 (0)