Standardize USE-001 error codes in add_labels and dismiss_pull_request_review - #51338
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
add_labels and dismiss_pull_request_review
PR Triage
Standardizes USE-001 error codes in two safe-output handlers. Small, isolated, well-tested change.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
There was a problem hiding this comment.
Pull request overview
Standardizes safe-output runtime errors with E099 for label node-ID resolution and review-fetch failures.
Changes:
- Prefixes both affected errors with
SAFE_OUTPUT_E099. - Adds focused regression assertions for both failure paths.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_labels.cjs |
Adds E099 to missing node-ID errors. |
actions/setup/js/add_labels.test.cjs |
Tests the standardized error code. |
actions/setup/js/dismiss_pull_request_review.cjs |
Wraps review-fetch failures with contextual E099 errors. |
actions/setup/js/dismiss_pull_request_review.test.cjs |
Verifies E099 on non-404 failures. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are minimal, correct, and well-tested. Both files now consistently prepend E099 to their error messages using the shared error_codes.cjs constant. The dismiss_pull_request_review.cjs change also improves error handling by using getErrorMessage() instead of mutating the original error object. Tests are updated to assert the error code is present.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.1 AIC · ⌖ 6.82 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — approving with one minor suggestion.
📋 Summary
Positive Highlights
- ✅ Consistent use of the shared
SAFE_OUTPUT_E099constant — no magic strings - ✅
dismiss_pull_request_reviewfix is strictly better: replaces a fragile in-place message mutation with a clean, descriptivenew Error(...) - ✅ Regression tests are well-scoped: they assert both the error code prefix and a human-readable fragment, which is the right level of specificity
Minor Suggestion
One inline comment: the new Error(...) in dismiss_pull_request_review.cjs discards the original stack. Adding { cause: getReviewError } would preserve it at no cost.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.7 AIC · ⌖ 6.53 AIC · ⊞ 7.1K
Comment /matt to run again
| getReviewError.message = `Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ` + getReviewError.message; | ||
| } | ||
| throw getReviewError; | ||
| throw new Error(`${SAFE_OUTPUT_E099}: Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ${getErrorMessage(getReviewError)}`); |
There was a problem hiding this comment.
[/codebase-design] The new Error(...) wrapper discards the original error's stack trace, which can make diagnosing production failures harder.
💡 Consider preserving the cause
Node 16.9+ supports the cause option on Error:
throw new Error(
`${SAFE_OUTPUT_E099}: Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ${getErrorMessage(getReviewError)}`,
{ cause: getReviewError }
);This keeps the standardized message and the original stack accessible via err.cause, useful when the error is logged or re-caught upstream.
@copilot please address this.
There was a problem hiding this comment.
Addressed — the new Error(...) now includes { cause: getReviewError } to preserve the original stack.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
|
|
@copilot run pr-finisher skill |
|
@copilot Quick triage nudge for this PR. Please review the remaining maintainer-facing follow-up below, refresh the branch if GitHub can update it cleanly, and run the
Run: https://github.com/github/gh-aw/actions/runs/31263551516
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
USE-001 flagged two safe-output handlers that emitted unstructured errors, breaking consistency with the shared error-code catalog and making automated triage harder. This change aligns both handlers with existing
E###-prefixed error behavior.add_labels: code-prefix missing GraphQL node ID failureSAFE_OUTPUT_E099fromerror_codes.cjs.E099.dismiss_pull_request_review: wrap rethrow with standardized codeSAFE_OUTPUT_E099.throw getReviewError(after context mutation) with a freshErrorcarryingE099and preserved context + original message.Regression coverage
E099is present for:node_idinadd_labelsissue-intent pathgetReviewfailure indismiss_pull_request_review