Repository navigation
acpmux: add authenticated Cloud trigger chat delivery - #30526
Conversation
… it is quiet (#30228) * acpmux: a turn the person did not start never pings, and says whether it is quiet Claude Code runs turns of its own (a task notification, a scheduled wakeup or loop tick, a hook). acpmux now brackets each with background_turn records, keeps its words off the chat's preview, and marks it quiet when its reply is one short line with no question and no link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-3bab30503f0b9a7059e5 * acpmux: rustfmt Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-3bab30503f0b9a7059e5 * acpmux: background_turn reaches transcript readers; a turn met mid-way after a restart and a long reply are handled Review of 56303e2 (router-review, gpt-6.1-sol): the records were left out of TRANSCRIPT_MUX_KINDS, so the pane never heard them. Also: model work with no acpmux turn and no system/init seen (a daemon restart mid-turn) starts a background turn, and a reply past the kept text is never quiet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-3bab30503f0b9a7059e5 * acpmux: rustfmt Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-3bab30503f0b9a7059e5 --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Review (claude-opus-5-5 medium, cloud-reviewer): SHIP at 68bfabb Delta review over my SHIP at a3aac43. The one new commit, 68bfabb ("clear trigger integration clippy warnings"), is lint only:
Authority, the restored Follow-ups: carried unchanged from the a3aac43 receipt:
|
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
There was a problem hiding this comment.
9 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmux-tui/crates/acpmux/src/hub/log_load.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/log_load.rs:318">
P2: Make this recovery idempotent before appending the terminal background marker. A crash after shutdown/cancellation has persisted `ended` but before `turn_result` causes each restart to append another `ended` event for the same trigger turn, producing an invalid duplicate lifecycle bracket.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/server/mod.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/server/mod.rs:1621">
P1: This check does not close the connection's scope race: the trigger handler is spawned asynchronously and only sets `trigger` after a network authorization await, so a client can pipeline an ordinary request or notification that runs while `is_trigger()` is still false. Mark the connection as trigger-pending and serialize or reject all non-trigger work until admission succeeds; apply the same fence to already-admitted notification work.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/prompt_provenance.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/prompt_provenance.rs:596">
P3: This test does not exercise trigger-field validation because `Sender::Local` rejects every trigger provenance before checking the newline. Call `of` with `Sender::Trigger` so the test fails if trigger connections start accepting newline-bearing fields.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/server/trigger_chat.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/server/trigger_chat.rs:366">
P2: A send that takes longer than four seconds to reach admission returns failure, but its detached `prompt_with` task keeps running and can deliver the prompt later. Cancel the task on timeout or keep the operation attached until admission has a definitive outcome.</violation>
<violation number="2" location="cmux-tui/crates/acpmux/src/server/trigger_chat.rs:429">
P1: The identity check ends before the local trigger operation starts, leaving a TOCTOU window during session startup and prompt delivery. Keep the identity fence through the side effect, including a final check at the dispatch point and a check before returning the operation result.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/hosts.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/hosts.rs:1183">
P2: Recovery writes an unknown outcome for an interrupted trigger turn without closing its background bracket. End the trigger background record before recording the unknown outcome, otherwise the durable transcript can leave this trigger running indefinitely.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/background_turn.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/background_turn.rs:185">
P2: `start_trigger_background` leaves `BackgroundTurns::hint` intact when replacing a pending or running background bracket. Clear the hint here, otherwise a later autonomous `system/init` can inherit an old `task_notification` or `scheduled` trigger label.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs:83">
P2: Every trigger status poll and send currently replays the entire session journal from sequence zero, so delivery checks become O(log size) and repeatedly parse unrelated history. Use an indexed or tail-bounded readback for the delivery instead of rescanning the complete log.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/cli/trigger_chat.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/cli/trigger_chat.rs:70">
P1: `trigger-chat` always reports the current daemon as unsupported because `daemon_lists` reads `features`, while `trigger-chat-v1` is advertised under `capabilities` and is absent from `features`. Read the initialize capability IDs (or add this ID to the feature list) before rejecting the operation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // principal it is a single-purpose channel. In particular, | ||
| // the local socket must not keep granting ordinary person | ||
| // RPCs after trigger authentication. | ||
| if conn.is_trigger() && !m.starts_with("trigger/chat.") { |
There was a problem hiding this comment.
P1: This check does not close the connection's scope race: the trigger handler is spawned asynchronously and only sets trigger after a network authorization await, so a client can pipeline an ordinary request or notification that runs while is_trigger() is still false. Mark the connection as trigger-pending and serialize or reject all non-trigger work until admission succeeds; apply the same fence to already-admitted notification work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/server/mod.rs, line 1621:
<comment>This check does not close the connection's scope race: the trigger handler is spawned asynchronously and only sets `trigger` after a network authorization await, so a client can pipeline an ordinary request or notification that runs while `is_trigger()` is still false. Mark the connection as trigger-pending and serialize or reject all non-trigger work until admission succeeds; apply the same fence to already-admitted notification work.</comment>
<file context>
@@ -1565,6 +1614,21 @@ async fn serve_connection_from(
+ // principal it is a single-purpose channel. In particular,
+ // the local socket must not keep granting ordinary person
+ // RPCs after trigger authentication.
+ if conn.is_trigger() && !m.starts_with("trigger/chat.") {
+ conn.send(&Message::err(
+ id,
</file context>
| match expected { | ||
| "ensure" => { | ||
| same_identity(&identity).await?; | ||
| ensure(hub, &scope).await |
There was a problem hiding this comment.
P1: The identity check ends before the local trigger operation starts, leaving a TOCTOU window during session startup and prompt delivery. Keep the identity fence through the side effect, including a final check at the dispatch point and a check before returning the operation result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/server/trigger_chat.rs, line 429:
<comment>The identity check ends before the local trigger operation starts, leaving a TOCTOU window during session startup and prompt delivery. Keep the identity fence through the side effect, including a final check at the dispatch point and a check before returning the operation result.</comment>
<file context>
@@ -0,0 +1,483 @@
+ match expected {
+ "ensure" => {
+ same_identity(&identity).await?;
+ ensure(hub, &scope).await
+ }
+ "send" => {
</file context>
| })), | ||
| ..Default::default() | ||
| }; | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
P2: A send that takes longer than four seconds to reach admission returns failure, but its detached prompt_with task keeps running and can deliver the prompt later. Cancel the task on timeout or keep the operation attached until admission has a definitive outcome.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/server/trigger_chat.rs, line 366:
<comment>A send that takes longer than four seconds to reach admission returns failure, but its detached `prompt_with` task keeps running and can deliver the prompt later. Cancel the task on timeout or keep the operation attached until admission has a definitive outcome.</comment>
<file context>
@@ -0,0 +1,483 @@
+ })),
+ ..Default::default()
+ };
+ tokio::spawn(async move {
+ let _ = hub_for_task
+ .prompt_with(
</file context>
| let mut closed_window = false; | ||
| let mut prompt_hash = None::<String>; | ||
| let mut permissions = HashSet::<String>::new(); | ||
| let scanned = self.store.scan_strict(&session.id, 0, &mut |rec| { |
There was a problem hiding this comment.
P2: Every trigger status poll and send currently replays the entire session journal from sequence zero, so delivery checks become O(log size) and repeatedly parse unrelated history. Use an indexed or tail-bounded readback for the delivery instead of rescanning the complete log.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs, line 83:
<comment>Every trigger status poll and send currently replays the entire session journal from sequence zero, so delivery checks become O(log size) and repeatedly parse unrelated history. Use an indexed or tail-bounded readback for the delivery instead of rescanning the complete log.</comment>
<file context>
@@ -63,6 +64,182 @@ impl Hub {
+ let mut closed_window = false;
+ let mut prompt_hash = None::<String>;
+ let mut permissions = HashSet::<String>::new();
+ let scanned = self.store.scan_strict(&session.id, 0, &mut |rec| {
+ let field = |key: &str| rec.msg.get(key).and_then(Value::as_str);
+ let mux = rec.dir == "mux";
</file context>
| let malformed = params( | ||
| json!({"kind": "trigger", "triggerId": "tr\n", "deliveryId": "d", "owner": "o", "team": "t", "machine": "m"}), | ||
| ); | ||
| assert!(of(&malformed, Sender::Local).is_err()); |
There was a problem hiding this comment.
P3: This test does not exercise trigger-field validation because Sender::Local rejects every trigger provenance before checking the newline. Call of with Sender::Trigger so the test fails if trigger connections start accepting newline-bearing fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/prompt_provenance.rs, line 596:
<comment>This test does not exercise trigger-field validation because `Sender::Local` rejects every trigger provenance before checking the newline. Call `of` with `Sender::Trigger` so the test fails if trigger connections start accepting newline-bearing fields.</comment>
<file context>
@@ -505,3 +556,43 @@ mod sender_tests {
+ let malformed = params(
+ json!({"kind": "trigger", "triggerId": "tr\n", "deliveryId": "d", "owner": "o", "team": "t", "machine": "m"}),
+ );
+ assert!(of(&malformed, Sender::Local).is_err());
+ }
+}
</file context>
| assert!(of(&malformed, Sender::Local).is_err()); | |
| assert!(of(&malformed, Sender::Trigger).is_err()); |
|
Review (acpmux area owner, daemon ownership and turn paths; claude-opus-5-5 subagent plus owner check): BLOCK at 80ade41 The trigger paths themselves meet the HQ#5846 contract: a scoped principal, fail-closed authorize, exact delivery/turn readback, and no auto-approval. The block is a regression in the merge 31a006b (base into the lane), which drops existing behaviour for every session. Blocking:
Conditions (acpmux-owner ACK 6554539408):
Follow-ups (file each one; none blocks):
ACK withheld until 1 and 2 are fixed. Send the delta (head..fix) and I will re-check just those paths. |
There was a problem hiding this comment.
1 existing issue remains and 7 new issues found across 39 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmux-tui/crates/acpmux/src/daemon.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/daemon.rs:1175">
P1: This wrapper does not preserve the no-caller guarantee on the alternate-socket fallback: `start_and_connect` probes the lock holder through `Client::connect`, which can send ambient `callerSession` metadata. Pass the no-caller connection strategy through `holder_elsewhere` as well, or make that probe use `Client::connect_without_caller`.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs:221">
P2: A terminal trigger can be reported as `attention` instead of `failed`: the pending-permission override runs after the cancellation, refusal, error, and interruption branches. Apply the override only while the turn is still non-terminal, so stale permission requests cannot resurrect a completed or failed receipt.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/lifecycle.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/lifecycle.rs:374">
P2: This unconditional disarm drops the retry path when failed-session cleanup errors, leaving the failed session registered and its fixed ID unavailable. Keep the `CreationGuard` armed until cleanup succeeds, or return cleanup status and disarm only after a successful purge.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/trigger_quiet_tests.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/trigger_quiet_tests.rs:154">
P2: This test ignores `prompt_with`'s result, so a trigger question that fails instead of being cleanly cancelled can still pass. Unwrap the inner RPC result to make the cancellation path part of the assertion.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/hub/withdraw.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/hub/withdraw.rs:171">
P1: This parameter trusts mutable connection-level trigger state rather than the authorization state captured when the request was admitted. Snapshot the trigger admission before spawning the request, or require the trigger scope to be validated for this exact prompt/session before selecting `Sender::Trigger`; otherwise a concurrent in-flight prompt can be treated as an authenticated trigger operation without its scope checks.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/server/requests.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/server/requests.rs:687">
P1: This branch bypasses the normal origin guard for trigger operations, allowing Web/Peer connections with a trigger credential to deliver to the local trigger chat. Restrict these RPCs to the local Unix-socket connection (or apply an explicit equivalent origin check) before dispatching them.</violation>
</file>
<file name="cmux-tui/crates/acpmux/src/server/trigger_chat.rs">
<violation number="1" location="cmux-tui/crates/acpmux/src/server/trigger_chat.rs:285">
P1: This fast path reuses any matching session ID, including a human or stale imported session, because it checks only mutable binding fields and no trigger ownership marker. Persist and verify trigger ownership before reusing a session; otherwise reject the collision so trigger delivery cannot enter a human-facing chat.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 8 unresolved issues already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
| pub async fn connect_without_caller(autostart: bool) -> Result<Arc<Client>> { | ||
| let home = home(); | ||
| let path = socket_path(); | ||
| connect_with_systemctl("systemctl", &home, &path, autostart, || { |
There was a problem hiding this comment.
P1: This wrapper does not preserve the no-caller guarantee on the alternate-socket fallback: start_and_connect probes the lock holder through Client::connect, which can send ambient callerSession metadata. Pass the no-caller connection strategy through holder_elsewhere as well, or make that probe use Client::connect_without_caller.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/daemon.rs, line 1175:
<comment>This wrapper does not preserve the no-caller guarantee on the alternate-socket fallback: `start_and_connect` probes the lock holder through `Client::connect`, which can send ambient `callerSession` metadata. Pass the no-caller connection strategy through `holder_elsewhere` as well, or make that probe use `Client::connect_without_caller`.</comment>
<file context>
@@ -1167,6 +1167,17 @@ pub async fn connect(autostart: bool) -> Result<Arc<Client>> {
+pub async fn connect_without_caller(autostart: bool) -> Result<Arc<Client>> {
+ let home = home();
+ let path = socket_path();
+ connect_with_systemctl("systemctl", &home, &path, autostart, || {
+ Client::connect_without_caller(&path)
+ })
</file context>
| pub(crate) fn prompt_provenance( | ||
| params: &Value, | ||
| origin: crate::server::Origin, | ||
| trigger: bool, |
There was a problem hiding this comment.
P1: This parameter trusts mutable connection-level trigger state rather than the authorization state captured when the request was admitted. Snapshot the trigger admission before spawning the request, or require the trigger scope to be validated for this exact prompt/session before selecting Sender::Trigger; otherwise a concurrent in-flight prompt can be treated as an authenticated trigger operation without its scope checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/withdraw.rs, line 171:
<comment>This parameter trusts mutable connection-level trigger state rather than the authorization state captured when the request was admitted. Snapshot the trigger admission before spawning the request, or require the trigger scope to be validated for this exact prompt/session before selecting `Sender::Trigger`; otherwise a concurrent in-flight prompt can be treated as an authenticated trigger operation without its scope checks.</comment>
<file context>
@@ -168,10 +168,16 @@ impl Hub {
pub(crate) fn prompt_provenance(
params: &Value,
origin: crate::server::Origin,
+ trigger: bool,
) -> Result<Option<Value>, RpcError> {
use crate::prompt_provenance::Sender;
</file context>
| "trigger connection accepts trigger/chat operations only", | ||
| )); | ||
| } | ||
| if matches!( |
There was a problem hiding this comment.
P1: This branch bypasses the normal origin guard for trigger operations, allowing Web/Peer connections with a trigger credential to deliver to the local trigger chat. Restrict these RPCs to the local Unix-socket connection (or apply an explicit equivalent origin check) before dispatching them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/server/requests.rs, line 687:
<comment>This branch bypasses the normal origin guard for trigger operations, allowing Web/Peer connections with a trigger credential to deliver to the local trigger chat. Restrict these RPCs to the local Unix-socket connection (or apply an explicit equivalent origin check) before dispatching them.</comment>
<file context>
@@ -678,6 +678,18 @@ pub(super) async fn handle_request(
+ "trigger connection accepts trigger/chat operations only",
+ ));
+ }
+ if matches!(
+ m,
+ method::TRIGGER_CHAT_ENSURE | method::TRIGGER_CHAT_SEND | method::TRIGGER_CHAT_STATUS
</file context>
| } | ||
|
|
||
| async fn ensure(hub: &Arc<Hub>, scope: &TriggerScope) -> Result<Value, RpcError> { | ||
| if let Ok(session) = exact_session(hub, &scope.session) { |
There was a problem hiding this comment.
P1: This fast path reuses any matching session ID, including a human or stale imported session, because it checks only mutable binding fields and no trigger ownership marker. Persist and verify trigger ownership before reusing a session; otherwise reject the collision so trigger delivery cannot enter a human-facing chat.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/server/trigger_chat.rs, line 285:
<comment>This fast path reuses any matching session ID, including a human or stale imported session, because it checks only mutable binding fields and no trigger ownership marker. Persist and verify trigger ownership before reusing a session; otherwise reject the collision so trigger delivery cannot enter a human-facing chat.</comment>
<file context>
@@ -0,0 +1,483 @@
+}
+
+async fn ensure(hub: &Arc<Hub>, scope: &TriggerScope) -> Result<Value, RpcError> {
+ if let Ok(session) = exact_session(hub, &scope.session) {
+ let meta = session.meta();
+ if meta.cwd != PathBuf::from(&scope.cwd)
</file context>
| // A partial journal is not evidence of completion or of a safe resend. | ||
| return json!({"delivery": delivery, "state": "failed", "error": "trigger delivery journal could not be read"}); | ||
| } | ||
| if !permissions.is_empty() { |
There was a problem hiding this comment.
P2: A terminal trigger can be reported as attention instead of failed: the pending-permission override runs after the cancellation, refusal, error, and interruption branches. Apply the override only while the turn is still non-terminal, so stale permission requests cannot resurrect a completed or failed receipt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/prompt_ledger.rs, line 221:
<comment>A terminal trigger can be reported as `attention` instead of `failed`: the pending-permission override runs after the cancellation, refusal, error, and interruption branches. Apply the override only while the turn is still non-terminal, so stale permission requests cannot resurrect a completed or failed receipt.</comment>
<file context>
@@ -63,6 +64,182 @@ impl Hub {
+ // A partial journal is not evidence of completion or of a safe resend.
+ return json!({"delivery": delivery, "state": "failed", "error": "trigger delivery journal could not be read"});
+ }
+ if !permissions.is_empty() {
+ state = "attention";
+ }
</file context>
| if !permissions.is_empty() { | |
| if !permissions.is_empty() && matches!(state, "queued" | "running") { |
| // A session whose agent never started is not left behind, and | ||
| // neither is a child that spawned but failed to initialize. | ||
| self.discard_failed_creation(&session, created_stops).await; | ||
| creation_guard.disarm(); |
There was a problem hiding this comment.
P2: This unconditional disarm drops the retry path when failed-session cleanup errors, leaving the failed session registered and its fixed ID unavailable. Keep the CreationGuard armed until cleanup succeeds, or return cleanup status and disarm only after a successful purge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/lifecycle.rs, line 374:
<comment>This unconditional disarm drops the retry path when failed-session cleanup errors, leaving the failed session registered and its fixed ID unavailable. Keep the `CreationGuard` armed until cleanup succeeds, or return cleanup status and disarm only after a successful purge.</comment>
<file context>
@@ -336,6 +371,7 @@ impl Hub {
// A session whose agent never started is not left behind, and
// neither is a child that spawned but failed to initialize.
self.discard_failed_creation(&session, created_stops).await;
+ creation_guard.disarm();
return Err(e);
}
</file context>
| assert!(!session.permissions.lock().unwrap().pending.is_empty()); | ||
| // Cancellation settles the outstanding question; it never auto-approves it. | ||
| hub.cancel(&session).await.unwrap(); | ||
| let _ = tokio::time::timeout(WAIT, task).await.unwrap().unwrap(); |
There was a problem hiding this comment.
P2: This test ignores prompt_with's result, so a trigger question that fails instead of being cleanly cancelled can still pass. Unwrap the inner RPC result to make the cancellation path part of the assertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/trigger_quiet_tests.rs, line 154:
<comment>This test ignores `prompt_with`'s result, so a trigger question that fails instead of being cleanly cancelled can still pass. Unwrap the inner RPC result to make the cancellation path part of the assertion.</comment>
<file context>
@@ -0,0 +1,201 @@
+ assert!(!session.permissions.lock().unwrap().pending.is_empty());
+ // Cancellation settles the outstanding question; it never auto-approves it.
+ hub.cancel(&session).await.unwrap();
+ let _ = tokio::time::timeout(WAIT, task).await.unwrap().unwrap();
+ let records = hub.events(&session.id, 0, 10_000).unwrap();
+ let ended = records
</file context>
| let _ = tokio::time::timeout(WAIT, task).await.unwrap().unwrap(); | |
| tokio::time::timeout(WAIT, task).await.unwrap().unwrap().unwrap(); |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmux-tui/crates/acpmux/src/hub/turns.rs">
<violation number="1">
P2: Digest turns contain messages from multiple agents, but this passes the first queued run to `run_params`, attaching that sender to the whole digest. Omit sender metadata for digests so relays do not misattribute the combined prompt.</violation>
<violation number="2" location="cmux-tui/crates/acpmux/src/hub/turns.rs:598">
P2: Trigger prompts are agent-less but this marks them as the person's turn. That makes `inbox_turn_started` hold pending agent messages behind a cloud trigger and applies human-turn inbox guards; exclude trigger provenance when computing `persons`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| let persons = super::inbox::persons_prompt(run.agent.as_deref(), run.provenance.as_ref()) | ||
| && self.config.read().await.agent_inbox(); |
There was a problem hiding this comment.
P2: Trigger prompts are agent-less but this marks them as the person's turn. That makes inbox_turn_started hold pending agent messages behind a cloud trigger and applies human-turn inbox guards; exclude trigger provenance when computing persons.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/turns.rs, line 598:
<comment>Trigger prompts are agent-less but this marks them as the person's turn. That makes `inbox_turn_started` hold pending agent messages behind a cloud trigger and applies human-turn inbox guards; exclude trigger provenance when computing `persons`.</comment>
<file context>
@@ -586,8 +595,11 @@ impl Hub {
// A continuation Stop overtook since it fired is refused here (`auto_resume.rs`).
// Another agent's message shows as its sender and words (`prompt_line.rs`).
let trigger = crate::prompt_provenance::is_trigger(run.provenance.as_ref());
+ let persons = super::inbox::persons_prompt(run.agent.as_deref(), run.provenance.as_ref())
+ && self.config.read().await.agent_inbox();
let mut turn = TurnInfo::prompted(client, &text, &turn_id, &prompt_id, control)
</file context>
| let persons = super::inbox::persons_prompt(run.agent.as_deref(), run.provenance.as_ref()) | |
| && self.config.read().await.agent_inbox(); | |
| let persons = !crate::prompt_provenance::is_trigger(run.provenance.as_ref()) | |
| && super::inbox::persons_prompt(run.agent.as_deref(), run.provenance.as_ref()) | |
| && self.config.read().await.agent_inbox(); |
| @@ -431,17 +431,15 @@ impl Hub { | |||
| && running.is_some() | |||
There was a problem hiding this comment.
P2: Digest turns contain messages from multiple agents, but this passes the first queued run to run_params, attaching that sender to the whole digest. Omit sender metadata for digests so relays do not misattribute the combined prompt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/acpmux/src/hub/turns.rs, line 874:
<comment>Digest turns contain messages from multiple agents, but this passes the first queued run to `run_params`, attaching that sender to the whole digest. Omit sender metadata for digests so relays do not misattribute the combined prompt.</comment>
<file context>
@@ -859,7 +871,7 @@ impl Hub {
session,
&child,
- json!({"sessionId": agent_sid, "prompt": wire}),
+ super::prompt_sender::run_params(&agent_sid, json!(wire), &run),
&turn_id,
false,
</file context>
|
Found 20 test failures on Blacksmith runners: Failures
...and 10 more test failures. View all on Blacksmith: run 41291141231, run 41291141175
|
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next crash ratchet; NEW: green on the base 7745465; do not merge until green; see lane-rules Shipped in:
|
…trigger-chat-auth
|
Review (claude-opus-5-5 medium, cloud-reviewer): SHIP at e4cb845 This is a delta review against my SHIP at 68bfabb. The only new commit is e4cb845, a merge of feat-cmux-next at 7745465 into 68bfabb. Carry check
Not proven here: a semantic compile and tests of the merged tree. A clean textual merge with #30337's hub, session and turn changes is not a compile receipt, so this head's own CI (acpmux build, tests and clippy) is the check for that. At the time of writing those checks are pending. The only failure is the risk-tier "hold high until a person looked" gate, not a code red. Failures inherited from base (for example #30337's registry and Rustls tests until #30775 lands) are base reds, not this PR's. Follow-ups, carried unchanged from 68bfabb (via the a3aac43 receipt):
🤖 Generated with Claude Code |
|
This PR made the Rust panic ratchet red on feat-cmux-next: acpmux |
|
Review (independent Codex gpt-6.1-sol, high, via the review router; full diff from merge base 7745465 to this head): BLOCK at e4cb845. [P1] [blocking] trigger_chat.rs:438-448 - The host identity is checked before |
… trigger chat use get and JsonSet::set At the first fix head 138e777 (job 4c82b1115ebde52dc9e7a883) the ratchet still has a second row: acpmux indexing_slicing 702 > 686. Every new hit is #30526's (68cab88); the other acpmux commits since the last green tip (57be1e3) add none in production code. - hub/prompt_ledger.rs: provenance and record reads now use get with as_str/as_bool (`v["k"] == "x"` is true only for that string, as `get(..).and_then(as_str) == Some("x")`); the `out[..] =` writes on the json! object are JsonSet::set. - server/trigger_chat.rs: the receipt/result writes are JsonSet::set (each target is the object validate_durable_receipt returned). No behavior change; the baseline is not raised.
prompt_provenance.rs test `!of_provenance(..).is_some()` is `of_provenance(..).is_none()` (clippy: this boolean expression can be simplified). Fixes #30526.
- acpmux server/trigger_chat.rs identity(): the non-Linux cfg block is the tail expression on macOS, so its `return` is needless there (clippy on macOS). It is now the block's value; on Linux that block is not compiled, as before. Fixes #30526. - cmux-tui link/start_chat_cli.rs plan_with: `create_key: keys.create.clone()` is the last use of keys.create (the workspace closure just before has already run), so it moves. Fixes #30574.
…s and lints, #30574's redundant clone (#30853) * base red: panic ratchet, #30526's two unreachable! in trigger chat return the caller's typed error The panic ratchet is red at 98dbb85 (job 770f13cbbcccb1d764e99793): acpmux clippy::unreachable 4 > 2. #30526 (68cab88) added the two: - cli/trigger_chat.rs run: the match over `operation(..)`'s result. Its fallback arm now returns the same anyhow error `operation` gives for a bad value. - server/trigger_chat.rs handle: the match over `expected`, set from the method a few lines up. Its fallback arm now returns the same RpcError::method_not_found(m) the first match gives. Both arms cannot run (the value is one of the three matched strings), so no path changes. Fixes #30526. * base red: panic ratchet, #30526's new Value index reads and writes in trigger chat use get and JsonSet::set At the first fix head 138e777 (job 4c82b1115ebde52dc9e7a883) the ratchet still has a second row: acpmux indexing_slicing 702 > 686. Every new hit is #30526's (68cab88); the other acpmux commits since the last green tip (57be1e3) add none in production code. - hub/prompt_ledger.rs: provenance and record reads now use get with as_str/as_bool (`v["k"] == "x"` is true only for that string, as `get(..).and_then(as_str) == Some("x")`); the `out[..] =` writes on the json! object are JsonSet::set. - server/trigger_chat.rs: the receipt/result writes are JsonSet::set (each target is the object validate_durable_receipt returned). No behavior change; the baseline is not raised. * base red: clippy nonminimal_bool in #30526's trigger provenance test prompt_provenance.rs test `!of_provenance(..).is_some()` is `of_provenance(..).is_none()` (clippy: this boolean expression can be simplified). Fixes #30526. * base red: clippy needless_return (#30526) and redundant_clone (#30574) - acpmux server/trigger_chat.rs identity(): the non-Linux cfg block is the tail expression on macOS, so its `return` is needless there (clippy on macOS). It is now the block's value; on Linux that block is not compiled, as before. Fixes #30526. - cmux-tui link/start_chat_cli.rs plan_with: `create_key: keys.create.clone()` is the last use of keys.create (the workspace closure just before has already run), so it moves. Fixes #30574.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Implements authenticated Cloud trigger chat delivery on acpmux with fixed session identities, scoped RPCs, durable receipts, quiet turn isolation, restart recovery, and purge-safe reuse.
The daemon now validates host identity and trigger scope on every operation, fences readback to the exact durable turn and prompt digest, rejects collisions and stale imports, and exposes the trigger-chat CLI path. The test coverage exercises authorization, fixed-id lifecycle, collision and expiry behavior, restart/readback recovery, delivery receipts, quiet notifications, and malformed credentials.
This is one cohesive security and lifecycle change: authorization must bind the delivery to a fixed chat, the journal must attest that exact prompt and turn, and recovery/quiet/purge handling must preserve those same boundaries. Splitting those paths into independently shipped changes would leave incomplete enforcement.
Base integration preserves sender metadata, human-turn admission/interrupt protection, and inbox release. It also restores the prompt_sender module registration that an earlier merge lost, which caused the Windows compile error.
Authorization is pinned by build configuration to the existing environment/origin table and defaults to production. Release builds reject development and staging before network access. HTTPS certificate/hostname validation, no redirects, and no environment proxy constrain the authority that can attest each active principal. Debug acceptance artifacts explicitly select development at compile time. An enrolled Linux guest with the new root host identity IPC is required; non-Linux trigger authorization fails closed, and older guests without an identity receipt need fresh enrollment.
Validation at a3aac43 passed in https://github.com/manaflow-ai/cmux/actions/runs/41290619157: authority 2/2, trigger chat 10/10, durable delivery 10/10, quiet 5/5, fixed IDs 7/7, background turns 8/8, purge 1/1, sender metadata 3/3, agent inbox 19/19, release authority refusal 3/3. The previously failing human-steer test now passes, and the Linux cmux-tui build passed.
That clippy run exposed three warnings (unused accessor and two owned path comparisons). Final head 68bfabb fixes only those warnings. Strict
cargo clippy -p acpmux --lib -- -D warnings, the same suites, and a replacement build/artifact are running in https://github.com/manaflow-ai/cmux/actions/runs/41291157112. Output includes the stripped executable, source snapshot SHA, PR head, SHA-256 digest, compile-time authority choice, file type, and required runtime libraries.Agent session: agent session