Skip to content

FindIssueCommentByMarkerAsync does not pin comment sort order #218

Description

@HavenDV

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions