fix(context-engine): address review findings F1–F3/S1

- Add CHANGELOG entry for #66887 (F1)
  - Guard Symbol.for() registry access with expect(registryState).toBeDefined() (F2)
  - Test resolveDefaultContextEngine factory-throws and contract-violation paths (F3)
  - Add inline comment clarifying sanitizeForLog layering on contractError (S1)
This commit is contained in:
openperf
2026-04-15 11:20:20 +08:00
parent f86def0672
commit 1ca42d052d
3 changed files with 69 additions and 10 deletions

View File

@@ -46,6 +46,7 @@ Docs: https://docs.openclaw.ai
- QQBot/cron: guard against undefined `event.content` in `parseFaceTags` and `filterInternalMarkers` so cron-triggered agent turns with no content payload no longer crash with `TypeError: Cannot read properties of undefined (reading 'startsWith')`. (#66302) Thanks @xinmotlanthua.
- CLI/plugins: stop `--dangerously-force-unsafe-install` plugin installs from falling back to hook-pack installs after security scan failures, while still preserving non-security fallback behavior for real hook packs. (#58909) Thanks @hxy91819.
- Claude CLI/sessions: classify `No conversation found with session ID` as `session_expired` so expired CLI-backed conversations clear the stale binding and recover on the next turn. (#65028) thanks @Ivan-Fn.
- Context Engine: gracefully fall back to the legacy engine when a third-party context engine plugin fails at resolution time (unregistered id, factory throw, or contract violation), preventing a full gateway outage on every channel. (#66887) Thanks @openperf.
## 2026.4.14

View File

@@ -725,23 +725,53 @@ describe("Invalid engine fallback", () => {
it("throws when the default engine itself is not registered", async () => {
// Access the process-global registry via the well-known symbol and clear it
// so even the default engine is missing.
// so even the default engine is missing. The symbol key must match the
// private CONTEXT_ENGINE_REGISTRY_STATE constant in registry.ts — guard
// against a silent key mismatch so a rename surfaces loudly.
const registryState = (globalThis as Record<symbol, unknown>)[
Symbol.for("openclaw.contextEngineRegistryState")
] as { engines: Map<string, unknown> };
const snapshot = new Map(registryState.engines);
registryState.engines.clear();
] as { engines: Map<string, unknown> } | undefined;
expect(registryState).toBeDefined();
const snapshot = new Map(registryState!.engines);
registryState!.engines.clear();
try {
await expect(resolveContextEngine()).rejects.toThrow("not registered");
} finally {
// Restore so other tests are not affected.
for (const [key, value] of snapshot) {
registryState.engines.set(key, value);
registryState!.engines.set(key, value);
}
}
});
it("propagates error when default engine factory throws", async () => {
// Override the default "legacy" engine with a throwing factory via the
// core-owner path so the registration is accepted.
registerContextEngineForOwner(
"legacy",
() => {
throw new Error("default engine init failed");
},
"core",
{ allowSameOwnerRefresh: true },
);
await expect(resolveContextEngine()).rejects.toThrow("default engine init failed");
});
it("propagates error when default engine fails contract validation", async () => {
registerContextEngineForOwner(
"legacy",
() => ({ broken: true }) as unknown as ContextEngine,
"core",
{ allowSameOwnerRefresh: true },
);
await expect(resolveContextEngine()).rejects.toThrow(
'Context engine "legacy" factory returned an invalid ContextEngine',
);
});
it("falls back to default engine when factory throws", async () => {
const engineId = `factory-throw-${Date.now().toString(36)}`;
registerContextEngine(engineId, () => {
@@ -824,6 +854,20 @@ describe("Invalid engine fallback", () => {
expect.stringContaining("missing assemble(), missing compact()"),
);
});
it("falls back to default engine when contract validation itself throws", async () => {
const engineId = `validation-throw-${Date.now().toString(36)}`;
// BigInt cannot be JSON.stringify'd — triggers a throw inside
// describeResolvedContextEngineContractError when the factory returns
// a non-object value that passes the typeof !== "object" branch.
registerContextEngine(engineId, () => 42n as unknown as ContextEngine);
const engine = await resolveContextEngine(configWithSlot(engineId));
expect(engine.info.id).toBe("legacy");
expect(console.error).toHaveBeenCalledWith(
expect.stringContaining("contract validation threw"),
);
});
});
// ═══════════════════════════════════════════════════════════════════════════

View File

@@ -494,13 +494,27 @@ export async function resolveContextEngine(config?: OpenClawConfig): Promise<Con
return resolveDefaultContextEngine(defaultEngineId);
}
const contractError = describeResolvedContextEngineContractError(engineId, engine);
let contractError: string | null;
try {
contractError = describeResolvedContextEngineContractError(engineId, engine);
} catch (validationError) {
if (isDefaultEngine) {
throw validationError;
}
console.error(
`[context-engine] Context engine "${sanitizeForLog(engineId)}" contract validation threw: ` +
`${sanitizeForLog(validationError instanceof Error ? validationError.message : String(validationError))}; ` +
`falling back to default engine "${defaultEngineId}".`,
);
return resolveDefaultContextEngine(defaultEngineId);
}
if (contractError) {
if (isDefaultEngine) {
throw new Error(contractError);
}
// contractError includes engineId from plugin config; sanitizeForLog covers it
console.error(
`[context-engine] ${sanitizeForLog(contractError)} Falling back to default engine "${defaultEngineId}".`,
`[context-engine] ${sanitizeForLog(contractError)}; falling back to default engine "${defaultEngineId}".`,
);
return resolveDefaultContextEngine(defaultEngineId);
}
@@ -518,14 +532,14 @@ async function resolveDefaultContextEngine(defaultEngineId: string): Promise<Con
const defaultEntry = getContextEngineRegistryState().engines.get(defaultEngineId);
if (!defaultEntry) {
throw new Error(
`Context engine fallback failed: default engine "${defaultEngineId}" is not registered. ` +
`[context-engine] fallback failed: default engine "${defaultEngineId}" is not registered. ` +
`Available engines: ${listContextEngineIds().join(", ") || "(none)"}`,
);
}
const engine = await defaultEntry.factory();
const contractError = describeResolvedContextEngineContractError(defaultEngineId, engine);
if (contractError) {
throw new Error(contractError);
throw new Error(`[context-engine] ${contractError}`);
}
return wrapContextEngineWithSessionKeyCompat(engine);
}