mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Add comprehensive root formatting check (#25683)
## Why The root formatting entrypoints could drift: `just fmt` did not format the Justfile itself, and the CI-facing check recipe only checked Python scripts instead of matching everything formatted by `just fmt`. ## What changed - Add a shared cross-platform Python formatter driver used by both `just fmt` and `just fmt-check`. - Run Justfile, Rust, Python SDK, and internal-script formatter groups concurrently while buffering each formatter group's output until it finishes. - Log formatter starts immediately, then print each formatter group's labeled output when it completes. - Keep the SDK lint-fix and Ruff formatting passes ordered, with source comments explaining their distinct roles and the check-mode equivalents. - Run Ruff through shared `uv run --no-sync --with ruff` overlays so formatting works on clean glibc Linux checkouts without installing the platform-specific SDK runtime wheel. - Show `fmt-check` help text in `just -l` and simplify CI to call the shared driver through `just fmt-check`. - Pin the general CI workflow to `just@1.51.0` so its formatter agrees with the checked-in Justfile. - Add regression coverage for the thin Just recipes and the driver's formatter graph. ## Validation - `just fmt` - `just fmt-check` - `python3 -m pytest sdk/python/tests/test_artifact_workflow_and_binaries.py -k 'root_fmt or root_format' -q` - `pnpm run format` - `git diff --check` - `just -l | rg -n '^ fmt|fmt-check'` - `uvx --from uv==0.7.22 uv run --frozen --project sdk/python --no-sync --with ruff ruff check --diff sdk/python`
This commit is contained in:
Adam Perry @ OpenAI
committed by
GitHub
Unverified
GitHub
parent
0002316687
commit
747f1003dd
@@ -76,13 +76,13 @@ jobs:
|
||||
|
||||
- uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2.62.49
|
||||
with:
|
||||
tool: just
|
||||
tool: just@1.51.0
|
||||
- name: Install uv
|
||||
uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0
|
||||
with:
|
||||
version: "0.11.3"
|
||||
- name: Ruff format Python scripts (run `just fmt` to fix)
|
||||
run: just fmt-scripts-check
|
||||
- name: Check formatting (run `just fmt` to fix)
|
||||
run: just fmt-check
|
||||
|
||||
- name: Prettier (run `pnpm run format:fix` to fix)
|
||||
run: pnpm run format
|
||||
|
||||
@@ -21,9 +21,9 @@ exec *args:
|
||||
cargo run --bin codex -- exec {args}
|
||||
|
||||
# Start `codex exec-server` and run codex-tui.
|
||||
[unix]
|
||||
[no-cd]
|
||||
[positional-arguments]
|
||||
[unix]
|
||||
tui-with-exec-server *args:
|
||||
{{ justfile_directory() }}/scripts/run_tui_with_exec_server.sh "$@"
|
||||
|
||||
@@ -36,16 +36,13 @@ app-server-test-client *args:
|
||||
cargo build -p codex-cli
|
||||
cargo run -p codex-app-server-test-client -- --codex-bin ./target/debug/codex {args}
|
||||
|
||||
# Format Rust, Python SDK code, and Python scripts.
|
||||
# Format the justfile, Rust, Python SDK code, and Python scripts.
|
||||
fmt:
|
||||
cargo fmt -- --config imports_granularity=Item {stderr-null}
|
||||
uv run --frozen --project ../sdk/python --extra dev ruff check --fix --fix-only ../sdk/python
|
||||
uv run --frozen --project ../sdk/python --extra dev ruff format ../sdk/python
|
||||
# Root scripts have their own locked Ruff environment.
|
||||
uv run --frozen --project ../scripts ruff format ../scripts
|
||||
{{ python }} ../scripts/format.py
|
||||
|
||||
fmt-scripts-check:
|
||||
uv run --frozen --project ../scripts ruff format --check ../scripts
|
||||
# Check formatting without modifying files.
|
||||
fmt-check:
|
||||
{{ python }} ../scripts/format.py --check
|
||||
|
||||
fix *args:
|
||||
cargo clippy --fix --tests --allow-dirty {args}
|
||||
@@ -97,21 +94,21 @@ bench-smoke:
|
||||
# Build and run Codex from source using Bazel.
|
||||
# On Unix, use `[no-cd]` and `--run_under="cd $PWD &&"` to ensure Bazel runs
|
||||
# the command in the current working directory.
|
||||
[unix]
|
||||
[no-cd]
|
||||
[unix]
|
||||
bazel-codex *args:
|
||||
bazel run //codex-rs/cli:codex --run_under="cd $PWD &&" -- "$@"
|
||||
|
||||
[windows]
|
||||
bazel-codex *args:
|
||||
bazel run //codex-rs/cli:codex --run_under='cd /d "{{invocation_directory_native()}}" &&' -- @($args | Select-Object -Skip 1)
|
||||
bazel run //codex-rs/cli:codex --run_under='cd /d "{{ invocation_directory_native() }}" &&' -- @($args | Select-Object -Skip 1)
|
||||
|
||||
[no-cd]
|
||||
bazel-lock-update:
|
||||
bazel mod deps --lockfile_mode=update
|
||||
|
||||
[unix]
|
||||
[no-cd]
|
||||
[unix]
|
||||
bazel-lock-check:
|
||||
{{ justfile_directory() }}/scripts/check-module-bazel-lock.sh
|
||||
|
||||
@@ -122,13 +119,13 @@ bazel-lock-check:
|
||||
bazel-test:
|
||||
bazel test --test_tag_filters=-argument-comment-lint //... --keep_going
|
||||
|
||||
[unix]
|
||||
[no-cd]
|
||||
[unix]
|
||||
bazel-clippy:
|
||||
bazel_targets="$({{ justfile_directory() }}/scripts/list-bazel-clippy-targets.sh)" && bazel build --config=clippy -- ${bazel_targets}
|
||||
|
||||
[unix]
|
||||
[no-cd]
|
||||
[unix]
|
||||
bazel-argument-comment-lint:
|
||||
bazel build --config=argument-comment-lint -- $({{ justfile_directory() }}/tools/argument-comment-lint/list-bazel-targets.sh)
|
||||
|
||||
@@ -155,8 +152,8 @@ write-hooks-schema:
|
||||
cargo run --manifest-path {{ justfile_directory() }}/codex-rs/Cargo.toml -p codex-hooks --bin write_hooks_schema_fixtures
|
||||
|
||||
# Run the argument-comment Dylint checks across codex-rs.
|
||||
[unix]
|
||||
[no-cd]
|
||||
[unix]
|
||||
argument-comment-lint *args:
|
||||
if [ "$#" -eq 0 ]; then \
|
||||
bazel build --config=argument-comment-lint -- $({{ justfile_directory() }}/tools/argument-comment-lint/list-bazel-targets.sh); \
|
||||
|
||||
@@ -0,0 +1,177 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Format repository sources or check that they are already formatted."""
|
||||
|
||||
import argparse
|
||||
import shlex
|
||||
import subprocess
|
||||
import sys
|
||||
from concurrent.futures import ThreadPoolExecutor, as_completed
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[1]
|
||||
CODEX_RS_ROOT = REPO_ROOT / "codex-rs"
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class Command:
|
||||
args: tuple[str, ...]
|
||||
cwd: Path = REPO_ROOT
|
||||
discard_stderr: bool = False
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class FormatterGroup:
|
||||
name: str
|
||||
commands: tuple[Command, ...]
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class FormatterResult:
|
||||
name: str
|
||||
output: str
|
||||
returncode: int
|
||||
|
||||
|
||||
def formatter_groups(*, check: bool) -> tuple[FormatterGroup, ...]:
|
||||
just_args = ["just", "--unstable", "--fmt"]
|
||||
cargo_args = ["cargo", "fmt", "--", "--config", "imports_granularity=Item"]
|
||||
# Use an unpinned overlay so Ruff is available without syncing project
|
||||
# dependencies. Each `--project` still retains its local configuration context.
|
||||
sdk_uv_run_args = [
|
||||
"uv",
|
||||
"run",
|
||||
"--frozen",
|
||||
"--project",
|
||||
"sdk/python",
|
||||
"--no-sync",
|
||||
"--with",
|
||||
"ruff",
|
||||
]
|
||||
scripts_uv_run_args = [
|
||||
"uv",
|
||||
"run",
|
||||
"--frozen",
|
||||
"--project",
|
||||
"scripts",
|
||||
"--no-sync",
|
||||
"--with",
|
||||
"ruff",
|
||||
]
|
||||
sdk_format_args = [
|
||||
*sdk_uv_run_args,
|
||||
"ruff",
|
||||
"format",
|
||||
]
|
||||
scripts_format_args = [
|
||||
*scripts_uv_run_args,
|
||||
"ruff",
|
||||
"format",
|
||||
]
|
||||
|
||||
if check:
|
||||
just_args.append("--check")
|
||||
cargo_args.append("--check")
|
||||
sdk_format_args.append("--check")
|
||||
scripts_format_args.append("--check")
|
||||
# `ruff check --diff` reports lint-driven rewrites without changing files.
|
||||
# It is the check-mode counterpart of `--fix --fix-only`, not a full lint gate.
|
||||
sdk_lint_args = ["ruff", "check", "--diff"]
|
||||
else:
|
||||
# Ruff's lint fixer and formatter are separate passes: the first applies
|
||||
# fixable lint rewrites, while the second formats source layout.
|
||||
sdk_lint_args = ["ruff", "check", "--fix", "--fix-only"]
|
||||
|
||||
return (
|
||||
FormatterGroup("Just", (Command(tuple(just_args)),)),
|
||||
FormatterGroup(
|
||||
"Rust",
|
||||
# Stable rustfmt repeats a nightly-only `imports_granularity` warning
|
||||
# for each crate, so suppress that expected stderr noise.
|
||||
(Command(tuple(cargo_args), CODEX_RS_ROOT, discard_stderr=True),),
|
||||
),
|
||||
FormatterGroup(
|
||||
"Python SDK",
|
||||
(
|
||||
Command(
|
||||
(
|
||||
*sdk_uv_run_args,
|
||||
*sdk_lint_args,
|
||||
"sdk/python",
|
||||
)
|
||||
),
|
||||
Command((*sdk_format_args, "sdk/python")),
|
||||
),
|
||||
),
|
||||
FormatterGroup(
|
||||
"Python scripts",
|
||||
(
|
||||
# The SDK and internal scripts intentionally use separate project
|
||||
# roots so uv and Ruff retain each project's configuration context.
|
||||
Command((*scripts_format_args, "scripts")),
|
||||
),
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
def run_formatter_group(group: FormatterGroup) -> FormatterResult:
|
||||
"""Run one formatter group sequentially and return its buffered output."""
|
||||
output: list[str] = []
|
||||
for command in group.commands:
|
||||
output.append(f"$ {shlex.join(command.args)}\n")
|
||||
try:
|
||||
process = subprocess.run(
|
||||
command.args,
|
||||
cwd=command.cwd,
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.DEVNULL
|
||||
if command.discard_stderr
|
||||
else subprocess.STDOUT,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
except OSError as error:
|
||||
output.append(f"{error}\n")
|
||||
return FormatterResult(group.name, "".join(output), 1)
|
||||
|
||||
output.append(process.stdout)
|
||||
if process.stdout and not process.stdout.endswith("\n"):
|
||||
output.append("\n")
|
||||
if process.returncode != 0:
|
||||
return FormatterResult(group.name, "".join(output), process.returncode)
|
||||
|
||||
return FormatterResult(group.name, "".join(output), 0)
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser = argparse.ArgumentParser()
|
||||
parser.add_argument(
|
||||
"--check",
|
||||
action="store_true",
|
||||
help="check formatting without modifying files",
|
||||
)
|
||||
args = parser.parse_args()
|
||||
groups = formatter_groups(check=args.check)
|
||||
|
||||
failures: list[str] = []
|
||||
with ThreadPoolExecutor(max_workers=len(groups)) as executor:
|
||||
futures = {}
|
||||
for group in groups:
|
||||
print(f"Starting {group.name} formatter...", flush=True)
|
||||
futures[executor.submit(run_formatter_group, group)] = group.name
|
||||
for future in as_completed(futures):
|
||||
result = future.result()
|
||||
print(f"==> {result.name} formatter finished")
|
||||
print(result.output, end="")
|
||||
if result.returncode != 0:
|
||||
failures.append(result.name)
|
||||
|
||||
if failures:
|
||||
print(f"Formatting failed: {', '.join(failures)}", file=sys.stderr)
|
||||
return 1
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main())
|
||||
@@ -14,6 +14,18 @@ import tomllib
|
||||
ROOT = Path(__file__).resolve().parents[1]
|
||||
|
||||
|
||||
def _load_root_format_script_module():
|
||||
"""Load the root formatter driver so tests exercise its real command graph."""
|
||||
script_path = ROOT.parents[1] / "scripts" / "format.py"
|
||||
spec = importlib.util.spec_from_file_location("format_repo", script_path)
|
||||
if spec is None or spec.loader is None:
|
||||
raise AssertionError(f"Failed to load script module: {script_path}")
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
sys.modules[spec.name] = module
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
|
||||
def _load_update_script_module():
|
||||
"""Load the maintenance script as a module so tests exercise real helpers."""
|
||||
script_path = ROOT / "scripts" / "update_sdk_artifacts.py"
|
||||
@@ -68,44 +80,135 @@ def test_generation_has_single_maintenance_entrypoint_script() -> None:
|
||||
assert scripts == ["update_sdk_artifacts.py"]
|
||||
|
||||
|
||||
def test_root_fmt_recipe_formats_rust_python_sdk_and_scripts() -> None:
|
||||
"""The repo fmt command should format Rust, the Python SDK, and scripts."""
|
||||
def test_root_fmt_recipes_use_shared_formatter_driver() -> None:
|
||||
"""The root formatting recipes should use the shared cross-platform driver."""
|
||||
justfile = ROOT.parents[1] / "justfile"
|
||||
lines = justfile.read_text().splitlines()
|
||||
fmt_index = lines.index("fmt:")
|
||||
fmt_check_index = lines.index("fmt-check:")
|
||||
next_recipe_index = next(
|
||||
index
|
||||
for index in range(fmt_index + 1, len(lines))
|
||||
for index in range(fmt_check_index + 1, len(lines))
|
||||
if lines[index] and not lines[index].startswith((" ", "\t", "#"))
|
||||
)
|
||||
fmt_recipe = lines[fmt_index:next_recipe_index]
|
||||
actual = {
|
||||
"working_directory": lines[0],
|
||||
"previous_comment": next(
|
||||
line for line in reversed(lines[:fmt_index]) if line.startswith("#")
|
||||
"fmt_comment": next(line for line in reversed(lines[:fmt_index]) if line.startswith("#")),
|
||||
"fmt_commands": [
|
||||
line.strip()
|
||||
for line in lines[fmt_index + 1 : fmt_check_index]
|
||||
if line.strip() and not line.startswith("#")
|
||||
],
|
||||
"fmt_check_comment": next(
|
||||
line for line in reversed(lines[:fmt_check_index]) if line.startswith("#")
|
||||
),
|
||||
"commands": [line.strip() for line in fmt_recipe[1:] if line.strip()],
|
||||
"fmt_check_commands": [
|
||||
line.strip() for line in lines[fmt_check_index + 1 : next_recipe_index] if line.strip()
|
||||
],
|
||||
}
|
||||
expected = {
|
||||
"working_directory": 'set working-directory := "codex-rs"',
|
||||
"previous_comment": "# Format Rust, Python SDK code, and Python scripts.",
|
||||
"commands": [
|
||||
"cargo fmt -- --config imports_granularity=Item {stderr-null}",
|
||||
"uv run --frozen --project ../sdk/python --extra dev ruff check --fix --fix-only ../sdk/python",
|
||||
"uv run --frozen --project ../sdk/python --extra dev ruff format ../sdk/python",
|
||||
"# Root scripts have their own locked Ruff environment.",
|
||||
"uv run --frozen --project ../scripts ruff format ../scripts",
|
||||
],
|
||||
"fmt_comment": "# Format the justfile, Rust, Python SDK code, and Python scripts.",
|
||||
"fmt_commands": ["{{ python }} ../scripts/format.py"],
|
||||
"fmt_check_comment": "# Check formatting without modifying files.",
|
||||
"fmt_check_commands": ["{{ python }} ../scripts/format.py --check"],
|
||||
}
|
||||
|
||||
assert actual == expected, (
|
||||
"The root `just fmt` recipe must run Rust fmt and Ruff for Python SDK code and scripts. "
|
||||
"Fix the `fmt` recipe in `justfile`, then run `just fmt`.\n"
|
||||
"The root formatting recipes must use the shared formatter driver. "
|
||||
"Fix the recipes in `justfile`, then run `just fmt`.\n"
|
||||
f"Expected: {json.dumps(expected, indent=2)}\n"
|
||||
f"Actual: {json.dumps(actual, indent=2)}"
|
||||
)
|
||||
|
||||
|
||||
def test_root_format_driver_covers_all_formatter_groups() -> None:
|
||||
"""The shared driver should retain every formatter in both modes."""
|
||||
script = _load_root_format_script_module()
|
||||
formatters = script.formatter_groups(check=False)
|
||||
checks = script.formatter_groups(check=True)
|
||||
|
||||
assert [group.name for group in formatters] == [
|
||||
"Just",
|
||||
"Rust",
|
||||
"Python SDK",
|
||||
"Python scripts",
|
||||
]
|
||||
assert [group.name for group in checks] == [group.name for group in formatters]
|
||||
assert [len(group.commands) for group in formatters] == [1, 1, 2, 1]
|
||||
assert [len(group.commands) for group in checks] == [
|
||||
len(group.commands) for group in formatters
|
||||
]
|
||||
sdk_uv_run_args = (
|
||||
"uv",
|
||||
"run",
|
||||
"--frozen",
|
||||
"--project",
|
||||
"sdk/python",
|
||||
"--no-sync",
|
||||
"--with",
|
||||
"ruff",
|
||||
)
|
||||
scripts_uv_run_args = (
|
||||
"uv",
|
||||
"run",
|
||||
"--frozen",
|
||||
"--project",
|
||||
"scripts",
|
||||
"--no-sync",
|
||||
"--with",
|
||||
"ruff",
|
||||
)
|
||||
assert all(
|
||||
command.args[: len(sdk_uv_run_args)] == sdk_uv_run_args
|
||||
for group in (formatters[2], checks[2])
|
||||
for command in group.commands
|
||||
)
|
||||
assert all(
|
||||
command.args[: len(scripts_uv_run_args)] == scripts_uv_run_args
|
||||
for group in (formatters[3], checks[3])
|
||||
for command in group.commands
|
||||
)
|
||||
assert formatters[2].commands[0].args[-5:] == (
|
||||
"ruff",
|
||||
"check",
|
||||
"--fix",
|
||||
"--fix-only",
|
||||
"sdk/python",
|
||||
)
|
||||
assert checks[2].commands[0].args[-4:] == (
|
||||
"ruff",
|
||||
"check",
|
||||
"--diff",
|
||||
"sdk/python",
|
||||
)
|
||||
assert formatters[0].commands[-1].args == ("just", "--unstable", "--fmt")
|
||||
assert checks[0].commands[-1].args == ("just", "--unstable", "--fmt", "--check")
|
||||
assert formatters[1].commands[-1].args == (
|
||||
"cargo",
|
||||
"fmt",
|
||||
"--",
|
||||
"--config",
|
||||
"imports_granularity=Item",
|
||||
)
|
||||
assert checks[1].commands[-1].args == (
|
||||
"cargo",
|
||||
"fmt",
|
||||
"--",
|
||||
"--config",
|
||||
"imports_granularity=Item",
|
||||
"--check",
|
||||
)
|
||||
assert [group.commands[-1].args[-3:] for group in formatters[2:]] == [
|
||||
("ruff", "format", "sdk/python"),
|
||||
("ruff", "format", "scripts"),
|
||||
]
|
||||
assert [group.commands[-1].args[-4:] for group in checks[2:]] == [
|
||||
("ruff", "format", "--check", "sdk/python"),
|
||||
("ruff", "format", "--check", "scripts"),
|
||||
]
|
||||
|
||||
|
||||
def test_generate_types_wires_all_generation_steps() -> None:
|
||||
"""The type generation command should refresh every schema-derived artifact."""
|
||||
source = (ROOT / "scripts" / "update_sdk_artifacts.py").read_text()
|
||||
|
||||
Reference in New Issue
Block a user