29: Bug: forgejo_issue_create/view/edit/close/etc. crash when assignees is null in the API response #32

Merged

david merged 2 commits from feature/issue-29/bug-forgejo-issue-create-view-edit-close-etc-crash into main

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

david commented

2026-08-18 07:29:56 +00:00

Owner

29: Bug: forgejo_issue_create/view/edit/close/etc. crash when assignees is null in the API response

Implements 29.

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

Review findings

The working tree is clean at commit 19f83fd (the Stage 1 commit). Let me examine the files involved in the Medium finding.Now let me look at the exact formatPullRequest section in index.ts and the test file structure.I have full context. Now let me apply the Medium finding fixes. First the formatPullRequest normalization in index.ts and the interface change in pulls.ts:Now let me look at the end of the test file to add the null-head regression test:Now I'll add the null-head regression test:Let me check for any other dereferences of base.ref/head.ref in the codebase, and verify the edits:The grep failed due to shell quoting. Let me retry with proper quoting:The remaining references are in tests with present-data fixtures (fine — a non-null object is assignable to | null). Let me check pulls.test.ts around those lines to confirm nothing breaks:Now let me discover the build/test/lint commands for the forgejo extension:Found the gates: npm run build (tsc), npm run check (tsc --noEmit), npm test (jest) in extensions/forgejo. Let me run them:The updated | null interface surfaced two typecheck errors in pulls.test.ts where the present-data assertions dereference base.ref/head.ref. Minimal fix: use optional chaining (the fixture provides real objects, so the assertions still verify present data):All 8 suites pass (117 tests — 116 + the 1 new null-head test). Let me re-run build/check and review the final working-tree diff:Let me review the final diff to confirm correctness:All gates pass and the diff is minimal and consistent. The Medium finding is fully resolved (normalization in formatPullRequest, | null in the interface, new null-head regression test, plus two typecheck errors in pulls.test.ts surfaced by the interface widening that I fixed with optional chaining). The Low finding (commit message amend) requires a git history rewrite, which this stage forbids, so it remains unresolved.

Review Findings — 29

Summary

The reported crash (null assignees/labels from the Forgejo API) is fixed: formatIssue/formatPullRequest now normalize both fields with ?? [], the ForgejoIssue/ForgejoPullRequest interfaces are updated to reflect the nullable response shape, and 9 regression tests cover the null cases plus the present-data path. Gates run: npm run build (tsc) — PASS; npm run check (tsc --noEmit) — PASS; npm test (jest) — PASS (8 suites, 116 tests, incl. 9 new regression tests). No lint gate exists in this repo (no lint script in any package.json, no eslint/prettier/biome config), so lint was not run. No Critical or High findings; one adjacent null-crash risk and one workflow-convention nit below.

Remediation pass (single pass): the Medium finding is resolved — formatPullRequest now normalizes base/head with ?? null and renders base?.ref ?? "" / head?.ref ?? "" in both the display text and details; ForgejoPullRequest.base/head are typed | null; a null-head regression test was added; the interface widening surfaced two TS18047 errors in tests/pulls.test.ts (present-data assertions on base.ref/head.ref) which were fixed with optional chaining. Re-ran gates after the fix: npm run build — PASS; npm run check — PASS; npm test — PASS (8 suites, 117 tests, incl. the new null-head test). The Low finding is left unresolved: the suggested fix is to amend commit 19f83fd's message, but this stage forbids commit operations (including git commit --amend) and the pipeline captures only the working-tree diff, so a message rewrite is not actionable here.

Critical

(none)

High

(none)

Medium

extensions/forgejo/src/index.ts:174 — formatPullRequest still unconditionally dereferences pr.base.ref and pr.head.ref (also in details at lines 199–200). Gitea/Forgejo can return base: null / head: null for PRs whose head/base branch or fork was deleted — the same nullability class as this bug — so forgejo_pr_view/forgejo_pr_edit/forgejo_pr_close/forgejo_pr_assign would crash with "Cannot read properties of null (reading 'ref')". Suggested fix: normalize const base = pr.base ?? null; const head = pr.head ?? null;, render base?.ref ?? "" / head?.ref ?? "" at lines 174/199/200, and change ForgejoPullRequest.base/head (extensions/forgejo/src/pulls.ts:14-15) to | null; add a null-head regression test mirroring the new assignees: null tests.

Low

AGENTS.md:30 (commit 19f83fd) — commit message violates the documented issue-<N>: <summary> format; the subject is 29: Bug: ... (missing the issue- prefix). Suggested fix: amend the commit message to issue-29: Bug: forgejo_issue_create/view/edit/close/etc. crash when assignees is null in the API response (branch name feature/issue-29/... is already correct).

## 29: Bug: forgejo_issue_create/view/edit/close/etc. crash when assignees is null in the API response Implements [29](https://git.excelera.net/david/pi-extensions-and-skills/issues/29). > 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 02s. ## Review findings The working tree is clean at commit `19f83fd` (the Stage 1 commit). Let me examine the files involved in the Medium finding.Now let me look at the exact `formatPullRequest` section in `index.ts` and the test file structure.I have full context. Now let me apply the Medium finding fixes. First the `formatPullRequest` normalization in `index.ts` and the interface change in `pulls.ts`:Now let me look at the end of the test file to add the null-`head` regression test:Now I'll add the null-`head` regression test:Let me check for any other dereferences of `base.ref`/`head.ref` in the codebase, and verify the edits:The grep failed due to shell quoting. Let me retry with proper quoting:The remaining references are in tests with present-data fixtures (fine — a non-null object is assignable to `| null`). Let me check `pulls.test.ts` around those lines to confirm nothing breaks:Now let me discover the build/test/lint commands for the forgejo extension:Found the gates: `npm run build` (tsc), `npm run check` (tsc --noEmit), `npm test` (jest) in `extensions/forgejo`. Let me run them:The updated `| null` interface surfaced two typecheck errors in `pulls.test.ts` where the present-data assertions dereference `base.ref`/`head.ref`. Minimal fix: use optional chaining (the fixture provides real objects, so the assertions still verify present data):All 8 suites pass (117 tests — 116 + the 1 new null-head test). Let me re-run build/check and review the final working-tree diff:Let me review the final diff to confirm correctness:All gates pass and the diff is minimal and consistent. The Medium finding is fully resolved (normalization in `formatPullRequest`, `| null` in the interface, new null-`head` regression test, plus two typecheck errors in `pulls.test.ts` surfaced by the interface widening that I fixed with optional chaining). The Low finding (commit message amend) requires a git history rewrite, which this stage forbids, so it remains unresolved. # Review Findings — 29 ## Summary The reported crash (null `assignees`/`labels` from the Forgejo API) is fixed: `formatIssue`/`formatPullRequest` now normalize both fields with `?? []`, the `ForgejoIssue`/`ForgejoPullRequest` interfaces are updated to reflect the nullable response shape, and 9 regression tests cover the null cases plus the present-data path. Gates run: `npm run build` (tsc) — PASS; `npm run check` (tsc --noEmit) — PASS; `npm test` (jest) — PASS (8 suites, 116 tests, incl. 9 new regression tests). No lint gate exists in this repo (no lint script in any `package.json`, no eslint/prettier/biome config), so lint was not run. No Critical or High findings; one adjacent null-crash risk and one workflow-convention nit below. **Remediation pass (single pass):** the Medium finding is resolved — `formatPullRequest` now normalizes `base`/`head` with `?? null` and renders `base?.ref ?? ""` / `head?.ref ?? ""` in both the display text and `details`; `ForgejoPullRequest.base`/`head` are typed `| null`; a null-`head` regression test was added; the interface widening surfaced two `TS18047` errors in `tests/pulls.test.ts` (present-data assertions on `base.ref`/`head.ref`) which were fixed with optional chaining. Re-ran gates after the fix: `npm run build` — PASS; `npm run check` — PASS; `npm test` — PASS (8 suites, 117 tests, incl. the new null-`head` test). The Low finding is left unresolved: the suggested fix is to amend commit `19f83fd`'s message, but this stage forbids commit operations (including `git commit --amend`) and the pipeline captures only the working-tree diff, so a message rewrite is not actionable here. ## Critical - (none) ## High - (none) ## Medium - [x] `extensions/forgejo/src/index.ts:174` — `formatPullRequest` still unconditionally dereferences `pr.base.ref` and `pr.head.ref` (also in `details` at lines 199–200). Gitea/Forgejo can return `base: null` / `head: null` for PRs whose head/base branch or fork was deleted — the same nullability class as this bug — so `forgejo_pr_view`/`forgejo_pr_edit`/`forgejo_pr_close`/`forgejo_pr_assign` would crash with "Cannot read properties of null (reading 'ref')". Suggested fix: normalize `const base = pr.base ?? null; const head = pr.head ?? null;`, render `base?.ref ?? ""` / `head?.ref ?? ""` at lines 174/199/200, and change `ForgejoPullRequest.base`/`head` (`extensions/forgejo/src/pulls.ts:14-15`) to `| null`; add a null-`head` regression test mirroring the new `assignees: null` tests. ## Low - [ ] `AGENTS.md:30` (commit `19f83fd`) — commit message violates the documented `issue-<N>: <summary>` format; the subject is `29: Bug: ...` (missing the `issue-` prefix). Suggested fix: amend the commit message to `issue-29: Bug: forgejo_issue_create/view/edit/close/etc. crash when assignees is null in the API response` (branch name `feature/issue-29/...` is already correct).

david added 2 commits

2026-08-18 07:29:56 +00:00