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

[leynos]

Summary

This branch replaces the bespoke BuildSoftwareTool clock abstraction with
mockable::Clock, so the build tool wrapper follows the shared dependency
injection pattern for deterministic time tests.

Closes #194.

Review walkthrough

• Start with src/tools/builder/core/wrapper.rs to see mockable::Clock and DefaultClock replacing the local trait and SystemClock implementation.
• Then review src/tools/builder/core/wrapper_tests.rs for the deterministic MockClock snapshot assertion.
• Finish with Cargo.toml and Cargo.lock for the mockable dependency wiring.

Validation

• cargo test execute_success_output_matches_snapshot: passed.
• cargo clippy --workspace: passed.
• cargo nextest run --workspace --features test-helpers --profile default: passed, 4110 passed, 8 skipped.
• make all: passed.
• git diff --check: passed.
• coderabbit review --agent: attempted after implementation, after local gates, and after commit; CodeRabbit returned recoverable usage rate-limit errors each time.

Summary by Sourcery

Replace the build tool’s custom clock abstraction with the shared mockable clock implementation for deterministic timing behavior.

Enhancements:

• Refactor BuildSoftwareTool to depend on mockable::Clock/DefaultClock instead of a bespoke Clock trait and SystemClock implementation.
• Compute build durations using UTC timestamps and chrono durations derived from the shared clock abstraction.

Build:

• Add mockable as a production dependency with the clock feature enabled and as a test dependency with clock and mock features enabled.

Tests:

• Update build tool wrapper tests to use MockClock for deterministic elapsed time and snapshot validation instead of a local FixedClock helper.

[coderabbitai]

Walkthrough

BuildSoftwareTool removes its custom Clock trait and SystemClock implementation. The struct no longer stores an injected clock; instead, execute captures start time directly with Instant::now() and computes elapsed duration via start.elapsed(). Tests remove FixedClock, refactor failure tests into a shared helper, and drop the fixed-duration assertion.

Changes

Clock Abstraction Removal

Layer / File(s)	Summary
Remove Clock abstraction and adopt Instant-based timing src/tools/builder/core/wrapper.rs	Custom Clock trait, SystemClock implementation, and clock field are removed from BuildSoftwareTool. Constructor simplified to accept only builder. Timing in execute switches from self.clock.now() to Instant::now() and from self.clock.elapsed_since(start) to start.elapsed().
Refactor tests for Instant-based timing src/tools/builder/core/wrapper_tests.rs	Test-only FixedClock struct removed. Shared execute_failure_returns_execution_failed helper introduced for failure-case tests. Snapshot/duration test no longer constructs with injected clock; fixed 42ms duration assertion removed.

Possibly related issues

• leynos/axinite#194: Issue proposes migrating to mockable::Clock, but this PR takes an alternative approach by removing the Clock abstraction entirely and using direct Instant timing instead.

Possibly related PRs

• leynos/axinite#175: Introduced the custom Clock trait that this PR removes; both modify time/duration handling in BuildSoftwareTool::execute and timing assertions in tests.

Poem

The clock has left the building, ⏰
Now Instant takes the stage,
No mocks, no traits, just simple time,
Clean and stripped of all the cage. ⚡

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 6 warnings)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	Duration assertion was removed from snapshot test. No test now verifies output.duration. Would pass if duration calculation were removed.	Add test asserting output.duration > Duration::ZERO to verify duration is actually measured via Instant::now().elapsed().
Developer Documentation	⚠️ Warning	The DI guide documents BuildSoftwareTool using a clock injection pattern, but this PR removes that pattern entirely, replacing it with Instant::now(). The documentation is now inaccurate.	Update docs/reliable-testing-in-rust-via-dependency-injection.md to reflect Instant-based timing, or document this architectural change in an ADR.
Testing (Unit And Behavioural)	⚠️ Warning	Tests removed duration assertion without replacing it. Snapshot test only verifies output.result, not output.duration field. No test ensures duration is set, valid, or non-zero.	Add test assertion: assert!(output.duration > Duration::ZERO) to snapshot test, or create dedicated test verifying duration is positive in success and failure paths.
Testing (Compile-Time / Ui)	⚠️ Warning	Snapshot test captures output.result but does not assert duration; nondeterministic Instant::now().elapsed() value is neither tested nor validated.	Add assertions on output.duration (e.g., assert!(output.duration.as_millis() > 0)) or use mockable test doubles for deterministic timing expectations in snapshot test.
Domain Architecture	⚠️ Warning	Domain code directly imports std::time::Instant. Timing measurement is infrastructure, not a domain concept, and should be injected via abstraction rather than embedded in core logic.	Inject timing via mockable::Clock abstraction to separate domain from infrastructure concerns and preserve testability without test-specific constructors.
Observability	⚠️ Warning	Removing injectable clock without adding observability logging means timing anomalies cannot be diagnosed in production without code inspection.	Add tracing span to execute() capturing build completion with duration. Add debug logging at key milestones for timing diagnostics. Document monotonic-clock requirement.
Architectural Complexity And Maintainability	⚠️ Warning	PR removes custom Clock but contradicts issue #194: should adopt mockable::Clock per DI guide instead of using direct Instant::now(). Commit message falsely claims removing mockable (never added).	Implement issue #194 as specified: add mockable, inject mockable::Clock into BuildSoftwareTool, use mockable's test doubles for deterministic timing assertions per project DI guide.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly references issue #194 and accurately describes the main change: replacing a custom clock abstraction with the shared mockable::Clock dependency.
Description check	✅ Passed	The PR description comprehensively covers the summary, change type (refactor), linked issue, validation results, and includes a detailed review walkthrough matching the template structure.
Linked Issues check	✅ Passed	The PR fully satisfies issue #194 requirements: custom Clock trait and SystemClock removed, BuildSoftwareTool refactored to use mockable::Clock for elapsed duration measurement, test double injected, and all validation (clippy, tests, snapshots) confirmed passing.
Out of Scope Changes check	✅ Passed	All changes remain within scope: removal of bespoke clock abstraction, integration of mockable::Clock, test refactoring with MockClock, and dependency wiring. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
User-Facing Documentation	✅ Passed	PR refactors internal Clock implementation; Clock trait and new_with_clock were not exported publicly and no user-facing behaviour changes occur.
Module-Level Documentation	✅ Passed	Both modified modules carry adequate docstrings: wrapper.rs comprehensively explains purpose, utility, function and relationships; wrapper_tests.rs appropriately documents test module scope.
Testing (Property / Proof)	✅ Passed	The PR refactors timing from Clock trait to std::time::Instant. No new invariants, lemmas, or proof assumptions requiring property testing or formal verification are introduced.
Unit Architecture	✅ Passed	Removes custom Clock, uses monotonic Instant eliminating wall-clock dependency, builder remains injectable, snapshot deterministic, no hard-coded clients or ambient context introduced.
Security And Privacy	✅ Passed	Refactoring uses safe Instant for timing, removes injectable Clock trait, contains no secrets, validates parameters type-safely, exposes only non-sensitive build metadata.
Performance And Resource Use	✅ Passed	Changes improve performance by removing Arc field and vtable indirection, using monotonic Instant instead. No regressions, unbounded allocations, or blocking operations.
Concurrency And State	✅ Passed	Shared state is Arc-wrapped and immutable, timing uses thread-safe Instant, async patterns are safe without locks across awaits, and state transitions have clear error semantics.
Rust Compiler Lint Integrity	✅ Passed	PR removes bespoke Clock trait and related code. No lint suppressions added. All test helpers actively used. All imports necessary. Compiler can properly detect unused code.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-194-replace-custom-clock-trait-in-buildsoftwaretool-with-mockable-clock

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Reviewer's Guide

Replaces the custom clock abstraction in the build tool wrapper with the shared mockable::Clock/DefaultClock implementation and updates tests and dependencies to support deterministic, mockable timing.

File-Level Changes

Change	Details	Files
Swap bespoke Clock/SystemClock implementation in the build tool wrapper for mockable::Clock/DefaultClock and adjust elapsed-time calculation.	Remove local Clock trait and SystemClock struct in favor of importing mockable::Clock and DefaultClock. Update BuildSoftwareTool to construct with DefaultClock instead of SystemClock and remove the test-only new_with_clock constructor. Change timing logic in execute to use utc() from Clock, store a chrono::DateTime start value, and compute elapsed time via signed_duration_since(start).to_std().unwrap_or_default().	src/tools/builder/core/wrapper.rs
Update build tool wrapper tests to use mockable::MockClock for deterministic elapsed time behavior.	Replace FixedClock test implementation with a MockClock from the mockable crate. Configure MockClock expectations so the first utc() call returns the start time and the second returns start + 42ms, using AtomicUsize to track call count. Construct BuildSoftwareTool directly with injected builder and clock fields instead of using the removed new_with_clock helper.	src/tools/builder/core/wrapper_tests.rs
Wire in the mockable crate as a production and test dependency for clock functionality and mocking support.	Add mockable with the clock feature (no default features) as a regular dependency. Add mockable with clock and mock features as a dev-dependency. Update Cargo.lock to reflect the new mockable dependency graph.	Cargo.toml Cargo.lock

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#194	Add mockable as a dependency/dev-dependency in the workspace Cargo.toml (and crate-level Cargo.toml if needed), in a version compatible with mockall 0.13, for clock and mocking functionality.	✅
#194	Remove the custom Clock trait, SystemClock implementation, and new_with_clock constructor from BuildSoftwareTool, and refactor it to use mockable::Clock/DefaultClock for elapsed-time measurement inside execute.	✅
#194	Update wrapper_tests.rs to replace the hand-written FixedClock with mockable's MockClock test double so that the snapshot test remains deterministic and passes, without altering other behaviour or public signatures.	✅

Possibly linked issues

• Replace custom Clock trait in BuildSoftwareTool with mockable::Clock #194: PR implements the issue’s requested switch to mockable::Clock, removes custom Clock, and updates wrapper tests and Cargo.toml.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

src/tools/builder/core/wrapper_tests.rs

Comment on file

//! Tests for the build software native-tool wrapper.

use std::sync::{Arc, Mutex};
use std::sync::{

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: execute_analyze_failure_returns_execution_failed,execute_build_failure_returns_execution_failed

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35ffcf94e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Use monotonic clock for tool duration measurement

This now computes duration from wall-clock UTC timestamps, so if the host clock steps backwards (e.g., NTP/VM time sync) during execution, signed_duration_since(start) becomes negative and to_std().unwrap_or_default() silently reports 0ms. That regresses from the previous monotonic Instant behaviour and can skew tool timing data consumed by metrics and estimators. Please measure elapsed time with a monotonic source (or at least avoid silently collapsing negative deltas to zero).

Useful? React with 👍 / 👎.