Sign up
View stargazersSign up

fix(server): watch_pull_request re-reads a PR stored as merged before refusing it#33416

open4
main
fix/issue-33407
opened3d agobyJJulius Marminge
4 files changed
View on GitHubTwo-way sync enabledPR last imported 2d ago

Problem

When a repository is deleted and recreated under the same name, the host restarts PR numbering. A thread's link to the old PR keeps its stored merged snapshot, so watch_pull_request on the new, open PR with that number answers "The pull request is merged, so there is nothing to watch." without asking the host. The sync never re-reads a merged snapshot with mergedAt, so it never heals.

Reproduced on main (b8c2cfad6b): a dev server thread links MR !1 while it is merged, the repository path then points at a new project whose MR !1 is open, and an agent calls watch_pull_request and gets the merged refusal. The stored link still says merged after later sync sweeps.

Change

When the stored snapshot says merged, watch_pull_request asks PullRequestSyncReactor.requestSync for that link and waits for the sync worker to drain. A requested sweep invalidates the cached read, asks the host, and writes the fresh snapshot. The tool then reads the thread again and refuses only if the link still says merged. Otherwise it watches as usual. Because the stored snapshot is corrected, the watch reactor no longer ends the watch on the stale state.

Scope and approval

Fixes pingdotgg/t3code#33407

Verification

  • Red test: watches a pull request saved as merged when the host now reports it open in apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts. On main it fails with PullRequestNotOpenError: The pull request is merged, so there is nothing to watch. With the fix it passes. It also fails if the handler skips drain, because the stub host answers only once the sweep drains. The existing refuses to watch a merged pull request… test still passes: a PR the host still reports merged is refused.
  • vp test run on handlers.test.ts, McpHttpServer.test.ts and AgentContextBudget.test.ts: 56/56 pass. vp run check:changed passes (format, lint, knip, typecheck, tests).
  • Runtime, same driver before and after, fresh dev databases, real agent turns, synthetic scratch GitLab projects (renames reproduce the reused number): runtime-transcript-33407.md
    • Before (main): The pull request is merged, so there is nothing to watch., and the link stays merged.
    • After: "watching":true. The link heals to open with the new MR's title, and the watch stays active on the open MR's head commit.
  • Server-only change with no render path, so there are no captures.
  • Not checked here: GitHub specifically (the reporter's forge). The changed path is forge-independent and was exercised on GitLab. A host read that fails or is rate-limited still leaves the snapshot merged and refuses as before.

AI-assisted; verified with the tests and tools listed above.

Checks

19 successful · 15 skipped
All checks passed
4cd0707

Added 3 commits

  • test(server): the stale-merged watch test answers from the host only after drain2de4b8a
  • fix(server): watch_pull_request re-reads a PR stored as merged before refusing it768ee13
  • test(server): reproduce watch refusal from a stale merged snapshot (#33407)c0996b9
Ggithub-actionsBot3d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
CodexLive turn WebSocket decoded21.5 KiB21.5 KiB0 B (0.0%)24.4 KiB✅
CodexLive turn messages110 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.9 KiB21.9 KiB0 B (0.0%)24.4 KiB✅
ClaudeLive turn messages110 (0.0%)8✅

Baseline: 5be6bc9 · PR result: 4cd0707 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

MmacroscopeappBot3d ago
Synced from GitHub

Approvability

Verdict: Approved at 2de4b8a

Macroscope's review found this PR approvable — Watching a PR stored as merged now refreshes its host state before rejecting it. The change reuses existing synchronization safeguards and retains rejection when the refreshed snapshot remains merged.

No code changes detected at 4cd0707. Prior analysis still applies.

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

CcoderabbitaiBot3d ago
Synced from GitHub

Explain Change →

Important

Review skipped

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

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between 2de4b8ad56b34ca1ff4ef7ae9bbea1bafcb67dce and 4cd0707ca5fe4b73ef5135a8ef435c5c1085a0c8.

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.

Walkthrough

When a watch request encounters a stored merged snapshot, the toolkit now requests synchronization, waits for it to drain, and reloads the thread. It rejects the request only if the refreshed snapshot is still merged. Tests cover a snapshot refreshed to open and provide the sync reactor in related test services.

Changes: Pull Request Watch State Refresh

Layer / File(s)Summary
Synchronize before rejecting a watch request
apps/server/src/mcp/toolkits/pullRequests/handlers.ts, apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts, apps/server/src/mcp/McpHttpServer.test.ts, apps/server/src/provider/AgentContextBudget.test.ts		setWatching synchronizes and reloads a stored merged snapshot before it decides whether to reject the request. The handler test covers a snapshot refreshed to open. Related test services now provide a PullRequestSyncReactor mock.

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant Caller
3 participant PullRequestsToolkit
4 participant PullRequestSyncReactor
5 participant Orchestrator
6 Caller->>PullRequestsToolkit: Request to watch a pull request
7 PullRequestsToolkit->>PullRequestSyncReactor: Request synchronization and drain
8 PullRequestsToolkit->>Orchestrator: Reload the thread
9 Orchestrator-->>PullRequestsToolkit: Refreshed thread and linked snapshot
10 PullRequestsToolkit-->>Caller: Reject if still merged, otherwise continue watching

Suggested reviewers: spoukyii, t3dotgg

Merge Risk: External image Low · previous review 2de4b

Watching a previously merged pull request may take longer while unrelated synchronization is running. The delay is bounded by the available evidence and does not establish a blocked watch, so the change is mergeable with owner awareness.

Pre-merge checks: Passed 4

Check nameStatusExplanation
Title checkPassedThe title names the main change: watch_pull_request re-reads a pull request stored as merged before refusing it. The changeset adds a sync request and drain before the merged refusal in the pull-reque…
Description checkPassedThe description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section explains the stale merged snapshot and gives reproduction steps. The Change …
Linked Issues checkPassedLinked issue #33407 is the only active direct target. Its coding requirement: when a stored merged snapshot is stale, watch_pull_request must read the host and watch the open pull request, not refus…
Out of Scope Changes checkPassedAll changed files serve the linked objective. handlers.ts adds the PullRequestSyncReactor dependency and the merged-snapshot re-sync branch in setWatching. handlers.test.ts adds the mock sync …
Full details: Title check

Explanation

The title names the main change: watch_pull_request re-reads a pull request stored as merged before refusing it. The changeset adds a sync request and drain before the merged refusal in the pull-request handler, and the tests cover that path. The title is specific, follows conventional commit format, and is a single sentence.

Full details: Description check

Explanation

The description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section explains the stale merged snapshot and gives reproduction steps. The Change section explains the sync-and-reread fix. Verification lists a red test, the existing test result, and runtime evidence on GitLab, and it states what was not checked (GitHub). Scope and approval links issue #33407, which the objectives describe as triaged. It does not explicitly cite a maintainer approval comment, and it does not say whether the change is a small focused fix that qualifies without prior approval. That is a minor gap. The template's closing note about naming the model and harness that did the work is also absent, though the description says it is AI-assisted without naming the model.

Full details: Linked Issues check

Explanation

Linked issue #33407 is the only active direct target. Its coding requirement: when a stored merged snapshot is stale, watch_pull_request must read the host and watch the open pull request, not refuse. The change in apps/server/src/mcp/toolkits/pullRequests/handlers.ts (setWatching) does this. When before?.snapshot?.state === "merged", the handler calls pullRequestSync.requestSync(before), then pullRequestSync.drain, rereads the thread, and refuses with PullRequestNotOpenError only if the link still says merged. Otherwise it dispatches thread.pull-request.watch. PullRequestSyncReactor.requestSync is documented as forcing a reread on the next sweep, including terminal snapshots. The requested set in isDue makes the key due, so the sweep reads the host and rewrites the snapshot. This addresses the issue's point that removing the refusal alone is not enough: the stored snapshot is corrected, so the watch reactor no longer ends the watch on it. Automated test: watches a pull request saved as merged when the host now reports it open in handlers.test.ts. It uses a stub sync reactor, so it checks handler behavior, not the real sweep. The existing refusal test for a PR the host still reports merged is kept. The author's runtime transcript on GitLab shows the link healing and the watch staying active. GitHub, the reporter's forge, was not exercised. The changed path is generic over host and key, and I did not find forge-specific logic in the diff, so this does not block a PASS.

Full details: Out of Scope Changes check

Explanation

All changed files serve the linked objective. handlers.ts adds the PullRequestSyncReactor dependency and the merged-snapshot re-sync branch in setWatching. handlers.test.ts adds the mock sync reactor, modeled pending requests and a regression test for the stale merged link. McpHttpServer.test.ts and AgentContextBudget.test.ts add a PullRequestSyncReactor mock to their layers. These mocks are required because PullRequestsToolkit now depends on that service. No change touches PullRequestWatchReactor, the link or unlink paths, or other watch behavior. The issue explicitly does not request changes to other watch behavior, so no unrelated change was found.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between b8c2cfad6b3363c4da144a859073b41ec77579ab and 2de4b8ad56b34ca1ff4ef7ae9bbea1bafcb67dce.

External image Files selected for processing: 4

  • apps/server/src/mcp/McpHttpServer.test.ts
  • apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts
  • apps/server/src/mcp/toolkits/pullRequests/handlers.ts
  • apps/server/src/provider/AgentContextBudget.test.ts

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

Added 1 commit

  • Merge remote-tracking branch 'origin/main' into fix/issue-334074cd0707
JJulius Marminge2d ago

Evidence

Ran both built servers independently with fresh databases. The same driver linked the live open #33416, awaited initial synchronization, stored a synthetic merged snapshot with a merge timestamp, called watch_pull_request, and read back persisted state. Both recordings use identical 1920×1080 terminal geometry and theme.

BeforeAfter
5be6bc99c2344cd0707ca5fe
Before recordingAfter recording
Refuses as merged; stale snapshot persists; watch absent.Returns watching: true; snapshot becomes open; mergedAt clears; watch active.

Shared driver and seed · Recording runner · Method and limitations

AI-assisted; verified with the tests and tools listed above.

Not shown:

  • The full GitHub repository deletion/recreation and number-reuse sequence was not performed because this step prohibits forge writes. Evidence uses the equivalent stale metadata precondition seeded through the real server command API against a live GitCafe PR.
  • GitHub-specific integration, refusal for a genuinely merged host PR, host-error behavior, and subsequent watch notifications were not captured.
  • Freshness of origin/main could not be verified: both fetch attempts printed 'error: RPC failed; HTTP 503 curl 22 The requested URL returned error: 503' and 'fatal: expected acknowledgments'. The tested base is the merge-base computed from the existing origin/main ref.