mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix: clear 55 pre-existing test failures uncovered after Wave A landed
Three classes of failure, all surfaced once Wave A's plan-required gate and the migration 013 went in. Per project standing rule: pre-existing errors are not a free pass — fix them. 1. Wave A1 ordering (32 lifecycle parity failures + 1 full-pipe test). _pm_sub_tasks_gate fired BEFORE _claim_plan_start_gate, so wrong-state PMs got `incomplete_input` (the gate's verdict) when the spec's lifecycle gate should have returned `invalid_state` first. Swapped: re-entry check → spec lifecycle gate → sub_tasks gate → claim_plan_run. Parity test now sees the spec's verdict as expected. 2. E2 enum naming (2 migration_013 failures + ripple). _str_enum in roboco/db/tables.py didn't pass name=… to SQLAlchemy Enum(...), so Base.metadata.create_all in test setup inferred `role` from the Python class `Role` while the alembic migrations declare `agentrole`. Tests saw two enums for the same class and hit `agentrole = role` operator errors. Fixed: default name to lower(class_name) (matches every migration), override `Role` → `agentrole`. One dict entry; no class-by-class registration needed. 3. _MockContentActions.note() signature drift. Wave 2 G4 added `structured` kwarg to ContentActions.note(). The integration mock at tests/integration/v2/test_full_pending_to_completed.py didn't accept the new kwarg → 1 test failed on the very first call from the v2 do/note route. Added `structured: object = None` and left it unused (the test asserts lifecycle, not journal rendering). Plus three ruff E501 line-length fixes in the test files I touched. Quality: ruff + mypy clean. pytest 6690 passed / 0 failed / 274 skipped.
This commit is contained in:
+24
-1
@@ -50,6 +50,18 @@ from roboco.models.base import (
|
|||||||
from roboco.models.session import SessionScope
|
from roboco.models.session import SessionScope
|
||||||
from roboco.models.work_session import WorkSessionStatus
|
from roboco.models.work_session import WorkSessionStatus
|
||||||
|
|
||||||
|
# Python class name → canonical postgres enum name (only the cases where
|
||||||
|
# the lowercased class name does NOT match the migration's `name=...`).
|
||||||
|
# Audited from alembic/versions/*.py — every other StrEnum uses
|
||||||
|
# lower(class_name), so we default to that.
|
||||||
|
_PG_ENUM_NAME_OVERRIDES: dict[str, str] = {
|
||||||
|
# The foundation's `Role` class binds to the postgres `agentrole` enum
|
||||||
|
# (see alembic 001 + 012). Without this override, SQLAlchemy infers
|
||||||
|
# `role` from the class name, producing the `operator does not exist:
|
||||||
|
# agentrole = role` regression that smoke run 2 hit.
|
||||||
|
"Role": "agentrole",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
def _str_enum(enum_cls: type) -> Enum:
|
def _str_enum(enum_cls: type) -> Enum:
|
||||||
"""SQLAlchemy Enum that serializes by `.value` (lowercase) for StrEnum types.
|
"""SQLAlchemy Enum that serializes by `.value` (lowercase) for StrEnum types.
|
||||||
@@ -57,8 +69,19 @@ def _str_enum(enum_cls: type) -> Enum:
|
|||||||
Matches the lowercase values declared in alembic/versions/001_initial_schema.py.
|
Matches the lowercase values declared in alembic/versions/001_initial_schema.py.
|
||||||
Without values_callable, SQLAlchemy uses `.name` (uppercase) which does not
|
Without values_callable, SQLAlchemy uses `.name` (uppercase) which does not
|
||||||
match the alembic-declared enum members.
|
match the alembic-declared enum members.
|
||||||
|
|
||||||
|
The `name=` is pinned to the canonical postgres enum name so that
|
||||||
|
``Base.metadata.create_all`` (test setup) produces the same enum
|
||||||
|
types the migrations create. Default is ``lower(class_name)`` —
|
||||||
|
matches every alembic migration's name=...; the only override is
|
||||||
|
``Role`` → ``agentrole`` (E2 fix).
|
||||||
"""
|
"""
|
||||||
return Enum(enum_cls, values_callable=lambda obj: [m.value for m in obj])
|
name = _PG_ENUM_NAME_OVERRIDES.get(enum_cls.__name__, enum_cls.__name__.lower())
|
||||||
|
return Enum(
|
||||||
|
enum_cls,
|
||||||
|
name=name,
|
||||||
|
values_callable=lambda obj: [m.value for m in obj],
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
|
|||||||
@@ -2045,14 +2045,22 @@ class Choreographer:
|
|||||||
verb_name="i_will_plan",
|
verb_name="i_will_plan",
|
||||||
)
|
)
|
||||||
# Re-entry check runs first — a respawned PM with thin args ("resume",
|
# Re-entry check runs first — a respawned PM with thin args ("resume",
|
||||||
# no sub_tasks) must short-circuit here before the sub_tasks gate.
|
# no sub_tasks) must short-circuit here before any gate.
|
||||||
if reentry := await self._handle_pm_reentry(
|
if reentry := await self._handle_pm_reentry(
|
||||||
ctx, t, pm_agent_id, task_id, role_str, briefing
|
ctx, t, pm_agent_id, task_id, role_str, briefing
|
||||||
):
|
):
|
||||||
return reentry
|
return reentry
|
||||||
# Gate runs only for initial-claim paths (not re-entry).
|
# Lifecycle spec gate runs BEFORE the sub_tasks gate so wrong-state
|
||||||
# PMs decompose; their plan MUST include approach + at least one sub_task.
|
# cases (e.g., task in backlog/claimed/completed) return invalid_state
|
||||||
# Devs execute; sub_tasks list can be empty (their plan is execution-shaped).
|
# — the lifecycle's verdict — instead of being masked by the
|
||||||
|
# PM-decomposition check. Parity test
|
||||||
|
# `test_lifecycle_consumer_parity.py::test_i_will_plan_matches_spec`
|
||||||
|
# asserts this order.
|
||||||
|
if rejection := await self._claim_plan_start_gate(ctx, role, spec_ctx):
|
||||||
|
return rejection
|
||||||
|
# Spec gate passed; now enforce the verb-specific PM-decomposition
|
||||||
|
# contract. PMs decompose; their plan MUST include approach + at
|
||||||
|
# least one sub_task. Devs execute; sub_tasks may be empty.
|
||||||
if rejection := await self._pm_sub_tasks_gate(
|
if rejection := await self._pm_sub_tasks_gate(
|
||||||
role_str=role_str,
|
role_str=role_str,
|
||||||
rich_plan=rich_plan,
|
rich_plan=rich_plan,
|
||||||
@@ -2062,8 +2070,6 @@ class Choreographer:
|
|||||||
briefing=briefing,
|
briefing=briefing,
|
||||||
):
|
):
|
||||||
return rejection
|
return rejection
|
||||||
if rejection := await self._claim_plan_start_gate(ctx, role, spec_ctx):
|
|
||||||
return rejection
|
|
||||||
envelope = await self._claim_plan_start_run(ctx, agent, spec_ctx)
|
envelope = await self._claim_plan_start_run(ctx, agent, spec_ctx)
|
||||||
return await self._post_claim_journal_gate(
|
return await self._post_claim_journal_gate(
|
||||||
"i_will_plan", pm_agent_id, task_id, envelope
|
"i_will_plan", pm_agent_id, task_id, envelope
|
||||||
|
|||||||
@@ -173,11 +173,16 @@ class _MockContentActions:
|
|||||||
text: object,
|
text: object,
|
||||||
scope: str = "note",
|
scope: str = "note",
|
||||||
task_id: object = None,
|
task_id: object = None,
|
||||||
|
structured: object = None,
|
||||||
) -> Envelope:
|
) -> Envelope:
|
||||||
|
# `structured` mirrors the Wave 2 G4 production signature (panel
|
||||||
|
# decision/reflect fields). The mock ignores it — the test asserts
|
||||||
|
# lifecycle transitions, not journal-entry rendering.
|
||||||
_ = agent_id
|
_ = agent_id
|
||||||
_ = text
|
_ = text
|
||||||
_ = scope
|
_ = scope
|
||||||
_ = task_id
|
_ = task_id
|
||||||
|
_ = structured
|
||||||
return Envelope.ok(status="noted", task_id=None, next="continue")
|
return Envelope.ok(status="noted", task_id=None, next="continue")
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -218,7 +218,9 @@ async def test_i_will_plan_rejects_non_pending_state() -> None:
|
|||||||
pm_id = uuid4()
|
pm_id = uuid4()
|
||||||
task_id = uuid4()
|
task_id = uuid4()
|
||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.get.return_value = MagicMock(id=task_id, status="in_progress", assigned_to=uuid4())
|
task_svc.get.return_value = MagicMock(
|
||||||
|
id=task_id, status="in_progress", assigned_to=uuid4()
|
||||||
|
)
|
||||||
task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend")
|
task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend")
|
||||||
deps = _make_deps(task=task_svc)
|
deps = _make_deps(task=task_svc)
|
||||||
c = Choreographer(deps)
|
c = Choreographer(deps)
|
||||||
@@ -392,8 +394,13 @@ async def test_i_will_plan_idempotent_when_already_in_progress_for_caller() -> N
|
|||||||
task_id,
|
task_id,
|
||||||
plan="re-entry plan",
|
plan="re-entry plan",
|
||||||
rich_plan={
|
rich_plan={
|
||||||
"approach": "Idempotent re-entry: task already in progress, refresh heartbeat.",
|
"approach": (
|
||||||
"sub_tasks": [{"title": "Re-entry subtask", "description": "Resume work"}],
|
"Idempotent re-entry: task already in progress, "
|
||||||
|
"refresh heartbeat."
|
||||||
|
),
|
||||||
|
"sub_tasks": [
|
||||||
|
{"title": "Re-entry subtask", "description": "Resume work"}
|
||||||
|
],
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -451,8 +458,13 @@ async def test_i_will_plan_recovery_when_already_claimed_for_caller() -> None:
|
|||||||
task_id,
|
task_id,
|
||||||
plan="re-entry plan",
|
plan="re-entry plan",
|
||||||
rich_plan={
|
rich_plan={
|
||||||
"approach": "Recovery re-entry: task claimed but not started; run set_plan + start.",
|
"approach": (
|
||||||
"sub_tasks": [{"title": "Recovery subtask", "description": "Resume from claimed"}],
|
"Recovery re-entry: task claimed but not started; "
|
||||||
|
"run set_plan + start."
|
||||||
|
),
|
||||||
|
"sub_tasks": [
|
||||||
|
{"title": "Recovery subtask", "description": "Resume from claimed"}
|
||||||
|
],
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -286,7 +286,8 @@ async def test_note_reflect_missing_required_fields_returns_incomplete_input() -
|
|||||||
body = env.as_dict()
|
body = env.as_dict()
|
||||||
|
|
||||||
assert body["error"] == "incomplete_input"
|
assert body["error"] == "incomplete_input"
|
||||||
assert {"what_done", "what_learned", "what_struggled"}.issubset(set(body["missing"]))
|
missing = set(body["missing"])
|
||||||
|
assert {"what_done", "what_learned", "what_struggled"}.issubset(missing)
|
||||||
journal_svc.write_entry.assert_not_awaited()
|
journal_svc.write_entry.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user