Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src-tauri/src/acp/feedback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ pub struct FeedbackItem {
pub text: String,
pub created_at: DateTime<Utc>,
pub status: FeedbackStatus,
/// When the agent read this note. `None` while `Pending`.
/// When the agent read this note. `None` while `Pending`. On the native
/// push path it is the submit instant (see [`Self::new_delivered`]) — a
/// lower bound on the read rather than the read itself.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub delivered_at: Option<DateTime<Utc>>,
}
Expand All @@ -73,6 +75,12 @@ impl FeedbackItem {
/// let `read_pending_feedback` hand the same text to a `check_user_feedback`
/// call and double-deliver it (the Pending-only read is the mutual
/// exclusion between the push and pull channels).
///
/// `at` is the SUBMIT instant, taken before the injection is handed to the
/// adapter (`submit_feedback_native`), so `created_at` is provably earlier
/// than the agent's own transcript copy of the message — the ordering the
/// frontend needs to recognize that copy. `delivered_at` shares it, and is
/// therefore a lower bound on the read rather than the read itself.
pub fn new_delivered(id: String, text: String, at: DateTime<Utc>) -> Self {
Self {
id,
Expand Down
64 changes: 59 additions & 5 deletions src-tauri/src/acp/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2648,6 +2648,11 @@ impl ConnectionManager {
/// note recorded right after `TurnComplete` is harmless — the notes
/// list renders only while prompting, and the next turn's `UserMessage`
/// clears `feedback`.
/// * `created_at` PRECEDES THE INJECTION. It is taken before the `Steer`
/// command is enqueued, so it is earlier than any transcript entry the
/// injection can cause. The frontend relies on that ordering to tell the
/// agent's own copy of the message from the same words sent in an
/// earlier round (`suppressPersistedSteeredPrompts`).
async fn submit_feedback_native(
conn_id: &str,
state: Arc<tokio::sync::RwLock<crate::acp::session_state::SessionState>>,
Expand All @@ -2664,6 +2669,18 @@ impl ConnectionManager {
let conn_id_for_task = conn_id.to_string();
let handle = tokio::spawn(async move {
let outcome: Result<FeedbackItem, AcpError> = async {
// Stamped BEFORE the command goes out, so the note's instant is
// causally earlier than anything the injection can cause. The
// adapter pushes the text to the agent before it answers, and
// the agent may write its own transcript copy of the message
// while this task is still awaiting that answer — a note
// stamped on the way back would then look NEWER than the copy
// it produced, and the frontend (which folds a persisted copy
// away only when it postdates the injection — see
// `suppressPersistedSteeredPrompts`) would show the message
// twice. Same clock, same host: `created_at` is when the note
// was created, which is also what the pull path records.
let created_at = chrono::Utc::now();
let (reply_tx, reply_rx) = tokio::sync::oneshot::channel();
cmd_tx
.send(ConnectionCommand::Steer {
Expand Down Expand Up @@ -2696,11 +2713,8 @@ impl ConnectionManager {
state.write().await.native_steering_available = false;
}
}
let item = FeedbackItem::new_delivered(
uuid::Uuid::new_v4().to_string(),
text,
chrono::Utc::now(),
);
let item =
FeedbackItem::new_delivered(uuid::Uuid::new_v4().to_string(), text, created_at);
// Ungated on purpose — see the invariant on this fn's doc.
emit_with_state(
&state,
Expand Down Expand Up @@ -7585,6 +7599,46 @@ mod tests {
})
}

/// The note's instant must precede the injection reaching the agent. The
/// adapter hands the text to the agent BEFORE it answers `injected`, so the
/// agent can write its own transcript copy of the message while this call
/// is still awaiting that answer. A note stamped on the way back would
/// postdate the copy it caused, and the frontend — which folds a persisted
/// copy away only when it postdates the injection, so that the same words
/// sent in an earlier round are never hidden — would show the message both
/// as a transcript turn and as a live one.
#[tokio::test]
async fn native_submit_stamps_the_note_before_the_agent_can_see_it() {
let mgr = ConnectionManager::new();
let mut rx = mgr
.insert_test_connection_live("c1", AgentType::ClaudeCode, None, EventEmitter::Noop)
.await;
mark_native_steering_ready(&mgr, "c1").await;
// Stand in for the adapter: note when the injection reached it (the
// earliest instant the agent could record the message), then dawdle
// before answering, as a real round-trip does.
let fake_loop = tokio::spawn(async move {
match rx.recv().await {
Some(ConnectionCommand::Steer { reply, .. }) => {
let seen_by_agent = chrono::Utc::now();
tokio::time::sleep(std::time::Duration::from_millis(20)).await;
let _ = reply.send(Ok(SteerOutcome::Injected));
seen_by_agent
}
_ => panic!("expected a Steer command"),
}
});

let item = mgr.submit_feedback("c1", "use the other API".into()).await.unwrap();
let seen_by_agent = fake_loop.await.unwrap();
assert!(
item.created_at <= seen_by_agent,
"created_at ({}) must precede the injection reaching the agent ({})",
item.created_at,
seen_by_agent
);
}

#[tokio::test]
async fn native_submit_injected_records_delivered_and_pull_stays_empty() {
let mgr = ConnectionManager::new();
Expand Down
3 changes: 3 additions & 0 deletions src/components/conversations/conversation-detail-panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2013,6 +2013,9 @@ const ConversationTabView = memo(function ConversationTabView({
connectionId: conn.connectionId,
connStatus,
enabled: feedbackEnabled,
// Notes the transcript adopted as mid-turn user turns show as messages,
// not as strips above the composer.
steeredMessageIds: conn.steeredMessageIds,
onResendAsPrompt: resendFeedbackAsPrompt,
})
// Composer "insert into current turn" (native steering only). Rethrows —
Expand Down
101 changes: 101 additions & 0 deletions src/components/message/message-list-view.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { describe, expect, it } from "vitest"
import {
advanceReplyFold,
extractDelegationSources,
isForkPointUnnamed,
markThreadTail,
mergeConsecutiveAssistantTurns,
singletonSourceTurns,
type MergedAssistantRunCache,
Expand Down Expand Up @@ -41,6 +43,7 @@ function assistantItem(
isRoleTransition: false,
previousUserIndex: null,
isLastAssistantRun: false,
isThreadTail: false,
sourceTurns: [],
}
}
Expand Down Expand Up @@ -333,6 +336,7 @@ function makeItem(
isRoleTransition: false,
previousUserIndex: null,
isLastAssistantRun: false,
isThreadTail: false,
sourceTurns: singletonSourceTurns(turn(group.id)),
}
}
Expand Down Expand Up @@ -607,3 +611,100 @@ describe("extractDelegationSources", () => {
expect(extractDelegationSources([refused])).toEqual([])
})
})

/**
* The fork affordance sends a turn id to the backend, and the backend cannot
* resolve an id this client minted for its own live stream — it tail-forks
* instead of refusing. That is the right answer for the newest reply and a
* silent wrong one for any earlier reply, which a steered turn creates: it
* promotes as assistant / user message / assistant, so its first half sits
* settled and non-tail with a fork button while the parser's name is still a
* reparse away.
*/
describe("isForkPointUnnamed", () => {
function forkTurn(id: string, sourceTurnId?: string | null): MessageTurn {
return {
id,
role: "assistant",
blocks: [],
timestamp: "",
...(sourceTurnId !== undefined ? { source_turn_id: sourceTurnId } : {}),
}
}

it("withholds a live-named reply that is not the thread's last item", () => {
expect(isForkPointUnnamed(forkTurn("live-7-lm-1"), false)).toBe(true)
})

it("allows one at the end of the thread — there the tail IS the fork point", () => {
expect(isForkPointUnnamed(forkTurn("live-7-lm-1"), true)).toBe(false)
})

it("withholds the newest REPLY when a message follows it", () => {
// Steering at the very end of a turn promotes as assistant + user message
// with nothing after it: the reply is the newest one, and still not the
// tail. The backend's tail fork would land after the steered message, and
// a parse ending on a user turn never backfills a name to correct it — so
// "newest assistant run" is the wrong exception and `isThreadTail` is the
// right one.
expect(isForkPointUnnamed(forkTurn("live-7-lm"), false)).toBe(true)
})

it("allows it again once the reparse names it", () => {
expect(isForkPointUnnamed(forkTurn("live-7-lm-1", "turn-4"), false)).toBe(
false
)
})

it("leaves parser-named history alone", () => {
// Every historical turn arrives under a parser id and no `source_turn_id`;
// treating that as unnamed would grey out the whole thread.
expect(isForkPointUnnamed(forkTurn("turn-4"), false)).toBe(false)
})

it("says nothing about a group with no turns", () => {
expect(isForkPointUnnamed(null, false)).toBe(false)
})
})

describe("markThreadTail", () => {
const compaction: ThreadItem = {
key: "persisted-compact",
kind: "compaction",
meta: { contextCompaction: true },
}
const tailFlags = (items: ThreadItem[]) =>
items.map((it) => (it.kind === "turn" ? it.isThreadTail : null))

it("marks the last rendered turn", () => {
const items = [assistantItem("a"), assistantItem("b")]
markThreadTail(items)
expect(tailFlags(items)).toEqual([false, true])
})

it("leaves a reply unmarked when a message follows it", () => {
// The shape a steer at the very end of a turn promotes to: the reply is
// still the newest one, and the tail is the message after it.
const items = [assistantItem("a"), makeUserItem("u", 1)]
markThreadTail(items)
expect(tailFlags(items)).toEqual([false, true])
})

it("marks nothing when a compaction divider is last", () => {
const items = [assistantItem("a"), compaction]
markThreadTail(items)
expect(tailFlags(items)).toEqual([false, null])
})

it("steps over a trailing turn that renders nothing", () => {
const items = [assistantItem("a"), assistantItem("empty", { parts: [] })]
markThreadTail(items)
expect(tailFlags(items)).toEqual([true, false])
})

it("marks nothing in an empty thread", () => {
const items: ThreadItem[] = []
markThreadTail(items)
expect(items).toEqual([])
})
})
Loading
Loading