Repository navigation
fix(agent-mesh): export complete audit logs - #4096
Conversation
Signed-off-by: Electric Wolfe Marshmallow Hypertext <tionne@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- agent-governance-python/agent-mesh/CHANGELOG.md:8 Add a line under Unreleased / Fixed: exports no longer cap at 10,000 records. The earlier Merkle-root fix has an entry there.
- Nit: the
large_audit_logfixture is function-scoped and builds 10,001 entries seven times (about 7.4 s for the class). Nothing mutates it, so class or module scope cuts that to about 1 s.
Signed-off-by: Electric Wolfe Marshmallow Hypertext <tionne@gmail.com>
MohammadHaroonAbuomar Addressed review items:
One other thing: branch is currently 19 commits behind main, can merge cleanly. Do you want me to update it now, or leave it as-is? |
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Approving at e1724aa. All five points are verified in code and CI is green (122 pass). The unbounded path is explicit now, the concurrent-append test discriminates (it fails with the previous form restored), the CHANGELOG line is accurate, and the fixture rebuild is down to about one second for the class.
No need to update the branch: it merges cleanly against main, and I will squash-merge it now.
One note for later, not for this PR: export() still reads the root after query() returns, so an append landing in that gap gives a root for N+1 entries against N records. AuditLog has no locking anywhere, so that is a broader question than this fix.
MohammadHaroonAbuomar I dug into the follow-up race you called out. I reproduced the N-records/N+1-root mismatch deterministically, including a case where merkle_root and chain_root can disagree within the same export. I have a local fix that snapshots the entries and root together under the same synchronization used by appends, with deterministic regression coverage. OK to open a focused follow-up PR for this? |
* fix(agent-mesh): export complete audit logs Signed-off-by: Electric Wolfe Marshmallow Hypertext <tionne@gmail.com> * fix(agent-mesh): address audit export review Signed-off-by: Electric Wolfe Marshmallow Hypertext <tionne@gmail.com> --------- Signed-off-by: Electric Wolfe Marshmallow Hypertext <tionne@gmail.com> Signed-off-by: yuvrajsingh2428 <offcyuvi2428@gmail.com>
Related Issue
Fixes #4095
Problem & Solution
AuditLog.export()andAuditLog.export_cloudevents()silently returned only the newest 10,000 matching records. Larger exports omitted earlier records without warning. The plain export still reported the Merkle root for the entire log, so the exported records could not reproduce that root.This change removes the fixed export limit by requesting the current number of records in the audit chain. Both formats now include every matching record. The separate
query()API keeps its existing default and explicit limits.Time filters and the existing full-chain root semantics are unchanged: a filtered plain export still reports the root for the complete log, not a new root calculated from the selected records.
Impact on Your Work
Found while testing audit export completeness and independent verification. This fixes incomplete exports above 10,000 records without requiring callers to retrieve the records and assemble their own export.
Timeline
None.
Alternatives Considered
Increasing the fixed limit would only move the failure to a larger log. Changing the public
query()API is unnecessary for this fix.Type of Change
Package(s) Affected
Only two files are changed:
agent-governance-python/agent-mesh/src/agentmesh/governance/audit.pyagent-governance-python/agent-mesh/tests/test_merkle_audit_root_reproducible.pyTesting
Unit Testing
Added seven regression cases covering:
query()limits remaining unchanged.Both 10,001-record completeness tests failed before the fix because only 10,000 records were returned. Both pass after the fix.
Manual Testing
Tested on Windows 11 with Python 3.12.10 against commit
c799225e50:test_merkle_audit_root_reproducible.py,test_governance.py,test_persistent_audit.py,governance/test_audit_backends.py, andgovernance/test_stdout_audit_sink.py.persistent_audit.pymodule and three POSIX-only tests.py_compilepassed for both changed files.git diff --checkpassed.The initial sandboxed test runs encountered temporary-directory permission errors. The final run above completed successfully with the required filesystem access.
Checklist
Attribution & Prior Art
The tests reuse this repository's existing independent Merkle-root helper. No code was copied from another project.
AI Assistance
Codex assisted with the investigation, implementation, regression tests, and validation. The contributor directed the scope, reviewed the reported changes and results, and explicitly authorized this PR.