diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index e2e3af61d..21997873d 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -144,6 +144,17 @@ jobs: - name: Self-test for the joined-path guard run: node --test .harness/scripts/ci/47-validate-joined-paths.test.mjs + # GT-587. The OpenTelemetry GenAI conventions are Development-status, and the + # Core emits their attribute names as plain LITERALS (it must not import an + # observability SDK — rule HXA-05). Literals do not move when upstream moves, so + # a rename there turns Evolith's "standard" telemetry back into a private + # vocabulary with nothing going red. This is the check that goes red. + - name: Validate the pinned OpenTelemetry semconv revision + run: node .harness/scripts/ci/51-validate-semconv-pin.mjs --verbose + + - name: Self-test for the semconv pin guard + run: node --test .harness/scripts/ci/51-validate-semconv-pin.test.mjs + # GT-597. The gate itself runs inside `openssf-scorecard.yml`, where the # Scorecard JSON exists; only its logic is tested here. That logic is the # difference between a weekly score that is published and a weekly score diff --git a/.harness/scripts/ci/51-validate-semconv-pin.mjs b/.harness/scripts/ci/51-validate-semconv-pin.mjs new file mode 100644 index 000000000..775b02efa --- /dev/null +++ b/.harness/scripts/ci/51-validate-semconv-pin.mjs @@ -0,0 +1,200 @@ +#!/usr/bin/env node + +/** + * GT-587 criterion 3 — the pinned OpenTelemetry semconv revision is DECLARED, and a + * drift in it is a red build. + * + * ## Why a pin needs a guard at all + * + * The GenAI semantic conventions are Development-status: upstream may rename or drop an + * attribute between releases. Evolith emits `gen_ai.*` and `mcp.*` names from + * `src/packages/core-domain/src/evaluation/telemetry/semconv.ts`, as plain literals — + * the Core is pure and must not import an observability SDK (rule HXA-05). Literals do + * not move when upstream moves. So the failure this guard exists to catch is not loud: + * the build stays green, the tests stay green, and the emitted telemetry quietly stops + * being the standard it claims to be. That is a SECOND private vocabulary wearing the + * costume of a standard one, which is worse than the private names it replaced. + * + * ## What it checks (three rules, each able to go red on its own) + * + * 1. **Version pin.** `SEMCONV_VERSION` must equal the `@opentelemetry/semantic-conventions` + * version resolved in `package-lock.json`. Reading the LOCKFILE, not `node_modules`, + * is deliberate: the lockfile is committed, so the check answers the same way on a + * machine that has never run `npm install`, and cannot be made green by local state. + * 2. **Literal agreement.** Every symbol declared `exported` in `PINNED_SEMCONV_ATTRIBUTES` + * must exist in the installed package with EXACTLY our value. A rename upstream, or a + * typo here, fails. + * 3. **Manifest completeness.** Every `ATTR_*` / `EVENT_*` / `MCP_*_VALUE_*` constant the + * module exports must appear in the manifest. Without this rule a new literal could be + * added and never checked — the manifest would still pass while covering less. + * + * `registry-only` symbols (in the upstream registry, not yet exported by the package at the + * pinned version) are the interesting third state: they are ALLOWED to be missing, and they + * FAIL the day the package starts exporting them with a different value. That is exactly the + * moment a locally-declared literal silently becomes wrong. + * + * ## Anti-vacuous pass + * + * Zero pinned symbols, or a semconv module that cannot be read, is a hard exit 1 through the + * repository's own `assertScanned` — never "nothing to check, all good". `resolve()` is + * fail-closed, so a moved file is a loud error rather than an empty corpus. + * + * Usage: + * node .harness/scripts/ci/51-validate-semconv-pin.mjs + * node .harness/scripts/ci/51-validate-semconv-pin.mjs --verbose + */ + +import { readFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { runInNewContext } from 'node:vm'; +import { assertScanned } from '../lib/coverage.mjs'; +import { resolve } from '../lib/paths.mjs'; + +const GUARD = '51-validate-semconv-pin'; +const VERBOSE = process.argv.slice(2).includes('--verbose'); + +/** Path KEY, not a literal — `lib/paths.mjs` owns the location and fails closed if it moves. */ +const SEMCONV_MODULE_KEY = 'semconvPin'; +const SEMCONV_PACKAGE = '@opentelemetry/semantic-conventions'; +const LOCKFILE_KEY = `node_modules/${SEMCONV_PACKAGE}`; + +const require_ = createRequire(import.meta.url); + +/** + * Load the pinned vocabulary from SOURCE. + * + * Not from `dist/`: that is gitignored build output, so a check reading it would be + * measuring whatever the last local build happened to leave behind. TypeScript is a + * declared dependency of this repository, so transpiling the source is both hermetic + * and exact — a hand-rolled regex over `export const` lines would drift the first time + * someone writes a multi-line declaration. + */ +function loadPinnedVocabulary(file) { + const ts = require_('typescript'); + const source = readFileSync(file, 'utf8'); + const { outputText } = ts.transpileModule(source, { + compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2022 }, + }); + const module_ = { exports: {} }; + runInNewContext(outputText, { module: module_, exports: module_.exports, require: require_ }); + return module_.exports; +} + +/** Version `npm ci` would install — read from the committed lockfile, not node_modules. */ +function lockedSemconvVersion() { + const lock = JSON.parse(readFileSync(resolve('rootPackageLock'), 'utf8')); + return lock.packages?.[LOCKFILE_KEY]?.version; +} + +function main() { + const modulePath = resolve(SEMCONV_MODULE_KEY); + const vocabulary = loadPinnedVocabulary(modulePath); + + const pinned = vocabulary.PINNED_SEMCONV_ATTRIBUTES ?? []; + assertScanned(pinned.length, { + what: 'pinned semconv symbols', + where: `${SEMCONV_MODULE_KEY} → PINNED_SEMCONV_ATTRIBUTES`, + }); + + const violations = []; + + // --- Rule 1: the declared revision is the revision the lockfile installs --------- + const declared = vocabulary.SEMCONV_VERSION; + const locked = lockedSemconvVersion(); + if (!locked) { + violations.push( + `${SEMCONV_PACKAGE} is not present in package-lock.json under '${LOCKFILE_KEY}'.\n` + + ` The pin cannot be verified against anything, which is indistinguishable from\n` + + ` not being pinned. Declare the dependency or remove the pin.`, + ); + } else if (declared !== locked) { + violations.push( + `SEMCONV_VERSION drift: the module declares '${declared}', the lockfile installs '${locked}'.\n` + + ` The GenAI conventions are Development-status, so an upgrade may have renamed or\n` + + ` dropped an attribute. Re-verify every literal in PINNED_SEMCONV_ATTRIBUTES against\n` + + ` ${SEMCONV_PACKAGE}@${locked}, THEN bump SEMCONV_VERSION — never the other way round.`, + ); + } + + // --- Rule 2: each pinned literal still matches upstream -------------------------- + let upstream; + try { + upstream = require_(`${SEMCONV_PACKAGE}/incubating`); + } catch (err) { + violations.push( + `cannot load '${SEMCONV_PACKAGE}/incubating' (${err.message}).\n` + + ` Without it the literals are unverifiable. This is a failure, not a skip: a check\n` + + ` that quietly passes when it cannot look is the defect it was written to prevent.`, + ); + upstream = undefined; + } + + let compared = 0; + if (upstream) { + for (const symbol of pinned) { + const actual = upstream[symbol.exportName]; + if (symbol.upstream === 'exported') { + compared += 1; + if (actual === undefined) { + violations.push( + `${symbol.exportName} is declared 'exported' but ${SEMCONV_PACKAGE}@${locked} no longer\n` + + ` exports it. Evolith is emitting '${symbol.value}' against a convention that moved.`, + ); + } else if (actual !== symbol.value) { + violations.push( + `${symbol.exportName} drift: pinned as '${symbol.value}', upstream now '${actual}'.\n` + + ` Update the literal in the ${SEMCONV_MODULE_KEY} module and everything that asserts on it.`, + ); + } + } else if (symbol.upstream === 'registry-only') { + if (actual !== undefined) { + compared += 1; + if (actual !== symbol.value) { + violations.push( + `${symbol.exportName} was declared 'registry-only' (locally spelled '${symbol.value}')\n` + + ` and upstream now exports it as '${actual}'. The local spelling is wrong from this\n` + + ` release onwards. Adopt the upstream value and change upstream: 'exported'.`, + ); + } + } + } else { + violations.push(`${symbol.exportName} has an unknown upstream status '${symbol.upstream}'.`); + } + } + } + + // --- Rule 3: the manifest covers every literal the module exports ---------------- + const manifestNames = new Set(pinned.map((s) => s.exportName)); + const declaredConstants = Object.keys(vocabulary).filter( + (k) => k.startsWith('ATTR_') || k.startsWith('EVENT_') || /^MCP_[A-Z_]+_VALUE_/.test(k), + ); + assertScanned(declaredConstants.length, { + what: 'exported semconv constants', + where: SEMCONV_MODULE_KEY, + }); + for (const name of declaredConstants) { + if (!manifestNames.has(name)) { + violations.push( + `${name} is exported but absent from PINNED_SEMCONV_ATTRIBUTES, so nothing checks it\n` + + ` against upstream. Add it to the manifest with its upstream status.`, + ); + } + } + + if (violations.length > 0) { + console.error(`✗ ${GUARD}: ${violations.length} finding(s)\n`); + for (const v of violations) console.error(` • ${v}\n`); + process.exit(1); + } + + console.log(`✅ ${GUARD}`); + console.log(` pinned revision .... ${declared} (lockfile: ${locked})`); + console.log(` symbols pinned ..... ${pinned.length}`); + console.log(` compared upstream .. ${compared}`); + console.log(` registry-only ...... ${pinned.filter((s) => s.upstream === 'registry-only').length} (allowed to be absent upstream)`); + if (VERBOSE) { + for (const s of pinned) console.log(` • ${s.exportName} = ${s.value} [${s.upstream}]`); + } +} + +main(); diff --git a/.harness/scripts/ci/51-validate-semconv-pin.test.mjs b/.harness/scripts/ci/51-validate-semconv-pin.test.mjs new file mode 100644 index 000000000..20fbbe5c2 --- /dev/null +++ b/.harness/scripts/ci/51-validate-semconv-pin.test.mjs @@ -0,0 +1,144 @@ +#!/usr/bin/env node --test + +/** + * Negative fixtures for `51-validate-semconv-pin.mjs` (GT-587 criterion 3). + * + * A pin check that has never been observed failing is decoration. Each case below + * materialises a repo-SHAPED sandbox — the three ROOT_MARKERS so `lib/paths.mjs` + * ascends to the sandbox and not to this repository, a copy of `.harness/scripts`, a + * synthetic `package-lock.json`, and one synthetic `semconv.ts` — and asserts the guard's + * exit code. The green case is included deliberately: without it, a guard that failed + * unconditionally would pass every red test and look thorough. + * + * Run: node --test .harness/scripts/ci/51-validate-semconv-pin.test.mjs + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { cpSync, existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url'; +import { spawnSync } from 'node:child_process'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(__dirname, '../../..'); +const GUARD_REL = '.harness/scripts/ci/51-validate-semconv-pin.mjs'; + +/** The version the fixture lockfile claims, matching the fixture module unless a case says otherwise. */ +const PINNED = '1.41.1'; + +/** + * A minimal but REAL semconv module: same export shapes the guard parses, and values + * that genuinely match `@opentelemetry/semantic-conventions@1.41.1`. Using fake values + * would make the green case pass for the wrong reason. + */ +function semconvModule({ version = PINNED, nameValue = 'gen_ai.evaluation.name', extraConstant = '' } = {}) { + return [ + `export const SEMCONV_VERSION = '${version}';`, + `export const EVENT_GEN_AI_EVALUATION_RESULT = 'gen_ai.evaluation.result';`, + `export const ATTR_GEN_AI_EVALUATION_NAME = '${nameValue}';`, + `export const ATTR_MCP_TOOL_NAME = 'mcp.tool.name';`, + extraConstant, + `export interface PinnedSemconvSymbol {`, + ` readonly exportName: string;`, + ` readonly value: string;`, + ` readonly upstream: 'exported' | 'registry-only';`, + `}`, + `export const PINNED_SEMCONV_ATTRIBUTES: readonly PinnedSemconvSymbol[] = [`, + ` { exportName: 'EVENT_GEN_AI_EVALUATION_RESULT', value: EVENT_GEN_AI_EVALUATION_RESULT, upstream: 'exported' },`, + ` { exportName: 'ATTR_GEN_AI_EVALUATION_NAME', value: ATTR_GEN_AI_EVALUATION_NAME, upstream: 'exported' },`, + ` { exportName: 'ATTR_MCP_TOOL_NAME', value: ATTR_MCP_TOOL_NAME, upstream: 'registry-only' },`, + `];`, + ].join('\n'); +} + +function sandbox(moduleSource, lockedVersion = PINNED) { + // realpath: on macOS os.tmpdir() is a symlink, and a guard comparing import.meta.url + // against process.argv[1] would then skip its own main() and exit 0 in silence. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'gt587-semconv-'))); + + writeFileSync(join(root, 'package.json'), JSON.stringify({ name: 'fixture', private: true })); + writeFileSync(join(root, 'evolith.yaml'), 'name: fixture\n'); + writeFileSync( + join(root, 'package-lock.json'), + JSON.stringify({ + packages: lockedVersion + ? { 'node_modules/@opentelemetry/semantic-conventions': { version: lockedVersion } } + : {}, + }), + ); + + mkdirSync(join(root, '.harness'), { recursive: true }); + cpSync(join(REPO_ROOT, '.harness/scripts'), join(root, '.harness/scripts'), { recursive: true }); + + const modulePath = join(root, 'src/packages/core-domain/src/evaluation/telemetry/semconv.ts'); + mkdirSync(dirname(modulePath), { recursive: true }); + writeFileSync(modulePath, moduleSource); + + const realNodeModules = join(REPO_ROOT, 'node_modules'); + if (existsSync(realNodeModules)) symlinkSync(realNodeModules, join(root, 'node_modules'), 'dir'); + + return root; +} + +function run(root) { + const res = spawnSync(process.execPath, [join(root, GUARD_REL)], { cwd: root, encoding: 'utf8' }); + return { code: res.status, out: `${res.stdout}${res.stderr}` }; +} + +function withSandbox(moduleSource, lockedVersion, fn) { + const root = sandbox(moduleSource, lockedVersion); + try { + fn(run(root)); + } finally { + rmSync(root, { recursive: true, force: true }); + } +} + +test('PASSES on a pin that agrees with the lockfile and with upstream', () => { + withSandbox(semconvModule(), PINNED, ({ code, out }) => { + assert.equal(code, 0, out); + assert.match(out, /pinned revision/); + }); +}); + +test('FAILS when the declared revision drifts from the lockfile', () => { + withSandbox(semconvModule({ version: '1.99.0' }), PINNED, ({ code, out }) => { + assert.equal(code, 1, out); + assert.match(out, /SEMCONV_VERSION drift/); + }); +}); + +test('FAILS when a pinned literal no longer matches upstream', () => { + withSandbox(semconvModule({ nameValue: 'gen_ai.eval.name' }), PINNED, ({ code, out }) => { + assert.equal(code, 1, out); + assert.match(out, /ATTR_GEN_AI_EVALUATION_NAME drift/); + }); +}); + +test('FAILS when a constant is exported but left out of the manifest', () => { + const extra = `export const ATTR_GEN_AI_REQUEST_MODEL = 'gen_ai.request.model';`; + withSandbox(semconvModule({ extraConstant: extra }), PINNED, ({ code, out }) => { + assert.equal(code, 1, out); + assert.match(out, /absent from PINNED_SEMCONV_ATTRIBUTES/); + }); +}); + +test('FAILS when the package is absent from the lockfile — an unverifiable pin is not a pin', () => { + withSandbox(semconvModule(), null, ({ code, out }) => { + assert.equal(code, 1, out); + assert.match(out, /not present in package-lock\.json/); + }); +}); + +test('FAILS on an empty manifest rather than reporting a vacuous pass', () => { + const empty = [ + `export const SEMCONV_VERSION = '${PINNED}';`, + `export const PINNED_SEMCONV_ATTRIBUTES: readonly { exportName: string; value: string; upstream: string }[] = [];`, + ].join('\n'); + withSandbox(empty, PINNED, ({ code, out }) => { + assert.equal(code, 1, out); + assert.match(out, /pinned semconv symbols/); + }); +}); diff --git a/.harness/scripts/lib/paths.mjs b/.harness/scripts/lib/paths.mjs index 86edf16dd..d448c79c4 100644 --- a/.harness/scripts/lib/paths.mjs +++ b/.harness/scripts/lib/paths.mjs @@ -49,6 +49,7 @@ export const ROOT_MARKERS = ['package.json', '.harness', 'evolith.yaml']; export const PATH_KEYS = Object.freeze({ // --- repo skeleton ------------------------------------------------------- rootPackageJson: 'package.json', + rootPackageLock: 'package-lock.json', evolithManifest: 'evolith.yaml', masterIndex: 'MASTER_INDEX.md', @@ -118,6 +119,8 @@ export const PATH_KEYS = Object.freeze({ // Formerly referenced as `packages/agent-runtime/...` (missing `src/`) in script 33. agentRuntimeAdaptersBarrel: 'src/packages/agent-runtime/src/adapters/index.ts', agentRuntimeInteractionAdapters: 'src/packages/agent-runtime/src/adapters/interaction', + // GT-587: the single declaration of the pinned OpenTelemetry semconv vocabulary. + semconvPin: 'src/packages/core-domain/src/evaluation/telemetry/semconv.ts', }); /** diff --git a/src/apps/core-api/src/app.module.ts b/src/apps/core-api/src/app.module.ts index 37f406d70..6c885b852 100644 --- a/src/apps/core-api/src/app.module.ts +++ b/src/apps/core-api/src/app.module.ts @@ -18,6 +18,7 @@ import { validateEnv } from './infrastructure/config/env.validation'; import { AuditThrottlerGuard } from './infrastructure/guards/audit-throttler.guard'; import { ApiKeyGuard } from './infrastructure/guards/api-key.guard'; import { MetricsService } from './infrastructure/metrics/metrics.service'; +import { EvaluationTelemetryService } from './infrastructure/observability/evaluation-telemetry.service'; import { CoreReferenceQueryService } from './application/services/core-reference-query.service'; import { ReferenceController } from './presentation/controllers/reference.controller'; import { CapabilitiesController } from './presentation/controllers/capabilities.controller'; @@ -110,6 +111,9 @@ import { CacheMetricsService } from './infrastructure/cache/cache-metrics.servic providers: [ HealthService, MetricsService, + // GT-587 — the standard (semconv) half of the evaluation signal; the private + // `evolith_*` series MetricsService emits stays exactly as it was. + EvaluationTelemetryService, CacheMetricsService, CoreReferenceQueryService, WorkspaceReferenceResolverService, diff --git a/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.spec.ts b/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.spec.ts new file mode 100644 index 000000000..6bfdc8917 --- /dev/null +++ b/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.spec.ts @@ -0,0 +1,61 @@ +/** + * GT-587 criterion 1, adapter half — the events actually reach a span. + * + * The mapper is unit-tested in core-domain; what is unproven there is that this + * adapter hands OTel the event NAME and the attribute map unchanged, and that it stays + * silent (rather than throwing) when tracing is off. Both were the pre-GT-587 state: + * there was no adapter at all, so `grep -rn gen_ai src` returned 0. + */ + +import { Verdict } from '@beyondnet/evolith-core-domain'; +import type { EvaluationResult } from '@beyondnet/evolith-core-domain/evaluation'; +import { EvaluationTelemetryService } from './evaluation-telemetry.service'; + +function fakeSpan() { + const events: Array<{ name: string; attributes: Record }> = []; + return { + events, + span: { addEvent: (name: string, attributes: Record) => { events.push({ name, attributes }); } } as any, + }; +} + +function result(): EvaluationResult { + return { + overallVerdict: Verdict.FAIL, + outcome: 'rejected', + results: { + compliance: { verdict: Verdict.FAIL, totalChecks: 4, passedChecks: 1, failedChecks: 3, skippedChecks: 0 }, + }, + rulesExecuted: [], + policiesApplied: [], + gaps: [], + risks: [], + missingEvidence: [], + incompleteArtifacts: [], + recommendations: [], + requiredActions: [], + confidence: 0.4, + rationale: 'three compliance checks failed', + versions: { core: '1.2.0' }, + evaluatedAt: '2026-07-30T00:00:00.000Z', + correlationId: 'corr-9', + schemaVersion: '1.0.0', + } as EvaluationResult; +} + +describe('EvaluationTelemetryService', () => { + it('adds one gen_ai.evaluation.result event per evaluated kind', () => { + const { span, events } = fakeSpan(); + const emitted = new EvaluationTelemetryService().record(result(), span); + + expect(emitted).toBe(2); // overall + compliance + expect(events.map((e) => e.name)).toEqual(['gen_ai.evaluation.result', 'gen_ai.evaluation.result']); + expect(events[0].attributes['gen_ai.evaluation.name']).toBe('evolith.overall'); + expect(events[0].attributes['gen_ai.evaluation.score.label']).toBe('FAIL'); + expect(events[1].attributes['gen_ai.evaluation.name']).toBe('evolith.compliance'); + }); + + it('is a silent no-op when nothing is tracing — emitting is a side channel, never a precondition', () => { + expect(new EvaluationTelemetryService().record(result(), undefined)).toBe(0); + }); +}); diff --git a/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.ts b/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.ts new file mode 100644 index 000000000..9d8f21165 --- /dev/null +++ b/src/apps/core-api/src/infrastructure/observability/evaluation-telemetry.service.ts @@ -0,0 +1,39 @@ +/** + * Attaches `gen_ai.evaluation.result` events to the request span (GT-587). + * + * The Core is pure and owns no span: `toGenAiEvaluationEvents` is a total function from + * an `EvaluationResult` to a list of `{name, attributes}` records, and THIS is the + * adapter that hands them to OpenTelemetry. That split is the whole reason the mapping + * is unit-testable without an SDK, and the reason rule HXA-05 (observability SDK inside + * the domain/application layer) is not tripped by the vocabulary living in core-domain. + * + * The span is the ambient one the auto-instrumented HTTP/Express layer already created, + * so the evaluation outcome lands on the same trace as the request that asked for it — + * which is what makes the signal joinable at all. When tracing is off (`OTEL_ENABLED` + * unset outside production) there is no active span and this is a no-op: emitting is a + * side channel, never a precondition of answering the request. + */ + +import { Injectable } from '@nestjs/common'; +import { trace, type Span } from '@opentelemetry/api'; +import { toGenAiEvaluationEvents } from '@beyondnet/evolith-core-domain/evaluation'; +import type { EvaluationResult } from '@beyondnet/evolith-core-domain/evaluation'; + +@Injectable() +export class EvaluationTelemetryService { + /** + * Emit one `gen_ai.evaluation.result` event per evaluated kind onto the active span. + * + * @returns how many events were emitted — 0 when no span is recording. Returned + * rather than voided so a test can distinguish "emitted nothing" from "did not run", + * the same distinction the harness's zero-coverage guardrail exists for. + */ + record(result: EvaluationResult, span: Span | undefined = trace.getActiveSpan()): number { + if (!span) return 0; + const events = toGenAiEvaluationEvents(result); + for (const event of events) { + span.addEvent(event.name, { ...event.attributes }); + } + return events.length; + } +} diff --git a/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts b/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts index 9338ec6fe..ed9aba677 100644 --- a/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts +++ b/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts @@ -34,6 +34,7 @@ import { type EvaluationOrchestratorFactory, } from '../../application/evaluation/evaluation-orchestrator.factory'; import { MetricsService } from '../../infrastructure/metrics/metrics.service'; +import { EvaluationTelemetryService } from '../../infrastructure/observability/evaluation-telemetry.service'; import { ApiEnvelopeResponse } from '../decorators/swagger-envelope.decorator'; import { createSuccessEnvelope, @@ -92,6 +93,11 @@ export class EvaluationController { @Optional() @Inject(EVALUATION_ORCHESTRATOR_FACTORY) private readonly makeOrchestrator?: EvaluationOrchestratorFactory, + // GT-587: the standard-vocabulary half of the same signal. Optional for the same + // reason `metrics` is, and LAST on purpose: several specs construct this controller + // positionally, so inserting a parameter mid-list silently shifts every argument + // after it — which is exactly how this landed as nine red tests the first time. + @Optional() private readonly evaluationTelemetry?: EvaluationTelemetryService, ) {} @Post() @@ -123,6 +129,9 @@ export class EvaluationController { const result = await this.orchestrator.evaluate(ctx); // GT-542: emit the evaluation verdict + latency signal. this.metrics?.recordGateEvaluation('evaluate', String(result.overallVerdict), phase, body.tenant?.tenantId, (Date.now() - start) / 1000); + // GT-587: the same outcome under the OpenTelemetry GenAI vocabulary, on the + // request's own span. Additive — the private series above is untouched. + this.evaluationTelemetry?.record(result); // GT-411: Return pre-built ADR-0073 envelope with canonical command name. return createSuccessEnvelope(result, { command: 'evolith evaluate', @@ -248,6 +257,10 @@ export class EvaluationController { const result = await orchestrator.evaluate(ctx); // GT-542: emit the inline evaluation verdict + latency signal. this.metrics?.recordGateEvaluation('evaluate', String(result.overallVerdict), phase, body.tenant?.tenantId, (Date.now() - start) / 1000); + // GT-587: both branches return the SAME EvaluationResult (GT-573), so both must + // emit the SAME standard events — an inline evaluation that stayed silent would + // make the telemetry depend on which transport the caller happened to use. + this.evaluationTelemetry?.record(result); // GT-573: the canonical EvaluationResult, in the same ADR-0073 envelope the // workspaceRef branch returns. return createSuccessEnvelope(result, { diff --git a/src/packages/core-domain/src/evaluation/index.ts b/src/packages/core-domain/src/evaluation/index.ts index 5640ba3a4..9a4fb2b78 100644 --- a/src/packages/core-domain/src/evaluation/index.ts +++ b/src/packages/core-domain/src/evaluation/index.ts @@ -20,3 +20,4 @@ export * from './canonical-result.mapper'; export * from './kind-selective-pipeline'; export * from './evaluation-orchestrator.service'; export * from './kind-evaluators'; +export * from './telemetry'; diff --git a/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.spec.ts b/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.spec.ts new file mode 100644 index 000000000..f3538fcd0 --- /dev/null +++ b/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.spec.ts @@ -0,0 +1,177 @@ +/** + * GT-587 criterion 1 — an evaluation result emits `gen_ai.evaluation.result` under + * the PINNED semconv revision. + * + * Every assertion here reads a literal wire name rather than the exported constant + * where the point of the test is the NAME. Asserting `attrs[ATTR_GEN_AI_EVALUATION_NAME]` + * would pass just as happily if the constant were renamed to `evolith.eval.name` — + * which is the exact regression this gap is about. + */ + +import { Verdict } from '../../domain/verdict/verdict'; +import type { EvaluationResult } from '../contracts/evaluation-result'; +import { toGenAiEvaluationEvents } from './gen-ai-evaluation'; +import { PINNED_SEMCONV_ATTRIBUTES, SEMCONV_VERSION } from './semconv'; + +function result(overrides: Partial = {}): EvaluationResult { + return { + overallVerdict: Verdict.PASS, + outcome: 'approved', + results: {}, + rulesExecuted: [], + policiesApplied: [], + gaps: [], + risks: [], + missingEvidence: [], + incompleteArtifacts: [], + recommendations: [], + requiredActions: [], + confidence: 0.8, + rationale: 'all required criteria met', + versions: { core: '1.2.0' }, + evaluatedAt: '2026-07-30T00:00:00.000Z', + correlationId: 'corr-1', + schemaVersion: '1.0.0', + ...overrides, + } as EvaluationResult; +} + +function byName(events: ReturnType, evaluator: string) { + return events.find((e) => e.attributes['gen_ai.evaluation.name'] === evaluator); +} + +describe('toGenAiEvaluationEvents — standard vocabulary', () => { + it('emits the standard event name and the four standard attributes', () => { + const [event] = toGenAiEvaluationEvents(result()); + + expect(event.name).toBe('gen_ai.evaluation.result'); + expect(event.attributes['gen_ai.evaluation.name']).toBe('evolith.overall'); + expect(event.attributes['gen_ai.evaluation.score.label']).toBe('PASS'); + expect(event.attributes['gen_ai.evaluation.score.value']).toBe(0.8); + expect(event.attributes['gen_ai.evaluation.explanation']).toBe('all required criteria met'); + }); + + it('keeps the private evolith.* names ALONGSIDE, never instead of', () => { + const [event] = toGenAiEvaluationEvents(result({ outcome: 'conditional' })); + + expect(event.attributes['evolith.outcome']).toBe('conditional'); + expect(event.attributes['evolith.evaluation.kind']).toBe('overall'); + expect(event.attributes['evolith.core.version']).toBe('1.2.0'); + expect(event.attributes['evolith.correlation_id']).toBe('corr-1'); + }); + + it('stamps the pinned semconv revision on every event', () => { + for (const event of toGenAiEvaluationEvents(result())) { + expect(event.attributes['evolith.semconv.version']).toBe(SEMCONV_VERSION); + } + }); + + it('always emits the overall event, even when nothing was evaluated', () => { + const events = toGenAiEvaluationEvents(result({ results: {} })); + expect(events).toHaveLength(1); + expect(byName(events, 'evolith.overall')).toBeDefined(); + }); +}); + +describe('toGenAiEvaluationEvents — one event per sub-result kind', () => { + it('folds a list of gates into the WORST verdict and a pass ratio', () => { + const events = toGenAiEvaluationEvents( + result({ + results: { + gate: [ + { gateId: 'g1', verdict: Verdict.PASS, artifactResults: [], risks: [], gaps: [], requiredActions: [] }, + { gateId: 'g2', verdict: Verdict.FAIL, artifactResults: [], risks: [], gaps: [], requiredActions: [] }, + { gateId: 'g3', verdict: Verdict.PASS, artifactResults: [], risks: [], gaps: [], requiredActions: [] }, + ], + }, + } as Partial), + ); + + const gate = byName(events, 'evolith.gate'); + expect(gate?.attributes['gen_ai.evaluation.score.label']).toBe('FAIL'); + expect(gate?.attributes['gen_ai.evaluation.score.value']).toBeCloseTo(2 / 3); + }); + + it('normalises maturity and completeness onto the same 0..1 axis as confidence', () => { + const events = toGenAiEvaluationEvents( + result({ + results: { + design: { + verdict: Verdict.PASS, + technicalMaturity: 62, + perConcernMaturity: [], + artifactStatus: [], + missingArtifacts: [], + deviationsRequiringAdr: [], + gaps: [], + recommendations: [], + }, + phaseArtifacts: { + verdict: Verdict.PASS, + phase: 'construction', + completeness: 40, + requiredArtifacts: [], + presentArtifacts: [], + missingArtifacts: [], + conditionalArtifacts: [], + gaps: [], + recommendations: [], + }, + }, + } as Partial), + ); + + expect(byName(events, 'evolith.design')?.attributes['gen_ai.evaluation.score.value']).toBeCloseTo(0.62); + expect(byName(events, 'evolith.phase-artifacts.construction')?.attributes['gen_ai.evaluation.score.value']).toBeCloseTo(0.4); + }); + + it('OMITS the score attribute for kinds with no honest numeric score', () => { + const events = toGenAiEvaluationEvents( + result({ + results: { + architecture: { verdict: Verdict.FAIL, risks: [], gaps: [], recommendations: [] }, + }, + } as Partial), + ); + + const architecture = byName(events, 'evolith.architecture'); + expect(architecture?.attributes['gen_ai.evaluation.score.label']).toBe('FAIL'); + // An invented 1.0 would be worse than nothing — absence is the signal. + expect(architecture?.attributes).not.toHaveProperty('gen_ai.evaluation.score.value'); + }); + + it('derives a compliance score from the check counts when none is supplied', () => { + const events = toGenAiEvaluationEvents( + result({ + results: { + compliance: { verdict: Verdict.FAIL, totalChecks: 8, passedChecks: 6, failedChecks: 2, skippedChecks: 0 }, + }, + } as Partial), + ); + + const compliance = byName(events, 'evolith.compliance'); + expect(compliance?.attributes['gen_ai.evaluation.score.value']).toBeCloseTo(0.75); + expect(compliance?.attributes['gen_ai.evaluation.explanation']).toContain('6/8 checks passed'); + }); +}); + +describe('the pinned manifest covers what is emitted', () => { + it('lists every standard attribute the mapper can emit', () => { + const pinned = new Set(PINNED_SEMCONV_ATTRIBUTES.map((a) => a.value)); + const events = toGenAiEvaluationEvents( + result({ + results: { + compliance: { verdict: Verdict.PASS, totalChecks: 1, passedChecks: 1, failedChecks: 0, skippedChecks: 0 }, + }, + } as Partial), + ); + + for (const event of events) { + expect(pinned).toContain(event.name); + for (const key of Object.keys(event.attributes)) { + if (key.startsWith('evolith.')) continue; + expect(pinned).toContain(key); + } + } + }); +}); diff --git a/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.ts b/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.ts new file mode 100644 index 000000000..543eb3891 --- /dev/null +++ b/src/packages/core-domain/src/evaluation/telemetry/gen-ai-evaluation.ts @@ -0,0 +1,186 @@ +/** + * `EvaluationResult` → OpenTelemetry `gen_ai.evaluation.result` events (GT-587). + * + * ## The shape match + * + * An Evolith evaluation is, in OpenTelemetry's GenAI vocabulary, exactly an + * *evaluation result*: a named evaluator produced a categorical outcome (the canonical + * {@link Verdict}), sometimes a numeric score, sometimes an explanation. That is the + * whole of `gen_ai.evaluation.result`, which is why this maps cleanly rather than + * approximately — the same claim ADR-0111 makes about a quality signal. + * + * ## What this deliberately does NOT do + * + * It emits nothing. It has no OTel import and no I/O: a pure function from a result to + * a list of `{name, attributes}` records. The Core stays pure (ADR-0101) and rule + * HXA-05 stays satisfied; the adapters that own a span attach these. + * + * ## Naming choices worth defending + * + * - `gen_ai.evaluation.name` carries a NAMESPACED value (`evolith.architecture`, not + * `architecture`). The attribute key is the standard one — that is what makes the + * signal joinable — but a bare `architecture` would collide with any other evaluator + * a customer runs. Namespacing the value, not the key, is the correct half to own. + * - `gen_ai.evaluation.score.value` is normalised to 0..1 across every kind + * (`technicalMaturity`/100, `completeness`/100, pass ratios), so one dashboard panel + * can plot all of them. Kinds with no honest numeric score omit the attribute rather + * than inventing one — an absent score is information; a fabricated 1.0 is not. + * - `gen_ai.evaluation.explanation` is set only where the result actually carries text. + * - The `evolith.*` attributes ride ALONGSIDE, never instead of: they are already being + * collected, and they carry what semconv has no field for (the governance `outcome`, + * the core version, the correlation id). + */ + +import { Verdict } from '../../domain/verdict/verdict'; +import type { EvaluationResult } from '../contracts/evaluation-result'; +import { + ATTR_GEN_AI_EVALUATION_EXPLANATION, + ATTR_GEN_AI_EVALUATION_NAME, + ATTR_GEN_AI_EVALUATION_SCORE_LABEL, + ATTR_GEN_AI_EVALUATION_SCORE_VALUE, + EVENT_GEN_AI_EVALUATION_RESULT, + SEMCONV_VERSION, +} from './semconv'; + +/** Attribute values OTel accepts on an event without further encoding. */ +export type TelemetryAttributeValue = string | number | boolean; + +/** A single `gen_ai.evaluation.result` event, ready for `span.addEvent`. */ +export interface GenAiEvaluationEvent { + /** Always {@link EVENT_GEN_AI_EVALUATION_RESULT}. */ + readonly name: string; + readonly attributes: Readonly>; +} + +/** Namespace prefix for the `gen_ai.evaluation.name` VALUE (see file header). */ +const EVALUATOR_NAMESPACE = 'evolith'; + +/** + * Severity ranking used to fold a list of sub-results into one label. FAIL dominates + * everything; a WAIVE is louder than a PASS because it records a human override; SKIP + * is the quietest because it asserts non-applicability. + */ +const VERDICT_SEVERITY: Readonly> = { + [Verdict.FAIL]: 3, + [Verdict.WAIVE]: 2, + [Verdict.PASS]: 1, + [Verdict.SKIP]: 0, +}; + +function worstVerdict(verdicts: readonly Verdict[]): Verdict | undefined { + let worst: Verdict | undefined; + for (const v of verdicts) { + if (worst === undefined || (VERDICT_SEVERITY[v] ?? 0) > (VERDICT_SEVERITY[worst] ?? 0)) worst = v; + } + return worst; +} + +/** Fraction of entries that passed, or undefined for an empty list (no honest ratio). */ +function passRatio(verdicts: readonly Verdict[]): number | undefined { + if (verdicts.length === 0) return undefined; + return verdicts.filter((v) => v === Verdict.PASS).length / verdicts.length; +} + +/** Clamp to the 0..1 range the score attribute is normalised to. */ +function normalise(value: number, scale: number): number | undefined { + if (!Number.isFinite(value)) return undefined; + return Math.min(1, Math.max(0, value / scale)); +} + +interface EventDraft { + readonly kind: string; + readonly verdict?: Verdict; + readonly score?: number; + readonly explanation?: string; +} + +function draftsFor(result: EvaluationResult): EventDraft[] { + const drafts: EventDraft[] = [ + { + kind: 'overall', + verdict: result.overallVerdict, + score: normalise(result.confidence, 1), + explanation: result.rationale || undefined, + }, + ]; + + const r = result.results; + + if (r.gate?.length) { + const verdicts = r.gate.map((g) => g.verdict); + drafts.push({ kind: 'gate', verdict: worstVerdict(verdicts), score: passRatio(verdicts) }); + } + if (r.artifact?.length) { + const verdicts = r.artifact.map((a) => a.verdict); + drafts.push({ kind: 'artifact', verdict: worstVerdict(verdicts), score: passRatio(verdicts) }); + } + if (r.evidence?.length) { + const verdicts = r.evidence.map((e) => e.verdict); + drafts.push({ kind: 'evidence', verdict: worstVerdict(verdicts), score: passRatio(verdicts) }); + } + if (r.checkpoint?.length) { + const verdicts = r.checkpoint.map((c) => c.verdict); + drafts.push({ kind: 'checkpoint', verdict: worstVerdict(verdicts), score: passRatio(verdicts) }); + } + if (r.architecture) { + drafts.push({ kind: 'architecture', verdict: r.architecture.verdict }); + } + if (r.blueprint) { + drafts.push({ kind: 'blueprint', verdict: r.blueprint.verdict }); + } + if (r.topology) { + drafts.push({ kind: 'topology', verdict: r.topology.verdict }); + } + if (r.deployment) { + drafts.push({ kind: 'deployment', verdict: r.deployment.verdict }); + } + if (r.compliance) { + const c = r.compliance; + const score = c.score !== undefined ? normalise(c.score, 1) : c.totalChecks > 0 ? c.passedChecks / c.totalChecks : undefined; + drafts.push({ + kind: 'compliance', + verdict: c.verdict, + score, + explanation: `${c.passedChecks}/${c.totalChecks} checks passed (${c.failedChecks} failed, ${c.skippedChecks} skipped)`, + }); + } + if (r.design) { + // ADR-0104: the design evaluator is ADVISORY — its primary output is maturity, + // not the verdict, so the score is the datum worth plotting. + drafts.push({ kind: 'design', verdict: r.design.verdict, score: normalise(r.design.technicalMaturity, 100) }); + } + if (r.phaseArtifacts) { + drafts.push({ + kind: `phase-artifacts.${r.phaseArtifacts.phase}`, + verdict: r.phaseArtifacts.verdict, + score: normalise(r.phaseArtifacts.completeness, 100), + }); + } + + return drafts; +} + +/** + * Map an {@link EvaluationResult} to `gen_ai.evaluation.result` events — one for the + * aggregate verdict plus one per sub-result kind actually present. A result with no + * sub-results still yields the `overall` event, so a consumer never has to distinguish + * "not emitted" from "nothing evaluated". + */ +export function toGenAiEvaluationEvents(result: EvaluationResult): GenAiEvaluationEvent[] { + return draftsFor(result).map((draft) => { + const attributes: Record = { + [ATTR_GEN_AI_EVALUATION_NAME]: `${EVALUATOR_NAMESPACE}.${draft.kind}`, + // Private names kept ALONGSIDE the standard ones — see the file header. + 'evolith.evaluation.kind': draft.kind, + 'evolith.outcome': result.outcome, + 'evolith.semconv.version': SEMCONV_VERSION, + 'evolith.core.version': result.versions.core, + }; + if (draft.verdict !== undefined) attributes[ATTR_GEN_AI_EVALUATION_SCORE_LABEL] = draft.verdict; + if (draft.score !== undefined) attributes[ATTR_GEN_AI_EVALUATION_SCORE_VALUE] = draft.score; + if (draft.explanation) attributes[ATTR_GEN_AI_EVALUATION_EXPLANATION] = draft.explanation; + if (result.correlationId) attributes['evolith.correlation_id'] = result.correlationId; + + return { name: EVENT_GEN_AI_EVALUATION_RESULT, attributes }; + }); +} diff --git a/src/packages/core-domain/src/evaluation/telemetry/index.ts b/src/packages/core-domain/src/evaluation/telemetry/index.ts new file mode 100644 index 000000000..c141bed5c --- /dev/null +++ b/src/packages/core-domain/src/evaluation/telemetry/index.ts @@ -0,0 +1,3 @@ +/** Standard telemetry vocabulary for evaluation results (GT-587). Pure: no OTel import. */ +export * from './semconv'; +export * from './gen-ai-evaluation'; diff --git a/src/packages/core-domain/src/evaluation/telemetry/semconv.ts b/src/packages/core-domain/src/evaluation/telemetry/semconv.ts new file mode 100644 index 000000000..8c8a1cb2b --- /dev/null +++ b/src/packages/core-domain/src/evaluation/telemetry/semconv.ts @@ -0,0 +1,121 @@ +/** + * Pinned OpenTelemetry semantic-convention vocabulary (GT-587). + * + * ## Why a pin, and why here + * + * Evolith already emits telemetry — `evolith_*` Prometheus series (GT-546) and + * `evolith.*` span attributes — but every one of those names is PRIVATE. A private + * name joins with nothing a customer already collects, and telemetry is not + * backfillable: a run recorded under `evolith.tenant` yesterday cannot be re-emitted + * as `gen_ai.*` today. So the vocabulary is the thing worth fixing early, and the + * fix is ADDITIVE — the semconv names are emitted ALONGSIDE the `evolith.*` ones, + * never instead of them, because the existing names are already being scraped. + * + * The OpenTelemetry GenAI semantic conventions registry is still **Development** + * status: attribute names in it may be renamed or removed between releases. Emitting + * against a moving registry without recording WHICH revision was emitted produces + * exactly the problem this file exists to avoid — a second private vocabulary, one + * that merely looks standard. Hence {@link SEMCONV_VERSION}: the emitted names are + * the names of one stated revision, and `.harness/scripts/ci/51-validate-semconv-pin.mjs` + * fails when the repository's resolved `@opentelemetry/semantic-conventions` moves + * off it or when any pinned literal stops matching its upstream constant. + * + * ## Why literals rather than importing the package + * + * `core-domain` is the pure Core. Rule HXA-05 — which this repository enforces on + * the workspaces it evaluates — flags an observability SDK imported into the domain + * or application layer, and `opentelemetry-tracker-adapter.ts` already duck-types the + * tracer interface for the same reason. This module therefore has NO imports at all: + * it is a vocabulary, not a client. The adapters that actually own a span + * (`core-api`, `mcp-server`) import these constants from here, so there is exactly + * one place that states which revision Evolith speaks. + * + * @see https://opentelemetry.io/docs/specs/semconv/gen-ai/ + * @see https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-events/ + */ + +/** + * The `@opentelemetry/semantic-conventions` revision every literal below is taken + * from. Bumping this WITHOUT re-verifying the literals is the failure mode the drift + * check exists to catch — the check compares this against the version resolved in + * `package-lock.json` and compares each literal against the package's own export. + */ +export const SEMCONV_VERSION = '1.41.1'; + +/** + * Event name for a single evaluation outcome. Upstream export: + * `EVENT_GEN_AI_EVALUATION_RESULT`. + */ +export const EVENT_GEN_AI_EVALUATION_RESULT = 'gen_ai.evaluation.result'; + +/** Name of the evaluator that produced the result. Upstream: `ATTR_GEN_AI_EVALUATION_NAME`. */ +export const ATTR_GEN_AI_EVALUATION_NAME = 'gen_ai.evaluation.name'; + +/** Numeric score, when the evaluator produces one. Upstream: `ATTR_GEN_AI_EVALUATION_SCORE_VALUE`. */ +export const ATTR_GEN_AI_EVALUATION_SCORE_VALUE = 'gen_ai.evaluation.score.value'; + +/** Categorical outcome (here: the canonical `Verdict`). Upstream: `ATTR_GEN_AI_EVALUATION_SCORE_LABEL`. */ +export const ATTR_GEN_AI_EVALUATION_SCORE_LABEL = 'gen_ai.evaluation.score.label'; + +/** Free-text rationale for the score. Upstream: `ATTR_GEN_AI_EVALUATION_EXPLANATION`. */ +export const ATTR_GEN_AI_EVALUATION_EXPLANATION = 'gen_ai.evaluation.explanation'; + +/** MCP JSON-RPC method the span covers. Upstream: `ATTR_MCP_METHOD_NAME`. */ +export const ATTR_MCP_METHOD_NAME = 'mcp.method.name'; + +/** MCP session identifier, when the transport has one. Upstream: `ATTR_MCP_SESSION_ID`. */ +export const ATTR_MCP_SESSION_ID = 'mcp.session.id'; + +/** Negotiated MCP protocol revision. Upstream: `ATTR_MCP_PROTOCOL_VERSION`. */ +export const ATTR_MCP_PROTOCOL_VERSION = 'mcp.protocol.version'; + +/** `tools/call` method value. Upstream: `MCP_METHOD_NAME_VALUE_TOOLS_CALL`. */ +export const MCP_METHOD_NAME_VALUE_TOOLS_CALL = 'tools/call'; + +/** + * Tool name on a `tools/call` span. The MCP semconv registry defines + * `mcp.tool.name`, but `@opentelemetry/semantic-conventions@1.41.1` does not yet + * export a constant for it — see {@link PINNED_SEMCONV_ATTRIBUTES}, where it is + * declared `registry-only` so the drift check watches for the day it appears + * upstream under a different spelling instead of silently disagreeing with it. + */ +export const ATTR_MCP_TOOL_NAME = 'mcp.tool.name'; + +/** + * How a pinned literal relates to the installed `@opentelemetry/semantic-conventions`. + * + * - `exported` — the package exports `exportName`; its value MUST equal `value`. + * The drift check fails if the export vanishes or its value changes. + * - `registry-only` — the convention exists in the upstream registry but the package + * does not export it at {@link SEMCONV_VERSION}. The drift check fails if it LATER + * appears with a different value, which is precisely the moment a locally-declared + * literal would otherwise start lying. + */ +export type SemconvUpstreamStatus = 'exported' | 'registry-only'; + +export interface PinnedSemconvSymbol { + /** The constant's name in `@opentelemetry/semantic-conventions`. */ + readonly exportName: string; + /** The wire name Evolith emits. */ + readonly value: string; + readonly upstream: SemconvUpstreamStatus; +} + +/** + * The manifest the drift check reads. Every literal above appears here exactly once; + * a constant added without a manifest entry is invisible to the check, so the check + * also asserts that this list covers every `ATTR_`/`EVENT_`/`MCP_…_VALUE_` export of + * this module. + */ +export const PINNED_SEMCONV_ATTRIBUTES: readonly PinnedSemconvSymbol[] = [ + { exportName: 'EVENT_GEN_AI_EVALUATION_RESULT', value: EVENT_GEN_AI_EVALUATION_RESULT, upstream: 'exported' }, + { exportName: 'ATTR_GEN_AI_EVALUATION_NAME', value: ATTR_GEN_AI_EVALUATION_NAME, upstream: 'exported' }, + { exportName: 'ATTR_GEN_AI_EVALUATION_SCORE_VALUE', value: ATTR_GEN_AI_EVALUATION_SCORE_VALUE, upstream: 'exported' }, + { exportName: 'ATTR_GEN_AI_EVALUATION_SCORE_LABEL', value: ATTR_GEN_AI_EVALUATION_SCORE_LABEL, upstream: 'exported' }, + { exportName: 'ATTR_GEN_AI_EVALUATION_EXPLANATION', value: ATTR_GEN_AI_EVALUATION_EXPLANATION, upstream: 'exported' }, + { exportName: 'ATTR_MCP_METHOD_NAME', value: ATTR_MCP_METHOD_NAME, upstream: 'exported' }, + { exportName: 'ATTR_MCP_SESSION_ID', value: ATTR_MCP_SESSION_ID, upstream: 'exported' }, + { exportName: 'ATTR_MCP_PROTOCOL_VERSION', value: ATTR_MCP_PROTOCOL_VERSION, upstream: 'exported' }, + { exportName: 'MCP_METHOD_NAME_VALUE_TOOLS_CALL', value: MCP_METHOD_NAME_VALUE_TOOLS_CALL, upstream: 'exported' }, + { exportName: 'ATTR_MCP_TOOL_NAME', value: ATTR_MCP_TOOL_NAME, upstream: 'registry-only' }, +]; diff --git a/src/packages/mcp-server/src/mcp/mcp-semconv-dispatch.spec.ts b/src/packages/mcp-server/src/mcp/mcp-semconv-dispatch.spec.ts new file mode 100644 index 000000000..a0f7a61f5 --- /dev/null +++ b/src/packages/mcp-server/src/mcp/mcp-semconv-dispatch.spec.ts @@ -0,0 +1,186 @@ +/** + * GT-587 criterion 2 — MCP spans carry `mcp.*` attributes and take their trace + * context from `_meta`. + * + * Both halves fail against the pre-GT-587 dispatch: it set no `mcp.*` attribute at + * all, and it read `traceparent` only out of the tool's own ARGUMENTS, so a `_meta` + * carrier produced an unlinked root span. + * + * The tracer here is a hand-rolled double rather than the OTel SDK on purpose: the + * assertion is about which attribute NAMES and which carrier the dispatcher uses, and + * an in-memory span exporter would let a wrong name pass as long as some span existed. + */ + +import { ToolDispatchService, traceCarrierFromMeta } from './mcp-tool-dispatch'; +import { MetricsService } from './metrics.service'; +import { ToolRegistryService } from './tool-registry.service'; +import { AbacEvaluator } from './abac-evaluator'; +import { McpTool } from './tool.interface'; +import { Logger } from '@nestjs/common'; + +class MockAbacEvaluator extends AbacEvaluator { + override evaluateNative() { + return { allowed: true, violations: [] }; + } + override async evaluateOpa() { + return { allowed: true, violations: [] }; + } +} + +interface RecordedSpan { + name: string; + attributes: Record; + /** Whatever context object was handed to `startSpan` as the parent. */ + parentContext: unknown; +} + +function recordingTracer(): { tracer: any; spans: RecordedSpan[] } { + const spans: RecordedSpan[] = []; + const tracer = { + startSpan(name: string, options: any, ctx: unknown) { + const recorded: RecordedSpan = { name, attributes: { ...(options?.attributes ?? {}) }, parentContext: ctx }; + spans.push(recorded); + return { + setStatus() {}, + setAttribute(key: string, value: unknown) { + recorded.attributes[key] = value; + }, + recordException() {}, + end() {}, + spanContext: () => ({ traceId: '0'.repeat(32), spanId: '0'.repeat(16), traceFlags: 0 }), + }; + }, + }; + return { tracer, spans }; +} + +function dispatcher(tool: McpTool) { + const registry = new ToolRegistryService([tool]); + const { tracer, spans } = recordingTracer(); + const service = new ToolDispatchService( + registry, + new MetricsService(), + new MockAbacEvaluator(), + new Logger('test'), + tracer, + ); + return { service, spans }; +} + +const OK_TOOL: McpTool = { + schema: { name: 'evolith-noop', description: 'd', inputSchema: { type: 'object', properties: {} } }, + execute: async () => ({ ok: true }), +}; + +/** A syntactically valid W3C traceparent with a non-zero trace id. */ +const TRACEPARENT = '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01'; + +describe('traceCarrierFromMeta', () => { + it('lifts the three W3C keys out of _meta', () => { + expect(traceCarrierFromMeta({ traceparent: TRACEPARENT, tracestate: 'a=1', baggage: 'k=v' })).toEqual({ + traceparent: TRACEPARENT, + tracestate: 'a=1', + baggage: 'k=v', + }); + }); + + it('drops everything else — _meta is an open map and must not become a carrier', () => { + const carrier = traceCarrierFromMeta({ + traceparent: TRACEPARENT, + 'progressToken': 7, + 'x-injected': 'nope', + tracestate: 42, // wrong type + }); + expect(carrier).toEqual({ traceparent: TRACEPARENT }); + }); + + it('is empty (not undefined) for an absent _meta', () => { + expect(traceCarrierFromMeta(undefined)).toEqual({}); + }); +}); + +describe('tools/call span — standard MCP attributes', () => { + it('carries mcp.method.name and mcp.tool.name', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', {}); + + expect(spans).toHaveLength(1); + expect(spans[0].attributes['mcp.method.name']).toBe('tools/call'); + expect(spans[0].attributes['mcp.tool.name']).toBe('evolith-noop'); + }); + + it('carries mcp.session.id when the transport supplied one', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', {}, { sessionId: 'sess-42' }); + + expect(spans[0].attributes['mcp.session.id']).toBe('sess-42'); + }); + + it('OMITS mcp.session.id for a transport without sessions (stdio)', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', {}); + + expect(spans[0].attributes).not.toHaveProperty('mcp.session.id'); + }); + + it('keeps the private evolith.* attributes alongside the standard ones', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', { tenant: 't1', initiative: 'i1', phase: 'design' }); + + expect(spans[0].attributes['evolith.tenant']).toBe('t1'); + expect(spans[0].attributes['evolith.initiative']).toBe('i1'); + expect(spans[0].attributes['evolith.phase']).toBe('design'); + expect(spans[0].attributes['tool.name']).toBe('evolith-noop'); + }); +}); + +describe('trace-context propagation', () => { + // `propagation.extract` is a no-op until a propagator is registered. The NodeSDK + // registers the W3C one at startup (`tracing.ts`); this reproduces that, because + // without it every assertion below would report "no parent" and pass for the wrong + // reason on a dispatcher that reads nothing at all. + beforeAll(() => { + const { propagation } = require('@opentelemetry/api'); + const { W3CTraceContextPropagator } = require('@opentelemetry/core'); + propagation.setGlobalPropagator(new W3CTraceContextPropagator()); + }); + afterAll(() => { + const { propagation } = require('@opentelemetry/api'); + propagation.disable(); + }); + + /** Read the remote span context the dispatcher extracted into the parent context. */ + function extractedTraceId(parentContext: unknown): string | undefined { + const { trace } = require('@opentelemetry/api'); + return trace.getSpanContext(parentContext)?.traceId; + } + + it('adopts the trace context carried in _meta', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', {}, { meta: { traceparent: TRACEPARENT } }); + + expect(extractedTraceId(spans[0].parentContext)).toBe('4bf92f3577b34da6a3ce929d0e0e4736'); + }); + + it('still honours the legacy traceparent ARGUMENT when _meta carries none', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', { traceparent: TRACEPARENT }); + + expect(extractedTraceId(spans[0].parentContext)).toBe('4bf92f3577b34da6a3ce929d0e0e4736'); + }); + + it('prefers _meta over the legacy argument when both are present', async () => { + const other = '00-11111111111111111111111111111111-2222222222222222-01'; + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', { traceparent: other }, { meta: { traceparent: TRACEPARENT } }); + + expect(extractedTraceId(spans[0].parentContext)).toBe('4bf92f3577b34da6a3ce929d0e0e4736'); + }); + + it('starts a root span when neither source carries context', async () => { + const { service, spans } = dispatcher(OK_TOOL); + await service.callTool('evolith-noop', {}); + + expect(extractedTraceId(spans[0].parentContext)).toBeUndefined(); + }); +}); diff --git a/src/packages/mcp-server/src/mcp/mcp-server.service.ts b/src/packages/mcp-server/src/mcp/mcp-server.service.ts index 66d2eadb2..452afe330 100644 --- a/src/packages/mcp-server/src/mcp/mcp-server.service.ts +++ b/src/packages/mcp-server/src/mcp/mcp-server.service.ts @@ -28,8 +28,12 @@ import { authenticateHttpRequest } from './mcp-server-auth'; import { createLocalSessionContext } from './mcp-auth-contexts'; import { evaluateStdioCredentialPolicy, StdioCredentialError } from './stdio-credential-policy'; import { loadOAuthConfig, createJwksResolver, type OAuthConfig, type JwksKeyResolver } from './oauth-resource-server'; -import { handleCallTool, handleListTools, ToolCallResult, redactArgs } from './mcp-tool-dispatch'; +import { handleCallTool, handleListTools, ToolCallOptions, ToolCallResult, redactArgs } from './mcp-tool-dispatch'; import { McpCacheService } from './mcp-cache.service'; +import { + ATTR_MCP_PROTOCOL_VERSION, + ATTR_MCP_SESSION_ID, +} from '@beyondnet/evolith-core-domain/evaluation'; export { McpUserContext, mcpContextStorage } from './mcp-user-context'; export { ToolCallResult } from './mcp-tool-dispatch'; @@ -160,11 +164,19 @@ export class McpServerService { return result; } - async handleCallTool(name: string, args: Record = {}): Promise { - return this.withTransportContext(() => this.callToolInContext(name, args)); + async handleCallTool( + name: string, + args: Record = {}, + options: ToolCallOptions = {}, + ): Promise { + return this.withTransportContext(() => this.callToolInContext(name, args, options)); } - private async callToolInContext(name: string, args: Record = {}): Promise { + private async callToolInContext( + name: string, + args: Record = {}, + options: ToolCallOptions = {}, + ): Promise { const correlationId = (args.correlationId as string) || generateCorrelationId(); const startTime = Date.now(); @@ -174,7 +186,7 @@ export class McpServerService { abacEvaluator: this.abacEvaluator, logger: this.logger, tracer, - }); + }, options); if (this.auditLogger) { const durationMs = Date.now() - startTime; @@ -253,11 +265,17 @@ export class McpServerService { const server = new Server({ name: SERVER_NAME, version: SERVER_VERSION }, { capabilities }); server.setRequestHandler(ListToolsRequestSchema, async () => this.handleListTools()); - server.setRequestHandler(CallToolRequestSchema, async (request) => { + // GT-587: `_meta` and `extra.sessionId` used to be destructured away here, which is + // why trace context had to travel as a tool ARGUMENT. Both now cross the boundary: + // `_meta` carries the W3C trace context a conformant client sends, `sessionId` is + // the `mcp.session.id` attribute. Neither reaches the tool — dispatch consumes them. + server.setRequestHandler(CallToolRequestSchema, async (request, extra) => { const { name, arguments: args } = request.params; - return this.handleCallTool(name, (args ?? {}) as Record) as unknown as Promise< - Record - >; + const meta = (request.params._meta ?? extra?._meta) as Record | undefined; + return this.handleCallTool(name, (args ?? {}) as Record, { + meta, + sessionId: extra?.sessionId, + }) as unknown as Promise>; }); if (this.resources) { @@ -518,11 +536,21 @@ export class McpServerService { const headerCorrelationId = (req.headers['x-correlation-id'] as string) || generateCorrelationId(); + // GT-587: the two `mcp.*` facts the Streamable HTTP transport actually carries + // as headers. They are read here rather than invented at the tool span, because + // the SDK does not expose the negotiated protocol version to a request handler — + // the header is the only honest source, and an attribute nobody can source is + // better absent than fabricated. + const httpSessionId = req.headers['mcp-session-id'] as string | undefined; + const httpProtocolVersion = req.headers['mcp-protocol-version'] as string | undefined; + const span = tracer.startSpan('mcp.http.request', { attributes: { 'http.method': req.method || 'UNKNOWN', 'http.url': url.pathname, 'correlation.id': headerCorrelationId, + ...(httpSessionId ? { [ATTR_MCP_SESSION_ID]: httpSessionId } : {}), + ...(httpProtocolVersion ? { [ATTR_MCP_PROTOCOL_VERSION]: httpProtocolVersion } : {}), }, }); diff --git a/src/packages/mcp-server/src/mcp/mcp-tool-dispatch.ts b/src/packages/mcp-server/src/mcp/mcp-tool-dispatch.ts index ecff98b50..2866dbc6a 100644 --- a/src/packages/mcp-server/src/mcp/mcp-tool-dispatch.ts +++ b/src/packages/mcp-server/src/mcp/mcp-tool-dispatch.ts @@ -8,6 +8,12 @@ import { AbacEvaluator, AbacInput } from './abac-evaluator'; import { ErrorCodes } from '../common/errors'; import { failure, generateCorrelationId, success, toErrorEnvelope } from '../common/envelopes'; import { runWithContext } from '@beyondnet/evolith-core-domain/common/request-context'; +import { + ATTR_MCP_METHOD_NAME, + ATTR_MCP_SESSION_ID, + ATTR_MCP_TOOL_NAME, + MCP_METHOD_NAME_VALUE_TOOLS_CALL, +} from '@beyondnet/evolith-core-domain/evaluation'; import { mcpContextStorage, McpUserContext } from './mcp-user-context'; import { describeAbacDenial } from './abac-denial-remediation'; import { @@ -50,6 +56,42 @@ export interface ToolCallResult { isError?: boolean; } +/** + * Per-call transport facts a tool's ARGUMENTS cannot carry (GT-587). + * + * `_meta` is the MCP request's out-of-band metadata field, and it is where the protocol + * documents trace-context propagation (`traceparent`/`tracestate`/`baggage`, SEP-414). + * That distinction matters: before this, the only way to join an MCP tool span to the + * caller's trace was a `traceparent` smuggled into the tool's own arguments — which + * every tool schema had to tolerate, which showed up in audit logs as if it were input, + * and which no standard client would ever send. Reading `_meta` means a conformant + * client's trace context works with no cooperation from the tool. + */ +export interface ToolCallOptions { + /** `request.params._meta` verbatim. Unknown keys are ignored. */ + readonly meta?: Record; + /** Transport session id (`RequestHandlerExtra.sessionId`), when the transport has one. */ + readonly sessionId?: string; +} + +/** + * W3C trace-context keys, lower-cased as the propagator expects them. Only these three + * are lifted out of `_meta`: `_meta` is an open map and copying it wholesale into a + * propagation carrier would let a caller inject arbitrary keys into the trace context. + */ +const TRACE_CONTEXT_KEYS = ['traceparent', 'tracestate', 'baggage'] as const; + +/** Build a propagation carrier from `_meta`, keeping only string-valued W3C keys. */ +export function traceCarrierFromMeta(meta: Record | undefined): Record { + const carrier: Record = {}; + if (!meta) return carrier; + for (const key of TRACE_CONTEXT_KEYS) { + const value = meta[key]; + if (typeof value === 'string' && value.length > 0) carrier[key] = value; + } + return carrier; +} + /** Dependencies for listing tools — only needs the registry. */ export interface ListToolsDeps { registry: ToolRegistryService; @@ -98,6 +140,7 @@ export class ToolDispatchService { async callTool( name: string, args: Record = {}, + options: ToolCallOptions = {}, ): Promise { const argContext = (args.context as Record | undefined) || {}; const correlationId = (args.correlationId as string) || (argContext.correlationId as string) || generateCorrelationId(); @@ -230,19 +273,35 @@ export class ToolDispatchService { })); } - const traceparent = args.traceparent as string | undefined; + // GT-587: `_meta` first — that is where the protocol puts trace context. The + // `args.traceparent` path stays as a FALLBACK rather than being deleted: clients + // already using it would otherwise silently lose their trace linkage, and a + // telemetry regression is invisible until someone goes looking for a trace. + const metaCarrier = traceCarrierFromMeta(options.meta); + const legacyTraceparent = args.traceparent as string | undefined; + const carrier = + Object.keys(metaCarrier).length > 0 + ? metaCarrier + : legacyTraceparent + ? { traceparent: legacyTraceparent } + : undefined; const otelGetter = { get: (c: Record, k: string) => c[k], keys: (c: Record) => Object.keys(c), }; - const otelCtx = traceparent - ? propagation.extract(otelContext.active(), { traceparent }, otelGetter) + const otelCtx = carrier + ? propagation.extract(otelContext.active(), carrier, otelGetter) : otelContext.active(); const span = this.tracer.startSpan( `mcp.tool.${name}`, { attributes: { + // GT-587 — standard MCP vocabulary, emitted ALONGSIDE the private names + // below (which are already collected and must not move). + [ATTR_MCP_METHOD_NAME]: MCP_METHOD_NAME_VALUE_TOOLS_CALL, + [ATTR_MCP_TOOL_NAME]: name, + ...(options.sessionId ? { [ATTR_MCP_SESSION_ID]: options.sessionId } : {}), 'correlation.id': correlationId, 'tool.name': name, 'tool.mutative': !!tool.mutative, @@ -313,10 +372,11 @@ export async function handleCallTool( name: string, args: Record = {}, deps: DispatchDeps, + options: ToolCallOptions = {}, ): Promise { const service = new ToolDispatchService( deps.registry, deps.metrics, deps.abacEvaluator, deps.logger, deps.tracer, deps.headShaReader ?? readHeadSha, ); - return service.callTool(name, args); + return service.callTool(name, args, options); }