Repository navigation
fix: govern - wire audit_file to FileAuditSink so file-based audit persistence actually works - #3916
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
tests
agent-mesh
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. |
github-actions
Bot
added
size/L
Fernando Marino` (fer-marino)
changed the title
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- govern.py (_get_shared_audit_sink) — Path(path).resolve() runs before the sink sees the path, so govern(audit_file=) writes into the symlink target and the sink's O_NOFOLLOW never fires (direct FileAuditSink() correctly refuses with ELOOP). Please check for a symlink before resolving (lstat) and refuse, keeping the realpath only as the registry key.
- Path validation is still first-call: a nonexistent parent directory constructs fine and raises FileNotFoundError on the first governed call (fail-closed, but the ask was construction-time); a directory path fails only by accident via IsADirectoryError in _read_last_hash. Please validate the path (parent exists, not a directory, not a symlink) at construction next to the key check. Minor: after an external rotation (rename), a new govern() on the same path reuses the cached sink and the new file starts with a stale previous_hash (verify_integrity breaks at entry 0); and a second instance that omits the key reuses the first sink even when AGT_AUDIT_SECRET_KEY holds a different key.
github-actions
Bot
added
size/XL
|
Fixed in 97e870e: Symlink before resolve: _get_shared_audit_sink now lstats the raw path (Path.is_symlink()) and refuses before calling resolve(), so the check can't be silently defeated by resolve() already having followed the symlink away by the time a sink would see it. The realpath is still what keys the registry. Construction-time path validation: FileAuditSink.init now checks (in order) that the path isn't a symlink, isn't an existing directory, and that its parent directory exists - raising ValueError/IsADirectoryError/FileNotFoundError at construction, next to the key check, instead of on the first write(). External rotation (minor): write()/write_batch() now compare (st_dev, st_ino) against what was last synced and re-derive previous_hash if the file's identity changed - covers both a rename-away (nothing left at that inode) and a same-path replace by another writer. Env-var key mismatch on reuse (minor): the effective key (explicit arg, falling back to AGT_AUDIT_SECRET_KEY) is now resolved and checked against the existing sink on every reuse, not only when the caller passed secret_key explicitly - while a reuse with no key information available at all (no arg, no env var) still just reuses, unchanged from before. |
97e870e to
63afec3
Compare
, microsoft#3916, microsoft#3924, microsoft#3853, advance watermarks
Carlos Hernandez (carloshvp)
left a comment
Carlos Hernandez (carloshvp)
left a comment
There was a problem hiding this comment.
Reviewed current head 63afec3 and simulated its merge into current main cleanly. The two targeted files pass 74 tests both before and after that merge. The prior symlink, construction-time validation, registry, and rotation findings are materially improved, but adversarial checks found four fail-closed/integrity gaps noted inline. Ruff also reports 61 findings on the changed files versus 54 on current main, including newly unsorted imports; please clean the changed-line delta before the next pass.
|
Carlos Hernandez (@carloshvp) All four fixed in af43caf:
|
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Wiring audit_file into govern() makes AuditLog.log() able to raise from a real FileAuditSink, and the audit call inside GovernedCallable._run_advisory sits inside the try/except that converts any exception into AdvisoryDecision(action='allow'). An audit-sink write failure that first appears between the policy_evaluation write and the advisory_check write of the same call (ENOSPC at close, EPERM from fchmod, ELOOP from a planted symlink, or the sink's own fail-closed ValueError when the file was replaced by a chain that does not verify under the key) silently downgrades an advisory BLOCK to allow and the wrapped function executes. The deterministic policy path is fail-closed (the earlier write raises out of call), so this is confined to the advisory layer and to a one-call window, but it is a security control being bypassed on an error path introduced by this PR, and the fix is to move the audit_log call out of the fail-open try (or catch only advisory.check). File: agent-governance-python/agent-mesh/src/agentmesh/governance/govern.py:285-286 (sink wired), :733-760 (_run_advisory; audit log at :743-744 inside the try, except Exception -> allow at :758-759).
|
MohammadHaroonAbuomar Fixed in 6775209: narrowed |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- DCO: commit 6775209 ('fix: address review — don't fail open on an audit-write failure during the advisory check') has no Signed-off-by trailer (verified via gh api .../pulls/3916/commits and a local replay of .github/workflows/dco.yml over 1a896e7..3733cef: MISS 6775209, OK for the five earlier commits; the merge 3733cef is exempt). Fix:
git commit --amend -s --no-editon that commit (orgit rebase --signoff 591b3687) and force-push; the DCO Check run for this head is fork-gated (action_required) so it has not surfaced yet. - .cspell-repo-terms.txt: the Spell Check gate still fails at this head even though 6775209 set out to fix it. Replaying .github/workflows/spell-check.yml (scripts/ci/changed_lines.py --base origin/main --mode added-lines, then cspell@8.17.3 --config .cspell.json) exits 1 with unknown words EPERM (audit_backends.py _append_line docstring and govern.py _run_advisory docstring, tests/governance/test_audit_backends.py, tests/test_govern.py:667), fileno (audit_backends.py
os.fchmod(fh.fileno(), 0o600)), fromhex (govern.py:114bytes.fromhex(env_value)), fstat (tests/governance/test_audit_backends.py). None of the four is in .cspell-repo-terms.txt or .cspell.json on head or main. Fix: add EPERM, fileno, fromhex, fstat to the '# --- audit_backends.py / govern.py fail-closed-error comment terms (PR #3916) ---' block in .cspell-repo-terms.txt. - .cspell-repo-terms.txt: the conflict resolution in merge 3733cef re-appended two blocks main already contains ('# --- Documentation site and ACS integration terms ---' AgentRateLimiter..Vercel and '# --- cloud-board cryptography 50.0.0 audit terms ---' AESGCM..SPKI), leaving 26 duplicate entries (
sort | uniq -d= 26 at head, 0 on origin/main; e.g. AESGCM at line 44 and again in the appended block). Fix: delete those two appended blocks (lines 1083-1111 of head's file) so only the PR's own 8-term block is added below main's list.
74fcb5f to
af912a5
Compare
|
Both fixed in af912a5. DCO on 6775209: rebased with --signoff onto current main and force-pushed - that also picked up main's own drift since this branch's last merge cleanly. The bigger one: the merge conflict resolution in 3733cef didn't just duplicate the two blocks you flagged via sort | uniq -d - it also silently dropped about 35 lines of terms that exist ONLY on main (African-regulatory: POPIA/NDPA/NFIU/CBN/BVN/NIN/..., TRACE/identity-chain, Flowise, Azure credential-pattern, and some misc test-code terms - all added by unrelated PRs merged after this branch forked). Merging this PR as it stood would have regressed the spell-check gate for every one of those words, not just left harmless duplicates. Rebuilt the file as origin/main's own content plus this PR's block (now with the four missing words: EPERM, fileno, fromhex, fstat), verified with the actual CI gate: scripts/ci/changed_lines.py --base origin/main --mode added-lines piped into cspell@8.17.3 --config .cspell.json exits 0, and sort | uniq -d on the result now matches main's own pre-existing baseline (44, unrelated to this PR) instead of the 26 extra duplicates. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Verified at af912a5: rebase --signoff left every tree unchanged, the spell-check gate replays clean with the four terms added, and the cspell file is main's content plus the PR's block with no duplicates. The advisory fail-open fix holds: the try wraps only advisory.check(), and the four real-sink failure probes raise instead of allowing.
|
Approved and ready, but the branch now conflicts with main after #3914 landed (both touch govern.py and the cspell terms). Please rebase onto current main and force-push; nothing else is needed. |
|
Carlos Hernandez (@carloshvp) I re-verified your four items in code at af912a5 so the stale request can be cleared: (1) audit_backends.py:384-387 runs os.fchmod(fh.fileno(), 0o600) before every write, and a pre-existing 0644 file ends up 0600 after the first write and again after being widened between writes; (2) _read_last_hash (:429-450) verifies continuity and the HMAC of every existing entry under the sink's key at construction and on inode change, and a chain signed under another key makes write() raise with the file untouched; (3) unparsable lines are skipped consistently by read_entries, verify_file and resume through one helper (:222-247), so after a truncated line and a resumed write verify_integrity returns true and read_entries returns both real entries, with a test at test_audit_backends.py:268-287; (4) govern.py:82 and :107-118 enforce a 32-byte minimum on explicit and env-derived keys, so empty, 1-byte and 31-byte keys raise. Ruff on the changed files went from 60 findings on main to 51, with unsorted imports at zero. Two optional follow-ups, not blockers: the same key-length check on FileAuditSink.init for the direct API, and verify_file returning a skipped-line count. Dismissing your review on that basis; reopen if you disagree. |
All four items verified fixed in code at af912a5 (fchmod on every write, authenticated predecessor chain, consistent parse-skip with verified integrity, 32-byte minimum key); evidence in the PR comment above.
af912a5 to
86576c1
Compare
The added-lines cspell gate flags these against the current main after rebasing feature/audit-file-persistence — same terms class as the existing PR microsoft#3916 os-constant/fd-handling entries in this section. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- .cspell-repo-terms.txt — the rebase re-appended 30 lines at the tail (the "Documentation site and ACS integration terms" block and the "cloud-board cryptography 50.0.0 audit terms" block) that main already carries at lines 14-76, so the file now has duplicate entries. Please delete those two appended blocks and keep only this PR's own 12-word block (EPERM, fileno, fromhex, fstat and the rest are needed: cspell reports 36 hits on the added lines without them, 0 with them). Everything else is verified identical to the approved head: 26/26 fail-closed probes pass, Carlos's four items hold, 36 + 60 tests pass, ruff clean.
…olution The rebase's conflict resolution on .cspell-repo-terms.txt kept both sides of a tail conflict, but main already carries the "Documentation site and ACS integration terms" and "cloud-board cryptography 50.0.0 audit terms" blocks earlier in the same alphabetized file - so the merge duplicated 30 lines instead of deduping them. Removes the duplicate; this PR's own PR-microsoft#3916 block is unaffected. Per MohammadHaroonAbuomar's review: 36 cspell hits on the added lines without this fix, 0 with it. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
|
Fixed in 12b440d - the rebase's conflict resolution had kept both sides of a tail conflict in .cspell-repo-terms.txt, duplicating the "Documentation site and ACS integration terms" and "cloud-board cryptography 50.0.0 audit terms" blocks that main already carries earlier in the same alphabetized file. Removed the 30 duplicate lines; this PR's own 12-word block (EPERM, fileno, fromhex, fstat, etc.) is untouched. Re-ran the cspell added-lines gate (0 hits) and the full test_govern.py/test_audit_backends.py suite (81 passed) after the fix. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Verified at 12b440d: the 30 duplicated word-list lines are removed and the file adds only this PR's 12-word block on top of main; code is unchanged from the twice-approved head, 96 tests pass, fail-closed probes hold.
|
Approved and green, but this morning's merges put the branch in conflict with main again (the shared cspell term list). One more rebase onto current main and force-push, please; I will merge as soon as it lands. |
…rsistence actually works
GovernanceConfig.audit_file is documented ("Path for file-based audit
log. None = in-memory only.") but GovernedCallable.__init__ never
read it: self._audit = AuditLog() if config.audit else None always
built a sink-less AuditLog, so every audit trail was lost on process
exit regardless of what the caller configured. The top-level govern()
factory - the module's documented 2-line-integration entrypoint -
didn't even expose audit_file as a parameter.
The library already has everything needed to make this work
(FileAuditSink: hash-chained, HMAC-signed JSON-lines, and
AuditLog(sink=...)) - govern() just never connected them.
Adds audit_secret_key alongside audit_file: FileAuditSink requires an
HMAC key, which a bare path string has no way to supply. A missing
key is auto-generated per instance so audit_file alone is still
enough to get persistence; a caller who needs signatures to verify
across process restarts can supply their own.
Fixes microsoft#3915.
Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
…plicit key Per MohammadHaroonAbuomar's review: - Two govern() calls pointed at the same audit_file each built their own FileAuditSink with an independent in-memory previous_hash. Interleaved writes desynced the chain and verify_integrity() reported a break that never happened from either writer's own perspective. Sinks are now shared per resolved path via a module-level registry. - A missing audit_secret_key silently generated a random one per instance, which could never verify across a restart and, combined with the above, meant two instances sharing a file could each sign with a different key even in-process. The key is now required — pass it explicitly or set AGT_AUDIT_SECRET_KEY (hex-encoded) — rather than auto-generated. - A corrupt trailing line (write interrupted by a crash) made FileAuditSink's constructor raise, permanently blocking every future append to that file. Chain resumption now skips back to the last line that parses instead, leaving the corrupt line in place for forensics. - FileAuditSink wrote through open(path, "a"), which creates a world-readable 0644 file and follows a symlink planted at the configured path. Appends now use O_CREAT|O_NOFOLLOW with mode 0600. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
…onstruction-time path validation, rotation resync Per MohammadHaroonAbuomar's follow-up review: - _get_shared_audit_sink resolved the path before the sink ever saw it, so a symlinked audit_file was silently followed: by the time FileAuditSink opened it, the symlink component was already gone and its O_NOFOLLOW had nothing left to refuse. Now lstats (Path.is_symlink) and refuses before resolving, keeping the realpath only as the registry key. - FileAuditSink's path was still only validated lazily, on the first write() (FileNotFoundError for a missing parent, an accidental IsADirectoryError for a directory). Now checked at construction, next to the existing secret-key validation. - A file replaced out from under a long-lived sink (external log rotation, or another process) left the cached previous_hash pointing at a chain that no longer exists at that path; verify_integrity() on the replacement broke at its first entry. write()/write_batch() now detect the identity change (st_dev/st_ino) and resync before signing the next entry. - A second govern() call omitting secret_key reused an existing shared sink unconditionally, even when AGT_AUDIT_SECRET_KEY now held a different key than the sink was actually signing with. The key is resolved (env var included) and compared on every reuse now, not only when the caller passed secret_key explicitly - while still allowing a reuse with no key information available at all, exactly as before. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
…, reject weak keys Four issues from review, each a real gap in the audit trail's integrity guarantees: - FileAuditSink resumed an existing file's hash chain by reading its last content_hash without verifying it authenticated under this sink's key. A file swapped in by another process, or an attacker, with a different key got silently extended as if it were ours. _read_last_hash now verifies the whole existing chain (HMAC + continuity) before trusting it, and raises rather than resuming onto something unauthenticated - at construction and on external-rotation resync alike. - A single unparsable line (crash mid-write) was already tolerated when resuming, but read_entries()/verify_integrity() still treated any such line anywhere as a hard failure, so a file that had recovered from one crash stayed permanently unreadable/unverifiable past that point. All three now share one _iter_parsed_entries helper that skips unparsable lines consistently - this doesn't weaken the chain, since a skipped line is simply excluded from it and a real entry swapped for garbage still breaks chain continuity at the next real one. - os.open(path, O_CREAT, 0o600)'s mode only applies if the call creates the file; one that already existed at a looser mode kept it, silently, for every subsequent write of an entry that can carry call arguments. _append_line now fchmods to 0o600 after opening too. - govern(audit_secret_key=...) accepted any non-None bytes, including b"" or one-byte keys, as an HMAC key. Now rejected below 32 bytes (HMAC-SHA256's output size), for both an explicit key and one read from AGT_AUDIT_SECRET_KEY. Also sorted the imports ruff flagged as new noise on top of the existing, unrelated style findings in these files (left alone - out of scope here). One existing test asserted the vulnerable resume behavior directly (external replacement by a different-key file silently continuing the chain); split it into same-key (still resyncs) and different-key (now fails closed) cases. New tests cover the other three findings. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
…iling write _append_line's fchmod ran between os.open and os.fdopen: if it raised (EPERM, the file owned by another user) the fd from os.open was never wrapped in anything that would close it, leaking one per failed write - confirmed 5 failed writes -> 5 leaked fds. Moved inside the fdopen block, on the file object's own descriptor, so a failing fchmod now closes it via the same exception path a failing write already did. A file this process can't chmod now fails every write instead of silently keeping its existing, looser mode - documented in _append_line's docstring alongside the already-documented one it can chmod being silently tightened. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
…g the advisory check _run_advisory's try/except was meant to fail open only when advisory.check() itself fails (non-deterministic, defense-in-depth). It also wrapped the audit log() call, so a failing write from the now file-backed sink (ENOSPC, EPERM, ELOOP, a tamper-detected chain) looked identical to the classifier failing and silently turned a BLOCK into allow. Narrowed the try to advisory.check() only, matching the deterministic policy_evaluation write a few lines up, which already fails closed by being unguarded. Also adds the words this and earlier commits introduced (ELOOP, fchmod, serialisation, etc.) to the repo's cspell term list - the 'Spell-check changed files' gate would have failed on them once the fork's workflows are approved to run, same class of failure flagged on microsoft#3914. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
The added-lines cspell gate flags these against the current main after rebasing feature/audit-file-persistence — same terms class as the existing PR microsoft#3916 os-constant/fd-handling entries in this section. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com>
12b440d to
da2190a
Compare
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Verified at da2190a: rebase-only update; code files are byte-identical to the approved head and the word list adds only this PR's own block with no duplicates. Seven signed commits.
…rsistence actually works (microsoft#3916) * fix: govern - wire audit_file to FileAuditSink so file-based audit persistence actually works GovernanceConfig.audit_file is documented ("Path for file-based audit log. None = in-memory only.") but GovernedCallable.__init__ never read it: self._audit = AuditLog() if config.audit else None always built a sink-less AuditLog, so every audit trail was lost on process exit regardless of what the caller configured. The top-level govern() factory - the module's documented 2-line-integration entrypoint - didn't even expose audit_file as a parameter. The library already has everything needed to make this work (FileAuditSink: hash-chained, HMAC-signed JSON-lines, and AuditLog(sink=...)) - govern() just never connected them. Adds audit_secret_key alongside audit_file: FileAuditSink requires an HMAC key, which a bare path string has no way to supply. A missing key is auto-generated per instance so audit_file alone is still enough to get persistence; a caller who needs signatures to verify across process restarts can supply their own. Fixes microsoft#3915. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * fix: address review — share one FileAuditSink per path, require an explicit key Per MohammadHaroonAbuomar's review: - Two govern() calls pointed at the same audit_file each built their own FileAuditSink with an independent in-memory previous_hash. Interleaved writes desynced the chain and verify_integrity() reported a break that never happened from either writer's own perspective. Sinks are now shared per resolved path via a module-level registry. - A missing audit_secret_key silently generated a random one per instance, which could never verify across a restart and, combined with the above, meant two instances sharing a file could each sign with a different key even in-process. The key is now required — pass it explicitly or set AGT_AUDIT_SECRET_KEY (hex-encoded) — rather than auto-generated. - A corrupt trailing line (write interrupted by a crash) made FileAuditSink's constructor raise, permanently blocking every future append to that file. Chain resumption now skips back to the last line that parses instead, leaving the corrupt line in place for forensics. - FileAuditSink wrote through open(path, "a"), which creates a world-readable 0644 file and follows a symlink planted at the configured path. Appends now use O_CREAT|O_NOFOLLOW with mode 0600. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * fix: address second round of review — symlink check before resolve, construction-time path validation, rotation resync Per MohammadHaroonAbuomar's follow-up review: - _get_shared_audit_sink resolved the path before the sink ever saw it, so a symlinked audit_file was silently followed: by the time FileAuditSink opened it, the symlink component was already gone and its O_NOFOLLOW had nothing left to refuse. Now lstats (Path.is_symlink) and refuses before resolving, keeping the realpath only as the registry key. - FileAuditSink's path was still only validated lazily, on the first write() (FileNotFoundError for a missing parent, an accidental IsADirectoryError for a directory). Now checked at construction, next to the existing secret-key validation. - A file replaced out from under a long-lived sink (external log rotation, or another process) left the cached previous_hash pointing at a chain that no longer exists at that path; verify_integrity() on the replacement broke at its first entry. write()/write_batch() now detect the identity change (st_dev/st_ino) and resync before signing the next entry. - A second govern() call omitting secret_key reused an existing shared sink unconditionally, even when AGT_AUDIT_SECRET_KEY now held a different key than the sink was actually signing with. The key is resolved (env var included) and compared on every reuse now, not only when the caller passed secret_key explicitly - while still allowing a reuse with no key information available at all, exactly as before. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * fix: address review — fail closed on chain resume, fix permission gap, reject weak keys Four issues from review, each a real gap in the audit trail's integrity guarantees: - FileAuditSink resumed an existing file's hash chain by reading its last content_hash without verifying it authenticated under this sink's key. A file swapped in by another process, or an attacker, with a different key got silently extended as if it were ours. _read_last_hash now verifies the whole existing chain (HMAC + continuity) before trusting it, and raises rather than resuming onto something unauthenticated - at construction and on external-rotation resync alike. - A single unparsable line (crash mid-write) was already tolerated when resuming, but read_entries()/verify_integrity() still treated any such line anywhere as a hard failure, so a file that had recovered from one crash stayed permanently unreadable/unverifiable past that point. All three now share one _iter_parsed_entries helper that skips unparsable lines consistently - this doesn't weaken the chain, since a skipped line is simply excluded from it and a real entry swapped for garbage still breaks chain continuity at the next real one. - os.open(path, O_CREAT, 0o600)'s mode only applies if the call creates the file; one that already existed at a looser mode kept it, silently, for every subsequent write of an entry that can carry call arguments. _append_line now fchmods to 0o600 after opening too. - govern(audit_secret_key=...) accepted any non-None bytes, including b"" or one-byte keys, as an HMAC key. Now rejected below 32 bytes (HMAC-SHA256's output size), for both an explicit key and one read from AGT_AUDIT_SECRET_KEY. Also sorted the imports ruff flagged as new noise on top of the existing, unrelated style findings in these files (left alone - out of scope here). One existing test asserted the vulnerable resume behavior directly (external replacement by a different-key file silently continuing the chain); split it into same-key (still resyncs) and different-key (now fails closed) cases. New tests cover the other three findings. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * fix: address review — close the fd on a failing fchmod, not just a failing write _append_line's fchmod ran between os.open and os.fdopen: if it raised (EPERM, the file owned by another user) the fd from os.open was never wrapped in anything that would close it, leaking one per failed write - confirmed 5 failed writes -> 5 leaked fds. Moved inside the fdopen block, on the file object's own descriptor, so a failing fchmod now closes it via the same exception path a failing write already did. A file this process can't chmod now fails every write instead of silently keeping its existing, looser mode - documented in _append_line's docstring alongside the already-documented one it can chmod being silently tightened. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * fix: address review — don't fail open on an audit-write failure during the advisory check _run_advisory's try/except was meant to fail open only when advisory.check() itself fails (non-deterministic, defense-in-depth). It also wrapped the audit log() call, so a failing write from the now file-backed sink (ENOSPC, EPERM, ELOOP, a tamper-detected chain) looked identical to the classifier failing and silently turned a BLOCK into allow. Narrowed the try to advisory.check() only, matching the deterministic policy_evaluation write a few lines up, which already fails closed by being unguarded. Also adds the words this and earlier commits introduced (ELOOP, fchmod, serialisation, etc.) to the repo's cspell term list - the 'Spell-check changed files' gate would have failed on them once the fork's workflows are approved to run, same class of failure flagged on microsoft#3914. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> * chore: add EPERM/fileno/fromhex/fstat to cspell wordlist The added-lines cspell gate flags these against the current main after rebasing feature/audit-file-persistence — same terms class as the existing PR microsoft#3916 os-constant/fd-handling entries in this section. Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> --------- Signed-off-by: Fernando Marino <fernando.marino85@gmail.com> Signed-off-by: yuvrajsingh2428 <offcyuvi2428@gmail.com>
3 participants
…rsistence actually works
GovernanceConfig.audit_file is documented ("Path for file-based audit log. None = in-memory only.") but GovernedCallable.init never read it: self._audit = AuditLog() if config.audit else None always built a sink-less AuditLog, so every audit trail was lost on process exit regardless of what the caller configured. The top-level govern() factory - the module's documented 2-line-integration entrypoint - didn't even expose audit_file as a parameter.
The library already has everything needed to make this work (FileAuditSink: hash-chained, HMAC-signed JSON-lines, and AuditLog(sink=...)) - govern() just never connected them.
Adds audit_secret_key alongside audit_file: FileAuditSink requires an HMAC key, which a bare path string has no way to supply. A missing key is auto-generated per instance so audit_file alone is still enough to get persistence; a caller who needs signatures to verify across process restarts can supply their own.
Fixes #3915.
Related Issue
Problem & Solution
Impact on Your Work
Timeline
Alternatives Considered
Type of Change
Package(s) Affected
Core & runtime:
Governance & security:
Platform & tooling:
CLI plugins:
Shared / other:
Testing
Unit Testing
Manual Testing
Checklist
Attribution & Prior Art
Prior art / related projects (if any):
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Claude (Anthropic) drafted this change end-to-end - found the bug by reading govern.py/audit.py/audit_backends.py, implemented the fix, wrote the tests, and ran them. Fernando reviewed this PR before it was submitted.
IP, Patents, and Licensing