Skip to content

fix(review): stop dirty-base PRs deferring forever on missing CI context - #7575

Merged
JSONbored merged 1 commit into
mainfrom
fix/dirty-base-missing-context-defer-forever
Jul 20, 2026
Merged

fix(review): stop dirty-base PRs deferring forever on missing CI context#7575
JSONbored merged 1 commit into
mainfrom
fix/dirty-base-missing-context-defer-forever

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • prReadyForReview (src/queue/processors.ts) deferred a PR with a missing required CI context forever, unlike its sibling "generic stuck CI" branch, which already finalizes past a cap. Live symptom: a real open pull request sat 40+ minutes with zero labels/gate check-run, repeating "Required CI context is still missing" every ~4 minutes, because its base was dirty and no required CI could ever arrive.
  • Fix: when the PR's live mergeableState is "dirty", skip the indefinite defer and fall through to the existing finalize-past-cap + per-head-SHA guard path, so the disposition planner (agent-actions.ts's isConflict) can close it — the documented "base conflict → CLOSE" behavior that currently never gets a chance to run for this case.
  • Narrows, not removes, an earlier design (see the queue.test.ts tests named #selfhost-ci-deferral-staleness) that made this defer unconditional for every OTHER missing-required-context PR — those regression tests, plus a related repair-budget test in queue-2.test.ts, are unchanged and still pass. That earlier design's concern was a clean-base PR's gate reading "passed" before a required check might still come back red; that can't happen for a dirty base, since isConflict closes the PR regardless of gate conclusion or CI outcome.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • 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
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • npm run ui:build/ui:test: this machine runs Node v26.5.0 (repo pins Node 22 via .nvmrc); every apps/loopover-ui/apps/loopover-miner-ui test touching window.localStorage fails locally with Cannot read properties of undefined (reading 'clear'), reproducing on both UI workspaces and in isolation from this change — a vitest-jsdom/Node compatibility issue unrelated to this diff (those apps are untouched here). Every CI workflow pins Node via .nvmrc, so this isn't expected to reproduce in the real gate. npm run test:ci ran clean through ui:typecheck, including the full unsharded test:coverage, before hitting this pre-existing local-environment gap.

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.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — backend-only change (src/queue/processors.ts, test/unit/queue.test.ts), no visible UI/frontend/docs surface.

Notes

prReadyForReview deferred a PR with a missing required CI context
unconditionally, with no staleness escalation, unlike its sibling
generic-stuck-CI branch which finalizes past a cap. A dirty base is
terminal regardless of CI: the disposition planner's isConflict check
already closes such a PR independent of gate conclusion or CI state,
so waiting could never change the outcome -- it just left the PR
stuck open and unlabeled indefinitely.
@JSONbored JSONbored self-assigned this Jul 20, 2026
@JSONbored
JSONbored merged commit 6b8b3dd into main Jul 20, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/dirty-base-missing-context-defer-forever branch July 20, 2026 22:38
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.46%. Comparing base (917b28b) to head (26ba627).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7575      +/-   ##
==========================================
- Coverage   88.48%   88.46%   -0.02%     
==========================================
  Files         720      720              
  Lines       75748    75749       +1     
  Branches    22558    22559       +1     
==========================================
- Hits        67023    67013      -10     
- Misses       7679     7686       +7     
- Partials     1046     1050       +4     
Flag Coverage Δ
shard-1 36.51% <100.00%> (+0.01%) ⬆️
shard-2 36.38% <0.00%> (-0.12%) ⬇️
shard-3 32.74% <0.00%> (-0.01%) ⬇️
shard-4 34.05% <0.00%> (-0.02%) ⬇️
shard-5 32.02% <0.00%> (-0.01%) ⬇️
shard-6 33.41% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 95.61% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

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.

fix(review): a dirty-base PR with missing required CI context defers forever instead of reaching the close disposition

1 participant