Skip to content

Add GitHub connector and review workflow#31

Merged
pmbstyle merged 6 commits intomainfrom
feat/github-connector
Apr 6, 2026
Merged

Add GitHub connector and review workflow#31
pmbstyle merged 6 commits intomainfrom
feat/github-connector

Conversation

@pmbstyle
Copy link
Copy Markdown
Owner

@pmbstyle pmbstyle commented Apr 6, 2026

Summary

  • add a GitHub connector with CLI auth/configuration, MCP server support, and Octo/worker alias tools
  • rebalance tool budgeting so mandatory and always-include sets stay system-focused while connector tools remain discoverable
  • let workers use connector alias tools and add a high-level github_review_bundle tool for PR review context
  • expand GitHub issue and PR workflows to support comments, reviews, merge-readiness, changed files, commits, and commit comments

Testing

  • uv run pytest tests/test_connector_tools.py tests/test_github_mcp_server.py tests/test_runtime_mcp_integration.py tests/test_connectors.py tests/test_cli_connectors.py
  • full uv run pytest was started and produced no failures before stalling late in the run without additional output

@pmbstyle pmbstyle self-assigned this Apr 6, 2026
Copy link
Copy Markdown
Owner Author

@pmbstyle pmbstyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall: Solid PR. Clean architecture, follows the Google connector pattern, comprehensive MCP server. Ship it.

What looks good 👍

  1. Connector lifecycle mirrors Google perfectly — status/configure/authorize/start/disconnect
  2. CLI refactor — Google-specific prompts moved inside the right branch, --token option for scripting
  3. Error chain in get_status() covers all real failure modes
  4. Pure normalization functions instead of methods — clean separation
  5. Honest docs about what's not supported yet

Concerns (follow-ups, not blockers)

  1. forget_credentials param unused in disconnect() — always clears token regardless of the flag value
  2. Token passed as env var to subprocess — visible via /proc on shared Linux hosts
  3. No scope validationauthorize() checks token validity but doesn't verify scopes match enabled services
  4. First commit is a monolith — future PRs benefit from splitting by layer (connector / MCP server / CLI / config)
  5. No max_results cap on list endpoints — could return massive responses for repos with thousands of issues/PRs

Minor nits

  • Alphabetical ordering in manager.py
  • Service validation in configure.py
  • "Stored credentials" wording fix ✅

Verdict

Ship it. The concerns above are follow-up material, not merge blockers. Nice work 🎉

@pmbstyle pmbstyle merged commit 47f13a8 into main Apr 6, 2026
3 checks passed
@pmbstyle pmbstyle deleted the feat/github-connector branch April 6, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant