Various cleanups and enhancements#18
Merged
Merged
Conversation
It probably does make sense to support.
Update repository URLs and container image references from cgwalters to LobsterTrap. Intentionally leaves cgwalters/playground and cgwalters/devaipod references since those repos remain under cgwalters. Also consolidates workspace dependencies (rmcp, tempfile), removes duplicate itertools dev-dep, bumps integration-tests rmcp from 0.13 to 0.14 to avoid pulling two versions, and removes a few dead imports and comments. Assisted-by: OpenCode (Claude Opus 4)
The HTTP proxy had stub permission validation for GitLab and Forgejo (unconditional Ok(())) and is not used in any deployment. Rather than fix the stubs, just remove the module — the REST API servers provide the same functionality with proper permission enforcement. Drops the direct hyper-util dependency. Assisted-by: OpenCode (Claude Opus 4)
The CLI subcommands are useful for local testing but are not a security boundary — only the MCP server provides credential isolation by running in a separate container. Update the package description, module docs, and subcommand help text to reflect this. Also hide the experimental REST API server flags (--github-port, --gitlab-port, etc.) from default --help output. Assisted-by: OpenCode (Claude Opus 4)
The JIRA permission model had a dead `create` field — `issue create`
was classified as OpType::Write, so `can_create()` was never called.
Fix this by adding OpType::Create and OpType::Comment variants, giving
JIRA four permission tiers:
read < {comment, create} < write
Comment and create are independent of each other (neither implies the
other), but both imply read, and write implies everything. This lets
you grant an agent comment-only access without allowing issue creation,
or create-only without allowing transitions/assignments.
Also adds:
- `global_read` on JiraScope — allows reading any project without
listing each one explicitly, matching the GitHub model
- `issue comment -i ISSUE-KEY -b BODY` command via gouqi's
AddComment API
- Simplify MCP and CLI permission checks to use `JiraScope::is_allowed()`
instead of duplicating the logic inline
Assisted-by: OpenCode (Claude Opus 4)
📝 WalkthroughWalkthroughThe PR repositions service-gator as an MCP server-focused application by removing the HTTP proxy module, refocusing CLI documentation, deprioritizing REST server flags, and expanding JIRA capabilities to support issue commenting. Concurrently, container image and repository references transition from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp.rs (1)
2673-2707:⚠️ Potential issue | 🟡 MinorUpdate MCP server metadata to match the new product positioning.
This block still advertises “Scoped CLI access for AI agents” and only mentions JIRA
read/create/writepermissions, even though this PR repositions the project as an MCP server and adds JIRA comment support. MCP clients that surfaceServerInfowill keep showing stale guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp.rs` around lines 2673 - 2707, Update the hardcoded ServerInfo block so its Implementation.name/title/instructions reflect the new MCP server positioning and include the added JIRA comment capability and accurate permission list: modify the Implementation fields (server_info: Implementation { name, title, ... }) and the long instructions string (currently mentioning "Scoped CLI access for AI agents" and "JIRA read/create/write") to describe the project as an MCP server, call out JIRA comment support, and ensure the listed tools and permission scopes (github_api_tool, github_push, github_pending_review_tool, gl, forgejo, gh_create_branch, gh_update_pr_head, git_push_local, jira) and their exact read/write/comment/create/approve scopes are accurate and up-to-date so ServerInfo consumers surface correct guidance.
🧹 Nitpick comments (2)
src/forgejo.rs (1)
198-199: Add regression tests for the newOpType::CommentandOpType::Createpath.Line [198]-Line [199] changes classification behavior, but there are no explicit unit tests for these two variants. Adding focused tests will prevent silent regressions.
Proposed test additions
#[test] fn test_classify_forgejo_op_write() { @@ } +#[test] +fn test_classify_forgejo_op_comment_and_create_variants() { + let comment_analysis = analysis_with_op(OpType::Comment); + assert_eq!( + classify_forgejo_op(&args("issue comment 123"), &comment_analysis), + ForgejoOpType::WriteResource + ); + + let create_analysis = analysis_with_op(OpType::Create); + assert_eq!( + classify_forgejo_op(&args("pr create --draft"), &create_analysis), + ForgejoOpType::CreateDraft + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/forgejo.rs` around lines 198 - 199, The change groups OpType::Comment and OpType::Create with OpType::Write for Forgejo but lacks unit tests, so add focused regression tests that assert the classification/handling code treats OpType::Comment and OpType::Create the same as OpType::Write; locate the function or match that performs this mapping (the match arm containing "OpType::Write | OpType::Comment | OpType::Create") and add tests that call the classifier/handler with each variant and verify the returned classification/behavior equals the Write-case behavior (and include any relevant context/mocks used by existing tests for Forgejo).src/jira_client.rs (1)
211-219: Add an ignored live test foradd_comment.This is now a public mutating client path, but unlike the other JIRA operations in this file it has no integration-style coverage. A small ignored test here would catch request-shape/auth regressions before they reach MCP/CLI runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jira_client.rs` around lines 211 - 219, Add a small integration-style ignored test exercising the public add_comment method: create an async test like #[tokio::test] #[ignore] async fn test_add_comment_live() -> Result<()> that builds or loads a live Jira client (reusing existing test helpers if present), reads the target issue key from an environment variable (e.g. JIRA_TEST_ISSUE) and early-skips when unset, calls jira_client.add_comment(&issue_key, "test comment from CI"), and asserts the call returns Ok(()); keep the test ignored so it only runs intentionally and include minimal setup/teardown and clear naming so it catches request-shape/auth regressions for add_comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jira.rs`:
- Around line 189-199: The IssueCommentArgs struct's issue field was changed to
a raw String which bypasses the typed issue-key validation used elsewhere;
change the type of IssueCommentArgs::issue back to the project-specific IssueKey
(or the same IssueKey/IssueId type used by other issue subcommands) so clap will
parse/validate it the same way and downstream parse_command() receives a
validated key; update any necessary imports and ensure any code using
IssueCommentArgs::issue still calls the same parse/validation helpers (or
converts the IssueKey to a string only where required).
In `@src/mcp.rs`:
- Around line 2441-2444: The project-list branch still errors out on
config.jira.projects.is_empty(), blocking tokens that have global_read scope;
change the is_project_list handling to consult the centralized permission check
instead of hard-failing: remove (or guard) the projects.is_empty() hard error
and call config.jira.is_allowed(...) (the same JiraScope::is_allowed() used
elsewhere) with the appropriate operation (project list/read) and no specific
project/issue to determine if the token is permitted to list projects; allow the
request when is_allowed returns true.
In `@src/scope.rs`:
- Around line 1121-1126: The has_any_read_access method currently only checks
i.read on each issue, but issue-level comment or write grants should also count
as readable; update has_any_read_access (the function named has_any_read_access)
to treat an issue as readable when i.read || i.comment || i.write (or equivalent
fields on the Issue permission struct) so that issues with only comment/write
grants cause the method to return true; keep the existing global_read and
project checks unchanged.
- Around line 1003-1005: The new comment permission field (Scope.comment) was
added to the model but the CLI flag parsing in src/bin/service_gator.rs that
handles --jira-project still only accepts read/create/write; update the parsing
logic that interprets the PROJ:scope string (the function handling
--jira-project parsing in service_gator.rs) to recognize and accept "comment" as
a valid scope value, map it to the same internal Scope.comment flag, and include
it in any validation/error messages so "service-gator --jira-project
PROJ:comment" works the same as read/create/write.
- Around line 1060-1062: The new Scope field global_read needs wiring where
configs are merged and CLI read-only checks occur: update merge_config() to copy
source.jira.global_read into the destination Scope (preserving semantics when
present), ensure the --scope JSON merge path handles the global_read key, and
modify the checks in src/bin/service_gator.rs that currently reject "project
list" and "search" when projects is empty to allow those operations if
scope.global_read == true (check Scope::global_read alongside the existing
projects logic). Ensure you reference Scope::global_read, merge_config(), and
the CLI handlers for "project list"/"search" so the flag works for both config
merging and read-only CLI paths.
---
Outside diff comments:
In `@src/mcp.rs`:
- Around line 2673-2707: Update the hardcoded ServerInfo block so its
Implementation.name/title/instructions reflect the new MCP server positioning
and include the added JIRA comment capability and accurate permission list:
modify the Implementation fields (server_info: Implementation { name, title, ...
}) and the long instructions string (currently mentioning "Scoped CLI access for
AI agents" and "JIRA read/create/write") to describe the project as an MCP
server, call out JIRA comment support, and ensure the listed tools and
permission scopes (github_api_tool, github_push, github_pending_review_tool, gl,
forgejo, gh_create_branch, gh_update_pr_head, git_push_local, jira) and their
exact read/write/comment/create/approve scopes are accurate and up-to-date so
ServerInfo consumers surface correct guidance.
---
Nitpick comments:
In `@src/forgejo.rs`:
- Around line 198-199: The change groups OpType::Comment and OpType::Create with
OpType::Write for Forgejo but lacks unit tests, so add focused regression tests
that assert the classification/handling code treats OpType::Comment and
OpType::Create the same as OpType::Write; locate the function or match that
performs this mapping (the match arm containing "OpType::Write | OpType::Comment
| OpType::Create") and add tests that call the classifier/handler with each
variant and verify the returned classification/behavior equals the Write-case
behavior (and include any relevant context/mocks used by existing tests for
Forgejo).
In `@src/jira_client.rs`:
- Around line 211-219: Add a small integration-style ignored test exercising the
public add_comment method: create an async test like #[tokio::test] #[ignore]
async fn test_add_comment_live() -> Result<()> that builds or loads a live Jira
client (reusing existing test helpers if present), reads the target issue key
from an environment variable (e.g. JIRA_TEST_ISSUE) and early-skips when unset,
calls jira_client.add_comment(&issue_key, "test comment from CI"), and asserts
the call returns Ok(()); keep the test ignored so it only runs intentionally and
include minimal setup/teardown and clear naming so it catches request-shape/auth
regressions for add_comment.
🪄 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: 872f260d-583d-45d2-8722-8b66df4a8140
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlJustfileREADME.mddocs/todo/openpolicyagent.mdintegration-tests/Cargo.tomlintegration-tests/src/mcp_client.rsintegration-tests/src/tests/mcp_server.rsintegration-tests/src/tests/rest_api.rssrc/bin/service_gator.rssrc/forgejo.rssrc/github.rssrc/gitlab.rssrc/jira.rssrc/jira_client.rssrc/lib.rssrc/mcp.rssrc/proxy.rssrc/scope.rssrc/servers/middleware/auth.rssrc/services/jira.rstest-config.toml
💤 Files with no reviewable changes (2)
- src/servers/middleware/auth.rs
- src/proxy.rs
Comment on lines
+189
to
+199
| /// Arguments for `issue comment`. | ||
| #[derive(Debug, Clone, Parser)] | ||
| pub struct IssueCommentArgs { | ||
| /// Issue key (e.g. PROJ-123) | ||
| #[arg(short = 'i', long = "issue")] | ||
| pub issue: String, | ||
|
|
||
| /// Comment body text | ||
| #[arg(short = 'b', long = "body")] | ||
| pub body: String, | ||
| } |
There was a problem hiding this comment.
Restore typed issue-key validation for issue comment.
Line 194 switches this subcommand to a raw String, so parse_command() now accepts issue keys that every other issue action rejects. That weakens the validation boundary and can also skip issue-level scope matching when later code reparses the key.
Suggested fix
pub struct IssueCommentArgs {
/// Issue key (e.g. PROJ-123)
#[arg(short = 'i', long = "issue")]
- pub issue: String,
+ pub issue: JiraIssueKey,
/// Comment body text
#[arg(short = 'b', long = "body")]
pub body: String,
} IssueAction::Comment(args) => {
- let project = project_from_issue(&args.issue);
(
- project,
- Some(args.issue.clone()),
+ Some(args.issue.project().to_string()),
+ Some(args.issue.to_string()),
"jira issue comment".to_string(),
)
} IssueAction::Comment(args) => {
vec![
"issue".to_string(),
"comment".to_string(),
"-i".to_string(),
- args.issue.clone(),
+ args.issue.to_string(),
"-b".to_string(),
args.body.clone(),
]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jira.rs` around lines 189 - 199, The IssueCommentArgs struct's issue
field was changed to a raw String which bypasses the typed issue-key validation
used elsewhere; change the type of IssueCommentArgs::issue back to the
project-specific IssueKey (or the same IssueKey/IssueId type used by other issue
subcommands) so clap will parse/validate it the same way and downstream
parse_command() receives a validated key; update any necessary imports and
ensure any code using IssueCommentArgs::issue still calls the same
parse/validation helpers (or converts the IssueKey to a string only where
required).
Comment on lines
+2441
to
+2444
| let allowed = | ||
| config | ||
| .jira | ||
| .is_allowed(project_key.as_str(), op_type, validated.issue.as_deref()); |
There was a problem hiding this comment.
project list still rejects global_read-only JIRA scopes.
This centralizes most permission checks through JiraScope::is_allowed(), but the adjacent is_project_list branch still hard-fails on config.jira.projects.is_empty(). With the new global_read mode in this PR, a token that is allowed to read any project will still get No JIRA projects configured for project list.
Suggested fix
- } else if is_project_list {
- // For project list, just check that at least one project is configured
- if config.jira.projects.is_empty() {
+ } else if is_project_list {
+ // For project list, any JIRA read capability is sufficient
+ if !config.jira.has_any_read_access() {
return Ok(CallToolResult::error(vec![Content::text(
- "No JIRA projects configured",
+ "JIRA read access not configured",
)]));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp.rs` around lines 2441 - 2444, The project-list branch still errors
out on config.jira.projects.is_empty(), blocking tokens that have global_read
scope; change the is_project_list handling to consult the centralized permission
check instead of hard-failing: remove (or guard) the projects.is_empty() hard
error and call config.jira.is_allowed(...) (the same JiraScope::is_allowed()
used elsewhere) with the appropriate operation (project list/read) and no
specific project/issue to determine if the token is permitted to list projects;
allow the request when is_allowed returns true.
Comment on lines
+1003
to
+1005
| /// Can comment on issues in this project. | ||
| #[serde(default)] | ||
| pub comment: bool, |
There was a problem hiding this comment.
comment scopes are not grantable from --jira-project yet.
This new permission is added to the model, but src/bin/service_gator.rs still parses --jira-project with only read, create, and write. service-gator --jira-project PROJ:comment ... will still fail, so the new comment operation is only configurable from TOML/JSON today.
Suggested fix
fn parse_jira_project_spec(spec: &str) -> Result<(JiraProjectKey, JiraProjectPermission)> {
@@
let mut perm = JiraProjectPermission::default();
for p in perms_str.split(',') {
match p.trim() {
"read" => perm.read = true,
+ "comment" => perm.comment = true,
"create" => perm.create = true,
"write" => perm.write = true,
"" => {}
- other => bail!("unknown permission '{other}'; valid: read, create, write"),
+ other => bail!("unknown permission '{other}'; valid: read, comment, create, write"),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Can comment on issues in this project. | |
| #[serde(default)] | |
| pub comment: bool, | |
| fn parse_jira_project_spec(spec: &str) -> Result<(JiraProjectKey, JiraProjectPermission)> { | |
| let mut perm = JiraProjectPermission::default(); | |
| for p in perms_str.split(',') { | |
| match p.trim() { | |
| "read" => perm.read = true, | |
| "comment" => perm.comment = true, | |
| "create" => perm.create = true, | |
| "write" => perm.write = true, | |
| "" => {} | |
| other => bail!("unknown permission '{other}'; valid: read, comment, create, write"), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope.rs` around lines 1003 - 1005, The new comment permission field
(Scope.comment) was added to the model but the CLI flag parsing in
src/bin/service_gator.rs that handles --jira-project still only accepts
read/create/write; update the parsing logic that interprets the PROJ:scope
string (the function handling --jira-project parsing in service_gator.rs) to
recognize and accept "comment" as a valid scope value, map it to the same
internal Scope.comment flag, and include it in any validation/error messages so
"service-gator --jira-project PROJ:comment" works the same as read/create/write.
Comment on lines
+1060
to
+1062
| /// Allow read access to all projects, not just those explicitly listed. | ||
| #[serde(default)] | ||
| pub global_read: bool, |
There was a problem hiding this comment.
global_read needs caller/config plumbing too.
Adding the field here is not enough: src/bin/service_gator.rs still rejects project list and search when projects is empty, and merge_config() there never copies source.jira.global_read. A config that relies on this new flag therefore still breaks in read-only CLI paths and via --scope JSON merging.
Suggested fix
fn merge_config(target: &mut ScopeConfig, source: ScopeConfig) {
@@
if source.jira.token.is_some() {
target.jira.token = source.jira.token;
}
+ if source.jira.global_read {
+ target.jira.global_read = true;
+ }
// Merge JIRA projects
for (project, perm) in source.jira.projects {
target.jira.projects.insert(project, perm);
}- if is_project_list || is_search {
- if config.jira.projects.is_empty() {
+ if is_project_list || is_search {
+ if !config.jira.global_read && config.jira.projects.is_empty() {
bail!("No JIRA projects configured");
}
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope.rs` around lines 1060 - 1062, The new Scope field global_read needs
wiring where configs are merged and CLI read-only checks occur: update
merge_config() to copy source.jira.global_read into the destination Scope
(preserving semantics when present), ensure the --scope JSON merge path handles
the global_read key, and modify the checks in src/bin/service_gator.rs that
currently reject "project list" and "search" when projects is empty to allow
those operations if scope.global_read == true (check Scope::global_read
alongside the existing projects logic). Ensure you reference Scope::global_read,
merge_config(), and the CLI handlers for "project list"/"search" so the flag
works for both config merging and read-only CLI paths.
Comment on lines
1121
to
1126
| /// Check if the user has any read access to any project. | ||
| pub fn has_any_read_access(&self) -> bool { | ||
| self.projects.values().any(|p| p.can_read()) || self.issues.values().any(|i| i.read) | ||
| self.global_read | ||
| || self.projects.values().any(|p| p.can_read()) | ||
| || self.issues.values().any(|i| i.read) | ||
| } |
There was a problem hiding this comment.
Count issue-level comment/write grants as readable access here.
Lines 1093-1096 now treat issue comment and write as read-capable, but this helper still only checks i.read. A scope with only issue-specific comment/write grants will be reported as having no JIRA read access.
Suggested fix
pub fn has_any_read_access(&self) -> bool {
self.global_read
|| self.projects.values().any(|p| p.can_read())
- || self.issues.values().any(|i| i.read)
+ || self
+ .issues
+ .values()
+ .any(|i| i.read || i.comment || i.write)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Check if the user has any read access to any project. | |
| pub fn has_any_read_access(&self) -> bool { | |
| self.projects.values().any(|p| p.can_read()) || self.issues.values().any(|i| i.read) | |
| self.global_read | |
| || self.projects.values().any(|p| p.can_read()) | |
| || self.issues.values().any(|i| i.read) | |
| } | |
| pub fn has_any_read_access(&self) -> bool { | |
| self.global_read | |
| || self.projects.values().any(|p| p.can_read()) | |
| || self | |
| .issues | |
| .values() | |
| .any(|i| i.read || i.comment || i.write) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope.rs` around lines 1121 - 1126, The has_any_read_access method
currently only checks i.read on each issue, but issue-level comment or write
grants should also count as readable; update has_any_read_access (the function
named has_any_read_access) to treat an issue as readable when i.read ||
i.comment || i.write (or equivalent fields on the Issue permission struct) so
that issues with only comment/write grants cause the method to return true; keep
the existing global_read and project checks unchanged.
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.
Biggest thing here is dropping some half implemented stuff like the REST proxy - that already exists in other places like OpenShell. It's most reasonable for us to implement a secure and flexible model by being an MCP server.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores