Replace custom build tool clock with mockable Clock (#194)

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,

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<std::collections::VecDeque<Instant>>,
+}
+
+#[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)
+    }
+}

src/tools/builder/core/wrapper.rs

@@ -7,43 +7,31 @@
 //! `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<dyn SoftwareBuilder>,
-    clock: Arc<dyn Clock>,
+    clock: Arc<dyn MonotonicClock>,
 }
 
 impl BuildSoftwareTool {
     /// Wraps a [`SoftwareBuilder`] for use as a [`NativeTool`].
     pub fn new(builder: Arc<dyn SoftwareBuilder>) -> Self {
         Self {
             builder,
-            clock: Arc::new(SystemClock),
+            clock: Arc::new(StdMonotonicClock),
         }
     }
 
     #[cfg(test)]
-    fn new_with_clock(builder: Arc<dyn SoftwareBuilder>, clock: Arc<dyn Clock>) -> Self {
+    pub(crate) fn new_with_clock(
+        builder: Arc<dyn SoftwareBuilder>,
+        clock: Arc<dyn MonotonicClock>,
+    ) -> Self {
         Self { builder, clock }
     }
 
@@ -193,7 +181,10 @@ impl NativeTool for BuildSoftwareTool {
             "phases": result.logs.iter().map(|l| format!("{:?}: {}", l.phase, l.message)).collect::<Vec<_>>()
         });
 
-        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.

src/tools/builder/core/wrapper_tests.rs

@@ -1,29 +1,17 @@
 //! 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;
 
 type AnalyzeResult = dyn Fn() -> Result<BuildRequirement, AgentToolError> + Send + Sync;
 type BuildResultFn = dyn Fn(&BuildRequirement) -> Result<BuildResult, AgentToolError> + 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<T: std::fmt::Debug>(result: Result<T, ToolError>, 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<dyn SoftwareBuilder>,
+    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!(