fix(company): make delegation review lifecycle durable
This commit is contained in:
+1254
-909
File diff suppressed because it is too large
Load Diff
@@ -131,6 +131,10 @@ _WORK_ITEM_FIELDS: tuple[MetadataFieldSpec, ...] = (
|
||||
_spec("review_attempt", MetadataOwner.WORK_ITEM, allowed_locations=("work_item", "task_execution_copy"), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
_spec("review_attempt_count", MetadataOwner.WORK_ITEM, allowed_locations=("work_item",), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
_spec("review_target_work_item_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item", "task_execution_copy"), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
_spec("review_source_report_work_item_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item",), migration_policy="work_item_wins"),
|
||||
_spec("review_resolution", MetadataOwner.WORK_ITEM, allowed_locations=("work_item",), migration_policy="work_item_wins"),
|
||||
_spec("review_resolution_state", MetadataOwner.WORK_ITEM, allowed_locations=("work_item",), migration_policy="work_item_wins"),
|
||||
_spec("review_resolution_applied_work_item_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item",), migration_policy="work_item_wins"),
|
||||
_spec("review_target_worker_task_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item", "task_execution_copy"), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
_spec("review_target_worker_role_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item", "task_execution_copy"), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
_spec("review_target_worker_seat_id", MetadataOwner.WORK_ITEM, allowed_locations=("work_item", "task_execution_copy"), legacy_fallback=True, migration_policy="work_item_wins"),
|
||||
|
||||
@@ -49,7 +49,9 @@ __all__ = [
|
||||
"phase_for_task_status",
|
||||
"coerce_phase",
|
||||
"is_review_execution_work_item_metadata",
|
||||
"is_runtime_auxiliary_work_item",
|
||||
"should_hide_work_item_from_company_kanban",
|
||||
"is_stale_claim_releasable",
|
||||
"is_resumable_after_claim_release",
|
||||
"is_orphaned",
|
||||
"is_dispatchable",
|
||||
@@ -146,6 +148,27 @@ def is_report_execution_work_item_metadata(metadata: Mapping[str, Any] | None) -
|
||||
return work_kind == "report" and bool(str(data.get("report_target_work_item_id", "") or "").strip())
|
||||
|
||||
|
||||
def is_runtime_auxiliary_work_item(value_or_metadata: Any) -> bool:
|
||||
"""True for attention, report, and review runtime helper cards.
|
||||
|
||||
Accepts either a work-item-like object, a serialized work item containing
|
||||
a ``metadata`` mapping, or the metadata mapping itself. Keeping this
|
||||
identity check below the company runtime gives lifecycle and board code a
|
||||
single dependency-free definition of a non-business child.
|
||||
"""
|
||||
if isinstance(value_or_metadata, Mapping):
|
||||
nested = value_or_metadata.get("metadata")
|
||||
metadata = nested if isinstance(nested, Mapping) else value_or_metadata
|
||||
else:
|
||||
metadata = getattr(value_or_metadata, "metadata", None)
|
||||
data = dict(metadata or {}) if isinstance(metadata, Mapping) else {}
|
||||
return (
|
||||
bool(data.get("attention_work_item", False))
|
||||
or is_report_execution_work_item_metadata(data)
|
||||
or is_review_execution_work_item_metadata(data)
|
||||
)
|
||||
|
||||
|
||||
def should_hide_work_item_from_company_kanban(metadata: Mapping[str, Any] | None) -> bool:
|
||||
"""True when the kanban UI should not display this work item."""
|
||||
data = dict(metadata or {})
|
||||
@@ -316,40 +339,45 @@ def is_runnable(phase: Phase) -> bool:
|
||||
return phase in RUNNABLE_PHASES
|
||||
|
||||
|
||||
# Phases whose runtime claim, when released as stale (process restart, crashed
|
||||
# session), allow the dispatcher to re-pick the card. Without this set, a card
|
||||
# that was actively running when the process died becomes a zombie: phase still
|
||||
# says RUNNING / WAITING_FOR_*, but no session is alive to make progress.
|
||||
_RESUMABLE_AFTER_STALE_CLAIM: frozenset[Phase] = frozenset({
|
||||
Phase.RUNNING,
|
||||
Phase.WAITING_FOR_PEER,
|
||||
Phase.WAITING_FOR_CHILDREN,
|
||||
Phase.PAUSED,
|
||||
Phase.NEEDS_ATTENTION,
|
||||
Phase.AWAITING_MANAGER_REVIEW,
|
||||
Phase.AWAITING_HUMAN,
|
||||
})
|
||||
# A process restart invalidates every persisted runtime claim, including claims
|
||||
# on passive review states. Releasing a dead claim and allowing the original
|
||||
# worker to execute again are intentionally separate decisions: review parents
|
||||
# must lose their dead claim but remain passive while their report/review
|
||||
# auxiliaries resume.
|
||||
_STALE_CLAIM_RELEASABLE_PHASES: frozenset[Phase] = (
|
||||
IN_PROGRESS_PHASES | IN_REVIEW_PHASES
|
||||
)
|
||||
_RESUMABLE_AFTER_CLAIM_RELEASE_PHASES: frozenset[Phase] = IN_PROGRESS_PHASES
|
||||
|
||||
|
||||
def is_stale_claim_releasable(phase: Phase) -> bool:
|
||||
"""Whether startup recovery may clear a dead runtime claim.
|
||||
|
||||
This includes passive review/human-wait states so a crashed resident
|
||||
session cannot retain ownership forever. It does *not* imply that the
|
||||
original work item may be dispatched again.
|
||||
"""
|
||||
return phase in _STALE_CLAIM_RELEASABLE_PHASES
|
||||
|
||||
|
||||
def is_resumable_after_claim_release(phase: Phase) -> bool:
|
||||
"""True iff a stale claim on this phase can be released and the card
|
||||
re-picked up by the dispatcher (rather than left as a zombie).
|
||||
"""Whether the original worker may resume after its claim is released.
|
||||
|
||||
Used by the periodic stale-claim sweeper. Every in-flight phase must
|
||||
return True here — the invariant test in
|
||||
test_phase_state_machine_invariants.py enforces this.
|
||||
Active execution phases can be re-picked after a crash. Passive
|
||||
``AWAITING_*`` parents cannot: their report/review chain (or human) owns
|
||||
forward progress.
|
||||
"""
|
||||
return phase in _RESUMABLE_AFTER_STALE_CLAIM
|
||||
return phase in _RESUMABLE_AFTER_CLAIM_RELEASE_PHASES
|
||||
|
||||
|
||||
def is_orphaned(item: Any) -> bool:
|
||||
"""A work item is orphaned when its phase says 'in flight' but no
|
||||
runtime session currently holds a claim on it.
|
||||
|
||||
Typical scenario: the process that owned the claim died (restart,
|
||||
crash). On startup the stale-claim sweeper clears the claim
|
||||
metadata; this function then lets the dispatcher re-pick the card
|
||||
on the next tick, eliminating zombie work items (Bug C).
|
||||
Typical scenario: the process that owned an execution claim died. On
|
||||
startup the stale-claim sweeper clears the claim metadata; this function
|
||||
then lets the dispatcher re-pick active execution, but never a passive
|
||||
review parent.
|
||||
"""
|
||||
if not is_resumable_after_claim_release(item.phase):
|
||||
return False
|
||||
|
||||
@@ -31,6 +31,7 @@ from opc.core.models import Phase, TaskStatus
|
||||
from opc.layer2_organization.phase import (
|
||||
DONE_PHASES,
|
||||
RUNNABLE_PHASES,
|
||||
is_stale_claim_releasable,
|
||||
register_phase_transition_hook,
|
||||
task_status_for_phase,
|
||||
)
|
||||
@@ -365,6 +366,17 @@ def _active_focus_id(session: Any, work_item_by_id: dict[str, Any]) -> str:
|
||||
return ""
|
||||
if getattr(focused_item, "phase", None) in DONE_PHASES:
|
||||
return ""
|
||||
phase = getattr(focused_item, "phase", None)
|
||||
if is_stale_claim_releasable(phase):
|
||||
claim = str(
|
||||
getattr(focused_item, "claimed_by_role_runtime_session_id", "") or ""
|
||||
).strip()
|
||||
if not claim:
|
||||
# On a live transition the focused item retains its claim. After
|
||||
# restart the startup sweep removes that dead-process claim; the
|
||||
# persisted session focus must stop blocking the orphaned item (or
|
||||
# a passive parent's report/review helper) from being dispatched.
|
||||
return ""
|
||||
return focused
|
||||
|
||||
|
||||
|
||||
@@ -30,6 +30,29 @@ from typing import Any, Mapping
|
||||
from opc.core.models import Phase
|
||||
|
||||
|
||||
MANAGER_DISPATCH_TURN_METADATA_KEYS: tuple[str, ...] = (
|
||||
"manager_board_mutation_performed",
|
||||
"manager_board_modified_work_item_ids",
|
||||
"manager_board_deleted_work_item_ids",
|
||||
"manager_no_delegation_justification",
|
||||
"no_delegation_justification",
|
||||
"manager_dispatch_guard_unresolved",
|
||||
)
|
||||
|
||||
|
||||
def reset_manager_dispatch_turn_metadata(metadata: Mapping[str, Any] | None) -> dict[str, Any]:
|
||||
"""Return mutable metadata with prior manager-turn outcomes removed.
|
||||
|
||||
These keys describe what happened in one agent turn. They must not be
|
||||
carried into a retry, rework turn, or user follow-up; durable board state
|
||||
(dependencies and child mutation revisions) is intentionally untouched.
|
||||
"""
|
||||
result = dict(metadata or {})
|
||||
for key in MANAGER_DISPATCH_TURN_METADATA_KEYS:
|
||||
result.pop(key, None)
|
||||
return result
|
||||
|
||||
|
||||
class TurnMode(str, Enum):
|
||||
EXECUTE = "execute"
|
||||
DELEGATE = "delegate"
|
||||
|
||||
@@ -155,25 +155,13 @@ def is_delivery_turn(value_or_metadata: Any) -> bool:
|
||||
|
||||
|
||||
def is_manager_reviewable_turn(value_or_metadata: Any) -> bool:
|
||||
"""Return True when a finished WorkItem should enter manager review flow.
|
||||
"""Return the turn type's default manager-review policy.
|
||||
|
||||
An explicit persisted ``turn_output_kind`` wins over the turn-type
|
||||
default: delegation-kind turns (dispatch/intake/plan) are review-exempt
|
||||
only because their deliverable is normally a child card set that gets
|
||||
reviewed per child. When such a turn completed with the manager's own
|
||||
work product instead, the done-transition stamps
|
||||
``turn_output_kind=self_produced`` on the WorkItem and that output is
|
||||
reviewable like any execute turn — every consumer of this predicate
|
||||
(DONE routing, report spawn, report completion, recovery scans) follows
|
||||
automatically.
|
||||
Dispatch/intake/plan are normally board-producing turns and therefore
|
||||
exempt. The executor handles the one dynamic exception — a current turn
|
||||
that produced no board mutation — before it writes the authoritative
|
||||
WorkItem phase. Downstream review plumbing follows that phase directly.
|
||||
"""
|
||||
metadata: Mapping[str, Any] | None = None
|
||||
if isinstance(value_or_metadata, Mapping):
|
||||
metadata = value_or_metadata
|
||||
elif hasattr(value_or_metadata, "metadata"):
|
||||
metadata = getattr(value_or_metadata, "metadata", None) or {}
|
||||
if metadata and str(metadata.get("turn_output_kind", "") or "").strip().lower() == "self_produced":
|
||||
return True
|
||||
turn_type = _turn_type_for_value(value_or_metadata, fallback="")
|
||||
if not turn_type:
|
||||
return False
|
||||
|
||||
Reference in New Issue
Block a user