[codex] use shared immutable releases for deploys - #407
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces shared immutable releases under ChangesImmutable Shared Release Model
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the deployment model from per-instance mutable checkouts/builds to shared, immutable release directories under /opt/docketworks/releases/<sha>, with each instance switching a current symlink to select the running release. This reduces cross-instance coupling (deps/build artifacts) and enables consistent roll-forward/roll-back behavior keyed by release SHA.
Changes:
- Introduces
release-utils.shto build, switch, and garbage-collect immutable releases (per-release venv + frontend/manual build). - Updates deploy/provision/rollback scripts plus systemd/nginx templates to run from
instances/<name>/currentinstead of per-instance checkouts/shared venv. - Updates frontend + backend build-id resolution and frontend env expectations to work in the shared-release layout, with docs/tests extended accordingly.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/server/templates/nginx-instance.conf.template | Serve frontend + manual assets from instances/<name>/current/... paths. |
| scripts/server/templates/gunicorn-instance.service.template | Run gunicorn from the release-local venv in current, set working dir to current. |
| scripts/server/templates/frontend-env-instance.template | Removes per-instance frontend .env template (no longer rendered). |
| scripts/server/templates/celery-worker-instance.service.template | Run celery worker from current release venv/working dir. |
| scripts/server/templates/celery-beat-instance.service.template | Run celery beat from current release venv/working dir. |
| scripts/server/templates/backup-db-instance.service.template | Run backup script from current release path. |
| scripts/server/server-setup.sh | Stops building shared venv/node_modules; prepares releases and npm cache dirs. |
| scripts/server/release-utils.sh | New helpers to build releases, switch current, and prune unreferenced releases. |
| scripts/server/README.md | Documents shared-release architecture and deploy/cleanup behavior. |
| scripts/server/instance.sh | Provision instances by creating current symlink and using shared releases; moves secret fixtures to instance-private .fixtures. |
| scripts/server/dw-run.sh | Executes commands inside the current release venv/working dir while sourcing instance .env. |
| scripts/server/deploy.sh | Deploy via --ref→SHA→shared release build, switch current, migrate, rerender units/nginx, cleanup releases. |
| scripts/server/common.sh | Adds RELEASES_DIR and centralizes read_env_value. |
| scripts/README.md | Updates backup/rollback descriptions to use release SHA switching. |
| scripts/predeploy_rollback.sh | Rollback now switches current to a release SHA and restarts services accordingly. |
| scripts/predeploy_backup.sh | Backup now stamps dumps with current release SHA instead of git checkout HEAD. |
| frontend/vite.config.ts | Build-id resolution from env/.release-sha before git; avoid backend .env dependency in production builds. |
| frontend/src/config/app.ts | Fallback default for APP_NAME when VITE_APP_NAME is unset. |
| frontend/src/config/adminPages.ts | Compute UAT URL dynamically from hostname instead of VITE_UAT_URL. |
| frontend/env.d.ts | Removes VITE_UAT_URL typing. |
| frontend/.env.example | Removes VITE_UAT_URL example entry. |
| docs/updating.md | Updates operator workflow docs for shared release deploys + cleanup-only mode. |
| docs/server_setup.md | Updates provisioning docs to reflect current -> releases/<sha> layout. |
| docketworks/settings.py | Build-id now resolves from env/.release-sha before falling back to git. |
| apps/workflow/tests/test_xero_instance_templates.py | Extends template/script tests to cover shared-release mechanics and build-id behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+156
to
+160
| for instance_dir in "$INSTANCES_DIR"/*; do | ||
| [[ -d "$instance_dir" ]] || continue | ||
| if [[ -L "$instance_dir/current" && "$(readlink -f "$instance_dir/current")" == "$(release_path "$sha")" ]]; then | ||
| return 0 | ||
| fi |
Owner
Author
There was a problem hiding this comment.
Intentionally not applying this. A broken current symlink is an invalid state, and this codebase fails fast on invalid state rather than tolerating it (ADR 0015: fix the data, don't add a read-side fallback). Wrapping readlink in || true would let cleanup silently proceed past a broken instance link, masking a real problem. If a stop is too opaque we make it clearer (a diagnostic error), never tolerant. The deploy/cleanup aborting on a broken symlink is the desired behaviour.
Comment on lines
+145
to
+150
| fqdn=$(grep -oP 'server_name \K[^;]+' "$existing_conf" | head -1 | awk '{$1=$1; print}') | ||
| cert_domain=$(grep -oP 'ssl_certificate /etc/letsencrypt/live/\K[^/]+' "$existing_conf" | head -1) | ||
| if [[ -z "$fqdn" || -z "$cert_domain" ]]; then | ||
| log " ERROR: Could not extract FQDN/CERT_DOMAIN from $existing_conf" | ||
| return 1 | ||
| fi |
Comment on lines
35
to
39
| DB_NAME=$(grep -E '^DB_NAME=' "$ENV_FILE" | cut -d= -f2) | ||
| if [[ -z "$DB_NAME" ]]; then | ||
| echo "ERROR: DB_NAME not set in $ENV_FILE" >&2 | ||
| exit 1 | ||
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/server/instance.sh (1)
225-241: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftHarden secret fixture staging against symlinks and failed cleanup.
The instance user owns
$INSTANCE_DIR, so.fixturescan be pre-created or replaced before this root script writes secrets into it. Reject symlinks, make the staging directory/files root-owned but readable by the instance user, and use a trap so failedloaddatacalls do not leave JSON secrets behind.Also applies to: 247-266, 496-508
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/server/instance.sh` around lines 225 - 241, The secret fixture staging in the AI providers fixture generation path is vulnerable because the instance user can pre-create or replace .fixtures before root writes sensitive JSON, and failed loaddata runs can leave secrets behind. Update the fixture creation flow around the ai_providers.json generation logic to reject symlinks, ensure the staging directory and file are root-owned while still readable by the instance user, and add a trap/cleanup path so temporary secret fixtures are removed on failure. Apply the same hardening to the related fixture-writing flows referenced by the same staging helpers so all secret-containing outputs are protected consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docketworks/settings.py`:
- Around line 18-34: `read_build_id()` is returning unvalidated values from
`DOCKETWORKS_BUILD_SHA` and `.release-sha`, so add upfront validation before
returning any build ID. Update the `read_build_id` flow to verify the value is a
real commit SHA format (for example, a full hex SHA) for both the environment
variable and the file contents, and fail fast with a clear error if it is
missing or invalid. Keep the existing fallback order in `read_build_id`, but
only return values that pass validation so `/api/build-id/` never exposes
malformed IDs.
In `@scripts/predeploy_rollback.sh`:
- Around line 65-70: Handle release-resolution failures in predeploy rollback
before assigning FULL_SHA. The current FULL_SHA="$(resolve_existing_release_sha
"$HASH")" path can terminate under set -e before the friendly release-missing
message is reached; update the rollback flow in predeploy_rollback.sh so
resolve_existing_release_sha/resolve_release_ref failures are captured
explicitly and reported with the intended error handling before checking
release_complete.
In `@scripts/server/deploy.sh`:
- Around line 316-327: The migration failure handler in deploy.sh only rolls
back the release pointer in the migrate failure path, which can leave the
database partially upgraded; update the migrate handling around the dw-run.sh
python manage.py migrate call to either invoke the pre-deploy database
restore/rollback flow or avoid switching back to the previous release unless DB
state has been explicitly restored or reconciled. Use the existing rollback
helpers and the switch_instance_release logic in this block to ensure the app
and schema stay in sync before continuing.
- Around line 333-335: Skip release cleanup when any instance has failed after
switching current. In deploy.sh, the failure path around render_nginx_config and
the later cleanup logic should be linked so that FAILED_INSTANCES prevents
deleting previous_sha entries that may still be serving traffic. Update the
cleanup section near the code that removes unreferenced SHAs to either
short-circuit when FAILED_INSTANCES is non-empty or explicitly keep previous
SHAs for failed instances, using the existing FAILED_INSTANCES, previous_sha,
and current release-handling logic to locate the fix.
In `@scripts/server/instance.sh`:
- Around line 323-328: The bootstrap detection in instance.sh is incorrectly
using the presence of .env to decide whether setup is complete, which can skip
migrations and admin initialization after a failed first run. Update the logic
around IS_EXISTING and NEEDS_APP_BOOTSTRAP so it relies on an explicit success
marker written only after bootstrap completes, or checks the database
migration/bootstrap state before setting NEEDS_APP_BOOTSTRAP false. Keep the fix
localized to the existing bootstrap flow near render_instance_env and the
current .env checks.
In `@scripts/server/README.md`:
- Line 208: The README inventory entry for server-setup.sh is stale because it
still mentions venv even though dependency installation now happens in release
builds via deploy.sh. Update the server-setup.sh description in the
scripts/server/README.md inventory row to remove the venv reference and keep the
wording aligned with its current host-level convergence responsibilities.
- Line 155: The fenced block in the README is missing a language identifier,
triggering the MD040 markdown lint rule. Update the fenced code block in the
server README section to include an explicit language tag (for example, the
block containing the /opt/docketworks/ path) so the opening and closing fences
remain balanced and markdown lint passes.
In `@scripts/server/release-utils.sh`:
- Around line 68-69: The switch_instance_release flow can update current to a
release that is missing or incomplete. Add a guard in switch_instance_release
using release_complete "$sha" before the ln -sfn and mv -Tf steps so the symlink
is only replaced when the target release is valid, and keep the check close to
the existing release selection logic in release-utils.sh.
- Around line 100-142: The release build currently creates the virtualenv under
the temporary .building path in the build block, which leaves venv metadata and
console scripts pointing at the wrong location after mv to the final release
directory. Update the release assembly logic in release-utils.sh so the
virtualenv used by the poetry install and subsequent npm steps is created or
rebuilt directly under the final release path (the release_dir/current .venv
location) before touch of .complete, and ensure the build commands reference
that final venv path instead of $tmp_dir/.venv.
---
Outside diff comments:
In `@scripts/server/instance.sh`:
- Around line 225-241: The secret fixture staging in the AI providers fixture
generation path is vulnerable because the instance user can pre-create or
replace .fixtures before root writes sensitive JSON, and failed loaddata runs
can leave secrets behind. Update the fixture creation flow around the
ai_providers.json generation logic to reject symlinks, ensure the staging
directory and file are root-owned while still readable by the instance user, and
add a trap/cleanup path so temporary secret fixtures are removed on failure.
Apply the same hardening to the related fixture-writing flows referenced by the
same staging helpers so all secret-containing outputs are protected
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fb9895b-6aa4-409c-b0c7-1132546231c3
📒 Files selected for processing (25)
apps/workflow/tests/test_xero_instance_templates.pydocketworks/settings.pydocs/server_setup.mddocs/updating.mdfrontend/.env.examplefrontend/env.d.tsfrontend/src/config/adminPages.tsfrontend/src/config/app.tsfrontend/vite.config.tsscripts/README.mdscripts/predeploy_backup.shscripts/predeploy_rollback.shscripts/server/README.mdscripts/server/common.shscripts/server/deploy.shscripts/server/dw-run.shscripts/server/instance.shscripts/server/release-utils.shscripts/server/server-setup.shscripts/server/templates/backup-db-instance.service.templatescripts/server/templates/celery-beat-instance.service.templatescripts/server/templates/celery-worker-instance.service.templatescripts/server/templates/frontend-env-instance.templatescripts/server/templates/gunicorn-instance.service.templatescripts/server/templates/nginx-instance.conf.template
💤 Files with no reviewable changes (3)
- frontend/.env.example
- frontend/env.d.ts
- scripts/server/templates/frontend-env-instance.template
Distil the load-bearing architectural rules and code-style gotchas from the ADRs and CLAUDE.md into .github/copilot-instructions.md so Copilot's chat, completions, and code review align with the codebase instead of suggesting typical Django/Vue defaults (deprecation shims, read-side fallbacks, |\| true tolerance patches). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a local shellcheck hook (system binary, default severity) plus a .shellcheckrc with external-sources + source-path=SCRIPTDIR so sourced libraries like scripts/server/common.sh are followed — killing the SC1091/SC2153 false positives. Then fix every remaining finding across the repo's shell scripts so they pass clean: - quote word-splitting expansions (SC2086/SC2046), drop useless cat (SC2002), use 'if cmd' over $? (SC2181), read -r (SC2162), split declare/assign (SC2155) - fix a real copy-paste bug in validate-migration.sh: the parseFloat() line was reporting the parseInt count - delete dead variables (SC2034); add justified directives for intentional dynamic 'source' (SC1090) and client-side ssh expansion (SC2029) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lure predeploy_rollback.sh restores the pre-deploy dump into a temporary database and atomically swaps it into place (rename live DB aside, promote restored DB, drop the old one) instead of dropping and restoring the live DB in place — a failed restore no longer destroys the running database. Adds a root-user check, requires DB_USER, and verifies the target release exists before prompting. deploy.sh stops an instance's services before switching releases. On a failed migration it leaves services stopped — the DB may be partially migrated, so code-only symlink rollback is unsafe — and prints the explicit predeploy_rollback.sh command and a showmigrations hint instead of auto-reverting. READMEs and docs/updating.md document the flow.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/server/README.md (2)
208-208: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
venvmention.
server-setup.shno longer owns dependency installation; the per-release virtualenv now comes from the release build. Keepingvenvhere misdescribes the operator flow.Proposed fix
-| `server-setup.sh` | Host-level convergence (packages, venv, SSL, shared config). Runs every deploy — see "Server Setup". | +| `server-setup.sh` | Host-level convergence (packages, SSL, shared config). Runs every deploy — see "Server Setup". |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/server/README.md` at line 208, The deployment table entry for server-setup.sh still mentions venv, but that responsibility has moved to the release build. Update the description in the README table row for server-setup.sh to remove the stale venv reference and keep the wording focused on host-level convergence items only, preserving the existing server-setup.sh and Server Setup references for location.
155-175: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a fence language tag.
The directory tree block still lacks a language identifier, so markdownlint will keep flagging MD040.
textis enough here.Proposed fix
-``` +```text /opt/docketworks/ ... -``` +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/server/README.md` around lines 155 - 175, Markdown in the directory tree block is missing a fence language tag, so update the fenced block in the README to use a text identifier to satisfy markdownlint MD040. Locate the directory tree snippet under the server README and change the opening fence to a text-labeled code block while keeping the rest of the content unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/server/README.md`:
- Line 208: The deployment table entry for server-setup.sh still mentions venv,
but that responsibility has moved to the release build. Update the description
in the README table row for server-setup.sh to remove the stale venv reference
and keep the wording focused on host-level convergence items only, preserving
the existing server-setup.sh and Server Setup references for location.
- Around line 155-175: Markdown in the directory tree block is missing a fence
language tag, so update the fenced block in the README to use a text identifier
to satisfy markdownlint MD040. Locate the directory tree snippet under the
server README and change the opening fence to a text-labeled code block while
keeping the rest of the content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16f0c6e2-ab52-4016-86ae-979cf6ceaf76
📒 Files selected for processing (21)
.pre-commit-config.yaml.shellcheckrcdocs/updating.mdfrontend/scripts/audit-numeric-conversions.shfrontend/scripts/validate-migration.shfrontend/tests/scripts/backup-db.shfrontend/tests/scripts/restore-db.shscripts/README.mdscripts/backup_db.shscripts/cleanup_backups.shscripts/predeploy_backup.shscripts/predeploy_rollback.shscripts/pull_prod_backup.shscripts/server/README.mdscripts/server/certbot-dreamhost-auth.shscripts/server/certbot-dreamhost-cleanup.shscripts/server/common.shscripts/server/deploy.shscripts/server/instance.shscripts/server/server-setup.shscripts/setup_database.sh
💤 Files with no reviewable changes (1)
- scripts/predeploy_backup.sh
✅ Files skipped from review due to trivial changes (11)
- scripts/backup_db.sh
- scripts/cleanup_backups.sh
- scripts/server/certbot-dreamhost-cleanup.sh
- .shellcheckrc
- frontend/scripts/audit-numeric-conversions.sh
- scripts/pull_prod_backup.sh
- scripts/setup_database.sh
- frontend/tests/scripts/restore-db.sh
- scripts/server/certbot-dreamhost-auth.sh
- scripts/README.md
- docs/updating.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/server/common.sh
- scripts/predeploy_rollback.sh
- scripts/server/deploy.sh
- scripts/server/instance.sh
…ling Build each shared release directly at releases/<sha> instead of building in a .building-<sha> staging dir and mv-ing it into place. Python venvs are not relocatable: the moved venv's console-script wrappers (gunicorn, celery) kept an absolute shebang to the staging path, so systemd service start failed after every deploy. Building in place keeps the shebangs valid; .complete (written last) stays the sole completion gate, and an interrupted build leaves an incomplete, unreferenced dir the next build clears (cleanup_stale_release_builds -> cleanup_incomplete_releases). Serial deploys mean no build races, so no lock is needed. Also: switch_instance_release refuses to point an instance at an incomplete/missing release, and resolve_existing_release_sha gates on release_complete now that incomplete dirs can exist at the canonical path; deploy.sh skips release cleanup when any instance failed (don't delete a failed instance's rollback target); render_nginx_config greps and predeploy DB_NAME/DB_USER reads no longer abort under set -e before their explicit guards run (read_env_value / || true). Addresses Copilot and CodeRabbit findings on #407. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
instance.sh decided whether to run one-time bootstrap (migrate + setup_dev_logins) from .env existence, but .env is written early — a first run that failed after that falsely skipped bootstrap and left a half-initialised instance. Gate on a .bootstrap-complete marker written only after bootstrap succeeds. setup_dev_logins resets staff passwords, so re-running bootstrap on a live instance is destructive: an existing instance with no marker is ambiguous (pre-marker vs failed first run), so stop with guidance rather than guess. Mark established instances once with 'sudo touch <dir>/.bootstrap-complete'. Addresses CodeRabbit finding on #407. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
read_build_id() returned any non-empty DOCKETWORKS_BUILD_SHA / .release-sha / git output into /api/build-id/; a malformed value would silently break the frontend version-check reload. Validate all three sources are a 40-char hex SHA and raise ImproperlyConfigured otherwise. Adds tests. Addresses CodeRabbit finding on #407. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The directory-tree fence lacked a language tag (MD040); the server-setup.sh inventory row said it installs a 'venv', but the app venv is built per-release by release-utils.sh. Addresses CodeRabbit findings on #407. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/server/release-utils.sh (1)
38-39: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse
release_completefor prefix-matched releases too.The prefix path accepts any directory with
.complete, bypassing the.release-shaidentity check used byrelease_complete. A stale/corrupt prefix match can prevent fallback ref resolution and break rollback/deploy selection.Proposed fix
- if (( ${`#matches`[@]} == 1 )) && [[ -f "${matches[0]}/.complete" ]]; then - basename "${matches[0]}" - return 0 + if (( ${`#matches`[@]} == 1 )); then + matched_sha="$(basename "${matches[0]}")" + if release_complete "$matched_sha"; then + echo "$matched_sha" + return 0 + fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/server/release-utils.sh` around lines 38 - 39, The prefix-match branch in release-utils.sh is bypassing the same release validation used by release_complete, so update the lookup logic around the prefix match handling to require the .release-sha identity check before accepting a directory. Reuse the release_complete validation path or apply the same check in the matching logic that returns basename for matches[0], so only valid completed releases are selected and stale/corrupt prefix matches are rejected.scripts/server/instance.sh (1)
512-524: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove generated fixture secrets on load failure.
With
set -e, a failingdw-run.sh ... loaddataexits before the followingrm -f, leaving generated AI/Xero fixture JSON on disk. Wrap each load so cleanup runs on both success and failure.🛡️ Proposed cleanup fix
render_ai_providers_fixture "$INSTANCE_DIR" "$INSTANCE_USER" log "Loading AI providers..." local AI_PROVIDERS_FIXTURE="$INSTANCE_DIR/.fixtures/ai_providers.json" - "$SCRIPT_DIR/dw-run.sh" "$INSTANCE" python manage.py shell -c \ - "from django.core.management import call_command; from apps.workflow.models import AIProvider; print('AIProvider already configured; skipping ai_providers.json load') if AIProvider.objects.exists() else call_command('loaddata', '$AI_PROVIDERS_FIXTURE')" + if ! "$SCRIPT_DIR/dw-run.sh" "$INSTANCE" python manage.py shell -c \ + "from django.core.management import call_command; from apps.workflow.models import AIProvider; print('AIProvider already configured; skipping ai_providers.json load') if AIProvider.objects.exists() else call_command('loaddata', '$AI_PROVIDERS_FIXTURE')"; then + rm -f "$AI_PROVIDERS_FIXTURE" + exit 1 + fi rm -f "$AI_PROVIDERS_FIXTURE" render_xero_apps_fixture "$INSTANCE_DIR" "$INSTANCE_USER" log "Loading Xero apps..." local XERO_APPS_FIXTURE="$INSTANCE_DIR/.fixtures/xero_apps.json" - "$SCRIPT_DIR/dw-run.sh" "$INSTANCE" python manage.py shell -c \ - "from django.core.management import call_command; from apps.workflow.models import XeroApp; print('XeroApp already configured; skipping xero_apps.json load') if XeroApp.objects.exists() else call_command('loaddata', '$XERO_APPS_FIXTURE')" + if ! "$SCRIPT_DIR/dw-run.sh" "$INSTANCE" python manage.py shell -c \ + "from django.core.management import call_command; from apps.workflow.models import XeroApp; print('XeroApp already configured; skipping xero_apps.json load') if XeroApp.objects.exists() else call_command('loaddata', '$XERO_APPS_FIXTURE')"; then + rm -f "$XERO_APPS_FIXTURE" + exit 1 + fi rm -f "$XERO_APPS_FIXTURE"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/server/instance.sh` around lines 512 - 524, The AI/Xero fixture JSON cleanup in instance.sh can be skipped when dw-run.sh or loaddata fails because set -e exits before the rm -f calls. Update the AI provider and Xero app load blocks in render_ai_providers_fixture/render_xero_apps_fixture flow so each generated fixture file is removed regardless of success or failure, for example by wrapping the load/skip logic in a cleanup-safe construct. Keep the existing behavior around AIProvider.objects.exists() and XeroApp.objects.exists(), but ensure the temporary .fixtures/ai_providers.json and .fixtures/xero_apps.json are always deleted.
🧹 Nitpick comments (1)
apps/workflow/tests/test_build_id.py (1)
32-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
.release-shabuild-id path.The new runtime contract reads
DOCKETWORKS_BUILD_SHA, then.release-sha, then git; these tests only cover the env branch, leaving the release-directory path untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workflow/tests/test_build_id.py` around lines 32 - 40, Add a test for the `.release-sha` branch in ReadBuildIdTests so the new fallback path is covered. Extend test_build_id.py by mocking the environment without DOCKETWORKS_BUILD_SHA, then patching the file access used by read_build_id() to make `.release-sha` return a valid SHA and asserting that value is returned. Use the existing read_build_id() and ReadBuildIdTests symbols to locate the fallback logic, and keep the current env validation tests unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docketworks/settings.py`:
- Around line 44-47: The git SHA lookup in the settings module relies on
subprocess.run with the bare "git" executable name, which can be hijacked via
PATH. Update the logic around the SHA validation helper that calls git rev-parse
HEAD to resolve the full git executable path first and then use that pinned path
in subprocess.run, keeping the rest of the _validate_sha flow unchanged.
In `@scripts/server/release-utils.sh`:
- Around line 91-97: `ensure_release` currently performs an in-place rebuild
without its own synchronization, so concurrent `instance.sh` invocations can
delete or rebuild the same release directory at the same time. Add a
release-scoped lock inside `ensure_release` itself, or make sure every caller
acquires the same lock before entering that path; use the existing `rm -rf
"$release_dir"`/build flow in `release-utils.sh` as the place to wrap with
locking.
---
Outside diff comments:
In `@scripts/server/instance.sh`:
- Around line 512-524: The AI/Xero fixture JSON cleanup in instance.sh can be
skipped when dw-run.sh or loaddata fails because set -e exits before the rm -f
calls. Update the AI provider and Xero app load blocks in
render_ai_providers_fixture/render_xero_apps_fixture flow so each generated
fixture file is removed regardless of success or failure, for example by
wrapping the load/skip logic in a cleanup-safe construct. Keep the existing
behavior around AIProvider.objects.exists() and XeroApp.objects.exists(), but
ensure the temporary .fixtures/ai_providers.json and .fixtures/xero_apps.json
are always deleted.
In `@scripts/server/release-utils.sh`:
- Around line 38-39: The prefix-match branch in release-utils.sh is bypassing
the same release validation used by release_complete, so update the lookup logic
around the prefix match handling to require the .release-sha identity check
before accepting a directory. Reuse the release_complete validation path or
apply the same check in the matching logic that returns basename for matches[0],
so only valid completed releases are selected and stale/corrupt prefix matches
are rejected.
---
Nitpick comments:
In `@apps/workflow/tests/test_build_id.py`:
- Around line 32-40: Add a test for the `.release-sha` branch in
ReadBuildIdTests so the new fallback path is covered. Extend test_build_id.py by
mocking the environment without DOCKETWORKS_BUILD_SHA, then patching the file
access used by read_build_id() to make `.release-sha` return a valid SHA and
asserting that value is returned. Use the existing read_build_id() and
ReadBuildIdTests symbols to locate the fallback logic, and keep the current env
validation tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edf2b137-6d45-4157-8c69-07f772d9f8f5
📒 Files selected for processing (8)
apps/workflow/tests/test_build_id.pydocketworks/settings.pyscripts/predeploy_backup.shscripts/predeploy_rollback.shscripts/server/README.mdscripts/server/deploy.shscripts/server/instance.shscripts/server/release-utils.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/predeploy_backup.sh
- scripts/predeploy_rollback.sh
- scripts/server/README.md
Comment on lines
+44
to
+47
| return _validate_sha( | ||
| subprocess.run( | ||
| ["git", "rev-parse", "HEAD"], | ||
| cwd=BASE_DIR, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden git invocation against PATH hijacking
At Line 46, subprocess.run uses a partial executable name ("git"). If PATH is compromised, startup could execute the wrong binary. Resolve and pin the executable path first, then invoke that path.
Suggested fix
+import shutil
+
+GIT_BIN = shutil.which("git")
+if not GIT_BIN:
+ raise ImproperlyConfigured("git executable not found in PATH")
+
def read_build_id() -> str:
@@
return _validate_sha(
subprocess.run(
- ["git", "rev-parse", "HEAD"],
+ [GIT_BIN, "rev-parse", "HEAD"],
cwd=BASE_DIR,
check=True,
capture_output=True,
text=True,
).stdout,🧰 Tools
🪛 ast-grep (0.44.0)
[error] 44-50: Command coming from incoming request
Context: subprocess.run(
["git", "rev-parse", "HEAD"],
cwd=BASE_DIR,
check=True,
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.18)
[error] 46-46: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docketworks/settings.py` around lines 44 - 47, The git SHA lookup in the
settings module relies on subprocess.run with the bare "git" executable name,
which can be hijacked via PATH. Update the logic around the SHA validation
helper that calls git rev-parse HEAD to resolve the full git executable path
first and then use that pinned path in subprocess.run, keeping the rest of the
_validate_sha flow unchanged.
Source: Linters/SAST tools
Comment on lines
+91
to
+97
| # Build directly at the final path. .complete (written last) is the only | ||
| # completion gate, so an interrupted build leaves an incomplete, unreferenced | ||
| # dir that we clear here and rebuild. Building in place (not build-then-mv) | ||
| # keeps the venv's console-script shebangs valid — a moved venv is not | ||
| # relocatable. Serial deploys mean there is no concurrent build to race. | ||
| rm -rf "$release_dir" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all ensure_release call paths are serialized.
# Expected: deploy.sh and instance.sh either share the same lock before calling
# ensure_release, or ensure_release itself has a release-scoped flock around
# rm/build/touch .complete.
rg -n -C 8 '\bensure_release\b|\.deploy\.lock|\bflock\b' \
scripts/server/instance.sh \
scripts/server/deploy.sh \
scripts/server/release-utils.shRepository: corrin/docketworks
Length of output: 4657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant release helper and the call sites around ensure_release
# to determine whether they share the same deployment lock or any equivalent
# serialization around release creation.
printf '\n== release-utils.sh ==\n'
sed -n '1,180p' scripts/server/release-utils.sh
printf '\n== deploy.sh (lock + ensure_release) ==\n'
sed -n '1,330p' scripts/server/deploy.sh | sed -n '1,80p;280,300p'
printf '\n== instance.sh (lock + ensure_release) ==\n'
sed -n '1,560p' scripts/server/instance.sh | sed -n '1,120p;470,520p'Repository: corrin/docketworks
Length of output: 15589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant release helper and the call sites around ensure_release
# to determine whether they share the same deployment lock or any equivalent
# serialization around release creation.
printf '\n== release-utils.sh ==\n'
sed -n '1,180p' scripts/server/release-utils.sh
printf '\n== deploy.sh (lock + ensure_release) ==\n'
sed -n '1,330p' scripts/server/deploy.sh | sed -n '1,80p;280,300p'
printf '\n== instance.sh (lock + ensure_release) ==\n'
sed -n '1,560p' scripts/server/instance.sh | sed -n '1,120p;470,520p'Repository: corrin/docketworks
Length of output: 15589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any locking/serialization in instance.sh and how ensure_release is reached.
rg -n -C 4 '\bflock\b|\.lock\b|ensure_release\b|current release|bootstrap|deploy' scripts/server/instance.shRepository: corrin/docketworks
Length of output: 3878
Guard ensure_release with a release-scoped lock. deploy.sh serializes this helper with .deploy.lock, but instance.sh calls the same in-place build path without that lock. Two concurrent instance.sh runs for the same SHA can remove and rebuild the canonical release directory out from under each other. Move the lock into ensure_release or acquire it at every caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/server/release-utils.sh` around lines 91 - 97, `ensure_release`
currently performs an in-place rebuild without its own synchronization, so
concurrent `instance.sh` invocations can delete or rebuild the same release
directory at the same time. Add a release-scoped lock inside `ensure_release`
itself, or make sure every caller acquires the same lock before entering that
path; use the existing `rm -rf "$release_dir"`/build flow in `release-utils.sh`
as the place to wrap with locking.
…creation Revert ca69148's .bootstrap-complete marker gate (and fail-loud branch) back to the original .env-based NEEDS_APP_BOOTSTRAP gate — the marker machinery wasn't worth the friction for a minor instance-creation retry edge case. Instance creation called setup_dev_logins.py, which also resets ALL staff passwords — a restore-prod-to-nonprod scrub that must never be triggered by provisioning. Add an --admin-only flag that ensures the default admin exists without touching staff passwords, and use it from instance.sh. Plain invocation (full reset) is unchanged for the restore flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 26, 2026
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.
Summary
/opt/docketworks/releases/<sha>directories, with each instance pointingcurrentat the selected release.node_modules, and prune unreferenced releases.Validation
./.venv/bin/python -m pytest apps/workflow/tests/test_xero_instance_templates.py— 14 passed.new: 0, frontend unit tests42 passed / 263 tests, frontend type-check, production build, typed-router build check, and workflow formatting.Notes
rrweb-playerbrowser externalization,pdf-vue3direct eval, and large chunks.Summary by CodeRabbit
currentrelease for code, dependencies, and static assets..release-sha, with early failure on malformed values.current; deployment can clean up unreferenced releases and verify typed-router drift.