fix: the round-2 audit blockers — my own blocker fix was a no-op - #386
Merged
Conversation
The re-audit found the guard I added for blocker 1 could never fire. It
classified command-mode-vs-skill-mode by the ABSENCE of ONEBRAIN_SCHEDULED —
which is the very property whose absence defines staleness. A stale skill-mode
plist (redirect, no marker) is byte-identical to a command-mode one in exactly
those two fields, so it was classified as command mode and skipped, and the
stale branch was unreachable. On the release machine the shipped binary emitted
the false reassurance verbatim, against the real plists.
The test could not catch it: it hand-injected Some(false) into the CONSUMER and
never fed a plist body to the CLASSIFIER. The sabotage inherited the same blind
spot. Now classified by ProgramArguments containing skill+run, which no
command-mode plist emits and which is independent of both the redirect and the
marker; and the probe is split into a pure classify_plists_in(dir) so tests
drive real plist bodies, including the mixed case where one current plist must
not mask a stale one. Restoring the old classifier now fails that test.
Blocker 2's fix had traded a false positive for a false negative: entries whose
window had not elapsed were suppressed from the accusation but still counted
into 'all producing output', so doctor asserted health for entries that had
produced nothing. The same overstatement, one branch over. They are now named
as too-soon-to-judge rather than folded into a green claim.
Also: write_job_log short-circuited on env::var("HOME"), unset on Windows, so
the LOCALAPPDATA branch was unreachable and Windows silently had no CLI-owned
job log — and the None it returned is the same value that means 'written fine',
so the record carried no [job log unavailable] note either. Two failures hiding
each other. Uses dirs::home_dir() and reports a real reason now.
The CHANGELOG bullet describing the log-dir warning was written before the
blocker fix changed that message and no longer matched it.
Its only caller is the cfg(macos) resolver, so on Linux and Windows the function has no caller in the non-test build and clippy -D warnings rejects it. Local clippy on macOS structurally cannot see this — the caller exists there. CI's Linux leg caught it. Kept ungated with a conditional allow rather than cfg'd to macOS: the logic is pure string matching over a plist body, so it stays testable on every platform, which is the whole reason it was split out of the $HOME resolver. The profile documents a cross-check for exactly this class (cargo clippy --target <other>) and I did not run it before pushing. It would not have helped here anyway — onebrain-cli cannot cross-compile locally because ring needs Windows headers, and the Linux std is not installed — so CI is the only available verifier for this file. Saying so rather than implying I proved it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second full-epic audit returned DO-NOT-TAG. It found that the fix for the first audit's blocker 1 could never fire, and that the fix for blocker 2 had traded a false positive for a false negative. Both verified against the release machine's real plists before acting.
The no-op, and why three layers of verification agreed it was fine
skill_mode_plists_are_currenthad to tell skill-mode from command-mode. It keyed that on the absence ofONEBRAIN_SCHEDULED— which is the very property whose absence defines a stale plist.Measured on the release machine:
Byte-identical in exactly the two fields the classifier read. So every stale skill-mode plist was classified as command-mode and skipped, and the stale branch was unreachable by construction. The audit reproduced the false reassurance verbatim by copying the real plists into a fixture and running the shipped binary.
Three things agreed it was correct and all three were blind for the same reason:
Some(false)into the consumer (scheduler_log_dir_verdict) and never fed a plist body to the classifierFix: classify by
ProgramArgumentscontainingskill+run— emitted only by the skill-mode renderer, and independent of both the redirect and the marker. The probe is split into a pureclassify_plists_in(dir)so tests drive real plist bodies, including the mixed case where a current plist must not mask a stale one. Restoring the old marker-based classifier now fails that test.That split is the v3.4.22 lesson applied again: a check that reads
$HOMEcannot be tested without lying about the machine it runs on.Blocker 2's fix overcorrected
Entries whose window had not elapsed were suppressed from the accusation — correct — but still counted into
observed, so the verdict returned:…over an entry that had produced nothing. A monthly job with a fire window three days past, inside the mechanism's lifetime, silently vouched for. The first fix removed a false accusation and installed a false reassurance one branch over.
They are now named as too soon to judge rather than folded into a green claim.
ab885e8's own commit message set the standard this violated: "Reporting green while unable to distinguish 'new' from 'all dead' is the check overstating its reach."Windows had no CLI-owned job log at all, and the failure erased its own evidence
write_job_logdidenv::var("HOME").map(PathBuf::from).ok()?.HOMEis normally unset on Windows, so:?short-circuited —default_log_dir'sLOCALAPPDATAbranch was unreachable from this callerNoneit returned is the same value that means "log written fine", sobuild_tailadded no[job log unavailable: …]noteTwo failures hiding each other. Uses
dirs::home_dir()now and returns a real reason when it cannot resolve. CI would not have caught this either — nothing asserts a job log exists on Windows, and the tests touching this path pinHOME, which masks it.CHANGELOG bullet corrected
It said the warning "names only the entries it is fatal for". After the blocker fix the stale path deliberately says ALL entries will fail — which is true while the plists are pre-v3.4.23. Bullet rewritten to describe both states.
Guards
Gates — bare, real exit codes, on a disk with room
The previous audit's
cargo testand coverage runs died onNo space left on device— the host was at 100% from five cargo worktrees left by this epic. 41 GB reclaimed; those gates were UNKNOWN, not failing.The audit will be re-armed against this tree. Two consecutive audits have each found real blockers inside the previous audit's fixes, so a third pass is not ceremony.