feat(i18n): extract claude_code_oauth/register_callbacks.py strings#653
feat(i18n): extract claude_code_oauth/register_callbacks.py strings#653thomwebb wants to merge 5 commits into
Conversation
thomwebb
left a comment
There was a problem hiding this comment.
Adversarial Review — Automated (code-puppy-4aeaa3)
Verdict: CONDITIONAL PASS — 4 blockers found
The headline claim "49 → 0 raw emit sites" is false. It is 49 → 4.
Blockers
1. Four raw emit sites were never wired to t()
The catalog keys exist and are correctly shaped, but the call sites were not updated:
| Line | Still-raw call | Orphaned key |
|---|---|---|
register_callbacks.py:124 |
emit_error("Could not start OAuth callback server; all candidate ports are in use") |
oauth.server.no_ports |
register_callbacks.py:133 |
emit_error(f"Failed to assign redirect URI for OAuth flow: {exc}") |
oauth.server.redirect_uri_error |
register_callbacks.py:142 |
emit_error(f"Could not parse pasted OAuth input: {exc}") |
oauth.pasteback.parse_error |
register_callbacks.py:146 |
emit_error(f"OAuth provider returned an error: {parsed.error_message}") |
oauth.pasteback.provider_error |
Fix: wire these four call sites. The translations are already correct.
2. Test suite has a structural blind spot — 14/14 green is misleading
The tests validate the catalog in isolation. They prove the JSON has correct keys and placeholders, but prove nothing about whether register_callbacks.py actually calls t() at those paths. This is exactly why all 14 pass while 4 raw strings hide in plain sight.
Add an AST-based structural test:
def test_no_raw_emit_in_register_callbacks():
import ast, pathlib
src = pathlib.Path("code_puppy/plugins/claude_code_oauth/register_callbacks.py").read_text()
tree = ast.parse(src)
offenders = []
for node in ast.walk(tree):
if isinstance(node, ast.Call):
fn = node.func
if isinstance(fn, ast.Name) and fn.id.startswith("emit_"):
for arg in node.args:
if isinstance(arg, (ast.Constant, ast.JoinedStr)):
offenders.append((node.lineno, fn.id))
assert not offenders, f"Raw emit_ calls found: {offenders}"3. Test population floor too loose
assert len(_oauth_keys()) >= 40 — PR adds 48 keys; 8 can silently vanish before this fails. Change to >= 48.
Should Fix
oauth.state_mismatchshared between two security paths (pasteback flow at line ~155, redirect callback at line ~180). A defender watching the terminal cannot tell if the mismatch is a CSRF attempt on redirect or a malformed paste. Consideroauth.pasteback.state_mismatch/oauth.callback.state_mismatch.error=excat line ~237 passes a bareException— implicitstr(exc)coercion, undocumented type contract. Useerror=str(exc).model_config.get("name")at line ~453 can returnNone→ user sees"skipping model 'None'.". Usemodel_config.get("name") or "(unknown)".
Track / Follow-up
_custom_help()description strings (lines ~259–275) are user-visible in/helpoutput but not scoped by theemit_*audit — open a follow-up issue
The 44 correctly wired call sites are clean. Key naming is coherent. Placeholder names match across catalog and call sites. The miss is entirely in 4 except clause paths that are structurally harder to audit by eye — which is exactly why the structural test needs to exist.
thomwebb
left a comment
There was a problem hiding this comment.
Review feedback addressed — commits c973189c + f864b204
Automated fix pass (code-puppy-4aeaa3). All 4 blockers resolved.
Landed:
- All 4 previously-unwired raw emit sites now call
t()with their catalog keys:oauth.server.no_portsoauth.server.redirect_uri_error(witherror=str(exc))oauth.pasteback.parse_error(witherror=str(exc))oauth.pasteback.provider_error(withmessage=parsed.error_message)
- AST-based structural test
test_no_raw_emit_in_register_callbacks— walks the file's AST and fails if anyemit_*call has aConstantorJoinedStrarg. This closes the "14/14 green while raw strings hide in plain sight" blind spot - Test floor tightened:
>= 40→>= 48 oauth.browser.open_failednow passeserror=str(exc)instead of raw exception objectoauth.model.no_api_keyguards againstNone:model_config.get("name") or "(unknown)"
Verification:
pytest tests/i18n/test_claude_oauth_i18n.py -q→ 15 passedruff check --fix/ruff format→ clean- Catalog JSON parses; all 4 keys confirmed present with correct placeholder names
Deferred (tracked as follow-up):
- Splitting
oauth.state_mismatchintooauth.pasteback.state_mismatch/oauth.callback.state_mismatchfor security-event distinguishability _custom_help()description strings — out of scope foremit_*audit; open a follow-up for/helpstring extraction
CI should re-run on the new commit.
f864b20 to
2cd3dd6
Compare
thomwebb
left a comment
There was a problem hiding this comment.
Rebased + piggyback test fix
Rebased onto latest main (now at f0dab866) and added a small piggyback commit fixing an unrelated broken test that CI was tripping over.
Context
Upstream commit f0dab866 (feat: add GPT-5.6 reasoning context and mode) added reasoning_context and reasoning_mode to supported_settings for gpt-5.6* models in chatgpt_oauth/utils.py but did not update tests/plugins/test_chatgpt_oauth_utils.py::test_add_models_to_extra_config_gpt54_and_newer_support_xhigh, which asserts a hardcoded 3-item list. Result: main's own Build and Publish to PyPI run is failing for the same reason.
What the piggyback commit does
Splits the assertion so codex-gpt-5.6-* variants expect the extra fields:
expected_settings = ["reasoning_effort", "summary", "verbosity"]
if model_name.startswith("codex-gpt-5.6"):
expected_settings.extend(["reasoning_context", "reasoning_mode"])
assert model_config["supported_settings"] == expected_settingsVerified locally: test passes on this branch.
Cleanup
This commit is a piggyback and will drop from the branch automatically once the same fix lands on upstream main via a rebase.
mpfaffenberger
left a comment
There was a problem hiding this comment.
Reviewed from a local worktree — thorough extraction, the str(exc) normalization at call sites is correct, and the 171-line test file pulls its weight. tests/i18n/ + tests/plugins/test_claude_code_oauth.py (180 tests) and ruff check pass locally. Comments:
1. Drop the shared test(chatgpt): update supported_settings expectation for gpt-5.6 commit
Same as noted on #652: main already carries an equivalent fix using startswith("codex-gpt-5.6-") (trailing dash) where this branch uses startswith("codex-gpt-5.6") — same lines, different content, guaranteed conflict, and unrelated to this PR. Rebase onto main and it should disappear.
2. Namespace: oauth.* is claimed with Claude-specific strings
Keys like oauth.browser.opening resolve to "Opening browser for Claude Code OAuth..." and oauth.cmd.fast.* are pure Claude features. When chatgpt_oauth gets extracted (it's the obvious next candidate), it can't reuse these and will have to mint oauth.chatgpt.*-style keys under a namespace whose bare keys are secretly Claude's. Suggest either:
- move provider-specific keys to
oauth.claude.*(orclaude_oauth.*), keeping only the genuinely provider-neutral ones (oauth.state_mismatch,oauth.callback.timeout,oauth.pasteback.*) at the shared level, or - at minimum note the intended convention in the PR so the chatgpt extraction follows it.
Cheap to fix now, annoying to fix after two more locales exist.
3. Plural nits (non-blocking)
oauth.auth.discovered_models ("Discovered {count} models") and oauth.cmd.logout.models_removed ("Removed {count} Claude Code models...") are ngettext candidates per docs/I18N.md. English hides it; plenty of target locales won't. Fine as a follow-up if you note it.
Nice detail: oauth.model.no_api_key guarding model_config.get("name") with or "(unknown)" instead of interpolating None.
2cd3dd6 to
2e9228e
Compare
thomwebb
left a comment
There was a problem hiding this comment.
Review feedback addressed — commit 5df11442
Automated fix pass. Applied the namespace refactor blocker.
Moved 24 Claude-specific keys → oauth.claude.*
Every key whose English value baked in "Claude Code" or referenced Claude-only concepts (fast mode, pasteback mode, model list refresh, etc.) is now under oauth.claude.*. Full mapping in the commit; a few examples:
| Before | After |
|---|---|
oauth.browser.opening |
oauth.claude.browser.opening |
oauth.cmd.fast.enabled |
oauth.claude.cmd.fast.enabled |
oauth.model.no_api_key |
oauth.claude.model.no_api_key |
oauth.reauth.refresh_failed |
oauth.claude.reauth.refresh_failed |
Kept at shared oauth.* level
Your explicit keep-list plus the other provider-neutral survivors:
oauth.state_mismatch,oauth.callback.timeout,oauth.pasteback.*oauth.server.*(all six subkeys — generic port/listener wording)oauth.callback.error,oauth.browser.headless_url,oauth.browser.fallback_url,oauth.browser.open_failed,oauth.browser.manual_urloauth.auth.exchanging,oauth.auth.exchange_failed,oauth.auth.save_failed,oauth.auth.no_access_tokenoauth.cmd.status.expires,oauth.auth.discovered_models
The reusable ones are ready for chatgpt_oauth extraction to slot into without minting duplicates.
One straggler flagged for the ngettext follow-up
oauth.cmd.logout.models_removed reads "Removed {count} Claude Code models..." — Claude-specific wording, but since it also needs ngettext treatment (which you deferred), I left it in place to keep this PR scope-clean. When the plural PR happens, it should land at oauth.claude.cmd.logout.models_removed and get pluralized.
Tests
180 passed (tests/i18n/ + tests/plugins/test_claude_code_oauth.py).
Deferred (per your nits)
oauth.auth.discovered_modelsngettext — follow-upoauth.cmd.logout.models_removedngettext (and the namespace move above) — follow-up
5df1144 to
7c9f0e7
Compare
Heads-up:
|
|
CI follow-up: I retriggered the failed Quality Checks run — it failed again, and it will keep failing on rerun regardless of your changes. Root cause: this branch's merge snapshot predates main's ruff pin ( Fix is the rebase onto current main already requested in the review (which also drops the duplicated gpt-5.6 test commit). A fresh push will generate a new merge ref with the pinned ruff and the lint job should go green — your actual changes pass |
Migrate 49 user-facing emit_* strings to the t() catalog; 49 → 0 raw sites. Key groupings (48 new oauth.* keys): oauth.server.* — callback server startup, ports, redirect-URI, paste hint oauth.pasteback.* — paste-back parse/provider errors, no-code, no-state oauth.state_mismatch / oauth.callback.* — state check, callback error/timeout oauth.browser.* — headless mode, opening, fallback URL, manual URL, open-failed oauth.auth.* — token exchange, save, success, model discovery oauth.reauth.* — refresh-failed, restored, no-token oauth.cmd.auth.* — starting, overwrite-warning oauth.cmd.status.* — authenticated, expires, models, no-models, not-authenticated, hint oauth.cmd.fast.* — wrong-model, enabled, enabled-detail, disabled oauth.cmd.logout.* — tokens-removed, models-removed, success oauth.model.* — no-api-key Tests: tests/i18n/test_claude_oauth_i18n.py (14 tests). All i18n tests pass. ruff clean.
7c9f0e7 to
ec1c5d4
Compare
What
Migrates 49 user-facing
emit_*strings fromplugins/claude_code_oauth/register_callbacks.pyto thet()catalog — previously the #1 highest-raw-site file (49 hits).49 raw → 0. No false positives.
48 new
oauth.*keysoauth.server.*oauth.pasteback.*oauth.state_mismatchoauth.callback.*oauth.browser.*oauth.auth.*oauth.reauth.*oauth.cmd.auth.*oauth.cmd.status.*oauth.cmd.fast.*oauth.cmd.logout.*oauth.model.*Tests
tests/i18n/test_claude_oauth_i18n.py(14 tests): namespace size, all keys resolve + pseudolocalize, interpolation for every key group, no leftover placeholders, module imports cleanly.Independence
Touches only
register_callbacks.py,en-US.json, and the new test — zero overlap with any open PR.