feat: add governed audit evidence review boundary - #102
Conversation
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - name: Compile audit evidence review package | ||
| run: python -m compileall -q packages/audit-evidence-review/src packages/audit-evidence-review/tests |
There was a problem hiding this comment.
📝 Info: compileall artifacts covered by gitignore
The quality workflow compiles the package in-tree then requires a clean checkout. The __pycache__ directories this creates do not break the check because .gitignore already ignores __pycache__/ and *.py[cod]. Confirmed, not a defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /tmp/orgmetra-audit-evidence-review-venv/bin/python -m pip install --require-hashes --no-deps --only-binary=:all: -r "$GITHUB_WORKSPACE/.github/requirements/foundation-test.txt" | ||
| wheel_path="$(find /tmp/orgmetra-audit-evidence-review-dist -maxdepth 1 -type f -name '*.whl' -print -quit)" | ||
| wheel_sha="$(sha256sum "$wheel_path" | awk '{print $1}')" | ||
| printf 'orgmetra-audit-evidence-review[test] @ file://%s --hash=sha256:%s\n' "$wheel_path" "$wheel_sha" > /tmp/orgmetra-audit-evidence-review-install.txt | ||
| /tmp/orgmetra-audit-evidence-review-venv/bin/python -m pip install --require-hashes --no-deps -r /tmp/orgmetra-audit-evidence-review-install.txt | ||
| /tmp/orgmetra-audit-evidence-review-venv/bin/python -m pip check |
There was a problem hiding this comment.
📝 Info: Test extra deps come from foundation-test, not the wheel
The venv installs the wheel with the [test] extra under --no-deps, so pytest and pytest-cov are not pulled from the extra. They are supplied by foundation-test.txt installed at line 59, which pins both. The test runner is present; not a defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def read_rows(self, query: AuditEvidenceQuery) -> tuple[PersistedAuditEvidenceRow, ...]: | ||
| """Return one ordered, tenant-bound page of revalidated immutable audit rows.""" | ||
| if type(query) is not AuditEvidenceQuery: | ||
| raise TypeError("query must be an exact AuditEvidenceQuery.") | ||
| verified_query = AuditEvidenceQuery( | ||
| tenant_record_id=query.tenant_record_id, | ||
| query_reference=query.query_reference, | ||
| requester_reference=query.requester_reference, | ||
| purpose_code=query.purpose_code, | ||
| recorded_from=query.recorded_from, | ||
| recorded_before=query.recorded_before, | ||
| limit=query.limit, | ||
| ) | ||
|
|
||
| with self.connection_factory() as connection: | ||
| with connection.cursor() as cursor: | ||
| cursor.execute(_READ_ONLY_SQL) | ||
| cursor.execute(_ROLE_GUARD_SQL) | ||
| if cursor.fetchone() is None: | ||
| raise PermissionError( | ||
| "audit evidence reads require a NOSUPERUSER NOBYPASSRLS database role." | ||
| ) | ||
| cursor.execute(_TENANT_CONTEXT_SQL, (str(verified_query.tenant_record_id),)) | ||
| cursor.execute( | ||
| _AUDIT_EVIDENCE_SQL, | ||
| ( | ||
| verified_query.tenant_record_id, | ||
| verified_query.recorded_from, | ||
| verified_query.recorded_before, | ||
| verified_query.limit, | ||
| ), | ||
| ) | ||
| rows = cursor.fetchmany(verified_query.limit) |
There was a problem hiding this comment.
📝 Info: Reader read path has no authorization gate
PostgresAuditEvidenceRowReader.read_rows performs the read-only role guard and tenant RLS binding but no authorization; that lives only in read_audit_evidence. By design the reader is meant to be composed under read_audit_evidence, but any future host that calls the reader directly loses the authorization-before-read guarantee.
Was this helpful? React with 👍 or 👎 to provide feedback.
| with self.connection_factory() as connection: | ||
| with connection.cursor() as cursor: | ||
| cursor.execute(_READ_ONLY_SQL) | ||
| cursor.execute(_ROLE_GUARD_SQL) | ||
| if cursor.fetchone() is None: | ||
| raise PermissionError( | ||
| "audit evidence reads require a NOSUPERUSER NOBYPASSRLS database role." | ||
| ) | ||
| cursor.execute(_TENANT_CONTEXT_SQL, (str(verified_query.tenant_record_id),)) |
There was a problem hiding this comment.
📝 Info: Read-only/RLS depends on non-autocommit connection
SET TRANSACTION READ ONLY and the set_config(..., true) tenant binding only take effect if the injected connection is not in autocommit and no statement has yet run in the transaction. This deployment assumption matches the existing services/job-analysis-api/.../postgres.py reader, so it is a consistent convention rather than a new defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| event_keys = frozenset(document) | ||
| if event_keys not in (_BASE_EVENT_KEYS, _BASE_EVENT_KEYS | {_CONFIRMATION_KEY}): | ||
| raise ValueError("canonical_event_json does not match the governed audit envelope shape.") | ||
| event_data = document.get("data") | ||
| if type(event_data) is not dict or frozenset(event_data) != _DATA_KEYS: | ||
| raise ValueError("canonical_event_json does not match the governed audit data shape.") | ||
| if document.get("specversion") != "1.0" or document.get("datacontenttype") != "application/json": | ||
| raise ValueError("audit event must use the governed CloudEvents 1.0 JSON contract.") | ||
| if document.get("id") != str(self.audit_event_record_id): | ||
| raise ValueError("audit event id does not match the persisted row identity.") | ||
| if document.get("orgmetratenant") != str(self.tenant_record_id): | ||
| raise ValueError("audit event tenant does not match the persisted row scope.") |
There was a problem hiding this comment.
📝 Info: Read-time checks are a subset of DB write validation
PersistedAuditEvidenceRow re-verifies key sets, data shape, canonical form, digest, specversion, datacontenttype, id and tenant, but not the source/type/subject/actor patterns that validate_audit_event_envelope enforces on write. Acceptable because the rows already passed that DB CHECK, so this is narrower defense-in-depth, not a hole.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@opencode-agent Please review the current unchanged head against protected |
1 participant
Buyer-visible gap
Protected
develop@9e3e4847510e1e612b48474ba42b177b8ed824dfalready writes PII-minimized immutable audit/outbox evidence, but it has no executable purpose-bound read boundary that proves authorization occurs before evidence access and re-verifies persisted canonical evidence before returning it to an auditor or HR governance workflow.This PR adds that bounded Orgmetra-owned review boundary and now includes the concrete least-privileged PostgreSQL reader for the existing audit store. It does not add a second system of record, does not query another CWL repository or cross-service HR application tables, and does not grant employment-decision authority.
Test-first implementation and repairs
Initial contract head
592cd8f097cb1d45cd901b85f542b590f9ebb546added the installed-wheel quality lane and an import-before-production test. Its workflows materialized but remained queued before follow-up commits, so it is test-first predecessor evidence, not claimed as terminal hosted RED, and no predecessor status is transferred.The implementation adds a bounded
AuditEvidenceQuery; exact-scopeAuditEvidenceReadAuthorityinvoked before storage; a read-only adapter protocol over the existing forced-RLS Orgmetra audit store; and defense-in-depth verification of exact UTF-8 canonical bytes, lower-case SHA-256, deterministic JSON, the existing PII-minimized envelope/data shape, CloudEvents 1.0 media/version, event/tenant identity, system-recorded window, row count, and strict(recorded_at, audit_event_record_id)order.Fresh self-review repaired two earlier Orgmetra-owned integrity defects test-first:
1aa81b250669373ffd63c2b398925773c6cf967cproves that adding a top-level HR field or nesteddatafield and recomputing a matching digest must still fail closed; malformed unencodable text must also become a stable validation failure. Root repairfb668be543d93c2bc7a1d0a930d5f889a916fd7bbinds verification to the exact protected-main PII-minimized envelope/data key sets and hashes one verified UTF-8 byte sequence.b9385dbe9c9501dd1748cca6261c4d4c118ac1ddproves a once-valid query cannot widen its limit, a once-valid authorization cannot replace booleanpermittedwith a truthy non-boolean, and a once-valid persisted row cannot replace canonical bytes plus digest after construction. Root repair5005c9f58c3f35c6a08b8134362478418d011bccreconstructs live query, authorization, and row fields through governed constructors and returns detached verified row snapshots.PostgreSQL host adapter slice
Regression head
df289f8506e170d290340c5d3dab32a31812e2cerequires a publicPostgresAuditEvidenceRowReader,SET TRANSACTION READ ONLY, a fail-closedCURRENT_USERproof forNOSUPERUSER NOBYPASSRLS, transaction-localorgmetra.tenant_record_idbinding, a static parameterized query overpublic.audit_event_record, the authorized half-openrecorded_atwindow and limit, deterministic ordering, direct-query revalidation before connection open, and fail-closed unexpected row shape. Its hosted workflows queued before implementation, so no terminal RED is claimed.Root implementation starts at
b7269c6b9fc091ab141c85e98c9c2c00580295bf. The adapter reads only the existing Orgmetra audit relation; deployment composition retains TLS, credentials, pooling and least-privileged role selection. README, CHANGELOG, ADR 0102 and traceability distinguish this implemented active-PR adapter from the still-planned customer-facing audit UI/API.Fresh source verification on the current head confirms
read_audit_evidence(...)snapshots and authorizes the exact query before invokingreader.read_rows(...).PostgresAuditEvidenceRowReaderis intentionally a storage adapter rather than a second authorization authority; direct use outside the governed orchestration function is not an authorized review path. The remaining review observations about compile caches, test-tool installation, non-autocommit deployment convention and DB-write-vs-read defense depth are informational and do not identify a current authorization/privacy/data-integrity bypass.CloudEvents v1.0.2 and NIST SP 800-53 Rev. 5 AU-6/current reviewed control catalog update 5.2.0 remain design evidence, not certification claims.
Exact-current-head evidence
Current exact head:
d87cb05f723f106c653f2ea07680872fd9c62ada.Fresh live base:
develop@9e3e4847510e1e612b48474ba42b177b8ed824df.GitHub reports the PR open, ready-for-review, and mergeable.
Every applicable exact-current-head hosted workflow is terminal GREEN:
32663401703— success.32663401728— success.32663401678— success.32663401822— success.32663401746— success.Fresh submitted reviews are COMMENTED/analysis only; there is no qualifying independent non-author
APPROVEorCHANGES_REQUESTED. The earlierdatacontenttypeconcern is resolved after verifying the protected audit writer/database contract. Current unresolved threads are informational observations, not verified defects.Merge governance
Ready-for-review is not merge authorization. Keep unmerged until a qualifying independent non-author approval exists on this unchanged head and issue #89's enforceable
developprotection defect is actually repaired. Immediately before any future merge, refetch exact head, live base, reviews, unresolved threads, effective rules/protection and exact-current-head checks and use expected-head protection only if every live gate is satisfied. Do not self-approve, weaken gates, simulate branch protection in workflow code, transfer predecessor evidence, or manufacture passing evidence.