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.
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: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep on 61b9bae7 — 10 findings that verify against current source and are distinct from the threads already discussed or fixed in this PR (each inline comment calls out the related thread where one exists).
One finding lands on a file outside this PR's diff, so it cannot be attached inline:
[MEDIUM] New eval-measure command missing from the user-facing CLI index — docs/cli/README.md (line 29)
eval-measure is a newly registered root command (internal/cli/evalmeasure.go:31) with user-facing Long help, and it is documented in docs/guides/dev/cli-internals.md and the new infrastructure guide — but it is absent from the "Additional commands" table in docs/cli/README.md (verified: eval-measure does not appear anywhere in that file; the table at lines 23-29 lists only fullsend run, fullsend lock, fullsend scan). Operators browsing the CLI overview will not discover the command. This is the same class of gap as the earlier "guides readme not updated" finding on this PR, which was accepted and fixed in 509444e4 by indexing the new guide in docs/guides/README.md — so the repo already treats index omissions here as real.
Suggestion: Add an eval-measure row to the Additional commands table in docs/cli/README.md with a one-line description and a link to docs/guides/infrastructure/eval-measurements.md, consistent with the existing entries.
| if done { | ||
| continue | ||
| } | ||
| all = append(all, r) |
There was a problem hiding this comment.
[MEDIUM] Fix commit 61b9bae does not achieve its stated goal — printed results still mismatch disk
The commit message for 61b9bae says it forwards "already-scored results when a later append/ledger write fails so stdout matches the JSONL on disk." Verified at head: all = append(all, r) (run.go:61) runs BEFORE AppendMeasurements(measPath, ...) (line 62), so when the append fails the row is still in the returned slice and is printed by printMeasurementResults on the error path (internal/cli/evalmeasure.go:61). Before the fix stdout under-reported (dropped rows that were on disk); after it over-reports by exactly the row whose write failed. Neither direction matches disk. Both new tests are loose in exactly the dimension the fix claims to address: internal/evalmeasure/run_test.go:126 and internal/cli/evalmeasure_test.go:132 both assert require.GreaterOrEqual(t, len(results), 1), which passes whether or not the failed row is included. Distinct from the already-resolved thread at internal/cli/evalmeasure.go:116 (that one was about the caller dropping the failing file's rows entirely).
Suggestion: Move all = append(all, r) to after the successful AppendMeasurements call (before or after RecordScored — pick the one that matches the intended contract), and tighten both tests to require.Len(t, results, 1) so the over-report is actually caught.
| for _, raw := range ss.Spans { | ||
| sp, err := convertSpan(raw) | ||
| if err != nil { | ||
| return nil, stats, fmt.Errorf("line %d span %s: %w", lineNo, raw.SpanID, err) |
There was a problem hiding this comment.
[MEDIUM] Parser fail-open is incomplete — a single bad span or oversized line still discards the whole file
The doc comment at parse.go:50-52 states "Truncated or corrupt lines are skipped (fail-open); SkippedLines is the count of those lines." That now holds only for json.Unmarshal failures (line 78-80, fixed in c8438d8). Two paths remain fail-closed: (1) a convertSpan error — e.g. a non-numeric startTimeUnixNano/endTimeUnixNano failing strconv.ParseUint — does return nil, stats, fmt.Errorf("line %d span %s: %w", ...) at line 87, discarding every trace already accumulated; (2) a line longer than the 10 MB scanner cap (sc.Buffer(buf, 10*1024*1024), line 67) makes sc.Err() return bufio.ErrTooLong and line 100-101 returns nil traces, again discarding everything parsed. In both cases one malformed record costs the entire run's measurements. This is a residual path of the earlier "discards the whole file" thread (parse.go:78), and the reply on that thread ("a bad/truncated line is skipped; earlier traces are kept") is not true for these two branches. Reachable in practice once span content capture grows (ADR 0087 anticipates Level 3 content capture in #5947).
Suggestion: Treat a convertSpan failure the same as a corrupt line — increment SkippedLines (or a dedicated SkippedSpans), skip the span, keep going. On sc.Err(), return the traces gathered so far alongside the error so the caller can still score and persist them.
| } | ||
| prefix := "" | ||
| if agent != "" { | ||
| prefix = "agent-" + strings.ToLower(agent) + "-" |
There was a problem hiding this comment.
[MEDIUM] Agent-name case mismatch between fullsend run and eval-measure silently yields zero measurements
FindPlatformTelemetry builds the run-directory prefix as "agent-" + strings.ToLower(agent) + "-" (find.go:41), and sanitizeMeasurementAgentName (internal/cli/evalmeasure.go:171) also lowercases. But fullsend run names the run directory from the raw argument: sandboxName := fmt.Sprintf("agent-%s-%d-%d", agentName, os.Getpid(), time.Now().Unix()) (internal/cli/run.go:900, agentName := args[0] at run.go:246) with no normalization, and writes fullsend.agent with the same raw casing (run.go:920). Mixed case IS reachable: findConfigAgentEntry (run.go:3719-3727) resolves config agents case-insensitively, so fullsend run Review succeeds and produces agent-Review-<pid>-<unix> — which the lowercased prefix never matches, so eval-measure prints "No platform run-telemetry.jsonl ... skipping" and exits 0. The same mismatch exists in ScoreTrace (internal/evalmeasure/registry.go:74): name != reg.Agent is a byte-for-byte compare, so a trace attribute of Review against a manifest agent: review returns nil rows with the misleading message "No new measurements (already scored or no matching traces)". Separately, the prefix check is strings.HasPrefix rather than an exact runDir identity, so a lookup for agent code also matches agent-code-review-<pid>-<unix>.
Suggestion: Normalize on both sides: lowercase agentName when building sandboxName (or fold case in the prefix comparison), and use strings.EqualFold(name, reg.Agent) in ScoreTrace. Tighten the directory match to the full agent-<name>-<digits>-<digits> shape so a longer agent name cannot be matched by a shorter one's prefix.
| } | ||
| return harness.ComposeOpts{ | ||
| WorkspaceRoot: abs, | ||
| FetchPolicy: fetch.DefaultPolicy, |
There was a problem hiding this comment.
[MEDIUM] eval-measure hardcodes fetch.DefaultPolicy and ignores offline mode
evalMeasureFetchContext builds harness.ComposeOpts{... FetchPolicy: fetch.DefaultPolicy ...} (evalmeasure.go:208). fetch.DefaultPolicy (internal/fetch/fetch.go:72) leaves Offline at its zero value, false. By contrast fullsend run sets policy.Offline = rFlags.offline (internal/cli/run.go:310), and fetchPinnedAgentsRepoFile has an explicit if composeOpts.FetchPolicy.Offline { return ... } guard (run.go:3769) — a guard that is dead for this caller. eval-measure exposes no --offline flag. In an air-gapped or offline-configured install, fullsend run --offline is now followed by a step that attempts GetRef against api.github.com plus a raw.githubusercontent.com fetch on every managed run, producing warnings and DNS/connect latency where the operator explicitly asked for no network.
Suggestion: Add --offline to eval-measure (or read the same source fullsend run uses) and set policy.Offline accordingly, so offline installs short-circuit to the local-manifest-only path via the existing guard.
| } | ||
| token, err := resolveToken() | ||
| if (err != nil || token == "") && printer != nil { | ||
| printer.StepWarn("No GH_TOKEN/GITHUB_TOKEN; agents@v0 GetRef is unauthenticated. GitLab jobs skip stock manifests unless an operator wires a GitHub token.") |
There was a problem hiding this comment.
[MEDIUM] "GitLab jobs skip stock manifests without a GitHub token" is asserted in three places and is false
Three texts assert manifest resolution skips when no GitHub token is present: the warn string in evalMeasureFetchContext ("No GH_TOKEN/GITHUB_TOKEN; agents@v0 GetRef is unauthenticated. GitLab jobs skip stock manifests unless an operator wires a GitHub token.", evalmeasure.go:204), the scaffold comment ("This job has no GH_TOKEN; stock-agent manifests skip unless an operator exports GH_TOKEN or GITHUB_TOKEN", internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:296-297), and docs/guides/infrastructure/eval-measurements.md. Verified against the code path: gh.New(token) stores the token unconditionally and LiveClient.do guards only the header — if c.token != "" { req.Header.Set("Authorization", "Bearer "+c.token) } (internal/forge/github/github.go:167-169). There is no empty-token early return in GetRef or in fetchPinnedAgentsRepoFile (internal/cli/run.go:3764-3790). GitHub serves unauthenticated GET /repos/{owner}/{repo}/git/ref/tags/v0 for a public repo, rate-limited to 60 req/hr per IP. So the real behaviour is the opposite of what is documented: GitLab jobs DO reach api.github.com and normally resolve the manifest; the actual failure mode is intermittent 403 rate-limit errors on shared runner IPs (silently swallowed by || true), plus an undocumented outbound GitHub dependency in GitLab CI. The warn string even contradicts itself — it says GetRef is unauthenticated and then that GitLab will skip.
Suggestion: Pick one behaviour and make code and docs agree: either add an explicit if token == "" { skip } guard in resolveEvalMeasureRegistry / tryAgentsRepoMeasurementManifest (matching the docs and making the GitLab path deterministic and network-free), or keep the unauthenticated fetch and correct all three texts to say the fetch IS attempted unauthenticated and is subject to GitHub's 60 req/hr/IP limit, with a token recommended on shared runners.
| # SHA-pinned fetch from public fullsend-ai/agents (GitHub GetRef). | ||
| # This job has no GH_TOKEN; stock-agent manifests skip unless an | ||
| # operator exports GH_TOKEN or GITHUB_TOKEN. Local override still works. | ||
| fullsend eval-measure \ |
There was a problem hiding this comment.
[MEDIUM] GitLab scaffold writes eval-measurements.jsonl to an ephemeral path with no artifact retention
The new step runs fullsend eval-measure --output-dir /tmp/fullsend-output || true (lines 298-302). eval-measure writes eval-measurements.jsonl beside the telemetry, i.e. under /tmp/fullsend-output/agent-<name>-*/. Verified there is no artifacts: key anywhere in the file, so the JSONL is destroyed with the job container and the score is never retained. This is the divergence from the GitHub Actions path, where the corresponding action.yml step was deliberately moved ahead of "Upload fullsend artifacts" in 615dd69 precisely so eval-measurements.jsonl is uploaded. Net effect on GitLab today: the step consumes job time and network but produces nothing durable. Note that a literal artifacts: paths: [/tmp/...] will not work — GitLab only collects paths under $CI_PROJECT_DIR.
Suggestion: Make the score survive the job: write the run output under $CI_PROJECT_DIR (or copy eval-measurements.jsonl there after the step) and add an artifacts: stanza with when: always covering it — mirroring what action.yml now does on the GitHub side. Otherwise gate the step off on GitLab so it does not run when it provably cannot produce a retained artifact.
| // UnknownSentinel is the CLI fallback when no issue- or PR-shaped env is set. | ||
| // Review jobs that have PR_NUMBER / GITHUB_PR_URL should not hit this | ||
| // after #5622; a remaining unknown is a real fitness fail. | ||
| {"work_item", workItemOK(run)}, |
There was a problem hiding this comment.
[MEDIUM] work_item check is structurally unsatisfiable for retro, which agents#722 nonetheless enables
workItemOK (fitness.go:139-142) fails when fullsend.work_item_id is absent or equals UnknownSentinel, and ScoreFitnessNamed requires all 8 checks for label: pass. resolveWorkItemID() (internal/cli/run.go:2380-2408, verified at head) reads only ISSUE_KEY, REPO_FULL_NAME+ISSUE_NUMBER, GITHUB_ISSUE_URL, bare ISSUE_NUMBER, REPO_FULL_NAME+PR_NUMBER, GITHUB_PR_URL, bare PR_NUMBER — else unknown. .github/workflows/reusable-retro.yml ("Run retro agent" step, lines 158-163) sets only ORIGINATING_URL and REPO_FULL_NAME; ORIGINATING_URL is not one of the keys resolveWorkItemID reads. So every retro trace records work_item_id=unknown, work_item fails, and em-001 is pinned at 0.875 / label: fail forever. Confirmed against the companion PR: fullsend-ai/agents#722 adds eval/measurements/retro.yaml enabling em-001 / trace_fitness / version 1, while that PR's own body says "Scribe intentionally omitted (no forge work-item identity for EM-001 today)" — the exclusion criterion was defined, retro meets it, but retro was not checked against its workflow env. Same risk for prioritize on any trigger whose event payload has no issue object. Consumers reading the trend will see a permanent structural fail as a regression. Distinct from the earlier fitness.go:34 thread, which was about review agents and the PR-fallback chain.
Suggestion: Fix on the fullsend side by adding ORIGINATING_URL as a fallback in resolveWorkItemID() (it is the retro run's forge entity URL — exactly the identity work_item wants), or have reusable-retro.yml export GITHUB_ISSUE_URL/GITHUB_PR_URL from the event payload. If instead retro genuinely has no work item, drop em-001 from eval/measurements/retro.yaml in agents#722 the same way scribe was excluded. Either way record the answer in ADR 0087 next to the pre-script-skip exclusion.
| // after #5622; a remaining unknown is a real fitness fail. | ||
| {"work_item", workItemOK(run)}, | ||
| {"operation", attrNonEmpty(run, AttrGenAIOperationName)}, | ||
| {"model", modelOK(run, agents)}, |
There was a problem hiding this comment.
[MEDIUM] Infrastructure failures score as a telemetry-contract fail, indistinguishable from instrumentation drift
The model, usage, and cost_tools_turns checks (fitness.go:69-71) key off attributes that fullsend run emits only inside if runCount > 0 { ... } (internal/cli/run.go:~950-966), and span_tree additionally requires a sandbox_create span. A run that dies before the first agent iteration — sandbox create failure, provider failure, image pull failure — emits exit_code, fullsend.agent, and gen_ai.operation.name but none of the metric attributes, scoring ~0.25-0.5 with label: fail. Only the pre-script-skip case is special-cased (fitness.go:34-51). ADR 0087 and the PR body describe EM-001 as a platform fitness check on the telemetry contract, but in this shape it also fires on ordinary infrastructure failures, so a rise in the fail rate cannot be attributed to instrumentation drift without manual triage — the pass/(pass+fail) trend measures runner health as much as the contract.
Suggestion: Extend the pattern the pre-script-skip exclusion already established: when the trace has no agent span at all (the run never reached an iteration), emit label: skip with an explanation, so pass/(pass+fail) measures the telemetry contract rather than runner health.
| AttrFullsendAgent = "fullsend.agent" | ||
| AttrFullsendWorkItemID = "fullsend.work_item_id" | ||
| AttrGenAIAgentName = "gen_ai.agent.name" | ||
| AttrGenAISystem = "gen_ai.system" |
There was a problem hiding this comment.
[MEDIUM] gen_ai.system was renamed upstream before this ships — the comment and ADR rationale are factually stale
types.go:9-11 states the gen_ai.* constants "follow experimental OpenTelemetry GenAI semantic conventions; an upstream rename is an em-001 version bump, not a silent fleet fail", and ADR 0087 repeats the claim. Verified against primary sources: the OpenTelemetry semantic-conventions CHANGELOG records under v1.37.0 Breaking changes — "Follow system-specific naming policy in GenAI semantic conventions. Rename gen_ai.system to gen_ai.provider.name" — and the current GenAI attribute registry (model/gen-ai/registry.yaml) contains gen_ai.provider.name with no gen_ai.system entry at all. AttrGenAISystem = "gen_ai.system" (types.go:16) is load-bearing: modelOK() (fitness.go:159-166) requires it non-empty on an agent span, so EM-001 hard-codes an attribute name upstream has removed. The other constants check out (gen_ai.agent.name, gen_ai.operation.name, gen_ai.request.model, gen_ai.usage.input_tokens/output_tokens are all still present). Nothing breaks today because fullsend's own emitter (ADR 0050) still writes gen_ai.system, but em-001@1 ships already-stale and will silently start failing model the moment the emitter migrates. The rename the earlier types.go:15 note treated as hypothetical has already happened.
Suggestion: Update the comment at types.go:9-11 and the corresponding ADR 0087 sentence to record that gen_ai.system is deprecated as of semconv v1.37.0 in favour of gen_ai.provider.name, and make modelOK() accept either key (prefer gen_ai.provider.name, fall back to gen_ai.system) so em-001@1 keeps passing across the emitter migration instead of requiring a fleet-wide version bump.
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.