mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
app-server: use permission ids and runtime workspace roots (#22611)
## Why This PR builds on [#22610](https://github.com/openai/codex/pull/22610) and is the app-server side of the migration from mutable per-turn `SandboxPolicy` replacement toward selecting immutable permission profiles by id plus mutable runtime workspace roots. Once permission profiles can carry their own immutable `workspace_roots`, app-server no longer needs to mutate the selected `PermissionProfile` just to represent thread-specific filesystem context. The mutable part now lives on the thread as explicit `runtimeWorkspaceRoots`, while `:workspace_roots` remains symbolic until the sandbox is realized for a turn. ## What Changed - Replaced the v2 permission-selection wrapper surface with plain profile ids for `thread/start`, `thread/resume`, `thread/fork`, and `turn/start`. - Removed the API surface for profile modifications (`PermissionProfileSelectionParams`, `PermissionProfileModificationParams`, `ActivePermissionProfileModification`). - Added experimental `runtimeWorkspaceRoots` fields to the thread lifecycle and turn-start APIs. - Threaded runtime workspace roots through core session/thread snapshots, turn overrides, app-server request handling, and command execution permission resolution. - Kept session permission state symbolic so later runtime root updates and cwd-only implicit-root retargeting rebind `:workspace_roots` correctly. - Updated the embedded clients just enough to send and restore the new thread state. - Refreshed the generated schema/TypeScript artifacts and the app-server README to match the new contract. ## Verification Targeted coverage for this layer lives in: - `codex-rs/app-server-protocol/src/protocol/v2/tests.rs` - `codex-rs/app-server/tests/suite/v2/thread_start.rs` - `codex-rs/app-server/tests/suite/v2/thread_resume.rs` - `codex-rs/app-server/tests/suite/v2/turn_start.rs` - `codex-rs/core/src/session/tests.rs` The key regression checks exercise that: - `runtimeWorkspaceRoots` resolve against the effective cwd on thread start. - Profile-declared workspace roots are excluded from the runtime workspace roots returned by app-server. - A turn-level runtime workspace-root update persists onto the thread and is returned by `thread/resume`. - A named permission profile selected on one turn remains symbolic so a later runtime-root-only turn update changes the actual sandbox writes. - A cwd-only turn update retargets the implicit runtime cwd root while preserving additional runtime roots. - The protocol fixtures and generated client artifacts stay in sync with the string-based permission selection contract. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22611). * #22612 * __->__ #22611
This commit is contained in:
committed by
GitHub
Unverified
parent
e6a7368810
commit
8a5306ff88
@@ -34,7 +34,6 @@ use codex_app_server_protocol::MemoryResetResponse;
|
||||
use codex_app_server_protocol::Model as ApiModel;
|
||||
use codex_app_server_protocol::ModelListParams;
|
||||
use codex_app_server_protocol::ModelListResponse;
|
||||
use codex_app_server_protocol::PermissionProfileModificationParams;
|
||||
use codex_app_server_protocol::PermissionProfileSelectionParams;
|
||||
use codex_app_server_protocol::RateLimitSnapshot;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
@@ -577,6 +576,12 @@ impl AppServerSession {
|
||||
responsesapi_client_metadata: None,
|
||||
environments: None,
|
||||
cwd: Some(cwd),
|
||||
runtime_workspace_roots: Some(
|
||||
workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect(),
|
||||
),
|
||||
approval_policy: Some(approval_policy),
|
||||
approvals_reviewer: Some(approvals_reviewer.into()),
|
||||
sandbox_policy,
|
||||
@@ -1175,34 +1180,22 @@ fn sandbox_mode_from_permission_profile(
|
||||
|
||||
fn permissions_selection_from_active_profile(
|
||||
active: ActivePermissionProfile,
|
||||
cwd: &std::path::Path,
|
||||
workspace_roots: &[AbsolutePathBuf],
|
||||
) -> PermissionProfileSelectionParams {
|
||||
let modifications = workspace_roots
|
||||
.iter()
|
||||
.filter(|root| root.as_path() != cwd)
|
||||
.cloned()
|
||||
.map(|path| PermissionProfileModificationParams::AdditionalWritableRoot { path })
|
||||
.collect::<Vec<_>>();
|
||||
PermissionProfileSelectionParams::Profile {
|
||||
id: active.id,
|
||||
modifications: (!modifications.is_empty()).then_some(modifications),
|
||||
}
|
||||
PermissionProfileSelectionParams::new(active.id)
|
||||
}
|
||||
|
||||
fn turn_permissions_overrides(
|
||||
permission_profile: &PermissionProfile,
|
||||
active_permission_profile: Option<ActivePermissionProfile>,
|
||||
cwd: &std::path::Path,
|
||||
workspace_roots: &[AbsolutePathBuf],
|
||||
_workspace_roots: &[AbsolutePathBuf],
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
) -> (
|
||||
Option<codex_app_server_protocol::SandboxPolicy>,
|
||||
Option<PermissionProfileSelectionParams>,
|
||||
) {
|
||||
let permissions = if matches!(thread_params_mode, ThreadParamsMode::Embedded) {
|
||||
active_permission_profile
|
||||
.map(|active| permissions_selection_from_active_profile(active, cwd, workspace_roots))
|
||||
active_permission_profile.map(permissions_selection_from_active_profile)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
@@ -1231,13 +1224,7 @@ fn permissions_selection_from_config(
|
||||
config
|
||||
.permissions
|
||||
.active_permission_profile()
|
||||
.map(|active| {
|
||||
permissions_selection_from_active_profile(
|
||||
active,
|
||||
config.cwd.as_path(),
|
||||
config.permissions.user_visible_workspace_roots(),
|
||||
)
|
||||
})
|
||||
.map(permissions_selection_from_active_profile)
|
||||
}
|
||||
|
||||
fn thread_start_params_from_config(
|
||||
@@ -1261,6 +1248,13 @@ fn thread_start_params_from_config(
|
||||
model_provider: thread_params_mode.model_provider_from_config(config),
|
||||
service_tier: service_tier_override_from_config(config),
|
||||
cwd: thread_cwd_from_config(config, thread_params_mode, remote_cwd_override),
|
||||
runtime_workspace_roots: Some(
|
||||
config
|
||||
.workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect(),
|
||||
),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(config),
|
||||
sandbox,
|
||||
@@ -1296,6 +1290,13 @@ fn thread_resume_params_from_config(
|
||||
model_provider: thread_params_mode.model_provider_from_config(&config),
|
||||
service_tier: service_tier_override_from_config(&config),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode, remote_cwd_override),
|
||||
runtime_workspace_roots: Some(
|
||||
config
|
||||
.workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect(),
|
||||
),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(&config),
|
||||
sandbox,
|
||||
@@ -1328,6 +1329,13 @@ fn thread_fork_params_from_config(
|
||||
model_provider: thread_params_mode.model_provider_from_config(&config),
|
||||
service_tier: service_tier_override_from_config(&config),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode, remote_cwd_override),
|
||||
runtime_workspace_roots: Some(
|
||||
config
|
||||
.workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect(),
|
||||
),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(&config),
|
||||
sandbox,
|
||||
@@ -1425,6 +1433,7 @@ async fn thread_session_state_from_thread_start_response(
|
||||
permission_profile,
|
||||
response.active_permission_profile.clone().map(Into::into),
|
||||
response.cwd.clone(),
|
||||
response.runtime_workspace_roots.clone(),
|
||||
response.instruction_sources.clone(),
|
||||
response.reasoning_effort,
|
||||
config,
|
||||
@@ -1457,6 +1466,7 @@ async fn thread_session_state_from_thread_resume_response(
|
||||
permission_profile,
|
||||
response.active_permission_profile.clone().map(Into::into),
|
||||
response.cwd.clone(),
|
||||
response.runtime_workspace_roots.clone(),
|
||||
response.instruction_sources.clone(),
|
||||
response.reasoning_effort,
|
||||
config,
|
||||
@@ -1489,6 +1499,7 @@ async fn thread_session_state_from_thread_fork_response(
|
||||
permission_profile,
|
||||
response.active_permission_profile.clone().map(Into::into),
|
||||
response.cwd.clone(),
|
||||
response.runtime_workspace_roots.clone(),
|
||||
response.instruction_sources.clone(),
|
||||
response.reasoning_effort,
|
||||
config,
|
||||
@@ -1531,6 +1542,7 @@ async fn thread_session_state_from_thread_response(
|
||||
permission_profile: PermissionProfile,
|
||||
active_permission_profile: Option<ActivePermissionProfile>,
|
||||
cwd: AbsolutePathBuf,
|
||||
runtime_workspace_roots: Vec<AbsolutePathBuf>,
|
||||
instruction_source_paths: Vec<AbsolutePathBuf>,
|
||||
reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
|
||||
config: &Config,
|
||||
@@ -1558,6 +1570,7 @@ async fn thread_session_state_from_thread_response(
|
||||
permission_profile,
|
||||
active_permission_profile,
|
||||
cwd,
|
||||
runtime_workspace_roots,
|
||||
instruction_source_paths,
|
||||
reasoning_effort,
|
||||
message_history: Some(MessageHistoryMetadata {
|
||||
@@ -1637,19 +1650,23 @@ mod tests {
|
||||
);
|
||||
|
||||
assert_eq!(params.cwd, Some(config.cwd.to_string_lossy().to_string()));
|
||||
assert_eq!(
|
||||
params.runtime_workspace_roots,
|
||||
Some(
|
||||
config
|
||||
.workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect()
|
||||
)
|
||||
);
|
||||
assert_eq!(params.sandbox, None);
|
||||
assert_eq!(
|
||||
params.permissions,
|
||||
config
|
||||
.permissions
|
||||
.active_permission_profile()
|
||||
.map(|active| {
|
||||
permissions_selection_from_active_profile(
|
||||
active,
|
||||
config.cwd.as_path(),
|
||||
config.permissions.user_visible_workspace_roots(),
|
||||
)
|
||||
})
|
||||
.map(permissions_selection_from_active_profile)
|
||||
);
|
||||
assert_eq!(params.model_provider, Some(config.model_provider_id));
|
||||
assert_eq!(params.thread_source, Some(ThreadSource::User));
|
||||
@@ -1676,11 +1693,8 @@ mod tests {
|
||||
let active_permission_profile =
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE);
|
||||
let workspace_roots = vec![cwd.clone()];
|
||||
let expected_permissions = permissions_selection_from_active_profile(
|
||||
active_permission_profile.clone(),
|
||||
cwd.as_path(),
|
||||
&workspace_roots,
|
||||
);
|
||||
let expected_permissions =
|
||||
permissions_selection_from_active_profile(active_permission_profile.clone());
|
||||
|
||||
let (sandbox_policy, permissions) = turn_permissions_overrides(
|
||||
&PermissionProfile::workspace_write(),
|
||||
@@ -1695,12 +1709,12 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn embedded_turn_permissions_include_extra_workspace_roots_as_modifications() {
|
||||
fn embedded_turn_permissions_select_profile_id_only() {
|
||||
let cwd = test_path_buf("/workspace/project").abs();
|
||||
let extra_root = test_path_buf("/workspace/cache").abs();
|
||||
let active_permission_profile =
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE);
|
||||
let workspace_roots = vec![cwd.clone(), extra_root.clone()];
|
||||
let workspace_roots = vec![cwd.clone(), extra_root];
|
||||
|
||||
let (sandbox_policy, permissions) = turn_permissions_overrides(
|
||||
&PermissionProfile::workspace_write(),
|
||||
@@ -1713,14 +1727,9 @@ mod tests {
|
||||
assert_eq!(sandbox_policy, None);
|
||||
assert_eq!(
|
||||
permissions,
|
||||
Some(PermissionProfileSelectionParams::Profile {
|
||||
id: BUILT_IN_PERMISSION_PROFILE_WORKSPACE.to_string(),
|
||||
modifications: Some(vec![
|
||||
PermissionProfileModificationParams::AdditionalWritableRoot {
|
||||
path: extra_root
|
||||
}
|
||||
]),
|
||||
})
|
||||
Some(PermissionProfileSelectionParams::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE
|
||||
))
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1777,6 +1786,13 @@ mod tests {
|
||||
&config.permissions.effective_permission_profile(),
|
||||
config.cwd.as_path(),
|
||||
);
|
||||
let expected_runtime_workspace_roots = Some(
|
||||
config
|
||||
.workspace_roots
|
||||
.iter()
|
||||
.map(AbsolutePathBuf::to_path_buf)
|
||||
.collect::<Vec<_>>(),
|
||||
);
|
||||
|
||||
let start = thread_start_params_from_config(
|
||||
&config,
|
||||
@@ -1800,6 +1816,18 @@ mod tests {
|
||||
assert_eq!(start.cwd, None);
|
||||
assert_eq!(resume.cwd, None);
|
||||
assert_eq!(fork.cwd, None);
|
||||
assert_eq!(
|
||||
start.runtime_workspace_roots,
|
||||
expected_runtime_workspace_roots
|
||||
);
|
||||
assert_eq!(
|
||||
resume.runtime_workspace_roots,
|
||||
expected_runtime_workspace_roots
|
||||
);
|
||||
assert_eq!(
|
||||
fork.runtime_workspace_roots,
|
||||
expected_runtime_workspace_roots
|
||||
);
|
||||
assert_eq!(start.model_provider, None);
|
||||
assert_eq!(resume.model_provider, None);
|
||||
assert_eq!(fork.model_provider, None);
|
||||
@@ -2070,6 +2098,10 @@ mod tests {
|
||||
model_provider: "openai".to_string(),
|
||||
service_tier: None,
|
||||
cwd: test_path_buf("/tmp/project").abs(),
|
||||
runtime_workspace_roots: vec![
|
||||
test_path_buf("/tmp/project").abs(),
|
||||
test_path_buf("/tmp/project/extra").abs(),
|
||||
],
|
||||
instruction_sources: vec![test_path_buf("/tmp/project/AGENTS.md").abs()],
|
||||
approval_policy: codex_app_server_protocol::AskForApproval::Never,
|
||||
approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::User,
|
||||
@@ -2090,6 +2122,10 @@ mod tests {
|
||||
.await
|
||||
.expect("resume response should map");
|
||||
assert_eq!(started.session.forked_from_id, Some(forked_from_id));
|
||||
assert_eq!(
|
||||
started.session.runtime_workspace_roots,
|
||||
response.runtime_workspace_roots
|
||||
);
|
||||
assert_eq!(
|
||||
started.session.instruction_source_paths,
|
||||
response.instruction_sources
|
||||
@@ -2097,6 +2133,17 @@ mod tests {
|
||||
assert_eq!(started.session.permission_profile, read_only_profile);
|
||||
assert_eq!(started.turns.len(), 1);
|
||||
assert_eq!(started.turns[0], response.thread.turns[0]);
|
||||
|
||||
let mut empty_roots_response = response;
|
||||
empty_roots_response.runtime_workspace_roots = Vec::new();
|
||||
let started = started_thread_from_resume_response(
|
||||
empty_roots_response,
|
||||
&config,
|
||||
ThreadParamsMode::Remote,
|
||||
)
|
||||
.await
|
||||
.expect("resume response should map");
|
||||
assert_eq!(started.session.runtime_workspace_roots, Vec::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -2193,6 +2240,7 @@ mod tests {
|
||||
/*active_permission_profile*/ None,
|
||||
test_path_buf("/tmp/project").abs(),
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
/*reasoning_effort*/ None,
|
||||
&config,
|
||||
)
|
||||
@@ -2227,6 +2275,7 @@ mod tests {
|
||||
/*active_permission_profile*/ None,
|
||||
test_path_buf("/tmp/project").abs(),
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
/*reasoning_effort*/ None,
|
||||
&config,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user