Add rustflags passthrough to setup-rust and rust-build-release - #391
Open
leynos wants to merge 10 commits into
Open
Add rustflags passthrough to setup-rust and rust-build-release#391leynos wants to merge 10 commits into
leynos wants to merge 10 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Targeted tests, Ruff, formatting checks, and Markdown linting passed; the full run reported 1,079 tests passed and 7 skipped. WalkthroughThe pull request adds configurable ChangesRust flags configuration
Repository maintenance
Documentation formatting
Sequence Diagram(s)sequenceDiagram
participant Caller
participant rust-build-release
participant GITHUB_ENV
participant setup-rust
participant RustToolchain
Caller->>rust-build-release: supply rustflags
rust-build-release->>GITHUB_ENV: export RUSTFLAGS before setup
rust-build-release->>setup-rust: invoke toolchain setup
setup-rust->>RustToolchain: forward inputs.rustflags
RustToolchain-->>setup-rust: complete installation
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Reviewer's GuideAdds a configurable rustflags passthrough to the setup-rust and rust-build-release composite actions, including wiring into nested setup-rust-toolchain calls, a guarded export step in rust-build-release, tests, and documentation/changelog updates while preserving existing default behavior. Sequence diagram for rustflags passthrough in setup-rust and rust-build-releasesequenceDiagram
actor Workflow
participant RustBuildRelease as rust-build-release
participant SetupRust as setup-rust
participant SetupRustToolchain as setup-rust-toolchain
Workflow->>RustBuildRelease: run (inputs.rustflags)
alt inputs.rustflags != ''
RustBuildRelease->>RustBuildRelease: Export caller RUSTFLAGS
alt env.RUSTFLAGS already set
RustBuildRelease-->>RustBuildRelease: keep inherited RUSTFLAGS
else env.RUSTFLAGS unset
RustBuildRelease-->>RustBuildRelease: write RUSTFLAGS to GITHUB_ENV
end
else inputs.rustflags == ''
RustBuildRelease-->>RustBuildRelease: environment untouched
end
RustBuildRelease->>SetupRust: Setup Rust toolchain (rustflags input)
SetupRust->>SetupRustToolchain: call with rustflags: ${{ inputs.rustflags }}
alt env.RUSTFLAGS unset in setup-rust-toolchain
SetupRustToolchain-->>SetupRustToolchain: export RUSTFLAGS "-D warnings"
else env.RUSTFLAGS set
SetupRustToolchain-->>SetupRustToolchain: keep existing RUSTFLAGS
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
July 29, 2026 12:40
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f667b1b631
ℹ️ 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".
Comment on lines
+85
to
+87
| echo "RUSTFLAGS<<__RBR_RUSTFLAGS_EOF__" | ||
| printf '%s\n' "$RBR_RUSTFLAGS" | ||
| echo "__RBR_RUSTFLAGS_EOF__" |
There was a problem hiding this comment.
When rustflags contains a line exactly equal to __RBR_RUSTFLAGS_EOF__, this fixed marker terminates the value early and GitHub parses the remaining caller-provided lines as additional environment-file commands; that can either fail the action with an invalid format or set unintended variables for subsequent setup and build steps. Generate a unique delimiter and verify it does not occur in the value, or reject multiline input before writing it.
AGENTS.md reference: AGENTS.md:L144-L144
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the action metadata and its directly referenced scripts without executing them.
ast-grep outline .github/actions/rust-build-release --items all 2>/dev/null || true
printf '\n--- RUSTFLAGS producer/consumer flow ---\n'
rg -n -C 5 \
'RBR_RUSTFLAGS|RUSTFLAGS|setup-rust|action_setup\.py|main\.py' \
.github/actions/rust-build-release .github/actions/setup-rust
printf '\n--- Current delimiter implementation ---\n'
sed -n '/name: Export caller RUSTFLAGS/,/name: Setup Rust toolchain/p' \
.github/actions/rust-build-release/action.ymlLength of output: 72850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the thread for the supplied review comment, its author, and whether it is already resolved.
gh api graphql \
-F owner='leynos' \
-F name='shared-actions' \
-F number=391 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 20) {
nodes {
url
author { login }
body
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .url == "https://github.com/leynos/shared-actions/pull/391#discussion_r3674325431"))
| {id, isResolved, comments}'Length of output: 2434
@leynos, treat this finding as resolved.
The codegraph/data-flow inspection confirms that inputs.rustflags now reaches $GITHUB_ENV through a per-run random delimiter, which is rejected if it occurs as a complete line in RBR_RUSTFLAGS. The regression test in .github/actions/rust-build-release/tests/test_manifest_input_step.py verifies that the former fixed-marker injection payload remains entirely inside RUSTFLAGS and does not create RBR_INJECTED.
Resolve thread PRRT_kwDOO9OxNc6UwZ-8 from the chatgpt-codex-connector account. I cannot resolve it through the API because that account, rather than coderabbitai, authored the original review comment.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. .github/actions/rust-build-release/tests/test_manifest_input_step.py Comment on lines +66 to +93 def _parse_env_file(text: str) -> dict[str, str]:
"""Parse ``GITHUB_ENV`` content, honouring heredoc-delimited values."""
values: dict[str, str] = {}
lines = text.splitlines()
index = 0
while index < len(lines):
line = lines[index]
index += 1
if not line:
continue
name, separator, remainder = line.partition("=")
if separator:
values[name] = remainder
continue
name, separator, delimiter = line.partition("<<")
if not separator:
message = f"unparsable environment-file line: {line!r}"
raise AssertionError(message)
collected: list[str] = []
while index < len(lines) and lines[index] != delimiter:
collected.append(lines[index])
index += 1
if index >= len(lines):
message = f"unterminated heredoc for {name}"
raise AssertionError(message)
index += 1
values[name] = "\n".join(collected)
return values❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
manifest=".github/actions/rust-build-release/action.yml"
test_file=".github/actions/rust-build-release/tests/test_manifest_input_step.py"
printf '%s\n' '--- manifest outline ---'
ast-grep outline "$manifest" --items all --view expanded || true
printf '%s\n' '--- action manifest rustflags-related sections ---'
rg -n -C 12 'rustflags|RUSTFLAGS|Setup Rust toolchain|GITHUB_ENV' "$manifest" || true
printf '%s\n' '--- focused test module outline ---'
ast-grep outline "$test_file" --items all --view expanded || true
printf '%s\n' '--- export helper and tests ---'
rg -n -C 10 '_run_export_script|test_export_rustflags|RUSTFLAGS|bash' "$test_file" || true
printf '%s\n' '--- all Bash -v environment-presence checks ---'
rg -n -C 4 '\[\[\s+-v\s+[A-Za-z_][A-Za-z0-9_]*\s*\]\]' .github --glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob '*.bash' || true
printf '%s\n' '--- portable environment-presence guard patterns ---'
rg -n -C 3 '\$\{[A-Za-z_][A-Za-z0-9_]*\+x\}|\$\{[A-Za-z_][A-Za-z0-9_]*:-|\$\{[A-Za-z_][A-Za-z0-9_]*\+[^}]' .github/actions --glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob '*.bash' || trueLength of output: 21673 🤖 web_search executed:
Length of output: 7105
Fix the macOS Bash 3.2 incompatibility in the inherited- CauseThe export script uses: if [[ -v RUSTFLAGS ]]; then
Replace it with the Bash-3.2-compatible setness expansion: if [[ ${RUSTFLAGS+x} ]]; thenThis retains the required behaviour under
Patch--- a/.github/actions/rust-build-release/action.yml
+++ b/.github/actions/rust-build-release/action.yml
@@
- if [[ -v RUSTFLAGS ]]; then
+ if [[ ${RUSTFLAGS+x} ]]; then
echo "RUSTFLAGS already set; leaving the inherited value in place" >&2
exit 0
fiUpdate the manifest-wiring assertion in - assert "-v RUSTFLAGS" in run_script
+ assert "${RUSTFLAGS+x}" in run_scriptAdd a runtime regression test which invokes
AI coding-agent prompt:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coverage:
- Around line 52-53: Remove the machine-specific .coverage artifact from the
change, add .coverage to the repository’s ignore configuration, and leave
coverage generation to CI. Do not retain the absolute worktree path; only
preserve repository-relative coverage data if intentionally versioned.
In @.github/actions/rust-build-release/action.yml:
- Around line 81-84: The inherited RUSTFLAGS check in
.github/actions/rust-build-release/action.yml lines 81-84 must use the Bash
3.2-compatible set-variable form [[ -n ${RUSTFLAGS+x} ]] instead of [[ -v
RUSTFLAGS ]]; update the corresponding assertion in
.github/actions/rust-build-release/tests/test_manifest_input_step.py line 197 to
expect the new guard.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 55-56: Update the subprocess.run call in the test to pass
check=False explicitly, preserving the existing manual returncode assertion and
command execution behavior.
In @.github/actions/setup-rust/action.yml:
- Around line 44-46: Insert a comma before “so” in the RUSTFLAGS description at
.github/actions/setup-rust/action.yml lines 44-46 and the corresponding README
text at .github/actions/setup-rust/README.md line 23; make no other wording
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d36f68c-9f28-483d-abf7-bcb8dd8380d8
📒 Files selected for processing (25)
.coverage.github/actions/linux-packages/scripts/package.py.github/actions/release-to-pypi-uv/tests/_helpers.py.github/actions/rust-build-release/CHANGELOG.md.github/actions/rust-build-release/README.md.github/actions/rust-build-release/action.yml.github/actions/rust-build-release/tests/test_manifest_input_step.py.github/actions/setup-rust/CHANGELOG.md.github/actions/setup-rust/README.md.github/actions/setup-rust/action.yml.github/actions/setup-rust/tests/test_setup_rust_manifest.py.rules/python-00.md.rules/python-context-managers.md.rules/python-exception-design-raising-handling-and-logging.md.rules/python-generators.md.rules/python-return.md.rules/python-typing.mddocs/cmd-mox-users-guide.mddocs/execplans/2-4-5-extend-architecture-enforcement-to-orchestration-code.mddocs/execplans/add-mutation-testing-workflows.mddocs/execplans/support-cranelift-codegen.mddocs/local-validation-of-github-actions-with-act-and-pytest.mddocs/python-action-scripts.mddocs/scripting-standards.mdworkflow_scripts/graphql_client.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/polythene(auto-detected)
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coverage:
- Around line 52-53: Remove the machine-specific .coverage artifact from the
change, add .coverage to the repository’s ignore configuration, and leave
coverage generation to CI. Do not retain the absolute worktree path; only
preserve repository-relative coverage data if intentionally versioned.
In @.github/actions/rust-build-release/action.yml:
- Around line 81-84: The inherited RUSTFLAGS check in
.github/actions/rust-build-release/action.yml lines 81-84 must use the Bash
3.2-compatible set-variable form [[ -n ${RUSTFLAGS+x} ]] instead of [[ -v
RUSTFLAGS ]]; update the corresponding assertion in
.github/actions/rust-build-release/tests/test_manifest_input_step.py line 197 to
expect the new guard.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 55-56: Update the subprocess.run call in the test to pass
check=False explicitly, preserving the existing manual returncode assertion and
command execution behavior.
In @.github/actions/setup-rust/action.yml:
- Around line 44-46: Insert a comma before “so” in the RUSTFLAGS description at
.github/actions/setup-rust/action.yml lines 44-46 and the corresponding README
text at .github/actions/setup-rust/README.md line 23; make no other wording
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d36f68c-9f28-483d-abf7-bcb8dd8380d8
📒 Files selected for processing (25)
.coverage.github/actions/linux-packages/scripts/package.py.github/actions/release-to-pypi-uv/tests/_helpers.py.github/actions/rust-build-release/CHANGELOG.md.github/actions/rust-build-release/README.md.github/actions/rust-build-release/action.yml.github/actions/rust-build-release/tests/test_manifest_input_step.py.github/actions/setup-rust/CHANGELOG.md.github/actions/setup-rust/README.md.github/actions/setup-rust/action.yml.github/actions/setup-rust/tests/test_setup_rust_manifest.py.rules/python-00.md.rules/python-context-managers.md.rules/python-exception-design-raising-handling-and-logging.md.rules/python-generators.md.rules/python-return.md.rules/python-typing.mddocs/cmd-mox-users-guide.mddocs/execplans/2-4-5-extend-architecture-enforcement-to-orchestration-code.mddocs/execplans/add-mutation-testing-workflows.mddocs/execplans/support-cranelift-codegen.mddocs/local-validation-of-github-actions-with-act-and-pytest.mddocs/python-action-scripts.mddocs/scripting-standards.mdworkflow_scripts/graphql_client.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/polythene(auto-detected)
🛑 Comments failed to post (1)
.coverage (1)
52-53: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Information Disclosure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Remove the machine-specific coverage database from the change.
Line 52-53 records an absolute
/home/leynos/.../worktrees/...path, exposing local filesystem metadata and making the generated coverage artefact stale and non-portable. Remove.coveragefrom the patch, ignore it, and regenerate coverage in CI; retain only repository-relative data if versioning is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coverage around lines 52 - 53, Remove the machine-specific .coverage artifact from the change, add .coverage to the repository’s ignore configuration, and leave coverage generation to CI. Do not retain the absolute worktree path; only preserve repository-relative coverage data if intentionally versioned.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
.github/actions/setup-rust/action.yml (1)
41-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing comma before "so" — still present from prior review.
"leave RUSTFLAGS unset so an inherited value" needs a comma before "so" to join the two clauses correctly.
✏️ Fix
Value exported as the RUSTFLAGS environment variable by the toolchain - setup step. Set to the empty string to leave RUSTFLAGS unset so an - inherited value or the project's Cargo configuration + setup step. Set to the empty string to leave RUSTFLAGS unset, so an + inherited value or the project's Cargo configuration (build.rustflags in .cargo/config.toml) applies. A pre-existing RUSTFLAGS environment variable always takes precedence over this input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/setup-rust/action.yml around lines 41 - 50, Update the rustflags description in the action metadata to insert a comma before “so” in the phrase describing leaving RUSTFLAGS unset, without changing the documented behavior or other text..github/actions/rust-build-release/tests/test_manifest_input_step.py (1)
55-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd explicit
check=False— still outstanding.Ruff PLW1510 fires because
subprocess.runomitscheck; the test manually assertsreturncode, so make the intent explicit. Flagged in a prior review and still present.🔧 Fix
result = subprocess.run( # noqa: S603,TID251 - exercise the bash fragment. [bash, "-c", _export_rustflags_run_script()], cwd=tmp_path, env=env, capture_output=True, text=True, timeout=30, + check=False, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/rust-build-release/tests/test_manifest_input_step.py around lines 55 - 61, Update the subprocess.run invocation in the test around _export_rustflags_run_script to pass check=False explicitly, preserving the existing manual returncode assertions and all other subprocess options.Source: Linters/SAST tools
.github/actions/rust-build-release/action.yml (1)
71-105: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
[[ -v RUSTFLAGS ]]still breaks on Bash 3.2 — unresolved from prior review.Line 81 still uses the Bash-4.2-only
-vunary test. macOS ships Bash 3.2 by default, so this guard raisesconditional binary operator expectedthere — matching the CI failures already reported for this action. Swap to the Bash-2.02-compatible parameter-expansion check.Also worth adding while touching this: the PR's comment history mentions adding coverage for an inherited empty
RUSTFLAGSvalue; no such test is visible in the current test file — verify it landed.🐛 Proposed fix
- if [[ -v RUSTFLAGS ]]; then + if [[ -n ${RUSTFLAGS+x} ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/rust-build-release/action.yml around lines 71 - 105, Replace the Bash-4.2-only [[ -v RUSTFLAGS ]] guard in the “Export caller RUSTFLAGS” step with a Bash-2.02-compatible parameter-expansion presence check that distinguishes an inherited empty value from an unset variable and preserves the existing early-exit behavior. Also verify the action’s tests include coverage for inherited empty RUSTFLAGS, adding it if absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coverage:
- Around line 52-53: Remove the committed environment-specific .coverage
artifact containing the embedded /home/leynos workspace path. Keep .coverage
uncommitted and ensure future coverage data uses stable repository-relative
paths when regenerated; do not modify scripts/typos_rollout_check.py.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 174-253: Add descriptive failure messages to every bare assertion
introduced in the RustFLAGS tests, including test_rustflags_input_declared,
test_export_rustflags_step_wiring, and the subsequent
export/delimiter/inheritance/order tests. Preserve each assertion’s existing
condition while using assert condition, "message" so failures identify the
specific manifest, script, or round-trip expectation that failed.
In @.github/actions/setup-rust/README.md:
- Line 23: Update the rustflags option description in the README table to insert
a comma before “so,” preserving the existing wording and meaning.
In @.github/actions/setup-rust/tests/test_setup_rust_manifest.py:
- Around line 380-401: Add descriptive failure messages to every bare assert in
test_rustflags_input_defaults_to_deny_warnings and
test_install_steps_forward_rustflags, including the required/default checks and
rustflags forwarding check. Preserve the existing assertions and make each
message identify the expected condition and relevant input or step_name.
---
Duplicate comments:
In @.github/actions/rust-build-release/action.yml:
- Around line 71-105: Replace the Bash-4.2-only [[ -v RUSTFLAGS ]] guard in the
“Export caller RUSTFLAGS” step with a Bash-2.02-compatible parameter-expansion
presence check that distinguishes an inherited empty value from an unset
variable and preserves the existing early-exit behavior. Also verify the
action’s tests include coverage for inherited empty RUSTFLAGS, adding it if
absent.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 55-61: Update the subprocess.run invocation in the test around
_export_rustflags_run_script to pass check=False explicitly, preserving the
existing manual returncode assertions and all other subprocess options.
In @.github/actions/setup-rust/action.yml:
- Around line 41-50: Update the rustflags description in the action metadata to
insert a comma before “so” in the phrase describing leaving RUSTFLAGS unset,
without changing the documented behavior or other text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8cd6766-c6d2-49d5-9b95-a1d79f4bebf2
📒 Files selected for processing (25)
.coverage.github/actions/linux-packages/scripts/package.py.github/actions/release-to-pypi-uv/tests/_helpers.py.github/actions/rust-build-release/CHANGELOG.md.github/actions/rust-build-release/README.md.github/actions/rust-build-release/action.yml.github/actions/rust-build-release/tests/test_manifest_input_step.py.github/actions/setup-rust/CHANGELOG.md.github/actions/setup-rust/README.md.github/actions/setup-rust/action.yml.github/actions/setup-rust/tests/test_setup_rust_manifest.py.rules/python-00.md.rules/python-context-managers.md.rules/python-exception-design-raising-handling-and-logging.md.rules/python-generators.md.rules/python-return.md.rules/python-typing.mddocs/cmd-mox-users-guide.mddocs/execplans/2-4-5-extend-architecture-enforcement-to-orchestration-code.mddocs/execplans/add-mutation-testing-workflows.mddocs/execplans/support-cranelift-codegen.mddocs/local-validation-of-github-actions-with-act-and-pytest.mddocs/python-action-scripts.mddocs/scripting-standards.mdworkflow_scripts/graphql_client.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/polythene(auto-detected)
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
♻️ Duplicate comments (3)
.github/actions/setup-rust/action.yml (1)
41-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing comma before "so" — still present from prior review.
"leave RUSTFLAGS unset so an inherited value" needs a comma before "so" to join the two clauses correctly.
✏️ Fix
Value exported as the RUSTFLAGS environment variable by the toolchain - setup step. Set to the empty string to leave RUSTFLAGS unset so an - inherited value or the project's Cargo configuration + setup step. Set to the empty string to leave RUSTFLAGS unset, so an + inherited value or the project's Cargo configuration (build.rustflags in .cargo/config.toml) applies. A pre-existing RUSTFLAGS environment variable always takes precedence over this input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/setup-rust/action.yml around lines 41 - 50, Update the rustflags description in the action metadata to insert a comma before “so” in the phrase describing leaving RUSTFLAGS unset, without changing the documented behavior or other text..github/actions/rust-build-release/tests/test_manifest_input_step.py (1)
55-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd explicit
check=False— still outstanding.Ruff PLW1510 fires because
subprocess.runomitscheck; the test manually assertsreturncode, so make the intent explicit. Flagged in a prior review and still present.🔧 Fix
result = subprocess.run( # noqa: S603,TID251 - exercise the bash fragment. [bash, "-c", _export_rustflags_run_script()], cwd=tmp_path, env=env, capture_output=True, text=True, timeout=30, + check=False, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/rust-build-release/tests/test_manifest_input_step.py around lines 55 - 61, Update the subprocess.run invocation in the test around _export_rustflags_run_script to pass check=False explicitly, preserving the existing manual returncode assertions and all other subprocess options.Source: Linters/SAST tools
.github/actions/rust-build-release/action.yml (1)
71-105: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
[[ -v RUSTFLAGS ]]still breaks on Bash 3.2 — unresolved from prior review.Line 81 still uses the Bash-4.2-only
-vunary test. macOS ships Bash 3.2 by default, so this guard raisesconditional binary operator expectedthere — matching the CI failures already reported for this action. Swap to the Bash-2.02-compatible parameter-expansion check.Also worth adding while touching this: the PR's comment history mentions adding coverage for an inherited empty
RUSTFLAGSvalue; no such test is visible in the current test file — verify it landed.🐛 Proposed fix
- if [[ -v RUSTFLAGS ]]; then + if [[ -n ${RUSTFLAGS+x} ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/rust-build-release/action.yml around lines 71 - 105, Replace the Bash-4.2-only [[ -v RUSTFLAGS ]] guard in the “Export caller RUSTFLAGS” step with a Bash-2.02-compatible parameter-expansion presence check that distinguishes an inherited empty value from an unset variable and preserves the existing early-exit behavior. Also verify the action’s tests include coverage for inherited empty RUSTFLAGS, adding it if absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coverage:
- Around line 52-53: Remove the committed environment-specific .coverage
artifact containing the embedded /home/leynos workspace path. Keep .coverage
uncommitted and ensure future coverage data uses stable repository-relative
paths when regenerated; do not modify scripts/typos_rollout_check.py.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 174-253: Add descriptive failure messages to every bare assertion
introduced in the RustFLAGS tests, including test_rustflags_input_declared,
test_export_rustflags_step_wiring, and the subsequent
export/delimiter/inheritance/order tests. Preserve each assertion’s existing
condition while using assert condition, "message" so failures identify the
specific manifest, script, or round-trip expectation that failed.
In @.github/actions/setup-rust/README.md:
- Line 23: Update the rustflags option description in the README table to insert
a comma before “so,” preserving the existing wording and meaning.
In @.github/actions/setup-rust/tests/test_setup_rust_manifest.py:
- Around line 380-401: Add descriptive failure messages to every bare assert in
test_rustflags_input_defaults_to_deny_warnings and
test_install_steps_forward_rustflags, including the required/default checks and
rustflags forwarding check. Preserve the existing assertions and make each
message identify the expected condition and relevant input or step_name.
---
Duplicate comments:
In @.github/actions/rust-build-release/action.yml:
- Around line 71-105: Replace the Bash-4.2-only [[ -v RUSTFLAGS ]] guard in the
“Export caller RUSTFLAGS” step with a Bash-2.02-compatible parameter-expansion
presence check that distinguishes an inherited empty value from an unset
variable and preserves the existing early-exit behavior. Also verify the
action’s tests include coverage for inherited empty RUSTFLAGS, adding it if
absent.
In @.github/actions/rust-build-release/tests/test_manifest_input_step.py:
- Around line 55-61: Update the subprocess.run invocation in the test around
_export_rustflags_run_script to pass check=False explicitly, preserving the
existing manual returncode assertions and all other subprocess options.
In @.github/actions/setup-rust/action.yml:
- Around line 41-50: Update the rustflags description in the action metadata to
insert a comma before “so” in the phrase describing leaving RUSTFLAGS unset,
without changing the documented behavior or other text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8cd6766-c6d2-49d5-9b95-a1d79f4bebf2
📒 Files selected for processing (25)
.coverage.github/actions/linux-packages/scripts/package.py.github/actions/release-to-pypi-uv/tests/_helpers.py.github/actions/rust-build-release/CHANGELOG.md.github/actions/rust-build-release/README.md.github/actions/rust-build-release/action.yml.github/actions/rust-build-release/tests/test_manifest_input_step.py.github/actions/setup-rust/CHANGELOG.md.github/actions/setup-rust/README.md.github/actions/setup-rust/action.yml.github/actions/setup-rust/tests/test_setup_rust_manifest.py.rules/python-00.md.rules/python-context-managers.md.rules/python-exception-design-raising-handling-and-logging.md.rules/python-generators.md.rules/python-return.md.rules/python-typing.mddocs/cmd-mox-users-guide.mddocs/execplans/2-4-5-extend-architecture-enforcement-to-orchestration-code.mddocs/execplans/add-mutation-testing-workflows.mddocs/execplans/support-cranelift-codegen.mddocs/local-validation-of-github-actions-with-act-and-pytest.mddocs/python-action-scripts.mddocs/scripting-standards.mdworkflow_scripts/graphql_client.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/polythene(auto-detected)
🛑 Comments failed to post (1)
.coverage (1)
52-53: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the environment-specific coverage path.
The database embeds
/home/leynos/.../worktrees/.../scripts/typos_rollout_check.py, which leaks local workspace metadata and prevents reliable path matching in other checkouts. Keep.coverageuncommitted, or regenerate it with stable repository-relative paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coverage around lines 52 - 53, Remove the committed environment-specific .coverage artifact containing the embedded /home/leynos workspace path. Keep .coverage uncommitted and ensure future coverage data uses stable repository-relative paths when regenerated; do not modify scripts/typos_rollout_check.py.
lodyai
Bot
force-pushed
the
add-rustflags-passthrough-inputs
branch
from
July 31, 2026 00:11
42e70a8 to
4b6120a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
`actions-rust-lang/setup-rust-toolchain` exports `RUSTFLAGS="-D warnings"` into the job environment whenever the variable is unset. Because an ambient `RUSTFLAGS` overrides Cargo's `build.rustflags` configuration, consumers whose builds require specific flags in `.cargo/config.toml` (for example netsuke's `-Zpolonius=next`, see leynos/netsuke#465) find them silently stripped by the setup step. - `setup-rust` gains a `rustflags` input forwarded to all three nested `setup-rust-toolchain` invocations. The default preserves the historical `-D warnings`; the empty string leaves `RUSTFLAGS` unset so the project's Cargo configuration applies. - `rust-build-release` gains a `rustflags` input exported (via a GITHUB_ENV heredoc, no template expansion in the script) before its internally pinned setup-rust step, whose nested toolchain setup only applies its default when `RUSTFLAGS` is unset. A pre-existing `RUSTFLAGS` still wins. This avoids bumping the internal setup-rust-v1 pin. Manifest tests cover the new inputs, the forwarding, the export step's env indirection, and its ordering before toolchain setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ruff 0.16 formats Python code blocks embedded in Markdown, so CI's unpinned `uv tool run ruff format --check` now covers the `.rules/` and `docs/` prose. Apply that formatting and move `None` to the end of the `JsonValue` union so the newly enforced RUF036 passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_helpers.py` derived the action's `scripts/` directory from `GITHUB_ACTION_PATH`, but that variable names whichever action is currently executing. The Makefile exports it as the repository root, so under `make test` the helper resolved `SCRIPTS_DIR` to the repository's top-level `scripts/` and every module load failed with `FileNotFoundError`. In CI the variable points inside `.github/actions`, where the root conftest re-pins it, which masked the breakage. Derive both paths from the helper's own location instead, and declare `OctalInt._octal_width` so `ty` can resolve the attribute assigned in `__new__`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A caller-supplied `rustflags` value containing a line equal to the fixed `__RBR_RUSTFLAGS_EOF__` marker closed the environment-file block early, leaving the rest of the value to be parsed as further environment-file commands. That could fail the action with an invalid format or set unintended variables for the setup and build steps that follow. Derive the delimiter from 16 random bytes and confirm the value does not contain it before writing, failing the step if no free delimiter can be found. Cover the fragment with tests that execute it against an ordinary value, a value carrying the old marker, and an inherited `RUSTFLAGS`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_parse_env_file` carried both the line-dispatch loop and the heredoc collection loop, which CodeScene flagged as a Complex Method (cc = 10). Move the inner loop and its unterminated-heredoc check into `_parse_heredoc_value`, which returns the body with the index past the closing delimiter. Behaviour and the assertion messages are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`.coverage` is a coverage.py SQLite database that `make spelling` rewrites on every run, and it embeds the absolute path of whichever machine produced it. Nothing reads the committed copy: the ratchet uses `.coverage-baseline`, and CI uploads `coverage.xml` from the generate-coverage action. Untrack it and add it to the ignore list so local gate runs stop dirtying the working tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`[[ -v NAME ]]` needs bash 4.2, but macOS runners ship bash 3.2 as /bin/bash,
so the export step would fail there for any caller that sets `rustflags`.
Use `[[ -n "${RUSTFLAGS+x}" ]]`, which is portable to bash 3.2 and keeps the
existing semantics: an inherited empty value still counts as set.
Cover that distinction with a test for an inherited empty `RUSTFLAGS`; the
`${VAR:-}` idiom used elsewhere in the repository would have overwritten it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rustflags assertions compared parsed manifest fragments without saying what the expectation meant, so a failure reported only the operator. Give each one a message naming the manifest, script, or round-trip expectation, and pass `check=False` explicitly when running the shell fragment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add the comma before "so" in the result clause of the setup-rust rustflags description, in the manifest, the README table, and the changelog entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test_module_globs_are_deduplicated_module_patterns` asserted that ".py" never appears in a glob, to catch unstripped suffixes. That is too broad: a directory segment may itself be named "py", so "src/a/py.py" correctly yields "a.py.*", and Hypothesis eventually generated exactly that. Compare against the expected module globs instead, which checks suffix stripping precisely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
add-rustflags-passthrough-inputs
branch
from
July 31, 2026 00:19
4b6120a to
8d3a70d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch adds a
rustflagspassthrough to thesetup-rustandrust-build-releasecomposite actions so consumers can stop the nestedactions-rust-lang/setup-rust-toolchainstep from silently stripping flags their builds require.That nested step exports
RUSTFLAGS="-D warnings"into the job environment whenever the variable is unset, and an ambientRUSTFLAGSoverrides Cargo'sbuild.rustflagsconfiguration. Projects whose source tree requires specific flags in.cargo/config.toml— netsuke's-Zpolonius=nextborrow-checker flag being the motivating case (leynos/netsuke#465, leynos/netsuke#472) — therefore fail to compile in any step after setup unless every recipe re-states the flags.setup-rust/action.ymlgains arustflagsinput forwarded to all three nestedsetup-rust-toolchaininvocations. The default preserves the historical-D warnings; the empty string leavesRUSTFLAGSunset so the project's Cargo configuration applies.rust-build-release/action.ymlgains arustflagsinput exported ahead of its internally pinnedsetup-ruststep, through aGITHUB_ENVheredoc with env-var indirection (no template expansion inside the script). A pre-existingRUSTFLAGSstill takes precedence. Implementing the export locally avoids bumping the internalsetup-rust-v1pin inside this pull request.Review walkthrough
setup-rust/action.ymlfor the input declaration and the three forwarding sites.rust-build-release/action.yml, including its inherited-value guard and ordering before "Setup Rust toolchain".test_setup_rust_manifest.pyandtest_manifest_input_step.py, plus the README and changelog updates for both actions.Validation
uv run --with pytest --with pyyaml pytest .github/actions/setup-rust/tests/ .github/actions/rust-build-release/tests/test_manifest_input_step.py -q: all pass (30 + 7).uv tool run ruff checkandruff format --checkon the changed test files: clean.markdownlint-cli2on the changed READMEs and changelogs: 0 errors.Notes
Behaviour is unchanged for existing consumers:
setup-rustdefaults to the historical-D warnings, andrust-build-releasedefaults to not touching the environment. Netsuke currently works around the issue with job-levelRUSTFLAGSenv blocks; once this lands and the action tags move, it can adopt the inputs instead.References
Summary by Sourcery
Add configurable RUSTFLAGS passthrough to the shared Rust setup and release build composite actions to avoid clobbering project-specific compiler flags while preserving default behaviour for existing consumers.
New Features:
Enhancements:
Tests: