Harden Audit Persistence and Schema Compatibility - #106

packages/audit/src/heartwood/audit/_log.py:112

• AuditLog.append() only verifies the tail (_verify_tail) and only checks session_id on the last two events. Because the code already materializes all existing events here, a log with an earlier session-id change, sequence gap, or bad hash link can still pass these checks and be appended to (making the corruption harder to detect later). This should fail closed by verifying the full existing chain before deriving the next sequence/hash, and then separately ensuring the existing log's session matches the session_id being appended.

            events = tuple(_audit_event(record) for record in records)
            if events:
                _verify_tail(events)
                if any(event.session_id != session_id for event in events[-2:]):
                    raise AuditIntegrityError("audit append session does not match existing log")

packages/persistence/src/heartwood/persistence/_files.py:226

• LockedJsonlStore.append_derived() reads and JSON-decodes the entire JSONL file (records = self._read_objects_locked()) on every append, even though many callers (e.g., audit appends) only need the current head (count + last record/hash). This makes appending O(n^2) over time and increases lock hold times for large logs, which can become a bottleneck for long-running sessions.

packages/audit/tests/test_checkpoint.py (2)

273-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Verify the bundle with its own public key.

_checkpoint_bundle(tmp_path / "valid") returns the public key for the key pair it created, but the test discards it and reuses public_key from Line 260. The assertion still passes, because _bundle_paths rejects unexpected files before _load_public_key runs. The mismatch makes the intent unclear and hides a later regression if the ordering of checks changes.

♻️ Proposed change

-    bundle, _ = _checkpoint_bundle(tmp_path / "valid")
+    bundle, bundle_public_key = _checkpoint_bundle(tmp_path / "valid")
     (bundle / "unexpected.txt").write_text("not part of the checkpoint", encoding="utf-8")
     with pytest.raises(AuditCheckpointError, match="unexpected files"):
-        verify_audit_checkpoint(bundle=bundle, public_key=public_key)
+        verify_audit_checkpoint(bundle=bundle, public_key=bundle_public_key)

🤖 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 `@packages/audit/tests/test_checkpoint.py` around lines 273 - 276, Update the
test around _checkpoint_bundle to retain the bundle’s generated public key and
pass that key to verify_audit_checkpoint instead of reusing the unrelated
public_key fixture. Keep the unexpected-files assertion unchanged so the test
explicitly verifies the bundle with its own key.

---

47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use one deployment identifier across the checkpoint tests.

This test uses "carina-research" and Line 75 uses "terra-research", while every other test uses "generic-research". Use "generic-research" everywhere. This keeps the public test fixtures neutral and avoids organization-specific names in source control.

Based on learnings: "Attribute Heartwood only to the Schmiedmayer Lab at Stanford University; do not add legacy organizational attributions."

♻️ Proposed change

-        deployment_id="carina-research",
+        deployment_id="generic-research",

🤖 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 `@packages/audit/tests/test_checkpoint.py` at line 47, Update the deployment_id
in the checkpoint test at the visible deployment configuration to use
"generic-research", and ensure the corresponding deployment identifier at the
other referenced checkpoint test location also uses "generic-research" so all
checkpoint fixtures share the neutral identifier.

Source: Learnings

packages/gateway/tests/test_audit_checkpoint.py (1)

20-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Release the gateway even when an assertion fails.

gateway.stop() runs only after the assertions. If any assertion fails, stop() is skipped and the gateway keeps its session writer lock and background resources. Later tests in the same process can then fail for an unrelated reason. The same pattern appears at Line 74 and Line 105. Use a fixture or contextlib.closing so cleanup always runs.

♻️ Proposed fixture

`@pytest.fixture`
def gateway_factory() -> Iterator[Callable[[Path], SessionGateway]]:
    created: list[SessionGateway] = []

    def build(project_root: Path) -> SessionGateway:
        gateway = SessionGateway(
            project=ProjectContext(project_root),
            backend_id="deterministic",
        )
        created.append(gateway)
        return gateway

    yield build
    for gateway in created:
        gateway.stop()

Then replace each direct construction and drop the trailing gateway.stop() calls.

🤖 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 `@packages/gateway/tests/test_audit_checkpoint.py` around lines 20 - 42, Ensure
all gateway instances in the affected tests, including the cases around
test_gateway_creates_and_verifies_checkpoint_outside_project and the tests at
the referenced later locations, are stopped even when assertions fail. Introduce
and use a pytest fixture or contextlib.closing-based cleanup around each
SessionGateway construction, then remove the trailing direct gateway.stop()
calls.

packages/core-adapter/src/heartwood/core_adapter/_state.py (1)

442-444: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Initialize the sequence and audit-hash caches on the empty-session path.

_verified_records returns early when session_dir does not exist. It does not set _next_sequence or _last_audit_hash. next_sequence() then calls replay_events(), finds _next_sequence still None, and raises SessionRecoveryError("session sequence could not be initialized") instead of returning 0. Callers that reach next_sequence() before acquire_writer() creates the directory get a recovery error for an empty session. Setting both caches also clears a stale _last_audit_hash if the directory is removed during the store's lifetime.

♻️ Proposed change

         if not self.session_dir.exists():
             verification = AuditLog(self.audit_path).verify(())
+            self._next_sequence = 0
+            self._last_audit_hash = None
             return (), (), verification

🤖 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 `@packages/core-adapter/src/heartwood/core_adapter/_state.py` around lines 442
- 444, Update the empty-session early-return path in _verified_records to
initialize _next_sequence to 0 and _last_audit_hash to the empty-session audit
hash state before returning. Preserve the existing verification result and
return values, ensuring next_sequence() works before acquire_writer() and stale
audit-hash state is cleared.

packages/cli/src/heartwood/cli/__init__.py (1)

1917-1928: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the audit checkpoint CLI print the gateway-resolved bundle path.

SessionGateway.create_audit_checkpoint resolves the destination through _deployment_owned_path before calling create_audit_checkpoint(), but AuditCheckpointVerification only exposes metadata, not that resolved path. If the CLI needs to print where the bundle was written, get and return/attach the resolved destination from the gateway API rather than recomputing output.expanduser().resolve() in _handle_audit_checkpoint.

🤖 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 `@packages/cli/src/heartwood/cli/__init__.py` around lines 1917 - 1928, The
audit checkpoint CLI currently recomputes the output path instead of reporting
the gateway-resolved destination. Update SessionGateway.create_audit_checkpoint
and the AuditCheckpointVerification result flow to return or attach the resolved
bundle path, then have _handle_audit_checkpoint print that value in the “Audit
checkpoint” line rather than calling output.expanduser().resolve().

Source: Coding guidelines

packages/persistence/src/heartwood/persistence/_migrations.py (1)

199-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider freezing the shared registry after construction.

PERSISTENCE_MIGRATIONS is a process-wide singleton, and register_kind and register stay public on it. Any imported module can change the persisted-schema contract for every consumer at import time. No current caller does this. A freeze flag keeps the deterministic contract enforced while MigrationRegistry stays mutable for the test-local registries in packages/persistence/tests/test_migrations.py.

♻️ Proposed freeze support

     def __init__(self) -> None:
         self._current: dict[str, str] = {}
         self._steps: dict[tuple[str, str], _MigrationStep] = {}
+        self._frozen = False
+
+    def freeze(self) -> None:
+        """Reject every further registration on this registry."""
+        self._frozen = True
+
+    def _require_mutable(self) -> None:
+        if self._frozen:
+            raise MigrationError("migration registry is frozen")

Call _require_mutable() at the start of register_kind and register, then freeze the singleton:

-    return registry
+    registry.freeze()
+    return registry

🤖 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 `@packages/persistence/src/heartwood/persistence/_migrations.py` around lines
199 - 247, Make MigrationRegistry support freezing by adding a frozen-state
guard and calling _require_mutable() at the start of register_kind and register.
After _build_registry() completes, freeze PERSISTENCE_MIGRATIONS so the
process-wide registry cannot be modified, while newly constructed test-local
registries remain mutable.

packages/schemas/tests/test_schema_records.py (1)

164-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend the negative coverage to the other new validators.

The test covers only the terminal-hash and event-count rule. Two other new rules stay uncovered: the retention-ordering rule in AuditCheckpointStatement._validate_terminal_hash and the canonical Ed25519 signature rule in AuditCheckpoint._validate_signature. Both rules protect checkpoint integrity, so add explicit rejection cases.

💚 Proposed additional assertions

     invalid = statement.model_dump(mode="json")
     invalid["audit_event_count"] = 0
     with pytest.raises(ValidationError):
         AuditCheckpointStatement.model_validate(invalid)
+
+    early_retention = statement.model_dump(mode="json")
+    early_retention["retention"]["retain_until"] = "2026-08-01"
+    with pytest.raises(ValidationError):
+        AuditCheckpointStatement.model_validate(early_retention)
+
+    for bad_signature in ("not-base64!", base64.b64encode(bytes(32)).decode("ascii")):
+        with pytest.raises(ValidationError):
+            AuditCheckpoint(
+                statement=statement,
+                signing_key_id=f"sha256:{'c' * 64}",
+                signature=bad_signature,
+            )

As per coding guidelines: "Add or update tests when changing detector logic, policy decisions, adapter behavior, skill validation, audit records, attestation export, CLI output, notebook view models, or web-UI view models."

🤖 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 `@packages/schemas/tests/test_schema_records.py` around lines 164 - 169, Extend
the negative test around AuditCheckpointStatement.model_validate and
AuditCheckpoint validation to cover both missing rules: construct a statement
with invalid retention ordering and assert ValidationError, then construct a
checkpoint with a non-canonical Ed25519 signature and assert ValidationError.
Keep the existing terminal-hash and audit_event_count rejection coverage
unchanged.

Source: Coding guidelines

packages/persistence/tests/test_files.py (2)

162-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert a distinct message for each journal failure mode.

The parameterization covers four different rejection paths in _recover_locked. The assertion at Line 194 accepts any AppendRecoveryError. A regression that routes every case through one check still passes all four parameters. Add the expected message fragment to each parameter.

♻️ Proposed fix to pin each failure to its own message

-@pytest.mark.parametrize("failure", ["malformed", "invalid", "digest", "suffix"])
+@pytest.mark.parametrize(
+    ("failure", "message"),
+    [
+        ("malformed", "journal is malformed"),
+        ("invalid", "journal is invalid"),
+        ("digest", "journal digest does not match"),
+        ("suffix", "does not match its recovery journal"),
+    ],
+)
 def test_jsonl_recovery_rejects_invalid_journal_or_target(
     tmp_path: Path,
     monkeypatch: pytest.MonkeyPatch,
     failure: str,
+    message: str,
 ) -> None:

-    with pytest.raises(AppendRecoveryError):
+    with pytest.raises(AppendRecoveryError, match=message):
         store.recover()

🤖 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 `@packages/persistence/tests/test_files.py` around lines 162 - 195, Update
test_jsonl_recovery_rejects_invalid_journal_or_target to parameterize each
failure mode with its expected AppendRecoveryError message fragment, then assert
that fragment via pytest.raises(match=...) when calling store.recover(). Keep
the existing setup and four failure cases unchanged.

---

269-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the lock timeout branch.

This test covers the OSError branch of native_file_lock. The FileLockTimeout branch at _files.py Line 193 produces a different message and stays uncovered. native_file_lock exposes a timeout parameter, so callers can reach that branch.

♻️ Proposed additional test

def test_native_lock_timeout_is_explicit(tmp_path: Path) -> None:
    lock_path = tmp_path / "records.jsonl.lock"
    with _files.native_file_lock(lock_path):
        holder = FileLock(str(lock_path), timeout=0, is_singleton=False)
        del holder
        with pytest.raises(NativeLockUnavailableError, match="timed out"):
            _run_in_child_process_expecting_timeout(lock_path)

The lock is reentrant inside one process, so acquire the second lock from a forked child, as test_jsonl_store_serializes_process_writers does.

🤖 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 `@packages/persistence/tests/test_files.py` around lines 269 - 276, Add a test
alongside test_native_lock_failure_is_explicit that exercises native_file_lock’s
timeout path using a forked child process, since same-process locking is
reentrant. Hold the lock in the parent, attempt acquisition from the child with
timeout=0, and assert NativeLockUnavailableError with the “timed out” message
while preserving the existing OSError coverage.

packages/persistence/src/heartwood/persistence/_files.py (1)

116-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Sync the parent directory after append_private_bytes creates the file.

append_private_bytes passes O_CREAT and syncs the file data. It does not sync the parent directory. If the append creates the file, the new directory entry can be lost after a crash even though os.fsync(descriptor) succeeded. write_private_bytes_atomic and unlink_durable both call fsync_directory, so this path is the only durable operation that omits it.

LockedJsonlStore masks the gap because the journal write syncs the directory first. Direct callers of append_private_bytes do not get that protection.

♻️ Proposed fix to sync the directory on creation

 def append_private_bytes(path: Path, content: bytes) -> None:
     """Durably append bytes to an owner-only regular file."""
     _prepare_parent(path)
     _reject_non_regular_target(path)
+    created = not path.exists()
     descriptor = _open_regular(
         path,
         os.O_APPEND | os.O_CREAT | os.O_WRONLY,
         mode=0o600,
     )
     try:
         os.fchmod(descriptor, 0o600)
         _write_all(descriptor, content, operation="append")
         os.fsync(descriptor)
     finally:
         os.close(descriptor)
+    if created:
+        fsync_directory(path.parent)

🤖 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 `@packages/persistence/src/heartwood/persistence/_files.py` around lines 116 -
130, Update append_private_bytes to sync the parent directory after the file
descriptor is durably written and closed, matching the durability behavior of
write_private_bytes_atomic and unlink_durable; ensure the directory sync occurs
for newly created entries without leaving the descriptor open.

packages/gateway/tests/test_openhands_persistence.py (2)

175-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a schema_version case to the marker rejection parameters.

_validate_marker rejects four conditions: an unexpected key set, an unsupported schema_version, an unsupported content_policy, and an unsupported adopted_from. This test covers three of them. An unsupported schema_version on an otherwise current marker reaches _migrated_marker first and must produce "schema is unsupported". Add that parameter to pin the behavior.

💚 Proposed change

         ({"unexpected": "field"}, "marker is malformed"),
+        ({"schema_version": "heartwood.openhands-state.v99"}, "schema is unsupported"),
         ({"content_policy": "unsupported"}, "content policy is unsupported"),
         ({"adopted_from": "unknown"}, "origin is unsupported"),

As per coding guidelines: "Add or update tests when changing detector logic, policy decisions, adapter behavior, skill validation, audit records, attestation export, CLI output, notebook view models, or web-UI view models."

🤖 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 `@packages/gateway/tests/test_openhands_persistence.py` around lines 175 - 196,
Add an unsupported schema_version case to the marker_update/message parameters
in test_store_rejects_incompatible_current_markers, using an otherwise current
marker and expecting “schema is unsupported.” Preserve the existing cases and
parameterized test structure.

Source: Coding guidelines

---

133-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the specific error message for each failure mode.

pytest.raises(OpenHandsPersistenceError) accepts any error from this module. _ensure_compatible_state raises the same exception type for symbolic links, malformed events, filename violations, and index gaps. A regression that moves one failure to a different check still passes this test. Add a match value per parameter, as the other tests in this file already do.

💚 Proposed change

-@pytest.mark.parametrize("failure", ["gap", "malformed", "symlink"])
-def test_markerless_store_rejects_incompatible_state(tmp_path: Path, failure: str) -> None:
+@pytest.mark.parametrize(
+    ("failure", "message"),
+    [
+        ("gap", "event sequence contains a gap"),
+        ("malformed", "event state is malformed"),
+        ("symlink", "symbolic link or special file"),
+    ],
+)
+def test_markerless_store_rejects_incompatible_state(
+    tmp_path: Path,
+    failure: str,
+    message: str,
+) -> None:
@@
-    with pytest.raises(OpenHandsPersistenceError):
+    with pytest.raises(OpenHandsPersistenceError, match=message):
         ContentMinimizedLocalFileStore(str(root))

🤖 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 `@packages/gateway/tests/test_openhands_persistence.py` around lines 133 - 154,
Update test_markerless_store_rejects_incompatible_state to supply a
failure-specific match pattern to pytest.raises for each parameterized case,
covering the symbolic-link, malformed-event, and index-gap error messages.
Follow the existing message-matching style used by other tests in this file
while preserving the current setup and exception type assertion.

packages/gateway/tests/test_project.py (2)

167-176: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Assert that the error message does not expose the persisted payload.

The fixture places "private":"value" in the unsupported marker. _migrate_state_marker formats the underlying error into f"unable to migrate .heartwood/state.json: {error}". The migration registry states that it does not expose payload content in errors, but this test does not verify that contract at the ProjectStateError boundary. test_store_rejects_malformed_event_without_exposing_content in packages/gateway/tests/test_openhands_persistence.py already uses this pattern.

💚 Proposed change

-    with pytest.raises(ProjectStateError):
+    with pytest.raises(ProjectStateError) as captured:
         project.initialize()

+    assert "value" not in str(captured.value)
     assert project.state_path.read_text(encoding="utf-8") == unsupported

🤖 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 `@packages/gateway/tests/test_project.py` around lines 167 - 176, Add an
assertion to test_project_context_preserves_state_when_migration_fails that the
raised ProjectStateError message does not contain the persisted payload value
"private" or "value", while retaining the existing state-preservation assertion.

---

68-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document why the unsynchronized first_thread guard is safe.

pause_first_write reads and appends to first_thread without synchronization. The test is correct only because the second thread blocks on state_lock_path before it reaches the .gitignore write, so only one thread ever enters this branch. If the lock ordering in initialize changes, both threads can pause and the test deadlocks until the 5-second waits expire. Add a short comment that records this dependency.

🤖 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 `@packages/gateway/tests/test_project.py` around lines 68 - 73, Add a short
comment immediately before the unsynchronized first_thread check in
pause_first_write, documenting that initialize’s state_lock_path ordering
ensures only one thread reaches the .gitignore branch and that changing this
ordering could cause both threads to wait and deadlock.

packages/gateway/src/heartwood/gateway/_openhands_persistence.py (1)

157-171: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Translate filesystem errors during the scan into OpenHandsPersistenceError.

path.lstat() and path.chmod() raise OSError directly. _ensure_compatible_state does not catch OSError for this call path, so a removed entry or a read-only mount surfaces a raw OSError from the constructor. Every other failure in this module surfaces as OpenHandsPersistenceError. Callers that handle only OpenHandsPersistenceError then fail unexpectedly.

🤖 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 `@packages/gateway/src/heartwood/gateway/_openhands_persistence.py` around
lines 157 - 171, Update the filesystem scan in _ensure_compatible_state to catch
OSError from path.lstat() and path.chmod() and re-raise it as
OpenHandsPersistenceError, preserving the original exception as context. Ensure
removed entries and read-only filesystem failures follow the module’s existing
OpenHandsPersistenceError contract.

packages/gateway/tests/test_project_config.py (1)

85-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify the fixture path and consider sharing the resolver.

parents[2] is already the packages directory, so the extra parents[3] / "packages" hop is redundant and more sensitive to a directory move. packages/core-adapter/tests/test_session_service.py resolves the same fixture directory through its own _COMPATIBILITY_FIXTURES constant. Three packages now duplicate this traversal. A single shared constant or a pytest fixture would keep the compatibility fixture location in one place.

♻️ Proposed refactor

-    fixture = (
-        Path(__file__).resolve().parents[3]
-        / "packages"
-        / "persistence"
-        / "tests"
-        / "fixtures"
-        / "project-config-v1.json"
-    )
+    fixture = (
+        Path(__file__).resolve().parents[2]
+        / "persistence"
+        / "tests"
+        / "fixtures"
+        / "project-config-v1.json"
+    )

🤖 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 `@packages/gateway/tests/test_project_config.py` around lines 85 - 99, Update
test_checked_in_project_config_compatibility_fixture_loads to resolve the
fixture from Path(__file__).resolve().parents[2] / "persistence" / "tests" /
"fixtures" / "project-config-v1.json", removing the redundant packages hop.
Prefer reusing or defining a shared compatibility-fixture resolver, such as the
existing _COMPATIBILITY_FIXTURES convention, so the fixture location is
centralized across package tests.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/persistence/src/heartwood/persistence/_files.py:226

• LockedJsonlStore.append_derived reads and JSON-parses the entire JSONL file into a tuple on every append. For long-lived audit/session logs this becomes O(n) memory per append (and tends toward O(n^2) total work across a session), which can become a practical bottleneck.
  packages/persistence/src/heartwood/persistence/_files.py:232
• append_derived computes prefix_sha256 by re-reading the file from disk (_sha256_prefix) after the file has already been read/parsed in _read_objects_locked. That adds another full file scan per append; for large logs this doubles IO and CPU for every write.
  packages/audit/src/heartwood/audit/_log.py:111
• AuditLog.append() builds events = tuple(_audit_event(record) for record in records) for the entire history on every append, even though only the current length and tail hashes are needed to derive the next event. This adds a full pydantic validation + migration pass per append and can significantly slow down large audit logs.

        def build(records: tuple[dict[str, object], ...]) -> dict[str, object]:
            events = tuple(_audit_event(record) for record in records)
            if events:
                _verify_tail(events)
                if any(event.session_id != session_id for event in events[-2:]):

packages/audit/src/heartwood/audit/_log.py:112

• AuditLog.append only checks the last two existing records for session_id mismatches. If older records contain a different session_id (e.g., due to corruption or partial manual edits), append could succeed and further entrench an invalid log until a later full verification fails.

Since events is already fully materialized here, validate the session_id across the entire existing log before appending.

        def build(records: tuple[dict[str, object], ...]) -> dict[str, object]:
            events = tuple(_audit_event(record) for record in records)
            if events:
                _verify_tail(events)
                if any(event.session_id != session_id for event in events[-2:]):
                    raise AuditIntegrityError("audit append session does not match existing log")

packages/skills/src/heartwood/skills/_verification.py:147

• load_skill_manifest maps filesystem safety failures (e.g., symlinks, non-regular files, invalid UTF-8) into the generic message "metadata.json is invalid JSON" because DurableFileError/OSError are caught alongside JSONDecodeError. That makes the failure mode misleading and can hinder remediation (the file may be valid JSON but an unsafe path).

Consider splitting the exception handling so file safety issues report a "must be a regular UTF-8 file"-style message, and reserve "invalid JSON" for JSONDecodeError.

packages/audit/src/heartwood/audit/_log.py:121

• AuditLog.append() currently re-validates (via _audit_event) every existing record on each append (events = tuple(...)). Since LockedJsonlStore.append_derived() already reads/parses the whole file, this adds an extra O(n) Pydantic validation pass per append, leading to O(n²) growth in CPU cost as the audit log grows. You only need the last one or two records to (a) ensure the tail is well-formed and (b) derive the next sequence and previous hash.

Consider validating just the tail records and deriving sequence from len(records), while keeping the same failure modes for an invalid tail or session mismatch.

        def build(records: tuple[dict[str, object], ...]) -> dict[str, object]:
            events = tuple(_audit_event(record) for record in records)
            if events:
                _verify_tail(events)
                if any(event.session_id != session_id for event in events[-2:]):

packages/schemas/src/heartwood/schemas/_records.py:207

• base64.b64decode(..., validate=True) can raise ValueError (e.g., non-ASCII input). Currently only binascii.Error is caught, which can surface a less helpful/less consistent validation message than the intended “signature must be canonical Base64”. Catch ValueError too so invalid inputs always fail with the same schema error message.

packages/audit/src/heartwood/audit/_signer.py (1)

181-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use a dedicated bound for the token length.

validate_checkpoint_signer_token bounds the token with _MAXIMUM_SIGNER_RESPONSE_BYTES. That constant describes the HTTP response envelope, not a credential length. A separate constant makes the limit explicit and prevents an accidental coupling if the response bound changes.

♻️ Proposed refactor

 _MAXIMUM_SIGNER_RESPONSE_BYTES = 64 * 1024
+_MAXIMUM_SIGNER_TOKEN_CHARACTERS = 4096

     if (
-        len(value) > _MAXIMUM_SIGNER_RESPONSE_BYTES
+        len(value) > _MAXIMUM_SIGNER_TOKEN_CHARACTERS
         or re.fullmatch(r"[A-Za-z0-9\-._~+/]+=*", value) is None
     ):

🤖 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 `@packages/audit/src/heartwood/audit/_signer.py` around lines 181 - 188, Update
validate_checkpoint_signer_token to compare token length against a dedicated
signer-token maximum constant rather than _MAXIMUM_SIGNER_RESPONSE_BYTES. Define
the new bound alongside the existing limits with an explicit token-specific
name, while preserving the current character validation and error behavior.

packages/audit/tests/test_checkpoint.py (1)

215-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Split the combined failure-mode test.

test_checkpoint_rejects_invalid_input_identity_and_key_material asserts seven unrelated failure modes: session mismatch, missing key, EC private key, wrong trusted key, corrupted audit input, invalid PEM, and a writable public key. A failure in an early assertion hides the later ones. Split the signer key-material cases into a parametrized test and keep the checkpoint input cases separate.

🤖 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 `@packages/audit/tests/test_checkpoint.py` around lines 215 - 263, Split
test_checkpoint_rejects_invalid_input_identity_and_key_material into focused
tests: keep session mismatch and corrupted audit input checkpoint cases
together, and move signer key-material failures into a parametrized test
covering missing, EC, invalid PEM, and writable-key scenarios. Keep the wrong
trusted public-key verification case separate, preserving each existing
exception type and match assertion.

packages/audit/src/heartwood/audit/_signer_registry.py (1)

300-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Make the endpoint and timeout validation explicit.

Lines 302-306 construct a RemoteCheckpointSigner and discard it. The construction is used only for its validation side effects, and it also allocates a default _UrlLibSignerTransport. Extract the endpoint and timeout checks into a public validation helper in _signer.py, then call that helper here. This also removes the redundant http:// check on line 300, because _validate_signer_endpoint already rejects cleartext endpoints when allow_insecure_loopback is 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 `@packages/audit/src/heartwood/audit/_signer_registry.py` around lines 300 -
306, The signer registry should validate configuration without constructing and
discarding a RemoteCheckpointSigner. Add a public validation helper in
_signer.py that checks the endpoint and timeout using the same
allow_insecure_loopback rules, then call it from this registry path with the
profile values; remove the redundant production http:// check and the unused
RemoteCheckpointSigner construction.

packages/gateway/tests/test_project_config.py (1)

230-230: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert that clearing removes the audit table from the file.

Line 230 checks the in-memory value after clearing. It does not check the persisted TOML. _config_mapping omits the audit table when signer_profile is None, and content minimization is a stated objective of this change. Add a file-level assertion to lock that behavior.

💚 Proposed addition

     assert store.select_checkpoint_signer(None).audit_settings == AuditCheckpointSettings()
+    cleared = tomllib.loads(project.config_path.read_text(encoding="utf-8"))
+    assert "audit" not in cleared

🤖 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 `@packages/gateway/tests/test_project_config.py` at line 230, Extend the test
around store.select_checkpoint_signer(None) to read the persisted TOML
configuration and assert that the audit table is absent after clearing. Keep the
existing in-memory AuditCheckpointSettings assertion and verify the file-level
result reflects _config_mapping’s omission when signer_profile is None.

packages/cli/src/heartwood/cli/__init__.py (4)

2073-2084: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Pass the resolved root to the initializer.

_require_deployment_owned_cli_path validates the resolved path and returns it, but the return value is discarded. initialize_local_checkpoint_signer then receives the unresolved root. If root is relative or contains symlinked parents, the validated path and the created path can differ.

🛡️ Proposed fix

-    _require_deployment_owned_cli_path(project, root, label="local signer directory")
+    resolved_root = _require_deployment_owned_cli_path(
+        project, root, label="local signer directory"
+    )
     setup = initialize_local_checkpoint_signer(
-        directory=root,
+        directory=resolved_root,

🤖 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 `@packages/cli/src/heartwood/cli/__init__.py` around lines 2073 - 2084, Update
the local signer setup around _require_deployment_owned_cli_path so its
resolved-path return value is assigned back to root, then pass that resolved
root to initialize_local_checkpoint_signer. Preserve the existing directory
selection and validation behavior.

---

2186-2192: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Reuse the gateway boundary check instead of duplicating it.

SessionGateway._deployment_owned_path and SessionGateway._resolve_audit_path in packages/gateway/src/heartwood/gateway/_gateway.py implement the same rule. The gateway version also rejects a symbolic link before resolving. This copy does not, so a symlinked registry, key, or token path passes the CLI check. Extract one shared helper and use it in both places.

🤖 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 `@packages/cli/src/heartwood/cli/__init__.py` around lines 2186 - 2192, Replace
the duplicated project-boundary logic in the CLI path-resolution helper with a
shared helper extracted from the gateway implementations, and use that helper
from both SessionGateway._deployment_owned_path and
SessionGateway._resolve_audit_path. Preserve the gateway behavior of rejecting
symlinks before resolution, while continuing to reject paths inside or equal to
project.root.

---

2119-2167: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Move the signer resolution policy into the audit package.

This block decides signer policy: it requires the development mode, matches the endpoint, resolves the default private-key path, validates the key fingerprint, and requires an authorization token. These are signer-domain rules, not presentation. initialize_local_checkpoint_signer already lives in packages/audit/src/heartwood/audit/_local_signer_setup.py.

Add a resolver in that package that returns the signer, the bind host, and the bind port. Keep the CLI limited to argument handling and printing.

As per coding guidelines: "Keep business rules, labels, setup choices, readiness, and persisted state in the gateway or their owning typed package; CLI, browser, and notebook code should only adapt shared projections to their interaction style."

🤖 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 `@packages/cli/src/heartwood/cli/__init__.py` around lines 2119 - 2167, Move
the signer-policy logic from the CLI block into a resolver alongside
initialize_local_checkpoint_signer in _local_signer_setup.py. Have the resolver
enforce development mode, endpoint matching, private-key defaulting and
ownership, fingerprint validation, and authorization-token availability, then
return the signer, bind host, and bind port; leave
initialize_local_checkpoint_signer or add a focused companion API as
appropriate. Reduce the CLI flow to argument handling, invoking the resolver,
and printing.

Source: Coding guidelines

---

560-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the exit code for project-boundary rejections and document the CLI exit codes.

_require_deployment_owned_cli_path raises CheckpointSignerError for a signer path inside the project, while the gateway raises ProjectStateError for the same condition. The CLI catches CheckpointSignerError as 65 and ProjectStateError as 64, so serve-local --trusted-public-key can use a different exit code than checkpoint verify --trusted-public-key for the same boundary failure. Use one exit code, or document the difference. Also, documentation/reference/cli.md has no exit-code guidance, so add it for user-visible failure codes.

🤖 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 `@packages/cli/src/heartwood/cli/__init__.py` around lines 560 - 576, Align the
CLI exit-code handling for project-boundary failures raised by
_require_deployment_owned_cli_path and the gateway, so CheckpointSignerError and
ProjectStateError produce the same user-visible code for equivalent conditions.
Update the relevant exception mapping in the CLI handler while preserving
unrelated audit and session codes, then document the resulting exit-code
guidance in documentation/reference/cli.md.

packages/gateway/src/heartwood/gateway/_gateway.py (1)

763-782: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

The signer registry cache never expires.

The registry is validated once and cached for the lifetime of the gateway. A long-lived gateway process keeps a stale trust store after an operator rotates a key or removes a profile from the registry file. Consider revalidating on a file modification time change, or document that operators must restart the gateway after a registry change.

🤖 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 `@packages/gateway/src/heartwood/gateway/_gateway.py` around lines 763 - 782,
The _checkpoint_signer_registry method must not retain a stale registry
indefinitely. Add file-change-aware invalidation or revalidation for the
discovered registry source so updates to keys and profiles are picked up, while
preserving the existing override and deployment-ownership validation behavior;
alternatively, explicitly document the required gateway restart behavior if
expiration is intentionally unsupported.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/audit/src/heartwood/audit/_log.py:112

• AuditLog.append() only calls _verify_tail(events) before deriving the next record. This means corrupted earlier records (e.g., a sequence gap or broken previous_event_hash before the tail) won’t be detected at append time, and a new record can be appended onto an already-invalid chain. Since this method is the write boundary for the log, it should verify the full chain (sequence + link + hash) under the lock before appending.

            if events:
                _verify_tail(events)
                if any(event.session_id != session_id for event in events):
                    raise AuditIntegrityError("audit append session does not match existing log")

packages/audit/tests/test_signer_registry.py (1)

242-256: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Set an explicit mode on the generated public key file.

public_path.write_bytes at Lines 249-254 uses the default mode (0o666 masked by the process umask). Every other file write in this test explicitly calls chmod(0o600) afterward (Lines 128, 133, 141, 216, 277, 292), but this one does not. If the ambient umask permits it, load_checkpoint_signer_registry can fail non-deterministically because _key_metadata in packages/audit/src/heartwood/audit/_signer.py rejects a public key writable by other users. This was flagged in a prior review round and the code is unchanged.

🐛 Proposed fix

     public_path.write_bytes(
         public_key.public_bytes(
             encoding=serialization.Encoding.PEM,
             format=serialization.PublicFormat.SubjectPublicKeyInfo,
         )
     )
+    public_path.chmod(0o644)
     registry_path = operator / "checkpoint-signers.toml"

🤖 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 `@packages/audit/tests/test_signer_registry.py` around lines 242 - 256, Set the
generated public key file permissions explicitly after public_path.write_bytes
in
test_production_registry_supports_p256_without_project_or_private_key_material,
using the same restrictive mode as the other test fixture files so
load_checkpoint_signer_registry behaves deterministically regardless of the
process umask.