Skip to content

Apply argument comment lint across codex-rs#14652

Merged
bolinfest merged 1 commit intomainfrom
pr14652
Mar 16, 2026
Merged

Apply argument comment lint across codex-rs#14652
bolinfest merged 1 commit intomainfrom
pr14652

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 14, 2026

Why

Once the repo-local lint exists, codex-rs needs to follow the checked-in convention and CI needs to keep it from drifting. This commit applies the fallback /*param*/ style consistently across existing positional literal call sites without changing those APIs.

The longer-term preference is still to avoid APIs that require comments by choosing clearer parameter types and call shapes. This PR is intentionally the mechanical follow-through for the places where the existing signatures stay in place.

After rebasing onto newer main, the rollout also had to cover newly introduced tui_app_server call sites. That made it clear the first cut of the CI job was too expensive for the common path: it was spending almost as much time installing cargo-dylint and re-testing the lint crate as a representative test job spends running product tests. The CI update keeps the full workspace enforcement but trims that extra overhead from ordinary codex-rs PRs.

What changed

  • keep a dedicated argument_comment_lint job in rust-ci
  • mechanically annotate remaining opaque positional literals across codex-rs with exact /*param*/ comments, including the rebased tui_app_server call sites that now fall under the lint
  • keep the checked-in style aligned with the lint policy by using /*param*/ and leaving string and char literals uncommented
  • cache cargo-dylint, dylint-link, and the relevant Cargo registry/git metadata in the lint job
  • split changed-path detection so the lint crate's own cargo test step runs only when tools/argument-comment-lint/* or rust-ci.yml changes
  • continue to run the repo wrapper over the codex-rs workspace, so product-code enforcement is unchanged

Most of the code changes in this commit are intentionally mechanical comment rewrites or insertions driven by the lint itself.

Verification

  • ./tools/argument-comment-lint/run.sh --workspace
  • cargo test -p codex-tui-app-server -p codex-tui
  • parsed .github/workflows/rust-ci.yml locally with PyYAML

@bolinfest bolinfest force-pushed the pr14651 branch 2 times, most recently from 139eaf7 to ce9a5a6 Compare March 14, 2026 01:05
@bolinfest bolinfest force-pushed the pr14652 branch 3 times, most recently from 07f8ccc to 5b505bb Compare March 14, 2026 01:42
@bolinfest bolinfest force-pushed the pr14652 branch 2 times, most recently from 2e751fd to cfc2e47 Compare March 14, 2026 01:52
@bolinfest bolinfest force-pushed the pr14652 branch 2 times, most recently from 7e41132 to 7308616 Compare March 14, 2026 02:20
@bolinfest bolinfest force-pushed the pr14652 branch 4 times, most recently from f98685f to f9a06aa Compare March 14, 2026 03:59
Base automatically changed from pr14651 to main March 14, 2026 15:18
apply_patch_tool_type: None,
web_search_tool_type: Default::default(),
truncation_policy: TruncationPolicyConfig::bytes(10_000),
truncation_policy: TruncationPolicyConfig::bytes(/*limit*/ 10_000),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like one example of where this is a little overeager - it's the only place I found in reading a majority of the fixes though, so no big deal (and not worth fixing).

@bolinfest bolinfest force-pushed the pr14652 branch 2 times, most recently from 3193fbb to 95eb498 Compare March 16, 2026 22:29
@bolinfest bolinfest merged commit b77fe8f into main Mar 16, 2026
54 of 56 checks passed
@bolinfest bolinfest deleted the pr14652 branch March 16, 2026 23:48
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants