Hypothesis stateful tests for the metrics event reducer (#78)

[leynos]

Summary

MetricsHook.__call__ was a phase-dispatch that reached straight into the collector, with no seam to verify which counters and histograms each event yields across varied phase/order/pid combinations (#78).

Seam

Extract the pure event-to-operation reducer _metric_operations(event) -> tuple[_MetricOp, ...]:

• _CounterOp / _HistogramOp records describe the intended operations; a _PHASE_COUNTERS lookup collapses the unit-counter phases.
• MetricsHook.__call__ now applies the reducer's operations, resolving labels only when there is at least one operation — so plan (and an unknown phase) still compute no labels, as the existing tests require.
• The former _increment/_record_stdin_bytes/_record_exit helpers are removed; behaviour is unchanged and test_metrics_adapter.py still passes.

Tests

cuprum/unittests/test_metrics_adapter_stateful.py:

• Property tests pin the operations produced per phase — unit counters for start/stdout/stderr/stdin_error; plan yields nothing; stdin yields a bytes counter only when a byte count is present; exit counts a failure only for a non-zero code and a duration only when measured; an unknown phase raises.
• A Hypothesis RuleBasedStateMachine streams random events (all seven phases, phase-appropriate fields) through a real MetricsHook/InMemoryMetrics and, after every step, checks the accumulated counters and histograms against an independent phase-count oracle (not the reducer). This proves counters and observations are created exactly when intended, and only then, across arbitrary event orders.

Scope

#78 is titled "Hypothesis stateful tests for metrics/tracing/logging hooks".
Its body scopes the work to cuprum/adapters/metrics_adapter.py, but taking the
title at its word, all three adapters now have randomised event coverage:

Table 1: verification shape for each observe hook, and why

Adapter	Coverage	Shape, and why
tracing_adapter.py	test_tracing_span_stateful.py (pre-existing)	state machine — holds _active_spans, so correlation and drain are the risks
metrics_adapter.py	test_metrics_adapter_stateful.py (added here)	state machine — accumulates counters and histograms
logging_adapter.py	test_logging_adapter_properties.py (added here)	@given properties — holds no state at all

The logging hook gets properties rather than a fourth state machine because it
carries nothing between events: one record in, one record out. Interleavings
cannot distinguish any two implementations of it. Its risks are per-event and
shape-dependent — a reserved-LogRecord collision, a phase falling through the
level map, a tag value the JSON formatter cannot serialize — and the five
properties pin exactly those.

On active map draining: only TracingHook has an active map.
metrics_adapter.py has none, and neither does the logging hook, so the claim
applies to tracing alone — where test_tracing_span_stateful.py asserts it
directly, cross-checking hook._active_spans against a model after every step
and pinning that an exit removes only its own execution's span.

#252 was raised to track the logging work while it was still outstanding; it is
now delivered here and can be closed.

Closes #78

🤖 Generated with Claude Code

Summary by Sourcery

Extract a pure event-to-metrics reducer for the metrics hook and add property-based and stateful tests to verify metrics behavior across execution phases.

New Features:

• Introduce a pure _metric_operations reducer that maps execution events to counter and histogram operations for the metrics hook.
• Add Hypothesis-based property and stateful tests that validate metrics accumulation over randomized execution event streams.

Enhancements:

• Refactor MetricsHook.__call__ to delegate to the shared reducer and a generic operation applier, avoiding label computation for no-op events.

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

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

• Extract a pure _metric_operations reducer from MetricsHook, with explicit counter and histogram operations.
• Preserve no-op behaviour for plan and uncounted stdin events. Retain errors for unknown phases.
• Add Hypothesis property-based and stateful tests for metrics and structured logging.
• Document the reducer, label-resolution flow, non-atomic operation application, and verification strategy in the design and developer guides.
• Update the wheel-manifest snapshot for the new metrics test module.

Walkthrough

Reduce execution events into explicit counter or histogram operations. Apply those operations through MetricsHook. Validate metrics and logging behaviour with property-based and stateful tests. Document the reducer flow and update the wheel snapshot.

Changes

Metrics operation pipeline

Layer / File(s)	Summary
Reduce events into metric operations cuprum/adapters/metrics_adapter.py	Create immutable operations and reducers for phase-specific counters, optional stdin byte counts, exit failures, and durations. Raise _UnhandledMetricsPhaseError for unknown phases.
Apply operations through the metrics hook cuprum/adapters/metrics_adapter.py	Reduce each event before extracting labels. Skip label extraction when no operations exist. Apply counter and histogram operations independently through MetricsCollector.
Verify and document adapter behaviour cuprum/unittests/test_metrics_adapter_stateful.py, cuprum/unittests/test_logging_adapter_properties.py, docs/cuprum-design.md, docs/developers-guide.md, cuprum/unittests/__snapshots__/test_maturin_build.ambr	Add unit, property-based, stateful, and failure-path tests. Document the two-stage metrics flow and verification shapes. Record the new stateful test in the wheel snapshot.

Sequence Diagram(s)

sequenceDiagram
  participant ExecEvent
  participant MetricsHook
  participant metric_operations
  participant MetricsCollector
  ExecEvent->>MetricsHook: submit execution event
  MetricsHook->>metric_operations: reduce event
  metric_operations-->>MetricsHook: return operations
  MetricsHook->>MetricsCollector: apply counter or histogram operation

Loading

Suggested labels: Issue

Poem

Reduce each event to an operation,
Apply counters with clear separation.
Record bytes, failures, and time,
Test each phase in ordered rhyme.
Keep unknown phases in line.

---

Important

Pre-merge checks failed

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

❌ Failed checks (2 inconclusive)

Check name	Status	Explanation	Resolution
Testing (Overall)	❓ Inconclusive	Evidence gathering is still in progress.	Inspect the complete adapter behaviour, existing tests, and generated-test execution before deciding.
Developer Documentation	❓ Inconclusive	I need to inspect the repository files and history before I can assess documentation coverage.	Provide the PR checkout or an accessible diff if the repository does not contain the proposed changes.

✅ Passed checks (18 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the metrics reducer tests and includes the linked issue reference (#78).
Description check	✅ Passed	The description clearly explains the metrics reducer, randomized tests, logging coverage, and scope for issue #78.
Linked Issues check	✅ Passed	The changes address issue #78 by adding metrics stateful tests, logging properties, reducer seams, and retaining tracing coverage.
Out of Scope Changes check	✅ Passed	The implementation, tests, and documentation support the linked issue objectives without unrelated code changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
User-Facing Documentation	✅ Passed	Pass: the PR refactors existing metrics behaviour without changing the public API or emitted metrics; the users guide already documents the metrics adapter.
Module-Level Documentation	✅ Passed	All 186 Python modules have module docstrings; the metrics adapter and both new property/stateful test modules clearly state their purpose, function, and component relationships.
Testing (Unit And Behavioural)	✅ Passed	Accept the coverage: reducer edge/error tests, a real-hook state machine with an independent oracle, and command plus logging boundary tests use real collectors and handlers.
Testing (Property / Proof)	✅ Passed	Hypothesis tests vary stdin counts and exit values; a RuleBasedStateMachine checks 60×20 event streams against an independent oracle, with five logging properties covering varied event shapes.
Testing (Compile-Time / Ui)	✅ Passed	Pass this check: no Rust or TypeScript files changed; metrics and JSON logging use focused property/state assertions, and the wheel snapshot records the added test entries.
Unit Architecture	✅ Passed	The reducer is side-effect free, while MetricsHook._apply performs explicit collector writes through an injected MetricsCollector; no hidden I/O, clock, network, or global mutable dependency was ad...
Domain Architecture	✅ Passed	Keep the boundary: _metric_operations and MetricsHook remain in cuprum.adapters, use the injectable MetricsCollector protocol, and core modules import no adapter or vendor code.
Observability	✅ Passed	The refactor preserves metric behaviour; _emit_exec_event logs phase, program, and error type, while docs define partial failures and metrics use only program/project labels.
Security And Privacy	✅ Passed	The PR adds metric operation records, tests, and documentation; it introduces no secrets, auth changes, injection sinks, permissions, or new sensitive-data telemetry.
Performance And Resource Use	✅ Passed	The production change uses O(1) phase lookup and applies at most two operations per event; new allocations are bounded per event, with no new I/O, retries, queues, or unbounded production state.
Concurrency And State	✅ Passed	MetricsHook keeps only local immutable operations; collector thread safety and _LockedStore locking are explicit, while docs and tests cover ordered application and partial failure.
Architectural Complexity And Maintainability	✅ Passed	The change adds no dependencies, registries, global mutable state, or cross-layer edges; the abstraction remains local to metrics_adapter.py.
Rust Compiler Lint Integrity	✅ Passed	Keep compiler lint integrity: the merge-base diff contains no Rust files, adds no Rust suppressions or clone calls, and current Rust has only one narrow, issue-linked #[expect].

📋 Issue Planner

Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).

View plan for ticket: #78

✨ 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 python-metrics-adapter-tests

---

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

[sourcery-ai]

Reviewer's Guide

Refactors the metrics adapter to introduce a pure event-to-metric-operations reducer and wires MetricsHook.call through it, then adds focused property tests and a Hypothesis rule-based state machine to verify that metrics counters and histograms are produced exactly as intended across all execution phases and event streams.

Sequence diagram for MetricsHook event-to-operations reducer

sequenceDiagram
    participant ExecEvent
    participant MetricsHook
    participant Reducer as _metric_operations
    participant Collector as MetricsCollector

    ExecEvent ->> MetricsHook: __call__(event)
    MetricsHook ->> Reducer: _metric_operations(event)
    Reducer -->> MetricsHook: tuple[_MetricOp]

    alt no operations
        MetricsHook -->> ExecEvent: return
    else has operations
        MetricsHook ->> MetricsHook: _extract_labels(event)
        loop for each operation
            MetricsHook ->> MetricsHook: _apply(operation, labels)
            alt _CounterOp
                MetricsHook ->> Collector: inc_counter(name, value, labels)
            else _HistogramOp
                MetricsHook ->> Collector: observe_histogram(name, value, labels)
            end
        end
    end

Loading

File-Level Changes

Change	Details	Files
Introduce pure event-to-operation reducer and small operation types to decouple event logic from the metrics collector.	Define immutable _CounterOp and _HistogramOp dataclasses plus the _MetricOp union for describing intended metric operations. Add the _PHASE_COUNTERS mapping for simple unit-counter phases and implement _exit_operations for exit-phase-specific logic. Implement _metric_operations(event) as the single event-to-operations reducer, handling plan/stdin/exit phases and enforcing unknown phases via _UnhandledMetricsPhaseError.	cuprum/adapters/metrics_adapter.py
Rewire MetricsHook to use the reducer and a generic apply path, simplifying phase dispatch and label handling.	Replace the match/case phase dispatch in MetricsHook.call with a call to _metric_operations and early-return when there are no operations so labels are not computed for no-op events. Introduce MetricsHook._apply to translate _CounterOp/_HistogramOp instances into collector calls, replacing the previous _increment/_record_stdin_bytes/_record_exit helpers. Remove the old helper methods and ensure behavior remains equivalent by reusing the same counter and histogram names and values.	cuprum/adapters/metrics_adapter.py
Add property-based and stateful tests to pin the reducer’s behavior and cross-check real metric accumulation against an independent oracle.	Add focused tests that assert each known phase yields the correct operations, including unit-counter phases, plan/no-op behavior, stdin with/without byte_count, exit combinations of exit_code and duration_s, and unknown-phase error raising. Introduce composable Hypothesis strategies for ExecEvent instances with phase-appropriate fields, used by both property tests and the state machine. Implement a RuleBasedStateMachine that feeds random ExecEvents into a real MetricsHook backed by InMemoryMetrics while maintaining an independent per-phase counter/duration oracle, with invariants asserting the collector’s counters and histograms match the oracle at every step and only expected histograms exist. Register the state machine as TestMetricsAccumulation with custom Hypothesis settings to control the number of examples and steps.	cuprum/unittests/test_metrics_adapter_stateful.py
Regenerate test snapshot metadata to reflect the updated wheel manifest.	Update the maturin build snapshot file so snapshot tests remain consistent with the current wheel manifest.	cuprum/unittests/__snapshots__/test_maturin_build.ambr

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#78	Extract a pure event-to-operation reducer from the metrics hook in cuprum/adapters/metrics_adapter.py to make metric behaviour verifiable independently of the collector.	✅
#78	Add Hypothesis-based property and stateful tests for the metrics hook in cuprum/adapters/metrics_adapter.py to systematically exercise varied event phase/order combinations and prove counters and histograms are created exactly when intended.	✅
#78	Add similar Hypothesis stateful tests for tracing and logging hooks (including proving active maps drain correctly) as referenced in the issue title.	❌	The PR explicitly scopes its work to the metrics adapter only, adding reducer extraction and stateful tests for MetricsHook and InMemoryMetrics. It does not modify or add tests for tracing or logging hooks, nor does it address active map draining.

---

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.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cuprum/adapters/metrics_adapter.py`:
- Line 200: Update the _MetricOp type alias to use the PEP 695 type statement
for the _CounterOp | _HistogramOp union, preserving the existing member types
and avoiding the legacy assignment syntax.
- Around line 226-249: Refactor `_metric_operations` to dispatch on
`event.phase` using a `match`/`case` statement rather than the current chained
top-level `if` branches. Preserve the existing behavior for `plan`, mapped
counter phases, `stdin` byte counts, `exit` via `_exit_operations`, and unknown
phases raising `_UnhandledMetricsPhaseError`; keep the nested `stdin` byte-count
check intact.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4739c9b9-c96c-4693-85fe-4df36863bc7f

📥 Commits

Reviewing files that changed from the base of the PR and between 302858c and 9d352de.

📒 Files selected for processing (3)

• cuprum/adapters/metrics_adapter.py
• cuprum/unittests/__snapshots__/test_maturin_build.ambr
• cuprum/unittests/test_metrics_adapter_stateful.py

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/shared-actions (auto-detected)
• leynos/pylint-pypy-shim (auto-detected)
• leynos/whitaker (auto-detected)

[pandalump]

Raised #251 to number this diagram once both PRs land.

The §8.4 diagram here is deliberately unnumbered: #245 inserts two figures earlier in §8 and pushes the old 4 and 5 to 6 and 7, so the correct number for this one is Figure 6 if #247 merges first and Figure 8 once #245 lands. Any fixed number would be wrong under one of the two merge orders, so it carries only its screen-reader caption, which is never wrong. No prose cross-references figure numbers, so nothing breaks in the interim.

#251 records the expected end state (Figures 3–8 with sections and subjects) and asks for a check that #245's renumbering survived the merge, since both PRs touch §8 prose.

While auditing for that issue I found three adjacent things and put them in #251 as decisions rather than acting on them here:

• docs/cuprum-design.md has 8 Mermaid diagrams but only 3 numbered figures; §8 numbers without screen-reader captions and §13 does the reverse.
• One diagram (§"Implementation notes for the first iteration", ~line 548) has neither caption, which contravenes both "caption every diagram" and "add screen reader descriptions before complex diagrams" in the documentation style guide. That is the only real accessibility gap of the eight and is worth fixing regardless of the numbering decision.
• Numbering starts at 3 — there is no Figure 1 or 2 — and the style guide's own example uses the italic _Figure N: ..._ form while this document uses plain Figure N:.

None of those are regressions from this PR, so they are flagged rather than folded in.

[pandalump]

Both findings verified against current code and fixed in bf74ced. Neither was stale.

1. PEP 695 alias

Valid. The project targets >=3.12, and PEP 695 type statements are already the established form here — four in cuprum/sh.py and two in cuprum/events.py (including ExecPhase, which this module consumes). The legacy assignment was the outlier.

type _MetricOp = _CounterOp | _HistogramOp

I checked the one thing that could have made this unsafe: a type alias produces a lazy TypeAliasType, which breaks runtime uses such as isinstance or a class pattern. _MetricOp appears only in annotations (_exit_operations, _metric_operations, _apply, and one list[_MetricOp] local), and the module has postponed evaluation, so there is no runtime impact. make typecheck is clean.

2. match/case dispatch

Valid, and it restores the idiom used elsewhere — MetricsHook._apply in this same module, and _subprocess_timeout.

One deliberate detail worth flagging. The mapped unit-counter phases stay keyed by _PHASE_COUNTERS behind a guard clause rather than being repeated as a literal alternation:

match phase:
    case "plan":
        return ()
    case "stdin":
        if event.byte_count is None:
            return ()
        return (_CounterOp("cuprum_stdin_bytes_total", float(event.byte_count)),)
    case "exit":
        return _exit_operations(event)
    case _ if (counter_name := _PHASE_COUNTERS.get(phase)) is not None:
        return (_CounterOp(counter_name, 1.0),)
    case _:
        raise _UnhandledMetricsPhaseError(phase)

Writing case "start" | "stdout" | "stderr" | "stdin_error": instead would duplicate the table's key list in the pattern, giving two places to update when a phase is added. The guard keeps the metric names defined exactly once. The guard is also order-safe: none of the table's keys overlap plan, stdin, or exit.

There is history here worth recording. An earlier revision of this function was a match over all eight phases, and was converted to chained ifs because ruff flagged C901 (complexity 9 > 8) and PLR0911 (8 returns > 6). Keeping the table lookup as a single guard clause rather than expanding it into four separate cases holds the structure under both limits — ruff is clean on this form.

plan, the nested stdin byte-count check, exit via _exit_operations, and the unknown-phase _UnhandledMetricsPhaseError all behave exactly as before.

Behaviour preserved, verified rather than assumed. Two mutations of the new dispatch are both caught by the existing suite:

• dropping the stdin byte-count check → test_stdin_yields_bytes_counter_only_when_counted and the stateful machine fail;
• returning () for an unknown phase instead of raising → both test_unknown_phase_raises_structured_error tests fail with DID NOT RAISE.

Validation

Command	Outcome
uv run pytest cuprum/unittests/test_metrics_adapter.py	13 passed
uv run pytest cuprum/unittests/test_metrics_adapter_stateful.py	9 passed
make check-fmt	pass
make lint	pass (ruff — no C901/PLR0911 — interrogate 100%, pylint 10.00/10, clippy)
make typecheck	pass (ty clean, confirming the lazy alias is fine)
make test	pass — Rust nextest 57/57, full Python suite green
make markdownlint / make nixie	pass

[pandalump]

@coderabbitai The Linked Issues check is right that the PR is narrower than #78's title, and I have resolved the gap rather than restating the scope.

#78 is titled "Hypothesis stateful tests for metrics/tracing/logging hooks", but its body scopes the work to cuprum/adapters/metrics_adapter.py (around line 206), and its stated verifiability improvement — "extract event-to-operation reducers from side-effecting collector/span calls" — is the metrics reducer this PR extracts. Taking the title at its word, though, the three adapters stand as follows:

Adapter	Randomised event-sequence coverage
tracing_adapter.py	cuprum/unittests/test_tracing_span_stateful.py — already present before this PR
metrics_adapter.py	cuprum/unittests/test_metrics_adapter_stateful.py — added here
logging_adapter.py	none

So tracing was already covered and only logging was outstanding. I have raised #252 for it.

Worth noting why that follow-up is not a fourth state machine: RuleBasedStateMachine earns its keep in the other two adapters because each holds an active map keyed by ExecId (TracingHook._active_spans, and the metrics collector's in-flight state), and the interesting bugs are drain and correlation bugs across interleaved sequences. structured_logging_hook has no such state — it maps a phase to a level, builds an extra mapping, and emits exactly one record per event. There is no map to drain, so a state machine would generate interleavings that cannot distinguish any two implementations. #252 specifies @given properties instead, with the cuprum_-prefix collision check and the JsonLoggingFormatter round-trip as the ones that carry real risk.

Keeping this PR to the metrics reducer therefore matches #78's body, and #252 carries the remainder of its title.

[coderabbitai]

Tip

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

@pandalump Record #78 as satisfied for this PR’s metrics-reducer scope. Keep the logging coverage work in #252: @given properties target the meaningful risks for the stateless structured_logging_hook, while state machines remain justified for adapters with ExecId-keyed lifecycle state.

🐇🔍

---

✏️ Learnings added

Learnt from: pandalump
URL: https://github.com/leynos/cuprum/pull/247

Timestamp: 2026-07-29T13:33:25.349Z
Learning: For cuprum telemetry tests, use Hypothesis RuleBasedStateMachine coverage where adapter behaviour depends on ExecId-keyed active or in-flight state and event interleavings can expose drain or correlation defects (for example, tracing and metrics). The stateless structured_logging_hook should instead use focused given property tests, including cuprum_-prefix collision handling and JsonLoggingFormatter round-trips; this follow-up is tracked in GitHub issue `#252`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/cuprum-design.md (1)

1475-1477: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Use one complete phase contract in both documents.

The shared event contract omits stdin and stdin_error, although the metrics reducer handles them. Both statements about the “documented phase contract” are therefore incomplete.

• docs/cuprum-design.md#L1475-L1477: Add the supported phases to the event and metrics descriptions, or scope the wording to the adapter's phase set.
• docs/developers-guide.md#L282-L284: Apply the same phase-contract wording used in the design document.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/cuprum-design.md` around lines 1475 - 1477, The phase-contract wording
is inconsistent because the shared event contract omits stdin and stdin_error
while the metrics reducer supports them. Update docs/cuprum-design.md lines
1475-1477 and docs/developers-guide.md lines 282-284 with the same complete
phase-contract wording, either listing stdin and stdin_error among supported
phases or explicitly scoping the statements to the adapter’s phase set.

cuprum/unittests/test_metrics_adapter_stateful.py (1)

261-277: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a named exception for the simulated collector failure.

Replace the generic RuntimeError with the package exception hierarchy or a narrowly named test exception ending in Error. Assert that exact exception type in the failure-path test. Keep the injected backend failure explicit.

As per coding guidelines, “Raise specific built-in exceptions or domain-specific exceptions instead of generic Exception or catch-all RuntimeError.”

Also applies to: 280-303

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cuprum/unittests/test_metrics_adapter_stateful.py` around lines 261 - 277,
Update _FailingHistogramCollector.observe_histogram to raise a specific named
exception, using the package exception hierarchy or a narrowly scoped test
exception ending in Error, while keeping the injected backend rejection
explicit. Update the failure-path test covering this collector to assert that
exact exception type instead of RuntimeError.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@cuprum/unittests/test_metrics_adapter_stateful.py`:
- Around line 261-277: Update _FailingHistogramCollector.observe_histogram to
raise a specific named exception, using the package exception hierarchy or a
narrowly scoped test exception ending in Error, while keeping the injected
backend rejection explicit. Update the failure-path test covering this collector
to assert that exact exception type instead of RuntimeError.

In `@docs/cuprum-design.md`:
- Around line 1475-1477: The phase-contract wording is inconsistent because the
shared event contract omits stdin and stdin_error while the metrics reducer
supports them. Update docs/cuprum-design.md lines 1475-1477 and
docs/developers-guide.md lines 282-284 with the same complete phase-contract
wording, either listing stdin and stdin_error among supported phases or
explicitly scoping the statements to the adapter’s phase set.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8d5f389d-4c42-4a5a-bf63-c4106053e3d5

📥 Commits

Reviewing files that changed from the base of the PR and between 1c094f2 and f13e304.

📒 Files selected for processing (4)

• cuprum/adapters/metrics_adapter.py
• cuprum/unittests/test_metrics_adapter_stateful.py
• docs/cuprum-design.md
• docs/developers-guide.md

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/shared-actions (auto-detected)
• leynos/pylint-pypy-shim (auto-detected)
• leynos/whitaker (auto-detected)

[pandalump]

Both outstanding findings are addressed in fe8f8b8. They were posted as
outside diff range comments, so there is no inline thread to reply to;
this is the reply. Acting on the second one turned up a third defect that
neither review caught.

1. One complete phase contract in both documents — taken

Verified first. ExecPhase (cuprum/events.py:20-28) has seven members:
plan, start, stdout, stderr, exit, stdin, stdin_error.
_metric_operations has an arm for every one of them — plan,
stdin, and exit explicitly, and start/stdout/stderr/stdin_error
via _PHASE_COUNTERS. So the reducer is not total over some subset; it is
total over the whole of ExecPhase.

The finding is right that the shared event contract was the weaker
document. §7.1 listed five phases both in prose and in the ExecEvent
sketch, and §8.1.3 said "Cuprum emits plan, start, stdout, stderr,
and exit". The two "documented phase contract" sentences then leant on
that incomplete list.

Rather than scope the wording down to an adapter subset — which would have
been the less true of the two options offered — I named the seven phases
where the event contract is introduced, and used one wording in both
docs/cuprum-design.md and docs/developers-guide.md.

I extended the fix to two places the finding did not cite, because they
were the same defect:

• docs/cuprum-design.md §7.1 — the source of the incomplete list, both
  the prose bullets and the Literal[...] in the ExecEvent sketch.
• docs/users-guide.md — the cuprum_phase field listed the same five
  values. That one is worth stating precisely: the structured logging
  adapter's _format_message is fail-open (case _: returns a generic
  cuprum.<phase> message), so it really does emit records with
  cuprum_phase=stdin and cuprum_phase=stdin_error.

The new wording also states the consequence, which is the part that
actually matters: MetricsHook is fail-closed (case _: raise), so a
documented set that lags the handled set is not merely untidy. The two
adapters take opposite stances on an unknown phase, and the docs now say
so.

2. Named exception for the simulated collector failure — taken

Checked what production does with a collector failure before choosing, and
the answer changed the shape of the fix.

Hook exceptions are not isolated. _emit_exec_event
(cuprum/_observability.py:85-99) logs observe_hook_failed and then
raises _ExecEventEmissionError; its sole call site,
_StageObservation.emit (cuprum/_pipeline_types.py:104-107), unwraps
that and re-raises exc.error — the collector's original exception.
test_cqrs_helpers.py:209 already pins this with
pytest.raises(_SyncObserveHookError). Confirmed end to end: a raising
observe hook makes run_sync() raise that hook's own exception type.

So a bare RuntimeError was understating the contract, not just tripping a
lint rule — pytest.raises(RuntimeError) cannot distinguish the injected
failure from an incidental one, and the test stopped at the hook boundary.

• _FailingHistogramCollector now raises _MetricsBackendError, a
  module-level test exception following the convention already in
  test_cqrs_helpers.py:43-52 (_AsyncObserveHookError,
  _SyncObserveHookError). The injected backend failure stays explicit.
• The failure-path test asserts that exact type.
• Added test_a_failing_collector_fails_the_command, which drives a real
  command through a failing collector and asserts the backend's own
  exception type reaches the caller of run_sync unchanged.

Non-vacuity check: replacing raise exc.error from exc in
_pipeline_types.py with a bare return turns the new test red
("DID NOT RAISE"), while the other ten still pass. It pins the contract
rather than restating the hook's internals.

3. A claim the code contradicts — found while verifying (2)

Not in either review. Three places asserted that
_emit_exec_event "catches it, logs observe_hook_failed, and lets the
command continue — a broken metrics backend must not fail the user's
command":

• cuprum/adapters/metrics_adapter.py (MetricsHook.__call__ docstring)
• docs/cuprum-design.md §"The exception propagates"
• docs/developers-guide.md

That is backwards. _emit_exec_event logs and re-raises; the command
dies with the collector's exception. The stateful test's docstring carried
the same error ("which isolates it, so the command survives"). All four are
corrected to describe the actual escalation path, including why the wrapper
exists — _ExecEventEmissionError carries already-scheduled observe tasks
through cleanup, it does not absorb the failure.

git log -S confirms the claim entered with this PR (1c094f2, de3f645)
and is absent from origin/main, so it is ours to fix rather than
inherited.

Bearing on the #243 / #244 counter declines

Both PRs declined an "add metrics counters" recommendation on the grounds
that a new ExecPhase would make the fail-closed MetricsHook raise for
every caller who has already registered it. Nothing here weakens that: this
PR does not make the match tolerant and introduces no facade that would
accept a new phase safely — _metric_operations still ends case _: raise.

If anything the declines were understated. Because the exception is
re-raised rather than swallowed, the consequence of adding a phase without
an arm is not a lost metric but a failed command for every such caller.
Those declines stand, and on firmer ground than was recorded at the time.

Gates

check-fmt, lint, typecheck, test, markdownlint, nixie, and
mbake validate Makefile all green. cs delta origin/main HEAD reports one
finding, a Code Duplication in cuprum/unittests/test_maturin_build.py;
that file is untouched by this commit and unchanged on this branch since
the merge-base (75387d7) — it arrived with the base via #240.

[codescene-access]

No quality gates enabled for this code.

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope).

#78 Add similar Hypothesis stateful tests for tracing and logging hooks (including proving active maps drain correctly) as referenced in the issue title. ❌ The PR explicitly scopes its work to the metrics adapter only, adding reducer extraction and stateful tests for MetricsHook and InMemoryMetrics. It does not modify or add tests for tracing or logging hooks, nor does it address active map draining.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== Issue `#78` =='
gh issue view 78 --repo leynos/cuprum --json number,state,title,body,url \
  --jq '{number,state,title,url,body}' || true

printf '%s\n' '== Relevant test files =='
fd -a -t f '^(test_metrics_adapter_stateful|test_tracing_span_stateful|test_logging_adapter_properties)\.py$' cuprum/unittests

printf '%s\n' '== Relevant symbol outlines =='
for file in \
  cuprum/adapters/metrics_adapter.py \
  cuprum/adapters/tracing_adapter.py \
  cuprum/adapters/logging_adapter.py \
  cuprum/unittests/test_metrics_adapter_stateful.py \
  cuprum/unittests/test_tracing_span_stateful.py \
  cuprum/unittests/test_logging_adapter_properties.py
do
  if [ -f "$file" ]; then
    echo "--- $file"
    ast-grep outline "$file" --items all || true
  fi
done

printf '%s\n' '== Direct coverage evidence =='
rg -n -C 3 \
  'RuleBasedStateMachine|_active_spans|active\.keys|_active_spans\.keys|assert .*not in .*_active_spans|`@given`|structured_logging_hook|JsonLoggingFormatter|cuprum_' \
  cuprum/adapters/tracing_adapter.py \
  cuprum/unittests/test_metrics_adapter_stateful.py \
  cuprum/unittests/test_tracing_span_stateful.py \
  cuprum/unittests/test_logging_adapter_properties.py \
  || true

Length of output: 37274

---

@leynos The failed #78 check is resolved.

Verify these completed areas:

• cuprum/unittests/test_metrics_adapter_stateful.py tests MetricsHook and InMemoryMetrics with a Hypothesis RuleBasedStateMachine. It checks counters and histograms against an independent oracle.
• cuprum/unittests/test_tracing_span_stateful.py tests interleaved events for two ExecId values that share one PID. It verifies that TracingHook._active_spans matches an independent model after every step. It also verifies that exit removes the matching active span.
• cuprum/unittests/test_logging_adapter_properties.py adds five Hypothesis @given properties for structured_logging_hook. They verify one record per event, cuprum_-prefixed extra fields, JSON formatting with non-native tag values, non-empty messages, and correct configured levels.

Do not add a logging state machine. structured_logging_hook has no active map or cross-event lifecycle state. Its property tests cover the relevant per-event risks.

Do not open a follow-up issue. No remaining work exists for this check.

Close #78. The issue remains open, but the current PR satisfies its stated coverage and active-map-draining requirements.

_{You are interacting with an AI system.}