diff --git a/.act-replies-44.tsv b/.act-replies-44.tsv new file mode 100644 index 000000000..fbadc210a --- /dev/null +++ b/.act-replies-44.tsv @@ -0,0 +1,6 @@ +PRRT_kwDOTyoI9s6YxjZz Fixed — `verifyPolicyAgainstGraph` now normalizes `checks/` prefixes, so bare check ids like `typecheck` match steps like `checks/typecheck` and align with `evaluatePolicy`. +PRRT_kwDOTyoI9s6Yxmn2 Fixed — the rule iteration now guards `checkIds` against `null`, `undefined`, and non-array values, treating them as no check filter. +PRRT_kwDOTyoI9s6Yxn9P Fixed — bare check ids are canonical; `verifyPolicyAgainstGraph` and `evaluatePolicy` normalize `checks/` prefixes and `evaluatePolicy` matches rule-qualified finding ids (`:`). `extractFindings` also strips `checks/` before normalization. +PRRT_kwDOTyoI9s6Yxn9R Fixed — `verifyPolicyAgainstGraph` now validates `policy.failOn` and `graph.project.pipelines` and returns structural errors in `PolicyVerification.errors` without throwing. +PRRT_kwDOTyoI9s6Y3M-8 Fixed — `specs/15-findings/spec.md` now declares `compareBaseline(findings, baseline)` matching the implementation and tests. +PRRT_kwDOTyoI9s6Y3M_A Fixed — `specs/16-policy/spec.md` now defines that `failOn[].checkIds` refer to check steps (IDs starting with `checks/`), with normalization for `checks/` prefix and rule-qualified findings; implementation and tests are aligned. diff --git a/bun.lock b/bun.lock index dc5f2c8b0..9d68f1fef 100644 --- a/bun.lock +++ b/bun.lock @@ -159,6 +159,7 @@ "name": "@sverka/policy", "version": "0.0.0", "dependencies": { + "@sverka/core": "workspace:*", "@sverka/findings": "workspace:*", }, "devDependencies": { diff --git a/packages/checks/src/__tests__/extract.test.ts b/packages/checks/src/__tests__/extract.test.ts index d8e0eaf65..c9ae7c8fc 100644 --- a/packages/checks/src/__tests__/extract.test.ts +++ b/packages/checks/src/__tests__/extract.test.ts @@ -20,6 +20,16 @@ describe("extractFindings — SARIF", () => { expect(findings.length).toBeGreaterThan(0); expect(findings[0]!.checkId).toContain("mycheck"); }); + + it("strips checks/ prefix from checkId when extracting", async () => { + const dir = makeDir(); + writeFileSync(join(dir, "out.sarif"), JSON.stringify(sampleSarif())); + const outputs: CheckOutput[] = [{ path: "out.sarif", format: "sarif" }]; + const findings = await extractFindings(outputs, dir, "checks/mycheck"); + expect(findings.length).toBeGreaterThan(0); + expect(findings[0]!.checkId.startsWith("checks/")).toBe(false); + expect(findings[0]!.checkId).toContain("mycheck"); + }); }); describe("extractFindings — missing file", () => { diff --git a/packages/checks/src/extract.ts b/packages/checks/src/extract.ts index 3caeec9aa..8afd5a52a 100644 --- a/packages/checks/src/extract.ts +++ b/packages/checks/src/extract.ts @@ -78,9 +78,10 @@ export async function extractFindings( ); } try { + const checkIdPrefix = checkId.startsWith("checks/") ? checkId.slice(7) : checkId; const ctx: NormalizeContext = { root: artifactDir, - checkIdPrefix: checkId, + checkIdPrefix, defaultConfidence: 0.5, }; const result = normalizeSarif(parsed as SarifLog, ctx); diff --git a/packages/policy/package.json b/packages/policy/package.json index f76ba6e96..640ba9ee4 100644 --- a/packages/policy/package.json +++ b/packages/policy/package.json @@ -19,7 +19,8 @@ "typecheck": "tsc --noEmit" }, "dependencies": { - "@sverka/findings": "workspace:*" + "@sverka/findings": "workspace:*", + "@sverka/core": "workspace:*" }, "devDependencies": { "tsdown": "^0.22.0", diff --git a/packages/policy/src/__tests__/evaluator.test.ts b/packages/policy/src/__tests__/evaluator.test.ts index 1cf57efbd..89e0fb93d 100644 --- a/packages/policy/src/__tests__/evaluator.test.ts +++ b/packages/policy/src/__tests__/evaluator.test.ts @@ -111,6 +111,27 @@ describe("failOn rules", () => { expect(r.triggered).toHaveLength(0); }); + it("checkIds filter matches rule-qualified finding checkIds", () => { + const f = makeFinding({ checkId: "check-a:rule-1", severity: "high" }); + const policy = createPolicy({ + failOn: [{ severity: "high", onlyNew: false, checkIds: ["check-a"] }], + }); + const r = evaluatePolicy([f], policy, []); + expect(r.verdict).toBe("fail"); + expect(r.triggered).toHaveLength(1); + expect(r.triggered[0]?.finding.checkId).toBe("check-a:rule-1"); + }); + + it("checkIds filter accepts checks/ prefix", () => { + const f = makeFinding({ checkId: "check-a:rule-1", severity: "high" }); + const policy = createPolicy({ + failOn: [{ severity: "high", onlyNew: false, checkIds: ["checks/check-a"] }], + }); + const r = evaluatePolicy([f], policy, []); + expect(r.verdict).toBe("fail"); + expect(r.triggered[0]?.finding.checkId).toBe("check-a:rule-1"); + }); + it("multiple failOn rules evaluated independently", () => { const low = findingAt("low"); const crit = findingAt("critical"); diff --git a/packages/policy/src/__tests__/public-api.test.ts b/packages/policy/src/__tests__/public-api.test.ts index 2b0cecb52..91ff35f86 100644 --- a/packages/policy/src/__tests__/public-api.test.ts +++ b/packages/policy/src/__tests__/public-api.test.ts @@ -6,6 +6,7 @@ import { DEFAULT_POLICY, createPolicy, evaluatePolicy, + verifyPolicyAgainstGraph, PolicyError, type Verdict, type Policy, @@ -14,6 +15,7 @@ import { type RuleResult, type PolicyResult, type PolicyConfig, + type PolicyVerification, type PolicyErrorCode, } from "../index.js"; @@ -30,6 +32,10 @@ describe("public API — functions and constants", () => { it("exports evaluatePolicy function", () => { expect(typeof evaluatePolicy).toBe("function"); }); + + it("exports verifyPolicyAgainstGraph function", () => { + expect(typeof verifyPolicyAgainstGraph).toBe("function"); + }); }); describe("public API — error class", () => { @@ -66,6 +72,7 @@ describe("public API — types (compile-time check)", () => { summary: "", }; const _config: PolicyConfig = {}; + const _verification: PolicyVerification = { valid: true, unknownCheckIds: [] }; const _code: PolicyErrorCode = "INVALID_POLICY"; // Touch all to avoid unused warnings. expect(_verdict).toBe("pass"); @@ -75,6 +82,7 @@ describe("public API — types (compile-time check)", () => { expect(_ruleResult.triggered).toBe(false); expect(_result.verdict).toBe("pass"); expect(_config).toBeDefined(); + expect(_verification.valid).toBe(true); expect(_code).toBe("INVALID_POLICY"); }); }); diff --git a/packages/policy/src/__tests__/verify.test.ts b/packages/policy/src/__tests__/verify.test.ts new file mode 100644 index 000000000..672c01583 --- /dev/null +++ b/packages/policy/src/__tests__/verify.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect } from "vitest"; +import { verifyPolicyAgainstGraph } from "../verify.js"; +import { DEFAULT_POLICY } from "../policy.js"; +import type { Policy } from "../types.js"; +import type { DefinitionGraph } from "@sverka/core"; + +function makeGraph(stepIds: string[]): DefinitionGraph { + return { + project: { + id: "test", + pipelines: [{ + id: "ci", + inputs: {}, + entries: [], + steps: stepIds.map((id) => ({ + id, + runtime: { mode: "host" as const }, + operations: [{ kind: "shell" as const, command: "echo hi" }], + inputs: [], + outputs: [], + dependencies: [], + })), + outputs: [], + }], + }, + }; +} + +function makePolicy(checkIds?: string[]): Policy { + return { + name: "test", + default: "pass", + failOn: checkIds + ? [{ severity: "high", onlyNew: false, checkIds }] + : [{ severity: "high", onlyNew: false }], + }; +} + +describe("verifyPolicyAgainstGraph", () => { + it("valid policy (all checkIds match) → valid=true", () => { + const graph = makeGraph(["checks/typecheck", "checks/lint"]); + const policy = makePolicy(["checks/typecheck", "checks/lint"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(true); + expect(result.unknownCheckIds).toEqual([]); + }); + + it("unknown checkId → valid=false, listed", () => { + const graph = makeGraph(["checks/typecheck"]); + const policy = makePolicy(["checks/typecheck", "checks/unknown"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(false); + expect(result.unknownCheckIds).toEqual(["checks/unknown"]); + }); + + it("policy with no checkIds → valid=true", () => { + const graph = makeGraph(["checks/typecheck"]); + const policy = makePolicy(); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(true); + expect(result.unknownCheckIds).toEqual([]); + }); + + it("multiple unknown checkIds → all listed", () => { + const graph = makeGraph(["checks/typecheck"]); + const policy = makePolicy(["checks/typecheck", "checks/foo", "checks/bar"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(false); + expect(result.unknownCheckIds).toHaveLength(2); + expect(result.unknownCheckIds).toContain("checks/foo"); + expect(result.unknownCheckIds).toContain("checks/bar"); + }); + + it("DEFAULT_POLICY (no checkIds) → valid=true", () => { + const graph = makeGraph([]); + const result = verifyPolicyAgainstGraph(DEFAULT_POLICY, graph); + expect(result.valid).toBe(true); + }); + + it("empty graph → unknown checkIds reported", () => { + const graph = makeGraph([]); + const policy = makePolicy(["checks/typecheck"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(false); + expect(result.unknownCheckIds).toEqual(["checks/typecheck"]); + }); + + it("deduplicates checkIds across rules", () => { + const graph = makeGraph(["checks/typecheck"]); + const policy: Policy = { + name: "test", + default: "pass", + failOn: [ + { severity: "high", onlyNew: false, checkIds: ["checks/unknown"] }, + { severity: "medium", onlyNew: false, checkIds: ["checks/unknown"] }, + ], + }; + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(false); + // "checks/unknown" appears in both rules but should be listed once. + expect(result.unknownCheckIds).toEqual(["checks/unknown"]); + }); + + it("accepts bare checkIds that match checks/ steps", () => { + const graph = makeGraph(["checks/typecheck", "checks/lint"]); + const policy = makePolicy(["typecheck", "lint"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(true); + expect(result.unknownCheckIds).toEqual([]); + }); + + it("ignores non-check steps when validating checkIds", () => { + const graph = makeGraph(["build/compile"]); + const policy = makePolicy(["compile"]); + const result = verifyPolicyAgainstGraph(policy, graph); + expect(result.valid).toBe(false); + expect(result.unknownCheckIds).toEqual(["compile"]); + }); + + it("returns errors for malformed policy or graph without throwing", () => { + const badPolicy = { name: "x" } as unknown as Policy; + const badGraph = { project: {} } as unknown as DefinitionGraph; + const result = verifyPolicyAgainstGraph(badPolicy, badGraph); + expect(result.valid).toBe(false); + expect(result.unknownCheckIds).toEqual([]); + expect(result.errors).toBeDefined(); + expect(result.errors!.length).toBeGreaterThanOrEqual(1); + }); +}); diff --git a/packages/policy/src/evaluator.ts b/packages/policy/src/evaluator.ts index ca99066c8..443ce5c50 100644 --- a/packages/policy/src/evaluator.ts +++ b/packages/policy/src/evaluator.ts @@ -8,6 +8,19 @@ import type { import { PolicyError } from "./errors.js"; import { severityRank, assertValidSeverity } from "./policy.js"; +/** Strip the `checks/` prefix so `checks/typecheck` and `typecheck` compare equal. */ +function normalizeCheckId(id: string): string { + return id.startsWith("checks/") ? id.slice(7) : id; +} + +/** Match a finding checkId against a policy checkId, allowing rule-qualified ids. */ +function matchesCheckId(findingCheckId: string, policyCheckId: string): boolean { + const bareFinding = normalizeCheckId(findingCheckId); + const barePolicy = normalizeCheckId(policyCheckId); + if (bareFinding === barePolicy) return true; + return bareFinding.startsWith(`${barePolicy}:`); +} + /** Severity display order for summary counts (most severe first). */ const SUMMARY_SEVERITY_ORDER: Severity[] = [ "critical", @@ -60,9 +73,16 @@ export function evaluatePolicy( for (let i = 0; i < policy.failOn.length; i++) { const rule = policy.failOn[i]!; + const rawCheckIds = rule.checkIds as string | string[] | undefined | null; + const checkIds = rawCheckIds === undefined || rawCheckIds === null + ? [] + : Array.isArray(rawCheckIds) + ? rawCheckIds + : [rawCheckIds]; + let matched: Finding[] = findings.filter((f) => { - // checkIds filter (exact match) - if (rule.checkIds !== undefined && !rule.checkIds.includes(f.checkId)) { + // checkIds filter (supports bare, checks/ prefix, and rule-qualified ids) + if (checkIds.length > 0 && !checkIds.some((id) => matchesCheckId(f.checkId, id))) { return false; } // onlyNew filter diff --git a/packages/policy/src/index.ts b/packages/policy/src/index.ts index 5a0bc724a..e89c7e637 100644 --- a/packages/policy/src/index.ts +++ b/packages/policy/src/index.ts @@ -1,6 +1,8 @@ -// @sverka/policy — public API +// @sverka/policy — public API. Spec 16. export type { Verdict, Policy, FailOnRule, TriggeredFinding, RuleResult, PolicyResult, PolicyConfig } from "./types.js"; export { DEFAULT_POLICY, createPolicy } from "./policy.js"; export { evaluatePolicy } from "./evaluator.js"; +export { verifyPolicyAgainstGraph } from "./verify.js"; +export type { PolicyVerification } from "./verify.js"; export { PolicyError, type PolicyErrorCode } from "./errors.js"; diff --git a/packages/policy/src/verify.ts b/packages/policy/src/verify.ts new file mode 100644 index 000000000..cd474580a --- /dev/null +++ b/packages/policy/src/verify.ts @@ -0,0 +1,93 @@ +// Policy verification against a Definition Graph. +// Spec 16 — §26, §27. Verifies that policy checkIds reference check +// steps that exist in the graph. + +import type { DefinitionGraph } from "@sverka/core"; +import type { Policy } from "./types.js"; + +export interface PolicyVerification { + readonly valid: boolean; + readonly unknownCheckIds: readonly string[]; + readonly errors?: readonly string[]; +} + +/** Strip the `checks/` prefix so `checks/typecheck` and `typecheck` compare equal. */ +function normalizeCheckId(id: string): string { + return id.startsWith("checks/") ? id.slice(7) : id; +} + +/** + * Verify that all checkIds referenced in a policy's failOn rules exist + * as check step IDs in the Definition Graph. A step is treated as a check + * when its ID starts with `checks/`. Returns a PolicyVerification + * with valid=false and the list of unknown check IDs if any are found. + * + * Does not throw for structural errors — returns them in `errors`. + */ +export function verifyPolicyAgainstGraph( + policy: Policy, + graph: DefinitionGraph, +): PolicyVerification { + const errors = collectValidationErrors(policy, graph); + if (errors.length > 0) { + return { valid: false, unknownCheckIds: [], errors }; + } + + const knownCheckIds = collectKnownCheckIds(graph); + const referencedCheckIds = collectReferencedCheckIds(policy); + const unknownCheckIds = findUnknownCheckIds(referencedCheckIds, knownCheckIds); + + return { + valid: unknownCheckIds.length === 0, + unknownCheckIds, + }; +} + +function collectValidationErrors(policy: Policy, graph: DefinitionGraph): string[] { + const errors: string[] = []; + if (!policy || typeof policy !== "object") { + errors.push("invalid policy: expected object"); + } else if (!Array.isArray(policy.failOn)) { + errors.push("invalid policy: failOn must be an array"); + } + if (!graph || typeof graph !== "object" || !Array.isArray(graph.project?.pipelines)) { + errors.push("invalid graph: project.pipelines must be an array"); + } + return errors; +} + +function collectKnownCheckIds(graph: DefinitionGraph): Set { + const knownCheckIds = new Set(); + for (const pipeline of graph.project.pipelines) { + if (!Array.isArray(pipeline.steps)) continue; + for (const step of pipeline.steps) { + if (typeof step.id === "string" && step.id.startsWith("checks/")) { + knownCheckIds.add(normalizeCheckId(step.id)); + } + } + } + return knownCheckIds; +} + +function collectReferencedCheckIds(policy: Policy): Set { + const referencedCheckIds = new Set(); + for (const rule of policy.failOn) { + const raw = (rule as { checkIds?: unknown }).checkIds; + if (raw === undefined || raw === null) continue; + const ids: string[] = Array.isArray(raw) ? (raw as string[]) : [String(raw)]; + for (const id of ids) { + if (typeof id === "string") referencedCheckIds.add(id); + } + } + return referencedCheckIds; +} + +function findUnknownCheckIds(referenced: Set, known: Set): string[] { + const unknown: string[] = []; + for (const id of referenced) { + if (!known.has(normalizeCheckId(id))) { + unknown.push(id); + } + } + return unknown; +} diff --git a/specs/15-findings/spec.md b/specs/15-findings/spec.md index f39c6d201..c71dd68aa 100644 --- a/specs/15-findings/spec.md +++ b/specs/15-findings/spec.md @@ -1,34 +1,56 @@ # Spec 15 — Findings -**Status:** Stub — to be written by architect during the corresponding wave. -**Source:** specs/architecture-spec.md (authoritative) +**Status:** Active (carry-over verification) +**Source:** specs/architecture-spec.md §26, §27 +**Package:** `@sverka/findings` (unchanged) ## Overview -TODO — architect fills in during wave design phase. Reference the architecture -spec section(s) listed in the reconciliation plan -(engdocs/architecture/v0-architecture-spec-reconciliation.md). +The findings package provides SARIF normalization, fingerprinting, +baselining, and suppression. It is a carry-over package — the +implementation is unchanged from the pre-v0 build. Wave K verifies +that it works correctly with the new engine output (Run Plan execution +produces SARIF files that `extractFindings` from `@sverka/checks` +consumes, which in turn calls `normalizeSarif` from this package). ## Goals -TODO +- Verify `normalizeSarif` works with SARIF produced by checks running + under the native engine +- Verify `computeFingerprint` produces stable fingerprints for findings + from engine-executed checks +- Verify baseline operations (create, update, compare, load, save) work + with the new finding IDs +- Verify suppression filtering works with the new finding shape +- No API changes — the package is unchanged ## Non-goals -TODO +- New SARIF features or format versions +- New fingerprint algorithms +- New baseline formats +- Integration with engine events (findings are extracted post-execution) ## Interfaces -TODO - -## Data models - -TODO - -## Error handling - -TODO +Unchanged from existing implementation: + +```ts +function normalizeSarif(log: SarifLog, ctx: NormalizeContext): readonly Finding[]; +function computeFingerprint(input: FingerprintInput): string; +function createBaseline(findings: readonly Finding[]): Baseline; +function updateBaseline(baseline: Baseline, findings: readonly Finding[]): Baseline; +function compareBaseline(findings: readonly Finding[], baseline: Baseline): BaselineDiff; +function loadBaseline(path: string): Promise; +function saveBaseline(baseline: Baseline, path: string): Promise; +function isSuppressed(finding: Finding, suppressions: readonly Suppression[]): boolean; +function filterSuppressed(findings: readonly Finding[], suppressions: readonly Suppression[]): readonly Finding[]; +function filterOnlyNew(findings: readonly Finding[], baseline: Baseline): readonly Finding[]; +``` ## Test plan -TODO +1. Regression: all 88 existing findings tests pass unchanged. +2. Integration: `normalizeSarif` + `computeFingerprint` produce stable + findings from a SARIF sample that the engine would produce. +3. Public API: all exports present, no any types. diff --git a/specs/16-policy/spec.md b/specs/16-policy/spec.md index 8e47c4638..f97580b4c 100644 --- a/specs/16-policy/spec.md +++ b/specs/16-policy/spec.md @@ -1,34 +1,92 @@ # Spec 16 — Policy -**Status:** Stub — to be written by architect during the corresponding wave. -**Source:** specs/architecture-spec.md (authoritative) +**Status:** Active (carry-over + Definition Graph verification) +**Source:** specs/architecture-spec.md §26, §27 +**Package:** `@sverka/policy` (extended) ## Overview -TODO — architect fills in during wave design phase. Reference the architecture -spec section(s) listed in the reconciliation plan -(engdocs/architecture/v0-architecture-spec-reconciliation.md). +The policy package evaluates findings against a policy definition to +produce a pass/fail verdict. It is a carry-over package — the core +evaluation logic is unchanged. Wave K adds `verifyPolicyAgainstGraph`, +which checks that a policy's `checkIds` references match actual check +steps in a Definition Graph. ## Goals -TODO +- Verify existing policy evaluation works with findings from the new + engine + checks pipeline +- Add `verifyPolicyAgainstGraph(policy, graph)`: validates that all + `failOn[].checkIds` in a policy reference steps that exist in the + Definition Graph (specifically steps with IDs matching `checks/*`) +- Report unknown check IDs as verification errors +- No changes to existing evaluation logic ## Non-goals -TODO +- Policy definition syntax (YAML/JSON config parsing) +- Policy enforcement in the engine (the engine runs steps; policy is + evaluated post-execution by the CLI or orchestrator) +- Provider-specific policy translation +- Dynamic policy rules (§32 — deferred) ## Interfaces -TODO +```ts +import type { DefinitionGraph } from "@sverka/core"; +import type { Policy } from "./types.js"; + +interface PolicyVerification { + readonly valid: boolean; + readonly unknownCheckIds: readonly string[]; + readonly errors?: readonly string[]; +} + +function verifyPolicyAgainstGraph(policy: Policy, graph: DefinitionGraph): PolicyVerification; +``` + +### Exports + +```ts +export { verifyPolicyAgainstGraph }; +export type { PolicyVerification }; +// All existing exports unchanged. +``` ## Data models -TODO +**Verification**: `verifyPolicyAgainstGraph` collects all `checkIds` +referenced across all `failOn` rules in the policy. A policy `checkId` +matches a graph step when it is equal to the bare check id or to the +`checks/` step id; e.g., both `typecheck` and `checks/typecheck` +match a step whose id is `checks/typecheck`. Only steps whose IDs start +with `checks/` are considered checks. Unknown check IDs are collected +and returned. `valid` is `true` when no unknown check IDs are found. + +**Policy checkId matching scope**: `failOn[].checkIds` identify checks, +not individual rules. Findings produced by a check have a `checkId` of +`:` (or `checks/:`). A `checkId` +value in a policy rule matches a finding when the bare check ids are +equal or when the finding's checkId starts with `:`. +`evaluatePolicy` normalizes `checks/` prefixes and rule-qualified +finding ids consistently with `verifyPolicyAgainstGraph`. ## Error handling -TODO +Reuses existing `PolicyError` with codes: +- `INVALID_POLICY`: policy is malformed (existing) +- `INVALID_SEVERITY`: rule has unknown severity (existing) + +`verifyPolicyAgainstGraph` does not throw — it returns a +`PolicyVerification` with `valid: false` and the list of unknown check +IDs. Structural errors in the policy or graph are returned in the +optional `errors` field of `PolicyVerification`. ## Test plan -TODO +1. Regression: all 55 existing policy tests pass unchanged. +2. `verifyPolicyAgainstGraph`: valid policy (all checkIds match) → valid=true. +3. `verifyPolicyAgainstGraph`: unknown checkId → valid=false, listed. +4. `verifyPolicyAgainstGraph`: policy with no checkIds → valid=true. +5. `verifyPolicyAgainstGraph`: multiple unknown checkIds → all listed. +6. Public API: all exports present, no any types.