Skip to content

fix(NATSRS-007): 4 review findings in test.yml #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+3 −5
Draft

fix(NATSRS-007): 4 review findings in test.yml #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ jobs:
with:
go-version: '1.22'
- name: Install nats-server
run: go install github.com/nats-io/nats-server/v2@main
run: go install github.com/nats-io/nats-server/v2@v2.10.24
- name: Install stable Rust on ${{ matrix.os }}
id: install-rust
uses: dtolnay/rust-toolchain@stable
Comment on lines 46 to 52

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 test.yml also installs nats-server from @main, making test results non-reproducible

Pinned go install github.com/nats-io/nats-server/v2@main to go install github.com/nats-io/nats-server/v2@v2.10.24 on line 37. The version v2.10.24 is a recent stable release tag; the actual minimum supported server version for this client is not visible in this file alone, so a reviewer should verify the correct pin against the crate's documented minimum server version or latest stable release. Changed in the test_matrix job's Install nats-server step.

🤖 Prompt for AI agents 
In .github/workflows/test.yml around line 37, review and complete this code-review fix: test.yml also installs nats-server from @main, making test results non-reproducible.
What the draft fix changed: Pinned `go install github.com/nats-io/nats-server/v2@main` to `go install github.com/nats-io/nats-server/v2@v2.10.24` on line 37. The version `v2.10.24` is a recent stable release tag; the actual minimum supported server version for this client is not visible in this file alone, so a reviewer should verify the correct pin against the crate's documented minimum server version or latest stable release. Changed in the `test_matrix` job's `Install nats-server` step.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -95,9 +95,7 @@ jobs:
run: cargo fmt -- --check

check_lint:

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 check_format job runs cargo fmt on the entire workspace but check_lint only covers async-nats, creating inconsistent coverage

This finding is resolved as a direct consequence of fix #1: by removing the working-directory: ./async-nats default from check_lint and adding --workspace, the clippy coverage now matches the full-workspace scope of check_format. No additional change was needed beyond what fix #1 already applied.

🤖 Prompt for AI agents 
In .github/workflows/test.yml around line 97, review and complete this code-review fix: check_format job runs cargo fmt on the entire workspace but check_lint only covers async-nats, creating inconsistent coverage.
What the draft fix changed: This finding is resolved as a direct consequence of fix #1: by removing the `working-directory: ./async-nats` default from `check_lint` and adding `--workspace`, the clippy coverage now matches the full-workspace scope of `check_format`. No additional change was needed beyond what fix #1 already applied.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

defaults:
run:
working-directory: ./async-nats
needs: [test_matrix]
name: check linter (ubuntu-latest / stable)
runs-on: ubuntu-latest
steps:
Expand All @@ -116,7 +114,7 @@ jobs:
save-if: false # the linter only run checks but not builds, so we don't have the full build to be cached.

- name: Run linter
run: cargo clippy --benches --tests --examples --all-features -- --deny clippy::all
run: cargo clippy --workspace --benches --tests --examples --all-features -- --deny clippy::all

check_docs:
defaults:
Comment on lines 114 to 120

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 clippy check_lint job scoped only to async-nats, nats/ crate never linted

Removed the defaults: run: working-directory: ./async-nats block from check_lint entirely, and changed the Run linter step from cargo clippy --benches --tests --examples --all-features -- --deny clippy::all to cargo clippy --workspace --benches --tests --examples --all-features -- --deny clippy::all. The command now runs from the repo root (no working-directory override) and covers all workspace crates including nats/ and nats-server/.

🤖 Prompt for AI agents 
In .github/workflows/test.yml around line 113, review and complete this code-review fix: clippy check_lint job scoped only to async-nats, nats/ crate never linted.
What the draft fix changed: Removed the `defaults: run: working-directory: ./async-nats` block from `check_lint` entirely, and changed the `Run linter` step from `cargo clippy --benches --tests --examples --all-features -- --deny clippy::all` to `cargo clippy --workspace --benches --tests --examples --all-features -- --deny clippy::all`. The command now runs from the repo root (no working-directory override) and covers all workspace crates including `nats/` and `nats-server/`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

Comment on lines 114 to 120

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 check_lint CI job uses save-if: false on rust-cache but the cache was never populated by this job, so it always starts cold

Added needs: [test_matrix] to the check_lint job so it runs after test_matrix has had a chance to populate the Rust build cache. This means check_lint will no longer race against test_matrix for a cold cache. Risk: check_lint now only runs after all three test_matrix matrix legs complete (ubuntu, macos, windows), which may add latency to the lint feedback. A reviewer may prefer needs: [test_matrix] scoped to only the ubuntu leg, but that is not expressible without restructuring the matrix.

🤖 Prompt for AI agents 
In .github/workflows/test.yml around line 113, review and complete this code-review fix: check_lint CI job uses save-if: false on rust-cache but the cache was never populated by this job, so it always starts cold.
What the draft fix changed: Added `needs: [test_matrix]` to the `check_lint` job so it runs after `test_matrix` has had a chance to populate the Rust build cache. This means `check_lint` will no longer race against `test_matrix` for a cold cache. Risk: `check_lint` now only runs after all three `test_matrix` matrix legs complete (ubuntu, macos, windows), which may add latency to the lint feedback. A reviewer may prefer `needs: [test_matrix]` scoped to only the ubuntu leg, but that is not expressible without restructuring the matrix.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

Expand Down
Loading