Skip to content

fix(commands): resolve duplicate @gittensory review handler breaking typecheck - #4259

Merged
JSONbored merged 1 commit into
mainfrom
fix/duplicate-review-command-handler
Jul 8, 2026
Merged

fix(commands): resolve duplicate @gittensory review handler breaking typecheck#4259
JSONbored merged 1 commit into
mainfrom
fix/duplicate-review-command-handler

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • main is currently red for tsc --noEmit: two independently-merged PRs (feat(commands): wire @gittensory review / re-review command (#2163) #4050 and feat(commands): wire @gittensory review and resume PR-comment commands #4175) both implemented the @gittensory review command, leaving two colliding declarations of maybeProcessReviewCommand/recordReviewCommandSkip and two dispatch call sites in src/queue/processors.ts — a hard TS2393: Duplicate function implementation error. This blocks the typecheck step of every PR's validate check right now, confirmed via a clean npm ci + npm run typecheck on a fresh checkout of current main.
  • Keeps the earlier, more complete implementation (feat(commands): wire @gittensory review and resume PR-comment commands #4175 — it also wires the paired resume command and a real hasAutoreviewPausedMarker bug fix the other PR never touched), and removes the later duplicate (feat(commands): wire @gittensory review / re-review command (#2163) #4050) entirely, along with its now-redundant duplicate test suite for the same command.
  • Folds in two things the removed duplicate actually got right that the kept implementation was missing:
    • needsMinerDetection: true on the authorization call. DEFAULT_COMMAND_AUTHORIZATION_POLICY deliberately widens review to confirmed_miner (the same self-rerun precedent already applied to review-now), but without this flag a confirmed miner re-triggering review on their own PR had no other role to match and was always wrongly 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 @gittensory review always dispatched live regardless of a maintainer's global pause or dry-run setting.
  • Adds regression coverage for both fixes above (a confirmed-miner-authorized self-rerun test, and an agentPaused/agentDryRun skip test).

Closes #2163 (no functional change to what #2163 asked for — this only removes the accidental duplicate and restores the two missing correctness gates).

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • 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.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck — clean (was failing with 4 TS2393 errors before this fix, confirmed on a fresh npm ci checkout of current main)
  • npm run test:coverage locally (unsharded, in an isolated worktree with a fresh npm ci) — full test/unit/queue.test.ts suite (739 tests) passes; every changed/new line in this diff is covered.
  • 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 — the confirmed-miner authorization branch and the pause/dry-run gate branch both had zero coverage from either original PR; both are covered now.

Also ran: npm run db:migrations:check, npm run db:schema-drift:check, npm run selfhost:env-reference:check, npm run selfhost:validate-observability, npm run cf-typegen:check, npm run test:engine-parity, the @jsonbored/gittensory-engine package's own tests, npm run build:miner, npm run test:miner-pack, npm run rees:test, npm run ui:openapi:settings-parity, npm run ui:version-audit, npm run docs:drift-check, npm run manifest:drift-check, npm run command-reference:check, npm run ui:test.

Note on unsharded coverage: one unrelated pre-existing failure surfaced during the full run — test/unit/mcp-cli-tools.test.ts's tool-count assertion (33 vs 34) — reproduced on a clean checkout of main before this fix, so it is a separate, already-broken drift unrelated to this diff. Flagged separately rather than bundled into this urgent fix.

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 (the confirmed-miner authorization path, specifically).
  • API/OpenAPI/MCP behavior is updated and tested where needed (no OpenAPI/MCP surface changed by this PR).
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks (no UI changed).
  • UI Evidence — not applicable, no visible UI/frontend/docs/extension change in this PR.
  • Public docs/changelogs are updated where needed (none needed); changelog itself is untouched.

Notes

This is a merge-collision cleanup, not new functionality — flagging for expedited review given main is currently red for every other open PR's typecheck check until this lands.

…typecheck

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.
@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 8, 2026
@JSONbored
JSONbored merged commit f498e70 into main Jul 8, 2026
7 checks passed
@JSONbored
JSONbored deleted the fix/duplicate-review-command-handler branch July 8, 2026 20:05
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 8, 2026
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@e1bc7f7). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4259   +/-   ##
=======================================
  Coverage        ?   93.80%           
=======================================
  Files           ?      398           
  Lines           ?    36768           
  Branches        ?    13437           
=======================================
  Hits            ?    34492           
  Misses          ?     1622           
  Partials        ?      654           
Files with missing lines Coverage Δ
src/queue/processors.ts 95.37% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(commands): @gittensory review / re-review command — parse + classify + dispatch to existing re-review action

1 participant