resolutionflow/docs/plans/2026-06-09-pr193-phase2a-review-findings.md

PR #193 (Phase 2A — L1 AI Tree Builder) Review Findings

Date: 2026-06-09 Reviewed: feat/l1-ai-tree-builder-phase-2a vs main (42 files, +2,326/−154) Process: 7 independent finder angles, every candidate independently verified against actual code (quoted lines confirmed, not speculation). Verdict: DO NOT MERGE as-is. The headline feature (AI-guided walkthrough) is non-functional end-to-end, two tasks recorded as complete in .ai/HANDOFF.md were never actually committed, and one DB constraint is a deletion time bomb.

---

✅ RESOLUTION (2026-06-09, same day)

All 10 findings resolved. Two architectural decisions taken (see .ai/DECISIONS.md): the root fix for Findings 8/9 (real category / problem_text / pending_node columns on l1_walk_sessions; the {"node_type":"meta"} walked_path convention deleted entirely — migration 61dda4f615c6), and restoring the ad-hoc walk (Finding 5 option a — adhoc=True intake + "Walk it ad-hoc" out_of_scope button).

• Finding 1 — ai_tree_builder._assign_id stamps uuid4().hex[:8] on every node (generated, depth-cap, generation-failed); current_node_id now real. Contract test added (test_ai_build_first_node_carries_id_and_advance_grows_walk).
• Finding 2a/3 — L1EscalationsSection mounted on EscalationQueuePage; ProposalDetail /pilot link gated on source_session_id, L1-source block added.
• Finding 2b — renders step.question ?? step.text, timeAgo, shows problem_text.
• Finding 4 — intake honors explicit flow_id (matcher bypassed); suggest card passes near_miss.flow_id; the three intake handlers collapsed into one runIntake.
• Finding 5 — ad-hoc walk restored (option a).
• Finding 6 — l1_session_id FK → ondelete=CASCADE (model + migration); cascade-delete test.
• Finding 7 — owner+admin at all three layers (GET dep, route guard, usePermissions); require_account_owner_or_admin delegates to User.can_manage_account; User.account_role TS type gains 'admin'.
• Finding 8 — pending_node column; /next-node replays the served node on re-mount (no duplicate paid generation); reads context off the session (no ticket re-fetch).
• Finding 9 — meta entry gone → empty walk is falsy (no junk proposal) and the depth cap counts only real steps.
• Finding 10 — escalate passes target_ids or None (default fallback), filters deleted_at IS NULL, warns when empty; two tests.
• Cleanups — dead ticket_ref deleted, IntakeResponse per-outcome validator + ticket_kind Literal restored, unused acknowledged dropped, escalations partial index added, restored the deleted no_kb_content audit assertion.

Verification: full Phase 2A backend set 110 passed / 0 failed; frontend tsc -b + eslint + vite build clean; migration upgrade→downgrade→upgrade roundtrip clean (columns + FK confdeltype + partial index confirmed); anti-parrot guardrail green.

How to use this file: work the findings in order. Findings 1–7 are merge blockers; 8–10 can be fast-follows. Each finding lists the verified evidence (file:line) and a suggested fix. Several findings share two root causes — fix those at the root rather than patching symptoms:

• Root cause A: AI-generated nodes have no id, but the advance protocol keys on node_id. (Finding 1; touches 8.)
• Root cause B: The intake category is smuggled into walked_path as a fake {"node_type":"meta"} entry that every consumer must know to skip — and most don't. (Findings 2b, 9; the deeper fix is a real category column on l1_walk_sessions, plus problem_text while you're there — see Finding 8's note.)

---

MERGE BLOCKERS

1. AI walkthrough can never advance past the first question (showstopper)

Evidence:

• backend/app/services/ai_tree_builder.py:37-41 — SYSTEM_PROMPT's JSON output shapes ({"node_type":"question","text":...} etc.) define no id field; validate_node (lines 71-79) returns the node unchanged; nothing anywhere assigns an id.
• backend/app/services/l1_session_service.py:156 — advance_ai_build only appends to walked_path if node_id is not None; docstring (line 139) says "On the first call (node_id is None) nothing is appended."
• frontend/src/components/l1/L1WalkTreeVariant.tsx:52-54 — sends node_id: node.id, which is undefined at runtime (server never sends an id; JSON.stringify drops undefined keys) → backend always receives node_id=None.
• l1_session_service.py:174 — session.current_node_id = next_node.get("id") is always None.

User impact: Tech answers the first question → answer is discarded, the same (or a re-rolled) first question regenerates forever. walked_path never grows past the meta entry, the depth cap never fires, and resolve captures an empty tree.

Why tests missed it: test_l1_api_ai_build and friends mock advance_ai_build / hand-craft nodes with id keys — a shape the real model is never instructed to produce.

Fix: Assign a server-side id to every generated node before returning it (e.g., uuid4().hex[:8] in generate_next_node after validate_node), persist it as session.current_node_id, and add a test that runs the real (unmocked-shape) prompt contract: generate → assert node has id → advance with that id → assert walked_path grew. Do NOT ask the LLM to invent ids.

2. Escalations from AI sessions go nowhere (two linked defects)

2a — Component never mounted.

• grep -rn "L1EscalationsSection" frontend/src → exactly one hit: its own definition (frontend/src/components/l1/L1EscalationsSection.tsx:10). It is imported nowhere.
• frontend/src/pages/EscalationQueuePage.tsx:3 imports only EscalationQueue, EscalationMetricCard from @/components/flowpilot.
• backend/app/services/notification_service.py:449 — "l1.session.escalated": "/escalations" deep-link → frontend/src/router.tsx:299 renders EscalationQueuePage → engineer sees only FlowPilot escalations; the L1 handoff is invisible. GET /l1/escalations (backend/app/api/endpoints/l1.py:330) has no UI surface.
• Note: .ai/HANDOFF.md:38 claims "L1EscalationsSection on EscalationQueuePage" — that claim is false (documentation drift; see Finding 3).

2b — Component renders wrong fields once mounted.

• backend/app/services/l1_session_service.py:162-168 — ai_build entries are {"node_type", "id", "text", "answer", "l1_note"} (key is text); legacy record_step (lines 199-204) uses question/node_id.
• L1EscalationsSection.tsx:61 renders {step.question} → blank for every ai_build entry.
• L1EscalationsSection.tsx:46 — {s.walked_path.length} steps walked counts the hidden meta entry → "N+1 steps walked".
• backend/app/api/endpoints/l1.py:41 — _to_response returns walked_path raw (meta entry included).

Fix: Mount L1EscalationsSection on EscalationQueuePage (or fold L1 rows into the existing queue). Render step.question ?? step.text. Filter node_type === 'meta' entries — ideally server-side in _to_response (or eliminate the meta entry entirely per Root cause B). Also use the shared timeAgo util (frontend/src/lib/timeAgo.ts) instead of new Date(...).toLocaleString() at line 50, to match every sibling queue.

3. Two tasks recorded as complete were never committed

• Task 16 ("ProposalDetail L1-source block") and Task 17 (mounting the escalations section) appear in .ai/HANDOFF.md / SESSION_LOG.md as done, but no hunk for ProposalDetail.tsx exists in the diff, and Finding 2a proves the mount never happened.
• Concrete user impact today: frontend/src/components/flowpilot/ProposalDetail.tsx:91-101 renders the "Source Session" card unconditionally; line 95 is to={`/pilot/${proposal.source_session_id}`} with no null guard. L1-sourced proposals (created with source_session_id=None, l1_session_id=<session>) reach the review queue as pending → engineers get a broken /pilot/null link.

Fix: Implement the missing work: in ProposalDetail.tsx, gate the /pilot/ link on source_session_id != null and render an L1-source block (problem statement, category, link to the L1 session / escalations view) when l1_session_id is set. The backend already serves l1_session_id via backend/app/schemas/flow_proposal.py. Then correct .ai/HANDOFF.md.

4. "Use this flow" button silently does nothing

• frontend/src/pages/l1/L1Dashboard.tsx:77-86 — useSuggestedFlow re-POSTs /l1/intake with the same text, no flow_id. The in-code comment ("it matches again and returns a matched outcome") is factually wrong: the same text scores in the same 0.60–0.75 suggest band (backend/app/services/match_or_build.py:66-72, MATCH_THRESHOLD = 0.75 line 21) → suggest again, no session_id → handler falls to resetPrompts() and the card vanishes. The suggested flow can never be started.
• backend/app/api/endpoints/l1.py — the rewritten intake never reads payload.flow_id (old branch deleted, diff confirms); IntakeRequest.flow_id (backend/app/schemas/l1.py:13) is now dead.

Fix: Make intake honor an explicit flow_id (bypass the matcher, call start_flow_session directly — restores the deleted behavior), and have the suggest card pass near_miss.flow_id. This also kills the wasteful re-run of the embedding + pgvector + keyword pipeline just to rediscover a flow_id the client already holds.

5. Out-of-scope problems lost the ad-hoc walk fallback

• Old intake had else: start_adhoc_session(...); the rewrite (backend/app/api/endpoints/l1.py:88-102) dispatches only matched/build/suggest/out_of_scope. start_adhoc_session (l1_session_service.py:82) now has zero callers — ad-hoc sessions are unreachable product-wide (the only remaining session_kind="adhoc" creation is escalate_without_walk, an audit record, not walkable).
• L1Dashboard.tsx:269-292 — out_of_scope prompt offers only "Escalate to engineering" / "Cancel".
• Stale copy: frontend/src/pages/account/L1CategoriesPage.tsx:57-58 still promises "Disabled categories fall back to an ad-hoc walk or escalation." A diff test comment also claims adhoc "is offered from the out_of_scope prompt" — it is not.

Fix (decide deliberately, don't drift): Either (a) add a "Walk it ad-hoc" option to the out_of_scope prompt that hits a path creating an adhoc session (restore the capability), or (b) if dropping ad-hoc is intentional, fix the L1CategoriesPage copy and the test comment, and note the decision in .ai/DECISIONS.md. Option (a) preserves pre-existing user capability; recommend (a).

6. DB constraint makes L1-session deletion always fail (time bomb)

• backend/app/models/flow_proposal.py:60-63 — CheckConstraint("(source_session_id IS NOT NULL) <> (l1_session_id IS NOT NULL)") (XOR).
• Line 87-92 — l1_session_id FK is ondelete="SET NULL"; source_session_id (line 83) is ondelete="CASCADE".
• Migration backend/alembic/versions/1fd88a68b145_flow_proposal_l1_source_linkage.py:32-45 ships the same DDL.
• Postgres CHECK constraints are non-deferrable and ARE evaluated on the UPDATE produced by ON DELETE SET NULL. So deleting any l1_walk_sessions row referenced by a proposal (whose source_session_id is NULL by construction) → both columns NULL → CHECK violation → the DELETE fails. The SET NULL action can literally never fire successfully.
• Reachable today via backend/app/api/endpoints/admin.py:1336 (hard_delete_user → db.delete(account), DB-side cascades with unspecified ordering), and via any future GDPR/retention purge.

Fix: Change l1_session_id to ondelete="CASCADE" (matching source_session_id's behavior — proposal dies with its source), in both the model and a new migration. Keep the XOR check. Alternative (num_nonnulls(...) >= 1 style relaxation) is weaker; prefer CASCADE.

7. Account admins locked out of L1 category settings (3-layer inconsistency)

• Frontend route: frontend/src/router.tsx:368-372 — requiredRole="owner"; frontend/src/hooks/usePermissions.ts:21-28 (getEffectiveRole) has no admin branch → account_role='admin' maps to viewer → bounced to /trees.
• Backend GET: backend/app/api/endpoints/accounts.py:175-178 uses require_l1_or_above (deps.py:235-242: l1_tech/engineer/owner only) → admin gets 403 on read.
• Backend PATCH: accounts.py:193-197 uses require_account_owner_or_admin (deps.py:279-289) → admin can write.
• admin is a real role: backend/app/models/user.py:25 CHECK constraint; user.py:132 treats admin as account-manager.

Fix: Pick one rule — owner+admin manage L1 categories — and apply it at all three layers: GET should use require_account_owner_or_admin too (or a combined dep), and the route guard needs admins to pass (either add an admin branch to getEffectiveRole — check blast radius on other requiredRole uses first — or a dedicated canManageAccount-style guard for this route). Also note require_account_owner_or_admin duplicates User.can_manage_account (user.py:130-132); delegate to it.

---

FAST-FOLLOWS (real bugs, lower urgency)

8. Every walk-view mount fires a fresh paid LLM call (and may swap the question)

• The served-but-unanswered node is never persisted: l1_session_service.py:156-174 — node_id is None path goes straight to generate_next_node; only current_node_id (always None today, see Finding 1) is stored. No replay branch.
• L1WalkTreeVariant.tsx:26-44 — mount effect unconditionally POSTs /next-node {}; frontend/src/main.tsx:4,34 — StrictMode is on, so dev double-mounts double-generate.

Impact: Refresh/back-forward = duplicate Sonnet spend, multi-second stall, and possibly a different question than the one the tech was answering.

Fix: Persist the pending node (e.g., a pending_node JSONB column on l1_walk_sessions, or reuse current_node_id + stored payload) and replay it when node_id is None and a pending node exists. Note: if adding columns, this is the moment to also add category and problem_text columns and delete the meta-entry convention (Root cause B) — /next-node currently re-fetches the internal ticket and re-scans walked_path on every step (l1.py:302-310) just to recover these immutable values.

9. Hidden meta entry: junk proposals + depth cap off-by-one

• Junk proposal: l1_session_service.py:270 — if helpful and session.session_kind == "ai_build" and session.walked_path: — a meta-only walked_path (seeded at intake, l1.py:132-134) is truthy. normalize_walked_path (ai_tree_builder.py:131-137) strips meta → empty → returns the "Empty walk — needs authoring." stub, which "passes the proposal approval guard" per its own docstring → a status="pending", validated_by_outcome=True junk proposal reaches the review queue when a tech resolves immediately after intake.
• Depth cap: l1_session_service.py:172-173 passes the raw walked_path; ai_tree_builder.py:82-83,96-98 — len(walked_path) >= MAX_DEPTH (12) counts the meta entry → cap fires after 11 real steps. _strip_meta is applied only downstream.

Fix (symptom-level): strip meta before both the truthiness guard and escalate_if_depth_exceeded. Fix (root): real category column, delete the meta convention (see Finding 8 note). Root fix preferred per project principle (correct architecture over minimal diff).

10. Escalation notification silently dropped when recipient query is empty

• notification_service.py:180 changed if target_user_ids: → if target_user_ids is not None: (intentional, documented at lines 176-178).
• l1_session_service.py:371-381 — escalate() passes its computed target_ids unconditionally; if all owners/admins/engineers are inactive, [] → zero in-app notifications, no log, no fallback. (Existing callers are safe — they all use [x] if x else None patterns.)
• Bonus divergence: escalate's hand-rolled query filters only is_active, while handoff_manager.py:323-333 also filters deleted_at IS NULL — soft-deleted engineers would be notified.

Fix: In escalate(): target_user_ids=target_ids or None (falls back to default recipients) plus a warning log when empty; add the deleted_at filter. Longer-term: give _resolve_recipients a roles parameter so callers stop hand-rolling recipient queries.

---

Cleanups (optional, do alongside adjacent fixes)

• L1Dashboard.tsx:47-110 — handleStart / useSuggestedFlow / buildNew are three near-identical intake calls; collapse to one runIntake(opts) switching on response.outcome (this also prevents Finding-4-class drift).
• backend/app/schemas/l1.py — IntakeRequest.flow_id is dead unless Finding 4 revives it; NextNodeRequest.acknowledged is sent by the frontend but never read by the backend (advance infers ack from answer is None) — wire it or drop it. IntakeResponse lost its per-outcome guarantees (all fields Optional, ticket_kind no longer Literal["psa","internal"]); add a model_validator(mode="after") requiring session_id/ticket_id when outcome is matched/build, and add a session_id null-guard before navigate() in handleStart (L1Dashboard.tsx:58-59 — currently navigates to /l1/walk/undefined on a regression).
• backend/app/services/match_or_build.py:55 — unused positional ticket_ref param (only caller passes ""); delete it. Also note classify() is a second bespoke LLM intake classifier alongside flowpilot_engine._classify_intake; l1.py passes problem_domain=None to matching, losing the domain signal the existing classifier provides — consider unifying in Phase 2B.
• backend/app/services/l1_category_service.py:17 + models/account.py:75-81 — DEFAULT_L1_CATEGORIES duplicated as a hand-escaped JSON server_default; derive one from the other (migration copy stays frozen).
• frontend/src/pages/account/L1CategoriesPage.tsx — local prettify() duplicates humanizeFeatureKey (UpgradePrompt.tsx:62); page skips shared PageHeader/Spinner used by sibling settings pages.
• Missing index for GET /l1/escalations (l1.py:338): consider CREATE INDEX ... ON l1_walk_sessions (account_id, last_step_at DESC) WHERE status = 'escalated'.
• backend/tests/test_l1_session_service.py — the escalation_reason_category == "no_kb_content" assertion was deleted from test_escalate_without_walk_writes_audit_log, weakening audit coverage; restore it.
• Per-step walked_path rewrite is O(n²) cumulative bytes (session.walked_path = [*session.walked_path, entry]); bounded by MAX_DEPTH=12 so fine today — note for Phase 2B if depth grows.

---

Suggested execution order

1. Finding 1 (node ids) — unblocks everything; add the contract test.
2. Finding 6 (FK/constraint) — new migration; do early so it ships in the same release.
3. Findings 2 + 3 together (mount section, fix field names/meta filter, ProposalDetail L1 block + null-guard the /pilot link).
4. Finding 4 (intake honors flow_id; suggest card passes it).
5. Finding 5 (decide adhoc: restore option (a) recommended, or fix copy + DECISIONS.md).
6. Finding 7 (align all three permission layers).
7. Findings 8 + 9 via the root fix: add category/problem_text (+ optionally pending_node) columns, delete the meta-entry convention, strip-meta fixes become moot.
8. Finding 10 (one-line guard + deleted_at filter).
9. Cleanups opportunistically alongside the file they touch.

After fixes: run the 11 Phase 2A backend test files together (authoritative gate per HANDOFF — do NOT trust a full local serial pytest tests/; use --override-ini="addopts="), frontend tsc -b + lint + build, and migration downgrade/upgrade roundtrip. Update .ai/HANDOFF.md to correct the Task 16/17 record.