mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
chore: clean up argument-comment lint and roll out all-target CI on macOS (#16054)
## Why `argument-comment-lint` was green in CI even though the repo still had many uncommented literal arguments. The main gap was target coverage: the repo wrapper did not force Cargo to inspect test-only call sites, so examples like the `latest_session_lookup_params(true, ...)` tests in `codex-rs/tui_app_server/src/lib.rs` never entered the blocking CI path. This change cleans up the existing backlog, makes the default repo lint path cover all Cargo targets, and starts rolling that stricter CI enforcement out on the platform where it is currently validated. ## What changed - mechanically fixed existing `argument-comment-lint` violations across the `codex-rs` workspace, including tests, examples, and benches - updated `tools/argument-comment-lint/run-prebuilt-linter.sh` and `tools/argument-comment-lint/run.sh` so non-`--fix` runs default to `--all-targets` unless the caller explicitly narrows the target set - fixed both wrappers so forwarded cargo arguments after `--` are preserved with a single separator - documented the new default behavior in `tools/argument-comment-lint/README.md` - updated `rust-ci` so the macOS lint lane keeps the plain wrapper invocation and therefore enforces `--all-targets`, while Linux and Windows temporarily pass `-- --lib --bins` That temporary CI split keeps the stricter all-targets check where it is already cleaned up, while leaving room to finish the remaining Linux- and Windows-specific target-gated cleanup before enabling `--all-targets` on those runners. The Linux and Windows failures on the intermediate revision were caused by the wrapper forwarding bug, not by additional lint findings in those lanes. ## Validation - `bash -n tools/argument-comment-lint/run.sh` - `bash -n tools/argument-comment-lint/run-prebuilt-linter.sh` - shell-level wrapper forwarding check for `-- --lib --bins` - shell-level wrapper forwarding check for `-- --tests` - `just argument-comment-lint` - `cargo test` in `tools/argument-comment-lint` - `cargo test -p codex-terminal-detection` ## Follow-up - Clean up remaining Linux-only target-gated callsites, then switch the Linux lint lane back to the plain wrapper invocation. - Clean up remaining Windows-only target-gated callsites, then switch the Windows lint lane back to the plain wrapper invocation.
This commit is contained in:
@@ -8747,7 +8747,7 @@ mod tests {
|
||||
fn config_load_error_marks_non_auth_cloud_requirements_failures_without_relogin() {
|
||||
let err = std::io::Error::other(CloudRequirementsLoadError::new(
|
||||
CloudRequirementsLoadErrorCode::RequestFailed,
|
||||
None,
|
||||
/*status_code*/ None,
|
||||
"failed to load your workspace-managed config",
|
||||
));
|
||||
|
||||
@@ -8961,7 +8961,8 @@ mod tests {
|
||||
fn merge_persisted_resume_metadata_skips_missing_values() -> Result<()> {
|
||||
let mut request_overrides = None;
|
||||
let mut typesafe_overrides = ConfigOverrides::default();
|
||||
let persisted_metadata = test_thread_metadata(None, None)?;
|
||||
let persisted_metadata =
|
||||
test_thread_metadata(/*model*/ None, /*reasoning_effort*/ None)?;
|
||||
|
||||
merge_persisted_resume_metadata(
|
||||
&mut request_overrides,
|
||||
@@ -9013,7 +9014,7 @@ mod tests {
|
||||
path.clone(),
|
||||
&head,
|
||||
&session_meta,
|
||||
None,
|
||||
/*git*/ None,
|
||||
"test-provider",
|
||||
timestamp.clone(),
|
||||
)
|
||||
@@ -9223,9 +9224,9 @@ mod tests {
|
||||
source,
|
||||
Some("atlas".to_string()),
|
||||
Some("explorer".to_string()),
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
/*git_sha*/ None,
|
||||
/*git_branch*/ None,
|
||||
/*git_origin_url*/ None,
|
||||
);
|
||||
|
||||
let thread = summary_to_thread(summary);
|
||||
@@ -9244,7 +9245,9 @@ mod tests {
|
||||
|
||||
manager.connection_initialized(connection).await;
|
||||
manager
|
||||
.try_ensure_connection_subscribed(thread_id, connection, false)
|
||||
.try_ensure_connection_subscribed(
|
||||
thread_id, connection, /*experimental_raw_events*/ false,
|
||||
)
|
||||
.await
|
||||
.expect("connection should be live");
|
||||
{
|
||||
@@ -9288,11 +9291,19 @@ mod tests {
|
||||
manager.connection_initialized(connection_a).await;
|
||||
manager.connection_initialized(connection_b).await;
|
||||
manager
|
||||
.try_ensure_connection_subscribed(thread_id, connection_a, false)
|
||||
.try_ensure_connection_subscribed(
|
||||
thread_id,
|
||||
connection_a,
|
||||
/*experimental_raw_events*/ false,
|
||||
)
|
||||
.await
|
||||
.expect("connection_a should be live");
|
||||
manager
|
||||
.try_ensure_connection_subscribed(thread_id, connection_b, false)
|
||||
.try_ensure_connection_subscribed(
|
||||
thread_id,
|
||||
connection_b,
|
||||
/*experimental_raw_events*/ false,
|
||||
)
|
||||
.await
|
||||
.expect("connection_b should be live");
|
||||
{
|
||||
@@ -9325,7 +9336,9 @@ mod tests {
|
||||
|
||||
assert!(
|
||||
manager
|
||||
.try_ensure_connection_subscribed(thread_id, connection, false)
|
||||
.try_ensure_connection_subscribed(
|
||||
thread_id, connection, /*experimental_raw_events*/ false
|
||||
)
|
||||
.await
|
||||
.is_none()
|
||||
);
|
||||
|
||||
@@ -137,7 +137,7 @@ mod tests {
|
||||
&all_connectors,
|
||||
&[],
|
||||
&[AppConnectorId("alpha".to_string())],
|
||||
false,
|
||||
/*codex_apps_ready*/ false,
|
||||
),
|
||||
Vec::new()
|
||||
);
|
||||
|
||||
@@ -90,7 +90,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn compute_source_filters_defaults_to_interactive_sources() {
|
||||
let (allowed_sources, filter) = compute_source_filters(None);
|
||||
let (allowed_sources, filter) = compute_source_filters(/*source_kinds*/ None);
|
||||
|
||||
assert_eq!(allowed_sources, INTERACTIVE_SESSION_SOURCES.to_vec());
|
||||
assert_eq!(filter, None);
|
||||
|
||||
@@ -786,7 +786,8 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn in_process_start_clamps_zero_channel_capacity() {
|
||||
let client = start_test_client_with_capacity(SessionSource::Cli, 0).await;
|
||||
let client =
|
||||
start_test_client_with_capacity(SessionSource::Cli, /*channel_capacity*/ 0).await;
|
||||
let response = loop {
|
||||
match client
|
||||
.request(ClientRequest::ConfigRequirementsRead {
|
||||
|
||||
@@ -860,7 +860,10 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn log_format_from_env_value_defaults_for_non_json_values() {
|
||||
assert_eq!(LogFormat::from_env_value(None), LogFormat::Default);
|
||||
assert_eq!(
|
||||
LogFormat::from_env_value(/*value*/ None),
|
||||
LogFormat::Default
|
||||
);
|
||||
assert_eq!(LogFormat::from_env_value(Some("")), LogFormat::Default);
|
||||
assert_eq!(LogFormat::from_env_value(Some("text")), LogFormat::Default);
|
||||
assert_eq!(LogFormat::from_env_value(Some("jsonl")), LogFormat::Default);
|
||||
|
||||
@@ -147,7 +147,7 @@ impl TracingHarness {
|
||||
}),
|
||||
},
|
||||
},
|
||||
None,
|
||||
/*trace*/ None,
|
||||
)
|
||||
.await;
|
||||
assert!(harness.session.initialized);
|
||||
@@ -213,7 +213,7 @@ async fn build_test_config(codex_home: &Path, server_uri: &str) -> Result<Config
|
||||
codex_home,
|
||||
server_uri,
|
||||
&BTreeMap::new(),
|
||||
8_192,
|
||||
/*auto_compact_limit*/ 8_192,
|
||||
Some(false),
|
||||
"mock_provider",
|
||||
"compact",
|
||||
@@ -509,7 +509,9 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() ->
|
||||
..
|
||||
} = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022");
|
||||
|
||||
let _: ThreadStartResponse = harness.start_thread(20_002, None).await;
|
||||
let _: ThreadStartResponse = harness
|
||||
.start_thread(/*request_id*/ 20_002, /*trace*/ None)
|
||||
.await;
|
||||
let untraced_spans = wait_for_exported_spans(harness.tracing, |spans| {
|
||||
spans.iter().any(|span| {
|
||||
span.span_kind == SpanKind::Server
|
||||
@@ -538,10 +540,16 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() ->
|
||||
.span_context
|
||||
.trace_id(),
|
||||
);
|
||||
assert_has_internal_descendant_at_min_depth(&untraced_spans, untraced_server_span, 1);
|
||||
assert_has_internal_descendant_at_min_depth(
|
||||
&untraced_spans,
|
||||
untraced_server_span,
|
||||
/*min_depth*/ 1,
|
||||
);
|
||||
|
||||
let baseline_len = untraced_spans.len();
|
||||
let _: ThreadStartResponse = harness.start_thread(20_003, Some(remote_trace)).await;
|
||||
let _: ThreadStartResponse = harness
|
||||
.start_thread(/*request_id*/ 20_003, Some(remote_trace))
|
||||
.await;
|
||||
let spans = wait_for_new_exported_spans(harness.tracing, baseline_len, |spans| {
|
||||
spans.iter().any(|span| {
|
||||
span.span_kind == SpanKind::Server
|
||||
@@ -561,8 +569,8 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() ->
|
||||
assert!(server_request_span.parent_span_is_remote);
|
||||
assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id);
|
||||
assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID);
|
||||
assert_has_internal_descendant_at_min_depth(&spans, server_request_span, 1);
|
||||
assert_has_internal_descendant_at_min_depth(&spans, server_request_span, 2);
|
||||
assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 1);
|
||||
assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 2);
|
||||
harness.shutdown().await;
|
||||
|
||||
Ok(())
|
||||
@@ -572,7 +580,7 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() ->
|
||||
async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> {
|
||||
let _guard = tracing_test_guard().lock().await;
|
||||
let mut harness = TracingHarness::new().await?;
|
||||
let thread_start_response = harness.start_thread(2, None).await;
|
||||
let thread_start_response = harness.start_thread(/*request_id*/ 2, /*trace*/ None).await;
|
||||
let thread_id = thread_start_response.thread.id.clone();
|
||||
|
||||
harness.reset_tracing();
|
||||
|
||||
@@ -886,7 +886,7 @@ mod tests {
|
||||
.register_request_context(RequestContext::new(
|
||||
request_id.clone(),
|
||||
tracing::info_span!("app_server.request", rpc.method = "thread/start"),
|
||||
None,
|
||||
/*parent_trace*/ None,
|
||||
))
|
||||
.await;
|
||||
assert_eq!(outgoing.request_context_count().await, 1);
|
||||
@@ -997,14 +997,14 @@ mod tests {
|
||||
.register_request_context(RequestContext::new(
|
||||
closed_connection_request,
|
||||
tracing::info_span!("app_server.request", rpc.method = "turn/interrupt"),
|
||||
None,
|
||||
/*parent_trace*/ None,
|
||||
))
|
||||
.await;
|
||||
outgoing
|
||||
.register_request_context(RequestContext::new(
|
||||
open_connection_request,
|
||||
tracing::info_span!("app_server.request", rpc.method = "turn/start"),
|
||||
None,
|
||||
/*parent_trace*/ None,
|
||||
))
|
||||
.await;
|
||||
assert_eq!(outgoing.request_context_count().await, 2);
|
||||
|
||||
@@ -530,7 +530,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn resolves_in_progress_turn_to_active_status() {
|
||||
let status = resolve_thread_status(ThreadStatus::Idle, true);
|
||||
let status = resolve_thread_status(ThreadStatus::Idle, /*has_in_progress_turn*/ true);
|
||||
assert_eq!(
|
||||
status,
|
||||
ThreadStatus::Active {
|
||||
@@ -538,7 +538,8 @@ mod tests {
|
||||
}
|
||||
);
|
||||
|
||||
let status = resolve_thread_status(ThreadStatus::NotLoaded, true);
|
||||
let status =
|
||||
resolve_thread_status(ThreadStatus::NotLoaded, /*has_in_progress_turn*/ true);
|
||||
assert_eq!(
|
||||
status,
|
||||
ThreadStatus::Active {
|
||||
@@ -550,11 +551,14 @@ mod tests {
|
||||
#[test]
|
||||
fn keeps_status_when_no_in_progress_turn() {
|
||||
assert_eq!(
|
||||
resolve_thread_status(ThreadStatus::Idle, false),
|
||||
resolve_thread_status(ThreadStatus::Idle, /*has_in_progress_turn*/ false),
|
||||
ThreadStatus::Idle
|
||||
);
|
||||
assert_eq!(
|
||||
resolve_thread_status(ThreadStatus::SystemError, false),
|
||||
resolve_thread_status(
|
||||
ThreadStatus::SystemError,
|
||||
/*has_in_progress_turn*/ false
|
||||
),
|
||||
ThreadStatus::SystemError
|
||||
);
|
||||
}
|
||||
|
||||
@@ -506,8 +506,14 @@ mod tests {
|
||||
}),
|
||||
);
|
||||
let tampered = token.replace(".eyJleHAi", ".eyJleHBi");
|
||||
let err = verify_signed_bearer_token(&tampered, shared_secret, None, None, 30)
|
||||
.expect_err("tampered jwt should fail");
|
||||
let err = verify_signed_bearer_token(
|
||||
&tampered,
|
||||
shared_secret,
|
||||
/*issuer*/ None,
|
||||
/*audience*/ None,
|
||||
/*max_clock_skew_seconds*/ 30,
|
||||
)
|
||||
.expect_err("tampered jwt should fail");
|
||||
assert_eq!(err.status_code(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
@@ -522,8 +528,14 @@ mod tests {
|
||||
"aud": "audience",
|
||||
}),
|
||||
);
|
||||
verify_signed_bearer_token(&token, shared_secret, Some("issuer"), Some("audience"), 30)
|
||||
.expect("valid signed token should verify");
|
||||
verify_signed_bearer_token(
|
||||
&token,
|
||||
shared_secret,
|
||||
Some("issuer"),
|
||||
Some("audience"),
|
||||
/*max_clock_skew_seconds*/ 30,
|
||||
)
|
||||
.expect("valid signed token should verify");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -536,8 +548,14 @@ mod tests {
|
||||
"aud": ["other-audience", "audience"],
|
||||
}),
|
||||
);
|
||||
verify_signed_bearer_token(&token, shared_secret, None, Some("audience"), 30)
|
||||
.expect("jwt audience arrays should verify");
|
||||
verify_signed_bearer_token(
|
||||
&token,
|
||||
shared_secret,
|
||||
/*issuer*/ None,
|
||||
Some("audience"),
|
||||
/*max_clock_skew_seconds*/ 30,
|
||||
)
|
||||
.expect("jwt audience arrays should verify");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -550,9 +568,14 @@ mod tests {
|
||||
);
|
||||
let header_segment = URL_SAFE_NO_PAD.encode(br#"{"alg":"none","typ":"JWT"}"#);
|
||||
let token = format!("{header_segment}.{claims_segment}.");
|
||||
let err =
|
||||
verify_signed_bearer_token(&token, b"0123456789abcdef0123456789abcdef", None, None, 30)
|
||||
.expect_err("alg=none jwt should be rejected");
|
||||
let err = verify_signed_bearer_token(
|
||||
&token,
|
||||
b"0123456789abcdef0123456789abcdef",
|
||||
/*issuer*/ None,
|
||||
/*audience*/ None,
|
||||
/*max_clock_skew_seconds*/ 30,
|
||||
)
|
||||
.expect_err("alg=none jwt should be rejected");
|
||||
assert_eq!(err.status_code(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
@@ -565,8 +588,14 @@ mod tests {
|
||||
"iss": "issuer",
|
||||
}),
|
||||
);
|
||||
let err = verify_signed_bearer_token(&token, shared_secret, None, None, 30)
|
||||
.expect_err("jwt without exp should be rejected");
|
||||
let err = verify_signed_bearer_token(
|
||||
&token,
|
||||
shared_secret,
|
||||
/*issuer*/ None,
|
||||
/*audience*/ None,
|
||||
/*max_clock_skew_seconds*/ 30,
|
||||
)
|
||||
.expect_err("jwt without exp should be rejected");
|
||||
assert_eq!(err.status_code(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
|
||||
@@ -646,7 +646,7 @@ mod tests {
|
||||
initialized,
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
opted_out_notification_methods,
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
@@ -686,7 +686,7 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::from(["configWarning".to_string()]))),
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
@@ -726,7 +726,7 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
@@ -772,7 +772,7 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(false)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
@@ -834,7 +834,7 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
@@ -1006,7 +1006,7 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
None,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user