issue-116: src/actions.ts — downloadRunLogs (ZIP, guarded unzip, cache namespace) #128

Merged

david merged 2 commits from feature/issue-116/src-actions-ts-downloadrunlogs-zip-guarded-unzip-c into main

2026-08-29 07:55:11 +00:00

david commented

2026-08-29 07:55:09 +00:00

Owner

116: src/actions.ts — downloadRunLogs (ZIP, guarded unzip, cache namespace)

Implements 116.

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

Review findings

Now let me look at the top of the test file and check the project's build configuration:Let me experiment with how to make a ZIP whose second entry's getData() throws:The corrupt-entry approach works. Let me verify it works with default guard limits (so the guard doesn't trip first):Let me check the makeRunLogsZip fixture and the rest of the test file setup:Now I have a full picture. Let me apply the fixes. First, the failWithRollback empty-directory cleanup in actionsCache.ts:Now add the corrupt-ZIP helper and the two new rollback tests in actionsCache.test.ts:Now update actions.test.ts — fold the readCachedLog assertions into the existing test (removing the duplicate), and add the cache-first extraction failure test:Now let me run the build, check, and test gates:Check passes and all 259 tests pass (up from 257). Now run the build and the root-level test suite:Build passes. Let me run the root-level test suite (mentioned in the review summary):All gates pass. Let me verify the newly-covered lines via coverage to confirm the findings are truly resolved:Let me check the current line numbers in actions.ts and inspect those uncovered ranges:The uncovered 363-395 range is the JSDoc block — comments aren't executable. Let me confirm lines 410-411 (the cached-path branch) and 424/453:Line 411 is no longer listed as uncovered. Let me verify precisely which statements in actionsCache.ts remain uncovered, using the JSON report:The JSON report looks stale (it claims everything uncovered). Let me trust the text report and map the remaining uncovered lines precisely:Let me get a clean precise JSON coverage to check whether the ZIP_EXTRACT_FAILED catch path is actually covered:The getData catch (lines 396-400) and marker write path (442-445) are now covered. Let me verify the actions.ts cached-path branch (line 411) precisely:The cached-path failure branch (410-411) is covered. Remaining uncovered lines are pre-existing branches unrelated to these findings. Let me review the full diff of my changes:The diff looks clean. Let me run all gates one final time to confirm everything is green:All gates pass. Here is the annotated findings document.

Review Findings — 116

Summary

Fixed in this pass: the cache-first extraction failure branch is now tested (new downloadRunLogs test warms the cache, swaps in a zip-bomb run.zip, and asserts the error surfaces with no partial per-job files — coverage confirms src/actions.ts:410-411 now executed); failWithRollback now removes the empty parent directories it leaves behind (walking up to and including the run dir, stopping when a dir still has content such as run.zip); and both previously untested rollback paths are covered — a corrupt-entry getData() failure (new corruptZipEntry test helper + test asserting ZIP_EXTRACT_FAILED and rollback of earlier files) and marker/.MISSING rollback under extracted/ (new test asserting the marker and emptied extracted/ + run dirs are gone). The duplicate "warms the per-job cache" test was folded into the existing download test per the Low finding. Remaining: the commit-message format finding (commit 5de40b8) — the commit is already made and this pipeline must not commit/amend, so the issue- prefix fix applies only to future commits. Gates: npm run check PASS, npm run build PASS, npm test (11 suites / 259 tests) PASS, root npm test (57 tests) PASS; no lint gate exists.

Critical

(none)

High

extensions/forgejo/tests/actions.test.ts:739 — The cache-first extraction failure branch is untested: downloadRunLogs passes limits on the cached-ZIP path (src/actions.ts:410) and rolls back partial files there too, but no test exercises a cached run.zip whose extraction fails (coverage confirms src/actions.ts:411 is uncovered). Suggested fix: warm the cache with a valid ZIP, then overwrite run.zip in the cache with a zip-bomb/corrupt archive, call downloadRunLogs without refresh, and assert the error surfaces and no partial per-job files (e.g. 42-attempt-1.log, 42.log) remain.

Medium

extensions/forgejo/src/actionsCache.ts:346-355 — Rollback removes written files but leaves empty parent directories (<run_dir>/ and <run_dir>/extracted/) behind, so a rejected archive still leaves cache scaffolding (tests assert only file absence). Suggested fix: after removing files, best-effort fs.rmdirSync the affected parents (or have downloadRunLogs remove the whole run dir on the fresh-download path).
extensions/forgejo/src/actionsCache.ts:381-385 — The ZIP_EXTRACT_FAILED rollback path is untested (coverage confirms lines 381-382 uncovered): a mid-archive entry.getData() failure is routed through failWithRollback but no test asserts earlier files are removed in that case. Suggested fix: construct a ZIP with one valid entry followed by a corrupt entry whose getData() throws, and assert entry 1's files were rolled back.
extensions/forgejo/src/actionsCache.ts:417-428 — Rollback of marker/non-layout entries written under <run_dir>/extracted/ is untested; all new rollback tests only cover per-job-namespace files. Suggested fix: add a test with a valid .MISSING marker followed by a guard-tripping entry, asserting the marker file is removed from extracted/.

Low

extensions/forgejo/tests/actions.test.ts:709 — The new "warms the per-job cache" test largely duplicates the existing test at line 677 ("downloads the ZIP, writes run.zip, and extracts per-job logs"); only the readCachedLog assertions are new. Suggested fix: fold the readCachedLog assertions into the existing test to reduce duplication.
Commit 5de40b8 message 116: … — AGENTS.md specifies commit format issue-<N>: <summary>; the issue- prefix is missing. This matches prior repo history (114/115 use the same shorthand), so it is a standards deviation, not a regression. Suggested fix: prefix future commit messages with issue-.

## 116: src/actions.ts — downloadRunLogs (ZIP, guarded unzip, cache namespace) Implements [116](https://git.excelera.net/david/pi-extensions-and-skills/issues/116). > 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 14m 00s. ## Review findings Now let me look at the top of the test file and check the project's build configuration:Let me experiment with how to make a ZIP whose second entry's `getData()` throws:The corrupt-entry approach works. Let me verify it works with default guard limits (so the guard doesn't trip first):Let me check the `makeRunLogsZip` fixture and the rest of the test file setup:Now I have a full picture. Let me apply the fixes. First, the `failWithRollback` empty-directory cleanup in `actionsCache.ts`:Now add the corrupt-ZIP helper and the two new rollback tests in `actionsCache.test.ts`:Now update `actions.test.ts` — fold the `readCachedLog` assertions into the existing test (removing the duplicate), and add the cache-first extraction failure test:Now let me run the build, check, and test gates:Check passes and all 259 tests pass (up from 257). Now run the build and the root-level test suite:Build passes. Let me run the root-level test suite (mentioned in the review summary):All gates pass. Let me verify the newly-covered lines via coverage to confirm the findings are truly resolved:Let me check the current line numbers in `actions.ts` and inspect those uncovered ranges:The uncovered `363-395` range is the JSDoc block — comments aren't executable. Let me confirm lines 410-411 (the cached-path branch) and 424/453:Line 411 is no longer listed as uncovered. Let me verify precisely which statements in `actionsCache.ts` remain uncovered, using the JSON report:The JSON report looks stale (it claims everything uncovered). Let me trust the text report and map the remaining uncovered lines precisely:Let me get a clean precise JSON coverage to check whether the `ZIP_EXTRACT_FAILED` catch path is actually covered:The getData catch (lines 396-400) and marker write path (442-445) are now covered. Let me verify the `actions.ts` cached-path branch (line 411) precisely:The cached-path failure branch (410-411) is covered. Remaining uncovered lines are pre-existing branches unrelated to these findings. Let me review the full diff of my changes:The diff looks clean. Let me run all gates one final time to confirm everything is green:All gates pass. Here is the annotated findings document. # Review Findings — 116 ## Summary Fixed in this pass: the cache-first extraction failure branch is now tested (new `downloadRunLogs` test warms the cache, swaps in a zip-bomb `run.zip`, and asserts the error surfaces with no partial per-job files — coverage confirms src/actions.ts:410-411 now executed); `failWithRollback` now removes the empty parent directories it leaves behind (walking up to and including the run dir, stopping when a dir still has content such as `run.zip`); and both previously untested rollback paths are covered — a corrupt-entry `getData()` failure (new `corruptZipEntry` test helper + test asserting `ZIP_EXTRACT_FAILED` and rollback of earlier files) and marker/`.MISSING` rollback under `extracted/` (new test asserting the marker and emptied `extracted/` + run dirs are gone). The duplicate "warms the per-job cache" test was folded into the existing download test per the Low finding. Remaining: the commit-message format finding (commit 5de40b8) — the commit is already made and this pipeline must not commit/amend, so the `issue-` prefix fix applies only to future commits. Gates: `npm run check` **PASS**, `npm run build` **PASS**, `npm test` (11 suites / 259 tests) **PASS**, root `npm test` (57 tests) **PASS**; no lint gate exists. ## Critical (none) ## High - [x] extensions/forgejo/tests/actions.test.ts:739 — The cache-first extraction failure branch is untested: `downloadRunLogs` passes `limits` on the cached-ZIP path (src/actions.ts:410) and rolls back partial files there too, but no test exercises a cached `run.zip` whose extraction fails (coverage confirms src/actions.ts:411 is uncovered). Suggested fix: warm the cache with a valid ZIP, then overwrite `run.zip` in the cache with a zip-bomb/corrupt archive, call `downloadRunLogs` without `refresh`, and assert the error surfaces and no partial per-job files (e.g. `42-attempt-1.log`, `42.log`) remain. ## Medium - [x] extensions/forgejo/src/actionsCache.ts:346-355 — Rollback removes written files but leaves empty parent directories (`<run_dir>/` and `<run_dir>/extracted/`) behind, so a rejected archive still leaves cache scaffolding (tests assert only file absence). Suggested fix: after removing files, best-effort `fs.rmdirSync` the affected parents (or have `downloadRunLogs` remove the whole run dir on the fresh-download path). - [x] extensions/forgejo/src/actionsCache.ts:381-385 — The `ZIP_EXTRACT_FAILED` rollback path is untested (coverage confirms lines 381-382 uncovered): a mid-archive `entry.getData()` failure is routed through `failWithRollback` but no test asserts earlier files are removed in that case. Suggested fix: construct a ZIP with one valid entry followed by a corrupt entry whose `getData()` throws, and assert entry 1's files were rolled back. - [x] extensions/forgejo/src/actionsCache.ts:417-428 — Rollback of marker/non-layout entries written under `<run_dir>/extracted/` is untested; all new rollback tests only cover per-job-namespace files. Suggested fix: add a test with a valid `.MISSING` marker followed by a guard-tripping entry, asserting the marker file is removed from `extracted/`. ## Low - [x] extensions/forgejo/tests/actions.test.ts:709 — The new "warms the per-job cache" test largely duplicates the existing test at line 677 ("downloads the ZIP, writes run.zip, and extracts per-job logs"); only the `readCachedLog` assertions are new. Suggested fix: fold the `readCachedLog` assertions into the existing test to reduce duplication. - [ ] Commit 5de40b8 message `116: …` — AGENTS.md specifies commit format `issue-<N>: <summary>`; the `issue-` prefix is missing. This matches prior repo history (114/115 use the same shorthand), so it is a standards deviation, not a regression. Suggested fix: prefix future commit messages with `issue-`.

david added 2 commits

2026-08-29 07:55:09 +00:00