Skip to content

test(labels): exercise the priority-label path's six degraded branches - #9867

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
test/priority-label-degraded-9866
Jul 29, 2026
Merged

test(labels): exercise the priority-label path's six degraded branches#9867
loopover-orb[bot] merged 1 commit into
mainfrom
test/priority-label-degraded-9866

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #9866. Follow-up to #9737 / #9862.

Problem

maybeHandlePriorityLabelEligibility wraps every I/O call it makes in a .catch that degrades rather than failing the webhook delivery — the settings resolve, the collaborator-permission read, the label removal, the policy comment, the audit row, and the webhook-event row.

Those handlers are the entire reason a label-mutating rule is safe to run on a webhook path: a throw that escaped would fail the whole delivery, taking every handler after it on the same event with it. None of the six was exercised — the existing "fails open when the permission cannot be read" case asserts a 404 response, which takes the ordinary return path rather than the catch. codecov/patch on #9862 measured 93.18% against the 99% target for exactly these six arrow bodies.

What changed

Test-only, +71 lines, four cases that make the calls genuinely throw and assert the degraded outcome rather than merely the absence of a throw:

  • settings resolver rejects → falls back to the built-in gittensor:priority and still enforces
  • permission read throws (not a 404) → fails open, no strip, because a thrown read is not evidence of ineligibility
  • label removal and comment both throw → the delivery still resolves, and the decision is still on the record
  • audit and webhook-event writes throw → the delivery still resolves, and the label was still removed

The D1 stub matches both insert into audit_events and Drizzle's quoted insert into "audit_events", since the two writers generate different SQL.

Validation

17 tests pass. Measured against the v8 report, branch-counted, over the whole maybeHandlePriorityLabelEligibility body: zero uncovered statements, zero uncovered functions (the six .catch arrows), zero uncovered branches. src/github/comments.ts and src/review/priority-label-eligibility.ts are likewise clean across the #9737 diff.

#9866)

Every I/O call in maybeHandlePriorityLabelEligibility is wrapped in a .catch that
degrades rather than failing the webhook delivery -- which is what makes a
label-mutating rule safe to run on a webhook at all, since an escaping throw would
take every handler after it on the same event down with it.

None of the six was exercised: the existing fail-open case asserts a 404 RESPONSE,
which takes the ordinary return path, not the catch. These make each call actually
throw and assert the degraded outcome.
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-29 17:33:38 UTC

1 file · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a test-only PR that adds four new test cases exercising the six .catch-degradation branches in maybeHandlePriorityLabelEligibility by making the underlying I/O calls genuinely throw, rather than just return non-2xx, closing the coverage gap called out against #9862. The tests are well-constructed: they simulate real thrown errors (not fabricated impossible states) at the settings resolver, permission read, label removal/comment, and D1 write layers, and assert the actually-relevant degraded outcome (fallback label name, fail-open, audit row still written, delivery not throwing) rather than just absence-of-throw. The regex matching both quoted and unquoted insert statements to intercept Drizzle vs raw-SQL writers is a reasonable, low-risk approach.

Nits — 5 non-blocking
  • The D1 prepare mock in the last test intercepts any INSERT into audit_events or webhook_events regardless of which delivery/test run it belongs to — if these tests ever run concurrently against a shared env this could be fragile, though currently they use per-test envs so it's fine.
  • The four new tests share a lot of stub-fetch boilerplate with the rest of the file; consider extracting a small helper for 'stub with per-URL throw' to reduce duplication (test/unit/priority-label-webhook.test.ts).
  • Consider asserting that no audit row is written in the 'permission read throws' case, to positively confirm the fail-open path takes no side-effecting action at all, not just that DELETE wasn't called.
  • The comment on line matching the audit/webhook_events regex could reference the exact source files (e.g. src/db/repositories.ts vs the raw writer) for future readers tracing which writer generates which SQL shape.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9866
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 13 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 334 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal
Linked issue satisfaction

Addressed
The PR adds four new test cases that make the settings resolver, permission read, label removal/comment, and audit/webhook-event writes actually throw and asserts the degraded outcomes (fallback label, fail-open, delivery still resolves, label still removed), matching each bullet in the issue's scope.

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 334 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: merge · clause: success
  • config: 340d6835d1f1955e67181b4134f8c37eeb671f5bc2ea96534d7df13fc21ba2fc · pack: oss-anti-slop · ci: passed
  • record: 70a8924b800a0765cc9b9544d43473701f2a85ea81a378879a768d62f8bc9d18 (schema v5, head 0302806)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@JSONbored JSONbored self-assigned this Jul 29, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.87%. Comparing base (889defe) to head (0302806).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9867      +/-   ##
==========================================
- Coverage   91.74%   90.87%   -0.88%     
==========================================
  Files         918      918              
  Lines      112965   112965              
  Branches    27184    27184              
==========================================
- Hits       103642   102654     -988     
- Misses       8037     9223    +1186     
+ Partials     1286     1088     -198     
Flag Coverage Δ
backend 94.12% <ø> (-1.57%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 4 files with indirect coverage changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Queued in the merge train behind #9865, which touches overlapping work and was opened first. This PR merges automatically once #9865 completes (or leaves the train). No action needed. This is an automated maintenance action.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored JSONbored removed the manual-review Gittensor contributor context label Jul 29, 2026
@loopover-orb
loopover-orb Bot merged commit 4e86232 into main Jul 29, 2026
8 checks passed
@loopover-orb
loopover-orb Bot deleted the test/priority-label-degraded-9866 branch July 29, 2026 17:33
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.

test(labels): cover the priority-label enforcement path's degraded branches

1 participant