fix: pass mentions config to add_comment handler so allowed mentions aren't escaped#32805
Conversation
…add_comment respects allowed mentions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…g map Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes safe-output handler configuration so global mention allowlists can be forwarded to the add_comment handler.
Changes:
- Adds top-level
mentionstoGH_AW_SAFE_OUTPUTS_HANDLER_CONFIG. - Adds Go test coverage for mention propagation into handler config.
- Updates the
pr-sous-chefcompiled workflow lock.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_safe_outputs_config.go |
Emits global mentions config for handler manager consumption. |
pkg/workflow/add_comment_target_repo_test.go |
Verifies mentions are present or omitted in handler config as expected. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerated handler config includes the mentions allowlist. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| GITHUB_SERVER_URL: ${{ github.server_url }} | ||
| GITHUB_API_URL: ${{ github.api_url }} | ||
| GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"add_comment\":{\"max\":20,\"target\":\"*\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"report_incomplete\":{},\"update_pull_request\":{\"allow_body\":true,\"allow_title\":false,\"default_operation\":\"append\",\"max\":10,\"target\":\"*\",\"update_branch\":true}}" | ||
| GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"add_comment\":{\"max\":20,\"target\":\"*\"},\"create_report_incomplete_issue\":{},\"mentions\":{\"allowed\":[\"copilot\"]},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"report_incomplete\":{},\"update_pull_request\":{\"allow_body\":true,\"allow_title\":false,\"default_operation\":\"append\",\"max\":10,\"target\":\"*\",\"update_branch\":true}}" |
| func buildMentionsHandlerConfig(m *MentionsConfig) map[string]any { | ||
| cfg := make(map[string]any) | ||
| if m.Enabled != nil { | ||
| cfg["enabled"] = *m.Enabled | ||
| } | ||
| if m.AllowTeamMembers != nil { | ||
| cfg["allowTeamMembers"] = *m.AllowTeamMembers | ||
| } | ||
| if m.AllowContext != nil { | ||
| cfg["allowContext"] = *m.AllowContext | ||
| } | ||
| if len(m.Allowed) > 0 { | ||
| cfg["allowed"] = m.Allowed | ||
| } | ||
| if m.Max != nil { | ||
| cfg["max"] = *m.Max | ||
| } |
| // Include top-level mentions configuration so the handler manager can pass it to | ||
| // the add_comment handler (which calls sanitizeContent with the allowed aliases). | ||
| if safeOutputs.Mentions != nil { | ||
| mentionsCfg := buildMentionsHandlerConfig(safeOutputs.Mentions) | ||
| if len(mentionsCfg) > 0 { | ||
| config["mentions"] = mentionsCfg |
|
@copilot review all comments, avoid duplicate code |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Test Analysis
All three rows test observable output (the compiled step JSON) rather than internal implementation details — a strong behavioral contract test. Build tag: ✅ Test Inflation NoteThe test file gained 99 lines vs. 38 lines added to the production file ( 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. References: §25991308933
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…puts handler config Generated by the Design Decision Gate workflow. Documents the decision to allow non-handler top-level keys (e.g. `mentions`) in GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG, widening the Go config map type to `map[string]any` and activating existing runtime forwarding logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures the structural decision implied by this PR: widening the Go config map from What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why this PR tripped the gateThe gate is volume-based when no 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References:
|
… regenerate locks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 94610ff:
|
mentions.allowedentries (e.g.@copilotinpr-sous-chef) were being escaped inadd_commentoutput because the Go compiler never included the globalmentionskey inGH_AW_SAFE_OUTPUTS_HANDLER_CONFIG. The handler manager already had forwarding logic for this — it was just a dead path.Changes
compiler_safe_outputs_config.go—addHandlerManagerConfigEnvVarnow emits"mentions"at the top level ofGH_AW_SAFE_OUTPUTS_HANDLER_CONFIG, activating the existing forwarding insafe_output_handler_manager.cjs:Config map type widened from
map[string]map[string]any→map[string]anyto accommodate non-handler top-level keys.add_comment_target_repo_test.go—TestAddCommentMentionsInHandlerConfigverifies the mentions config is present in the handler config JSON and correctly structured.pr-sous-chef.lock.yml— Regenerated;GH_AW_SAFE_OUTPUTS_HANDLER_CONFIGnow contains"mentions":{"allowed":["copilot"]}.