Add confirmed repository deletion tool - #3076
Conversation
Add a destructive delete_repository tool that requires an exact owner/repo confirmation through multi-round-trip elicitation. Gate the tool to MCP protocol 2026-07-28 and newer across local and remote transports. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
There was a problem hiding this comment.
Pull request overview
Adds protocol-gated, elicitation-confirmed repository deletion.
Changes:
- Adds
delete_repositorywithdelete_reposcope and destructive annotations. - Filters tools by minimum MCP protocol version.
- Adds schema, unit, transport, and documentation updates.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents the new tool. |
pkg/scopes/scopes.go |
Defines delete_repo. |
pkg/scopes/scopes_test.go |
Tests scope expansion. |
pkg/inventory/server_tool.go |
Adds minimum protocol metadata. |
pkg/inventory/registry.go |
Installs protocol filtering. |
pkg/inventory/protocol_version.go |
Implements listing/call filtering. |
pkg/inventory/protocol_version_test.go |
Tests protocol gating. |
pkg/http/handler_test.go |
Tests HTTP tool visibility. |
pkg/github/tools.go |
Registers the deletion tool. |
pkg/github/repositories.go |
Implements confirmation and deletion. |
pkg/github/repositories_test.go |
Tests deletion and elicitation. |
pkg/github/helper_test.go |
Adds the mock endpoint constant. |
pkg/github/__toolsnaps__/delete_repository.snap |
Captures the tool schema. |
internal/ghmcp/oauth.go |
Reuses the protocol constant. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
Gate protocol-restricted tools on required elicitation capabilities and enforce direct calls inside the registered handler so SDK result finalization remains intact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Seal repository deletion targets for self-hosted HTTP with a stable AES-256-GCM key. Hide only delete_repository when no key is configured and expose an optional sealer interface for remote integrators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Bind sealed repository deletion state to the immutable repository ID and a ten-minute expiry. Re-check identity before deletion so replay cannot affect a recreated repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
…lete-repository-tool # Conflicts: # pkg/http/handler_test.go
Apply static allowlists before removing unavailable tools and fail closed on invalid configured tool names. Model independent OAuth requirements as conjunctive groups so repository deletion requires both delete_repo and repo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
|
Reviewed this closely because it is destructive and security-sensitive. Two blockers, both reproducible. Blocker 1: the confirmation does nothing in stdio mode
What is left is I called the handler directly with The repository is deleted in one round trip and the user is never asked anything. This is also why stdio is the primary target for this server, so the tool ships without the control it advertises. Reprofunc TestStdioModeConfirmationIsBypassable(t *testing.T) {
client := NewMockedHTTPClient(
WithRequestMatchHandler(DeleteReposByOwnerByRepo,
mockResponse(t, http.StatusNoContent, nil)),
)
deps := BaseDeps{Client: mustNewGHClient(t, client)}
require.Nil(t, deps.GetRequestStateSealer()) // stdio never configures one
tool := DeleteRepository(translations.NullTranslationHelper)
request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo"})
// Server never issued an elicitation. Client asserts the user confirmed.
request.Params.InputResponses = mcp.InputResponseMap{
deleteRepositoryConfirmationID: &mcp.ElicitResult{
Action: "accept",
Content: map[string]any{deleteRepositoryConfirmationField: "owner/repo"},
},
}
request.Params.RequestState = ""
result, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request)
require.NoError(t, err)
require.False(t, result.IsError) // passes: repository deleted
}Blocker 2: scope checks changed from ANY to ALL for every tool
Three existing tools declare
Running
Consequence: the README is now wrong
// Scope filtering uses "any of" semantics (see scopes.HasRequiredScopes),
// so when multiple required scopes are listed, render them as alternatives
// rather than implying all are required.The generator was not updated, so the README documents "any of" for tools that now require all three entries:
To unblock
The availability work in |
Give stdio a process-local request-state sealer and make deletion fail closed without one. Preserve legacy any-of OAuth behavior globally while documenting and enforcing delete_repository's conjunctive delete_repo and repo requirements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
|
Addressed both blockers from the latest review:
Full lint and race suites pass on |
Include delete_repo in the supported OAuth scope set used by stdio login, HTTP protected-resource metadata, and tool filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Keep delete_repo in protected-resource discovery for step-up authorization while excluding it from the default stdio OAuth grant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Generate protected-resource supported scopes and the lower-risk default OAuth grant from one canonical scope definition list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Move supported and default OAuth scope policy into pkg/scopes so protected-resource metadata and stdio grants derive from the scope domain package. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
Keep workflow and codespace in protected-resource discovery while excluding both from the default OAuth grant alongside delete_repo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b04480c-c2e9-483e-9b0f-34830b76a2f8
…lete-repository-tool
…lete-repository-tool
…lete-repository-tool
…lete-repository-tool
…lete-repository-tool
* fix(evidence): account for every narrowing decision (#403) Five failures across two adoption walks had one shape: a stage computed the right signal, stored it, and did not connect it to the decision. The sharpest instance is a fail-open in the reward-hacking shape this product exists to catch — github/github-mcp-server#3076 adds `delete_repository` (`destructiveHint: true`) to a published MCP server, and the run reported `unbound_tools: 1` beside `gap_count: 0` and `pass_eligible: true`. The checks that would have blocked it are correct; the tool left the analysed surface before they ran. Reports now carry `surface_exclusions` (report schema 0.34 → 0.35): one typed record per subject a stage removed, derived in one place from the facts the decision itself read. `detect --json` and `trigger --json` emit the same record for the stages they own, replacing four ad-hoc spellings of the same event. `accounting` makes each record checkable — `evidence_gap` (a gap row names this subject), `route_blocked` (the stage withheld its verdict), or `not_claimed` (nothing claims the subject as capability). A conservation invariant is enforced at emission: observed == analysed ∪ excluded, every excluded subject is in the ledger, every `evidence_gap` record is backed by a gap row with the same subject, and a subject this change newly excluded can never be `not_claimed`. The gate moves only where a diff proves it should. `binding_surface_diff` gains `added_unbound_tool_ids` — head exclusions minus base exclusions — and a tool in that set raises a `missing_binding_evidence` gap naming it. A pre-existing unbound catalog entry is unchanged: `samples/large_multi_framework_agent` has 58 by design, and gating on those would make declaring a spec self-blocking. `skip` now requires positive evidence. A non-empty change set no rule classified returns `evaluation_status: "unclassified"` with `should_run: null` and a next action routing forward to the scan; an empty change set keeps `no_match`. Trigger catalog 0.3 → 0.4 also adds `TRIGGER-MCP-TOOL-SCHEMA-CONTENT`, which recognises an MCP tool definition by its content rather than by a naming convention the repository never agreed to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evidence): one spelling for every gap that names a catalog tool Review of the exclusion ledger found the failure it exists to prevent, reproduced one layer up. `partial_binding_evidence` and the binding graph-issue rows spelled their subject as the raw canonical tool id, while `_unreached_tool_gaps` and `_semantic_gap` rendered `name [provider]`. The ledger joined on the second spelling, missed, and recorded a tool the decision had gapped as `not_claimed` — `binding_coverage.gap_count: 1` beside `surface_exclusions.gated: 0`, and the whole suite stayed green. Every tool-scoped gap now renders its subject through the shared `catalog_subject`, and the conservation invariant gained the two claims that would have caught it: no joinable gap may name a catalog tool by raw id, and an excluded tool the decision gapped may never be recorded `not_claimed`. The spelling rule is scoped to the gap kinds the ledger actually joins, so it does not force unrelated surfaces to change for a join that does not exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(evidence): cover the pre-decision stages, and bound the trigger ledger Review pass 2. `build_detect_exclusions` had no coverage at all — the three paths it derives from (a capped walk, a contested scope, a rejected source candidate) were exercised only through fields it does not read. Added a test per path plus the negative case, and verified each against real `detect` output rather than a constructed result. Also bounded the trigger's own ledger at 25 entries rather than the shared 200. When no rule matches, every changed file is unclassified, so those rows enumerate a list the same payload already carries in full under `changed_files` — while the result is embedded verbatim in `verifier.json` and in the Codex boundary payload written to stdout, where a few hundred copies of one identical sentence buy nothing. `total` and `gated` stay exact, so nothing that reads the counts is affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(evidence): state conservation as a property over the whole fixture corpus The invariant is enforced in `validate_semantic_consistency` at emission, so a scan returning at all already proves it. That made the proof depend on which samples other tests happen to scan. This sweeps every bundled manifest and states the property directly — including the half emission cannot check for itself, that a sample with a non-empty excluded set never ships an empty ledger. `benchmark/repos/` is materialized from `samples/` (eight of its nine archetypes are copies, per its README), so covering samples covers the benchmark corpus by construction rather than by a second sweep that would drift from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evidence): close the four fail-open routes review found (#403) All eight review findings on PR #404, each reproduced first. [P1] A negative detector no longer discards an explicit capability match. `stop_conditions` won before `run_shipgate`, so a `.snap` file holding an MCP tool schema — invisible to detect's `suggested_sources` globs, which is why this PR added content recognition for it — matched TRIGGER-MCP-TOOL-SCHEMA-CONTENT and was skipped anyway, preserving the #403 fail-open in the pre-adoption flow that supplies a complete negative detect result. The stop is terminal only while nothing contradicts it: a matched run rule is diff evidence the whole-workspace negative never accounted for. [P1] A requested base comparison that could not be performed now fails closed. `binding_surface_diff.enabled == false` conflated "nobody asked" with "asked and could not" — a v0.30 base, a baseline, or a failed verify base scan — so a head scan concluded an unbound destructive tool was pre-existing from a comparison it never ran. `base_comparison_requested` (and `VerificationContext.base_comparison_unavailable` for the verify path) separates them; that state raises one gap naming the unusable base, and ledger rows are `unverified` rather than `not_claimed`. [P1] The tri-state verdict reaches the consumers that act on it. The hooks branched on `not should_run`, turning a withheld verdict into silence; they now read `evaluation_status` and word the two cases differently. `decide-shipgate-relevance.md` teaches the tri-state and the new precedence. `trigger_catalog_schema_version` moved in step across the contract payload, `.well-known`, the local contract render, and the docs — a drift nothing compared, so a cross-surface equality test now does. [P1] A `dry_run` match no longer hides unclassified siblings. Coverage is per changed path: a dependency bump beside an opaque capability file matched a rule whose glob leg covered only the manifest. `TRIGGER-DOCS-ONLY-NEGATIVE` is unaffected by construction — `every_file_matches` only fires when it classified everything, which is the difference between a negative rule and an absent one. [P2] `BINDING_GAP_KINDS` is derived from `AgentBindingIssue.kind` instead of restated; the copy had drifted and omitted `invalid_binding_annotation`. [P2] The `adapter_parse` rows are gone. Every `source_warning` became "part of that input never entered the catalog", which is false of most: `simple_crewai_agent`'s `FileReadTool` is in the catalog, in the inventory, reachable, and high-confidence. No adapter records a typed omission today, and every provable one already reaches the ledger through `surface_completeness`, so the decision loses nothing and the ledger loses a claim it could not support. [P2] The v0.35 schema pins the nested required lists. `surface_exclusions: {}` and a dropped `added_unbound_tool_ids` both validated, so a nominally valid report could erase this PR's evidence. [P2] The cap no longer discards gated rows. Sorting them first was not enough: `rows[:limit]` still dropped one at 201 while reporting `gated=201`. The cap applies to the rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evidence): close the review-2 fail-opens and the ledger integrity gaps Ten findings on 120ccce, each reproduced first. [P1] The GitHub Action reinstated a skip the runtime refused. `trigger_action` read the raw `stop_conditions_fired` bit before the winning verdict, so the capability-content/negative-detect case — the `.snap` shape this PR exists for — published `skip_shipgate` while the runtime said run. It also collapsed both withheld states into `none`, indistinguishable from "matched nothing". It now projects the verdict, returns `withheld`, and `action.yml` exports `trigger_evaluation_status` so a workflow can tell "run the scan" from "repair the input". The CLI no longer claims a stop overrode a published RUN. [P1] The unavailable-base gap advertised a command that removed the comparison. `scan -c shipgate.yaml --format json` is executable against the head, where it drops `--diff-from` and clears the very gap it was meant to answer — and a published command reaches `fix_task.allowed_repairs`, making it a machine-readable instruction to delete the evidence. Both this gap and the pre-existing base-regeneration one now publish no command; the two steps are in `expects`, and `path` keeps the rows addressable. [P1] First adoption was routed into the failed-comparison path. `missing_manifest` means the base was read successfully and has no gate yet — the distinction `safe_recovery` already draws one function over — so asking the adopter to regenerate a base report that cannot exist made adoption over a partially-wired catalog unfinishable without falsely binding unrelated tools. [P1] The unavailable-base state was erasable. Only `unverified => base gap` was checked, so rewriting the row to `not_claimed` and dropping `gated` to 0 passed while the base gap stood. The converse is now enforced. [P2] The ledger joined tools by `name [provider]`, which two catalog ids can share. `EvidenceGap.subject_id` carries the canonical id, and every entry now names the gap accounting for it through an explicit `accounted_by` pointer — one join for three different gap shapes, instead of three renderings assumed to agree. [P2] `gated` was unvalidated: `entries: []` beside `gated: 999` passed Pydantic, the schema, and semantic validation alike. Counts are now checked against the rows in all three. [P2] The cap's guarantee was untrue of two accountings. 201 `route_blocked` or `unverified` rows kept 200 while reporting `gated=201`. `gap_backed` is the count the cap guarantees exactly — those rows carry per-row proof and are never dropped; the other two are one whole-run fact a single row proves as well as five hundred. The published wording says so. [P2] Adapter omissions are recorded again, from `LoadedToolSource.omissions` — a typed fact the MCP loader records at both of its skip branches — rather than from warning prose. An entry that genuinely never entered the catalog reaches the ledger; warnings about tools that did load stay out. [P2] `BindingSurfaceDiff.base_report_schema_version` joins the required list, and `--diff-from` that fails to parse now counts as a requested comparison: whether the bytes parsed is not the same question as whether the caller asked. [P2] `AGENTS.md`, `llms-full.txt`, and `docs/agent-contract-current.md` teach catalog 0.4 and both withheld states; the accounting enum is documented where agents read it. The parity test covers the prose surfaces and the enum now — it named only the machine payloads, which is why they drifted.
Summary
Adds a destructive
delete_repositoryMCP tool that deletes a repository only after the user enters the exactowner/reponame through elicitation. The tool is exposed only for MCP protocol2026-07-28and newer when the client supports form elicitation.Why
Repository deletion needs a stronger confirmation boundary than ordinary write operations. Self-hosted stateless HTTP deployments also need authenticated encryption for client-held MRTR request state so retries cannot alter the confirmed target.
N/A - no linked issue.
What changed
delete_repositorywith thedelete_repoOAuth scope, destructive annotations, and exact repository-name confirmation.tools/listand enforced direct calls inside registered handlers across stdio and remote HTTP transports.GITHUB_MCP_SERVER_MRTR_STATE_KEY.github.RequestStateSealerprovider contract so hosted integrations can retain their existing key format without changingToolDependencies.MCP impact
The new schema accepts
ownerandrepo; execution then requestsrepository_namethrough multi-round-trip form elicitation before calling GitHub's delete repository API. HTTP mode exposes the tool only when a valid request-state encryption key is configured.Prompts tested (tool changes only)
owner/reporepository" - automated in-memory MCP coverage verifies elicitation, exact matching, refusal paths, tamper rejection, target binding, and successful deletion.Security / limits
The tool requires the dedicated
delete_repoOAuth scope, refuses declined or mismatched confirmation, and is hidden and refused unless the request uses protocol2026-07-28or newer and advertises form elicitation support. Self-hosted HTTP uses AES-256-GCM request-state protection; missing keys hide only this tool and malformed keys fail startup.Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs