Skip to content

feat(mcp): register plan-DAG tools + local scorer in packages/loopover-mcp - #6490

Closed
galuis116 wants to merge 3 commits into
JSONbored:mainfrom
galuis116:feat/mcp-register-plan-dag-and-scorer-tools
Closed

feat(mcp): register plan-DAG tools + local scorer in packages/loopover-mcp#6490
galuis116 wants to merge 3 commits into
JSONbored:mainfrom
galuis116:feat/mcp-register-plan-dag-and-scorer-tools

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Closes #6150

Summary

src/mcp/server.ts registers loopover_run_local_scorer, loopover_build_plan, loopover_plan_status, loopover_record_step_result, and loopover_predict_gate on the remote server, and packages/loopover-mcp/bin/loopover-mcp.js's miner-auto-dev profile listed all five in recommendedTools — but none were actually registered as local stdio tools, only the string literals existed. A contributor relying on the local server for this profile couldn't invoke any of them.

  • loopover_run_local_scorer: computeLocalScorerTokens imported directly from @loopover/engine (already exported at the package root) — same pattern as the existing loopover_check_slop_risk/loopover_lint_pr_text pure in-process tools. Pure, deterministic, no repo/network access.
  • loopover_build_plan / loopover_plan_status / loopover_record_step_result: the plan-DAG state machine (src/services/plan-dag.ts) was never extracted to @loopover/engine's export map, so there's nothing to import — hand-duplicated here following the exact same precedent this file already uses for MAINTAIN_ACTION_CLASSES/AUTONOMY_LEVELS when the published package's export map doesn't cover something. Pure + stateless (no DB, no network) — the harness runs each step and calls loopover_record_step_result to report it back.
  • loopover_predict_gate: cannot be pure-local — it needs live repo/issue/PR/manifest data only the server can assemble (env.DB-backed). Proxies to the existing POST /v1/local/branch-analysis route, which already computes predictedGate via buildPredictedGateVerdict (the identical logic the remote tool uses) and returns it as a top-level response field — no new backend endpoint needed. Uses a metadata-only input shape (no git/workspace context), unlike the sibling branch-analysis tools that shell out to git.

Note on re-open

This resubmits #6462, which was auto-closed by red CI. That failure was two pre-existing, unrelated regressions in main at the time — not caused by this diff — both now fixed as part of this branch (see below) and rebased onto current main:

  1. test/unit/mcp-tool-rename-aliases.test.ts hardcodes the exact count of registered loopover_-prefixed stdio tools as a regression guard; this PR's 5 new tools take that count from 55 to 60, so the guard's three hardcoded assertions needed updating (own diff's consequence — this part was already in feat(mcp): register plan-DAG tools + local scorer in packages/loopover-mcp #6462).
  2. test/unit/backfill.test.ts had two tests that create a fixture repo literally named JSONbored/gittensory (the default self-repo identity createTestEnv() uses) without mocking fetch. A very recent, unrelated main commit (feat(agent): autonomy-levels framework (observe→…→auto) #773) gave the real JSONbored/gittensory repo's live .loopover.yml genuine autonomy: { merge: auto, ... } config — so these two tests' un-mocked fetch calls now hit that real, live manifest over the network and inherit its autonomy instead of the DB-only settings they're meant to test. Confirmed unrelated to this PR by reproducing the identical failure in an isolated git worktree of clean upstream/main. Fixed by mocking fetch (matching this file's own established pattern) and scoping the fixture's self-repo override away from the real repo name, same as several sibling tests in the same file already do.

Incidental fix

While testing, found packages/loopover-mcp/node_modules/@loopover/engine was a stale, non-symlinked directory shadowing the correct root-level workspace symlink, breaking the CLI's own @loopover/engine/signals/slop etc. subpath imports — confirmed pre-existing and unrelated to this change via git stash comparison against a clean checkout. Removed it; the root symlink resolves correctly.

Scope

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck (root) — reliably OOMs on this shared sandbox regardless of what changed (reproduced repeatedly this session). packages/loopover-mcp is plain JS with its own npm run build (node --check across every lib/bin file) — ran it directly and it passes clean, and confirmed via direct execution that loopover-mcp --help and loopover-mcp tools --json (listing all 60 tools, count verified) both run without error.
  • npm run test:coverage — not run repo-wide (same OOM risk). Ran the full MCP CLI test suite (30 files, 230 tests) plus mcp-tool-rename-aliases.test.ts and backfill.test.ts in full (137 tests) after rebasing onto current main — all passing. test/unit/mcp-cli-plan-scorer-tools.test.ts (15 new tests) covers registration + success/rejection paths for all 5 tools, including the API-failure path for loopover_predict_gate.
  • npm run test:workers — N/A, no Worker-facing code changed (this is the local CLI, not src/).
  • npm run build:mcp / npm run test:mcp-pack — both run directly and pass clean.
  • npm run ui:openapi:check / ui:lint / ui:typecheck / ui:build — N/A, no apps/loopover-ui changes.
  • npm audit --audit-level=moderate — 0 vulnerabilities.
  • New/changed behavior has tests — 15 new tests covering all 5 tools' success paths, zod-rejection paths, and (for the HTTP-backed tool) an API-failure path via a new localBranchAnalysisStatus fixture-server option added to test/unit/support/mcp-cli-harness.ts, mirroring the existing intakeStatus pattern.

If any required check was skipped, explain why:

  • Root npm run typecheck / npm run test:coverage: reliably OOMs on this shared sandbox under memory pressure from concurrent sessions, independent of the diff. Substituted with packages/loopover-mcp's own build (clean), direct CLI execution confirming all 5 tools register and respond correctly, and the broader test suites listed above, all passing.

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 changes; the one HTTP-backed tool does have a negative-path (API-failure) test.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — New local MCP tools added and tested; no new backend API surface (reuses the existing /v1/local/branch-analysis route).
  • 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 below with screenshots. — N/A, no visible UI change (CLI tool registration only).
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. — CHANGELOG.md untouched.

Notes

  • loopover_build_plan/loopover_plan_status/loopover_record_step_result's hand-duplicated plan-DAG logic in loopover-mcp.js is a deliberate architectural choice, not an oversight: this file already documents (in the MAINTAIN_ACTION_CLASSES/AUTONOMY_LEVELS comment block) that it resolves @loopover/engine through the published package, whose export map exposes only a curated set of subpaths — widening that public API is a separate, larger decision than "register these 5 tools locally," so this follows the existing precedent rather than introducing a new one.

…r-mcp

The miner-auto-dev profile's recommendedTools listed
loopover_run_local_scorer/loopover_build_plan/loopover_plan_status/
loopover_record_step_result/loopover_predict_gate, but none were
registered as local stdio tools -- only the string literals existed.

- loopover_run_local_scorer: computeLocalScorerTokens imported
  directly from @loopover/engine (already exported), same pattern as
  the existing loopover_check_slop_risk/loopover_lint_pr_text pure
  in-process tools.
- loopover_build_plan / loopover_plan_status /
  loopover_record_step_result: the plan-DAG state machine
  (src/services/plan-dag.ts) was never extracted to @loopover/engine's
  export map, so it's hand-duplicated here following the same
  MAINTAIN_ACTION_CLASSES/AUTONOMY_LEVELS precedent this file already
  uses for exactly this situation. Pure + stateless -- no DB, no
  network access.
- loopover_predict_gate: cannot be pure-local (needs live repo/issue/
  PR/manifest data only the server can assemble). Proxies to the
  existing POST /v1/local/branch-analysis route, which already
  computes predictedGate via buildPredictedGateVerdict -- the same
  logic the remote tool uses -- and returns it as a top-level field.
  No new backend endpoint needed. Metadata-only input (no git
  required), unlike the branch-analysis tools that shell out to git.

Along the way, found and fixed a stale, non-symlinked
packages/loopover-mcp/node_modules/@loopover/engine directory
shadowing the correct root-level workspace symlink, which was
breaking the CLI's own subpath imports (unrelated to this change --
confirmed pre-existing via git stash).

Added test/unit/mcp-cli-plan-scorer-tools.test.ts (15 tests) covering
all 5 tools' success + rejection paths, and a
localBranchAnalysisStatus fixture-server option +
predictedGate field on localBranchAnalysisFixture in
test/unit/support/mcp-cli-harness.ts to test loopover_predict_gate's
API-failure path, mirroring the existing intakeStatus pattern.

Closes JSONbored#6150
JSONbored#6150 registered loopover_run_local_scorer, loopover_build_plan,
loopover_plan_status, loopover_record_step_result, and
loopover_predict_gate, taking the total loopover_-prefixed stdio tool
count from 55 to 60. mcp-tool-rename-aliases.test.ts hardcodes this
count as a regression guard against silent alias/registration drift;
update it to match.
Both tests create a fake repo literally named "JSONbored/gittensory"
(the default self-repo identity test/helpers/d1.ts's createTestEnv()
uses) but never mock fetch, so the repo-settings resolver's manifest
loader fell through to a REAL network request to the real, live
JSONbored/gittensory GitHub repo's .loopover.yml. That real manifest
now carries autonomy: { merge: auto, ... } (JSONbored#773),
so the live-fetched content silently overrode the DB-only settings
these two tests exist to verify -- unrelated to and unaffected by this
branch's own diff, confirmed by reproducing the identical failure via
`git stash` and again in a clean upstream/main worktree.

Mock fetch to 404 (matching this file's established pattern for
network-adjacent tests) and keep the LOOPOVER_DRIFT_ISSUE_REPO
override the sibling tests in this file already use, so the fixture
repo name no longer collides with the real self-repo's live config or
its bundled fallback.
@galuis116
galuis116 requested a review from JSONbored as a code owner July 16, 2026 11:04
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2336 1 2335 0
View the top 1 failed test(s) by shortest run time
test/unit/check-migrations-script.test.ts > check-migrations script > reports every grandfathered duplicate migration number in the success summary
Stack Traces | 1.04s run time
Error: Command failed: .../loopover/node_modules/.bin/tsx scripts/check-migrations.mjs
check-migrations: duplicate migration number 0156: "0156_draft_pr_close_policy.sql", "0156_pull_request_screenshot_table_presence_satisfied.sql". Two PRs grabbed the same number — renumber the newest to the next free number (0157).

 ❯ test/unit/check-migrations-script.test.ts:38:20

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { status: 1, signal: null, output: [ null, '', 'check-migrations: duplicate migration number 0156: "0156_draft_pr_close_policy.sql", "0156_pull_request_screenshot_table_presence_satisfied.sql". Two PRs grabbed the same number — renumber the newest to the next free number (0157).\n' ], pid: 4269, stdout: '', stderr: 'check-migrations: duplicate migration number 0156: "0156_draft_pr_close_policy.sql", "0156_pull_request_screenshot_table_presence_satisfied.sql". Two PRs grabbed the same number — renumber the newest to the next free number (0157).\n' }

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

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

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-16 11:11:38 UTC

5 files · 1 AI reviewer · 1 blocker · CI failing · blocked

🛑 Suggested Action - Reject/Close

Review summary
This PR registers the five previously-orphaned MCP tool names (loopover_run_local_scorer, loopover_build_plan, loopover_plan_status, loopover_record_step_result, loopover_predict_gate) on the local stdio server, faithfully mirroring src/services/plan-dag.ts's exported functions and adding solid zod-schema-validated tests for each (including cycle detection, retry-exhaustion, and API-failure paths). The plan-DAG duplication is a defensible workaround given the stated export-map constraint and matches an established precedent in this file (MAINTAIN_ACTION_CLASSES). The diff also folds in unrelated changes to test/unit/backfill.test.ts (env var + fetch stubbing) that have nothing to do with the stated #6150 scope and aren't mentioned in the PR description.

Blockers

Nits — 5 non-blocking
  • packages/loopover-mcp/bin/loopover-mcp.js's loopover_predict_gate handler assumes /v1/local/branch-analysis already returns a top-level predictedGate field computed via buildPredictedGateVerdict, but src/mcp/server.ts isn't in this diff — confirm the live route actually returns that field before treating this as a no-backend-change proxy.
  • The plan-DAG logic in loopover-mcp.js (buildPlanDag/validatePlanDag/nextReadySteps/applyStepResult/planProgress) is a line-for-line hand duplication of src/services/plan-dag.ts; two copies of a stateful cycle-detection algorithm will silently drift if one is patched and not the other — worth a follow-up to extract this into @​loopover/engine's export map rather than leaving it duplicated indefinitely.
  • validatePlanDag's cycle-detection closure (loopover-mcp.js:134) reaches ~16 branches / depth 5, exceeding this file's usual complexity — no functional issue since it mirrors the source module, but it's a readability outlier worth a short explanatory comment if it isn't going to be extracted.
  • Numerous new magic-number length caps (400, 500, 50, 300, 60, 2000, 40000, etc.) across the new zod shapes have no named constants or comments explaining the chosen limits — a couple of inline comments on the less-obvious ones (e.g. why 40000 for body, why 500 for changedFiles) would help future maintainers.
  • Split the backfill.test.ts fetch-stubbing changes into a separate PR with its own issue link, since they're orthogonal to the plan-DAG/local-scorer tool registration this PR claims to deliver.

Why this is blocked

📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. test/unit/backfill.test.ts's LOOPOVER\_DRIFT\_ISSUE\_REPO override \+ vi.stubGlobal\("fetch", …\) changes are unrelated to the stated \#6150 scope \(MCP tool registration\) and are not mentioned anywhere in the PR description — this is undisclosed scope creep bundled into a feature PR and should be split into its own PR tied to its own issue.

CI checks failing

  • validate
  • validate-code
  • validate-tests (6)
  • validate-tests (2)
  • validate-tests (5)

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6150
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High 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: 1937 registered-repo PR(s), 1276 merged, 54 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1937 PR(s), 54 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff registers all five tools (loopover_run_local_scorer, loopover_build_plan, loopover_plan_status, loopover_record_step_result, loopover_predict_gate) via registerStdioTool in packages/loopover-mcp/bin/loopover-mcp.js, reusing the existing apiPost/local server conventions and mirroring the remote server's schemas, and adds dedicated tests covering success and failure paths for each tool plus

Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, Python, Dart, TypeScript, HTML, MDX, Rust, C++
  • Official Gittensor activity: 1937 PR(s), 54 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • 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.

🟩 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

@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-code, validate-tests (6), validate-tests (2), validate-tests (5)); AI reviewers agree on a likely critical defect: test/unit/backfill.test.ts's LOOPOVER_DRIFT_ISSUE_REPO override + vi.stubGlobal("fetch", …) changes are unrelated to the stated #6150 scope (MCP tool registration) and are not mentioned anywhere in the PR description — this is undisclosed scope creep bundled into a feature PR and should be split into its own PR tied to its own issue.). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(mcp): register plan-DAG tools + local scorer (build_plan/plan_status/record_step_result/run_local_scorer/predict_gate) in packages/loopover-mcp

1 participant