fix(server): unlink_pull_request reports a dismissed stack PR as not linked#25562

open2

main

GitHub fork: fix/mcp-unlink-dismissed-pr

opened4d agobyMMarcoDotIO

View on GitHubTwo-way sync enabledPR last imported 10h ago

Problem

When the user removes a pull request from a stack, the thread keeps it as a hidden stack-dismissed record so stack sync does not add it back. The other MCP pull request tools already treat that record as not linked: list_thread_pull_requests hides it, link_pull_request links it again, and watch_pull_request ignores it. unlink_pull_request was the only one that counted it. An agent that unlinks such a PR got wasLinked: true for a PR its own list never showed, and every repeat call did the same. Each call also sent a real thread.pull-request.unlink command. That command bumps the thread's updatedAt, and when the dismissed record no longer belongs to a stack it deletes the record, so stack sync can add the PR back.

Repro: a thread has #9 with source: "stack-dismissed". Call unlink_pull_request { repository, number: 9 }.

Before: wasLinked: true, and an unlink command is dispatched.
After: wasLinked: false, and nothing is dispatched, as the tool description says ("Succeeds with wasLinked=false when it was not linked").

Change

The unlink_pull_request handler (apps/server/src/mcp/toolkits/pullRequests/handlers.ts) now skips stack-dismissed records when it checks for a link, the same check setWatching and link_pull_request already use. The PR can still be shown again with link_pull_request, which un-dismisses it.

Scope and approval

This is a very small fix for an obvious bug, so it has no prior issue. The tool contract says wasLinked=false when the PR is not linked. The sibling tools in the same file already treat stack-dismissed as not linked. The change is one condition in one handler plus one focused test. Nothing changes in the client, contracts or orchestrator.

Verification

node_modules/.bin/vp test run apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts
- Without the fix (handler reset to upstream/main), the new test fails: × reports a dismissed stack member as not linked and leaves it dismissed, AssertionError: expected true to be false. Result: 1 failed, 18 passed.
- With the fix: 19 passed.
vp lint --report-unused-disable-directives apps/server/src/mcp/toolkits/pullRequests: no findings. vp fmt --check: clean.
tsc --noEmit in apps/server: exit 0.
Not checked: a live agent session against a real stacked PR. The handler test runs the toolkit through McpToolAccess with a mocked orchestrator.

Claude Opus 5.5 (Claude Code desktop app)

🤖 Generated with Claude Code

Checks

19 successful · 9 skipped

All checks passed

Added 1 commit4d ago

fix(server): unlink_pull_request reports a dismissed stack PR as not linkedbcd6989

MmacroscopeappBotapproved these changes4d ago

MmacroscopeappBot4d ago

Approvability

Verdict: Approved at bcd6989

Macroscope's review found this PR approvable — This is a narrowly scoped bug fix that makes unlink_pull_request consistently treat dismissed stack members as unlinked and avoids removing their tombstones. Existing link behavior is unchanged, and the new case is covered by a focused test.

^{You can add or adjust custom eligibility rules. Learn more.}

CcoderabbitaiBot4d ago

Review in Change Stack →

Important

Review skipped

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

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: main
File: .coderabbit.config.ts
Commit: 9bc0c33
Layers: Repository TS config
Review profile: CHILL
Plan: Advanced
Run ID: ef317d95-0c50-499a-a366-c3ddcd80ff1f

External image Commits

Reviewing files that changed from the base of the PR and between bcd69897fdb3b515693b105e387d9b98aa4ae99b and 40e48497d2cc047dde7382c2a84473bef332b6a8.

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.

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: main
File: .coderabbit.config.ts
Commit: 210b2e7
Layers: Repository TS config
Review profile: CHILL
Plan: Advanced
Run ID: ac11a02c-fd76-4db4-8414-6a049c609664

External image Commits

Reviewing files that changed from the base of the PR and between 210b2e7c6a8be11441b7afc94742ce3c76860111 and bcd69897fdb3b515693b105e387d9b98aa4ae99b.

External image Files selected for processing: 2

apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts
apps/server/src/mcp/toolkits/pullRequests/handlers.ts

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

Walkthrough

The unlink handler now excludes links with source stack-dismissed when checking whether a pull request is linked. A test verifies that this case returns wasLinked: false and dispatches no command.

Changes: Dismissed Stack Link Unlink Handling

Layer / File(s)	Summary
Exclude dismissed links from unlink checks `apps/server/src/mcp/toolkits/pullRequests/handlers.ts`, `apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts`	The unlink check ignores `stack-dismissed` links. The test verifies the result is `wasLinked: false` and no command is recorded.

Priority: External image Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · previous review bcd69

The focused change matches the intended dismissed-link behavior and has direct regression coverage.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title clearly and concisely describes the main fix: dismissed stack pull requests are reported as not linked by unlink_pull_request.
Description check	Passed	The description includes all required sections. It explains the problem, expected behavior, implementation, scope rationale, focused verification, and unverified live-session testing.
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.

Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

Commit to this branch
Create a new PR

🧪 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.}

Added 1 commit3d ago

Merge upstream main40e4849

Mmaria11h ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified unchanged head 40e48497d2cc047dde7382c2a84473bef332b6a8 on this linux boat vm. reproduced the bug first on github/main at 305a90dce44b8b7813d951662f13ce8510a3c669 in a separate worktree. installed each checkout with vp i and ran one background dev server at a time with isolated state.

used playwright from the repo with /usr/bin/google-chrome. created real repository projects and threads through the web ui, selected codex gpt-6.1-sol, and sent real turns through the local proxy. established the target pr link with source: "stack" through the public websocket orchestration.dispatchCommand api, then clicked "dismiss from thread" in the app. no mocks, database writes, fixtures, demo data, or changes to another pr.

main: the list omitted the dismissed link, but the agent's first unlink returned wasLinked: true. persisted event sequence 82 dispatched an mcp unlink and removed the tombstone. the second call returned false because the record was already gone.
pr: the agent's two unlink calls both returned wasLinked: false. the stack-dismissed record remained persisted, and neither call dispatched an unlink command. checked the provider-native completed mcp results as well as the app display.
nearby behavior: the agent restored the dismissed link (alreadyLinked: false), linked again (alreadyLinked: true), and listed its source as agent. ordinary unlink then returned true once, false on repeat, and left the list empty.

vp test run apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts passed all 19 tests through blacksmith-check. the unchanged head has 28 successful ci checks, zero failing or pending. no code fix or push was needed.

text

1	main pr
2	list after dismissal [] []
3	unlink first wasLinked=true wasLinked=false
4	unlink second wasLinked=false wasLinked=false
5	dismissed record deleted preserved
6	mcp unlink events 1 0

short command and provider transcript

main: empty list but first agent unlink incorrectly returns true

pr: both agent unlink calls correctly return false

pr: dismiss in the web ui, then real codex unlink calls return false

Not verified

automatic discovery or rediscovery from a real forge stack: this target pr has no stack, so the stack source was supplied through the public server api. the ui dismissal, persisted tombstone, provider calls, and relink/unlink behavior were exercised against the real running server.

native ios/android, electron, macos/windows, other providers, and relay/tunnel connections were not exercised on this linux vm. this pass used the direct web client and codex path.