audit trail system — SEC-001 phases 2b/3/4/5 (#20)

[dolho]

Summary

Complete implementation of the Trinity platform audit trail (SEC-001 / #20) across four phases, landed together on this branch.

• Phase 2b — Agent lifecycle, auth (login success/failure), sharing, credentials, settings, rename; request-ID correlation middleware.
• Phase 3 — MCP tool-call audit via transparent wrapper covering all 71 tools (user/agent/system scopes), with failure tracking.
• Phase 4 — Append-only SQLite triggers + optional SHA-256 hash chain with verify + JSON/CSV export + stats/filter API.
• Phase 5 (this commit) — Fills remaining spec actions declared in docs/requirements/AUDIT_TRAIL_ARCHITECTURE.md and fixes a hash-chain verify bug.

Action coverage vs spec (AuditEventType)

Category	Covered	Notes
agent_lifecycle	create, start, stop, delete, rename	recreate has no public endpoint
execution	chat_started, task_triggered, schedule_triggered	all three spec actions
authentication	login_success, login_failed	logout / token_refresh — no endpoints in Trinity
authorization	share, unshare, permission_grant, permission_revoke, permissions_set	all spec actions
configuration	settings_change, resource_limits, autonomy_toggle	all spec actions
credentials	inject, export, import, oauth_complete	CRED-002 replaced spec's create/delete/reload with file-injection ops
mcp_operation	tool_call, key_create, key_revoke, key_delete	all spec actions
git_operation	sync, pull, init	commit folded into sync endpoint
system	startup, shutdown, emergency_stop	all spec actions

Hash chain fix

_compute_hash normalizes details to dict before serializing. Write-time stored details as a JSON string (json.dumps); read-time (_row_to_dict) deserialized back to a dict. Re-hashing the two forms produced different SHA-256 inputs, so verify_chain returned valid=false for every entry with non-null details. Reproduced live (test 13 in the manual plan) and confirmed fixed after the change.

API surface

• GET /api/audit-log — list + filters (event_type, actor_type, actor_id, target_type/id, source, start/end_time)
• GET /api/audit-log/{event_id} — single entry
• GET /api/audit-log/stats — totals + breakdowns
• GET /api/audit-log/export?format=json|csv
• POST /api/audit-log/verify?start_id=&end_id=
• POST /api/audit-log/hash-chain/enable?enabled=true|false
• POST /api/internal/audit — fire-and-forget write path for MCP server
• Admin-only; non-admin returns 403

Storage

audit_log table in the main SQLite DB, distinct from the Process Engine's audit_entries (per AUDIT_TRAIL_ARCHITECTURE.md decision). Append-only enforced by SQLite triggers: UPDATE blocked unconditionally, DELETE blocked within a 365-day retention window.

Test plan

• Manual test plan checked in at docs/testing/audit-trail-manual-test-plan.md — 19 acceptance checks across request-ID, auth, sharing, credentials, settings, rename, MCP user/agent/failure scopes, hash chain enable/verify/disable, JSON/CSV export, stats/filters, non-admin 403.
• 18/19 passed live. Test 13 (hash chain verify) regressed during run and is fixed by this commit; re-verified valid: true, checked: 3 after the fix.
• Phase 5 integrations smoke-tested live: system.startup/shutdown, mcp_operation.key_create/revoke/delete, configuration.autonomy_toggle/resource_limits, authorization.permission_grant/revoke, execution.chat_started/task_triggered, system.emergency_stop.
• Wired but not yet live-tested (require natural triggers): git_operation.sync/pull/init (git-enabled agent), execution.schedule_triggered (cron fire), credentials.oauth_complete (Slack OAuth flow).
• Confirm email login_success audit entry (only failure path exercised so far).
• UI/export smoke test in the admin Audit Log page.

Why WIP

PR marked wip: because:

1. Three wired integrations lack live coverage as noted above.
2. No unit tests added yet for the new audit call sites (existing service has coverage; integration coverage is via the manual plan).
3. Frontend audit log page needs a visual check.

🤖 Generated with Claude Code

[dolho]

Post-rebase status (2026-04-20):

• Rebased onto main (88e5525). Resolved 3 conflicts: architecture.md, settings.py (kept PAT propagation feat: Auto-propagate GitHub PAT to all agents on update #211 + audit log), main.py imports.
• 51 unit tests in tests/test_audit_log_unit.py — all pass (phases 1, 2a, 2b, 3, 4, 5 covered at service layer). The "no unit tests added yet" note in original PR body is outdated.
• Manual integration plan remains the coverage for live router → service flow (19/19 passed).

Remaining gaps (per PR body):

• Live coverage for git_operation.sync/pull/init, execution.schedule_triggered, credentials.oauth_complete — wired but require natural triggers. Low risk.
• Frontend audit log page visual check.

Propose: merge as-is, open follow-up for UI check + natural-trigger verification.

[vybe]

PR Validation Report — /validate-pr

Mergeable: ✅ MERGEABLE · State: OPEN (author-flagged WIP) · Files: 25 (+1944 / −177)

Summary

Category	Status	Notes
Commit Messages	✅	Conventional (feat/docs/wip), detailed bodies. Phase 5 uses wip: deliberately.
GitHub Issue	✅	#20 referenced; labels intact (priority-p1, type-feature).
Requirements	✅	docs/memory/requirements.md — SEC-001 expanded across all 5 phases.
Architecture	⚠️	docs/memory/architecture.md endpoint table lists only Phase 1 endpoints — missing /export, /verify, /hash-chain/enable, /api/internal/audit.
Feature Flow	✅	feature-flows/audit-trail.md updated (+168 / −19).
Security Check	✅	No secrets, keys, real emails, IPs, env files, or credential files.
Tests	⚠️	51 unit tests pass; no unit coverage added for Phase 2b/5 call sites (acknowledged).
Manual Test Plan	✅	18/19 acceptance checks passed live; hash-chain regression reproduced and fixed.

Security Checklist

• ✅ No secrets/keys/emails/IPs in diff
• ✅ Credential router logs only file path keys, never values
• ✅ Auth audit records username only on failure; success targets user.username via target_id
• ✅ MCP internal audit endpoint gated by verify_internal_secret (C-003, hmac.compare_digest)
• ✅ INTERNAL_API_SECRET already in .env.example:22

Issues Found

Critical (Block Merge): none.

Warnings (Review Required):

1. Author self-flagged WIP. PR description's "Why WIP" lists three open items:
   • 3 integrations wired but not live-tested: git_operation.sync/pull/init, execution.schedule_triggered, credentials.oauth_complete
   • No unit tests added for new audit call sites (only service layer has 51 tests)
   • Frontend audit log page visual check pending
2. architecture.md endpoint table stale. §Platform Audit Log lists 3 endpoints; 4 new ones (/export, /verify, /hash-chain/enable, /api/internal/audit) are in requirements.md but not in arch. Keeps invariants Feature/vector log retention #3/feat: SMARTS trading pipeline with Telegram notifications and Miro visualization #13 in sync.
3. Phase 5 commit uses wip: prefix (33d12c0) — cosmetic if squash-merged, but preserved in a merge commit.

Suggestions (Optional):

1. src/mcp-server/src/server.ts:189–220 — createXTools() is invoked twice (once for registration, once for counting). Capture once into locals.
2. Add a unit test for the _compute_hash string-vs-dict details round-trip — that's the exact Phase 5 bug; a test would prevent regression.
3. PlatformAuditService.enable_hash_chain seed (services/platform_audit_service.py:190–199) swallows all exceptions without logging — consider logger.warning.

Recommendation

REQUEST CHANGES (light) / NEEDS DISCUSSION

Implementation quality is high — clean service boundaries, disciplined call sites (credential audit logs paths not values, request-ID middleware wires request.state.request_id everywhere, fire-and-forget semantics preserved, hash-chain bug caught and fixed with clear reproduction note). Security surface is clean.

The only reason not to land this as-is is that the author explicitly marked it WIP with three pending items they identified themselves.

Suggested path to merge:

• Add the 4 new endpoints to docs/memory/architecture.md §Platform Audit Log table
• Either (a) live-test the 3 uncovered integrations and tick them in the PR description, or (b) open a follow-up issue and drop the WIP flag
• Add a minimal unit test for the hash-chain round-trip (prevents regression of the bug this PR fixed)
• Drop wip: prefix from Phase 5 commit if merge-committing (no-op if squashing)

Nice-to-have:

• Deduplicate createXTools() calls in src/mcp-server/src/server.ts
• Log a warning on hash-chain seed failure

---

🤖 Generated via /validate-pr with Claude Code

[dolho]

PR Validation Report — /validate-pr (post-rebase 2026-04-22)

State: OPEN · Mergeable: ✅ MERGEABLE (CLEAN) · Files: 25 (+2011 / −177)

Summary

Category	Status	Notes
Commit Messages	✅	Conventional (feat/docs/wip/fix), detailed bodies. Phase 5 = wip:.
GitHub Issue	✅	#20 open, labels priority-p1 + type-feature. Closed on merge.
Requirements	✅	requirements.md §20.1 SEC-001 expanded across phases.
Architecture	✅	architecture.md §Platform Audit Log lists all 7 endpoints (prior gap fixed in 681956f).
Feature Flow	✅	feature-flows/audit-trail.md (+168/−19), all required sections present, phase tables filled.
Security Check	✅	No keys / real emails / public IPs / .env / hardcoded secrets / credential files.
Code Quality	✅	Focused scope, audit wrapper centralizes MCP side, createXTools() dedup landed.
Tests	⚠️	53 unit tests pass (incl. 2 new hash-chain regression tests). Route-level integration still via manual plan.
Manual Test Plan	✅	19/19 acceptance checks passed live; hash-chain regression reproduced + fixed.

Security Checklist

• ✅ No API keys/tokens in diff
• ✅ No real emails (only noreply@anthropic.com + @router / @app.middleware decorators)
• ✅ No public IPs
• ✅ No .env / credential files staged
• ✅ Credential router logs file-path keys, never values
• ✅ Auth failure audit records username only; success targets resolved user
• ✅ /api/internal/audit gated by verify_internal_secret (C-003, hmac.compare_digest)

Issues Found

Critical: none.

Warnings:

1. Author-flagged WIP remnants — three wired integrations (git_operation.sync/pull/init, execution.schedule_triggered, credentials.oauth_complete) still lack live coverage. Low risk; natural-trigger only.
2. Frontend audit log page visual check pending (unchanged since last report).

Suggestions: none remaining — prior validation's 3 non-blockers all addressed in 681956f (endpoint table, createXTools() dedup, seed-failure warning, hash-chain regression test).

Recommendation

APPROVE (with WIP-prefix caveat)

Rebase clean onto main (67fbe74). All three non-blocking suggestions from the prior /validate-pr run are resolved in 681956f. Mergeable=CLEAN, 53/53 unit tests pass, security surface clean, docs + architecture + feature flow all aligned. Only open items are author-acknowledged WIP items that are low-risk follow-ups.

Suggested path to merge:

• Squash-merge (auto-drops the wip: Phase 5 commit prefix), OR reword to feat: if preserving history.
• Open follow-up issue for natural-trigger verification + frontend visual check, then drop WIP tag.

---

🤖 Generated via /validate-pr with Claude Code

[dolho]

🤖 PR Reviewer (Trinity)

PR Review — audit trail system — SEC-001 phases 2b/3/4/5 (#20)

Overall: Large but well-structured multi-phase PR. The hash-chain fix is legitimate and the audit-never-blocks-caller invariant is correctly maintained throughout. Several items to flag.

Bugs

Hash chain still has a serialization fragility: The fix normalizes details to dict before hashing at write time, but if any audit call site passes details as a non-JSON-serializable object (e.g. a Pydantic model, a set), json.dumps will raise at hash-compute time. The existing PlatformAuditService.log callers in this diff all pass plain dicts, but future callers may not. The hash computation should defensively str()-fallback or validate details type at the service boundary.

request.client can be None in all new call sites (agent_config.py:834, agent_files.py, agent_rename.py, etc.): request.client.host if request.client else None is correctly guarded in some places, but at least the rename handler at agent_rename.py:980 accesses request.client.host — verify that the guard is consistently applied across every new audit call site (the diff shows it is in most, but a scan of agent_rename.py is warranted).

autonomy_toggle audit fires even when set_autonomy_status_logic raises (agent_config.py:827-842): The audit log() call is unconditional after await set_autonomy_status_logic(...). If the logic raises an HTTPException (e.g. agent not found), the exception propagates before reaching the audit call — so in practice no spurious audit row is written. But the ordering is fragile: if the logic is ever refactored to return normally on error, a false audit row will appear. Consider auditing inside a try/finally or only on success, consistent with the pattern used in agents.py.

Security

MCP audit endpoint POST /api/internal/audit accepts actor_agent_name from the request body (internal.py per the PR description): If the shared secret INTERNAL_API_SECRET is ever leaked, an attacker can inject arbitrary audit rows with any actor attribution. This is a known trade-off of the fire-and-forget pattern, but it should be documented as a threat model assumption. Consider binding actor_agent_name server-side from the MCP key's identity rather than trusting the body.

INTERNAL_API_SECRET added to docker-compose.yml as blank default (docker-compose.yml:10): INTERNAL_API_SECRET=${INTERNAL_API_SECRET:-} means if the env var is unset the secret is an empty string. An empty shared secret effectively disables authentication on POST /api/internal/audit. Document that operators must set this variable in .env and consider rejecting empty secrets at startup.

Credential values: Audit call sites for credentials correctly log only file paths and counts — no values. Good.

Missing test coverage (noted as WIP)

• git_operation (sync/pull/init), execution.schedule_triggered, and credentials.oauth_complete lack live test coverage (acknowledged in PR body). These are real gaps for a security audit feature — worth tracking as follow-up issues rather than leaving in the WIP note.
• No unit tests for the Phase 5 call sites (e.g. emergency_stop, chat_started, key_create/revoke/delete). The existing 51-test suite covers the service layer but not integration shapes for new call sites.

Scope

This PR spans 25 files and 4+ phases, which makes it hard to review atomically. The WIP flag is appropriate. The PR description's "Why WIP" section is honest and appreciated.