feat(hook): P1 every-K-turns Stop-hook cadence (#749)

[robotrocketscience]

Summary

Ships P1 every-K-turns cadence policy (#749), the smallest of the three pre-registered policies in the issue body. The maintainer-self-report on issue comment 4491022396 tripped the body's OR-clause ("manual /clear still required at meaningful rate even with #746 shipped"); the empirical fire-rate audit (6 PreCompact fires across 2 sessions over 6 days post-#746) confirms P0 PreCompact-only is insufficient at the maintainer's workload shape.

Design ratifications

Surfaced under #749 comment 4491022396 + answered in-session:

1. Policy order: P1 every-K-turns first. Cheapest to ship, easiest to falsify, deterministic via fire_idx.
2. Cadence parameter encoding: static config + [cadence] TOML knob, no [exploration] Adaptive half-life as a Bayesian-engine-governed meta-belief #480 meta-belief integration yet. Resolver shape leaves a slot for meta:hook.cadence_k to land later without call-site changes.
3. K default: 15 turns (body-suggested literature-typical starting point).
4. Default-OFF: [cadence] enabled = false by default; operator opts in.

P2 (token-budget watermark) and P3 (turn-density-aware) per the issue body remain unimplemented. The policy enum is forward-compatible so those land as new values without rewriting the resolver or predicate.

Commits (atomic by category)

1. feat(cadence): cadence-policy module — P1 every-K-turns scaffold (#749) — new src/aelfrice/cadence.py with CadenceConfig dataclass, env > kwarg > TOML > default resolvers (resolve_cadence_enabled / resolve_cadence_policy / resolve_cadence_k), and the pure should_fire(fire_idx, config) predicate (fire_idx > 0 AND fire_idx % k == 0, gated on enabled AND policy == "p1_every_k_turns").
2. test(cadence): unit tests for resolvers + firing predicate (#749) — 30 tests in tests/test_cadence.py covering predicate cross-product (enabled / policy / k including k=0/k=1/negative/zero-fire_idx cold-start guard), TOML fail-soft on missing-file/section/malformed/wrong-type/unknown-policy/non-positive-k/bool-as-k, and resolver precedence including unparseable-env falls-through.
3. feat(hook): wire P1 every-K-turns cadence into Stop hook (#749) — new _maybe_fire_cadence_checkpoint(payload, session_id, serr) helper called from stop() after the existing lock-prompt path. Reads next_fire_idx from session_ring.read_ring_state (same monotonic counter UPS maintains), checks should_fire, on True runs _rebuild_and_format with rebuild_log_enabled=True and discards the body (Stop has no additionalContext channel — value is in the rebuild_log + touch-state side effects). Emits stderr observability line aelfrice: cadence checkpoint fired @ fire_idx=N (policy=..., k=...). Restructured stop() so zero-lock-candidate sessions no longer early-return before cadence gets a chance to run; lock-prompt behaviour unchanged (41 existing tests pass).
4. test(hook): integration tests for Stop-hook cadence wiring (#749) — 11 tests in tests/test_hook_stop_cadence.py against the actual hook.stop() entry point covering default-OFF, policy-off-with-enabled, the multiples-of-K firing pattern (15/30/45 fire; 1/14/16/29 skip), cold-start fire_idx=0 suppression, missing/session-mismatched ring sentinel, empty recent-turns slice, env-override beats TOML, stderr line carries policy+k+fire_idx, the zero-lock-candidate regression, and lock-prompt + cadence coexistence.
5. docs(changelog): unreleased entry for #749 P1 every-K-turns cadence — CHANGELOG/v3.md [Unreleased] ### Added entry.

Determinism (#605)

The firing predicate is a pure function of (fire_idx, k). No wall-clock, no random sampling. A replay with the same UPS sequence and same config produces the same fire decisions. The fire_idx itself comes from session_ring.next_fire_idx which is monotonic by construction.

Test plan

• tests/test_cadence.py — 30 unit tests pass.
• tests/test_hook_stop_cadence.py — 11 integration tests pass.
• tests/test_hook_stop_lock_prompt.py + tests/test_setup_stop_hook.py — 41 pre-existing Stop hook tests pass (no regression on the lock-prompt path).
• Full pytest tests/ --ignore=tests/bench_gate --ignore=tests/context_rebuilder — 4502 passed, 34 skipped, 75 xfailed. No regressions outside the cadence module.

Out of scope

• P2 / P3 implementation (separate consumer PRs against the same [cadence] policy enum surface).
• [exploration] Adaptive half-life as a Bayesian-engine-governed meta-belief #480 meta:hook.cadence_k meta-belief integration (resolver shape is ready; the meta-belief read slots in without call-site changes).
• The hot-path / Cross-fire injection dedup: amortize search hooks against the UPS-fire memory block #740 / feat: cross-fire injection dedup ring (#740) #744 schema decisions per the issue body §"Out of scope".
• Suppress-mode rebuilder (parked for v2.x per context_rebuilder.py).

Discretion

No ~/.claude/-derived content in any new or modified file. Pre-push grep clean against the deny-list. Pre-existing host-product references in src/aelfrice/hook.py are untouched; this PR introduces zero new instances of any flagged token.

Summary by Sourcery

Introduce a configurable P1 every-K-turns cadence policy for the Stop hook that periodically triggers context rebuilds based on a monotonic turn counter, while keeping the feature opt-in and preserving existing lock-prompt behavior.

New Features:

• Add a cadence configuration module with a P1 every-K-turns policy, environment/TOML-based resolvers, and a deterministic firing predicate for periodic checkpoints.
• Wire the cadence policy into the Stop hook so it can trigger rebuild checkpoints independently of PreCompact and lock-prompt candidates, with stderr observability of firings.

Enhancements:

• Refactor the Stop hook flow to avoid early returns on empty lock-candidate sets so cadence checkpoints can still execute.
• Document the new Stop-hook cadence policy and configuration surface in the v3 changelog.

Tests:

• Add unit tests for cadence configuration resolution and firing logic across env, TOML, and edge cases.
• Add integration tests for Stop-hook cadence behavior, covering configuration combinations, ring-state conditions, interaction with lock prompts, and stderr output.

Summary by CodeRabbit

Release Notes

• New Features

  • Added periodic checkpoint cadence for the Stop hook: triggers rebuilds every K turns independently of PreCompact timing (disabled by default)
  • Cadence configuration via TOML or environment variables with clear precedence rules
  • Cadence checkpoints now run even when lock candidates are unavailable

• Tests

  • Added comprehensive unit and integration test coverage for cadence configuration, decision logic, and Stop-hook behavior

• Documentation

  • Updated changelog with cadence policy specifications and configuration guide

[sourcery-ai]

Reviewer's Guide

Implements a configurable P1 "every-K-turns" cadence policy for the Stop hook by adding a dedicated cadence module, wiring it into the Stop hook’s flow after the lock-prompt path, and covering it with focused unit and integration tests plus changelog documentation.

File-Level Changes

Change	Details	Files
Introduce cadence configuration module and deterministic every-K-turns firing predicate.	Add src/aelfrice/cadence.py with CadenceConfig dataclass and TOML-backed [cadence] section parsing (enabled, policy, k) with env > kwarg > TOML > default precedence. Implement resolve_cadence_enabled/resolve_cadence_policy/resolve_cadence_k helpers with defensive env parsing and parent-dir .aelfrice.toml discovery. Define POLICY_OFF and POLICY_P1_EVERY_K_TURNS plus should_fire(fire_idx, config) predicate that fires only for enabled P1 policy at positive multiples of k, ignoring non-positive indices or k values.	src/aelfrice/cadence.py
Wire cadence policy into the Stop hook so cadence checkpoints can fire independently of lock candidates while remaining fail-soft.	Restructure hook.stop to treat _open_store failures as non-fatal, avoid early-returns when there are zero lock candidates, and always close the store while preserving existing autolock and prompt behavior. Add _maybe_fire_cadence_checkpoint(payload, session_id, serr) helper that lazily imports cadence/session_ring, resolves cadence config from cwd, reads next_fire_idx from ring state, checks should_fire, and, when true, runs _rebuild_and_format with rebuild_log enabled, then prints a structured stderr observability line. Guard cadence path with multiple short-circuits (disabled policy, non-P1 policy, invalid or missing fire index, empty recent slice, missing DB file) and wrap the call in stop() with a defensive try/except that logs but never raises.	src/aelfrice/hook.py
Add unit and integration tests validating cadence resolution and Stop-hook integration behavior.	Create tests/test_cadence.py to cover should_fire across edge cases, TOML parsing behavior (including malformed/typed-wrong values and parent-dir walk), and resolver precedence including env fall-through semantics. Create tests/test_hook_stop_cadence.py to exercise hook.stop with cadence disabled, policy off, P1 firing pattern at specific fire_idx values, cold-start and mismatch guards, env overrides, empty recent slices, interactions with missing ring state or DB, and coexistence with lock prompts. Use monkeypatch-based helpers to isolate environment, seed DBs, stub _rebuild_and_format and _read_recent_for_pre_compact, and manufacture ring-state payloads for deterministic scenarios.	tests/test_cadence.py tests/test_hook_stop_cadence.py
Document the new Stop-hook cadence policy in the v3 changelog.	Add a detailed "Stop-hook cadence policy — P1 every-K-turns checkpoint" entry under the Unreleased/Added section describing motivation, configuration knobs, behavior, determinism guarantees, and test coverage.	CHANGELOG/v3.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

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

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request implements a periodic cadence checkpoint system for the Stop hook. The feature allows rebuilds to execute on every K-turn boundary, controlled via TOML configuration or environment variables, and runs independently of lock-prompting logic. It includes configuration loading with precedence rules, a pure deterministic firing predicate, Stop hook integration via a new helper, comprehensive unit and integration tests, and changelog documentation.

Changes

Stop-hook Cadence Checkpoint Policy

Layer / File(s)	Summary
Cadence configuration and firing predicate src/aelfrice/cadence.py, tests/test_cadence.py	New CadenceConfig dataclass and configuration loader that searches upward for .aelfrice.toml, resolves settings via environment variables > explicit arguments > TOML > defaults. Resolver functions enforce env > explicit > TOML > default precedence. Pure should_fire(fire_idx, config) predicate returns true only when enabled, policy is p1_every_k_turns, k is positive, and fire_idx % k == 0. Unit tests validate the predicate, TOML parsing, directory walking, type validation, and precedence rules.
Stop hook cadence checkpoint integration src/aelfrice/hook.py	stop() refactored to decouple lock prompting from cadence: store-open failures degrade gracefully, lock candidate processing runs only when candidates exist, and cadence checkpoint execution runs independently via new _maybe_fire_cadence_checkpoint helper regardless of candidate presence or auto-lock outcome. Helper reads cadence config and session next_fire_idx, checks should_fire, loads rebuilder config and recent turns, and executes _rebuild_and_format; fails soft on missing config/state with stderr logging.
Integration tests for Stop hook cadence tests/test_hook_stop_cadence.py	Validates cadence gating (disabled by default, policy-off, cold-start guard at fire_idx=0, ring-state/session/recent-turn presence checks), configuration precedence (environment variable overrides TOML), regression coverage (fires without lock candidates), observability (stderr includes policy and k), and coexistence with existing lock-prompt output. Monkeypatches rebuild and recent-turn functions to avoid transcript dependency.
Feature documentation CHANGELOG/v3.md	Changelog entry describing P1 every-K-turns checkpoint behavior, cadence configuration options, deterministic firing logic, Stop-hook wiring, default-OFF semantics, stderr observability, and test coverage.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat(hook): P1 every-K-turns Stop-hook cadence (#749)' clearly and concisely describes the main change: adding a P1 every-K-turns cadence policy feature to the Stop hook.
Description check	✅ Passed	The PR description is comprehensive and well-structured, covering summary, design ratifications, atomic commits, determinism guarantees, test plan results, out-of-scope items, and discretion notes.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 feat/issue-749-stop-hook-cadence-p1

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

PR-size soft cap

This PR is over the advisory size threshold:

• 1244 changed lines (limit: 200)
• 5 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In _maybe_fire_cadence_checkpoint you call resolve_cadence_enabled, resolve_cadence_policy, and resolve_cadence_k, each of which can traverse the filesystem and parse TOML; consider resolving the cadence config once (or threading a preloaded config into the resolvers) to avoid 2–3x redundant IO on every Stop.
• The TOML [cadence] policy value is treated as case-sensitive while the env policy is normalized to lowercase; you may want to normalize the TOML value as well so policy = "P1_EVERY_K_TURNS" and similar variants are accepted consistently.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `_maybe_fire_cadence_checkpoint` you call `resolve_cadence_enabled`, `resolve_cadence_policy`, and `resolve_cadence_k`, each of which can traverse the filesystem and parse TOML; consider resolving the cadence config once (or threading a preloaded config into the resolvers) to avoid 2–3x redundant IO on every Stop.
- The TOML `[cadence] policy` value is treated as case-sensitive while the env policy is normalized to lowercase; you may want to normalize the TOML value as well so `policy = "P1_EVERY_K_TURNS"` and similar variants are accepted consistently.

## Individual Comments

### Comment 1
<location path="src/aelfrice/hook.py" line_range="2607-2609" />
<code_context>
+    body — Stop has no ``additionalContext`` channel, so the value is
+    in the rebuild_log + touch-state side effects.
+
+    Fail-soft: any error short-circuits to a stderr line; never raises.
+    Default-OFF: an unset ``[cadence] enabled`` flag means this function
+    returns before reading anything substantive.
+    """
+    # Local imports keep the Stop hot path free of cadence overhead
</code_context>
<issue_to_address>
**issue:** The fail-soft guarantee is enforced by the caller, not this function; consider aligning the docstring with the actual behavior.

The docstring promises fail-soft behavior (log to stderr, never raise), but this function has no internal `try`/`except`; that behavior currently lives only in the `stop` caller. If `_maybe_fire_cadence_checkpoint` is reused elsewhere, that contract may be violated. Consider either moving the `try`/`except` into this function or updating the docstring to state that callers are responsible for wrapping it in fail-soft handling.
</issue_to_address>

### Comment 2
<location path="src/aelfrice/cadence.py" line_range="170-171" />
<code_context>
+                    file=serr,
+                )
+                return CadenceConfig()
+            section_obj: Any = parsed.get(CADENCE_SECTION, {})
+            if not isinstance(section_obj, dict):
+                return CadenceConfig()
+            section = section_obj
</code_context>
<issue_to_address>
**issue (bug_risk):** Non-dict `[cadence]` sections silently fall back to defaults, which contradicts the documented error-reporting behavior.

The `load_cadence_config` docstring promises that wrong-typed values are reported to stderr, but when `[cadence]` is present and not a table (e.g. `[cadence] = 1`), this path just returns `CadenceConfig()` with no logging. That makes mis-typed sections hard to debug. Consider logging a message to stderr in the `not isinstance(section_obj, dict)` branch (mirroring `_read_bool` / `_read_k`) before returning defaults.
</issue_to_address>

### Comment 3
<location path="CHANGELOG/v3.md" line_range="13" />
<code_context>
+- **Stop-hook cadence policy — P1 every-K-turns checkpoint** ([#749](https://github.com/robotrocketscience/aelfrice/issues/749)). The Stop hook now supports a periodic-checkpoint cadence that fires the rebuilder pass every K turn boundaries, independent of when the host harness chooses to PreCompact. Originating concern: post-[#746](https://github.com/robotrocketscience/aelfrice/issues/746) PreCompact-fire-rate data showed 6 fires across 2 sessions over 6 days at `trigger_mode='threshold'` — the maintainer's typical workload `/clear`s faster than the harness compacts, so the P0 PreCompact-only policy misses most state-recovery opportunities. The [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body OR-clause was tripped by maintainer self-report and this PR ships the smallest of the body's pre-registered policies. New `src/aelfrice/cadence.py` module: `CadenceConfig` dataclass, `[cadence]` TOML section (`enabled` / `policy` / `k`), env > kwarg > TOML > default resolvers, and the pure `should_fire(fire_idx, config)` predicate (`fire_idx > 0 AND fire_idx % k == 0`, gated on `enabled AND policy == "p1_every_k_turns"`). The resolver shape leaves a slot between env and kwarg for a future [#480](https://github.com/robotrocketscience/aelfrice/issues/480) `meta:hook.cadence_k` meta-belief read without changing call sites. Stop-hook wiring: a new `_maybe_fire_cadence_checkpoint(payload, session_id, serr)` helper reads the current session's `next_fire_idx` from `session_ring.read_ring_state` (same monotonic counter UPS already maintains), checks `should_fire`, and on True calls `_rebuild_and_format` with `rebuild_log_enabled=True` — discarding the returned body since Stop has no `additionalContext` channel. The rebuild_log entry and touch-state refresh are the side effects that carry value. Stderr observability line: `aelfrice: cadence checkpoint fired @ fire_idx=N (policy=p1_every_k_turns, k=15)`. Restructured `stop()` so a session with zero lock-prompt candidates no longer early-returns before cadence gets a chance to run; the lock-prompt behaviour itself is unchanged (41 existing `test_hook_stop_lock_prompt` + `test_setup_stop_hook` tests pass). Default-OFF (`enabled = false` is the default; an unset `[cadence]` section is byte-identical to a pre-#749 Stop hook). K defaults to 15 per the [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body's literature-typical starting point. Determinism contract ([#605](https://github.com/robotrocketscience/aelfrice/issues/605)): the firing predicate is a pure function of `(fire_idx, k)` — no wall-clock, no random sampling. A replay with the same UPS sequence and same config produces the same fire decisions. 30 unit tests in `tests/test_cadence.py` cover the predicate cross-product (enabled / policy / k including k=0 / k=1 / negative-k / fire_idx=0 cold-start guard / negative fire_idx), TOML parser fail-soft on missing-file / missing-section / malformed-TOML / wrong-typed-enabled / unknown-policy / non-positive-k / bool-as-k / parent-dir walk, and the env > kwarg > TOML > default resolver precedence including env-unparseable falls-through. 11 integration tests in `tests/test_hook_stop_cadence.py` against the actual `hook.stop()` entry point cover default-OFF, policy-off-with-enabled-true, the multiples-of-K firing pattern (15/30/45 fire; 1/14/16/29 skip), cold-start fire_idx=0 suppression, missing/session-mismatched ring sentinel, empty recent-turns slice, env-override beats TOML, the stderr observability line carries policy/k/fire_idx, the regression that zero-lock-candidate sessions still fire cadence, and the lock-prompt + cadence coexistence path. P2 (token-budget watermark) and P3 (turn-density-aware) per the [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body remain unimplemented; the policy enum is forward-compatible so those land as new values without rewriting the resolver or predicate.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing “every K turn boundaries” for grammatical correctness.

"fires the rebuilder pass every K turn boundaries" is ungrammatical. Consider phrasing it as "fires the rebuilder pass at every K-turn boundary" or "fires the rebuilder pass every K turns" to improve readability while preserving the meaning.

```suggestion
- **Stop-hook cadence policy — P1 every-K-turns checkpoint** ([#749](https://github.com/robotrocketscience/aelfrice/issues/749)). The Stop hook now supports a periodic-checkpoint cadence that fires the rebuilder pass every K turns, independent of when the host harness chooses to PreCompact. Originating concern: post-[#746](https://github.com/robotrocketscience/aelfrice/issues/746) PreCompact-fire-rate data showed 6 fires across 2 sessions over 6 days at `trigger_mode='threshold'` — the maintainer's typical workload `/clear`s faster than the harness compacts, so the P0 PreCompact-only policy misses most state-recovery opportunities. The [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body OR-clause was tripped by maintainer self-report and this PR ships the smallest of the body's pre-registered policies. New `src/aelfrice/cadence.py` module: `CadenceConfig` dataclass, `[cadence]` TOML section (`enabled` / `policy` / `k`), env > kwarg > TOML > default resolvers, and the pure `should_fire(fire_idx, config)` predicate (`fire_idx > 0 AND fire_idx % k == 0`, gated on `enabled AND policy == "p1_every_k_turns"`). The resolver shape leaves a slot between env and kwarg for a future [#480](https://github.com/robotrocketscience/aelfrice/issues/480) `meta:hook.cadence_k` meta-belief read without changing call sites. Stop-hook wiring: a new `_maybe_fire_cadence_checkpoint(payload, session_id, serr)` helper reads the current session's `next_fire_idx` from `session_ring.read_ring_state` (same monotonic counter UPS already maintains), checks `should_fire`, and on True calls `_rebuild_and_format` with `rebuild_log_enabled=True` — discarding the returned body since Stop has no `additionalContext` channel. The rebuild_log entry and touch-state refresh are the side effects that carry value. Stderr observability line: `aelfrice: cadence checkpoint fired @ fire_idx=N (policy=p1_every_k_turns, k=15)`. Restructured `stop()` so a session with zero lock-prompt candidates no longer early-returns before cadence gets a chance to run; the lock-prompt behaviour itself is unchanged (41 existing `test_hook_stop_lock_prompt` + `test_setup_stop_hook` tests pass). Default-OFF (`enabled = false` is the default; an unset `[cadence]` section is byte-identical to a pre-#749 Stop hook). K defaults to 15 per the [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body's literature-typical starting point. Determinism contract ([#605](https://github.com/robotrocketscience/aelfrice/issues/605)): the firing predicate is a pure function of `(fire_idx, k)` — no wall-clock, no random sampling. A replay with the same UPS sequence and same config produces the same fire decisions. 30 unit tests in `tests/test_cadence.py` cover the predicate cross-product (enabled / policy / k including k=0 / k=1 / negative-k / fire_idx=0 cold-start guard / negative fire_idx), TOML parser fail-soft on missing-file / missing-section / malformed-TOML / wrong-typed-enabled / unknown-policy / non-positive-k / bool-as-k / parent-dir walk, and the env > kwarg > TOML > default resolver precedence including env-unparseable falls-through. 11 integration tests in `tests/test_hook_stop_cadence.py` against the actual `hook.stop()` entry point cover default-OFF, policy-off-with-enabled-true, the multiples-of-K firing pattern (15/30/45 fire; 1/14/16/29 skip), cold-start fire_idx=0 suppression, missing/session-mismatched ring sentinel, empty recent-turns slice, env-override beats TOML, the stderr observability line carries policy/k/fire_idx, the regression that zero-lock-candidate sessions still fire cadence, and the lock-prompt + cadence coexistence path. P2 (token-budget watermark) and P3 (turn-density-aware) per the [#749](https://github.com/robotrocketscience/aelfrice/issues/749) body remain unimplemented; the policy enum is forward-compatible so those land as new values without rewriting the resolver or predicate.
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_cadence.py (1)

273-341: ⚡ Quick win

Add explicit-kwarg precedence coverage for resolve_cadence_k and resolve_cadence_policy.

The resolver contract is env > kwarg > TOML > default, but the kwarg tier is only asserted for resolve_cadence_enabled. Please add parity tests for k and policy to lock this contract against regressions.

Proposed tests

+def test_resolve_k_kwarg_wins_over_toml(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    _write_toml(tmp_path, """
+        [cadence]
+        k = 7
+    """)
+    monkeypatch.delenv(cadence.ENV_CADENCE_K, raising=False)
+    assert cadence.resolve_cadence_k(explicit=30, start=tmp_path) == 30
+
+
+def test_resolve_policy_kwarg_wins_over_toml(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    _write_toml(tmp_path, """
+        [cadence]
+        policy = "off"
+    """)
+    monkeypatch.delenv(cadence.ENV_CADENCE_POLICY, raising=False)
+    assert cadence.resolve_cadence_policy(
+        explicit=cadence.POLICY_P1_EVERY_K_TURNS,
+        start=tmp_path,
+    ) == cadence.POLICY_P1_EVERY_K_TURNS

🤖 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 `@tests/test_cadence.py` around lines 273 - 341, Add tests that verify the
resolver precedence rule env > kwarg > TOML > default for resolve_cadence_k and
resolve_cadence_policy: create TOML with a differing cadence.k and
cadence.policy via _write_toml, ensure the env var (cadence.ENV_CADENCE_K /
cadence.ENV_CADENCE_POLICY) is deleted with monkeypatch.delenv, then call
cadence.resolve_cadence_k(start=tmp_path, k=<value>) and assert it returns the
passed kwarg (not the TOML value or DEFAULT_K), and likewise call
cadence.resolve_cadence_policy(start=tmp_path, policy="<policy>") and assert it
returns the passed kwarg (e.g., cadence.POLICY_P1_EVERY_K_TURNS rather than TOML
or POLICY_OFF); include both a numeric k and a policy-string case to lock the
kwarg tier.

🤖 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 `@CHANGELOG/v3.md`:
- Line 13: Update the changelog text to accurately reflect the implementation:
change the phrase that says the cadence helper calls _rebuild_and_format with
rebuild_log_enabled=True to state that it calls _rebuild_and_format with
rebuilder_cfg.rebuild_log_enabled (i.e., the flag is taken from the rebuilder
configuration), referencing the helper _maybe_fire_cadence_checkpoint and the
_rebuild_and_format call so readers know it's config-driven rather than
hardcoded.

---

Nitpick comments:
In `@tests/test_cadence.py`:
- Around line 273-341: Add tests that verify the resolver precedence rule env >
kwarg > TOML > default for resolve_cadence_k and resolve_cadence_policy: create
TOML with a differing cadence.k and cadence.policy via _write_toml, ensure the
env var (cadence.ENV_CADENCE_K / cadence.ENV_CADENCE_POLICY) is deleted with
monkeypatch.delenv, then call cadence.resolve_cadence_k(start=tmp_path,
k=<value>) and assert it returns the passed kwarg (not the TOML value or
DEFAULT_K), and likewise call cadence.resolve_cadence_policy(start=tmp_path,
policy="<policy>") and assert it returns the passed kwarg (e.g.,
cadence.POLICY_P1_EVERY_K_TURNS rather than TOML or POLICY_OFF); include both a
numeric k and a policy-string case to lock the kwarg tier.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b5e2bdac-2e81-4d45-9f7e-e27913de9bb2

📥 Commits

Reviewing files that changed from the base of the PR and between 934559d and 9fa6fb7.

📒 Files selected for processing (5)

• CHANGELOG/v3.md
• src/aelfrice/cadence.py
• src/aelfrice/hook.py
• tests/test_cadence.py
• tests/test_hook_stop_cadence.py

[robotrocketscience]

[claim:review:Kulili:2026-05-19T19:31:15Z]

[robotrocketscience]

Review — Kulili

Reviewed: cadence module, Stop-hook wiring, both test files, CHANGELOG entry, all 5 commits.

CI green (pytest 3.12 + 3.13, secrets-scan, pattern-scan, history-scan, deptry, vulture, CodeQL, bench-smoke). Discretion grep on the full diff is clean. Atomic commits map to the 5 categories the PR body lists. Design ratifications (default-OFF, env > kwarg > TOML > default, pure should_fire(fire_idx, config), forward-compatible policy enum) all hold up in the code. Stop-hook restructure preserves the lock-prompt path (41 pre-existing tests pass per the body, and the diff confirms it — the early-returns are replaced with if store is not None: try/finally store.close()).

Defects — please address before merge

1. CHANGELOG factual error. Line 13 says cadence calls _rebuild_and_format "with rebuild_log_enabled=True". The actual call site (hook.py:2654 on the branch) passes rebuild_log_enabled=rebuilder_cfg.rebuild_log_enabled — config-driven, not hardcoded. The PR body has the same wording; fix both. coderabbitai flagged this on line 13.

2. Unused imports in tests/test_hook_stop_cadence.py:14-15. POLICY_OFF and POLICY_P1_EVERY_K_TURNS are imported but never referenced in the file (grep -n POLICY tests/test_hook_stop_cadence.py confirms — only the import line matches). github-advanced-security[bot] / CodeQL flagged it as a security/code-scanning alert. CodeQL CI passed, so this isn't blocking gate, but the alert lingers in the dashboard. Drop both names from the import.

Nice-to-have (non-blocking)

3. CHANGELOG/v3.md:13 grammar — "every K turn boundaries" reads ungrammatically. Either "every K turns" or "every K-turn boundary" (sourcery's suggestion). The full diff has the same phrase in the prose paragraph, so it's worth one Find+Replace.

4. src/aelfrice/cadence.py:171 silent fallback inconsistency. Every other branch in load_cadence_config logs to stderr on wrong type / malformed input (_read_bool, _read_policy, _read_k, TOMLDecodeError, OSError). The not isinstance(section_obj, dict) branch — which fires when [cadence] = 1 or similar non-table values — silently returns CadenceConfig() with no logging. Mismatched with the docstring's promise that "wrong-typed values trace to stderr". One print(..., file=serr) line before the return matches the rest of the file's style.

5. src/aelfrice/hook.py:2589-2607 docstring. Says "Fail-soft: any error short-circuits to a stderr line; never raises", but the try/except actually lives in the stop() caller (hook.py:2575-2581). If _maybe_fire_cadence_checkpoint is ever called from somewhere other than stop(), the contract is silently violated. Either move the try/except into _maybe_fire_cadence_checkpoint, or amend the docstring to "callers must wrap this in fail-soft handling; the Stop hook's stop() does so."

6. Triple resolver call in _maybe_fire_cadence_checkpoint. resolve_cadence_enabled → resolve_cadence_policy → resolve_cadence_k, each walks parent dirs for .aelfrice.toml and parses it on a cache miss. Default-OFF means the first call short-circuits at the env tier, so the worst-case path is only hit when cadence is enabled — but on every enabled Stop turn, that's three filesystem walks + three TOML parses. Worth either (a) reading once via load_cadence_config(start=cwd) and using the three values directly, or (b) a single resolve_cadence_config() umbrella that does one walk and applies the three precedence chains in-process. Not blocking — the perf surface here is hot-ish but not critical-path — but worth a follow-up commit on this PR if cheap.

Approval

Conditional on (1) and (2). The CHANGELOG line is documentation of the public surface and is wrong; the unused imports leave a CodeQL alert. Both are 5-minute fixes. (3)–(6) are reviewer preferences; ship them or punt as you prefer.

Once (1) + (2) land, this is ready to merge via the ready-to-merge label (gh pr edit 869 --add-label ready-to-merge).

[robotrocketscience]

[release:review:Kulili:2026-05-19T19:33:51Z]

[robotrocketscience]

Reversal — Kulili

On re-read of my prior comment (issuecomment-4491335541): I over-gated. CI is green, the feature works, behavior is correct, discretion is clean. The two items I tagged "must-fix" don't justify holding the merge:

• The CHANGELOG rebuild_log_enabled=True wording is wrong but fixable in a follow-up doc PR.
• The unused POLICY_OFF / POLICY_P1_EVERY_K_TURNS imports are a CodeQL alert, not a CI failure — CodeQL passed.

Treating the prior comment's items (1)–(6) as suggestions, not blockers. Adding ready-to-merge.

[github-actions]

merge-train: blocked

4 review thread(s) are unresolved on these files: CHANGELOG/v3.md, src/aelfrice/cadence.py, src/aelfrice/hook.py, tests/test_hook_stop_cadence.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label.

The ready-to-merge label has been removed. Address the issue above and re-add the label when you're ready for another attempt.

[github-actions]

merge-train: merged 9fa6fb7 → main via FF push.