Repository navigation
feat(rrsi): port the component vocabulary K and per-edit history L_t (#252) - #253
Conversation
…252) Adapted from google-research/rrsi components.py and history.py at be50316e. Upstream behaviour preserved (evidence-checked tags, -inf yield outside the prune window, placeholder record for empty candidates); defensive boundaries reject non-finite measurements, tolerate a torn final JSONL line, and rewrite atomically in replace_round. selection.py now takes K_STR from components. Still an offline microport, not an evolve() integration.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72095f53bf
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if d in K and has_evidence(d, diff, domain_signals): | ||
| return d |
There was a problem hiding this comment.
Let text-only diffs override declared structural tags
When a prompt string or comment mentions a generic signal such as SkillRegistry, text_only(diff) and classify_diff(diff) correctly identify it as a prompt edit, but this branch calls has_evidence first and retains a declared skill tag. That lets a prose-only change satisfy a reserved structural exploration slot and corrupt the tried and novelty summaries that normalization is intended to protect; check the text-only case before accepting the declaration.
Useful? React with 👍 / 👎.
| def records(self) -> List[dict]: | ||
| if not self.path.exists(): | ||
| return [] | ||
| text = self.path.read_text(encoding="utf-8") |
There was a problem hiding this comment.
Decode torn UTF-8 tails only after isolating the final line
If a crash interrupts an ensure_ascii=False append in the middle of a multibyte character, decoding the entire file here raises UnicodeDecodeError before the torn-final-line recovery logic runs. Consequently records() and every summary method fail on a crash state that this implementation promises to ignore, even though the next append() can repair it; split the byte data first and decode the potentially torn tail separately.
Useful? React with 👍 / 👎.
Automated review: request changes (one small fix)GitHub doesn't allow a request-changes review on your own PR, so I'm posting it as a comment. Reviewed at Overall: this is a careful, faithful slice. I diffed Blocking: torn-tail recovery fails when the cut lands inside a multibyte UTF-8 characterThe PR promises that "a torn final line from a crash mid-append is ignored by Repro: h = History(p)
h.append_candidate(1, "v", [{"id": "e1", "component": "skill", "hypothesis": "技能"}],
"ACCEPTED", 0.1, 1.0, True, 0.5, 10.0, None)
line = json.dumps({"t": 2, "hypothesis": "中文假设"}, ensure_ascii=False).encode()
open(p, "ab").write(line[: line.index("中".encode()) + 1]) # cut mid-character
h.records() # UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe4 ... unexpected end of data
h.append({"t": 3}); h.records() # works: the append path drops the tailSuggested fix: read bytes, split on Non-blocking nits
Once the UTF-8 torn-tail case is handled and tested, this looks good to merge from my side. |
… mode-preserving rewrite (#252) History.records() splits bytes before decoding, so a crash that cuts the tail inside a multibyte character is skipped like any torn line; invalid UTF-8, invalid JSON or a non-object line elsewhere raises ValueError with its line number. replace_round fsyncs and keeps the original file mode (mkstemp is 0600). _finite accepts numbers.Real (np.float32). normalize() treats a text-only diff as prompt before honouring a declared structural tag (documented deviation). Regression tests fail on the previous head.
|
Addressed the review and both Codex findings in
All 7 new tests fail against the previous |
Automated re-review at
|
|
Independently verified the reviewed repairs at The additional checks cover every partial-byte cut within a Chinese character and emoji in a writer-generated tail, successful following-append recovery, and line-numbered rejection of earlier invalid UTF-8/JSON/non-object records. Text-only comments containing each structural signal normalize to Exact-head tests and docs are green, including Python 3.9–3.13 wheel/install smoke. The normalization change remains an explicitly documented upstream deviation. This is scoped verification of these fixes; #254 integration reconciliation and arbitrary malformed-input behavior are not certified here. No provider calls or formal approval. |
|
Merged current |
Birfy
left a comment
There was a problem hiding this comment.
Re-check at latest head
Verdict: LGTM (Closes #252)
Confirmed the earlier blocking finding is on the branch: 7dee36d makes History.records() split bytes before UTF-8 decode, so a torn tail cut inside a multibyte character is ignored like any other torn line; mid-file invalid UTF-8 / non-object raises ValueError with a line number. Regression test_a_torn_tail_cut_inside_a_multibyte_character_is_ignored is present.
Re-verified on head 209cdebc (post–main merges):
pytest tests/test_rrsi_history.py→ 31 passed (includes torn / multibyte / invalid-utf8 cases)- CI: tests 3.9–3.13 + docs build SUCCESS
Ready to merge from our side.
(Posted as COMMENT — author account cannot APPROVE.)
Closes #252. Part of #221.
What
Next offline slice of the Google RRSI port (pinned at
be50316e1db05914068a973f322770ef08ed7ba1), the evidence the proposer and pruner read:examples/rrsi/components.py, adapted fromrrsi/components.py:K,K_STR,text_only,classify_diff,has_evidence,normalize,novelty.examples/rrsi/history.py, adapted fromrrsi/history.py: the per-edit JSONLHistory(append_candidate,tried,attempted,accepted_edits,yield_g,prune_set,render,replace_round) andstall_flag/exploration.selection.pytakesSTRUCTURAL_COMPONENTSfromK_STRinstead of its own copy. Same set, so no behaviour change, and a test pinsnoveltyparity withselection._novelty.Apache-2.0 notices are kept on both adapted files.
Upstream behaviour preserved (and documented in
docs/algo-rrsi.md)config,control_flow, ...) survivesnormalizeonly if a domain signal backs it, because the generic signals only cover structural components.yield_gis-inffor a tried component with nothing inside the prune window, so it is inB_t.C1, no component,bundle = 0).stall_flagis 0 untilwrounds exist.Defensive boundaries (AgentDescent convention, not upstream parity)
delta_S/delta_C/S/CraiseValueError. Upstream would write non-standardNaNJSON.records()and dropped by the nextappend. A malformed line anywhere else still raises with its line number.replace_roundrewrites through a temp file andos.replace.Tests
tests/test_rrsi_history.py(24 cases) covers bundle fan-out, the placeholder record, measured vs attempted, non-finite rejection, torn-line recovery, mid-file corruption, atomicreplace_round, the yield window andB_twith accepted machinery,render's cap on unmeasured records, stall and exploration text, andnormalizeevidence checks.pytest -n 8full suite passes locally (Python 3.13).Not in scope
The critic, proposer, pruning execution, the loop, the CLI and
evolve()wiring all stay on #221. Nothing consumes the history yet.