diff --git a/packages/gittensory-engine/package.json b/packages/gittensory-engine/package.json index 9e6b30ef25..1505664f7e 100644 --- a/packages/gittensory-engine/package.json +++ b/packages/gittensory-engine/package.json @@ -31,10 +31,6 @@ "types": "./dist/index.d.ts", "default": "./dist/index.js" }, - "./miner-goal-spec-parse": { - "types": "./dist/miner-goal-spec-parse.d.ts", - "default": "./dist/miner-goal-spec-parse.js" - }, "./scoring/model": { "types": "./dist/scoring/model.d.ts", "default": "./dist/scoring/model.js" diff --git a/packages/gittensory-engine/src/miner-goal-spec-parse.ts b/packages/gittensory-engine/src/miner-goal-spec-parse.ts deleted file mode 100644 index 20ac143ebe..0000000000 --- a/packages/gittensory-engine/src/miner-goal-spec-parse.ts +++ /dev/null @@ -1,110 +0,0 @@ -import { - DEFAULT_MINER_GOAL_SPEC, - type MinerGoalSpec, - type MinerIssueDiscoveryPolicy, -} from "./miner-goal-spec.js"; - -export type MinerGoalSpecParseResult = { - present: boolean; - spec: Readonly; - warnings: readonly string[]; -}; - -const MAX_LIST_ENTRIES = 200; -const MAX_STRING_LEN = 300; -const POLICIES = new Set(["encouraged", "neutral", "discouraged"]); - -function freezeSpec(spec: MinerGoalSpec): Readonly { - return Object.freeze({ - ...spec, - wantedPaths: Object.freeze([...spec.wantedPaths]), - blockedPaths: Object.freeze([...spec.blockedPaths]), - preferredLabels: Object.freeze([...spec.preferredLabels]), - blockedLabels: Object.freeze([...spec.blockedLabels]), - }); -} - -function parseStringList(value: unknown, field: string, warnings: string[]): string[] { - if (value === undefined) return []; - if (!Array.isArray(value)) { - warnings.push(`MinerGoalSpec field "${field}" must be an array; ignoring.`); - return []; - } - const seen = new Set(); - const out: string[] = []; - for (const [index, entry] of value.entries()) { - if (index >= MAX_LIST_ENTRIES) { - warnings.push(`MinerGoalSpec field "${field}" is capped at ${MAX_LIST_ENTRIES} entries; dropping the rest.`); - break; - } - if (typeof entry !== "string") { - warnings.push(`MinerGoalSpec field "${field}" entries must be strings; skipping non-string.`); - continue; - } - const trimmed = entry.trim(); - if (!trimmed || trimmed.length > MAX_STRING_LEN) continue; - if (seen.has(trimmed)) continue; - seen.add(trimmed); - out.push(trimmed); - if (out.length >= MAX_LIST_ENTRIES) break; - } - return out; -} - -function parseBoolean(value: unknown, field: string, fallback: boolean, warnings: string[]): boolean { - if (value === undefined) return fallback; - if (typeof value === "boolean") return value; - warnings.push(`MinerGoalSpec field "${field}" must be a boolean; using default.`); - return fallback; -} - -function parseClaims(value: unknown, warnings: string[]): number { - if (value === undefined) return DEFAULT_MINER_GOAL_SPEC.maxConcurrentClaims; - if (typeof value !== "number" || !Number.isFinite(value)) { - warnings.push(`MinerGoalSpec field "maxConcurrentClaims" must be a number; using default.`); - return DEFAULT_MINER_GOAL_SPEC.maxConcurrentClaims; - } - const floored = Math.floor(value); - if (floored < 1) { - warnings.push(`MinerGoalSpec field "maxConcurrentClaims" must be >= 1; using 1.`); - return 1; - } - return floored; -} - -function parsePolicy(value: unknown, warnings: string[]): MinerIssueDiscoveryPolicy { - if (value === undefined) return DEFAULT_MINER_GOAL_SPEC.issueDiscoveryPolicy; - if (typeof value !== "string") { - warnings.push(`MinerGoalSpec field "issueDiscoveryPolicy" must be a string; using neutral.`); - return "neutral"; - } - const normalized = value.trim().toLowerCase(); - if (POLICIES.has(normalized as MinerIssueDiscoveryPolicy)) return normalized as MinerIssueDiscoveryPolicy; - warnings.push( - `MinerGoalSpec field "issueDiscoveryPolicy" must be encouraged, neutral, or discouraged; using neutral.`, - ); - return "neutral"; -} - -/** Parse raw JSON/YAML-decoded config into a deep-frozen {@link MinerGoalSpec}. Pure — no IO. */ -export function parseMinerGoalSpec(raw: unknown): MinerGoalSpecParseResult { - if (raw === null || raw === undefined || typeof raw !== "object" || Array.isArray(raw)) { - return { present: false, spec: DEFAULT_MINER_GOAL_SPEC, warnings: [] }; - } - - const record = raw as Record; - const warnings: string[] = []; - const present = Object.keys(record).length > 0; - - const spec = freezeSpec({ - minerEnabled: parseBoolean(record.minerEnabled, "minerEnabled", DEFAULT_MINER_GOAL_SPEC.minerEnabled, warnings), - wantedPaths: parseStringList(record.wantedPaths, "wantedPaths", warnings), - blockedPaths: parseStringList(record.blockedPaths, "blockedPaths", warnings), - preferredLabels: parseStringList(record.preferredLabels, "preferredLabels", warnings), - blockedLabels: parseStringList(record.blockedLabels, "blockedLabels", warnings), - maxConcurrentClaims: parseClaims(record.maxConcurrentClaims, warnings), - issueDiscoveryPolicy: parsePolicy(record.issueDiscoveryPolicy, warnings), - }); - - return { present, spec, warnings: Object.freeze(warnings) }; -} diff --git a/packages/gittensory-engine/src/miner-goal-spec.ts b/packages/gittensory-engine/src/miner-goal-spec.ts index 34e8b58ca8..c76477dc6a 100644 --- a/packages/gittensory-engine/src/miner-goal-spec.ts +++ b/packages/gittensory-engine/src/miner-goal-spec.ts @@ -105,7 +105,11 @@ function normalizeStringList(value: unknown, field: string, warnings: string[]): } const result: string[] = []; const seen = new Set(); - for (const entry of value) { + for (const [index, entry] of value.entries()) { + if (index >= MAX_LIST_ITEMS) { + warnings.push(`MinerGoalSpec field "${field}" exceeded ${MAX_LIST_ITEMS} entries; extra entries ignored.`); + break; + } if (typeof entry !== "string") { warnings.push(`MinerGoalSpec field "${field}" skipped a non-string entry.`); continue; @@ -118,10 +122,6 @@ function normalizeStringList(value: unknown, field: string, warnings: string[]): normalized = normalized.slice(0, MAX_ITEM_LENGTH); } if (seen.has(normalized)) continue; - if (result.length >= MAX_LIST_ITEMS) { - warnings.push(`MinerGoalSpec field "${field}" exceeded ${MAX_LIST_ITEMS} entries; extra entries ignored.`); - break; - } result.push(normalized); seen.add(normalized); } diff --git a/packages/gittensory-engine/test/miner-goal-spec-discovery.test.ts b/packages/gittensory-engine/test/miner-goal-spec-discovery.test.ts index 5890c7d8f4..0d4bd583ff 100644 --- a/packages/gittensory-engine/test/miner-goal-spec-discovery.test.ts +++ b/packages/gittensory-engine/test/miner-goal-spec-discovery.test.ts @@ -1,5 +1,5 @@ // Tests for MinerGoalSpec file discovery (#2294). The tolerant parser itself is covered by -// miner-goal-spec-parse.test.ts / miner-goal-spec-parser.test.ts; this covers only the discovery order. Pure — +// miner-goal-spec-parser.test.ts; this covers only the discovery order. Pure — // the existence check is injected, so no filesystem is touched. Runs against compiled dist/. import { test } from "node:test"; import assert from "node:assert/strict"; diff --git a/packages/gittensory-engine/test/miner-goal-spec-parse.test.ts b/packages/gittensory-engine/test/miner-goal-spec-parse.test.ts deleted file mode 100644 index a65e9524d3..0000000000 --- a/packages/gittensory-engine/test/miner-goal-spec-parse.test.ts +++ /dev/null @@ -1,101 +0,0 @@ -import { test } from "node:test"; -import assert from "node:assert/strict"; - -import { DEFAULT_MINER_GOAL_SPEC } from "../dist/miner-goal-spec.js"; -import { parseMinerGoalSpec } from "../dist/miner-goal-spec-parse.js"; - -test("parseMinerGoalSpec returns defaults for missing or non-object input", () => { - for (const raw of [undefined, null, "nope", 42, []]) { - const result = parseMinerGoalSpec(raw); - assert.equal(result.present, false); - assert.equal(result.spec, DEFAULT_MINER_GOAL_SPEC); - assert.deepEqual(result.warnings, []); - } -}); - -test("parseMinerGoalSpec coerces a full valid object and marks present", () => { - const result = parseMinerGoalSpec({ - minerEnabled: false, - wantedPaths: [" src/** ", "src/**"], - blockedPaths: ["dist/**"], - preferredLabels: [" bug ", "feature"], - blockedLabels: [" wontfix ", "duplicate"], - maxConcurrentClaims: 3, - issueDiscoveryPolicy: "ENCOURAGED", - }); - - assert.equal(result.present, true); - assert.deepEqual(result.spec, { - minerEnabled: false, - wantedPaths: ["src/**"], - blockedPaths: ["dist/**"], - preferredLabels: ["bug", "feature"], - blockedLabels: ["wontfix", "duplicate"], - maxConcurrentClaims: 3, - issueDiscoveryPolicy: "encouraged", - }); - assert.deepEqual(result.warnings, []); - assert.ok(Object.isFrozen(result.spec)); - assert.ok(Object.isFrozen(result.spec.wantedPaths)); -}); - -test("parseMinerGoalSpec floors claims and rejects values below 1", () => { - const floored = parseMinerGoalSpec({ maxConcurrentClaims: 2.9 }); - assert.equal(floored.spec.maxConcurrentClaims, 2); - - const zero = parseMinerGoalSpec({ maxConcurrentClaims: 0 }); - assert.equal(zero.spec.maxConcurrentClaims, 1); - assert.match(zero.warnings.join(" "), /maxConcurrentClaims/); -}); - -test("parseMinerGoalSpec warns and falls back on malformed fields", () => { - const result = parseMinerGoalSpec({ - minerEnabled: "yes", - wantedPaths: "src/**", - maxConcurrentClaims: "two", - issueDiscoveryPolicy: "aggressive", - }); - - assert.equal(result.present, true); - assert.equal(result.spec.minerEnabled, DEFAULT_MINER_GOAL_SPEC.minerEnabled); - assert.deepEqual(result.spec.wantedPaths, []); - assert.equal(result.spec.maxConcurrentClaims, DEFAULT_MINER_GOAL_SPEC.maxConcurrentClaims); - assert.equal(result.spec.issueDiscoveryPolicy, "neutral"); - assert.ok(result.warnings.length >= 4); -}); - -test("parseMinerGoalSpec caps list inspection for invalid entries", () => { - const result = parseMinerGoalSpec({ - wantedPaths: Array.from({ length: 1_000 }, () => null), - }); - - assert.deepEqual(result.spec.wantedPaths, []); - assert.equal(result.warnings.length, 201); - assert.match(result.warnings.at(-1) ?? "", /capped at 200 entries/); -}); - -test("parseMinerGoalSpec caps list inspection for duplicate, empty, and overlong entries", () => { - const duplicates = parseMinerGoalSpec({ - wantedPaths: Array.from({ length: 1_000 }, () => "src/**"), - }); - assert.deepEqual(duplicates.spec.wantedPaths, ["src/**"]); - assert.deepEqual(duplicates.warnings, [ - 'MinerGoalSpec field "wantedPaths" is capped at 200 entries; dropping the rest.', - ]); - - const empty = parseMinerGoalSpec({ - wantedPaths: Array.from({ length: 1_000 }, () => " "), - }); - assert.deepEqual(empty.spec.wantedPaths, []); - assert.deepEqual(empty.warnings, [ - 'MinerGoalSpec field "wantedPaths" is capped at 200 entries; dropping the rest.', - ]); - - const overlong = parseMinerGoalSpec({ - wantedPaths: Array.from({ length: 1_000 }, () => "x".repeat(301)), - }); - assert.deepEqual(overlong.spec.wantedPaths, []); - assert.deepEqual(overlong.warnings, [ - 'MinerGoalSpec field "wantedPaths" is capped at 200 entries; dropping the rest.', - ]); -}); diff --git a/packages/gittensory-engine/test/miner-goal-spec-parser.test.ts b/packages/gittensory-engine/test/miner-goal-spec-parser.test.ts index d3b3856383..502ef8b92f 100644 --- a/packages/gittensory-engine/test/miner-goal-spec-parser.test.ts +++ b/packages/gittensory-engine/test/miner-goal-spec-parser.test.ts @@ -58,6 +58,57 @@ test("parseMinerGoalSpec: exactly 100 unique entries are accepted without a cap assert.ok(!parsed.warnings.some((warning) => /exceeded 100 entries/i.test(warning))); }); +test("parseMinerGoalSpec: caps inspection of a hostile all-non-string list instead of scanning it in full", () => { + // Regression: the cap used to be checked only after a candidate was accepted (duplicate-check-adjacent), so + // an array of entries that ALWAYS take the `continue` path (non-string, duplicate, or empty-after-trim) never + // hit the cap and got fully scanned -- unbounded CPU/memory work and unbounded warnings for a hostile input. + // The cap must be checked against the raw index, before any per-entry work. `minerEnabled: false` keeps the + // spec "present" regardless of what wantedPaths ends up as, so these assertions stay focused on the list cap. + const parsed = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => null), + }); + + assert.deepEqual(parsed.spec.wantedPaths, []); + assert.equal(parsed.warnings.length, 101); + assert.match(parsed.warnings.at(-1) ?? "", /exceeded 100 entries/); +}); + +test("parseMinerGoalSpec: caps inspection of a hostile all-duplicate or all-empty list to a single cap warning", () => { + const duplicates = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => "src/**"), + }); + assert.deepEqual(duplicates.spec.wantedPaths, ["src/**"]); + assert.deepEqual(duplicates.warnings, [ + 'MinerGoalSpec field "wantedPaths" exceeded 100 entries; extra entries ignored.', + ]); + + const empty = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => " "), + }); + assert.deepEqual(empty.spec.wantedPaths, []); + assert.deepEqual(empty.warnings, [ + 'MinerGoalSpec field "wantedPaths" exceeded 100 entries; extra entries ignored.', + ]); +}); + +test("parseMinerGoalSpec: caps inspection of a hostile all-overlong list, even though each entry still warns once", () => { + // Different from the duplicate/empty case: an overlong entry is TRUNCATED (with its own warning), not + // silently dropped, so every one of the 100 inspected entries produces a truncation warning before the + // post-truncation duplicate check collapses them into one result entry. Still bounded to exactly + // 100 truncate warnings + 1 cap warning, never proportional to the input's true length. + const parsed = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => "x".repeat(300)), + }); + + assert.deepEqual(parsed.spec.wantedPaths, ["x".repeat(256)]); + assert.equal(parsed.warnings.length, 101); + assert.match(parsed.warnings.at(-1) ?? "", /exceeded 100 entries/); +}); + test("parseMinerGoalSpec: malformed fields fall back independently with targeted warnings", () => { const longEntry = "x".repeat(300); const parsed = parseMinerGoalSpec({ diff --git a/test/unit/miner-goal-spec-parser.test.ts b/test/unit/miner-goal-spec-parser.test.ts index 0acb3c6742..67684ffd93 100644 --- a/test/unit/miner-goal-spec-parser.test.ts +++ b/test/unit/miner-goal-spec-parser.test.ts @@ -78,6 +78,50 @@ describe("MinerGoalSpec parser (#2301)", () => { expect(parsed.warnings.join(" ")).not.toMatch(/exceeded 100 entries/i); }); + it("bounds inspection of a hostile all-non-string list instead of scanning it in full", () => { + // Regression: the cap used to be checked only after a candidate was accepted (duplicate-check-adjacent), + // so an array of entries that ALWAYS take the `continue` path (non-string, duplicate, or empty-after-trim) + // never hit the cap and got fully scanned -- unbounded CPU/memory work and unbounded warnings for a hostile + // input. The cap must now be checked against the raw index, before any per-entry work. `minerEnabled: false` + // keeps the spec "present" regardless of what wantedPaths ends up as, so these assertions stay focused on + // the list cap rather than incidentally exercising the separate all-fields-default fallback. + const wantedPaths = Array.from({ length: 1_000 }, () => null); + const parsed = parseMinerGoalSpec({ minerEnabled: false, wantedPaths }); + + expect(parsed.spec.wantedPaths).toEqual([]); + expect(parsed.warnings).toHaveLength(101); + expect(parsed.warnings.at(-1)).toMatch(/exceeded 100 entries/); + }); + + it("bounds inspection of a hostile all-duplicate or all-empty list to a single cap warning", () => { + const duplicates = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => "src/**"), + }); + expect(duplicates.spec.wantedPaths).toEqual(["src/**"]); + expect(duplicates.warnings).toEqual(['MinerGoalSpec field "wantedPaths" exceeded 100 entries; extra entries ignored.']); + + const empty = parseMinerGoalSpec({ + minerEnabled: false, + wantedPaths: Array.from({ length: 1_000 }, () => " "), + }); + expect(empty.spec.wantedPaths).toEqual([]); + expect(empty.warnings).toEqual(['MinerGoalSpec field "wantedPaths" exceeded 100 entries; extra entries ignored.']); + }); + + it("bounds inspection of a hostile all-overlong list, even though each inspected entry still warns once", () => { + // Different from the duplicate/empty case: an overlong entry is TRUNCATED (with its own warning), not + // silently dropped, so every one of the 100 inspected entries produces a truncation warning before the + // post-truncation duplicate check collapses them into one result entry. Still bounded to exactly + // 100 truncate warnings + 1 cap warning, never proportional to the input's true length. + const wantedPaths = Array.from({ length: 1_000 }, () => "x".repeat(300)); + const parsed = parseMinerGoalSpec({ minerEnabled: false, wantedPaths }); + + expect(parsed.spec.wantedPaths).toEqual(["x".repeat(256)]); + expect(parsed.warnings).toHaveLength(101); + expect(parsed.warnings.at(-1)).toMatch(/exceeded 100 entries/); + }); + it("falls back per field for invalid values without throwing", () => { const parsed = parseMinerGoalSpec({ minerEnabled: "yes",