Align safe-output no-context behavior: allow MCP enqueue, rely on runtime soft-skip#48955
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Allows context-free safe-output items to enqueue and rely on runtime soft-skipping.
Changes:
- Removes MCP no-context rejection for comments and PR updates.
- Updates enqueue expectations for push, schedule, and dispatch events.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_outputs_handlers.cjs |
Defers no-context handling to runtime. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests successful context-free enqueueing. |
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: 2
- Review effort level: Medium
| // Build the entry with a temporary_id | ||
| const entry = { ...(args || {}), type: "add_comment" }; |
There was a problem hiding this comment.
Addressed in commit 704795f. Both addCommentHandler and updatePullRequestHandler now call resolveInvocationContext(context) unconditionally (not gated on no-context checks). Any ERR_VALIDATION error — including a disallowed target_repo (SEC-005) — is surfaced immediately as an MCP intent error before writing to NDJSON. Non-validation structural errors are still silently passed downstream. A dedicated test case (should return intent error when workflow_dispatch has disallowed target_repo (SEC-005)) confirms the rejection path for each handler.
| } | ||
| } | ||
|
|
||
| return defaultHandler("update_pull_request")(args || {}); |
There was a problem hiding this comment.
Addressed in commit 704795f. updatePullRequestHandler now calls resolveInvocationContext(context) unconditionally and surfaces ERR_VALIDATION errors (including SEC-005 / disallowed target_repo) at MCP-phase before appending to NDJSON. No-context events (schedule, push, workflow_dispatch without aw_context) are enqueued and soft-skipped at runtime. A dedicated test case (should return intent error when workflow_dispatch has disallowed target_repo (SEC-005)) verifies that a disallowed target_repo is rejected with an intent error and mockAppendSafeOutput is never called.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business dirs: src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
|
![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 /diagnosing-bugs and /codebase-design — commenting, not blocking.
📋 Key Themes & Highlights
Key Themes
- SEC-005 regression (pre-existing comments #3677053881, #3677053921): The removed blocks contained the only MCP-phase SEC-005 (
target_repo) validation for these two handlers. Previous review comments already flag this — that path should be preserved or lifted out before the context gate. - Runtime soft-skip coverage gap: Tests confirm successful MCP enqueue but do not cover the runtime processor's soft-skip behavior. If the downstream handler regresses, the MCP test suite stays green.
- Asymmetric documentation:
updatePullRequestHandlerreceived a JSDoc update explaining the delegation;addCommentHandlerdid not.
Positive Highlights
- ✅ Correct fix for the mixed-payload hard-fail problem (#48925) — delegating no-context resolution to runtime is the right architecture.
- ✅ Wildcard-target and payload-shape validations are preserved.
- ✅ Tests are clearly renamed to reflect the new behavior.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34 AIC · ⌖ 10.6 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/safe_outputs_handlers.test.cjs:2288
[/diagnosing-bugs] These tests now assert MCP enqueue succeeds, but there is no coverage for what happens to these entries in the runtime processor — the correctness of the soft-skip itself is untested here.
<details>
<summary>💡 Suggested additional coverage</summary>
Consider adding integration-style or mocked runtime handler tests for entries written from non-PR/issue/discussion contexts:
it('runtime handler soft-skips add_comment entries from push context', () => {
// Arrange…
</details>
<details><summary>actions/setup/js/safe_outputs_handlers.cjs:1942</summary>
**[/codebase-design]** The updated JSDoc comment for `updatePullRequestHandler` accurately describes the new behavior, but `addCommentHandler` above still has no equivalent comment explaining the soft-skip delegation. A reader unfamiliar with the change will see validation removed with no explanation.
<details>
<summary>💡 Suggestion</summary>
Add a brief comment above the `addCommentHandler` entry block similar to the one added to `updatePullRequestHandler`:
```js
// For target:triggering i…
</details>
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Test comment
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 20/100 — Poor. 100% implementation tests (threshold: ≤30%). This PR flips a critical behavioral contract (MCP hard-fail → downstream soft-skip) but tests only verify MCP-layer acceptance, not the downstream guarantee. Review flagged tests in the PR comment above.
There was a problem hiding this comment.
Review Summary
The behavior change is clearly motivated and the test updates are correct. Two existing inline comments already flag the main concern: removing the MCP-phase context gate for add_comment and update_pull_request also removes the independent SEC-005 (target_repo) validation that previously fired inside those blocks.
Additional observation: update_issue (~line 2180) still retains its context guard and early-rejects on non-issue events. With this PR, add_comment and update_pull_request diverge to the enqueue+runtime-soft-skip path. The asymmetry appears intentional but should be documented in a JSDoc comment or follow-up issue so future reviewers understand why the three handlers behave differently.
No new blocking issues beyond what the existing inline comments already surfaced.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.3 AIC · ⌖ 8.84 AIC · ⊞ 5.3K
Triage: refactor (safe-output behavior)Risk: medium | Priority: high (score 66/100) Breakdown: impact 30/50 (touches safe-output MCP/runtime hard-fail vs soft-skip behavior, cross-cutting) · urgency 21/30 (in-progress CI, active review thread, addresses real production mismatch) · quality 15/20 (detailed description, has reviewer comments to address, tests updated) Recommended action: Behavior-affecting change to safe-output enqueue/soft-skip semantics with existing reviewer feedback (2 review comments) — recommend expedited human review before merge.
|
|
@copilot please run the Reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30481139487
|
|
@copilot please run the Unresolved review threads:
|
|
@copilot implement the best systems so that the agent gets a feedback in the chat that it hasn't used the safeoutputs correctly and can recover |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. When |
|
@copilot please run the Unresolved review threads (newest first):
|
The two unresolved threads (r3677518165, r3677518154) are outdated — both show
All 194 tests in |
Triage: Align safe-output no-context behavior (soft-skip/hard-fail)
|
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30505477072
|
Both threads (r3677053921, r3677053881) are
The threads show |
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30507870059
|
Same status as previous sessions — both threads are |
Safe-output handling had an MCP/runtime mismatch: scheduled or context-free runs could hard-fail at MCP enqueue for
update_pull_request/add_comment, even though runtime handlers are designed to soft-skip unresolvedtarget: triggeringcases. This surfaced in PR Sous Chef mixed payloads (create_issue+update_pull_request+add_comment).Behavior change: remove MCP hard-stop for no-trigger context
actions/setup/js/safe_outputs_handlers.cjsadd_commentwithtarget: triggeringand no issue/PR/discussion contextupdate_pull_requestwithtarget: triggeringand no PR contextScope preserved
target: '*'still requires explicit item number fields)update_pull_requeststill requires one oftitle|body|update_branch)Targeted test expectation updates
actions/setup/js/safe_outputs_handlers.test.cjspush,schedule, andworkflow_dispatch(without context override) to assert successful enqueue foradd_commentandupdate_pull_requestinstead of MCP intent errors.