refactor: replace getAllContributors hand-rolled loop with PaginatedRequest

[Hayena]

Summary

Implements #20

Replaces the hand-rolled while(true) pagination loop in GitHubRepositoryClient.getAllContributors with the shared PaginatedRequest mechanism, consistent with all other paginated list endpoints in the codebase.

Changes

• getAllContributors now delegates to PaginatedRequest — no more manual Link header parsing or page accumulation loop
• Method signature and observable behaviour unchanged
• Existing tests still pass; regression tests added for empty/partial-result cases
• Code is noticeably simpler

[Hayena]

🔴 Bug: response body stream double-read — lens(response) will deserialize an empty body

bodyString() on line 167 reads (and for stream-backed HTTP responses, consumes) the body. When the body is not blank, lens(response) on line 174 tries to read it a second time — but the stream has already been exhausted, so the lens will deserialize an empty string and silently return an empty list instead of the actual contributors.

Tests pass only because Response.body(string) in http4k wraps a MemoryBody, which is re-readable. A real HTTP response from the underlying client will have a stream-backed body where this silently misfires.

Fix: read the body once and deserialize from the captured string:

val bodyString = response.bodyString()
if (bodyString.isBlank()) {
    // ... log and return emptyList()
} else {
    Json.decodeFromString<List<GitHubRepositoryContributor>>(bodyString)
}

Or, simpler — drop the blank-body guard entirely and let SerializationException handle it (it is already caught on line 176).

[Hayena]

🟡 lens is a required constructor parameter but goes unused when pageResult is provided

When a custom pageResult lambda is passed, defaultPageResult is never called, so the lens stored on line 80 is dead weight. Yet callers still have to provide it — here the caller even declares its own local lens on line 125 and closes over it inside the lambda instead of using the one stored in PaginatedRequest.

This makes the API misleading: the signature implies lens is always needed, but it only matters for the default code path.

Consider one of:

• Make the lens parameter optional (null default), assert-fail in defaultPageResult if it is missing, and document the contract clearly.
• Or pass lens into the pageResult lambda so callers do not need to close over their own copy: pageResult: ((Response, Int, BiDiBodyLens<List<T>>) -> PaginatedPage<T>)?
• Or split into two factory-style builders — one for the default case, one for custom handling.

[Hayena]

🟡 null used as a sentinel to fork logging logic — prefer an explicit parameter or overload

page: Int? = null is used purely to decide which log message to emit. This makes the function do two meaningfully different things under one signature, and forces every call site to reason about what null means. It also suppresses the "empty body" debug log in the paginated path silently — if someone calls this with a page number, they get a different (or no) log with no indication why.

Prefer making the intent explicit, e.g.:

private fun parseContributorsResponse(
    response: Response,
    lens: BiDiBodyLens<List<GitHubRepositoryContributor>>,
    owner: String,
    repositoryName: String,
    pageContext: String = "/",  // or just pass an already-formatted label
): List<GitHubRepositoryContributor>

Or simply inline the two slightly-different log messages at each of the two call sites and avoid the shared helper altogether — both callers are short enough to handle it themselves.

[Hayena]

🟡 Silent partial-result on mid-pagination error — caller cannot distinguish success from truncated data

When any unexpected status (5xx, 429 rate-limit, etc.) is received after at least one successful page, this returns PaginatedPage(emptyList(), hasNext = false), causing getAllContributors to return only the items collected so far with no signal of failure. The test 'return contributors collected before a later page fails' locks in this behaviour.

This is a silent data integrity hazard: callers will act on what appears to be a full contributor list but may be missing arbitrarily many contributors. Consider whether the contract should instead surface a GitHubApiException (consistent with how getContributors' else branch behaves) or at minimum return a typed result that distinguishes 'complete' from 'partial'.

[Hayena]

Kotlin Code Review

The structural direction of this PR is good — replacing the hand-rolled while(true) pagination loop with PaginatedRequest eliminates duplicated boilerplate, and the new tests cover the multi-page and error-status paths meaningfully. One real bug needs fixing before this merges; two design issues worth addressing are called out inline.

🔴 Critical

• GitHubRepositoryClient.kt:174 — parseContributorsResponse calls response.bodyString() to check for a blank body, then calls lens(response) to deserialise. For memory-backed test responses this is fine, but for stream-backed HTTP responses from a real client the stream is consumed after the first read, so lens(response) deserialises an empty string and silently returns emptyList() instead of the actual contributors. Fix: capture bodyString once and deserialise from it directly, or drop the blank-body guard and let the already-caught SerializationException handle that edge case.

🟡 Improvement

• Http4kExtensions.kt:81 — lens is a required constructor parameter of PaginatedRequest but is dead weight whenever a custom pageResult is provided, because defaultPageResult is never called. getAllContributors even declares its own local lens and closes over it inside the lambda rather than using the stored one. The constructor API is misleading — see inline comment for options.

• GitHubRepositoryClient.kt:151 — The else branch in the paginated page handler swallows unexpected errors (500, 429, …) and returns a partial list with no indication of failure. This is inconsistent with getContributors, which throws on the else branch. See inline comment.

• GitHubRepositoryClient.kt:164 — page: Int? = null is used as a sentinel to branch on which log message to emit, making the function do two subtly different things under one signature. Prefer an explicit label parameter or two separate call sites.

🟢 Looks Good

• The PaginatedPage data class is a clean, reusable abstraction.
• Test coverage is substantive: single-page, multi-page link-following, empty/not-found status codes, and mid-pagination failure are all exercised.
• The Defaults.contributor() helper is a welcome addition to the test fixtures.