Add ifc label for issue_read tool#2457
Open
gokhanarkan wants to merge 1 commit into
Open
Conversation
Emits an IFC SecurityLabel on the issue_read tool result when the InsidersMode flag is enabled, mirroring the pattern landed for get_me in #2432, list_issues in #2453, get_file_contents in #2454, and search_issues in #2456. issue_read operates on a single issue in a single repository so the label has the same per-repo semantics as list_issues; the helper ifc.LabelListIssues is reused directly. Integrity is always untrusted (issue contents, comments, and label descriptions are user-authored). Public repos are labelled PublicUntrusted; private repos are labelled PrivateUntrusted with the repository's collaborator logins, falling back to [owner] when the collaborators lookup fails. The IssueRead handler dispatches to four sub-functions (GetIssue, GetIssueComments, GetSubIssues, GetIssueLabels). The IFC label is attached at the dispatch site via a single attachIFC closure, so all four method branches emit the label without changes to the underlying helpers. Visibility-lookup failures cause the label to be omitted entirely (consistent with get_file_contents and search_issues). A future cleanup PR can extract attachIFC into a shared helper now that get_file_contents, search_issues, and issue_read use near-identical closures; intentionally not bundled here to keep the diff minimal. Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. Note: chained on #2456 (gokhanarkan/fides-search-issues), which is in turn chained on #2454. GitHub will retarget the base to main once those merge.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds best-effort IFC SecurityLabel metadata to the issue_read MCP tool output when InsidersMode is enabled, aligning issue_read with the existing IFC-labeling pattern used by other ingress tools in this codebase.
Changes:
- Attach
_meta.ifcto successfulissue_readresults behindInsidersMode, using per-repo visibility + collaborators to compute aLabelListIssues-semantics label. - Apply IFC attachment uniformly across all
issue_readdispatch methods (get,get_comments,get_sub_issues,get_labels) via a single wrapper closure. - Add unit tests covering insiders on/off, public/private repo labeling, and “visibility lookup fails → omit label” behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/issues.go | Wraps IssueRead dispatch returns with an attachIFC helper that lazily computes and attaches _meta.ifc in insiders mode. |
| pkg/github/issues_test.go | Adds Test_IssueRead_IFC_InsidersMode to validate IFC metadata behavior for issue_read across key scenarios. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
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.
Emits an IFC
SecurityLabelon theissue_readtool result when theInsidersModeflag is enabled, mirroring the pattern landed forget_mein #2432,list_issuesin #2453,get_file_contentsin #2454, andsearch_issuesin #2456.Refs github/copilot-mcp-core#1623, github/copilot-mcp-core#1389. One of the ingress tools listed in #1623's tool table.
What this PR does
_meta.ifconto theissue_readCallToolResultwhendeps.GetFlags(ctx).InsidersModeis true. No behaviour change when the flag is off.issue_readoperates on a single issue in a single repository, so the label has the same per-repo semantics aslist_issues: integrity alwaysuntrusted; public repos →PublicUntrusted; private repos →PrivateUntrusted(collaborators)with[owner]fallback if the collaborators lookup fails.ifc.LabelListIssuesis reused directly — no newpkg/ifcconstructor.IssueReadhandler dispatches to four sub-functions (GetIssue,GetIssueComments,GetSubIssues,GetIssueLabels). The IFC label is attached at the dispatch site via a singleattachIFCclosure, so all fourmethod=branches emit the label without modifying the underlying helpers.get_file_contentsandsearch_issues) to avoid misclassifying the result.Suggested follow-up (not in this PR)
After this lands, three handlers (
get_file_contents,search_issues,issue_read) carry near-identicalattachIFCclosures (~25 lines each). A small follow-up PR could extract them into a single shared helper (e.g.AttachRepoIFC(ctx, deps, client, owner, repo, labelFn)inpkg/github/ifc_attach.go), saving ~60 lines and keeping the pattern consistent across tools. Intentionally not bundled here to keep this PR minimal and easy to review.Tests
Test_IssueRead_IFC_InsidersModein pkg/github/issues_test.go with 4 subtests:result.Meta == nil.method=get→integrity=untrusted,confidentiality=["public"].method=get_comments→untrusted/collaborator list (proves the wrapper applies to non-getdispatch branches).ifcmeta.get_labels(GraphQL) gets the same wrapper coverage but isn't separately tested for IFC — the wrapper sits outside the dispatch and is method-agnostic.Validation
go test -race ./...— green.gofmt -sclean;go vet ./...clean../script/lintitself fails locally with a pre-existing golangci-lint Go-version mismatch unrelated to this change.)