mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Apply argument comment lint across codex-rs (#14652)
## Why Once the repo-local lint exists, `codex-rs` needs to follow the checked-in convention and CI needs to keep it from drifting. This commit applies the fallback `/*param*/` style consistently across existing positional literal call sites without changing those APIs. The longer-term preference is still to avoid APIs that require comments by choosing clearer parameter types and call shapes. This PR is intentionally the mechanical follow-through for the places where the existing signatures stay in place. After rebasing onto newer `main`, the rollout also had to cover newly introduced `tui_app_server` call sites. That made it clear the first cut of the CI job was too expensive for the common path: it was spending almost as much time installing `cargo-dylint` and re-testing the lint crate as a representative test job spends running product tests. The CI update keeps the full workspace enforcement but trims that extra overhead from ordinary `codex-rs` PRs. ## What changed - keep a dedicated `argument_comment_lint` job in `rust-ci` - mechanically annotate remaining opaque positional literals across `codex-rs` with exact `/*param*/` comments, including the rebased `tui_app_server` call sites that now fall under the lint - keep the checked-in style aligned with the lint policy by using `/*param*/` and leaving string and char literals uncommented - cache `cargo-dylint`, `dylint-link`, and the relevant Cargo registry/git metadata in the lint job - split changed-path detection so the lint crate's own `cargo test` step runs only when `tools/argument-comment-lint/*` or `rust-ci.yml` changes - continue to run the repo wrapper over the `codex-rs` workspace, so product-code enforcement is unchanged Most of the code changes in this commit are intentionally mechanical comment rewrites or insertions driven by the lint itself. ## Verification - `./tools/argument-comment-lint/run.sh --workspace` - `cargo test -p codex-tui-app-server -p codex-tui` - parsed `.github/workflows/rust-ci.yml` locally with PyYAML --- * -> #14652 * #14651
This commit is contained in:
committed by
GitHub
Unverified
parent
6f05d8d735
commit
b77fe8fefe
@@ -78,7 +78,7 @@ pub(crate) fn typed_request_span(
|
||||
.or(session.client_version.as_deref()),
|
||||
);
|
||||
|
||||
attach_parent_context(&span, &method, request.id(), None);
|
||||
attach_parent_context(&span, &method, request.id(), /*parent_trace*/ None);
|
||||
span
|
||||
}
|
||||
|
||||
|
||||
@@ -1988,7 +1988,7 @@ async fn handle_turn_interrupted(
|
||||
conversation_id,
|
||||
event_turn_id,
|
||||
TurnStatus::Interrupted,
|
||||
None,
|
||||
/*error*/ None,
|
||||
outgoing,
|
||||
)
|
||||
.await;
|
||||
@@ -2379,7 +2379,7 @@ fn render_review_output_text(output: &ReviewOutputEvent) -> String {
|
||||
sections.push(explanation.to_string());
|
||||
}
|
||||
if !output.findings.is_empty() {
|
||||
let findings = format_review_findings_block(&output.findings, None);
|
||||
let findings = format_review_findings_block(&output.findings, /*selection*/ None);
|
||||
let trimmed = findings.trim();
|
||||
if !trimmed.is_empty() {
|
||||
sections.push(trimmed.to_string());
|
||||
@@ -2577,7 +2577,7 @@ async fn on_command_execution_request_approval_response(
|
||||
item_id.clone(),
|
||||
completion_item.command,
|
||||
completion_item.cwd,
|
||||
None,
|
||||
/*process_id*/ None,
|
||||
completion_item.command_actions,
|
||||
status,
|
||||
&outgoing,
|
||||
|
||||
@@ -446,7 +446,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
|
||||
pub(crate) async fn maybe_start_curated_repo_sync_for_latest_config(&self) {
|
||||
match self.load_latest_config(None).await {
|
||||
match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => self
|
||||
.thread_manager
|
||||
.plugins_manager()
|
||||
@@ -1623,7 +1623,10 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
|
||||
let cwd = cwd.unwrap_or_else(|| self.config.cwd.clone());
|
||||
let mut env = create_env(&self.config.permissions.shell_environment_policy, None);
|
||||
let mut env = create_env(
|
||||
&self.config.permissions.shell_environment_policy,
|
||||
/*thread_id*/ None,
|
||||
);
|
||||
if let Some(env_overrides) = env_overrides {
|
||||
for (key, value) in env_overrides {
|
||||
match value {
|
||||
@@ -1659,8 +1662,8 @@ impl CodexMessageProcessor {
|
||||
Some(spec) => match spec
|
||||
.start_proxy(
|
||||
self.config.permissions.sandbox_policy.get(),
|
||||
None,
|
||||
None,
|
||||
/*policy_decider*/ None,
|
||||
/*blocked_request_observer*/ None,
|
||||
managed_network_requirements_enabled,
|
||||
NetworkProxyAuditMetadata::default(),
|
||||
)
|
||||
@@ -2092,7 +2095,7 @@ impl CodexMessageProcessor {
|
||||
otel.name = "app_server.thread_start.resolve_status",
|
||||
))
|
||||
.await,
|
||||
false,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
|
||||
let response = ThreadStartResponse {
|
||||
@@ -2541,7 +2544,7 @@ impl CodexMessageProcessor {
|
||||
self.thread_watch_manager
|
||||
.loaded_status_for_thread(&thread.id)
|
||||
.await,
|
||||
false,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
|
||||
self.outgoing
|
||||
@@ -2592,10 +2595,10 @@ impl CodexMessageProcessor {
|
||||
Some(state_db_ctx),
|
||||
rollout_path.as_path(),
|
||||
self.config.model_provider_id.as_str(),
|
||||
None,
|
||||
/*builder*/ None,
|
||||
&[],
|
||||
None,
|
||||
None,
|
||||
/*archived_only*/ None,
|
||||
/*new_thread_memory_mode*/ None,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -2663,10 +2666,10 @@ impl CodexMessageProcessor {
|
||||
Some(state_db_ctx),
|
||||
rollout_path.as_path(),
|
||||
self.config.model_provider_id.as_str(),
|
||||
None,
|
||||
/*builder*/ None,
|
||||
&[],
|
||||
None,
|
||||
None,
|
||||
/*archived_only*/ None,
|
||||
/*new_thread_memory_mode*/ None,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -2853,7 +2856,7 @@ impl CodexMessageProcessor {
|
||||
self.thread_watch_manager
|
||||
.loaded_status_for_thread(&thread.id)
|
||||
.await,
|
||||
false,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
self.attach_thread_name(thread_id, &mut thread).await;
|
||||
let thread_id = thread.id.clone();
|
||||
@@ -3327,8 +3330,13 @@ impl CodexMessageProcessor {
|
||||
|
||||
for connection_id in connection_ids {
|
||||
Self::log_listener_attach_result(
|
||||
self.ensure_conversation_listener(thread_id, connection_id, false, ApiVersion::V2)
|
||||
.await,
|
||||
self.ensure_conversation_listener(
|
||||
thread_id,
|
||||
connection_id,
|
||||
/*raw_events_enabled*/ false,
|
||||
ApiVersion::V2,
|
||||
)
|
||||
.await,
|
||||
thread_id,
|
||||
connection_id,
|
||||
"thread",
|
||||
@@ -3463,7 +3471,7 @@ impl CodexMessageProcessor {
|
||||
self.ensure_conversation_listener(
|
||||
thread_id,
|
||||
request_id.connection_id,
|
||||
false,
|
||||
/*raw_events_enabled*/ false,
|
||||
ApiVersion::V2,
|
||||
)
|
||||
.await,
|
||||
@@ -3495,7 +3503,11 @@ impl CodexMessageProcessor {
|
||||
.loaded_status_for_thread(&thread.id)
|
||||
.await;
|
||||
|
||||
set_thread_status_and_interrupt_stale_turns(&mut thread, thread_status, false);
|
||||
set_thread_status_and_interrupt_stale_turns(
|
||||
&mut thread,
|
||||
thread_status,
|
||||
/*has_live_in_progress_turn*/ false,
|
||||
);
|
||||
|
||||
let response = ThreadResumeResponse {
|
||||
thread,
|
||||
@@ -3813,7 +3825,7 @@ impl CodexMessageProcessor {
|
||||
if let Err(message) = populate_thread_turns(
|
||||
&mut thread,
|
||||
ThreadTurnSource::HistoryItems(&history_items),
|
||||
None,
|
||||
/*active_turn*/ None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
@@ -3930,7 +3942,7 @@ impl CodexMessageProcessor {
|
||||
sandbox,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
None,
|
||||
/*personality*/ None,
|
||||
);
|
||||
typesafe_overrides.ephemeral = ephemeral.then_some(true);
|
||||
// Derive a Config using the same logic as new conversation, honoring overrides if provided.
|
||||
@@ -4001,7 +4013,7 @@ impl CodexMessageProcessor {
|
||||
self.ensure_conversation_listener(
|
||||
thread_id,
|
||||
request_id.connection_id,
|
||||
false,
|
||||
/*raw_events_enabled*/ false,
|
||||
ApiVersion::V2,
|
||||
)
|
||||
.await,
|
||||
@@ -4035,7 +4047,8 @@ impl CodexMessageProcessor {
|
||||
} else {
|
||||
let config_snapshot = forked_thread.config_snapshot().await;
|
||||
// forked thread names do not inherit the source thread name
|
||||
let mut thread = build_thread_from_snapshot(thread_id, &config_snapshot, None);
|
||||
let mut thread =
|
||||
build_thread_from_snapshot(thread_id, &config_snapshot, /*path*/ None);
|
||||
let history_items = match read_rollout_items_from_rollout(rollout_path.as_path()).await
|
||||
{
|
||||
Ok(items) => items,
|
||||
@@ -4055,7 +4068,7 @@ impl CodexMessageProcessor {
|
||||
if let Err(message) = populate_thread_turns(
|
||||
&mut thread,
|
||||
ThreadTurnSource::HistoryItems(&history_items),
|
||||
None,
|
||||
/*active_turn*/ None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
@@ -4069,7 +4082,7 @@ impl CodexMessageProcessor {
|
||||
&& let Err(message) = populate_thread_turns(
|
||||
&mut thread,
|
||||
ThreadTurnSource::RolloutPath(fork_rollout_path.as_path()),
|
||||
None,
|
||||
/*active_turn*/ None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
@@ -4085,7 +4098,7 @@ impl CodexMessageProcessor {
|
||||
self.thread_watch_manager
|
||||
.loaded_status_for_thread(&thread.id)
|
||||
.await,
|
||||
false,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
|
||||
let response = ThreadForkResponse {
|
||||
@@ -4398,7 +4411,7 @@ impl CodexMessageProcessor {
|
||||
params: ExperimentalFeatureListParams,
|
||||
) {
|
||||
let ExperimentalFeatureListParams { cursor, limit } = params;
|
||||
let config = match self.load_latest_config(None).await {
|
||||
let config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
@@ -4513,7 +4526,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
|
||||
async fn mcp_server_refresh(&self, request_id: ConnectionRequestId, _params: Option<()>) {
|
||||
let config = match self.load_latest_config(None).await {
|
||||
let config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
@@ -4572,7 +4585,7 @@ impl CodexMessageProcessor {
|
||||
request_id: ConnectionRequestId,
|
||||
params: McpServerOauthLoginParams,
|
||||
) {
|
||||
let config = match self.load_latest_config(None).await {
|
||||
let config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
@@ -4684,7 +4697,7 @@ impl CodexMessageProcessor {
|
||||
let request = request_id.clone();
|
||||
|
||||
let outgoing = Arc::clone(&self.outgoing);
|
||||
let config = match self.load_latest_config(None).await {
|
||||
let config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request, error).await;
|
||||
@@ -4856,7 +4869,7 @@ impl CodexMessageProcessor {
|
||||
async fn finalize_thread_teardown(&mut self, thread_id: ThreadId) {
|
||||
self.pending_thread_unloads.lock().await.remove(&thread_id);
|
||||
self.outgoing
|
||||
.cancel_requests_for_thread(thread_id, None)
|
||||
.cancel_requests_for_thread(thread_id, /*error*/ None)
|
||||
.await;
|
||||
self.thread_state_manager
|
||||
.remove_thread_state(thread_id)
|
||||
@@ -4919,7 +4932,7 @@ impl CodexMessageProcessor {
|
||||
// Any pending app-server -> client requests for this thread can no longer be
|
||||
// answered; cancel their callbacks before shutdown/unload.
|
||||
self.outgoing
|
||||
.cancel_requests_for_thread(thread_id, None)
|
||||
.cancel_requests_for_thread(thread_id, /*error*/ None)
|
||||
.await;
|
||||
self.thread_state_manager
|
||||
.remove_thread_state(thread_id)
|
||||
@@ -5092,7 +5105,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
|
||||
async fn apps_list(&self, request_id: ConnectionRequestId, params: AppsListParams) {
|
||||
let mut config = match self.load_latest_config(None).await {
|
||||
let mut config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
@@ -5389,7 +5402,7 @@ impl CodexMessageProcessor {
|
||||
} = params;
|
||||
let roots = cwds.unwrap_or_default();
|
||||
|
||||
let mut config = match self.load_latest_config(None).await {
|
||||
let mut config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
@@ -5422,7 +5435,7 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
}
|
||||
|
||||
config = match self.load_latest_config(None).await {
|
||||
config = match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
@@ -5692,9 +5705,9 @@ impl CodexMessageProcessor {
|
||||
Vec::new()
|
||||
} else {
|
||||
let (all_connectors_result, accessible_connectors_result) = tokio::join!(
|
||||
connectors::list_all_connectors_with_options(&config, true),
|
||||
connectors::list_all_connectors_with_options(&config, /*force_refetch*/ true),
|
||||
connectors::list_accessible_connectors_from_mcp_tools_with_options_and_status(
|
||||
&config, true
|
||||
&config, /*force_refetch*/ true
|
||||
),
|
||||
);
|
||||
|
||||
@@ -6046,7 +6059,7 @@ impl CodexMessageProcessor {
|
||||
.ensure_conversation_listener(
|
||||
thread_id,
|
||||
request_id.connection_id,
|
||||
false,
|
||||
/*raw_events_enabled*/ false,
|
||||
ApiVersion::V2,
|
||||
)
|
||||
.await
|
||||
@@ -6327,7 +6340,7 @@ impl CodexMessageProcessor {
|
||||
usize::MAX,
|
||||
config,
|
||||
rollout_path,
|
||||
false,
|
||||
/*persist_extended_history*/ false,
|
||||
self.request_trace_context(request_id).await,
|
||||
)
|
||||
.await
|
||||
@@ -6341,7 +6354,7 @@ impl CodexMessageProcessor {
|
||||
self.ensure_conversation_listener(
|
||||
thread_id,
|
||||
request_id.connection_id,
|
||||
false,
|
||||
/*raw_events_enabled*/ false,
|
||||
ApiVersion::V2,
|
||||
)
|
||||
.await,
|
||||
@@ -6362,7 +6375,7 @@ impl CodexMessageProcessor {
|
||||
self.thread_watch_manager
|
||||
.loaded_status_for_thread(&thread.id)
|
||||
.await,
|
||||
false,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
let notif = ThreadStartedNotification { thread };
|
||||
self.outgoing
|
||||
@@ -7050,7 +7063,7 @@ impl CodexMessageProcessor {
|
||||
tokio::spawn(async move {
|
||||
let derived_config = derive_config_for_cwd(
|
||||
&cli_overrides,
|
||||
None,
|
||||
/*request_overrides*/ None,
|
||||
ConfigOverrides {
|
||||
cwd: Some(command_cwd.clone()),
|
||||
..Default::default()
|
||||
|
||||
@@ -15,15 +15,16 @@ pub(super) async fn load_plugin_app_summaries(
|
||||
return Vec::new();
|
||||
}
|
||||
|
||||
let connectors = match connectors::list_all_connectors_with_options(config, false).await {
|
||||
Ok(connectors) => connectors,
|
||||
Err(err) => {
|
||||
warn!("failed to load app metadata for plugin/read: {err:#}");
|
||||
connectors::list_cached_all_connectors(config)
|
||||
.await
|
||||
.unwrap_or_default()
|
||||
}
|
||||
};
|
||||
let connectors =
|
||||
match connectors::list_all_connectors_with_options(config, /*force_refetch*/ false).await {
|
||||
Ok(connectors) => connectors,
|
||||
Err(err) => {
|
||||
warn!("failed to load app metadata for plugin/read: {err:#}");
|
||||
connectors::list_cached_all_connectors(config)
|
||||
.await
|
||||
.unwrap_or_default()
|
||||
}
|
||||
};
|
||||
|
||||
connectors::connectors_for_plugin_apps(connectors, plugin_apps)
|
||||
.into_iter()
|
||||
|
||||
@@ -201,7 +201,9 @@ impl CommandExecManager {
|
||||
let sessions = Arc::clone(&self.sessions);
|
||||
tokio::spawn(async move {
|
||||
let _started_network_proxy = started_network_proxy;
|
||||
match codex_core::sandboxing::execute_env(exec_request, None).await {
|
||||
match codex_core::sandboxing::execute_env(exec_request, /*stdout_stream*/ None)
|
||||
.await
|
||||
{
|
||||
Ok(output) => {
|
||||
outgoing
|
||||
.send_response(
|
||||
|
||||
@@ -390,8 +390,8 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle {
|
||||
Arc::clone(&outbound_initialized),
|
||||
Arc::clone(&outbound_experimental_api_enabled),
|
||||
Arc::clone(&outbound_opted_out_notification_methods),
|
||||
true,
|
||||
None,
|
||||
/*allow_legacy_notifications*/ true,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
let mut outbound_handle = tokio::spawn(async move {
|
||||
|
||||
@@ -266,10 +266,10 @@ fn app_text_range(range: &CoreTextRange) -> AppTextRange {
|
||||
fn project_config_warning(config: &Config) -> Option<ConfigWarningNotification> {
|
||||
let mut disabled_folders = Vec::new();
|
||||
|
||||
for layer in config
|
||||
.config_layer_stack
|
||||
.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
|
||||
{
|
||||
for layer in config.config_layer_stack.get_layers(
|
||||
ConfigLayerStackOrdering::LowestPrecedenceFirst,
|
||||
/*include_disabled*/ true,
|
||||
) {
|
||||
if !matches!(layer.name, ConfigLayerSource::Project { .. })
|
||||
|| layer.disabled_reason.is_none()
|
||||
{
|
||||
@@ -418,7 +418,7 @@ pub async fn run_main_with_transport(
|
||||
|
||||
let auth_manager = AuthManager::shared(
|
||||
config.codex_home.clone(),
|
||||
false,
|
||||
/*enable_codex_api_key_env*/ false,
|
||||
config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
cloud_requirements_loader(
|
||||
|
||||
@@ -37,7 +37,7 @@ fn main() -> anyhow::Result<()> {
|
||||
arg0_paths,
|
||||
CliConfigOverrides::default(),
|
||||
loader_overrides,
|
||||
false,
|
||||
/*default_analytics_enabled*/ false,
|
||||
transport,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -332,7 +332,7 @@ impl MessageProcessor {
|
||||
request_id.clone(),
|
||||
codex_request,
|
||||
session,
|
||||
None,
|
||||
/*outbound_initialized*/ None,
|
||||
request_context.clone(),
|
||||
)
|
||||
.await;
|
||||
@@ -358,7 +358,8 @@ impl MessageProcessor {
|
||||
};
|
||||
let request_span =
|
||||
crate::app_server_tracing::typed_request_span(&request, connection_id, session);
|
||||
let request_context = RequestContext::new(request_id.clone(), request_span, None);
|
||||
let request_context =
|
||||
RequestContext::new(request_id.clone(), request_span, /*parent_trace*/ None);
|
||||
tracing::trace!(
|
||||
?connection_id,
|
||||
request_id = ?request_id.request_id,
|
||||
|
||||
@@ -234,7 +234,10 @@ impl OutgoingMessageSender {
|
||||
&self,
|
||||
request: ServerRequestPayload,
|
||||
) -> (RequestId, oneshot::Receiver<ClientRequestResult>) {
|
||||
self.send_request_to_connections(None, request, None).await
|
||||
self.send_request_to_connections(
|
||||
/*connection_ids*/ None, request, /*thread_id*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
fn next_request_id(&self) -> RequestId {
|
||||
|
||||
@@ -284,7 +284,7 @@ impl ThreadStateManager {
|
||||
{
|
||||
let mut thread_state_guard = thread_state.lock().await;
|
||||
if experimental_raw_events {
|
||||
thread_state_guard.set_experimental_raw_events(true);
|
||||
thread_state_guard.set_experimental_raw_events(/*enabled*/ true);
|
||||
}
|
||||
}
|
||||
Some(thread_state)
|
||||
|
||||
@@ -91,13 +91,17 @@ impl ThreadWatchManager {
|
||||
}
|
||||
|
||||
pub(crate) async fn upsert_thread(&self, thread: Thread) {
|
||||
self.mutate_and_publish(move |state| state.upsert_thread(thread.id, true))
|
||||
.await;
|
||||
self.mutate_and_publish(move |state| {
|
||||
state.upsert_thread(thread.id, /*emit_notification*/ true)
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(crate) async fn upsert_thread_silently(&self, thread: Thread) {
|
||||
self.mutate_and_publish(move |state| state.upsert_thread(thread.id, false))
|
||||
.await;
|
||||
self.mutate_and_publish(move |state| {
|
||||
state.upsert_thread(thread.id, /*emit_notification*/ false)
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(crate) async fn remove_thread(&self, thread_id: &str) {
|
||||
|
||||
Reference in New Issue
Block a user