fix(acp): make advertised mode changes safe - #3914
Closed
probepark wants to merge 13 commits into
Closed
Conversation
Add durable register/read/ack Codex handoff MCP tools with wake events keyed by work-unit:event-seq for cross-process idempotent creation (exclusive create, EEXIST loser), full durable schema validation, and bounded allowlisted registration responses. Publisher enforces unix/loopback-only endpoints, token-file-only references (no token material persisted), thread/resume then idle-only turn/start with deterministic clientUserMessageId, pending fallback for active threads, and prompts that never include final_response text. Wake pipeline: per-thread FIFO drains across sibling work units, startup drain of persisted pending/failed wakes, durable failure diagnostics in codex-wake-errors.log, and production question.opened events emitted once per canonical pending question. UserPromptSubmit persists exact $gjc-mcp-delegate-flow host context without activating any workflow; four bundled workflows unchanged. Includes focused restart/duplicate/security/fake-transport tests, red-team suite, mutation-validated assertions, and coordinator stdio smoke evidence in artifacts/.
…real app-server transport
Delegate creation (gjc_delegate_plan/execute/team) now consumes persisted
$gjc-mcp-delegate-flow host context and auto-registers each new session
to the source Codex thread: N:1 thread-to-work-unit registrations with
validated origin metadata (delegate session/turn, Codex thread/turn,
host session, delegation id, workflow), exclusive hard-link publication
that never overwrites, fresh-first origin-free fallback with stale and
cross-host-context ambiguity failing closed via durable diagnostics, and
codex_handoff {auto_bound, thread_id} in delegate responses. Auto-bind
failures never break delegation.
Real Codex app-server compatibility (verified read-only against the
installed codex-cli 0.144.5 socket): WebSocket upgrade over unix or
loopback TCP with bracketed IPv6 Host, token-file content sent only as
an Authorization Bearer upgrade header (never RPC params), bounded
establishment deadline with end/close rejection, masked frames with
ping/pong and id-matched responses, initialize -> initialized ->
thread/resume (thread.status.type idle-only gate) -> turn/start with
schema-shaped input [{type:'text',text,text_elements:[]}]. Wake prompts
carry identifiers only; no event summary or final_response text.
Wake drains stop on first transport failure, keep pending wakes for the
next trigger or startup drain, and stay serialized per Codex thread.
Delegate-flow prompts never activate workflow skills; host-context
persistence is best-effort with bounded byte caps and validated
enumeration. Hermes list_questions/submit_question_answer remains the
only question channel; exactly four workflows preserved.
…egate auto-bind When Codex is the MCP host, no ambient .gjc host-context exists (that file is written only by GJC's own UserPromptSubmit hook), so implicit inference cannot identify the host Codex session. Add the smallest explicit correlation input: an optional codex_host_session_id on gjc_delegate_plan/execute/team carrying the session_id previously passed to gjc_coordinator_register_codex_handoff. A valid explicit id resolves the source registration deterministically via readCodexHandoff and skips ambient enumeration entirely; missing, malformed, or corrupt sources record a durable codex_handoff_explicit_source_missing diagnostic and return auto_bound:false without failing delegation. Absent parameter keeps the ambient host-context fallback for the GJC-host case. Installed plugin contract (generated commands + gjc-delegation skill) now documents the correlation flow: register the app-server handoff, then pass the same session_id on delegate calls so new GJC sessions wake the owning Codex thread on completion/questions.
…us write itself fails Port the corrective patch from feat/codex-gjc-resume-bridge (d57dfae): publishRecordedCodexWake now appends a bounded per-wake diagnostic line (wake=<key> error=<code>) to codex-wake-errors.log before recording the failed status, and guards the updateCodexWakeEvent call so a failing durable write cannot mask the original transport error — the update failure is also appended as a diagnostic. The drain still stops on the first failed outcome and never storms the endpoint.
…nsport RED proof
Regenerated protocol bindings from the installed CLI
(codex app-server generate-ts, codex-cli 0.144.5) and hardened the
WebSocket fixture to enforce them: raw JSONL (non-WebSocket) clients are
destroyed before any exchange, requests before initialize/initialized
receive JSON-RPC -32600, and turn/start bodies using legacy prompt or a
non-conforming input array receive -32602 (TurnStartParams requires
input: [{type:'text', text, text_elements}]; clientUserMessageId is
retained because it is present in the generated schema).
Two regression tests prove the f792165-era transport fails against
this fixture: a raw newline JSON-RPC prompt-based client is rejected
with zero accepted messages, and pre-initialize or prompt-shaped
requests fail while schema-shaped requests succeed. No invented
thread/status method is used anywhere; idle/active derives from the
documented thread/resume response thread.status.
Evidence artifact updated with the generated-schema findings, fixture
hardening notes, RED transcripts, and a fresh read-only GREEN smoke
against the installed app-server socket (initialize returns the real
userAgent; bogus thread/resume yields a JSON-RPC error over WebSocket).
…ched app-server smoke
The generated ClientNotification type is { "method": "initialized" }
with no params member, so the initialized notification now omits params
entirely (transport notify accepts optional params and drops the member
when absent).
Evidence adds a full end-to-end smoke against a self-launched installed
`codex app-server --listen unix://...`: initialize -> initialized ->
thread/start (status.type idle) -> turn/start with the exact generated
TurnStartParams shape ({threadId, clientUserMessageId, input:
[{type:'text', text, text_elements:[]}]}) ACCEPTED with a real turn id,
then turn/interrupt cleanup. Cross-scope thread/resume returning
-32600 "no rollout found" is documented as the failed-wake path.
…lan names
Rename gjc_coordinator_ack_codex_wake to gjc_coordinator_ack_codex_handoff
across contract, server, tests, docs, and the generated plugin skill/
commands (tool count stays 22; wake_key argument unchanged).
Expose the approved public lifecycle via a schema-versioned mapping
instead of a durable-state migration: codexWakeLifecycle maps the
durable statuses (pending/published/acked/failed, schema_version 1
on disk) to requested/delivered/acknowledged/failed; read responses
carry lifecycle_schema {version, mapping} and every returned wake event
is decorated with its lifecycle label. attempts/last_error remain the
documented failure/retry metadata.
Capability-gate the heartbeat honestly: no callable automation_update
exists in this environment, so register/read responses report
heartbeat {supported:false, reason:"automation_update_unavailable"}
and no scheduling code exists or is claimed. Wake delivery stays
event-driven with startup drain, keeping parent-turn wakes durable
while heartbeat is unavailable.
…rver_timeout) and GREEN on the same live launched app-server socket
…h hostile-injection e2e buildCodexWakePrompt already carries only the resume instruction plus work_unit/wake_key and optional turn/question identifiers (summary was removed in the transport rewrite); this locks the contract end-to-end. New red-team test appends a real coordinator event whose summary contains instruction injection (IGNORE ALL PREVIOUS INSTRUCTIONS, rm -rf), question text, delegated-output and final_response sentinels, and a 50KB log dump, then asserts the app-server turn/start input text contains none of it - identifiers and the fixed instruction only, bounded under 500 chars - while the summary persists solely as bounded (<=240 char) durable diagnostic metadata. Mutation proof recorded: reintroducing the summary line into the prompt fails the test.
…y token capture tests The reviewed params-token merge was already removed (token flows solely as Authorization: Bearer in the HTTP Upgrade handshake); a new handshake-capture test locks it: the header is present only when token_file is configured, absent otherwise, and the token string never appears in any JSON-RPC frame payload. Manual framing is retained (no maintained WebSocket client in the tree supports unix sockets) and now assembles fragmented messages per RFC 6455: a FIN=0 text frame followed by opcode-0x0 continuation frames is buffered and delivered as one message, so thread/resume cannot time out on normal fragmentation. RED captured with a fixture emitting a legal fragmented response split across TCP chunks with a notification frame first (pre-fix client timed out); GREEN after the fix. Real self-launched app-server smoke extended to record the initialize response shape, thread/start idle status, accepted schema-shaped turn/start, and a live active-status thread/resume during the running turn (interrupted for cleanup).
…d desktop control socket; confirm Bearer-only auth and disposable-thread turn/start policy
…ping The reported swap (Codex host context ids written into gjc_session_id/ gjc_turn_id with codex_turn_id null) existed only in the first auto-bind iteration and was already corrected: gjc_session_id is the new delegate coordinator session, gjc_turn_id the accepted GJC turn id, codex_thread_id equals source.thread_id (enforced by the binder), codex_turn_id carries context.turn_id, and codex_host_session_id is the dedicated Codex host session field. New assertions prove two concurrent delegates get DISTINCT GJC session+turn identities while sharing one Codex thread and preserving identical codex_host_session_id/codex_turn_id correlation, and that GJC ids never equal Codex host ids. Mutation proof recorded: reintroducing the swapped mapping fails the test; the corrected mapping passes.
Collaborator
Author
|
Consolidated into PR #3912, which now contains both the ACP implementation fix and regression tests. |
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
1 participant
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
session/set_mode(default)as a safe no-op for ACP sessionsunsupportederror forplanmode when no host plan-mode lifecycle existsmode.plan.setcontrolsFixes #3905.
This is the bottom/core PR of the ACP stacked change.