From 0e6e4bf3e4b424b2fc0524f844e48ceefc0caf40 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 29 Jul 2026 01:05:32 +0530 Subject: [PATCH 1/2] fix(flare_git): validate action before repo/client setup; resolve default branch via API on repo override --- src/github/mod.rs | 1 + src/github/repos.rs | 46 +++++++++++++++++++++++++ src/mcp_server/flare_git.rs | 68 +++++++++++++++++++++++++++++++------ 3 files changed, 105 insertions(+), 10 deletions(-) create mode 100644 src/github/repos.rs diff --git a/src/github/mod.rs b/src/github/mod.rs index 3e79ec92..398b91fa 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -13,6 +13,7 @@ pub mod mcp; pub mod models; pub mod pulls; pub mod releases; +pub mod repos; #[cfg(test)] pub(crate) mod test_support; diff --git a/src/github/repos.rs b/src/github/repos.rs new file mode 100644 index 00000000..9cd4fa09 --- /dev/null +++ b/src/github/repos.rs @@ -0,0 +1,46 @@ +//! Repository-level GitHub API calls (currently just default-branch lookup). + +use crate::github::{Client, GitHubError, RepoId}; + +/// Fetches `repo`'s default branch via the GitHub API. Used when an explicit +/// `repo` override is given, since there's no local checkout to read it from. +pub fn get_default_branch(client: &Client, repo: &RepoId) -> Result { + let path = format!("/repos/{}/{}", repo.owner, repo.repo); + let json = client.request("GET", &path, None)?; + json.get("default_branch") + .and_then(|v| v.as_str()) + .map(str::to_string) + .ok_or_else(|| GitHubError::Parse("missing default_branch".to_string())) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::github::test_support::{MockResponse, MockServer}; + + fn repo() -> RepoId { + RepoId { + owner: "o".into(), + repo: "r".into(), + } + } + + #[test] + fn get_default_branch_reads_the_field() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"default_branch":"main"}"#, + )]); + let client = server.client(None); + assert_eq!(get_default_branch(&client, &repo()).unwrap(), "main"); + assert_eq!(server.requests()[0].path, "/repos/o/r"); + } + + #[test] + fn get_default_branch_errors_when_field_missing() { + let server = MockServer::start(vec![MockResponse::json(200, "{}")]); + let client = server.client(None); + assert!(get_default_branch(&client, &repo()).is_err()); + let _ = server.requests(); + } +} diff --git a/src/mcp_server/flare_git.rs b/src/mcp_server/flare_git.rs index 201f64fe..d2f2ffa5 100644 --- a/src/mcp_server/flare_git.rs +++ b/src/mcp_server/flare_git.rs @@ -4,7 +4,40 @@ use super::*; impl AgentflareMcp { pub fn flare_git_impl(&self, req: GitHubRequest) -> Result { - use crate::github::{Client, RepoId, actions, issues, mcp, pulls, releases}; + use crate::github::{Client, RepoId, actions, issues, mcp, pulls, releases, repos}; + + // Keep in sync with the action names matched below — validating + // first means an unknown action fails before repo/client setup + // (and credentials) rather than surfacing as an unrelated error. + const KNOWN_ACTIONS: &[&str] = &[ + "pr_create", + "pr_list", + "pr_get", + "pr_status", + "pr_merge", + "pr_comment", + "pr_request_review", + "issue_create", + "issue_list", + "issue_get", + "issue_comment", + "issue_close", + "issue_label", + "release_list", + "release_get", + "release_latest", + "release_create", + "run_list", + "run_get", + "run_rerun", + "workflow_dispatch", + ]; + if !KNOWN_ACTIONS.contains(&req.action.as_str()) { + return Err(ErrorData::invalid_params( + format!("unknown action: {}", req.action), + None, + )); + } let repo = match &req.repo { Some(r) => RepoId::parse(r) @@ -253,17 +286,14 @@ impl AgentflareMcp { } let git_ref = match req.git_ref.as_deref() { Some(r) => r.to_string(), - None => { - if req.repo.is_some() { - return Err(ErrorData::invalid_params( - "git_ref is required when repo is overridden (cannot infer the target repo default branch)", - None, - )); + None => match &req.repo { + Some(_) => { + repos::get_default_branch(&client, &repo).map_err(to_mcp_error)? } - flare_git_core::branch::resolve_default_branch( + None => flare_git_core::branch::resolve_default_branch( &std::env::current_dir().unwrap_or_default(), - ) - } + ), + }, }; actions::dispatch(&client, &repo, wf, &git_ref, req.inputs.as_ref()) .map_err(to_mcp_error)?; @@ -279,3 +309,21 @@ impl AgentflareMcp { Ok(out) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn unknown_action_is_rejected_before_repo_or_client_setup() { + // Credential-independent: an unknown action must fail on its own + // merits, not because there's no repo/token in the test environment. + let mcp = AgentflareMcp::default(); + let req = GitHubRequest { + action: "bogus".to_string(), + ..Default::default() + }; + let err = mcp.flare_git_impl(req).unwrap_err(); + assert!(err.to_string().contains("unknown action: bogus"), "{err}"); + } +} From 1016b9f812a7ecda50db68ad451673d24baef5b7 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 29 Jul 2026 01:20:18 +0530 Subject: [PATCH 2/2] test(flare_git): cover both workflow_dispatch default-branch resolution paths --- src/mcp_server/flare_git.rs | 69 ++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/src/mcp_server/flare_git.rs b/src/mcp_server/flare_git.rs index d2f2ffa5..05e8d013 100644 --- a/src/mcp_server/flare_git.rs +++ b/src/mcp_server/flare_git.rs @@ -284,17 +284,17 @@ impl AgentflareMcp { None, )); } - let git_ref = match req.git_ref.as_deref() { - Some(r) => r.to_string(), - None => match &req.repo { - Some(_) => { - repos::get_default_branch(&client, &repo).map_err(to_mcp_error)? - } - None => flare_git_core::branch::resolve_default_branch( + let git_ref = resolve_workflow_git_ref( + req.git_ref.as_deref(), + req.repo.is_some(), + || repos::get_default_branch(&client, &repo), + || { + flare_git_core::branch::resolve_default_branch( &std::env::current_dir().unwrap_or_default(), - ), + ) }, - }; + ) + .map_err(to_mcp_error)?; actions::dispatch(&client, &repo, wf, &git_ref, req.inputs.as_ref()) .map_err(to_mcp_error)?; format!("Dispatched {wf} on {git_ref}") @@ -310,10 +310,61 @@ impl AgentflareMcp { } } +/// Decides the git ref for `workflow_dispatch`: an explicit `git_ref` wins; +/// otherwise an overridden `repo` resolves its default branch via +/// `remote_default` (a GitHub API call), and the no-override case via +/// `local_default` (the checkout's origin). Pulled out standalone so both +/// paths are unit-testable without a network-backed `Client`. +fn resolve_workflow_git_ref( + explicit: Option<&str>, + repo_overridden: bool, + remote_default: impl FnOnce() -> Result, + local_default: impl FnOnce() -> String, +) -> Result { + match explicit { + Some(r) => Ok(r.to_string()), + None if repo_overridden => remote_default(), + None => Ok(local_default()), + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn resolve_workflow_git_ref_prefers_the_explicit_ref() { + let r = resolve_workflow_git_ref( + Some("refs/heads/explicit"), + true, + || panic!("must not resolve a default when a ref is given"), + || panic!("must not resolve a default when a ref is given"), + ); + assert_eq!(r.unwrap(), "refs/heads/explicit"); + } + + #[test] + fn resolve_workflow_git_ref_uses_the_remote_api_when_repo_is_overridden() { + let r = resolve_workflow_git_ref( + None, + true, + || Ok("main".to_string()), + || panic!("an overridden repo must resolve via the API, not local git"), + ); + assert_eq!(r.unwrap(), "main"); + } + + #[test] + fn resolve_workflow_git_ref_uses_local_resolution_without_a_repo_override() { + let r = resolve_workflow_git_ref( + None, + false, + || panic!("no repo override must not hit the GitHub API"), + || "develop".to_string(), + ); + assert_eq!(r.unwrap(), "develop"); + } + #[test] fn unknown_action_is_rejected_before_repo_or_client_setup() { // Credential-independent: an unknown action must fail on its own