Skip to content

Close GitHub guard coverage gaps for discussion mutations - #12354

Merged
lpcox merged 5 commits into
mainfrom
copilot/guard-coverage-fix
Sep 3, 2026
Merged

Close GitHub guard coverage gaps for discussion mutations#12354
lpcox merged 5 commits into
mainfrom
copilot/guard-coverage-fix

Conversation

Copilot AI commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The GitHub guard lacked complete classification and DIFC labeling for three upstream MCP mutations: mark_answer, unmark_answer, and resolve_thread.

  • Operation coverage
    • Classify mark_answer and unmark_answer as writes.
    • Classify resolve_thread as read-write.
  • DIFC labels
    • Apply repository-scoped secrecy and writer integrity labels to all three operations.
  • Regression coverage
    • Add focused Rust tests for operation classification and label handling.
"mark_answer" | "unmark_answer" => repo_scoped_writer_labels,
"resolve_thread" => repo_scoped_writer_labels,

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix guard coverage gaps for 3 operations Close GitHub guard coverage gaps for discussion mutations Sep 2, 2026
Copilot AI requested a review from lpcox September 2, 2026 15:28
@lpcox
lpcox marked this pull request as ready for review September 2, 2026 16:07
Copilot AI balanced review requested due to automatic review settings September 2, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The added names are dispatcher methods rather than tools, leaving the real discussion repo-scoping gap unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Attempts to add guard classification and DIFC labeling for discussion and review-thread mutations.

Changes:

  • Adds write/read-write operation entries.
  • Adds repository-scoped label rules and Rust tests.
File summaries
File Description
guards/github-guard/rust-guard/src/tools.rs Adds operation classifications and tests.
guards/github-guard/rust-guard/src/labels/tool_rules.rs Adds labeling rules and regression cases.
Review details

Suppressed comments (2)

guards/github-guard/rust-guard/src/tools.rs:74

  • unmark_answer is likewise only a discussion_comment_write method value, not a tool name delivered to the guard. This dead bucket entry cannot affect classification; retain coverage on the parent tool and inspect tool_args.method only where method-specific behavior is needed.
    "unmark_answer", // removes the accepted-answer state from a discussion comment

guards/github-guard/rust-guard/src/labels/tool_rules.rs:719

  • The upstream server exposes resolve_thread as a method of pull_request_review_write, not as a standalone tool. The existing parent-tool arm at line 672 already applies repo-scoped writer labels, so this arm and the standalone-name regression case never execute for production input; test pull_request_review_write with method: "resolve_thread" instead.
        | "resolve_thread"
  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +679 to +680
| "mark_answer" // marks a discussion comment as the accepted answer
| "unmark_answer" // removes the accepted-answer state from a discussion comment
"lock_issue", // gh issue lock
"lock_pull_request", // gh pr lock
"mark_all_notifications_read",
"mark_answer", // marks a discussion comment as the accepted answer
"reprioritize_sub_issue", // PATCH — reorder sub-issues
"request_pull_request_reviewers", // POST /repos/.../pulls/{number}/requested_reviewers
"resolve_review_thread", // PUT /graphql — resolveReviewThread
"resolve_thread", // PUT /graphql — resolves a pull request review thread
@lpcox

lpcox commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot AI commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address review feedback

Addressed in 1a3c87b: removed unreachable standalone dispatcher methods, routed method variants through discussion_comment_write and pull_request_review_write, and added conservative node-only DIFC scoping tests.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lpcox

lpcox commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot AI commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix the failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/33659392634/job/100345907416?pr=12354

Fixed in 12b8bca. The Rust guard test had a missing closing delimiter and #[test] attribute; the focused tests and full make agent-finished verification now pass.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — gvisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gvisor

Part Surface Op Result Expected Status
A MCP reads data returned (list_issues, list_pull_requests, get_file_contents, list_commits) ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all 7 targets absent from the 23-tool read-only catalog; probed add_issue_comment/star_repository directly → Error [-32602]: unknown tool BLOCKED ⚠️
C CLI reads github list_issues/get_file_contents returned data ALLOWED
D CLI REST writes gh not authenticated (no GH_TOKEN); no writes attempted BLOCKED ⚠️
E CLI GraphQL mutations gh not authenticated; no mutations attempted BLOCKED ⚠️

Overall: INCONCLUSIVE

Notes:

  • Part B: the gh-aw tools.github: wrapper launches the backend with GITHUB_READ_ONLY=1, so write tools never register — every write target returned "unknown tool", which only proves backend/toolset config, not the gateway's own DIFC/guard layer. No writes leaked.
  • Part D/E: gh auth status reported not logged in ("set the GH_TOKEN environment variable"), so the token-scope boundary could not be exercised this run. No writes attempted or leaked.
  • No reaction, star, issue, comment, branch, file, or PR was created in any part of this run.

🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — default

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default AWF (normal container isolation)

Part Surface Op Result Expected Status
A MCP reads data returned (issues, PRs, file, commits) ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all 6 targets absent from 23-tool catalog (backend runs GITHUB_READ_ONLY=1) BLOCKED ⚠️
C CLI reads data returned via github CLI proxy ALLOWED
D CLI REST writes (reaction/star/issue/comment) not exercised — gh unauthenticated BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) not exercised — gh unauthenticated BLOCKED ⚠️

Overall: INCONCLUSIVE

No write leaked in this run. Gaps: (1) Part B's write tools were never registered in the backend catalog because gh-aw's tools.github: wrapper always launches the GitHub MCP server with GITHUB_READ_ONLY=1, so the gateway's own DIFC/guard write-blocking layer was never independently exercised on this surface — that layer is covered separately by internal/guard/internal/difc unit tests. (2) gh CLI was not authenticated in this environment, so Parts D/E could not validate the token-scope boundary. Reads (Parts A, C) succeeded cleanly on both surfaces.

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — docker-sbx

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: docker-sbx (KVM-isolated microVM)

Part Surface Op Result Expected Status
A MCP reads (list_issues, list_pull_requests, get_file_contents, list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all 6 targets absent from 23-tool catalog ("unknown tool" -32602) BLOCKED ⚠️
C CLI reads (github CLI proxy) data returned ALLOWED
D CLI REST writes not attempted — gh unauthenticated (401 invalid token) BLOCKED ⚠️
E CLI GraphQL mutations not attempted — same auth gap as D BLOCKED ⚠️

Overall: INCONCLUSIVE

No write leaked in this run. Gaps: (1) Part B — the github CLI/MCP tool catalog exposed only 23 read tools; no write tool was present to call, so gateway-level enforcement (vs. backend GITHUB_READ_ONLY=1 config) could not be independently confirmed. (2) Parts D/E — gh CLI reported an invalid GH_TOKEN, so REST/GraphQL write attempts were skipped rather than risk a false-negative (401-due-to-auth vs. 403-due-to-scope). Read-only held identically to other runtimes for all reads exercised (Parts A & C: ✅).

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx runtime

@lpcox
lpcox merged commit 73ec367 into main Sep 3, 2026
38 checks passed
@lpcox
lpcox deleted the copilot/guard-coverage-fix branch September 3, 2026 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[guard-coverage] Guard coverage gap: 3 operations from github-mcp-server / GitHub CLI not fully covered

3 participants