Enable the ASYNC (flake8-async) ruff rule group (#221)

[leynos]

Summary

Closes #221.

The ASYNC (flake8-async) rule group was not in ruff's select list, so
async-correctness lints such as ASYNC109 and ASYNC240 never ran. This change
enables the group and resolves the eight findings it surfaced, treating each
finding on its merits rather than with a blanket suppression.

Changes

• Adopt asyncio.timeout in the internal wait helpers (ASYNC109 ×2).
  _wait_for_exit_code and _run_subprocess_with_streams no longer take a
  timeout parameter; the deadline now belongs to the caller via
  async with asyncio.timeout(...), the modern replacement for
  asyncio.wait_for. A deadline expiry now arrives as CancelledError and is
  torn down identically to an external cancellation, while asyncio.timeout
  re-raises expiry as TimeoutError at the context boundary. Existing timeout,
  termination, and consumer-draining behaviour is unchanged.
• Rename carried metadata (ASYNC109 ×1, false positive).
  _handle_stream_timeout's timeout parameter is renamed to
  configured_timeout, reflecting that it populates the raised error rather
  than bounding an await.
• Keep the public run(timeout=…) API (ASYNC109 ×2).
  SafeCmd.run and Pipeline.run retain their documented timeout keyword,
  which mirrors subprocess.run(timeout=…); each carries a per-line
  # noqa: ASYNC109 with a rationale comment.
• Scope test scaffolding (ASYNC109 ×1, ASYNC240 ×2).
  The async PID-file polling helper uses asyncio-only primitives, so ASYNC109
  and ASYNC240 are added to the existing **/test_*.py per-file-ignore with
  an explanatory comment (trio/anyio async paths are not in use).
• Enable the rule. "ASYNC" is added to [tool.ruff.lint].select.

Verification

• make check-fmt, make lint, make typecheck, and make test all pass.
• uv run ruff check --select ASYNC . reports no findings.
• The affected subprocess timeout/cancellation unit and behaviour tests pass,
  confirming the asyncio.timeout refactor preserves behaviour.
• CodeRabbit (coderabbit review --agent) reported zero findings.

References

• Issue: Enable the ASYNC (flake8-async) Ruff rule group and resolve its findings #221
• Lody session: https://lody.ai/leynos/sessions/4501495f-9c83-4ef7-98a3-5d62dcfb38a2

🤖 Generated with Claude Code

[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]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5f6e49ba-e84a-459e-b0e9-c39bef5f8719

📥 Commits

Reviewing files that changed from the base of the PR and between 88fb97a and 59e1935.

📒 Files selected for processing (19)

• cuprum/_pipeline_types.py
• cuprum/_subprocess_execution.py
• cuprum/_subprocess_timeout.py
• cuprum/adapters/metrics_adapter.py
• cuprum/adapters/tracing_adapter.py
• cuprum/events.py
• cuprum/sh.py
• cuprum/unittests/__snapshots__/test_adapter_projection.ambr
• cuprum/unittests/__snapshots__/test_maturin_build.ambr
• cuprum/unittests/_adapter_test_support.py
• cuprum/unittests/test_async_timeout_docs.py
• cuprum/unittests/test_metrics_adapter.py
• cuprum/unittests/test_observe.py
• cuprum/unittests/test_subprocess_timeout.py
• cuprum/unittests/test_tracing_adapter.py
• docs/adr-007-subprocess-execution-module-boundaries.md
• docs/developers-guide.md
• docs/users-guide.md
• pyproject.toml

Walkthrough

Subprocess exit waiting now separates process termination and stream-consumer draining from timeout handling. Timeout metadata is clarified, public timeout parameters retain lint suppressions, regression tests cover non-positive deadlines, and Ruff enables async checks.

Changes

Subprocess timeout lifecycle

Layer / File(s)	Summary
Exit waiting and consumer teardown cuprum/_subprocess_execution.py	Process waiting now uses dedicated timeout-aware logic, terminates processes before cancelling and draining consumers, and updates all execution paths and exports.
Timeout metadata and public API annotations cuprum/_subprocess_timeout.py, cuprum/sh.py	Stream timeout metadata is renamed to configured_timeout, while public run timeout parameters retain explanatory comments and lint suppressions.
Regression coverage and async lint configuration cuprum/unittests/test_subprocess_timeout.py, pyproject.toml	Tests cover cancellation cleanup and non-positive deadlines; Ruff enables ASYNC checks and ignores selected async rules in test files.

Sequence Diagram(s)

sequenceDiagram
  participant SafeCmd
  participant WaitHelper
  participant Process
  participant StreamConsumers
  SafeCmd->>WaitHelper: pass execution.timeout
  WaitHelper->>Process: await process.wait()
  WaitHelper->>Process: terminate on timeout or cancellation
  WaitHelper->>StreamConsumers: cancel and gather pending tasks
  StreamConsumers-->>SafeCmd: drained stream results

Loading

Possibly related PRs

• leynos/cuprum#158: Refactors related subprocess exit-wait teardown and stream-consumer cleanup.

Suggested labels: Issue

Suggested reviewers: codescene-access, codescene-delta-analysis

Poem

Processes wait, then pipes grow still,
Consumers drain at timeout’s will.
Async rules now guard the stream,
Deadlines sharpen every dream.
Ruff hums softly, tests take flight.

🚥 Pre-merge checks | ✅ 17 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
User-Facing Documentation	⚠️ Warning	Document the new non-positive-timeout behaviour in docs/users-guide.md; the guide only covers generic timeout expiry, and no migration note exists.	Add a users-guide timeout note for timeout=0/negative immediate expiry and the preserved run(timeout=...) API, or add a migration note if you treat it as breaking.
Developer Documentation	⚠️ Warning	The PR adds new internal subprocess helpers and ASYNC lint policy, but the developer guide and design docs were not updated to describe them.	Update docs/developers-guide.md and the subprocess design/ADR with the new wait helper split, timeout semantics, and ASYNC lint policy; tick any linked roadmap item if one exists.
Observability	⚠️ Warning	Timeout/cancellation refactor changes production behaviour, but no new logs, metrics, spans, or alerts mark the new expiry/teardown branches; only existing exception flow remains.	Add structured logging or observation events at timeout expiry, the non-positive fast-path, and teardown/drain failures; include stable fields such as operation, pid, timeout, and error class.

✅ Passed checks (17 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the change and includes the linked issue number, so it satisfies the stated title rule.
Description check	✅ Passed	The description is clearly about enabling ASYNC and resolving the surfaced findings, so it is on topic.
Linked Issues check	✅ Passed	The PR enables ASYNC, resolves the eight findings, preserves public timeout APIs, and adds the scoped test ignores required by #221.
Out of Scope Changes check	✅ Passed	The changed files all support the ASYNC-rule rollout or the related timeout refactor; no unrelated scope is evident.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Testing (Overall)	✅ Passed	Use PASS: add property-based and async cleanup tests that fail on missing cancellation, drain, or non-positive timeout handling, plus end-to-end timeout coverage.
Module-Level Documentation	✅ Passed	Pass: every touched Python module has a top-level docstring stating its purpose and, where needed, its relation to subprocess execution or tests.
Testing (Unit And Behavioural)	✅ Passed	Accept the new unit tests: they cover cleanup invariants and edge cases, and the behaviour suite exercises the public timeout boundary.
Testing (Property / Proof)	✅ Passed	The new timeout/order invariants are covered by Hypothesis and parametrized regression tests, so no extra property/proof recommendation is needed.
Testing (Compile-Time / Ui)	✅ Passed	PASS: the PR changes async subprocess internals only; no compile-time or UI surface needs trybuild/snapshot coverage, and behaviour is covered by focused unit tests.
Unit Architecture	✅ Passed	PASS: The refactor keeps subprocess side-effects in the internal command layer, makes timeout ownership explicit, and tests exercise doubles at the seam.
Domain Architecture	✅ Passed	Changes stay in private subprocess/runtime modules and tests; ADR 007 frames these as execution/infrastructure boundaries, not domain model code.
Security And Privacy	✅ Passed	PASS: The patch only refactors timeout cleanup, keeps argv-based subprocess exec, and adds comments/tests with fake PIDs; no secrets, auth, injection, permission, or exposure changes.
Performance And Resource Use	✅ Passed	All new loops are fixed-size or deadline-bounded; teardown cancels and drains at most two consumer tasks and uses bounded grace periods, with no new hot-path blocking or unbounded growth.
Concurrency And State	✅ Passed	Ownership stays local, teardown cancels and drains tasks before re-raising, and tests cover timeout, cancellation, non-positive deadlines, and ordering interleavings.
Architectural Complexity And Maintainability	✅ Passed	The refactor stays local and removes duplicated teardown logic; no new layer, registry, or dependency edge was added, and the public API remains intact.
Rust Compiler Lint Integrity	✅ Passed	No Rust sources or Rust lint suppressions were changed; the PR only modifies Python subprocess helpers and tests, so the Rust lint integrity check is not applicable.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-221-enable-the-async-flake8-async-ruff-rule-group-and-resolve-its-findings

---

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b58b6fab3e

ℹ️ 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".

[leynos]

Follow-up: preserve immediate expiry for non-positive timeouts (d1ebe4d)

Addressing the review concern that adopting asyncio.timeout() changed the behaviour of non-positive timeouts.

Unlike the asyncio.wait_for() it replaced, asyncio.timeout() only schedules its cancellation for the next event-loop iteration. A fast, already-exited process whose wait() returns without suspending therefore raced past a zero or negative deadline and reported success, making run(timeout=0) timing-dependent (notably on the stream-capture path).

Both wait call sites now route through _wait_for_exit_code_within_timeout, which special-cases a non-positive execution.timeout: it tears the process down and raises TimeoutError immediately without awaiting wait(), restoring the deterministic expiry the previous implementation guaranteed. The shared terminate-and-drain teardown was extracted into _terminate_and_drain_consumers so the immediate-expiry and cancellation paths reuse identical cleanup. Deterministic regression tests cover zero and negative timeouts against an already-exited process, including consumer draining.

Gates green (check-fmt, lint, typecheck, test) and CodeRabbit reports 0 findings.

[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.

[coderabbitai]

Actionable comments posted: 1

🤖 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/unittests/test_subprocess_timeout.py`:
- Around line 13-17: Move the _SubprocessExecution import into a
TYPE_CHECKING-only block, while keeping _wait_for_exit_code and
_wait_for_exit_code_within_timeout as runtime imports. Preserve the string-based
typ.cast("_SubprocessExecution", ...) usage so no runtime dependency is
introduced.

🪄 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: 309ceeab-7faf-4a34-ad02-e94ede7af196

📥 Commits

Reviewing files that changed from the base of the PR and between aa1fd32 and d1ebe4d.

📒 Files selected for processing (5)

• cuprum/_subprocess_execution.py
• cuprum/_subprocess_timeout.py
• cuprum/sh.py
• cuprum/unittests/test_subprocess_timeout.py
• pyproject.toml

🔗 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)

[leynos]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[leynos]

Follow-up: developer documentation + timeout observability (c0f6f6f, c881860)

Completing the required documentation and observability work in this PR (not deferred to a follow-up issue).

Timeout observability — implemented via the least-invasive existing mechanism: a new cuprum.timeout logger with structured cuprum_* extra fields, following the established cuprum.stdin convention. A new ExecEvent phase was deliberately avoided — it would be shotgun surgery across the public ExecPhase, the duplicated _StageObservation.emit literal, the raising MetricsHook dispatch, the tracing/logging adapters, _support.py, and the auto-parametrised syrupy projection snapshot.

Three distinguishable, best-effort records:

• subprocess_timeout_expired (WARNING) with cuprum_timeout_mode="elapsed" for an elapsed deadline;
• the same record with cuprum_timeout_mode="immediate" for the non-positive fast path;
• subprocess_teardown_drain_failed (ERROR, cuprum_teardown_outcome="drain_error") when a consumer drains with an unexpected exception.

Stable fields: cuprum_operation, cuprum_pid, cuprum_timeout_s, cuprum_error_type, cuprum_timeout_mode, cuprum_teardown_outcome. Emission is swallowed on failure so telemetry can never mask TimeoutExpired/CancelledError, and the public TimeoutExpired and existing exit event are unchanged.

Documentation — developers' guide gains a "Ruff ASYNC (flake8-async) policy" subsection (rationale, public-API # noqa: ASYNC109, test-scaffolding per-file-ignore) and a "Subprocess timeout observability" section; ADR-007 gains a dated addendum (wait-helper split, caller-owned asyncio.timeout, non-positive fast path, _terminate_and_drain_consumers, and the no-orphan invariant). New test_async_timeout_docs.py locks the required wording as documentation-contract tests.

Validation (exact commands / outcomes):

• make check-fmt → pass · make lint → pass (ruff, interrogate 100%, pylint 10.00/10, clippy, typos) · make typecheck → pass · make test → pass (768 passed, 40 skipped, incl. new tests) · make markdownlint → pass · make nixie → pass
• uv run ruff check --select ASYNC . → All checks passed
• uv run pytest cuprum/unittests/test_async_timeout_docs.py cuprum/unittests/test_subprocess_timeout.py → 47 passed
• CodeRabbit review --agent → 0 findings

[leynos]

Follow-up: observe-hook timeout observability + documentation (bdc18a1, ebc84a5, cde4651)

Reworking the timeout observability to go through the existing ExecEvent / sh.observe() observe-hook mechanism (per the review), and completing the developer documentation and documentation-contract tests.

Event contract (smallest compatible extension). Two ancillary phases added to the public ExecPhase — timeout and teardown_error — plus two fields, timeout_s and timeout_mode:

• timeout (before TimeoutExpired): operation="wait", pid, timeout_s, error_type="TimeoutError", timeout_mode ∈ {elapsed_deadline, non_positive_immediate}.
• teardown_error (consumer drain failure during cleanup): operation="drain", pid, error_type (joined failure classes).

Adapters updated: the metrics adapter (whose dispatch raises on unknown phases) counts cuprum_timeouts_total / cuprum_teardown_errors_total; the tracing adapter records them as ancillary span events that leave the span open for the preserved exit. The start/exit events and public TimeoutExpired mapping (with partial output) are unchanged. The parallel cuprum.timeout log records are retained (mirroring the _emit_stdin_error dual log+emit pattern).

Exception precedence. Emission is best-effort via _safe_emit, which swallows a synchronous observe-hook failure (including a hook raising CancelledError) so telemetry can never mask TimeoutExpired/CancelledError; scheduled async-hook tasks are recorded before any raise, so they still drain. Teardown and consumer draining are preserved regardless of emission outcome.

Tests. Unit-level recording-observation tests for elapsed/immediate timeout and teardown_error events; end-to-end sh.observe() tests driving the deterministic non-positive fast path (run_sync(timeout=0)) that assert the event fields, preserved start/exit events, and that a hook failing on the timeout event does not mask TimeoutExpired; adapter unit coverage; regenerated projection snapshot. Existing zero/negative-timeout and consumer-draining regressions retained.

Documentation. Users' guide structured-events section documents the new phases/fields; developers' guide rewrites the timeout observability section for the event contract; ADR-007 records the observe events. test_async_timeout_docs.py locks the required wording (ASYNC policy, users' guide timeout contract, developers' guide + ADR event contract).

Validation (exact commands / outcomes):

• make check-fmt �· make lint ✓ (ruff, interrogate 100%, pylint 10.00/10, clippy, typos) · make typecheck ✓ · make test ✓ (798 passed, 40 skipped + Rust nextest 33/33) · make markdownlint ✓ · make nixie ✓
• uv run ruff check --select ASYNC . → All checks passed
• targeted timeout/observe/doc-contract pytest → all green
• CodeRabbit review --agent → 0 findings (19 files)

[codescene-access]

No quality gates enabled for this code.

[leynos]

Pre-merge check warnings: already addressed at head (59e1935)

These three warnings appear to have been evaluated against an earlier commit. All three were implemented and validated before the branch was rebased onto origin/main — and the rebase rewrote every SHA, so the pre-merge walkthrough is likely still pointing at pre-rebase commits. Evidence against current head follows; no further edits are needed.

1. User-Facing Documentation — non-positive timeout ✅

docs/users-guide.md:368-371 (in the existing ### Timeouts section):

Non-positive timeout behaviour: a timeout of 0 or a negative value is treated as already elapsed, so the command expires immediately, terminating any already-running process without waiting on it and raising TimeoutExpired.

The public run(timeout=...) / run_sync(timeout=...) API is unchanged and still documented in the same section. No migration note was added deliberately: this is not a behavioural change — the non-positive fast path restores the deterministic expiry the superseded asyncio.wait_for() implementation already had (f24c109). A migration note would wrongly imply callers must change code. docs/users-guide.md:643-662 additionally documents the new timeout / teardown_error observe events.

Commits: dbaa099, 59e1935.

2. Developer Documentation — ASYNC policy, wait-helper split, ADR ✅

• docs/developers-guide.md:1414 — "Ruff ASYNC (flake8-async) policy": why the family is selected, the narrowly scoped public-API # noqa: ASYNC109 on SafeCmd.run / Pipeline.run, and the ASYNC109/ASYNC240 test-scaffolding per-file-ignore linked to the adjacent pyproject.toml comments.
• docs/developers-guide.md:1697 — caller-owned asyncio.timeout() deadlines, the _wait_for_exit_code / _wait_for_exit_code_within_timeout split, and _terminate_and_drain_consumers.
• docs/adr-007-subprocess-execution-module-boundaries.md:115 — dated addendum covering caller-owned deadlines, the non-positive fast path, shared teardown, and the no-orphan invariant.
• No roadmap item is linked to Enable the ASYNC (flake8-async) Ruff rule group and resolve its findings #221 (checked docs/roadmap.md), so there is nothing to tick.

Commits: 6debabd, 59e1935.

3. Observability — logs and observe events at all three branches ✅

Both mechanisms are implemented, not just exception flow:

Branch	Observe event	Structured log
Ordinary expiry	timeout, timeout_mode="elapsed_deadline"	subprocess_timeout_expired (WARNING)
Non-positive fast path	timeout, timeout_mode="non_positive_immediate"	same record, distinct mode
Teardown/drain failure	teardown_error	subprocess_teardown_drain_failed (ERROR)

Stable fields are present on both: operation, pid, configured timeout (timeout_s), and error class (error_type), plus timeout_mode and cuprum_teardown_outcome.

• cuprum/events.py:28-29,139-140 — timeout / teardown_error added to the public ExecPhase; timeout_s / timeout_mode fields.
• cuprum/_subprocess_execution.py:152-161,176-185 — emission at both expiry routes; :86-88 — teardown/drain failure.
• cuprum/_subprocess_timeout.py:29,70,99 — the cuprum.timeout logger and both records.
• Adapters updated: cuprum_timeouts_total / cuprum_teardown_errors_total counters and tracing span events.

Emission is best-effort throughout, so telemetry cannot mask TimeoutExpired or CancelledError, and cleanup precedence is preserved.

Commits: dfa480b, b31a8a2, 94aaaea.

Validation at head

uv run pytest cuprum/unittests/test_subprocess_timeout.py cuprum/unittests/test_async_timeout_docs.py cuprum/unittests/test_observe.py → 87 passed, covering exactly these three areas (including documentation-contract tests that assert the required wording mechanically).

Full post-rebase gates all green: make check-fmt, make lint, make typecheck, make test (828 Python + 57 Rust), make markdownlint, make nixie. CodeRabbit's own review --agent reported 0 findings.

Could the pre-merge checks be re-run against head?