Register replace_label handler in safe-output collect job dispatch map#58917
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Category: bug | Risk: medium | Score: 71/100 (impact 35/50, urgency 22/30, quality 14/20) | Action: fast_track | Batch: workflow-reliability | Note: safe-output handler bug with regression tests and no CI yet.
|
|
Category: bug | Risk: medium | Score: 59 (impact 28, urgency 20, quality 13) | Action: fast_track | Batch: workflow-reliability
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🔵 Needs a closer look
Security-gate and exact handler-mapping coverage remain incomplete.
Pull request overview
Registers the replace_label safe-output handler and applies threat-warning abort handling.
Changes:
- Adds
replace_labelto handler dispatch and threat-warning classification. - Adds regression tests for registration and missing-handler behavior.
- Two testing nits remain: abort-policy coverage omits
replace_label, and the mapping assertion does not verify the exact key-value pair.
File summaries
| File | Description |
|---|---|
actions/setup/js/safe_output_handler_manager.test.cjs |
Adds registration and dispatch regression tests. |
actions/setup/js/safe_output_handler_manager.cjs |
Registers and classifies replace_label. |
Review details
Suppressed comments (2)
actions/setup/js/safe_output_handler_manager.cjs:202
- The new threat-warning classification is not covered by the existing parameterized abort-policy test, which currently exercises other mutation types but omits
replace_label. Addreplace_labelto that test so a future removal from this security gate cannot silently re-enable label mutation when detection concludes with a warning.
"replace_label",
actions/setup/js/safe_output_handler_manager.test.cjs:2374
- This assertion only proves that the filename appears somewhere in
HANDLER_MAP; it would still pass if the module were accidentally registered under the wrong key, while the functional test below manually constructs the handler map and therefore cannot catch that mistake. Since the regression is specifically thereplace_labelkey-to-module mapping, assert the exact pair (or exerciseloadHandlers({ replace_label: ... })).
expect(handlerFiles).toContain("replace_label.cjs");
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
@copilot Please take the next forward-progress pass on this PR.
Recent review blockers:
|
…place_label Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added exact key-to-module mapping test coverage for |
|
Category: bug | Risk: high | Score: 74/100 (impact 36/50, urgency 22/30, quality 16/20) | Action: fast_track | Batch: workflow-reliability | CI: unknown
|
@copilot Please take the next forward-progress pass on PR #58917.
|
Verified that both exact key-to-module mapping test coverage ( |
@copilot Please take the next forward-progress pass on PR #58917.
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
Re-checked current HEAD, branch merge state, and tests for PR #58917. The branch is fully up to date with |
|
✅ Great work! This PR is well-aligned with the project's guidelines and ready for review. Summary: This is a focused bug fix that registers the
The fix is minimal, focused, and includes regression tests to prevent future occurrences of the same issue. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
replace-labelsamples compiled and replayed successfully at the MCP layer, but the actual label mutation never ran — the safe output was silently dropped and fixture labels stayed unchanged.call-workflowanddispatch-workflowwere already fixed previously (dynamic MCP tool naming +HANDLER_MAPentries) and are confirmed working.Root cause
replace_label.cjsis a fully implemented handler, but it was missing fromHANDLER_MAPinactions/setup/js/safe_output_handler_manager.cjs— the map the collect job uses to load/dispatch handlers for each entry in the agent'soutputs.jsonl.GH_AW_SAMPLESgeneration, and the MCPtools/callreplay itself. Without aHANDLER_MAPentry, the collect job had nothing to invoke forreplace_labelentries, so they were skipped and no GitHub API mutation occurred.Changes
replace_label: "./replace_label.cjs"toHANDLER_MAPso the collect job loads and invokes it."replace_label"toTHREAT_WARNING_ABORT_TYPES, alongsideadd_labels/remove_labels, since it performs the same class of non-reviewable label mutation.safe_output_handler_manager.test.cjs:HANDLER_MAPcontainsreplace_label.cjsreplace_labelhandler is invoked without a "no handler loaded" error"No handler loaded for type 'replace_label'")Run: https://github.com/github/gh-aw/actions/runs/34042322138
$
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
github.comregistry.npmjs.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.