Skip to content

Commit 2df6e04

Browse files
zwickCopilot
andcommitted
Cap embedded closing pull requests and report the total
This enrichment runs on every issue_read get, so embedding up to 25 references costs more than the common case is worth. Embed at most 5, keeping orderByState so open pull requests are the ones that survive. Select totalCount alongside the nodes and return the summary as an object of total_count plus references, so the rare issue with more than five linked pull requests cannot be read as a complete list. The common zero-to-two case stays compact and an empty result stays definitive. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a0b58914-0d94-47a9-8229-c0ef7e32e69f
1 parent 1a433e8 commit 2df6e04

5 files changed

Lines changed: 88 additions & 38 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -911,7 +911,7 @@ The following sets of tools are available:
911911
- `issue_number`: The number of the issue (number, required)
912912
- `method`: The read operation to perform on a single issue.
913913
Options are:
914-
1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` lists the pull requests configured to close the issue.
914+
1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.
915915
2. get_comments - Get issue comments.
916916
3. get_sub_issues - Get sub-issues (children) of the issue.
917917
4. get_parent - Get the parent issue, if this issue is a sub-issue of another.

pkg/github/__toolsnaps__/issue_read.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
"type": "number"
1313
},
1414
"method": {
15-
"description": "The read operation to perform on a single issue.\nOptions are:\n1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` lists the pull requests configured to close the issue.\n2. get_comments - Get issue comments.\n3. get_sub_issues - Get sub-issues (children) of the issue.\n4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n5. get_labels - Get labels assigned to the issue.\n",
15+
"description": "The read operation to perform on a single issue.\nOptions are:\n1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.\n2. get_comments - Get issue comments.\n3. get_sub_issues - Get sub-issues (children) of the issue.\n4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n5. get_labels - Get labels assigned to the issue.\n",
1616
"enum": [
1717
"get",
1818
"get_comments",

pkg/github/issues.go

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -615,7 +615,7 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool {
615615
Type: "string",
616616
Description: "The read operation to perform on a single issue.\n" +
617617
"Options are:\n" +
618-
"1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` lists the pull requests configured to close the issue.\n" +
618+
"1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.\n" +
619619
"2. get_comments - Get issue comments.\n" +
620620
"3. get_sub_issues - Get sub-issues (children) of the issue.\n" +
621621
"4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n" +
@@ -791,14 +791,18 @@ func applyIssueReadEnrichment(ctx context.Context, minimalIssue *MinimalIssue, e
791791
}
792792
}
793793

794-
// An empty list is meaningful here: it tells an agent that nothing is currently set up to
795-
// close the issue, so it does not need to fall back to scanning pull requests.
796-
closing := make([]MinimalPullRequestRef, 0, len(enrichment.ClosedByPullRequests))
794+
// A zero total is meaningful here: it tells an agent that nothing is currently set up to close
795+
// the issue, so it does not need to fall back to scanning pull requests. Only a few references
796+
// are embedded, so total_count is what distinguishes a complete list from a truncated one.
797+
closing := MinimalClosingPullRequests{
798+
TotalCount: enrichment.ClosedByPullRequestsTotal,
799+
References: make([]MinimalPullRequestRef, 0, len(enrichment.ClosedByPullRequests)),
800+
}
797801
for _, pr := range enrichment.ClosedByPullRequests {
798802
if lockdownMode && !isSafeRefContent(ctx, cache, pr.Ref.Repository, pr.AuthorLogin) {
799803
continue
800804
}
801-
closing = append(closing, pr.Ref)
805+
closing.References = append(closing.References, pr.Ref)
802806
}
803807
minimalIssue.ClosedByPullRequests = &closing
804808

@@ -1852,8 +1856,9 @@ func fetchIssueFieldValuesByNodeID(ctx context.Context, gqlClient *githubv4.Clie
18521856
// extra round-trips.
18531857
//
18541858
// closedByPullRequestsReferences needs includeClosedPrs so that a merged or closed pull request
1855-
// still explains why an issue was closed, and orderByState so that open pull requests come first
1856-
// and survive the node cap.
1859+
// still explains why an issue was closed, and orderByState so that open pull requests come first.
1860+
// Only a handful of references are embedded because this enrichment runs on every issue_read `get`;
1861+
// totalCount is selected so that a truncated list is never mistaken for the complete set.
18571862
type issueReadEnrichmentQuery struct {
18581863
Nodes []struct {
18591864
Issue struct {
@@ -1874,7 +1879,8 @@ type issueReadEnrichmentQuery struct {
18741879
}
18751880
}
18761881
ClosedByPullRequestsReferences struct {
1877-
Nodes []struct {
1882+
TotalCount githubv4.Int
1883+
Nodes []struct {
18781884
Number githubv4.Int
18791885
Title githubv4.String
18801886
State githubv4.String
@@ -1886,7 +1892,7 @@ type issueReadEnrichmentQuery struct {
18861892
NameWithOwner githubv4.String
18871893
}
18881894
}
1889-
} `graphql:"closedByPullRequestsReferences(first: 25, includeClosedPrs: true, orderByState: true)"`
1895+
} `graphql:"closedByPullRequestsReferences(first: 5, includeClosedPrs: true, orderByState: true)"`
18901896
SubIssuesSummary struct {
18911897
Total githubv4.Int
18921898
Completed githubv4.Int
@@ -1912,10 +1918,11 @@ type issueReadClosingPullRequest struct {
19121918

19131919
// issueReadEnrichment is the flattened result of the issue_read `get` enrichment query.
19141920
type issueReadEnrichment struct {
1915-
FieldValues []MinimalFieldValue
1916-
Parent *issueReadParent
1917-
ClosedByPullRequests []issueReadClosingPullRequest
1918-
SubIssuesSummary MinimalSubIssuesSummary
1921+
FieldValues []MinimalFieldValue
1922+
Parent *issueReadParent
1923+
ClosedByPullRequests []issueReadClosingPullRequest
1924+
ClosedByPullRequestsTotal int
1925+
SubIssuesSummary MinimalSubIssuesSummary
19191926
}
19201927

19211928
// fetchIssueReadEnrichment runs one GraphQL nodes() query for the given issue node ID and returns
@@ -1969,6 +1976,7 @@ func fetchIssueReadEnrichment(ctx context.Context, gqlClient *githubv4.Client, n
19691976
})
19701977
}
19711978
enrichment.ClosedByPullRequests = closing
1979+
enrichment.ClosedByPullRequestsTotal = int(n.Issue.ClosedByPullRequestsReferences.TotalCount)
19721980

19731981
enrichment.SubIssuesSummary = MinimalSubIssuesSummary{
19741982
Total: int(n.Issue.SubIssuesSummary.Total),

pkg/github/issues_test.go

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func newRepoAccessHTTPClient() *http.Client {
4949
return &http.Client{Transport: &repoAccessMockTransport{responses: responses}}
5050
}
5151

52-
const issueReadEnrichmentQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}},parent{number,title,state,url,author{login},repository{nameWithOwner}},closedByPullRequestsReferences(first: 25, includeClosedPrs: true, orderByState: true){nodes{number,title,state,url,author{login},repository{nameWithOwner}}},subIssuesSummary{total,completed,percentCompleted}}}}"
52+
const issueReadEnrichmentQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}},parent{number,title,state,url,author{login},repository{nameWithOwner}},closedByPullRequestsReferences(first: 5, includeClosedPrs: true, orderByState: true){totalCount,nodes{number,title,state,url,author{login},repository{nameWithOwner}}},subIssuesSummary{total,completed,percentCompleted}}}}"
5353

5454
// newIssueReadEnrichmentMatcher builds a matcher for the issue_read `get` enrichment query for a
5555
// single issue node ID.
@@ -822,7 +822,8 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
822822
tests := []struct {
823823
name string
824824
closingPRs []map[string]any
825-
assertResponse func(t *testing.T, refs []MinimalPullRequestRef)
825+
totalCount int
826+
assertResponse func(t *testing.T, closing MinimalClosingPullRequests)
826827
}{
827828
{
828829
name: "closing pull requests are returned as compact references",
@@ -844,27 +845,40 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
844845
"repository": map[string]any{"nameWithOwner": "fork-owner/repo"},
845846
},
846847
},
847-
assertResponse: func(t *testing.T, refs []MinimalPullRequestRef) {
848-
require.Len(t, refs, 2)
848+
totalCount: 2,
849+
assertResponse: func(t *testing.T, closing MinimalClosingPullRequests) {
850+
assert.Equal(t, 2, closing.TotalCount)
851+
require.Len(t, closing.References, 2)
849852
assert.Equal(t, MinimalPullRequestRef{
850853
Number: 4242,
851854
Title: "Fix the broken thing",
852855
State: "OPEN",
853856
URL: "https://github.com/owner/repo/pull/4242",
854857
Repository: "owner/repo",
855-
}, refs[0])
858+
}, closing.References[0])
856859
// Closed and cross-repository pull requests are kept: they still explain what
857860
// is (or was) set up to close the issue.
858-
assert.Equal(t, 77, refs[1].Number)
859-
assert.Equal(t, "CLOSED", refs[1].State)
860-
assert.Equal(t, "fork-owner/repo", refs[1].Repository)
861+
assert.Equal(t, 77, closing.References[1].Number)
862+
assert.Equal(t, "CLOSED", closing.References[1].State)
863+
assert.Equal(t, "fork-owner/repo", closing.References[1].Repository)
861864
},
862865
},
863866
{
864-
name: "no closing pull requests yields an explicit empty list",
867+
name: "no closing pull requests yields an explicit zero total",
865868
closingPRs: []map[string]any{},
866-
assertResponse: func(t *testing.T, refs []MinimalPullRequestRef) {
867-
assert.Empty(t, refs)
869+
totalCount: 0,
870+
assertResponse: func(t *testing.T, closing MinimalClosingPullRequests) {
871+
assert.Equal(t, 0, closing.TotalCount)
872+
assert.Empty(t, closing.References)
873+
},
874+
},
875+
{
876+
name: "total count exceeding the embedded references marks the list as truncated",
877+
closingPRs: closingPullRequestFixtures(5),
878+
totalCount: 9,
879+
assertResponse: func(t *testing.T, closing MinimalClosingPullRequests) {
880+
require.Len(t, closing.References, 5, "at most five references are embedded")
881+
assert.Equal(t, 9, closing.TotalCount, "total_count must report the full set so a truncated list is not read as complete")
868882
},
869883
},
870884
{
@@ -879,9 +893,10 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
879893
"repository": map[string]any{"nameWithOwner": "owner/repo"},
880894
},
881895
},
882-
assertResponse: func(t *testing.T, refs []MinimalPullRequestRef) {
883-
require.Len(t, refs, 1)
884-
assert.Equal(t, "Fix the thing", refs[0].Title)
896+
totalCount: 1,
897+
assertResponse: func(t *testing.T, closing MinimalClosingPullRequests) {
898+
require.Len(t, closing.References, 1)
899+
assert.Equal(t, "Fix the thing", closing.References[0].Title)
885900
},
886901
},
887902
}
@@ -898,7 +913,7 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
898913
"id": "I_node_2990",
899914
"issueFieldValues": map[string]any{"nodes": []map[string]any{}},
900915
"parent": nil,
901-
"closedByPullRequestsReferences": map[string]any{"nodes": tc.closingPRs},
916+
"closedByPullRequestsReferences": map[string]any{"totalCount": tc.totalCount, "nodes": tc.closingPRs},
902917
"subIssuesSummary": map[string]any{"total": 0, "completed": 0, "percentCompleted": 0},
903918
},
904919
},
@@ -928,7 +943,7 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
928943
require.False(t, result.IsError, "expected result to not be an error")
929944

930945
text := getTextResult(t, result).Text
931-
assert.Contains(t, text, `"closed_by_pull_requests"`, "the key must always be present on an enriched issue so an empty list is a definitive answer")
946+
assert.Contains(t, text, `"closed_by_pull_requests"`, "the key must always be present on an enriched issue so a zero total is a definitive answer")
932947

933948
var returnedIssue MinimalIssue
934949
require.NoError(t, json.Unmarshal([]byte(text), &returnedIssue))
@@ -938,6 +953,23 @@ func Test_GetIssue_ClosedByPullRequests(t *testing.T) {
938953
}
939954
}
940955

956+
// closingPullRequestFixtures builds n distinct closing pull request nodes for the GraphQL mock.
957+
func closingPullRequestFixtures(n int) []map[string]any {
958+
prs := make([]map[string]any, 0, n)
959+
for i := range n {
960+
number := 4242 + i
961+
prs = append(prs, map[string]any{
962+
"number": number,
963+
"title": fmt.Sprintf("Candidate fix %d", number),
964+
"state": "OPEN",
965+
"url": fmt.Sprintf("https://github.com/owner/repo/pull/%d", number),
966+
"author": map[string]any{"login": "author"},
967+
"repository": map[string]any{"nameWithOwner": "owner/repo"},
968+
})
969+
}
970+
return prs
971+
}
972+
941973
func Test_GetIssue_ClosedByPullRequests_Lockdown(t *testing.T) {
942974
mockIssue := &github.Issue{
943975
Number: github.Ptr(2990),
@@ -962,6 +994,7 @@ func Test_GetIssue_ClosedByPullRequests_Lockdown(t *testing.T) {
962994
"issueFieldValues": map[string]any{"nodes": []map[string]any{}},
963995
"parent": nil,
964996
"closedByPullRequestsReferences": map[string]any{
997+
"totalCount": 2,
965998
"nodes": []map[string]any{
966999
{
9671000
"number": 4242,
@@ -1013,8 +1046,9 @@ func Test_GetIssue_ClosedByPullRequests_Lockdown(t *testing.T) {
10131046
require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returnedIssue))
10141047

10151048
require.NotNil(t, returnedIssue.ClosedByPullRequests)
1016-
require.Len(t, *returnedIssue.ClosedByPullRequests, 1, "unverified pull request references should be filtered out under lockdown")
1017-
assert.Equal(t, 4242, (*returnedIssue.ClosedByPullRequests)[0].Number)
1049+
require.Len(t, returnedIssue.ClosedByPullRequests.References, 1, "unverified pull request references should be filtered out under lockdown")
1050+
assert.Equal(t, 4242, returnedIssue.ClosedByPullRequests.References[0].Number)
1051+
assert.Equal(t, 2, returnedIssue.ClosedByPullRequests.TotalCount, "total_count reports what GitHub linked, so a filtered list is not read as complete")
10181052
}
10191053

10201054
func Test_SearchIssues(t *testing.T) {

pkg/github/minimal_types.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -485,11 +485,19 @@ type MinimalIssue struct {
485485
Parent *MinimalIssueRef `json:"parent,omitempty"`
486486
SubIssuesSummary *MinimalSubIssuesSummary `json:"sub_issues_summary,omitempty"`
487487

488-
// ClosedByPullRequests lists the pull requests configured to close this issue. It is a
489-
// pointer so that an enriched issue with no such pull requests serializes as an explicit
490-
// empty list, which is a definitive "nothing will close this issue" answer, while issues
491-
// returned by paths that never run the enrichment omit the key entirely.
492-
ClosedByPullRequests *[]MinimalPullRequestRef `json:"closed_by_pull_requests,omitempty"`
488+
// ClosedByPullRequests summarizes the pull requests configured to close this issue. It is a
489+
// pointer so that an enriched issue with no such pull requests still serializes a definitive
490+
// "nothing will close this issue" answer, while issues returned by paths that never run the
491+
// enrichment omit the key entirely.
492+
ClosedByPullRequests *MinimalClosingPullRequests `json:"closed_by_pull_requests,omitempty"`
493+
}
494+
495+
// MinimalClosingPullRequests summarizes the pull requests configured to close an issue.
496+
// References is capped, so TotalCount is authoritative: when it exceeds the number of
497+
// references the list is a truncated view rather than the complete set.
498+
type MinimalClosingPullRequests struct {
499+
TotalCount int `json:"total_count"`
500+
References []MinimalPullRequestRef `json:"references"`
493501
}
494502

495503
// MinimalPullRequestRef is a compact reference to a related pull request.

0 commit comments

Comments
 (0)