Preserve allowlisted mentions in add_comment sanitization (@copilot regression)#32683
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
add_comment sanitization (@copilot regression)
There was a problem hiding this comment.
Pull request overview
Fixes a regression where the add_comment handler re-sanitized comment bodies using a narrower mention allowlist than the workflow-level safe-outputs mention policy, causing allowlisted mentions (notably @copilot) to be escaped.
Changes:
- Propagate top-level
mentionspolicy into theadd_commenthandler viasafe_output_handler_manager. - Merge configured mention allowlist entries with parent entity authors when building
allowedAliasesforsanitizeContent, with@-prefix normalization and case-insensitive dedupe. - Add a regression test ensuring
@copilotremains unescaped when explicitly allowed.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_output_handler_manager.cjs | Passes workflow-level mentions config into the add_comment handler config. |
| actions/setup/js/add_comment.cjs | Merges configured allowed mention aliases with parent authors before the second sanitization pass. |
| actions/setup/js/add_comment.test.cjs | Adds a regression test for preserving @copilot when allowlisted. |
| .github/workflows/workflow-health-manager.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/weekly-blog-post-writer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/technical-doc-writer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/smoke-ci.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/sergo.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/security-compliance.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/pr-triage-agent.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/metrics-collector.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/glossary-maintainer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/firewall-escape.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/discussion-task-miner.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/developer-docs-consolidator.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/delight.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/deep-report.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-sentrux-report.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-news.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-community-attribution.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-code-metrics.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-cli-performance.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/daily-agent-of-the-day-blog-writer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-token-optimizer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-token-audit.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-session-insights.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-cli-deep-research.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/copilot-agent-analysis.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/audit-workflows.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
| .github/workflows/agent-performance-analyzer.lock.yml | Updates GH_AW_MEMORY_CONSTRAINTS text (max patch size “max” value). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 33/33 changed files
- Comments generated: 3
| @@ -362,6 +362,7 @@ async function main(config = {}) { | |||
| const maxCount = config.max || 20; | |||
| const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); | |||
| const includeFooter = parseBoolTemplatable(config.footer, true); | |||
| const configuredMentionAliases = Array.isArray(config.mentions?.allowed) ? config.mentions.allowed.map(alias => (typeof alias === "string" ? alias.trim().replace(/^@+/, "") : "")).filter(alias => alias.length > 0) : []; | |||
| // Pass top-level mentions policy through to add_comment so the handler can | ||
| // preserve the same allowed mention aliases used during collection. | ||
| if (type === "add_comment" && handlerConfig.mentions == null && config.mentions != null) { | ||
| handlerConfig.mentions = config.mentions; | ||
| } |
| GH_AW_GITHUB_WORKSPACE: ${{ github.workspace }} | ||
| GH_AW_MCP_CLI_SERVERS_LIST: '- `safeoutputs` — run `safeoutputs --help` to see available tools' | ||
| GH_AW_MEMORY_BRANCH_NAME: 'memory/meta-orchestrators' | ||
| GH_AW_MEMORY_CONSTRAINTS: "\n\n**Constraints:**\n- **Allowed Files**: Only files matching patterns: **\n- **Max File Size**: 102400 bytes (0.10 MB) per file\n- **Max File Count**: 100 files per commit\n- **Max Patch Size**: 51200 bytes (50 KB) total per push (max: 100 KB)\n" | ||
| GH_AW_MEMORY_CONSTRAINTS: "\n\n**Constraints:**\n- **Allowed Files**: Only files matching patterns: **\n- **Max File Size**: 102400 bytes (0.10 MB) per file\n- **Max File Count**: 100 files per commit\n- **Max Patch Size**: 51200 bytes (50 KB) total per push (max: 1024 KB)\n" |
![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 /diagnose and /tdd — this is a targeted bug fix with a regression test, which maps squarely to those two skills.
Key Themes
- Test coverage gaps: The regression test covers the happy path (
@copilotwith@prefix) but misses (a) the no-@normalization path and (b) the deduplication path whenparentAuthorsandmentions.allowedoverlap — both of which have dedicated logic in the implementation. mergeUniqueAliasesabstraction not extracted: The PR description describes the fix as a call tomergeUniqueAliases(parentAuthors, config.mentions?.allowed), but the implementation inlines the two-loop dedupe. A named helper would be directly unit-testable and make the call site self-documenting.- Config propagation generality: The
type === "add_comment"guard insafe_output_handler_manager.cjsis correct for now but creates an inconsistency that may need to be repeated for future handlers.
Positive Highlights
- ✅ Root cause is properly addressed at the right layer — config propagation from
safe_output_handler_managerrather than a band-aid inside the handler itself. - ✅ Alias normalization (
replace(/^@+/, "")) and case-insensitive deduplication are both correct and handle real-world input variance. - ✅ Regression test follows the established
eval-based test pattern and clearly demonstrates the before/after behaviour. - ✅ PR description is detailed and includes a clear before/after code snippet — easy to understand the intent.
Verdict
No blocking issues — the fix is correct and the regression test is meaningful. Suggestions above are improvements to test coverage and a modest refactor opportunity; none are required for merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.7M
| type: "add_comment", | ||
| body: "@copilot review all comments", | ||
| }; | ||
|
|
There was a problem hiding this comment.
[/tdd] The new test only exercises @copilot (with the @ prefix), but the normalization in add_comment.cjs also strips leading @ so both "@copilot" and "copilot" should resolve to the same alias. A complementary test case using mentions: { allowed: ["copilot"] } (no @) would lock down the normalization path and prevent a future regression if the .replace(/^@+/, "") logic changes.
it("should preserve `@copilot` mention when allowlist entry omits @ prefix", async () => {
// ... same setup ...
const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["copilot"] } }); })()`)
const result = await handler({ type: "add_comment", body: "`@copilot` review" }, {})
expect(capturedBody).toContain("`@copilot`")
expect(capturedBody).not.toContain("`@copilot`")
})| expect(capturedBody).toContain("@copilot"); | ||
| expect(capturedBody).not.toContain("`@copilot`"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The deduplication path (where an alias appears in both parentAuthors and mentions.allowed) isn't exercised. If parentAuthors already contains "copilot" and mentions.allowed also lists "@copilot", the merged list should still contain only one @copilot in the sanitized output. A test covering this would lock down the seenAllowedMentionAliases Set logic:
it("should not duplicate alias when it appears in both parentAuthors and mentions.allowed", async () => {
// payload where PR author is also in mentions.allowed
mockContext.payload = {
pull_request: { number: 1, user: { login: "copilot", type: "User" } },
}
const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["`@copilot`"] } }); })()`)
const result = await handler({ type: "add_comment", body: "`@copilot` thanks" }, {})
// `@copilot` should appear exactly once, not escaped
expect(capturedBody.match(/`@copilot/g`)).toHaveLength(1)
})| @@ -362,6 +362,7 @@ async function main(config = {}) { | |||
| const maxCount = config.max || 20; | |||
There was a problem hiding this comment.
[/diagnose] The PR description references a mergeUniqueAliases helper but the implementation inlines the two-loop deduplication instead. The inline approach works correctly, but the logic in add_comment.cjs (lines 562–585) is non-trivial enough that a named, exported helper would:
- Enable isolated unit tests for the merge/dedupe logic without spinning up the full handler
- Make the call site at line 590 read like the description's pseudocode
Not blocking — just noting the gap between the stated design and what landed.
| // Call the factory function with config to get the message handler | ||
| const handlerConfig = { ...(config[type] || {}) }; | ||
|
|
||
| // Pass top-level mentions policy through to add_comment so the handler can |
There was a problem hiding this comment.
[/diagnose] The type === "add_comment" guard is a targeted fix that works for the immediate regression, but config propagation is now inconsistent: add_comment gets mentions forwarded; other handlers that may grow similar needs in the future won't. An alternative that generalises without requiring per-type patches:
// Merge top-level keys that the handler config doesn't already override
const handlerConfig = { ...config, ...(config[type] || {}) };This follows the same shallow-merge pattern already used elsewhere and removes the need for the handler-type guard. Worth considering if a second handler ever needs a top-level field.
🧪 Test Quality Sentinel ReportTest Quality Score: 70/100
Test Classification Details
Flagged Tests — Suggestions
|
|
|
|
@copilot review all comments and review comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in
|
add_commentwas re-sanitizing comment bodies with a local allowlist that omitted workflow-level mention policy, so@copilotwas escaped even when explicitly allowed (e.g., PR Sous Chef). This change aligns handler-time sanitization with configured safe-output mention policy.Config propagation
mentionsconfiguration into theadd_commenthandler viasafe_output_handler_manager, instead of limiting handler context toadd_comment-scoped fields.Allowlist merge in
add_commentallowedAliasesfrom:mentions.allowedentries (new behavior).Alias normalization
copilotand@copilotresolve to the same alias before sanitization.Behavioral effect
@copilotremains unescaped in posted comments.