mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
ci: verify codex-rs Cargo manifests inherit workspace settings (#16353)
## Why Bazel clippy now catches lints that `cargo clippy` can still miss when a crate under `codex-rs` forgets to opt into workspace lints. The concrete example here was `codex-rs/app-server/tests/common/Cargo.toml`: Bazel flagged a clippy violation in `models_cache.rs`, but Cargo did not because that crate inherited workspace package metadata without declaring `[lints] workspace = true`. We already mirror the workspace clippy deny list into Bazel after [#15955](https://github.com/openai/codex/pull/15955), so we also need a repo-side check that keeps every `codex-rs` manifest opted into the same workspace settings. ## What changed - add `.github/scripts/verify_cargo_workspace_manifests.py`, which parses every `codex-rs/**/Cargo.toml` with `tomllib` and verifies: - `version.workspace = true` - `edition.workspace = true` - `license.workspace = true` - `[lints] workspace = true` - top-level crate names follow the `codex-*` / `codex-utils-*` conventions, with explicit exceptions for `windows-sandbox-rs` and `utils/path-utils` - run that script in `.github/workflows/ci.yml` - update the current outlier manifests so the check is enforceable immediately - fix the newly exposed clippy violations in the affected crates (`app-server/tests/common`, `file-search`, `feedback`, `shell-escalation`, and `debug-client`) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16353). * #16351 * __->__ #16353
This commit is contained in:
@@ -0,0 +1,125 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
"""Verify that codex-rs crates inherit workspace metadata, lints, and names.
|
||||
|
||||
This keeps `cargo clippy` aligned with the workspace lint policy by ensuring
|
||||
each crate opts into `[lints] workspace = true`, and it also checks the crate
|
||||
name conventions for top-level `codex-rs/*` crates and `codex-rs/utils/*`
|
||||
crates.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import tomllib
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[2]
|
||||
CARGO_RS_ROOT = ROOT / "codex-rs"
|
||||
WORKSPACE_PACKAGE_FIELDS = ("version", "edition", "license")
|
||||
TOP_LEVEL_NAME_EXCEPTIONS = {
|
||||
"windows-sandbox-rs": "codex-windows-sandbox",
|
||||
}
|
||||
UTILITY_NAME_EXCEPTIONS = {
|
||||
"path-utils": "codex-utils-path",
|
||||
}
|
||||
|
||||
|
||||
def main() -> int:
|
||||
failures = [
|
||||
(path.relative_to(ROOT), errors)
|
||||
for path in cargo_manifests()
|
||||
if (errors := manifest_errors(path))
|
||||
]
|
||||
if not failures:
|
||||
return 0
|
||||
|
||||
print(
|
||||
"Cargo manifests under codex-rs must inherit workspace package metadata and "
|
||||
"opt into workspace lints."
|
||||
)
|
||||
print(
|
||||
"Cargo only applies `codex-rs/Cargo.toml` `[workspace.lints.clippy]` "
|
||||
"entries to a crate when that crate declares:"
|
||||
)
|
||||
print()
|
||||
print("[lints]")
|
||||
print("workspace = true")
|
||||
print()
|
||||
print(
|
||||
"Without that opt-in, `cargo clippy` can miss violations that Bazel clippy "
|
||||
"catches."
|
||||
)
|
||||
print()
|
||||
print(
|
||||
"Package-name checks apply to `codex-rs/<crate>/Cargo.toml` and "
|
||||
"`codex-rs/utils/<crate>/Cargo.toml`."
|
||||
)
|
||||
print()
|
||||
for path, errors in failures:
|
||||
print(f"{path}:")
|
||||
for error in errors:
|
||||
print(f" - {error}")
|
||||
|
||||
return 1
|
||||
|
||||
|
||||
def manifest_errors(path: Path) -> list[str]:
|
||||
manifest = load_manifest(path)
|
||||
package = manifest.get("package")
|
||||
if not isinstance(package, dict):
|
||||
return []
|
||||
|
||||
errors = []
|
||||
for field in WORKSPACE_PACKAGE_FIELDS:
|
||||
if not is_workspace_reference(package.get(field)):
|
||||
errors.append(f"set `{field}.workspace = true` in `[package]`")
|
||||
|
||||
lints = manifest.get("lints")
|
||||
if not (isinstance(lints, dict) and lints.get("workspace") is True):
|
||||
errors.append("add `[lints]` with `workspace = true`")
|
||||
|
||||
expected_name = expected_package_name(path)
|
||||
if expected_name is not None:
|
||||
actual_name = package.get("name")
|
||||
if actual_name != expected_name:
|
||||
errors.append(
|
||||
f"set `[package].name` to `{expected_name}` (found `{actual_name}`)"
|
||||
)
|
||||
|
||||
return errors
|
||||
|
||||
|
||||
def expected_package_name(path: Path) -> str | None:
|
||||
parts = path.relative_to(CARGO_RS_ROOT).parts
|
||||
if len(parts) == 2 and parts[1] == "Cargo.toml":
|
||||
directory = parts[0]
|
||||
return TOP_LEVEL_NAME_EXCEPTIONS.get(
|
||||
directory,
|
||||
directory if directory.startswith("codex-") else f"codex-{directory}",
|
||||
)
|
||||
if len(parts) == 3 and parts[0] == "utils" and parts[2] == "Cargo.toml":
|
||||
directory = parts[1]
|
||||
return UTILITY_NAME_EXCEPTIONS.get(directory, f"codex-utils-{directory}")
|
||||
return None
|
||||
|
||||
|
||||
def is_workspace_reference(value: object) -> bool:
|
||||
return isinstance(value, dict) and value.get("workspace") is True
|
||||
|
||||
|
||||
def load_manifest(path: Path) -> dict:
|
||||
return tomllib.loads(path.read_text())
|
||||
|
||||
|
||||
def cargo_manifests() -> list[Path]:
|
||||
return sorted(
|
||||
path
|
||||
for path in CARGO_RS_ROOT.rglob("Cargo.toml")
|
||||
if path != CARGO_RS_ROOT / "Cargo.toml"
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -14,6 +14,9 @@ jobs:
|
||||
- name: Checkout repository
|
||||
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
|
||||
|
||||
- name: Verify codex-rs Cargo manifests inherit workspace settings
|
||||
run: python3 .github/scripts/verify_cargo_workspace_manifests.py
|
||||
|
||||
- name: Setup pnpm
|
||||
uses: pnpm/action-setup@a8198c4bff370c8506180b035930dea56dbd5288 # v5
|
||||
with:
|
||||
|
||||
@@ -8,6 +8,9 @@ license.workspace = true
|
||||
name = "codex_ansi_escape"
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
ansi-to-tui = { workspace = true }
|
||||
ratatui = { workspace = true, features = [
|
||||
|
||||
@@ -7,6 +7,9 @@ license.workspace = true
|
||||
[lib]
|
||||
path = "lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
base64 = { workspace = true }
|
||||
|
||||
@@ -27,7 +27,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo {
|
||||
},
|
||||
supported_in_api: preset.supported_in_api,
|
||||
priority,
|
||||
upgrade: preset.upgrade.as_ref().map(|u| u.into()),
|
||||
upgrade: preset.upgrade.as_ref().map(Into::into),
|
||||
base_instructions: "base instructions".to_string(),
|
||||
model_messages: None,
|
||||
supports_reasoning_summaries: false,
|
||||
|
||||
@@ -8,6 +8,9 @@ publish = false
|
||||
[lib]
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = "1"
|
||||
serde = { version = "1", features = ["derive"] }
|
||||
|
||||
@@ -8,6 +8,9 @@ license.workspace = true
|
||||
name = "codex_backend_openapi_models"
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
# Important: generated code often violates our workspace lints.
|
||||
# Allow unwrap/expect in this crate so the workspace builds cleanly
|
||||
# after models are regenerated.
|
||||
|
||||
@@ -7,6 +7,9 @@ license.workspace = true
|
||||
[lib]
|
||||
path = "lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
assert_cmd = { workspace = true }
|
||||
|
||||
@@ -4,6 +4,9 @@ version.workspace = true
|
||||
edition.workspace = true
|
||||
license.workspace = true
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow.workspace = true
|
||||
clap = { workspace = true, features = ["derive"] }
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
#![allow(clippy::expect_used)]
|
||||
use std::io::BufRead;
|
||||
use std::io::BufReader;
|
||||
use std::io::Write;
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
#![allow(clippy::expect_used)]
|
||||
use std::io;
|
||||
use std::io::IsTerminal;
|
||||
use std::io::Write;
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
#![allow(clippy::expect_used)]
|
||||
use std::io::BufRead;
|
||||
use std::io::BufReader;
|
||||
use std::process::ChildStdout;
|
||||
@@ -113,7 +114,7 @@ fn handle_server_request(
|
||||
stdin: &Arc<Mutex<Option<std::process::ChildStdin>>>,
|
||||
output: &Output,
|
||||
) -> anyhow::Result<()> {
|
||||
let server_request = match ServerRequest::try_from(request.clone()) {
|
||||
let server_request = match ServerRequest::try_from(request) {
|
||||
Ok(server_request) => server_request,
|
||||
Err(_) => return Ok(()),
|
||||
};
|
||||
|
||||
@@ -4,6 +4,9 @@ version.workspace = true
|
||||
edition.workspace = true
|
||||
license.workspace = true
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
|
||||
@@ -95,10 +95,12 @@ impl CodexFeedback {
|
||||
|
||||
pub fn snapshot(&self, session_id: Option<ThreadId>) -> FeedbackSnapshot {
|
||||
let bytes = {
|
||||
#[allow(clippy::expect_used)]
|
||||
let guard = self.inner.ring.lock().expect("mutex poisoned");
|
||||
guard.snapshot_bytes()
|
||||
};
|
||||
let tags = {
|
||||
#[allow(clippy::expect_used)]
|
||||
let guard = self.inner.tags.lock().expect("mutex poisoned");
|
||||
guard.clone()
|
||||
};
|
||||
@@ -324,7 +326,7 @@ impl FeedbackSnapshot {
|
||||
use sentry::protocol::Values;
|
||||
|
||||
event.exception = Values::from(vec![Exception {
|
||||
ty: title.clone(),
|
||||
ty: title,
|
||||
value: Some(r.to_string()),
|
||||
..Default::default()
|
||||
}]);
|
||||
@@ -430,6 +432,7 @@ where
|
||||
return;
|
||||
}
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
let mut guard = self.inner.tags.lock().expect("mutex poisoned");
|
||||
for (key, value) in visitor.tags {
|
||||
if guard.len() >= MAX_FEEDBACK_TAGS && !guard.contains_key(&key) {
|
||||
|
||||
@@ -12,6 +12,9 @@ path = "src/main.rs"
|
||||
name = "codex_file_search"
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
clap = { workspace = true, features = ["derive"] }
|
||||
|
||||
@@ -195,10 +195,10 @@ pub fn create_session(
|
||||
threads: threads.get(),
|
||||
compute_indices,
|
||||
respect_gitignore,
|
||||
cancelled: cancelled.clone(),
|
||||
cancelled,
|
||||
shutdown: Arc::new(AtomicBool::new(false)),
|
||||
reporter,
|
||||
work_tx: work_tx.clone(),
|
||||
work_tx,
|
||||
});
|
||||
|
||||
let matcher_inner = inner.clone();
|
||||
@@ -611,13 +611,14 @@ struct RunReporter {
|
||||
|
||||
impl SessionReporter for RunReporter {
|
||||
fn on_update(&self, snapshot: &FileSearchSnapshot) {
|
||||
#[expect(clippy::unwrap_used)]
|
||||
#[allow(clippy::unwrap_used)]
|
||||
let mut guard = self.snapshot.write().unwrap();
|
||||
*guard = snapshot.clone();
|
||||
}
|
||||
|
||||
fn on_complete(&self) {
|
||||
let (cv, mutex) = &self.completed;
|
||||
#[allow(clippy::unwrap_used)]
|
||||
let mut completed = mutex.lock().unwrap();
|
||||
*completed = true;
|
||||
cv.notify_all();
|
||||
@@ -627,10 +628,15 @@ impl SessionReporter for RunReporter {
|
||||
impl RunReporter {
|
||||
fn wait_for_complete(&self) -> FileSearchSnapshot {
|
||||
let (cv, mutex) = &self.completed;
|
||||
#[allow(clippy::unwrap_used)]
|
||||
let mut completed = mutex.lock().unwrap();
|
||||
while !*completed {
|
||||
completed = cv.wait(completed).unwrap();
|
||||
#[allow(clippy::unwrap_used)]
|
||||
{
|
||||
completed = cv.wait(completed).unwrap();
|
||||
}
|
||||
}
|
||||
#[allow(clippy::unwrap_used)]
|
||||
self.snapshot.read().unwrap().clone()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -27,8 +27,11 @@ struct StdioReporter {
|
||||
impl Reporter for StdioReporter {
|
||||
fn report_match(&self, file_match: &FileMatch) {
|
||||
if self.write_output_as_json {
|
||||
println!("{}", serde_json::to_string(&file_match).unwrap());
|
||||
#[allow(clippy::unwrap_used)]
|
||||
let json = serde_json::to_string(file_match).unwrap();
|
||||
println!("{json}");
|
||||
} else if self.show_indices {
|
||||
#[allow(clippy::expect_used)]
|
||||
let indices = file_match
|
||||
.indices
|
||||
.as_ref()
|
||||
@@ -61,7 +64,9 @@ impl Reporter for StdioReporter {
|
||||
fn warn_matches_truncated(&self, total_match_count: usize, shown_match_count: usize) {
|
||||
if self.write_output_as_json {
|
||||
let value = json!({"matches_truncated": true});
|
||||
println!("{}", serde_json::to_string(&value).unwrap());
|
||||
#[allow(clippy::unwrap_used)]
|
||||
let json = serde_json::to_string(&value).unwrap();
|
||||
println!("{json}");
|
||||
} else {
|
||||
eprintln!(
|
||||
"Warning: showing {shown_match_count} out of {total_match_count} results. Provide a more specific pattern or increase the --limit.",
|
||||
|
||||
@@ -7,6 +7,9 @@ license.workspace = true
|
||||
[lib]
|
||||
path = "lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
codex-core = { workspace = true }
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
[package]
|
||||
name = "codex-network-proxy"
|
||||
edition = "2024"
|
||||
version = { workspace = true }
|
||||
edition.workspace = true
|
||||
version.workspace = true
|
||||
license.workspace = true
|
||||
|
||||
[lib]
|
||||
|
||||
@@ -8,6 +8,9 @@ version.workspace = true
|
||||
name = "codex-execve-wrapper"
|
||||
path = "src/bin/main_execve_wrapper.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
async-trait = { workspace = true }
|
||||
|
||||
@@ -601,7 +601,7 @@ mod tests {
|
||||
let execve_wrapper_str = execve_wrapper.to_string_lossy().to_string();
|
||||
let server = EscalateServer::new(
|
||||
PathBuf::from("/bin/zsh"),
|
||||
execve_wrapper.clone(),
|
||||
execve_wrapper,
|
||||
DeterministicEscalationPolicy {
|
||||
decision: EscalationDecision::run(),
|
||||
},
|
||||
|
||||
@@ -62,7 +62,7 @@ fn extract_fds(control: &[u8]) -> Vec<OwnedFd> {
|
||||
let fd_count: usize = {
|
||||
// `cmsghdr::cmsg_len` is not typed consistently across targets, so normalize it
|
||||
// before doing the size arithmetic.
|
||||
#[allow(clippy::useless_conversion)]
|
||||
#[allow(clippy::useless_conversion, clippy::expect_used)]
|
||||
let cmsg_data_len = usize::try_from(unsafe { (*cmsg).cmsg_len })
|
||||
.expect("cmsghdr length fits")
|
||||
- unsafe { libc::CMSG_LEN(0) as usize };
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
[package]
|
||||
edition = "2024"
|
||||
edition.workspace = true
|
||||
license.workspace = true
|
||||
name = "codex-utils-pty"
|
||||
version.workspace = true
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
[package]
|
||||
build = "build.rs"
|
||||
edition = "2024"
|
||||
edition.workspace = true
|
||||
license.workspace = true
|
||||
name = "codex-windows-sandbox"
|
||||
version.workspace = true
|
||||
@@ -17,6 +17,9 @@ path = "src/bin/setup_main.rs"
|
||||
name = "codex-command-runner"
|
||||
path = "src/bin/command_runner.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = "1.0"
|
||||
base64 = { workspace = true }
|
||||
|
||||
Reference in New Issue
Block a user