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