Repository navigation
🤖 refactor: agent-loop tools read Runtime.location instead of instanceof - #7351
Merged
Merged
Conversation
…tion # Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
Member
Author
|
@codex review |
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. |
|
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. |
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 29 model calls · 196.7k tokens (71% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 0s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 27.8s · 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 |
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…ifications (coder#7352) ## Summary 5a PR 4 of phase 5 (Loop host I/O) of the agent-loop refactor. The notify tool shows OS notifications through a new `DesktopNotifications` service, not through its own `import("electron")`. So `tools/notify.ts` loses its frozen `electron` pair of `local/no-host-imports`. The allow map goes from 19 pairs in 11 files (5a PR 3) to 18 pairs in 10 files. Outside Electron the tool returns the same result text as before. ## Order This PR is stacked on 5a PR 3, as in the plan. It waited for phase 3 PR 8 (coder#7277, merged), because `ToolConfiguration` and `turnRequestBuilder.ts` changed there. 5a PR 5 (coder#7319, merged) also added tags to `StoreTags` in `di/tags.ts`. Merge commit f542862 joins both tag lists. 5a PR 3 (coder#7351) merged at 15:40 UTC, so this PR now targets main. Merge commit c525854 brings in main 0856ca4. It keeps both imports and both layer entries in `di/layers/stores.ts`, where phase 4 PR 4 (coder#7345) added `SessionLocksNode`. In the lint configs it keeps main's lines and removes only the notify.ts pair. ## Implementation - `desktopNotifications/desktopNotifications.ts`: the `DesktopNotifications` interface with one method, `show({ title, body?, workspaceId? })`, which resolves `{ success, error? }`. - `desktopNotifications/electronDesktopNotifications.ts`: the Node layer. `createElectronDesktopNotifications(host)` takes the Electron check and the electron import, so tests can pass a fake electron module. It holds the code that `notify.ts` had before (the `import("electron")`, its eslint-disable and today's error text outside Electron). It is not in `AGENT_LOOP_FILES`. - `recordingDesktopNotifications.testHarness.ts`: a test layer that records each request and returns a set result. - `di/tags.ts`, `di/layers/stores.ts`, `di/layers/core.ts`: `DesktopNotificationsTag` joins `StoreTags`, `StoresLive` gives the Electron layer, and `AILive` passes it to `AIService`. - `aiService.ts`, `turnRequestBuilder.ts`, `common/utils/tools/tools.ts`: `AIService` takes optional loop host services and `TurnRequestBuilder` puts `desktopNotifications` on each `ToolConfiguration`. A config without it (tests that build one by hand) uses the Electron layer. - `tools/notify.ts`: calls `config.desktopNotifications.show(...)`, and falls back to the browser notification as before when nothing shows. ## Validation - `tools/notify.test.ts`: 2 new cases. The tool sends the trimmed title, body and workspace ID to `config.desktopNotifications` and reports `notifiedVia: "electron"`. When the service shows nothing, the tool reports `notifiedVia: "browser"`. - `electronDesktopNotifications.test.ts` (new): outside Electron the result is today's text. In Electron (a fake electron host, commit c27edf3), the layer cuts the title and body to the OS limits, shows nothing on a platform without notifications, reports a failed import, and on a click restores and focuses the first window and sends the workspace ID. Quality / CRAP had flagged the moved function as new and untested (CRAP 34). With these tests a local `crap-check` with coverage no longer reports it. - `bun test` of `notify`, `desktopNotifications/`, `turnRequestBuilder` and `scripts/noHostImports.test.ts`: 50 pass, 0 fail. - After merge commit c525854: `make static-check`, and the `notify`, `desktopNotifications/`, `turnRequestBuilder`, `sessionLocks/`, `di/` and `scripts/noHostImports.test.ts` tests (89 pass, 0 fail). - `make typecheck`, `make static-check`, `make lint-eslint`. After c27edf3: `make typecheck`, oxlint and eslint on the folder, and the `notify` and `desktopNotifications/` tests (13 pass). - `make test` (host suite lock) on head b882a24 (before c27edf3, which changes only the Electron layer and its test): the `bun test src` run had 27771 pass, 7 skip and 1 fail. The fail is the known flake `BackgroundProcessManager > spawn > gives concurrent same-name spawns from two backends distinct directories on a non-host runtime`. That file alone then passed (163 pass, 0 fail). `make test` stops at the first failing run, so I ran its other steps by hand: the shared scripts/storybook/vscode run (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. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$85.60`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=85.60 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…er#7360) ## 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 of `bash.ts` make their host file calls through `HostFiles` (5a PR 5, coder#7319). So 6 frozen pairs of `local/no-host-imports` leave 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 same `fs/promises` and `writeFileAtomic` functions. ## Order 5a PR 3 (coder#7351) merged at 15:40 UTC and 5a PR 4 (coder#7352) at 17:05 UTC, so this PR now targets main. The merge queue removed it once at 17:05 UTC: `Test / Stories` failed 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 touches `bash.ts` and 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.promises` in place of the `fs/promises` namespace import, as 5a PRs 6 and 7 did for the skill tools. - `tools/bash.ts:969`: `assertLocalRepoDirectoryExists` calls `nodeHostFiles.promises.stat`. It was the only use of the `fs` import, so the import goes. - `tools/shared/configReadWrite.ts`: `readConfigDocumentUnvalidated` and `writeConfigDocument` take a last `files: HostFiles` argument that defaults to `nodeHostFiles`. The read, the `lstat` symlink check, the `mkdir` and 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 the `HostFiles` member takes only the options object. `writeFileAtomic` resolves both forms to the same options. - `eslint.config.mjs`, `.oxlintrc.json` and `scripts/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 written `config.json` reads back with a new `writeId`, a written `providers.jsonc` keeps its comment header and reads back, and a write to a symlinked `config.json` throws and leaves the link's target unchanged. 8 pass. - `bun test` of the plan's list (`xum_agents`, `bash`, `shared/configReadWrite`) plus `xum_config_read`, `xum_config_write` and `scripts/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. - The ui line on that head: 732 pass, 0 fail. - Loop-test tripwire on that head, 3 runs against main: 0 trips. - After merge commit 9c3218d: `make static-check` and the targeted list (158 pass, 0 fail). `git diff origin/main` of the three lint files shows only the 6 pairs. - After merge commit 7a1c206 (5a PR 4 at c525854, with main 0856ca4): `make static-check`, `make lint-eslint`, and the targeted list plus `notify` and `desktopNotifications/` (171 pass, 0 fail). The merge had one conflict: the `hostClock` import line of `bash.ts`, where main added `nodeClock`. The merge keeps both imports. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$90.60`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=90.60 -->
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 3 of phase 5 (Loop host I/O) of the agent-loop refactor. Agent-loop tools no longer use
instanceof LocalBaseRuntimeorinstanceof RemoteRuntime. They read a newRuntime.locationfield instead. Sotools/bash.ts,tools/skillFileHint.tsandtools/agent_skill_read_file.tsno longer import a runtime class, and their 3 frozen pairs oflocal/no-host-importsleave the allow map. On main (7265ac0) the map holds 22 pairs in 13 files. On this branch it holds 19 pairs in 11 files. Every runtime gives the same answer asinstanceofdid, so behavior does not change.Order
The plan order of stack 5a is PR 1 to PR 14. This PR waited for phase 3 PR 8 (#7277, merged at 11:41 UTC), because
bash.tschanged there. 5a PRs 5, 6 and 7 (#7319, #7329, #7338) merged before this PR. They change no source file of this PR. #7329 and #7338 removed neighboring allow-map lines, so merge commit d156ebc keeps the deletions of both sides. 5a PR 4 is stacked on this PR.Implementation
runtime/Runtime.ts: optionalreadonly location?: "local" | "devcontainer" | "remote", and two helpers.isLocalBaseRuntime(runtime)is true for "local" and "devcontainer".isRemoteRuntime(runtime)is true for "remote".LocalBaseRuntime.ts,DevcontainerRuntime.ts,RemoteRuntime.ts:locationis a prototype getter. Subclasses and theObject.create(...prototype)doubles of tests inherit it.MultiProjectRuntimeand plain object doubles leave it absent, andinstanceofis false for them too.tools/bash.ts:1041and:1064,tools/skillFileHint.ts,tools/agent_skill_read_file.ts: the helpers in place ofinstanceof.utils/ipAddress.ts: one more change (commit 4927205). The check of the impossiblenet.isIPanswer now uses the sharedasserthelper instead of a hand-writtenthrow, as the repo's assertion style asks. It changes no reachable path.eslint.config.mjs,.oxlintrc.jsonandscripts/noHostImports.test.ts: the 3 pairs leave the allow map and the frozen list.Validation
src/node/runtime/runtimeLocation.test.ts(new): a table of 12 runtime classes (the 8 runtime classes,MultiProjectRuntimeand the 3 test doubles oftools/testHelpers.ts). For each class,isLocalBaseRuntimeandisRemoteRuntimegive theinstanceofanswers on anObject.createinstance. A runtime withoutlocationtakes neither branch.bun testof the plan's list (src/node/runtime/,bash,skillFileHint,agent_skill_read_file) plusipAddress.test.tsandscripts/noHostImports.test.ts: 760 pass, 0 fail.make typecheck,make static-check,make lint-eslint.make test(host suite lock), on head b2593a1 (main b3911c8 merged in, before the merge of the current main): 27765 pass, 7 skip, 0 fail, and every isolated file passes.make typecheck,make static-check,make lint-eslintand the targeted list (760 pass) again. 5a PR 4 contains this tree, and its full gates ran on its final head.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$85.60