[jsweep] Clean add_comment.cjs

[github-actions]

Summary

Cleaned actions/setup/js/add_comment.cjs as part of the daily jsweep unbloat cycle.

Context Type

GitHub Script (github-script) — uses core.info/warning/error, github.rest.*, github.graphql(), and globals like context.

Changes Made

Extracted deduplicateCaseInsensitive helper (main improvement):

Replaced two near-identical 8-line deduplication loops for allowedMentionAliases with a single, readable helper function:

function deduplicateCaseInsensitive(aliases) {
  const seen = new Set();
  return aliases.filter(alias => {
    const key = alias.toLowerCase();
    if (seen.has(key)) return false;
    seen.add(key);
    return true;
  });
}

The call site becomes a clear one-liner:

const allowedMentionAliases = deduplicateCaseInsensitive([...parentAuthors, ...configuredMentionAliases]);

This removes ~16 lines of repetitive loop code while preserving identical behavior: case-insensitive deduplication, original casing preserved, insertion order maintained, parentAuthors take precedence over configuredMentionAliases.

Test Improvements

Added 2 new tests to add_comment.test.cjs in the parent author mention allowlist describe block:

1. should deduplicate allowed aliases case-insensitively across parentAuthors and configured mentions — verifies that when the issue author "Alice" and configured alias "alice" overlap (case-insensitive), only one entry is preserved, while unique alias "Bob" is included.
2. should preserve order: parentAuthors first, then configuredMentionAliases — verifies that parentAuthors and configuredMentionAliases are all preserved when there are no duplicates.

Total test count: 106 tests (was 104).

Validation ✅

• Formatting: npm run format:cjs ✓
• Linting: npm run lint:cjs ✓
• Type checking: npm run typecheck ✓
• Tests: npm run test:js -- --no-file-parallelism add_comment → 106 passed ✓

Generated by 🧹 jsweep - JavaScript Unbloater · ● 16.2M · ◷

• expires on May 20, 2026, 5:07 AM UTC

[Copilot]

Pull request overview

This PR refactors mention allowlist deduplication in add_comment.cjs and adds tests around parent author/configured mention interactions.

Changes:

• Extracts case-insensitive alias deduplication into a helper.
• Uses the helper to combine parent authors with configured allowed mentions.
• Adds tests for deduplication and ordering scenarios.

Show a summary per file

File	Description
actions/setup/js/add_comment.cjs	Adds and applies deduplicateCaseInsensitive for allowed mention aliases.
actions/setup/js/add_comment.test.cjs	Adds tests for configured mention aliases combined with parent authors.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

actions/setup/js/add_comment.test.cjs:2837

• This test name claims to preserve ordering, but these assertions only check that each mention survived sanitization; they do not observe the order of allowedMentionAliases, so reversing the merge order would still pass. Please assert the allowlist log/order or test the helper directly so this protects the behavior described by the PR.

      expect(capturedBody).toContain("@Charlie");
      expect(capturedBody).toContain("@Dave");
      expect(capturedBody).toContain("@Eve");

• Files reviewed: 2/2 changed files
• Comments generated: 1

[github-actions]

Skills-Based Review 🧠

Applied /tdd — this is a refactor + cleanup PR with accompanying tests.

Key Themes

• One minor test gap: The should preserve order test asserts presence but not order (see inline comment). It would be easy to strengthen.

Positive Highlights

• ✅ deduplicateCaseInsensitive is a clean, readable extraction — replaces ~16 lines of duplicated imperative loop code with a well-named, composable function
• ✅ JSDoc on the helper is clear and accurate
• ✅ Tests exercise the behavior through the public integration path rather than testing internals
• ✅ Full validation (format, lint, typecheck, 106 tests passing) confirmed in PR description

Verdict

Approving — the refactor is correct and the code is meaningfully cleaner. The inline suggestion is a nice-to-have, not a blocker.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3.8M

[github-actions]

[/tdd] The test name promises ordering verification (parentAuthors first, then configuredMentionAliases) but the assertions only check presence — they do not verify that Charlie appears before Dave/Eve in the final allowedMentionAliases list.

If the ordering guarantee matters (e.g. parentAuthors always take precedence), consider asserting order directly via the core.info log message:

// Capture info logs to assert ordering
const infos = [];
mockCore.info = msg => infos.push(msg);
// ... run handler ...
const infoLog = infos.find(m => m.startsWith('[MENTIONS] Allowing aliases'));
const aliases = infoLog.replace('[MENTIONS] Allowing aliases in comment: ', '').split(', ');
expect(aliases.indexOf('Charlie')).toBeLessThan(aliases.indexOf('Dave'));
expect(aliases.indexOf('Charlie')).toBeLessThan(aliases.indexOf('Eve'));

As written the test passes even if configuredMentionAliases were accidentally prepended before parentAuthors, making it a weaker specification than its name implies.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 75/100

⚠️ Acceptable — with suggestions

Metric	Value
New/modified tests analyzed	2
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	1 (50%)
Duplicate test clusters	0
Test inflation detected	Yes (61 test lines added / 16 production lines added ≈ 3.8:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Issues Detected
should deduplicate allowed aliases case-insensitively...	actions/setup/js/add_comment.test.cjs	✅ Design	Covers edge case (case-insensitive dedup); no error path tested
should preserve order: parentAuthors first, then configuredMentionAliases	actions/setup/js/add_comment.test.cjs	✅ Design	Assertions do not actually verify ordering; only presence is checked

---

Flagged Tests — Requires Review

⚠️ should preserve order: parentAuthors first, then configuredMentionAliases

Classification: Design test (minor assertion gap)
Issue: The test name promises it verifies ordering of parentAuthors before configuredMentionAliases, but the three assertions (toContain("@charlie"), toContain("@dave"), toContain("@eve")) only confirm presence, not order. A bug that reversed the two arrays would not be caught.
What design invariant does this test enforce? That all three mention aliases appear in the final comment body — but not that their relative order is preserved.
Suggested improvement: Assert the relative position of the mentions, e.g.:

const charliePos = capturedBody.indexOf("`@Charlie`");
const davePos    = capturedBody.indexOf("`@Dave`");
expect(charliePos).toBeLessThan(davePos);

---

Score Calculation

Component	Score
Behavioral coverage: 2/2 design tests × 40	40
Edge/error coverage: 1/2 tests with edge cases × 30	15
Low duplication: 0 duplicate clusters	20
Proportional growth: inflation ratio 3.8:1 (>2:1)	0
Total	75

i️ Test inflation note: 61 lines were added to the test file versus 16 production lines added (the new deduplicateCaseInsensitive helper). This triggers the inflation flag mechanically, but the refactoring extracted meaningful logic that genuinely warrants dedicated tests — the flag is informational here.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 2 tests (vitest)
• 🐹 Go (*_test.go): 0 tests

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both new tests verify observable behavioral contracts of the extracted deduplicateCaseInsensitive helper. The ordering test has a minor assertion gap (see above) but is not severe enough to fail the check.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References:

• §26033825745

🧪 Test quality analysis by Test Quality Sentinel · ● 5.6M · ◷

[github-actions]

Test Quality Sentinel: 75/100. Test quality is acceptable - 0% of new tests are implementation tests (threshold: 30%). Both tests verify behavioral contracts of the new deduplicateCaseInsensitive helper. Minor: the ordering test only asserts presence, not sequence - see comment for improvement suggestion.

[github-actions]

@copilot review all comments and address the unresolved review feedback on this PR.

Generated by 👨‍🍳 PR Sous Chef · ◷

[github-actions]

Please tighten the remaining assertion and post a short completion plan after that lands.

Generated by 👨‍🍳 PR Sous Chef · ◷

[pelikhan]

@copilot review all comments and reviews

[Copilot]

@copilot review all comments and reviews

Done. I addressed the review feedback by updating the two tests to assert observable allowlist behavior via core.info ([MENTIONS] Allowing aliases in comment: ...) so case-insensitive deduplication and ordering are explicitly verified. Included in commit fc28feb.