Files
t0saki 0ec8d25996 fix(plugins): resolve one connection for every plugin's hooks and MCP proxy (#5132)
* fix(codex): read the MCP proxy's connection from the hooks' loadConfig

The proxy resolved url, api_key, account and user through the bare
credential chain while taking everything else from loadConfig(). Since
the loader became buildPluginConfig() it adds layers that chain never
sees: ovcli.conf's plugin.codex apiKey/accountId/userId, and ov.conf's
codex.apiKey when ovcli.conf names only the server. With either, hooks
authenticated and every MCP tool call went out without a key.

Codex also hands a stdio MCP server only the env vars .mcp.json lists,
and OPENVIKING_AUTH_MODE was not one of them, so an env-set auth mode
decided the identity headers for hooks but not for MCP calls.

* fix(dsh): forward the resolved auth mode and timeout to the MCP proxy

The proxy runs as a child whose env DSH scrubs, so the parent forwards
what it resolved. It forwarded the endpoint, key, account, user and
peer but not the auth mode or request timeout, so a Cordis patch that
set either configured the in-process runtime and not the MCP calls.

The proxy now also takes its credential source and watched paths from
the resolved config instead of a second credential-chain call, and a
shared test keeps every proxy that ships beside hooks off that chain.

* refactor(shared): one connection resolver for hooks and the MCP proxy

The credential chain lived in two layers. `resolveOpenVikingCredentials()`
could not read ovcli.conf's `plugin.<harness>` keys, the ov.conf harness
fallback or the root-key tail; `buildPluginConfig()` patched those in, and
any caller that used the lower layer alone resolved a different key and
identity than the hooks did.

`resolveConnection(harness, { env, files, hostInput, rootKeyFallback })`
now answers server, key, identity and auth mode in one place, reading only
host input, the environment and the two ~/.openviking files. The hook
loader and `buildProxyConnection()` both consume it, and the old
two-layer entry points (`resolveOpenVikingCredentials`, `resolveAuthMode`,
the credentials.mjs CLI) are gone so a half-resolved chain cannot be
written again.

Behaviour is unchanged for every hook harness (checked field by field
against the previous implementation over thousands of generated file/env
combinations). Two deliberate additions: the portable agent-plugins proxy
now honours ovcli.conf's `plugin` connection keys and ov.conf's harness
section like every other harness, and dsh hands the host's `authMode`
(or `auth_mode`) over as host input, ranking it with the host's endpoint,
key and identity.

* fix(shared): a forced env credential source reads only the environment

`OPENVIKING_CREDENTIAL_SOURCE=env` is documented as "env vars only", but
only the url honoured it: the key, account, user, ovcli.conf's actor peer
and the auth mode still fell through to ovcli.conf, its plugin keys,
ov.conf and the root key when the variable was unset. A process that
exported an empty key to mean "no key" was silently handed whatever the
files held.

Forced to `env`, the connection now reads no file and an unset variable
stays empty; the url defaults to http://127.0.0.1:1933. The `peerId`
setting keeps its own layers. The doctor labels that mode instead of
pointing at files the chain skipped.

* refactor(shared): one proxy-config mapper and one forwarded-env list

Each proxy entrypoint copied a dozen fields out of its loader by hand,
under two sets of names, and the copies had drifted. What the proxy
process must be handed was a second hand-kept list, in Codex's
`.mcp.json` and in its test.

`toMcpProxyConfig(cfg, options)` maps a resolved loader or proxy
connection to the proxy config once. `MCP_PROXY_ENV_VARS` names every
variable that changes what a proxy sends; Codex's `env_vars` is now
checked against it, which adds the missing `OPENVIKING_STATE_DIR`.

* refactor(plugins): every loader takes an env, every proxy exports readProxyConfig(env)

The six MCP proxy entrypoints now reduce to one line: resolve through the
harness's own loader (or `buildProxyConnection` for the hook-less package)
and hand the result to `toMcpProxyConfig`. Every one exports
`readProxyConfig(env)`, and the codex, claude-code, opencode and agent-hook
loaders accept an injected env, so a test can drive a hook and its proxy
from the same inputs without touching process.env.

Mapping through one function fixes what the hand copies had lost: the
Claude Code and DSH proxies never passed `mcpUrl`, so
`OPENVIKING_MCP_URL` moved the hooks and left the tools behind.

The source guard now requires the shared mapper and the exported reader
in every proxy, and `buildProxyConnection` reports its two config paths
instead of a watch list of its own.

* fix(dsh): forward the resolved connection to the MCP proxy

DSH starts its MCP subprocess with the parent's environment minus
credential-shaped names (`/KEY|PASSWORD|SECRET|TOKEN/i`), so the bundle
forwards what it resolved. It forwarded the values but not the mode, and
only the non-empty ones:

- A child that receives `OPENVIKING_URL` runs the chain unpinned. Where
  the parent's chain was pinned to an ovcli.conf that names only a url,
  the parent sent no key while the proxy fell through to ov.conf's
  `server.root_api_key`, so the tools reached the server as root while
  the hooks were anonymous.
- `OPENVIKING_ACCOUNT`, `OPENVIKING_USER` and `OPENVIKING_PEER_ID` survive
  DSH's scrub, so a value the parent's chain ignored filled the gap in
  the child and went out as an identity header.
- With no peer to forward, the proxy derived one from its own launch
  directory and sent an actor peer the runtime did not.

`forwardConnectionEnv(connection)` now writes every credential variable,
the empty ones too, with the forced `env` source, so the child reads no
file and resolves exactly the parent's url, MCP url, key, identity, auth
mode and peer. The proxy takes its peer from that environment only.
`buildMcpConfig` moves to `mcp-env.mjs`, which carries no host
dependency, so shared tests can build the child environment without the
DSH bridge.

* test(shared): prove the proxy and the hooks resolve one connection

The existing guards checked shape — that a proxy called the shared
builder — never that it reached the server as the same caller its hooks
did, which is how two harnesses shipped proxies that disagreed with them.

`mcp-hook-parity.test.mjs` runs every harness that ships a proxy beside
hooks through a dozen configurations: ovcli.conf's own fields, its
`plugin.<harness>` and shared plugin keys, ov.conf-only installs, the
pinned fallbacks to a harness key and to the root key, credential and
auth-mode variables, a forced source over stale variables, an explicit
MCP URL, a host's own input, and a workspace file that tries to move the
connection. The hook loader sees the full environment; the proxy sees
only what its host lets through — Codex's `env_vars`, DSH's scrubbed
inheritance plus the forwarded connection, everyone else's full
environment — and the url, key, identity and identity-header switch they
put on the wire must match. Scenarios with a known answer pin it too, and
a coverage check fails when a new proxy or hook client has no row.

The two codex-only proxy tests the matrix now covers are removed.

* docs(plugins): one connection for hooks and MCP, and version bumps

The capability reference, plugin development guide, Agent Plugins and
Codex pages (en/zh), both doctor references and the plugin READMEs now
describe the chain `resolveConnection()` runs: host input first, the
pinned ovcli.conf branch and what still falls through it, the auth mode
reading `OPENVIKING_AUTH_MODE` and the `plugin` keys in every mode, a
forced `env` source reading no file, and the two ways a connection crosses
into an MCP process (Codex's forwarded-variable list, dsh's forwarded
connection). The parity test is registered with the credential tests.

Versions move past both this branch's base and main: claude-code 0.5.2,
codex 0.9.2, agent-hook 0.3.2, opencode 0.3.2, dsh 0.4.3, pi 0.3.2,
agent-plugins 0.1.2.
2026-09-17 19:38:18 +08:00

197 lines
6.9 KiB
JavaScript

/**
* Helpers shared by the plugin test files.
*
* They live outside `lib/` on purpose: `sync.mjs` copies that directory into
* every plugin, and nothing here belongs in a shipped plugin. Importing a
* helper from a `*.test.mjs` file would also register that file's own tests a
* second time in whichever runner picked it up.
*/
import assert from "node:assert/strict";
import { spawn } from "node:child_process";
import { mkdtemp, rm, writeFile } from "node:fs/promises";
import http from "node:http";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { buildPluginConfig } from "../lib/plugin-config.mjs";
/**
* A built config resolved against nothing at all, for callers that only need
* the shape of the object a loader receives.
*/
export function buildConfigForTest(harness) {
const dir = join(tmpdir(), "ov-plugin-config-absent");
return buildPluginConfig(harness, {
cwd: dir,
env: {
OPENVIKING_CLI_CONFIG_FILE: join(dir, "ovcli.conf"),
OPENVIKING_CONFIG_FILE: join(dir, "ov.conf"),
OPENVIKING_HOME: dir,
},
});
}
/**
* An ovcli.conf / ov.conf pair in a fresh directory, and the env that points
* the credential chain at it. A file given as `null` is not written, so the
* chain sees it as absent rather than as a real ~/.openviking.
*/
export async function writeCredentialFiles(prefix, { ovcli = null, ov = null } = {}) {
const dir = await mkdtemp(join(tmpdir(), prefix));
const cliPath = join(dir, "ovcli.conf");
const ovPath = join(dir, "ov.conf");
if (ovcli) await writeFile(cliPath, JSON.stringify(ovcli, null, 2) + "\n");
if (ov) await writeFile(ovPath, JSON.stringify(ov, null, 2) + "\n");
return {
dir,
cliPath,
ovPath,
env: { OPENVIKING_CLI_CONFIG_FILE: cliPath, OPENVIKING_CONFIG_FILE: ovPath, OPENVIKING_HOME: join(dir, "home") },
};
}
const isOpenVikingVar = (name) => name.startsWith("OPENVIKING_") || name === "OV_DEBUG_LOG";
/**
* Clear the developer shell's `OPENVIKING_*` out of process.env, and return
* the function that puts them back. A test that injects its env everywhere
* still runs under this, so a read that slips past the injection fails the
* same way on every machine instead of passing on the one with a config.
*/
export function scrubOpenVikingEnv() {
const saved = Object.entries(process.env).filter(([name]) => isOpenVikingVar(name));
for (const [name] of saved) delete process.env[name];
return () => {
for (const name of Object.keys(process.env)) {
if (isOpenVikingVar(name)) delete process.env[name];
}
for (const [name, value] of saved) process.env[name] = value;
};
}
const PENDING_ENV_KEYS = [
"OPENVIKING_PENDING_DIR",
"OPENVIKING_PENDING_MAX_RETRIES",
"OPENVIKING_PENDING_REPLAY_LIMIT",
"OPENVIKING_PENDING_TTL_DAYS",
];
/**
* Run `fn` against an empty pending queue in a throwaway directory.
*
* The queue reads its directory and its limits from the environment, so the
* knobs are cleared for the duration and every one of them is restored after,
* whether or not the test set it.
*/
export async function withPendingDir(fn) {
const saved = PENDING_ENV_KEYS.map((key) => [key, process.env[key]]);
const dir = await mkdtemp(join(tmpdir(), "openviking-pending-test-"));
for (const key of PENDING_ENV_KEYS) delete process.env[key];
process.env.OPENVIKING_PENDING_DIR = dir;
try {
return await fn(dir);
} finally {
for (const [key, value] of saved) {
if (value === undefined) delete process.env[key];
else process.env[key] = value;
}
await rm(dir, { recursive: true, force: true });
}
}
/** The JSON body of a request, or `null` when the request carried no body. */
export function readRequestBody(req) {
return new Promise((resolve, reject) => {
const chunks = [];
req.on("data", (chunk) => chunks.push(chunk));
req.on("end", () => {
const raw = Buffer.concat(chunks).toString("utf-8");
try {
resolve(raw ? JSON.parse(raw) : null);
} catch (error) {
reject(error);
}
});
req.on("error", reject);
});
}
/** Answer with a JSON body, 200 unless the caller names another status. */
export function writeJson(res, value, statusCode = 200) {
res.writeHead(statusCode, { "Content-Type": "application/json" });
res.end(JSON.stringify(value));
}
/**
* Run `fn` against a mock OpenViking listening on loopback.
*
* `handler` may be sync or async; whatever it throws becomes a 500 rather than
* an unhandled rejection that outlives the test. `fn` receives the base URL and
* a live log of every request the mock saw, so a test that only cares about
* which endpoints were hit does not have to record them inside its handler.
*/
export async function withMockOpenViking(handler, fn) {
const requests = [];
const server = http.createServer((req, res) => {
requests.push({
method: req.method,
path: new URL(req.url, "http://127.0.0.1").pathname,
url: req.url,
headers: req.headers,
});
Promise.resolve(handler(req, res)).catch((error) => {
writeJson(res, { status: "error", error: String(error?.stack || error) }, 500);
});
});
await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve));
try {
const { port } = server.address();
return await fn(`http://127.0.0.1:${port}`, requests);
} finally {
await new Promise((resolve) => server.close(resolve));
}
}
/**
* Run a hook script as its host runs it, and never reject.
*
* A hook that exits non-zero is a result a test may well be asserting on, so
* the exit code comes back like any other output; call `expectExit` where a
* clean exit is part of the expectation.
*/
export function runHookScript(scriptPath, { argv = [], input, env, cwd } = {}) {
return new Promise((resolve) => {
const child = spawn(process.execPath, [scriptPath, ...argv], {
cwd,
env: { ...process.env, ...env },
stdio: ["pipe", "pipe", "pipe"],
});
let stdout = "";
let stderr = "";
let spawnError = null;
let settled = false;
const settle = (code, signal) => {
if (settled) return;
settled = true;
resolve({ code, signal, stdout, stderr, error: spawnError });
};
child.stdout.on("data", (chunk) => { stdout += chunk; });
child.stderr.on("data", (chunk) => { stderr += chunk; });
child.on("error", (error) => { spawnError = error; settle(null, null); });
child.on("close", settle);
// A hook that answers without draining stdin makes this write fail EPIPE.
child.stdin.on("error", () => {});
child.stdin.end(
input === undefined || typeof input === "string" ? (input ?? "") : JSON.stringify(input),
);
});
}
/** Assert a `runHookScript` result exited with `code`, and return the result. */
export function expectExit(result, code = 0) {
const detail = result.stderr.trim() || String(result.error || "");
assert.equal(result.code, code, detail || `expected exit ${code}, got ${result.code}`);
return result;
}