Compare commits

...

6 Commits

Author SHA1 Message Date
Michael Bolin
086ca7c728 bazel: lint rust_test targets in clippy workflow 2026-04-01 09:24:07 -07:00
Michael Bolin
75365bf718 fix: remove unused import (#16449)
https://github.com/openai/codex/pull/16433 resulted in an unused import
inside `mod tests`. This is flagged by `cargo clippy --tests`, which is
run as part of
https://github.com/openai/codex/actions/workflows/rust-ci-full.yml, but
is not caught by our current Bazel setup for clippy.

Fixing this ASAP to get
https://github.com/openai/codex/actions/workflows/rust-ci-full.yml green
again, but am looking at fixing the Bazel workflow in parallel.
2026-04-01 09:14:29 -07:00
Michael Bolin
5cca5c0093 docs: update argument_comment_lint instructions in AGENTS.md (#16375)
I noticed that Codex was spending more time on running this lint check
locally than I would like. Now that we have the linter running
cross-platform using Bazel in CI, I find it's best just to update the PR
ASAP to get CI going than to wait for `just argument-comment-lint` to
finish locally before updating the PR.
2026-04-01 15:44:34 +00:00
Dylan Hurd
d3b99ef110 fix(core) rm execute_exec_request sandbox_policy (#16422)
## Summary
In #11871 we started consolidating on ExecRequest.sandbox_policy instead
of passing in a separate policy object that theoretically could differ
(but did not). This finishes the some parameter cleanup.

This should be a simple noop, since all 3 callsites of this function
already used a cloned object from the ExecRequest value.

## Testing
- [x] Existing tests pass
2026-04-01 11:03:48 -04:00
jif-oai
f839f3ff2e feat: auto vaccum state DB (#16434)
Start with a full vaccum the first time, then auto-vaccum incremental
2026-04-01 16:46:21 +02:00
jif-oai
c846a57d03 chore: drop log DB (#16433)
Drop the log table from the state DB
2026-04-01 15:49:17 +02:00
11 changed files with 62 additions and 38 deletions

View File

@@ -5,15 +5,15 @@ The workflows in this directory are split so that pull requests get fast, review
## Pull Requests
- `bazel.yml` is the main pre-merge verification path for Rust code.
It runs Bazel `test` and Bazel `clippy` on the supported Bazel targets.
It runs Bazel `test` and Bazel `clippy` on the supported Bazel targets,
including the generated Rust test binaries needed to lint inline `#[cfg(test)]`
code.
- `rust-ci.yml` keeps the Cargo-native PR checks intentionally small:
- `cargo fmt --check`
- `cargo shear`
- `argument-comment-lint` on Linux, macOS, and Windows
- `tools/argument-comment-lint` package tests when the lint or its workflow wiring changes
The PR workflow still keeps the Linux lint lane on the default-targets-only invocation for now, but the released linter runs on Linux, macOS, and Windows before merge.
## Post-Merge On `main`
- `bazel.yml` also runs on pushes to `main`.

View File

@@ -126,13 +126,17 @@ jobs:
with:
target: ${{ matrix.target }}
- name: bazel build --config=clippy //codex-rs/...
- name: bazel build --config=clippy lint targets
env:
BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }}
shell: bash
run: |
# Keep the initial Bazel clippy scope on codex-rs and out of the
# V8 proof-of-concept target for now.
bazel_target_lines="$(./scripts/list-bazel-clippy-targets.sh)"
bazel_targets=()
while IFS= read -r target; do
bazel_targets+=("${target}")
done <<< "${bazel_target_lines}"
./.github/scripts/run-bazel-ci.sh \
-- \
build \
@@ -140,8 +144,7 @@ jobs:
--build_metadata=COMMIT_SHA=${GITHUB_SHA} \
--build_metadata=TAG_job=clippy \
-- \
//codex-rs/... \
-//codex-rs/v8-poc:all
"${bazel_targets[@]}"
# Save bazel repository cache explicitly; make non-fatal so cache uploading
# never fails the overall job. Only save when key wasn't hit.

View File

@@ -15,7 +15,8 @@ In the codex-rs folder where the rust code lives:
- When you cannot make that API change and still need a small positional-literal callsite in Rust, follow the `argument_comment_lint` convention:
- Use an exact `/*param_name*/` comment before opaque literal arguments such as `None`, booleans, and numeric literals when passing them by position.
- Do not add these comments for string or char literals unless the comment adds real clarity; those literals are intentionally exempt from the lint.
- If you add one of these comments, the parameter name must exactly match the callee signature.
- The parameter name in the comment must exactly match the callee signature.
- You can run `just argument-comment-lint` to run the lint check locally. This is powered by Bazel, so running it the first time can be slow if Bazel is not warmed up, though incremental invocations should take <15s. Most of the time, it is best to update the PR and let CI take responsibility for checking this (or run it asynchronously in the background after submitting the PR). Note CI checks all three platforms, which the local run does not.
- When possible, make `match` statements exhaustive and avoid wildcard arms.
- Newly added traits should include doc comments that explain their role and how implementations are expected to use them.
- When writing tests, prefer comparing the equality of entire objects over fields one by one.
@@ -50,8 +51,6 @@ Run `just fmt` (in `codex-rs` directory) automatically after you have finished m
Before finalizing a large change to `codex-rs`, run `just fix -p <project>` (in `codex-rs` directory) to fix any linter issues in the code. Prefer scoping with `-p` to avoid slow workspacewide Clippy builds; only run `just fix` without `-p` if you changed shared crates. Do not re-run tests after running `fix` or `fmt`.
Also run `just argument-comment-lint` to ensure the codebase is clean of comment lint errors.
## The `codex-core` crate
Over time, the `codex-core` crate (defined in `codex-rs/core/`) has become bloated because it is the largest crate, so it is often easier to add something new to `codex-core` rather than refactor out the library code you need so your new code neither takes a dependency on, nor contributes to the size of, `codex-core`.

View File

@@ -314,7 +314,6 @@ pub fn build_exec_request(
pub(crate) async fn execute_exec_request(
exec_request: ExecRequest,
sandbox_policy: &SandboxPolicy,
stdout_stream: Option<StdoutStream>,
after_spawn: Option<Box<dyn FnOnce() + Send>>,
) -> Result<ExecToolCallOutput> {
@@ -328,13 +327,12 @@ pub(crate) async fn execute_exec_request(
sandbox,
windows_sandbox_level,
windows_sandbox_private_desktop,
sandbox_policy: _sandbox_policy_from_env,
sandbox_policy,
file_system_sandbox_policy,
network_sandbox_policy,
windows_restricted_token_filesystem_overlay,
arg0,
} = exec_request;
let _ = _sandbox_policy_from_env;
let params = ExecParams {
command,
@@ -354,7 +352,7 @@ pub(crate) async fn execute_exec_request(
let raw_output_result = exec(
params,
sandbox,
sandbox_policy,
&sandbox_policy,
&file_system_sandbox_policy,
windows_restricted_token_filesystem_overlay.as_ref(),
network_sandbox_policy,

View File

@@ -141,14 +141,7 @@ pub async fn execute_env(
exec_request: ExecRequest,
stdout_stream: Option<StdoutStream>,
) -> crate::error::Result<ExecToolCallOutput> {
let effective_policy = exec_request.sandbox_policy.clone();
execute_exec_request(
exec_request,
&effective_policy,
stdout_stream,
/*after_spawn*/ None,
)
.await
execute_exec_request(exec_request, stdout_stream, /*after_spawn*/ None).await
}
pub async fn execute_exec_request_with_after_spawn(
@@ -156,6 +149,5 @@ pub async fn execute_exec_request_with_after_spawn(
stdout_stream: Option<StdoutStream>,
after_spawn: Option<Box<dyn FnOnce() + Send>>,
) -> crate::error::Result<ExecToolCallOutput> {
let effective_policy = exec_request.sandbox_policy.clone();
execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn).await
execute_exec_request(exec_request, stdout_stream, after_spawn).await
}

View File

@@ -185,14 +185,9 @@ pub(crate) async fn execute_user_shell_command(
tx_event: session.get_tx_event(),
});
let exec_result = execute_exec_request(
exec_env,
&sandbox_policy,
stdout_stream,
/*after_spawn*/ None,
)
.or_cancel(&cancellation_token)
.await;
let exec_result = execute_exec_request(exec_env, stdout_stream, /*after_spawn*/ None)
.or_cancel(&cancellation_token)
.await;
match exec_result {
Err(CancelErr::Cancelled) => {

View File

@@ -0,0 +1,3 @@
PRAGMA auto_vacuum = INCREMENTAL;
DROP TABLE IF EXISTS logs;

View File

@@ -147,12 +147,28 @@ fn base_sqlite_options(path: &Path) -> SqliteConnectOptions {
}
async fn open_state_sqlite(path: &Path, migrator: &'static Migrator) -> anyhow::Result<SqlitePool> {
let options = base_sqlite_options(path);
let options = base_sqlite_options(path).auto_vacuum(SqliteAutoVacuum::Incremental);
let pool = SqlitePoolOptions::new()
.max_connections(5)
.connect_with(options)
.await?;
migrator.run(&pool).await?;
let auto_vacuum = sqlx::query_scalar::<_, i64>("PRAGMA auto_vacuum")
.fetch_one(&pool)
.await?;
if auto_vacuum != SqliteAutoVacuum::Incremental as i64 {
// Existing state DBs need one non-transactional `VACUUM` before
// SQLite persists `auto_vacuum = INCREMENTAL` in the database header.
sqlx::query("PRAGMA auto_vacuum = INCREMENTAL")
.execute(&pool)
.await?;
// We do it on best effort. If the lock can't be acquired, it will be done at next run.
let _ = sqlx::query("VACUUM").execute(&pool).await;
}
// We do it on best effort. If the lock can't be acquired, it will be done at next run.
let _ = sqlx::query("PRAGMA incremental_vacuum")
.execute(&pool)
.await;
Ok(pool)
}

View File

@@ -537,7 +537,6 @@ mod tests {
use crate::LogQuery;
use crate::logs_db_path;
use crate::migrations::LOGS_MIGRATOR;
use crate::state_db_path;
use chrono::Utc;
use pretty_assertions::assert_eq;
use sqlx::SqlitePool;
@@ -590,10 +589,8 @@ mod tests {
.await
.expect("insert test logs");
let state_count = log_row_count(state_db_path(codex_home.as_path()).as_path()).await;
let logs_count = log_row_count(logs_db_path(codex_home.as_path()).as_path()).await;
assert_eq!(state_count, 0);
assert_eq!(logs_count, 1);
let _ = tokio::fs::remove_dir_all(codex_home).await;

View File

@@ -69,8 +69,9 @@ bazel-lock-check:
bazel-test:
bazel test --test_tag_filters=-argument-comment-lint //... --keep_going
[no-cd]
bazel-clippy:
bazel build --config=clippy -- //codex-rs/... -//codex-rs/v8-poc:all
bazel_targets="$(./scripts/list-bazel-clippy-targets.sh)" && bazel build --config=clippy -- ${bazel_targets}
[no-cd]
bazel-argument-comment-lint:

View File

@@ -0,0 +1,20 @@
#!/usr/bin/env bash
set -euo pipefail
repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
cd "${repo_root}"
# Resolve the dynamic targets before printing anything so callers do not
# continue with a partial list if `bazel query` fails.
manual_rust_test_targets="$(bazel query 'kind("rust_test rule", attr(tags, "manual", //codex-rs/... except //codex-rs/v8-poc/...))')"
printf '%s\n' \
"//codex-rs/..." \
"-//codex-rs/v8-poc:all"
# `--config=clippy` on the `workspace_root_test` wrappers does not lint the
# underlying `rust_test` binaries. Add the internal manual `*-unit-tests-bin`
# targets explicitly so inline `#[cfg(test)]` code is linted like
# `cargo clippy --tests`.
printf '%s\n' "${manual_rust_test_targets}"