issue-88: Implement git pull and npm install steps in scripts/local-install.mjs #96

Merged

david merged 2 commits from feature/issue-88/implement-git-pull-and-npm-install-steps-in-script into main

2026-08-28 00:57:34 +00:00

david commented

2026-08-28 00:57:32 +00:00

Owner

88: Implement git pull and npm install steps in scripts/local-install.mjs

Implements 88.

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 34m 00s.

Review findings

Now let me check the CHANGELOG and docs referenced by the findings:

The working tree is clean (changes committed on the feature branch), and npm test is the only gate. Let me check the docs that pin down the failure-message formats before changing them:

The grep tool mangled the multi-word pattern. Let me use bash directly:

The inline format is pinned in both CHANGELOG.md:11 and docs/IMPLEMENTATION_PLAN.md:42,45. Let me see the plan doc context:

I now have the full picture. Applying fixes highest severity first:

Medium — test coverage for failureDetail() fallback branches: add a silent-fail pull shim mode + test.

Low — multi-line failure messages: restructure gitPull/npmInstall fail messages, and update CHANGELOG + plan doc which pin the format.

Low — blank line on empty output: guard the console.log.

Now update the docs that pin the message format (CHANGELOG per the finding, plus the plan doc for consistency):

Now the test file — extract the shared withMockedExit helper (Low finding), add the silent-fail shim mode + exit-status test (Medium finding), and pin the new multi-line message layout:

Now run the project's test gate (only test script exists in package.json — no build/lint):

One regression: in my edit of runMainAndCaptureAbort I dropped npmFails from the makeShimDir call. Fixing:

All 35 tests pass (34 prior + 1 new). Final sanity check — verify the working-tree diff and that no stale references to the old inline format remain:

The README hit is just a preflight remediation table row (advice, not message format) — fine. But package.json/package-lock.json show unexpected changes I didn't make. Let me inspect:

I found that my earlier grep command with unquoted patterns accidentally executed npm install failed in the repo, adding an unwanted dependency. I'll revert those changes now.

Working tree now contains only the four intended files. Re-running the gate once more to confirm green state:

All gates green (35/35, up from 34 with the new test). All four findings are resolved; here is the annotated findings document.

Review Findings — 88

Summary

All four findings fixed in a single pass: (1) added a pullSilentFail shim mode (pull case body is just exit 1) plus a test covering the failureDetail() exit-status fallback; (2) failure details now print on their own line(s) below the pull failed: / npm install failed: header, with CHANGELOG.md and docs/IMPLEMENTATION_PLAN.md updated to match; (3) empty step output no longer prints a blank line (if (out) guard in both steps); (4) extracted a shared withMockedExit(fn) helper reused by runPreflightAndCaptureAbort, runMainAndCaptureAbort, and the "main warns and continues" test. Gate: npm test passes 35/35 (incl. 1 new test). No build/lint scripts exist at repo root. Nothing remains unresolved.

Critical

(empty)

High

(empty)

Medium

scripts/local-install.test.mjs:150 — The new failureDetail() helper (scripts/local-install.mjs:241) has four branches but only the stderr branch is exercised; the fallbacks (non-zero exit with no output → exit status N, spawn error → ENOENT message, which I verified are reachable via spawnSync) have no test coverage. Suggested fix: add a shim mode where the pull case body is just exit 1 (no stderr), then assert the captured abort message matches /pull failed: exit status 1/.

Low

scripts/local-install.mjs:345 — Multi-line stderr is inlined into the single-line Aborted: message, so the trailing hint lands mid-block on the last line of stderr (git/npm failures are typically multi-line), e.g. Aborted: pull failed: line1\nline2 — check your network/credentials. Suggested fix: put the detail on its own line(s), e.g. fail(\pull failed:\n${failureDetail(result)}\nCheck your network/credentials`)`; note CHANGELOG.md documents the current inline format, so update it if changed.
scripts/local-install.mjs:347 — console.log(combinedOutput(result)) prints a blank line when the step produced no output (same at :355 for npm install). Suggested fix: guard with const out = combinedOutput(result); if (out) console.log(out);.
scripts/local-install.test.mjs:150 — runMainAndCaptureAbort duplicates the env-patch + process.exit/console.error mock scaffolding already present in runPreflightAndCaptureAbort (~line 108) and inline in the "main warns and continues" test. Suggested fix: extract a shared helper (e.g. withMockedExit(fn)) returning { stderr, aborted } and reuse it in all three places.

## 88: Implement git pull and npm install steps in scripts/local-install.mjs Implements [88](https://git.excelera.net/david/pi-extensions-and-skills/issues/88). > 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 34m 00s. ## Review findings Now let me check the CHANGELOG and docs referenced by the findings: The working tree is clean (changes committed on the feature branch), and `npm test` is the only gate. Let me check the docs that pin down the failure-message formats before changing them: The grep tool mangled the multi-word pattern. Let me use bash directly: The inline format is pinned in both CHANGELOG.md:11 and docs/IMPLEMENTATION_PLAN.md:42,45. Let me see the plan doc context: I now have the full picture. Applying fixes highest severity first: **Medium — test coverage for `failureDetail()` fallback branches:** add a silent-fail pull shim mode + test. **Low — multi-line failure messages:** restructure `gitPull`/`npmInstall` fail messages, and update CHANGELOG + plan doc which pin the format. **Low — blank line on empty output:** guard the `console.log`. Now update the docs that pin the message format (CHANGELOG per the finding, plus the plan doc for consistency): Now the test file — extract the shared `withMockedExit` helper (Low finding), add the silent-fail shim mode + exit-status test (Medium finding), and pin the new multi-line message layout: Now run the project's test gate (only `test` script exists in package.json — no build/lint): One regression: in my edit of `runMainAndCaptureAbort` I dropped `npmFails` from the `makeShimDir` call. Fixing: All 35 tests pass (34 prior + 1 new). Final sanity check — verify the working-tree diff and that no stale references to the old inline format remain: The README hit is just a preflight remediation table row (advice, not message format) — fine. But `package.json`/`package-lock.json` show unexpected changes I didn't make. Let me inspect: I found that my earlier grep command with unquoted patterns accidentally executed `npm install failed` in the repo, adding an unwanted dependency. I'll revert those changes now. Working tree now contains only the four intended files. Re-running the gate once more to confirm green state: All gates green (35/35, up from 34 with the new test). All four findings are resolved; here is the annotated findings document. # Review Findings — 88 ## Summary All four findings fixed in a single pass: (1) added a `pullSilentFail` shim mode (pull case body is just `exit 1`) plus a test covering the `failureDetail()` exit-status fallback; (2) failure details now print on their own line(s) below the `pull failed:` / `npm install failed:` header, with CHANGELOG.md and docs/IMPLEMENTATION_PLAN.md updated to match; (3) empty step output no longer prints a blank line (`if (out)` guard in both steps); (4) extracted a shared `withMockedExit(fn)` helper reused by `runPreflightAndCaptureAbort`, `runMainAndCaptureAbort`, and the "main warns and continues" test. Gate: `npm test` passes 35/35 (incl. 1 new test). No build/lint scripts exist at repo root. Nothing remains unresolved. ## Critical (empty) ## High (empty) ## Medium - [x] scripts/local-install.test.mjs:150 — The new `failureDetail()` helper (scripts/local-install.mjs:241) has four branches but only the stderr branch is exercised; the fallbacks (non-zero exit with no output → `exit status N`, spawn error → ENOENT message, which I verified are reachable via `spawnSync`) have no test coverage. Suggested fix: add a shim mode where the `pull` case body is just `exit 1` (no stderr), then assert the captured abort message matches `/pull failed: exit status 1/`. ## Low - [x] scripts/local-install.mjs:345 — Multi-line stderr is inlined into the single-line `Aborted:` message, so the trailing hint lands mid-block on the last line of stderr (git/npm failures are typically multi-line), e.g. `Aborted: pull failed: line1\nline2 — check your network/credentials`. Suggested fix: put the detail on its own line(s), e.g. `fail(\`pull failed:\n${failureDetail(result)}\nCheck your network/credentials\`)`; note CHANGELOG.md documents the current inline format, so update it if changed. - [x] scripts/local-install.mjs:347 — `console.log(combinedOutput(result))` prints a blank line when the step produced no output (same at :355 for npm install). Suggested fix: guard with `const out = combinedOutput(result); if (out) console.log(out);`. - [x] scripts/local-install.test.mjs:150 — `runMainAndCaptureAbort` duplicates the env-patch + `process.exit`/`console.error` mock scaffolding already present in `runPreflightAndCaptureAbort` (~line 108) and inline in the "main warns and continues" test. Suggested fix: extract a shared helper (e.g. `withMockedExit(fn)`) returning `{ stderr, aborted }` and reuse it in all three places.

david added 2 commits

2026-08-28 00:57:32 +00:00