Repository navigation
audit: VerifierBoost bootstrap gate must beat a constant-mean baseline (#306) - #311
Conversation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98b885998a
ℹ️ 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 beats_raw and not beats_const: | ||
| return self._deactivate( |
There was a problem hiding this comment.
When the new constant-baseline gate rejects a candidate, this call routes through _deactivate(), which constructs a state with the default error_const=inf. Consequently, retrain() and status() report that the baseline was not measured even though error_const was just calculated, making the newly exposed field unusable for diagnosing the rejection; preserve the computed baseline in the rejected BoostState.
Useful? React with 👍 / 👎.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Automated review (head 98b8859): LGTM, approving in spirit. Posted as a comment because GitHub doesn't allow self-approval.
Correctness
- The fix targets exactly what #306 describes.
error_constis the validation MSE of the train-slice mean (ysis train-only), so validation labels don't leak into the baseline. Requiringerror_after <= error_const * (1 - min_gain)alongside the raw test is the right gate: a pure bias shift can't beat a constant, but a curve that actually ranks can. - Order of checks: a curve that fails the raw test keeps the old reason, and one that passes raw but not constant gets the new "no discrimination beyond a constant" reason. Both rejections go through
_deactivate, so_xs/_vals/_fit_hashare cleared andversion_tag()staysboost:inactive. That meansVerifierWatchno longer withdraws calibration on an uninformative change. 👍 - The degenerate-curve guard is correct. Worth knowing: a single-level PAV curve is exactly the train mean, so
error_after == error_constand the constant gate already rejects it whenevermin_gain > 0. The new branch only matters whenmin_gain == 0orerror_const == 0, which is how the test exercises it. It's a fine belt-and-braces check, and a one-line comment saying so would help future readers. BoostState.error_const = math.infas a trailing default keeps the old positional constructors and consumers working, andstatus()only adds a key.
Tests
- Locally on a fresh clone:
tests/test_verifier_boost.pygives 23 passed. Withboost.pyreverted to main, all 4 new tests fail: the repro activates, and the control/status tests have noerror_const. So they genuinely guard the change. - The control (oracle = 1 iff f > 0.9) proves the gate didn't just get tighter across the board. Asserting that
cheap_evalis untouched on the rejection path is a nice touch. - I didn't finish a full local
pytest -qrun (the shared box is under load, and it was past 50% with all green when I stopped). CI build is green; the 3.9–3.13 tests were still running at review time.
Non-blocking suggestions
- Observability on rejection:
_deactivatewriteserror_before/error_after/error_const = inf. So right after the new #306 rejection,status()["error_const"]reads "not measured" even though it was measured (the numbers only survive inside thereasonstring). The test even codifies "after a rejection the baseline is not measured". Consider letting_deactivateoptionally carry the measured errors for the two gate rejections, so dashboards and the audit can tell "too few labels" apart from "measured and lost to a constant". - Docs drift: the
retrain()docstring ("activate only if the corrected validation error beats the raw bymin_gain") and the module docstring around line 19 still describe a raw-only gate. Please add the constant-mean condition there. - Follow-up idea (out of scope here): the gate is still a single holdout split. With small
min_val_labels, both comparisons are noisy. A paired bootstrap or a minimum-val-size rule for the constant comparison could be its own issue.
The changelog fragment changelog.d/306-boost-constant-baseline.md (Fixed) matches the fragment convention. Scope stays within the gate, as the issue asked (no eval_counts/full_eval or #304 changes).
…ion (#306) When the bootstrap gate rejects because a candidate fails the constant-mean baseline (or the raw-error / degenerate checks after those MSEs were computed), keep error_const / validation MSEs / n_train / n_val on the rejected BoostState instead of _deactivate()'s default inf, so status() can diagnose the rejection.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Verdict: LGTM (posted as COMMENT; self-APPROVE is often blocked). Reviewed at head dd9d47a.
Closes #306. The bootstrap gate now also requires the fitted curve to beat a constant-mean baseline by min_gain, and rejects degenerate single-level curves. BoostState / status() expose error_const.
Follow-up on this head addresses the Codex P2: when the constant-baseline / bootstrap / degenerate path rejects after MSEs were computed, _deactivate(..., error_const=...) retains the measured diagnostics instead of defaulting to inf ("not measured"). Tests cover pure-bias rejection with finite error_const / error_before / error_after / n_*, informative activation still passing both gates, status key surface, and degenerate rejection.
Fragment changelog.d/306-boost-constant-baseline.md is present; no CHANGELOG.md Unreleased edit.
Merge note: this and #312 both touch agentdescent/audit/boost.py and tests/test_verifier_boost.py — expect a conflict if merged in either order without rebasing the other.
Closes #306
What
VerifierBoost.retrain()activated a curve whenever validation MSE beat the raw verifier's bymin_gain, which a pure bias shift satisfies even with zero discriminating power. The gate now also requireserror_after <= error_const * (1 - min_gain), whereerror_constis the validation MSE of the train slice's constant-mean predictor. A failure is recorded as a rejection ("no discrimination beyond a constant ..."), the boost stays inactive andversion_tag()staysboost:inactive. A curve with < 2 distinct fitted levels is rejected explicitly as well.BoostStategainserror_const(defaultinf, so existing constructors keep working) andstatus()exposes it; existing keys are unchanged. Scope is confined to the gate inretrain(no change toeval_counts/full_evaldelegation or the #304 items).Tests
tests/test_verifier_boost.py: the issue's repro (stays inactive, reason mentions the constant baseline), a control (oracle = 1 iff f > 0.9, scores in [0.8, 1.0] still activates), status/BoostState compatibility, degenerate-curve rejection. Fragment:changelog.d/306-boost-constant-baseline.md. Full localpytest -qpasses (one timing-sensitivetest_era_algotunesandbox test failed under heavy shared load and passes in isolation).