Clarify fallback issue paths for manual PR creation#56572
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
| > [!TIP] | ||
| > **Your pull request is ready to create! 🎉 ✅** | ||
| > | ||
| > Everything is OK—the changes have been pushed to branch `{branch_name}`. Create the pull request when you are ready. |
There was a problem hiding this comment.
@copilot This should also explain that the PR changes some protected files, which the user needs to review
There was a problem hiding this comment.
Updated the permission-denied fallback callout to ask users to review the changes, including any protected files, before creating the PR. Commit: c3d474a.
|
Lean already. Ship. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 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.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No actionable changed-line issues found in this PR.
Review notes
The template reordering is consistent across both fallback paths, the protected-files messaging is still present where it matters, and the regression test covers the new body ordering well enough to catch accidental reversions. I did not find a correctness, security, or maintainability problem in the changed lines that justifies blocking this merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 2.74 AIC · ⌖ 7.31 AIC · ⊞ 4.6K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Clarifies manual pull-request creation paths in fallback issues.
Changes:
- Adds prominent creation links and reassuring guidance.
- Reorders protected-file and permission details.
- Tests protected-file fallback ordering.
Show a summary per file
| File | Description |
|---|---|
actions/setup/md/pr_permission_denied_fallback.md |
Restructures permission fallback guidance. |
actions/setup/md/manifest_protection_create_pr_fallback.md |
Promotes the protected-file recovery path. |
actions/setup/js/create_pull_request.test.cjs |
Verifies protected-file body ordering. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| > | ||
| > **[Create the pull request]({create_pr_url})** | ||
| > | ||
| > The original pull request description is below. |
There was a problem hiding this comment.
Fixed the permission-denied fallback to pass the footer-free main body and footer separately, placing the permission guidance before the footer. Added ordering regression coverage. Commit: $(git rev-parse --short HEAD).
There was a problem hiding this comment.
Reviewed with clarify mode (documentation/copy changes only).
The restructuring is a clear UX improvement:
- Front-loading a
[!TIP]callout with a prominent CTA reduces cognitive friction for users who just need to create their PR. - Downgrading
[!WARNING]→[!NOTE]for protected files is appropriate — the content is informational, not a hard blocker. - The
pr_permission_denied_fallback.mdNOTE is now tighter and avoids redundancy. - Tests correctly assert both content presence and relative ordering.
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 13.8 AIC · ⌖ 10.6 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting on test coverage gap and template asymmetry.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: the new ordering assertions cover the protected-file fallback but not the permission-denied fallback (
pr_permission_denied_fallback.md). - Template asymmetry: the
> [!NOTE]block inpr_permission_denied_fallback.mddrops the "changes are pushed and ready" reassurance that the parallel template retains.
Positive Highlights
- ✅ Promoting the PR link to a top-level
[!TIP]callout is a clear UX improvement. - ✅ Downgrading
[!WARNING]→[!NOTE]reduces alarm fatigue for a routine situation. - ✅ Good symmetry applied across both fallback templates.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 23.3 AIC · ⌖ 10.6 AIC · ⊞ 7.6K
Comment /matt to run again
| expect(createCall.body).toContain("/compare/main..."); | ||
| expect(createCall.body).not.toContain("gh run download"); | ||
| expect(createCall.body).not.toContain("git am --3way"); | ||
| expect(createCall.body).toContain("Your pull request is ready to create! 🎉 ✅"); |
There was a problem hiding this comment.
[/tdd] The regression test covers the protected-file fallback body ordering, but there is no parallel test for pr_permission_denied_fallback.md — leaving that template's new heading order unverified.
💡 Suggested approach
Add equivalent indexOf-ordering assertions in the permission-denied fallback test, similar to the four lines added here:
expect(createCall.body).toContain("Your pull request is ready to create! 🎉 ✅");
expect(createCall.body.indexOf("Your pull request is ready to create! 🎉 ✅"))
.toBeLessThan(createCall.body.indexOf(/* original body content */));This ensures both fallback paths stay consistent as the templates evolve.
@copilot please address this.
There was a problem hiding this comment.
Added parallel permission-denied fallback assertions covering tip → original body → permission details → footer ordering. Commit: $(git rev-parse --short HEAD).
|
|
||
| {body} | ||
|
|
||
| --- |
There was a problem hiding this comment.
[/codebase-design] The > [!NOTE] block now loses the context that the changes were pushed and the user should review them. The manifest_protection_create_pr_fallback.md counterpart retains "Please review the protected files, then create the pull request when you are ready", but this template omits equivalent guidance.
💡 Suggested fix
Add a brief reassurance sentence:
> [!NOTE]
> GitHub Actions is not permitted to create or approve pull requests in this repository.
> The changes have been pushed to branch `{branch_name}` and are ready to review.This keeps the two templates symmetric and ensures users understand the state of their changes.
@copilot please address this.
There was a problem hiding this comment.
Added explicit guidance that the pushed changes are ready for review in the permission-denied fallback note. Commit: $(git rev-parse --short HEAD).
✅ Test Quality Sentinel: ApprovePR #56572 — Regression test additions to verify fallback issue body ordering Analysis SummaryThis PR modifies markdown templates for fallback issues and enhances existing test coverage with regression assertions. No new test functions were added—only strategic additions to verify critical behavior. Test ModificationsFile: 4 Assertion Lines Added:
Quality Assessment
Verdict✅ APPROVE — Regression test assertions are well-crafted and provide concrete validation of the new fallback issue structure. The modifications ensure the critical UX improvement (prominent recovery path) cannot regress. Analysis Note: Pre-fetched test data was unavailable; analysis performed against live repository. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: Score N/A—Regression test only (4 assertion lines added to verify fallback issue body ordering). No new test functions. Classification: High-value design test with no violations. Regression assertions ensure critical UX structure (prominent recovery path) cannot break.
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
Category: docs | Risk: low | Score: 45/100 (impact 20/50, urgency 10/30, quality 15/20) | Action: auto_merge | Batch: workflow-ux
|
|
Category: docs | Risk: low | Score: 45/100 (impact 20/50, urgency 10/30, quality 15/20) | Action: auto_merge | Batch: workflow-ux
|
|
@copilot There is still unresolved review feedback on this PR. Please cover the permission-denied fallback path with the same ordering/regression assertions or explain why the asymmetry is intentional, refresh the branch if needed, and then run the Current blocking feedback, newest first:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the permission fallback asymmetry: main body and footer are now separate placeholders, permission guidance appears before the footer, and ordering regression assertions cover the permission path. Commit: |
Protected-file fallback issues buried the PR creation link below the original description and presented it as a warning, making the intended next step unclear.
Prominent recovery path
Readable issue structure
Run: https://github.com/github/gh-aw/actions/runs/33195749972
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.