v0.1.7: APT Clean dry-run safety + docker test exit-code propagation#67
Conversation
Previously, Clean() executed `apt autoclean` regardless of the DryRun option, silently defeating the safety mechanism users expect from dry-run mode. The YUM/Snap/Flatpak Clean implementations on main already honored DryRun; APT now matches that behavior. Add the early-return guard immediately after nil-opts defaulting (matching the pattern at yum/yum.go:467, snap/snap.go:295, flatpak/flatpak.go:300). Move context creation below the guard so the DryRun path doesn't allocate an unused context. Regression tests cover three contracts: - Clean(DryRun=true) makes zero underlying command calls - Clean(DryRun=false) does invoke `apt autoclean` (guards against the fix being implemented as a blanket no-op) - Clean(nil) preserves the existing nil-opts default behavior This is security-relevant: a user testing with `--dry-run` on shared or production hosts could trigger cache cleanup unexpectedly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The compose test entrypoints used `bash -c` with `&&` chains and trailing `|| true` on fixture-generation steps. Due to bash operator precedence, the trailing `|| true` caught failures from earlier in the chain — including `go test` itself — letting failed tests pass CI silently while only the (allowed-to-fail) fixture-generation step appeared to succeed. Switch to `bash -ec` with explicit `;` separators for required steps: - Required commands (go test, apt update) abort the script on non-zero exit (set -e via -e flag) - Fixture-generation lines retain `|| true` so they remain individually allowed to fail - Statements use `;` instead of `&&` so set -e can actually trigger on intermediate failures (set -e doesn't apply to commands inside `&&` lists except the final command) Applied to ubuntu-apt-test, rockylinux-yum-test, almalinux-yum-test. Defense-in-depth on the test-all aggregator service: - read_only: true (it only runs an echo, no writes needed) - security_opt: [no-new-privileges:true] The required services (apt/yum tests) need write access for fixture generation and don't get these constraints. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Document the APT Clean DryRun safety fix, the docker-compose exit code propagation fix, the test-all defense-in-depth hardening, the PackageInfo JSON tags change (#40), and the go.mod toolchain version fix (#40). Also picks up pre-commit-mandated cleanup of pre-existing trailing whitespace and missing trailing newline. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces security enhancements and test reliability improvements for version 0.1.7. The APT package manager's ChangesAPT Dry-Run Security & Docker Test Hardening Release 0.1.7
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
No IssuesNo security issues were detected in the SAST scan. The code changes appear to follow secure coding practices. fossabot analyzed this PR using SAST security analysis (changed files only). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testing/docker/docker-compose.test.yml (1)
146-153: ⚡ Quick winHarden
test-allfurther by making the/workspacebind mount read-only.
test-allnow hasread_onlyroot FS andno-new-privileges, but the bind mount remains writable. Since this service only echoes status,:rowould better match the hardening goal.Suggested diff
test-all: @@ volumes: - - ../..:/workspace + - ../..:/workspace:ro🤖 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 `@testing/docker/docker-compose.test.yml` around lines 146 - 153, The bind mount for the test-all service is still writable; update the volumes entry that currently maps "../..:/workspace" to be read-only by appending ":ro" (i.e., change the volumes line in the test-all service from ../..:/workspace to ../..:/workspace:ro) so /workspace is mounted read-only while keeping the service's read_only and security_opt settings intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manager/apt/apt_clean_dryrun_test.go`:
- Around line 1-8: This test file (package apt_test in apt_clean_dryrun_test.go)
needs Go build tags for selective test execution; add a top-of-file build
constraint such as a modern //go:build line combining the desired selectors (for
example unit and apt) and the corresponding legacy // +build line, placed before
the package declaration so the test runs only when those tags are specified.
---
Nitpick comments:
In `@testing/docker/docker-compose.test.yml`:
- Around line 146-153: The bind mount for the test-all service is still
writable; update the volumes entry that currently maps "../..:/workspace" to be
read-only by appending ":ro" (i.e., change the volumes line in the test-all
service from ../..:/workspace to ../..:/workspace:ro) so /workspace is mounted
read-only while keeping the service's read_only and security_opt settings
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10870b6a-434c-460d-a473-f090514e4772
📒 Files selected for processing (4)
CHANGELOG.mdmanager/apt/apt.gomanager/apt/apt_clean_dryrun_test.gotesting/docker/docker-compose.test.yml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a security issue where the APT Clean() method ignored the DryRun option and updates Docker test runners to ensure test failures are correctly reported by using bash -ec. It also updates PackageInfo JSON fields to snake_case and hardens the Docker aggregator service. Feedback suggests improving the Docker Compose configuration by using the long form of depends_on to properly propagate failures, simplifying redundant option initializations in the APT manager, and correcting the syntax for the no-new-privileges security option.
Three small follow-ups to the test-all aggregator service from PR #67 review: - depends_on switched to long-form with `condition: service_completed_successfully` for each dependent service. The short form only waited for dependents to *start*, so test-all's `echo` could finish in milliseconds and abort the compose run (--abort-on-container-exit) before the real tests reported failure. Same class of CI-honesty bug the bash -ec change in this same release fixes one layer up. (gemini-code-assist HIGH) - /workspace bind mount made read-only (`:ro`). test-all only runs an echo, so the mount doesn't need write access. Defense-in-depth consistent with the existing read_only and security_opt. (CodeRabbit) - Quoted "no-new-privileges:true" to eliminate YAML-parser ambiguity around `key:value` parsing while preserving Docker's documented form. (gemini-code-assist SECURITY-MEDIUM) Verified via `docker compose config`: depends_on shows long-form with condition: service_completed_successfully; read_only: true on the bind mount; security_opt preserves the documented form. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Review feedback applied in 1e42e97. Three test-all hardening items accepted, two declined with reasoning on their respective threads. Accepted (commit 1e42e97):
Declined:
Note on the failing "Quality Gate" check: the SonarQube workflow has been failing on every recent PR and on |
No IssuesNo security issues were detected in the SAST scan. The code changes appear to follow secure coding practices. fossabot analyzed this PR using SAST security analysis (changed files only). |
![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: 1e42e97549
ℹ️ 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".
…ow tests
`--abort-on-container-exit` aborts the compose run when ANY container
exits — success or failure. With multi-service runs (`make test-docker`,
`make test-docker-all`), the FASTEST test container's success would
kill the slower siblings mid-test, masking failures and skipping most
of the matrix. Long-form depends_on on `test-all` from the prior
commit only addressed the test-all → dependents race; the test
containers race against each other was still there.
Switch to `--abort-on-container-failure` (Docker Compose v2.12+) which
aborts only when a container exits non-zero. Now multi-service runs
wait for all test containers to finish, fail fast on actual failure,
and surface real CI signal.
Applied uniformly across test-docker, test-docker-{ubuntu,rocky,alma},
test-fixtures, and the commented-out future targets.
Catch from chatgpt-codex-connector P1 review on PR #67.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesNo security issues were detected in the SAST scan. The code changes appear to follow secure coding practices. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5594581084
ℹ️ 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".
Docker Compose v1 (`docker-compose`, the standalone Python binary) was EOL'd June 2023 and does not support `--abort-on-container-failure` introduced in the prior commit. With the Makefile invoking the legacy hyphenated form, environments still using v1 would have all the `make test-docker*` targets fail before any tests run. Switch all 8 invocations from `docker-compose ...` to `docker compose ...` (v2 plugin / standalone v2 binary). Filenames containing `docker-compose` (e.g. `docker-compose.test.yml`) are unchanged. Verified locally: `make test-docker-ubuntu` exits 0 with the v2 command. Catch from chatgpt-codex-connector P2 review on PR #67. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No IssuesNo security issues were detected in the SAST scan. The code changes appear to follow secure coding practices. fossabot analyzed this PR using SAST security analysis (changed files only). |
1 participant
Summary
Three atomic commits prepping a v0.1.7 hotfix release:
fix(apt): honor DryRun in Clean()— APT'sClean()was runningapt autocleanregardless of theDryRunoption, defeating the safety guarantee users expect from dry-run mode. The YUM/Snap/Flatpak Clean implementations onmainalready honored DryRun; APT now matches. Includes 3 regression tests with a TDD red-green-restore cycle verified locally.ci(docker): make test failures propagate + harden test-all service— The compose entrypoints usedbash -cwith&&chains and trailing|| true. Due to bash operator precedence, the|| truecaught failures from earlier in the chain (includinggo test), letting failed tests pass CI silently. Switched tobash -ecwith;separators so test failures abort immediately while fixture-generation steps remain individually allowed to fail. Addedread_only: trueandno-new-privileges:trueto thetest-allaggregator service.docs(changelog): add v0.1.7 entry— Documents all of the above plus the @aijanai contribution merged in add json tags to PackageInfo struct and tests #40.Security relevance
The DryRun fix is security-relevant: a user testing destructive operations with
--dry-runon shared / production hosts could trigger cache cleanup unexpectedly. The CI exit-code fix is also security-adjacent: tests can no longer pass silently when they should fail.Test plan
TestCleanRespectsDryRunFAILs without fix, PASSes with fix)go build ./...exit 0go vet ./...exit 0gofmt -l ./cleango test ./manager/apt/— 3 new tests PASS; pre-existing env-coupled FAILs identical to baseline (no regressions)make test-docker-ubuntuexit 0 — confirms fix works on real Ubuntu APT, plus new bash -ec format is functionalmake test-docker-rockyexit 0 — confirms new bash -ec format works for YUMdocker compose -f testing/docker/docker-compose.test.yml configparses cleanlyv0.1.7Notes
+from the worktree tooling; rename if you'd like before merge.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation