diff --git a/reference/core/interfaces/how-to-construction.md b/reference/core/interfaces/how-to-construction.md index 3efea321..7572290f 100644 --- a/reference/core/interfaces/how-to-construction.md +++ b/reference/core/interfaces/how-to-construction.md @@ -124,6 +124,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { @@ -222,6 +223,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { @@ -314,6 +316,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { diff --git a/reference/core/interfaces/how-to-qa.md b/reference/core/interfaces/how-to-qa.md index 89d4c336..0ea31412 100644 --- a/reference/core/interfaces/how-to-qa.md +++ b/reference/core/interfaces/how-to-qa.md @@ -59,6 +59,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { @@ -157,6 +158,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { @@ -249,6 +251,7 @@ Response shape (captured live): "version": null }, "corpusTotal": "", + "engine": "", "erroredRuleIds": [], "issues": [ { diff --git a/src/packages/core-domain/src/application/use-cases/validate-satellite.use-case.ts b/src/packages/core-domain/src/application/use-cases/validate-satellite.use-case.ts index 43d521d2..e773ba42 100644 --- a/src/packages/core-domain/src/application/use-cases/validate-satellite.use-case.ts +++ b/src/packages/core-domain/src/application/use-cases/validate-satellite.use-case.ts @@ -195,6 +195,10 @@ export class ValidateSatelliteUseCase { '', `**Status:** ${result.status.toUpperCase()}`, `**Rules Checked:** ${result.rulesChecked}`, + // #628 — the markdown report is the surface people paste into a PR, so it + // is the one where a coverage figure with no engine beside it does the + // most damage: the reader attributes the engine's reach to the repository. + ...(result.engine ? [`**Engine:** ${result.engine}`] : []), `**Timestamp:** ${result.timestamp}`, '', ]; diff --git a/src/packages/core-domain/src/application/validators/engine-coverage-advisory.spec.ts b/src/packages/core-domain/src/application/validators/engine-coverage-advisory.spec.ts new file mode 100644 index 00000000..a382c57a --- /dev/null +++ b/src/packages/core-domain/src/application/validators/engine-coverage-advisory.spec.ts @@ -0,0 +1,73 @@ +/** + * #628 — `evolith validate` with no flag runs the native evaluator, which decides + * materially fewer rules than `--engine opa` over the same corpus. Both totals + * were honest and every skip was published; what was missing was the sentence + * telling the reader the missing coverage belongs to the ENGINE THEY DID NOT + * CHOOSE rather than to their repository. + * + * These tests pin the two things that make the row worth having: it fires on the + * shape a reader misreads, and it stays quiet otherwise. A row on every run is + * noise that teaches people to skim past it. + */ + +import { RulesetValidatorService } from './ruleset-validator.service'; +import type { RuleCoverage } from './ruleset-validator.types'; + +type Issue = { ruleId: string; blocking: boolean; severity: string; title: string; description: string }; + +function coverage(checked: number, skipped: number, total: number): RuleCoverage { + return { + rulesChecked: checked, + rulesSkipped: skipped, + rulesErrored: 0, + rulesTotal: total, + skippedRuleIds: [], + erroredRuleIds: [], + } as unknown as RuleCoverage; +} + +function advisoryFor(engineType: 'native' | 'opa', c: RuleCoverage): Issue | undefined { + const service = Object.create(RulesetValidatorService.prototype) as Record; + service.engineType = engineType; + return (service as unknown as { + engineCoverageAdvisory(c: RuleCoverage): Issue | undefined; + }).engineCoverageAdvisory(c); +} + +describe('engine coverage advisory (#628)', () => { + it('fires when the native engine skips more than it checks', () => { + const issue = advisoryFor('native', coverage(41, 118, 159)); + + expect(issue).toBeDefined(); + expect(issue!.ruleId).toBe('GOV-ENGINE-COVERAGE'); + expect(issue!.severity).toBe('COULD'); + // Reporting a coverage gap must not fail a run: the two engines are allowed + // to differ on reach, and only on reach. + expect(issue!.blocking).toBe(false); + // The point of the row is the attribution, so it has to be in the title -- + // a reader who only sees the issue table still gets it. + expect(issue!.title).toContain('this is the engine, not your repository'); + expect(issue!.description).toContain('--engine opa'); + expect(issue!.description).toContain('41'); + expect(issue!.description).toContain('118'); + }); + + it('says nothing on the opa engine, however little it decided', () => { + expect(advisoryFor('opa', coverage(2, 157, 159))).toBeUndefined(); + }); + + it('says nothing when the native engine decided most of its scope', () => { + expect(advisoryFor('native', coverage(133, 26, 159))).toBeUndefined(); + }); + + it('does not fire on a tie, only when skips genuinely outnumber checks', () => { + expect(advisoryFor('native', coverage(80, 80, 160))).toBeUndefined(); + expect(advisoryFor('native', coverage(79, 81, 160))).toBeDefined(); + }); + + it('reports the skipped share, and does not divide by zero on an empty scope', () => { + expect(advisoryFor('native', coverage(0, 0, 0))).toBeUndefined(); + const issue = advisoryFor('native', coverage(40, 160, 200)); + expect(issue!.description).toContain('80%'); + }); +}); diff --git a/src/packages/core-domain/src/application/validators/ruleset-validator.service.ts b/src/packages/core-domain/src/application/validators/ruleset-validator.service.ts index f1ceee38..6a9533ea 100644 --- a/src/packages/core-domain/src/application/validators/ruleset-validator.service.ts +++ b/src/packages/core-domain/src/application/validators/ruleset-validator.service.ts @@ -44,6 +44,7 @@ export class RulesetValidatorService { /** GT-569 — optional coverage floor; see {@link RulesetValidatorOptions.maxSkippedFraction}. */ private readonly maxSkippedFraction?: number; private readonly rulesetRepo: IRulesetRepository; + private readonly engineType: 'native' | 'opa'; /** GT-571 — filter the corpus by rule audience / topology / SDLC phase. */ private readonly applyRuleApplicability: boolean; /** @@ -89,6 +90,9 @@ export class RulesetValidatorService { this.processRunner = options.processRunner; this.metrics = options.metrics; this.rulesetRepo = options.rulesetRepo; + // #628: the report has to be able to say WHICH engine produced it. The two + // do not cover the same ground, and the default is the one that covers less. + this.engineType = options.engineType === 'opa' ? 'opa' : 'native'; const baseStrategy = options.engineType === 'opa' ? new OpaEvaluator(this.fs, this.logger) @@ -241,6 +245,8 @@ export class RulesetValidatorService { const thresholdIssue = this.coverageThresholdIssue(coverage); if (thresholdIssue) issues.push(thresholdIssue); issues.push(...this.corpusLoadIssues()); + const engineIssue = this.engineCoverageAdvisory(coverage); + if (engineIssue) issues.push(engineIssue); } catch (err: unknown) { // GT-474: an unresolvable/empty ruleset corpus must never be downgraded to // a warning here — that is exactly how `validate` came to report @@ -274,6 +280,10 @@ export class RulesetValidatorService { // without parsing issue text. blockingSkippedRuleIds: coverage.blockingSkippedRuleIds, perRuleset: coverage.perRuleset, + // #628 — WHICH engine produced these numbers. Two engines ship and they do + // not cover the same ground, so a coverage figure without an engine beside + // it is not readable. + engine: this.engineType, // GT-661 — WHY this scope, not just how much of it. selection: selectionReport, issues, @@ -447,6 +457,49 @@ export class RulesetValidatorService { return issues; } + /** + * #628 -- `evolith validate` with no flag runs the native evaluator, which + * decides materially fewer rules than `--engine opa` over the same corpus. + * Both totals were honest and the skips were all published; what was missing + * was the sentence telling the reader that the missing coverage belongs to the + * ENGINE THEY DID NOT CHOOSE rather than to their repository. + * + * Deliberately narrow. It fires only on the native engine and only when skips + * outnumber checks, because that is the shape a reader misreads. A run where + * the engine decided most of what it was handed needs no explanation, and a + * row on every run is noise that teaches people to skim past it. + * + * Non-blocking. The engines are ALLOWED to differ on coverage -- + * `68-validate-engine-verdict-parity.mjs` holds them to agreement on facts, + * not on reach -- so this reports a fact about the run, it does not fail it. + */ + private engineCoverageAdvisory(coverage: RuleCoverage): ValidationIssue | undefined { + if (this.engineType !== 'native') return undefined; + if (coverage.rulesSkipped <= coverage.rulesChecked) return undefined; + + const share = coverage.rulesTotal > 0 + ? Math.round((coverage.rulesSkipped / coverage.rulesTotal) * 100) + : 0; + + return { + ruleId: 'GOV-ENGINE-COVERAGE', + severity: 'COULD', + category: 'governance', + title: + `The native engine skipped more rules than it checked ` + + `(${coverage.rulesSkipped} of ${coverage.rulesTotal}) — this is the engine, not your repository`, + description: + `This run used the native evaluator, the default when no \`--engine\` is given. It decided ` + + `${coverage.rulesChecked} of the ${coverage.rulesTotal} rules in scope and skipped ` + + `${coverage.rulesSkipped} (${share}%). A skip here usually means the native evaluator has no ` + + 'handler for that rule, not that your repository failed to satisfy it. ' + + 'Re-run with `--engine opa` to evaluate against the compiled Rego bundle, which decides more of ' + + 'the same corpus. The two engines are held to agreement on the verdicts they both reach; they ' + + 'are not held to equal reach, and this run got the shorter one.', + blocking: false, + }; + } + private applicabilityAdvisory(notApplicable: readonly NotApplicableRule[]): ValidationIssue | undefined { if (notApplicable.length === 0) return undefined; diff --git a/src/packages/core-domain/src/application/validators/ruleset-validator.types.ts b/src/packages/core-domain/src/application/validators/ruleset-validator.types.ts index fe298b48..2341384a 100644 --- a/src/packages/core-domain/src/application/validators/ruleset-validator.types.ts +++ b/src/packages/core-domain/src/application/validators/ruleset-validator.types.ts @@ -101,6 +101,13 @@ export interface RuleApplicabilitySummary { export interface ValidationResult { status: 'passed' | 'failed' | 'warning'; + /** + * #628 — which evaluator produced these numbers. Two ship and they do not + * cover the same ground, so a coverage figure without an engine beside it + * cannot be read. Optional for the same additive reason as the counters + * below; `RulesetValidatorService.validate` always populates it. + */ + engine?: 'native' | 'opa'; rulesChecked: number; /** * GT-569 coverage denominator. Declared OPTIONAL only so the wire envelope diff --git a/src/sdk/cli/src/commands/validate/validate.command.ts b/src/sdk/cli/src/commands/validate/validate.command.ts index 55da66be..9fe01953 100644 --- a/src/sdk/cli/src/commands/validate/validate.command.ts +++ b/src/sdk/cli/src/commands/validate/validate.command.ts @@ -560,6 +560,9 @@ export class ValidateCommand extends BaseEvolithCommand { rulesSkipped: result.rulesSkipped, rulesErrored: result.rulesErrored, rulesTotal: result.rulesTotal, + // #628 — the machine-readable surfaces carry the engine too; a captured + // table with a coverage figure and no engine cannot be compared to another. + engine: result.engine, issues: result.issues.map(i => ({ ruleId: i.ruleId, severity: i.severity, @@ -703,10 +706,27 @@ export class ValidateCommand extends BaseEvolithCommand { const errored = result.rulesErrored ?? 0; const total = result.rulesTotal ?? checked + skipped + errored; + // #628 — the engine travels WITH the counts, on the same line, because the + // counts are as much a property of the evaluator as of the repository: two + // engines ship, they do not cover the same ground, and the default is the + // one that covers less. A denominator without the engine beside it is the + // GT-569 defect one level up. + const engine = result.engine ? ` — engine: ${result.engine}` : ''; this.promptService.showInfo( - `\nRules: ${checked} checked / ${skipped} skipped / ${errored} errored / ${total} total`, + `\nRules: ${checked} checked / ${skipped} skipped / ${errored} errored / ${total} total${engine}`, ); + // Only on the shape a reader misreads: the default engine, skipping more + // than it decided. A hint on every native run is noise that teaches people + // to skim past it, and there is nothing to redirect an `--engine opa` run to. + if (result.engine === 'native' && skipped > checked) { + this.promptService.showWarning( + ' Most of those skips belong to the ENGINE, not to your repository: the native evaluator has no ' + + 'handler for them. Re-run with `--engine opa` to evaluate the same corpus against the compiled ' + + 'Rego bundle, which decides more of it.', + ); + } + // GT-661 — the SCOPE, next to the counts, because the human reader is the // one who most needs to tell "the pack I adopted failed" from "the Core // evaluated all of its opinions and something failed". Both render the same diff --git a/src/sdk/cli/src/commands/validate/validate.engine-disclosure.spec.ts b/src/sdk/cli/src/commands/validate/validate.engine-disclosure.spec.ts new file mode 100644 index 00000000..c7e8d597 --- /dev/null +++ b/src/sdk/cli/src/commands/validate/validate.engine-disclosure.spec.ts @@ -0,0 +1,171 @@ +/** + * #628 — `evolith validate` with no flag runs the native evaluator, which decides + * materially fewer rules than `--engine opa` over the same corpus. Both totals + * were honest and every skip was published; what was missing was the line telling + * the reader WHICH engine produced them, so the missing coverage read as a fact + * about their repository. + * + * GT-569 (see `validate.coverage-report.spec.ts`) pinned that the denominator is + * always reported. These pin the other half: the denominator is attributed, and + * the redirect to the wider engine stays quiet on every shape a reader would not + * misread. + */ + +import { ValidateCommand } from './validate.command'; + +jest.mock('@beyondnet/evolith-core-domain/application/use-cases/validate-satellite.use-case', () => ({ + ValidateSatelliteUseCase: jest.fn().mockImplementation(() => ({ execute: jest.fn() })), +})); + +jest.mock('@beyondnet/evolith-core-domain/application/validators/ruleset-validator.service', () => ({ + RulesetValidatorService: jest.fn().mockImplementation(() => ({ + validate: jest.fn(), + validateArchitecture: jest.fn(), + loadRulesetById: jest.fn(), + })), +})); + +jest.mock('../../infrastructure/paths/rulesets-resolver', () => ({ + resolveRulesets: (override?: string) => ({ + coreRoot: override ?? '/bundled-core', + rulesetsRoot: `${override ?? '/bundled-core'}/rulesets`, + source: override ? 'override' : 'bundled', + }), +})); + +jest.mock('../../infrastructure/prompts/prompt.service', () => ({ + PromptService: jest.fn().mockImplementation(() => ({ + showIntro: jest.fn(), showOutro: jest.fn(), showSuccess: jest.fn(), + showError: jest.fn(), showWarning: jest.fn(), showInfo: jest.fn(), + startSpinner: jest.fn(), stopSpinner: jest.fn(), confirm: jest.fn(), + })), +})); + +jest.mock('../../infrastructure/observability', () => ({ + logger: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, +})); + +jest.mock('../../infrastructure/formatters/output-formatter.service', () => ({ + OutputFormatterService: jest.fn().mockImplementation(() => ({ + format: jest.fn(() => 'formatted output'), + })), +})); + +import { ValidateSatelliteUseCase } from '@beyondnet/evolith-core-domain/application/use-cases/validate-satellite.use-case'; +import { RulesetValidatorService } from '@beyondnet/evolith-core-domain/application/validators/ruleset-validator.service'; +import { OutputFormatterService } from '../../infrastructure/formatters/output-formatter.service'; +import { PromptService } from '../../infrastructure/prompts/prompt.service'; + +const mockExecute = jest.fn(); +const mockValidateArchitecture = jest.fn(); +const mockFormat = jest.fn(); + +(ValidateSatelliteUseCase as jest.Mock).mockImplementation(() => ({ execute: mockExecute })); +(RulesetValidatorService as jest.Mock).mockImplementation(() => ({ validateArchitecture: mockValidateArchitecture })); +(OutputFormatterService as jest.Mock).mockImplementation(() => ({ format: mockFormat })); + +/** A run in which only 2 of 6 rules actually executed. */ +const partiallyCoveredResult = { + status: 'passed' as const, + rulesChecked: 2, + rulesSkipped: 3, + rulesErrored: 1, + rulesTotal: 6, + skippedRuleIds: ['SKIP-01', 'SKIP-02', 'SKIP-03'], + erroredRuleIds: ['CRASH-01'], + issues: [], + coreRef: { version: '1.0.0', path: '/core' }, + timestamp: '2024-01-01T00:00:00.000Z', +}; + +describe('#628 · validate names the engine that produced the coverage', () => { + let command: ValidateCommand; + let prompts: jest.Mocked; + let logSpy: jest.SpyInstance; + let exitSpy: jest.SpyInstance; + + beforeEach(() => { + const useCase = new ValidateSatelliteUseCase(); + const validator = new RulesetValidatorService(); + prompts = new PromptService() as jest.Mocked; + command = new ValidateCommand(useCase, validator, prompts); + logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => undefined as never); + jest.clearAllMocks(); + mockExecute.mockReset(); + mockValidateArchitecture.mockReset(); + mockFormat.mockReset().mockReturnValue('formatted output'); + }); + + afterEach(() => { + logSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + const jsonEnvelope = (): any => { + const calls = logSpy.mock.calls; + return JSON.parse(calls[calls.length - 1][0] as string); + }; + const infoLines = (): string[] => prompts.showInfo.mock.calls.map(c => String(c[0])); + const warnLines = (): string[] => prompts.showWarning.mock.calls.map(c => String(c[0])); + + it('names the engine on the same line as the counts', async () => { + mockExecute.mockResolvedValue({ result: { ...partiallyCoveredResult, engine: 'native' } }); + + await command.run([], { format: 'unknown' }); + + expect(infoLines().some(l => /2 checked.*6 total.*engine: native/.test(l))).toBe(true); + }); + + it('redirects to --engine opa when the default engine skipped more than it checked', async () => { + mockExecute.mockResolvedValue({ + result: { ...partiallyCoveredResult, engine: 'native', rulesChecked: 41, rulesSkipped: 118, rulesErrored: 0, rulesTotal: 159 }, + }); + + await command.run([], { format: 'unknown' }); + + const hint = warnLines().find(l => l.includes('--engine opa')); + expect(hint).toBeDefined(); + // The attribution is the whole point: a reader who sees only this line + // must learn the skips are the evaluator's, not their repository's. + expect(hint).toContain('not to your repository'); + }); + + it('stays quiet about the other engine when the native run decided most of its scope', async () => { + mockExecute.mockResolvedValue({ + result: { ...partiallyCoveredResult, engine: 'native', rulesChecked: 133, rulesSkipped: 26, rulesErrored: 0, rulesTotal: 159 }, + }); + + await command.run([], { format: 'unknown' }); + + expect(warnLines().some(l => l.includes('--engine opa'))).toBe(false); + }); + + it('never redirects an opa run to itself, however little it decided', async () => { + mockExecute.mockResolvedValue({ + result: { ...partiallyCoveredResult, engine: 'opa', rulesChecked: 2, rulesSkipped: 157, rulesErrored: 0, rulesTotal: 159 }, + }); + + await command.run([], { format: 'unknown' }); + + expect(infoLines().some(l => l.includes('engine: opa'))).toBe(true); + expect(warnLines().some(l => l.includes('--engine opa'))).toBe(false); + }); + + it('omits the engine rather than guessing when the producer did not report one', async () => { + mockExecute.mockResolvedValue({ result: partiallyCoveredResult }); + + await command.run([], { format: 'unknown' }); + + expect(infoLines().some(l => l.includes('engine:'))).toBe(false); + }); + + it('carries the engine on the wire, so a captured envelope can be compared to another (#628)', async () => { + mockExecute.mockResolvedValue({ result: { ...partiallyCoveredResult, engine: 'native' } }); + + await command.run([], { format: 'json' }); + + expect(jsonEnvelope().data).toHaveProperty('engine', 'native'); + }); + +}); diff --git a/src/tests/contract/sdk-type-contract.types.ts b/src/tests/contract/sdk-type-contract.types.ts index 02c8cbc6..45d3a7de 100644 --- a/src/tests/contract/sdk-type-contract.types.ts +++ b/src/tests/contract/sdk-type-contract.types.ts @@ -323,6 +323,11 @@ export const WIRE_VALIDATION_RESULT: WireCheck = { accepts: oneOf('passed', 'failed', 'warning'), }, rulesChecked: { required: true, declaredAs: 'number', accepts: isNumber }, + // #628: WHICH evaluator produced the counts. Two engines ship and they do not + // cover the same ground, so a consumer comparing two captures cannot read a + // coverage figure without this. Optional for the same additive reason as the + // GT-569 fields below: an envelope from a producer that predates it is valid. + engine: { required: false, declaredAs: "'native'|'opa'", accepts: oneOf('native', 'opa') }, // GT-569: `rulesChecked` alone counts only what was evaluated, so it silently // redefined its own denominator — a corpus of 380 rules could report 111 // "checked" with 269 never executed and nothing on the wire said so. These five