diff --git a/docs/plans/edit-constraint-guard-plan.md b/docs/plans/edit-constraint-guard-plan.md new file mode 100644 index 0000000000..dc8b5c4212 --- /dev/null +++ b/docs/plans/edit-constraint-guard-plan.md @@ -0,0 +1,118 @@ +# Edit Constraint Guard（编辑约束守卫）设计 + +## 目标 + +当用户明确要求“不修改某些文件、目录或文件类型”时，产品应在文件真正写入前执行该约束，并向 +agent 返回可解释的拒绝原因。约束属于用户会话状态，必须在恢复、fork 和回滚后保持一致。 + +该能力是通用产品行为，不承担评测环境隔离职责。 + +## 边界 + +产品内负责： + +- 从每条新的用户指令中抽取“哪些文件不可修改”的约束； +- 将约束持久化为会话状态，并支持显式追加和撤销； +- 在直接文件工具、shell/WriteStdin、Git 工作树操作和递归删除前执行确定性检查； +- 按显式 opt-in 记录决策和成功文件操作，便于诊断误判与遗漏。 + +产品内不负责： + +- 屏蔽 WebFetch、WebSearch、GitHub、Sourcegraph 或任意代码托管域名； +- 禁用 Git 网络、远端引用、commit hash 或历史读取； +- 根据 benchmark、job 名称、环境变量或分支切换工具能力； +- 最终 patch 过滤、基准泄漏判定或评分。 + +网络出口、仓库历史净化和评测审计属于评测管道。产品工具保持普通用户预期的 Web 与 Git +能力，guard 代码不得出现 benchmark 名称或站点黑名单。 + +## 模块与职责 + +| 模块 | 职责 | +|---|---| +| `edit_constraint_guard.rs` | 对外兼容入口；抽取编排、确定性决策、递归删除检查、telemetry | +| `edit_constraint_guard/model.rs` | 可序列化约束、抽取记录、会话状态合并与回滚 | +| `edit_constraint_guard/shell_targets.rs` | 从常见 shell 命令中识别显式文件变更目标 | +| `session_manager.rs` | 状态持久化、恢复、fork 继承与按存活 turn 回滚 | +| 文件与 shell 工具的 `validate_input` | 在执行前调用 guard；不复制约束策略 | + +`edit_constraint_guard.rs` 保留既有 public re-export，避免状态类型的存储路径因内部拆分而 +改变。模型、解析器和执行编排之间只传递结构化约束或路径列表。 + +## 状态模型 + +每条约束包含稳定 id、人类可读描述、来源、操作范围和一个受限 matcher： + +- `test_files`：常见测试文件与测试目录约定； +- `path_contains`：路径包含明确字面值； +- `path_under_dir`：路径位于明确目录下； +- `extension`：文件扩展名匹配； +- `unmatched`：只记录，不执行。 + +操作范围为 `all` 或 `delete_only`。自由正则和自由 glob 不进入持久化契约，避免模型生成 +不可预测的匹配规则。 + +状态同时保存： + +- fork 时继承的 active constraints 与 agent-created paths 基线； +- 每次抽取的输入摘要、状态、耗时、模型输出摘要和失败原因； +- 生效、撤销及无法匹配的撤销 id； +- agent 创建文件的路径和创建它的 dialog turn。 + +父会话状态在 fork 时固化为子会话基线。子会话回滚从该基线开始，只重放仍然存活的子会话 +turn 抽取与文件来源记录；旧格式中缺少 turn id 的来源记录不授予豁免。 + +## 生命周期 + +1. execution engine 对每个不同的用户 turn 处理一次约束抽取。 +2. 明确的测试文件禁改措辞先由确定性句法规则兜底；规则要求禁止词、变更动作与测试文件目标 + 形成直接关系，不以关键词共现推断约束。fast 模型补充其他 matcher，并返回显式撤销。 +3. 有约束、发生模型尝试或抽取失败时才将结果写入 session metadata；普通无信号 turn 不产生 + guard 状态，也不触发 metadata 重写。抽取失败单独记录，执行阶段 fail open。 +4. 文件工具在 `validate_input` 中调用统一检查；shell、WriteStdin 和 Git 工具先将可静态识别的 + 变更目标解析为路径。存在 active constraint 时，无法解析目标的高风险变更命令 fail closed。 +5. 命中约束时返回 403 和结构化 `edit_constraint_guard` 元数据，不执行写入。 +6. 设置 `BITFUN_EDIT_CONSTRAINT_TELEMETRY=1` 后，guard 决策和成功的直接文件工具操作写入 + session-scoped JSONL，用于产品诊断；默认不创建 JSONL。该开关不影响 AI provider 请求审计。 + +撤销只接受当前 active constraint id，且只有真实用户提交的 turn 可以授权撤销。含糊表达、 +未知 id 或模型解析失败都不会放宽既有约束。 + +## 执行规则 + +- matcher 执行是确定性的，不在每次工具调用时请求模型。 +- 普通无约束会话在工具热路径上不解析 shell、不解析路径、不查询远程文件是否存在，也不写 + session metadata 或 JSONL；只有 TestFiles 全操作约束需要维护 agent-created 文件来源。 +- `force` 不是模型可用的逃生口；旧调用携带 `force` 时拒绝并记录。 +- “不要修改测试”允许新建 agent 自己的测试辅助文件，也允许后续修改或删除该辅助文件。 +- `delete_only` 约束不因 agent-created provenance 放宽。 +- 递归删除先检查目标和所有非符号链接后代；存在生效约束时，无法完成检查则 fail closed。 +- shell 解析优先检查明确目标；存在 active constraint 时，变量/glob 目标、交互式 shell、未知 + archive/patch 内容及其他无法确定影响范围的高风险变更命令在执行前拒绝。无约束会话不改变 + 普通 shell、WriteStdin 或 Git 行为。 + +## 关键不变量 + +- 无 active constraint 时，guard 不改变普通编辑、Web 或 Git 行为，也不执行路径/远程存在性检查。 +- 同一 dialog turn 和消息 hash 不重复抽取。 +- 未知撤销 id 永远不删除约束。 +- session restore 与 fork 后的 active constraints 与父会话一致。 +- rollback 后不能保留来自已删除 turn 的约束或 agent-created 豁免。 +- 拒绝发生在写入前，递归删除不会出现部分删除后才发现受保护文件。 +- telemetry 默认关闭；启用后的写入失败不能导致普通工具操作失败。 + +## 验证策略 + +最小测试集覆盖： + +- matcher 规则和 operation scope； +- 确定性抽取、模型解析失败、合法与非法撤销； +- 新建测试辅助文件、已有测试文件和 delete-only 的差异； +- shell 重定向、`tee`、`cp`、`mv`、`rm`、in-place `sed/perl`、Python、Node、WriteStdin、 + Git pathspec 与无法解析目标的高风险命令； +- 递归删除、符号链接、远程工作区检查失败； +- session 持久化、fork、回滚和旧 schema 兼容； +- 普通 Web/Git 命令不被评测策略拦截。 + +Rust 变更至少运行该 crate 的 focused tests 和 `cargo check --workspace`。行为边界变化必须先 +更新本文，再改实现；大提交的说明应记录动机、边界、失败策略和验证结果。 diff --git a/src/apps/cli/src/main.rs b/src/apps/cli/src/main.rs index 2e8044d25d..949848ba83 100644 --- a/src/apps/cli/src/main.rs +++ b/src/apps/cli/src/main.rs @@ -61,6 +61,13 @@ pub fn get_mcp_status_text() -> String { } } +fn final_change_verification_enabled( + verify_final_changes: bool, + no_verify_final_changes: bool, +) -> bool { + verify_final_changes && !no_verify_final_changes +} + /// Get the global MCP service instance (if initialized) pub fn get_mcp_service() -> Option<&'static std::sync::Arc> { MCP_SERVICE.get() @@ -129,6 +136,14 @@ enum Commands { #[arg(long, num_args = 0..=1, default_missing_value = "-")] output_patch: Option, + /// Verify workspace changes before a successful headless exit (enabled by default) + #[arg(long, default_value_t = true, action = clap::ArgAction::SetTrue)] + verify_final_changes: bool, + + /// Disable automatic final-change verification + #[arg(long, conflicts_with = "verify_final_changes")] + no_verify_final_changes: bool, + /// Auto-approve tool permissions that are not explicitly denied #[arg(long, conflicts_with = "confirm")] auto: bool, @@ -835,6 +850,8 @@ async fn run_cli() -> Result<()> { fork_session, output_format, output_patch, + verify_final_changes, + no_verify_final_changes, auto, confirm, }) => { @@ -860,6 +877,10 @@ async fn run_cli() -> Result<()> { fork_session, output_format, output_patch, + verify_final_changes: final_change_verification_enabled( + verify_final_changes, + no_verify_final_changes, + ), approval_mode, }, ) @@ -1361,3 +1382,32 @@ mod bootstrap_profile_tests { assert!(exec_requests_json_output(&args)); } } + +#[cfg(test)] +mod final_change_verification_cli_tests { + use super::{final_change_verification_enabled, Cli, Commands}; + use clap::Parser; + + fn parse_flags(args: &[&str]) -> (bool, bool) { + let cli = Cli::try_parse_from(args).expect("exec args"); + let Some(Commands::Exec { + verify_final_changes, + no_verify_final_changes, + .. + }) = cli.command + else { + panic!("expected exec command"); + }; + (verify_final_changes, no_verify_final_changes) + } + + #[test] + fn verification_is_enabled_by_default_and_can_be_disabled() { + let (verify, disable) = parse_flags(&["bitfun", "exec", "task"]); + assert!(final_change_verification_enabled(verify, disable)); + + let (verify, disable) = + parse_flags(&["bitfun", "exec", "--no-verify-final-changes", "task"]); + assert!(!final_change_verification_enabled(verify, disable)); + } +} diff --git a/src/apps/cli/src/modes/exec.rs b/src/apps/cli/src/modes/exec.rs index fe7bdd8087..ae7d04ae89 100644 --- a/src/apps/cli/src/modes/exec.rs +++ b/src/apps/cli/src/modes/exec.rs @@ -2,6 +2,7 @@ mod lifecycle; mod patch; #[cfg(test)] mod tests; +mod verification; pub(crate) use lifecycle::{ emit_preflight_json_error, ExecApprovalMode, ExecMode, ExecOutputFormat, ExecSessionOptions, diff --git a/src/apps/cli/src/modes/exec/lifecycle.rs b/src/apps/cli/src/modes/exec/lifecycle.rs index 3f3237120e..668f209fa7 100644 --- a/src/apps/cli/src/modes/exec/lifecycle.rs +++ b/src/apps/cli/src/modes/exec/lifecycle.rs @@ -125,6 +125,8 @@ pub(super) struct ExecJsonResult { usage: Option, #[serde(skip_serializing_if = "Option::is_none")] patch: Option, + #[serde(skip_serializing_if = "Option::is_none")] + verification: Option, } #[derive(Clone, Debug, PartialEq, Eq, Serialize)] @@ -400,6 +402,7 @@ impl ExecJsonResult { turn_id, usage, patch: None, + verification: None, } } @@ -407,6 +410,14 @@ impl ExecJsonResult { self.patch = patch; self } + + fn with_verification( + mut self, + verification: Option, + ) -> Self { + self.verification = verification; + self + } } pub(crate) fn emit_preflight_json_error( @@ -442,8 +453,17 @@ pub(crate) struct ExecMode { agent: Arc, runtime: Arc, pub(super) workspace_path: Option, + /// Git tree captured before execution so committed agent changes remain + /// visible to patch export and final verification. + pub(super) initial_diff_base: Option, + /// Untracked files that predate execution belong to the caller, not the + /// agent, and must not leak into its patch or verification scope. + pub(super) initial_untracked_files: std::collections::BTreeSet, /// None: no patch output, Some("-"): output to stdout, Some(path): save to file pub(super) output_patch: Option, + pub(super) verify_final_changes: bool, + pub(super) verification_retries_used: u32, + pub(super) latest_verification: Option, pub(super) output_format: ExecOutputFormat, approval_mode: ExecApprovalMode, session_options: ExecSessionOptions, @@ -457,6 +477,7 @@ impl ExecMode { runtime: Arc, workspace_path: Option, output_patch: Option, + verify_final_changes: bool, output_format: ExecOutputFormat, session_options: ExecSessionOptions, ) -> Self { @@ -470,6 +491,18 @@ impl ExecMode { runtime.as_ref(), workspace_path.clone(), )); + let (initial_diff_base, initial_untracked_files) = + if super::verification::needs_change_baseline( + output_patch.as_deref(), + verify_final_changes, + ) { + workspace_path + .as_deref() + .map(super::patch::capture_change_baseline) + .unwrap_or_default() + } else { + Default::default() + }; Self { config, @@ -478,7 +511,12 @@ impl ExecMode { agent, runtime, workspace_path, + initial_diff_base, + initial_untracked_files, output_patch, + verify_final_changes, + verification_retries_used: 0, + latest_verification: None, output_format, approval_mode, session_options, @@ -1003,6 +1041,109 @@ impl ExecMode { } } }; + + if turn_settled + && terminal_outcome + .as_ref() + .is_some_and(|outcome| outcome.is_ok()) + && self.verify_final_changes + { + let workspace = self + .workspace_path + .clone() + .or_else(|| std::env::current_dir().ok()) + .unwrap_or_else(|| PathBuf::from(".")); + let verification_workspace = + super::patch::repository_root(&workspace).unwrap_or(workspace); + if let Some(command) = super::verification::detect_verify_command( + &verification_workspace, + self.initial_diff_base.as_deref(), + &self.initial_untracked_files, + ) { + let config = super::verification::VerifyConfig::from_env(); + let verification = super::verification::run_verifier( + &verification_workspace, + &command, + &config, + self.verification_retries_used, + ) + .await; + self.latest_verification = Some(verification.clone()); + + if self.output_format == ExecOutputFormat::StreamJson { + println!( + "{}", + serde_json::to_string(&serde_json::json!({ + "type": "verify_result", + "session_id": &session_id, + "attempt": self.verification_retries_used + 1, + "status": verification.status, + "command": &verification.command, + "exit_code": verification.exit_code, + "duration_ms": verification.duration_ms, + "output_tail": &verification.output_tail, + }))? + ); + } + + if verification.status == super::verification::VerifyStatus::Passed { + self.print_text(|| { + eprintln!( + "\nFinal-change verification passed: {}", + verification.command + ) + }); + } else if self.verification_retries_used < config.max_retries { + self.print_text(|| { + eprintln!( + "\nFinal-change verification failed (attempt {}, exit {:?}): {}", + self.verification_retries_used + 1, + verification.exit_code, + verification.command + ); + eprintln!("Asking the agent to repair the verified changes."); + }); + + let mut retry = ExecMode::new( + self.config.clone(), + super::verification::build_retry_message(&verification), + self.agent_type.clone(), + self.runtime.clone(), + self.workspace_path.clone(), + self.output_patch.clone(), + self.verify_final_changes, + self.output_format, + ExecSessionOptions { + resume: Some(session_id.clone()), + ..Default::default() + }, + ); + retry.initial_diff_base = self.initial_diff_base.clone(); + retry.initial_untracked_files = self.initial_untracked_files.clone(); + retry.verification_retries_used = self.verification_retries_used + 1; + return Box::pin(retry.run()).await; + } else { + self.print_text(|| { + eprintln!( + "\nFinal-change verification is still failing after {} attempt(s); \ + finishing with unverified changes: {}", + self.verification_retries_used + 1, + verification.command + ) + }); + } + } else if self.output_format == ExecOutputFormat::StreamJson { + println!( + "{}", + serde_json::to_string(&serde_json::json!({ + "type": "verify_skipped", + "session_id": &session_id, + "reason": "no_applicable_changed_files", + }))? + ); + } + } + let (patch, patch_error) = if turn_settled { self.output_patch_if_needed() } else { @@ -1054,6 +1195,7 @@ impl ExecMode { } } .with_patch(patch); + let result = result.with_verification(self.latest_verification.clone()); println!("{}", serde_json::to_string_pretty(&result)?); } terminal_outcome diff --git a/src/apps/cli/src/modes/exec/patch.rs b/src/apps/cli/src/modes/exec/patch.rs index b5ee0a7cf8..84ae610ad8 100644 --- a/src/apps/cli/src/modes/exec/patch.rs +++ b/src/apps/cli/src/modes/exec/patch.rs @@ -1,4 +1,5 @@ use std::path::PathBuf; +use std::{collections::BTreeSet, path::Path}; use crate::diagnostics::ExitKind; @@ -7,12 +8,36 @@ use super::lifecycle::{ExecMode, ExecOutputFormat, ExecPatchOutput}; impl ExecMode { fn get_git_diff(&self) -> Option { let workspace = self.workspace_path.as_ref()?; - Self::get_git_diff_for_workspace(workspace, self.output_patch.as_deref()) + Self::get_git_diff_from_baseline( + workspace, + self.initial_diff_base.as_deref(), + &self.initial_untracked_files, + self.output_patch.as_deref(), + ) } + #[cfg(test)] pub(super) fn get_git_diff_for_workspace( workspace: &std::path::Path, output_target: Option<&str>, + ) -> Option { + let diff_base = git_diff_base(workspace); + // This helper is also used after fixture changes in unit tests, where + // existing untracked files are intentionally part of the requested + // snapshot. Runtime callers use the execution-start snapshot above. + Self::get_git_diff_from_baseline( + workspace, + diff_base.as_deref(), + &BTreeSet::new(), + output_target, + ) + } + + pub(super) fn get_git_diff_from_baseline( + workspace: &Path, + diff_base: Option<&str>, + initial_untracked_files: &BTreeSet, + output_target: Option<&str>, ) -> Option { let repo_root_output = bitfun_core::util::process_manager::create_command("git") .args(["rev-parse", "--show-toplevel"]) @@ -40,9 +65,10 @@ impl ExecMode { .then(|| relative.to_string_lossy().replace('\\', "/")) }); + let diff_base = diff_base?; let mut tracked_command = bitfun_core::util::process_manager::create_command("git"); tracked_command - .args(["diff", "--binary", "--no-color", "HEAD", "--", "."]) + .args(["diff", "--binary", "--no-color", diff_base, "--", "."]) .current_dir(&repo_root); if let Some(relative_path) = excluded_output.as_ref() { tracked_command.arg(format!(":(exclude,top,literal){relative_path}")); @@ -69,6 +95,9 @@ impl ExecMode { continue; } let relative_path = String::from_utf8_lossy(relative_path).to_string(); + if initial_untracked_files.contains(&relative_path) { + continue; + } if excluded_output.as_deref() == Some(relative_path.as_str()) { continue; } @@ -193,6 +222,74 @@ impl ExecMode { } } +pub(super) fn capture_change_baseline(workspace: &Path) -> (Option, BTreeSet) { + let root = repository_root(workspace).unwrap_or_else(|| workspace.to_path_buf()); + (git_diff_base(&root), untracked_files(&root)) +} + +pub(super) fn repository_root(workspace: &Path) -> Option { + let output = bitfun_core::util::process_manager::create_command("git") + .args(["rev-parse", "--show-toplevel"]) + .current_dir(workspace) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let root = String::from_utf8_lossy(&output.stdout).trim().to_string(); + (!root.is_empty()).then(|| PathBuf::from(root)) +} + +pub(super) fn git_diff_base(workspace: &Path) -> Option { + let head = bitfun_core::util::process_manager::create_command("git") + .args(["rev-parse", "--verify", "HEAD"]) + .current_dir(workspace) + .output() + .ok(); + if let Some(output) = head.filter(|output| output.status.success()) { + let head = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if !head.is_empty() { + return Some(head); + } + } + + // Repositories without an initial commit still need a stable base if the + // agent creates its first commit. + let empty_tree = bitfun_core::util::process_manager::create_command("git") + .args(["hash-object", "-t", "tree", "--stdin"]) + .current_dir(workspace) + .output() + .ok()?; + if !empty_tree.status.success() { + return None; + } + let oid = String::from_utf8_lossy(&empty_tree.stdout) + .trim() + .to_string(); + (!oid.is_empty()).then_some(oid) +} + +pub(super) fn untracked_files(workspace: &Path) -> BTreeSet { + let output = bitfun_core::util::process_manager::create_command("git") + .args(["ls-files", "--others", "--exclude-standard", "-z"]) + .current_dir(workspace) + .output(); + match output { + Ok(output) if output.status.success() => { + nul_separated_paths(&output.stdout).into_iter().collect() + } + _ => BTreeSet::new(), + } +} + +pub(super) fn nul_separated_paths(output: &[u8]) -> Vec { + output + .split(|byte| *byte == 0) + .filter(|path| !path.is_empty()) + .map(|path| String::from_utf8_lossy(path).to_string()) + .collect() +} + pub(super) fn write_patch_to_path(output_target: &str, patch: &str) -> std::io::Result<()> { use std::path::Path; diff --git a/src/apps/cli/src/modes/exec/tests.rs b/src/apps/cli/src/modes/exec/tests.rs index 03ec496348..ef40b057da 100644 --- a/src/apps/cli/src/modes/exec/tests.rs +++ b/src/apps/cli/src/modes/exec/tests.rs @@ -8,6 +8,8 @@ use super::lifecycle::{ ExecApprovalMode, ExecJsonResult, ExecMode, ExecTokenUsage, TOOL_START_INPUT_PREVIEW_CHARS, }; use super::patch::write_patch_to_path; +use super::patch::{git_diff_base, untracked_files}; +use super::verification::{changed_files, detect_verify_command, needs_change_baseline}; use crate::diagnostics::ExitKind; use bitfun_agent_runtime::sdk::{ PermissionDelegationContext, PermissionRequest, PermissionRequestSource, @@ -15,6 +17,7 @@ use bitfun_agent_runtime::sdk::{ }; use bitfun_events::{AgenticEvent, AgenticEventEnvelope, AgenticEventPriority, ToolEventIdentity}; use serde_json::json; +use std::collections::BTreeSet; fn delegated_permission_request() -> PermissionRequest { PermissionRequest { @@ -672,3 +675,135 @@ fn deferred_exec_event_projects_effective_name_and_input() { assert_eq!(input, &json!({ "title": "Ship deferred tools" })); assert_eq!(wire_input["tool_name"], "CreatePlan"); } + +fn run_git(workspace: &std::path::Path, args: &[&str]) { + let output = Command::new("git") + .args(args) + .current_dir(workspace) + .output() + .expect("run git"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn commit_verification_fixture(workspace: &std::path::Path) { + run_git(workspace, &["init", "--quiet"]); + run_git(workspace, &["add", "."]); + run_git( + workspace, + &[ + "-c", + "user.name=BitFun Test", + "-c", + "user.email=bitfun-test@example.invalid", + "commit", + "--quiet", + "-m", + "fixture", + ], + ); +} + +#[test] +fn final_verification_captures_a_baseline_without_patch_output() { + assert!(needs_change_baseline(None, true)); + assert!(needs_change_baseline(Some("result.patch"), false)); + assert!(!needs_change_baseline(None, false)); +} + +#[test] +fn patch_and_verifier_keep_agent_commits_and_exclude_preexisting_untracked_files() { + let temp = tempfile::tempdir().expect("tempdir"); + let workspace = temp.path(); + std::fs::write(workspace.join("tracked.py"), "value = 1\n").expect("tracked file"); + commit_verification_fixture(workspace); + std::fs::write(workspace.join("private.py"), "private = True\n") + .expect("preexisting untracked"); + + let base = git_diff_base(workspace).expect("execution base"); + let initial_untracked = untracked_files(workspace); + std::fs::write(workspace.join("tracked.py"), "value = 2\n").expect("agent edit"); + std::fs::write(workspace.join("created.py"), "created = True\n").expect("agent file"); + run_git(workspace, &["add", "tracked.py"]); + run_git( + workspace, + &[ + "-c", + "user.name=BitFun Test", + "-c", + "user.email=bitfun-test@example.invalid", + "commit", + "--quiet", + "-m", + "agent commit", + ], + ); + + let changed = changed_files(workspace, Some(&base), &initial_untracked); + let patch = + ExecMode::get_git_diff_from_baseline(workspace, Some(&base), &initial_untracked, None) + .expect("patch"); + + assert_eq!( + changed, + vec!["created.py".to_string(), "tracked.py".to_string()] + ); + assert!(patch.contains("value = 2"), "{patch}"); + assert!(patch.contains("created = True"), "{patch}"); + assert!(!patch.contains("private = True"), "{patch}"); +} + +#[test] +fn automatic_verifier_composes_scoped_go_and_typescript_checks() { + let temp = tempfile::tempdir().expect("tempdir"); + let workspace = temp.path(); + std::fs::write(workspace.join("go.mod"), "module example.com/fixture\n").expect("go.mod"); + std::fs::write( + workspace.join("tsconfig.json"), + r#"{"compilerOptions":{"noEmit":true},"include":["web"]}"#, + ) + .expect("tsconfig"); + std::fs::create_dir_all(workspace.join("pkg")).expect("go package"); + std::fs::create_dir_all(workspace.join("web")).expect("web package"); + let go_source = workspace.join("pkg/example.go"); + let ts_source = workspace.join("web/example.ts"); + std::fs::write(&go_source, "package pkg\n\nconst Value = 1\n").expect("go source"); + std::fs::write(&ts_source, "export const value: number = 1;\n").expect("ts source"); + commit_verification_fixture(workspace); + std::fs::write(&go_source, "package pkg\n\nconst Value = 2\n").expect("modify go"); + std::fs::write(&ts_source, "export const value: number = 2;\n").expect("modify ts"); + + let command = + detect_verify_command(workspace, Some("HEAD"), &BTreeSet::new()).expect("verifier"); + assert!(command.contains("go vet -printf=false -composites=false -stdmethods=false './pkg'")); + assert!(command.contains("npx --no-install tsc --noEmit -p 'tsconfig.json'")); +} + +#[test] +fn automatic_verifier_checks_the_nearest_cargo_package_and_test_target() { + let temp = tempfile::tempdir().expect("tempdir"); + let workspace = temp.path(); + std::fs::write( + workspace.join("Cargo.toml"), + "[package]\nname = \"fixture\"\nversion = \"0.1.0\"\nedition = \"2021\"\n", + ) + .expect("Cargo.toml"); + std::fs::create_dir_all(workspace.join("src")).expect("src"); + std::fs::create_dir_all(workspace.join("tests")).expect("tests"); + std::fs::write(workspace.join("src/lib.rs"), "pub fn value() {}\n").expect("lib"); + let test = workspace.join("tests/integration.rs"); + std::fs::write(&test, "#[test]\nfn works() {}\n").expect("test"); + commit_verification_fixture(workspace); + std::fs::write(&test, "#[test]\nfn still_works() {}\n").expect("modify test"); + + let command = + detect_verify_command(workspace, Some("HEAD"), &BTreeSet::new()).expect("verifier"); + assert!(command + .contains("cargo check --manifest-path 'Cargo.toml' -p 'fixture' --message-format=short")); + assert!(command.contains( + "cargo check --manifest-path 'Cargo.toml' -p 'fixture' --test 'integration' --message-format=short" + )); +} diff --git a/src/apps/cli/src/modes/exec/verification.rs b/src/apps/cli/src/modes/exec/verification.rs new file mode 100644 index 0000000000..027e03a952 --- /dev/null +++ b/src/apps/cli/src/modes/exec/verification.rs @@ -0,0 +1,494 @@ +use serde::Serialize; +use std::collections::{BTreeMap, BTreeSet}; +use std::path::{Path, PathBuf}; +use std::time::Duration; +use tokio::time::Instant; + +use super::patch::nul_separated_paths; + +pub(super) fn needs_change_baseline( + output_patch: Option<&str>, + verify_final_changes: bool, +) -> bool { + output_patch.is_some() || verify_final_changes +} + +#[derive(Debug, Clone)] +pub(super) struct VerifyConfig { + pub(super) timeout: Duration, + pub(super) max_retries: u32, +} + +impl VerifyConfig { + pub(super) fn from_env() -> Self { + let timeout = std::env::var("BITFUN_PATCH_VERIFY_TIMEOUT_SEC") + .ok() + .and_then(|value| value.trim().parse::().ok()) + .map(Duration::from_secs) + .unwrap_or_else(|| Duration::from_secs(900)); + let max_retries = std::env::var("BITFUN_PATCH_VERIFY_MAX_RETRIES") + .ok() + .and_then(|value| value.trim().parse::().ok()) + .unwrap_or(1); + Self { + timeout, + max_retries, + } + } +} + +#[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(super) enum VerifyStatus { + Passed, + Failed, + TimedOut, + SpawnError, +} + +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub(super) struct VerifyOutcome { + pub(super) status: VerifyStatus, + pub(super) command: String, + pub(super) exit_code: Option, + pub(super) duration_ms: u64, + pub(super) output_tail: String, + pub(super) retries_used: u32, +} + +pub(super) async fn run_verifier( + workspace: &Path, + command: &str, + config: &VerifyConfig, + retries_used: u32, +) -> VerifyOutcome { + let mut process = if cfg!(windows) { + let mut process = tokio::process::Command::new("cmd"); + process.arg("/C").arg(command); + process + } else { + let mut process = tokio::process::Command::new("sh"); + process.arg("-c").arg(command); + process + }; + process.current_dir(workspace); + process.kill_on_drop(true); + + let started = Instant::now(); + let result = tokio::time::timeout(config.timeout, process.output()).await; + let duration_ms = started.elapsed().as_millis() as u64; + match result { + Ok(Ok(output)) => { + let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = String::from_utf8_lossy(&output.stdout); + let combined = if stderr.trim().is_empty() { + stdout.to_string() + } else if stdout.trim().is_empty() { + stderr.to_string() + } else { + format!("{stderr}\n--- stdout ---\n{stdout}") + }; + VerifyOutcome { + status: if output.status.success() { + VerifyStatus::Passed + } else { + VerifyStatus::Failed + }, + command: command.to_string(), + exit_code: output.status.code(), + duration_ms, + output_tail: tail_chars(&combined, 4_000), + retries_used, + } + } + Ok(Err(error)) => VerifyOutcome { + status: VerifyStatus::SpawnError, + command: command.to_string(), + exit_code: None, + duration_ms, + output_tail: format!("spawn error: {error}"), + retries_used, + }, + Err(_) => VerifyOutcome { + status: VerifyStatus::TimedOut, + command: command.to_string(), + exit_code: None, + duration_ms, + output_tail: format!("timed out after {}s", config.timeout.as_secs()), + retries_used, + }, + } +} + +fn tail_chars(value: &str, max_chars: usize) -> String { + let count = value.chars().count(); + if count <= max_chars { + value.to_string() + } else { + value.chars().skip(count - max_chars).collect() + } +} + +pub(super) fn build_retry_message(outcome: &VerifyOutcome) -> String { + match outcome.status { + VerifyStatus::TimedOut => format!( + "\n\ +The verifier we ran timed out and did not return a pass/fail signal:\n\n\ +$ {command}\n\ +(timed out after {duration_ms}ms)\n\n\ +Do not rerun the same command verbatim. Run a concretely lighter, scoped check, \ +or finalize with a concrete justification if the changes are correct.\n\ +", + command = outcome.command, + duration_ms = outcome.duration_ms, + ), + VerifyStatus::SpawnError => format!( + "\n\ +The verifier could not start:\n\n\ +$ {command}\n\ +{output}\n\n\ +Use an available, scoped verification command and finalize.\n\ +", + command = outcome.command, + output = outcome.output_tail, + ), + VerifyStatus::Failed => format!( + "\n\ +Your changes did not pass external verification (exit {exit_code:?}):\n\n\ +$ {command}\n\n\ +Last output (truncated to 4000 characters):\n\ +{output}\n\n\ +Diagnose the remaining failure, fix it, and run a relevant scoped check before \ +finalizing. If the failure is unrelated, provide concrete evidence.\n\ +", + exit_code = outcome.exit_code, + command = outcome.command, + output = outcome.output_tail, + ), + VerifyStatus::Passed => { + "Verification passed; no retry is required." + .to_string() + } + } +} + +/// Select checks only for files changed since exec started. Mixed-language +/// changes compose their scoped checks; broad Make/just targets are never +/// inferred merely from their names. +pub(super) fn detect_verify_command( + workspace: &Path, + diff_base: Option<&str>, + initial_untracked_files: &BTreeSet, +) -> Option { + let changed = changed_files(workspace, diff_base, initial_untracked_files); + if changed.is_empty() { + return None; + } + + let mut commands = Vec::new(); + commands.extend(scoped_go_commands(workspace, &changed)); + commands.extend(scoped_cargo_commands(workspace, &changed)); + commands.extend(scoped_typescript_commands(workspace, &changed)); + if let Some(command) = build_parse_only_command(workspace, &changed) { + commands.push(command); + } + (!commands.is_empty()).then(|| commands.join(" && ")) +} + +pub(super) fn changed_files( + workspace: &Path, + diff_base: Option<&str>, + initial_untracked_files: &BTreeSet, +) -> Vec { + let mut files = BTreeSet::new(); + if let Some(diff_base) = diff_base { + if let Ok(output) = bitfun_core::util::process_manager::create_command("git") + .args(["diff", diff_base, "--name-only", "--find-renames", "-z"]) + .current_dir(workspace) + .output() + { + if output.status.success() { + files.extend(nul_separated_paths(&output.stdout)); + } + } + } + if let Ok(output) = bitfun_core::util::process_manager::create_command("git") + .args(["ls-files", "--others", "--exclude-standard", "-z"]) + .current_dir(workspace) + .output() + { + if output.status.success() { + files.extend( + nul_separated_paths(&output.stdout) + .into_iter() + .filter(|path| !initial_untracked_files.contains(path)), + ); + } + } + files.into_iter().collect() +} + +fn scoped_go_commands(workspace: &Path, files: &[String]) -> Vec { + let mut packages_by_module: BTreeMap> = BTreeMap::new(); + let mut manifest_only_modules = BTreeSet::new(); + for file in files { + let path = workspace.join(file); + let lower = file.to_ascii_lowercase(); + let Some(manifest) = find_nearest_manifest(workspace, &path, "go.mod") else { + continue; + }; + let module_dir = manifest.parent().unwrap_or(workspace).to_path_buf(); + if lower.ends_with(".go") { + let package_dir = path.parent().unwrap_or(&module_dir); + let has_go_source = std::fs::read_dir(package_dir) + .ok() + .into_iter() + .flatten() + .any(|entry| { + entry + .ok() + .and_then(|entry| entry.path().extension().map(|ext| ext == "go")) + .unwrap_or(false) + }); + if !has_go_source { + continue; + } + let relative = package_dir.strip_prefix(&module_dir).unwrap_or(package_dir); + let target = if relative.as_os_str().is_empty() { + ".".to_string() + } else { + format!("./{}", relative.to_string_lossy().replace('\\', "/")) + }; + packages_by_module + .entry(module_dir) + .or_default() + .insert(target); + } else if matches!( + Path::new(&lower).file_name().and_then(|name| name.to_str()), + Some("go.mod" | "go.sum") + ) { + manifest_only_modules.insert(module_dir); + } + } + + let mut commands = Vec::new(); + for (module_dir, packages) in &packages_by_module { + let targets = packages + .iter() + .map(|target| shell_single_quote(target)) + .collect::>() + .join(" "); + commands.push(command_in_directory( + workspace, + module_dir, + &format!("go vet -printf=false -composites=false -stdmethods=false {targets}"), + )); + } + for module_dir in manifest_only_modules { + if !packages_by_module.contains_key(&module_dir) { + commands.push(command_in_directory( + workspace, + &module_dir, + "go list -m all", + )); + } + } + commands +} + +fn scoped_cargo_commands(workspace: &Path, files: &[String]) -> Vec { + let mut packages: BTreeMap, BTreeSet)> = BTreeMap::new(); + for file in files { + let lower = file.to_ascii_lowercase(); + let is_source = lower.ends_with(".rs"); + let is_manifest = matches!( + Path::new(&lower).file_name().and_then(|name| name.to_str()), + Some("cargo.toml" | "cargo.lock") + ); + if !is_source && !is_manifest { + continue; + } + let path = workspace.join(file); + let Some(manifest) = find_nearest_manifest(workspace, &path, "Cargo.toml") else { + continue; + }; + let package = read_cargo_package_name(&manifest); + let integration_test = is_source + .then(|| cargo_integration_test_target(&manifest, &path)) + .flatten(); + let entry = packages + .entry(manifest) + .or_insert_with(|| (package, BTreeSet::new())); + if let Some(target) = integration_test { + entry.1.insert(target); + } + } + + packages + .into_iter() + .flat_map(|(manifest, (package, integration_tests))| { + let manifest = manifest + .strip_prefix(workspace) + .unwrap_or(&manifest) + .to_string_lossy() + .replace('\\', "/"); + match package { + Some(package) => { + let manifest = shell_single_quote(&manifest); + let package = shell_single_quote(&package); + let mut commands = vec![format!( + "cargo check --manifest-path {manifest} -p {package} --message-format=short" + )]; + if !integration_tests.is_empty() { + let targets = integration_tests + .iter() + .map(|target| format!("--test {}", shell_single_quote(target))) + .collect::>() + .join(" "); + commands.push(format!( + "cargo check --manifest-path {manifest} -p {package} {targets} --message-format=short" + )); + } + commands + } + None => vec![format!( + "cargo metadata --no-deps --format-version 1 --manifest-path {}", + shell_single_quote(&manifest) + )], + } + }) + .collect() +} + +fn scoped_typescript_commands(workspace: &Path, files: &[String]) -> Vec { + let mut configs = BTreeSet::new(); + for file in files { + let lower = file.to_ascii_lowercase(); + let is_source = [".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"] + .iter() + .any(|extension| lower.ends_with(extension)); + let is_config = + Path::new(&lower).file_name().and_then(|name| name.to_str()) == Some("tsconfig.json"); + if !is_source && !is_config { + continue; + } + if let Some(config) = + find_nearest_manifest(workspace, &workspace.join(file), "tsconfig.json") + { + configs.insert(config); + } + } + configs + .into_iter() + .map(|config| { + let relative = config + .strip_prefix(workspace) + .unwrap_or(&config) + .to_string_lossy() + .replace('\\', "/"); + format!( + "npx --no-install tsc --noEmit -p {}", + shell_single_quote(&relative) + ) + }) + .collect() +} + +fn find_nearest_manifest(workspace: &Path, file: &Path, name: &str) -> Option { + let mut current = file.parent()?; + loop { + let manifest = current.join(name); + if manifest.is_file() { + return Some(manifest); + } + if current == workspace { + return None; + } + current = current.parent()?; + } +} + +fn cargo_integration_test_target(manifest: &Path, source: &Path) -> Option { + if !source.is_file() { + return None; + } + let relative = source.strip_prefix(manifest.parent()?).ok()?; + let mut components = relative.components(); + if components.next()?.as_os_str() != "tests" { + return None; + } + let target = components.next()?.as_os_str().to_str()?; + if components.next().is_some() { + return None; + } + Path::new(target) + .file_stem() + .and_then(|stem| stem.to_str()) + .map(ToString::to_string) +} + +fn read_cargo_package_name(manifest: &Path) -> Option { + let content = std::fs::read_to_string(manifest).ok()?; + let mut in_package = false; + for line in content.lines() { + let trimmed = line.trim(); + if trimmed.is_empty() || trimmed.starts_with('#') { + continue; + } + if let Some(rest) = trimmed.strip_prefix('[') { + in_package = rest.starts_with("package]"); + continue; + } + if in_package { + if let Some(rest) = trimmed.strip_prefix("name") { + if let Some(rest) = rest.trim_start().strip_prefix('=') { + let value = rest.split('#').next().unwrap_or(rest).trim(); + let value = value.trim_matches(|character| matches!(character, '"' | '\'')); + if !value.is_empty() { + return Some(value.to_string()); + } + } + } + } + } + None +} + +fn build_parse_only_command(workspace: &Path, files: &[String]) -> Option { + let mut checks = Vec::new(); + for file in files { + if !workspace.join(file).is_file() { + continue; + } + let quoted = shell_single_quote(file); + let lower = file.to_ascii_lowercase(); + if lower.ends_with(".py") { + checks.push(format!( + "python3 -c 'import ast,sys; ast.parse(open(sys.argv[1]).read())' {quoted}" + )); + } else if lower.ends_with(".js") || lower.ends_with(".mjs") || lower.ends_with(".cjs") { + checks.push(format!("node --check {quoted}")); + } else if lower.ends_with(".go") + && find_nearest_manifest(workspace, &workspace.join(file), "go.mod").is_none() + { + checks.push(format!("gofmt -e -d {quoted}")); + } + } + (!checks.is_empty()).then(|| checks.join(" && ")) +} + +fn command_in_directory(workspace: &Path, directory: &Path, command: &str) -> String { + let relative = directory.strip_prefix(workspace).unwrap_or(directory); + if relative.as_os_str().is_empty() { + command.to_string() + } else { + format!( + "(cd {} && {command})", + shell_single_quote(&relative.to_string_lossy().replace('\\', "/")) + ) + } +} + +fn shell_single_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', r"'\''")) +} diff --git a/src/apps/cli/src/root_handlers.rs b/src/apps/cli/src/root_handlers.rs index 307e57ffdf..8b6d410e3e 100644 --- a/src/apps/cli/src/root_handlers.rs +++ b/src/apps/cli/src/root_handlers.rs @@ -37,6 +37,7 @@ pub(crate) struct ExecCommandArgs { pub fork_session: bool, pub output_format: ExecOutputFormat, pub output_patch: Option, + pub verify_final_changes: bool, pub approval_mode: ExecApprovalMode, } @@ -137,6 +138,7 @@ pub(crate) async fn handle_exec_command(config: CliConfig, args: ExecCommandArgs runtime.clone(), workspace_path_resolved, args.output_patch, + args.verify_final_changes, args.output_format, ExecSessionOptions { resume, diff --git a/src/crates/adapters/ai-adapters/src/providers/openai/message_converter.rs b/src/crates/adapters/ai-adapters/src/providers/openai/message_converter.rs index 3ec33276ea..016fe4bffc 100644 --- a/src/crates/adapters/ai-adapters/src/providers/openai/message_converter.rs +++ b/src/crates/adapters/ai-adapters/src/providers/openai/message_converter.rs @@ -195,6 +195,9 @@ impl OpenAIMessageConverter { .ok()? .as_array()? .clone(); + if items.is_empty() { + return None; + } let text_item_type = Self::responses_text_item_type(role); let mut content_items = Vec::with_capacity(items.len()); @@ -227,6 +230,9 @@ impl OpenAIMessageConverter { .ok()? .as_array()? .clone(); + if items.is_empty() { + return None; + } let mut content_items = Vec::with_capacity(items.len()); for item in items { diff --git a/src/crates/adapters/ai-adapters/tests/openai_empty_content_parts.rs b/src/crates/adapters/ai-adapters/tests/openai_empty_content_parts.rs new file mode 100644 index 0000000000..f5edc54553 --- /dev/null +++ b/src/crates/adapters/ai-adapters/tests/openai_empty_content_parts.rs @@ -0,0 +1,21 @@ +use bitfun_ai_adapters::providers::openai::OpenAIMessageConverter; +use bitfun_ai_adapters::Message; +use serde_json::json; + +#[test] +fn chat_completions_preserves_empty_json_array_as_text() { + let messages = OpenAIMessageConverter::convert_messages(vec![Message::user("[]".to_string())]); + + assert_eq!(messages[0]["content"], json!("[]")); +} + +#[test] +fn responses_preserves_empty_json_array_as_text() { + let (_, input) = + OpenAIMessageConverter::convert_messages_to_responses_input(vec![Message::user( + "[]".to_string(), + )]); + + assert_eq!(input[0]["content"][0]["type"], json!("input_text")); + assert_eq!(input[0]["content"][0]["text"], json!("[]")); +} diff --git a/src/crates/assembly/core/src/agentic/agents/prompts/agentic_mode.md b/src/crates/assembly/core/src/agentic/agents/prompts/agentic_mode.md index 8ec8c71b89..e03b8bb062 100644 --- a/src/crates/assembly/core/src/agentic/agents/prompts/agentic_mode.md +++ b/src/crates/assembly/core/src/agentic/agents/prompts/agentic_mode.md @@ -50,14 +50,39 @@ When presenting options or plans, never include time estimates - focus on what e # Doing tasks The user will primarily request you perform software engineering tasks. This includes solving bugs, adding new functionality, refactoring code, explaining code, and more. For these tasks the following steps are recommended: - Read relevant code before proposing concrete changes to it. For broad design discussion, state assumptions and inspect files before editing. +- Read nearby tests, examples, and similar implementations when available. Treat tests as executable specifications, but not as the entire specification. +- Before editing, clarify the intended behavior from the task description and any referenced tests: what inputs should work, what outputs are expected, what constraints are explicitly stated. Use this to guide your implementation. Cover equivalent manifestations of the same contract, but do not invent unrelated behavior or requirements. +- Before editing to fix a bug or change behavior, enumerate the *scope of impact* — every place the symptom can surface, not just the first hit. Bugs in shared hooks, decorators, config flags, or polymorphic methods typically have multiple sites: + - Search the symbol with Grep before any edit. Treat the first match as a starting point, not the answer. + - Explicitly enumerate likely variants: function vs method vs class-level, sync vs async, decorated vs undecorated, empty-args vs N-args, language/version branches. A single regex usually misses at least one — run targeted searches per variant. + - When grep alone is ambiguous (e.g., distinguishing a method from a same-named free function), use Bash with a short inline `python -c "import ast; ..."` (or the project's own parser) to inspect AST nodes. Reach for this only when grep is genuinely insufficient. + - List candidate sites in a TodoWrite item *before* writing the first edit. The list is the completion checklist: don't declare the fix done until each site is either changed or explicitly justified as not needing change. +- Prefer changing internal behavior over changing public surface because repository and downstream callers may depend on the current API. Concretely: + - When an issue says "X should behave like Y", change X's implementation; do not rename X to Y unless the task explicitly requires an API change. + - Avoid renaming or deleting public symbols (top-level functions, methods, types, constants, package-level vars). If a rename is required, preserve compatibility with a thin alias or re-export when practical. + - Before deleting or rewriting a non-trivial public symbol, Grep its name across the repository and inspect callers. Out-of-tree consumers cannot be discovered locally, so minimize unnecessary signature changes. +- When your change introduces a new import — especially any third-party package — update the language's dependency manifest in the same change, or the build will fail before any test runs: `go.mod`/`go.sum` for Go (run `go get ` or `go mod tidy` after editing imports), `Cargo.toml` for Rust, `package.json` plus its lockfile for JS/TS, `pyproject.toml`/`requirements.txt` for Python. In Go this failure is explicit: `no required module provides package ` from `go build`/`go test` means the manifest is missing an entry, not that the package is unavailable. Treat the manifest update as part of the code change, not an optional cleanup. When the task or repository conventions call for a well-known library, prefer using and declaring that dependency instead of hand-rolling an incompatible substitute. - Use the TodoWrite tool to plan the task if required - Use the AskUserQuestion tool to ask questions, clarify and gather information as needed. +- When you are done editing — not between every edit — run one verification pass before declaring the task complete. Stay language-agnostic and let the repository tell you how to verify: + - Discover the repo's own verification entry points before guessing a command: prefer `Makefile`/`justfile` targets (`test`, `check`, `ci`), then package manifests (`package.json` scripts, `pyproject.toml`, `Cargo.toml`, `go.mod`), then README/CI configs. Run the project's own runner, not an assumed default. + - **Scope the verifier to what you changed**, not the entire workspace. `go build ./internal/server/...` not `go build ./...`. `cargo check -p the_crate` not `--workspace`. `tsc --noEmit -p packages/foo` not the root. Whole-workspace builds in large repos can take many minutes and rarely add signal beyond what the scoped build gives you. Only widen if the scoped build passes and you suspect cross-package breakage. + - Run verification in layers, stopping at the most expensive layer you can actually afford: (1) parse / static checks on changed files (e.g. `python -c "import ast; ast.parse(open(p).read())"`, `node --check`, `gofmt -e`, `tsc --noEmit`); (2) build / typecheck via the discovered entry point, scoped as above; (3) targeted tests the task description or the changed code path point at. + - If the task description references specific tests, tracebacks, or reproduction scripts, run those — they were given to you as input. + - Also discover tests for every source file you edited: for each modified `foo.py` (or `foo.rs`, `foo.go`, etc.), find `test_foo.py`, `foo_test.go`, or any test file that imports `foo`. In compiled languages (Go, Rust), run at the package level (e.g. `go test ./that/pkg/...`) rather than by individual file. Run all discovered tests together with task-specified tests in a single verification pass. + - A verification command only counts as successful if it exits successfully and you inspected the relevant summary. Do not conclude success from truncated output, partial logs, or a subset that excludes relevant failing behavior. + - If you touched import statements in a Go project, run `go build` (or `go vet`) on the affected packages and watch specifically for `no required module provides package` — it means `go.mod` was not updated for a new import; fix it with `go get ` or `go mod tidy` before any further verification. + - In compiled languages, static analysis errors (`go vet`, `cargo check`, `tsc --noEmit`) take precedence over test results. A passing `go test` on a subset of packages does not override a failing `go vet` on the changed package. Fix all static analysis errors before treating verification as green. + - Batch your edits before verifying. Do not run a build after each individual file change — make the related set of changes, then verify once. If you find a problem, fix it and verify again. + - Treat any failure output as your next signal, not the end state. Do not declare the task done until the last verification you ran is green or every remaining failure is explicitly justified as unrelated to your change. + - Never pipe test runner output through `| head` or `| tail`. Test runners print tracebacks at the top and the FAILED summary at the bottom — truncating either end hides exactly the diagnostic you need. Test output is captured in full; read it whole. To keep output manageable, use the runner's own verbosity flags instead of piping: `pytest --tb=short` or `--tb=line` emits compact tracebacks across all failures without stopping early. Avoid `-x`/`--exitfirst` when you need to see all failures — it stops after the first one. + - Do not dismiss a failing test as "flaky" or "pre-existing" unless you can reproduce the failure without your changes: run `git stash`, then run the test, then `git stash pop` — always run `git stash pop` as a separate step regardless of the test result, so your changes are never left stranded in the stash. If the test fails on the unmodified codebase, it is pre-existing. If it passes, the failure is yours to fix — reasoning that "it seems unrelated" is not sufficient justification. - Be careful not to introduce security vulnerabilities such as command injection, XSS, SQL injection, and other OWASP top 10 vulnerabilities. If you notice that you wrote insecure code, immediately fix it. - Avoid over-engineering. Only make changes that are directly requested or clearly necessary. Keep solutions simple and focused. - Don't add features, refactor code, or make "improvements" beyond what was asked. A bug fix doesn't need surrounding code cleaned up. A simple feature doesn't need extra configurability. Don't add docstrings, comments, or type annotations to code you didn't change. Only add comments where the logic isn't self-evident. - Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees. Only validate at system boundaries (user input, external APIs). Don't use feature flags or backwards-compatibility shims when you can just change the code. - Don't create helpers, utilities, or abstractions for one-time operations. Don't design for hypothetical future requirements. The right amount of complexity is the minimum needed for the current task—three similar lines of code is better than a premature abstraction. -- Avoid backwards-compatibility hacks like renaming unused `_vars`, re-exporting types, adding `// removed` comments for removed code, etc. If something is unused, delete it completely. +- Do not add speculative backwards-compatibility hacks such as renaming unused `_vars`, re-exporting internal types, or adding `// removed` comments. Delete unused internal code completely. When the task explicitly changes a public symbol, follow the public-surface compatibility rule above instead of applying this internal-cleanup rule blindly. # Tool usage policy - Prefer the most direct tool path that preserves accuracy: use Read, Grep, and Glob for narrow lookups; use Task subagents for broad, multi-area, or independently delegable work. @@ -115,4 +140,4 @@ IMPORTANT: Whenever you mention a file path that the user might want to open, ma {LANGUAGE_PREFERENCE} -{READ_TERMINAL} \ No newline at end of file +{READ_TERMINAL} diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index af68709f13..ed176000e4 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -3692,6 +3692,18 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet "original_user_input".to_string(), original_user_input.clone(), ); + // Constraint revocation changes a user-authored safety boundary. Only + // submissions from an external user surface can authorize that change; + // agent-session follow-ups and scheduled/background work cannot speak + // for the user even though they also flow through a dialog turn. + let revocation_authorized = !matches!( + submission_policy.trigger_source, + DialogTriggerSource::AgentSession | DialogTriggerSource::ScheduledJob + ); + context_vars.insert( + "edit_constraint_revocation_authorized".to_string(), + revocation_authorized.to_string(), + ); // Pass model_id for token usage tracking if let Some(model_id) = &session.config.model_id { @@ -5285,6 +5297,11 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet .await; return Err(error); } + if let Some(source_session_id) = prompt_cache_source_session_id.as_deref() { + self.session_manager + .seed_forked_edit_constraints(source_session_id, &session_id) + .await; + } drop(session_name); drop(session_config); drop(created_by); @@ -6123,6 +6140,9 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet self.session_manager .seed_forked_skill_agent_listing_baselines(parent_session_id, &child_session.session_id) .await; + self.session_manager + .seed_forked_edit_constraints(parent_session_id, &child_session.session_id) + .await; self.session_manager .replace_context_messages(&child_session.session_id, snapshot.messages) diff --git a/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard.rs b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard.rs new file mode 100644 index 0000000000..f52da5ddec --- /dev/null +++ b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard.rs @@ -0,0 +1,1641 @@ +//! Edit constraint guard. +//! +//! Extracts explicit "don't modify X" constraints from user instructions and +//! exposes deterministic checks for file-mutation tools. Extraction evidence is +//! persisted with the session. Optional product diagnostics can record guard +//! decisions and successful direct mutations in a session-scoped JSONL stream. + +use log::warn; +use regex::Regex; +use serde::Deserialize; +use serde_json::{json, Value}; +use sha2::{Digest, Sha256}; +use std::fs::{self, OpenOptions}; +use std::io::Write; +use std::path::Path; +use std::sync::{Mutex, OnceLock}; +use std::time::{Instant, SystemTime, UNIX_EPOCH}; +use uuid::Uuid; + +use crate::agentic::coordination::get_global_coordinator; +use crate::agentic::tools::framework::{ToolUseContext, ValidationResult}; +use crate::infrastructure::ai::get_global_ai_client_factory; +use crate::util::json_extract::extract_json_from_ai_response; +use crate::util::types::Message; + +mod model; +mod shell_targets; + +pub use model::{ + AgentCreatedPathRecord, ConstraintExtractionRecord, ConstraintMatcher, + ConstraintOperationScope, ConstraintRevocation, ConstraintSource, EditConstraintState, + ExtractedConstraint, ExtractionFailure, ExtractionStatus, ModelExtractionStatus, +}; +#[cfg(test)] +use shell_targets::ShellMutationOperation; +use shell_targets::{explicit_bash_mutation_targets, has_unresolved_bash_mutation}; + +pub const EDIT_CONSTRAINT_METADATA_KEY: &str = "editConstraintGuard"; +const EDIT_CONSTRAINT_SCHEMA_VERSION: u32 = 6; +const MAX_PROMPT_CHARS: usize = 8_000; +const MAX_RESPONSE_TELEMETRY_CHARS: usize = 4_000; +const MAX_MODEL_ATTEMPTS: usize = 2; +const MAX_RECURSIVE_INSPECTION_ENTRIES: usize = 100_000; +const TELEMETRY_RELATIVE_PATH: &str = "telemetry/edit-constraint-guard.jsonl"; +const TELEMETRY_ENV: &str = "BITFUN_EDIT_CONSTRAINT_TELEMETRY"; + +const EXTRACTION_SYSTEM_PROMPT: &str = r#"You update the active file-edit prohibitions for a software task. + +You receive the currently active prohibitions and the latest user message. + +- Add a prohibition only when the latest message explicitly forbids modifying + certain files, file types, or categories of files. +- Revoke an active prohibition only when the latest message explicitly cancels, + relaxes, or contradicts it (e.g. "you may modify tests now"). A revocation + MUST copy the exact constraint_id from the active list. Never invent an id. +- An unrelated message does not revoke anything. +- Ignore constraints about anything other than *which files may be edited*. + +For each added prohibition, classify it into exactly ONE matcher kind and one +operation scope: +- "test_files": the prohibition is about test files / testing logic in general +- "path_contains": the prohibition names specific files or keywords (give the literal substrings) +- "path_under_dir": the prohibition names a specific directory (give the directory names) +- "extension": the prohibition is about a specific file type (give the extensions, including the dot) +- "unmatched": you found a prohibition but it doesn't fit any of the above + +Use operation_scope "delete_only" only when the user explicitly prohibits +deleting/removing files, without also prohibiting other edits. Otherwise use +"all". + +Respond with ONLY a fenced ```json code block containing this exact shape: +```json +{ + "additions": [ + {"description": "", "operation_scope": "all", "matcher": {"kind": "test_files"}}, + {"description": "", "operation_scope": "all", "matcher": {"kind": "path_contains", "substrings": ["..."]}}, + {"description": "", "operation_scope": "all", "matcher": {"kind": "path_under_dir", "dirs": ["..."]}}, + {"description": "", "operation_scope": "all", "matcher": {"kind": "extension", "exts": [".ext"]}}, + {"description": "", "operation_scope": "all", "matcher": {"kind": "unmatched"}} + ], + "revocations": [ + {"constraint_id": "", "description": ""} + ] +} +``` +If the latest message changes nothing, return empty `additions` and +`revocations`."#; + +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct ExtractionResponse { + additions: Vec, + revocations: Vec, +} + +pub fn message_sha256(message: &str) -> String { + format!("{:x}", Sha256::digest(message.as_bytes())) +} + +fn timestamp_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_millis() + .try_into() + .unwrap_or(u64::MAX) +} + +fn truncate_for_extraction(message: &str) -> (String, bool) { + let chars = message.chars().collect::>(); + if chars.len() <= MAX_PROMPT_CHARS { + return (message.to_string(), false); + } + + let side = MAX_PROMPT_CHARS / 2; + let mut output = chars[..side].iter().collect::(); + output.push_str("\n...[middle of task omitted for constraint extraction]...\n"); + output.extend(chars[chars.len() - side..].iter()); + (output, true) +} + +fn relevant_sentences(message: &str) -> impl Iterator { + message + .split_inclusive(|character: char| { + matches!(character, '.' | '!' | '?' | '\n' | '。' | '！' | '？') + }) + .map(str::trim) + .filter(|sentence| !sentence.is_empty()) +} + +fn has_prohibition_signal(message: &str) -> bool { + let lower = message.to_lowercase(); + [ + "do not", + "don't", + "dont", + "must not", + "should not", + "never modify", + "never change", + "do not delete", + "don't delete", + "must not delete", + "not allowed to modify", + "avoid modifying", + "keep ", + "leave ", + "off limits", + "read-only", + "must remain unchanged", + "must remain untouched", + "without modifying", + "without changing", + "only modify", + "only change", + "non-test files only", + "不得", + "不能修改", + "不能删除", + "禁止修改", + "禁止删除", + "不要修改", + "不要更改", + "不要删除", + "测试文件保持不变", + "仅修改非测试", + ] + .iter() + .any(|signal| lower.contains(signal)) +} + +fn has_relaxation_signal(message: &str) -> bool { + let lower = message.to_lowercase(); + [ + "may modify", + "can modify", + "may change", + "can change", + "may edit", + "can edit", + "allowed to modify", + "allowed to change", + "allowed to edit", + "feel free to modify", + "feel free to change", + "feel free to edit", + "restriction is lifted", + "restriction no longer applies", + "no longer off limits", + "forget that restriction", + "remove that restriction", + "可以修改", + "可以改", + "允许修改", + "不再禁止", + "取消限制", + "解除限制", + ] + .iter() + .any(|signal| lower.contains(signal)) +} + +fn matches_any_regex( + patterns: &'static OnceLock>, + sources: &[&str], + text: &str, +) -> bool { + patterns + .get_or_init(|| { + sources + .iter() + .map(|source| Regex::new(source).expect("valid edit-constraint regex")) + .collect() + }) + .iter() + .any(|pattern| pattern.is_match(text)) +} + +fn explicit_test_mutation_restriction(text: &str) -> bool { + static PATTERNS: OnceLock> = OnceLock::new(); + matches_any_regex( + &PATTERNS, + &[ + r"\b(?:do not|don't|dont|must not|should not|never|avoid)\s+(?:modify|change|edit|touch|update|alter|write)\s+(?:any\s+)?(?:of\s+)?(?:the\s+)?(?:test files?|tests?|testing logic)\b", + r"\b(?:test files?|tests?|testing logic)\s+(?:must|should|can)\s+not\s+be\s+(?:modified|changed|edited|touched|updated|altered|written)\b", + r"\b(?:keep|leave)\s+(?:the\s+)?(?:test files?|tests?|testing logic)\s+(?:unchanged|untouched|as[- ]is)\b", + r"\b(?:test files?|tests?|testing logic)\s+(?:are|is)\s+(?:off limits|read[- ]only)\b", + r"\b(?:test files?|tests?|testing logic)\s+must\s+remain\s+(?:unchanged|untouched)\b", + r"\bwithout\s+(?:modifying|changing|editing|touching|updating|altering)\s+(?:the\s+)?(?:test files?|tests?|testing logic)\b", + r"\bonly\s+(?:modify|change|edit|touch|update)\s+(?:the\s+)?non[- ]tests?(?:\s+files?)?\b", + r"\b(?:changes?|edits?)\s+(?:must|should)\s+be\s+(?:limited|restricted)\s+to\s+non[- ]tests?(?:\s+files?)?\b", + r"(?:不要|不得|禁止|不能)(?:修改|改动|更改|编辑|触碰|写入)(?:任何)?(?:测试文件|测试|测试逻辑)", + r"(?:测试文件|测试|测试逻辑)(?:不得|禁止|不能)(?:被)?(?:修改|改动|更改|编辑|触碰|写入)", + r"(?:保持|维持)(?:测试文件|测试|测试逻辑)不变", + r"(?:测试文件|测试|测试逻辑)(?:保持|维持)不变", + r"仅(?:修改|改动|更改|编辑)非测试(?:文件)?", + ], + text, + ) +} + +fn explicit_test_delete_restriction(text: &str) -> bool { + static PATTERNS: OnceLock> = OnceLock::new(); + matches_any_regex( + &PATTERNS, + &[ + r"\b(?:do not|don't|dont|must not|should not|never|avoid)\s+(?:delete|remove|unlink)\s+(?:any\s+)?(?:of\s+)?(?:the\s+)?(?:test files?|tests?|testing logic)\b", + r"\b(?:test files?|tests?|testing logic)\s+(?:must|should|can)\s+not\s+be\s+(?:deleted|removed|unlinked)\b", + r"(?:不要|不得|禁止|不能)(?:删除|移除)(?:任何)?(?:测试文件|测试|测试逻辑)", + r"(?:测试文件|测试|测试逻辑)(?:不得|禁止|不能)(?:被)?(?:删除|移除)", + ], + text, + ) +} + +fn deterministic_test_constraint(message: &str) -> Option { + relevant_sentences(message).find_map(|sentence| { + let lower = sentence.to_lowercase(); + let explicitly_relaxes_tests = [ + "may modify tests", + "may modify test files", + "can modify tests", + "can modify test files", + "allowed to modify tests", + "allowed to modify test files", + "tests are no longer off limits", + "test files are no longer off limits", + "test restriction is lifted", + "test-file restriction is lifted", + "可以修改测试", + "可以改测试", + "允许修改测试", + "测试文件可以修改", + "测试可以修改", + "不再禁止修改测试", + ] + .iter() + .any(|signal| lower.contains(signal)); + if explicitly_relaxes_tests { + return None; + } + let prohibits_mutation = explicit_test_mutation_restriction(&lower); + let prohibits_delete = explicit_test_delete_restriction(&lower); + + (prohibits_mutation || prohibits_delete).then(|| { + let source_text = sentence.chars().take(500).collect::(); + ExtractedConstraint { + id: if prohibits_delete && !prohibits_mutation { + "deterministic:test_files:delete_only".to_string() + } else { + "deterministic:test_files".to_string() + }, + description: if prohibits_delete && !prohibits_mutation { + "The task explicitly says not to delete test files or testing logic".to_string() + } else { + "The task explicitly says not to modify test files or testing logic".to_string() + }, + operation_scope: if prohibits_delete && !prohibits_mutation { + ConstraintOperationScope::DeleteOnly + } else { + ConstraintOperationScope::All + }, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Deterministic, + source_text: Some(source_text), + } + }) + }) +} + +fn candidate_paths(context: Option<&ToolUseContext>, file_path: &str) -> Vec { + let mut candidates = vec![file_path.replace('\\', "/")]; + let Some(context) = context else { + return candidates; + }; + let Ok(resolved) = context.resolve_tool_path(file_path) else { + return candidates; + }; + for path in [ + resolved.logical_path.clone(), + resolved.resolved_path.clone(), + ] { + let normalized = path.replace('\\', "/"); + if !candidates.contains(&normalized) { + candidates.push(normalized); + } + } + if !resolved.uses_remote_workspace_backend() { + let resolved_path = Path::new(&resolved.resolved_path); + let canonical = fs::canonicalize(resolved_path).ok().or_else(|| { + let parent = resolved_path.parent()?; + let file_name = resolved_path.file_name()?; + fs::canonicalize(parent) + .ok() + .map(|parent| parent.join(file_name)) + }); + if let Some(canonical) = canonical { + let normalized = canonical.to_string_lossy().replace('\\', "/"); + if !candidates.contains(&normalized) { + candidates.push(normalized); + } + } + } + candidates +} + +fn normalize_model_constraints(constraints: &mut [ExtractedConstraint], message_sha256: &str) { + let message_prefix = message_sha256.chars().take(12).collect::(); + for (index, constraint) in constraints.iter_mut().enumerate() { + constraint.id = format!("model:{message_prefix}:{index}"); + constraint.source = ConstraintSource::Model; + } +} + +fn validated_revocation_ids( + revocations: &[ConstraintRevocation], + active_constraints: &[ExtractedConstraint], + revocation_authorized: bool, +) -> (Vec, Vec) { + if !revocation_authorized { + return (Vec::new(), Vec::new()); + } + + let mut revoked = Vec::new(); + let mut unmatched = Vec::new(); + for revocation in revocations { + let constraint_id = revocation.constraint_id.trim(); + if active_constraints + .iter() + .any(|constraint| constraint.id == constraint_id) + { + if !revoked.iter().any(|existing| existing == constraint_id) { + revoked.push(constraint_id.to_string()); + } + } else if !unmatched.iter().any(|existing| existing == constraint_id) { + unmatched.push(constraint_id.to_string()); + } + } + (revoked, unmatched) +} + +fn response_excerpt(response: &str) -> String { + response + .chars() + .take(MAX_RESPONSE_TELEMETRY_CHARS) + .collect() +} + +/// Extract constraints from one user instruction when there is no prior +/// session state. Kept as the simple entry point for callers and tests that do +/// not need revocation semantics. +pub async fn extract_constraints(user_message: &str) -> ConstraintExtractionRecord { + extract_constraints_with_active(user_message, &[]).await +} + +/// Extract additions and explicit revocations from one user instruction. +/// Explicit test-file prohibitions are recognized deterministically before the +/// model call, while revocations are accepted only from a successfully parsed +/// fast-model response that references an active constraint id. +pub async fn extract_constraints_with_active( + user_message: &str, + active_constraints: &[ExtractedConstraint], +) -> ConstraintExtractionRecord { + extract_constraints_with_active_and_revocation_authorization( + user_message, + active_constraints, + true, + ) + .await +} + +/// Extract additions and explicit revocations from one instruction, applying +/// revocations only when the caller has established that the text came from a +/// real user submission. Internal follow-ups may add protections but must +/// never relax a protection on the user's behalf. +pub async fn extract_constraints_with_active_and_revocation_authorization( + user_message: &str, + active_constraints: &[ExtractedConstraint], + revocation_authorized: bool, +) -> ConstraintExtractionRecord { + let started_at = Instant::now(); + let input_chars = user_message.chars().count(); + let message_sha256 = message_sha256(user_message); + let active_constraint_ids = active_constraints + .iter() + .map(|constraint| constraint.id.clone()) + .collect::>(); + if user_message.trim().is_empty() { + return ConstraintExtractionRecord { + message_sha256, + dialog_turn_id: None, + status: ExtractionStatus::NoConstraints, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 0, + active_constraint_ids, + revocation_authorized, + model_status: ModelExtractionStatus::NotRun, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars, + prompt_chars: 0, + input_truncated: false, + latency_ms: 0, + extracted_at_ms: timestamp_ms(), + failure: None, + response_excerpt: None, + }; + } + + let mut constraints = deterministic_test_constraint(user_message) + .into_iter() + .collect::>(); + let deterministic_constraint_count = constraints.len(); + let (truncated, input_truncated) = truncate_for_extraction(user_message); + let prompt_chars = truncated.chars().count(); + + // Irrelevant follow-ups stay on the local fast path even when constraints + // are active. Only messages that may add or relax a file-edit boundary use + // the model-backed classifier. + if !has_prohibition_signal(user_message) && !has_relaxation_signal(user_message) { + return ConstraintExtractionRecord { + message_sha256, + dialog_turn_id: None, + status: if constraints.is_empty() { + ExtractionStatus::NoConstraints + } else { + ExtractionStatus::Extracted + }, + constraints, + deterministic_constraint_count, + model_attempts: 0, + active_constraint_ids, + revocation_authorized, + model_status: ModelExtractionStatus::NotRun, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars, + prompt_chars, + input_truncated, + latency_ms: started_at + .elapsed() + .as_millis() + .try_into() + .unwrap_or(u64::MAX), + extracted_at_ms: timestamp_ms(), + failure: None, + response_excerpt: None, + }; + } + + let factory = match get_global_ai_client_factory().await { + Ok(factory) => factory, + Err(error) => { + return extraction_with_failure( + started_at, + message_sha256, + constraints, + deterministic_constraint_count, + 0, + active_constraint_ids, + revocation_authorized, + input_chars, + prompt_chars, + input_truncated, + "client_factory", + error.to_string(), + None, + ); + } + }; + let client = match factory.get_client_resolved("fast").await { + Ok(client) => client, + Err(error) => { + return extraction_with_failure( + started_at, + message_sha256, + constraints, + deterministic_constraint_count, + 0, + active_constraint_ids, + revocation_authorized, + input_chars, + prompt_chars, + input_truncated, + "client_resolution", + error.to_string(), + None, + ); + } + }; + + let active_constraints_json = + serde_json::to_string(active_constraints).unwrap_or_else(|_| "[]".to_string()); + let task_message = format!( + "\n{active_constraints_json}\n\n\ + \n{truncated}\n" + ); + let mut failure = None; + let mut last_response_excerpt = None; + let mut model_attempts = 0; + let mut model_status = ModelExtractionStatus::Failed; + let mut model_constraints = Vec::new(); + let mut model_revocations = Vec::new(); + let mut revoked_constraint_ids = Vec::new(); + let mut unmatched_revocation_ids = Vec::new(); + + for attempt in 1..=MAX_MODEL_ATTEMPTS { + model_attempts = attempt; + let response = match client + .send_message( + vec![ + Message::system(EXTRACTION_SYSTEM_PROMPT.to_string()), + Message::user(task_message.clone()), + ], + None, + ) + .await + { + Ok(response) => response, + Err(error) => { + failure = Some(ExtractionFailure { + stage: "model_request".to_string(), + reason: error.to_string(), + }); + continue; + } + }; + + last_response_excerpt = Some(response_excerpt(&response.text)); + if response.text.trim().is_empty() { + failure = Some(ExtractionFailure { + stage: "empty_response".to_string(), + reason: "The extraction model returned no text".to_string(), + }); + continue; + } + let Some(json_string) = extract_json_from_ai_response(&response.text) else { + failure = Some(ExtractionFailure { + stage: "json_extraction".to_string(), + reason: "No JSON object was found in the extraction response".to_string(), + }); + continue; + }; + match serde_json::from_str::(&json_string) { + Ok(mut parsed) => { + normalize_model_constraints(&mut parsed.additions, &message_sha256); + model_constraints = parsed.additions.clone(); + model_revocations = parsed.revocations; + + (revoked_constraint_ids, unmatched_revocation_ids) = validated_revocation_ids( + &model_revocations, + active_constraints, + revocation_authorized, + ); + + for constraint in &model_constraints { + if !constraints + .iter() + .any(|existing| existing.matcher == constraint.matcher) + { + constraints.push(constraint.clone()); + } + } + model_status = ModelExtractionStatus::Parsed; + failure = None; + break; + } + Err(error) => { + failure = Some(ExtractionFailure { + stage: "schema_validation".to_string(), + reason: error.to_string(), + }); + } + } + } + + let status = if !constraints.is_empty() || !revoked_constraint_ids.is_empty() { + ExtractionStatus::Extracted + } else if failure.is_some() { + ExtractionStatus::Failed + } else { + ExtractionStatus::NoConstraints + }; + + ConstraintExtractionRecord { + message_sha256, + dialog_turn_id: None, + status, + constraints, + deterministic_constraint_count, + model_attempts, + active_constraint_ids, + revocation_authorized, + model_status, + model_constraints, + model_revocations, + revoked_constraint_ids, + unmatched_revocation_ids, + input_chars, + prompt_chars, + input_truncated, + latency_ms: started_at + .elapsed() + .as_millis() + .try_into() + .unwrap_or(u64::MAX), + extracted_at_ms: timestamp_ms(), + failure, + response_excerpt: last_response_excerpt, + } +} + +/// Returns whether an extraction contains state or diagnostic evidence worth +/// persisting. The common no-signal path deliberately leaves session metadata +/// untouched so ordinary turns do not grow the guard history. +pub fn extraction_requires_session_state(record: &ConstraintExtractionRecord) -> bool { + record.status != ExtractionStatus::NoConstraints || record.model_attempts > 0 +} + +#[allow(clippy::too_many_arguments)] +fn extraction_with_failure( + started_at: Instant, + message_sha256: String, + constraints: Vec, + deterministic_constraint_count: usize, + model_attempts: usize, + active_constraint_ids: Vec, + revocation_authorized: bool, + input_chars: usize, + prompt_chars: usize, + input_truncated: bool, + stage: &str, + reason: String, + response_excerpt: Option, +) -> ConstraintExtractionRecord { + ConstraintExtractionRecord { + message_sha256, + dialog_turn_id: None, + status: if constraints.is_empty() { + ExtractionStatus::Failed + } else { + ExtractionStatus::Extracted + }, + constraints, + deterministic_constraint_count, + model_attempts, + active_constraint_ids, + revocation_authorized, + model_status: ModelExtractionStatus::Failed, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars, + prompt_chars, + input_truncated, + latency_ms: started_at + .elapsed() + .as_millis() + .try_into() + .unwrap_or(u64::MAX), + extracted_at_ms: timestamp_ms(), + failure: Some(ExtractionFailure { + stage: stage.to_string(), + reason, + }), + response_excerpt, + } +} + +pub fn find_violation<'a>( + constraints: &'a [ExtractedConstraint], + file_path: &str, +) -> Option<&'a ExtractedConstraint> { + find_violation_for_operation(constraints, file_path, "write") +} + +fn find_violation_for_operation<'a>( + constraints: &'a [ExtractedConstraint], + file_path: &str, + operation: &str, +) -> Option<&'a ExtractedConstraint> { + constraints.iter().find(|constraint| { + constraint.operation_scope.applies_to(operation) && constraint.matcher.matches(file_path) + }) +} + +pub fn violation_message(file_path: &str, constraint: &ExtractedConstraint) -> String { + format!( + "This file (`{file_path}`) matches a constraint stated in the task: \"{}\". \ + This edit was not applied.\n\n\ + Editing a file you were told not to touch usually means your own implementation \ + doesn't match what's expected — not that the file is wrong. Reconsider your \ + source-code approach instead of adjusting this file.", + constraint.description + ) +} + +fn telemetry_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) +} + +fn telemetry_setting_enabled(value: Option<&str>) -> bool { + value.is_some_and(|value| { + matches!( + value.trim().to_ascii_lowercase().as_str(), + "1" | "true" | "yes" | "on" + ) + }) +} + +fn edit_constraint_telemetry_enabled() -> bool { + static ENABLED: OnceLock = OnceLock::new(); + *ENABLED.get_or_init(|| telemetry_setting_enabled(std::env::var(TELEMETRY_ENV).ok().as_deref())) +} + +fn try_append_tool_telemetry( + context: &ToolUseContext, + event: &Value, +) -> Result { + let Some(session_id) = context.session_id.as_deref() else { + return Err("tool context has no session id".to_string()); + }; + let session_dir = context + .current_workspace_session_dir(session_id) + .map_err(|error| format!("failed to resolve session directory: {error}"))?; + let path = session_dir.join(TELEMETRY_RELATIVE_PATH); + append_jsonl(&path, event)?; + Ok(path) +} + +fn append_jsonl(path: &Path, event: &Value) -> Result<(), String> { + let parent = path + .parent() + .ok_or_else(|| "telemetry path has no parent".to_string())?; + + let _guard = telemetry_lock() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + fs::create_dir_all(parent) + .map_err(|error| format!("failed to create telemetry directory: {error}"))?; + let mut file = OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .map_err(|error| format!("failed to open telemetry stream: {error}"))?; + serde_json::to_writer(&mut file, event) + .and_then(|_| file.write_all(b"\n").map_err(serde_json::Error::io)) + .map_err(|error| format!("failed to append telemetry event: {error}"))?; + Ok(()) +} + +fn append_tool_telemetry(context: &ToolUseContext, event: &Value) { + if !edit_constraint_telemetry_enabled() { + return; + } + if let Err(error) = try_append_tool_telemetry(context, event) { + warn!("Failed to append edit constraint telemetry event: {error}"); + } +} + +fn resolved_path(context: &ToolUseContext, file_path: &str) -> Option { + context + .resolve_tool_path(file_path) + .ok() + .map(|resolved| resolved.resolved_path) +} + +fn decision_result( + context: Option<&ToolUseContext>, + tool_name: &str, + operation: &str, + file_path: &str, + decision: &str, + force_requested: bool, + state: Option<&EditConstraintState>, + violation: Option<&ExtractedConstraint>, + message: Option, + error_code: Option, +) -> Option { + let telemetry_enabled = edit_constraint_telemetry_enabled(); + if message.is_none() && !telemetry_enabled { + return None; + } + let decision_id = Uuid::new_v4().to_string(); + if telemetry_enabled { + if let Some(context) = context { + append_tool_telemetry( + context, + &json!({ + "event": "guard_decision", + "schema_version": EDIT_CONSTRAINT_SCHEMA_VERSION, + "decision_id": decision_id, + "timestamp_ms": timestamp_ms(), + "session_id": context.session_id, + "dialog_turn_id": context.dialog_turn_id, + "tool_call_id": context.tool_call_id, + "agent_type": context.agent_type, + "tool_name": tool_name, + "operation": operation, + "requested_path": file_path, + "resolved_path": resolved_path(context, file_path), + "workspace_kind": if context.is_remote() { "remote" } else { "local" }, + "decision": decision, + "force_requested": force_requested, + "extraction_status": state.and_then(EditConstraintState::latest_status), + "constraint": violation, + }), + ); + } + } + + message.map(|message| ValidationResult { + result: false, + message: Some(message), + error_code, + meta: Some(json!({ + "failure_kind": "edit_constraint_guard", + "guard_decision_id": decision_id, + "guard_decision": decision, + "constraint_id": violation.map(|constraint| constraint.id.as_str()), + "protected_path": file_path, + "force_requested": force_requested, + })), + }) +} + +/// Deterministic guard check shared by direct file mutation tools. +/// +/// `force` is no longer a model-controlled escape hatch. A stale caller that +/// still sends it is rejected and recorded explicitly. +pub fn check( + context: Option<&ToolUseContext>, + tool_name: &str, + operation: &str, + file_path: &str, + force_requested: bool, +) -> Option { + let state = context + .and_then(|value| value.session_id.as_deref()) + .and_then(|session_id| { + get_global_coordinator()? + .get_session_manager() + .edit_constraint_state(session_id) + }); + + if !force_requested + && !edit_constraint_telemetry_enabled() + && state + .as_ref() + .map_or(true, |state| !state.has_enforceable_constraints()) + { + return None; + } + if !force_requested + && state + .as_ref() + .map_or(true, |state| !state.has_enforceable_constraints()) + { + if edit_constraint_telemetry_enabled() { + decision_result( + context, + tool_name, + operation, + file_path, + "allow_no_active_constraint", + false, + state.as_ref(), + None, + None, + None, + ); + } + return None; + } + + if force_requested { + return decision_result( + context, + tool_name, + operation, + file_path, + "force_denied", + true, + state.as_ref(), + None, + Some( + "`force` cannot override constraints stated by the user. Reconsider the source-code approach without modifying the protected file." + .to_string(), + ), + Some(403), + ); + } + + let paths = candidate_paths(context, file_path); + let violation = state.as_ref().and_then(|state| { + paths + .iter() + .find_map(|path| find_violation_for_operation(&state.constraints, path, operation)) + }); + if let Some(violation) = violation { + return decision_result( + context, + tool_name, + operation, + file_path, + "deny", + false, + state.as_ref(), + Some(violation), + Some(violation_message(file_path, violation)), + Some(403), + ); + } + + let decision = match state.as_ref().and_then(EditConstraintState::latest_status) { + Some(ExtractionStatus::Failed) | None => "allow_extraction_unavailable", + _ => "allow", + }; + decision_result( + context, + tool_name, + operation, + file_path, + decision, + false, + state.as_ref(), + None, + None, + None, + ); + None +} + +async fn write_target_exists(context: &ToolUseContext, file_path: &str) -> Option { + let resolved = context.resolve_tool_path(file_path).ok()?; + if resolved.uses_remote_workspace_backend() { + let fs = context.ws_fs()?; + // Treat a failed remote inspection as existing so an unavailable + // filesystem cannot become a way to overwrite a protected test file. + return Some(fs.exists(&resolved.resolved_path).await.unwrap_or(true)); + } + + Some(Path::new(&resolved.resolved_path).exists()) +} + +fn has_only_relaxable_test_file_violations( + state: &EditConstraintState, + paths: &[String], + operation: &str, +) -> bool { + let violations = paths + .iter() + .flat_map(|path| { + state.constraints.iter().filter(move |constraint| { + constraint.operation_scope.applies_to(operation) && constraint.matcher.matches(path) + }) + }) + .collect::>(); + + !violations.is_empty() + && violations.iter().all(|constraint| { + matches!(&constraint.matcher, ConstraintMatcher::TestFiles) + && constraint.operation_scope == ConstraintOperationScope::All + }) +} + +fn can_mutate_agent_created_test_file( + state: Option<&EditConstraintState>, + paths: &[String], + operation: &str, + newly_created: bool, +) -> bool { + state.is_some_and(|state| { + (newly_created || state.is_agent_created_path(paths)) + && has_only_relaxable_test_file_violations(state, paths, operation) + }) +} + +/// Guard a Write operation while allowing a newly-created test helper. +/// +/// A task instruction not to modify tests protects the repository's existing +/// tests from being adjusted to satisfy the task. It does not prohibit an +/// agent from creating an untracked repro or verification file. Other +/// constraint kinds retain their strict create-or-modify semantics. +pub async fn check_write( + context: Option<&ToolUseContext>, + tool_name: &str, + operation: &str, + file_path: &str, + force_requested: bool, +) -> Option { + let state = context + .and_then(|value| value.session_id.as_deref()) + .and_then(|session_id| { + get_global_coordinator()? + .get_session_manager() + .edit_constraint_state(session_id) + }); + + if !force_requested + && state + .as_ref() + .map_or(true, |state| !state.tracks_agent_created_test_paths()) + { + return check(context, tool_name, operation, file_path, false); + } + + let is_new_file = if force_requested { + false + } else if let Some(context) = context { + write_target_exists(context, file_path).await == Some(false) + } else { + false + }; + let paths = candidate_paths(context, file_path); + + if can_mutate_agent_created_test_file(state.as_ref(), &paths, operation, is_new_file) { + decision_result( + context, + tool_name, + operation, + file_path, + if is_new_file { + "allow_new_test_file" + } else { + "allow_agent_created_test_file" + }, + false, + state.as_ref(), + None, + None, + None, + ); + return None; + } + + check(context, tool_name, operation, file_path, force_requested) +} + +/// Guard an Edit operation while preserving the session provenance of helper +/// tests the agent created itself. +pub fn check_edit( + context: Option<&ToolUseContext>, + tool_name: &str, + operation: &str, + file_path: &str, + force_requested: bool, +) -> Option { + let state = context + .and_then(|value| value.session_id.as_deref()) + .and_then(|session_id| { + get_global_coordinator()? + .get_session_manager() + .edit_constraint_state(session_id) + }); + if !force_requested + && !edit_constraint_telemetry_enabled() + && state + .as_ref() + .map_or(true, |state| !state.has_enforceable_constraints()) + { + return None; + } + if !force_requested + && state + .as_ref() + .map_or(true, |state| !state.tracks_agent_created_test_paths()) + { + return check(context, tool_name, operation, file_path, false); + } + let paths = candidate_paths(context, file_path); + if !force_requested + && can_mutate_agent_created_test_file(state.as_ref(), &paths, operation, false) + { + decision_result( + context, + tool_name, + operation, + file_path, + "allow_agent_created_test_file", + false, + state.as_ref(), + None, + None, + None, + ); + return None; + } + check(context, tool_name, operation, file_path, force_requested) +} + +/// Guard a Delete operation while allowing the agent to clean up a test file +/// it created in this session. A user-authored delete-only prohibition remains +/// strict and is never relaxed by file provenance. +pub fn check_delete( + context: Option<&ToolUseContext>, + tool_name: &str, + operation: &str, + file_path: &str, + force_requested: bool, +) -> Option { + check_edit(context, tool_name, operation, file_path, force_requested) +} + +/// Preflight file targets in terminal commands. Explicit targets are checked +/// directly. When constraints are active, high-risk commands whose targets +/// remain dynamic or implicit are rejected before execution; ordinary build, +/// test, and read-only commands retain the normal shell path. +pub fn check_bash_command(context: &ToolUseContext, command: &str) -> Option { + let has_active_constraints = context.session_id.as_deref().is_some_and(|session_id| { + get_global_coordinator() + .and_then(|coordinator| { + coordinator + .get_session_manager() + .edit_constraint_state(session_id) + }) + .is_some_and(|state| state.has_enforceable_constraints()) + }); + if !has_active_constraints { + return None; + } + let targets = explicit_bash_mutation_targets(command); + for target in &targets { + if let Some(rejection) = check( + Some(context), + "Bash", + target.operation.guard_operation(), + &target.path, + false, + ) { + return Some(rejection); + } + } + if has_unresolved_bash_mutation(command, &targets) { + let state = context.session_id.as_deref().and_then(|session_id| { + get_global_coordinator()? + .get_session_manager() + .edit_constraint_state(session_id) + }); + if let Some((state, constraint)) = state.and_then(|state| { + let constraint = state + .constraints + .iter() + .find(|constraint| constraint.matcher.enforceable())? + .clone(); + Some((state, constraint)) + }) { + return decision_result( + Some(context), + "Bash", + "unresolved_shell_mutation", + "", + "deny_unresolved_target", + false, + Some(&state), + Some(&constraint), + Some( + "This command may modify files through a dynamic or implicit target while an edit constraint is active. Use a direct file tool or a command with explicit literal paths so the protected scope can be checked before execution." + .to_string(), + ), + Some(403), + ); + } + } + None +} + +pub fn check_git_command( + context: &ToolUseContext, + operation: &str, + arguments: &str, +) -> Option { + let command = if arguments.trim().is_empty() { + format!("git {operation}") + } else { + format!("git {operation} {}", arguments.trim()) + }; + check_bash_command(context, &command) +} + +/// Checks the target and every non-symlink descendant before recursive delete. +/// Inspection failures are fail-closed only when an enforceable constraint is +/// active, because otherwise there is no protected path to discover. +pub async fn check_recursive_delete( + context: Option<&ToolUseContext>, + root_path: &str, + force_requested: bool, +) -> Option { + if let Some(rejection) = check_delete( + context, + "Delete", + "recursive_delete", + root_path, + force_requested, + ) { + return Some(rejection); + } + let context = context?; + let session_id = context.session_id.as_deref()?; + let state = get_global_coordinator()? + .get_session_manager() + .edit_constraint_state(session_id)?; + if !state.has_enforceable_constraints() { + return None; + } + + let resolved = match context.resolve_tool_path(root_path) { + Ok(resolved) => resolved, + Err(_) => return None, + }; + let Some(workspace_fs) = context.ws_fs() else { + if resolved.uses_remote_workspace_backend() { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(&state), + None, + Some( + "Recursive delete was not applied because the remote workspace filesystem is unavailable" + .to_string(), + ), + Some(503), + ); + } + return check_local_recursive_delete(context, root_path, &resolved.resolved_path, &state); + }; + match workspace_fs.is_dir(&resolved.resolved_path).await { + Ok(true) => {} + Ok(false) => return None, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(&state), + None, + Some(format!( + "Recursive delete was not applied because the target type could not be inspected: {error}" + )), + Some(503), + ); + } + } + + let mut pending = vec![resolved.resolved_path]; + let mut inspected = 0usize; + while let Some(directory) = pending.pop() { + let entries = match workspace_fs.read_dir(&directory).await { + Ok(entries) => entries, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(&state), + None, + Some(format!( + "Recursive delete was not applied because protected descendants could not be inspected: {error}" + )), + Some(503), + ); + } + }; + + for entry in entries { + if entry.is_symlink { + continue; + } + inspected += 1; + if inspected > MAX_RECURSIVE_INSPECTION_ENTRIES { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_limit", + false, + Some(&state), + None, + Some("Recursive delete was not applied because protected-path inspection exceeded its safety limit".to_string()), + Some(413), + ); + } + let entry_paths = vec![entry.path.clone()]; + if !can_mutate_agent_created_test_file( + Some(&state), + &entry_paths, + "recursive_delete", + false, + ) { + if let Some(violation) = find_violation_for_operation( + &state.constraints, + &entry.path, + "recursive_delete", + ) { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + &entry.path, + "deny", + false, + Some(&state), + Some(violation), + Some(violation_message(&entry.path, violation)), + Some(403), + ); + } + } + if entry.is_dir { + pending.push(entry.path); + } + } + } + None +} + +fn check_local_recursive_delete( + context: &ToolUseContext, + root_path: &str, + resolved_root: &str, + state: &EditConstraintState, +) -> Option { + let root = Path::new(resolved_root); + let metadata = match fs::symlink_metadata(root) { + Ok(metadata) => metadata, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(state), + None, + Some(format!( + "Recursive delete was not applied because the target could not be inspected: {error}" + )), + Some(503), + ); + } + }; + if !metadata.is_dir() { + return None; + } + + let mut pending = vec![root.to_path_buf()]; + let mut inspected = 0usize; + while let Some(directory) = pending.pop() { + let entries = match fs::read_dir(&directory) { + Ok(entries) => entries, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(state), + None, + Some(format!( + "Recursive delete was not applied because protected descendants could not be inspected: {error}" + )), + Some(503), + ); + } + }; + for entry in entries { + let entry = match entry { + Ok(entry) => entry, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(state), + None, + Some(format!( + "Recursive delete was not applied because a descendant could not be inspected: {error}" + )), + Some(503), + ); + } + }; + let path = entry.path(); + let entry_metadata = match fs::symlink_metadata(&path) { + Ok(metadata) => metadata, + Err(error) => { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_failed", + false, + Some(state), + None, + Some(format!( + "Recursive delete was not applied because a descendant type could not be inspected: {error}" + )), + Some(503), + ); + } + }; + if entry_metadata.file_type().is_symlink() { + continue; + } + inspected += 1; + if inspected > MAX_RECURSIVE_INSPECTION_ENTRIES { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + root_path, + "deny_inspection_limit", + false, + Some(state), + None, + Some("Recursive delete was not applied because protected-path inspection exceeded its safety limit".to_string()), + Some(413), + ); + } + let path_string = path.to_string_lossy().to_string(); + let entry_paths = vec![path_string.clone()]; + if !can_mutate_agent_created_test_file( + Some(state), + &entry_paths, + "recursive_delete", + false, + ) { + if let Some(violation) = find_violation_for_operation( + &state.constraints, + &path_string, + "recursive_delete", + ) { + return decision_result( + Some(context), + "Delete", + "recursive_delete", + &path_string, + "deny", + false, + Some(state), + Some(violation), + Some(violation_message(&path_string, violation)), + Some(403), + ); + } + } + if entry_metadata.is_dir() { + pending.push(path); + } + } + } + None +} + +/// Records that a path was first created through a direct agent file tool. +/// This provenance is persisted with the session so later Write/Edit/Delete +/// calls can clean up the helper without being confused for repository tests. +pub async fn remember_agent_created_file(context: &ToolUseContext, file_path: &str) { + let Some(session_id) = context.session_id.as_deref() else { + return; + }; + let coordinator = get_global_coordinator(); + let should_track = coordinator.as_ref().is_some_and(|coordinator| { + coordinator + .get_session_manager() + .edit_constraint_state(session_id) + .is_some_and(|state| state.tracks_agent_created_test_paths()) + }); + let telemetry_enabled = edit_constraint_telemetry_enabled(); + if !should_track && !telemetry_enabled { + return; + } + let paths = candidate_paths(Some(context), file_path); + if should_track { + let coordinator = coordinator.expect("coordinator checked above"); + coordinator + .get_session_manager() + .remember_edit_constraint_agent_created_paths( + session_id, + paths.clone(), + context.dialog_turn_id.as_deref().unwrap_or_default(), + ) + .await; + } + if telemetry_enabled { + append_tool_telemetry( + context, + &json!({ + "event": "session_file_origin", + "schema_version": EDIT_CONSTRAINT_SCHEMA_VERSION, + "timestamp_ms": timestamp_ms(), + "session_id": context.session_id, + "dialog_turn_id": context.dialog_turn_id, + "tool_call_id": context.tool_call_id, + "requested_path": file_path, + "resolved_path": resolved_path(context, file_path), + "origin": "agent_created", + }), + ); + } +} + +/// Clears agent-created provenance after a successful direct delete. +pub async fn forget_agent_created_file(context: &ToolUseContext, file_path: &str) { + let Some(session_id) = context.session_id.as_deref() else { + return; + }; + let coordinator = get_global_coordinator(); + let should_forget = coordinator.as_ref().is_some_and(|coordinator| { + coordinator + .get_session_manager() + .edit_constraint_state(session_id) + .is_some_and(|state| state.has_agent_created_paths()) + }); + let telemetry_enabled = edit_constraint_telemetry_enabled(); + if !should_forget && !telemetry_enabled { + return; + } + let paths = candidate_paths(Some(context), file_path); + if should_forget { + let coordinator = coordinator.expect("coordinator checked above"); + coordinator + .get_session_manager() + .forget_edit_constraint_agent_created_paths_under(session_id, paths.clone()) + .await; + } + if telemetry_enabled { + append_tool_telemetry( + context, + &json!({ + "event": "session_file_origin", + "schema_version": EDIT_CONSTRAINT_SCHEMA_VERSION, + "timestamp_ms": timestamp_ms(), + "session_id": context.session_id, + "dialog_turn_id": context.dialog_turn_id, + "tool_call_id": context.tool_call_id, + "requested_path": file_path, + "resolved_path": resolved_path(context, file_path), + "origin": "removed", + }), + ); + } +} + +/// Records a successful direct mutation when product diagnostics are enabled. +pub fn record_mutation_applied( + context: &ToolUseContext, + tool_name: &str, + operation: &str, + file_path: &str, +) { + if !edit_constraint_telemetry_enabled() { + return; + } + append_tool_telemetry( + context, + &json!({ + "event": "mutation_applied", + "schema_version": EDIT_CONSTRAINT_SCHEMA_VERSION, + "mutation_id": Uuid::new_v4().to_string(), + "timestamp_ms": timestamp_ms(), + "session_id": context.session_id, + "dialog_turn_id": context.dialog_turn_id, + "tool_call_id": context.tool_call_id, + "agent_type": context.agent_type, + "tool_name": tool_name, + "operation": operation, + "requested_path": file_path, + "resolved_path": resolved_path(context, file_path), + "workspace_kind": if context.is_remote() { "remote" } else { "local" }, + }), + ); +} + +#[cfg(test)] +mod tests; diff --git a/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/model.rs b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/model.rs new file mode 100644 index 0000000000..384a12517a --- /dev/null +++ b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/model.rs @@ -0,0 +1,373 @@ +use super::EDIT_CONSTRAINT_SCHEMA_VERSION; +use serde::{Deserialize, Serialize}; +use std::collections::HashSet; +use std::path::Path; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum ConstraintSource { + Deterministic, + Model, + #[default] + Legacy, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ExtractedConstraint { + #[serde(default)] + pub id: String, + /// Human-readable paraphrase shown to the agent in the rejection message. + pub description: String, + #[serde(default)] + pub operation_scope: ConstraintOperationScope, + pub matcher: ConstraintMatcher, + #[serde(default)] + pub source: ConstraintSource, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub source_text: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum ConstraintOperationScope { + #[default] + All, + DeleteOnly, +} + +impl ConstraintOperationScope { + pub(super) fn applies_to(self, operation: &str) -> bool { + match self { + Self::All => true, + Self::DeleteOnly => matches!(operation, "delete" | "recursive_delete"), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub enum ConstraintMatcher { + TestFiles, + PathContains { + substrings: Vec, + }, + PathUnderDir { + dirs: Vec, + }, + Extension { + exts: Vec, + }, + /// Recorded for analysis but never enforced. + Unmatched, +} + +impl ConstraintMatcher { + pub fn matches(&self, file_path: &str) -> bool { + let normalized = file_path.replace('\\', "/"); + match self { + ConstraintMatcher::TestFiles => is_test_file(&normalized), + ConstraintMatcher::PathContains { substrings } => substrings + .iter() + .any(|value| !value.is_empty() && normalized.contains(value.as_str())), + ConstraintMatcher::PathUnderDir { dirs } => dirs.iter().any(|dir| { + let dir = dir.trim_matches('/'); + !dir.is_empty() + && (normalized == dir + || normalized.starts_with(&format!("{dir}/")) + || normalized.contains(&format!("/{dir}/"))) + }), + ConstraintMatcher::Extension { exts } => exts + .iter() + .any(|extension| !extension.is_empty() && normalized.ends_with(extension)), + ConstraintMatcher::Unmatched => false, + } + } + + pub(super) fn enforceable(&self) -> bool { + !matches!(self, ConstraintMatcher::Unmatched) + } +} + +fn is_test_file(path: &str) -> bool { + let normalized = path.replace('\\', "/"); + let lowercase = normalized.to_lowercase(); + let name = Path::new(&lowercase) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or_default(); + let stem = Path::new(name) + .file_stem() + .and_then(|value| value.to_str()) + .unwrap_or_default(); + + stem.starts_with("test_") + || stem.starts_with("test-") + || stem.ends_with("_test") + || stem.ends_with("-test") + || stem.ends_with("_tests") + || stem.ends_with("_spec") + || stem.ends_with("-spec") + || name.contains(".test.") + || name.contains(".spec.") + || lowercase + .split('/') + .any(|segment| matches!(segment, "tests" | "test" | "__tests__" | "spec" | "specs")) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ExtractionStatus { + Extracted, + NoConstraints, + Failed, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum ModelExtractionStatus { + #[default] + NotRun, + Parsed, + Failed, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ConstraintRevocation { + pub constraint_id: String, + pub description: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ExtractionFailure { + pub stage: String, + pub reason: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ConstraintExtractionRecord { + pub message_sha256: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub dialog_turn_id: Option, + pub status: ExtractionStatus, + pub constraints: Vec, + pub deterministic_constraint_count: usize, + pub model_attempts: usize, + /// Snapshot of the active ids shown to fast for this extraction. + #[serde(default)] + pub active_constraint_ids: Vec, + /// Whether this turn originated at a real user submission and can therefore + /// relax an existing user-authored edit constraint. + #[serde(default)] + pub revocation_authorized: bool, + #[serde(default)] + pub model_status: ModelExtractionStatus, + /// Exact additions parsed from the fast model, before deterministic + /// additions are merged in. + #[serde(default)] + pub model_constraints: Vec, + /// Exact revocation requests parsed from the fast model. Invalid ids remain + /// here for telemetry but are never applied. + #[serde(default)] + pub model_revocations: Vec, + /// Revocations validated against the active constraint ids supplied to the + /// model. Only these ids are applied to session state. + #[serde(default)] + pub revoked_constraint_ids: Vec, + #[serde(default)] + pub unmatched_revocation_ids: Vec, + pub input_chars: usize, + pub prompt_chars: usize, + pub input_truncated: bool, + pub latency_ms: u64, + pub extracted_at_ms: u64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub failure: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub response_excerpt: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct EditConstraintState { + pub schema_version: u32, + /// Active constraints inherited at fork time. Child rollbacks always begin + /// from this baseline before replaying surviving child turns. + #[serde(default)] + pub inherited_constraints: Vec, + #[serde(default)] + pub constraints: Vec, + #[serde(default)] + pub extractions: Vec, + /// Paths first created through direct agent file tools in this session. + /// They remain distinct from repository files across session restoration. + #[serde(default)] + pub agent_created_paths: Vec, + /// Agent-created paths inherited at fork time. They are part of the child + /// baseline rather than parent turn-scoped history. + #[serde(default)] + pub inherited_agent_created_paths: Vec, + /// Turn-scoped provenance used to rewind helper-file permissions when a + /// session is rolled back. `agent_created_paths` remains for backwards + /// compatibility and is rebuilt from these records when possible. + #[serde(default)] + pub agent_created_path_records: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct AgentCreatedPathRecord { + pub path: String, + pub dialog_turn_id: String, +} + +impl Default for EditConstraintState { + fn default() -> Self { + Self { + schema_version: EDIT_CONSTRAINT_SCHEMA_VERSION, + inherited_constraints: Vec::new(), + constraints: Vec::new(), + extractions: Vec::new(), + agent_created_paths: Vec::new(), + inherited_agent_created_paths: Vec::new(), + agent_created_path_records: Vec::new(), + } + } +} + +impl EditConstraintState { + pub fn mark_current_state_as_fork_baseline(&mut self) { + self.schema_version = EDIT_CONSTRAINT_SCHEMA_VERSION; + self.inherited_constraints = self.constraints.clone(); + self.inherited_agent_created_paths = self.agent_created_paths.clone(); + } + + pub fn merge_extraction(&mut self, extraction: ConstraintExtractionRecord) { + self.schema_version = EDIT_CONSTRAINT_SCHEMA_VERSION; + self.constraints.retain(|constraint| { + !extraction + .revoked_constraint_ids + .iter() + .any(|constraint_id| constraint_id == &constraint.id) + }); + for constraint in &extraction.constraints { + if !self.constraints.iter().any(|existing| { + existing.matcher == constraint.matcher + && existing.operation_scope == constraint.operation_scope + }) { + self.constraints.push(constraint.clone()); + } + } + self.extractions.push(extraction); + } + + pub fn message_processed(&self, dialog_turn_id: &str, message_sha256: &str) -> bool { + self.extractions.iter().any(|record| { + record.dialog_turn_id.as_deref() == Some(dialog_turn_id) + && record.message_sha256 == message_sha256 + && record.status != ExtractionStatus::Failed + }) + } + + pub fn latest_status(&self) -> Option { + self.extractions.last().map(|record| record.status) + } + + pub fn has_enforceable_constraints(&self) -> bool { + self.constraints + .iter() + .any(|constraint| constraint.matcher.enforceable()) + } + + pub(super) fn tracks_agent_created_test_paths(&self) -> bool { + self.constraints.iter().any(|constraint| { + matches!(constraint.matcher, ConstraintMatcher::TestFiles) + && constraint.operation_scope == ConstraintOperationScope::All + }) + } + + pub(super) fn has_agent_created_paths(&self) -> bool { + !self.agent_created_paths.is_empty() + } + + pub fn remember_agent_created_paths( + &mut self, + paths: impl IntoIterator, + dialog_turn_id: &str, + ) { + for path in paths { + let normalized = path.replace('\\', "/"); + if !normalized.is_empty() && !self.agent_created_paths.contains(&normalized) { + self.agent_created_paths.push(normalized.clone()); + } + if !normalized.is_empty() + && !dialog_turn_id.is_empty() + && !self.agent_created_path_records.iter().any(|record| { + record.path == normalized && record.dialog_turn_id == dialog_turn_id + }) + { + self.agent_created_path_records + .push(AgentCreatedPathRecord { + path: normalized, + dialog_turn_id: dialog_turn_id.to_string(), + }); + } + } + } + + pub fn forget_agent_created_paths_under(&mut self, paths: &[String]) { + self.agent_created_paths.retain(|created| { + !paths.iter().any(|path| { + let path = path.trim_end_matches('/'); + created == path || created.starts_with(&format!("{path}/")) + }) + }); + self.agent_created_path_records.retain(|record| { + !paths.iter().any(|path| { + let path = path.trim_end_matches('/'); + record.path == path || record.path.starts_with(&format!("{path}/")) + }) + }); + } + + pub(super) fn is_agent_created_path(&self, paths: &[String]) -> bool { + paths + .iter() + .any(|path| self.agent_created_paths.contains(path)) + } + + /// Rebuilds the state from events belonging to turns that survive a + /// session rollback. Older provenance entries did not carry a turn id, so + /// they are deliberately discarded rather than granting a stale exemption + /// to a file created only in a rolled-back future turn. + pub fn rollback_to_surviving_turns(&mut self, surviving_turn_ids: &HashSet) { + let retained_extractions = self + .extractions + .iter() + .filter(|record| { + record + .dialog_turn_id + .as_ref() + .is_some_and(|turn_id| surviving_turn_ids.contains(turn_id)) + }) + .cloned() + .collect::>(); + let retained_path_records = self + .agent_created_path_records + .iter() + .filter(|record| surviving_turn_ids.contains(&record.dialog_turn_id)) + .cloned() + .collect::>(); + + self.schema_version = EDIT_CONSTRAINT_SCHEMA_VERSION; + self.constraints = self.inherited_constraints.clone(); + self.extractions.clear(); + for extraction in retained_extractions { + self.merge_extraction(extraction); + } + self.agent_created_paths = self.inherited_agent_created_paths.clone(); + self.agent_created_path_records = retained_path_records; + for record in &self.agent_created_path_records { + if !self.agent_created_paths.contains(&record.path) { + self.agent_created_paths.push(record.path.clone()); + } + } + } +} diff --git a/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/shell_targets.rs b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/shell_targets.rs new file mode 100644 index 0000000000..a0ab08dc92 --- /dev/null +++ b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/shell_targets.rs @@ -0,0 +1,553 @@ +use regex::Regex; +use std::path::Path; +use std::sync::OnceLock; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum ShellMutationOperation { + Write, + Delete, +} + +impl ShellMutationOperation { + pub(super) fn guard_operation(self) -> &'static str { + match self { + Self::Write => "write", + Self::Delete => "delete", + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct ShellMutationTarget { + pub(super) path: String, + pub(super) operation: ShellMutationOperation, +} + +pub(super) fn explicit_bash_mutation_targets(command: &str) -> Vec { + let mut targets = Vec::new(); + // Python `-c` programs commonly contain semicolons inside the quoted + // program. Scan the complete command before shell-level segmentation so a + // later `Path(...).write_text()` expression keeps its Python context. + push_python_mutation_targets(&mut targets, command); + push_node_mutation_targets(&mut targets, command); + for segment in command + .split(['\n', ';', '|']) + .flat_map(|part| part.split("&&")) + .flat_map(|part| part.split("||")) + { + let words = segment + .split_whitespace() + .map(|word| { + word.trim_matches(|c: char| matches!(c, '\'' | '"' | '(' | ')' | '[' | ']')) + }) + .filter(|word| !word.is_empty()) + .collect::>(); + if words.is_empty() { + continue; + } + + for (index, word) in words.iter().enumerate() { + let redirection = word.trim_start_matches(|c| matches!(c, '0'..='9')); + if matches!(redirection, ">" | ">>" | "1>" | "1>>") { + if let Some(path) = words.get(index + 1) { + push_bash_target(&mut targets, path, ShellMutationOperation::Write); + } + } else if let Some(path) = redirection + .strip_prefix(">>") + .or_else(|| redirection.strip_prefix('>')) + { + if !path.is_empty() { + push_bash_target(&mut targets, path, ShellMutationOperation::Write); + } + } + } + + let Some(command_index) = words.iter().position(|word| !word.contains('=')) else { + continue; + }; + let command_name = Path::new(words[command_index]) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(words[command_index]) + .to_ascii_lowercase(); + let arguments = &words[command_index + 1..]; + match command_name.as_str() { + "tee" => { + for argument in arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, argument, ShellMutationOperation::Write); + } + } + "cp" | "install" => { + if let Some(path) = arguments + .iter() + .rev() + .find(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, path, ShellMutationOperation::Write); + } + } + "dd" => { + for argument in arguments { + if let Some(path) = argument.strip_prefix("of=") { + push_bash_target(&mut targets, path, ShellMutationOperation::Write); + } + } + } + "ln" | "rsync" => { + if let Some(path) = arguments + .iter() + .rev() + .find(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, path, ShellMutationOperation::Write); + } + } + "mkdir" => { + for argument in arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, argument, ShellMutationOperation::Write); + } + } + "mv" => { + // Moving a protected source removes it from its original + // location, so both sides are mutation targets. The previous + // destination-only handling let `mv tests/a.rs src/a.rs` + // bypass a test-file constraint. + let paths = arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + .collect::>(); + for (index, argument) in paths.iter().enumerate() { + let operation = if index + 1 == paths.len() { + ShellMutationOperation::Write + } else { + ShellMutationOperation::Delete + }; + push_bash_target(&mut targets, argument, operation); + } + } + "touch" | "truncate" => { + for argument in arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, argument, ShellMutationOperation::Write); + } + } + "rm" | "rmdir" | "unlink" => { + for argument in arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(&mut targets, argument, ShellMutationOperation::Delete); + } + } + "sed" | "perl" => { + if arguments.iter().any(|argument| in_place_flag(argument)) { + let mut script_seen = false; + for argument in arguments + .iter() + .filter(|argument| !argument.starts_with('-')) + { + if !script_seen { + script_seen = true; + continue; + } + if argument.starts_with('/') + || argument.starts_with("./") + || argument.starts_with("../") + || argument.contains('.') + || argument.starts_with("test/") + || argument.starts_with("tests/") + { + push_bash_target(&mut targets, argument, ShellMutationOperation::Write); + } + } + } + } + "git" => push_git_mutation_targets(&mut targets, arguments), + _ => {} + } + } + targets +} + +pub(super) fn has_unresolved_bash_mutation(command: &str, targets: &[ShellMutationTarget]) -> bool { + if targets + .iter() + .any(|target| path_has_shell_expansion(&target.path)) + { + return true; + } + + let lower_command = command.to_ascii_lowercase(); + if (lower_command.contains("python") || lower_command.contains("path(")) + && python_segment_may_mutate(&lower_command) + { + let mut python_targets = Vec::new(); + push_python_mutation_targets(&mut python_targets, command); + if python_targets.is_empty() { + return true; + } + } + if lower_command.contains("node") && node_segment_may_mutate(&lower_command) { + let mut node_targets = Vec::new(); + push_node_mutation_targets(&mut node_targets, command); + if node_targets.is_empty() { + return true; + } + } + + for segment in command + .split(['\n', ';', '|']) + .flat_map(|part| part.split("&&")) + .flat_map(|part| part.split("||")) + { + let words = segment + .split_whitespace() + .map(|word| { + word.trim_matches(|c: char| matches!(c, '\'' | '"' | '(' | ')' | '[' | ']')) + }) + .filter(|word| !word.is_empty()) + .collect::>(); + let Some(command_index) = words.iter().position(|word| !word.contains('=')) else { + continue; + }; + let command_name = Path::new(words[command_index]) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(words[command_index]) + .to_ascii_lowercase(); + let arguments = &words[command_index + 1..]; + let lower_segment = segment.to_ascii_lowercase(); + let segment_targets = explicit_bash_mutation_targets(segment); + + if matches!(command_name.as_str(), "bash" | "sh" | "zsh" | "fish") { + let Some(payload) = nested_shell_payload(segment) else { + return true; + }; + let nested_targets = explicit_bash_mutation_targets(payload); + if has_unresolved_bash_mutation(payload, &nested_targets) { + return true; + } + continue; + } + if matches!( + command_name.as_str(), + "eval" + | "xargs" + | "patch" + | "apply_patch" + | "ruby" + | "php" + | "powershell" + | "pwsh" + | "cmd" + ) { + return true; + } + if command_name == "tar" + && !arguments.iter().any(|argument| { + matches!(*argument, "-t" | "--list") + || (argument.starts_with('-') && argument.contains('t')) + }) + { + return true; + } + if command_name == "unzip" + && !arguments + .iter() + .any(|argument| matches!(*argument, "-l" | "-v")) + { + return true; + } + if command_name == "awk" + && (arguments.iter().any(|argument| in_place_flag(argument)) + || lower_segment.contains("system(")) + { + return true; + } + if command_name == "find" + && arguments + .iter() + .any(|argument| matches!(*argument, "-delete" | "-exec" | "-execdir")) + { + return true; + } + if matches!(command_name.as_str(), "python" | "python3") + && python_segment_may_mutate(&lower_segment) + && segment_targets.is_empty() + { + return true; + } + if command_name == "node" + && node_segment_may_mutate(&lower_segment) + && segment_targets.is_empty() + { + return true; + } + if command_name == "git" + && git_command_may_change_worktree(arguments) + && segment_targets.is_empty() + { + return true; + } + } + + false +} + +fn in_place_flag(argument: &str) -> bool { + argument == "--in-place" + || argument.starts_with("--in-place=") + || argument + .strip_prefix('-') + .is_some_and(|flags| !flags.starts_with('-') && flags.contains('i')) +} + +fn nested_shell_payload(segment: &str) -> Option<&str> { + let mut parts = segment.trim().splitn(3, char::is_whitespace); + parts.next()?; + let flags = parts.next()?; + if !flags.starts_with('-') || !flags.contains('c') { + return None; + } + let payload = parts.next()?.trim(); + Some(payload.trim_matches(|character| matches!(character, '\'' | '"'))) +} + +fn path_has_shell_expansion(path: &str) -> bool { + path.contains('$') + || path.contains('`') + || path.contains('*') + || path.contains('?') + || path.contains('[') + || path.contains('{') +} + +fn python_segment_may_mutate(segment: &str) -> bool { + segment.contains("write_text") + || segment.contains("write_bytes") + || segment.contains(".unlink(") + || segment.contains(".rename(") + || segment.contains(".replace(") + || (segment.contains("open(") + && ["'w'", "\"w\"", "'a'", "\"a\"", "'x'", "\"x\""] + .iter() + .any(|mode| segment.contains(mode))) +} + +fn node_segment_may_mutate(segment: &str) -> bool { + [ + "writefile", + "appendfile", + "unlink", + "rmsync", + "rename", + "copyfile", + ] + .iter() + .any(|operation| segment.contains(operation)) +} + +fn git_command_may_change_worktree(arguments: &[&str]) -> bool { + let Some(subcommand) = arguments.iter().find(|argument| !argument.starts_with('-')) else { + return false; + }; + matches!( + *subcommand, + "checkout" + | "switch" + | "pull" + | "merge" + | "rebase" + | "reset" + | "restore" + | "stash" + | "clean" + | "cherry-pick" + ) +} + +fn push_git_mutation_targets(targets: &mut Vec, arguments: &[&str]) { + let Some((subcommand_index, subcommand)) = arguments + .iter() + .enumerate() + .find(|(_, argument)| !argument.starts_with('-')) + else { + return; + }; + let remaining = &arguments[subcommand_index + 1..]; + match *subcommand { + "mv" => { + let paths = remaining + .iter() + .filter(|argument| !argument.starts_with('-')) + .collect::>(); + for (index, argument) in paths.iter().enumerate() { + let operation = if index + 1 == paths.len() { + ShellMutationOperation::Write + } else { + ShellMutationOperation::Delete + }; + push_bash_target(targets, argument, operation); + } + } + "rm" => { + for argument in remaining + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(targets, argument, ShellMutationOperation::Delete); + } + } + "restore" => { + for argument in remaining + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(targets, argument, ShellMutationOperation::Write); + } + } + "checkout" => { + // `git checkout ` is not a path mutation by itself. The + // pathspec form is unambiguous only after `--`. + if let Some(separator) = remaining.iter().position(|argument| *argument == "--") { + for argument in remaining[separator + 1..] + .iter() + .filter(|argument| !argument.starts_with('-')) + { + push_bash_target(targets, argument, ShellMutationOperation::Write); + } + } + } + _ => {} + } +} + +fn push_python_mutation_targets(targets: &mut Vec, segment: &str) { + static OPEN_FOR_WRITE: OnceLock = OnceLock::new(); + static PATH_WRITE: OnceLock = OnceLock::new(); + static PATH_DELETE: OnceLock = OnceLock::new(); + static PATH_MOVE: OnceLock = OnceLock::new(); + let open_for_write = OPEN_FOR_WRITE.get_or_init(|| { + Regex::new(r#"(?i)\bopen\s*\(\s*["']([^"']+)["']\s*,\s*["'][wax][^"']*["']"#) + .expect("valid Python open-for-write regex") + }); + let path_write = PATH_WRITE.get_or_init(|| { + Regex::new( + r#"(?i)\bPath\s*\(\s*["']([^"']+)["']\s*\)\s*\.\s*(?:write_text|write_bytes)\s*\("#, + ) + .expect("valid pathlib write regex") + }); + let path_delete = PATH_DELETE.get_or_init(|| { + Regex::new(r#"(?i)\bPath\s*\(\s*["']([^"']+)["']\s*\)\s*\.\s*unlink\s*\("#) + .expect("valid pathlib delete regex") + }); + let path_move = PATH_MOVE.get_or_init(|| { + Regex::new( + r#"(?i)\bPath\s*\(\s*["']([^"']+)["']\s*\)\s*\.\s*(?:rename|replace)\s*\(\s*["']([^"']+)["']"#, + ) + .expect("valid pathlib move regex") + }); + + for captures in open_for_write.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } + for captures in path_write.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } + for captures in path_delete.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Delete); + } + } + for captures in path_move.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Delete); + } + if let Some(path) = captures.get(2) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } +} + +fn push_node_mutation_targets(targets: &mut Vec, segment: &str) { + static SINGLE_PATH_WRITE: OnceLock = OnceLock::new(); + static SINGLE_PATH_DELETE: OnceLock = OnceLock::new(); + static TWO_PATH_COPY: OnceLock = OnceLock::new(); + static TWO_PATH_MOVE: OnceLock = OnceLock::new(); + let single_path_write = SINGLE_PATH_WRITE.get_or_init(|| { + Regex::new( + r#"(?i)\b(?:fs\s*\.\s*)?(?:writefilesync|appendfilesync)\s*\(\s*["']([^"']+)["']"#, + ) + .expect("valid Node single-path write regex") + }); + let single_path_delete = SINGLE_PATH_DELETE.get_or_init(|| { + Regex::new(r#"(?i)\b(?:fs\s*\.\s*)?(?:unlinksync|rmsync)\s*\(\s*["']([^"']+)["']"#) + .expect("valid Node single-path delete regex") + }); + let two_path_copy = TWO_PATH_COPY.get_or_init(|| { + Regex::new( + r#"(?i)\b(?:fs\s*\.\s*)?copyfilesync\s*\(\s*["']([^"']+)["']\s*,\s*["']([^"']+)["']"#, + ) + .expect("valid Node copy regex") + }); + let two_path_move = TWO_PATH_MOVE.get_or_init(|| { + Regex::new( + r#"(?i)\b(?:fs\s*\.\s*)?renamesync\s*\(\s*["']([^"']+)["']\s*,\s*["']([^"']+)["']"#, + ) + .expect("valid Node move regex") + }); + + for captures in single_path_write.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } + for captures in single_path_delete.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Delete); + } + } + for captures in two_path_copy.captures_iter(segment) { + if let Some(path) = captures.get(2) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } + for captures in two_path_move.captures_iter(segment) { + if let Some(path) = captures.get(1) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Delete); + } + if let Some(path) = captures.get(2) { + push_bash_target(targets, path.as_str(), ShellMutationOperation::Write); + } + } +} + +fn push_bash_target( + targets: &mut Vec, + raw_path: &str, + operation: ShellMutationOperation, +) { + let path = raw_path.trim_matches(|c: char| matches!(c, '\'' | '"' | ',')); + if !path.is_empty() + && !targets + .iter() + .any(|existing| existing.path == path && existing.operation == operation) + { + targets.push(ShellMutationTarget { + path: path.to_string(), + operation, + }); + } +} diff --git a/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/tests.rs b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/tests.rs new file mode 100644 index 0000000000..711918ce8f --- /dev/null +++ b/src/crates/assembly/core/src/agentic/execution/edit_constraint_guard/tests.rs @@ -0,0 +1,859 @@ +use super::*; +use crate::agentic::tools::ToolRuntimeRestrictions; +use crate::agentic::WorkspaceBinding; +use std::collections::{HashMap, HashSet}; + +fn constraint(description: &str, matcher: ConstraintMatcher) -> ExtractedConstraint { + ExtractedConstraint { + id: format!("test:{description}"), + description: description.to_string(), + operation_scope: ConstraintOperationScope::All, + matcher, + source: ConstraintSource::Legacy, + source_text: None, + } +} + +fn parsed_shell_targets(command: &str) -> Vec<(String, ShellMutationOperation)> { + explicit_bash_mutation_targets(command) + .into_iter() + .map(|target| (target.path, target.operation)) + .collect() +} + +#[test] +fn test_files_matcher_covers_common_conventions() { + let matcher = ConstraintMatcher::TestFiles; + for path in [ + "report/util_test.go", + "pkg/foo/test_bar.py", + "pkg/foo/bar_test.py", + "src/foo.test.tsx", + "src/foo.spec.ts", + "spec/models/user_spec.rb", + "pkg/foo_test.cc", + "src/foo-test.js", + "src/test-widget.ts", + "test/components/Foo-test.tsx", + "__tests__/foo.js", + "TEST/UPPER.spec.ts", + ] { + assert!(matcher.matches(path), "expected test path: {path}"); + } + assert!(!matcher.matches("src/foo.ts")); + assert!(!matcher.matches("report/util.go")); +} + +#[test] +fn deterministic_extractor_recognizes_direct_test_file_restriction() { + let message = "Do not modify the testing logic or any test files."; + let extracted = deterministic_test_constraint(message).expect("test constraint"); + assert_eq!(extracted.matcher, ConstraintMatcher::TestFiles); + assert_eq!(extracted.source, ConstraintSource::Deterministic); + assert!(extracted.source_text.is_some()); +} + +#[test] +fn deterministic_extractor_does_not_confuse_do_not_run_tests() { + assert!(deterministic_test_constraint("Do not run the tests on Windows.").is_none()); +} + +#[test] +fn deterministic_extractor_rejects_keyword_cooccurrence_false_positives() { + for message in [ + "You don't have to modify tests.", + "Do not delete source files because tests fail.", + "Don't forget to modify tests.", + "The tests already cover this behavior.", + "Avoid modifying production behavior just to satisfy tests.", + ] { + assert!( + deterministic_test_constraint(message).is_none(), + "expected no deterministic constraint for: {message}" + ); + } +} + +#[test] +fn generic_path_restriction_phrases_reach_the_model_prefilter() { + for message in [ + "Cargo.lock is off limits.", + "Leave package.json untouched.", + "Keep generated/schema.rs unchanged.", + ] { + assert!( + has_prohibition_signal(message), + "expected prohibition signal for: {message}" + ); + } +} + +#[test] +fn deterministic_extractor_recognizes_unchanged_and_non_test_only_wording() { + for message in [ + "Keep test files unchanged.", + "Tests must remain unchanged.", + "Only modify non-test files.", + "测试文件保持不变。", + ] { + assert!( + deterministic_test_constraint(message).is_some(), + "expected deterministic constraint for: {message}" + ); + } +} + +#[test] +fn deterministic_extractor_does_not_turn_explicit_relaxation_into_a_prohibition() { + for message in [ + "You can modify tests now.", + "Test files are allowed to be modified.", + "现在可以修改测试文件。", + ] { + assert!( + deterministic_test_constraint(message).is_none(), + "expected no deterministic prohibition for: {message}" + ); + } +} + +#[test] +fn long_prompt_keeps_both_ends() { + let input = format!("start{}do not modify tests", "x".repeat(MAX_PROMPT_CHARS)); + let (truncated, was_truncated) = truncate_for_extraction(&input); + assert!(was_truncated); + assert!(truncated.starts_with("start")); + assert!(truncated.ends_with("do not modify tests")); +} + +#[test] +fn matchers_cover_paths_extensions_and_unmatched() { + assert!(ConstraintMatcher::PathContains { + substrings: vec!["package-lock.json".to_string()] + } + .matches("frontend/package-lock.json")); + assert!(ConstraintMatcher::PathUnderDir { + dirs: vec!["migrations".to_string()] + } + .matches("db/migrations/0002_add_column.sql")); + assert!(ConstraintMatcher::Extension { + exts: vec![".lock".to_string()] + } + .matches("Cargo.lock")); + assert!(!ConstraintMatcher::Unmatched.matches("anything.go")); +} + +#[test] +fn terminal_preflight_finds_explicit_mutation_targets() { + assert_eq!( + parsed_shell_targets("sed -i 's/old/new/' tests/example.rs"), + vec![( + "tests/example.rs".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets("printf x > test/unit/output.txt && touch src/lib.rs"), + vec![ + ( + "test/unit/output.txt".to_string(), + ShellMutationOperation::Write + ), + ("src/lib.rs".to_string(), ShellMutationOperation::Write) + ] + ); + assert_eq!( + parsed_shell_targets("cargo test -p core"), + Vec::<(String, ShellMutationOperation)>::new() + ); + assert_eq!( + parsed_shell_targets( + r#"python3 -c \"open('/app/test/unit/example_test.py', 'w').write('x')\""# + ), + vec![( + "/app/test/unit/example_test.py".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets( + r#"python -c \"from pathlib import Path; Path('tests/repro_test.py').write_text('x')\""# + ), + vec![( + "tests/repro_test.py".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets("mv tests/existing_test.py src/existing.py"), + vec![ + ( + "tests/existing_test.py".to_string(), + ShellMutationOperation::Delete + ), + ("src/existing.py".to_string(), ShellMutationOperation::Write) + ] + ); + assert_eq!( + parsed_shell_targets( + r#"node -e \"require('fs').writeFileSync('tests/example.test.js', 'x')\""# + ), + vec![( + "tests/example.test.js".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets("git mv tests/example.rs src/example.rs"), + vec![ + ( + "tests/example.rs".to_string(), + ShellMutationOperation::Delete + ), + ("src/example.rs".to_string(), ShellMutationOperation::Write) + ] + ); + assert_eq!( + parsed_shell_targets("git checkout HEAD -- tests/example.rs"), + vec![( + "tests/example.rs".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets("dd if=/tmp/input of=tests/example.rs"), + vec![( + "tests/example.rs".to_string(), + ShellMutationOperation::Write + )] + ); + assert_eq!( + parsed_shell_targets("rsync -a src/ tests/generated/"), + vec![( + "tests/generated/".to_string(), + ShellMutationOperation::Write + )] + ); +} + +#[test] +fn terminal_preflight_marks_unresolved_mutations() { + for command in [ + "target=tests/example.rs; printf x > \"$target\"", + "python -c \"from pathlib import Path; Path(target).write_text('x')\"", + "bash", + "find tests -type f -exec rm {} +", + "git checkout HEAD tests/example.rs", + "tar -xf generated-tests.tar", + "unzip generated-tests.zip", + "patch -p1 < change.patch", + ] { + let targets = explicit_bash_mutation_targets(command); + assert!( + has_unresolved_bash_mutation(command, &targets), + "expected unresolved mutation: {command}" + ); + } + + for command in [ + "cargo test -p core", + "bash -lc 'cargo test -p core'", + "git status", + "git checkout HEAD -- tests/example.rs", + "dd if=/tmp/input of=tests/example.rs", + "rsync -a src/ tests/generated/", + "tar -tf generated-tests.tar", + "unzip -l generated-tests.zip", + ] { + let targets = explicit_bash_mutation_targets(command); + assert!( + !has_unresolved_bash_mutation(command, &targets), + "expected resolved or read-only command: {command}" + ); + } +} + +#[test] +fn shell_delete_targets_apply_delete_only_constraints() { + let delete_only = ExtractedConstraint { + id: "test:delete-only".to_string(), + description: "do not delete tests".to_string(), + operation_scope: ConstraintOperationScope::DeleteOnly, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Deterministic, + source_text: Some("Do not delete tests.".to_string()), + }; + + for command in [ + "rm tests/example.rs", + "git rm tests/example.rs", + "python -c \"from pathlib import Path; Path('tests/example.rs').unlink()\"", + "node -e \"require('fs').unlinkSync('tests/example.rs')\"", + ] { + let target = explicit_bash_mutation_targets(command) + .into_iter() + .find(|target| target.path == "tests/example.rs") + .unwrap_or_else(|| panic!("missing delete target for {command}")); + assert_eq!(target.operation, ShellMutationOperation::Delete); + assert!(find_violation_for_operation( + std::slice::from_ref(&delete_only), + &target.path, + target.operation.guard_operation(), + ) + .is_some()); + } + + let write = explicit_bash_mutation_targets("touch tests/example.rs") + .into_iter() + .next() + .expect("write target"); + assert_eq!(write.operation, ShellMutationOperation::Write); + assert!(find_violation_for_operation( + &[delete_only], + &write.path, + write.operation.guard_operation(), + ) + .is_none()); +} + +#[test] +fn fast_response_parser_requires_the_observable_update_schema() { + let valid = r#"{ + "additions": [{ + "description": "do not modify tests", + "matcher": {"kind": "test_files"} + }], + "revocations": [{ + "constraint_id": "deterministic:test_files", + "description": "tests may now be modified" + }] + }"#; + let parsed: ExtractionResponse = serde_json::from_str(valid).expect("valid schema"); + assert_eq!(parsed.additions.len(), 1); + assert_eq!(parsed.revocations.len(), 1); + + assert!(serde_json::from_str::( + r#"{"constraints": [], "revocations": []}"# + ) + .is_err()); + assert!(serde_json::from_str::(r#"{"additions": []}"#).is_err()); +} + +#[test] +fn state_distinguishes_failed_from_processed_extraction() { + let mut state = EditConstraintState::default(); + let failed = ConstraintExtractionRecord { + message_sha256: "hash".to_string(), + dialog_turn_id: Some("turn-1".to_string()), + status: ExtractionStatus::Failed, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 2, + active_constraint_ids: Vec::new(), + revocation_authorized: true, + model_status: ModelExtractionStatus::Failed, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars: 10, + prompt_chars: 10, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: Some(ExtractionFailure { + stage: "schema_validation".to_string(), + reason: "bad json".to_string(), + }), + response_excerpt: None, + }; + state.merge_extraction(failed); + assert!(!state.message_processed("turn-1", "hash")); + + let mut completed = state.extractions[0].clone(); + completed.status = ExtractionStatus::NoConstraints; + completed.failure = None; + state.merge_extraction(completed); + assert!(state.message_processed("turn-1", "hash")); + assert!(!state.message_processed("turn-2", "hash")); +} + +#[test] +fn internal_turns_cannot_revoke_a_user_edit_constraint() { + let protected = constraint("don't touch tests", ConstraintMatcher::TestFiles); + let revocation = ConstraintRevocation { + constraint_id: protected.id.clone(), + description: "tests may be modified now".to_string(), + }; + + let (revoked, unmatched) = validated_revocation_ids(&[revocation], &[protected.clone()], false); + + assert!(revoked.is_empty()); + assert!(unmatched.is_empty()); + let (revoked, unmatched) = validated_revocation_ids( + &[ConstraintRevocation { + constraint_id: protected.id.clone(), + description: "tests may be modified now".to_string(), + }], + &[protected], + true, + ); + assert_eq!(revoked, vec!["test:don't touch tests".to_string()]); + assert!(unmatched.is_empty()); +} + +#[test] +fn state_applies_only_validated_explicit_revocations() { + let protected = constraint("don't touch tests", ConstraintMatcher::TestFiles); + let protected_id = protected.id.clone(); + let mut state = EditConstraintState::default(); + state.constraints.push(protected); + + state.merge_extraction(ConstraintExtractionRecord { + message_sha256: "relaxation-hash".to_string(), + dialog_turn_id: Some("turn-2".to_string()), + status: ExtractionStatus::Extracted, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![protected_id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: vec![ConstraintRevocation { + constraint_id: protected_id.clone(), + description: "tests may be modified now".to_string(), + }], + revoked_constraint_ids: vec![protected_id], + unmatched_revocation_ids: Vec::new(), + input_chars: 24, + prompt_chars: 24, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: Some( + r#"{"additions":[],"revocations":[{"constraint_id":"test:don't touch tests"}]}"# + .to_string(), + ), + }); + + assert!(state.constraints.is_empty()); + assert_eq!(state.schema_version, EDIT_CONSTRAINT_SCHEMA_VERSION); +} + +#[test] +fn failed_or_unmatched_revocation_keeps_active_constraint() { + let protected = constraint("don't touch tests", ConstraintMatcher::TestFiles); + let mut state = EditConstraintState::default(); + state.constraints.push(protected.clone()); + + state.merge_extraction(ConstraintExtractionRecord { + message_sha256: "invalid-relaxation-hash".to_string(), + dialog_turn_id: Some("turn-2".to_string()), + status: ExtractionStatus::NoConstraints, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![protected.id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: vec![ConstraintRevocation { + constraint_id: "invented-id".to_string(), + description: "ambiguous relaxation".to_string(), + }], + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: vec!["invented-id".to_string()], + input_chars: 20, + prompt_chars: 20, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: None, + }); + + assert_eq!(state.constraints, vec![protected]); +} + +#[test] +fn find_violation_returns_first_match() { + let constraints = vec![ + constraint("don't touch tests", ConstraintMatcher::TestFiles), + constraint( + "don't touch lockfiles", + ConstraintMatcher::Extension { + exts: vec![".lock".to_string()], + }, + ), + ]; + assert_eq!( + find_violation(&constraints, "report/util_test.go") + .map(|constraint| constraint.description.as_str()), + Some("don't touch tests") + ); + assert_eq!( + find_violation(&constraints, "Cargo.lock") + .map(|constraint| constraint.description.as_str()), + Some("don't touch lockfiles") + ); +} + +#[test] +fn new_files_are_exempt_only_from_test_file_constraints() { + let test_only = EditConstraintState { + constraints: vec![constraint( + "don't touch tests", + ConstraintMatcher::TestFiles, + )], + ..Default::default() + }; + let test_path = vec!["test/repro-test.ts".to_string()]; + assert!(has_only_relaxable_test_file_violations( + &test_only, &test_path, "write" + )); + + let stricter = EditConstraintState { + constraints: vec![ + constraint("don't touch tests", ConstraintMatcher::TestFiles), + constraint( + "don't modify generated files", + ConstraintMatcher::PathUnderDir { + dirs: vec!["test".to_string()], + }, + ), + ], + ..Default::default() + }; + assert!(!has_only_relaxable_test_file_violations( + &stricter, &test_path, "write" + )); +} + +#[test] +fn agent_created_test_files_can_be_cleaned_up_but_delete_only_rules_remain_strict() { + let path = vec!["test/repro-test.ts".to_string()]; + let mut state = EditConstraintState { + constraints: vec![constraint( + "don't modify tests", + ConstraintMatcher::TestFiles, + )], + ..Default::default() + }; + state.remember_agent_created_paths(path.clone(), "turn-1"); + assert!(can_mutate_agent_created_test_file( + Some(&state), + &path, + "edit", + false + )); + assert!(can_mutate_agent_created_test_file( + Some(&state), + &path, + "delete", + false + )); + + state.constraints.push(ExtractedConstraint { + id: "test:do-not-delete".to_string(), + description: "don't delete tests".to_string(), + operation_scope: ConstraintOperationScope::DeleteOnly, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Deterministic, + source_text: Some("Do not delete tests.".to_string()), + }); + assert!( + find_violation_for_operation(&state.constraints, "test/repro-test.ts", "delete").is_some() + ); + assert!( + find_violation_for_operation(&state.constraints, "test/repro-test.ts", "edit").is_some() + ); + assert!(!can_mutate_agent_created_test_file( + Some(&state), + &path, + "delete", + false + )); + + state.forget_agent_created_paths_under(&path); + assert!(!state.is_agent_created_path(&path)); +} + +#[test] +fn rollback_discards_future_constraints_and_helper_provenance() { + let initial_constraint = constraint("don't modify tests", ConstraintMatcher::TestFiles); + let future_constraint = constraint( + "don't modify lockfiles", + ConstraintMatcher::Extension { + exts: vec![".lock".to_string()], + }, + ); + let mut state = EditConstraintState::default(); + state.merge_extraction(ConstraintExtractionRecord { + message_sha256: "turn-1-hash".to_string(), + dialog_turn_id: Some("turn-1".to_string()), + status: ExtractionStatus::Extracted, + constraints: vec![initial_constraint.clone()], + deterministic_constraint_count: 1, + model_attempts: 0, + active_constraint_ids: Vec::new(), + revocation_authorized: true, + model_status: ModelExtractionStatus::NotRun, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars: 10, + prompt_chars: 10, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: None, + }); + state.remember_agent_created_paths(vec!["tests/kept_repro.rs".to_string()], "turn-1"); + state.merge_extraction(ConstraintExtractionRecord { + message_sha256: "turn-2-hash".to_string(), + dialog_turn_id: Some("turn-2".to_string()), + status: ExtractionStatus::Extracted, + constraints: vec![future_constraint], + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![initial_constraint.id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: vec![initial_constraint.id.clone()], + unmatched_revocation_ids: Vec::new(), + input_chars: 10, + prompt_chars: 10, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 2, + failure: None, + response_excerpt: None, + }); + state.remember_agent_created_paths(vec!["tests/future_repro.rs".to_string()], "turn-2"); + + state.rollback_to_surviving_turns(&HashSet::from(["turn-1".to_string()])); + + assert_eq!(state.constraints, vec![initial_constraint]); + assert_eq!(state.extractions.len(), 1); + assert_eq!( + state.agent_created_paths, + vec!["tests/kept_repro.rs".to_string()] + ); + assert_eq!(state.agent_created_path_records.len(), 1); + assert_eq!(state.agent_created_path_records[0].dialog_turn_id, "turn-1"); +} + +#[test] +fn fork_baseline_survives_child_rollback_and_replays_child_revocation() { + let inherited = constraint("don't modify tests", ConstraintMatcher::TestFiles); + let inherited_id = inherited.id.clone(); + let mut child_state = EditConstraintState { + constraints: vec![inherited.clone()], + agent_created_paths: vec!["tests/parent_repro.rs".to_string()], + ..Default::default() + }; + child_state.mark_current_state_as_fork_baseline(); + + let mut rolled_back_to_fork = child_state.clone(); + rolled_back_to_fork.rollback_to_surviving_turns(&HashSet::new()); + assert_eq!(rolled_back_to_fork.constraints, vec![inherited.clone()]); + assert_eq!( + rolled_back_to_fork.agent_created_paths, + vec!["tests/parent_repro.rs".to_string()] + ); + + child_state.merge_extraction(ConstraintExtractionRecord { + message_sha256: "child-revocation".to_string(), + dialog_turn_id: Some("child-turn-1".to_string()), + status: ExtractionStatus::Extracted, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![inherited_id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: vec![ConstraintRevocation { + constraint_id: inherited_id.clone(), + description: "tests may now be modified".to_string(), + }], + revoked_constraint_ids: vec![inherited_id], + unmatched_revocation_ids: Vec::new(), + input_chars: 25, + prompt_chars: 25, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 2, + failure: None, + response_excerpt: None, + }); + child_state.rollback_to_surviving_turns(&HashSet::from(["child-turn-1".to_string()])); + assert!(child_state.constraints.is_empty()); +} + +#[test] +fn deterministic_extractor_marks_explicit_test_deletion_as_delete_only() { + let constraint = deterministic_test_constraint("Do not delete test files.") + .expect("explicit test deletion should be extracted"); + assert_eq!( + constraint.operation_scope, + ConstraintOperationScope::DeleteOnly + ); + assert!(constraint.matcher.matches("tests/example.rs")); +} + +#[test] +fn force_is_rejected_even_without_runtime_context() { + let rejection = + check(None, "Edit", "edit", "tests/example.rs", true).expect("force must be denied"); + assert!(!rejection.result); + assert_eq!(rejection.error_code, Some(403)); + assert_eq!( + rejection + .meta + .as_ref() + .and_then(|value| value.get("guard_decision")) + .and_then(Value::as_str), + Some("force_denied") + ); +} + +#[tokio::test] +async fn blank_input_is_no_constraints_not_failure() { + let extraction = extract_constraints(" \n ").await; + assert_eq!(extraction.status, ExtractionStatus::NoConstraints); + assert!(extraction.constraints.is_empty()); + assert!(extraction.failure.is_none()); +} + +#[tokio::test] +async fn irrelevant_follow_up_with_active_constraints_skips_model() { + let active = constraint("don't touch tests", ConstraintMatcher::TestFiles); + let extraction = + extract_constraints_with_active("Continue with the implementation.", &[active]).await; + + assert_eq!(extraction.status, ExtractionStatus::NoConstraints); + assert_eq!(extraction.model_attempts, 0); + assert!(extraction.constraints.is_empty()); + assert!(extraction.failure.is_none()); +} + +#[tokio::test] +async fn no_signal_extraction_does_not_require_session_state() { + let mut extraction = extract_constraints("Continue with the implementation.").await; + assert!(!extraction_requires_session_state(&extraction)); + + extraction.model_attempts = 1; + assert!(extraction_requires_session_state(&extraction)); + + extraction.model_attempts = 0; + extraction.status = ExtractionStatus::Failed; + assert!(extraction_requires_session_state(&extraction)); +} + +#[test] +fn agent_created_provenance_is_only_needed_for_full_test_constraints() { + let mut state = EditConstraintState::default(); + state.constraints.push(constraint( + "don't touch lockfiles", + ConstraintMatcher::Extension { + exts: vec![".lock".to_string()], + }, + )); + assert!(!state.tracks_agent_created_test_paths()); + + state.constraints.push(constraint( + "don't touch tests", + ConstraintMatcher::TestFiles, + )); + assert!(state.tracks_agent_created_test_paths()); +} + +#[test] +fn telemetry_requires_explicit_opt_in_value() { + for value in ["1", "true", "TRUE", "yes", "on", " On "] { + assert!( + telemetry_setting_enabled(Some(value)), + "expected opt-in: {value}" + ); + } + for value in ["", "0", "false", "off", "enabled"] { + assert!( + !telemetry_setting_enabled(Some(value)), + "unexpected opt-in: {value}" + ); + } + assert!(!telemetry_setting_enabled(None)); +} + +#[test] +fn successful_mutation_telemetry_is_persisted_as_jsonl() { + let root = std::env::temp_dir().join(format!( + "bitfun-edit-constraint-telemetry-{}", + Uuid::new_v4() + )); + fs::create_dir_all(&root).expect("create temp workspace"); + let event = json!({ + "event": "mutation_applied", + "tool_call_id": "tool-call-1", + "requested_path": "tests/example.rs", + }); + let telemetry_path = root.join(TELEMETRY_RELATIVE_PATH); + append_jsonl(&telemetry_path, &event).expect("append telemetry event"); + let line = fs::read_to_string(&telemetry_path).expect("read telemetry"); + let event: Value = serde_json::from_str(line.trim()).expect("valid jsonl event"); + assert_eq!(event["event"], "mutation_applied"); + assert_eq!(event["tool_call_id"], "tool-call-1"); + assert_eq!(event["requested_path"], "tests/example.rs"); + + let _ = fs::remove_dir_all(root); +} + +#[test] +fn local_recursive_delete_fallback_finds_protected_descendant() { + let root = std::env::temp_dir().join(format!( + "bitfun-edit-constraint-recursive-delete-{}", + Uuid::new_v4() + )); + let target = root.join("parent"); + fs::create_dir_all(target.join("tests")).expect("create test directory"); + fs::write(target.join("tests/example.rs"), "test").expect("create test file"); + let context = ToolUseContext { + tool_call_id: Some("tool-call-1".to_string()), + agent_type: Some("agentic".to_string()), + session_id: None, + dialog_turn_id: Some("turn-1".to_string()), + workspace: Some(WorkspaceBinding::new(None, root.clone())), + loaded_deferred_tool_specs: Vec::new(), + primary_model_facts: tool_runtime::context::PrimaryModelFacts::default(), + custom_data: HashMap::new(), + computer_use_host: None, + runtime_tool_restrictions: ToolRuntimeRestrictions::default(), + runtime_handles: bitfun_runtime_ports::ToolRuntimeHandles::default(), + }; + let state = EditConstraintState { + constraints: vec![constraint( + "don't touch tests", + ConstraintMatcher::TestFiles, + )], + ..Default::default() + }; + + let rejection = + check_local_recursive_delete(&context, "parent", &target.to_string_lossy(), &state) + .expect("recursive delete should be denied"); + assert_eq!(rejection.error_code, Some(403)); + assert!(rejection + .message + .as_deref() + .unwrap_or_default() + .contains("tests")); + + let _ = fs::remove_dir_all(root); +} diff --git a/src/crates/assembly/core/src/agentic/execution/execution_engine.rs b/src/crates/assembly/core/src/agentic/execution/execution_engine.rs index 9869ff08c7..e84bf1584c 100644 --- a/src/crates/assembly/core/src/agentic/execution/execution_engine.rs +++ b/src/crates/assembly/core/src/agentic/execution/execution_engine.rs @@ -2615,6 +2615,46 @@ impl ExecutionEngine { .get("original_user_input") .cloned() .unwrap_or_default(); + + // Edit constraint guard: process each distinct user instruction once. + // The fast extractor receives the active state so explicit additions + // and revocations form an auditable session-persistent state machine. + if !original_user_input.trim().is_empty() { + let revocation_authorized = context + .context + .get("edit_constraint_revocation_authorized") + .is_some_and(|value| value == "true"); + let message_sha256 = crate::agentic::execution::edit_constraint_guard::message_sha256( + &original_user_input, + ); + let already_processed = self + .session_manager + .edit_constraint_state(&context.session_id) + .is_some_and(|state| { + state.message_processed(&context.dialog_turn_id, &message_sha256) + }); + if !already_processed { + let active_constraints = self + .session_manager + .edit_constraints(&context.session_id) + .unwrap_or_default(); + let mut extraction = crate::agentic::execution::edit_constraint_guard::extract_constraints_with_active_and_revocation_authorization( + &original_user_input, + &active_constraints, + revocation_authorized, + ) + .await; + extraction.dialog_turn_id = Some(context.dialog_turn_id.clone()); + if crate::agentic::execution::edit_constraint_guard::extraction_requires_session_state( + &extraction, + ) { + self.session_manager + .remember_edit_constraint_extraction(&context.session_id, extraction) + .await; + } + } + } + let model_id = self .resolve_model_id_for_turn( &session, diff --git a/src/crates/assembly/core/src/agentic/execution/mod.rs b/src/crates/assembly/core/src/agentic/execution/mod.rs index 44a3e4ab54..8d0360be71 100644 --- a/src/crates/assembly/core/src/agentic/execution/mod.rs +++ b/src/crates/assembly/core/src/agentic/execution/mod.rs @@ -2,6 +2,7 @@ //! //! Responsible for AI interaction and model round control +pub mod edit_constraint_guard; pub mod execution_engine; pub(crate) mod model_exchange_trace; pub mod round_executor; diff --git a/src/crates/assembly/core/src/agentic/session/session_manager.rs b/src/crates/assembly/core/src/agentic/session/session_manager.rs index abf42e4bdf..f4e0374858 100644 --- a/src/crates/assembly/core/src/agentic/session/session_manager.rs +++ b/src/crates/assembly/core/src/agentic/session/session_manager.rs @@ -165,6 +165,11 @@ pub struct SessionManager { token_anchor_store: Arc, turn_skill_agent_snapshot_store: Arc, skill_agent_baseline_override_snapshot_store: Arc>, + /// Session-scoped edit-constraint state. The in-memory copy serves the hot + /// tool-validation path; the same state is persisted in session metadata so + /// restore and fork paths preserve both constraints and extraction evidence. + edit_constraints_store: + Arc>, file_read_state_store: Arc, evidence_ledger: Arc, persistence_manager: Arc, @@ -1563,6 +1568,7 @@ impl SessionManager { token_anchor_store: Arc::new(TokenAnchorStore::new()), turn_skill_agent_snapshot_store: Arc::new(TurnSkillAgentSnapshotStore::new()), skill_agent_baseline_override_snapshot_store: Arc::new(DashMap::new()), + edit_constraints_store: Arc::new(DashMap::new()), file_read_state_store: Arc::new(FileReadStateStore::new()), evidence_ledger: Arc::new(SessionEvidenceLedger::new()), persistence_manager, @@ -1769,6 +1775,7 @@ impl SessionManager { let turn_skill_agent_snapshot_store = self.turn_skill_agent_snapshot_store.clone(); let skill_agent_baseline_override_snapshot_store = self.skill_agent_baseline_override_snapshot_store.clone(); + let edit_constraints_store = self.edit_constraints_store.clone(); let file_read_state_store = self.file_read_state_store.clone(); let evidence_ledger = self.evidence_ledger.clone(); let persistence_manager = self.persistence_manager.clone(); @@ -1797,6 +1804,7 @@ impl SessionManager { token_anchor_store, turn_skill_agent_snapshot_store, skill_agent_baseline_override_snapshot_store, + edit_constraints_store, file_read_state_store, evidence_ledger, persistence_manager, @@ -2389,6 +2397,196 @@ impl SessionManager { } } + /// Merges one extraction record into the active session state and persists + /// the resulting constraints plus extraction evidence. + pub async fn remember_edit_constraint_extraction( + &self, + session_id: &str, + extraction: crate::agentic::execution::edit_constraint_guard::ConstraintExtractionRecord, + ) { + let mut state = self.edit_constraint_state(session_id).unwrap_or_default(); + state.merge_extraction(extraction); + self.edit_constraints_store + .insert(session_id.to_string(), state.clone()); + + if self.should_persist_session_id(session_id) { + if let Err(error) = self + .merge_session_custom_metadata( + session_id, + json!({ + crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY: state, + }), + ) + .await + { + warn!( + "Failed to persist edit constraint state: session_id={}, error={}", + session_id, error + ); + } + } + } + + /// Records paths first created through direct agent file tools. This is + /// session-persistent provenance used to distinguish temporary agent + /// helpers from repository files protected by edit constraints. + pub async fn remember_edit_constraint_agent_created_paths( + &self, + session_id: &str, + paths: Vec, + dialog_turn_id: &str, + ) { + let mut state = self.edit_constraint_state(session_id).unwrap_or_default(); + state.remember_agent_created_paths(paths, dialog_turn_id); + self.edit_constraints_store + .insert(session_id.to_string(), state.clone()); + + if self.should_persist_session_id(session_id) { + if let Err(error) = self + .merge_session_custom_metadata( + session_id, + json!({ + crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY: state, + }), + ) + .await + { + warn!( + "Failed to persist agent-created file provenance: session_id={}, error={}", + session_id, error + ); + } + } + } + + /// Removes direct-agent provenance after a successful delete. Descendants + /// are removed as well so recursive cleanup cannot leave stale records. + pub async fn forget_edit_constraint_agent_created_paths_under( + &self, + session_id: &str, + paths: Vec, + ) { + let Some(mut state) = self.edit_constraint_state(session_id) else { + return; + }; + state.forget_agent_created_paths_under(&paths); + self.edit_constraints_store + .insert(session_id.to_string(), state.clone()); + + if self.should_persist_session_id(session_id) { + if let Err(error) = self + .merge_session_custom_metadata( + session_id, + json!({ + crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY: state, + }), + ) + .await + { + warn!( + "Failed to persist removed agent-created file provenance: session_id={}, error={}", + session_id, error + ); + } + } + } + + /// Rewinds edit constraints and direct-file provenance to the turns that + /// remain after a session rollback. This prevents a restriction, explicit + /// relaxation, or temporary helper created in discarded future context + /// from leaking into the resumed branch. + pub async fn rollback_edit_constraint_state_to_turns( + &self, + session_id: &str, + surviving_turn_ids: &std::collections::HashSet, + ) { + let Some(mut state) = self.edit_constraint_state(session_id) else { + return; + }; + state.rollback_to_surviving_turns(surviving_turn_ids); + self.edit_constraints_store + .insert(session_id.to_string(), state.clone()); + + if self.should_persist_session_id(session_id) { + if let Err(error) = self + .merge_session_custom_metadata( + session_id, + json!({ + crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY: state, + }), + ) + .await + { + warn!( + "Failed to persist rolled-back edit constraint state: session_id={}, error={}", + session_id, error + ); + } + } + } + + pub fn edit_constraint_state( + &self, + session_id: &str, + ) -> Option { + self.edit_constraints_store + .get(session_id) + .map(|value| value.clone()) + } + + fn edit_constraint_state_from_metadata( + metadata: Option<&SessionMetadata>, + ) -> Option { + let value = metadata? + .custom_metadata + .as_ref()? + .get(crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY)?; + match serde_json::from_value(value.clone()) { + Ok(state) => Some(state), + Err(error) => { + warn!("Failed to restore edit constraint state from session metadata: {error}"); + None + } + } + } + + pub fn edit_constraints( + &self, + session_id: &str, + ) -> Option> { + self.edit_constraint_state(session_id) + .map(|state| state.constraints) + } + + /// Subagents inherit both active constraints and extraction evidence. + pub async fn seed_forked_edit_constraints( + &self, + parent_session_id: &str, + child_session_id: &str, + ) { + if let Some(mut state) = self.edit_constraint_state(parent_session_id) { + state.mark_current_state_as_fork_baseline(); + self.edit_constraints_store + .insert(child_session_id.to_string(), state.clone()); + if self.should_persist_session_id(child_session_id) { + if let Err(error) = self + .merge_session_custom_metadata( + child_session_id, + json!({ + crate::agentic::execution::edit_constraint_guard::EDIT_CONSTRAINT_METADATA_KEY: state, + }), + ) + .await + { + warn!( + "Failed to persist inherited edit constraint state: session_id={}, error={}", + child_session_id, error + ); + } + } + } + } + pub async fn rebuild_skill_agent_listing_baseline_to_latest(&self, session_id: &str) -> bool { let Some(turn_index) = self .sessions @@ -4000,6 +4198,8 @@ impl SessionManager { } let listing_baseline_rebuild_turn_index = Self::listing_baseline_rebuild_turn_index_from_metadata(session_metadata.as_ref()); + let restored_edit_constraint_state = + Self::edit_constraint_state_from_metadata(session_metadata.as_ref()); debug!( "Session restore phase completed: session_id={}, phase=load_metadata, duration_ms={}", session_id, @@ -4282,6 +4482,10 @@ impl SessionManager { .await; } } + if let Some(state) = restored_edit_constraint_state { + self.edit_constraints_store + .insert(session_id.to_string(), state); + } Ok((session, persisted_turns)) } @@ -4373,8 +4577,8 @@ impl SessionManager { self.prune_token_anchors_to_messages(session_id, &messages) .await; - let last_user_dialog_agent_type = if target_turn == 0 { - None + let (last_user_dialog_agent_type, surviving_dialog_turn_ids) = if target_turn == 0 { + (None, std::collections::HashSet::new()) } else { let kept_turns = surviving_turns .into_iter() @@ -4384,10 +4588,15 @@ impl SessionManager { .sessions .get(session_id) .map(|session| session.agent_type.clone()); - Self::derive_last_user_dialog_agent_type_from_turns( + let last_agent_type = Self::derive_last_user_dialog_agent_type_from_turns( &kept_turns, fallback_agent_type.as_deref(), - ) + ); + let turn_ids = kept_turns + .iter() + .map(|turn| turn.turn_id.clone()) + .collect::>(); + (last_agent_type, turn_ids) }; // 3) Truncate session turn list & persist @@ -4441,6 +4650,8 @@ impl SessionManager { } self.turn_skill_agent_snapshot_store .remove_from(session_id, target_turn); + self.rollback_edit_constraint_state_to_turns(session_id, &surviving_dialog_turn_ids) + .await; Ok(()) } @@ -6005,6 +6216,7 @@ impl SessionManager { let turn_skill_agent_snapshot_store = self.turn_skill_agent_snapshot_store.clone(); let skill_agent_baseline_override_snapshot_store = self.skill_agent_baseline_override_snapshot_store.clone(); + let edit_constraints_store = self.edit_constraints_store.clone(); let file_read_state_store = self.file_read_state_store.clone(); let evidence_ledger = self.evidence_ledger.clone(); @@ -6078,6 +6290,7 @@ impl SessionManager { file_read_state_store.as_ref(), evidence_ledger.as_ref(), ); + edit_constraints_store.remove(&candidate.session_id); } } } @@ -8414,6 +8627,12 @@ mod tests { #[tokio::test] async fn rollback_context_deletes_persisted_turns_from_target() { + use crate::agentic::execution::edit_constraint_guard::{ + ConstraintExtractionRecord, ConstraintMatcher, ConstraintOperationScope, + ConstraintRevocation, ConstraintSource, ExtractedConstraint, ExtractionStatus, + ModelExtractionStatus, + }; + let workspace = TestWorkspace::new(); let persistence_manager = Arc::new( PersistenceManager::new(workspace.path_manager()).expect("persistence manager"), @@ -8430,6 +8649,85 @@ mod tests { ) .await .expect("session should create"); + let test_constraint = ExtractedConstraint { + id: "deterministic:test_files".to_string(), + description: "do not modify tests".to_string(), + operation_scope: ConstraintOperationScope::All, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Deterministic, + source_text: Some("Do not modify tests.".to_string()), + }; + manager + .remember_edit_constraint_extraction( + &session.session_id, + ConstraintExtractionRecord { + message_sha256: "turn-0-hash".to_string(), + dialog_turn_id: Some("turn-0".to_string()), + status: ExtractionStatus::Extracted, + constraints: vec![test_constraint.clone()], + deterministic_constraint_count: 1, + model_attempts: 0, + active_constraint_ids: Vec::new(), + revocation_authorized: true, + model_status: ModelExtractionStatus::NotRun, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars: 20, + prompt_chars: 20, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: None, + }, + ) + .await; + manager + .remember_edit_constraint_agent_created_paths( + &session.session_id, + vec!["tests/kept-repro.rs".to_string()], + "turn-0", + ) + .await; + manager + .remember_edit_constraint_extraction( + &session.session_id, + ConstraintExtractionRecord { + message_sha256: "turn-1-hash".to_string(), + dialog_turn_id: Some("turn-1".to_string()), + status: ExtractionStatus::Extracted, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![test_constraint.id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: vec![ConstraintRevocation { + constraint_id: test_constraint.id.clone(), + description: "tests may now be modified".to_string(), + }], + revoked_constraint_ids: vec![test_constraint.id.clone()], + unmatched_revocation_ids: Vec::new(), + input_chars: 24, + prompt_chars: 24, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 2, + failure: None, + response_excerpt: None, + }, + ) + .await; + manager + .remember_edit_constraint_agent_created_paths( + &session.session_id, + vec!["tests/future-repro.rs".to_string()], + "turn-1", + ) + .await; for index in 0..3 { let mut turn = DialogTurnData::new( @@ -8505,6 +8803,17 @@ mod tests { .await .expect("snapshot load should succeed") .is_none()); + assert_eq!( + manager.edit_constraints(&session.session_id), + Some(vec![test_constraint.clone()]) + ); + assert_eq!( + manager + .edit_constraint_state(&session.session_id) + .expect("constraint state should remain cached") + .agent_created_paths, + vec!["tests/kept-repro.rs".to_string()] + ); manager.evict_loaded_session_for_test(&session.session_id); let restored = manager @@ -8530,6 +8839,13 @@ mod tests { .expect("metadata should load") .expect("metadata should exist"); assert_eq!(metadata.turn_count, 1); + let restored_state = SessionManager::edit_constraint_state_from_metadata(Some(&metadata)) + .expect("constraint metadata should restore"); + assert_eq!(restored_state.constraints, vec![test_constraint]); + assert_eq!( + restored_state.agent_created_paths, + vec!["tests/kept-repro.rs".to_string()] + ); } #[tokio::test] @@ -9568,6 +9884,229 @@ mod tests { ); } + #[tokio::test] + async fn edit_constraints_are_cached_and_inherited_by_forked_children() { + use crate::agentic::execution::edit_constraint_guard::{ + ConstraintExtractionRecord, ConstraintMatcher, ConstraintOperationScope, + ConstraintSource, ExtractedConstraint, ExtractionStatus, ModelExtractionStatus, + }; + + let workspace = TestWorkspace::new(); + let persistence_manager = + Arc::new(PersistenceManager::new(workspace.path_manager()).expect("persistence")); + let manager = test_manager(persistence_manager); + + // Uncached: distinct from "cached but empty". + assert_eq!(manager.edit_constraints("parent-session"), None); + + let constraints = vec![ExtractedConstraint { + id: "test-files".to_string(), + description: "don't modify test files".to_string(), + operation_scope: ConstraintOperationScope::All, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Legacy, + source_text: None, + }]; + manager + .remember_edit_constraint_extraction( + "parent-session", + ConstraintExtractionRecord { + message_sha256: "message-hash".to_string(), + dialog_turn_id: Some("turn-1".to_string()), + status: ExtractionStatus::Extracted, + constraints: constraints.clone(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: Vec::new(), + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: constraints.clone(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars: 10, + prompt_chars: 10, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: None, + }, + ) + .await; + assert_eq!( + manager.edit_constraints("parent-session"), + Some(constraints.clone()) + ); + manager + .remember_edit_constraint_agent_created_paths( + "parent-session", + vec!["tests/parent_repro.rs".to_string()], + "turn-1", + ) + .await; + + // A forked child with no prior extraction inherits the parent's list. + assert_eq!(manager.edit_constraints("child-session"), None); + manager + .seed_forked_edit_constraints("parent-session", "child-session") + .await; + assert_eq!( + manager.edit_constraints("child-session"), + Some(constraints.clone()) + ); + manager + .rollback_edit_constraint_state_to_turns( + "child-session", + &std::collections::HashSet::new(), + ) + .await; + let child_state = manager + .edit_constraint_state("child-session") + .expect("forked state after rollback"); + assert_eq!(child_state.constraints, constraints); + assert_eq!( + child_state.agent_created_paths, + vec!["tests/parent_repro.rs".to_string()] + ); + + // Seeding from a parent with no cached constraints is a no-op, not a panic. + manager + .seed_forked_edit_constraints("no-such-parent", "another-child") + .await; + assert_eq!(manager.edit_constraints("another-child"), None); + } + + #[tokio::test] + async fn edit_constraint_state_persists_across_session_restore() { + use crate::agentic::execution::edit_constraint_guard::{ + ConstraintExtractionRecord, ConstraintMatcher, ConstraintOperationScope, + ConstraintRevocation, ConstraintSource, ExtractedConstraint, ExtractionStatus, + ModelExtractionStatus, EDIT_CONSTRAINT_METADATA_KEY, + }; + + let workspace = TestWorkspace::new(); + let persistence_manager = + Arc::new(PersistenceManager::new(workspace.path_manager()).expect("persistence")); + let manager = test_manager(persistence_manager.clone()); + let session = manager + .create_session( + "Edit constraint persistence".to_string(), + "agentic".to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().to_string()), + ..Default::default() + }, + ) + .await + .expect("session should be created"); + let constraint = ExtractedConstraint { + id: "deterministic:test_files".to_string(), + description: "do not modify tests".to_string(), + operation_scope: ConstraintOperationScope::All, + matcher: ConstraintMatcher::TestFiles, + source: ConstraintSource::Deterministic, + source_text: Some("Do not modify tests.".to_string()), + }; + manager + .remember_edit_constraint_extraction( + &session.session_id, + ConstraintExtractionRecord { + message_sha256: "message-hash".to_string(), + dialog_turn_id: Some("turn-1".to_string()), + status: ExtractionStatus::Extracted, + constraints: vec![constraint.clone()], + deterministic_constraint_count: 1, + model_attempts: 0, + active_constraint_ids: Vec::new(), + revocation_authorized: true, + model_status: ModelExtractionStatus::NotRun, + model_constraints: Vec::new(), + model_revocations: Vec::new(), + revoked_constraint_ids: Vec::new(), + unmatched_revocation_ids: Vec::new(), + input_chars: 20, + prompt_chars: 20, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 1, + failure: None, + response_excerpt: None, + }, + ) + .await; + manager + .remember_edit_constraint_agent_created_paths( + &session.session_id, + vec!["tests/temporary-repro.rs".to_string()], + "turn-1", + ) + .await; + manager + .remember_edit_constraint_extraction( + &session.session_id, + ConstraintExtractionRecord { + message_sha256: "relaxation-hash".to_string(), + dialog_turn_id: Some("turn-2".to_string()), + status: ExtractionStatus::Extracted, + constraints: Vec::new(), + deterministic_constraint_count: 0, + model_attempts: 1, + active_constraint_ids: vec![constraint.id.clone()], + revocation_authorized: true, + model_status: ModelExtractionStatus::Parsed, + model_constraints: Vec::new(), + model_revocations: vec![ConstraintRevocation { + constraint_id: constraint.id.clone(), + description: "tests may be modified now".to_string(), + }], + revoked_constraint_ids: vec![constraint.id.clone()], + unmatched_revocation_ids: Vec::new(), + input_chars: 24, + prompt_chars: 24, + input_truncated: false, + latency_ms: 1, + extracted_at_ms: 2, + failure: None, + response_excerpt: None, + }, + ) + .await; + + let metadata = persistence_manager + .load_session_metadata(workspace.path(), &session.session_id) + .await + .expect("metadata load") + .expect("metadata should exist"); + assert!(metadata + .custom_metadata + .as_ref() + .and_then(|value| value.get(EDIT_CONSTRAINT_METADATA_KEY)) + .is_some()); + + let restored_manager = test_manager(persistence_manager); + restored_manager + .restore_session(workspace.path(), &session.session_id) + .await + .expect("session should restore"); + assert_eq!( + restored_manager.edit_constraints(&session.session_id), + Some(Vec::new()) + ); + let restored_state = restored_manager + .edit_constraint_state(&session.session_id) + .expect("constraint state should restore"); + assert_eq!(restored_state.extractions.len(), 2); + assert_eq!( + restored_state.extractions[1].revoked_constraint_ids, + vec![constraint.id] + ); + assert_eq!( + restored_state.agent_created_paths, + vec!["tests/temporary-repro.rs".to_string()] + ); + } + #[tokio::test] async fn seed_forked_skill_agent_listing_baselines_splits_prompt_and_diff_baselines() { let workspace = TestWorkspace::new(); diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/bash_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/bash_tool.rs index 472db3e199..803ac5abd2 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/bash_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/bash_tool.rs @@ -497,6 +497,15 @@ Usage notes: }; } + if let Some(rejection) = + crate::agentic::execution::edit_constraint_guard::check_bash_command( + context, + command.unwrap_or_default(), + ) + { + return rejection; + } + match Self::resolve_working_directory(input, context) { Ok(Some(resolved_dir)) => { match Self::is_existing_workspace_directory(context, &resolved_dir).await { diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/delete_file_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/delete_file_tool.rs index 7942795343..56b27386e4 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/delete_file_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/delete_file_tool.rs @@ -154,6 +154,28 @@ Important notes: }; } + let force = input + .get("force") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + let recursive = input + .get("recursive") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + let rejection = if recursive { + crate::agentic::execution::edit_constraint_guard::check_recursive_delete( + context, path_str, force, + ) + .await + } else { + crate::agentic::execution::edit_constraint_guard::check_delete( + context, "Delete", "delete", path_str, force, + ) + }; + if let Some(rejection) = rejection { + return rejection; + } + let resolved = match context.map(|ctx| ctx.resolve_tool_path(path_str)) { Some(Ok(value)) => value, Some(Err(err)) => { @@ -337,6 +359,21 @@ Important notes: stderr ))); } + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Delete", + if recursive { + "recursive_delete" + } else { + "delete" + }, + &resolved.logical_path, + ); + crate::agentic::execution::edit_constraint_guard::forget_agent_created_file( + context, + &resolved.logical_path, + ) + .await; let result_data = json!({ "success": true, @@ -371,6 +408,21 @@ Important notes: }); let result_text = self.render_result_for_assistant(&result_data); + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Delete", + if recursive { + "recursive_delete" + } else { + "delete" + }, + &resolved.logical_path, + ); + crate::agentic::execution::edit_constraint_guard::forget_agent_created_file( + context, + &resolved.logical_path, + ) + .await; Ok(vec![ToolResult::Result { data: result_data, @@ -379,3 +431,15 @@ Important notes: }]) } } + +#[cfg(test)] +mod tests { + use super::DeleteFileTool; + use crate::agentic::tools::framework::Tool; + + #[test] + fn schema_does_not_expose_force_override() { + let schema = DeleteFileTool::new().input_schema(); + assert!(schema["properties"].get("force").is_none()); + } +} diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/command.rs b/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/command.rs index 71456c321a..9428f3b0ce 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/command.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/command.rs @@ -30,7 +30,8 @@ use tokio::sync::mpsc; use tool_runtime::exec_command::{ exec_command_argv_for_shell, exec_command_background_output_status, exec_command_lifecycle_background_output_status, exec_command_lifecycle_status_name, - exec_command_noninteractive_env, exec_command_result_value, exec_command_run_input_from_input, + exec_command_noninteractive_env, exec_command_pipeline_failure_policy, + exec_command_result_value, exec_command_run_input_from_input, exec_command_run_input_validation_message, exec_command_shell_escape, exec_command_shell_invocation_for_model, fallback_remote_exec_shell, parse_remote_exec_shell_probe_output, remote_exec_login_shell_command, @@ -200,11 +201,21 @@ impl ExecCommandTool { shell: &RemoteShell, env_snapshot: Option<&RemoteEnvSnapshot>, ) -> String { - remote_exec_login_shell_command(workdir, cmd, &shell.path, env_snapshot) + remote_exec_login_shell_command( + workdir, + cmd, + &shell.path, + exec_command_shell_kind(&shell.shell_type), + env_snapshot, + ) } - fn remote_non_tty_control_wrapper(cmd: &str, shell_path: &str) -> String { - remote_exec_non_tty_control_wrapper(cmd, shell_path) + fn remote_non_tty_control_wrapper( + cmd: &str, + shell_path: &str, + shell_type: &ShellType, + ) -> String { + remote_exec_non_tty_control_wrapper(cmd, shell_path, exec_command_shell_kind(shell_type)) } fn remote_shell_metadata( @@ -212,6 +223,7 @@ impl ExecCommandTool { shell: &RemoteShell, env_snapshot_applied: bool, ) -> ExecCommandShellMetadata { + let shell_kind = exec_command_shell_kind(&shell.shell_type); ExecCommandShellMetadata { name: shell.shell_type.name().to_string(), kind: shell.shell_type.to_string(), @@ -220,8 +232,9 @@ impl ExecCommandTool { "`cd {} && env ... {} {} `", exec_command_shell_escape(workdir), exec_command_shell_escape(&shell.path), - remote_exec_shell_login_args().join(" ") + remote_exec_shell_login_args(&shell_kind).join(" ") ), + pipeline_failure_policy: exec_command_pipeline_failure_policy(&shell_kind).to_string(), remote_env_snapshot_applied: Some(env_snapshot_applied), } } @@ -234,11 +247,13 @@ impl ExecCommandTool { } fn shell_metadata_value(shell: &ResolvedLocalExecShell) -> ExecCommandShellMetadata { + let shell_kind = exec_command_shell_kind(&shell.shell_type); ExecCommandShellMetadata { name: shell.display_name.clone(), kind: shell.shell_type.to_string(), path: shell.path.to_string_lossy().to_string(), invocation: Self::shell_invocation_for_model(&shell.path, &shell.shell_type), + pipeline_failure_policy: exec_command_pipeline_failure_policy(&shell_kind).to_string(), remote_env_snapshot_applied: None, } } @@ -408,7 +423,7 @@ impl ExecCommandTool { let command_body = if tty { cmd.to_string() } else { - Self::remote_non_tty_control_wrapper(cmd, &shell.path) + Self::remote_non_tty_control_wrapper(cmd, &shell.path, &shell.shell_type) }; let command = Self::remote_login_shell_command( &workdir, @@ -540,6 +555,7 @@ Waiting and continuation: Output: - Output is only what was produced during this tool call's wait window. +- Bash, Zsh, and Ksh run with `pipefail`, so a pipeline reports a non-zero exit code when any stage fails unless the command explicitly handles that failure. - In non-TTY mode, stdout and stderr ordering is not guaranteed; use tty=true or redirect stderr with 2>&1 when terminal ordering matters."# .to_string()) } @@ -617,7 +633,7 @@ Output: async fn validate_input( &self, input: &Value, - _context: Option<&ToolUseContext>, + context: Option<&ToolUseContext>, ) -> ValidationResult { if let Some(message) = exec_command_run_input_validation_message(input) { return ValidationResult { @@ -627,6 +643,17 @@ Output: meta: None, }; } + if let (Some(context), Some(parsed)) = + (context, exec_command_run_input_from_input(input)) + { + if let Some(rejection) = + crate::agentic::execution::edit_constraint_guard::check_bash_command( + context, parsed.cmd, + ) + { + return rejection; + } + } ValidationResult { result: true, message: None, @@ -907,7 +934,7 @@ mod tests { assert!(command.starts_with("cd '/home/me/project' && env ")); assert!(command.contains("'BITFUN_NONINTERACTIVE=1'")); - assert!(command.ends_with(" '/bin/bash' -lc 'printf '\\''hi'\\'''")); + assert!(command.ends_with(" '/bin/bash' -o pipefail -lc 'printf '\\''hi'\\'''")); } #[test] @@ -951,16 +978,19 @@ mod tests { ); assert!(command.contains("'PATH=/home/me/.nvm/bin:/usr/bin'")); - assert!(command.ends_with(" '/bin/bash' -lc 'node --version'")); + assert!(command.ends_with(" '/bin/bash' -o pipefail -lc 'node --version'")); assert!(!command.contains(" -lic ")); } #[test] fn remote_non_tty_control_wrapper_cleans_process_group_after_interrupt_grace() { - let wrapper = - ExecCommandTool::remote_non_tty_control_wrapper("python3 -c 'print(1)'", "/bin/bash"); + let wrapper = ExecCommandTool::remote_non_tty_control_wrapper( + "python3 -c 'print(1)'", + "/bin/bash", + &ShellType::Bash, + ); - assert!(wrapper.contains("setsid \"$__bitfun_shell\" -lc \"$__bitfun_cmd\" &")); + assert!(wrapper.contains("setsid \"$__bitfun_shell\" -o pipefail -lc \"$__bitfun_cmd\" &")); assert!(wrapper.contains("trap '__bitfun_stop INT 130 2' INT")); assert!(wrapper.contains("trap '__bitfun_stop KILL 137 0' TERM")); assert!(wrapper.contains("__bitfun_grace=${3:-2}")); @@ -1011,8 +1041,15 @@ mod tests { } #[test] - fn remote_shell_login_args_use_login_without_interactive_startup() { - assert_eq!(remote_exec_shell_login_args(), &["-lc"]); + fn remote_shell_args_enable_pipefail_without_interactive_startup() { + assert_eq!( + remote_exec_shell_login_args(&super::exec_command_shell_kind(&ShellType::Bash)), + &["-o", "pipefail", "-lc"] + ); + assert_eq!( + remote_exec_shell_login_args(&super::exec_command_shell_kind(&ShellType::Sh)), + &["-lc"] + ); } #[tokio::test] diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/stdin.rs b/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/stdin.rs index 8efaee5ef2..181ead569c 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/stdin.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/exec_command/stdin.rs @@ -162,7 +162,7 @@ Output is only what was produced during this tool call's wait window."# async fn validate_input( &self, input: &Value, - _context: Option<&ToolUseContext>, + context: Option<&ToolUseContext>, ) -> ValidationResult { if let Some(message) = write_stdin_input_validation_message(input) { return ValidationResult { @@ -172,6 +172,14 @@ Output is only what was produced during this tool call's wait window."# meta: None, }; } + if let (Some(context), Some(chars)) = (context, input.get("chars").and_then(Value::as_str)) + { + if let Some(rejection) = + crate::agentic::execution::edit_constraint_guard::check_bash_command(context, chars) + { + return rejection; + } + } ValidationResult { result: true, message: None, diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs index 65e8aeaa01..20db3ea1da 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/file_edit_tool.rs @@ -205,6 +205,16 @@ impl Tool for FileEditTool { }; } + let force = input + .get("force") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + if let Some(rejection) = crate::agentic::execution::edit_constraint_guard::check_edit( + context, "Edit", "edit", file_path, force, + ) { + return rejection; + } + let old_string = input .get("old_string") .and_then(|v| v.as_str()) @@ -361,6 +371,12 @@ impl Tool for FileEditTool { &edit_result.new_content, timestamp_ms, ); + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Edit", + "edit", + &resolved.logical_path, + ); let result = ToolResult::Result { data: json!({ @@ -410,6 +426,12 @@ impl Tool for FileEditTool { &edit_result.new_content, timestamp_ms, ); + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Edit", + "edit", + &resolved.logical_path, + ); let result = ToolResult::Result { data: json!({ @@ -489,6 +511,7 @@ mod tests { .get("old_string") .and_then(|value| value.get("minLength")) .is_none()); + assert!(properties.get("force").is_none()); } #[test] diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/file_read_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/file_read_tool.rs index d4b13b531c..19d08c60aa 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/file_read_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/file_read_tool.rs @@ -265,6 +265,7 @@ Usage: - This tool can only read files, not directories. - You can call multiple tools in a single response. It is always better to speculatively read multiple potentially useful files in parallel. - Avoid tiny repeated slices (e.g. 30-100 line chunks). If you need more context, read a larger window that covers the whole block you will edit. +- Do not use `limit` with a small value (e.g. < 50) to probe file type or structure. Source files typically begin with copyright headers — a probe read returns no useful code. "#, self.default_max_lines_to_read, self.max_line_chars, self.max_total_chars )) diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/file_write_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/file_write_tool.rs index 9ce45c77ef..66c930e918 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/file_write_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/file_write_tool.rs @@ -226,7 +226,7 @@ impl FileWriteTool { .as_object() .into_iter() .flat_map(|object| object.keys()) - .filter(|name| name.as_str() != "payload") + .filter(|name| !matches!(name.as_str(), "payload" | "force")) .cloned() .collect::>(); parameter_names.sort(); @@ -419,6 +419,21 @@ impl Tool for FileWriteTool { None }; + if let ParsedWritePayload::Target { file_path, .. } = &parsed { + let force_requested = input.get("force").and_then(Value::as_bool).unwrap_or(false); + if let Some(rejection) = crate::agentic::execution::edit_constraint_guard::check_write( + context, + "Write", + "write", + file_path, + force_requested, + ) + .await + { + return rejection; + } + } + if let Some(ctx) = context { let preflight_error = match &parsed { ParsedWritePayload::Target { file_path, .. } => { @@ -541,6 +556,19 @@ impl Tool for FileWriteTool { .map_err(|e| BitFunError::tool(format!("Failed to write file: {}", e)))?; let timestamp_ms = file_mutation_timestamp_ms(context, &resolved).await; update_file_read_state_after_mutation(context, &resolved, &content, timestamp_ms); + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Write", + "write", + &resolved.logical_path, + ); + if !file_already_exists { + crate::agentic::execution::edit_constraint_guard::remember_agent_created_file( + context, + &resolved.logical_path, + ) + .await; + } let result = Self::write_success_result( &resolved.logical_path, @@ -563,6 +591,19 @@ impl Tool for FileWriteTool { let timestamp_ms = file_mutation_timestamp_ms(context, &resolved).await; update_file_read_state_after_mutation(context, &resolved, &content, timestamp_ms); + crate::agentic::execution::edit_constraint_guard::record_mutation_applied( + context, + "Write", + "write", + &resolved.logical_path, + ); + if !file_already_exists { + crate::agentic::execution::edit_constraint_guard::remember_agent_created_file( + context, + &resolved.logical_path, + ) + .await; + } let result = Self::write_success_result( &resolved.logical_path, @@ -742,6 +783,30 @@ mod tests { assert_eq!(data["message"], expected); } + #[tokio::test] + async fn validate_input_rejects_stale_force_without_runtime_context() { + let tool = FileWriteTool::new(); + let validation = tool + .validate_input( + &json!({ + "payload": "+++ new.txt\nalpha", + "force": true + }), + None, + ) + .await; + + assert!(!validation.result); + assert_eq!(validation.error_code, Some(403)); + assert_eq!( + validation + .meta + .as_ref() + .and_then(|meta| meta["guard_decision"].as_str()), + Some("force_denied") + ); + } + #[tokio::test] async fn call_impl_accepts_path_only_for_empty_file() { let root = std::env::temp_dir().join(format!("bitfun-write-test-{}", uuid::Uuid::new_v4())); diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/git_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/git_tool.rs index af8fa712c9..bb44e39cb7 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/git_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/git_tool.rs @@ -1250,7 +1250,7 @@ When creating commits, use this format for the commit message: async fn validate_input( &self, input: &Value, - _context: Option<&ToolUseContext>, + context: Option<&ToolUseContext>, ) -> ValidationResult { let input = &Self::normalize_git_input(input.clone()); @@ -1287,6 +1287,16 @@ When creating commits, use this format for the commit message: // Get arguments (if any) let args = input.get("args").and_then(|v| v.as_str()).unwrap_or(""); + if let Some(context) = context { + if let Some(rejection) = + crate::agentic::execution::edit_constraint_guard::check_git_command( + context, operation, args, + ) + { + return rejection; + } + } + // Security check: prohibit interactive operations. Match whole tokens // only so text like "fix-ui" or a quoted message cannot false-trip. let arg_tokens = Self::tokenize_args(args); diff --git a/src/crates/execution/tool-execution/src/exec_command.rs b/src/crates/execution/tool-execution/src/exec_command.rs index f4f938278e..63229e5195 100644 --- a/src/crates/execution/tool-execution/src/exec_command.rs +++ b/src/crates/execution/tool-execution/src/exec_command.rs @@ -111,6 +111,7 @@ pub struct ExecCommandShellMetadata { pub kind: String, pub path: String, pub invocation: String, + pub pipeline_failure_policy: String, pub remote_env_snapshot_applied: Option, } @@ -185,6 +186,10 @@ impl ExecCommandShellKind { | Self::Custom(_) ) } + + fn supports_pipefail(&self) -> bool { + matches!(self, Self::Bash | Self::Zsh | Self::Ksh) + } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -293,7 +298,14 @@ pub fn exec_command_argv_for_shell( ) -> Vec { let shell = shell_path.into(); if shell_kind.uses_posix_invocation() { - return vec![shell, "-lc".to_string(), cmd.to_string()]; + let mut argv = vec![shell]; + argv.extend( + exec_command_posix_shell_args(&shell_kind) + .iter() + .map(|arg| (*arg).to_string()), + ); + argv.push(cmd.to_string()); + return argv; } match shell_kind { @@ -321,7 +333,10 @@ pub fn exec_command_shell_invocation_for_model( shell_kind: ExecCommandShellKind, ) -> String { if shell_kind.uses_posix_invocation() { - return format!("`{shell_path} -lc `"); + return format!( + "`{shell_path} {} `", + exec_command_posix_shell_args(&shell_kind).join(" ") + ); } match shell_kind { @@ -337,10 +352,11 @@ pub fn remote_exec_login_shell_command( workdir: &str, cmd: &str, shell_path: &str, + shell_kind: ExecCommandShellKind, env_snapshot: Option<&ExecCommandRemoteEnvSnapshot>, ) -> String { let env_words = remote_command_env_words(merged_remote_exec_env(env_snapshot)); - let shell_args = remote_exec_shell_login_args().join(" "); + let shell_args = remote_exec_shell_login_args(&shell_kind).join(" "); format!( "cd {} && env {} {} {} {}", @@ -352,16 +368,21 @@ pub fn remote_exec_login_shell_command( ) } -pub fn remote_exec_non_tty_control_wrapper(cmd: &str, shell_path: &str) -> String { +pub fn remote_exec_non_tty_control_wrapper( + cmd: &str, + shell_path: &str, + shell_kind: ExecCommandShellKind, +) -> String { let escaped_shell = exec_command_shell_escape(shell_path); let escaped_cmd = exec_command_shell_escape(cmd); + let shell_args = remote_exec_shell_login_args(&shell_kind).join(" "); format!( r#"__bitfun_shell={escaped_shell} __bitfun_cmd={escaped_cmd} if command -v setsid >/dev/null 2>&1; then - setsid "$__bitfun_shell" -lc "$__bitfun_cmd" & + setsid "$__bitfun_shell" {shell_args} "$__bitfun_cmd" & else - "$__bitfun_shell" -lc "$__bitfun_cmd" & + "$__bitfun_shell" {shell_args} "$__bitfun_cmd" & fi __bitfun_child=$! __bitfun_pgid=$__bitfun_child @@ -415,8 +436,24 @@ pub fn fallback_remote_exec_shell() -> ExecCommandRemoteShell { } } -pub fn remote_exec_shell_login_args() -> &'static [&'static str] { - &["-lc"] +pub fn exec_command_posix_shell_args(shell_kind: &ExecCommandShellKind) -> &'static [&'static str] { + if shell_kind.supports_pipefail() { + &["-o", "pipefail", "-lc"] + } else { + &["-lc"] + } +} + +pub fn exec_command_pipeline_failure_policy(shell_kind: &ExecCommandShellKind) -> &'static str { + if shell_kind.supports_pipefail() { + "pipefail" + } else { + "last_command" + } +} + +pub fn remote_exec_shell_login_args(shell_kind: &ExecCommandShellKind) -> &'static [&'static str] { + exec_command_posix_shell_args(shell_kind) } pub fn remote_exec_env_snapshot_capture_policy() -> ExecCommandRemoteEnvSnapshotCapturePolicy { @@ -635,6 +672,7 @@ pub fn exec_command_shell_metadata_value(metadata: ExecCommandShellMetadata) -> "type": metadata.kind, "path": metadata.path, "invocation": metadata.invocation, + "pipeline_failure_policy": metadata.pipeline_failure_policy, }); if let Some(remote_env_snapshot_applied) = metadata.remote_env_snapshot_applied { value["remote_env_snapshot_applied"] = json!(remote_env_snapshot_applied); @@ -1083,6 +1121,61 @@ mod tests { assert_eq!(prefixed[2], script); } + #[test] + fn supported_posix_shells_enable_pipefail() { + for shell_kind in [ + ExecCommandShellKind::Bash, + ExecCommandShellKind::Zsh, + ExecCommandShellKind::Ksh, + ] { + let argv = + exec_command_argv_for_shell("/bin/shell", shell_kind.clone(), "false | tail -n 1"); + assert_eq!( + argv, + ["/bin/shell", "-o", "pipefail", "-lc", "false | tail -n 1"] + ); + assert_eq!( + exec_command_pipeline_failure_policy(&shell_kind), + "pipefail" + ); + } + } + + #[test] + fn unsupported_posix_shells_keep_last_command_pipeline_status() { + let argv = + exec_command_argv_for_shell("/bin/sh", ExecCommandShellKind::Sh, "false | tail -n 1"); + + assert_eq!(argv, ["/bin/sh", "-lc", "false | tail -n 1"]); + assert_eq!( + exec_command_pipeline_failure_policy(&ExecCommandShellKind::Sh), + "last_command" + ); + } + + #[cfg(unix)] + #[test] + fn bash_pipefail_preserves_pipeline_success_and_failure() { + for (command, expected_success) in [ + ("printf 'ok\\n' | tail -n 1", true), + ("false | tail -n 1", false), + ("true | false | tail -n 1", false), + ] { + let argv = + exec_command_argv_for_shell("/bin/bash", ExecCommandShellKind::Bash, command); + let status = std::process::Command::new(&argv[0]) + .args(&argv[1..]) + .status() + .expect("bash pipeline should execute"); + + assert_eq!( + status.success(), + expected_success, + "unexpected status for {command:?}: {status:?}" + ); + } + } + #[test] fn remote_login_shell_command_applies_snapshot_then_tool_env() { let snapshot = ExecCommandRemoteEnvSnapshot { @@ -1096,6 +1189,7 @@ mod tests { "/home/me/project", "node --version", "/bin/bash", + ExecCommandShellKind::Bash, Some(&snapshot), ); @@ -1103,14 +1197,18 @@ mod tests { assert!(command.contains("'PATH=/home/me/.nvm/bin:/usr/bin'")); assert!(command.contains("'TERM=dumb'")); assert!(!command.contains("'TERM=xterm-256color'")); - assert!(command.ends_with(" '/bin/bash' -lc 'node --version'")); + assert!(command.ends_with(" '/bin/bash' -o pipefail -lc 'node --version'")); } #[test] fn remote_non_tty_control_wrapper_preserves_interrupt_cleanup_contract() { - let wrapper = remote_exec_non_tty_control_wrapper("python3 -c 'print(1)'", "/bin/bash"); + let wrapper = remote_exec_non_tty_control_wrapper( + "python3 -c 'print(1)'", + "/bin/bash", + ExecCommandShellKind::Bash, + ); - assert!(wrapper.contains("setsid \"$__bitfun_shell\" -lc \"$__bitfun_cmd\" &")); + assert!(wrapper.contains("setsid \"$__bitfun_shell\" -o pipefail -lc \"$__bitfun_cmd\" &")); assert!(wrapper.contains("trap '__bitfun_stop INT 130 2' INT")); assert!(wrapper.contains("trap '__bitfun_stop KILL 137 0' TERM")); assert!(wrapper.contains("__bitfun_grace=${3:-2}")); @@ -1316,6 +1414,7 @@ mod tests { kind: "powershell_core".to_string(), path: "pwsh".to_string(), invocation: "`pwsh -Command `".to_string(), + pipeline_failure_policy: "last_command".to_string(), remote_env_snapshot_applied: None, }, }); @@ -1339,7 +1438,8 @@ mod tests { "name": "PowerShell Core", "type": "powershell_core", "path": "pwsh", - "invocation": "`pwsh -Command `" + "invocation": "`pwsh -Command `", + "pipeline_failure_policy": "last_command" } }) ); diff --git a/src/web-ui/src/app/components/NavPanel/sections/workspaces/WorkspaceItem.tsx b/src/web-ui/src/app/components/NavPanel/sections/workspaces/WorkspaceItem.tsx index d62aef68aa..0117a9a9b8 100644 --- a/src/web-ui/src/app/components/NavPanel/sections/workspaces/WorkspaceItem.tsx +++ b/src/web-ui/src/app/components/NavPanel/sections/workspaces/WorkspaceItem.tsx @@ -52,7 +52,13 @@ const WorkspaceSessionBatchModal = lazy(() => import('./WorkspaceSessionBatchMod const ScheduledJobsModal = lazy(() => import('@/app/components/scheduled-jobs/ScheduledJobsModal')); const MAX_WORKSPACE_NAME_CHARS = 80; -const WORKSPACE_NAME_CONTROL_CHARACTERS = /[\u0000-\u001f\u007f]/; + +function containsWorkspaceNameControlCharacter(value: string): boolean { + return Array.from(value).some((character) => { + const codePoint = character.codePointAt(0); + return codePoint !== undefined && (codePoint <= 0x1f || codePoint === 0x7f); + }); +} interface WorkspaceItemProps { workspace: WorkspaceInfo; @@ -507,7 +513,7 @@ const WorkspaceItem: React.FC = ({ if (!normalizedName) { return t('nav.workspaces.renameDialog.validation.required'); } - if (WORKSPACE_NAME_CONTROL_CHARACTERS.test(normalizedName)) { + if (containsWorkspaceNameControlCharacter(normalizedName)) { return t('nav.workspaces.renameDialog.validation.invalidCharacters'); } if (Array.from(normalizedName).length > MAX_WORKSPACE_NAME_CHARS) { diff --git a/src/web-ui/src/flow_chat/services/FlowChatManager.test.ts b/src/web-ui/src/flow_chat/services/FlowChatManager.test.ts index e15dd1e5cb..da7f45d7d9 100644 --- a/src/web-ui/src/flow_chat/services/FlowChatManager.test.ts +++ b/src/web-ui/src/flow_chat/services/FlowChatManager.test.ts @@ -60,6 +60,10 @@ vi.mock('./EventBatcher', () => ({ }, })); +vi.mock('./flow-chat-manager/PeerSessionRefreshModule', () => ({ + installPeerSessionRefresh: vi.fn(() => () => {}), +})); + vi.mock('./flow-chat-manager', () => ({ saveAllInProgressTurns: vi.fn(), immediateSaveDialogTurn: vi.fn(), diff --git a/src/web-ui/src/flow_chat/services/flow-chat-manager/PeerSessionRefreshModule.ts b/src/web-ui/src/flow_chat/services/flow-chat-manager/PeerSessionRefreshModule.ts index 60a0144d20..6b71e32197 100644 --- a/src/web-ui/src/flow_chat/services/flow-chat-manager/PeerSessionRefreshModule.ts +++ b/src/web-ui/src/flow_chat/services/flow-chat-manager/PeerSessionRefreshModule.ts @@ -120,9 +120,8 @@ export function installPeerSessionRefresh(context: FlowChatContext): () => void let inFlight = false; let queued = false; let immediateTimer: ReturnType | null = null; - let scheduleRefresh: RefreshRequester = () => {}; - const runRefresh = async (requestedSessionId?: string): Promise => { + async function runRefresh(requestedSessionId?: string): Promise { if (disposed || inFlight || !isPeerDeviceModeActive()) { if (inFlight) { queued = true; @@ -206,9 +205,9 @@ export function installPeerSessionRefresh(context: FlowChatContext): () => void scheduleRefresh(); } } - }; + } - scheduleRefresh = (sessionId) => { + function scheduleRefresh(sessionId?: string): void { if (disposed) { return; } @@ -219,7 +218,7 @@ export function installPeerSessionRefresh(context: FlowChatContext): () => void immediateTimer = null; void runRefresh(sessionId); }, 0); - }; + } installedRefreshRequester = scheduleRefresh;