Repository navigation
feat: run ompweb's own omp build over its own channel instead of RPC - #473
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 203 files, which is 103 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
Start of a custom omp entry that would host ompweb sessions over a local socket. Paused: omp collab already covers most of the design, and the direction (collab guest, custom build or hybrid) is pending.
runner/ is ompweb's own omp build on pinned @oh-my-pi/pi-coding-agent 18.19.0. src/cli.ts stands in for upstream cli.ts as the process entry and swaps the launch command for one whose session factory (src/session.ts) shares every session through the host's loopback collab relay (runtime overrides only) and attaches the NDJSON control socket (src/control.ts: get_state, set_model). src/relay.ts is the loopback relay; scripts/ holds the guest and host probes. Verified: --smoke-test ok; stock and runner TUIs joined by the guest probe; a guest prompt raised an ask that the guest answered; set_model over the control socket switched the TUI model and reached the guest as entry+state.
Drop the collab relay, overrides and guest probe. The session factory now connects src/channel.ts: hello with the session file, every AgentSession event, commands (get_state, set_model, prompt, abort) against the same AgentSession the TUI drives, and dialogs raced between the TUI and the host by wrapping the tool UI context (first answer wins, the other side closes). Verified: --smoke-test ok; a host-sent prompt opened an ask whose whole HTML form (checked state included) reached the host as formDialog; the host answer closed the TUI form; a TUI answer sent ui_end to the host.
src/commands/ holds one CommandTable and FrameTable per group (prompting, model, session, tools, agents) behind a shared CommandContext; the channel dispatches cmd frames by op and other host frames by t.
Agents were cut off mid-task; this preserves their partial edits (runner headless/events/correlation, ompweb process/daemon/manager/auth, TUI attach socket, release/smoke), plus the spec and DESIGN/AGENTS updates. Not verified as a whole.
Skill diagnostics came from an unmerged omp PR that the pinned runner lacks; the feature, its setting toggle and docs are removed. Root tsconfig excludes runner/, which has its own. E2E (server+daemon+compiled runner): prompt, whole ask form with pre-checked default, set_model+edit_message and TUI attach all passed.
…layout Fake omp children in host tests speak hello/cmd/res/event instead of RPC; RemoteRpcTransport tags link samples from hello/session frames. The web ask form adds the automatic Other choice per radio/checkbox group, as omp does.
Fake hosts and workers in agent-tools, api-contract, message-outbox-review, outbox-handoff, session-resume and session-routes speak hello/cmd/res/event; the rename-during-edit test holds a native edit_message command. The late second RPC response case is gone: the channel reports it as prompt_error.
Both handlers existed but were missing from the command table, so the daemon's event filter and the outbox status probe got unknown-command errors.
The root suite ignores runner/** (its deps live in runner/node_modules); runner gets a typecheck and bun test job on PRs that touch it. zh-TW, added on main, still carried the removed skill-diagnostics keys.
…p dirs npm test lists the app directories instead of ignoring runner/** (the ignore glob missed Windows paths). Smoke needs a real runner: CI builds it first, and packed-install smokes pass it with --omp until the per-platform runner packages are published.
…ing HOME omp's daemon broker, LSP mux and embedding server detach and outlive the session by design; on Windows they keep the throwaway HOME locked.
…ng it The server closes the peer dialog without waiting for omp, so the child's answer record can land after the cancel frame (seen on Windows CI).
A freshly copied runner binary's first start on Windows waits on the antivirus scan; the 10 s probe reported omp as missing for the whole daemon session. Smoke waits up to 60 s for the link.
The runner cannot add clientMessageId to omp's entries, so it journals
{clientMessageId, entryId} in a custom entry. The session reader now takes
the id from either source, on the active branch only, for outbox
reconciliation, full context and history pages.
Runner: headless mode boots through runRootCommand's runSessionHost hook (upstream PR pending; carried as a Bun patch of the exact diff) instead of swapping omp's rpc-ui mode with a build plugin, and no runner module imports omp's RPC helpers any more (prompt results, goals, session events and settle, message paging, host tools/URIs, btw, dictation, live, subagents are ported onto omp's non-RPC modules). ompweb: the server passes the runner's ui_request/ui_end/ui frames through unchanged and the browser renders each omp dialog method from its own arguments, answering ui_response with the exact omp return value; the extension_ui_* translation is gone.
Daemon: unowned res frames are dropped and startup configure is
fire-and-forget (a slow extension startup no longer resolves the web's
commands with stray replies); a TUI runner cannot attach to a session that
already has a live channel ({t:"refused"}); the host link moves to protocol
v10 (MIN = VERSION), deleting spawnLegacy, legacy channels and every older
version gate; a restarted server sees each event once and in order
(continuous rejoin); the host socket is created under umask 0177; numeric
string dialog ids are normalised; dead capability and lifecycle plumbing
removed.
Runner: only interactive TUIs dial the host, redialling stops after a
refusal, no dialling on Windows yet, open dialogs are re-sent after a
redial and ui_end follows a failed local dialog; tests now kill the
reviewer's mutants and cover edit_message.
Browser: agent forms with controls named like HTMLFormElement members
still submit. Docs: spec, README, AGENTS.md and DESIGN.md describe the
runner and its channel.
Fake daemons serve v10 open instead of the removed spawn op, hellos and welcomes use LINK_PROTOCOL_VERSION, and the removed ready capability is gone from expectations. Tests of below-v10 join blocks are deleted (such a host is refused at link time); session-resume asserts that v10 hosts own their runs. clusterJoinBlock drops its dead protocol parameter.
…ne redial test On a slow socket (Windows' small loopback buffers) the resend outlasted the test's ack hold: the released acks cover the lines main got before the drop, so the daemon rightly skipped the rest of the resend and the test waited for lines that would never come again. Hold acks until a full window is resent.
…e it listens omp children spawned during the pending listen() (restored sessions at startup) inherited umask 0177, so every file and directory their tools created was 0600.
ompweb preflight starts the session omp as a new host session would (same resolver and spawn, --no-session, scratch cwd) and waits up to 30 s for its hello. The host updater and the web self-update worker run the candidate's preflight in the live environment before stopping anything and abort on failure. An npm ompweb host now resolves the runner from its own package.
The systemd and launchd installers wrote OMP_WEB_OMP_BIN=$(which omp) into the service environment. That override wins over ompweb's own ompweb-omp, and a stock omp cannot run ompweb's sessions, so every session on such a machine failed. Only an explicit OMP_WEB_OMP_BIN is written now; the install test puts a stock omp on PATH and checks neither the env file nor the unit names it.
use-omp-build.ts unpacks into vendor.new/ and swaps it in only once index.json holds every required package, so a missing leaf or a failed download leaves vendor/ matching bun.lock. Overrides are replaced, not merged, so a package the new release dropped keeps no override, and the download is retried once.
ompweb preflight can take 30 s for the hello, about 7 s to stop omp and 30 s for the version probe, over the 60 s both updaters allowed it; a slow first start failed an update the check would have passed. Both now allow 120 s.
No more runner npm packages. A release attaches ompweb-omp-<platform> and ompweb-omp.sha256; lib/omp/runner-fetch.ts downloads this version's asset, checks it against that listing, and renames it into place executable: beside a compiled ompweb, else into a per-version cache under the agent dir (omp-web/omp/<version>/), which resolveOmpBin now probes instead of @kahme247/ompweb-omp-<platform>. OMP_WEB_OMP_BIN still wins, and a source checkout still uses its own runner build. - `ompweb fetch-omp` fetches it when missing. The host updater and the web self-update run the candidate's fetch-omp (15 min) before its preflight, so the candidate is checked with the runner it will run. - The web server and `ompweb host` fetch it at start, in the background, when it is missing. - A failed fetch names the asset URL and OMP_WEB_OMP_BIN. The release repository is read from package.json's `repository` only (lib/release-repo.ts): the runner download, the host binary download (passed to the self-contained worker in its descriptor), the candidate protocol lookup and the binary update link.
… archives publish.yml builds the archives --no-runner, attaches the five runners and their ompweb-omp.sha256 to the release, and publishes the GitHub release before the npm package, whose installs fetch their runner from it. The per-platform runner npm packages and the optionalDependencies pinning are gone. The binary smoke gets the runner with --omp, since a draft release serves no assets. Prereleases keep the runner in their archives: their binaries carry the version of a release that is not theirs.
…ith the runner that ships
…ed one manual update
omp's ask tool calls ui.formDialog whenever the UI provides it, so the
runner's ask arrives as formDialog. The composer stayed visible for it,
and a steer sent from there interrupted the ask ("Ask interrupted by
message").
omp's ask tool aborts when a user steer arrives. A steer already in the outbox now waits until the ask is answered, and send_to_session refuses a steer into a session waiting on an ask.
The wrapper now reports when a session's last open ask closes (answered, cancelled, expired or its child gone), and the outbox delivers a steer it held for that ask at once, instead of on its retry timer.
A steer would interrupt the target's open ask. send_to_session now delivers it as a follow-up and reports that delivery in its result, instead of refusing it.
…etween omp builds
andrebrait
marked this pull request as ready for review
November 13, 2026 17:35
Collaborator
Author
|
@codex review |
Codex Review Summary: ✅ CompletedThis 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: bf50397b62
ℹ️ 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".
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| writeChannel = process.stdout.write.bind(process.stdout); | ||
| // Extensions and libraries may print; on stdout that would corrupt the channel. | ||
| process.stdout.write = process.stderr.write.bind(process.stderr); | ||
| console.log = console.info = console.debug = console.error; |
There was a problem hiding this comment.
Test comment from Kilo review
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: Oh wait, the channel architecture is actually clean. NDJSON over stdio/Unix socket with proper protocol versioning and redial logic. The new 💀 Worst part: The 📊 Overall: Like a first pancake — the shape is wrong but the ingredients are there. The runner/channel split is sound; the new peer-outcome plumbing is clean; just watch the edge cases (and maybe give Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit bf50397)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bf50397)Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: Oh wait, the channel architecture is actually clean. NDJSON over stdio/Unix socket with proper protocol versioning and redial logic. I need to sit down. 💀 Worst part: The 📊 Overall: Like a first pancake — the shape is wrong but the ingredients are there. The runner/channel split is sound; just watch the edge cases. Files Reviewed (203 files)Key files reviewed: Reviewed by free · Input: 98.7K · Output: 5.2K · Cached: 289.3K |
…entity A non-owning session's first-write relocation sent the new session frame before the persistence notice, so the daemon dropped the original id's aliases and a later open by that id started a second child.
…fications A prompt's prompt_result/prompt_error went only to the submitting server, so the elected notifier saw session_settled without the failure and reported a false Completed. Peers now get peer_prompt_outcome (no command id), which only feeds their notification detector.
Collaborator
Author
|
This is intentional. In headless mode, stdout is the NDJSON channel to the host, and any stray line there would corrupt it. |
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
ompweb stops driving omp through
omp --mode rpc-uiand runs its own omp build,ompweb-omp(runner/). Each session starts through omp's own startup (settings, logins,--resume,--model,--tools, extensions, MCP) and is connected toompweb hostover ompweb's own NDJSON channel. The host receives the sameAgentSessionevents and calls the same session methods omp's TUI does, so RPC's gaps go away: the wholeaskform with its pre-checked choices, missing hooks, negotiation and chunking quirks, and the session-edit extension. Design:docs/specs/2026-11-12-omp-runner-design.md.runner/): headless mode (--headless, channel on stdio), a Unix socket at$XDG_RUNTIME_DIR/ompweb/host.sockfor TUI-started sessions, command groups, an event bridge with deltamessage_update, and client-message correlation. Built per platform byrunner/scripts/build-binary.ts.ompweb-dev/oh-my-pi, the upstream release plus carried PRs, including feat(coding-agent): let embedders run a started session in their own host can1357/oh-my-pi#19075 (therunSessionHosthook the runner boots through). Its packages are vendored from a pinned fork release (runner/omp-build.json,runner/scripts/use-omp-build.ts); there is no npm pin and no patch. Every (pre)release re-pins to the newest green fork build, or fails. Design:docs/specs/2026-11-12-omp-build-in-ompweb-design.md.ompweb-omp-<platform>(linux-x64, linux-arm64, darwin-x64, darwin-arm64, windows-x64) andompweb-omp.sha256; the archives no longer contain it. Every install, binary or npm/npx, fetches the runner of its exact version from its release and checks the SHA-256 (ompweb fetch-omp): at start when it is missing, and in both updaters before the preflight.OMP_WEB_OMP_BINoverrides it, for offline machines.ompweb preflight: starts the session omp exactly as the host would and requires its hello. The in-app host updater and the web self-update run it on the candidate and keep the current install when it fails. The service installers no longer write a stockompfound onPATHinto the service environment.askis rendered natively, including omp's automatic "Other" choice, andedit_messageis a native command. The daemon adopts TUI-started runner sessions from its socket. The omp row in Settings → Hosts is read-only ("Updated with ompweb").rpc_chunk,set_ask_dialog, the session-edit extension, skill startup notices and "View skill diagnostics".Upgrade
ompweb hostonce by hand: a daemon from before this change is refused, so Settings cannot update it. npm hosts: restartompweb host(npx@latest, ornpm install -gfirst); binary hosts: replace the directory. That start fetches the runner; later updates work from Settings.OMP_WEB_OMP_BINwhere it points at a plainomp: a plain omp has no--headlessmode, and its sessions fail with "omp did not say hello".ompweb preflightreports this before an upgrade switches. Reinstall a systemd or launchd service whose environment still names one.Verification
ompweb preflighton the built binary, and the updater's own size checks on every built archive).npm packinstall each fetched the runner (same SHA-256 as the list) and passedompweb preflight.tscclean;bun test27/27. The compiled binary prints the fork build's version, passes--smoke-test, and its headless mode says hello.ask, model switching, message editing, cluster configuration, and TUI-started sessions shown live in the web.Known gaps
set_modelno longer saves the default model role (RPC did).