fix: soft-skip target:triggering safe outputs when no triggering context#40035
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ft skip - Add MCP-level validation in updateIssueHandler to reject update_issue when target is triggering (or unset) and there's no issue context - Belt-and-suspenders: patch update_handler_factory to propagate shouldFail:false from resolveTarget as skipped:true result, preventing any existing NDJSON entries from failing the Process Safe Outputs step - Add tests for both MCP handler validation and factory soft-skip logic Fixes #40017 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review other safe outputs that support target triggering to have the same validation behavior in the MCP server |
…nt and update_pull_request Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Applied the same MCP-level context validation to the other two handlers that support
Also fixed a pre-existing omission: Commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in the “Process Safe Outputs” pipeline where target: triggering safe-output updates emitted during scheduled (non-issue/PR) runs were counted as fatal failures instead of being treated as no-ops/skips. It does so by adding context-aware validation at tool-invocation time (MCP handlers) and by propagating shouldFail:false through the NDJSON-processing update handler factory as a “soft skip” (skipped: true).
Changes:
- Added MCP-level context validation for
add_comment,update_issue, andupdate_pull_requestsotarget: triggeringis rejected early when no triggering context exists, preventing NDJSON writes that would fail downstream. - Updated the NDJSON update handler factory to propagate
shouldFail:falseand return{ success: false, skipped: true }so scheduled-run “triggering” updates don’t count toward fatal failures. - Updated setup/runtime packaging and compiled workflow locks (including tool allowlists) to include newly required helpers and permissions.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/setup.sh | Ensures invocation_context_helpers.cjs is packaged as part of safe-outputs runtime files. |
| actions/setup/js/update_handler_factory.cjs | Propagates shouldFail:false and converts those cases into soft-skipped processing results. |
| actions/setup/js/update_handler_factory.test.cjs | Adds coverage ensuring shouldFail:false is preserved and produces skipped: true results. |
| actions/setup/js/safe_outputs_handlers.cjs | Adds context-aware early validation for add_comment, update_issue, and update_pull_request before NDJSON writes. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Extends handler tests to cover schedule/push/non-triggering contexts and workflow_dispatch aw_context resolution. |
| actions/setup/js/safe_outputs_tools_loader.cjs | Registers the new update_issue handler wiring into the safe-outputs tool loader. |
| actions/setup-cli/install.sh | Updates sync note comment to reflect the canonical script source. |
| .github/workflows/spec-librarian.lock.yml | Updates compiled workflow lock to include shell(awk) in the allowed tools list. |
| .github/workflows/go-fan.lock.yml | Updates compiled workflow lock to include Bash(awk) in the allowed tools list. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Updates compiled workflow lock to include shell(awk) in the allowed tools list. |
| .github/workflows/daily-file-diet.lock.yml | Updates compiled workflow lock to include shell(awk) in the allowed tools list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 0
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40035 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). requires_adr_by_default_volume is false. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking issue; two non-blocking observations
The belt-and-suspenders approach (MCP-level early rejection + NDJSON-layer soft-skip) is the right design. The shouldFail:false propagation through createStandardResolveNumber is clean and the new handler tests have solid coverage. The blocking issue is a correctness hole in the exception-handling path that defeats the stated MCE1 goal.
Findings summary
Blocking (1)
- All three new
try/catchblocks aroundresolveInvocationContextsilently swallowERR_VALIDATIONfromcheckAllowedRepo— a real security-validation error gets discarded instead of surfaced to the agent (see inline comment on line 1912).
Non-blocking (2)
prEventNamesSet literal is duplicated verbatim inaddCommentHandlerandupdatePullRequestHandler; extract a module-level constant.- One bypass test relies on the implicit default event context rather than setting it explicitly.
🔎 Code quality review by PR Code Quality Reviewer
| invocationContext = resolveInvocationContext(context); | ||
| } catch { | ||
| // If context resolution fails fall through and let the downstream handler deal with it. | ||
| return defaultHandler("update_issue")(args || {}); |
There was a problem hiding this comment.
All three handlers swallow ERR_VALIDATION from checkAllowedRepo, silently defeating MCE1 early feedback when a workflow_dispatch specifies a disallowed cross-repo target.
💡 Details and suggested fix
resolveInvocationContext throws new Error(ERR_VALIDATION + ": ...") via checkAllowedRepo when target_repo is not in the allowlist (SEC-005). All three catch blocks swallow this without telling the agent:
updateIssueHandler(this catch, line 1912): callsdefaultHandler, writes to NDJSON, returns success to agentupdatePullRequestHandler(line ~1959): same patternaddCommentHandler(line ~1654): leavesinvocationContextundefined → validation skipped → proceeds to write NDJSON
The agent gets result: "success" from the MCP layer, then hits an unexpected failure during NDJSON processing. The "immediate feedback before writing to NDJSON" goal is broken on exactly the adversarial/misconfigured path.
Fix — distinguish the error kind before deciding how to handle it:
} catch (err) {
// A validation error (e.g. disallowed target_repo / SEC-005) is a real failure,
// not an unresolvable-context situation. Surface it immediately.
if (err?.message?.startsWith(ERR_VALIDATION)) {
return buildIntentErrorResponse(err.message);
}
// Unexpected structural error — fall through to downstream handler.
return defaultHandler("update_issue")(args || {});
}Same change needed in updatePullRequestHandler and addCommentHandler (where the catch should re-throw or return an error rather than silently continuing).
There was a problem hiding this comment.
Fixed in 132323f. All three catch blocks now inspect err?.message?.startsWith(ERR_VALIDATION) before deciding how to handle: validation errors (e.g. checkAllowedRepo / SEC-005) are returned immediately via buildIntentErrorResponse; unexpected structural errors fall through to the downstream handler as before.
| if (invocationContext !== undefined) { | ||
| const effectiveEventName = invocationContext?.eventName || context.eventName; | ||
| const effectivePayload = invocationContext?.eventPayload || context.payload; | ||
| const prEventNames = new Set(["pull_request", "pull_request_target", "pull_request_review", "pull_request_review_comment"]); |
There was a problem hiding this comment.
prEventNames Set is copy-pasted identically at line 1659 (addCommentHandler) and line 1963 (updatePullRequestHandler); adding a new PR event type will silently miss one.
💡 Suggested fix
Extract a module-level constant to eliminate the duplication:
// Near the top of the module, after requires:
const PR_EVENT_NAMES = new Set([
"pull_request",
"pull_request_target",
"pull_request_review",
"pull_request_review_comment",
]);Then both spots just reference PR_EVENT_NAMES. If merge_group or another PR-adjacent event ever needs coverage, there is one place to update.
There was a problem hiding this comment.
Fixed in 132323f. Extracted PR_EVENT_NAMES as a module-level const (new Set([...]) defined once at load time). Both addCommentHandler and updatePullRequestHandler now reference the shared constant, so a future addition only needs one edit.
| } | ||
| }); | ||
|
|
||
| it("should write entry when explicit item_number bypasses context check in non-issue/PR event", () => { |
There was a problem hiding this comment.
This test asserts bypass behavior without explicitly setting a non-triggering context; if the default test context ever changes, it will pass for the wrong reason.
💡 Suggested fix
The test relies on global.context having eventName: "push" from the module-level mockContext setup (line 35). That's currently true, but the intent is invisible in the test body. Any future beforeEach that sets an issue or PR context will make the test vacuously pass even if the bypass logic is broken.
Make the context explicit, consistent with every other new test in this block:
it("should write entry when explicit item_number bypasses context check in non-issue/PR event", () => {
const savedContext = global.context;
global.context = { ...global.context, eventName: "push", payload: {} };
try {
const result = handlers.addCommentHandler({
body: "A real comment body that is substantive enough",
item_number: 42,
});
expect(result.isError).toBeUndefined();
const responseData = JSON.parse(result.content[0].text);
expect(responseData.result).toBe("success");
expect(mockAppendSafeOutput).toHaveBeenCalledWith(
expect.objectContaining({ type: "add_comment", item_number: 42 })
);
} finally {
global.context = savedContext;
}
});There was a problem hiding this comment.
Fixed in 132323f. The test now explicitly sets global.context = { ...global.context, eventName: "push", payload: {} } before calling the handler, making the non-triggering context visible in the test body and immune to future beforeEach changes.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out to this bug-fix PR. All findings are non-blocking suggestions — no request for changes.
📋 Key Themes & Highlights
Key Themes
- Consistency (3 comments): The three new handlers share the same pattern but implement it with slight variations — exception handling style, a repeated two-line extraction, and a
new Set()allocated on every call. Worth unifying before this grows to more handlers. - Test coverage gaps (2 comments): The
issue_comment-on-PR boundary (rejected byupdate_issue, accepted byadd_comment) and bareworkflow_dispatchwithout anevent_nameinput are untested edge cases that could regress silently. shouldFailnaming (1 comment): The flag propagates correctly but the negative-sense name makes the callsite harder to scan.
Positive Highlights
- ✅ Two-layer fix is well architected: MCP-level early rejection gives the LLM an actionable error before touching NDJSON; downstream
skipped: trueprotects against pre-existing entries. - ✅ Error messages are excellent — they name the event, explain why it doesn't work, and suggest concrete alternatives (
create_discussion,create_issue, explicititem_number). - ✅ Catching the missing
invocation_context_helpers.cjsentry insetup.shis the kind of latent-crash fix that's easy to overlook — good catch. - ✅ Registering
updateIssueHandlerinsafe_outputs_tools_loader.cjswas the key prerequisite for the primary fix; correctly identified and wired. - ✅ The regression test expansion (361 additions) across all three handlers and multiple event types gives strong confidence in the new behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| } catch { | ||
| // Context resolution failed; skip validation and let downstream handle gracefully | ||
| } | ||
| if (invocationContext !== undefined) { |
There was a problem hiding this comment.
[/diagnose] The invocationContext !== undefined guard is an unconventional way to detect a thrown exception — it relies on the catch block leaving the let invocationContext binding at its undefined initial value. updateIssueHandler and updatePullRequestHandler (added in this same PR) use an explicit early-return inside catch, which is easier to reason about.
💡 Suggested approach for consistency
Match the pattern used in the sibling handlers:
let invocationContext;
try {
invocationContext = resolveInvocationContext(context);
} catch {
// Context resolution failed; let downstream handle gracefully.
invocationContext = null;
}
if (invocationContext != null) {
// validate ...
}There was a problem hiding this comment.
Fixed in 132323f. addCommentHandler now initialises let invocationContext = null, sets it to null explicitly in the ERR_VALIDATION path (which returns early), and guards on invocationContext != null — matching the early-return pattern used in updateIssueHandler and updatePullRequestHandler.
| if (invocationContext !== undefined) { | ||
| const effectiveEventName = invocationContext?.eventName || context.eventName; | ||
| const effectivePayload = invocationContext?.eventPayload || context.payload; | ||
| const prEventNames = new Set(["pull_request", "pull_request_target", "pull_request_review", "pull_request_review_comment"]); |
There was a problem hiding this comment.
[/diagnose] new Set([...]) is instantiated on every addCommentHandler call. The same set appears again at line 1963 inside updatePullRequestHandler. Extract it as a module-level constant to avoid repeated allocations on every invocation.
💡 Suggested fix
At the top of the module, before any handler definitions:
const PR_EVENT_NAMES = new Set([
'pull_request',
'pull_request_target',
'pull_request_review',
'pull_request_review_comment',
]);Then reference PR_EVENT_NAMES in both handlers.
There was a problem hiding this comment.
Fixed in 132323f. PR_EVENT_NAMES is now a module-level const Set instantiated once at load time, referenced by both addCommentHandler and updatePullRequestHandler.
| // Context resolution failed; skip validation and let downstream handle gracefully | ||
| } | ||
| if (invocationContext !== undefined) { | ||
| const effectiveEventName = invocationContext?.eventName || context.eventName; |
There was a problem hiding this comment.
[/zoom-out] The two-line context extraction pattern (effectiveEventName + effectivePayload) is repeated verbatim in all three handlers (lines 1657–1658, 1914–1915, 1961–1962). Extracting a small helper would remove the duplication and make a single place to fix if resolveInvocationContext's return shape ever changes.
💡 Suggested helper
function resolveEffectiveContext(invocationContext, rawContext) {
return {
eventName: invocationContext?.eventName || rawContext.eventName,
payload: invocationContext?.eventPayload || rawContext.payload,
};
}Each handler then becomes one line: const { eventName: effectiveEventName, payload: effectivePayload } = resolveEffectiveContext(invocationContext, context);
There was a problem hiding this comment.
Fixed in 132323f. Added resolveEffectiveContext(invocationContext, rawContext) as a module-level helper that returns { effectiveEventName, effectivePayload }. All three handlers now call it with one line instead of repeating the two-line extraction.
| // shouldFail:false means the target cannot be resolved in this context but it is not | ||
| // an error (e.g. target:triggering on a schedule run has no triggering issue). | ||
| // Treat it as a soft skip so it does not count toward fatal failures. | ||
| if (itemNumberResult.shouldFail === false) { |
There was a problem hiding this comment.
[/diagnose] shouldFail === false is correct (strict equality guards against undefined), but the boolean flag name requires double-negation thinking: "should fail" set to false means "is a soft skip". A positive-sense name like skipSilently: true or isContextMismatch: true would make the callsites read as intent rather than as a negated condition.
💡 Example rename
In resolveTarget return sites: { success: false, skipSilently: true, error: ... }
In createStandardResolveNumber: return { success: false, skipSilently: targetResult.skipSilently, error: ... };
In createUpdateHandlerFactory: if (itemNumberResult.skipSilently) { ... }
This reads: "if we should skip silently, do so" rather than "if we should NOT fail, skip".
There was a problem hiding this comment.
Declining this rename. shouldFail is used in ~10 files outside this PR (safe_output_helpers.cjs, add_comment.cjs, comment_memory.cjs, assign_to_agent.cjs, create_check_run.cjs, submit_pr_review.cjs, safe_output_processor.cjs). Renaming a public contract field across all callsites goes well beyond the scope of this fix and risks introducing regressions in production handlers. The existing shouldFail === false check is correct and the comment in the factory already explains its semantics. Happy to track the rename as a follow-up task.
| }); | ||
| }); | ||
|
|
||
| describe("updateIssueHandler", () => { |
There was a problem hiding this comment.
[/tdd] The updateIssueHandler describe block is missing a boundary test for issue_comment events that fire on a PR. The code at line 1916 in handlers intentionally treats issue_comment with payload.issue.pull_request set as PR context, not issue context, so update_issue should reject it. Without a test this invariant is invisible and could silently regress.
💡 Suggested test
it('should return intent error for issue_comment on a PR (not issue context)', () => {
const savedContext = global.context;
global.context = {
...global.context,
eventName: 'issue_comment',
payload: { issue: { number: 7, pull_request: { url: '(redacted)' } } },
};
try {
const result = handlers.updateIssueHandler({ body: 'Update body' });
expect(result.isError).toBe(true);
const data = JSON.parse(result.content[0].text);
expect(data.error).toContain('issue context');
expect(mockAppendSafeOutput).not.toHaveBeenCalled();
} finally {
global.context = savedContext;
}
});A companion test for addCommentHandler should confirm the inverse: issue_comment on a PR succeeds (it is valid PR context for add_comment).
There was a problem hiding this comment.
Fixed in 132323f. Added two tests:
updateIssueHandler:issue_commenton a PR →isError: true, error contains"issue context",appendSafeOutputnot called.addCommentHandler+updatePullRequestHandler:issue_commenton a PR → success, entry written (valid PR context for both).
| expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment", item_number: 42 })); | ||
| }); | ||
|
|
||
| it("should write entry on workflow_dispatch with issue aw_context", () => { |
There was a problem hiding this comment.
[/tdd] Coverage exists for workflow_dispatch with aw_context resolving to an issue/PR, but there is no test for a bare workflow_dispatch with no event_name in inputs. In that case resolveInvocationContext returns eventName: "workflow_dispatch", which is not issue/PR/discussion context, so all three handlers should reject it. Without a test, a future change to resolveInvocationContext that accidentally passes "workflow_dispatch" through could go undetected.
💡 Suggested test (addCommentHandler; repeat for update_issue and update_pull_request)
it('should return intent error on workflow_dispatch with no event_name override', () => {
const savedContext = global.context;
global.context = {
...global.context,
eventName: 'workflow_dispatch',
payload: { inputs: {} }, // no event_name, no aw_context
};
try {
const result = handlers.addCommentHandler({ body: 'A real comment body that is substantive' });
expect(result.isError).toBe(true);
const data = JSON.parse(result.content[0].text);
expect(data.error).toContain('"workflow_dispatch"');
expect(mockAppendSafeOutput).not.toHaveBeenCalled();
} finally {
global.context = savedContext;
}
});There was a problem hiding this comment.
Fixed in 132323f. Added a bare workflow_dispatch (no event_name, no aw_context) test for each of the three handlers. All three verify isError: true with the error containing "workflow_dispatch" and appendSafeOutput not called.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics & Test Classification (16 tests analyzed)
Go: 0; JavaScript: 16 ( Scoring breakdown:
Mock usage:
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dlers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Scheduled workflows (no triggering issue or PR) were failing the entire
Process Safe Outputsstep when an agent emittedupdate_issuewithtarget: triggering— the default. The skip was tallied as a fatal failure rather than a no-op.MCP-level validation (primary fix)
Added context-aware handlers in
safe_outputs_handlers.cjsthat validate event context before writing to the NDJSON safe-outputs file. Whentargetistriggering(explicit or by default) and no appropriate context exists, the handler returns an inline error to the agent so it can adjust its output immediately rather than failing downstream.This validation is applied to all safe output tools that support
target: triggering:updateIssueHandler— rejects when not in an issue context; suggestscreate_discussion,create_issue, ortarget: '*'with an explicitissue_number.updatePullRequestHandler— rejects when not in a PR context; suggestscreate_discussion,create_issue, ortarget: '*'with an explicitpull_request_number.addCommentHandler— rejects when not in an issue, PR, or discussion context and no explicititem_numberis provided. Explicititem_number/issue_number/pr-numberin the args bypasses the check, mirroring downstreamadd_comment.cjsbehaviour.All handlers correctly pass through:
triggeringtargets (e.g.target: '*')workflow_dispatchruns withaw_contextresolving to an appropriate eventRegistered the handlers in
safe_outputs_tools_loader.cjs.Also fixed a missing entry:
invocation_context_helpers.cjsis now listed inSAFE_OUTPUTS_FILESinsetup.shas required by its transitive dependency onsafe_outputs_handlers.cjs.Belt-and-suspenders (existing NDJSON entries)
Patched
update_handler_factory.cjsto propagateshouldFail: falsefromresolveTarget()throughcreateStandardResolveNumber, and return{ success: false, skipped: true }inhandleUpdate. This covers existing NDJSON entries already written without MCP validation and any codepaths that bypass the MCP server —isFailedProcessingResult()already exemptsskipped: trueresults from the failure count.