Skip to content

feat(i18n): extract core_commands.py user-facing strings#652

Open
thomwebb wants to merge 4 commits into
mpfaffenberger:mainfrom
thomwebb:feat/i18n-extract-core-commands
Open

feat(i18n): extract core_commands.py user-facing strings#652
thomwebb wants to merge 4 commits into
mpfaffenberger:mainfrom
thomwebb:feat/i18n-extract-core-commands

Conversation

@thomwebb

Copy link
Copy Markdown
Contributor

What

Migrates 41 user-facing emit_* strings across 7 slash-command handlers in command_line/core_commands.py to the t() catalog — the #2 extraction target at 47 raw audit sites (41 genuinely translatable; 6 false positives).

Result

47 raw → 6 — all remaining hits are false positives:

  • Rich-markup f-strings in picker-failure fallback paths (Text.from_markup(f"[dim]...{var}...[/dim]"))
  • Traceback diagnostics (f"Traceback: {traceback.format_exc()}")
  • Pure-variable agent description strings (f"{new_agent.description}")

31 new cmd.* keys added to en-US.json; 5 reuse existing keys:

Existing key reused From
cli.goodbye /exit goodbye message
cfg.agent.not_found agent not-found error
cli.agent.available agent listing
cmd.agent.picker_failed model picker failure (same message)

Key groupings

Namespace Commands covered
cmd.cd.* /cd: success, list-error, agent-updated, reload-error, not-a-dir
cmd.paste.* /paste: no-image, hint, count, send-hint, failed
cmd.tutorial.* /tutorial: chatgpt-oauth, claude-oauth, complete, skipped
cmd.agent.* /agent: already-using, switch-failed, switched, cancelled, picker-failed, usage
cmd.model.* /model: success, cancelled, usage, available
cmd.add_model.* /add_model: success, failed
cmd.model_settings.* /model_settings: success, agent-reloaded, reload-failed, failed
cmd.plan.usage /plan: usage error

Bonus fix

Renamed the loop variable ttoken in handle_generate_pr_description_command — it was shadowing the module-level t() import (caught by ruff F402).

Tests

tests/i18n/test_core_commands_i18n.py (10 tests): namespace populated, all keys resolve + pseudolocalize, interpolation for each command group, no leftover placeholders, module imports cleanly. All 128 existing i18n tests still pass. ruff clean.

Independence

Touches only core_commands.py, en-US.json, and the new test — no overlap with any open PR.

@thomwebb thomwebb left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review — Automated (code-puppy-4aeaa3)

Verdict: CONDITIONAL PASS — mechanical extraction is correct, no crash vectors. Three issues before merge.


Must Fix

1. Key reuse: cmd.agent.picker_failed used inside handle_model_command

core_commands.py:488 (approx) — the model command's picker-failure path reuses cmd.agent.picker_failed. If a translator ever needs to word these differently (agent picker vs model picker), there's no seam. Create cmd.model.picker_failed in en-US.json and use it in handle_model_command.

2. Test namespace floor too loose

tests/i18n/test_core_commands_i18n.pyassert len(_cmd_keys()) >= 25 allows 6 keys to silently vanish. The PR adds 31 keys; floor should be >= 31.

3. No targeted interpolation test for cmd.add_model.failed

The key has an {error} placeholder. test_no_leftover_placeholder_for_supplied_params catches this generically with "X", but a param-name typo in the catalog would still pass. Add one explicit assert.


Track / Follow-up (not merge blockers)

  • Text.from_markup("[yellow]Usage:[/yellow] /agent <agent-name>") near line 384 is content-identical to cmd.agent.usage but bypasses t() — will drift if a translator updates the catalog key
  • "No description" fallback (line 371) is user-visible, still raw
  • "Failed" in result dispatch in the undo handler will silently break if undo_manager.py ever gets localized — track in a ticket
  • Add # i18n: skip - developer output comments on the two Traceback: f-string lines so future auditors don't re-flag them

Everything else checks out: import is at module level, all 31 keys exist in en-US.json, all param names match, t() is crash-safe (missing key returns key, missing param leaves placeholder intact), no Rich markup in catalog values.

@thomwebb thomwebb left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review feedback addressed — commit 3b67f7ed

Automated fix pass (code-puppy-4aeaa3). Applied the three "must fix" items:

Landed:

  • Added new key cmd.model.picker_failed ("Model picker failed to open: {error}"); handle_model_command no longer reuses cmd.agent.picker_failed
  • Test floor tightened: >= 25>= 32 (matches actual current key count)
  • Added test_add_model_failed_interpolates_error_param — explicit interpolation check that catches param-name typos independently of the generic sweep test
  • Also added the new cmd.model.picker_failed key to test_model_keys_interpolate

Verification:

  • pytest tests/i18n/test_core_commands_i18n.py -q → 11 passed
  • ruff check --fix / ruff format → clean
  • Catalog JSON parses; 32 cmd.* keys

Deferred (track as follow-up, not blocking):

  • Text.from_markup("[yellow]Usage:[/yellow] /agent <agent-name>") at ~line 384 duplicates cmd.agent.usage content but bypasses t()
  • "No description" fallback at line 371 still raw
  • "Failed" in result dispatch in undo handler will break if undo_manager.py gets localized
  • Traceback: f-string lines still need # i18n: skip - developer output comments

CI should re-run on the new commit.

TJ Webb added 4 commits July 20, 2026 13:28
Migrate 41 user-facing emit_* strings across 7 command handlers
(/cd, /paste, /tutorial, /exit, /agent, /model, /add_model,
/model_settings, /plan) to the t() catalog.

- /cd: directory-change success/error, agent-context reload (cmd.cd.*)
- /paste: clipboard image flow, count, hints (cmd.paste.*)
- /tutorial: OAuth flow start, completion, skip notices (cmd.tutorial.*)
- /exit: reuses existing cli.goodbye key (no new key)
- /agent: already-using, switch-failed, switched, cancelled,
  picker-failed, usage (cmd.agent.*); not-found reuses cfg.agent.not_found,
  available-list reuses cli.agent.available
- /model: success, cancelled, usage, available (cmd.model.*);
  picker-failed reuses cmd.agent.picker_failed
- /add_model: success, failed (cmd.add_model.*)
- /model_settings: success, agent-reloaded, reload-failed, failed
  (cmd.model_settings.*)
- /plan: usage error (cmd.plan.usage)

Adds 31 cmd.* keys; 47 raw sites -> 6 (all 6 remaining are false
positives: Rich-markup f-strings in fallback paths, traceback
diagnostics, and pure-variable agent description strings).

Also renames the loop variable 't' -> 'token' in
handle_generate_pr_description_command to avoid shadowing the
module-level t() import (caught by ruff F402).

Tests: tests/i18n/test_core_commands_i18n.py (10 tests).
All 128 i18n tests pass. ruff clean.
- Add distinct cmd.model.picker_failed key (was incorrectly reusing
  cmd.agent.picker_failed in handle_model_command fallback path).
- Point the model-picker fallback emit_warning at the new key so the
  displayed message matches the actual subsystem that failed.
- Tighten the cmd.* namespace floor test to >= 32 (the true post-
  extraction count) so accidental key removals fail fast instead of
  silently shrinking the catalog.
- Add explicit interpolation test for cmd.add_model.failed that
  substitutes error='XYZ' and asserts the literal '{error}' token is
  gone -- catches placeholder-name typos independently of the generic
  sweep.
The '--show' branch of handle_model_settings_command used 'for t in tokens[1:]'
which shadowed the module-level 't' translation import for the entire
function scope. This caused UnboundLocalError on every subsequent t() call
in the non-show branch. Renamed loop variable to 'token'.
Upstream commit f0dab86 added reasoning_context and reasoning_mode to
supported_settings for gpt-5.6* models but did not update this test.
Split the expected list so gpt-5.6* variants get the extra fields.

This piggyback will drop from the branch once a standalone upstream fix lands.
@thomwebb
thomwebb force-pushed the feat/i18n-extract-core-commands branch from 584d76e to 0342aff Compare July 20, 2026 20:28

@thomwebb thomwebb left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_settings

Verified 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant