spec: alert maintainers about duplicate PRs (#10395)#10495
spec: alert maintainers about duplicate PRs (#10395)#10495lonexreb wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec defines the duplicate-PR detection signals, UI callout, dismissal behavior, external maintainer signal, settings, acceptance criteria, tests, and telemetry. The core behavior is covered, but several requirements need tightening before implementation.
Concerns
- PR-comment dismissal persistence needs an explicit trust model so untrusted comments cannot forge suppression markers.
- Team Slack webhook storage and handling must be specified as secret material.
- External Slack/Linear signals need persistent dedupe or cooldown beyond a single review session.
- The relationship between a per-user toggle and team-level external alert routing is ambiguous.
Security
- The spec introduces a stored Slack webhook URL and structured PR comments that affect alert behavior; both need explicit security controls before implementation.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ### B4. Suppression | ||
|
|
||
| The callout exposes a "Mark as not duplicate" button per candidate. Clicking it records a dismissal as a structured PR comment with marker `<!-- warp-dup-dismiss pr=<candidate_pr_number> -->`. On subsequent opens, the dismissed candidate is filtered out for that (PR, candidate) pair only. Dismissals are bidirectional — dismissing on PR A also hides PR A from PR B's candidate list. |
There was a problem hiding this comment.
|
|
||
| Team-level (admin-managed; surfaced under team settings UI): | ||
|
|
||
| - `team.duplicate_alert.signal.slack_webhook` (string, default empty) — Slack incoming-webhook URL. |
There was a problem hiding this comment.
| - `team.duplicate_alert.signal.slack_webhook` (string, default empty) — Slack incoming-webhook URL. | ||
| - `team.duplicate_alert.signal.linear_issue_template` (string, default empty) — Linear issue body template; supports `{pr_number}`, `{candidates}`, `{signals}` placeholders. | ||
|
|
||
| If `code.review.duplicate_alert.enabled` is `false`, neither the callout nor the signal fires for that user. |
There was a problem hiding this comment.
| - A2. Two PRs touching ≥3 files with ≥50% Jaccard path-set overlap each show the other in their callout with signal label "X% file overlap". | ||
| - A3. A 1–2 file PR matches another only when path sets are identical AND title cosine ≥ 0.70. | ||
| - A4. A dismissed candidate stays hidden for that (PR, candidate) pair on subsequent opens, in both directions. | ||
| - A5. The `MaintainerAlert.DuplicatePr` signal fires at most once per (PR, candidate) pair per session; subsequent detections of the same pair do not re-fire. |
There was a problem hiding this comment.
|
Round-1 fixes for oz-for-oss review: (1) PR-comment dismissal trust model — |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines duplicate-PR detection, in-product callouts, dismissals, and team-routed Slack/Linear maintainer alerts. The overall structure is complete, but a few requirements are ambiguous or internally inconsistent enough to produce incorrect or unsafe behavior if implemented as written.
Concerns
- The rapid-fire same-author signal does not define the
diff overlapmetric, while other sections say V1 should use path-only overlap. - The dismissal trust model uses
author_associationas a proxy for write access, butMEMBERdoes not guarantee repository write permission. - External-signal dedupe is keyed directionally even though the spec promises the cooldown applies when either PR in the pair is opened.
Security
- Dismissal suppression must be gated on an actual repository permission check; otherwise lower-privileged org members could forge suppression markers and hide duplicate alerts.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 1. **Same issue reference.** Another open PR in the same repo references the same issue via `Closes #N`, `Fixes #N`, or `Resolves #N` (case-insensitive) in its body. | ||
| 2. **File-overlap.** For PRs touching ≥3 files, Jaccard similarity of changed-file path sets ≥ 0.50 with another open PR. | ||
| 3. **Small-PR heuristic.** For PRs touching 1–2 files, all changed paths match another open PR AND title TF-IDF cosine similarity ≥ 0.70. | ||
| 4. **Rapid-fire same author.** Same author submitting more than one open PR with ≥30% diff overlap within a 24-hour window. (Captures force-push-as-new-PR cases.) |
There was a problem hiding this comment.
diff overlap is undefined and conflicts with the V1 path-only constraint/open question; specify whether this is path-set Jaccard, line-add/remove overlap, the denominator, and thresholds so tests and implementation match.
| Server-side verification: the GitHub API exposes the comment author's permissions via the `author_association` field on each comment. Suppression is only applied when: | ||
|
|
||
| ``` | ||
| author_association ∈ { OWNER, MEMBER, COLLABORATOR } |
There was a problem hiding this comment.
MEMBER is not equivalent to repo write access on org-owned repos, so this can honor forged dismissals from org members without write permission; require a direct repository permission check (write/admin) before applying the marker.
|
|
||
| ### B-Dedup. Persistent dedupe / cooldown for external signals | ||
|
|
||
| External signals (Slack/Linear) are deduped by the tuple `(source_pr, candidate_pr, signal_type)` where `signal_type ∈ { same_issue_ref, file_overlap, small_pr_match, rapid_fire_same_author }`. |
There was a problem hiding this comment.
|
Addressed all 3 important concerns from the latest oz-for-oss review:
Commit: ea02067 |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines duplicate-PR detection, Code Review pane surfacing, dismissal persistence, external Slack/Linear routing, cooldown behavior, permissions, telemetry, and tests.
Concerns
- The user-level toggle contract is internally inconsistent: the API surface says it toggles both the callout and signal emission, while the later independence rules and acceptance criteria say external signals continue regardless of individual user settings.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| User-level (Settings → Code Review → "Duplicate PR alerts"): | ||
|
|
||
| - `code.review.duplicate_alert.enabled` (bool, default `true`) — toggles both the in-product callout and signal emission for the current user. |
There was a problem hiding this comment.
Resolve user-level toggle scope inconsistency. The API surface description previously said the toggle controlled both callout and signal emission, contradicting the independence rules and acceptance criteria. The setting description now states it suppresses only the per-user callout and has no effect on team-level external signals. Added an explicit 'What it does NOT control' column to the scope table and marked it authoritative.
|
Addressed the round-N review concern about the user-toggle inconsistency.
Independence rules and acceptance criteria (A6) are now consistent with the API description. |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines duplicate-PR detection, an in-product callout, maintainer alert routing, dismissal persistence, and Slack/Linear destination handling. The high-level behavior is mostly covered, but several contract details are still ambiguous enough to produce inconsistent implementations.
Concerns
- The maintainer-alert payload is aggregated while the cooldown model is keyed per candidate pair and signal type, leaving mixed deduped/new candidate runs undefined.
- Bidirectional dismissal uses a one-sided PR comment marker, but the spec does not define where markers are scanned, how many are written, or how reversal works.
- The repository's spec convention expects product and tech contracts in predictable files, while this PR adds only a combined SPEC.md.
Security
- Slack webhook validation only requires HTTPS; the spec should either constrain destinations to Slack webhook hosts with SSRF-safe redirect/network handling or explicitly define arbitrary outbound webhook behavior and protections.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| @@ -0,0 +1,211 @@ | |||
| # Alert Maintainers When There Are Duplicates Of A PR (GH-10395) | |||
There was a problem hiding this comment.
💡 [SUGGESTION] This repo's spec workflow typically expects separate product.md and tech.md files under specs/GH10395/; either split this combined SPEC.md or state that the combined file is intentional so downstream implementation/review workflows know which contract to consume.
|
|
||
| ### B3. Maintainer signal | ||
|
|
||
| When ≥1 candidate is detected for a PR, emit `MaintainerAlert.DuplicatePr { pr_number, candidate_pr_numbers, signal_types }`. Configured destinations are read from team-level settings; admins configure once. Supported destinations: |
There was a problem hiding this comment.
|
|
||
| ### B4. Suppression | ||
|
|
||
| The callout exposes a "Mark as not duplicate" button per candidate. Clicking it records a dismissal as a structured PR comment with marker `<!-- warp-dup-dismiss pr=<candidate_pr_number> -->`. On subsequent opens, the dismissed candidate is filtered out for that (PR, candidate) pair only. Dismissals are bidirectional — dismissing on PR A also hides PR A from PR B's candidate list. |
There was a problem hiding this comment.
both sides delete semantics require knowing which PR threads are scanned and cleared; specify one-marker vs two-marker storage and deletion rules.
| - Admin reads of the webhook (e.g., to copy it for rotation) are themselves an audited event in telemetry: `team.duplicate_alert.webhook_revealed { admin_user_id }`. | ||
|
|
||
| **Transmission.** | ||
| - The webhook is invoked over **TLS only**. Plain HTTP webhook URLs are rejected at save time with a validation error. |
There was a problem hiding this comment.
1 participant
Spec for #10395.
Summary
Detect candidate-duplicate PRs (same issue reference, file-overlap Jaccard, small-PR title similarity, or rapid-fire same-author signals) and surface a non-blocking "Possible duplicates" callout in the Warp Code Review pane. Emit a structured
MaintainerAlert.DuplicatePrsignal that team admins can route to a configured Slack webhook and/or Linear issue.Scope
Notes
ready-to-spec.Test plan
code_review.*namespace