45: Fix local:install preflight: env-var requirements should not block install #46

Merged

david merged 2 commits from feature/issue-45/fix-local-install-preflight-env-var-requirements-s into main

2026-08-18 21:36:45 +00:00

david commented

2026-08-18 21:36:43 +00:00

Owner

45: Fix local:install preflight: env-var requirements should not block install

Implements 45.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 6m 10s.

Review findings

Now I have the full picture. Let me apply the fixes. Starting with scripts/local-install.mjs (finding: dead dotEnv parameter):Let me check the exact content around those functions:The content matches — let me use smaller, targeted edits instead:Now the test file — update the hasConfigValue test and add the main warn-and-continue orchestration test:Now the documentation wording fix in docs/IMPLEMENTATION_PLAN.md:Now let me verify the changes and run the gates:All 31 tests pass (30 existing + 1 new). Let me review the final diff and confirm the working tree state:All gates pass. The three code/doc findings are resolved; the commit-message finding cannot be fixed in the working tree (the stage prohibits committing, and the suggested fix is the eventual PR title at merge time).

Review Findings — 45

Summary

The change correctly removes the env-var hard gates from local:install preflight: the DATABASE_URL check is deleted and a missing FORGEJO_TOKEN now prints a [local:install] Warning: to stderr and continues. Behavior is consistent between code, README, and IMPLEMENTATION_PLAN; no stale references to the old fail-fast messages remain. Gates run: build — no build step exists (plain Node ESM + pi-loaded .ts extensions; no compile script), node --check passes on both changed scripts; test — npm test passes 30/30; lint — no lint script/config is configured in the repo. The change is small, correct, and adequately tested at the preflight level; findings below are minor.

Remediation (single pass): Added a full main() orchestration test asserting warn-and-continue with FORGEJO_TOKEN unset (warning on stderr, pull → npm install → registration all still run, no abort); reworded the IMPLEMENTATION_PLAN warn-only rationale to match the env-only behavior; dropped the now-dead dotEnv parameter from hasConfigValue and noted the .env helpers are retained for tests. npm test now passes 31/31. Remaining: the commit-message finding cannot be applied in the working tree (stage prohibits commit/stage), so it stays unchecked — the fix is the PR title at merge time.

Critical

High

Medium

scripts/local-install.test.mjs:416 — No end-to-end test covers the warn-and-continue path through the full main() flow (pull → npm install → register) with FORGEJO_TOKEN unset; the unit test at line 346 only proves preflight does not abort, while docs/IMPLEMENTATION_PLAN.md:83 claims "install continues (exit 0)". Suggested fix: add a variant of the main orchestration test (e.g. omit FORGEJO_TOKEN at line 433) and assert the git/npm/pi shim logs are still written and no abort occurs, locking in the continuation behavior against future regressions.

Low

docs/IMPLEMENTATION_PLAN.md:38 — The warn-only rationale says env vars "live in per-machine .env files, not the checkout", but the check hasConfigValue("FORGEJO_TOKEN", env, {}) (scripts/local-install.mjs:307) and the forgejo extension itself (extensions/forgejo/src/forgejo.ts:26) read process.env only — a token placed in .env still produces the warning. Suggested fix: reword the rationale to "per-machine environment (export FORGEJO_TOKEN), not the checkout" so the doc matches the behavior, or read .env via readDotEnv if .env support is intended.
scripts/local-install.mjs:307 — After removing the readDotEnv(cwd) call, the dotEnv parameter of hasConfigValue is dead in the production path (only caller passes a hard-coded {}), and the .env-aware helpers (readDotEnv, parseEnvFile) are now exercised only by tests. Suggested fix (optional polish): drop the dotEnv parameter / export or add a comment noting these helpers are retained for tests, so future readers don't mistake the path as live.
Commit d7ac06d — Message 45: Fix local:install preflight: env-var requirements should not block install does not match the AGENTS.md convention issue-<N>: <summary> (missing the issue- prefix). Suggested fix: use issue-45: … as the PR title (repo is squash-merged, so the PR title becomes the commit on main).

## 45: Fix local:install preflight: env-var requirements should not block install Implements [45](https://git.excelera.net/david/pi-extensions-and-skills/issues/45). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 6m 10s. ## Review findings Now I have the full picture. Let me apply the fixes. Starting with `scripts/local-install.mjs` (finding: dead `dotEnv` parameter):Let me check the exact content around those functions:The content matches — let me use smaller, targeted edits instead:Now the test file — update the `hasConfigValue` test and add the `main` warn-and-continue orchestration test:Now the documentation wording fix in `docs/IMPLEMENTATION_PLAN.md`:Now let me verify the changes and run the gates:All 31 tests pass (30 existing + 1 new). Let me review the final diff and confirm the working tree state:All gates pass. The three code/doc findings are resolved; the commit-message finding cannot be fixed in the working tree (the stage prohibits committing, and the suggested fix is the eventual PR title at merge time). # Review Findings — 45 ## Summary The change correctly removes the env-var hard gates from `local:install` preflight: the `DATABASE_URL` check is deleted and a missing `FORGEJO_TOKEN` now prints a `[local:install] Warning:` to stderr and continues. Behavior is consistent between code, README, and IMPLEMENTATION_PLAN; no stale references to the old fail-fast messages remain. Gates run: **build** — no build step exists (plain Node ESM + pi-loaded `.ts` extensions; no compile script), `node --check` passes on both changed scripts; **test** — `npm test` passes 30/30; **lint** — no lint script/config is configured in the repo. The change is small, correct, and adequately tested at the preflight level; findings below are minor. **Remediation (single pass):** Added a full `main()` orchestration test asserting warn-and-continue with `FORGEJO_TOKEN` unset (warning on stderr, pull → npm install → registration all still run, no abort); reworded the IMPLEMENTATION_PLAN warn-only rationale to match the env-only behavior; dropped the now-dead `dotEnv` parameter from `hasConfigValue` and noted the `.env` helpers are retained for tests. `npm test` now passes 31/31. Remaining: the commit-message finding cannot be applied in the working tree (stage prohibits commit/stage), so it stays unchecked — the fix is the PR title at merge time. ## Critical - ## High - ## Medium - [x] `scripts/local-install.test.mjs:416` — No end-to-end test covers the warn-and-continue path through the full `main()` flow (pull → npm install → register) with `FORGEJO_TOKEN` unset; the unit test at line 346 only proves `preflight` does not abort, while `docs/IMPLEMENTATION_PLAN.md:83` claims "install continues (exit 0)". Suggested fix: add a variant of the `main` orchestration test (e.g. omit `FORGEJO_TOKEN` at line 433) and assert the git/npm/pi shim logs are still written and no abort occurs, locking in the continuation behavior against future regressions. ## Low - [x] `docs/IMPLEMENTATION_PLAN.md:38` — The warn-only rationale says env vars "live in per-machine `.env` files, not the checkout", but the check `hasConfigValue("FORGEJO_TOKEN", env, {})` (`scripts/local-install.mjs:307`) and the forgejo extension itself (`extensions/forgejo/src/forgejo.ts:26`) read `process.env` only — a token placed in `.env` still produces the warning. Suggested fix: reword the rationale to "per-machine environment (export FORGEJO_TOKEN), not the checkout" so the doc matches the behavior, or read `.env` via `readDotEnv` if `.env` support is intended. - [x] `scripts/local-install.mjs:307` — After removing the `readDotEnv(cwd)` call, the `dotEnv` parameter of `hasConfigValue` is dead in the production path (only caller passes a hard-coded `{}`), and the `.env`-aware helpers (`readDotEnv`, `parseEnvFile`) are now exercised only by tests. Suggested fix (optional polish): drop the `dotEnv` parameter / export or add a comment noting these helpers are retained for tests, so future readers don't mistake the path as live. - [ ] Commit `d7ac06d` — Message `45: Fix local:install preflight: env-var requirements should not block install` does not match the AGENTS.md convention `issue-<N>: <summary>` (missing the `issue-` prefix). Suggested fix: use `issue-45: …` as the PR title (repo is squash-merged, so the PR title becomes the commit on `main`).

david added 2 commits

2026-08-18 21:36:44 +00:00