feat: per-turn hook audit log (#280 mitigation 3) - #314
Conversation
Adds hook_audit.jsonl alongside the existing hook_telemetry.jsonl. Where telemetry records counts, audit records the *payload* — the exact rendered <aelfrice-memory> / <aelfrice-baseline> block injected into a given turn — so a reviewer can later see what the hook actually emitted on the turn a suspect critique landed. Defaults match the spec in docs/hook_hardening.md: - Default-on. Opt-out via AELFRICE_HOOK_AUDIT=0 env var or [hook_audit] enabled = false in .aelfrice.toml. - 10 MB cap, configurable via [hook_audit] max_bytes. - Single-slot rotation: live -> hook_audit.jsonl.1 on rollover. - Fail-soft: any I/O error is traced to stderr and swallowed. Wires into both user_prompt_submit (records emitted block + locked count + n_beliefs + session_id when available) and session_start (records baseline block, hook='session_start'). The session-start path now also reads session_id from the payload (best-effort) and exposes _retrieve_baseline_with_block() as the non-formatting-only retrieval helper so the hook can pass hits to the audit record without re-querying the store. Closes the audit-log half of #280; mitigations 1+2 (framing-tag contract + render-time belief-content escape) already shipped via PR #292.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 36 minutes and 6 seconds.Comment |
Reviewer's GuideImplements a per-turn hook audit logging system that records the exact rendered hook block for user_prompt_submit and session_start into a new JSONL audit file with configurable enablement and rotation, plus supporting config resolution, session_id extraction, and comprehensive tests. Sequence diagram for per-turn hook audit write on user_prompt_submitsequenceDiagram
actor Harness
participant Hook as user_prompt_submit
participant Extract as _extract_session_id
participant Audit as _write_hook_audit_record
participant Config as load_hook_audit_config
participant DBPath as db_path
participant FS as Filesystem
Harness->>Hook: invoke with stdin payload
Hook->>Hook: _extract_prompt(raw)
Hook->>Extract: _extract_session_id(raw)
Extract-->>Hook: session_id or None
Hook->>Hook: retrieve(..., token_budget)
Hook-->>Harness: write rendered_block to stdout
Hook->>Audit: _write_hook_audit_record(hook, prompt, rendered_block, n_beliefs, n_locked, session_id)
activate Audit
Audit->>Config: load_hook_audit_config()
Config-->>Audit: HookAuditConfig(enabled, max_bytes)
Audit->>Audit: check enabled
alt audit disabled
Audit-->>Hook: return (no-op)
else audit enabled
Audit->>DBPath: db_path()
DBPath-->>Audit: Path to memory.db
Audit->>Audit: _audit_path_for_db(db_path)
Audit->>FS: _append_audit(audit_path, record, max_bytes)
FS-->>Audit: write/rotate or error
Audit-->>Hook: return
end
deactivate Audit
Sequence diagram for per-turn hook audit write on session_startsequenceDiagram
actor Harness
participant Hook as session_start
participant Extract as _extract_session_id
participant Retrieve as _retrieve_baseline_with_block
participant Audit as _write_hook_audit_record
participant Config as load_hook_audit_config
participant DBPath as db_path
participant FS as Filesystem
Harness->>Hook: invoke with stdin payload
Hook->>Hook: read raw from stdin
Hook->>Extract: _extract_session_id(raw)
Extract-->>Hook: session_id or None
Hook->>Retrieve: _retrieve_baseline_with_block(token_budget)
Retrieve->>Retrieve: retrieve(store, "", token_budget)
Retrieve-->>Hook: hits, rendered_block
alt rendered_block empty
Hook-->>Harness: return 0 (no baseline block)
else rendered_block non-empty
Hook-->>Harness: write rendered_block to stdout
Hook->>Audit: _write_hook_audit_record(hook, prompt="", rendered_block, n_beliefs=len(hits), n_locked, session_id)
activate Audit
Audit->>Config: load_hook_audit_config()
Config-->>Audit: HookAuditConfig(enabled, max_bytes)
Audit->>Audit: check enabled
alt audit disabled
Audit-->>Hook: return (no-op)
else audit enabled
Audit->>DBPath: db_path()
DBPath-->>Audit: Path to memory.db
Audit->>Audit: _audit_path_for_db(db_path)
Audit->>FS: _append_audit(audit_path, record, max_bytes)
FS-->>Audit: write/rotate or error
Audit-->>Hook: return
end
deactivate Audit
end
Class diagram for new hook audit structures and helpersclassDiagram
class HookAuditConfig {
<<dataclass>>
bool enabled
int max_bytes
}
class HookAuditModule {
+int AUDIT_DEFAULT_MAX_BYTES
+int AUDIT_PROMPT_PREFIX_CAP
+str AUDIT_FILENAME
+str AUDIT_ROTATED_SUFFIX
+str AUDIT_HOOK_USER_PROMPT_SUBMIT
+str AUDIT_HOOK_SESSION_START
+HookAuditConfig load_hook_audit_config(start, env, stderr)
+Path _audit_path_for_db(db_path_val)
+void _append_audit(audit_path, record, max_bytes, stderr)
+void _write_hook_audit_record(hook, prompt, rendered_block, n_beliefs, n_locked, session_id, config, stderr)
+list~dict~ read_hook_audit(path)
+str _extract_session_id(raw)
+tuple~list~Belief~~str~ _retrieve_baseline_with_block(token_budget)
}
class Belief {
}
HookAuditModule --> HookAuditConfig : uses
HookAuditModule --> Belief : returns list of
class HookHooks {
+int user_prompt_submit(token_budget, sin, sout, serr)
+int session_start(token_budget, sin, sout, serr)
}
HookHooks --> HookAuditModule : calls _write_hook_audit_record
HookHooks --> HookAuditModule : calls _extract_session_id
HookHooks --> HookAuditModule : calls _retrieve_baseline_with_block
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Both
user_prompt_submitandsession_startnow parse the JSON payload separately in_extract_promptand_extract_session_id; consider refactoring to parse once and thread the decoded payload through to avoid redundant work and potential divergence in behavior. _append_auditcallsos.fsyncon every write, which can be quite expensive on some filesystems; if this log is not strictly durability-critical, consider making fsync configurable or relaxing it (e.g., periodic or best-effort) to reduce latency in hot paths.- The
test_audit_write_failsoft_on_unwriteable_pathtest is annotated in comments as POSIX-only but is not conditionally skipped on Windows; consider adding apytest.mark.skipifor equivalent guard so this test doesn’t become flaky or fail on non-POSIX platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `user_prompt_submit` and `session_start` now parse the JSON payload separately in `_extract_prompt` and `_extract_session_id`; consider refactoring to parse once and thread the decoded payload through to avoid redundant work and potential divergence in behavior.
- `_append_audit` calls `os.fsync` on every write, which can be quite expensive on some filesystems; if this log is not strictly durability-critical, consider making fsync configurable or relaxing it (e.g., periodic or best-effort) to reduce latency in hot paths.
- The `test_audit_write_failsoft_on_unwriteable_path` test is annotated in comments as POSIX-only but is not conditionally skipped on Windows; consider adding a `pytest.mark.skipif` or equivalent guard so this test doesn’t become flaky or fail on non-POSIX platforms.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="432-435" />
<code_context>
+ try:
+ audit_path.parent.mkdir(parents=True, exist_ok=True)
+ line = json.dumps(record) + "\n"
+ with open(audit_path, "a", encoding="utf-8") as f:
+ f.write(line)
+ f.flush()
+ os.fsync(f.fileno())
+ if audit_path.stat().st_size > max_bytes:
+ rotated = audit_path.with_name(
</code_context>
<issue_to_address>
**suggestion (performance):** Per-record `fsync` could be expensive under high hook volume.
This gives strong durability but may become a bottleneck on busy or slow filesystems. Consider making the sync policy configurable (e.g., batch/periodic sync or OS buffering only) so operators can tune the durability vs throughput trade-off for their deployment.
Suggested implementation:
```python
with open(audit_path, "a", encoding="utf-8") as f:
f.write(line)
# sync_policy controls durability vs throughput:
# - "always": flush + fsync each record (strong durability, slowest)
# - "flush": flush only, rely on OS to sync (middle ground)
# - "none": rely entirely on OS buffering (fastest)
if sync_policy in ("always", "flush"):
f.flush()
if sync_policy == "always":
os.fsync(f.fileno())
```
To fully implement configurability of the sync behavior, you should also:
1. Update the function signature that contains this code (likely a hook audit/write helper) to accept a new parameter, for example:
- `sync_policy: str = "always"`
2. Extend the function docstring to document the `sync_policy` parameter and the supported values: `"always"`, `"flush"`, and `"none"`, and the durability/throughput trade-offs.
3. Propagate the new parameter from configuration:
- Add a configuration option (CLI flag, config file value, or environment variable) that determines the desired `sync_policy`.
- Thread that configuration value through to all call sites of this function.
4. Ensure that any direct calls to this function in tests or other modules are updated to either:
- Rely on the default `sync_policy="always"` for current behavior, or
- Explicitly pass `"flush"` or `"none"` where stronger throughput is desired.
5. Optionally, validate `sync_policy` inside the function (e.g., raise `ValueError` or fall back to `"always"` if an unknown value is provided) to avoid silent misconfiguration.
</issue_to_address>
### Comment 2
<location path="tests/test_hook_audit.py" line_range="148-157" />
<code_context>
+def test_user_prompt_submit_writes_audit_record(
</code_context>
<issue_to_address>
**suggestion (testing):** Add focused tests for `_extract_session_id` edge cases (invalid JSON, non-dict payload, missing/empty `session_id`).
Current integration tests only cover the happy path; they don’t exercise malformed JSON, non-dict roots, or missing/invalid `session_id` values. Direct unit tests on `_extract_session_id` for these cases would better guard the audit trail behavior and future payload shape changes.
Suggested implementation:
```python
# ---------------------------------------------------------------------------
# _extract_session_id unit tests
# ---------------------------------------------------------------------------
def test_extract_session_id_invalid_json() -> None:
# Malformed JSON should not raise and should return no session id
assert _extract_session_id("{not-json") is None
def test_extract_session_id_non_dict_root() -> None:
# Non-object JSON roots should be ignored
assert _extract_session_id('["not", "a", "dict"]') is None
assert _extract_session_id('"plain-string"') is None
assert _extract_session_id("123") is None
assert _extract_session_id("null") is None
def test_extract_session_id_missing_key() -> None:
# Valid JSON object without a session_id key should yield no session id
assert _extract_session_id("{}") is None
assert _extract_session_id('{"other_key": "value"}') is None
def test_extract_session_id_empty_or_non_string_value() -> None:
# Empty or non-string session_id values should be treated as absent
assert _extract_session_id('{"session_id": ""}') is None
assert _extract_session_id('{"session_id": null}') is None
assert _extract_session_id('{"session_id": 123}') is None
assert _extract_session_id('{"session_id": {}}') is None
assert _extract_session_id('{"session_id": []}') is None
# ---------------------------------------------------------------------------
# Hook integration: writes a record on UserPromptSubmit fire
# ---------------------------------------------------------------------------
def test_user_prompt_submit_writes_audit_record(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
) -> None:
db = tmp_path / "memory.db"
_seed_db(db, [_mk("F1", "the kitchen is full of bananas")])
_set_db(monkeypatch, db)
monkeypatch.delenv("AELFRICE_HOOK_AUDIT", raising=False)
monkeypatch.chdir(tmp_path)
sin = io.StringIO(_payload("bananas", session_id="sess-abc"))
sout = io.StringIO()
rc = user_prompt_submit(stdin=sin, stdout=sout)
```
1. Ensure `_extract_session_id` is imported into `tests/test_hook_audit.py` from the module under test (likely alongside `user_prompt_submit` and `load_hook_audit_config`). For example:
- `from aelfrice.hook_audit import _extract_session_id`
2. If the actual contract of `_extract_session_id` differs (e.g., it raises on malformed JSON or returns `""` instead of `None`), adjust the assertions accordingly to match the existing implementation.
</issue_to_address>
### Comment 3
<location path="tests/test_hook_audit.py" line_range="79-84" />
<code_context>
+ assert cfg.max_bytes == AUDIT_DEFAULT_MAX_BYTES
+
+
+def test_env_disable_overrides_toml(tmp_path: Path) -> None:
+ (tmp_path / ".aelfrice.toml").write_text(
+ "[hook_audit]\nenabled = true\n", encoding="utf-8",
+ )
+ cfg = load_hook_audit_config(
+ start=tmp_path, env={"AELFRICE_HOOK_AUDIT": "0"},
+ )
+ assert cfg.enabled is False
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to confirm that `AELFRICE_HOOK_AUDIT` values other than "0" do *not* disable auditing.
Current tests cover only the disabling case (`AELFRICE_HOOK_AUDIT="0"` and TOML). Since the code checks specifically for `env_val.strip() == "0"`, please add a test where `AELFRICE_HOOK_AUDIT` is set to a non-zero/non-empty value (e.g., "1" or "foo") and verify that auditing stays enabled and config resolution behaves normally. This will lock in the intended env override semantics for future changes.
```suggestion
def test_default_config_is_enabled_with_default_max_bytes(
tmp_path: Path,
) -> None:
cfg = load_hook_audit_config(start=tmp_path, env={})
assert cfg.enabled is True
assert cfg.max_bytes == AUDIT_DEFAULT_MAX_BYTES
def test_env_nonzero_does_not_disable_toml_enabled(tmp_path: Path) -> None:
(tmp_path / ".aelfrice.toml").write_text(
"[hook_audit]\nenabled = true\n",
encoding="utf-8",
)
cfg = load_hook_audit_config(
start=tmp_path,
env={"AELFRICE_HOOK_AUDIT": "1"},
)
assert cfg.enabled is True
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| with open(audit_path, "a", encoding="utf-8") as f: | ||
| f.write(line) | ||
| f.flush() | ||
| os.fsync(f.fileno()) |
There was a problem hiding this comment.
suggestion (performance): Per-record fsync could be expensive under high hook volume.
This gives strong durability but may become a bottleneck on busy or slow filesystems. Consider making the sync policy configurable (e.g., batch/periodic sync or OS buffering only) so operators can tune the durability vs throughput trade-off for their deployment.
Suggested implementation:
with open(audit_path, "a", encoding="utf-8") as f:
f.write(line)
# sync_policy controls durability vs throughput:
# - "always": flush + fsync each record (strong durability, slowest)
# - "flush": flush only, rely on OS to sync (middle ground)
# - "none": rely entirely on OS buffering (fastest)
if sync_policy in ("always", "flush"):
f.flush()
if sync_policy == "always":
os.fsync(f.fileno())To fully implement configurability of the sync behavior, you should also:
- Update the function signature that contains this code (likely a hook audit/write helper) to accept a new parameter, for example:
sync_policy: str = "always"
- Extend the function docstring to document the
sync_policyparameter and the supported values:"always","flush", and"none", and the durability/throughput trade-offs. - Propagate the new parameter from configuration:
- Add a configuration option (CLI flag, config file value, or environment variable) that determines the desired
sync_policy. - Thread that configuration value through to all call sites of this function.
- Add a configuration option (CLI flag, config file value, or environment variable) that determines the desired
- Ensure that any direct calls to this function in tests or other modules are updated to either:
- Rely on the default
sync_policy="always"for current behavior, or - Explicitly pass
"flush"or"none"where stronger throughput is desired.
- Rely on the default
- Optionally, validate
sync_policyinside the function (e.g., raiseValueErroror fall back to"always"if an unknown value is provided) to avoid silent misconfiguration.
| def test_user_prompt_submit_writes_audit_record( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| db = tmp_path / "memory.db" | ||
| _seed_db(db, [_mk("F1", "the kitchen is full of bananas")]) | ||
| _set_db(monkeypatch, db) | ||
| monkeypatch.delenv("AELFRICE_HOOK_AUDIT", raising=False) | ||
| monkeypatch.chdir(tmp_path) | ||
| sin = io.StringIO(_payload("bananas", session_id="sess-abc")) | ||
| sout = io.StringIO() |
There was a problem hiding this comment.
suggestion (testing): Add focused tests for _extract_session_id edge cases (invalid JSON, non-dict payload, missing/empty session_id).
Current integration tests only cover the happy path; they don’t exercise malformed JSON, non-dict roots, or missing/invalid session_id values. Direct unit tests on _extract_session_id for these cases would better guard the audit trail behavior and future payload shape changes.
Suggested implementation:
# ---------------------------------------------------------------------------
# _extract_session_id unit tests
# ---------------------------------------------------------------------------
def test_extract_session_id_invalid_json() -> None:
# Malformed JSON should not raise and should return no session id
assert _extract_session_id("{not-json") is None
def test_extract_session_id_non_dict_root() -> None:
# Non-object JSON roots should be ignored
assert _extract_session_id('["not", "a", "dict"]') is None
assert _extract_session_id('"plain-string"') is None
assert _extract_session_id("123") is None
assert _extract_session_id("null") is None
def test_extract_session_id_missing_key() -> None:
# Valid JSON object without a session_id key should yield no session id
assert _extract_session_id("{}") is None
assert _extract_session_id('{"other_key": "value"}') is None
def test_extract_session_id_empty_or_non_string_value() -> None:
# Empty or non-string session_id values should be treated as absent
assert _extract_session_id('{"session_id": ""}') is None
assert _extract_session_id('{"session_id": null}') is None
assert _extract_session_id('{"session_id": 123}') is None
assert _extract_session_id('{"session_id": {}}') is None
assert _extract_session_id('{"session_id": []}') is None
# ---------------------------------------------------------------------------
# Hook integration: writes a record on UserPromptSubmit fire
# ---------------------------------------------------------------------------
def test_user_prompt_submit_writes_audit_record(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
) -> None:
db = tmp_path / "memory.db"
_seed_db(db, [_mk("F1", "the kitchen is full of bananas")])
_set_db(monkeypatch, db)
monkeypatch.delenv("AELFRICE_HOOK_AUDIT", raising=False)
monkeypatch.chdir(tmp_path)
sin = io.StringIO(_payload("bananas", session_id="sess-abc"))
sout = io.StringIO()
rc = user_prompt_submit(stdin=sin, stdout=sout)- Ensure
_extract_session_idis imported intotests/test_hook_audit.pyfrom the module under test (likely alongsideuser_prompt_submitandload_hook_audit_config). For example:from aelfrice.hook_audit import _extract_session_id
- If the actual contract of
_extract_session_iddiffers (e.g., it raises on malformed JSON or returns""instead ofNone), adjust the assertions accordingly to match the existing implementation.
| def test_default_config_is_enabled_with_default_max_bytes( | ||
| tmp_path: Path, | ||
| ) -> None: | ||
| cfg = load_hook_audit_config(start=tmp_path, env={}) | ||
| assert cfg.enabled is True | ||
| assert cfg.max_bytes == AUDIT_DEFAULT_MAX_BYTES |
There was a problem hiding this comment.
suggestion (testing): Add a test to confirm that AELFRICE_HOOK_AUDIT values other than "0" do not disable auditing.
Current tests cover only the disabling case (AELFRICE_HOOK_AUDIT="0" and TOML). Since the code checks specifically for env_val.strip() == "0", please add a test where AELFRICE_HOOK_AUDIT is set to a non-zero/non-empty value (e.g., "1" or "foo") and verify that auditing stays enabled and config resolution behaves normally. This will lock in the intended env override semantics for future changes.
| def test_default_config_is_enabled_with_default_max_bytes( | |
| tmp_path: Path, | |
| ) -> None: | |
| cfg = load_hook_audit_config(start=tmp_path, env={}) | |
| assert cfg.enabled is True | |
| assert cfg.max_bytes == AUDIT_DEFAULT_MAX_BYTES | |
| def test_default_config_is_enabled_with_default_max_bytes( | |
| tmp_path: Path, | |
| ) -> None: | |
| cfg = load_hook_audit_config(start=tmp_path, env={}) | |
| assert cfg.enabled is True | |
| assert cfg.max_bytes == AUDIT_DEFAULT_MAX_BYTES | |
| def test_env_nonzero_does_not_disable_toml_enabled(tmp_path: Path) -> None: | |
| (tmp_path / ".aelfrice.toml").write_text( | |
| "[hook_audit]\nenabled = true\n", | |
| encoding="utf-8", | |
| ) | |
| cfg = load_hook_audit_config( | |
| start=tmp_path, | |
| env={"AELFRICE_HOOK_AUDIT": "1"}, | |
| ) | |
| assert cfg.enabled is True |
|
[claim:review:Setr:2026-04-29T16:25:13Z] |
|
[release:review:Setr:2026-04-29T16:26:26Z] |
Summary
Adds the per-turn audit log called for in
docs/hook_hardening.md(#280 mitigation 3) — a JSONL sibling of the existinghook_telemetry.jsonlthat records the exact rendered hook block on every fire. Telemetry records counts; audit records what was actually injected.<git-common-dir>/aelfrice/hook_audit.jsonl.user_prompt_submitandsession_start.AELFRICE_HOOK_AUDIT=0env or[hook_audit] enabled = falsein.aelfrice.toml.[hook_audit] max_bytes); single-slot rotation tohook_audit.jsonl.1on rollover.Spec references in this repo:
docs/hook_hardening.md— full design.docs/hook_hardening.md#3-per-turn-audit-log— record schema (matches what this PR implements).Mitigations 1 and 2 from the same spec (framing-tag contract + render-time belief-content escape) already shipped via #292; this PR closes the third and final mitigation.
Test plan
tests/test_hook_audit.pycovering: default config, env disable, TOML disable, max_bytes override, malformed/wrong-typed TOML graceful degradation, UserPromptSubmit + SessionStart write paths, n_locked / session_id / prompt_prefix capping, no-op when disabled, no-op when retrieval is empty, rotation at threshold, single-slot rotation overwriting prior.1, read API + corruption handling, fail-soft on unwriteable path.Summary by Sourcery
Introduce a per-turn hook audit log that records rendered hook blocks for user prompt submissions and session starts, with configurable opt-out and size-based rotation.
New Features:
Enhancements:
Tests: