Skip to content

Commit ecc9b73

Browse files
committed
fix: a dependency bump and a quiet hook say what they are
Impact dropped lock files from every count, so a commit that only moved dependency versions answered "No changes to analyse" (#495): the answer now names them in lockfile_changes and is never empty for them. The hook printed nothing for an empty review and for a commit it could not read, so a clean commit looked like a hook that never ran and the SessionStart rule asked for a manual rerun every time (#464): before a commit or push it now says one line, "reaches nothing outside the diff" or "did not analyse this command" with the command to run; the log keeps a session. Gates, red on fd07858: a lock-only commit is named and not empty; an empty commit, a subshell git and an unresolvable cd each get their line. Refs: #495 #464, sweep root A (#505) Claude-Session: https://claude.ai/code/session_01Fzbq9adLJyZGLxqHrCiW29
1 parent 4b34482 commit ecc9b73

6 files changed

Lines changed: 154 additions & 21 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5353

5454
### Fixed
5555

56+
- A dependency bump is a change: impact names the lock files a range
57+
changes (`lockfile_changes`) and never answers "No changes to analyse" for
58+
them (#495).
59+
- Before a commit or push the hook says what it did when it has no impact
60+
to show: one line when the change reaches nothing outside the diff, and
61+
one when it could not read the command (a git inside a subshell, a `cd`
62+
it cannot resolve), with the command to run instead. Silence then means
63+
the hook did not run, not a clean review; the hook log keeps tens of
64+
thousands of runs so an audit reads a whole session (#464).
5665
- The MCP tool timed out at 300 s on a monorepo range the CLI answered in
5766
four seconds (#457), for two reasons. It widened a subdirectory
5867
`repo_path` to the whole repository; it now analyses the directory asked

‎crates/diffctx-native/src/hook.rs‎

Lines changed: 61 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@
88
//! agent already walks, with nothing new to decide. After a text search
99
//! for a name the pending change edits, that name's callers.
1010
//!
11-
//! Each reads the event on stdin and answers with `additionalContext`, or
12-
//! with nothing when the impact is empty, the same content was already
13-
//! reviewed, or anything at all goes wrong. A hook that blocks a commit
11+
//! Each reads the event on stdin and answers with `additionalContext`: the
12+
//! impact, or one line before a commit or push when it is empty or the line
13+
//! could not be read (#464); nothing when the same content was already
14+
//! reviewed or anything at all goes wrong. A hook that blocks a commit
1415
//! because it failed would be worse than no hook; the one deliberate block,
1516
//! the opt-in strict gate, denies with the impact attached so the retry
1617
//! passes.
@@ -1656,6 +1657,8 @@ struct Trace {
16561657
verb: Option<&'static str>,
16571658
root: Option<PathBuf>,
16581659
range: Option<String>,
1660+
/// Why a one-line answer was given in place of a review, for the log.
1661+
noted: Option<Silence>,
16591662
}
16601663

16611664
/// Why the hook stayed silent, as its log line names it; `shown` otherwise.
@@ -2341,7 +2344,32 @@ fn decide(
23412344
.map(|c| hook_json(event, &c, false));
23422345
}
23432346
}
2344-
let prepared = prepare(event, &payload, trace)?;
2347+
let prepared = match prepare(event, &payload, trace) {
2348+
Ok(p) => p,
2349+
// A commit or push the hook could not read is said so, so that no
2350+
// block means the hook did not run, never "reviewed, nothing" (#464).
2351+
Err(why @ ("unsupported-syntax" | "unresolved-dir"))
2352+
if event == Event::PreToolUse
2353+
&& payload["tool_input"]["command"]
2354+
.as_str()
2355+
.is_some_and(names_a_recording_verb) =>
2356+
{
2357+
trace.noted = Some(why);
2358+
let how = if why == "unresolved-dir" {
2359+
"it changes into a directory the hook cannot resolve"
2360+
} else {
2361+
"its git runs inside a subshell, eval or command substitution"
2362+
};
2363+
return Ok(hook_json(
2364+
event,
2365+
&format!(
2366+
"diffctx did not analyse this command: {how}. Run `diffctx <repo> --diff --mode impact` in that repository before going on."
2367+
),
2368+
false,
2369+
));
2370+
}
2371+
Err(why) => return Err(why),
2372+
};
23452373
if event == Event::PreToolUse && is_seen(&prepared.record_key) {
23462374
return Err("seen");
23472375
}
@@ -2391,11 +2419,32 @@ fn decide(
23912419
if event == Event::PreToolUse {
23922420
mark_seen(&record_key);
23932421
}
2394-
answer.map(|a| hook_json(event, &a.context, gate && !a.partial))
2422+
match answer {
2423+
// One line, so that no block means the hook did not run (#464).
2424+
Err("empty") if event == Event::PreToolUse => {
2425+
trace.noted = Some("empty");
2426+
let range = trace.range.as_deref().unwrap_or("this change");
2427+
Ok(hook_json(
2428+
event,
2429+
&format!("diffctx: {range} reaches nothing outside the diff."),
2430+
false,
2431+
))
2432+
}
2433+
answer => answer.map(|a| hook_json(event, &a.context, gate && !a.partial)),
2434+
}
23952435
}
23962436

2397-
/// The log is cut over to `hook.log.1` past this size: a few thousand runs.
2398-
const LOG_MAX_BYTES: u64 = 256 * 1024;
2437+
/// A line that records or publishes commits: the events whose silence would
2438+
/// otherwise read as a clean review.
2439+
fn names_a_recording_verb(command: &str) -> bool {
2440+
let words = || command.split(|c: char| !(c.is_alphanumeric() || c == '-'));
2441+
words().any(|w| w == "git")
2442+
&& words().any(|w| matches!(w, "commit" | "push" | "merge" | "cherry-pick"))
2443+
}
2444+
2445+
/// The log is cut over to `hook.log.1` past this size: tens of thousands of
2446+
/// runs, so an audit reads a whole session (#464).
2447+
const LOG_MAX_BYTES: u64 = 4 * 1024 * 1024;
23992448

24002449
/// One line per run in `<cache>/diffctx/hook.log` (#338): Claude Code keeps
24012450
/// only hooks that print, so without it a silent hook that decided "nothing
@@ -2441,12 +2490,11 @@ pub fn respond_within_deadline(event: Event, stdin_json: String, gate: bool) ->
24412490
let hook_run = crate::resource::RunContext::for_run(HOOK_DEADLINE_SECS);
24422491
let _in_hook = hook_run.enter();
24432492
let answer = decide(event, &stdin_json, gate, started, &mut trace);
2444-
log_outcome(
2445-
event,
2446-
&trace,
2447-
answer.as_ref().err().copied().unwrap_or("shown"),
2448-
started.elapsed(),
2449-
);
2493+
let outcome = match &answer {
2494+
Ok(_) => trace.noted.unwrap_or("shown"),
2495+
Err(why) => why,
2496+
};
2497+
log_outcome(event, &trace, outcome, started.elapsed());
24502498
answer.ok()
24512499
}
24522500

‎crates/diffctx-native/src/impact.rs‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ pub struct ImpactOutput {
150150
/// read "no resolved static callers" while the function had three (#498).
151151
#[serde(default, skip_serializing_if = "Vec::is_empty")]
152152
pub layout_only: Vec<String>,
153+
/// Lock files the range changes: dependency versions moved, and what that
154+
/// reaches is not traced. Dropped from every count, a dependency bump
155+
/// answered "No changes to analyse" (#495).
156+
#[serde(default, skip_serializing_if = "Vec::is_empty")]
157+
pub lockfile_changes: Vec<String>,
153158
/// `layout_only` entries left out to fit the cap; the count is whole.
154159
#[serde(default, skip_serializing_if = "crate::render::is_zero")]
155160
pub layout_only_omitted: usize,
@@ -2569,7 +2574,8 @@ pub fn build_impact(
25692574
&& contracts.is_empty()
25702575
&& limits.is_empty()
25712576
&& stale.is_empty()
2572-
&& unanalysed.is_empty();
2577+
&& unanalysed.is_empty()
2578+
&& state.lockfile_changes.is_empty();
25732579
ImpactOutput {
25742580
schema: IMPACT_SCHEMA,
25752581
name: state
@@ -2597,6 +2603,7 @@ pub fn build_impact(
25972603
untested_unlisted,
25982604
untested_called_in_diff,
25992605
layout_only,
2606+
lockfile_changes: state.lockfile_changes.clone(),
26002607
layout_only_omitted: 0,
26012608
traced_definitions,
26022609
withheld_files: state.policy_excluded_count + state.run.withheld_count(),
@@ -3325,7 +3332,8 @@ fn fit_with(output: &mut ImpactOutput, cap: u32, measure: &dyn Fn(&ImpactOutput)
33253332
&& output.truncated == 0
33263333
&& output.contracts_omitted == 0
33273334
&& output.limits.is_empty()
3328-
&& output.unanalysed_files.is_empty();
3335+
&& output.unanalysed_files.is_empty()
3336+
&& output.lockfile_changes.is_empty();
33293337
}
33303338

33313339
/// One step of shedding, least needed first; `false` once nothing more can
@@ -3932,6 +3940,13 @@ pub fn layout_only_names(output: &ImpactOutput) -> impl Iterator<Item = &str> {
39323940
}
39333941

39343942
fn write_tail(out: &mut String, output: &ImpactOutput) {
3943+
write_list(
3944+
out,
3945+
"Lock files changed (dependency versions moved; what that reaches is not traced):",
3946+
&output.lockfile_changes,
3947+
usize::MAX,
3948+
0,
3949+
);
39353950
write_layout_only(out, output);
39363951
write_untested_unlisted(out, output);
39373952
let stale: Vec<String> = output

‎crates/diffctx-native/tests/hook.rs‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,11 @@ fn comments_subshell_cds_cd_dash_and_heredoc_messages_keep_their_commit() {
302302
cache.path(),
303303
&payload(repo, "cd - && git commit -am vat"),
304304
);
305-
assert_eq!(h.stdout, "", "cd - names no directory");
305+
assert!(
306+
h.stdout.contains("did not analyse this command"),
307+
"cd - names no directory, and says so: {}",
308+
h.stdout
309+
);
306310

307311
let other = repo_with_pending_change();
308312
write(
@@ -393,16 +397,24 @@ fn a_commit_of_unparsed_code_says_it_was_not_analysed() {
393397
);
394398
}
395399

400+
/// #464: an empty answer is one line, so that no block means the hook did
401+
/// not run, never "reviewed, nothing".
396402
#[test]
397-
fn a_change_nothing_depends_on_is_silence() {
403+
fn a_change_nothing_depends_on_says_so_in_one_line() {
398404
let tmp = repo_with_pending_change();
399405
let cache = TempDir::new().unwrap();
400406
let repo = tmp.path();
401407
git(repo, &["checkout", "-q", "--", "shop/pricing.py"]);
402408
write(repo, "README.md", "# shop\n\nVAT included.\n");
403409
let h = hook(repo, cache.path(), &payload(repo, "git commit -am docs"));
404410
assert_eq!(h.code, Some(0));
405-
assert_eq!(h.stdout, "");
411+
assert!(
412+
h.stdout.contains("reaches nothing outside the diff"),
413+
"{}",
414+
h.stdout
415+
);
416+
assert!(!h.stdout.contains("permissionDecision"), "{}", h.stdout);
417+
assert!(last_log_line(cache.path()).contains(" empty "));
406418
}
407419

408420
#[test]
@@ -986,7 +998,12 @@ fn the_strict_gate_denies_once_with_the_impact_attached() {
986998
&["--gate"],
987999
&payload(repo, "git commit -am docs"),
9881000
);
989-
assert_eq!(docs.stdout, "");
1001+
assert!(
1002+
!docs.stdout.contains("permissionDecision"),
1003+
"{}",
1004+
docs.stdout
1005+
);
1006+
assert!(docs.stdout.contains("reaches nothing"), "{}", docs.stdout);
9901007
}
9911008

9921009
#[test]
@@ -1338,7 +1355,11 @@ fn a_git_command_hidden_in_a_subshell_is_logged_as_unsupported() {
13381355
cache.path(),
13391356
&payload(repo, "sh -c 'git commit -am vat'"),
13401357
);
1341-
assert_eq!(h.stdout, "");
1358+
assert!(
1359+
h.stdout.contains("did not analyse this command"),
1360+
"{}",
1361+
h.stdout
1362+
);
13421363
let log = std::fs::read_to_string(cache.path().join("diffctx").join("hook.log")).unwrap();
13431364
assert!(log.contains("unsupported-syntax"), "{log}");
13441365
}
@@ -1565,7 +1586,11 @@ fn an_empty_answer_stays_empty_after_the_commit_lands() {
15651586
write(repo, "README.md", "# shop\n\nmore\n");
15661587
let command = "git add README.md && git commit -qm readme";
15671588
let pre = hook(repo, cache.path(), &payload(repo, command));
1568-
assert_eq!(pre.stdout, "");
1589+
assert!(
1590+
pre.stdout.contains("reaches nothing outside the diff"),
1591+
"{}",
1592+
pre.stdout
1593+
);
15691594
git(repo, &["add", "README.md"]);
15701595
git(repo, &["commit", "-qm", "readme"]);
15711596
inspect(repo, cache.path(), command);

‎crates/diffctx-native/tests/impact.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1536,6 +1536,35 @@ fn a_removed_name_too_common_to_trace_is_unresolved_not_listed() {
15361536
);
15371537
}
15381538

1539+
/// #495: a dependency bump is a change: the lock files are named and the
1540+
/// answer is never empty, where it read "No changes to analyse".
1541+
#[test]
1542+
fn a_lock_file_change_is_named_never_empty() {
1543+
let tmp = repo_of(
1544+
&[
1545+
("app.py", "def f():\n return 1\n"),
1546+
("uv.lock", "x==1\n"),
1547+
("Cargo.lock", "[package]\nname = \"x\"\n"),
1548+
],
1549+
&[
1550+
("uv.lock", "x==2\n"),
1551+
("Cargo.lock", "[package]\nname = \"y\"\n"),
1552+
],
1553+
);
1554+
let doc = impact(tmp.path(), "HEAD~1..HEAD");
1555+
assert_eq!(doc["empty"], false, "{doc:#}");
1556+
let locks: Vec<&str> = doc["lockfile_changes"]
1557+
.as_array()
1558+
.into_iter()
1559+
.flatten()
1560+
.filter_map(|v| v.as_str())
1561+
.collect();
1562+
assert_eq!(locks, ["Cargo.lock", "uv.lock"], "{doc:#}");
1563+
let md = impact_markdown(tmp.path(), "HEAD~1..HEAD");
1564+
assert!(!md.contains("No changes to analyse"), "{md}");
1565+
assert!(md.contains(" - uv.lock"), "{md}");
1566+
}
1567+
15391568
/// #387: callers that could not be resolved are not "0 callers".
15401569
#[test]
15411570
fn unresolved_callers_are_not_reported_as_none() {

‎schemas/diffctx.impact.v1.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,13 @@
132132
"type": "string"
133133
}
134134
},
135+
"lockfile_changes": {
136+
"description": "Lock files the range changes: dependency versions moved, and what that\nreaches is not traced. Dropped from every count, a dependency bump\nanswered \"No changes to analyse\" (#495).",
137+
"type": "array",
138+
"items": {
139+
"type": "string"
140+
}
141+
},
135142
"layout_only_omitted": {
136143
"description": "`layout_only` entries left out to fit the cap; the count is whole.",
137144
"type": "integer",

0 commit comments

Comments
 (0)