Skip to content

fix: resolve 17 bugs found during comprehensive code review#22

Merged
SimplicityGuy merged 4 commits intomainfrom
fix/bug-review-fixes
Apr 4, 2026
Merged

fix: resolve 17 bugs found during comprehensive code review#22
SimplicityGuy merged 4 commits intomainfrom
fix/bug-review-fixes

Conversation

@SimplicityGuy
Copy link
Copy Markdown
Owner

Summary

Comprehensive bug review identified and fixes 17 issues across entrypoint.sh, Dockerfile, CI/CD workflows, and config samples.

Critical fixes

  • CRONTAB_FILE resolved before HOME_DIR — crontab written to /crontab instead of ${HOME_DIR}/crontab
  • jq pipeline crashes on array-format configsto_entries assumes object input; array configs (documented format) caused script exit
  • @every schedule silently dropped — BusyBox crond doesn't support @every; jobs never ran. Now errors clearly, samples updated to */2 * * * *
  • update-project.yml never creates PRs — grep pattern didn't match Dockerfile, detection logic was inverted

Important fixes

  • Container leak: docker run missing --rm, stopped containers accumulate indefinitely
  • crond log level inverted: -d 7 is nearly silent, not max debug; changed to -d 0
  • ARM binary compatibility: glibc-linked rq binaries fail on Alpine musl; added gcompat
  • LABEL created: Was literal $(date ...) string, now uses BUILD_DATE build-arg
  • DOCKER_PORT_2375_TCP check: [ -a ] tests file existence, not variable non-emptiness
  • COMMENT injection: Newlines in comment field could inject crontab entries
  • Slug collision: Duplicate slugs silently overwrote job scripts
  • Non-atomic script writes: Scripts now built in temp file, then mv'd into place
  • Onstart error tracking: Background jobs now tracked with wait + exit status warning
  • DOCKER_GID validation: Now rejects non-numeric values
  • Schedule escaping: Removed unnecessary \* escape/unescape round-trip
  • IFS scoped locally: Global IFS set moved to read command scope
  • Config samples: Fixed corrupted TOML key, wrong volume mount, EOL alpine:3.5

Test plan

  • Build Docker image successfully on amd64: docker build -t crontab .
  • Build Docker image on arm64 (verify gcompat resolves rq binary issue)
  • Test with array-format JSON config (config.sample.json)
  • Test with mapping-format JSON config (config.sample.mapping.json)
  • Test with TOML config (config.sample.toml)
  • Verify @every schedule produces clear error message
  • Verify generated crontab file is written to correct ${HOME_DIR}/crontab path
  • Verify --rm is present in generated docker run commands
  • Verify crond output is verbose (log level 0)
  • Verify docker inspect labels show BUILD_DATE value
  • Run update-project workflow manually to verify base image detection

🤖 Generated with Claude Code

SimplicityGuy and others added 4 commits April 3, 2026 21:49
Rename update-dependencies.sh to update-project.sh with expanded scope:
- Add pre-commit autoupdate --freeze support
- Scan all workflow files for GitHub Actions updates (not just build.yml)
- Add docker build verification step after updates
- Add --skip-tests flag and git dirty-tree warning
- Use portable sed for macOS/Linux compatibility
- Adopt emoji-based section logging style

Also rename workflow, update dependabot with security update groups,
and fix stale GitHub Action versions in build.yml.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
entrypoint.sh:
- Fix CRONTAB_FILE evaluated before HOME_DIR is resolved
- Fix jq pipeline crash on array-format configs (JSON/YAML)
- Reject unsupported @every schedule with clear error
- Remove fragile \* escape/unescape round-trip in schedule parsing
- Fix [ -a ] test operator to [ -n ] for DOCKER_PORT_2375_TCP check
- Add --rm to docker run to prevent container leak on every cron job
- Sanitize COMMENT field to strip newlines (injection prevention)
- Detect slug collisions and append counter suffix
- Write scripts atomically via temp file then mv
- Track onstart job PIDs and warn on non-zero exit
- Warn when no config file is found
- Scope IFS to read command instead of setting globally

Dockerfile:
- Fix LABEL created using BUILD_DATE build-arg instead of literal $(date)
- Fix crond -d 7 (nearly silent) to -d 0 (most verbose)
- Add gcompat for ARM glibc-linked rq binary compatibility
- Validate DOCKER_GID is numeric before use

CI/CD & config:
- Fix update-project.yml grep pattern to match actual Dockerfile FROM lines
- Fix update-project.yml update detection logic (was inverted)
- Pass BUILD_DATE build-arg in build.yml
- Remove deprecated docker-compose version key
- Update EOL alpine:3.5 to alpine:3.21 in docker-compose and samples
- Replace unsupported @every 2m with */2 * * * * in all sample configs
- Fix corrupted TOML table key for logrotate entry
- Fix TOML sample using ${PWD} instead of named volume
- Add backups/ to .gitignore

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@SimplicityGuy SimplicityGuy merged commit 575c6d3 into main Apr 4, 2026
2 checks passed
@SimplicityGuy SimplicityGuy deleted the fix/bug-review-fixes branch April 4, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant