From 4a8e3a95985123836a599d89b3883a5652274db9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:24:21 -0700 Subject: [PATCH 1/4] chore(selfhost): validate Grafana dashboard JSON and Prometheus alert-rule syntax (#1943) Completes the last unverified deliverable from #1943 -- the rest (dashboard panels, alert rules, troubleshooting runbooks) was audited and found already comprehensive or shipped in #2638. Grafana and Prometheus both fail OPEN on a malformed dashboard/rule file (skip it, log a warning), so nothing else catches this until an operator notices a panel or alert is simply missing. scripts/validate-observability-configs.mjs checks every grafana/dashboards/*.json file parses and has a title + panels array, and prometheus/rules/alerts.yml parses and every rule has alert/expr/labels.severity/annotations.summary. Wired into test:ci. 12 unit tests cover both validators' happy paths and failure modes. --- package.json | 3 +- scripts/validate-observability-configs.d.mts | 3 + scripts/validate-observability-configs.mjs | 89 ++++++++++++++ ...idate-observability-configs-script.test.ts | 116 ++++++++++++++++++ 4 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 scripts/validate-observability-configs.d.mts create mode 100644 scripts/validate-observability-configs.mjs create mode 100644 test/unit/validate-observability-configs-script.test.ts diff --git a/package.json b/package.json index a415ff50d5..1ea9cc4e44 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "selfhost:postgres:migrate": "tsx scripts/migrate-selfhost-sqlite-to-postgres.ts", "selfhost:env-reference": "node scripts/gen-selfhost-env-reference.mjs", "selfhost:env-reference:check": "node scripts/gen-selfhost-env-reference.mjs --check", + "selfhost:validate-observability": "node scripts/validate-observability-configs.mjs", "cf-typegen": "wrangler types && perl -pi -e 's/[[:blank:]]+$//' worker-configuration.d.ts", "cf-typegen:check": "wrangler types --check", "db:migrate:local": "wrangler d1 migrations apply gittensory --local", @@ -66,7 +67,7 @@ "test:smoke:observability": "node scripts/smoke-observability-traces.mjs", "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node scripts/smoke-ui-browser.mjs", - "test:ci": "git diff --check && npm run actionlint && npm run db:migrations:check && npm run selfhost:env-reference:check && npm run cf-typegen:check && npm run typecheck && npm run test:coverage && npm run test:workers && npm run build:mcp && npm run test:mcp-pack && npm run build:miner && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run db:migrations:check && npm run selfhost:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run typecheck && npm run test:coverage && npm run test:workers && npm run build:mcp && npm run test:mcp-pack && npm run build:miner && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci && npm run changelog:check:mcp", "test:watch": "vitest", diff --git a/scripts/validate-observability-configs.d.mts b/scripts/validate-observability-configs.d.mts new file mode 100644 index 0000000000..89b17aad3a --- /dev/null +++ b/scripts/validate-observability-configs.d.mts @@ -0,0 +1,3 @@ +export declare function validateDashboards(dir: string): string[]; + +export declare function validateAlertRules(path: string): string[]; diff --git a/scripts/validate-observability-configs.mjs b/scripts/validate-observability-configs.mjs new file mode 100644 index 0000000000..1bfce12bbb --- /dev/null +++ b/scripts/validate-observability-configs.mjs @@ -0,0 +1,89 @@ +#!/usr/bin/env node +// Validate Grafana dashboard JSON and Prometheus alert-rule YAML syntax + basic shape (#1943 deliverable: +// "Add validation for dashboard JSON / alert rule syntax"). Catches a broken JSON/YAML file or a +// structurally malformed dashboard/rule before it silently fails to load in the running stack -- Grafana +// and Prometheus both fail OPEN on a malformed file (skip it, log a warning), so nothing else would catch +// this until an operator notices a panel or alert is simply missing. +import { readFileSync, readdirSync } from "node:fs"; +import { pathToFileURL } from "node:url"; +import { parse as parseYaml } from "yaml"; + +export function validateDashboards(dir) { + const errors = []; + let files; + try { + files = readdirSync(dir).filter((f) => f.endsWith(".json")); + } catch (error) { + return [`${dir}: could not read directory — ${error.message}`]; + } + if (files.length === 0) errors.push(`${dir}: no dashboard JSON files found`); + for (const file of files) { + const path = `${dir}/${file}`; + let dashboard; + try { + dashboard = JSON.parse(readFileSync(path, "utf8")); + } catch (error) { + errors.push(`${path}: invalid JSON — ${error.message}`); + continue; + } + if (typeof dashboard.title !== "string" || !dashboard.title) { + errors.push(`${path}: missing a non-empty top-level "title"`); + } + if (!Array.isArray(dashboard.panels)) { + errors.push(`${path}: missing a top-level "panels" array`); + } + } + return errors; +} + +export function validateAlertRules(path) { + const errors = []; + let doc; + try { + doc = parseYaml(readFileSync(path, "utf8")); + } catch (error) { + return [`${path}: invalid YAML — ${error.message}`]; + } + if (!Array.isArray(doc?.groups)) { + return [`${path}: missing a top-level "groups" array`]; + } + for (const group of doc.groups) { + if (typeof group.name !== "string" || !group.name) { + errors.push(`${path}: a group is missing a non-empty "name"`); + } + if (!Array.isArray(group.rules)) { + errors.push(`${path}: group "${group.name ?? "?"}" is missing a "rules" array`); + continue; + } + for (const rule of group.rules) { + const label = rule.alert ?? "(unnamed rule)"; + if (typeof rule.alert !== "string" || !rule.alert) { + errors.push(`${path}: a rule in group "${group.name}" is missing "alert"`); + } + if (typeof rule.expr !== "string" || !rule.expr) { + errors.push(`${path}: rule "${label}" is missing a non-empty "expr"`); + } + if (typeof rule.labels?.severity !== "string" || !rule.labels.severity) { + errors.push(`${path}: rule "${label}" is missing "labels.severity"`); + } + if (typeof rule.annotations?.summary !== "string" || !rule.annotations.summary) { + errors.push(`${path}: rule "${label}" is missing "annotations.summary"`); + } + } + } + return errors; +} + +/* v8 ignore start -- CLI entrypoint; the exported functions above carry the tested logic. */ +function main() { + const errors = [...validateDashboards("grafana/dashboards"), ...validateAlertRules("prometheus/rules/alerts.yml")]; + if (errors.length > 0) { + console.error(`validate-observability-configs: ${errors.length} problem(s) found:`); + for (const e of errors) console.error(` - ${e}`); + process.exit(1); + } + console.log("validate-observability-configs: dashboards and alert rules are valid"); +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) main(); +/* v8 ignore stop */ diff --git a/test/unit/validate-observability-configs-script.test.ts b/test/unit/validate-observability-configs-script.test.ts new file mode 100644 index 0000000000..1030530c2b --- /dev/null +++ b/test/unit/validate-observability-configs-script.test.ts @@ -0,0 +1,116 @@ +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { validateAlertRules, validateDashboards } from "../../scripts/validate-observability-configs.mjs"; + +function tmpDashboardDir(files: Record): string { + const dir = mkdtempSync(join(tmpdir(), "gt-dash-")); + for (const [name, content] of Object.entries(files)) { + writeFileSync(join(dir, name), content); + } + return dir; +} + +function tmpAlertFile(content: string): string { + const dir = mkdtempSync(join(tmpdir(), "gt-alerts-")); + const path = join(dir, "alerts.yml"); + writeFileSync(path, content); + return path; +} + +describe("validate-observability-configs (#1943)", () => { + it("passes the real dashboards and alert rules shipped in the repo", () => { + expect(validateDashboards("grafana/dashboards")).toEqual([]); + expect(validateAlertRules("prometheus/rules/alerts.yml")).toEqual([]); + }); + + describe("validateDashboards", () => { + it("flags invalid JSON", () => { + const dir = tmpDashboardDir({ "broken.json": "{ not json" }); + const errors = validateDashboards(dir); + expect(errors.some((e) => e.includes("invalid JSON"))).toBe(true); + }); + + it("flags a dashboard missing title or panels", () => { + const dir = tmpDashboardDir({ "incomplete.json": JSON.stringify({}) }); + const errors = validateDashboards(dir); + expect(errors.some((e) => e.includes('missing a non-empty top-level "title"'))).toBe(true); + expect(errors.some((e) => e.includes('missing a top-level "panels" array'))).toBe(true); + }); + + it("flags an empty dashboards directory", () => { + const dir = mkdtempSync(join(tmpdir(), "gt-dash-empty-")); + const errors = validateDashboards(dir); + expect(errors).toEqual([`${dir}: no dashboard JSON files found`]); + }); + + it("passes a well-formed dashboard", () => { + const dir = tmpDashboardDir({ + "ok.json": JSON.stringify({ title: "OK Dashboard", panels: [] }), + }); + expect(validateDashboards(dir)).toEqual([]); + }); + + it("reports the directory itself when unreadable", () => { + const errors = validateDashboards(join(tmpdir(), "gt-does-not-exist-" + Math.random())); + expect(errors.length).toBe(1); + expect(errors[0]).toContain("could not read directory"); + }); + }); + + describe("validateAlertRules", () => { + it("flags invalid YAML", () => { + const path = tmpAlertFile("groups:\n - name: [unterminated"); + const errors = validateAlertRules(path); + expect(errors.some((e) => e.includes("invalid YAML"))).toBe(true); + }); + + it("flags a missing top-level groups array", () => { + const path = tmpAlertFile("not_groups: []"); + expect(validateAlertRules(path)).toEqual([`${path}: missing a top-level "groups" array`]); + }); + + it("flags a group missing name or rules", () => { + const path = tmpAlertFile("groups:\n - {}\n"); + const errors = validateAlertRules(path); + expect(errors.some((e) => e.includes('missing a non-empty "name"'))).toBe(true); + expect(errors.some((e) => e.includes('missing a "rules" array'))).toBe(true); + }); + + it("flags a rule missing alert, expr, severity, or summary", () => { + const path = tmpAlertFile( + ["groups:", " - name: test-group", " rules:", " - expr: up == 0"].join("\n"), + ); + const errors = validateAlertRules(path); + expect(errors.some((e) => e.includes('missing "alert"'))).toBe(true); + expect(errors.some((e) => e.includes('missing "labels.severity"'))).toBe(true); + expect(errors.some((e) => e.includes('missing "annotations.summary"'))).toBe(true); + }); + + it("passes a well-formed rule", () => { + const path = tmpAlertFile( + [ + "groups:", + " - name: test-group", + " rules:", + " - alert: TestAlert", + " expr: up == 0", + " for: 5m", + " labels:", + " severity: warning", + " annotations:", + " summary: test", + ].join("\n"), + ); + expect(validateAlertRules(path)).toEqual([]); + }); + + it("reports the file itself when unreadable", () => { + const path = join(tmpdir(), "gt-does-not-exist-" + Math.random() + ".yml"); + const errors = validateAlertRules(path); + expect(errors.length).toBe(1); + expect(errors[0]).toContain("invalid YAML"); + }); + }); +}); From 9de3a7ec6bcf02ae857cbc602b8faeae1695dbcf Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:36:31 -0700 Subject: [PATCH 2/4] fix(selfhost): guard against non-object dashboard/group/rule values crashing the validator Gate review caught a real bug: valid JSON/YAML that parses to a non-object (a dashboard file containing literally "null", or a YAML sequence entry like "- null") crashed the validator with a TypeError instead of producing the structured validation error it exists to provide -- the one failure mode it's explicitly meant to catch cleanly. Added an isObject() guard before every dereference (dashboard, group, rule) and 4 new regression tests covering null/array/string/primitive-at-each-level. --- scripts/validate-observability-configs.mjs | 25 +++++++++++++++++-- ...idate-observability-configs-script.test.ts | 21 ++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/scripts/validate-observability-configs.mjs b/scripts/validate-observability-configs.mjs index 1bfce12bbb..1ba1d506aa 100644 --- a/scripts/validate-observability-configs.mjs +++ b/scripts/validate-observability-configs.mjs @@ -8,6 +8,13 @@ import { readFileSync, readdirSync } from "node:fs"; import { pathToFileURL } from "node:url"; import { parse as parseYaml } from "yaml"; +// Valid JSON/YAML can parse to a non-object (null, a string, a number, an array of non-objects) -- +// dereferencing a property on that crashes instead of producing a validation error. Every dereference +// below goes through this guard first. +function isObject(value) { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + export function validateDashboards(dir) { const errors = []; let files; @@ -26,6 +33,10 @@ export function validateDashboards(dir) { errors.push(`${path}: invalid JSON — ${error.message}`); continue; } + if (!isObject(dashboard)) { + errors.push(`${path}: top level must be a JSON object, not ${JSON.stringify(dashboard)}`); + continue; + } if (typeof dashboard.title !== "string" || !dashboard.title) { errors.push(`${path}: missing a non-empty top-level "title"`); } @@ -47,7 +58,11 @@ export function validateAlertRules(path) { if (!Array.isArray(doc?.groups)) { return [`${path}: missing a top-level "groups" array`]; } - for (const group of doc.groups) { + for (const [groupIndex, group] of doc.groups.entries()) { + if (!isObject(group)) { + errors.push(`${path}: groups[${groupIndex}] must be an object, not ${JSON.stringify(group)}`); + continue; + } if (typeof group.name !== "string" || !group.name) { errors.push(`${path}: a group is missing a non-empty "name"`); } @@ -55,7 +70,13 @@ export function validateAlertRules(path) { errors.push(`${path}: group "${group.name ?? "?"}" is missing a "rules" array`); continue; } - for (const rule of group.rules) { + for (const [ruleIndex, rule] of group.rules.entries()) { + if (!isObject(rule)) { + errors.push( + `${path}: group "${group.name}" rules[${ruleIndex}] must be an object, not ${JSON.stringify(rule)}`, + ); + continue; + } const label = rule.alert ?? "(unnamed rule)"; if (typeof rule.alert !== "string" || !rule.alert) { errors.push(`${path}: a rule in group "${group.name}" is missing "alert"`); diff --git a/test/unit/validate-observability-configs-script.test.ts b/test/unit/validate-observability-configs-script.test.ts index 1030530c2b..ddae8e5923 100644 --- a/test/unit/validate-observability-configs-script.test.ts +++ b/test/unit/validate-observability-configs-script.test.ts @@ -57,6 +57,14 @@ describe("validate-observability-configs (#1943)", () => { expect(errors.length).toBe(1); expect(errors[0]).toContain("could not read directory"); }); + + it("reports a validation error (not a crash) when a dashboard file is valid JSON but not an object", () => { + const dir = tmpDashboardDir({ "null.json": "null", "array.json": "[]", "string.json": '"hi"' }); + const errors = validateDashboards(dir); + expect(errors.some((e) => e.includes("null.json") && e.includes("must be a JSON object"))).toBe(true); + expect(errors.some((e) => e.includes("array.json") && e.includes("must be a JSON object"))).toBe(true); + expect(errors.some((e) => e.includes("string.json") && e.includes("must be a JSON object"))).toBe(true); + }); }); describe("validateAlertRules", () => { @@ -112,5 +120,18 @@ describe("validate-observability-configs (#1943)", () => { expect(errors.length).toBe(1); expect(errors[0]).toContain("invalid YAML"); }); + + it("reports a validation error (not a crash) when a group entry is not an object", () => { + const path = tmpAlertFile("groups:\n - null\n - 5\n - just-a-string\n"); + const errors = validateAlertRules(path); + expect(errors.length).toBe(3); + for (const e of errors) expect(e).toContain("must be an object"); + }); + + it("reports a validation error (not a crash) when a rule entry is not an object", () => { + const path = tmpAlertFile(["groups:", " - name: test-group", " rules:", " - null"].join("\n")); + const errors = validateAlertRules(path); + expect(errors.some((e) => e.includes("rules[0]") && e.includes("must be an object"))).toBe(true); + }); }); }); From 5b6a5297fc4e4e71f263c780745e6afb5acfbfb9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:46:29 -0700 Subject: [PATCH 3/4] fix(selfhost): add a lightweight PromQL sanity check, drop unused test import Gate review correctly noted the validator claimed to check "alert rule syntax" but only verified expr was a non-empty string -- malformed PromQL like "up ==" passed silently. Added a lightweight (not a real parser -- no promtool/PromQL-grammar dependency, out of scope for this "if available" deliverable) sanity check: balanced brackets and no dangling trailing binary operator. Verified zero false positives against all 21 real rules in prometheus/rules/alerts.yml. Also dropped an unused mkdirSync import the gate flagged as a nit. --- scripts/validate-observability-configs.mjs | 33 ++++++++++++++ ...idate-observability-configs-script.test.ts | 44 ++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/scripts/validate-observability-configs.mjs b/scripts/validate-observability-configs.mjs index 1ba1d506aa..30361ff356 100644 --- a/scripts/validate-observability-configs.mjs +++ b/scripts/validate-observability-configs.mjs @@ -4,6 +4,11 @@ // structurally malformed dashboard/rule before it silently fails to load in the running stack -- Grafana // and Prometheus both fail OPEN on a malformed file (skip it, log a warning), so nothing else would catch // this until an operator notices a panel or alert is simply missing. +// +// The alert-rule `expr` check is a lightweight sanity check (balanced brackets, no dangling binary +// operator), NOT a real PromQL parser -- this repo has no promtool/PromQL-grammar dependency, and adding +// one is out of scope for this "if available" deliverable. It catches the obvious copy-paste/typo class +// of mistake; it does not validate PromQL semantics (unknown functions, wrong label matchers, etc.). import { readFileSync, readdirSync } from "node:fs"; import { pathToFileURL } from "node:url"; import { parse as parseYaml } from "yaml"; @@ -15,6 +20,31 @@ function isObject(value) { return typeof value === "object" && value !== null && !Array.isArray(value); } +const BINARY_OPERATORS = ["==", "!=", ">=", "<=", ">", "<", "+", "-", "*", "/", "%", "^", "and", "or", "unless"]; + +// A deliberately lightweight PromQL sanity check -- NOT a real parser (no promtool dependency; see the +// module doc comment). Catches the class of mistake a copy-paste/typo produces: unbalanced brackets, or +// an expression left dangling on a binary operator with no right-hand side (e.g. "up =="). +function promqlSanityIssue(expr) { + const trimmed = expr.trim(); + let depth = 0; + const pairs = { ")": "(", "]": "[", "}": "{" }; + const opens = new Set(["(", "[", "{"]); + for (const ch of trimmed) { + if (opens.has(ch)) depth++; + else if (ch in pairs) { + depth--; + if (depth < 0) return `unbalanced brackets (unexpected "${ch}")`; + } + } + if (depth !== 0) return "unbalanced brackets"; + const lastToken = trimmed.split(/\s+/).pop() ?? ""; + if (BINARY_OPERATORS.includes(lastToken)) { + return `expression ends in the binary operator "${lastToken}" with no right-hand side`; + } + return null; +} + export function validateDashboards(dir) { const errors = []; let files; @@ -83,6 +113,9 @@ export function validateAlertRules(path) { } if (typeof rule.expr !== "string" || !rule.expr) { errors.push(`${path}: rule "${label}" is missing a non-empty "expr"`); + } else { + const issue = promqlSanityIssue(rule.expr); + if (issue) errors.push(`${path}: rule "${label}" has a suspect "expr" — ${issue}`); } if (typeof rule.labels?.severity !== "string" || !rule.labels.severity) { errors.push(`${path}: rule "${label}" is missing "labels.severity"`); diff --git a/test/unit/validate-observability-configs-script.test.ts b/test/unit/validate-observability-configs-script.test.ts index ddae8e5923..16c0114bc8 100644 --- a/test/unit/validate-observability-configs-script.test.ts +++ b/test/unit/validate-observability-configs-script.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; @@ -133,5 +133,47 @@ describe("validate-observability-configs (#1943)", () => { const errors = validateAlertRules(path); expect(errors.some((e) => e.includes("rules[0]") && e.includes("must be an object"))).toBe(true); }); + + function ruleFile(expr: string): string { + return tmpAlertFile( + [ + "groups:", + " - name: test-group", + " rules:", + " - alert: TestAlert", + ` expr: ${expr}`, + " labels:", + " severity: warning", + " annotations:", + " summary: test", + ].join("\n"), + ); + } + + it("flags an expr with a dangling binary operator", () => { + const errors = validateAlertRules(ruleFile("up ==")); + expect(errors.some((e) => e.includes("dangling") || e.includes('binary operator "=="'))).toBe(true); + }); + + it("flags an expr with unbalanced brackets", () => { + const errors = validateAlertRules(ruleFile("sum(rate(foo[5m])")); + expect(errors.some((e) => e.includes("unbalanced brackets"))).toBe(true); + }); + + it("flags an expr with an unexpected closing bracket", () => { + const errors = validateAlertRules(ruleFile("up)")); + expect(errors.some((e) => e.includes("unbalanced brackets"))).toBe(true); + }); + + it("passes well-formed real-shaped PromQL expressions", () => { + for (const expr of [ + "up == 0", + "sum(rate(gittensory_jobs_total[5m])) by (status) > 10", + 'histogram_quantile(0.95, rate(gittensory_http_duration_seconds_bucket{route="/health"}[5m]))', + "(a + b) / c", + ]) { + expect(validateAlertRules(ruleFile(expr))).toEqual([]); + } + }); }); }); From 0da7a3ec18cc8da2ed820d0e05170de6ef5c2658 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:54:51 -0700 Subject: [PATCH 4/4] fix(selfhost): use a bracket stack, not a depth counter, in the PromQL sanity check Gate review caught a real bug: a plain depth counter only checks net nesting count, so "sum(foo[5m))" -- opened with "(" and "[", closed with ")" and ")" -- nets to depth 0 and wrongly passes as balanced, even though the second ")" doesn't match the "[" it's supposed to close. Switched to a stack that checks each closer against the delimiter it actually needs to match. Verified against the real alerts.yml (still zero false positives) and added a regression test for the exact mismatched-type case the gate identified. --- scripts/validate-observability-configs.mjs | 18 ++++++++++++------ ...lidate-observability-configs-script.test.ts | 9 +++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/scripts/validate-observability-configs.mjs b/scripts/validate-observability-configs.mjs index 30361ff356..a3d57c13b7 100644 --- a/scripts/validate-observability-configs.mjs +++ b/scripts/validate-observability-configs.mjs @@ -27,17 +27,23 @@ const BINARY_OPERATORS = ["==", "!=", ">=", "<=", ">", "<", "+", "-", "*", "/", // an expression left dangling on a binary operator with no right-hand side (e.g. "up =="). function promqlSanityIssue(expr) { const trimmed = expr.trim(); - let depth = 0; + // A stack, not a depth counter: a depth counter only checks NESTING COUNT, so "sum(foo[5m))" -- opened + // with "(" and "[", closed with ")" and ")" -- reaches depth 0 and would wrongly look balanced. The + // stack checks each closer against the delimiter it's actually supposed to match. const pairs = { ")": "(", "]": "[", "}": "{" }; const opens = new Set(["(", "[", "{"]); + const stack = []; for (const ch of trimmed) { - if (opens.has(ch)) depth++; - else if (ch in pairs) { - depth--; - if (depth < 0) return `unbalanced brackets (unexpected "${ch}")`; + if (opens.has(ch)) { + stack.push(ch); + } else if (ch in pairs) { + const top = stack.pop(); + if (top !== pairs[ch]) { + return `unbalanced brackets (unexpected "${ch}"${top ? `, expected the match for "${top}"` : ""})`; + } } } - if (depth !== 0) return "unbalanced brackets"; + if (stack.length > 0) return `unbalanced brackets (unclosed "${stack[stack.length - 1]}")`; const lastToken = trimmed.split(/\s+/).pop() ?? ""; if (BINARY_OPERATORS.includes(lastToken)) { return `expression ends in the binary operator "${lastToken}" with no right-hand side`; diff --git a/test/unit/validate-observability-configs-script.test.ts b/test/unit/validate-observability-configs-script.test.ts index 16c0114bc8..9fa5410a50 100644 --- a/test/unit/validate-observability-configs-script.test.ts +++ b/test/unit/validate-observability-configs-script.test.ts @@ -165,6 +165,15 @@ describe("validate-observability-configs (#1943)", () => { expect(errors.some((e) => e.includes("unbalanced brackets"))).toBe(true); }); + it("flags mismatched bracket TYPES even when nesting depth balances out (REGRESSION)", () => { + // sum(foo[5m)) -- opened with "(" and "[", closed with ")" and ")". A naive depth counter (net + // opens - closes = 0) wrongly calls this balanced; the closer must match its actual opener. + const errors = validateAlertRules(ruleFile("sum(foo[5m))")); + expect(errors.some((e) => e.includes("unbalanced brackets") && e.includes('expected the match for "["'))).toBe( + true, + ); + }); + it("passes well-formed real-shaped PromQL expressions", () => { for (const expr of [ "up == 0",