issue-115: src/actions.ts — downloadJobLog (cache-first, attempt param) #127

Merged

david merged 2 commits from feature/issue-115/src-actions-ts-downloadjoblog-cache-first-attempt into main

2026-08-29 07:41:07 +00:00

david commented

2026-08-29 07:41:04 +00:00

Owner

115: src/actions.ts — downloadJobLog (cache-first, attempt param)

Implements 115.

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 22s.

Review findings

Now I understand the full picture. Let me apply the fixes: (1) import ApiError so the narrowing cast works, (2) make the statusCode assertion unconditional via a hoisted narrowed local, (3) pin the error message, (4) prefix the test title.The fix looks good. Now let me run the project gates — jest tests and TypeScript checks for the forgejo extension.All forgejo gates pass. Let me also re-run the root test gate that the review ran, for completeness.All gates are green after remediation:

forgejo jest: 252/252 pass (including the updated actions.test.ts)
forgejo npm run check (tsc --noEmit): pass
forgejo npm run build (tsc): pass
root npm test: 57/57 pass

Review Findings — 115

Summary

Stage 1's delta is a single test added to extensions/forgejo/tests/actions.test.ts covering the previously-untested fetch-rejection path of downloadJobLog (asserting the error maps to NETWORK_FAILURE and nothing is written to the cache). The downloadJobLog implementation itself (cache-first + attempt param) already exists on main, so this branch's contribution is test coverage only. Gates run: forgejo extension jest (252/252 pass, including 30/30 in actions.test.ts), tsc --noEmit (pass), npm run build/tsc (pass), root npm test (57/57 pass). No lint configuration exists in the repository (no eslint/prettier/biome configs found), so no lint gate ran.

Remediation pass: all three Low findings in actions.test.ts were fixed in place — the statusCode assertion is now unconditional (via a hoisted narrowed local), the error message is pinned with toContain("ECONNREFUSED"), and the test title is prefixed with the function name. Gates re-run and green after the edits.

Critical

No findings.

High

No findings.

Medium

No findings.

Low

extensions/forgejo/tests/actions.test.ts:612 — The statusCode assertion is wrapped in if (result.error.code === ApiErrorCode.NETWORK_FAILURE), a condition the preceding assertion on line 610 already guarantees, so the assertion is silently skippable if that guard is ever reordered or removed. (The narrowing is needed for TS because result.error is ApiError | CacheError, but the assertion should not live behind a condition that is provably always true.) Suggested fix: assert unconditionally after the code check, e.g. expect((result.error as ApiError).statusCode).toBeUndefined(); or hoist a narrowed local (const apiError = result.error as ApiError;).
extensions/forgejo/tests/actions.test.ts:610 — The test asserts the error code but not the message, so a regression in forgejoApiCall's rejection mapping (e.g. a dropped Network error: ... message) would go undetected. Suggested fix: add expect(result.error.message).toContain("ECONNREFUSED"); to pin the mapping.
extensions/forgejo/tests/actions.test.ts:596 — The identical test title "maps a fetch rejection (network failure) to NETWORK_FAILURE" is now the fourth occurrence in this file (listActionRuns, getActionRun, listActionJobs, downloadJobLog). Jest tolerates this and it matches the existing file convention, but prefixing the suite name (e.g. "downloadJobLog maps a fetch rejection ...") would make failures easier to locate. Optional polish only.

## 115: src/actions.ts — downloadJobLog (cache-first, attempt param) Implements [115](https://git.excelera.net/david/pi-extensions-and-skills/issues/115). > 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 22s. ## Review findings Now I understand the full picture. Let me apply the fixes: (1) import `ApiError` so the narrowing cast works, (2) make the statusCode assertion unconditional via a hoisted narrowed local, (3) pin the error message, (4) prefix the test title.The fix looks good. Now let me run the project gates — jest tests and TypeScript checks for the forgejo extension.All forgejo gates pass. Let me also re-run the root test gate that the review ran, for completeness.All gates are green after remediation: - forgejo `jest`: **252/252 pass** (including the updated `actions.test.ts`) - forgejo `npm run check` (`tsc --noEmit`): pass - forgejo `npm run build` (`tsc`): pass - root `npm test`: **57/57 pass** # Review Findings — 115 ## Summary Stage 1's delta is a single test added to `extensions/forgejo/tests/actions.test.ts` covering the previously-untested fetch-rejection path of `downloadJobLog` (asserting the error maps to `NETWORK_FAILURE` and nothing is written to the cache). The `downloadJobLog` implementation itself (cache-first + `attempt` param) already exists on main, so this branch's contribution is test coverage only. Gates run: forgejo extension `jest` (252/252 pass, including 30/30 in `actions.test.ts`), `tsc --noEmit` (pass), `npm run build`/`tsc` (pass), root `npm test` (57/57 pass). No lint configuration exists in the repository (no eslint/prettier/biome configs found), so no lint gate ran. Remediation pass: all three Low findings in `actions.test.ts` were fixed in place — the `statusCode` assertion is now unconditional (via a hoisted narrowed local), the error message is pinned with `toContain("ECONNREFUSED")`, and the test title is prefixed with the function name. Gates re-run and green after the edits. ## Critical - [ ] No findings. ## High - [ ] No findings. ## Medium - [ ] No findings. ## Low - [x] extensions/forgejo/tests/actions.test.ts:612 — The `statusCode` assertion is wrapped in `if (result.error.code === ApiErrorCode.NETWORK_FAILURE)`, a condition the preceding assertion on line 610 already guarantees, so the assertion is silently skippable if that guard is ever reordered or removed. (The narrowing is needed for TS because `result.error` is `ApiError | CacheError`, but the assertion should not live behind a condition that is provably always true.) Suggested fix: assert unconditionally after the code check, e.g. `expect((result.error as ApiError).statusCode).toBeUndefined();` or hoist a narrowed local (`const apiError = result.error as ApiError;`). - [x] extensions/forgejo/tests/actions.test.ts:610 — The test asserts the error code but not the message, so a regression in `forgejoApiCall`'s rejection mapping (e.g. a dropped `Network error: ...` message) would go undetected. Suggested fix: add `expect(result.error.message).toContain("ECONNREFUSED");` to pin the mapping. - [x] extensions/forgejo/tests/actions.test.ts:596 — The identical test title "maps a fetch rejection (network failure) to NETWORK_FAILURE" is now the fourth occurrence in this file (listActionRuns, getActionRun, listActionJobs, downloadJobLog). Jest tolerates this and it matches the existing file convention, but prefixing the suite name (e.g. "downloadJobLog maps a fetch rejection ...") would make failures easier to locate. Optional polish only.

david added 2 commits

2026-08-29 07:41:05 +00:00