chore: Dev tools for consistent pre-PR state#287
Closed
taqtiqa-mark wants to merge 7 commits intoqhkm:mainfrom
Closed
chore: Dev tools for consistent pre-PR state#287taqtiqa-mark wants to merge 7 commits intoqhkm:mainfrom
taqtiqa-mark wants to merge 7 commits intoqhkm:mainfrom
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:
📝 WalkthroughWalkthroughAdds containerized Rust development tooling (Dockerfile.dev, container orchestration scripts), clippy/lint configuration, developer docs, and updates ignore rules to include Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Host as Host Shell
participant Script as container-base.sh
participant Runtime as Podman/Docker
participant Container as Dev Container
participant Cargo as Cargo / Rust Tools
participant Cache as Mounted Caches
Dev->>Host: run scripts/{lint,test,bench}-container.sh
Host->>Script: source & invoke container_run
Script->>Runtime: ensure image, create/attach volumes, set env
Runtime->>Container: start container with mounts
Container->>Cache: access target/registry/benches and sccache
Container->>Cargo: run cargo clippy/test/bench (uses /clippy.toml)
Cargo-->>Dev: outputs results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 14
🧹 Nitpick comments (3)
src/skills/github_source.rs (2)
208-211: Consider defensive parsing forfull_name.The code assumes
full_nameis always in "owner/repo" format. While this is guaranteed by GitHub's API, defensive parsing would prevent panics if data is malformed.💡 Optional: defensive split
let checks = search_response.items.iter().map(|repo| { - let owner_repo: Vec<&str> = repo.full_name.split('/').collect(); - // Assume GitHub repos have owner/repo format - check_skill_md_exists(client, owner_repo[0], owner_repo[1], github_token) + let (owner, repo_name) = repo.full_name.split_once('/').unwrap_or(("", &repo.full_name)); + check_skill_md_exists(client, owner, repo_name, github_token) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/github_source.rs` around lines 208 - 211, The code assumes repo.full_name always splits into owner/repo which can panic; update the mapping in the iterator over search_response.items to defensively parse full_name (use splitn(2, '/') or equivalent), check that you get exactly two parts before indexing, and handle malformed values by logging or skipping that item instead of indexing owner_repo[0]/owner_repo[1]; apply this change in the closure that calls check_skill_md_exists so only valid owner/repo pairs are passed to check_skill_md_exists.
206-234: Deep/fast mode branching is well-structured.The concurrent SKILL.md checks using
join_allare efficient, andunwrap_or(false)provides graceful error handling.One consideration: the GitHub search API request (lines 193-198) doesn't use the token, only the SKILL.md checks do. Using the token for the search request too would increase rate limits from 10 req/min (unauthenticated) to 30 req/min (authenticated).
💡 Optional: use token for search API request too
- let response = client + let mut request = client .get(&url) .header("User-Agent", "zeptoclaw") - .header("Accept", "application/vnd.github.v3+json") - .send() - .await?; + .header("Accept", "application/vnd.github.v3+json"); + if let Some(token) = github_token { + request = request.header("Authorization", format!("Bearer {}", token)); + } + let response = request.send().await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/github_source.rs` around lines 206 - 234, The GitHub search request currently ignores github_token; modify the code that performs the search (the call that produces search_response) to include the Authorization header when github_token.is_some() so the search call is authenticated; use the existing github_token value (e.g., "token <token>" or "Bearer <token>" as appropriate for your HTTP client) so the search API benefits from authenticated rate limits and matches the subsequent check_skill_md_exists usage.scripts/container-base.sh (1)
36-44: Consider non-interactive environments.The
-itflags require a TTY and may fail in CI pipelines or non-interactive contexts. For a dev tool this is acceptable, but consider making it conditional for broader compatibility.💡 Optional: conditional TTY allocation
container_run() { local cmd=("$@") + local tty_flags="" + if [[ -t 0 ]]; then + tty_flags="-it" + fi $RUNTIME run --rm -it \ + $RUNTIME run --rm $tty_flags \ -w "$WORKDIR" \ -v "$(pwd)":/src:rw \ $TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mount \ $IMAGE \ bash -c "${cmd[*]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` around lines 36 - 44, The container_run function currently always passes -it to $RUNTIME run which forces TTY allocation and breaks in non-interactive CI; change the invocation to conditionally include -i/-t (or neither) based on a check (e.g., if [ -t 1 ] or an env var like CONTAINER_TTY) so that the $RUNTIME run call omits -it in non-TTY environments; update the $RUNTIME run command construction in container_run to add the flags only when the check/env indicates an interactive terminal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.dockerignore:
- Line 21: The negation line '!scripts/artifacts/' in .dockerignore is
ineffective unless a broader ignore (e.g., 'scripts/' or a pattern that matches
it) exists earlier; either remove the negation if you intend to include the
whole scripts/ tree, or re-add an ignore for 'scripts/' (or the broader exclude
you previously had) above the '!scripts/artifacts/' line so that the negation
actually allows only scripts/artifacts/ into the Docker build context—update the
.dockerignore accordingly and re-run the provided verification script to confirm
the scripts/ path behavior.
In @.github/workflows/sync.yml:
- Around line 4-5: The schedule cron line currently uses an invalid minute step
"*/180 * * * *" under the schedule -> cron entry; replace that cron expression
with a standard 5-field POSIX expression that runs at minute 0 of every 3rd hour
(use "0 */3 * * *") so the workflow runs every 3 hours rather than every hour at
minute 0.
In `@Cargo.toml`:
- Around line 282-292: The Cargo.toml currently uses crate-wide suppression
sections ([lints.clippy] and [lints.rust]) that blanket-ignore rules like
unnecessary_unwrap and deprecated; move these suppressions out of Cargo.toml and
into code with explicit justification by removing the [lints.clippy] and
[lints.rust] entries and instead applying targeted #[allow(...)] attributes only
where necessary or, if a genuine crate-wide exception is required, add a single
commented crate-level allowance in lib.rs (with a clear rationale and TODO)
referencing the specific lint names (e.g., unnecessary_unwrap, deprecated) so
clippy can still be run with -D warnings and only pre-approved exceptions are
silenced.
In `@Dockerfile.dev`:
- Around line 1-17: The Dockerfile currently leaves the container running as
root and sets WORKDIR /src, which causes bind-mounted files to be created as
root on the host; fix by adding a non-root user and switching to it before
touching /src (or support passing the caller UID/GID via build/run ARGs and
creating the user with that UID/GID), ensure you create the user (e.g., adduser
or useradd) and set USER to that account (and optionally chown /src after
creating the user) so subsequent cargo/rust operations do not write files as
root; update the Dockerfile entries around WORKDIR /src, the RUN lines that
install components, and the final USER/CMD to reflect the non-root user.
- Around line 9-10: The Dockerfile currently installs cargo-nextest without
pinning the crate version; update the RUN command that installs cargo-nextest so
it pins an explicit version using the cargo install syntax (e.g., change the RUN
installing "cargo-nextest" to use "cargo-nextest@<version> --locked") so the
build is reproducible and the installed binary is fixed.
In `@docs/scripts.md`:
- Around line 8-48: Update each fenced code block in the docs/scripts.md diff to
include the language tag "bash" (change ``` to ```bash) so markdownlint MD040 is
satisfied; this includes the initial Docker/Podman build block, the Podman
machine init/start block, the Volumes block, the sccache Host/Env block, and the
Usage examples block (all triple-backtick fenced sections shown in the diff).
- Line 50: Update the Dry-run example that currently shows the non-functional
string "echo DRY_RUN=1 ./scripts/..." so it is actually executable; replace that
example with a runnable form by instructing readers to prefix the script
invocation with the environment variable (e.g. set DRY_RUN=1 for the single
command) or use the env utility to run the script with DRY_RUN=1, and keep the
explanatory text about printing docker/podman commands intact.
- Around line 7-12: Remove the environment-dependent timing estimates from the
docs: delete “~2min” after the Docker/Podman build lines and “~20s” after the
rust:1.88-slim fallback, or replace them with a reproducible measurement command
if you want to keep timings; update the text around the build commands (`docker
build -f Dockerfile.dev -t zeptodev .`, `podman build -f Dockerfile.dev -t
localhost/zeptodev .`) and the `rust:1.88-slim` fallback so no absolute
durations are asserted without a repo command to reproduce them.
In `@scripts/container-base.sh`:
- Line 46: The script currently calls container_run "$@" unconditionally,
causing it to run when the file is sourced; wrap that invocation in a "only when
executed, not sourced" guard (use the standard BASH_SOURCE vs $0 check) so
container_run is only invoked when the script is run directly; locate the
container_run "$@" call and replace it with a conditional that checks whether
the script is the main script before calling container_run.
- Around line 30-34: The sccache mount uses a literal tilde so the host path
won't expand; update the assignment to sccache_mount inside the if block to use
an expanded home variable (e.g., $HOME or ${HOME}) instead of "~" so the mount
becomes something like -v "$HOME/.cache/sccache:/root/.cache/sccache:rw"; keep
the sccache_mount variable name and the existing conditional (if [[ "${1:-}" ==
"--sccache" ]]) but replace the quoted "~/.cache/sccache" with an expanded $HOME
path and preserve proper quoting around the entire -v value.
In `@scripts/lint-container.sh`:
- Around line 12-15: The Dockerfile path is fragile because ../Dockerfile.dev
assumes current working directory is scripts/; update the script to compute the
script's directory (using the script source/directory variable) and use a
script-relative path when invoking $RUNTIME build (replace the current -f
../Dockerfile.dev usage) so IMAGE_TAG build always points to the repo Dockerfile
regardless of how the script is invoked; keep references to RUNTIME and
IMAGE_TAG unchanged.
In `@scripts/test-container.sh`:
- Around line 16-18: The "all" case calls cargo nextest without installing it,
causing failures in fresh containers; modify the "all" branch so it performs the
same cargo-nextest installation step used by the "lib" branch before invoking
tests—i.e., add the nextest installation command (or call the same installer
function) prior to container_run "cargo nextest run --lib && cargo test" so
cargo-nextest is guaranteed to be present.
- Around line 6-7: The script sets MODE="${1:-all}" then unconditionally calls
shift which fails under set -e when no arguments are passed; update the logic
around the MODE assignment and the shift call so you only invoke shift when
there was an actual positional argument (e.g. check the positional parameter
count or test "$#" > 0 before calling shift), keeping the variable name MODE and
the existing default behavior unchanged.
In `@src/config/mod.rs`:
- Around line 1168-1172: The env-var handling for ZEPTOCLAW_SKILLS_GITHUB_TOKEN
should treat an explicitly empty value as clearing the configured token; change
the block that currently only sets self.skills.github_token = Some(val) when
non-empty so that when std::env::var("ZEPTOCLAW_SKILLS_GITHUB_TOKEN") is Ok(val)
you set self.skills.github_token = None if val.trim().is_empty(), otherwise set
self.skills.github_token = Some(val.trim().to_string()); update the code around
self.skills.github_token to follow the same empty-string-to-None normalization
used by the other token overrides.
---
Nitpick comments:
In `@scripts/container-base.sh`:
- Around line 36-44: The container_run function currently always passes -it to
$RUNTIME run which forces TTY allocation and breaks in non-interactive CI;
change the invocation to conditionally include -i/-t (or neither) based on a
check (e.g., if [ -t 1 ] or an env var like CONTAINER_TTY) so that the $RUNTIME
run call omits -it in non-TTY environments; update the $RUNTIME run command
construction in container_run to add the flags only when the check/env indicates
an interactive terminal.
In `@src/skills/github_source.rs`:
- Around line 208-211: The code assumes repo.full_name always splits into
owner/repo which can panic; update the mapping in the iterator over
search_response.items to defensively parse full_name (use splitn(2, '/') or
equivalent), check that you get exactly two parts before indexing, and handle
malformed values by logging or skipping that item instead of indexing
owner_repo[0]/owner_repo[1]; apply this change in the closure that calls
check_skill_md_exists so only valid owner/repo pairs are passed to
check_skill_md_exists.
- Around line 206-234: The GitHub search request currently ignores github_token;
modify the code that performs the search (the call that produces
search_response) to include the Authorization header when github_token.is_some()
so the search call is authenticated; use the existing github_token value (e.g.,
"token <token>" or "Bearer <token>" as appropriate for your HTTP client) so the
search API benefits from authenticated rate limits and matches the subsequent
check_skill_md_exists usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ba332ce-21b2-45ce-89b1-b7c45cd0afd1
📒 Files selected for processing (15)
.dockerignore.github/workflows/sync.yml.gitignoreCargo.tomlDockerfile.devdocs/scripts.mdscripts/artifacts/clippy.tomlscripts/bench-container.shscripts/container-base.shscripts/lint-container.shscripts/test-container.shsrc/cli/skills.rssrc/config/mod.rssrc/config/types.rssrc/skills/github_source.rs
Comment on lines
+282
to
+292
| [lints.clippy] | ||
| # Suppress pre-#187 chore lints | ||
| unnecessary_unwrap = "allow" | ||
| uninlined_format_args = "allow" | ||
| field_reassign_with_default = "allow" | ||
| default_constructed_unit_structs = "allow" | ||
| assertions_on_constants = "allow" | ||
| unnecessary_get_then_check = "allow" | ||
|
|
||
| [lints.rust] | ||
| deprecated = "allow" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and inspect Cargo.toml around lines 282-292
wc -l Cargo.tomlRepository: qhkm/zeptoclaw
Length of output: 72
🏁 Script executed:
# Look at the relevant section in Cargo.toml
sed -n '280,295p' Cargo.tomlRepository: qhkm/zeptoclaw
Length of output: 397
🏁 Script executed:
# Check for any existing lint configuration in lib.rs
fd "lib.rs" --type f | head -5 | xargs -I {} grep -l "lints\|allow" {} 2>/dev/nullRepository: qhkm/zeptoclaw
Length of output: 42
🏁 Script executed:
# Search for existing clippy allow directives throughout the codebase
rg "#\[allow\(" --type rs -c | head -20Repository: qhkm/zeptoclaw
Length of output: 84
🏁 Script executed:
# Look for unwrap() usage in the codebase
rg "\.unwrap\(" --type rs -c | head -10Repository: qhkm/zeptoclaw
Length of output: 84
🏁 Script executed:
# Find lib.rs files in the codebase
fd "lib\.rs"Repository: qhkm/zeptoclaw
Length of output: 68
🏁 Script executed:
# Search for allow directives in the codebase with correct rg syntax
rg "#\[allow\(" --type rust | head -20Repository: qhkm/zeptoclaw
Length of output: 1006
🏁 Script executed:
# Check for unwrap() calls in the codebase
rg "\.unwrap\(\)" --type rust | wc -lRepository: qhkm/zeptoclaw
Length of output: 62
🏁 Script executed:
# Check for deprecated usage in the codebase
rg "deprecated" --type rust | head -10Repository: qhkm/zeptoclaw
Length of output: 253
🏁 Script executed:
# Look for any comments in the PR/commit explaining why these lints need suppression
rg "pre-#187\|#187\|suppress.*lint" --type rust --type toml | head -20Repository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Check the entire lib.rs file for lint configuration
cat src/lib.rs | head -100Repository: qhkm/zeptoclaw
Length of output: 3507
🏁 Script executed:
# Look for any existing [lints] configuration in lib.rs
rg "^\[lints\]|#!\[warn\|#!\[forbid\|#!\[allow" src/lib.rsRepository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Check git history to understand the context of these lint suppressions
git log --oneline --all | grep -i "187\|lint\|suppress" | head -10Repository: qhkm/zeptoclaw
Length of output: 139
Move blanket lint suppressions from Cargo.toml to lib.rs with explicit justification.
Package-level [lints.clippy] and [lints.rust] sections suppress warnings across the entire crate, including unnecessary_unwrap and deprecated, which weakens the cargo clippy -- -D warnings enforcement. The codebase has 2,819 unwrap() calls representing real technical debt that should be fixed incrementally, not silenced globally.
Per coding guidelines, localize unavoidable suppressions to affected code paths using #[allow(...)] attributes, or move truly crate-wide exceptions into lib.rs with clear justification—not in Cargo.toml as blanket applies. This allows cargo clippy -- -D warnings to catch new issues while acknowledging pre-existing debt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 282 - 292, The Cargo.toml currently uses crate-wide
suppression sections ([lints.clippy] and [lints.rust]) that blanket-ignore rules
like unnecessary_unwrap and deprecated; move these suppressions out of
Cargo.toml and into code with explicit justification by removing the
[lints.clippy] and [lints.rust] entries and instead applying targeted
#[allow(...)] attributes only where necessary or, if a genuine crate-wide
exception is required, add a single commented crate-level allowance in lib.rs
(with a clear rationale and TODO) referencing the specific lint names (e.g.,
unnecessary_unwrap, deprecated) so clippy can still be run with -D warnings and
only pre-approved exceptions are silenced.
qhkm
requested changes
Mar 9, 2026
Owner
qhkm
left a comment
qhkm
left a comment
There was a problem hiding this comment.
Thanks for the contribution @taqtiqa-mark! The containerized dev tooling is a welcome addition.
Before we can merge, please address the CodeRabbit findings — most are legitimate bugs:
Must fix
-
.github/workflows/sync.yml— Cron*/180 * * * *is invalid (minute field max is 59). This runs every hour, not every 3h. Use0 */3 * * *. -
scripts/container-base.sh:34— Tilde~doesn't expand inside double quotes. The sccache mount path will be literal~, not$HOME. Replace with$HOME. -
scripts/container-base.sh:46—container_run "$@"executes unconditionally when sourced by other scripts. Add aBASH_SOURCEguard. -
scripts/test-container.sh:7—shiftwith no args underset -ecrashes the script. Useshift || trueor guard with$# -gt 0. -
scripts/test-container.sh:18—allmode callscargo nextestwithout installing it first (unlikelibmode). -
scripts/lint-container.sh:15—../Dockerfile.devbreaks when run from repo root. Use script-relative path viaSCRIPT_DIR. -
Dockerfile.dev— Runs as root (Trivy DS-0002), missing--no-install-recommends(Trivy DS-0029), andcargo-nextestnot pinned to a version. -
src/config/mod.rs:1172— EmptyZEPTOCLAW_SKILLS_GITHUB_TOKEN=""should clear the token (normalize toNone), matching other token overrides in the module.
Should fix
-
Cargo.toml[lints.clippy]/[lints.rust]— Blanket suppression ofunnecessary_unwrapanddeprecatedweakens ourcargo clippy -- -D warningsCI gate. These should be localized#[allow(...)]on specific code paths, not crate-wide. Please remove this section. -
docs/scripts.md— Missingbashlanguage tags on code fences, broken dry-run example (echo DRY_RUN=1doesn't execute the script), and environment-dependent timing estimates.
Suggestion
This PR bundles 4 separate concerns (containerized devtools, sync workflow, lint config, skill search feature). Consider splitting the skill search github_token feature (closes #285) into its own PR — it'll be easier to review and merge independently.
This was referenced Mar 10, 2026
taqtiqa-mark
force-pushed
the
feature/chore-dev-tools
branch
from
84b51c9 to
de80685
Compare
taqtiqa-mark
force-pushed
the
feature/chore-dev-tools
branch
from
de80685 to
220a6aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs/scripts.md (1)
58-64: Wrap pre-push workflow commands in a code block.Per coding guidelines, include runnable examples for new workflows. The commands on lines 60-62 should be in a fenced code block with a language tag.
♻️ Proposed fix
## Pre-Push Workflow (Recommended) -./scripts/lint-container.sh -./scripts/test-container.sh lib -./scripts/bench-container.sh +```bash +./scripts/lint-container.sh +./scripts/test-container.sh lib +./scripts/bench-container.sh +``` Add to .git/hooks/pre-push for auto.As per coding guidelines: "When adding new commands or workflows, include a runnable example in documentation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 58 - 64, Wrap the three workflow commands (./scripts/lint-container.sh, ./scripts/test-container.sh lib, ./scripts/bench-container.sh) under the "Pre-Push Workflow (Recommended)" heading in a fenced code block with a language tag (bash) so readers get a runnable example; ensure you add the opening fence with "```bash" before the first command and the closing "```" after the last command, leaving the explanatory line "Add to .git/hooks/pre-push for auto." outside the code block.scripts/container-base.sh (1)
38-43: Quote mount variables to prevent word splitting.The unquoted
$TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mounton line 41 could cause issues if any variable is empty or contains spaces. While currently safe, quoting prevents future regressions.♻️ Proposed fix using array expansion
container_run() { local cmd=("$@") + local mounts=() + [[ -n "$TARGET_MOUNT" ]] && mounts+=($TARGET_MOUNT) + [[ -n "$REGISTRY_MOUNT" ]] && mounts+=($REGISTRY_MOUNT) + [[ -n "$BENCH_MOUNT" ]] && mounts+=($BENCH_MOUNT) + [[ -n "$sccache_mount" ]] && mounts+=($sccache_mount) $RUNTIME run --rm -it \ -w "$WORKDIR" \ -v "$(pwd)":/src:rw \ - $TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mount \ + "${mounts[@]}" \ $IMAGE \ bash -c "${cmd[*]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` around lines 38 - 43, The run command is passing unquoted mount variables ($TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mount) which can cause word-splitting when variables are empty or contain spaces; update the invocation to either expand a prepared array (e.g. a MOUNTS array and use "${MOUNTS[@]}") or quote each variable in place ("$TARGET_MOUNT" "$REGISTRY_MOUNT" "$BENCH_MOUNT" "$sccache_mount") so the values are preserved, and keep the existing "${cmd[*]}" (or switch to "${cmd[@]}" if splitting behavior is intended) when calling RUNTIME run.Dockerfile.dev (1)
4-4: Add--no-install-recommendsto reduce image size.The
apt-get installis missing the--no-install-recommendsflag, which pulls in unnecessary suggested packages and bloats the image.♻️ Proposed fix
-RUN apt-get update && apt-get install -y pkg-config libssl-dev protobuf-compiler && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends pkg-config libssl-dev protobuf-compiler && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.dev` at line 4, The RUN apt-get install invocation in the Dockerfile.dev currently omits --no-install-recommends causing extra packages to be pulled; update the RUN line that calls apt-get update && apt-get install -y pkg-config libssl-dev protobuf-compiler && rm -rf /var/lib/apt/lists/* (the RUN command shown in the diff) to include --no-install-recommends immediately after install -y so the image size is reduced while preserving the same packages and the final rm -rf remains in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.dev`:
- Line 1: Dockerfile.dev is pinned to rust:1.93.0-slim while
scripts/container-base.sh and docs/scripts.md reference rust:1.88-slim; update
them to use the same toolchain or align Dockerfile.dev to the fallback. Pick a
single Rust version (e.g., 1.93.0) and update the rust tag in
scripts/container-base.sh and docs/scripts.md to match, or change Dockerfile.dev
to rust:1.88-slim so all three references (Dockerfile.dev,
scripts/container-base.sh, docs/scripts.md) use the identical
rust:<version>-slim tag.
In `@scripts/test-container.sh`:
- Line 4: The problem is that sourcing container-base.sh causes it to
unconditionally call container_run "$@"; modify container-base.sh so the final
call to container_run is only executed when the file is run directly, not when
sourced — wrap the container_run "$@" invocation in a guard that checks
BASH_SOURCE (e.g., only call container_run when "${BASH_SOURCE[0]}" == "$0") so
sourcing from scripts like test-container.sh will not trigger premature
execution; keep the function name container_run and its behavior unchanged.
---
Nitpick comments:
In `@Dockerfile.dev`:
- Line 4: The RUN apt-get install invocation in the Dockerfile.dev currently
omits --no-install-recommends causing extra packages to be pulled; update the
RUN line that calls apt-get update && apt-get install -y pkg-config libssl-dev
protobuf-compiler && rm -rf /var/lib/apt/lists/* (the RUN command shown in the
diff) to include --no-install-recommends immediately after install -y so the
image size is reduced while preserving the same packages and the final rm -rf
remains in place.
In `@docs/scripts.md`:
- Around line 58-64: Wrap the three workflow commands
(./scripts/lint-container.sh, ./scripts/test-container.sh lib,
./scripts/bench-container.sh) under the "Pre-Push Workflow (Recommended)"
heading in a fenced code block with a language tag (bash) so readers get a
runnable example; ensure you add the opening fence with "```bash" before the
first command and the closing "```" after the last command, leaving the
explanatory line "Add to .git/hooks/pre-push for auto." outside the code block.
In `@scripts/container-base.sh`:
- Around line 38-43: The run command is passing unquoted mount variables
($TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mount) which can cause
word-splitting when variables are empty or contain spaces; update the invocation
to either expand a prepared array (e.g. a MOUNTS array and use "${MOUNTS[@]}")
or quote each variable in place ("$TARGET_MOUNT" "$REGISTRY_MOUNT"
"$BENCH_MOUNT" "$sccache_mount") so the values are preserved, and keep the
existing "${cmd[*]}" (or switch to "${cmd[@]}" if splitting behavior is
intended) when calling RUNTIME run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6f4123f-2b60-4e80-b76f-79ef2c55f85c
📒 Files selected for processing (10)
.dockerignore.gitignoreCargo.tomlDockerfile.devdocs/scripts.mdscripts/artifacts/clippy.tomlscripts/bench-container.shscripts/container-base.shscripts/lint-container.shscripts/test-container.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/artifacts/clippy.toml
- scripts/bench-container.sh
- .gitignore
- .dockerignore
- scripts/lint-container.sh
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
.dockerignore (1)
21-21:⚠️ Potential issue | 🟡 MinorRemove the no-op
!scripts/artifacts/negation.Nothing above ignores
scripts/, so this pattern never changes the Docker build context. It just makes the include/exclude rules harder to reason about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dockerignore at line 21, The .dockerignore contains a no-op negation pattern "!scripts/artifacts/" which has no effect because nothing earlier ignores scripts/; remove the line "!scripts/artifacts/" from .dockerignore to simplify include/exclude rules and avoid confusion (locate and delete the exact pattern string "!scripts/artifacts/").scripts/test-container.sh (2)
6-7:⚠️ Potential issue | 🟠 MajorOnly
shiftwhen a mode argument was actually provided.With no CLI args,
MODEfalls back toallbutshiftstill exits non-zero underset -e, so the default path never runs.Suggested fix
-MODE="${1:-all}" -shift +if [[ $# -gt 0 ]]; then + MODE="$1" + shift +else + MODE="all" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-container.sh` around lines 6 - 7, The script sets MODE="${1:-all}" then always runs shift which fails under set -e when no args are passed; change the logic so shift is only executed when an explicit argument was provided (check the positional count or whether $1 is non-empty) — e.g., guard the shift with a conditional like testing "$#" -gt 0 or "[ -n \"$1\" ]" before calling shift so MODE defaults to "all" without causing an exit; update the area around the MODE assignment and the standalone shift accordingly.
16-17:⚠️ Potential issue | 🟠 MajorInstall
cargo-nextestinallmode too.The fallback image path documented in this PR does not preinstall
cargo-nextest.allgoes straight tocargo nextest, so fresh fallback runs fail whilelibmode succeeds.Suggested fix
all) - container_run "cargo nextest run --lib && cargo test" "$@" + container_run "cargo install cargo-nextest --locked || true && cargo nextest run --lib && cargo test" "$@" ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-container.sh` around lines 16 - 17, The "all" case in the scripts/test-container.sh switch runs container_run "cargo nextest run --lib && cargo test" but the fallback image doesn't have cargo-nextest installed; modify the "all" branch so it either installs cargo-nextest before invoking it or reuses the same preinstall step used by the "lib" path (i.e., ensure the container_run invocation for the "all" case calls the installation of cargo-nextest or the shared setup function that installs it before running cargo nextest and cargo test).Dockerfile.dev (2)
15-17:⚠️ Potential issue | 🟠 MajorRun the dev image as a non-root user.
scripts/container-base.shmounts the host checkout into/src:rw, so leaving this image on the default root user will create root-owned artifacts in contributors' worktrees on Linux.Suggested fix
+ARG UID=1000 +ARG GID=1000 +RUN groupadd -g "${GID}" dev && useradd -m -u "${UID}" -g "${GID}" -s /bin/bash dev + WORKDIR /src +USER dev CMD ["/bin/bash"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.dev` around lines 15 - 17, Add a non-root user and switch to it so mounted files aren’t created as root: create a user (e.g., "dev" or "appuser") in the Dockerfile before the WORKDIR/CMD lines, set its home, chown /src to that user, and then add USER dev (or USER appuser) so the container runs unprivileged; update lines around WORKDIR and CMD to ensure the new user owns /src and is the active user at runtime.
9-10:⚠️ Potential issue | 🟠 MajorPin
cargo-nextestto a specific release.
cargo install --lockedonly locks that release's dependency graph; it still fetches whatevercargo-nextestversion is current when the image is built. That makes this “consistent pre-PR state” drift over time.Suggested fix
-# nextest latest -RUN cargo install cargo-nextest --locked +# nextest +RUN cargo install cargo-nextest --version <pinned-version> --locked🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.dev` around lines 9 - 10, The Dockerfile currently installs cargo-nextest without pinning a release via the RUN command (RUN cargo install cargo-nextest --locked), which causes the installed version to drift; update that RUN invocation to pin to a specific released version by adding the cargo install --version <RELEASE> (or --version x.y.z) flag alongside --locked (e.g., RUN cargo install cargo-nextest --version <RELEASE> --locked) so builds are reproducible and deterministic.scripts/container-base.sh (3)
46-46:⚠️ Potential issue | 🔴 CriticalDon't auto-run when this file is sourced.
The other container scripts source this file for
container_run(). Executingcontainer_run "$@"unconditionally here triggers a container before the caller's own argument parsing and mode selection runs.Suggested fix
-container_run "$@" +if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then + container_run "$@" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` at line 46, The script unconditionally calls container_run "$@" even when sourced; guard that call so it only runs when the file is executed directly (not sourced) by checking the execution context and invoking container_run "$@" only when the script is the main script; locate the unconditional call to container_run and wrap it with the standard "sourced vs executed" check (use the BASH_SOURCE vs $0 test or equivalent) so callers can source the file to access the container_run function without triggering execution.
30-34:⚠️ Potential issue | 🔴 CriticalExpand
$HOMEin the sccache mount path.
~is inside a quoted assignment here, so it stays literal instead of expanding to the caller's home directory. The optional mount will point at~/.cache/sccache, not the real cache path.Suggested fix
if [[ "${1:-}" == "--sccache" ]]; then shift - sccache_mount="-v ~/.cache/sccache:/root/.cache/sccache:rw" # Host sccache (user: curl install.sh; export RUSTC_WRAPPER=sccache) + sccache_mount="-v $HOME/.cache/sccache:/root/.cache/sccache:rw" # Host sccache (user: curl install.sh; export RUSTC_WRAPPER=sccache) fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` around lines 30 - 34, The sccache_mount assignment uses a quoted tilde so the path won't expand; update the assignment inside the if-block that sets sccache_mount so it uses the caller's expanded home directory (e.g. replace "~/.cache/sccache" with "$HOME/.cache/sccache" or ${HOME}/.cache/sccache) so sccache_mount points to the real host cache path; ensure the change is made where the sccache_mount variable is set in the if [[ "${1:-}" == "--sccache" ]] block.
9-9:⚠️ Potential issue | 🟠 MajorKeep the fallback Rust image aligned with
Dockerfile.dev.This fallback still points at
rust:1.88-slimwhileDockerfile.devusesrust:1.93.0-slim. Anyone who skips the custom image will run a different compiler/clippy/rustfmt stack, which undercuts the PR's consistency goal.#!/bin/bash rg -n 'rust:[0-9]+\.[0-9]+(\.[0-9]+)?-slim' Dockerfile.dev scripts/container-base.sh docs/scripts.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` at line 9, Update the fallback IMAGE value to match the Rust version used in Dockerfile.dev so builds without a custom image use the same toolchain; change the IMAGE default from "rust:1.88-slim" to "rust:1.93.0-slim" (the IMAGE variable assignment in scripts/container-base.sh) and re-run the provided grep check to verify both Dockerfile.dev and scripts/container-base.sh reference the same rust:<version>-slim tag.docs/scripts.md (3)
8-48:⚠️ Potential issue | 🟡 MinorAdd
bashto the fenced command blocks.markdownlint is already flagging these new fences with MD040.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 8 - 48, Update the fenced code blocks in docs/scripts.md that contain shell commands to specify the language by changing each ``` to ```bash; specifically add bash to the blocks showing the Docker/Podman build commands, the Podman machine commands, the Volumes creation commands, the sccache install/env lines, and the Usage block that contains ./scripts/test-container.sh, ./scripts/lint-container.sh and ./scripts/bench-container.sh so markdownlint MD040 is satisfied.
7-12:⚠️ Potential issue | 🟡 MinorDrop the timing estimates from the setup steps.
~2minand~20sare environment-dependent, and there isn't a repository command here that reproduces them.As per coding guidelines "Do not add performance numbers to documentation unless they are reproducible with repository commands".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 7 - 12, Remove the environment-dependent timing annotations from the "Build dev image" section: delete "~2min" in the heading/parenthetical (one-time, pre-clippy/nextest ~2min) and remove "(rustup install each run ~20s)" after "rust:1.88-slim"; keep the Docker and Podman command lines as-is (the lines containing `docker build -f Dockerfile.dev -t zeptodev .`, `podman build -f Dockerfile.dev -t localhost/zeptodev .` and the reference to `rust:1.88-slim`) so the steps remain but no longer include non-reproducible timing claims.
50-50:⚠️ Potential issue | 🟡 MinorMake the dry-run example executable.
echo DRY_RUN=1 ./scripts/...only prints the string; it never runs the script withDRY_RUN=1.Suggested fix
-**Dry-run:** Scripts print docker/podman cmd if `echo DRY_RUN=1 ./scripts/...` +**Dry-run:** Scripts print docker/podman cmd if `DRY_RUN=1 ./scripts/...`As per coding guidelines "When adding new commands or workflows, include a runnable example in documentation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` at line 50, Replace the non-runnable example string "echo DRY_RUN=1 ./scripts/..." with a runnable example that actually sets the environment variable for the script invocation, e.g. use DRY_RUN=1 ./scripts/... so the script runs with DRY_RUN=1; update the docs line describing Dry-run to show this exact command and a brief note that it sets the env var for the single command invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/scripts.md`:
- Around line 58-64: Wrap the Pre-Push Workflow commands in a fenced code block
so they are copy/pasteable; add an opening fence with language marker (e.g.,
```bash) immediately before the lines containing ./scripts/lint-container.sh,
./scripts/test-container.sh lib, ./scripts/bench-container.sh and a closing ```
after them, leaving the trailing sentence "Add to .git/hooks/pre-push for auto."
as normal text; locate these commands in docs/scripts.md (the Pre-Push Workflow
section) and apply the fence around that exact command list.
In `@scripts/container-base.sh`:
- Around line 28-43: The container_run() function currently calls the
auto-detected runtime variable RUNTIME directly, which ignores user overrides;
update the invocation inside container_run to use CONTAINER_RUNTIME (the
override-aware variable) instead of RUNTIME so the function respects
CONTAINER_RUNTIME environment overrides—replace the call to $RUNTIME run ...
with $CONTAINER_RUNTIME (or ${CONTAINER_RUNTIME}) run ... inside the
container_run() body (keeping the rest of the flags and args unchanged).
---
Duplicate comments:
In @.dockerignore:
- Line 21: The .dockerignore contains a no-op negation pattern
"!scripts/artifacts/" which has no effect because nothing earlier ignores
scripts/; remove the line "!scripts/artifacts/" from .dockerignore to simplify
include/exclude rules and avoid confusion (locate and delete the exact pattern
string "!scripts/artifacts/").
In `@Dockerfile.dev`:
- Around line 15-17: Add a non-root user and switch to it so mounted files
aren’t created as root: create a user (e.g., "dev" or "appuser") in the
Dockerfile before the WORKDIR/CMD lines, set its home, chown /src to that user,
and then add USER dev (or USER appuser) so the container runs unprivileged;
update lines around WORKDIR and CMD to ensure the new user owns /src and is the
active user at runtime.
- Around line 9-10: The Dockerfile currently installs cargo-nextest without
pinning a release via the RUN command (RUN cargo install cargo-nextest
--locked), which causes the installed version to drift; update that RUN
invocation to pin to a specific released version by adding the cargo install
--version <RELEASE> (or --version x.y.z) flag alongside --locked (e.g., RUN
cargo install cargo-nextest --version <RELEASE> --locked) so builds are
reproducible and deterministic.
In `@docs/scripts.md`:
- Around line 8-48: Update the fenced code blocks in docs/scripts.md that
contain shell commands to specify the language by changing each ``` to ```bash;
specifically add bash to the blocks showing the Docker/Podman build commands,
the Podman machine commands, the Volumes creation commands, the sccache
install/env lines, and the Usage block that contains
./scripts/test-container.sh, ./scripts/lint-container.sh and
./scripts/bench-container.sh so markdownlint MD040 is satisfied.
- Around line 7-12: Remove the environment-dependent timing annotations from the
"Build dev image" section: delete "~2min" in the heading/parenthetical
(one-time, pre-clippy/nextest ~2min) and remove "(rustup install each run ~20s)"
after "rust:1.88-slim"; keep the Docker and Podman command lines as-is (the
lines containing `docker build -f Dockerfile.dev -t zeptodev .`, `podman build
-f Dockerfile.dev -t localhost/zeptodev .` and the reference to
`rust:1.88-slim`) so the steps remain but no longer include non-reproducible
timing claims.
- Line 50: Replace the non-runnable example string "echo DRY_RUN=1
./scripts/..." with a runnable example that actually sets the environment
variable for the script invocation, e.g. use DRY_RUN=1 ./scripts/... so the
script runs with DRY_RUN=1; update the docs line describing Dry-run to show this
exact command and a brief note that it sets the env var for the single command
invocation.
In `@scripts/container-base.sh`:
- Line 46: The script unconditionally calls container_run "$@" even when
sourced; guard that call so it only runs when the file is executed directly (not
sourced) by checking the execution context and invoking container_run "$@" only
when the script is the main script; locate the unconditional call to
container_run and wrap it with the standard "sourced vs executed" check (use the
BASH_SOURCE vs $0 test or equivalent) so callers can source the file to access
the container_run function without triggering execution.
- Around line 30-34: The sccache_mount assignment uses a quoted tilde so the
path won't expand; update the assignment inside the if-block that sets
sccache_mount so it uses the caller's expanded home directory (e.g. replace
"~/.cache/sccache" with "$HOME/.cache/sccache" or ${HOME}/.cache/sccache) so
sccache_mount points to the real host cache path; ensure the change is made
where the sccache_mount variable is set in the if [[ "${1:-}" == "--sccache" ]]
block.
- Line 9: Update the fallback IMAGE value to match the Rust version used in
Dockerfile.dev so builds without a custom image use the same toolchain; change
the IMAGE default from "rust:1.88-slim" to "rust:1.93.0-slim" (the IMAGE
variable assignment in scripts/container-base.sh) and re-run the provided grep
check to verify both Dockerfile.dev and scripts/container-base.sh reference the
same rust:<version>-slim tag.
In `@scripts/test-container.sh`:
- Around line 6-7: The script sets MODE="${1:-all}" then always runs shift which
fails under set -e when no args are passed; change the logic so shift is only
executed when an explicit argument was provided (check the positional count or
whether $1 is non-empty) — e.g., guard the shift with a conditional like testing
"$#" -gt 0 or "[ -n \"$1\" ]" before calling shift so MODE defaults to "all"
without causing an exit; update the area around the MODE assignment and the
standalone shift accordingly.
- Around line 16-17: The "all" case in the scripts/test-container.sh switch runs
container_run "cargo nextest run --lib && cargo test" but the fallback image
doesn't have cargo-nextest installed; modify the "all" branch so it either
installs cargo-nextest before invoking it or reuses the same preinstall step
used by the "lib" path (i.e., ensure the container_run invocation for the "all"
case calls the installation of cargo-nextest or the shared setup function that
installs it before running cargo nextest and cargo test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c31be5ed-2b56-470a-b8c6-1ce317c710ab
📒 Files selected for processing (10)
.dockerignore.gitignoreCargo.tomlDockerfile.devdocs/scripts.mdscripts/artifacts/clippy.tomlscripts/bench-container.shscripts/container-base.shscripts/lint-container.shscripts/test-container.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/bench-container.sh
- .gitignore
- scripts/lint-container.sh
- Cargo.toml
qhkm
requested changes
Mar 11, 2026
Owner
qhkm
left a comment
qhkm
left a comment
There was a problem hiding this comment.
Review
Thanks for the contribution — containerized dev tooling for consistency is a great idea. However, there are several issues that need fixing before this can merge.
Critical
1. Cargo.toml lint suppressions — must be removed
[lints.clippy]
unnecessary_unwrap = "allow"
uninlined_format_args = "allow"
# ... etc
[lints.rust]
deprecated = "allow"This globally suppresses real clippy warnings to make the container lint pass. This hides existing issues AND prevents catching new violations. The right fix is either:
- Fix the violations (preferred)
- Use
#[allow(...)]on specific call sites
Suppressing deprecated = "allow" globally is particularly dangerous — we'd never know when dependencies deprecate APIs.
2. container-base.sh double-execution bug
The file defines container_run() as a function AND calls container_run "$@" at the bottom. Since other scripts source this file and then call container_run themselves, the function runs twice — once with the sourcing script's $@ (likely empty), then again with the intended command. Remove the trailing container_run "$@" — this is a library file meant to be sourced.
3. Rust version mismatch
Dockerfile.dev:rust:1.93.0-slimcontainer-base.shdefault IMAGE:rust:1.88-slim
These must match. Different Rust versions defeats the purpose of "consistent" environments.
Medium
4. Tilde won't expand in double quotes
sccache_mount="-v ~/.cache/sccache:/root/.cache/sccache:rw"Should be "$HOME/.cache/sccache" — tilde expansion doesn't happen inside double quotes.
5. test-container.sh reinstalls nextest every run
cargo install cargo-nextest --locked || trueIf using Dockerfile.dev (which pre-installs nextest), this is redundant. If using the fallback image, it adds ~30s per run. Should check existence first (command -v cargo-nextest || cargo install ...).
6. Missing trailing newlines — Every new file is missing a final newline (\ No newline at end of file on all added files).
Summary
The concept is solid. To move forward:
- Remove all
[lints.*]additions fromCargo.toml - Fix
container-base.shsourcing pattern (remove the trailing call) - Align Rust versions between Dockerfile.dev and container-base.sh
- Fix tilde expansion (
$HOMEinstead of~) - Add trailing newlines to all new files
This was referenced Mar 11, 2026
Contributor
Author
|
Working on this in evenings and weekends as I can. |
Owner
|
Hey no worries take your time. No rush |
This was referenced Mar 13, 2026
…d volume remount Closes qhkm#369
…less Podman - Override CARGO_HOME env to /cargo-home in container_run() to avoid root-owned /usr/local/cargo writes - Remount zeptoclaw-registry_cache volume to /cargo-home:rw,U (from /src/Cargo-registry) - Add explanatory comment for rootless Podman compatibility - No functional changes to lint-container.sh; relies on updated base script
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
scripts/container-base.sh (1)
28-39:⚠️ Potential issue | 🟠 MajorUse
CONTAINER_RUNTIMEinstead of$RUNTIMEincontainer_run().Line 22 sets
CONTAINER_RUNTIME="${CONTAINER_RUNTIME:-$RUNTIME}"to allow overrides, but line 31 invokes$RUNTIMEdirectly. This causes documented overrides (e.g.,CONTAINER_RUNTIME=docker) to be silently ignored.🔧 Suggested fix
container_run() { # Override CARGO_HOME and mount for rootless Podman (avoids /usr/local/cargo root perms) local cmd=("$@") - $RUNTIME run --userns=keep-id --rm -it \ + $CONTAINER_RUNTIME run --userns=keep-id --rm -it \ -w "$WORKDIR" \ -v "$(pwd):/src:Z" \ -v "$(pwd)/scripts/artifacts/clippy.toml:/clippy.toml:Z" \ -e CARGO_HOME=/cargo-home \ $TARGET_MOUNT $REGISTRY_MOUNT $BENCH_MOUNT $sccache_mount \ $IMAGE \ bash -c "${cmd[*]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/container-base.sh` around lines 28 - 39, The container_run() function uses the wrong variable for invoking the runtime: it calls $RUNTIME directly which ignores overrides set via CONTAINER_RUNTIME; update the invocation inside container_run (the line that currently calls $RUNTIME run ...) to use $CONTAINER_RUNTIME instead, keeping all other flags/arguments unchanged so that the CONTAINER_RUNTIME="${CONTAINER_RUNTIME:-$RUNTIME}" fallback still works and overrides like CONTAINER_RUNTIME=docker are honored.docs/scripts.md (4)
56-62:⚠️ Potential issue | 🟡 MinorFence the pre-push workflow as a runnable example.
These commands are plain text, not in a code block. Per coding guidelines: "Include a runnable example in documentation."
✏️ Suggested fix
## Pre-Push Workflow (Recommended) -./scripts/lint-container.sh -./scripts/test-container.sh lib -./scripts/bench-container.sh +```bash +./scripts/lint-container.sh +./scripts/test-container.sh lib +./scripts/bench-container.sh +``` Add to .git/hooks/pre-push for auto.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 56 - 62, Wrap the Pre-Push Workflow commands under the "Pre-Push Workflow (Recommended)" header into a fenced code block (use bash/fenced triple backticks) so the three script lines ./scripts/lint-container.sh, ./scripts/test-container.sh lib, and ./scripts/bench-container.sh become a runnable example and remain followed by the note about adding them to .git/hooks/pre-push; ensure the fence markers are added before and after the listed commands and do not alter the commands themselves.
18-22:⚠️ Potential issue | 🟡 MinorDrop the timing estimate or make it reproducible.
The
~2min firsttiming is environment-dependent and violates the guideline: "Do not add performance numbers unless they are reproducible with repository commands."✏️ Suggested fix
-**Build dev image** (~2min first): +**Build dev image** (first run builds deps):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 18 - 22, Remove the non-reproducible timing from the "Build dev image (~2min first):" heading in docs/scripts.md; either delete the "~2min first" text or replace it with a reproducible measurement method (e.g., instructions to run a timed command or include a note that build time varies by environment) so no hard performance number remains in the documentation.
10-12:⚠️ Potential issue | 🟡 MinorAdd language tags to fenced code blocks.
Multiple code blocks lack language specifiers (MD040). Mark them as
bashfor proper rendering and lint compliance.Also applies to: 19-22, 29-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` around lines 10 - 12, Fenced code blocks in docs/scripts.md (e.g., the block containing "printf '%s\n' zeptoclaw-{target,registry,benches,sccache}_cache | xargs -I {} podman volume create {}") are missing language tags which triggers MD040; update those triple-backtick blocks to include the bash language specifier (```bash) for the blocks at the shown ranges (10-12, also 19-22 and 29-46) so they render correctly and pass linting.
48-48:⚠️ Potential issue | 🟡 MinorFix the dry-run example.
echo DRY_RUN=1 ./scripts/...only prints the string; it doesn't run the script. The example should be executable as written. As per coding guidelines: "When adding new commands or workflows, include a runnable example."✏️ Suggested fix
-**Dry-run:** Scripts print docker/podman cmd if `echo DRY_RUN=1 ./scripts/...` +**Dry-run:** Scripts print docker/podman cmd with `DRY_RUN=1 ./scripts/...`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts.md` at line 48, The "Dry-run" example in docs/scripts.md is incorrect: it shows the literal string `echo DRY_RUN=1 ./scripts/...` which doesn't execute the script; replace that example with a runnable form that sets DRY_RUN as an environment variable when invoking the script (so the script runs and receives DRY_RUN), and update the "Dry-run" section text to reflect the new example instead of the echo-based one.
🧹 Nitpick comments (4)
scripts/lint-container.sh (2)
6-13: Remove unused variablesSCRIPT_DIRandTTY_FLAG.These variables are defined but never used, as flagged by Shellcheck SC2034. Either use them (e.g., pass
$TTY_FLAGto container commands) or remove them.✏️ Suggested fix (removal)
source "$(dirname "$0")/container-base.sh" -SCRIPT_DIR="$(dirname "$0")" -# Check if stdin is a TTY and set flags accordingly -TTY_FLAG="" -if [ -t 0 ] && [ -t 1 ]; then - TTY_FLAG="-it" -else - TTY_FLAG="-i" -fi - # Auto-build zeptodev if missing (docker: zeptodev, podman: localhost/zeptodev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-container.sh` around lines 6 - 13, Remove the unused shell variables by deleting the SCRIPT_DIR assignment and the TTY_FLAG detection block (the lines defining TTY_FLAG and the if/else that sets it); if you intended to use TTY_FLAG for container commands instead, wire $TTY_FLAG into the docker/run command (or another container invocation) and keep the detection logic, otherwise remove both SCRIPT_DIR and TTY_FLAG to satisfy Shellcheck SC2034.
23-27: Dockerfile path assumes repo root execution.
-f Dockerfile.devwith context.requires the script to be invoked from the repository root. Consider using a script-relative path for robustness.✏️ Suggested fix
+REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" + # Auto-build zeptodev if missing (docker: zeptodev, podman: localhost/zeptodev) IMAGE_TAG="zeptodev" if [[ "$RUNTIME" == "podman" ]]; then IMAGE_TAG="localhost/${IMAGE_TAG}:custom" if ! $RUNTIME image inspect "$IMAGE_TAG" >/dev/null 2>&1; then echo "Building $IMAGE_TAG first-run image (Dockerfile.dev)..." buildah bud \ --userns=host \ - -f Dockerfile.dev \ - -t "${IMAGE_TAG}" . + -f "$REPO_ROOT/Dockerfile.dev" \ + -t "${IMAGE_TAG}" "$REPO_ROOT" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-container.sh` around lines 23 - 27, The buildah invocation assumes repo-root execution because it uses "-f Dockerfile.dev" with context ".", so update scripts/lint-container.sh to compute the script directory (e.g. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)") and then call buildah bud with an explicit script-relative Dockerfile and context (e.g. -f "${SCRIPT_DIR}/../Dockerfile.dev" and context "${SCRIPT_DIR}/..") so the script works regardless of the current working directory.Dockerfile.dev (1)
27-38: Step numbering in comments is inconsistent.The comments use step numbers 1, 2, 3, 4, 5, then repeat 3, 4 (lines 27 and 32). This may cause confusion during maintenance.
✏️ Suggested fix
-# 3. Build dependencies (this uses the volumes mounted in the build command) +# 6. Build dependencies (this uses the volumes mounted in the build command) RUN --mount=type=cache,id=zeptoclaw-registry,target=/usr/local/cargo/registry,U \ --mount=type=cache,id=zeptoclaw-target,target=/src/target,U \ ls -ld /src/target && cargo build && rm -rf src benches -# 4. NOW copy the real source (this won't be clobbered because the dummy is gone) +# 7. NOW copy the real source (this won't be clobbered because the dummy is gone) COPY . . -# compile deps +# 8. Final build RUN --mount=type=cache,id=zeptoclaw-registry,target=/usr/local/cargo/registry,U \ --mount=type=cache,id=zeptoclaw-target,target=/src/target,U \ cargo build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.dev` around lines 27 - 38, The inline step comments in the Dockerfile are misnumbered (they repeat "3" and "4" around the RUN/COPY blocks), which is confusing; update the comments near the RUN --mount and COPY . . blocks (and any other step comments) to use a consistent sequential numbering scheme (e.g., continue from the prior steps so they become unique like 6,7 or renumber all steps 1..N), ensuring each comment above the corresponding RUN or COPY accurately reflects the correct step order and remove any duplicate numbers.scripts/test-container.sh (1)
11-17: Inconsistentcargo installargument order.Line 11 uses
cargo install cargo-nextest --lockedwhile line 17 usescargo install --locked cargo-nextest. Both work, but consistency aids maintainability.✏️ Suggested fix
all) - container_run "cargo install --locked cargo-nextest || true && cargo nextest run --lib && cargo test" "$@" + container_run "cargo install cargo-nextest --locked || true && cargo nextest run --lib && cargo test" "$@" ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-container.sh` around lines 11 - 17, Standardize the `cargo install` argument order used in the container_run invocations: replace the inconsistent usage in the `container_run` calls that install cargo-nextest so both use the same ordering (e.g., `cargo install --locked cargo-nextest`) — update the occurrences inside the `container_run` calls (the lines that currently run `cargo install cargo-nextest --locked || true` and `cargo install --locked cargo-nextest || true`) so they match the chosen canonical form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/scripts.md`:
- Around line 24-27: Remove the duplicate markdown heading by deleting or
renaming one of the repeated "## Usage" headings (the two identical "## Usage"
entries in the snippet) so MD024 no longer triggers; ensure the remaining
heading correctly reflects the section (or rename the second heading to the
intended title, e.g., "## Tests" or another unique section name) and update any
adjacent content if needed to match the new heading.
---
Duplicate comments:
In `@docs/scripts.md`:
- Around line 56-62: Wrap the Pre-Push Workflow commands under the "Pre-Push
Workflow (Recommended)" header into a fenced code block (use bash/fenced triple
backticks) so the three script lines ./scripts/lint-container.sh,
./scripts/test-container.sh lib, and ./scripts/bench-container.sh become a
runnable example and remain followed by the note about adding them to
.git/hooks/pre-push; ensure the fence markers are added before and after the
listed commands and do not alter the commands themselves.
- Around line 18-22: Remove the non-reproducible timing from the "Build dev
image (~2min first):" heading in docs/scripts.md; either delete the "~2min
first" text or replace it with a reproducible measurement method (e.g.,
instructions to run a timed command or include a note that build time varies by
environment) so no hard performance number remains in the documentation.
- Around line 10-12: Fenced code blocks in docs/scripts.md (e.g., the block
containing "printf '%s\n' zeptoclaw-{target,registry,benches,sccache}_cache |
xargs -I {} podman volume create {}") are missing language tags which triggers
MD040; update those triple-backtick blocks to include the bash language
specifier (```bash) for the blocks at the shown ranges (10-12, also 19-22 and
29-46) so they render correctly and pass linting.
- Line 48: The "Dry-run" example in docs/scripts.md is incorrect: it shows the
literal string `echo DRY_RUN=1 ./scripts/...` which doesn't execute the script;
replace that example with a runnable form that sets DRY_RUN as an environment
variable when invoking the script (so the script runs and receives DRY_RUN), and
update the "Dry-run" section text to reflect the new example instead of the
echo-based one.
In `@scripts/container-base.sh`:
- Around line 28-39: The container_run() function uses the wrong variable for
invoking the runtime: it calls $RUNTIME directly which ignores overrides set via
CONTAINER_RUNTIME; update the invocation inside container_run (the line that
currently calls $RUNTIME run ...) to use $CONTAINER_RUNTIME instead, keeping all
other flags/arguments unchanged so that the
CONTAINER_RUNTIME="${CONTAINER_RUNTIME:-$RUNTIME}" fallback still works and
overrides like CONTAINER_RUNTIME=docker are honored.
---
Nitpick comments:
In `@Dockerfile.dev`:
- Around line 27-38: The inline step comments in the Dockerfile are misnumbered
(they repeat "3" and "4" around the RUN/COPY blocks), which is confusing; update
the comments near the RUN --mount and COPY . . blocks (and any other step
comments) to use a consistent sequential numbering scheme (e.g., continue from
the prior steps so they become unique like 6,7 or renumber all steps 1..N),
ensuring each comment above the corresponding RUN or COPY accurately reflects
the correct step order and remove any duplicate numbers.
In `@scripts/lint-container.sh`:
- Around line 6-13: Remove the unused shell variables by deleting the SCRIPT_DIR
assignment and the TTY_FLAG detection block (the lines defining TTY_FLAG and the
if/else that sets it); if you intended to use TTY_FLAG for container commands
instead, wire $TTY_FLAG into the docker/run command (or another container
invocation) and keep the detection logic, otherwise remove both SCRIPT_DIR and
TTY_FLAG to satisfy Shellcheck SC2034.
- Around line 23-27: The buildah invocation assumes repo-root execution because
it uses "-f Dockerfile.dev" with context ".", so update
scripts/lint-container.sh to compute the script directory (e.g. SCRIPT_DIR="$(cd
"$(dirname "${BASH_SOURCE[0]}")" && pwd)") and then call buildah bud with an
explicit script-relative Dockerfile and context (e.g. -f
"${SCRIPT_DIR}/../Dockerfile.dev" and context "${SCRIPT_DIR}/..") so the script
works regardless of the current working directory.
In `@scripts/test-container.sh`:
- Around line 11-17: Standardize the `cargo install` argument order used in the
container_run invocations: replace the inconsistent usage in the `container_run`
calls that install cargo-nextest so both use the same ordering (e.g., `cargo
install --locked cargo-nextest`) — update the occurrences inside the
`container_run` calls (the lines that currently run `cargo install cargo-nextest
--locked || true` and `cargo install --locked cargo-nextest || true`) so they
match the chosen canonical form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 838622f7-d59b-49e1-be9f-dd5e7aa023ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Dockerfile.devdocs/scripts.mdscripts/container-base.shscripts/lint-container.shscripts/test-container.sh
taqtiqa-mark
force-pushed
the
feature/chore-dev-tools
branch
from
0ff17c0 to
8145d33
Compare
Contributor
Author
Agree this is serious. However, it just exposes the current state of the codebase -- which is fine, its pre-release. This requirement pushes an enormous amount of technical debt on to this PR. Essentially, the Cargo.toml lint suppression is the same as disregarding this PR gate:
Since that PR gate is being disregarded, the lint suppression has simply bubbled to the surface the fact that current PR gates are theater (no criticism, that is just the current state of play) I've added a This really needs the maintainer to drive rather than me asking about each discretionary choice. I also suggest that if the lint suppression blocks this PR then issue #187 and #186 are not really So I propose:
@qhkm, let me know how you want to proceed. |
This was referenced Mar 18, 2026
Contributor
Author
|
This is still in play, awaiting :
|
qhkm
added a commit
that referenced
this pull request
Mar 25, 2026
Apply changes from taqtiqa-mark's PR #287 (containerized dev tools for consistent pre-PR state). Adds container-base.sh, test/bench/fix container scripts, clippy.toml artifact, docs, and Cargo.toml lint suppressions. Lint suppressions will be addressed in a follow-up PR. Closes #287 Co-Authored-By: Mark Van de Vyver <mark@taqtiqa.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
Owner
|
Thanks @taqtiqa-mark for the work on this! We've applied your changes in #422 (rebased cleanly against current main since this had conflicts). The Closing in favor of #422. Appreciate the contribution! 🙏 |
qhkm
added a commit
that referenced
this pull request
Mar 25, 2026
## Summary - Applies changes from @taqtiqa-mark's PR #287 (containerized dev tools for consistent pre-PR state) - Adds `container-base.sh`, `test-container.sh`, `bench-container.sh`, `fix-container.sh` scripts - Adds `scripts/artifacts/clippy.toml` and `docs/scripts.md` - Includes `Cargo.toml` lint suppressions (to be reverted in follow-up PR) - Updates `.dockerignore` and `.gitignore` Closes #287 ## Note The `[lints.clippy]` and `[lints.rust]` suppressions in `Cargo.toml` will be addressed in a follow-up PR that fixes the underlying lint issues instead of suppressing them. ## Test plan - [x] `cargo clippy -- -D warnings` passes - [x] `cargo fmt -- --check` passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added container-based development scripts for running tests, benchmarks, and automatic code fixes in isolated environments * **Documentation** * Added comprehensive guide for container-based development setup, configuration, and recommended workflows * **Chores** * Updated compiler linting configuration * Refined build context and version control ignore patterns <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Mark Van de Vyver <mark@taqtiqa.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
qhkm
added a commit
that referenced
this pull request
Mar 25, 2026
The [lints.clippy] and [lints.rust] sections added in PR #287 suppress lints that are already allow-by-default in clippy. The pre-push gate (`cargo clippy -- -D warnings`) passes cleanly without them, so the suppressions are unnecessary and weaken future lint coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
qhkm
added a commit
that referenced
this pull request
Mar 25, 2026
The [lints.clippy] and [lints.rust] sections added in PR #287 suppress lints that are already allow-by-default in clippy. The pre-push gate (`cargo clippy -- -D warnings`) passes cleanly without them, so the suppressions are unnecessary and weaken future lint coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
qhkm
added a commit
that referenced
this pull request
Mar 25, 2026
## Summary - Removes `[lints.clippy]` and `[lints.rust]` sections added in #422 (from PR #287) - These suppress lints that are already allow-by-default in clippy - `cargo clippy -- -D warnings` passes cleanly without them - Gates `provider_name_for_model_with_available` with `#[cfg(test)]` — it's only used in tests, and causes dead_code errors under `--no-default-features` / `--features panel` / `--features tool-pdf` (pre-existing issue from #417) - Includes formatting fixes for unformatted code introduced by #417 ## Test plan - [x] `cargo clippy -- -D warnings` passes - [x] `cargo fmt -- --check` passes - [x] `cargo check --no-default-features` passes - [x] `cargo check --features panel` passes - [x] `cargo check --features tool-pdf` passes - [x] `cargo nextest run --lib` — 3278 passed 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Removed project-level lint overrides to rely on default linting behavior. * Restricted a previously public helper so it’s now available only in test builds (no functional behavior changes). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <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
2 participants
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.
Summary
Provides contributors a common dev/test environment.
Facilitates local test/lint checks that are consistent between disparate devs -- before PRs are generated.
These PR checklist items can now have a common basis:
cargo testpassescargo clippy -- -D warningspassescargo fmt --checkpassesRelated Issue
#187
Scope
upstream/maincargo testpassescargo clippy -- -D warningspassescargo fmt --checkpassesTest Plan
`./scripts/lint-container.sh
Summary by CodeRabbit