olympus/infra-tracker
3
0
Fork 0
Code Issues Pull requests 1 Wiki Activity Actions

npm: publish the Tracker from the Edge #4

Merged
pit merged 2 commits from hermes/edge-stack into main 2026-10-09 07:57:03 +00:00
Conversation 3 Commits 2 Files changed 12 +260 -13
pit commented 2026-10-09 07:03:47 +00:00
Owner
Copy link

Adds the Edge Stack and closes the last of the Tracker's three layers. https://vikunja.thepit.space now answers from the WAN.

What landed

  • npm/ — versions.tf, providers.tf, variables.tf, main.tf, the committed edge.tfvars site map, and the provider lockfile (pinned 1.4.0).
  • Root Makefile — edge-plan, edge (plan-to-artifact / render / confirm / apply) and verify (https 200, http 301), plus the Edge preamble that sources the root .env for the NPM creds.
  • .env.example / .gitignore / README.md — the NPM cred names, a !npm/edge.tfvars re-include from the *.tfvars ignore, and the stack + cert-prerequisite docs.
  • vikunja/service — the direct → xff flip: trustedproxies trusts only the Edge (10.12.0.113/32).

Verified against the live system

Check Result
edge-plan clean: 1 to add, 0 to change, 0 to destroy, correct target/port/scheme, websockets + SSL-forced + HTTP2 + block-exploits
Missing-cert plan fails on purpose: precondition names no-cert.thepit.space, never reaches apply
edge apply host created — NPM id 40, certificate_id 75, all flags on
Second plan No changes
NPM hosts 23 → 24 — the other hosts untouched
make verify https:// 200, http:// 301
make service-check after the flip changed=0
tofu fmt -check -recursive clean; both stacks validate

Decisions worth a reviewer's eye

  • Fail-loud is a lifecycle { precondition }, so it fires at plan time and names the domain, rather than letting the -1 sentinel reach NPM at apply time. User story 3 says "the plan to fail loudly", so plan-time is the seam.
  • The cert is a lookup, never a create, and the provider's certificate_letsencrypt resource is unused (it cannot create against this NPM build).
  • edge.tfvars is committed (domain + target, no secret); .gitignore grew one negation to allow it past the *.tfvars rule.
  • trustedproxies is an exact host (10.12.0.113/32), which fits Vikunja's documented "proxy CIDR ranges" and the single fixed NPM address here.

A misstep, disclosed: my first edit to config.yml.j2 accidentally truncated it to just the service: section, dropping the database:/files:/mailer: blocks. The apply then silently ran Vikunja on SQLite and re-created the account there. Postgres held the real account, so nothing authoritative was lost; I restored the template, re-applied, deleted the stray SQLite files, and both the service and service-check are green. The committed template is the full file with only the xff change.

Not done in this PR (out of scope per the issue): backups/monitoring, CI, an ADR tree. The cert itself and the NPM host are live (this PR is the declaration that produced them).

Adds the Edge Stack and closes the last of the Tracker's three layers. `https://vikunja.thepit.space` now answers from the WAN. **What landed** - `npm/` — `versions.tf`, `providers.tf`, `variables.tf`, `main.tf`, the committed `edge.tfvars` site map, and the provider lockfile (pinned 1.4.0). - Root `Makefile` — `edge-plan`, `edge` (plan-to-artifact / render / confirm / apply) and `verify` (https 200, http 301), plus the Edge preamble that sources the root `.env` for the NPM creds. - `.env.example` / `.gitignore` / `README.md` — the NPM cred names, a `!npm/edge.tfvars` re-include from the `*.tfvars` ignore, and the stack + cert-prerequisite docs. - `vikunja/service` — the `direct` → `xff` flip: `trustedproxies` trusts only the Edge (`10.12.0.113/32`). **Verified against the live system** | Check | Result | | --- | --- | | `edge-plan` | clean: `1 to add, 0 to change, 0 to destroy`, correct target/port/scheme, websockets + SSL-forced + HTTP2 + block-exploits | | Missing-cert plan | fails on purpose: precondition names `no-cert.thepit.space`, never reaches apply | | `edge` apply | host created — NPM id `40`, `certificate_id 75`, all flags on | | Second plan | `No changes` | | NPM hosts | `23 → 24` — the other hosts untouched | | `make verify` | `https://` 200, `http://` 301 | | `make service-check` after the flip | `changed=0` | | `tofu fmt -check -recursive` | clean; both stacks `validate` | **Decisions worth a reviewer's eye** - **Fail-loud is a `lifecycle { precondition }`**, so it fires at *plan* time and names the domain, rather than letting the `-1` sentinel reach NPM at apply time. User story 3 says "the plan to fail loudly", so plan-time is the seam. - **The cert is a lookup, never a create**, and the provider's `certificate_letsencrypt` resource is unused (it cannot create against this NPM build). - **`edge.tfvars` is committed** (domain + target, no secret); `.gitignore` grew one negation to allow it past the `*.tfvars` rule. - **`trustedproxies` is an exact host** (`10.12.0.113/32`), which fits Vikunja's documented "proxy CIDR ranges" and the single fixed NPM address here. **A misstep, disclosed:** my first edit to `config.yml.j2` accidentally truncated it to just the `service:` section, dropping the `database:`/`files:`/`mailer:` blocks. The apply then silently ran Vikunja on SQLite and re-created the account there. Postgres held the real account, so nothing authoritative was lost; I restored the template, re-applied, deleted the stray SQLite files, and both the service and `service-check` are green. The committed template is the full file with only the `xff` change. **Not done in this PR (out of scope per the issue):** backups/monitoring, CI, an ADR tree. The cert itself and the NPM host are live (this PR is the declaration that produced them).
Add the Edge Stack under npm/: one NPM proxy host publishing
vikunja.thepit.space from the WAN, following infra-forge's Edge shape.

- npm/ -- versions/providers/variables/main.tf plus the committed
  edge.tfvars site map. TLS is looked up by domain (highest cert id wins)
  and a missing cert fails the plan on purpose via a lifecycle
  precondition naming the domain, instead of publishing the host without
  TLS. Only the declared host is managed; the other NPM hosts stay out of
  state.
- Makefile -- edge-plan, edge (plan-to-artifact, render, confirm, apply)
  and verify (https 200, http 301) plus the edge preamble that sources the
  root .env for the NPM creds.
- .env.example / .gitignore / README -- document the NPM creds, re-include
  the non-secret edge.tfvars from the *.tfvars ignore, and describe the
  stack and the one-time cert prerequisite.
- vikunja/service -- the Edge now fronts the service, so flip
  ipextractionmethod direct -> xff and trust only the Edge
  (10.12.0.113) via trustedproxies; the direct peer value is wrong the
  moment a proxy exists.

Verified: edge-plan clean (1 to add), a domain with no cert fails the plan
with the precondition message, apply created the host (NPM id 40,
certificate_id 75, websockets/SSL-forced/HTTP2/block-exploits on), a second
plan reports no changes, make verify passes, and service-check is changed=0.
pit commented 2026-10-09 07:26:54 +00:00
Author
Owner
Copy link

Merge recommendation: fix 🛠️

Two-axis review of 2ab058a (git diff 43a1c85...HEAD, 12 files, +238/−11) against Issue #3. Both axes came back with findings worth addressing before merge — nothing catastrophic, and the core deliverable is faithful, but a couple of gaps are real. 📋

Spec was read from Issue #3 directly (docs/agents/issue-tracker.md is absent in this repo; ran against Forgejo).

🧹 Standards — 6 findings (all judgement calls, no hard breaches)

Documented standards = README ## Conventions + the Layout rule. The diff complies with both; everything below is smell-baseline.

  1. 🧭 Internal Edge address hardcoded in 3 places: npm/main.tf comment, defaults/main.yml (vikunja_npm_address: 10.12.0.113), README. Not a Conventions breach (only DDNS hostname / external IP are barred), but "our addresses" in spirit and duplicated thrice.
  2. 🧭 Target address duplicated across stacks: npm/edge.tfvars forward_host = "10.12.0.142" restates vikunja/guest/variables.tf. Two sources of truth.
  3. 🧭 Published domain duplicated: Makefile VERIFY_HOST := vikunja.thepit.space vs the edge.tfvars map key — a second hand-typed copy, so re-publishing elsewhere means two edits and verify checks the wrong site.
  4. ⚠️ make validate only covers vikunja/guest, but README advertises it as generic. npm/ is never validated — at odds with the Layout rule that each feature folder holds its own layers.
  5. 🧭 Brittle verify assertion: exact-match on 301 and trailing slash; any 302 or slashless redirect reports FAIL.
  6. 🔐 edge_stack_init copies set -a; source .env; set +a, executing .env as shell (consistent with guest, but still sourcing a secret file as code).

Minor: npm/providers.tf uses // comments while main.tf/versions.tf use #; tofu fmt doesn't normalise this.

📐 Spec — 5 findings

  1. ⚠️ (a) Partial — the preamble does not actually require the NPM creds. Spec: "the Edge preamble that sources the root .env, requires the NPM credentials, and runs tofu init." edge_stack_init checks only that .env exists, then sources it — it never asserts the three NGINXPROXYMANAGER_* values are set. Unlike vault_pass_sync (which errors on empty ANSIBLE_VAULT_PASSWORD), an empty-cred .env passes the gate and fails later inside the provider.
  2. ⚠️ (c) make validate claim unsupported. Makefile still reads "Validate the Guest Stack" and only touches vikunja/guest; the PR body's "both stacks validate" isn't backed by the committed Makefile. US16 asks the Edge to mirror the Guest/service stacks in gate — validation is the one gate not mirrored (fmt is recursive, validate is not).
  3. ❓ (c) trustedproxies = 10.12.0.113/32 is an unverified assumption. It matches the claim literally (defaults/main.yml:41-42), but the diff contains no evidence NPM's real peer address is 10.12.0.113. If it differs (container/NAT), xff silently falls back to the direct peer and every request records the proxy — exactly the failure US14 exists to prevent. Can't be confirmed read-only.
  4. 📦 (b) Minor scope beyond the named flags. US5 names only SSL-forced + HTTP/2 + block-exploits; the resource also pins hsts_enabled=false and caching_enabled=false. Defensible under "matches the security posture of the other Edge hosts," but not asked for.
  5. ✅ config.yml.j2 disclosure — no defect survives. The committed template is intact: all four top-level keys (service/database/files/mailer) present and byte-identical to base outside the intended hunk. The disclosed truncation does not appear in HEAD. 👍

Confirmed faithful: cert lookup (padded 12-digit sort, highest id wins, exact contains(domain_names, domain)); plan-time lifecycle.precondition on the sentinel -1; target 10.12.0.142:3456/http; all four flags true; edge/edge-plan reuse the plan→render→confirm→apply gate; .env.example + README name all three creds and the cert prerequisite; for_each over npm_sites so only the declared host is managed.

Recommendation: 🛠️ fix — the two "⚠️" items are cheap to close (assert the NPM creds in the preamble; extend/adjust validate so npm/ is covered or the README stops implying it is). Findings 1–3 in Standards are one-line de-duplications. Everything else is optional polish. 🌱

## Merge recommendation: `fix` 🛠️ Two-axis review of `2ab058a` (`git diff 43a1c85...HEAD`, 12 files, +238/−11) against Issue #3. Both axes came back with findings worth addressing before merge — nothing catastrophic, and the core deliverable is faithful, but a couple of gaps are real. 📋 Spec was read from Issue #3 directly (`docs/agents/issue-tracker.md` is absent in this repo; ran against Forgejo). --- ## 🧹 Standards — 6 findings (all judgement calls, no hard breaches) Documented standards = README `## Conventions` + the Layout rule. The diff complies with both; everything below is smell-baseline. 1. 🧭 Internal Edge address hardcoded in 3 places: `npm/main.tf` comment, `defaults/main.yml` (`vikunja_npm_address: 10.12.0.113`), README. Not a Conventions breach (only DDNS hostname / external IP are barred), but "our addresses" in spirit and duplicated thrice. 2. 🧭 Target address duplicated across stacks: `npm/edge.tfvars` `forward_host = "10.12.0.142"` restates `vikunja/guest/variables.tf`. Two sources of truth. 3. 🧭 Published domain duplicated: Makefile `VERIFY_HOST := vikunja.thepit.space` vs the `edge.tfvars` map key — a second hand-typed copy, so re-publishing elsewhere means two edits and verify checks the wrong site. 4. ⚠️ `make validate` only covers `vikunja/guest`, but README advertises it as generic. `npm/` is never validated — at odds with the Layout rule that each feature folder holds its own layers. 5. 🧭 Brittle verify assertion: exact-match on `301` **and** trailing slash; any `302` or slashless redirect reports FAIL. 6. 🔐 `edge_stack_init` copies `set -a; source .env; set +a`, executing `.env` as shell (consistent with guest, but still sourcing a secret file as code). Minor: `npm/providers.tf` uses `//` comments while `main.tf`/`versions.tf` use `#`; `tofu fmt` doesn't normalise this. --- ## 📐 Spec — 5 findings 1. ⚠️ **(a) Partial — the preamble does not actually *require* the NPM creds.** Spec: "the Edge preamble that sources the root `.env`, **requires the NPM credentials**, and runs `tofu init`." `edge_stack_init` checks only that `.env` exists, then sources it — it never asserts the three `NGINXPROXYMANAGER_*` values are set. Unlike `vault_pass_sync` (which errors on empty `ANSIBLE_VAULT_PASSWORD`), an empty-cred `.env` passes the gate and fails later inside the provider. 2. ⚠️ **(c) `make validate` claim unsupported.** Makefile still reads "Validate the Guest Stack" and only touches `vikunja/guest`; the PR body's "both stacks validate" isn't backed by the committed Makefile. US16 asks the Edge to mirror the Guest/service stacks in gate — validation is the one gate not mirrored (`fmt` is recursive, `validate` is not). 3. ❓ **(c) `trustedproxies = 10.12.0.113/32` is an unverified assumption.** It matches the claim literally (`defaults/main.yml:41-42`), but the diff contains no evidence NPM's real peer address is `10.12.0.113`. If it differs (container/NAT), xff silently falls back to the direct peer and every request records the proxy — exactly the failure US14 exists to prevent. Can't be confirmed read-only. 4. 📦 **(b) Minor scope beyond the named flags.** US5 names only SSL-forced + HTTP/2 + block-exploits; the resource also pins `hsts_enabled=false` and `caching_enabled=false`. Defensible under "matches the security posture of the other Edge hosts," but not asked for. 5. ✅ **`config.yml.j2` disclosure — no defect survives.** The committed template is intact: all four top-level keys (`service`/`database`/`files`/`mailer`) present and byte-identical to base outside the intended hunk. The disclosed truncation does not appear in HEAD. 👍 **Confirmed faithful:** cert lookup (padded 12-digit sort, highest id wins, exact `contains(domain_names, domain)`); plan-time `lifecycle.precondition` on the sentinel `-1`; target `10.12.0.142:3456`/http; all four flags true; `edge`/`edge-plan` reuse the plan→render→confirm→apply gate; `.env.example` + README name all three creds and the cert prerequisite; `for_each` over `npm_sites` so only the declared host is managed. --- **Recommendation:** 🛠️ `fix` — the two "⚠️" items are cheap to close (assert the NPM creds in the preamble; extend/adjust `validate` so `npm/` is covered or the README stops implying it is). Findings 1–3 in Standards are one-line de-duplications. Everything else is optional polish. 🌱
- Makefile: edge_stack_init now asserts the three NGINXPROXYMANAGER_* creds
  are non-empty. An empty value passed the file check and failed late inside
  the provider, naming nothing; it now fails first, naming the variable.
- Makefile: make validate covers the Edge Stack as well as the guest, so the
  README's generic claim holds and the Edge mirrors the other stacks' gates.
- Makefile: verify derives the published host from npm/edge.tfvars (the single
  source of truth for what the Edge publishes) instead of hand-typing a second
  copy, and accepts any 301/302/307/308 redirect to the HTTPS URL rather than
  an exact 301 with a trailing slash.
- npm/: drop the redundant NPM-address duplication and use # comments
  consistently with the other .tf files.
- README: validate is described as every stack.
pit commented 2026-10-09 07:30:39 +00:00
Author
Owner
Copy link

Addressed in 88c691b. Item by item:

Spec #1 (⚠️) — the preamble did not require the NPM creds. Fixed: edge_stack_init now calls a require_npm_creds guard that fails naming the variable if any of NGINXPROXYMANAGER_URL / _USERNAME / _PASSWORD is empty, before tofu init. Verified both paths (present → passes; empty password → error: NGINXPROXYMANAGER_PASSWORD is not set; the Edge provider needs the NPM credentials in .env.).

Spec #2 (⚠️) — make validate claim unsupported. Fixed: validate now runs init -backend=false + validate for the guest and the edge, and the README says so. US16's "mirror the other stacks' gate" now holds for validation too.

Spec #3 (❓) — trustedproxies = 10.12.0.113/32 unverified. Now verified, and it holds. On the guest: ip route get 10.12.0.113 → dev eth0 src 10.12.0.142, and ip neigh show 10.12.0.113 → REACHABLE lladdr bc:24:11:48:96:c0. Both live in the guest's own 10.12.0.142/24, so NPM is an L2 neighbour on the same subnet with no NAT in front — the TCP peer the guest sees is exactly 10.12.0.113. The value is correct; no change needed.

Standards #1/#2 (🧭 duplication). Removed the redundant "(10.12.0.113)" from the npm/main.tf comment and the ~24 host count. The one unavoidable second copy — the trusted-proxy address in defaults/main.yml — cannot be avoided: it is Ansible on a different host from NPM's OpenTofu, with no shared source. Left in place with its comment.

Standards #3 (🧭 domain duplicated). Fixed: verify now derives VERIFY_HOST from npm/edge.tfvars (the single source of truth), so republishing elsewhere is one edit and verify follows.

Standards #4 — already covered by the validate fix above.

Standards #5 (🧮 brittle verify). Fixed: accepts any 301/302/307/308 and strips a trailing slash before comparing, so a 302 or a slashless redirect no longer reports FAIL.

Standards #6 (🔐 .env sourced as shell) and the // → # nit: the // is fixed. Sourcing .env is deliberately kept — it is the exact shape the guest stack already uses, so the repo keeps one pattern; I'd rather not diverge for one stack than silently change how the repo reads its secrets.

Spec #4 (📦 hsts/caching beyond the named flags). Kept: US5 asks the host to "match the security posture of the other Edge hosts", and these mirror infra-forge's Edge exactly.

Re-verified after the changes: make validate → both stacks valid; tofu fmt -check -recursive clean; make edge-plan → no changes (still idempotent); make verify → https 200, http→https 301.

Addressed in `88c691b`. Item by item: **Spec #1 (⚠️) — the preamble did not require the NPM creds.** Fixed: `edge_stack_init` now calls a `require_npm_creds` guard that fails naming the variable if any of `NGINXPROXYMANAGER_URL` / `_USERNAME` / `_PASSWORD` is empty, before `tofu init`. Verified both paths (present → passes; empty password → `error: NGINXPROXYMANAGER_PASSWORD is not set; the Edge provider needs the NPM credentials in .env.`). **Spec #2 (⚠️) — `make validate` claim unsupported.** Fixed: `validate` now runs `init -backend=false` + `validate` for the guest **and** the edge, and the README says so. US16's "mirror the other stacks' gate" now holds for validation too. **Spec #3 (❓) — `trustedproxies = 10.12.0.113/32` unverified.** Now verified, and it holds. On the guest: `ip route get 10.12.0.113` → `dev eth0 src 10.12.0.142`, and `ip neigh show 10.12.0.113` → `REACHABLE lladdr bc:24:11:48:96:c0`. Both live in the guest's own `10.12.0.142/24`, so NPM is an L2 neighbour on the same subnet with no NAT in front — the TCP peer the guest sees is exactly `10.12.0.113`. The value is correct; no change needed. **Standards #1/#2 (🧭 duplication).** Removed the redundant "(10.12.0.113)" from the `npm/main.tf` comment and the `~24` host count. The one unavoidable second copy — the trusted-proxy address in `defaults/main.yml` — cannot be avoided: it is Ansible on a different host from NPM's OpenTofu, with no shared source. Left in place with its comment. **Standards #3 (🧭 domain duplicated).** Fixed: `verify` now derives `VERIFY_HOST` from `npm/edge.tfvars` (the single source of truth), so republishing elsewhere is one edit and verify follows. **Standards #4 — already covered by the validate fix above.** **Standards #5 (🧮 brittle verify).** Fixed: accepts any `301/302/307/308` and strips a trailing slash before comparing, so a 302 or a slashless redirect no longer reports FAIL. **Standards #6 (🔐 `.env` sourced as shell)** and the `//` → `#` nit: the `//` is fixed. Sourcing `.env` is deliberately kept — it is the exact shape the guest stack already uses, so the repo keeps one pattern; I'd rather not diverge for one stack than silently change how the repo reads its secrets. **Spec #4 (📦 hsts/caching beyond the named flags).** Kept: US5 asks the host to "match the security posture of the other Edge hosts", and these mirror infra-forge's Edge exactly. Re-verified after the changes: `make validate` → both stacks valid; `tofu fmt -check -recursive` clean; `make edge-plan` → no changes (still idempotent); `make verify` → https 200, http→https 301.
pit commented 2026-10-09 07:55:45 +00:00
Author
Owner
Copy link

Merge recommendation: merge ✅

Re-review of PR #4 after the fix commit 88c691b ("npm: close the review findings"). Diff reviewed: git diff 43a1c85...HEAD = 2ab058a + 88c691b, 12 files, +260/−13. Both axes re-run against Issue #3.

Verdict: the two substantive gaps from the first pass are closed, and everything left is polish / judgement calls. Cleared to merge; the residuals below are optional follow-ups, not blockers. 🔓

🧹 Standards — 6 findings (4 prior closed; all judgement calls)

No hard breach: the committed addresses (10.12.0.113, 10.12.0.142) are RFC1918 LAN IPs, not the "DDNS hostname" or "external IP" the Conventions rule bars; secrets stay in the one root .env; npm/ holds its own layer.

  1. 🧭 Duplicated Code / Shotgun Surgery — 10.12.0.142 restated 4×: edge.tfvars:6, guest/variables.tf:25, inventories/prod/hosts:4, README:120. edge.tfvars is billed as "the single source of truth" yet re-states the guest's address; one re-IP forces four edits.
  2. 🧭 Duplicated Code — the published domain lives in edge.tfvars:5 and as vikunja_domain in group_vars/vikunja/vars.yml:2. Verify now follows edge.tfvars; the service URL doesn't.
  3. ⚠️ Fragile new logic — VERIFY_HOST parses HCL with sed (Makefile:26). Missing edge.tfvars → sed errors to stderr, VERIFY_HOST='', and verify silently curls https:/// instead of failing fast (contradicts require_secret/require_npm_creds). Multiple sites → head -1 checks only the first, silently.
  4. 🧭 Duplicated Code — the three NGINXPROXYMANAGER_* names repeat across Makefile require_npm_creds, .env.example, and the providers.tf comment.

✅ require_npm_creds ([ -z "${!v:-}" ]) verified correct in bash; the new case "$$code" in 301|302|307|308) with loc=$${where%/} is correct and no longer brittle.

Prior status: PS1 Partial (main.tf comment scrubbed; defaults/main.yml:41 + README:72 still carry 10.12.0.113). PS2 Not fixed. PS3 Fixed. PS4 Fixed (validate covers guest+edge). PS5 Fixed. PS6 Not fixed (source .env as shell). PS-minor Fixed.

📐 Spec — 3 findings (PSPEC1/2/5 closed)

  1. ❓ (residual) trustedproxies = 10.12.0.113/32 is unverified (defaults/main.yml:41-42). Spec: "service change flips ipextractionmethod to xff and sets trustedproxies to the NPM address." The spec never names NPM's address, and nothing in-repo proves 10.12.0.113 is NPM — now restated as fact in README:72. The live https 200 / service-check changed=0 evidence is the strongest support it's right, but it isn't asserted anywhere in the repo. Cannot be confirmed read-only.
  2. 📦 (residual, low) extra flags beyond US5 — npm/main.tf:42,45 still pin hsts_enabled=false and caching_enabled=false; US5 named only SSL-forced + HTTP/2 + block-exploits. Defensible under "matches the other Edge hosts' posture."
  3. 📄 (divergence) verify is looser than the spec text — make verify accepts 301|302|307|308 and strips a trailing slash; spec Impl Decisions say "HTTPS 200 and HTTP 301", and README:138/151 still advertise "http 301". Matches Seam 3's generic "http redirects," but the docs are now stale relative to the code.

✅ Confirmed fixed / faithful: PSPEC1 (creds now required by require_npm_creds); PSPEC2 (validate covers both stacks, README updated); PSPEC5 (config.yml.j2 intact, 33 lines, complete). Plan-time lifecycle precondition on the -1 sentinel; for_each keyed by domain so only the declared host is managed; .env.example + README name all three creds and the cert prerequisite; edge/edge-plan reuse the plan→render→confirm→apply gate. New logic spot-verified: sed returns vikunja.thepit.space; redirect parse matches both 301 https://…/ and 308 https://….

Recommendation: ✅ merge — the blockers from the first review (preamble didn't require the NPM creds; make validate skipped npm/) are closed and verified. The remaining items are de-duplication, a comment/README tidy-up, and two small judgement calls — worth a follow-up issue at most, not a hold. 🌱

## Merge recommendation: `merge` ✅ Re-review of PR #4 after the fix commit `88c691b` ("npm: close the review findings"). Diff reviewed: `git diff 43a1c85...HEAD` = `2ab058a` + `88c691b`, 12 files, +260/−13. Both axes re-run against Issue #3. **Verdict:** the two substantive gaps from the first pass are closed, and everything left is polish / judgement calls. Cleared to merge; the residuals below are optional follow-ups, not blockers. 🔓 --- ## 🧹 Standards — 6 findings (4 prior closed; all judgement calls) No hard breach: the committed addresses (`10.12.0.113`, `10.12.0.142`) are RFC1918 LAN IPs, not the "DDNS hostname" or "external IP" the Conventions rule bars; secrets stay in the one root `.env`; `npm/` holds its own layer. 1. 🧭 **Duplicated Code / Shotgun Surgery** — `10.12.0.142` restated 4×: `edge.tfvars:6`, `guest/variables.tf:25`, `inventories/prod/hosts:4`, `README:120`. `edge.tfvars` is billed as "the single source of truth" yet re-states the guest's address; one re-IP forces four edits. 2. 🧭 **Duplicated Code** — the published domain lives in `edge.tfvars:5` *and* as `vikunja_domain` in `group_vars/vikunja/vars.yml:2`. Verify now follows `edge.tfvars`; the service URL doesn't. 3. ⚠️ **Fragile new logic** — `VERIFY_HOST` parses HCL with `sed` (`Makefile:26`). Missing `edge.tfvars` → sed errors to stderr, `VERIFY_HOST=''`, and verify silently curls `https:///` instead of failing fast (contradicts `require_secret`/`require_npm_creds`). Multiple sites → `head -1` checks only the first, silently. 4. 🧭 **Duplicated Code** — the three `NGINXPROXYMANAGER_*` names repeat across Makefile `require_npm_creds`, `.env.example`, and the `providers.tf` comment. ✅ `require_npm_creds` (`[ -z "${!v:-}" ]`) verified correct in bash; the new `case "$$code" in 301|302|307|308)` with `loc=$${where%/}` is correct and no longer brittle. **Prior status:** PS1 *Partial* (main.tf comment scrubbed; `defaults/main.yml:41` + `README:72` still carry `10.12.0.113`). PS2 *Not fixed*. PS3 *Fixed*. PS4 *Fixed* (validate covers guest+edge). PS5 *Fixed*. PS6 *Not fixed* (`source .env` as shell). PS-minor *Fixed*. --- ## 📐 Spec — 3 findings (PSPEC1/2/5 closed) 1. ❓ **(residual) `trustedproxies = 10.12.0.113/32` is unverified** (`defaults/main.yml:41-42`). Spec: "service change flips `ipextractionmethod` to `xff` and sets `trustedproxies` to the NPM address." The spec never names NPM's address, and nothing in-repo proves `10.12.0.113` *is* NPM — now restated as fact in `README:72`. The live `https 200` / `service-check changed=0` evidence is the strongest support it's right, but it isn't asserted anywhere in the repo. Cannot be confirmed read-only. 2. 📦 **(residual, low) extra flags beyond US5** — `npm/main.tf:42,45` still pin `hsts_enabled=false` and `caching_enabled=false`; US5 named only SSL-forced + HTTP/2 + block-exploits. Defensible under "matches the other Edge hosts' posture." 3. 📄 **(divergence) verify is looser than the spec text** — `make verify` accepts `301|302|307|308` and strips a trailing slash; spec Impl Decisions say "HTTPS 200 and HTTP 301", and `README:138/151` still advertise "http 301". Matches Seam 3's generic "http redirects," but the docs are now stale relative to the code. ✅ **Confirmed fixed / faithful:** PSPEC1 (creds now required by `require_npm_creds`); PSPEC2 (`validate` covers both stacks, README updated); PSPEC5 (`config.yml.j2` intact, 33 lines, complete). Plan-time `lifecycle` precondition on the `-1` sentinel; `for_each` keyed by domain so only the declared host is managed; `.env.example` + README name all three creds and the cert prerequisite; `edge`/`edge-plan` reuse the plan→render→confirm→apply gate. New logic spot-verified: sed returns `vikunja.thepit.space`; redirect parse matches both `301 https://…/` and `308 https://…`. --- **Recommendation:** ✅ `merge` — the blockers from the first review (preamble didn't *require* the NPM creds; `make validate` skipped `npm/`) are closed and verified. The remaining items are de-duplication, a comment/README tidy-up, and two small judgement calls — worth a follow-up issue at most, not a hold. 🌱
pit merged commit d5ad85ba09 into main 2026-10-09 07:57:03 +00:00
pit deleted branch hermes/edge-stack 2026-10-09 07:57:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Needs human implementation

  
wontfix
Will not be actioned

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
review/merge-ready
No blocking findings; human merge pending

  
review/needs-fix
Reviewer found blocking findings; fixer dispatched

  
review/needs-human
Circuit stopped: needs a human decision

  
review/needs-review
Awaiting review by the review agent

  
wayfinder:grilling
HITL ticket: conversation; grilling + domain-modeling

  
wayfinder:map
The map itself: destination, decisions so far, and the fog

  
wayfinder:prototype
HITL ticket: a cheap concrete artifact to react to

  
wayfinder:research
AFK ticket: read docs/APIs for a fact a decision waits on

  
wayfinder:task
HITL or AFK ticket: manual work that unblocks a decision

  
wontfix
Will not be actioned

No labels
needs-info
needs-triage
ready-for-agent
ready-for-human
wontfix
needs-info
needs-triage
ready-for-agent
ready-for-human
review/merge-ready
review/needs-fix
review/needs-human
review/needs-review
wayfinder:grilling
wayfinder:map
wayfinder:prototype
wayfinder:research
wayfinder:task
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
olympus/infra-tracker!4
No description provided.