Add PR-targeting support to create_check_run (including target: "*" flows)#38237
Add PR-targeting support to create_check_run (including target: "*" flows)#38237Copilot wants to merge 5 commits into
create_check_run (including target: "*" flows)#38237Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
create_check_run (including target: "*" flows)
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds explicit pull request targeting to the create_check_run safe-output so checks can be attached to the intended PR head SHA (including target: "*" per-call routing), instead of relying on event SHA heuristics.
Changes:
- Emit
targetin compiled handler config forcreate_check_runand parse it from workflow frontmatter. - Extend the
create_check_runtool schema to acceptpull_request_numberplus aliases for wildcard targeting. - Update the JS handler to resolve the PR first and use
pull_request.head.shawhentargetis configured; add JS + compiler-level tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_handler_registry.go | Emits target into handler config for create_check_run. |
| pkg/workflow/js/safe_outputs_tools.json | Documents pull_request_number + aliases for wildcard PR targeting. |
| pkg/workflow/create_check_run.go | Adds Target to CreateCheckRunConfig and parses it from frontmatter. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Adds compiler coverage asserting target is emitted for create_check_run. |
| actions/setup/js/safe_outputs_tools.json | Mirrors tool-schema updates for the setup action bundle. |
| actions/setup/js/create_check_run.test.cjs | Adds runtime tests for wildcard + explicit PR target resolution paths. |
| actions/setup/js/create_check_run.cjs | Implements PR resolution + PR head SHA attachment when target is configured. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
| githubClient.rest.pulls.get({ | ||
| owner, | ||
| repo, | ||
| pull_number: pullRequestNumber, | ||
| }), |
There was a problem hiding this comment.
Fixed in the first commit. Added NewPermissionsContentsReadChecksWritePRRead() to permissions_factory.go (contents:read + checks:write + pull-requests:read) and updated the create-check-run PermissionBuilder in safe_output_handlers.go to use it when safeOutputs.CreateCheckRun.Target != "".
| // Parse target (optional PR targeting mode) | ||
| if target, exists := configMap["target"]; exists { | ||
| if targetStr, ok := target.(string); ok { | ||
| checkRunConfig.Target = targetStr | ||
| createCheckRunLog.Printf("Using check run target: %s", targetStr) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed. Added an else branch that calls createCheckRunLog.Printf("Warning: create-check-run target value %v is not a string and will be ignored", target) in create_check_run.go when the type assertion fails.
| for _, step := range steps { | ||
| if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { | ||
| parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") | ||
| if len(parts) == 2 { | ||
| jsonStr := strings.TrimSpace(parts[1]) | ||
| jsonStr = strings.Trim(jsonStr, "\"") | ||
| jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") | ||
|
|
||
| var config map[string]map[string]any | ||
| err := json.Unmarshal([]byte(jsonStr), &config) | ||
| require.NoError(t, err) | ||
|
|
||
| checkRunConfig, ok := config["create_check_run"] | ||
| require.True(t, ok) | ||
|
|
||
| target, ok := checkRunConfig["target"] | ||
| require.True(t, ok) | ||
| assert.Equal(t, "*", target) | ||
| } |
There was a problem hiding this comment.
Fixed. Added foundHandlerConfig := false before the loop, set it to true inside the matching branch, and added require.True(t, foundHandlerConfig, "Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in generated steps") after the loop — matching the pattern already used by TestHandlerConfigUpdatePullRequestUpdateBranch.
![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 /tdd, /zoom-out, and /grill-with-docs — no blocking correctness issues, but four coverage gaps and one schema inconsistency are worth addressing.
�� Key Themes & Highlights
Key Themes
- Test coverage gaps: The
pulls.getcatch block and thetarget: "triggering"code path are both exercised by production code but have no tests. The Go compiler test uses a conditional loop pattern that can pass vacuously if the emitted YAML changes shape. target: "triggering"makes a redundant API call: functionally equivalent to no-target, but routes throughresolveTarget+pulls.getinstead of readingcontext.payload.pull_request.head.shadirectly — introducing extra latency and a new failure mode without an obvious benefit.- Schema alias consistency:
pr_number,pr,pull_numberlackx-synonymsentries that similar aliases carry elsewhere in the schema.
Positive Highlights
- ✅ Clean reuse of the shared
resolveTargethelper — new behavior is consistent with how other PR-targeting handlers work - ✅ Good error handling structure: target resolution failures, missing SHA, and API errors each return distinct, descriptive error objects
- ✅ Three meaningful new JS tests covering wildcard success, wildcard missing-number error, and explicit numeric target
- ✅ End-to-end wiring (Go struct → YAML parser → handler registry → JS runtime) is complete and tightly scoped
- ✅
additionalProperties: falsepreserved in schema — new fields are additive and safe
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 382.9 AIC · ⌖ 13.8 AIC
| return { success: false, error: msg }; | ||
| } | ||
| core.info(`Using PR #${pullRequestNumber} head SHA ${headSha} (target=${checkRunTarget})`); | ||
| } catch (error) { |
There was a problem hiding this comment.
[/tdd] The pulls.get catch block has no test coverage — if the GitHub API returns a 404 (non-existent PR) or a 403 (auth failure), this error path silently goes untested.
💡 Suggested test case
it("returns error when pulls.get throws", async () => {
mockGithub.rest.pulls = {
get: async () => { throw new Error("Not Found"); },
};
const { main } = require("./create_check_run.cjs");
const handler = await main({ max: 10, target: "*" });
const result = await handler(
{ type: "create_check_run", pull_request_number: 42, conclusion: "success", title: "T", summary: "S" },
{}
);
expect(result.success).toBe(false);
expect(result.error).toContain("Failed to resolve pull request");
});This path is the primary recovery surface for transient network errors and stale PR references.
There was a problem hiding this comment.
Added "returns error when pulls.get throws (e.g. 404 or 403)" test in the target resolution describe block. It stubs mockGithub.rest.pulls.get to throw new Error("Not Found") and asserts result.success === false with result.error containing "Failed to resolve pull request".
| expect(result.success).toBe(true); | ||
| expect(capturedParams.head_sha).toBe("target-pr-sha-7"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] target: "triggering" is a documented valid value but has no test — its code path (resolveTarget → pulls.get in a PR event context) is distinct from both the no-target fallback and the wildcard/explicit paths.
💡 Suggested test case
it("resolves triggering PR context when target is \"triggering\"", async () => {
mockContext.eventName = "pull_request";
mockContext.payload = { pull_request: { number: 99 } };
mockGithub.rest.pulls = {
get: async ({ pull_number }) => ({
data: { number: pull_number, head: { sha: "triggering-pr-sha" } },
}),
};
let capturedParams;
mockGithub.rest.checks.create = makeChecksCreate(p => { capturedParams = p; });
const { main } = require("./create_check_run.cjs");
const handler = await main({ max: 10, target: "triggering" });
const result = await handler(
{ type: "create_check_run", conclusion: "success", title: "T", summary: "S" },
{}
);
expect(result.success).toBe(true);
expect(capturedParams.head_sha).toBe("triggering-pr-sha");
});Without this test the target: "triggering" branch in create_check_run.cjs is invisible to the test suite.
There was a problem hiding this comment.
Added "resolves triggering PR context when target is 'triggering'" test. It sets mockContext.eventName = "pull_request" with payload.pull_request.number = 99, stubs pulls.get to return head.sha = "api-sha-99", and asserts that capturedParams.head_sha equals the API-fetched SHA (not the payload SHA), confirming the refreshed-SHA intent documented in the code comment.
| "description": "Pull request number to attach the check run to when the workflow uses `create-check-run: target: \"*\"` (or equivalent explicit PR targeting). This is the numeric ID from the GitHub URL (e.g., 876 in github.com/owner/repo/pull/876).", | ||
| "x-synonyms": ["pullRequestNumber"] | ||
| }, | ||
| "pr_number": { |
There was a problem hiding this comment.
[/grill-with-docs] The three alias fields (pr_number, pr, pull_number) are missing x-synonyms entries, unlike similar aliases in add_comment and the primary pull_request_number field which has "x-synonyms": ["pullRequestNumber"]. Agents that resolve field names via synonym matching may not recognize camelCase variants of these aliases.
💡 Suggested additions
"pr_number": {
"type": ["number", "string"],
"description": "Alias for pull_request_number. Prefer pull_request_number in new calls.",
"x-synonyms": ["prNumber"]
},
"pr": {
"type": ["number", "string"],
"description": "Alias for pull_request_number. Prefer pull_request_number in new calls.",
"x-synonyms": ["pullRequest", "pull"]
},
"pull_number": {
"type": ["number", "string"],
"description": "Alias for pull_request_number. Prefer pull_request_number in new calls.",
"x-synonyms": ["pullNumber"]
}The same change applies to pkg/workflow/js/safe_outputs_tools.json (the identical copy).
There was a problem hiding this comment.
Added x-synonyms to all three alias fields in both actions/setup/js/safe_outputs_tools.json and pkg/workflow/js/safe_outputs_tools.json: pr_number → ["prNumber"], pr → ["pullRequest", "pull"], pull_number → ["pullNumber"].
| var steps []string | ||
| compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) | ||
|
|
||
| for _, step := range steps { |
There was a problem hiding this comment.
[/tdd] The for _, step := range steps loop is conditional — if addHandlerManagerConfigEnvVar generates no step containing GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG, the loop body never executes, all assertions are skipped, and the test passes vacuously. This is the same pattern as TestHandlerConfigClosePullRequestTargetRepo so it is pre-existing, but worth locking down.
💡 Suggested fix
Add a sentinel after the loop to assert the step was actually found:
found := false
for _, step := range steps {
if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") {
found = true
// ... existing assertions ...
}
}
require.True(t, found, "expected a step containing GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG")This prevents the test from silently passing if the config env var is accidentally removed from the emitted YAML.
There was a problem hiding this comment.
Fixed. Added foundHandlerConfig := false before the loop, set to true inside the matching branch, and require.True(t, foundHandlerConfig, "Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in generated steps") after the loop.
| const headSha = prHeadSha || process.env.GITHUB_SHA || context.sha; | ||
| let headSha = ""; | ||
|
|
||
| if (checkRunTarget) { |
There was a problem hiding this comment.
[/zoom-out] When target: "triggering" is configured and the context is a pull_request event, this branch calls resolveTarget (returns context.payload.pull_request.number), then makes a pulls.get API call to fetch the head SHA. The else branch below reads the same SHA directly from context.payload.pull_request.head.sha with no API call. This means target: "triggering" is functionally equivalent to no target, but slower and with an extra failure mode.
💡 Two options
Option A — Short-circuit for triggering in PR context:
Inside the checkRunTarget branch, after a successful resolveTarget, check targetResult.contextType === "pull request" and whether the payload already has the SHA; if so, skip the API call and use context.payload.pull_request.head.sha directly.
Option B — Document the semantic difference:
If the API call is intentional (e.g., to get a refreshed SHA in case the PR was force-pushed between trigger and handler), add a brief comment explaining that intent, and add a test that explicitly asserts the API call is made rather than the payload SHA used.
There was a problem hiding this comment.
Took Option B. Added a code comment above the pulls.get call explaining the intent: "We intentionally go through the API even when the context payload already carries a SHA (e.g. target: "triggering" on a pull_request event) so that we always use the most recent head in case the PR was force-pushed between the triggering event and when this handler runs." Also added a test that asserts capturedParams.head_sha equals the API-fetched SHA rather than the payload SHA.
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 58/100 — Needs Improvement
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 58/100. Test quality is acceptable — 25% of new tests are implementation tests (threshold: 30%). Note: test inflation (4.1:1 Go ratio) and missing assertion messages reduce the score; see the flagged test in the comment above for suggested improvements.
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking test issue, one incorrect schema annotation.
🔍 Findings summary
🔴 High — TestHandlerConfigCreateCheckRunTarget can silently pass without validating
compiler_safe_outputs_config_test.go — The new test is missing the foundHandlerConfig guard that every analogous test in the same file uses. If addHandlerManagerConfigEnvVar never emits a GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG step, or the step format changes, the loop body is never entered, all assertions are skipped, and the test passes green. This gives completely false confidence that target serialization works.
🟡 Medium — x-synonyms: ["pullRequestNumber"] is wrong
safe_outputs_tools.json — The runtime (resolveTarget) checks pr_number, pr, pull_number — all snake_case. pullRequestNumber (camelCase) is not handled anywhere. The annotation will mislead tools/agents that read x-synonyms to discover valid aliases.
🔵 Low — Staged mode makes a live pulls.get() API call that gets discarded
create_check_run.cjs — When checkRunTarget is set, the PR head SHA is fetched before the isStaged check. In staged mode the SHA is then thrown away without being included in the preview log. Either skip the fetch in staged mode or surface the SHA in the log.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.9 AIC
| assert.Equal(t, "*", target) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Test silently passes without asserting anything: if addHandlerManagerConfigEnvVar produces no step matching GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG (or the format changes), the loop body is never entered and the test exits with zero assertions.
💡 Suggested fix
Every equivalent test in this file (e.g., TestHandlerConfigUpdatePullRequestUpdateBranch, TestHandlerConfigClosePullRequestTargetRepo) tracks whether the config step was found and asserts it at the end:
var steps []string
compiler.addHandlerManagerConfigEnvVar(&steps, workflowData)
foundHandlerConfig := false // ADD
for _, step := range steps {
if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") {
foundHandlerConfig = true // ADD
parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ")
if len(parts) == 2 {
// ... existing JSON parsing ...
}
}
}
require.True(t, foundHandlerConfig, "Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in generated steps") // ADDWithout this guard, a future regression that stops serializing target into the env var would go completely undetected because require.True(t, ok) and assert.Equal(t, "*", target) inside the if len(parts) == 2 block are simply never reached.
There was a problem hiding this comment.
Fixed. Added foundHandlerConfig := false sentinel before the loop, set to true inside the matching branch, and require.True(t, foundHandlerConfig, "Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in generated steps") after the loop.
| "pull_request_number": { | ||
| "type": ["number", "string"], | ||
| "description": "Pull request number to attach the check run to when the workflow uses `create-check-run: target: \"*\"` (or equivalent explicit PR targeting). This is the numeric ID from the GitHub URL (e.g., 876 in github.com/owner/repo/pull/876).", | ||
| "x-synonyms": ["pullRequestNumber"] |
There was a problem hiding this comment.
x-synonyms annotation points to a field the runtime never reads: pullRequestNumber (camelCase) is listed as the synonym, but resolveTarget in safe_output_helpers.cjs only checks item.pull_request_number || item.pr_number || item.pr || item.pull_number — all snake_case. No camelCase alias is handled anywhere.
💡 Suggested fix
Either:
- Fix the annotation to list the actual aliases that
resolveTargetsupports:
"x-synonyms": ["pr_number", "pr", "pull_number"]- Or add camelCase handling in
resolveTarget:
const pullRequestNumberField =
item.pull_request_number ||
item.pullRequestNumber || // ADD
item.pr_number ||
item.pr ||
item.pull_number;The same change applies to pkg/workflow/js/safe_outputs_tools.json which is an identical copy.
As-is, any tool that reads x-synonyms to discover equivalent aliases will advertise pullRequestNumber as valid, but passing it will silently be ignored (filtered by additionalProperties: false before reaching the runtime).
There was a problem hiding this comment.
Fixed pull_request_number's x-synonyms to list the actual alias property names the runtime checks: ["pr_number", "pr", "pull_number"]. Also added x-synonyms for each alias field. Applied to both actions/setup/js/safe_outputs_tools.json and pkg/workflow/js/safe_outputs_tools.json.
| try { | ||
| const { data: pullRequest } = await withRetry( | ||
| () => | ||
| githubClient.rest.pulls.get({ |
There was a problem hiding this comment.
Live API call executes even in staged mode: githubClient.rest.pulls.get() runs before the isStaged check at line 182. In staged mode, the PR head SHA is fetched, used for nothing (the staged preview log does not include it), then discarded.
💡 Suggested fix
Either guard the API call behind !isStaged, or include the resolved SHA in the staged preview output so the call is justified:
// Option A: skip the API call in staged mode
if (isStaged) {
logStagedPreviewInfo(`Would create check run "${checkRunName}" targeting PR #${pullRequestNumber} with conclusion=${conclusion}, title="${resolvedTitle}"`);
processedCount++;
return { success: true, staged: true, ... };
}
// Only fetch the SHA when we actually need it
const { data: pullRequest } = await withRetry(...);// Option B: include the SHA in the staged preview (already fetched, make it useful)
logStagedPreviewInfo(
`Would create check run "${checkRunName}" on PR #${pullRequestNumber} @ ${headSha} with conclusion=${conclusion}, title="${resolvedTitle}"`
);There was a problem hiding this comment.
Fixed with Option A. The staged-mode early return now happens inside the checkRunTarget branch before the pulls.get call, so no live API call is made in staged mode. The staged preview message includes the PR number for context. Added a test "skips pulls.get API call and returns staged preview when target is set and staged mode is active" that asserts pullsGetCalled === false.
| supportsIssue: false, | ||
| }); | ||
| if (!targetResult.success) { | ||
| core.warning(targetResult.error); |
There was a problem hiding this comment.
core.warning used for hard failures (shouldFail === true) instead of core.error: when target resolution fails with a genuine error — invalid PR number, target: "*" with no pull_request_number in the message — the log shows a yellow warning instead of a red error annotation, even though the workflow will ultimately fail. Compare assign_to_agent.cjs which correctly branches on shouldFail.
💡 Suggested fix
Mirror the pattern from assign_to_agent.cjs:
if (!targetResult.success) {
if (targetResult.shouldFail) {
core.error(targetResult.error); // hard failure → red annotation
} else {
core.info(targetResult.error); // expected soft skip → informational
}
return {
success: false,
error: targetResult.error,
skipped: !targetResult.shouldFail,
};
}As written, a misconfigured target (e.g., target: "abc" — not a number) logs a warning, the outer processor later emits a core.error and core.setFailed, and the reader has to hunt for why the step failed after seeing only a warning about it.
There was a problem hiding this comment.
Fixed. Now uses core.error when targetResult.shouldFail is true (hard failure → red annotation) and core.info when it is false (expected soft skip → informational), mirroring the assign_to_agent.cjs pattern. The skipped field is now !targetResult.shouldFail.
|
@copilot run pr-finisher skill |
- Add NewPermissionsContentsReadChecksWritePRRead for target-configured check runs - Conditionally emit pull-requests:read when create-check-run has a target set - Warn when target config value is non-string rather than silently ignoring - Fix TestHandlerConfigCreateCheckRunTarget false-positive by tracking and asserting foundHandlerConfig Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…synonyms, and tests - Use core.error/core.info instead of core.warning for target resolution failures - Skip pulls.get API call in staged mode; include PR number in staged preview message - Add comment explaining why target: "triggering" always goes via API (refreshed SHA) - Add tests: pulls.get throw, target: "triggering", staged mode skips API call - Fix x-synonyms: pull_request_number lists alias property names; add synonyms for all aliases - Apply x-synonyms fix to both safe_outputs_tools.json copies Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot do deep review of the changes, fix security. YAGNI/SOLID/DRY |
…turns, strengthen test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit Security — missing DRY — two duplicate staged-mode return blocks Log accuracy — misleading "Creating check run" log in staged mode |
|
@copilot update safe outputs specification using w3c spec writer |
…ification v1.23.0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot add the missing test guard and fix the x-synonyms annotation, then request another review.
|
create_check_runpreviously attached checks using event SHA heuristics and could not target arbitrary pull requests the waycreate-pull-request-adjacent tools can. This change adds explicit PR targeting semantics so check runs can be attached to the intended PR head SHA in wildcard and explicit-target workflows.Config surface: add PR target support
safe-outputs.create-check-runnow acceptstarget(triggering,*, or explicit PR number) in compiler config.targetforcreate_check_run.targetvalues in YAML frontmatter now produce a warning log instead of being silently ignored.Runtime behavior: resolve PR first, then attach check run
create_check_runnow resolves target PR via shared target resolution logic whentargetis configured.core.error(red annotation); expected soft skips emitcore.info.targetconfigured, thepulls.getAPI call is skipped entirely; the staged preview includes the resolved PR number.GITHUB_SHA→context.sha) when notargetis configured.if (isStaged)block after SHA resolution).Permissions: least-privilege
pull-requests: readwhen targetingcreate-check-runhas atargetconfigured, the compiled workflow now requestspull-requests: readin addition tocontents: read+checks: write, so thepulls.getcall does not 403 in least-privilege environments.Tool contract: per-call PR selection in wildcard mode
create_check_runtool schema documentspull_request_numberand aliases (pr_number,pr,pull_number) fortarget: "*"usage.x-synonymsentries for camelCase discovery;pull_request_number'sx-synonymslists the actual alias property names the runtime checks.Specification:
create_check_runadded to Safe Outputs MCP Gateway Specification (v1.23.0)create_check_runindocs/src/content/docs/specs/safe-outputs-specification.md.target), and example frontmatter + agent message.Coverage updates
triggeringPR target resolution paths.pulls.getAPI error recovery (including assertion thatcore.erroris called) and staged-mode-with-target no-API-call behavior.targetis emitted forcreate_check_runhandler config, with a sentinel to prevent false-positive test passes.{"type":"create_check_run","pull_request_number":876,"conclusion":"failure","title":"3 issues found","summary":"See findings"}