feat(workhub): resume delegated work from the coordination conversation - #4713
feat(workhub): resume delegated work from the coordination conversation#4713ARE404 wants to merge 12 commits into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at cbfb198. The problem is real (Slice 5's exit sentence names resume), and the core of the design holds: the resume goes through the Action Gate, the Gate revalidates the active set and the named Session on the Host, and planResume/resumeDelegation call the same turn.resume.query / turn.resume.start handlers the interrupted-turn banner uses. Deriving the continuation turnId from the delegation and source run is fine too; that id has always been caller-supplied, so this is a stronger caller, not a second authority. One P1 and four P2s before it can merge, plus an ordering constraint with #4699.
P1: after a resume, Stop Payments cannot stop the work and reports it terminated. The continuation Turn is admitted with content: null and no source message (root-turn-coordinator.ts:2873-2879), so readMessageExecutionDisposition keeps resolving the delegation to the original root turn/run. retireDelegation builds its identity from that disposition (execution-composition.ts:1503-1512) and stopOwnedWorkHubRoot compares it against the active registry (:2130), where the continuation lives under wht_…. Nothing is stopped; coordinator.read(original) is failed/cancelled (that is why the resume was allowed), so the outcome is already_terminal, the fact is committed, and the delegation leaves the active set while the continuation keeps running in the target Session. replace and readDelegationRetirement take the same path, so "observe" is wrong too. Fix at the owner: the Host resolves "latest run on this root Turn's lineage" (the continuation admission already carries sourceRunId/sourceTurnId), and both retireDelegation and planResume use it. That also deletes the next item.
P2: lineage is recovered by scanning the coordination transcript, which is exactly what #4699 removes. workhub-coordination-coordinator.ts:424-432 does readMessagesSnapshot(WORKHUB_COORDINATION_SESSION_ID) to find the previous delegation_resume_resolved. That (turnId, runId) is Host-owned state mirrored into the transcript. #4699 deletes readMessagesSnapshot from CoordinationStores and replaces the unbounded scan with a paged, incremental active-assignment projection, so this hunk will not typecheck after it lands; both branches also claim epoch 113. #4699 has to go first, and this PR should rebase onto it and take the Host-side lineage from the P1 fix instead of the scan.
P2: already_running is decided from the persisted run snapshot without asking the Host, and every failure becomes parked. execution-composition.ts:1425-1431 returns already_running for admitted/created/running, which is the state a Host crash leaves behind; the stop path has a recovering outcome for exactly this window and its comment explains why. Then :1418-1422, :1446-1452 and :1473-1476 collapse a bare catch, turn.resume.query park reasons and every !ok (host_draining, internal_failure, session_archived, …) into parked and commit it as an immutable delegation_resume_resolved. Forward the Host's park reason, let operation failures surface as errors rather than facts, and give resume a recovering exit.
P2: the Host execution side has no coverage. planResume / resumeDelegation (execution-composition.ts:1396-1483) are stubbed in the coordinator test (:1636-1648) and untouched by the gate test, so lineage selection, the already_running decision, the error-to-parked mapping and the P1 path are all outside the suite. The resume_work branches in protocol/workhub-coordination.ts:541-570, 681-691 are not in the protocol test either. The P1 scenario is the natural regression.
P2: continue / 继续 / restart / 重新开始 should not be resume verbs. Continue Payments and 继续支付任务 are how people hand the next instruction to a Session; with a Session of that name the text is consumed as a resume (workhub-controller.ts:505 returns before ordinary routing) and, on already_running, the user reads "no need to resume" and their message was never sent. Restart Payments maps to the same derived continuation Turn (workhub-coordination-action-gate.ts:1163), which is the opposite of what the word asks. The PR's own comment at workhub-creation-intent.ts:113-117 names the ambiguity; the name-match guard does not help when the whole message is the verb plus the name. Keep resume / 恢复 / 接着跑, drop the other four.
Smaller, in one pass with the above:
workhub-controller.ts:505-556is:562-616with four tokens changed; route-policy already extractedresolveNamedDelegationActionfor the same reason. OnesubmitNamedDelegationActionhelper takes both to ~6 lines. Same in core:directWorkHubResumeCue/Targetcopy the stop pair line for line, and reuseUNSAFE_STOP_TARGET/stripMatchingStopQuotesunder stop's name.WorkHubRequestIntent.resume.cuehas no reader thatimperativedoes not already answer (workhub-route-policy.ts:190vs:255).whr_+ sha256(actionId) is already the runId prefix forassign; here it names a message id.workhub-surface.tsx:586-588: the frame state readsstop?.outcomebut notresume?.outcome, so a parked resume renders as a completed turn.- The body is out of date on three facts: epoch is 112 → 113, migration 39 rebuilds
workhub_action_claims(SQLite cannot widen a CHECK in place; the migration itself is written correctly), and the Host-side stop does not useworkHubNamedDelegationActionTargetsSession. Please rewrite the compatibility section before squash.
This is a user-visible surface (three new outcome states, two clarification prompts, three locales) with no before/after evidence in the body. Manual checks that remain after the fixes: delegate → interrupt → Resume X → Stop X, confirm the continuation actually stops and the card reflects it; kill the Host mid-run and resume after restart, confirm it does not answer already_running; a parked resume's frame state.
Evidence boundary: static read at cbfb198 against c180a2bac3 plus the #4699 branch; git merge-tree for the conflict; no build, no tests, no Desktop launched. The P1 chain is derived from the admission shape and isActiveWorkHubRoot, not observed.
AI-assisted review: drafted with Maka; I verified the retirement identity path, the transcript scan and the epoch/migration facts myself.
github-actions
Bot
added
effort/XL
4477263 to
6c62c31
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 6c62c31, reading the PR's own delta over the #4699 commit it sits on (28 files, +2159/−157). Every item from the last round is closed in code:
- P1:
readLatestRootTurnLineage(root-turn-coordinator.ts:523) walkssafe_boundary_continuationadmissions forward from the Host's own ledger, andreadDelegationRetirement,retireDelegationandplanResumeall use it. The delegate → interrupt →Resume→Stopregression goes through the production composition and asserts the continuation iscancelled. - Transcript scan: gone. Lineage comes from the Host; the two new coordination messages carry the plan and its outcome, and
sourceRuntimeEventHighWatergives the persisted plan a real job on replay. Migration 39 rebuildsworkhub_action_claimsinside oneBEGIN IMMEDIATE, no index or FK on that table, so it is correct. already_runningnow asks the Host;recoveringthrowsoperation_unavailablewithout writing a fact; park reasons pass through;!okthrows.planResume/resumeDelegation, the Gate and theresume_workprotocol branches are under test through their owners.- Verbs are
resume/恢复/接着跑only, with the four rejected forms asserted negative. - The duplicated controller blocks,
cue, thewhr_reuse,resume?.outcomeon the surface and the three stale body facts are all fixed.
One new P1, then three P2s, then ordering.
P1: in a default build, resume always fails and records the failure as a durable fact. safeBoundaryResumeEnabled is process.env.MAKA_RUNTIME_SAFE_BOUNDARY_RESUME === '1' (execution-composition.ts:1024), off unless set. With it off the kernel skips continuation inspection (runtime-kernel.ts:647) and turn.resume.query parks with resume_feature_disabled; planResume turns that into { kind: 'parked' } (execution-composition.ts:1432), and the coordinator commits delegation_resume_requested + delegation_resume_resolved for it, then the surface says "Could not carry this work on. Open its Session to see why". So on the shipped configuration the new command fails every time, sends the user to a Session that has nothing wrong with it, and leaves two immutable records per attempt. The one useful fact, the park reason, reaches WorkHubSubmission.parkReason (workhub-controller.ts:413) and nothing reads it. Treat resume_feature_disabled like recovering: no persisted fact, operation_unavailable, and a clarification that says the runtime has resume off (the CLI already has the wording at pi-tui-runner.ts:348). The body's Behavior section should state the flag.
P2: CI is red on formatting. Biome wants packages/core/src/session.ts:1729-1730 on one line; npm run format. The body says lint passed, which is a different check.
P2: resume and stop disagree on what "one delegation on this Session" means. #resumeSource (workhub-coordination-action-gate.ts:618-627) rejects whenever onTarget.length !== 1. #stopSource (:689-700) first drops delegations whose work is retired and accepts if one is still holding work. Delegate to Payments twice, let the first finish, interrupt the second: Stop Payments resolves, Resume Payments answers "this name matches several pieces of work". That is the rule the PR's own route-policy comment says must not happen, broken on the Host side. Reuse stop's retirement filter in one helper.
P2: lineage resolution reads the Session's whole root-admission ledger. readLatestRootTurnLineage calls listRootTurnAdmissionsForRecovery(sessionId), which materialises and chain-validates every root admission (up to 1 MB each) for the Session, and it now sits under readDelegationRetirement, which the stop path calls once per active delegation on the target. A long-lived Session pays an unbounded read on every stop, replace or resume; that is the shape #4699 just removed from the transcript, moved to a different ledger. Add a store query that returns only safe_boundary_continuation admissions for the Session (or walks forward from a given turnId) instead of the full chain.
Smaller:
parkReasononWorkHubSubmissionhas no reader outside tests; either it lands on screen with the P1 fix or it goes, same ascuelast time.isWorkHubCoordinationFailurereplaces the deleted re-export with a new export;workhub-surface.tsx:30-38already imports from the controller, so a direct value import of the class is one fewer concept. #4822's ask was three lines deleted, not one deleted and one added.- In
resolveNamedDelegationActiontheunnamedReasonbranch is unreachable for resume (imperativeis only true with atarget), and the twokind === 'resume' ? … : …return blocks insubmitNamedDelegationActionhave identical structure.
Ordering with #4699: unchanged, #4699 first, then rebase; the head is on an older #4699 commit and a5e1832283 has landed on that branch since. git merge-tree reports five conflicts, four mechanical (epoch table in protocol/index.ts, adjacent hunks in execution-composition.ts, two test files). The one that needs care: a5e1832283 changes sessionActions.assign from returning committedAssignment to taking a publishCommittedAssignment callback, in the same lines where this PR extracts the CoordinationSessionActions type (workhub-coordination-coordinator.ts:139-156); redo the extraction on the new signature. Keep #resume's beforeAppend calling the internal #listActiveAssignments as stop does, not the #admission.run-wrapped effect, or it re-enters the lease.
Manual acceptance after the fixes (three new outcome strings, two clarifications, three locales, no screenshots in the body): default build with the flag unset, Resume X should clarify rather than record; flag on, delegate → interrupt → Resume X → Stop X and watch the card; kill the Host mid-run, restart, Resume X should be retryable, not already_running; the two-delegations case above; parked frame state, light and dark, before/after.
Evidence boundary: static read of 6c62c3147e against 29367f7b1c and a5e1832283, plus one read-only biome format check; no build, no suites, no Desktop.
AI-assisted review: drafted with Maka; I verified the flag path, the resume/stop asymmetry and the conflict set myself.
Generated-by: Codex
Generated-by: Codex
Resume asks the Host to carry on work an interruption left unfinished. It is admitted on the same terms as a stop — a direct speech act naming one existing Session — so it reuses the stop reader's rules: a question is not a command, a malformed literal is refused, and an anaphoric target carries the cue without claiming a target so the surface can ask which work rather than guess. English covers resume / continue / restart and Chinese 继续 / 恢复 / 接着跑 / 重新开始, the same colloquial range each language already has for stop. A cue alone resumes nothing: the reference must still resolve to a Session, which is what keeps `continue with the refactor` ordinary work. This is the reader only. The disposition that consumes it lands with the Gate admission in the same change. Generated-by: Claude Opus
Stop was the only way WorkHub could change delegated execution, so work an interruption left unfinished could only be restarted by leaving the conversation, opening the Session and using its own banner. Slice 5 asks for resume from the coordination transcript, and the capability to do it already exists. Resume composes the two operations that banner uses — ask the Host whether this Session has a continuation to make, then make it — and adds no recovery machinery of its own. A repeat is safe because the Host parks a continuation that already exists rather than forking a second one, so the Turn identity is derived from the delegation and the source run rather than minted, and two attempts name the same Turn. It goes through the Gate for the same reason every other disposition does: it changes execution state, and the Gate is where that is admitted. It carries no confirmation, because it destroys nothing and grants no authority a delegation did not already grant — it proves only that the words named one Session and that the Session still owns one link. `WorkHubActionOperation` gains `resume` so one action identity still means one operation. That is a durable vocabulary change with no migration: the column already exists, and only a Host that predates this value would refuse to read a claim carrying it. Refs apache#3492 Generated-by: Claude Opus
The Host could resume interrupted work, but only its own Session could ask: the user had to leave WorkHub, open the Session and use its banner. This wires the conversation to the disposition the Gate already admits. Stop and resume ask the same question of the same words — which visible Session does this reference name — so they now ask it with one matcher. Two copies would be two chances for `Resume Payments` and `Stop Payments` to disagree about which Session they mean. They diverge on what an unnamed reference means, and that asymmetry is the point. A stop must answer it: stopping is destructive, so `Stop it` has to be met with a question rather than a guess. A resume must not: `继续这个工作` is how someone carries on with the Session they are already in, and answering it would take an ordinary instruction away from ordinary routing. Resuming nothing costs nothing, so an unnamed resume falls through and only a named one becomes an action. The three answers the Host can give are all reported as themselves. `already_running` and `parked` are outcomes, not failures: the first says the work never stopped, and the second is the Host declining, whose reason is its own. Refs apache#3492 Generated-by: Claude Opus
|
Following up on the findings above with the thing that should come first: please reshape this PR to the smaller form below before addressing the P1/P2s individually, because most of them disappear with it. The delta over #4699 is +2159/−157 for a command whose whole job is "find the interrupted delegation on X, call the same 1. Resume copies stop's two-phase "write the intent, then execute, then write the result" for an action that is not destructive. That is what forces 2. The kernel's park reason is treated as a domain fact that must be persisted and transported. It moves from What survives, and why: the intent verbs and The shape I am asking for, concretely: one This also folds the P1 in naturally: with no persisted request, Evidence boundary: static read of the delta plus the |
87b4c0b to
db17b3f
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head b2289fba53905374c717758b84a513d28bf20a5d (OPEN). One P1 and one P3 below. This is a feature; merging is a human decision regardless. Scope note: schema migration and protocol boundaries were reviewed line by line; the WorkHub controller/route-policy/surface files (~10 files) were not individually reviewed — flagged below, not covered.
P1 — the new unique index can break opening existing databases
sqlite-core-execution-schema.ts bumps the version 7 → 8 with a unique partial index on (session_id, sourceTurnId, sourceRunId) for safe_boundary_continuation rows. But that kind is not new — it already exists on main (runtime-invocation.ts:263) — and today's admitRootTurn dedupes only on turn_id, never checking the source triple. So two continuation records from the same boundary with different turn_ids can legally coexist today — exactly what the index wants to forbid.
If any installed database already holds such a pair, CREATE UNIQUE INDEX throws inside a bare db.exec that runs at database open (operational-state-store.ts:460, operational-target-schema.ts:71) — the outcome is not "feature unavailable" but "database won't open". There is no dedup or backfill step, and the new test only exercises the happy path on a fresh empty database. Whether such duplicates exist in the wild depends on whether historical flows could produce two turns from one boundary — that is for the author/maintainers to confirm, and the severity hinges entirely on the answer: impossible → P3 (missing migration test); possible → release-blocking.
Suggested fix: dedupe first (keep earliest or latest), or build a non-unique index and enforce uniqueness on the write path; either way, add a migration test with pre-existing duplicates.
P3 — epoch 113 is claimed by two open PRs; merge order matters
Main is at epoch 112. This PR writes 113 ("WorkHub exposes current active delegation linkage") plus 114 for itself — i.e. it stacks on a not-yet-merged sibling. Meanwhile PR #3299 independently claims 112 → 113 for a different reason. Whoever merges first takes 113; the other must renumber, or the epoch comments (the only trail for future cross-version handshake debugging) will mismatch real history. This is a merge-ordering dependency for planning, not a defect of this PR.
Verified good (worth stating)
The session-metadata migration 39 is exemplary: the comment explains why the table must be rebuilt (SQLite cannot relax a CHECK in place); rename-then-rebuild loses nothing (the v38 table deliberately had no indexes or foreign keys); explicit column lists on INSERT and SELECT; runs in a transaction (BEGIN IMMEDIATE); and a version-vs-entries-count assertion catches forgotten bumps. Likewise, readRootTurnContinuationAdmission re-validates the three fields after the index lookup instead of trusting json_extract blindly.
What I could not judge
The app was not run; resume-flow interaction unobserved. Whether duplicate boundary records exist in the wild (see P1) needs author/maintainer confirmation.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff；我核的是当前 head 有没有漂移。当前 head 是 b2289fb，未关闭。P1 是新唯一索引可能让已有数据库打不开，要作者确认历史数据形态。P3 是 113 纪元被两个 PR 同时占用，合并顺序决定谁改号。功能类合并由人类拍板。
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up to my review above, same head b2289fba53905374c717758b84a513d28bf20a5d: this covers the controller/UI portion that the first review explicitly left out (supplemental review of workhub-coordination-port.ts, workhub-controller.ts, workhub-route-policy.ts, workhub-surface.tsx, preload and related copy). One P2 and two P3s; still no P0–P1 from either part. Feature — merging remains a human decision.
The biggest win here is deleted code: the renderer used to replay the whole coordination history (rebuildCompleteHistory: loop loadBefore until no older, then loadAround) on every reset — linear in conversation length — just to derive current state. Now it asks the host (deps.candidates()), which owns the authoritative data. Direction is right, which is exactly why the P2 below is worth fixing.
P2 — one Host IPC per transcript batch, with no coalescing
At the end of the batch callback, if (handle && (reset || changed || batch.ready)) scheduleRefresh(refreshGeneration) queues one deps.candidates() onto the serial refreshLane — per batch. Bursty transcript batches therefore produce one Host round-trip each. Please coalesce (e.g. trailing-edge or generation check) so a burst resolves to a single refresh.
P3 — resume with a verb but no clear object gets no feedback at all
Entry handling is asymmetric: stop (workhub-route-policy.ts) with a cue but no usable target falls into a clarification branch that asks the user back; resume with if (!action.imperative || !action.target) return { kind: 'not_requested' } silently treats "keep working on that thing" as never requested — the message lands as ordinary chat and the user likely believes an instruction was issued. Please clarify or execute.
P3 — resume_target_unavailable merges a transient failure with a permanent state
workhub-controller.ts:425-429 maps both operation_unavailable and host_not_ready to the same clarification. But host_not_ready is transient (host still starting; retry helps) while operation_unavailable is capability-based (this host never enabled safe-boundary resume; retry never helps). Please distinguish them so the user knows whether to wait or stop trying.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条是上一条的补充，同样来自 @Opus-Qronos-AstroHan 的审查，覆盖之前声明没审的控制器界面部分。P2 是每批转录都发一次 Host 请求没合并，另两条 P3 是无目标恢复没反馈、临时和永久失败混为一谈。功能类合并由人类拍板。
2 participants
Summary
WorkHub could delegate, observe, correct, and stop work, but it could not carry interrupted delegated work on from the coordination conversation. This PR adds a narrow named command—
Resume Payments/恢复支付任务/接着跑支付任务—that reuses the Host's existing safe-boundary resume operations.Refs #3492
Behavior
Continue Payments,继续支付任务, and restart requests remain ordinary work.turn.resume.query/turn.resume.start, and records onedelegation_resumefact with eitherresume_startedoralready_running.MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1) or the Host is recovering, WorkHub clarifies without recording a resume fact.Design boundary
The renderer owns trusted text parsing and Session-name resolution. The Action Gate owns admission and durable action identity. The Runtime Host owns execution lineage, recovery state, and
turn.resume.query/turn.resume.start.Resume does not copy stop's two-phase durable request/result protocol because it does not retire a durable link. Replay safety comes from the action claim, deterministic continuation Turn identity, and Host admission idempotency. Stop and resume share the same named-Session resolver, submission path, and non-retired-delegation selection; only stop carries destructive confirmation.
The implementation is rebased on the latest #4699 history. Relative to that dependency tip, the current PR delta is +848/-158 production lines and +927/-16 test lines. The ablation removed the persisted resume plan/request, request replay path, cross-layer park-reason type and transport, unused renderer state, duplicate controller branches, and tests that existed only for those mechanisms.
Compatibility
RUNTIME_HOST_COMPATIBILITY_EPOCHis 114. The closed resume wire result has two outcomes and no park reason, so peers from another epoch are rejected during handshake.SQLite schema migration 39 rebuilds
workhub_action_claimsto widen its CHECK constraint with theresumeoperation, preserves existing rows, and removes the v38 table. SQLite cannot widen that CHECK constraint in place.Verification
Observed locally after the review fixes and ablation:
Resume Payments→Stop Payments; the continuation becomes cancelled and the stop targets its Turn.Resume Paymentsreturnsoperation_unavailableand writes no resume fact.git diff --checkpass.Not performed as manual evidence: launching the full Desktop and killing its Host process interactively. The equivalent authority paths above are covered by automated Host/production-composition tests. Packaging remains CI-owned.
AI use
Tool(s) and scope: Claude Code wrote the original implementation and tests under human direction. Codex analyzed review feedback, implemented the Host-lineage and recovery fixes, rebased on #4699/current main, performed the ablation, and executed the reported validation.
Checklist
Does this PR entail a change in behavior?