fix: surface comment-post failures instead of swallowing them

[jongjesse]

Problem

upsertComment returned void and swallowed read/write failures into a warn(). A failed post (e.g. the AzDO build identity lacking Contribute to pull requests, a 403, a transient list error) was indistinguishable from a successful one — run-ci reported posted: true regardless, and a fail-on-drift run could go red with no comment on the PR explaining why. On Azure DevOps a single-page threads read could also miss our existing comment on a busy PR and post a duplicate.

Change

• upsertComment now returns boolean across the PrPlatform interface and both impls: true = a comment was actually created/updated, false = the write was intentionally skipped (nothing to do, or no credentials). Real read/write failures throw instead of warn-and-continue.
• run-ci tracks the true outcome (posted / postError), warns loudly when a post fails ("it is NOT on the PR"), and folds the post error into the fail-on-drift failure message so a red build explains itself.
• AzDO findExistingComment paginates every thread page via x-ms-continuationtoken (capped at MAX_THREAD_PAGES = 50 so a misbehaving server can't loop forever). A read failure throws rather than masquerading as "not found", which would duplicate-post on the next run.
• The read (pagination) and write paths now share a single fetchWithTimeout helper, so the new pagination path inherits the same 15s AbortController abort as every other call — no duplicated boilerplate.

Tests

tests/platforms.test.ts covers: pagination following the continuation token to a later page, the MAX_THREAD_PAGES loop guard, throw-on-read-failure (no duplicate POST), token-empty skip reported as false, and run-ci post-state tracking including a failed write folded into the fail-on-drift message (against a real git base, since fail-on-drift is gated on base availability). Full suite: 198 passing, typecheck clean, dist rebuilt.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling and reporting when comment posting fails; errors now surface to users instead of being silently suppressed.
  • Enhanced Azure DevOps comment detection by scanning all available pages, reducing risk of duplicate posts or missed existing comments.
  • Standardized request timeout handling across platforms for more consistent behavior.

• Tests

  • Added comprehensive test coverage for comment posting and error state tracking.

[coderabbitai]

📝 Walkthrough

Walkthrough

The upsertComment method signature is promoted from Promise<void> to Promise<boolean> across the PrPlatform interface, GitHubPlatform, and AzdoPlatform. Azure DevOps thread discovery is rewritten to paginate via continuation tokens with a hard page cap. runCi captures the write outcome and thrown errors as posted/postError fields in RunCiResult.

Changes

upsertComment boolean contract + AzDO pagination + runCi error tracking

Layer / File(s)	Summary
PrPlatform interface and GitHub upsertComment boolean return src/platforms/types.ts, src/platforms/github.ts	PrPlatform.upsertComment contract updated to Promise<boolean> with throw-on-failure semantics; GitHubPlatform.upsertComment returns true on write and false on intentional skip.
AzDO constants, constructor cleanup, upsertComment, and paginated thread discovery src/platforms/azdo.ts	Adds MAX_THREAD_PAGES and REQUEST_TIMEOUT_MS constants; removes injected warn callback from constructor and fromEnv; rewrites upsertComment to return boolean and propagate errors; replaces single-page thread lookup with continuation-token pagination in findExistingComment; adds listThreadsPage helper; updates fetchWithTimeout to use the new constant.
runCi posted/postError result tracking src/run-ci.ts	Adds postError?: string to RunCiResult; wraps platform.upsertComment in try/catch to assign posted from the boolean return and capture thrown errors in postError; extends the fail-on-drift message to include post error text when present.
Platform and runCi tests tests/platforms.test.ts	Adds tests for empty-token false return, multi-page marker discovery, page-cap enforcement, read-failure throw propagation (using stubbed global fetch), and runCi post-state semantics (posted/postError/failed) under four scenarios using real git repos and a fake platform.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• usepolder/drift#2: Modifies src/platforms/azdo.ts HTTP handling (request timeouts) and run-ci fail-on-drift behavior with baseAvailable gating, which are directly adjacent to the AzDO transport and runCi changes in this PR.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately and concisely describes the main change: surfacing comment-post failures instead of swallowing them, which is the core objective of the PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/surface-comment-post-failures

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/platforms/github.ts (1)

86-95: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

GitHub comment lookup lacks pagination, risking duplicates on busy PRs.

findExistingComment fetches only the first 100 comments. If a PR accumulates more than 100 issue comments (rare but possible on long-running PRs), the existing Polder Drift comment could be missed, causing a duplicate post on subsequent runs.

The AzDO implementation now paginates; consider adding pagination here for parity, or document the limitation as acceptable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/platforms/github.ts` around lines 86 - 95, The findExistingComment method
only retrieves the first 100 comments due to the per_page limit and lacks
pagination logic. To fix this, implement pagination by fetching comments in a
loop, making multiple calls to this.octokit.rest.issues.listComments with
incrementing page numbers until the response returns fewer than 100 comments.
Accumulate all comments from all pages into a single array before filtering for
the marker. This ensures no existing Polder Drift comments are missed on PRs
with more than 100 comments, preventing duplicate posts on subsequent runs.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/platforms/github.ts`:
- Around line 86-95: The findExistingComment method only retrieves the first 100
comments due to the per_page limit and lacks pagination logic. To fix this,
implement pagination by fetching comments in a loop, making multiple calls to
this.octokit.rest.issues.listComments with incrementing page numbers until the
response returns fewer than 100 comments. Accumulate all comments from all pages
into a single array before filtering for the marker. This ensures no existing
Polder Drift comments are missed on PRs with more than 100 comments, preventing
duplicate posts on subsequent runs.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d4481e81-132e-49db-85b6-c2fe89feade5

📥 Commits

Reviewing files that changed from the base of the PR and between 2da0a3a and 269aaf7.

⛔ Files ignored due to path filters (2)

• dist/cli/index.js is excluded by !**/dist/**, !dist/**
• dist/index.js is excluded by !**/dist/**, !dist/**

📒 Files selected for processing (5)

• src/platforms/azdo.ts
• src/platforms/github.ts
• src/platforms/types.ts
• src/run-ci.ts
• tests/platforms.test.ts