🤖 ci: accept resolved Codex advisories on the review summary board - #4223
Merged
Conversation
Codex keeps resolved security advisories listed on its summary board and only adds the Resolved marker once a later review completes, so a completed board with the "Security findings" section failed the Codex Comments gate forever. Whitelist the section header, the advisory count line and bullets that link a review thread on this PR and end with Codex's Resolved marker; bare bullets, unknown sections and non-thread links keep blocking.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Sep 11, 2026
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 12, 2026
…4211) ## Summary Replaces the always-expanded init banner pinned at the top of the transcript with a Codex-style workspace creation card: it renders directly after the user message that created the workspace, shows a step checklist with a live checkout progress bar while running, collapses to a one-line **Workspace created in Ns** header on success, and stays expanded with the exit code and stderr on failure. ## Background The `workspace-init` row was prepended to `getDisplayedMessages` with `historySequence: -1` and rendered fully expanded forever, so every new chat opened with a wall of setup output above the first message. Runtime step markers (`InitLogger.logStep`) and raw hook stdout were flattened into identical `init-output` lines, and local worktree creation ran `git worktree add` buffered, so no checkout progress existed to display. ## Implementation - **Placement:** the card is inserted after the first `user` row of the displayed transcript (index 0 when there is none). Chronology is not usable because `init-start` fires before the first message is persisted. - **Contract:** `init-output` gains optional `step: true` for `logStep` lines (persisted and replayed; older `init-status.json` files simply render without a checklist). A new ephemeral `init-progress { label, percent }` event and `InitLogger.logProgress?` carry checkout progress; it is never persisted or replayed. - **Local checkout progress:** `WorktreeManager.createWorkspace` now runs `git worktree add --no-checkout`; `materializeWorkspace` then populates the files with `git -c core.hooksPath=/dev/null checkout --quiet --progress --no-recurse-submodules` while HEAD still holds the branch, using `GIT_PROGRESS_DELAY=0` (no `--force`: the checkout runs in an announced workspace, so anything written there meanwhile fails the checkout instead of being overwritten), streaming stderr through a small `GitProgressParser` that splits on `\r`/`\n` and emits deduplicated percent updates. Git's diagnostics are held until the exit status is known and classified once: plain output on success, error output on failure. Once the files are in place HEAD moves to an unborn placeholder ref and a second, write-free `git checkout` switches back, which keeps the `post-checkout` hook contract identical to a plain `git worktree add` (`<null> <new> 1`) while the branch stays claimed for the whole streamed checkout; and `--no-recurse-submodules` mirrors what `worktree add` does internally (linked-worktree submodule repos do not exist yet; `syncLocalGitSubmodules` materializes them). Remote runtimes are unchanged; their existing `logStep` calls become checklist items automatically. - **Deferred local materialization:** the renderer only subscribes once `create()` has announced the workspace, so progress emitted inside `runtime.createWorkspace` could never reach the card (Codex caught this; the new IPC test that subscribes after `create()` resolves fails on the previous head). `WorkspaceService.create` now asks the worktree runtime for `deferMaterialization`: creation reserves the worktree (`add --no-checkout`, unborn HEAD, branch mapping) and returns, the workspace is announced, and the streamed checkout, `.xumignore` sync, fast-forward and submodule sync run in `Runtime.materializeWorkspace()` at the start of the background init, ahead of the init hook. This matches the `Runtime` contract (create is fast, init streams) and how SSH runtimes already sync in `initWorkspace`. The reserved worktree keeps HEAD on the branch through the reservation gap and the streamed checkout itself (the placeholder flip happens only after the files have landed), so no other worktree can claim the branch until the workspace is complete. Plugin-override sanitization for deferred worktrees runs on every materialization exit, success or failure, and before the hook, exactly like task worktrees; a sanitize failure still tears the creation down. A checkout failure fails the init like a remote sync failure (red card, workspace stays for inspection) and puts HEAD and the index back on the workspace branch, so the retained worktree shows a stray file as a modification rather than committing to the placeholder ref or staging every file as deleted. Only removal may interrupt the file checkout itself (`checkoutAbortSignal`, which kills the whole git process tree, smudge filters included); archive aborts init but keeps the checkout and never reruns it, so it waits for the files to land and parks a complete, sanitized worktree, while everything after them (hook switch, `.xumignore` sync, fast-forward, submodules) honours its abort, `.xumignore` sync included (its `git ls-files` runs with the signal and each copy checks it). If another worktree claims the branch in the instant between the placeholder flip and the hook switch, the failure path detaches HEAD at the branch tip instead of re-attaching, so the rival stays the sole holder and the card shows git's error. `startInit` persists the running init record and `replayInit` finalizes a running record that no live init owns as a failed creation (exit -1 plus an interruption line), so a creation that died with the app opens as a failed card instead of a complete-looking workspace; archive deletes the record it orphans so a cancelled init is not reported as an app exit. Fork, restore, sub-agent tasks, multi-project and devcontainer creation stay eager, and `task(kind="workspace")` opts out via `create(..., { awaitMaterialization: true })` because its agentId validation reads the checkout under the task mutex. - **UI:** `InitMessage.tsx` rewritten in place: header button (`aria-expanded`) with shimmer while running, step checklist (check / spinner / error icon), new shared `ProgressBar` (`role="progressbar"`, no animation), and a **More details** toggle for the raw log and project path. Expand/collapse defaults derive from status (collapsed only on success; details open once finished) with user toggles winning; the only effect keeps the newest log line in view. - **Store:** init output and progress events coalesce UI bumps through `scheduleIdleStateBump`, which now runs a pre-bump prelude that flushes the aggregator's throttled cache so a bump never renders the stale row. ## Validation - Remote dogfood UAT (Coder Agents) on the pre-polish head `c900320a6f` against a real 12k-file `coder/coder` worktree with success and failing `.xum/init` fixtures: placement after the user bubble, running checklist and streaming details, auto-collapse on success and header toggling, failure stays expanded with red stderr, reload persistence, legacy persisted data without step flags, and 375px width with no horizontal overflow all passed. Not observed there: the live percentage bar, because git suppresses progress under 2s and that checkout took about 2s. The follow-up commit sets `GIT_PROGRESS_DELAY=0`, guarded by a real-git `WorktreeManager` test that fails without it (red-green verified). Not covered: sub-agent child transcripts and the packaged Electron shell (renderer only). - Real-git `WorktreeManager` tests guard each checkout invariant and were red-green verified: progress is reported for a one-file checkout (fails without `GIT_PROGRESS_DELAY=0`), `post-checkout` receives `<null-oid> <new> 1` (fails without the unborn HEAD, also across the deferred split), a repo with `submodule.recurse=true` still checks out (fails without `--no-recurse-submodules`), routine git chatter lands in stdout with an empty stderr, a failed checkout is rolled back with its diagnostics classified once and no `\r` in any logged line, a deferred worktree is reserved empty and populated by `materializeWorkspace`, a competing `git worktree add` for the reserved branch is refused during the gap (red when the placeholder HEAD is set at reservation time) and while a gated smudge filter holds the checkout mid-stream (red when the placeholder HEAD is set before the checkout), a rival that claims the branch in the placeholder instant (injected through the exec spy) leaves this worktree detached at the tip with the rival as sole holder (red on d28e33a: two holders), and a reserved-but-never-materialized worktree force-deletes cleanly (what Cancel creation does). - Jest `tests/ipc/workspace/init.test.ts` through the real ORPC path: a subscriber attaching after `create()` resolves receives `init-progress` and sees the checkout complete before the hook (red on the previous head); a repository that tracks `.xum/mcp.local.jsonc` with a committed `plugin:` enable has it pruned after the deferred checkout and before the hook runs (red when the deferred sanitize is removed); a failing checkout ends init with exit code -1, reports the git error once, skips the hook, and keeps the workspace; a broken submodule gitlink that fails materialization after the checkout still gets its committed `plugin:` enable pruned (red when the sanitize only runs on success); archiving while a slow smudge filter stalls the checkout parks a complete checkout with the enable pruned and HEAD on the branch (red with the abort-signal guard); archiving once the files have landed while a trusted post-checkout hook sleeps returns promptly with a complete, clean checkout (times out when archive is not forwarded past the file checkout); `serverUpdateRestartBlockers` waits for the deferred init to settle before enabling the updater, since the checkout is a restart blocker until then. `xumignore.test.ts`: a cancelled signal rejects and copies nothing (copied on the previous head). `initStateManager.test.ts`: a running init record with no live init replays as start, error line, end(-1) and is finalized on disk (nothing was persisted before `endInit` on the previous head). WorktreeManager: cancelling a deferred checkout stalled in a smudge filter shim settles and leaves no helper processes behind (times out without killTreeOnTermination). WorktreeManager: a file written into the reserved worktree before the checkout survives and fails the checkout by name, with HEAD and index restored (red with `--force`). - Restart blocker: `collectRestartBlockers` keeps counting an init until its final status write has landed (`endInit` turns the in-memory status final only after the write); the `serviceContainer` blocker inventory fails without it (red-green verified), and an `initStateManager` test holds the workspace file lock to check the window itself. - Not yet re-run after the deferral: the remote dogfood UAT above (renderer behaviour is unchanged; the bar now has a window to appear during the checkout). - `make static-check`, targeted Bun suites (aggregator, messageUtils, initStateManager, WorktreeManager, gitProgress, WorkspaceStore including a bump-timing test that fails without the pre-bump flush), Jest `tests/ipc/workspace/init.test.ts` and `tests/ui/chat/initMessage.test.ts`, and the InitMessage / App.chatLoading Storybook plays including the pinned phone viewport. ## Declined review findings - Cross-process pending marker for deferred registrations: needs a second Xum process to register the same half-populated worktree directory as a project-dir local workspace during the checkout; task worktrees already have this shape today, and holding the cross-process registration lock across whole checkouts would block every other registration. Left as a known tradeoff. - Checklist failure marker lands on the most recent step (a completion-style step such as "Fetched latest from origin" can be marked failed when the next operation fails): fixing it needs explicit per-step status in the init-output contract; the git error is shown in the red output right below. Follow-up candidate. - Resuming or quarantining a deferred checkout after an app exit: the working tree may hold partially written files, so a resumed populate would need `--force` (removed to protect files the user wrote there), and the workspace may already carry the user's first prompt, so it is neither deleted nor gated at startup. The interrupted creation is reported as a failed card ("Check the checkout before using it, or recreate the workspace") and recreate is the recovery; before this PR the same crash left an orphaned directory with no workspace at all. - Closing the placeholder instant between the symbolic-ref and the hook switch: two consecutive local git commands with nothing in between; removing it means dropping the null-oid post-checkout contract (round 2) or a cross-process lock (declined above). A lost race now fails loudly with a single holder instead of a silent double claim. The holder check and the restore inside that failure path are likewise two consecutive git commands (round 11); the same tradeoff applies and no further narrowing is planned. - Aborting an in-flight `.xumignore` copy: the sync now checks the archive signal between files and its `git ls-files` subprocess is killable; a single in-flight `fs.copyFile` is bounded by that one file and completes rather than leaving a torn destination. Chunked or stream-based abortable copying would add machinery to a best-effort phase for an archive that already waits on the file checkout by design (rounds 8 and 9); no further narrowing of the archive-abort contract is planned in this PR. - Cross-process owner or lease for init records: two Xum processes sharing one config root is not a supported configuration, so replay treats a running record with no live init in this process as an interrupted creation. ## Review record Codex code + security review on every pushed head; each round's findings were reproduced first, fixed or declined with an inline reply, and resolved. | Head | Findings | Disposition | | --- | --- | --- | | `4348f56` | 3 (P1, P2, P2) | fixed in `02ec2c2` | | `02ec2c2` | 2 (P1, P2) | fixed in `2267420` | | `2267420` | 2 (P2, P2) | fixed in `c68f1dd` | | `c68f1dd` | none ("Didn't find any major issues") | | | `8704b61` | 2 (P2, P2) | fixed in `92baaf9` (deferred materialization) | | `92baaf9` | 3 (P2, P2, security) | fixed in `0cda234` | | `0cda234` | 5 (P1, P1, P1, P2, security) | 3 fixed in `a422dae`, 2 declined (below) | | `a422dae` | 3 (P1, P1, P1) | fixed in `9d65557` | | `9d65557` | 2 (P1, P1) | fixed in `d28e33a` | | `d28e33a` | 3 (P1, P2, P2) | 2 fixed in `e501fbb`, 1 fixed in consequence and instant declined (below) | | `e501fbb` | 4 (P1, P2, P2, P2) | 1 fixed in `0668eb8` (restart blocker), 3 declined (below) | | `0668eb8` | 1 (P2) | declined (below) | | `0668eb8` (re-review) | none ("Didn't find any major issues"), security review clean | | The `Codex Comments` gate rejected the review summary board once it listed resolved security advisories; coder#4223 fixed the gate on main. ## Risks - Medium, local workspace creation: the two-step checkout changes how every local worktree is materialized, and UI-created worktrees are now announced before their files exist. Anything that reads the checkout right after `create()` must wait for init like it already does for SSH/Coder workspaces (tools, sends, attachments and skills already gate on `waitForInit`); `task(kind="workspace")` keeps the eager path. A crash between announcement and materialization is reported on the next load as an interrupted creation (failed card) that the user recreates like any failed workspace. Covered by real-git tests for new-branch, existing-branch, deferred, and rollback paths; the resulting worktree, branch, and clean status are asserted. - Low, transcript ordering: only the init row moves; other rows keep array order. Reconnect replay idempotency is unchanged and tested. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `xhigh` • Cost: `$127.35`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=xhigh costs=127.35 -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
Codex Commentsgate treats Codex's review summary board as a blocking comment whenever it carries a "Security findings" section, even after every advisory thread is resolved and both reviews are complete. This whitelists the section when each advisory bullet carries Codex's own Resolved marker, so PRs that once had a security advisory can passRequiredagain. Unblocks #4211.Background
Codex keeps resolved security advisories listed on the board and only adds the
· **Resolved**marker when a later review completes (verified from the board's edit history on #4211: the marker appeared at the next review completion, not when the thread was resolved). The gate's line whitelist (#4149, #4158) does not know the section, so a completed board with resolved advisories is reported as an unresolved Codex comment andRequiredcan never go green. On #4211 the gate log at 17:24Z shows exactly that:status: completed, both rows Completed, two**Resolved**advisories, still counted as blocking. The two most recent merged PRs with such boards (#4170, #4176) only passed because the section was added after their last gate run.Implementation
scripts/lib/codex_comments.jqaccepts three more line shapes inside a completed board:### Security findings,#### Advisory findings (N), and a bullet that links a review thread on a PR (.../pull/N#discussion_r<id>), names a severity, and ends with· **Resolved**. A bullet without the marker is a live finding and keeps blocking, as do unknown sections, non-thread links, and trailing text.Validation
python3 scripts/check_codex_comments_test.py: the existing unresolved-advisory case still expects blocking; new cases cover the resolved board (informational) and three malformed variants (still blocking). Red without the jq change: the resolved case fails1 != 0.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:xhigh• Cost:$16.42