Bcn bridge gateway - #14
Conversation
…cc/codex) Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
- Register crates/adapters/bridge-provider workspace member - Add tokio-stream = "0.1" to [workspace.dependencies] - Cargo.toml scaffold with workspace-shared deps (axum, serde, toml, thiserror, tokio, ...) - config.rs: EngineKind (cfuse-cc | cfuse-codex), BotConfig, ProviderConfig with load()/bot() - ConfigError wraps io::Error (read) and toml::de::Error (parse) - TDD: tests for happy-path load+bot lookup and unknown-engine rejection Co-Authored-By: Claude Code <noreply@anthropic.com>
Pure-function SSE frame encoder. encode_frame takes a pre-serialized single-line JSON string and emits event:/id:/data: lines terminated by a blank line. Rejects embedded newlines in data (FrameError::MultilineData) and frames exceeding 8MiB (FrameError::FrameTooLarge). Exposes the HEARTBEAT comment constant. Keeping encode_frame on a raw &str avoids a workspace-wide serde_json preserve_order feature flag; callers that hold a serde_json::Value serialize it themselves and propagate via FrameError::Json. Golden test aligns byte-exact with spec §10 wire form (insertion order preserved via raw string passthrough). Co-Authored-By: Claude Code <noreply@anthropic.com>
Add 8 emit-side constructors (chat_delta/final/error/aborted, agent_tool/thinking/lifecycle, interaction_event) and event_to_frame, which serializes StreamEvent to a Provider 2.0 SSE wire frame (camelCase keys) and delegates to encode_frame. Approval/Phase/Unknown agent data and Ping/Unknown top-level events return FrameError::Unsupported. Roundtrip contract tests parse each emitted frame through bcs_protocol::stream::parse_stream_event (the BCS-side parser). Co-Authored-By: Claude Code <noreply@anthropic.com>
Add forbidden_event_kinds_are_rejected asserting event_to_frame returns FrameError::Unsupported for StreamEvent::Ping, StreamEvent::Unknown, AgentData::Approval, AgentData::Phase, and AgentData::Unknown — locking the spec §2 forbidden-output contract that had no test coverage. Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 4 of bridge-provider: error type (BCN error table) + webhook router with the fixed validation chain (Authorization 401 → to_bot.provider_id 403 → method known 501 → params 400) and bot.ping. chat.send/chat.inject/chat.abort/interaction.resolve return 503 unavailable placeholder; implemented in subsequent tasks. Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 4 fix round 1: - Remove dead StatusCode import from src/webhook.rs (all statuses flow through BridgeError constructors). - Remove dead header import from tests/e2e_webhook.rs. - Add run_terminated() constructor to src/error.rs (HTTP 410 GONE, code run_terminated, retryable=false) per spec error table; needed by Task 14 (chat.abort). Now compiles with zero warnings. Co-Authored-By: Claude Code <noreply@anthropic.com>
Add IdempotencyLedger with begin/complete per spec §5.5:
- same id + same fingerprint completed → replay last response
- same id + same fingerprint in-progress → replay {"ok":true}
- same id + different fingerprint → Conflict (409)
Wire ledger into AppState; handlers consume it in later tasks.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 7 of bridge-provider: tokio subprocess wrapper with piped stdin/stdout/stderr and kill_on_drop(true). stderr lines are forwarded to tracing::debug! by a detached task and never enter protocol frames. Co-Authored-By: Claude Code <noreply@anthropic.com>
Define the Engine trait, TurnRequest/TurnOutcome/TurnError types, and a
build_engine factory in engine/mod.rs. build_engine returns a StubEngine
that records the configured EngineKind and fails every run_turn with
EngineExited("engine not wired") until Tasks 9/10 wire the real
CfuseCc/CfuseCodex drivers. No unimplemented! in production code.
TurnRequest intentionally omits the interactions field; Task 12 adds it
together with the InteractionRegistry it depends on.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 9: implement the first real engine driver. `map_cc_line` is a pure NDJSON→StreamEvent mapper over the cc stream-json contract (system/init→ SessionId; content_block_delta/text_delta→chat_delta; assistant tool_use→ agent_tool Start; user tool_result→agent_tool Result; result/success→Final; result non-success→Failed→EngineExited with a sanitized note). The control_request/can_use_tool arm is a placeholder emitting agent_thinking (observable, never stream:"approval"); Task 12 wires the real approval flow. CfuseCc::run_turn spawns `cfuse --cc --output-format stream-json ...`, writes a claude stream-json user message to stdin, and pumps stdout lines through map_cc_line in a tokio::select! abort loop (EOF-without-final→EngineExited, abort→kill+Aborted). TurnError gains thiserror::Error + an Io(#[from] io::Error) variant; build_engine wires EngineKind::CfuseCc (CfuseCodex stays stubbed pending Task 10). Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 10: implement the codex engine driver. `map_codex_block` is a pure SSE-block→CodexMap mapper over the codex Responses-API contract (response.output_text.delta→chat_delta; response.completed→Final accumulated text, run_turn falls back to streamed deltas; response.failed/error→Failed with a sanitized/bounded message→EngineExited; others→Ignore). CodexMap is Events/Final/Failed/Ignore (derives Debug for test panics). CfuseCodex::run_turn spawns `cfuse --codex --output-format sse ...`, writes a codex Responses user-input envelope to stdin, and pumps stdout SSE blocks through the new CliSession::next_sse_block in a tokio::select! abort loop (EOF-without-final→EngineExited, abort→kill+Aborted, Failed→kill+ EngineExited). engine_session_id stays None: aix-relay's codex runtime drives `codex app-server` (JSON-RPC) and resumes via the per-turn session_id param, not a --resume CLI flag; the cfuse --codex SSE path has no verified --resume equivalent, so follow-up context relies on caller-prepended pending injects (spec-sanctioned fallback). build_engine wires EngineKind::CfuseCodex to the real driver; the now-dead StubEngine is removed and its stub test is replaced with a kind-only wiring assertion. Co-Authored-By: Claude Code <noreply@anthropic.com>
…verified) Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 10 fix round 1 (pre-review). A probe of the real `cfuse --codex` CLI
proved the cc-analogy spawn shape wrong: `cfuse --codex` passes args through
to `codex exec`, whose `--json` output is JSONL (one JSON object per line) —
NOT SSE. `event:`/`data:` blocks and `response.output_text.delta`/
`response.completed` belong to aix-relay's app-server/proxy route, not this
CLI path.
Probe ground truth:
cfuse --codex exec --json --skip-git-repo-check -C <cwd> "prompt"
-> {"type":"thread.started","thread_id":"01a058a5-..."}
{"type":"turn.started"}
{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"ok"}}
{"type":"turn.completed","usage":{"input_tokens":5523,"output_tokens":24}}
Resume EXISTS via `codex exec resume <SESSION_ID> [PROMPT] --json`, so
engine_session_id is NOT恒None — capture thread_id from thread.started and
resume with it on follow-up turns.
Changes:
- map_codex_block(event,data) -> map_codex_line(line, run_id): JSONL mapper.
thread.started -> SessionId(thread_id); turn.started -> Ignore;
item.completed/agent_message -> chat_delta (+ run_turn accumulates);
item.completed/reasoning -> agent_thinking(delta=item.text);
item.completed/other -> Ignore (Task 12 wires tools);
turn.completed -> Final(best-effort text, run_turn falls back to
accumulated deltas); turn.failed/top-level error -> Failed(sanitized message,
fixed "engine turn failed" fallback when no message field — never dump the
raw JSON line); unknown type / non-JSON -> Ignore.
- CodexMap gains SessionId(String); keeps Events/Final/Failed/Ignore.
- run_turn: first turn `cfuse --codex exec --json --skip-git-repo-check -C
<cwd> [-m <model>] <prompt>`; resume `cfuse --codex exec resume <sid> --json
[-m <model>] <prompt>`. Prompt as argv. Drops the (cc-only)
--permission-mode (codex exec doesn't accept it). Tracks engine_session_id
from thread.started and resumes with it. Reads via next_line() (not SSE
blocks).
- cli.rs: stdin now Option<ChildStdin>; new close_stdin() drops the handle so
`codex exec` (which reads piped stdin as extra input) sees EOF right after
spawn — prompt is already in argv. Removed next_sse_block + its mock_sse.sh
fixture + its test (YAGNI — nothing uses it now). Added
cli_session_close_stdin_blocks_further_writes test.
- Fixture: codex_turn.sse -> codex_turn.jsonl modeled on the probe
(thread.started with fixed thread_id, turn.started, two agent_message
items "正在"/"排查", one reasoning item, turn.completed). Tests assert
session id captured, deltas=="正在排查", one thinking event from reasoning,
Final on turn.completed (empty-text, run_turn fallbacks to deltas).
- Killed the "resume unsupported" code comments.
cargo test -p bridge-provider: 42 unit + 1 e2e + 10 golden, 0 failed, 0 warnings.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Wires chat.send from the webhook through RunRegistry + the run loop to a self-managed SSE frame stream. The run driver selects over an engine-event channel, a 20s heartbeat, and a deadline timer; each event is stamped with a monotonic seq, encoded via event_to_frame, and appended to a replay buffer plus a broadcast channel. A BCS disconnect (broadcast send with no subscribers) aborts the engine and closes the run; an oversize frame yields a terminal chat_error instead of the oversize frame. The forwarder replays the buffer snapshot then follows the broadcast, with subscribe+snapshot made atomic under a std::sync::Mutex buffer so frames are neither duplicated nor lost (the snapshot partitions exactly from post-subscribe broadcast messages). Same-id active replays buffer and follows; same-id terminal replays buffered frames as a one-shot stream; same-id different body is a 409. Idempotency is checked before session reservation so re-attach works; a new run reserves the slot via try_start_run (429 on a busy session). AppState gains a RunRegistry; dispatch becomes async. Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 12: surface engine permission requests (cc can_use_tool control messages) as BCN interaction/requested SSE events, park the run, accept interaction.resolve webhook calls, write the decision back to the engine's stdin control channel, and emit interaction/resolved. Replaces Task 9's agent_thinking placeholder for control_request. - New InteractionRegistry (Arc<Mutex>, Clone): register/resolve/invalidate_run, mints interactionId as int-<uuidv4 simple>, idempotent replay -> Duplicate, deny fallback release on run terminal (spec 6.3). Unit-tested. - CfuseCc: CcMap::ControlRequest variant + handle_control_request wiring (register -> requested -> await resolution|abort -> control_response -> resolved). ask_user (AskUserQuestion) maps questions[] (questionId=header/fallback question_N, options label->label+value) and refuses secret-marked questions with an engine deny + warning, no interaction. resolution_to_behavior collapses ask_user to allow/deny (cc v1 has no answers channel; cancel/missing answers -> deny). - Webhook: interaction.resolve arm with spec 5.1 STRING-error ACK shape (ok:false,retryable:false,error:<string>) distinct from BridgeError; Delivered/Duplicate -> ok:true. - run.rs: pass state.interactions into TurnRequest; invalidate_run(run_id, deny fallback) before abort on any terminal path so parked receivers release. - TurnRequest/AppState gain interactions (new(config) signature unchanged); updated Task 8 fake-engine test struct literal. - mock_cc_approval.sh fixture: read user msg -> emit can_use_tool control_request -> await control_response -> emit result by behavior (uses /bin/echo to flush before read; avoids bash builtin pipe deadlock). Co-Authored-By: Claude Code <noreply@anthropic.com>
Implements Task 13: chat.inject handler routes through the idempotency ledger
(fingerprint method+provider_bot_ref+session_id+message), then sinks the inject
into the engine's own per-session transcript (ClaudeJsonlSink for cc bots with
an established engine_session_id) or falls back to pending_injects (codex always;
cc without an engine session or on sink failure). pending_injects are drained
FIFO on the next chat.send and prepended to the prompt as [from:{name}] {text}
lines, separated from the message body by a blank line. inject never drives an
engine run (spec §5.1).
Sink-first-then-store: SessionStore has no remove-one API, so for cc with an
established session id we attempt the transcript sink and only add_inject on
failure/no-session; codex and cc-without-session always store. Idempotent sink
on bridgeInjectId=run_id (scan-before-append); parentUuid from the last line.
TDD: RED (compile failure: ClaudeJsonlSink undefined), then GREEN.
- engine/transcript.rs: TranscriptSink trait, ClaudeJsonlSink, 6 unit tests.
- webhook.rs: split chat.inject from chat.abort; handle_chat_inject handler.
- run.rs: assemble_prompt blank-line separator; extract_message_text pub(crate).
- tests: inject_then_send_prepends_for_codex end-to-end + mock_codex.sh emitting
codex JSONL (per executed amendment; not SSE).
Co-Authored-By: Claude Code <noreply@anthropic.com>
Closes two Important review findings on Task 13:
1. cc sink-success branch had no handler-level test — a regression that always
calls add_inject after a successful sink (double-delivery: transcript entry
+ prepended prompt) would have passed every prior test. Adds
inject_sinks_to_cc_transcript_and_does_not_pending: spawns cfuse-cc with
HOME pointed at a tempdir, pre-seeds engine_session_id via the shared
AppState, POSTs chat.inject, then asserts (a) the cc transcript file at
<home>/.claude/projects/-tmp/cc-sess-1.jsonl holds exactly one user entry
tagged bridgeInjectId=inj-1, (b) pending_injects stayed empty (the sink must
not double-write), and (c) idempotent replay of the same id keeps the file
at one entry.
- support/mod.rs: spawn_app_with_state / spawn_app_with_mock_and_state
return (url, Arc<AppState>) so the test can call set_engine_session_id
and take_pending_injects directly.
- HOME_LOCK (std::sync::Mutex, const-ctor) pairs with HomeGuard RAII to
serialize env mutation across cargo test's parallel OS threads; on
edition 2024 set_var/remove_var are unsafe, so the impls carry
#[allow(unsafe_code)] under the workspace deny.
2. inject_then_send_prepends_for_codex only asserted both substrings present;
strengthened to assert the inject prefix `[from:观察者] 观察上下文` appears
literally AND strictly precedes `正式问题` in the SSE body (positionally),
so the FIFO preserve-order + blank-separator contract is locked.
cargo test -p bridge-provider: 63 unit + 6 e2e (incl. new test) + 10 golden,
all pass, 0 warnings.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Wire chat.abort end-to-end with the spec §5.3 200/410 terminal matrix.
- run.rs: RunHandle gains abort_requested (AtomicBool) and
abort_stop_reason (Mutex<String>, default "user_cancelled") plus
request_abort()/is_abort_requested()/abort_stop_reason(). The run loop
now emits a terminal chat_aborted frame in the
Err(TurnError::Aborted) branch ONLY when an explicit abort was
requested — a passive BCS disconnect stays silent (stream just ends).
- run.rs: RunRegistry consolidates to a single RunRegistryInner mutex
with map + run_session reverse index (run_id -> (bot, session)).
New: record_session() (called by handle_chat_send on is_new),
find_terminal_run() (410-leg of the matrix), and async abort_all(reason)
for Task 15 graceful shutdown (cancels every active run, skips
terminal; lock released before request_abort so it's non-blocking).
- webhook.rs: handle_chat_abort — active run → 200 {ok,aborted:true,
aborted_run_ids:[run_id]} (invalidate_run deny BEFORE engine kill,
then request_abort); terminal run recorded → 410 run_terminated
(stable on repeat); no record → 200 aborted:false. Routes through the
idempotency ledger with fingerprint
method+provider_bot_ref+session_id.
- idempotency.rs: IdemDecision::Replay now carries (StatusCode, body)
so a same-id retry of a 410 abort replays as 410, not the default
in-flight 200 ack. New complete_with_status() records non-200
responses; existing complete() delegates with 200. Inject handler
updated to the new Replay shape.
- error.rs: BridgeError::into_parts() exposes (StatusCode, body Value)
so the abort handler can store the 410 body in the ledger and return
the exact same status+body directly.
E2E: abort_active_run_emits_aborted_terminal asserts the full matrix
(aborted:true + state=aborted SSE frame → 410 on retry → 200 aborted:false
on unknown session). Unit tests cover request_abort, find_terminal_run,
abort_all, and complete_with_status replay.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Round-1 review fix: the original chat.send handler reserved the
session slot (sessions.try_start_run sets active_run) BEFORE creating
the RunRegistry entry (runs.begin). An early chat.abort landing in
that window read active_run=Some + runs.get=None and fell through to
the matrix's no-record branch, wrongly returning 200 aborted:false
even though a fresh run was actively starting.
Fix: invert the ordering in handle_chat_send — begin FIRST, then
try_start_run. The registry entry now exists the moment active_run
flips to Some, so abort's branch 1 (active → 200 aborted:true) sees
a consistent pair. Re-attach semantics preserved (Task 11 ordering):
- begin is the get-or-create atomic — same-id retries return
is_new=false BEFORE try_start_run, so they neither 429 themselves
nor touch the session slot. Same-id-different-body still 409.
- On 429 (a different run_id holds this session), roll back the
placeholder entry via a new RunRegistry::remove (bypasses the
grace-TTL pinning that finish would impose — a never-spawned
placeholder must not stall same-id retries behind a fake
terminal-replay entry). The run_id is unique here (a colliding id
would have returned is_new=false above), so remove cannot touch
another run's entry.
Side fixes:
- Branch-3 comment in handle_chat_abort updated: the
"swept-past-grace" edge it described is now unreachable (the
driver calls sessions.finish_run between runs.finish and the
sweep's grace expiry, so active_run is cleared before the entry
is reclaimable). The only former source of active_run=Some +
handle=None was this TOCTOU, now eliminated.
- record_session moved to fire AFTER try_start_run succeeds (so the
session association is only recorded for an actually-spawned run).
Tests:
- e2e chat_send_creates_run_entry_before_session_slot_claim: polls
AppState.sessions.active_run + RunRegistry::get directly while a
slow-mock chat.send is in flight, asserting the moment active_run
flips to Some(r-order) the runs entry is also present — a marker
for the new begin-first invariant. The microsecond-scale window
between try_start_run and begin is not catchable deterministically
via the public webhook interface (no hook to pause mid-handler),
so the test asserts the invariant the fix establishes; code
inspection is the proof (the begin call now precedes try_start_run
unconditionally).
- run::tests::remove_drops_entry_and_run_session_association_for_rollback
locks in the new remove() rollback semantics.
cargo test -p bridge-provider: 86 passed; 0 failed; 0 warnings.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Adds the `bridge-provider` binary target (src/main.rs): loads config from
`BRIDGE_CONFIG` (default `bridge.toml`), inits tracing with an EnvFilter
from `RUST_LOG` (falls back to `info`), and serves webhook::router on
`config.listen` via axum. Graceful shutdown on SIGINT or SIGTERM (tokio
select over ctrl_c + unix signal::terminate) stops new connections, then
RunRegistry::abort_all("shutdown") cancels every in-flight run; the
process exits 0. No unwrap/expect/panic in main.rs (uses anyhow::Result).
Adds the binary smoke test (tests/e2e_webhook.rs): spawns the real binary
on the fixed port 21999, polls until bot.ping returns 200, sends SIGTERM,
asserts exit 0. Pid's stdout/stderr piped to null for pristine test
output; try_wait surfaces a clear early-exit failure if the port is taken.
Cargo.toml: anyhow workspace dep + tracing-subscriber (env-filter) dep;
libc dev-dep for the SIGTERM kill in the smoke test.
HTTP/2 h2c verified manually (not in CI): axum::serve uses hyper-util's
auto builder which serves h2c prior-knowledge — `curl --http2-prior-knowledge`
returns `< HTTP/2 200 {"ok":true}` against the running binary.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 16 adds the spec §5/§6 protocol-regression e2e suite pinning the four behaviors added across Tasks 1–15: - duplicate_send_reattaches_with_replay: same id + same body re-attach path (200, not 429/409), re-attached stream starts from seq 1. - same_id_different_body_conflicts: RunRegistry fingerprint mismatch → 409. - missing_protocol_2_header_rejected: chat.send without X-BCN-Protocol-Version: 2.0 → 400 invalid_request. - utf8_chinese_deltas_stay_intact: 40 Chinese text_delta frames stay valid UTF-8 end-to-end; every data: line parses as JSON. - oversize_single_frame_becomes_chat_error: a 9 MiB text_delta exceeds MAX_FRAME_BYTES (8 MiB) → terminal chat/error, no oversize emission. Fixtures added: mock_cc_utf8.sh (40 Chinese deltas + terminal) and mock_cc_big.sh (single >8 MiB text_delta via head -c /dev/zero | tr). The re-attach test deviates from the brief's literal second.text().await + drop(first) shape: mock_cc_slow.sh would block 30s. Per the brief's own note, a chat.abort is sent after asserting the 200 re-attach to trigger a terminal frame promptly (seq=1), and kill_on_drop reclaims the subprocess. Cargo.lock re-synced to include the already-declared anyhow/libc/ tracing-subscriber entries for bridge-provider. Co-Authored-By: Claude Code <noreply@anthropic.com>
Task 16 review fix round 1: the original duplicate_send_reattaches_with_replay
used mock_cc_slow.sh (sleep 30s, emits nothing), so the run's buffer was empty
at re-attach time — seqs.first() == Some(&1) would pass even if the buffer-
replay loop in forward_stream were deleted. The "replay from seq 1" behavior
was unverified.
Strengthened the test to genuinely cover the buffer snapshot → replay → live-
follow partition:
- New fixture mock_cc_burst.sh: emits two text_delta lines IMMEDIATELY (突发一,
突发二), then sleeps 30s — so seq 1 and 2 land in the buffer BEFORE re-attach.
- Test flow:
1. read the FIRST stream until both deltas arrive (proves buffer non-empty),
2. re-attach (second POST, same id+body) → 200 (not 429/409),
3. read the SECOND stream until both 突发一+突发二 replay — asserting them
before the abort proves they came from the buffer snapshot (seq 1, 2),
not a live re-emission; the broadcast is quiet during the 30s sleep so a
deleted replay loop would block past the 5s deadline and fail,
4. chat.abort → terminal chat_aborted (seq 3) pushed AFTER re-attach's
subscribe → arrives via the live broadcast leg,
5. drain until aborted at seq 3 — asserting seq == [1,2,3] verifies seq
continuity across the buffer→broadcast partition.
RED demonstration: deleting the `for frame in snapshot` replay loop in
forward_stream makes the test fail at step 3 ("did not replay both deltas
within 5s"), confirming it genuinely exercises the buffer-replay path.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…be order, session-id validation, conservative decisions) - C1: build_requested_extra omits `command` (emits synthesized `description`) when input has no non-empty command string; BCS rejected present-but-null. - I1: spawn_run builds the forward stream (subscribes) before spawning the driver so the heartbeat's first tick always has a live receiver. - I2: is_valid_engine_session_id validator applied at cc system/init and codex thread.started capture sites; invalid ids are logged and dropped (never persisted / resumed / used as a transcript path component). - H: resolution_to_behavior only allows allow_once/allow_session/ allow_persistent/allow_always; everything else maps to deny (conservative). Co-Authored-By: Claude Code <noreply@anthropic.com>
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. |
![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: aad9300742
ℹ️ 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 mut obj = json!({ | ||
| "questionId": question_id, | ||
| "question": question, | ||
| "options": options, | ||
| }); |
There was a problem hiding this comment.
When Claude emits an AskUserQuestion without options, this mapper inserts "options": []. The Provider 2.0 contract requires free-text questions to omit options, and BCS rejects a present empty array in validate_questions; the interaction is therefore dropped while this bridge continues waiting for a resolution until the run deadline. Only include options when the engine supplied at least one valid option.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| match resolution["action"].as_str() { | ||
| Some("answer") => { | ||
| let has_answers = resolution["answers"] | ||
| .as_array() | ||
| .map_or(false, |a| !a.is_empty()); |
There was a problem hiding this comment.
Every valid ask-user submission from BCS uses action: "submit" and an answers object, but this branch only accepts action: "answer" with an array. Consequently all submitted answers map to deny, and the later control response also drops the answers, so the engine is told that the user rejected the question instead of receiving their response. Translate the documented submit/object payload or stop advertising ask-user support.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| let injects = state | ||
| .sessions | ||
| .take_pending_injects(&bot.provider_bot_ref, session_id) | ||
| .await; |
There was a problem hiding this comment.
For the pending-inject fallback path, take_pending_injects permanently drains session context before the engine is spawned. If cfuse is missing, the cwd is invalid, the process fails to start, or the run reaches an immediate deadline, the send returns an error but those previously acknowledged chat.inject messages are gone and cannot be supplied to a later retry, violating the contract that inject writes session context. Requeue them on pre-turn failure or only commit the drain after successful startup.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| heartbeat.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); | ||
| // Self-terminate ~30s ahead of the hard deadline so a terminal chat_error can | ||
| // still flush before the client times out. | ||
| let deadline_ms = timeout_ms.saturating_sub(30_000); |
There was a problem hiding this comment.
When a caller supplies timeout_ms <= 30_000, saturating_sub produces zero, so the deadline branch is ready immediately and the engine turn is cancelled without receiving any of the caller's requested execution budget. This makes every otherwise valid short-timeout request deterministically fail; reserve a smaller proportional flush window or clamp the execution duration to a positive value.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| // active: re-attach — replay buffer then follow broadcast. Do NOT | ||
| // call try_start_run: the slot is already held by this same run_id, | ||
| // and a fresh claim would 429 ourselves. | ||
| return Ok(crate::run::sse_response(crate::run::forward_stream(handle))); |
There was a problem hiding this comment.
If a session is already busy and two same-ID retries arrive concurrently, the first request creates this registry entry before awaiting try_start_run; the second sees it as active and returns this SSE stream, after which the first fails the session claim and removes the placeholder at line 161 without marking it terminal. The second response then has no driver and no terminal notification and hangs until its client timeout. Distinguish an unclaimed placeholder from an active run or propagate the claim failure to all reattachments.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| if let Some(sid) = o.engine_session_id { | ||
| state.sessions | ||
| .set_engine_session_id(&bot.provider_bot_ref, &session_id, &sid) | ||
| .await; |
There was a problem hiding this comment.
The drivers capture a valid engine session ID as soon as system/init or thread.started arrives, but this is the only place it is copied into SessionStore, and it executes only for an Ok(TurnOutcome). If the engine creates a transcript and then crashes, times out, or is disconnected later in the turn, the ID is discarded and the next run starts a new session instead of resuming the surviving transcript. Persist the validated ID at capture time, as required for mid-run recovery.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3be5e741a5
ℹ️ 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".
| state | ||
| .runs | ||
| .record_session(&run_id, &bot.provider_bot_ref, &session_id); | ||
| Ok(crate::run::spawn_run(state, req, bot, session_id, handle)) |
There was a problem hiding this comment.
When direct A2A chat originates from bcs-cli, a2a_chat/mod.rs selects ProviderTransportPreference::Callback, so BCS sends Accept: application/json and only consumes a JSON acknowledgement plus later /bot/events callbacks. This handler ignores that negotiation and always returns an SSE stream; the BCS transport therefore waits for the stream to finish, attempts to decode it as JSON, and fails the run, while this bridge has no callback delivery path. Branch on Accept/X-BCN-Transport or reject unsupported callback requests before starting the engine.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| "approvalPolicy": "never", | ||
| "approvalsReviewer": "user", | ||
| "sandboxPolicy": { | ||
| "type": "readOnly", | ||
| "networkAccess": false, |
There was a problem hiding this comment.
When a cfuse-codex bot is asked to edit files, this payload forces every turn into a read-only, network-disabled sandbox with approvals set to never, regardless of BotConfig.permission_mode; the same driver also rejects every app-server request. Consequently the advertised coding-engine path can analyze but cannot complete ordinary write operations or surface them through the required HITL flow. Map the configured permission policy into Codex sandbox/approval settings and handle approval requests instead of hard-coding this restrictive mode.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| pub fn chat_final(run_id: &str, text: String) -> StreamEvent { | ||
| let message = json!({"role":"assistant","content":[{"type":"text","text":text}]}); |
There was a problem hiding this comment.
When an engine supplies its answer only in the terminal result, such as the completed_text fallback in the Codex app-server driver, this message cannot deserialize as BCS's required MessageContent because timestamp is absent. build_event_payload consequently discards the message, and with no preceding deltas the user receives an empty final response. Stamp the final message with the same millisecond timestamp used by the enclosing event.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
| } | ||
| // 3. method | ||
| match req.method.as_str() { | ||
| "bot.ping" => Ok(Json(json!({"ok": true})).into_response()), |
There was a problem hiding this comment.
When provider_bot_ref is unknown or its configured cfuse_bin is missing or unusable, this branch still returns {"ok":true} without even resolving the bot configuration. Health checks therefore advertise a broken target as ready, although the next chat.send fails after opening its SSE stream. Validate the requested bot and probe the selected engine binary before acknowledging readiness.
AGENTS.md reference: AGENTS.md:L67-L76
Useful? React with 👍 / 👎.
1 participant
Problem
Solution
Validation
Compatibility and risk (optional)
Spec (optional)
Related issues (optional)