Clamp classifier reasoningEffort per-model, fix real root cause of state-file corruption - #12
Merged
Merged
Conversation
…st.ts This file independently duplicates the PI_CODING_AGENT_DIR isolation pattern from auto-router-extension.test.ts (same debounced-save race, same explanatory comment) but didn't get the fix applied to that file earlier - and it just corrupted the real ~/.pi/agent/auto-router-state.json again, verified while investigating an unrelated question. Same fix: wait out AutoRouterHealthStore's SAVE_DEBOUNCE_MS in afterAll before deleting the env var or removing any temp dir. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ot cause of the state-file leak Two independent findings from a live retest after PR #11 merged: 1. The classifier still came back empty for gpt-5.3-codex-spark even with a valid, model-configured effort ("max") and the higher token cap. Root cause: Model.thinkingLevelMap's own doc says "missing keys use provider defaults" - in practice an unmapped level is forwarded to the raw API as its own literal name, and "max" is pi's own extended vocabulary that several models (this one included, per its own map only covering xhigh and minimal) don't actually understand despite it type-checking. This is exactly why the very first footer-badge mismatch existed: Pi's own real-turn dispatch already knows to clamp such a model down to what it actually supports (observably "xhigh"), but the classifier's raw completion call bypassed that entirely. Now mirrors it: "max" clamps to "xhigh" unless the model's own thinkingLevelMap explicitly confirms support. 2. My earlier per-test-file "wait out the debounce before teardown" fixes only protected each file against its *own* later teardown - they did nothing against a *different* test file's beforeEach changing the same process-wide PI_CODING_AGENT_DIR while an earlier file's save was still pending, which is exactly how the real ~/.pi/agent/auto-router-state.json kept getting corrupted with test fixture data even after those fixes landed. Root fix: AutoRouterHealthStore now pins its target path once, at construction, instead of re-resolving the env var on every debounced flush - making each instance immune to any later change to that var from anywhere, not just careful test teardown. This makes the ad-hoc per-file debounce waits unnecessary; removed them (verified corruption no longer reproduces, twice, with the real full test suite run twice against the actual global state file). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAuto Router now validates reasoning efforts against model capabilities, reports unsupported configurations, and applies effective levels during dispatch. AutoRouterHealthStore captures its state path at construction for consistent persistence operations. ChangesReasoning effort validation
Health state path capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…wn capability check The previous fix quietly clamped a classifier's "max" request down to "xhigh" with a hand-rolled, "max"-only heuristic - working around the symptom without telling the user their config asked for something the model can't actually do. Replaced with the real thing: pi-ai exports getSupportedThinkingLevels()/clampThinkingLevel(), the exact same capability resolution real turn dispatch already relies on (this is why real turns silently ran spark at "xhigh" instead of the configured "max" all along, invisibly). Added resolveSupportedEffort() in auto-router.ts, used at both call sites that dispatch a model at a configured effort: - applyRouting(), before pi.setThinkingLevel() - the real per-turn dispatch - routeForPrompt()'s classifier effort resolution Whenever the configured effort isn't in the model's actual supported set, it now warns explicitly (model, requested effort, what it does support, what's being used instead) rather than substituting silently. This surfaces the mismatch in general - not just for the classifier's "max" case that started this investigation, and not just when it happens to produce an empty reply. auto-router-classify.ts no longer does its own clamping: the caller guarantees an effort the model actually supports before calling it, so classifyTurnComplexity just trusts what it's given (still gating the raw reasoningEffort field to APIs verified to accept it at all). Also fixed the extension test suite's shared `model()` fixture helper, which didn't set `reasoning: true` or a thinkingLevelMap - meaning every existing fixture model looked entirely non-reasoning to getSupportedThinkingLevels and broke 6 unrelated tests. Now defaults to "fully capable" (reasoning: true, xhigh/max both mapped) so existing routing/escalation tests are unaffected; the new clamp-and-warn test uses its own narrower fixture modeled directly on the real spark model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resolveSupportedEffort only warns about a mismatch once a turn happens to route to that specific model - a model configured only in a rarely-hit tier, or one nothing has routed to yet this session, could otherwise sit silently misconfigured indefinitely. warnAboutUnsupportedConfiguredEfforts now checks every model+effort override in the config once at session start (deduplicated across tiers) and reports every mismatch in one notification, independent of routing activity. Verified against the real ~/.pi/agent/settings.json: correctly flags gpt-5.3-codex-spark, minimax/MiniMax-M3, and zai/glm-5.3 (all configured for "max" without actually supporting it) while correctly leaving gpt-5.6-luna, gpt-5.6-sol, zai/glm-5.2, and opencode-go/kimi-k3 alone (all genuinely support their configured effort). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #11 (merged). Findings from a live retest after that merge, in order of discovery:
1. Classifier still empty even with a valid, model-configured effort.
gpt-5.3-codex-sparkstill came back with(empty reply)after #11 landed - this time witheffort: "max"(correctly resolved from the medium tier's config) and the 8000-token cap, so it wasn't truncation. Root cause:Model.thinkingLevelMap's own doc comment says "Missing keys use provider defaults." In practice, an unmapped level gets forwarded to the raw API as its own literal name, and"max"is Pi's own extended vocabulary that several models don't actually understand despite it type-checking. This model's own map only confirmsxhighandminimal→low- no entry formax.This is also, retroactively, the explanation for the very first mismatch in this whole investigation (
Auto (max)badge next to a model running atxhigh): Pi's own real-turn dispatch already knows to clamp such a model down to what it actually supports - the classifier's raw completion call just never had that same protection.2. Don't silently clamp - warn, using the real capability check.
My first pass at fixing (1) quietly clamped
"max"to"xhigh"with a hand-rolled,max-only heuristic. Per review feedback: don't paper over a config/model mismatch, surface it. Replaced withpi-ai's own exportedgetSupportedThinkingLevels()/clampThinkingLevel()- the exact same capability resolution real turn dispatch already relies on internally (confirming why real turns to spark/luna at"max"have silently run at"xhigh"this whole time, with zero visibility). AddedresolveSupportedEffort()inauto-router.ts, used at both places a model gets dispatched at a configured effort - the classifier's own reasoning effort, and real per-turn dispatch viaapplyRouting(). Either now warns explicitly (model, requested effort, what it actually supports, what's being used instead) whenever there's a mismatch, rather than substituting silently in only one of the two places it happens.3. Validate the whole config up front, not just reactively.
Per-dispatch warnings only fire once a turn happens to route to a mismatched model - a model configured only in a rarely-hit tier could sit silently misconfigured indefinitely. Added
warnAboutUnsupportedConfiguredEfforts(), run once at session start: checks every model+effort override in the config (deduplicated across tiers) and reports every mismatch in one notification, independent of routing activity. Verified against the real~/.pi/agent/settings.json: correctly flagsgpt-5.3-codex-spark,minimax/MiniMax-M3, andzai/glm-5.3(all configured for"max"without actually supporting it), while correctly leavinggpt-5.6-luna,gpt-5.6-sol,zai/glm-5.2, andopencode-go/kimi-k3alone (all genuinely support their configured effort).4. The real
~/.pi/agent/auto-router-state.jsonkept getting corrupted, even after prior per-file test fixes.Earlier per-file "wait out the debounce before teardown" fixes only protected each file against its own later cleanup - not against a different test file's
beforeEachchanging the same process-widePI_CODING_AGENT_DIRwhile an earlier file's save was still pending. Root fix:AutoRouterHealthStorenow pins its target path once, at construction, instead of re-resolving the env var on every debounced flush - immune to any later change to that var from anywhere. Verified against the real file across multiple full-suite runs with zero corruption.Test plan
bun test(full suite) - 353/353 pass, run multiple times~/.pi/agent/auto-router-state.json: mtime and content unchanged across repeated full-suite runs (previously reproduced corruption every time)warnAboutUnsupportedConfiguredEffortsagainst the real~/.pi/agent/settings.jsonautoRouter configbunx tsc --noEmit- cleanSummary by CodeRabbit
New Features
Bug Fixes
Documentation