[codex] Add audit debugging platform PRD#114
Conversation
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit 765fdbc on Jun 25, 2026 2:21pm UTC. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA PRD is added for the Goggles audit debugging platform, and a new v2 JSON Schema defines forensic audit log events with shared context, evidence, event unions, and audit-data-mode validation rules. ChangesAudit debugging platform design and schema
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)docs/audit-debugging-platform-prd.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audit-debugging-platform-prd.md`:
- Around line 129-135: Make the full data auditing rollout dependent on an
explicit retention/deletion/access-control policy before it can be enabled.
Update the PRD section around the full data auditing requirements and the open
question near the retention/deletion item so that the policy is a prerequisite
deliverable, not an unresolved follow-up. Use the existing “full data auditing,”
“mode-change audit rows,” and “retention/deletion/access-control” sections to
keep the requirement and rollout gating aligned.
- Around line 338-350: The read-endpoint requirements are too generic and only
mention authentication, so update the PRD section around the listed GET routes
to explicitly define object-level authorization and tenant scoping for each
cross-entity resource. In the endpoints list and the related “Require
authentication” requirements, add per-object access checks for
group/account/engine membership, anti-enumeration behavior for unauthorized or
unknown resources, and least-privilege defaults for baseline responses so the
rules are clear for every read path.
- Around line 259-262: The EvidenceRef shape is exposing raw event JSON on every
derived object, which should be removed from the ubiquitous reference type.
Update the PRD sections around EvidenceRef so it is pointer-only, keeping only
the identifying fields such as file_id, line, and hash, and move raw JSON access
to a separate privileged evidence retrieval flow. Make the same adjustment
wherever EvidenceRef is described, including the later duplicate mention, so all
derived objects still carry evidence references without embedding payload data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2de3bd98-b9ad-4a99-bb3b-a58ec95f78e1
📒 Files selected for processing (1)
docs/audit-debugging-platform-prd.md
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/audit-debugging-platform-prd.md (3)
270-272: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove raw payload embedding from
EvidenceRef.
EvidenceRefstill includes raw JSON, which overexposes sensitive data on every derived object. Keep it pointer-only and require privileged evidence fetch for payload content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/audit-debugging-platform-prd.md` around lines 270 - 272, Remove raw payload fields from EvidenceRef and keep it pointer-only: update the EvidenceRef definition and any derived-object construction so it retains only identifying reference data like raw file id, line number, line hash, and raw kind type, while excluding raw JSON. Then adjust any code or docs that assume payload embedding in EvidenceRef to use privileged evidence fetch for content access, using the EvidenceRef and derived object references to locate the affected definitions.
372-387: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthentication-only requirement is insufficient for these read endpoints.
Add explicit object-level authorization and tenant/account/group/engine scoping requirements (plus anti-enumeration behavior) for all listed read routes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/audit-debugging-platform-prd.md` around lines 372 - 387, The read-route guidance under the API contract is missing object-level authorization and scope enforcement, so update the requirements around the documented endpoints to explicitly call out tenant/account/group/engine scoping, per-object authorization checks, and anti-enumeration behavior. Use the existing stable API section and the listed read-route requirements to add that every access path must verify the caller’s scope before returning data, not just authenticate, and ensure the response behavior does not reveal resource existence across unauthorized tenants or accounts.
490-505: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake full-data retention/deletion/access-control policy a rollout gate, not an open follow-up.
The PRD still allows retaining full-data audit files while deferring deletion/retention policy details. This should be a prerequisite before enabling/continuing full-data auditing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/audit-debugging-platform-prd.md` around lines 490 - 505, The full-data retention/deletion/access-control policy is still deferred in the PRD, but it needs to be defined as a rollout gate before full-data auditing can proceed. Update the “During the internal-testing phase” guidance and the “Remaining Open Questions” section in audit-debugging-platform-prd.md so the retention period, deletion mechanism, and access-control requirements are explicit and required, rather than left as a later follow-up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/audit-debugging-platform-prd.md`:
- Around line 270-272: Remove raw payload fields from EvidenceRef and keep it
pointer-only: update the EvidenceRef definition and any derived-object
construction so it retains only identifying reference data like raw file id,
line number, line hash, and raw kind type, while excluding raw JSON. Then adjust
any code or docs that assume payload embedding in EvidenceRef to use privileged
evidence fetch for content access, using the EvidenceRef and derived object
references to locate the affected definitions.
- Around line 372-387: The read-route guidance under the API contract is missing
object-level authorization and scope enforcement, so update the requirements
around the documented endpoints to explicitly call out
tenant/account/group/engine scoping, per-object authorization checks, and
anti-enumeration behavior. Use the existing stable API section and the listed
read-route requirements to add that every access path must verify the caller’s
scope before returning data, not just authenticate, and ensure the response
behavior does not reveal resource existence across unauthorized tenants or
accounts.
- Around line 490-505: The full-data retention/deletion/access-control policy is
still deferred in the PRD, but it needs to be defined as a rollout gate before
full-data auditing can proceed. Update the “During the internal-testing phase”
guidance and the “Remaining Open Questions” section in
audit-debugging-platform-prd.md so the retention period, deletion mechanism, and
access-control requirements are explicit and required, rather than left as a
later follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdbd70ac-c926-4b6a-a1a9-bb7c0a76faf3
📒 Files selected for processing (1)
docs/audit-debugging-platform-prd.md
| "msg_id": { | ||
| "$ref": "#/$defs/hexString" |
There was a problem hiding this comment.
Should msg_id be the MLS content id (SHA-256 of the MLS bytes)? It’s undocumented and typed as any-length hex, not digestHex.
| "member_removed", | ||
| "member_left", |
There was a problem hiding this comment.
Just something to note, member_left vs member_removed reads as a voluntary-vs-admin split, but the engine only distinguishes them on the normal path, a departure resolved through convergence is always recorded as member_removed with no actor, even a voluntary self-leave.
We'll need to distinguish member_removed with an actor (admin removal) from member_removed with actor = None
| "welcome_event_id": { | ||
| "$ref": "#/$defs/hexString" | ||
| }, |
There was a problem hiding this comment.
welcome_event_id is hexString while its siblings (nostr_event_id, gift_wrap_event_id) are digestHex, is this intentional? Which welcome id is it: the outer kind-1059, the inner kind-444 rumor, the KeyPackage e-tag (the one that links back to the adding commit) or something else?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/audit-debugging-platform-prd.md (1)
410-429: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake the data-mode signal per object, not just per response.
A response-level classification is too coarse for the mixed-mode comparisons this PRD explicitly allows. Clients need per-derived-object mode metadata so they can tell which rows may contain plaintext versus digests.
Suggested PRD tweak
- Support filters for audit data mode and a response-level classification that tells clients whether decrypted content may be present. + Support filters for audit data mode. + Each returned derived object must carry its own audit-data-mode classification + (obfuscated_sensitive_data vs full_data); response-level metadata may only be a + coarse summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/audit-debugging-platform-prd.md` around lines 410 - 429, The audit data-mode signal is currently described only at the response level, but it needs to be tracked per derived object so mixed-mode results can be interpreted correctly. Update the PRD text around the audit data mode and response-level classification requirements to specify object-level mode metadata on each derived object/row, indicating whether that object may contain plaintext or only digests. Keep the existing least-privilege and evidence-ref guidance, but make the mode classification explicit per object rather than only per response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audit-debugging-platform-prd.md`:
- Around line 140-146: The PRD’s retention/deletion policy in the
audit-debugging platform spec needs to explicitly require that purge/delete
operations themselves create an immutable audit record. Update the policy text
around the full-data auditing requirements to state that when the documented
audit-data purge command is run, the deletion action must be written to a
separate durable audit trail that is retained even after the underlying forensic
evidence is removed; keep this aligned with the existing retention and
access-control language in the same section.
---
Outside diff comments:
In `@docs/audit-debugging-platform-prd.md`:
- Around line 410-429: The audit data-mode signal is currently described only at
the response level, but it needs to be tracked per derived object so mixed-mode
results can be interpreted correctly. Update the PRD text around the audit data
mode and response-level classification requirements to specify object-level mode
metadata on each derived object/row, indicating whether that object may contain
plaintext or only digests. Keep the existing least-privilege and evidence-ref
guidance, but make the mode classification explicit per object rather than only
per response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21824229-56ac-4646-9318-91b8a961d272
📒 Files selected for processing (1)
docs/audit-debugging-platform-prd.md
| Full data auditing must not be enabled unless the deployment has an explicit | ||
| retention, deletion, and access-control policy in place. For the current | ||
| internal-testing deployment, that policy is: retain uploaded audit evidence until | ||
| an authenticated operator runs the documented audit-data purge command, allow | ||
| read access only to authenticated internal Goggles users, and allow uploads to be | ||
| paused during cutovers or incidents. Broader deployments must define stricter | ||
| object-level access rules before full-data capture is enabled. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require purge/deletion actions to be auditable.
The retention policy allows an operator to delete forensic evidence, but the PRD never requires an immutable audit row for the deletion itself. In a forensic system, the delete action needs to survive the purge.
Suggested PRD tweak
- retain uploaded audit evidence until an authenticated operator runs the documented audit-data purge command
+ retain uploaded audit evidence until an authenticated operator runs the documented audit-data purge command,
+ and emit immutable audit rows for each purge request/result, retained independently
+ from the deleted evidence.Also applies to: 545-549
🧰 Tools
🪛 LanguageTool
[grammar] ~144-~144: Ensure spelling is correct
Context: ...d access only to authenticated internal Goggles users, and allow uploads to be paused d...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/audit-debugging-platform-prd.md` around lines 140 - 146, The PRD’s
retention/deletion policy in the audit-debugging platform spec needs to
explicitly require that purge/delete operations themselves create an immutable
audit record. Update the policy text around the full-data auditing requirements
to state that when the documented audit-data purge command is run, the deletion
action must be written to a separate durable audit trail that is retained even
after the underlying forensic evidence is removed; keep this aligned with the
existing retention and access-control language in the same section.
2 participants
Summary
Adds a draft PRD for evolving Goggles from a raw audit-log browser into a forensic debugging platform for Marmot audit logs.
The PRD covers:
Validation
just checkSummary by CodeRabbit