Repository navigation
feat(rrsi): wire the regularized RRSI mechanism into evolve() through the Policies seams - #254
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Thanks for this. Wiring RRSI in through the Policies seams while reusing the pinned selection.judge / edit_budget is the right shape, and the new ScreenPolicy seam is small, opt-in, and placed correctly: it runs before staleness and well before the held-out sweep. The deviations section in docs/algo-rrsi.md is also appreciated. I'm requesting changes because of one CI-breaking regression and a few places where the integration doesn't yet do what the PR description says it does.
Blocking
1. Aggregator.checkpoint() regression (CI red on 3.9/3.10, and locally on 3.13).
The new hook reads self.acceptance_policy outside the existing try/except AttributeError. So an aggregator that doesn't have the attribute (the four tests/test_checkpoint.py cases build one via Aggregator.__new__, and a subclass that skips super().__init__ would hit the same thing) now raises where checkpoint() used to work:
FAILED tests/test_checkpoint.py::test_reference_aggregator_checkpoint_roundtrip - AttributeError: 'Aggregator' object has no attribute 'acceptance_policy'
FAILED tests/test_checkpoint.py::test_population_aggregator_checkpoint_roundtrip
FAILED tests/test_checkpoint.py::test_population_checkpoint_includes_parent_state
FAILED tests/test_checkpoint.py::test_population_restore_recovers_parent_state
main passes all of tests/test_checkpoint.py. Use getattr(self, "acceptance_policy", None) in both checkpoint() and restore(). The "171 tests green" in the description must have left out test_checkpoint.py, so please run the full suite.
2. The annealed round counter only advances on accepted merges.
RRsiAcceptance.accept does state.round += 1 and state.progress.append(...) only on the admissible branch. Critic rejections and below-floor or cost-rule rejections never advance the schedule. That contradicts the docstring ("advanced once per merge") and the deviations note, and it switches off two of the six regularizers in exactly the situation they exist for:
st = RRsiState(RRsiConfig(b_min=1, b_max=5, T=4, stall_window=3), base_score=0.5)
acc = RRsiAcceptance(st)
for _ in range(10):
acc.accept(_context(st, base=(5, 5), cand=(3, 7))) # all rejected
# -> round 0, budget 5, stalled False, ledger 10After ten straight rejections the budget is still b_max, so update sparsity never anneals. stalled() is still False, because progress only grows on accepts, so stall exploration can never fire while the search is stuck. Please advance round and append S* to progress on every merge decision, including the screen-rejected path (perhaps through a small hook the aggregator calls once per merge). Then add a test that a streak of rejections anneals the budget and trips the stall note.
3. The cost rule compares batch-summed proposer-rollout tokens.
_card_cost sums cost_tokens across ctx.cards, and incumbent_cost is set to that sum on accept. This has two problems:
- It depends on batch size. Three cards at an identical 100 tokens per rollout, against an incumbent of 100, give
dC = +2.0, and a +0.3 score gain is rejected withcost-rule / large-gain cost rule failed(confirmed locally with the default config). Whether a candidate passes then depends on how many cards happened to land in the bucket, not on the harness. - It measures the wrong harness.
EvidenceCard.cost_tokensis the usage of the proposing rollout, which ran the incumbent. Algorithm 2'sdCis the candidate harness's policy-token cost relative to the incumbent's, measured on the same evaluation. As written, "candidate cost" is really "the incumbent's cost on the tasks that triggered this proposal".
At a minimum, use a per-rollout mean. Ideally take the cost from the held-out measurement of both sides. If that isn't available yet, I'd rather pass cost=None (so dC = 0, the documented missing-telemetry behaviour) and record it as a deviation than run a cost rule on a proxy that is systematically wrong.
Should fix
-
Novelty is effectively always 0, and its counts mean something different.
default_componentsreturns the first path segment ("skills","value", ...), butSTRUCTURAL_COMPONENTSis{"client_tool", "skill", "memory", "subagent"}, so a FileTreeskills/...edit never counts as novel. Also,judge(..., incumbent_counts=dict(state.attempts))passes "how often a component was proposed", while upstream'sincumbent_countsis the component inventory of the incumbent harness. Withw_n > 0, the within-band novelty branch is therefore unreachable on real layouts. Please map onto the pinned vocabulary and pass incumbent counts, or declare it as a deviation. Note that #253 is porting the upstream component vocabularyKand the per-edit historyL_t(examples/rrsi/components.py/history.py). This PR'sEditRecordledger anddefault_componentsduplicate that with different semantics, so it would be good to build on #253 (or agree on the order) rather than land two histories. -
The edit budget counts ops (files or keys), not independent edits. For a FileTree harness,
len(diff.ops)is a changed-file count, which #221 explicitly says the budget is not. A single coherent edit that touchesSKILL.mdplus a helper uses 2 of the budget, while ten unrelated rules in one file use 1. If attributed-edit counting has to wait for #253'sK, please say so in the deviations list. -
Partial screen rejections vanish. When some cards in a batch are screened out and others survive, the rejected cards are settled and counted, but their reasons go nowhere: they're not on the merge report and not in
RRsiState.ledger. #221 asks that "rejected candidates are logged without invented measurements", and the proposer should be able to condition on "rejected: leakage" as well. Please record screen rejections (with no measureddS/dC) in the state, and surface the reasons in the report detail even when the batch proceeds. -
Outcome labelling.
"floor" in decision.reasonmaps every other non-admissible reason toCOST_RULE, includinginvalid or missing candidate measurement(confirmed with a NaN count),invalid historical-best score, and any futuredomain guard violated. Please map explicitly, and fall back to the existing generic rejection category for invalid or guard reasons rather than mislabelling them as cost decisions.
Nits
- In
policies.py, thePolicies.screencomment says "every card that passes the trust region and staleness filter is merged", but the screen runs before staleness. The aggregator comment has it right. RRsiCriticuses plain substring matching for leakage, so a short protected string ("1", a common word) rejects almost everything. Consider word-boundary matching or a minimum length, and mention it in the docstring.- The model reviewer fails open on any exception or unparsable reply. That's reasonable, but it would be useful to count it (for example
critic_reviewer_errors) so a silently broken reviewer shows up. restore()swallows every exception from the acceptance hook. Since this is the path that "preservesS*", a corrupted payload that silently resets the floor tobase_scoredeserves at least a warning.
Tests and docs
The 16 new tests pass locally, and the end-to-end offline fixtures (leak gives critic-rejected with no commit, the untouched path reports zeros, resume keeps the floor) are good. Please add regression tests for items 1, 2, 3 and 6. The full suite on this head (3.13, local) has 4 failures, all in tests/test_checkpoint.py as above, and CI shows the same on 3.9/3.10. The CHANGELOG and docs/algo-rrsi.md updates read well. Once the round and cost semantics change, the deviations list will need a refresh.
Happy to re-review once these are addressed.
24ab7d8 to
872d707
Compare
|
All three findings are addressed on the new head 872d707:
|
|
Rechecked P2: record the accepted best score before appending progress. An external corrected-order control retains all three decision increments and ledger entries, records The new bounded controls yielded seven passes and this one failure; old-head controls separately confirm the original fixes discriminate. All checks were offline/synthetic. Exact-head docs are green; the test matrix is still running. Other items in the existing review remain outside this scoped verification; no formal approval is submitted. |
|
Status check at |
872d707 to
dac42bb
Compare
|
Rebased onto the current The P2 finding is fixed: rrsi/checkpoint/trajectory (92) plus aggregator/evolution/async/API/metrics (91) tests pass locally. |
Birfy
left a comment
There was a problem hiding this comment.
Re-review at dac42bb: the three blocking items are fixed
Verified against the prior REQUEST_CHANGES:
- Checkpoint safety —
checkpoint()/restore()now usegetattr(self, "acceptance_policy", None).tests/test_checkpoint.pypasses locally. - Per-merge anneal —
record_merge()runs on every decision (including screen-reject via the aggregator hook).test_a_streak_of_rejections_anneals_the_budget_and_trips_stalland the post-decision S* regressions pin the stuck-search case; accept now appends the post-decision S*. - Batch-independent cost —
_card_costis a per-rollout mean, documented as a proxy / deviation until candidate-side held-out cost exists.
Locally on Python 3.13, tests/test_rrsi_integration.py + tests/test_checkpoint.py are green. Exact-head CI is green.
Remaining non-blocking: novelty still maps via default_components path segments rather than the pinned STRUCTURAL_COMPONENTS / #253 vocabulary (examples/rrsi/components.py is now on main). Declared as a deviation is fine for this PR; a follow-up that reuses K would make the within-band novelty branch reachable on real layouts. Also: the branch conflicts with main after #253/#264 — please merge main in before landing.
Approving the blocking fixes.
|
Status check (four-hour routine, 2026-10-16 ~12:30 CST): still APPROVED from the |
|
主分支已有较多更新,当前 PR 与 main 存在冲突。请 rebase/merge main 并解决冲突后推送,我们会尽快合入。谢谢! |
… the Policies seams build_rrsi_bundle() assembles three policies that share one checkpointable RRsiState (S*, the measured per-edit ledger, component bookkeeping, the annealed round counter) into a Policies bundle, reusing the pinned selection core (judge / update_best_score) and the annealed budget (edit_budget) rather than rewriting them: - RRsiCritic (Policies(screen=...)): pre-evaluation screen -- annealed edit-budget gate + deterministic evolve-set leakage screening + optional model reviewer. Runs before measurement, so a leaking or over-budget card is settled and reported critic-rejected without a held-out sweep. - RRsiProposalPolicy (Policies(proposal=...)): injects budget, measured per-edit history, pruning targets and stall-exploration into the reflection. - RRsiAcceptance (Policies(acceptance=...)): the pinned judge decision with monotone S*, cost from EvidenceCard.cost_tokens (dC=0 without telemetry). Engine additions: ScreenPolicy seam on Policies/aggregator._prepare (before staleness and measurement); MergeOutcome critic-rejected / noise-floor / cost-rule; counters critic_checked / critic_rejected; aggregator checkpoint carries an acceptance policy's optional state so a resume keeps S*. The build check no longer demands propose= when Policies(proposal=...) supplies one. Default behaviour unchanged. Declared deviations (one-candidate judging, per-merge anneal off the barrier-free path, configured noise_band) recorded in docs/algo-rrsi.md.
Three review findings. 1. Aggregator.checkpoint()/restore() read self.acceptance_policy outside the AttributeError guard, so an aggregator built without that attribute (the checkpoint tests build one via __new__) raised instead of skipping the optional acceptance-state hook. Both reads now use getattr. 2. RRsiAcceptance.accept advanced the round and progress only on an accept, so a streak of rejections left the budget pinned at b_max and stalled() False -- the anneal and stall note never moved exactly when the search was stuck. RRsiState.record_merge advances the schedule per decision, accept() calls it on every outcome, and the aggregator's screen-rejected path calls an on_screen_reject hook so a critic-rejected batch also advances the anneal. Tests: a rejection streak anneals the budget and trips stall; screen rejections advance the schedule. 3. _card_cost batch-summed EvidenceCard.cost_tokens, so the cost rule depended on how many cards landed in the bucket (and measured the proposing rollout -- the incumbent side -- not the candidate). It is now a per-rollout mean, documented as a proxy until candidate-side held-out telemetry exists; missing/zero telemetry yields dC=0 as before. Test: three 100-token cards read the same as one.
record_merge() ran before best_score was updated on the accept branch, so an improvement that broke a rejection streak appended the pre-decision S* and read as a stall ([0.5, 0.5, 0.5] with best_score 0.8). The accept now advances the schedule after updating best_score, so progress records the post-decision S*; rejections and screen-rejected batches still advance exactly once with the unchanged S*. Regressions: an improvement after a rejection streak is not a stall, and the corrected progress survives checkpoint/restore (both fail against the pre-decision append).
dac42bb to
05a4c40
Compare
|
Rebased onto the current All previously-reviewed fixes are intact: absent-policy checkpoint/restore safety, per-merge anneal (including the screen-rejected path), the post-decision S* append on accepts (improve-after-rejection-streak is not a stall), and the batch-size-independent per-rollout cost mean. rrsi selection/schedule/history/integration + checkpoint (143) and aggregator/evolution/async/API/metrics/trajectory/multi-head/invariants (165) tests pass locally. |
|
P2 — The newly integrated invariant-rejection path bypasses RRSI annealing (source-only review at The rebase adds an early Proposed regression: combine the ordinary RRSI bundle with |
Birfy
left a comment
There was a problem hiding this comment.
Review at 05a4c40: prior blocking fixes look good; one new blocker
Fresh pass after the rebase onto current main. The three items from the earlier REQUEST_CHANGES still look fixed on this head:
- Checkpoint safety —
checkpoint()/restore()usegetattr(self, "acceptance_policy", None)plus optionalcheckpoint_state/restore_statehooks. - Per-merge anneal —
record_merge()/on_screen_reject()advance the schedule on accept, reject, and screen-reject; post-decisionS*regressions are present. - Batch-independent cost —
_card_costis a per-card mean token proxy, documented as a deviation.
CI on this head is green (build + test 3.9–3.13). ScreenPolicy seam placement (before staleness / held-out) and the declared deviations in docs/algo-rrsi.md look right.
Must fix: changelog fragment convention
main now uses changelog fragments (#295 / #296). PRs must not edit CHANGELOG.md; they add changelog.d/<n>-<slug>.md instead (see changelog.d/README.md). This branch still appends the RRSI bullet directly under [Unreleased] in CHANGELOG.md and carries no fragment under changelog.d/.
Please:
- Revert the
CHANGELOG.mdedit relative tomain. - Add something like
changelog.d/254-rrsi-integration.md(or the tracking issue number) with categoryAddedand the entry text (mention(#254)/#221as appropriate).
Until that lands, this will conflict with the weekly release path and with every other fragment-based PR.
Non-blocking (unchanged from prior review)
Novelty still maps via default_components path segments rather than the pinned STRUCTURAL_COMPONENTS / #253 vocabulary. Declared as a deviation is fine for this PR.
Happy to approve once the fragment is in place.
Move the Unreleased RRSI bullet out of CHANGELOG.md into changelog.d/254-rrsi-integration.md (main's fragment convention). Also call the acceptance policy's on_screen_reject hook on the violates-invariant path so a streak of governance refusals still anneals the edit budget and trips stall (same per-merge accounting as an all-screen-rejected batch). Regression covers RRSI + mutable_keys.
Birfy
left a comment
There was a problem hiding this comment.
Re-review after maintainer push (65b5c7e)
Addressed the open CHANGES_REQUESTED items on this head:
- Changelog fragment —
CHANGELOG.mdmatchesmain; entry moved tochangelog.d/254-rrsi-integration.md.python -m tools.changelog_fragments checkis green. - Invariant refusal anneal (P2) —
Aggregator._prepare'sviolates-invariantpath now calls the acceptance policy'son_screen_rejecthook (same per-merge accounting as an all-screen-rejected batch). Regression:test_invariant_rejections_advance_the_rrsi_schedule.
Merged origin/main (includes #298). Local: rrsi-integration + checkpoint + invariants + changelog-fragments green.
Approving; arming auto-squash when CI is green.
|
Maintainer push on
Auto-squash armed once CI is green. |
Conflict resolution in MergeOutcome / docs/api.md keeps both the multi-artifact missing-adapters outcome and the RRSI critic/floor/cost outcomes landed by Birfy#254.
CI failed test_the_api_reference_is_in_sync_with_the_package — run tools.gen_api_docs so VerifierBoost exports are reflected.
… the verifier, under a gate (#273) * feat(audit): verifier bootstrap flywheel — labels improve the verifier under a gate VerifierBoost wraps any VerifierProtocol and learns a monotone calibration curve (isotonic_fit, numpy-only PAV) from the audit's IMPROVEMENT pool. The bootstrap gate activates it only when the corrected residual error beats the raw by min_gain on a held-out slice of the same pool; otherwise the curve is recorded as a rejection, never installed. Safety invariants: - trains on the IMPROVEMENT pool only — never the CALIBRATION pool that measures the verifier (store.for_calibration already asserts the split); - corrects the cheap/learned layers (ranking + audit trust signal) but delegates eval_counts/full_eval raw, so the commit gate keeps the oracle's authority; - declares itself a new verifier version (version_tag) so wiring it as the audit's version_extra makes VerifierWatch withdraw the old calibration and re-measure the boosted verifier. attach(boost=...) and Audit.recompute() retrain on a resolved batch. Default unchanged: no boost -> no fit, no correction. * merge origin/main after #254; regenerate docs/api.md CI failed test_the_api_reference_is_in_sync_with_the_package — run tools.gen_api_docs so VerifierBoost exports are reflected. --------- Co-authored-by: Birfy <15378933+Birfy@users.noreply.github.com>
feat(rrsi): wire the regularized RRSI mechanism into evolve() through the Policies seams
The problem
The pinned RRSI fixtures (
examples/rrsi/selection.py— Algorithm 2's selection rule — andschedule.py— the cosine-annealed edit budget) have been merged, anddocs/algo-rrsi.mdsays exactly what remains: "no RRSI CLI, harness-directory domain, implemented leakage critic, edit-budget schedule, measured per-edit history, multi-round resume, trial aggregation, pruning loop, or production evaluation/accounting adapter yet." Issue #221's acceptance criteria (leakage screening before evaluation, edit-budget boundaries, history attribution, stall exploration, pruning, resume that preservesS*, evolve/holdout isolation, offline fixtures) are all unmet.The paper's mechanism (arXiv:2609.24972v3) is six coupled regularizers — L0-style annealed update sparsity, evidence-aware credit assignment, structured exploration, leakage screening, a noise-adjusted historical-best floor, and L1-style structural pruning — that together make harness self-improvement transfer beyond the evolve set at ~30% fewer policy tokens. This PR wires those regularizers into
evolve()through the engine's existingPoliciesseams, reusing the pinned selection core and schedule rather than rewriting them.What this PR adds
examples/rrsi/rrsi_policies.py—build_rrsi_bundle(complete, protected=..., reviewer=..., config=..., base_score=...)assembles three policies that share one checkpointableRRsiState(S*, the measured per-edit ledger, component bookkeeping, the annealed round counter):Policies(screen=...)RRsiCriticlen(ops) > b_trejected) and deterministic evolve-set leakage screening (protectedstrings), plus an optional modelreviewer. Runs before measurement, so a leaking or over-budget card is settled and reportedcritic-rejectedwithout a held-out sweep.Policies(proposal=...)RRsiProposalPolicyPolicies(acceptance=...)RRsiAcceptanceselection.judgedecision — historical-best floor, both cost-rule branches, novelty — withS*advanced through the pinnedupdate_best_score.A new engine seam —
ScreenPolicy(Policies(screen=...)): a card-level pre-evaluation gate wired intoAggregator._preparebetween the trust region and staleness, i.e. before any measurement. New outcomescritic-rejected/noise-floor/cost-rule; new counterscritic_checked/critic_rejected. The aggregator's checkpoint now carries an acceptance policy's optional state, so a resumed run does not loseS*(the test asserts it).An ergonomic fix —
_build_engineno longer demandspropose=whenPolicies(proposal=...)already supplies one (three honest sources: agent, callables, bundle).Declared deviations (recorded in
docs/algo-rrsi.md, not silent)select_round's "choose highest admissible argmax" is not exercised; each merge's candidate is judged against the incumbent andS*. This is the "one winner per round" semantics issue Support Google RRSI for regularized harness self-improvement #221 asks to establish first.noise_bandis a configured parameter (default0.0); the paper estimates it from the unchanged base harness.EvidenceCard.cost_tokens; missing/zero telemetry yieldsdC = 0, matching the pinned upstream behaviour.protectedstrings come from the evolve set only — never the final holdout; the tests enforce that discipline by construction.Backward compatibility
Policies.screenNonecritic-rejectedoutcomesAggregator(screen=)None_prepareskips the screen blockMergeOutcomeMeter/EvolutionResult_build_enginecheckpropose=requiredPolicies(proposal=...)— now accepted and honouredacceptance_stateadded; unknown keys ignored on restoreVerification
b_0wide →b_{T-1}narrow) and the critic enforces itS*monotone; checkpoint/restore round-tripS*critic-rejected, no commitdocs/api.mdregenerated,test_api_referencegreenFiles
12 files changed, +1093, -29
New:
examples/rrsi/rrsi_policies.py(+~430) —RRsiConfig,RRsiState,EditRecord,RRsiCritic,RRsiProposalPolicy,RRsiAcceptance,build_rrsi_bundletests/test_rrsi_integration.py(+~330) — 16 testsModified:
agentdescent/policies.py—ScreenPolicyprotocol,Policies.screenagentdescent/aggregator.py— screen wiring in_prepare, threeMergeOutcomemembers, checkpoint hookagentdescent/evolution.py—_WIRED_POLICIES+screenwiring + propose-source relaxation,critic_*result fieldsagentdescent/population.py—screenthroughpopulation_factoryagentdescent/metrics.py—critic_checked,critic_rejectedagentdescent/__init__.py—ScreenPolicyexportdocs/algo-rrsi.md— wired-integration section + declared deviationsdocs/api.md(+9) — regeneratedCHANGELOG.md(+17) — entryexamples/rrsi/__init__.py— docstring