feat(ci): verifies that the committed Go FFI binding - #7478
feat(ci): verifies that the committed Go FFI binding#7478EclesioMeloJunior wants to merge 3 commits into
Conversation
e221734 to
702ce1c
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR adds conditional Go FFI regeneration, a ChangesGo FFI freshness validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CI as Go lint workflow
participant Mise as mise lint:go-ffi
participant Cargo as cargo check
participant BuildScript as interop-tests/build.rs
participant Binding as ffi_gen.go
CI->>Mise: run freshness check
Mise->>Cargo: regenerate bindings
Cargo->>BuildScript: set FOREST_REGENERATE_GO_FFI
BuildScript->>Binding: write generated binding
Mise->>Binding: detect file differences
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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/go-lint.yml:
- Line 62: Update the actions/checkout@v7 step in the workflow to set
persist-credentials to false before the mise lint:go-ffi step, while preserving
the job’s ability to inspect local git diff output.
In `@CHANGELOG.md`:
- Line 42: Update the CHANGELOG entry for issue `#7467` so the mention of “mise
run lint:go-ffi” forms a grammatically complete clause, splitting the sentence
or replacing “included” with appropriate wording while preserving the described
CI behavior.
🪄 Autofix
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
Review profile: CHILL
Plan: Pro
Run ID: 8ee1180e-ff1e-4091-bcad-c981915108ac
📒 Files selected for processing (4)
.github/workflows/go-lint.ymlCHANGELOG.mdinterop-tests/build.rsmise.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
| - uses: jdx/mise-action@v4 | ||
| - name: Apt Dependencies | ||
| run: sudo apt-get install -y libclang-dev # required by rust2go's bindgen | ||
| - run: mise lint:go-ffi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
sed -n '1,110p' .github/workflows/go-lint.yml
printf '%s\n' '--- relevant mise tasks and scripts ---'
rg -n -C 5 'lint:go-ffi|go-ffi|cargo|go generate|git diff' mise.toml .mise.toml mise.yml .mise.yml .github scripts Makefile 2>/dev/null || true
printf '%s\n' '--- checkout configuration across workflows ---'
rg -n -C 4 'actions/checkout|persist-credentials' .github/workflows 2>/dev/null || trueRepository: ChainSafe/forest
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FFI package manifest and build scripts ---'
rg -n -C 8 'forest-interop-tests|build\s*=|FOREST_REGENERATE_GO_FFI|go_ffi|ffi_gen' Cargo.toml Cargo.lock interop-tests f3-sidecar 2>/dev/null | head -n 240
printf '%s\n' '--- exact checkout settings in the reviewed workflow ---'
nl -ba .github/workflows/go-lint.yml | sed -n '45,70p'
printf '%s\n' '--- workflow token and repository context usage ---'
rg -n -C 3 'GITHUB_TOKEN|github.token|secrets\.[A-Za-z0-9_]+|permissions:' .github/workflows/go-lint.yml .github/actions .github 2>/dev/null | head -n 160Repository: ChainSafe/forest
Length of output: 14883
🌐 Web query:
actions/checkout persist-credentials false local git config http extraheader GITHUB_TOKEN documentation
💡 Result:
The persist-credentials input in actions/checkout controls whether the action automatically configures the local Git repository with credentials (typically the GITHUB_TOKEN) to enable subsequent authenticated Git operations [1][2][3]. When persist-credentials is set to true (the default), the action configures Git to use an authorization header for requests to GitHub [1][3]. The specific mechanism has evolved: In versions v5 and earlier, the action directly injected an http.extraheader containing the authorization token into the local.git/config file [4][5]. In version v6 and later, the action enhances security by writing the credentials to a separate file within the runner's temporary directory ($RUNNER_TEMP) and adding an includeIf.gitdir configuration to.git/config that points to that file [4][6]. Setting persist-credentials: false opts out of this behavior [1][2]. This is often recommended when: You do not need to perform subsequent Git operations that require the default token [4]. You want to prevent the GITHUB_TOKEN from being persisted in any configuration file, which can be a security best practice to reduce the attack surface for credential leakage [7][4][8]. You are using custom authentication (e.g., a personal access token or SSH key) and want to ensure the default GITHUB_TOKEN does not override or interfere with your configuration [9][5][6]. To use this option in your workflow, add it to the with block of your checkout step: - name: Checkout repository uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout?tab=readme-ov-file
- 2: https://github.com/actions/checkout
- 3: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 4: Set
persist-credentials: falseon checkout steps cli/go-gh#225 - 5:
gh auth setup-gitdoes not override the git user following anactions/checkoutinvocation cli/cli#10905 - 6: ci(sync-repos): fix actions/checkout v6 credential persistence axsaucedo/kaos#158
- 7: Always emit
persist-credentials: falsewith actions/checkout github/gh-aw#2641 - 8: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312 - 9: Header Token overrides user token actions/checkout#181
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
workflow = Path(".github/workflows/go-lint.yml").read_text()
mise = Path("mise.toml").read_text()
build = Path("interop-tests/build.rs").read_text()
manifest = Path("interop-tests/Cargo.toml").read_text()
checks = {
"lint workflow has checkout before FFI task": bool(re.search(
r"actions/checkout@v7[\s\S]*?mise lint:go-ffi", workflow)),
"FFI task runs cargo check": "FOREST_REGENERATE_GO_FFI=1 cargo check -p forest-interop-tests" in mise,
"FFI task checks generated file with git diff": "git diff --exit-code -- interop-tests/src/tests/go_app/ffi_gen.go" in mise,
"interop-tests has a Cargo build script": 'build = "build.rs"' in manifest,
"build script enables regeneration from environment": 'FOREST_REGENERATE_GO_FFI' in build,
"build script writes generated Go file": 'src/tests/go_app/ffi_gen.go' in build,
}
for name, result in checks.items():
print(f"{name}: {result}")
if not all(checks.values()):
raise SystemExit("invariant failed")
PYRepository: ChainSafe/forest
Length of output: 450
Disable persisted checkout credentials before mise lint:go-ffi.
cargo check executes the forest-interop-tests build script and regenerates Go bindings. A build script or dependency could read the persisted GITHUB_TOKEN. Set persist-credentials: false on the actions/checkout@v7 step; the job only needs local git diff.
🤖 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/go-lint.yml at line 62, Update the actions/checkout@v7
step in the workflow to set persist-credentials to false before the mise
lint:go-ffi step, while preserving the job’s ability to inspect local git diff
output.
Source: Linters/SAST tools
|
|
||
| ### Changed | ||
|
|
||
| - [#7467](https://github.com/ChainSafe/forest/issues/7467): CI now verifies that the committed Go FFI binding `interop-tests/src/tests/go_app/ffi_gen.go` stays in sync with `go_ffi.rs` through the `FOREST_REGENERATE_GO_FFI` environment variable, included `mise run lint:go-ffi`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the changelog wording.
The phrase included mise run lint:go-ffi is not grammatical. Split the sentence or replace included with a complete clause.
Proposed wording
-- [`#7467`](https://github.com/ChainSafe/forest/issues/7467): CI now verifies that the committed Go FFI binding `interop-tests/src/tests/go_app/ffi_gen.go` stays in sync with `go_ffi.rs` through the `FOREST_REGENERATE_GO_FFI` environment variable, included `mise run lint:go-ffi`.
+- [`#7467`](https://github.com/ChainSafe/forest/issues/7467): CI now verifies that the committed Go FFI binding `interop-tests/src/tests/go_app/ffi_gen.go` stays in sync with `go_ffi.rs` through the `FOREST_REGENERATE_GO_FFI` environment variable. Run `mise run lint:go-ffi` to check freshness.📝 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.
| - [#7467](https://github.com/ChainSafe/forest/issues/7467): CI now verifies that the committed Go FFI binding `interop-tests/src/tests/go_app/ffi_gen.go` stays in sync with `go_ffi.rs` through the `FOREST_REGENERATE_GO_FFI` environment variable, included `mise run lint:go-ffi`. | |
| - [`#7467`](https://github.com/ChainSafe/forest/issues/7467): CI now verifies that the committed Go FFI binding `interop-tests/src/tests/go_app/ffi_gen.go` stays in sync with `go_ffi.rs` through the `FOREST_REGENERATE_GO_FFI` environment variable. Run `mise run lint:go-ffi` to check freshness. |
🤖 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 `@CHANGELOG.md` at line 42, Update the CHANGELOG entry for issue `#7467` so the
mention of “mise run lint:go-ffi” forms a grammatically complete clause,
splitting the sentence or replacing “included” with appropriate wording while
preserving the described CI behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 11 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
|
||
| // the generated Go file has been committed to the git repository | ||
| // set the var to regenerate the file, CI sets this var to verify freshness. | ||
| if std::env::var_os("FOREST_REGENERATE_GO_FFI").is_some() { |
There was a problem hiding this comment.
new flags should be added to env variable reference in docs.
There was a problem hiding this comment.
There's also the root repo's build.rs that has similar logic - what about it?
2 participants
Summary of changes
Changes introduced in this pull request:
interop-testsgenerating the go-ffi only ifFOREST_REGENERATE_GO_FFIis presentmise run lint:go-ffithat setsFOREST_REGENERATE_GO_FFIand generate the Go FFI binding then perform a diff checkmise run lint:go-ffiin thego-lint.ymlworkflowReference issue to close (if applicable)
Closes #7467
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
Chores