set_issue_field: remove org-level issueFields fallback from field discovery query#42517
set_issue_field: remove org-level issueFields fallback from field discovery query#42517Copilot wants to merge 3 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42517 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR updates the set_issue_field handler’s issue-field discovery to query only repository-level issueFields, avoiding org-scope GraphQL fragments that can fail under GITHUB_TOKEN with “Resource not accessible by integration”.
Changes:
- Reduced the discovery GraphQL query in
fetchIssueFieldsto repository scope only and removed the org-owner fallback path. - Updated unit tests to stop depending on org-owner field data and to assert the query does not include org/owner selections.
- Updated the live-schema integration query fixture to match the repository-only discovery query.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/set_issue_field.cjs | Removes org-owner issueFields fragment from discovery and simplifies field resolution to repository-only nodes. |
| actions/setup/js/set_issue_field.test.cjs | Updates mocks and adds assertions to enforce the repository-only discovery query contract. |
| actions/setup/js/set_issue_field_api_query.integration.test.cjs | Updates the integration test’s query fixture to match the new repository-only discovery query. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| expect(capturedQuery).not.toContain("... on User"); | ||
| expect(capturedQuery).not.toContain("... on Organization"); | ||
| expect(capturedQuery).not.toContain("owner {"); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: set_issue_field — org-level issueFields fallback removal
The fix is correct and well-motivated: querying Organization.issueFields with GITHUB_TOKEN causes the entire GraphQL request to fail ("Resource not accessible by integration"), so scoping discovery to repository-level fields is the right default.
The code simplification and updated tests (especially the new not.toContain assertions) are clean.
One concern worth addressing (see inline comment): this silently removes the org-level fallback that was previously reachable for PAT or GitHub App callers with org-read access. Users whose issue fields live at the org level (rather than the repo level) will now get an empty discovery result and an opaque "field not found" error. A CHANGELOG entry or a brief note in the action README would be enough to address this.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 31.7 AIC · ⌖ 6.58 AIC · ⊞ 4.9K
|
|
||
| const ownerFields = (result?.repository?.owner?.issueFields?.nodes ?? []).filter(isValidNode); | ||
| return ownerFields; | ||
| return (result?.repository?.issueFields?.nodes ?? []).filter(isValidNode); |
There was a problem hiding this comment.
The simplification is clean and correct for GITHUB_TOKEN, but this removes the org-level field fallback entirely — a silent behavioral regression for callers using a PAT or GitHub App token with org-read access.
Previously, if repository.issueFields returned empty, discovery fell back to Organization.issueFields. That path is now gone: a user whose custom fields live at the org level will get an empty result and a confusing "field not found" error with no indication of why.
Consider:
- Documenting this as a breaking change in a CHANGELOG or the action README, or
- Offering an
org_fallbackinput (defaultfalseforGITHUB_TOKENsafety) so PAT/GitHub App callers can opt back in.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (4 tests analyzed)
Go: 0 ( Test inflation check:
Verdict
References: §28463223094
|
There was a problem hiding this comment.
Non-blocking observation on test coverage
The core change is correct and achieves its stated goal: removing the org-level issueFields fragment eliminates the GITHUB_TOKEN permission failure. The simplified return logic is clean and semantically equivalent for the supported case.
Findings summary
1 non-blocking finding (see inline comment):
- The
ISSUE_FIELDS_DISCOVERY_QUERYconstant in the integration test is a standalone copy of the production query, not imported from the source module. If the query changes in the future, the integration test will silently validate the stale fixture rather than the real query.
Intentional tradeoff acknowledged:
- Removing the org-level fallback means repositories that define issue fields only at the org level (not mirrored at repo level) will now return an empty field list and fail with a generic "field not found" error. This is documented and accepted in the PR description as a trade-off for permissions compatibility. A warning log in
fetchIssueFieldswhen the returned field list is empty would improve debuggability.
Existing comment:
- The
not.toContain("owner {")whitespace fragility already flagged by another reviewer — no need to re-litigate.
🔎 Code quality review by PR Code Quality Reviewer · 57.1 AIC · ⌖ 7.41 AIC · ⊞ 1.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/set_issue_field_api_query.integration.test.cjs:4
Integration test fixture is a standalone duplicate — it cannot detect production query drift: ISSUE_FIELDS_DISCOVERY_QUERY is copy-pasted inline rather than imported from set_issue_field.cjs. If the production query changes in the future, this integration test will keep validating the old string against the live schema and silently pass — providing false confidence.
<details>
<summary>💡 Suggested fix</summary>
Export the query string from the production module and import it here:
``…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /grill-with-docs — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed: Removing the org-level
issueFieldsfragment is the right fix — the fragment was structurally guaranteed to fail withGITHUB_TOKEN(issues: writeis repo-scoped only). The fallback logic was effectively dead code. - Regression prevention is solid: New negative assertions (
not.toContain("... on Organization"),not.toContain("owner {")) cleanly prevent query-shape regressions. The integration test fixture is aligned with the new contract. - Minor gaps: No diagnostic logging when field discovery returns empty; no JSDoc note explaining the intentional scope;
nullissueFields not explicitly exercised in tests (guarded by?? []but untested).
Positive Highlights
- ✅ Clean simplification: the conditional fallback → single
returnreads clearly - ✅ Query-shape tests are the right regression strategy for a GQL query fix
- ✅ Integration test fixture keeps the contract explicit and auditable
- ✅ Well-scoped change — only touches what's necessary
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 49.8 AIC · ⌖ 7.86 AIC · ⊞ 6.5K
Comment /matt to run again
|
|
||
| const ownerFields = (result?.repository?.owner?.issueFields?.nodes ?? []).filter(isValidNode); | ||
| return ownerFields; | ||
| return (result?.repository?.issueFields?.nodes ?? []).filter(isValidNode); |
There was a problem hiding this comment.
[/diagnosing-bugs] When fetchIssueFields returns an empty array, the caller will eventually throw a confusing "field not found" error with no indication that field discovery came back empty.
💡 Suggested improvement
Consider 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.
|
|
||
| /** | ||
| * Fetches available issue fields for the repository/owner. | ||
| * Fetches available issue fields for the repository. |
There was a problem hiding this comment.
[/grill-with-docs] The JSDoc update from repository/owner to repository correctly reflects the scope reduction, but it doesn't explain why org-level fields are excluded.
💡 Suggested addition
/**
* Fetches available issue fields for the repository.
* `@note` Intentionally scoped to repository-level fields only. The org-level
* `issueFields` query (`repository.owner { ... on Organization }`) fails
* with `GITHUB_TOKEN` ("Resource not accessible by integration") since
* Actions tokens only carry `issues: write` at the repo level.
* `@param` {Object} githubClient ...Without this context, a future contributor might re-add org discovery as a "fix" and reintroduce the permission error.
@copilot please address this.
| 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"); |
There was a problem hiding this comment.
[/tdd] Good negative assertions — these prevent the problematic query patterns from quietly re-appearing. Consider also adding a case that verifies fetchIssueFields returns [] when result.repository.issueFields is null (not just { nodes: [] }), which can happen when the field doesn't exist in the GraphQL schema for a specific GitHub plan.
💡 Sketch
it('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 ?? [] guard already handles this safely, but making it explicit in the test suite documents the expected behaviour and catches future regressions if the guard is accidentally removed.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the reviewer's suggestion: replaced |
🤖 PR Triage
Score breakdown: Impact 30/50 · Urgency 20/30 · Quality 17/20 Rationale: Removes broken org-level GraphQL fallback that causes full query failures. CI (github-actions) approved. Clean deletion-focused fix (+4/-43). Ready for expedited human review.
|
set_issue_fieldfield discovery was querying both repository and organization issue fields in one GraphQL request. WithGITHUB_TOKEN, the org fragment can return “Resource not accessible by integration,” causing the entire query to fail and preventing field resolution.Handler query scope reduction
fetchIssueFieldsinactions/setup/js/set_issue_field.cjsto query repository-levelissueFieldsonly.repository.owner { ... on Organization { issueFields } }from the discovery query.ownerFields.Test updates for new contract
actions/setup/js/set_issue_field.test.cjsto stop mocking org-owner issue field fallback data.owner {or... on Organization.actions/setup/js/set_issue_field_api_query.integration.test.cjsquery fixture to match repository-only discovery.Behavioral outcome
set_issue_fielddiscovery now aligns with the permissions available to ActionsGITHUB_TOKEN(issues: write) and avoids org-scope query failures.