Skip to content

fix(desktop): localize expected errors - #4457

Open
orangeCatDeveloper wants to merge 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/desktop-error-codes
Open

fix(desktop): localize expected errors#4457
orangeCatDeveloper wants to merge 1 commit 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 11 attachment validations 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. 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). RuntimeHostOperationError stays in the protocol package under the declared compatible extension at epoch 117. 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 branch base is the current main (411512bd9), so no prerequisite PRs remain; the draft blocker (#4493 checker policy) landed on 2026-09-02.

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.

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.

Re-reviewed exact head c9e4fc96629e4c44214585c6facb0b5d2019f4a4 after the follow-up commits.

The attachment-token matcher is now end-anchored and the prior P3 is fixed. The Plan Mode change still does not preserve structured Runtime Host failures across the actual Electron IPC boundary, so the prior P2 remains and this head is not ready to merge.

Validation on Node 24.18.1: clean install, build:test, focused expected-error/Plan Mode/session-domain tests (22/22), Desktop typecheck, renderer architecture checks (101/101 including the base-relative gate), Biome lint/format, locale hygiene, ASF headers, and git diff --check. A real Electron main-to-preload-to-renderer probe confirmed that an IPC handler rejection arrives as a plain Error with no operation or code. The exact-head package check is green; the required hosted test check is red only at the unrelated product-composer-slash-menu--context-switch-starts-with-a-loading-catalog Storybook assertion after all preceding steps, including Desktop E2E, passed. The final c9e4fc966 commit is an empty CI retrigger. 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/locales/plan-mode-copy.ts Outdated
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 2 times, most recently from 583da57 to e0ea0c3 Compare September 6, 2026 01:32

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

This revision fixes the previously reported Electron serialization problem by carrying expected Plan control failures in a structured IPC envelope. I found one new P2 in the main-process adapter, so this is not merge-ready yet.

The focused Plan/main tests pass (21/21), as do the full Desktop suite (2230/2230), build:test, Desktop typecheck, renderer architecture (101/101), Biome, formatting, locale hygiene, ASF headers, and git diff --check. The branch merges cleanly with current main (2bfea3c32). The required hosted test is still red on the unrelated product-sidebar-session-list--project-groups focus assertion; package was still running at publication time. 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/main/runtime-host-session-domains-ipc-main.ts
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/desktop-error-codes branch 2 times, most recently from 3dcb7a1 to 9b2f54c Compare September 6, 2026 02:47

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

Reviewed at 9b2f54c (a rebase of 67072ee onto current main; the only delta is the ledger token count) against main, +1565/−573 over 50 files, CI green, merge-tree clean, two independent passes.

The problem is real and the direction is the one this family has settled on: session-collaboration dialogs, the plan panel, the work board, WorkHub and the guest-mount path all rendered error.message or classified English prose by regex; now producers throw ExpectedOperationError with a closed code and presenters look the code up in a UiCatalog through lookupCopy. I walked every producer and presenter pair: WorkHub's two codes, the work board's five plus unknown, and the collaboration import's five reasons each have a full catalog entry, so no presenter can receive a code it has no line for. Deleting the MAKA_SESSION_READ_MESSAGES_ERROR marker (consumer-only on main), the WorkHub regex classifier and the four hard-coded Chinese web-search sentences in preload.ts that the page never rendered is net subtraction. The three earlier findings are closed: the shell-copy matcher is anchored (shell-copy.ts:2358-2371) with a pinned negative, the plan IPC carries a PlanControlIpcResult envelope, and all five session-domain handlers rethrow unrecognised errors with tests. zh-TW is real traditional copy; the diff would pass the locale-hygiene ratchet.

Two P2s, then P3s.

P2: bridge-contract.d.ts uses PlanControlIpcResult without importing it. Lines 1249, 1256, 1257 and 1261 reference the type; the only plan import in the file is PlanSessionState at :68. skipLibCheck is on in tsconfig.base.json:7 and all four desktop tsconfigs extend it without override, so the unresolved name is not an error, and since global.d.ts:20 types window.maka from this file, the five plan bridge methods are Promise<any> in the renderer and the panel's result.ok has no type behind it. The pattern is already in the file at :185 for WorkBoardIpcResult. Fix: export type { PlanControlIpcResult } from '../shared/plan-mode-ipc.js';.

P2: the plan-control mapping exists twice and the test covers the copy nobody calls. planControlFailureCopy (plan-mode-copy.ts:184-193) is referenced only by expected-error-presentation.test.ts:86-99; plan-mode-panel.tsx:99-103 inlines copy.controlFailure[code] ?? copy.operationFailed instead. The test case says "at the panel" and never reaches it, so a wrong mapping in the panel stays green. The work board already solved this shape with workBoardActionErrorText (work-board-panel.tsx:404). Fix: have the panel call planControlFailureCopy and drop its unused locale and unexpected parameters and the ok branch, or delete the function and export the panel's mapping as the seam.

P3:

  • The classified sentences are gone from the shell path and the body does not say so. localizedShellErrorMessage and its app-shell-copy.ts twin used generalizedErrorMessageForLocale, which never returns raw text, only the localized timeout / 429 / 401 / 5xx / network line or the fallback; both now return unexpectedOperationFallback directly, and the last case in expected-error-presentation.test.ts pins that. Across 52 call sites the output is still a correct localized sentence, only less specific, and errors that carry a reason (model-connection-errors.ts:63-64 via describeSessionErrorReason) keep their specific copy, so this is a P3 not a P2. But fifteen-odd sibling presenters still classify (daily-review-actions.ts:33, settings-error-copy.ts:31, artifact-pane.tsx:618, use-session-trace.ts:148, …), so either say in the body that classification of raw text is being retired from the shell first, or keep it: report through unexpectedOperationFallback first so the diagnostic still lands, then classify.
  • copy.controlFailure[code] and errorCopy[board.error] index bare where the same diff uses lookupCopy elsewhere. Unreachable today because decodeOperationError rejects undeclared codes, except unauthorized, which operations.ts:483-485 admits unconditionally while CONTROL_ERRORS (plan.ts:64-70) lacks it, so the as PlanControlErrorCode cast at runtime-host-session-domains-ipc-main.ts:601-609 is false for that one (lands on operationFailed, harmless).
  • PlanControlIpcResult is declared twice (shared/plan-mode-ipc.ts:31-40, plan-mode-copy.ts:29-38); the comment says the renderer cannot import shared/, but use-work-board.ts already does. Delete the catalog copy.
  • ExpectedOperationError narrows to ExpectedOperationError<any> (operation-diagnostics.ts:26-32), so an unrelated code family (WorkHub candidates_changed into workBoardErrorCodeCopy) compiles.
  • preload.ts:2377 re-encodes the envelope as 'plan_control:${code}:${message}' and nothing parses it; the only consumer (app-shell.tsx:1017) takes the fallback.
  • session-manager.ts:883-889 adds eight session_control_blocked:* codes with no catalog and no presenter, replacing eight Chinese sentences with bare tokens; the three setters have no non-test caller (Desktop goes through updateConfiguration, CLI through transitionSessionConfiguration), and the first caller to arrive (pi-tui-runner.ts:938-943 prints error.message) would show the token. Drop the eight, or add the catalog when a caller exists.
  • plan-mode-panel.tsx:95 has trailing spaces after the parameter list; the body says Biome is clean on touched files.

Largest removable cost, acceptance unchanged: newSessionFallbackTitle threaded through WorkHubSubmitInputWorkHubRoutePolicy.resolveworkHubNewSessionName, which puts presentation copy into routing policy and accounts for most of the controller test growth. Smaller shape: the policy returns title: undefined when there is none and the surface fills copy.newSessionFallbackTitle when it renders or creates the Session.

Manual acceptance, three locales: plan approve / revise / abandon failures; a work-board action on a corrupt record; the session-collaboration dialogs with a blocked mode change; an in-conversation 429 with and without a reason.

Evidence boundary: static read against main; no build, no typecheck, no ratchet run, no Storybook. The Promise<any> claim follows from skipLibCheck and global.d.ts:20, not from running tsc.

AI-assisted review: two Maka passes; I verified the missing import, the generalizedErrorMessageForLocale removal and the planControlFailureCopy call graph myself.

@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 9b2f54c5b6060b1c655535f9ea89145e15fe01c0 against current main (d2d7efe645b6f1af2cb47ec9c85f28a85b32b5f9).

The previous unexpected-error handling issue is fixed. The four Plan control handlers that previously fell through after planControlIpcFailure() returned no expected envelope now rethrow the original error; the existing approve path has equivalent rejection behavior. The added regression covers all five Plan control channels with the same unexpected error and verifies that each rejects, so these failures no longer emit a false change event or return { ok: true }.

I found no new P0-P3 issue in the current PR diff. This head is not merge-ready yet because current main advanced after the PR was based and the merge tree now conflicts in apps/desktop/renderer-architecture.json; the branch needs to be rebased and revalidated.

Validation completed on this head: clean install, build:test, the focused main-process suite (15/15), Desktop tests (2238/2238), Desktop typecheck, renderer architecture tests (101/101), Biome, format, ASF header, and diff checks. The exact-head Windows package workflow passed. The required test workflow failed only in product-composer-slash-menu--context-switch-starts-with-a-loading-catalog; I reproduced the identical visibility assertion failure in an isolated worktree at the PR's exact base ef2a724537188aaafc4e3a8f2340b137109e6c5f, so I do not attribute it to this PR. I did not run native macOS behavior 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.

@orangeCatDeveloper orangeCatDeveloper 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.

Thanks — no new P0-P3 in this diff. I addressed the Astro-Han review in 4c9e7bb10 (bridge import, seam dedup, unauthorized declaration, report-then-classify shell presenter; details in the thread below), and the branch is now rebased onto current main at 0f25f62cf with the renderer-architecture.json token count resolved (verified against origin/main: ratchet passes, presentation 7/7, plan/session-domain 16/16, epoch guard passes, biome clean).

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

}, [copy.operationFailed, session?.id, session?.collaborationMode, refresh]);
const run = useCallback(
async (action: () => Promise<PlanControlIpcResult<unknown>>): Promise<void> => {
setError(undefined);

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.

[P2] Restore setPending(true) before starting the action. This hook still clears the flag in finally, and all four Plan controls use planMode.pending as their only in-flight guard, but the latest formatting/refactor commit removed the matching transition to true. A React hook probe with a deferred requestPlanRevision rendered pending=false for the entire unsettled request, so users can click again and dispatch concurrent revision/approve/resume/abandon operations against the same Plan. Please restore the transition and add a production-path regression that holds one bridge promise open and verifies the controls stay disabled until it settles.

@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

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