refactor(shared): replace vi.mock with injected dep in resolve-users

[ColeMurray]

What changed and why

packages/shared/src/slack/resolve-users.test.ts was using vi.mock("./client", ...) with vi.hoisted to swap out getUserInfo during tests. This violates the project's testing standards, which require real seams (constructor/parameter injection, in-memory adapters) rather than module-level mocking.

Approach

An optional deps parameter was added to resolveUserNames with a production default:

type GetUserInfo = (token: string, userId: string) => ReturnType<typeof defaultGetUserInfo>;

export async function resolveUserNames(
  token: string,
  userIds: string[],
  deps: { getUserInfo?: GetUserInfo } = {}
): Promise<Map<string, string>>

• The new parameter is optional with a default of {}, so all existing callers (e.g. slack-bot) continue to work without any changes.
• The production default is the real getUserInfo imported from ./client.

Test changes

• Removed all vi.mock, vi.hoisted, vi.fn, and vi.mocked usage.
• Removed the vi import entirely.
• Each test now defines a local inline fake getUserInfo function and passes it via deps.
• The "empty input" test tracks call count with a closure variable instead of a spy.
• All 6 original test scenarios are preserved with identical assertions.

Verification

• npm test -w @open-inspect/shared — 220 tests pass across 18 files.
• npx tsc --noEmit in packages/shared — no type errors.

---

Created with Open-Inspect

Summary by CodeRabbit

• Bug Fixes
  • Improved Slack user name resolution reliability, including correct fallbacks when display names are unavailable and clearer handling for missing or failed lookups.
  • More robust behavior for rejected requests and non-OK responses, ensuring the result map reflects the expected user identifier when details can’t be retrieved.
• Tests
  • Updated tests to use isolated per-case stubs, strengthening coverage for empty input, single-user fallback behavior, and distinct parallel user scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c6445832-d007-4d42-bbce-bf8058c483b9

📥 Commits

Reviewing files that changed from the base of the PR and between b293506 and 31a2e23.

📒 Files selected for processing (2)

• packages/shared/src/slack/resolve-users.test.ts
• packages/shared/src/slack/resolve-users.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/shared/src/slack/resolve-users.test.ts

---

📝 Walkthrough

Walkthrough

resolveUserNames now accepts an injectable getUserInfo argument with a default fallback, and the Slack resolver tests were rewritten to use per-test fakes instead of module-level mocking.

Changes

Dependency injection for resolveUserNames

Layer / File(s)	Summary
Type and parameter injection packages/shared/src/slack/resolve-users.ts	The imported getter is aliased as defaultGetUserInfo, a GetUserInfo type is exported, and resolveUserNames accepts a third getUserInfo argument with a default value.
Per-test fake resolver usage packages/shared/src/slack/resolve-users.test.ts	Tests pass injected fakes directly, covering display-name fallback, rejected calls, missing-user fallback, parallel lookups, real-name exclusion, and the empty-input no-call case.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐇 I hop through tests with fakes in tow,
And watch the Slack names rise and glow.
One tiny param, now neat and bright,
Makes each test clean and just right.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: replacing module-level vi.mock with dependency injection in resolve-users.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch open-inspect/68fba49c178432c3eded91ebd99a0a92

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Terraform Validation Results

Step	Status
Format	✅
Init	✅
Validate	✅

Note: Terraform plan was skipped because secrets are not configured. This is expected for external contributors. See docs/GETTING_STARTED.md for setup instructions.

Pushed by: @ColeMurray, Action: pull_request

[open-inspect]

Summary

Reviewed PR #822, refactor(shared): replace vi.mock with injected dep in resolve-users, by @ColeMurray. The change is limited to 2 files with 42 additions and 34 deletions, replacing module-level Vitest mocking with an optional dependency injection seam for resolveUserNames.

Critical Issues

None found.

Suggestions

None blocking. The injected dependency keeps production callers unchanged while making the test seam explicit.

Nitpicks

None.

Positive Feedback

• The production default preserves existing behavior for current callers.
• The tests now avoid module-level mocking and exercise behavior through an explicit dependency seam.
• Existing edge cases are preserved, including empty input, Slack error envelopes, thrown errors, and the real_name exclusion.

Questions

None.

Verification

• npm test -w @open-inspect/shared: 220 tests passed across 18 files.
• npx tsc --noEmit in packages/shared: passed.

Verdict

Approve.

[coderabbitai]

🧹 Nitpick comments (1)

packages/shared/src/slack/resolve-users.ts (1)

3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider exporting GetUserInfo to avoid duplicating the type in tests.

The test file redefines an equivalent GetUserInfo type (resolve-users.test.ts Line 5). Exporting it here lets the test import the single source of truth, preventing the two definitions from silently diverging.

♻️ Proposed change

-type GetUserInfo = (token: string, userId: string) => ReturnType<typeof defaultGetUserInfo>;
+export type GetUserInfo = (token: string, userId: string) => ReturnType<typeof defaultGetUserInfo>;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/shared/src/slack/resolve-users.ts` at line 3, Export the GetUserInfo
type from resolve-users so the tests can import the same source of truth instead
of redefining an equivalent local type. Update the type declaration near
defaultGetUserInfo usage to be exported, then adjust resolve-users.test.ts to
import GetUserInfo rather than duplicating its shape, keeping the test and
implementation aligned if the signature changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/shared/src/slack/resolve-users.ts`:
- Line 3: Export the GetUserInfo type from resolve-users so the tests can import
the same source of truth instead of redefining an equivalent local type. Update
the type declaration near defaultGetUserInfo usage to be exported, then adjust
resolve-users.test.ts to import GetUserInfo rather than duplicating its shape,
keeping the test and implementation aligned if the signature changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2f288d5e-cd4f-4395-87e8-31e8e723d858

📥 Commits

Reviewing files that changed from the base of the PR and between c935020 and b293506.

📒 Files selected for processing (2)

• packages/shared/src/slack/resolve-users.test.ts
• packages/shared/src/slack/resolve-users.ts

[github-actions]

Terraform Validation Results

Step	Status
Format	✅
Init	✅
Validate	✅

Note: Terraform plan was skipped because secrets are not configured. This is expected for external contributors. See docs/GETTING_STARTED.md for setup instructions.

Pushed by: @open-inspect[bot], Action: pull_request