issue-113: src/actionsCache.ts — cache paths + cache-first read/write #125

Merged

david merged 2 commits from feature/issue-113/src-actionscache-ts-cache-paths-cache-first-read-w into main

2026-08-29 07:26:59 +00:00

david commented

2026-08-29 07:26:57 +00:00

Owner

113: src/actionsCache.ts — cache paths + cache-first read/write

Implements 113.

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 7m 53s.

Review findings

Now let me look at the callers and tests to understand the full picture:The shell interpreted the pipe. Let me quote the pattern:Now let me read the test file sections and check the project's build configuration:Let me check AGENTS.md line 26 and whether the commit message is recorded anywhere, plus look at git log for the Stage 1 commit:The commit message finding anchors to AGENTS.md's convention documentation; the Stage 1 commit 970391c uses 113: … instead of issue-113: …. I cannot rewrite commits in this stage (no git operations allowed), so that finding stays unresolved.

Now let me implement the code fixes. First, src/actionsCache.ts:Now update the callers in src/actions.ts:Now update the tests in tests/actionsCache.test.ts:Let me verify no other callers use the old signatures, then run the gates:All callers updated. Now run the project gates (build, check, test):All gates pass (build, check, 244 tests). Let me review the final diff of my remediation changes:The diff is clean and all tests pass. Let me double-check there's no test asserting the old swallow-error behavior in downloadJobLog:All gates pass and the changes are consistent. Here is the final annotated findings document.

Review Findings — 113

Summary

Reviewed the Stage 1 changes for issue 113 (cache paths + cache-first read/write in extensions/forgejo/src/actionsCache.ts and its callers/tests). Gates run from extensions/forgejo: npm run build (tsc) PASS, npm run check (tsc --noEmit) PASS, npm test (jest, 11 suites / 244 tests) PASS; lint N/A — the repo has no lint tooling (no eslint/prettier configs or lint scripts in the root or extensions/forgejo package). The refactor (options-object path helpers, injectable env for cacheRoot, readCachedLogIfPresent → readCachedLog) is internally consistent, all callers were updated, no stale references to removed symbols remain, and the new tests cover the changed behavior. Findings below are diagnostic-quality and cleanup items; none block merge.

Remediation pass (single pass, gates re-run green): the Medium finding was fixed by restoring a Result<string | null, CacheError> return for readCachedLog that distinguishes ENOENT (cache miss) from other read failures (surfaced as CacheError READ_FAILED), which also resolves the dead-READ_FAILED Low finding. The runDir Low finding was fixed by converting it to an options object (RunDirOptions) consistent with the other path helpers. The commit-message Low finding (AGENTS.md:26) remains unresolved — this stage is prohibited from git operations, so the Stage 1 commit message cannot be rewritten here; the squash PR title/body must carry issue-113: … + Closes #113.

Critical

(none)

High

(none)

Medium

extensions/forgejo/src/actionsCache.ts:188 — readCachedLog now swallows all read errors (catch { return null; }), not just ENOENT. A cached-but-unreadable log (e.g. EACCES on the cache file, or ENOTDIR) is silently treated as a cache miss, so downloadJobLog re-downloads from the network even though a local copy exists; if the network is unavailable the user now gets ApiError NETWORK_FAILURE instead of the old, accurate CacheError READ_FAILED ("Failed to read cached log …"). The previous Result-returning read preserved this diagnostic, and the new behavior can also cause a redundant download on every call when the cache dir permissions are wrong. Suggested fix: keep the string | null shape for the happy path but distinguish ENOENT (return null = miss) from other errors (return/rethrow a CacheError with CacheErrorCode.READ_FAILED so the caller surfaces it), or at minimum log the underlying error before falling back to re-download.

Low

extensions/forgejo/src/errors.ts:74 — CacheErrorCode.READ_FAILED is now dead code: its only consumer (readCachedLogIfPresent) was removed in this change, and a repo-wide grep shows no remaining usage in src/. Suggested fix: remove the enum member if the swallow-all-read-errors design is kept, or restore its use per the Medium finding above.
AGENTS.md:26 — the Stage 1 commit message is 113: src/actionsCache.ts — cache paths + cache-first read/write, which does not match the repo convention issue-<N>: <summary> (e.g. issue-42: add user auth; the issue- prefix is missing). The branch name (feature/issue-113/…) follows the convention correctly. Suggested fix: rewrite the commit (or the eventual squash PR title/body) to issue-113: … and reference Closes #113 so downstream artifacts stay issue-number-driven.
extensions/forgejo/src/actionsCache.ts:97 — runDir still takes positional args (root, owner, repo, runId) while its two path helpers cachedLogPath and runZipPath were converted to options objects in this same change (and the new JSDoc calls them "Inputs"). The mixed style is mildly inconsistent for readers of the module. Suggested fix: either convert runDir to an options object too (e.g. RunDirOptions) or note in its JSDoc why it intentionally remains positional; the runDir(root, owner, repo, runId) call at actions.ts:392 would need the same update.

## 113: src/actionsCache.ts — cache paths + cache-first read/write Implements [113](https://git.excelera.net/david/pi-extensions-and-skills/issues/113). > 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 7m 53s. ## Review findings Now let me look at the callers and tests to understand the full picture:The shell interpreted the pipe. Let me quote the pattern:Now let me read the test file sections and check the project's build configuration:Let me check AGENTS.md line 26 and whether the commit message is recorded anywhere, plus look at git log for the Stage 1 commit:The commit message finding anchors to AGENTS.md's convention documentation; the Stage 1 commit `970391c` uses `113: …` instead of `issue-113: …`. I cannot rewrite commits in this stage (no git operations allowed), so that finding stays unresolved. Now let me implement the code fixes. First, `src/actionsCache.ts`:Now update the callers in `src/actions.ts`:Now update the tests in `tests/actionsCache.test.ts`:Let me verify no other callers use the old signatures, then run the gates:All callers updated. Now run the project gates (build, check, test):All gates pass (build, check, 244 tests). Let me review the final diff of my remediation changes:The diff is clean and all tests pass. Let me double-check there's no test asserting the old swallow-error behavior in `downloadJobLog`:All gates pass and the changes are consistent. Here is the final annotated findings document. # Review Findings — 113 ## Summary Reviewed the Stage 1 changes for issue 113 (cache paths + cache-first read/write in `extensions/forgejo/src/actionsCache.ts` and its callers/tests). Gates run from `extensions/forgejo`: `npm run build` (tsc) **PASS**, `npm run check` (tsc --noEmit) **PASS**, `npm test` (jest, 11 suites / 244 tests) **PASS**; lint **N/A** — the repo has no lint tooling (no eslint/prettier configs or lint scripts in the root or `extensions/forgejo` package). The refactor (options-object path helpers, injectable env for `cacheRoot`, `readCachedLogIfPresent` → `readCachedLog`) is internally consistent, all callers were updated, no stale references to removed symbols remain, and the new tests cover the changed behavior. Findings below are diagnostic-quality and cleanup items; none block merge. Remediation pass (single pass, gates re-run green): the Medium finding was fixed by restoring a `Result<string | null, CacheError>` return for `readCachedLog` that distinguishes `ENOENT` (cache miss) from other read failures (surfaced as `CacheError READ_FAILED`), which also resolves the dead-`READ_FAILED` Low finding. The `runDir` Low finding was fixed by converting it to an options object (`RunDirOptions`) consistent with the other path helpers. The commit-message Low finding (AGENTS.md:26) remains unresolved — this stage is prohibited from git operations, so the Stage 1 commit message cannot be rewritten here; the squash PR title/body must carry `issue-113: …` + `Closes #113`. ## Critical - (none) ## High - (none) ## Medium - [x] `extensions/forgejo/src/actionsCache.ts:188` — `readCachedLog` now swallows **all** read errors (`catch { return null; }`), not just `ENOENT`. A cached-but-unreadable log (e.g. `EACCES` on the cache file, or `ENOTDIR`) is silently treated as a cache miss, so `downloadJobLog` re-downloads from the network even though a local copy exists; if the network is unavailable the user now gets `ApiError NETWORK_FAILURE` instead of the old, accurate `CacheError READ_FAILED` ("Failed to read cached log …"). The previous `Result`-returning read preserved this diagnostic, and the new behavior can also cause a redundant download on every call when the cache dir permissions are wrong. Suggested fix: keep the `string | null` shape for the happy path but distinguish `ENOENT` (return `null` = miss) from other errors (return/rethrow a `CacheError` with `CacheErrorCode.READ_FAILED` so the caller surfaces it), or at minimum log the underlying error before falling back to re-download. ## Low - [x] `extensions/forgejo/src/errors.ts:74` — `CacheErrorCode.READ_FAILED` is now dead code: its only consumer (`readCachedLogIfPresent`) was removed in this change, and a repo-wide grep shows no remaining usage in `src/`. Suggested fix: remove the enum member if the swallow-all-read-errors design is kept, or restore its use per the Medium finding above. - [ ] `AGENTS.md:26` — the Stage 1 commit message is `113: src/actionsCache.ts — cache paths + cache-first read/write`, which does not match the repo convention `issue-<N>: <summary>` (e.g. `issue-42: add user auth`; the `issue-` prefix is missing). The branch name (`feature/issue-113/…`) follows the convention correctly. Suggested fix: rewrite the commit (or the eventual squash PR title/body) to `issue-113: …` and reference `Closes #113` so downstream artifacts stay issue-number-driven. - [x] `extensions/forgejo/src/actionsCache.ts:97` — `runDir` still takes positional args (`root, owner, repo, runId`) while its two path helpers `cachedLogPath` and `runZipPath` were converted to options objects in this same change (and the new JSDoc calls them "Inputs"). The mixed style is mildly inconsistent for readers of the module. Suggested fix: either convert `runDir` to an options object too (e.g. `RunDirOptions`) or note in its JSDoc why it intentionally remains positional; the `runDir(root, owner, repo, runId)` call at `actions.ts:392` would need the same update.

david added 2 commits

2026-08-29 07:26:58 +00:00