feat: add hspt, sfdc, and cr tools; fix gro cask; add CI#7
Conversation
- Add hubspot-cli (hspt), salesforce-cli (sfdc), and codereview-cli (cr) to CLI_CONFIG with generic MCP tools and a salesforce_query wrapper - Fix gro cask name: the tap has no 'gro' cask; it is 'google-readonly', so install/update tooling for gro was broken - Add GitHub Actions CI workflow running pytest on Python 3.11-3.13 (closes #4) - Sync README and CLAUDE.md tool tables; normalize brew install commands; note cpm is excluded as an interactive TUI
|
⏱️ Automated review timed out after 15 minutes, so no review was posted. Ways to get this PR reviewed:
|
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 3ebf6aa2f834
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| mcp:cli-wrapper-safety | 1 |
| automation:ci-release | 0 |
| documentation:docs | 0 |
mcp:cli-wrapper-safety (1 finding)
Minor - server.py:420
salesforce_query builds its args via f'query "{soql}" --output json' and then re-parses that string with shlex.split inside salesforce_cli. If the SOQL text contains a double quote (e.g. a string literal like WHERE Name = 'O"Brien' or a caller-supplied value with embedded quotes), shlex.split will mis-tokenize the string, either raising a ValueError or, worse, splitting the query into extra shell-style tokens that get passed as separate argv entries to the sfdc binary — letting query content smuggle in additional CLI flags. This mirrors the pre-existing gmail_search/drive_search/slack_search_messages pattern, but it's the new instance introduced by this PR, so it's a good point to fix instead of propagate. Prefer building the argv list directly (e.g. [config["path"], "query", soql, "--output", "json"]) or shlex.quote the interpolated value before handing it back through shlex.split.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| mcp:cli-wrapper-safety | complete_broad | CLAUDE.md, README.md, server.py, test_server.py | unavailable | unavailable |
| automation:ci-release | complete_broad | .github/workflows/ci.yml | unavailable | unavailable |
| documentation:docs | complete_broad | CLAUDE.md, README.md | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 52s | unavailable | claude-sonnet-5 | cr 0.10.257
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | mcp:cli-wrapper-safety, automation:ci-release, documentation:docs |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 52s wall · 1m 54s compute |
| Cost | unavailable |
| Tokens | 38 in / 6.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 1.5k | 30.4k | 18.9k | unavailable | 20s |
| mcp:cli-wrapper-safety | claude-sonnet-5 | 10 | 2.3k | 107.0k | 64.0k | unavailable | 35s |
| automation:ci-release | claude-sonnet-5 | 10 | 1.2k | 94.2k | 22.5k | unavailable | 19s |
| documentation:docs | claude-sonnet-5 | 8 | 1.3k | 71.9k | 25.9k | unavailable | 26s |
| orchestrator-rollup | claude-sonnet-5 | 4 | 366 | 12.7k | 27.7k | unavailable | 13s |
| @mcp.tool() | ||
| def salesforce_query(soql: str) -> str: | ||
| """Run a SOQL query against Salesforce (e.g., "SELECT Id, Name FROM Account LIMIT 10").""" | ||
| return salesforce_cli(f'query "{soql}" --output json') |
There was a problem hiding this comment.
salesforce_query builds its args via f'query "{soql}" --output json' and then re-parses that string with shlex.split inside salesforce_cli. If the SOQL text contains a double quote (e.g. a string literal like WHERE Name = 'O"Brien' or a caller-supplied value with embedded quotes), shlex.split will mis-tokenize the string, either raising a ValueError or, worse, splitting the query into extra shell-style tokens that get passed as separate argv entries to the sfdc binary — letting query content smuggle in additional CLI flags. This mirrors the pre-existing gmail_search/drive_search/slack_search_messages pattern, but it's the new instance introduced by this PR, so it's a good point to fix instead of propagate. Prefer building the argv list directly (e.g. [config["path"], "query", soql, "--output", "json"]) or shlex.quote the interpolated value before handing it back through shlex.split.
Reply inline to this comment.
2 participants
Summary
Brings the MCP server up to date with the current state of the collective:
hspt(hubspot-cli),sfdc(salesforce-cli), andcr(codereview-cli) toCLI_CONFIG, each with a generic*_cli(args)MCP tool, plus asalesforce_query(soql)convenience wrapper.crgets a 600s subprocess timeout since live reviews run an LLM loop.gro's cask was configured asopen-cli-collective/tap/gro, but the tap has nogrocask — it'sgoogle-readonly.install_missing_tools/update_tools/ status checks for gro were broken.newrelic-cli; it'snrq), brew install commands normalized to--cask, and a note thatcpmis deliberately excluded because it's an interactive TUI.Not included
cpm— interactive TUI, can't run as a non-interactive MCP subprocess.Known issue elsewhere
The
hubspot-clicask in the tap is currently broken: its.goreleaser.ymlhomebrew_casksentry omitsbinary: hspt, so the generated cask declaresbinary "hubspot-cli"while the tarball shipshspt. Filing separately against hubspot-cli.Testing
pytest test_server.py— 10 passed (includes new shlex coverage for the three new tools and thesalesforce_querywrapper).