Guard PR-comment/lease listing against non-list API payloads (KeyError: slice) #485

New Issue

2026-07-08T01:04:36-05:00

jcwalker3 commented

2026-07-08 01:04:36 -05:00

Problem

_list_pr_lease_comments in gitea_mcp_server.py does:

comments = api_request("GET", api, auth) or []
return list(comments[:limit])

When the comments API returns a non-list payload (e.g. an error object such as an HTTP 401 body {"message": ...}), comments is a truthy dict, so comments[:limit] raises KeyError: slice(None, 100, None) instead of failing closed. This crashes the reviewer / conflict-fix lease path (_pr_work_lease_reviewer_block) rather than treating the response as "no lease comments".

A second, structurally identical site exists around gitea_mcp_server.py:5122:

comments = api_request("GET", api, auth) or []
...
for c in comments[:limit]:

which has the same non-list crash exposure.

How it surfaced

During PR #416 (#399) test-regression work, tests whose mocks returned a PR/error dict for the /comments endpoint crashed with KeyError: slice on the lease path. PR #416 merged (commit d4dc9aa, merge ae0478b) fixed the affected tests by patching _list_pr_lease_comments to [] in those tests, but did not fix the underlying source, so the defect is now present on master.

Required behavior

_list_pr_lease_comments (and the sibling site ~:5122) must fail safe to an empty list when the API returns a non-list payload — lease state can only be proven from real comment entries, never inferred from an error shape.
No change to behavior for valid list payloads.
Do not weaken #399 conflict-fix lease / stale-head protection, or #438/#442/#443 lock behavior.

Suggested fix (from local finding `805f324`)

comments = api_request("GET", api, auth)
if not isinstance(comments, list):
    return []
return list(comments[:limit])

Apply the same non-list guard to the :5122 site.

Acceptance criteria

_list_pr_lease_comments returns [] on a non-list payload (dict / None / error object) without raising.
The :5122 comment-iteration site is likewise guarded.
Normal list payloads behave exactly as before (lease/comment parsing unchanged).
Regression tests cover: non-list payload → no crash + empty result; valid list payload → unchanged parsing.
No relaxation of #399 lease/stale-head gates.

Reference

Local finding 805f324 (unpushed; PR #416 was already merged as d4dc9aa before the source guard could land). This issue carries that guard forward as a follow-up.

## Problem `_list_pr_lease_comments` in `gitea_mcp_server.py` does: ```python comments = api_request("GET", api, auth) or [] return list(comments[:limit]) ``` When the comments API returns a **non-list payload** (e.g. an error object such as an HTTP 401 body `{"message": ...}`), `comments` is a truthy dict, so `comments[:limit]` raises `KeyError: slice(None, 100, None)` instead of failing closed. This crashes the reviewer / conflict-fix lease path (`_pr_work_lease_reviewer_block`) rather than treating the response as "no lease comments". A second, structurally identical site exists around `gitea_mcp_server.py:5122`: ```python comments = api_request("GET", api, auth) or [] ... for c in comments[:limit]: ``` which has the same non-list crash exposure. ## How it surfaced During PR #416 (#399) test-regression work, tests whose mocks returned a PR/error dict for the `/comments` endpoint crashed with `KeyError: slice` on the lease path. PR #416 merged (commit `d4dc9aa`, merge `ae0478b`) fixed the affected **tests** by patching `_list_pr_lease_comments` to `[]` in those tests, but did **not** fix the underlying source, so the defect is now present on `master`. ## Required behavior - `_list_pr_lease_comments` (and the sibling site ~`:5122`) must fail safe to an empty list when the API returns a non-list payload — lease state can only be proven from real comment entries, never inferred from an error shape. - No change to behavior for valid list payloads. - Do not weaken #399 conflict-fix lease / stale-head protection, or #438/#442/#443 lock behavior. ## Suggested fix (from local finding `805f324`) ```python comments = api_request("GET", api, auth) if not isinstance(comments, list): return [] return list(comments[:limit]) ``` Apply the same non-list guard to the `:5122` site. ## Acceptance criteria 1. `_list_pr_lease_comments` returns `[]` on a non-list payload (dict / None / error object) without raising. 2. The `:5122` comment-iteration site is likewise guarded. 3. Normal list payloads behave exactly as before (lease/comment parsing unchanged). 4. Regression tests cover: non-list payload → no crash + empty result; valid list payload → unchanged parsing. 5. No relaxation of #399 lease/stale-head gates. ## Reference Local finding `805f324` (unpushed; PR #416 was already merged as `d4dc9aa` before the source guard could land). This issue carries that guard forward as a follow-up.

jcwalker3 added the status:in-progress label 2026-07-08 01:17:49 -05:00

jcwalker3 commented

2026-07-08 01:17:49 -05:00

Issue claim heartbeat

kind: claim
issue: Guard PR-comment/lease listing against non-list API payloads (KeyError: slice) (#485)
branch: feat/issue-485-lease-comments-non-list-guard
phase: claimed
profile: prgs-author
pr: none
blocker: none
next_action: create worktree and begin implementation

**Issue claim heartbeat** - kind: claim - issue: #485 - branch: feat/issue-485-lease-comments-non-list-guard - phase: claimed - profile: prgs-author - pr: none - blocker: none - next_action: create worktree and begin implementation

jcwalker3 referenced this issue from a commit

2026-07-08 01:17:51 -05:00

fix: guard PR/issue comment listing against non-list API payloads (Closes #485)

jcwalker3 referenced a pull request that will close this issue

2026-07-08 01:17:58 -05:00

fix: guard PR/issue comment listing against non-list API payloads (Closes #485) #487

sysadmin referenced this issue

2026-07-08 02:23:28 -05:00

fix: guard PR/issue comment listing against non-list API payloads (Closes #485) #487

jcwalker3 referenced this issue

2026-07-08 02:45:48 -05:00

Isolate MCP workspace binding across author/reviewer/merger namespaces #510