fix(l1): resolve PR #193 backend review findings (1,4,5,6,7,8,9,10)
Server-assigns a uuid4 id to every AI-generated node (Finding 1 showstopper:
nodes had no id but the advance protocol keys on node_id, so ai_build walks
never advanced past question 1). Replaces the hidden {"node_type":"meta"}
walked_path convention with real category/problem_text/pending_node columns on
l1_walk_sessions (migration 61dda4f615c6) — fixes junk proposals + off-by-one
depth cap (Findings 8,9), and pending_node replays the served node on re-mount
(no duplicate paid LLM call). Intake honors explicit flow_id and adhoc=True
(Findings 4,5); flow_proposals.l1_session_id FK -> CASCADE (Finding 6 time
bomb); L1 category GET is owner+admin like PATCH and require_account_owner_or_admin
delegates to User.can_manage_account (Finding 7); escalate falls back to default
recipients + filters deleted_at + warns when empty (Finding 10). Cleanups: dead
ticket_ref removed, IntakeResponse per-outcome validator, unused acknowledged
dropped, escalations partial index, restored a deleted audit assertion.
Full Phase 2A backend set: 110 passed / 0 failed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -11,6 +11,7 @@ from app.models.user import User
|
||||
from app.models.tree import Tree
|
||||
from app.models.ai_session import AISession
|
||||
from app.models.flow_proposal import FlowProposal
|
||||
from app.models.l1_walk_session import L1WalkSession
|
||||
from app.services.l1_session_service import (
|
||||
start_flow_session,
|
||||
start_proposal_session,
|
||||
@@ -1073,3 +1074,138 @@ async def test_escalate_without_walk_writes_audit_log(test_db: AsyncSession):
|
||||
)
|
||||
row = result.scalar_one()
|
||||
assert row.account_id == account.id
|
||||
# Audit coverage: the reason category must be recorded (restored — a prior
|
||||
# edit dropped this assertion, weakening the audit guarantee).
|
||||
assert row.details["escalation_reason_category"] == "no_kb_content"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Finding 1 (server-assigned node ids) + Finding 8 (pending-node replay)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class _FakeProvider:
|
||||
def __init__(self, raw):
|
||||
self._raw = raw
|
||||
|
||||
async def generate_json(self, *, system_prompt, messages, max_tokens):
|
||||
return self._raw, None, None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ai_build_first_node_carries_id_and_advance_grows_walk(
|
||||
test_db: AsyncSession, monkeypatch,
|
||||
):
|
||||
"""Finding 1 contract: the SYSTEM_PROMPT never asks for an id, yet the first
|
||||
generated node must carry one — and advancing with that id must grow walked_path
|
||||
(the original showstopper: node_id was always None, so the walk never advanced)."""
|
||||
from app.services import l1_session_service as svc
|
||||
from app.services import ai_tree_builder
|
||||
account = await _make_account(test_db)
|
||||
l1_user = await _make_user(test_db, account_id=account.id)
|
||||
s = await svc.start_ai_build_session(
|
||||
test_db, account_id=account.id, user=l1_user,
|
||||
ticket_id="t-contract", ticket_kind="internal",
|
||||
category="printer", problem_text="printer offline")
|
||||
|
||||
# Real generator + a provider that omits id (the shape the model produces).
|
||||
monkeypatch.setattr(
|
||||
ai_tree_builder, "get_ai_provider",
|
||||
lambda *a, **k: _FakeProvider('{"node_type":"question","text":"Plugged in?"}'))
|
||||
|
||||
first = await svc.advance_ai_build(
|
||||
test_db, session_id=s.id, problem_text="printer offline",
|
||||
category="printer", node_id=None)
|
||||
assert first.get("id"), "first node must carry a server-assigned id"
|
||||
|
||||
# Answer it with the id we were handed; walked_path must grow by one.
|
||||
await svc.advance_ai_build(
|
||||
test_db, session_id=s.id, problem_text="printer offline", category="printer",
|
||||
node_id=first["id"], node_text=first["text"], answer="no")
|
||||
refreshed = await test_db.get(L1WalkSession, s.id)
|
||||
assert len(refreshed.walked_path) == 1
|
||||
assert refreshed.walked_path[0]["id"] == first["id"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_advance_ai_build_replays_pending_node_without_regenerating(
|
||||
test_db: AsyncSession, monkeypatch,
|
||||
):
|
||||
"""Finding 8: a re-mount (node_id=None) replays the served-but-unanswered node
|
||||
instead of firing a fresh paid LLM call (which could also swap the question)."""
|
||||
from app.services import l1_session_service as svc
|
||||
from app.services import ai_tree_builder
|
||||
account = await _make_account(test_db)
|
||||
l1_user = await _make_user(test_db, account_id=account.id)
|
||||
s = await svc.start_ai_build_session(
|
||||
test_db, account_id=account.id, user=l1_user,
|
||||
ticket_id="t-replay", ticket_kind="internal",
|
||||
category="printer", problem_text="printer offline")
|
||||
|
||||
calls = {"n": 0}
|
||||
|
||||
async def fake_next(problem, category, walked):
|
||||
calls["n"] += 1
|
||||
return {"node_type": "question", "id": f"q{calls['n']}", "text": "?"}
|
||||
monkeypatch.setattr(ai_tree_builder, "generate_next_node", fake_next)
|
||||
|
||||
first = await svc.advance_ai_build(
|
||||
test_db, session_id=s.id, problem_text="p", category="printer", node_id=None)
|
||||
# Re-mount without answering — must NOT regenerate.
|
||||
replay = await svc.advance_ai_build(
|
||||
test_db, session_id=s.id, problem_text="p", category="printer", node_id=None)
|
||||
assert calls["n"] == 1
|
||||
assert replay["id"] == first["id"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Finding 10: escalation recipient resolution
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_escalate_skips_soft_deleted_engineer(test_db: AsyncSession, monkeypatch):
|
||||
"""A soft-deleted engineer must not be paged (is_active alone misses them)."""
|
||||
from datetime import datetime, timezone
|
||||
from app.services import l1_session_service as svc
|
||||
calls = {}
|
||||
|
||||
async def fake_notify(event, account_id, payload, db, target_user_ids=None):
|
||||
calls["target_user_ids"] = target_user_ids
|
||||
monkeypatch.setattr(svc, "notify", fake_notify)
|
||||
|
||||
account = await _make_account(test_db)
|
||||
l1_user = await _make_user(test_db, account_id=account.id)
|
||||
live_eng = await _make_user(test_db, account_id=account.id, account_role="engineer")
|
||||
dead_eng = await _make_user(test_db, account_id=account.id, account_role="engineer")
|
||||
dead_eng.deleted_at = datetime.now(timezone.utc)
|
||||
await test_db.flush()
|
||||
ticket = await _make_internal_ticket(test_db, account_id=account.id, user_id=l1_user.id)
|
||||
s = await svc.start_ai_build_session(
|
||||
test_db, account_id=account.id, user=l1_user,
|
||||
ticket_id=str(ticket.id), ticket_kind="internal")
|
||||
await svc.escalate(test_db, session_id=s.id, reason="x", reason_category="exhausted_safe_steps")
|
||||
assert live_eng.id in calls["target_user_ids"]
|
||||
assert dead_eng.id not in calls["target_user_ids"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_escalate_with_no_engineers_falls_back_to_default_recipients(
|
||||
test_db: AsyncSession, monkeypatch,
|
||||
):
|
||||
"""Finding 10: when no eligible engineer exists, pass None (not []) so notify()
|
||||
falls back to the default owner/admin set instead of silently dropping it."""
|
||||
from app.services import l1_session_service as svc
|
||||
calls = {}
|
||||
|
||||
async def fake_notify(event, account_id, payload, db, target_user_ids=None):
|
||||
calls["target_user_ids"] = target_user_ids
|
||||
monkeypatch.setattr(svc, "notify", fake_notify)
|
||||
|
||||
account = await _make_account(test_db)
|
||||
# Only an l1_tech exists — not in the owner/admin/engineer recipient query.
|
||||
l1_user = await _make_user(test_db, account_id=account.id)
|
||||
ticket = await _make_internal_ticket(test_db, account_id=account.id, user_id=l1_user.id)
|
||||
s = await svc.start_ai_build_session(
|
||||
test_db, account_id=account.id, user=l1_user,
|
||||
ticket_id=str(ticket.id), ticket_kind="internal")
|
||||
await svc.escalate(test_db, session_id=s.id, reason="x", reason_category="exhausted_safe_steps")
|
||||
assert calls["target_user_ids"] is None
|
||||
|
||||
Reference in New Issue
Block a user