Uh oh!
There was an error while loading. Please reload this page.
feat(S14): close 4 secondary gaps (SDOH rebalance, review:apply, confidence, SMART A+B) - #23
Merged
Merged
Conversation
Adds the four S14 spec artifacts at the start of the feature branch so the implementation commits have their full provenance in git. - grill-secondary-gaps.md: 9-question grill, S14/S15 split - prd-s14.md: 35 user stories + 10 implementation decisions + 5 testing decisions - implementation-plan-s14.md: 4-commit task-by-task plan - tasks/todo.md: active checklist (S11 archived, S14 active) Out of scope (deferred to S15): Risk agent v2 rubric + LLM-variance root cause. Tracked in verification-s13.md §6.
Adds AHC-HRSN screening Observations for james-okafor, angela-diaz, pop-0010 (positive) and robert-kim, pop-0005 (explicit-negative). Brings SDOH distribution from 1/16 positive (trivially gameable) to 4/16 positive + 2/16 explicit-negative (not gameable). Updates labels.json _meta.labelingRules.sdoh to document the new rule. Spec: prd-s14.md D2 + grill-secondary-gaps.md §2
Adds apps/api/src/scripts/apply-clinician-review.ts that reads labels.clinician-review.json (downloaded from the review:render HTML form) and writes overrides back into data/eval/labels.json. Sets source: 'clinician' on touched rows, populates the clinicianOverride slot, prints a CHANGELOG summary. New npm run review:apply script. The review:render half shipped in S9 C2; this commit closes the round-trip so the eval-report 'Status' disclosure can move from 'all dev' to 'N clinician-validated (X%), M dev-labeled (Y%)'. Spec: prd-s14.md D3 + grill-secondary-gaps.md §3
Adds apps/api/src/agents/confidenceScorer.ts with 3 pure scoring functions (Risk/CareGap/SDOH) + 1 derivation helper (Action Planner task confidence = min of contributing findings). Adds confidence: 0-1 to every finding in apps/api/src/agents/agent.ts (RiskOutput.flags, CareGapOutput.gaps, SdohOutput.barriers, ActionPlannerOutput.tasks). Adds applyConfidence helper in apps/api/src/agents/citationValidator.ts. Wires the scorer into apps/api/src/routes/analysis.ts so the validated output carries confidence (collected per-finding, then propagated into the Action Planner's tasks via deriveActionPlannerTaskConfidence). Why heuristic, not model self-report: the model is already biased on Risk (see verification-s13.md §4 LLM variance); we don't compound that with biased self-reported confidence. Heuristic scores are auditable, deterministic, and reproducible. Test files (actionPlannerAgent.test.ts, careGapAgent.test.ts, riskAgent.test.ts, sdohAgent.test.ts, analysis.test.ts, cdsCardMapping.test.ts, cdsHooks.test.ts, mockAnalysis.ts) updated to fill confidence: 0.5 placeholders so the new required schema field compiles — the real number lands via the scorer in production. Spec: prd-s14.md D4 + D5 + grill-secondary-gaps.md §4
A (app-side, developer guard): new apps/api/src/middleware/smartAuth.ts
decodes the Bearer token, verifies signature against the existing
serverSecret, checks exp/aud/scope, throws 401/403 with structured
reason codes. Mounted on HAPI-touching routes in index.ts.
B (HAPI-side, the real fix): new docker-compose.yml env vars
(hapi.fhir.security.oauth.enable_jwt_validation=true +
public_key_location) + bind-mount of apps/api/src/smart/keys/
smart-public.pem. HAPI now rejects unauthenticated calls at the FHIR
boundary; verification-s14.md documents the curl 401/200 evidence.
The lightweight B config trusts any token signed by the configured
public key — correct for the POC's client_credentials flow but not
the right shape for production multi-actor SMART. verification-s14.md
notes the production handoff (point HAPI at a real SMART auth server).
Spec: prd-s14.md D6 + D7 + grill-secondary-gaps.md §5
Notes:
- Signing-key decision: the in-process token server in apps/api/src/smart/
tokenServer.ts issues access tokens as HS256 JWTs signed with its
serverSecret (default 'caresync-dev-authz-server-secret-do-not-use-in-
production'), NOT with the client's RSA keypair. The smartAuth middleware
therefore verifies against serverSecret via verifyAccessToken() — passing
the RSA public key would reject every legitimate token the app mints.
The RSA public key file at apps/api/src/smart/keys/smart-public.pem is
for HAPI's separate OAuthAuthorizationServletFilter (its config expects
RS256 with a public key, not HS256 with a shared secret); the two
verifiers target different components and use different keys by design.
- requireAuth kept alongside smartAuth on every HAPI-touching route. The
developer guard (smartAuth) validates the SMART access-token claim shape
(signature, exp, audience, scope); the login guard (requireAuth) validates
the CareSync session JWT. Either failing surfaces 401/403. Layering means
existing route tests (which use login Bearer tokens) continue to pass
unchanged — the test apps build their own Express stack without
smartAuth mounted, so the new middleware never sees a login token in
tests. SmartAuthErrorHandler is mounted once at the bottom of index.ts
so every smartAuth throw across all HAPI-touching routes resolves to
the documented JSON { error: 'smart_auth_failed', reason } envelope.
- Phase D deferral: the stock hapiproject/hapi:v7.2.0 image does not
include the OAuthAuthorizationServletFilter, so the env vars + public
key bind-mount in docker-compose.yml are present but inert — HAPI
still returns 404/200 on Patient reads regardless of Authorization
header. Verified by running docker compose up -d hapi-fhir with the new
config: 'curl -i http://localhost:8080/fhir/Patient/maria-chen' still
returns HTTP/1.1 404 (the patient data was re-imported successfully,
so the 404 isn't a data-missing artifact; it's HAPI not enforcing).
The 401-without-token + 200-with-token curl evidence is deferred
pending a HAPI image that ships the security filter, or a custom
Dockerfile derived from hapi-fhir-jpaserver-starter. The app-side
middleware (A) is fully exercised by smartAuth.test.ts's 5 cases —
Phase D's gap is a docker-compose image issue, not an API bug.
- Test files unchanged: all 5 existing route test suites (patients,
analysis, population, governance, quality, team, tasks, sdoh,
carePlans, alerts) keep using login Bearer tokens in their fixtures
and continue to pass — they build their own Express app without
mounting smartAuth, so the developer guard never runs in tests.…sclosure
Completes the eval-report half of S14 (the architecture §"Architecture"
listed eval.ts as a modified file but no S14 commit touched it). Two
additive changes:
1. `computeMetrics.ts`: SDOH now gets a full `ConfusionMatrix` (was
agreement-rate-only). Wired through `AgreementMetrics.matrix` and
tallied via the existing `tallyConfusionMatrix` helper. Same
TP/FP/TN/FN shape as Care Gap + Risk. Backwards-compatible at the
JSON-summary level (new field on the existing `sdoh` key).
2. `eval.ts` `renderMarkdown` + `buildJsonSummary`:
- Status disclosure now data-driven: counts `source: "clinician"`
rows in the label file. Pre-S14 said "DEV-LABELED BASELINE,
NOT CLINICIAN-VALIDATED" unconditionally; now says "X of N
clinician-validated (Y%), M of N dev-labeled (Z%)". The
"Not clinician-validated (GD8)" caveat only renders when the
count is 0.
- SDOH section now prints the confusion matrix in addition to the
agreement rate. The misleading pre-S14 `_meta.limitations` caveat
("only one positive example") is replaced with the S14-truthful
version that names the 5-row rebalance.
Confidence-bucketed accuracy sub-tables remain deferred (open
follow-up) — surfacing them requires plumbing `confidence` through
`PatientFindings` so `computeMetrics` can bucket findings, a
non-trivial change that's blocked on a non-trivial post-S14 refactor
(scheduled for S15 alongside the LLM-variance work).
Regenerated docs/eval-report.md captured at the same SHA shows the
new signals: SDOH agreement 66.7% (2/3), SDOH matrix
TP=1/TN=1/FP=0/FN=1, Status reads "0 of 16 clinician-validated
(0.0%), 16 of 16 dev-labeled (100.0%)". Only 3/16 patients scored
this run (OpenAI quota exhausted on patients 4-16) — see
verification-s14.md §4 for the full evidence + the
follow-up-required caveat.
Note: docs/eval-report.json (machine summary, consumed by
governance/service.ts) is NOT committed per the S14 hard-constraint
that untracked test artifacts stay out of git history; the .md is
the human-readable evidence surface.
Spec: prd-s14.md D8 + grill-secondary-gaps.md §6verification-s14.md covers the 4-row verification matrix from implementation-plan-s14.md §Phase E: - Row 1 (SDOH rate off 100%): ✅ 66.7% (2/3) — undershoots target slightly because only 3/16 patients scored this run (OpenAI quota exhausted mid-eval; pre-existing limitation, not S14 regression). - Row 2 (review:apply round-trip): ✅ 2/2 tests pass. - Row 3 (confidence buckets): ❌ deferred — requires plumbing confidence through PatientFindings so computeMetrics can bucket. Underlying data is in place (Commit 3); only the eval-report rendering layer is missing. - Row 4 (401/200 SMART curl): ❌ deferred — stock hapiproject/hapi:v7.2.0 image does NOT include OAuthAuthorizationServletFilter (verified by docker inspect + curl). Plan's curl tests would only succeed once HAPI is rebuilt from hapi-fhir-jpaserver-starter or pointed at a real SMART auth server. review-s14.md covers the two-axis review (Standards + Spec) following the review-s13.md precedent: - Standards: 2 mid-slice fixes (eval.ts follow-through at c6587f1, server.ts → index.ts mount correction), 6 judgement calls left as-is. - Spec: 2 of 4 verification matrix rows deferred (above). Slice closes 3 of 5 secondary gaps outright and makes real progress on #5. Both files cite the S13b reversion log so future readers can find why the Risk agent was deliberately untouched. Spec: prd-s14.md D8 + grill-secondary-gaps.md §6
External code review (Spec axis) caught a partial implementation: grill-secondary-gaps.md §3 says a row is "touched" (and should flip source: 'clinician') when "any dim with a non-endorse choice OR any non-empty notes." The pre-fix code only checked `hasOverride || hasAbstain`, so a reviewer who picked "endorse" on every dim but added a clinical note would not mark the row clinician-validated. That defeats the eval-report's "X of N clinician-validated" disclosure whenever a clinician uses the notes field. Fix in apply-clinician-review.ts: add a `hasNotes` check (any careGap/risk/sdoh notes non-empty after trim) and include it in the source-flip guard. Test updates: - The existing round-trip test's james-okafor fixture had non-empty 'Endorsed' notes while asserting source stayed 'dev' — that pinned the bug. Updated the fixture to use empty notes (preserves the original "all-endorse-stays-dev" assertion but now under the correct precondition). - New test: "flips source to clinician when all dims endorse but notes are non-empty" — pins the grill §3 trigger. Test suite: 281/281 (was 279/280 with the pre-existing flake; the flake happened to pass this run). tsc clean. No spec change — the implementation now matches grill §3.
External review via the repo's `code-review` skill ran Standards + Spec sub-axes in parallel. Aggregated into review-s14.md as a new top section above the existing self-review: Standards axis: - 0 hard documented-standard violations (CLAUDE.md is honored). - 7 baseline smells (Fowler ch.3) — all judgement calls, all left as-is with reasoning per smell: Duplicated Code + Data Clump in apply-clinician-review.ts, Speculative Generality in confidenceScorer.ts / smartAuth.ts / citationValidator.ts, Data Clump in seed-patients.ts, mild Speculative Generality in population.ts. Spec axis: - 1 real defect: apply-clinician-review.ts skipped the grill §3 "any non-empty notes" trigger for flipping source. Fixed in b3f8167 (which the previous commit already landed). - 1 documented design tradeoff: smartAuth.ts verifies against serverSecret (HS256) rather than the public key (RSA). Forced by the existing token server's HS256 design; the follow-up (point HAPI at a real SMART auth server) is in verification-s14.md §6 #2. - 1 plan hygiene: scoreRiskFlag expected value typo in the implementation plan (0.5 should be 0.7). Test asserts 0.7 (correct); not fixing the plan doc. - 0 scope creep; 0 looks-implemented-but-wrong items. Final test count: 281/281 (was 279/280 with the pre-existing flake; the flake passed this run). tsc clean. Slice ready to PR.
…ry-gaps/ Follows the established convention from S2-S12 (one <date>-changelog.md per branch under docs/superpowers/specs/). Per-commit summary + verification + open follow-ups + migration notes — all matching the content already in verification-s14.md and review-s14.md but in the changelog format the S9 precedent established (S9 is the most recent changelog that exists).
Caught by live smoke test (this session, after the PR was open): the smoke test of gap #4 hit a regression that 281/281 unit tests couldn't catch — every test file builds its own Express app without mounting smartAuth, so the mount-order bug in index.ts was invisible to the suite. The regression: smartAuth was mounted via app.use('/api/patients', smartAuth, createPatientsRouter(fhirService)) which meant smartAuth ran BEFORE the route's inner `requireAuth`. A login JWT (signed with auth/jwt.ts's JWT_SECRET) would hit smartAuth first, fail signature verification (smartAuth expects SMART tokens signed with serverSecret), and 401 with `invalid_signature` before requireAuth ever saw it. Net effect: every legitimate API caller with a login JWT — the UI, scripts, the test-via-server path — was locked out of every HAPI-touching route. Two-part fix: 1. smartAuth no-double-auth pass-through. If `req.auth` is already set by an upstream middleware (requireAuth), smartAuth calls next() without validating the SMART shape. This is a 4-line additive check at the top of the middleware function. Preserves all 5 existing unit tests (none of them pre-set req.auth) and adds a 6th test "passes through when req.auth is already set by an upstream middleware (no double-auth)" that pins the new behavior with a fake upstream. 2. Mount order: smartAuth is now wrapped INSIDE each router, AFTER requireAuth. New helper `wrapRouterWithSmartAuth(router, smartAuth)` in smartAuth.ts does `router.use(smartAuth)` post-construction. index.ts mounts become app.use('/api/patients', wrapRouterWithSmartAuth(createPatientsRouter(fhirService), smartAuth)) — same shape for all 10 HAPI-touching prefixes. The error handler remains mounted globally at the bottom of index.ts. Net result: login JWT → requireAuth passes → smartAuth no-double-auth passes → route handler runs. SMART-shape tokens still hit smartAuth's full validation (when requireAuth is taught to accept them too — tracked as follow-up #7 in verification-s14.md). Live smoke test (npm run dev): - Login JWT → GET /api/patients/maria-chen → 200 ✓ (was 401) - No token → 401 {"error":"Missing bearer token"} (caught by requireAuth) - Garbage SMART-shape token → 401 {"error":"Invalid or expired token"} (caught by requireAuth; smartAuth never runs) Test suite: 282/282 (was 281; the new pass-through test is the +1). tsc clean. Note: this is a real regression introduced by Commit 4 (5e73c68). The Commit 4 self-review claim that "tests are unchanged because each test file builds its own Express app without mounting smartAuth" was true at face value but missed the production mount order. The unit tests for the middleware itself were always green; the regression lived in the integration glue (index.ts). This is a useful lesson for S15: mount-order bugs don't surface from per-route test apps. Follow-up #7 (verification-s14.md): requireAuth should also learn to accept SMART-shape tokens so the two tiers fully interoperate. Out of scope for this fix — the immediate regression is closed.
Three doc updates to match the now-published reality (vs the pre-smoke-test state when verification-s14.md + the changelog were first written): 1. verification-s14.md §6 — two new follow-ups: - #7 (FIXED in f8d0862): the mount-order regression the live smoke test caught. Records the methodology lesson (per-route unit tests are blind to mount-order bugs; future slices need an integration smoke test against npm run dev). - #8 (out of scope): requireAuth itself still rejects SMART-shape tokens, so SMART-token-only callers would 401 at requireAuth before smartAuth ever runs. Asymmetry left by the f8d0862 fix. 2. review-s14.md — added a "Post-review update (commit f8d0862)" paragraph that discloses the regression caught after the PR was open and the fix. 3. changelog — added commits 9-11 (changelog + regression fix), the "Live smoke tests" section, follow-up #7 + #8, and the verification count update (281/281 → 282/282 with the new pass-through test). PR description was also updated (separately, via gh pr edit) to include a "🚨 Regression caught post-PR (and fixed in f8d0862)" section + the new follow-ups + the live smoke-test evidence. Unrelated pre-existing uncommitted changes in apps/web/ (PatientDetail.tsx + MyPatients.test.tsx) are NOT part of this commit; they were left in the working tree from an earlier session.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes 3 of 5 secondary gaps from the HL7 evaluation and makes real progress on the 4th. The 5th (Risk PPV / LLM variance) is explicitly out of scope (S15 owns it).
docs/eval-report.mdline 38apply-clinician-review.ts+ 3 tests (round-trip, notes-trigger, unknown-patient) +npm run review:applyconfidenceScorer.ts(3 pure scorers + 1 derivation helper) + 5 tests +applyConfidencewired inroutes/analysis.tsriskAgent.ts.Why
The 9-question grill (
docs/plans/caresync-ai/grill-secondary-gaps.md) resolved the S14/S15 split + the per-question decisions (SMART A+B = Q7+Q8, confidence heuristic = Q6, patient picks for SDOH = Q1, etc.). The PRD (docs/plans/caresync-ai/prd-s14.md) carries D1–D10 implementation decisions + T1–T5 testing decisions. The implementation plan (docs/plans/caresync-ai/implementation-plan-s14.md) carries the 4-commit task-by-task plan + Phase E verification matrix.How
11 commits on
feature/s14-secondary-gaps(base169174b= S12 coordinator-grid merge):e61382cspec(S14) — grill + prd + implementation plan + active todob807aaffeat(S14): rebalance SDOH labels (3 positive + 2 explicit-negative)ab3baf4feat(S14): review:apply (the missing apply half)3cbe8dcfeat(S14): per-finding confidence via bundle-evidence heuristic5e73c68feat(S14): SMART enforcement A+B (app middleware + HAPI config)c6587f1feat(S14): eval.ts surfaces SDOH TP/FP/TN/FN + clinician-validated disclosurea31fb6ddocs(S14): verification + code reviewb3f8167fix(S14): review:apply flips source on non-empty notes (grill §3)2a5b1c9docs(S14): review-s14.md aggregates external code-review5752744docs(S14): changelog under docs/superpowers/specs/f8d0862fix(S14): smartAuth runs AFTER requireAuth + no-double-auth pass-through (post-Commit-4 regression caught by live smoke test)Each implementation commit is independently revertable; full-PR revert reproduces pre-S14 state. The 4-commit task table from the plan is unchanged.
🚨 Regression caught post-PR (and fixed in
f8d0862)The 281/281 unit-test suite did NOT catch a mount-order bug in
index.ts: smartAuth was mounted AHEAD of the route's innerrequireAuth, so login JWTs (signed withauth/jwt.ts'sJWT_SECRET) hit smartAuth first, failed signature verification (smartAuth expects SMART tokens signed withserverSecret), and returned{"error":"smart_auth_failed","reason":"invalid_signature"}beforerequireAuthever saw them. Net effect: every legitimate API caller with a login JWT was locked out of every HAPI-touching route.The unit tests couldn't catch this because each
routes/*.test.tsbuilds its own Express app without mounting smartAuth — the test pattern is correct for per-route coverage but blind to mount-order bugs. The Commit 4 self-review claimed "tests are unchanged because each test file builds its own Express app without mounting smartAuth"; that was true at face value but missed the production mount order. Methodology lesson recorded inverification-s14.md §6 #7: future slices need at least one integration smoke test against the runningnpm run devserver, not just per-route unit tests.Fix in
f8d0862:if (req.auth) return next()pass-through at the top of smartAuth (preserves all 5 original tests; adds a 6th test pinning the new path).wrapRouterWithSmartAuth(router, smartAuth)helper.index.tsmounts becomeapp.use('/api/patients', wrapRouterWithSmartAuth(createPatientsRouter(fhirService), smartAuth))— same shape for all 10 HAPI-touching prefixes.Follow-up #8 (
verification-s14.md §6): requireAuth itself still rejects SMART-shape tokens, so a SMART-token-only caller would 401 at requireAuth before smartAuth ever runs. Out of scope for the immediate regression fix.Verification
npx tsc --noEmit— cleannpx jest— 282/282 (43 suites)npm run eval— regenerateddocs/eval-report.mdshows 3 of 4 plan E1 acceptance criteria (SDOH rate off 100%, SDOH matrix visible, Status disclosure data-driven). 4th (confidence buckets) is deferred — the data is in place from Commit 3 but rendering requires aPatientFindingsplumbing refactor that is not in S14 scope. Seedocs/plans/caresync-ai/verification-s14.md§6 feat(S1): Walking Skeleton — login, My Patient Panel, live FHIR reads #1.curl /fhir/Observation/<id>confirmed all 5 new screenings present in HAPI/tmpmutatedlabels.jsoncorrectly, then restoredf8d0862Open Follow-ups
Tracked in
docs/plans/caresync-ai/verification-s14.md§6:hapi-fhir-jpaserver-starteror point at a real SMART auth server) — closes gap feat(S4): agent-graph canvas + analysis cache/replay #5 fullyanalysis.test.ts— 1-line fix, not load-bearingf8d0862Spec
docs/plans/caresync-ai/grill-secondary-gaps.md(9-question grill, S14/S15 split)docs/plans/caresync-ai/prd-s14.md(D1–D10, T1–T5)docs/plans/caresync-ai/implementation-plan-s14.md(Phases A–E)Evidence
docs/plans/caresync-ai/verification-s14.md— verification matrix, command evidence, TDD traces, post-PR live smoke-test results including the mount-order regressiondocs/plans/caresync-ai/review-s14.md— two-axis review (Standards + Spec) with the one defect caught + fixed inb3f8167and the post-PR regression caught + fixed inf8d0862docs/superpowers/specs/feature-s14-secondary-gaps/2026-07-08-changelog.md— per-commit changelog🤖 Generated with Claude Code