Repository navigation
refactor(rrsi): fidelity closure — the wiring consumes the pinned K/L_t fixtures - #333
Conversation
…_t fixtures Component attribution goes through the pinned upstream vocabulary (components.normalize/classify_diff: skills/ -> skill, ToolRegistry -> client_tool, pure prose -> prompt) instead of the first-path-segment heuristic. The per-edit ledger is history.History (durable JSONL) when build_rrsi_bundle(history_path=...) is given, with an in-memory twin pinned to the same summaries by a parity test. - RRsiProposalPolicy reads History.render / prune_set (with actionable accepted edits) / stall_flag; - RRsiAcceptance scores novelty over History.incumbent_component_counts and records via append_candidate; - the aggregator checkpoint now carries only the search state (round, S*, incumbent cost, progress); the per-edit evidence is the History's own file, so a resumed run reads the same evidence the loop conditioned on; - stalled() uses the pinned history.stall_flag, which requires a full window of progress points (t >= w) — a stricter, correcter reading than the old tail-slice heuristic, and the integration tests were updated to that. Six new fidelity tests pin the vocabulary, memory/file parity, durable resume and a genuinely novel structural component carrying a within-band candidate.
Birfy
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
This is the right fidelity closure: the wiring stops re-deriving what components / history already pin, and the docs/tests match the claim.
Correctness
upstream_components→normalize(None, key\nvalue)is the right call site. Spot-checkedskills/…→skill,ToolRegistry→client_tool, prose →prompt; novelty/K_STRnow see the vocabulary Algorithm 2 actually counts.judge(..., history.incumbent_component_counts())is a real fix vs the oldattemptsCounter._novelty/components.noveltywant accepted structural counts on the incumbent; feeding proposal attempts suppressed novelty after a rejected try.- Checkpoint / History split is sound: aggregator payload keeps search state (
round/S*/ cost /progress); the JSONL is the durable ledger. Resume with the samehistory_pathre-reads the evidence the loop conditioned on. stalled()→stall_flag(progress, len-1, w, δ)matches the pinnedt >= wguard; integration tests correctly moved to a full window.
Design fit
- Reusing
History/componentsinstead of a parallel ledger is the port convention the module docstring promised. _InMemoryHistoryas a scratch twin + parity test is a good compromise for tests that do not want a path.build_rrsi_bundle(history_path=...)is the right seam; docs/algo-rrsi.mdupdated accordingly.
Tests / verification (local)
Fresh clone at f79e936. 107 RRSI tests green (test_rrsi_fidelity 6, integration 22, history 31, selection 29, schedule 19). CI: build green; test matrix 3.9–3.13 still pending at review time.
Docs / changelog
Fidelity table and wiring section in docs/algo-rrsi.md match the code. Fragment is present and readable.
Non-blocking follow-ups
- Changelog slug
331-rrsi-fidelity.md— open issue #331 is the unrelated Calibratormin_labels/min_per_stratumvalidation bug. Prefer renaming the fragment to track this PR (e.g.333-…) or an issue you open for the fidelity closure, so the next release notes are not confused with #331. - Parity is incomplete on rounded fields. File
History.append_candidateroundsdelta_S/delta_C/S/C;_InMemoryHistorydoes not (and skips_finite). Clean values intest_memory_and_file_history_agreepass, but e.g.delta_S=0.123456789/C=100.44makesrenderdiverge. Mirror the rounding (and ideally_finite) in the twin, or assert only the summary methods that stay equal. - Nits: unused
import jsonintests/test_rrsi_fidelity.py; unusedfieldimport inrrsi_policies.py; mid-modulefrom .components import K as _K_ALLcould sit with the other imports. - Outcome spelling: wiring writes lowercase
"accepted"/"rejected"whileMEASURED_OUTCOMESdocumentsACCEPTED/REJECTED/LOST. Harmless today (append does not validate), but aligning would reduce future drift.
No merge from this review (routine policy).
|
Two additional P2 findings from independent source review at
|
The problem
The RRSI wiring (
examples/rrsi/rrsi_policies.py, merged #254) re-derived two things the pinned upstream fixtures already provide: component attribution used a first-path-segment heuristic (skills/x/SKILL.md-> "skills") instead of the pinnedcomponentsvocabulary (K/K_STR/normalizeevidence rules -> "skill"), and the per-edit ledger was an in-memory list instead of the pinnedhistory.History(durable JSONLL_t, merged #253). The fidelity claim indocs/algo-rrsi.mdsaid the port is pinned; the wiring was not.What this PR changes
RRsiState.components_ofnow classifies each op viacomponents.normalize/classify_diff:skills/x/SKILL.mdis askilledit,ToolRegistry(...)isclient_tool, a pure prose edit isprompt. Novelty and pruning now count the components RRSI actually counts.history.History(durable) whenhistory_path=is given, with an in-memory twin for scratch runs and tests, pinned to the same summaries by a parity test so the twin cannot silently drift.RRsiProposalPolicyreadsHistory.render/prune_set(with the actionable accepted edits to remove) /stall_flag.RRsiAcceptancescores novelty overHistory.incumbent_component_countsand records every decision viaHistory.append_candidate.S*, incumbent cost, progress); the per-edit evidence is the History's own file, so a resumed run reads the same evidence the loop conditioned on.stalled()uses the pinnedhistory.stall_flag, which requires a full window of progress points (t >= w) — a stricter, correcter reading than the old tail-slice heuristic; the integration tests were updated to that semantics.Fidelity / deviations
skills/x/SKILL.mdK_STR)ToolRegistry(...)history.HistoryJSONL (durable)stall_flag(t >= w)Verification
yield_g/prune_set/render/counts)Files
5 files changed, +435, -159
Modified:
examples/rrsi/rrsi_policies.py—upstream_components,_InMemoryHistory, History-backedRRsiState/RRsiProposalPolicy/RRsiAcceptance,build_rrsi_bundle(history_path=)tests/test_rrsi_integration.py— updated to the upstream vocabulary and History API (intent preserved)docs/algo-rrsi.md— the wiring now consumes the pinned fixturesNew:
tests/test_rrsi_fidelity.py(+~150) — 6 fidelity testschangelog.d/331-rrsi-fidelity.md— fragment