feat(routing): emit true-θ̂ RMSE and persist effort snapshots - #650
feat(routing): emit true-θ̂ RMSE and persist effort snapshots#650cursor[bot] wants to merge 5 commits into
Conversation
…lation Issue #568 first slice: versioned per-role reasoning_effort_profile, fail-closed parse, replayable snapshot, and true-θ RMSE ablation. Temperature is not effort. Production route/conduct defaults stay locked. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
…onduct Same snapshot_hash on both paths when the operator passes a catalog. Default constructor still omits the key so production payloads stay put. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Buyer next action: pass role_effort_catalog to attach the same snapshot on route and conduct, or omit it to keep today's payload. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Issue #568 honesty slice: θ̂ is (1-λ)θ and RMSE uses θ̂-θ, not a rank constant. Parse/hash fail closed on missing profiles, blank versions, fractional seeds, and extra roles. The same snapshot survives run, stream_route, and batch_route. Production defaults stay locked while measurement_status is estimated. Buyer next action: python tests/test_reasoning_effort_profile.py Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (substance)
Buyer next action: merge #662 (f547cc6) instead of this tip. Then run python tests/test_reasoning_effort_profile.py. Keep live route/conduct defaults locked. Do not merge #631 over this slice. Do not raise temperature to stand in for effort.
The honesty core on this tip is real: θ̂ is (1−λ)θ, RMSE is √mean((θ̂−θ)²), temperature is ignored, and production_default_change_allowed stays false while measurement_status is estimated. Three fail-closed / persist seams still leak:
run_equal_budget_ablation((True, False))succeeds and emits θ̂.estimate_thetaon the same vector raises.- The fuzz target puts
true_thetainside the profile payload, so parse fails and Hypothesis/Atheris never reach the ablation lock asserts. stream_routewrites_workflow_runsonly. With--state-db, a restart drops the streamed snapshot whilerunandbatch_routesurvive.
#662 lands those three fixes on this substrate. After that lands, the next buyer gap is applying the opt-in catalog to ModelClient request knobs. Live NIM evidence stays issue #86.
CodeRabbit CLI is not authenticated in this environment (coderabbit missing / agent login unavailable). This review is source-and-math against 6841b71...a1ab71e.
Sent by Cursor Automation: Fix Issues
| chain-of-thought. Buyer next action: read ``measurement_status`` and | ||
| ``production_default_change_allowed`` before changing live defaults. | ||
| """ | ||
| theta = tuple(float(value) for value in true_theta) |
There was a problem hiding this comment.
run_equal_budget_ablation((True, False)) succeeds and emits θ̂ [0.714…, 0.0]. estimate_theta on the same vector raises EffortProfileError because _reject_non_finite_number rejects bools. float() also accepts numeric strings (("1.5", "-0.5")).
This is the buyer-facing true-θ entry. Validate each value with the same fail-closed helper (or call estimate_theta on the raw iterable) before building arms, so a boolean or string θ cannot be laundered into an RMSE report labeled usage_source="synthetic_true_theta".
Landed on #662.
| assert profile.access_list_scope in ACCESS_LIST_SCOPES | ||
| assert math.isfinite(profile.temperature) | ||
| assert math.isfinite(profile.top_p) | ||
| theta = value.get("true_theta") |
There was a problem hiding this comment.
This ablation branch is unreachable. true_theta is not in _PROFILE_KEYS, so any dict that includes it fails parse_reasoning_effort_profile at the try above and returns. Hypothesis and the Atheris harness never execute run_equal_budget_ablation or the production_default_change_allowed is False asserts, despite the docstring claiming they do.
Pop true_theta before parse, or after a successful parse run the ablation on a generated finite vector that is not part of the profile payload.
Landed on #662.
| "verification": {"accepted": True, "reason": "single route path", "verifier_output": ""}, | ||
| } | ||
| ) | ||
| self._workflow_runs[record["workflow_run_id"]] = record |
There was a problem hiding this comment.
run and batch_route call _store.save after _with_effort_snapshot. stream_route only writes _workflow_runs. With --state-db, a process restart drops the streamed run and its reasoning_effort_snapshot while route/batch snapshots survive.
Persist this record when self._store is not None, same as the other two paths this slice claims to keep.
Landed on #662.
seonghobae
left a comment
There was a problem hiding this comment.
Verdict: not ready
Head a1ab71e vs main 6841b71. This tip is the honest successor to #631 cb4a026 for θ̂ / RMSE and in-process snapshots. It is not the landing vehicle while the production-default gate still fail-opens on junk reports, and while the three earlier persist/parse seams on this head remain open.
CodeRabbit CLI 0.7.3 installed; coderabbit auth login --agent failed with automatic_login_failed / Automatic login timed out. Review below is from the head diff plus python3 tests/test_reasoning_effort_profile.py (ok).
Checklist
- θ̂ / RMSE — yes.
estimate_thetaemitsestimated_theta = (1−λ)θandrmse = RMSE(θ̂, θ). Different true-θ magnitudes change RMSE. Temperature is validated then ignored (same θ̂ at 0.1 and 1.5). Residual: RMSE is |λ|·RMS(θ), so same-scale permutations match; labeledmeasurement_status=estimated/usage_source=synthetic_true_theta. - Snapshot on complete / run / stream / batch — in-process yes, durable stream no.
_with_effort_snapshotis onroute_once,conduct,run,stream_route, andbatch_route.stream_routestill skips_store.save, so--state-dbrestart drops the streamed snapshot. Already filed; draft #662 claims the persist fix. - Parse / hash fail-closed — yes for the asked cases.
None, blankprofile_version, fractional seed, extra/missing roles, and{}catalogs raiseEffortProfileError. Remaining:run_equal_budget_ablationusesfloat()and accepts(True, False)/ numeric strings. Already filed; draft #662 claims the helper reuse. - Production gate — no.
{}andmeasurement_status="estimated"stay false. Omitting the field, or sending""/"not-a-status"with RMSE numbers androbustness_passed=True, returns True.OrchestrationPolicydefaults are unchanged (2.5/2/True/template/6/terms). - Buyer-felt honesty leftovers. Gate fail-open (this review). Boolean/string θ laundering (
reasoning_effort_profile.py:404). Dead fuzz ablation (fuzz/targets.py:225). Streamed snapshot not durable (orchestrator.py:973). HTTPchat_completion_responsestill omits the snapshot; catalog is not wired into the server constructor. - Tests — yes for RMSE, no for the gate junk path. IRT-scale
(-1.5, …, 1.5)vs wide(100, 200, 300), residual RMSE check, temperature invariance. Missing: omitted/junkmeasurement_statusmust stay locked; boolean θ must fail closed on this tip.
Remaining bugs on a1ab71e
| Severity | Location | Impact |
|---|---|---|
| Warning | reasoning_effort_profile.py:511 |
Junk / omitted measurement_status unlocks production_default_change_allowed. Still present on draft #662. |
| Warning | reasoning_effort_profile.py:404 |
Ablation float() launders bool/string θ. |
| Warning | fuzz/targets.py:225 |
true_theta is an unknown profile key, so Hypothesis/Atheris never hit the ablation asserts. |
| Warning | orchestrator.py:973 |
stream_route snapshot is memory-only under --state-db. |
Next buyer gap
Yes — ModelClient request knobs (reasoning_effort, max_output_tokens) remain the correct next gap after the honesty slice lands. chat / stream_chat / _batch_run still send max_tokens=self.max_output_tokens and a temperature argument; they never read the opt-in catalog. Do not fold that onto this PR. Do not change live route/conduct defaults. Live NIM evidence stays issue #86.
Do not merge #650 or #631. Prefer a successor that fail-closes the gate (draft #662 does not). Do not approve.
| return False | ||
| if not math.isfinite(baseline) or not math.isfinite(candidate) or baseline <= 0: | ||
| return False | ||
| if report.get("measurement_status") == "estimated": |
There was a problem hiding this comment.
This is not fail-closed on junk reports. report.get("measurement_status") == "estimated" is false when the field is omitted, blank, or a junk string, so a report with only RMSE numbers and robustness_passed: True unlocks:
production_default_change_allowed({
"single_model_baseline": {"rmse": 1.0},
"role_differentiated": {"rmse": 0.1},
"robustness_passed": True,
}) # TrueProbed on a1ab71e: omit / "" / "not-a-status" all return True. The CHANGELOG claim that junk reports stay locked is false.
Require an explicit proven status (for example measurement_status == "measured") before the improvement check. Treat missing or unknown status as locked. Draft #662 still has this same == "estimated" test.
There was a problem hiding this comment.
Buyer next action
Prefer #677 (55ce1ac) over this PR: #677
Run python tests/test_reasoning_effort_profile.py on #677. Keep live route/conduct defaults unchanged while production_default_change_allowed is false.
Why this tip is not the landing vehicle
θ̂ RMSE and in-process snapshots on complete / run / stream_route / batch_route are the right honesty slice over #631. Remaining buyer-facing gaps on a1ab71e:
production_default_change_allowedfail-opens whenmeasurement_statusis omitted, blank, or junk androbustness_passedis True.run_equal_budget_ablationaccepts(True, False)because itfloat()s before the bool reject.- The fuzz target treats
true_thetaas an unknown profile key, so Hypothesis/Atheris never reach the ablation asserts. stream_routewrites_workflow_runsonly;--state-dbrestart drops the streamed snapshot.ModelClientstill sends a singlemax_tokensand noreasoning_effortwhen the operator opted into the catalog.
#677 fail-closes the measured-status gate, rejects boolean θ, persists streamed snapshots, reaches the ablation from the fuzz target, and applies opt-in catalog knobs. Independent review from seonghobae is requested there. Do not merge #631. Do not change OrchestrationPolicy defaults. Live NIM stays issue #86.
Sent by Cursor Automation: Fix Issues
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head1e5e60c0d1aacca8a220eeca19a731d9345747e0. -
Head SHA:
1e5e60c0d1aacca8a220eeca19a731d9345747e0 -
Workflow run: 32162749860
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: fuzz.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (10 files)"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file (10 files)"]
R2 --> V2["required checks"]
Evidence --> S3["Docs (5 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs (5 files)"]
R3 --> V3["docs review"]
Evidence --> S4["Test (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (2 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: fuzz.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (10 files)"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file (10 files)"]
R2 --> V2["required checks"]
Evidence --> S3["Docs (5 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs (5 files)"]
R3 --> V3["docs review"]
Evidence --> S4["Test (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (2 files)"]
R4 --> V4["targeted test run"]
|


Buyer next action
Run
python tests/test_reasoning_effort_profile.py. Prefer this PR over #631. Keep live route/conduct defaults unchanged whileproduction_default_change_allowedis false. Do not raise temperature to stand in for reasoning effort.Why this PR
Issue #568 first slice on #631 stamped a catalog and claimed a true-θ RMSE ablation, but
estimate_theta_rmseignored the θ values andrun/stream_route/batch_routedropped the snapshot. This successor keeps the catalog and lock, and makes the evidence honest.What landed
estimated_tokens_usedunder a shared cap.profile_version, fractional seed, extra/missing catalog roles.reasoning_effort_snapshotsurvivescomplete,run,stream_route, andbatch_routewhen the operator opts in.measurement_statusisestimated.parse_reasoning_effort_profile.Doctoring (APA 7th)
Out of scope (do not fold in)
OrchestrationPolicydefaults.ModelClientrequest knobs (next buyer gap after this lands).Verify