Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4fa041e7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| fi | ||
|
|
||
| lint_args+=("$arg") |
There was a problem hiding this comment.
Skip storing
-- in lint args before forwarding cargo args
The parser appends every pre-separator token to lint_args before checking whether it is --, then later always appends another separator when cargo_args is non-empty. For calls that intentionally pass cargo flags (for example ./tools/argument-comment-lint/run.sh -p codex-core -- --tests), this builds ... -- -- --tests, so the forwarded target flag is no longer parsed as intended. This breaks the explicit target-selection override path introduced by this change; handle -- before pushing into lint_args (or suppress the extra separator when one was already consumed).
Useful? React with 👍 / 👎.
| continue | ||
| fi | ||
|
|
||
| lint_args+=("$arg") |
There was a problem hiding this comment.
Preserve single separator when forwarding prebuilt wrapper args
This wrapper has the same separator regression: -- is first stored in lint_args, and then a new -- is appended when reconstructing final_args with cargo_args. Any invocation that forwards cargo-level options via -- (for example ./tools/argument-comment-lint/run-prebuilt-linter.sh -p codex-core -- --tests) ends up with a doubled separator, so Cargo no longer receives the target-selection flag in the expected position. Please treat -- as a pure split marker instead of forwarding it twice.
Useful? React with 👍 / 👎.
28ac0c9 to
880a15a
Compare
Why
argument-comment-lintwas 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 thelatest_session_lookup_params(true, ...)tests incodex-rs/tui_app_server/src/lib.rsnever 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
argument-comment-lintviolations across thecodex-rsworkspace, including tests, examples, and benchestools/argument-comment-lint/run-prebuilt-linter.shandtools/argument-comment-lint/run.shso non---fixruns default to--all-targetsunless the caller explicitly narrows the target set--are preserved with a single separatortools/argument-comment-lint/README.mdrust-ciso the macOS lint lane keeps the plain wrapper invocation and therefore enforces--all-targets, while Linux and Windows temporarily pass-- --lib --binsThat 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-targetson 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.shbash -n tools/argument-comment-lint/run-prebuilt-linter.sh-- --lib --bins-- --testsjust argument-comment-lintcargo testintools/argument-comment-lintcargo test -p codex-terminal-detectionFollow-up