Apply mention limits to output content instead of allowlists - #57747
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #57747 and found no actionable changed-line issues to comment on. 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
- "registry.npmjs.org"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail review: two spots where the new mention-limit plumbing adds a second abstraction for one caller. net: -18 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57747 · codex · mai10 · 7.77 AIC · ⌖ 0.442 AIC · ⊞ 14K
Comment /ponytail to run again
| * @returns {string} Processed string | ||
| */ | ||
| function neutralizeMentions(s, allowedLowercase) { | ||
| function neutralizeMentions(s, allowedLowercase, maxAllowed) { |
There was a problem hiding this comment.
actions/setup/js/sanitize_content.cjs:195-205: yagni: per-call mention-count state inside sanitizeContent. Drop the extra maxMentions limiter and let the caller pre-filter aliases once.
There was a problem hiding this comment.
Not applying this suggestion: pre-filtering aliases reintroduces the reported defect by making allowlist order decide authorization. The limiter must evaluate approved identities as they occur in the output; its state is intentionally local to a message/item.
| @@ -31,6 +31,7 @@ const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; | |||
| /** | |||
There was a problem hiding this comment.
actions/setup/js/safe_output_type_validator.cjs:31-39: yagni: generic ValidateOptions plumbing for a single maxMentions knob. Inline the single sanitizeContent call site instead of threading a new option through the validator layer.
There was a problem hiding this comment.
Not applying this simplification: validation sanitizes several fields and arrays for each safe-output type. Passing the existing validation options is necessary to share the per-item cap across those fields without changing validation behavior.
There was a problem hiding this comment.
🟡 Changes recommended
Per-message state, final-write enforcement, and the public type declaration remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves mention limits from authorization to per-output sanitization so complete allowlists remain valid.
Changes:
- Preserves all authorized identities while limiting unknown resolution candidates.
- Enforces distinct approved-mention limits during sanitization.
- Adds late-allowlist regression coverage.
File summaries
| File | Description |
|---|---|
actions/setup/js/sanitize_content.test.cjs |
Tests per-content mention limiting. |
actions/setup/js/sanitize_content.cjs |
Adds approved-alias counting. |
actions/setup/js/safe_output_type_validator.cjs |
Propagates mention limits during validation. |
actions/setup/js/resolve_mentions.test.cjs |
Tests known-author resolution behavior. |
actions/setup/js/resolve_mentions.cjs |
Limits only unknown resolution candidates. |
actions/setup/js/resolve_mentions_from_payload.test.cjs |
Tests complete allowlist preservation. |
actions/setup/js/resolve_mentions_from_payload.cjs |
Removes authorization-list truncation. |
actions/setup/js/collect_ndjson_output.test.cjs |
Adds large-allowlist regression coverage. |
actions/setup/js/collect_ndjson_output.cjs |
Reads and forwards configured maximums. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| const validationResult = validateItem(item, itemType, i + 1, { | ||
| allowedAliases: allowedMentions, | ||
| maxMentions, | ||
| maxBotMentions, |
There was a problem hiding this comment.
Reviewed: this is already addressed by 92deabd. validateItem now creates one allowedAliasesSeen set for the output item and supplies it to each field sanitizer; the collector’s safe-job path does the same. The regression test verifies the cap spans title and body while allowing a repeated approved identity.
| // If collaborator mentions are disabled, only use known authors (context + allowed list) | ||
| if (!allowCollaboratorMentions) { | ||
| core.info(`[MENTIONS] Collaborator mentions disabled - only allowing context (${deduplicatedKnownAuthors.length} users)`); | ||
| if (deduplicatedKnownAuthors.length > maxMentions) { | ||
| core.warning(`[MENTIONS] Mention limit exceeded: ${deduplicatedKnownAuthors.length} mentions, limiting to ${maxMentions}`); | ||
| } | ||
| return deduplicatedKnownAuthors.slice(0, maxMentions); | ||
| return deduplicatedKnownAuthors; |
There was a problem hiding this comment.
Addressed in 92deabd: all built-in final-write handlers and synthetic-update helpers parse and pass mentions.max to sanitizeContent, preserving the cap for newly assembled output.
| * @typedef {Object} SanitizeOptions | ||
| * @property {number} [maxLength] - Maximum length of content (default: 524288) | ||
| * @property {string[]} [allowedAliases] - List of aliases (@mentions) that should not be neutralized | ||
| * @property {number} [maxMentions] - Maximum number of unique allowed aliases to preserve | ||
| * @property {number} [maxBotMentions] - Maximum bot trigger references before filtering (default: 10) |
There was a problem hiding this comment.
Addressed in 92deabd: /home/runner/work/gh-aw/gh-aw/actions/setup/js/types/safe-output-script.d.ts now declares maxMentions?: number in SanitizeOptions.
|
@copilot Please address the current reviewer feedback, inspect the failing checks listed on this PR, refresh the branch if needed, and run the
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.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the reviewer findings in 92deabd: mention counts now span every field in an output item, final safe-output writes (including synthetic updates) enforce |
Inspected both failed jobs and reproduced their shared |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
/review |
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR #57747 review via submit_pull_request_review; no inline comments were needed. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Verdict
No blocking issues in the changed lines I could substantiate.
Notes
The mention-limit change appears to preserve mentions.max as runtime metadata while keeping the config-building and validation wiring consistent across the handler config, generated config JSON, and validation JSON. I discarded the sub-agent finding because it pointed at a lockfile line outside the reviewed changed source scope and did not map to a substantiated regression in this PR.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
api.github.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 114.5 AIC · ⌖ 7.24 AIC · ⊞ 21.8K
Comment /review to run again
|
@copilot Review this change and determine why it is so extensive. It initially seemed like a small feature, but it resulted in a substantial change. Identify why multiple features are being implemented. We should focus only on the minimal fix for the original issue. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and reduced in 0f1e40e. The 136-file diff was caused by branch-refresh merge conflict churn in 112 generated workflow lockfiles; those are removed, leaving 24 mention-related files. The remaining final-write enforcement and templatable parsing are required follow-ups from review and the earlier maintainer request, respectively. |
|
@copilot review #57747 (comment) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please take another pass on this PR: refresh the branch with the latest base, address any outstanding review or CI follow-up, and then continue with the 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.
|
Refreshed through latest |
|
@copilot Review the change with respect to the original reported issue and goal. Assess the quality of the fix. |
Reviewed against the original goal: the fix is correct. Authorization retains the full configured/context allowlist; only unknown-candidate resolution remains bounded. Sanitization limits distinct approved aliases encountered in each output item, does not charge disallowed aliases, and preserves repeats. Regression coverage includes the requested 60-user late-list case plus cross-field and final-write enforcement. Current CI’s completed build, lint, Go/JS tests, and typecheck jobs are green. |
safe-outputs.mentions.maxtruncated the configured allowlist during authorization, causing valid users later in large lists to be escaped. The limit should apply only to approved identities present in each output message.Authorization
Output filtering
max; disallowed identities do not consume the limit.Regression coverage
max: 3and a message mentioning only late-list users.Branch refresh requested by pr-sous-chef: https://github.com/github/gh-aw/actions/runs/33548654788
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/33581675739
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.