-
Notifications
You must be signed in to change notification settings - Fork 2.3k
faster, cheaper (pick two): improve CI workflow and switch to free github runner #5702
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.
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
Conversation
1715ab2 to
aa9b866
Compare
091b3ad to
1f1bb64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes the CI infrastructure by migrating from a custom runner to GitHub's free ubuntu-latest runners, implementing better caching strategies, and splitting jobs for improved parallelization. The changes enable the workflow to run on fork PRs without approval while reducing infrastructure costs.
Key Changes:
- Replaced custom "goose" runner with ubuntu-latest for Rust jobs
- Migrated from manual cache configuration to Swatinem/rust-cache@v2 action across all workflows
- Split CI jobs into separate format, build-test, lint, and openapi-schema-check jobs for better parallelization
- Improved test isolation by using thread-specific temporary directories in test mode
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Splits monolithic job into separate format, build-test, lint, and openapi-schema-check jobs; switches to ubuntu-latest runner and rust-cache |
.github/workflows/pr-smoke-test.yml |
Replaces manual cache configuration with rust-cache action |
.github/workflows/bundle-desktop.yml |
Consolidates three separate cargo cache steps into single rust-cache action |
.github/workflows/bundle-desktop-windows.yml |
Simplifies Windows cross-compilation cache to use rust-cache |
.github/workflows/bundle-desktop-linux.yml |
Updates Linux cross-compilation to use rust-cache with custom key |
.github/workflows/bundle-desktop-intel.yml |
Consolidates Intel macOS cache steps into single rust-cache action |
.github/workflows/build-cli.yml |
Updates CLI build caching for both cross-platform and Windows builds |
crates/goose/src/session/session_manager.rs |
Adds thread-specific test directories to prevent test isolation issues; moves Paths import to non-test code only |
scripts/clippy-baseline.sh |
Removes trailing whitespace; adds tee to clippy output for better visibility |
Co-authored-by: Copilot <[email protected]>
.github/workflows/build-cli.yml
Outdated
| - name: Cache Cargo registry (Windows) | ||
| if: matrix.use-docker | ||
| uses: actions/cache@2f8e54208210a422b2efd51efaa6bd6d7ca8920f | ||
| # Cache Cargo artifacts for cross-platform builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well drop the comment (or change it into the name below)
| - name: Lint | ||
| run: | | ||
| source ./bin/activate-hermit | ||
| hermit uninstall rustup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could do with a comment
The swatinem/rust-cache github action is the go-to for caching rust projects because it does a good job of filtering out all of the incremental artifacts created from compiling your own project code, and keeping the dependencies. But there's one problem with our setup: it distinguishes between first and third party code by looking at the path on disk, and if the artifact source path prefix-matches your project directory, it marks it as (non-cached) project code. When we use hermit, the CARGO_HOME is within the project directory, whereas in a typical setup it would be at ~/.cargo.
Here, since this step needs both rust (for the openapi code gen) plus npm and just, it made sense to just remove the rust bit from hermit while keeping the other parts.
This problem goes away if we decide to stop using hermit for the rust toolchain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| let temp_base = std::env::temp_dir().join("goose_test"); | ||
| let session_dir = temp_base | ||
| .join(format!("thread_{:?}", thread_id)) | ||
| .join(SESSIONS_FOLDER); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test directories in temp aren't cleaned up automatically. Consider using a test setup/teardown pattern or document that CI should clean /tmp between runs.
9b41cce to
e715df3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| let session_dir = temp_base | ||
| .join(format!("thread_{:?}", thread_id)) | ||
| .join(SESSIONS_FOLDER); | ||
| eprintln!("session dir: {session_dir:?}"); |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug print statement left in production code. Remove or convert to proper tracing/logging if needed for debugging.
| eprintln!("session dir: {session_dir:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
| let thread_id = std::thread::current().id(); | ||
| let temp_base = std::env::temp_dir().join("goose_test"); | ||
| let session_dir = temp_base | ||
| .join(format!("thread_{:?}", thread_id)) |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ThreadId with {:?} formatting creates non-deterministic paths across test runs (e.g., ThreadId(123) vs ThreadId(456)), which breaks cache persistence. For tokio async tests, this also won't provide proper isolation since multiple tests may run on the same thread from the tokio runtime thread pool.
| let thread_id = std::thread::current().id(); | |
| let temp_base = std::env::temp_dir().join("goose_test"); | |
| let session_dir = temp_base | |
| .join(format!("thread_{:?}", thread_id)) | |
| let process_id = std::process::id(); | |
| let temp_base = std::env::temp_dir().join("goose_test"); | |
| let session_dir = temp_base | |
| .join(format!("test_session_{}", process_id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
| local temp_json=$(mktemp) | ||
| cargo clippy --jobs 2 --message-format=json -- $clippy_flags > "$temp_json" | ||
| cargo clippy --jobs 2 --message-format=json -- $clippy_flags | tee "$temp_json" |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using tee here will output all clippy JSON diagnostics to stdout, creating noise in CI logs. The original > redirect was silent. Consider reverting to > \"$temp_json\" unless the verbose JSON output is intentionally needed for debugging.
| cargo clippy --jobs 2 --message-format=json -- $clippy_flags | tee "$temp_json" | |
| cargo clippy --jobs 2 --message-format=json -- $clippy_flags > "$temp_json" |
* main: (65 commits) Fix: Recipes respect the quiet flag (#5743) docs: update cli commands (#5744) Run smoke tests on a free runner (#5745) faster, cheaper (pick two): improve CI workflow and switch to free github runner (#5702) Compaction resiliency improvements (#5618) docs: ask goose button (#5730) Update prompt injection detection metrics (due to errors exporting to datadog) (#5692) Spence/icon2 tokyo drift (#5728) docs: logs rotation and misc updates (#5727) docs: automatic ollama model detection (#5725) Fix context progress bar not resetting after /clear command (#5652) docs: removing double announcements (#5714) docs: mcp sampling support (#5708) hackathon banner (#5710) Fix documentation-only change detection for push events (#5712) Added transaction commits to multi sql functions in session_manager (#5693) fix: improve and simplify tool call chain rendering (#5704) Fix: Always show autocompact threshold ui (#5701) chore: Update governance to include Discord (#5690) Ollama improvements (#5609) ...
* main: scan recipe for security when saving recipe (#5747) feat: trying grok for live test (#5732) Platform Extension MOIM (Minus One Info Message) (#5027) docs: remove hackathon banner (#5756) Fix: Recipes respect the quiet flag (#5743) docs: update cli commands (#5744) Run smoke tests on a free runner (#5745) faster, cheaper (pick two): improve CI workflow and switch to free github runner (#5702) Compaction resiliency improvements (#5618) docs: ask goose button (#5730) Update prompt injection detection metrics (due to errors exporting to datadog) (#5692) Spence/icon2 tokyo drift (#5728) docs: logs rotation and misc updates (#5727) docs: automatic ollama model detection (#5725) Fix context progress bar not resetting after /clear command (#5652) docs: removing double announcements (#5714) docs: mcp sampling support (#5708)
…thub runner (block#5702) Co-authored-by: Douwe Osinga <[email protected]> Co-authored-by: Copilot <[email protected]>
…thub runner (block#5702) Co-authored-by: Douwe Osinga <[email protected]> Co-authored-by: Copilot <[email protected]>
…thub runner (block#5702) Co-authored-by: Douwe Osinga <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Blair Allan <[email protected]>
This:
with the end goal of opening up our workflows to run on fork PRs without approval