[Birfy]

Closes #282

Problem

AdaptiveTrustRegion.observe() computed each next cap as round(cap * factor) clamped to [minimum, maximum]:

• Op cap ratchet. With the default widen=1.25, round(2 * 1.25) == round(2.5) == 2 (banker's rounding) and round(1 * 1.25) == 1. Two reversals from the default 6 (6 → 3 → 2) left the op cap at 2 for the rest of the run. Every later clean window still grew chars and appended to history, so the region looked adaptive while its binding cap was frozen. Repro on main: window=1, two oracle-rejected, ten committed → TrustRegion(ops=2, chars=74508).
• A tighten could widen. For a cap below minimum (e.g. initial=TrustRegion(3, 500) as in the OVERSIZED merge report quotes the config constants, not the adaptive trust region that actually rejected the diff #224/AdaptiveTrustRegion ignores OVERSIZED outcomes, so a binding size cap never widens #226 regressions), a reversal computed max(2000, 250) == 2000 and quadrupled the char cap on a rollback.
• Bad knobs failed late. window=0 raised ZeroDivisionError on the first observe(); widen < 1, tighten > 1, NaN/inf factors, etc. were accepted and silently inverted the rule.

Fix

• New AdaptiveTrustRegion._step(). A widen moves a cap up by at least one unit (max(cur + 1, round(cur * factor))), then clamps to [minimum, maximum]. A tighten moves it down by at least one unit, clamps, and never ends above cur. maximum remains a hard ceiling on every move (and wins where minimum > maximum, which test_it_stops_at_its_bounds relies on). Factors of exactly 1.0 are neutral.
• __init__ validation (ValueError): window int ≥ 1; widen finite ≥ 1; tighten finite in (0, 1]; accept_rate_to_widen finite in [0, 1]; every bound an int ≥ 1. I did not enforce minimum <= maximum (the issue proposed it), because passing only a small maximum below the default minimum.chars is pinned by an existing test as a supported way to cap a run. initial stays unconstrained.
• Class docstring documents the rule; CHANGELOG Fixed entry. docs/api.md regenerated: no change (it carries only the summary line).

Tests

tests/test_advantage.py: 9 new tests / 26 cases, covering widening out of 1- and 2-op caps, a full 6→3→2→3→4→5→6 recovery, one-unit tightening (round(3*0.9)==3), no loosening below minimum, maximum still capping a slow widen and a tighten, invalid knobs, and neutral factors. 21 of the new cases fail on main and all pass on the branch. Every existing trust-region expectation, including #224/#226 in tests/test_aggregator.py and tests/test_ab_run.py, is unchanged: test_advantage + test_aggregator + test_ab_run all pass; full suite result below.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-15T12:59:34.426537Z	97f7042	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Birfy]

Review: LGTM ✅

Thanks, this is a clean, well-scoped fix for #282. I checked out bdeedd0 and probed it locally.

Correctness

• _step() fixes the ratchet: a widen is now max(cur + 1, round(cur * factor)), so 1 and 2 ops grow again, and the full 6 → 3 → 2 → 3 → 4 → 5 → 6 recovery holds. A tighten is min(cur - 1, round(cur * factor)), so round(3 * 0.9) == 3 no longer counts as a move.
• The min(cur, hi, max(lo, target)) form in the tighten branch is right. A cap below minimum stays where it is, so TrustRegion(3, 500) is no longer quadrupled to 2000 chars by a rollback. maximum still wins when minimum > maximum, which keeps test_it_stops_at_its_bounds and the "small maximum caps a run" usage working. Not enforcing minimum <= maximum was a reasonable call, and the inline comment explains it.
• Neutral factors (1.0) return cur, and nothing is added to history. Good.
• The constructor now rejects window=0 (which used to raise ZeroDivisionError), inverted or non-finite factors, an out-of-range accept_rate_to_widen, and non-int or non-positive bounds, all with ValueError and with bools excluded. initial stays unconstrained, as the issue asked.

Tests

• tests/test_advantage.py: 21 of the new cases fail on main (21 failed / 36 passed) and all pass on the branch. test_advantage + test_aggregator + test_ab_run give 91 passed, and the #224/#226 expectations are unchanged.
• CI: build and tests on 3.10–3.13 pass. 3.9 was still pending when I looked.

Docs: the class docstring and the CHANGELOG Fixed entry are accurate.

Non-blocking nits

1. When initial is above maximum, a widen now narrows the region. For example, TrustRegion(100, 600_000) becomes TrustRegion(64, 512_000) on a clean window. That's the conservative choice, and the code comment says so, but it differs from the issue's "a widen never narrows" and no test pins it. A one-line test for whichever behavior you want would lock it in.
2. The behavior below minimum is asymmetric. A tighten holds a sub-minimum cap, but a widen jumps it straight to minimum (chars 500 → 2000 in one step at widen=1.25). This is pre-existing behavior, but it might deserve half a sentence in the "Every move is a move" paragraph.
3. widen=True / tighten=True still pass validation, since a bool counts as 1, while window explicitly refuses bools. A non-numeric factor raises TypeError from math.isfinite rather than ValueError. Both are trivial.
4. Pre-existing: when a tighten leaves both caps unchanged (both at their floor), _recent isn't cleared, so the same reversal is re-counted on each following observe until it leaves the window. That's harmless, so I'm only noting it.

Good to merge once CI is fully green.

[Birfy]

Auto-review: LGTM (COMMENT, since self-approve is blocked)

Closes #282. Checked on bdeedd0 against main @ 3947aee.

Correctness

• Op-cap ratchet. On main, window=1 with two reversals and ten clean windows ends at TrustRegion(ops=2, chars=74508), so ops never move. _step with factor > 1 uses max(cur + 1, round(cur * factor)), giving 6 → 3 → 2 → 3 → 4 → 5 → 6. That is pinned exactly by test_two_reversals_then_clean_windows_recover_the_default_cap.
• A tighten never loosens. min(cur, hi, max(lo, target)): a cap below minimum stays put on a reversal instead of being clamped up. On main, initial=(3, 500) plus one oracle-rejected gave chars 2000. The ops side still steps down to the floor.
• maximum stays a hard ceiling on both widen and tighten, and wins over minimum where they disagree, which keeps test_it_stops_at_its_bounds (max chars 1000 < default min 2000) passing unchanged. A factor of exactly 1.0 is neutral and appends nothing to history.
• Validation at construction. window=0 (was ZeroDivisionError on the first observe), float or bool window, widen < 1, tighten ∉ (0, 1], NaN/inf factors, accept_rate_to_widen ∉ [0, 1], and non-int or < 1 bounds now raise. Not enforcing minimum <= maximum is the right call given the existing pinned usage, and the PR body explains the deviation from the issue.

Tests

• 9 new tests / 26 cases in tests/test_advantage.py. 21 cases fail on main and all pass on the branch. window=-1 already raised via deque(maxlen=-1), so it is a guard rather than a regression.
• test_advantage + test_aggregator + test_ab_run pass, including the #224/#226 adaptive-region regressions with initial below minimum. Full local suite: exit 0.
• Exact-head CI: build + 3.9–3.13 green. The Codex review on 97f7042 finished with no findings, and bdeedd0 differs from it only by a blank line.

Docs

• Class docstring states the "every move is a move" rule and the asymmetry. The CHANGELOG Fixed entry is accurate. docs/api.md regenerated with no diff (it carries only the summary line).

Nits (non-blocking)

• When ops sit at maximum and only chars move, history still records a new value, which is correct but means _fired_counts("trust-region") can count a window that changed only chars. That is pre-existing behaviour, unchanged here.
• GroupAdvantage.observe has a related numerical edge: one NaN or inf reward silently pins a group's advantage to None for the rest of that base version. That is a separate issue, not this PR.

[Birfy]

Re-checked after merging main (924c25c, includes #272) at 7c28530. The only conflict was CHANGELOG.md (### Fixed): both entries are kept, the #282 AdaptiveTrustRegion entry above the #271 runstore entry, and the spacing is intact. No code conflicts. tests/test_advantage.py passes locally on the merge commit, and CI is fully green (build + 3.9–3.13). No unresolved review threads. LGTM, still merge-ready.

[Birfy]

Merged main (751b8dd) into this branch — CHANGELOG-only conflict resolved (kept #282 + #286 Fixed entries). Head 6087fed. tests/test_advantage.py 57 passed locally.

LGTM once CI is green.