Make add_comment wildcard target misses non-fatal in safe outputs#37041
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>
add_comment wildcard target misses non-fatal in safe outputs
There was a problem hiding this comment.
Pull request overview
This PR adjusts the add_comment safe-output handler so that a specific target-resolution failure mode is treated as non-fatal: when target: "*" is configured but the emitted safe-output message lacks any explicit numeric target fields, the handler now returns { success: false, skipped: true } (and logs a warning) instead of hard-failing processing.
Changes:
- Updated
actions/setup/js/add_comment.cjsto detect the “wildcard target with no explicit target fields” case and return a skipped result rather than a hard failure. - Updated
actions/setup/js/add_comment.test.cjsto assertskipped: truefor that scenario.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_comment.cjs | Treat unresolved target: "*" with no explicit target fields as skip+warning (non-fatal) instead of a hard failure. |
| actions/setup/js/add_comment.test.cjs | Updates the test to expect skipped: true for the unresolved wildcard-target case. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37041 does not have the implementation label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well under the 100-line threshold). The changes are limited to shell/python skill scripts and an MCP server file, none of which fall in the configured business logic directories. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with minor suggestions on test coverage and log level.
📋 Key Themes & Highlights
Key Themes
- Narrow, correct fix: The skip-vs-fail branch is tightly scoped to
target: "*"+ no explicit field. The symmetry with the existing non-shouldFailelsebranch (which already returnsskipped: true) is clean and intentional. - Test coverage gap: The skip path is now tested, but the complementary hard-fail path (explicit field present → strict behavior preserved) is not. One extra test case closes this gap.
- Log level mismatch:
core.warningfires before the skip check, so non-fatal skips still appear as yellow warnings in the Actions log. Restructuring the log call to follow the skip decision would better communicate intent. WILDCARD_TARGET_FIELDSlist includes pre-handled fields:item_numberandissue_numberare resolved earlier byresolveSafeOutputIssueTargetand will always be null/absent at theresolveTargetfailure site. The list works correctly but silently carries dead entries — worth a clarifying comment.
Positive Highlights
- ✅ Root cause properly addressed: the exact combination of
target: "*"+ no numeric field was the intermittent failure trigger - ✅ Existing strict behavior for real target-resolution errors is structurally preserved by the early-return ordering
- ✅ Test description updated to match the new behavioral contract (
should skip (not fail)) - ✅
WILDCARD_TARGET_FIELDSconstant centralizes the six accepted field names — a good pattern to prevent future drift
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.9M · 233.7 AIC
| }); | ||
|
|
||
| it("should fail when target is '*' but no item_number provided", async () => { | ||
| it("should skip (not fail) when target is '*' but no item_number provided", async () => { |
There was a problem hiding this comment.
[/tdd] Missing regression test for the "strict behavior preserved" invariant — when target: "*" fires with an explicit PR field present, the handler should still hard-fail.
The updated test only locks in the skip path. Without a complementary test for the hard-fail path, the !hasExplicitWildcardTargetField condition can be accidentally inverted without breaking the suite.
💡 Suggested additional test
Add a test adjacent to the current one:
it("should hard-fail (not skip) when target is * but explicit field resolves nothing meaningful", async () => {
// Tests that having a recognized field still triggers hard-fail
// when resolveTarget returns shouldFail (strict behavior preserved)
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");
const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: * }); })()`);
const message = {
type: "add_comment",
pull_request_number: 42, // explicit field present — should still hard-fail
body: "Test comment",
};
const result = await handler(message, {});
expect(result.success).toBe(false);
expect(result.skipped).toBeUndefined(); // NOT a skip — explicit field was provided
});This makes the skip-vs-fail boundary explicit and protects against accidental widening of the skip condition.
| if (targetResult.shouldFail) { | ||
| const hasExplicitWildcardTargetField = WILDCARD_TARGET_FIELDS.some(field => message[field] != null); | ||
| const missingWildcardTarget = commentTarget === "*" && !hasExplicitWildcardTargetField; | ||
| core.warning(targetResult.error); |
There was a problem hiding this comment.
[/diagnose] core.warning fires unconditionally before the skip check, so skipped wildcard misses still surface as yellow warnings in the GitHub Actions log — contradicting the non-fatal intent.
The PR description explicitly calls this "skip + warning", so if warning-level logging is intentional here, a brief comment would clarify the choice. Otherwise, demoting to core.info for the skip path keeps logs clean.
💡 Suggested restructuring
Move the log level decision after the skip check:
const hasExplicitWildcardTargetField = WILDCARD_TARGET_FIELDS.some(field => message[field] != null);
const missingWildcardTarget = commentTarget === "*" && !hasExplicitWildcardTargetField;
if (missingWildcardTarget) {
core.info(targetResult.error); // non-fatal: expected when no target field is provided
return { success: false, skipped: true, error: targetResult.error };
}
core.warning(targetResult.error); // still a real failure
return { success: false, error: targetResult.error };|
|
||
| /** @type {string} Safe output type handled by this module */ | ||
| const HANDLER_TYPE = "add_comment"; | ||
| const WILDCARD_TARGET_FIELDS = ["item_number", "issue_number", "pull_request_number", "pr_number", "pr", "pull_number"]; |
There was a problem hiding this comment.
[/diagnose] item_number and issue_number in WILDCARD_TARGET_FIELDS are pre-handled by resolveSafeOutputIssueTarget at line 471 (using the same aliases). By the time resolveTarget fails, those two fields are already absent or null in the message — making their presence in this list technically dead weight.
The fields that can actually reach this code path unaided are pull_request_number, pr_number, pr, and pull_number. A short JSDoc comment would make this intent explicit and prevent future drift.
📋 Reasoning
resolveSafeOutputIssueTarget(line 471) uses aliases["item_number", "issue_number", "pr-number"]- If any of these are non-null, it resolves before reaching
resolveTarget— we never enter theshouldFailbranch - If it succeeds with
number: null, those fields are absent/null in the message - So at the wildcard check site,
message.item_number != nullandmessage.issue_number != nullare always false
Consider narrowing and documenting:
// Fields recognized by resolveTarget (pull-only path, supportsIssue: false).
// item_number/issue_number/pr-number are handled earlier by resolveSafeOutputIssueTarget.
const WILDCARD_TARGET_FIELDS = ["pull_request_number", "pr_number", "pr", "pull_number"];Or keep the full list and add a comment explaining it serves as a belt-and-suspenders guard.
|
@copilot review all comments and reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed all comments/reviews and applied actionable feedback in Changes made:
Validation run:
|
Contribution CheckandPR Sous Chefwere intermittently failing insafe_outputsafter successful agent work because a singleadd_commentwithtarget: "*"and no concrete target number hard-failed the job. This dropped valid outputs and prevented intended comments from posting.Safe-output handler behavior
add_commenthandling to treat unresolved wildcard targets as skip + info log (non-fatal) instead of a hard failure.target: "*"and no explicit numeric target field is provided.Target detection clarity
item_number,issue_number,pull_request_number,pr_number,pr,pull_numberRegression coverage
Workflow prompt contract alignment
add_commentpayloads must carry numeric targets in Contribution Check / PR Sous Chef paths.