ADR-0002: Implement the PHI compliance data layer, enforced access logging, and authenticated compliance API - #2
Merged
Conversation
…ted API The compliance service queried fifteen tables that no migration defined, so every endpoint on all three routers failed at its first query. It was also reachable with no authentication at all. Schema: adds migrations/V002_compliance_schema.sql creating all fifteen tables. phi_access_logs is partitioned by timestamp with a DEFAULT partition, and is append-only -- UPDATE and DELETE raise from a trigger, with a REVOKE as defence in depth -- plus a prev_hash/entry_hash chain for tamper evidence. Auth: promotes billing's JWT middleware into the compliance service and mounts it on all three routers, ahead of executionContextMiddleware so an unauthenticated request gets 401 rather than 400. Fixes the field-name trap that would have made this a silent no-op: the routes read req.user.id, which AuthenticatedUser does not have, so every record was attributed to the literal 'system'. All nine sites now read req.user.userId, and the || 'system' fallback is gone. GET /phi-access requires compliance-auditor; BAA and breach writes require compliance-officer. Adds Zod validation, which makes a forged userId in the body a 400. Pseudonymization: pseudonymize() becomes keyed HMAC-SHA256 -- deterministic, non-reversible, fixed-width. The previous random-IV AES meant patient-scoped lookup never matched and uniquePatients counted one patient per access, making disclosure accounting under 164.528 impossible. Key material: removes the hardcoded default key and salt. The service now refuses to start when NODE_ENV=production and HIPAA_ENCRYPTION_KEY, HIPAA_PSEUDONYM_SALT, or JWT_SECRET is unset. Enforced logging: PHI access now routes through PHIRepository, which emits the access record as a side effect of the operation rather than trusting callers to report themselves. Reading the disclosure log is itself logged. 59 tests across 7 suites, run against PostgreSQL 16 with both migrations applied. Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
…E role Adds .github/workflows/compliance-service.yml: a Node job with a PostgreSQL 16 service container that applies V001, applies V002, re-applies V002 to prove idempotency, asserts all fifteen tables exist, then runs the suite. Kept out of ci.yml because every job there is a cargo job, and so this does not conflict with the ci.yml change in the ADR-0001 branch. Fixes a real defect found while writing it. V002 guarded its REVOKE on a role named app_user, but the role V001 creates is llm_copilot_app, so the guard never matched and the REVOKE never ran. That mattered: V001:444 grants SELECT, INSERT, UPDATE, DELETE on ALL TABLES to that role, so the application role inherited UPDATE and DELETE on the PHI audit log. Verified against a real database that the role now holds SELECT and INSERT but not UPDATE or DELETE, while retaining UPDATE on audit_logs as a control. The CI database is named llm_copilot_agent because V001:442 grants on that name; under any other name V001 fails partway and never creates the role. That is why this was not caught earlier. Adds an integration test asserting the privileges, closing the "REVOKE untested" gap reported previously. Suite is now 60 tests across 7 suites. Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
createAudit passed JSON.stringify(audit.findings) for a TEXT[] column. Postgres rejects the resulting '[]' with: ERROR: malformed array literal: "[]" DETAIL: "[" must introduce explicitly-specified array dimensions. Reproduced against PostgreSQL 16 before fixing. node-pg serializes a JS array to a proper array literal, so passing audit.findings directly is all that is needed. The column must be TEXT[] rather than JSONB because the UPDATE at :439 uses array_append, and mapAuditRow at :903 reads it back as string[] -- both of which only work with a real array type. This INSERT was the one call site that had not been updated to match. Adds an integration test covering the full round trip: create an audit, append two findings via array_append, and read them back through mapAuditRow. Verified the test genuinely catches the defect by reverting the fix, which reproduces the malformed array literal error. Also makes the tamper-evidence forgery test transactional. It previously committed a forged row to prove verifyChain detects it, but phi_access_logs is append-only so the row could never be removed -- it broke the chain permanently and every later run against the same database failed. The forgery is now rolled back, and verifyChain takes an optional executor so it can inspect uncommitted rows. Confirmed by running the suite twice against one database: 15 passed both times. Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implements
docs/adr/ADR-0002-implement-phi-compliance-data-layer.md, which makes the concrete technical decisions for ADR-0001 Phases 2-3. The ADR is included here since it was not previously committed. Stacked on #1 conceptually but independent in code — #1 is documentation only and touches noservices/file.Migration execution status — read this first
The migration WAS executed against a real PostgreSQL 16 database, not merely written and reviewed.
I ran
postgres:16-alpinein a local Docker container, appliedV001_initial_schema.sql, then appliedV002_compliance_schema.sqlon top. It applied with zero errors, and re-applying it is clean (idempotent). All 15 tables verified present viainformation_schema. The 13 integration tests below execute real SQL against that database through the actual service code.What that does NOT cover, and you should not read it as covering: this was an ephemeral local container, not a staging or production database. It had no pre-existing data, no
app_userrole (so theREVOKEblock was skipped by itsIF EXISTSguard and is untested), and no replication or connection-pooler in front of it. No live infrastructure was touched. A real deploy still needs a rehearsal against a database that has the application role provisioned.Why
migrations/V001_initial_schema.sqlis the only migration in the repo and defines nine tables. The compliance services reference fifteen others, none of which exist anywhere in any.sqlfile. Every endpoint on all three routers fails at its first query withrelation ... does not exist. The service has never successfully served a request against a real database. On top of that, all three routers were mounted behindexecutionContextMiddleware— span plumbing — so they were reachable with no authentication at all, andGET /phi-accesswould have returned patient identifiers and IP addresses to anyone who could reach port 3009.I verified each of the ADR's six findings against the code before changing anything. All six reproduce exactly as described, including the line numbers.
What changed
1.
migrations/V002_compliance_schema.sql(new) — all fifteen tablesphi_access_logsis partitioned byRANGE (timestamp)mirroring theaudit_logspattern, with aDEFAULTpartition so an insert never fails on a missing range — for an audit control, rejecting the write is the wrong failure direction. Indexed on(patient_id, timestamp DESC),(user_id, timestamp DESC), and(timestamp DESC): the three filters athipaaService.ts:116-136.patient_idisCHAR(64)since the HMAC output is fixed-width, andkey_versionis present from day one so a future key-rotation ADR isn't blocked on a migration.Append-only is enforced by a
BEFORE UPDATE OR DELETEtrigger that raises, plus aREVOKE— belt and braces, because a futureGRANTwould silently re-open mutation but cannot disable the trigger. This matters specifically for partitioned tables:REVOKEon the parent does not cover a direct mutation of a partition, whereas the row trigger propagates to all of them.I confirmed each of the three schema gotchas the ADR documents, by executing them:
business_associate_agreements.documentsJSONB NOT NULL DEFAULT '[]'hipaaService.ts:360appends with||, which returnsNULLon aNULLleft operand and would erase the listjsonb_array_length= 2hipaa_breaches.phi_types/.containment_actionsTEXT[]:719passes JS arrays as parameters directly, withoutJSON.stringify['name','ssn','diagnosis']intactcompliance_controlsneeds surrogateidand distinctcontrol_id:93looks up byid;hipaaService.ts:613-616joins oncontrol_idagainst identifiers like164.308(a)(1)One thing the ADR did not flag, which I found and did not silently paper over:
compliance_audits.findingsis written two incompatible ways by existing code. The INSERT atcomplianceService.ts:296passesJSON.stringify(audit.findings)(implying JSONB), but the UPDATE at:439usesarray_append(findings, $1), which requires a real array type and fails against JSONB. I made the columnTEXT[], becausearray_appendconstrains the type andAuditSchema.findingsisz.array(z.string()). This means the INSERT at:296will fail —'[]'is not a validtext[]literal. I did not changecomplianceService.tsto match, because that is a service-logic fix outside this ADR's stated scope and I would rather you see the inconsistency than have me pick a side quietly. It needs a one-line follow-up changingJSON.stringify(audit.findings)toaudit.findings.2. Authentication on all three routers
Promoted billing's middleware into
services/compliance/src/middleware/auth.tsalong with the minimalutils/errorsandutils/loggerit needs.The field-name trap is fixed. Billing sets
req.user.userId; the compliance routes readreq.user?.id, whichAuthenticatedUserdoes not have. Mounting the middleware unchanged would have looked like a fix while still attributing every record to'system'. I found 9 such sites, not the 2 the ADR cites — the same bug is inroutes/compliance.ts(5) androutes/dataResidency.ts(4). All nine now readreq.user!.userId, and the|| 'system'fallback is deleted everywhere; a test asserts the string is gone from all three routers.Mount order is
authenticate→executionContextMiddleware, deliberately. The latter rejects requests missingX-Parent-Span-Idwith 400, so the documented order would have made unauthenticated requests return 400 instead of 401, and would do span work for an unauthenticated caller.Per-route guards:
GET /phi-accessandPOST /phi-access/reportrequirecompliance-auditor/admin; BAA and breach writes requirecompliance-officer/admin;/requirementsand/assessmentrequire any authenticated user. Zod schemas (already a declared, unused dependency) validate every body and query, and are.strict()— so a forgeduserIdin the body is a 400, not a silently ignored field.3.
pseudonymize()→ keyed HMAC-SHA256Deterministic, non-reversible, fixed 64-char output. No other call site needed changing;
:65and:123simply start agreeing with each other.4. Fail-closed key loading
config/env.tsthrows whenNODE_ENV=productionandHIPAA_ENCRYPTION_KEYorHIPAA_PSEUDONYM_SALTis unset. Both string literals deleted. I extended this toJWT_SECRET, which the ADR does not name: billing defaults it to'development-secret', and a default JWT secret on a PHI API means anyone can mint a valid token for any user and any role. Failing closed was the more conservative choice. Outside production an explicitly-labelled ephemeral key is permitted and logs a warning that pseudonyms are not stable across restarts.5. PHI data-access layer
PHIRepositoryis now the only module permitted to issue SQL against a PHI-classified table. It emits the access record as a side effect of the operation, computes the tamper-evidence hash chain, and takesuserIdfrom an injected context —PHIAccessRecorddeliberately has nouserIdfield to forge. Reading the disclosure log is itself logged as a disclosure.Test output — real, not fabricated
npx jestagainst a freshly created PostgreSQL 16 database with both migrations applied:Every ADR Verification item, mapped:
pseudonymize.test.tsintegration.db.test.tsuniquePatients:1,uniqueUsers:4from 20 accessesintegration.db.test.tsauth.test.tscompliance-auditorauth.test.tsuserIddoes not override tokenauth.test.ts'system'attribution leaksauth.test.tsservices/failClosed.test.tsUPDATE/DELETEboth raiseintegration.db.test.tsschema-drift.test.tsphi-enforcement.test.tsSelected real output:
Four tests are deliberately negative, because a check that only ever passes is indistinguishable from a check that is broken. Each writes a real defect, asserts the detector catches it, then removes it: an ePHI read that bypasses the repository; a query against a nonexistent table; a forged hash-chain entry; a fabricated
SOC 2 Type II certifiedclaim (that last one in #1).The hash-chain verifier caught a genuine planted row on its first run — a hand-crafted entry left over from my schema smoke test — before I had written the test for that case. That is why the tamper-detection test exists.
Typecheck:
npx tsc --noEmitreports the same 4 errors before and after my change. All four are pre-existing, from the cross-service../../ai-platform/src/middleware/executionContextimport atindex.ts:19violating the service's ownrootDir. I verified this by stashing my work and running againstmain: byte-identical output. My changes introduce zero new type errors, and I did not fix the pre-existing four — that's a tsconfig/monorepo question, not this ADR's.Deviations from the ADR — please review these four
1. Auth middleware is a copy, not a shared package (ADR step 5). The ADR says promote it to a location both services import, and warns "Do not fork it; a second copy of authentication logic is how the two drift." It also permits
services/compliance/src/middleware/auth.ts"if extraction is deferred". I took the deferred option: the repo has no npm workspaces, the rootpackage.jsoncontains one dependency, and each service's tsconfig setsrootDir: ./srcso TypeScript refuses to compile a file outside it. Building shared-package infrastructure across eleven services is a much larger change than this ADR. Mitigation:auth-drift.test.tsfails if billing renames theuserIdfield, if the twoAuthenticatedUsershapes diverge, or ifauthorize()'s role-matching rule changes in one copy — i.e. it catches exactly the drift the ADR warns about.2.
authenticateInternalintentionally diverges from billing's. Billing's synthesisesuserId: 'system'. Compliance's must not:phi_access_logs.user_idisUUID NOT NULL REFERENCES users(id), so an audit record must name a real person, and'system'is not a UUID. Internal ingestion instead supplies an explicitonBehalfOfUUID and is rejected without one. This is the more conservative reading — an unattributable PHI record is worse than a rejected request — but it is a design decision the ADR left open, so flagging it. Note the residual property: anyone holdingINTERNAL_SERVICE_KEYcan still attribute an access to any user. That is inherent to service-to-service ingestion as the ADR specified it. Tests confirm the key reaches onlyPOST /phi-accessand no other route.3.
.env.examplewas NOT updated (ADR step 4). My sandbox denies read and write on.env*files, so I could not add the variables. Someone needs to add these by hand:4. Deployment manifests were NOT added (ADR step 8 / ADR-0001 step 16). This needs a human decision, so I stopped rather than guessing. No service in this repo has a Dockerfile, and none of the eleven Node services appear in
docker-compose.yml— onlycopilot-agent,postgres,redis,nats, and observability. Addingcompliancealone would require inventing a Dockerfile and a build pipeline, and choosing a secret store for the keys above. Those are deployment-architecture decisions I should not make unilaterally in a PHI-handling PR. Consequence: none of this is exercised in CI yet. The tests run locally and need a Postgres service container wired into the workflow (I could not touch.github/workflows/— the token lacksworkflowscope, same as #1).Left undone
compliance_audits.findingsINSERT/UPDATE inconsistency described above — one line incomplianceService.ts, deliberately left for a decision.REVOKE UPDATE, DELETE ON phi_access_logs FROM app_useris guarded byIF EXISTSand was not exercised, since the test container has noapp_userrole. The trigger — the real enforcement — is fully tested.PHI_CLASSIFIED_TABLEScurrently registers onlyphi_access_logs, which is correct today (no other repo code reads ePHI), but it is a registry to grow, not a finished inventory. Adding a table to it immediately brings it under enforcement.key_versionis stored but no rotation procedure exists. The ADR defers this to a follow-up, and the column being present from day one means that ADR won't need a migration.