issue-112: Add zip-extraction dependency (adm-zip vs yauzl) #124

Merged

david merged 3 commits from feature/issue-112/add-zip-extraction-dependency-adm-zip-vs-yauzl into main

2026-08-29 07:19:02 +00:00

david commented

2026-08-29 07:19:01 +00:00

Owner

112: Add zip-extraction dependency (adm-zip vs yauzl)

Implements 112.

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 9m 18s.

Review findings

Now I have a full picture. Let me apply the fixes. First, the Critical finding — add adm-zip to the root pi-package manifest:adm-zip now resolves for the extension (nested dev copy here; on a fresh clone the root node_modules provides it since the nested one is gitignored). Now the README install section (High + Medium findings):Now the actions.ts comment — trim the inline rationale (Low 2) and make the {@link ZipGuardLimits} resolve (Low 1) by importing the type:Now move the full rationale into the project decision record (docs/DESIGN.md Tradeoffs) per Low 2's suggested fix:Now re-run the gates. Root test and the forgejo extension's check/build/test:All gates pass. Let me review the final diff:Let me do a lightweight fresh-checkout simulation (copy tree without git/node_modules, npm install, resolve the extension's adm-zip import):All fixes are applied and gates are green. Fresh-checkout simulation confirms the Critical fix: a tree without the gitignored nested node_modules resolves adm-zip from the root install.

Review Findings — 112

Summary

Gates run and results: root npm test (node --test, 57/57 pass), forgejo extension npm run check (tsc --noEmit, pass), npm run build (tsc emit, pass), and npm test (jest, 241/241 pass). No lint script exists in either package.json, so no lint gate was available. The change under review is a single commit that adds a 23-line rationale comment to extensions/forgejo/src/actions.ts; it does not add or change any dependency. The adm-zip dependency and the extractRunLogsZip/ZipGuardLimits code it documents already exist from issue-111 — but only in the nested extensions/forgejo/package.json, which is not the install path pi/install:local uses, so the pi package's actual dependency manifest is still missing adm-zip and would fail at runtime for consumers.

Remediation applied in this pass: added "adm-zip": "^0.6.0" to the root pi-package dependencies and regenerated package-lock.json (verified via a fresh tree copy + root npm install that extensions/forgejo/src/index.ts resolves adm-zip); updated the README install section (pi-package flow + corrected manual flat-copy recipe with adm-zip); trimmed the inline rationale in actions.ts to a pointer with resolvable {@link}s and moved the full adm-zip-vs-yauzl comparison into docs/DESIGN.md Tradeoffs. Remaining: the commit-message rewrite (Low) is out of scope for this stage, which must not commit/stage/push and only captures the working-tree diff.

Critical

package.json:28 — The issue's core task ("Add zip-extraction dependency") is not implemented where pi actually installs deps. The root pi-package manifest declares only dotenv/pg/pgpass/typebox (lines 28–33) and the root package-lock.json has no adm-zip entry; there is no workspaces field, scripts/local-install.mjs runs npm install only at the repo root, and extensions/forgejo/node_modules/ is gitignored. But extensions/forgejo/src/index.ts eagerly imports ./actions → ./actionsCache → import AdmZip from "adm-zip" (actionsCache.ts:4), so any machine that installs this package via pi install or npm run install:local will fail to load the entire forgejo extension (Cannot find package 'adm-zip'). The comment added at actions.ts:18-19 ("runtime dep of extensions/forgejo") is therefore misleading for package consumers; the dev machine only works because of a manually-installed, gitignored nested node_modules. Suggested fix: add "adm-zip": "^0.6.0" to the root package.json dependencies and run npm install at the repo root to regenerate package-lock.json (verify with a fresh clone + install:local that the extension loads).

High

extensions/forgejo/README.md:32 — The documented manual-install flow for this extension is now broken by the actions tools: the snippet runs cd ~/.pi/agent/extensions/forgejo && npm install typebox only, but the actions tools (documented in this same README at lines 83–132) import adm-zip at module load. A user following the documented install path gets a forgejo extension that fails to load. Suggested fix: update the snippet to npm install typebox adm-zip and add a line noting forgejo_action_run_logs requires the adm-zip runtime dependency (and that the @types/adm-zip devDep is for development only).

Medium

extensions/forgejo/README.md:25-33 — The manual-install section copies src/*.ts and hand-writes the extension package.json; the new src/actionsCache.ts (and its adm-zip import) was added by issue-111 but this install recipe was never updated, so it is doubly stale (missing file copy and missing dep). Suggested fix: revise the recipe to copy all current sources and to include adm-zip in the package.json it asks the user to write, or replace the section with a pointer to the pi-package install flow.

Low

extensions/forgejo/src/actions.ts:36 — The comment references {@link ZipGuardLimits}, but that symbol is not imported into actions.ts (only extractRunLogsZip is, at line 13), so the JSDoc link is dangling and won't resolve in editors/typedoc. Suggested fix: add ZipGuardLimits to the ./actionsCache import (type-only) or rephrase as plain text ZipGuardLimits in actionsCache.ts.
extensions/forgejo/src/actions.ts:17-39 — A 23-line dependency-decision rationale is inlined at the top of a hot-path source file; this is design documentation that better fits the issue body or a project decision doc. Suggested fix: keep a 2–3 line pointer in the source ("adm-zip chosen over yauzl; rationale in issue-112; zip-bomb guards in extractRunLogsZip") and move the full comparison into the issue/decision record.
Commit dcc1ab5 — Message is 112: Add zip-extraction dependency (adm-zip vs yauzl), but AGENTS.md requires the issue-<N>: <summary> format (e.g. issue-112: …). Suggested fix: amend the commit message to issue-112: Add zip-extraction dependency (adm-zip vs yauzl) before opening the PR (branch name already follows the convention).

Documentation

Updated files:

README.md
docs/IMPLEMENTATION_PLAN.md

## 112: Add zip-extraction dependency (adm-zip vs yauzl) Implements [112](https://git.excelera.net/david/pi-extensions-and-skills/issues/112). > 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 9m 18s. ## Review findings Now I have a full picture. Let me apply the fixes. First, the Critical finding — add `adm-zip` to the root pi-package manifest:adm-zip now resolves for the extension (nested dev copy here; on a fresh clone the root `node_modules` provides it since the nested one is gitignored). Now the README install section (High + Medium findings):Now the `actions.ts` comment — trim the inline rationale (Low 2) and make the `{@link ZipGuardLimits}` resolve (Low 1) by importing the type:Now move the full rationale into the project decision record (`docs/DESIGN.md` Tradeoffs) per Low 2's suggested fix:Now re-run the gates. Root test and the forgejo extension's check/build/test:All gates pass. Let me review the final diff:Let me do a lightweight fresh-checkout simulation (copy tree without git/node_modules, `npm install`, resolve the extension's adm-zip import):All fixes are applied and gates are green. Fresh-checkout simulation confirms the Critical fix: a tree without the gitignored nested `node_modules` resolves `adm-zip` from the root install. # Review Findings — 112 ## Summary Gates run and results: root `npm test` (node --test, 57/57 pass), forgejo extension `npm run check` (tsc --noEmit, pass), `npm run build` (tsc emit, pass), and `npm test` (jest, 241/241 pass). No lint script exists in either package.json, so no lint gate was available. The change under review is a single commit that adds a 23-line rationale comment to `extensions/forgejo/src/actions.ts`; it does **not** add or change any dependency. The adm-zip dependency and the `extractRunLogsZip`/`ZipGuardLimits` code it documents already exist from issue-111 — but only in the nested `extensions/forgejo/package.json`, which is **not** the install path pi/`install:local` uses, so the pi package's actual dependency manifest is still missing adm-zip and would fail at runtime for consumers. Remediation applied in this pass: added `"adm-zip": "^0.6.0"` to the root pi-package `dependencies` and regenerated `package-lock.json` (verified via a fresh tree copy + root `npm install` that `extensions/forgejo/src/index.ts` resolves adm-zip); updated the README install section (pi-package flow + corrected manual flat-copy recipe with `adm-zip`); trimmed the inline rationale in `actions.ts` to a pointer with resolvable `{@link}`s and moved the full adm-zip-vs-yauzl comparison into `docs/DESIGN.md` Tradeoffs. Remaining: the commit-message rewrite (Low) is out of scope for this stage, which must not commit/stage/push and only captures the working-tree diff. ## Critical - [x] `package.json:28` — The issue's core task ("Add zip-extraction dependency") is not implemented where pi actually installs deps. The root pi-package manifest declares only `dotenv`/`pg`/`pgpass`/`typebox` (lines 28–33) and the root `package-lock.json` has no adm-zip entry; there is no `workspaces` field, `scripts/local-install.mjs` runs `npm install` only at the repo root, and `extensions/forgejo/node_modules/` is gitignored. But `extensions/forgejo/src/index.ts` eagerly imports `./actions` → `./actionsCache` → `import AdmZip from "adm-zip"` (actionsCache.ts:4), so any machine that installs this package via `pi install` or `npm run install:local` will fail to load the entire forgejo extension (`Cannot find package 'adm-zip'`). The comment added at `actions.ts:18-19` ("runtime dep of extensions/forgejo") is therefore misleading for package consumers; the dev machine only works because of a manually-installed, gitignored nested node_modules. Suggested fix: add `"adm-zip": "^0.6.0"` to the root `package.json` `dependencies` and run `npm install` at the repo root to regenerate `package-lock.json` (verify with a fresh clone + `install:local` that the extension loads). ## High - [x] `extensions/forgejo/README.md:32` — The documented manual-install flow for this extension is now broken by the actions tools: the snippet runs `cd ~/.pi/agent/extensions/forgejo && npm install typebox` only, but the actions tools (documented in this same README at lines 83–132) import adm-zip at module load. A user following the documented install path gets a forgejo extension that fails to load. Suggested fix: update the snippet to `npm install typebox adm-zip` and add a line noting `forgejo_action_run_logs` requires the `adm-zip` runtime dependency (and that the `@types/adm-zip` devDep is for development only). ## Medium - [x] `extensions/forgejo/README.md:25-33` — The manual-install section copies `src/*.ts` and hand-writes the extension package.json; the new `src/actionsCache.ts` (and its adm-zip import) was added by issue-111 but this install recipe was never updated, so it is doubly stale (missing file copy and missing dep). Suggested fix: revise the recipe to copy all current sources and to include `adm-zip` in the package.json it asks the user to write, or replace the section with a pointer to the pi-package install flow. ## Low - [x] `extensions/forgejo/src/actions.ts:36` — The comment references `{@link ZipGuardLimits}`, but that symbol is not imported into `actions.ts` (only `extractRunLogsZip` is, at line 13), so the JSDoc link is dangling and won't resolve in editors/typedoc. Suggested fix: add `ZipGuardLimits` to the `./actionsCache` import (type-only) or rephrase as plain text `ZipGuardLimits` in `actionsCache.ts`. - [x] `extensions/forgejo/src/actions.ts:17-39` — A 23-line dependency-decision rationale is inlined at the top of a hot-path source file; this is design documentation that better fits the issue body or a project decision doc. Suggested fix: keep a 2–3 line pointer in the source ("adm-zip chosen over yauzl; rationale in issue-112; zip-bomb guards in extractRunLogsZip") and move the full comparison into the issue/decision record. - [ ] Commit `dcc1ab5` — Message is `112: Add zip-extraction dependency (adm-zip vs yauzl)`, but AGENTS.md requires the `issue-<N>: <summary>` format (e.g. `issue-112: …`). Suggested fix: amend the commit message to `issue-112: Add zip-extraction dependency (adm-zip vs yauzl)` before opening the PR (branch name already follows the convention). ## Documentation Updated files: - README.md - docs/IMPLEMENTATION_PLAN.md

david added 3 commits

2026-08-29 07:19:01 +00:00