diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9adc90e04..63c6ffe52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 diff --git a/justfile b/justfile index 2c57e1cdf..34b5115ee 100644 --- a/justfile +++ b/justfile @@ -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); \ diff --git a/scripts/format.py b/scripts/format.py new file mode 100644 index 000000000..0c64d2aa8 --- /dev/null +++ b/scripts/format.py @@ -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()) diff --git a/sdk/python/tests/test_artifact_workflow_and_binaries.py b/sdk/python/tests/test_artifact_workflow_and_binaries.py index 78fbdd623..a8dda4885 100644 --- a/sdk/python/tests/test_artifact_workflow_and_binaries.py +++ b/sdk/python/tests/test_artifact_workflow_and_binaries.py @@ -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()