Skip to content

mcp(stdio): restore the CurrentBranchInput narrowing on the eight local-branch tools - #10241

Closed
phamngocquy wants to merge 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-10034
Closed

mcp(stdio): restore the CurrentBranchInput narrowing on the eight local-branch tools#10241
phamngocquy wants to merge 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-10034

Conversation

@phamngocquy

Copy link
Copy Markdown
Contributor

Summary

packages/loopover-contract/src/tools/local-branch.ts:106 widens the current-branch family's contract
input, and makes two previously-optional fields required:

export const LocalBranchAnalysisInput = CurrentBranchInput.extend({
  login: z.string().min(1).max(SCENARIO_LIMITS.branchRefChars),
  repoFullName: z.string().min(3).max(SCENARIO_LIMITS.repoFullNameChars),
  baseSha: z.string().min(1).optional(),
  ...

The doc directly above it, at packages/loopover-contract/src/tools/local-branch.ts:101, states the
intended split:

 * The contract is the wider surface, per the rule a server narrows FROM: the remote accepts this whole
 * shape because a caller may supply the metadata itself, and the stdio server narrows to `CurrentBranchInput`
 * because it reads the shas, the diff and the scorer probe off the local checkout instead of taking them
 * from the caller -- serving less, and saying so, rather than advertising a field it ignores.

The stdio server never got that narrowing. registerStdioTool
(packages/loopover-mcp/bin/loopover-mcp.ts:897) advertises and enforces
overrides?.input ?? contract.input at line 923, and none of the eight tools whose contract input is
LocalBranchAnalysisInput passes an override. Only five overrides exist in the whole file
(loopover-mcp.ts:1604, :1618, :1630, :1780, :1893), and none of them is one of these:

tool contract entry stdio registration handler's declared arg type
loopover_preflight_current_branch local-branch.ts:142 loopover-mcp.ts:1661 z.infer<typeof CurrentBranchInput>
loopover_preview_current_branch_score local-branch.ts:155 loopover-mcp.ts:1679 z.infer<typeof CurrentBranchInput>
loopover_rank_local_next_actions local-branch.ts:168 loopover-mcp.ts:1693 z.infer<typeof CurrentBranchInput>
loopover_explain_local_blockers local-branch.ts:181 loopover-mcp.ts:1701 z.infer<typeof CurrentBranchInput>
loopover_remediation_plan local-branch.ts:194 loopover-mcp.ts:1717 z.infer<typeof CurrentBranchInput>
loopover_prepare_pr_packet local-branch.ts:207 loopover-mcp.ts:1727 z.infer<typeof CurrentBranchInput>
loopover_draft_pr_body local-branch.ts:251 (DraftPrBodyInput = LocalBranchAnalysisInput.extend(...)) loopover-mcp.ts:1738 z.infer<typeof DraftPrBodyInput>
loopover_agent_prepare_pr_packet local-branch.ts:222 loopover-mcp.ts:1822 z.infer<typeof CurrentBranchInput>

This is a behaviour regression, not a latent inconsistency. Before the widening,
preflightCurrentBranchTool.input was CurrentBranchInput — verifiable with
git show c3b9a9b1f^:packages/loopover-contract/src/tools/local-branch.ts (line 99: input: CurrentBranchInput).
CurrentBranchInput (local-branch.ts:70) makes both fields optional precisely because
"both servers resolve it themselves -- the stdio server from its persisted session (or LOOPOVER_LOGIN)".

What breaks in practice: loopover_preflight_current_branch with {} — the ordinary "check the branch
I am on" call, and the one the CLI's own guidance recommends at loopover-mcp.ts:432 and :453 — is now
rejected by the MCP SDK with a -32602 for missing login and repoFullName, on a server whose handler
resolves both from the checkout and the active session and does not need either.

Nothing catches it. checkInputNarrowing
(scripts/lib/validate-mcp/invariants.ts) only fails when the ADVERTISED input requires something the
contract does not — here advertised and contract are the same object, so it passes. And
test/contract/validate-mcp.test.ts:100 synthesizes smoke arguments from the advertised schema, so the
validator dutifully supplies login and repoFullName and the call succeeds.

Deliverables

  • A narrowing exported from packages/loopover-contract/src/tools/local-branch.ts, derived from
    LocalBranchAnalysisInput, whose JSON Schema required array contains neither login nor
    repoFullName, and a loopover_draft_pr_body variant of it that still declares format.
  • All eight registerStdioTool call sites in packages/loopover-mcp/bin/loopover-mcp.ts
    (lines 1661, 1679, 1693, 1701, 1717, 1727, 1738, 1822) pass that narrowing as the { input } override.
  • A test in test/unit/contract-registry.test.ts asserting that, for each of the eight tool names,
    the narrowing's required array is a subset of LocalBranchAnalysisInput's and excludes login
    and repoFullName, and that every property the narrowing declares also exists on the contract
    entry's input (so checkInputNarrowing still passes).
  • A regression test at test/unit/mcp-cli-current-branch-input.test.ts (new file) that connects an
    in-process client to the stdio server export (the InMemoryTransport pattern already used in
    test/contract/validate-mcp.test.ts:150), reads tools/list, and asserts that the advertised
    inputSchema.required for all eight tool names contains neither login nor repoFullName
    named for this bug (REGRESSION: the stdio branch tools must not demand a login the CLI resolves itself).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example fixing loopover_preflight_current_branch alone and leaving the other seven, or adding the
narrowing to the contract without wiring it into the registrations — does not resolve this issue.

Test plan

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include covers packages/loopover-contract/src/**/*.ts (line 108) and
packages/loopover-mcp/bin/**/*.ts (line 120), so both touched paths are measured and gated.
The narrowing declarations are plain schema constants with no branches; registerStdioTool's
overrides?.input ?? contract.input (loopover-mcp.ts:923) is an existing ?? whose BOTH arms must be
exercised — the eight new override call sites cover the left arm, and at least one existing
override-free registration must still be asserted to cover the right arm.

Fixes #10034

@phamngocquy
phamngocquy requested a review from JSONbored as a code owner July 31, 2026 13:52
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-31 14:20:26 UTC

4 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

Review summary
This PR wires eight local-branch stdio tool registrations to a new StdioLocalBranchAnalysisInput/StdioDraftPrBodyInput override, restoring the narrower CurrentBranchInput schema documented in local-branch.ts's own header comment but never actually applied at the stdio registration sites. The fix matches the pattern already used elsewhere in the file (e.g. StdioMarkNotificationsReadInput, StdioWatchIssuesInput), and both a runtime MCP client test and a static contract-registry test verify the eight tools no longer require login/repoFullName. The change is narrowly scoped to the documented defect and includes real regression coverage.

Nits — 6 non-blocking
  • test/unit/mcp-cli-current-branch-input.test.ts:31 sets `process.env.LOOPOVER_API_TOKEN = "in-process-token"`, a hardcoded placeholder token flagged by secret scanning — worth an inline comment or a less scanner-triggering name (e.g. `test-token`) to avoid false-positive alerts in future scans.
  • The PR description doesn't explicitly link to an open issue by number in the visible truncated text (only 'mcp(stdio): restore the CurrentBranchInput narrowing on the eight local-branch tools #10034' appears inside a code comment/test), so confirm this closes a maintainer-tracked issue rather than being self-motivated cleanup.
  • packages/loopover-contract/src/tools/local-branch.ts:242-243 and :247-249 — the two new `Stdio*Input` exports have single-line JSDoc but no test co-located in this file confirming they stay in sync with `CurrentBranchInput` if that type changes shape later; the contract-registry test covers this today but relies on the hand-maintained `narrowingByTool` map.
  • Consider asserting in test/unit/contract-registry.test.ts that `StdioLocalBranchAnalysisInput === CurrentBranchInput` (reference equality) rather than only checking property/required overlap, since local-branch.ts:191 defines it as a straight alias — this would catch a future accidental divergence immediately.
  • In loopover-mcp.ts, the eight `{ input: StdioLocalBranchAnalysisInput }` overrides are correct but repeated eight times inline; if more tools are added to this family later, consider a small constant/comment noting the eight call sites so a future contributor doesn't miss one the way this bug arose.
  • Possible secret-shaped assignment in the diff (generic_secret_assignment) — Verify the value is not a real credential.

CI checks failing

  • validate
  • validate-tests

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #10034
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 69 registered-repo PR(s), 19 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor phamngocquy; Gittensor profile; 69 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The PR adds StdioLocalBranchAnalysisInput (= CurrentBranchInput) and StdioDraftPrBodyInput (CurrentBranchInput + format), both derived from existing contract shapes as required, and applies them as input overrides at all eight registerStdioTool sites named in the issue, with tests verifying {} calls no longer trip schema validation for login/repoFullName and that DraftPrBodyInput retains format.

Review context
  • Author: phamngocquy
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 69 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; 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.

Decision record
  • action: close · clause: policy_close:heuristic
  • config: 16a405d30a0b8a99742267a33ef44201ba3c3890d6e020cbca5a9d02a8a9e786 · pack: oss-anti-slop · ci: failed
  • record: b0d369eb9412fe07e2540559e73950ae282db328dd084971e945301c034ee53a (schema v6, head f6eb68c)

🟩 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

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2724 1 2723 0
View the top 1 failed test(s) by shortest run time
test/unit/mcp-cli-current-branch-input.test.ts > REGRESSION: the stdio branch tools must not demand a login the CLI resolves itself > accepts an empty argument object without schema rejection for login or repoFullName
Stack Traces | 17s run time
Error: Already connected to a transport. Call close() before connecting to a new transport, or use a separate Protocol instance per connection.
 ❯ Server.connect node_modules/@.../src/shared/protocol.ts:609:18
 ❯ McpServer.connect node_modules/@.../src/server/mcp.ts:112:33
 ❯ connectClient test/unit/mcp-cli-current-branch-input.test.ts:51:23
 ❯ test/unit/mcp-cli-current-branch-input.test.ts:75:26

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:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 31, 2026
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests)). 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.

@loopover-orb loopover-orb Bot closed this Jul 31, 2026
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.

mcp(stdio): restore the CurrentBranchInput narrowing on the eight local-branch tools

1 participant