fix(commands): harden scripts against silent failure modes - #302
Open
fjen wants to merge 13 commits into
Open
Conversation
…file Backup script assumed that the volume is dependend on the parent dir name. Could fail silently if directory is renamed. Derive volume name from compose file instead.
Docker command on postgres was run with `-t` flag, which would fail when running in non-interactive terminals (e.g, cron).
Passing --env without additional parameter would hand the script before.
…ithout params. The tail block ran at source time with the *sourcing* script's positional params, guarded by `[ $# -gt 0 ]`. Invoking a script without an env flag skipped it entirely, leaving COMPOSE_BASE_CMD empty. So `$COMPOSE_BASE_CMD down` degraded to plain `down: command not found` (127) while the auto-detection meant to handle exactly that case was unreachable dead code. common.sh is now a define-only library; every consumer calls parse_environment_args + init_environment explicitly, and the documented `show_usage` behavior is reachable via a BASH_SOURCE standalone-CLI guard (sourcing stays side-effect-free).
The script's four compose invocations hardwired `--env-file
$ENV_DIR/coda.env --env-file $POSTGRES_ENV_FILE`, bypassing
COMPOSE_BASE_CMD from common.sh. Two consequences:
- On --local the contract is django.env + postgres.env; coda.env is the
*production* env file. The upgrade path (build args, ${POSTGRES_VERSION}
interpolation, compose project inputs) could therefore resolve
differently than every other command — and did so quietly, agreeing
with normal operation only because a coda.env also exists locally.
- `pg_isready -U django` hardcoded the local username; production's
POSTGRES_USER is different. It now reads
${POSTGRES_USER:?POSTGRES_USER missing in ...}, so both the correct
user is used and a missing env entry fails with a named error.
import_invoices.sh and import_requests.sh were 21-line clones differing
only in the management command they invoked. One script now takes the
manage.py command verbatim as its first argument:
./commands/import.sh import_fundingrequests --local requests.json
./commands/import.sh import_invoices --production invoices.json
No alias table: a future import command works the day it lands, with no
edit here and no second naming scheme to keep in sync and the script's
argument is exactly what manage.py receives. A shape check
(^[a-z][a-z0-9_]*$) rejects paths and flags before they reach the
container. Unknown command names are reported by manage.py itself, which
knows the real command list.
Both old entry points are removed in the same commit and the user docs
were updated to the real command names. The realpath file check applies
to all imports now. It fails faster on bad path than invoking the whole
django stack.
…git steps
The script had no global failure mode: step_fetch_and_switch ignored the
result of `git fetch` and `git checkout -b`, and its
`git checkout "$BRANCH" 2>/dev/null` classified *every* checkout error —
network, conflict, unrelated breakage — as "branch missing locally".
The concrete hazard, with CODA already stopped: fetch fails, the
local branch does not exist yet, `checkout -b stable origin/stable` fails
against the never-fetched ref, HEAD stays put — and the step still
returned 0 (its status came from `echo ""`). Step 4 then ran
`git pull origin stable` *into the wrong branch*, and step 5 started
CODA from the merged result.
- `set -euo pipefail` makes every unchecked command fatal. The previous
`step_x || exit 1` call sites are replaced with plain calls, because
`set -e` is deliberately disabled inside functions invoked in a
conditional context — with `|| exit 1` kept, the option would have
applied to nothing inside the steps. The now-dead `if [[ $? -ne 0 ]]`
checks are gone; remaining checks are the ones that add a message.
- The pull-failure rollback (restore previous branch, pop stash) moved
from the middle of step 4 into the EXIT trap via ROLLBACK_ON_EXIT —
under set -e the in-line recovery would be skipped on exactly the
abort it was written for. The trap preserves the exit status.
- Branch existence is decided by `git show-ref --verify` instead of
trial-and-error checkout with stderr discarded.
- `--branch` value read via `${2:-}` so set -u yields the script's own
error message rather than an unbound-variable abort.
Verified: unknown branch and missing --branch value exit 1 before any
service is touched; --help exits 0; all steps pass bash -n.
Give the git stash executed during upgrade a name instead of relying that it stays on top of the stack.
The coda-oa/coda fallback in _get_repo can never executed. Made sure the mechanism also works for git+ssh, not only git+http.
- quote @{upstream}, $cmd, --$CODA_ENV expansions (SC1083/SC2086)
- show_usage drops its unused script-name parameter (SC2119/SC2120)
- source-path=SCRIPTDIR directives let shellcheck follow common.sh from
any invocation directory; this also clears the remaining_args SC2154
warnings by making the assignment visible to the checker
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several changes introduce or preserve concrete failure modes (notably set -u incompatibility in common.sh and a broken backups.sh restore command argument passing) that can cause scripts to fail or behave incorrectly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
commands/update-coda.sh:176
STASH_REFis still tied to the stash queue position (stash@{0}), which can change if another stash is created; capture the stash commit id instead so we always refer to the stash created by this run.
if has_uncommitted_changes; then
echo "Stashing uncommitted changes..."
git stash push --include-untracked -m "$STASH_MSG"
STASH_REF="stash@{0}"
fi
- Files reviewed: 13/13 changed files
- Comments generated: 10
- Review effort level: Lite
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A systematic pass over the
commands/scripts. Each commit targets one failure mechanism where a script continued or reported success despite underlying failures: unchecked git operations during an update, a stash popped by queue position instead of identity, an argument parser that spun forever, backup commands that died without a TTY, a postgres upgrade that could silently target the wrong volume, and fallbacks that could never execute.Verified best practices with
shellcheckat the end. Maybe we include that in our pre-commit pipeline?