Limit update_project item lookup to target content - #50839
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Moves existing project-item lookup to the target issue or pull request, reducing unrelated repository permission requirements.
Changes:
- Queries
projectItemsfrom target content and matches the requested project. - Preserves item creation when no match exists.
- Updates fixtures and adds focused issue-side coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_project.cjs |
Implements content-side item lookup. |
actions/setup/js/update_project.test.cjs |
Updates mocks and tests issue lookup behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| ... on PullRequest { | ||
| projectItems(first: 100, after: $after) { |
There was a problem hiding this comment.
Added finds an existing pull request item from the pull request side in cf04dbd. It asserts the item-lookup query contains the ... on PullRequest fragment (the mock throws otherwise), returns a matching item for the target project, and verifies addProjectV2ItemById is never called.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The optimization looks correct. Querying projectItems from the content node (Issue/PullRequest) rather than scanning all project items is more efficient and avoids loading unrelated project items. The fragment placement and pagination logic are correct, and tests are updated consistently.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.9 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — two issues found, commenting rather than blocking.
📋 Key Themes & Highlights
Key Themes
- Missing PullRequest content type test: the new focused test only exercises the
Issuepath; thePullRequestbranch of the dual... on Issue / ... on PullRequestquery has no dedicated coverage. - Misleading warning on empty project items:
result?.node?.projectItemsbeing falsy can mean node-not-found or legitimately no project items — the current warning doesn't distinguish these cases.
Positive Highlights
- ✅ Excellent security improvement: querying from the content side avoids expanding unrelated board items and reduces permission blast radius on org-wide projects.
- ✅ The fragment
ProjectItemProjectis a clean, reusable abstraction that avoids duplication across the two type branches. - ✅ Comprehensive fixture updates keep all existing tests honest about the new query shape.
|
|
||
| it("adds a draft issue to a project board", async () => { | ||
| const projectUrl = "https://github.com/orgs/testowner/projects/60"; | ||
| const output = { |
There was a problem hiding this comment.
[/tdd] The new focused test only covers content_type: "issue", but the GraphQL query handles both ... on Issue and ... on PullRequest. A PR content type could silently fall back to addProjectV2ItemById without any test catching it.
💡 Suggested addition
Add a sibling test with content_type: "pull_request" that:
- verifies the query contains
projectItems( - verifies
item-idis returned from the content-side lookup - asserts
addProjectV2ItemByIdis not called
This closes the coverage gap between the two ... on Issue / ... on PullRequest branches in the new query.
@copilot please address this.
There was a problem hiding this comment.
Added a content_type: "pull_request" sibling test in cf04dbd that resolves the lookup only when the query contains ... on PullRequest and projectItems(, returns the existing item id, and asserts addProjectV2ItemById is not called.
| const found = result.node.items.nodes.find(item => item.content?.id === contentId); | ||
| const found = projectItems.nodes.find(item => item.project?.id === projectId); | ||
| if (found) return found; | ||
|
|
There was a problem hiding this comment.
[/codebase-design] result?.node?.projectItems silently breaks when the queried node is a PullRequest — GraphQL returns the data under result.node.projectItems for both types, so the path is the same, but the warning message says Content ${contentId} not found or inaccessible. If the content node exists but has no project items (legitimate empty result), this warning fires incorrectly.
💡 Suggested fix
Distinguish between node-not-found and empty-items:
if (!result?.node) {
core.warning(`Content ${contentId} not found or inaccessible; stopping project item search.`);
break;
}
const projectItems = result.node.projectItems;
if (!projectItems) break; // node exists but has no projectItems field — shouldn't happenThis prevents the misleading warning from firing on legitimate empty-project cases.
@copilot please address this.
There was a problem hiding this comment.
Fixed in cf04dbd: the warning now only fires when result.node is missing; a present node without projectItems just stops the search silently.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve with non-blocking test-coverage gaps
The refactor from a broad ProjectV2.items scan to a targeted Issue.projectItems/PullRequest.projectItems lookup is correct and reduces the permission blast radius as intended. Core logic (pagination loop, cursor advancement, matching on project.id) is sound and the caller always resolves content to an Issue or PullRequest ID before invoking this function, so the "unsupported content type" concern raised by first-pass mining doesn't apply here.
💡 Themes and highlights
- Correctness: query/fragment shape, pagination cursor handling, and warning-on-missing-node behavior are consistent with the prior implementation's guarantees.
- Test gaps (non-blocking): the new dedicated regression test only covers the
Issuefragment path with a single (non-paginated) response. Neither thePullRequestfragment branch with a found item, nor multi-page pagination throughprojectItems, are exercised by any test. Filed two review comments requesting coverage for both. - These are maintainability/test-debt concerns, not functional bugs in the shipped diff — they lower confidence in future regressions to this code path but don't block this PR.
| orgProjectV2Response(projectUrl, 60, "project-pagination"), | ||
| issueResponse("issue-id-15"), | ||
| existingItemResponse("issue-id-15", "item-pagination"), | ||
| existingItemResponse("project-pagination", "item-pagination"), |
There was a problem hiding this comment.
This pagination test reuses the old single-page existingItemResponse for the item lookup, so the new paginated projectItems query path (multi-page hasNextPage: true → false) is never actually exercised.
💡 Why this matters
The refactor changed the query shape (from ProjectV2.items to Issue/PullRequest.projectItems) and kept the same pagination loop structure, but no test drives projectItems.pageInfo.hasNextPage === true for more than one page. A regression such as forgetting to pass after: endCursor correctly, or an infinite loop if endCursor is not advanced, would go completely undetected — the only paginated test in this file (line 1151) exercises field pagination, not item-lookup pagination.
Suggested fix: add a dedicated test where findExistingItemByContentId receives two projectItems responses — first with hasNextPage: true and a cursor, second with the match — and assert both graphql calls happened with the correct after variable.
There was a problem hiding this comment.
Added paginates content project items until the target project item is found in cf04dbd: the first projectItems page returns an item for a different project with hasNextPage: true, and the test asserts both lookup calls happened with after: null then after: "cursor-items-1".
| } | ||
| } | ||
| } | ||
| ... on PullRequest { |
There was a problem hiding this comment.
The new dedicated regression test only exercises the Issue inline fragment; the PullRequest branch of this query is completely untested, so a typo or schema drift in the PR-side fragment usage would silently break PR item lookups.
💡 Why this matters
Both ... on Issue and ... on PullRequest blocks duplicate the same projectItems(first: 100, after: $after) { nodes { ...ProjectItemProject } pageInfo {...} } shape. The existing pull_request test ("adds a pull request to the project board") uses emptyItemsResponse() which returns items/projectItems with empty nodes, so it never verifies that a found existing item is correctly returned via the PullRequest branch, nor that the query variables/shape are right for that branch.
Suggested fix: mirror the new issue-side test for content_type: "pull_request", asserting the query resolves via the PullRequest fragment and returns a matching item from existingItemResponse.
There was a problem hiding this comment.
Mirrored the issue-side test for content_type: "pull_request" in cf04dbd; the mock only answers the lookup when the query contains the ... on PullRequest fragment, and the test asserts the found item is returned without addProjectV2ItemById.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Maintainer triage for this PR:
Run: https://github.com/github/gh-aw/actions/runs/31113572782
|
Ran Review follow-ups — already landed in
Branch refresh — the branch-update merge Local validation on the merged head — Status
No merge was performed. |
|
🎉 This pull request is included in a new release. Release: |
safe-outputs.update_projectmatched existing issue/PR items by scanning everyProjectV2.itemspage and expanding each item’s content. On mixed-repository org projects, that can require repo/PR read access unrelated to the target item.Content-side lookup
Issue.projectItems/PullRequest.projectItemsfor the resolved content node.project.idmatches the requested project.Reduced permission blast radius
addProjectV2ItemByIdwhen the target content is not already on the board.Coverage
projectItemsand do not query broadProjectV2.itemscontent.Run: https://github.com/github/gh-aw/actions/runs/31113572782> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.2 AIC · ⊞ 8.3K · ◷