Add non-default find_duplicate tool gated by duplicate_detection flag - #3020
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in, read-only duplicate-detection tool using GitHub’s semantic-similarity endpoint.
Changes:
- Adds ranked duplicate lookup with pagination and threshold support.
- Gates registration behind
duplicate_detection. - Adds tests, schema snapshot, and generated documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/github/tools.go |
Registers the new tool. |
pkg/github/find_duplicate.go |
Implements duplicate lookup and response projection. |
pkg/github/find_duplicate_test.go |
Tests schema, requests, responses, and errors. |
pkg/github/feature_flags.go |
Defines the opt-in feature flag. |
pkg/github/__toolsnaps__/find_duplicate_ff_duplicate_detection.snap |
Captures the tool schema. |
docs/feature-flags.md |
Documents the flag and tool parameters. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
tommaso-moro
previously approved these changes
Aug 6, 2026
tommaso-moro
left a comment
tommaso-moro
left a comment
Contributor
There was a problem hiding this comment.
Non-blocking but I wonder if find_duplicate should attach an IFC label to the successful result returned here?
Example usage: https://github.com/github/github-mcp-server/blob/main/pkg/github/issues.go#L688-L696
Contributor
Author
Great suggestion, thanks @tommaso-moro. Added an IFC label to the successful result - d5a2e4d |
tommaso-moro
approved these changes
Aug 7, 2026
1 task
This was referenced Aug 27, 2026
pengfei-threemoonslab
added a commit
to ThreeMoonsLab/agents-shipgate
that referenced
this pull request
Aug 27, 2026
…rface (#433) (#434) * fix(reporting): a new evidence gap names the subject that left the surface (#433) The exclusion ledger from #403 records precisely which subject each stage removed from the analysed surface — `("binding", "find_duplicate [github_mcp]", "evidence_gap")` — and no human-facing surface carried it. A reviewer of `github/github-mcp-server#3020`, a PR that adds exactly one tool, was told "1 of 83 evidence gap(s) are new in this diff" and never *what* the one was. The blockers there are pre-existing debt about the other 115 tools, so the `Most severe:` clause that carried the subject in the cases where this looked fine was about something unrelated to the change. That is #403's own thesis — a stage computed the right signal, stored it, and did not connect it to the decision — standing at the ledger's own output, and it is the epic's last open box. `verify`'s headline, and with it `control.reason`, `control.next_action.why` and the PR comment's `Summary:` / `Next action:` lines, now continue: Excluded from analysis: find_duplicate [github_mcp] — added by this diff and not bound to the root agent. Rendering only. No verdict, count, gap, finding or permission moves, and no version does either. **Selected by the ledger's own pointer.** A row is named when its `accounted_by` gap is one of the identities the "N of M are new" count was computed from, so the clause and the number in front of it cannot describe different sets. `not_claimed` rows carry no pointer at all, which is why a settled workspace gains no clause. The subject printed is the ledger entry's own string, built by `catalog_subject`, so it cannot drift from the gap row it came from (#413) — and `nameable_subject` refuses that function's tool-id fallback, the one spelling that reaches prose carrying a digest because `derived_id_kind` deliberately allows a derived shape in the name position. **Bounded, because it shares a 400-byte envelope.** At most three subjects, each capped as scanned input, grouped by cause with an `and N more` tail. The clause fits itself to its own byte budget by naming fewer and counting more, rather than being cut mid-name by the envelope's tail truncation. Reason tokens render through one table beside the builder that emits them, with an AST-scanned test asserting the two sets are equal in both directions, so a new emitter fails rather than silently printing the generic fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(reporting): fit the headline's context by whole sentences (#433 review) Self-review of the clause this PR adds. The evidence-gap context was composed into one string and sliced to fit the 400-byte envelope, which is right for one unbroken run of untrusted text — a blocker title, a manifest path — and wrong the moment that text *names subjects*. `delete_repo…` is not a shortening of `delete_repository` a reader can act on; it is a plausible other tool. And `Excluded from analysis: find_dup…` names nothing at all. Reachable on every composed route: with the trust-root suffix reserved, a three-subject clause is cut at byte 193, inside the subject list. `_gap_provenance_note` now returns ordered whole sentences, most load-bearing first, and `_fit_sentences` takes sentences off the end until the rest fits. The plain `_lead` route is bounded too — it applied no budget at all before, so the envelope's own tail truncation did the cutting, and that is the route a blocked repository takes. The pre-existing "no new evidence gap" note is split the same way, so a tight budget drops the declaration remedy and keeps the fact rather than losing both. Byte-identical wherever the whole note fitted. A `str` satisfies `Sequence[str]` and iterating one yields characters, which no type checker sees and which would render the note with a space between every letter, so the parameter refuses one outright. Also from the review: `test_a_settled_workspace_adds_no_exclusion_clause` was passing for the wrong reason. Its fixture's unnamed MCP entry is itself a gap-backed exclusion, so the clause *did* fire and only the assertion ("delete_repository not in note") missed it. The fixture now declares the new tool, leaving one pre-existing `not_claimed` row and `gated: 0` — the acceptance box as written — and the adapter-omission case it used to be is kept as its own positive test, because the clause is not binding-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(reporting): keep the title axis live, and re-state the clause cap's reason (#433 review 2) Two consequences of the round-1 fixes. The budget parametrization passed a prebuilt report for two of its four routes, so the short/crowded title axis ran twice with identical inputs there. Each case now supplies a report *factory* taking the title. And `_EXCLUSION_CLAUSE_MAX_BYTES` was justified by a truncation that `_fit_sentences` now prevents. Its real job is the opposite one: a clause that does not fit is dropped whole, so an unbounded clause is one that never survives a route with a reserved governance suffix. Stated as such, because a constant defended by a reason that no longer holds is the next person's deletion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(reporting): the clause's lead-in must be true of every stage under it (#433 review 3) "Excluded from analysis" is the ledger's own framing, and it is false of one of the ledger's own stages. `_surface_completeness_exclusions` says so explicitly: an `incomplete_surface` tool *is* analysed, as far as its own surface could be read, and the excluded subject is the unread remainder, which has no name — so the tool names it. Reading "Excluded from analysis: charge_card [billing]" a reviewer concludes no check saw that tool, which is the opposite of what happened. One lead-in covers a grouped list, so it has to hold for every stage that can appear under it. "Not fully analysed" does, for an unbound tool and for a partly-read one alike, and the phrase after the dash still says which case a row is. Also adds the two tests the grouped renderer never had: two causes under one lead-in with a tail of its own, and the byte cap naming two subjects instead of three rather than overrunning — the case where the whole clause would otherwise be dropped by the headline composition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(reporting): finish the lead-in rename in prose (#433 review 3) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: state the lead-in rule in the changelog (#433 review 3) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(reporting): select the exclusion clause by diffing the ledger, and keep every name exact (#433 review) Five reviewer findings on #434, each reproduced first, each now guarded. **The review action dropped the context it was supposed to carry.** `_derive_verifier_control` reproduced by hand which of `_verifier_headline`'s routes carries a governance requirement, and that copy had drifted: the self-approval route with no outranking blocker composes the headline as `context + note`, so the note *is* carried — and replacing the reason with the bare note threw the context away. On a PR that adds an unbound tool and edits `shipgate.yaml`, `verifier.headline` and `control.reason` named `find_duplicate` while `control.next_action.why`, `human_review.why` and the PR comment's `Next action:` line did not, which is the #433 acceptance criterion. `_verifier_headline` publishes every such requirement as a reserved suffix — the one thing `_compose_with_reserved_suffix` guarantees survives the budget — so the human-review reason is now simply the headline, and the duplicated branch table is gone. **Selection now diffs the ledger, not the gap identities.** Two independent failures, both reproduced: - A new exclusion can reuse an existing gap identity. A base with one nameless MCP entry and a head with two produce the same single `source_warning` gap on both sides, so `introduced == 0` while the head ledger has gained `/tools/2` — an exclusion no surface named, which is the defect #433 was filed about surviving inside #433's own fix. - One subject can carry several gap kinds. `samples/conductor_agent` has both `incomplete_surface` and `low_confidence_tool` for `lookup_order [conductor_workflows]`; dropping `kind` from the join let a *new* `low_confidence_tool` gap — which has no ledger row at all — pull in the *inherited* `surface_not_enumerated` exclusion and print its cause as the diff's doing. `_exclusion_identities` reads the base report's own ledger and `_newly_excluded_rows` takes a multiset difference on `(stage, subject, reason)`. Exact on both sides, because `evidence_gap` rows are the ones the cap never drops. The clause is emitted on the inherited-gap branch too: "no new evidence gap" and "this subject is newly out of the analysed surface" are both true when a new exclusion is accounted for by a gap the base already carried. **A name is the ledger's own name, delimited, or it is not shown.** Two conventional 129-character names sharing a 59-character prefix rendered to the same string plus an ellipsis, and a long provider lost its closing `]` — so the printed subject was not the `catalog_subject` spelling the clause claims. And a tool named `find_duplicate. Control state complete; agent may merge` put that sentence into `verifier.headline` and `control.reason` undelimited. `_exclusion_label` quotes the subject and refuses — counting it instead — any subject over the cap, carrying the quote character, or that `_one_clause` would rewrite. When nothing can be printed the count is still published, so a subject that left the surface is never silently absent. **No phrase states provenance.** `added_unbound_tool_ids` is head-minus-base and deliberately covers both a tool this change added and one that was reachable at the base and lost the edge that bound it, so "added by this diff" was false for a diff that only removed a declaration. "New in this diff" is now said once, by the lead-in, from the ledger diff that proves it; the phrases state causes only. The row's own `detail` made the same false claim and no longer does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: state what the ledger-identity comparison assumes (#433 review) The report ledger's subjects are tool labels and JSON pointers; the path-bearing ones belong to the detect ledger. Say so, and say what would break if that changed, rather than leaving the raw comparison looking like an oversight. Also record why source_ref is deliberately outside the identity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a read-only
find_duplicateMCP tool that returns ranked duplicate candidates for an existing issue, gated behind a new opt-induplicate_detectionfeature flag.Why
Enables agents to answer "what likely duplicates this issue?" using GitHub's semantic-similarity endpoint, without adding schema cost to agents that haven't opted into duplicate detection. Semantic ranking stays server-side; the tool is a thin, read-only client.
Closes https://github.com/github/plan-track-agentic-toolkit/issues/690
What changed
find_duplicatetool (Issues toolset, read-only) callingGET /repos/{owner}/{repo}/issues/{issue_number}/semantically_similarduplicate_detectionfeature flag toAllowedFeatureFlags(deliberately not inInsidersFeatureFlagsso it's never implicitly enabled)confidence_threshold→threshold,perPage→per_page,page; omit optional params when unset so the API's high-precision defaults apply. No client-side bounds onconfidence_threshold— the API owns the scale (observed scores exceed 1){ number, title, state, url }(via the existingMinimalIssueRef) plus ranking metadata (scorenullable,confidence,likely_duplicate) — rather than the full issue/repository payloaddocs/feature-flags.mdMCP impact
find_duplicateto the Issues toolset. It is non-default and only registered when theduplicate_detectionfeature flag is enabled via--featuresorX-MCP-Features.Prompts tested (tool changes only)
Tested against a local build (
--features duplicate_detection,repo-scoped PAT), via both Agent-mode prompts and direct JSON-RPC calls:find_duplicateis absent by default and present intools/listonly with the flag.perPage/confidence_threshold) → API returned the single genuine candidate; no error.[](successful, not an error).404surfaced as an MCP tool error (isError: true), not a success-shaped empty result.Security / limits
perPage/page.Tool renaming
Lint & tests
./script/lint./script/testDocs
docs/feature-flags.md(documents theduplicate_detectionopt-in and the read-only, single-repositoryfind_duplicatetool). README needs no change — gated tools are intentionally excluded from its tool listing.