refactor(cli): revert dynamic slash command LLM translation#4145
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Removes the runtime LLM-translation path for dynamic slash command descriptions added in #3871, along with its `general.dynamicCommandTranslation` setting and the `/language translate` subcommand tree. Keeps the built-in locale coverage from the same PR untouched. Localization of dynamic command descriptions should be solved at the source (manifest fields, not runtime model calls); see #4137 for the proposed alternative.
Follow-up to the dynamic command translation revert: the 7 prompt keys were stripped from every locale file in the previous commit, but the allow-list in mustTranslateKeys still demanded them.
Follow-up cleanup after the dynamic command translation revert. CommandService.fromCommands was introduced by #3871 solely to wrap the LLM-translated command list. With the LLM-translation path gone, it has no remaining non-test callers — remove it and the matching test mock. Also drop two assertions in languageCommand.test.ts that checked for the absence of a top-level /language cache command. They tested a migration state that never existed in this branch and now pass vacuously.
Two user-facing docs documented the /language translate subcommands (status/on/off/cache refresh/clear) that were removed in the dynamic command translation revert. Strip them so users following the docs don't hit "Invalid command" errors.
tanzhenxin
force-pushed
the
revert/dynamic-command-translation
branch
from
2a817b5 to
1eeafd1
Compare
wenshao
reviewed
May 14, 2026
wenshao
reviewed
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] localizeDescription field is now dead code after this revert — the only reader (DynamicCommandLocalizationService) has been removed, but the field definition and 5 setters remain as orphans:
| File | Line |
|---|---|
packages/cli/src/ui/commands/types.ts (field + JSDoc) |
377-381 |
packages/cli/src/services/BundledSkillLoader.ts |
67 |
packages/cli/src/services/command-factory.ts |
118 |
packages/cli/src/services/McpPromptLoader.ts |
51, 60 |
packages/cli/src/services/SkillCommandLoader.ts |
83 |
The modelDescription JSDoc (line 373-374) also still references "Dynamic command localization may rewrite description".
Suggested cleanup in a follow-up: remove the localizeDescription field and its JSDoc from types.ts, remove the localizeDescription: true assignments from all 5 loaders, and update the modelDescription JSDoc.
Nice to have — additional cleanup opportunities:
getLanguageNameForTranslationTarget()inpackages/cli/src/i18n/languages.ts:176is dead code (only called by the removed service)- Outdated comment in
packages/cli/src/ui/commands/languageCommand.ts:99: "Reload commands to update localized descriptions" — the "localized descriptions" part refers to the removed feature
Deterministic checks: tsc + eslint clean on changed files. All 275 targeted tests pass.
Verdict: Clean revert. The LLM-translation layer removal is surgical and complete — all 3 command-loading paths (interactive, non-interactive, ACP) are correctly updated, all i18n keys are cleaned, and the settings parameter removal from getAvailableCommands/buildSystemMessage is consistent across all callers. No functional issues.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Clean revert — the dynamic slash command LLM translation feature has been removed cleanly with no logic errors, security issues, or test failures. Build and tests all pass (294 tests, 0 failures).
A few minor cleanup items the revert left behind:
Suggestion: getLanguageNameForTranslationTarget is now dead code
File: packages/cli/src/i18n/languages.ts:176
The function getLanguageNameForTranslationTarget was only called by the now-removed DynamicCommandLocalizationService. It remains defined in languages.ts and re-exported from i18n/index.ts (lines 16, 30), but has zero callers in the worktree.
Fix:
// In languages.ts, remove the function:
// export function getLanguageNameForTranslationTarget(...)
// In i18n/index.ts, remove the import and re-export
Nice to have: modelDescription JSDoc references removed feature
File: packages/cli/src/ui/commands/types.ts:374-377
The JSDoc on modelDescription says "Dynamic command localization may rewrite description for UI display while keeping this value stable" — but dynamic command localization no longer exists. This comment is now misleading.
Suggestion: Orphaned translation cache file
The DynamicCommandLocalizationService wrote to ~/.qwen/dynamic-command-translations.json. After this revert, no code reads or cleans up this file, leaving it as orphaned data on user systems. Consider adding a one-time startup cleanup in a follow-up.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
The DynamicCommandLocalizationService that read this flag was removed in the revert, leaving the field with five setters and zero readers. Drop the field, its JSDoc, and the five `localizeDescription: true` assignments. Also tidy the now-misleading `modelDescription` JSDoc and the stale `reloadCommands` comment that referenced the removed feature.
The only caller was the removed DynamicCommandLocalizationService. Remove the function from `i18n/languages.ts` and the matching import + re-export from `i18n/index.ts`.
Collaborator
Author
|
Re: the round-2 review — |
Collaborator
Author
|
Re: the review summary —
|
wenshao
approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
previously approved these changes
May 15, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Clean surgical revert — all 2010 removed lines are consistent, no remaining references to deleted symbols (DynamicCommandLocalizationService, localizeDescription, getLanguageNameForTranslationTarget, fromCommands), all test assertions and mocks are correctly aligned with new code paths, and CI is all green (9/9 checks passing).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…d-translation # Conflicts: # packages/cli/src/i18n/locales/zh-TW.js
wenshao
approved these changes
May 15, 2026
TaimoorSiddiquiOfficial
added a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
) Remove DynamicCommandLocalizationService and dynamicCommandTranslation setting. Apply HopCode branding fixes (hopcode-core import, hopcode serve references). Cherry-picked from upstream f6315b3 with conflict resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TaimoorSiddiquiOfficial
added a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
…teKeys directly After removing MUST_TRANSLATE_KEYS from i18n/index.ts exports (upstream revert of QwenLM#4145 pattern), update test to import from source module directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
3 participants
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
general.dynamicCommandTranslationsetting and the/language translatesubcommand tree. The built-in i18n coverage from the same PR is preserved in full.Rationale
The feature added in #3871 makes a translation model call to localize dynamic command descriptions (skills, file commands, MCP prompts) at startup, language switch, and command-set reload. The concerns, also captured in #4137:
/my-mcp-prompt), so translating only the description leaves the menu mixed-language anyway.The underlying problem (dynamic command descriptions are English while built-ins are localized) is real. A reasonable shape for a future solution is locale-keyed description fields at the source — declared in the MCP prompt manifest or skill frontmatter, with fallback to the default. No runtime model call, deterministic output, translator-controlled quality. That's filed as the proposed follow-up in #4137.
Validation
npm run build npm run typecheck # plus targeted vitest runs on every affected test file/language translate ...invocations return "Invalid command".npm run typecheck && cd packages/cli && npx vitest run src/ui/commands/languageCommand.test.ts src/ui/hooks/slashCommandProcessor.test.ts src/nonInteractiveCliCommands.test.ts src/nonInteractiveCli.test.ts src/utils/nonInteractiveHelpers.test.ts src/acp-integration/session/Session.test.ts src/i18n/mustTranslateKeys.test.ts.Scope / Risk
general.dynamicCommandTranslationsince feat(cli): core built-in i18n coverage #3871 landed will see their setting become a silent no-op. Default-off + ~1 week since merge means very low blast radius./languageflows were not exercised against a running CLI.general.dynamicCommandTranslationsetting and the/language translatesubcommand tree are removed; no migration is provided (the cached translation file under~/.qwen-code/is orphaned but harmless).Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Closes #4137
References #3871 (the PR being partially reverted)