Fix effort persistence display across sessions#1083
Conversation
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Targeted review of current head 93a831ef, focused on the CI auth change and the effort/startup behavior changed in this PR.
Verdict: Needs changes
Blocking issues:
- Focused tests fail locally:
detectProvider — startup effort displayno longer appends the effective effort forgpt-5.4.- the Anthropic
modelOverridealias test receivesgpt-5.5instead of resolvingopus, which suggests provider/default state changed unexpectedly in this path.
- The workflow change scopes
GITHUB_TOKENtobun install --frozen-lockfile. That exposes the repo token to every dependency install/postinstall script. For a PR workflow, that is too broad. If ripgrep install needs authenticated GitHub access, please scope the token only to the specific download/install step or use a safer mechanism that does not place the token in the full dependency install environment.
Validation:
bun test ./src/utils/effort.codex.test.ts ./src/components/StartupScreen.test.ts- Result:
effort.codexpasses,StartupScreen.test.tshas 4 failures.
Happy to re-review once the tests pass and the token exposure is narrowed.
6cdf9ed to
5141b37
Compare
There was a problem hiding this comment.
Reviewed current head 5141b37. The effort precedence changes now look consistent across the picker, startup banner, and suffix display, and the CI token workaround is narrowed to the ripgrep fallback instead of the full install environment.
Validated locally:
bun test src/components/StartupScreen.test.ts src/utils/effort.codex.test.tsResult: 48 pass, 0 fail. I also checked the current GitHub PR checks; both smoke-and-tests and web are passing.
No blocking findings from me.
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Targeted re-review of current head 5141b37, focused on the effort-display tests and the CI token-scoping concern from my earlier review.
Verdict: Approve-ready
What I checked:
- Startup banner effort precedence now matches the intended order: env override, saved
/effort, provider alias default, model/default fallback. - The prior focused
StartupScreen.test.tsfailures are fixed. - The CI auth workaround is no longer scoped across the full dependency install path.
- Ran
bun test ./src/components/StartupScreen.test.ts ./src/utils/effort.codex.test.ts: 48/48 passing.
I do not see a remaining blocker on current head.
BlockersNone. Non-BlockingNone. Looks Good
Verdict: Approve — clean effort persistence fix. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Clean effort persistence fix.
gnanam1990
left a comment
There was a problem hiding this comment.
The effort-precedence unification itself looks solid — picker/banner/suffix now share one chain, the "don't mutate app state before persistence succeeds" change is a genuine bug fix, and the test coverage on current head is good. Removing the stale tengu_effort_command call from the picker wrapper is a nice cleanup too. One thing to address before approve:
This PR also modifies .github/workflows/pr-checks.yml and release.yml. The token scoping was correctly narrowed after earlier feedback so I'm not worried about exposure — but a "fix effort display" PR shouldn't carry CI plumbing for an unrelated ripgrep-install flake. Could you split the workflow changes into their own PR so each can be reviewed on its own merits and this one stays focused?
Minor housekeeping: please drop the Co-Authored-By: OpenClaude (gpt-5.5) trailer on 2cef0c9 before merge, per convention. Happy to approve the effort changes as soon as the CI bits are split out — thanks!
Ensure /effort selections persist consistently from both direct commands and the picker, and make the startup banner use the same effective effort as the in-app UI instead of provider alias defaults. Document CLAUDE_CODE_EFFORT_LEVEL precedence and add focused tests covering startup display, suffix display, and unsupported models.
Preserve provider alias reasoning defaults in the startup banner when no saved /effort value or CLAUDE_CODE_EFFORT_LEVEL override is present. Saved settings and env overrides still take precedence, and xhigh aliases are normalized through the standard effort representation before display. Add a regression test for the gpt-5.4 OpenAI banner showing the alias default high effort in a clean settings session.
Drop an unused model lookup and OpenAI effort helper import left behind after centralizing EffortPicker state updates through the command result path.
Replace leaking effort test provider mocks with env-driven provider setup, expand provider env isolation, and keep the shared mutation lock so the focused tests do not leak state across Bun module imports.
techbrewboss
left a comment
There was a problem hiding this comment.
Review summary
Thanks for narrowing the PR back to the effort-display/persistence scope. The runtime direction looks valuable, and I did not find suspicious or malicious behavior in the current eight-file diff. I do see one blocking test-isolation issue that keeps the PR from being merge-ready.
Findings
src/utils/effort.codex.test.ts:91- Effort test mocks still leak intoStartupScreen.test.ts.
Impact: the PR's own focused validation command fails when run as stated.importFreshEffortModule()installs fixed provider/support mocks, and because Bun module mocks are process-global, the lastcodex/ unsupported mock bleeds intoStartupScreen.test.ts. That makes the startup effort assertions receivegpt-5.4instead ofgpt-5.4 (high|medium|low), and makes the Anthropicopusalias resolve togpt-5.5.
Suggested fix: avoid leaving fixed provider mocks installed across files. Reset the provider/providerConfig mocks to an env-driven or actual implementation after each test, or isolate this suite soStartupScreen.test.tssees the real provider modules. The claimed command should pass reliably:
bun test src/components/StartupScreen.test.ts src/utils/effort.codex.test.tsValidation
bun test src/components/StartupScreen.test.tspasses: 39 pass, 0 fail.bun test src/components/StartupScreen.test.ts src/utils/effort.codex.test.tsfails: 44 pass, 4 fail.- Current GitHub checks show
smoke-and-testsandwebpassing, but the local focused validation above reproduces the order-dependent failure.
This PR fixes effort display and persistence mismatches across the
/effortpicker, the startup banner, and in-app effort labels.
Previously,
/effort mediumcould appear active in one part of the UI while anew session's startup banner still showed a provider alias/default effort such
as
high. The issue was caused by effort display being split across severalpaths with slightly different precedence rules.
This PR makes those paths consistently respect:
CLAUDE_CODE_EFFORT_LEVEL/effortWhat Changed
Fixed
/effortpicker persistence:/effortcommands;effortLevel, matching/effort auto;EffortPickerno longer mutates app state before persistence succeeds.Fixed startup banner effort display:
/effortandCLAUDE_CODE_EFFORT_LEVELnow take precedence overprovider alias defaults;
exists, so aliases like
gpt-5.4continue to display their configureddefault effort accurately.
Unified in-app effort display:
getEffortSuffix()now uses the same displayed/effective effort level asthe bottom-right effort indicator;
Cleaned up effort command wiring:
state updates through the command result path.
Updated effort utility typing/imports:
toPersistableEffort()input typing so OpenAI-stylexhighnormalization is accepted.
Updated docs:
CLAUDE_CODE_EFFORT_LEVELin.env.example;/effortpersistence vs env override precedence indocs/advanced-setup.md.Tests
Added coverage for:
/effortover provider alias default;CLAUDE_CODE_EFFORT_LEVELover saved/effort;Validation
Ran focused tests:
bun test src/components/StartupScreen.test.ts src/utils/effort.codex.test.tsResult:
Also ran:
Result: no whitespace errors.
Notes
The full repo
bun run typecheckstill reports broad unrelated baseline issuesin generated/missing-module and non-effort areas.