Skip to content

Prevent rejected issue-intent closes from bypassing review - #64778

Merged
pelikhan merged 5 commits into
mainfrom
copilot/close-issue-failures-fallback
Oct 1, 2026
Merged

pelikhan merged 5 commits into
mainfrom
copilot/close-issue-failures-fallback

Conversation

Copilot AI commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

A rejected issue-intent close previously fell through to a plain issue update, potentially closing the issue without the configured review. Duplicate-close validation failures could trigger this bypass.

  • Fallback boundary: Fail the item when the intent request is rejected or otherwise fails. Retain the legacy fallback only for HTTP 404 or 501, which may indicate the intent endpoint is unavailable.
  • Regression coverage: Cover rejected requests and endpoint-unavailable fallback.

Canonical-issue metadata for duplicate intents and the failing markAsDuplicate mutation remain separate follow-ups.


pr-sous-chef

Branch refresh requested by PR Sous Chef in run https://github.com/github/gh-aw/actions/runs/36880336135

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 33.6 AIC · ⌖ 8.56 AIC · ⊞ 9.2K · ◷
Comment /souschef to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix intent-path failures in close-issue Prevent rejected issue-intent closes from bypassing review Oct 1, 2026
Copilot AI requested a review from pelikhan October 1, 2026 14:20
@pelikhan
pelikhan marked this pull request as ready for review October 1, 2026 14:25
Copilot AI balanced review requested due to automatic review settings October 1, 2026 14:25
@pelikhan

pelikhan commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

@copilot update spec

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

No GitHub write emitted yet; probing CLI invocation path only.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

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.

Generated by Ponytail Reviewer for #64778

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The fallback boundary matches the stated security requirement and is covered by focused regression tests.

Review effort: Balanced
Findings: None

What changed in this PR

Prevents failed issue-intent closes from bypassing review through the legacy close path.

Changes:

  • Restricts legacy fallback to HTTP 404/501 responses.
  • Adds regression coverage for rejected and unavailable intent endpoints.
File Description
actions/​setup/​js/​close_issue.cjs Fails closed for non-availability errors.
actions/​setup/​js/​close_issue.test.cjs Tests rejection and fallback behavior.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Comment Memory

reviewed_at: 2026-10-01T14:28:41Z
review_event: REQUEST_CHANGES
top_themes:
  - error-status normalization on compatibility fallback
files_reviewed:
  - actions/setup/js/close_issue.cjs
  - actions/setup/js/close_issue.test.cjs
comment_count: 1

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 29.5 AIC · ⌖ 7.14 AIC · ⊞ 20.2K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

The compatibility fallback is still too brittle: it only recognizes top-level error.status, so wrapped 404/501 request failures can bypass the legacy path and break closes on the servers this patch is meant to protect.

Blocking theme

Normalize the HTTP status check to inspect response.status before error.status, and add a regression test for the wrapped-error shape so the fallback boundary stays pinned.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 29.5 AIC · ⌖ 7.14 AIC · ⊞ 20.2K
Comment /review to run again

Comment thread actions/setup/js/close_issue.cjs Outdated
});
return issue;
} catch (error) {
const status = error && typeof error === "object" && "status" in error ? error.status : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks error.status, so wrapped 404/501 request failures will rethrow instead of taking the legacy fallback.

💡 Why this matters

Elsewhere in this repo we normalize HTTP failures with response.status before falling back to error.status because some request wrappers only populate the nested field. If the intent endpoint is unavailable in that shape, this change regresses closes back into hard failures on the older or partially deployed servers it is meant to protect. Please normalize both fields here and add a regression test for a wrapped 404 response.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4841a79: the close handler now checks response.status before error.status, and the regression test covers a wrapped 404 fallback.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — no actionable issues found.

📋 Analysis

Root cause fix

The bug (rejected issue-intent closes silently falling back to a plain issues.update) is fixed at its root: the catch block in closeIssue (close_issue.cjs:183-187) now re-throws unless the error status is 404 or 501, which are the only cases that genuinely indicate the intent endpoint is unavailable. All other statuses (e.g. validation rejection, server error) now correctly propagate and fail the operation instead of silently bypassing review via the legacy path.

Regression coverage

The two new it.each blocks in close_issue.test.cjs exercise both branches precisely:

  • 403/422/500 → confirms update is never called and the original error message surfaces in result.error
  • 404/501 → confirms the legacy fallback still fires

Test names read as clear specifications, and each asserts both the outcome (result.success) and the side effect (updateCalled), which is exactly the kind of regression-proofing /diagnosing-bugs calls for — the fix is paired with tests that would have caught the original bypass.

Minor, non-blocking observation

The inline status extraction (error && typeof error === "object" && "status" in error ? error.status : undefined) duplicates a pattern that several other files in this codebase (daily_aic_scan.cjs, set_issue_type.cjs, update_pull_request.cjs) have already factored into a shared getErrorStatus(error) helper. Not blocking — the inline form matches other recent call sites (approve_workflow_run.cjs, create_pull_request.cjs) too — but worth considering for a future cleanup pass if this pattern keeps spreading.

Positive highlights

  • ✅ Narrow, surgical fix scoped exactly to the reported bypass
  • ✅ Clear PR description with explicit fallback-boundary rationale
  • ✅ No unrelated changes

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 39.5 AIC · ⌖ 14.3 AIC · ⊞ 10K
Comment /matt to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

@copilot update spec

Updated the safe-outputs specification to document fail-closed issue-intent close behavior, with legacy fallback limited to HTTP 404/501. Published in commit 6f580df.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

pr-sous-chef
@copilot this PR still needs forward progress before a maintainer can investigate efficiently.

Please address the outstanding review feedback, then run the pr-finisher skill.

Outstanding review feedback

Failed checks to address

A branch refresh was also requested by PR Sous Chef in this run.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 24.3 AIC · ⌖ 8.45 AIC · ⊞ 9.7K · ◷
Comment /souschef to run again

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

pr-sous-chef
@copilot this PR still needs forward progress before a maintainer can investigate efficiently.

Please address the outstanding review feedback, branch refresh, and then run the pr-finisher skill.

Outstanding review feedback

A branch refresh was also requested by PR Sous Chef in this run.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 33.6 AIC · ⌖ 8.56 AIC · ⊞ 9.2K · ◷
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

pr-sous-chef
@copilot this PR still needs forward progress before a maintainer can investigate efficiently.
...

Addressed in 4841a79: wrapped HTTP status handling and its 404 regression test are committed and pushed. The cited workflow run completed without failed jobs; the review finding was the blocker. The branch is current with main; local checks pass. The PR remains blocked pending maintainer review, and CI must be re-triggered after this push.

Copilot AI commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

pr-sous-chef
@copilot this PR still needs forward progress before a maintainer can investigate efficiently.
...

The review finding was fixed and answered in 4841a79, including a wrapped-404 regression test. I fetched main; this branch is current (0 commits behind), so no refresh merge was needed. The review thread is outdated but remains unresolved; this environment has no thread-resolution control.

@pelikhan

pelikhan commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

@copilot add logging (privacy preserving)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

@copilot add logging (privacy preserving)

Added attempt, success, rejection, and fallback diagnostics in actions/setup/js/close_issue.cjs. The logs avoid issue content and raw exception text; tests confirm the emitted diagnostics are sanitized. Committed as f183092.

pr-sous-chef
@copilot this PR still needs forward progress before a maintainer can investigate efficiently.
...

The branch-refresh check confirmed the branch is current with main, so no merge was needed. The prior review finding was fixed in 4841a79; run 36876205785 completed with no failed jobs. The final local validation gate passed, including all 76 issue-close tests, and privacy-safe logging is pushed in f183092. CI for the new head still needs a maintainer-triggered run.

@pelikhan
pelikhan merged commit a74f516 into main Oct 1, 2026
3 checks passed
@pelikhan
pelikhan deleted the copilot/close-issue-failures-fallback branch October 1, 2026 15:56
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.90.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

close-issue: intent-path failures silently fall back to an unreviewed legacy close

4 participants