chore(deps+ci): consolidate the dependabot backlog, fix dead CI triggers, drop the Linux gate - #71
Merged
Conversation
…workspace Supersedes dependabot PRs #69, #53, #52, #51 and #49. All five were opened against the pre-2b916b4 main and are now 61 commits behind. Merging them individually would have replayed five stale Cargo.lock files over the lockfile that carries the quinn-proto 0.11.17 fix for Dependabot alert #28, silently reverting it. Bumps, all re-resolved against current main and verified together: log 0.4.29 -> 0.4.34 (#69, lockfile only) memmap2 0.9.10 -> 0.9.11 (#49, lockfile only) mimalloc 0.1.50 -> 0.1.52 (#52, lockfile only; libmimalloc-sys 0.1.49) windows 0.58 -> 0.62.2 (#53, pcai_core_lib manifest) tokenizers 0.22 -> 0.23.1 (#51, pcai_media manifest; resolves 0.23.2) Verified on this branch rather than on the individual PR branches: cargo fmt --all --check clean cargo clippy --workspace --all-targets -D warnings clean cargo test --workspace --features server,ffi --lib 229 passed, 0 failed cargo check -p pcai-media -p pcai-media-model -p pcai-media-server --all-targets clean The three media crates are checked explicitly because portable-ci.yml excludes all of them from both its clippy and its test step. Without that check the tokenizers bump would have reached main with no gate covering it. quinn-proto remains at 0.11.17.
Supersedes dependabot PR #64, which raised PcaiNative alone. PcaiNative 10.0.9 -> 10.0.11 (#64) PcaiServiceHost 10.0.7 -> 10.0.11 (not covered by #64) Dependabot opened a PR for PcaiNative only, leaving PcaiServiceHost two further patch releases behind on the same package. Two versions of System.Text.Json across a single .NET surface is the kind of drift that goes unnoticed until a serialization difference shows up at runtime, so both move together here. Verified: dotnet build Native/PcaiNative/PcaiNative.csproj -c Release succeeded dotnet build Native/PcaiServiceHost/PcaiServiceHost.csproj -c Release succeeded The single CS1574 warning in PcaiNative/MediaModule.cs is pre-existing on main and unrelated to this bump.
… gaps
Push triggers
─────────────
ci.yml, rust-guidelines.yml and nvidia-validation.yml each declare
push:
branches: [develop]
There is no develop branch. `git rev-parse origin/develop` fails and the
remote has never carried one, so all three workflows have only ever run on
pull_request. Nothing has validated main after a merge: the unified gate,
the Rust guidelines job and the NVIDIA stack job all sat idle on every push
that has ever landed. This is the same class of defect as the rest of the
2b916b4 merge -- configuration that reads as enforced and is not.
All three now trigger on [main, develop]. develop is kept so the trigger
still works if that branch is ever created; main is what actually fires
today. portable-ci.yml is deliberately left pull_request-only -- it is the
slow job that has hit the 60-minute cap before, and duplicating it on every
merge buys little that its PR run has not already proved.
dependabot.yml
──────────────
1. Grouping. Every ecosystem now groups minor+patch into one PR. Ungrouped
updates produced nine open PRs in the 2026-09 batch alone, which then
fell 61 commits behind main, each carrying a stale Cargo.lock. Replaying
those lockfiles one at a time would have reverted the quinn-proto
0.11.17 fix for alert #28. Major bumps stay ungrouped so an API break
still gets its own reviewable PR.
2. Coverage. The .NET config listed PcaiNative and PcaiChatTui only. The
repo has five csproj files. PcaiServiceHost was unwatched and had drifted
to System.Text.Json 10.0.7 while the watched PcaiNative sat at 10.0.9 --
two versions of one package in one solution, invisible because only half
the projects were being looked at. PcaiServiceHost and both test projects
are now covered.
3. candle is pinned. Dependabot raised candle-transformers 0.9.2 -> 0.11.0
in #66 by itself, leaving rust-functiongemma-core depending on both
candle-core 0.9.2 and candle-transformers 0.11.0 -- two semver-
incompatible copies of Tensor in one crate. The workspace [patch] table
also points at vendored kernels pinned to 0.9.2, so moving candle means
re-vendoring them. The whole candle stack is now ignored; that upgrade is
a deliberate coordinated change, not a dependabot bump.
The /Deploy entry carries a comment recording that no CI job builds that
workspace, so its bumps must be verified locally.
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
The tokenizers version in pcai_media/Cargo.toml does not match the resolved Cargo.lock version, creating a mismatch with the PR’s stated dependency target.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Consolidates a backlog of stale Dependabot updates into a single, up-to-date dependency refresh, while also fixing GitHub Actions workflows that were effectively “PR-only” due to a non-existent develop push trigger—ensuring main merges are validated post-merge.
Changes:
- Bumped Rust and .NET dependencies (including
tokenizers,windows,memmap2,mimalloc,log, andSystem.Text.Json) while updatingCargo.lockaccordingly. - Repaired CI workflow
pushtriggers to run onmain(anddevelop) forci.yml,rust-guidelines.yml, andnvidia-validation.yml. - Updated
dependabot.ymlto group minor/patch updates per ecosystem and expanded NuGet coverage (including ServiceHost + test projects), while explicitly ignoring the candle stack in/Deploy.
File summaries
| File | Description |
|---|---|
| Native/PcaiServiceHost/PcaiServiceHost.csproj | Bumps System.Text.Json to align ServiceHost with other .NET projects. |
| Native/PcaiNative/PcaiNative.csproj | Bumps System.Text.Json to the consolidated target version. |
| Native/pcai_core/pcai_media/Cargo.toml | Updates Rust dependency versions for the media crate (tokenizers). |
| Native/pcai_core/pcai_core_lib/Cargo.toml | Updates windows crate version for Windows-specific bindings. |
| Native/pcai_core/Cargo.lock | Consolidated lockfile refresh for the Rust workspace dependency bumps. |
| .github/workflows/rust-guidelines.yml | Ensures Rust guidelines workflow runs on pushes to main. |
| .github/workflows/nvidia-validation.yml | Ensures NVIDIA validation workflow runs on pushes to main. |
| .github/workflows/ci.yml | Ensures main CI gate runs on pushes to main. |
| .github/dependabot.yml | Groups minor/patch updates, expands NuGet coverage, and pins/ignores candle stack upgrades in /Deploy. |
Review details
- Files reviewed: 8/9 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3faca543bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This repo targets Windows and is aggressively optimized for it. Cross-platform portability was never an objective; Linux is in scope only where a Docker/WSL build, test or deploy requirement calls for it. `portable-ci.yml` ran the whole PowerShell unit suite on ubuntu-latest, which is not that. It has never passed. Every branch fails it -- main's, #58's, and all nine of the open dependabot branches -- and it only stopped blocking work because the gate is not a required check. The failures are not defects: - `Join-Path $PSScriptRoot '..\..\Modules\...'` uses backslashes, which are not path separators on Linux, so 17 files failed at import and every subsequent `Mock -ModuleName ...` reported the module was not loaded. - `Mock Get-CimInstance` / `Mock Get-Service` cannot bind on Linux, because Pester's Mock copies parameter metadata from an *existing* command. - There are 144 further Windows-specific constructs across 24 test files: registry providers, drive letters, %TEMP% semantics. Those tests are correct. They were mistagged 'Portable' and pointed at a runner that cannot host them. Rewriting 24 files to satisfy a platform the project does not target would trade real Windows coverage for none. No Rust coverage is lost. rust-guidelines.yml already runs, on windows-latest and against the same virtual workspace root: cargo fmt --all --check cargo clippy --all-targets --no-default-features --features server,ffi -- -D warnings cargo test --no-default-features --features server,ffi --lib `Native/pcai_core/Cargo.toml` has no [package] table, so those root-level invocations cover all seven members -- the same scope portable-ci.yml had, minus the wrong operating system. ci.yml additionally covers security scan, PowerShell lint and tests, the llamacpp build and integration tests, all on windows-latest. CLAUDE.md now records the platform scope explicitly, so this does not get reintroduced as "missing coverage".
David-Martel
changed the title
git rev-parse origin/develop fails and the remote has never carried one. CLAUDE.md no longer documents it either. Listing it in pull_request and push triggers left exactly the pattern this branch set out to remove: configuration that reads as meaningful and is not. Every workflow 2026-09-08 13:37:39 UTC UTC targets main only.
…to the lock
Addresses both review findings on this PR.
1. pcai_media declared `tokenizers = "0.23.1"` while Cargo.lock resolves
0.23.2, so the manifest disagreed with what `--locked` actually builds.
The manifest now says 0.23.2; the lockfile is unchanged.
2. This PR adds dependabot coverage for PcaiServiceHost and both .NET test
projects -- but **no CI job builds any .NET project**. The only .NET build
that has ever run lived in portable-ci.yml, which compiled PcaiNative
alone, on Linux, and converted failures into warnings; and that workflow is
deleted in this same PR. A dependabot bump to ServiceHost could therefore
satisfy every gate while leaving the project unable to restore or compile.
That is exactly the "configuration that reads as enforced and is not"
pattern the rest of this branch exists to remove, and adding the dependabot
entries without a build would have widened it rather than closed it.
The new dotnet-build job compiles all five csproj files and runs both test
projects, and is wired into the CI Gate's `needs` and its result list, so a
.NET failure now fails the gate. It discovers projects rather than listing
them, with a positive control: discovering fewer than five is a hard error,
so a glob that silently matches nothing cannot report success.
Verified locally before pushing:
Release build, 5/5 succeeded PcaiChatTui, PcaiChatTui.Tests, PcaiNative,
PcaiNative.Tests, PcaiServiceHost
dotnet test PcaiChatTui.Tests 9 passed, 0 failed
PcaiNative.Tests 6 passed, 0 failed
One inconsistency found while doing this and deliberately not changed:
PcaiChatTui.Tests targets net10.0 while the other four target net8.0 -- a test
project on a newer framework than the code it exercises. It builds locally
because the 10.x SDK is present, and would have failed on a runner given only
8.0.x, which is part of why the gap stayed invisible. The job installs both
SDKs so the gate works today; aligning the TFMs is a separate decision.
The new dotnet-build job failed with MSB3030: Could not copy the file Native/PcaiNative/pcai_core_lib.dll because it was not found. PcaiServiceHost.csproj copied that Rust artifact with no Exists condition, while PcaiNative.csproj already guards its identical copies. The DLL is not in git, so the project could only build on a machine that had run the Rust build first -- which is exactly why nothing noticed until a CI job compiled it for the first time. Verified both directions locally: with the DLL moved aside, PcaiServiceHost builds clean (exit 0, 0 errors); with it present, it is still copied to bin/Release/net8.0/win-x64/pcai_core_lib.dll, so runtime behaviour is unchanged.
This was referenced Sep 8, 2026
David-Martel
added a commit
that referenced
this pull request
Sep 8, 2026
…precation (#85) * chore(deps): land the 17-update grouped bump, fixing the ollama-rs deprecation 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. * fix(ollama): parse the endpoint forms Ollama actually emits 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 --------- 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.
Consolidates the dependabot backlog, repairs CI triggers that never fired, and removes a Linux gate that enforced a non-goal.
Why not just merge the dependabot PRs
All nine were opened against the pre-
2b916b4main and are 61 commits behind. Each carries its own staleCargo.lock. Merging them one at a time would replay those lockfiles over the one on main that carries quinn-proto 0.11.17 — the fix for Dependabot alert #28 — silently reverting it. Resolving them together against current main avoids that.quinn-protois verified still at 0.11.17.Dependency bumps (supersedes #69, #53, #52, #51, #49, #64)
logmemmap2mimallocwindowstokenizersSystem.Text.Json(PcaiNative)System.Text.Json(PcaiServiceHost)Verified locally on this branch, not on the individual PR branches:
CI triggers that never fired
ci.yml,rust-guidelines.ymlandnvidia-validation.ymleach declaredpush: branches: [develop]. There is nodevelopbranch —git rev-parse origin/developfails and the remote has never had one. All three therefore only ever ran onpull_request; nothing validatedmainafter a merge. All three now trigger on[main, develop].Removing
portable-ci.ymlThis repo targets Windows and is optimized for it; cross-platform portability is not a goal.
portable-ci.ymlran the whole PowerShell unit suite onubuntu-latestand has never passed on any branch — main's, #58's, or any of the nine dependabot branches. It only stopped blocking work because the gate is not a required check.The failures were not defects: backslash paths in
Join-Path $PSScriptRoot '..\..\Modules\...'broke 17 imports on Linux,Mock Get-CimInstancecannot bind to a command that does not exist there, and there are 144 further Windows-specific constructs across 24 test files. Those tests are correct and were mistaggedPortable.No Rust coverage is lost.
rust-guidelines.ymlalready runscargo fmt --all --check,cargo clippy --all-targets -- -D warningsandcargo test --libon windows-latest against the same workspace root.Native/pcai_core/Cargo.tomlhas no[package]table, so those root-level invocations cover all seven members — the same scope, minus the wrong OS.CLAUDE.md now records the platform scope explicitly.
PR #66 should be closed, not merged
#66raisescandle-transformers0.9.2 → 0.11.0 alone, leavingrust-functiongemma-coredepending oncandle-core 0.9.2,candle-nn 0.9.2andcandle-transformers 0.11.0— two semver-incompatible copies ofTensorin one crate. The[patch]table also pins vendored kernels at0.9.2. The candle stack is now in dependabot'signorelist.#67 / #68 are not included — the Deploy workspace does not compile
Verified in a VS developer environment with CUDA 13.1:
cargo checkonDeployfails with 10 errors inrust-functiongemma-core(model.rs:91,149-151,206-209). A baseline run on unmodified main produces the identical 10 errors, so this is pre-existing and unrelated to itertools/nvml.Cause:
rust-functiongemma-coredeclaresqlora-rs = "1.0.5", a caret range, and cargo has drifted it to 1.3.0, which moved to candle 0.11 — so the crate mixes candle 0.9.2 and 0.11.0 types. Pinning--precise 1.0.5does not fix it: qlora-rs 1.0.5 then fails to compile on its own (9 errors). This needs real repair, not a version tweak, and nothing in CI notices becauserust-guidelines.ymlhas theDeploymatrix entry commented out.