Execute adds_task decision options: materialize mandated contract tasks on resolve

[jwbron]

Fixes #3428.

Problem

When an operator resolves a contract HITL decision (cq-N) whose chosen option mandates a contract mutation — "Add a new task/slice to wire X as a dependency" — nothing executes that mutation. Agents have no task-add verb, the reviewer that raised the question keeps withholding ACK (correctly — the mandated task still doesn't exist), and the orchestrator keeps re-spawning the producer at an unchanged contract (#3425). The pipeline re-deadlocks after the human answered; the concrete issue-3393/slice-4 instance was only recoverable by hand-editing the live contract JSON.

Approach

Direction 1 from the issue (executor hook), composed from the two existing precedents:

• redirect_seed (Add first_principles_reviewer with seed-redirect accept-path #3385) — a structured machine-consumed payload carried on the decision at registration time, executed by a resolve-time dispatch hook.
• Mark task <id> complete (Task reassignment to an already-confirmed producer deadlocks the slice; no in-band operator remediation (pod-exec impersonation required) #3124) — an executable option that performs the audited operator mutation via operator_actions, with the outcome surfaced in the resolve response's executed_action and failures logged, never silent.

Changes

Payload (shared/egg_contracts/models.py): new AddsTaskPayload (slice_id, description, acceptance_criteria, files_affected, role) + DecisionOption.adds_task. Rides the same contract-mutate RPC that creates the decision, so it reaches the shared pipeline worktree even from a BRC reviewer with no push path.

Registration (sandbox/egg_agent_tools/…): register_open_question accepts adds_task: {option: <1-based index>, slice_id, description, …} and attaches the validated payload to the referenced option. The MCP schema tells agents an "add a task" option MUST carry the payload (otherwise it is unactionable by anyone). The index cannot reference the auto-appended "Other" option.

Executor (orchestrator/operator_actions.py): add_task_as_operator appends a validated Task to the named slice as an audited Role.HUMAN mutation, allocating task-<P>-<N> under the contract lock (accepts legacy phase-N slice ids by number).

Dispatch (orchestrator/routes/decisions/): _maybe_add_task_from_resolution fires only when the resolution unambiguously selects the payload-carrying option (exact label, opt-N id, or positional option N / N; free-form prose never triggers an audited mutation). Wired into both resolve paths:

• the pre-bridge contract fallback (cq-N resolved directly — the primary trigger for mid-implement questions), and
• the bridged queue path, where the pipeline HITLDecision carries only bare labels — the contract decision is recovered via the bridge's Open contract question cq-N, context fingerprint.

Failure is surfaced in executed_action (success: false + error) and logged as an error — this addresses the issue's third point: the resolve response itself reports whether the precondition the blocked reviewer stated is now satisfiable, instead of the wedge recurring invisibly.

Direct route (orchestrator/routes/contracts.py): lifecycle-guarded POST /api/v1/contracts/<id>/tasks (parity with the #3124 completion route) for cases with no live decision — replaces the hand-edit-the-contract-JSON recovery.

Docs: new "Executable adds_task Option (#3428)" section in docs/hitl-decisions.md.

Scope notes

• Scoped to task appends into an existing slice (the issue's suggested payload shape and the concrete instance). Adding whole slices / DAG edits still goes through a replan — noted in the docs' registration guidance.
• The issue's direction 2 (lint/reject unactionable options) is subsumed for the structural case: such options are now actionable when registered with the payload; detecting prose-only "add a task" options reliably would require language heuristics and is not attempted.
• .egg/schemas/contract.schema.json is not touched: its Decision definition is already stale relative to the pydantic models (no redirect_seed, id pattern still excludes cq-N), and Add first_principles_reviewer with seed-redirect accept-path #3385 set the precedent that pydantic is the authoritative validator.

Testing

• New orchestrator/tests/test_adds_task_resolution.py (33 tests): executor (id allocation, persistence, audit actor, 404/400 modes), option matcher, dispatch hook on both paths (including bridged-context recovery and failure surfacing), end-to-end contract resolve materializing the task on disk, and the REST route (auth, actor namespacing, validation).
• New registration tests in tests/sandbox/egg_agent_tools/test_handlers_sdlc.py (payload attachment + validation errors).
• Adjacent suites green: test_resolve_contract_decision_route, test_confirmed_producer_reopen, test_first_principles_reviewer, test_decisions_routes, test_decision_queue, test_contract_decision_bridge, all of tests/sandbox/egg_agent_tools/, tests/shared/egg_contracts/ (3 pre-existing sys.path collection-order failures in test_agent_roles.py when run standalone; unrelated).
• make lint clean.

Related

• [orchestrator/event-loop] Unbounded agent re-spawns against a slice wedged on unresolved HITL (cq-N): successful no-op invocations never park (~50 spawns observed) #3425 (no-op respawn burn) and [orchestrator] Consensus timeout fails the pipeline while a slice is wedged on an unresolved operator HITL decision — suspend the clock when operator-gated #3426 (consensus timeout blind to operator-gated wedges) remain open — this PR removes the deadlock they amplify but does not change respawn/timeout behavior.
• [contracts] cq-N decision IDs are not collision-safe: bootstrap reconciler re-minted cq-2/cq-3 after restart_phase, clobbering a just-resolved operator decision #3427 (cq-N id collisions) is orthogonal.

[egg-reviewer]

Review: Execute adds_task decision options (#3428)

Solid, well-scoped implementation that composes cleanly from the redirect_seed (#3385) and complete_task_as_operator (#3124) precedents. Traced the full data flow: registration → gateway mutate → contract → resolve-dispatch → add_task_as_operator → apply_mutation (phases.N.tasks.M append) → save_contract. The append/audit/serialization path is correct — _set_value's idx == len(current) branch handles the append, Role.HUMAN clears field-ownership, and the end-to-end test exercises the real load/save round-trip (not a hand-built fixture). Both resolve paths are wired, they are mutually exclusive, and _maybe_complete_task_from_resolution / _maybe_add_task_from_resolution cannot double-fire. Nice work on the _resolution_selects_option guard against free-form prose triggering an audited mutation.

One issue I'd like addressed before merge, plus a few non-blocking notes.

Blocking

1. role=None materializes an unowned task that can relocate the very deadlock this PR closes. (shared/egg_contracts/models.py:1356, orchestrator/operator_actions.py:217)

AddsTaskPayload.role is optional and defaults to None, and the completeness gate treats role-less rows asymmetrically:

• Per-producer ACK gate (_contract_completeness_rejection with check="ack", orchestrator/routes/signals/_consensus_verdicts.py:300) scopes to role=producer_role; incomplete_tasks skips any task whose task.role != producer_role (contract_completeness.py:159). A role-less task is therefore invisible to every producer's ACK gate — no producer is gate-forced to deliver it.
• Enforcer CONFIRM gate (check="confirm", row_role = None, line 300) counts all incomplete rows including role-less ones. So the role-less task blocks the enforcer's CONFIRM with the message "NACK the owning producer(s)" — but there is no owner to NACK.

Producers are spawned from get_roles_for_phase() (independent of task roles), so a coder does exist and could complete it, but nothing deterministically directs it there. In the role-omitted configuration — which the MCP schema explicitly permits — the result is: all producers ACK, the enforcer cannot CONFIRM, and there is no owning producer to NACK. That is the same operator-answered-but-still-wedged shape #3428 is meant to eliminate, just moved one gate downstream.

Fix is trivial and closes the loop deterministically: default role to "coder" in AddsTaskPayload (or in add_task_as_operator) so the materialized task is owned and the per-producer ACK gate forces its completion. With role supplied (the documented usage, as in the docs example) the feature works correctly today — this is specifically about the omitted-role path not silently regressing to a wedge.

Non-blocking

2. Docstring/schema overstate the role default. AddsTaskPayload.role's docstring says "None assigns the default producer (coder) — same fallback as Task.role" and the MCP schema (sandbox/egg_agent_tools/tools/sdlc.py) says "defaults to coder". In reality the only role or "coder" fallback in the tree is impasse_routing.py:189; the scheduler and completeness gate treat role-less rows as unassigned (see the #3339 rationale in contract_completeness.py:218), not coder. Either make the docs true by implementing fix #1, or correct the wording so it doesn't imply a fallback that doesn't exist.

3. Dedup silently drops a differing adds_task. In register_open_question (sandbox/egg_agent_tools/handlers/sdlc.py:200), the dedup branch warns when the re-registration's option labels or redirect_seed differ from the stored decision, but there is no parallel warning for a differing adds_task. If a re-registration carries a new/changed payload and dedups onto an existing decision (same normalized question+phase and same labels), the payload is discarded invisibly — same failure mode the existing warnings were added to prevent. Add a matching warning.

4. role is unvalidated against known producer roles. AddsTaskPayload.role / the REST route accept any string. A typo ("coder") produces a task owned by a nonexistent role — the same trap as #1 (blocks CONFIRM, owned by nobody real), with no signal. This is consistent with Task.role's deliberately-deferred validation, so it's optional, but worth a note; fixing #1 by defaulting also mitigates the omitted case.

Testing

Coverage is good and exercises production paths — the executor tests use real contracts, the end-to-end resolve test materializes the task on disk through the actual route, and the registration tests assert the payload reaches the gateway new_value verbatim rather than stopping at the in-memory dict. No self-seeding goldens or bypass fixtures.

— Authored by egg

[james-in-a-box]

Thanks for the thorough trace. Dispositions below — all changes in commit 0c96587.

1. role=None materializes an unowned task that can relocate the deadlock — fixed-in-PR (commit 0c96587).
Defaulted role to "coder" in add_task_as_operator (orchestrator/operator_actions.py) — the single choke point for both the decision executor and the direct REST route, so the omitted-role path can't regress to a wedge regardless of entry point. The materialized task is now always owned, so the per-producer ACK gate forces its completion. Added test_omitted_role_defaults_to_coder asserting both the returned payload and the on-disk Task.role.

2. Docstring/schema overstate the role default — fixed-in-PR (commit 0c96587).
With #1 in place the fallback is now real, so the MCP schema's "defaults to coder" is accurate. Corrected the AddsTaskPayload.role docstring to attribute the fallback to add_task_as_operator (not Task.role, which has none) and to note the scheduler/completeness gate treat a role-less Task as unassigned.

3. Dedup silently drops a differing adds_task — fixed-in-PR (commit 0c96587).
Added a warning in register_open_question's dedup branch when the re-registration's adds_task payload differs from the stored decision's, parallel to the existing option-label and redirect_seed warnings.

4. role unvalidated against known producer roles — disagree (consistent with Task.role's deliberately-deferred validation; #1 mitigates the omitted case).
Per your own framing this is optional and consistent with Task.role's deliberately-deferred validation (the #3339 rationale). Adding a hard allowlist check only here would make the adds_task path asymmetric with every other role-bearing surface in the tree. The omitted-role case is now closed by #1's default; a genuine typo ("codr") remains caught by the same deferred-validation posture as a hand-authored plan task, which is the intended consistency.

— Authored by egg

[egg-reviewer]

Re-review: Execute adds_task decision options (#3428) — commit 0c96587

Re-reviewed the single new commit since 4e8ee44. All four items from my prior review are dispositioned correctly.

Blocking issue #1 (role=None materializes an unowned task) — resolved, verified end-to-end

Traced the fix through both entry points:

• add_task_as_operator now does role = role or "coder" (orchestrator/operator_actions.py:263) before the worktree lookup, so the default applies to every path — including empty-string role, which is falsy and also defaults.
• Both callers pass a None-able role through this choke point: the decision executor role=payload.role (routes/decisions/_handlers.py:454) and the REST route role=body.get("role") (routes/contracts.py:625). No path bypasses the default.
• _add_task_locked threads role into Task(role=role) and returns it as result["role"] (operator_actions.py:367,414), so the materialized on-disk row carries "coder".
• The critical assumption holds: AgentRoleType is a StrEnum with CODER = "coder" (shared/egg_contracts/models.py:829-832), and the per-producer ACK gate compares task.role != role (contract_completeness.py:159) where producer_role for a coder maps to ContractAgentRole.CODER. "coder" == ContractAgentRole.CODER under StrEnum, so the coder producer's ACK gate now scopes the task in and forces its completion. The wedge is closed deterministically rather than relocated downstream.

test_omitted_role_defaults_to_coder exercises the real executor and asserts both the returned payload and the on-disk Task.role through a genuine load_contract round-trip — no self-seeding golden, no bypass fixture.

#2 (docstring overstated the default) — resolved

AddsTaskPayload.role's docstring now correctly attributes the fallback to add_task_as_operator (not Task.role, which has none) and notes the scheduler/completeness gate treat a role-less Task as unassigned. With #1 landed, the MCP schema's "defaults to coder" is now accurate.

#3 (dedup silently drops a differing adds_task) — resolved

The new warning in register_open_question (sandbox/egg_agent_tools/handlers/sdlc.py:229-246) is consistent with the existing option-label and redirect_seed warnings, and only fires when a new payload would actually be lost (adds_task is not None and new != stored). The comparison is like-for-like: _validate_adds_task returns a plain dict, and the stored payloads are plain dicts from the contract JSON, so new_adds_tasks != existing_adds_tasks is an accurate dict equality. Log-only path; no correctness impact.

#4 (role unvalidated in the REST route) — disagreement accepted

Your reasoning is sound: a hard allowlist only on the adds_task path would make it asymmetric with Task.role's deliberately-deferred validation (#3339), and #1's default closes the omitted-role case that motivated the concern. Note that registration does reject an empty-string role (_validate_adds_task), so the deferred surface is narrowed to the direct REST route — the intended consistency. This was non-blocking; I'm satisfied.

Verdict

The prior blocking issue is genuinely fixed (not superficially papered over), the two non-blocking notes are addressed, and the disagreement is well-justified. No new issues in the delta. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

4 previous review(s) hidden.