feat(cadence): p3_substantive dispatch + shadow capture + 4-policy scorer + offline replay (#876) - #920
Conversation
📝 WalkthroughWalkthroughExtends the cadence policy framework to support P3 substantive-turn density detection. Stop-side implements per-turn classification, stores rolling history in session state, and fires when substantive-window threshold is met. UPS-side reads the same history without mutation and injects rebuilds. Cadence-score now reports pairwise agreement across all four policies, and a new offline replay benchmark enables shadow evaluation of fixture-based scenarios. ChangesP3 Substantive Cadence Policy Implementation and Analytics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Reviewer's GuideImplements live dispatch of the p3_substantive cadence policy in both Stop and UPS hooks, extends the shadow cadence logger and scoring CLI to handle all four cadence policies with pairwise comparisons, and adds an offline deterministic replay benchmark plus tests and changelog updates to support P3 cadence evaluation for #876. Sequence diagram for live p3_substantive cadence dispatch in Stop and UPS hookssequenceDiagram
participant StopHook as Stop_hook_maybe_fire_cadence_checkpoint
participant UPSHook as UPS_hook_maybe_run_ups_cadence_checkpoint
participant SessionRing as session_ring
participant Cadence as cadence_predicates
participant Rebuilder as context_rebuilder
StopHook->>Cadence: resolve_cadence_p3_substantive_window
StopHook->>Cadence: resolve_cadence_p3_substantive_threshold
StopHook->>Cadence: is_substantive_turn
StopHook->>SessionRing: push_classification
StopHook->>SessionRing: read_ring_state
StopHook->>Cadence: should_fire_p3_substantive
alt [should_fire_p3_substantive]
StopHook->>Rebuilder: _run_cadence_rebuild
Rebuilder-->>StopHook: body
StopHook->>Rebuilder: _write_cadence_resume_cache
end
UPSHook->>Cadence: resolve_cadence_p3_substantive_window
UPSHook->>Cadence: resolve_cadence_p3_substantive_threshold
UPSHook->>SessionRing: read_ring_state
UPSHook->>Cadence: should_fire_p3_substantive
alt [should_fire_p3_substantive]
UPSHook->>Rebuilder: _run_cadence_rebuild
Rebuilder-->>UPSHook: body
end
Flow diagram for offline cadence replay benchmark and scoringflowchart TD
A[CLI main] --> B[parse_args]
B --> C[read fixture JSON]
C --> D[_config_from_fixture]
D --> E[replay_fixture]
E --> F[for each tick]
F --> G[_write_transcript]
G --> H[estimate_transcript_bytes]
H --> I[would_fire_p1/p2]
H --> J[would_fire_p3_velocity]
F --> K[would_fire_p3_substantive]
I --> L[build shadow row]
J --> L
K --> L
L --> M[rows list]
M --> N[compute_summary]
N --> O[format_report]
O --> P[stdout]
O --> Q{args.output?}
Q -->|yes| R[write JSON summary]
Q -->|no| P
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The computation of the P3 substantive ratio (reading
classifications, slicing by window, summingTrues) is duplicated in the Stop dispatcher, UPS dispatcher, shadow logger, and replay bench; consider factoring this into a small shared helper to keep the window semantics aligned if they ever change. - Similarly, the P3 velocity input derivation (bytes-at-last-fire, fire-idx-at-last-fire, transcript-bytes, turns-since) is reimplemented in both the shadow logger and replay bench; centralising that logic behind a helper would reduce the risk of the replay diverging subtly from live behaviour.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The computation of the P3 substantive ratio (reading `classifications`, slicing by window, summing `True`s) is duplicated in the Stop dispatcher, UPS dispatcher, shadow logger, and replay bench; consider factoring this into a small shared helper to keep the window semantics aligned if they ever change.
- Similarly, the P3 velocity input derivation (bytes-at-last-fire, fire-idx-at-last-fire, transcript-bytes, turns-since) is reimplemented in both the shadow logger and replay bench; centralising that logic behind a helper would reduce the risk of the replay diverging subtly from live behaviour.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="3284-3293" />
<code_context>
+ if policy == POLICY_P3_SUBSTANTIVE:
</code_context>
<issue_to_address>
**suggestion:** Guard or normalise p3_substantive_window to avoid zero/negative-window edge cases and keep semantics consistent.
Across Stop/UPS/shadow/bench we use `p3_substantive_window` directly in `classifications[-window:]`. If a resolver/fixture ever passes `0` or a negative value, the slice semantics become surprising (`[-0:]` is the full list and negatives expand the slice). Consider normalising the value once (e.g. `max(window, 0)` or early-return on non-positive) and centralising this in a helper to keep P3-substantive behaviour consistent across call sites.
Suggested implementation:
```python
from aelfrice.session_ring import ( # noqa: PLC0415
push_classification,
read_ring_state,
update_p3_velocity_state,
)
def _normalise_p3_substantive_window(window: int) -> int:
"""Clamp the P3 substantive window to zero or greater.
This keeps downstream slicing semantics (`classifications[-window:]`)
predictable across call sites.
"""
return max(window, 0)
)
return
if policy == POLICY_P3_SUBSTANTIVE:
window = _normalise_p3_substantive_window(
resolve_cadence_p3_substantive_window(start=cwd)
)
```
To fully implement the suggestion across the codebase, any place that uses `p3_substantive_window` in `classifications[-window:]` (Stop/UPS/shadow/bench) should normalise the window in the same way before slicing, e.g. by calling a shared helper with the same `max(window, 0)` semantics or inlining that normalisation at each call site. If you prefer early-return semantics for non-positive windows instead, you should replace the `max(window, 0)` behaviour with an explicit guard (e.g. `if window <= 0: return ...`) both here and at those other sites.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/hook.py" line_range="3678" />
<code_context>
+ if isinstance(raw_fire_last, int) and not isinstance(raw_fire_last, bool)
+ else 0
+ )
+ transcript_bytes = estimate_transcript_bytes(tp)
+ turns_since_last_fire = fire_idx - fire_idx_at_last_fire
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid passing a possibly-None transcript path into estimate_transcript_bytes.
In the shadow logger, `tp` can be `None` when the transcript path is missing or malformed, but it’s still passed to `estimate_transcript_bytes`, which likely expects a `Path`/PathLike. Add a small guard here (e.g., treat `None` as zero bytes) so shadow logging doesn’t fail on incomplete payloads, while keeping live cadence behavior unchanged.
</issue_to_address>
### Comment 3
<location path="benchmarks/cadence_replay.py" line_range="73-82" />
<code_context>
+def _config_from_fixture(raw: dict[str, Any]) -> CadenceConfig:
</code_context>
<issue_to_address>
**suggestion:** Fixture-driven ints/floats are cast without validation, which can make the bench brittle on malformed input.
In `_config_from_fixture`, numeric fields are cast via `int(...)` / `float(...)` without validation, so a bad fixture value (e.g. `"k": "ten"`) will raise and stop the replay. Consider catching `ValueError` per field and either falling back to the default or raising a more targeted error that clearly identifies the offending key/value.
Suggested implementation:
```python
would_fire_p3_velocity,
)
from typing import Any, Callable, Mapping, TypeVar
from aelfrice.cadence_score import compute_summary, format_report
TNum = TypeVar("TNum", int, float)
def _coerce_numeric_field(
key: str,
raw_cfg: Mapping[str, Any],
coerce: Callable[[Any], TNum],
default: TNum,
) -> TNum:
"""Return a validated numeric config value for the given key.
The function:
* Returns ``default`` when the key is absent from ``raw_cfg``.
* Raises a ValueError with a clear message (including the key and value)
when the provided value cannot be coerced to the requested numeric type.
"""
if key not in raw_cfg:
return default
value = raw_cfg[key]
try:
return coerce(value)
except (TypeError, ValueError) as exc:
raise ValueError(
f"Invalid value for cadence config key {key!r}: {value!r} "
f"(expected a value coercible to {coerce.__name__})"
) from exc
```
To fully implement the comment in `_config_from_fixture`, update each place where numeric fields are currently coerced with `int(...)` / `float(...)` on raw fixture values. For example, change patterns like:
```python
window_days = int(cfg.get("window_days", defaults.window_days))
```
to:
```python
window_days = _coerce_numeric_field(
"window_days",
cfg,
int,
defaults.window_days,
)
```
and for floats:
```python
some_threshold = _coerce_numeric_field(
"some_threshold",
cfg,
float,
defaults.some_threshold,
)
```
This ensures malformed numeric values in fixtures raise a targeted `ValueError` that identifies the offending key and value, instead of a generic `ValueError` at the cast site. Apply this replacement to all numeric config fields inside `_config_from_fixture`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:garsecg:2026-05-26T18:40:53Z] |
The p3_substantive predicate (axis-1 option C) shipped in PR 1/5 but was never wired into the live dispatchers — only p3_velocity was (PR 3/5). This adds the POLICY_P3_SUBSTANTIVE branch to both _maybe_fire_cadence_ checkpoint (Stop) and _maybe_run_ups_cadence_checkpoint (UPS). Stop owns the per-turn classification push: each tick the policy is active it classifies the last user prompt via is_substantive_turn and push_classification's it onto the session-ring rolling window, then reads the last p3_substantive_window entries and fires when the substantive ratio meets p3_substantive_threshold. UPS reads the window read-only (no push) to keep the window advancing exactly once per turn — a one-turn read lag matching the established p3_velocity counter- sharing semantics. Deterministic (#605), no embeddings. Without this, selecting policy=p3_substantive was a silent no-op (the classifications slot was never populated, so the predicate could never fire).
The shadow-evaluation logger (#875) evaluated only would_fire_p1 and would_fire_p2, so the cadence_shadow JSONL — the bake data source for aelf cadence-score — carried no P3 decisions. With p3_velocity and p3_substantive now live policies, the head-to-head comparison the #876 axis-3 bake needs was impossible to collect. _maybe_log_cadence_shadow_tick now resolves the P3 knobs, derives the p3_velocity inputs (byte delta / turns since last fire) and the p3_substantive input (substantive ratio over the rolling window) from the same ring state, evaluates both would_fire_p3_* predicates, and records all four policies in the shadow row. Selected-policy fired derivation extended to the two P3 branches. All inputs read fail-soft with the same defaults the live dispatch uses.
per_policy_fire_count/total already spanned every policy present in the shadow log, so once the logger captures P3 (prior commit) the per-policy rates cover all four. The only P1/P2-specific surface was the 2x2 agreement matrix. Add a generalised pairwise_agreement table to ShadowSummary: for every unordered policy pair logged on a row, count both / a_only / b_only / neither (a < b lexicographically). format_report renders a 'pairwise policy agreement' section with a per-pair divergence rate, and the --json payload gains a pairwise_agreement object. The legacy 2x2 P1-vs-P2 agreement_matrix is retained unchanged for backward compatibility, so existing consumers and tests are unaffected.
Completes the #876 bench gate: a deterministic offline replay that produces the four-policy comparison aelf cadence-score emits from a live shadow bake, without an operator-week of real data. benchmarks/cadence_replay.py reads a self-contained synthetic fixture (per-tick fire_idx / byte counts / prompt / classification window + the cadence config), feeds each tick through would_fire_p1 / p2 / p3_velocity / p3_substantive, and emits shadow-log-shaped rows that compute_summary aggregates exactly as it would a live bake — per-policy fire rates + pairwise divergence. P2 reads transcript size from a temp file and p3_velocity takes the int; the harness writes one temp transcript per tick and uses its actual size for both so the two predicates see a consistent figure. Deterministic (#605): same fixture -> byte-identical rows. Discretion: synthetic fixture only, no live state, no ~/.claude content. Sample fixture at benchmarks/fixtures/cadence_replay_sample.json.
a0b1581 to
51cea5c
Compare
|
merge-train: blocked 2 review thread(s) are unresolved on these files: benchmarks/cadence_replay.py, src/aelfrice/hook.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
|
merge-train: blocked 2 review thread(s) are unresolved on these files: benchmarks/cadence_replay.py, src/aelfrice/hook.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/aelfrice/cadence_score.py (1)
144-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly update legacy P1/P2 agreement when both decisions are present and valid.
Line 161 currently increments the legacy matrix even when P1/P2 entries are missing or malformed, which can overcount
(False, False)and skew agreement reporting.Suggested fix
p1_fire = False p2_fire = False + p1_seen = False + p2_seen = False row_fire: dict[str, bool] = {} for policy, decision in shadow.items(): if not isinstance(decision, dict): continue @@ row_fire[policy] = would if policy == POLICY_P1_EVERY_K_TURNS: p1_fire = would + p1_seen = True elif policy == POLICY_P2_CTX_THRESHOLD: p2_fire = would - agreement[(p1_fire, p2_fire)] += 1 + p2_seen = True + if p1_seen and p2_seen: + agreement[(p1_fire, p2_fire)] += 1🤖 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 `@src/aelfrice/cadence_score.py` around lines 144 - 162, The legacy P1/P2 agreement matrix is updated unconditionally at the end of the loop, which counts rows where P1 or P2 entries are missing/malformed; modify the logic so agreement[(p1_fire, p2_fire)] += 1 only runs when both POLICY_P1_EVERY_K_TURNS and POLICY_P2_CTX_THRESHOLD were actually present and parsed as booleans: detect presence while iterating shadow (e.g., track flags or check row_fire keys for those two policies), and move or guard the increment accordingly so it only updates agreement when both valid decisions exist.src/aelfrice/hook.py (1)
3155-3161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShadow P3-substantive evaluation is one turn stale.
At Line 3155, shadow evaluation runs before the Stop-side
p3_substantivepush at Line 3310. But shadowp3_substantiveuses pre-pushclassificationsat Lines 3689-3692, sofired/would_firecan diverge from the live decision for the same tick.This skews shadow rows and downstream pairwise scoring for P3-substantive.
Proposed fix (compute P3-substantive shadow on effective current-turn window, without mutating ring)
@@ from aelfrice.cadence import ( # noqa: PLC0415 CadenceConfig, @@ estimate_transcript_bytes, format_shadow_row, + is_substantive_turn, read_last_user_prompt, @@ last_prompt = read_last_user_prompt(tp) @@ raw_classes: Any = ( state.get("classifications") if isinstance(state, dict) else None ) classifications = raw_classes if isinstance(raw_classes, list) else [] + current_turn_substantive = is_substantive_turn(last_prompt) + effective_classes = classifications + [current_turn_substantive] substantive_count = sum( - 1 for c in classifications[-p3_substantive_window:] if c is True + 1 for c in effective_classes[-p3_substantive_window:] if c is True )Also applies to: 3310-3327, 3685-3719
🤖 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 `@src/aelfrice/hook.py` around lines 3155 - 3161, Shadow P3-substantive is computed from stale classifications because _maybe_log_cadence_shadow_tick is invoked before the Stop-side p3_substantive push; fix by computing the shadow P3-substantive on the effective current-turn window without mutating the ring or by invoking the shadow logic after the Stop-side push. Concretely, update _maybe_log_cadence_shadow_tick (or the caller site around the Stop-side p3_substantive push) to either (a) accept a snapshot/copy of current classifications and compute p3_substantive from that snapshot/window so it matches the live decision, or (b) move the call to _maybe_log_cadence_shadow_tick to run after the Stop-side p3_substantive action (the block around your Stop push at the other site), ensuring you reference p3_substantive, classifications, and ring when locating code to change.
🤖 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 `@tests/test_hook_stop_cadence_p3_substantive.py`:
- Line 209: The test unpacks an unused `serr` from the tuple returned by
_run_stop (e.g., "serr, calls, state_dir = _run_stop(...)") which triggers Ruff
RUF059; update the unpacking in both occurrences (around the _run_stop calls at
the two reported sites) to drop or ignore `serr` (use "_, calls, state_dir =
_run_stop(...)" or "calls, state_dir = _run_stop(...)" depending on return
shape) so the unused variable is not created.
---
Outside diff comments:
In `@src/aelfrice/cadence_score.py`:
- Around line 144-162: The legacy P1/P2 agreement matrix is updated
unconditionally at the end of the loop, which counts rows where P1 or P2 entries
are missing/malformed; modify the logic so agreement[(p1_fire, p2_fire)] += 1
only runs when both POLICY_P1_EVERY_K_TURNS and POLICY_P2_CTX_THRESHOLD were
actually present and parsed as booleans: detect presence while iterating shadow
(e.g., track flags or check row_fire keys for those two policies), and move or
guard the increment accordingly so it only updates agreement when both valid
decisions exist.
In `@src/aelfrice/hook.py`:
- Around line 3155-3161: Shadow P3-substantive is computed from stale
classifications because _maybe_log_cadence_shadow_tick is invoked before the
Stop-side p3_substantive push; fix by computing the shadow P3-substantive on the
effective current-turn window without mutating the ring or by invoking the
shadow logic after the Stop-side push. Concretely, update
_maybe_log_cadence_shadow_tick (or the caller site around the Stop-side
p3_substantive push) to either (a) accept a snapshot/copy of current
classifications and compute p3_substantive from that snapshot/window so it
matches the live decision, or (b) move the call to
_maybe_log_cadence_shadow_tick to run after the Stop-side p3_substantive action
(the block around your Stop push at the other site), ensuring you reference
p3_substantive, classifications, and ring when locating code to change.
🪄 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: bda64743-2f2a-49df-8758-46d33a59d166
📒 Files selected for processing (10)
CHANGELOG/v3.mdbenchmarks/cadence_replay.pybenchmarks/fixtures/cadence_replay_sample.jsonsrc/aelfrice/cadence_score.pysrc/aelfrice/hook.pytests/test_cadence_replay.pytests/test_cadence_score.pytests/test_hook_stop_cadence_p3_substantive.pytests/test_hook_stop_cadence_shadow.pytests/test_hook_ups_cadence_p3_substantive.py
|
merge-train: merged 51cea5c → |
|
[release:review:garsecg:2026-05-26T18:51:04Z] |
Audit #957: feed log is feed.jsonl (not .aelfrice-feed.jsonl); #876 entry described an unshipped p3_turn_density design — rewritten around the shipped p3_velocity (live) + p3_substantive (scaffolding; dispatch landed 3.4.0 PR #920); #875 shadow entry matches the per-session log path, (would_fire, reason) predicates, 2x2 matrix, and real CLI flags; #870 UPS checkpoint injects the current turn and Stop remains the cache writer; #887 recent-work block uses regex issue refs with no gh resolution; #935 slot extractor, #933 index, #937/#941/#932 wording, #850 test filename.
Summary
Completes the #876 P3 cadence work and makes the bench gate runnable.
The prior series (#884/#885/#886, PRs 1–3 of 5) shipped the enum
constants, session-ring slots, and the
p3_velocitylive dispatch, butstopped there:
p3_substantivewas never dispatched (selecting it was asilent no-op), the shadow logger captured only P1/P2, and
aelf cadence-scorecould only report a 2×2 P1-vs-P2 matrix. So thebench-gatedcomparison the triage (axis-3 bake) depends on had no P3data and no way to produce it offline.
This PR closes that gap in four atomic commits.
Commits
feat(cadence): dispatch p3_substantive live in Stop + UPS— addsthe
POLICY_P3_SUBSTANTIVEbranch to both dispatchers. Stop owns theper-turn classification push (
is_substantive_turn→push_classificationonto the session-ring rolling window) and readsthe window to fire; UPS reads the window read-only (no push) to keep
it advancing exactly once per turn — a one-turn read lag matching the
established
p3_velocitycounter-sharing semantics.feat(cadence): capture both P3 policies in shadow log—_maybe_log_cadence_shadow_ticknow evaluateswould_fire_p3_velocityand
would_fire_p3_substantiveagainst the same tick inputs andrecords all four policies in the shadow row. This is the bake data
source.
feat(cadence-score): four-policy pairwise comparison— per-policyfire rates already spanned every logged policy; adds a generalised
pairwise_agreementtable (both / a-only / b-only / neither + adivergence rate) across every policy pair, so all four compare
head-to-head. The legacy 2×2 P1-vs-P2 matrix is retained unchanged.
feat(bench): offline cadence-policy replay harness— the"replay" half:
benchmarks/cadence_replay.pyproduces the samefour-policy comparison from a self-contained synthetic fixture, with
no operator-week of live shadow data. Each tick is fed through every
would_firepredicate and aggregated bycompute_summary.Deterministic (v3.0 PHILOSOPHY: natural-language-relatedness gate — deterministic vs embedding #605); sample fixture included.
Determinism / discretion
All predicates are pure functions (#605); the replay is byte-identical
across runs. No
~/.claude/-derived content — the fixture is syntheticinput authored alongside the bench.
Testing
test_hook_stop_cadence_p3_substantive,test_hook_ups_cadence_p3_substantive, the P3-capture test intest_hook_stop_cadence_shadow, four-policy/pairwise tests intest_cadence_score, andtest_cadence_replay(8 tests incl. CLI +determinism).
p1 ↔ p3_substantiveco-fire on the dense-substantive tick,p2 ↔ p3_velocityco-fire on the high-byte boundary tick.Closes #876.
Refs #749, #875, #884, #885, #886.
Summary by Sourcery
Wire the P3 substantive cadence policy through live dispatch, shadow logging, scoring, and benchmarking to complete four-policy cadence evaluation and offline replay.
New Features:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Tests
Documentation