feat(i18n): extract session_commands.py strings#655
Conversation
thomwebb
left a comment
There was a problem hiding this comment.
Adversarial Review — Automated (code-puppy-4aeaa3)
Verdict: CONDITIONAL PASS — 2 blockers, headline claim is incorrect
The "33 raw → 0" claim is false. At minimum 3 raw user-facing strings survive.
Blockers
1. dump_context success message is still raw
session_commands.py calls persist_named_session(..., success_message_template=("Context saved: ...")). That template string goes directly to emit_success(success_message_template.format(...)) inside session_lifecycle.py — it is a real user-facing emit, it is a raw English string, and it is not in the catalog.
The cmd.dump_context.* namespace has 4 keys (usage, invalid_name, no_history, failed) and is conspicuously missing cmd.dump_context.success. The PR description confirms this omission: "dump_context: usage, invalid-name, no-history, failed".
Fix: add cmd.dump_context.success with params {count}, {tokens}, {pickle_path}, {metadata_path}, and either pass a pre-rendered catalog string into the template, or refactor persist_named_session to accept a catalog key.
2. Two raw English strings smuggled through {strategy_info} in cmd.compact.success
strategy_info = (
f"using {compaction_strategy} strategy" # raw English
if compaction_strategy == "truncation"
else "via summarization" # raw English
)
emit_success(t("cmd.compact.success", ..., strategy_info=strategy_info, ...))Both branches produce untranslated English injected as an opaque blob into {strategy_info}. A translator localizing cmd.compact.success has no way to control these sub-phrases.
Fix: add cmd.compact.strategy.truncation and cmd.compact.strategy.summarization keys, and pass strategy_info=t(strategy_key).
Should Fix
3. repr(session_name) pre-baked before t() — locale quoting impossible
Line ~384: t("cmd.dump_context.invalid_name", name=repr(session_name)) — the {name} slot receives an already-repr'd string. German uses „…", French «…», Japanese 「…」; translators cannot adjust. Also, the test passes name="'bad'" (already pre-quoted) rather than exercising the actual repr() call path. Pass name=session_name raw; handle display quoting at the catalog layer or document why Python-style quotes are intentional.
4. Pre-formatted numbers block locale number rendering
f"{tokens:,}" / f"{pct:.1f}" are pre-baked to English formatting (comma-thousands, dot-decimal) before reaching t(). A de-DE locale expects 10.000 and 75,1. If this tradeoff is intentional, add an explicit code comment so future i18n infra work knows to fix it.
5. Near-duplicate keys with cli.resume.* from cli_runner.py
cmd.quick_resume.searching and cli.resume.quick_searching are near-identical (same prose, same {scope} param, differ only in emoji prefix). Same for cmd.quick_resume.no_session vs cli.resume.none_found ("staying in current session" vs "starting fresh" — intentionally different?). Audit whether these are truly distinct semantic paths or should be unified. Add a comment either way.
6. Rich markup in catalog value
cmd.session.info contains [bold magenta]Autosave Session[/bold magenta]. In headless/CI/pipe output, the user sees raw Rich tags. Pre-existing pattern in the codebase, but baking it into the catalog makes it worse — translators must now know Rich syntax. Document the constraint.
Nits
emit_warning(t("cmd.clear.cleared"))— clearing history is a success event; should beemit_success. Pre-existing bug now enshrined in a catalog key.- Test threshold
>= 28should be>= 33. cmd.load_context.success— 3 visual lines, 5 params, parenthetical explanation — consider splitting for translator flexibility (though acceptable as-is for now).
The 30 correctly extracted sites are clean. test_no_leftover_placeholders is genuinely thorough. The _interpolate design's forgiving behavior (unknown placeholder stays intact) means catalog-kwarg mismatches are survivable at runtime. Fix B1 and B2 and this is close to shippable.
thomwebb
left a comment
There was a problem hiding this comment.
Review feedback addressed — commit fe7a44fc
Automated verification pass (code-puppy-4aeaa3). Both blockers were already fixed in the most recent commit before my review pass — this comment confirms the state.
Verified landed:
- Blocker #1 —
cmd.dump_context.successkey exists inen-US.json(line 143) with placeholders{message_count},{total_tokens},{pickle_path},{metadata_path}. Handler passessuccess_message_template=t("cmd.dump_context.success")intopersist_named_session; catalog-sourced template renders through the existing.format(...)insession_lifecycle.py. No refactor needed. - Blocker #2 —
cmd.compact.strategy.truncationandcmd.compact.strategy.summarizationboth exist. Compact handler usest(strategy_key)— no raw-Englishstrategy_infoblob left. - Test floor — assertion is
>= 35(stricter than my suggested>= 33); new interpolation asserts cover all three added keys.
Verification:
pytest tests/i18n/test_session_commands_i18n.py -q→ 12 passedpytest -k "session or compact or dump_context" --timeout=30→ 599 passed, 6 skipped- Catalog JSON parses; 141 total keys
Deferred (tracked as follow-up, not blocking this PR):
repr(session_name)pre-baked beforet()— locale-specific quoting impossible- Pre-formatted numbers (
f"{tokens:,}",f"{pct:.1f}") block locale number rendering - Near-duplicate keys
cmd.quick_resume.*vscli.resume.*— audit whether truly distinct semantic paths - Rich markup
[bold magenta]incmd.session.infocatalog value emit_warning(t("cmd.clear.cleared"))should beemit_success(pre-existing bug now in catalog)
Ready to merge from my side.
33 raw sites -> 0. 33 new catalog keys across 7 commands.
cmd.session.* — /session info, new, usage
cmd.clear.* — history cleared, agent notice, session rotated, clipboard cleared
cmd.compact.* — queued, no-history, compacting, failed, success (pre-formatted
:,/:. values passed as strings), /compact error
cmd.truncate.* — usage, must-be-positive, invalid-int, no-history,
already-short, success
cmd.quick_resume.* — searching, no-session, file-not-found, failed, success
cmd.dump_context.* — usage, invalid-name, no-history, failed
cmd.load_context.* — usage, not-found, available, failed, success
The load_context success message contains the full multi-line context detail
(message count, tokens, path, new autosave ID, preservation note).
The compact success message pre-formats numbers (f'{tokens:,}', f'{pct:.1f}')
before passing to t() so translators never see Python format specs.
Tests: tests/i18n/test_session_commands_i18n.py (12 tests).
All i18n tests pass. ruff clean.
- Add cmd.dump_context.success catalog key; replace raw emoji template
string in success_message_template= with t('cmd.dump_context.success')
- Add cmd.compact.strategy.truncation and cmd.compact.strategy.summarization
keys; replace raw strategy_info f-string/literal with t() calls
- Tighten test floor from >= 28 to >= 35 (33 original + 3 new keys)
- Add interpolation assertions for the three new keys
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.
fe7a44f to
1af995f
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.
What
Migrates 33 user-facing emit_ strings* from session_commands.py to the t() catalog.
33 raw -> 0.
33 new catalog keys across 7 commands
Design notes
Tests
tests/i18n/test_session_commands_i18n.py (12 tests). All i18n tests pass. ruff clean.