Repository navigation
🤖 fix: verify-xum names the umask fix, fails fast on a lost leader, keeps the guard from agent-browser - #7917
Conversation
…eeps the guard from agent-browser Under umask 002 git makes a clone's folder, .git and .git/worktrees/<name> 0775, so every command refused without saying what to do. The git-dir refusals now name the folder and the fix (chmod g-w,o-w on it, or a checkout made under umask 022), and SKILL.md says so next to the rule. A leader that cannot write its id to state.env exits before make, but launch waited for the whole deadline. The wait loop now stops when the leader is gone and no process carries the run's id, and says why. From launch_failed, cleanup runs agent-browser with the launch guard on fd 9. Those calls now close fd 9, so an agent-browser daemon started there cannot hold the flock and block every later launch. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh` • Cost: `$186.72`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=xhigh costs=186.72 -->
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. |
![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: 1b3afd7df1
ℹ️ 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".
The umask fix printed the folder as is, so a folder with a space or a glob character gave a command that split or expanded the path. The fix now quotes the folder with printf %q, so the command can be copied as printed. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh` • Cost: `$186.72`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=xhigh costs=186.72 -->
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dc5c63a5a
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Bash 5.2's patsub_replacement reads & and \ in the replacement of
${var//pat/rep} again, so the printf %q quoting of a folder named
amp&ersand or back\slash was undone and the printed chmod command ran
shell syntax or missed the folder. The fix text is now split at @dir@
and the folder is printed with %q between the two parts.
---
_Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh` • Cost: `$186.72`_
<!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=xhigh costs=186.72 -->
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…ly for your own folders (coder#7919) ## Summary Every verify-xum launch on a host whose `/usr/bin/date` is uutils coreutils 0.8 failed at the 300 s deadline. This PR fixes that: launch now reads its start time from `node -p 'Date.now()'`. It also adds one coder#7917 advisor follow-up: the `chmod` fix in the git-dir refusal now appears only for a folder the user owns. Only files under `.xum/skills/verify-xum/` change. ## Background - Launch waits for a build stamp newer than its own start (`t0`). `scripts/build-main-watch.js` writes that stamp as `Date.now()` in milliseconds. `t0` came from `date +%s%3N`. - uutils date 0.8.0 ignores the `3` in `%3N` and prints nanoseconds. On this host, `date +%s%3N` printed `1793814151776897404` (19 digits). - No 13-digit stamp is ever newer than that, so launch never saw a fresh build. - coder#7917 added a `chmod g-w,o-w <folder>` fix to the refusals for group-writable git dirs and the folders above them. For a folder the user does not own, such as a root-owned, group-writable folder above the checkout, that command fails with "Operation not permitted". ## Implementation - `t0=$(node -p 'Date.now()')` takes the stamp's own clock and unit. `node` is already a launch requirement. `t0` is the only millisecond timestamp in the skill. The run id's `date -u +%Y%m%d-%H%M%S` has no width flag. - In `require_safe_ancestors`, a writable folder that the user does not own gets "use a checkout under folders you own" instead of the `chmod` fix. `refusal_fix` now also prints a fix text that names no folder. SKILL.md says the same next to the umask 002 note. ## Validation - **Launch on this host**, with `VERIFY_XUM_READY_TIMEOUT=120`, comparing main (96216fc) with this branch: | `date` on PATH | `date +%s%3N` prints | main | this branch | | --- | --- | --- | --- | | uutils 0.8.0 (`/usr/bin/date`) | 19 digits | fails at the deadline after 122 s: "launch did not finish in 120 s" | ready after 19 s | | GNU coreutils 9.7 (`/usr/bin/gnudate`, linked as `date` on PATH) | 13 digits | ready after 13 s | ready after 13 s | - **Folder you do not own:** a repo made under umask 002, below a root-owned 0775 folder (created with `sudo` for the test). - Main printed `run chmod g-w,o-w $B/rootdir` for the root-owned folder. That command failed with "Operation not permitted", and every later run printed it again. - This branch still prints the `chmod` fix for the user's own `.git` and repo folder, and those commands exit 0. For the root-owned folder it says "use a checkout under folders you own". - **Full token-mode proof** of SKILL.md and every `features/*.md` recipe, on this host with uutils date: 184 PASS, 0 FAIL at 15e96ea (and at d9ce137 before the merge of coder#7917). - `make static-check`, plus shellcheck 0.8.0 (CI's version) over all 46 shell scripts. ## Risks The risk is low. Only the agent-facing verify-xum skill changes. `t0` now comes from the same clock that writes the build stamp. The refusal change only alters the text of a message that already refuses. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh` • Cost: `$213.46`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=xhigh costs=213.46 -->
Summary
This PR adds three small verify-xum follow-ups from the #7914 advisor. On a umask 002 host, the git-dir refusal now names the folder and how to fix it. Launch fails within seconds when the new server group cannot record its id, instead of waiting out the deadline. Cleanup's agent-browser calls no longer inherit the launch guard, so they cannot block later launches. Only files under
.xum/skills/verify-xum/change.Background
.gitand.git/worktrees/<name>with mode 0775. So every command refused, and the message did not say what to do.makewhen it cannot append its id tostate.env. Launch still waited for the wholeVERIFY_XUM_READY_TIMEOUT(default 300 s) before it failed.launch_failedruns cleanup while it holds the launch guard on fd 9. Cleanup's agent-browser calls inherited that fd, so any daemon they started would hold the flock and block every later launch from the checkout.Implementation
require_private_dirandrequire_safe_ancestorstake an optional fix text. The git-dir rule passes one that names the refused folder: "run chmod g-w,o-w (git makes its folders group-writable under umask 002), or use a checkout made under umask 022". SKILL.md says the same next to the rule. The folder in that command is quoted withprintf %qand inserted by splitting the text at@DIR@, not by${var//pat/rep}, because bash 5.2'spatsub_replacementundoes the quoting of&and\(review: quote the directory, preserve escaping). A folder with a space, a glob character,&or\can then be fixed by copying the command.$!. When that process is gone and no process carriesVERIFY_XUM_SERVER_OF=<run id>(the leader has it from its start, also when setsid forked), the loop readsstate.envonce more. If no id is there, it fails with "the server's process group exited before it wrote its id to …/state.env (a read-only file or a full disk?)".agent-browser session listandclosein cleanup run with9>&-.Validation
Each proof compares this branch with main at 26b8c8f.
git initandgit worktree addunder umask 002 had the repo,.gitand.git/worktrees/wtat 0775.chmodfix:worktrees/wt,worktrees,.git, then the repo. After the fourchmodcommands it names,envreaches "no run at …", so the git-dir checks pass.$F/Xum Repo*,$F/amp&ersandand$F/back\slash, each printed command exits 0, andenvthen passes. Before the quoting, the printed command failed for the first. With replacement-based quoting, the&case ranersand/.gitas a second command and the\case failed.setsidmadestate.envread-only just before the leader appends its id (VERIFY_XUM_READY_TIMEOUT=20).agent-browserstarted a stand-in daemon (sleep, with its stdio on /dev/null, keeping every inherited fd) onsession list.session listandcloseof an unknown session left no process holding a test flock. The stand-in shows what the change prevents. It is not a failure I saw with 0.33.2.features/*.mdrecipe: 184 PASS, 0 FAIL (at cdcce43, before the merge of main and the quoting fix, which changes only the refusal text).make static-check, plus shellcheck 0.8.0 (CI's version).Risks
The risk is low. Only the agent-facing verify-xum skill changes. The fail-fast check runs only after
$!has exited without an id instate.env, and it readsstate.envonce more before it fails. A leader that wrote its id and then died is still handled by the startup checks that follow.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh• Cost:$186.72