Repository navigation
Diagnose Copilot assignment credentials only after failures - #67409
Conversation
Refs #67374. Validate the selected user credential before assignment, retain actionable errors without retrying lower-priority tokens, and preserve staging and label gating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ 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 for PR #67409: the PR does not carry the 'implementation' label and has 0 new lines of code in business logic directories (3 files changed, threshold 100). Source: /tmp/gh-aw/agent/adr-prefetch-summary.json, no custom .design-gate.yml present.
|
|
✅ PR Code Quality Reviewer completed the code quality review. No GitHub review was submitted because safeoutputs review-write commands were denied with a non-interactive permission error ('could not request permission from user'). Review findings are reported in the agent response instead.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
There was a problem hiding this comment.
🟡 Changes recommended
Required-label gating can bypass preflight diagnostics, and per-handler App credentials are misidentified.
2 open findings
What changed in this PR
Adds credential preflight diagnostics for Copilot assignment without changing token precedence or assignment gating.
Changes:
- Adds cached, timeout-bounded credential validation.
- Preserves assignment errors while avoiding failed-preflight comments.
- Adds regression tests and authentication documentation.
| File | Description |
|---|---|
actions/setup/js/assign_to_agent.cjs |
Implements credential preflight and diagnostics. |
actions/setup/js/assign_to_agent.test.cjs |
Covers credential precedence and failure behavior. |
docs/src/content/docs/reference/copilot-cloud-agent.mdx |
Documents preflight scope and remediation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed with Impeccable harden + audit modes (bug-fix/error-state change — credential preflight diagnostics).
Found one blocking correctness issue: the new validateCredential() preflight is ordered after checkRequiredFilter, so handlers configured with required_labels can still surface a raw, unwrapped auth error before the new actionable diagnostics ever run — undermining the PR's core goal for exactly the configuration most likely to need it. See inline comment for details and a suggested fix.
Everything else (lazy/cached preflight, ghs_ rejection without an API call, no token-value leakage, preserved ignore-if-error/staged-mode behavior, docs) looks solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 69.1 AIC · ⌖ 14.2 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design on the new credential-preflight path in assign_to_agent.cjs. The change is well-scoped: lazy, cached GET /user check, clear error naming, token redaction in assertions, and solid new test coverage (precedence, 401 vs non-auth status codes, staged/filtered skip, shared-cache-across-messages). Two non-blocking findings below.
📋 Key Themes & Highlights
Key Themes
- Unsafe property access on caught error (
error.nameat line 611): if any step in thetryblock throws a non-Errorvalue, this currently throws a freshTypeErrorinstead of surfacing the real failure — same defensive pattern used a few lines earlier forerror.statusshould be applied here too. - Pre-existing filter duplication now bracketing the new preflight:
checkRequiredFilteralready ran twice per assignment before this PR; the newvalidateCredential()call sits between the two checks, tripling relevant API calls per eligible item. Not introduced by this PR, but a short comment explaining the ordering would help future readers avoid conflating the two.
Positive Highlights
- ✅ Token value never logged or included in thrown errors (verified by
rejects.not.toThrow(token)test) - ✅ Validation is cached per handler invocation and correctly skipped for staged/filtered assignments
- ✅
ignore-if-errorinteraction with 401 vs 403/404/429/500 is explicitly tested — good edge-case coverage per/tdd - ✅ Docs update accurately describes the new behavior and its limits (doesn't claim to verify repo write permissions or Copilot availability)
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 147.6 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 3b61c35
|
…nt-credential-preflight Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Remove credential preflight calls from the successful assignment path. Enrich failed assignment diagnostics without additional probes or assignment retries, and retain HTTP 401 classification and existing label gating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
Move failure-only credential guidance into actions/setup/md and render it with the existing message-template helpers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Refs #67374; the issue assessment records dedicated assignment-client HTTP 401 failures while ordinary safe outputs succeed. The selected assignment credential still requires an authorized owner's repair.
Per user direction, diagnostics are failure-only: no
GET /userpreflight, no extra diagnostic requests on the happy path, and no assignment retries or lower-priority credential fallback. Successful assignment uses its original lookup/assignment flow with one assignment POST.ignore-if-error, and best-effort failure comments.Validation
make fmt-cjsandmake lint-cjspassed with repository-pinned stable Prettier.make agent-report-progresspassed build, formatting/lint, schema freshness, JavaScript typecheck and impacted tests (133 passed, 2 skipped). It reported three pre-existing warnings in the prior branch'shandle_agent_failure.cjschanges, unrelated to this revision. No Go/workflow/compiler changes required recompilation.No credentials changed and no Actions runs dispatched. This PR fixes diagnostics, not the operational requirement to configure a valid user token and observe successful assignment in a subsequent normally scheduled run. Do not close #67374 solely on merge or a green Actions conclusion.