From b3506b5e73d73bdc8260ba31da64d680746ec8b0 Mon Sep 17 00:00:00 2001 From: Michael Chihlas Date: Fri, 24 Apr 2026 16:09:23 -0400 Subject: [PATCH] docs(pilot): phase 9 review issues Review findings companion to docs/FlowAssist_Migration/Issues/phase-8-review-issues.md. Documents the issues addressed by commit 24972e8 (partial-outcome notes + per-fix script-builder remount). Co-Authored-By: Claude Opus 4.7 --- .../Issues/phase-9-review-issues.md | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 docs/FlowAssist_Migration/Issues/phase-9-review-issues.md diff --git a/docs/FlowAssist_Migration/Issues/phase-9-review-issues.md b/docs/FlowAssist_Migration/Issues/phase-9-review-issues.md new file mode 100644 index 00000000..dfad62e6 --- /dev/null +++ b/docs/FlowAssist_Migration/Issues/phase-9-review-issues.md @@ -0,0 +1,87 @@ +# Phase 9 Review Issues + +Date: 2026-04-24 + +Scope reviewed: +- `backend/app/api/endpoints/script_builder.py` +- `backend/app/api/endpoints/session_suggested_fixes.py` +- `backend/app/services/script_builder_service.py` +- `frontend/src/pages/AssistantChatPage.tsx` +- `frontend/src/components/pilot/ScriptBuilderTab.tsx` +- `frontend/src/components/pilot/EscalateInterceptDialog.tsx` + +## 1. "Applied partially" from the escalation intercept cannot persist + +Severity: High + +The escalation intercept offers an "applied partially" choice, but the frontend +sends `applied_partial` without notes. The backend requires notes for that +outcome and returns 400. The frontend catches the error silently and still opens +the conclude modal, so the user can believe the partial outcome was recorded +when it was not. + +Relevant files: +- `frontend/src/pages/AssistantChatPage.tsx:659` +- `frontend/src/components/pilot/EscalateInterceptDialog.tsx:56` +- `backend/app/api/endpoints/session_suggested_fixes.py:316` + +Why this matters: +- `handleInterceptChoice()` maps the partial button directly to + `patchOutcome(..., "applied_partial")`. +- The call does not provide `notes`. +- `PATCH /suggested-fixes/{fix_id}/outcome` rejects `applied_partial` without + notes. +- The catch block is silent and the UI continues into the conclude flow. +- The recorded fix status therefore remains unchanged while the user sees a + flow that implies the partial outcome was accepted. + +Recommended fix: +- Prompt for partial notes before calling `patchOutcome()` with + `applied_partial`. +- Do not proceed to the conclude modal if the partial outcome write fails. +- Consider hiding or disabling the partial option when it is not applicable, or + pass the current fix status into `EscalateInterceptDialog` so it can render + valid choices only. +- Add a regression test covering the partial escalation-intercept path. + +## 2. Script Builder can attach stale script state to a newer active fix + +Severity: Medium/High + +`ScriptBuilderTab` keeps local builder state across active-fix changes within +the same pilot chat. If a new active fix supersedes the previous one while the +tab remains mounted, old messages, `latestScript`, or editor text can remain in +memory while submission uses the new `fix.id`. + +Relevant files: +- `frontend/src/components/pilot/ScriptBuilderTab.tsx:55` +- `frontend/src/components/pilot/ScriptBuilderTab.tsx:78` +- `frontend/src/components/pilot/ScriptBuilderTab.tsx:150` +- `frontend/src/pages/AssistantChatPage.tsx:399` +- `frontend/src/pages/AssistantChatPage.tsx:1630` + +Why this matters: +- `ScriptBuilderTab` initializes `editorBuffer`, messages, and latest script + from props and builder-session data. +- The create/resume effect depends on `pilotSessionId`, not `fix.id`. +- `AssistantChatPage` detects active-fix changes but only closes the script + panel. +- The rendered `ScriptBuilderTab` is not keyed by active fix id. +- Submitting a stale builder draft calls the script patch endpoint with the + current `fix.id`, so an older script can be attached to a newer fix. + +Recommended fix: +- Reset Script Builder local state when `activeFix.id` changes. +- Key the rendered `ScriptBuilderTab` by `activeFix.id` if the intended UX is a + fresh builder surface per fix. +- If inline builder conversations are intended to resume per fix, extend the + backend idempotency model to include the fix id instead of only + `(user_id, ai_session_id)`. +- Add a frontend regression test for an active fix changing while the Script + Builder tab is mounted. + +## Review Context + +This review was based on code inspection of the latest committed Phase 9 +implementation. No tracked working-tree diffs were present at review time. +