test(harness): complete the auth mock in the definition list wiring test [SAP-3214]

[antoine-berger]

Primary change type

• Bug fix
• Documentation
• Feature
• Tests
• Dependency update
• Maintenance or refactor

Problem and motivation

main is red since #880 merged: src/server/definition-list-enrichment.test.ts fails with No "credentialsFilePath" export is defined on the "@sapiom/mcp/auth" mock. The server started importing credentialsFilePath for the credential store observer in 4fd155b, which landed on main after #880 was rebased, so the PR's own CI passed while the merge commit does not.

Summary and scope

The test's vi.mock("@sapiom/mcp/auth") factory now spreads the real module and only replaces the credential store and the browser flow, with credentialsFilePath pointing at a store that does not exist (the observer sees ENOENT and stays quiet, nothing under ~/.sapiom is watched or written). A future import in the server can no longer turn this fake into a missing-export failure. Test file only, no runtime change.

Related work

Related issue or discussion: SAP-3214, follow-up to #880.

Validation

pnpm --filter @sapiom/mcp build (local dist was stale): rebuilt
pnpm --filter @sapiom/harness exec vitest run src/server/definition-list-enrichment.test.ts src/server/canvas-build-status-wiring.test.ts src/server/auth-mcp-wiring.test.ts: 17/17 passed on main b460c9aa plus this change
pnpm --filter @sapiom/harness typecheck (server): passed

Tests and documentation

The change is the test itself. Documentation: N/A, test-only change.

Compatibility and release impact

• Breaking or externally visible changes: None, test file only.
• Changeset: N/A, test-only change with no published package impact.

Security

• I have not included secrets, credentials, private data, or unsanitized logs.
• This pull request does not publicly disclose a suspected vulnerability. I
  will follow the
  Security Policy for
  private reporting.

AI assistance

• I did not use AI assistance for this change.
• I used AI assistance and have described it below.

Claude Code (Claude Fable 5.1) diagnosed the failing main run, wrote the mock change and ran the tests listed above. Verified by reading the diff and the passing run against current main.

Checklist

• I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
• This pull request addresses one focused problem and contains no unrelated cleanup.
• I added or updated tests, or explained above why tests are not applicable.
• I ran the relevant build, typecheck, lint, and test commands, or explained
  any N/A checks above.
• I updated documentation for user-facing changes, or marked it N/A above.
• I added a Changeset for a published-package change, or explained why it is not applicable.
• I can explain and maintain every submitted change, including any AI-assisted work.

Summary by CodeRabbit

• Tests
  • Improved automated test reliability for authentication-related scenarios, including handling cases where credentials are unavailable.
  • Expanded test isolation to ensure authentication behavior is validated consistently.
  • No user-facing functionality or public API changes are included in this update.

[coderabbitai]

Warning

Review limit reached

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: dfb2a208-a2ff-46d6-9464-36212b66f425

📥 Commits

Reviewing files that changed from the base of the PR and between 567945d and 2d2ca90.

📒 Files selected for processing (2)

• packages/harness/src/server/auth-mcp-wiring.test.ts
• packages/harness/src/server/definition-list-enrichment.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 265b41a3-f77e-4363-842f-202bd2f3dce1

📥 Commits

Reviewing files that changed from the base of the PR and between b460c9a and 567945d.

📒 Files selected for processing (1)

• packages/harness/src/server/definition-list-enrichment.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The test authentication mock now preserves real module exports, stubs credential and browser-auth behavior, and provides deterministic missing-credential and credential-path fixtures.

Changes

Authentication test mock

Layer / File(s)	Summary
Preserve auth exports and credential paths packages/harness/src/server/definition-list-enrichment.test.ts	The mock imports real authentication exports and overrides credential storage, browser authentication, related helpers, and credentialsFilePath.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 56794

This test-only change completes the authentication mock so credential observation remains isolated from user credentials. No runtime or release behavior is changed, and no merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description follows the repository template. It identifies Tests as the change type, explains the failing mock export, defines scope, links SAP-3214, lists validation commands and results, covers …
Title check	✅ Passed	The title clearly identifies a test-only change that completes the auth mock in the definition list wiring test. It is specific and directly matches the primary change.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch antoineberger/sap-3214-enrichment-test-auth-mock

---

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

_{A rabbit guards the auth-file door
Real exports hop safely through once more
Missing credentials wait in line
Mocked paths stay fixed and fine
Tests twitch their noses: all is sure}

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

[github-actions]

Review — PR #899 (round 1)

Test-only change to packages/harness/src/server/definition-list-enrichment.test.ts. Nothing published changes: harness ships files: ["dist", …] and tests are not in dist, so no changeset is needed and no prose in this diff reaches an npm consumer. No confidentiality issues — the diff names no company, no private host, and no business arrangement.

1. The same landmine is left armed in the sibling test — packages/harness/src/server/auth-mcp-wiring.test.ts:32

Only two tests boot startServer() behind a wholesale vi.mock("@sapiom/mcp/auth", () => ({ … })): the one fixed here, and auth-mcp-wiring.test.ts, whose factory still enumerates exports with no importOriginal spread. That is exactly the shape that reddened main when src/server/index.ts began importing credentialsFilePath — the next auth import added to the server (which already pulls resolveEnvironment, readCredentialsOrThrow, performBrowserAuth, credentialsFilePath from that module) fails there with the identical No "<export>" export is defined on the "@sapiom/mcp/auth" mock, and main goes red again the same way. The PR fixes one of the two instances of a class bug. Apply the same ...(await importOriginal<typeof import("@sapiom/mcp/auth")>()) spread to auth-mcp-wiring.test.ts — its authFixture already carries credentialsPath and every override it needs, so the spread is a one-line addition.

(The other three @sapiom/mcp/auth mocks — auth-routes.test.ts, profiles/system-prompt-fetch.test.ts, cli/auth.test.ts — import narrow modules rather than the server graph, so they are not exposed to this and do not need touching.)

2. The spread trades a loud failure for a silent real-filesystem read

This is the deliberate point of the change, but the cost is worth naming in the file: with the spread, a future server import of a new real export from @sapiom/mcp/auth no longer fails — it silently runs the real implementation, and every function in that module resolves through credentialsFilePath() = path.join(os.homedir(), ".sapiom", "credentials.json") (packages/mcp/src/credentials.ts:63). So the failure mode becomes "the test quietly reads or writes the developer's real credential store", while the file's own header (lines 1–6) still promises "@sapiom/mcp/auth mocked so the disconnect route never touches ~/.sapiom".

The repo already has the idiom that makes the real code path safe instead of merely absent — redirect the home directory: vi.mock("node:os", async (importOriginal) => ({ ...(await importOriginal()), homedir: () => tmpDir })) at packages/harness/src/core/inject/mcp-config.test.ts:8. With that, the real credentialsFilePath() already points into a temp dir, the explicit credentialsFilePath override and the authFixture.credentialsPath literal both become unnecessary, and any future real export stays contained. At minimum, update the header comment so it states the mock is now partial.

I verified the change's own claim holds today: the mocked path's parent ($TMPDIR/sap3214-enrichment-missing) does not exist, observeCredentialStore watches dirname(filePath) (packages/harness/src/core/credential-store-observer.ts:45), so the watch throws ENOENT, onError filters ENOENT, onUnavailable nulls the observer, and nothing is watched or written. The inline comment is accurate. I could not execute the suite here (build/test commands were not approved in this environment), so I did not independently reproduce the pass.

Nit

• process.env.TMPDIR ?? "/tmp" does not catch TMPDIR="" (path becomes root-relative); harmless here since the path must not exist either way, but the sibling test's plain /tmp/... literal is simpler and already the convention.

Verdict: correct, minimal fix for the red main; land it, then apply the same spread to auth-mcp-wiring.test.ts so the next auth import does not reintroduce the failure.

[github-actions]

Review — PR #899 (round 2, delta since 567945d1)

The push added the same importOriginal spread to auth-mcp-wiring.test.ts, rewrote the header comment, and dropped the TMPDIR expression. Still test-only, still nothing published, no confidentiality issues.

New finding — the containment claim in the new comment is false

packages/harness/src/server/definition-list-enrichment.test.ts:26-27: "A new real export would resolve paths through the mocked credentialsFilePath, so it stays contained."

It would not. vi.mock replaces the module's export binding; it does not rewrite calls made inside packages/mcp/src/credentials.ts, where readCredentialsFileOrThrow (line 82) and writeCredentialsFile (line 100) call the module-local credentialsFilePath() — the real one, path.join(os.homedir(), ".sapiom", "credentials.json") (line 63). Today every export of @sapiom/mcp/auth is overridden, so nothing real is reachable and the test is safe. The moment a new export is added to that module and left unmocked, it reads or writes the developer's (or the CI runner's) real credential store — and this comment tells the next reader it cannot. Either delete the claim and say plainly that safety depends on every export being overridden, or make it true by redirecting the home dir: vi.mock("node:os", async (importOriginal) => ({ ...(await importOriginal()), homedir: () => tmpDir })), the idiom already used at packages/harness/src/core/inject/mcp-config.test.ts:8.

Earlier findings

• Round-1 cleanup HTTP SDKs #1 (sibling auth-mcp-wiring.test.ts left unspread) — fixed, spread applied with overrides after ...actual.
• Round-1 update SDKs to use payment protocol data #2 (silent real-filesystem read once the mock is partial) — not fixed; the header now says PARTIAL, which was the minimum asked, but the mitigation was replaced by the incorrect rationale above.
• Round-1 nit (TMPDIR="") — fixed.
• Nothing in round 1 needs correcting.

Verdict: fix or delete the two-line rationale comment; the code itself is correct and ready to land.

[gwitwer]

Approving. Test-only, and the partial mock plus the node:os home-dir redirect is the right shape: a future auth import in the server can no longer become a missing-export failure, and anything left real stays under a temp home that does not exist.

One thing to update in the body: main is no longer red. #812 (merged 23:16Z) fixed the same failure the direct way, by adding credentialsFilePath: vi.fn() to this mock and pointing it at a temp path. I checked the merge result of this branch against current main: the file ends up with your spread plus that explicit override plus the home redirect, which is coherent and slightly belt-and-braces. Worth a one-line note so the next reader knows why both are there. Both server wiring tests and the full suite are green on main and on this head.