codewhale-hq/Codewhale
1
0 Fork 0
mirror of https://github.com/codewhale-hq/Codewhale.git synced 2026-11-08 17:10:50 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix(core): preserve restore data and offload approval evidence

Browse Source 
Require a durable safety snapshot before restore, reject unsafe path type
transitions, and attempt rollback after a partial checkout failure. Treat
failed git status as unknown worktree state so it cannot authorize closure.

Move the existing auto-review evidence classifier to the blocking pool for
Engine, ACP, and child-gate consumers; a worker failure denies admission.
Run /restore through the existing command bridge and blocking pool, keeping
standalone synchronous hosts and existing restore errors compatible.

Keep the lexical blocking-call scanner unchanged. Record its three existing
auto-review canonicalize helper sites after auditing every production caller,
and the new snapshot preflight site (std_fs 17 -> 18) after protecting the
remaining /restore caller. Mark the integration-test read helper cfg(test).
Classify English Fleet docs as heavy CI input and repair the public rustdoc
link to the private wrapper-depth constant.

Validation:
- Initial affected-surface batch: 108 passed, 0 failed (43 auto-review,
  31 child gate, 17 restore, 7 ACP, 5 Engine, 4 GitHub closure, 1 config).
  Its ten source diffs remain identical; only the command bridge and budget
  entries were subsequently added.
- Final all-features library build: 5 passed, 0 failed (restore command
  roundtrip and four safety regressions). Each exact filter ran one test.
- Negative control build succeeded, then 0 passed, 4 failed at the intended
  backup, rollback, Git-status, and runtime-liveness assertions. All three
  mutated sources were restored byte-for-byte. The earlier control that
  failed compilation is not counted as regression proof.
- Feature registry: 6 passed, 0 failed.
- Execpolicy documentation with RUSTDOCFLAGS=-Dwarnings: passed.
- Blocking-call scanner: 753 sites across 210 files, within budget;
  touched Rust formatting and git diff --check passed.
- Reused matching-source root npm gate: 777 passed, 0 failed
  (75 wrapper + 19 SDK + 54 extension host + 629 web); check:web passed.
- Reused workflow checks: 138 consumed non-Rust paths classify heavy,
  11 event cases x 12 wrapper steps, 13 hermetic workflow checks, and
  nightly/release asset contracts passed.

Evidence: artifacts/takeover-20260929/core-followup-qualification, including
source hashes, exact commands, logs, control restoration, and caller audit.
Local focused evidence only; hosted cross-platform CI and native release
qualification remain separate gates.

Signed-off-by: Hunter B <[email protected]>
This commit is contained in:
Hunter B committed 2026-09-30 00:12:37 -07:00
1 parent 7eb71724a2
commit 2bbe598bad
12 files changed
+609 -180
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.github/workflows/ci.yml
+1 -1
View File
@@ -162,7 +162,7 @@ jobs:
scripts/release/npm-wrapper-smoke.js|scripts/mobile-smoke.sh|scripts/check-provider-registry.py|scripts/check-config-example.py)
heavy=true
;;
crates/*|docs/HOOKS.md|docs/KEYBINDINGS.md|docs/TELEMETRY.md|docs/FLEET_WORKFLOW_TUTORIAL.md|docs/zh_hans/FLEET.md|docs/2512.24601v2.pdf|docs/cloud-facts/*|docs/examples/*)
crates/*|docs/HOOKS.md|docs/KEYBINDINGS.md|docs/TELEMETRY.md|docs/FLEET.md|docs/FLEET_WORKFLOW_TUTORIAL.md|docs/zh_hans/FLEET.md|docs/2512.24601v2.pdf|docs/cloud-facts/*|docs/examples/*)
heavy=true
;;
docs/*|*.md|packaging/aur/*|.github/PULL_REQUEST_TEMPLATE.md|.github/ISSUE_TEMPLATE/*|.github/scripts/agent-task-metadata.test.sh|.github/workflows/agent-task-labels.yml|.github/workflows/auto-tag.yml|.github/workflows/stale.yml|.github/workflows/triage.yml|scripts/release/check-versions.sh|scripts/release/check-ohos-deps.sh|scripts/release/install-dogfood.sh|scripts/release/install-dogfood.test.sh|scripts/release/prepare-release.sh|scripts/release/prepare-release.test.sh|scripts/dev-cache.sh|scripts/dev-cache.test.sh|scripts/dev-cargo.sh|scripts/dev-test.sh)
crates/execpolicy/src/command_safety.rs
+1 -1
View File
@@ -2073,7 +2073,7 @@ pub fn is_literal_rm_invocation(command: &str) -> bool {
///
/// Deliberately over-inclusive (`echo rm -rf /etc` yields an `rm` argv too):
/// callers use it to *hold* catastrophic commands, never to allow anything.
/// `None` when words nest deeper than [`MAX_WRAPPER_DEPTH`]: fail closed.
/// `None` when words nest deeper than `MAX_WRAPPER_DEPTH`: fail closed.
pub fn command_invocations(command: &str) -> Option<Vec<Vec<String>>> {
fn collect(command: &str, depth: usize, out: &mut Vec<Vec<String>>) -> bool {
if depth > MAX_WRAPPER_DEPTH {
crates/tui/src/acp_server.rs
+50 -29
View File
@@ -672,7 +672,7 @@ struct PreparedAcpTool {
/// carve-out: a remembered exact allow rule may clear the ordinary tool hold,
/// but the built-in safety floor and repository law can always re-add a prompt
/// or hard block afterwards.
fn prepare_acp_tool_admission(
async fn prepare_acp_tool_admission(
config: &Config,
registry: &ToolRegistry,
call: &PendingToolCall,
@@ -745,14 +745,14 @@ fn prepare_acp_tool_admission(
} else {
crate::tui::auto_review::RunOrigin::Headless
};
let review_context = crate::tui::auto_review::AutoReviewContext::from_tool_call(
let review_context = crate::tui::auto_review::AutoReviewContext::from_tool_call_async(
&call.name,
&prepared.input,
run_origin,
approval_mode,
crate::config::is_workspace_trusted(workspace),
Some(workspace),
);
)
.await?;
let (auto_review, _audit) =
auto_review_plan_decision_for_context(&config.auto_review_policy(), &review_context);
match auto_review {
@@ -828,7 +828,8 @@ async fn prepare_acp_tool_with_hooks(
if let Some(updated_input) = hook_outcome.updated_input {
final_call.input = updated_input;
}
let (prepared, mut admission) = prepare_acp_tool_admission(config, registry, &final_call)?;
let (prepared, mut admission) =
prepare_acp_tool_admission(config, registry, &final_call).await?;
if hook_outcome.requires_approval && matches!(admission, AcpToolAdmission::Auto) {
admission = AcpToolAdmission::RequestPermission(
"A ToolCallBefore hook requires explicit approval for this call.".to_string(),
@@ -3218,8 +3219,8 @@ mod tests {
assert_eq!(acp_approval_mode(&Config::default()), ApprovalMode::Suggest);
}
#[test]
fn yolo_admission_auto_executes_write_without_permission_round_trip() {
#[tokio::test]
async fn yolo_admission_auto_executes_write_without_permission_round_trip() {
// #6337: an unattended `--yolo` session must execute tools instead of
// stalling on permission requests no headless client answers.
let (dir, registry) = workspace_registry();
@@ -3231,13 +3232,15 @@ mod tests {
"File",
json!({"action": "write", "path": "yolo.txt", "content": "yolo"}),
);
let (_, admission) = prepare_acp_tool_admission(&config, &registry, &call).unwrap();
let (_, admission) = prepare_acp_tool_admission(&config, &registry, &call)
.await
.unwrap();
assert_eq!(admission, AcpToolAdmission::Auto);
assert_eq!(registry.context().workspace, dir.path());
}
#[test]
fn default_admission_still_requests_permission_for_write() {
#[tokio::test]
async fn default_admission_still_requests_permission_for_write() {
// Pins the Ask default the yolo test above contrasts with: without
// `--yolo`, a write surfaces a permission request to the client.
let (_dir, registry) = workspace_registry();
@@ -3245,8 +3248,9 @@ mod tests {
"File",
json!({"action": "write", "path": "ask.txt", "content": "ask"}),
);
let (_, admission) =
prepare_acp_tool_admission(&Config::default(), &registry, &call).unwrap();
let (_, admission) = prepare_acp_tool_admission(&Config::default(), &registry, &call)
.await
.unwrap();
assert!(matches!(admission, AcpToolAdmission::RequestPermission(_)));
}
@@ -4429,7 +4433,9 @@ mod tests {
);
let registry = build_acp_tool_registry(&config, dir.path(), false);
let raw = pending_call("File", json!({"action": "read", "path": "safe.txt"}));
let (_, raw_admission) = prepare_acp_tool_admission(&config, &registry, &raw).unwrap();
let (_, raw_admission) = prepare_acp_tool_admission(&config, &registry, &raw)
.await
.unwrap();
assert_eq!(raw_admission, AcpToolAdmission::Auto);
let prepared = prepare_acp_tool_with_hooks(&config, "test-model", &registry, &raw)
@@ -4443,8 +4449,8 @@ mod tests {
assert!(!dir.path().join("rewritten.txt").exists());
}
#[test]
fn acp_admission_is_input_specific_and_has_no_workspace_write_carve_out() {
#[tokio::test]
async fn acp_admission_is_input_specific_and_has_no_workspace_write_carve_out() {
let (dir, registry) = workspace_registry();
let config = Config::default();
let read = pending_call("File", json!({"action": "read", "path": "src/lib.rs"}));
@@ -4453,8 +4459,12 @@ mod tests {
json!({"action": "write", "path": "src/lib.rs", "content": "new"}),
);
let (_, read_admission) = prepare_acp_tool_admission(&config, &registry, &read).unwrap();
let (_, write_admission) = prepare_acp_tool_admission(&config, &registry, &write).unwrap();
let (_, read_admission) = prepare_acp_tool_admission(&config, &registry, &read)
.await
.unwrap();
let (_, write_admission) = prepare_acp_tool_admission(&config, &registry, &write)
.await
.unwrap();
assert_eq!(read_admission, AcpToolAdmission::Auto);
assert!(matches!(
@@ -4464,8 +4474,8 @@ mod tests {
assert_eq!(registry.context().workspace, dir.path());
}
#[test]
fn acp_admission_folds_typed_rules_then_headless_safety_floor() {
#[tokio::test]
async fn acp_admission_folds_typed_rules_then_headless_safety_floor() {
let (dir, registry) = workspace_registry();
let workspace = dir.path().to_string_lossy().into_owned();
let input = json!({"action": "write", "path": "allowed.txt", "content": "new"});
@@ -4474,12 +4484,16 @@ mod tests {
let allow = codewhale_execpolicy::ToolAskRule::file_path("write_file", "allowed.txt")
.into_exact_workspace_allow(workspace.clone());
let (_, admission) =
prepare_acp_tool_admission(&config_with_policy_rule(allow), &registry, &call).unwrap();
prepare_acp_tool_admission(&config_with_policy_rule(allow), &registry, &call)
.await
.unwrap();
assert_eq!(admission, AcpToolAdmission::Auto);
let ask = codewhale_execpolicy::ToolAskRule::file_path("write_file", "allowed.txt");
let (_, admission) =
prepare_acp_tool_admission(&config_with_policy_rule(ask), &registry, &call).unwrap();
prepare_acp_tool_admission(&config_with_policy_rule(ask), &registry, &call)
.await
.unwrap();
assert!(matches!(
admission,
AcpToolAdmission::RequestPermission(reason) if reason.contains("requires approval")
@@ -4490,7 +4504,9 @@ mod tests {
..codewhale_execpolicy::ToolAskRule::file_path("write_file", "allowed.txt")
};
let (_, admission) =
prepare_acp_tool_admission(&config_with_policy_rule(deny), &registry, &call).unwrap();
prepare_acp_tool_admission(&config_with_policy_rule(deny), &registry, &call)
.await
.unwrap();
assert!(matches!(admission, AcpToolAdmission::Block(_)));
let command = "rm -rf ~/";
@@ -4502,6 +4518,7 @@ mod tests {
&registry,
&shell_call,
)
.await
.unwrap();
assert!(matches!(
admission,
@@ -4510,8 +4527,8 @@ mod tests {
));
}
#[test]
fn acp_admission_blocks_detached_and_stateful_bash_inputs() {
#[tokio::test]
async fn acp_admission_blocks_detached_and_stateful_bash_inputs() {
let (_dir, registry) = workspace_registry();
for input in [
json!({"command": "sleep 30", "background": true}),
@@ -4524,8 +4541,9 @@ mod tests {
json!({"action": "cancel", "task_id": "shell-1"}),
] {
let call = pending_call("Bash", input);
let (_, admission) =
prepare_acp_tool_admission(&Config::default(), &registry, &call).unwrap();
let (_, admission) = prepare_acp_tool_admission(&Config::default(), &registry, &call)
.await
.unwrap();
assert!(matches!(
admission,
AcpToolAdmission::Block(reason)
@@ -4535,8 +4553,8 @@ mod tests {
assert!(!acp_shell_command_requests_detach("echo '&' && echo done"));
}
#[test]
fn acp_admission_auto_review_and_repo_law_override_typed_allow() {
#[tokio::test]
async fn acp_admission_auto_review_and_repo_law_override_typed_allow() {
let (dir, registry) = workspace_registry();
let workspace = dir.path().to_string_lossy().into_owned();
let command = "cargo test";
@@ -4557,6 +4575,7 @@ mod tests {
&registry,
&pending_call("Bash", json!({"command": command})),
)
.await
.unwrap();
assert!(matches!(
admission,
@@ -4584,7 +4603,9 @@ mod tests {
"File",
json!({"action": "write", "path": path, "content": "new"}),
);
let (_, admission) = prepare_acp_tool_admission(&config, &registry, &call).unwrap();
let (_, admission) = prepare_acp_tool_admission(&config, &registry, &call)
.await
.unwrap();
if expected_block {
assert!(matches!(
admission,
crates/tui/src/commands/contract.rs
+67 -33
View File
@@ -3099,19 +3099,33 @@ impl CommandSkillGroupContext for SkillGroupAdapter<'_> {
fn restore_snapshot(&mut self, id: &str) -> Result<(), String> {
let workspace = self.host.app.borrow().workspace.clone();
let repo = match crate::snapshot::SnapshotRepo::open_or_init(&workspace) {
Ok(repo) => repo,
Err(err) => {
return Err(format!(
"Snapshot repo unavailable for {}: {err}",
workspace.display(),
));
}
let id = id.to_owned();
let restore = move || {
let repo = match crate::snapshot::SnapshotRepo::open_or_init(&workspace) {
Ok(repo) => repo,
Err(err) => {
return Err(format!(
"Snapshot repo unavailable for {}: {err}",
workspace.display(),
));
}
};
let id = crate::snapshot::SnapshotId::parse(&id)
.map_err(|err| format!("Restore failed: {err}"))?;
repo.restore(&id)
.map_err(|err| format!("Restore failed: {err}"))
};
let id = crate::snapshot::SnapshotId::parse(id)
.map_err(|err| format!("Restore failed: {err}"))?;
repo.restore(&id)
.map_err(|err| format!("Restore failed: {err}"))
// Standalone synchronous hosts have no runtime worker to protect.
// Live TUI dispatch uses the existing bridge and blocking pool for
// the entire restore, including its mandatory safety snapshot.
if tokio::runtime::Handle::try_current().is_err() {
return restore();
}
run_async(async move {
tokio::task::spawn_blocking(restore)
.await
.map_err(|error| format!("Restore task failed: {error}"))?
})
}
fn approval_state(&self) -> CommandApprovalState {
@@ -6037,28 +6051,48 @@ mod tests {
#[test]
fn skill_group_snapshot_list_and_restore_roundtrip() {
let tmp = TempDir::new().unwrap();
let _home = scoped_home(&tmp);
let skills_dir = tmp.path().join("skills");
let file = tmp.path().join("a.txt");
let repo = crate::snapshot::SnapshotRepo::open_or_init(tmp.path()).unwrap();
std::fs::write(&file, b"v1").unwrap();
repo.snapshot("pre-turn:1").unwrap();
std::fs::write(&file, b"v2").unwrap();
let mut app = skill_test_app(&tmp, &skills_dir);
{
let mut bundle = app.command_contexts();
let group = bundle
.parts()
.skill_group
.expect("skill_group facet must be present");
let entries = group.snapshot_list(20).unwrap();
assert_eq!(entries.len(), 1);
assert_eq!(entries[0].label, "pre-turn:1");
assert!(!entries[0].id.is_empty());
group.restore_snapshot(&entries[0].id).unwrap();
for in_runtime in [false, true] {
let tmp = TempDir::new().unwrap();
let _home = scoped_home(&tmp);
let skills_dir = tmp.path().join("skills");
let file = tmp.path().join("a.txt");
let repo = crate::snapshot::SnapshotRepo::open_or_init(tmp.path()).unwrap();
std::fs::write(&file, b"v1").unwrap();
repo.snapshot("pre-turn:1").unwrap();
std::fs::write(&file, b"v2").unwrap();
let mut app = skill_test_app(&tmp, &skills_dir);
{
let mut bundle = app.command_contexts();
let group = bundle
.parts()
.skill_group
.expect("skill_group facet must be present");
let entries = group.snapshot_list(20).unwrap();
assert_eq!(entries.len(), 1);
assert_eq!(entries[0].label, "pre-turn:1");
assert!(!entries[0].id.is_empty());
let mut restore = || {
group.restore_snapshot(&entries[0].id).unwrap();
assert!(
group
.restore_snapshot("not-a-snapshot")
.unwrap_err()
.starts_with("Restore failed:")
);
};
if in_runtime {
tokio::runtime::Builder::new_multi_thread()
.worker_threads(1)
.enable_all()
.build()
.unwrap()
.block_on(async { restore() });
} else {
restore();
}
}
assert_eq!(std::fs::read_to_string(&file).unwrap(), "v1");
}
assert_eq!(std::fs::read_to_string(&file).unwrap(), "v1");
}
#[test]
crates/tui/src/core/engine/turn_loop.rs
+74 -67
View File
@@ -866,7 +866,7 @@ impl Engine {
)),
super::reviewer::ReviewerOutcome::Cancelled => None,
};
let result = review.outcome.into_tool_result(context.tool_name);
let result = review.outcome.into_tool_result(context.tool_name.as_ref());
emit_tool_audit(json!({
"event": "tool.auto_review",
"gate": "guardian",
@@ -3818,72 +3818,79 @@ impl Engine {
}
if blocked_error.is_none() {
let review_context = crate::tui::auto_review::AutoReviewContext::from_tool_call(
&tool_name,
&tool_input,
auto_review_run_origin_for_plan(detached_start),
self.session.approval_mode,
crate::config::is_workspace_trusted(&self.session.workspace),
Some(&self.session.workspace),
);
let (decision, audit_event) = auto_review_plan_decision_for_context(
&self.config.auto_review_policy,
&review_context,
);
emit_tool_audit(json!({
"event": "tool.auto_review",
"gate": "deterministic",
"tool_id": tool_id.clone(),
"auto_review": audit_event,
}));
match decision {
AutoReviewPlanDecision::NoChange => {}
AutoReviewPlanDecision::Allow => {
if !hook_requires_approval && !approval_force_prompt {
approval_required = false;
}
}
AutoReviewPlanDecision::ForcePrompt(reason) => {
// The built-in safety floor is deliberately
// non-bypassable. Ask/Auto-Review surface the hold;
// Full Access turns this disposition into a hard
// block below, without opening a modal.
approval_required = true;
approval_description = reason;
approval_force_prompt = true;
}
AutoReviewPlanDecision::Block(reason) => {
approval_required = false;
approval_force_prompt = false;
let _ = self
.tx_event
.send(Event::ToolGateDecision {
agent_id: None,
tool_id: tool_id.clone(),
tool_name: tool_name.clone(),
gate: crate::core::events::ToolGate::AutoReviewDeterministic,
decision: crate::core::events::ToolGateVerdict::Denied,
risk: None,
reason: crate::core::events::bounded_gate_reason(&reason),
})
.await;
blocked_error = Some(auto_review_block_tool_error(&reason));
}
AutoReviewPlanDecision::ConsultReviewer(held_reason) => {
if let Err(error) = self
.consult_auto_review_guardian(
client,
&review_context,
&tool_input,
&held_reason,
&tool_id,
turn,
)
.await
{
blocked_error = Some(error);
} else if !hook_requires_approval && !approval_force_prompt {
approval_required = false;
let review_context =
crate::tui::auto_review::AutoReviewContext::from_tool_call_async(
&tool_name,
&tool_input,
auto_review_run_origin_for_plan(detached_start),
self.session.approval_mode,
Some(&self.session.workspace),
)
.await;
match review_context {
Err(error) => blocked_error = Some(error),
Ok(review_context) => {
let (decision, audit_event) = auto_review_plan_decision_for_context(
&self.config.auto_review_policy,
&review_context,
);
emit_tool_audit(json!({
"event": "tool.auto_review",
"gate": "deterministic",
"tool_id": tool_id.clone(),
"auto_review": audit_event,
}));
match decision {
AutoReviewPlanDecision::NoChange => {}
AutoReviewPlanDecision::Allow => {
if !hook_requires_approval && !approval_force_prompt {
approval_required = false;
}
}
AutoReviewPlanDecision::ForcePrompt(reason) => {
// The built-in safety floor is deliberately
// non-bypassable. Ask/Auto-Review surface the hold;
// Full Access turns this disposition into a hard
// block below, without opening a modal.
approval_required = true;
approval_description = reason;
approval_force_prompt = true;
}
AutoReviewPlanDecision::Block(reason) => {
approval_required = false;
approval_force_prompt = false;
let _ = self
.tx_event
.send(Event::ToolGateDecision {
agent_id: None,
tool_id: tool_id.clone(),
tool_name: tool_name.clone(),
gate:
crate::core::events::ToolGate::AutoReviewDeterministic,
decision: crate::core::events::ToolGateVerdict::Denied,
risk: None,
reason: crate::core::events::bounded_gate_reason(&reason),
})
.await;
blocked_error = Some(auto_review_block_tool_error(&reason));
}
AutoReviewPlanDecision::ConsultReviewer(held_reason) => {
if let Err(error) = self
.consult_auto_review_guardian(
client,
&review_context,
&tool_input,
&held_reason,
&tool_id,
turn,
)
.await
{
blocked_error = Some(error);
} else if !hook_requires_approval && !approval_force_prompt {
approval_required = false;
}
}
}
}
}
crates/tui/src/snapshot/repo.rs
+197 -32
View File
@@ -865,30 +865,72 @@ impl SnapshotRepo {
/// Restore the workspace to the state at `id`.
///
/// Uses `git checkout <sha> -- :/` which checks out every path in the
/// snapshot tree relative to the workspace root. We do NOT touch the
/// user's own `.git` — snapshots only contain working-tree files.
/// Requires a durable safety snapshot before changing files. A failed
/// restore attempts to put those files back; if that also fails, the
/// error identifies the retained safety snapshot for recovery. This is
/// not atomic against an external editor changing the live workspace.
/// File/directory transitions are refused before checkout because the
/// replaced directory may contain files excluded from the backup.
/// We never touch the user's own `.git`.
pub fn restore(&self, id: &SnapshotId) -> io::Result<()> {
self.with_write_lock(|| self.restore_locked(id))
}
fn restore_locked(&self, id: &SnapshotId) -> io::Result<()> {
// Restore is the one destructive operation with no undo of its own.
// Capture the pre-restore state first so the restore itself can be
// reversed (2026-08-04 snapshot hunt: makes several other findings
// recoverable instead of final). The `pre-restore:` prefix is
// deliberately not a `/undo` or `revert_turn` candidate label, so the
// safety net never changes snapshot selection. Best-effort: a failed
// safety snapshot must never block the restore the user asked for.
// The backup label is deliberately not an undo/revert-turn candidate.
let target_short = &id.as_str()[..id.as_str().len().min(12)];
if let Err(e) = self.snapshot_with_session(&format!("pre-restore:{target_short}"), None) {
tracing::warn!(
target: "snapshot",
"pre-restore safety snapshot failed (restore will proceed): {e}"
);
}
let current_paths = self.tree_paths("HEAD")?;
let backup = self
.snapshot_with_session(&format!("pre-restore:{target_short}"), None)
.map_err(|error| {
io_other(format!(
"pre-restore safety snapshot failed; no workspace files were changed: {error}"
))
})?;
let current_paths = self.tree_paths(backup.as_str())?;
let target_paths = self.tree_paths(id.as_str())?;
for rel in &target_paths {
match std::fs::symlink_metadata(self.work_tree.join(rel)) {
Ok(metadata) if metadata.is_dir() => {
return Err(io_other(format!(
"'{}' requires a file/directory transition; nothing was restored",
rel.display()
)));
}
Ok(_) if !current_paths.contains(rel) => {
return Err(io_other(format!(
"'{}' was excluded from the safety snapshot; nothing was restored",
rel.display()
)));
}
Err(error) if error.kind() == io::ErrorKind::NotADirectory => {
return Err(io_other(format!(
"'{}' requires a file/directory transition; nothing was restored",
rel.display()
)));
}
Err(error) if error.kind() != io::ErrorKind::NotFound => return Err(error),
_ => {}
}
}
if let Err(error) = self.restore_tree(id, &current_paths, &target_paths) {
let recovery = match self.restore_tree(&backup, &target_paths, &current_paths) {
Ok(()) => "previous snapshot files were restored".to_string(),
Err(rollback) => format!("rollback also failed: {rollback}"),
};
return Err(io_other(format!(
"restore failed: {error}; {recovery}; safety snapshot {} retains the previous files",
backup.as_str()
)));
}
Ok(())
}
fn restore_tree(
&self,
id: &SnapshotId,
current_paths: &HashSet<PathBuf>,
target_paths: &HashSet<PathBuf>,
) -> io::Result<()> {
// An empty target (the first snapshot of an empty directory) has no
// path for `:/` to match, and git refuses the checkout outright; there
// is nothing to write back, only the later files to remove.
@@ -905,8 +947,7 @@ impl SnapshotRepo {
)));
}
}
self.remove_paths_missing_from_target(&current_paths, &target_paths)?;
Ok(())
self.remove_paths_missing_from_target(current_paths, target_paths)
}
/// File restore never traverses symlinks, directories, or Git metadata.
@@ -1474,9 +1515,8 @@ impl SnapshotRepo {
.difference(target_paths)
.filter(|rel| is_safe_relative_path(rel))
.collect();
// The removal list comes from the side repo, not the live tree. When
// the pre-restore safety snapshot failed it is the previous HEAD's, so
// a directory it names can have been replaced by a symlink since, and
// The removal list comes from the side repo, not the live tree. A
// directory may have been replaced by a symlink since the backup, and
// `remove_file` would follow it and delete outside the workspace.
// Refuse the whole removal before deleting anything.
for rel in &removals {
@@ -1488,11 +1528,18 @@ impl SnapshotRepo {
continue;
}
let path = self.work_tree.join(rel);
let Ok(metadata) = std::fs::symlink_metadata(&path) else {
continue;
let metadata = match std::fs::symlink_metadata(&path) {
Ok(metadata) => metadata,
Err(error) if error.kind() == io::ErrorKind::NotFound => continue,
Err(error) => return Err(error),
};
if metadata.file_type().is_dir() {
let _ = std::fs::remove_dir(&path);
// A file-to-directory transition can make this path a
// required parent of files just restored from the target.
if target_paths.iter().any(|target| target.starts_with(rel)) {
continue;
}
std::fs::remove_dir(&path)?;
} else {
std::fs::remove_file(&path)?;
}
@@ -2572,10 +2619,129 @@ mod tests {
);
}
/// When the pre-restore safety snapshot fails, the removal list is the
/// previous HEAD's. A directory it names that was replaced by a symlink
/// since used to be followed, deleting the same-named file outside the
/// workspace (reachable once restore to an empty tree skipped checkout).
#[test]
fn restore_keeps_current_files_when_safety_snapshot_fails() {
let tmp = tempdir().unwrap();
let (repo, _home) = make_repo(tmp.path());
let empty = repo.snapshot("pre-turn:1").unwrap();
let file = repo.work_tree().join("new-work.txt");
std::fs::write(&file, b"only copy of new work").unwrap();
repo.snapshot("post-turn:1").unwrap();
std::fs::write(repo.git_dir().join("index.lock"), b"").unwrap();
let error = repo.restore(&empty).expect_err("backup is mandatory");
assert!(
error.to_string().contains("safety snapshot failed"),
"{error}"
);
assert_eq!(std::fs::read(&file).unwrap(), b"only copy of new work");
}
#[test]
fn restore_rolls_back_files_after_a_partial_checkout_error() {
let tmp = tempdir().unwrap();
let (repo, _home) = make_repo(tmp.path());
let first = repo.work_tree().join("a.txt");
let last_dir = repo.work_tree().join("z");
let last = last_dir.join("last.txt");
std::fs::create_dir(&last_dir).unwrap();
std::fs::write(&first, b"old first").unwrap();
std::fs::write(&last, b"old last").unwrap();
let target = repo.snapshot("pre-turn:1").unwrap();
let object = run_git(
repo.git_dir(),
repo.work_tree(),
&["rev-parse", &format!("{}:z/last.txt", target.as_str())],
)
.unwrap();
assert!(object.status.success());
let object = String::from_utf8(object.stdout).unwrap();
let object = object.trim();
std::fs::write(&first, b"new first").unwrap();
std::fs::remove_file(&last).unwrap();
let before = repo.snapshot("before-restore-proof").unwrap();
// A missing target-only blob makes Git fail after it writes a.txt.
// The backup needs neither that blob nor z/last.txt for recovery.
std::fs::remove_file(
repo.git_dir()
.join("objects")
.join(&object[..2])
.join(&object[2..]),
)
.unwrap();
// Prove the failure fixture really permits a partial overwrite.
let partial = run_git(
repo.git_dir(),
repo.work_tree(),
&["checkout", target.as_str(), "--", ":/"],
)
.unwrap();
assert!(!partial.status.success());
assert_eq!(std::fs::read(&first).unwrap(), b"old first");
let reset = run_git(
repo.git_dir(),
repo.work_tree(),
&["checkout", before.as_str(), "--", ":/"],
)
.unwrap();
assert!(reset.status.success());
assert_eq!(std::fs::read(&first).unwrap(), b"new first");
let error = repo.restore(&target).expect_err("target blob is missing");
assert!(error.to_string().contains("unable to read"), "{error}");
assert!(
error
.to_string()
.contains("previous snapshot files were restored"),
"{error}"
);
assert!(error.to_string().contains("safety snapshot"), "{error}");
assert_eq!(std::fs::read(&first).unwrap(), b"new first");
assert!(!last.exists());
}
#[test]
fn restore_refuses_file_directory_transitions_without_changing_files() {
for target_is_dir in [false, true] {
let tmp = tempdir().unwrap();
let (repo, _home) = make_repo(tmp.path());
let path = repo.work_tree().join("a");
let child = path.join("child");
if target_is_dir {
std::fs::create_dir(&path).unwrap();
std::fs::write(&child, b"old child").unwrap();
} else {
std::fs::write(&path, b"old file").unwrap();
}
let target = repo.snapshot("pre-turn:1").unwrap();
if target_is_dir {
std::fs::remove_file(&child).unwrap();
std::fs::remove_dir(&path).unwrap();
std::fs::write(&path, b"new file").unwrap();
} else {
std::fs::remove_file(&path).unwrap();
std::fs::create_dir(&path).unwrap();
std::fs::write(&child, b"new child").unwrap();
}
let error = repo
.restore(&target)
.expect_err("type transition is refused");
assert!(
error.to_string().contains("file/directory transition"),
"{error}"
);
if target_is_dir {
assert_eq!(std::fs::read(&path).unwrap(), b"new file");
} else {
assert_eq!(std::fs::read(&child).unwrap(), b"new child");
}
}
}
/// A failed safety snapshot refuses before a symlinked parent could be
/// followed, even when restoring to an empty target skips checkout.
#[cfg(unix)]
#[test]
fn restore_refuses_to_remove_through_a_symlinked_parent() {
@@ -2593,14 +2759,13 @@ mod tests {
std::fs::write(&sentinel, b"outside").unwrap();
std::fs::remove_dir_all(&src).unwrap();
std::os::unix::fs::symlink(outside.path(), &src).unwrap();
// A stale index lock makes the pre-restore safety snapshot fail, so
// restore works from the previous HEAD's path list.
// A stale index lock makes the mandatory safety snapshot fail.
std::fs::write(repo.git_dir().join("index.lock"), b"").unwrap();
let err = repo
.restore(&empty)
.expect_err("a removal through a symlinked parent is refused");
assert_eq!(err.kind(), io::ErrorKind::InvalidInput, "{err}");
assert!(err.to_string().contains("safety snapshot failed"), "{err}");
assert_eq!(
std::fs::read(&sentinel).unwrap(),
b"outside",
crates/tui/src/tools/github/cli.rs
+6
View File
@@ -76,5 +76,11 @@ pub(super) fn ensure_github_repo(context: &ToolContext) -> Result<(), ToolError>
pub(super) fn git_status_porcelain(context: &ToolContext) -> Result<String, ToolError> {
let out = crate::dependencies::Git::output(&["status", "--porcelain"], &context.workspace)
.map_err(|e| ToolError::execution_failed(format!("failed to run git status: {e}")))?;
if !out.status.success() {
return Err(ToolError::execution_failed(format!(
"git status failed ({}); cannot verify that the worktree is clean",
out.status
)));
}
Ok(String::from_utf8_lossy(&out.stdout).to_string())
}
crates/tui/src/tools/github/mod.rs
+42
View File
@@ -248,6 +248,48 @@ mod tests {
use super::*;
use crate::tools::spec::ToolSpec;
#[test]
fn close_refuses_when_git_cannot_determine_worktree_status() {
use crate::dependencies::{ExternalTool, Git};
let tmp = tempfile::tempdir().expect("tempdir");
assert!(
Git::output(&["init", "-q"], tmp.path())
.unwrap()
.status
.success()
);
let context = ToolContext::new(tmp.path());
assert!(
super::cli::git_status_porcelain(&context)
.unwrap()
.is_empty()
);
std::fs::write(tmp.path().join("work.txt"), "uncommitted work").unwrap();
assert!(
!super::cli::git_status_porcelain(&context)
.unwrap()
.is_empty()
);
// A real Git failure can have empty stdout; that is not a clean tree.
std::fs::write(tmp.path().join(".git/index"), "invalid index").unwrap();
let input = json!({
"number": 424_242,
"dry_run": true,
"acceptance_criteria": ["done"],
"evidence": {
"files_changed": ["work.txt"],
"tests_run": ["local fixture"],
"final_status": "green"
}
});
let error = close_github_thread(input, &context, GithubCloseTarget::Issue)
.expect_err("unknown worktree state must not authorize closure")
.to_string();
assert!(error.contains("git status failed"), "{error}");
assert!(error.contains("cannot verify"), "{error}");
}
#[test]
fn close_schema_requires_structured_evidence() {
let schema = GithubTool::alias("github_close_issue", "close_issue").input_schema();
crates/tui/src/tools/subagent/mod.rs
+20 -4
View File
@@ -18956,7 +18956,6 @@ impl SubAgentToolRegistry {
{
return ChildGateVerdict::Deny(reason);
}
let workspace_trusted = crate::config::is_workspace_trusted(&workspace);
// Destructive work nobody is watching holds in every posture: a call
// that starts detached, or any call of an agent that runs detached
// (no foreground turn owns it). A foreground agent's call is judged
@@ -18966,14 +18965,31 @@ impl SubAgentToolRegistry {
.registry
.get(name)
.is_some_and(|spec| spec.starts_detached_for(input));
let review_context = AutoReviewContext::from_tool_call(
let review_context = match AutoReviewContext::from_tool_call_async(
name,
input,
auto_review_run_origin_for_plan(detached),
approval_mode,
workspace_trusted,
Some(&workspace),
);
)
.await
{
Ok(context) => context,
Err(error) => {
let reason = error.to_string();
self.emit_child_gate_receipt(
agent_id,
tool_id,
name,
ToolGate::AutoReviewDeterministic,
ToolGateVerdict::Denied,
None,
&reason,
)
.await;
return ChildGateVerdict::Deny(reason);
}
};
let (decision, _audit) = auto_review_plan_decision_for_context(
&self.gate_runtime.auto_review_policy,
&review_context,
crates/tui/src/tui/auto_review.rs
+146 -12
View File
@@ -9,6 +9,7 @@
use crate::tui::approval::{RiskLevel, ToolCategory, classify_risk, get_tool_category_for_call};
use codewhale_execpolicy::ApprovalMode;
use serde_json::{Value, json};
use std::borrow::Cow;
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum AutoReviewAction {
@@ -92,6 +93,9 @@ impl ToolActionKind {
/// `workspace`, when known, lets a forced delete of a path inside it stay
/// ordinary work instead of a catastrophic system-path delete.
/// A workspace enables filesystem evidence, so runtime callers must use
/// the blocking-pool context constructor. UI-only classification passes
/// `None` and does no filesystem I/O.
#[must_use]
pub fn from_tool_call(
tool_name: &str,
@@ -398,7 +402,7 @@ impl RunOrigin {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct AutoReviewContext<'a> {
pub tool_name: &'a str,
pub tool_name: Cow<'a, str>,
pub category: ToolCategory,
pub risk: RiskLevel,
pub action_kind: ToolActionKind,
@@ -415,6 +419,42 @@ pub struct AutoReviewContext<'a> {
}
impl<'a> AutoReviewContext<'a> {
/// Resolve filesystem and Git evidence on the blocking pool. Worker
/// failure is an admission error, never evidence that a call is safe.
pub async fn from_tool_call_async(
tool_name: &str,
params: &Value,
run_origin: RunOrigin,
approval_mode: ApprovalMode,
workspace: Option<&std::path::Path>,
) -> Result<Self, crate::tools::spec::ToolError> {
let tool_name = tool_name.to_owned();
let params = params.clone();
let workspace = workspace.map(std::path::Path::to_path_buf);
tokio::task::spawn_blocking(move || {
let trusted = workspace
.as_deref()
.is_some_and(crate::config::is_workspace_trusted);
AutoReviewContext::<'static>::from_tool_call_inner(
Cow::Owned(tool_name),
&params,
run_origin,
approval_mode,
trusted,
workspace.as_deref(),
)
})
.await
.map_err(|error| {
crate::tools::spec::ToolError::execution_failed(format!(
"Auto-review evidence could not be prepared: {error}"
))
})
}
/// Synchronous construction is reserved for focused policy tests. Runtime
/// callers must use the blocking-pool constructor above.
#[cfg(test)]
#[must_use]
pub fn from_tool_call(
tool_name: &'a str,
@@ -424,19 +464,37 @@ impl<'a> AutoReviewContext<'a> {
workspace_trusted: bool,
workspace: Option<&std::path::Path>,
) -> Self {
let category = get_tool_category_for_call(tool_name, params);
Self::from_tool_call_inner(
Cow::Borrowed(tool_name),
params,
run_origin,
approval_mode,
workspace_trusted,
workspace,
)
}
fn from_tool_call_inner(
tool_name: Cow<'a, str>,
params: &Value,
run_origin: RunOrigin,
approval_mode: ApprovalMode,
workspace_trusted: bool,
workspace: Option<&std::path::Path>,
) -> Self {
let name = tool_name.as_ref();
let category = get_tool_category_for_call(name, params);
// A read-category name that also says it mutates is not benign, or
// the read-only allow would wave it through before any review (V3).
let risk = if matches!(category, ToolCategory::Safe | ToolCategory::McpRead)
&& read_prefixed_name_mutates(&action_qualified_tool_name(tool_name, params))
&& read_prefixed_name_mutates(&action_qualified_tool_name(name, params))
{
RiskLevel::Destructive
} else {
classify_risk(tool_name, category, params)
classify_risk(name, category, params)
};
let action_kind = ToolActionKind::from_tool_call(tool_name, params, category, workspace);
let action_kind = ToolActionKind::from_tool_call(name, params, category, workspace);
Self {
tool_name,
category,
risk,
action_kind,
@@ -446,18 +504,18 @@ impl<'a> AutoReviewContext<'a> {
approval_mode,
workspace_trusted,
outbound_web_request: matches!(
crate::tools::canonical_action::canonical_action_alias(tool_name, params),
crate::tools::canonical_action::canonical_action_alias(name, params),
"web_search" | "fetch_url" | "web_run" | "web.run"
),
write_targets_bounded: workspace
.zip(file_write_target_paths(tool_name, params))
.zip(file_write_target_paths(name, params))
.is_some_and(|(workspace, paths)| {
crate::core::authority::paths_within_workspace_write_carve_out(
workspace, &paths,
)
}),
unrecoverable_deletes: {
let deletes = file_write_delete_paths(tool_name, params, workspace);
let deletes = file_write_delete_paths(name, params, workspace);
if deletes.is_empty() {
deletes
} else {
@@ -466,6 +524,7 @@ impl<'a> AutoReviewContext<'a> {
.unwrap_or(deletes)
}
},
tool_name,
}
}
}
@@ -513,7 +572,7 @@ impl AutoReviewRule {
fn matches(&self, ctx: &AutoReviewContext<'_>) -> bool {
if let Some(tool_name) = self.tool_name.as_deref()
&& tool_name != ctx.tool_name
&& tool_name != ctx.tool_name.as_ref()
{
return false;
}
@@ -906,9 +965,9 @@ pub(crate) fn build_reviewer_context(
use codewhale_secrets::redact::{redact_json_model_bound_secrets, redact_model_bound_secrets};
let input = redact_json_model_bound_secrets(tool_input);
let tool = redact_model_bound_secrets(ctx.tool_name);
let tool = redact_model_bound_secrets(ctx.tool_name.as_ref());
let hold_reason = redact_model_bound_secrets(held_reason);
let credentials_masked = input != *tool_input || tool != ctx.tool_name;
let credentials_masked = input != *tool_input || tool != ctx.tool_name.as_ref();
serde_json::to_string(&serde_json::json!({
"proposed_tool_call": {
"tool": tool,
@@ -2630,6 +2689,81 @@ mod tests {
)
}
#[cfg(unix)]
#[tokio::test(flavor = "current_thread")]
async fn auto_review_filesystem_evidence_does_not_park_runtime() {
use std::os::unix::ffi::OsStrExt;
use std::os::unix::fs::OpenOptionsExt;
use std::time::{Duration, Instant};
let dir = patch_workspace();
let config_path = dir.path().join(".git/config");
let saved_config_path = dir.path().join(".git/config.saved");
std::fs::rename(&config_path, &saved_config_path).unwrap();
let path = std::ffi::CString::new(config_path.as_os_str().as_bytes()).unwrap();
// SAFETY: a live, NUL-terminated path inside this test's private repo.
assert_eq!(unsafe { libc::mkfifo(path.as_ptr(), 0o600) }, 0);
let (opened_tx, opened_rx) = tokio::sync::oneshot::channel();
let (release_tx, release_rx) = std::sync::mpsc::channel();
let writer = std::thread::spawn(move || {
// Nonblocking writer-open bounds the case where admission never
// reaches Git. Once Git reads the FIFO, an independent OS-thread
// watchdog also releases it if the current-thread runtime stalls.
let deadline = Instant::now() + Duration::from_secs(5);
let pipe = loop {
match std::fs::OpenOptions::new()
.write(true)
.custom_flags(libc::O_NONBLOCK)
.open(&config_path)
{
Ok(pipe) => break Some(pipe),
Err(_) if Instant::now() < deadline => {
std::thread::sleep(Duration::from_millis(5));
}
Err(_) => break None,
}
};
let peer_released = if pipe.is_some() {
let _ = opened_tx.send(());
release_rx.recv_timeout(Duration::from_secs(5)).is_ok()
} else {
false
};
// Replace the FIFO atomically before closing this writer: later
// Git opens see the original regular file, while the waiting
// reader receives EOF. No restoration write can block on a FIFO.
let restored = std::fs::rename(&saved_config_path, &config_path);
drop(pipe);
restored.unwrap();
peer_released
});
let workspace = dir.path().to_path_buf();
let params = delete_patch("untracked.txt", "only copy");
let review = tokio::spawn(async move {
AutoReviewContext::from_tool_call_async(
"apply_patch",
&params,
RunOrigin::Interactive,
ApprovalMode::Auto,
Some(&workspace),
)
.await
});
let opened = tokio::time::timeout(Duration::from_secs(12), opened_rx).await;
let peer_ran_before_watchdog =
opened.is_ok_and(|result| result.is_ok()) && release_tx.send(()).is_ok();
let context = review.await.unwrap().unwrap();
let released_by_peer = writer.join().unwrap();
assert!(
peer_ran_before_watchdog && released_by_peer,
"the runtime must release filesystem evidence before the OS-thread watchdog"
);
assert_eq!(context.tool_name, "apply_patch");
assert_eq!(context.unrecoverable_deletes, vec!["untracked.txt"]);
assert!(context.write_targets_bounded);
}
#[test]
fn patch_deletes_git_cannot_restore_are_reviewed() {
use crate::core::engine::{AutoReviewPlanDecision, auto_review_plan_decision_for_context};
crates/tui/tests/feature_registry.rs
+1
View File
@@ -45,6 +45,7 @@ fn repo_root() -> PathBuf {
Path::new(env!("CARGO_MANIFEST_DIR")).join("../..")
}
#[cfg(test)]
fn read(root: &Path, rel: &str) -> String {
std::fs::read_to_string(root.join(rel)).unwrap_or_else(|err| panic!("read {rel}: {err}"))
}
scripts/check-blocking-calls-budget.json
+4 -1
View File
@@ -399,7 +399,7 @@
},
"crates/tui/src/snapshot/repo.rs": {
"path_canonicalize": 4,
"std_fs": 17,
"std_fs": 18,
"thread_sleep": 1
},
"crates/tui/src/tools/git.rs": {
@@ -492,6 +492,9 @@
"crates/tui/src/tui/app/init.rs": {
"std_fs": 1
},
"crates/tui/src/tui/auto_review.rs": {
"path_canonicalize": 3
},
"crates/tui/src/tui/clipboard.rs": {
"std_fs": 2
},