tryAGI / tryAGI/GitHub.NET

FindIssueCommentByMarkerAsync does not pin comment sort order

Open Beginner friendly
#218 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
2
Forks
0
Avg merge
1d 8h
Merged PRs (30d)
2

Description

Summary

GitHubIssueExtensions.FindIssueCommentByMarkerAsync pages through issue comments without
pinning the sort order. It requests:

/repos/{owner}/{repo}/issues/{number}/comments?per_page=100&page=1

The equivalent hand-rolled call it replaced in a downstream consumer sent:

/repos/{owner}/{repo}/issues/{number}/comments?sort=created&direction=asc&per_page=100&page=1

Why it matters

The extension finds a comment by an embedded marker. When more than one comment carries the
marker — which happens on retry, on webhook echo, or when an idempotency key is replayed — the
result depends on the traversal order, and the traversal order is now GitHub's default rather
than something the caller stated.

GitHub's documented default for listing issue comments is ascending by created, so present-day
behaviour is very likely unchanged. The concern is that the guarantee is no longer expressed: a
default that is not requested can change, and a marker search that silently returns a different
comment is not the kind of failure that surfaces quickly. The downstream correlation this feeds
records WebhookDeliveryId against the matched comment, so picking a different one attributes a
webhook to the wrong mutation.

Suggested fix

Send sort=created&direction=asc explicitly in the paginated request, so the ordering the search
relies on is stated rather than inherited.

Reproduction

Observed in tryAGI.GitHub 0.10.5 by asserting the outgoing request URI from a scripted
HttpMessageHandler:

expected: /repos/HavenDV/Advantage/issues/495/comments?sort=created&direction=asc&per_page=100&page=1
actual:   /repos/HavenDV/Advantage/issues/495/comments?per_page=100&page=1

Related observation

ApiException.Message is the raw response body. A consumer that logs ex.Message stores a bare
blob of JSON, which reads like a transport fault rather than a 4xx — it cost two wrong diagnoses
downstream before the call was instrumented. Including the status code in the message (or
documenting StatusCode as the thing to read) would make failures self-describing. Happy to open
that separately if it is worth its own issue.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at GitHubIssueExtensions.FindIssueCommentByMarkerAsync and inspect the paginated issue-comments request. Use the scripted HttpMessageHandler reproduction to verify the outgoing URI includes sort=created and direction=asc, then confirm the marker search still returns the expected comment when markers repeat.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
api
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.