Skip to content

fix(grug): self-heal the ruleset grug actually found, not the one on file - #811

Merged
quadseven merged 4 commits into
mainfrom
fix/686-heal-live-matched-ruleset
Aug 1, 2026
Merged

fix(grug): self-heal the ruleset grug actually found, not the one on file#811
quadseven merged 4 commits into
mainfrom
fix/686-heal-live-matched-ruleset

Conversation

@quadseven

Copy link
Copy Markdown
Owner

Why

detect_enforcement can classify a repo grug_managed through its name-prefix heuristic on a ruleset whose id is not the stored one - a rename or other drift leaves the stored id stale (grug#565). But ensure_enforcement's self-heal still targeted stored_ruleset_id.

When they diverge: get_ruleset(stale_id) 404s, the caller's broad except logs a warning, and that repo silently never heals. A self-heal that cannot report its own failure is worse than none - it reads as working.

Flagged by CodeRabbit on PR #685 and scoped out of it deliberately, because fixing it is a public-contract change.

Summary

  • detect_enforcement now returns EnforcementDetection(state, ruleset_id) - the id of the ruleset that actually produced the classification.
  • The self-heal uses that id, and persists the correction so the next pass starts from truth (same posture as heal_enforcement's post-recreate persist). The persist is best-effort: a store hiccup must not stop the heal it was recording.
  • ruleset_id is None whenever no specific ruleset was matched (external via legacy branch protection, and every none), so the self-heal falls back to stored_ruleset_id and that path is unchanged.

Call sites: enforcement.py consumes the id; poller_handler.py and installations.py take .state.

Acceptance criteria

  • detect_enforcement returns the matched live ruleset id, not just the state string - EnforcementDetection NamedTuple
  • Self-heal uses the live-matched id when it differs from stored, and persists the correction
  • All 3 call sites updated; the detect endpoint's HTTP response shape is unchanged (still just the state string)
  • Regression test: detection finds a live Grug ruleset whose id differs from the persisted one, and migrate_check_context is asserted to target the LIVE id (999), not the stale stored one (555)
  • Full webhook suite green: 1871 passed, 2 skipped, 0 failed. API suite: 288 passed.

Size

Size: M

Out of scope

  • Any change to detect_enforcement's matching or classification LOGIC (prefix heuristic, id-match-first ordering). Only its return shape changed.

Note on the 40 updated test sites

Existing mocks now return ruleset_id=None on purpose: that reproduces the old fall-back-to-stored behaviour exactly, so every updated test keeps asserting what it always asserted rather than being re-baselined to the new code. The drift case gets three new tests of its own - heal targets the live id, no needless store write when it already matches, and a persist failure does not block the heal.

closes #686

The backlog went from ~16 open issues on 2026-07-01 to 85 on 2026-07-31.
Intake measured 159 issues / 30 days against 90 closed, so it grows no
matter how fast work ships - and 34 of the 85 (40%) belonged to no epic.

Filing is frictionless; linking to an epic means knowing the epic map.
So the cheap half of the workflow runs at 5.3/day and the expensive half
does not run at all. That asymmetry, not any neglected epic, is why the
orphan pool tracks the backlog.

The rule goes in the two places a filer actually reads: the work-item
template (humans, with the live epic list inline) and the agent
issue-tracker doc (agents filing via gh, which never see the template).
`orphan-ok` is a real answer, not a loophole - it makes a standalone
item a visible decision instead of a silent omission.

Also refreshes the doc's label list, which had drifted: adds orphan-ok
and moves epic-security / epic-grug-saas to their archived- names.

closes #808
Elder's pre-publish freshness guard compared only the freshness id. That
id also hashes title and body, so editing the PR body moved it while the
code under review was byte-identical - and the guard then discarded the
finished review as stale.

The discard is not one wasted pass. It re-enqueues against the SAME sha,
`elder_in_progress_check_skipped: already_in_progress` declines to post a
check-run because one is still open, and the check sits `pending`.

Live on quadseven/infra#2133 today: Elder passed at 03:00:27, the author
edited the body to satisfy Chief's own DoR gate at 03:08, and a docs-only
PR went back to pending for 25 minutes. Satisfying one grug check should
not stall another.

Head sha now decides. When the reviewed sha is still current the findings
anchor to identical lines, so they publish; only the intent blurb the
model also saw has changed. A rewritten intent can legitimately change
findings, so the re-enqueued pass still runs - it just no longer destroys
a good verdict first, and the check stays green meanwhile. This is the
same bounded-staleness trade-off review_freshness_id already makes for
base_sha, applied to the other two fields it hashes.

Also stops the cancel path saying "superseded by a newer commit" when the
watcher fired on a title/body edit and no commit ever landed. The cancel
is real; naming the wrong cause is what sent operators hunting.

Two tests asserted the old behavior and encoded the bug: one drove the
full dispatch path with an unchanged sha, the other computed a changed
freshness id but returned the OLD sha as current. Both now match reality.

Covers #773 acceptance criteria 1 and 4, and removes the trigger for 2.
Criterion 3 (bound repeated re-enqueues on an unchanged sha) and the
cave-transport question in item 4 are untouched, so this does not close
the issue.

Refs #773
detect_enforcement can classify a repo grug_managed through its
name-prefix heuristic on a ruleset whose id is NOT the stored one - a
rename or other drift leaves the stored id stale (grug#565). But
ensure_enforcement's self-heal still targeted stored_ruleset_id. When
they diverge, get_ruleset(stale) 404s, the broad except swallows it as a
warning, and that repo silently never heals.

detect_enforcement now returns EnforcementDetection(state, ruleset_id) -
the id of the ruleset that actually produced the classification. The
self-heal uses it, and persists the correction so the next pass starts
from truth (same posture as heal_enforcement's post-recreate persist).
The persist is best-effort: a store hiccup must not stop the heal it was
recording.

ruleset_id is None whenever no specific ruleset was matched - external
via legacy branch protection, and every none - so the self-heal falls
back to stored_ruleset_id and behaviour is unchanged on that path.

Call sites: enforcement.py consumes the id; poller_handler.py and
installations.py take .state. The detect endpoint's HTTP response shape
is deliberately unchanged - it still returns just the state string even
though detect_enforcement now returns more.

Existing mocks return ruleset_id=None on purpose, which reproduces the
old fall-back-to-stored behaviour exactly, so the 40 updated test sites
keep asserting what they always asserted. Three new tests cover the
drift case itself: heal targets the live id, no needless store write
when it already matches, and a persist failure does not block the heal.

closes #686
@grug-tribe

grug-tribe Bot commented Aug 1, 2026

Copy link
Copy Markdown

Grug walk most of trail. Some ground not walked.

fix(grug): self-heal the ruleset grug actually found, not the one on file

Grug Elder - Partial coverage - no markings on the ground Grug walked - check `neutral`
  • Fast look only - deep look may add more.

Some ground not walked this pass - part of the diff did not fit one look. What Grug did walk is above. Grug not say trail safe for ground Grug not walk.

Grug Elder marked detect_enforcement `high-complexity` on this PR, and it
was right: tracking WHICH ruleset matched added two more branches and
pushed it to cyclomatic 16 against a cap of 15.

The scan loop moves to _match_enforcing_ruleset, which returns the
classification plus the matching id, or None when no ruleset enforces the
check so the caller falls back to legacy branch protection.

detect_enforcement 16 -> 6, _match_enforcing_ruleset 11. Behaviour is
unchanged: 1871 webhook + 288 api tests still pass with no test edits.
@quadseven

Copy link
Copy Markdown
Owner Author

Elder's high-complexity marking on github_rulesets_client.py:482 was correct and is now fixed in f8f9bfe.

Tracking WHICH ruleset matched (the point of #686) added two branches and pushed detect_enforcement to cyclomatic 16 against a cap of 15. Confirmed with radon rather than argued with:

before:  detect_enforcement          C (16)
after:   detect_enforcement          B (6)
         _match_enforcing_ruleset    C (11)

The scan loop moved to _match_enforcing_ruleset, which returns the classification plus the matching id, or None when no ruleset enforces the check so the caller still falls back to legacy branch protection. Behaviour is unchanged - 1871 webhook + 288 api tests pass with no test edits.

Noting for the record that this finding arrived only in the board comment, not as a review thread, so pr-bot-comments.sh reported clean. Worth reading the board even when the sweep exits 0.

@quadseven
quadseven merged commit 5fc6567 into main Aug 1, 2026
20 checks passed
@quadseven
quadseven deleted the fix/686-heal-live-matched-ruleset branch August 1, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enforcement: self-heal should migrate the live matched ruleset, not just the stored ID

1 participant