fix(scm): restore GitLab inline posting after the bin/ launcher removal - #111
Merged
Conversation
#77 consolidated the bin/ launchers and deleted bin/mcp-upstream-gitlab and bin/mcp-upstream-github, but left mcp.DEFAULT_MCP_SERVER and scm.github._GITHUB_MCP_SERVER pointing at the deleted wrappers. GitLabProvider.post_inline_comment called mcp.call_tool with no try/except, so the missing launcher raised FileNotFoundError (an OSError) from the subprocess spawn BEFORE the REST fallback — crashing every real (non-dry-run) GitLab inline finding post since v0.13.0. GitHub survived only because its post path already caught OSError and fell back to REST. - scm/gitlab.py: wrap the MCP call in try/except (RuntimeError, TimeoutError, OSError) and fall back to REST, mirroring the GitHub provider. - mcp.py / scm/github.py: repoint the dead launcher constants at the real dispatcher as argv lists ([.../bin/bubo, mcp-upstream, <provider>]) so MCP posting works again when the upstream server is installed; call_tool now accepts a command list (argv), avoiding shell-split space-in-path issues. - tests/test_gitlab_provider.py: pin the regression — REST recovery on a FileNotFoundError/RuntimeError MCP failure, MCP preferred when it returns an id, and the default targets the dispatcher. dry_run reviews were unaffected (findings are planned, not posted), which is why default-config runs and the suite never surfaced it.
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
1 participant
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.
Severity: production regression (since v0.13.0 / #77)
Found while converging the gaps left in PRs merged yesterday/today.
#77 consolidated
bin/and deletedbin/mcp-upstream-gitlab+bin/mcp-upstream-github, but the Python posting path still referenced them:mcp.DEFAULT_MCP_SERVERandscm/github.py:_GITHUB_MCP_SERVERpointed at the deleted wrappers.GitLabProvider.post_inline_commentcalledmcp.call_tool(...)with no try/except →call_tooldoesPopen([deleted_path])→ uncaughtFileNotFoundErrorbefore the REST fallback.Result: every real (non-
dry_run) GitLab inline finding post crashed and the review recordedFAILED. GitLab is the default provider. GitHub survived only because its post path already caughtOSError→ REST.dry_run(the default) plans findings instead of posting, so default-config runs and the unit suite (which mockscall_tool) never surfaced it.Fix
scm/gitlab.py— wrap the MCP call intry/except (RuntimeError, TimeoutError, OSError)and fall back to REST, mirroring the GitHub provider. (restores posting — the critical fix)mcp.py/scm/github.py— repoint the dead constants at the real dispatcher as argv lists ([.../bin/bubo, mcp-upstream, <provider>]);call_toolnow accepts a command list, so MCP posting works again when the upstream server is installed, and there's no shell-split space-in-path risk.tests/test_gitlab_provider.py(new) — pins the regression: REST recovery onFileNotFoundError/RuntimeError, MCP preferred when it returns an id, and a guard that the default targets the dispatcher (not a deleted wrapper).Verification
ruff+mypyclean on all changed files.