feat(eval): add eval measurements and EM-001 trace_fitness scorer - #6036
feat(eval): add eval measurements and EM-001 trace_fitness scorer#6036ascerra wants to merge 10 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 11:45 AM UTC · Completed 12:02 PM UTC Commit: |
PR Summary by QodoAdd eval measurements CLI and EM-001 trace_fitness scoring
AI Description
Diagram
High-Level Assessment
Files changed (31)
|
Site previewPreview: https://c7807bd5-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsLow
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (7)ReviewFindingsHigh
Medium
Low
Labels: PR adds new Go eval-measure package (internal/evalmeasure/), modifies CI workflows (.github/workflows/), and adds documentation (docs/guides/, docs/ADRs/) Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
type/feature
|
🤖 Review · ❌ Terminated · Started 1:34 PM UTC · Ended 1:54 PM UTC Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:54 PM UTC Commit: |
- Fix data-loss bug: swap AppendMeasurements before RecordScored so ledger only marks scored after measurement is persisted - Fix broken curl header args in action.yml using bash array instead of parameter expansion (word-split safe) - Add Prerequisites section to eval-measurements guide - Add OTEL/OTLP inline definitions on first use in guide - Use > **Planned:** callout format for unimplemented OTLP export - Update docs/guides/README.md with eval measurements entry - Consolidate ADR 0087 Decision into single paragraph (was multiple sub-decisions); trim Consequences to 5 bullets - ADR 0050 cross-reference uses Planned callout format - Rename printResults → printMeasurementResults Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 1:15 AM UTC · Completed 1:33 AM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Code review by qodo was updated up to the latest commit 615dd69 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 11:49 AM UTC · Completed 12:02 PM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings (security/reliability), verified against current head and cross-checked against existing PR comments for duplicates.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings (security/reliability), verified against current head and cross-checked against existing PR comments for duplicates.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings (3 items) — see inline comments.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 1 finding.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional findings on files not touched by this PR's diff (so they can't be attached as inline comments):
[MEDIUM] docs/problems/operational-observability.md (line 194) — open questions not annotated even though ADR 0087 partially answers them
This PR does not touch docs/problems/operational-observability.md at all, yet two of its still-open questions are partially resolved by ADR 0087: "How do we measure 'is the system getting better'?" (line 194) now has a partial answer via the new deterministic trace-fitness scoring/trend layer, and "At what scale does a dedicated LLM observability platform justify its operational overhead?" (line 195) is partially answered by the ADR's explicit choice to stay backend-agnostic (local JSONL + reuse of OTLP export) rather than adopt a vendor platform. Other ADRs in this problem doc (0041, 0021, 0050) are annotated with strikethrough + a pointer once decided; these two are left unmarked.
Suggestion: Annotate the relevant open questions in docs/problems/operational-observability.md with a pointer to ADR 0087 (following the existing strikethrough + link pattern used for ADR 0041/0021/0050), noting it's a partial/first-ship answer where applicable.
[MEDIUM] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml (line 299) — GitLab-managed agent jobs get no equivalent eval-measure step
ADR 0087 states measurements run "in the same managed job" as fullsend run, but only the GitHub composite action (action.yml) was updated with an Eval-measurements step; the GitLab CI template calls fullsend run "${STAGE}" ... (line 299) with no follow-on fullsend eval-measure invocation anywhere in the file. Confirmed via grep: no eval-measure, eval_measure, or EvalMeasure reference exists under internal/scaffold/fullsend-repo-gitlab/. GitLab-hosted agent runs therefore never get scored, silently diverging from GitHub-hosted runs with no documentation of this gap.
Suggestion: Add the same fail-open eval-measure step to the GitLab CI template, or explicitly scope ADR 0087 / this PR as GitHub-first and note the GitLab gap as a follow-up in the ADR's Consequences or a tracking issue.
SHA-pin agents@v0 measurement manifests in the eval-measure binary, score only platform run-telemetry.jsonl, skip unknown scorers and pre-script-skipped runs, and keep GitLab measure fail-open after a failed agent run. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
|
Pushed
Thread replies are on the review comments. Left threads unresolved for Wayne to re-review. |
ascerra
left a comment
ascerra
left a comment
There was a problem hiding this comment.
Inline responses on the current diff (c8438d83) for Wayne’s eval-measure findings. Earlier replies were attached to the pre-push commit and GitHub marked those threads outdated.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep on top of the c8438d8 fixes — 6 findings that verify against current source and are distinct from the threads already discussed/fixed in this PR (called out inline where a finding is a follow-up to an already-resolved issue in the same area).
| # Eval measurements (fail-open): score run-telemetry.jsonl with the agents | ||
| # measurement manifest. Same job as fullsend run; never fails the agent. | ||
| # Scores always land in eval-measurements.jsonl (tool-agnostic artifact). | ||
| - name: Eval measurements |
There was a problem hiding this comment.
[HIGH] Eval-measure step/scaffold never inject GH_TOKEN, so manifest GetRef runs unauthenticated
The "Eval measurements" step's env: block (action.yml:418-420) sets only AGENT and FULLSEND_DIR — unlike the preceding "Run fullsend" step (action.yml:379), which explicitly sets GH_TOKEN: ${{ inputs.github_token }}. evalMeasureFetchContext (internal/cli/evalmeasure.go:191) calls token, _ := resolveToken() and discards the error; resolveToken() (internal/cli/admin.go:91-106) checks GH_TOKEN, then GITHUB_TOKEN, then gh auth token, returning an error if none are set — which happens for this step. The resulting empty token is passed into gh.New(token) and used by fetchPinnedAgentsRepoFile (internal/cli/run.go:3751) to call forgeClient.GetRef(ctx, ..., "tags/"+config.DefaultUpstreamRef) against api.github.com. An unauthenticated GetRef is subject to GitHub's 60 req/hour per-IP limit, shared across the whole GitHub-hosted runner IP pool.
The identical gap exists in the new GitLab CI scaffold step (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:310-320), which also invokes fullsend eval-measure --agent ... --fullsend-dir ... with no GITHUB_TOKEN/GH_TOKEN configured anywhere in that pipeline (only GITLAB_TOKEN is exported).
Because the whole step is fail-open (continue-on-error: true / || true), failures are silent StepWarn skips, so stock-agent measurement scoring will intermittently (GHA) or consistently (GitLab, no GitHub identity at all) fail to fetch the SHA-pinned manifest from fullsend-ai/agents@v0.
Note: this is distinct from the earlier reviewed-and-fixed concern about sending a Bearer token to the public raw.githubusercontent.com curl (that curl call is gone in c8438d83) — this is about the replacement GetRef API call having no token available at all.
Suggestion: add GH_TOKEN: ${{ inputs.github_token }} to the action.yml "Eval measurements" step's env block (mirroring "Run fullsend"), and document/accept explicitly that the GitLab scaffold step has no GitHub token available so stock-agent measurement fetch will reliably skip there unless an operator wires one in.
There was a problem hiding this comment.
Fixed in 3d5aefc6. The Eval measurements step now sets GH_TOKEN: ${{ inputs.github_token }} for GetRef of agents@v0 (not a Bearer header to raw.githubusercontent.com).
GitLab: documented skip — this job has no GitHub token, so stock-agent manifests skip unless an operator exports GH_TOKEN/GITHUB_TOKEN. Local .fullsend override still works. Empty-token path logs a warning (TestActionYML_EvalMeasureNoFloatingV0Curl asserts GH_TOKEN: on the step; GitLab test asserts stock-agent manifests skip).
| │ └── url # Validate URLs against SSRF attacks | ||
| ├── post-review # Post PR review comments to GitHub | ||
| ├── post-comment # Post issue/PR comments to GitHub | ||
| ├── eval-measure # Score wild-run traces (eval measurements) |
There was a problem hiding this comment.
[MEDIUM] cli-internals.md lists eval-measure's --telemetry/--registry as unconditionally required
This doc documents only --telemetry <path> (required), --registry <path> (required), and --out-dir. The actual contract in internal/cli/evalmeasure.go is alternative flags: resolveEvalMeasureTelemetry (~line 120) errors only if both --telemetry and --output-dir are empty ("either --telemetry or --output-dir is required"), and resolveEvalMeasureRegistry (~line 130) errors only if both --registry and --agent are empty ("either --registry or --agent is required"). The doc omits --agent, --fullsend-dir, and --output-dir entirely, even though those are exactly the flags action.yml and the GitLab scaffold template actually pass (--agent, --fullsend-dir, --output-dir) — an operator following only this doc's tree would conclude the real managed-job invocation is invalid.
Suggestion: update the eval-measure entry to show both flag pairs as mutually-exclusive alternatives (--telemetry|--output-dir, --registry|--agent) and add the missing --agent/--fullsend-dir/--output-dir flags.
There was a problem hiding this comment.
Fixed in the merge (4ff94995 / 3d5aefc6). docs/guides/dev/cli-internals.md now lists both pairs: --telemetry or --output-dir, --registry or --agent, plus --fullsend-dir and --out-dir.
| return ok && v != "" | ||
| } | ||
|
|
||
| func modelOK(run Span, agents []Span) bool { |
There was a problem hiding this comment.
[MEDIUM] modelOK() treats empty-string gen_ai.request.model/gen_ai.system as present
modelOK checks _, ok := run.AttrString("gen_ai.request.model") / _, ok := a.AttrString("gen_ai.system") and treats ok alone as satisfying the check. AttrString (internal/evalmeasure/types.go:44-55) returns (v, true) for any present non-nil value, including the empty string "" — it only returns false when the key is absent or nil. This is inconsistent with attrNonEmpty/identityOK/workItemOK in this same file, which explicitly require v != "". As written, a span carrying gen_ai.request.model="" or gen_ai.system="" (effectively missing instrumentation) still satisfies the model sub-check in EM-001's 8-check contract.
Suggestion: reuse attrNonEmpty (or add an explicit v != "" check) for both gen_ai.request.model and gen_ai.system inside modelOK.
There was a problem hiding this comment.
Fixed in 3d5aefc6. modelOK uses attrNonEmpty for gen_ai.request.model and gen_ai.system, so "" fails the model check. Test: TestScoreFitness_EmptyModelStringFails.
Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Inject GH_TOKEN for SHA-pinned agents@v0 GetRef, score only the host runDir for --agent, reject empty gen_ai model/system strings, and warn when telemetry JSONL lines are unreadable. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
ascerra
left a comment
ascerra
left a comment
There was a problem hiding this comment.
Inline replies on 3d5aefc6 for Wayne’s second-round findings (also merged origin/main to clear conflicts).
|
Merged
Inline replies are on the current diff. |
|
🤖 Finished Review · ✅ Success · Started 6:51 PM UTC · Completed 7:08 PM UTC Commit: |
There was a problem hiding this comment.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/evalmeasure/run.go:46: [low] data-quality-on-partial-failure
If AppendMeasurements succeeds but RecordScored fails, the measurement is persisted to eval-measurements.jsonl but the ledger is not updated. On retry, the same measurement is appended again (duplicate line). Documented in TestMeasureFile_AppendBeforeLedger; consumers can deduplicate on (trace_id, name, version).
internal/evalmeasure/export_local.go:51: [low] ledger-key-injection
The ledger key format is traceID|evalName|version. Registry validation rejects pipe and newline in evalName fields, but traceID comes from parsed OTLP data and is not validated for pipe or newline characters. A malformed traceID containing | could cause a false-positive match in AlreadyScored. In practice, OTLP trace IDs are 32-character hex strings, and FindPlatformTelemetry ensures only host-written files are scored.
internal/evalmeasure/find.go:37: [low] edge-case
FindPlatformTelemetry returns at most one telemetry path when searching child runDirs (the newest by modification time). If an outputDir legitimately contains telemetry from multiple agent runDirs for different agents in the same job, only the newest is scored. Documented and intentional.
internal/cli/evalmeasure.go(file-level): Line 643 · [low] naming-inconsistency
The CLI flag --out-dir uses a different naming convention than --output-dir. Both are defined on the same command with subtly different semantics (measurements output directory vs CI output base), creating potential confusion.
Suggested fix: Consider renaming --out-dir to --measurements-dir or --score-dir to distinguish it from --output-dir.
| printer.StepWarn(fmt.Sprintf("%s: skipped %d of %d unreadable telemetry line(s)", p, stats.SkippedLines, stats.NonEmptyLines)) | ||
| } | ||
| if err != nil { | ||
| return all, false, err |
There was a problem hiding this comment.
[MEDIUM] runEvalMeasure drops the failing telemetry file's already-scored/written results when returning an error
In the loop over telemPaths, evalmeasure.MeasureAndExport is called per file and, on a mid-loop failure (ledger-check/append-measurements/record-scored error), it returns its own partial results alongside the error (see internal/evalmeasure/run.go: MeasureAndExport appends each already-persisted EvaluationResult to its local all before hitting a failing step). But this caller's loop does return all, false, err here without first doing all = append(all, results...) — it only carries forward results from prior, already-succeeded iterations of the outer loop. Since RunE's error branch calls printMeasurementResults(printer, results, false) with this returned all, any measurements already written to eval-measurements.jsonl by the failing file's MeasureAndExport call are silently omitted from stdout, even though the file on disk has them.
This contradicts the earlier review threads on run.go (lines 37/39/41) claiming "Fixed in 509444e: CLI prints partial results before returning a write/ledger error" — those threads are still open, and commit 509444e4 is not an ancestor of the current PR head (it diverged, likely dropped in a rebase/squash before the multi-file telemPaths loop was introduced here). The existing test TestEvalMeasureCmd_ErrorDoesNotPrintWrote only asserts the "Wrote N measurement(s)" summary line is absent on error — it does not assert that already-scored rows from the failing file are actually printed, so it doesn't catch this gap.
Suggested fix: In the error branch, append the current call's partial results before returning, e.g. return append(all, results...), false, err, and add a regression test with 2+ traces in one telemetry file where the second trace's ledger/append/record step fails, asserting the first trace's result is present in the printed/returned results.
There was a problem hiding this comment.
Fixed in 61b9bae7. The error branch now returns append(all, results...), so already-persisted rows from the failing file stay in the CLI result slice and get printed. wroteOK stays false, so we still do not print Wrote N.
Regression: two traces in one telemetry file; the second persist is forced to fail after the first RecordScored. Tests: TestRunEvalMeasure_ErrorIncludesPartialResults, TestEvalMeasureCmd_ErrorPrintsPartialFromFailingFile, TestMeasureAndExport_KeepsFirstWhenSecondPersistFails.
Forward MeasureAndExport's already-scored results when a later append/ledger write fails so stdout matches the JSONL on disk. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
ascerra
left a comment
ascerra
left a comment
There was a problem hiding this comment.
Reply on the persist-error thread (61b9bae7).
| printer.StepWarn(fmt.Sprintf("%s: skipped %d of %d unreadable telemetry line(s)", p, stats.SkippedLines, stats.NonEmptyLines)) | ||
| } | ||
| if err != nil { | ||
| return append(all, results...), false, err |
There was a problem hiding this comment.
Fixed in 61b9bae7: return append(all, results...), false, err so the failing file's already-scored rows are printed. Tests: TestRunEvalMeasure_ErrorIncludesPartialResults, TestEvalMeasureCmd_ErrorPrintsPartialFromFailingFile.
|
🤖 Finished Review · ❌ Failure · Started 1:58 AM UTC · Completed 2:00 AM UTC Commit: |
3 participants
Summary
Introduce eval measurements: fail-open same-job scoring of wild agent OTEL traces (
fullsend eval-measure), always writing portableeval-measurements.jsonlbeside telemetry. First scorer istrace_fitness(EM-001). Decision recorded in ADR 0087.Companion default-policy PR: fullsend-ai/agents#722 (manifests under
eval/measurements/). Until those land onagents@v0, the measure step skips cleanly.Ownership (please read)
internal/evalmeasure/)FULLSEND_DIRfullsend-ai/agents@v0when no local file exists — installs do not copy manifests to score stock agents.${FULLSEND_DIR}/eval/measurements/${AGENT}.yamlis override / opt-out / custom-agent only.eval-measureis the released binary that readsrun-telemetry.jsonl(which fullsend writes). Agents is content/policy, not that binary.assert:→ fullsend PR; new id / enable / thresholds on an existing scorer for a stock agent → agents-only; org-specific policy → local override.Tool-agnostic export
Core does not pick an observability product. Scores always land in local
eval-measurements.jsonl. Remote score export (when implemented) reuses the sameOTEL_EXPORTER_OTLP_*path as ADR 0050. No vendor Assessments adapters orMLFLOW_*(or similar) wiring in managed workflows.Related Issue
N/A (architecture + first scorer). Adjacent: #5947, #5944, #2423.
Changes
internal/evalmeasureparser +trace_fitness+ local JSONL/ledgerfullsend eval-measureCLI; fail-open post-step inaction.ymlFULLSEND_DIRthenagents@v0fetchTesting
go test ./internal/evalmeasure/(+ focused CLI eval-measure tests)v0(agents#722)Tested on local MLflow instance. Testing the case where a team chooses to use MLflow as their system to send the eval measurements scorer results too. See this example of the MLflow UI showing that trace_fitness ran and failed one time
Then see here the as part of the trace on the UI we can see a new assessment (what MLflow calls scorers) showing a passing results and what the trace_fitness eval measurementlooks for
Checklist
Notes for reviewers
--registry(path to the YAML); rename to “manifest” is follow-up.