From 4def1d69b4d044b1771f64367bf0a1427c7cef0e Mon Sep 17 00:00:00 2001 From: Laith Al-Saadoon <9553966+theagenticguy@users.noreply.github.com> Date: Fri, 5 Jun 2026 16:25:15 -0500 Subject: [PATCH] fix(cli): wire changed-paths and license classification into verdict policy context buildPolicyContext previously hardcoded licenseViolations: [], touchedPaths: [], and ownersByPath: new Map(), so an authored opencodehub.policy.yaml with license_allowlist or ownership_required rules schema-validated but never blocked a PR (false green on exit codes). - touchedPaths: surface the changed-file list computeVerdict already derives from detect_changes as VerdictResponse.changedFiles, and thread it into touchedPaths so path-scoped rules (ownership_required) evaluate against the real diff. - licenseViolations: classify the indexed Dependency nodes via classifyDependencies and project flagged deps (copyleft/proprietary/unknown) into LicenseViolationInput so license_allowlist fires. Declared license surfaced verbatim; missing/UNKNOWN normalised to "UNKNOWN" so a strict policy can deny those too. Scan only runs when a policy file is actually loaded. - ownership: require_approval_from now works against real touched paths. Per-path graph-owner mapping (ownersByPath) stays empty pending a contributor-email->team reconciliation source (separate design item). Tests prove a license_allowlist rule flips the verdict to block (exit 3) when a violating dep is present, license_allowlist passes for permissive deps, UNKNOWN-deny works, ownership_required blocks on an unapproved changed path and passes when no path matches. Existing exit-code ladder tests stay green. --- packages/analysis/src/verdict-types.ts | 7 + packages/analysis/src/verdict.test.ts | 1 + packages/analysis/src/verdict.ts | 2 + packages/cli/src/commands/verdict.test.ts | 212 +++++++++++++++++++++- packages/cli/src/commands/verdict.ts | 80 ++++++-- packages/policy/src/index.ts | 7 +- 6 files changed, 290 insertions(+), 19 deletions(-) diff --git a/packages/analysis/src/verdict-types.ts b/packages/analysis/src/verdict-types.ts index e4595346..0b5f4ded 100644 --- a/packages/analysis/src/verdict-types.ts +++ b/packages/analysis/src/verdict-types.ts @@ -70,6 +70,13 @@ export interface VerdictResponse { readonly communitiesTouched: readonly string[]; /** Number of changed files. */ readonly changedFileCount: number; + /** + * Repo-relative paths of the changed files (forward-slash separated), as + * computed by `detect_changes`. Surfaced so policy evaluation can run + * path-scoped rules (`ownership_required`, `changed_paths`) against the + * real diff rather than a re-derived file list. + */ + readonly changedFiles: readonly string[]; /** Number of affected symbols. */ readonly affectedSymbolCount: number; } diff --git a/packages/analysis/src/verdict.test.ts b/packages/analysis/src/verdict.test.ts index df589128..12e2d63f 100644 --- a/packages/analysis/src/verdict.test.ts +++ b/packages/analysis/src/verdict.test.ts @@ -236,6 +236,7 @@ test("renderVerdictMarkdown: header + tier + labels rendered", () => { blastRadius: 55, communitiesTouched: ["c1"], changedFileCount: 3, + changedFiles: ["a.ts", "b.ts", "c.ts"], affectedSymbolCount: 5, }); assert.match(md, /OpenCodeHub Verdict: `block`/); diff --git a/packages/analysis/src/verdict.ts b/packages/analysis/src/verdict.ts index 8396a347..24a77121 100644 --- a/packages/analysis/src/verdict.ts +++ b/packages/analysis/src/verdict.ts @@ -304,6 +304,7 @@ export async function computeVerdict( blastRadius: state.blastRadius, communitiesTouched, changedFileCount: changes.changedFiles.length, + changedFiles: changes.changedFiles, affectedSymbolCount: changes.affectedSymbols.length, }; const reviewCommentMarkdown = renderVerdictMarkdown(response); @@ -343,6 +344,7 @@ function finaliseEmpty( blastRadius: 0, communitiesTouched: [], changedFileCount: changes.changedFiles.length, + changedFiles: changes.changedFiles, affectedSymbolCount: 0, }; return { ...response, reviewCommentMarkdown: renderVerdictMarkdown(response) }; diff --git a/packages/cli/src/commands/verdict.test.ts b/packages/cli/src/commands/verdict.test.ts index f9fce59c..7ec53ba0 100644 --- a/packages/cli/src/commands/verdict.test.ts +++ b/packages/cli/src/commands/verdict.test.ts @@ -30,7 +30,16 @@ import { cliExitCodeForTier } from "./verdict-render.js"; // --- fixtures -------------------------------------------------------------- -function fakeStore(): IGraphStore { +interface FakeDependency { + readonly id: string; + readonly name: string; + readonly version: string; + readonly ecosystem: string; + readonly license?: string; + readonly lockfileSource: string; +} + +function fakeStore(deps: readonly FakeDependency[] = []): IGraphStore { const unreachable = () => { throw new Error("fakeStore used for a real query — test is mis-wired"); }; @@ -47,11 +56,14 @@ function fakeStore(): IGraphStore { getMeta: async () => undefined, setMeta: async () => undefined, healthCheck: async () => ({ ok: true }), + listDependencies: async () => deps, } as unknown as IGraphStore; } -function stubStoreFactory(): () => Promise<{ store: IGraphStore; repoPath: string }> { - return async () => ({ store: fakeStore(), repoPath: "/tmp/fake-repo" }); +function stubStoreFactory( + deps: readonly FakeDependency[] = [], +): () => Promise<{ store: IGraphStore; repoPath: string }> { + return async () => ({ store: fakeStore(deps), repoPath: "/tmp/fake-repo" }); } function verdictFixture( @@ -75,6 +87,11 @@ function verdictFixture( blastRadius: 42, communitiesTouched: ["c1", "c2", "c3"], changedFileCount: 7, + changedFiles: [ + "packages/storage/src/duckdb-adapter.ts", + "packages/cli/src/index.ts", + "README.md", + ], affectedSymbolCount: 19, }; return { ...base, ...overrides }; @@ -433,9 +450,10 @@ test("runVerdict: ownership_required rule passes when approvals are supplied", a }, ], }; - // touchedPaths comes from the verdict pipeline (not yet surfaced in v1), - // so this rule is a no-op until that lands. We still assert the pass - // to pin down the current behavior. + // touchedPaths now comes from the verdict pipeline (verdict.changedFiles). + // The auto_merge fixture touches `packages/storage/src/duckdb-adapter.ts`, + // which matches the rule glob — so the rule fires, but the supplied + // @storage-team approval satisfies require_approval_from → pass. const { exitCode } = await withExitCode(async () => { try { await runVerdict({ @@ -457,6 +475,188 @@ test("runVerdict: ownership_required rule passes when approvals are supplied", a assert.equal(exitCode, 0); }); +test("runVerdict: license_allowlist rule blocks (exit 3) when a denied license is present", async () => { + const cap = captureStdout(); + const pol: Policy = { + version: 1, + rules: [{ type: "license_allowlist", id: "no-copyleft", deny: ["GPL-3.0"] }], + }; + // The store reports a GPL-3.0 dependency — classifyDependencies flags it + // copyleft, the CLI projects it into licenseViolations, and the deny list + // matches → block, escalating the auto_merge exit (0) to a policy block (3). + const { exitCode } = await withExitCode(async () => { + try { + await runVerdict({ + outputFormat: "summary", + exitCode: true, + storeFactory: stubStoreFactory([ + { + id: "Dependency:npm:left-pad", + name: "left-pad", + version: "1.3.0", + ecosystem: "npm", + license: "GPL-3.0", + lockfileSource: "pnpm-lock.yaml", + }, + ]), + computeVerdictFn: stubCompute("auto_merge"), + loadPolicyFn: async () => pol, + }); + } finally { + cap.restore(); + } + }); + const output = cap.chunks.join(""); + assert.match(output, /Policy: block/); + assert.match(output, /no-copyleft: license "GPL-3.0" from package "left-pad" is denied/); + assert.equal(exitCode, 3); +}); + +test("runVerdict: license_allowlist passes when no dependency license is denied", async () => { + const cap = captureStdout(); + const pol: Policy = { + version: 1, + rules: [{ type: "license_allowlist", id: "no-copyleft", deny: ["GPL-3.0"] }], + }; + // A permissive dep is never flagged by classifyDependencies, so it never + // reaches licenseViolations — the deny list has nothing to match. + const { exitCode } = await withExitCode(async () => { + try { + await runVerdict({ + outputFormat: "json", + exitCode: true, + storeFactory: stubStoreFactory([ + { + id: "Dependency:npm:lodash", + name: "lodash", + version: "4.17.21", + ecosystem: "npm", + license: "MIT", + lockfileSource: "pnpm-lock.yaml", + }, + ]), + computeVerdictFn: stubCompute("auto_merge"), + loadPolicyFn: async () => pol, + }); + } finally { + cap.restore(); + } + }); + const output = cap.chunks.join(""); + const parsed = JSON.parse(output) as Record; + const policy = parsed["policy"] as { status: string; violations: unknown[] }; + assert.equal(policy.status, "pass"); + assert.deepEqual(policy.violations, []); + assert.equal(exitCode, 0); +}); + +test("runVerdict: license_allowlist can deny UNKNOWN for deps with no declared license", async () => { + const cap = captureStdout(); + const pol: Policy = { + version: 1, + rules: [{ type: "license_allowlist", id: "no-unknown", deny: ["UNKNOWN"] }], + }; + // A dep with a missing license is normalised to "UNKNOWN" by the CLI, so + // a policy can deny it explicitly. + const { exitCode } = await withExitCode(async () => { + try { + await runVerdict({ + outputFormat: "summary", + exitCode: true, + storeFactory: stubStoreFactory([ + { + id: "Dependency:npm:mystery", + name: "mystery", + version: "0.1.0", + ecosystem: "npm", + lockfileSource: "pnpm-lock.yaml", + }, + ]), + computeVerdictFn: stubCompute("auto_merge"), + loadPolicyFn: async () => pol, + }); + } finally { + cap.restore(); + } + }); + const output = cap.chunks.join(""); + assert.match(output, /Policy: block/); + assert.match(output, /no-unknown: license "UNKNOWN" from package "mystery" is denied/); + assert.equal(exitCode, 3); +}); + +test("runVerdict: ownership_required blocks (exit 3) when a changed path lacks approval", async () => { + const cap = captureStdout(); + const pol: Policy = { + version: 1, + rules: [ + { + type: "ownership_required", + id: "storage-owner", + paths: ["packages/storage/**"], + require_approval_from: ["@storage-team"], + }, + ], + }; + // The auto_merge fixture touches packages/storage/src/duckdb-adapter.ts, + // which matches the rule glob. No approval supplied → block, proving the + // rule sees the real changedFiles threaded through touchedPaths. + const { exitCode } = await withExitCode(async () => { + try { + await runVerdict({ + outputFormat: "summary", + exitCode: true, + storeFactory: stubStoreFactory(), + computeVerdictFn: stubCompute("auto_merge"), + loadPolicyFn: async () => pol, + }); + } finally { + cap.restore(); + } + }); + const output = cap.chunks.join(""); + assert.match(output, /Policy: block/); + assert.match( + output, + /storage-owner: path "packages\/storage\/src\/duckdb-adapter.ts" requires approval from one of: @storage-team/, + ); + assert.equal(exitCode, 3); +}); + +test("runVerdict: ownership_required passes when no changed path matches the glob", async () => { + const cap = captureStdout(); + const pol: Policy = { + version: 1, + rules: [ + { + type: "ownership_required", + id: "infra-owner", + paths: ["infra/**"], + require_approval_from: ["@infra-team"], + }, + ], + }; + // None of the fixture's changedFiles live under infra/ → rule is a no-op. + const { exitCode } = await withExitCode(async () => { + try { + await runVerdict({ + outputFormat: "json", + exitCode: true, + storeFactory: stubStoreFactory(), + computeVerdictFn: stubCompute("auto_merge"), + loadPolicyFn: async () => pol, + }); + } finally { + cap.restore(); + } + }); + const output = cap.chunks.join(""); + const parsed = JSON.parse(output) as Record; + const policy = parsed["policy"] as { status: string }; + assert.equal(policy.status, "pass"); + assert.equal(exitCode, 0); +}); + test("runVerdict propagates base/head/config to the compute fn", async () => { const cap = captureStdout(); let seen: VerdictQuery | undefined; diff --git a/packages/cli/src/commands/verdict.ts b/packages/cli/src/commands/verdict.ts index a635e05f..864c5e4a 100644 --- a/packages/cli/src/commands/verdict.ts +++ b/packages/cli/src/commands/verdict.ts @@ -4,7 +4,9 @@ import { join } from "node:path"; import { + classifyDependencies, computeVerdict, + type DependencyRef, type VerdictConfig, type VerdictQuery, type VerdictResponse, @@ -12,6 +14,7 @@ import { } from "@opencodehub/analysis"; import { evaluatePolicy, + type LicenseViolationInput, loadPolicy, type Policy, type PolicyContext, @@ -119,9 +122,15 @@ export async function runVerdict(opts: VerdictCliOptions = {}): Promise { const load = opts.loadPolicyFn ?? loadPolicy; const policyPath = join(repoPath, "opencodehub.policy.yaml"); const policy = await load(policyPath); + // Only pay the dependency scan when a policy is actually loaded — the + // starter (no-file) repo skips it entirely. + const licenseViolations = policy !== undefined ? await collectLicenseViolations(store) : []; const policyDecision = policy !== undefined - ? evaluatePolicy(policy, buildPolicyContext(verdict, opts.approvals ?? [])) + ? evaluatePolicy( + policy, + buildPolicyContext(verdict, opts.approvals ?? [], licenseViolations), + ) : undefined; const baseOutput = @@ -149,24 +158,71 @@ export async function runVerdict(opts: VerdictCliOptions = {}): Promise { } } -function buildPolicyContext(verdict: VerdictResponse, approvals: readonly string[]): PolicyContext { +function buildPolicyContext( + verdict: VerdictResponse, + approvals: readonly string[], + licenseViolations: readonly LicenseViolationInput[], +): PolicyContext { return { - // v1 wiring: verdict does not yet compute a license audit, so we - // surface an empty set. license_allowlist rules therefore pass until a - // follow-up task wires SBOM license data in. - licenseViolations: [], + // License findings come from `classifyDependencies` over the indexed + // Dependency nodes (see collectLicenseViolations). `license_allowlist` + // rules fire when a flagged dep's declared license is in the rule's + // `deny` list. + licenseViolations, blastRadiusTier: POLICY_TIER_FOR_VERDICT[verdict.verdict], - // v1 wiring: ownership_required rules inspect touched paths. We don't - // have the raw changed-file list on VerdictResponse yet — the closest - // structured data is communitiesTouched. Leave touchedPaths empty for - // now; this means ownership_required is a no-op until the verdict - // pipeline surfaces changed paths explicitly. - touchedPaths: [], + // `ownership_required` / changed-path rules evaluate against the real + // diff: the changed-file list `computeVerdict` already derived from + // `detect_changes`, surfaced on the response. + touchedPaths: verdict.changedFiles, + // ownersByPath stays empty: mapping each path to an approving owner + // needs a contributor-email -> team/handle reconciliation source that + // does not exist yet (separate design item). `require_approval_from` + // still works because the evaluator unions it with any path owners. ownersByPath: new Map(), approvals, }; } +/** + * Classify the indexed Dependency nodes and project the flagged ones into + * `LicenseViolationInput[]` so `license_allowlist` rules have something to + * match against. The declared license string is surfaced verbatim (so a + * policy can `deny: ["GPL-3.0"]`); deps with a missing or `"UNKNOWN"` + * license report as `"UNKNOWN"` so a strict policy can deny those too. + * + * Defensive: legacy test fakes inject a bare `IGraphStore` without + * `listDependencies`. When the finder is absent or throws (pre-dependency + * index), we return no violations rather than crashing the verdict. + */ +async function collectLicenseViolations( + store: IGraphStore | Store, +): Promise { + const graph: IGraphStore = "graph" in store ? store.graph : store; + if (typeof graph.listDependencies !== "function") return []; + let deps: DependencyRef[]; + try { + const all = await graph.listDependencies(); + deps = all.map((d) => ({ + id: d.id, + name: d.name, + version: typeof d.version === "string" ? d.version : "UNKNOWN", + ecosystem: typeof d.ecosystem === "string" ? d.ecosystem : "unknown", + license: typeof d.license === "string" && d.license.length > 0 ? d.license : "UNKNOWN", + lockfileSource: typeof d.lockfileSource === "string" ? d.lockfileSource : "", + })); + } catch { + return []; + } + const result = classifyDependencies(deps); + const out: LicenseViolationInput[] = []; + // Copyleft + proprietary carry their declared license; unknown deps were + // normalised to "UNKNOWN" above. + for (const d of result.flagged.copyleft) out.push({ license: d.license, package: d.name }); + for (const d of result.flagged.proprietary) out.push({ license: d.license, package: d.name }); + for (const d of result.flagged.unknown) out.push({ license: d.license, package: d.name }); + return out; +} + /** * Merge the policy decision into the JSON output. Parsing the rendered JSON * is marginally slower than re-stringifying `verdict`, but it keeps a single diff --git a/packages/policy/src/index.ts b/packages/policy/src/index.ts index 4cd1e6a8..0cf5beba 100644 --- a/packages/policy/src/index.ts +++ b/packages/policy/src/index.ts @@ -8,7 +8,12 @@ * - Schemas + types for the 3 v1 rule shapes. */ -export type { PolicyContext, PolicyDecision, PolicyViolation } from "./evaluate.js"; +export type { + LicenseViolationInput, + PolicyContext, + PolicyDecision, + PolicyViolation, +} from "./evaluate.js"; export { evaluatePolicy } from "./evaluate.js"; export { loadPolicy, PolicyValidationError } from "./load.js"; export type {