fix(brain): attach the default safety audit log - #402
Conversation
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 0b5bc9ea. The fix is right and the hole was real. Two mechanical things are keeping rust red.
You found a case where the module doc says "'All' is now literal" and production disagreed with it. I checked the claim rather than taking it from your body:
$ git grep -n 'with_audit_log' origin/main -- crates apps tests
origin/main:crates/sysknife-brain/src/planner.rs:635: pub fn with_audit_log(mut self, log: SafetyAuditLog) -> Self {
origin/main:crates/sysknife-brain/tests/planner.rs:714: .with_audit_log(audit_log);
origin/main:crates/sysknife-brain/tests/planner.rs:2223: .with_audit_log(SafetyAuditLog::new(&log_path));
origin/main:crates/sysknife-brain/tests/planner.rs:2257: .with_audit_log(SafetyAuditLog::new(&log_path));
The setter and three test call sites. No production caller at all, so self.audit_log was None on every real planner, the if let Some(audit) at planner.rs:1199 and :1247 never fired, and a safety-fence rejection produced neither a JSONL line nor a journald record. from_config is the right place to fix it: the CLI at runner.rs, the MCP server at mcp_server.rs and the Tauri shell all construct through it, and new stays clean for embedded callers and tests.
Your test is not decorative. maintainer screen 402 returns DO NOT EXECUTE, so this ran in docker.io/library/rust:1-slim under rootless podman, --network=none, isolated target directory:
$ out=$(bash /home/entropia/.local/state/sysknife-maint/ptcargo.sh iso402 p402 bash -c "$(cat /tmp/sk402proof.sh)" 2>&1)
### M0. unmutated
running 1 test
test planner::tests::configured_planner_attaches_audit_log ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 254 filtered out; finished in 0.12s
exit=0
### M1. revert the production hunk: drop .with_audit_log(...) from from_config
target line: 868
let mut planner =
Self::new(provider, state_client, config.max_turns)
;
0 — production call removed
running 1 test
test planner::tests::configured_planner_attaches_audit_log ... FAILED
thread 'planner::tests::configured_planner_attaches_audit_log' (5236) panicked at crates/sysknife-brain/src/planner.rs:1534:9:
from_config must attach the safety audit log
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 254 filtered out; finished in 0.09s
exit=101
Remove the fix and the test names it. running 1 test both times, so the filter selected something.
Blocking, both mechanical
cargo fmt --all --check fails, and it is the first step of the rust job, so nothing after it ran and the CI log never reached your change:
$ out=$(bash /home/entropia/.local/state/sysknife-maint/ptcargo.sh t402 p402 cargo fmt --all --check 2>&1); rc=$?; printf '%s\n' "$out" | grep -v '^time=' | head -30; echo "=== cargo fmt --all --check exit=$rc ==="
Diff in /repo/crates/sysknife-brain/src/planner.rs:863:
None => provider,
};
- let mut planner =
- Self::new(provider, state_client, config.max_turns)
- .with_audit_log(SafetyAuditLog::new(SafetyAuditLog::default_path()));
+ let mut planner = Self::new(provider, state_client, config.max_turns)
+ .with_audit_log(SafetyAuditLog::new(SafetyAuditLog::default_path()));
// `from_config` is the production construction path used by the CLI,
Diff in /repo/crates/sysknife-brain/src/planner.rs:1536:
"from_config must attach the safety audit log"
);
}
-
#[test]
fn into_authorized_replaces_every_step_risk() {
=== cargo fmt --all --check exit=1 ===
Two hunks: the statement wants to break after Self::new(...) rather than after =, and there is a double blank line before into_authorized_replaces_every_step_risk. cargo fmt --all writes both.
Second, the published test count. Your PR adds one test, tests/evidence/workspace-tests.json still reads 1845, and scripts/test_baseline.sh requires exact equality. Fixing fmt alone leaves rust red for this. CONTRIBUTING.md:130 says to leave the artifact alone when you cannot build the GUI workspace members; if you can run cargo nextest run --workspace --locked locally, run UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh and move the three prose figures in README.md, docs/introduction.md and docs/distro-support.md in the same commit. If you cannot, say which tool blocked you and leave all four files; it is mine to move then.
Order note so the number does not surprise you: #385 also needs 1846 and #384 needs 1850. Whichever lands first shifts the baseline under the other two. My queue problem, not yours.
Not blocking, and worth a comment at least
Your change makes the /tmp fallback production-reachable for the first time. audit.rs:
let home = std::env::var("HOME").unwrap_or_else(|_| {
eprintln!(
"[sysknife-brain] audit: HOME is unset; audit log will be written to /tmp \
— this path is world-writable and insecure for production use"
);
"/tmp".into()
});With HOME and XDG_DATA_HOME both unset, default_path() returns /tmp/.local/share/sysknife/safety-audit.jsonl, and append_entry does create_dir_all then OpenOptions::create(true).append(true).open(), both of which follow symlinks. Before your PR nothing in production constructed a SafetyAuditLog, so that branch was test-only. Now every from_config reaches it, and a local user who wins the /tmp/.local tree can redirect or forge safety-fence records.
Reachability is narrow: the interactive CLI, MCP hosts and systemd User= units all set HOME. So this is defence in depth rather than a live hole, and I am not blocking on it. The repo's own direction is to refuse rather than degrade (see 7eca3f3, fail closed when a peer cannot be pinned), so having default_path() return an error and from_config refuse to attach a log it cannot place safely would match. If you would rather keep this PR to one concern, say so and I will file it separately with your name on the finding.
Your claim on #236
The claimed label is on and the reservation stands. The assignment did not take:
$ maintainer assign 236 vsolano9
maintainer: #236 still has no assignees after asking for vladimirrott.
GitHub drops an assignee who is not a collaborator and has
not posted on that issue.
GitHub returns 200 and silently drops an assignee who is not a collaborator and has not commented on the issue itself. So assignee:@me will not show you #236 even though it is reserved for you. Posting once on the issue thread makes the assignment stick, if having it on your dashboard is useful; nothing depends on it and the label is what other contributors read.
Next
Push the fmt fix and either the baseline or the note saying you could not measure it, and I will approve and take it from there. The rest of the review is already done, so there is nothing else waiting on you.
The open-issue pool is thin tonight and the four remaining ones are spoken for. You went after a log that existed and was never attached, which is a specific and useful thing to be good at, and the audit chain has more of it. I will bring you the next one directly rather than making you watch the tracker.
Somebody fixing safety-fence audit logging is running this somewhere real, or plans to. If that is a box of your own, sysknife "show me failed units and recent auth failures" plans and previews and executes nothing, and it exercises the path you repaired. No obligation.
cargo fmt --all --check is the first rust CI step and failed on the from_config statement wrap plus a double blank line before the next test.
|
Pushed Could not measure the workspace baseline. On the Local checks on this push: |
CI ran cargo nextest --workspace --locked on this branch: 1846 passed. scripts/test_baseline.sh then failed because the published figure was still 1845. Move the artifact and the three prose claims together.
|
Follow-up: the rust job on |
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Approving at 8e9a4ccb. Both items I blocked on are closed, and the reason I
like this diff is the test you added rather than the line you changed: it pins
the constructor instead of the behaviour, which is the only thing that would have
caught the gap in the first place. Proof of that below.
cargo fmt is clean now:
$ ./ptcargo.sh t402b p402 cargo fmt --all --check 2>&1 | grep -vE '^time=' | head -10
That printed nothing, where the previous head printed two diffs in planner.rs.
The baseline moved with the suite. 1,846 in tests/evidence/workspace-tests.json
and in all three prose figures, and CI's rust job recounted and agreed.
The added test is the guard, and nothing else is
I deleted the .with_audit_log(SafetyAuditLog::new(SafetyAuditLog::default_path()))
call from a scratch copy of the tree with a Python edit that asserts the call is
gone before it writes, then ran your test and the four audit-log tests that
already existed:
$ ./ptcargo.sh t402b p402 cargo test -p sysknife-brain --lib --offline configured_planner_attaches_audit_log 2>&1 | grep -E "^test |test result|panicked|must attach"
test planner::tests::configured_planner_attaches_audit_log ... FAILED
thread 'planner::tests::configured_planner_attaches_audit_log' (292) panicked at crates/sysknife-brain/src/planner.rs:1532:9:
from_config must attach the safety audit log
$ ./ptcargo.sh t402b p402 cargo test -p sysknife-brain --test planner --offline audit 2>&1 | grep -E "^test |test result" | tail -8
test planner_without_audit_log_does_not_panic_on_rejection ... ok
test rejected_plan_is_written_to_safety_audit_log ... ok
test an_unknown_tool_call_is_written_to_the_safety_audit_log ... ok
test a_reasonless_refusal_is_written_to_the_safety_audit_log ... ok
test result: ok. 4 passed; 0 failed
Four tests named after the rejection path stay green with the fix removed,
because each one builds its planner through new and attaches the log by hand.
Your unit test is the only assertion in the workspace that fails. That is the
shape this repo keeps getting wrong, and you closed it from the right side.
What changes for someone running it
from_config is the production constructor:
$ git grep -n "from_config(" -- apps crates | grep -v "^crates/sysknife-brain/src/planner.rs" | head -10
apps/sysknife-cli/src/mcp_server.rs:792: let mut planner = LlmPlanner::from_config(config, Box::new(state_client))
apps/sysknife-cli/src/runner.rs:1744: let mut planner = LlmPlanner::from_config(config, Box::new(plan_client))
apps/sysknife-shell/src-tauri/src/commands.rs:192: let planner = LlmPlanner::from_config(config, build_state_client()).unwrap_or_else(|err| {
apps/sysknife-shell/src-tauri/src/commands.rs:208: LlmPlanner::from_config(BrainConfig::ollama_defaults(), build_state_client())
On main, with_audit_log has no caller outside tests:
$ git grep -n 'with_audit_log' origin/main -- crates apps tests
origin/main:crates/sysknife-brain/src/planner.rs:635: pub fn with_audit_log(mut self, log: SafetyAuditLog) -> Self {
origin/main:crates/sysknife-brain/tests/planner.rs:714: .with_audit_log(audit_log);
origin/main:crates/sysknife-brain/tests/planner.rs:2223: .with_audit_log(SafetyAuditLog::new(&log_path));
origin/main:crates/sysknife-brain/tests/planner.rs:2257: .with_audit_log(SafetyAuditLog::new(&log_path));
So all three if let Some(audit) = self.audit_log.clone() branches in
planner.rs (lines 1199, 1247, 1414) were unreachable in a shipped binary.
After this, a fence rejection during sysknife ask or an MCP planning call
appends a JSONL row to $XDG_DATA_HOME/sysknife/safety-audit.jsonl, defaulting
to ~/.local/share/sysknife/safety-audit.jsonl. An operator who wants to know
what the planner tried and the fence stopped now has a file to read.
One note I raised last round and am not asking you to change: this makes
default_path's /tmp fallback reachable in production for the first time. It
warns on stderr when HOME is unset, and every shipped entry point sets HOME,
so I would rather fix that in a separate change that refuses instead of degrades.
What stops the merge, and it is not yours
#384 sets the same artifact to 1,850, and both numbers are right against today's
main. Whichever lands first moves it under the other, so if yours is second it
needs UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh and the three prose
figures in one commit. Nothing else.
My merge gate cannot write a receipt for this PR either, which is my tooling:
$ maintainer-merge verify 402 8e9a4ccb6e8fe74168f7e989ad49aa0ab0900dac "configured_planner_attaches_audit_log" "s/\.with_audit_log(SafetyAuditLog::new(SafetyAuditLog::default_path()))//" 2>&1 | head -25
maintainer-merge: no single suite in <profile>/verify.d covers every changed path:
README.md docs
crates/sysknife-brain/src/planner.rs rust
docs/distro-support.md docs
docs/introduction.md docs
tests/evidence/workspace-tests.json no suite covers this
A Rust change that adds a test always moves that artifact, so the gate refuses
every well-formed Rust PR here. I am fixing my side of that.
Two housekeeping corrections
I told you last round that the assignment on #236 had not taken. It has:
assignees: ["vsolano9"] with the claimed label. My earlier note is stale, and
the issue is recorded as yours where GitHub can see it.
#397 is the one I would point you at next. CHAIN_ROW_COLUMNS is declared twice
and only a live Postgres run notices when the copies drift, which is the same
shape as this change: one declaration is load-bearing and the other is a
copy nobody proved. It is labelled easy, it is free, and I will hold it for you
the moment this lands.
Your diff reads like someone who wants to know what a machine refused and why.
If you administer Ubuntu boxes, the read-only surface is worth pointing at one:
sysknife ask "what is pending on this host" plans and previews without touching
anything, and the safety audit log you wired up here is the file that tells you
what the fence caught. Take it or leave it, and it has nothing to do with the
merge.
|
Merged as I verified the fix rather than reading it. On your head, Restored, I wrote the CHANGELOG entry at merge, under Unreleased. Your note about the two rate-limit races reproducing on clean Two things about the mechanics on your side, both mine rather than yours. The board went from Next, if you want it: #407. Unrelated, and easy to say no to. Four of your merged PRs are audit plumbing, so you are the person most likely to notice what a real audit trail looks like when it is wrong. |
2 participants
Summary
SafetyAuditLog::default_path()in the productionLlmPlanner::from_configpathLlmPlanner::newconstruction opt-in for embedded callers and testsFixes #236
Verification
cargo test -p sysknife-brain --locked -- --test-threads=1(333 passed)cargo test -p sysknife-brain --lockedalso exercised the default parallel suite; it reproduces two pre-existing rate-limit test races (calls_up_to_limit_all_succeed,call_over_limit_returns_error). The same baseline command on cleanupstream/mainreproduced the race (one of the two tests failed there).