fix(USE-001): add standardized error codes to pr_review_buffer and set_issue_type handlers#41692
Conversation
…t_issue_type handlers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR updates two actions/setup/js safe-output handlers to conform with USE-001 by ensuring thrown Error messages begin with standardized error-code prefixes from error_codes.cjs, matching the existing convention used across sibling handlers.
Changes:
pr_review_buffer.cjs: prefixes the non-transient “capture PR review state” exception withERR_API:.set_issue_type.cjs: prefixes the invalidintentMetadata.confidenceexception withERR_VALIDATION:.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/pr_review_buffer.cjs | Imports ERR_API and prefixes the non-transient execution-state capture throw to satisfy standardized error-code requirements. |
| actions/setup/js/set_issue_type.cjs | Imports ERR_VALIDATION and prefixes the invalid-confidence throw to satisfy standardized error-code requirements. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41692 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.
Review: APPROVE
Both changes are correct, minimal, and follow the established codebase pattern.
ERR_APIis the right code for a non-transient GitHub REST API failure infetchPullRequestReviewState.ERR_VALIDATIONis the right code for an invalid input value in the confidence switch.
Each file now has exactly one throw new Error and it is properly prefixed. No logic changes, no regression risk.
🔎 Code quality review by PR Code Quality Reviewer · 49.1 AIC · ⌖ 6.84 AIC · ⊞ 5.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — commenting with observations (no blocking issues).
📋 Key Themes & Highlights
Key Themes
- Correct fix, right error codes:
ERR_APIfor a GitHub API failure inpr_review_buffer.cjsandERR_VALIDATIONfor a bad input value inset_issue_type.cjsboth match theerror_codes.cjstaxonomy precisely. - Pattern consistency: Both changes faithfully follow the import-and-prefix pattern already used across sibling handlers — no surprises.
- Missing regression tests: Neither changed error path is exercised by the existing test suites, so a future editor could silently drop a prefix and only the daily checker would notice. See inline comments for suggested tests.
Positive Highlights
- ✅ Minimal, surgical changes — exactly 2 lines of production code altered per file
- ✅ PR description is clear and self-contained with before/after snippets
- ✅ Root cause (two non-compliant throws) is fully addressed, not just symptomised
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 42.9 AIC · ⌖ 7.16 AIC · ⊞ 6.5K
| break; | ||
| default: | ||
| throw new Error(`Invalid confidence ${JSON.stringify(intentMetadata.confidence)}. Expected one of: LOW, MEDIUM, HIGH.`); | ||
| throw new Error(`${ERR_VALIDATION}: Invalid confidence ${JSON.stringify(intentMetadata.confidence)}. Expected one of: LOW, MEDIUM, HIGH.`); |
There was a problem hiding this comment.
[/tdd] This fixed error path has no regression test — a future refactor could silently drop the ERR_VALIDATION: prefix and the conformance checker would only catch it in the next daily run.
💡 Suggested test
it("throws ERR_VALIDATION for an unrecognised confidence value", () => {
const intentMetadata = { confidence: "UNKNOWN" };
expect(() => toRestIssueIntentMetadata(intentMetadata))
.toThrow(/^ERR_VALIDATION:/);
});This keeps the conformance guarantee self-verifying and avoids another USE-001 regression.
| } catch (error) { | ||
| if (!isTransientError(error)) { | ||
| throw new Error(`Failed to capture ${phase} PR review state for #${pullRequestNumber}: ${getErrorMessage(error)} (non-transient)`, { cause: error }); | ||
| throw new Error(`${ERR_API}: Failed to capture ${phase} PR review state for #${pullRequestNumber}: ${getErrorMessage(error)} (non-transient)`, { cause: error }); |
There was a problem hiding this comment.
[/tdd] The non-transient error branch (the one just fixed) isn't exercised by any test in pr_review_buffer.test.cjs — so the ERR_API: prefix could regress silently.
💡 Suggested test
it("fetchReviewStateBestEffort rethrows non-transient errors with ERR_API prefix", async () => {
const nonTransientErr = new Error("quota exceeded");
mockFetchPullRequestReviewState.mockRejectedValue(nonTransientErr);
mockIsTransientError.mockReturnValue(false);
await expect(buffer.fetchReviewStateBestEffort(repoParts, 123, "before"))
.rejects.toThrow(/^ERR_API:/);
});Pairs naturally with the existing transient-error path test to give full branch coverage.
Two safe-output handlers threw errors without standardized error code prefixes, causing USE-001 conformance failures in the daily safe-outputs checker.
Changes
actions/setup/js/pr_review_buffer.cjs— importERR_APIfromerror_codes.cjs; prefix the non-transient execution-state capture failure withERR_API:actions/setup/js/set_issue_type.cjs— importERR_VALIDATIONfromerror_codes.cjs; prefix the invalidconfidencevalue throw withERR_VALIDATION:Error codes follow the existing
error_codes.cjsregistry and the same import/prefix pattern already used across sibling handlers.