mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-07-26 09:52:11 +03:00
refactor(guardrails): narrow the documented void return at the dispatch site (#8525)
`BaseGuardrail.preCall`/`postCall` return `GuardrailResult<unknown> | void`,
and that `| void` is deliberate: docs/security/GUARDRAILS.md documents returning
nothing as one of the three "no change" signals, and CredentialMaskerGuardrail
still declares it. So the union stays.
The problem is the consumption site. `registry.ts` reads `result?.block`,
`result?.message`, `result?.meta`, `result?.modifiedPayload` — but optional
chaining does not make `void` inspectable, and neither does a truthiness test.
That is all 12 TS2339s in the file: one union, six property reads, two dispatch
loops.
Fixed by funnelling the return through `unknown` once, in a local
`asGuardrailResult()` helper, so the loops work against a plain
`GuardrailResult | undefined`. The public contract is untouched — no signature
narrowed, no guardrail changed, callers outside this file unaffected.
280 -> 268, zero new, on a line-number-agnostic diff of the full tsc error set.
Tests: the `void` arm of the documented contract had NO coverage —
guardrails-registry.test.ts exercises guardrails that return a result and one
that throws, never one that returns nothing. Added
guardrails-void-no-change-contract.test.ts (4 tests): silent preCall/postCall
pass through untouched and are recorded as ran-not-skipped-not-errored;
returning `{}` produces an identical execution record; a silent guardrail does
not stop a later one from modifying. They pass against the parent commit's
registry.ts too, which is the evidence for no behavior change.
75/75 across the 13 guardrail suites.
Co-authored-by: backryun <busan011@ormbiz.co.kr>
This commit is contained in:
@@ -1,9 +1,30 @@
|
||||
import { BaseGuardrail, type GuardrailContext, type GuardrailExecutionResult } from "./base";
|
||||
import {
|
||||
BaseGuardrail,
|
||||
type GuardrailContext,
|
||||
type GuardrailExecutionResult,
|
||||
type GuardrailResult,
|
||||
} from "./base";
|
||||
import { PIIMaskerGuardrail } from "./piiMasker";
|
||||
import { PromptInjectionGuardrail } from "./promptInjection";
|
||||
import { VisionBridgeGuardrail } from "./visionBridge";
|
||||
import { CredentialMaskerGuardrail } from "./credentialMasker";
|
||||
|
||||
/**
|
||||
* `preCall`/`postCall` may legitimately return nothing — that is the documented
|
||||
* "no change" signal, alongside `{}` and `{ block: false }`
|
||||
* (`docs/security/GUARDRAILS.md`), and `CredentialMaskerGuardrail` still declares
|
||||
* the `| void` arm.
|
||||
*
|
||||
* `void` is not a value the checker lets us inspect, so neither `result?.block`
|
||||
* nor a truthiness test compiles against `GuardrailResult | void`. Funnel the
|
||||
* return through `unknown` once, here, and hand the dispatch loops a plain
|
||||
* optional. Runtime behavior is unchanged: a guardrail that returns nothing
|
||||
* still yields `undefined` and is still treated as "passed".
|
||||
*/
|
||||
function asGuardrailResult(raw: unknown): GuardrailResult<unknown> | undefined {
|
||||
return raw && typeof raw === "object" ? (raw as GuardrailResult<unknown>) : undefined;
|
||||
}
|
||||
|
||||
type HeadersLike = Headers | Record<string, unknown> | null | undefined;
|
||||
|
||||
function isHeaderStore(headers: HeadersLike): headers is Headers {
|
||||
@@ -128,7 +149,7 @@ export class GuardrailRegistry {
|
||||
}
|
||||
|
||||
try {
|
||||
const result = await guardrail.preCall(currentPayload, context);
|
||||
const result = asGuardrailResult(await guardrail.preCall(currentPayload, context));
|
||||
const modified = result?.modifiedPayload !== undefined;
|
||||
const meta = result?.meta || null;
|
||||
|
||||
@@ -201,7 +222,7 @@ export class GuardrailRegistry {
|
||||
}
|
||||
|
||||
try {
|
||||
const result = await guardrail.postCall(currentResponse, context);
|
||||
const result = asGuardrailResult(await guardrail.postCall(currentResponse, context));
|
||||
const modified = result?.modifiedResponse !== undefined;
|
||||
const meta = result?.meta || null;
|
||||
|
||||
|
||||
121
tests/unit/guardrails-void-no-change-contract.test.ts
Normal file
121
tests/unit/guardrails-void-no-change-contract.test.ts
Normal file
@@ -0,0 +1,121 @@
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import { BaseGuardrail, GuardrailRegistry } from "../../src/lib/guardrails/index.ts";
|
||||
|
||||
/**
|
||||
* `docs/security/GUARDRAILS.md`: "A guardrail signals 'no change' by returning
|
||||
* either `void`, `{}`, or ...".
|
||||
*
|
||||
* The `void` arm of that contract had no test — `guardrails-registry.test.ts`
|
||||
* covers guardrails that return a result and one that throws, but never one
|
||||
* that simply returns nothing. These tests pin it, because the registry's
|
||||
* dispatch reads the return value's properties and must keep treating "nothing"
|
||||
* as a clean pass rather than a block or a modification.
|
||||
*/
|
||||
|
||||
class SilentGuardrail extends BaseGuardrail {
|
||||
constructor(name = "silent") {
|
||||
super(name, { priority: 10 });
|
||||
}
|
||||
|
||||
override async preCall() {
|
||||
// no return — the documented "no change" signal
|
||||
}
|
||||
|
||||
override async postCall() {
|
||||
// no return — the documented "no change" signal
|
||||
}
|
||||
}
|
||||
|
||||
class EmptyResultGuardrail extends BaseGuardrail {
|
||||
constructor(name = "empty") {
|
||||
super(name, { priority: 10 });
|
||||
}
|
||||
|
||||
override async preCall() {
|
||||
return {};
|
||||
}
|
||||
|
||||
override async postCall() {
|
||||
return {};
|
||||
}
|
||||
}
|
||||
|
||||
test("a preCall returning nothing passes the payload through untouched", async () => {
|
||||
const registry = new GuardrailRegistry();
|
||||
registry.register(new SilentGuardrail());
|
||||
|
||||
const payload = { messages: [{ role: "user", content: "hello" }] };
|
||||
const result = await registry.runPreCallHooks(payload, {});
|
||||
|
||||
assert.equal(result.blocked, false, "returning nothing must not block");
|
||||
assert.deepEqual(result.payload, payload, "payload must be forwarded unchanged");
|
||||
|
||||
const execution = result.results[0];
|
||||
assert.equal(execution?.guardrail, "silent");
|
||||
assert.equal(execution?.blocked, false);
|
||||
assert.equal(execution?.modified, false, "nothing returned means nothing modified");
|
||||
assert.equal(execution?.skipped, false, "the guardrail ran — it is not 'skipped'");
|
||||
assert.equal(execution?.error, undefined, "a silent pass is not an error");
|
||||
assert.equal(execution?.stage, "pre");
|
||||
});
|
||||
|
||||
test("a postCall returning nothing passes the response through untouched", async () => {
|
||||
const registry = new GuardrailRegistry();
|
||||
registry.register(new SilentGuardrail());
|
||||
|
||||
const response = { choices: [{ message: { content: "hi" } }] };
|
||||
const result = await registry.runPostCallHooks(response, {});
|
||||
|
||||
assert.equal(result.blocked, false);
|
||||
assert.deepEqual(result.response, response);
|
||||
|
||||
const execution = result.results[0];
|
||||
assert.equal(execution?.modified, false);
|
||||
assert.equal(execution?.skipped, false);
|
||||
assert.equal(execution?.error, undefined);
|
||||
assert.equal(execution?.stage, "post");
|
||||
});
|
||||
|
||||
test("returning an empty object behaves identically to returning nothing", async () => {
|
||||
const payload = { messages: [{ role: "user", content: "hello" }] };
|
||||
|
||||
const silent = new GuardrailRegistry();
|
||||
silent.register(new SilentGuardrail("g"));
|
||||
const silentResult = await silent.runPreCallHooks(payload, {});
|
||||
|
||||
const empty = new GuardrailRegistry();
|
||||
empty.register(new EmptyResultGuardrail("g"));
|
||||
const emptyResult = await empty.runPreCallHooks(payload, {});
|
||||
|
||||
assert.deepEqual(
|
||||
silentResult.results,
|
||||
emptyResult.results,
|
||||
"the two documented no-change signals must produce the same execution record"
|
||||
);
|
||||
assert.deepEqual(silentResult.payload, emptyResult.payload);
|
||||
});
|
||||
|
||||
test("a silent guardrail does not stop later guardrails from modifying", async () => {
|
||||
class AppendGuardrail extends BaseGuardrail {
|
||||
constructor() {
|
||||
super("append", { priority: 20 });
|
||||
}
|
||||
|
||||
override async preCall(payload: unknown) {
|
||||
return { modifiedPayload: { ...(payload as Record<string, unknown>), seen: true } };
|
||||
}
|
||||
}
|
||||
|
||||
const registry = new GuardrailRegistry();
|
||||
registry.register(new SilentGuardrail());
|
||||
registry.register(new AppendGuardrail());
|
||||
|
||||
const result = await registry.runPreCallHooks({ messages: [] }, {});
|
||||
|
||||
assert.equal((result.payload as Record<string, unknown>).seen, true);
|
||||
assert.equal(result.results.length, 2);
|
||||
assert.equal(result.results[0]?.modified, false, "silent guardrail ran first, modified nothing");
|
||||
assert.equal(result.results[1]?.modified, true, "the later guardrail still applied");
|
||||
});
|
||||
Reference in New Issue
Block a user