fix(compliance): make a failed consent record visible and retryable

[aabusair]

Defects 7, 8 and 9 from the policy_selector rollout. Defects 4–6 are deliberately not here — they all require a fetch failure, and the measured rate is zero across 214 evaluations in 7 sweeps, which is why #72 was closed.

7 and 8 are one bug

record_signature() fails
  -> return False, discarded
  -> set_commit_status(success) runs unconditionally
  -> PR green, nothing in signatures/<doc>.json
  -> next sweep: #67 short-circuit sees success, returns early, never retries

7 creates the gap, 8 makes it permanent. The signature comment is evidence of intent; the JSON file is the record of record — and it was silently optional.

Fix. Capture the return value. On failure the status stays SUCCESS — the contributor signed, and an infrastructure fault on our side isn't theirs to pay for — but the description gains a record pending marker, and the short-circuit exempts marked statuses so the next sweep retries. A successful retry repaints the clean description, so the PR converges and stops being re-processed.

get_existing_status_state is renamed to get_existing_status, not just changed to return a tuple. A caller left comparing the old name to "success" would silently evaluate False against a tuple, the #67 short-circuit would stop firing, and the repaint loop it exists to prevent would come straight back. An AttributeError is the better failure mode.

9 — a failed sweep must not look like a clean one

Every PR is swept inside a try/except that logs and continues, so a sweep where every PR failed exited 0 and was indistinguishable from success in the Actions UI. On a 5-minute cron with nobody reading logs, that's invisible indefinitely.

Failures are now collected, named, and the sweep exits non-zero — behind SWEEPER_STRICT_EXIT so a transient fault can be quieted without reverting code or disabling the schedule. An empty installation list is now a failure too, rather than a quiet return.

Scope note, to avoid overclaiming: 9 catches exceptions, not failed record writes. record_signature returns False rather than raising, so a pending record does not redden the sweep. The retry and the status description are what surface that. The two changes are complementary, not the same mechanism.

An existing test was hiding the bug

test_correct_sentence_passes_and_is_recorded never gave its fake a writable signatures route, so record_signature was returning False on every run. The test asserted the clean description and passed — because the failure was invisible even inside the suite. It now uses writable routes, and the failure path has its own tests.

Verification

• 126 tests, up from 101. cla_sweeper.py had no tests at all; it now has 11.
• All 16 mutations caught by their named tests, tree restored clean after each:

Mutation	Caught by
Discard the return value again	test_failed_write_is_marked_in_the_description
Paint clean regardless of the write	same
Block the contributor instead of passing	test_failed_write_still_passes_the_contributor
Drop the pending-record exemption	test_success_with_pending_record_is_reprocessed
has_pending_record pinned False	test_marker_is_detected
has_pending_record pinned True	test_plain_success_is_still_skipped + 1
Case-sensitive marker match	test_detection_is_case_insensitive
Marker drifts from the painted text	test_marker_is_detected
Skip on any status, not just success	test_failure_and_pending_states_are_unaffected
Stop counting failures	test_any_failed_pr_returns_nonzero
Force exit 0	same
Ignore the kill switch	test_kill_switch_suppresses_the_nonzero_exit
Kill switch defaults off	test_default_is_strict
Unrecognised value disables strict	test_anything_else_leaves_strict_exit_on
Empty repo list quiet again	test_no_repositories_is_a_failure_not_a_quiet_success

• All 25 new tests provably killable — 21 by reverting a changed file, the 4 characterisation tests by targeted mutation.
• Suite runtime 9s → 0.04s. The harness stubs the real time.sleep in the compliant path and record_signature's retry jitter. Also extracted ProcessSinglePrHarness so the new class doesn't inherit and re-run TestProcessSinglePr's 24 tests under its own name.

Behaviour worth a reviewer's judgement

A permanently failing write means that PR is re-processed every sweep indefinitely — repainting bumps updated_at, keeping it in the lookback window. That's the #67 loop, deliberately re-opened for this narrow case, because the alternative is a consent record that never lands. It costs one status repaint per sweep for that PR, posts no comments, and is visible on the PR itself as record pending. I think that's the right trade, but it is a trade.

🤖 Generated with Claude Code