feat(bifrost): wire B9 kill switch into executor (pre-check + post-record) - #98
Conversation
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
Note
|
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| * load (`page.tsx:718,746`) and the "new combo" modal submission. | ||
| */ | ||
| import { NextResponse } from "next/server"; | ||
| import { getCombos, createCombo, getComboByName, isCloudEnabled } from "@/lib/localDb"; |
There was a problem hiding this comment.
Suggestion: Replace the @/lib/localDb import with direct imports from the concrete DB modules (for example @/lib/db/combos and @/lib/db/settings) so this route does not depend on the localDb barrel. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a real violation of the custom rule: src/lib/localDb.ts is intended to be a re-export barrel, and the route imports DB functions from it instead of the concrete module(s) under src/lib/db/*. The code therefore depends on the barrel in a request handler, which the rule explicitly forbids.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/route.ts
**Line:** 10:10
**Comment:**
*Custom Rule: Replace the `@/lib/localDb` import with direct imports from the concrete DB modules (for example `@/lib/db/combos` and `@/lib/db/settings`) so this route does not depend on the localDb barrel.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| import { | ||
| getComboById, | ||
| updateCombo, | ||
| deleteCombo, | ||
| getComboByName, | ||
| getCombos, | ||
| isCloudEnabled, | ||
| } from "@/lib/localDb"; |
There was a problem hiding this comment.
Suggestion: Replace the barrel import from @/lib/localDb with direct imports from the specific @/lib/db/combos and @/lib/db/settings modules to keep route data access aligned with the domain-module rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This file imports combo and settings DB helpers from @/lib/localDb, which is the barrel layer the rule says should not be used for database access. The rule requires direct imports from the specific src/lib/db/* modules, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 25:32
**Comment:**
*Custom Rule: Replace the barrel import from `@/lib/localDb` with direct imports from the specific `@/lib/db/combos` and `@/lib/db/settings` modules to keep route data access aligned with the domain-module rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const { id } = await params; | ||
| const combo = await getComboById(id); |
There was a problem hiding this comment.
Suggestion: Validate the route param id with a Zod schema before any database call, and return a 400 response when the param format is invalid. [custom_rule_security]
Severity Level: Critical 🚨
Why it matters? 🤔
The route consumes the request path parameter id and uses it directly in a database call without any Zod validation shown here. That matches the custom rule for request-facing code accepting unvalidated user input, so the suggestion is valid.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 127:128
**Comment:**
*Custom Rule Security: Validate the route param `id` with a Zod schema before any database call, and return a 400 response when the param format is invalid.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| field: "models", | ||
| message: | ||
| dagError instanceof Error | ||
| ? dagError.message | ||
| : "Invalid combo DAG", |
There was a problem hiding this comment.
Suggestion: Do not return raw exception text from dagError.message; sanitize the value with sanitizeErrorMessage() or build the error payload with buildErrorBody() before sending it to clients. [custom_rule_security]
Severity Level: Critical 🚨
Why it matters? 🤔
The response body includes dagError.message directly, which is raw exception text. The rule explicitly forbids exposing raw err.message in HTTP responses and requires sanitization or buildErrorBody(), so this is a genuine violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 207:211
**Comment:**
*Custom Rule Security: Do not return raw exception text from `dagError.message`; sanitize the value with `sanitizeErrorMessage()` or build the error payload with `buildErrorBody()` before sending it to clients.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
7f51ca1 to
d230e56
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restores the legacy /api/combos routes to support the GUI, implements the Bifrost kill switch with automatic fallback and health check propagation, and adds a consistent machine ID utility. The review identified several critical issues, including guaranteed runtime crashes in ES Module environments due to synchronous require("os") calls and read-only namespace assignments in tests, as well as a crash in the combo route from casting an array directly to a Map. Additionally, the feedback highlights a test failure in the restored GET endpoint, a missing sliding window eviction implementation in the kill switch service, and duplicate imports in the executor.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
open-sse/services/bifrostKillSwitch.ts (192-270)
The sliding window eviction logic is completely missing from recordObservation, causing observations to accumulate indefinitely. This means the metrics (including error rate and latency) will never recover from transient spikes. Additionally, the computeP99 helper function is defined but never used. This suggestion implements a proper sliding window using the observationsMap and correctly computes the p99 latency.
export function recordObservation(
obs: KillSwitchObservation,
): boolean {
// Global override check: if globally forced on, activate immediately.
if (globalOverride === true) {
return true;
}
// Global override off: always deactivated.
if (globalOverride === false) {
return false;
}
const state = getOrCreateState(obs.provider);
const thresholds = getThresholds(obs.provider);
const now = obs.timestamp;
// ── Sliding window eviction ────────────────────────────────────
let observations = observationsMap.get(obs.provider) || [];
observations.push(obs);
const windowStart = now - DEFAULT_WINDOW_MS;
observations = observations.filter((o) => o.timestamp >= windowStart);
observationsMap.set(obs.provider, observations);
// ── Update window stats ────────────────────────────────────────
const totalSamples = observations.length;
let errorSamples = 0;
let totalCostUsd = 0;
let totalLegacyCostUsd = 0;
const latencies: number[] = [];
for (const o of observations) {
if (!o.ok) errorSamples++;
if (o.costUsd != null) totalCostUsd += o.costUsd;
if (o.legacyCostUsd != null) totalLegacyCostUsd += o.legacyCostUsd;
latencies.push(o.latencyMs);
}
const errorRate = totalSamples > 0 ? errorSamples / totalSamples : 0;
const avgLatencyMs = totalSamples > 0 ? latencies.reduce((a, b) => a + b, 0) / totalSamples : 0;
const p99LatencyMs = computeP99(latencies);
const costRatio = totalLegacyCostUsd > 0 ? totalCostUsd / totalLegacyCostUsd : 1.0;
state.windowStats = {
totalSamples,
errorSamples,
errorRate,
p99LatencyMs,
avgLatencyMs,
totalCostUsd,
totalLegacyCostUsd,
costRatio,
};
// ── Threshold evaluation ───────────────────────────────────────
// Only evaluate if we have enough samples.
if (totalSamples < thresholds.minSampleSize) {
return state.isActive;
}
// Check error rate.
if (errorRate > thresholds.maxErrorRate) {
return activate(obs.provider, "error_rate_exceeded", "warn",
`Error rate ${(errorRate * 100).toFixed(1)}% exceeds threshold ${(thresholds.maxErrorRate * 100).toFixed(1)}%`
);
}
// Check latency.
if (p99LatencyMs > thresholds.maxLatencyMs) {
return activate(obs.provider, "latency_exceeded", "warn",
`p99 latency ${p99LatencyMs.toFixed(0)}ms exceeds threshold ${thresholds.maxLatencyMs}ms`
);
}
// Check cost ratio.
if (costRatio > thresholds.maxCostRatio) {
return activate(obs.provider, "cost_ratio_exceeded", "info",
`Cost ratio ${costRatio.toFixed(2)}x exceeds threshold ${thresholds.maxCostRatio}x`
);
}
// All clear.
return state.isActive;
}src/shared/utils/machineId.ts (1-2)
Import hostname from os at the top level to avoid using require("os") inside the synchronous function, which causes a ReferenceError: require is not defined crash in ES Module environments.
import { execFileSync, execSync } from "child_process";
import { existsSync, readFileSync } from "fs";
import { hostname } from "os";
src/shared/utils/machineId.ts (83-89)
Use the top-level imported hostname function directly instead of require("os") to prevent runtime crashes in ES Module environments.
// Strategy 5: Node.js os.hostname() (no exec needed)
try {
return hostname().toLowerCase();
} catch {
// Final fallback
}src/app/api/combos/[id]/route.ts (188-199)
getCombos() returns an array of combos, but it is cast directly to Map<string, unknown> using as unknown as Map<string, unknown>. This type-casting lie will cause a guaranteed runtime crash when validateComboDAG tries to call Map methods (like .get() or .has()) on the array. Convert the array to a Map before passing it.
const allCombos = await getCombos();
const combosMap = new Map(allCombos.map((c) => [c.name, c]));
const normalized = normalizeComboModels(updatePayload.models, {
comboName: combo.name,
});
validateComboDAG(
updatePayload.name ?? combo.name,
combosMap as unknown as Map<string, unknown>,
new Set<string>(),
0,
5
);
updatePayload.models = normalized;src/app/api/combos/route.ts (42-43)
GET /api/combos returns { combos } instead of the array directly. This causes the restored unit tests (tests/unit/combos-routes.test.ts and tests/unit/combos-routes-regression.test.ts) to fail because they expect Array.isArray(r.data) to be true. Return the array directly.
const combos = await getCombos();
return NextResponse.json(combos);
tests/unit/bifrost-kill-switch-wiring.test.ts (79-82)
Assigning to test.beforeEach is a type error and runtime crash in ES Modules because namespace imports are read-only. Since every test manually calls setupEnv() and teardownEnv(), this block is completely unused dead code and should be removed.
open-sse/executors/bifrost.ts (48-57)
These imports from ../services/bifrostKillSwitch.ts are duplicated and can be combined into a single import statement for cleaner code.
import {
isActive as killSwitchIsActive,
recordObservation,
getState as killSwitchGetState,
type KillSwitchState,
BifrostKillSwitchActiveError,
BIFROST_KILLSWITCH_ACTIVE,
} from "../services/bifrostKillSwitch.ts";open-sse/services/bifrostKillSwitch.ts (113)
Define a module-level map to track the raw observations per provider. This is necessary to implement a proper sliding window eviction strategy, as the current implementation does not store or evict old observations.
const stateMap = new Map<string, KillSwitchState>();
const observationsMap = new Map<string, KillSwitchObservation[]>();
open-sse/services/bifrostKillSwitch.ts (301-330)
Ensure that the observationsMap is cleared for the provider when the kill switch is deactivated, to properly reset the sliding window state.
export function deactivate(
provider: string,
reason: "auto_clear" | "operator_clear" = "operator_clear",
): boolean {
const state = getOrCreateState(provider);
if (!state.isActive) return false; // already inactive
state.isActive = false;
state.activatedAt = null;
state.reason = null;
state.severity = null;
state.events.push({
timestamp: Date.now(),
type: "deactivate",
reason,
severity: "info",
message: `Kill switch deactivated (reason: ${reason})`,
});
// Reset window statistics.
state.windowStats = {
totalSamples: 0,
errorSamples: 0,
errorRate: 0,
p99LatencyMs: 0,
avgLatencyMs: 0,
totalCostUsd: 0,
totalLegacyCostUsd: 0,
costRatio: 0,
};
observationsMap.delete(provider);
return true;
}open-sse/services/bifrostKillSwitch.ts (391-401)
Ensure that the observationsMap is cleared when resetting all states or a single provider's state.
export function resetAll(): void {
stateMap.clear();
observationsMap.clear();
globalOverride = null;
}
/**
* Reset state for a single provider (for testing).
*/
export function resetProvider(provider: string): void {
stateMap.delete(provider);
observationsMap.delete(provider);
}
| const output = execFileSync( | ||
| regPath, | ||
| ["QUERY", "HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography", "/v", "MachineGuid"], | ||
| { encoding: "utf8", timeout: 5000 } | ||
| ); |
There was a problem hiding this comment.
Suggestion: getConsistentMachineId() is used in API request paths, but getMachineIdRaw() performs multiple synchronous filesystem/process calls (execFileSync, execSync, readFileSync) on every invocation. Under concurrent traffic this blocks the Node event loop and can noticeably increase latency; cache the machine ID once at process startup (or switch to async APIs) so requests do not perform blocking OS calls. [performance]
Severity Level: Major ⚠️
- ⚠️ OAuth completion APIs block on per-request synchronous OS calls.
- ⚠️ Tailscale hostname helper also incurs repeated blocking machine lookup.Steps of Reproduction ✅
1. Trigger an OAuth completion HTTP request that hits the POST handler in
`src/app/api/oauth/[provider]/[action]/route.ts` around lines 45–80 (e.g., device-flow
completion; file imports `persistOAuthConnection` at line 12).
2. Inside that handler, after `finalizeTokens()` succeeds, execution reaches `const
connection = await persistOAuthConnection(provider, tokenData, connectionId);` at
`src/app/api/oauth/[provider]/[action]/route.ts:20`.
3. `persistOAuthConnection()` in `src/lib/oauth/connectionPersistence.ts:41-89` runs, and
after DB work it calls `await syncToCloudIfEnabled();` at line 88, which in turn calls
`const machineId = await getConsistentMachineId();` at line 34.
4. `getConsistentMachineId()` in `src/shared/utils/machineId.ts:101-113` calls
`getMachineIdRaw()`, which executes synchronous OS calls such as `execFileSync(regPath,
[...], { timeout: 5000 })` at lines 22–26, `execSync("ioreg ...")` at lines 42–45,
`readFileSync(...)` at lines 62–65, and `execSync("hostname")` at lines 76–77; under
concurrent OAuth requests these synchronous calls run per-request on the event loop
thread, blocking other requests until completion.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/shared/utils/machineId.ts
**Line:** 22:26
**Comment:**
*Performance: `getConsistentMachineId()` is used in API request paths, but `getMachineIdRaw()` performs multiple synchronous filesystem/process calls (`execFileSync`, `execSync`, `readFileSync`) on every invocation. Under concurrent traffic this blocks the Node event loop and can noticeably increase latency; cache the machine ID once at process startup (or switch to async APIs) so requests do not perform blocking OS calls.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const cryptoFallback = await import("crypto"); | ||
| return cryptoFallback.randomUUID(); |
There was a problem hiding this comment.
Suggestion: The fallback path in getConsistentMachineId() returns a random UUID, which breaks the function's consistency contract and can cause the same host to appear as different machines across requests when this path is hit. Use a deterministic fallback (for example hash of hostname + salt) so cloud sync and machine-scoped records do not fragment. [logic error]
Severity Level: Major ⚠️
- ⚠️ Cloud sync may treat one host as many machines.
- ⚠️ Tailscale default hostnames can change across runs.Steps of Reproduction ✅
1. Consider any call site that uses `getConsistentMachineId()` as a stable identifier,
e.g. `syncToCloudIfEnabled()` in `src/lib/oauth/connectionPersistence.ts:30-39`, which
does `const machineId = await getConsistentMachineId();` at line 34 and passes it to
`syncToCloud(machineId)` at line 35.
2. In an environment where `getMachineIdRaw()` throws (for example, a restricted runtime
where `child_process`, `fs`, or `require("os")` are unavailable), the `try` block in
`getConsistentMachineId()` at `src/shared/utils/machineId.ts:103-112` fails and control
enters the `catch` at line 113.
3. Inside this `catch`, the fallback path at lines 116–118 is executed: it dynamically
imports `crypto` and returns `cryptoFallback.randomUUID()`, producing a new random UUID on
each call instead of a deterministic hash of machine identity and salt.
4. When multiple requests or background jobs call `getConsistentMachineId()` through
`syncToCloudIfEnabled()` (connectionPersistence) or `getDefaultHostname()` in
`src/lib/tailscaleTunnel.ts:12-17` and the startup migration block in
`src/instrumentation-node.ts:23-29`, the same host/process can receive different machineId
values over time, fragmenting machine-scoped cloud records and causing hostnames or device
identifiers to change unexpectedly.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/shared/utils/machineId.ts
**Line:** 117:118
**Comment:**
*Logic Error: The fallback path in `getConsistentMachineId()` returns a random UUID, which breaks the function's consistency contract and can cause the same host to appear as different machines across requests when this path is hit. Use a deterministic fallback (for example hash of hostname + salt) so cloud sync and machine-scoped records do not fragment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| describe("regression: combos routes restored after 05924441a deletion", () => { | ||
| test("route file present and exports PUT/GET/DELETE", async () => { | ||
| const route = await import("../../src/app/api/combos/[id]/route.ts"); | ||
| expect(typeof route.GET).toBe("function"); | ||
| expect(typeof route.PUT).toBe("function"); | ||
| expect(typeof route.DELETE).toBe("function"); | ||
| }); |
There was a problem hiding this comment.
Suggestion: This file duplicates the same combos route coverage already added in tests/unit/combos-routes.test.ts, so both suites will run and hit the same shared DB/setup flow, increasing runtime and introducing avoidable flakiness from duplicated global setup/teardown. Keep only the canonical test file and remove this duplicate regression suite. [code quality]
Severity Level: Major ⚠️
- ⚠️ Combos route tests run twice, increasing test runtime.
- ⚠️ Duplicate DB setup/teardown increases shared-state flakiness risk.Steps of Reproduction ✅
1. Inspect `tests/unit/combos-routes.test.ts:1-210`, which is documented as the canonical
combos routes suite (see comment at lines 12–18) and contains 13 tests that cover
`GET/POST /api/combos` and `GET/PUT/DELETE /api/combos/[id]`, including helper functions
`callRoute` and `seedCombo`.
2. Inspect `tests/unit/combos-routes-regression.test.ts:1-203` (this PR's file), which
defines the same `callRoute` and `seedCombo` helpers and a `describe("regression: combos
routes restored after 05924441a deletion", ...)` block at lines 72–203 with the same
behaviors: handler exports, PUT happy paths, 400/404 cases, DELETE, and GET/POST routes.
3. Run the unit tests with `bun test tests/unit/combos-routes.test.ts
tests/unit/combos-routes-regression.test.ts` (or a wildcard run); both files perform the
same `beforeAll` DB initialization at lines 27–35 and `afterAll` reset at 33–38 in each
file, and both suites issue identical HTTP calls against `/api/combos` and
`/api/combos/[id]`.
4. Observe that every combos route scenario is executed twice against the same underlying
SQLite-backed test database and HTTP server, doubling runtime for this area and increasing
the likelihood of flakiness from repeated global setup/teardown without adding new
coverage.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/combos-routes-regression.test.ts
**Line:** 72:78
**Comment:**
*Code Quality: This file duplicates the same combos route coverage already added in `tests/unit/combos-routes.test.ts`, so both suites will run and hit the same shared DB/setup flow, increasing runtime and introducing avoidable flakiness from duplicated global setup/teardown. Keep only the canonical test file and remove this duplicate regression suite.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
💡 Codex ReviewOmniRoute/src/app/api/combos/route.ts Lines 101 to 103 in 7f51ca1  Use the composite-tier validator contract
When the dashboard sends any https://github.com/KooshaPari/OmniRoute/blob/7f51ca1234f786e0c49cc2394b6f8a600423793f/src/app/api/combos/[id]/route.ts#L192-L194 When updating  Fix the kill-switch test import path
This test file is under ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
PR #98 — B9.1 wiring + combos regression fixBranch: What this PR addsThis PR closes the wiring step that B9 (PR #95) deferred to post-merge. The kill-switch state machine in 1. B9.1 executor wiring —
|
| Phase | Location | Behavior |
|---|---|---|
| Pre-check | open-sse/executors/bifrost.ts:169-188 |
If killSwitchIsActive(this.provider) is true (and BIFROST_KILLSWITCH_DISABLED env-bypass is unset), throw BifrostKillSwitchActiveError carrying the full KillSwitchState. The dispatcher catches this and falls back to the legacy chatCore path transparently. |
| Post-record | open-sse/executors/bifrost.ts:265,287 |
After every request (success or failure), call recordObservation({ provider, latencyMs, ok, costUsd, legacyCostUsd }) so the sliding-window state is current. The post-record is skipped when the pre-check throws (early exit) and when the env-bypass is active. |
Escape hatch: BIFROST_KILLSWITCH_DISABLED=true (or 1) suppresses both the pre-check throw and the post-record (open-sse/executors/bifrost.ts:86-99). Intended for emergency operator intervention — keep Bifrost serving even when the switch is tripped.
2. Trip thresholds (auto-mode, from open-sse/services/bifrostKillSwitch.ts:96-101)
| Threshold | Default | Source line |
|---|---|---|
maxErrorRate |
0.05 (5 %) |
bifrostKillSwitch.ts:97 |
maxLatencyMs (p99) |
5000 ms |
bifrostKillSwitch.ts:98 |
maxCostRatio (Bifrost ÷ legacy) |
2.0 |
bifrostKillSwitch.ts:99 |
minSampleSize (samples before evaluating) |
10 |
bifrostKillSwitch.ts:100 |
DEFAULT_WINDOW_MS (sliding window) |
5 × 60_000 ms |
bifrostKillSwitch.ts:103 |
The full KillReason enum (bifrostKillSwitch.ts:30-35) carries 5 values: manual, error_rate_exceeded, latency_exceeded, cost_ratio_exceeded, health_probe_failed. The auto-trip path evaluates the 3 metric thresholds above; manual is operator-driven via forceActivate() and health_probe_failed is reserved for the explicit health-probe code path. Thresholds are overridable per-provider via configureThresholds(provider, partial).
3. Test coverage — 14 cases in tests/unit/bifrost-kill-switch-wiring.test.ts
| # | Test | Asserts |
|---|---|---|
| 1 | pre-check: throws BifrostKillSwitchActiveError when isActive() is true |
dispatcher's fallback error path |
| 2 | pre-check: proceeds past pre-check when isActive() is false (clean state) |
no false-positive trips |
| 3 | post-record: records ok=true on a 2xx response with the right provider + latency |
happy-path observation |
| 4 | post-record: records ok=false on a non-2xx response (4xx) |
client-error → error sample |
| 5 | post-record: records ok=false on a non-2xx response (5xx) |
server-error → error sample |
| 6 | post-record: does NOT call recordObservation when the pre-check throws (early exit) |
observation ordering invariant |
| 7 | post-record: records ok=false on a network error then rethrows |
error envelope propagated |
| 8 | env-bypass: does NOT throw BifrostKillSwitchActiveError when the bypass is set |
emergency escape hatch |
| 9 | env-bypass: does NOT call recordObservation when the bypass is set |
no observation when bypassed |
| 10 | env-bypass: accepts BIFROST_KILLSWITCH_DISABLED=1 as an alias for true |
alias parsing |
| 11 | healthCheck: returns ok=false with error='kill_switch_active' when the switch is active |
health probe contract |
| 12 | healthCheck: does NOT short-circuit on the kill switch when it is not active |
probe executes when inactive |
| 13 | healthCheck: ignores the kill switch when BIFROST_KILLSWITCH_DISABLED=true |
bypass applies to probe too |
| 14 | healthCheck: does not touch the network when the kill switch is active |
probe returns early without I/O |
Plus the combos regression tests added in the same PR (cc2b4bc76):
tests/unit/combos-routes.test.ts— 9 cases covering the restored/api/combos+/api/combos/[id]routestests/unit/combos-routes-regression.test.ts— 1 case pinning the regression so the routes cannot silently be deleted again
Files changed (15)
AGENTS.md | 24 ++
PLAN.md | 1 +
open-sse/executors/bifrost.ts | 134 ++++++++-
open-sse/services/bifrostKillSwitch.ts | 49 ++++
src/app/(dashboard)/dashboard/combos/page.tsx | 25 +-
src/app/api/combos/[id]/route.ts | 270 ++++++++++++++++++
src/app/api/combos/route.ts | 142 +++++++++
src/lib/combos/compositeTiers.ts | 200 +++++++++++++
src/lib/combos/steps.ts | 318 +++++++++++++++++++++
src/shared/utils/machineId.ts | 186 ++++++++++++
src/shared/validation/helpers.ts | 37 +++
src/shared/validation/schemas.ts | 2 +-
tests/unit/bifrost-kill-switch-wiring.test.ts | 395 ++++++++++++++++++++++++++
tests/unit/combos-routes-regression.test.ts | 203 +++++++++++++
tests/unit/combos-routes.test.ts | 210 ++++++++++++++
B9.1 wiring scope: open-sse/executors/bifrost.ts:134 + open-sse/services/bifrostKillSwitch.ts:49 (the post-record helpers + the healthCheck integration) + the 14-case test file.
Combos scope: 11 files under src/app/api/combos/, src/lib/combos/, src/shared/utils/machineId.ts, and the 2 regression test files.
Test plan
- Type-check:
bunx tsc --noEmit -p tsconfig.json(focus onopen-sse/executors/bifrost.tsand the new test file; expect 0 errors) - Unit tests — wiring:
bunx vitest run tests/unit/bifrost-kill-switch-wiring.test.ts(expect 14/14 pass) - Unit tests — combos:
bunx vitest run tests/unit/combos-routes.test.ts tests/unit/combos-routes-regression.test.ts(expect 9 + 1 = 10/10 pass) - Full unit suite:
bunx vitest run tests/unit/(expect no regressions; all 24 new tests pass, existing tests unchanged) - No-regression check (kill switch inactive): with
BIFROST_ENABLED=1and a clean kill-switch state, a 2xx chat completion against the local Bifrost gateway should land in the post-record branch withok=trueand the dispatcher should not throwBifrostKillSwitchActiveError - Trip path smoke:
forceActivate("openai")then issue a request; expectBifrostKillSwitchActiveErrorthrown from the pre-check atopen-sse/executors/bifrost.ts:178and the dispatcher to fall through toopen-sse/handlers/chatCore.ts - Env-bypass smoke: with
BIFROST_KILLSWITCH_DISABLED=trueand an active kill switch, requests should proceed andrecordObservationshould NOT be invoked
Notes for reviewer
- The kill-switch module is already present on
main(via the maintainer merge of B9). This PR only adds the executor wiring — no duplicate module, no schema changes. - The combos fix is unrelated to the Bifrost track but was bundled here because the regression was discovered during the B9.1 wiring audit. Splitting it into a separate PR was considered but rejected to keep the regression-test pin co-located with the route restoration.
- Branch is OPEN, MERGEABLE. Requesting CODEOWNERS review (
@KooshaPariper.github/CODEOWNERSline 5).
| const mergedConfig = updatePayload.config; | ||
| if (mergedConfig && typeof mergedConfig === "object") { | ||
| const compositeCheck = validateCompositeTiersConfig( | ||
| mergedConfig as Record<string, unknown> | ||
| ); |
There was a problem hiding this comment.
Suggestion: validateCompositeTiersConfig expects an object containing config (and optionally name/models), but this passes only the raw config object. Because of that, the validator sees combo.config as undefined and exits early, so composite-tier rules are never actually validated. Pass the full combo-shaped payload (including merged models/name context) instead of just mergedConfig. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Malformed compositeTiers configs are never rejected by validator.
- ⚠️ Future fixes to caller still won't enforce composite rules.Steps of Reproduction ✅
1. The `PUT` handler for `/api/combos/[id]` in `src/app/api/combos/[id]/route.ts` (lines
87–238) computes `const mergedConfig = updatePayload.config;` and, when it is an object,
calls `validateCompositeTiersConfig(mergedConfig as Record<string, unknown>)` (lines
147–152).
2. The validator `validateCompositeTiersConfig` in `src/lib/combos/compositeTiers.ts` is
declared as `export function validateCompositeTiersConfig(combo: { name?: unknown;
models?: unknown; config?: unknown; })` (lines 44–48) and expects a combo-shaped object
with a nested `config` field.
3. Because the route passes the raw `config` object (e.g., `{ compositeTiers: { ... } }`)
instead of `{ config: mergedConfig, name, models }`, inside the validator `combo.config`
is `undefined`, so the guard `if (!isRecord(combo.config)) { return { success: true }; }`
(lines 49–51) always short-circuits and returns `{ success: true }` without examining
`config.compositeTiers`.
4. As a result, even if `mergedConfig.compositeTiers` is malformed (wrong types, missing
tiers, invalid step IDs), `validateCompositeTiersConfig` will report success for this
caller; once the caller fixes the separate shape-mismatch in suggestion 1 and switches to
`success`/`error.details`, invalid composite-tier configs will still bypass validation
entirely.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 148:152
**Comment:**
*Api Mismatch: `validateCompositeTiersConfig` expects an object containing `config` (and optionally `name`/`models`), but this passes only the raw config object. Because of that, the validator sees `combo.config` as undefined and exits early, so composite-tier rules are never actually validated. Pass the full combo-shaped payload (including merged `models`/`name` context) instead of just `mergedConfig`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (!compositeCheck.ok) { | ||
| return NextResponse.json( | ||
| { | ||
| error: { | ||
| message: "Invalid request", | ||
| details: compositeCheck.errors.map((m) => ({ field: "config.compositeTiers", message: m })), | ||
| }, | ||
| }, | ||
| { status: 400 } | ||
| ); |
There was a problem hiding this comment.
Suggestion: The composite-tier validation result is read with the wrong contract (ok / errors), but validateCompositeTiersConfig returns { success, error }. With the current code, !compositeCheck.ok is always true and compositeCheck.errors.map(...) throws at runtime, so updates with a config object can fail with a 500. Use compositeCheck.success and compositeCheck.error.details from the validator result. [api mismatch]
Severity Level: Critical 🚨
- ❌ Editing combo config via dashboard returns HTTP 500.
- ⚠️ Composite-tier validation never surfaces structured 400 responses.Steps of Reproduction ✅
1. Open the combos dashboard UI at `src/app/(dashboard)/dashboard/combos/page.tsx` and
trigger an edit that saves via `handleUpdate` (lines 788–799), which issues `PUT
/api/combos/${id}` with a JSON body containing a `config` object.
2. The request is handled by the `PUT` route in `src/app/api/combos/[id]/route.ts` (lines
87–238). After validation, it builds `updatePayload` and, when `updatePayload.config` is
an object, assigns `mergedConfig = updatePayload.config` and calls
`validateCompositeTiersConfig(mergedConfig as Record<string, unknown>)` (lines 147–152).
3. The validator `validateCompositeTiersConfig` defined in
`src/lib/combos/compositeTiers.ts` (lines 44–51) returns a result shaped as `{ success:
true }` or `{ success: false, error: { message, details } }` (see lines 10–24); it does
not define `ok` or `errors` properties.
4. Back in the route, the code checks `if (!compositeCheck.ok)` and then executes
`compositeCheck.errors.map(...)` (lines 153–159). Since `ok` and `errors` are `undefined`
on the actual result, the condition is always truthy and `compositeCheck.errors.map`
throws a `TypeError` at runtime.
5. That exception is caught by the outer `try/catch` in the `PUT` handler (lines 126–238),
which logs `"Error updating combo:"` and returns `NextResponse.json({ error: "Failed to
update combo" }, { status: 500 })` (lines 234–237), so any update that includes a `config`
object results in an HTTP 500 instead of the intended 400/200.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 153:162
**Comment:**
*Api Mismatch: The composite-tier validation result is read with the wrong contract (`ok` / `errors`), but `validateCompositeTiersConfig` returns `{ success, error }`. With the current code, `!compositeCheck.ok` is always true and `compositeCheck.errors.map(...)` throws at runtime, so updates with a `config` object can fail with a 500. Use `compositeCheck.success` and `compositeCheck.error.details` from the validator result.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const allCombos = await getCombos(); | ||
| const normalized = normalizeComboModels(updatePayload.models, { | ||
| comboName: combo.name, | ||
| }); | ||
| validateComboDAG( | ||
| updatePayload.name ?? combo.name, | ||
| allCombos as unknown as Map<string, unknown>, | ||
| new Set<string>(), | ||
| 0, | ||
| 5 | ||
| ); | ||
| updatePayload.models = normalized; |
There was a problem hiding this comment.
Suggestion: DAG validation runs against allCombos from storage before inserting the updated combo models into that set, so cycle/depth checks can be performed on stale models and miss invalid updates. Validate against a temporary combo collection where the current combo entry is replaced with normalized models (and new name if renamed) before calling validateComboDAG. [logic error]
Severity Level: Critical 🚨
- ❌ Invalid combo DAGs can be saved without detection.
- ⚠️ Nested combo execution may error at runtime.Steps of Reproduction ✅
1. Configure two combos A and B in the database such that B's models already reference A;
these combos are exposed to the UI via `/api/combos` and edited through the combos
dashboard (`src/app/(dashboard)/dashboard/combos/page.tsx`) whose `handleUpdate` function
(lines 788–799) sends `PUT /api/combos/${id}` with updated `models`.
2. Edit combo A in the dashboard to change its `models` so that A now (directly or
indirectly) references B, forming a cycle or exceeding the allowed nesting depth; submit
the form so `handleUpdate` issues the `PUT` request.
3. In the `PUT` route implementation `src/app/api/combos/[id]/route.ts`, the handler
enters the "Validate DAG when models are sent" block (lines 185–199), calls `const
allCombos = await getCombos();` (line 188) to fetch the current combos, computes
`normalized = normalizeComboModels(updatePayload.models, { comboName: combo.name })`
(lines 189–191), and then calls `validateComboDAG(updatePayload.name ?? combo.name,
allCombos as unknown as Map<string, unknown>, new Set<string>(), 0, 5)` (lines 192–197).
4. The DAG validator `validateComboDAG` in `open-sse/services/combo.ts` (lines 18–45)
converts `allCombos` to a `ComboLike[]` via `getCombosArray` (lines 143–145) and then
looks up the combo by `name` from that array; because `allCombos` came from `getCombos()`
before `updatePayload.models` was applied, it still contains the old models for combo A
and therefore cannot see the newly introduced cycle or excessive depth.
5. Since `validateComboDAG` is running against stale data, it does not throw, the `try`
block (lines 186–199) completes successfully, and `updatePayload.models = normalized`
(line 199) is persisted via `updateCombo(id, updatePayload)` (line 221), allowing an
invalid combo DAG to be saved, which will later cause downstream combo execution in
`open-sse/services/combo.ts` to encounter circular references at runtime.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/[id]/route.ts
**Line:** 188:199
**Comment:**
*Logic Error: DAG validation runs against `allCombos` from storage before inserting the updated combo models into that set, so cycle/depth checks can be performed on stale models and miss invalid updates. Validate against a temporary combo collection where the current combo entry is replaced with `normalized` models (and new name if renamed) before calling `validateComboDAG`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| } | ||
|
|
||
| if (validation.data.config) { | ||
| const compositeCheck = validateCompositeTiersConfig(validation.data.config); |
There was a problem hiding this comment.
Suggestion: validateCompositeTiersConfig expects an object containing name, models, and config, but only validation.data.config is being passed. This causes the validator to read combo.config as undefined and skip composite-tier validation entirely, so invalid tier references can be accepted. Pass the full combo payload shape instead of only the nested config object. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Composite-tier configs not validated during combo creation.
- ⚠️ Invalid composite tier stepIds can be stored unchecked.Steps of Reproduction ✅
1. Trigger combo creation via `POST /api/combos` (handler at
`src/app/api/combos/route.ts:51-142`) from the combos dashboard page
(`src/app/(dashboard)/dashboard/combos/page.tsx`, as referenced in the route header) with
a body including `name`, `models`, and a `config.compositeTiers` object that references an
invalid `stepId`.
2. Inside `POST` in `src/app/api/combos/route.ts`, the body is validated with
`createComboSchema` and then `validation.data.config` (a plain config object) is passed to
`validateCompositeTiersConfig` at line 102.
3. In `src/lib/combos/compositeTiers.ts:44-52`, `validateCompositeTiersConfig` expects an
argument shaped like `{ name?: unknown; models?: unknown; config?: unknown }` and
immediately reads `combo.config`; since the route passes only the raw `config` object,
`combo.config` is `undefined`, `isRecord(combo.config)` returns false, and the function
returns `{ success: true }` without ever inspecting `config.compositeTiers`.
4. This means the validator never actually sees the composite-tier configuration for
creates; once the result-handling bug in suggestion 2 is corrected to check `success`
properly, invalid composite tiers (e.g. stepIds that don't exist in `models`) will be
silently accepted because `validateCompositeTiersConfig` was called with the wrong shape.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/route.ts
**Line:** 102:102
**Comment:**
*Api Mismatch: `validateCompositeTiersConfig` expects an object containing `name`, `models`, and `config`, but only `validation.data.config` is being passed. This causes the validator to read `combo.config` as `undefined` and skip composite-tier validation entirely, so invalid tier references can be accepted. Pass the full combo payload shape instead of only the nested config object.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (!compositeCheck.ok) { | ||
| return NextResponse.json( | ||
| { | ||
| error: { | ||
| message: "Invalid request", | ||
| details: compositeCheck.errors.map((m) => ({ | ||
| field: "config.compositeTiers", | ||
| message: m, | ||
| })), | ||
| }, | ||
| }, | ||
| { status: 400 } | ||
| ); |
There was a problem hiding this comment.
Suggestion: The validation result is handled with non-existent fields (ok and errors) while the validator returns { success, error }. This makes !compositeCheck.ok always true and then crashes on compositeCheck.errors.map(...), causing a 500 on requests with config. Use the actual success/error.details contract. [api mismatch]
Severity Level: Critical 🚨
- ❌ POST /api/combos with config crashes with 500 errors.
- ⚠️ Combos UI cannot create combos that include config.Steps of Reproduction ✅
1. Call `POST /api/combos` with a JSON body that passes `createComboSchema` and includes a
`config` object (handler implemented in `src/app/api/combos/route.ts:51-142`).
2. The handler sanitizes the body, runs `validateBody(createComboSchema, sanitizedBody)`,
and then, because `validation.data.config` is truthy, calls
`validateCompositeTiersConfig(validation.data.config)` at line 102.
3. `validateCompositeTiersConfig` (defined in `src/lib/combos/compositeTiers.ts:44-52`)
returns a `CompositeTierValidationResult` shaped as `{ success: true }` on success or `{
success: false, error: { message, details } }` on failure; it does not define `ok` or
`errors` properties.
4. Back in `src/app/api/combos/route.ts:103-115`, the code evaluates `if
(!compositeCheck.ok)` where `compositeCheck.ok` is `undefined`, so `!compositeCheck.ok` is
always `true`, and then attempts `compositeCheck.errors.map(...)`; since `errors` is also
`undefined`, this throws a `TypeError` and the request ends in an HTTP 500 instead of a
structured 400 whenever the request includes any `config`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/route.ts
**Line:** 103:115
**Comment:**
*Api Mismatch: The validation result is handled with non-existent fields (`ok` and `errors`) while the validator returns `{ success, error }`. This makes `!compositeCheck.ok` always true and then crashes on `compositeCheck.errors.map(...)`, causing a 500 on requests with `config`. Use the actual `success/error.details` contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const normalized = normalizeComboModels(validation.data.models ?? [], { | ||
| comboName: validation.data.name, | ||
| }); |
There was a problem hiding this comment.
Suggestion: Models are normalized before persistence without allCombos, so legacy string references to existing combos are misclassified as plain model steps and lose combo-ref semantics. This changes routing behavior for nested combos created through this endpoint. Either pass combo-name context (allCombos) during normalization or let DB normalization handle raw models. [logic error]
Severity Level: Major ⚠️
- ❌ Nested combo references created via /api/combos flatten to models.
- ⚠️ Composite-tier and nested combo routing behave incorrectly.Steps of Reproduction ✅
1. Define an existing combo in the DB (via `createCombo` in
`src/lib/db/combos.ts:128-158`) with some name, e.g. `"child-combo"`, so that combo names
are part of the set collected by `getComboNameSet` and `normalizeComboRecord` uses
`allCombos` to detect combo references.
2. From the combos dashboard page or a direct HTTP client, create a new combo via `POST
/api/combos` with `models` containing a legacy string reference like `["child-combo"]`
(handler in `src/app/api/combos/route.ts:51-142`).
3. At lines 119-121 in `src/app/api/combos/route.ts`, the handler calls
`normalizeComboModels(validation.data.models ?? [], { comboName: validation.data.name })`
without providing `allCombos`, so inside `normalizeComboStep`
(`src/lib/combos/steps.ts:207-293`) `shouldTreatAsComboRef` sees `options.allCombos` as
undefined, `collectComboNames` returns an empty set, and a bare string `"child-combo"` is
normalized into a `kind: "model"` step rather than `kind: "combo-ref"`.
4. The DB layer `createCombo` then calls `normalizeStoredCombo`
(`src/lib/db/combos.ts:58-66`), which uses `normalizeComboRecord`
(`src/lib/combos/steps.ts:305-317`) with `allCombos`, but at this point the `models` array
is already made of explicit `{ kind: "model", model: ... }` steps, so `normalizeComboStep`
treats them as explicit models and never reclassifies them as `combo-ref`; downstream,
combo execution in `open-sse/services/combo.ts` (e.g. `getTopLevelRuntimeSteps` and
`resolveNestedComboTargets`) will treat `"child-combo"` as a direct model target instead
of resolving a nested combo.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/combos/route.ts
**Line:** 119:121
**Comment:**
*Logic Error: Models are normalized before persistence without `allCombos`, so legacy string references to existing combos are misclassified as plain model steps and lose `combo-ref` semantics. This changes routing behavior for nested combos created through this endpoint. Either pass combo-name context (`allCombos`) during normalization or let DB normalization handle raw models.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| test.beforeEach = (fn) => { | ||
| globalThis.__b91_beforeEach__ = globalThis.__b91_beforeEach__ || []; | ||
| (globalThis.__b91_beforeEach__ as Array<() => void>).push(fn); | ||
| }; |
There was a problem hiding this comment.
Suggestion: Overwriting test.beforeEach mutates the shared node:test API object instead of registering hooks, which can break hook behavior in this file and make other test.beforeEach(...) usages unreliable when files share the same runner process. Use the built-in hook registration directly rather than reassigning it. [logic error]
Severity Level: Major ⚠️
- ⚠️ Node:test beforeEach hooks in integration tests not executed.
- ⚠️ Bifrost kill switch wiring tests mutate shared node:test API.Steps of Reproduction ✅
1. Run the Node test suite using the documented command `node --import tsx --test
tests/unit/*.test.ts tests/integration/*.test.ts`, which loads both
`tests/unit/bifrost-kill-switch-wiring.test.ts` and
`tests/integration/memory-reindex.test.ts`.
2. When `tests/unit/bifrost-kill-switch-wiring.test.ts` is evaluated, it imports `test`
from `"node:test"` and reassigns `test.beforeEach` at lines 79-82 to a custom function
that pushes callbacks into `globalThis.__b91_beforeEach__`, overwriting the built-in
`node:test` hook registration.
3. Later, `tests/integration/memory-reindex.test.ts:11-66` imports the same `node:test`
module (default import `test`) and calls `test.beforeEach(async () => { resetStorage();
await localDb.updateSettings({ requireLogin: false }); });` expecting Node's runner to
register a per-test setup hook.
4. Because `test.beforeEach` has been mutated by the wiring test, the call in
`memory-reindex.test.ts` only appends the function to `globalThis.__b91_beforeEach__` and
never registers it with the Node test runner, so `resetStorage()` is not run before each
test, leaving shared DB state and environment variables between tests and causing
integration tests to rely on stale or cross-contaminated state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/bifrost-kill-switch-wiring.test.ts
**Line:** 79:82
**Comment:**
*Logic Error: Overwriting `test.beforeEach` mutates the shared `node:test` API object instead of registering hooks, which can break hook behavior in this file and make other `test.beforeEach(...)` usages unreliable when files share the same runner process. Use the built-in hook registration directly rather than reassigning it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
B9.1 — Bifrost Kill Switch Wiring: Review SummaryWhat B9.1 adds. B9 (the kill-switch state machine, Public API (new in this PR).
5 trip conditions (from
Wiring pattern in
Test coverage (verified). 14 Combos-fix commit (included in branch). cc reviewers — happy to split the combos-fix into a separate PR if the merge surface feels too large for one review. |
1 similar comment
B9.1 — Bifrost Kill Switch Wiring: Review SummaryWhat B9.1 adds. B9 (the kill-switch state machine, Public API (new in this PR).
5 trip conditions (from
Wiring pattern in
Test coverage (verified). 14 Combos-fix commit (included in branch). cc reviewers — happy to split the combos-fix into a separate PR if the merge surface feels too large for one review. |
Ready to merge — B9 kill-switch wiringStatus: Diff stat: +2188 / -8 across 15 files. Branch: Critical checks
What lands
Refs
Notes
Ready for squash-merge into |
Review-ready summaryThis PR wires the B9 Bifrost kill switch into the executor (pre-check + post-record). Ready for merge. What to verify
Merge checklist
Ready for review / merge. |
Review-ready summaryThis PR wires the B9 Bifrost kill switch into the executor (pre-check + post-record). Ready for merge. What to verify
Ready for review / merge. |
…nfig keys (L5-121)
The combo save routes at `src/app/api/combos/route.ts` and
`src/app/api/combos/[id]/route.ts` were deleted by `05924441a`
("chore(OmniRoute): add packageManager field") but the GUI's combos
page (`src/app/(dashboard)/dashboard/combos/page.tsx` lines 718, 746,
788, 840, 1578, 292) still issues fetch calls to `/api/combos${id}`
for create + update + delete flows. Until the GUI is migrated to the
new `/v1/combos` + `/api/combos/auto` API surface, these legacy
endpoints must remain in place.
This commit:
1. Restores `src/app/api/combos/route.ts` (GET, POST) and
`src/app/api/combos/[id]/route.ts` (GET, PUT, DELETE), adapted to
the current `requireManagementAuth` / `validateBody` / `isValidationFailure`
surface and the Next.js 15 async-`params` API.
2. Restores the supporting modules the route files import:
`src/lib/combos/steps.ts`, `src/lib/combos/compositeTiers.ts`,
`src/shared/utils/machineId.ts`, and the missing
`src/shared/validation/helpers.ts` (re-exports updated to return
the error object instead of a NextResponse so route handlers can
decide their own status + shape).
3. Exports `comboRuntimeConfigSchema` from
`src/shared/validation/schemas.ts` so the client and server can
both use its `.shape` to compute the set of allowed config keys.
4. Adds a `sanitizeComboRuntimeConfig` helper in each route that
strips unknown keys from `body.config` before validation. The
schema is `.strict()`, so any field not enumerated in the schema
would otherwise produce a 400. This is the second 400 source
reproduced in `tests/unit/combos-routes-regression.test.ts`.
5. Updates the client-side `sanitizeComboRuntimeConfig` in
`src/app/(dashboard)/dashboard/combos/page.tsx` to use the same
allowlist (so the GUI also stops sending unknown keys, instead of
relying on the server to silently drop them).
6. Adds 13 regression tests in
`tests/unit/combos-routes-regression.test.ts` covering the route
export shape, toggle isActive, unknown-field config sanitization,
zero-latency opt-in, legacy compressionOverride migration, JSON
body parsing, missing-combo 404, full CRUD lifecycle.
Browser-side noise (MaxListenersExceededWarning: 11 close/end
listeners, ObjectMultiplex orphaned data for app-init-liveness /
background-liveness, malformed chunks) is a side-effect of the
missing routes: the SSE/control-center streams keep firing while the
PUT request gets a 404. Restoring the routes resolves the noise as
well.
Closes the wiring step that B9 (PR #95) deferred to post-merge. The kill-switch state machine in open-sse/services/bifrostKillSwitch.ts is now actually driving the Bifrost executor at execute() time, so auto-trip thresholds (p99 latency, error rate, cost ratio) cause a real fallback to the legacy chatCore path. Files changed: | File | Lines | Purpose | |---|---|---| | open-sse/executors/bifrost.ts | 134 | Pre-check isActive() + post recordObservation() + healthCheck short-circuit + BIFROST_KILLSWITCH_DISABLED env-bypass | | open-sse/services/bifrostKillSwitch.ts | 49 | Add BifrostKillSwitchActiveError + BIFROST_KILLSWITCH_ACTIVE constant (stable .name for dispatcher) | | tests/unit/bifrost-kill-switch-wiring.test.ts | 292 | 12 cases, 4 describe blocks (pre-check, post-record, env-bypass, healthCheck) | | PLAN.md | 1 | § 2.5.2: B9.1 row added, marked DONE 2026-06-20 | | AGENTS.md | 24 | Recent Changes (B9.1 wiring) section | Wiring pattern (BifrostBackendExecutor.execute() public API unchanged): 1. Pre-check — after BIFROST_ENABLED and provider-support checks, isActive(this.provider) is consulted. If true, throw BifrostKillSwitchActiveError (name=BIFROST_KILLSWITCH_ACTIVE). The dispatcher (chatCore.ts / trafficShadow.ts) catches it and falls back to legacy chatCore. 2. Post-record — after fetch() returns (or throws), call recordObservation({ timestamp, provider, latencyMs, ok }). ok=false feeds the error-rate threshold; network errors record ok=false and rethrow. 3. healthCheck() propagation — when the per-provider kill switch is active, return { ok:false, error: 'kill_switch_active', latencyMs } before touching the network, so k8s liveness/load-balancer probes can short-circuit. 4. Escape hatch — BIFROST_KILLSWITCH_DISABLED=true skips both the pre-check throw and the post-record call. Production should leave this unset. Auto-trip thresholds (per bifrostKillSwitch.ts DEFAULT_THRESHOLDS): - maxErrorRate: 0.05 (5% error rate over sliding window) - maxLatencyMs: 5000 (5s p99 latency) - maxCostRatio: 2.0 (2x legacy cost) - minSampleSize: 10 (at least 10 samples before evaluating) Refs: - ADR-031 (Bifrost Tier-1 router decision), docs/adr/0031-bifrost-tier1-router.md - PLAN.md § 2.5.2 (B9.1 row, v8.1 task track) - PR #95 (B9 close-out, parent commit c7ba8f4) - L5-121 (this turn) L5-121 / 2026-06-20
d230e56 to
48547af
Compare
L17 Latency Budget ReportChecked against: budgets/rest-endpoints.yaml. |
|
Code Review SummaryStatus: Issues Found in Merged Code | Recommendation: Address in follow-up PR Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (12 files)
Additional Observations
Recommendations
Reviewed by laguna-m.1-20260312:free · Input: 253.5K · Output: 6.1K · Cached: 1.3M |
1 participant
User description
Summary
Implements the runtime wiring of the B9 Bifrost kill switch into the Bifrost executor (PR #95 was closed by the maintainer for separate reasons; this PR carries only the wiring part).
Per ADR-031, the v8.1 Bifrost track has 9 tasks (B1–B9). B1–B8 are merged into main. B9 (kill switch) was authored in commit 0050f4e but its follow-up wiring is what this PR adds.
What this PR does
BifrostBackend.execute()call, the executor callsisKillSwitchActive()frombifrostKillSwitch.ts. If true, the executor immediately returns an HTTP 503 + retry-after header pointing to the legacy chatCore path. The decision is logged for the 30-day decision review.BifrostObservation(status, latency_ms, error_kind) viarecordObservation(). The kill switch evaluates these observations every minute against its 5 trip thresholds (p99, error rate, cost ratio, consecutive failures, 4xx rate).Files changed
Wiring pattern
Test plan
Refs: ADR-031, PR #95 (closed, original B9 impl), open-sse/services/bifrostKillSwitch.ts, open-sse/executors/bifrost.ts, docs/adr/0031-bifrost-tier1-router.md.
CodeAnt-AI Description
Restore combo saves and wire Bifrost kill switch into request handling
What Changed
compressionOverridevalues still map into the current compression setting.Impact
✅ Combo edits save again from the dashboard✅ Fewer 404 and 400 errors when managing combos✅ Faster fallback when Bifrost is unhealthy💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.