Skip to content

The safety-audit log is built and tested but never attached, so no rejection is ever recorded #236

Description

@vladimirrott

SafetyAuditLog is built, documented and tested, and nothing in production ever attaches it, so every rejection it exists to record is discarded.

The field defaults to None (crates/sysknife-brain/src/planner.rs:624):

audit_log: None,

The only way to set it is the builder at :635:

pub fn with_audit_log(mut self, log: SafetyAuditLog) -> Self {
    self.audit_log = Some(log);

and its only callers are tests:

$ grep -rn 'with_audit_log' --include=*.rs crates apps
crates/sysknife-brain/src/planner.rs:635:    pub fn with_audit_log(...)
crates/sysknife-brain/tests/planner.rs:714
crates/sysknife-brain/tests/planner.rs:2223
crates/sysknife-brain/tests/planner.rs:2257

All three write sites are guarded on the field (planner.rs:1199, :1247, :1414):

if let Some(audit) = self.audit_log.clone() {

So in production audit_log is always None and log_rejection_async never runs.

Why it matters

The safety-audit log is the record of what the planner refused: a sensitive intent caught by contains_sensitive, a plan rejected by a fence, an unknown action name. Those are precisely the events an operator would want after the fact, and precisely the ones nothing writes. The feature reads as present from the code and from its tests, and a reader has to trace the builder to discover it never runs.

Worth stating plainly for anyone assessing severity: nothing is logged that should not be, and no gate is weakened. A refusal still refuses. What is missing is the record of it.

Scope

Two directions, and this needs a maintainer call before code:

  • Wire it up. Attach a SafetyAuditLog where the CLI builds the planner. SafetyAuditLog::default_path already exists (crates/sysknife-brain/src/audit.rs:100) and is itself called only from its own test, so it was written for exactly this. Decide whether the log is on by default or opt-in, and whether the path belongs in config.
  • Or delete it, with default_path, the builder and the three guarded call sites, and stop implying a capability that does not run.

Wiring it up is the better outcome if the record is wanted, and deleting it is honest if it is not. Either is better than the current state.

A caution for whoever takes this

If you wire it up, check what reaches disk. contains_sensitive is a substring denylist, so an intent can be refused for one reason while carrying a credential shape the denylist does not name, and AuditEntry.intent stores the intent verbatim. Turning on a log that records refused intents is worth pairing with a decision about redacting them.

Difficulty

medium. The wiring is small; deciding the default, the path and the redaction policy is the part worth discussing here first.

Getting started

CONTRIBUTING.md has the build and test commands, and docs/architecture.md covers the trust boundary. No CLA and no copyright waiver. The project is MIT.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingclaimedSomeone has said in the thread that they are working on thishelp wantedExtra attention is neededmediumDifficulty: needs familiarity with one subsystem

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions