Prevent rejected issue-intent closes from bypassing review - #64778
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update spec |
|
✅ PR Code Quality Reviewer completed the code quality review. No GitHub write emitted yet; probing CLI invocation path only.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ 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 happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
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.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
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
| }); | ||
| return issue; | ||
| } catch (error) { | ||
| const status = error && typeof error === "object" && "status" in error ? error.status : undefined; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed in 4841a79: the close handler now checks response.status before error.status, and the regression test covers a wrapped 404 fallback.
There was a problem hiding this comment.
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→ confirmsupdateis never called and the original error message surfaces inresult.error404/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>
Updated the safe-outputs specification to document fail-closed issue-intent close behavior, with legacy fallback limited to HTTP 404/501. Published in commit |
|
Please address the outstanding review feedback, then run the Outstanding review feedbackFailed checks to address
A branch refresh was also requested by PR Sous Chef in this run.
|
|
Please address the outstanding review feedback, branch refresh, and then run the Outstanding review feedbackA branch refresh was also requested by PR Sous Chef in this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
The review finding was fixed and answered in |
|
@copilot add logging (privacy preserving) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added attempt, success, rejection, and fallback diagnostics in
The branch-refresh check confirmed the branch is current with |
|
🎉 This pull request is included in a new release. Release: |
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.
Canonical-issue metadata for duplicate intents and the failing
markAsDuplicatemutation remain separate follow-ups.pr-sous-chefBranch refresh requested by PR Sous Chef in run https://github.com/github/gh-aw/actions/runs/36880336135