fix(audit-sprint-6c): compliance attestation UI for 157 manual evaluators - #173
Conversation
…tors
The 2026-05-23 audit deep-dive caught that ~157 control evaluators
across SOC2/ISO27001/HIPAA/PCI-DSS/CIS/NIST-CSF/GDPR packs emit
`status='not_evaluated'` with a gap of `code='manual_attestation_required'`
— but there was no UI for a human to complete the attestation. The
framework packs were structurally complete but practically PowerPoint.
This PR closes the loop end-to-end with a two-state workflow
(claimed → reviewed) enforcing separation of duties.
Deliverables (1337 net lines)
1. Migration `20260624021_audit_sprint6c_control_attestations.sql`:
- org_control_attestations table (org_id + framework_id + control_key,
status, claimed_by/at, reviewed_by/at, rejected_reason, evidence_id
NOT NULL, notes)
- CHECK constraints:
* status IN ('claimed','reviewed','rejected')
* reviewed_by != claimed_by when status='reviewed' (segregation
of duties — DB-enforced, defense in depth alongside the
server-action check)
* rejected requires non-empty rejected_reason
- RLS policies: org-scoped SELECT/INSERT/UPDATE via org_members
- updated_at trigger
- Compound indexes for the list-resolver join + a partial index on
pending ('claimed') rows for the "awaiting review" bucket
2. lib/compliance/attestations.ts — data layer
- listControlsNeedingAttestation(orgId): joins
org_control_evaluations rows whose details.gaps contains
MANUAL_GAP_CODE against the latest attestation per
(framework, control). Returns ControlNeedingAttestation[] the
UI groups into 3 buckets.
- insertAttestationClaim() + updateAttestationReview() — narrow
writers with their own validation (separation of duties, reject
reason required, status check).
3. app/app/actions/compliance-attestations.ts — server actions
- claimAttestation, reviewAttestation, listMyAttestations
- Each wraps the data layer with getUserOrgMembership() auth + a
writeAuditLog() hash-chain entry (Sprint 1's audit-engine).
- Hash-chain failure is non-blocking (logged) so an unrelated
chain conflict can't break the workflow.
- TODO marker: once Sprint 1 PR #162 merges, wrap with
assertOrgCanWrite() from lib/billing/enforce-grace-period.ts
so past-due orgs in read-only mode can't attest.
4. /app/compliance/attestations page + AttestationsClient
- 3 tabs: Awaiting attestation / Awaiting review / Reviewed
- Card per control with framework chip + control key + message +
conditional action button (Claim / Review / "Awaiting another
reviewer" if you're the claimer)
- ClaimDialog: evidence-id input + notes textarea + Cancel/Claim
- ReviewDialog: evidence + claimer-notes + Approve/Reject + the
segregation-of-duties banner when reviewer == claimer
- RejectDialog (AlertDialog): mandatory reason textarea
- Built on Sprint 4c Dialog/AlertDialog/Toaster primitives.
- Empty states for each bucket.
5. __tests__/lib/compliance/attestations.test.ts (7 tests)
- List filters out non-manual_attestation_required gaps
- List attaches latest attestation per (fw, control)
- List returns [] when no matching evaluations
- updateAttestationReview throws on reject-without-reason
- updateAttestationReview throws on reviewer == claimer
- updateAttestationReview throws when status != 'claimed'
- approve path returns the updated row
Built on top of fix/audit-sprint-4c-modal-primitives (PR #167) for the
Dialog/AlertDialog/Toaster primitives.
Validation
- tsc -p tsconfig.typecheck.json: clean
- eslint: 0 errors, 18 warnings (baseline)
- jest: 5326/5341 pass (+7 new attestation tests on top of 4c's 5319)
Test plan after merge
- Apply migration 20260624021 to staging.
- Visit /app/compliance/attestations as a user whose org has at
least one active framework with manual evaluators.
- As user A, claim an attestation with an evidence row id.
- As user B (different user), open the same row, click Approve.
- Verify writeAuditLog wrote two hash-chained entries:
compliance.attestation.claimed (user A)
compliance.attestation.approved (user B)
- Try to approve your own claim — UI should show the segregation
banner and disable the Approve button; if you bypass the client
check, the server action / DB CHECK both reject.
- Reject with no reason — UI Submit stays disabled; server action
rejects if you bypass it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a manual-attestation workflow that closes the loop for the ~157 control evaluators across SOC2/ISO27001/HIPAA/PCI-DSS/CIS/NIST-CSF/GDPR packs which emit not_evaluated + manual_attestation_required. A new org_control_attestations table, server actions, and a Sprint-4c-primitives-based UI implement a two-state claimed → reviewed workflow with DB-enforced separation of duties and hash-chained audit logging.
Changes:
- New migration
20260624021_audit_sprint6c_control_attestations.sqladding theorg_control_attestationstable (with status/segregation-of-duties/rejected-reason CHECK constraints, RLS byorg_members, and supporting indexes). - Data layer (
lib/compliance/attestations.ts) joiningorg_control_evaluations.details.gapsagainst the latest attestation per(framework, control), plusinsertAttestationClaimandupdateAttestationReviewhelpers; server actions inapp/app/actions/compliance-attestations.tswirewriteAuditLogfor claim/approve/reject. - New
/app/compliance/attestationspage +AttestationsClient.tsxwith three buckets (awaiting / awaiting review / reviewed) and Dialog/AlertDialog/Toaster UX, and Jest coverage for filtering, separation-of-duties, and reject-without-reason.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260624021_audit_sprint6c_control_attestations.sql | New table, constraints, indexes, RLS, and updated_at trigger backing the workflow. |
| lib/compliance/attestations.ts | Data layer: list resolver + claim/review helpers with snake↔camel mapping. |
| app/app/actions/compliance-attestations.ts | Server actions: membership resolution, mutation, and hash-chain audit writes. |
| app/app/compliance/attestations/page.tsx | Server entry; redirects unauthenticated users and renders client component. |
| app/app/compliance/attestations/AttestationsClient.tsx | Client UI: bucketed list, Claim/Review/Reject dialogs on Sprint-4c primitives. |
| tests/lib/compliance/attestations.test.ts | Jest coverage for list filtering, separation-of-duties, and reject-without-reason. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function listMyAttestations() { | ||
| const membership = await getUserOrgMembership(); | ||
| if (!membership) { | ||
| return actionError(new Error('Unauthorized')); | ||
| } | ||
| try { | ||
| const rows = await listControlsNeedingAttestation(membership.orgId); | ||
| return actionOk<ControlNeedingAttestation[]>(rows); | ||
| } catch (err) { | ||
| return actionError(err); | ||
| } | ||
| } | ||
|
|
||
| export async function claimAttestation(input: { | ||
| frameworkId: string; | ||
| controlKey: string; | ||
| evidenceId: string; | ||
| notes?: string; | ||
| }) { | ||
| const membership = await getUserOrgMembership(); | ||
| if (!membership) return actionError(new Error('Unauthorized')); |
| -- org_control_evaluations. | ||
| CREATE INDEX IF NOT EXISTS org_control_attestations_org_fw_ctrl_idx | ||
| ON public.org_control_attestations (organization_id, framework_id, control_key, claimed_at DESC); | ||
|
|
| controlKey, | ||
| // We don't have a separate control title table populated for | ||
| // every pack today — fall back to the code itself. | ||
| controlTitle: controlKey, | ||
| message, | ||
| latestAttestation: latestByKey.get(`${frameworkId}|${controlKey}`) ?? null, |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b2b88e3ce
ℹ️ 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".
| .eq('id', input.attestationId) | ||
| .maybeSingle(); |
There was a problem hiding this comment.
updateAttestationReview fetches and updates rows by id only while using createSupabaseAdminClient (service-role), so RLS does not protect tenant boundaries here. If a user obtains another org’s attestation UUID, they can approve/reject that foreign record because neither the read nor write query constrains organization_id; this also causes the audit log in reviewAttestation to be written under the caller’s org instead of the mutated row’s org.
Useful? React with 👍 / 👎.
| control_key: input.controlKey, | ||
| status: 'claimed', | ||
| claimed_by: input.claimedBy, | ||
| evidence_id: input.evidenceId, | ||
| notes: input.notes ?? null, |
There was a problem hiding this comment.
insertAttestationClaim inserts evidence_id directly with the service-role client and never verifies that the referenced evidence belongs to input.orgId. Because the foreign key is only on org_evidence(id), a caller who knows another tenant’s evidence UUID can create a cross-tenant reference, which corrupts data ownership and can block deletion of that evidence (ON DELETE RESTRICT).
Useful? React with 👍 / 👎.
Summary
Audit caught that ~157 control evaluators across SOC2/ISO27001/HIPAA/PCI-DSS/CIS/NIST-CSF/GDPR emit
manual_attestation_requiredbut had no UI for a human to complete. The framework packs were structurally complete but practically PowerPoint. This PR closes the loop with a two-state workflow (claimed → reviewed) and DB-enforced segregation of duties.Deliverables (1,337 net lines)
How it works
List resolver joins
org_control_evaluations.details.gaps(where any gap hascode='manual_attestation_required') against the latestorg_control_attestationsrow per(framework, control). UI groups into 3 buckets:status='claimed'(someone else needs to approve)status='reviewed'Segregation of duties is enforced in 3 places:
reviewed_by != claimed_bywhenstatus='reviewed'(defense in depth)Hash-chained audit — every claim/approve/reject writes a
writeAuditLogentry (Sprint 1's hash-chain engine) with the framework + control + decision.Evidence required —
evidence_idisNOT NULLin the table; the act of attesting always leaves an artefact.TODOs after merge
20260624021to staging then prodassertOrgCanWriteonce Sprint 1 PR fix(audit-sprint-1): 7 stop-the-bleed fixes from 2026-05-23 E2E audit #162 merges — past-due orgs in read-only mode should not be able to attest/app/compliance/attestationsisn't wired to the compliance landing yet (next PR)Validation
npm run type-checkcleannpm run lint0 errors, 18 warnings (baseline)npx jest5326/5341 pass (+7 new attestation tests on top of 4c's 5319)Test plan
20260624021to staging/app/compliance/attestationsas a user in an org with active frameworks/app/vaultcompliance.attestation.claimed(A) +compliance.attestation.approved(B)🤖 Generated with Claude Code