Skip to content

feat(commands): wire @gittensory review and resume PR-comment commands - #4175

Merged
JSONbored merged 1 commit into
mainfrom
feat/review-resume-commands-1960
Jul 8, 2026
Merged

feat(commands): wire @gittensory review and resume PR-comment commands#4175
JSONbored merged 1 commit into
mainfrom
feat/review-resume-commands-1960

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Wires the last two handlers in the @gittensory PR-comment command surface: maybeProcessReviewCommand and maybeProcessResumeCommand, both mirroring the existing classify → authorize → dispatch shape used by pause/resolve/explain.
  • review (alias re-review) dispatches to the existing reReviewStoredPullRequest path with force: true, so a maintainer explicitly asking for a fresh pass bypasses the AI-review cache/dedup instead of getting a stale verdict. It never touches the Gate check-run, AgentActionMode, or the one-shot disposition — whatever the re-review's own gate evaluation produces is exactly what a scheduled sweep pass would produce.
  • resume fixes a real bug in hasAutoreviewPausedMarker: it previously only checked whether an autoreview_paused row had ever been recorded, so a resume command could authorize and post a confirmation comment but silently fail to actually resume anything — the next re-review pass would still see the stale pause as active forever. It now reads the most recent of {autoreview_paused, autoreview_resumed} for the target (order by created_at desc, rowid desc), with the rowid tiebreaker covering same-millisecond writes from back-to-back commands in a test/rapid-succession scenario.

Scope

Closes #2163, Closes #2165, Closes #1960 (the 10th and final sub-issue completed under the parent — command registration, authorization policy, pause/resolve/explain/configuration/help handlers, and the did-you-mean fallback all shipped in prior PRs).

Validation

  • npx vitest run test/unit/queue.test.ts — 714/714 passing, including ~10 new tests covering authorized dispatch, re-review alias resolution, unauthorized denial, missing-PR skip, and bot-author skip for review; and authorized resume superseding an earlier pause, full pause→resume→pause-again ordering, unauthorized denial, missing-PR skip, and bot-author skip for resume.
  • Deleted one pre-existing test (#2160) whose entire premise — "a catalog-registered verb with no dispatch handler exists" — is no longer true now that all 7 registered verbs have handlers; not a coverage dodge, the scenario it guarded against can no longer occur.
  • npx tsc --noEmit clean.
  • npm run docs:drift-check clean (no gate-mode/command-catalog field added; review/resume were already registered verbs pre-dating this PR).
  • git diff --check clean.
  • Rebased onto latest origin/main, no conflicts.

Safety

Notes

No generated-artifact regeneration needed (no API/schema, wrangler binding/var, src/selfhost/** env read, or DB schema changes in this PR).

Adds maybeProcessReviewCommand (#2163) and maybeProcessResumeCommand
(#2165), the last two handlers in the @gittensory PR-comment command
surface (#1960). Both mirror the existing classify -> authorize ->
dispatch shape used by pause/resolve/explain.

review dispatches to the existing reReviewStoredPullRequest path with
force:true so a maintainer gets a fresh verdict instead of a cached
one; it never touches the Gate check-run or one-shot disposition.

resume fixes hasAutoreviewPausedMarker, which previously only checked
whether a pause row had EVER been recorded, so a resume command could
authorize and confirm but never actually un-pause anything. It now
reads the most recent of {paused, resumed} for the target, with a
rowid tiebreaker for same-millisecond writes.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.73%. Comparing base (4664ad2) to head (a93956f).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4175   +/-   ##
=======================================
  Coverage   93.72%   93.73%           
=======================================
  Files         387      387           
  Lines       36320    36371   +51     
  Branches    13306    13316   +10     
=======================================
+ Hits        34042    34092   +50     
  Misses       1621     1621           
- Partials      657      658    +1     
Files with missing lines Coverage Δ
src/queue/processors.ts 95.30% <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.

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-08 09:53:33 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): src/queue/processors.ts (matched src/queue/**).

Review summary
This PR wires the final two @​gittensory PR-comment commands: `review`/`re-review` (dispatches reReviewStoredPullRequest with force:true, bypassing cache without touching Gate/disposition) and `resume` (which also fixes a real latent bug in hasAutoreviewPausedMarker — the old query only checked whether a pause row had ever existed, so resume could authorize and post a confirmation while never actually clearing the paused state). The new query now reads the most-recent of {autoreview_paused, autoreview_resumed} ordered by created_at desc, rowid desc, correctly letting resume supersede an earlier pause while still allowing a later pause to re-pause. Both handlers mirror the existing classify → authorize → dispatch shape used by pause/resolve/explain, and the new tests exercise the real paths (a live PR resync fetch proves reReviewStoredPullRequest actually ran, and pause→resume→pause-again ordering is asserted against the real query) rather than fabricating unreachable states.

Nits — 7 non-blocking
  • The classify → authorize → dispatch boilerplate is now duplicated four times (pause/resolve/review/resume) with only the audit event names and confirmation text differing — consider extracting a shared `dispatchPrCommand` helper now that the pattern is fully established across all four handlers.
  • processors.ts:11094+ uses non-null assertion `payload.issue!` in both new handlers; this mirrors the existing pause/resolve pattern so it's consistent, but worth confirming GitHub always sends `issue` on `issue_comment` events for PR threads (it does for PRs, just flagging the assumption).
  • test/unit/queue.test.ts's `isCurrentlyPaused` test helper duplicates the exact SQL from `hasAutoreviewPausedMarker` rather than importing it — acceptable given the file's existing convention of raw audit_events queries in tests, but a shared test utility would reduce drift risk if the query ever changes again.
  • The review-command confirmation comment text ('Re-running auto-review for this PR...') and resume's ('Auto-review is resumed for this PR...') are hand-written strings in the handler bodies; consider centralizing PR-comment templates the way other command confirmations likely already do, for consistency if wording needs a global update later.
  • Extract the common classify/authorize/audit-skip scaffolding shared by maybeProcessReviewCommand, maybeProcessResumeCommand, maybeProcessPauseCommand, and maybeProcessResolveCommand into a single higher-order dispatcher to cut ~30 duplicated lines per handler.
  • 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.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2163, #2165, #1960
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 (3 linked issues).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 43 merged, 486 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 52 PR(s), 486 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Linked issue satisfaction

Addressed
The diff adds maybeProcessReviewCommand mirroring the existing classify→authorize→dispatch shape, registers it in the comment-dispatch chain, calls the existing reReviewStoredPullRequest path (with force:true) rather than reimplementing review, records review_command_completed/denied/skipped audit and product-usage events, and includes unit tests for authorized dispatch, the re-review alias, unaut

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 52 PR(s), 486 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 8, 2026
@JSONbored
JSONbored merged commit 7d4117c into main Jul 8, 2026
10 checks passed
@JSONbored
JSONbored deleted the feat/review-resume-commands-1960 branch July 8, 2026 09:53
JSONbored added a commit that referenced this pull request Jul 8, 2026
…typecheck (#4259)

Two independently-merged PRs (#4050 and #4175) both implemented the
@gittensory review command, leaving two colliding declarations of
maybeProcessReviewCommand/recordReviewCommandSkip and two dispatch call
sites -- a hard tsc "Duplicate function implementation" error that
blocks every PR's typecheck/validate check on main.

Keeps the earlier, more complete implementation (which also wires the
paired resume command and a real hasAutoreviewPausedMarker fix), and
folds in two things only the later, now-removed duplicate got right:

- needsMinerDetection: true on the authorization call. "review" is
  deliberately widened to confirmed_miner (self-rerun precedent, same
  as review-now), so without this flag a confirmed miner re-triggering
  review on their own PR had no other role to match and was always
  denied.
- A resolveAgentActionMode pause/dry-run gate before dispatching,
  matching every other action command (pause/resolve/explain/
  gate-override/generate-tests). The kept implementation had no such
  gate at all, so review always dispatched live regardless of a
  maintainer's global pause or dry-run setting.

Removes the redundant duplicate test suite for the same command and
adds regression coverage for both fixes above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment