From 7c90f21309e0ee8332c2964ad6cd56c1a864411d Mon Sep 17 00:00:00 2001 From: wsp Date: Mon, 7 Sep 2026 20:24:59 +0800 Subject: [PATCH] fix(agent-tools): remove read freshness gating Remove session-scoped Read freshness checks from Edit and Write, including state recording, mutation backfills, and obsolete contracts. Keep prompt guidance requiring agents to read files before editing. The gate cannot correct an inaccurate old_string. It adds rejection conditions beyond content matching: changes outside the target block can invalidate an otherwise applicable edit, while rollback or context replacement clears cached reads. Since the state is memory-only, restarting also loses Read records even when the restored conversation still contains the relevant file content. This follows established approaches in Codex and OpenCode: - Codex apply_patch reads current file content and matches patch context without requiring session-scoped prior-Read credentials. - OpenCode previously tracked read timestamps, mtime, and size per session and path, rejecting edits and overwrites without a prior Read or after metadata changes. Commit 76a141090, "chore: delete filetime module (#22999)", removed that module, its Read/Edit/Write integration, the disable-check flag, and associated tests. - Current OpenCode edit and apply_patch match against current content; write likewise has no runtime prior-Read gate. Preserve Deep Review range receipts in a separate store, including their session cleanup behavior. Improve old_string mismatch errors to mention external changes or incorrectly copied or generated text, retain nearby-content diagnostics, and direct agents to Read the current target region before correcting the edit and retrying. --- .../rules/source/forbidden-rules.mjs | 31 +- .../rules/source/public-api-rules.mjs | 2 +- .../rules/source/required-rules.mjs | 59 +- scripts/core-boundaries/self-test.mjs | 36 +- .../src/agentic/session/file_read_state.rs | 4 - .../assembly/core/src/agentic/session/mod.rs | 4 +- .../agentic/session/review_read_receipt.rs | 5 + .../src/agentic/session/session_manager.rs | 53 +- .../agentic/tools/file_read_state_runtime.rs | 452 ----------- .../tools/implementations/file_edit_tool.rs | 93 +-- .../tools/implementations/file_read_tool.rs | 49 +- .../tools/implementations/file_write_tool.rs | 98 +-- .../assembly/core/src/agentic/tools/mod.rs | 2 +- .../tools/review_read_receipt_runtime.rs | 181 +++++ src/crates/execution/agent-runtime/AGENTS.md | 2 +- .../agent-runtime/src/file_read_state.rs | 734 ------------------ src/crates/execution/agent-runtime/src/lib.rs | 4 +- .../agent-runtime/src/review_read_receipt.rs | 213 +++++ src/crates/execution/tool-contracts/AGENTS.md | 3 +- .../tool-contracts/src/file_read_freshness.rs | 53 -- .../execution/tool-contracts/src/lib.rs | 5 - .../tool-contracts/tests/tool_contracts.rs | 96 --- .../tool-execution/src/fs/edit_file.rs | 17 +- 23 files changed, 503 insertions(+), 1693 deletions(-) delete mode 100644 src/crates/assembly/core/src/agentic/session/file_read_state.rs create mode 100644 src/crates/assembly/core/src/agentic/session/review_read_receipt.rs delete mode 100644 src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs create mode 100644 src/crates/assembly/core/src/agentic/tools/review_read_receipt_runtime.rs delete mode 100644 src/crates/execution/agent-runtime/src/file_read_state.rs create mode 100644 src/crates/execution/agent-runtime/src/review_read_receipt.rs delete mode 100644 src/crates/execution/tool-contracts/src/file_read_freshness.rs diff --git a/scripts/core-boundaries/rules/source/forbidden-rules.mjs b/scripts/core-boundaries/rules/source/forbidden-rules.mjs index c005cb7764..d7c518d177 100644 --- a/scripts/core-boundaries/rules/source/forbidden-rules.mjs +++ b/scripts/core-boundaries/rules/source/forbidden-rules.mjs @@ -1665,22 +1665,27 @@ export const forbiddenContentRules = [ ], }, { - path: 'src/crates/assembly/core/src/agentic/session/file_read_state.rs', + path: 'src/crates/assembly/core/src/agentic/session/review_read_receipt.rs', patterns: [ { - regex: /\bpub struct FileReadState\b/, + regex: /\bpub struct FileRevision\b/, message: - 'core file_read_state must not own file-read state DTOs; use openbitfun-agent-runtime file_read_state', + 'core review_read_receipt must not own file revision DTOs; use openbitfun-agent-runtime review_read_receipt', }, { - regex: /\bpub struct FileReadStateStore\b/, + regex: /\bpub struct ReviewReadCoverage\b/, message: - 'core file_read_state must not own in-memory file-read state store; use openbitfun-agent-runtime file_read_state', + 'core review_read_receipt must not own review coverage DTOs; use openbitfun-agent-runtime review_read_receipt', + }, + { + regex: /\bpub struct ReviewReadReceiptStore\b/, + message: + 'core review_read_receipt must not own the receipt store; use openbitfun-agent-runtime review_read_receipt', }, { regex: /\bDashMap\b/, message: - 'core file_read_state must not own file-read state storage maps; use openbitfun-agent-runtime file_read_state', + 'core review_read_receipt must not own receipt storage maps; use openbitfun-agent-runtime review_read_receipt', }, ], }, @@ -2715,12 +2720,12 @@ export const forbiddenContentRules = [ ], }, { - path: 'src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs', + path: 'src/crates/assembly/core/src/agentic/tools/review_read_receipt_runtime.rs', patterns: [ { regex: /framework::(?:\{[^}]*\bToolUseContext\b[^}]*\}|\bToolUseContext\b)/, message: - 'file read-state runtime must import ToolUseContext from tool_context_runtime, not the framework re-export', + 'review read receipt runtime must import ToolUseContext from tool_context_runtime, not the framework re-export', }, ], }, @@ -3047,16 +3052,6 @@ export const forbiddenContentRules = [ }, ], }, - { - path: 'src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs', - patterns: [ - { - regex: /\bnormalize_string\b/, - message: - 'core file read-state runtime must delegate pure freshness normalization to openbitfun-agent-tools', - }, - ], - }, { path: 'src/crates/assembly/core/src/agentic/tools/tool_result_storage.rs', patterns: [ diff --git a/scripts/core-boundaries/rules/source/public-api-rules.mjs b/scripts/core-boundaries/rules/source/public-api-rules.mjs index a95725a776..369e57a966 100644 --- a/scripts/core-boundaries/rules/source/public-api-rules.mjs +++ b/scripts/core-boundaries/rules/source/public-api-rules.mjs @@ -30,7 +30,7 @@ export const agentRuntimeRootPublicModules = [ 'event_source', 'events', 'evidence_ledger', - 'file_read_state', + 'review_read_receipt', 'native_hooks', 'output_surface', 'permission', diff --git a/scripts/core-boundaries/rules/source/required-rules.mjs b/scripts/core-boundaries/rules/source/required-rules.mjs index dfa82f0157..0bd032aa48 100644 --- a/scripts/core-boundaries/rules/source/required-rules.mjs +++ b/scripts/core-boundaries/rules/source/required-rules.mjs @@ -975,29 +975,29 @@ export const requiredContentRules = [ ], }, { - path: 'src/crates/execution/agent-runtime/src/file_read_state.rs', + path: 'src/crates/execution/agent-runtime/src/review_read_receipt.rs', reason: - 'agent-runtime must own provider-neutral file-read state facts and session-scoped in-memory store', + 'agent-runtime must own provider-neutral code-review read receipts and their session-scoped in-memory store', patterns: [ { - regex: /\bpub struct FileReadState\b/, - message: 'missing agent-runtime file-read state DTO', + regex: /\bpub struct FileRevision\b/, + message: 'missing agent-runtime file revision DTO', }, { - regex: /\bpub fn is_full_file_read\b/, - message: 'missing agent-runtime file-read completeness policy', + regex: /\bpub struct ReviewReadCoverage\b/, + message: 'missing agent-runtime review read coverage DTO', }, { - regex: /\bpub struct FileReadStateStore\b/, - message: 'missing agent-runtime file-read state store', + regex: /\bpub struct ReviewReadReceiptStore\b/, + message: 'missing agent-runtime review read receipt store', }, { - regex: /\bfile_read_state_accepts_nonempty_whole_file\b/, - message: 'missing agent-runtime file-read completeness regression', + regex: /\breview_read_receipt_store_scopes_entries_by_session\b/, + message: 'missing review read receipt session scoping regression', }, { - regex: /\bfile_read_state_store_scopes_entries_by_session\b/, - message: 'missing agent-runtime file-read state session scoping regression', + regex: /\breview_read_receipt_covers_only_previously_returned_lines\b/, + message: 'missing review read receipt coverage regression', }, ], }, @@ -2853,14 +2853,17 @@ export const requiredContentRules = [ ], }, { - path: 'src/crates/assembly/core/src/agentic/session/file_read_state.rs', + path: 'src/crates/assembly/core/src/agentic/session/review_read_receipt.rs', reason: - 'core file_read_state path must stay a compatibility facade over agent-runtime', + 'core review_read_receipt path must stay a compatibility facade over agent-runtime', patterns: [ { - regex: - /pub use openbitfun_agent_runtime::file_read_state::\{FileReadState, FileReadStateStore\};/, - message: 'missing agent-runtime file-read state compatibility re-export', + regex: /openbitfun_agent_runtime::review_read_receipt::\{/, + message: 'missing agent-runtime review read receipt compatibility re-export', + }, + { + regex: /\bReviewReadReceiptStore\b/, + message: 'missing review read receipt store compatibility re-export', }, ], }, @@ -5629,28 +5632,6 @@ export const requiredContentRules = [ }, ], }, - { - path: 'src/crates/execution/tool-contracts/src/file_read_freshness.rs', - reason: 'agent-tools owns pure file-read freshness policy for Read/Edit/Write guardrails', - patterns: [ - { - regex: /\bpub struct FileReadFreshnessFacts\b/, - message: 'missing file-read freshness facts contract', - }, - { - regex: /\bpub fn normalize_tool_file_content\b/, - message: 'missing provider-neutral file content normalization helper', - }, - { - regex: /\bpub fn file_read_facts_content_matches\b/, - message: 'missing file-read content equivalence helper', - }, - { - regex: /\bpub fn file_read_facts_are_fresh\b/, - message: 'missing file-read freshness policy helper', - }, - ], - }, { path: 'src/crates/execution/tool-contracts/src/tool_result_storage.rs', reason: diff --git a/scripts/core-boundaries/self-test.mjs b/scripts/core-boundaries/self-test.mjs index 0667091eae..eb9c74642b 100644 --- a/scripts/core-boundaries/self-test.mjs +++ b/scripts/core-boundaries/self-test.mjs @@ -2368,12 +2368,17 @@ export function runManifestParserSelfTest({ ) { throw new Error('agentic system boundary rule must forbid terminal provider construction'); } - const coreFileReadStateRuleText = forbiddenRuleTextForPath( - 'src/crates/assembly/core/src/agentic/session/file_read_state.rs', + const coreReviewReadReceiptRuleText = forbiddenRuleTextForPath( + 'src/crates/assembly/core/src/agentic/session/review_read_receipt.rs', ); - for (const contract of ['FileReadState', 'FileReadStateStore', 'DashMap']) { - if (!coreFileReadStateRuleText.includes(contract)) { - throw new Error(`core file_read_state boundary rule must forbid ${contract}`); + for (const contract of [ + 'FileRevision', + 'ReviewReadCoverage', + 'ReviewReadReceiptStore', + 'DashMap', + ]) { + if (!coreReviewReadReceiptRuleText.includes(contract)) { + throw new Error(`core review_read_receipt boundary rule must forbid ${contract}`); } } const coreEvidenceLedgerRuleText = forbiddenRuleTextForPath( @@ -3180,13 +3185,13 @@ export function runManifestParserSelfTest({ ], }, { - path: 'src/crates/execution/agent-runtime/src/file_read_state.rs', + path: 'src/crates/execution/agent-runtime/src/review_read_receipt.rs', contracts: [ - 'FileReadState', - 'is_full_file_read', - 'FileReadStateStore', - 'file_read_state_accepts_nonempty_whole_file', - 'file_read_state_store_scopes_entries_by_session', + 'FileRevision', + 'ReviewReadCoverage', + 'ReviewReadReceiptStore', + 'review_read_receipt_store_scopes_entries_by_session', + 'review_read_receipt_covers_only_previously_returned_lines', ], }, { @@ -3643,15 +3648,6 @@ export function runManifestParserSelfTest({ 'is_file_tool_guidance_message', ], }, - { - path: 'src/crates/execution/tool-contracts/src/file_read_freshness.rs', - contracts: [ - 'FileReadFreshnessFacts', - 'normalize_tool_file_content', - 'file_read_facts_content_matches', - 'file_read_facts_are_fresh', - ], - }, { path: 'src/crates/execution/tool-contracts/src/tool_result_storage.rs', contracts: [ diff --git a/src/crates/assembly/core/src/agentic/session/file_read_state.rs b/src/crates/assembly/core/src/agentic/session/file_read_state.rs deleted file mode 100644 index 36405272dd..0000000000 --- a/src/crates/assembly/core/src/agentic/session/file_read_state.rs +++ /dev/null @@ -1,4 +0,0 @@ -//! Compatibility facade for session-scoped file read state. - -pub use openbitfun_agent_runtime::file_read_state::{FileReadState, FileReadStateStore}; -pub use openbitfun_agent_runtime::file_read_state::{FileRevision, ReviewReadCoverage}; diff --git a/src/crates/assembly/core/src/agentic/session/mod.rs b/src/crates/assembly/core/src/agentic/session/mod.rs index d72b4513eb..7aadd0708f 100644 --- a/src/crates/assembly/core/src/agentic/session/mod.rs +++ b/src/crates/assembly/core/src/agentic/session/mod.rs @@ -6,9 +6,9 @@ pub mod compression; pub mod context_store; mod context_usage; pub mod evidence_ledger; -pub mod file_read_state; pub mod prompt_cache; pub(crate) mod revert; +pub mod review_read_receipt; pub mod session_manager; pub mod session_store_port; pub mod token_anchor; @@ -19,8 +19,8 @@ pub use compression::*; pub use context_store::*; pub use context_usage::*; pub use evidence_ledger::*; -pub use file_read_state::*; pub use prompt_cache::*; +pub use review_read_receipt::*; pub use session_manager::*; pub use session_store_port::*; pub use token_anchor::*; diff --git a/src/crates/assembly/core/src/agentic/session/review_read_receipt.rs b/src/crates/assembly/core/src/agentic/session/review_read_receipt.rs new file mode 100644 index 0000000000..d6570826f6 --- /dev/null +++ b/src/crates/assembly/core/src/agentic/session/review_read_receipt.rs @@ -0,0 +1,5 @@ +//! Compatibility facade for session-scoped code-review read receipts. + +pub use openbitfun_agent_runtime::review_read_receipt::{ + FileRevision, ReviewReadCoverage, ReviewReadReceiptStore, +}; 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 3b3c1f7428..a3b88c12ba 100644 --- a/src/crates/assembly/core/src/agentic/session/session_manager.rs +++ b/src/crates/assembly/core/src/agentic/session/session_manager.rs @@ -18,9 +18,9 @@ use crate::agentic::session::session_store_port::CoreSessionStorePort; use crate::agentic::session::{ prompt_cache_persist_action, reconcile_prompt_cache_restore, CachedSystemPrompt, CachedUserContext, EvidenceLedgerCheckpoint, EvidenceLedgerEvent, EvidenceLedgerEventStatus, - EvidenceLedgerSummary, EvidenceLedgerTargetKind, FileReadState, FileReadStateStore, - FileRevision, PromptCacheLookup, PromptCachePersistenceWriteAction, PromptCachePolicy, - PromptCacheRestoreDecision, PromptCacheScope, ReviewReadCoverage, SessionContextStore, + EvidenceLedgerSummary, EvidenceLedgerTargetKind, FileRevision, PromptCacheLookup, + PromptCachePersistenceWriteAction, PromptCachePolicy, PromptCacheRestoreDecision, + PromptCacheScope, ReviewReadCoverage, ReviewReadReceiptStore, SessionContextStore, SessionEvidenceLedger, SessionPromptCache, SessionPromptCacheStore, SystemPromptCacheIdentity, TokenAnchor, TokenAnchorSelection, TokenAnchorStore, TurnSkillAgentSnapshotStore, UserContextCacheIdentity, @@ -391,7 +391,7 @@ pub struct SessionManager { /// restore and fork paths preserve both constraints and extraction evidence. edit_constraints_store: Arc>, - file_read_state_store: Arc, + review_read_receipt_store: Arc, evidence_ledger: Arc, evidence_ledger_operation_locks: Arc, persistence_manager: Arc, @@ -414,7 +414,7 @@ fn clear_session_runtime_stores( token_anchor_store: &TokenAnchorStore, turn_skill_agent_snapshot_store: &TurnSkillAgentSnapshotStore, skill_agent_baseline_override_snapshot_store: &DashMap, - file_read_state_store: &FileReadStateStore, + review_read_receipt_store: &ReviewReadReceiptStore, evidence_ledger: &SessionEvidenceLedger, ) { context_store.delete_session(session_id); @@ -422,7 +422,7 @@ fn clear_session_runtime_stores( token_anchor_store.delete_session(session_id); turn_skill_agent_snapshot_store.delete_session(session_id); skill_agent_baseline_override_snapshot_store.remove(session_id); - file_read_state_store.delete_session(session_id); + review_read_receipt_store.delete_session(session_id); evidence_ledger.delete_session(session_id); } @@ -2064,7 +2064,7 @@ impl SessionManager { 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()), + review_read_receipt_store: Arc::new(ReviewReadReceiptStore::new()), evidence_ledger: Arc::new(SessionEvidenceLedger::new()), evidence_ledger_operation_locks: Arc::new(KeyedAsyncLock::default()), persistence_manager, @@ -2567,7 +2567,7 @@ impl SessionManager { 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 review_read_receipt_store = self.review_read_receipt_store.clone(); let evidence_ledger = self.evidence_ledger.clone(); let evidence_ledger_operation_locks = self.evidence_ledger_operation_locks.clone(); let persistence_manager = self.persistence_manager.clone(); @@ -2603,7 +2603,7 @@ impl SessionManager { turn_skill_agent_snapshot_store, skill_agent_baseline_override_snapshot_store, edit_constraints_store, - file_read_state_store, + review_read_receipt_store, evidence_ledger, evidence_ledger_operation_locks, persistence_manager, @@ -2894,7 +2894,7 @@ impl SessionManager { self.token_anchor_store.create_session(&session_id); self.turn_skill_agent_snapshot_store .create_session(&session_id); - self.file_read_state_store.create_session(&session_id); + self.review_read_receipt_store.create_session(&session_id); self.commit_session_storage_path_claim(&session_id, &session_storage_path, storage_claim); self.commit_active_session_reservation(&session_id, active_session_permit); if let Some(write_lock) = session_write_lock { @@ -5039,7 +5039,7 @@ impl SessionManager { self.token_anchor_store.as_ref(), self.turn_skill_agent_snapshot_store.as_ref(), self.skill_agent_baseline_override_snapshot_store.as_ref(), - self.file_read_state_store.as_ref(), + self.review_read_receipt_store.as_ref(), self.evidence_ledger.as_ref(), ); self.release_session_write_lock(session_id); @@ -5082,7 +5082,7 @@ impl SessionManager { self.token_anchor_store.as_ref(), self.turn_skill_agent_snapshot_store.as_ref(), self.skill_agent_baseline_override_snapshot_store.as_ref(), - self.file_read_state_store.as_ref(), + self.review_read_receipt_store.as_ref(), self.evidence_ledger.as_ref(), ); @@ -6278,7 +6278,7 @@ impl SessionManager { self.token_anchor_store.as_ref(), self.turn_skill_agent_snapshot_store.as_ref(), self.skill_agent_baseline_override_snapshot_store.as_ref(), - self.file_read_state_store.as_ref(), + self.review_read_receipt_store.as_ref(), self.evidence_ledger.as_ref(), ); } @@ -6410,7 +6410,7 @@ impl SessionManager { }; self.context_store.replace_context(session_id, messages); - self.file_read_state_store.clear_session(session_id); + self.review_read_receipt_store.clear_session(session_id); let fallback_agent_type = self .sessions .get(session_id) @@ -6572,7 +6572,7 @@ impl SessionManager { // 2) Restore the in-memory context cache. self.context_store .replace_context(session_id, messages.clone()); - self.file_read_state_store.clear_session(session_id); + self.review_read_receipt_store.clear_session(session_id); self.prune_token_anchors_to_messages(session_id, &messages) .await; @@ -9253,26 +9253,13 @@ impl SessionManager { pub async fn replace_context_messages(&self, session_id: &str, messages: Vec) { self.context_store .replace_context(session_id, messages.clone()); - self.file_read_state_store.clear_session(session_id); + self.review_read_receipt_store.clear_session(session_id); self.prune_token_anchors_to_messages(session_id, &messages) .await; self.persist_current_turn_context_snapshot_best_effort(session_id, "context_replaced") .await; } - pub fn set_file_read_state(&self, session_id: &str, logical_path: &str, state: FileReadState) { - self.file_read_state_store - .set(session_id, logical_path, state); - } - - pub fn get_file_read_state( - &self, - session_id: &str, - logical_path: &str, - ) -> Option { - self.file_read_state_store.get(session_id, logical_path) - } - pub fn record_review_read( &self, session_id: &str, @@ -9282,7 +9269,7 @@ impl SessionManager { end_line: usize, total_lines: usize, ) { - self.file_read_state_store.record_review_read( + self.review_read_receipt_store.record_review_read( session_id, logical_path, revision, @@ -9300,7 +9287,7 @@ impl SessionManager { start_line: usize, limit: usize, ) -> Option { - self.file_read_state_store.review_read_coverage( + self.review_read_receipt_store.review_read_coverage( session_id, logical_path, revision, @@ -9631,7 +9618,7 @@ impl SessionManager { 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 review_read_receipt_store = self.review_read_receipt_store.clone(); let evidence_ledger = self.evidence_ledger.clone(); tokio::spawn(async move { @@ -9728,7 +9715,7 @@ impl SessionManager { token_anchor_store.as_ref(), turn_skill_agent_snapshot_store.as_ref(), skill_agent_baseline_override_snapshot_store.as_ref(), - file_read_state_store.as_ref(), + review_read_receipt_store.as_ref(), evidence_ledger.as_ref(), ); edit_constraints_store.remove(&candidate.session_id); diff --git a/src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs b/src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs deleted file mode 100644 index 7a31f5a557..0000000000 --- a/src/crates/assembly/core/src/agentic/tools/file_read_state_runtime.rs +++ /dev/null @@ -1,452 +0,0 @@ -//! Runtime helpers for session-scoped file read state used by Read/Edit/Write tools. - -use crate::agentic::coordination::get_global_coordinator; -use crate::agentic::session::{FileReadState, FileRevision, ReviewReadCoverage}; -use crate::agentic::tools::framework::ToolPathResolution; -use crate::agentic::tools::tool_context_runtime::ToolUseContext; -use crate::util::errors::OpenBitFunResult; -pub use openbitfun_agent_runtime::file_read_state::{ - assert_file_not_unexpectedly_modified, content_unchanged_since_full_read, - FILE_UNEXPECTEDLY_MODIFIED_ERROR, -}; -use openbitfun_agent_runtime::file_read_state::{ - validate_edit_content_freshness_against_read_state, validate_prior_read_state, - validate_write_content_freshness_against_read_state, - validate_write_mtime_freshness_against_read_state, FileMutationKind, -}; -use sha2::{Digest, Sha256}; -use std::time::UNIX_EPOCH; -use tool_runtime::fs::read_file::ReadFileResult; -use tool_runtime::util::read_line_prefix::read_tool_output_to_file_content; - -pub fn validate_write_has_prior_read( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let session_id = context.session_id.as_deref()?; - let coordinator = get_global_coordinator()?; - let read_state = coordinator - .get_session_manager() - .get_file_read_state(session_id, &resolved.logical_path); - validate_prior_read_state( - &resolved.logical_path, - read_state.as_ref(), - FileMutationKind::Write, - ) -} - -pub fn read_state_tracking_enabled(context: &ToolUseContext) -> bool { - context.session_id.is_some() && get_global_coordinator().is_some() -} - -pub fn record_file_read_state( - context: &ToolUseContext, - resolved: &ToolPathResolution, - read_result: &ReadFileResult, - timestamp_ms: u64, -) { - let Some(session_id) = context.session_id.as_deref() else { - return; - }; - let Some(coordinator) = get_global_coordinator() else { - return; - }; - - // `is_partial_view` is reserved for auto-injected content the model has not - // explicitly read (see Claude Code's FileState.isPartialView). Normal Read - // tool calls with offset/limit still count as a valid read for Edit/Write. - let state = FileReadState::from_read_tool_content_with_truncation( - read_tool_output_to_file_content(&read_result.content), - timestamp_ms, - read_result.start_line, - read_result.end_line, - read_result.total_lines, - read_result.content_truncated, - ); - - coordinator.get_session_manager().set_file_read_state( - session_id, - &resolved.logical_path, - state, - ); -} - -pub fn review_read_receipts_enabled(context: &ToolUseContext) -> bool { - context.custom_data.contains_key("deep_review_run_manifest") - || context.agent_type.as_deref().is_some_and(|agent_type| { - matches!( - agent_type, - "CodeReview" | "DeepReview" | "ReviewWorker" | "ReviewJudge" - ) - }) -} - -/// Capture the same revision facts from either workspace provider. The hash -/// is streamed and the metadata is checked again so a detected concurrent -/// change never becomes a reusable review receipt. -pub async fn file_revision( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - use tokio::io::AsyncReadExt; - let file_system = context.file_system_for_path(resolved).ok()?; - let before = file_system - .metadata(&resolved.resolved_path, true) - .await - .ok()??; - if before.kind != openbitfun_runtime_ports::WorkspacePathKind::File { - return None; - } - let modified_ns = before.modified?.duration_since(UNIX_EPOCH).ok()?.as_nanos(); - let mut reader = file_system.open_read(&resolved.resolved_path).await.ok()?; - let mut hasher = Sha256::new(); - let mut byte_len = 0_u64; - let mut buffer = [0_u8; 64 * 1024]; - loop { - let count = reader.read(&mut buffer).await.ok()?; - if count == 0 { - break; - } - byte_len = byte_len.checked_add(count as u64)?; - hasher.update(&buffer[..count]); - } - let after = file_system - .metadata(&resolved.resolved_path, true) - .await - .ok()??; - if before.kind != after.kind || before.size != after.size || before.modified != after.modified { - return None; - } - if after.size.is_some_and(|size| size != byte_len) { - return None; - } - Some(FileRevision { - modified_ns, - byte_len, - content_sha256: hasher.finalize().into(), - }) -} - -pub fn get_review_read_coverage( - context: &ToolUseContext, - resolved: &ToolPathResolution, - revision: FileRevision, - start_line: usize, - limit: usize, -) -> Option { - if !review_read_receipts_enabled(context) { - return None; - } - let session_id = context.session_id.as_deref()?; - let coordinator = get_global_coordinator()?; - coordinator.get_session_manager().review_read_coverage( - session_id, - &resolved.logical_path, - revision, - start_line, - limit, - ) -} - -pub fn record_review_read_receipt( - context: &ToolUseContext, - resolved: &ToolPathResolution, - revision: FileRevision, - read_result: &ReadFileResult, -) { - if read_result.content_truncated || !review_read_receipts_enabled(context) { - return; - } - let Some(session_id) = context.session_id.as_deref() else { - return; - }; - let Some(coordinator) = get_global_coordinator() else { - return; - }; - coordinator.get_session_manager().record_review_read( - session_id, - &resolved.logical_path, - revision, - read_result.start_line, - read_result.end_line, - read_result.total_lines, - ); -} - -pub fn get_stored_file_read_state( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let session_id = context.session_id.as_deref()?; - let coordinator = get_global_coordinator()?; - coordinator - .get_session_manager() - .get_file_read_state(session_id, &resolved.logical_path) -} - -pub async fn validate_edit_against_read_state( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let session_id = context.session_id.as_deref()?; - let coordinator = get_global_coordinator()?; - let read_state = coordinator - .get_session_manager() - .get_file_read_state(session_id, &resolved.logical_path)?; - - let current_content = match read_current_file_content(context, resolved).await { - Ok(content) => content, - Err(error) => { - return Some(format!( - "File {} could not be re-read before editing ({}). Read it again when the workspace is available.", - resolved.logical_path, error - )); - } - }; - let current_mtime_ms = file_modification_time_ms(context, resolved).await; - - validate_edit_content_freshness_against_read_state( - &resolved.logical_path, - &read_state, - ¤t_content, - current_mtime_ms, - ) -} - -pub async fn validate_write_against_read_state( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let read_state = get_stored_file_read_state(context, resolved)?; - - if let Some(current_mtime_ms) = file_modification_time_ms(context, resolved).await { - return validate_write_mtime_freshness_against_read_state( - &resolved.logical_path, - &read_state, - current_mtime_ms, - ); - } - - let current_content = read_current_file_content(context, resolved).await.ok()?; - validate_write_content_freshness_against_read_state( - &resolved.logical_path, - &read_state, - ¤t_content, - ) -} - -pub async fn validate_existing_file_read_before_write( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - if let Some(message) = validate_write_has_prior_read(context, resolved) { - return Some(message); - } - - validate_write_against_read_state(context, resolved).await -} - -pub fn validate_edit_has_prior_read( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let session_id = context.session_id.as_deref()?; - let coordinator = get_global_coordinator()?; - let read_state = coordinator - .get_session_manager() - .get_file_read_state(session_id, &resolved.logical_path); - validate_prior_read_state( - &resolved.logical_path, - read_state.as_ref(), - FileMutationKind::Edit, - ) -} - -pub fn update_file_read_state_after_mutation( - context: &ToolUseContext, - resolved: &ToolPathResolution, - content: &str, - timestamp_ms: u64, -) { - let Some(session_id) = context.session_id.as_deref() else { - return; - }; - let Some(coordinator) = get_global_coordinator() else { - return; - }; - - let state = FileReadState::from_full_content(content, timestamp_ms); - - coordinator.get_session_manager().set_file_read_state( - session_id, - &resolved.logical_path, - state, - ); -} - -pub async fn read_current_file_content( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> OpenBitFunResult { - context - .file_system_for_path(resolved)? - .read_file_text(&resolved.resolved_path) - .await - .map_err(|error| { - crate::util::errors::OpenBitFunError::tool(format!( - "Failed to read file {}: {:#}", - resolved.logical_path, error - )) - }) -} - -pub async fn file_modification_time_ms( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> Option { - let metadata = context - .file_system_for_path(resolved) - .ok()? - .metadata(&resolved.resolved_path, true) - .await - .ok()??; - metadata - .modified? - .duration_since(UNIX_EPOCH) - .ok() - .map(|duration| duration.as_millis() as u64) -} - -pub async fn file_mutation_timestamp_ms( - context: &ToolUseContext, - resolved: &ToolPathResolution, -) -> u64 { - // Unknown workspace mtime is not the controller's wall clock. A zero - // timestamp keeps the same unknown-fact representation used by Read. - file_modification_time_ms(context, resolved) - .await - .unwrap_or(0) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::agentic::tools::framework::ToolPathBackend; - use crate::agentic::tools::tool_context_runtime::ToolUseContext; - use crate::agentic::WorkspaceBinding; - use std::collections::HashMap; - use std::path::PathBuf; - - fn test_context(session_id: Option<&str>, root: PathBuf) -> ToolUseContext { - ToolUseContext { - tool_call_id: None, - agent_type: None, - session_id: session_id.map(str::to_string), - dialog_turn_id: Some("turn-1".to_string()), - workspace: Some(WorkspaceBinding::new(None, root)), - 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: Default::default(), - runtime_handles: openbitfun_runtime_ports::ToolRuntimeHandles::default(), - } - } - - #[test] - fn validate_edit_has_prior_read_skips_without_session_id() { - let context = test_context(None, PathBuf::from("/tmp")); - - assert!(validate_edit_has_prior_read( - &context, - &ToolPathResolution { - logical_path: "src/main.rs".to_string(), - resolved_path: "src/main.rs".to_string(), - requested_path: "src/main.rs".to_string(), - backend: ToolPathBackend::Local, - runtime_root: None, - runtime_scope: None, - } - ) - .is_none()); - } - - #[test] - fn validate_edit_has_prior_read_skips_without_coordinator() { - let context = test_context(Some("session-1"), PathBuf::from("/tmp")); - - assert!(validate_edit_has_prior_read( - &context, - &ToolPathResolution { - logical_path: "src/main.rs".to_string(), - resolved_path: "src/main.rs".to_string(), - requested_path: "src/main.rs".to_string(), - backend: ToolPathBackend::Local, - runtime_root: None, - runtime_scope: None, - } - ) - .is_none()); - } - - #[test] - fn validate_edit_has_prior_read_rejects_auto_injected_partial_view() { - let context = test_context(Some("session-1"), PathBuf::from("/tmp")); - let resolution = ToolPathResolution { - logical_path: "src/main.rs".to_string(), - resolved_path: "src/main.rs".to_string(), - requested_path: "src/main.rs".to_string(), - backend: ToolPathBackend::Local, - runtime_root: None, - runtime_scope: None, - }; - - // Without a coordinator this stays permissive in unit tests. - assert!(validate_edit_has_prior_read(&context, &resolution).is_none()); - } - - #[tokio::test] - async fn file_revision_detects_same_size_content_changes_with_restored_mtime() { - let temp = tempfile::tempdir().expect("temp dir"); - let path = temp.path().join("review.txt"); - std::fs::write(&path, b"alpha").expect("write original"); - let original_mtime = filetime::FileTime::from_last_modification_time( - &std::fs::metadata(&path).expect("original metadata"), - ); - let context = test_context(None, temp.path().to_path_buf()); - let resolved = context - .resolve_tool_path("review.txt") - .expect("resolve file"); - let original = file_revision(&context, &resolved) - .await - .expect("original revision"); - - std::fs::write(&path, b"bravo").expect("write replacement"); - filetime::set_file_mtime(&path, original_mtime).expect("restore mtime"); - let replacement = file_revision(&context, &resolved) - .await - .expect("replacement revision"); - - assert_eq!(original.modified_ns, replacement.modified_ns); - assert_eq!(original.byte_len, replacement.byte_len); - assert_ne!(original.content_sha256, replacement.content_sha256); - assert_ne!(original, replacement); - } - - #[test] - fn review_read_receipts_do_not_treat_a_custom_legacy_name_as_a_builtin_worker() { - let mut custom = test_context(Some("session-1"), PathBuf::from("/tmp")); - custom.agent_type = Some("ReviewSecurity".to_string()); - assert!(!review_read_receipts_enabled(&custom)); - - custom.custom_data.insert( - "deep_review_run_manifest".to_string(), - serde_json::json!({}), - ); - assert!(review_read_receipts_enabled(&custom)); - - let mut worker = test_context(Some("session-2"), PathBuf::from("/tmp")); - worker.agent_type = Some("ReviewWorker".to_string()); - assert!(review_read_receipts_enabled(&worker)); - } -} 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 0e14ac6da1..594bee46be 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 @@ -1,10 +1,4 @@ use crate::agentic::tools::file_permissions::file_permission_intents_allowing_managed_plan_edits; -use crate::agentic::tools::file_read_state_runtime::{ - assert_file_not_unexpectedly_modified, file_modification_time_ms, file_mutation_timestamp_ms, - get_stored_file_read_state, read_current_file_content, read_state_tracking_enabled, - update_file_read_state_after_mutation, validate_edit_against_read_state, - validate_edit_has_prior_read, FILE_UNEXPECTEDLY_MODIFIED_ERROR, -}; use crate::agentic::tools::file_tool_guidance::file_tool_guidance_message; use crate::agentic::tools::framework::{ PermissionIntent, Tool, ToolPathResolution, ToolResult, ToolUseContext, ValidationResult, @@ -22,7 +16,7 @@ pub struct FileEditTool; const EDIT_TOOL_PROMPT: &str = r#"Performs exact string replacements in files. Usage: -- You must use your `Read` tool at least once in the conversation before editing. This tool will error if you attempt an edit without reading the file. +- You must read the current file contents before editing. - The `file_path` parameter must be a workspace-relative path, an absolute path inside the current workspace, or an exact `openbitfun://...` URI returned by another tool. - When editing text from Read tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: spaces + line number + tab. Everything after that is the actual file content to match. Never include any part of the line number prefix in the old_string or new_string. - Copy `old_string` verbatim from your latest Read of this file. Do not reformat HTML/CSS/JS, do not normalize indentation, and do not reconstruct the block from memory. @@ -54,50 +48,20 @@ impl FileEditTool { } } - fn format_edit_freshness_guidance(logical_path: &str, error: String) -> String { - if error == FILE_UNEXPECTEDLY_MODIFIED_ERROR || error.contains("unexpectedly modified") { - format!( - "The file {} changed since it was last read. Use Read again, then retry Edit.", - logical_path - ) - } else { - error - } - } - - async fn edit_read_state_guardrail_error( + async fn read_current_file_content( context: &ToolUseContext, resolved: &ToolPathResolution, - ) -> Option { - if let Some(message) = validate_edit_has_prior_read(context, resolved) { - return Some(message); - } - - validate_edit_against_read_state(context, resolved).await - } - - async fn assert_edit_freshness( - context: &ToolUseContext, - resolved: &ToolPathResolution, - content: &str, - ) -> OpenBitFunResult<()> { - if !read_state_tracking_enabled(context) { - return Ok(()); - } - - let read_state = get_stored_file_read_state(context, resolved); - let current_mtime_ms = file_modification_time_ms(context, resolved).await; - - if let Some(error) = - assert_file_not_unexpectedly_modified(read_state.as_ref(), content, current_mtime_ms) - .err() - { - return Err(OpenBitFunError::tool(file_tool_guidance_message( - Self::format_edit_freshness_guidance(&resolved.logical_path, error), - ))); - } - - Ok(()) + ) -> OpenBitFunResult { + context + .file_system_for_path(resolved)? + .read_file_text(&resolved.resolved_path) + .await + .map_err(|error| { + OpenBitFunError::tool(format!( + "Failed to read file {}: {:#}", + resolved.logical_path, error + )) + }) } } @@ -254,11 +218,7 @@ impl Tool for FileEditTool { }; } - if let Some(message) = Self::edit_read_state_guardrail_error(ctx, &resolved).await { - return Self::guidance_failure(message); - } - - let file_content = match read_current_file_content(ctx, &resolved).await { + let file_content = match Self::read_current_file_content(ctx, &resolved).await { Ok(content) => content, Err(error) => { return ValidationResult { @@ -351,8 +311,7 @@ impl Tool for FileEditTool { .await?; let file_system = context.file_system_for_path(&resolved)?; - let content = read_current_file_content(context, &resolved).await?; - Self::assert_edit_freshness(context, &resolved, &content).await?; + let content = Self::read_current_file_content(context, &resolved).await?; let edit_result = apply_edit_to_content(&content, old_string, new_string, replace_all) .map_err(|error| { if is_edit_content_guardrail_error(&error) { @@ -371,13 +330,6 @@ impl Tool for FileEditTool { )) })?; - let timestamp_ms = file_mutation_timestamp_ms(context, &resolved).await; - update_file_read_state_after_mutation( - context, - &resolved, - &edit_result.new_content, - timestamp_ms, - ); crate::agentic::execution::edit_constraint_guard::record_mutation_applied( context, "Edit", @@ -410,21 +362,6 @@ mod tests { use crate::agentic::tools::framework::Tool; use serde_json::{json, Value}; - #[tokio::test] - async fn edit_tool_prompt_matches_claude_style() { - let description = FileEditTool::new() - .description() - .await - .expect("description"); - - assert_eq!(description, EDIT_TOOL_PROMPT); - assert!(description.contains("You must use your `Read` tool")); - assert!(description.contains("spaces + line number + tab")); - assert!(description.contains("verbatim from your latest Read")); - assert!(description.contains("NEVER write new files unless explicitly required")); - assert!(!description.contains("auto-strip")); - } - #[test] fn edit_tool_schema_describes_exact_copy_from_read() { let schema = FileEditTool::new().input_schema(); 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 a7678807d3..c785ef4b16 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 @@ -1,8 +1,4 @@ use crate::agentic::tools::file_permissions::file_permission_intents; -use crate::agentic::tools::file_read_state_runtime::{ - file_modification_time_ms, file_revision, get_review_read_coverage, record_file_read_state, - record_review_read_receipt, review_read_receipts_enabled, -}; use crate::agentic::tools::framework::{ PermissionIntent, Tool, ToolRenderOptions, ToolResult, ToolUseContext, ValidationResult, }; @@ -10,6 +6,10 @@ use crate::agentic::tools::framework::{ use crate::agentic::tools::miniapp_context_runtime::{ is_virtual_context_path, requires_virtual_context_path, virtual_context_file, }; +use crate::agentic::tools::review_read_receipt_runtime::{ + file_revision, get_review_read_coverage, record_review_read_receipt, + review_read_receipts_enabled, +}; use crate::agentic::tools::workspace_paths::is_openbitfun_tool_uri; use crate::agentic::tools::ToolPathOperation; use crate::util::errors::{OpenBitFunError, OpenBitFunResult}; @@ -767,12 +767,6 @@ Usage: (result, None) }; - if document_metadata.is_none() { - let timestamp_ms = file_modification_time_ms(context, &resolved) - .await - .unwrap_or(0); - record_file_read_state(context, &resolved, &read_file_result, timestamp_ms); - } if let Some(revision_before) = revision_before_read { if let Some(revision_after) = file_revision(context, &resolved).await { if revision_before == revision_after { @@ -1035,41 +1029,6 @@ mod tests { assert!(error.to_string().contains("unavailable"), "{error}"); } - #[tokio::test] - async fn long_line_read_is_explicit_but_cannot_claim_full_content_freshness() { - use openbitfun_agent_runtime::file_read_state::{ - assert_file_not_unexpectedly_modified, validate_prior_read_state, FileMutationKind, - FileReadState, - }; - use tool_runtime::util::read_line_prefix::read_tool_output_to_file_content; - let content = format!("{}\n", "a".repeat(3_000)); - let context = remote_context(content.as_bytes().to_vec(), Arc::new(AtomicUsize::new(0))); - let results = FileReadTool::new() - .call(&json!({"file_path":"long.txt"}), &context) - .await - .unwrap(); - let ToolResult::Result { data, .. } = &results[0] else { - panic!("result"); - }; - assert_eq!(data["total_lines"], 1); - assert_eq!(data["content_truncated"], true); - let state = FileReadState::from_read_tool_content_with_truncation( - read_tool_output_to_file_content(data["content"].as_str().unwrap()), - 100, - data["start_line"].as_u64().unwrap() as usize, - data["lines_read"].as_u64().unwrap() as usize, - data["total_lines"].as_u64().unwrap() as usize, - data["content_truncated"].as_bool().unwrap(), - ); - assert!(!state.is_full_file_read()); - assert!(!state.is_partial_view); - assert!( - validate_prior_read_state("long.txt", Some(&state), FileMutationKind::Edit).is_none() - ); - assert!(assert_file_not_unexpectedly_modified(Some(&state), &content, Some(100)).is_ok()); - assert!(assert_file_not_unexpectedly_modified(Some(&state), &content, Some(200)).is_err()); - } - #[test] fn read_tool_schema_prefers_offset() { let schema = FileReadTool::new().input_schema(); 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 05453853c4..d3d835c5a7 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 @@ -2,15 +2,7 @@ use super::plan_artifact_diagnostics::{ diagnose_plan_artifact, is_plan_artifact_path, PlanArtifactIssue, }; use crate::agentic::tools::file_permissions::file_permission_intents_allowing_managed_plan_edits; -use crate::agentic::tools::file_read_state_runtime::{ - assert_file_not_unexpectedly_modified, file_modification_time_ms, file_mutation_timestamp_ms, - get_stored_file_read_state, read_current_file_content, read_state_tracking_enabled, - update_file_read_state_after_mutation, validate_existing_file_read_before_write, - FILE_UNEXPECTEDLY_MODIFIED_ERROR, -}; -use crate::agentic::tools::file_tool_guidance::{ - file_tool_guidance_message, is_file_tool_guidance_message, -}; +use crate::agentic::tools::file_tool_guidance::is_file_tool_guidance_message; use crate::agentic::tools::framework::{ PermissionIntent, Tool, ToolPathResolution, ToolRenderOptions, ToolResult, ToolUseContext, ValidationResult, @@ -66,22 +58,6 @@ impl FileWriteTool { Self } - fn format_write_freshness_guidance(logical_path: &str, error: String) -> String { - if error == FILE_UNEXPECTEDLY_MODIFIED_ERROR || error.contains("unexpectedly modified") { - format!( - "The file {} changed since it was last read. Use Read again, then retry Write.", - logical_path - ) - } else if error.contains("modified since read") { - format!( - "The file {} changed after it was last read. Use Read again, then retry Write.", - logical_path - ) - } else { - error - } - } - async fn file_exists( context: &ToolUseContext, resolved: &ToolPathResolution, @@ -113,65 +89,6 @@ impl FileWriteTool { Ok(existing == content.as_bytes()) } - async fn existing_file_write_freshness_error( - context: &ToolUseContext, - resolved: &ToolPathResolution, - ) -> Option { - match Self::file_exists(context, resolved).await { - Ok(false) => return None, - Ok(true) => {} - Err(error) => return Some(error.to_string()), - } - if !read_state_tracking_enabled(context) { - return None; - } - - let current_content = match read_current_file_content(context, resolved).await { - Ok(content) => content, - Err(error) => return Some(error.to_string()), - }; - let read_state = get_stored_file_read_state(context, resolved); - let current_mtime_ms = file_modification_time_ms(context, resolved).await; - - assert_file_not_unexpectedly_modified( - read_state.as_ref(), - ¤t_content, - current_mtime_ms, - ) - .err() - .map(|error| Self::format_write_freshness_guidance(&resolved.logical_path, error)) - } - - async fn assert_write_freshness_if_exists( - context: &ToolUseContext, - resolved: &ToolPathResolution, - ) -> OpenBitFunResult<()> { - if let Some(error) = Self::existing_file_write_freshness_error(context, resolved).await { - return Err(OpenBitFunError::tool(file_tool_guidance_message(error))); - } - - Ok(()) - } - - async fn write_guardrail_preflight_error( - context: &ToolUseContext, - resolved: &ToolPathResolution, - ) -> Option { - match Self::file_exists(context, resolved).await { - Ok(false) => return None, - Ok(true) => {} - Err(error) => return Some(error.to_string()), - } - - if let Some(message) = validate_existing_file_read_before_write(context, resolved).await { - return Some(file_tool_guidance_message(message)); - } - - Self::existing_file_write_freshness_error(context, resolved) - .await - .map(file_tool_guidance_message) - } - pub(crate) async fn preflight_write_error( context: &ToolUseContext, file_path: &str, @@ -185,7 +102,10 @@ impl FileWriteTool { return Some(err.to_string()); } - Self::write_guardrail_preflight_error(context, &resolved).await + Self::file_exists(context, &resolved) + .await + .err() + .map(|error| error.to_string()) } fn parse_payload(input: &Value) -> Result, String> { @@ -599,8 +519,6 @@ impl Tool for FileWriteTool { return Ok(vec![result]); } - Self::assert_write_freshness_if_exists(context, &resolved).await?; - context .file_system_for_path(&resolved)? .write_file(&resolved.resolved_path, content.as_bytes()) @@ -614,8 +532,6 @@ impl Tool for FileWriteTool { let outcome = write_file_success_outcome(&resolved.logical_path, file_already_exists, &content); - 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", @@ -798,7 +714,7 @@ mod tests { } #[tokio::test] - async fn write_never_overwrites_an_unreadable_existing_file_without_read_state() { + async fn write_never_overwrites_an_unreadable_existing_file() { for remote in [false, true] { let root = if remote { PathBuf::from("/remote/workspace") @@ -918,7 +834,7 @@ mod tests { } #[tokio::test] - async fn preflight_write_error_allows_existing_file_without_read_state_tracking() { + async fn preflight_write_error_allows_existing_file() { let root = std::env::temp_dir().join(format!("openbitfun-write-test-{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(&root).expect("create temp workspace"); diff --git a/src/crates/assembly/core/src/agentic/tools/mod.rs b/src/crates/assembly/core/src/agentic/tools/mod.rs index bdf63d8226..0baef60f54 100644 --- a/src/crates/assembly/core/src/agentic/tools/mod.rs +++ b/src/crates/assembly/core/src/agentic/tools/mod.rs @@ -7,7 +7,6 @@ pub mod computer_use_capability; pub mod computer_use_host; pub mod computer_use_optimizer; pub(crate) mod file_permissions; -pub mod file_read_state_runtime; pub mod file_tool_guidance; pub mod framework; #[cfg(feature = "tools-creation")] @@ -32,6 +31,7 @@ pub(crate) mod post_call_hooks; pub mod product_runtime; pub mod registry; pub mod restrictions; +pub mod review_read_receipt_runtime; pub(crate) mod tool_adapter; pub(crate) mod tool_context_runtime; pub(crate) mod tool_result_storage; diff --git a/src/crates/assembly/core/src/agentic/tools/review_read_receipt_runtime.rs b/src/crates/assembly/core/src/agentic/tools/review_read_receipt_runtime.rs new file mode 100644 index 0000000000..7aa15873d0 --- /dev/null +++ b/src/crates/assembly/core/src/agentic/tools/review_read_receipt_runtime.rs @@ -0,0 +1,181 @@ +//! Runtime helpers for code-review read receipts. + +use crate::agentic::coordination::get_global_coordinator; +use crate::agentic::session::{FileRevision, ReviewReadCoverage}; +use crate::agentic::tools::framework::ToolPathResolution; +use crate::agentic::tools::tool_context_runtime::ToolUseContext; +use sha2::{Digest, Sha256}; +use std::time::UNIX_EPOCH; +use tool_runtime::fs::read_file::ReadFileResult; + +pub fn review_read_receipts_enabled(context: &ToolUseContext) -> bool { + context.custom_data.contains_key("deep_review_run_manifest") + || context.agent_type.as_deref().is_some_and(|agent_type| { + matches!( + agent_type, + "CodeReview" | "DeepReview" | "ReviewWorker" | "ReviewJudge" + ) + }) +} + +/// Capture the same revision facts from either workspace provider. The hash +/// is streamed and the metadata is checked again so a detected concurrent +/// change never becomes a reusable review receipt. +pub async fn file_revision( + context: &ToolUseContext, + resolved: &ToolPathResolution, +) -> Option { + use tokio::io::AsyncReadExt; + let file_system = context.file_system_for_path(resolved).ok()?; + let before = file_system + .metadata(&resolved.resolved_path, true) + .await + .ok()??; + if before.kind != openbitfun_runtime_ports::WorkspacePathKind::File { + return None; + } + let modified_ns = before.modified?.duration_since(UNIX_EPOCH).ok()?.as_nanos(); + let mut reader = file_system.open_read(&resolved.resolved_path).await.ok()?; + let mut hasher = Sha256::new(); + let mut byte_len = 0_u64; + let mut buffer = [0_u8; 64 * 1024]; + loop { + let count = reader.read(&mut buffer).await.ok()?; + if count == 0 { + break; + } + byte_len = byte_len.checked_add(count as u64)?; + hasher.update(&buffer[..count]); + } + let after = file_system + .metadata(&resolved.resolved_path, true) + .await + .ok()??; + if before.kind != after.kind || before.size != after.size || before.modified != after.modified { + return None; + } + if after.size.is_some_and(|size| size != byte_len) { + return None; + } + Some(FileRevision { + modified_ns, + byte_len, + content_sha256: hasher.finalize().into(), + }) +} + +pub fn get_review_read_coverage( + context: &ToolUseContext, + resolved: &ToolPathResolution, + revision: FileRevision, + start_line: usize, + limit: usize, +) -> Option { + if !review_read_receipts_enabled(context) { + return None; + } + let session_id = context.session_id.as_deref()?; + let coordinator = get_global_coordinator()?; + coordinator.get_session_manager().review_read_coverage( + session_id, + &resolved.logical_path, + revision, + start_line, + limit, + ) +} + +pub fn record_review_read_receipt( + context: &ToolUseContext, + resolved: &ToolPathResolution, + revision: FileRevision, + read_result: &ReadFileResult, +) { + if read_result.content_truncated || !review_read_receipts_enabled(context) { + return; + } + let Some(session_id) = context.session_id.as_deref() else { + return; + }; + let Some(coordinator) = get_global_coordinator() else { + return; + }; + coordinator.get_session_manager().record_review_read( + session_id, + &resolved.logical_path, + revision, + read_result.start_line, + read_result.end_line, + read_result.total_lines, + ); +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::agentic::tools::tool_context_runtime::ToolUseContext; + use crate::agentic::WorkspaceBinding; + use std::collections::HashMap; + use std::path::PathBuf; + + fn test_context(session_id: Option<&str>, root: PathBuf) -> ToolUseContext { + ToolUseContext { + tool_call_id: None, + agent_type: None, + session_id: session_id.map(str::to_string), + dialog_turn_id: Some("turn-1".to_string()), + workspace: Some(WorkspaceBinding::new(None, root)), + 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: Default::default(), + runtime_handles: openbitfun_runtime_ports::ToolRuntimeHandles::default(), + } + } + + #[tokio::test] + async fn file_revision_detects_same_size_content_changes_with_restored_mtime() { + let temp = tempfile::tempdir().expect("temp dir"); + let path = temp.path().join("review.txt"); + std::fs::write(&path, b"alpha").expect("write original"); + let original_mtime = filetime::FileTime::from_last_modification_time( + &std::fs::metadata(&path).expect("original metadata"), + ); + let context = test_context(None, temp.path().to_path_buf()); + let resolved = context + .resolve_tool_path("review.txt") + .expect("resolve file"); + let original = file_revision(&context, &resolved) + .await + .expect("original revision"); + + std::fs::write(&path, b"bravo").expect("write replacement"); + filetime::set_file_mtime(&path, original_mtime).expect("restore mtime"); + let replacement = file_revision(&context, &resolved) + .await + .expect("replacement revision"); + + assert_eq!(original.modified_ns, replacement.modified_ns); + assert_eq!(original.byte_len, replacement.byte_len); + assert_ne!(original.content_sha256, replacement.content_sha256); + assert_ne!(original, replacement); + } + + #[test] + fn review_read_receipts_do_not_treat_a_custom_legacy_name_as_a_builtin_worker() { + let mut custom = test_context(Some("session-1"), PathBuf::from("/tmp")); + custom.agent_type = Some("ReviewSecurity".to_string()); + assert!(!review_read_receipts_enabled(&custom)); + + custom.custom_data.insert( + "deep_review_run_manifest".to_string(), + serde_json::json!({}), + ); + assert!(review_read_receipts_enabled(&custom)); + + let mut worker = test_context(Some("session-2"), PathBuf::from("/tmp")); + worker.agent_type = Some("ReviewWorker".to_string()); + assert!(review_read_receipts_enabled(&worker)); + } +} diff --git a/src/crates/execution/agent-runtime/AGENTS.md b/src/crates/execution/agent-runtime/AGENTS.md index e36ba688dc..3afed0c6f5 100644 --- a/src/crates/execution/agent-runtime/AGENTS.md +++ b/src/crates/execution/agent-runtime/AGENTS.md @@ -62,7 +62,7 @@ port-backed `sdk` / `AgentRuntime` facade that can be built and tested without queue decisions, registry source/profile facts, prompt-loop user-context policy, prompt listing reminder ordering, prompt-cache policy/identity/store, prompt runtime/workspace/user-context rendering, turn skill/agent snapshot - state, file-read session state, session evidence ledger projection, + state, code-review read receipts, session evidence ledger projection, finish-reason labels, session-state event labels, and turn-outcome event facts. - Keep concrete prompt fact collection, workspace context IO, prompt-cache diff --git a/src/crates/execution/agent-runtime/src/file_read_state.rs b/src/crates/execution/agent-runtime/src/file_read_state.rs deleted file mode 100644 index ad411d5ea0..0000000000 --- a/src/crates/execution/agent-runtime/src/file_read_state.rs +++ /dev/null @@ -1,734 +0,0 @@ -//! Session-scoped file read state used to gate Edit/Write reliability. - -use dashmap::DashMap; -use openbitfun_agent_tools::{ - file_read_facts_are_fresh, file_read_facts_content_matches, FileReadFreshnessFacts, -}; -use std::sync::Arc; - -pub const FILE_UNEXPECTEDLY_MODIFIED_ERROR: &str = - "File has been unexpectedly modified. Read it again before attempting to write it."; - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum FileMutationKind { - Edit, - Write, -} - -impl FileMutationKind { - fn tool_name(self) -> &'static str { - match self { - Self::Edit => "Edit", - Self::Write => "Write", - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct FileReadState { - /// Raw file content without Read-tool line-number prefixes (LF-normalized view). - pub content: String, - /// File mtime in milliseconds since UNIX epoch when recorded, if known. - pub timestamp_ms: u64, - pub start_line: usize, - pub end_line: usize, - pub total_lines: usize, - /// True when this entry was populated by auto-injection and the model has - /// not explicitly read the file. Range reads from the Read tool do not set this. - pub is_partial_view: bool, - /// The explicit Read view omitted characters because of its output budget. - /// This is still a valid prior Read, but cannot prove full-content equality. - pub content_truncated: bool, -} - -impl FileReadState { - pub fn from_read_tool_content( - content: String, - timestamp_ms: u64, - start_line: usize, - end_line: usize, - total_lines: usize, - ) -> Self { - Self::from_read_tool_content_with_truncation( - content, - timestamp_ms, - start_line, - end_line, - total_lines, - false, - ) - } - - pub fn from_read_tool_content_with_truncation( - content: String, - timestamp_ms: u64, - start_line: usize, - end_line: usize, - total_lines: usize, - content_truncated: bool, - ) -> Self { - Self { - content, - timestamp_ms, - start_line, - end_line, - total_lines, - is_partial_view: false, - content_truncated, - } - } - - pub fn from_full_content(content: &str, timestamp_ms: u64) -> Self { - let line_count = content.lines().count(); - let (start_line, end_line) = if line_count == 0 { - (0, 0) - } else { - (1, line_count) - }; - - Self { - content: content.to_string(), - timestamp_ms, - start_line, - end_line, - total_lines: line_count, - is_partial_view: false, - content_truncated: false, - } - } - - pub fn is_full_file_read(&self) -> bool { - if self.is_partial_view || self.content_truncated { - return false; - } - - if self.total_lines == 0 { - return self.start_line == 0 && self.end_line == 0; - } - - self.start_line == 1 && self.end_line >= self.total_lines - } -} - -pub fn validate_prior_read_state( - logical_path: &str, - read_state: Option<&FileReadState>, - mutation: FileMutationKind, -) -> Option { - let Some(read_state) = read_state else { - return Some(format!( - "Use Read to load the current contents of {} before calling {} on it.", - logical_path, - mutation.tool_name() - )); - }; - - if read_state.is_partial_view { - return Some(format!( - "Use Read to load the full contents of {} before calling {} on it.", - logical_path, - mutation.tool_name() - )); - } - - None -} - -pub fn content_unchanged_since_full_read( - read_state: &FileReadState, - current_content: &str, -) -> bool { - file_read_facts_content_matches(file_read_freshness_facts(read_state), current_content) -} - -pub fn assert_file_not_unexpectedly_modified( - read_state: Option<&FileReadState>, - current_content: &str, - current_mtime_ms: Option, -) -> Result<(), String> { - let Some(read_state) = read_state else { - return Err(FILE_UNEXPECTEDLY_MODIFIED_ERROR.to_string()); - }; - - if !file_read_facts_are_fresh( - file_read_freshness_facts(read_state), - current_content, - current_mtime_ms, - ) { - return Err(FILE_UNEXPECTEDLY_MODIFIED_ERROR.to_string()); - } - - Ok(()) -} - -pub fn validate_edit_content_freshness_against_read_state( - logical_path: &str, - read_state: &FileReadState, - current_content: &str, - current_mtime_ms: Option, -) -> Option { - if file_read_facts_are_fresh( - file_read_freshness_facts(read_state), - current_content, - current_mtime_ms, - ) { - return None; - } - - if current_mtime_ms.is_some() { - return Some(format!( - "The file {} changed after it was last read. Use Read again, then retry Edit.", - logical_path - )); - } - - Some(format!( - "The file {} no longer matches the last Read result. Use Read again, then retry Edit.", - logical_path - )) -} - -pub fn validate_write_mtime_freshness_against_read_state( - logical_path: &str, - read_state: &FileReadState, - current_mtime_ms: u64, -) -> Option { - if current_mtime_ms > read_state.timestamp_ms { - return Some(format!( - "The file {} changed after it was last read. Use Read again, then retry Write.", - logical_path - )); - } - - None -} - -pub fn validate_write_content_freshness_against_read_state( - logical_path: &str, - read_state: &FileReadState, - current_content: &str, -) -> Option { - if !file_read_facts_are_fresh(file_read_freshness_facts(read_state), current_content, None) { - return Some(format!( - "The file {} no longer matches the last Read result. Use Read again, then retry Write.", - logical_path - )); - } - - None -} - -fn file_read_freshness_facts(read_state: &FileReadState) -> FileReadFreshnessFacts<'_> { - FileReadFreshnessFacts { - content: &read_state.content, - timestamp_ms: read_state.timestamp_ms, - is_full_file_read: read_state.is_full_file_read(), - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct FileRevision { - pub modified_ns: u128, - pub byte_len: u64, - pub content_sha256: [u8; 32], -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct ReviewReadCoverage { - pub start_line: usize, - pub end_line: usize, - pub total_lines: usize, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -struct ReviewReadReceipt { - revision: FileRevision, - ranges: Vec<(usize, usize)>, - total_lines: usize, -} - -#[derive(Default)] -pub struct FileReadStateStore { - session_states: Arc>>, - review_read_receipts: Arc>>, -} - -impl FileReadStateStore { - pub fn new() -> Self { - Self::default() - } - - pub fn create_session(&self, session_id: &str) { - self.session_states - .entry(session_id.to_string()) - .or_default(); - } - - pub fn delete_session(&self, session_id: &str) { - self.session_states.remove(session_id); - self.review_read_receipts.remove(session_id); - } - - pub fn clear_session(&self, session_id: &str) { - if let Some(states) = self.session_states.get(session_id) { - states.clear(); - } - if let Some(receipts) = self.review_read_receipts.get(session_id) { - receipts.clear(); - } - } - - pub fn set(&self, session_id: &str, logical_path: &str, state: FileReadState) { - let session_states = self - .session_states - .entry(session_id.to_string()) - .or_default(); - session_states.insert(logical_path.to_string(), state); - } - - pub fn get(&self, session_id: &str, logical_path: &str) -> Option { - self.session_states - .get(session_id) - .and_then(|states| states.get(logical_path).map(|entry| entry.clone())) - } - - pub fn record_review_read( - &self, - session_id: &str, - logical_path: &str, - revision: FileRevision, - start_line: usize, - end_line: usize, - total_lines: usize, - ) { - if start_line == 0 || end_line < start_line { - return; - } - - let session_receipts = self - .review_read_receipts - .entry(session_id.to_string()) - .or_default(); - let mut receipt = session_receipts - .entry(logical_path.to_string()) - .or_insert_with(|| ReviewReadReceipt { - revision, - ranges: Vec::new(), - total_lines, - }); - if receipt.revision != revision { - receipt.revision = revision; - receipt.ranges.clear(); - } - receipt.total_lines = total_lines; - receipt.ranges.push((start_line, end_line)); - receipt.ranges.sort_unstable_by_key(|range| range.0); - - let mut merged = Vec::<(usize, usize)>::with_capacity(receipt.ranges.len()); - for (start, end) in receipt.ranges.drain(..) { - if let Some(last) = merged.last_mut() { - if start <= last.1.saturating_add(1) { - last.1 = last.1.max(end); - continue; - } - } - merged.push((start, end)); - } - receipt.ranges = merged; - } - - pub fn review_read_coverage( - &self, - session_id: &str, - logical_path: &str, - revision: FileRevision, - start_line: usize, - limit: usize, - ) -> Option { - if start_line == 0 || limit == 0 { - return None; - } - let session_receipts = self.review_read_receipts.get(session_id)?; - let receipt = session_receipts.get(logical_path)?; - if receipt.revision != revision || start_line > receipt.total_lines { - return None; - } - let end_line = start_line - .saturating_add(limit.saturating_sub(1)) - .min(receipt.total_lines); - receipt - .ranges - .iter() - .any(|(covered_start, covered_end)| { - *covered_start <= start_line && *covered_end >= end_line - }) - .then_some(ReviewReadCoverage { - start_line, - end_line, - total_lines: receipt.total_lines, - }) - } -} - -#[cfg(test)] -mod tests { - use super::{ - assert_file_not_unexpectedly_modified, validate_edit_content_freshness_against_read_state, - validate_prior_read_state, validate_write_content_freshness_against_read_state, - validate_write_mtime_freshness_against_read_state, FileMutationKind, FileReadState, - FileReadStateStore, FileRevision, ReviewReadCoverage, - }; - - fn sample_state( - start_line: usize, - end_line: usize, - total_lines: usize, - is_partial_view: bool, - ) -> FileReadState { - FileReadState { - content: String::new(), - timestamp_ms: 0, - start_line, - end_line, - total_lines, - is_partial_view, - content_truncated: false, - } - } - - #[test] - fn file_read_state_accepts_nonempty_whole_file() { - let state = sample_state(1, 10, 10, false); - assert!(state.is_full_file_read()); - } - - #[test] - fn file_read_state_rejects_partial_view() { - let state = sample_state(1, 10, 10, true); - assert!(!state.is_full_file_read()); - } - - #[test] - fn file_read_state_accepts_empty_file_from_read_tool() { - let state = sample_state(0, 0, 0, false); - assert!(state.is_full_file_read()); - } - - #[test] - fn file_read_state_rejects_empty_file_with_one_based_range() { - let state = sample_state(1, 0, 0, false); - assert!(!state.is_full_file_read()); - } - - #[test] - fn file_read_state_store_scopes_entries_by_session() { - let store = FileReadStateStore::new(); - store.create_session("session-a"); - store.create_session("session-b"); - - store.set("session-a", "src/lib.rs", sample_state(1, 1, 1, false)); - - assert!(store.get("session-a", "src/lib.rs").is_some()); - assert!(store.get("session-b", "src/lib.rs").is_none()); - assert!(store.get("session-a", "src/main.rs").is_none()); - } - - #[test] - fn file_read_state_store_clear_and_delete_drop_session_entries() { - let store = FileReadStateStore::new(); - store.set("session-a", "src/lib.rs", sample_state(1, 1, 1, false)); - store.set("session-b", "src/lib.rs", sample_state(1, 1, 1, false)); - - store.clear_session("session-a"); - assert!(store.get("session-a", "src/lib.rs").is_none()); - assert!(store.get("session-b", "src/lib.rs").is_some()); - - store.delete_session("session-b"); - assert!(store.get("session-b", "src/lib.rs").is_none()); - } - - #[test] - fn review_read_receipt_covers_only_previously_returned_lines() { - let store = FileReadStateStore::new(); - let revision = FileRevision { - modified_ns: 100, - byte_len: 4096, - content_sha256: [1; 32], - }; - store.record_review_read("review-session", "src/large.rs", revision, 1, 2000, 3000); - - assert_eq!( - store.review_read_coverage("review-session", "src/large.rs", revision, 1403, 27,), - Some(ReviewReadCoverage { - start_line: 1403, - end_line: 1429, - total_lines: 3000, - }) - ); - assert!(store - .review_read_coverage("review-session", "src/large.rs", revision, 2001, 20,) - .is_none()); - } - - #[test] - fn review_read_receipt_merges_ranges_and_invalidates_on_revision_change() { - let store = FileReadStateStore::new(); - let original = FileRevision { - modified_ns: 100, - byte_len: 4096, - content_sha256: [1; 32], - }; - store.record_review_read("review-session", "src/lib.rs", original, 1, 100, 300); - store.record_review_read("review-session", "src/lib.rs", original, 101, 200, 300); - - assert!(store - .review_read_coverage("review-session", "src/lib.rs", original, 50, 151) - .is_some()); - - let changed = FileRevision { - modified_ns: 100, - byte_len: 4096, - content_sha256: [2; 32], - }; - assert!(store - .review_read_coverage("review-session", "src/lib.rs", changed, 50, 151) - .is_none()); - } - - #[test] - fn validate_prior_read_state_requires_initial_read() { - assert_eq!( - validate_prior_read_state("src/main.rs", None, FileMutationKind::Edit).as_deref(), - Some("Use Read to load the current contents of src/main.rs before calling Edit on it.") - ); - } - - #[test] - fn validate_prior_read_state_rejects_auto_injected_partial_view() { - let state = sample_state(1, 10, 10, true); - - assert_eq!( - validate_prior_read_state("src/main.rs", Some(&state), FileMutationKind::Write) - .as_deref(), - Some("Use Read to load the full contents of src/main.rs before calling Write on it.") - ); - } - - #[test] - fn validate_prior_read_state_accepts_read_tool_range() { - let state = sample_state(50, 100, 556, false); - - assert!( - validate_prior_read_state("src/main.rs", Some(&state), FileMutationKind::Edit) - .is_none() - ); - } - - #[test] - fn file_read_state_from_full_content_sets_empty_file_range() { - let state = FileReadState::from_full_content("", 100); - - assert_eq!(state.start_line, 0); - assert_eq!(state.end_line, 0); - assert_eq!(state.total_lines, 0); - assert!(state.is_full_file_read()); - } - - #[test] - fn content_unchanged_since_full_read_rejects_partial_ranges() { - let state = sample_state(50, 100, 556, false); - - assert!(!super::content_unchanged_since_full_read( - &state, "middle\n" - )); - } - - #[test] - fn validate_edit_content_freshness_rejects_partial_read_range_without_full_file() { - let state = FileReadState { - content: "middle\n".to_string(), - timestamp_ms: 100, - start_line: 50, - end_line: 100, - total_lines: 556, - is_partial_view: false, - content_truncated: false, - }; - - assert!(validate_edit_content_freshness_against_read_state( - "src/state.js", - &state, - "different full file\n", - Some(200), - ) - .is_some()); - } - - #[test] - fn assert_file_not_unexpectedly_modified_allows_matching_full_read_after_newer_mtime() { - let state = read_state("alpha\n", 100); - - assert!(assert_file_not_unexpectedly_modified(Some(&state), "alpha\n", Some(200)).is_ok()); - } - - #[test] - fn assert_file_not_unexpectedly_modified_rejects_changed_full_read_after_newer_mtime() { - let state = read_state("alpha\n", 100); - - assert!(assert_file_not_unexpectedly_modified(Some(&state), "beta\n", Some(200)).is_err()); - } - - #[test] - fn assert_file_not_unexpectedly_modified_rejects_partial_read_after_newer_mtime() { - let state = FileReadState { - content: "middle\n".to_string(), - timestamp_ms: 100, - start_line: 50, - end_line: 100, - total_lines: 556, - is_partial_view: false, - content_truncated: false, - }; - - assert!( - assert_file_not_unexpectedly_modified(Some(&state), "full file\n", Some(200)).is_err() - ); - } - - fn read_state(content: &str, timestamp_ms: u64) -> FileReadState { - FileReadState { - content: content.to_string(), - timestamp_ms, - start_line: 1, - end_line: 1, - total_lines: 1, - is_partial_view: false, - content_truncated: false, - } - } - - #[test] - fn validate_edit_content_freshness_allows_matching_remote_content_without_mtime() { - let state = read_state("alpha\n", 100); - - assert!(validate_edit_content_freshness_against_read_state( - "src/main.rs", - &state, - "alpha\n", - None, - ) - .is_none()); - } - - #[test] - fn validate_edit_content_freshness_allows_remote_content_missing_cached_trailing_newline() { - // Regression test: the Read tool's cached content is reconstructed via - // a line-split/join that drops a trailing newline, while a remote - // SFTP re-read of the same unchanged file preserves it. Remote - // workspaces have no mtime to short-circuit the comparison, so this - // must not be reported as "no longer matches the last Read result". - let state = read_state("alpha\nbeta", 100); - - assert!(validate_edit_content_freshness_against_read_state( - "Caddyfile", - &state, - "alpha\nbeta\n", - None, - ) - .is_none()); - } - - #[test] - fn validate_edit_content_freshness_rejects_changed_remote_content_without_mtime() { - let state = read_state("alpha\n", 100); - - assert_eq!( - validate_edit_content_freshness_against_read_state( - "src/main.rs", - &state, - "beta\n", - None, - ) - .as_deref(), - Some( - "The file src/main.rs no longer matches the last Read result. Use Read again, then retry Edit." - ) - ); - } - - #[test] - fn validate_edit_content_freshness_allows_newer_mtime_when_full_read_content_matches() { - let state = read_state("alpha\n", 100); - - assert!(validate_edit_content_freshness_against_read_state( - "src/main.rs", - &state, - "alpha\n", - Some(200), - ) - .is_none()); - } - - #[test] - fn validate_edit_content_freshness_rejects_newer_mtime_when_content_differs() { - let state = read_state("alpha\n", 100); - - assert!(validate_edit_content_freshness_against_read_state( - "src/main.rs", - &state, - "beta\n", - Some(200), - ) - .is_some()); - } - - #[test] - fn validate_edit_content_freshness_rejects_changed_content_with_older_mtime() { - let state = read_state("alpha\n", 200); - - assert!(validate_edit_content_freshness_against_read_state( - "src/main.rs", - &state, - "beta\n", - Some(100), - ) - .is_some()); - } - - #[test] - fn validate_write_mtime_freshness_rejects_newer_mtime_before_content_compare() { - let state = read_state("alpha\n", 100); - - assert_eq!( - validate_write_mtime_freshness_against_read_state("src/main.rs", &state, 200) - .as_deref(), - Some("The file src/main.rs changed after it was last read. Use Read again, then retry Write.") - ); - } - - #[test] - fn validate_write_mtime_freshness_allows_older_mtime_without_content_compare() { - let state = read_state("alpha\n", 200); - - assert!( - validate_write_mtime_freshness_against_read_state("src/main.rs", &state, 100).is_none() - ); - } - - #[test] - fn validate_write_content_freshness_rejects_changed_remote_content_without_mtime() { - let state = read_state("alpha\n", 100); - - assert_eq!( - validate_write_content_freshness_against_read_state( - "src/main.rs", - &state, - "beta\n", - ) - .as_deref(), - Some( - "The file src/main.rs no longer matches the last Read result. Use Read again, then retry Write." - ) - ); - } -} diff --git a/src/crates/execution/agent-runtime/src/lib.rs b/src/crates/execution/agent-runtime/src/lib.rs index a73173eb32..167c2103d3 100644 --- a/src/crates/execution/agent-runtime/src/lib.rs +++ b/src/crates/execution/agent-runtime/src/lib.rs @@ -29,8 +29,6 @@ pub mod event_source; pub mod events; #[cfg(feature = "agent-runtime")] pub mod evidence_ledger; -#[cfg(feature = "agent-runtime")] -pub mod file_read_state; #[cfg(feature = "native-hook-settings")] pub mod native_hooks; #[cfg(feature = "agent-runtime")] @@ -48,6 +46,8 @@ pub mod prompt_markup; #[cfg(feature = "agent-runtime")] pub mod remote_file_delivery; #[cfg(feature = "agent-runtime")] +pub mod review_read_receipt; +#[cfg(feature = "agent-runtime")] pub mod runtime; #[cfg(feature = "agent-runtime")] pub mod scheduled_job; diff --git a/src/crates/execution/agent-runtime/src/review_read_receipt.rs b/src/crates/execution/agent-runtime/src/review_read_receipt.rs new file mode 100644 index 0000000000..04ccb4bfba --- /dev/null +++ b/src/crates/execution/agent-runtime/src/review_read_receipt.rs @@ -0,0 +1,213 @@ +//! Session-scoped receipts for file ranges returned during code review. + +use dashmap::DashMap; +use std::sync::Arc; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FileRevision { + pub modified_ns: u128, + pub byte_len: u64, + pub content_sha256: [u8; 32], +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ReviewReadCoverage { + pub start_line: usize, + pub end_line: usize, + pub total_lines: usize, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct ReviewReadReceipt { + revision: FileRevision, + ranges: Vec<(usize, usize)>, + total_lines: usize, +} + +#[derive(Default)] +pub struct ReviewReadReceiptStore { + session_receipts: Arc>>, +} + +impl ReviewReadReceiptStore { + pub fn new() -> Self { + Self::default() + } + + pub fn create_session(&self, session_id: &str) { + self.session_receipts + .entry(session_id.to_string()) + .or_default(); + } + + pub fn delete_session(&self, session_id: &str) { + self.session_receipts.remove(session_id); + } + + pub fn clear_session(&self, session_id: &str) { + if let Some(receipts) = self.session_receipts.get(session_id) { + receipts.clear(); + } + } + + pub fn record_review_read( + &self, + session_id: &str, + logical_path: &str, + revision: FileRevision, + start_line: usize, + end_line: usize, + total_lines: usize, + ) { + if start_line == 0 || end_line < start_line { + return; + } + + let session_receipts = self + .session_receipts + .entry(session_id.to_string()) + .or_default(); + let mut receipt = session_receipts + .entry(logical_path.to_string()) + .or_insert_with(|| ReviewReadReceipt { + revision, + ranges: Vec::new(), + total_lines, + }); + if receipt.revision != revision { + receipt.revision = revision; + receipt.ranges.clear(); + } + receipt.total_lines = total_lines; + receipt.ranges.push((start_line, end_line)); + receipt.ranges.sort_unstable_by_key(|range| range.0); + + let mut merged = Vec::<(usize, usize)>::with_capacity(receipt.ranges.len()); + for (start, end) in receipt.ranges.drain(..) { + if let Some(last) = merged.last_mut() { + if start <= last.1.saturating_add(1) { + last.1 = last.1.max(end); + continue; + } + } + merged.push((start, end)); + } + receipt.ranges = merged; + } + + pub fn review_read_coverage( + &self, + session_id: &str, + logical_path: &str, + revision: FileRevision, + start_line: usize, + limit: usize, + ) -> Option { + if start_line == 0 || limit == 0 { + return None; + } + let session_receipts = self.session_receipts.get(session_id)?; + let receipt = session_receipts.get(logical_path)?; + if receipt.revision != revision || start_line > receipt.total_lines { + return None; + } + let end_line = start_line + .saturating_add(limit.saturating_sub(1)) + .min(receipt.total_lines); + receipt + .ranges + .iter() + .any(|(covered_start, covered_end)| { + *covered_start <= start_line && *covered_end >= end_line + }) + .then_some(ReviewReadCoverage { + start_line, + end_line, + total_lines: receipt.total_lines, + }) + } +} + +#[cfg(test)] +mod tests { + use super::{FileRevision, ReviewReadCoverage, ReviewReadReceiptStore}; + + fn revision(hash_byte: u8) -> FileRevision { + FileRevision { + modified_ns: 100, + byte_len: 4096, + content_sha256: [hash_byte; 32], + } + } + + #[test] + fn review_read_receipt_store_scopes_entries_by_session() { + let store = ReviewReadReceiptStore::new(); + let revision = revision(1); + store.create_session("session-a"); + store.create_session("session-b"); + store.record_review_read("session-a", "src/lib.rs", revision, 1, 20, 20); + + assert!(store + .review_read_coverage("session-a", "src/lib.rs", revision, 1, 20) + .is_some()); + assert!(store + .review_read_coverage("session-b", "src/lib.rs", revision, 1, 20) + .is_none()); + } + + #[test] + fn review_read_receipt_covers_only_previously_returned_lines() { + let store = ReviewReadReceiptStore::new(); + let revision = revision(1); + store.record_review_read("review-session", "src/large.rs", revision, 1, 2000, 3000); + + assert_eq!( + store.review_read_coverage("review-session", "src/large.rs", revision, 1403, 27), + Some(ReviewReadCoverage { + start_line: 1403, + end_line: 1429, + total_lines: 3000, + }) + ); + assert!(store + .review_read_coverage("review-session", "src/large.rs", revision, 2001, 20) + .is_none()); + } + + #[test] + fn review_read_receipt_merges_ranges_and_invalidates_on_revision_change() { + let store = ReviewReadReceiptStore::new(); + let original = revision(1); + store.record_review_read("review-session", "src/lib.rs", original, 1, 100, 300); + store.record_review_read("review-session", "src/lib.rs", original, 101, 200, 300); + + assert!(store + .review_read_coverage("review-session", "src/lib.rs", original, 50, 151) + .is_some()); + assert!(store + .review_read_coverage("review-session", "src/lib.rs", revision(2), 50, 151) + .is_none()); + } + + #[test] + fn review_read_receipt_store_clear_and_delete_drop_session_entries() { + let store = ReviewReadReceiptStore::new(); + let revision = revision(1); + store.record_review_read("session-a", "src/lib.rs", revision, 1, 20, 20); + store.record_review_read("session-b", "src/lib.rs", revision, 1, 20, 20); + + store.clear_session("session-a"); + assert!(store + .review_read_coverage("session-a", "src/lib.rs", revision, 1, 20) + .is_none()); + assert!(store + .review_read_coverage("session-b", "src/lib.rs", revision, 1, 20) + .is_some()); + + store.delete_session("session-b"); + assert!(store + .review_read_coverage("session-b", "src/lib.rs", revision, 1, 20) + .is_none()); + } +} diff --git a/src/crates/execution/tool-contracts/AGENTS.md b/src/crates/execution/tool-contracts/AGENTS.md index 12b6f88a5e..65800deefa 100644 --- a/src/crates/execution/tool-contracts/AGENTS.md +++ b/src/crates/execution/tool-contracts/AGENTS.md @@ -21,7 +21,7 @@ the product tool runtime. registration stay outside this crate until a reviewed owner move proves behavior equivalence. - Do not move `ToolUseContext`, concrete tools, workspace services, cancellation - tokens, session file-read state storage, tool-result filesystem writes, + tokens, code-review read receipt storage, tool-result filesystem writes, state update side effects, snapshot decoration, collapsed unlock state, product registry snapshot access, or concrete `GetToolSpecTool` execution here without an owner design and equivalence tests. @@ -39,7 +39,6 @@ the product tool runtime. ```bash cargo test --locked -p openbitfun-agent-tools --no-default-features -cargo test --locked -p openbitfun-agent-tools --no-default-features --test tool_contracts file_read_freshness_ cargo test --locked -p openbitfun-agent-tools --no-default-features --features acp-bridge --test tool_contracts acp_external_agent_bridge_preserves_tool_contract cargo test --locked -p openbitfun-agent-tools --no-default-features --features mcp-bridge --test tool_contracts mcp_tool_bridge cargo test --locked -p openbitfun-agent-tools --no-default-features --features computer-use-contract --lib computer_use:: diff --git a/src/crates/execution/tool-contracts/src/file_read_freshness.rs b/src/crates/execution/tool-contracts/src/file_read_freshness.rs deleted file mode 100644 index 2ae9e554a6..0000000000 --- a/src/crates/execution/tool-contracts/src/file_read_freshness.rs +++ /dev/null @@ -1,53 +0,0 @@ -//! Pure file-read freshness rules for Read/Edit/Write guardrails. - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct FileReadFreshnessFacts<'a> { - pub content: &'a str, - pub timestamp_ms: u64, - pub is_full_file_read: bool, -} - -/// Normalize file content for freshness comparison only (not for diffing or -/// content substitution). -/// -/// The cached "last Read result" content is reconstructed from formatted -/// `cat -n`-style tool output via a line-split/join, which always drops a -/// trailing newline even when the real file ends with one. A fresh re-read -/// (local `fs::read_to_string` or remote SFTP `read_file_text`) preserves it. -/// Without normalizing this away, every full-file Edit/Write on a file that -/// ends with a newline (the common case) would look "changed" purely from -/// that reconstruction gap. This applies equally to local and remote reads. -pub fn normalize_tool_file_content(content: &str) -> String { - let normalized = if content.contains("\r\n") { - content.replace("\r\n", "\n") - } else { - content.to_string() - }; - normalized.trim_end_matches('\n').to_string() -} - -pub fn file_read_facts_content_matches( - read_facts: FileReadFreshnessFacts<'_>, - current_content: &str, -) -> bool { - read_facts.is_full_file_read - && normalize_tool_file_content(current_content) - == normalize_tool_file_content(read_facts.content) -} - -pub fn file_read_facts_are_fresh( - read_facts: FileReadFreshnessFacts<'_>, - current_content: &str, - current_mtime_ms: Option, -) -> bool { - // A known content difference is stronger evidence than timestamps: SFTP - // timestamps can have second precision, and any filesystem can restore an - // older mtime while changing the bytes. - if read_facts.is_full_file_read { - return file_read_facts_content_matches(read_facts, current_content); - } - - // Partial reads retain their existing weaker guarantee; the unobserved - // portion cannot be compared with the cached content. - current_mtime_ms.is_none_or(|mtime| mtime <= read_facts.timestamp_ms) -} diff --git a/src/crates/execution/tool-contracts/src/lib.rs b/src/crates/execution/tool-contracts/src/lib.rs index 18c9c01a33..02770c4a9b 100644 --- a/src/crates/execution/tool-contracts/src/lib.rs +++ b/src/crates/execution/tool-contracts/src/lib.rs @@ -12,7 +12,6 @@ pub mod deferred_tool; pub mod element_token; pub mod execution_gate; pub mod file_guidance; -pub mod file_read_freshness; pub mod framework; pub mod input_validator; #[cfg(feature = "mcp-bridge")] @@ -45,10 +44,6 @@ pub use execution_gate::{ pub use file_guidance::{ file_tool_guidance_message, is_file_tool_guidance_message, FILE_TOOL_GUIDANCE_PREFIX, }; -pub use file_read_freshness::{ - file_read_facts_are_fresh, file_read_facts_content_matches, normalize_tool_file_content, - FileReadFreshnessFacts, -}; pub use framework::{ build_get_tool_spec_assistant_detail, build_get_tool_spec_catalog_description, build_get_tool_spec_catalog_description_from_provider, build_get_tool_spec_description, diff --git a/src/crates/execution/tool-contracts/tests/tool_contracts.rs b/src/crates/execution/tool-contracts/tests/tool_contracts.rs index 5622d8ad7a..fae01a96d9 100644 --- a/src/crates/execution/tool-contracts/tests/tool_contracts.rs +++ b/src/crates/execution/tool-contracts/tests/tool_contracts.rs @@ -64,10 +64,6 @@ use openbitfun_agent_tools::{ tool_result_is_persisted_output, PersistedToolOutput, ToolResultPersistenceCandidate, FILE_TOOL_GUIDANCE_PREFIX, PERSISTED_OUTPUT_TAG, TOOL_RESULT_PREVIEW_CHARS, }; -use openbitfun_agent_tools::{ - file_read_facts_are_fresh, file_read_facts_content_matches, normalize_tool_file_content, - FileReadFreshnessFacts, -}; use openbitfun_agent_tools::{ materialize_static_tool_provider_groups, ContextualToolManifestItem, DynamicToolDescriptor, DynamicToolProvider, GetToolSpecCatalogProvider, PortResult, PortableToolContextProvider, @@ -942,98 +938,6 @@ fn file_tool_guidance_marker_is_provider_neutral() { assert!(!is_file_tool_guidance_message("Read the file first")); } -#[test] -fn file_read_freshness_policy_preserves_read_edit_write_guardrails() { - let full_read = FileReadFreshnessFacts { - content: "alpha\r\n", - timestamp_ms: 100, - is_full_file_read: true, - }; - - assert_eq!(normalize_tool_file_content("alpha\r\n"), "alpha"); - assert!(file_read_facts_content_matches(full_read, "alpha\n")); - assert!(file_read_facts_are_fresh(full_read, "alpha\n", Some(200))); - assert!(!file_read_facts_are_fresh(full_read, "beta\n", Some(200))); - assert!(!file_read_facts_are_fresh(full_read, "beta\n", Some(50))); - assert!(!file_read_facts_are_fresh(full_read, "beta\n", None)); - - let partial_read = FileReadFreshnessFacts { - content: "middle\n", - timestamp_ms: 100, - is_full_file_read: false, - }; - assert!(!file_read_facts_content_matches(partial_read, "middle\n")); - assert!(!file_read_facts_are_fresh( - partial_read, - "full file\n", - Some(200) - )); - assert!(file_read_facts_are_fresh(partial_read, "full file\n", None)); -} - -#[test] -fn file_read_freshness_full_read_rejects_same_tick_and_restored_mtime_changes() { - let read = FileReadFreshnessFacts { - content: "alpha\nbeta", - timestamp_ms: 1_700_000_000_000, - is_full_file_read: true, - }; - for modified in [ - Some(read.timestamp_ms), - Some(read.timestamp_ms - 1_000), - None, - ] { - assert!(!file_read_facts_are_fresh(read, "alpha\nzeta\n", modified)); - assert!(file_read_facts_are_fresh( - read, - "alpha\r\nbeta\r\n", - modified - )); - } - let partial = FileReadFreshnessFacts { - is_full_file_read: false, - ..read - }; - assert!(file_read_facts_are_fresh( - partial, - "unobserved content", - Some(read.timestamp_ms) - )); - assert!(!file_read_facts_are_fresh( - partial, - "unobserved content", - Some(read.timestamp_ms + 1_000) - )); -} - -#[test] -fn file_read_freshness_tolerates_read_tool_trailing_newline_reconstruction_gap() { - // The cached "last Read result" content is rebuilt from cat -n-style - // output via a line-split/join, which drops a trailing newline even when - // the file on disk ends with one. Every full-file Edit/Write on a - // trailing-newline file must still be considered fresh. - let cached_without_trailing_newline = FileReadFreshnessFacts { - content: "alpha\nbeta", - timestamp_ms: 100, - is_full_file_read: true, - }; - - assert!(file_read_facts_content_matches( - cached_without_trailing_newline, - "alpha\nbeta\n" - )); - assert!(file_read_facts_are_fresh( - cached_without_trailing_newline, - "alpha\nbeta\n", - None - )); - assert!(!file_read_facts_are_fresh( - cached_without_trailing_newline, - "alpha\ngamma\n", - None - )); -} - #[test] fn persisted_tool_output_message_keeps_reference_preview_and_metadata_shape() { let rendered = build_persisted_tool_output_message( diff --git a/src/crates/execution/tool-execution/src/fs/edit_file.rs b/src/crates/execution/tool-execution/src/fs/edit_file.rs index 0c3fcc5215..3a5f088a62 100644 --- a/src/crates/execution/tool-execution/src/fs/edit_file.rs +++ b/src/crates/execution/tool-execution/src/fs/edit_file.rs @@ -440,7 +440,7 @@ pub fn apply_edit_to_content( } Err(format!( - "{}\n{}", + "{}\nPossible causes: the file might be changed externally after you inspected it, or old_string was generated incorrectly (for example, with line-number prefixes, different whitespace or indentation, or truncated output).\nInspect the current target region, correct old_string to match the current file content exactly, then retry.\n{}", last_error, build_not_found_diagnostics(content, old_string) )) @@ -622,21 +622,6 @@ mod tests { assert!(error.contains("second block")); } - #[test] - fn apply_edit_to_content_not_found_includes_nearby_diagnostics() { - let error = apply_edit_to_content( - "fn main() {\n println!(\"hello\");\n}\n", - "println!(\"goodbye\");", - "println!(\"hi\");", - false, - ) - .expect_err("missing text should fail"); - - assert!(error.contains("old_string not found in file.")); - assert!(error.contains("[nearby content around line 2]")); - assert!(error.contains("println!(\"hello\");")); - } - #[test] fn apply_edit_to_content_not_found_calls_out_read_prefixes() { let error = apply_edit_to_content(