Skip to content

fix(desktop): localize expected errors - #4457

Open
orangeCatDeveloper wants to merge 2 commits into
apache:mainfrom
orangeCatDeveloper:fix/desktop-error-codes
Open

fix(desktop): localize expected errors#4457
orangeCatDeveloper wants to merge 2 commits into
apache:mainfrom
orangeCatDeveloper:fix/desktop-error-codes

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Desktop surfaces rendered raw exception text as user-facing copy: a zh user hitting an expected failure saw an untranslated English error.message, and unexpected exceptions could leak internal details into toasts and error banners. Producers (guest-session mounts, Work Board IPC, WorkHub controller, the composer attachment pipeline) now return machine-readable codes — { code, params } where a message needs data — and the renderer maps codes through the locale catalogs. Expected failures throw a typed ExpectedOperationError; everything else logs a redacted diagnostic and shows a localized fallback. This also deletes WorkHub's English-message-regex classifier and the MAKA_SESSION_READ_MESSAGES_ERROR marker protocol.

Per review, the global error mapper is split: each domain maps its own code union at its presenter; the ExpectedOperationError transport and redacted diagnostics live once in the renderer's application/contracts/operation-diagnostics entry, now delegating to reportUnexpectedOperation/unexpectedOperationFallback in @maka/core/redaction so copy catalogs (bare-import-only) and the renderer share one diagnostics channel. They are Desktop-only, and hosting them in @maka/ui pulled the icon bundle into the main-process test bundles. The attachment validations (15 in the main-side ingest resolver plus 3 in the preload payload cap and 3 in the renderer preflight) reject with stable attachment_ingest:<code> tokens that survive the Electron IPC wrapper, mapped at the shared localizedShellErrorMessage entry — both locales previously lost these reasons to a generic fallback, and the total_size_exceeded budget rejections introduced by #4956 are tokenized in this branch too. The Runtime's 8 session-control guards emit session_control_blocked:<code> tokens; today no in-repo caller reaches those guards (Desktop changes session settings through session.configuration.update, whose failures already carry protocol codes, and the CLI drives SessionManager only through the Runtime Host protocol), so the tokens stay as a typed, test-pinned contract for the future CLI-local path. A structured {ok, code} envelope for configuration setters and attachment failures is the follow-up.

The WorkHub waiting summary is a complete catalog message in each locale, independent of the separately displayed status and retry paragraphs. This preserves existing output while letting future translations choose their own sentence structure.

Refs #2672

Verification

Before (zh UI, stale Work Board write / import over mount limit — raw English internals):

Work Board item wb-01 revision changed from 1 to 3
At most 12 shared Sessions can be retained

After (same failures; raw detail now only in the redacted console diagnostic):

工作板内容已更新,请刷新后重试。
共享 Session 数量已达上限。
apps/desktop full dist suite:   2228 pass / 0 fail
packages/core tests:            829 pass / 0 fail
packages/runtime tests:         3246 pass (5 sandbox-only filesystem-worker failures, unrelated)
typecheck (4 tsconfigs):        0 errors
renderer architecture check:    passes under the #4581 checker
repo format (biome):            clean on all touched files

Rebase/review follow-up (2a7c11d6a) re-verified all of the above after moving the redacted unexpected-failure diagnostics into @maka/core/redaction (reportUnexpectedOperation / unexpectedOperationFallback) so the copy catalog and the renderer entry share one channel; operation-diagnostics keeps its public surface for its seven renderer importers and delegates to the core helper. No behavior change.

Review follow-up (575f10b39 + fe1190919): folded the duplicate attachmentIngestBlocked imports; extracted workBoardActionErrorText in work-board-panel.tsx (exported test seam) so the presentation test drives the panel's real code-to-copy branch instead of re-implementing it — a panel mis-map now fails the test. The copy catalog stays free of transport imports (copy catalogs may only hold bare package runtime imports), and the expected-code lookup lives in the catalog as workBoardErrorCodeCopy so the panel adds no dependency-debt edge. Re-verified against the PR base: desktop 2228/0, four typechecks 0 errors, renderer architecture ratchet (--base 411512bd9), biome clean.

Review follow-up round 1 (16a2fe84f) and round 2 (e0ea0c3e3), addressing the two review comments:

  • P3 (shell-copy): the attachment_ingest:<code> matcher is anchored at the message tail (bare token or the IPC-wrapped error line end), so an unrelated message that merely contains the substring — e.g. a path like /tmp/attachment_ingest:count_limit/report.txt — keeps its fallback and the unexpected-diagnostics path. Negative tests pin both the anchor and the prototype-pollution cases.
  • P2 (plan-mode-panel), envelope round: the second review confirmed via a real Electron probe that the IPC boundary strips custom fields from thrown Errors, so the renderer can never see RuntimeHostOperationError — the previous instanceof fix was unreachable in production. The five plan-mode control channels (requestRevision, abandon, approve, resume, abandonExecution) now return a structured PlanControlIpcResult envelope (mirroring the WorkBoardIpcResult precedent): main catches the typed rejection and returns {ok:false, error:{code,message}}; the panel checks the envelope and maps error.code through planModeCopy.controlFailure (all three locales), with unknown/future codes falling back to the generic line. abandonPlanProposal keeps its throwing shape (the app-shell legacy closure is token-frozen; its generic failure copy flows through sessionSettingFailureCopy). The envelope type lives in plan-mode-copy.ts (bare imports exempt from the debt ratchet) and shared/plan-mode-ipc.ts (main/preload) — its final placement is round 6 below. RuntimeHostOperationError stays in the protocol package under the declared compatible extension (epoch synced to main's current value in round 6). The panel seam test now drives planControlFailureCopy with the wire envelope shape.

Re-verified after the envelope round: desktop 2230/0 (presentation tests drive the wire envelope: expected codes per locale, success envelopes, unknown-code fallback), core 829/0, four typechecks 0 errors, renderer architecture ratchet against the PR base, biome clean, epoch guard passes. The one runtime-host failure (execution-model-composition sandbox ENOENT under macOS /var/folders) reproduces on the unmodified base and is unrelated to this diff.

Review follow-up round 3 (67072ee53): the envelope's catch blocks converted an unrecognized rejection (network fault, programming error, non-plan.* operation) into fall-through success — emitModeChanged fired and the renderer got ok:true for a failed action. Every plan-mode handler now rethrows when planControlIpcFailure produces no envelope, and a new test drives all five channels with a non-plan control failure asserting the exact cause propagates. Re-verified: desktop 2231/0, typecheck clean, architecture ratchet and biome pass.

Review follow-up round 4 (4624383de, rebased onto main c0229b0c6): the cleanup in 0f25f62cf had dropped setPending(true) from the plan panel's run(), so the four controls gated on planMode.pending stayed enabled while an action was in flight. Restored, with a hook-level regression that mounts usePlanModeState through the real providers, holds one bridge promise open with deferred, and asserts pending stays true until it settles (fails without the fix: pending must hold while the bridge call is unsettled). Two P3s along with it: planControlFailureCopy takes the envelope error directly (the ok branch was unreachable), and localizedShellErrorMessage records the unexpected diagnostic only when the generalized classifier has no category, so a timeout or 429 no longer logs an "operation failed" stack. Re-verified: targeted dist tests 10/0, four typechecks clean except stories/app-shell.stories.tsx (rightCollapsed, identical to main), renderer architecture ratchet against c0229b0c6, biome clean, epoch guard passes.

Earlier follow-up verification still holds: all 122 WorkHub tests pass, including three-locale summary assertions that remain valid when the separate paragraph copy changes; all 101 architecture-checker tests pass. The draft blocker (#4493 checker policy) landed on 2026-09-02.

Review follow-up round 5 (9e1759de0, on the branch rebased onto main): the new-task send path ran the renderer-side preflightAttachmentItems before newTasks.create, so the main-side token validation never saw an over-limit request — and the preflight threw localized prose the token-only presenter cannot recognize, so a nine-attachment send got the generic "try again later" fallback in both locales (retrying the unchanged input can never succeed) plus a spurious unexpected diagnostic. The preflight now rejects with the same stable attachment_ingest:<code> tokens main's resolveIngestItems rejects with (count_limit / item_too_large / duplicate_source, same semantics as the mirrored main checks), so localizedShellErrorMessage maps the real reason with no new code path; renderer-local throws never cross the IPC boundary, so the end-anchored tail matcher sees the bare token. The quote-companion send catch — which rendered error.message directly and would have leaked the raw token — now routes through localizedShellErrorMessage. The conversation-copy attachments prose the preflight was the only consumer of is deleted. The regression drives the real createAppShellChatActions send path with nine staged attachments (send rejected, newTasks.create never called, toast carries the count_limit reason in zh-CN and en, console.error silent) and fails on the previous head; the rewritten preflight unit test pins the exact tokens (and the oversized-approval-by-pending-size case).

Review follow-up round 6 (folded into the first commit, same rebase): the plan-control envelope moved out of the Host protocol package — no wire codec uses it and only Desktop consumes it — into a type-only apps/desktop/src/shared/plan-mode-ipc.d.ts beside the WorkBoardIpcResult seam, following the session-collaboration.d.ts precedent so it stays outside the renderer architecture debt closure (a plain src/shared module imported from the app-shell closure would be a new legacyAppShellClosure debt entry, which the checker forbids). PlanControlErrorCode stays in the protocol next to the CONTROL_ERRORS list it aliases; the compatible-change declaration keeps covering exactly the real protocol changes (the error-class move, the code alias, the unauthorized declaration).

Review follow-up round 7 (rebase onto main 7779d69a7, which landed #4956 durable local delivery): the rebase resolution keeps main's new prepareIngestItems/commit(admit) structure with every prose rejection converted to a token — the pre-existing checks map to their existing codes, and #4956's new total-size budget rejections (5 sites across the main-side prepare/commit and the preload payload cap) map to a new total_size_exceeded code with copy in all three locales (the existing size_exceeded copy describes a single oversized item, which would misrender a total-budget breach). Main's new tests that asserted the prose (/过期|无效/, /附件总量超出大小限制/) now pin the tokens instead. The compatible-change declaration synced to epoch 124 (equal to main's). Branch history: a83b34bb4 (localization + envelope placement + #4956 reconciliation) + 9e1759de0 (attachment preflight tokens) on 7779d69a7.

Verified on the rebased head: desktop full dist suite 2357 pass / 0 fail, four typechecks 0 errors, renderer architecture ratchet against 7779d69a7 (no new debt entries; the ledger is consistent at each commit — checked out and re-verified the first commit separately), biome clean on all touched files, protocol diff vs main is exactly the two declared files at epoch 124.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — analysis, implementation, tests, and this description, under the contributor's direction; the commit carries a Generated-by: Claude Code trailer.

OpenCode implemented the complete waiting-summary catalog messages and added output tests; its follow-up commit carries Generated-by: OpenCode. Claude Code landed the rebase/review follow-up (2a7c11d6a: single diagnostics channel in @maka/core/redaction, operation-diagnostics delegating), verified the branch against current main, and updated this description.

pi (GLM) landed the attachment-preflight token round and the plan-control envelope ownership move (9e1759de0; the envelope move folded into a83b34bb4), rebased the branch onto current main with the epoch declaration reconciled to main's current value, and verified the full suite; those commits carry Generated-by: pi (GLM) trailers.

Checklist

  • Tests cover the change and fail without it

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

Converting to draft: blocked on the copy-gate/architecture-ratchet collision described in #2672 (comment)#4493 resolves the checker policy; this branch then rebases (and splits the global error mapper per review). Will mark ready once green.

@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 2 times, most recently from adf4f4a to 6b28e30 Compare September 2, 2026 08:40
Astro-Han pushed a commit that referenced this pull request Sep 2, 2026
…cture ratchet (#4493)

Two repository gates deadlocked. The locale policy (#2672) moves user-visible copy out of business files into src/renderer/locales/*-copy.ts catalogs, which adds an import edge; the renderer architecture ratchet forbids legacy files from growing their dependency count and rejects new AppShell-closure entries. #4457 failed CI on exactly that collision, and no placement of a catalog could satisfy both gates.

Admit one dependency class instead of loosening the ratchet: a validated copy catalog, recognized structurally and re-verified on every run, never grandfathered. It must live under src/renderer/locales/*-copy.ts, carry the UiCatalog marker from @maka/core/ui-locale, have zero tracked capabilities by the checker's own metrics, and import bare package specifiers only, so it can never become a tunnel to renderer implementation. Admitted edges are excluded from the dependency-count ratchets and closure admission, and the ledger drops 26 budget entries that only existed for catalog imports. Root-entry files get no discount, and a catalog that grows a hook or a relative import loses admission immediately.

The environment-capability predicate now counts identifiers only in value-reference positions, so a copy key named history or a parameter named location no longer reads as a browser global, and type-only imports and exports are not counted as runtime dependencies. Adversarial fixtures cover hook smuggling, implementation-import smuggling, a missing marker, dynamic imports and an unrelated dependency added beside a valid catalog.

Part of #2672.

Generated-by: Codex
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 4 times, most recently from 13b0d58 to ad06382 Compare September 2, 2026 18:13
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 3, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 12 times, most recently from 9050c08 to 47537fd Compare September 5, 2026 04:24

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 0f25f62cfca23484934b1509913464e19c0ddeb0. The expected-error refactor is mostly coherent, but the latest cleanup introduces one P2 regression in the Plan Mode action guard, so I do not recommend merging this head yet.

Local validation passed: clean install, build:test, focused presentation/Plan IPC tests (23/23), full Desktop tests (2247/2247), Desktop typecheck after clearing a stale incremental cache, renderer architecture (101/101 plus base-relative ratchet), Biome, format, locale hygiene, ASF headers, protocol epoch guard, diff check, and a clean merge-tree against current main (a5022e562). A real React hook probe also reproduced the finding: while a deferred plan-control request remained unsettled, pending rendered as false.

Both required hosted checks are currently red. test fails in the unchanged transcript-scroll E2E surface, and package terminates while the Windows verifier queries the packaged process; neither failure overlaps this commit's changed paths, but the required gates still need to return green. Native macOS/Windows behavior was not run locally.

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.

Comment thread apps/desktop/src/renderer/plan-mode-panel.tsx

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at 0f25f62 against main (a5a99a633). The increment over 9b2f54c is one commit of yours plus a rebase; the transcript files in the range are main drift from #4886, not your change.

Both P2s are fixed, and better than asked: PlanControlIpcResult now lives once in packages/runtime-host/src/protocol/plan.ts:75-89 and is re-exported from bridge-contract.d.ts:186, so shared/plan-mode-ipc.ts is gone and the duplicate-declaration P3 closes with it; the panel calls planControlFailureCopy(result, copy) (plan-mode-panel.tsx:102) and the test hits the same entry. The shell path now records the diagnostic and then classifies, unauthorized is in CONTROL_ERRORS with copy in three locales, and the epoch declaration is updated.

P1 (path ①): the plan panel lost setPending(true). main and 9b2f54c both open run with setPending(true) (main:89, 9b2f54c5:96); the head's run (plan-mode-panel.tsx:97-111) starts at setError(undefined) and only has the finally { setPending(false) }. So pending is never true and the four buttons gated on planMode.pending (:247,254,334,341) stay enabled while an action is in flight: approve twice lands on operation_conflict through expectedRevision, abandon re-prompts and re-sends. No test covers it. One line back, above setError(undefined).

P3:

  • planControlFailureCopy's result.ok branch (plan-mode-copy.ts:176-183) is unreachable now that the only caller is inside !result.ok; take error instead of result.
  • shell-copy.ts:2369-2377 calls unexpectedOperationFallback unconditionally for its console.error before classifying, so a 429 or timeout also logs an "operation failed" stack across the 52 call sites. Report only in the unclassified branch.
  • Still open from last time: the ExpectedOperationError<any> narrowing, the plan_control:<code>:<message> re-encoding in preload.ts:2377 (which the restored classifier will now read as "auth" for unauthorized), the eight session_control_blocked:* codes with no catalog, and newSessionFallbackTitle through the routing policy. None blocks.

On CI at this head: two failures, neither yours. test fails on transcript-scroll-cost.spec.ts:239 (.poll timeout, 34 passed / 1 failed); that spec and the code it drives came in with #4886, whose own run on main is green, so it is that spec's flake. Release Windows check was killed on a Get-CimInstance timeout. I will rerun once the P1 is pushed.

Evidence boundary: static read; the setPending regression is a line-by-line comparison of the same function at main, 9b2f54c and the head, not reproduced in the app; CI logs read with gh run view --log-failed.

AI-assisted review: drafted with Maka; I verified the missing setPending(true) against both earlier heads and the two P2 fixes myself.

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

@Astro-Han Fixed in 4624383de (rebased onto current main, c0229b0c6).

  • P1: setPending(true) is back at the top of run(). The regression test mounts usePlanModeState through the real LocaleProvider/ToastProvider chain with a stubbed window.maka.sessions, holds requestPlanRevision open with deferred, and asserts pending stays true until the promise settles — it fails on the previous head with pending must hold while the bridge call is unsettled.
  • P3: planControlFailureCopy(error, copy) now takes the envelope error; the unreachable ok branch is gone.
  • P3: localizedShellErrorMessage classifies first and only calls unexpectedOperationFallback when there is no category, with a test pinning that a timeout renders its category with zero console.error calls while an unrecognized message logs once.

The remaining non-blocking items (plan_control: re-encoding on the app-shell abandon path, the ExpectedOperationError<any> narrowing, the uncatalogued session_control_blocked:* codes) stay as follow-ups.

Verified: targeted dist tests 10/0, Desktop typecheck clean apart from stories/app-shell.stories.tsx (rightCollapsed, same on main), renderer architecture ratchet against c0229b0c6, biome, epoch guard.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 4624383dec43de8d55fdd8174928ef6f8591ee10. The previously reported Plan Mode concurrency regression is fixed, and I found no remaining P0-P3 issue in this revision.

usePlanModeState.run() now enters pending before invoking the bridge and clears it in finally, so all four controls that share this path become disabled while an action is unsettled. The new real-hook regression holds requestPlanRevision open and observes the rendered state transition. An independent mutation that removed setPending(true) made the new test fail at the pending assertion. The same commit also removes the unreachable success branch from the Plan control copy helper and reports unexpected shell diagnostics only after generalized classification fails.

Validation on this head: clean install, build:test, focused expected-error/Plan tests (25/25), full Desktop tests (2308/2308), renderer architecture (101/101), changed-file Biome, format, locale hygiene, ASF headers, and diff check. The head is based directly on current main (c0229b0c6) and the merge tree is clean. The Windows package check is green.

The required hosted test check remains red at the current-main Electron E2E budget mismatch (session-workbar.spec.ts has 6 tests while the budget records 5). Local Desktop typecheck likewise reaches unchanged current-main Storybook errors around WorkbarLayoutState.rightCollapsed. Neither surface is changed by this final commit, so I am not filing a PR finding for them, but the required gate is not green and this is not a merge approval. Native macOS/Windows behavior was not run locally.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at a0ef3e1 against main, clean merge, CI green.

All three are fixed at the owner: setPending(true) is the first line of run() again with the finally clearing it (plan-mode-panel.tsx:99, :112), planControlFailureCopy takes the error branch only (plan-mode-copy.ts:175-180), and localizedShellErrorMessage classifies before it falls back so a classified error no longer logs (shell-copy.ts:2373-2377), with the same copy per case as before.

The regression test is the right shape: it mounts the production usePlanModeState under the real LocaleProvider / ToastProvider with only window.maka.sessions stubbed, holds requestPlanRevision open and asserts pending before, during and after (plan-mode-panel-pending.test.ts:77, :118). On the previous head pending starts false and run() wrote nothing before its await, so the middle assertion fails there. The DOM boilerplate matches goal-dialog.test.ts, which is this directory's convention.

Interdiff from 0f25f62 is those three fixes, the two signature follow-ups plus one new case in expected-error-presentation.test.ts, the new test, and rebase noise. The three follow-ups you listed stay P3: the plan_control: re-encoding has no parser and only unauthorized could misclassify, on a path Desktop's local Host does not take; the <any> narrowing is type-only; the eight session_control_blocked:* codes sit in setters the Desktop bypasses through updateConfiguration.

AI-assisted review: drafted with Maka; I verified the three fixes, the test's failure on the old head and the interdiff myself.

@Astro-Han

Astro-Han commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Approved, but the merge is refused: renderer-architecture.json conflicts with main after #4491, #3828 and #4227 landed. Please rebase and regenerate the ledger with node apps/desktop/scripts/check-renderer-architecture.mjs --write; I will merge once CI is green on the rebased head.

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (00cada01c); head is now 364a6467b. The renderer-architecture.json conflict was resolved by regenerating the ledger with --write at each affected commit, so every commit in the range carries a consistent ledger rather than one fixup at the tip.

Verified on the rebased head: Desktop suite 2311/0, four typechecks 0 errors (the rightCollapsed story drift is gone now that #4895 is in), renderer architecture ratchet against 00cada01c, E2E budget holds (31 tests in 16 files), Biome clean.

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

@Astro-Han Rebased and CI passed now

@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 2 times, most recently from 6e6bef9 to 5625712 Compare September 6, 2026 11:03

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the complete 50-file merge-base diff at 5625712. The earlier Plan false-success and missing-pending defects are fixed. I found no reproduced P0–P2 production defect; one nonblocking P3 regression-test gap is detailed inline.

This changes expected-error transport and presentation, not just copy: Plan preserves structured Host failures over IPC; Work Board, attachments and collaboration map stable codes to locale catalogs; unexpected errors use redacted diagnostics; WorkHub copy is separated; WebSearch settlement retains a nonempty error message. I followed producer/transport/presenter paths and verified all declared Plan error codes through the registered handlers, including an actual missing-proposal storage failure. The deferred React hook test also confirms pending stays true until the bridge settles.

A clean lockfile install, repository dependency patches, full build:test and 158 focused compiled tests passed. The independent producer/IPC/pending probes passed separately. Hosted test and package are successful on this head. No real Electron UI, native-platform acceptance or full-repository test run is claimed.

This head is not merge-ready: GitHub reports CONFLICTING/DIRTY, and merge-tree against current main 2310035 independently reports conflicts in renderer-architecture.json and workhub-surface.tsx. Resolve those and validate the resulting new head; the current review cannot certify that future merge result.

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.

Comment thread apps/desktop/src/main/__tests__/expected-error-presentation.test.ts

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the complete rebased change at ae0e0cf. The previous P3 regression-test gap is fixed; no new substantiated P0–P3 finding.

The new test exercises all five registered Plan IPC handlers with typed Host failures, checks the preserved error code and asserts that no mode change is emitted. Disabling the actual production conversion now makes that test fail, unlike the previous head. The unexpected-error rethrow and React pending regressions also remain covered. I checked the rebase adaptations for current WorkHub resume outcomes/clarifications, locale catalogs, fallback-title callers and the protocol compatibility acknowledgment.

On this isolated head, clean install plus repository dependency patches, full build:test, 167 focused compiled tests and 101 renderer-architecture tests all passed; the actual architecture check passed too. Real Electron visual behavior, the full Desktop suite and native-platform acceptance were not run.

The previous main-conflict caveat is now resolved: GitHub reports MERGEABLE/CLEAN, hosted test/package are successful, and a local merge-tree against current main 2310035 is clean. This COMMENT records source and test results; it is not a human review or merge authorization.

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.

orangeCatDeveloper added a commit to orangeCatDeveloper/maka that referenced this pull request Sep 6, 2026
Squash of the stacked increment, rebased onto fix/desktop-error-codes
(apache#4457) after its rewrite onto main: typed AttachmentIngestBlockedError
with the attachment_blocked envelope, ExpectedOperationError across the
preload and session-settings surface, typed session-configuration
transition errors in the Runtime, and the sessionSettingsActions
updateFailures catalog per locale. localizedShellErrorMessage no longer
parses any token.

Generated-by: Claude Code
Generated-by: OpenCode
orangeCatDeveloper added a commit to orangeCatDeveloper/maka that referenced this pull request Sep 6, 2026
Squash of the stacked increment, rebased onto fix/desktop-error-codes
(apache#4457) after its rewrite onto main: typed AttachmentIngestBlockedError
with the attachment_blocked envelope, ExpectedOperationError across the
preload and session-settings surface, typed session-configuration
transition errors in the Runtime, and the sessionSettingsActions
updateFailures catalog per locale. localizedShellErrorMessage no longer
parses any token.

Generated-by: Claude Code
Generated-by: OpenCode
@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

@Astro-Han Rebased. PR is ready to be merged

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the localization work. Additional independent review of ae0e0cff46611dcdf26472b95f0455c0f8d4f108 found one remaining attachment acceptance gap in the normal new-task send path. The IPC envelope behavior was independently exercised in Electron; the finding concerns a renderer preflight that runs before that boundary.

AI disclosure: Codex agents performed this review, and the coordinating agent checked the production call path and reran the preflight/presenter counterexample. This is not an independent human review.

中文摘要

重新审查发现新建任务发送附件时仍有一条未覆盖的正常路径。Electron IPC envelope 已独立实测;这里的问题发生在进入 IPC 前的 renderer 预检。主审已核对真实发送调用链,并重跑预检与错误映射反例。这是自动化评审,不冒充人工验收。

Comment thread apps/desktop/src/renderer/locales/shell-copy.ts

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates. A fresh, multi-reviewer pass at ae0e0cff46611dcdf26472b95f0455c0f8d4f108 found one additional, optional ownership simplification below. The previously reported new-task attachment preflight issue remains unchanged. This review was AI-assisted and cross-checked by the coordinating AI reviewer; it does not represent completed human visual acceptance.

中文

谢谢更新。本轮独立深审新增一条可选的职责归属精简意见;此前新任务附件预检的问题仍未变化。本次由 AI 协助审查并由主审 AI 交叉验证,不代表已经完成人工视觉验收。

Comment thread packages/runtime-host/src/protocol/plan.ts Outdated
Raw exception text no longer reaches the UI: expected failures carry
machine-readable codes (with params where needed) that the renderer maps
through locale catalogs, and unexpected failures show a localized
fallback while redacted diagnostics go to the console. Plan controls and
work-board actions carry those codes across IPC in a structured envelope,
since Electron strips custom fields off thrown Errors.

Generated-by: Claude Code
…est tokens

The new-task send path runs preflightAttachmentItems in the renderer
before newTasks.create, so the main-side token validation never sees an
over-limit request - and the preflight threw localized prose, which the
send catch's token-only presenter cannot recognize. A user sending nine
attachments got "The message could not be sent. Try again later." in
both locales (retrying the unchanged input can never succeed) plus a
spurious unexpected-error diagnostic, losing the actual count reason.

The preflight now rejects with the same stable attachment_ingest:<code>
tokens the main-side resolveIngestItems validation rejects with
(count_limit / item_too_large / duplicate_source - same semantics as
the mirrored main checks), so localizedShellErrorMessage maps the real
reason with no new code path; renderer-local throws never cross the IPC
boundary, so the end-anchored tail matcher sees the bare token. The
quote-companion send catch rendered error.message directly and would
have leaked the raw token, so it now routes through
localizedShellErrorMessage. The conversation-copy attachments prose the
preflight was the only consumer of is deleted.

Regression drives the real createAppShellChatActions send path with nine
staged attachments: the send is rejected, newTasks.create is never
called, the toast carries the count_limit reason in zh-CN and en, and
console.error stays silent. It fails on the previous head with the
generic fallback; the rewritten preflight unit test pins the exact
tokens (and the oversized-approval-by-pending-size case).

Verified: desktop full dist suite 2336 pass / 0 fail (send-path test
fails on the previous head, passes here), typecheck (4 tsconfigs) 0
errors, renderer architecture ratchet against 2310035, biome clean.

Generated-by: pi (GLM)

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. A focused size/complexity follow-up is attached inline. This is AI-assisted source review, cross-checked by the coordinating reviewer; no new full-suite run.

中文感谢更新,行内补充一条规模与复杂度复核意见。本次为 AI 辅助源码审查,经主审交叉核对,未重跑全仓测试。

errorClass: string;
};

export type SessionControlBlockedCode =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Leave the future-only Session-control token contract out of this localization change

Thanks for documenting the reachability limit explicitly. The PR body says these eight guards have no current in-repository caller path and the token contract is reserved for a future CLI-local caller. The new session_control_blocked:* family has no renderer presenter; current Desktop configuration uses the Host protocol errors instead. Could we revert this new union/helper, the eight exception-text replacements and the two corresponding regex-only test edits, while preserving the guards themselves? This leaves the reachable Desktop localization behavior intact and avoids adding an unused error contract. This is scope/maintenance cleanup, not a hypothetical future runtime defect; the future-only path is not being reported as a reachable bug.

中文

感谢明确说明可达性限制。正文确认这八个 guard 当前没有仓内调用路径,新增 token 是为未来 CLI-local 调用方保留;现有 Desktop 配置走 Host 协议错误,这套 token 也没有 renderer 映射。建议撤回新增 union/helper、八处错误文本替换及两处仅更换正则的测试修改,保留 guard 本身。这样不影响当前可达的 Desktop 本地化验收,也不增加尚未使用的错误契约。这是 P3 范围和维护性精简,不把未来假设路径当作运行时缺陷。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the localization work and the production-path fixes. The combined review/simplification audit found three additional P3 opportunities to reduce new contracts; they are inline. The existing future-only Session-control token comment still applies.

One further optional consolidation spans several files: reportUnexpectedError and unexpectedErrorFallback merely forward to the new core diagnostic helpers. Diagnostic callers can use those core helpers directly, removing the forwarding API and its dependency edges; keep ExpectedOperationError, which has actual WorkBoard/WorkHub consumers.

The Plan IPC envelope itself, domain-owned catalogs, attachment preflight validation, and the real pending/IPC regression coverage have current obligations and should remain.

AI-assisted review by Codex; checked against the production producers and consumers.

中文

本轮新增三项 P3 简化意见见行内,之前未来 Session-control token 的意见仍适用。另可让诊断调用方直接使用 core helper,删除两个纯转发 API;ExpectedOperationError 有真实消费者,应保留。Plan IPC envelope、领域 catalog、附件预检及真实 pending/IPC 回归保护都有当前义务,不建议整片删除。

readonly results: ReadonlyArray<WebSearchResultRow>;
}
| { readonly ok: false; readonly reason: WebSearchErrorReason; readonly message: string };
| { readonly ok: false; readonly reason: WebSearchErrorReason; readonly message?: string };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Keep the existing Web Search error contract for this Desktop localization change

The Desktop settings consumer already renders copy.errors[result.reason] for query/test failures. Removing the preload diagnostic message therefore does not improve localization there, but making this public field optional spreads the change into the Runtime fallback and the new four-case ToolRuntime settlement matrix. The Host wire decoder still requires a string message, and the real Host/Tavily producers still supply it.

Could we keep message required, have the preload early returns supply a stable locale-independent diagnostic, and remove the optional-field change plus the Runtime fallback/test expansion? This preserves the existing producer/codec contract and the localized UI behavior. This is a normal-path scope/maintenance concern, not a claim that current Host producers omit the field.

AI-assisted review by Codex; checked against the production producers and consumers.

中文

设置页已经按 reason 查本地化文案,不直接展示 message。为了删除 preload 诊断文本而把公共 message 改为可选,又引出 Runtime 兜底与四组 settlement 测试;Host wire decoder 仍要求必填字符串。建议保留原必填契约,preload 返回稳定、与 locale 无关的诊断文本,撤掉由可选字段引出的整条扩张。当前真实 Host producer 并未省略字段,此项为 P3 范围与维护问题。

/** Thrown client-side when a Runtime Host operation resolves to a failure
* outcome; the structured `code` lets presenters map expected failures to
* copy instead of treating them as unexpected. */
export class RuntimeHostOperationError extends Error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Keep the client exception in its existing client export

RuntimeHostOperationError is constructed by the Host client when a wire failure becomes a local exception. The new Desktop main adapter can import the existing class from @maka/runtime-host/client, as the other production consumers do. Moving it into /protocol adds a public runtime export, a client re-export and compatibility bookkeeping without changing any wire behavior.

Please retain the original class owner/export and switch the new main-process catch to that import. Likewise, PlanControlErrorCode can use a file-private CONTROL_ERRORS; exporting that runtime array is not required by its type alias. Keep the actual Plan code type/spec changes and adjust, rather than blindly delete, their compatibility declaration.

AI-assisted review by Codex; checked against the production producers and consumers.

中文

这个异常由 Host client 将 wire failure 转成本地异常时构造;Desktop main 可直接使用原来的 client 导出。搬进 protocol 再从 client 转导出,新增公共运行时接口和兼容维护,却没有新增 wire 语义。建议恢复原归属;CONTROL_ERRORS 也只需在文件内供类型别名使用。保留真正需要的 Plan 错误码类型/spec 改动及相应声明。

}

export type AttachmentIngestBlockedCode =
| 'count_exceeded'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Use one attachment reason per validation outcome

The new token union distinguishes preload count_exceeded / size_exceeded / payload_invalid from main/preflight count_limit / item_too_large / items_invalid. These represent the same count limit, single-item size limit and invalid-input outcomes; the presenter does not branch by producer and instead maintains parallel copy entries in every locale.

Could preload emit the latter three codes as well, removing the three aliases and duplicate catalog entries? Keep the separate total-size, duplicate-source and expired-source outcomes. These tokens are introduced by this unmerged change, so this consolidation does not require a legacy token reader. The visible difference is consistent wording for the same failure at each layer.

AI-assisted review by Codex; checked against the production producers and consumers.

中文

preload 与 main/preflight 对同一数量上限、单件大小上限、无效输入分别引入两套名字,展示层并不按生产层区分,却要维护三种语言的重复文案。建议统一成 count_limit / item_too_large / items_invalid,删除另三个别名及重复 catalog;总量超限、来源重复、来源过期仍是不同语义,应保留。

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

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants