feat(agent): generic ACP runtime + omp backend - #126
Merged
Conversation
Generic ACP framing and session/update mapping with unit tests from live omp 17.1.1 spike fixtures. Runtime/backends land next.
OmpBackend + AcpAdapter (connection), TOOL_PRIORITY/TOOLS/min 17.1.0, ToolIcon/i18n/type union. Settings will list omp via detect_tools.
Global per-backend omp acp child; JSON-RPC demux to session subscribers; prompt uses long timeout (cancel aborts); permission replies are idempotent.
spawn_acp_turn + consumer stream ChatEvents; permission via AskRegistry; queue drain loops without recursive futures. inject no-ops for ACP tools and exposes acp_mcp_servers for session/new.
discover_slash reads ACP available_commands cache; tool-command bounce shuts down the ACP child; rewind cuts ~/.omp session jsonl before the matched user message (handles system-prompt prepend suffix match).
Connection-shaped omp has no resident stdin when idle; bus wakes and coordinator nudges were Noop. Treat ACP like per-turn for hidden_delivery and dispatch spawn_acp_turn from send_hidden_inner.
omp is not per_turn and not claude, so ensure_running_locked bailed with "unknown lead tool omp" before send could reach spawn_acp_turn. Early-return Ok(None) for ACP tools the same way as per-turn dialects.
Holding self.inner across request("initialize") deadlocked the first
connection (request_timeout needs the same lock). Also release POOL.clients
before ensure_connected. Live omp_acp_pong_live passes.
![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: 9742d68dfa
ℹ️ 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".
- permission pending set by request id (reuse-safe); always-cache by intent - drop locks before await; serialize first child create; pass command pin - stop_quiet cancels/unsubscribes ACP session - resubscribe when runtime lost route; MCP by thread kind (concierge=global only) - rewind: prepend-only suffix match, missing file errors, no symlink walk
Owner
Author
Review pass (Codex P1/P2) — addressed on
|
| Finding | Fix |
|---|---|
ensure_running unknown tool omp |
Early-return for is_acp_tool (2842573) |
| Permission id reuse (id:0) | pending_permission_ids insert/remove; always-cache keyed by intent |
| Stop/takeover leaves ACP running | stop_quiet cancel + unsubscribe before clear |
| Curator/concierge MCP wrong | MCP from thread.kind (concierge=global only, no bus) |
| Deadlock on initialize | drop(inner) before request("initialize") (601dd85) |
| Resubscribe after reconnect | Always check is_subscribed; re-subscribe if route lost |
| write_raw under mutex | Drop guard before any await in on_permission |
P2
| Finding | Fix |
|---|---|
| First-create race | CREATE_LOCK + recheck pool |
| Command pin | client(backend, program) from session-effective command |
| Rewind suffix too broad | Suffix only when body has system-prepend \n\n |
| Missing session file | Err instead of silent fresh session |
| Symlink walk | Skip symlinks + visited canonical dirs |
| Images | v1: text/files appendix only (same as non-claude); native ACP image blocks = follow-up |
Verification
cargo test --lib acp:: # 22 passed
cargo test --lib acp::runtime::tests::omp_acp_pong_live -- --ignored # ok
cargo test --lib hidden_delivery # 3 passed
Head: 82c6150
Merge origin/main (canonical Always keys #89) and wire ACP SessionEvent::Permission through ask::action_key + 6-arg request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 560968421c
ℹ️ 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".
- acp_emit_turn_end bails when stopped/reset_epoch advanced - track+cancel ACP permission asks on stop/skill-refresh - curator skips ACP-only default (omp) for next installed CLI - rewind system-prepend match requires prefix+separator+user - bound omp session walk; exclude omp from skill discovery UI - parse session/new|resume configOptions into SessionOpen meta
Owner
Author
Round 2 review (7 threads) — addressed on
|
| P | Finding | Fix |
|---|---|---|
| P1 | late overwrites stopped | epoch guard + early return |
| P1 | omp in curator exec | → next non-ACP CLI |
| P1 | permission ask survives stop | cancelled on stop/refresh |
| P2 | rewind blank-paragraph false positive | system-prepend = prefix+\n\n+user only |
| P2 | skill discovery pending forever | exclude omp |
| P2 | unbounded session walk | caps + skip artifact dirs |
| P2 | blank model in Session Info | parse configOptions on session open |
Local: cargo test --lib acp:: → 23 passed.
Later user prompts with blank paragraphs no longer steal rewind ordinals. Regression test covers first-vs-later prepend behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186de3ab0c
ℹ️ 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".
- recheck stopped/epoch immediately before session/prompt - rewind first-user is row order (seen_users), not match count - permission timeout bound to per-request generation (id:0 safe) - buffer session/update until subscribe (slash commands) - cap ACP tool output at 16k; curator honors command overrides - keep usage_update context; persist ACP thinking as reasoning
Owner
Author
Round 3 (8 threads) —
|
| P | Fix |
|---|---|
| P1 stop→prompt race | epoch/stop recheck before + inside prompt task |
| P2 first-user rewind | seen_users row order, not match count |
| P2 permission timeout | generation token per pending id |
| P2 pre-subscribe updates | buffer until subscribe, then drain |
| P2 tool output | cap_output 16k |
| P2 curator override | tool_command::command_for |
| P2 context gauge | keep usage_update.used, ignore billing totals |
| P2 thinking | last_reasoning + LeadStateInfo |
cargo test --lib acp:: 23 ok; rewind omp match tests ok.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73e8c5140
ℹ️ 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".
- buffer session/update only while opening; purge on unsubscribe - connection generation so stale read_loop cannot drop reconnect - fail/shutdown via proc_registry::reap (OMP tool descendants) - resume+load both fail → error (no stale native id) - seed Session Info MCP from injected specs; empty commands clear - tauri.conf withGlobalTauri for MCP bridge E2E Live verified via Tauri MCP: Oh My Pi lead issue #7, prompt→pong, model xai-oauth/grok-4.5 + context gauge 24k/500k.
Owner
Author
Live E2E (Tauri MCP) + round-4 fixes —
|
| Item | Fix |
|---|---|
| P1 unsub buffer replay | buffer only while opening_sessions; purge on unsubscribe |
| P1 process tree | proc_registry::reap on fail/shutdown |
| P2 connection gen | stale read_loop cannot drop reconnect |
| P2 load fail | no stale native id |
| P2 empty commands | clear slash cache |
| P2 Session Info MCP | seed from injected MCP specs |
cargo test --lib acp:: → 24 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cf087d004
ℹ️ 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".
- map agent_thought_chunk → Activity "thinking" with accumulated tail - clear thinking chip on first text/tool; frontend empty-name clears - gather_omp cwd skills; enable skill discovery for omp - seed+Init MCP/model after ACP open; lead_session_meta overlays engine - metaFromSnapshot/Init/fillMetaHoles set mcpAuthoritative so filtered weft_* internals show "no user MCP" instead of forever-pending
omp/grok often skips agent_thought_chunk; emit Activity thinking when session/prompt starts so the busy line is not a blank wait.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fdd2d07db
ℹ️ 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".
Drop injected/discovered skill subgroups for one deduped chip list. Stop filtering weft_bus/planner/global from Session Info so omp sessions show the MCP they actually have (status tagged "weft").
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 035b458d4c
ℹ️ 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".
- Consumer epoch (P1). `acp_consumer` read `reset_epoch` after being spawned, so a Stop/rewind/switch landing in that gap made it adopt the NEW generation and treat buffered events from the abandoned session as live. Captured at subscription time and passed in, so a route keeps the generation that created it. - Reconnect retired mid-handshake (P1, a regression from last round). The retry-reap I added fires when `pending` drains — which happens the moment `initialize` resolves on a reconnected handle, while no route exists yet. It shut down the client the caller was about to use, whose `session/new` then failed with `ACP not connected`. `ever_had_session` separates "done with its routes" from "has not opened one", and it lives in `may_retire` rather than at the call site so the predicate stays testable. - Queued-prompt errors (P1). The follow-up error branch skipped the drain and went straight to rollback, which clears `inner.current` without finalizing it — leaving `streaming` rows behind and letting consumer updates land on a reset turn. It now drains and finalizes in the same order the success path does. - Force-reset notice (P1). Emitted prose, now a stable token localized by the catalogs — the shape this PR already uses for `acp.permission_required`. I argued last round that Rust could not reach the catalogs; that was wrong, and the counter-example was in this same PR. The mapping moved to `src/lib/noticeTokens.ts` so both sides are pinned: a Rust test asserts the literal, a frontend test asserts every token resolves in BOTH catalogs. - Symlinked session root (P2). `is_dir()` follows a symlink and the other checks only reject one at the final component, so a redirected root put the whole scan outside the tree. The root is now checked with `symlink_metadata`, and — the actual boundary — the selected path must canonicalize to somewhere under the canonical root. That predicate is unreachable via the scans (they refuse symlinks first), so it is tested directly rather than left as an unverifiable guard. It does NOT cover hard links; `canonicalize` does not resolve them. - Slash-palette clearing (P2). `discoverSlash` returns an empty list for an authoritative empty palette AND for transient failures, and the call site stored it unconditionally — despite its own comment promising otherwise. Both the worker and lead paths now keep the existing list on empty. - Rewind ids (P2). Derived from the clock, so two concurrent OMP rewinds on one tick produced the same id and their truncating `fs::write`s overwrote each other. Uses the existing random `new_uuid_v4`; the clock helper is gone. Also serialized the four HOME-overriding rewind tests behind one lock — cargo runs tests on threads and they were racing each other's roots. Verification - `cargo test`: 1507 lib passed (was 1501), all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 154 passed (+3); `git diff --check` clean - Mutation-tested all four new guards — dropping the root symlink check, comparing path strings instead of real locations, drifting the notice token, and removing the never-opened-a-route guard all go RED Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab6718e32
ℹ️ 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".
Four of these sit on code this PR's earlier rounds touched.
- Pathless reads (P1). My previous round established "an empty set is Unknown,
never ReadOnly" for multi-path requests, then contradicted it one branch over
by tiering a pathless request from its verb. For a write that is right — the
verb alone establishes mutation. For a READ it is not: `classify_file("Read",
"")` answers `ReadOnly`, which a read-only session/issue grant releases with
no card, so a sparse request carrying its target in `title`/`content` could
read a credential unseen. Split into an explicit per-verb pathless verdict.
- Sparse grant identities (P1). `grant_identity` keyed on `rawInput` +
`locations` only, so a request that omits both — valid, and described in
`title`/`content` — collapsed to one identity, and an Always granted to the
first `execute` silently approved a different one. Every action-bearing field
now joins the key; absent stays distinguishable from present-but-null.
- Auto-approve raced teardown (P1). Only the waited-for-a-human branch
rechecked stop state before replying; the Always/Full branch has no ask to
cancel and rechecked nothing, so a Stop landing between the `reject_now`
sample and the `auto_decision` verdict reached the wire as an allow — queued
ahead of `session/cancel`, starting a tool after the user stopped the turn.
The check moved to the single path every branch leaves through, so "never
allow after teardown" is an invariant of the reply rather than something each
branch has to remember. It now covers `reset_epoch` too (rewind/switch).
- Shutdown raced client creation (P1). `shutdown_all` did not take
`CREATE_LOCK`, which `client()` holds from lookup through `ensure_connected`
and releases only after inserting — so the drain could run entirely between
spawn and insert, find an empty pool, and let the exit path terminate with an
untracked agent still running. Shutdown now serializes with creation and sets
a flag; a creation that finds it reaps its own child instead of inserting
into a pool nobody will drain again.
- Orphan after a failed open (P2). `resolve`'s retry declines while
`expecting_session` is still standing, and the failure paths cleared that
counter without retrying — leaving a route-less child the engine never
published, unreachable by any later stop/switch. Clearing the LAST
expectation now retries retirement; a successful open marks its route first,
so it is untouched.
Verification
- `cargo test`: 1508 lib passed, all integration suites, 0 failed
- `pnpm build` clean; `pnpm test` 154 passed; `git diff --check` clean
- Mutation-tested both new predicates — restoring the verb fallback for
pathless reads and narrowing the grant identity back to two fields both
go RED
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8438de2d3
ℹ️ 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".
- Both path sources (P1). `tool_paths` returned structured `locations` and only fell back to `rawInput` — treating them as alternatives. A request could put an ordinary file in `locations` and a credential in `rawInput.path`, be tiered `ReadOnly` off the ordinary one, and be released by a read-only grant with no card. Both sources are collected now, de-duplicated, worst tier wins. - OMP in automatic fallback (P2). Adding OMP to `candidate_list` so manual pins could resolve also made it reachable from the legacy-fallback arm, which contradicts the closed Codex/Claude automatic pool and could route curator work to an engine the curator refuses. Excluded there exactly as OpenCode is; a test pins both halves of the contract (manual pin resolves, automatic fallback never selects it). - Force-reset notice kind (P2). Posted via `ask_human`, so Needs-you rendered an answer box whose reply would inject a stray bus message into the session that was just reset. It is a persistent, non-answerable notice — `notify_human_action_required`. The IM bridge also forwarded the raw token, so `bus::notice_text` now resolves tokens for consumers that cannot reach the webview catalogs (IM sends on its own fixed locale). - Post-switch rewind (P2): PINNED AS A LIMIT, NOT FIXED. The report is accurate — a rewind of the first message after a switch to OMP always fails. The dispatched text is `system + context_digest + user` while the row holds only `user`, and the digest is kept out of the DB deliberately (PR #139: it can carry paths and pasted secrets), so nothing matches. Not fixed here because the failure is already the safe outcome — `fork_omp_at` runs before any stop/truncate, so the session is left fully intact — and the available "fix" is to accept any trailing segment after a blank line, which is precisely the mis-cut the previous round tightened this matcher to prevent. A real fix must anchor the OMP-native history boundary: the jsonl begins at the switch while the DB ordinal counts the whole session, so the two disagree about more than the digest. That is cross-engine rewind semantics, not a matcher tweak. A test pins the current refusal so a future loosening cannot turn it into a wrong cut silently. Verification - `cargo test`: 1514 lib passed, all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 154 passed; `git diff --check` clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df423afaa3
ℹ️ 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".
Two of the P1s are residue from this PR's own earlier fixes. - Abandoned route left registered (P1). The stale-native-id clear I added two rounds ago cleared the engine and DB ids but left the runtime `SessionRoute` and its consumer in place, so late text/tool/permission events kept arriving in this engine after the retry opened a different session — and the surviving route pinned the pooled client against retirement. It now cancels and unsubscribes first, like every other abandonment path. - Teardown admitted work it would then discard (P1). Bumping `reset_epoch` early closed one direction of this race: work reserved BEFORE a teardown can no longer land after it. The other direction stayed open — a send arriving DURING the teardown captures the already-bumped epoch, passes every check, and has its queued work replaced by `TurnState::default()` when the teardown reacquires the lock. The epoch cannot express "mid-teardown", so that window is now an explicit `tearing_down` flag that `send_reservation_valid` refuses. - Permission cards outlived a dead prompt (P1). On a transport failure the consumer stays blocked on the `AskRegistry` receiver, so nothing retired the card: it remained actionable for the full hour, and Always/Full would persist a grant for a request whose native session was already gone. Both prompt failure paths — first and queued — now cancel the tracked asks. - Writes labelled as reads (P2). `tool_name_from_raw` excluded `write` from its mutation kinds, so the timeline gave a file write a read's name and icon while the permission layer classified the same call as a mutation. - Truncated scan reported as missing (P2). A bucket past the 4096 scan cap made `find_omp_session_file` answer `Ok(None)`, which surfaces as `omp_session_not_found` — telling the user their session is gone when the scan merely stopped looking. Truncation is now tracked through both the preferred scan and the fallback walk and reported as its own error. Reaching the HIT cap is deliberately NOT truncation: the session was found. Verification - `cargo test`: 1517 lib passed (was 1514), all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 154 passed; `git diff --check` clean - Mutation-tested the three new guards — dropping the teardown gate, collapsing `write` back into `read`, and removing the truncation error all go RED Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab038af647
ℹ️ 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".
- Notice copy duplicated across languages AND runtimes (P1). Last round I added
a Rust table so the IM bridge could render the force-reset token, which left
the same two sentences transcribed in `bus/notice_text.rs` and in
`src/i18n/{en,zh}.ts` — a catalog edit would silently give remote and in-app
users different text. Now `src/i18n/notices.json` is the single source: the TS
catalogs import it, Rust bakes it in with `include_str!`, and a test on each
side asserts the rendered copy equals the shared file. A malformed file
degrades to empty rather than panicking, so a Rust test asserts it parses and
carries both languages — otherwise that degradation would ship as raw tokens.
- Process-wide creation lock (P2). `CREATE_LOCK` was held across
`ensure_connected`, whose handshake waits up to 60s, so one slow or broken
command pin blocked the first send of every OTHER `(backend, program)` key —
defeating the isolation the pool key exists to provide. Creation and
retirement now take a per-key lock under a read guard; only `shutdown_all`
takes the write guard, which preserves the drain-vs-create serialization
added last round.
Verification
- `cargo test`: 1520 lib passed (was 1517), all integration suites, 0 failed
- `pnpm build` clean; `pnpm test` 155 passed (+1); `git diff --check` clean
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dd2b7ef4c
ℹ️ 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".
… latency - Freeze recovery lacked both guards the force-reset path got (P1 x2). I fixed one path and left its twin: freeze recovery neither reserved the engine (`tearing_down`) across its cancel/unsubscribe/finalize/marker/native-id cleanup, nor retired a client whose cancel was ignored. So a send admitted mid-cleanup could resume the session being abandoned, and the next send could retrieve the same pooled process still running the frozen prompt. Both mirrored, plus `native_id` is TAKEN rather than cloned, as force-reset does. - Notice copy authored outside the catalogs (P1). Last round's `notices.json` made a third localization source, which is what AGENTS.md forbids. The catalogs are authoritative again; `scripts/gen-notice-copy.mjs` mirrors the listed tokens into `notices.generated.json` for the IM bridge to render, and `noticeCopy.test.ts` fails if the mirror drifts. - Truncated-scan code reached the UI raw (P2). `omp_session_scan_truncated` is new this PR, and `RewindDialog` already had the mapping table it belonged in — I added the code without its entry. Distinct copy from "not found" on purpose: the session likely exists, the scan stopped at its cap. - Exit blocked on in-flight handshakes (P2, from last round's fix). The write guard I added made `ExitRequested` wait out a 60s handshake — a hung-looking desktop app. The mutual exclusion moved to the pool lock itself: `client()` checks the shutdown flag and inserts under it, `shutdown_all` sets the flag and drains under it. A racing creation either lands before the drain or reaps its own child, so no untracked survivor, and exit never waits. Verification - `cargo test`: 1521 lib passed, all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 155 passed; `git diff --check` clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fde95eab48
ℹ️ 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".
Both of these are mine — one a regression from the previous round, one the same omission for the third time. - Forced reset did not reserve the engine (P1). `stop_quiet` got `tearing_down`, then freeze recovery got it, and `force_acp_turn_reset` — the third path with the identical shape — still did not. It matters most here: `reset_frozen_appserver_turn` clears `turn.busy` AND `interrupting`, so a send arriving during the cancel/unsubscribe/retire/DB-clear awaits captures the already-bumped epoch, is admitted onto a fresh session, and then has its native id cleared and an idle state emitted over it by the older reset. - Pooled handle escaped the shutdown check (P1, from last round). Moving the mutual exclusion onto the pool lock covered the fresh-handle path only. An EXISTING handle returns early, and `shutdown_all` can drain it and clear its `inner` between the lookup and `ensure_connected` — which then spawns a replacement child that the early return hands back without re-inserting, leaving a child nothing tracks and an exit path already past. That path now re-checks under the pool lock and re-inserts if the drain removed it. Verification - `cargo test`: 1521 lib passed, all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 155 passed; `git diff --check` clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd25d3f36
ℹ️ 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 fourth gap in the same guard, and the one that made the previous three incomplete rather than merely partial. `send_reservation_valid` checks `tearing_down`, but hidden delivery — a bus wake, a plumbing send — never passes through it, and `spawn_acp_turn` did not check it either. So all three teardown paths that take the reservation (`stop_quiet`, freeze recovery, forced reset) were invisible to a bus wake: it started a fresh native session mid-cleanup, and the older reset then cleared that session's native id and emitted idle over its active turn. Enforced through one predicate used by both entry points, so they cannot drift, and testable without an `AppHandle`. A skipped bus wake is not lost — the coordinator re-delivers on its next sweep — whereas one admitted mid-teardown has its turn reset out from under it. `spawn_acp_turn` checks too, as defence in depth: it is the single gate every ACP turn passes. Verification - `cargo test`: 1522 lib passed, all integration suites, 0 failed - `pnpm test` 155 passed; `git diff --check` clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79aacbdc61
ℹ️ 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".
Both fixes are on the sparse-request shape ACP permits: no `rawInput`, no
`locations`, the action described only in `title`/`content`.
- Empty card body (P1). The detail builder read `rawInput` and paths only, so
such a request produced an EMPTY body — leaving the human to blind-approve or
refuse, and both are product failures. `content` is used as a fallback (not an
addition: for an ordinary request it would restate `rawInput` as `{"command":
"ls"}` plus `$ ls`), so ordinary cards are unchanged.
This one is provable from inside the module rather than from an agent that
emits the shape: `grant_identity` ALREADY treats `content` as action-bearing,
which it must, or two different content-only actions share one Always grant.
The two halves of the authorization decision have to agree on what the action
is — one cannot key grants on a field the other refuses to display.
- Untranslatable title (P2). An absent title defaulted to the literal `"tool"`,
which is non-empty, so the branch that emits `acp.permission_required` — the
token `ConfirmationCard` maps through both catalogs — was unreachable. Every
locale showed `tool`. The catalog entry and the mapping already existed; only
the placeholder stood in the way.
Scoped deliberately: the freeze-marker ordering and the PowerShell resume
quoting from the same review round are follow-ups. The first needs a DB write to
fail (where the user's problem is not the rewind anchor), and the second is for
Windows, which `release.yml` does not build.
Verification
- `cargo test`: 1524 lib passed, all integration suites, 0 failed
- `pnpm build` clean; `pnpm test` 155 passed; `git diff --check` clean
- Mutation-tested: ignoring `content`, restoring the `"tool"` literal, and
appending `content` instead of falling back to it all go RED
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9b20fab42
ℹ️ 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".
`interrupt()` sets `interrupting` WITHOUT bumping `reset_epoch` — deliberately, so a queued message survives interrupting the current turn — and it cancels whatever prompt is live at that moment. A Stop landing after `on_turn_end` has promoted a queued item but before it is dispatched therefore cancels NOTHING: the old prompt has ended, the new one has not started. The dispatch guard checked only the epoch, which still matched, so the queued prompt ran anyway — executing tools — until it finished on its own or the 8s forced reset fired. Stop not stopping is the one behaviour a user cannot be asked to tolerate, so this is fixed rather than deferred with the other two findings from this round. The guard is now a named predicate covering every admission condition (`stopped`, `interrupting`, `tearing_down`, epoch), matching the other three admission points rather than being a bespoke epoch comparison — the drift between them is what produced this gap. The two connection-lifecycle findings from the same round (untracked in-flight handles at exit, frames not fenced to their connection generation) are recorded as follow-ups: both need connection-layer rework, both have narrow triggers, and that layer has produced the most regressions across these rounds. Verification - `cargo test`: 1525 lib passed, all integration suites, 0 failed - `pnpm test` 155 passed; `git diff --check` clean - Mutation-tested: reducing the guard back to the epoch comparison goes RED Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`rust-test (ubuntu-latest)` failed on this branch with a `Broken pipe` panic at `hook_test_support.rs:51` in `codex_hook_ignores_a_non_loopback_route_without _posting`, while macOS passed and 30 consecutive local runs passed. Not a flake in the environment — a race the test drives itself. Several of these hooks exit WITHOUT reading stdin, and that one is named for exactly that behaviour: a non-loopback route returns without posting. So `write_all(...) .unwrap()` races a legitimate early exit, and loses on a loaded runner. The write is now allowed to fail. This cannot mask a real defect: if a script that SHOULD consume stdin stops doing so, it never receives the payload, and the stdout and exit-code assertions below — which are what every one of these tests actually checks — fail as they should. The line predates this branch (PR #141); this PR does not otherwise touch the file. Fixed here rather than separately because it blocks this merge and the cause was diagnosable from the failure. Verification: `cargo test` 1525 lib passed, all integration suites, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19221ec3a5
ℹ️ 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".
| let thread_id = eng.lock().await.thread_id; | ||
| let session_id = eng.lock().await.session_id; | ||
| mark_queued_delivered(&app, &db, thread_id, session_id, &msg).await; | ||
| match client.prompt(&sid, &msg.text, &msg.images).await { |
There was a problem hiding this comment.
When Stop lands after the admissibility check releases the engine lock but before this RPC is enqueued, interrupt() sends session/cancel while no prompt is active and this queued prompt then starts anyway, allowing tools to run until completion or the delayed force reset. Fresh evidence after the earlier fix is that the new predicate is released before both mark_queued_delivered and client.prompt, so it remains a TOCTOU guard; serialize prompt arming with Stop or reissue cancellation after the request is registered.
AGENTS.md reference: AGENTS.md:L58-L58
Useful? React with 👍 / 👎.
Comment on lines
+5461
to
+5464
| if let Err(err) = clear_native_id(&db, session_id, thread_id).await { | ||
| eprintln!( | ||
| "[weft] acp force reset: failed to clear native id for thread {thread_id}: {err}" | ||
| ); |
There was a problem hiding this comment.
When this database write fails, such as on a locked or full SQLite database, the force-reset path has already removed the in-memory native ID but merely logs the failure and makes the engine sendable again. The database therefore still names the abandoned session; a restart before a successful new-session persistence resumes that wedged session despite the user's forced reset. Keep teardown reserved until the clear succeeds, or preserve a retryable tombstone so the durable ID cannot remain stale.
AGENTS.md reference: AGENTS.md:L58-L58
Useful? React with 👍 / 👎.
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
2 participants
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
src-tauri/src/acp/) — JSON-RPC 2.0 framing, session/update → ChatEvent map, permission option mapping, global multiplexed child pool.AcpBackend(connection-shapedAcpAdapter), detect/Settings/UI identity, min version 17.1.0, priority last.spawn_acp_turn+ consumer streams text/tools/usage; AskRegistry forsession/request_permission; MCP viasession/newHTTP specs; empty inject for ACP tools.omp --resume, canRewind + jsonl cut-before rewind (ACP fork is full-history only; S0 proved hand-cut load).Design notes
docs/(not in this PR).src-tauri/tests/fixtures/acp/from live omp 17.1.1.Verification
Every new guard in the review rounds below was mutation-tested: reverting the
guard turns its test RED, and the files are restored byte-exact afterwards.
Merge + review rounds (post-handoff)
Merged 59 commits of main (
#150PR/MR state machine,#146/#153askallowlist,
#151quota failover,#149engine routing,#152auto-merge).The previous merge had dropped main's
teardown_for_switchrewrite instead ofreconciling it:
stop_quiethad independently grown a fifth element on eachside (
acp_askshere,was_busyon main), leaving a 4-tuple destructuringagainst a 5-tuple return — the compile failure that had rust-test red on both
platforms. It returns a named
StopQuietOutcomenow, so a future field cannotsilently invalidate a call site.
Twelve Codex findings across two rounds, all real, all fixed — highlights:
intent_keyisa lossy grant-cache token (
execute:bash), so comparing it to bare verbsnever matched and shell commands scored as generic tool calls. Now classified
from the
toolCallitself via aPermissionIntentmapped exhaustively.rawInput, which is identical (often empty) for two edits whose paths liveonly in
toolCall.locations. Keyed on a length-prefixed identity coveringevery location.
critical section that takes the ACP client, and a wedged session is abandoned
(id cleared, client retired under an explicit policy) even when a shared
pooled client legitimately survives for another route.
shutdown_allhad no production caller;wired into
RunEvent::ExitRequestedand awaited.{system}\n\n{user}prependwas guessed as "before the last blank line"; the real system prompt is now
stripped exactly, and a changed prompt declines rather than mis-cutting.
Follow-ups
TO omp fails (
omp_user_not_found). The dispatched text carries a contextdigest that is deliberately absent from the persisted row, and more
fundamentally the omp jsonl begins at the switch while the DB ordinal counts
the whole session — the two disagree about the history range. The failure is
safe (the fork runs before any stop/truncate, so the session stays intact)
and is pinned by a test so a future matcher loosening cannot turn it into a
silent wrong cut.
apply_to_existingduring a busy turn setspending_command_refresh, but thequeue drain dispatches straight to
client.prompt, bypassingsend— theonly path that bounces. Already-queued messages therefore finish on the old
executable. Fixing it means carrying the deferred refresh through the idle
transition without disturbing
bus_read_pos(the bus wake's position relativeto the queue), which
on_turn_endadvances in the same step.SessionInfoPanelmergesmeta.engineSkillsfor omp, andgather_ompscans.agents/.claude, soWeft-materialized built-ins can be listed for a session with an empty
workspace selection. Display-only — nothing keys off that list — and the right
fix is an exhaustive tool-to-policy mapping rather than another special case.
restart at 1 on every spawn, and per-frame dispatch (
resolve,on_session_update,on_permission) does not check the reader'sconnection_gen— so a late response from a replaced child can satisfy a newconnection's
initializeor prompt. EOF cleanup IS fenced; per-frame dispatchis not. Relatedly, a handle that is spawned but still awaiting
initializeisnot yet in
POOL, so the exit drain cannot see or cancel it; that wants anin-flight registry rather than another relocated check.
read_loopclearsInneronly whenconnection_genmatches; the writer /flush-timeout failure path carries no generation, so a stale task can reap a
replacement child and fail its initialization.
connect/prompt races return hardcoded sentences that
notifySendFailedshowsverbatim. They want the stable-code-plus-catalog treatment
acp_session_open_failedalready has.stamp_freeze_markerruns, so a FAILED marker write (locked/full DB) leaves no witness to restore
it from. Reachable only when the DB cannot be written at all.
src/lib/resume.ts). A quoted executable needsthe call operator (
&) to run. Belongs with Windows support —release.ymlbuilds macOS only, so there is no way to test it today.
one thing not covered by the suites above.
acp_consumer, which needs a liveAppHandle+Db.