🧪 Add MCP and RPC target implementations#63
Conversation
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces support for MCP analyzer targets as an alternative to existing analyzers (kantra, tackle-hub, vscode, etc.). It adds MCP configuration schema, factory-pattern support, a new MCPTarget implementation, and updates rule handling from simple strings to structured CustomRule types with Git/File distinction. CI workflow enhanced with unit test job; dependencies bumped. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
JonahSussman
changed the title
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/target_config.go (1)
1-154:⚠️ Potential issue | 🔴 CriticalCritical:
pkg/configpackage no longer compiles —git_url_test.gowas not migrated to[]CustomRule.The test file
pkg/config/git_url_test.gohas compilation errors that prevent the entire repository from building:
Lines 127, 144, 155:
Rulesfield initialized with[]string{…}but now expects[]CustomRulewith either{File: &config.FilePathRule{FilePath: "…"}}for local paths or{Git: &config.GitRepoRule{GitRepo: "…"}}for Git repositories.Lines 195–197: References to
ac.RulesGitComponentswhich no longer exists. TheAnalysisConfigstruct only hasApplicationGitComponents; rule parsing is not implemented in the currentParseGitURLs()method.Migrate using the same pattern applied to
pkg/targets/kantra_test.goandpkg/targets/tackle_hub_test.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/target_config.go` around lines 1 - 154, The test git_url_test.go fails because the AnalysisConfig.Rules field is still being initialized as []string but the code now expects []CustomRule; update the test to construct rules as []config.CustomRule using File: &config.FilePathRule{FilePath: "..."} for local paths and Git: &config.GitRepoRule{GitRepo: "..."} for git entries (mirror the pattern used in pkg/targets/kantra_test.go and pkg/targets/tackle_hub_test.go). Also remove or replace any uses of ac.RulesGitComponents (e.g., lines referencing RulesGitComponents) to use the current fields — use ac.ApplicationGitComponents where appropriate and ensure ParseGitURLs() is exercised against ApplicationGitComponents or adapted to parse rules if you add rule parsing. Update git_url_test.go so all Rules initializations and assertions match the new CustomRule types and existing ParseGitURLs()/ApplicationGitComponents API.
🧹 Nitpick comments (5)
.github/workflows/permerge-ci.yaml (1)
14-22: Consider enabling Go module caching to speed up the job.
actions/setup-go@v5enables module/build caching by default, but it keys offgo.sum. Since this repo has ago.sumat root that should work out of the box — just worth confirming the cache hits show up in the Actions run, otherwise addcache: trueexplicitly. No behavior change needed otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/permerge-ci.yaml around lines 14 - 22, The unit-test workflow currently uses actions/setup-go@v5 but doesn't explicitly enable module caching; update the unit-test job to ensure Go module cache is enabled (either confirm that the existing go.sum is used or set cache: true on the actions/setup-go@v5 step) so dependencies are cached across runs and the Run unit tests step (go test -short -timeout 5m ./...) benefits from faster setup.pkg/targets/tackle_hub_test.go (1)
691-691: Pre-existing typo:GetCompentsshould beGetComponents.Not introduced by this PR (it's defined on
GitRepoRuleinpkg/config/types.go), but since this PR is the first place new code is calling it andkantra_test.gopicks it up too, it may be a good moment to rename the method in a follow-up. Purely cosmetic — no behavior change.Also applies to: 707-707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/tackle_hub_test.go` at line 691, Rename the misspelled method GetCompents to GetComponents on the GitRepoRule type and update all call sites (e.g., the call in the tests referencing analysis.Rules[0].Git.GetCompents and the occurrences in kantra_test.go) to use GetComponents; ensure you update the method declaration in the GitRepoRule type (pkg/config/types.go), any implementing types or interfaces, and imports/uses so the compiler sees the new name everywhere.pkg/config/target_config.go (2)
125-132: Document stdio/http mutual exclusivity and consider secret handling forToken.Two things:
- The mutual-exclusivity/required rules between
BinaryPathandEndpointare enforced inNewMCPTarget(perpkg/targets/mcp_test.go) but are not reflected in structvalidate:tags or in this comment. Either add a brief doc comment describing the two modes (stdio viaBinaryPath/ProviderConfig/Rulesvs http viaEndpoint/Token) or use validator'srequired_without/excluded_withtags soLoadTargetConfigcatches misconfiguration consistently with other config types.Tokenis read from plain YAML on disk. Consider supporting env-var substitution (or documenting that callers should expand env vars before loading) so users can avoid committing tokens, matching howTackleHubConfig.Tokenis already used.📝 Suggested doc comment
-// MCPConfig for MCP analyzer server +// MCPConfig for MCP analyzer server. +// +// Two transport modes are supported and are mutually exclusive: +// - stdio: set BinaryPath (plus optional ProviderConfig and Rules) to spawn +// a local analyzer-mcp process. +// - http: set Endpoint (plus optional Token) to connect to a remote +// analyzer-mcp server. type MCPConfig struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/target_config.go` around lines 125 - 132, The MCPConfig struct lacks documentation and validation for the mutually-exclusive stdio vs http modes and doesn't handle secret Tokens securely; update the MCPConfig definition to (a) add a brief doc comment explaining the two modes — stdio mode using BinaryPath/ProviderConfig/Rules and http mode using Endpoint/Token — and note that BinaryPath and Endpoint are mutually exclusive and one is required, and (b) add validator tags (e.g. required_without/excluded_with equivalents) on BinaryPath and Endpoint so LoadTargetConfig rejects invalid combos consistent with NewMCPTarget behavior; additionally, add support or a note for env-var substitution for Token (like TackleHubConfig.Token uses) so secrets are not stored directly in YAML or document that callers must expand env vars before loading.
129-129:Ruleshere is a single path string; consider renaming or clarifying to avoid confusion withAnalysisConfig.Rules.
AnalysisConfig.Rulesis now[]CustomRule(mix of git and file rules) after this PR.MCPConfig.Rulesis a single path forwarded to the MCP server. Users reading YAML will likely be confused that the same key name has a completely different shape in two different contexts. ConsiderRulesPath string(or similar) for clarity.♻️ Suggested rename
- Rules string `yaml:"rules,omitempty"` + RulesPath string `yaml:"rulesPath,omitempty"`Update callers in
pkg/targets/mcp.go,pkg/targets/mcp_test.go, andpkg/targets/factory_test.goaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/target_config.go` at line 129, The struct field Rules in pkg/config/target_config.go is a single path string but shares the same key name as AnalysisConfig.Rules (which is now []CustomRule), causing confusion; rename the field to a clearer name like RulesPath (or RulesFile/RulesURI) and update all callers that reference MCPConfig.Rules — notably pkg/targets/mcp.go, pkg/targets/mcp_test.go, and pkg/targets/factory_test.go — to use the new field name and the YAML tag (e.g., `yaml:"rules_path,omitempty"`) so the schema and code remain consistent.pkg/targets/mcp_test.go (1)
9-83: Good constructor coverage; consider adding endpoint URL validation cases.The table covers nil, missing, mutex, and both happy paths well. A couple of optional additions worth considering as you flesh out
Execute():
- Invalid/malformed
Endpoint(e.g."not-a-url", missing scheme) — useful onceExecute()actually dials the endpoint, to fail fast at construction.ProviderConfigorRulesset withoutBinaryPath— arguably a misconfiguration (these fields only make sense for stdio mode).Tokenset withoutEndpoint— similar rationale (http-only field).Not blocking; just flagging before the
Execute()implementation lands so the validation surface is locked in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/mcp_test.go` around lines 9 - 83, Add tests and constructor validation for additional misconfigurations: extend TestNewMCPTarget table to include malformed Endpoint values (e.g. "not-a-url" or missing scheme) and assert NewMCPTarget returns an error; add cases where ProviderConfig or Rules are set while BinaryPath is empty and assert an error; add a case where Token is set but Endpoint is empty and assert an error. Implement corresponding validation in NewMCPTarget (inspect cfg.Endpoint with URL parsing, check that ProviderConfig/Rules require BinaryPath, and that Token requires Endpoint) so these test cases fail fast during construction rather than at Execute().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/permerge-ci.yaml:
- Line 22: Tests in pkg/config/git_url_test.go are failing to compile because
they still construct Rules as []string and reference the removed
AnalysisConfig.RulesGitComponents field; update those tests to use the new
[]CustomRule type and remove or replace any usage of RulesGitComponents.
Specifically, change test fixtures and helper functions that build Rules to
return []CustomRule (matching the CustomRule struct/constructor in
target_config.go) and remove references to AnalysisConfig.RulesGitComponents,
replacing them with the current field or API described in target_config.go
(adjust test assertions and setup accordingly). Ensure imports and any
conversions are updated so go test ./... compiles cleanly.
In `@pkg/targets/mcp.go`:
- Line 14: MCPTarget.rules is still defined as a plain string while the codebase
now uses []CustomRule; update the type for MCPTarget.rules and the underlying
MCPConfig.Rules to []CustomRule so they match the new schema and can accept
Git-sourced rule definitions, adjust any constructors/parsers that populate
MCPConfig (look for functions that build MCPConfig or MCPTarget) to
parse/deserialize rule entries into []CustomRule, and ensure the
MCPTarget.Execute method (when implemented) can consume the new []CustomRule
field without further changes.
---
Outside diff comments:
In `@pkg/config/target_config.go`:
- Around line 1-154: The test git_url_test.go fails because the
AnalysisConfig.Rules field is still being initialized as []string but the code
now expects []CustomRule; update the test to construct rules as
[]config.CustomRule using File: &config.FilePathRule{FilePath: "..."} for local
paths and Git: &config.GitRepoRule{GitRepo: "..."} for git entries (mirror the
pattern used in pkg/targets/kantra_test.go and pkg/targets/tackle_hub_test.go).
Also remove or replace any uses of ac.RulesGitComponents (e.g., lines
referencing RulesGitComponents) to use the current fields — use
ac.ApplicationGitComponents where appropriate and ensure ParseGitURLs() is
exercised against ApplicationGitComponents or adapted to parse rules if you add
rule parsing. Update git_url_test.go so all Rules initializations and assertions
match the new CustomRule types and existing
ParseGitURLs()/ApplicationGitComponents API.
---
Nitpick comments:
In @.github/workflows/permerge-ci.yaml:
- Around line 14-22: The unit-test workflow currently uses actions/setup-go@v5
but doesn't explicitly enable module caching; update the unit-test job to ensure
Go module cache is enabled (either confirm that the existing go.sum is used or
set cache: true on the actions/setup-go@v5 step) so dependencies are cached
across runs and the Run unit tests step (go test -short -timeout 5m ./...)
benefits from faster setup.
In `@pkg/config/target_config.go`:
- Around line 125-132: The MCPConfig struct lacks documentation and validation
for the mutually-exclusive stdio vs http modes and doesn't handle secret Tokens
securely; update the MCPConfig definition to (a) add a brief doc comment
explaining the two modes — stdio mode using BinaryPath/ProviderConfig/Rules and
http mode using Endpoint/Token — and note that BinaryPath and Endpoint are
mutually exclusive and one is required, and (b) add validator tags (e.g.
required_without/excluded_with equivalents) on BinaryPath and Endpoint so
LoadTargetConfig rejects invalid combos consistent with NewMCPTarget behavior;
additionally, add support or a note for env-var substitution for Token (like
TackleHubConfig.Token uses) so secrets are not stored directly in YAML or
document that callers must expand env vars before loading.
- Line 129: The struct field Rules in pkg/config/target_config.go is a single
path string but shares the same key name as AnalysisConfig.Rules (which is now
[]CustomRule), causing confusion; rename the field to a clearer name like
RulesPath (or RulesFile/RulesURI) and update all callers that reference
MCPConfig.Rules — notably pkg/targets/mcp.go, pkg/targets/mcp_test.go, and
pkg/targets/factory_test.go — to use the new field name and the YAML tag (e.g.,
`yaml:"rules_path,omitempty"`) so the schema and code remain consistent.
In `@pkg/targets/mcp_test.go`:
- Around line 9-83: Add tests and constructor validation for additional
misconfigurations: extend TestNewMCPTarget table to include malformed Endpoint
values (e.g. "not-a-url" or missing scheme) and assert NewMCPTarget returns an
error; add cases where ProviderConfig or Rules are set while BinaryPath is empty
and assert an error; add a case where Token is set but Endpoint is empty and
assert an error. Implement corresponding validation in NewMCPTarget (inspect
cfg.Endpoint with URL parsing, check that ProviderConfig/Rules require
BinaryPath, and that Token requires Endpoint) so these test cases fail fast
during construction rather than at Execute().
In `@pkg/targets/tackle_hub_test.go`:
- Line 691: Rename the misspelled method GetCompents to GetComponents on the
GitRepoRule type and update all call sites (e.g., the call in the tests
referencing analysis.Rules[0].Git.GetCompents and the occurrences in
kantra_test.go) to use GetComponents; ensure you update the method declaration
in the GitRepoRule type (pkg/config/types.go), any implementing types or
interfaces, and imports/uses so the compiler sees the new name everywhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ddc8c3d-8187-4e7d-858d-20dab1b5076e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/permerge-ci.yamlgo.modpkg/config/target_config.gopkg/targets/factory.gopkg/targets/factory_test.gopkg/targets/kantra_test.gopkg/targets/mcp.gopkg/targets/mcp_test.gopkg/targets/tackle_hub_test.go
| type MCPTarget struct { | ||
| binaryPath string | ||
| providerConfig string | ||
| rules string |
There was a problem hiding this comment.
rules field type inconsistent with the CustomRule refactor.
This PR migrates rule handling from []string to []CustomRule elsewhere in the codebase, but MCPTarget.rules (and the underlying MCPConfig.Rules in pkg/config/target_config.go) are still plain string. When Execute is implemented, it will not be able to consume Git-sourced rule definitions the way other targets now do. Consider aligning the schema/field with []CustomRule before the Execute implementation lands, to avoid a breaking config change later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/mcp.go` at line 14, MCPTarget.rules is still defined as a plain
string while the codebase now uses []CustomRule; update the type for
MCPTarget.rules and the underlying MCPConfig.Rules to []CustomRule so they match
the new schema and can accept Git-sourced rule definitions, adjust any
constructors/parsers that populate MCPConfig (look for functions that build
MCPConfig or MCPTarget) to parse/deserialize rule entries into []CustomRule, and
ensure the MCPTarget.Execute method (when implemented) can consume the new
[]CustomRule field without further changes.
✅ Actions performedReviews paused. |
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
Should the MCP server expose every violation and insight to the LLM?
Many informational/tagging incidents will cause noise and be unactionable.
2 participants
Summary
MCPConfigand MCP target type for connecting to analyzer-mcp servers (stdio and HTTP modes)MCPTarget.Execute()with MCP protocol (connects via go-sdk, callsanalyzetool, parses rulesets)KaiRPCTarget.Execute()for connecting to kai-analyzer-rpc servers via rpc2kantra_test.goandtackle_hub_test.go(Rulesfield changed from[]stringto[]CustomRulebut tests were never updated)go testto premerge CI workflow so unit test breakage is caught before mergemcpto CLI help text for-tflagTest plan
go test ./...passes across all packages