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
11 changes: 11 additions & 0 deletions crates/jcode-app-core/src/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,17 @@ impl Agent {
self.rewind_undo_snapshot = None;
}

/// Synchronize the remote client's selected skill, accepting only names
/// present in the daemon's own registry snapshot.
pub(super) fn set_remote_active_skill(&mut self, active_skill: Option<String>) -> bool {
let skills = self.current_skills_snapshot();
let recognized = active_skill
.as_ref()
.is_none_or(|name| skills.get(name).is_some());
self.active_skill = active_skill.filter(|name| skills.get(name).is_some());
recognized
}

fn sync_session_compaction_state_from_manager(
&mut self,
manager: &crate::compaction::CompactionManager,
Expand Down
1 change: 1 addition & 0 deletions crates/jcode-app-core/src/server/client_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ impl Client {
content: content.to_string(),
images: vec![],
system_reminder: None,
active_skill: None,
no_reply: false,
};
let json = serde_json::to_string(&request)? + "\n";
Expand Down
18 changes: 18 additions & 0 deletions crates/jcode-app-core/src/server/client_lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ struct ProcessingMessage {
content: String,
images: Vec<(String, String)>,
system_reminder: Option<String>,
active_skill: Option<String>,
}

struct ProcessingState<'a> {
Expand Down Expand Up @@ -1105,6 +1106,7 @@ pub(super) async fn handle_client(
content,
images,
system_reminder,
active_skill,
no_reply,
} => {
if no_reply {
Expand Down Expand Up @@ -1133,6 +1135,7 @@ pub(super) async fn handle_client(
content,
images,
system_reminder,
active_skill,
},
&client_session_id,
&mut ProcessingState {
Expand Down Expand Up @@ -2850,6 +2853,7 @@ async fn start_processing_message(
content,
images,
system_reminder,
active_skill,
} = message;
if server_reload_starting() {
crate::logging::info(&format!(
Expand All @@ -2869,6 +2873,20 @@ async fn start_processing_message(
return;
}

if !agent
.lock()
.await
.set_remote_active_skill(active_skill.clone())
Comment on lines +2876 to +2879

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Shared active skill is overwritten between queued turns

active_skill is written to the session-wide Agent before the message task is spawned, while the task later reacquires that same agent lock to build and run the turn. Another client attached to the same session can submit a message with a different skill during that gap. In a deterministic two-request run, the first request selected skill-alpha, the second selected skill-beta, and both provider turns used beta. The first client can therefore receive a response generated with a different skill than it selected. Keep the selected skill request-scoped, or capture and apply it atomically inside the spawned turn before prompt construction.

Artifacts

Focused active-skill interleaving harness

  • Review-authored Rust test source that holds the shared mutex, queues alpha then beta requests, releases it, and records the skills observed by both real provider turns.

Focused interleaving execution output

  • Captured output from the executed Cargo test showing the observed values `[(false, true), (false, true)]` and exit code 0, confirming both turns used beta.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-app-core/src/server/client_lifecycle.rs
Line: 2876-2879

Comment:
**Shared active skill is overwritten between queued turns**

`active_skill` is written to the session-wide `Agent` before the message task is spawned, while the task later reacquires that same agent lock to build and run the turn. Another client attached to the same session can submit a message with a different skill during that gap. In a deterministic two-request run, the first request selected `skill-alpha`, the second selected `skill-beta`, and both provider turns used beta. The first client can therefore receive a response generated with a different skill than it selected. Keep the selected skill request-scoped, or capture and apply it atomically inside the spawned turn before prompt construction.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

{
let skill_name = active_skill.as_deref().unwrap_or_default();
let _ = client_event_tx.send(ServerEvent::Error {
id,
message: format!("Skill '{skill_name}' is not installed on the server"),
retry_after_secs: None,
});
return;
}

*state.client_is_processing = true;
*state.message_id = Some(id);
*state.session_id = Some(client_session_id.to_string());
Expand Down
4 changes: 4 additions & 0 deletions crates/jcode-app-core/src/server/client_lifecycle_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,7 @@ fn reload_starting_rejects_new_turn_without_spawning_processing_task() {
content: "do not start during reload".to_string(),
images: Vec::new(),
system_reminder: None,
active_skill: None,
},
"session_guard",
&mut ProcessingState {
Expand Down Expand Up @@ -1009,6 +1010,7 @@ async fn client_initiated_turn_fans_out_stream_and_terminal_events_to_live_attac
content: "stream to every attachment".to_string(),
images: Vec::new(),
system_reminder: None,
active_skill: None,
},
session_id,
&mut ProcessingState {
Expand Down Expand Up @@ -1133,6 +1135,7 @@ fn accepted_reload_recovery_continuation_marks_intent_delivered() -> anyhow::Res
content: "continue after reload".to_string(),
images: Vec::new(),
system_reminder: Some(continuation.to_string()),
active_skill: None,
},
session_id,
&mut ProcessingState {
Expand Down Expand Up @@ -1232,6 +1235,7 @@ fn reload_starting_rejects_new_turns_for_multiple_sessions() {
content: format!("do not start {session_id} during reload"),
images: Vec::new(),
system_reminder: None,
active_skill: None,
},
session_id,
&mut ProcessingState {
Expand Down
26 changes: 22 additions & 4 deletions crates/jcode-app-core/src/server/client_session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,12 @@ pub(super) async fn handle_clear_session(
// `/clear` creates a genuinely fresh session. Do not migrate the old
// session's swarm membership or plan participation to the replacement:
// doing so lets a subsequent plan snapshot repopulate the cleared UI.
let swarm_id_for_update = {
let (swarm_id_for_update, swarm_enabled, friendly_name) = {
let mut members = swarm_members.write().await;
members
.remove(client_session_id)
.and_then(|member| member.swarm_id)
match members.remove(client_session_id) {
Some(member) => (member.swarm_id, member.swarm_enabled, member.friendly_name),
None => (None, false, None),
}
};
if let Some(ref swarm_id) = swarm_id_for_update {
let mut swarms = swarms_by_id.write().await;
Expand All @@ -251,6 +252,23 @@ pub(super) async fn handle_clear_session(
channel_subscriptions_by_session,
)
.await;
// The connection remains subscribed across `/clear`, so there is no later
// subscribe request to register the replacement session. Register it as a
// fresh root while deliberately leaving the old swarm and plan behind.
ensure_client_swarm_member(
&new_id,
client_connection_id,
&friendly_name,
client_event_tx,
agent,
swarm_enabled,
swarm_members,
swarms_by_id,
event_history,
event_counter,
swarm_event_tx,
)
.await;
update_member_status(
&new_id,
"ready",
Expand Down
22 changes: 20 additions & 2 deletions crates/jcode-app-core/src/server/client_session_tests/clear.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,27 @@ async fn handle_clear_session_replaces_runtime_handles_and_updates_shutdown_regi
.await;

assert_ne!(client_session_id, old_session_id);
assert!(swarm_members.read().await.is_empty());
assert!(swarm_members.read().await.get(&client_session_id).is_none());
let members = swarm_members.read().await;
assert!(members.get(old_session_id).is_none());
let replacement_member = members
.get(&client_session_id)
.expect("replacement session should remain registered for swarm tools");
assert!(replacement_member.swarm_enabled);
assert_eq!(replacement_member.status, "ready");
assert_ne!(replacement_member.swarm_id.as_deref(), Some("swarm-test"));
let replacement_swarm_id = replacement_member
.swarm_id
.clone()
.expect("replacement session should get a fresh swarm identity");
drop(members);
assert!(swarms_by_id.read().await.get("swarm-test").is_none());
assert!(
swarms_by_id
.read()
.await
.get(&replacement_swarm_id)
.is_some_and(|sessions| sessions.contains(&client_session_id))
);
let plans = swarm_plans.read().await;
assert!(!plans["swarm-test"].participants.contains(old_session_id));
assert!(
Expand Down
1 change: 1 addition & 0 deletions crates/jcode-app-core/src/tool/communicate_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,7 @@ impl RawClient {
content: content.to_string(),
images: vec![],
system_reminder: None,
active_skill: None,
no_reply: false,
})
.await
Expand Down
15 changes: 12 additions & 3 deletions crates/jcode-protocol/src/protocol_tests/core_events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ fn test_request_roundtrip() -> Result<()> {
content: "hello".to_string(),
images: vec![],
system_reminder: None,
active_skill: Some("verification".to_string()),
no_reply: false,
};
let json = serde_json::to_string(&req)?;
Expand Down Expand Up @@ -33,7 +34,10 @@ fn test_soft_interrupt_images_roundtrip_and_legacy_default() -> Result<()> {
return Err(anyhow!("wrong request type"));
};
assert_eq!(content, "look at this");
assert_eq!(images, vec![("image/png".to_string(), "ZmFrZQ==".to_string())]);
assert_eq!(
images,
vec![("image/png".to_string(), "ZmFrZQ==".to_string())]
);
assert!(urgent);

let legacy = r#"{"type":"soft_interrupt","id":3,"content":"legacy","urgent":false}"#;
Expand Down Expand Up @@ -144,10 +148,15 @@ fn test_notify_auth_changed_typed_auth_payload_roundtrip() -> Result<()> {
assert!(!prefer_strongest);
let auth = auth.expect("typed auth payload should roundtrip");
assert_eq!(auth.provider.as_str(), "cerebras");
assert_eq!(auth.credential_source, Some(AuthCredentialSource::ApiKeyFile));
assert_eq!(
auth.credential_source,
Some(AuthCredentialSource::ApiKeyFile)
);
assert_eq!(auth.auth_method, Some(AuthMethod::RemoteTuiPasteApiKey));
assert_eq!(
auth.expected_runtime.as_ref().map(RuntimeProviderKey::as_str),
auth.expected_runtime
.as_ref()
.map(RuntimeProviderKey::as_str),
Some("openai-compatible")
);
assert_eq!(
Expand Down
7 changes: 4 additions & 3 deletions crates/jcode-protocol/src/protocol_tests/misc_events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ fn test_message_request_roundtrip_preserves_images_and_system_reminder() -> Resu
("image/jpeg".to_string(), "BBB".to_string()),
],
system_reminder: Some("be concise".to_string()),
active_skill: Some("verification".to_string()),
no_reply: true,
};
let json = serde_json::to_string(&req)?;
Expand All @@ -425,6 +426,7 @@ fn test_message_request_roundtrip_preserves_images_and_system_reminder() -> Resu
content,
images,
system_reminder,
active_skill,
no_reply,
} = decoded
else {
Expand All @@ -436,6 +438,7 @@ fn test_message_request_roundtrip_preserves_images_and_system_reminder() -> Resu
assert_eq!(images[0].0, "image/png");
assert_eq!(images[1].0, "image/jpeg");
assert_eq!(system_reminder.as_deref(), Some("be concise"));
assert_eq!(active_skill.as_deref(), Some("verification"));
assert!(no_reply);
Ok(())
}
Expand All @@ -460,9 +463,7 @@ fn test_provider_guardrail_event_roundtrip() -> Result<()> {
assert_eq!(message, "Provider guardrail stopped the response");

// stop_reason is optional on the wire.
let decoded = parse_event_json(
r#"{"type":"provider_guardrail","message":"blocked"}"#,
)?;
let decoded = parse_event_json(r#"{"type":"provider_guardrail","message":"blocked"}"#)?;
let ServerEvent::ProviderGuardrail { stop_reason, .. } = decoded else {
return Err(anyhow!("expected ProviderGuardrail event"));
};
Expand Down
7 changes: 6 additions & 1 deletion crates/jcode-protocol/src/protocol_tests/randomized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ fn test_protocol_request_roundtrip_randomized_samples() -> Result<()> {
content: content.clone(),
images: images.clone(),
system_reminder: system_reminder.clone(),
active_skill: None,
no_reply: rng.random_bool(0.5),
};
let decoded = parse_request_json(&serde_json::to_string(&req)?)?;
Expand All @@ -37,6 +38,7 @@ fn test_protocol_request_roundtrip_randomized_samples() -> Result<()> {
images: decoded_images,
system_reminder: decoded_system_reminder,
no_reply: decoded_no_reply,
..
} = decoded
else {
return Err(anyhow!("expected randomized Message"));
Expand All @@ -45,7 +47,10 @@ fn test_protocol_request_roundtrip_randomized_samples() -> Result<()> {
assert_eq!(decoded_content, content);
assert_eq!(decoded_images, images);
assert_eq!(decoded_system_reminder, system_reminder);
assert_eq!(decoded_no_reply, matches!(req, Request::Message { no_reply: true, .. }));
assert_eq!(
decoded_no_reply,
matches!(req, Request::Message { no_reply: true, .. })
);
}

for id in 100..132u64 {
Expand Down
4 changes: 4 additions & 0 deletions crates/jcode-protocol/src/wire.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ pub enum Request {
images: Vec<(String, String)>,
#[serde(default, skip_serializing_if = "Option::is_none")]
system_reminder: Option<String>,
/// Skill selected by the client for this and subsequent turns. The
/// daemon resolves the name against its own skill registry.
#[serde(default, skip_serializing_if = "Option::is_none")]
active_skill: Option<String>,
/// Append the user message as context only. The daemon persists it and
/// acknowledges it without starting a model turn.
#[serde(default, skip_serializing_if = "is_false")]
Expand Down
3 changes: 2 additions & 1 deletion crates/jcode-tui/src/tui/app/remote/input_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ pub(in crate::tui::app) async fn begin_remote_send(
retry_attempts: u8,
) -> Result<u64> {
let msg_id = remote
.send_message_with_images_and_reminder(
.send_message_with_images_reminder_and_skill(
content.clone(),
images.clone(),
system_reminder.clone(),
app.active_skill.clone(),
)
.await?;
app.current_message_id = Some(msg_id);
Expand Down
12 changes: 12 additions & 0 deletions crates/jcode-tui/src/tui/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,17 @@ impl RemoteConnection {
content: String,
images: Vec<(String, String)>,
system_reminder: Option<String>,
) -> Result<u64> {
self.send_message_with_images_reminder_and_skill(content, images, system_reminder, None)
.await
}

pub async fn send_message_with_images_reminder_and_skill(
&mut self,
content: String,
images: Vec<(String, String)>,
system_reminder: Option<String>,
active_skill: Option<String>,
) -> Result<u64> {
// Output token usage snapshots are cumulative within a single API call.
// Reset per-call watermark before sending the next user request.
Expand All @@ -562,6 +573,7 @@ impl RemoteConnection {
content,
images,
system_reminder,
active_skill,
no_reply: false,
};
self.next_request_id += 1;
Expand Down
Loading