feat(workhub): add direct stop coordination - #4439
Conversation
M4n5ter
left a comment
There was a problem hiding this comment.
English
I recommend addressing three correctness issues before merging. The review was performed against exact head fe7e267e.
-
P1 — one
actionIdcan stop two delegations. Stop claims are looked up and persisted bystopsDelegationId, without a durable global owner for the stop action ID. If the first request is persisted but retirement returnsrecovering, the rejection releases the in-memory action slot; the sameactionIdcan then claim and retire another delegation. I reproduced this on the exact head without a restart. This is also reachable through the normal UI lease behavior, which preserves the action ID across reload while allowing edited text. Please establish one durableactionId -> exact operation/fingerprintclaim and reject cross-delegation or cross-disposition reuse before any effect. (gate, durable request) -
P2 — target Session removal can permanently lose the stop outcome.
delegation_stop_requestedis committed before retirement and resolution. After a pending cancellation succeeds, a crash beforedelegation_stop_resolvedfollowed by target Session removal can cascade-delete the cancellation claim/Turn proof. Retry then finds no receipt, steering proof, tombstone, or admission and remainsrecoveringforever. Session removal must not erase proof needed by an unresolved stop claim; coordinate removal with the claim or retain the proof outside the target Session lifetime. (effect/resolution seam, fallback torecovering) -
P2 — replacement is persisted as direct-stop provenance. Replacement also calls
retireDelegation(source, replacement.actionId), but every owning-root retirement now goes throughstopOwnedWorkHubRoot(), which hardcodessource: 'workhub_direct_stop'. A normal route correction is therefore permanently recorded asworkhub.direct_stop.<replacementAction>despite having no direct-stop request oruser_stopconfirmation. Please separate the cancellation claim identity from the Stop operation/provenance; only direct stop should emitworkhub_direct_stop. (replacement path, provenance write)
Two non-blocking simplifications are worth considering in this PR:
- Make
cancellationClaimIdmandatory incancelMessageIfPending()and remove its unused production no-claim mode. - Represent stop parsing as a single
none | unusable | nameddiscriminated union and parse once, instead of maintainingcue + imperative + target?and matching twice.
Validation: the relevant Core, Storage, Runtime, Runtime Host, and Desktop builds passed, as did 448 targeted tests. The P1 path was independently reproduced. CI is green, but these boundary cases are not covered. The PR currently conflicts with main because both branches independently use compatibility epoch 87; after rebasing, the combined protocol epoch should be advanced rather than resolving the conflict by retaining 87.
中文
建议合并前修复以下三个 correctness 问题。本次审查基于精确 head fe7e267e。
-
P1 — 一个
actionId可以停止两个 delegation。 Stop claim 按stopsDelegationId查询和持久化,没有 durable 的全局 stop action ID owner。第一个 request 已持久化、但 retirement 返回recovering时,失败会释放内存 action slot;同一个actionId随即可以 claim 并退役另一条 delegation。我已在精确 head 上复现,而且无需重启。正常 UI lease 也会在 reload 后保留 action ID、同时允许修改文本,因此这是实际可达路径。请建立统一的 durableactionId -> exact operation/fingerprintclaim,并在任何 effect 前拒绝跨 delegation 或 disposition 的复用。(gate,durable request) -
P2 — 删除 target Session 会永久丢失 stop outcome。
delegation_stop_requested在 retirement 和 resolution 之前提交。Pending cancellation 已成功后,如果 Host 在写入delegation_stop_resolved前崩溃,随后 target Session 被删除,cancellation claim/Turn proof 会被级联清除。重试找不到 receipt、steering proof、tombstone 或 admission,只会永久停在recovering。Session removal 不得清除 unresolved stop claim 所需的证明;应协调 removal 与 claim,或将证明保存在 target Session 生命周期之外。(effect/resolution seam,fallback torecovering) -
P2 — replacement 被持久化成 direct-stop provenance。 Replacement 同样调用
retireDelegation(source, replacement.actionId),但 owning-root retirement 现在都会经过stopOwnedWorkHubRoot(),并硬编码source: 'workhub_direct_stop'。因此普通 route correction 会被永久记录为workhub.direct_stop.<replacementAction>,尽管不存在 direct-stop request 或user_stopconfirmation。请分离 cancellation claim identity 与 Stop operation/provenance;只有 direct stop 可以写入workhub_direct_stop。(replacement path,provenance write)
另有两项值得在本 PR 中考虑的非阻塞简化:
- 将
cancelMessageIfPending()的cancellationClaimId改为必填,删除生产环境已无调用方的 no-claim 模式。 - 将 stop parser 收敛成单一的
none | unusable | named判别联合并只解析一次,删除cue + imperative + target?组合及重复匹配。
验证结果:相关 Core、Storage、Runtime、Runtime Host 和 Desktop 构建通过,448 项定向测试通过;P1 路径已独立复现。CI 虽然全绿,但没有覆盖这些边界。PR 当前还与 main 冲突,因为两个分支分别使用了 compatibility epoch 87;rebase 后应推进合并协议的 epoch,而不是保留 87 来解决冲突。
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head fe7e267eb3cd9a4053666557bf9583488d154011 (+2760/-152, 42 files; production ≈ +1206, tests ≈ +1512, docs +42). heavy, plan, test, label, windows_recovery all green; MERGEABLE; no prior review. Read-only against source. I executed the two stop regexes from this head to produce the table below; everything else I verified by reading. I did not run a live session.
First, the part I have no findings on. The durable claim → effect → resolution ordering, the action-derived abort source that lets a replay distinguish WorkHub's own delivery from a concurrent manual Stop, the runMany admission over the Coordination Session plus every active target lane, and the fail-closed re-proof inside that lease are correct as far as I can follow them. I traced the crash seam in #stop: a crash between retireDelegation and resolveStop replays to the same outcome on both the pending-cancel and the owning-root branch, which is exactly what the provenance binding buys. That is the dangerous half of this change and it is well built.
My findings are all on the other side — how ordinary text becomes a stop. None of them are about the destructive machinery being too complex; they are about a ten-line function in the renderer.
P1 — the stop route is one-way, and its clarification carries no options
resolveStop commits to the stop route on intent.stop.cue alone. Whether the named target resolves to an active delegation only selects between target and clarification — it never withdraws the routing decision. So a failed reference is reported to the user as a problem with their phrasing, which is the one thing that was fine.
flowchart TD
U["<b>Stop using the deprecated API</b><br/>an ordinary work request"]
G{"resolveStop diverts on<br/>intent.stop.cue alone"}
A["normal routing<br/><i>where this input belongs</i>"]
B["stop route<br/>target = 'using the deprecated API'"]
C["no active delegation<br/>has that name"]
D["clarification: 'Name the work explicitly'<br/>with an empty options list"]
U --> G
G -->|"not taken"| A
G -->|"taken: the sentence starts with a stop verb"| B
B --> C
C --> D
D -. "user restates it, identically" .-> U
D -. "missing: withdraw the route" .-> A
classDef dead fill:#FCEBEB,stroke:#A32D2D,color:#501313
classDef hot fill:#FAECE7,stroke:#993C1D,color:#4A1B0C
class D dead
class B,C hot
Two things have to be true at once for the loop to close, and both are new here.
It is one-way. Every other branch in workhub-route-policy.ts that fails to resolve a reference either falls through to the next hypothesis or degrades into a clarification that offers alternatives — resolve does this three times (affirmedCorrections, alternatives, related plus the rest). resolveStop is the first branch in this file that cannot be left, and control returns from createWorkHubController before coordination.candidates() is ever reached.
The clarification has nothing in it. The convention in this file is that a clarification carries the choices it is asking about. The one exception is ambiguous_command, and that exception is principled: it is about the sentence being unparseable, so there is nothing to list. stop_target_required copies that shape but is about a target, so it inherits emptiness exactly where the answer set is what the user needs — either "these are the delegations you can stop" or "none of them."
The reachable surface is wider than the stop feature. DIRECT_STOP_REQUEST captures everything after the leading verb as the target, so any work request beginning with stop/cancel/terminate/halt or 停止/取消/终止/中止 parses as a named direct stop. Running the two regexes and UNSAFE_STOP_TARGET from this head:
| input | parsed stop target |
|---|---|
Stop using the deprecated API in the parser |
using the deprecated API in the parser |
Cancel the pending invoices feature |
pending invoices feature |
Terminate the SSH connection when idle |
SSH connection when idle |
Halt the retry loop when the queue drains |
retry loop when the queue drains |
停止使用废弃的 API |
使用废弃的 API |
取消订单超时的重试逻辑 |
订单超时的重试逻辑 |
Each is imperative: true, matches no Session, and lands on the empty clarification. The copy then tells the user to name the work explicitly — which they did — so restating it reproduces the identical response. Nothing routes and nothing is created. The escape (We should stop using the deprecated API) exists only because the regex is start-anchored, which no user can be expected to discover.
Your own negative-case table shows the gap: it covers pronouns, pause/wait, questions, negation, and malformed literals, and Pause Payments correctly stays out of the stop branch and routes normally. There is no case where a stop-shaped sentence is ordinary work.
Smallest fix, at the same site: when a stop parses but names nothing stoppable, return not_requested and let normal routing have it. Keep clarification for cue && !imperative (a stop speech act with an unsafe target) and for a genuinely ambiguous matching.length > 1. Regression worth pinning: Stop using the deprecated API routes as ordinary work while Stop Payments still stops the Payments delegation.
P2 — after a not_owned outcome, that delegation can never be stopped again
not_owned deliberately writes no terminal state: activeWorkHubAssignments filters only outcome !== 'not_owned', and the controller keeps the action active with if (admitted.outcome !== 'not_owned') removeActiveAction(...). So the link stays active and the Session stays stoppable in the UI, and the user — who was just told "Open the Session to stop it" — will reasonably try again.
The second attempt carries a new requestId. In the gate, readStopRequest finds the existing claim and assertStopReplay rejects on its first comparison, request.actionId !== input.actionId, raising action_conflict — permanently, for every future attempt.
Neither test covers this. The controller test named "retryable" stubs act to return not_owned unconditionally, so it proves the renderer re-submits, not that the gate accepts. The gate test replays with the same actionId, which is the crash-replay case. The domain-language entry says a not_owned resolution releases the exclusion so a later route correction can proceed — it does not release it for a second stop, and I think that is the gap rather than the intent.
P2 — the same dead end for a real stop whose target is not exactly one active delegation
stoppable keeps only Sessions with exactly one active delegation, so naming a Session with zero (already finished, or never delegated by WorkHub) or with two produces the same empty clarification. Host-side the proof is consistent and fails closed, so nothing unsafe happens; the user simply cannot learn why, and repeating the name cannot help. The P1 fix covers this if the copy also distinguishes "WorkHub is not running work called X" from "X has more than one active delegation."
P3 — the stop authorization proof is written twice
WorkHubCoordinationActionGate.act()'s stop_work branch and HostWorkHubCoordinationCoordinator.#prepareStop's beforeAppend both call readWorkHubRequestIntent(userText), build sessionNameById from listHeaders(), filter active assignments through workHubStopTargetsSession, require exactly one that is the named source, and check supersession and replacement. Only the second runs inside the admission lease, so only the second is authoritative; the first is an early rejection whose policy can drift into something different without a test noticing, since the two raise different action_conflict messages for the same condition. Worth converging on one predicate both sites call, leaving the gate with what only it can do: the user_stop confirmation, the assignment lookup, and the replay assertions.
P3 — the correction path's Stop provenance changes without being mentioned
retireDelegation(source, replacement.actionId) now routes a correction that retires a running owned root through stopOwnedWorkHubRoot, stamping workhub.direct_stop.<digest> on the target Turn where main passed no source, and switching that path from cancelMessageAdmissions to claimMessageAdmissionCancellation. I checked both: the claim variant's body is the single-message body of cancelMessageAdmissions plus one column, and turnAbortStatusLabel only special-cases renderer.stop_button, so I do not believe anything user-visible changes. But the durable record now labels correction-initiated stops as direct_stop, on a path shipped in #4242, and the Summary describes only direct stop. Either name the source for both destructive paths or say so in the Summary.
P3 — the abort source hashes an identifier that is already stored in the clear
workHubDirectStopAbortSource writes workhub.direct_stop.<48 hex of sha256(actionId)>. That same actionId sits in plaintext in delegation_stop_requested.actionId, and it is already constrained to [A-Za-z0-9_-]{1,128}, so there is no secret and no length problem. The digest costs a node:crypto import in session-projection-helpers.ts and makes the durable abort source unreadable when diagnosing a stuck stop. workhub.direct_stop.<actionId> compares just as exactly.
P3 — cancelMessageIfPending's claim id is optional but never omitted
execution-composition.ts:1312 is the only production caller and always passes one, so if (!cancellationClaimId) { await cancelMessageAdmissions(...) } and the claimOutcome === undefined case are unreachable. Making the parameter required deletes both and leaves one cancellation write on this path. (cancelMessageAdmissions keeps its three other callers.)
Next step
Requesting changes for the P1 only; it is confined to resolveStop plus the clarification copy, and the second P2 falls out of the same fix. The not_owned P2 I would also like fixed here since the durable claim is what blocks the retry, but I will not hold the PR on it if you would rather resolve it in the follow-up that adds pause/resume — say which and I will re-review promptly either way. The P3s are yours to take or defer.
The ADR and domain-language entries are unusually good and are most of the reason this was reviewable at all. Thank you for writing them.
AI-assisted review: I used Maka to read the diff and the surrounding authorities on main, and to draft this comment. I executed the two stop regexes and UNSAFE_STOP_TARGET from this head to produce the P1 table, and verified every other claim against source myself. I own this assessment.
Dismissing my own blocking state — the findings above stand as review comments rather than a merge block. #4439 already carries independent findings from another reviewer, and the author should weigh both without two separate blocks to clear.
fe7e267 to
be83bef
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
I found one P1 blocker on exact head be83bef50ac0a42f6077b919b62c769e6f501a04.
The change adds direct WorkHub stop coordination, global durable action ownership, target-removal convergence, replacement-specific cancellation provenance, and the associated Core/Storage/Runtime/Host/Desktop protocol and UI paths. I reviewed the complete 42-file diff, the current-main merge, migrations 37-38, action replay/assignment ordering, stop and replacement effects, renderer send-lease behavior, and the prior findings on the older head.
The previous current concerns are addressed for newly admitted actions, removed stop targets, replacement provenance, stop parsing, and not_owned retry behavior. However, an existing replacement prepared before the new action-claim table was introduced can still perform destructive retirement before its action identity is checked against a conflicting post-upgrade claim; see the inline P1.
Validation: clean npm ci, npm run build:test, full typecheck, Core 750/750, 458 affected exact-head tests, and 458 affected current-main synthetic-merge tests passed. Full Runtime Host completed 1,563 pass / 1 fail / 12 skip; the sole live-sandbox failure reproduced on the current-main synthetic merge and is environment-related. Full Desktop completed 1,837 pass / 0 fail / 8 cancelled in the unchanged MCP OAuth deadline group. git diff --check passed. Hosted plan and windows_recovery are green; heavy is still running.
Unable to validate a real interactive WorkHub/provider session in this Linux environment.
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.
Architecture discussion: resolve Sessions, not stop syntaxThis is a first-principles / Occam pass, not a claim that I already have the final design. I think direct stop belongs in WorkHub. At this point WorkHub is naturally becoming both a coordination control plane and a navigation layer. My concern is narrower: the current change makes target resolution a stop-specific text grammar. The current path is roughly: That gives Stop its own target resolver in Starting from the underlying model:
A possible direction to discuss is: For an initial deterministic baseline, Session retrieval could use BM25 over permitted raw-message chunks and Session metadata, aggregate message hits by Visibility also fits this model cleanly. Hidden, archived, or logically deleted Sessions can be excluded from the recall projection according to explicit policy while their underlying records follow the existing retention/recovery contract. The search index remains rebuildable rather than becoming lifecycle authority. From this perspective, the valuable part of this PR is the latter half: durable action identity, exact Message ownership, pending cancellation, owning-root Stop, crash replay, and stop/replacement arbitration. The questionable part is permanently binding that control protocol to a bespoke direct-stop parser and exact Session-name uniqueness proof. My current Occam conclusion is:
BM25 is only one plausible first baseline, not the final answer. I would like to discuss whether there are repository or product constraints that rule out Session-level relevance recall, and whether this PR can keep the durable stop execution work while avoiding the regex/exact-name path becoming the canonical target-resolution contract. |
|
@likun666661 I agree with this boundary. The direction I propose is: The Resolver is shared by continue, inspect, stop, resume, delegation, and future control actions. It may combine structured references, visible Session metadata, focus/recency/state signals, and later lexical retrieval such as BM25. It only proposes identities; it never grants execution authority.
For this PR, the durable half remains valuable and should stay: action identity, exact ownership, cancellation, owning-root Stop, replay, and stop/replacement arbitration. The current direct-stop parser and exact-name lookup should be treated as a compatibility adapter, not the canonical architecture. Follow-up work can introduce the shared Resolver contract first, move stop onto it, then add deterministic retrieval/BM25 behind the same contract. |
|
Thanks, I think we are aligned on the long-term boundary. My preference is to do the shared Session Resolver properly rather than knowingly establish a second target-resolution path. If delivery timing requires this PR to keep the deterministic regex/exact-name behavior temporarily, I think the minimum acceptable shape is to introduce the shared Without that port, the current code is not really a compatibility adapter: there is no previous direct-stop contract to preserve. It becomes the canonical implementation by construction, and its regex, display-name matching, clarification behavior, tests, and domain language will all become migration constraints for the future Resolver. Putting the boundary in place now would let this PR retain its valuable durable stop execution work while making the temporary resolver visibly replaceable. I would also avoid documenting the exact-name grammar as a long-term product contract, and link a concrete follow-up with removal criteria for the temporary resolver. One smaller modeling question: I think a Session Resolver should probably return ranked existing Sessions, If there is no timing constraint, I would rather introduce the real shared Resolver path in this change. If there is one, the interface plus temporary implementation seems like the smallest step that does not harden the wrong boundary. |
|
@likun666661 Agreed on both points. I will treat the shared I also agree that I added a proposed design document that records the original experiment branches and R2.4/R3 plan, the production slices already delivered, the new pipeline and contracts, the reframed experiment arms, and the remaining PR-sized work:
The WorkHub domain language now also names |
af0c519 to
16d15de
Compare
There was a problem hiding this comment.
Reviewed 16d15dec (+4845/−172, 47 files: production 1822, tests 2583, docs 440). Built the workspaces and ran test:dist for core, storage, runtime-host and the desktop WorkHub suites, plus desktop typecheck and the renderer architecture check; all green. No live session.
First, the good news, because there is a lot of it. Every point from my last round is closed for real except one: the stop-shaped ordinary work now falls back to normal routing, not_owned converges instead of conflicting forever, correction no longer writes direct-stop provenance, cancelMessageIfPending lost its optional claim, and admission is now proven from opaque delegation identity rather than display names. M4n5ter's one-actionId-two-delegations case is closed by the durable claim table, hqhq1025's prepared-replacement ordering is closed by claiming before replay. The claim → effect → resolution ordering and the crash replay through same_claim are the most solid part of the PR.
Why it is this big. Since my last review the PR grew by 2085 lines: 52% tests, 19% docs, 29% production. It is big in redundancy, not in scope, and I do not think it should be split; there is no merge constraint between the halves, and the port without a consumer or the schema without a claimant would each leave main in a state only the next PR explains. What I would like instead is the list at the end.
One thing that blocks merge (P1, normal user path). Delete any Session that was ever a WorkHub target, and every direct stop in the system fails from then on. workhub-coordination-action-gate.ts:385-392 rejects the proposal if any active assignment points at a Session that is no longer listed, and the coordinator repeats the check under the lease. Nothing ever retires that assignment: activeWorkHubAssignments only knows superseded, aborted and stop-resolved, Session deletion writes nothing to the coordination log, and a stop or correction aimed at the dangling delegation is refused by the same visibility check before it can resolve. The existing test rejects a stop when its target is removed before stop admission ends in exactly this state; append one more stop against a different Session to it and it fails. The fix is to scope the visibility proof to the delegation being stopped (currentTargetName and the coordinator's visibleSessionIds.has(input.targetSessionId) already do that) and delete the two global some(...) checks; if you want dangling delegations out of the active set, probeSessionRemoval is the tombstone-backed proof to use.
Two P2s, same root.
stopOwnedWorkHubRoot(execution-composition.ts:1978-1996) decides "active" fromreadRootState, which is in-memory. After a Host restart, before recovery re-registers the execution, it skipsstopRoot, reads a snapshot that can still berunning, and returnsalready_terminal, which#stopthen commits as an immutabledelegation_stop_resolved. The user sees "stopped"; the Turn keeps running.isHostedExecutionTerminalfromhosted-execution-authority.tsis the seam; returnrecoveringwhen the stop was not delivered and the snapshot is not terminal. I could not confirm whethercoordination.actis admitted before recovery completes, so the window may be narrower than it looks, but the committed fact is still wrong when it happens.- No fact records that a delegation's work finished. Delegate to Session S twice, let the first complete normally, and S has two "active" delegations forever: renderer answers
stop_target_not_unique, Gate and coordinator refuselength !== 1, correction takes.at(-1). S can never be direct-stopped again. This is the death end from my last round with better wording; the wording is now also untrue ("more than one active delegation" when one is done). This PR is the first to let "active delegation" carry destructive authority, so it needs to say which concept that is: a link that ends only by supersession, or an execution that ends when the Turn does. If the completion fact belongs to a later slice, the body should state the constraint explicitly.
Two more P2s on the resolver work.
- The two stop clarifications
stop_target_not_activeandstop_target_not_unique(workhub-route-policy.ts:197-203) are decided from the renderer'sactiveActionIdsBySessionIdmirror alone; nocoordination.actis sent, so the Host never checks them. A second window or a reload before the coordination stream has refilled gives a zero-option clarification that contradicts Host state. Either read the Host before answering, or submit and let the Gate's conflict reason come back as the copy; the second also removes the duplicated sole-delegation predicate. - The port went in but the path it was meant to replace stayed.
workHubSessionReferenceNamesSessiondelegates tostopTargetMatchesSession(workhub-creation-intent.ts:444), whilecorrectionTargetMatchesSessionat:397is a second ~25-line copy of the same rule, andresolve()in route-policy still runs its ownrankExactSessions/rankRelatedSessionswithout touching the port. So the exact-name grammar is still canonical by construction, which is what likun666661's thread set out to avoid, and the port is a third path rather than the shared one. I tried both ways to close this on a scratch branch. Folding the two matchers into one strict core with per-action tails is a net −3 lines and makes the rule single-sourced; the tails are genuinely different (correction needs the withdrawal guard and accepts a trailing executable clause, stop accepts punctuation only), so the core is the real seam. Routing correction through the port instead costs +37 lines and forces ascopeon the reference, so the port starts choosing the action, which its own doc says it never does. Removing the port entirely is −394 lines with every core and desktop test green and behaviour unchanged. likun666661's ask was one of two shapes: the real shared resolver now, or the port as the first replaceable implementation. Pick one in this PR: either bringresolve()and correction onto the resolver too, or drop the port and let the folded matcher be the shared seam until a ranked resolver has a second real consumer. What is there now is neither.
What I would cut, in this PR. I tried each of these on a scratch branch from 16d15dec before asking (build, runtime-host and storage suites, and knocking out each guard one at a time to see which test still catches it), so the numbers below are measured, and two items I had first drafted turned out wrong and are corrected here. Roughly 1400 lines below this head in total, with the P1 fixed and one new regression test, all suites green:
expects.activeActionIds(protocol, Gate checks at:401-421,sameActiveDelegationSet,listActiveAssignments, renderer proposal: −192 lines). The coordinator proves the same thing from durable state under the lease; with the field gone, no guard mutation survives, so the client field cannot reject anything the proof would admit. It is arequireExactRecordmember on epoch 95 now, so removing it later costs another epoch. The Gate'sreadSupersession/readReplacementre-reads before the lease and the coordinator's secondvisibleSessionIdslookup are the same shape: no test kills them even today, because the same proof is made again later under the lease.- Most of the multi-lane admission in
#prepareStop. Every writer of a delegation fact already holds the Coordination lane (execution-composition.ts:1422and the three terminal facts), so that lane alone is exclusive over the active set, andinitiallyActive/admittedTargetSessionIds/ the "active set changed" check only pin the set seen at the advisory read; they also block unrelated Turns on every delegated Session and make a stop of A fail because someone delegated to B. The one window they do not close is Session deletion:session-retirement-coordinator.ts:437runs on the deleted Session's lanes without the Coordination lane, so the stop target can vanish betweenbeforeAppendand the append. One line closes it,runMany([WORKHUB_COORDINATION_SESSION_ID, input.targetSessionId]), which holds exactly the lane a concurrent delete would take. Keep that, drop the rest. readWorkHubActionClaimacrossSessionAuthorityStore,SqliteSessionStoreandexecution-stores: zero production readers. AssertclaimWorkHubAction'sclaimed → same_claim → conflictsequence instead.- Not the claim on
answer_hereandclarify. I first drafted "claim only for destructive operations"; removing those two#claimActioncalls leaves all 1611 runtime-host tests green, and then this passes:answer_herewith some actionId, Host restart (empties the replay map),stop_workwith the same actionId, admitted, delegation retired.readWorkHubAssignmentcannot catch it because an answer never creates an assignment, so the claim row is the only durable owner of that identity. Keep the calls and add that scenario as a test; nothing covers it today. What remains open is that the table is never deleted from andclaimed_athas no reader, so say what retention it implies. docs/architecture/workhub-action-resolution-design.md(326 lines). Its contract half is already in the ADR and the domain-language file this PR also edits; the rest is history, delivered-slice tables, plan and deferred decisions, whichdocs/README.mdassigns to issues and discussions. The nine branches it cites do not exist on apache/maka. Move the narrative to #3286 and delete the file.- Tests: the two runtime-host files carry 1170 lines against 559 production lines, largely the same fail-closed precondition spread over fixture tables in both. After the P1 fix and the cuts above, each stop guard is killed by exactly one test and those two files lose about 250 lines;
workhub-session-resolver-port.test.tskeeps its first case (the described-resolver swap is the one independent obligation) and drops the other four, which re-prove controller and core tests one layer down. One test is missing:stoppedin thedata-link-statetable ofworkhub-surface-flow.test.ts.
Smaller: workHubDirectStopAbortSource still hashes an actionId that is stored in clear two facts earlier; WorkHubResolverSession.projectName / updatedAt have no reader; stop_target_required remains a zero-option clarification. Epoch 95 is correctly bumped here, but #4386 also claims 95, so whichever merges second must take 96, not resolve the conflict to 95. The body has no evidence for the eight new copy strings and the stop turn rendering; there is no WorkHub story, so a recording is fine.
Evidence boundary: static read of 16d15dec against main 92fa5281; core 772/772, storage 1089 pass, runtime-host 1599 pass, desktop WorkHub suites 110/110, typecheck, architecture check, Biome on touched files; no live session; cross-Host execution ownership not checked.
AI-assisted review: drafted with Maka across three passes (Host, core/storage, desktop/docs); I verified the global visibility check, the missing completion fact, the restart path in stopOwnedWorkHubRoot and the size split myself.
简体中文
上轮的意见除 abort source 哈希外都真正关了,claim → effect → resolution 的顺序和崩溃重放是全 PR 最扎实的部分。体量:上轮之后新增 2085 行,测试 52%、文档 19%、生产 29%,大在冗余不在范围,不建议拆。
阻塞一条:删掉任何一个曾被 WorkHub 委派过的 Session,之后全局所有 direct stop 永久失败。Gate 第 385 行对全部 active assignment 做可见性检查,而 delegation 没有完成事实、Session 删除也不写 coordination log,悬空的那条永远清不掉。修法是把可见性证明收敛到被停的那一条,删掉两处全局 some。
四条 P2:stopOwnedWorkHubRoot 用内存态判 active,Host 重启后会把仍在运行的 Turn 记成 already_terminal 并落成不可变事实;同一 Session 被派活两次后永久不可 stop,因为没有完成事实,本 PR 第一次让「active delegation」承担破坏性权威,必须说清它是链接还是执行概念;stop 的两条 clarification 只信 renderer 本地镜像,Host 不参与;port 只接了 stop,correction 的名字匹配和 resolve() 的检索仍是独立实现,exact-name 文法仍按构造是规范实现,port 成了第三条路。实测:两个 matcher 合一净 −3 行且规则单源;correction 接上 port 要 +37 行并让 port 知道 action;整个去掉 port −394 行行为不变。李坤要的是两种形状之一,本 PR 二选一,别留半截。
可删清单我在实验分支上逐条试过,实测约 −1400 行、全绿,其中两条修正:answer/clarify 的 claim 不能删,去掉后同 actionId 先 answer、重启、再 stop 会被放行并真的执行 retire,请保留并补这条回归;多通道 admission 保护的只有「Session 删除不持 Coordination 通道」这一个窗口,一行 runMany([COORDINATION, targetSessionId]) 才能关上,其余那套可删。成立的:expects.activeActionIds(−192,无 mutant 幸存)、Gate 的两处 lease 前重读和 coordinator 的第二份可见性查找(今天就没有测试能杀死它们)、readWorkHubActionClaim 四层、326 行设计文档、测试矩阵收敛后每条守卫恰一条测试。epoch 95 与 #4386 撞号,后合的改 96。
|
@ARE404 Two things outside this PR that surfaced on the scratch branch, in case you want to file them:
Happy to share the branches if you open something for either. 简体中文两件本 PR 之外的事,供你决定要不要开 issue:main 上带引号的命名请求会被判不可执行;系列里有几条已无生产者或消费者的路径,删掉后测试全绿。需要的话我把实验分支给你。 |
Stop admission proved that *every* active delegation still had a visible target Session. Nothing ever retires a delegation whose Session the user deleted: `activeWorkHubAssignments` ends a delegation only on supersession, replacement abort, or a resolved stop, and Session removal writes nothing to the coordination log. One deleted target therefore left a permanently active delegation that failed the global check, and from that point every direct stop in the system was refused — including stops aimed at healthy, unrelated Sessions. Prove visibility only for the delegation being stopped, in both the Gate and the coordinator's under-lock recheck. A dangling delegation still fails closed when it is itself the target, which is correct: its Message proof is gone. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
The stop proposal carried the active delegation set the policy observed, and the Gate compared it against current state. That proof was already made from durable facts under the admission lease, where it is authoritative; the client-supplied copy could not reject anything the durable proof would admit, and as a member of a closed protocol shape it would have cost another compatibility epoch to remove later. Drop it. The proposal keeps the one precondition it can meaningfully assert — the Session it resolved the delegation against — and the Gate proves sole-active-delegation from the assignments it just read. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Root registration is in-memory. Between a Host restart and execution recovery a still-running root looks inactive, so `stopOwnedWorkHubRoot` skipped `stopRoot`, read a snapshot that was still `running`, and reported `already_terminal` — which the Gate then commits as an immutable `delegation_stop_resolved`. The user is told the work stopped while the Turn keeps running. `already_terminal` is an observation that can never be revised, so only a durably terminal snapshot may claim it. Anything else is `recovering`, which admission already handles by leaving the claim unresolved for a later attempt. `readDelegationRetirement` proved retirement the same way and gains the same rule: an unregistered root is not evidence that its work ended. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Nothing records that a delegation's work is done. A link ends only by supersession or a resolved stop, so delegating to one Session twice and letting the first complete normally left it with two permanently active delegations: the renderer answered `stop_target_not_unique`, the Gate and coordinator refused on the count, and that Session could never be direct-stopped again. The two senses of "active" were being conflated. The link is still active — WorkHub still owns it, and correction still works through it — but a delegation whose work has finished is not a competing stop target, because there is nothing left in it to stop. Only work that could still be stopped makes a Session's stop target ambiguous. Prove that from the target Session, which owns execution state, through the existing non-destructive `readDelegationRetirement`. The renderer applies the same rule to its own delegation mirror using the read-only execution projection it already receives. Execution state that cannot be read counts as competing, never as finished, so the stop still fails closed while the owner is unresolved. This deliberately does not add a completion fact to the coordination log: target execution state stays an ordinary Session fact that WorkHub reads as a rebuildable projection. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
`stop_target_not_active` and `stop_target_not_unique` were decided from the renderer's own delegation mirror, and both returned without ever calling `coordination.act` — so the Host never saw the request and never got to disagree. That mirror is filled only by the Coordination stream, which means it is empty in a second window, after a reload, and across a reconnect. In that window WorkHub told the user, specifically and confidently, that a Session had no running work while it did. Add a read-only `workhub.coordination.delegations` query and ask it once a reference has resolved to one Session, so an ordinary message never pays for the round trip. The Host answers with its active links and, for each, whether it still holds work a stop could reach — the same judgement admission makes, from the same durable state. That also removes the client-side settled-delegation mirror added for the completion-fact fix: which delegations are stoppable now has one owner instead of a renderer projection that had to agree with the Host. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Generated-by: Codex
Generated-by: Codex
Generated-by: Claude Opus
…lver Direct stop owned its own target resolver: it recognized stop-specific text and then matched Session display names itself, ahead of the ordinary routing path. Every future control verb would have grown one the same way. Introduce the shared `SessionResolver` port. It answers one question — which visible existing Sessions a trusted user reference recalls — as ranked candidates, nothing, or ambiguity. Candidates are opaque Runtime-issued references bounded by the caller's visible set, so a resolver can neither invent an identity nor widen its own visibility. `create_new` is absent from the result type: creation is an Action Policy decision, never a retrieval result. Exact display-name matching becomes the first implementation behind that port rather than the stop path's own rule, and stop becomes an Action Policy: Action Intent supplies the reference, the Resolver recalls Sessions, and the policy alone decides destructive sufficiency. The stop decision now carries the resolved delegation identity, so the renderer no longer re-derives it from its own active-delegation bookkeeping. Exact-name syntax is not the long-term product boundary. It can be removed once every target-bearing WorkHub action resolves through this port, the replacement resolver passes the common routing evaluation, and its rollout retains a tested rollback path. Generated-by: Claude Opus
Stop admission re-derived its target from the request text: the Action Gate and the coordinator's under-lock recheck both parsed the user's words again and proved uniqueness by matching Session display names. That made display names the destructive authority and coupled the durable stop protocol to one resolver's grammar, so replacing the resolver could not have kept the protocol intact. The stop proposal now carries what the Action Policy resolved: the opaque delegation identity, the Session it belongs to, and every active WorkHub delegation the policy observed for that Session. The Gate revalidates exactly that immediately before any effect — the assignment exists, it still belongs to the proposed Session, and that Session's current active delegations are still the set the policy saw, which for stop must be the one delegation being stopped. A stale resolution fails closed; a rename between resolution and admission correctly no longer invalidates a claim. Trusted user text must still carry a direct stop imperative, and `user_stop` confirmation stays outside strategy output, so neither model output nor a display name can select what gets stopped. The Gate no longer proves that the text names the target: that binding now rests on the proposal being produced by trusted policy, which is what lets a ranked resolver replace exact naming without touching durable stop execution, replay, ownership, or arbitration. `stop_work` shapes are introduced by this compatibility epoch, so refining the proposal needs no further epoch. Generated-by: Claude Opus
Describe direct stop as it now behaves: its target comes from the shared Session Resolver, its proposal carries opaque identities and the expected active-delegation state, and admission revalidates that state rather than re-deriving a display-name match. Say plainly that a rename between resolution and admission is irrelevant while a concurrent delegation to the same Session is not, and that the trusted-text binding is now the stop imperative and the out-of-band confirmation rather than a name. Record in the design doc why admission by expected state is what makes the port real, and note that correction still resolves its own target and joins the port with continue, inspect, and resume under item 2. Generated-by: Claude Opus
Stop admission proved that *every* active delegation still had a visible target Session. Nothing ever retires a delegation whose Session the user deleted: `activeWorkHubAssignments` ends a delegation only on supersession, replacement abort, or a resolved stop, and Session removal writes nothing to the coordination log. One deleted target therefore left a permanently active delegation that failed the global check, and from that point every direct stop in the system was refused — including stops aimed at healthy, unrelated Sessions. Prove visibility only for the delegation being stopped, in both the Gate and the coordinator's under-lock recheck. A dangling delegation still fails closed when it is itself the target, which is correct: its Message proof is gone. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
The port went in but the paths it was meant to replace stayed: stop resolved through it, correction kept a second near-identical copy of the same name-matching rule, and the exact-name grammar was still canonical by construction. That is a third path, not a shared one. The two matchers differed only in what they allowed *after* the name — a stop reference may carry punctuation and nothing else, a correction may name its target and go on to say what to do with it. That difference is an action's rule, not a retrieval rule, and it is why folding the two matchers into the Resolver would have forced the Resolver to know which action it was serving, contradicting its own contract. So the Resolver reports where the name matched and what text was left over, and says nothing about whether that leftover is acceptable. One `matchWorkHubSessionName` now owns the matching rule for both actions. Stop's tail rule moves into the stop Action Policy; correction's stays in its own predicate, which now takes a match so a caller that already resolved candidates applies the rule to exactly that recall instead of matching names a second time. Correction resolves through the port too. The Resolver stays action-agnostic: reporting a remainder is a neutral fact about a match, not a decision about what to do with it. Generated-by: Claude Opus
The stop proposal carried the active delegation set the policy observed, and the Gate compared it against current state. That proof was already made from durable facts under the admission lease, where it is authoritative; the client-supplied copy could not reject anything the durable proof would admit, and as a member of a closed protocol shape it would have cost another compatibility epoch to remove later. Drop it. The proposal keeps the one precondition it can meaningfully assert — the Session it resolved the delegation against — and the Gate proves sole-active-delegation from the assignments it just read. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Root registration is in-memory. Between a Host restart and execution recovery a still-running root looks inactive, so `stopOwnedWorkHubRoot` skipped `stopRoot`, read a snapshot that was still `running`, and reported `already_terminal` — which the Gate then commits as an immutable `delegation_stop_resolved`. The user is told the work stopped while the Turn keeps running. `already_terminal` is an observation that can never be revised, so only a durably terminal snapshot may claim it. Anything else is `recovering`, which admission already handles by leaving the claim unresolved for a later attempt. `readDelegationRetirement` proved retirement the same way and gains the same rule: an unregistered root is not evidence that its work ended. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Nothing records that a delegation's work is done. A link ends only by supersession or a resolved stop, so delegating to one Session twice and letting the first complete normally left it with two permanently active delegations: the renderer answered `stop_target_not_unique`, the Gate and coordinator refused on the count, and that Session could never be direct-stopped again. The two senses of "active" were being conflated. The link is still active — WorkHub still owns it, and correction still works through it — but a delegation whose work has finished is not a competing stop target, because there is nothing left in it to stop. Only work that could still be stopped makes a Session's stop target ambiguous. Prove that from the target Session, which owns execution state, through the existing non-destructive `readDelegationRetirement`. The renderer applies the same rule to its own delegation mirror using the read-only execution projection it already receives. Execution state that cannot be read counts as competing, never as finished, so the stop still fails closed while the owner is unresolved. This deliberately does not add a completion fact to the coordination log: target execution state stays an ordinary Session fact that WorkHub reads as a rebuildable projection. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
The design document duplicated the contract half of the ADR and the domain-language file, and its remaining half — history, delivered-slice tables, plan and deferred decisions — is what docs/README.md assigns to issues and discussions. It also cited branches that do not exist on this repository. Delete it; the narrative belongs in discussion apache#3286. Record what this slice actually settled in the two files that keep contracts: which sense of "active delegation" carries destructive authority, why visibility is proved for one delegation rather than the whole set, and why an unrecovered root can never be reported terminal. Generated-by: Claude Opus
`stop_target_not_active` and `stop_target_not_unique` were decided from the renderer's own delegation mirror, and both returned without ever calling `coordination.act` — so the Host never saw the request and never got to disagree. That mirror is filled only by the Coordination stream, which means it is empty in a second window, after a reload, and across a reconnect. In that window WorkHub told the user, specifically and confidently, that a Session had no running work while it did. Add a read-only `workhub.coordination.delegations` query and ask it once a reference has resolved to one Session, so an ordinary message never pays for the round trip. The Host answers with its active links and, for each, whether it still holds work a stop could reach — the same judgement admission makes, from the same durable state. That also removes the client-side settled-delegation mirror added for the completion-fact fix: which delegations are stoppable now has one owner instead of a renderer projection that had to agree with the Host. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
f133e26 to
70955be
Compare
`stopsActionId` made the client name the delegation to stop, so the policy had to learn one before it could submit. It did that by reading `workhub.coordination.delegations`, which reads the whole global Coordination transcript and parses every record — once per stop-shaped message, and twice more inside `#prepareStop`. The client cannot prove which link is live anyway. The Gate already resolved the assignment, checked it against the named Session, and proved sole-active-delegation from durable state under the admission lease, so the field was a weaker copy of an answer the Host was making regardless. Drop it. The proposal names only the Session it resolved, and the Gate resolves the delegation from its own active links: one link is the answer whatever state its work is in, and only several need separating, by the rule competition already used. Resolving again on replay would fail, because a resolved stop takes its delegation out of the active set — the second attempt would find nothing where the first found one. `workhub_action_claims` already records the delegation each action bound itself to, is written before any effect, is never deleted, and outlives removal of the target Session; it is read here for the first time. The claim-then-request order leaves one seam, where an action owns a stop with no request behind it, and nothing destructive has happened there, so it resolves like a first attempt. That retires the `delegations` query with its transcript scan, and with it the renderer's last stop-state judgement: `stop_target_not_active` and `stop_target_not_unique` were the two answers a client mirror could give while contradicting the Host, and the Gate's refusal now carries them. Reported by Astro-Han in review of apache#4439. Generated-by: Claude Opus
Summary
Add WorkHub Slice 5D: direct stop for one resolved, active durable delegation.
SessionResolverport that returns ranked candidates, nothing, or ambiguity — nevercreate_new, never an identity outside the caller's visible candidate setalready_terminalfor a Turn that is still runningdelegation_stop_requestedanddelegation_stop_resolvedfacts and project their outcomes into WorkHubnot_ownedasked to stop) without claiming execution completionArchitecture: the shared Session Resolver port
This addresses the architecture discussion on this PR. The pipeline is now:
SessionResolveris action-agnostic and injectable.createExactNameSessionResolveris a temporary deterministic baseline, removable once the replacement resolver passes the common routing evaluation and its rollout retains a tested rollback path.Stop and correction previously carried two near-identical copies of the same name-matching rule. They differed only in what may follow the name — a stop reference may carry punctuation and nothing else, a correction may name its target and go on to say what to do with it. That is an action's rule, not a retrieval rule, which is why folding both into the Resolver would have forced it to know which action it was serving. Instead one
matchWorkHubSessionNameowns matching and reports what text was left over; each action's policy judges that leftover. Both actions now resolve through the port.resolve()'s broader lexical retrieval is a different algorithm and joins the port with continue, inspect, and resume in the follow-up slice.Making the port real required moving admission off display names. Previously the Action Gate and the coordinator's under-lock recheck each re-parsed
userTextand proved uniqueness by matching Session names, which made presentation metadata the destructive authority and would have rejected any resolver that recalled a Session by anything else. Stop now proposes{ stopsActionId, expects: { targetSessionId } }and asserts no proof of its own; the Host proves from durable state that the assignment exists, still belongs to that Session, and that no other delegation there still holds stoppable work. A stale resolution fails closed; a rename between resolution and admission correctly no longer invalidates a claim.Destructive answers do not come from projections
WorkHub projections are rebuildable and can be empty when a window opens. Two stop refusals were decided from the renderer's delegation mirror and returned without ever calling the Action Gate, so a fresh window, a reload, or a reconnect produced a confident, specific, and false "there is nothing to stop". A new read-only
workhub.coordination.delegationsquery is asked once a reference resolves — an ordinary message never pays for it — and the Host answers which links still hold stoppable work, using the same judgement admission makes.Which sense of "active delegation" carries destructive authority
A delegation link ends only by supersession or a resolved stop, so work that simply finished stays linked. This slice is the first to let "active delegation" gate a destructive action, and it settles the question narrowly: the link stays active, but a delegation whose work has finished is not a competing stop target, because there is nothing left in it to stop. Execution state that cannot be read counts as competing, never as finished. No completion fact is added to the coordination log — target execution state remains an ordinary Session fact that WorkHub reads as a rebuildable projection.
Boundary change reviewers should weigh
The Action Gate no longer proves that trusted user text names the target.
Stop Loginpaired with a proposal pointing at the Payments delegation was previously rejected at the Gate; it now depends on the proposal being correct. What still holds:user_stopconfirmation andstopsActionIdare both outside strategy output, so no model proposal can authorize or retarget a Stop.This is the boundary the discussion proposed: the Gate owns existence, Host scope, ownership, active delegation identity, and the cancellation itself, while recall quality is the Resolver's concern. It is called out here because it is a deliberate trade, not a side effect.
stop_workproposal shapes are introduced by this PR's compatibility epoch (95), so refining the proposal needs no further epoch. Epochs through 94 remain assigned to changes already onmain.Verification
git diff --check, ASF license headers, knip at baseline, renderer architecture checkrechecks sole-delegation stop preconditions after the advisory active-link readworkhub-session-resolver.test.tsfor the port contract, andworkhub-session-resolver-port.test.ts, which swaps in a resolver that recalls by description instead of display name and shows stop producing the same typed opaque decision with no change to the durable protocolrecoveringrather thanalready_terminal; a stop answered with an unfilled delegation mirror still reaches the Action Gate; and thestoppedlink state joins the surface-flow tablenon-serving Runtime Host kernel,deleted account identity survives same-slug reuse until explicit recovery,two Clients share exact retryable Session branch and revision authority) pass in a smaller batch, andowned Host exits promptly after its first connection closesis intermittent even alone — its own comment notes its 8s election deadline and 500ms settle window are too tight under load. None touch WorkHub code.Review focus
The destructive authority remains delegation- and Message-centered, and is now identity-centered end to end: the renderer proposes only opaque identities and the state it resolved against; Session, Message, Turn, and Run identities are resolved and verified by Host-owned durable state. Pending cancellation and owning-root Stop each retain action-bound durable provenance across the request/resolution crash seam.
The most valuable review targets are the boundary change above, and the "active delegation" definition this slice settles.
docs/architecture/workhub-action-resolution-design.mdhas been deleted: its contract half duplicated the ADR and the domain-language file, and its narrative half belongs in discussion #3286.AI use
Select exactly one:
Tool(s) and scope: Codex implemented, tested, documented, rebased, and adversarially reviewed the original direct-stop change. Claude Opus was consulted during design planning, and implemented, tested, and documented the shared Session Resolver port and the identity-based stop admission.
Checklist
Does this PR entail a change in behavior?