feat: support exporting compliance audit logs as CSV#438
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CSV export for compliance reporting, persists audit logs to disk on engine startup, exposes JSON/CSV API responses, adds CLI export commands, and extends tests for CSV generation and filtering. ChangesCSV Compliance Export
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@arcgod-design Thanks for the contribution. Before merge, I'd like some clarification around the export source. The CSV export functionality itself looks useful, but I want to ensure the exported report reflects actual compliance/audit findings rather than data generated from a fresh local compliance engine at export time. Please clarify:
Also, let's wait for the remaining CI checks to complete before making a merge decision. Once that's clarified and CI is green, I'll take another look. |
🧪 PR Test Results
Python 3.12 · commit 3c53f27 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_compliance.py (2)
248-315: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a persisted-audit-log CSV test to lock the audit-source contract.
Current tests validate in-memory entries only. They don’t cover the key contract that CSV export reflects persisted JSONL audit history (the main maintainer concern).
Suggested test addition
+import json @@ def test_compliance_reporter_generate_csv_includes_allowed(): @@ actions = [row[5] for row in rows[1:]] assert "updated" in actions + + +def test_compliance_reporter_generate_csv_uses_persisted_audit_log(tmp_path): + audit_path = tmp_path / "audit.jsonl" + persisted = { + "audit_id": "persisted-1", + "timestamp": datetime.now(UTC).isoformat(), + "principal_id": "u1", + "event_type": "permission_denied", + "resource": "policy", + "action": "safety:override", + "allowed": False, + "session_id": "", + "details": {}, + } + audit_path.write_text(json.dumps(persisted) + "\n", encoding="utf-8") + + engine = GovernanceEngine(audit_log_path=audit_path) + reporter = ComplianceReporter(engine) + rows = list(csv.DictReader(io.StringIO(reporter.generate_csv(include_allowed=True)))) + + assert len(rows) == 1 + assert rows[0]["audit_id"] == "persisted-1" + assert rows[0]["event_type"] == "permission_denied"🤖 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 `@tests/test_compliance.py` around lines 248 - 315, Add a new test function that validates CSV export from persisted JSONL audit history, not just in-memory entries. The test should create a ComplianceReporter with a GovernanceEngine that has been initialized from a persisted JSONL audit log file, then call generate_csv() and verify the output contains the persisted audit entries with correct format and fields. This locks the contract that CSV export accurately reflects the persisted audit source data, which is the main concern for audit compliance tracking.
274-315: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten CSV assertions to avoid false positives.
A few checks are currently too permissive (
"permission_denied" in csv_output,len(rows) >= ...). With a deterministic fixture, column-level checks and stricter row expectations will catch regressions earlier.Suggested assertion hardening
def test_compliance_report_to_csv_contains_denials(): @@ report = reporter.generate() csv_output = report.to_csv() - assert "permission_denied" in csv_output + rows = list(csv.DictReader(io.StringIO(csv_output))) + assert any(row["event_type"] == "permission_denied" for row in rows) @@ def test_compliance_reporter_generate_csv_denials_only(): @@ rows = list(reader) - assert len(rows) >= 2 + assert len(rows) == 3 # header + 2 denied rows from fixture for row in rows[1:]: - assert "permission_denied" in row[3] or "safety_override" in row[3] + assert row[3] == "permission_denied" + assert row[6] == "False" @@ def test_compliance_reporter_generate_csv_includes_allowed(): @@ rows = list(reader) - assert len(rows) >= 3 + assert len(rows) == 4 # header + 3 total rows from fixture actions = [row[5] for row in rows[1:]] assert "updated" in actions🤖 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 `@tests/test_compliance.py` around lines 274 - 315, The CSV assertions in these test functions are too permissive and may not catch regressions effectively. In test_compliance_report_to_csv_contains_denials, replace the broad string check with CSV parsing and column-level validation to verify the denial appears in the correct column. In test_compliance_reporter_generate_csv_denials_only, replace the >= comparison with an exact row count assertion and check specific columns (like column index 3 for event type) instead of searching for the string anywhere in the row. In test_compliance_reporter_generate_csv_includes_allowed, similarly replace len(rows) >= 3 with an exact equality check and validate that specific columns contain the expected values rather than just checking if "updated" exists in a loosely defined list.
🤖 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 `@agentwatch/api/server.py`:
- Around line 1129-1133: The `format` parameter in the compliance_audit_log_csv
function is accepted as a Query parameter but is never actually used within the
function implementation, making the API contract misleading. Remove the unused
`format: str = Query("csv", alias="format")` parameter from the function
signature of compliance_audit_log_csv to clarify that the endpoint only returns
CSV format and eliminate the ambiguity about whether the parameter is respected.
In `@agentwatch/cli/main.py`:
- Around line 1927-1939: The reported entry count in the success message at the
end of the command execution is inaccurate because `loaded =
len(engine._audit_log)` counts all audit entries, but when `include_allowed` is
False, the CSV generated by
`reporter.generate_csv(include_allowed=include_allowed)` filters to only denials
and overrides. Additionally, the code accesses the private `_audit_log`
attribute instead of using the public API. Calculate the actual count of entries
that were exported by counting the entries in the `csv_content` returned by
generate_csv (accounting for the header row), and use that count in the success
message console.print statement to accurately reflect what was written to the
file. Optionally, replace the direct access to `engine._audit_log` with the
public `engine.stats()` method if available.
In `@agentwatch/governance/compliance_reporter.py`:
- Around line 95-97: The generate_csv() method hard-codes a limit of 10_000 rows
in the self._governance.get_audit_log(limit=10_000) call, which silently
truncates audit records and produces incomplete compliance CSV exports without
user awareness. Remove the hard-coded limit parameter to retrieve all available
records, or if a limit is necessary for performance, implement pagination to
fetch all records in batches or add a clear warning message when results are
truncated so users know the export may be incomplete.
In `@agentwatch/governance/engine.py`:
- Around line 199-215: Valid JSON lines that are not objects (such as arrays or
primitives) will raise a TypeError when attempting to access fields using
bracket notation (e.g., data["audit_id"]), but this TypeError is not currently
caught by the exception handler. Add TypeError to the exception tuple in the
except clause alongside json.JSONDecodeError, KeyError, and ValueError so that
these corrupt entries are properly logged as warnings instead of causing the
load to crash unexpectedly.
---
Nitpick comments:
In `@tests/test_compliance.py`:
- Around line 248-315: Add a new test function that validates CSV export from
persisted JSONL audit history, not just in-memory entries. The test should
create a ComplianceReporter with a GovernanceEngine that has been initialized
from a persisted JSONL audit log file, then call generate_csv() and verify the
output contains the persisted audit entries with correct format and fields. This
locks the contract that CSV export accurately reflects the persisted audit
source data, which is the main concern for audit compliance tracking.
- Around line 274-315: The CSV assertions in these test functions are too
permissive and may not catch regressions effectively. In
test_compliance_report_to_csv_contains_denials, replace the broad string check
with CSV parsing and column-level validation to verify the denial appears in the
correct column. In test_compliance_reporter_generate_csv_denials_only, replace
the >= comparison with an exact row count assertion and check specific columns
(like column index 3 for event type) instead of searching for the string
anywhere in the row. In test_compliance_reporter_generate_csv_includes_allowed,
similarly replace len(rows) >= 3 with an exact equality check and validate that
specific columns contain the expected values rather than just checking if
"updated" exists in a loosely defined list.
🪄 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 Plus
Run ID: bcbb464b-a004-4db7-955d-4e0c7e57b3ac
📒 Files selected for processing (5)
agentwatch/api/server.pyagentwatch/cli/main.pyagentwatch/governance/compliance_reporter.pyagentwatch/governance/engine.pytests/test_compliance.py
| async def compliance_audit_log_csv( | ||
| _auth: None = Depends(_require_api_key), | ||
| format: str = Query("csv", alias="format"), | ||
| include_allowed: bool = Query(False), | ||
| ): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove or enforce format on /api/v1/compliance/audit-log.
format is accepted but ignored; the handler always returns CSV. This makes the API contract ambiguous.
🔧 Proposed fix (remove unused parameter)
async def compliance_audit_log_csv(
_auth: None = Depends(_require_api_key),
- format: str = Query("csv", alias="format"),
include_allowed: bool = Query(False),
):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def compliance_audit_log_csv( | |
| _auth: None = Depends(_require_api_key), | |
| format: str = Query("csv", alias="format"), | |
| include_allowed: bool = Query(False), | |
| ): | |
| async def compliance_audit_log_csv( | |
| _auth: None = Depends(_require_api_key), | |
| include_allowed: bool = Query(False), | |
| ): |
🤖 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 `@agentwatch/api/server.py` around lines 1129 - 1133, The `format` parameter in
the compliance_audit_log_csv function is accepted as a Query parameter but is
never actually used within the function implementation, making the API contract
misleading. Remove the unused `format: str = Query("csv", alias="format")`
parameter from the function signature of compliance_audit_log_csv to clarify
that the endpoint only returns CSV format and eliminate the ambiguity about
whether the parameter is respected.
Where the exported records originate from: The CSV export reads directly from the persisted JSONL audit log file on disk. When the server runs with AGENTWATCH_AUDIT_LOG_PATH set, every compliance decision (denials, overrides, approvals) is appended to that JSONL file in real-time. The GovernanceEngine loads this file into memory on init, and the ComplianceReporter.generate_csv() iterates over those loaded entries to produce the CSV. CLI vs on-demand: Both paths export the same persisted history — there's no synthetic data generation. The CLI export-local command (agentwatch compliance export-local --audit-log ) reads from the same JSONL file the server writes to. The API endpoint /api/v1/compliance/audit-log does the same thing but serves it as an HTTP response. Neither path creates new audit entries — they only serialize what's already been recorded. How local export maps to real audit findings: The JSONL file is the single source of truth. Each line is a JSON object with audit_id, timestamp, principal_id, event_type, resource, action, allowed, and details. The CSV is a 1:1 row-per-entry mapping of this data. When --include-allowed is false (default), only denials and overrides are exported — the same entries that would appear in a compliance review. The key invariant: if it's not in the JSONL file, it won't be in the CSV. The export is purely a format conversion of persisted audit records. |
…add CSV format to API endpoint - GovernanceEngine: add load_from_disk() to reload persisted JSONL entries - Server: enable audit_log_path with AGENTWATCH_AUDIT_LOG_PATH env var - Server: add ?format=csv to compliance-report endpoint + dedicated /api/v1/compliance/audit-log - CLI export-local: load from JSONL file instead of empty engine, add --audit-log option - CLI export-csv: hit correct endpoint (/api/v1/governance/compliance-report) Closes sreerevanth#148
…t, catch TypeError in audit log loader (closes sreerevanth#148)
arcgod-design
force-pushed
the
feat/issue-148-csv-export
branch
from
94f9a45 to
0fc4414
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@agentwatch/cli/main.py`:
- Around line 146-148: The REPL clear path in `main.py` is shelling out with
`os.system`, which violates the CLI utility rule. Replace that direct shell call
with `console.clear()` in the same REPL flow, and keep all command execution
routed through `agentwatch/cli/_utils`. Also move the Docker check used by
`doctor` behind a helper in `agentwatch/cli/_utils` and call that helper instead
of invoking `subprocess.run` directly, so the `doctor` and clear paths both use
the approved CLI layer.
- Around line 1924-1940: The export-local command is still defaulting to the
hardcoded audit log path instead of the persisted location used elsewhere.
Update compliance_export_local to source its default audit_log path from
AGENTWATCH_AUDIT_LOG_PATH (the same path used by audit log persistence) and keep
_DEFAULT_AUDIT_LOG_PATH only as the fallback if needed, so export-local reads
the correct file without requiring an explicit flag.
🪄 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 Plus
Run ID: b7793a79-dff2-4219-90ac-da6cd57bc28b
📒 Files selected for processing (5)
agentwatch/api/server.pyagentwatch/cli/main.pyagentwatch/governance/compliance_reporter.pyagentwatch/governance/engine.pytests/test_compliance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- agentwatch/governance/compliance_reporter.py
- tests/test_compliance.py
- agentwatch/api/server.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 `@agentwatch/cli/main.py`:
- Around line 146-148: The REPL clear path in `main.py` is shelling out with
`os.system`, which violates the CLI utility rule. Replace that direct shell call
with `console.clear()` in the same REPL flow, and keep all command execution
routed through `agentwatch/cli/_utils`. Also move the Docker check used by
`doctor` behind a helper in `agentwatch/cli/_utils` and call that helper instead
of invoking `subprocess.run` directly, so the `doctor` and clear paths both use
the approved CLI layer.
- Around line 1924-1940: The export-local command is still defaulting to the
hardcoded audit log path instead of the persisted location used elsewhere.
Update compliance_export_local to source its default audit_log path from
AGENTWATCH_AUDIT_LOG_PATH (the same path used by audit log persistence) and keep
_DEFAULT_AUDIT_LOG_PATH only as the fallback if needed, so export-local reads
the correct file without requiring an explicit flag.
🪄 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 Plus
Run ID: b7793a79-dff2-4219-90ac-da6cd57bc28b
📒 Files selected for processing (5)
agentwatch/api/server.pyagentwatch/cli/main.pyagentwatch/governance/compliance_reporter.pyagentwatch/governance/engine.pytests/test_compliance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- agentwatch/governance/compliance_reporter.py
- tests/test_compliance.py
- agentwatch/api/server.py
🛑 Comments failed to post (2)
agentwatch/cli/main.py (2)
146-148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Route shell-outs through CLI utilities.
os.system(...)and the Dockersubprocess.run(...)bypass the required CLI utility layer. Preferconsole.clear()for REPL clearing, and move the Docker probe behindagentwatch/cli/_utilsbefore calling it fromdoctor. As per coding guidelines, "agentwatch/cli/**/*.py: CLI commands must be routed through agentwatch/cli/_utils — do not shell out directly."Suggested local cleanup for the REPL clear path
- os.system( # noqa: S605, S607 - "cls" if os.name == "nt" else "clear" - ) # nosec + console.clear()Also applies to: 2009-2031
🤖 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 `@agentwatch/cli/main.py` around lines 146 - 148, The REPL clear path in `main.py` is shelling out with `os.system`, which violates the CLI utility rule. Replace that direct shell call with `console.clear()` in the same REPL flow, and keep all command execution routed through `agentwatch/cli/_utils`. Also move the Docker check used by `doctor` behind a helper in `agentwatch/cli/_utils` and call that helper instead of invoking `subprocess.run` directly, so the `doctor` and clear paths both use the approved CLI layer.Source: Coding guidelines
1924-1940: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find all references to the audit log path env var and the export command. rg -n "AGENTWATCH_AUDIT_LOG_PATH|audit-log|export-local|_DEFAULT_AUDIT_LOG_PATH" agentwatch -S # Inspect the relevant section of the CLI file with line numbers. sed -n '1900,1965p' agentwatch/cli/main.py | cat -nRepository: sreerevanth/AgentWatch
Length of output: 3780
Honor
AGENTWATCH_AUDIT_LOG_PATHinexport-local.compliance_export_localstill defaults todata/audit-log.jsonl, while persistence usesAGENTWATCH_AUDIT_LOG_PATH, so the command can export the wrong or empty file unless the path is passed explicitly.🤖 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 `@agentwatch/cli/main.py` around lines 1924 - 1940, The export-local command is still defaulting to the hardcoded audit log path instead of the persisted location used elsewhere. Update compliance_export_local to source its default audit_log path from AGENTWATCH_AUDIT_LOG_PATH (the same path used by audit log persistence) and keep _DEFAULT_AUDIT_LOG_PATH only as the fallback if needed, so export-local reads the correct file without requiring an explicit flag.
|
Someone is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Closes #148 |
|
Hey @sreerevanth, this PR has been ready for review for a while -- offering this friendly bump. The implementation matches the issue's specified behavior and CI is passing. PR: #438 When you have a moment, a review would be much appreciated. I am happy to iterate on any feedback. If the timing is not right, no worries at all -- just a gentle nudge from a contributor who has been waiting patiently. � |
2 participants
Summary
Changes
Testing
All 5 CSV tests pass:
Closes
closes #148
Summary by CodeRabbit
compliancesection with commands to export compliance data to CSV (from the API or a persisted local audit log).