Skip to content

Commit 0c42c76

Browse files
Your Nameclaude
andcommitted
fix(diff_impact): implement the documented default unstaged-diff behavior
get_git_diff's "neither staged nor commits" branch returned a hard error instead of ever running plain `git diff` — so the tool's own schema ("staged: false or omitted analyzes the unstaged working-tree diff") was never actually true. Calling diff_impact with no arguments at all (the most natural shape — "check my current uncommitted changes") always failed with INVALID_INPUT/FEATURE_UNAVAILABLE. Implemented the missing branch, and relaxed diff_impact's own input validation from "exactly one of diff/staged/commits" to "at most one" so omitting all three reaches it instead of being rejected first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 9d45c63 commit 0c42c76

3 files changed

Lines changed: 63 additions & 13 deletions

File tree

crates/ci-core/src/analysis/diff_impact.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,12 @@ pub fn get_git_diff(
7676
} else if let Some(range) = commits {
7777
vec!["diff".into(), "-M".into(), range.into()]
7878
} else {
79-
return (
80-
None,
81-
Some("Provide exactly one of staged=true or commits=<range>.".into()),
82-
);
79+
// Neither staged nor a commit range: the unstaged working-tree diff
80+
// (plain `git diff`, no `--cached`) — this is the documented default
81+
// for `staged=false`/omitted, but was previously unimplemented here
82+
// (this branch returned a hard error instead), contradicting the
83+
// tool's own schema description.
84+
vec!["diff".into(), "-M".into()]
8385
};
8486

8587
match run_with_timeout("git", cmd_args, project_root, timeout_secs) {

crates/ci-server/src/tools.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1575,6 +1575,53 @@ mod tests {
15751575
let _ = std::fs::remove_dir_all(&dir);
15761576
}
15771577

1578+
/// Regression: `diff_impact` with all three of `diff`/`staged`/`commits`
1579+
/// omitted must analyze the unstaged working-tree diff (plain `git
1580+
/// diff`), per the tool's own schema description — `get_git_diff`'s
1581+
/// "neither staged nor commits" branch used to return a hard error
1582+
/// instead of ever running plain `git diff`, so this exact case (the
1583+
/// most natural call shape — "just check my current uncommitted
1584+
/// changes") always failed.
1585+
#[test]
1586+
fn diff_impact_with_no_params_analyzes_unstaged_working_tree_diff() {
1587+
fn run_git(dir: &std::path::Path, args: &[&str]) {
1588+
let status = std::process::Command::new("git")
1589+
.args(args)
1590+
.current_dir(dir)
1591+
.status()
1592+
.unwrap();
1593+
assert!(status.success(), "git {args:?} failed");
1594+
}
1595+
1596+
let dir =
1597+
std::env::temp_dir().join(format!("ci_diff_impact_unstaged_{}", std::process::id()));
1598+
let _ = std::fs::remove_dir_all(&dir);
1599+
std::fs::create_dir_all(&dir).unwrap();
1600+
run_git(&dir, &["init", "-q"]);
1601+
run_git(&dir, &["config", "user.email", "test@example.com"]);
1602+
run_git(&dir, &["config", "user.name", "Test"]);
1603+
1604+
std::fs::write(dir.join("foo.rs"), "fn foo() {}\n").unwrap();
1605+
run_git(&dir, &["add", "."]);
1606+
run_git(&dir, &["commit", "-q", "-m", "init"]);
1607+
1608+
// Uncommitted, unstaged change — not `git add`ed.
1609+
std::fs::write(dir.join("foo.rs"), "fn foo() {\n 1\n}\n").unwrap();
1610+
1611+
let server = CodeIntelligenceServer::new(dir.clone(), dir.join("index.db")).unwrap();
1612+
let output = server.diff_impact(DiffImpactParams {
1613+
diff: None,
1614+
staged: None,
1615+
commits: None,
1616+
});
1617+
let v: serde_json::Value = serde_json::from_str(&output).unwrap();
1618+
1619+
assert!(v.get("error").is_none(), "expected success, got error: {v}");
1620+
assert_eq!(v["files_changed"], serde_json::json!(["foo.rs"]));
1621+
1622+
let _ = std::fs::remove_dir_all(&dir);
1623+
}
1624+
15781625
#[test]
15791626
fn session_context_tracks_tool_calls_and_explored_state() {
15801627
let dir = std::env::temp_dir().join(format!("ci_session_ctx_{}", std::process::id()));

crates/ci-server/src/tools/guardrails.rs

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -153,18 +153,18 @@ impl CodeIntelligenceServer {
153153

154154
#[tool(
155155
name = "diff_impact",
156-
description = "CALL THIS after every code change, BEFORE commit or push — never skip. USE WHEN: you have uncommitted changes and want to verify blast radius. NOT FOR: pre-edit analysis (use edit_context). vs edit_context: edit_context=pre-edit; diff_impact=post-edit. Provide exactly one of: diff, staged, commits."
156+
description = "CALL THIS after every code change, BEFORE commit or push — never skip. USE WHEN: you have uncommitted changes and want to verify blast radius. NOT FOR: pre-edit analysis (use edit_context). vs edit_context: edit_context=pre-edit; diff_impact=post-edit. Omit all three for the unstaged working-tree diff, or provide at most one of: diff, staged=true, commits=<range>."
157157
)]
158158
pub(crate) fn diff_impact(&self, #[tool(aggr)] p: DiffImpactParams) -> String {
159159
self.timed_tool("diff_impact", || {
160160
self.clear_written_files();
161161

162162
let input_count =
163163
p.diff.is_some() as u8 + p.staged.is_some() as u8 + p.commits.is_some() as u8;
164-
if input_count != 1 {
164+
if input_count > 1 {
165165
return error_json(
166166
"INVALID_INPUT",
167-
"Exactly one of diff, staged, or commits must be provided",
167+
"At most one of diff, staged, or commits may be provided (omit all three for the unstaged working-tree diff)",
168168
false,
169169
);
170170
}
@@ -537,18 +537,19 @@ pub(crate) struct EditContextOutput {
537537
#[derive(Deserialize, JsonSchema)]
538538
pub(crate) struct DiffImpactParams {
539539
/// A raw unified diff (`git diff` output) to analyze directly, instead
540-
/// of having this tool run git itself. Exactly one of `diff`, `staged`,
541-
/// `commits` must be set.
540+
/// of having this tool run git itself. At most one of `diff`, `staged`,
541+
/// `commits` may be set — omitting all three analyzes the unstaged
542+
/// working-tree diff (plain `git diff`).
542543
#[serde(skip_serializing_if = "Option::is_none")]
543544
pub(crate) diff: Option<String>,
544545
/// `true` to analyze the staged diff (`git diff --cached`); `false` or
545-
/// omitted analyzes the unstaged working-tree diff. Exactly one of
546-
/// `diff`, `staged`, `commits` must be set.
546+
/// omitted analyzes the unstaged working-tree diff. At most one of
547+
/// `diff`, `staged`, `commits` may be set.
547548
#[serde(skip_serializing_if = "Option::is_none")]
548549
pub(crate) staged: Option<bool>,
549550
/// A commit range/rev understood by `git diff`, e.g. `HEAD~3..HEAD` or
550-
/// a single commit SHA. Exactly one of `diff`, `staged`, `commits`
551-
/// must be set.
551+
/// a single commit SHA. At most one of `diff`, `staged`, `commits`
552+
/// may be set.
552553
#[serde(skip_serializing_if = "Option::is_none")]
553554
pub(crate) commits: Option<String>,
554555
}

0 commit comments

Comments
 (0)