Skip to content

fix(server): dev database copies drop pull request watches - #32740

Closed
Lenoridev wants to merge 1 commit into
pingdotgg:mainfrom
Lenoridev:fix/dev-seed-drops-pr-watches
Closed

Lenoridev wants to merge 1 commit into
pingdotgg:mainfrom
Lenoridev:fix/dev-seed-drops-pr-watches

Conversation

@Lenoridev

Copy link
Copy Markdown
Contributor

Problem

vp run migrate-dev-db, and the automatic seed on a worktree's first vp run dev, keep stopped threads together with their active pull request watches (pullRequests[].watch in the thread payload). The dev server's PullRequestWatchReactor then reads those PRs with this machine's credentials. When something changes, it wakes the thread, which can start the developer's agent in the thread's real worktree while the live install's agent works on the same PR.

Fixes #32736.

Change

The prune transaction in apps/server/scripts/migrate-dev-db.ts runs one more UPDATE alongside the other clearing. It removes watch from every element of pullRequests on the kept threads. The links themselves stay, in their original order, because the UI shows them as thread history. endedWatch also stays: the reactor never reads it, and it only seeds a later re-watch. The AGENTS.md sentence about what the seed drops now includes pull request watches.

Scope and approval

This is the part of #15513 that remains. Julius's triage there named the gap: "an active PR watch on a stopped, unsettled thread survives it and can still wake" (comment). Theo closed #15513 as fixed by #22744 and asked for a new issue for any part that remains, which is #32736. #22744 does not touch watches.

Verification

  • In the existing "keeps stopped and settled thread families and clears pending work" case, the stopped-thread fixture now has a PR link with an active watch. The case asserts that the copy keeps the link without watch. On main it fails because the copy still has watch: { startedAt: … }. With the fix, all 9 tests in migrate-dev-db.test.ts pass.
  • From the code: PullRequestWatchReactor.sweep loads every watch on a non-archived thread. It skips the host read only for merged snapshots, settled threads, and subagents. A wake becomes a message.dispatch that starts a turn, and no environment or provider check blocks it.
  • vp run check:changed passes: format, lint, knip, typecheck, and tests.
  • Not checked: a real dev server waking a real watched PR. That needs GitHub credentials and would start a paid agent turn.

🤖 Generated with Claude Code

The dev seed kept stopped threads with their active pull request watches.
The dev server's PullRequestWatchReactor then read those PRs and could wake
the thread, starting the developer's agent in its real worktree. Pruning now
removes `watch` from every kept pull request link and keeps the links.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 8, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4022597

Macroscope's review found this PR approvable — Development database copies now strip pull-request watch metadata while preserving links, preventing copied watches from triggering activity in the source installation. The change is confined to the development snapshot and leaves the source database unchanged.

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

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The dev database migration now removes non-null watch metadata from pull-request links in retained thread payloads. The links remain. The test covers this behavior, and the migration instructions now list pull-request watches among removed data.

Changes: Development database watch pruning

Layer / File(s) Summary
Prune and verify pull-request watches
apps/server/scripts/migrate-dev-db.ts, apps/server/scripts/migrate-dev-db.test.ts, AGENTS.md
The migration removes non-null watch metadata from retained pull-request links. The test checks that PR 7's number and URL remain without its watch metadata. The migration instructions include pull-request watches among removed data.

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: maria-rcks

Merge Risk: Minimal · up to 40225

The change stops copied dev databases from waking agents through inherited PR watches. No merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: the dev database copy (migrate-dev-db prune) now drops pull request watches, which the diff implements. It follows conventional commit form and fits the changeset's sc…
Description check Passed The description covers the problem, the change, scope and approval, and verification. It links issue #32736 and the prior triage context. It states the test results and what was not checked. It uses t…
Linked Issues check Passed [#32736] The requirement is to keep pull request links in copied threads and remove active watches (pullRequests[].watch). The reviewed head adds an UPDATE in pruneSnapshot (apps/server/scripts/mi…
Out of Scope Changes check Passed All changed files relate to #32736. The migrate-dev-db.ts hunk implements the watch removal. The migrate-dev-db.test.ts hunk adds the watched fixture and its assertion. The AGENTS.md hunk updates the …
Full details: Title check

Explanation

The title names the main change: the dev database copy (migrate-dev-db prune) now drops pull request watches, which the diff implements. It follows conventional commit form and fits the changeset's scope.

Full details: Description check

Explanation

The description covers the problem, the change, scope and approval, and verification. It links issue #32736 and the prior triage context. It states the test results and what was not checked. It uses the repo's section headings, but the Problem section does not give a reproduction or environment in detail, and the Scope section cites #15513 history rather than a maintainer approval comment for this issue.

Full details: Linked Issues check

Explanation

[#32736] The requirement is to keep pull request links in copied threads and remove active watches (pullRequests[].watch). The reviewed head adds an UPDATE in pruneSnapshot (apps/server/scripts/migrate-dev-db.ts). It runs after the CLEARED_TABLES deletes. It rebuilds each thread's $.pullRequests with json_remove(link.value, '$.watch') and ORDER BY link.key. Links, their order, and other fields stay. Only threads with at least one watch value are updated. The endedWatch field is not changed. The test fixture stopped-thread now carries a PR 7 link with a watch. The test asserts that the migrated value is [{number: 7, url: ...}] with no watch. This matches the fixture repro in #32736. [#15513] is closed and is context only. It adds no active requirement. Limit: I did not re-read PullRequestWatchReactor in this review. The check that the reactor reads watches only from the thread payload relies on the issue text and the PR description. Those sources agree.

Full details: Out of Scope Changes check

Explanation

All changed files relate to #32736. The migrate-dev-db.ts hunk implements the watch removal. The migrate-dev-db.test.ts hunk adds the watched fixture and its assertion. The AGENTS.md hunk updates the seed description to name pull request watches. #32736 quotes that sentence as the expected behavior, so the doc change follows from the fix. The diff adds no other files or behavior changes. No unrelated change was found.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a9e709f and 4022597.

Files selected for processing: 3

  • AGENTS.md
  • apps/server/scripts/migrate-dev-db.test.ts
  • apps/server/scripts/migrate-dev-db.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.

Copy link
Copy Markdown
Member

Thank you @Lenoridev for the fix and regression test. #32744, merged as 0a9abddaee, already removes active watches from seeded threads while preserving their PR links. The remaining documentation wording is small; please feel free to open a focused PR for it if it still matters to you.

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

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

2 participants