Skip to content

Commit 0b26f4d

Browse files
jamesadevineCopilot
andcommitted
feat(create-pr): align with gh-aw create-pull-request implementation
Closes the gap between ado-aw and gh-aw's create-pull-request safe output: Security/Correctness: - Add file protection system blocking manifests (package.json, go.mod, Cargo.toml, etc.), CI configs (.github/, .pipelines/), and agent instruction files (.agents/, .claude/, .copilot/) by default - Add max-files limit per PR (default: 100, configurable) - Add 3-way merge fallback when patch application fails on conflicts - Replace predictable timestamp-based branch suffix with cryptographic random hex (rand crate) Feature Parity: - Add draft PR support (default: true, operator-enforced via isDraft) - Add fallback-as-work-item: record branch info on PR creation failure - Add excluded-files glob config to filter files from patches - Add if-no-changes config (warn/error/ignore) for empty patches - Add allowed-labels allowlist to restrict agent-provided labels - Add title-prefix config for operator branding - Add agent-provided labels parameter validated against allowed-labels Patch Format: - Migrate from raw git diff to git format-patch for proper commit metadata, rename detection, and binary file handling - Stage 2 applies patches via git am --3way with git apply fallback - Add collect_changes_from_diff_tree for committed patch changes - Add git to default bash command allowlist so agents can commit - Update tool description to encourage staging commits before PR creation Other: - Add provenance footer to PR body with timestamp and compiler version - Add ExecutionResult::failure_with_data for structured error responses Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ffdd1f9 commit 0b26f4d

7 files changed

Lines changed: 970 additions & 150 deletions

File tree

Cargo.lock

Lines changed: 49 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ terminal_size = "0.4.3"
3030
url = "2.5.8"
3131
axum = { version = "0.8.8", features = ["tokio"] }
3232
subtle = "2.6.1"
33+
rand = "0.10.1"
34+
glob-match = "0.2.1"
3335

3436
[dev-dependencies]
3537
reqwest = { version = "0.12", features = ["blocking"] }

docs/safeoutputs-tool-filtering.md

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
# SafeOutputs Tool Filtering — Options
2+
3+
## Problem
4+
5+
SafeOutputs currently exposes **all** tools (noop, missing-tool, missing-data, create-work-item, create-pull-request) to every agent, regardless of `safe-outputs:` front matter configuration. This causes:
6+
7+
1. **Unnecessary tool access** — agents can call tools they shouldn't need
8+
2. **Context pollution** — extra tool schemas waste agent context window
9+
10+
## MCPG `tools` field — current status
11+
12+
From [MCPG CONFIGURATION.md](https://github.com/github/gh-aw-mcpg/blob/main/docs/CONFIGURATION.md):
13+
14+
> **`tools`** (optional): List of tool names intended to be exposed from this server
15+
>
16+
> **Note: This field is stored but not currently enforced at runtime; all tools from the backend are always exposed regardless of this value**
17+
18+
**MCPG does NOT currently filter tools at the gateway level.** Setting `tools` on the safeoutputs entry will have no effect today.
19+
20+
## Options
21+
22+
### Option A: Set `tools` field in MCPG config only (forward-compatible)
23+
24+
**What**: Set the `tools` field on the safeoutputs MCPG entry based on front matter.
25+
26+
**Pros**: Zero risk, documents intent, will work when MCPG adds enforcement.
27+
**Cons**: Doesn't solve the problem today.
28+
29+
**Files**: `src/compile/standalone.rs` only.
30+
31+
### Option B: Filter tools at the SafeOutputs MCP server level
32+
33+
**What**: Add a `--tools` CLI argument to `mcp-http` that controls which tools get registered in the `ToolRouter`. Only listed tools appear in `tools/list` and are callable.
34+
35+
**Pros**: Actually enforces filtering today, regardless of MCPG.
36+
**Cons**: More invasive — changes to `mcp.rs`, `main.rs`, `base.yml` template.
37+
38+
**Files**: `src/mcp.rs`, `src/main.rs`, `templates/base.yml`, `src/compile/standalone.rs`.
39+
40+
### Option C: Both A and B (recommended)
41+
42+
**What**: Set MCPG `tools` for documentation + forward-compat, AND filter at the server level for actual enforcement.
43+
44+
**Pros**: Works today AND future-proofs for when MCPG adds enforcement.
45+
**Cons**: Most work, but straightforward.
46+
47+
## Tool categories
48+
49+
| Tool | Category | Availability |
50+
|------|----------|-------------|
51+
| `noop` | Baseline | Always available |
52+
| `missing-tool` | Baseline | Always available |
53+
| `missing-data` | Baseline | Always available |
54+
| `create-work-item` | Conditional | Only if `safe-outputs.create-work-item` configured |
55+
| `create-pull-request` | Conditional | Only if `safe-outputs.create-pull-request` configured |
56+
| `memory` | N/A | Not an MCP tool (filesystem-based) |
57+
58+
## Implementation sketch (Option C)
59+
60+
1. **`standalone.rs`** — In `generate_mcpg_config()`, build a `tools` list:
61+
- Always: `["noop", "missing-tool", "missing-data"]`
62+
- Add `"create-work-item"` if `front_matter.safe_outputs.contains_key("create-work-item")`
63+
- Add `"create-pull-request"` if `front_matter.safe_outputs.contains_key("create-pull-request")`
64+
- Set `tools: Some(list)` on the safeoutputs `McpgServerConfig`
65+
66+
2. **`main.rs`** — Add `--tools` arg to `McpHttp` command (comma-separated list)
67+
68+
3. **`mcp.rs`** — Accept tools filter in `run_http()`, conditionally register only matching `#[tool]` handlers
69+
70+
4. **`base.yml`** — Pass `--tools` flag to SafeOutputs HTTP server step using a new `{{ safeoutputs_tools }}` marker
71+
72+
5. **Tests** — Update MCPG config tests, HTTP integration tests, smoke script

src/compile/common.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,9 +261,9 @@ pub fn validate_checkout_list(repositories: &[Repository], checkout: &[String])
261261
Ok(())
262262
}
263263

264-
/// Default bash commands allowed for agents (matches gh-aw defaults + yq)
264+
/// Default bash commands allowed for agents (matches gh-aw defaults + yq + git)
265265
const DEFAULT_BASH_COMMANDS: &[&str] = &[
266-
"cat", "date", "echo", "grep", "head", "ls", "pwd", "sort", "tail", "uniq", "wc", "yq",
266+
"cat", "date", "echo", "git", "grep", "head", "ls", "pwd", "sort", "tail", "uniq", "wc", "yq",
267267
];
268268

269269
/// Generate copilot CLI params from front matter configuration

src/mcp.rs

Lines changed: 70 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,11 @@ fn slugify_title(title: &str) -> String {
5252
collapsed.chars().take(50).collect()
5353
}
5454

55-
/// Generate a short random suffix for branch uniqueness
55+
/// Generate a short cryptographically random suffix for branch uniqueness
5656
fn generate_short_id() -> String {
57-
use std::time::{SystemTime, UNIX_EPOCH};
58-
let timestamp = SystemTime::now()
59-
.duration_since(UNIX_EPOCH)
60-
.unwrap_or_default()
61-
.as_millis();
62-
// Take last 6 hex digits of timestamp for short unique suffix
63-
format!("{:06x}", (timestamp & 0xFFFFFF) as u32)
57+
use rand::RngExt;
58+
let value: u32 = rand::rng().random();
59+
format!("{:08x}", value)
6460
}
6561

6662
// ============================================================================
@@ -198,79 +194,80 @@ impl SafeOutputs {
198194
}
199195
};
200196

201-
// Run git diff against the target branch to capture all changes
202-
// Try origin/main first (remote tracking), then main, then HEAD as fallback
203-
let diff_targets = ["origin/main", "main", "HEAD"];
204-
let mut last_error = String::new();
205-
let mut diff_output = None;
206-
207-
for target in &diff_targets {
208-
let output = Command::new("git")
209-
.args(["diff", target])
210-
.current_dir(&git_dir)
211-
.output()
212-
.await
213-
.map_err(|e| {
214-
anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git diff: {}", e))
215-
})?;
197+
// Generate patch using git format-patch for proper commit metadata,
198+
// rename detection, and binary file handling.
199+
//
200+
// Steps:
201+
// 1. Stage all changes (tracked + untracked)
202+
// 2. Create a temporary commit
203+
// 3. Generate format-patch output
204+
// 4. Reset to undo the temporary commit (preserving working tree)
205+
206+
// Stage all changes including untracked files
207+
let add_output = Command::new("git")
208+
.args(["add", "-A"])
209+
.current_dir(&git_dir)
210+
.output()
211+
.await
212+
.map_err(|e| {
213+
anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git add -A: {}", e))
214+
})?;
216215

217-
if output.status.success() {
218-
diff_output = Some(output);
219-
break;
220-
}
221-
last_error = String::from_utf8_lossy(&output.stderr).to_string();
216+
if !add_output.status.success() {
217+
return Err(anyhow_to_mcp_error(anyhow::anyhow!(
218+
"git add -A failed: {}",
219+
String::from_utf8_lossy(&add_output.stderr)
220+
)));
222221
}
223222

224-
let mut patch = if let Some(output) = diff_output {
225-
String::from_utf8_lossy(&output.stdout).to_string()
226-
} else {
223+
// Create a temporary commit with the staged changes
224+
let commit_output = Command::new("git")
225+
.args(["commit", "-m", "agent changes", "--allow-empty", "--no-verify"])
226+
.current_dir(&git_dir)
227+
.output()
228+
.await
229+
.map_err(|e| {
230+
anyhow_to_mcp_error(anyhow::anyhow!("Failed to create temporary commit: {}", e))
231+
})?;
232+
233+
if !commit_output.status.success() {
234+
// Reset staging on failure
235+
let _ = Command::new("git")
236+
.args(["reset", "HEAD", "--quiet"])
237+
.current_dir(&git_dir)
238+
.output()
239+
.await;
227240
return Err(anyhow_to_mcp_error(anyhow::anyhow!(
228-
"git diff failed against all targets (origin/main, main, HEAD): {}",
229-
last_error
241+
"Failed to create temporary commit: {}",
242+
String::from_utf8_lossy(&commit_output.stderr)
230243
)));
231-
};
244+
}
232245

233-
// Also include untracked files that have been added
234-
let status_output = Command::new("git")
235-
.args(["status", "--porcelain"])
246+
// Generate format-patch for the last commit (the temporary one)
247+
let format_patch_output = Command::new("git")
248+
.args(["format-patch", "-1", "--stdout", "-M"])
236249
.current_dir(&git_dir)
237250
.output()
238251
.await
239-
.map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git status: {}", e)))?;
252+
.map_err(|e| {
253+
anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git format-patch: {}", e))
254+
})?;
240255

241-
if !status_output.status.success() {
256+
// Undo the temporary commit but keep changes in working tree
257+
let _ = Command::new("git")
258+
.args(["reset", "HEAD~1", "--mixed", "--quiet"])
259+
.current_dir(&git_dir)
260+
.output()
261+
.await;
262+
263+
if !format_patch_output.status.success() {
242264
return Err(anyhow_to_mcp_error(anyhow::anyhow!(
243-
"git status failed: {}",
244-
String::from_utf8_lossy(&status_output.stderr)
265+
"git format-patch failed: {}",
266+
String::from_utf8_lossy(&format_patch_output.stderr)
245267
)));
246268
}
247269

248-
let status = String::from_utf8_lossy(&status_output.stdout);
249-
for line in status.lines() {
250-
if line.starts_with("?? ") {
251-
// Untracked file - generate a diff for it
252-
let file_path = line[3..].trim();
253-
let file_full_path = git_dir.join(file_path);
254-
255-
if file_full_path.is_file() {
256-
if let Ok(content) = tokio::fs::read_to_string(&file_full_path).await {
257-
patch.push_str(&format!("diff --git a/{} b/{}\n", file_path, file_path));
258-
patch.push_str("new file mode 100644\n");
259-
patch.push_str("--- /dev/null\n");
260-
patch.push_str(&format!("+++ b/{}\n", file_path));
261-
262-
let line_count = content.lines().count();
263-
patch.push_str(&format!("@@ -0,0 +1,{} @@\n", line_count));
264-
265-
for line in content.lines() {
266-
patch.push('+');
267-
patch.push_str(line);
268-
patch.push('\n');
269-
}
270-
}
271-
}
272-
}
273-
}
270+
let patch = String::from_utf8_lossy(&format_patch_output.stdout).to_string();
274271

275272
Ok(patch)
276273
}
@@ -407,7 +404,10 @@ fields you want to update."
407404

408405
#[tool(
409406
name = "create-pull-request",
410-
description = "Create a new pull request to propose code changes. Use this after making file edits to submit them for review and merging. The PR will be created from the current branch with your committed changes. Use 'self' for the pipeline's own repository, or a repository alias from the checkout list."
407+
description = "Create a new pull request to propose code changes. Before calling this tool, \
408+
stage and commit your changes using git add and git commit — each logical change should be its \
409+
own commit with a descriptive message. The PR will be created from your committed changes. \
410+
Use 'self' for the pipeline's own repository, or a repository alias from the checkout list."
411411
)]
412412
async fn create_pr(
413413
&self,
@@ -464,6 +464,7 @@ fields you want to update."
464464
source_branch,
465465
patch_filename,
466466
repository.to_string(),
467+
sanitized.labels,
467468
);
468469

469470
// Write to safe outputs
@@ -1035,7 +1036,7 @@ mod tests {
10351036
#[test]
10361037
fn test_generate_short_id_format() {
10371038
let id = generate_short_id();
1038-
assert_eq!(id.len(), 6);
1039+
assert_eq!(id.len(), 8);
10391040
assert!(id.chars().all(|c| c.is_ascii_hexdigit()));
10401041
}
10411042

0 commit comments

Comments
 (0)