mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-18 09:06:53 +03:00
Fix: distinguish existing Stop findings from new debt (#754)
Use verified first-edit baselines to distinguish pre-existing text findings from new or unknown Stop findings. Preserve dirty worktrees, bound notice rendering, and keep explicit scans unchanged. Verified with the full Rust and Bun/Node suites and real Claude Code edit-to-Stop sessions. Related to #522; keep it open until an engine release ships the fix. AI assistance: Codex, under maintainer direction.
This commit is contained in:
+64
-10
@@ -7,6 +7,7 @@ use impeccable_core::js;
|
||||
use serde_json::{Map, Value};
|
||||
|
||||
use crate::hook_lib::*;
|
||||
use crate::stop_baseline;
|
||||
use crate::util::{
|
||||
exists, iso_now, jsp, node_read_error, now_ms, str_field, truthy_value, utf16_len,
|
||||
};
|
||||
@@ -237,11 +238,19 @@ pub fn run_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
}
|
||||
}
|
||||
|
||||
let use_html_engine = match configured {
|
||||
Some(c) => c.engine == "html",
|
||||
None => ext == ".html" || ext == ".htm",
|
||||
};
|
||||
if primary_files.contains(file_path) {
|
||||
if harness == "claude" {
|
||||
stop_baseline::capture(rt, &event, &mut cache, &session_id, file_path, use_html_engine);
|
||||
}
|
||||
let edit_count = bump_edit_count(&mut cache, &session_id, file_path);
|
||||
cache_dirty = true;
|
||||
audit.insert("editCount".into(), Value::from(edit_count as u64));
|
||||
if edit_count > EDIT_COUNT_THRESHOLD as f64 {
|
||||
stop_baseline::invalidate(&mut cache, &session_id, file_path);
|
||||
let just_crossed = edit_count == (EDIT_COUNT_THRESHOLD + 1) as f64;
|
||||
if just_crossed && suppression_winner.is_none() {
|
||||
suppression_winner = Some(file_path.clone());
|
||||
@@ -266,10 +275,6 @@ pub fn run_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
};
|
||||
}
|
||||
};
|
||||
let use_html_engine = match configured {
|
||||
Some(c) => c.engine == "html",
|
||||
None => ext == ".html" || ext == ".htm",
|
||||
};
|
||||
let mut detector_threw = false;
|
||||
let findings: Vec<Finding> = if use_html_engine {
|
||||
match detector_detect_html(rt, file_path, &scan) {
|
||||
@@ -282,6 +287,9 @@ pub fn run_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
} else {
|
||||
detector_detect_text(&content, file_path, &scan)
|
||||
};
|
||||
if !detector_threw && !use_html_engine {
|
||||
stop_baseline::reconcile(&mut cache, &session_id, file_path, &findings);
|
||||
}
|
||||
let raw_count = findings.len();
|
||||
let filtered = filter_findings(findings, &config);
|
||||
let (immediate, deferred) = if tiered {
|
||||
@@ -660,6 +668,9 @@ pub fn run_stop_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
let mut fresh_groups: Vec<Group> = Vec::new();
|
||||
let mut scanned = 0usize;
|
||||
let mut cache_dirty = false;
|
||||
let mut pre_existing = 0usize;
|
||||
let mut new_findings = 0usize;
|
||||
let mut unknown = 0usize;
|
||||
for file_path in &touched {
|
||||
if scanned >= STOP_MAX_FILES {
|
||||
break;
|
||||
@@ -705,8 +716,15 @@ pub fn run_stop_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
} else {
|
||||
detector_detect_text(&content, file_path, &scan)
|
||||
};
|
||||
if !use_html_engine {
|
||||
stop_baseline::reconcile(&mut cache, &session_id, file_path, &findings);
|
||||
}
|
||||
let filtered = filter_findings(findings, &config);
|
||||
let fresh = dedupe_against_cache(&filtered, &mut cache, &session_id, file_path);
|
||||
let classified = stop_baseline::classify(&cache, &session_id, file_path, use_html_engine, filtered.clone());
|
||||
pre_existing += classified.pre_existing;
|
||||
new_findings += classified.new;
|
||||
unknown += classified.unknown;
|
||||
let fresh = dedupe_against_cache(&classified.findings, &mut cache, &session_id, file_path);
|
||||
// JS: sync to the live scan, including empty. Remembering only
|
||||
// `fresh` (or skipping the write on a clean Stop) left stale keys in
|
||||
// place, so a finding that was fixed and later reintroduced never
|
||||
@@ -721,6 +739,9 @@ pub fn run_stop_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
}
|
||||
}
|
||||
audit.insert("scannedFiles".into(), Value::from(scanned));
|
||||
audit.insert("preExistingFindings".into(), Value::from(pre_existing));
|
||||
audit.insert("newFindings".into(), Value::from(new_findings));
|
||||
audit.insert("unknownFindings".into(), Value::from(unknown));
|
||||
if fresh_groups.is_empty() {
|
||||
if cache_dirty {
|
||||
persist_cache(rt, &project_cwd, &cache);
|
||||
@@ -735,19 +756,52 @@ pub fn run_stop_hook(rt: &Runtime, stdin: &str) -> RunResult {
|
||||
);
|
||||
}
|
||||
let short = footer_mode_short(&mut cache, &session_id);
|
||||
let reserve = design_note_reserve(rt, &scan, &mut cache, &session_id);
|
||||
let rendered = render_grouped_template(
|
||||
let first_unknown = fresh_groups.iter().flat_map(|group| &group.findings)
|
||||
.position(|f| f.name.starts_with("[attribution unknown]"));
|
||||
let mut attribution_note = if first_unknown.is_some() {
|
||||
format!("{ENVELOPE_PREFIX} {}", stop_baseline::UNKNOWN_NOTE)
|
||||
} else {
|
||||
String::new()
|
||||
};
|
||||
// Findings and attribution take priority. Append the lower-priority stale
|
||||
// DESIGN.md notice only if it fits, without consuming its session flag.
|
||||
let render = |note: &str, render_config: &HookConfig| render_grouped_template(
|
||||
rt,
|
||||
&fresh_groups,
|
||||
&config,
|
||||
render_config,
|
||||
&RenderOpts {
|
||||
cwd: Some(project_cwd.clone()),
|
||||
short_footer: short,
|
||||
reserve_chars: reserve,
|
||||
reserve_chars: if note.is_empty() { 0.0 } else { (utf16_len(note) + 2) as f64 },
|
||||
},
|
||||
);
|
||||
let mut rendered = render(&attribution_note, &config);
|
||||
if !attribution_note.is_empty() && !rendered.lines().any(|line| {
|
||||
line.starts_with("- ") && (line.contains("[attribution unknown]") || line.contains("[new]"))
|
||||
}) {
|
||||
// At the minimum budget, a grouped header and policy footer may crowd
|
||||
// out even the first finding. Shorten the notice before losing it.
|
||||
attribution_note = format!("{ENVELOPE_PREFIX} {}", stop_baseline::COMPACT_UNKNOWN_NOTE);
|
||||
rendered = render(&attribution_note, &config);
|
||||
}
|
||||
// maxFindings / maxChars may also remove all unknown findings. Do not
|
||||
// attach their guidance to an output that only shows confirmed new debt.
|
||||
let shows_unknown = rendered.lines().any(|line| {
|
||||
line.starts_with("- ") && line.contains("[attribution unknown]")
|
||||
});
|
||||
if !shows_unknown {
|
||||
if let Some(prefix @ 1..) = first_unknown {
|
||||
// Reclaim the unused notice budget for the known-new prefix.
|
||||
// Keep the unknown suffix omitted: simply expanding the budget
|
||||
// could reveal an unknown finding without its required guidance.
|
||||
let mut visible_config = config.clone();
|
||||
visible_config.limits.max_findings = cap_of(&config).min(prefix) as f64;
|
||||
rendered = render("", &visible_config);
|
||||
}
|
||||
}
|
||||
let text = if shows_unknown { format!("{attribution_note}\n\n{rendered}") } else { rendered };
|
||||
let text =
|
||||
append_design_system_note_once(rt, &rendered, &scan, &mut cache, &session_id, &config);
|
||||
append_design_system_note_once(rt, &text, &scan, &mut cache, &session_id, &config);
|
||||
commit_footer_shown(rt, &mut cache, &session_id, &text);
|
||||
persist_cache(rt, &project_cwd, &cache);
|
||||
let all: usize = fresh_groups.iter().map(|g| g.findings.len()).sum();
|
||||
|
||||
@@ -1314,7 +1314,7 @@ pub struct RenderOpts {
|
||||
pub reserve_chars: f64,
|
||||
}
|
||||
|
||||
fn cap_of(config: &HookConfig) -> usize {
|
||||
pub(crate) fn cap_of(config: &HookConfig) -> usize {
|
||||
let mf = config.limits.max_findings;
|
||||
let mf = if mf == 0.0 || mf.is_nan() {
|
||||
DEFAULT_MAX_FINDINGS
|
||||
|
||||
@@ -14,6 +14,7 @@ pub mod admin;
|
||||
pub mod before_edit;
|
||||
pub mod hook;
|
||||
pub mod hook_lib;
|
||||
mod stop_baseline;
|
||||
pub mod util;
|
||||
|
||||
use impeccable_common::Io;
|
||||
|
||||
@@ -0,0 +1,279 @@
|
||||
//! Conservative Stop attribution. Never use HEAD as a session baseline: the
|
||||
//! working tree may already be dirty. Only a verified first Edit/Write preimage
|
||||
//! from the tool result establishes a baseline, and only for the pure text
|
||||
//! detector. DOM and design-system findings can depend on other files.
|
||||
|
||||
use impeccable_core::findings::Finding;
|
||||
use serde_json::{json, Map, Value};
|
||||
use sha2::{Digest, Sha256};
|
||||
|
||||
use crate::hook_lib::{
|
||||
detector_detect_text, ensure_file, sessions, Cache, HookScanOptions, Runtime,
|
||||
};
|
||||
|
||||
const FIELD: &str = "stopBaseline";
|
||||
const MAX_BYTES: usize = 512 * 1024;
|
||||
const MAX_FINDINGS: usize = 256;
|
||||
pub const UNKNOWN_NOTE: &str = "Findings marked attribution unknown may predate this session; do not treat them as regressions or broaden the task without asking.";
|
||||
pub const COMPACT_UNKNOWN_NOTE: &str = "Unknown findings may predate this session; ask before expanding scope.";
|
||||
|
||||
fn independent(finding: &Finding) -> bool {
|
||||
!finding.antipattern.starts_with("design-system-")
|
||||
}
|
||||
|
||||
// Exclude line numbers so an unrelated insertion/deletion does not make old
|
||||
// debt new. Preserve multiplicity so adding an identical occurrence is new.
|
||||
// Hash the detector identity rather than storing source or snippets in cache.
|
||||
fn key(finding: &Finding) -> String {
|
||||
let identity = json!([finding.antipattern, finding.snippet, finding.extras]);
|
||||
format!("{:x}", Sha256::digest(identity.to_string().as_bytes()))
|
||||
}
|
||||
|
||||
fn counts(findings: &[Finding]) -> Map<String, Value> {
|
||||
let mut counts = Map::new();
|
||||
for finding in findings.iter().filter(|f| independent(f)) {
|
||||
let key = key(finding);
|
||||
let count = counts.get(&key).and_then(Value::as_u64).unwrap_or(0);
|
||||
counts.insert(key, Value::from(count + 1));
|
||||
}
|
||||
counts
|
||||
}
|
||||
|
||||
fn entry<'a>(cache: &'a Cache, session: &str, file: &str) -> Option<&'a Map<String, Value>> {
|
||||
sessions(cache)?
|
||||
.get(session)?
|
||||
.get("files")?
|
||||
.get(file)?
|
||||
.as_object()
|
||||
}
|
||||
|
||||
fn baseline(cache: &Cache, session: &str, file: &str) -> Option<Map<String, Value>> {
|
||||
let value = entry(cache, session, file)?.get(FIELD)?;
|
||||
if value.get("version")?.as_u64()? != 1
|
||||
|| value.get("engine")?.as_str()? != env!("CARGO_PKG_VERSION")
|
||||
{
|
||||
return None;
|
||||
}
|
||||
let counts = value.get("counts")?.as_object()?;
|
||||
if counts.len() > MAX_FINDINGS
|
||||
|| counts.iter().any(|(k, v)| {
|
||||
k.len() != 64
|
||||
|| !k.bytes().all(|b| b.is_ascii_hexdigit())
|
||||
|| !matches!(v.as_u64(), Some(1..=256))
|
||||
})
|
||||
{
|
||||
return None;
|
||||
}
|
||||
Some(counts.clone())
|
||||
}
|
||||
|
||||
/// Called before the first primary edit is recorded. An entry without a
|
||||
/// baseline (old cache, co-scan, incomplete payload) must stay unknown rather
|
||||
/// than adopting a later, already-edited file as its starting point.
|
||||
pub fn capture(
|
||||
rt: &Runtime,
|
||||
event: &Map<String, Value>,
|
||||
cache: &mut Cache,
|
||||
session: &str,
|
||||
file: &str,
|
||||
html: bool,
|
||||
) {
|
||||
if html || session.is_empty() || session == "unknown" || entry(cache, session, file).is_some() {
|
||||
return;
|
||||
}
|
||||
let Some(response) = event.get("tool_response").and_then(Value::as_object) else {
|
||||
return;
|
||||
};
|
||||
let Some(path) = response.get("filePath").and_then(Value::as_str) else {
|
||||
return;
|
||||
};
|
||||
let cwd = event
|
||||
.get("cwd")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or(&rt.proc_cwd);
|
||||
if rt.resolve(&[cwd, path]) != file || response.get("userModified") == Some(&Value::Bool(true))
|
||||
{
|
||||
return;
|
||||
}
|
||||
let Some(tool) = event.get("tool_name").and_then(Value::as_str) else {
|
||||
return;
|
||||
};
|
||||
let original = match response.get("originalFile") {
|
||||
Some(Value::String(text)) if text.len() <= MAX_BYTES => text.as_str(),
|
||||
// null on an update may mean "too large", not an empty original.
|
||||
Some(Value::Null)
|
||||
if tool == "Write"
|
||||
&& response.get("type").and_then(Value::as_str) == Some("create") =>
|
||||
{
|
||||
""
|
||||
}
|
||||
_ => return,
|
||||
};
|
||||
let expected = match tool {
|
||||
"Edit" => {
|
||||
let Some(old) = response.get("oldString").and_then(Value::as_str) else {
|
||||
return;
|
||||
};
|
||||
let Some(new) = response.get("newString").and_then(Value::as_str) else {
|
||||
return;
|
||||
};
|
||||
if old.is_empty() || new.len() > MAX_BYTES || !original.contains(old) {
|
||||
return;
|
||||
}
|
||||
match response.get("replaceAll").and_then(Value::as_bool) {
|
||||
Some(true) => {
|
||||
let occurrences = original.matches(old).count();
|
||||
let size = original.len() - occurrences * old.len()
|
||||
+ occurrences.saturating_mul(new.len());
|
||||
if size > MAX_BYTES {
|
||||
return;
|
||||
}
|
||||
original.replace(old, new)
|
||||
}
|
||||
Some(false) if original.matches(old).count() == 1 => original.replacen(old, new, 1),
|
||||
_ => return,
|
||||
}
|
||||
}
|
||||
"Write" => {
|
||||
if !matches!(
|
||||
response.get("type").and_then(Value::as_str),
|
||||
Some("create" | "update")
|
||||
) {
|
||||
return;
|
||||
}
|
||||
let Some(content) = response.get("content").and_then(Value::as_str) else {
|
||||
return;
|
||||
};
|
||||
if content.len() > MAX_BYTES {
|
||||
return;
|
||||
}
|
||||
content.to_string()
|
||||
}
|
||||
_ => return,
|
||||
};
|
||||
if expected.len() > MAX_BYTES
|
||||
|| std::fs::metadata(file)
|
||||
.map(|m| m.len() > MAX_BYTES as u64)
|
||||
.unwrap_or(true)
|
||||
{
|
||||
return;
|
||||
}
|
||||
// A formatter, stale event, or concurrent write invalidates attribution.
|
||||
if std::fs::read_to_string(file).ok().as_deref() != Some(expected.as_str()) {
|
||||
return;
|
||||
}
|
||||
let findings = detector_detect_text(original, file, &HookScanOptions::default());
|
||||
if findings.len() > MAX_FINDINGS {
|
||||
return;
|
||||
}
|
||||
ensure_file(cache, session, file).insert(
|
||||
FIELD.into(),
|
||||
json!({
|
||||
"version": 1, "engine": env!("CARGO_PKG_VERSION"), "counts": counts(&findings),
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
/// Once existing debt disappears, it cannot exempt a later reintroduction.
|
||||
pub fn reconcile(cache: &mut Cache, session: &str, file: &str, findings: &[Finding]) {
|
||||
let Some(mut old) = baseline(cache, session, file) else {
|
||||
return;
|
||||
};
|
||||
let current = counts(findings);
|
||||
old.retain(|key, value| {
|
||||
let count = current
|
||||
.get(key)
|
||||
.and_then(Value::as_u64)
|
||||
.unwrap_or(0)
|
||||
.min(value.as_u64().unwrap_or(0));
|
||||
*value = Value::from(count);
|
||||
count > 0
|
||||
});
|
||||
ensure_file(cache, session, file).get_mut(FIELD).unwrap()["counts"] = Value::Object(old);
|
||||
}
|
||||
|
||||
/// Do not retain an exemption through edits we deliberately stop scanning.
|
||||
pub fn invalidate(cache: &mut Cache, session: &str, file: &str) {
|
||||
ensure_file(cache, session, file).remove(FIELD);
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
pub struct Classified {
|
||||
pub findings: Vec<Finding>,
|
||||
pub pre_existing: usize,
|
||||
pub new: usize,
|
||||
pub unknown: usize,
|
||||
}
|
||||
|
||||
pub fn classify(
|
||||
cache: &Cache,
|
||||
session: &str,
|
||||
file: &str,
|
||||
html: bool,
|
||||
findings: Vec<Finding>,
|
||||
) -> Classified {
|
||||
let mut baseline = if html {
|
||||
None
|
||||
} else {
|
||||
baseline(cache, session, file)
|
||||
};
|
||||
let mut result = Classified::default();
|
||||
for mut finding in findings {
|
||||
let known = baseline.as_mut().filter(|_| independent(&finding));
|
||||
if let Some(counts) = known {
|
||||
let key = key(&finding);
|
||||
let count = counts.get(&key).and_then(Value::as_u64).unwrap_or(0);
|
||||
if count > 0 {
|
||||
counts.insert(key, Value::from(count - 1));
|
||||
result.pre_existing += 1;
|
||||
continue;
|
||||
}
|
||||
result.new += 1;
|
||||
finding.name = format!("[new] {}", finding.name);
|
||||
} else {
|
||||
result.unknown += 1;
|
||||
finding.name = format!("[attribution unknown] {}", finding.name);
|
||||
}
|
||||
result.findings.push(finding);
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use impeccable_core::findings::finding;
|
||||
|
||||
#[test]
|
||||
fn stop_baseline_dependency_sensitive_findings_stay_unknown() {
|
||||
let mut cache = Cache::new();
|
||||
let font = finding("design-system-font", "a.css", "font-family: serif", 1.0);
|
||||
let css = finding("side-tab", "a.html", "border-left: 4px solid red", 1.0);
|
||||
ensure_file(&mut cache, "s", "a.css").insert(
|
||||
FIELD.into(),
|
||||
json!({
|
||||
"version": 1, "engine": env!("CARGO_PKG_VERSION"), "counts": {},
|
||||
}),
|
||||
);
|
||||
// Even a known text preimage cannot establish the state of DESIGN.md
|
||||
// before the session, or of the stylesheets a DOM scan reads.
|
||||
assert_eq!(classify(&cache, "s", "a.css", false, vec![font]).unknown, 1);
|
||||
assert_eq!(classify(&cache, "s", "a.css", true, vec![css]).unknown, 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stop_baseline_old_engine_and_malformed_cache_stay_unknown() {
|
||||
let mut cache = Cache::new();
|
||||
let f = finding("side-tab", "a.css", "border-left: 4px solid red", 1.0);
|
||||
for record in [
|
||||
json!({"version": 1, "engine": "0.0.0", "counts": {}}),
|
||||
json!({"version": 1, "engine": env!("CARGO_PKG_VERSION"), "counts": {"bad": -1}}),
|
||||
] {
|
||||
ensure_file(&mut cache, "s", "a.css").insert(FIELD.into(), record);
|
||||
assert_eq!(
|
||||
classify(&cache, "s", "a.css", false, vec![f.clone()]).unknown,
|
||||
1
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user