MCP GitHub tool surface over local-git backend + branch diff tools - #11
Merged
Conversation
…ckoff - real JSON-RPC 2.0 stdio MCP client (initialize handshake, tools/call, timeouts) — drives any MCP server, e.g. the GitHub MCP server - LocalGitMCP: same tool surface backed by a real local git repo, so the full swarm path runs offline with zero credentials; branches, commits, diffs and merges are real git operations; conflicts throw MERGE_CONFLICT and leave the repo clean — nothing ever force-resolves - one PR per branch (idempotent create) - rate limiter: token bucket + in-flight cap + Retry-After-aware backoff
- get_branch_diff: git diff base...branch without needing a PR number — the reviewer runs before any PR exists - read_file_at_ref: file contents at a branch ref, null when absent - remote backend maps to compare_commits / get_file_contents with arg translation; branch diff normalized to unified-diff text
…mic commits, path safety
- facade: merge/close are structurally impossible without a satisfied T2
approval ({approvalId, decidedBy}); the decision is audited at execution
- remote schema: full arg translation (owner/repo injection, head/base,
pullNumber/issue_number) matching the official GitHub MCP schemas
- remote commits: per-file loop replaced with one atomic push_files batch
(raw content) — no partial branches, no base64/sha bookkeeping errors
- remote diffs: patchless/binary files emit explicit metadata records so an
empty diff can only mean no changes
- local-git: agent paths validated against traversal, absolute escapes, and
symlinks (incl. dangling ones via lstat walk); review diffs fail closed
instead of surfacing git errors as 'no changes'
- client: typed MCPTransportError; spawn 'error' handler registered before
handshake; failed init reaps the child; initialize has its own timeout
- ratelimit: transient transport failures retry with backoff; tool errors do
not; backoff sleeps outside the concurrency slot
- tests: +11 (client fixture server, escape attempts, fail-closed diffs,
gated merge, translation, slot-free backoff) — 58 total
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd MCP GitHub facade with offline local-git backend
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. mergePR bypasses approval gate
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| getPRDiff(number) { return this.call('get_pull_request_diff', { number }, { tier: 'T0' }); } | ||
| getBranchDiff(branch, base = 'main') { return this.call('get_branch_diff', { branch, base }, { tier: 'T0' }); } | ||
| readFileAtRef(branch, filePath) { return this.call('read_file_at_ref', { branch, path: filePath }, { tier: 'T0' }); } | ||
| mergePR(number) { return this.call('merge_pull_request', { number }, { tier: 'T2' }); } |
There was a problem hiding this comment.
1. mergepr bypasses approval gate 📜 Skill insight ⛨ Security
The new public mergePR() helper dispatches a protected merge directly, despite documentation requiring merge-queue invocation only after approval. The tier: 'T2' value is not enforced because call() only rate-limits and dispatches.
Agent Prompt
## Issue description
`GitHubMCP.mergePR()` can execute a merge without validating the required T2 policy decision and approval gate.
## Issue Context
The new facade documents that merges occur only through the merge queue after approval, but `call()` merely dispatches to the selected backend. Ensure merge execution fails closed unless a verified approval authorizes it, and align the merge action name with the policy tier registry.
## Fix Focus Areas
- harness/mcp_clients/github.mjs[74-92]
- harness/mcp_clients/github.mjs[142-142]
- harness/policy/tiers.mjs[47-55]
- harness/gate/approval_gate.mjs[53-99]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| import { spawn } from 'node:child_process'; | ||
|
|
||
| export class MCPClient { |
There was a problem hiding this comment.
2. mcpclient lacks tests 📜 Skill insight ▣ Testability
The new MCPClient source file has no accompanying or existing test coverage; repository search finds it only in its definition and production import. Its JSON-RPC handshake, timeout, parsing, error, and shutdown behavior are therefore unverified.
Agent Prompt
## Issue description
The newly added `MCPClient` implementation is not covered by tests.
## Issue Context
Rule 2905097 requires new source files to have accompanying test coverage. Add tests using a controlled stdio child process to verify initialization, tool calls, protocol errors, malformed output, timeouts, process exits, and close behavior.
## Fix Focus Areas
- harness/mcp_clients/client.mjs[10-141]
- harness/test/mcp.test.mjs[1-153]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+100
| const res = await this.remote.callTool(remoteName, this._translateArgs(tool, args)); | ||
| return JSON.parse(findText(res) ?? 'null'); | ||
| }); | ||
| } | ||
|
|
||
| /** The GitHub MCP server names some args differently than our tool surface. */ | ||
| _translateArgs(tool, args) { | ||
| if (tool === 'read_file_at_ref') { | ||
| return { owner: this.owner, repo: this.repo, path: args.path, ref: args.branch }; | ||
| } | ||
| return args; |
There was a problem hiding this comment.
3. Remote pr schema is wrong 🐞 Bug ≡ Correctness
createPR() sends {branch,title,body} unchanged, omitting owner, repo, and required base
while using branch instead of required head. The official GitHub MCP server therefore rejects
every remote pull-request creation call.
Agent Prompt
## Issue description
Remote facade arguments do not match the official GitHub MCP schemas, causing remote operations such as PR creation to fail validation.
## Issue Context
Inject repository identity and explicitly translate each internal tool's field names and required defaults instead of passing most argument objects through unchanged.
## Fix Focus Areas
- harness/mcp_clients/github.mjs[84-101]
- harness/mcp_clients/github.mjs[133-143]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+123
to
+126
| await this.remote.callTool('create_or_update_file', { | ||
| owner: this.owner, repo: this.repo, path: f.path, branch, message, | ||
| content: encodeBase64(f.content), | ||
| }); |
There was a problem hiding this comment.
4. Remote commits corrupt content 🐞 Bug ≡ Correctness
_remoteCommitFiles() base64-encodes file contents even though the official MCP tool requires raw content and performs encoding itself, so newly created files contain base64 text rather than the requested source. It also omits the blob sha required to update an existing file, making ordinary edits fail.
Agent Prompt
## Issue description
Remote file creation sends encoded text and remote updates omit the required existing blob SHA.
## Issue Context
Use the server's batch file tool where possible; otherwise send raw content and resolve/pass each existing file's SHA before updating it.
## Fix Focus Areas
- harness/mcp_clients/github.mjs[118-129]
- harness/mcp_clients/github.mjs[151-153]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+122
to
+126
| for (const f of files) { | ||
| await this.remote.callTool('create_or_update_file', { | ||
| owner: this.owner, repo: this.repo, path: f.path, branch, message, | ||
| content: encodeBase64(f.content), | ||
| }); |
There was a problem hiding this comment.
5. Multi-file commits are partial 🐞 Bug ☼ Reliability
The remote commit_files implementation performs one persistent MCP mutation per file, so a failure on a later file leaves earlier files committed even though the overall call rejects. Retrying the outer operation then repeats already-applied writes and cannot preserve the local backend's single-commit behavior.
Agent Prompt
## Issue description
A failed remote multi-file commit leaves a partially modified branch and retries previously successful mutations.
## Issue Context
Replace the per-file loop with the GitHub MCP batch/push-files operation, or implement rollback/idempotency so the facade only reports failure without partial persistence.
## Fix Focus Areas
- harness/mcp_clients/github.mjs[118-129]
- harness/mcp_clients/github.mjs[79-92]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+94
to
+98
| for (const f of files) { | ||
| const abs = path.join(this.repoDir, f.path); | ||
| fs.mkdirSync(path.dirname(abs), { recursive: true }); | ||
| fs.writeFileSync(abs, f.content); | ||
| this._git(['add', f.path]); |
There was a problem hiding this comment.
7. File paths escape repository 🐞 Bug ⛨ Security
commit_files() joins untrusted file paths to repoDir without checking containment, allowing ../ paths or in-tree symlinks to overwrite files outside the target repository. The later `git add` failure does not undo the external write.
Agent Prompt
## Issue description
Agent-supplied paths can escape the local repository and overwrite arbitrary filesystem locations.
## Issue Context
Resolve and validate paths against the canonical repository root, reject absolute/traversal paths, and prevent writes through symlinks that leave the tree before creating or writing files.
## Fix Focus Areas
- harness/mcp_clients/local_git.mjs[92-100]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+139
to
+140
| const r = this._git(['diff', `${base}...${branch}`], { allowFail: true }); | ||
| return r.stdout; |
There was a problem hiding this comment.
8. Diff failures look clean 🐞 Bug ≡ Correctness
Both local diff methods suppress every Git failure and return stdout without checking status, so a missing branch, malformed ref, or repository error is reported as an empty diff. The review stage can consequently treat an unreadable branch as having no changes.
Agent Prompt
## Issue description
Failed Git diff commands are indistinguishable from valid empty diffs.
## Issue Context
Do not use `allowFail` for review diffs; validate refs and propagate stderr/status so reviewers fail closed when changes cannot be read.
## Fix Focus Areas
- harness/mcp_clients/local_git.mjs[130-140]
- harness/mcp_clients/local_git.mjs[182-190]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+48
to
+52
| } catch (err) { | ||
| lastErr = err; | ||
| if (!this._isRateLimit(err)) throw err; | ||
| const waitMs = this._retryAfterMs(err) ?? 2 ** attempt * 1000; | ||
| await sleep(waitMs); |
There was a problem hiding this comment.
9. Transport failures never retry 🐞 Bug ☼ Reliability
Despite the advertised retry-on-transport-failure behavior, RateLimiter.run() retries only errors whose message looks like an HTTP rate limit. MCP timeouts, server exits, and other transport failures from MCPClient are immediately rethrown on the first attempt.
Agent Prompt
## Issue description
MCP transport failures bypass the newly added retry/backoff mechanism.
## Issue Context
Introduce typed transport errors or error codes in MCPClient and retry only safe, transient transport failures with bounded backoff; preserve non-retryable tool errors.
## Fix Focus Areas
- harness/mcp_clients/client.mjs[40-40]
- harness/mcp_clients/client.mjs[118-131]
- harness/mcp_clients/ratelimit.mjs[41-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+40
| this._proc.stdout.on('data', (d) => this._onStdout(d)); | ||
| this._proc.stderr.on('data', (d) => process.stderr.write(`[mcp] ${d}`)); | ||
| this._proc.on('exit', (code) => this._rejectAll(new Error(`MCP server exited (${code})`))); |
There was a problem hiding this comment.
10. Spawn errors crash client 🐞 Bug ☼ Reliability
connect() never registers a ChildProcess error listener, so an invalid or unavailable MCP command emits an unhandled error event instead of rejecting connect() cleanly. The spawned-process lifecycle also receives no cleanup when initialization rejects or times out.
Agent Prompt
## Issue description
MCP server spawn failures can terminate the Node process, and failed initialization can leak the child.
## Issue Context
Attach an error handler before starting initialization, reject pending RPCs with that error, and close/kill/reset the process in a catch path around initialization.
## Fix Focus Areas
- harness/mcp_clients/client.mjs[33-48]
- harness/mcp_clients/client.mjs[69-81]
- harness/mcp_clients/client.mjs[110-116]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+50
to
+55
| if (!this._isRateLimit(err)) throw err; | ||
| const waitMs = this._retryAfterMs(err) ?? 2 ** attempt * 1000; | ||
| await sleep(waitMs); | ||
| } finally { | ||
| this._release(); | ||
| } |
There was a problem hiding this comment.
11. Backoff blocks unrelated calls 🐞 Bug ➹ Performance
A rate-limited attempt sleeps before its finally releases the in-flight slot, so enough long Retry-After responses occupy every slot and prevent unrelated operations from starting. With the default capacity, 30 sleeping retries stall the entire facade for the full server-provided delay.
Agent Prompt
## Issue description
Retry backoff incorrectly counts sleeping operations as active in-flight requests.
## Issue Context
Release the concurrency slot immediately after each attempt and perform backoff outside that slot before reacquiring for the next attempt.
## Fix Focus Areas
- harness/mcp_clients/ratelimit.mjs[19-33]
- harness/mcp_clients/ratelimit.mjs[41-57]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
All findings addressed (commit b2bc26e):
58/58 tests green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2
What
harness/mcp/): list issues, read repo, branch, commit, open PR — spoken over the Model Context Protocol so any MCP-aware agent can drive Foreman's repo operations.Why
Tool calls are only trustworthy if their surface is small and typed. Routing every GitHub interaction through one MCP server means the policy engine classifies actions in exactly one place, and the local-git adapter keeps the whole harness testable without touching github.com.
Test plan
7 new tests (47 total green): tool contract parity between backends, diff/read-at-ref correctness, retry/backoff behavior.