Skip to content

Fix workspace restore idempotency and strict RPC parsing #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jmcte wants to merge 2 commits into from
Closed

Fix workspace restore idempotency and strict RPC parsing #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0eb580b
Fix workspace restore, connect, and RPC parsing robustness
jmcte Feb 20, 2026
29777b5
Merge branch 'main' into pr/470
Dimillian Feb 20, 2026
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
46 changes: 46 additions & 0 deletions src-tauri/src/bin/codex_monitor_daemon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1685,6 +1685,52 @@ mod tests {
});
}

#[test]
fn rpc_open_workspace_in_invalid_args_is_rejected() {
run_async_test(async {
let tmp = make_temp_dir("rpc-open-workspace-in-invalid-args");
let state = test_state(&tmp);

let err = rpc::handle_rpc_request(
&state,
"open_workspace_in",
json!({
"path": "/tmp/repo",
"args": [1, 2],
}),
"daemon-test".to_string(),
)
.await
.expect_err("invalid args should be rejected");

assert_eq!(err, "invalid `args`");
let _ = std::fs::remove_dir_all(&tmp);
});
}

#[test]
fn rpc_list_git_roots_invalid_depth_is_rejected() {
run_async_test(async {
let tmp = make_temp_dir("rpc-list-git-roots-invalid-depth");
let state = test_state(&tmp);

let err = rpc::handle_rpc_request(
&state,
"list_git_roots",
json!({
"workspaceId": "ws-invalid-depth",
"depth": "10",
}),
"daemon-test".to_string(),
)
.await
.expect_err("invalid depth should be rejected");

assert_eq!(err, "invalid `depth`");
let _ = std::fs::remove_dir_all(&tmp);
});
}

#[test]
fn rpc_daemon_info_reports_identity() {
run_async_test(async {
Expand Down
102 changes: 83 additions & 19 deletions src-tauri/src/bin/codex_monitor_daemon/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,22 @@ pub(super) fn parse_optional_string(value: &Value, key: &str) -> Option<String>
}
}

pub(super) fn parse_optional_u32(value: &Value, key: &str) -> Option<u32> {
pub(super) fn parse_optional_u32(value: &Value, key: &str) -> Result<Option<u32>, String> {
match value {
Value::Object(map) => map.get(key).and_then(|value| value.as_u64()).and_then(|v| {
if v > u32::MAX as u64 {
None
} else {
Some(v as u32)
Value::Object(map) => match map.get(key) {
None => Ok(None),
Some(value) => {
let raw = value
.as_u64()
.filter(|value| *value <= u32::MAX as u64)
.map(|value| value as u32);
match raw {
Some(value) => Ok(Some(value)),
None => Err(format!("invalid `{key}`")),
}
}
}),
_ => None,
},
_ => Err(format!("invalid `{key}`")),
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 20, 2026

Choose a reason for hiding this comment

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

P2 Badge Treat null RPC params as absent for optional u32 parsing

The transport layer uses Value::Null when a JSON-RPC request omits params (src-tauri/src/bin/codex_monitor_daemon/transport.rs), but this parser now returns Err for any non-object, so methods with only optional numeric inputs (like local_usage_snapshot) fail with invalid \days`when callers send no params. This is a backward-incompatible regression from the previous behavior where omitted optional values were accepted asNone`.

Useful? React with 👍 / 👎.

}
}

Expand All @@ -105,23 +111,33 @@ pub(super) fn parse_optional_bool(value: &Value, key: &str) -> Option<bool> {
}
}

pub(super) fn parse_optional_string_array(value: &Value, key: &str) -> Option<Vec<String>> {
pub(super) fn parse_optional_string_array(
value: &Value,
key: &str,
) -> Result<Option<Vec<String>>, String> {
match value {
Value::Object(map) => map
.get(key)
.and_then(|value| value.as_array())
.map(|items| {
items
Value::Object(map) => match map.get(key) {
None => Ok(None),
Some(value) => {
let Some(items) = value.as_array() else {
return Err(format!("invalid `{key}`"));
};
let parsed_items = items
.iter()
.filter_map(|item| item.as_str().map(|value| value.to_string()))
.collect::<Vec<_>>()
}),
_ => None,
.map(|item| item.as_str().map(|value| value.to_string()))
.collect::<Option<Vec<_>>>();
match parsed_items {
Some(items) => Ok(Some(items)),
None => Err(format!("invalid `{key}`")),
}
}
},
_ => Err(format!("invalid `{key}`")),
}
}

pub(super) fn parse_string_array(value: &Value, key: &str) -> Result<Vec<String>, String> {
parse_optional_string_array(value, key).ok_or_else(|| format!("missing `{key}`"))
parse_optional_string_array(value, key)?.ok_or_else(|| format!("missing `{key}`"))
}

pub(super) fn parse_optional_value(value: &Value, key: &str) -> Option<Value> {
Expand Down Expand Up @@ -184,3 +200,51 @@ pub(super) fn spawn_rpc_response_task(
}
});
}

#[cfg(test)]
mod tests {
use serde_json::json;

#[test]
fn parse_optional_u32_rejects_non_numeric_values() {
let err = super::parse_optional_u32(&json!({ "limit": "20" }), "limit")
.expect_err("limit should be invalid");
assert_eq!(err, "invalid `limit`");
}

#[test]
fn parse_optional_u32_rejects_overflow_values() {
let err = super::parse_optional_u32(&json!({ "limit": 4294967296u64 }), "limit")
.expect_err("limit should overflow u32");
assert_eq!(err, "invalid `limit`");
}

#[test]
fn parse_optional_u32_allows_missing_value() {
let value = super::parse_optional_u32(&json!({ "depth": 5 }), "limit")
.expect("parse should succeed");
assert!(value.is_none());
}

#[test]
fn parse_optional_string_array_rejects_non_array_values() {
let err = super::parse_optional_string_array(&json!({ "images": "banner.png" }), "images")
.expect_err("images should be an array");
assert_eq!(err, "invalid `images`");
}

#[test]
fn parse_optional_string_array_rejects_mixed_type_items() {
let err =
super::parse_optional_string_array(&json!({ "images": ["image.png", 5] }), "images")
.expect_err("images should only contain strings");
assert_eq!(err, "invalid `images`");
}

#[test]
fn parse_optional_string_array_allows_missing_value() {
let value = super::parse_optional_string_array(&json!({ "images": ["image.png"] }), "args")
.expect("parse should succeed");
assert!(value.is_none());
}
}
36 changes: 29 additions & 7 deletions src-tauri/src/bin/codex_monitor_daemon/rpc/codex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ pub(super) async fn try_handle(
Err(err) => return Some(Err(err)),
};
let cursor = parse_optional_string(params, "cursor");
let limit = parse_optional_u32(params, "limit");
let limit = match parse_optional_u32(params, "limit") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let sort_key = parse_optional_string(params, "sortKey");
let cwd = parse_optional_string(params, "cwd");
Some(
Expand All @@ -102,7 +105,10 @@ pub(super) async fn try_handle(
Err(err) => return Some(Err(err)),
};
let cursor = parse_optional_string(params, "cursor");
let limit = parse_optional_u32(params, "limit");
let limit = match parse_optional_u32(params, "limit") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
Some(
state
.list_mcp_server_status(workspace_id, cursor, limit)
Expand Down Expand Up @@ -162,7 +168,10 @@ pub(super) async fn try_handle(
let model = parse_optional_string(params, "model");
let effort = parse_optional_string(params, "effort");
let access_mode = parse_optional_string(params, "accessMode");
let images = parse_optional_string_array(params, "images");
let images = match parse_optional_string_array(params, "images") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let app_mentions = parse_optional_value(params, "appMentions")
.and_then(|value| value.as_array().cloned());
let collaboration_mode = parse_optional_value(params, "collaborationMode");
Expand Down Expand Up @@ -214,7 +223,10 @@ pub(super) async fn try_handle(
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let images = parse_optional_string_array(params, "images");
let images = match parse_optional_string_array(params, "images") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let app_mentions = parse_optional_value(params, "appMentions")
.and_then(|value| value.as_array().cloned());
Some(
Expand Down Expand Up @@ -261,8 +273,15 @@ pub(super) async fn try_handle(
Err(err) => return Some(Err(err)),
};
let cursor = parse_optional_string(params, "cursor");
let limit = parse_optional_u32(params, "limit");
Some(state.experimental_feature_list(workspace_id, cursor, limit).await)
let limit = match parse_optional_u32(params, "limit") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
Some(
state
.experimental_feature_list(workspace_id, cursor, limit)
.await,
)
}
"collaboration_mode_list" => {
let workspace_id = match parse_string(params, "workspaceId") {
Expand Down Expand Up @@ -410,7 +429,10 @@ pub(super) async fn try_handle(
Err(err) => return Some(Err(err)),
};
let cursor = parse_optional_string(params, "cursor");
let limit = parse_optional_u32(params, "limit");
let limit = match parse_optional_u32(params, "limit") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let thread_id = parse_optional_string(params, "threadId");
Some(
state
Expand Down
10 changes: 8 additions & 2 deletions src-tauri/src/bin/codex_monitor_daemon/rpc/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ pub(super) async fn try_handle(
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let depth = parse_optional_u32(params, "depth").map(|value| value as usize);
let depth = match parse_optional_u32(params, "depth") {
Ok(value) => value.map(|value| value as usize),
Err(err) => return Some(Err(err)),
};
let roots = match state.list_git_roots(workspace_id, depth).await {
Ok(value) => value,
Err(err) => return Some(Err(err)),
Expand All @@ -73,7 +76,10 @@ pub(super) async fn try_handle(
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let limit = parse_optional_u32(params, "limit").map(|value| value as usize);
let limit = match parse_optional_u32(params, "limit") {
Ok(value) => value.map(|value| value as usize),
Err(err) => return Some(Err(err)),
};
let log = match state.get_git_log(workspace_id, limit).await {
Ok(value) => value,
Err(err) => return Some(Err(err)),
Expand Down
10 changes: 8 additions & 2 deletions src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,10 @@ pub(super) async fn try_handle(
};
let app = parse_optional_string(params, "app");
let command = parse_optional_string(params, "command");
let args = parse_optional_string_array(params, "args").unwrap_or_default();
let args = match parse_optional_string_array(params, "args") {
Ok(value) => value.unwrap_or_default(),
Err(err) => return Some(Err(err)),
};
Some(
state
.open_workspace_in(path, app, args, command)
Expand All @@ -402,7 +405,10 @@ pub(super) async fn try_handle(
Some(serde_json::to_value(icon).map_err(|err| err.to_string()))
}
"local_usage_snapshot" => {
let days = parse_optional_u32(params, "days");
let days = match parse_optional_u32(params, "days") {
Ok(value) => value,
Err(err) => return Some(Err(err)),
};
let workspace_path = parse_optional_string(params, "workspacePath");
let snapshot = match state.local_usage_snapshot(days, workspace_path).await {
Ok(value) => value,
Expand Down
17 changes: 16 additions & 1 deletion src-tauri/src/shared/workspaces_core/connect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ where
F: Fn(WorkspaceEntry, Option<String>, Option<String>, Option<PathBuf>) -> Fut,
Fut: Future<Output = Result<Arc<WorkspaceSession>, String>>,
{
{
let sessions = sessions.lock().await;
if sessions.contains_key(&workspace_id) {
return Ok(());
}
}

let (entry, parent_entry) = resolve_entry_and_parent(workspaces, &workspace_id).await?;
let (default_bin, codex_args) = {
let settings = app_settings.lock().await;
Expand All @@ -34,7 +41,15 @@ where
};
let codex_home = resolve_workspace_codex_home(&entry, parent_entry.as_ref());
let session = spawn_session(entry.clone(), default_bin, codex_args, codex_home).await?;
sessions.lock().await.insert(entry.id, session);
{
let mut sessions = sessions.lock().await;
if sessions.contains_key(&entry.id) {
let mut child = session.child.lock().await;
kill_child_process_tree(&mut child).await;
return Ok(());
}
sessions.insert(entry.id, session);
}
Ok(())
}

Expand Down
Loading
Loading