Repository navigation
🤖 refactor: agent-file, config and bash stat calls use HostFiles - #7360
Merged
Merged
Conversation
…tion # Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
xum_agents_path/read/write and the bash.ts repo-dir stat go through nodeHostFiles.promises. configReadWrite takes a HostFiles argument that defaults to the host, and a new test runs read, write and the symlink refusal on the Node and memory layers. Six pairs leave the local/no-host-imports allow map (18 pairs in 10 files -> 12 in 5). --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$85.60`_
…tests cover it Quality / CRAP flagged sendElectronNotification as a new function (moved from notify.ts) at CRAP 34. createElectronDesktopNotifications takes the electron check and import, and tests drive a fake electron module through the shown notification, the unsupported platform, a failed import and the click. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$85.60`_
…fications # Conflicts: # .oxlintrc.json # eslint.config.mjs # src/node/services/di/layers/stores.ts
# Conflicts: # src/node/services/tools/bash.ts
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. |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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. |
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 30 model calls · 207.3k tokens (73% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 14s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.1k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 22.1s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
# Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
Member
Author
|
@codex review |
🛡️ 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. |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
This was referenced Oct 23, 2026
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…2.23.4 sandbox image (coder#7369) ## Summary This PR fixes the two problems that keep main red, in one PR, because each fix fails CI without the other. The first commit is Thomas's fix from coder#7364 (cherry-picked, his authorship kept). It moves the ask_user_question argument schemas out of `toolDefinitions.ts`, so the renderer entry chunk no longer pulls in every tool schema and `rendererFirstLoad.test.ts` passes again. The second commit pins the bug-bash sandbox image that main published from coder#7362 (base `node:22.23.4-slim`, the CVE-2026-111497 fix), so the model-driven story jobs run again on main and in the merge queue. Fixes coder#7363. ## Background - coder#6887 made `openAskUserQuestions.ts`, which the eager `StreamingMessageAggregator` loads, import `TOOL_DEFINITIONS`. That moved `toolDefinitions.ts` and its imports into `main-*.js` (+92 KB raw, +23.7 KB br) and fails `Test / Unit (5/6)`: "renderer first load > the entry's static imports do not reach the tool definitions module". - coder#7362 changed the sandbox Dockerfile, so the inputs key on main is now `2bf9e12d…`, but `image.json` still pins the old image. `make test-stories` refuses the old record ("sandbox refused: stale image: image.json has inputs key 08a81da3ddeb1ec1, this checkout 2bf9e12d3ac7dcb1"). `Test / Stories` and `Test / Stories (Haiku)` count in `Required` (`STORIES_MODE` and `STORIES_PR_MODE` are `block`), so the queue removed coder#7360 and coder#7364 on that failure. So coder#7364 alone fails the story jobs, and the pin alone fails the unit test. The queue groups entries with `ALLGREEN`, so neither can merge first. This PR carries both. ## Implementation - `perf: keep toolDefinitions out of the main chunk (coder#7363)` is fd6b719 from coder#7364 with no change (the same `git patch-id`). The new `src/common/utils/tools/askUserQuestionSchemas.ts` imports only zod and holds the three ask_user_question schemas. `toolDefinitions.ts` imports and re-exports them, and `openAskUserQuestions.ts` parses with `AskUserQuestionToolArgsSchema` directly. coder#7364 has the bundle measurements: the main chunk is back within 750 B br of the build before coder#6887. coder#7364 can close after this merges. - `tests/bugbash/sandbox/image.json` gets this record, written with the same `jq -n` command that `build.sh --push` uses: | Field | Value | | ---------------- | ---------------------------------------------------------------------------------------------------------------------------- | | `digest` | `sha256:e3804b5418a8e6783698d807c75cc256efae45b81b03400644ae7f13364c9a03` | | `inputsKey` | `2bf9e12d3ac7dcb10397698df2792bf1b244b3fcbe68225378302ef695ba302a`, what `make -s bugbash-sandbox-key` prints on this commit | | `playwrightCore` | `1.63.0` (unchanged) | | `publishedFrom` | `2e2a2b769f417aedcecc09f33abb379d6c8be7b1` (main, the coder#7362 merge) | ## Validation - `rendererFirstLoad.test.ts` fails on the pin commit alone (21f9c3f) and passes with the cherry-pick. - Publish run on main: [40016150145](https://github.com/coder/xum/actions/runs/40016150145), from `2e2a2b769f`. The digest is the subject of its build provenance attestation. - `gh attestation verify oci://ghcr.io/coder/xum-bugbash-sandbox@sha256:e3804b54… --repo coder/xum` passes. The signer is `.github/workflows/bugbash-sandbox-image.yml@refs/heads/main` (workflow_dispatch, run 40016150145), and the source commit is `2e2a2b769f`. - I pulled the image by digest. It is linux/amd64, its `org.xum.bugbash.inputs` label is `2bf9e12d…a302a`, its `org.xum.bugbash.playwright-core` label is `1.63.0`, and `node -v` in it prints `v22.23.4`. Co-authored-by: Thomas Kosiewski <tk@coder.com> --------- Co-authored-by: Thomas Kosiewski <tk@coder.com>
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
… contract (coder#7367) ## Summary Adds two test layers for `SessionLocks` and runs the lock contract on four layers: node, memory, node + foreign and memory + foreign. It is PR 6 of phase 4. It changes only test harnesses and one test file. ## Order - Plan order: PR 6 comes after PR 5 (coder#7355), on top of PR 8 (coder#7354). - Landing order: PR 6 lands on main as its own PR, before PR 8 and PR 5. - Why: PR 6 waits only for PR 4 (coder#7345), which has merged. It uses no code from PR 8 or PR 5. PR 5 is held until coder#7360 merges, because both change the lint configs. Basing PR 6 on main keeps it out of that wait. ## Background Phase 4 moves the loop's lock calls behind `SessionLocks`. Later PRs (9 to 16) test the moved code with locks that need no real files and no real waits, and that play other backends. One contract runs on every layer, so the fakes cannot drift from the Node locks. ## Implementation - `memorySessionLocks.testHarness.ts`: `SessionLocksMemory` builds a fresh `MemoryLockTable` at each build of the layer. An acquire takes a free or dead lock at once. Otherwise it waits in FIFO order until a release hands the lock over, or until the `HostClock` passes the timeout. Under a TestClock the test moves time with `adjust`. Refusals use today's text (`fileLockTimeoutMessage`, `crossProcessLockTimeoutMessage`) and today's argument asserts. An aborted signal throws today's abort error. A record lock creates its missing parent, as Node does. Three parts stay on the Node locks: - Init-record locks (`foundByReaddir`: the path has the segment `/locks/init-record/`). `InitStateManager` lists their folder, so a holder in memory would leave no file and a live init would look abandoned. - Record locks with `createParentDirectory: false`. WorkflowRunStore needs their acquire to fail with ENOENT once the run directory is gone, with no window between a check and the take. Only the Node lock's own write gives that. - The use leases. The mutation gate, `isHeld` and `taskService` read other backends' lease files. Phase 5 moves them. - `foreignHolders.testHarness.ts`: `makeForeignHolders()` gives the test a handle (`hold`, `release`, `reclaimed`). `withForeignHolders(inner, holders)` wraps a layer. A live foreign holder refuses at once with today's text, and an aborted signal wins over it. A dead one is reclaimed by the next acquire, and its path goes to `reclaimed`. A probe reclaims nothing. `hold()` publishes the lease first and probes the gate second, as `WorkspaceUseLeases.hold` does. A live holder on `workspaceMutationLockPath` makes it release the lease and throw `WorkspaceMutationInProgressError` with today's text. `isHeld` counts a live fake lease in the workspace's lease folder. Every fake holder records an identity (`foreignIdentity()`), so its text equals the Node text for a current record. `liveBlocker(pid)` gives that blocker. Fake pids are `foreignPid(n)` = `process.pid + n`, so a fake holder is never this process. - `sessionLocks.contract.test.ts`: the contract runs on the three new layers. The Node text test now compares against `liveBlocker`, so the fake text equals the Node text for the child's pid. Four new contract cases run on all 4 layers: an aborted signal wins over a live holder, `createParentDirectory: false` on a missing parent rejects with ENOENT, an aborted signal wins over a missing parent, and a record lock creates its missing parent by default. Eight more cases: - memory: a 60 s timeout passes on `TestClock.adjust`, and it has not settled 1 ms before the timeout. - memory: a release hands the file lock to the waiters in FIFO order, and the old handle's `assertStillOwned` then throws. - memory: an init-record lock makes a real file, and inspect sees it. - memory: each build of `SessionLocksMemory` has its own table. - foreign: dead holders are reclaimed in order, and a probe reclaims none. - foreign: a live foreign mutator makes `hold()` refuse with today's text. - foreign: `hold()` publishes its lease before it probes the gate. - foreign: a live fake lease of another backend makes `isHeld` true. ## Validation - `bun test ./src/node/services/sessionLocks/`: pass. The contract file has 53 pass, 0 fail. - Scratch checks, each reverted: - Drop the live check in `wrapWithForeignHolders`: the 4 live cases of node + foreign and memory + foreign fail. - Drop `foundByReaddir`: the init-record case fails. - Share one table across builds of `SessionLocksMemory`: the fresh-table case fails. - Drop the abort check in the foreign layer: the abort case of both foreign layers fails. - Send `createParentDirectory: false` to the memory table instead of the Node locks: the ENOENT case of both memory layers fails. - Drop the memory layer's mkdir: the create-parent case of both memory layers fails. - Drop the fake-lease check in the foreign `isHeld`: the isHeld case fails. - Probe the fake gate before the lease: the gate-order case fails. - `make static-check`: pass. - `make crap-check`, `make dry-check`, `make mutate-check` (base = the parent commit): PASS with 0 functions in scope. The PR changes only test harnesses and a test file. - `make test` on 3e31a0aa6a, which is this diff on coder#7355's head 2f34c99: 27689 pass, 0 fail in the main run, and every other run passes. The second half (`tests/ui/storybook/`, `tests/ui/domIsolation.test.ts`, `vscode/src/`, `scripts/`): 732 pass, 0 fail. - Tripwire (`scripts/perf/loopTestBaseline.ts --runs 3`, base = coder#7355's head 2f34c99, on 3e31a0aa6a): 0 trips. - On the head 423f813: `bun test ./src/node/services/sessionLocks/`, `make static-check` and `make lint-eslint` pass. The tripwire and `make test` were not rerun after the move to main and the Codex fixes, because the PR changes only test files under `sessionLocks/`. ## Follow-ups - Codex round 4 (deferred, not a blocker): the memory layer's `acquireProcessFileLock` does not create the missing parent of `lockPath`, while `fileLock.ts` does. No test uses the memory layer yet. PR 9 adds the mkdir and a contract case before it moves any caller. Tracked in coder#7326. (PR 9 fixes this one.) - Codex round 5 (deferred, not a blocker): in the foreign-holder layer, a fake live holder on a record lock with `createParentDirectory: false` refuses with a timeout even when the parent is gone, where Node gives ENOENT. No test combines the two yet. The first PR that runs such a caller on `withForeignHolders` fixes it. Tracked in coder#7326. ## Risks Test-only. No production file imports the new harnesses. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$103.72`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=103.72 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…rough HostProcesses (coder#7366) ## Summary 5a PR 9 of phase 5 (Loop host I/O) of the agent-loop refactor. A new `HostProcesses` service holds the host calls that the import lint cannot see: the `execFileAsync` spawn and the global `fetch`. `tools/skillsCatalogFetch.ts` makes its 3 git spawns and its catalog search through it. `gitNoRepoAutomationEnvForLocalRepo` (the bash tool's repo-automation probe) takes it as an optional argument. A script-table test layer answers commands and URLs without a process or a socket, and 5a PR 10 (FakeRuntime) reuses its table. Behavior does not change: the Node layer calls the same `execFileAsync` and `fetch`. The allow map does not change (12 pairs in 5 files). ## Order This PR is stacked on 5a PR 8 (coder#7360). Plan order is PR 1 to PR 14, so it keeps its place. ## Implementation - `hostProcesses/hostProcesses.ts`: `HostProcesses` with `execFile(file, args, options?)`, which returns `{ result }` and is `Disposable` like `execFileAsync`'s result, and `fetchUrl(url, init?)`. The plan names the second member `fetch`. I named it `fetchUrl`, so that the plan's end check `rg '\bfetch\('` over the tools still finds only direct calls of the global. - `hostProcesses/nodeHostProcesses.ts`: the Node layer. Each member reads `disposableExec.execFileAsync` and `globalThis.fetch` when it is called, so the existing spies of `skillsCatalogFetch.test.ts` and `gitNoHooksEnv.discovery.test.ts` still reach the moved calls. - `hostProcesses/scriptedCommands.testHarness.ts`: a table from command line to outcome (stdout, stderr, exit code, or a function of the call), and `createScriptedHostProcesses(commands, urls)` on it. A command or URL without an entry rejects and names itself. A non-zero exit rejects with the same error fields as `execFileAsync`. - `di/tags.ts`, `di/layers/stores.ts`: `HostProcessesTag` joins `StoreTags`, and `StoresLive` gives the Node layer. - `tools/skillsCatalogFetch.ts`: `nodeHostProcesses.execFile` and `nodeHostProcesses.fetchUrl` in place of `execFileAsync` and `fetch`. - `utils/gitNoHooksEnv.ts`: `gitNoRepoAutomationEnvForLocalRepo(repoPath, signal, allowNonRepository, processes = nodeHostProcesses)`. `bash.ts` keeps its call as it is and gets the default. A later PR passes the loop's `HostProcesses` there. ## Validation - `hostProcesses/hostProcesses.test.ts` (new, 9 pass): the Node and scripted layers give the same result for the same two commands (exit 0 with output, exit 3 with stderr). The scripted layer names a command or URL that has no entry and records each call. The probe spawns through the processes it gets and fails closed. Spies on the `disposableExec` namespace and on `globalThis.fetch` reach the Node layer. - `bun test` of the plan's list (`skillsCatalogFetch`, `gitNoHooksEnv`, `gitNoHooksEnv.discovery`, `hostProcesses/`) plus `bash`, `skills_catalog_read`, `di/` and `scripts/noHostImports.test.ts`: 295 pass, 0 fail. - `rg -n 'execFileAsync|fetch\(' src/node/services/tools/skillsCatalogFetch.ts` prints nothing. - `make static-check`, `make lint-eslint`, `make dry-check`. - `make test` (host suite lock) on head d971f7f (this PR merged onto 5a PR 8 at 9c3218d): 27679 pass, 7 skip, 0 fail, then 729 pass, 0 fail, and each of the 18 isolated files 0 fail. - The ui line on that head: 732 pass, 0 fail. - Loop-test tripwire on that head, 3 runs against main: 0 trips. - Round 3 (c6d5dce, test only), after the Codex re-review of the restacked head: `commandLine` JSON-quotes a token with whitespace, a quote or a backslash (and the empty token), so two argv arrays never share a key. The scripted layer rejects once its signal aborts, as `execFileAsync` does after it kills the child. A new case runs on both layers and times out on the scripted layer without the fix. `bun test src/node/services/hostProcesses/` plus `gitNoHooksEnv` and `skillsCatalogFetch` tests: 71 pass. `make static-check`. - Round 4 (c6bda94, test only): the scripted layer drops no option that a caller sets. A fixed outcome settles at once and reads no env, so `timeoutMs`, `killTreeOnTermination` and `env` hold for it. A function entry refuses `timeoutMs` and `env`. An outcome with more output than `maxOutputBytes` is refused, and `onStderrData` is always refused. Each refusal names the option. No existing test changed. A new case fails without the fix. The `hostProcesses/`, `gitNoHooksEnv`, `gitNoHooksEnv.discovery` and `skillsCatalogFetch` tests: 163 pass. `make static-check`. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$94.44`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=94.44 -->
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
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
5a PR 8 of phase 5 (Loop host I/O) of the agent-loop refactor. The AGENTS.md tools (
xum_agents_*), the config file helper of the config tools and the repo-dir check ofbash.tsmake their host file calls throughHostFiles(5a PR 5, #7319). So 6 frozen pairs oflocal/no-host-importsleave the allow map. The map goes from 18 pairs in 10 files (5a PR 4) to 12 pairs in 5 files. Behavior does not change: the Node layer calls the samefs/promisesandwriteFileAtomicfunctions.Order
5a PR 3 (#7351) merged at 15:40 UTC and 5a PR 4 (#7352) at 17:05 UTC, so this PR now targets main. The merge queue removed it once at 17:05 UTC:
Test / Storiesfailed because its Sonnet probe got HTTP 502 twice, before any story ran. Merge commit 9c3218d brings in main 32b80b2. Its only conflicts were the allow-map lines next to the notify.ts pair, and it keeps this PR's removals. Plan order is PR 1 to PR 14, so this PR keeps its place. It touchesbash.tsand the allow-map lines next to the lines that PRs 3 and 4 change, so it builds on them and does not conflict with them.Implementation
tools/xum_agents_path.ts,tools/xum_agents_read.ts,tools/xum_agents_write.ts:nodeHostFiles.promisesin place of thefs/promisesnamespace import, as 5a PRs 6 and 7 did for the skill tools.tools/bash.ts:969:assertLocalRepoDirectoryExistscallsnodeHostFiles.promises.stat. It was the only use of thefsimport, so the import goes.tools/shared/configReadWrite.ts:readConfigDocumentUnvalidatedandwriteConfigDocumenttake a lastfiles: HostFilesargument that defaults tonodeHostFiles. The read, thelstatsymlink check, themkdirand both atomic writes go through it. The callers (xum_config_read.ts,xum_config_write.ts) do not change. The async write passes{ encoding: "utf-8" }in place of the string"utf-8", because theHostFilesmember takes only the options object.writeFileAtomicresolves both forms to the same options.eslint.config.mjs,.oxlintrc.jsonandscripts/noHostImports.test.ts: the 6 pairs leave the allow map and the frozen list.Validation
tools/shared/configReadWrite.test.ts(new): on the Node layer (a temp dir) and on the memory layer (createMemoryHost), a missing file reads as{}, a writtenconfig.jsonreads back with a newwriteId, a writtenproviders.jsonckeeps its comment header and reads back, and a write to a symlinkedconfig.jsonthrows and leaves the link's target unchanged. 8 pass.bun testof the plan's list (xum_agents,bash,shared/configReadWrite) plusxum_config_read,xum_config_writeandscripts/noHostImports.test.ts: 158 pass, 0 fail.make static-check,make lint-eslint.make test(host suite lock) on head 1ae3fe1 (before the merge of 5a PR 4): 27780 pass, 7 skip, 0 fail, then 729 pass, 0 fail, and each of the 18 isolated files 0 fail.make static-checkand the targeted list (158 pass, 0 fail).git diff origin/mainof the three lint files shows only the 6 pairs.make static-check,make lint-eslint, and the targeted list plusnotifyanddesktopNotifications/(171 pass, 0 fail). The merge had one conflict: thehostClockimport line ofbash.ts, where main addednodeClock. The merge keeps both imports.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$90.60