Reduce Smoke CI REST churn by caching mention-resolution collaborator lookups

[Copilot]

Smoke CI is the largest recurring REST consumer (~1,465 calls/run), with repeated read-heavy calls being a primary optimization target. This change reduces avoidable mention-resolution API traffic without changing smoke coverage behavior.

• What changed

  • Added repo-scoped in-process caches in actions/setup/js/resolve_mentions.cjs for:
    • repos.listCollaborators results
    • repos.getCollaboratorPermissionLevel results per username
  • Added an early exit when input text contains no mentions to skip collaborator/permission lookups entirely.
  • Exposed a test reset hook for deterministic cache behavior across unit tests.

• Why this reduces API footprint

  • Mention resolution is invoked by multiple safe-output paths in a single run.
  • Repeated collaborator/permission reads for the same repo/user now deduplicate to one lookup per process (or zero when no mentions are present).

• Targeted test updates

  • Added coverage to assert:
    • no API calls occur when there are no mentions
    • repeated resolution calls reuse collaborator cache (single collaborator-list call)

// Before: listCollaborators called on each resolution pass
await resolveMentionsLazily("Hello @maintainer1", [], "owner", "repo", github, core);
await resolveMentionsLazily("Hello @maintainer1", [], "owner", "repo", github, core);

// After: second call reuses in-memory repo cache
expect(github.rest.repos.listCollaborators).toHaveBeenCalledTimes(1);

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #36960 does not have the implementation label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). Neither Condition A nor Condition B is met.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 85/100 — Excellent

Analyzed 2 test(s): 2 design, 0 implementation, 0 guideline violation(s).

📊 Metrics & Test Classification (2 tests analyzed)

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	No (0.32 ratio — 17 test lines / 53 production lines)
🚨 Coding-guideline violations	0

Test Classification Details

Test	File	Classification	Issues Detected
should skip collaborator lookups when text has no mentions	actions/setup/js/resolve_mentions.test.cjs	✅ Design	Covers empty-input edge case; verifies no API calls are made
should reuse cached collaborator list across repeated resolution calls	actions/setup/js/resolve_mentions.test.cjs	✅ Design	Verifies cache hit prevents redundant API calls; second-call output not re-asserted (minor)

Language Support

Tests analyzed:

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

⚠️ Flagged Tests — Requires Review (1 minor suggestion)

💡 should reuse cached collaborator list across repeated resolution calls (actions/setup/js/resolve_mentions.test.cjs)

Classification: Design test
Minor gap: The test asserts listCollaborators was called exactly once (verifying the cache contract), but does not assert that the return value of the second resolveMentionsLazily call is still correct. A bug that returns stale or empty data from the cache would go undetected.
What design invariant does this test enforce? That the collaborator API is not called more than once per repo — the core caching promise of this PR.
What would break if deleted? The caching feature could regress silently (API called on every call) without failing CI.
Suggested improvement: Add an assertion on the return value of the second call, e.g. expect(result2.allowedMentions).toEqual(["maintainer1"]), to confirm the cached path returns correct data.

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both tests directly exercise the behavioral contract introduced by this PR: the early-exit for mention-free text and the single-fetch caching guarantee.

📖 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: §26968772281

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 1.1M · ◷

[github-actions]

✅ Test Quality Sentinel: 85/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both tests directly verify the behavioral contracts introduced by this PR.

[github-actions]

Skills-Based Review 🧠

Applied /tdd, /diagnose, and /improve-codebase-architecture — the caching strategy is well-targeted and the early-exit optimisation is clean. Three areas worth a follow-up pass before this pattern spreads.

📋 Key Themes & Highlights

Key Themes

• userPermissionCache untested (/tdd): The two new tests exercise getRecentCollaborators cache-hit and the no-mention short-circuit, but the checkUserPermission fallback path — and its bot/error memoisation — has no cache-hit coverage.
• Transient errors permanently cached (/diagnose): The catch block in checkUserPermission stores false unconditionally; a 503 or rate-limit response during a run silently denies a valid contributor for the entire process.
• Test-only reset hook in production exports (/improve-codebase-architecture): resetMentionResolutionCaches is wired into beforeEach and exported alongside production functions, leaking test infrastructure through the public API. A factory or injected-cache approach would avoid this.

Positive Highlights

• ✅ Early exit for mention-free text is clean and well-tested — zero API calls for the common case
• ✅ getRepoCacheKey normalises to lowercase for correct case-insensitive matching
• ✅ permissionsForRepo initialised with cachedPermissions || new Map() correctly preserves existing repo entries on a per-user miss
• ✅ Error path is memoised (good intent — prevents retry storms on 404s); just needs the transient-vs-deterministic distinction
• ✅ resetMentionResolutionCaches wired into beforeEach keeps all existing tests isolated

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.1M

[github-actions]

[/diagnose] Any transient API failure (rate limit, 503, network blip) in this catch block is cached as a permanent false for the rest of the process — the same as a deterministic "user not found" result. A retry that would succeed is silently suppressed.

💡 Suggested fix: only cache deterministic failures

} catch (error) {
  // Only cache 404 (user doesn’t exist) — deterministic, safe to memoize
  const status = error?.status ?? error?.response?.status;
  if (status === 404) {
    permissionsForRepo.set(usernameKey, false);
    userPermissionCache.set(repoCacheKey, permissionsForRepo);
  }
  // Transient errors (5xx, network): fall through without caching so a later
  // call within the same run gets a fresh attempt.
  return false;
}

In a short-lived Action process the blast radius is limited to one run, but this could silently deny a valid contributor if the first lookup races against a transient outage.

[github-actions]

[/improve-codebase-architecture] Two module-level Map instances give the caches process lifetime, which is correct for a single-run Action but couples the module to a global singleton. The only way to reset them is resetMentionResolutionCaches, which is a test-only hook that leaks through the public module.exports.

💡 Alternatives that avoid exporting test infrastructure

Option A — factory function (minimal change, caches scoped to the resolver instance):

function createMentionResolver() {
  const collaboratorsCache = new Map();
  const permissionCache = new Map();
  return {
    resolveMentionsLazily: (text, knownAuthors, owner, repo, github, core) =>
      _resolveMentionsLazily(text, knownAuthors, owner, repo, github, core,
                             collaboratorsCache, permissionCache),
  };
}
// Tests just call createMentionResolver() per test — no reset hook needed.

Option B — injected cache bag (most explicit):
Pass { collaboratorsCache, permissionCache } as a parameter with a sensible default (module-scoped Maps for production, fresh Maps in tests). This avoids exporting resetMentionResolutionCaches entirely.

The current approach works, but the reset-hook-in-exports pattern is a signal worth addressing before it multiplies.

[github-actions]

[/tdd] The two new tests cover getRecentCollaborators cache-hit and the no-mention early exit, but userPermissionCache — the more complex code path — has no cache-hit test. This path is reached when a mentioned user is absent from the collaborator list and falls through to checkUserPermission, which also caches bot results, permission levels, and errors.

💡 Suggested test skeleton

it("should reuse cached permission when user is not in collaborator list", async () => {
  // User not in collaborators → falls through to checkUserPermission
  mockGithub.rest.repos.listCollaborators.mockResolvedValue({ data: [] });
  mockGithub.rest.users.getByUsername.mockResolvedValue({ data: { login: "external1", type: "User" } });
  mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue(
    { data: { permission: "write" } }
  );

  await resolveMentionsLazily("Hello `@external1`", [], "owner", "repo", mockGithub, mockCore);
  await resolveMentionsLazily("Hello `@external1`", [], "owner", "repo", mockGithub, mockCore);

  // Second resolution should hit userPermissionCache, not call the API again
  expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalledTimes(1);
});

Without this, the error-path memoisation (catch → false) and bot-user caching are also untested.

[pelikhan]

@copilot review all comments and review, lint js, typecheck, apply copilot-review

[pelikhan]

@copilot review all comments and review, lint js, typecheck, apply copilot-review