Skip to content

Add the clock provider seam to the stdlib time module (7.1.1) - #696

Merged
wafflecat-df12 merged 54 commits into
mainfrom
7-1-1-clock-provider-seam
Sep 24, 2026
Merged

wafflecat-df12 merged 54 commits into
mainfrom
7-1-1-clock-provider-seam

Conversation

@leynos

@leynos leynos commented Sep 8, 2026 •

Copy link
Copy Markdown
Owner

Summary

Implements roadmap item 7.1.1, which makes the Netsuke stdlib now() Jinja
function testable by reading its instant through an injected clock provider
instead of calling OffsetDateTime::now_utc() directly.

Plan and delivery record:
docs/execplans/7-1-1-clock-provider-seam.md

What this delivers

A caller that builds a StdlibConfig may supply a ClockProvider, and every
now() call in that Jinja environment returns exactly that instant. A caller
that supplies nothing keeps today's behaviour precisely. There is no
user-visible change: a manifest author sees identical now() behaviour before
and after.

The seam is a prerequisite refactor for the Netsukefile testing framework
(roadmap phase 7), specified by
technical design §5.2,
and is deliberately scoped as a deliverable in its own right — no netsuke test command, no test dialect, no src/testing module.

Where it landed:

  • src/stdlib/time/clock.rs — ClockProvider, system_clock(),
    fixed_clock(), and the private WallClock container that normalizes every
    read to UTC and labels the clock in Debug output.
  • src/stdlib/time/mod.rs — now() reads through the injected clock and
    re-expresses the instant for an explicit offset=.
  • src/stdlib/config/mod.rs — the clock's single owner; with_clock is the
    injection point.
  • src/stdlib/register.rs — register_functions captures the provider (the
    provider, not an instant) at registration; register_manifest_query receives
    no clock and keeps its refusing now stub.
  • Coverage: src/stdlib/time/clock_tests.rs, the stdlib::time unit tests,
    tests/std_filter_tests/time_functions.rs (through the real registration
    path), and the stdlib_time.feature BDD scenarios.

One follow-on extraction sits outside the seam.
tests/bdd/steps/stdlib/rendering.rs now builds its StdlibConfig in a private
configure_stdlib helper: applying the clock inside
render_template_with_context tripped CodeScene's Complex Method rule
(10.00 → 9.69 on the file), and the split takes the function from cyclomatic
complexity 10 to 3, with the helper's 8 under the threshold. Option order, the
four error-context strings, and rendering semantics are unchanged.

Design decisions worth a reviewer's attention

  • Port shape. ClockProvider = Arc<dyn Fn() -> OffsetDateTime + Send + Sync>,
    the EnvReader shape. ADR-008 justifies that shape by MiniJinja's Send + Sync requirement, but that argument is necessary and not sufficient — a
    Box satisfies it too. The decisive constraint is that StdlibConfig
    derives Clone, which Box<dyn Fn> cannot provide (D1).
  • The strongest alternative was a resolved-value enum in the
    HomeDirectory shape (Ambient / Fixed(OffsetDateTime)), which would
    derive Debug and Clone for free. It is rejected in D10 — though note the
    first draft rejected it for the wrong reason, claiming the enum made the
    per-call negative control unwriteable. It does not; an enum could carry a
    Sequence variant. The surviving argument is cohesion: preserving that
    control under an enum means adding a test-only variant to a production type,
    forcing every match site to service a case production never takes.
  • A WallClock newtype absorbs the Debug problem. StdlibConfig derives
    Debug and Arc<dyn Fn> does not implement it; a one-field newtype with a
    handwritten impl confines the boilerplate. It is named WallClock rather than
    Clock because Clock already names a monotonic clock generic in
    src/runner/process/mod.rs, alongside two other MonotonicClock spellings.
    The design document did not name this container; §5.2 now records it.
  • Two crates that look like ready-made answers are dead ends.
    mockable::Clock is chrono-typed and sits behind a feature this workspace
    does not enable; monotony abstracts monotonic elapsed time only and has no
    wall-clock type. Both were checked against published API docs (D3).
  • ADR-008's jurisdiction is extended, and the addendum says so. Its context
    section is scoped to environment variables, and no lint forbids reading the
    clock. D11 records that applying the taxonomy to a clock is a decision, not an
    inheritance.
  • time::OffsetDateTime is now on netsuke's public surface through
    ClockProvider, so a time 0.4 bump becomes a breaking library-API change.
    The plan states the consequence rather than inheriting it silently.

Verification

All commit gates pass: make check-fmt, make typecheck, make lint,
make test, make doc-coverage, make markdownlint, and make nixie.

The mutation exercise the plan required was run and all six designated
mutations were rejected by the test the plan nominated:

# Mutation Outcome
1 clock.read() → OffsetDateTime::now_utc() rejected: all four now_uses_injected_clock cases fail
2 Instant baked in at registration rejected: now_reads_the_provider_on_every_call fails
3 Offset applied as an arithmetic shift rejected: now_offset_preserves_the_instant fails at +00:00:01
4 Registration passes WallClock::default() rejected by the integration and BDD layers only — all 49 unit tests still pass
5 Refusing now stub deleted rejected: both refusal cases fail
6 UTC normalization removed from WallClock::read rejected: only the non-UTC case fails

Mutation 3 produced a finding worth carrying: taken literally as
timestamp + Duration::seconds(offset) it is a no-op, because
replace_offset preserves the wall-clock time rather than the instant, so the
offset shift is exactly cancelled by the added duration. The mutation was
re-run in a form that moves the instant while still setting the offset. The
to_offset / replace_offset near-miss is now documented, since the seam's
contract depends on the former.

Two pre-existing gaps the planning surfaced:

  • No test anywhere in the repository asserted that now() is refused in
    manifest-query mode, despite docs/users-guide.md promising it. There is now,
    in both halves: one case asserting the refusal, one asserting now is
    undefined after the permissive query registration alone.
  • RFC 0006 §3.3 recorded this exact gap and left it as open question 7, which
    no roadmap item cross-referenced. Both are now closed.

Property testing covers the offset invariant (the same instant, re-expressed);
Kani and Verus are explicitly ruled out with reasons, since the only arithmetic
involved belongs to the time crate and is treated as an axiom.

Documentation

  • ADR-008 gains the addendum "2026-09-11: Stdlib clock seam" and an
    Implementation references entry for src/stdlib/time/clock.rs.
  • docs/developers-guide.md's "Environment and template ports" documents the
    clock's ownership and module boundary, in the same commit as the ADR.
  • Technical design §5.2 moves from proposal to implemented state.
  • RFC 0007's gap list and RFC 0006 §3.3 / §16 question 7 record the seam as
    supplied.

Roadmap

Item 7.1.1 and its four sub-bullets are marked done in docs/roadmap.md, each
mapped to a named artefact in the exec plan's Outcomes & retrospective.

References

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Summary

  • Add an injectable ClockProvider seam for the stdlib time module.
  • Preserve UTC system-clock behaviour when no provider is configured.
  • Capture the configured clock during now() registration.
  • Preserve manifest-query refusal of now().
  • Support fixed clocks and offset re-expression without changing the instant.
  • Add unit, integration, filter, property-based, and BDD coverage.
  • Extract configure_stdlib to reduce BDD rendering complexity.
  • Document the design in ADR-008, the technical design, and related RFCs.
  • Complete roadmap item 7.1.1 and align the implementation with the clock-provider execplan.
  • Record the implementation’s deviation from the execplan’s stated file and line-change limits.

Walkthrough

The stdlib now supports injectable system and fixed clocks for now(). StdlibConfig owns the clock, registration captures it, and UTC normalisation remains enforced. Unit, integration, BDD, property, and documentation updates cover the new seam.

Changes

Stdlib clock seam

Layer / File(s) Summary
Clock provider contract
src/stdlib/time/clock.rs, src/stdlib/config/*, src/stdlib/mod.rs
Adds ClockProvider, WallClock, system and fixed clock constructors, UTC normalisation, provenance reporting, and StdlibConfig::with_clock.
Clock registration and evaluation
src/stdlib/time/mod.rs, src/stdlib/register.rs
Captures the configured clock during registration and uses it for each now() evaluation.
Clock integration and validation
src/stdlib/time/*tests*, tests/bdd/*, tests/features/stdlib_time.feature, tests/std_filter_tests/*, proptest-regressions/*
Adds deterministic clock fixtures and coverage for fallback behaviour, offsets, provider reads, manifest-query behaviour, and BDD rendering.
Clock seam documentation
docs/adr-008-environment-seam-taxonomy.md, docs/developers-guide.md, docs/netsuke-test-framework-technical-design.md, docs/rfcs/*, docs/roadmap.md, docs/users-guide.md, docs/v0-1-0-migration-guide.md
Documents the implemented seam, its manifest-query boundary, public API, testing shape, migration guidance, and completed roadmap status.

Sequence Diagram(s)

sequenceDiagram
  participant Template
  participant StdlibConfig
  participant register_with_config
  participant now
  participant ClockProvider
  Template->>StdlibConfig: configure clock
  StdlibConfig->>register_with_config: provide WallClock
  register_with_config->>now: capture clock
  now->>ClockProvider: read instant
  ClockProvider-->>now: return timestamp
  now-->>Template: render formatted time
Loading

Suggested labels: Roadmap

Priority: ⬇️ Low

Change: Refactor

Merge Risk: 🔵 Low · up to 8de3c

The guides currently describe the clock lifecycle incorrectly, which can mislead users configuring stateful test clocks. Correct the wording before merge; runtime behavior and timestamp parsing are otherwise supported.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Observability ⚠️ Warning The pull request changes runtime output: StdlibConfig::with_clock exposes a new provider, register_with_config passes it to now(), and each evaluation reads that provider. The changed clock and … Add a structured debug field at the clock-registration decision point, using the bounded values system and injected from WallClock::is_system(). Keep the event free of timestamps, provider output, and other unbounded values. Add a tra…
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the injectable clock-provider seam and references roadmap item 7.1.1.
Description check ✅ Passed The description directly explains the clock-provider implementation, scope, tests, documentation, roadmap completion, and known scope deviation.
Docstring Coverage ✅ Passed Docstring coverage is 82.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 16 files. (5 skipped: 5…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Overall) ✅ Passed Accept the testing coverage. The diff adds substantive unit tests, real StdlibConfig integration tests, and BDD scenarios for injected clocks, ambient fallback, UTC normalisation, repeated reads, pe…
User-Facing Documentation ✅ Passed Pass this check. The pull request adds a clear Inject the clock for deterministic tests section to docs/users-guide.md. It documents StdlibConfig::with_clock, ClockProvider, fixed_clock, `sy…
Developer Documentation ✅ Passed Pass this check. The developer's guide documents the ClockProvider shape, StdlibConfig ownership, production and fixed adapters, registration boundary, and manifest-query refusal. The technical de…
Module-Level Documentation ✅ Passed PASS. All modules introduced or modified by the pull request have module-level //! documentation. The new clock modules explain their purpose, utility, provider relationship, registration behaviour,…
Testing (Unit And Behavioural) ✅ Passed Pass the Testing (Unit and Behavioural) check. The unit suite covers the clock seam's local behaviour, including system-clock fallback, fixed and sequenced providers, per-call reads, call counts, UTC …
Testing (Property / Proof) ✅ Passed Mark this check PASS. The change introduces the invariant that now(offset=...) preserves the instant across the valid offset range. src/stdlib/time/clock_tests.rs adds a proptest property over b…
Testing (Compile-Time / Ui) ✅ Passed Pass. The change adds runtime clock injection, not compiler diagnostics or compile-time rejection behaviour. Its new public Rust API has executable doctests, and the repository runs doctests through t…
Unit Architecture ✅ Passed The change preserves the stated unit boundaries. OffsetDateTime::now_utc() is isolated in the system_clock() adapter; StdlibConfig::with_clock injects a narrow ClockProvider at the configurati…
Domain Architecture ✅ Passed Accept the change. The diff places the host-clock adapter in src/stdlib/time/clock.rs (system_clock()), accepts a domain-shaped ClockProvider, and injects it through StdlibConfig::with_clock. …
Full details: Observability

Explanation

The pull request changes runtime output: StdlibConfig::with_clock exposes a new provider, register_with_config passes it to now(), and each evaluation reads that provider. The changed clock and now() paths emit no logs or metrics. The only registration log records file_max_read_bytes; it does not record whether the registered clock is system or injected. WallClock has a bounded Debug label, but the code does not emit that value, and no runtime call site formats StdlibConfig. This leaves a wrong clock wiring or unexpected production clock source difficult to diagnose. Metrics and tracing are not required here because the change does not affect the listed throughput, latency, queue, resource, or cross-process boundary concerns.

Resolution

Add a structured debug field at the clock-registration decision point, using the bounded values system and injected from WallClock::is_system(). Keep the event free of timestamps, provider output, and other unbounded values. Add a tracing test that verifies both source values are emitted during registration. Preserve the existing caller-visible errors for invalid offsets; do not add alerts or per-call metrics for this infallible, in-process provider.


A fixed clock ticks on cue
UTC keeps each instant true
now() reads the supplied light
Tests hold time in place just right
Docs record the seam in view

Comment @coderabbitai help to get the list of available commands.

codescene-access[bot]

This comment was marked as outdated.

@sourcery-ai

sourcery-ai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

This documentation-only PR adds a draft ExecPlan for making the stdlib now() helper deterministic through an injected, thread-safe clock provider stored in StdlibConfig, with an ambient system-clock fallback, protected manifest-query behavior, explicit design decisions, implementation milestones, and comprehensive verification requirements.

Sequence diagram for configured now() rendering

sequenceDiagram
    participant Caller
    participant Config as StdlibConfig
    participant Registration
    participant Time as time module
    participant Provider as ClockProvider

    Caller->>Config: with_clock(provider)
    Caller->>Registration: register_with_config(config)
    Registration->>Config: clock()
    Registration->>Time: register_functions(WallClock)
    Caller->>Time: Render template containing now()
    Time->>Provider: read provider()
    Provider-->>Time: OffsetDateTime instant
    Time->>Time: to_offset(parsed)
    Time-->>Caller: Rendered timestamp
Loading

File-Level Changes

Change Details Files
Adds a detailed implementation plan for introducing an injectable wall-clock seam while preserving ambient behavior by default.
  • Defines the ClockProvider port, WallClock wrapper, and system/fixed clock adapters.
  • Threads the clock through StdlibConfig and stdlib time registration while keeping query-mode registration clock-free.
  • Preserves UTC normalization, offset handling, Debug/Clone configuration derives, and existing public behavior.
  • Specifies unit, integration, behavioral, property, regression, and mutation testing obligations.
  • Documents the required ADR, technical-design, developer-guide, RFC, and roadmap updates for implementation completion.
docs/execplans/7-1-1-clock-provider-seam.md

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

@leynos
leynos force-pushed the 7-1-1-clock-provider-seam branch from d91ee5f to 1e6c93e Compare September 8, 2026 15:20
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@leynos leynos changed the title Plan: Add the clock provider seam to the stdlib time module (7.1.1) Add the clock provider seam to the stdlib time module (7.1.1) Sep 10, 2026
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

leynos added a commit that referenced this pull request Sep 10, 2026
PR #696 still described itself as plan-only. The description now covers
the delivered seam, and the plan records that swap plus the fact that the
draft flag was left alone on purpose: whether to mark the PR ready before
or after CodeRabbit's PR-level review is the maintainer's call.

Co-Authored-By: Claude Code <noreply@anthropic.com>
codescene-access[bot]

This comment was marked as outdated.

@leynos

leynos commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

Complex Method

tests/bdd/steps/stdlib/rendering.rs: render_template_with_context

What lead to degradation?

render_template_with_context has a cyclomatic complexity of 9, threshold = 9

Why does this problem occur?

A Complex Method has a high cyclomatic complexity. The recommended threshold for the Rust language is a cyclomatic complexity lower than 9.

How to fix it?

There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring.

Helpful refactoring examples

To get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes.

SAMPLE

# complex_method.js
 function postItem(item) {
   if (!item.id) {
-    if (item.x != null && item.y != null) {
-      post(item);
-    } else {
-      throw Error("Item must have x and y");
-    }
+    // extract a separate function for creating new item
+    postNew(item);
   } else {
-    if (item.x < 10 && item.y > 25) {
-      put(item);
-    } else {
-      throw Error("Item must have an x and y value between 10 and 25");
-    }
+    // and one for updating existing items
+    updateItem(item);
   }
 }
+
+function postNew(item) {
+  validateNew(item);
+  post(item);
+}
+
+function updateItem(item) {
+  validateUpdate(item);
+  put(item);
+}
+

@coderabbitai

This comment was marked as resolved.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

leynos added a commit that referenced this pull request Sep 10, 2026
PR #696 still described itself as plan-only. The description now covers
the delivered seam, and the plan records that swap plus the fact that the
draft flag was left alone on purpose: whether to mark the PR ready before
or after CodeRabbit's PR-level review is the maintainer's call.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@leynos
leynos force-pushed the 7-1-1-clock-provider-seam branch from d93e55e to 1b87a1e Compare September 10, 2026 23:43
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@leynos
leynos marked this pull request as ready for review September 11, 2026 00:10
leynos and others added 18 commits September 24, 2026 22:47
Recording a total changes it: every plan-only commit that maintains this
record adds its own length to the diff, so a head-specific total is stale the
moment it is written. Replace the quoted total with the two quantities that
do not move — changed-file count and the non-plan remainder — and make the
attribution section attribute that remainder rather than a total.
Every substantive check passes and the pull request is approved; the only red
check is the non-required CodeScene review of the base branch. Name the four
checks the ruleset actually requires, so the distinction is on the record
rather than left to be re-derived.
Three edits, each verified against current source before repair.

`tests/documentation_examples_tests.rs`: the CodeScene duplication
thread on `clock_snippet_mirrors_the_doctest` was valid — that test,
`env_reader_snippet_mirrors_the_doctest` and
`ninja_request_snippet_names_both_request_types` were three copies of
one shape.  Extract `assert_snippet_names`, which carries the shared
"Rust fence, then each needle" contract and takes the example id, the
label used in failure messages, and the needles.  Behaviour is
unchanged: all 32 tests in the target still pass.

`docs/adr-008-environment-seam-taxonomy.md`: the `with_clock`
injection-point link still named `src/stdlib/config/mod.rs`.  That is
stale because the clock seam was split into its own
`src/stdlib/config/clock.rs` earlier in this branch, so the ADR
pointed at a file that no longer holds the function.

`docs/execplans/7-1-1-clock-provider-seam.md`: three corrections to the
living document — remove the duplicated `use std::{fmt, sync::Arc};`
and `time::OffsetDateTime` import lines from the implementation sketch;
record in D4 that the first review upheld a "User-Facing Documentation"
warning against the decision's rationale, since `with_clock` is a
public Rust API and not only a manifest-author concern; and correct the
recovery instructions, which described `git reset --hard` and
`git checkout --` as though they were scoped to the mutation when both
discard uncommitted work more broadly.

Gates: `make check-fmt` (including mdtablefix under the Makefile's own
flag set), `make markdownlint`, `make lint`, and the
`documentation_examples_tests` target.
`Windows / build-test-windows` fails at this head, which looks alarming
next to a green branch history.  It is an estate-wide breakage: the same
job fails on `origin/main` (`ef7ed760`) and on every unrelated branch
tested, and it last passed anywhere at 09:38Z on `36e03c7f`.

The failing case is
`stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response`,
which lives on `origin/main` in `src/stdlib/network/redirect_error_tests.rs`
(via #667).  This branch's diff against its merge base `a273fad3` adds
zero bytes under `src/stdlib/network/`.  The error is a Windows socket
race (`WSAECONNABORTED`, `os error 10053`) against the test's own
loopback listener.

Also recorded: the job share is not a required check.  The ruleset
`main-required-checks` requires only `build-test`, `kani-smoke`,
`netsukefile` and `release / metadata`; `build-test` is a different job
and passes at this head, so the required set is green.

Gates: `make check-fmt` and `make markdownlint`.
The review asked for in this round is a posted *request*, not a completed
review, and the plan should not read as though the two are the same.  The
entry names the queued head, the queue id, the quoted delay, and the fact
that a comment body does not pin a revision — so whichever commit
CodeRabbit inspects has to be read back afterwards.

Gates: `make check-fmt` and `make markdownlint`.
The first rebase landed at 07248a3; origin/main has since advanced to
79545e1 (the 19-update GitHub-actions group bump). Replay the 40
branch-owned commits above the new merge base with the same explicit
options used before.

The re-target is byte-for-byte identity-preserving: every commit is `=`
under range-diff, there are no merges and no conflicts, and the net diff
is unchanged at 27 files / 3603 insertions / 161 deletions. Cargo.toml
and Cargo.lock are byte-identical to origin/main, so no regeneration was
needed.

The new commit is a workflows-and-contract-test delta with zero file
overlap with this branch, and `make test` runs only Rust targets, so it
lies outside this branch's gate surface. It does not move the Windows
job's line anchors, so the recorded Windows diagnosis still holds.

Weave again did not participate: the driver is registered globally but
merge attributes are `unspecified` for every branch-owned path.
CodeRabbit reviewed 8de3c96 and raised one inline finding plus an
Observability pre-merge warning. Both are valid against the current
source; neither was present when the branch was last reviewed.

The inline finding is a real wording defect in both guides. They said the
provider is read "rather than captured at registration", but
`register_functions` moves a `WallClock` into the registered closure, so
the clock *is* captured while the *instant* is not. The crate's own
docs, ADR-008 and the technical design all state this correctly, which
leaves the two guides as the outliers. Both passages now say that
registration captures the adapter and each call invokes it afresh.

The Observability warning asks for a bounded debug field at the
clock-registration decision point. PR #669 added exactly such an event
for the file filters one line below, so the gap is genuine and the shape
is settled. `WallClock::source_label` now names the provenance from a
closed set, and `register_with_config` records `clock_source` alongside
"registered stdlib time helpers". `Debug` reuses the same accessor, so
the label has one definition.

`registration_reports_the_clock_source` covers both provenances and
asserts the event never carries a provider's instant. It lives in the
integration suite because `register_with_config` is public and the event
is emitted there, not in `time::register_functions`. Removing the label
mutation turns the injected case red, so the assertion has teeth.
`make check-fmt` rejected two spots: the `source_label` if-else on one
line, and an over-long `assert!` in the new integration test. Both are
formatting only; no behaviour changed.
The queued review is closed as a *completed* review rather than a pending
request: its read-back shows CodeRabbit inspected 8de3c96, not the
pre-rebase head the queue comment named, and returned CHANGES_REQUESTED with
one inline finding and an Observability pre-merge warning.

Both findings are disposed of with evidence. The inline wording finding is
valid — register_functions moves a WallClock into the registered closure, so
the clock is captured while the instant is not — and both guides now say so.
The Observability warning is valid and answered with source_label plus the
clock_source debug field, covered by a mutation-tested integration case.

Adds two evidence entries: check-fmt is two gates behind one name, and the
re-target boundary is the current merge base rather than an earlier one.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The registration event case returned Result while asserting with assert!,
which clippy::panic_in_result_fn rejects under -D warnings; make lint
aborted at lint-clippy on the std_filter_tests target.

The assertion is replaced by a contextual `?`, so a registration failure
propagates as the error the signature already promises. The test still
fails on the same condition and the closed-set assertion is untouched:
mutating source_label to report "system" for both provenances turns
case_2_injected red, and the source is restored byte-identically.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The -8 gate run failed on one test outside the change surface that timed out
at the 300 s per-test allowance. Recorded as artefact entry 21 with the
measurement that settles it: raising the ceiling shows the test completing in
292.1 s, of which 291.6 s is its nested cold cargo build, matching the 688.6 s
figure the developers' guide already records for that build under contention.

Also records that my first explanation -- heavy load -- was refuted by two
isolated re-runs that timed out at load 9.3 and 7.0, while its conclusion was
right. The mechanism is a cold build behind a shared package-cache lock, which
bites at moderate load; the entry keeps the measurement and drops the story.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The `-9` gate run on `25787722` reproduced the single-test signature of
`-8`: `harness_compiles_under_a_split_build_dir` timed out at its 300 s
allowance with 3245 of 3250 passing, and no other test failed.

Issue #732 already describes this mechanism, names this test, and states
that the harnesses' repeated compilation "is what puts these tests near the
300 s per-test allowance". Recording it turns "not this branch's defect"
from an assertion into a citation, and keeps the plan from re-deriving the
diagnosis a third time.

Folded into entry 21's body rather than added as a sibling list item: a new
marker at that position restarts markdown's ordered list, and mdtablefix's
`--renumber` rewrites it to `1.`, which is not canonical.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The re-target push advanced the branch to `f81f2f98` on base `79545e12`,
under a lease bound to the previously recorded remote head `8de3c963` so a
concurrent rewrite would fail the push rather than be overwritten.

Records the gate state at that head and, specifically, why the `lint` re-run
mattered: the `panic_in_result_fn` error was only confirmed fixed by running
the gate at a head containing the fix, since the earlier `-8` log predates
it. Also records that both review replies were posted against this head, and
that the CI run the push triggered is not yet claimed as green.

Co-Authored-By: Claude Code <noreply@anthropic.com>
All four required checks pass on `74822cc2`: `build-test` and `kani-smoke`
(CI run 35454416505), `netsukefile` (35454416354), and `release / metadata`
(35454416662). The sole red job is the non-required
`Windows / build-test-windows`, failing for the already-recorded
pre-existing reason, re-verified here against `main` at `ef7ed760` rather
than assumed.

Also corrects a wrong premise I supplied while briefing the monitor: I
described that job as failing in `git submodule` before project code runs.
It does not — those lines are post-job cleanup from a successful checkout,
and the real failure is the loopback race at the test step. The conclusion
held, which is why the premise had to be checked rather than inherited.

Co-Authored-By: Claude Code <noreply@anthropic.com>
CodeRabbit's separate answer to the pre-merge reconciliation confirms the
implementation by static inspection and instructs that the observability
warning be marked resolved, with no follow-up issue and no further code work.

Records two facts separately rather than merging them: both review threads
are resolved and the queue is empty, while the `CHANGES_REQUESTED` decision
persists and is anchored to `8de3c963`, which is no longer an ancestor of
the branch. A stale anchor is not an approval, and clearing it would mean
dismissing a review or approving on the bot's behalf — so the decision is
left with its designated owner, and the four required checks are recorded as
`SUCCESS` on the published head.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Recording "CI is green at head H" is itself a commit, and that commit moves
the head, invalidating the verdict it records. The plan is not a passive
record here: `build-test` runs `make check-fmt`, markdownlint over `**/*.md`,
`make spelling`, and the workspace test suite, and
`tests/execplan_status_contract_tests.rs` reads `docs/execplans/` — so an edit
to this document is an input to the same required checks whose result it
reports.

Three pushes were spent rediscovering that. The fix is not to keep
re-verifying but to name the head that was verified rather than implying the
newest one is, so this is the last plan commit for the re-target.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The reconciliation entry recorded that the walkthrough's Observability row
persists because the comment "has not been re-edited". That is wrong: the
comment's updated_at moved to 2026-09-19T17:25:12Z, after CodeRabbit's
confirmation at 16:13:51Z, so it was touched.

What did not happen is any change to what it says. Three captures of the body
taken after that edit (17:55, 18:51, 20:06) are byte-identical to the live
body, so the touch was content-preserving and left the row standing.

The distinction is load-bearing. "The table is stale" would justify asking for
another pass; "the table was refreshed and the finding still stands" would not.
Only the first reading fits the evidence, and the second is the one the
recorded reason implied.

Dispositions are unchanged: both threads isResolved, both fixes verified
present at current source, all four required checks SUCCESS on 74822cc. The
row remains CodeRabbit's to flip; ticking its Ignore checkbox or dismissing
the review to force the table green is not done here.

Gates: make check-fmt, make markdownlint, make spelling all exit 0, and
execplan_status_contract_tests 9/9 passed -- the one test reading docs/execplans/.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Two entries, both about replacing a record that no longer describes reality.

The Progress section gains the re-rebase onto the current origin/main:
boundary 79545e1, target 397fb58, 52 commits replayed, exactly one
conflict, in docs/v0-1-0-migration-guide.md where main's #737 rewrote the
at-a-glance table while this branch appended a row to it. Both intents were
additive, so the resolution keeps main's table and appends the row;
range-diff reports 51 of 52 pairs identical and the one divergence is
padding. The substantive fixes are shown to survive by whole-file patch-id
comparison rather than by reading subjects.

The timeout disposition in entry 21 is marked superseded rather than left to
read as current. It cited issue 732 as a live tracker and concluded there
was nothing to fix here. The rebase adopted #752, which closed 732 and
replaced that test's live private rebuild with a recorded Cargo JSON
fixture, so the test no longer spawns Cargo and was dropped from the
nested-cargo-builds group. The first rebased gate run confirms it: make test
is 3394 of 3394 passing, 0 failed, 5 skipped in 276 s, with the
formerly-timing-out test green and no longer slow.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@leynos
leynos force-pushed the 7-1-1-clock-provider-seam branch from 1ef9859 to cabb7c6 Compare September 24, 2026 21:13
codescene-access[bot]

This comment was marked as outdated.

@buzzybee-df12

Copy link
Copy Markdown
Collaborator

@coderabbitai Please reconcile the Observability row against the current head.

The branch has been rebased onto the current origin/main, so the commit
identifiers in this thread are superseded. The disposition is unchanged:
resolved, and your own static analysis confirmed it — the rewrite does not
alter what was confirmed.

  • Replay boundary 79545e12 → target 397fb589; 52 commits replayed;
    git range-diff reports 51 of 52 pairs =.
  • Previous head 1ef98593; new head cabb7c63.

I read your 2026-09-24T20:42:22Z reply, which confirmed this warning at
1ef98593. That head has been superseded by the rebase, so I am re-anchoring
the confirmation rather than asking you to repeat it. The evidence is
byte-identical across the replay rather than merely re-asserted — I compared
whole-file patch-ids before and after:

  • src/stdlib/time/clock.rs — IDENTICAL. source_label() remains a closed
    two-value result returning "system" or "injected", and Debug for WallClock still reuses it, so the debug label and the telemetry label cannot
    diverge.
  • src/stdlib/register.rs — IDENTICAL. register_with_config still emits
    clock_source = clock.source_label() immediately before
    time::register_functions, with the message "registered stdlib time helpers", and the event carries neither the clock instant nor provider
    output.
  • tests/std_filter_tests/time_functions.rs — IDENTICAL.
    registration_reports_the_clock_source still covers both provenance values,
    asserts the expected clock_source field, and rejects leakage of the fixed
    provider instant.

You closed that reply with "Treat the observability warning as resolved" and "No
AI agent prompt or follow-up issue is needed for this warning." Nothing in
the replay changes that, so no further code work has been done for it and no
follow-up issue has been created.

The two checks you explicitly did not treat as resolved

You were careful to add that your assessment was not a resolution of the failed
Windows build-test or the CodeScene coverage timeout. Both are disposed of here.

Windows / build-test-windows (failed at 1ef98593). One test failed:
stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response,
reporting io: An established connection was aborted by the software in your host machine. (os error 10053). That is the Windows close-abort fixture defect:
the fixture's naked listener close reset the connection, so the client reported
an aborted connection in place of the parse failure it was meant to reach.

This was a superseded head, not a live defect. The fix, #749
(061182b1, "Complete local HTTP fixture responses with a write-side
shutdown"), replaces the race with a write-side shutdown(Write). I checked its
ancestry directly: 061182b1 was not an ancestor of 1ef98593, so the
failing head did not contain the fix; it is an ancestor of the new head
cabb7c63, because the rebase onto 397fb589 now inherits it. The test file
was not touched by any commit of mine in 397fb589..HEAD. It is also not in the
repository's required set.

CodeScene Code Coverage (main) (timed out at 1ef98593). This check is
known to time out on pull requests by design: it writes trunk-only coverage
metrics. It is not in the required set either. The required set on main is
exactly build-test, kani-smoke, netsukefile and release / metadata
(ruleset main-required-checks, id 18427981). CodeScene Code Health Review (main), the check that does grade this branch, reports success.

Scope of this reconciliation, stated plainly

The walkthrough's pre-merge row still renders as a warning, because the row is
yours to redraw and I have not ticked its Ignore checkbox or suppressed
anything to force it green. Separately, the historical CHANGES_REQUESTED
review object remains pinned to 8de3c963; I verified with
git merge-base --is-ancestor 8de3c963 cabb7c63 that the rebase has made it
non-ancestral, so it now describes a revision on a superseded history line
rather than merely an old one.

Gates on the rebased head: make check-fmt, make test (3394/3394 passed,
0 failed, 5 skipped in 276 s), make typecheck, make lint — all exit 0. The
only commit made after those gates is documentation-only
(docs/execplans/7-1-1-clock-provider-seam.md, 47 insertions, no Rust,
manifest or lockfile), so the Markdown-sensitive gates and the execplan status
contract test were re-run at cabb7c63.

One operational note so the record is accurate: the CodeRabbit commit status
on cabb7c63 reads success after about five seconds with the description
"Review paused", not "Review completed". I am treating that as the
auto-pause stamp and not as a review of the new head.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo '--- revision ---'
git rev-parse HEAD
echo '--- file discovery ---'
fd -a '^(clock|register|time_functions)\.rs$' src tests
echo '--- outlines ---'
ast-grep outline src/stdlib/time/clock.rs --items all --match 'WallClock|source_label' --view expanded
ast-grep outline src/stdlib/register.rs --items all --match 'register_with_config' --view expanded
ast-grep outline tests/std_filter_tests/time_functions.rs --items all --match 'registration_reports_the_clock_source' --view expanded
echo '--- accessor and debug ---'
rg -n -A20 -B9 'source_label|impl\s+Debug\s+for\s+WallClock' src/stdlib/time/clock.rs
echo '--- registration ---'
rg -n -A22 -B15 'registered stdlib time helpers|time::register_functions' src/stdlib/register.rs
echo '--- trace test ---'
rg -n -A105 -B18 'registration_reports_the_clock_source' tests/std_filter_tests/time_functions.rs
echo '--- relevant ancestry ---'
for sha in 061182b1 8de3c963; do
 if git merge-base --is-ancestor "$sha" HEAD; then echo "$sha ancestor"; else echo "$sha not ancestor or unavailable"; fi
done
echo '--- PR head and checks ---'
gh pr view 696 --repo leynos/netsuke --json headRefOid,statusCheckRollup --jq '{headRefOid,checks:[.statusCheckRollup[]? | {name:(.name // .context),status:(.status // .state),conclusion:.conclusion,description:.description}]}'

Length of output: 9054


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo '--- relevant test locations ---'
rg -n -A35 -B15 'fn\s+protocol_failures_are_classified_from_a_live_response\b|shutdown\(Shutdown::Write\)|shutdown\(Write\)' --type rust src tests | head -180
echo '--- PR 749 ---'
gh pr view 749 --repo leynos/netsuke --json number,title,state,mergedAt,mergeCommit,url --jq '{number,title,state,mergedAt,mergeCommit,url}'
echo '--- current commit check ---'
gh pr view 696 --repo leynos/netsuke --json headRefOid,statusCheckRollup --jq '{headRefOid,checks:[.statusCheckRollup[]? | select((.name // .context) == "Windows / build-test-windows" or (.name // .context) == "CodeScene Code Coverage (main)" or (.name // .context) == "CodeRabbit") | {name:(.name // .context),status:(.status // .state),conclusion:.conclusion}]}'

Length of output: 6142


@buzzybee-df12 The additional check confirms that #749 merged and that the current Windows test uses the raw-response fixture described as completing its response with a write-side shutdown. This supports your explanation of the historical failure.

Keep the Observability row resolved at cabb7c63. Keep the Windows result separate: Windows / build-test-windows was still queued at that head when checked. Do not mark that check as passed until it reports a result.


🧠 Learnings used
Learnt from: leynos
URL: https://github.com/leynos/netsuke/pull/696

Timestamp: 2026-09-24T20:42:21.347Z
Learning: For leynos/netsuke PR reviews, treat observability, code safety, documentation, and validation coverage as in scope. Treat warnings as required work, not optional improvements. If required work is outside a PR's scope, propose a GitHub issue unless one already exists.

You are interacting with an AI system.

The rebased head was published with a force-with-lease bound to the
previously recorded remote head, and the PR base now reads the same SHA
as the replay target.

Records how CodeRabbit's two explicitly-unresolved checks were disposed
of. The Windows close-abort failure is superseded by this rebase, proved
by ancestry: the fix was absent from the head that failed and is present
afterwards. The CodeScene coverage timeout is trunk-only by design and
not in the required set.

Also records that the new head's green CodeRabbit status carries the
description "Review paused" rather than "Review completed", so it is a
pause stamp and not evidence of a review.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Comments resolved and changes approved.

@wafflecat-df12
wafflecat-df12 merged commit 96aefc9 into main Sep 24, 2026
22 checks passed
@wafflecat-df12
wafflecat-df12 deleted the 7-1-1-clock-provider-seam branch September 24, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Roadmap A pull request originating from a roadmap item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants