GT-587: emit the wire format collectors already understand, and pin the vocabulary that can move - #308
Open
beyondnetPeru wants to merge 2 commits into
Open
GT-587: emit the wire format collectors already understand, and pin the vocabulary that can move#308beyondnetPeru wants to merge 2 commits into
beyondnetPeru wants to merge 2 commits into
Conversation
`grep -rn "gen_ai" src` returned 0: the tracing plumbing existed and GT-546 emitted `evolith_*` metrics, but every attribute name was private, so nothing Evolith produced could join against what a customer already collects — and telemetry is not backfillable. - core-domain gains a PURE pinned vocabulary (`evaluation/telemetry/semconv.ts`) and a total `EvaluationResult -> gen_ai.evaluation.result` mapper. No OTel import: rule HXA-05 forbids an observability SDK in the domain/application layer, so the Core owns the words and the adapters own the spans. - core-api attaches one event per evaluated kind to the request span, on BOTH the canonical and inline branches (they return the same result, so they must emit the same signal). - mcp-server reads trace context from the request's `_meta` (SEP-414) instead of a `traceparent` smuggled through tool ARGUMENTS, and stamps `mcp.method.name`, `mcp.tool.name`, `mcp.session.id` and `mcp.protocol.version`. Only the three W3C keys are lifted out of `_meta` — it is an open map and must not become an injectable propagation carrier. The argument path stays as a fallback so existing callers do not silently lose trace linkage. Every semconv name rides ALONGSIDE the `evolith.*` ones, never instead of them. The pin is the load-bearing part: the GenAI registry is Development-status, so `51-validate-semconv-pin.mjs` fails on three independent conditions — the declared revision drifting from package-lock.json, a pinned literal no longer matching upstream, and a constant exported without a manifest entry. All three were observed red before this landed, plus the empty-corpus fixture via guard 43. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📊 Bilingual Coverage ImpactPR Changes
Repository Coverage
To create skeletons: node .harness/scripts/generate-es-skeleton.mjs <file.md>Generated by GitHub Actions |
The only conflict was two sessions appending a step at the same point in docs.yml — the semconv pin guard (this branch) and the Scorecard regression self-test (GT-597, already on develop). Both are kept; they are independent checks. The filename collision that would have made this dangerous was already avoided by renumbering the Scorecard guard 51 -> 52 before it merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes GT-587 (P2,
Evolith Core). Starting state verified:grep -rn "gen_ai" src→ 0 hits. The plumbing was already there; only the vocabulary was private.What landed
core-domain/src/evaluation/telemetry/semconv.ts— the pinned vocabulary,SEMCONV_VERSION = '1.41.1', plus a manifest the drift check reads. Zero imports: rule HXA-05 flags@opentelemetry/apiinside domain/application layers, andopentelemetry-tracker-adapter.tsalready duck-types for the same reason. The Core owns the words; the adapters own the spans.gen-ai-evaluation.ts— pureEvaluationResult → gen_ai.evaluation.resultevents, one per evaluated kind plusoverall. Scores normalised to 0..1; kinds with no honest score omit the attribute rather than inventing one.evaluation-telemetry.service.tswired into both branches ofevaluation.controller.ts.mcp.method.name/mcp.tool.name/mcp.session.id, and trace context lifted from_metainstead of thetraceparentpreviously smuggled through tool arguments. Only the three W3C keys are lifted:_metais an open map, and copying it wholesale would let a caller inject arbitrary propagation keys. The argument path survives as a fallback._metaandextra.sessionIdwere previously destructured away inmcp-server.service.ts; both now cross.mcp.protocol.versionis read from the HTTP header because the SDK does not expose the negotiated version to a handler — an attribute nobody can source is left absent, not fabricated.The semconv attributes are emitted alongside
evolith.*, not instead of them: the existing names are already being collected.Criterion 3 is the one that would be easy to fake
.harness/scripts/ci/51-validate-semconv-pin.mjschecks three independently-failable rules and reads the committed lockfile, notnode_modules, so local install state cannot make it green. All three were observed red:Independently falsified by the integrating session: bumping the pinned version → exit 1 naming the drift; restored → exit 0. Guard 43 (empty-corpus fixture)
RED 1/1 · reported a PASS 0; guard 4243/43; self-test 6/6 including the green case, so an unconditionally-failing guard could not pass. Wired intodocs.yml—ci-cd.ymland the two other files contested by an open PR were left untouched.The pin matters because that registry is still in Development status and can change underneath us.
Suites
core-domain 1464 passed / 131 suites · mcp-server 444 / 54 · core-api 181 / 32 · root
tsc -bclean.An environment finding that matters beyond this PR
This worktree's
node_modulessymlink resolved@beyondnet/*back to the MAIN checkout. Until it was fixed, every cross-package test in the worktree was measuring the main checkout's built code rather than the branch's — a false-green generator for any worktree agent.node_moduleswas rebuilt as a real directory with per-entry symlinks, worktree-local@beyondnetlinks, and the six nestednode_modules(@nestjs/cache-managerlives only in those). All numbers above are post-fix.Relatedly,
dist/is load-bearing: core-api and mcp-server jest resolve core-domain through itsexportsmap todist/, so a new module is invisible until built. The guard deliberately reads source, not dist, for exactly that reason.Also fixed en route: inserting the new controller dependency mid-constructor shifted positional construction in specs (9 red tests). Moved to last, with a comment saying why.
🤖 Generated with Claude Code