Suggest permissions.copilot-requests: write in agent failure issue when COPILOT_GITHUB_TOKEN is missing#38722
Conversation
…gest permissions.copilot-requests:write Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38722 does not have the implementation label (has_implementation_label=false) and has 0 new lines of code in business logic directories (≤100 threshold, requires_adr_by_default_volume=false). |
There was a problem hiding this comment.
Clean refactor that correctly extracts duplicated inline string-building into a named, testable function. No blocking bugs found. Four non-blocking issues noted inline.
### Summary of findings
Medium
engineIdcomparison at line 1889 does not normalize to lowercase internally; the function is now exported and any future caller skipping.toLowerCase()will silently miss the Copilot suggestion.- User-facing copy at line 1891 says "workflow frontmatter" — not a GitHub Actions concept; users searching the docs for this term will find nothing.
Low
- Copilot test is missing an assertion for the
#github-copilot-defaultURL, the only output line unique to the Copilot branch. - Test description uses
permissions.copilot-requests:write(dot notation, no space) rather than the actual YAML formcopilot-requests: write.
🔎 Code quality review by PR Code Quality Reviewer · 400.8 AIC · ⌖ 13.3 AIC
| buildWarningAlertLine("Secret Verification Failed", "The workflow's secret validation step failed. Please check that the required secrets are configured in your repository settings.") + | ||
| "\nFor more information on configuring tokens, see: https://github.github.com/gh-aw/reference/engines/\n"; | ||
|
|
||
| if (engineId === "copilot") { |
There was a problem hiding this comment.
engineId comparison is not normalized — callers passing mixed-case will silently miss the Copilot suggestion. The function is now exported and reusable, but it places the burden on every caller to pre-lowercase the value.
💡 Suggested fix
Add a single normalization inside the function instead of relying on caller discipline:
if ((engineId || "").toLowerCase() === "copilot") {The two existing call sites in main() already pass a lowercased engineId (line 2572), so this is safe and backward-compatible. But as soon as a third call site is added — or a test passes "Copilot" — the Copilot-specific block will be silently skipped with no error or warning, and users will see only the generic secret-verification message.
|
|
||
| if (engineId === "copilot") { | ||
| context += | ||
| "\n**Alternative**: If your organization has a Copilot subscription, you can avoid the need for a personal access token by adding the following to your workflow frontmatter. This enables Copilot inference through the org using the built-in GitHub Actions token.\n" + |
There was a problem hiding this comment.
"workflow frontmatter" is not a GitHub Actions concept and will confuse users. Frontmatter is a static-site-generator term (YAML delimited by --- in Markdown files). GitHub Actions workflows have no such element, so users who search the docs for this term will find nothing.
💡 Suggested fix
Replace:
adding the following to your workflow frontmatter
With something like:
adding a top-level `permissions` block to your workflow file
For example:
This enables Copilot inference through the org using the built-in GitHub Actions token by adding a top-level `permissions` block to your workflow file.
This matches the language used in GitHub Actions documentation and helps users find the right place to make the change.
| const result = buildSecretVerificationContext("failed", "copilot"); | ||
| expect(result).toContain("Secret Verification Failed"); | ||
| expect(result).toContain("required secrets are configured"); | ||
| expect(result).toContain("```yaml\npermissions:\n copilot-requests: write\n```"); |
There was a problem hiding this comment.
The Copilot-specific reference URL is untested — if it were accidentally removed, all tests would still pass. The implementation appends a See: https://github.github.com/gh-aw/reference/engines/#github-copilot-default line that is unique to the Copilot branch, but no assertion checks for it.
💡 Suggested addition
expect(result).toContain("https://github.github.com/gh-aw/reference/engines/#github-copilot-default");The generic URL (/engines/) is already covered by the claude test above; the Copilot-specific anchor (#github-copilot-default) is not. Adding this closes the coverage gap and locks in the exact link that users are expected to follow.
| expect(result).not.toContain("copilot-requests"); | ||
| }); | ||
|
|
||
| it("returns copilot-specific message with permissions.copilot-requests:write suggestion when verification failed", () => { |
There was a problem hiding this comment.
Test description notation does not match the YAML it describes. permissions.copilot-requests:write (dot-delimited, no space after colon) is not valid YAML syntax; the actual permission key-value pair emitted is copilot-requests: write (colon + space). The mismatch is visible in every CI run and misleads whoever reads the test output.
💡 Suggested fix
Change:
"returns copilot-specific message with permissions.copilot-requests:write suggestion when verification failed"
To:
"returns copilot-specific message with copilot-requests: write permissions suggestion when verification failed"
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher pass and pushed follow-up fixes in 33bfd58.
Local validation passed: CI on this new HEAD is still unverified until a maintainer re-triggers workflows. |
permissions.copilot-requests: write in agent failure issue when COPILOT_GITHUB_TOKEN is missing
When the Copilot engine's secret verification fails, the failure issue shows a generic "configure your secrets" message — missing a key alternative: org-level Copilot inference via
permissions.copilot-requests: write, which requires no PAT at all.Changes
handle_agent_failure.cjs: Extracts inlinesecret_verification_contextternary intobuildSecretVerificationContext(secretVerificationResult, engineId). For Copilot, appends an additional paragraph pointing users topermissions.copilot-requests: writeas a PAT-free alternative, presented as a fenced YAML code block with valid syntax; other engines keep the existing generic message. The Copilot branch now normalizesengineIdcase internally.handle_agent_failure.test.cjs: Adds tests covering the no-failure (empty), non-copilot generic, and copilot-specific branches, including assertions for the Copilot-specific docs URL and mixed-caseengineIdhandling.Example of the added guidance for the
copilotengine: