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(