Skip to content

fix(desktop): return expected failures as codes across IPC - #4878

Merged
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/ipc-expected-failure-envelope
Sep 13, 2026
Merged

fix(desktop): return expected failures as codes across IPC#4878
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/ipc-expected-failure-envelope

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Changing a busy task's permission, plan, orchestration, or model setting shows a generic fallback instead of the actionable reason. WorkHub attachment validation can likewise collapse to “Retry” because Electron discards custom Error fields at IPC boundaries.

Expected failures now cross Electron as discriminated return values while unexpected failures still throw. Session settings and Plan Mode preserve Runtime Host codes, and attachment send paths preserve AttachmentIngestBlockedError.code; renderer adapters reconstruct typed errors for the locale presenters. No product path parses Error.message.

Boundaries: the Runtime Host protocol is unchanged, so there is no Host version skew. The plan-mode write still uses the session-settings port to keep the renderer architecture ratchet flat.

Refs #2672

Verification

WorkHub attachment rejection with a specific reason: 0/1 before → 1/1 after.

Before

WorkHub attachment rejection before

After

WorkHub attachment rejection after

Electron expected-failure e2e       2 pass / 0 fail
focused Desktop dist tests        111 pass / 0 fail
apps/desktop typecheck (4)          0 errors
renderer architecture ratchet      passes against ca4136a02
npm run format:check               clean

A session_busy result also reaches both locale presenters as structured data:

当前任务正在运行或有交互待处理,等结束后再改设置。

A task is running or waiting on you. Change this setting after it settles.

Not run: packaged build.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Code and Codex — traced the failure paths, implemented the structured envelopes and locale presentation, simplified the error contract and comments, added tests and Electron evidence, rebased onto current main, and updated this description. The protocol boundary and user-facing copy were reviewed by hand.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 5, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch 5 times, most recently from 3af86a4 to 8aaadc1 Compare September 6, 2026 21:32
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 7, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch 2 times, most recently from b98a8d3 to ba1495e Compare September 8, 2026 21:08
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/XXL Over 2500 readable lines labels Sep 9, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch 3 times, most recently from 7c5f3fd to b84d0d6 Compare September 11, 2026 04:12
@orangeCatDeveloper
orangeCatDeveloper marked this pull request as ready for review September 11, 2026 04:21
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch from b84d0d6 to b05a71d Compare September 12, 2026 05:31

@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 moving expected failures into structured results. The overall direction looks sound: Runtime/Host keeps ownership of configuration and Plan state, while Desktop transports the failure code and the renderer owns its presentation. I found one remaining serialization boundary issue in the Plan-abandon path, noted inline.

For simplification, I would keep attachment rejection presentation separate from showSkillInvocationFeedback: its new attachment branch makes a Skill-specific helper responsible for a general submit failure. This is an optional cleanup, not a correctness blocker.

Reviewed at b05a71dde11606456ef1ff902b7dacfc81889d5f. The inline finding was checked with a minimal Electron 43.4.1 contextBridge probe; the full Desktop suite was not rerun, and CI is still running.

AI assistance: Codex with a deep reviewer; the finding was cross-checked against the production path and the Electron probe.

Comment thread apps/desktop/src/preload/preload.ts Outdated

@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 fix. An independent fresh-eye re-review of 2dc82e4596b77ed8cbe42718ce5454c792cd79d7 found no remaining P0–P3 code issue.

The previous Plan-abandon serialization defect is fixed: the structured result remains intact through main and preload, and the renderer-owned session-settings adapter unwraps it after the contextBridge boundary. The setting-update and attachment-rejection paths were also traced through their production consumers. I do not have a further simplification to request.

Two verification items remain: the PR currently conflicts with main, and the required test check failed on the upward-traversal-holds-turn-geometry Storybook render timeout; Windows packaging passed. That story was not changed directly, but this PR does touch AppShell, so the single timeout does not establish that the failure is unrelated. Please resolve the conflicts and verify the resulting head, including a focused Electron check of busy-setting, Plan-abandon, and attachment-rejection feedback.

This pass used exact-source checks and the live CI evidence; it did not rerun the full build or packaged Electron E2E.

AI assistance: a new Codex deep reviewer performed the fresh-eye pass, followed by a primary-agent cross-check.

@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch 2 times, most recently from ae40c34 to fe92570 Compare September 12, 2026 18:55

@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 update! The Session-setting and Plan-abandon paths look sound in this pass. I found one remaining WorkHub attachment issue, noted inline.

This also changes user-visible feedback. Could you add actual before/after screenshots of the resulting toasts? The current text examples help explain the wording, but do not show the interaction. A short recording covering busy-setting rejection, a Plan-abandon conflict, and attachment rejection would be helpful too.

I checked head fe9257020: 106 focused tests passed, and the Electron boundary probe confirmed both the corrected paths and the remaining WorkHub issue. This does not replace full UI acceptance.

AI assistance: Codex and Reviewer Sol, with the finding independently reproduced.

Comment thread apps/desktop/src/renderer/locales/shell-copy.ts
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch 3 times, most recently from d76a809 to 5092dbe Compare September 12, 2026 19:48
@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 13, 2026
@github-actions github-actions Bot removed the effort/L Under 1000 readable lines label Sep 13, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch from 5092dbe to 2df6502 Compare September 13, 2026 04:55

@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 closing the WorkHub attachment gap and adding real screenshots. I found no remaining P0–P3 issue at 2df6502. Expected codes now survive main, preload and contextBridge, with typed errors reconstructed in the renderer adapter. The shared attachment preparation also removes duplication. Verification covered 44 focused tests and an isolated Electron probe using this head’s full preload, real WorkHub handler and renderer adapters; setting, Plan and WorkHub attachment failures retained their codes and localized feedback. CI is green. The current conflict in create-workhub-services.ts still needs resolution: keep both main’s new interaction ports and this PR’s attachment result unwrapping, then verify the updated head before merging. AI-assisted review with Codex and Reviewer Sol.

Expected failures must survive both Electron IPC and contextBridge serialization so the renderer can present actionable localized copy. Return structured envelopes through those boundaries, unwrap them only in renderer-owned adapters, and keep attachment rejection presentation separate from Skill feedback. Runtime configuration guards use typed transition errors instead of localized prose.

Generated-by: Claude Code
Generated-by: OpenCode
Generated-by: Codex
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/ipc-expected-failure-envelope branch from 2df6502 to 0bda47e Compare September 13, 2026 07:46
@Astro-Han
Astro-Han merged commit 09f0a5d into apache:main Sep 13, 2026
2 checks passed
@orangeCatDeveloper
orangeCatDeveloper deleted the fix/ipc-expected-failure-envelope branch September 13, 2026 08:23
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 13, 2026
…f0a5d)

Eight upstream commits. One reaches the new renderer's contracts: apache#4878
returns expected failures as codes across IPC — the five Session setters
(`setPermissionMode`, `setCollaborationMode`, `setOrchestrationMode`,
`setModelConfiguration`, `setThinkingLevel`) answer
`DesktopSessionUpdateResult` instead of throwing, `abandonPlanProposal`
answers `PlanControlIpcResult` like its siblings, `send` / `submitMessage`
gain an `attachment_blocked` refusal, and the attachment guard throws a typed
`AttachmentIngestBlockedError` in place of the `attachment_ingest:<code>`
message token. Also in: apache#5216 makes HTML artifacts directly openable
(`app.showArtifactInFolder` beside `openArtifactPath`, which now hands an
HTML artifact to the default app; `isArtifactUserVisible` admits HTML tool
results), apache#5198 unifies WorkHub conversation identity and Host-owned choices
(an Astryx `ChoicePanel` in packages/ui, `keyboardHint` copy, a
`preserveFocus` scroll target), apache#5249's skill picker fix
(`selectedSkillIds` in `chat-input-behavior`), apache#4815 admits structured-only
Messages (`hasMeaningfulMessageContent` in core), apache#4862's ACP live session
lifecycle in the CLI, apache#5204's workbar tab scrollbar css and apache#5180's wider
locale hygiene gate.

Resolution per the sync policy: conflicts under the old renderer's trees,
packages/ui's deleted components, stories, e2e specs and the main tests that
import them stay deleted, and upstream's new files there are dropped
(`features/workhub/model/workspace-name.ts`, packages/ui's `choice-panel.tsx`
and its `index.ts` export, the `styles/base.css` / `workhub.css` /
`maka-tokens.css` edits, `expected-error-presentation.test.ts` and the WorkHub
main tests). git's rename pairing had put upstream's
`features/session-settings/ports.ts`, `features/workhub/testing.ts` and
`platform/desktop/create-session-settings-services.ts` into
`bridge/e2e-fixture.ts`, `components/ui/skeleton.tsx` and
`lib/ported/display-frame-scheduler.ts`; all three keep ours. The renderer
architecture ledger keeps ours, rewritten with `--write`. The e2e budget and
`transcript-scroll-cost.spec.ts` keep ours; upstream's new
`expected-failure-feedback.spec.ts` is trimmed to its second case (the IPC
round trip of the setting and Plan codes), the first needing the WorkHub
surface this build does not ship.

Re-implemented for the new contracts:
- `bridge/sessions.ts` unwraps every update result and rethrows a refusal as
  `ExpectedOperationError` (new `bridge/expected-operation-error.ts`, ported
  from upstream's `operation-diagnostics.ts`), so the turn actions store and
  every caller keep awaiting a summary. `localizedShellErrorMessage` renders
  the code through the new `updateFailures` copy on every surface and
  `AttachmentIngestBlockedError` by its `code`; `sessionSettingFailureCopy`
  is upstream's.
- `ChatInput` routes an `attachment_blocked` refusal through upstream's
  `showSubmissionFeedback` and keeps the draft with the ingest reason;
  `showSkillInvocationFeedback` stays exported for the partial-success toast.
- `FilesTab` reveals through `showArtifactInFolder`; an HTML row shows "View
  in Maka", its menu offers "Open in Default App" first, and its preview's
  external action opens rather than reveals. `artifact-copy` gains
  `viewInMaka` / `openInDefaultApp` in three locales.
- `TipTapEditor` hides Skills already in the draft from the picker (apache#5249);
  the chips are atoms here, so the set is read from the document rather than
  from the `/skill:x` text upstream scans.
- `composer-state.test.ts` asserts the typed preflight error.

packages/ui: `use-chat-scroll.ts` merged cleanly (`preserveFocus`) on top of
our `holdTurn` extensions; `conversation-copy.ts` and `chat-input-behavior.ts`
take upstream's additions. apache#5217's live-turn buffer stays out as before.

The compatible-change declaration is unchanged this round. The release
checklist's baseline note records that the eight commits add no unlisted
renderer surface.

Gates: build:test + build:renderer, typecheck, biome lint and format, locale
hygiene (the widened apache#5180 gate), ASF headers, renderer architecture ledger
(rewritten with `--write`), e2e budget, third-party notices, knip (39 unused
files, unchanged from the twelfth sync), workspace dist tests (desktop 3484
of 3498 with 14 skipped, every other workspace green), Electron smoke (44
checks, no renderer errors), core-dialogue smoke, streaming-switch smoke, and
the trimmed `expected-failure-feedback` e2e case against the real preload.
`packages/runtime` `model-adapter-onerror` fails on this machine before and
after, as in the eleventh and twelfth syncs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

2 participants