test(extensibility): drop dynamic import from #6449 regression test
Extract the first-wins global registration into an exported ensureGraphCommonJsRequireRegistered() seam and assert its idempotent (first-wins) contract directly, instead of copying the module and importing the copy at runtime. Removes the inline await import() banned under packages/coding-agent and keeps regression coverage: the test fails if the registration reverts to an unconditional set. Fixes #6449
This commit is contained in:
roboomp
@@ -1504,16 +1504,26 @@ function evaluateGraphCommonJs(modulePath: string): unknown {
|
||||
}
|
||||
}
|
||||
|
||||
// First-wins registration. On source-link installs the pi-coding-agent root
|
||||
// shim is served from src/, so an extension import can evaluate a second
|
||||
// instance of this module with empty graph state. An unconditional set would
|
||||
// let that empty instance clobber the host bundle's populated bridge and break
|
||||
// transitive CommonJS resolution (#6449); guarding preserves the first
|
||||
// (host-owned) registration.
|
||||
if (!Reflect.get(globalThis, COMMONJS_REQUIRE_GLOBAL)) {
|
||||
Reflect.set(globalThis, COMMONJS_REQUIRE_GLOBAL, evaluateGraphCommonJs);
|
||||
/**
|
||||
* Register {@link evaluateGraphCommonJs} as the graph-owned CommonJS require
|
||||
* bridge on `globalThis`, first-wins.
|
||||
*
|
||||
* On source-link installs the `@(scope)/pi-coding-agent` root shim is served
|
||||
* from `src/`, so an extension import can evaluate a second instance of this
|
||||
* module with empty graph state. An unconditional set would let that empty
|
||||
* instance clobber the host bundle's populated bridge and break transitive
|
||||
* CommonJS resolution (#6449); guarding preserves the first (host-owned)
|
||||
* registration. Idempotent: a subsequent call with a value already present is a
|
||||
* no-op.
|
||||
*/
|
||||
export function ensureGraphCommonJsRequireRegistered(): void {
|
||||
if (!Reflect.get(globalThis, COMMONJS_REQUIRE_GLOBAL)) {
|
||||
Reflect.set(globalThis, COMMONJS_REQUIRE_GLOBAL, evaluateGraphCommonJs);
|
||||
}
|
||||
}
|
||||
|
||||
ensureGraphCommonJsRequireRegistered();
|
||||
|
||||
let legacyPiLoadTag = 0;
|
||||
|
||||
function nextLegacyPiLoadTag(): string {
|
||||
|
||||
@@ -1,61 +1,44 @@
|
||||
import { afterEach, describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import * as path from "node:path";
|
||||
import {
|
||||
installLegacyPiSpecifierShim,
|
||||
loadLegacyPiModule,
|
||||
} from "@oh-my-pi/pi-coding-agent/extensibility/plugins/legacy-pi-compat";
|
||||
import { TempDir } from "@oh-my-pi/pi-utils";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "bun:test";
|
||||
import { ensureGraphCommonJsRequireRegistered } from "@oh-my-pi/pi-coding-agent/extensibility/plugins/legacy-pi-compat";
|
||||
|
||||
const COMPAT_MODULE = Bun.resolveSync(
|
||||
"@oh-my-pi/pi-coding-agent/extensibility/plugins/legacy-pi-compat",
|
||||
import.meta.dir,
|
||||
);
|
||||
// The global key the host bundle and every re-instantiated shim copy share to
|
||||
// hand graph-owned CommonJS modules to whichever instance owns the populated
|
||||
// graph state.
|
||||
const COMMONJS_REQUIRE_GLOBAL = "__ompLegacyPiRequireGraphModule";
|
||||
|
||||
describe("issue #6449: legacy pi CommonJS graph across double instantiation", () => {
|
||||
let dupPath: string | undefined;
|
||||
let tempDir: TempDir | undefined;
|
||||
describe("issue #6449: legacy pi CommonJS graph registration is first-wins", () => {
|
||||
let original: unknown;
|
||||
|
||||
beforeEach(() => {
|
||||
original = Reflect.get(globalThis, COMMONJS_REQUIRE_GLOBAL);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (dupPath) {
|
||||
fs.rmSync(dupPath, { force: true });
|
||||
dupPath = undefined;
|
||||
}
|
||||
if (tempDir) {
|
||||
tempDir.removeSync();
|
||||
tempDir = undefined;
|
||||
}
|
||||
Reflect.set(globalThis, COMMONJS_REQUIRE_GLOBAL, original);
|
||||
});
|
||||
|
||||
it("keeps the host graph bridge after a second legacy-pi-compat instance loads", async () => {
|
||||
installLegacyPiSpecifierShim();
|
||||
// On source-link installs the pi-coding-agent root shim is served from src/,
|
||||
// so an extension import evaluates a SECOND instance of legacy-pi-compat with
|
||||
// empty graph state. Its module-level registration must not clobber the host
|
||||
// bundle's populated bridge — a regression to an unconditional `Reflect.set`
|
||||
// broke transitive CommonJS resolution ("Missing graph-owned CommonJS
|
||||
// definition"). `ensureGraphCommonJsRequireRegistered` is exactly the
|
||||
// module-level registration a second instance runs.
|
||||
it("does not overwrite an existing bridge registration", () => {
|
||||
const hostBridge = (path: string): unknown => `host:${path}`;
|
||||
Reflect.set(globalThis, COMMONJS_REQUIRE_GLOBAL, hostBridge);
|
||||
|
||||
tempDir = TempDir.createSync("@issue-6449-");
|
||||
const extDir = tempDir.absolute();
|
||||
fs.writeFileSync(path.join(extDir, "dep.cjs"), 'module.exports = { greet: () => "hello" };\n');
|
||||
const entryPath = path.join(extDir, "index.ts");
|
||||
// Import the CommonJS dependency lazily so the graph scan records it into
|
||||
// the CommonJS graph at load time, but its bridge only evaluates when
|
||||
// useDep() runs — after the second instance has re-registered the global.
|
||||
fs.writeFileSync(
|
||||
entryPath,
|
||||
`export async function useDep(): Promise<string> {\n\tconst mod = await import("./dep.cjs");\n\treturn (mod as { greet(): string }).greet();\n}\n`,
|
||||
);
|
||||
// Re-running the registration (as a second instance would) is a no-op.
|
||||
ensureGraphCommonJsRequireRegistered();
|
||||
|
||||
const ns = (await loadLegacyPiModule(entryPath)) as { useDep(): Promise<string> };
|
||||
expect(Reflect.get(globalThis, COMMONJS_REQUIRE_GLOBAL)).toBe(hostBridge);
|
||||
});
|
||||
|
||||
// A source-link install serves the pi-coding-agent root shim from src/, so
|
||||
// an extension's import evaluates a SECOND on-disk instance of this module
|
||||
// with empty graph state. Reproduce that distinct module identity by
|
||||
// copying the module beside the original (identical relative imports
|
||||
// resolve to the same shared deps) and importing the copy, which re-runs
|
||||
// its top-level global registration. A regression to an unconditional
|
||||
// `Reflect.set` clobbers the host bridge and makes useDep() throw
|
||||
// "Missing graph-owned CommonJS definition".
|
||||
dupPath = path.join(path.dirname(COMPAT_MODULE), "issue-6449-compat-dup.ts");
|
||||
fs.copyFileSync(COMPAT_MODULE, dupPath);
|
||||
await import(Bun.pathToFileURL(dupPath).href);
|
||||
it("registers a bridge when none is present yet", () => {
|
||||
Reflect.deleteProperty(globalThis, COMMONJS_REQUIRE_GLOBAL);
|
||||
|
||||
expect(await ns.useDep()).toBe("hello");
|
||||
ensureGraphCommonJsRequireRegistered();
|
||||
|
||||
expect(typeof Reflect.get(globalThis, COMMONJS_REQUIRE_GLOBAL)).toBe("function");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user