feat: CDS Hooks 2.0.1 service + the OpenAPI integration contract - #469
Conversation
…outcomes
Adds GET /cds-services (public), POST /cds-services/{id} and its /feedback
sibling (both bearer-gated), so an open measure gap can reach a clinician
through the community standard rather than a bespoke endpoint.
Cards render a COMPLETED run's outcome; they never trigger an evaluation. No
prefetch is declared, because none is evaluated -- usageRequirements says so in
the machine-readable contract. Three refusals are load-bearing and
mutation-checked: critical is unrepresentable in CdsCard, since WorkWell is
supplementary to WebChart and may not tell a clinician not to proceed; a
suggestion is offered only where the order code has an APPROVED terminology
mapping read from the store, so cms122/cms125 carry information and a link
rather than a demo-grade CPT for one-click creation; and a patient with no
finalized outcome gets an informational card, never an empty card list, which
at the point of care would read as "no gaps".
Card uuids derive from (runId, subjectId, measureId), so feedback correlates by
recomputation and the endpoint needs no schema change. The measure-outcome to
card mapping is ours: HL7 blesses PlanDefinition/$apply -> RequestOrchestration
-> cards, and the DEQM care-gap to card leg is unpublished.
/cds-services is outside /api/, where authorize ends in permitAll, so the two
new rules are mandatory rather than a refinement -- asserted both as a unit call
and end-to-end through the worker, and both fail when either is removed.
Suite 1964, 0 fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed against drift GET /api/v1/openapi.json now serves a hand-authored 3.1.1 document covering the PROMISED surface only -- /api/v1/compliance, the three /cds-services operations, health and version -- and stating that the ~40 internal /api/** routes are excluded because they carry no stability promise. Public: reading a contract should not require credentials. The guard is what makes hand-authoring defensible, and it is two-way. Every (path, method, status) the document declares is produced by a real request through the real worker, and every response the tests observe is declared. A documented route that is not routed fails with "documented but NOT ROUTED" -- which is precisely how ARCHITECTURE.md came to assert a springdoc OpenAPI document for a year after the JVM was retired. All three directions mutation-checked: deleting the route, removing a produced status, and requiring an absent property each fail the intended assertion. Redocly lints the document in CI (pinned, telemetry off, no ignore file) because it catches a class the contract test cannot -- and did on the first run: five uses of `nullable`, which OpenAPI 3.1 removed in favour of type unions. The five remaining warnings are explained in spec.ts rather than silenced; four are operations that genuinely have no 4xx, and their 405 belongs to the path, not the GET, which the coverage test correctly refused to let us mis-model. Also adds the first smoke coverage of the integration surface: the document, CDS discovery, one real card assembly and one v1 compliance read. None was probed before. Suite 1969 across the affected files, 0 fail. Redocly: 0 errors, 5 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ment /api-docs is a top-level route, deliberately outside (dashboard) and unauthenticated: the document it renders is public, and an integrator should be able to read the contract before they have credentials. It fetches with a plain fetch rather than lib/api/client, whose token attachment and silent-refresh behaviour would be wrong on a page nobody is logged into. Hand-rolled rather than vendored, and not only on the no-new-dependency rule: swagger-ui-react peers on react ">=16.8 <19" and this app is on React 19, so the React integration does not exist for us; Swagger UI's dark mode is a hard-coded html.dark-mode class that would fight this app for ownership of <html>; and Scalar and Redoc both default to a CDN script that the CSP and offline demo rule out. The page inherits the Enterprise Health brand and dark mode instead (ADR-004). The trade is no try-it-out console -- a copyable curl instead, which for a bearer-token API is about as useful and needs no proxy. The load-bearing test is that no operation is silently dropped: an untagged operation lands under "Other" rather than vanishing, which is the same defect class as a documented-but-unrouted path one layer up. A 3.1 type union renders as "string | null" rather than "string", so a nullable field reads as nullable. Frontend: lint clean, 186 tests pass, build clean, /api-docs prerendered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… stale ARCHITECTURE claims ADR-067 (CDS Hooks cards render a completed evaluation and never trigger one) and ADR-068 (the OpenAPI document covers the promised surface only, guarded by a routed-path test), plus docs/CDS_HOOKS.md as the contract an integrator reads. Two ARCHITECTURE corrections, one of which was the point of the work: §9 asserted "The OpenAPI document (workwell.swagger.enabled=true) advertises version v1" -- a springdoc property from a backend retired in #109 PR4 -- while §7 did not mention /api/v1/compliance at all, so the file simultaneously claimed a document it did not serve and omitted the one contract it does. Writing the document also surfaced that §9 promised /api/version returns `uptime`; it does not. STANDARDS_CONFORMANCE gains two rows written through the conformance skill. Both name who graded what: CDS Hooks 2.0.1 is structurally conformant and SELF-graded, because no external grader exists (the community validator is JSON Schemas last pushed 2018, the sandbox is ungraded, Inferno has no CDS Hooks kit); and the gap-to-card leg is recorded as a LOCAL mapping, since HL7 blesses PlanDefinition/$apply -> RequestOrchestration -> cards but nobody publishes a route from a DEQM care gap to a card. Neither row is justified by certification: ONC (b)(11) and HTI-1 do not name CDS Hooks. AI_GUARDRAILS gains §1.1 -- a card is a rendering, every clinical statement in it is the CQL outcome verbatim, and systemActions is never emitted. S7 is rewritten with its per-part table split along the line that matters: delivering a finding, offering an order, and did-anyone-act move to built, while evaluating data supplied on the request stays not built -- that is step 2, and prefetch is where it would go. The sequence diagram draws that leg dashed. Also closes a real gitignore gap: docs/transcripts/ was only covered by basename globs, so `docs/transcripts/2026-08-16 call.md` was committable. Verified with git check-ignore. Backend suite 1970, 0 fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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
workwell/backend-ts/src/cds/cards.ts
Line 78 in 3843242
Replace the literal NUL byte with an escaped separator
This string contains an actual NUL byte rather than an escaped source representation. Git consequently classifies the entire TypeScript file as binary (git diff reports Binary files differ), hiding this core card-building implementation and all future edits from normal review and text-based tooling. Preserve the same hash input with an escaped form such as "\0" or "\u0000" so the file remains ordinary text.
ℹ️ 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".
| } catch (err) { | ||
| // Best-effort at the response boundary: an audit failure must not turn a correct answer into a 500. | ||
| console.error(`WORKWELL_ALERT cds-hooks audit write failed: ${String(err)}`); |
There was a problem hiding this comment.
When appendAudit fails during a feedback POST, this catch suppresses the failure and the handler still returns 200, even though the audit event is the only persisted record of the accepted/overridden action. A transient database failure therefore permanently loses the feedback and tells the CDS client not to retry; feedback writes must propagate the failure (even if invocation/read auditing remains best-effort) to preserve the repository's no-exceptions audit invariant.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in 3af2d83 — the most serious finding, and my own reviewer raised it independently.
Feedback now returns 503 audit_write_failed rather than 200, with recorded / of so a retry is informed, and the body states retry is safe (feedback is idempotent by (card, outcome, outcomeTimestamp)). Invoke deliberately stays best-effort, and auditCds now returns a boolean so the two call sites make that difference explicit instead of sharing one swallow.
You identified exactly why the asymmetry matters: for invoke the cards are still correct, so a 500 would be the worse outcome; for feedback the audit event is the persistence (ADR-067 d10 — the whole reason the endpoint needed no schema change), so a swallowed failure was a silent no-op that told the client never to retry. Recorded as ADR-067 decision 13, pinned by a test that injects a throwing store.
| : await buildComplianceCards(found.rows, { | ||
| subjectId: found.subjectId, |
There was a problem hiding this comment.
When a WebChart request supplies a bare patient ID such as 4821, lookup intentionally resolves it to the internal key wc|4821, and this line passes that internal key into order construction. toServiceRequest() consequently emits subject.reference: "Patient/wc|4821"; | is not valid in a FHIR resource id and the reference also does not identify the client's Patient/4821, so approved-order suggestions cannot be applied by a conforming CDS client. Keep the internal subject ID for outcome lookup/card identity, but use the original hook patientId for the suggested FHIR resource.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 3af2d83. This is the one that only bites on the deployment the feature exists for, which is what makes it a good catch.
buildComplianceCards now takes the hook patientId alongside the internal subjectId, and the emitted resource is re-pointed via a small withClientSubject helper: Patient/4821, not Patient/wc|4821. Kept out of toServiceRequest itself deliberately — GET /api/orders/proposals shares that function and the internal id is the right answer there, so changing it at the source would have fixed one caller by breaking another.
Card and suggestion identity still derive from the internal subject id, so feedback correlation is unaffected. Pinned by a test that builds with subjectId: "wc|4821" / patientId: "4821" and asserts both the reference and that no pipe survives into a FHIR id. CardOptions.patientId is required rather than optional, so the compiler forces every future call site to state which id it means.
| properties: { | ||
| card: str("The `card.uuid` from the invoke response."), | ||
| outcome: { type: "string", enum: ["accepted", "overridden"] }, | ||
| acceptedSuggestions: { type: "array", description: "Required when `outcome` is `accepted`.", items: { type: "object", properties: { id: str("The `suggestion.uuid`.") } } }, |
There was a problem hiding this comment.
CDS Hooks 2.0.1 defines acceptedSuggestions as an array of suggestion UUID strings, but this schema advertises objects containing an id. Generated clients and the new reference page will therefore produce values such as [{"id":"..."}] (or even [{}]) instead of ["..."], making the published integration contract nonconformant despite the runtime's permissive array-only check. Change the OpenAPI schema, TypeScript wire type, documentation, and tests to use string items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked this against the specification and I do not think it is right, so I have left the schema as it is — flagging rather than silently declining.
CDS Hooks 2.0.1 defines acceptedSuggestions as an array of json objects, not of strings. From the Feedback section:
acceptedSuggestions| CONDITIONAL | array | An array of json objects identifying one or more of the user’s AcceptedSuggestions. Required foracceptedoutcomes.
and each AcceptedSuggestion is:
id| REQUIRED | string | Thecard.suggestion.uuidfrom the CDS Hooks response.
Every example in the spec is the object form, e.g.
"acceptedSuggestions": [ { "id": "e56e1945-20b3-4393-8503-a1a20fd73152" } ]So the published schema (array of objects with a required id) matches the contract, and changing it to string items would make our document non-conformant rather than fix it — which is the direction that would actually hurt, since the whole point of publishing it is that a generated client can be trusted.
Two things your comment did surface that were real, and are fixed in 3af2d83: the runtime check was permissive (it required the array but not the ids), and the audit event did not record which suggestions were accepted. Both now do. Happy to reopen if you are reading a different revision of the spec — a pointer to it would settle it.
…and an engine failure is not a clinical claim
Follow-up to the code review and Codex's PR comments. Seven substantive fixes,
four vacuous guards replaced, and the doc claims corrected where they overstated.
Correctness and safety:
- Feedback returns 503, not 200, when its audit write fails. The audit event IS
the entire persistence for that endpoint, and the spec gives feedback no
response body to signal partial success, so a swallowed failure told the client
never to retry and lost the accepted order silently. Invoke stays best-effort,
since its cards are correct regardless. Both reviewers raised this
independently; CLAUDE.md's "every state change writes audit_event -- no
exceptions" decides it.
- A FAILED evaluation is reported as ours. PARTIAL_FAILURE rows are served, and a
subject whose evaluation threw persists as MISSING_DATA + evaluationError --
which deriveCell rendered as "No record on file" and nextActionFor turned into
"Collect the missing documentation", asserting a fact about the patient when our
engine threw. Now a "could not be evaluated" card, info, no suggested order.
- A suggested ServiceRequest references the id the CLIENT sent. On a live tenant
it carried Patient/wc|4821, which names nothing the client can resolve and is
not a legal FHIR id, so the suggestion could not be applied (Codex). Re-pointed
in the CDS layer only; /api/orders/proposals is unchanged.
- Feedback is bounded: 100 entries per request, userComment truncated at 8000
chars. Each entry is an append to the append-only ledger by a machine
credential, and userComment is the first unstructured clinical prose in
audit_events -- now noted in the PHI posture.
- card.uuid is a conformant UUID (version 8 per RFC 9562, variant 10xx). CDS
Hooks types it as a UUID and a validating client would reject a bare hash.
- refRunId was dead: no caller ever set detail.runId. The invoke event now records
the run and maps each emitted uuid to its measure, which makes the documented
correlation claim true -- it previously overstated by a step, since inverting a
uuid meant a search over every run in the subject's history.
- Removed the authorize rule for /api/v1/openapi.json: handleOpenApi runs before
the auth gate, so the rule could never be consulted while reading as the control.
Guards that could not fail, now replaced:
- The alias assertion was `assert.notEqual(authorize(...).ok && alias === PATH,
true)` -- the && is always false, so it passed for any implementation. Now
asserts the aliases return the worker's 501 not_implemented, with a token, so it
proves "not served" rather than "not reachable".
- The `critical` loop asserted a value against a type that permits only those two
values. Replaced with the priorityFor mapping, which is failable.
- assert.ok(Array.isArray(cardUuids)) pinned a field name and nothing else, and
`.find()` returned the OLDEST event despite being named `latest`. Now
deep-equals the uuids the client received.
- The openapi fixture had no APPROVED mapping, so the suggestion subschemas -- the
deepest in the document -- were never schema-validated. Added.
Also: the reachability walk starts from the paths rather than every component (a
dead schema referencing another marked its target live) and follows $ref into its
target; parameter schemas are walked; endpointsOf skips non-operation path-item
keys; and a test pins dispositionFor's OPEN set equal to order-proposal's AT_RISK
set, an undocumented coupling the suggestion dedupe silently depends on.
One finding NOT acted on: Codex's claim that acceptedSuggestions is an array of
UUID strings. The spec defines it as "an array of json objects identifying one or
more of the user's AcceptedSuggestions", each with a REQUIRED `id` string, and
every example is [{"id": "..."}]. The schema was already right.
And a self-inflicted one worth recording: a sed -i over this UTF-8 source replaced
a space with a literal NUL byte, twice, and it reached a commit -- it typechecked,
every test passed, and the only symptom was grep reporting the file as binary.
Fixed, with a test that would have caught both.
backend 1976 tests / 0 fail; frontend 187 / lint clean / build clean; redocly 0
errors.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The feedback check required `acceptedSuggestions` to be present but not that its
entries identify anything, so `[{}]` passed and would have been recorded as an
accepted suggestion nobody can resolve. The spec makes AcceptedSuggestion.id
REQUIRED -- it is the card.suggestion.uuid we emitted -- so this is now enforced,
with `[{}]` and `[{"id":""}]` pinned as 400s.
Surfaced while replying to a Codex comment that argued acceptedSuggestions should
be an array of strings rather than objects. That part is not right -- the spec
defines "an array of json objects identifying one or more of the user's
AcceptedSuggestions", each with a required `id`, and every example is the object
form -- but checking it properly exposed that our runtime was more permissive than
the schema we publish.
Suite 1976, 0 fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…w round CDS_HOOKS.md "Limits, stated" now says an invocation is a bounded but unindexed read rather than a constant-time one, and points at #470 -- an integrator reading the contract should not have to discover that. Names the four other callers that share the scan, so it reads as a pre-existing property rather than a property of this endpoint. JOURNAL records what the review changed: the two findings both reviewers reached independently, the four guards of mine that could not fail, the one Codex comment I declined with the spec text, and the NUL byte a sed run put in source twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Makes the integration surface real and demonstrable. Two stakeholder asks and one tracked TODO converge here, which is why they ship together: a CDS Hooks discovery endpoint is itself an API that wants documenting alongside
/api/v1/compliance.501 not_implementedfrom all four candidate paths, whileARCHITECTURE.md§9 claimed a springdoc OpenAPI document belonging to a backend retired in [Phase 5] Deploy cutover to Node container + JVM retirement + docs #109 PR4, and §7 did not mention/api/v1/complianceat all.cqf-fhir-crkeeps only its B7 cross-check role.What ships
GET /cds-servicesPOST /cds-services/{id}patient-viewhook, from the most recent finalized run. Bearer-gated.POST /cds-services/{id}/feedbackGET /api/v1/openapi.json/api-docs(frontend)Contracts:
docs/CDS_HOOKS.md,docs/COMPLIANCE_API.md. Decisions: ADR-067, ADR-068.The refusals are the substance
Each is mutation-checked — the test fails when the guard is removed.
criticalis never emitted, and is unrepresentable in the card type. In CDS Hooks it means the user must not proceed; WorkWell is supplementary to WebChart (locked decision 1) and is not entitled to say that about someone else's encounter.systemActionslikewise — nothing we return may change a chart without a human choosing it.infocard.{"cards":[]}at the point of care reads as "no gaps" — the confusion ADR-061's 404 exists to prevent, and the one that would have hidden thewc|<patientId>namespace trapPROPOSALS_2026-08.md§P1 names. An empty list is reserved for a subject we did evaluate and found clean.prefetchis declared, because none is evaluated, andusageRequirementssays so in the machine-readable contract. Declaring a template we ignore would make a client fetch and transmit for nothing.APPROVEDterminology mapping, read from the store so approving one unlocks it without a code change. Consequence, stated up front:cms122andcms125get no suggestion — their CPT codes have no mapping at all, andorder-catalog.tscalls its codes "representative (demo, not billing-certified)". Offering a demo-grade CPT for one-click creation in a certified EHR is the harm the rule prevents.The security-relevant part is small and was nearly invisible
/cds-servicesis outside/api/, andauthorizeends inreturn { ok: true }for non-/apipaths — permitAll, mirroring Spring'sanyRequest().permitAll(). Without an explicit rule the invoke endpoint would have served per-patient clinical status to anonymous callers. Two rules added, ordering load-bearing (/cds-services/**also matches the bare path), asserted both as a unitauthorizecall and end-to-end through the worker. Deleting either rule fails both assertions.Both guards caught real things, and neither would have caught the other's
The OpenAPI document is hand-authored, so the contract test is treated as the other half of the decision rather than garnish. It asserts two-way coverage: every documented
(path, method, status)produced by a real request through the real worker, and every observed response documented. A documented-but-unrouted path fails withdocumented but NOT ROUTED— which is exactly howARCHITECTURE.mdcame to assert an OpenAPI document for a year after the JVM was retired.405under the GET operations to silence a Redocly warning; 405 belongs to the path, so the document would have asserted that a GET returns 405. The test failed and the 405s came out.nullable— a 3.0 keyword OpenAPI 3.1 removed in favour of type unions. My hand-rolled check structurally cannot see that class. Same lesson as the CVU+ XSD/Schematron episode: a check's scope was narrower than the claim it was cited for.Five warnings remain, explained in
spec.tsrather than ignore-filed.Research corrected three assumptions before code was written
sub, no token endpoint, and it SHALL NOT be signed with a symmetric algorithm — so our HS256 token can never be a conformant CDS Hooks JWT. Recorded as a named gap, not built:jkufetching is SSRF-by-design, and a verifier whose allowlist nobody has populated is a control that reads as present and cannot fire.STANDARDS_CONFORMANCE.mdsays structurally conformant, self-graded, unverified by any external suite — the ADR-065 posture.PlanDefinition/$applythenRequestOrchestrationthen cards, but DEQM's$care-gapsstops at aDetectedIssueand nobody publishes the gap-to-card bridge. Recorded as a local mapping rather than cited to an IG.No new dependencies
Zero, runtime or dev. Redocly runs via pinned
npxwith nopackage.jsonentry. The reference page is hand-rolled — and not only on the rule:swagger-ui-reactpeers onreact@">=16.8 <19"and this app is React 19, Swagger UI's dark mode is a hard-codedhtml.dark-modeclass that would contest ownership of<html>, and Scalar and Redoc both default to a CDN script the CSP and offline demo rule out. Trade: no try-it-out console, a copyablecurlinstead.Other
card.uuidderives from(runId, subjectId, measureId), so feedback correlates by recomputation. Schema is the owner's alone (CLAUDE.md).WORKWELL_CORS_ALLOWED_ORIGINSdeliberately; the spec requires CORS support but declines to specify an allowlist rule./api/versionreturnsuptime; it does not.prefetchis where it would go.docs/transcripts/is now gitignored — only basename globs matched before, so a file likedocs/transcripts/2026-08-16 call.mdwas committable. Verified withgit check-ignore.Verification
Also driven end-to-end through the real worker: discovery public; invoke 401 without a token; the audiogram card carrying a
ServiceRequestatintent=proposal/status=draftwith CPT 92557; cms125 carrying a link and no suggestion; an unknown patient getting oneinfocard; feedback returning a bodyless 200.Not verified: the HTTP-level run.
@mieweb/cloud-localwants Redis and a service on:9000, neither running locally — a pre-existing environment requirement, not introduced here.Owner steps this creates
Two, both cheap and both for other people.
issand JWKS URL? No public evidence either way — zero hits across both MIE docs sites.Not done, deliberately
No
encounter-startservice (maturity 1 in the CDS Hooks Library IG, and it would return the same cards); noprefetchevaluation; no OpenAPI path aliases beyond the canonical one; nooverrideReasons; no tenant scoping (#269, pre-existing).🤖 Generated with Claude Code