chore(deps): land the 17-update grouped bump, fixing the ollama-rs deprecation - #85
Merged
Merged
Conversation
…precation Takes the lockfile from dependabot's grouped PR #83 -- the first PR produced by the grouping added in #71, and exactly what that change was for: one PR with 17 minor/patch updates instead of 17 separate ones. #83 could not merge on its own. `ollama-rs` 0.3.4 -> 0.3.6 deprecated `Ollama::new` in favour of `Ollama::builder().host(host).port(port).build()`, and the workspace builds with `-D warnings`, so the deprecation is a hard error: error: use of deprecated associated function `ollama_rs::Ollama::new` --> pcai_ollama_rs\src\main.rs:382:16 = note: `-D deprecated` implied by `-D warnings` That is the gate working. A grouped lockfile bump reached a real API change and the build refused it, which is the whole point of running clippy with `-D warnings` across the workspace rather than only on pcai_inference. `build_client` now uses the builder. Note `build()` returns `Ollama` directly, not a `Result` -- the first attempt applied `?` to it and failed to compile, which the local build caught before this was pushed. Verified against the grouped lockfile: cargo clippy --workspace --all-targets -D warnings exit 0 cargo test --workspace --features server,ffi --lib 229 passed, 0 failed The one remaining warning is `linker_messages` from the pcai-inference build script, which the compiler itself notes "ignores -D warnings"; it is pre-existing and not introduced here.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
build_client still formats the parsed host in a way that breaks valid IPv6 base URLs (e.g. http://[::1]:11434) by dropping required brackets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR lands a Dependabot grouped set of 17 minor/patch Rust dependency updates for the Native/pcai_core workspace and fixes the ollama-rs deprecation that was blocking -D warnings builds, keeping the workspace CI gate green.
Changes:
- Update
pcai_ollama_rsto construct theOllamaclient viaOllama::builder()instead of deprecatedOllama::new. - Apply the grouped dependency bumps via the updated
Native/pcai_core/Cargo.lock.
File summaries
| File | Description |
|---|---|
| Native/pcai_core/pcai_ollama_rs/src/main.rs | Switches to the new ollama-rs builder API for client creation. |
| Native/pcai_core/Cargo.lock | Updates the lockfile to reflect the grouped dependency bumps. |
Review details
- Files reviewed: 1/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses the IPv6 review finding on this PR, plus two further defects the
test for it uncovered. All three predate this branch -- `build_client` derived
host and port the same way before the builder migration -- but they live in
the function under review, so they are fixed here rather than deferred.
Host derivation moves into `split_base_url`, a pure function, so the cases are
testable at all; `build_client` now just calls it.
1. IPv6 literals lost their brackets
`Url::host_str()` returns a bare `::1` for `http://[::1]:11434`, so
`format!("{}://{}", scheme, host)` produced `http://::1` -- not a valid URL.
`Url::host()` is used instead, whose `Display` re-adds the brackets.
2. The scheme-less form failed outright
`OLLAMA_HOST` is conventionally written the way Ollama documents it, with no
scheme -- this workstation sets `0.0.0.0:11434`. `Url::parse` rejects that, and
the fallback path parses `default_ollama_url()`, which returns `OLLAMA_HOST`
verbatim, so it failed too. `build_client` returned Err in that configuration.
`parse_ollama_url` retries with an `http://` prefix. The `has_host` guard
covers the opposite trap: `localhost:11434` *does* parse, as scheme `localhost`
with path `11434` and no host, so accepting it unchecked would have produced a
client pointed at nothing.
3. The unspecified address was used as a destination
`OLLAMA_HOST` does double duty -- it tells the server what to bind and clients
where to connect. `0.0.0.0` means "bind every interface"; as a destination it
relies on the OS to reinterpret it. `client_host` resolves the unspecified
address to loopback, v4 and v6. Routable addresses are left alone.
Nine unit tests cover IPv4, IPv6, scheme-less, `localhost:port`, unspecified v4
and v6, a routable address, a non-default scheme and port, port defaulting, and
the fallback branch. This is the first test module in this crate.
Scope of the ast-grep change: `flag-hardcoded-endpoints-rs` already exempts
`**/tests/**`, but that path is unreachable for a *binary* crate -- an
integration test cannot call a private fn, so a URL-parsing unit test has to
live in an inline `mod tests`, where literal endpoints are the fixture rather
than a hardcoded default. The rule now exempts that module and nothing else.
Confirmed in both directions: the test module scans clean, and a planted
`"http://127.0.0.1:8080"` inside `build_client` in the same file still raises
the error. Narrowed, not disabled.
Verified:
sg scan (changed file) 0 errors
cargo clippy --workspace --all-targets --no-deps -- -D warnings exit 0
cargo test -p pcai-ollama-rs --bins 9 passed
Not verified: no Ollama daemon was running on this host, so this proves the
endpoint is derived correctly and `build_client` no longer returns Err -- not
that a live connection succeeds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t
This was referenced Sep 8, 2026
David-Martel
added a commit
that referenced
this pull request
Sep 8, 2026
Review caught that the first version of this PR could not bootstrap its own cache, which would have left the workflow permanently worse rather than better. Two compounding faults: `timeout-minutes: 45` covered the whole job, not the audit step. A cold run has to build cargo-audit from source -- ~37 minutes on #85, on top of ~7 for checkout, setup, fmt, clippy and tests. The cap would have killed the very first run mid-install. A cancelled job does not run its post-job phase, and a single `actions/cache` step only saves there. So the killed run would save nothing, the next run would be cold, and it would die the same way. Every subsequent Rust PR stuck in the cold-install cycle the change was meant to remove. Fixed on both axes: - actions/cache split into restore + save, with the save immediately after the install and before `cargo audit` runs. The binary is cached the moment it exists, so even a failing audit leaves a populated cache. - Job timeout raised to 90, above the demonstrated ~45-minute cold run. - The tight bound moves to the audit step itself, `timeout-minutes: 10`, which is where a genuine hang would show once the binary is cached. That bound no longer sits in front of the install, so it cannot starve it. The install is now its own step gated on `cache-hit != 'true'` rather than a `Get-Command` probe inside the audit step, so the cache outcome is visible in the job log instead of inferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t
David-Martel
added a commit
that referenced
this pull request
Sep 9, 2026
* perf(ci): stop rebuilding cargo-audit on every Rust Guidelines run The Audit Dependencies step ran `cargo install cargo-audit --locked` on every invocation, compiling it from source on a Windows runner. PR #85 sat on that one step for 45 minutes with format, clippy and tests already green. It was structurally guaranteed to be a cold build. The Cache Cargo step covers ~/.cargo/registry, ~/.cargo/git and target/ -- not ~/.cargo/bin, where `cargo install` puts the binary. And its key is the Cargo.lock hash, so even if it had covered ~/.cargo/bin, every dependency PR invalidates it by definition: the one kind of PR that most needs an audit was the one guaranteed to pay full build cost for the tool. cargo-audit now has its own cache entry under a stable key, so it survives lockfile churn, and the install is skipped when the binary is already present. Also removes `continue-on-error: true`. The step could not fail, so it reported nothing and gated nothing -- a security check that cannot fail is not a check. Verified before flipping it rather than assuming: cargo audit --file Native/pcai_core/Cargo.lock 776 crate dependencies, exit 0 0 vulnerabilities, 5 advisory warnings (unmaintained/yanked) cargo audit fails on vulnerabilities, not warnings, so this does not red the repo on the current dependency set. It will now fail on a real advisory, which is the point -- this is the only per-PR cargo audit in the repo. ci.yml's Security Scan is a credential scanner, and maintenance.yml only runs weekly. Adds `timeout-minutes: 45` to the job, which otherwise inherits the 6-hour default. The 45-minute audit step was indistinguishable from a hang while it was running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t * fix(ci): break the cold-cache deadlock in the cargo-audit caching Review caught that the first version of this PR could not bootstrap its own cache, which would have left the workflow permanently worse rather than better. Two compounding faults: `timeout-minutes: 45` covered the whole job, not the audit step. A cold run has to build cargo-audit from source -- ~37 minutes on #85, on top of ~7 for checkout, setup, fmt, clippy and tests. The cap would have killed the very first run mid-install. A cancelled job does not run its post-job phase, and a single `actions/cache` step only saves there. So the killed run would save nothing, the next run would be cold, and it would die the same way. Every subsequent Rust PR stuck in the cold-install cycle the change was meant to remove. Fixed on both axes: - actions/cache split into restore + save, with the save immediately after the install and before `cargo audit` runs. The binary is cached the moment it exists, so even a failing audit leaves a populated cache. - Job timeout raised to 90, above the demonstrated ~45-minute cold run. - The tight bound moves to the audit step itself, `timeout-minutes: 10`, which is where a genuine hang would show once the binary is cached. That bound no longer sits in front of the install, so it cannot starve it. The install is now its own step gated on `cache-hit != 'true'` rather than a `Get-Command` probe inside the audit step, so the cache outcome is visible in the job log instead of inferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
David-Martel
added a commit
that referenced
this pull request
Sep 9, 2026
* docs(todo): reconcile four items against what actually happened TODO.md still described a state that three merges and a workflow deletion have since changed. Marking resolved items resolved, and correcting one that had become misleading rather than merely stale. Dependabot backlog -- drained to zero, was "6 open PRs, 1 high-severity alert". The alert is fixed and cargo audit on Native/pcai_core is exit 0 (776 dependencies, 0 vulnerabilities, 5 advisory warnings). Grouping landed in #71, so minor+patch now arrive as one PR per ecosystem. #85 took the 17-update grouped bump, #86 the four .NET test-package majors, #87 held sha2 below 0.11. #84 was refused on evidence, not deferred: RustCrypto 0.11 moves digest output to hybrid-array::Array, which lacks LowerHex, and two transitive deps still require the 0.10 line -- so it breaks three call sites and duplicates sha2. #88/#89/#90 were deferred rather than judged, because they target Deploy. Portable CI (Linux) -- moot, the workflow was deleted. The item asked for a measured timeout, which now cannot be measured and should not be: this repo targets Windows by design, and a Linux job running the PowerShell suite was a misconfiguration rather than coverage. Its one piece of real coverage, workspace-wide cargo checks, moved to rust-guidelines.yml on windows-latest. Left as a struck-through entry rather than deleted so the reasoning survives. CargoTools cargo shim -- second instance recorded. The same preflight that breaks `Build.ps1 -Component functiongemma-router-data` also breaks `cargo test --manifest-path <path> -p <crate>`, dying with a rustfmt usage dump that looks like the crate under test but is entirely the shim. `Get-Command cargo` resolves to ~\bin\cargo.ps1, not rustup's. Workaround for scoped builds is rustup's binary directly. Both instances are one bug in the shim's preflight argument construction, which is worth knowing before someone debugs the second as if it were unrelated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t * docs(todo): reconcile the advisory-warning count with the crate list Review caught that "5 advisory warnings (core2, fxhash, number_prefix, paste)" names four crates for five warnings, which is internally inconsistent and exactly the kind of number nobody can later verify. The count is right and the list was incomplete rather than wrong: core2 is reported twice, once unmaintained and once yanked, so four crates produce five warnings. Spelled out, along with why the exit code is still 0 -- cargo audit fails on vulnerabilities, not warnings. Crate: core2 Warning: unmaintained Crate: fxhash Warning: unmaintained Crate: number_prefix Warning: unmaintained Crate: paste Warning: unmaintained Crate: core2 Warning: yanked warning: 5 allowed warnings found Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
Supersedes #83, dependabot's first grouped PR — and the proof that the grouping added in #71 works: one PR with 17 minor/patch updates instead of 17 separate ones.
Why #83 couldn't merge on its own
Its Rust Guidelines job failed.
ollama-rs0.3.4 → 0.3.6 deprecatedOllama::newin favour ofOllama::builder().host(host).port(port).build(), and the workspace builds with-D warnings, so the deprecation is a hard error:That is the gate working as designed. A lockfile-only bump reached a real API change and the build refused it — which is exactly why workspace-wide clippy matters, rather than checking
pcai_inferencealone asci.yml'srust-checkdoes.build_clientnow uses the builder. Worth noting:build()returnsOllamadirectly, not aResult— my first attempt applied?to it and failed to compile. The local build caught that before it was pushed.Verification
The one remaining warning is
linker_messagesfrom thepcai-inferencebuild script, which the compiler itself notes "ignores-D warnings" — pre-existing, not introduced here.Note on branching
This branches from
mainand takes #83's lockfile viagit checkout <branch> -- Cargo.lock, rather than checking out #83 directly. Another writer has uncommitted work inTools/in this tree; #83 predates those files, so a full checkout would have deleted them. Their work is untouched.Scope grew after review: three endpoint-parsing bugs (9867be6)
Copilot flagged that
Url::host_str()drops IPv6 brackets, soformat!("{}://{}", scheme, host)producedhttp://::1. Correct. Fixing it properly meant making the code testable, and the test surfaced two more defects in the same function. All three predate this branch —build_clientderived host and port identically before the builder migration — but they are in the function under review.Host derivation moved into
split_base_url, a pure function.build_clientjust calls it.http://::1)Url::host(), whoseDisplayre-adds themOLLAMA_HOSTfailed outrighthttp://prefix, guarded byhas_host0.0.0.0used as a connect destinationclient_hostresolves the unspecified address to loopbackDefect 2 is the significant one.
OLLAMA_HOSTis conventionally written the way Ollama documents it, without a scheme — this workstation sets0.0.0.0:11434.Url::parserejects that, and the fallback parsesdefault_ollama_url(), which returnsOLLAMA_HOSTverbatim, so it failed too andbuild_clientreturnedErr. Thehas_hostguard covers the mirror-image trap:localhost:11434does parse, as schemelocalhostwith path11434and no host at all.Nine unit tests — the first test module in this crate.
This also required narrowing an ast-grep rule
Not something a "grouped dependency bump" title leads you to expect, so calling it out explicitly.
flag-hardcoded-endpoints-rsisseverity: errorand blocked the commit: the tests contain literal endpoints. The rule already exempts**/tests/**, but that path is unreachable for a binary crate — an integration test cannot call a private fn, so a URL-parsing unit test has to live in an inlinemod tests. There, literal endpoints are the fixture, not a hardcoded default. The rule now exempts that module and nothing else.Verified in both directions, because a lint that cannot fail is not a lint:
"http://127.0.0.1:8080"insidebuild_client, same file, still raises the errorVerification
Not verified: no Ollama daemon was running on this host, so the above proves the endpoint is derived correctly and
build_clientno longer returnsErr— not that a live connection succeeds.