Pass through env variables#53
Conversation
📝 WalkthroughWalkthroughThis PR standardizes the Rust build toolchain across CI, release, and local environments, enhances the continue command to properly forward editor environment variables to git rebase, adds test infrastructure for consistent git command setup, and includes a minor loop refactoring in stack section removal. ChangesRust Toolchain and Git Environment Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/release.yml:
- Around line 40-43: The setup step using
actions-rust-lang/setup-rust-toolchain@v1 currently enables the default cache;
to force release isolation disable caching by adding the cache input set to
false for that action (update the step that references uses:
actions-rust-lang/setup-rust-toolchain@v1 and its with: block), so the workflow
will not restore ~/.cargo or target/ during tag-triggered releases and will
ensure a clean build environment.
In `@tests/move_tests.rs`:
- Around line 299-360: The test currently covers only the EDITOR branch in
test_move_continue_forwards_editor_env_to_git; add two additional assertions (or
separate tests) to cover the VISUAL fallback and the "do not override existing
GIT_EDITOR" behavior by invoking the same flow that triggers continue_cmd: (1)
run kin_cmd().arg("continue") with env("VISUAL","true") and assert success and
the same rebase/move result as the EDITOR case, and (2) set
env("GIT_EDITOR","preexisting") plus env("EDITOR","true") (or VISUAL) before
running kin_cmd().arg("continue") and assert the command succeeds and that
GIT_EDITOR remains "preexisting" (or at least that the behavior uses the
existing GIT_EDITOR rather than replacing it). Locate changes around the
existing test function name test_move_continue_forwards_editor_env_to_git,
kin_cmd() invocations, and the continue_cmd logic to implement these assertions.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45ea330c-c961-4f75-9e5f-588b6a76b24e
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/release.ymlrust-toolchain.tomlsrc/commands/continue_cmd.rssrc/commands/pr.rstests/common/mod.rstests/move_tests.rstests/reorder_tests.rs
| uses: actions-rust-lang/setup-rust-toolchain@v1 | ||
| with: | ||
| targets: ${{ matrix.target }} | ||
| target: ${{ matrix.target }} | ||
| rustflags: "" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
actions-rust-lang/setup-rust-toolchain default cache input behavior
💡 Result:
The actions-rust-lang/setup-rust-toolchain GitHub Action provides built-in support for caching Rust dependencies and toolchain artifacts [1][2]. Its default behavior regarding caching is controlled by the cache input, which is enabled by default [1][2]. Key details regarding the cache input behavior include: Default State: The cache input defaults to true, meaning the action will automatically attempt to configure Rust caching when used in a workflow [1][2]. Underlying Mechanism: The action leverages the Swatinem/rust-cache action to perform the actual caching operations [1][2]. Additional Caching Controls: - cache-on-failure: Defaults to true, determining whether the cache is saved even if the job fails [1][2]. - cache-bin: Defaults to true, which propagates the cache-bin setting to the underlying Swatinem/rust-cache [1][2]. - cache-provider: Defaults to 'github', specifying the caching service used [1][2]. - cache-all-crates and cache-workspace-crates: Both default to false [1][2]. Because this action propagates many of its caching inputs to Swatinem/rust-cache, users can further refine behavior using inputs like cache-directories, cache-key, and cache-shared-key if specific customization is required for complex monorepo or workspace setups [1][2].
Citations:
- 1: https://github.com/actions-rust-lang/setup-rust-toolchain
- 2: https://github.com/actions-rust-lang/setup-rust-toolchain/blob/main/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/release.yml"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Lines 1-120 =="
nl -ba "$FILE" | sed -n '1,120p'
echo
echo "== Lines around 30-80 =="
nl -ba "$FILE" | sed -n '30,90p'Repository: Pajn/Kindra
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/release.yml"
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Matches for triggers/releases =="
rg -n --hidden --no-messages -S "on:|tags:|github\.com|release|upload|action-gh-release|upload-release-asset|softprops" "$FILE" || true
echo
echo "== Lines 1-80 =="
cat -n "$FILE" | sed -n '1,80p'
echo
echo "== Lines 80-110 =="
cat -n "$FILE" | sed -n '80,110p'Repository: Pajn/Kindra
Length of output: 4023
🌐 Web query:
Swatinem/rust-cache default cached directories target directory and cache key contents
💡 Result:
The Swatinem/rust-cache GitHub Action is designed to speed up Rust builds by caching critical Cargo directories and build artifacts [1][2]. Default Cached Directories By default, the action caches the following locations [1][3]: - ~/.cargo: This includes the cargo registry (downloaded crates), git dependencies, installed binaries, and the cargo cache [1][3][2]. -./target: This directory contains compiled build artifacts for your project [1][3][2]. The./target directory is cached by default, but this can be toggled using the cache-targets input [1][4]. Furthermore, you can define specific Cargo workspace mappings using the workspaces input, which follows the format $workspace -> $target (defaulting to. -> target if not specified) [1][4]. Cache Key Contents The cache key is automatically generated to ensure builds are only restored when the environment is compatible [5]. By default, this generated key includes [5]: - A hash of all Cargo.lock and Cargo.toml files found in the repository [5]. - A hash of all rust-toolchain or rust-toolchain.toml files in the root directory [5]. - A hash of all.cargo/config.toml files in the root directory [5]. - Values from specific compiler-related environment variables (e.g., RUSTFLAGS) [5]. - Optionally, the job ID, if add-job-id-key is set to true (default is true) [5]. You can control these behaviors via parameters like add-rust-environment-hash-key and prefix-key to customize how or when the cache is invalidated or reused [5].
Citations:
- 1: https://github.com/Swatinem/rust-cache/blob/11da8522/README.md
- 2: https://cicube.io/workflow-hub/Swatinem-rust-cache/
- 3: https://deepwiki.com/Swatinem/rust-cache/3.3-caching-strategy
- 4: https://github.com/swatinem/rust-cache
- 5: https://github.com/Swatinem/rust-cache/tree/refs/tags/v2.9.1
🌐 Web query:
rust-cache caches target correctness cargo rebuild fingerprints stale artifacts
💡 Result:
Cargo’s rebuild correctness (i.e., whether stale artifacts can be reused) is driven by two things: (1) unit fingerprints that encode “what should be rebuilt”, and (2) filesystem mtime checks against per-unit “dep-info” files under target/.fingerprint. 1) What Cargo stores to decide if an artifact is up to date - Cargo computes a Fingerprint (a hash) for each compilation “Unit” and saves it on disk in a.fingerprint directory; if the fingerprint is missing, or if any fingerprint fields change, the Unit is considered dirty and will rebuild [1][2][3]. - The fingerprint includes information about the unit’s dependencies so that if an upstream crate changes, downstream units become dirty and will rebuild as well [1][3]. 2) How Cargo detects “stale” vs “fresh” on the filesystem - Cargo’s documentation explains the core mechanism: it compares the mtime of a Unit’s source/input files against the mtime of the unit’s dep-info file in the fingerprint directory; if input files are missing or newer than the dep-info, the unit is dirty [2]. - The fingerprint implementation also records whether produced output files are present and whether dependencies’ outputs are newer; if any dependency is newer, the unit is stale, and if output files are missing, the unit is stale [2]. - The source code comments describe how mtime equality is treated conservatively: Cargo considers equal mtimes “stale” to avoid missing changes on filesystems with low timestamp precision (e.g., 1s). It also discusses edge cases where kernel time caching can cause very small updates to not be detected reliably, and the equality check is intended as a conservative approximation to mitigate that [4]. 3) “Target correctness”: what this means for stale artifacts In practice, “stale artifacts in target/” are prevented because Cargo: - Won’t reuse a compiled unit if it has no prior fingerprint state or if relevant fingerprint inputs changed (source, flags, compiler version, etc.) [1][3]. - Won’t reuse it if filesystem checks determine inputs/dependencies are newer than the recorded dep-info anchor, or if expected outputs are missing [2]. - Propagates dirtiness through dependency fingerprints so that a rebuild upstream forces downstream recompilation [1][3]. 4) Rebuild fingerprints vs. “content-based” fingerprints - Cargo’s current approach is mtime-based for filesystem freshness checks (as described above) rather than always hashing file contents. - There is historical discussion/issue material about possibly switching to content-based fingerprinting (digesting files) instead of mtime, but it’s noted that mtime was chosen partly to avoid race conditions around file changes during the build [5]. If you want, tell me which workflow you’re worried about (e.g., switching branches, network filesystems/mtime precision, using incremental compilation, CI cache restore, or running parallel builds), and I can map the risk to the specific fingerprint/mtime checks above and suggest mitigation commands/config.
Citations:
- 1: https://doc.rust-lang.org/nightly/nightly-rustc/cargo/core/compiler/fingerprint/struct.Fingerprint.html
- 2: https://doc.rust-lang.org/beta/nightly-rustc/cargo/core/compiler/fingerprint/index.html
- 3: https://docs.rs/cargo/latest/src/cargo/core/compiler/fingerprint/mod.rs.html
- 4: https://github.com/rust-lang/cargo/blob/502f0ae72ba3b64f96dab8a00b33e93240e68de4/src/cargo/core/compiler/fingerprint.rs
- 5: (Option to) Fingerprint by file contents instead of mtime rust-lang/cargo#6529
Downgrade cache-poisoning concern to defense-in-depth; consider disabling cache for release isolation.
actions-rust-lang/setup-rust-toolchain@v1 defaults to cache: true (via Swatinem/rust-cache), which restores ~/.cargo and target. Since this workflow builds with cargo build --locked, Cargo’s fingerprint/dep-info + mtime checks should prevent stale target/ from being reused for different sources/inputs, so this is unlikely to “silently” change release artifacts. If you still want stricter isolation/repro hardening on tag-triggered releases, disable the cache here.
🔒 Proposed fix
- name: Set up Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
target: ${{ matrix.target }}
rustflags: ""
+ cache: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions-rust-lang/setup-rust-toolchain@v1 | |
| with: | |
| targets: ${{ matrix.target }} | |
| target: ${{ matrix.target }} | |
| rustflags: "" | |
| uses: actions-rust-lang/setup-rust-toolchain@v1 | |
| with: | |
| target: ${{ matrix.target }} | |
| rustflags: "" | |
| cache: false |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 40-40: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 40-40: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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/workflows/release.yml around lines 40 - 43, The setup step using
actions-rust-lang/setup-rust-toolchain@v1 currently enables the default cache;
to force release isolation disable caching by adding the cache input set to
false for that action (update the step that references uses:
actions-rust-lang/setup-rust-toolchain@v1 and its with: block), so the workflow
will not restore ~/.cargo or target/ during tag-triggered releases and will
ensure a clean build environment.
| #[test] | ||
| fn test_move_continue_forwards_editor_env_to_git() { | ||
| let dir = tempdir().unwrap(); | ||
| let repo = repo_init(dir.path()); | ||
|
|
||
| let base_id = make_commit(&repo, "refs/heads/main", "file.txt", "base", "initial", &[]); | ||
| let base = repo.find_commit(base_id).unwrap(); | ||
|
|
||
| let target_id = make_commit( | ||
| &repo, | ||
| "refs/heads/target", | ||
| "file.txt", | ||
| "target content", | ||
| "target commit", | ||
| &[&base], | ||
| ); | ||
|
|
||
| let feature_id = make_commit( | ||
| &repo, | ||
| "refs/heads/feature", | ||
| "file.txt", | ||
| "feature content", | ||
| "feature commit", | ||
| &[&base], | ||
| ); | ||
| let feature = repo.find_commit(feature_id).unwrap(); | ||
|
|
||
| repo.set_head("refs/heads/feature").unwrap(); | ||
| repo.checkout_tree( | ||
| feature.as_object(), | ||
| Some(git2::build::CheckoutBuilder::new().force()), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| kin_cmd() | ||
| .arg("move") | ||
| .arg("--onto") | ||
| .arg("target") | ||
| .current_dir(dir.path()) | ||
| .assert() | ||
| .failure() | ||
| .stderr(predicates::str::contains("Resolve conflicts")); | ||
|
|
||
| fs::write(dir.path().join("file.txt"), "resolved content").unwrap(); | ||
| run_ok("git", &["add", "file.txt"], dir.path()); | ||
|
|
||
| kin_cmd() | ||
| .arg("continue") | ||
| .current_dir(dir.path()) | ||
| .env("EDITOR", "true") | ||
| .assert() | ||
| .success(); | ||
|
|
||
| let feature_new = repo | ||
| .find_branch("feature", git2::BranchType::Local) | ||
| .unwrap(); | ||
| let target = repo.find_commit(target_id).unwrap(); | ||
| assert!( | ||
| repo.graph_descendant_of(feature_new.get().target().unwrap(), target.id()) | ||
| .unwrap() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add integration coverage for the remaining editor-env branches.
This test validates EDITOR, but the new continue_cmd logic also includes VISUAL fallback and “don’t override existing GIT_EDITOR”. Please add cases for both branches to lock in behavior.
🤖 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 `@tests/move_tests.rs` around lines 299 - 360, The test currently covers only
the EDITOR branch in test_move_continue_forwards_editor_env_to_git; add two
additional assertions (or separate tests) to cover the VISUAL fallback and the
"do not override existing GIT_EDITOR" behavior by invoking the same flow that
triggers continue_cmd: (1) run kin_cmd().arg("continue") with
env("VISUAL","true") and assert success and the same rebase/move result as the
EDITOR case, and (2) set env("GIT_EDITOR","preexisting") plus
env("EDITOR","true") (or VISUAL) before running kin_cmd().arg("continue") and
assert the command succeeds and that GIT_EDITOR remains "preexisting" (or at
least that the behavior uses the existing GIT_EDITOR rather than replacing it).
Locate changes around the existing test function name
test_move_continue_forwards_editor_env_to_git, kin_cmd() invocations, and the
continue_cmd logic to implement these assertions.
1 participant
Stack
abort Make sure abort doesn't throw away other git state #51(merged)completions Add completion support #52(merged)Summary by CodeRabbit
Chores
Bug Fixes
Refactor