Skip to content

Commit d8b97fe

Browse files
bluetclaude
andauthored
v0.1.7: APT Clean dry-run safety + docker test exit-code propagation (#67)
* fix(apt): honor DryRun in Clean() to prevent silent destructive ops 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> * ci(docker): make test failures propagate + harden test-all service 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> * docs(changelog): add v0.1.7 entry 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> * ci(docker): apply review feedback on test-all hardening 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> * ci(docker): switch to --abort-on-container-failure to stop killing slow 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> * ci(docker): use Compose v2 syntax (`docker compose`) for new flag 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> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 738e5ab commit d8b97fe

5 files changed

Lines changed: 152 additions & 39 deletions

File tree

CHANGELOG.md

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
## [0.1.7] - 2026-05-10
11+
12+
### Security
13+
- **APT `Clean()` now respects `DryRun`**. Previously, `apt autoclean` executed regardless of the `DryRun` option, silently defeating the safety mechanism users expect from dry-run mode. The YUM/Snap/Flatpak Clean implementations already honored DryRun; APT now matches that behavior. Regression tests added in `manager/apt/apt_clean_dryrun_test.go`.
14+
- **Docker test runners no longer mask test failures**. The compose entrypoints used `bash -c` with `&&` chains and trailing `|| true` on fixture-generation steps; due to bash operator precedence, the `|| true` caught failures from earlier in the chain (including `go test`), letting failed tests pass CI silently. Switched to `bash -ec` with explicit `;` separators so test failures abort immediately while fixture-generation steps remain individually allowed to fail.
15+
- Added `read_only: true` and `no-new-privileges:true` to the `test-all` aggregator service for defense-in-depth.
16+
17+
### Changed
18+
- **`PackageInfo` JSON output now uses snake_case field names** (e.g. `package_manager`, `new_version`, `additional_data`) instead of Go's default PascalCase. Required fields (`name`, `status`, `package_manager`) are always emitted; optional fields use `omitempty`. Consumers parsing JSON output must update field names. (#40, thanks @aijanai)
19+
20+
### Fixed
21+
- `go.mod` now declares `go 1.23.0` (full version) instead of `go 1.23`, resolving Go toolchain download failures in some environments. (#40)
22+
1023
## [0.1.6] - 2025-11-01
1124

1225
### Added
@@ -37,14 +50,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3750
- Technical debt cleanup and APT Upgrade method fix
3851
- APT Upgrade method now correctly uses `apt install` for specific packages
3952

40-
## Recent Achievements ✅
53+
## Recent Achievements ✅
4154

4255
### Architecture & Code Quality
4356
-**CommandRunner Architecture**: Complete architectural consistency (Issue #20, PR #26)
4457
-**APT & YUM executeCommand Pattern**: Centralized command execution, eliminated code duplication
4558
-**Technical Debt Cleanup**: Fixed APT Upgrade method bug, removed misleading TODOs, verified no resource leaks
4659

47-
### Security Enhancements
60+
### Security Enhancements
4861
-**Security Enhancements**: Input validation for package names (Issue #23, PR #25)
4962
-**Command Injection Prevention**: Comprehensive ValidatePackageName implementation across all package managers
5063

@@ -77,7 +90,7 @@ Current development focus areas (see [GitHub Issues](https://github.com/bluet/sy
7790
- **Security scanning with Snyk** - Add to CI/CD pipeline
7891
- **CommandRunner migration** - Complete Snap and Flatpak integration (Issues #28, #29)
7992

80-
### Medium Priority Pending
93+
### Medium Priority Pending
8194
- **Test coverage improvements** - YUM gaps (Issue #32), Snap & Flatpak comprehensive suites
8295
- **CLI improvements** - Upgrade display (Issue #3), macOS apt conflict (Issue #2)
8396
- **Code quality** - Context support, custom error types, DRY principle improvements
@@ -90,7 +103,7 @@ Current development focus areas (see [GitHub Issues](https://github.com/bluet/sy
90103

91104
### Currently Supported ✅
92105
- **APT** (Ubuntu/Debian) - Full feature support
93-
- **YUM** (Rocky Linux/AlmaLinux/RHEL) - Full feature support
106+
- **YUM** (Rocky Linux/AlmaLinux/RHEL) - Full feature support
94107
- **Snap** (Universal packages) - Full feature support
95108
- **Flatpak** (Universal packages) - Full feature support
96109

@@ -101,4 +114,4 @@ Current development focus areas (see [GitHub Issues](https://github.com/bluet/sy
101114
### Planned 📋
102115
- **Homebrew** (macOS) - Planned for cross-platform expansion
103116
- **Chocolatey/Scoop/winget** (Windows) - Planned for Windows support
104-
- **Zypper** (openSUSE) - Lower priority
117+
- **Zypper** (openSUSE) - Lower priority

Makefile

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,43 +88,43 @@ install-tools:
8888
# Docker testing targets
8989
test-docker:
9090
@echo "Running tests in Docker containers..."
91-
docker-compose -f testing/docker/docker-compose.test.yml up --abort-on-container-exit --remove-orphans
91+
docker compose -f testing/docker/docker-compose.test.yml up --abort-on-container-failure --remove-orphans
9292

9393
test-docker-ubuntu:
9494
@echo "Running Ubuntu APT tests..."
95-
docker-compose -f testing/docker/docker-compose.test.yml up ubuntu-apt-test --abort-on-container-exit
95+
docker compose -f testing/docker/docker-compose.test.yml up ubuntu-apt-test --abort-on-container-failure
9696

9797
test-docker-rocky:
9898
@echo "Running Rocky Linux YUM tests..."
99-
docker-compose -f testing/docker/docker-compose.test.yml up rockylinux-yum-test --abort-on-container-exit
99+
docker compose -f testing/docker/docker-compose.test.yml up rockylinux-yum-test --abort-on-container-failure
100100

101101
test-docker-alma:
102102
@echo "Running AlmaLinux YUM tests..."
103-
docker-compose -f testing/docker/docker-compose.test.yml up almalinux-yum-test --abort-on-container-exit
103+
docker compose -f testing/docker/docker-compose.test.yml up almalinux-yum-test --abort-on-container-failure
104104

105105
# TODO: Enable when DNF support is implemented
106106
# test-docker-fedora:
107107
# @echo "Running Fedora DNF tests..."
108-
# docker-compose -f testing/docker/docker-compose.test.yml up fedora-dnf-test --abort-on-container-exit
108+
# docker compose -f testing/docker/docker-compose.test.yml up fedora-dnf-test --abort-on-container-failure
109109

110110
# TODO: Enable when APK support is implemented
111111
# test-docker-alpine:
112112
# @echo "Running Alpine APK tests..."
113-
# docker-compose -f testing/docker/docker-compose.test.yml up alpine-apk-test --abort-on-container-exit
113+
# docker compose -f testing/docker/docker-compose.test.yml up alpine-apk-test --abort-on-container-failure
114114

115115
test-docker-all: test-docker
116116

117117
# Generate test fixtures from different OS
118118
test-fixtures:
119119
@echo "Generating test fixtures from multiple OS..."
120120
@mkdir -p testing/fixtures/{apt,yum,dnf,apk}
121-
docker-compose -f testing/docker/docker-compose.test.yml up --abort-on-container-exit
121+
docker compose -f testing/docker/docker-compose.test.yml up --abort-on-container-failure
122122
@echo "Test fixtures generated in testing/fixtures/"
123123

124124
# Clean up Docker resources
125125
test-docker-clean:
126126
@echo "Cleaning up Docker test resources..."
127-
docker-compose -f testing/docker/docker-compose.test.yml down --volumes --remove-orphans
127+
docker compose -f testing/docker/docker-compose.test.yml down --volumes --remove-orphans
128128
docker system prune -f --filter "label=com.docker.compose.project=syspkg-test"
129129

130130
# Unit tests only (no integration/system tests)

manager/apt/apt.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -378,16 +378,23 @@ func (a *PackageManager) UpgradeAll(opts *manager.Options) ([]manager.PackageInf
378378

379379
// Clean cleans the local package cache used by the apt package manager.
380380
func (a *PackageManager) Clean(opts *manager.Options) error {
381-
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
382-
defer cancel()
383-
384381
if opts == nil {
385382
opts = &manager.Options{
386383
DryRun: false,
387384
Interactive: false,
388385
Verbose: false,
389386
}
390387
}
388+
389+
// Handle dry run mode
390+
if opts.DryRun {
391+
log.Println("Dry run mode: would execute 'apt autoclean'")
392+
return nil
393+
}
394+
395+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
396+
defer cancel()
397+
391398
args := []string{"autoclean"}
392399
out, err := a.executeCommand(ctx, args, opts)
393400
if err != nil {
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package apt_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/bluet/syspkg/manager"
7+
"github.com/bluet/syspkg/manager/apt"
8+
)
9+
10+
// TestCleanRespectsDryRun is the regression test for the security-relevant bug
11+
// where Clean() executed `apt autoclean` even when opts.DryRun was true.
12+
// Behavior contract: Clean(DryRun=true) MUST NOT execute any underlying command.
13+
func TestCleanRespectsDryRun(t *testing.T) {
14+
mockRunner := manager.NewMockCommandRunner()
15+
pm := apt.NewPackageManagerWithCustomRunner(mockRunner)
16+
17+
if err := pm.Clean(&manager.Options{DryRun: true}); err != nil {
18+
t.Fatalf("Clean(DryRun=true) returned error: %v", err)
19+
}
20+
21+
if got := len(mockRunner.EnvCalls); got != 0 {
22+
t.Errorf("Clean(DryRun=true) executed %d non-interactive command(s); expected 0. Calls: %v",
23+
got, mockRunner.EnvCalls)
24+
}
25+
if got := len(mockRunner.InteractiveCalls); got != 0 {
26+
t.Errorf("Clean(DryRun=true) executed %d interactive command(s); expected 0. Calls: %v",
27+
got, mockRunner.InteractiveCalls)
28+
}
29+
}
30+
31+
// TestCleanRunsWithoutDryRun guards against the Clean(DryRun) fix being
32+
// implemented as a blanket no-op. Without DryRun, Clean MUST invoke
33+
// `apt autoclean`.
34+
func TestCleanRunsWithoutDryRun(t *testing.T) {
35+
mockRunner := manager.NewMockCommandRunner()
36+
mockRunner.AddCommand("apt", []string{"autoclean"}, []byte("Reading package lists...\n"), nil)
37+
pm := apt.NewPackageManagerWithCustomRunner(mockRunner)
38+
39+
if err := pm.Clean(&manager.Options{DryRun: false}); err != nil {
40+
t.Fatalf("Clean(DryRun=false) returned error: %v", err)
41+
}
42+
43+
if _, ok := mockRunner.EnvCalls["apt autoclean"]; !ok {
44+
t.Errorf("Clean(DryRun=false) didn't invoke 'apt autoclean'. Recorded calls: %v",
45+
mockRunner.EnvCalls)
46+
}
47+
}
48+
49+
// TestCleanRespectsDryRunWithNilOptsDefault verifies the nil-opts branch:
50+
// when opts == nil, the code path defaults DryRun to false, so Clean
51+
// SHOULD execute (proving the nil-opts default is preserved by the fix).
52+
func TestCleanRunsWithNilOpts(t *testing.T) {
53+
mockRunner := manager.NewMockCommandRunner()
54+
mockRunner.AddCommand("apt", []string{"autoclean"}, []byte("Reading package lists...\n"), nil)
55+
pm := apt.NewPackageManagerWithCustomRunner(mockRunner)
56+
57+
if err := pm.Clean(nil); err != nil {
58+
t.Fatalf("Clean(nil) returned error: %v", err)
59+
}
60+
61+
if _, ok := mockRunner.EnvCalls["apt autoclean"]; !ok {
62+
t.Errorf("Clean(nil) didn't invoke 'apt autoclean'. Recorded calls: %v",
63+
mockRunner.EnvCalls)
64+
}
65+
}

testing/docker/docker-compose.test.yml

Lines changed: 51 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,19 @@ services:
1818
volumes:
1919
- ../..:/workspace
2020
working_dir: /workspace
21+
# NOTE: bash -ec ensures any failed required command (test, apt update)
22+
# exits immediately. The `|| true` on fixture-generation lines is the
23+
# explicit opt-out — those steps are allowed to fail without failing the
24+
# build. Previously this used `&&` chaining which silently masked test
25+
# failures because trailing `|| true` caught failures from earlier in the
26+
# chain.
2127
command: >
22-
bash -c "
23-
echo 'Running Ubuntu APT tests...' &&
24-
go test -v -tags='unit integration apt' ./manager/apt ./osinfo &&
25-
echo 'Generating APT fixtures...' &&
26-
apt update &&
27-
apt search vim > testing/fixtures/apt/search-vim-ubuntu22.txt 2>/dev/null || true &&
28+
bash -ec "
29+
echo 'Running Ubuntu APT tests...';
30+
go test -v -tags='unit integration apt' ./manager/apt ./osinfo;
31+
echo 'Generating APT fixtures...';
32+
apt update;
33+
apt search vim > testing/fixtures/apt/search-vim-ubuntu22.txt 2>/dev/null || true;
2834
apt show vim > testing/fixtures/apt/show-vim-ubuntu22.txt 2>/dev/null || true
2935
"
3036
@@ -42,13 +48,14 @@ services:
4248
volumes:
4349
- ../..:/workspace
4450
working_dir: /workspace
51+
# See ubuntu-apt-test for rationale on bash -ec + ; separators.
4552
command: >
46-
bash -c "
47-
echo 'Running Rocky Linux YUM tests...' &&
48-
go test -v -tags='unit integration yum' ./manager/yum ./osinfo &&
49-
echo 'Generating YUM fixtures...' &&
50-
yum search vim > testing/fixtures/yum/search-vim-rocky8.txt 2>/dev/null || true &&
51-
yum info vim-enhanced > testing/fixtures/yum/info-vim-rocky8.txt 2>/dev/null || true &&
53+
bash -ec "
54+
echo 'Running Rocky Linux YUM tests...';
55+
go test -v -tags='unit integration yum' ./manager/yum ./osinfo;
56+
echo 'Generating YUM fixtures...';
57+
yum search vim > testing/fixtures/yum/search-vim-rocky8.txt 2>/dev/null || true;
58+
yum info vim-enhanced > testing/fixtures/yum/info-vim-rocky8.txt 2>/dev/null || true;
5259
yum list --installed > testing/fixtures/yum/list-installed-rocky8.txt 2>/dev/null || true
5360
"
5461
@@ -66,12 +73,13 @@ services:
6673
volumes:
6774
- ../..:/workspace
6875
working_dir: /workspace
76+
# See ubuntu-apt-test for rationale on bash -ec + ; separators.
6977
command: >
70-
bash -c "
71-
echo 'Running AlmaLinux YUM tests...' &&
72-
go test -v -tags='unit integration yum' ./manager/yum ./osinfo &&
73-
echo 'Generating YUM fixtures...' &&
74-
yum search vim > testing/fixtures/yum/search-vim-alma8.txt 2>/dev/null || true &&
78+
bash -ec "
79+
echo 'Running AlmaLinux YUM tests...';
80+
go test -v -tags='unit integration yum' ./manager/yum ./osinfo;
81+
echo 'Generating YUM fixtures...';
82+
yum search vim > testing/fixtures/yum/search-vim-alma8.txt 2>/dev/null || true;
7583
yum info vim-enhanced > testing/fixtures/yum/info-vim-alma8.txt 2>/dev/null || true
7684
"
7785
@@ -125,14 +133,34 @@ services:
125133
# Test runner that runs all tests in parallel
126134
test-all:
127135
image: ubuntu:24.04
136+
# Long-form depends_on with service_completed_successfully so test-all
137+
# waits for the actual tests to complete before running. With short-form,
138+
# test-all's `echo` could finish in milliseconds and abort the compose
139+
# run (--abort-on-container-exit) before the real tests had a chance to
140+
# report failure — exactly the same class of CI-honesty bug the bash -ec
141+
# change in this same release fixes one layer up.
128142
depends_on:
129-
- ubuntu-apt-test
130-
- rockylinux-yum-test
131-
- almalinux-yum-test
132-
# - fedora-dnf-test # TODO: Enable when DNF support is implemented
133-
# - alpine-apk-test # TODO: Enable when APK support is implemented
143+
ubuntu-apt-test:
144+
condition: service_completed_successfully
145+
rockylinux-yum-test:
146+
condition: service_completed_successfully
147+
almalinux-yum-test:
148+
condition: service_completed_successfully
149+
# fedora-dnf-test: # TODO: Enable when DNF support is implemented
150+
# condition: service_completed_successfully
151+
# alpine-apk-test: # TODO: Enable when APK support is implemented
152+
# condition: service_completed_successfully
153+
# Defense-in-depth: this aggregator service only runs an echo, so it can
154+
# safely use a read-only root, no-new-privileges, and a read-only bind
155+
# mount. Required services (ubuntu/rocky/alma) need write access for
156+
# fixture generation, so they don't get these constraints.
157+
read_only: true
158+
security_opt:
159+
# Quoted to avoid YAML parser ambiguity around `key:value` parsing
160+
# while preserving Docker's documented `no-new-privileges:true` form.
161+
- "no-new-privileges:true"
134162
volumes:
135-
- ../..:/workspace
163+
- ../..:/workspace:ro
136164
working_dir: /workspace
137165
command: >
138166
bash -c "

0 commit comments

Comments
 (0)