feat(desktop): add conservative WorkHub session router - #3497
Conversation
Refs: apache#3492 Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the architectural discipline here is real and worth saying first. "A Work is a user-facing view of one ordinary Session; WorkHub owns no second database, event log, or lifecycle" is exactly the right constraint for a feature like this, and the code holds to it: workhub-session-port.ts reads through to the Session, the policy module owns only transient inference state, and there is no shadow store. The rollout preference and the archived-Sessions-inspectable-but-not-routable rule are both good conservative choices. Publishing the policy comparison table instead of just asserting a choice is also the right instinct.
Reviewed at exact head 8177693fd2deee796246290b236a9a75090dff00 against base 17f9351a849821cd4f2401ecea8f062966614348. One P1 and two P2, plus two P3. CI is currently red on this head.
P1 — the routing policy is substantially a Chinese-language heuristic, in an English-first product. Details inline; the short version is that several routing signals have no English path at all, and the ones that do have English use the same numeric thresholds across two languages where those thresholds do not mean the same thing.
P2 — the description's own table argues against the policy it selects. R2 is listed at 9/11 routing; R2.3 at 8/11 routing and 7/11 strict E2E. The text says R2.3 is the best current compromise under the product constraints: no new authority, conservative behavior when evidence is ambiguous, and a usable path for creating new work — but R2 is described as conservative routing with more clarification, which sounds like it satisfies those same three constraints while scoring higher on the shared axis. What is the dimension on which R2.3 beats R2? It may well exist — R2's extra clarification prompts could be judged too costly in practice — but the table as written does not show it, and a reader has to take the conclusion on faith. Since the whole point of publishing the comparison is to let a reviewer check the reasoning, this gap matters more than it would in an ordinary PR. One or two sentences would close it.
Worth stating plainly: at 7/11 strict E2E, roughly three in ten submissions land somewhere other than where the user meant. The rollout preference makes that an opt-in cost rather than a regression, so it does not block. But it does mean the honest framing of this MVP is "an experiment that needs real usage data", and the description's confidence should match that.
P2 — CI is red for a generated artifact, not a logic failure. astryx:surface-inventory fails because .paths is missing apps/desktop/src/renderer/styles/workhub.css and workhub-surface.tsx, and docs/astryx-surface-file-inventory.md no longer matches. Running npm run astryx:surface-inventory:write and committing the result should clear it. Flagging because the description lists a full green verification run, so this was presumably generated before the last two files landed.
P3 — the routing thresholds are unnamed magic numbers. longestMatch >= 4 appears twice with different meaning, alongside >= 3 in correctedTarget, >= 2 in the exact-name filter, and slice(0, 5) / slice(0, 4) for clarification options. For a policy that will be tuned against real usage, these are the knobs — naming them as constants with a one-line rationale each would make the next tuning pass reviewable.
P3 — recentTargets[1] as "the previous focus" is positional. looksLikePreviousFocus resolves to index 1 of an array whose ordering is maintained by rememberTarget. It works, but the meaning lives in an array position rather than in a name, and a future change to how targets are remembered would silently change what "previous" means.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
| ); | ||
| } | ||
|
|
||
| function looksLikeTargetUncertainty(value: string): boolean { |
There was a problem hiding this comment.
[P1] Several routing signals have no English path, and the ones that do are tuned on Chinese.
The description says the policies were evaluated against the same 10-Session snapshot and Chinese acceptance guide. That shows through in the implementation in ways that leave English input on a different code path rather than a slightly worse one:
looksLikeTargetUncertaintyis Chinese-only. There is no English alternative in the pattern at all. An English user typing "I'm not sure which one this belongs to" never reaches the clarification branch — it falls through to correction/focus/entity matching and gets routed somewhere with no prompt. This is the one signal whose entire job is to stop the router from guessing, and English input cannot trigger it.- The boilerplate stripping in
routingTermsis Chinese-only (只分析风险和测试点,不要修改任何文件,先不动代码). The English equivalents — "just analyse the risk", "don't change any files", "don't touch the code yet" — stay in the term set and score against session identities. ROUTING_STOP_TERMSis 18 Chinese terms and 4 English ones (please,continue,work,task). Common English routing noise —the,this,file,issue,check,update,fix,code,test— is absent, so those words all contribute toscoreand tolongestMatch.workHubNewSessionNameonly extracts a title from Chinese markers (标题|名称|名字), and only strips Chinese creation prefixes.create a new session called Parser Cleanupproduces a session named from the first 48 characters of the whole sentence rather thanParser Cleanup— even thoughlooksLikeExplicitNewSessiondoes have an English branch, so English users reach this function and then get the degraded name.looksExecutable's design-question veto includes\bwhat\b, so "fix what is broken in the parser" is classified as a design question and routed to discussion instead of being executable.
This is ordinary-user-path reachable, and the failure is silent: the message goes to the wrong Session, or to discussion, with no signal that routing was uncertain.
We are not asking for full parity in this PR — an MVP is allowed to be narrow. What we would ask for is that the narrowness be declared rather than latent. Either scope the feature explicitly (the preference could state that routing is currently tuned for Chinese input), or add the English counterpart to looksLikeTargetUncertainty specifically, since that one is the safety valve: when the router cannot tell, asking is always the correct fallback, and right now English users cannot reach it.
| right.score - left.score || right.session.updatedAt - left.session.updatedAt); | ||
| } | ||
|
|
||
| function routingTerms(value: string): string[] { |
There was a problem hiding this comment.
[P1, same finding] The scoring layer treats Chinese and English asymmetrically, and then applies one threshold to both.
routingTerms builds terms two different ways:
- Chinese: every 2-to-6 character n-gram of each run, fully expanded. A 6-character phrase yields 15 terms, so
longestMatchfor a genuine phrase match is naturally 4–6. - English:
[a-z0-9_./-]{2,}— whole tokens only, no n-grams.longestMatchis just the length of the longest matching word.
Then rankRelatedSessions matches with identity.includes(term) against a normalizeIdentityText string that has had all whitespace and punctuation removed. For Chinese n-grams that is fine, since the terms are already substrings by construction. For English it means every term is matched as an unanchored substring of a concatenated blob, so word boundaries are gone.
Concrete failure: a Session named profile-settings normalizes to profilesettings. A user types "check the file parser". file survives (not a stop term), identity.includes('file') is true via profilesettings, giving score = 4 and longestMatch = 4. If no other session matches at 4, this hits the related[0].longestMatch >= 4 && (related[1]?.longestMatch ?? 0) < 4 branch and routes directly with evidence core_entity — no clarification. The word file had nothing to do with that session.
The same >= 4 threshold means "one meaningful Chinese phrase" and "any four-letter English word" — test, data, main, code, file, user. That asymmetry is why English input is more likely to route confidently and more likely to route wrong.
Two changes would address the mechanism rather than the symptom: match Latin terms on word boundaries instead of raw includes (keeping the concatenated form only for the Chinese n-gram path), and give the Latin path its own threshold rather than sharing the Chinese one. Both are local to this function and rankRelatedSessions.
A regression case worth pinning either way: the profile-settings / "file" shape above, asserting it yields clarification rather than a confident target.
Refs: apache#3492 Generated-by: Codex
Generated-by: Codex
Preserve WorkHub availability semantics across the extracted Workbar controller. Generated-by: Codex
Generated-by: Codex
Summary
Adds the first usable WorkHub MVP as a unified entry point over existing ordinary Sessions.
R2.3-session-authoritative-hybrid.Refs #3492
Routing choices and experiments
We evaluated all policies against the same 10-Session snapshot and Chinese acceptance guide. The scores describe different test layers and runtime shapes, so they are not directly interchangeable:
R2.3 is not selected because it has the highest raw score. R2 and R2.1 were measured in the earlier experiment, where
WorkHubSnapshot.routingMemorygave WorkHub its own durable routing state and the surrounding runtime had more mature interaction surfaces. Porting that result directly would violate the chosen architecture boundary. R2.3 deliberately trades some measured accuracy for a smaller production shape: Session remains the sole persistent authority, while WorkHub keeps only discardable in-memory focus and correction hints. The current 7/11 strict result therefore supports shipping this as an opt-in MVP experiment, not declaring the routing problem solved.Its precedence is: exact full Session name; explicit creation/correction; core entity evidence; then bounded focus confirmed inside the current WorkHub lifetime. Weak multi-match input asks for clarification. Clear unmatched executable input creates an ordinary Session, while discussion, negated creation, and creation questions remain in WorkHub. At equal evidence, the latest correction wins.
Adversarial review also hardened the implementation so archived Sessions remain inspectable but cannot be routed to, creation negation/questions cannot write, same-frame double submissions are gated synchronously, failed/waiting submissions preserve the draft, English routing uses whole-word evidence and explicit uncertainty, and routing only consumes evidence populated by the production Session adapter.
Verification
npm run lint— passed (2,591 files)npm run format:check— passednpm run typecheck— passednpm run build— passednpx knip --workspace apps/desktop— passednpx knip --workspace packages/ui— passedgit diff --check— passedUI evidence
Captured from the real development app on this branch with the current 10-Session projection.
WorkHub overview — the unified entry point models the existing ordinary Sessions without creating a second Work store.
Routed to an existing Session — the request stays visible in WorkHub together with the selected target and its current status.
Correction picker — the user can inspect and change the selected Session without introducing another authority.
Follow-up scope
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the WorkHub MVP, produced routing experiments and tests, and performed parallel Standards/Spec adversarial review. The affected commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?