Continuous deployment: pinned data submodules, release bundle, VPS deploy - #21
Merged
Conversation
Records the deployment architecture: data pinned as git submodules, a release bundle (wheel + offline wheelhouse + the three data trees + a manifest) scp'd to the VPS, and a synchronous deploy script run by a dedicated non-chrooted user under two narrow sudoers rules. Rejects Docker (host Python 3.12 is pinnable, so a container would only add a registry credential able to pull the private corpus) and rejects pip-install-from-git (puts a second read path to martyrology-texts on the server, and requires packaging metadata in a repo curated by another group). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six TDD tasks: manifest reader and /healthz, submodule pinning, bundle builder, on-VPS deploy script, VPS provisioning, release workflow. Amends the spec to split the service environment into a root-only secret file and a deploy-readable runtime file. deploy.sh must read the live port to poll /healthz before deciding whether to roll back, and it must not be able to read Zitadel/OpenFGA credentials to do so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plan's own test called assemble() without write_manifest(), which the implementer resolved by making assemble() fabricate a placeholder manifest. That ships a bundle with empty api_commit and empty data — it passes deploy.sh's manifest check and serves with no audit trail, defeating the guarantee the manifest exists to provide. Fix the test; fail the build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A tarball with no manifest.json would still pass deploy.sh's manifest check and serve with an empty audit trail (api_commit/data all null), defeating the one guarantee the manifest exists to provide. Replace the placeholder-manifest fallback in assemble() with a hard FileNotFoundError, and cover it with test_assemble_refuses_a_staging_tree_with_no_manifest. The prior "manifest exists" test now calls write_manifest() first, as the real CI flow (main()) already does.
…n, link screening, offline venv, and rollback gaps Round-1 review found the brief's deploy.sh had two critical and five important defects, all inherited verbatim from the plan. Fixes: - Path-traversal guard failed open: piping `tar -tzf | grep -q` let grep exit on first match and SIGPIPE the still-writing tar, making the pipeline non-zero and skipping `die` under `pipefail`. Now `tar -tvzf` output is captured to a variable first, then screened twice. - Redeploying the currently active version wiped it via `rm -rf` before the replacement was verified, with no useful rollback target left. Now refused outright before extraction. - Symlink/hardlink targets were never screened (tar -t only lists member names, not link targets); added a dedicated check against the "name -> target" column from the verbose listing. - `pip install --upgrade pip` in the "offline" venv build reached PyPI; removed, the venv's bundled pip is sufficient for --no-index installs. - A failed `systemctl restart`, missing runtime.env, or unset MARTYROLOGY_PORT after flipping `current` left the flip stranded with no rollback, since set -e exits before the old manual rollback check was reached. Now an EXIT trap is armed right after the flip and disarmed only once the live health check passes, so any failure in that window (including ones set -e exits on immediately) restores the previous release. - `sha256sum -c` verifies whatever filename the .sha256 file names, not the bundle itself; now the digest and the named filename are both checked explicitly against the bundle. - Rollback silently reported success without checking the previous release still exists, checking the restart succeeded, or re-polling health. - --dry-run was only honoured as $1; now accepted in any argument position, with unrecognised arguments rejected. - smoke.log was written into the release tree; now a mktemp path. - Reworded the header comment's "nothing from the payload is ever executed" claim, which pip-installing and running the bundle's own wheels contradicts. Adds regression tests for all of the above, including a multi-member archive (traversal member plus 20k filler members) that reproduces the SIGPIPE-masking bug directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SUBMODULE_TOKEN is a fine-grained PAT with a hard expiry; when it lapses actions/checkout fails at the vendor/texts submodule, and since deploys only fire on published releases that surfaces mid-release. Reads the expiry from GitHub's GitHub-Authentication-Token-Expiration response header rather than a hardcoded date, so it survives rotation, and doubles as a liveness check for a revoked token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ting, plus rollback robustness
Round-2 review found the round-1 SIGPIPE fix (capturing tar -tvzf output
before screening it) had regressed the member-name traversal check itself:
screening `awk '{print $NF}'` over a verbose listing line, instead of the
raw member name, only screens a symlink's target (never its own name, since
$NF on a "name -> target" line is the target) and only the last
whitespace-delimited token of any name containing a space.
Fixed by capturing two things instead of one:
- BUNDLE_NAMES (`tar -tzf`, one name per line) for the whole-line,
unsplit name screen.
- BUNDLE_MEMBERS (`tar -tvzf`) kept only for the link-target screen, which
needs the verbose "-> target" column tar -t does not print.
Both captures still avoid piping into grep -q, preserving the round-1
SIGPIPE fix. Added three isolated single-member regression tests (a
combined archive masks the failure, which is how this slipped through
round 1): a symlink whose own name traverses but whose target does not, a
traversal member with a space in its name, and an absolute member with a
space in its name.
Also folded in three robustness fixes to the rollback trap built in round
1:
- rollback_on_failure now guards its own ln/mv calls so a failure there
cannot abort the trap under set -e before it reaches the final
`exit "$status"`, losing the original diagnostic.
- The smoke-check EXIT trap is now armed immediately after
`SMOKE_LOG=$(mktemp)`, before the background uvicorn is started, so the
temp file cannot leak if something fails in between.
- The post-flip rollback trap is now registered for EXIT, INT, and TERM
(not just EXIT), since a signal during the flip window would otherwise
leave `current` half-flipped with no trap to catch it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nk/dotdot screening
Round-3 review reproduced a new Important finding in the round-2 rollback
trap: on SIGTERM delivered to the deploy.sh process alone (a CI
cancellation, an ssh disconnect), bash defers running an INT/TERM trap
until the current foreground command completes. If that command (the
`sleep 1` inside wait_healthy) finishes normally in the interim, $? in the
trap is 0 -- not signal-derived -- so rollback_on_failure's early-return
on status 0 skipped rollback entirely and the script exited 0 with
`current` left pointing at an unverified release. Before round 2 the same
signal reliably produced exit 143 (bash's default disposition) and the
caller saw a failure; round 2's `trap rollback_on_failure EXIT INT TERM`
made that specific case silently look like success instead.
Fixed by giving INT/TERM their own explicit status so a signal can never
present as 0:
trap rollback_on_failure EXIT
trap 'rollback_on_failure 143' INT TERM
`local status="${1:-$?}"` in the handler uses the explicit argument when
given (a signal) and falls back to $? otherwise (a plain command
failure). Verified deterministically with a standalone harness (not
flaky -- matches bash's documented "defer trap until the current
foreground command completes" behavior): old wiring exits 0 with no
rollback message on TERM sent to the process alone; new wiring exits 143
and rolls back.
Also folded in three more findings, flagged as pre-existing minors but
cheap to fix in code already being touched:
- The link-target screen only matched a symlink's "-> target" rendering;
a hardlink's "link to target" rendering (from `tar -tv`) was not
screened at all. Extended the regex to match both. (GNU tar 1.35, as
installed here, proactively normalizes hard link targets during listing
itself -- stripping a leading "/" and collapsing every ".." before the
line is ever displayed -- so this is defense-in-depth over that specific
tar implementation's own hardening, not a gap it currently leaves open
on this system; documented as such rather than overstated.)
- A symlink/hardlink target of exactly ".." (or ending in "..' with no
trailing slash, e.g. "a/..") was not caught -- the old pattern required
a trailing "/" after "..". Anchored the pattern to also match ".." at
end-of-string.
- `tar -tzf`/`tar -tvzf` failing under `set -e` (a corrupt or truncated
bundle) aborted with a bare non-zero exit and no diagnostic. Both
captures now die with a message naming the bundle.
- The smoke-check EXIT trap is now also registered for INT/TERM, so a
signal during the smoke phase kills the smoke uvicorn and removes
SMOKE_LOG instead of orphaning both.
New tests, most exercising the real script end-to-end; two documented as
white-box/harness tests where the real path is unreachable, per the
coordinator's own guidance for exactly that situation, with what each
does and does not cover stated explicitly in its docstring:
- Real, end-to-end: bare ".." and "a/.." symlink-target rejection, a
benign hardlink acceptance (positive control), and a corrupt-bundle
diagnostic.
- White-box: a hardlink-target rejection test that extracts the actual
link-screening regex from deploy.sh's source and runs it through the
real `grep -E` binary against a synthetic listing line, since no real
archive built with this system's GNU tar can produce the dangerous text
the regex is meant to catch (tar already neutralizes it first).
- Harness: a signal-handling regression test that extracts the actual
rollback_on_failure() function body and its two trap-arming lines from
deploy.sh's current source (not hand-duplicated, to avoid drift) and
splices them into a minimal standalone script, then sends SIGTERM to
it directly. Verified to genuinely fail when the fix is reverted
(confirmed by temporarily restoring the old single-trap wiring and
re-running this test alone, then restoring the fix).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… links pre-tar-sanitization The smoke phase registered one `trap '<cleanup>' EXIT INT TERM`. A bash signal trap returns control to the script rather than terminating it, so a TERM anywhere in the smoke window ran the cleanup and then carried on into the flip, the systemctl restart and the prune — finishing a cancelled deploy and exiting 0. Before that wiring existed bash's default disposition exited 143, so the trap had turned a loud failure into a silent success. Split it the way the rollback trap already is: a named smoke_cleanup that clears its own traps first, `trap smoke_cleanup EXIT`, and `trap 'smoke_cleanup; exit 143' INT TERM`. Also take the two tar *listing* captures with -P. Without it GNU tar rewrites the listing before the screen sees it — a hard link target of `/etc/passwd` or `../../etc/passwd` lists as a harmless `etc/passwd` — so the link guard was depending on tar's own sanitization, which is exactly what these checks exist not to depend on. Extraction deliberately keeps running without -P, so tar's stripping stays as a last line of defense. The hardlink test is now a real black-box test through the script instead of a white-box regex test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- chmod the APP_DIR subdirectories explicitly (not just APP_DIR itself) so the service account's read access doesn't depend on the deploy user's umask or CI tar member modes; add a post-provision self-check that fails loudly if the service account can't traverse APP_DIR or read runtime.env. - Check for python3.12, curl, tar, sha256sum, and python3.12-venv up front so a missing prerequisite fails at provisioning time, not mid-deploy. - Give the service account no login shell (--system --shell /usr/sbin/nologin --no-create-home), matching the spec and the script's own header comment; the deploy account keeps its shell. - Verify /etc/sudoers pulls in /etc/sudoers.d before installing the sudoers drop-in, so a missing includedir fails here instead of at the first deploy's sudo call. - Print the port actually in force (read back from runtime.env) instead of the process default on re-runs, note that overriding it needs `sudo -E`, and warn in the banner that an unfilled secrets file leaves auth/authz disabled.
The martyrology service account shares no group with martyrology-deploy, so its ability to traverse and read releases/<version>/ depends entirely on world permission bits. Provisioning now locks down APP_DIR and its immediate subdirectories, but the release tree itself is created here, taking its modes from this script's umask and from CI-runner tar member modes -- neither guaranteed permissive. On a hardened host (UMASK 027, pam_umask) the deploy reports success and systemd then fails ExecStart with Permission denied. chmod -R a+rX "$RELEASE" after the venv is built normalises this; capital X keeps data files from becoming spuriously executable. A follow-up self-check (find + die) proves the chmod actually stuck, without requiring any sudo grant beyond what's already provisioned.
Several code blocks in the plan contained defects that review caught only after implementation, so re-executing it verbatim would reintroduce them. Records what each defect was, so the document remains a useful history rather than a trap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uard The release tree was left world-readable (`chmod -R a+rX`, plus 0755 directories from provisioning), which published the licensed martyrology-texts corpus under releases/<v>/data/texts to every local account on a Plesk host, where each hosted subscription runs its own non-chrooted uid. That is the disclosure the private-submodule architecture exists to prevent. The world bits were there for a real reason: the service account shared no group with the deploy user, so a hardened umask left systemd unable to read its own release. Fix both halves instead of trading one for the other. setup-vps-deploy-user.sh now creates the martyrology group explicitly, pins the service account to it, adds martyrology-deploy to it, owns the tree martyrology-deploy:martyrology and drops every "other" bit: 0750 on $APP_DIR/bin/config, 2750 on releases/ so new release directories inherit the group, 0700 on incoming/ (which holds the corpus in bundle form), 0640 on runtime.env. Its recursive chmod and the runtime.env chown/chmod run unconditionally, so a re-run retracts the world bits from trees an earlier version already wrote. deploy.sh chgrp's and chmod's each release to u+rwX,g+rX,o-rwx, and tightens the uploaded bundle to 0600 as soon as its path is verified -- it is only deleted on success, so a failed deploy used to leave a permissive copy behind indefinitely. Both permission self-checks now assert both halves, and each half fails on its own: group bits present, no "other" bit anywhere, and the group really being martyrology. Symlinks are excluded from the mode arms, since a symlink's mode is inert and chmod -R does not follow it. The provisioning script additionally proves the deploy user is in the group and that the service account cannot read incoming/. Also in the workflow: APP_DIR is exported to the remote (deploy.sh otherwise fell back to its own /opt/martyrology default, so any other vars.APP_DIR uploaded to one directory and looked in another); a release tag that does not match the pyproject.toml version now fails loudly instead of building a bundle the host silently refuses; and the staged shape check counts staging/data/editions/*/01.json, the one tree whose presence previously masked the private corpus going missing. Finally, deploy.sh arms the rollback trap before the symlink flip rather than after, closing the window in which a TERM left `current` flipped with the service never restarted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves the adjudicated residuals out of untracked scratch and into the repo's follow-ups doc, so they survive the working directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds continuous deployment for the API. The change packages pinned submodule data into checksummed release bundles, provisions a VPS runtime, validates and activates releases with rollback, exposes deployment health metadata, and adds GitHub Actions automation. ChangesContinuous deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant VPS
participant deploy.sh
participant Uvicorn
ReleaseWorkflow->>VPS: Upload bundle and checksum
VPS->>deploy.sh: Invoke versioned deployment
deploy.sh->>Uvicorn: Run isolated smoke check
Uvicorn-->>deploy.sh: Return health and edition data
deploy.sh->>VPS: Activate current release
VPS->>Uvicorn: Restart service
Uvicorn-->>deploy.sh: Return live health
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
.github/workflows/deploy.yml (2)
75-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
uvin the build step.
pip install uvresolves to whatever version is current at run time. Every action in this workflow is pinned by commit SHA, and the wheelhouse this step produces is the artifact the VPS installs offline. An unpinned build tool makes the one step that resolves and builds dependencies non-reproducible, and it accepts any newly publisheduvrelease without review.Pin an exact version.
♻️ Proposed pin
- pip install uv + pip install 'uv==0.9.5'Replace the version with the one you have validated.
🤖 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/deploy.yml around lines 75 - 82, Pin the uv installation in the “Build wheel and offline wheelhouse” step to an exact, validated version instead of installing the unversioned package. Keep the existing build and wheelhouse commands unchanged.
219-224: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
StrictHostKeyChecking=yesexplicitly onscpandssh.The workflow pins the host key and verifies that the pin covers the target. Neither the
scpcommand here nor thesshcommand at lines 254-257 states the host-key policy, so both rely on the OpenSSH default ofask, which is interpreted as refuse in a non-interactive session. That default is a client-configuration property, not a property of this workflow.State the policy. It also documents the trust model at the call site.
🔒️ Proposed change, apply to both commands
if scp -i ~/.ssh/deploy_key \ + -o StrictHostKeyChecking=yes -o UserKnownHostsFile=~/.ssh/known_hosts \ -o ConnectTimeout=10 -o ServerAliveInterval=15 -o ServerAliveCountMax=2 \ "$BUNDLE" "$BUNDLE.sha256" \ "${VPS_USERNAME}@${VPS_HOST}:${APP_DIR}/incoming/"; then🤖 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/deploy.yml around lines 219 - 224, Update the scp command in the upload retry loop and the ssh command near the deployment step to explicitly pass StrictHostKeyChecking=yes, preserving the existing pinned-host-key verification and other connection options.
🤖 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 @.github/workflows/deploy.yml:
- Around line 150-152: Update the comment above the sha256sum command to remove
the outdated claim that deploy.sh runs sha256sum -c, and instead state that
deploy.sh parses the checksum file and requires field 2 to contain the bundle’s
bare basename. Keep the checksum generation command unchanged.
In @.github/workflows/token-expiry-watch.yml:
- Around line 96-108: Remove the leading indentation from continuation lines in
both multi-line issue-body strings used by open_issue, including the bodies near
the expiry warning and the earlier warning. Preserve the YAML structure and
message content while ensuring each Markdown line begins without shell-string
padding so bold text, inline code, and paragraphs render correctly.
- Around line 44-50: Set an explicit sufficiently high --limit on the gh issue
list command in the issue-deduplication flow before checking existing titles, so
all relevant open warning issues are considered and duplicate expiry issues
cannot be created. Preserve the existing exact-title comparison and issue
creation behavior.
In `@docs/superpowers/plans/2026-08-01-continuous-deployment.md`:
- Line 30: Remove the blank line within the blockquote in the deployment plan so
its quoted lines remain contiguous, resolving the Markdownlint MD028 violation.
In `@docs/superpowers/specs/2026-08-01-continuous-deployment-design.md`:
- Around line 125-133: Language-tag all three fenced blocks in
docs/superpowers/specs/2026-08-01-continuous-deployment-design.md: use text for
the bundle tree at lines 125-133 and filesystem layout at lines 242-260, and use
text or sudoers for the sudoers entry at lines 295-298.
- Around line 3-4: Update the Status field in the continuous-deployment design
document to reflect the post-merge state: mark the design as implemented, or
explicitly note that only the initial VPS rollout remains pending.
In `@scripts/deploy/build_bundle.py`:
- Around line 83-85: Update assemble() so each staging path is added without
recursively re-adding directory contents, using file-only adds or
recursive=False for directories. Add coverage verifying archive member names are
unique and repeated builds produce byte-identical artifacts.
In `@scripts/deploy/deploy.sh`:
- Around line 146-152: Align the release-version validation before bundle
creation with the `deploy.sh` guard: reject PEP 440 prerelease and postrelease
versions when deriving or validating `VERSION` in the workflow and install path,
so only deployable dot-separated numeric versions reach bundle creation. Reuse
the existing version-validation logic or policy consistently rather than
allowing versions that `deploy.sh` will later refuse.
- Around line 217-228: Update the archive validation and release-permission
normalization to clear setuid and setgid bits. In the existing chmod
normalization, include a-s so extracted files cannot retain these mode bits, and
ensure the self-check covering bundle members also rejects setuid/setgid modes
if it validates normalized permissions.
In `@src/martyrology_api/manifest.py`:
- Around line 17-22: Strengthen manifest validation used by load_manifest() so
the data mapping contains all three required repository keys, commit IDs are
exactly 40 hexadecimal characters, file paths are relative, and file digests are
exactly 64 hexadecimal SHA-256 characters. Ensure invalid or missing values
cause manifest loading to reject the manifest, and add rejection tests covering
each validation rule.
In `@tests/test_deploy_script.py`:
- Around line 710-736: Update
test_permission_selfcheck_fails_loudly_when_the_tree_is_not_in_the_service_group
to avoid hard-coding root as the non-service group: derive a group guaranteed
not to own tmp_path at runtime, or skip the test when the process already runs
with root as its primary group. Preserve the assertions that the self-check
fails with “not group-readable” and does not reach the end.
- Around line 807-822: Update `_build_signal_harness` to print a readiness
marker immediately after arming its traps, then change
`test_signal_during_activation_phase_rolls_back` to read and wait for that
marker before calling `proc.send_signal(signal.SIGTERM)`, replacing the fixed
`time.sleep(0.3)` delay while preserving the existing timeout and stderr
assertions.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 75-82: Pin the uv installation in the “Build wheel and offline
wheelhouse” step to an exact, validated version instead of installing the
unversioned package. Keep the existing build and wheelhouse commands unchanged.
- Around line 219-224: Update the scp command in the upload retry loop and the
ssh command near the deployment step to explicitly pass
StrictHostKeyChecking=yes, preserving the existing pinned-host-key verification
and other connection options.
🪄 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: 577ca225-ea38-4362-8857-f9b92cf884d3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.env.example.github/dependabot.yml.github/workflows/ci.yml.github/workflows/deploy.yml.github/workflows/token-expiry-watch.yml.gitmodulesdocs/architecture.mddocs/follow-ups.mddocs/superpowers/plans/2026-08-01-continuous-deployment.mddocs/superpowers/specs/2026-08-01-continuous-deployment-design.mdscripts/deploy/build_bundle.pyscripts/deploy/deploy.shscripts/deploy/setup-vps-deploy-user.shsrc/martyrology_api/app.pysrc/martyrology_api/config.pysrc/martyrology_api/manifest.pysrc/martyrology_api/models.pytests/test_build_bundle.pytests/test_deploy_script.pytests/test_health_api.pytests/test_manifest.pyvendor/clbdrvendor/crmedrvendor/texts
Comment on lines
+96
to
+108
| open_issue "SUBMODULE_TOKEN expires soon ($expiry)" \ | ||
| "\`SUBMODULE_TOKEN\` expires on **$expiry** — $days_left days from now. | ||
|
|
||
| When it lapses, the release workflow fails at the \`vendor/texts\` | ||
| submodule checkout, and because deploys only run on published releases | ||
| you will discover it mid-release. | ||
|
|
||
| Renew: mint a fine-grained PAT (resource owner \`CatholicOS\`, | ||
| Contents: Read-only on \`$WATCHED\`), approve it in the org's pending | ||
| requests, then \`gh secret set SUBMODULE_TOKEN --repo $REPO --app actions\`. | ||
|
|
||
| Longer term, an org-owned GitHub App installation token removes this | ||
| expiry cycle entirely (see the deployment spec, §3)." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The issue body is indented, so GitHub renders most of it as a code block.
The body is a multi-line double-quoted shell string. Every continuation line keeps the YAML block indentation, so those lines start with 10 spaces inside the string value. Markdown treats 4 or more leading spaces as an indented code block. The rendered issue therefore shows the "Renew:" instructions and the closing paragraph as preformatted text, and the inline backtick and bold markup in them is not applied.
The same applies to the body at lines 65-73.
Remove the leading whitespace from the continuation lines.
📝 Proposed fix, apply the same shape to both bodies
open_issue "SUBMODULE_TOKEN expires soon ($expiry)" \
"\`SUBMODULE_TOKEN\` expires on **$expiry** — $days_left days from now.
-
- When it lapses, the release workflow fails at the \`vendor/texts\`
- submodule checkout, and because deploys only run on published releases
- you will discover it mid-release.
-
- Renew: mint a fine-grained PAT (resource owner \`CatholicOS\`,
- Contents: Read-only on \`$WATCHED\`), approve it in the org's pending
- requests, then \`gh secret set SUBMODULE_TOKEN --repo $REPO --app actions\`.
-
- Longer term, an org-owned GitHub App installation token removes this
- expiry cycle entirely (see the deployment spec, §3)."
+
+When it lapses, the release workflow fails at the \`vendor/texts\`
+submodule checkout, and because deploys only run on published releases
+you will discover it mid-release.
+
+Renew: mint a fine-grained PAT (resource owner \`CatholicOS\`,
+Contents: Read-only on \`$WATCHED\`), approve it in the org's pending
+requests, then \`gh secret set SUBMODULE_TOKEN --repo $REPO --app actions\`.
+
+Longer term, an org-owned GitHub App installation token removes this
+expiry cycle entirely (see the deployment spec, §3)."🤖 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/token-expiry-watch.yml around lines 96 - 108, Remove the
leading indentation from continuation lines in both multi-line issue-body
strings used by open_issue, including the bodies near the expiry warning and the
earlier warning. Preserve the YAML structure and message content while ensuring
each Markdown line begins without shell-string padding so bold text, inline
code, and paragraphs render correctly.
| > Two further defects were introduced by fix rounds and caught by scoped re-review: | ||
| > the SIGPIPE fix broke the name screen via `awk '{print $NF}'`, and adding | ||
| > `INT TERM` to two traps made signalled deploys exit 0 with `current` left flipped. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line in the blockquote.
Markdownlint reports MD028 for this line. Keep the blockquote contiguous or end it before the blank line.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 30-30: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/superpowers/plans/2026-08-01-continuous-deployment.md` at line 30,
Remove the blank line within the blockquote in the deployment plan so its quoted
lines remain contiguous, resolving the Markdownlint MD028 violation.
Source: Linters/SAST tools
Comment on lines
+146
to
+152
| # Anchored, no metacharacters: the version becomes part of a path and of a | ||
| # filename, so anything outside this shape is refused outright. | ||
| [[ "$VERSION" =~ ^v?[0-9]+(\.[0-9]+)*$ ]] || die "refusing suspicious version string: $VERSION" | ||
|
|
||
| BUNDLE="$APP_DIR/incoming/martyrology-${VERSION}-linux-x86_64-cp312.tar.gz" | ||
| [ -f "$BUNDLE" ] || die "bundle not found: $BUNDLE" | ||
| [ -f "$BUNDLE.sha256" ] || die "checksum not found: $BUNDLE.sha256" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^scripts/deploy/deploy\.sh$|(^|/)(deploy\.yml|pyproject\.toml)$)' || true
printf '\n--- scripts/deploy/deploy.sh excerpt ---\n'
sed -n '110,180p' scripts/deploy/deploy.sh
printf '\n--- workflow references to VERSION/pyproject ---\n'
rg -n "version|VERSION|project\.version|pyproject|bundle|deploy" .github scripts || true
printf '\n--- pyproject version context ---\n'
sed -n '1,120p' pyproject.tomlRepository: CatholicOS/martyrology-api
Length of output: 20460
Reject prerelease/postrelease versions before building the bundle.
The deploy workflow derives VERSION from pyproject.toml and uses it as the release filename, and this guard accepts only dot-separated digit versions. Set a project policy: either allow the wider valid PEP 440 shapes safely in both workflow and install script, or reject prerelease/postrelease tags earlier than deploy time so a release bundle cannot be built for a version deploy.sh will refuse.
🤖 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/deploy/deploy.sh` around lines 146 - 152, Align the release-version
validation before bundle creation with the `deploy.sh` guard: reject PEP 440
prerelease and postrelease versions when deriving or validating `VERSION` in the
workflow and install path, so only deployable dot-separated numeric versions
reach bundle creation. Reuse the existing version-validation logic or policy
consistently rather than allowing versions that `deploy.sh` will later refuse.
Comment on lines
+17
to
+22
| bundle_format: int | ||
| api_version: str | ||
| api_commit: str | ||
| data: dict[str, str] | ||
| python_requires: str | ||
| files: dict[str, str] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate manifest provenance values.
The model accepts empty or malformed commit IDs, missing data repositories, arbitrary file paths, and invalid file digests. load_manifest() then treats this manifest as usable, so /healthz can report incomplete or invalid release provenance.
Require the three data keys, 40-character hexadecimal commit IDs, relative file paths, and 64-character hexadecimal SHA-256 values. Add rejection tests for each invalid value.
🤖 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 `@src/martyrology_api/manifest.py` around lines 17 - 22, Strengthen manifest
validation used by load_manifest() so the data mapping contains all three
required repository keys, commit IDs are exactly 40 hexadecimal characters, file
paths are relative, and file digests are exactly 64 hexadecimal SHA-256
characters. Ensure invalid or missing values cause manifest loading to reject
the manifest, and add rejection tests covering each validation rule.
Seven items triaged as low or cosmetic during the final review, applied now that the branch is otherwise settled: - quote the scp destination's APP_DIR, matching the ssh step - remediate a stale world-readable bundle in incoming/ instead of aborting provisioning over it - re-normalise and re-assert release permissions after the smoke check, which can write __pycache__ into the tree - assert $APP_DIR, releases/ and incoming/ carry no "other" bits at deploy time, not only at provisioning time - close the signal window in the smoke teardown by disarming last - replace the token-watch dedup's `printf | grep -q` with a here-string, the same SIGPIPE class already documented for the tar listings - assert /healthz reports the deployed version after restart, so a flip that silently did not take no longer reads as a successful deploy The remaining item is GitHub platform behaviour with no in-repo fix and stays documented in docs/follow-ups.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the reviewed CodeRabbit findings on PR #21. build_bundle.assemble() wrote every file into the tarball more than once: tarfile.add() recurses by default, so adding a directory added its whole subtree and the rglob walk then added each of those files again. On the test staging tree that was 10 members for 6 unique paths, with one file written three times; on a real bundle it multiplies the artifact size and makes extraction rewrite every file repeatedly. Pass recursive=False, and assert member-name uniqueness (plus the exact member set, so uniqueness cannot be bought by dropping entries). Byte-identical repeat builds are deliberately not attempted: w:gz embeds the current time and member metadata comes from the filesystem, and auditability here rests on the per-file sha256 map in manifest.json and the bundle checksum deploy.sh verifies, not on tarball byte-identity. deploy.sh now clears setuid/setgid on the extracted tree (a-s) and fails the self-check on any survivor (-perm /6000). Scope is $RELEASE only; $APP_DIR/releases keeps its deliberate 2750. Because releases/ is setgid, $RELEASE and every directory tar creates inside it inherit that bit, so the chmod is what lets the new check pass on a real deploy at all -- a test builds exactly that shape to keep the two from drifting apart. manifest.load_manifest() rejects a manifest whose data mapping lacks texts, crmedr or clbdr. Membership, not equality, so a fourth data repo does not break older readers. This is the reader-side counterpart to the workflow's staged-shape check and guards the one failure this project has hit: a data tree vanishing while everything still reported healthy. The hex-length and path-shape validation also proposed is not added -- the manifest lives inside a bundle whose sha256 is verified before anything is extracted, so a malformed manifest implies a CI bug, not tampering. deploy.yml refuses a non-deployable pyproject version (0.2.0rc1, 1.0.0.post1) in the Resolve version step rather than after building and uploading a whole bundle; pins uv==0.12.1, exercised locally against uv build --wheel and uv export --frozen; sets StrictHostKeyChecking=yes explicitly on scp and ssh; and corrects the stale comment claiming deploy.sh runs sha256sum -c. token-expiry-watch.yml passes --limit 500 to gh issue list, which otherwise defaults to 30 and would file a duplicate warning once the repo has more open issues than that. Tests: derive the "wrong" group at runtime instead of hard-coding root, which would have inverted into a silent pass wherever root is the primary group; and replace the fixed sleep before SIGTERM with a readiness marker the harness prints once its traps are armed. CodeRabbit's report that the token-watch issue bodies render as code blocks is a false positive: the leading spaces belong to the YAML block scalar and are stripped before the shell ever sees them. Verified by executing the extracted run script with gh stubbed and printing the body. Docs: spec Status reflects that the design is implemented and the VPS provisioned, with only the first release deploy outstanding; three untagged fences tagged; one blockquote made contiguous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_deploy_script.py`:
- Around line 211-225: Update _wait_for_ready to wait for HARNESS_READY with a
bounded timeout instead of blocking indefinitely on proc.stdout.readline(). On
timeout or missing readiness, terminate the harness and collect it before
failing, while preserving the existing readiness assertion and diagnostic
context for successful reads.
🪄 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: e733a395-449b-473c-8d48-7ea54d67c55f
📒 Files selected for processing (10)
.github/workflows/deploy.yml.github/workflows/token-expiry-watch.ymldocs/superpowers/plans/2026-08-01-continuous-deployment.mddocs/superpowers/specs/2026-08-01-continuous-deployment-design.mdscripts/deploy/build_bundle.pyscripts/deploy/deploy.shsrc/martyrology_api/manifest.pytests/test_build_bundle.pytests/test_deploy_script.pytests/test_manifest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/token-expiry-watch.yml
- scripts/deploy/build_bundle.py
- .github/workflows/deploy.yml
- docs/superpowers/specs/2026-08-01-continuous-deployment-design.md
- docs/superpowers/plans/2026-08-01-continuous-deployment.md
_wait_for_ready blocked on an unbounded proc.stdout.readline(). A harness that started but never reached its marker would hang the whole suite -- this repo configures no pytest timeout, so that means the CI job runs to GitHub's limit rather than one test failing. The assertion path also left the process running. Reads through a joinable thread with a 10s backstop, and always terminates and collects the harness before raising. stderr is read directly rather than via communicate(), which would race the reader thread for stdout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Adds continuous deployment for the API. On a published release, CI builds a wheel plus an offline wheelhouse, assembles them with the three pinned data trees and a
manifest.jsoninto a tarball, scp's it to the VPS, and runs an on-VPS script over ssh that verifies, installs, smoke-checks, activates via symlink flip, restarts systemd, and rolls back automatically if the live health check fails.Design:
docs/superpowers/specs/2026-08-01-continuous-deployment-design.md.What's here
GET /healthzandsrc/martyrology_api/manifest.py— reports the version, the bundled data commits and the loaded editions. ReturnsNonerather than raising for an absent or malformed manifest, because the deploy script polls it to decide whether to roll back.vendor/{crmedr,clbdr,texts}submodules (HTTPS URLs, required byactions/checkout's extraheader auth) plus the Dependabotgitsubmoduleecosystem, so a data update becomes an auto-merged pin bump.scripts/deploy/build_bundle.py— assembles the bundle and writes the manifest that records exactly which corpus is live.scripts/deploy/deploy.sh— the on-VPS installer.scripts/deploy/setup-vps-deploy-user.sh— one-time root provisioning: two distinct accounts, a shared group, the sudoers drop-in, and the systemd unit..github/workflows/deploy.yml, ashellcheckCI job, and.github/workflows/token-expiry-watch.yml(weekly check thatSUBMODULE_TOKENstill works and is not near expiry, read from GitHub's expiry response header rather than a hardcoded date).Why it's built this way
Docker was rejected because the host's Python 3.12 is pinnable, so a container would add a registry credential on the VPS — a second path by which the private corpus could be pulled — for isolation that isn't needed. Installing from a private pip package was rejected for the same reason, plus it would require packaging metadata in a repo curated by another group.
The served corpus is deliberately frozen to the release artifact and auditable via
manifest.json; curation merges reach production when a release is cut, not before.Notable things review caught
cp -a vendor/textscopied the repo root, butStorescans children of each data path forMM.jsonand the editions live atdata/editions/<edition>/. The smoke check still passed on the public editions, so the deploy would have gone green with no 2004-family texts in production.incoming/at 0700, and self-checks asserting both group access and the absence of "other" access.tar | grep -qpath-traversal guard failed open underpipefail(grep's early exit gave tar a SIGPIPE, making the pipeline non-zero so theifnever fired), andrm -rf "$RELEASE"could destroy the live release on a redeploy.Operator steps before the first deploy
sudo apt install python3.12-venvon the VPS, then runscripts/deploy/setup-vps-deploy-user.shas root and follow its printed next steps.VPS_HOST,VPS_SSH_KEY,VPS_USERNAME,SUBMODULE_TOKEN, and variablesVPS_HOST_KEY,APP_DIR.Note that
token-expiry-watch.ymlstays dormant until this merges — scheduled workflows only run from the default branch.Parked follow-ups are recorded in
docs/follow-ups.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores