Tests: dedupe contract helper plumbing by vincentkoc · Pull Request #48760 · openclaw/openclaw

vincentkoc · 2026-03-17T05:45:30Z

Summary

Problem: the global contract layer still had duplicated provider test helpers and a three-file inbound capture shim chain.
Why it matters: the contract tree is supposed to be the deduped source of truth, but these helpers still repeated logic and import indirection.
What changed: extracted shared provider contract test helpers, moved provider compat normalization into the registry, and collapsed the inbound contract capture helpers into one testkit module.
What did NOT change (scope boundary): no runtime product behavior changed; this is test and contract cleanup only.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related Tests: centralize contract coverage follow-ups #48751

User-visible / Behavior Changes

None.

Security Impact (required)

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: local workspace
Model/provider: N/A
Integration/channel (if any): plugin/channel contract tests
Relevant config (redacted): default local dev config

Steps

Extract provider contract test helper duplication into one shared module.
Collapse the inbound contract capture helper chain into one testkit.
Run focused static verification on the touched files.

Expected

The contract cleanup reduces duplication without changing contract behavior.

Actual

The provider contract tests now reuse one shared helper module.
The inbound contract test path now uses one shared inbound-testkit.ts instead of three thin files.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: rebased onto latest origin/main; ran pnpm exec oxlint on the touched cleanup files.
Edge cases checked: ensured the branch diff against origin/main only contains the helper-extraction and inbound-testkit collapse.
What you did not verify: the focused pnpm test -- ... lane again stalled at Vitest startup in this checkout, so I do not have a completed test run for this slice.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? Yes
Config/env changes? No
Migration needed? No
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert this PR.
Files/config to restore: src/plugins/contracts/testkit.ts, src/plugins/contracts/registry.ts, and src/channels/plugins/contracts/inbound-testkit.ts plus the replaced import sites.
Known bad symptoms reviewers should watch for: contract tests failing to resolve provider registration helpers or inbound context captures.

Risks and Mitigations

Risk:
- The helper extraction could accidentally change test wiring if one contract depended on local helper behavior.
- Mitigation: the cleanup is confined to the contract/test layer and keeps the same call sites and assertions.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5110a8150

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-17T05:51:38Z

extensions/discord/src/monitor/message-handler.inbound-context.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { inboundCtxCapture as capture } from "../../../../src/channels/plugins/contracts/inbound-contract-dispatch-mock.js";
+import { inboundCtxCapture as capture } from "../../../../src/channels/plugins/contracts/inbound-testkit.js";


Restore dispatch mock setup for Discord inbound test

Switching this import from inbound-contract-dispatch-mock to inbound-testkit drops the side-effect vi.mock(".../auto-reply/dispatch.js") that used to populate inboundCtxCapture.ctx. In this test file there is no replacement mock, so processDiscordMessage calls the real dispatch path and capture.ctx never gets set, making assertions like expect(capture.ctx).toBeTruthy() fail whenever this test runs.

Useful? React with 👍 / 👎.

greptile-apps · 2026-03-17T05:51:54Z

Greptile Summary

This PR is a test/contract-layer refactor that consolidates three thin inbound-capture shim files into inbound-testkit.ts and extracts duplicated registerProviders/requireProvider helpers from four contract test files into src/plugins/contracts/testkit.ts. The kimi-coding → kimi compat normalisation is also lifted from an inline test function into registry.ts as providerContractCompatPluginIds.

The deduplication intent is correct and most of the changes are clean, but the inbound-testkit consolidation introduces two test regressions caused by a missing vi.mock():

extensions/discord/src/monitor/message-handler.inbound-context.test.ts — The old inbound-contract-dispatch-mock.ts registered a vi.mock("../../../auto-reply/dispatch.js", ...) as a module-level side effect; importing it was enough to set up the mock. inbound-testkit.ts intentionally omits this side effect, but the Discord test file has no vi.mock() of its own, so dispatch.js is never mocked, capture.ctx stays undefined, and both test assertions fail.
src/channels/plugins/contracts/inbound.contract.test.ts line 192 — The local vi.mock() in this file writes captured context to signalCapture.ctx, yet the Discord-specific test case asserts inboundCtxCapture.ctx. With the helper module's side-effect mock now removed, inboundCtxCapture.ctx is never populated and the assertion fails.

The PR author explicitly notes no completed test run was obtained for this slice ("the focused pnpm test -- ... lane again stalled at Vitest startup"), which is consistent with these failures going undetected.

Confidence Score: 2/5

Not safe to merge — two test files have lost their dispatch.js mock wiring and will produce failing assertions on CI.
The structural deduplication is correct, but the inbound-testkit consolidation removed a vi.mock() side effect that two test files depended on without adding equivalent vi.mock() calls in those files. The Discord extension test has no mock at all, and the inbound.contract.test.ts Discord test case asserts a capture object that the active mock never writes to. The author confirmed tests were not run. No runtime product code was changed, so the scope is limited to test correctness.
extensions/discord/src/monitor/message-handler.inbound-context.test.ts (no vi.mock() after import swap) and src/channels/plugins/contracts/inbound.contract.test.ts line 192 (asserts inboundCtxCapture.ctx but active mock only writes signalCapture.ctx).

Comments Outside Diff (1)

src/channels/plugins/contracts/inbound.contract.test.ts, line 192-193 (link)

inboundCtxCapture.ctx is never written by the active mock

The vi.mock() block defined in this file (lines 31–39) replaces dispatchInboundMessage with dispatchInboundMessageMock, which captures the context into signalCapture.ctx — not inboundCtxCapture.ctx. The old inbound-contract-dispatch-mock.ts had its own module-level vi.mock() that used buildDispatchInboundContextCapture to write into inboundCtxCapture.ctx. Since inbound-testkit.ts no longer carries that side-effect mock, no code path sets inboundCtxCapture.ctx, so this assertion will fail:

expect(inboundCtxCapture.ctx).toBeTruthy(); // → ctx remains undefined → FAILS

The "keeps Signal inbound context finalized" test (lines 196–217) correctly checks signalCapture.ctx (what the local mock actually writes). The Discord test case here should either:

Switch to checking signalCapture.ctx (which the active mock already writes), or
Update dispatchInboundMessageMock to also write inboundCtxCapture.ctx = params.ctx.

Using signalCapture.ctx is the simpler fix since that object is already populated:

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/channels/plugins/contracts/inbound.contract.test.ts
Line: 192-193

Comment:
**`inboundCtxCapture.ctx` is never written by the active mock**

The `vi.mock()` block defined in this file (lines 31–39) replaces `dispatchInboundMessage` with `dispatchInboundMessageMock`, which captures the context into `signalCapture.ctx` — not `inboundCtxCapture.ctx`. The old `inbound-contract-dispatch-mock.ts` had its own module-level `vi.mock()` that used `buildDispatchInboundContextCapture` to write into `inboundCtxCapture.ctx`. Since `inbound-testkit.ts` no longer carries that side-effect mock, no code path sets `inboundCtxCapture.ctx`, so this assertion will fail:

```
expect(inboundCtxCapture.ctx).toBeTruthy(); // → ctx remains undefined → FAILS
```

The "keeps Signal inbound context finalized" test (lines 196–217) correctly checks `signalCapture.ctx` (what the local mock actually writes). The Discord test case here should either:

1. Switch to checking `signalCapture.ctx` (which the active mock already writes), or
2. Update `dispatchInboundMessageMock` to also write `inboundCtxCapture.ctx = params.ctx`.

Using `signalCapture.ctx` is the simpler fix since that object is already populated:



How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: extensions/discord/src/monitor/message-handler.inbound-context.test.ts
Line: 2

Comment:
**Missing `vi.mock()` — `capture.ctx` will always be `undefined`**

The old `inbound-contract-dispatch-mock.ts` module contained a module-level `vi.mock("../../../auto-reply/dispatch.js", ...)` call that was executed as a side effect of importing it. This mock intercepted `dispatchInboundMessage` and set `inboundCtxCapture.ctx` on each call.

The new `inbound-testkit.ts` consolidates all the helper utilities but deliberately omits this `vi.mock()` call. As a result, `dispatch.js` is no longer mocked in this test file — there is no `vi.mock()` here at all. When `processDiscordMessage(messageCtx)` runs, the real (unmocked) `dispatchInboundMessage` executes and nothing writes to `capture.ctx`. Both test assertions will then fail:

```
expect(capture.ctx).toBeTruthy(); // → capture.ctx is undefined → FAILS
expectInboundContextContract(capture.ctx!);  // → called with undefined
```

The Signal extension test (`event-handler.mention-gating.test.ts`) correctly handles this by calling `vi.mock()` directly in the test file and using `buildDispatchInboundCaptureMock`. The same pattern needs to be applied here:

```typescript
vi.mock("../../../../src/auto-reply/dispatch.js", async (importOriginal) => {
  const actual = await importOriginal<typeof import("../../../../src/auto-reply/dispatch.js")>();
  return buildDispatchInboundCaptureMock(actual, (ctx) => {
    capture.ctx = ctx as MsgContext;
  });
});
```

Note the path depth — this test lives under `extensions/discord/src/monitor/`, four levels from the repo root, so the relative path to `dispatch.js` differs from the one in `inbound-testkit.ts`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/channels/plugins/contracts/inbound.contract.test.ts
Line: 192-193

Comment:
**`inboundCtxCapture.ctx` is never written by the active mock**

The `vi.mock()` block defined in this file (lines 31–39) replaces `dispatchInboundMessage` with `dispatchInboundMessageMock`, which captures the context into `signalCapture.ctx` — not `inboundCtxCapture.ctx`. The old `inbound-contract-dispatch-mock.ts` had its own module-level `vi.mock()` that used `buildDispatchInboundContextCapture` to write into `inboundCtxCapture.ctx`. Since `inbound-testkit.ts` no longer carries that side-effect mock, no code path sets `inboundCtxCapture.ctx`, so this assertion will fail:

```
expect(inboundCtxCapture.ctx).toBeTruthy(); // → ctx remains undefined → FAILS
```

The "keeps Signal inbound context finalized" test (lines 196–217) correctly checks `signalCapture.ctx` (what the local mock actually writes). The Discord test case here should either:

1. Switch to checking `signalCapture.ctx` (which the active mock already writes), or
2. Update `dispatchInboundMessageMock` to also write `inboundCtxCapture.ctx = params.ctx`.

Using `signalCapture.ctx` is the simpler fix since that object is already populated:

```suggestion
    expect(signalCapture.ctx).toBeTruthy();
    expectChannelInboundContextContract(signalCapture.ctx!);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: e5110a8}

greptile-apps · 2026-03-17T05:51:57Z

extensions/discord/src/monitor/message-handler.inbound-context.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { inboundCtxCapture as capture } from "../../../../src/channels/plugins/contracts/inbound-contract-dispatch-mock.js";
+import { inboundCtxCapture as capture } from "../../../../src/channels/plugins/contracts/inbound-testkit.js";


Missing vi.mock() — capture.ctx will always be undefined

The old inbound-contract-dispatch-mock.ts module contained a module-level vi.mock("../../../auto-reply/dispatch.js", ...) call that was executed as a side effect of importing it. This mock intercepted dispatchInboundMessage and set inboundCtxCapture.ctx on each call.

The new inbound-testkit.ts consolidates all the helper utilities but deliberately omits this vi.mock() call. As a result, dispatch.js is no longer mocked in this test file — there is no vi.mock() here at all. When processDiscordMessage(messageCtx) runs, the real (unmocked) dispatchInboundMessage executes and nothing writes to capture.ctx. Both test assertions will then fail:

expect(capture.ctx).toBeTruthy(); // → capture.ctx is undefined → FAILS expectInboundContextContract(capture.ctx!); // → called with undefined

The Signal extension test (event-handler.mention-gating.test.ts) correctly handles this by calling vi.mock() directly in the test file and using buildDispatchInboundCaptureMock. The same pattern needs to be applied here:

vi.mock("../../../../src/auto-reply/dispatch.js", async (importOriginal) => { const actual = await importOriginal<typeof import("../../../../src/auto-reply/dispatch.js")>(); return buildDispatchInboundCaptureMock(actual, (ctx) => { capture.ctx = ctx as MsgContext; }); });

Note the path depth — this test lives under extensions/discord/src/monitor/, four levels from the repo root, so the relative path to dispatch.js differs from the one in inbound-testkit.ts.

Prompt To Fix With AI

This is a comment left during a code review. Path: extensions/discord/src/monitor/message-handler.inbound-context.test.ts Line: 2 Comment: **Missing `vi.mock()` — `capture.ctx` will always be `undefined`** The old `inbound-contract-dispatch-mock.ts` module contained a module-level `vi.mock("../../../auto-reply/dispatch.js", ...)` call that was executed as a side effect of importing it. This mock intercepted `dispatchInboundMessage` and set `inboundCtxCapture.ctx` on each call. The new `inbound-testkit.ts` consolidates all the helper utilities but deliberately omits this `vi.mock()` call. As a result, `dispatch.js` is no longer mocked in this test file — there is no `vi.mock()` here at all. When `processDiscordMessage(messageCtx)` runs, the real (unmocked) `dispatchInboundMessage` executes and nothing writes to `capture.ctx`. Both test assertions will then fail: ``` expect(capture.ctx).toBeTruthy(); // → capture.ctx is undefined → FAILS expectInboundContextContract(capture.ctx!); // → called with undefined ``` The Signal extension test (`event-handler.mention-gating.test.ts`) correctly handles this by calling `vi.mock()` directly in the test file and using `buildDispatchInboundCaptureMock`. The same pattern needs to be applied here: ```typescript vi.mock("../../../../src/auto-reply/dispatch.js", async (importOriginal) => { const actual = await importOriginal<typeof import("../../../../src/auto-reply/dispatch.js")>(); return buildDispatchInboundCaptureMock(actual, (ctx) => { capture.ctx = ctx as MsgContext; }); }); ``` Note the path depth — this test lives under `extensions/discord/src/monitor/`, four levels from the repo root, so the relative path to `dispatch.js` differs from the one in `inbound-testkit.ts`. How can I resolve this? If you propose a fix, please make it concise.

* Plugins: share contract test helpers * Channels: collapse inbound contract testkit

* Plugins: share contract test helpers * Channels: collapse inbound contract testkit (cherry picked from commit 64c69c3) # Conflicts: # extensions/discord/src/monitor/message-handler.inbound-context.test.ts # extensions/signal/src/monitor/event-handler.mention-gating.test.ts # src/channels/plugins/contracts/inbound.contract.test.ts # src/plugins/contracts/auth-choice.contract.test.ts # src/plugins/contracts/auth.contract.test.ts # src/plugins/contracts/discovery.contract.test.ts # src/plugins/contracts/loader.contract.test.ts # src/plugins/contracts/registry.ts # test/helpers/dispatch-inbound-capture.ts # test/helpers/inbound-contract-capture.ts # test/helpers/inbound-contract-dispatch-mock.ts

* Plugins: share contract test helpers * Channels: collapse inbound contract testkit (cherry picked from commit 64c69c3) # Conflicts: # extensions/discord/src/monitor/message-handler.inbound-context.test.ts # src/plugins/contracts/auth.contract.test.ts # src/plugins/contracts/discovery.contract.test.ts # src/plugins/contracts/registry.ts # test/helpers/dispatch-inbound-capture.ts # test/helpers/inbound-contract-capture.ts # test/helpers/inbound-contract-dispatch-mock.ts

vincentkoc added 2 commits March 16, 2026 22:44

Plugins: share contract test helpers

6ba651f

Channels: collapse inbound contract testkit

e5110a8

openclaw-barnacle bot added channel: discord Channel integration: discord channel: signal Channel integration: signal labels Mar 17, 2026

vincentkoc marked this pull request as ready for review March 17, 2026 05:45

vincentkoc self-assigned this Mar 17, 2026

openclaw-barnacle bot added size: M maintainer Maintainer-authored PR labels Mar 17, 2026

vincentkoc merged commit 64c69c3 into openclaw:main Mar 17, 2026
12 of 13 checks passed

vincentkoc mentioned this pull request Mar 17, 2026

Tests: share provider registration helpers #48767

Merged

20 tasks

chatgpt-codex-connector bot reviewed Mar 17, 2026

View reviewed changes

greptile-apps bot reviewed Mar 17, 2026

View reviewed changes

github-actions bot mentioned this pull request Mar 17, 2026

📡 Upstream Digest — 2026-03-17 06:55 UTC curtismercier/openclaw-mods#286

Open

nikolaisid pushed a commit to nikolaisid/openclaw that referenced this pull request Mar 18, 2026

Tests: dedupe contract helper plumbing (openclaw#48760)

e0307f2

* Plugins: share contract test helpers * Channels: collapse inbound contract testkit

alexey-pelykh mentioned this pull request Mar 23, 2026

Cherry-pick (blocked): Discord adapter (1/2) (50 commits) remoteclaw/remoteclaw#1922

Open

sbezludny pushed a commit to sbezludny/openclaw that referenced this pull request Mar 27, 2026

Tests: dedupe contract helper plumbing (openclaw#48760)

214a4e3

* Plugins: share contract test helpers * Channels: collapse inbound contract testkit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Tests: dedupe contract helper plumbing#48760

Tests: dedupe contract helper plumbing#48760
vincentkoc merged 2 commits intoopenclaw:mainfrom
vincentkoc:vincentkoc-code/slack-plugin-interactive-dedupe

vincentkoc commented Mar 17, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 17, 2026

Uh oh!

greptile-apps bot commented Mar 17, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vincentkoc commented Mar 17, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 2/5

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps bot commented Mar 17, 2026 •

edited

Loading