From db3d45d8c0ac34ff12decd9865116769338d5e48 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:31:54 +0000 Subject: [PATCH] fix(miner): wire AMS policy resolver to the documented discovery order AMS_POLICY_SPEC_FILENAMES documented a 4-path first-match-wins discovery order "mirroring MINER_GOAL_SPEC_FILENAMES", but ams-policy.ts resolved policy from a single operator-config-dir filename (.loopover-ams.yml), never imported the constant, and had no fallback chain -- an operator using the .github/ or .json variants silently got defaults. Wire it up (issue option b): add a pure discoverAmsPolicySpecPath helper to the engine mirroring discoverMinerGoalSpecPath, and have the miner's readLocalAmsPolicyContent probe the full order inside the operator config directory. An explicit LOOPOVER_MINER_AMS_POLICY_PATH still bypasses discovery to one exact file. The config-dir resolution is factored into a shared resolveLocalStoreConfigDir so all LOOPOVER_MINER_CONFIG_DIR reads stay in local-store.ts and the generated env-reference is unchanged. Closes #8863 --- .../loopover-engine/src/ams-policy-spec.ts | 10 +++ packages/loopover-engine/src/index.ts | 1 + .../test/ams-policy-spec-discovery.test.ts | 47 ++++++++++++ packages/loopover-miner/lib/ams-policy.ts | 52 ++++++++++--- packages/loopover-miner/lib/local-store.ts | 13 +++- test/unit/miner-ams-policy.test.ts | 75 ++++++++++++++++++- 6 files changed, 181 insertions(+), 17 deletions(-) create mode 100644 packages/loopover-engine/test/ams-policy-spec-discovery.test.ts diff --git a/packages/loopover-engine/src/ams-policy-spec.ts b/packages/loopover-engine/src/ams-policy-spec.ts index f437c28565..0bd5b27062 100644 --- a/packages/loopover-engine/src/ams-policy-spec.ts +++ b/packages/loopover-engine/src/ams-policy-spec.ts @@ -410,3 +410,13 @@ export function parseAmsPolicySpecContent(content: string | null | undefined): P /** The documented `.loopover-ams` file-discovery order (first match wins), mirroring `MINER_GOAL_SPEC_FILENAMES`. */ export const AMS_POLICY_SPEC_FILENAMES = [".loopover-ams.yml", ".github/loopover-ams.yml", ".loopover-ams.json", ".github/loopover-ams.json"] as const; + +/** + * The first {@link AMS_POLICY_SPEC_FILENAMES} candidate that exists, or null. Pure: the caller injects the existence + * check (e.g. `fs.existsSync`) so this module stays IO-free and unit-testable — mirroring `discoverMinerGoalSpecPath`. + * A caller reads the returned path and feeds its content to {@link parseAmsPolicySpecContent}. + */ +export function discoverAmsPolicySpecPath(exists: (path: string) => boolean): string | null { + for (const name of AMS_POLICY_SPEC_FILENAMES) if (exists(name)) return name; + return null; +} diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 29272ae686..943cef9f60 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -559,6 +559,7 @@ export { DEFAULT_AMS_POLICY_SPEC, parseAmsPolicySpec, parseAmsPolicySpecContent, + discoverAmsPolicySpecPath, AMS_POLICY_SPEC_FILENAMES, AMS_NETWORK_ALLOWLIST_ECOSYSTEMS, type AmsCapLimits, diff --git a/packages/loopover-engine/test/ams-policy-spec-discovery.test.ts b/packages/loopover-engine/test/ams-policy-spec-discovery.test.ts new file mode 100644 index 0000000000..154cb0018b --- /dev/null +++ b/packages/loopover-engine/test/ams-policy-spec-discovery.test.ts @@ -0,0 +1,47 @@ +// Tests for AmsPolicySpec file discovery (#8863). The tolerant parser itself is covered by +// ams-policy-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"; +import { discoverAmsPolicySpecPath, AMS_POLICY_SPEC_FILENAMES } from "../dist/index.js"; + +test("AMS_POLICY_SPEC_FILENAMES lists the documented discovery order", () => { + assert.deepEqual([...AMS_POLICY_SPEC_FILENAMES], [ + ".loopover-ams.yml", + ".github/loopover-ams.yml", + ".loopover-ams.json", + ".github/loopover-ams.json", + ]); +}); + +test("discoverAmsPolicySpecPath: returns the first existing candidate, first match wins", () => { + assert.equal(discoverAmsPolicySpecPath(() => true), ".loopover-ams.yml"); + // repo-root yml missing but the .github yml present → that one is chosen + assert.equal( + discoverAmsPolicySpecPath((p) => p !== ".loopover-ams.yml"), + ".github/loopover-ams.yml", + ); + // only a JSON variant present + assert.equal(discoverAmsPolicySpecPath((p) => p === ".loopover-ams.json"), ".loopover-ams.json"); +}); + +test("discoverAmsPolicySpecPath: short-circuits — stops probing once a candidate matches", () => { + const probed: string[] = []; + const result = discoverAmsPolicySpecPath((p) => { + probed.push(p); + return p === ".github/loopover-ams.yml"; // the 2nd candidate matches + }); + assert.equal(result, ".github/loopover-ams.yml"); + // only the first two candidates are probed; the later .json variants are never reached + assert.deepEqual(probed, [".loopover-ams.yml", ".github/loopover-ams.yml"]); +}); + +test("discoverAmsPolicySpecPath: returns null when no candidate exists, and never probes unlisted paths", () => { + const probed: string[] = []; + const result = discoverAmsPolicySpecPath((p) => { + probed.push(p); + return false; + }); + assert.equal(result, null); + assert.deepEqual(probed, [...AMS_POLICY_SPEC_FILENAMES]); // exactly the listed candidates, in order +}); diff --git a/packages/loopover-miner/lib/ams-policy.ts b/packages/loopover-miner/lib/ams-policy.ts index 0a115662b5..896d45ec00 100644 --- a/packages/loopover-miner/lib/ams-policy.ts +++ b/packages/loopover-miner/lib/ams-policy.ts @@ -1,17 +1,20 @@ import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; import type { AmsPolicySpec } from "@loopover/engine"; -import { DEFAULT_AMS_POLICY_SPEC, parseAmsPolicySpecContent } from "@loopover/engine"; -import { resolveLocalStoreDbPath } from "./local-store.js"; +import { AMS_POLICY_SPEC_FILENAMES, DEFAULT_AMS_POLICY_SPEC, discoverAmsPolicySpecPath, parseAmsPolicySpecContent } from "@loopover/engine"; +import { resolveLocalStoreConfigDir, resolveLocalStoreDbPath } from "./local-store.js"; -// Resolver for the operator-local `.loopover-ams.yml` (#5132, Wave 3.5 follow-up). AmsPolicySpec -// (ams-policy-spec.ts, engine package) is the type/parser surface; this module is the actual local -// read+resolve caller. +// Resolver for the operator-local AMS policy files (#5132, Wave 3.5 follow-up; discovery order #8863). +// AmsPolicySpec (ams-policy-spec.ts, engine package) is the type/parser surface; this module is the actual +// local read+resolve caller, probing the documented AMS_POLICY_SPEC_FILENAMES order inside the operator config +// directory (an explicit LOOPOVER_MINER_AMS_POLICY_PATH still points at one exact file, bypassing discovery). // // This is deliberately NOT the same resolution shape as self-review-context.js/rejection-signal.js, which // read from the target repo: AmsPolicySpec's fields are the OPERATOR's own execution-risk policy, so an // untrusted target repo must never get final say over them. -const AMS_POLICY_FILENAME = ".loopover-ams.yml"; +// The canonical write path is the first documented discovery candidate; reads probe the full order. +const AMS_POLICY_FILENAME = AMS_POLICY_SPEC_FILENAMES[0]; export type AmsPolicySource = "local" | "default"; @@ -60,6 +63,13 @@ export function resolveAmsPolicyConfigPath(env: Record = process.env): string { + return resolveLocalStoreConfigDir(env); +} + function normalizeOptions(options: AmsPolicyOptions = {}): NormalizedAmsPolicyOptions { return { readFileSync: options.readFileSync ?? readFileSync, @@ -68,11 +78,12 @@ function normalizeOptions(options: AmsPolicyOptions = {}): NormalizedAmsPolicyOp }; } -/** Read the operator's own local `.loopover-ams.yml`, if one exists. Never throws: an unreadable file is - * treated the same as an absent one, falling through to the next resolution layer. */ +/** Read the operator's own local AMS policy file, if one exists in the documented {@link AMS_POLICY_SPEC_FILENAMES} + * discovery order. Never throws: an unreadable file is treated the same as an absent one, falling through to the + * next resolution layer. */ function readLocalAmsPolicyContent(resolved: NormalizedAmsPolicyOptions): string | null { - const path = resolveAmsPolicyConfigPath(resolved.env); - if (!resolved.existsSync(path)) return null; + const path = resolveLocalAmsPolicyReadPath(resolved.env, resolved.existsSync); + if (path === null) return null; try { return resolved.readFileSync(path, "utf8"); } catch { @@ -80,9 +91,26 @@ function readLocalAmsPolicyContent(resolved: NormalizedAmsPolicyOptions): string } } +/** Path of the AMS policy file to read: an explicit `LOOPOVER_MINER_AMS_POLICY_PATH` override (when present) points + * at one exact file and bypasses discovery; otherwise the first {@link AMS_POLICY_SPEC_FILENAMES} candidate that + * exists in the operator config directory (first match wins), or null when none of them exist. */ +function resolveLocalAmsPolicyReadPath( + env: Record, + existsSync: (path: string) => boolean, +): string | null { + const configDir = resolveAmsPolicyConfigDir(env); + const canonicalPath = resolveAmsPolicyConfigPath(env); + if (canonicalPath !== join(configDir, AMS_POLICY_FILENAME)) { + return existsSync(canonicalPath) ? canonicalPath : null; + } + const relativePath = discoverAmsPolicySpecPath((candidate) => existsSync(join(configDir, candidate))); + return relativePath === null ? null : join(configDir, relativePath); +} + /** - * Resolve the real, effective AMS execution policy for one attempt: the operator's own local - * `.loopover-ams.yml` when present (source: "local"), else the engine's safe defaults (source: "default"). + * Resolve the real, effective AMS execution policy for one attempt: the operator's own local AMS policy file when + * present in the documented {@link AMS_POLICY_SPEC_FILENAMES} discovery order (source: "local"), else the engine's + * safe defaults (source: "default"). * Never throws -- an unreadable/malformed local file degrades through the tolerant parser to the safe * defaults, same discipline as every other tolerant parser in this pipeline. * diff --git a/packages/loopover-miner/lib/local-store.ts b/packages/loopover-miner/lib/local-store.ts index 06600cfb66..7c822730df 100644 --- a/packages/loopover-miner/lib/local-store.ts +++ b/packages/loopover-miner/lib/local-store.ts @@ -24,16 +24,25 @@ export function resolveLocalStoreDbPath( ): string { const explicitPath = typeof env[explicitEnvVarName] === "string" ? env[explicitEnvVarName].trim() : ""; if (explicitPath) return explicitPath; + return join(resolveLocalStoreConfigDir(env), defaultDbFileName); +} +/** + * Resolve the package's local-store config directory (the directory the `defaultDbFileName` files live in when no + * explicit per-store env override is set), from `LOOPOVER_MINER_CONFIG_DIR`, then `XDG_CONFIG_HOME` (falling back to + * `~/.config`) — the shared dir half of `resolveLocalStoreDbPath`, exposed so callers that discover among several + * filenames in the same directory (e.g. AMS policy's documented discovery order, #8863) resolve it the same way. + */ +export function resolveLocalStoreConfigDir(env: Record = process.env): string { const explicitConfigDir = typeof env.LOOPOVER_MINER_CONFIG_DIR === "string" ? env.LOOPOVER_MINER_CONFIG_DIR.trim() : ""; - if (explicitConfigDir) return join(explicitConfigDir, defaultDbFileName); + if (explicitConfigDir) return explicitConfigDir; const configHome = typeof env.XDG_CONFIG_HOME === "string" && env.XDG_CONFIG_HOME.trim() ? env.XDG_CONFIG_HOME.trim() : join(homedir(), ".config"); - return join(configHome, "loopover-miner", defaultDbFileName); + return join(configHome, "loopover-miner"); } /** Trim and validate a caller-supplied (or resolved-default) DB path, throwing `invalidPathError` if it is empty. */ diff --git a/test/unit/miner-ams-policy.test.ts b/test/unit/miner-ams-policy.test.ts index 482fa078cd..0c1ca1507f 100644 --- a/test/unit/miner-ams-policy.test.ts +++ b/test/unit/miner-ams-policy.test.ts @@ -1,5 +1,5 @@ -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -8,7 +8,7 @@ vi.mock("@loopover/engine", async () => { }); import { DEFAULT_AMS_POLICY_SPEC } from "../../packages/loopover-engine/src/index"; -import { resolveAmsPolicy, resolveAmsPolicyConfigPath, amsPolicyWarningJsonFields, renderAmsPolicyWarnings } from "../../packages/loopover-miner/lib/ams-policy.js"; +import { resolveAmsPolicy, resolveAmsPolicyConfigPath, resolveAmsPolicyConfigDir, amsPolicyWarningJsonFields, renderAmsPolicyWarnings } from "../../packages/loopover-miner/lib/ams-policy.js"; const roots: string[] = []; @@ -29,6 +29,18 @@ describe("resolveAmsPolicyConfigPath (#5132)", () => { }); }); +describe("resolveAmsPolicyConfigDir (#8863)", () => { + it("resolves the operator config directory from LOOPOVER_MINER_CONFIG_DIR or XDG_CONFIG_HOME", () => { + expect(resolveAmsPolicyConfigDir({ LOOPOVER_MINER_CONFIG_DIR: "/cfg" })).toBe("/cfg"); + expect(resolveAmsPolicyConfigDir({ XDG_CONFIG_HOME: "/xdg" })).toBe(join("/xdg", "loopover-miner")); + }); + + it("falls back to ~/.config/loopover-miner when no config overrides are set or usable", () => { + expect(resolveAmsPolicyConfigDir({})).toBe(join(homedir(), ".config", "loopover-miner")); + expect(resolveAmsPolicyConfigDir({ XDG_CONFIG_HOME: " " })).toBe(join(homedir(), ".config", "loopover-miner")); + }); +}); + describe("amsPolicy warning surfacing helpers (#8853)", () => { it("omits JSON fields and human lines when warnings are empty", () => { expect(amsPolicyWarningJsonFields({ source: "default", warnings: [] })).toEqual({}); @@ -138,4 +150,61 @@ describe("resolveAmsPolicy (#5132)", () => { const result = await resolveAmsPolicy("acme/widgets", { existsSync: () => false }); expect(result).toEqual({ spec: DEFAULT_AMS_POLICY_SPEC, source: "default", warnings: [] }); }); + + it.each([ + [".github/loopover-ams.yml", "submissionMode: observe\n"], + [".loopover-ams.json", '{"submissionMode":"observe"}'], + [".github/loopover-ams.json", '{"submissionMode":"observe"}'], + ])( + "REGRESSION #8863: discovers operator policy at %s in the documented fallback order", + async (relativePath, content) => { + const root = tempRoot(); + mkdirSync(join(root, ".github"), { recursive: true }); + writeFileSync(join(root, relativePath), content); + const result = await resolveAmsPolicy("acme/widgets", { env: { LOOPOVER_MINER_CONFIG_DIR: root } }); + expect(result.source).toBe("local"); + expect(result.spec.submissionMode).toBe("observe"); + }, + ); + + it("REGRESSION #8863: first match wins when multiple AMS_POLICY_SPEC_FILENAMES candidates exist", async () => { + const root = tempRoot(); + mkdirSync(join(root, ".github"), { recursive: true }); + writeFileSync(join(root, ".loopover-ams.yml"), "submissionMode: enforce\n"); + writeFileSync(join(root, ".github/loopover-ams.yml"), "submissionMode: observe\n"); + const result = await resolveAmsPolicy("acme/widgets", { env: { LOOPOVER_MINER_CONFIG_DIR: root } }); + expect(result.source).toBe("local"); + expect(result.spec.submissionMode).toBe("enforce"); + }); + + it("REGRESSION #8863: an explicit LOOPOVER_MINER_AMS_POLICY_PATH bypasses the discovery order", async () => { + const root = tempRoot(); + const explicitPath = join(root, "custom-policy.json"); + writeFileSync(explicitPath, '{"submissionMode":"observe"}'); + writeFileSync(join(root, ".loopover-ams.yml"), "submissionMode: enforce\n"); + const result = await resolveAmsPolicy("acme/widgets", { + env: { LOOPOVER_MINER_CONFIG_DIR: root, LOOPOVER_MINER_AMS_POLICY_PATH: explicitPath }, + }); + expect(result.source).toBe("local"); + expect(result.spec.submissionMode).toBe("observe"); + }); + + it("REGRESSION #8863: treats a missing explicit LOOPOVER_MINER_AMS_POLICY_PATH as absent", async () => { + const result = await resolveAmsPolicy("acme/widgets", { + env: { LOOPOVER_MINER_AMS_POLICY_PATH: "/missing/policy.yml" }, + existsSync: () => false, + }); + expect(result).toEqual({ spec: DEFAULT_AMS_POLICY_SPEC, source: "default", warnings: [] }); + }); + + it("REGRESSION #8863: treats an unreadable explicit LOOPOVER_MINER_AMS_POLICY_PATH as absent", async () => { + const result = await resolveAmsPolicy("acme/widgets", { + env: { LOOPOVER_MINER_AMS_POLICY_PATH: "/unreadable/policy.yml" }, + existsSync: () => true, + readFileSync: () => { + throw new Error("EACCES: permission denied"); + }, + }); + expect(result).toEqual({ spec: DEFAULT_AMS_POLICY_SPEC, source: "default", warnings: [] }); + }); });