fix(i18n): code-ize health signals and drop silent zh locale defaults - #4524
Conversation
6adeb74 to
51661cb
Compare
646c302 to
6ddc0ac
Compare
22879d0 to
cfe5ba2
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The shape is right. The producer emits codes, the existing UiCatalog maps them, Record<HealthSignalMessageCode, string> makes a missing entry a compile error, and the never checks pin detail exhaustiveness. The reverse-mapping it deletes was genuinely fragile: matching signal.message === '不是工作区的默认模型来源。' meant any wording edit in health.ts silently broke English. And I checked the "code without copy leaves a blank" worry, it is not reachable, both maps are total by type and nothing persists a HealthSignal.
[P2] The zh health center now prints raw English bot diagnostics
Two lines apart in the same switch:
// signalDetailZh
return capabilityReasonsZh[detail.reason] ?? detail.reason;
// signalDetailEn
return capabilityReasonsEn[detail.reason] ?? 'See the corresponding settings page for details.';The zh side passes the unknown reason through verbatim. Bot reasons are English: settings-ipc-helpers.ts:235 produces ${label} requires a Bot Token. and ${label} rejected the Bot Token., which reach capabilityDetail unchanged through readinessReason and runtimeProbeFromBotReadiness. So configure a bot with a bad token, test it, open Settings and Health in Chinese, and an English sentence is what you get.
On main the zh side had a fallback for exactly this: /[㐀-鿿]/.test(raw) ? raw : '状态详情请见对应设置页。'. So a PR whose purpose is removing locale leakage introduces the mirror leak. The comment above that line says unknown reasons "are today's zh snapshot diagnostics", and the new test only pins the zh-text case, which is why it went unnoticed.
Fix is to mirror the en branch, plus a test with an English unknown reason.
[P2] last_test_message puts a machine token on the user-visible surface
runtime-host-connections-ipc-main.ts:429 sends lastTestMessage: connection.lastTest.errorClass, and errorClass is the closed six-value union in connection-effects.ts:69. So the string the user sees is literally auth. The type comments call it raw producer text rendered verbatim by design, but it is an enum, which is the thing this PR code-izes everywhere else. zh is unchanged from main; en regresses from the fallback sentence to the bare token. There is already a seam for this exact field, connectionLastTestMessageDisplay(message, locale) in provider-panel-shared.ts:94.
[P3] Three smaller ones
capabilityReasonsZh and capabilityReasonsEn carry four keys each (disabled, missing platform credentials, macOS TCC only, and the Apple Events one) that capability-snapshot.ts never emits. Main had four dead mappings in the producer; this moves them faithfully and doubles them.
permission-center-page.tsx:705 still uses the /[㐀-鿿]/u CJK sniff on the same capability.*.reason field this PR just stopped sniffing. Not a regression, both predate this, but it means an English user sees the bot's English message in the Permission Center and the fallback sentence in Health, for one field. Worth naming as a known remainder given the PR's stated obligation.
settings-pages.stories.tsx:600 maps a storage-scope signal to capability_ok, so the story renders capability copy for a SQLite row. Root cause is that HealthSignalMessageCode has no code for the app and storage scopes those fixtures use, and no producer emits them.
Next step
Both P2s are one catalog entry each. P3-1 is a free deletion in the same file.
This is a copy change with no screenshots, so it needs manual acceptance in Settings and Health, both locales: a connection in each state, the runtime row with a successful and a failed send, the runtime label suffix that moved from producer to presenter, and a bot capability after a failed credential test, which is the P2 reproduction.
Evidence boundary: read at cfe5ba2 against base b1ec289, no tests, typecheck or build, and the app was not launched. Both P2s are traced through source but not observed at runtime. The claim that no production caller relied on the removed zh defaults rests on a repo-wide grep plus the compiler, not a typecheck run.
AI-assisted review: drafted with Maka.
b40cbd4 to
297488f
Compare
|
@Astro-Han Comments are addressed. |
874ac68 to
447db95
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The P2 is closed, and closed harder than I asked for. Neither presenter passes raw producer text through any more: capability_reason and last_test_message both return a fixed, localized sentence, so the whole class of "English bot diagnostic surfaces in the Chinese Health center" is gone rather than patched at the one call site I found. The exhaustive switch with no default is what makes that hold — a new detail kind is a compile error in both locales.
CI is unrelated: the failure is host-kernel.test.js:613 (elects one owner ... releases ownership after true-idle shutdown, got unavailable where it wanted connected), which no i18n change can reach. I've rerun it.
P2 — two detail fields are carried but never read, and the comment promises otherwise.
// Raw machine token from the capability snapshot; the presenter maps the
// known vocabulary per locale and falls back to a generic hint.
| { kind: 'capability_reason'; reason: string }
// Legacy connection-test diagnostic; the presenter uses locale-specific fallback copy.
| { kind: 'last_test_message'; text: string }Neither reason nor text has a reader. signalDetailZh and signalDetailEn both return the generic sentence unconditionally for those two kinds — the per-locale vocabulary map the first comment describes doesn't exist any more. So the producer computes a value, it crosses the IPC boundary, and nothing consumes it.
Two coherent ways out, and I don't mind which:
- Implement what the comment says for
capability_reason.reasonis a machine token, not free text, which is exactly the shape a per-locale map wants — and it recovers the specific wording that the generic hint currently costs. - Or drop both fields. If the decision is that a generic pointer to the settings page is the right detail, then the payload should say so.
textin particular is free-form English from the producer; keeping it around is an invitation to render it later and reintroduce the leak this PR just removed.
Either way the comments need to match. Right now the type documents behaviour the presenters don't have, which is the same failure mode as the string matching you deleted — a promise held in prose instead of in types.
Everything else stands: Record<HealthSignalMessageCode, string> making a missing entry a compile error is the right shape, and the reverse-mapping it replaces was genuinely fragile.
17b73e9 to
06b1f63
Compare
|
Addressed by dropping the unused |
8883ac8 to
1acd169
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 1acd169. Last round's P2 is closed as asked and the regressions from round one stay closed: no presenter passes producer text through, the auth token is mapped on both surfaces, the four dead capability mappings went with userVisibleCapabilityReason, and the app / storage fixtures were removed because nothing produces them (I checked; health.ts only emits llm_connection and capability). Three things this round, and the first one is partly on me.
P2: Chinese users lose the capability diagnostics they see today. I offered "drop both fields" as one of two outs last round; for last_test_message that was right, for capability_reason it was not. The reasons in capability-snapshot.ts:83, 93, 103, 111, 246, 349 are live Chinese product copy (未配置平台凭据, 本地 MEMORY.md 已可见;自动抽取/写入仍需用户确认, …), and main shows them verbatim in the zh Health center. After this PR every one becomes 状态详情请见对应设置页。, while the Permission Center (permission-center-page.tsx:707) keeps showing the specific sentence for the same fact. Either code these reasons too (CapabilityReasonCode, same medicine as the rest of the PR, producer side in capability-snapshot.ts), or keep the reason payload and let the zh presenter pass CJK through as main does, labeled as the interim state. Whichever you pick, the body needs to name this as a user-visible change.
P2: the never checks are gone. The previous head had exhaustiveness guards on the three signalDetail* switches; this one has none, the switches have no default, the return type is string | undefined, and nothing enables noImplicitReturns. A new HealthSignalDetail kind now compiles and silently drops the detail in all three locales, which is the failure mode the Record<HealthSignalMessageCode, string> side was built to prevent. Three default: { const _: never = detail; return undefined; }.
P2: runtime probe error classes are looked up in the wrong vocabulary. localizedRuntimeErrorClass indexes connectionTestErrorMessages (auth | timeout | provider_unavailable | network | unknown), but the probe's errorClass is the turn's failureClass, i.e. ModelFailureKind (model-protocol.ts:331: rate_limit, context_overflow, provider_capacity, provider_billing, abort, …). Everything outside the five overlapping names falls to ?? unknownRuntimeErrorClass, so a rate-limited send that main reports as 错误类型=rate_limit now reads 错误类型=未知错误, and the new future_runtime_error → 未知错误 test pins the wrong side. Fall back to the raw token on a miss, or key a runtime table on ModelFailureKind.
Smaller:
HealthConnectionTestErrorClassduplicatesConnectionTestErrorClass(llm-connections.ts:520) and addsinvalid_response, which the wire codec rejects (connection-catalog-codec.ts:707-719), so its seven copy entries are unreachable and a new class in core will not be a compile error here. Alias the core type.- Six of the eight
HealthSignalDetailvariants carry no data and four of them map 1:1 to a message code; the presenter can derive those from the code, leavingruntime_probe_resultandlast_test_error_class. - The 15-line
applyLiveTurnEventwrapper is pasted into three test files, plus three stream wrappers, all to avoid writing'zh-CN'at the call site; the diff also leaves runs of 3–4 blank lines in five test files. - With
app/storagefixtures gone,HealthSignalScope's'app' | 'storage'and the three-localescopes.app / scopes.storage / sources.storageentries are dead too.
Ordering: provider-panel-shared.ts is touched here in the body right under the signature line #4640 changes, so one of the two rebases; #4551 edits the same file. materialize.test.ts and live-turn-projection.test.ts merge mechanically with #4640 but the second one to land should re-run packages/ui. Against #4817 this PR only lowers counts.
Manual acceptance still owed (body says the app was not launched), three locales: Health center rows for configuration / validation / runtime_probe, a failed send's runtime detail with the error-class segment, a capability signal with a reason (the first P2), the connection detail lastTest row, and the runtime label suffix that moved to the presenter.
Evidence boundary: static read at 1acd169 against cbeb1a93c7; vocabularies traced through source, not observed at runtime; no tests, no typecheck.
AI-assisted review: drafted with Maka; I verified the capability reasons on main, the two error-class unions, and the missing never guards myself.
1acd169 to
03681a0
Compare
94ddb8f to
f5c725f
Compare
Health signals carried zh product copy from the producer; the en presenter reverse-mapped it by exact string match, so any wording edit silently broke English rendering. Signals now carry stable message codes and structured details, both locales map codes in the presenter catalog, and raw capability or connection diagnostics no longer leak machine or wrong-locale text into Settings surfaces. Capability reasons are still zh prose from the snapshot, so the zh presenter keeps showing them until they are coded. The ui stream/projection seams lose their silent 'zh' locale defaults so callers must thread the real locale. Generated-by: Claude Code Generated-by: OpenCode Claude-Session: https://claude.ai/code/session_01AqdSkg56F2x55wEGRWvzcB
f5c725f to
2f75546
Compare
|
@Astro-Han RFR, after this get merged, I will rebase and reopen draft PR #4824 #4641 and #4551 |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 2f75546d5e8b4a95f8a8b9f541901fd6e6932b81 against base 1e543a7385614adc671623efe2586cf5317582d4.
No P0-P3 findings.
The change now has one typed health-copy boundary: HealthSignalMessageCode and HealthSignalDetail replace producer-owned prose (packages/core/src/health.ts:52-92), while the three locale catalogs own the rendered messages and structured details (apps/desktop/src/renderer/locales/settings-health-copy.ts:172-355). I also traced the live producers and callers: Runtime Host connection tests project the closed error-class vocabulary at apps/desktop/src/main/runtime-host-connections-ipc-main.ts:422-430, health aggregation consumes those projections at apps/desktop/src/main/runtime-host-permissions-ipc-main.ts:101-121, and the removed stream/projection locale defaults have explicit production callers, including apps/desktop/src/renderer/app-shell-session-events.ts:146-152 and packages/ui/src/chat-view.tsx:767-772.
The prior review items are closed on this head:
- zh-CN keeps current CJK capability detail while English and Traditional Chinese use fixed locale copy (
settings-health-copy.ts:256-338; covered athealth-center-copy.test.ts:99-116). - all three detail presenters restore the
neverexhaustiveness guard (settings-health-copy.ts:281-282,309-310,337-338). - runtime probe classes outside the connection-test vocabulary stay visible as their raw diagnostic token instead of becoming “unknown” (
settings-health-copy.ts:342-355; covered athealth-center-copy.test.ts:77-97). - connection-test classes alias the core union and are localized on both the Health and connection-detail surfaces (
health.ts:71-81,407-418;provider-panel-shared.ts:85-93).
Verification on this exact head:
- Hosted
test: success, run33954160323. build:test, full workspace typecheck, renderer production build, and Storybook build: passed.@maka/uitests: 373 passed;@maka/coretests: 825 passed; focused changed-path tests: 39 passed.- format, ASF headers, AppShell hook scope, Desktop knip, renderer architecture, and
git diff --check: passed. - Fresh
origin/main(3fdaddb4b9a8383a0612c55bf28f7a0e5c624ea0) merges cleanly; the 16 post-base main files do not overlap this PR's 25 changed files.
Evidence boundary: I did not launch the interactive Desktop app, so the three-locale visual/manual acceptance described in the PR remains unobserved. The local full Desktop suite reports 2,161 passed, 0 failed, and 8 cancelled in the unchanged mcp-oauth-controller.test deadline group; I reproduced that with Node 22.22.1 and 22.22.2. The exact-head hosted check is green, and the changed-path focused suite has no cancellations.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 2f75546. All three P2s from last round are closed, and the smaller items with them:
- Capability reasons:
reasonis back on the detail, the zh-CN presenter passes CJK prose through asmaindoes, en and zh-TW keep the fixed pointer (which is whatmain's zh-TW presenter already did atsettings-health-copy.ts:140), the sniff is labeled interim with theCapabilityReasonCodefollow-up named, and the body says so. Covered both ways inhealth-center-copy.test.ts. neverguards: all three switches end indefault: return unhandledDetail(detail)with_detail: never.- Runtime error classes: unmapped
failureClassvalues fall back to the raw token; therate_limitfixture now pins the right side. HealthConnectionTestErrorClassaliasesConnectionTestErrorClass, the unreachableinvalid_responseentries are gone, the deadapp/storagescope and source entries are gone, and the three pasted test wrappers collapsed into one sharedlive-turn-zh.ts.
Checked against #4817's ratchet from its current head: this branch passes, and it retires the two packages/ui silent-locale-default hits it says it retires. Merges clean against main and against #4526 at b01cb0c (the two shared files, settings-provider-copy.ts and the zh-TW peer-mesh test, do not collide).
One body nit, not blocking: the checklist lost its second line and the "change in behavior" question; the answer is yes, for the en Health center's capability rows, which the Interim paragraph already describes.
Manual acceptance is still owed by whoever lands it (body says the app was not launched): Health center rows in the three locales, a failed send's runtime detail with an unmapped class, and a capability signal with a reason in zh-CN vs en.
Evidence boundary: static read of the delta since 1acd169 plus one run of the #4817 script on this head; no build, no suites, no Desktop.
AI-assisted review: drafted with Maka; I verified the three presenters, the error-class fallback and the ratchet run myself.
Adding a UI locale compiled cleanly and still rendered the wrong language wherever code compared `locale` to a literal, defaulted a `locale: UiLocale` parameter, sniffed CJK in a payload, or translated by looking up one locale's string. The type system cannot see any of the four, and Biome cannot express "no growth against base" for the two unformatted trees. `scripts/check-locale-hygiene.mjs` counts those four patterns per file in `apps/desktop/src`, `packages/core/src` and `packages/ui/src`, both quote styles, per match, and fails CI when any (file, rule) count grows against the merge base or `BASE_SHA`. There is no ledger to maintain; only files in the diff are scanned. With no resolvable base the step fails instead of skipping. The step runs with the install-free gates before `setup-node`. `locale-literal-compare` is deliberately narrower than `check-tui-copy`'s AST rule so it stays dependency-free. No runtime or migration impact. Baseline on this commit: 67 / 4 / 12 / 32; the four `silent-locale-default` hits retire with #4524 and #4551, after which that rule can become a hard zero. Refs #2672 Generated-by: Claude Code
Summary
Health signals emitted zh product copy from
packages/core/src/health.ts, and the English presenter reverse-mapped it by exact string match; any wording edit silently broke English, and the runtime-probe detail was regex-parsed back out of a formatted zh string.Signals now carry a closed
HealthSignalMessageCodeplus structuredHealthSignalDetail; each locale maps codes in its catalog and the producer is locale-free. Connection-test failure classes are a closed detail mapped on both the Health and connection-detail surfaces. Runtime-probe error classes use the turn's failure vocabulary (rate_limit, …), so unmapped classes stay visible as their token instead of collapsing to "unknown error".Interim: capability reasons are still zh prose from
capability-snapshot.ts, so the zh-CN presenter shows them asmaindoes while en / zh-TW show a fixed settings-page pointer. Coding them as aCapabilityReasonCodeis the follow-up that removes this sniff and the Permission Center's.The ui stream/projection seams also lose their silent
'zh'locale defaults; every production caller already passes the real locale, so tests pin zh via a shared wrapper.Sequencing: follows #4823 and #4640 (merged, in this base). #4824 and #4551 rebase onto this; #4551 touches the same
health.tsandprovider-panel-shared.ts.Refs #2672
Verification
The app was not launched for manual acceptance.
AI use
Select exactly one:
Tool(s) and scope: Claude Code — analysis, implementation, tests. OpenCode — review follow-up and tests. Each commit carries its
Generated-bytrailer.Checklist