Release process hardening plan - #406
Merged
Merged
Conversation
Make docs/updating.md the single source of truth for deploying a new version; reduce server_setup.md (Part D + Part F) and scripts/server/README.md to pointers. Fix the credentials-file path in instance-setup-demo.md and switch restore-prod-to-nonprod.md and instance-setup-production.md to the reconfigure subcommand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds ChangesInstance reconfigure flow
Determinism and workflow checks
Sequence Diagram(s)sequenceDiagram
participant Operator
participant instance.sh
participant common.sh
participant Django
Operator->>instance.sh: reconfigure <client> <env>
instance.sh->>common.sh: ensure_config_dir() / require_root_owned_credentials_file()
instance.sh->>instance.sh: detect existing instance and parse options
instance.sh->>instance.sh: render .env and frontend/.env
instance.sh->>Django: load missing fixtures when DB state requires it
instance.sh-->>Operator: reconfigured successfully
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
190-207: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffConsider hardening GitHub Actions security posture.
The new job follows the existing pattern in this workflow, but static analysis identifies several security gaps that apply to both this and other jobs:
- Unpinned actions (lines 197-198): Using
@v7and@v6tags instead of commit SHA pins allows potential supply-chain attacks if tags are moved.- Missing
persist-credentials: false(line 197): Allows the GitHub token to persist in git config during checkout.- No permissions block (lines 190-206): Job inherits default workflow permissions rather than declaring minimal required permissions.
Since these issues exist in other CI jobs (e.g.,
frontend-type-checkat lines 174-189), consider addressing them repository-wide rather than just for this new job to maintain consistency.🛡️ Example hardening for this job
frontend-typed-router-determinism: name: Frontend Typed Router Determinism runs-on: ubuntu-latest + permissions: + contents: read defaults: run: working-directory: frontend steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v7.2.0 + with: + persist-credentials: false - - uses: actions/setup-node@v6 + - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v6.1.0 with: node-version: 22🤖 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 @.github/workflows/ci.yml around lines 190 - 207, The `frontend-typed-router-determinism` job has three security issues that should be addressed. First, replace the unpinned action version tags `@v7` for `actions/checkout` and `@v6` for `actions/setup-node` with specific commit SHA pins instead of major version tags to prevent supply-chain attacks. Second, add `persist-credentials: false` to the checkout action configuration to prevent the GitHub token from persisting in git config. Third, add a `permissions` block at the job level (after the `runs-on` line) that explicitly declares minimal required permissions instead of inheriting the default workflow permissions. Consider applying these same hardening measures to other existing jobs in the workflow like `frontend-type-check` for consistency across all CI jobs.Source: Linters/SAST tools
🤖 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 `@scripts/server/instance.sh`:
- Around line 432-433: The chown command on line 433 is transferring ownership
of the credentials file to INSTANCE_USER, creating a privilege escalation
vulnerability since the script later sources this file during root execution,
allowing INSTANCE_USER to edit the credentials and inject malicious content.
Remove or modify the chown statement so that the credentials file ownership
remains with a privileged user (such as root) instead of being transferred to
INSTANCE_USER, while keeping the chmod 600 permission restriction in place.
- Around line 95-106: The read_env_value() function has a security vulnerability
where it sources the .env file using bash -c with 'set -a; source "$1"...',
which executes any shell content in the writable .env file as root. Replace the
sourcing approach with a safe parsing method that reads the .env file as plain
text without executing it, such as using grep to extract the variable assignment
line and then safely parsing the value without any shell command execution.
In `@scripts/server/README.md`:
- Around line 98-103: There is a conflict in the credentials documentation
regarding where Xero credentials (Client ID, Client Secret, and webhook signing
key) should be stored. Line 98 instructs to place these into the instance
credentials file, but other parts of this section apparently state these values
do not go in the credentials file. Review the entire credentials section to
identify the conflicting statements and unify them to establish a single, clear
source of truth about where Xero credentials should be placed, ensuring
operators have unambiguous instructions to avoid misconfiguration.
In `@scripts/server/server-setup.sh`:
- Around line 699-706: The REQUIRED_NODE_MAJOR variable assignment on line 699
uses tr -d 'v[:space:]' which removes the 'v' and whitespace characters but
fails to extract only the major version number from the .nvmrc file. When .nvmrc
contains a full version like "18.2.0" or "v18.2.0", it leaves the entire version
string instead of just "18", causing the comparison with CURRENT_NODE_MAJOR to
fail incorrectly. Replace the tr command with a sed expression that properly
extracts only the major version number (the first numeric component) from the
.nvmrc content, handling formats like "18", "v18", "18.2.0", and "v18.2.0".
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 190-207: The `frontend-typed-router-determinism` job has three
security issues that should be addressed. First, replace the unpinned action
version tags `@v7` for `actions/checkout` and `@v6` for `actions/setup-node`
with specific commit SHA pins instead of major version tags to prevent
supply-chain attacks. Second, add `persist-credentials: false` to the checkout
action configuration to prevent the GitHub token from persisting in git config.
Third, add a `permissions` block at the job level (after the `runs-on` line)
that explicitly declares minimal required permissions instead of inheriting the
default workflow permissions. Consider applying these same hardening measures to
other existing jobs in the workflow like `frontend-type-check` for consistency
across all CI jobs.
🪄 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: 9af02e8a-e5ef-42c8-a0e0-f979a7dd2e2c
📒 Files selected for processing (19)
.github/workflows/ci.yml.github/workflows/stale.yml.gitignore.pre-commit-config.yamlapps/workflow/tests/test_xero_instance_templates.pydocs/instance-setup-demo.mddocs/instance-setup-production.mddocs/restore-prod-to-nonprod.mddocs/server_setup.mddocs/updating.mdfrontend/package.jsonrequirements.txtscripts/check_requirements.shscripts/generate_requirements.shscripts/pre-pushscripts/server/README.mdscripts/server/deploy.shscripts/server/instance.shscripts/server/server-setup.sh
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the server deployment/provisioning workflow by adding determinism checks around generated frontend artifacts, making instance provisioning convergent via a new instance.sh reconfigure path, and consolidating operator docs into a single deploy runbook.
Changes:
- Added CI + server-side guards to ensure
frontend/src/typed-router.d.tsstays deterministic across build/deploy. - Refactored
instance.shto support safe re-runs (reconfigure) with.envre-rendering and guarded fixture seeding. - Replaced requirements autogeneration with deterministic verification scripts + updated hooks/docs/runbooks.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds a typed-router determinism job to CI. |
.github/workflows/stale.yml |
Normalizes quoting/formatting in stale PR workflow. |
.gitignore |
Ignores temporary .env render files (.env.tmp.*). |
.pre-commit-config.yaml |
Swaps requirements autogen for a deterministic “check” hook; adds workflow-format hook. |
apps/workflow/tests/test_xero_instance_templates.py |
Adds tests asserting convergent instance.sh behavior and new subcommand wiring. |
docs/instance-setup-demo.md |
Updates credentials path documentation. |
docs/instance-setup-production.md |
Updates credentials path + recommends reconfigure after credential edits. |
docs/restore-prod-to-nonprod.md |
Updates guidance to use instance.sh reconfigure for existing instances. |
docs/server_setup.md |
Updates onboarding commands and points to unified updating runbook. |
docs/updating.md |
Becomes the single deploy/operator runbook with deploy.sh + when to run reconfigure. |
frontend/package.json |
Adds check:typed-router:build and check:workflow-format scripts. |
requirements.txt |
Regenerated deterministically from poetry.lock. |
scripts/check_requirements.sh |
New: verifies requirements.txt matches deterministic poetry export. |
scripts/generate_requirements.sh |
New: regenerates requirements.txt deterministically from poetry.lock. |
scripts/pre-push |
Adds requirements, typed-router build determinism, and workflow-format checks to pre-push. |
scripts/server/README.md |
Updates instance commands/docs pointers; references unified deploy runbook. |
scripts/server/deploy.sh |
Adds Node major check, uses npm ci, runs typed-router checks and detects post-build diffs. |
scripts/server/instance.sh |
Adds reconfigure, extracts shared configure logic, re-renders .env, and guards fixture seeding. |
scripts/server/server-setup.sh |
Adds Node major check and switches shared install to npm ci --include=dev. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/server/README.md (1)
99-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe
XERO_DEFAULT_USER_IDworkflow is blocked by the current create contract.This step says to create the instance first and only then populate
XERO_DEFAULT_USER_ID, butrequire_instance_credentials()currently refuses bothcreateandreconfigurewhen that variable is empty. Either makeXERO_DEFAULT_USER_IDoptional for the initial create path, or rewrite this runbook to use a bootstrap value that exists before creation.🤖 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 99, The XERO_DEFAULT_USER_ID setup flow is currently inconsistent with require_instance_credentials(), since it blocks both create and reconfigure when the value is missing. Update the credential validation logic around require_instance_credentials() so the initial create path can proceed without XERO_DEFAULT_USER_ID, or adjust the create/reconfigure contract to accept a bootstrap value that can be supplied before instance creation. Ensure the behavior matches the runbook referenced by XERO_DEFAULT_USER_ID and the instance creation/reconfigure flow.
🤖 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.
Outside diff comments:
In `@scripts/server/README.md`:
- Line 99: The XERO_DEFAULT_USER_ID setup flow is currently inconsistent with
require_instance_credentials(), since it blocks both create and reconfigure when
the value is missing. Update the credential validation logic around
require_instance_credentials() so the initial create path can proceed without
XERO_DEFAULT_USER_ID, or adjust the create/reconfigure contract to accept a
bootstrap value that can be supplied before instance creation. Ensure the
behavior matches the runbook referenced by XERO_DEFAULT_USER_ID and the instance
creation/reconfigure flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c74cdab0-f5a1-4319-b89b-cc0cb4c223c5
📒 Files selected for processing (10)
apps/workflow/tests/test_xero_instance_templates.pydocs/client_onboarding.mddocs/instance-setup-demo.mddocs/instance-setup-production.mddocs/server_setup.mdscripts/server/README.mdscripts/server/common.shscripts/server/deploy.shscripts/server/instance.shscripts/server/server-setup.sh
✅ Files skipped from review due to trivial changes (3)
- docs/instance-setup-demo.md
- docs/instance-setup-production.md
- docs/server_setup.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/server/server-setup.sh
- scripts/server/deploy.sh
- apps/workflow/tests/test_xero_instance_templates.py
- scripts/server/instance.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/instance-setup-production.md (1)
113-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRenumber the remaining setup steps.
After the old Step 7 was removed, the guide now jumps from Step 6 to Step 8. That makes the runbook look incomplete and harder to follow; please renumber this heading and the subsequent ones to keep the sequence contiguous.
🤖 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 `@docs/instance-setup-production.md` at line 113, The setup guide has a numbering gap after the removed Step 7, so the remaining headings need to be renumbered to stay sequential. Update the Step 8 heading in the instance setup production doc and adjust the following step titles in that runbook so the sequence continues contiguously from Step 6 without skipping a number.
🤖 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.
Outside diff comments:
In `@docs/instance-setup-production.md`:
- Line 113: The setup guide has a numbering gap after the removed Step 7, so the
remaining headings need to be renumbered to stay sequential. Update the Step 8
heading in the instance setup production doc and adjust the following step
titles in that runbook so the sequence continues contiguously from Step 6
without skipping a number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a50b7ea-839f-460e-9f6c-f29dbcc428d2
📒 Files selected for processing (9)
apps/workflow/tests/test_xero_instance_templates.pydocs/instance-setup-demo.mddocs/instance-setup-production.mdscripts/server/README.mdscripts/server/common.shscripts/server/deploy.shscripts/server/instance.shscripts/server/server-setup.shscripts/server/templates/credentials-instance.template
✅ Files skipped from review due to trivial changes (1)
- scripts/server/templates/credentials-instance.template
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/server/deploy.sh
- scripts/server/server-setup.sh
- scripts/server/README.md
- scripts/server/instance.sh
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.
"## \ud83d\udcdd Description\n\nThis PR hardens the deployment process across three axes: determinism guards (so deploys don't fail from stale generated files), convergent instance provisioning (so
instance.shcan be re-run safely), and documentation consolidation (so there's one deploy runbook).\n\nThe context:deploy.shwas failing on every run because a prior deploy's frontend build regeneratedfrontend/src/typed-router.d.tsslightly differently from what was committed, making the git tree dirty. The clean-tree check on the next deploy then aborted. Separately,instance.sh createwas not truly convergent \u2014 re-running it could fail on the AI providers fixture (already deleted), skip.envwhen credentials changed, and do heavy operations unconditionally.\n\n## \ud83d\udd17 Related Jira Work Item\n\nJira: KAN-257\n\n## \ud83d\ude80 Changes\n\n### Determinism guards\n\n- CI:frontend-typed-router-determinismjob \u2014 checks thatnpm run check:typed-routerpasses AND thatnpm run builddoes not modifysrc/typed-router.d.ts\n-deploy.sh: runsnpm run check:typed-routerbefore the build, then verifiesgit diff --exit-code -- frontend/src/typed-router.d.tsafter the build \u2014 if the server-generated output diverges, the instance fails with a clear error instead of silently dirtying the tree for the next deploy\n-deploy.sh: validates Node major version matchesfrontend/.nvmrcbeforenpm ci\n-deploy.sh+server-setup.sh:npm install\u2192npm ci --include=devfor deterministic installs\n- Pre-push hook: now runscheck:typed-router:buildandcheck:workflow-formatin addition to existing checks; also runscheck_requirements.sh\n-requirements.txt: pre-commit no longer auto-regenerates it \u2014 it now checks viacheck_requirements.shand errors if stale (prevents drift betweenpoetry.lockand committedrequirements.txt)\n- Pre-commit: newfrontend-workflow-formathook checks GitHub Actions YAML formatting\n\n### Convergent instance provisioning (instance.sh)\n\n- Newreconfiguresubcommand \u2014 re-renders all config (.env, systemd units, nginx, rclone) without DB bootstrap (no migrate, no admin user creation, no frontend build). Safe to run after editing credentials. Used in the updated onboarding docs instead of "re-run create".\n-createandreconfiguresharedo_configure\u2014 extracted common logic;allow_seedflag controls whether--seedis accepted (refused for existing checkouts)\n-.envalways re-rendered \u2014 newly generated secrets (DB_PASSWORD, SECRET_KEY, etc.) are preserved from the existing.envwhen present; credentials-side values (Xero keys, API keys) always update from the credentials file\n- Git pull removed from existing-checkout path \u2014instance.shonly ensures the origin remote points at the local repo;deploy.showns the pull step\n- AI providers / Xero apps guarded \u2014loaddataonly runs whenAIProvider.objects.exists()/XeroApp.objects.exists()is false; safe on re-run even after fixture file deletion\n---seedrejected for existing instances \u2014 explicit error instead of silently clobbering\n-chownno longer recursive \u2014 instance dir gets 750; subdirs get explicitchowninstead of the-Rblast\n- New directories created:phone-recordings/,session-replays/(gated to instance user, mode 700)\n\n### Documentation consolidation\n\n-docs/updating.mdis now the single deploy runbook \u2014 contains both thedeploy.shcommand and when to runinstance.sh reconfigure\n-docs/server_setup.mdandscripts/server/README.mdreduced to pointers referencingupdating.md\n- Fixed stale file paths ininstance-setup-demo.mdandinstance-setup-production.md(credentials file path, reconfigure subcommand)\n-restore-prod-to-nonprod.md: referencesinstance.sh reconfigureinstead of "re-run create"\n\n---\n\n## Files changed\n\n| File | Change |\n|------|--------|\n|.github/workflows/ci.yml| +frontend-typed-router-determinism job |\n|.github/workflows/stale.yml| Quote normalization |\n|.gitignore| +.env.tmp.*|\n|.pre-commit-config.yaml| requirements check \u2192 verify; +workflow-format hook |\n|apps/workflow/tests/test_xero_instance_templates.py| +5 tests for reconfigure and convergent behaviour |\n|docs/instance-setup-demo.md| Fix credentials path |\n|docs/instance-setup-production.md| reconfigure \u2192 recommended re-run command |\n|docs/restore-prod-to-nonprod.md| instance.sh reconfigure reference |\n|docs/server_setup.md| Pointers to updating.md |\n|docs/updating.md| Now the deploy runbook |\n|frontend/package.json| +check:typed-router:build, +check:workflow-format |\n|requirements.txt| Regenerated deterministically |\n|scripts/check_requirements.sh| New: verify requirements.txt matches poetry.lock |\n|scripts/generate_requirements.sh| New: deterministic requirements.txt generation |\n|scripts/pre-push| +requirements check, +typed-router:build check, +workflow-format check |\n|scripts/server/README.md| Pointer to updating.md; reconfigure in command list |\n|scripts/server/deploy.sh| check:typed-router, typed-router post-build diff, node version check, npm ci |\n|scripts/server/instance.sh| reconfigure, render functions, convergent guards, no-recursive chown |\n|scripts/server/server-setup.sh| Node version check, npm ci |\n\n## \u2705 Checklist\n\nQuality & Formatting\n- [x] Passes Prettier\n- [x] Passes ESLint with zero warnings\n- [x] Passes mypy (full-strict)\n- [x] New backend unit tests included (5 tests for instance.sh convergence)\n- [x] Pre-push hook updated with new checks\n\nDefinition of Done\n- [x]check:typed-router:buildpasses \u2014 build does not modify typed-router.d.ts\n- [x]check_requirements.shpasses \u2014 committed requirements.txt matches poetry.lock\n- [x]instance.sh reconfigurereruns successfully on an existing instance\n- [x]deploy.shno longer dirties the instance git tree after building\n- [x] All doc cross-references updated (credentials path, reconfigure subcommand)\n"Summary by CodeRabbit
New Features
instance.sh reconfigureflow to update existing instances without full recreation, preserving generated secrets (and rejecting--seedfor existing checkouts)./opt/docketworks/config/<client>-<env>.credentials.envwith strict root ownership/permissions.Bug Fixes
Documentation
reconfigurestep.Tests / CI / Chores
requirements.txtgeneration/check tooling.