Skip to content

fix(server): explain PR watcher read failures - #19254

Closed
andrewfree wants to merge 2 commits into
pingdotgg:mainfrom
andrewfree:fix/pr-watch-read-diagnostics-20261014
Closed

andrewfree wants to merge 2 commits into
pingdotgg:mainfrom
andrewfree:fix/pr-watch-read-diagnostics-20261014

Conversation

@andrewfree

@andrewfree andrewfree commented Oct 14, 2026 •

Copy link
Copy Markdown

When a pull-request watch exhausts eight failed reads, its agent wake currently says only that the host could not be read. The typed failure is still available, but the agent gets no reason to distinguish unavailable credentials, inaccessible pull requests, and host request failures.

Append a safe summary of the last failed read to that wake. Use typed error categories and the existing fixed unavailable-host guidance; never copy arbitrary error details, URLs, tokens, or command output. The eight-failure threshold, successful-read reset, rate-limit exemption, and original failure cause stay unchanged.

This is a focused diagnostic fix to an existing failure path, submitted under the small obvious-bug exception in CONTRIBUTING.md.

Verification on macOS arm64 with Node 24.21.0:

  • Expanded the existing real-orchestrator regression from one case to ten. On the unchanged implementation all ten failed because the persisted wake omitted its diagnostic.
  • From apps/server, vp test run src/orchestration-v2/runtimeLayer.test.ts src/orchestration-v2/pullRequestWatch.test.ts: 100 tests passed. Coverage includes the exact eighth failure, reset after recovery, rate-limit exemption, one persisted wake, known categories, unknown fallback, and exclusion of synthetic token/URL/stdout/stderr values.
  • Server package tsc-rs --noEmit and targeted vp lint for the two changed files passed.
  • No live host requests or UI verification were needed; the tests exercise the persisted message with synthetic host responses.

Implemented and independently tested with GPT-6 Astra through the Codex harness in T3 Code. Reviewed by Claude Opus 5.5 and Claude Fable 5.1 through the Claude app provider. Both passed the exact commit; maintainer human approval remains outstanding.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This is a focused diagnostic fix that changes persisted agent-facing messages on PR watch failures and handles potentially credential-bearing host errors. The summaries are allowlisted and well tested, but the sensitive-data boundary warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ed0b3dcd-39f1-49e7-a19a-14a20cbaa0c9
📥 Commits

Reviewing files that changed from the base of the PR and between 6c38fa1 and 52682f5.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5eba1c1e-6c84-40c9-a809-1a1c66f92968
📥 Commits

Reviewing files that changed from the base of the PR and between d765d03 and 6c38fa1.

📒 Files selected for processing: 2
  • apps/server/src/orchestration-v2/PullRequestWatchReactor.ts
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The pull request watch reactor now includes a typed summary of the last read failure in its stop notification after repeated failures. Tests cover several failure categories, recovery, rate-limit pauses, and notification privacy.

Changes: Pull request watch failure diagnostics

Layer / File(s) Summary
Failure summaries on stopped watches
apps/server/src/orchestration-v2/PullRequestWatchReactor.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts
The reactor maps read failures to user-facing summaries and includes the last failure summary when it stops a watch. Tests cover failure categories, recovery after a successful read, rate-limit pauses, safe diagnostics, and duplicate-notification prevention.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 6c38f

The stopped-watch diagnostic change has no identified issue requiring resolution before merge.

Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
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.
Title check Passed The title is concise, specific, and accurately describes the main change: explaining pull-request watcher read failures.
Description check Passed The description covers the problem, implementation, scope rationale, verification results, limitations, and agent disclosure. It does not use the template headings or link an issue, but it explains th…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@andrewfree

Copy link
Copy Markdown
Author

Evidence for the requested human review at 6c38fa19c9e2c8734fa51ef20748fb91ebaf052c:

  • readFailureSummary selects fixed text from typed error categories. It does not interpolate operation, detail, cause, stdout, or stderr.
  • The PullRequestUnavailableError.message branch uses the existing contract getter. It selects fixed guidance from the reason and provider requirement map, without copying the underlying error message.
  • The orchestrator regression injects synthetic private URLs, credentials, and command output, then asserts that none reaches the persisted stop event. All 100 focused tests pass, including the failure threshold, recovery reset, and rate-limit behavior.

Claude Opus 5.5 and Claude Fable 5.1 independently reviewed this exact commit through the Claude app provider and found no blocking code issues. GPT-6 Astra through Codex independently reran the focused tests. This evidence supports maintainer review; the requested human approval remains outstanding.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks for improving the watcher diagnostics. This PR adds categorized text to an existing failure notification; it does not repair failed reads or watch behavior. Under Read this first, right now we are only taking focused bug fixes from outside contributors. Please raise the missing diagnostic context in an issue and agree on scope with a maintainer before requesting reconsideration.

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants