Conversation
Add HandoffExecutor — the reusable runtime operation for formal workflow ownership transfer via the handoff contract (#922). One operation: 1. validates task/run state (terminal runs reject handoffs), 2. resolves the sender node + declared transition (resolveHandoffTransition), 3. resolves target node(s)/slot(s) from the transition target, 4. authorizes the declared channel topology (ChannelResolver; open topology permits all), 5. enforces cyclic transition maxCycles, 6. authorizes + commits the transition's declared gate fields (GateDataRepository + evaluateGate), rejecting data keys outside the gate's writable shape, 7. runs the transition's hook validator (HookExecutor), 8. activates/reuses the target worker session (existing activateTargetSession) and delivers the existing peer-message envelope (formatAgentMessage), queueing when the target is not yet live, 9. returns a structured delivered/queued/blocked/failed result. Reuses ChannelRouter/ChannelResolver, gate evaluation, the hook engine, and existing target-activation code rather than duplicating transition logic. Authoritative fresh-turn packet construction (and the sender round-completion that accompanies a delivered handoff) is a later task; this does not alter generic send_message semantics. Adds a handoff_cycles table (migration 187) + HandoffCycleRepository mirroring channel_cycles, keyed by (run_id, transition_key). Handoff cycle caps reset alongside channel cycles on human touch. Registers the table in the db-query space scope and the schema-parity allowlist. Focused runtime tests cover authorization, gate/hook failure + success, cycle limits, queued activation, terminal-run rejection, broadcast, and the pure cyclicity/target-resolution helpers.
P1#1 (cycle burned on failed delivery): switch from post-delivery increment to reserve-before-delivery — the atomic increment both checks the cap and reserves in one step (closing the check-then-increment TOCTOU two concurrent handoffs had), and a failed delivery refunds the reservation so only TAKEN handoffs (delivered||queued) count. Adds HandoffCycleRepository.decrement. New cyclic+failed-delivery test asserts the cap stays intact across failed attempts and a later successful handoff still proceeds. P1#2 (execute() never-throws): wrap the throw-prone paths (gate merge/eval, hook validator, target-session lookup) with a typed HandoffExecutionError(stage) caught at the top level of execute(), so a DB/script/validator throw maps to a structured `failed` result instead of escaping to the caller. P2: hoist listByWorkflowRun in deliver (one read up front + one per activation, not 2× per slot). Surface partial-delivery diagnostics on the delivered branch. Add coverage for cancelled run state, gate rateLimited propagation, hook retryable_block propagation, partial delivery, autonomy-path gate write (sufficient + insufficient), and the throwing-validator never-throws path. P3: drop the redundant gate-row re-read (use the merge return's updatedAt). 31 focused runtime tests pass; typecheck/lint/knip/parity and the runtime shards green.
Resolve migration-number collision: dev shipped M187/M188/M189 for delivery (#2463), so renumber the handoff_cycles migration 187→190 (function, registration, index export, test-helper comment). All other files auto-merged cleanly. typecheck/lint/knop/parity green; migration shards and handoff tests pass.
Addresses the self-contained points from the inline review: - Cap-before-side-effects (P1): read-only isCapReached check now runs BEFORE gate/hook, so a capped cyclic transition no longer persists gate data or runs hook validators on a guaranteed cycle_limit. The atomic reservation still runs right before delivery (TOCTOU-safe). - Dedupe cycle (P2): a DEDUPED enqueue (message already pending from a prior attempt) no longer charges a cycle; only new deliveries/enqueues count as taken. - Self-target rejection (P1): a transition that resolves to the sender's own node is rejected — queueing it back into the sender's live session would violate the round-boundary contract. - Run-state recheck (P2): reload run state immediately before delivery so a cancellation during async gate/hook validation doesn't deliver into a terminal run. - Hook workspace (P1): carry config.workspacePath into hook context so validators/script hooks resolve the task worktree, not the daemon cwd. - Hook rawParams (P1): populate rawParams with the full operation payload so built-in validators reading (rawParams ?? params).data see the supplied fields (e.g. pr_url). - Hook authorization (P1): enforce enabled/humanOnly/sourceNode/targetNode/ authorizedCallers before executing, mirroring WorkflowHookEngine. Two new tests (hook-auth block, self-target rejection); 33 pass. Batch 3 (node-scoped delivery, cyclic resetOnCycle gate reset, gate-data publication, stale-execution filtering, and routing handoff hooks through WorkflowHookEngine for patch_params/task-phase/frozen-identity/external- lookups, plus wiring the handoff tool) is tracked with the reviewer.
- Stale executions (P2): exclude pending/cancelled node-executions from live-session lookup so a retained dead agentSessionId (e.g. after a spawn retry) is not selected over activation. Mirrors the production activation path's status filter. - Data without declared shape (P2): reject `data` keys on a transition that declares neither a gate nor a hook — the contract requires keys to come from the bound gate fields or hook template fields. Two new tests (stale-execution skip, ungated/unhooked data rejection); 35 pass.
Adds the non-blocking P3 from the approving review: enqueue the same handoff twice and assert the 2nd (deduped) outcome is still `queued` while the cycle counter stays at 1 (the dedupe path is refunded, not charged). 36 tests pass.
…#923] dev removed the legacy gate subsystem entirely (M190 drops gate_data / gate_open_state / space_workflows.gates; gate-data-repository, gate- evaluator, gate-features, gate-open-state-repository deleted; HandoffTransition.gateId removed — handoffs now authorize via hooks only, matching dev's new direction). Reconcile: - Renumber the handoff_cycles migration 190→191 (dev shipped M190 for the gate removal). Registered in runMigrations; index.ts kept at dev's export set (no per-migration re-export needed). - Strip the gate path from HandoffExecutor entirely: drop commitGate, GateDataRepository/evaluateGate/getEffectiveGate/scriptExecutor deps, gateOutcome from the result, and the gate-commit tests. Authorization is now hook-only (transition.hookId); data keys require a bound hook. - scope-config.test / parity / space-test-db: drop gate_data & gate_open_state, keep handoff_cycles (dev + this PR). 29 handoff tests pass; typecheck/lint/knip/parity green; migration, handlers, and runtime shards green (one unrelated flaky git-worktree test in space-worktree-manager, stable in isolation).
…eness) [#923] - Hook template data (P2): pass `hook.templateData` into the validator context so hooks with template-defined values see them. - Delimiter-safe cycle keys (P2): URI-encode the node/transition ids in the cycle-counter key so a literal '/' in an id can't collide. - Stricter live-session filter (P1): only `in_progress`/`idle` executions count as having a usable session — terminal (done) / blocked / waiting_rebind rows can retain a dead session id and must not be selected over activation. Extends the prior pending/cancelled guard. 29 tests pass; typecheck/lint green.
…ndoff tool [#939] Follow-up to the runtime handoff transition (#923, PR #2466) addressing the five deferred review points. Handoffs are hook-only after dev's gate-subsystem removal, so transition-hook execution now routes through WorkflowHookEngine (which already owns patch/state/connector-auth/task-phase) instead of HookExecutor directly, and the agent-facing handoff tool is wired in. 1. Wire a handoff({ target, summary, data? }) tool into node-agent-tools (HandoffSchema + handler + conditional registration). TaskAgentManager constructs the HandoffExecutor and injects it alongside agentMessageRouter. 2. Node-scoped delivery: resolve node/slot pairs and scope activation, live-session lookup, enqueue (idempotency key + persisted workflowNodeId), and auto-resume by the resolved node id so a shared slot name across two nodes can no longer leak. 3. Apply a successful patch_params result to the handoff payload before delivery (e.g. pr_ready discovering data.pr_url). 4. Populate taskStatus / frozenPrUrl / persisted hookLocalState via the engine context builder (post_approval_only / pr_ready post-approval branches). 5. Derive permittedExternalLookups from the hook validator (connector auth) so authenticated validation works on the handoff path. Engine additions: runDeclaredHook (run a single transition-bound hook by id through the shared pipeline), persistHookOutcome (the post-action persist, now shared by the MCP wrapper and the executor), and generalized the pr_ready identity-stamp + target-patch-strip gates to also cover the handoff method.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5424fb456d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (HyperNeo)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: z.ai
Reviewing the #939 follow-up (commit 5424fb456) — routing transition-hook execution through WorkflowHookEngine instead of HookExecutor direct, plus the agent-facing handoff tool and node-scoped delivery. I read the full follow-up diff, the integrated state of the new files, traced the runtime integration (TaskAgentManager construction, activateTargetSessionsForMessage, flushPendingMessagesForTarget, the queue repo), and the tests. (Note: this PR is stacked on #2466; my focus is the single follow-up commit, but I reviewed the shipped state of the touched files.)
Verdict: the implementation is correct and the five deferred points are genuinely addressed. This is a clean, well-factored change. The findings below are test-coverage gaps, not defects — the code does the right thing, but several subtle/security-adjacent behaviors aren't locked in by a regression test, so a future refactor could silently regress them.
What's right (verified)
- #2 node-scoped delivery is genuinely end-to-end. I traced every hop: enqueue persists
workflowNodeId+ the idempotency key is node-scoped;activateTargetSessionsForMessagefiltersmatchesNodeand re-validates the slot belongs to that node; live-session lookup (sessionsFor) filters bye.workflowNodeId === workflowNodeId; andflushPendingMessagesForTargetderives the drain node from the session's execution (execution?.workflowNodeId) and even gates the execution-less (merger/post-approval) path to legacy null-node rows only. A shared slot name across two nodes cannot leak at any hop. This is the strongest part of the PR. - #3 patch_params merge (
{ ...operation.data, ...fp.data }) preserves sender keys and overlays the patch; the patched payload flows intodeliver/envelope/idempotency-key consistently. - #4/#5 context enrichment flows from the shared
buildExecutorContext—taskStatus(viagetTaskStatuswired inTaskAgentManager),frozenPrUrl(resolveFrozenPrUrl),hookLocalState(loaded +recentResultRef), andpermittedExternalLookupsderived fromgetBuiltInConnectorDeps(hook.validator.id)rather than hardcoded. I confirmed the engineTaskAgentManagerconstructs actually wiresgetTaskStatus/resolveFrozenPrUrl. - The dropped
targetNodeauthorization check is justified. InresolveMatchingHooks,hook.targetNodeis a discovery filter for method-matched hooks, not an auth gate. For handoffs the hook is named by id via the transition contract, and the transition's declaredtarget(validated byresolveHandoffTransition) + channel auth (ChannelResolver) ARE the target authorization. Keepingenabled/humanOnly/sourceNode/authorizedCallers(fail-closed) is the correct subset. targetis genuinely un-patchable on the handoff path (methodHasRoutableTargetincludes'handoff', strip + warning fire).- Migration 179 (
ALTER TABLE pending_agent_messages ADD COLUMN workflow_node_id TEXT) is idempotent (tableHasColumnguard), nullable/no-backfill, safe on a hot table.HandoffCycleRepository.increment/decrementare atomic UPSERTs — race-safe under concurrent handoffs.
Findings
P2 — handoff is missing from METHOD_PARAM_SCHEMAS, so patched params aren't validated on the handoff path. validatePatchedParams('handoff', …) early-returns [] because the method isn't registered, while every other routable method (send_message, save_artifact, …) validates patch_params results against its zod schema. HandoffSchema already exists in the sibling schemas module; adding handoff: HandoffSchema is a one-line defense-in-depth fix so a hook can't inject a malformed data/summary shape. (The executor does type-guard each field individually, so this is hardening, not a live hole — hence P2 not P1.)
P2 — No regression test locks in three of the five points on the handoff path. The executor's hook tests use stubHookEngine, which fabricates a HookActionOutcome and bypasses runDeclaredHook/runHookPipeline/buildExecutorContext entirely. That's fine for isolating the executor, but it leaves the engine-executor seam untested for handoff:
- #4/#5: No
runDeclaredHooktest captures the validator context to asserttaskStatus,frozenPrUrl, andpermittedExternalLookupsare populated. The existing context-capture tests useexecuteAction(thesend_messagepath); thepermittedExternalLookupstests hand-set the field and only exerciseHookExecutorconsumption, not the engine's derivation. IfrunDeclaredHookstopped routing throughbuildExecutorContext, no handoff test would fail. - #3 (target-strip):
methodHasRoutableTarget('handoff')strips a patchedtarget, but this is only asserted forsend_message(workflow-hook-engine.test.ts:637). No test patchestargetviarunDeclaredHook(…, 'handoff', …)and asserts it survives unchanged. If'handoff'were dropped from the set, nothing would catch it.
P2 — No test for the node-scoped idempotency key across two same-named slots. The node-scoping tests (handoff-executor.test.ts:946, :977) cover live-delivery scoping and that the queued row carries workflowNodeId. But there's no test with two nodes sharing a slot name where a handoff to node A is enqueued and a handoff to node B (same slot, same message body) is also enqueued, asserting two distinct rows (i.e., they don't dedupe against each other). The nodeId in the idempotency-key tuple is the field that prevents cross-node dedup; if it were removed, no test would fail. (The real queue-drain node filter is also only tested at the repo-SQL level, never through the live flushPendingMessagesForTarget derivation — lower priority, but related.)
Suggested additions (non-blocking shape)
- Add
handoff: HandoffSchematoMETHOD_PARAM_SCHEMAS. - One
runDeclaredHooktest with a context-capturingHookExecutorassertingtaskStatus/frozenPrUrl/permittedExternalLookupsare populated on the handoff path. - One
runDeclaredHook(…, 'handoff', …)test asserting apatch: { target: 'x' }is stripped. - One executor test: two nodes sharing a slot, enqueue to both, assert two distinct pending rows.
No P0/P1. The core change is correct; these are about locking in the subtle behaviors so they survive the next refactor. Recommendation: REQUEST_CHANGES (own-PR fallback marker).
Address the P2 test-coverage / hardening gaps from review of #939 (PR #2472). No behavior change beyond defense-in-depth: - Add `handoff` to METHOD_PARAM_SCHEMAS so a hook's patch_params is validated against HandoffSchema like every other routable method (the executor's per-field type guards remain the primary check). - runDeclaredHook: capture-context test asserting taskStatus / frozenPrUrl / permittedExternalLookups are populated on the handoff path (points #4/#5), so routing through buildExecutorContext is regression-guarded. - runDeclaredHook: test that a patched `target` is stripped on the handoff path (methodHasRoutableTarget), matching the existing send_message assertion. - runDeclaredHook: test that an invalid patch (empty summary) is rejected by the schema re-validation. - HandoffExecutor: test that two same-named slots across nodes queue as two distinct rows (the node id in the idempotency key prevents cross-node dedup).
|
Thanks for the review — all four are addressed in
Re-requesting review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48269b43f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… slots, cycle graph, method admission [#939] Address the inline review threads on PR #2472 that fall in scope of the handoff-executor / hook-engine work. (No P0/P1; the human review summary approved.) - Side-effect hook decision (#3): the executor now keys the block/retry decision off the engine's reduced HookActionOutcome.decision, not the raw per-hook result.type, so a side_effect hook that returns block is non-blocking on the handoff path too — matching send_message semantics. - Sibling slots (#4): the self-target check is scoped to targets that resolve back to the sender's OWN slot/session, not the whole node, so a handoff to a distinct sibling slot in a multi-agent node is no longer falsely blocked. - Cycle detection (#8): handoff cyclicity is derived from HANDOFF TRANSITIONS only (control flow), not messaging channels — a one-way handoff A→B with a discussion channel B→A is no longer misclassified as cyclic. - Method admission (#9): add 'handoff' to WorkflowHookMcpMethod (shared type), VALID_METHODS (workflow-hook-validation), the import/export zod enum (export-format), and the visual-editor method dropdown, so a transition-bound hook can declare method: 'handoff' and pass validation/import/editing. Tests: side-effect-non-blocking, sibling-slot handoff, channel-not-cyclic, and handoff-method-admitted. Lint/typecheck/knip/format green; the only shard failure is the pre-existing flaky space-worktree-manager git-worktree test.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (HyperNeo)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: z.ai
Re-review of the two new commits (b48269b43 + 3c8c5781b) at head 3c8c5781b. I read both diffs in full, traced each fix into the engine, and confirmed the tests. Verdict: APPROVE. Head is stable and CI is fully green (All Tests Pass + Coverage Quality Gate + every daemon/space shard).
Previous-round findings — all resolved (b48269b43)
handoff: HandoffSchemaadded toMETHOD_PARAM_SCHEMAS(+ import) →patch_paramsnow validated on the handoff path like every other routable method. ✅- New
runDeclaredHookcapture-context test assertstaskStatus/frozenPrUrl/permittedExternalLookupsare populated on the handoff path (and thatpermittedExternalLookupsis derived,length > 0). ✅ - New test asserts a patched
targetis stripped viarunDeclaredHook('handoff'). ✅ - New test enqueues to two same-named slots across nodes and asserts two distinct pending rows — locks in the node-scoped idempotency key. ✅
New follow-up fixes (3c8c5781b) — all verified correct
#3 side-effect decision (this was a real latent bug — good catch). The executor previously keyed block/retry off raw result.type, so a side_effect hook returning block would have wrongly blocked the handoff. The fix switches to hookOutcome.decision. I confirmed the engine only sets blockedByValidation for validation-classification hooks (a side_effect block is recorded but does not stop — workflow-hook-engine.ts:701-704), so the reduced decision is the correct signal and now matches send_message semantics. The blockReason/retryAfterMs extraction stays consistent with decision because for a single declared hook executionLog[0] is the relevant hook and decision === 'retryable_block' only occurs when that hook's result was retryable_block.
#4 sibling slots. Self-target check now scoped to the sender's own slot (t.nodeId === sourceNode.id && t.slot === fromAgentName) instead of the whole node — a handoff to a distinct sibling slot in a multi-agent node now delivers correctly. Broadcast semantics are unchanged (broadcast already excludes the sender's own node, so no sibling-leak regression there).
#8 cycle graph. buildNodeGraph now uses handoff transitions only (control flow), not messaging channels. This fixes a genuine false-positive (a one-way handoff A→B with a discussion channel B→A was misclassified as cyclic, so maxCycles would have wrongly capped it). No false-negative risk: without a transition path back there is no ownership loop to cap. Sound model.
#9 method admission. 'handoff' added to all four sites — WorkflowHookMcpMethod (shared type), VALID_METHODS, the export zod enum, and the visual-editor MCP_METHODS — with a validateWorkflowHooks acceptance test. The web MCP_METHODS derives from the shared type, so it stays consistent.
Notes
- The side-effect test uses
stubHookEngine({ decision: 'allow' }), which proves the executor keys offdecision(the scope of this PR's change) rather than the real engine's reduction. That's the right scope here — the engine's side_effect-non-blocking behavior is pre-existing and shared with send_message viarunHookPipeline, so re-testing it would be redundant. - The 9 deferred items each have a defensible rationale and are genuinely outside #939's five-point scope (sender round-completion, undeclared-data-key enforcement, etc. are documented as later work). No objection to deferring.
Clean follow-up. Recommendation: APPROVE.
|
Closing: owning task was cancelled and nothing is driving this PR. Branch kept for reference — reopen via a fresh task if this work is resumed. |
|
Reopening: the owning task was mass-cancelled by the 2026-08-16 space-stop incident, not deliberately abandoned — the earlier closure mistook incident quiescence for dead work. Task restoration pending the update_task status capability. |
|
Closing: stacked on #2466, which is closed; see that PR for the re-slice note. Branch retained. |
1 participant
Follow-up to the runtime handoff transition (#923 / PR #2466) addressing the five review points that were deferred when the executor landed. This branch is stacked on top of #2466's branch (which is still open), so the diff includes #2466 — the follow-up itself is the single commit
feat(space): route handoff hooks through WorkflowHookEngine + wire handoff tool [#939]. If #2466 merges first, this PR collapses to just that commit.Transition-hook execution now routes through
WorkflowHookEngine(which already owns patch/state/connector-auth/task-phase) instead ofHookExecutordirectly, and the agent-facinghandofftool is wired in. The five points: (1)handoff({ target, summary, data? })tool in node-agent-tools withTaskAgentManagerconstructing+injecting theHandoffExecutor; (2) node-scoped delivery via node/slot pairs (activation, live-session lookup, enqueue + idempotency key, and auto-resume all carry the resolved node id) so a shared slot name across two nodes can't leak; (3) a successfulpatch_paramsresult is applied to the payload before delivery; (4)taskStatus/frozenPrUrl/persistedhookLocalStatenow flow via the engine context builder; (5)permittedExternalLookupsis derived from the hook validator so authenticated validation works on the handoff path. Engine additions:runDeclaredHook,persistHookOutcome, and the pr_ready identity-stamp / target-patch-strip gates generalized to the handoff method.Daemon typecheck/lint/knip green; runtime-a/runtime-b/agent-other shards pass (639 + 1074 + 1172). Note:
bun run checkis red on the base for an unrelated pre-existing test-quality issue inprovider-registry.test.ts(not touched here).