Skip to content

refactor(selfhost): dedup shared logic in deploy and backup scripts - #2951

Merged
JSONbored merged 1 commit into
mainfrom
claude/dedup-deploy-backup-scripts-2910
Jul 4, 2026
Merged

refactor(selfhost): dedup shared logic in deploy and backup scripts#2951
JSONbored merged 1 commit into
mainfrom
claude/dedup-deploy-backup-scripts-2910

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Two self-host script pairs duplicated substantial shell logic. Investigated both pairs thoroughly (exact diff of every shared function, every call site, container-mount constraints, and existing test coverage) before deciding what was safe to merge.

deploy-selfhost-image.sh / deploy-selfhost-prebuilt.sh — copy-pasted require_cmd()/env_get()/env_put()/compose_file_args() verbatim. Extracted into scripts/lib/selfhost-deploy-common.sh, sourced via a BASH_SOURCE-relative path from both (works regardless of invocation style — relative, absolute, or bash scripts/..., matching how the existing test spawns it). Found and reconciled a genuine, undocumented drift while extracting: env_put()'s temp-file location differed between the two scripts — deploy-selfhost-image.sh created it in the same directory as the target .env file (avoiding a cross-filesystem write if $ENV_FILE lives on a different mount than the system tmpdir), deploy-selfhost-prebuilt.sh used a plain mktemp with no comment explaining the difference. Both now use the safer same-directory version.

backup.sh / verify-backup.sh — duplicated url_decode()/pgpass_escape() byte-for-byte. Extracted into scripts/selfhost-pg-url.sh — a sibling file, not a lib/ subdirectory, because both scripts are bind-mounted individually at container root (/backup.sh, /verify-backup.sh) with no shared /lib path; docker-compose.yml gets a matching bind-mount (./scripts/selfhost-pg-url.sh:/selfhost-pg-url.sh:ro) so . "$(dirname "$0")/selfhost-pg-url.sh" resolves correctly both in the container and when either script is run directly from a checkout (as the tests do).

Deliberately did NOT merge prepare_pg_env() (backup.sh) and pg_connect_arg() (verify-backup.sh), despite them sharing the identical URI-parsing algorithm — full analysis confirmed genuine, non-cosmetic differences in their PGPASSFILE lifecycle: prepare_pg_env() reads the URL from a single global and runs once per invocation; pg_connect_arg() takes the URL as an argument, unsets PGPASSFILE at the top of every call (a reentrancy guard against a stale value leaking from a previous call for a different URL — required because it's called up to 4 times per run for different URLs), and tracks a list of created passfiles instead of one. Forcing these into a single function risked losing that reentrancy guard, which would leak a password across connections. Left as-is with a strengthened cross-reference comment in both files explaining exactly why.

Verification (not assumed safe): ran the existing — and for the backup pair, deliberately adversarial — test suites. All 47 tests across selfhost-image-deploy, selfhost-sentry-release, backup-script, selfhost-backup-script, and selfhost-verify-backup-script pass unchanged, including the test that specifically proves no password leaks between connections across the full multi-URL scratch-restore flow. deploy-selfhost-prebuilt.sh has no existing script-execution test coverage (a pre-existing gap, not introduced here) — manually exercised it end-to-end for real (a full docker run build) to confirm the sourced functions work correctly; it completed successfully well past every shared-function call site. shellcheck is clean on every touched/new file.

Resolves #2910. Part of the #1667 self-host review-stack roadmap.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused (one theme: script deduplication) and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • Linked issue: refactor(selfhost): dedup shared logic in deploy and backup scripts #2910.

Validation

  • git diff --check
  • npm run typecheck
  • shellcheck on every touched/new shell file — clean (only pre-existing, unrelated info-level notices remain in verify-backup.sh)
  • vitest run on all 5 relevant test files — 47 tests passed
  • Manual end-to-end execution: deploy-selfhost-prebuilt.sh run for real (Docker build) to verify the shared-lib sourcing works where no automated test exists
  • docker-compose.yml re-validated as parseable YAML after the new bind-mount
  • npm run actionlint / npm run test:workers / npm run build:mcp / npm run test:mcp-pack / npm audit / ui:* — not run locally; no workflow, worker-pool, MCP-package, or UI files touched. CI runs the full gate, including the self-host "build + boot smoke test" job (this PR touches docker-compose.yml and scripts/, so that job will run and is the authoritative confirmation the backup service still boots with its new bind-mount).

If any required check was skipped, explain why:

  • test:coverage/test:ci not run locally — no src/** files changed (this is a scripts/**/docker-compose.yml diff only, both Codecov-ignored paths); the shell-script test suites above are the real correctness signal for this change, and they all pass.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed. This PR touches Postgres-credential-handling code directly — extra care was taken (see Summary) to preserve every existing safeguard (percent-decoding, newline/CR rejection, 600-permission passfiles, the reentrancy unset-guard, full cleanup-trap coverage) and confirm it with the existing adversarial test suite rather than only by inspection.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no such changes; existing negative-path tests for the credential-handling code all still pass.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/schema/MCP surface touched.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A — no changelog edit.)

Two script pairs duplicated substantial shell logic:

- deploy-selfhost-image.sh / deploy-selfhost-prebuilt.sh copy-pasted
  require_cmd()/env_get()/env_put()/compose_file_args() verbatim. Extracted
  into scripts/lib/selfhost-deploy-common.sh, sourced via a BASH_SOURCE-
  relative path from both. Reconciled an unintentional drift found while
  extracting: env_put()'s temp-file location differed (image.sh created it
  same-directory as the target, avoiding cross-filesystem write issues;
  prebuilt.sh used a plain mktemp with no documented reason) -- both now use
  the safer same-directory version.

- backup.sh / verify-backup.sh duplicated url_decode()/pgpass_escape()
  byte-for-byte. Extracted into scripts/selfhost-pg-url.sh (a sibling file,
  not a lib/ subdirectory, since both scripts are bind-mounted individually
  at container root with no shared /lib path -- docker-compose.yml gets a
  matching bind-mount). prepare_pg_env()/pg_connect_arg() are deliberately
  NOT merged despite sharing the same URI-parsing algorithm: they have
  genuinely different PGPASSFILE lifecycles (single global vs. per-call arg,
  a reentrancy unset-guard, one passfile vs. a tracked list) that a forced
  merge would risk breaking -- left as-is with a stronger cross-reference
  comment explaining why.

Verified via the existing (and, for the backup pair, deliberately
adversarial) test suites rather than assumed safe: all 47 tests across
selfhost-image-deploy, selfhost-sentry-release, backup-script,
selfhost-backup-script, and selfhost-verify-backup-script still pass
unchanged, including the reentrancy/leak-guard test that proves no password
crosses between connections. Also manually exercised both deploy scripts
end-to-end (deploy-selfhost-prebuilt.sh has no existing script-execution
test coverage) and confirmed shellcheck is clean on every touched file.
@JSONbored JSONbored self-assigned this Jul 4, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 4, 2026
@loopover-orb

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-04 06:40:50 UTC

7 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Maintainer test expectations unmet — Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.

Review summary
The PR cleanly extracts duplicated deploy helpers and Postgres URL credential helpers while preserving the visible call paths: deploy scripts resolve the shared library from BASH_SOURCE, and backup/verify scripts mount/source their sibling helper consistently with the container layout. The only behavioral change in the visible diff is adopting the same-directory temp-file strategy for the prebuilt deploy env writer, which is a safer version of the existing image-script behavior. I do not see a reachable correctness break in the changed hunks.

Nits — 6 non-blocking
  • scripts/lib/selfhost-deploy-common.sh:42 documents the old cross-filesystem concern as applying to `cat "$tmp" >"$file"`, but the remaining operation is not a rename, so the comment should say the temp file stays beside the target for locality/permissions/consistency rather than implying `cat` crosses filesystem boundaries unsafely.
  • scripts/selfhost-pg-url.sh:18 says the callers decode both userinfo and query-string values while also explaining `+` is not form-decoded; confirm that this remains intentional for libpq `password=` query parameters, since many operators may expect query-style plus handling there.
  • scripts/lib/selfhost-deploy-common.sh:42 shorten the long historical comment and move the refactor(selfhost): dedup shared logic in deploy and backup scripts #2910 rationale into commit/PR context; keeping only the invariant makes this shared helper easier to maintain.
  • scripts/selfhost-pg-url.sh:18 add or keep a focused test for `password=foo+bar` in the query-string path so the deliberate non-form-decoding behavior is locked down.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Diff looks like trivial or whitespace-only churn — Reduce whitespace-only or formatting-only churn and keep the diff focused on substantive changes.

Concerns raised — review before merging

  • Maintainer test expectations unmet — Add or update tests, or attach passing validation output that satisfies the maintainer's test expectations.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #2910
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 60 registered-repo PR(s), 51 merged, 437 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 437 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 60 PR(s), 437 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.05%. Comparing base (461aed5) to head (6364ca2).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files         259      259           
  Lines       28459    28459           
  Branches    10348    10348           
=======================================
  Hits        27335    27335           
  Misses        489      489           
  Partials      635      635           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 31d8fee into main Jul 4, 2026
12 checks passed
@JSONbored
JSONbored deleted the claude/dedup-deploy-backup-scripts-2910 branch July 4, 2026 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

refactor(selfhost): dedup shared logic in deploy and backup scripts

1 participant