267: Network-level fetch failures produce opaque "fetch failed" errors and skip bounded retries #272

david · 2026-08-18T07:05:06Z

david commented

2026-08-18 07:05:06 +00:00

267: Network-level fetch failures produce opaque "fetch failed" errors and skip bounded retries

Implements 267.

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

Review findings

Now let's add the missing tests for High finding, then fix Medium and Low findings.Now the Medium finding: DOMAIN.md update.Now the Medium finding for jiraError.ts duplication:Now let's check the diff for jiraError.ts type file — no change needed since already formatted. Let's run the full gates.Good — the diff-touched files (classifyFetchError.ts, classifyHttpStatus.ts, index.ts, jiraError.ts) are no longer flagged. DOMAIN.md is flagged but that's pre-existing (it was flagged before my edit too, likely). Let's check.DOMAIN.md was already in the pre-existing "unformatted" list (a known table-column alignment issue unrelated to this diff, matches original review note about pre-existing files not touched by diff). Good, not something I need to fix formatting-wise since it's a table alignment issue outside this diff's scope. Now run full test/build/lint.All 142 test files pass (2003 tests now, up from 2001 due to the 2 new tests). Build and lint clean. Let's confirm git diff status and double check jiraError.ts file content.All formatting fixes applied correctly, and consistent with the pattern used in issueSourceError.ts. Let's do a final full test/build/lint run to be safe.All gates pass. Here's the final annotated findings document.

Review Findings — 267

Summary

Fixed the High finding (added dedicated regression tests for the auto-merge-timeout/auto-merge-failed VALID_KINDS fix) and both Medium findings (added network/auto-merge-timeout/auto-merge-failed to DOMAIN.md's error.json kind glossary row to match DESIGN.md/IMPLEMENTATION_PLAN.md/README.md; consolidated jiraError.ts's classifyHttpStatus into a thin wrapper delegating to the shared classifyHttpStatus, matching the pattern already used by issueSourceError.ts/mrClientError.ts). Fixed the Low finding by running Prettier on all files flagged by npm run format:check that were touched by this diff (classifyFetchError.ts/.test.ts, classifyHttpStatus.ts/.test.ts, shared/http/index.ts, jira/types/jiraError.ts); the pre-existing DOMAIN.md formatting issue (table column alignment, unrelated to this diff's touched lines) remains and was left as-is since fixing it is out of scope of this diff's own formatting. npm run build, npm run lint, and npm test (142 files / 2003 tests) all pass after the changes.

Critical

High

src/artifacts/helpers/validateErrorReport.ts:33-34 — auto-merge-timeout and auto-merge-failed were added to VALID_KINDS (previously missing despite being valid ErrorKind values, so writeErrorReport/readErrorReport would throw ArtifactValidationError for real auto-merge failures) but no test in validateErrorReport.test.ts exercises either kind. Suggested fix: add it('accepts the auto-merge-timeout kind', …) and it('accepts the auto-merge-failed kind', …) cases mirroring the new network test, to lock in this bug fix and prevent regression.

Medium

DOMAIN.md:124 — The error.json kind glossary row still lists only config | auth | not-found | rate-limit | transient | agent-failure | git | unknown, missing network (and auto-merge-timeout/auto-merge-failed, which predate this diff) even though DESIGN.md:324, IMPLEMENTATION_PLAN.md:183, and README.md:454 were all updated to include network in the same change. Suggested fix: update the DOMAIN.md row to match the other three docs (config \| auth \| not-found \| rate-limit \| transient \| network \| agent-failure \| git \| auto-merge-timeout \| auto-merge-failed \| unknown).
src/jira/helpers/jiraError.ts:26-44 — classifyHttpStatus here is now byte-for-byte identical to src/shared/http/classifyHttpStatus.ts's classifyHttpStatus (both were updated in lockstep to add the network/NETWORK_ERROR_STATUS branch), unlike issueSourceError.ts's classifyIssueHttpStatus and mrClientError.ts's classifyMrHttpStatus, which are thin wrappers delegating to the shared implementation. This duplication predates the diff but the diff was a natural point to consolidate it, and instead doubled the duplicated surface. Suggested fix: replace the Jira module's local classifyHttpStatus with a thin wrapper over shared/http's classifyHttpStatus (returning JiraErrorKind), the same pattern already used by the issue and MR domains.

Low

src/shared/http/classifyFetchError.ts:65-85, src/shared/http/classifyHttpStatus.ts, src/shared/http/index.ts, src/jira/types/jiraError.ts, src/shared/http/classifyFetchError.test.ts, src/shared/http/classifyHttpStatus.test.ts — npm run format:check reports Prettier violations in these files (all touched by this diff); npm run lint doesn't catch them since eslint-config-prettier only disables conflicting rules rather than enforcing formatting. Suggested fix: run npm run format (or npx prettier --write <files>) before merging so the new/changed files match the project's Prettier defaults per CODING_STANDARDS.md §5.

## 267: Network-level fetch failures produce opaque "fetch failed" errors and skip bounded retries Implements [267](https://git.excelera.net/david/pi-loop/issues/267). > 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 30m 34s. ## Review findings Now let's add the missing tests for High finding, then fix Medium and Low findings.Now the Medium finding: DOMAIN.md update.Now the Medium finding for jiraError.ts duplication:Now let's check the diff for jiraError.ts type file — no change needed since already formatted. Let's run the full gates.Good — the diff-touched files (classifyFetchError.ts, classifyHttpStatus.ts, index.ts, jiraError.ts) are no longer flagged. DOMAIN.md is flagged but that's pre-existing (it was flagged before my edit too, likely). Let's check.DOMAIN.md was already in the pre-existing "unformatted" list (a known table-column alignment issue unrelated to this diff, matches original review note about pre-existing files not touched by diff). Good, not something I need to fix formatting-wise since it's a table alignment issue outside this diff's scope. Now run full test/build/lint.All 142 test files pass (2003 tests now, up from 2001 due to the 2 new tests). Build and lint clean. Let's confirm git diff status and double check jiraError.ts file content.All formatting fixes applied correctly, and consistent with the pattern used in `issueSourceError.ts`. Let's do a final full test/build/lint run to be safe.All gates pass. Here's the final annotated findings document. # Review Findings — 267 ## Summary Fixed the High finding (added dedicated regression tests for the `auto-merge-timeout`/`auto-merge-failed` `VALID_KINDS` fix) and both Medium findings (added `network`/`auto-merge-timeout`/`auto-merge-failed` to DOMAIN.md's `error.json kind` glossary row to match DESIGN.md/IMPLEMENTATION_PLAN.md/README.md; consolidated `jiraError.ts`'s `classifyHttpStatus` into a thin wrapper delegating to the shared `classifyHttpStatus`, matching the pattern already used by `issueSourceError.ts`/`mrClientError.ts`). Fixed the Low finding by running Prettier on all files flagged by `npm run format:check` that were touched by this diff (`classifyFetchError.ts`/`.test.ts`, `classifyHttpStatus.ts`/`.test.ts`, `shared/http/index.ts`, `jira/types/jiraError.ts`); the pre-existing DOMAIN.md formatting issue (table column alignment, unrelated to this diff's touched lines) remains and was left as-is since fixing it is out of scope of this diff's own formatting. `npm run build`, `npm run lint`, and `npm test` (142 files / 2003 tests) all pass after the changes. ## Critical ## High - [x] src/artifacts/helpers/validateErrorReport.ts:33-34 — `auto-merge-timeout` and `auto-merge-failed` were added to `VALID_KINDS` (previously missing despite being valid `ErrorKind` values, so `writeErrorReport`/`readErrorReport` would throw `ArtifactValidationError` for real auto-merge failures) but no test in `validateErrorReport.test.ts` exercises either kind. Suggested fix: add `it('accepts the auto-merge-timeout kind', …)` and `it('accepts the auto-merge-failed kind', …)` cases mirroring the new `network` test, to lock in this bug fix and prevent regression. ## Medium - [x] DOMAIN.md:124 — The `error.json` `kind` glossary row still lists only `config | auth | not-found | rate-limit | transient | agent-failure | git | unknown`, missing `network` (and `auto-merge-timeout`/`auto-merge-failed`, which predate this diff) even though DESIGN.md:324, IMPLEMENTATION_PLAN.md:183, and README.md:454 were all updated to include `network` in the same change. Suggested fix: update the DOMAIN.md row to match the other three docs (`config \| auth \| not-found \| rate-limit \| transient \| network \| agent-failure \| git \| auto-merge-timeout \| auto-merge-failed \| unknown`). - [x] src/jira/helpers/jiraError.ts:26-44 — `classifyHttpStatus` here is now byte-for-byte identical to `src/shared/http/classifyHttpStatus.ts`'s `classifyHttpStatus` (both were updated in lockstep to add the `network`/`NETWORK_ERROR_STATUS` branch), unlike `issueSourceError.ts`'s `classifyIssueHttpStatus` and `mrClientError.ts`'s `classifyMrHttpStatus`, which are thin wrappers delegating to the shared implementation. This duplication predates the diff but the diff was a natural point to consolidate it, and instead doubled the duplicated surface. Suggested fix: replace the Jira module's local `classifyHttpStatus` with a thin wrapper over `shared/http`'s `classifyHttpStatus` (returning `JiraErrorKind`), the same pattern already used by the issue and MR domains. ## Low - [x] src/shared/http/classifyFetchError.ts:65-85, src/shared/http/classifyHttpStatus.ts, src/shared/http/index.ts, src/jira/types/jiraError.ts, src/shared/http/classifyFetchError.test.ts, src/shared/http/classifyHttpStatus.test.ts — `npm run format:check` reports Prettier violations in these files (all touched by this diff); `npm run lint` doesn't catch them since `eslint-config-prettier` only disables conflicting rules rather than enforcing formatting. Suggested fix: run `npm run format` (or `npx prettier --write <files>`) before merging so the new/changed files match the project's Prettier defaults per CODING_STANDARDS.md §5.

david added 2 commits

2026-08-18 07:05:06 +00:00

267: Network-level fetch failures produce opaque "fetch failed" errors and skip bounded retries bfb1273e2c