fix(team): contain startup rollback to exact owned panes#3265
Conversation
# Conflicts: # RELEASE_BODY.md
|
Maintainer action requested for release unblock:
No code change or failed-job rerun is needed to start CI. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ab6dcebc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const paneId = parseSplitWindowPaneId(hudResult.stdout); | ||
| if (!paneId) return null; |
There was a problem hiding this comment.
When standalone HUD restoration runs during shared-session shutdown and tmux split-window succeeds but emits extra/noisy stdout, parseSplitWindowPaneId returns null here after the pane has already been created. This path then returns without writing restored-HUD cleanup debt or killing/reconciling the newly observed pane, so shutdown can leave an untracked HUD pane in the user's tmux window and future cleanup won't know its PID; please mirror the before/after topology reconciliation used by runSourceAuthorizedSplit, or persist cleanup debt before failing.
Useful? React with 👍 / 👎.
Independent Adversarial Review — Verdict: OWNER_CONFIRMATION_REQUIREDReviewed commits: 1. Startup rollback / pane receipt bug — VERIFIED, fix is correctThe bug is real:
2. Exact-owned-pane / PID-proof + leader/HUD semantics — PRESERVEDDiff touches only 3. tmux/lterm residue E2E isolation — VERIFIED SAFEThe tmux case uses 4. v0.20.3 ancestry restoration — VERIFIED against graph and release contractTreated as hostile until proven; it holds up:
5. Local verification at current head
|
Round 2/5; blocker fingerprint: HIGH|reliability|src/team/tmux-session.ts|runSourceAuthorizedSplit post-effect reconciliation failure|failed topology read discards possible split cleanup debt. Constraint: ultra-review-loop requires one verified atomic commit for the consensus blocker Confidence: high Scope-risk: narrow Directive: Keep ambiguous split reconciliation failures as non-destructive partial cleanup debt Tested: npm run build; targeted regression; tmux-session 287/287; npm run check:no-unused; npm run lint; git diff --check Not-tested: full coverage and packed-install run after review convergence
Independent Adversarial Review v2 — Verdict: OWNER_CONFIRMATION_REQUIREDSupersedes my earlier verdict (comment 5050724193). Current exact head reviewed: Delta review:
|
Round 3/5; blocker fingerprint: HIGH|reliability|src/team/tmux-session.ts|restoreStandaloneHudPane post-split parse failure|strict parse failure leaves restored HUD untracked. Constraint: ultra-review-loop requires one verified atomic commit for the consensus blocker Confidence: high Scope-risk: narrow Directive: Preserve PID-less cleanup debt after ambiguous restored-HUD split output without authorizing a kill Tested: npm run build; targeted restored-HUD regression; tmux-session 288/288; npm run check:no-unused; npm run lint; git diff --check Not-tested: full coverage and packed-install run after review convergence
Independent Adversarial Review v3 — Verdict: OWNER_CONFIRMATION_REQUIREDSupersedes my v2 verdict (comment 5050875177). Current exact head reviewed: Delta review:
|
Final merge/release evidence — exact head
|
Round 1/5; blocker fingerprint: HIGH|reliability|src/team/__tests__/pane-teardown-residue-e2e.test.ts|Promise.all residue capture|detached residue cleanup after sibling record failure. Constraint: ultra-review-loop requires one verified atomic commit per fix group Confidence: high Scope-risk: narrow Directive: Keep future edits limited to the blocker fingerprint unless a later review expands scope Tested: npm run build; node --test dist/team/__tests__/pane-teardown-residue-e2e.test.js; npm run check:no-unused; npm run lint; git diff --check Not-tested: disposable lterm E2E (LTERM_BIN unset)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf263d0986
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pane_id: newlyObservedPaneIds[0]!, | ||
| pane_pid: null, |
There was a problem hiding this comment.
When the restored-HUD split succeeds but prints noisy output and topology reconciliation finds one new pane, this branch records only pane_pid: null and then throws before returning the pane to the caller. Fresh evidence: the new ambiguity path now writes a debt file, but reconcileRestoredHudCleanupDebtSync refuses live PID-less debt, and shutdown never gets to update/finalize config because restoreStandaloneHudPane throws, so the next shutdown retry is stuck on restored_hud_cleanup_debt_unresolved until the user manually removes the pane. Once a single new pane is identified, prove and persist its PID (or let reconciliation resolve it) before failing/returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e435fb4a. The single-new-pane ambiguity path still writes the PID-less debt first for crash safety, then immediately obtains an exact live pane proof and overwrites the debt with the frozen PID before returning the fail-closed ambiguity error. The next shutdown retry can therefore reconcile only the pinned pane through the existing leader-owner and HUD-command identity checks instead of getting stuck on live PID-less debt.
Regression coverage now reproduces noisy split output, verifies pane_pid: 2000000044, and proves replay removes %44 and clears the debt. Fresh validation: lint, no-unused, build, restored/standalone HUD tests 19/19, and private real-tmux + disposable-lterm E2E 2/2.
Final owner handoff — exact head
|
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
OWNER_CONFIRMATION_REQUIRED
Signed receipt: gjc-terminal-verdict/pr-3265/bf263d098695414f5c897fb0c95058e3292d8067 (SSH-signed annotated tag, target bf263d098695414f5c897fb0c95058e3292d8067).
Exact-head review against current dev (5ac2e59868b8a32ea93dc827edc8483c611beefe): 4 files, +842/-19. No concrete source blocker found. The split receipt parser now accepts only one canonical pane-id receipt; ambiguous output preserves unproven cleanup debt and does not introduce direct process signalling.
Topology verified: b739e69e8dfeb01ce23815fd24a88889f66ed03b is the no-tree-change two-parent release-lineage merge, and later a7adaabf5162ee7d42e7ed0a22d4294edc86df0f merges current dev. A future merge must be merge-commit only; never squash or rebase.
Fresh exact-head validation passed: build, lint, no-unused, tmux-session 288/288, and private real-tmux E2E. The lterm subtest is an explicit local test gap here because LTERM_BIN is unavailable. Contributor final evidence claims the paired private lterm run and release packaging checks; that claim was recorded but not independently rerun.
Fork CI run 29969028751 began action_required; approved via REST for this exact SHA and currently remains in progress.
Maintainer judgment is required for accepting the release-lineage change and staged OMX-first/lterm release contract. No source, contributor branch, PR state, release, or merge was mutated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e435fb4a49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (error.newlyObservedPaneIds.length === 1) { | ||
| partialWorkerPaneIdsByIndex[i - 1] = error.newlyObservedPaneIds[0]!; | ||
| } |
There was a problem hiding this comment.
When split-window succeeds but prints noisy output and topology reconciliation finds exactly one new worker pane, this catch path records that pane with panePid = null and then rethrows. The startup error path preserves this partial session, applyCreatedInteractiveSessionToConfig will persist the pane_id without a pid, and later teardownWorkerPanes refuses to kill any live pane without a positive expected PID, so this failure mode leaves the newly-created worker pane as durable cleanup debt that cannot be retried automatically. Please prove and store the reconciled pane's PID before preserving the partial session, as the normal split path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed and retained in exact head 5628ec1e. A uniquely reconciled ambiguous worker pane is now exact-proven before partial state is finalized; its frozen PID is stored in workerPanePidsByIndex, owner-tagged, and used by immediate/later teardown. The maintainer-added recycled-pane regression is integrated: a same-ID/different-PID pane is never killed.
|
REQUEST_CHANGES Exact PR head reviewed through REST: Blocking finding
That state is not actionable cleanup debt: the outer startup cleanup deliberately skips rollback entries whose PID is null ( Before preserving the partial session, exact-prove the sole reconciled worker pane and persist its PID, analogous to the restored-HUD reconciliation added at this head. PID-less debt should remain only when exact proof is unavailable; the regression should assert that a uniquely reconciled worker PID is pinned and can be replayed for authorized teardown rather than codifying Verification
Review-only lane: no source edits, commits, branches, or pushes were made. — |
|
Closing this external PR after the fresh exact-head adversarial review found a blocking PID-less worker cleanup debt. The signed REQUEST_CHANGES verdict above contains the concrete repair contract and verification evidence. — |
|
Correction: this PR has been reopened. The independent review lane was authorized to post — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abbac3f17a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function runSourceAuthorizedSplit(source: SourcePaneAuthority, effect: string): string { | ||
| const windowTarget = `${source.sessionName}:${source.windowIndex}`; |
There was a problem hiding this comment.
When the ambiguous-output path is taken, this reconciliation target is session:index; if a user or hook renumbers or moves tmux windows between the pre-split snapshot and the post-split snapshot, list-panes can read a different window even though the split itself was authorized against the captured source.windowId. Tmux documents mysession:1 as an index target and @N window IDs as unchanged for the window lifetime (https://man.openbsd.org/tmux.1), so use the captured window ID for both topology reads to avoid treating unrelated panes as rollback debt and possibly killing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in exact head 5628ec1e. Both before/after topology reads now use the captured immutable source.windowId; the maintainer commit d61ae7e2 and its window/PID-reuse regressions were merged without rebasing.
| for (const candidatePaneId of error.newlyObservedPaneIds) { | ||
| rollbackPanes.set(candidatePaneId, null); | ||
| partialWorkerPaneIds.push(candidatePaneId); | ||
| } | ||
| if (error.newlyObservedPaneIds.length === 1) { |
There was a problem hiding this comment.
When reconciliation reports more than one new pane, this branch appends the candidates only to partialWorkerPaneIds, but the partial session always includes a workerPaneIdsByIndex array; applyCreatedInteractiveSessionToConfig returns after processing that by-index array, so with all slots still null none of these pane IDs are saved in the preserved startup config. A noisy split racing with another pane creation therefore leaves live PID-less panes that are neither killed by rollback nor available to a shutdown retry; persist an unresolved debt list or avoid supplying an empty by-index array for this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in exact head 5628ec1e. Multi-candidate ambiguous splits are now filtered by a per-split random receipt embedded in pane_start_command, exact-proven and Team-owner-tagged, then persisted in the dedicated startup_cleanup_panes debt list with frozen PIDs. shutdownTeam replays that list only after exact PID + Team-owner authorization and clears it transactionally after teardown.
Repair update — REQUEST_CHANGES blocker (issuecomment-5053684796) addressedExact head: Fix mechanismIn the worker
Regression coverage
Deferral disclosureThe same-session HUD ambiguous catch has the same null-PID debt shape. Per the narrow scope of this blocker (worker path only), HUD parity is deferred as owner-accepted PID-less cleanup debt for a follow-up; no HUD-path behavior changed in this repair. VerificationFork CI run 29976454605 approved at head Repair-Update-By: gjc (maintainer fix) |
… pinned-PID reuse - runSourceAuthorizedSplit reconciles against the immutable window ID so a concurrent renumber cannot turn an unrelated pane into kill authority - recycled pane-ID regression: pinned ambiguous worker PID never authorizes killing a recycled %2 during rollback or later teardown - explicit unavailable/gone/fully-cleaned coverage for the uniquely reconciled ambiguous worker pane - startTeam regression preserving startup_rollback_pane_proof_unavailable for an ambiguous worker split with unavailable reconciled proof
Repair update v2 — new exact head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51685442e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const newlyObservedPaneIds = afterTopology.error | ||
| ? [] | ||
| : afterTopology.panes | ||
| .map((pane) => pane.paneId) | ||
| .filter((candidate) => !beforePaneIds.has(candidate)); |
There was a problem hiding this comment.
When the guarded if-shell predicate is false (for example because the source pane PID or owner changed), tmux runs the else branch display-message -p '', so parseSplitWindowPaneId also lands here even though our split never ran. If a user or hook creates one pane in this window between the before/after reads, this code reports it as newlyObservedPaneIds; the worker catch then pins its PID and rollback calls killExactPaneSync without an owner tag, so startup can delete an unrelated pane. Please make the source-failure branch distinguishable from ambiguous successful split output before reconciling/rolling back candidates.
Useful? React with 👍 / 👎.
| if (error.newlyObservedPaneIds.length === 1) { | ||
| partialHudPaneId = error.newlyObservedPaneIds[0]!; | ||
| partialHudPanePid = null; |
There was a problem hiding this comment.
When the team HUD split succeeds but its output is ambiguous and reconciliation finds exactly one new HUD pane, this preserves hudPaneId with hudPanePid left null. startTeam writes that partial config, but later shared-session shutdown rejects a persisted HUD with a missing PID before it can call teardownWorkerPanes, so the retry state is not actually cleanable. Mirror the worker/restored-HUD path by proving and storing the reconciled HUD pane PID, or drop it only if it is proven gone.
Useful? React with 👍 / 👎.
…0.20.4' into fix/safe-shared-startup-rollback-v0.20.4 # Conflicts: # src/team/tmux-session.ts
Repair update v3 — exact head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5628ec1e74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| }); | ||
| assertPaneTeardownProofsAvailable('startup_cleanup', teardown.proofUnavailable); | ||
| for (const paneId of teardown.killedPaneIds) resolvedPaneIds.add(paneId); |
There was a problem hiding this comment.
When a startup_cleanup_panes entry is live during the initial proof but exits before teardownWorkerPanes can kill it, the teardown reports that pane via provenGonePaneIds, not killedPaneIds. Since only killed panes are added to resolvedPaneIds here, the already-gone debt is written back into the config and reconcileStartupCleanupPanes throws startup_cleanup_pane_teardown_failed, blocking shutdown/retry even though there is nothing left to clean up. Include teardown.provenGonePaneIds in the resolved set as well.
Useful? React with 👍 / 👎.
CI green on exact head
|
CI green on exact head
|
Independent Adversarial Review — Verdict: MERGE_READYSupersedes the REQUEST_CHANGES verdict (issuecomment-5053684796) and all prior rounds. Exact head reviewed: Prior blocker — REPAIRED and verifiedThe blocking finding (PID-less reconciled worker debt with no authorized retry path) is closed: the ambiguous-split catch now exact-proves each receipt-matched candidate via Red-team results on the full delta
Verification executed on this exact head (local, this lane)
Merge-method constraint (unchanged, still binding)Create a merge commit only. Do not squash or rebase. Squash/rebase would sever the intentionally restored — |
2 participants
Summary
split-windowpane receipts, rejecting multi-row/noisy output instead of trusting a first lineMerge requirement
This PR head contains the real
v0.20.3release lineage. Merge commit only; squashing/rebasing would lose the restored ancestry. After merge, verify:Verification
npm run buildnpm run check:no-unusednpm run linttmux-session.test.js: 287/287coverage:team-criticaland packed-install smoke launched locally; GitHub CI is authoritative for platform matrixNo live/default tmux or lterm socket is used by the new E2E.