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 <noreply@anthropic.com>
This commit is contained in:
2026-04-24 16:09:23 -04:00
parent b14a16a1ab
commit b3506b5e73

View File

@@ -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.