-
Notifications
You must be signed in to change notification settings - Fork 458
set_issue_field: remove org-level issueFields fallback from field discovery query #42517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
81f1607
5e019aa
56a9d94
53da6a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,7 @@ async function getIssueNodeId(githubClient, owner, repo, issueNumber) { | |
| } | ||
|
|
||
| /** | ||
| * Fetches available issue fields for the repository/owner. | ||
| * Fetches available issue fields for the repository. | ||
| * @param {Object} githubClient - Authenticated GitHub client | ||
| * @param {string} owner - Repository owner | ||
| * @param {string} repo - Repository name | ||
|
|
@@ -55,35 +55,14 @@ async function fetchIssueFields(githubClient, owner, repo) { | |
| ... on IssueFieldMultiSelect { id name options { id name } } | ||
| } | ||
| } | ||
| owner { | ||
| __typename | ||
| ... on Organization { | ||
| issueFields(first: 100) { | ||
| nodes { | ||
| __typename | ||
| ... on IssueFieldText { id name } | ||
| ... on IssueFieldNumber { id name } | ||
| ... on IssueFieldDate { id name } | ||
| ... on IssueFieldSingleSelect { id name options { id name } } | ||
| ... on IssueFieldMultiSelect { id name options { id name } } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }`, | ||
| { owner, repo } | ||
| ); | ||
|
|
||
| const isValidNode = node => typeof node?.id === "string" && typeof node?.name === "string"; | ||
|
|
||
| const repoFields = (result?.repository?.issueFields?.nodes ?? []).filter(isValidNode); | ||
| if (repoFields.length > 0) { | ||
| return repoFields; | ||
| } | ||
|
|
||
| const ownerFields = (result?.repository?.owner?.issueFields?.nodes ?? []).filter(isValidNode); | ||
| return ownerFields; | ||
| return (result?.repository?.issueFields?.nodes ?? []).filter(isValidNode); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The simplification is clean and correct for Previously, if Consider:
@copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] When 💡 Suggested improvementConsider adding a warning log before the return: const fields = (result?.repository?.issueFields?.nodes ?? []).filter(isValidNode);
if (fields.length === 0) {
console.warn(`[set_issue_field] fetchIssueFields: no valid fields found for ${owner}/${repo}`);
}
return fields;This surfaces the failure point earlier, especially during first-time setup when a misconfigured project or insufficient token scope would otherwise produce a silent empty result followed by a cryptic downstream error. @copilot please address this. |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,10 +274,6 @@ describe("set_issue_field (Handler Factory Architecture)", () => { | |
| return Promise.resolve({ | ||
| repository: { | ||
| issueFields: { nodes: [] }, | ||
| owner: { | ||
| __typename: "Organization", | ||
| issueFields: { nodes: [] }, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
@@ -349,6 +345,8 @@ describe("set_issue_field (Handler Factory Architecture)", () => { | |
| await h({ type: "set_issue_field", issue_number: 42, field_name: "Customer Impact", value: "High" }, {}); | ||
|
|
||
| expect(capturedQuery).not.toContain("... on User"); | ||
| expect(capturedQuery).not.toContain("... on Organization"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Good negative assertions — these prevent the problematic query patterns from quietly re-appearing. Consider also adding a case that verifies 💡 Sketchit('fetchIssueFields returns [] when issueFields is null', async () => {
const mockClient = {
graphql: () => Promise.resolve({ repository: { issueFields: null } })
};
const result = await fetchIssueFields(mockClient, 'owner', 'repo');
expect(result).toEqual([]);
});The @copilot please address this. |
||
| expect(capturedQuery).not.toMatch(/owner\s*\{/); | ||
| }); | ||
|
|
||
| it("fetchIssueFields filters out nodes missing id or name (null entries and unknown types)", async () => { | ||
|
|
@@ -365,7 +363,6 @@ describe("set_issue_field (Handler Factory Architecture)", () => { | |
| { __typename: "IssueFieldUnknown", name: "Orphan" }, // missing id | ||
| ], | ||
| }, | ||
| owner: { __typename: "Organization", issueFields: { nodes: [] } }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The JSDoc update from
repository/ownertorepositorycorrectly reflects the scope reduction, but it doesn't explain why org-level fields are excluded.💡 Suggested addition
Without this context, a future contributor might re-add org discovery as a "fix" and reintroduce the permission error.
@copilot please address this.