Standardize Copilot agent-assignment permission guidance#46753
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Standardizes Copilot assignment permission and remediation guidance across runtime errors, summaries, templates, documentation samples, and tests.
Changes:
- Moves assignment guidance into reusable Markdown templates.
- Aligns PAT permissions, unsupported token guidance, and reference links.
- Expands template-rendering tests and refreshes a generated workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/workflows/test-assign-to-agent.md |
Updates sample authentication guidance. |
actions/setup/md/copilot_assignment_permission_requirements.md |
Adds summary guidance template. |
actions/setup/md/copilot_assignment_permission_references.md |
Adds reference links template. |
actions/setup/md/copilot_assignment_permission_error.md |
Adds log error template. |
actions/setup/md/copilot_assignment_errors_context.md |
Adds failure context template. |
actions/setup/md/assign_copilot_to_created_issues_failure.md |
Standardizes created-issue failure remediation. |
actions/setup/js/test_prompt_templates.js |
Adds test template synchronization helper. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests rendered failure guidance. |
actions/setup/js/handle_agent_failure.cjs |
Renders assignment errors from templates. |
actions/setup/js/assign_agent_helpers.test.cjs |
Updates permission-message assertions. |
actions/setup/js/assign_agent_helpers.cjs |
Replaces inline guidance with templates. |
.github/workflows/daily-vulnhunter-scan.lock.yml |
Refreshes generated runtime versions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/handle_agent_failure.test.cjs:1400
- This second process-wide environment assignment also needs restoration. Otherwise every subsequent test resolves prompts from
runtimePromptsDir, ignoring its ownRUNNER_TEMP; notably, the missing-template coverage at line 3555 can no longer exercise the missing-file path.
process.env.GH_AW_PROMPTS_DIR = runtimePromptsDir;
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Medium
| }); | ||
|
|
||
| it("renders assignment failures with token guidance docs", () => { | ||
| process.env.GH_AW_PROMPTS_DIR = runtimePromptsDir; |
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { syncRuntimePromptTemplates } from "./test_prompt_templates.js"; | ||
|
|
||
| syncRuntimePromptTemplates(import.meta.url); |
|
|
||
| **Required token options** | ||
| - **Fine-grained personal access token** — Read access to **metadata** and read/write access to **actions**, **contents**, **issues**, and **pull requests** | ||
| - **Classic personal access token** — **`repo`** scope |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46753 does not have the 'implementation' label and has only 3 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes look clean and consistent. The refactoring extracts hardcoded permission error strings into reusable template files, standardizes the messaging across all assignment failure paths, and accurately documents that GitHub App installation tokens are not supported for Copilot assignment. Tests are updated to match the new template content. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 23.7 AIC · ⌖ 6.98 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
|
There was a problem hiding this comment.
Skills-Based Review\n\nApplied /tdd requesting changes on one test correctness issue.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80.8 AIC · ⌖ 4.43 AIC · ⊞ 6.7K
Comment /matt to run again
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { syncRuntimePromptTemplates } from "./test_prompt_templates.js"; | ||
|
|
||
| syncRuntimePromptTemplates(import.meta.url); |
There was a problem hiding this comment.
[/tdd] The return value of syncRuntimePromptTemplates is discarded here, so GH_AW_PROMPTS_DIR is never set. Unlike handle_agent_failure.test.cjs which captures runtimePromptsDir and sets the env var before each template-exercising test, this file never sets process.env.GH_AW_PROMPTS_DIR. If RUNNER_TEMP is unset, getPromptPath() will throw and every test that indirectly calls logPermissionError or generatePermissionErrorSummary will fail.\n\n
💡 Suggested fix
\n\njs\nconst { runtimePromptsDir } = syncRuntimePromptTemplates(import.meta.url);\nprocess.env.GH_AW_PROMPTS_DIR = runtimePromptsDir;\n\n\nOr set/restore in beforeEach/afterEach around just the permission-error tests, as done in handle_agent_failure.test.cjs.\n\n
There was a problem hiding this comment.
❌ Request Changes — 2 correctness bugs must be fixed
Blocking issues found:
-
PR failures mislabeled as Issue () — drops (type field), so every PR assignment failure renders as "Issue #N" instead of "PR #N". The sibling does this correctly and this regresses that behavior.
-
No fallback when templates are missing () — and now throw unhandled exceptions if / is unset or the template file doesn't exist. The previous hard-coded strings were unconditionally safe; the new template path has zero fallback, silencing all permission error output in production when templates fail to load.
Non-blocking observations
- is set inside test bodies without cleanup — can bleed into subsequent tests and mask real failures. (See comment on .)
- calls but never sets , relying on being set at module load time — fragile in environments where differs between module init and test execution.
🔎 Code quality review by PR Code Quality Reviewer · 112.9 AIC · ⌖ 4.73 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/handle_agent_failure.cjs:2226
PR assignment failures always render as "Issue #N": parts[0] (the type field: "issue" or "pr") is silently dropped, so every PR failure is mislabeled as an issue in the context block.
<details>
<summary>💡 Suggested fix</summary>
if (parts.length >= 4) {
const type = parts[0];
const number = parts[1];
const error = parts.slice(3).join(':');
issueList += `- ${type === 'issue' ? 'Issue' : 'PR'} #${number}: ${error}\n`;
}The sibling buildAssignmentErrorsContext al…
actions/setup/js/assign_agent_helpers.cjs:435
Template read failure in logPermissionError silently swallows all error output: if getPromptPath throws (e.g. RUNNER_TEMP unset, GH_AW_PROMPTS_DIR unset) or renderTemplateFromFile cannot read the file, the exception propagates uncaught and the user sees nothing — not even the original "Insufficient permissions" line.
<details>
<summary>💡 Suggested fix</summary>
Wrap the template render in a try/catch and fall back to the essential inline message:
function logPermissionEr…
</details>
<details><summary>actions/setup/js/handle_agent_failure.test.cjs:1401</summary>
**`process.env.GH_AW_PROMPTS_DIR` set in test body but never cleaned up**: tests that run after this one will inherit the fixture prompts dir, potentially masking missing-template bugs in subsequent test cases.
<details>
<summary>💡 Suggested fix</summary>
Use `afterEach` to restore the original value:
```js
let originalPromptsDir;
beforeEach(() => { originalPromptsDir = process.env.GH_AW_PROMPTS_DIR; });
afterEach(() => {
if (originalPromptsDir === undefined) delete process.env.GH_AW_PROM…
</details>
|
@copilot run pr-finisher skill |
|
@copilot please run the Unresolved review feedback:
Failed checks:
Run: https://github.com/github/gh-aw/actions/runs/29731780198 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in fcc498e. I fixed the prompt-template test isolation, reconciled the PAT guidance in |
|
🎉 This pull request is included in a new release. Release: |
Agent-assignment failures were surfacing inconsistent permission and remediation text across CLI/workflow outputs. This update aligns the messaging so assignment failures clearly describe the missing capability, the supported token types, and the expected remediation path.
Shared assignment error messaging
assign_agent_helpers.cjsGH_AW_AGENT_TOKENmetadata: read,actions|contents|issues|pull requests: write)reposcopeFailure summaries and generated remediation
handle_agent_failure.cjsso follow-up issue/comment content points to the same remediation guidanceDocs/sample alignment
test-assign-to-agentworkflow note so the embedded guidance matches current supported auth behaviorFocused test coverage
Example of the normalized guidance shape: