fix(caller-drift): a fully remediated run is green, and the headline says so (backend#1965) - #255
Conversation
…says so (backend#1965)
Remediation runs after the report is built and never touches `findings`, so a
--create-prs run that opened a PR for every remediable copy still fell through to
`if findings: return 1`. The watchdog then headlines it Drift -- indistinguishable
from "drift exists and nobody has done anything".
THE ONE-LINE FIX WOULD HAVE MADE IT WORSE. caller-drift.yml already refuses to let
a clean exit code sit beside a non-zero findings count:
code == 0 and FINDINGS != 0
-> "⚠️ Inconsistent result -- the audit exited clean while reporting N
finding(s). Treat fleet conformance as UNKNOWN and fix the audit."
So `return 0` alone trades a false Drift for a false the-audit-is-broken, which is
the worse of the two: it tells the reader to stop trusting the audit. The exit code
and the step outputs have to move together, so they do:
findings= now the UN-REMEDIATED count (what the guard should compare)
findings_total= the total, for the body
remediated= how many ended the run with a PR open
and the verdict gains a branch, because a remediated run is green but is NOT
"every entry matched" -- the drift is real and still on the fleet until those PRs
merge. Saying Conformant would be this bug mirrored: a true exit code under a
false headline.
✅ Remediated — N drift finding(s), every one with a PR open. The fleet is NOT
yet conformant, it is fixed-pending-merge...
standards-sync.py's `return 0 if args.create_prs else 1` is NOT copied. It is safe
there because that tool has one homogeneous drift family. Here `findings` mixes
callers, copies, protection, rulesets and quality, and only `required` copies are
remediable -- a blanket rule would hide an unremediable protection or ruleset
finding behind a green run, a fail-open in the guard written to refuse exactly
that. The predicate is MEASURED instead: `remediated` is incremented beside the
write that earns it, only when the PR succeeded, and green requires it to account
for every finding there is.
The rule is now decide_exit(), a pure function, because that is what made it
testable. It previously lived inline at the bottom of main(), reachable only by a
full audit against the live org -- so the selftest asserted nothing about it, which
is why this survived. 11 new cases: the three the ticket names plus the ones that
keep the fix safe -- partial remediation stays RED and says how many are left, one
PR does not make nine findings green, and an unreadable read or a failed
remediation still outranks a fully remediated one.
Mutation-proved, anchors asserted: removing the remediated branch reddens the
ticket's own case; relaxing `remediated >= findings` to `if remediated` reddens the
nine-findings case. A third mutation -- counting entries whose PR FAILED -- does
NOT redden, and that is inert by construction rather than uncovered:
remediation_failures returns 2 before the remediated branch is reached, so a
miscount there cannot produce a false green. Said plainly instead of claimed as
coverage.
Mutating the remediated branch out also printed "-1 finding(s) remain", so the
partial-remediation count is clamped. Unreachable today; a nonsense count is how a
real message stops being read.
Scope note for the ticket: --create-prs only runs on a manual workflow_dispatch
with the input set, never on the weekly cron, so this was never the recurring
org-wide false red the ticket describes -- one dispatch exists in the run history
and it succeeded. Real, latent, and correctly last of the five.
VERIFIED: selftest 173/173 (was 162), make check green, ruff clean, actionlint
clean on caller-drift.yml. The verdict logic was extracted from the workflow and
EXECUTED over all six outcomes rather than read: clean/Drift/Remediated/partial/
remediation-failed/unreadable each render the intended headline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…atch (backend#1965) The exit-0 "Fail the run" step printed "every inventory entry matched" for any clean exit, but this PR made exit 0 also mean "fully remediated" -- drift found, every finding PR'd, fleet fixed-pending-merge. That is the false-green headline the watchdog Remediated branch already removes from the issue body; this final-step log was the one place still saying it. Give the step the REMEDIATED/FINDINGS_TOTAL outputs it lacked and branch the exit-0 message on the same signal the verdict uses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 463069e. Configure here.
… reddening the audit here
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Approve (backend#1965). A fully-remediated --create-prs caller-drift run was falling through to if findings: return 1 and reporting a red "Drift" — indistinguishable from unfixed drift — because remediation happens after the report and never decrements findings. Fix extracts decide_exit as a pure function (so it is finally assertable, covered by caller-drift-selftest.py) with a clear contract: 2 = unreadable/remediation-failed (state UNKNOWN), 1 = findings remain un-remediated (partial says so in the numbers), 0 = nothing wrong OR every finding got a PR. New "✅ Remediated" verdict is green but explicitly NOT conformant (fixed-pending-merge, red again if those PRs close unmerged). The watchdog body and the run-log branch on the same REMEDIATED signal so the headline can never contradict the counts, and the existing FINDINGS-vs-total inconsistency guard correctly compares the un-remediated count. >= + clamp guard the nonsense-count edge (found by mutating the branch out). CI green, Bugbot clean.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approve (backend#1965). Correctness review — focused on the exit contract and its coupling to the watchdog.
What it does: extracts the exit rule into a pure decide_exit() and makes a fully-remediated --create-prs run exit 0 instead of falling through to if findings: return 1. Step outputs split into findings (un-remediated), findings_total, and remediated; the workflow gains a Remediated verdict and a matching final-log branch.
Verified the core invariant. Each required copy that is missing/drifted appends exactly one entry to findings and one to remediable, 1:1 (caller-drift.py:1994-1997, 2005-2013), and remediated += len(entries) counts only groups whose PR succeeded (2175-2181). So remediated <= len(findings) always, and remediated >= findings is true only when every finding is remediable and every one got a PR — non-remediable protection/ruleset/quality findings correctly keep the run red.
Coupling holds. Because decide_exit returns 0 only when the un-remediated count is 0, the output findings=max(len - remediated, 0) is 0 on every green run, so the workflow consistency guard (code == 0 && FINDINGS != 0) can never fire a false "Inconsistent result" against a remediated run. Verdict precedence (unreadable → remediation-failure → remediated → partial/un-remediated) is right in both decide_exit and the YAML.
Checks reproduced locally: caller-drift-selftest.py → 173/173. The 11 new cases pin the ticket case, partial-stays-red, over-count-stays-green, and unread/failed outranking remediated.
No correctness, convention, or coverage issues found. Bugbot's earlier "green log still claims full match" finding was addressed in 463069e.
|
Thanks @shujaatTracebloc — approval received and the review itself stands. Flagging a mechanical problem so it isn't mistaken for a stale check: this PR still reads Added @saadqbal, who has Two things for @LukasWodka:
|
Closes tracebloc/backend#1965
The bug
Remediation runs after the report is built and never touches
findings, so a--create-prsrun that opened a PR for every remediable copy still fell through toif findings: return 1. The watchdog headlines that Drift — indistinguishable from "drift exists and nobody has done anything".The one-line fix would have made it worse
caller-drift.ymlalready refuses to let a clean exit code sit beside a non-zero findings count:So
return 0alone trades a false Drift for a false the-audit-is-broken — the worse of the two, because that headline tells the reader to stop trusting the audit. The exit code and the step outputs have to move together:findings=findings_total=remediated=And the verdict gains a branch, because a remediated run is green but is not "every entry matched" — the drift is real and still on the fleet until those PRs merge:
Saying Conformant there would be this bug mirrored: a true exit code under a false headline.
standards-sync.py's rule is deliberately not copiedreturn 0 if args.create_prs else 1is safe there because that tool has one homogeneous drift family. Herefindingsmixes callers, copies, protection, rulesets and quality, and onlyrequiredcopies are remediable — so a blanket rule would hide an unremediable protection or ruleset finding behind a green run. That is a fail-open in the guard written to refuse exactly that.The predicate is measured instead:
remediatedis incremented beside the write that earns it, only when the PR succeeded, and green requires it to account for every finding there is.Why this survived so long
The rule lived inline at the bottom of
main(), reachable only by a full audit against the live org — so the selftest asserted nothing about it. It is nowdecide_exit(), a pure function, which is what makes it assertable.11 new cases: the three the ticket names, plus the ones that keep the fix safe —
Verification
The verdict logic was extracted from the workflow and executed over all six outcomes rather than read:
Mutation-proved, anchors asserted:
That third one is inert by construction, not uncovered:
remediation_failuresreturns 2 before the remediated branch is reached, so a miscount there cannot produce a false green. Stated plainly rather than claimed as coverage.Mutating the remediated branch out also printed
-1 finding(s) remain, so the partial count is now clamped. Unreachable today — a nonsense count is how a real message stops being read.Scope correction for the ticket
--create-prsonly runs on a manualworkflow_dispatchwith the input set, never on the weekly cron, so this was never the "recurring org-wide audit" false red the ticket describes — one dispatch exists in the run history and it succeeded. Real, latent, and correctly last of the five.Test plan
python3 scripts/tests/caller-drift-selftest.py— 173/173make check— greenruff check scripts/caller-drift.py— cleanactionlint .github/workflows/caller-drift.yml— clean🤖 Generated with Claude Code
Note
Medium Risk
Changes audit exit semantics and org watchdog messaging for manual
--create-prsruns; behavior is covered by new unit tests but affects how operators interpret green CI and the conformance issue body.Overview
Fixes backend#1965: after
--create-prs, remediation ran after the report and never reducedfindings, so a run that opened a PR for every remediable copy still exited 1 and the watchdog showed Drift—the same headline as untouched drift.decide_exit()incaller-drift.pycentralizes the exit contract (0 = clean or fully remediated, 1 = un-remediated findings, 2 = unreadable / remediation failure). Green on remediation requiresremediated >= findings(not a blanketcreate_prsgreen), so protection/ruleset/quality findings that no PR can fix stay red.Step outputs now split counts:
findings= un-remediated (for the workflow consistency guard), plusfindings_totalandremediated.remediatedincrements only when a copy remediation PR actually succeeds.caller-drift.ymladds a Remediated verdict and findings breakdown when exit is 0 but PRs are open; the final job log distinguishes “fixed-pending-merge” from true conformant. Eleven selftest cases lockdecide_exit()behavior (full/partial remediation, unreadable, failed writes).Reviewed by Cursor Bugbot for commit 8c10915. Bugbot is set up for automated code reviews on this repo. Configure here.