feat: combine #9 (Docker workflow + GHCR) and #11 (harden GH Actions CI) into one PR - #17
Conversation
Added caching and stale issue management to the workflow.
…anguage/rust/, schedule, concurrency, rust-cache, llvm-cov upload, separate ymls ready; remove junk rust.yml; fmt cleanup for green checks; README dev/CI section) - Implements key ACs from #11 (and reviews: Codecov, no gpu test, separate yml pref) - Aligns local to remote + enhancements from corinth patterns (one-way inspiration, no dep) - Prepares for #8/#9 cross refs and other CI issues - Addresses local divergence (junk rust.yml gone) Refs #11, #8, #9
…CI) into one PR - #11: hardened ci.yml (schedule, concurrency, Swatinem/rust-cache, Codecov per your review link, etc.), added codecov.yml, README Development/CI section, removed junk rust.yml, fmt cleanup - #9: added Dockerfile (multi-stage CPU-only, RUST_VERSION=1.85 for edition2024, .dockerignore to exclude nested dir/target for clean builds) + .github/workflows/docker-build.yml (modeled on corinth, standard ubuntu runners, GHA cache, GHCR publish only on main) - Per user: one PR for 9+11; skipping #8 (Azure) for now - Using your Docker CLI wrapper for any local verification (as required in reviews) Refs #9 #11
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2995be4e-fe63-403d-8b73-d45deaa983ee) |
|
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:
📝 WalkthroughWalkthroughAdds Docker build inputs, CI coverage and caching updates, a Docker image build workflow with GHCR publishing, and README sections describing the new workflows and related project context. ChangesCI/CD and Docker setup
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
PR Summary by QodoHarden GitHub Actions CI with Codecov and add GHCR Docker build workflow
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
|
CodeAnt AI finished reviewing your PR. |
Code Review by Qodo
Context used 1.
|
Qodo Fixer✅ Committed (5) · ☑ Fixed (5) Commits pushed directly to this PR — no separate fix PR opened. Process — 5 fixed
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Incremental Review (since e2a9959)All 1 changed file reviewed. The incremental change consolidates the previously split
No new bugs, security issues, or code quality concerns found in the changed code. Previous Review Summaries (9 snapshots, latest commit e2a9394)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e2a9394)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental Review (since daa9959)All 2 changed files reviewed. The incremental changes are cleanup/corrections:
No new bugs, security issues, or code quality concerns found in the changed code. Previous review (commit daa9959)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental Review (since 2c1643e)All 2 changed files reviewed. The incremental changes are improvements that address prior review findings:
No new bugs, security issues, or code quality concerns found in the changed code. Previous review (commit 2c1643e)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Incremental Review (since fe1bf98)All 4 changed files reviewed. The incremental changes are improvements that address prior review findings:
No new bugs, security issues, or code quality concerns found in the changed code. Previous review (commit fe1bf98)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 4e90722)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 6cf0e32)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit fec8432)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 5a382a4)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Files Reviewed (21 files)
Fix these issues in Kilo Cloud Previous review (commit 2d43222)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (11 files)
Reviewed by mimo-v2.5 · Input: 100.7K · Output: 6.7K · Cached: 583.3K |
- .dockerignore: remove Cargo.lock exclusion that broke Docker COPY - Dockerfile: fix CMD to use echo instead of unavailable cargo binary - Dockerfile: remove unnecessary pkg-config/libssl-dev (zero-dep crate) - Dockerfile: remove no-op dummy-main dep caching layer - Dockerfile: copy only library artifact instead of entire target/release - ci.yml: consolidate redundant rustup component install into dtolnay action - docker-build.yml: fix checkout action SHA (was invalid v5, now v4.2.2) Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
… CI/Docker additions) Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Remove non-functional runtime stage that copied an .rlib into a debian image with no Rust toolchain. The image now serves as a CI verification / reproducible build environment with cargo available. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
- Pin Debian base image to bookworm-slim for reproducibility - Move GitHub expressions to env vars to prevent template injection - Both changes address coderabbitai review feedback Co-Authored-By: Mimo Code agent: MiMo-V2.5
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
22-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
appuserdoesn't own/app, so the default CMD will fail with permission denied.
useraddruns at Line 22, but/appand its contents (including/app/targetbuilt at Lines 33-34) are all owned byroot, since everyCOPY/RUNbeforeUSER appuser(Line 36) executes as root. When the container runs the defaultCMD ["cargo", "test", "--all-features"](Line 38) asappuser, cargo needs write access to/app/target(and potentially the registry cache) to compile the test binaries — butappuserhas no write permission there. This directly breaks the documented usage in the README (docker run --rm engram-parser cargo test --all-features).🐛 Proposed fix
RUN cargo build --release --all-features && \ cargo test --release --all-features +RUN chown -R appuser:appuser /app + USER appuser🤖 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 `@Dockerfile` around lines 22 - 38, The default container user cannot write to /app or the build artifacts created by the Dockerfile, so the Cargo test command will fail under appuser. Update the Dockerfile so the /app directory and relevant contents are owned or writable by appuser before switching users, using the existing WORKDIR, COPY, cargo build, and USER appuser steps to locate the change. Ensure the final CMD can run cargo test --all-features without permission issues by assigning ownership or adjusting permissions for /app and /app/target after the build.
🧹 Nitpick comments (1)
Dockerfile (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCMD test flags diverge from the build-time test invocation.
Line 34 runs
cargo test --release --all-features, but the defaultCMDat Line 38 runscargo test --all-featureswithout--release. This triggers a separate debug-profile compilation at container start rather than reusing the release artifacts already built and verified in the image, wasting time and (compounding the ownership issue above) requiring extra writable build output.♻️ Proposed fix
-CMD ["cargo", "test", "--all-features"] +CMD ["cargo", "test", "--release", "--all-features"]🤖 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 `@Dockerfile` around lines 33 - 38, The Dockerfile’s default CMD is running cargo test without the same release profile used during the image build, so it recompiles in debug mode at startup instead of reusing the verified artifacts. Update the CMD to match the build-time test invocation by using the same cargo test flags as the existing release test step, and keep the change consistent with the RUN cargo build and cargo test sequence already present.
🤖 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.
Outside diff comments:
In `@Dockerfile`:
- Around line 22-38: The default container user cannot write to /app or the
build artifacts created by the Dockerfile, so the Cargo test command will fail
under appuser. Update the Dockerfile so the /app directory and relevant contents
are owned or writable by appuser before switching users, using the existing
WORKDIR, COPY, cargo build, and USER appuser steps to locate the change. Ensure
the final CMD can run cargo test --all-features without permission issues by
assigning ownership or adjusting permissions for /app and /app/target after the
build.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 33-38: The Dockerfile’s default CMD is running cargo test without
the same release profile used during the image build, so it recompiles in debug
mode at startup instead of reusing the verified artifacts. Update the CMD to
match the build-time test invocation by using the same cargo test flags as the
existing release test step, and keep the change consistent with the RUN cargo
build and cargo test sequence already present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11c7d360-5659-4f09-ab01-e83831ec283e
📒 Files selected for processing (2)
DockerfileREADME.md
✅ Files skipped from review due to trivial changes (1)
- README.md
- Change coverage command from --lib to --all-targets (codacy suggestion) - Fixes coverage to include integration tests and doc tests Addresses review feedback from codacy-production and coderabbitai bots. Co-Authored-By: Mimo Code agent: MiMo-V2.5
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…permissions) Addresses three outstanding review comments from PR #17: 1. src/moe/extract.rs: Use checked_add for end calculation in stacked_slice_range to prevent overflow from untrusted GGUF metadata (codacy-production review). 2. .github/workflows/ci.yml: Add clean-tree guard step after build+test to verify no unexpected artifacts are left behind (qodo-code-review). 3. .github/workflows/docker-build.yml: Split Docker workflow into build (minimal permissions for PRs) + publish (packages:write only on main push) jobs to follow least-privilege principle (qodo-code-review). Co-Authored-By: Cline agent: Deepseek-v4-flash
- Dockerfile: Add RUN cargo fetch between manifest COPY and source COPY to complete the Docker layer caching pattern for future dependency use (devin-ai-integration review) - docker-build.yml: Make cancel-in-progress conditional on pull_request events (like ci.yml), preventing accidental cancellation of main-branch Docker image publications (devin-ai-integration review) - docker-build.yml: Use github.event.pull_request.head.sha for PR builds instead of github.sha (merge commit), providing more intuitive source traceability for PR Docker image tags (devin-ai-integration review) Co-Authored-By: Cline agent: Deepseek-v4-pro
cargo fetch requires a target (src/lib.rs, src/main.rs, [lib], or [[bin]]) to parse Cargo.toml. Since source is copied after manifests, cargo fetch fails with "no targets specified in the manifest". The Docker layer caching pattern is already adequately handled by the separate COPY steps; adding cargo fetch for a future crate with deps would also require dummy source scaffolding (rejected for simplicity per ce7aeab single-stage design). Verified: docker build succeeds, docker run passes all 6 tests. Co-Authored-By: Cline agent: Deepseek-v4-pro
The parsers.gcov.branch_detection section in .github/codecov.yml was dead config: CI generates lcov format via cargo llvm-cov --lcov, not gcov. Removed to avoid confusion (flagged by devin-ai-integration). Co-Authored-By: Cline agent: Deepseek-v4-pro
| COPY Cargo.toml Cargo.lock ./ | ||
|
|
||
| # Copy source | ||
| COPY . . |
There was a problem hiding this comment.
📝 Info: Dockerfile COPY pattern defeats dependency layer caching
The Dockerfile copies manifests first (COPY Cargo.toml Cargo.lock ./ at line 27), then immediately copies everything (COPY . . at line 30) before any cargo build. The typical Docker caching pattern is: copy manifests → run dependency build → copy source → run full build. Since there's no build step between the two COPYs, the first COPY provides no caching benefit — any source change invalidates the COPY . . layer and triggers a full rebuild anyway. For a zero-dependency crate this is moot today, but the comment "Copy manifests and lock file for reproducibility" is misleading about the actual purpose. If dependencies are ever added, this pattern would need restructuring to actually cache them.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the separate COPY Cargo.toml/Cargo.lock before COPY . . is kept for organizational clarity and forward compatibility. This crate has zero deps today so there is no caching benefit either way. Adding RUN cargo fetch between the COPYs was attempted but failed because cargo requires a target (src/lib.rs) to parse Cargo.toml. The standard Docker dummy-source pattern was rejected for simplicity in the single-stage design (ce7aeab).
— Cline agent: DeepSeek-v4-pro
…checks) The separate publish job (if: push to main, needs: build) showed as "Skipped" in the PR checks UI, cluttering the status list. Merged back into a single build job with login+push gated by if conditions. The packages:write permission is requested at the job level but the token is only used for publishing on main pushes (login and push steps both have the same if guard). Co-Authored-By: Cline agent: DeepSeek-v4-pro
Summary
Fixes all bugs found by Devin Review in the combined Docker (#9) + CI hardening (#11) PR, plus resolves merge conflicts with
main.Docker fixes (
Dockerfile,.dockerignore,docker-build.yml):.dockerignorethat brokeCOPY Cargo.lockCMD ["cargo", "--version"]→CMD ["echo", ...](cargo not available indebian:stable-slimruntime)pkg-config/libssl-dev(zero-dep crate)[dependencies])COPY --from=builder /app/target/release→ copy onlylibengram_parser.rlibRUST_VERSION1.85 → 1.87 to matchCargo.tomlrust-versiondocker/setup-buildx-action(required forcache-from/to: type=gha)github.repositoryin tag script (Docker rejects uppercase)CI fixes (
.github/workflows/ci.yml):rustup component add rustfmt clippyintodtolnay/rust-toolchaincomponents:field (nowclippy, rustfmt, llvm-tools-preview)Checkout SHA fix (
docker-build.yml):actions/checkout@93cb6efe...(invalid "v5") →@11bd71901bbe...(v4.2.2, matchesci.yml)Merge with
main:cursor.rs,layout.rs,tensor.rs,extract.rs,ci.yml,README.mdchunks_exactapproach overunsafe from_raw_partsin tensor readersslice_stacked_expert(layout, block, tensor, expert)signature, fixing the hardcodedblock: 0inExpertOutOfRangeerrorread_tensor_entry,validate_stacked_expert_count, etc.)Link to Devin session: https://app.devin.ai/sessions/4f1d3f109f70401285b8f5fbd9dd7ed1
Requested by: @rmems
Summary by CodeRabbit
.dockerignoreto reduce Docker build context.Greptile Summary
This PR adds Docker publishing and hardens the Rust CI setup. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix: merge publish job back into single ..." | Re-trigger Greptile