Follow-up on #751: code review found 13 issues in the agent-mode fix, 2 of them real defects in the tests #752

/code-review xhigh was run on #751 after it merged — my process error; the gate belongs before the merge, and it would have caught all of this pre-merge. Filing rather than hot-fixing: none of it is user-facing breakage, and the shipped fix is live-verified working for the reported case.

Assessments below are mine, after checking each claim against the tree. I disagree with two of them in part, and say so.

Confirmed defects — fix these

1. tests/e2e/test_migrated_host_e2e.py:157 — monkeypatch.undo() reverts the isolation fixtures too. Verified: tests/conftest.py:77 _isolate_settings is autouse and takes monkeypatch; tests/e2e/conftest.py sets GFLOW_CLI_HOME through the same function-scoped instance. undo() consumes the whole stack, so from that line on get_settings() resolves against the developer's ambient environment — this is #86's catalog-pollution mode reintroduced mid-test. Fix: with monkeypatch.context() as m: m.setattr(...) around the control arm only. Highest-value finding in the set.

2. tests/e2e/test_migrated_host_e2e.py:146 — the test drives the account into agent mode with no restore in finally. A test that mutates persistent server-side account state must restore it. Fix: unconditional button.agent-mode-chip[aria-pressed='true'] click in the finally.

Severity note — the review says this leaves the profile "permanently bricked"; that overstates it. Every sibling test in the file routes through ensure_editor, which now leaves agent mode automatically, so it self-heals on the next run. It is still wrong, and it WOULD strand a user still on 0.71.0.

3. scripts/dev/spike_migrated_composer_arms.py:242 — the probe uses button[aria-pressed], which its own PR documents as ambiguous. migrated_composer.py says in as many words that a second button[aria-pressed] (agent-action-button, ligature article_spark) appears in agent mode, and the spike doc's "What was NOT measured" says to anchor on .agent-mode-chip. The script does neither. Re-run against a profile already in agent mode and .first may toggle the wrong control, making a non-transition look like evidence. Anchor on button.agent-mode-chip[aria-pressed].

Diagnostic quality — the fix works, the messages can mislead

4. migrated_composer.py:566 — "the chip was clicked to leave it" is printed even when the click raised, and that exception is dropped into a truncated warning instead of being chained. A "Get started" modal intercepting the click sends the user to toggle a chip when the real cause was the modal.

5. migrated_composer.py:530 — "the mode may be pinned" is claimed without re-reading aria-pressed. Agent mode + genuine selector drift reports as pinned mode, and the real drift bug never gets filed. One count() re-check separates them. No unit case covers agent_mode=True + trigger_present=False.

6. migrated_composer.py:524 — after a failed click it still burns the full 20 s on a trigger it knows did not change. Worst-case path goes 30 s → ~55 s. A tri-state return (clicked / found-but-not-clicked / absent) fixes 4, 5 and 6 together.

7. migrated_composer.py:668 — _open_pane still guards with count(), not visibility. This is the same present-but-hidden confusion #749 turned on, at the second gate: a mode flip between ensure_editor and apply_video_settings escapes as a bare Playwright TimeoutError with no exit-23 mapping and no mention of agent mode. Good catch — the fix's own insight was not applied consistently.

Lower confidence

8. Recovery is keyed off the symptom, not the state (migrated_composer.py:515) — if a cohort renders the agent prompt box while leaving the trigger visible, send_prompt would type into the agent composer via [contenteditable='true'].first. No such cohort is measured, so this is speculative and I would normally leave it — but probing after a successful wait costs one count(), which is cheap enough to just do.

Hygiene

9. No KNOWN_ISSUES.md entry. Valid: the two siblings this fix's own spike names — #493 and #721 — both have entries, and the pinned-mode message gives a user nothing to search for.
10. The A/B control asserts only the exception type, so it passes on either drift message and would stop discriminating if the recovery ever fired wrongly. match="did not become visible" pins it at zero cost.
11. Dom.agent_chip_present is a dead knob — never set False by any test. Either add the agent_mode=True + no-chip case or drop the field.
12. visible: bool | Any collapses to Any, erasing the annotation under pyright strict. Should be bool | Callable[[], bool].
13. The same measurement narrative is written out five times (constant comment, docstring, test comments, CHANGELOG, spike doc). Fair, and it is the duplication-drifts failure AGENTS.md warns about. The constant needs the selector rationale plus a spike pointer; the history belongs in the spike alone.

Refs #749