Skip to content

Commit e26bc29

Browse files
Your Nameclaude
andcommitted
fix(core,server): security audit fixes + response-size hardening
Fixes 2 security bugs found via deep audit, closes 2 more targeted gaps from the same audit's follow-up, and ships the pagination default flip that audit had left pending sign-off: - fingerprint_edit_lines/edit_symbol/hunks: switch to length-prefixed (netstring-style) field encoding, closing a delimiter-injection preimage collision that could forge a fingerprint match and skip HIGH_RISK_REQUIRES_INDEPENDENT_REVIEW via a stale approved review. - run_migrations_from: read-decide-write is now atomic per step (fresh PRAGMA user_version inside BEGIN IMMEDIATE, never-regress guard on both the per-step and final stamp), closing a TOCTOU that could let a concurrent/earlier writer's version get stamped backwards below the real physical schema. - compute_touch_risk: multi-hunk signature-escalation gap -- a signature rewritten by several hunks that jointly cover it, with no single hunk covering it alone, previously escaped detection. New compute_touch_risk_with_reconstruction reuses the real apply_hunks splice edit_lines_impl_gated already computes (for validate_syntax_diff) instead of a new diff algorithm; every other caller is unaffected (they only ever construct one whole-range hunk, for which the original heuristic is already exact). - claim_approved_matching: pending_reviews approvals are now one-shot -- atomically consumes the matching row instead of a bare read, so one human approval authorizes exactly one write. - SourceParams/UnderstandParams.max_lines: default flips to 300 when the JSON key is omitted (explicit null still unlimited), based on live measurement (p99=260 production symbols, 14 outliers over 300). Also fixes understand()'s suggested_next, which never checked for truncation before pointing at edit_context. - SymbolsBatchParams.lean + schemars description overrides on 4 shared structs trim response/schema size. Full regression coverage for every fix; cargo test/clippy/fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent f1183ef commit e26bc29

53 files changed

Lines changed: 2537 additions & 369 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

crates/calm-core/src/analysis/coverage.rs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,31 @@ impl CoverageData {
2121
};
2222
(line_start..=line_end).any(|ln| file_cov.contains(&ln))
2323
}
24+
25+
/// P1 (audit follow-up, 2026-08-23): fraction of `[line_start,
26+
/// line_end]` (inclusive) that has SOME recorded execution, as
27+
/// `covered / total_lines_in_range`. `0.0` when the file has no
28+
/// coverage data at all or the range is empty/inverted. Distinct from
29+
/// `is_covered`'s any-line-at-all question -- that answers "did this
30+
/// range ever run", correct for `compute_dead_code_confidence`'s
31+
/// liveness check, but too lenient for a caller asking "is this edit
32+
/// adequately tested": a 50-line range with exactly 1 covered line
33+
/// reads as fully `is_covered`, hiding that the other 49 lines are
34+
/// genuinely untested. Callers deciding whether to escalate risk
35+
/// should compare this ratio against their own threshold instead.
36+
pub fn coverage_ratio(&self, abs_path: &str, line_start: i64, line_end: i64) -> f64 {
37+
if line_end < line_start {
38+
return 0.0;
39+
}
40+
let total = (line_end - line_start + 1) as f64;
41+
let Some(file_cov) = self.covered_lines.get(abs_path) else {
42+
return 0.0;
43+
};
44+
let covered = (line_start..=line_end)
45+
.filter(|ln| file_cov.contains(ln))
46+
.count() as f64;
47+
covered / total
48+
}
2449
}
2550

2651
// Defined in `indexer::coverage_paths`, not here: `indexer::refresh` also
@@ -332,6 +357,32 @@ mod tests {
332357
assert!(!cov.is_covered("/bar.py", 1, 100));
333358
}
334359

360+
/// P1 (audit follow-up, 2026-08-23): coverage_ratio must report the
361+
/// real fraction, not just any-line-at-all -- this is the fact
362+
/// touches_uncovered_code's threshold check now relies on to catch a
363+
/// mostly-untested hunk that is_covered alone would have missed (1
364+
/// covered line out of a 50-line range read as "fully covered").
365+
#[test]
366+
fn test_coverage_ratio() {
367+
let mut lines = HashSet::new();
368+
lines.insert(5);
369+
lines.insert(10);
370+
let cov = CoverageData {
371+
source: "lcov".to_string(),
372+
covered_lines: HashMap::from([("/foo.py".to_string(), lines)]),
373+
};
374+
// 2 covered lines (5, 10) out of a 10-line range [1, 10].
375+
assert_eq!(cov.coverage_ratio("/foo.py", 1, 10), 0.2);
376+
// Fully covered single-line range.
377+
assert_eq!(cov.coverage_ratio("/foo.py", 10, 10), 1.0);
378+
// No overlap at all.
379+
assert_eq!(cov.coverage_ratio("/foo.py", 20, 25), 0.0);
380+
// Unknown file -> 0.0, same convention as is_covered.
381+
assert_eq!(cov.coverage_ratio("/bar.py", 1, 100), 0.0);
382+
// Inverted range is defensively 0.0, not a panic.
383+
assert_eq!(cov.coverage_ratio("/foo.py", 10, 1), 0.0);
384+
}
385+
335386
#[test]
336387
fn test_normalize_path_strips_dot_segments_when_not_existing() {
337388
let dir = tempfile::tempdir().unwrap();

crates/calm-core/src/authority/mod.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@ pub mod snapshot;
1313

1414
pub use pending_review::{
1515
AgentRelayOutcome, NewPendingReview, PENDING_REVIEW_DEFAULT_TTL_SECS, PendingReview,
16-
approve_pending_review, decide_via_agent_relay, decline_pending_review, find_approved_matching,
17-
get_pending_review, insert_pending_review, list_pending_reviews,
16+
approve_pending_review, claim_approved_matching, decide_via_agent_relay,
17+
decline_pending_review, find_approved_matching, get_pending_review, insert_pending_review,
18+
list_pending_reviews,
1819
};
1920
pub use receipt::{ApprovalReceipt, insert_approval_receipt};
2021
pub use review::{

crates/calm-core/src/authority/pending_review.rs

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,53 @@ pub fn find_approved_matching(
289289
.optional()
290290
}
291291

292+
/// Bug (2026-08-23 audit, pending_review not one-shot): the write-gate
293+
/// claim `edit_lines_impl_gated` should use instead of a bare
294+
/// `find_approved_matching` -- atomically transitions the matching
295+
/// unexpired `status = 'approved'` row to `status = 'consumed'` and
296+
/// returns it, so ONE human approval authorizes exactly ONE write, not an
297+
/// unlimited number of them for up to `PENDING_REVIEW_DEFAULT_TTL_SECS`.
298+
/// Previously a plain `SELECT` with no consumption meant any later call
299+
/// that happened to reproduce the identical `(path, fingerprint)` --
300+
/// e.g. the file legitimately cycling back to a byte-identical state via
301+
/// an unrelated edit-then-revert within the TTL window -- would silently
302+
/// reuse a stale human decision instead of asking again.
303+
///
304+
/// The `UPDATE ... WHERE status = 'approved'` is the actual atomic claim:
305+
/// if two callers somehow race on the exact same row, SQLite serializes
306+
/// the two `UPDATE`s and only the first can actually flip the row, so at
307+
/// most one caller's `rows_affected` is 1 -- no separate TOCTOU window to
308+
/// close with an explicit transaction (unlike the migration-stamp race
309+
/// fixed the same day, which needed one because it was read-decide-write
310+
/// across multiple statements; here the claim IS the one statement). The
311+
/// preceding `find_approved_matching` call is just a read to pick a
312+
/// candidate to try -- if it was concurrently claimed or expired before
313+
/// the `UPDATE` runs, `rows_affected == 0` and this honestly returns
314+
/// `None`, exactly as if no match had ever existed, and the caller falls
315+
/// through to requiring a fresh review.
316+
pub fn claim_approved_matching(
317+
conn: &Connection,
318+
path: &str,
319+
fingerprint: &str,
320+
) -> rusqlite::Result<Option<PendingReview>> {
321+
let Some(candidate) = find_approved_matching(conn, path, fingerprint)? else {
322+
return Ok(None);
323+
};
324+
let now = now_epoch_secs();
325+
let updated = conn.execute(
326+
"UPDATE pending_reviews SET status = 'consumed' \
327+
WHERE review_id = ?1 AND status = 'approved' AND expires_at > ?2",
328+
params![candidate.review_id, now],
329+
)?;
330+
if updated == 0 {
331+
return Ok(None);
332+
}
333+
Ok(Some(PendingReview {
334+
status: "consumed".to_string(),
335+
..candidate
336+
}))
337+
}
338+
292339
#[cfg(test)]
293340
mod tests {
294341
use super::*;
@@ -355,6 +402,51 @@ mod tests {
355402
assert_eq!(found.review_id, id);
356403
}
357404

405+
#[test]
406+
fn claim_approved_matching_consumes_the_row_so_a_second_claim_no_longer_matches() {
407+
// Bug (2026-08-23 audit, pending_review not one-shot): a plain
408+
// find_approved_matching (a SELECT) let the SAME approved row
409+
// authorize an unlimited number of writes for up to
410+
// PENDING_REVIEW_DEFAULT_TTL_SECS -- claim_approved_matching must
411+
// atomically consume it instead, so a second attempt at the exact
412+
// same (path, fingerprint) no longer matches.
413+
let conn = state_conn();
414+
let id =
415+
insert_pending_review(&conn, &new_review("edit_lines", "a.py", "sha256:abc")).unwrap();
416+
assert!(approve_pending_review(&conn, &id, "cli_manual_review").unwrap());
417+
418+
let first = claim_approved_matching(&conn, "a.py", "sha256:abc")
419+
.unwrap()
420+
.expect("an approved, unexpired, matching row must be claimable once");
421+
assert_eq!(first.review_id, id);
422+
assert_eq!(first.status, "consumed");
423+
424+
let second = claim_approved_matching(&conn, "a.py", "sha256:abc").unwrap();
425+
assert_eq!(
426+
second, None,
427+
"the same approval must not authorize a second write"
428+
);
429+
430+
// The row itself is really gone from the approved pool, not just
431+
// invisible to this one function -- the plain read-only lookup
432+
// must agree.
433+
assert_eq!(
434+
find_approved_matching(&conn, "a.py", "sha256:abc").unwrap(),
435+
None
436+
);
437+
let got = get_pending_review(&conn, &id).unwrap().unwrap();
438+
assert_eq!(got.status, "consumed");
439+
}
440+
441+
#[test]
442+
fn claim_approved_matching_misses_cleanly_when_nothing_is_approved() {
443+
let conn = state_conn();
444+
assert_eq!(
445+
claim_approved_matching(&conn, "a.py", "sha256:abc").unwrap(),
446+
None
447+
);
448+
}
449+
358450
#[test]
359451
fn find_approved_matching_misses_on_a_different_fingerprint() {
360452
// Content-addressed: the proposal changed since review was opened.

crates/calm-core/src/db/state_migrations.rs

Lines changed: 76 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -942,42 +942,63 @@ pub(crate) fn run_migrations_from(
942942
migrations: &[StateMigration],
943943
target: i64,
944944
) -> Result<(), StateMigrationError> {
945-
let on_disk: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?;
946-
let mut current = if on_disk == 0 {
947-
BASELINE_VERSION
948-
} else {
949-
on_disk
950-
};
951-
952-
while current < target {
953-
let step = match migrations.iter().find(|m| m.from == current) {
954-
Some(s) => s,
955-
None => {
956-
return Err(StateMigrationError::MissingMigration {
945+
loop {
946+
conn.execute_batch("BEGIN IMMEDIATE")?;
947+
let step_result = (|| -> Result<bool, StateMigrationError> {
948+
// Fresh read INSIDE this transaction -- closes the TOCTOU a bare
949+
// read-before-the-loop + unconditional-stamp-after-the-loop had
950+
// (2026-08-23 audit, Bug B): a concurrent OTHER process (e.g. a
951+
// newer binary migrating this same file further) could commit
952+
// its own advance between an outer read taken once before a
953+
// loop and a later unconditional stamp, silently REGRESSING
954+
// user_version below what that process already established --
955+
// even though the newer schema's DDL is still physically
956+
// present, defeating `refuse_if_schema_newer`'s downgrade guard
957+
// for every process that opens the file afterward. Re-reading
958+
// here, inside our own BEGIN IMMEDIATE, sees the true latest
959+
// committed value (no other writer can commit while we hold
960+
// this lock), so `current` is never stale by the time we
961+
// decide what (if anything) to write.
962+
let on_disk: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?;
963+
let current = if on_disk == 0 {
964+
BASELINE_VERSION
965+
} else {
966+
on_disk
967+
};
968+
if current >= target {
969+
// Nothing left for THIS call to do -- possibly because a
970+
// concurrent process already finished it. Never regress a
971+
// version someone else already stamped higher; only bring
972+
// a genuinely unstamped/behind file up to `target`.
973+
if on_disk < target {
974+
conn.pragma_update(None, "user_version", target)?;
975+
}
976+
return Ok(true);
977+
}
978+
let step = migrations.iter().find(|m| m.from == current).ok_or(
979+
StateMigrationError::MissingMigration {
957980
from: current,
958981
target,
959-
});
960-
}
961-
};
962-
debug_assert_eq!(
963-
step.to,
964-
step.from + 1,
965-
"state.db migrations must be consecutive single-version steps (got {}: {} -> {})",
966-
step.name,
967-
step.from,
968-
step.to
969-
);
970-
971-
conn.execute_batch("BEGIN IMMEDIATE")?;
972-
let step_result = (|| -> Result<(), StateMigrationError> {
982+
},
983+
)?;
984+
debug_assert_eq!(
985+
step.to,
986+
step.from + 1,
987+
"state.db migrations must be consecutive single-version steps (got {}: {} -> {})",
988+
step.name,
989+
step.from,
990+
step.to
991+
);
973992
(step.apply)(conn)?;
974993
conn.pragma_update(None, "user_version", step.to)?;
975-
Ok(())
994+
Ok(false)
976995
})();
977996
match step_result {
978-
Ok(()) => {
997+
Ok(done) => {
979998
conn.execute_batch("COMMIT")?;
980-
current = step.to;
999+
if done {
1000+
return Ok(());
1001+
}
9811002
}
9821003
Err(e) => {
9831004
// Undoes both the DDL and (if it was reached) the
@@ -988,13 +1009,6 @@ pub(crate) fn run_migrations_from(
9881009
}
9891010
}
9901011
}
991-
992-
// Always stamp explicitly, even when zero migrations ran -- a fresh or
993-
// `user_version == 0` file must still come out of this function stamped
994-
// to `target`, not left at its raw on-disk value, or the downgrade
995-
// guard has nothing to check next time.
996-
conn.pragma_update(None, "user_version", target)?;
997-
Ok(())
9981012
}
9991013

10001014
#[cfg(test)]
@@ -1213,6 +1227,32 @@ mod tests {
12131227
));
12141228
}
12151229

1230+
#[test]
1231+
fn run_migrations_from_never_regresses_a_version_already_stamped_higher() {
1232+
// Bug B (2026-08-23 audit): simulates the concurrent-process race
1233+
// deterministically, without real threads -- a newer binary (or
1234+
// just an earlier, faster call) already advanced this exact file
1235+
// to version 3 (with real physical schema to match) before THIS
1236+
// call, whose own `target` is only 2, ever got to look. The OLD
1237+
// code read `user_version` once, decided `current(2) < target(2)`
1238+
// was false so no step ran, then unconditionally stamped `target`
1239+
// (2) anyway at its final line -- regressing the file from 3 back
1240+
// to 2 even though its real schema was already ahead. The fix must
1241+
// leave an already-higher on-disk version untouched.
1242+
let conn = fresh_conn();
1243+
conn.pragma_update(None, "user_version", 3).unwrap();
1244+
1245+
let migrations: [StateMigration; 0] = [];
1246+
run_migrations_from(&conn, &migrations, 2).unwrap();
1247+
1248+
assert_eq!(
1249+
user_version(&conn),
1250+
3,
1251+
"a version a concurrent/earlier writer already stamped higher must never be \
1252+
regressed down to this call's own (now-stale) target"
1253+
);
1254+
}
1255+
12161256
#[test]
12171257
fn registered_v1_to_v2_migration_creates_the_new_tables_and_stamps_version() {
12181258
let conn = fresh_conn();

0 commit comments

Comments
 (0)