feat(mcp): add custom comment minimize tool

[chaodu-agent]

Summary

Partially addresses #44 by adding the first ghpool-owned review operation: ghpool_review_minimize_comment.

What changed

• Adds a namespaced custom MCP tool to tools/list only when an authenticated agent explicitly allowlists it.
• Handles the tool locally instead of forwarding it to GitHub's hosted MCP server.
• Executes GitHub's minimizeComment GraphQL mutation with the existing scoped MCP credential.
• Verifies that the target node is an issue or pull-request comment authored by the current GitHub identity before mutating it.
• Reuses the existing write gate, repository routing/allowlist, in-flight limit, and fail-closed audit path.
• Keeps ghpool_* tools out of the upstream X-MCP-Tools header.
• Adds JSON/SSE tool-list injection tests and documents the configuration/behavior.

Security and compatibility

• The custom tool is default-deny through the existing exact agent tool allowlist.
• Invalid classifiers, missing arguments, unauthorized repositories, and non-owned comments are rejected.
• Existing upstream GitHub MCP tools remain unchanged.
• No GitHub CLI dependency is introduced.

Validation

• git diff --check passed.
• Local Rust validation could not run because this environment does not have cargo/rustc; GitHub CI is expected to run cargo check, cargo clippy -- -D warnings, and cargo test.

This PR intentionally keeps the first slice narrow; pending operations from #44 (restore, pending-review deletion/submission, and commit status) can follow the same custom-tool contract.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ - The aggregate review identifies one critical authorization issue and five important discovery, documentation, protocol, and test gaps.

Consolidated review: #45 (comment)

GitHub event: COMMENT - self-review delivery only; this is not an approval.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ - The hardened commit closes the original repository-binding and allowlist leaks, but discovery transformation safety, bounded resources, and handler-level coverage still need follow-up.

What This PR Does

This PR adds the first ghpool-owned MCP operation, ghpool_review_minimize_comment, allowing an authenticated agent to minimize an issue or pull-request comment through GitHub GraphQL without holding a GitHub token. The operation is namespaced, explicitly allowlisted, gated by enable_writes, repository-bound, and audited.

How It Works

The proxy applies authentication, the agent tool allowlist, write classification, repository policy, credential routing, in-flight limits, and fail-closed audit before local dispatch. The handler verifies the current GitHub App identity and the target comment's repository, then calls minimizeComment with the scoped App credential. For discovery, ghpool buffers and transforms JSON or SSE tools/list responses to filter upstream tools and add the local definition.

Findings

#	Severity	Finding	Location
F1	🟡	SSE tools/list transformation is lossy and can fail open. It collapses a multi-event stream into one synthetic event, dropping event metadata and unrelated messages; parse failure and overflow paths can forward an unfiltered catalog.	src/mcp.rs:493-521, src/mcp.rs:668-731
F2	🟡	Custom tools/list transformations have no bounded concurrency or aggregate memory budget. The per-request cap is checked after appending a chunk, while parse and serialization add further allocations.	src/mcp.rs:493-526, src/mcp.rs:618-630
F3	🟡	The security-sensitive handle_minimize_comment flow lacks handler-level tests for argument errors, GraphQL ownership/repository mismatch, and successful verify-plus-mutate behavior.	src/mcp.rs:798-886
F4	🟡	audit::parse_tool_outcome retains a separate single-data: SSE parser, so audit outcome parsing can diverge from the corrected tools/list parser for multi-line events.	src/audit.rs:115-128
F5	🟡	The custom GraphQL endpoint is hardcoded to github.com, so GHES or private GitHub deployments cannot use the operation without code changes or an explicit documented scope.	src/mcp.rs:58-59
F6	🟡	Verification and mutation use two sequential POST_TIMEOUT_SECS budgets. With a 120-second timeout, a stalled call can hold a per-agent write slot for nearly 240 seconds.	src/mcp.rs:768-781
F7	🟡	Custom-tool dispatch, discovery injection, header filtering, and handler selection are currently hardcoded in separate paths; adding a second ghpool-owned tool would increase maintenance cost.	src/mcp.rs:493-733

Finding Details

🟡 F1: Preserve the complete SSE envelope and fail closed

The current parser retains only the last accumulated data payload, and the serializer emits event: message plus one data field. This can discard preceding or following events and fields such as id, retry, comments, and extension fields. The BufferedBody::Overflow path forwards the original stream without filtering, and the parse-failure fallback forwards the original body without injecting or filtering. A restrictive agent can therefore receive an incomplete or unrestricted discovery catalog.

Requested change: Parse the SSE event sequence, modify only the JSON-RPC tools/list response matching the request, preserve all other events and metadata, and fail closed when filtering cannot be completed. Add multi-event, metadata-preservation, unmatched-ID, malformed, and overflow tests.

🟡 F2: Bound custom discovery resources

The custom discovery branch runs before the write in-flight guard and performs buffering, JSON parsing, and serialization. Multiple parallel requests multiply those allocations, and checking the cap after extend_from_slice permits a single oversized chunk to exceed the intended bound.

Requested change: Add a bounded per-agent or global semaphore/aggregate budget, enforce the cap before appending or split safely, and add concurrent-request and oversized-chunk tests while preserving streaming for non-custom traffic.

🟡 F3: Add handler-level GraphQL coverage

Helper tests cover schema creation, allowlist filtering, headers, and SSE assembly, but not the local handler's argument failures, classifier rejection, ownership failure, repository mismatch 403, GraphQL error response, or successful verification followed by mutation.

Requested change: Add mock GraphQL integration tests for the success path and representative fail-closed paths, including repository mismatch and invalid arguments.

🟡 F4: Share the SSE outcome parser

src/mcp.rs now joins multi-line data fields, while src/audit.rs still selects only the last data: line. A shared parser or equivalent common implementation is needed so audit and discovery interpret the same wire format.

Requested change: Move the parser to a shared module or make audit::parse_tool_outcome use the same multi-line event handling, with a regression test for audit parsing.

🟡 F5: Define the GraphQL endpoint boundary

The endpoint is a source-level constant rather than a configured GitHub API base. If github.com-only deployment is intentional, document that constraint; otherwise derive the endpoint from configuration and add coverage for the selected base.

Requested change: Make the deployment scope explicit or add a configurable endpoint without weakening credential scoping.

🟡 F6: Bound the end-to-end timeout

The two GraphQL requests each receive the full POST timeout, while the in-flight guard spans the entire local handler. A single end-to-end deadline or shorter verification budget would prevent stalled ownership checks from consuming all write slots for an excessive interval.

Requested change: Apply one total deadline across verification and mutation, or use a materially shorter verification timeout, and test slot release after timeout.

🟡 F7: Keep the MVP boundary maintainable

The narrow MVP framing and explicit invariants are useful, and a general registry is not required for this one tool. However, if another ghpool-owned tool is added, the current duplicated interception, discovery, and dispatch paths should be consolidated before expanding the surface.

Requested change: Track a follow-up to introduce a small registry/dispatch abstraction only when a second custom tool is accepted; do not broaden this PR speculatively.

Addressing Review Feedback

• Repository/node binding is now verified for both supported comment node shapes and compared case-insensitively with the authorized repository.
• Custom-only and mixed allowlists, the enable_writes discovery gate, README permissions/configuration, and multi-line data assembly were confirmed as addressed.
• Remaining feedback converges on complete SSE envelope preservation, fail-closed overflow/parse handling, bounded discovery resources, handler-level GraphQL tests, shared audit parsing, endpoint/timeout boundaries, and future custom-tool maintainability.
• The correctness review found no new correctness blocker; the remaining items are important hardening, coverage, protocol, and operability work.

Baseline Check

• PR: openabdev/ghpool#45
• Base: main
• Merge-base: 072e16dd470e73e425d90ffaf6bb94742ba2c5c2
• Head: 3162031c8d92bebe0746329d6ff03c0cd2c5462c
• Diff: README.md, docs/DESIGN.md, and src/mcp.rs; 503 additions and 7 deletions
• Net-new value: a namespaced, App-backed local MCP minimize operation with repository binding, explicit discovery gating, and fail-closed audit integration.

Validation

• git diff --check origin/main...HEAD passed.
• GitHub Actions run 29875736383 passed cargo check, cargo clippy, and cargo test for this head.
• Local cargo, rustc, and rustfmt are unavailable in the coordinator environment; Rust validation is therefore delegated to the verified GitHub CI run.
• No additional safety finding was inferred because the re-delegated safety review had not submitted a report when this aggregate was requested.

What's Good (🟢)

• The operation is namespaced and does not expose arbitrary GraphQL.
• Existing authentication, repository policy, scoped App credentials, write gate, in-flight guard, and fail-closed audit are reused.
• Repository binding now prevents cross-repository node substitution.
• The new local write gate also prevents network-trust mode from reaching the local mutation path; allowlist filtering closes the custom-only discovery gap on the successful parse path.
• README and DESIGN documentation define the supported comment types and narrow MVP boundary.

5. Three Reasons We Might Not Need This PR

1. GitHub's hosted MCP server may eventually provide a vendor-maintained minimize operation, making this local surface redundant.
2. Administrative GraphQL mutations could belong in a dedicated integration service rather than a generic MCP proxy.
3. The operational need may be solvable by procedural comment cleanup, while the long-term maintenance cost of a custom protocol/tool boundary remains uncertain.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ - Consolidated review identifies SSE discovery protocol/resource gaps, handler test coverage, parser duplication, endpoint/timeout boundaries, and a future maintainability follow-up.

Consolidated review: #45 (comment)

GitHub event: COMMENT - self-review delivery only; this is not an approval.

[chaodu-agent]

🟡 F1 - Preserve the complete SSE envelope and fail closed

This serializer emits one synthetic event and drops preceding/following events plus fields such as id, retry, comments, and extension fields. The parse-failure and overflow paths can also forward an unfiltered catalog.

Requested change: Parse the event sequence, modify only the matching tools/list response, preserve all other events and metadata, and fail closed when filtering cannot be completed. Add multi-event, unmatched-ID, malformed, and overflow tests.

[chaodu-agent]

🟡 F2 - Bound custom discovery resources

This custom discovery branch buffers, parses, and serializes responses before the write in-flight guard. Parallel requests multiply allocations, and the cap is checked after appending a chunk.

Requested change: Add a bounded semaphore or aggregate budget, enforce the cap before appending or split safely, and add concurrent and oversized-chunk tests.

[chaodu-agent]

🟡 F3 - Add handler-level GraphQL coverage

The security-sensitive handler now performs argument, ownership, and repository checks, but the test suite does not exercise the verify-plus-mutate success path or representative fail-closed responses.

Requested change: Add mock GraphQL integration tests for success, repository mismatch, ownership/GraphQL errors, invalid arguments, and classifier rejection.

[chaodu-agent]

🟡 F5 - Define the GraphQL endpoint boundary

The custom operation is tied to the github.com GraphQL endpoint while the proxy has configurable upstream behavior. GHES/private deployments cannot use this operation without code changes.

Requested change: Make github.com-only scope explicit, or derive the endpoint from configuration and test the selected base.

[chaodu-agent]

🟡 F6 - Bound the end-to-end timeout

Verification and mutation each receive the full POST timeout while the in-flight guard spans the local handler. Two stalled requests can therefore hold one write slot for nearly twice the configured timeout.

Requested change: Apply one total deadline across both requests or use a shorter verification budget, and add a timeout test proving slot release.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ → fixed in e43fb9c — the design is sound and the policy wiring is correct, but the ownership check was empirically broken: the tool could never minimize the App's own comments, which are the only comments it is allowed to touch.

What This PR Does

Adds the first ghpool-owned MCP tool, ghpool_review_minimize_comment: a namespaced, default-deny, locally-handled tool that executes GitHub's minimizeComment GraphQL mutation through the agent's scoped App credential, after verifying the comment is App-authored and belongs to the policy-checked repository.

Findings

#	Severity	Finding	Location
1	🔴	Ownership check can never pass: viewer.login for an installation token is <app-slug>[bot] but Bot comment authors carry the bare <app-slug> login — strict equality always fails, so every legitimate call is refused. Empirically verified against api.github.com with a real installation token: viewer { login } → "oab-ghpool[bot]", while a Bot-authored issue's author.login → "oab-ghpool" (__typename: Bot)	src/mcp.rs (handle_minimize_comment)
2	🔴	GITHUB_GRAPHQL_URL hard-coded const made handle_minimize_comment untestable — which is exactly why finding 1 shipped: zero tests exercised the handler; all tests covered injection/gating helpers only	src/mcp.rs
3	🟡	Tool schema declared classifier both required and with a "default" — contradictory; the handler requires it	custom_tool_definition
4	🟢	Policy wiring verified correct: parse_frame sets tool only for tools/call, so the local dispatch cannot be reached via other methods; the pre-credential deny gate covers no-agent/writes-off; allowlist + repo policy + in-flight cap + fail-closed audit preflight all run before the local handler; node_id is passed as a GraphQL variable (no injection); result-audit failure logs loudly, consistent with the existing upstream write path	src/mcp.rs:184-196, 335-360
5	🟢	The repo-binding check (node's repository.nameWithOwner vs policy-checked owner/repo) correctly prevents cross-repo minimization via a foreign node_id, and unsupported node types (commit/gist/discussion comments) fall out as author: None → refused	handle_minimize_comment

Fixes (e43fb9c)

• F1: normalize_actor() strips the [bot] suffix on both sides of the viewer/author comparison.
• F2: execute_graphql/handle_minimize_comment now take the GraphQL URL as a parameter (production passes the api.github.com const), enabling handler-level tests against a mock GraphQL server:
  • test_minimize_accepts_app_bot_authored_comment — reproduces the exact live API shape (viewer oab-ghpool[bot], author oab-ghpool); fails against the original code, passes with the fix; asserts the mutation payload (subjectId, classifier).
  • test_minimize_rejects_human_authored_comment — zero mutations.
  • test_minimize_rejects_repo_mismatch — 403, zero mutations.
  • test_minimize_rejects_graphql_errors_and_bad_classifier — GraphQL soft errors (HTTP 200 + errors[]) fail closed; unknown classifier rejected before any network call.
• F3: dropped the contradictory default from the schema.

Verified

• Live GraphQL identity shapes confirmed with a freshly minted installation token for App oab-ghpool (read-only queries; token discarded).
• At e43fb9c on arm64 macOS: cargo clippy --all-targets -- -D warnings clean; cargo test → 112 passed, 0 failed (all 13 custom-tool tests included).

5️⃣ Three Reasons We Might Not Need This PR

1. Use the REST proxy — comment minimization has no REST endpoint; it is GraphQL-only, so the existing /graphql passthrough (client's own token, no App attribution) is the only alternative — precisely what ghpool's App-backed policy plane is meant to replace.
2. Wait for upstream MCP — GitHub's hosted MCP server may eventually expose minimize; but review workflows (Add a small custom MCP review-operations layer #44) need it now, and the narrow local-tool contract keeps removal cheap if upstream ships it.
3. Let agents keep OUTDATED comments visible — cheap, but the one-comment-per-PR review SOP depends on minimizing stale bot comments; leaving them defeats the review UX this enables.

The slice is appropriately narrow. With the identity fix and handler coverage in place, this is ready.