Re: [PR] fix(desktop): let WorkHub select a missing default model [maka]

["jackwener (via GitHub)" <gi...@apache.org>]

jackwener commented on code in PR #5481:
URL: https://github.com/apache/maka/pull/5481#discussion_r4047964751


##########
apps/desktop/src/main/runtime-host-workhub-ipc-main.ts:
##########
@@ -52,9 +54,21 @@ export function registerRuntimeHostWorkHubIpc(
   options: RuntimeHostWorkHubIpcOptions,
 ): void {
   ipcMain.handle('workhub:getSession', async () => toDesktopHostSessionSummary(await client.getWorkHubSession()));
-  ipcMain.handle('workhub:resolveCoordinationSession', () =>
-    client.resolveWorkHubCoordinationSession(),
-  );
+  ipcMain.handle('workhub:resolveCoordinationSession', async (): Promise<WorkHubCoordinationSessionResolution> => {
+    try {
+      return await client.resolveWorkHubCoordinationSession();
+    } catch (error) {
+      if (
+        error instanceof RuntimeHostOperationError &&
+        error.operation === 'workhub.coordination.resolve' &&
+        error.code === 'operation_conflict' &&
+        error.message === WORKHUB_COORDINATION_DEFAULT_MODEL_REQUIRED_MESSAGE

Review Comment:
   [P2] This treats a catch-all fallback as a specific diagnosis, so unrelated failures will tell the user to configure a model.
   
   `createTargetFailure` (`workhub-coordination-coordinator.ts:1098-1103`) is not a "missing default model" reporter. It is a two-branch fallback:
   
   ```ts
   if (error instanceof SessionOperationFailure && error.code === 'persistence_failed') {
     return failure('persistence_failed', error.message);
   }
   return failure('operation_conflict', WORKHUB_COORDINATION_DEFAULT_MODEL_REQUIRED_MESSAGE);
   ```
   
   **Everything** that is not a `persistence_failed` session failure comes back as `operation_conflict` with that exact message, whatever actually went wrong, and the original error is discarded — not even its message survives. The string is the fallback's text, not a signal about models.
   
   Matching on it here turns that fallback into a confident, user-facing instruction. The renderer acts on it: `model_required` becomes `WorkHubModelConfigurationRequiredError`, which sets `modelSetupRequired` and opens model setup (`use-workhub-controller.ts`). So a resolve that failed for an unrelated reason now sends the user to pick a default model, they pick one, the underlying failure recurs, and they are sent around the same loop with no indication of the real cause.
   
   To be precise about what this pull request is and is not responsible for: **the flattening in `createTargetFailure` is pre-existing and I am not asking you to fix it here.** What is new is *consuming* it as if it were specific. Before this change the failure surfaced as a failure; now it surfaces as a confident wrong answer, and a confident wrong answer is harder for a user to get out of.
   
   The message match also does not survive version skew, which this repo takes seriously elsewhere. `WORKHUB_COORDINATION_DEFAULT_MODEL_REQUIRED_MESSAGE` is shared at compile time, but the comparison runs against whatever Host is actually on the other end. The compatibility note added in `be7a35eaf` says older and newer peers observe identical *protocol* behaviour — true, and exactly the point: message prose is not part of that protocol. `operation_conflict` is the declared contract; the sentence is a diagnostic. A Host built before this constant, or after someone rewords it, silently drops the actionable path.
   
   Both problems have the same fix: give the condition a real code. Either add a distinct error to `workhub.coordination.resolve`'s error union, or return a structured detail alongside `operation_conflict`, and have the coordinator emit it only when a default model is genuinely missing — leaving the fallback to stay a fallback. That also makes `createTargetFailure`'s flattening visible to whoever next tries to debug it.
   
   <details>
   <summary>简体中文</summary>
   
   **[P2] 这里把一个兜底分支当成了具体诊断,于是无关的失败也会叫用户去配置模型。**
   
   `createTargetFailure`(`workhub-coordination-coordinator.ts:1098-1103`)不是"缺少默认模型"的报告者,它是一个两分支兜底:**除了 `persistence_failed` 的会话失败之外,所有错误都会变成 `operation_conflict` + 那句固定文案**,不管实际出了什么事,而且**原始错误被整个丢弃,连 message 都没留下**。那句话是兜底分支的文本,不是关于模型的信号。
   
   在这里匹配它,就把兜底变成了一句面向用户的确定指令。渲染层真的会照做:`model_required` → `WorkHubModelConfigurationRequiredError` → `modelSetupRequired` → 打开模型设置。于是**因为无关原因失败的 resolve,现在会把用户送去挑默认模型;挑完,底层失败复现,再被送进同一个循环,而真实原因全程不可见。**
   
   范围我说清楚:**`createTargetFailure` 的压平是既有的,我不要求你在这个 PR 里修它。** 新增的是**把它当作具体信号来消费**。改动之前,失败就是失败;改动之后,失败变成了一个自信的错误答案——**而自信的错误答案更难让用户脱身**。
   
   另外,按文案匹配也扛不住版本偏斜,而这个仓库在别处对此很当真:常量是编译期共享的,但比较发生在"对面究竟是哪个 Host"之上。`be7a35eaf` 里的兼容性声明说新旧对端观察到相同的**协议**行为——没错,而这正是要点:**文案不属于该协议**,`operation_conflict` 才是声明的契约,那句话只是诊断。早于该常量构建的 Host、或将来有人改写措辞,都会让这条可操作路径静默失效。
   
   两个问题同一个修法:**给这个状况一个真正的码**——在 `workhub.coordination.resolve` 的错误联合里加一个独立错误,或在 `operation_conflict` 之外返回结构化细节,并让协调器**只在确实缺少默认模型时**发它,兜底就老老实实做兜底。这也顺带让 `createTargetFailure` 的压平对下一个来排查的人可见。
   
   </details>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org