fix(team): make PowerShell psmux pane creation safe#3145
Conversation
|
Verification receipt for exact head
Evidence is Linux mocked-contract coverage only; real Windows/psmux runtime was not run. This PR remains open and unmerged for a separate exact-head adversarial review. — |
Exact-head adversarial reviewVerdict: REQUEST_CHANGES Blocking finding — pane identity is lexical in OMX but numeric/fallback in psmux
psmux v3.3.4 parses
This contradicts the PR's stated fail-closed contract that malformed or ambiguous split IDs never gain destructive authority. The existing tests cover Required correction: accept only pane IDs that are canonical and round-trippable under the actual psmux target grammar, reject leading-zero/overflow spellings before recording rollback authority, and add adversarial tests proving alias/overflow inputs cannot reach tag/resize/select/kill sinks. A verified pre/post global pane-set difference plus immediate sink revalidation would be safer than trusting the split string alone. Additional observationA leader-owned pre-existing HUD is removed before worker/replacement-HUD success ( Evidence
Review only; no source changes, review-state mutation, merge, auto-merge, push, or ref update were performed. Signed: GJC exact-head adversarial review for PR #3145; reviewer account — |
8727999 to
5db7cd3
Compare
Repair evidence after exact-head
|
Fresh exact-head adversarial reviewVerdict: REQUEST_CHANGES The earlier review was stale after the force-update. This review is bound to repaired head Blocking finding — fresh HUD snapshot bypasses canonical validation before
|
5db7cd3 to
6a5a61d
Compare
![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: 6a5a61dab4
ℹ️ 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 command = `${pane.startCommand} ${pane.currentCommand}`; | ||
| const owner = readHudPaneOwner(pane); | ||
| return parseShellEnvAssignment(command, OMX_TMUX_HUD_OWNER_ENV) === '1' | ||
| return parsePowerShellHudEnvPrefix(command).present |
There was a problem hiding this comment.
When a HUD watch command contains an unrelated or malformed PowerShell env assignment but no OMX HUD ownership (for example $env:PATH = '...'; & node omx.js hud --watch), this marks the pane as having owner metadata solely because $env: is present. shouldReapDeletedCwdHudPane then treats a deleted-cwd pane as owner-tagged, while readHudPaneOwner returns no leader/session, so the !owner.leaderPaneId path can kill a live, unowned HUD. Please require a valid OMX owner marker/session/leader assignment before treating the PowerShell prefix as ownership metadata.
Useful? React with 👍 / 👎.
Exact-head adversarial rereviewTerminal verdict: REQUEST_CHANGES This review is bound to PR head Prior blocker status — fixed on the reviewed pathThe exact two-snapshot HUD cleanup path from That repair is real, but canonical pane identity is still not enforced at other fresh/persisted authority boundaries. Blocking finding 1 — persisted and scale-operation pane IDs can still alias the leader or overflow into psmux fallbackThe new canonical parser is local to startup/restore (
A read-only external exact-head harness called the public teardown primitive with leader Under the already-established psmux v3.3.4 target grammar, Required correction: make the canonical round-trip parser a shared pane-target boundary and apply it before every persisted/fresh pane ID can reach display/probe, set-option/tag, resize, select, kill, rollback, or split-target use. Compare leader/HUD exclusions only after canonical validation. Invalid or ambiguous persisted rows must fail closed, not be skipped into a fallback target. Blocking finding 2 — unrelated PowerShell env prefixes are treated as HUD ownership and can authorize reapingThe exact-head Codex automated review reported a valid P2 that is independently reproducible:
An exact-head harness using this unowned command: with a deleted cwd caused Required correction: unrelated PowerShell assignments must not count as OMX HUD owner metadata. Require an unambiguous recognized OMX owner marker/session/leader key before ownership-sensitive cleanup, and add a deleted-cwd regression proving unrelated or malformed PowerShell prefixes are preserved. Blocking finding 3 — shared HUD helpers still accept the same aliases and overflow spellingsThe repaired Team callsite is not the only HUD pane-authority path. Shared HUD helpers still use weaker lexical checks:
Consequently, an owner-matching Required correction: all HUD enumeration, split-output, display-context, cleanup, resize, and hook helpers must use the same canonical round-trip parser before returning or consuming pane authority. Additional fail-closed gaps
These cases need regressions covering POSIX duplicate owner keys, duplicate local pane rows, and pre-existing split results on non-native platforms. Adversarial coverage and gates
Evidence boundaryNo real Windows + PowerShell + psmux lifecycle was available. The alias/overflow blockers are nevertheless demonstrated at command boundaries and use the same psmux semantics established in the prior review. The ownership/reaping blocker is platform-independent helper behavior reproduced against the exact compiled head; the remaining gaps are exact-source authority traces confirmed by an independent architecture audit. Review only; no product-source edits, commit, push, merge, auto-merge, ref update, or approval was performed. Signed: GJC exact-head adversarial rereview for PR #3145; reviewer account |
Final exact-head red-team merge reviewTerminal verdict: REQUEST_CHANGES Reviewed exact PR head Blocker 1 — non-canonical persisted pane IDs retain destructive authorityThe new
Thus Blocker 2 — unrelated PowerShell env assignments falsely mark a HUD as OMX-owned
Blocker 3 — shared HUD pane helpers still use lexical
|
6a5a61d to
6056ef6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6056ef6e41
ℹ️ 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".
| if (!v.permissions_snapshot || typeof v.permissions_snapshot !== 'object') return false; | ||
| if (v.schema_version !== 2 || !isNonBlankString(v.name) || !TEAM_NAME_SAFE_PATTERN.test(v.name)) return false; | ||
| if (!isNonBlankString(v.task) || !isNonBlankString(v.tmux_session) || !isNonBlankString(v.created_at) || !Number.isFinite(Date.parse(v.created_at))) return false; | ||
| if (!isNonBlankString(v.agent_type) || !isSafePositiveInteger(v.max_workers) || v.max_workers > ABSOLUTE_MAX_WORKERS) return false; |
There was a problem hiding this comment.
When upgrading a team created by prior releases, manifest.json does not contain agent_type or max_workers because the previous manifest writer only persisted those in config.json. This new validation rejects that manifest before readTeamStateOutcome can merge with the still-valid config or run any migration, so readTeamConfig returns null and existing team commands behave as if the team is missing. Please default these fields from the config or keep the absence legacy-compatible.
Useful? React with 👍 / 👎.
|
|
||
| function buildHookIdentityRegistrationSuffix(hookTarget: string, hookSlot: string, hookName: string): string[] { | ||
| return [ | ||
| '\\;', 'set-option', '-t', hookTarget, |
There was a problem hiding this comment.
These args are passed directly to tmux via spawn/execFile, not through sh; the tmux man page says command separators are individual ; tokens and that \; will not mark a command sequence (https://man7.org/linux/man-pages/man1/tmux.1.html). With '\\;' here, the set-option identity registration is not executed as a separate command at hook registration time, so the guarded unregister/self-unregister checks see no identity option and leave resize hooks behind after team/HUD teardown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5228ea412e
ℹ️ 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 owner = readPaneTeamOwnerTagResult(paneId); | ||
| if (owner.status === 'value' && owner.value === authority.expectedOwnerId) return true; | ||
| return authority.verifyOwnership?.(paneId) ?? false; |
There was a problem hiding this comment.
When shutting down a running team created before @omx_team_pane_owner_id was written, readPaneTeamOwnerTagResult returns missing. The old runtime bounded that case to persisted worker pane IDs; this new authority check returns false unless the caller supplies verifyOwnership, and the shutdown paths that pass persisted/listed worker panes do not provide such a fallback for detached sessions while shared sessions filter missing-owner panes out earlier. Those upgraded teams now either fail with shutdown_worker_teardown_failed or skip killing live persisted workers while state cleanup continues, so please keep the persisted-pane legacy fallback until those panes are migrated.
Useful? React with 👍 / 👎.
| const match = /^(%0|%[1-9][0-9]*) 0 ([1-9][0-9]*)$/.exec(line); | ||
| const paneId = match?.[1] ? parseCanonicalTmuxPaneId(match[1]) : null; | ||
| if (!paneId || !match?.[2] || incarnations.has(paneId)) return null; |
There was a problem hiding this comment.
When a tmux server has a remain-on-exit pane, list-panes -a still lists it (tmux documents -a as listing all panes, and exited panes can remain open via remain-on-exit: https://man7.org/linux/man-pages/man1/tmux.1.html). This regex only accepts rows where pane_dead is 0, so one dead pane anywhere makes readStrictTmuxPaneIncarnations() return null; inside-tmux launches then take the direct fallback at the authority check and lose HUD setup/cleanup even though the live pane IDs are still usable. Please skip pane_dead=1 rows instead of invalidating the whole snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8844f2ba62
ℹ️ 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".
| } | ||
|
|
||
| function tmuxCommandQuote(value: string): string { | ||
| return `'${value.replace(/'/g, "\\'")}'`; |
There was a problem hiding this comment.
When a hook plugin calls tmux.sendKeys with text containing a single quote, this helper builds an if-shell then-command parsed by tmux, not by a POSIX shell, and tmux's parsing rules only apply backslash replacements outside quotes or inside double quotes. Escaping ' as \' inside single quotes therefore terminates/mangles the argument, so payloads like don't fail or inject the wrong text; use tmux-safe quoting for single quotes or avoid reparsing the payload in a command string.
Useful? React with 👍 / 👎.
| if (lines.length === 0) return null; | ||
| const panePids = new Map<string, string>(); | ||
| for (const line of lines) { | ||
| const match = /^(%\S+) 0 ([1-9]\d*)$/.exec(line); |
There was a problem hiding this comment.
In tmux servers with remain-on-exit panes, list-panes -a can include dead panes (pane_dead is the documented flag for this), and this regex rejects the entire snapshot unless every row has pane_dead equal to 0. A single unrelated dead pane makes readStrictHudPanePids() return null, causing omx hud --tmux to refuse to reuse/resize an existing live HUD with “Failed to establish exact tmux pane authority”; skip dead rows instead of invalidating the whole PID snapshot.
Useful? React with 👍 / 👎.
Hostile exact-head terminal reviewTerminal verdict: REQUEST_CHANGES Reviewed exact pushed head New Codex adversarial inputs
Remaining blockers
These are high-severity destructive TOCTOU gaps. This head is not merge-ready. Verification evidence
The prior psmux Signed: GJC hostile exact-head terminal review for PR #3145; exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa3dd99c9
ℹ️ 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".
|
|
||
| function writeHudSplitOperationMarkedCommand(command: string, marker: string): string { | ||
| if (process.platform === 'win32') return `$env:${OMX_TMUX_SPLIT_OPERATION_MARKER_ENV} = '${marker}'; ${command}`; | ||
| return `${OMX_TMUX_SPLIT_OPERATION_MARKER_ENV}='${marker}'; export ${OMX_TMUX_SPLIT_OPERATION_MARKER_ENV}; ${command}`; |
There was a problem hiding this comment.
On POSIX this prefixes the HUD command with OMX_TMUX_SPLIT_OPERATION_MARKER='...'; export ...;, and tmux persists that full string in pane_start_command. When the wrapped command is the generated watcher with OMX_TMUX_HUD_OWNER=1/OMX_TMUX_HUD_LEADER_PANE=..., parsePosixHudEnvAssignments rejects the semicolon before it reaches those owner assignments, so readHudPaneOwner treats freshly created omx hud --tmux/team HUD panes as ambiguous and unowned; prompt-submit reconciliation and duplicate cleanup then fail to find the existing HUD and can create another one. The duplicated POSIX marker wrapper in src/team/tmux-session.ts has the same effect.
Useful? React with 👍 / 👎.
|
|
||
| function buildHudHookRegistrationSuffix(context: HudResizeHookContext, hookSlot: string): string[] { | ||
| return [ | ||
| '\\;', 'set-option', '-t', context.sessionId, |
There was a problem hiding this comment.
These arguments are passed directly to tmux via execFileSync, not through a shell; the tmux manual describes command separators as individual ; tokens, with \; only needed for a shell to produce that argv token (https://man7.org/linux/man-pages/man1/tmux.1.html). Passing '\\;' here makes tmux see a literal semicolon, so the following set-option identity registration is not executed; guarded unregister/self-unregister checks then never see the identity option and leave the HUD resize/layout hooks installed.
Useful? React with 👍 / 👎.
| fields.length !== 5 | ||
| || parseCanonicalTmuxPaneId(fields[0]) !== fields[0] | ||
| || (fields[1] !== '0' && fields[1] !== '1') | ||
| || !isStrictPanePid(fields[2]) |
There was a problem hiding this comment.
When the target session contains a remain-on-exit pane, tmux can still list a row with pane_dead=1 and a non-live PID value such as 0; this positive-PID check runs before dead rows are skipped, so one unrelated dead pane makes parseSessionPaneRows return null and tmux.sendKeys({ sessionName }) reports target_missing even though a live Codex pane is present. Parse dead rows enough to ignore them instead of invalidating the whole session snapshot.
Useful? React with 👍 / 👎.
dfa3dd9 to
65e9574
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65e9574e22
ℹ️ 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".
| let leaderPanePid: string | null = null; | ||
| let hudPanePid: string | null = null; | ||
| for (const line of lines) { | ||
| const match = /^(%\S+) ([01]) ([1-9][0-9]*)$/.exec(line); |
There was a problem hiding this comment.
Fresh evidence: this new global incarnation scan still requires every list-panes -a row to have a positive PID before it reaches the pane_dead skip. tmux keeps remain-on-exit panes listed after exit (tmux(1): https://man7.org/linux/man-pages/man1/tmux.1.html), and dead rows can report #{pane_pid} as 0; one unrelated dead pane therefore makes readHudResizeHookPaneIncarnations() return null, so registerHudResizeHook() refuses to install resize/layout hooks for an otherwise live HUD. Accept dead rows first and only require a positive PID for live panes.
Useful? React with 👍 / 👎.
| const seen = new Set<string>(); | ||
| let incarnation: { paneDead: boolean; panePid: string } | null = null; | ||
| for (const line of lines) { | ||
| const match = /^(%0|%[1-9][0-9]*) ([01]) ([1-9][0-9]*)$/.exec(line); |
There was a problem hiding this comment.
Fresh evidence: the new team-wide incarnation reader has the same positive-PID requirement before it can skip pane_dead=1. In a tmux server with any remain-on-exit pane reporting a row like %9 1 0, readPaneIncarnation() returns null for every target; splitAndAdoptPane() can then fail after creating a worker/HUD pane, and later liveness/shutdown authority checks also fail. Parse dead rows enough to ignore them before validating live pane PIDs.
Useful? React with 👍 / 👎.
65e9574 to
4d2d18d
Compare
Hostile exact-head terminal reviewTerminal verdict: REQUEST_CHANGES Reviewed exact pushed head Verified repairs and gatesSDK hostile payload bytes use argv/file buffer transport and multiline paste is gated by Remaining blockers from fresh independent Architect + QA reviews
Green CI does not override these source-proven authority gaps. PR #3145 is not merge-ready and was not merged. Issue #3123 remains open. Signed: GJC hostile exact-head terminal review for PR #3145; exact head |
Fresh exact-head hostile terminal reviewSigned terminal verdict: REQUEST_CHANGES Reviewed exact PR head Blocking Architect findings
Blocking QA findings
Both fresh independent reviews are bound to the exact head/base above and returned Signed-by: GJC exact-head delivery authority — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19afca39ad
ℹ️ 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".
| } | ||
|
|
||
| export async function readTeamManifestV2(teamName: string, cwd: string): Promise<TeamManifestV2 | null> { | ||
| const outcome = await readTeamManifestV2Outcome(teamName, cwd); |
There was a problem hiding this comment.
For teams created by the previous release, manifest.v2.json can lack the newly required agent_type and max_workers fields. readTeamStateOutcome() supplies compatibility defaults from config.json, but this public reader still calls the validator without those defaults, so direct callers such as assignTask, monitorTeam, and mailbox send paths see null and silently fall back to default policy/governance; a legacy team with plan_approval_required or delegation_only in its manifest can therefore have those guards bypassed. Please make this reader apply the same config-backed defaults or avoid using it for policy-sensitive reads.
Useful? React with 👍 / 👎.
| const [paneId, paneDead, panePid, sessionName, ownerId] = fields; | ||
| if (!paneId || parseCanonicalTmuxPaneId(paneId) !== paneId || (paneDead !== '0' && paneDead !== '1') || !/^[1-9][0-9]*$/.test(panePid) || !Number.isSafeInteger(Number(panePid)) || sessionName === '' || ownerId === '' || rows.has(paneId)) return null; |
There was a problem hiding this comment.
When the tmux server contains any remain-on-exit pane, list-panes -a still lists it (tmux(1) documents -a as listing all panes), and dead rows commonly have #{pane_pid} as 0. This parser rejects the entire authority snapshot before it can skip/ignore that dead row, so one unrelated dead pane makes every Team notify-hook dispatch fail with persisted_worker_authority_invalid/persisted_leader_authority_invalid even when the target pane is live. Please accept pane_dead=1 rows enough to skip them, and only require a positive PID for live panes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71650d638
ℹ️ 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".
| function quoteTmuxCommandArgument(value: string): string { | ||
| return JSON.stringify(value); |
There was a problem hiding this comment.
When a worker trigger contains $ (for example a custom team-state path like /tmp/$USER/omx or any direct send text with shell variables), this helper returns a double-quoted tmux command argument such as "Read /tmp/$USER/...". tmux parses the if-shell command argument itself, and its parsing rules expand variables preceded by $ even inside double quotes (tmux(1) parsing syntax: https://man7.org/linux/man-pages/man1/tmux.1.html), so send-keys -l sends the environment-expanded text rather than the exact trigger. Please use tmux-safe quoting that escapes $, or avoid reparsing the literal payload in a tmux command string.
Useful? React with 👍 / 👎.
| const unregister = sessionName && hookSlot | ||
| ? `${tmux} if-shell -F -t ${sessionName} ${quote(`#{==:${identityOption},${identityToken}}`)} ${quote(`${tmux} set-hook -u -t ${sessionName} ${hookSlot} \\; ${tmux} set-option -u -t ${sessionName} ${identityOption}`)} ${quote('')}` | ||
| : ''; | ||
| const prune = `${tmux} if-shell -F -t ${canonicalLeaderPaneId} ${quote(buildTmuxPaneIncarnationCondition(canonicalLeaderPaneId, exactLeaderPanePid))} ${quote(`${tmux} clear-history -t ${canonicalLeaderPaneId}`)} ${quote(unregister)}`; |
There was a problem hiding this comment.
When the detached history-prune hook fires, this builds a shell command that invokes tmux if-shell ... 'tmux clear-history -t %x'; however if-shell's success/failure arguments are tmux commands parsed by tmux, not shell commands (tmux(1): if-shell ... shell-command command [command]). The nested command name tmux is therefore invalid, so detached sessions do not clear history and the unregister path has the same problem. Please pass clear-history .../set-hook ... as tmux commands or wrap external tmux ... invocations in run-shell.
Useful? React with 👍 / 👎.
Exact-head terminal review — c71650dSigned terminal verdict: REQUEST_CHANGES Exact head: Fresh independent Architect and QA reviews both returned Blocking findings
The PR remains open. The retained corrective lane is repairing these exact findings; no merge will occur until a new exact head has fully green hosted CI and fresh dual hostile Signed-by: GJC exact-head delivery authority — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce1d46393f
ℹ️ 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".
| ? `#{==:#{${OMX_TEAM_PANE_OWNER_OPTION}},${expectedOwnerId}}` | ||
| : `#{==:#{${OMX_TEAM_PANE_OWNER_OPTION}},}`; | ||
| const sessionIdCondition = expectedSessionId ? `#{==:#{session_id},${expectedSessionId}}` : '1'; | ||
| const condition = `#{&&:${buildTeamPaneIncarnationCondition(paneId, expectedPanePid)},#{&&:#{==:#{session_name},${sessionName}},#{&&:${sessionIdCondition},${ownerCondition}}}}`; |
There was a problem hiding this comment.
createTeamSession persists shared interactive teams as <session>:<window>, and shutdown passes that same config.tmux_session into this authority. In tmux, #{session_name} expands only the base session, so for a normal shared team like dev:0 this condition is always false even when the pane is still the right one; if-shell then skips kill-pane, causing shutdown/scale-down to report teardown failure and leave worker panes behind. Compare against the same target shape, or split the base session/window before gating the kill.
Useful? React with 👍 / 👎.
| const fields = line.split('\t'); | ||
| if (fields.length !== 5) return null; | ||
| const [paneId, paneDead, panePid, sessionName, ownerId] = fields; | ||
| if (!paneId || parseCanonicalTmuxPaneId(paneId) !== paneId || (paneDead !== '0' && paneDead !== '1') || !/^[1-9][0-9]*$/.test(panePid) || !Number.isSafeInteger(Number(panePid)) || sessionName === '' || ownerId === '' || rows.has(paneId)) return null; |
There was a problem hiding this comment.
This snapshot is from list-panes -a, so it includes unrelated live panes that legitimately have no @omx_team_pane_owner_id. With even one such row, ownerId === '' makes parseStrictTeamPaneSnapshot() return null for the whole batch, so provePersistedPaneAuthority() rejects the actual worker/leader and the notify hook reports persisted_*_authority_invalid without sending. Parse ownerless rows and only require the owner on the target row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887c5613a5
ℹ️ 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".
| || pane.paneDead !== false | ||
| || !/^[1-9][0-9]*$/.test(pane.panePid ?? '') |
There was a problem hiding this comment.
When the current window contains an unrelated remain-on-exit pane, tmux still lists that pane in list-panes for the window (tmux(1): https://man7.org/linux/man-pages/man1/tmux.1.html), but this snapshot validator rejects the entire window unless every row has paneDead === false and a live PID. That makes prompt-submit HUD reconciliation return failed before it can reuse, resize, dedupe, or create the HUD even though the current leader pane is still live; please ignore paneDead rows instead of invalidating the whole snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650a4ecae0
ℹ️ 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".
|
|
||
| let paneCurrentCommand = ''; | ||
| let paneRunningShell = false; | ||
| const buildReadinessResult = (ok: boolean, reason: string, paneCapture: string, readinessEvidence: string) => ({ |
There was a problem hiding this comment.
When auto-nudge or generic tmux injection reaches a ready pane, callers still read paneGuard.exactPaneProof?.pid (auto-nudge.ts and tmux-injection.ts) and skip with pane_readiness_unverified if it is missing. This result builder no longer includes exactPaneProof, so those paths never get a PID and return before sending any nudge/injection. Please keep returning the captured proof/PID or update those callers to use the new authority API.
Useful? React with 👍 / 👎.
| if (!rawRows || rawRows.includes('\r') || !rawRows.endsWith('\n') || rawRows.endsWith('\n\n')) return null; | ||
| const rows = rawRows.slice(0, -1).split('\n').map((line) => line.split('\t')); | ||
| const seenPaneIds = new Set(); | ||
| if (rows.some((row) => row.length !== 5 || parseCanonicalTmuxPaneId(row[0]) !== row[0] || (row[1] !== '0' && row[1] !== '1') || !/^[1-9][0-9]*$/.test(row[2]) || !row[3] || !row[4] || seenPaneIds.has(row[0]) || !seenPaneIds.add(row[0]))) return null; |
There was a problem hiding this comment.
When the tmux server has any unrelated pane, this list-panes -a snapshot rejects the whole batch because every row must have a positive PID and non-empty @omx_team_pane_owner_id; tmux documents -a as listing all panes on the server (https://man7.org/linux/man-pages/man1/tmux.1.html). A normal unowned shell pane or dead remain-on-exit pane therefore makes readLeaderPaneAuthority() return null and maybeNudgeTeamLeader suppresses a valid leader with leader_authority_invalid. Validate owner/session/PID only on the selected leader row and skip irrelevant dead/ownerless rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57f248144d
ℹ️ 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".
| if (!managedPayload) { | ||
| return { | ||
| paneId: anchorPaneId, | ||
| state: currentState, |
There was a problem hiding this comment.
When the fallback watcher cannot build managed-session evidence (for example after session.json is stale or missing), this branch returns the persisted tmux_pane_id without rechecking the stored pane PID/session/owner that requireFrozenRalphPaneBinding used to enforce. The later readiness guard only inspects the target pane state, and emitRalphContinueSteer sends keys directly, so a reused %pane can receive the Ralph continue text. Please either fail closed here or keep the exact PID/session/owner proof before returning anchorPaneId.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed replacement terminal verdict — exact head
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
REQUEST_CHANGES Exact revision evidence (freshly fetched 2026-07-17):
This is a fail-closed final review verdict. Hosted exact-head CI being green does not resolve the repository-local review blockers. Blocking Architect receipt:
Blocking Critic receipt:
Required before merge readiness can be established:
No product files were edited, and no merge, push, release, or artifact publication was performed. — |
Signed frozen-diff terminal dispositionREQUEST_CHANGES / BLOCK — commit, push, and merge are not approved Remote PR head remains The proposed repair was reviewed as an immutable eight-file patch:
Terminal review results
Because one reviewer explicitly blocked and the other review artifact is schema-invalid/truncated, the required dual schema-valid zero-blocker approval does not exist. No source mutation followed these verdicts; no commit, push, merge, release, publish, or tag was performed. Validated blocked receipt
Signed: GJC exact-head delivery authority; reviewer session |
|
NOT_MERGE_READY — approved neutral-candidate execution stopped at the focused deterministic gate. Evidence:
Disposition: retain the neutral candidate and protected frozen evidence for a corrective follow-up; all exact-head delivery authority is denied until focused tests, broad gates, schema-valid reviews, and hosted CI are clean on one immutable candidate. |
|
NOT_MERGE_READY — corrective candidate reached green issue-focused and static gates, but two required terminal gates remain blocked outside the approved issue scope. Candidate evidence:
Green evidence:
Blocking terminal evidence:
Because the required broad and packed gates are not terminal green, no candidate commit was created, no refspec push occurred, no identical-diff MERGE_READY reviews were solicited, and no merge was attempted. The internal PR and Ultragoal session remain open. |
|
REQUEST_CHANGES Adversarial recovery review was re-bound to exact remote PR head Blocking current-state evidence
No commit, push, review approval, auto-merge, merge-queue, merge, release, branch deletion, or history rewrite was performed. The dirty worktree is preserved for corrective continuation. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4e3106c94
ℹ️ 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".
| } | ||
|
|
||
| function unwrapKnownPowerShellCommand(command: string): { command: string; remainder: string } | null { | ||
| const match = /^\s*powershell(?:\.exe)?\s+-NoLogo\s+-NoExit\s+-Command\s+'((?:''|[^'])*)'(.*)$/i.exec(command); |
There was a problem hiding this comment.
For native-Windows standalone HUDs, createHudWatchPane prepends $env:OMX_TMUX_SPLIT_OPERATION_MARKER = ...; to the powershell.exe -NoExit -Command '...' string generated by buildHudStartupCommand. This regex only unwraps when powershell.exe is first, so those owner assignments remain inside the outer quote; readHudPaneOwner then treats the pane as ambiguous/unowned, and the inside-tmux launch path cannot register/reuse/dedupe the HUD it just created. Please strip the split-marker prefix before unwrapping or support the marker-prefixed envelope.
Useful? React with 👍 / 👎.
| const posixCommand = `exec[[:space:]]+env[[:space:]]+OMX_SESSION_ID='${expectedOwner.sessionId}'[[:space:]]+${OMX_TMUX_HUD_OWNER_ENV}=1[[:space:]]+${OMX_TMUX_HUD_LEADER_PANE_ENV}='${canonicalLeaderPaneId}'[[:space:]]+${posixRootAssignment}(node|${posixQuoted})[[:space:]]+${posixQuoted}[[:space:]]+hud[[:space:]]+--watch([[:space:]]+'--preset=(minimal|focused|full)')?$`; | ||
| const powerShellCommand = `\\$env:OMX_SESSION_ID[[:space:]]*=[[:space:]]*'${expectedOwner.sessionId}';[[:space:]]+\\$env:${OMX_TMUX_HUD_OWNER_ENV}[[:space:]]*=[[:space:]]*'1';[[:space:]]+\\$env:${OMX_TMUX_HUD_LEADER_PANE_ENV}[[:space:]]*=[[:space:]]*'${canonicalLeaderPaneId}';[[:space:]]+${powerShellRootAssignment}&[[:space:]]+${powerShellQuoted}[[:space:]]+${powerShellQuoted}[[:space:]]+hud[[:space:]]+--watch([[:space:]]+(minimal|focused|full))?$`; | ||
| const envelopedPowerShellCommand = `powershell\\.exe[[:space:]]+-NoLogo[[:space:]]+-NoExit[[:space:]]+-Command[[:space:]]+'\\$env:OMX_SESSION_ID[[:space:]]*=[[:space:]]*''${expectedOwner.sessionId}'';[[:space:]]+\\$env:${OMX_TMUX_HUD_OWNER_ENV}[[:space:]]*=[[:space:]]*''1'';[[:space:]]+\\$env:${OMX_TMUX_HUD_LEADER_PANE_ENV}[[:space:]]*=[[:space:]]*''${canonicalLeaderPaneId}'';[[:space:]]+${envelopedPowerShellRootAssignment}&[[:space:]]+''([^']|'')*''[[:space:]]+''([^']|'')*''[[:space:]]+hud[[:space:]]+--watch([[:space:]]+(minimal|focused|full))?$`; | ||
| return `#{||:#{m/r:^${posixSplitMarker}${posixCommand},#{pane_start_command}},#{||:#{m/r:^${powerShellSplitMarker}${powerShellCommand},#{pane_start_command}},#{m/r:^${envelopedPowerShellCommand},#{pane_start_command}}}}`; |
There was a problem hiding this comment.
For the native-Windows HUD shape created via createHudWatchPane, pane_start_command starts with $env:OMX_TMUX_SPLIT_OPERATION_MARKER = ...; powershell.exe -NoLogo -NoExit -Command .... The owner guard only allows powerShellSplitMarker before the raw $env:...; & node command, while the enveloped powershell.exe branch has no marker prefix, so mutateInsideTmuxHudPane rejects resize/kill operations for those HUD panes and leaves duplicates or stale sizes in the inside-tmux launch workflow. Please allow the same split-marker prefix on the enveloped PowerShell branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d01d62b135
ℹ️ 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".
| config.workers = config.workers.filter(w => !removedSet.has(w.name)); | ||
| config.worker_count = config.workers.length; | ||
| await saveTeamConfig(config, leaderCwd); |
There was a problem hiding this comment.
When scaleDown removes an idle selected worker that still owns a pending task, or any worker with a live claim under force, this path persists the worker removal without the deleted reconciliation that cleared owner/claim for nonterminal tasks. Those tasks keep pointing at a worker no longer in config.workers; claimTask rejects them for other workers as claim_conflict, so work can be orphaned after a successful scale-down. Requeue or clear affected tasks under the task locks before this save.
Useful? React with 👍 / 👎.
| killOperationPane(context.paneId); | ||
|
|
||
| for (const taskId of createdTaskIds) { | ||
| await rm(join(leaderCwd, '.omx', 'state', 'team', sanitized, 'tasks', `task-${taskId}.json`), { force: true }).catch(() => {}); |
There was a problem hiding this comment.
When a team is running with an explicit state root (OMX_TEAM_STATE_ROOT/OMX_ROOT, so canonical state is not <leaderCwd>/.omx/state), createStateTask writes the new scale-up tasks under the canonical root but rollback deletes only this hard-coded default path. A later pane or dispatch failure returns an error while leaving tasks assigned to a worker that was never added, so subsequent scheduling sees stale work. Use the same canonical/team state root used for task creation here.
Useful? React with 👍 / 👎.
| const worktreePlan = planWorktreeTarget({ | ||
| const worktreeMode = effectiveWorktreeMode; | ||
| const workerWorkspaceResult = worktreeMode.enabled | ||
| ? ensureWorktree(planWorktreeTarget({ |
There was a problem hiding this comment.
In worktree mode, planWorktreeTarget or ensureWorktree can throw for normal runtime conditions such as a missing/dirty/conflicting git worktree, but scale-up has already materialized incoming tasks before reaching this call and there is no surrounding try/catch to invoke rollbackScaleUp. That means the promise rejects and leaves tasks and the prepared worker directory for a worker pane that was never created; catch preparation failures and return through the rollback path like the previous implementation did.
Useful? React with 👍 / 👎.
Fresh exact-head adversarial GJC verdictREQUEST_CHANGES / BLOCK — not MERGE_READY Reviewed exact pushed head Blocking findings
Verified repair evidence
Evidence boundaryLinux source review and Linux mocked-contract fixtures only. No real Windows, default Windows PowerShell, psmux 3.3.4, pane-recycling, session-replacement, or full lifecycle execution was run. This limitation is preserved honestly and does not override the source-proven blockers. No merge, auto-merge, release, publish, main-branch, or branch-deletion action was performed. Signed: GJC exact-head adversarial review for PR #3145; reviewer session |
cdafe72 to
149776b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Terminal exact-head hostile review — MERGE_READYExact head: Verdict: MERGE_READY / APPROVE All five canonical P1 exact-authority gaps are source-proven closed. The final detached-HUD path now uses a quote-free immutable operation marker, preserves a single tmux Exact-tree gates:
No real Windows/default PowerShell/psmux lifecycle claim is made. Signed: GJC hostile exact-head Architect review; Architectural Status CLEAR; Code Review Recommendation APPROVE; terminal verdict MERGE_READY. — |
149776b to
527f509
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Corrected terminal exact-head verdict — MERGE_READYThis verdict supersedes the earlier MERGE_READY / APPROVE — zero P0/P1/P2 findings. The final delta closes the healthy detached-HUD marker mismatch by emitting the UUID operation marker quote-free, exactly matching every immediate/deferred final-sink predicate. All five canonical P1 authority gaps from comment Verification evidence for this exact tree:
Signed: GJC hostile exact-head Architect review; Architectural Status CLEAR; Code Review Recommendation APPROVE; terminal verdict MERGE_READY. — |
Merge and closure receipt
— |
1 participant
Closes #3123
Summary
$env:assignments plus&for Team and standalone HUD startup, preserving paths and values containing spaces or embedded single quotes<pane_id> 0observations before usesrc/team/__tests__/runtime.test.tsis the one documented exception to the original four-file plan: its native-Windows shutdown mock needed stateful global snapshot/liveness responses so the broader regression gate exercised the new contract instead of failing from stale fixture topology.Verification
Contract tests: Linux fixtures.
npm run build— passnode --test dist/hud/__tests__/tmux.test.js dist/team/__tests__/tmux-session.test.js— 280/280 passnode dist/scripts/run-test-files.js dist/hud/__tests__ dist/team/__tests__— 49 test files passnpm run lint— passnpm run check:no-unused— passgit diff --check— passorigin/devbaseline comparison for the six previously failing startup/shutdown cases — 6/6 pass on baseline and 6/6 pass on the branch after the issue-scoped fixesIndependent exact-head gates for
87279997415ed7c9c89f76c38284408c932c3c74:Scope boundaries
Real Windows/psmux runtime: not run. Linux mocked
win32fixtures prove command/parser/lifecycle contracts only.—
[repo owner's gaebal-gajae (clawdbot) 🦞]