Repository navigation
fix(audit): VerifierBoost recompute oscillation and legacy full-eval delegation - #312
Conversation
…ation delegation (#304) Separate the scorer identity from the correction's measurement identity so a validated curve is not withdrawn when the unchanged scorer merely lacks new labels, and delegate full_eval/oracle_eval through full_eval_of so an oracle_eval-only verifier works when wrapped.
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. |
Birfy
left a comment
There was a problem hiding this comment.
Verdict: LGTM (posted as COMMENT; self-APPROVE is often blocked). Reviewed at head 1c803a3 (includes merge of origin/main).
Closes #304. Both acceptance criteria look met:
- Recompute oscillation:
AuditedReward.scorer_version/AuditRecord.scorer_versionseparate scorer identity from correction measurement identity.VerifierBoost.retrain(..., scorer_version=...)trains across correction states for the same scorer;_retainkeeps a validated curve when the unchanged scorer merely lacks labels; a different scorer still withdraws. Wiring passesscorer_version=self.reward.scorer_versionon recompute. - Legacy full-eval:
full_eval/oracle_evaldelegate throughfull_eval_of, so anoracle_eval-only verifier withoracle_shares_full_set=Falseworks when wrapped (active or inactive).
Tests in tests/test_verifier_boost.py cover stable recompute without new data, unstamped-label path, scorer change withdrawal, and wrapped legacy oracle_eval. Locally: pytest -q tests/test_verifier_boost.py tests/test_audit*.py all green after merging main. Fragment changelog.d/304-verifier-boost-recompute-and-legacy-eval.md present; no Unreleased CHANGELOG.md edit.
Merge note: conflicts likely with #311 (boost.py + test_verifier_boost.py). Prefer merging one, then updating the other branch from main.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd35c66ea4
ℹ️ 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".
| #: records written without a correction layer (and on older stores), where | ||
| #: ``verifier_version`` is the scorer's identity; omitted from the on-disk | ||
| #: line then, so those files are unchanged. | ||
| scorer_version: str = "" |
There was a problem hiding this comment.
Version the new persisted record shape
When a boosted audit writes a nonempty scorer_version, the JSONL record gains this field but still declares schema version 1. A resolver or worker running the previous release therefore passes the version check, forwards the unknown field to its old AuditRecord constructor, and catches the resulting TypeError as a corrupt line; every such pending record is silently omitted and cannot be resolved. Introduce a new schema version with explicit migration support for version-1 records so older processes reject the new shape rather than silently losing audit work.
Useful? React with 👍 / 👎.
Birfy
left a comment
There was a problem hiding this comment.
Review — #312 (Closes #304)
Verdict: LGTM. Both acceptance checks in #304 are addressed as two separate fixes, the fingerprints stay backward-compatible, and the regressions are real. Nothing blocking; a few follow-ups below, one of which (forward compatibility of the record format) I'd like at least a conscious decision on.
What I verified (fresh clone of fd35c66 + merge 1c803a3)
tests/test_verifier_boost.py: all pass on the PR. Withagentdescent/reverted toorigin/main, all 9 new tests fail (5 oscillation/identity, 1 record round-trip, 3 legacy-eval), so they genuinely pin the bugs.- All
test_audit_*,test_verifier*,test_aggregator,test_changelog_fragmentspass locally;python -m tools.changelog_fragments check→ 7 fragments OK. CI:buildgreen, the 3.9–3.13 test matrix was still running when I posted. - Probed the identities: with a boost attached,
scorer_versionequals the no-boostverifier_version(f073b06…) andverifier_versionis the tagged one (0831ab3…) — i.e. the split is exactly "old fingerprint without the tag" vs. "old fingerprint with the tag", so existing stores keep their identities.
Correctness
- Oscillation fix — sound. Training is now keyed on the scorer identity, and
_trained_formakes "too few labels" retain the curve only when that identity is unchanged;_deactivateclears it, so a withdrawn curve can't be resurrected by the retain branch. A failed bootstrap gate with enough labels still withdraws, which is right. _derive_versions— byte-identical to before in all three cases (explicit version; no correction; correction withbase is None→extra=tag). The[base, tag]branch only arises when a caller passes both, whichattachdoesn't (it only installs the tag whenversion_extra is None), so no existing fingerprint moves.- Legacy
oracle_eval— routing bothfull_evaland the wrapper'soracle_evalalias throughfull_eval_of(self._inner)is the right fix;shares_eval_counts/oracle_shares_full_setstill come from the inner verifier, and the aggregator test shows the independent gate refuses the same regression wrapped and unwrapped instead of dying withAttributeError.
Suggestions (non-blocking)
- Older builds will silently drop stamped records.
scorer_versionis a new dataclass field butSCHEMA_VERSIONstays 1. A v0.6.0 reader doescls(**d)→TypeError→_load_linecounts the line as corrupt and skips it. Since every record written with a boost attached is now stamped (even while the boost is inactive, becausescorer_version != verifier_version), downgrading, or running a mixed-version audit service against the same store, quietly loses those labels rather than failing loudly. Options: bumpSCHEMA_VERSION(and have this build accept 1 and 2) so old builds hit the explicit "refusing to guess" error, or at least note in the changelog fragment that stores written from this version on are not readable by ≤0.6.0. - Pre-boost labels of the same scorer are still excluded. Unstamped records written before a boost was attached have
verifier_version == scorer_version(no tag), butfor_improvement(..., scorer_version=s)only falls back tor.verifier_version == verifier_version(the tagged one). A one-line widening — also acceptnot r.scorer_version and r.verifier_version == scorer_version— would let those labels train the curve. Not a regression (main excluded them too), but it's the same "one scorer, every label" idea this PR introduces. test_a_real_scorer_change_still_withdraws_the_correctionchanges the scorer by assigningaudit.reward.scorer_versiondirectly and callingretrainby hand. A version that changes the scorer for real (e.g. a differentverifier_version/version_extra, or a secondattach) and goes throughaudit.recompute()would cover the wiring path too.- Other
for_improvement(v)callers (audit/service.pyimprovement_labels, therebalancedocstring example inwiring.py) still count by measurement version, so after activation they report only post-activation labels while the boost trains on the whole scorer pool. Probably fine, but worth a sentence in the docs, or switching the service stat to the scorer pool. - Nits: the
from ..verifier import full_eval_ofinside both methods could be module-level if there's no import cycle (there doesn't seem to be one:verifier.pyonly importsevolvable). Thedocs/api.mdrow forfor_improvementcollapsed tofor_improvement(...), which drops the signature from the reference table (generator truncation?).
Thanks, this is a clean, well-scoped fix with regressions that actually pin the bugs.
…r-boost-recompute-and-legacy-eval # Conflicts: # agentdescent/audit/boost.py # tests/test_verifier_boost.py
#324) * fix(audit): bump SCHEMA_VERSION to 2 for scorer-stamped records (#315) Records with the post-#312 scorer_version shape now persist schema_version=2 so older v1-only readers hit their explicit unsupported-version raise instead of TypeError→corrupt-skip on pending work. New reader migrates v1 lines (including the mistaken early stamp under schema_version=1). Scorer-identity behaviour from #304/#312 is unchanged. * fix(audit): malformed v1 lines stay corrupt-skipped; reject unknown fields (#315) Addresses Codex review on #324: - P1: AuditStore._load_line preflight now only raises on versions outside SUPPORTED_SCHEMA_VERSIONS, so a malformed v1 line goes through the guarded construction and is counted corrupt instead of aborting the whole store. - P2: AuditRecord.from_dict no longer filters unknown keys; a misspelt field (e.g. strtaum) raises TypeError and is counted corrupt rather than loading under default values. Also regenerates docs/api.md (SCHEMA_VERSION=2), which failed the test matrix.
Closes #304
Two separate findings from the #273 review, kept as two separate fixes.
1. Recompute oscillation
Activation changed the audit's
verifier_version(it embedsboost.version_tag()), andAudit.recomputetrained only on labels of the current version. After activating, the next recompute found no labels under the new version, hit the insufficient-labels branch, deactivated (tag back toboost:inactive), and the following recompute re-fit the original pool.AuditedRewardnow keepsscorer_version(the scorer alone) apart fromverifier_version(the measurement identity, unchanged: scorer + boost tag). Newcorrection_extra=carries the live tag;attach(boost=...)uses it. Fingerprints are byte-identical to before.AuditRecord.scorer_version(omitted from the JSONL line when empty, so existing files/records are unchanged).AuditStore.for_improvement(..., scorer_version=)selects every label of one scorer; unstamped records still match byverifier_version.VerifierBoost.retrain(..., scorer_version=)trains on that pool and remembers the identity the active curve was fit for. If the scorer is unchanged and the pool merely has too few labels, the validated curve is kept; a different scorer (or failed gate) still withdraws it.2. oracle_eval-only verifier fails when wrapped
VerifierBoost.full_eval/oracle_evalcalledinner.full_evaldirectly. They now go throughfull_eval_of, the engine's compatibility reader, so a legacy verifier works wrapped (active or inactive) andoracle_shares_full_set=Falseindependence is preserved.Tests (offline,
tests/test_verifier_boost.py)Repeated recompute with no new data stays active/stable (also for unstamped older stores); post-activation sub-
min_labelsbatch joins the same pool and stays active; a real scorer change still withdraws; record (de)serialization compat; wrappedoracle_eval-only verifier, active and inactive, incl. the wrapper alias; aggregator independent gate refuses the same regression wrapped and unwrapped. They fail on main. Fullpytest -qpasses locally;docs/api.mdregenerated; changelog fragmentchangelog.d/304-verifier-boost-recompute-and-legacy-eval.mdvalidated.