diff --git a/src/tools/builder/core.rs b/src/tools/builder/core.rs index d811d33c8..3e1c58784 100644 --- a/src/tools/builder/core.rs +++ b/src/tools/builder/core.rs @@ -49,10 +49,12 @@ use crate::tools::tool::{ mod build_loop; mod builder_impl; +mod clock; mod domain; mod setup; mod wrapper; +pub(crate) use clock::{MonotonicClock, StdMonotonicClock}; pub use domain::{ BuildLog, BuildPhase, BuildRequirement, BuildResult, BuilderConfig, ExecutionCommand, Language, NativeSoftwareBuilder, ProjectName, SoftwareBuilder, SoftwareType, diff --git a/src/tools/builder/core/clock.rs b/src/tools/builder/core/clock.rs new file mode 100644 index 000000000..01cf13932 --- /dev/null +++ b/src/tools/builder/core/clock.rs @@ -0,0 +1,47 @@ +//! Monotonic clocks for measuring build tool execution duration. + +use std::time::Instant; + +/// Clock abstraction for monotonic elapsed-time measurements. +pub trait MonotonicClock: Send + Sync { + /// Returns the current monotonic instant. + fn now(&self) -> Instant; +} + +/// Monotonic clock backed by [`Instant::now`]. +pub struct StdMonotonicClock; + +impl MonotonicClock for StdMonotonicClock { + fn now(&self) -> Instant { + Instant::now() + } +} + +#[cfg(test)] +pub struct FixedMonotonicClock { + instants: std::sync::Mutex>, +} + +#[cfg(test)] +impl FixedMonotonicClock { + pub fn with_elapsed(elapsed: std::time::Duration) -> Self { + let start = Instant::now(); + Self { + instants: std::sync::Mutex::new(std::collections::VecDeque::from([ + start, + start + elapsed, + ])), + } + } +} + +#[cfg(test)] +impl MonotonicClock for FixedMonotonicClock { + fn now(&self) -> Instant { + self.instants + .lock() + .expect("fixed monotonic clock mutex should not be poisoned") + .pop_front() + .unwrap_or_else(Instant::now) + } +} diff --git a/src/tools/builder/core/wrapper.rs b/src/tools/builder/core/wrapper.rs index 58306df9e..58bf64eb3 100644 --- a/src/tools/builder/core/wrapper.rs +++ b/src/tools/builder/core/wrapper.rs @@ -7,30 +7,15 @@ //! `language` parameters are resolved through private helpers that //! centralize fallback and error-message logic. +#[cfg(test)] +use super::clock; use super::*; - -trait Clock: Send + Sync { - fn now(&self) -> std::time::Instant; - - fn elapsed_since(&self, start: std::time::Instant) -> std::time::Duration; -} - -struct SystemClock; - -impl Clock for SystemClock { - fn now(&self) -> std::time::Instant { - std::time::Instant::now() - } - - fn elapsed_since(&self, start: std::time::Instant) -> std::time::Duration { - start.elapsed() - } -} +use super::{MonotonicClock, StdMonotonicClock}; /// Tool that allows the agent to build software on demand. pub struct BuildSoftwareTool { builder: Arc, - clock: Arc, + clock: Arc, } impl BuildSoftwareTool { @@ -38,12 +23,15 @@ impl BuildSoftwareTool { pub fn new(builder: Arc) -> Self { Self { builder, - clock: Arc::new(SystemClock), + clock: Arc::new(StdMonotonicClock), } } #[cfg(test)] - fn new_with_clock(builder: Arc, clock: Arc) -> Self { + pub(crate) fn new_with_clock( + builder: Arc, + clock: Arc, + ) -> Self { Self { builder, clock } } @@ -193,7 +181,10 @@ impl NativeTool for BuildSoftwareTool { "phases": result.logs.iter().map(|l| format!("{:?}: {}", l.phase, l.message)).collect::>() }); - Ok(ToolOutput::success(output, self.clock.elapsed_since(start))) + Ok(ToolOutput::success( + output, + self.clock.now().duration_since(start), + )) } /// Approval is required unless the surrounding job auto-approves tools. diff --git a/src/tools/builder/core/wrapper_tests.rs b/src/tools/builder/core/wrapper_tests.rs index bb8ab573f..841b2eb7c 100644 --- a/src/tools/builder/core/wrapper_tests.rs +++ b/src/tools/builder/core/wrapper_tests.rs @@ -1,8 +1,10 @@ //! Tests for the build software native-tool wrapper. use std::sync::{Arc, Mutex}; +use std::time::Duration; use super::super::domain::SoftwareBuilderFuture; +use super::clock::FixedMonotonicClock; use super::*; use insta::assert_snapshot; use rstest::rstest; @@ -10,20 +12,6 @@ use rstest::rstest; type AnalyzeResult = dyn Fn() -> Result + Send + Sync; type BuildResultFn = dyn Fn(&BuildRequirement) -> Result + Send + Sync; -struct FixedClock { - elapsed: std::time::Duration, -} - -impl Clock for FixedClock { - fn now(&self) -> std::time::Instant { - std::time::Instant::now() - } - - fn elapsed_since(&self, _start: std::time::Instant) -> std::time::Duration { - self.elapsed - } -} - fn assert_invalid_parameters(result: Result, expected_msg: &str) { match result.expect_err("expected invalid parameters error") { ToolError::InvalidParameters(msg) => assert_eq!(msg, expected_msg), @@ -248,41 +236,38 @@ async fn execute_missing_description_returns_error() { assert_invalid_parameters(result, "missing 'description'"); } -#[tokio::test] -async fn execute_analyze_failure_returns_execution_failed() { - let builder = FakeSoftwareBuilder::analyze_error("analysis exploded"); - let tool = BuildSoftwareTool::new(Arc::new(builder)); - +async fn execute_failure_returns_execution_failed( + builder: Arc, + expected_msg: &str, +) { + let tool = BuildSoftwareTool::new(builder); let result = tool .execute( - serde_json::json!({ - "description": "build a test tool", - }), + serde_json::json!({ "description": "build a test tool" }), &JobContext::default(), ) .await; + assert_execution_failed(result, expected_msg); +} - assert_execution_failed( - result, +#[tokio::test] +async fn execute_analyze_failure_returns_execution_failed() { + let builder = FakeSoftwareBuilder::analyze_error("analysis exploded"); + execute_failure_returns_execution_failed( + Arc::new(builder), "Analysis failed: Tool builder failed: analysis exploded", - ); + ) + .await; } #[tokio::test] async fn execute_build_failure_returns_execution_failed() { let builder = FakeSoftwareBuilder::build_error(test_requirement(), "build exploded"); - let tool = BuildSoftwareTool::new(Arc::new(builder)); - - let result = tool - .execute( - serde_json::json!({ - "description": "build a test tool", - }), - &JobContext::default(), - ) - .await; - - assert_execution_failed(result, "Build failed: Tool builder failed: build exploded"); + execute_failure_returns_execution_failed( + Arc::new(builder), + "Build failed: Tool builder failed: build exploded", + ) + .await; } #[rstest] @@ -314,7 +299,10 @@ async fn execute_valid_params_returns_success_output() { let requirement = test_requirement(); let build_result = test_build_result(requirement.clone()); let builder = FakeSoftwareBuilder::success(requirement, build_result); - let tool = BuildSoftwareTool::new(Arc::new(builder)); + let tool = BuildSoftwareTool::new_with_clock( + Arc::new(builder), + Arc::new(FixedMonotonicClock::with_elapsed(Duration::from_millis(1))), + ); let output = tool .execute( @@ -326,6 +314,10 @@ async fn execute_valid_params_returns_success_output() { .await .expect("expected execute to return successful output"); + assert!( + output.duration > Duration::ZERO, + "duration must be positive" + ); assert_eq!(output.result["success"], true); let captured = execute_capturing_requirement(serde_json::json!({ @@ -341,10 +333,10 @@ async fn execute_success_output_matches_snapshot() { let requirement = test_requirement(); let build_result = test_build_result(requirement.clone()); let builder = FakeSoftwareBuilder::success(requirement, build_result); - let clock = Arc::new(FixedClock { - elapsed: std::time::Duration::from_millis(42), - }); - let tool = BuildSoftwareTool::new_with_clock(Arc::new(builder), clock); + let tool = BuildSoftwareTool::new_with_clock( + Arc::new(builder), + Arc::new(FixedMonotonicClock::with_elapsed(Duration::from_millis(42))), + ); let output = tool .execute( @@ -356,7 +348,11 @@ async fn execute_success_output_matches_snapshot() { .await .expect("expected execute to return successful output"); - assert_eq!(output.duration, std::time::Duration::from_millis(42)); + assert_eq!( + output.duration, + Duration::from_millis(42), + "duration must reflect the clock seam, not wall time" + ); assert_eq!(output.cost, None); assert_eq!(output.raw, None); assert_snapshot!(