fix(deploy): install workspace npm + service pip deps in deploy.sh - #196
Merged
Conversation
Collaborator
Senior review — PR #196
|
| Check | Result |
|---|---|
bash -n scripts/deploy.sh |
✅ pass |
make check-citations |
✅ 7 OK, 0 problems |
changed_match regex matrix (10 scenarios) |
✅ gates fire as intended |
| CI on the PR | ✅ all 9 checks green |
| Merge state | MERGEABLE / CLEAN (against main, ignoring staleness) |
shellcheck |
⏭️ not installed locally |
Couldn't test locally (needs the prod host): the actual end-to-end deploy.sh run (pm2 / systemd timer / /var/www/highfive), a real root npm ci resolve+build, and pip install resolving an onnxruntime cp310 wheel on the real 3.10 interpreter.
Reviewed with Claude Code (senior-reviewer gate). Findings are input, not a verdict — worth a second look at the P0 against your intended requirements strategy.
Collaborator
Reviewed + pushed a fix —
|
deploy.sh rebuilt/reloaded but never installed new deps, so a dep-adding release failed its build and auto-rolled-back. npm: it gated npm ci on backend/package-lock.json, but this is a workspaces monorepo with one ROOT lockfile, so new backend/homepage deps were missed (broke on rotating-file-stream, #178). Now a single root 'npm ci' gated on the root lockfile / any workspace package.json, before the builds; dropped the wrong per-prefix ci. pip: never ran. Now 'python3 -m pip install -r <svc>/requirements.txt' for duckdb-service/image-service when their requirements changed, into the system python3 pm2 uses; non-fatal, the post-reload health check is the real gate (graceful degradation on a missing optional dep). Also rewrites the stale production-runbook 'Updates & Redeployment' section to match reality (main branch, root npm ci, pip into system python3, all 4 pm2 apps, health checks, Python 3.10 / onnxruntime 1.23.2 note). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…R-029) The PR's runbook rewrite asserted onnxruntime is "pinned to 1.23.2" under a "Python 3.10 ceiling". After folding in main (#195 / ADR-029), the real requirements float numpy>=2.0.0 / onnxruntime>=1.23.2 / pydantic>=2.12.5 for a 3.10-3.14 matrix — a floor, not a pin. Rewrote the step-2 comment and the "Python 3.10 floor" paragraph to match, citing ADR-029, and noted that a pip upgrade is not reverted on rollback. Added a ch11 lessons-learned entry for the workspace-lockfile npm-ci miss this PR corrects (per CLAUDE.md's mandatory doc gate). Addresses the senior-reviewer P0/P2 findings on the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cofade
force-pushed
the
fix/deploy-sh-install-deps
branch
from
June 29, 2026 20:27
1951ddc to
135dc1f
Compare
6 tasks
…json Applies the two P2s the previous review round identified but did not push. 1. pip output went to /dev/null, so the WARN it emits on failure carried no reason. The exact failure this step exists to tolerate is "no matching distribution" for a wheel that doesn't exist on the host interpreter -- which is precisely the detail that was being discarded. pip stdout+stderr now append to the deploy log next to the WARN, and the runbook says where to look with a copy-paste grep. 2. The npm ci gate listed the three workspace package.json files but not the root one, which is where the workspace LIST lives -- adding a workspace without touching the lockfile would have skipped the install. Theoretical, but its absence beside the other three read as an oversight rather than a decision. Both pip calls are now one helper instead of two near-identical blocks, and the `changed_match X && ...` form was replaced with an `if` -- the script runs under `set -euo pipefail`, and while a failing left side of && is exempt from set -e (verified), the `if` form does not depend on knowing that. Verified: `bash -n` clean; a 13-case matrix over the real regexes (in the PR discussion) confirms both npm and pip gates fire exactly when intended, including that requirements-dev.txt does NOT trigger a pip install and that one service's requirements cannot fire the other's gate. Merged current main (which carries #193) -- clean, no conflicts. Co-Authored-By: WOZCODE <contact@withwoz.com>
…ed as OK Round-2 review of #196. Three findings, all verified against the code before fixing; the first is a latent production outage this PR would have introduced. 1. P0 -- `npm ci` DELETES node_modules before installing, and rollback() never put it back. It restores backend/dist, homepage/dist and the git tree; none of those is node_modules. Because a failed install happens BEFORE any reload, RELOADED is still empty, the untouched pm2 cluster keeps answering from modules already resident in RAM, the rollback's own health check PASSES, and it notifies "old version is running and healthy" -- while the host now has a wiped dependency tree that dies on its next pm2 restart, which max_memory_restart schedules unprompted. A transient npm error would have silently armed an outage and announced it as a clean recovery. rollback() now reinstalls from the restored lockfile and escalates to a NEEDS-A-HUMAN notification if that also fails. 2. P1 -- "the health check is the real gate" was false for exactly the dependency the non-fatal pip design was built around. image-service imports cv2/numpy/onnxruntime under a try/except (_RUNTIME_AVAILABLE) and /health is a pure liveness probe that never touches them, so a missing OPTIONAL wheel leaves health green, hole detection silently dead, and Discord reporting "Deploy OK". The graceful degradation cited as the justification is what defeats the gate. A failed pip install is now recorded and the deploy reports "Deploy DEGRADED" instead of success. 3. P1 -- the two services pinned DIFFERENT requests versions (2.32.3 vs 2.32.5) while sharing one system site-packages with no venv, so each install succeeded and the last one silently violated the other's pin. This PR installs both on every dep change, which would have made it flap on every deploy. Reconciled to 2.32.5 with a keep-in-sync note in both files. Also: HUSKY=0 on the deploy's npm ci -- the root package.json declares "prepare": "husky", so a root install on the host would set core.hooksPath and make deploy.sh's own git commit in publish_firmware fire developer pre-commit hooks; that call is unguarded, so a hook failure would kill the script mid firmware-publish and brick every later tick on the dirty-tree check. npm ci output now goes to the log too (it is the FATAL one -- "root npm ci failed" with no reason was the message an operator could least act on). The pip step logs which interpreter it installs into, turning the unverifiable "same python3 pm2 uses" assumption into a recorded fact. Docs: the runbook claimed the manual steps "mirror" the automated deploy. They do not -- the manual homepage build passes VITE_API_URL and deploy.sh does not, so the automated path depends on a gitignored homepage/.env.production on the host or ships a bundle pointing at localhost (the homepage health check only verifies the HTML loads, so it cannot detect this). Documented as a simplified hand-deploy with the three real differences, including that hidden host contract. Also corrected "four pm2 apps" -> three, and the now-false header note that nothing live is touched before reload. Chapter 11's entry gains the three generalisable traps: a rollback that doesn't restore what a step mutated has a green check measuring the wrong thing; graceful degradation and health-check gating cancel each other out; and "no venv" is a coupling, not a configuration. Verified: bash -n clean; a 5-case harness (extracting the real rollback() and running it against stubs) confirms the reinstall fires only when npm ci ran, and that a failing reinstall produces the INCOMPLETE alert rather than the old false "healthy" one; the 13-case gate matrix still passes; duckdb-service 232/232 after the requests bump; doc-citation and python-version gates green. Not runnable here: the real deploy (needs pm2/systemd//var/www/highfive). Co-Authored-By: WOZCODE <contact@withwoz.com>
…k gate Round-3 review of #196. 1. rollback() probed only HEALTH_BACKEND and then announced "old version is running and healthy" -- but it is reachable from duckdb, image AND homepage health failures, where the backend was never touched and passes trivially. That is the same report-green-without-checking mistake this PR's own title is about, three lines below the code it added. It now builds the probe list from what was actually reloaded (plus the homepage when dist.old was restored) and names the failing endpoints. 2. The new escalation path exited WITHOUT reloading, while telling the operator "the cluster is still serving from memory". On the path most likely to reach it -- build OK, reload done, health failed, then the reinstall fails -- that is false twice over: the cluster is serving the failed build, and the restored artifacts were never loaded. It now reloads first and describes the actual disk/process split. 3. A lockfile-only tick (an `npm audit fix`, a transitive bump) matched neither ^backend/ nor ^homepage/, so it wiped and reinstalled every production dependency under the live cluster with NO health check at all and then reported "(no service rebuild)". It now records `npm-deps` as a real action and health-checks the backend, which is the Node consumer of that tree. 4. The rollback reinstall is now conditional on node_modules actually being gone. npm validates package.json/lockfile agreement BEFORE clearing the tree, so an abort at that stage leaves a good tree -- reinstalling anyway would have manufactured the outage the guard exists to prevent. node_modules/.package-lock.json (written on a completed install) is the signal. 5. Added a shellcheck CI job. scripts/deploy.sh is the highest-blast-radius file in the repo -- root, unattended, every 2 minutes, can reload services, reset the tree, and publish an irreversible fleet OTA -- and nothing checked it at all; `bash -n` in a PR description was the entire verification story, which is how round 1's false "health check is the real gate" assumption shipped. All six scripts/*.sh are clean at -S warning after fixing an unused loop variable here and unchecked `cd` in the check-* scripts. Doc/comment corrections: the requirements sync note claimed deploy.sh "installs both on every dep change" (it gates each service on its own file); the runbook cited ADR-028 (ML-inference-server-side) for graceful degradation when it is ADR-027; the runbook's step 4 hands the reader a pm2 reload of three apps while its own ecosystem template defines one, which is now called out inline; and the DEGRADED notification no longer offers a hole-detection example when the failure was duckdb-service. Verified: shellcheck clean over all scripts; bash -n clean; an 11-case harness that extracts the real rollback()/rollback_health_targets() and runs them against stubs -- covering wiped-vs-intact node_modules, reinstall success and failure, the reload-before-escalate ordering, and per-service probe selection; the 13-case gate matrix still passes. Not runnable here: a real deploy. Co-Authored-By: WOZCODE <contact@withwoz.com>
… guards Round-4 review of #196. Three findings, all control-flow facts rather than opinions, and all cases where a comment asserted a guarantee the code could not provide. 1. RELOADED is empty at the npm ci call site -- it is initialised at :273 and first appended at :322, after the install. So on the npm-ci-failure path, which is the ONLY path that reaches the rollback reinstall, the reload added last round iterated over nothing and the health probe fell through to its backend fallback, probing a process that was never restarted and printing "verified healthy". The escalation message likewise claimed "services reloaded" 100% of the times it fired. highfive-api is now added to RELOADED before the install runs, so both the rollback reload and the probe are real. 2. The same emptiness made the dependency-only health check unfalsifiable: it probed a Node process still holding the pre-install module graph, so it passed regardless of what was on disk. Worse, it was a real functional gap -- a lockfile-only tick (dependabot, npm audit fix) installed a new tree, reloaded nothing, and reported Deploy OK while every service still ran the old modules. Same fix; the probe now means something. A dependency change also rebuilds the homepage bundle, which otherwise kept a bumped dependency out of the shipped site until some unrelated homepage file happened to change. 3. The node_modules/.package-lock.json heuristic skipped the reinstall in the MORE common shape. NPM_CI_RAN is set before the install, not after it succeeds, so "tree intact" cannot distinguish "npm aborted pre-wipe" from "npm succeeded and a later step failed" -- and in the latter, node_modules is synced to the NEW lockfile while the tree is reset to the old one, so the old code runs against the new dependency set and it is announced as a clean restore. Three documents asserted the reinstall was unconditional. It now is; a redundant npm ci during an already-failing deploy is cheap insurance. Also from this round: - A failure marker stops the 2-minute timer retrying a known-broken SHA forever. That was merely noisy before; with a dependency wipe inside the loop it re-tears-down production's node_modules every two minutes. - reload_services no longer swallows pm2 failures. "process or namespace not found" is a real state here (the runbook's ecosystem template registers only highfive-api) and silently succeeding on it is how a deploy reports green for a service it never restarted. - The homepage build now fails the deploy if the bundle contains localhost:3002 -- the VITE_API_URL landmine documented last round was left armed, and the homepage health check cannot see it because it only verifies the HTML loads. - add_reload() dedupes, since a backend change and a dependency change both select highfive-api. - shellcheck gate tightened to -S info (SC2086 lives there) and extended to ESP32-CAM/build.sh, which produces the artifacts the fleet OTA ships. Both verified clean at that level. - docs/10-quality-requirements/ci-gates.md updated -- it still said "ten parallel jobs" while this PR made it eleven, which CLAUDE.md's own mandatory-update table required. - The requirements sync comments are scoped to the PM2 path; under Docker Compose the two services have separate site-packages and would not collide. - "Rebuilt: npm-deps" -> "Built/installed:", since nothing was rebuilt. Verified: shellcheck -S info clean over scripts/*.sh + ESP32-CAM/build.sh; bash -n clean; the 11-case rollback harness passes against the corrected contract (including the two cases whose expectations this commit deliberately inverted); the 13-case gate matrix passes; doc-citation gate clean. Co-Authored-By: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
…loud Three conflicts, all where #196's deploy.sh hardening met this branch's BRANCH=main -> production switch. Resolved by keeping BOTH sides rather than picking one: - deploy.sh header: kept main's dependency-install CAVEAT (npm ci wipes node_modules in place, pip upgrades are forward-only and not reverted on rollback) and applied this branch's production wording. - deploy.sh config block: kept FAILED_MARKER (the retry-storm guard) and set BRANCH="production". - .deploy.env.example: kept the new pm2-environment section (HIGHFIVE_ENV / HIGHFIVE_API_KEY precedence) and this branch's "deploy branch (production)" wording on the firmware-OTA note. - production-runbook.md: kept the gated-release-branch explanation AND main's simplified-hand-deploy caveats, and corrected the manual step from `git pull --ff-only origin main` to `production` — it would otherwise have told an operator to hand-deploy from the integration line this change exists to stop deploying from. Beyond the merge, one behavioural addition, because this switch has a failure mode nobody would see. deploy.sh exits early when the host checkout is not on BRANCH: [ "$cur_branch" != "$BRANCH" ] && log "skip"; exit 0 That was harmless while BRANCH was `main` (the host is on main). Flipping it to `production` makes it fire on EVERY tick until someone runs `git checkout production` on the host -- so the moment this merges, auto-deploy stops, silently, with no notification, while `main` keeps accumulating commits nobody realises are unshipped. A silent stop is the worst shape for this: it looks exactly like "no new commits". It now sends ONE Discord notification naming the exact cutover command, with a marker file so it doesn't repeat every two minutes, and clears the marker once the host is on the right branch. Verified: bash -n clean, shellcheck -S info clean, the 11-case rollback harness and 13-case gate matrix from #196 still pass, and the doc-citation, bind-claims and hardcoded-key gates are green. Co-Authored-By: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
Senior review of the production-branch adoption. Two P0s, both verified with single git commands against the repo rather than read off the prose. 1. THE DOCUMENTED CUTOVER WOULD HAVE REVERTED PRODUCTION. The procedure said "git checkout production && git reset --hard origin/production" with no promotion step. But origin/production is a0e7374, four commits behind main: git show origin/production:scripts/deploy.sh | grep '^BRANCH=' -> "main" ...and 0 hits for FAILED_MARKER|NPM_CI_RAN|PIP_FAILED|add_reload| rollback_health_targets|HUSKY=0 (26 on this branch) So running it today rolls the live host back past #193, #196 and #222 -- discarding the deploy hardening and the security track-A work -- and leaves the checkout on `production` running a driver that tracks `main`. That takes the old bare `log "skip"; exit 0` path: deploys stop silently, forever, and the wrong-branch alert that would have said so was reverted away with everything else. Silent, permanent, self-concealing. The procedure is now explicitly ordered: promote, VERIFY (`git show origin/production:scripts/deploy.sh | grep '^BRANCH='` must print production), then check out. The same ordering and the same verify command are in the Discord alert body, because that is what the operator actually reads at 2am. 2. ADR-030's recorded root cause was FALSE, and it was the sole justification for force-resetting a release branch: git rev-list --max-parents=0 main -> d9ac93d (one root) git rev-list --max-parents=0 bf8b314 -> d9ac93d (the SAME root) git merge-base main bf8b314 -> da1b21d git rev-list --count main..bf8b314 -> 25 (not 136) There was no orphan root, no rebuilt history, no unrelated ancestry -- a merge was available the entire time and was rejected for tidiness. The cited #124 is a senior-reviewer config commit. Corrected in ADR-030 and in chapter 11, kept as a visible correction rather than a quiet edit, because ch11 had already generalised the false cause into a "how to avoid this next time" rule that future maintainers would have applied to a scenario that never happened. Also from this round: - "A real promotion gate" was overstated: neither branch has protection, CI runs on main only, and any fast-forwarding commit is accepted. Now says gate-by-convention, and states that production MUST stay unprotected -- protecting it would reject publish_firmware's push and ship OTAs whose SEQUENCE bump is not in git. - The ADR's one acknowledged invariant violation (publish_firmware commits to production) had "tracked as a follow-up" with nothing behind it. Filed #225. - The OTA notification -- attached to the single irreversible action in the system -- cited ADR-028 (ML inference server-side) instead of ADR-030. This is the ADR-renumber-on-collision trap the repo already documented. - The archive tag is NOT on the remote (`git ls-remote --tags origin` finds nothing), so the 25 commits survive only via a stale branch clean_gone would delete. Both docs now say so instead of claiming a recovery point exists. - CONTRIBUTING.md, which CLAUDE.md names as the authority for the branch model, said only "branch off main" and never mentioned production. It now documents the promote-don't-PR rule and the never-force-push constraint. - The branch-mismatch marker is hoisted to a BRANCH_MARKER constant next to FAILED_MARKER (it was a local var re-typed as a literal in the rm), and the marker is now written BEFORE notify -- notify can fail under set -e, which would have produced the every-two-minutes alert the marker exists to stop. Not done, stated plainly: image-service/tests/test_upload.py carries ruff format reflow. main's version is not ruff-clean, and the pre-commit hook reformats any staged .py, so it cannot be reverted without bypassing the hook. Verified: bash -n and shellcheck -S info clean; #196's 11-case rollback harness and 13-case gate matrix still pass; all five repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
* chore: adopt production as the gated release source (#152) Reconcile the documented services deploy source with reality and unify it with firmware OTA on a single gated `production` branch. Investigation for #152 found three stacked problems: the docs named `production` while the live auto-deploy pulled `main`; firmware OTA and the services track were documented as separate; and `main`/`production` shared no common git ancestor (main's history was rebuilt), so `production` could never fast-forward and silently rotted. Decision (per maintainer): `production` becomes the single gated release branch for both web services and firmware OTA. `main` is the integration line; a release is a fast-forward of `production` onto a chosen `main` commit. `prod-*` tags are cut on `production`. - scripts/deploy.sh: BRANCH main -> production; branch-agnostic notify text - production-deployment.md: drop drift warning; add release/promotion + one-time host cutover section - production-runbook.md: document the promote-then-pull model - firmware-release.md: rewrite the branch & tag model (both tracks on production); replace the "known drift" callout with a history note; update the release-checklist commit/tag step - chapter 11: mark the drift lesson RESOLVED; record the unrelated-history root cause and the fast-forwardable-deploy-branch rule - new ADR-028; update README/esp-flashing/CLAUDE.md pointers The branch reconciliation (archive tag + force-reset of origin/production) and the one-time prod-host checkout are operator steps documented in ADR-028 and production-deployment.md, to run after this lands on main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017drgAN84qrn61eZ1yZTdgS * docs: add "never release from main" critical rule to CLAUDE.md The production-as-gated-release-branch model (#152 / ADR-030) was only spelled out in the firmware-OTA section. Add a concise hard rule to the top-level "Critical rules (do NOT violate)" list so every session knows prod releases ship from `production`, never from `main`. Links to the full mechanics rather than duplicating them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: correct ADR-030's root cause and invert the cutover order Senior review of the production-branch adoption. Two P0s, both verified with single git commands against the repo rather than read off the prose. 1. THE DOCUMENTED CUTOVER WOULD HAVE REVERTED PRODUCTION. The procedure said "git checkout production && git reset --hard origin/production" with no promotion step. But origin/production is a0e7374, four commits behind main: git show origin/production:scripts/deploy.sh | grep '^BRANCH=' -> "main" ...and 0 hits for FAILED_MARKER|NPM_CI_RAN|PIP_FAILED|add_reload| rollback_health_targets|HUSKY=0 (26 on this branch) So running it today rolls the live host back past #193, #196 and #222 -- discarding the deploy hardening and the security track-A work -- and leaves the checkout on `production` running a driver that tracks `main`. That takes the old bare `log "skip"; exit 0` path: deploys stop silently, forever, and the wrong-branch alert that would have said so was reverted away with everything else. Silent, permanent, self-concealing. The procedure is now explicitly ordered: promote, VERIFY (`git show origin/production:scripts/deploy.sh | grep '^BRANCH='` must print production), then check out. The same ordering and the same verify command are in the Discord alert body, because that is what the operator actually reads at 2am. 2. ADR-030's recorded root cause was FALSE, and it was the sole justification for force-resetting a release branch: git rev-list --max-parents=0 main -> d9ac93d (one root) git rev-list --max-parents=0 bf8b314 -> d9ac93d (the SAME root) git merge-base main bf8b314 -> da1b21d git rev-list --count main..bf8b314 -> 25 (not 136) There was no orphan root, no rebuilt history, no unrelated ancestry -- a merge was available the entire time and was rejected for tidiness. The cited #124 is a senior-reviewer config commit. Corrected in ADR-030 and in chapter 11, kept as a visible correction rather than a quiet edit, because ch11 had already generalised the false cause into a "how to avoid this next time" rule that future maintainers would have applied to a scenario that never happened. Also from this round: - "A real promotion gate" was overstated: neither branch has protection, CI runs on main only, and any fast-forwarding commit is accepted. Now says gate-by-convention, and states that production MUST stay unprotected -- protecting it would reject publish_firmware's push and ship OTAs whose SEQUENCE bump is not in git. - The ADR's one acknowledged invariant violation (publish_firmware commits to production) had "tracked as a follow-up" with nothing behind it. Filed #225. - The OTA notification -- attached to the single irreversible action in the system -- cited ADR-028 (ML inference server-side) instead of ADR-030. This is the ADR-renumber-on-collision trap the repo already documented. - The archive tag is NOT on the remote (`git ls-remote --tags origin` finds nothing), so the 25 commits survive only via a stale branch clean_gone would delete. Both docs now say so instead of claiming a recovery point exists. - CONTRIBUTING.md, which CLAUDE.md names as the authority for the branch model, said only "branch off main" and never mentioned production. It now documents the promote-don't-PR rule and the never-force-push constraint. - The branch-mismatch marker is hoisted to a BRANCH_MARKER constant next to FAILED_MARKER (it was a local var re-typed as a literal in the rm), and the marker is now written BEFORE notify -- notify can fail under set -e, which would have produced the every-two-minutes alert the marker exists to stop. Not done, stated plainly: image-service/tests/test_upload.py carries ruff format reflow. main's version is not ruff-clean, and the pre-commit hook reformats any staged .py, so it cannot be reverted without bypassing the hook. Verified: bash -n and shellcheck -S info clean; #196's 11-case rollback harness and 13-case gate matrix still pass; all five repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com> * docs: retract the history claim in the runbook too, and price the cutover honestly Round-2 review. The pattern in the finding is the same one this PR is about. 1. The false "shared no common ancestor" claim was fixed in ADR-030 and chapter 11 and LEFT STANDING in docs/07-deployment-view/firmware-release.md -- so the branch shipped three documents describing one event, two of which called the third a fabrication. The survivor was the runbook: the file someone opens WHILE cutting a release, while the retractions sat in an ADR and a tech-debt log nobody reads mid-release. Fixed, with the retraction visible there too. Swept the tree afterwards; the only remaining matches are inside the correction blocks that quote the claim in order to retract it. 2. "verified stale" did not survive its own standard. The ADR had just spent twenty lines explaining why unverified assertions here are dangerous, and then rested the whole justification for discarding 25 commits on an enumeration that omitted four test files and never mentioned the ESP work. Re-derived from the repo, and one finding is worth the trouble: the archived TIP (bf8b314, "use esp_task_wdt_reconfigure and defer loopTask subscribe past AP setup") uses an API that appears NOWHERE on main -- `git grep esp_task_wdt_reconfigure origin/main -- ESP32-CAM/` is empty, and main still uses the IDF-4 esp_task_wdt_init/add pair. Main fixes the same AP-mode reboot loop a different way (>=60s TASK_WDT_TIMEOUT_S plus runAccessPoint feeding the watchdog, recorded as fixed in troubleshooting.md), so nothing live is lost -- but "already exists on main" was the wrong description, and the ADR now says what was actually checked. 3. The OTA notification told the operator to run `git checkout main` with no statement of WHERE. It arrives while they are looking at the host, and the commands run there -- where `git checkout main` immediately trips the branch-mismatch guard this same PR adds and pauses every deploy. Now says "FROM A MAINTAINER CLONE, NOT THIS HOST" and explains the consequence. 4. The reordered cutover fixed the direction but not the rebuild. `git reset --hard` restores the source tree only; backend/dist, homepage/dist and node_modules stay at whatever the host last built, and no later tick repairs that because deploy.sh exits at `[ "$PREV_SHA" = "$REMOTE_SHA" ] && exit 0`. Step 3's own health checks pass in that skewed state. Added an explicit rebuild, and said when it can be skipped. 5. docs/02-constraints/README.md -- which CLAUDE.md's critical-rules section names as the full list -- had no production-branch rule at all. This PR found and fixed exactly that gap in CONTRIBUTING.md and left the file CLAUDE.md points at. Added, including the never-force-push and must-stay-unprotected constraints. P2s: the ADR said CI "runs on main only" (it triggers on main push+PR and never on production -- same conclusion, wrong sentence); the Decision section described the OTA publish as unconditional when the whole block is gated behind FIRMWARE_AUTO_OTA=1; and chapter 11's "how to avoid" paragraph still led with the history-rewrite scenario the correction above it calls fictional -- it now leads with the real rule (no promotion mechanism, no staleness signal). Verified: bash -n and shellcheck -S info clean, the 11-case rollback harness passes, all repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
…rname bans your IP (#226) * docs(troubleshooting): ssh publickey failure, and why guessing bans your IP Two failures that present as one, and the second is self-inflicted. An ~/.ssh/config pinning only HostName makes OpenSSH fall back to the LOCAL account name, so a correctly installed key still gets "Permission denied (publickey)" — the key is only half the credential and the login name is not derivable from the key comment or an email address. Probing plausible names to find it then trips the host's brute-force protection, which black-holes port 22 while https keeps returning 200. That combination reads as "the server is down", which is the wrong conclusion and leads to more retries, which with fail2ban refresh the ban timer and keep you locked out. Includes the diagnostic that distinguishes the two (web fine + port 22 dead = banned, not an outage), the config fix, how to get unbanned, and a single-attempt verification rather than a loop. Three things fixed while writing it up, all repo conventions: - No angle-bracket placeholders. CLAUDE.md forbids them in PowerShell snippets because `<` is a redirection operator; this file already uses the `$var = "..."` form elsewhere ($repo, $PORT, $mac) and now does here too. - No hardcoded username. An earlier draft used the author's Windows login as the example; it now says $env:USERNAME, matching the de-hardcoding applied to this file's ruff entry in #196. - MaxAuthTries and a ban are no longer conflated. MaxAuthTries closes one connection and you still get a prompt next time; a ban drops packets. That distinction is exactly what "connection timed out" vs "Permission denied" tells you, so it is load-bearing for the diagnosis. Which mechanism this host runs was NOT confirmed from outside, and the doc says so rather than asserting fail2ban. Verified: every PowerShell snippet parsed with [System.Management.Automation.Language.Parser]::ParseInput (6 of 6 clean), and the here-string append was executed against a temp file — it expands $sshUser correctly and writes ASCII with no BOM (first bytes 72,111,115), which matters because PowerShell's default redirect would emit UTF-16 LE with a BOM into .ssh/config. Added a Select-String guard first, since a blind append duplicates an existing Host block and OpenSSH silently honours the FIRST match. The production host was deliberately not probed over SSH while writing this. Co-Authored-By: WOZCODE <contact@withwoz.com> * docs: make the SSH recovery block safe to paste, and log the incident Review of the first draft found that I had validated the wrong thing. I verified every snippet PARSED and executed the here-string against a temp file I created myself -- the one input shape that cannot fail. Every real defect lived in runtime behaviour against a hostile starting state. All reproduced before fixing: - P0: `Out-File -Append` into an existing ~/.ssh/config with no trailing newline welds the new stanza onto the last line. Measured output: " User olduserHost highfive". The stanza never registers, and HostName now carries three trailing tokens, which OpenSSH treats as a fatal parse error for EVERY ssh invocation -- so the documented remedy escalates "I can't reach one server" into "ssh doesn't run at all", for someone already locked out. Now mirrors the safe-write pattern this same file already uses for .wslconfig: Test-Path, and if the file exists print + open notepad rather than appending. Only auto-writes when absent. - P0: "with fail2ban every further attempt refreshes the ban timer" is false. Once banned, the firewall drops packets before sshd, so no new auth failure is ever logged and nothing re-triggers the filter; bantime.increment lengthens a SUBSEQUENT ban after the current one expires. The advice (stop retrying) is right, the reason was wrong -- and it contradicted my own blockquote three paragraphs up that declines to assert which mechanism this host even runs. - `-ErrorAction SilentlyContinue` does NOT suppress Select-String's ObjectNotFound on a missing file in PS 5.1 (reproduced: full red ItemNotFoundException), and Out-File on a path whose .ssh dir does not exist throws DirectoryNotFoundException. So the fresh-machine case -- the likeliest state for someone who has never logged in -- produced a scary error followed by a hard failure. Now New-Item -Force the directory first. - `-o BatchMode=yes` was the wrong verification: it suppresses passphrase and host-key prompts, so an unloaded agent identity fails with "Permission denied (publickey)", byte-identical to the error being diagnosed. Replaced with `ssh -v`, whose "debug1: Authenticating to host:22 as 'name'" line IS the diagnosis. Confirmed against a real SSH endpoint rather than assumed. - Test-NetConnection | Select-Object emits a table plus a WARNING, not the bare "False" the comment implied; now -InformationLevel Quiet. Also: the ipify hop can report a proxy's egress while ssh goes out from another address, so the owner would unban a stranger -- replaced with `fail2ban-client status sshd`, read on the host. A sentinel throw stops the unedited placeholder from being written as a real User. IdentityFile is now conditional (pinning it overrides the defaults, so naming a missing key burns another auth attempt). "byte-for-byte" corrected -- sshd ignores the trailing comment. Structure: moved out of "## Server stack" (whose other entries are all dev stack) into its own "## Production host access (SSH)" section, and linked from both places a reader is told to ssh: production-runbook.md and production-deployment.md. Added the chapter-11 lesson, since "probing prod with guessed usernames gets you banned, and the ban looks like an outage" is an incident, not a symptom->fix. Verified this time at runtime, not by parsing: the safe-write block exercised against all three starting states (existing-config-without-trailing-newline is left byte-identical; fresh machine creates the file, registers the stanza, and writes ASCII with no BOM; the sentinel throws), plus all 5 fenced snippets parse and the ssh -v line shape confirmed live. Repo gates and prettier clean. Co-Authored-By: WOZCODE <contact@withwoz.com> * docs(troubleshooting): lead with ssh -G, and gate the write instead of throwing Round-2 review. Both findings were verified by running them, which is the habit the previous two rounds kept missing. 1. The section led with `ssh -v`, which is the wrong tool twice over: it sends an auth attempt to production — in a document whose entire point is "stop sending auth attempts to production" — and once you ARE banned (part 2 of the same section) it hangs at "Connecting to" and never reaches the line being quoted, so the "one command that shows you this" is unavailable exactly when it is needed. `ssh -G highfive` prints the resolved config with ZERO packets; run here it printed `user wienh`, which is the entire bug, offline and free. It now leads the section, and `ssh -v` is demoted to the final single-attempt verification where a real connection is the point. 2. The placeholder sentinel did not hold. `if (...) { throw }` is a complete statement, so pasted into a console the throw prints red and execution CONTINUES into the next statement, which wrote: User the-name-the-owner-gave-you — a config that looks fixed while sending a name indistinguishable from a guess, i.e. feeding the exact ban this page exists to prevent. Reproduced in a real runspace, statement by statement. The check is now the first arm of the if/elseif/else that does the writing, so it cannot be stepped past: the whole chain is one statement, which is precisely why it works. Also, verified with `ssh -G -F`: an earlier `Host *` / `User git` block beats a later `Host highfive` / `User realname`. Appending at the end — which the notepad branch effectively told the reader to do — is silently wrong. That branch now prints the literal three lines and says to put them ABOVE any `Host *`. And the central diagnostic over-committed: "web fine + port 22 dead = banned" is equally consistent with your own network blocking outbound tcp/22 (corp LAN, VPN, hotel), which sends the operator to ask the owner to unban an address that was never banned. Added a third-host control probe and a three-row table that separates ban / egress-block / real outage. Smaller corrections: dropped-vs-rejected is now hedged (fail2ban's default iptables action REJECTs, so "connection refused" is as likely as a timeout); `bantime.increment` is flagged as off-by-default rather than asserted; the sample output no longer contains the maintainer's real Windows account and home path; production-deployment.md now carries a real markdown link rather than a comment inside a bash fence; and there is an out-of-band path for the case where the OWNER is the one locked out (fail2ban-client needs a shell on the host, so it is useless to them). Verified at the outcome level, not the mechanism level: the fix block was executed in a real runspace with correct paste semantics against all three starting states — unedited placeholder writes NOTHING, a real name on a fresh machine produces a config that `ssh -G` resolves to `user realname` in ASCII with no BOM, and an existing config with no trailing newline is left byte-identical. The maintainer's real ~/.ssh/config was confirmed untouched throughout. Repo gates and prettier clean. Co-Authored-By: WOZCODE <contact@withwoz.com> --------- Co-authored-by: WOZCODE <contact@withwoz.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.
Problem
scripts/deploy.shrebuilds + reloads but never installs new dependencies, so a release that adds a dep fails its build and auto-rolls-back:npm --prefix backend cionly whenbackend/package-lock.jsonchanged. But this is an npm workspaces monorepo (contracts/backend/homepageshare one rootpackage-lock.json), so a new backend/homepage dep changes the root lockfile — the condition never fires,npm ciis skipped, andtsc/vitebuild against missing deps → rollback. Hit onrotating-file-stream(Turn the admin "Server Logs" panel into a real live log console #178).npm --prefix <pkg> ciis also the wrong command for workspaces.opencv/numpy/onnxruntime) are never installed beforepm2 reload.Changes —
scripts/deploy.shnpm ciwhen the rootpackage-lock.jsonor any workspacepackage.jsonchanged, run before the builds; dropped the per-prefixnpm --prefix backend|homepage ci. Fatal on failure (rollback) — a broken install means a broken build anyway.pip installforduckdb-service/image-servicewhen theirrequirements.txtchanged, into the systempython3pm2 runs them with (no venv). Non-fatal: a resolver miss (e.g. an onnxruntime with no wheel for this Python) must not block the deploy — the existing post-reload health check is the real gate (services degrade gracefully on a missing optional dep; a genuinely-required missing module crashes the reload → health fails → rollback).Changes —
docs/07-deployment-view/production-runbook.mdRewrote the stale "Updates & Redeployment" section to match reality: deploys from
main(not aproductionbranch), rootnpm ci(workspaces),pip installfor both Python services into systempython3, the Node builds,pm2 reloadof all four apps, and the four health checks. Added the Python-3.10 ceiling note (onnxruntime pinned to 1.23.2 = max cp310 wheel; ESP runs no models — ADR-028).Verification
bash -n scripts/deploy.shpasses.onnxruntime==1.23.2loads and runs the realhole_detector.onnx; image-service also boots with opencv/numpy/onnxruntime all absent (graceful no-op) — which is why the pip step is non-fatal.rotating-file-stream(Turn the admin "Server Logs" panel into a real live log console #178) failure; rootnpm ciresolves it (the dep is in the root lockfile).Pairs with #191 (Python 3.10 compat) — together they let
mainredeploy cleanly on the 3.10 host.🤖 Generated with Claude Code