fix(web): re-evaluate Plan-mode auto-open when the post-turn file list settles - #6842
fix(web): re-evaluate Plan-mode auto-open when the post-turn file list settles#6842maxmilian wants to merge 8 commits into
Conversation
…t settles The turn-end auto-open pass decides from one `fresh: true` file-list read taken right after the daemon reports terminal status. The generated file does not always appear in that read — the daemon's write, the chokidar event, its coalescing window and the refetch behind it settle on their own clocks — and nothing revisits the decision, so `index.html` never becomes an openable tab. Keep nexu-io#5602's centralised turn-end sequencing and instead re-run the same selection against each settled file list, until it resolves, its 15s window expires, or focus moves somewhere the turn did not put it. Touched file NAMES are re-resolved per list, because that resolution reads the list too: a list still missing `index.html` also fails to resolve the write that produced it. Refs nexu-io#5352
|
Thanks @maxmilian — the re-evaluate-on-settle direction makes sense here, especially keeping the turn-end sequencing from #5602 intact while fixing the file-list race left in the handoff.
|
lefarcen
added
size/L
|
One quick follow-up from triage: this will need a manual QA pass before merge because it changes the Plan-mode auto-open behavior in the web app, so please hold off self-merging once reviews are green. Also, could you update the Surface area checklist and tick the user-visible box that matches this change? The rest of the write-up is already very clear. |
Visual regression reviewHead:
0 changed · 0 unchanged · 0 new without baseline · 1 failed Capture or diff failures
Visual diff is advisory only and does not block merging. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found two blocking lifecycle races in the new settle watcher. The detailed inline comments explain how a delayed completion can outlive a newer turn and how a tab change during the async handoff can be mistaken for the baseline. Please add an ownership/focus witness before merging so the retry cannot move the user to stale output.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| ], | ||
| autoOpenArtifactOptions, | ||
| ); | ||
| const activeFileNameAtTurnEnd = openTabsActiveRef.current; |
There was a problem hiding this comment.
Blocking: this snapshot is taken only after the completion continuation has awaited the post-run file refresh, artifact recovery/persistence, and the second refresh. The user can change tabs during that interval; that tab then becomes activeFileNameAtTurnEnd, so a later settled list treats the user's still-focused tab as the turn's baseline and reopens the resolved artifact instead of stopping. This contradicts the stated focus-move guard and can yank the user out of a tab they chose. Capture the focus/activation witness at terminal handoff before these awaits (or track an explicit user-vs-auto activation sequence) and only allow the settle request when that witness is unchanged; add a regression test that changes the active tab while the finalizer is waiting.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // support file first — and nothing revisits it. Hand the same | ||
| // turn inputs to the settle watcher so the next file lists that | ||
| // land re-run the selection instead. | ||
| pendingAutoOpenSettleRef.current = { |
There was a problem hiding this comment.
Blocking: clearing pendingAutoOpenSettleRef when a new turn starts does not protect this assignment because it runs in an unawaited completion continuation. Turn A can mark itself terminal, then a new send reaches the clear at line 6969 while A is still awaiting refresh/persistence; A subsequently reaches this line and installs its old producedFiles/resolver. The next turn's file-list generations then invoke evaluateAutoOpenSettle and can request Turn A's artifact while Turn B is running. Guard the continuation with a monotonic auto-open generation or run-owner token captured at send start, and check ownership immediately before arming/evaluating so an older finalizer cannot re-arm the watcher; add an overlap regression test.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks for the careful pass here. @nettee's two blocking comments on the current head look like the right next step: the settle watcher needs a turn-ownership guard so an older completion can't re-arm it, and the focus witness needs to be captured before the async completion handoff so a user tab switch isn't mistaken for the turn baseline. Once those are addressed, happy to take another look. |
Review on nexu-io#6842 found two lifecycle races in the settle watcher, both rooted in the completion continuation being an unawaited async IIFE. The focus witness was sampled after the post-run refresh, artifact recovery/persistence and the second refresh had all been awaited. A tab the user selected during that window therefore became the turn's own baseline, so the focus-move guard read their choice as "where the turn put focus" and reopened the artifact over it — the exact yank the guard exists to prevent. Sample it at terminal handoff instead, before those awaits. Moving the sample alone is not enough: the continuation opens a recovered same-turn write itself, so the turn's own activations are now recorded and travel with the request as turnOwnedFileNames rather than reading as the user moving on. Clearing the pending watch when a new turn starts also could not protect the arming site, because an older finalizer reaches it after the clear runs and reinstalls its own producedFiles/resolver; the newer turn's file-list generations then drove the older turn's artifact into focus. Add a monotonic generation bumped per send, checked immediately before arming and again before every evaluation. The immediate turn-end open predates this change and is left as is. Refs nexu-io#5352
lefarcen
added
size/XL
|
Thanks for pushing the follow-up. I re-requested @nettee on the new head since their current-head blockers were the ones this update was aimed at. One quick PR-body follow-up while that review refreshes: could you add the Surface area checklist and a short Bug fix verification note for the red→green seam? The write-up already explains the race and the fix path clearly — those are the two remaining template pieces that would help the next pass. |
|
Thanks @nettee — both findings are real, and I reproduced the timing for each rather than taking them at face value. Fixed in ① Focus witness sampled too late ( Confirmed. The snapshot sat after Moving the sample earlier alone isn't enough, though: the continuation itself moves focus during that window (
I kept the field name ② Finalizer re-arming a watcher it no longer owns ( Confirmed, and it's an ordinary interleaving rather than a narrow race: the clear at Added a monotonic On the tests — worth describing, because my first two attempts were silently vacuous and both traps are easy to fall into:
Both regression tests fail on the pre-fix tree ( One thing I deliberately left alone: the immediate Full |
|
Thanks for writing this up so concretely. Reproducing both races, documenting why the first fixes were too weak, and adding the ProjectView-level regression coverage is exactly the kind of update that makes the next pass easier to trust. I've already re-requested @nettee on this head, so the useful next step is their refreshed read on the watcher ownership/focus path. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian I reviewed every changed range, including the new pure selector, the ProjectView lifecycle, the regression tests, and the coverage note. The focused auto-open suites pass (58/58) and web typecheck is clean, but three stale-auto-open paths still bypass or outlive the new owner token; the inline comments show concrete interleavings that can focus an older turn's artifact during a newer turn or conversation. Please address those ownership boundaries before merging.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/components/ProjectView.tsx RIGHT line 7155
Blocking: this unawaited per-write refresh is outside the new generation fence. If Turn A's refresh resolves after Turn B starts, completionSelectedAutoOpen can still be false and this branch calls requestOpenFile(decision.fileName) using A's file path; the settle-generation check later in the completion path does not protect this callback. A late A write can therefore steal focus during B even when the terminal watcher is skipped. Capture the run generation at send start and require it to match immediately before this request (ideally through one generation-aware auto-open helper), then add a delayed per-write overlap regression.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Inline comment could not be anchored: anchor_outside_complete_diff
| // generation gives that finalizer a token to check against, so it can | ||
| // recognise that it no longer owns auto-open instead of reinstalling | ||
| // itself over this turn. | ||
| const autoOpenSettleGeneration = ++autoOpenSettleGenerationRef.current; |
There was a problem hiding this comment.
Blocking: this token is advanced only after the async preflight/queue gates, and it is not tied to the active conversation. ProjectView intentionally stays mounted while activeConversationId changes, so if Turn A's finalizer is pending and the user switches to conversation B (or B is waiting in AMR preflight), this line has not run; A can later arm/evaluate its request against B's file-list generations and focus A's artifact. Advance/invalidate the owner before those awaits and clear/retire the pending request on conversation/authority changes (or store the conversation id in the request and reject mismatches). Add a switch-before-finalizer regression.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // behind unawaited awaits, so a newer send may already have taken | ||
| // over; arming here would then let THIS turn's artifact ride the | ||
| // NEXT turn's file-list generations into focus. | ||
| if (autoOpenSettleGenerationRef.current === autoOpenSettleGeneration) { |
There was a problem hiding this comment.
Blocking: this check is too late to protect the completion path. If Turn A is paused in the awaited refresh/persistence and Turn B starts, A can execute requestTurnOpenFile(sameTurnWrite.name) or requestTurnOpenFile(producedArtifactToOpen) above (and persistArtifact can call requestOpenFile) before reaching this if. The mismatch then skips only pendingAutoOpenSettleRef; the stale openRequest has already been sent and can focus A's file during B. Make every completion auto-open request generation-aware, including the artifact-persistence path, and add an overlap regression with a previewable post-run artifact.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks for the update here. @nettee's latest pass is the one to drive from now: the remaining blockers are the three stale-auto-open ownership paths called out in that review, so once those are addressed on a new head we should be in a much clearer spot. |
…oken Second review round on nexu-io#6842 found three stale-auto-open paths that either bypass or outlive the owner token the previous round introduced. The token only fenced the settle watcher's arming site. Everything else a run can ask focus for is unawaited too: the per-write refresh that lets a file open while the run streams, and the completion continuation's own opens, which sit above that arming site behind the post-run refresh and artifact persistence. A run parked in any of them can resolve after a newer send has taken over and pull focus to its own output during that newer run — the same stale focus the watcher fence exists to stop, just arriving directly instead of through the watcher. Route all of them through one run-scoped helper that checks the token before requesting focus, and count only a request that actually went out as turn-owned focus. The token cannot cover a conversation switch: switching chats starts no new turn, so nothing bumps it. ProjectView outlives conversation switches (only ChatPane is keyed by the active conversation) and the file workspace is project-scoped, so a watch armed in one chat stayed live and could focus that chat's artifact underneath the new one. The pending watch now carries its conversation and is retired as soon as the user leaves it. Three regressions, each red without the fix: a superseded completion-path open with a previewable post-run artifact, a delayed per-write refresh settling during a newer run, and a conversation switch before the finalizer lands. Each is paired with a positive control, so a "did not open" assertion cannot pass by the mechanism never firing. Refs nexu-io#5352
|
Thanks @nettee — I went through all three interleavings against the code rather than the summary. Two are real and are fixed in Fixed — every auto-open a run can request now goes through one owner-token fence. You are right that the token only guarded the arming site. The per-write refresh and the completion continuation are unawaited too, so a run parked in either can resolve after a newer send has taken over and pull focus to its own output — the same stale focus, just not arriving through the watcher. All of them now route through a single run-scoped helper that checks the token before requesting focus, and only a request that actually went out counts as turn-owned focus (a suppressed one moved nothing, so it must not widen the focus-move guard's "where the turn put focus" union). That covers the completion path you flagged ( Fixed — the watch is now conversation-scoped. This one the token genuinely cannot cover, and the repo says so itself: Not changed — advancing the token before the AMR preflight awaits. The premise is right: the bump sits after Scope — the recovery/replay completion paths. "Make every completion auto-open request generation-aware" is broader than this PR can reach: there are four more completion sites in the reattach / replay / recovery branches ( Regressions. Three new ones, each red with the fix stashed and green with it:
Each is paired with a positive control, because the two guards I wrote in the previous round initially passed for the wrong reason — the mechanism under test never fired. The per-write harness parks the refresh with an explicit hold armed right before the tool result rather than counting file reads (the send path takes its own pre-turn snapshot, so a fixed read index parks the wrong request), and keeps the turn's post-run list free of previewable files so the released per-write refresh is the only thing that can move focus. Local on |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian I found four blocking ownership races in the new settle watcher. The focused selector/lifecycle tests, web typecheck, and guard pass, but these async paths can still focus stale output after a conversation or run handoff. Please address the inline comments and add the overlap coverage described there before merging.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // send has taken over and move focus to this run's output during that one. | ||
| // Requesting focus for a run that no longer owns auto-open is the same | ||
| // stale-focus bug whether it arrives through the watcher or directly. | ||
| const requestRunOpenFile = (fileName: string) => { |
There was a problem hiding this comment.
Blocking: this generation-only gate still allows stale auto-open after a conversation switch. The generation is bumped when a new run starts, but switching from conversation A to B does not bump it, so a delayed per-write callback at the changed call sites below or a completion request can still pass this check for A and focus A's file in the shared workspace while B is active. The pending watcher has a conversation check, but these direct requests bypass it. Capture the run conversation and require it to match a live active-conversation ref before calling requestOpenFile, and route every run-owned opener (including persistence) through that check; add a switch-before-delayed-open regression.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| // diffing produced files). Nothing re-renders on arming alone, | ||
| // so evaluate once here rather than waiting for a further | ||
| // refresh that may never come. | ||
| evaluateAutoOpenSettle(); |
There was a problem hiding this comment.
Blocking: the conversation guard can be bypassed by the unawaited finalizer's stale closure. This call uses the evaluateAutoOpenSettle function captured by the handleSend render that started conversation A; after the user switches to B, that old evaluator still compares pending.conversationId against A's captured activeConversationId (the handleSend dependency list does not refresh this in-flight closure). If the settled artifact is already in projectFilesRef when the finalizer arms, it can request A's file before B's next effect clears the pending ref. Read the current conversation from a ref inside the evaluator (or call a current ref-held evaluator), and add a switch-before-finalizer test with the target already present in the accepted list.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| resolveTurnOptions: turnAutoOpenOptionsFor, | ||
| requestedFileName: producedArtifactToOpen ?? null, | ||
| activeFileNameAtTurnEnd: activeFileNameAtTerminalHandoff, | ||
| turnOwnedFileNames: [...turnAutoOpenedFileNames], |
There was a problem hiding this comment.
Blocking: this snapshot only contains files opened through requestTurnOpenFile in the completion continuation. The earlier per-write callbacks were changed to requestRunOpenFile, so an accepted per-write open such as plan.md that settles after terminal handoff is absent from turnOwnedFileNames. When the settle list later selects index.html, activeFileName=plan.md is then neither the handoff baseline, requestedFileName, nor a recorded turn-owned name, so reevaluateAutoOpenOnFilesSettled retires instead of upgrading even though the same turn caused the focus. Keep a run-scoped set updated by every generation-accepted opener and snapshot that set here, with a delayed same-run per-write-then-settle regression.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| savedArtifactRef.current = sameTurnWrite.name; | ||
| completionSelectedAutoOpen = true; | ||
| requestOpenFile(sameTurnWrite.name); | ||
| requestTurnOpenFile(sameTurnWrite.name); |
There was a problem hiding this comment.
Blocking: the new wrapper only protects the sameTurnWrite branch. If that branch misses, the else path immediately calls persistArtifact, whose pointer and successful-write paths still call raw requestOpenFile (ProjectView.tsx:3617 and 3682). If turn A is awaiting that persistence while turn B starts, A can resolve the write and focus its artifact after the generation changes; this request never reaches requestRunOpenFile. Pass the generation-aware opener into persistArtifact or remove its internal auto-open and issue requestTurnOpenFile only after the guarded persistence completes, then add a delayed-persistence overlap regression.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks for the detailed update here. @nettee's current-head review is the one to drive from now: the remaining blockers are the four ownership/focus races called out inline, so the useful next step is resolving those paths on a new head and then getting their refreshed pass. |
Third review round on nexu-io#6842 found four stale-auto-open paths. Three are real holes in the fence this PR builds; the fourth is declined on the PR comment. The conversation guard added last round was read from the render's `activeConversationId`, but the completion continuation calls the evaluator captured by the render that STARTED the run — a dependency list cannot refresh a closure already in flight. The guard therefore compared conversation A against conversation A and passed, exactly at the arming site where the finalizer arms and evaluates in one go, so a watch could still open A's file underneath B before the next effect-driven evaluation retired it. Read the conversation through a ref, and keep `activeConversationId` as an effect trigger so leaving the chat still retires the watch at once. The focus-move guard's turn-owned set was filled only by the completion continuation's own opens. A per-write refresh is fire-and-forget and can land its open after terminal handoff, so `plan.md` was neither the handoff baseline nor a recorded activation, and when `index.html` finally settled the watcher read the run's own focus as "the user moved on" and retired instead of upgrading — the bug this PR exists to fix, re-entering through the fix's own bookkeeping. Record every request the fence lets through, and hand the watch the live set rather than a copy taken before those late opens exist. `persistArtifact` auto-opens what it writes, straight to the unfenced opener. The completion path delegates to it in the branch next to the one it fences, and it is awaited, so a run parked on that write could still move focus after a newer send took over. It now takes the caller's opener. Three regressions, each red without its fix, each paired with a positive control: an arming-site evaluation after a conversation switch, a per-write open followed by a settled upgrade, and a persistence write that resolves during a newer run. Refs nexu-io#5352
|
Thanks @nettee — three of these four are real holes in the fence, and one of them is in the guard I added last round. All three are fixed in Fixed
Declined, with reasoning
Follow-ups already noted on this PR (unchanged from last round): the four completion paths in the reattach/replay/recovery branches (
|
|
Thanks for the detailed write-up on |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian The owner-token work is substantial, and the focused selector/lifecycle tests, web typecheck, and guard are green. Two blocking focus-ownership gaps remain in the changed path: the witness does not observe all workspace activations, and direct run-owned opens are not invalidated by a conversation switch. Both can still move a user's tab after they have chosen another workspace or chat; the inline comments describe the interleavings and regression coverage needed.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| }); | ||
| // Mirror for the run-completion continuation, which reads the active tab | ||
| // long after the render that captured it. | ||
| const openTabsActiveRef = useRef<string | null>(openTabsState.active); |
There was a problem hiding this comment.
Blocking: this focus witness is not the actual workspace focus. openTabsState.active contains only persisted tab activations, while FileWorkspace.activatePending() deliberately changes its local activeTab without calling onTabsStateChange for an unsaved sketch. Interleaving: the terminal handoff records notes.md; while the completion read is held, the user activates a pending sketch; when index.html settles, openTabsActiveRef still reports notes.md, so the watcher treats the handoff as unchanged and opens the generated file over the sketch the user chose. The same name-based witness cannot distinguish a later manual activation of a file that the run auto-opened. Propagate the effective workspace active tab plus a monotonic activation/user witness (including transient tabs) into the settle request, retire it after any user activation after handoff, and add a ProjectView regression that activates a pending sketch while the finalizer is held.
| // path's own opens. | ||
| const runAutoOpenedFileNames = new Set<string>(); | ||
| const requestRunOpenFile = (fileName: string) => { | ||
| if (autoOpenSettleGenerationRef.current !== autoOpenSettleGeneration) return false; |
There was a problem hiding this comment.
Blocking: this helper fences only the monotonic send generation. Switching from conversation A to B does not increment that ref, and ProjectView intentionally stays mounted while the project-scoped FileWorkspace remains shared. A delayed per-write refresh or completion/persistence continuation from A can therefore pass this check after the switch and call requestOpenFile for A's file under B; the settle watcher's conversation check is bypassed because these direct paths call requestRunOpenFile instead. Capture runConversationId in this helper and require activeConversationIdRef.current === runConversationId immediately before the request (or invalidate the helper on conversation change), then add a delayed per-write/completion conversation-switch regression.
The post-turn auto-open watch decided "has focus moved somewhere this turn
did not put it" from the active tab NAME, read through ProjectView's mirror
of the persisted tab state. That witness is blind in two ways, and both let
the watcher take focus away from a tab the user chose:
* It does not see every activation. `FileWorkspace.activatePending` flips
to an unsaved sketch locally and deliberately does not round-trip that
through `onTabsStateChange`, so while the completion continuation is
parked on its post-run read the user can activate a pending sketch and
the mirror still reports the tab the turn recorded at handoff. When
index.html settles, the guard finds focus exactly where the turn left it
and opens over the sketch.
* A name cannot say WHO activated it. A file this run auto-opened reads the
same whether the run opened it or the user picked it afterwards, so the
turn-owned union accepts a deliberate user choice as the run's own.
FileWorkspace now reports every activation it makes that the parent did not
ask for (`onUserActivateTab`), via one effect on `activeTab` rather than at
each of the twenty-odd activation sites, with the two parent-driven origins
— the `tabsState.active` hydration and the `openRequest` effect — marked so
a run's own auto-open cannot retire the watch it just armed. ProjectView
counts those reports in a ref, samples the count at terminal handoff beside
the focus witness, and `reevaluateAutoOpenOnFilesSettled` retires the watch
whenever it has moved. Anything unmarked counts as the user's: over-
reporting only ends a watch early, under-reporting steals focus.
The count is a required field on both the request and the evaluation context
so a future caller cannot drop the guard silently.
Tests: the pending-sketch interleaving at the ProjectView level (against the
existing positive control, which is what proves the watcher fires there at
all); both blind spots as unit cases plus a positive control that the count
is not a blanket off-switch; and the `onUserActivateTab` contract itself at
the FileWorkspace level, since ProjectView's suite mocks that component out.
(1)
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian The owner fencing and settle-window coverage are materially stronger, and I verified the changed ranges with pnpm guard and the web typecheck. One blocking ownership gap remains: the new activation witness treats every openRequest as an internal request, although that same path carries user-initiated chat file opens. The inline comment gives a concrete interleaving where a user's choice is then overwritten by the settle watcher and a targeted fix/test.
| } | ||
| // Marked so the reporter above does not read the parent's own auto-open as | ||
| // the user taking over — that would retire the very watch that issued it. | ||
| parentRequestedActivationRef.current = name; |
There was a problem hiding this comment.
Blocking: this marker treats every openRequest as a parent-owned activation, but requestOpenFile is also used for user-originated ChatPane file-link and chip clicks, and the only user signal is emitted later from the activeTab effect. That creates concrete under-reporting paths: a chat click is marked here and never increments the count, while a pending-sketch activation can be held in afterActiveManualEditSettles until after the settle watcher evaluates. In either case ProjectView still sees the baseline persisted focus (or a run-owned name) and can open the later higher-ranked artifact over the user's choice. Carry explicit request-source metadata and mark only run-owned requests, or report user intent before the asynchronous activation gate; add regressions for a ChatPane open while the watcher is pending and for a delayed pending-sketch activation.
|
Thanks for the latest round here. @nettee's current-head blocking review is the one to drive from now: the remaining gap is that the new activation witness still treats user-initiated Once that lands on a new head, we can re-check from there. |
…t prop `openRequest` carries two different kinds of open. It started as the chat file-chip click and later picked up the run's auto-opens, so FileWorkspace marking every arrival as parent-requested silently reclassified the user's click: the settle watcher then saw "the user has not chosen anything" and opened its own higher-ranked pick over the file she had just opened. Requests now carry an explicit `source`, required at every ProjectView callsite, and the workspace acts on it at the manual-edit settle gate — the single choke point every activation in there passes through. Reporting at the gate rather than off the landed activation also fixes the second half: an open manual edit parks the activation, so a report derived from it could arrive after the watcher's deadline, or never. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQKCR9FRJiexWWUpcoFLpm
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian I found two remaining blocking stale-focus races in ProjectView.tsx. The focused auto-open tests, web typecheck, and guard pass locally, but these asynchronous paths can still focus output from an older run or conversation. Please address the inline comments and add the overlap regressions before merging.
| // path's own opens. | ||
| const runAutoOpenedFileNames = new Set<string>(); | ||
| const requestRunOpenFile = (fileName: string) => { | ||
| if (autoOpenSettleGenerationRef.current !== autoOpenSettleGeneration) return false; |
There was a problem hiding this comment.
Blocking: this generation check does not include the active conversation. If run A is in the delayed per-write refresh, completion, or persistArtifact continuation, switching from conversation A to B leaves autoOpenSettleGenerationRef unchanged because no new send started. When A resumes, this check passes and requestOpenFile focuses A's artifact in the project-scoped workspace while B is active; the settle watch's conversation guard is bypassed by this direct opener. Capture runConversationId in the owner token and require activeConversationIdRef.current === runConversationId immediately before every run-owned request (or invalidate the token on conversation change), and add a delayed per-write/completion conversation-switch regression.
| setLiveArtifactEvents((prev) => appendLiveArtifactEventItem(prev, ev)); | ||
| void refreshLiveArtifacts().then(() => { | ||
| if (ev.action !== 'deleted') requestOpenFile(liveArtifactTabId(ev.artifactId)); | ||
| if (ev.action !== 'deleted') requestOpenFile(liveArtifactTabId(ev.artifactId), 'internal'); |
There was a problem hiding this comment.
Blocking: this live-artifact refresh callback bypasses the new run fence. refreshLiveArtifacts() is fire-and-forget, so after run A emits this event a newer send can advance autoOpenSettleGenerationRef; when the refresh resolves, this raw request still opens A's live:<id> tab during run B. It also is not added to runAutoOpenedFileNames, so if it lands after terminal handoff the settle watcher treats that run-owned focus as an external move and retires instead of upgrading the intended artifact. Route this callback through requestRunOpenFile and add a deferred-refresh overlap test so stale live-tab opens cannot cross a run boundary.
…acts through the run Two ways a run could still move focus somewhere it no longer owns. The generation token fences a newer SEND, because only a send advances it. Leaving the conversation advances nothing, so a run parked in a per-write refresh, the completion continuation, or persistArtifact still passed the token check on resume and opened its artifact into the workspace while another chat was on screen. The settle watcher already refused to do that; these direct openers were the way around it. Checked at the moment the request goes out rather than at arming, since the whole point is that an unbounded amount of time can pass in between. The live-artifact open was issued as a raw request from inside a fire-and-forget refreshLiveArtifacts() continuation, so nothing fenced it at all. Routing it through the run's opener also records the name, which matters separately: an unrecorded open landing after terminal handoff reads to the settle watcher as the user choosing something else, and retires the watch instead of upgrading. The live-artifact regressions live in their own file. Parking fetchLiveArtifacts needs a mock regime the sibling suite does not use, and running both in one file made an unrelated per-write test go red under full-suite load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQKCR9FRJiexWWUpcoFLpm
|
Both blocking points are real. Fixed in (1)
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian I found one remaining blocking stale-focus race in the new ownership path. The generation/conversation checks and focused regressions pass at this head, but the owner witness stops when openRequest is enqueued; a queued internal request can still activate after the run or conversation that created it has been replaced. Please carry the owner metadata through request consumption and cover the deferred handoff.
| const requestOpenFile = useCallback((name: string, source: WorkspaceOpenRequestSource) => { | ||
| if (!name) return; | ||
| setOpenRequest({ name, nonce: Date.now() }); | ||
| setOpenRequest({ name, nonce: Date.now(), source }); |
There was a problem hiding this comment.
Blocking: the new fence ends at setOpenRequest(...); the request carries only name, nonce, and source, so it loses autoOpenSettleGeneration and runConversationId before FileWorkspace consumes it. FileWorkspace handles an internal request in its [openRequest] effect and can park afterActiveManualEditSettles on the active viewer. If run A enqueues here and the user starts run B or switches conversations before that manual-edit promise resolves, A's queued callback still passes the gate (it only checks the source tab and activation sequence) and focuses A's file; the checks at lines 7101–7102 have already run and cannot stop it. This is stale focus in the shared project workspace the owner token is meant to protect. Carry the owner generation/conversation (or a cancellation predicate) through WorkspaceOpenRequest and re-check it when the gate resolves/activation executes, then add a regression with a deferred manual-edit exit plus a run/conversation handoff.
|
Thanks for writing up the deferred-handoff case so concretely. I read the new head and the latest blocker, and the actionable next step still looks like @nettee's current-head point: the owner metadata needs to survive the queued The red web/UI checks line up with that path still being in play, so once that handoff is fenced and the overlap regression is in, this should be in a much clearer spot. |
… runs Every ownership check so far ran when the request was made. The workspace can park an activation behind an unsettled manual edit for an unbounded time, and the gate that releases it only re-checks the source tab and the activation sequence — neither of which a run or conversation handoff changes. So a request enqueued by run A could still focus A's file after the user started run B or moved to another chat. Requests now carry `isStillOwned`, re-asked at the moment the activation executes. A predicate rather than data because only the requester knows what owning it means: run-owned opens carry their generation and conversation, the settle watch carries its conversation, and a user's click carries nothing — a user's choice cannot go stale by waiting. This closes the class rather than another instance of it: anything that becomes false between enqueue and activation is now caught at activation. Also polls the delayed per-write assertion instead of asserting on the tick after the release. That race made a green guard look like a blocking one under full-suite load, which is an expensive way to be wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQKCR9FRJiexWWUpcoFLpm
|
Confirmed, and this one is a different shape from the previous rounds. Fixed in You're right that the fence ended at What the fix does. const run = () => {
if (isStillOwned && !isStillOwned()) return;
action();
};It lives inside Why a predicate rather than carrying the generation and conversation as data. Only the requester knows what owning its request means, and the three kinds differ:
Passing data would have meant teaching Tests. The FileViewer mock in the timing suite now hands the test a resolvable exit handler instead of a permanently pending one — these two cases need the parked activation released after the handoff, to show it is dropped at that moment rather than merely still waiting.
Red-check: removing the re-check inside One thing you should know about, since it affected an earlier roundWhile verifying this I ran the full suite six times: green four times, and twice red — once on a test with no relationship to this change, and once on It asserted on the tick after I mention it because it means one earlier "the guard blocked it" reading in this PR's history was mistaken, and because if you see that test go red in future it is worth checking the assertion timing before the guard.
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@maxmilian I found one blocking ownership gap in the settle-watcher activation handoff. The queued request can outlive a newer same-conversation send; the inline comment on ProjectView.tsx:3867 details the interleaving and the regression needed.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| requestOpenFile( | ||
| decision.openFileName, | ||
| 'internal', | ||
| () => activeConversationIdRef.current === watchConversationId, |
There was a problem hiding this comment.
Blocking: this queued settle-watch request does not carry the turn generation into activation. Turn A can evaluate here, call requestOpenFile, and then have FileWorkspace park the request behind an active manual-edit exit. If Turn B starts in the same conversation while that promise is pending, handleSend increments autoOpenSettleGenerationRef and clears the pending watch, but this predicate still returns true because it compares only activeConversationIdRef. When the manual edit settles, FileWorkspace therefore opens Turn A's stale artifact during Turn B. That violates the newer-send ownership fence added above and can steal focus from the current run. Capture/check pending.generation (or pass the same runStillOwnsAutoOpen-style predicate through the watcher) at release time, and add a regression that defers manual-edit settlement, starts a same-conversation second send, then releases the gate and asserts no stale open.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks for spelling out the new head so concretely. @nettee's current-head blocker is still the one to drive from here: the queued internal open needs to carry the same-turn ownership check all the way through Once that handoff is fenced and the regression they asked for lands, this should be in a much clearer spot for the next pass. |
4 participants
Closes #5352.
Follow-up to #5602 and #6755 — it keeps both, and fixes the race that is left
inside the handoff between them.
Why
The turn-end auto-open pass decides from ONE
fresh: truefile-list read takenright after the daemon reports terminal status. The generated file does not
always appear in that read: the daemon's write, the chokidar event, its
coalescing window (80ms / 250ms) and the refetch behind it all settle on their
own clocks. Nothing revisits the decision afterwards, so
index.htmlneverbecomes an openable tab.
@lefarcen picked this shape in
#5352 (comment) —
re-evaluate when the post-turn file list settles, rather than making the whole
turn-end path await a global settle point. This PR is that direction: #5602's
centralised turn-end sequencing is untouched.
What it changes
auto-open-file.ts: new purereevaluateAutoOpenOnFilesSettled(). Given theturn's inputs and a settled file list it answers "open this now" / "keep
waiting" / "stop".
selectAutoOpenTurnArtifactandselectAutoOpenProducedArtifactare unchanged.ProjectView.tsx: the completion path arms that request with the turn's owninputs, and every accepted file-list generation (plus every focus change)
re-runs the decision for up to 15s.
Two things worth calling out, because they are what makes it actually work:
resolveAgentTouchedFileNamesresolves names FROM the file list, so a list that is still missing
index.htmlalso fails to resolve the write event that produced it — andselectAutoOpenTurnArtifactthen drops the file as "not touched" even afterit lands. The request therefore stores a resolver, not a computed set. This
is the
claude-protocol half of the bug;codex(no write events) suppliesan empty set and rides the pure mtime window.
turn-end pass could only settle for
plan.md, stopping there would meannever noticing
index.htmlarriving in the next list. The deadline ends thewatch; a focus move to something neither the turn nor its open request put
there also ends it, so the user is never yanked out of a tab they chose.
What users will see
In Plan mode, after "generate the deliverable", the generated
index.htmltakes focus reliably instead of intermittently leaving the user on
plan.md(first generation) or on the stale tab (regeneration).
Surface area
apps/webonly — auto-open selection + one ProjectView effect. No daemon, API,schema, or persisted-state change. Nothing is opened that the turn-end pass
would not have opened, only later and against a list that has caught up.
Verification
Real daemon + web,
--workers=1, no assertion or timeout was relaxed(
e2e/ui/real-daemon-run.test.ts,--grep "Plan mode"):85d2e489)[P1] Plan mode regeneration re-opens the existing generated HTML file[P1] Plan mode generation turn auto-opens the generated HTML file[P1] Plan mode daemon run … editable markdown planEvery "before" failure is the same shape:
expectProjectFilesToContain(page, projectId, ['index.html', 'plan.md'])passes, and then theindex.htmltab isnever found — the file exists, it just never becomes a tab. That is exactly the
handoff this patch re-runs.
Also:
apps/webunit suite 615 files / 6422 passed / 1 expected fail / 11skipped;
tsc -b --noEmitclean; new pure-function coverage inapps/web/tests/components/auto-open-file.test.ts(9 cases, 53 in the file).Two things I want to flag rather than bury
scenario). This is the second of the two failure modes you mentioned: the
index.htmltab is present butaria-selectedstaysfalse. That scenariotypes into the
plan.mdsplit editor before generating, so the active tabholds a manual-edit exit handler and
FileWorkspace.afterActiveManualEditSettles()(FileWorkspace.tsx:1575) defersthe activation behind
settleManualEdit(...), where afalseresult or asuperseding activation sequence drops it silently. That is a tab-activation
gate, not a file-list race, and re-issuing the open request only queues
behind the same gate — so the settle re-evaluation does not reach it. I did
not widen this PR to touch tab activation — happy to open a separate
issue/PR if you want it chased.
[P1] real daemon run treats an in-place artifact edit as produced workfails on my machine (
producedFiles: []vs['real-daemon-smoke.html']).It fails 3/3 with this branch AND 3/3 on
upstream/mainwith the branchstashed, so it is pre-existing here and untouched by this diff.
docs
docs/testing/e2e-coverage/status.md— the Plan-mode line now carries themeasured reliability wording instead of a bare "restored to a positive
regression" entry, with the rerun counts (
bd6a624d: 3/4 and 2/8;85d2e489: 2/4 after #6755 removed the markers) and this fix's 12/12.One note on the red-signal bar
You asked that #6708's red coverage stay in place until this goes green
without loosening assertions. By the time I started there was no marker left to
preserve: #6755 (
028bde50a, @AmyShang-alt, 2026-08-12, about 16 hours beforeI began) had already removed both markers from these two specs —
— and the same PR rewrote the
status.mdline you pointed me at, moving itfrom the "remaining gaps" section into the "restored to a positive regression"
list. The bug itself survived that change: on
85d2e489, with the markersgone, regeneration still failed 3 of 5 runs, so those two specs were sitting on
the prerelease gate as intermittently-red positive tests.
I did not add the markers back. Reversing a decision your side made 16 hours
earlier is your call, not mine, so I left the specs as #6755 has them and am
flagging it here instead. The enforceable half of your bar is intact: no
assertion and no timeout was loosened to get these green.