test(browser): guard thinking-effort locale word lists against silent drift - #417
Open
OfficialAbhinavSingh wants to merge 1 commit into
Open
Conversation
… drift LEVEL_TOKENS, EFFORT_WORDS, and ADVANCED_WORDS in thinkingTime.ts each carry a per-locale word list, grown language-by-language across steipete#377, steipete#405, and other fixes (German, Chinese, Japanese, Spanish, Portuguese, Italian, Dutch, Polish). The existing tests only assert each list's leading word(s) as a stand-in for "the array still starts the same way" - a word dropped from the middle or end of any list would pass every existing test unnoticed, the exact gap docs/browser-mode.md's "self check" note is about. Add three tests asserting every word in every list is still present. Verified these actually catch drift: temporarily removed 'esforco' (mid-array in EFFORT_WORDS, a word none of the existing tests touch) from the source and reran - the new test failed on exactly that word, confirmed, then restored the source unchanged. No production code touched. Same pattern as steipete#416, piloted on archiveConversation.ts.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P3
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 25, 2026, 6:07 AM ET / 10:07 UTC. ClawSweeper reviewWhat this changesThe PR adds browser tests intended to preserve every localized thinking-level, effort-menu, and advanced-menu label in the generated ChatGPT picker expression. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain Keep open: the new assertions search the entire generated expression, so they do not reliably prove membership in each declared locale list; real behavior proof is also still needed before merge. Priority: P3 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherOracle’s browser mode generates JavaScript that finds ChatGPT’s multilingual thinking-effort picker controls and selects a requested tier. That expression runs in the active browser page before the prompt is submitted. flowchart LR
A[CLI thinking-time option] --> B[Browser thinking-time action]
B --> C[Generated page expression]
C --> D[Localized picker labels]
D --> E[Select and verify effort]
E --> F[Browser prompt submission]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Make each inventory test inspect only its corresponding literal array or export immutable locale data for direct equality checks, then show that removing an overlapping term fails and restoring it passes. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: removing Is this the best way to solve the issue? No: the tests must scope checks to each named array; global string presence cannot enforce the claimed per-list inventory contract. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against 79e483bd9dc8. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
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
thinkingTime.ts's three word lists:LEVEL_TOKENS(5 tiers),EFFORT_WORDS,ADVANCED_WORDS.Problem
Same gap named in
docs/browser-mode.md:thinkingTime.tsalready has good multilingual test coverage (#377, #405 both added regression tests), but on inspection each existing check only pins a word list's leading entry as a stand-in for "unchanged", e.g.:That proves
'extended'and'high'are still first — it says nothing about'hoch','erweitert','高い','扩展','深度','加强', or'高', the other 7 words in that same array. Same shape forEFFORT_WORDS(2 of 11 words checked) andADVANCED_WORDS(0 of 8 checked anywhere). A word dropped from the middle or end of any of these arrays passes every existing test.Change
tests/browser/thinkingTime.test.ts: three new tests, each asserting every word inLEVEL_TOKENS(per tier),EFFORT_WORDS, andADVANCED_WORDSis present in the generated expression.Verification that this actually catches drift
Temporarily removed
esforco— a mid-arrayEFFORT_WORDSentry none of the existing tests touch — reran, restored:Restored the source (
git diff --stat src/browser/actions/thinkingTime.ts→ no diff) and confirmed green again.Checks
pnpm vitest run tests/browser/thinkingTime.test.ts— 63 passed (was 60 before this PR)pnpm test— 152 files / 1797 passed / 44 skipped (unchanged skip count)pnpm run typecheck— cleannpx oxlint tests/browser/thinkingTime.test.ts— cleannpx oxfmt --check— cleanCHANGELOG.mdleft as-is (mis-formatted on main; release owner per #349).Scope note
Second in the same series as #416 (archive matcher pilot). Same pattern, applied to the file with the most locale-drift history (#377, #405). Remaining action files with per-locale matchers (
modelSelection.ts,navigation.ts,attachments.ts, etc.) are natural next candidates.