feat: graceful shutdown, shell completion, tests & CI improvements#13
feat: graceful shutdown, shell completion, tests & CI improvements#13
Conversation
lanathlor
commented
Mar 5, 2026
lanathlor
commented
- Add graceful shutdown manager with priority-based handler execution and operation tracking
- Add shell completion infrastructure (bash/zsh/fish/powershell) with dynamic completions for configs, backups, and config keys
- Add daemon unit tests (compose handlers, health, readiness, router, upload, security middleware)
- Add daemon integration test CI workflow
- Add remote includes support for registry models and improved config list output
- Update Go modules and Makefile with completion and doc generation targets
…to utils Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lanathlor
left a comment
lanathlor
left a comment
There was a problem hiding this comment.
Review — Issues introduced by this PR
[blocking] Insecure temp file permissions (internal/models/config.go)
Remote include temp files are created with 0644 (world-readable) and predictable names (remote-include-<hash>-<pid>.yaml). Sensitive config data could be read by other users on the system.
Fix: Use 0600 and randomized filenames (e.g. os.CreateTemp pattern).
[blocking] TryPullConfig silent failure (internal/models/registry.go)
In pullRemoteInclude(), TryPullConfig() logs ErrPullingImage but doesn't return it — execution continues to ContainerCreate() which fails with a cryptic error.
Fix: Return ErrPullingImage instead of just logging it.
[blocking] OperationTracker done() can panic (internal/shutdown/tracker.go)
If the returned done function is called twice, wg.Done() panics with "negative WaitGroup counter", violating the project's no-panics convention.
Fix: Wrap with sync.Once.
[suggestion] forceQuitCount logic error (internal/shutdown/manager.go:131)
count >= 1 is always true after the first Add(1). The intended double-SIGINT force-quit requires count >= 2.
[suggestion] SetTimeout() data race (internal/shutdown/manager.go:78-80)
m.timeout is written without lock protection but read in Shutdown(). Protect with mutex.
[suggestion] CI permissions too broad (.github/workflows/ci.yml:9-11)
contents: write and packages: write are granted globally to all jobs. Scope these to only the release job.
[suggestion] Docker command format (internal/models/registry.go:146)
Cmd: []string{"sleep 60"} passes a single string — should be []string{"sleep", "60"}. (Line 419 already does it correctly.)
[suggestion] Global function pointer mocking in tests (internal/stamus/instances_test.go)
Package-level mock pointers (osOpenFile, ioReadAll, getContainersByProject) will break under t.Parallel(). Consider interfaces or dependency injection.
[suggestion] Timing-dependent tests (internal/completion/cache_test.go, internal/shutdown/shutdown_test.go)
1ms TTL + 5ms sleep is prone to flakiness on slow CI. Use longer durations or mock time.
[nit] Missing bounds check (internal/completion/backups.go — formatSize)
"KMGTPE"[exp] panics for values >= 1024^6 bytes.
[nit] Inconsistent error wrapping (internal/models/registry.go)
Some errors use %w wrapping, others return bare sentinels. Prefer consistent wrapping with context.
[nit] Unused interfaces (internal/completion/providers.go)
CompletionProvider and ContextualProvider are defined but never used — consider removing or documenting intended use.
- Secure temp file creation with randomized name and 0600 perms (config.go) - Fix cache file permissions from 0644 to 0600 (registry.go) - Return error on ErrPullingImage instead of silent fallthrough (registry.go) - Fix Docker cmd format: split "sleep 60" into ["sleep", "60"] (registry.go) - Wrap sentinel errors with underlying cause for better diagnostics (registry.go) - Prevent double-call panic in OperationTracker done() with sync.Once (tracker.go) - Fix forceQuitCount threshold from >= 1 to >= 2 (manager.go) - Protect SetTimeout/Shutdown timeout field with mutex (manager.go)
- Scope contents:write and packages:write to release job only (ci.yml) - Increase TTL/sleep margins in cache expiration tests (cache_test.go) - Increase timing margins in shutdown tests (shutdown_test.go)
…ed DI Introduce FileOpener and ContainerLister interfaces with a ConfigManager struct that holds dependencies. Move package-level functions to methods on ConfigManager and add backward-compatible wrapper functions so external callers remain unchanged. Tests now use mock implementations per test instead of mutating shared global function pointers, making them safe for t.Parallel().
LeVraiBaptiste
force-pushed
the
prod-ready
branch
from
a299dd8 to
5f16661
Compare
|
2 participants
