Skip to content

Catch module uninstalled + code removed in same push (reroll of #120)#131

Open
bserem wants to merge 1 commit into
mainfrom
claude/fix-pr-120-sanity-check-mQxcs
Open

Catch module uninstalled + code removed in same push (reroll of #120)#131
bserem wants to merge 1 commit into
mainfrom
claude/fix-pr-120-sanity-check-mQxcs

Conversation

@bserem
Copy link
Copy Markdown
Collaborator

@bserem bserem commented May 16, 2026

$(cat <<'EOF'

Summary

Reroll of #120 — the original PR added the safe-uninstall check directly into the monolithic sanity-check file, but sanity-check has since been refactored into a modular _lib/ architecture. This PR ports the same logic to fit that new structure.

  • Adds critical_fail(), bail_if_critical(), and CRITICAL_ERRORS counter to commands/host/sanity-check
  • Extracts the uninstall-safety check into a new commands/host/_lib/check-drupal-safe-uninstall.sh, sourced right after check-drupal-extensions.sh (which sets EXT_FILE)
  • Updates scripts/git-hooks/pre-push to expose GIT_PUSH_RANGE_BASE/GIT_PUSH_RANGE_TIP from the push ref stdin, and blocks the push unconditionally (no bypass prompt) when a CRITICAL: line appears in sanity-check output

What it catches

When a developer tries to push a commit that simultaneously:

  1. Removes a module from config/sync/core.extension.yml, and
  2. Removes the drupal/<module> package from composer.lock (or the module directory is already gone from disk)

…the check flags it as CRITICAL and bail_if_critical exits with code 2. The pre-push hook detects the CRITICAL: string in the output and blocks the push with no option to continue — unlike normal sanity-check errors which can be bypassed with a y prompt.

The correct workflow is: uninstall the module → deploy → remove the code in a separate change.

Test plan

  • Push a branch where core.extension.yml removes a module AND composer.lock removes drupal/<that_module> — push should be blocked with a CRITICAL message
  • Push a branch where only the extension is removed (code still present) — should pass with a green tick
  • Push a branch where only composer package is removed (extension still enabled) — unrelated to this check, should not be flagged here
  • ddev sanity-check with no git changes — safe-uninstall section is silently skipped

https://claude.ai/code/session_01KWHPaEeFbQ9MKU2wLzaKNU
EOF
)


Generated by Claude Code

- Add critical_fail() / bail_if_critical() helpers and CRITICAL_ERRORS
  counter to the main sanity-check command
- Extract the module safe-uninstall check into _lib/check-drupal-safe-uninstall.sh,
  sourced after check-drupal-extensions.sh (which sets EXT_FILE)
- Update the pre-push hook to expose GIT_PUSH_RANGE_BASE/TIP so the
  check can diff only the committed changes being pushed; capture
  sanity-check output and block unconditionally on CRITICAL: lines

https://claude.ai/code/session_01KWHPaEeFbQ9MKU2wLzaKNU
@bserem
Copy link
Copy Markdown
Collaborator Author

bserem commented May 16, 2026

@dimitriskr can you do a functional review here please?

You'll have to simulate the problem:
Uninstall pathauto (or anything else)
and
Remove it from composer and try to push your changes

Thanks

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.

2 participants