Update goose to 1.39 - #48
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR updates downstream Cargo dependencies and feature flags, adjusts TLS and ring build behavior, changes session insertion SQL, refreshes prompt snapshots, and advances changelog and RPM metadata to version 1.39.0. ChangesDependency and build configuration
Runtime SQL and prompt snapshots
Release metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
0100-Downstream-only-never-use-pre-generated-object-files.patch (1)
41-57: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winReturn
out_dir.clone()from the block.
generate_sources_and_preassemble(...)is used for its side effects; without a final expression,generated_dirbecomes(), and the later build step will not type-check.🤖 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 `@0100-Downstream-only-never-use-pre-generated-object-files.patch` around lines 41 - 57, Update the generated_dir initialization block around generate_sources_and_preassemble so it returns out_dir.clone() after the side-effecting call. Preserve the existing source-generation behavior and ensure generated_dir remains a PathBuf for the later build steps.0004-aws-lc-rs-feature-flag.patch (1)
35-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
rcgenbehind a backend feature rcgen is still compiled when neither TLS feature is enabled, anddefault-features = falseleaves it without a backend in that case. Make it conditional onrustls-tls/native-tls, or add a fallback backend for the base build.🤖 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 `@0004-aws-lc-rs-feature-flag.patch` around lines 35 - 50, Update the rcgen feature configuration in the rustls-tls and native-tls feature definitions so rcgen is not compiled without a crypto backend when neither TLS feature is enabled. Either make the rcgen dependency conditional on those TLS features or add a suitable fallback backend for the base build, while preserving the existing aws_lc_rs and ring backend selections.
🧹 Nitpick comments (1)
0003-Downgrade-pkcs8-to-0.10.2-for-native-tls-compat.patch (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pkcs8 compatibility constraint explicit.
version = "0.10.2"permits later compatible 0.10.x releases. If compatibility is specific to 0.10.2, use=0.10.2; otherwise document the tested range and rationale. Cargo treats omitted operators as caret requirements. (doc.rust-lang.org)As per path instructions, version-pinning constraints and their rationale should be documented in comments.
🤖 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 `@0003-Downgrade-pkcs8-to-0.10.2-for-native-tls-compat.patch` around lines 18 - 19, Make the pkcs8 dependency constraint explicit in the manifest: use an exact 0.10.2 requirement if compatibility is limited to that release, or retain the 0.10.x range only with a nearby comment documenting the tested range and rationale. Update the pkcs8 dependency declaration in the patch accordingly.Source: Path instructions
🤖 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 `@0001-Strip-non-Linux-deps-and-use-system-libraries.patch`:
- Around line 51-59: Restore the [profile.dev.package."*"] configuration with
debug = false in the patch, preserving the existing downstream debug-build
optimization while leaving workspace members and release behavior unchanged.
In `@changelog`:
- Around line 37-54: Remove the duplicate “Resolve bundled extensions from
discovery” entry in the changelog, keeping a single occurrence in the 1.39.0
release notes.
---
Outside diff comments:
In `@0004-aws-lc-rs-feature-flag.patch`:
- Around line 35-50: Update the rcgen feature configuration in the rustls-tls
and native-tls feature definitions so rcgen is not compiled without a crypto
backend when neither TLS feature is enabled. Either make the rcgen dependency
conditional on those TLS features or add a suitable fallback backend for the
base build, while preserving the existing aws_lc_rs and ring backend selections.
In `@0100-Downstream-only-never-use-pre-generated-object-files.patch`:
- Around line 41-57: Update the generated_dir initialization block around
generate_sources_and_preassemble so it returns out_dir.clone() after the
side-effecting call. Preserve the existing source-generation behavior and ensure
generated_dir remains a PathBuf for the later build steps.
---
Nitpick comments:
In `@0003-Downgrade-pkcs8-to-0.10.2-for-native-tls-compat.patch`:
- Around line 18-19: Make the pkcs8 dependency constraint explicit in the
manifest: use an exact 0.10.2 requirement if compatibility is limited to that
release, or retain the 0.10.x range only with a nearby comment documenting the
tested range and rationale. Update the pkcs8 dependency declaration in the patch
accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71799489-4aa8-4dd5-8dcb-7b0b587a2df6
📒 Files selected for processing (9)
0001-Strip-non-Linux-deps-and-use-system-libraries.patch0002-Set-downstream-feature-flags.patch0003-Downgrade-pkcs8-to-0.10.2-for-native-tls-compat.patch0004-aws-lc-rs-feature-flag.patch0020-Fix-sql-statement-from-session-manager.patch0021-Update-snapshot-test-without-codemode-instructions.patch0100-Downstream-only-never-use-pre-generated-object-files.patchchangeloggoose.spec
|
This seems like it's going in the right direction. We need to dig in and figure out why the builds are failing. |
|
|
|
I think we need to add something like this to patch 0001. diff --git crates/goose/Cargo.toml crates/goose/Cargo.toml
index 9dc486e06..b6150e856 100644
--- crates/goose/Cargo.toml
+++ crates/goose/Cargo.toml
@@ -41,7 +41,6 @@ aws-providers = [
]
cuda = ["local-inference", "candle-core/cuda", "candle-nn/cuda", "llama-cpp-2/cuda"]
vulkan = ["local-inference", "llama-cpp-2/vulkan"]
-mlx = ["local-inference", "dep:mlx-rs", "dep:mlx-lm", "dep:mlx-lm-utils"]
rustls-tls = [
"dep:rustls",
"reqwest/rustls",
@@ -204,9 +203,6 @@ nostr = { version = "0.44", default-features = false, features = ["nip44", "std"
nostr-sdk = { version = "0.44", default-features = false, features = ["nip44"], optional = true }
rustls = { workspace = true, optional = true }
hf-hub = { version = "1.0.0-rc.1", default-features = false, optional = true }
-mlx-rs = { version = "0.25.3", default-features = false, features = ["accelerate", "metal", "safetensors"], optional = true }
-mlx-lm = { git = "https://github.com/jh-block/mlx-lm", optional = true }
-mlx-lm-utils = { git = "https://github.com/jh-block/mlx-lm", optional = true }
pctx_code_mode = { version = "0.3", default-features = false, optional = true } |
|
Thanks for the input & review @samdoran! Now I've addressed two issues actually:
|
ff533ac to
a578c22
Compare
|
Also squashed all my commits cause I had to do a lot of them :) |
r0x0d
left a comment
r0x0d
left a comment
There was a problem hiding this comment.
On a top-level view, this looks mostly fine to me.
Something we could try doing is dropping the feature flags patch, and inside our build-scripts and specfile use more of the --disable-default-features --features native-tls,.... This way we know that we are always disabling and enabling only what we need and we remove a bit of the pain of managing patches for this kind of stuff.
|
So let's hold this one until #50 is merged |
a578c22 to
663d530
Compare
43c15c5 to
13672f2
Compare
13672f2 to
8b4d5d9
Compare
3 participants
PR Description
Updates Goose to version 1.39.0, following the pattern of the previous version updates like:
Used claude code for this task -> especially what we have here:
.claude/commands/goose-update/Changes:
goose.spec: version bumped to 1.39.0changelog: new 1.39.0-1 entry addedNotes for the reviewer
Assisted-by: Claude Sonnet 4.6
Summary by CodeRabbit
/statusCLI with improved session/extension inspection and broader provider/config integration, plus ACP workflow improvements.INSERT ... RETURNINGissues).