Skip to content

fix(review): never let an inconclusive linked-issue recheck downgrade a propagated label - #4816

Merged
JSONbored merged 2 commits into
mainfrom
claude/pr-labeling-regression-83df0a
Jul 11, 2026
Merged

fix(review): never let an inconclusive linked-issue recheck downgrade a propagated label#4816
JSONbored merged 2 commits into
mainfrom
claude/pr-labeling-regression-83df0a

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • A merge-time (or later sweep/webhook) recheck of a PR's linked-issue label propagation could not distinguish "the recheck genuinely couldn't be verified this pass" (a transient GitHub fetch/rate-limit failure) from "the linked issue confirms no propagation applies" — both fell through to the blunt title heuristic and silently, permanently overwrote whatever correct label a prior pass had already applied.
  • Confirmed live via server telemetry (Loki/Tempo/Prometheus on the self-host fleet): 118 PRs mislabeled across gittensory and metagraphed in a 2-day sample, 27 of them on the day this fix was written, all still wrong on live GitHub — including contributor-reported PRs feat(ui): check-run details-page readiness table (#2216) #4716 and Multi-tenant hosted architecture spec #4783. This reproduces (via a different trigger) the exact symptom the fix(review): merging a PR strips its propagated gittensor:priority/feature labels #4528 fix (merged 2026-07-09) targeted, which only closed the narrower "issue closed by this PR's own merge" race, not a concurrent-pass race or any other transient recheck failure.
  • Fix, five parts (all evidence-grounded, see the #regression-safe-propagation doc comments in each file for the specific mechanism each one closes):
    1. linked-issue-label-propagation-fetch.ts: fetchLinkedIssueLabelsForPropagation now returns {labels, inconclusive} instead of a bare label list; isRepoMaintainerLogin distinguishes a confirmed permission result from an errored (inconclusive) one and logs the error instead of silently swallowing it.
    2. processors.ts: the type-label block now skips the mutation entirely (leaving existing labels untouched) when the recheck was inconclusive rather than confirmed-negative, and claims the existing per-PR actuation lock (claimPrActuationLock, already used elsewhere under fix(queue): concurrent webhook and sweep jobs for the same PR are not mutually excluded #2129) so two concurrent passes for the same PR can no longer race each other here.
    3. client.ts: excludes the two trust-deciding endpoint shapes (bare linked-issue reads, collaborator-permission checks) from cross-caller request coalescing, so one caller's transient failure can no longer become a different concurrent caller's answer.
    4. public.ts / rag-index.ts / grounding-wire.ts: three GitHub callers were computing their rate-limit admission key before a token fallback was applied (or never attributing one at all for a high-volume caller) — this is what buried the live incident in an unattributed key_scope="unknown" metric bucket instead of a diagnosable one.
  • Does not touch the ~118 already-mislabeled PRs found live — those need a separate, deliberate backfill (like the 2026-07-09 83-PR backfill), not bundled into this code fix.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused — one root cause (an inconclusive-vs-confirmed-negative conflation in the propagation recheck, plus the three concrete rate-limit-attribution bugs that made it hard to diagnose), touching only the review-engine/GitHub-client code paths that chain to it.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves — this is a maintainer-authored fix for a bug found via direct production investigation (not a pre-filed issue); see Summary for the evidence trail instead.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — 100% line/branch coverage on every changed line across all 6 changed source files (verified directly against coverage/lcov.info for the diff's exact line ranges, not just the repo-wide trend number)
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — includes 3 end-to-end regression tests in queue.test.ts reproducing the exact PR feat(ui): check-run details-page readiness table (#2216) #4716/Multi-tenant hosted architecture spec #4783 race (inconclusive-skip-never-downgrades, lock-contention-skip, and a rewrite of a test that previously asserted the old buggy behavior)

Full npm run test:ci (all 27 steps, including test:engine-parity/test:driver-parity/the engine's own suite/test:workers/build:miner/test:miner-pack/rees:test/every ui:* check/ui:build) run twice end-to-end locally under Node 22 (per .nvmrc) with a clean exit both times.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A, no auth/session/CORS surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no API/OpenAPI/MCP surface touched (ui:openapi:check confirms no drift).
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI changes.
  • Visible UI changes include a UI Evidence section — N/A, backend-only change.
  • Public docs/changelogs are updated where needed — N/A, no user-facing docs affected; changelog intentionally not touched.

Notes

  • Root-caused via live Loki/Tempo/Prometheus telemetry on the self-host fleet, not source-reading alone — see the #regression-safe-propagation comments in each changed file for the specific confirmed mechanism (a Grafana alert/runbook that queries the rate-limit-responses metric by key_scope specifically depends on the "unknown" bucket staying visible for diagnosing exactly this class of bug in the future, which is why the metric-emission gating considered during development was reverted in favor of fixing the callers' own attribution instead — see the code comment on fetchWithGitHubRetry in client.ts).
  • Follow-ups intentionally out of scope for this PR: (1) a fresh backfill correcting the ~118 PRs already mislabeled live, (2) deploying this fix to the self-host production engine.

… a propagated label

A merge-time (or later sweep/webhook) recheck of a PR's linked-issue label
propagation could not distinguish "the recheck genuinely couldn't be
verified this pass" (a transient GitHub fetch/rate-limit failure) from "the
issue confirms no propagation applies" -- both fell through to the blunt
title heuristic and silently overwrote whatever correct label a prior pass
had already applied. Confirmed live: 118 PRs mislabeled across gittensory
and metagraphed in a 2-day sample, including contributor-reported PRs #4716
and #4783.

- linked-issue-label-propagation-fetch.ts: fetchLinkedIssueLabelsForPropagation
  now returns {labels, inconclusive} instead of a bare label list;
  isRepoMaintainerLogin distinguishes a confirmed permission result from an
  errored (inconclusive) one, and logs the error instead of swallowing it.
- processors.ts: the type-label block skips the mutation entirely (leaving
  existing labels untouched) when the recheck was inconclusive rather than
  confirmed-negative, and now claims the existing per-PR actuation lock so
  two concurrent passes for the same PR can no longer race each other here.
- client.ts: excludes the two trust-deciding endpoints (linked-issue reads,
  collaborator-permission checks) from cross-caller request coalescing, so
  one caller's transient failure can no longer become a different
  concurrent caller's answer.
- public.ts / rag-index.ts / grounding-wire.ts: three GitHub callers were
  computing their rate-limit admission key before a token fallback was
  applied (or never attributing one at all for a high-volume caller),
  which is what buried the live incident in an unattributed metric bucket.

Full regression coverage added, including an end-to-end reproduction of
the exact PR #4716/#4783 race in queue.test.ts.
…el tests

Rebased onto main's gateCheckMode-deprecation migration (#4618); the two new
regression tests added alongside it still need reviewCheckMode set directly,
matching every sibling test in the same describe block.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@JSONbored JSONbored self-assigned this Jul 11, 2026
@JSONbored
JSONbored merged commit cbe243c into main Jul 11, 2026
7 checks passed
@JSONbored
JSONbored deleted the claude/pr-labeling-regression-83df0a branch July 11, 2026 00:33
@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.23%. Comparing base (9277537) to head (4d600ca).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4816   +/-   ##
=======================================
  Coverage   94.22%   94.23%           
=======================================
  Files         441      441           
  Lines       38745    38766   +21     
  Branches    14106    14111    +5     
=======================================
+ Hits        36508    36530   +22     
  Misses       1577     1577           
+ Partials      660      659    -1     
Files with missing lines Coverage Δ
src/github/client.ts 100.00% <ø> (ø)
src/github/public.ts 98.57% <100.00%> (+1.60%) ⬆️
src/queue/processors.ts 95.71% <100.00%> (+<0.01%) ⬆️
src/review/grounding-wire.ts 96.70% <100.00%> (-0.04%) ⬇️
src/review/linked-issue-label-propagation-fetch.ts 100.00% <100.00%> (ø)
src/review/rag-index.ts 93.86% <100.00%> (-0.04%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added a commit that referenced this pull request Jul 11, 2026
…es (#4814)

* test(unit): split queue.test.ts and backfill.test.ts into smaller files

queue.test.ts (33,461 lines / 810 tests) and backfill.test.ts (6,874 lines
/ 236 tests) each ran as one atomic unit under a single vitest worker --
queue.test.ts alone accounted for ~282s (roughly half the wall-clock) of
the full coverage run's ~565s duration, since vitest schedules whole test
files to workers and can't parallelize within one file.

Split each into several files along safe, verified boundaries (no shared
mutable state crosses a file boundary; hoisted helper functions used
across sections were promoted into each file's shared header). Test
count and pass/fail results are identical before and after (810 and 236
tests respectively, all passing) -- verified via `vitest run` on the full
old-vs-new file sets.

Also fixes two stale comments in src/queue/processors.ts that named the
old monolithic queue.test.ts file for a test that moved.

* fixup: port #4757's e2e-test-gen changes into split queue-5.test.ts

Rebasing onto main picked up #4757 (feat(review): decouple e2e-test-gen
auto-trigger and widen checkbox auth), which touched the monolithic
queue.test.ts before it was split. That content now lives in
queue-5.test.ts, so port the same test changes there: the new
autoTrigger opt-in on seedAutoTriggerPr, its three new/updated tests,
and the [BETA] badge text update. Verified against main's actual source
change (src/queue/processors.ts already carries the [BETA] label) and a
full run of all 6 split queue files: 813 tests passing (810 + 3 new).

* fixup: port #4732, #4659, #4816 test changes into split queue files

* fixup: port #4732 and #4816 test changes into split queue files

* fixup: port latest main's queue.test.ts changes into split queue-4.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant