feat(eval): add measurement manifests for online scoring - #722
Conversation
PR Summary by QodoAdd per-agent measurement manifests for online scoring (trace_fitness em-001)
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 11:55 AM UTC · Completed 12:08 PM UTC Commit: |
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsHigh
Low
Labels: PR adds new eval/measurements subsystem (YAML manifests, linter, tests) — feature addition, not purely documentation Next steps:
Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Labels: PR adds YAML policy files and documentation for eval measurement manifests Previous run (5)ReviewFindingsLow
|
![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
the
ready-for-merge
c1dbd68 to
74b8959
Compare
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:48 PM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
MEDIUM: No CI/pre-commit validation for eval/measurements/*.yaml — bad scorer/agent values silently no-op forever
(Referencing .pre-commit-config.yaml line 49, which isn't part of this PR's diff, so noting it here in the review body instead of inline.)
Confirmed on head 93d3db9f: the repo has a pre-commit hook lint-agent-docs (./hack/lint-agent-docs) scoped via files: ^(harness/|docs/|hack/lint-agent-docs) that validates every harness/*.yaml has a valid doc: field, and a separate eval/lint-cases.sh that validates eval/<agent>/cases and eval.yaml judges — but neither covers the new eval/measurements/*.yaml files added in this PR, and no .github/workflows/*.yml step does either. Only the generic check-yaml pre-commit hook (syntax-only) applies.
On the consumer side, in the companion PR fullsend-ai/fullsend#6036, internal/evalmeasure/registry.go's ScoreTrace has:
switch m.Scorer {
case ScorerFitness: ...
default: // Unknown scorers are skipped (forward-compatible)
}i.e. a typo'd scorer: value (e.g. trace-fitness with a hyphen) or a misspelled agent: value that never matches a trace's agent name would pass this repo's CI cleanly, merge, and then silently produce zero measurements for that agent in production with no error/log/CI failure.
The six current manifests all correctly use the literal trace_fitness matching const ScorerFitness = "trace_fitness" in fitness.go, so nothing is broken today, but there's no guardrail against this on the next manifest edit or new-agent addition.
Suggestion: Add a small lint script (sibling to hack/lint-agent-docs, or extend eval/lint-cases.sh) that validates eval/measurements/*.yaml: agent: matches an existing agents/<name>.md, measurements[].id is unique per file, and .scorer/.version are present/well-formed (and, once the fullsend-side registry is stable, checked against a known-scorer allow-list). Wire it into .pre-commit-config.yaml (files: ^eval/measurements/) so a typo fails CI instead of silently no-op'ing at runtime.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
documentation
93d3db9 to
228c7b5
Compare
|
🤖 Review · Commit: |
|
🤖 Review · ❌ Terminated · Started 11:51 AM UTC · Ended 12:04 PM UTC Commit: |
|
Code review by qodo was updated up to the latest commit 228c7b5 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 11:51 AM UTC · Completed 12:04 PM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 3 findings (0 duplicates skipped after checking existing inline comments/reviews on this PR).
| @@ -0,0 +1,6 @@ | |||
| --- | |||
There was a problem hiding this comment.
MEDIUM: review.yaml enables em-001 even though its work_item sub-check can never pass for review traces
Verified against the companion PR's actual code (fullsend-ai/fullsend#6036, internal/evalmeasure/fitness.go @ 615dd691c2): ScoreFitnessNamed requires 8/8 sub-checks to pass for label="pass". One sub-check, workItemOK(run), requires fullsend.work_item_id to be set and not the literal "unknown". The code's own comment states: '"unknown" is the CLI sentinel when no ISSUE_*/GITHUB_ISSUE_URL is set (common for review, which wires PR_NUMBER / GITHUB_PR_URL instead).' This is confirmed by the companion PR's own test fixture testdata/review-unknown-workitem.jsonl (fullsend.work_item_id="unknown") and score_test.go's TestScoreFitness_ReviewUnknownWorkItemFails, which asserts Explanation contains work_item=fail and missing: work_item for an agent:review trace. Since this agents-repo PR enables em-001 for review, review's fitness score is structurally capped at 7/8=0.875 and will always report label=fail by design — nothing in this PR or the companion PR's manifest/README documents that cap specifically for review, and it is presented as uniform across all six enabled agents.
Suggestion: Add a note next to review.yaml (or in the README's per-agent table) that review's em-001 label will always read 'fail' until the scorer gains a PR-based identity check or review starts populating fullsend.work_item_id, so downstream trend consumers don't mistake the permanent fail for a regression.
There was a problem hiding this comment.
Disagree — this is not a structural cap on review, and we are not going to document a permanent fail or drop review.yaml.
The comment you quoted from fitness.go @ 615dd691c2 was already stale relative to production CLI. resolveWorkItemID() gained a PR-env fallback in fullsend-ai/fullsend#5622 (fixes fullsend-ai/fullsend#5621, merged 2026-07-27, present in v0.33.0 through current v0.36.0). After issue-shaped env is exhausted it uses REPO_FULL_NAME+PR_NUMBER, then GITHUB_PR_URL, then bare PR_NUMBER.
Current fullsend-ai/fullsend#6036 fitness.go matches that: remaining "unknown" is a real fitness fail, not expected for review. TestScoreFitness_ReviewUnknownWorkItemFails / review-unknown-workitem.jsonl are a synthetic regression fixture (work_item_id="unknown"), not wild review traces.
Proof from a live review job today on v0.36.0 (https://github.com/fullsend-ai/.fullsend/actions/runs/32057512371, reviewing fullsend#6111):
GITHUB_ISSUE_URLempty (the exact #5621 case)PR_NUMBER=6111,GITHUB_PR_URL=https://github.com/fullsend-ai/fullsend/pull/6111- root
runspan:"fullsend.work_item_id": "fullsend-ai/fullsend#6111"
workItemOK still rejects the sentinel — that is the scorer working. Review can pass 8/8. Leaving review.yaml enabled.
|
|
||
| Executable logic stays in fullsend because `fullsend eval-measure` is the | ||
| released binary that reads `run-telemetry.jsonl` (produced by fullsend). This | ||
| repo is content/policy, not that binary. Platform checks like EM-001 |
There was a problem hiding this comment.
MEDIUM: EM-001/em-001 casing inconsistency reintroduced after prior fix
A prior review thread on this PR (resolved) flagged the manifests using 'em-001' while the README used 'EM-001', and commit e8b75c0 ('fix(eval): normalize ID casing and add YAML document markers') fixed that original occurrence, correctly using lowercase in the 'First ship' section and the id: field. However, a later commit, 4d5728a ('docs(eval): clarify measurement manifest ownership'), rewrote/expanded the README and introduced a brand-new sentence at this line — 'Platform checks like EM-001 (trace_fitness) still get enabled here for each stock agent' — using uppercase again, while lines 47/54 in the same file (added by the same rewrite) correctly use lowercase 'em-001' matching the manifests' literal id: fields. This is a new, unresolved instance of the exact ambiguity the earlier fix addressed, at a different location than the already-resolved thread, so it was never caught by the earlier fix.
Suggestion: Change 'Platform checks like EM-001' to 'Platform checks like em-001' so the file consistently uses the manifest's literal id: em-001 casing everywhere.
There was a problem hiding this comment.
Fixed in a309a25 — that sentence now uses em-001 to match the manifest id: fields and the First ship section.
| | Custom policy for one org or a BYOA agent | Local override in the consumer repo | | ||
|
|
||
| Companion platform PR: [fullsend-ai/fullsend#6036](https://github.com/fullsend-ai/fullsend/pull/6036) | ||
| ([ADR 0087](https://github.com/fullsend-ai/fullsend/blob/main/docs/ADRs/0087-eval-measurements-online-trace-scoring.md) |
There was a problem hiding this comment.
MEDIUM: ADR 0087 hyperlink 404s (target doesn't exist on fullsend main)
Both this line and eval/README.md:151 render a live markdown hyperlink to https://github.com/fullsend-ai/fullsend/blob/main/docs/ADRs/0087-eval-measurements-online-trace-scoring.md. Verified with curl (https://raw.githubusercontent.com/fullsend-ai/fullsend/main/docs/ADRs/0087-eval-measurements-online-trace-scoring.md -> HTTP 404) and via gh pr view 6036 --repo fullsend-ai/fullsend (state OPEN, mergedAt null) that ADR 0087 only exists on the still-unmerged companion PR fullsend-ai/fullsend#6036. This is distinct from the two already-posted review comments on this PR (one asked to add a direct URL 'when the ADR is published', the other flagged plain-text-vs-hyperlink inconsistency) — commit 228c7b5 ('docs(eval): hyperlink ADR 0087 in eval README', current head) added the real hyperlink before the ADR was published, so the link now renders and 404s for anyone who clicks it prior to #6036 merging.
Suggestion: Hold merging this PR until fullsend-ai/fullsend#6036 lands, or temporarily point both links at the PR itself (https://github.com/fullsend-ai/fullsend/pull/6036) and swap to the permanent docs/ADRs/0087 URL once #6036 merges.
There was a problem hiding this comment.
Fixed in a309a25. Both eval/measurements/README.md and eval/README.md no longer hyperlink docs/ADRs/0087-*.md on fullsend main (still 404 while #6036 is open). They point at fullsend-ai/fullsend#6036 and say ADR 0087 lands with that PR.
Merge order stays engine first (fullsend#6036) then this PR; we can swap to the permanent ADR blob URL after #6036 is on main if we still want a direct file link.
228c7b5 to
b1de714
Compare
|
@waynesun09 on the 10 Aug review-body finding (no CI for Fixed in b1de714. Added It fails CI on: unknown |
|
🤖 Finished Review · ✅ Success · Started 7:33 PM UTC · Completed 7:48 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 5 findings (0 duplicates skipped after checking existing inline comments/reviews on this PR).
| Managed fullsend jobs resolve manifests as: | ||
|
|
||
| 1. Local `${FULLSEND_DIR}/eval/measurements/${AGENT}.yaml` if present (override / BYOA) | ||
| 2. Else this repo at the `v0` pin: |
There was a problem hiding this comment.
HIGH: README documents an insecure/unpinned raw-URL manifest fetch that contradicts the companion PR's actual (tested) implementation
README states as fact that managed jobs fetch stock manifests via https://raw.githubusercontent.com/fullsend-ai/agents/v0/eval/measurements/${AGENT}.yaml. Verified against the companion PR fullsend-ai/fullsend#6036 (head 3d5aefc6) that this repo's PR is coupled to: action.yml line ~422 explicitly comments "For GetRef of agents@v0 (SHA pin). Not sent to raw.githubusercontent.com." and requires GH_TOKEN; internal/cli/evalmeasure.go warns "No GH_TOKEN/GITHUB_TOKEN; agents@v0 GetRef is unauthenticated"; and internal/cli/evalmeasure_test.go::TestActionYML_EvalMeasureNoFloatingV0Curl asserts action.yml does NOT contain the string raw.githubusercontent.com/fullsend-ai/agents/v0/eval/measurements. The actual mechanism is an authenticated, SHA-pinned GitHub GetRef call, not the unauthenticated floating-v0 curl this README asserts as fact. This claim is still present unmodified at current head and has not been raised or addressed in any existing review thread on this PR.
Suggestion: Update the README to describe the actual mechanism (SHA-pinned GitHub API GetRef requiring GH_TOKEN/GITHUB_TOKEN), or defer to fullsend-ai/fullsend's own docs for the resolution mechanism instead of asserting a specific (incorrect) URL scheme here.
There was a problem hiding this comment.
Fixed in 2a54535. The README no longer claims a floating raw.githubusercontent.com/fullsend-ai/agents/v0/... fetch. It now matches fullsend#6036: local FULLSEND_DIR override, else SHA-pin tags/v0 via GitHub GetRef (managed jobs pass GH_TOKEN / GITHUB_TOKEN) and fetch eval/measurements/${AGENT}.yaml at that commit.
| name="$(basename "$yaml_file")" | ||
| stem="${name%.yaml}" | ||
|
|
||
| mapfile -t parsed < <(parse_manifest "$yaml_file") |
There was a problem hiding this comment.
HIGH: New pre-commit-hook script uses Bash 4+ constructs (mapfile, declare -A), breaking the repo's established macOS bash-3.2 compatibility convention for locally-executed hooks
eval/lint-measurements.sh uses mapfile -t parsed < <(...) (line 92) and declare -A seen_ids=() (line 122), both requiring Bash >=4. It is wired into .pre-commit-config.yaml as a language: script local hook (entry: ./eval/lint-measurements.sh, files: ^(eval/measurements/|eval/lint-measurements)), meaning it runs automatically on every local commit touching those paths, on whatever bash the contributor's machine provides. This repo has an established, explicit convention of avoiding these exact constructs for such scripts: scripts/lib/gitleaks-install.lib.sh (used by pre-commit-invoked scripts/post-fix.sh/scripts/post-code.sh) states in a comment "Uses case statements (not declare -A / mapfile) so the script runs on bash 3.2 (macOS system bash)", and the sibling pre-commit hook script hack/lint-agent-docs likewise avoids mapfile/declare -A. (Note: mapfile/declare -A do appear elsewhere in the repo, e.g. scripts/bundle-sh.sh and .github/scripts/select-eval-agents.sh, but neither of those is wired as an automatic local pre-commit hook the way this new script is — they run via make or in CI.) On stock macOS /usr/bin/bash (3.2.57), this hook will hard-fail with mapfile: command not found / declare: -A: invalid option, blocking local commits to eval/measurements/*.yaml even though CI (Linux, bash 5) passes fine.
Suggestion: Rewrite the parser to avoid mapfile/declare -A (e.g. plain indexed arrays, a temp file, or a case-statement pattern like scripts/lib/gitleaks-install.lib.sh), consistent with this repo's existing convention for pre-commit-invoked scripts, or explicitly document/enforce a Bash >=4 requirement for this hook if the team decides to break that convention.
There was a problem hiding this comment.
Fixed in 2a54535. eval/lint-measurements.sh is a bash 3.2 wrapper that python3s the checks — no mapfile, no declare -A. Same pattern as keeping pre-commit hooks runnable on macOS /usr/bin/bash.
| file_errors=$((file_errors + 1)) | ||
| continue | ||
| fi | ||
| if [[ ! "$mid" =~ ^[a-z][a-z0-9]*-[0-9]+$ ]]; then |
There was a problem hiding this comment.
MEDIUM: Lint enforces a stricter measurement-id format than fullsend's actual manifest loader requires
The lint hard-fails any measurement id not matching ^[a-z][a-z0-9]*-[0-9]+$ ("must be lowercase like em-001"). Verified against the companion PR's actual LoadRegistry validation (fullsend-ai/fullsend#6036, internal/evalmeasure/registry.go): it only requires ID != "" and rejects pipe/newline characters — there is no lowercase/hyphen/digit-suffix requirement in the accepted contract. This id-format convention is invented in this repo's linter and enforced as a hard failure with no indication it's a local-only style rule rather than an upstream-mirrored constraint, so it could reject an otherwise-valid id that fullsend's loader would accept.
Suggestion: Document this as an agents-repo-only style convention independent of fullsend's validation, or relax the regex so it doesn't reject ids that fullsend's registry would accept.
There was a problem hiding this comment.
Kept the em-001 lowercase style for stock manifests in this repo (it is the casing this PR standardized), and documented it as an agents-repo style convention stricter than fullsend LoadRegistry (non-empty id, no pipe/newline). The linter header, README, and the error string now say that explicitly so it is not mistaken for an upstream contract.
| name="$(basename "$yaml_file")" | ||
| stem="${name%.yaml}" | ||
|
|
||
| mapfile -t parsed < <(parse_manifest "$yaml_file") |
There was a problem hiding this comment.
MEDIUM: Python parser subprocess failures inside process substitution are swallowed under set -e, producing a misleading error
mapfile -t parsed < <(parse_manifest "$yaml_file") uses process substitution. The script sets set -euo pipefail (line 14), but a non-zero exit from a command substituted via <(...) is not propagated to set -e in the calling shell (a well-known bash limitation — set -e does not observe failures inside process substitutions). If the embedded Python heredoc raises (e.g. a UTF-8 decode error or unexpected traceback), the script won't fail with a clear parser error; parsed ends up empty/short, agent resolves to an empty string, and the script instead reports the misleading "missing agent: field" error (lines 98-102) rather than the real parser crash.
Suggestion: Capture the Python subprocess's exit code explicitly — e.g. parsed_raw=$(parse_manifest "$yaml_file") || { echo "ERROR: $name: parser failed"; exit 1; } using command substitution instead of process substitution — so a genuine parser crash is reported distinctly from a malformed manifest.
There was a problem hiding this comment.
Fixed in 2a54535. The bash wrapper uses command substitution into python3 (not process substitution / mapfile), so a parser crash exits non-zero and prints parser failed instead of a fake missing-agent: error.
| exit 1 | ||
| fi | ||
|
|
||
| parse_manifest() { |
There was a problem hiding this comment.
MEDIUM: Hand-rolled line-oriented regex parser is not real YAML and will silently misparse as the manifest schema grows
The embedded Python parser (lines 35-85) is a regex/state-machine line scanner, not a YAML parser: it only recognizes block-style lists and a fixed id|scorer|version key set. eval/measurements/README.md (lines ~35-39 in its "What lives where" table) already anticipates a future declarative assert: block under a measurement item; with the current parser, any nested - line under in_measurements (lines 56-64) is treated as a new top-level measurement rather than a nested structure, silently producing a wrong count and confusing errors instead of a clean parse failure. Similarly, valid flow-style YAML (e.g. measurements: [{id: em-001, ...}]) that the real runtime parser (yaml.v3 in fullsend's LoadRegistry) accepts fine is invisible to this parser and reported as the generic "measurements list is empty" error.
Suggestion: Either constrain the parser strictly to the currently-shipped three-field block-style shape and document that assumption prominently (failing closed with a clear "unsupported YAML shape" error when zero items are extracted from a non-empty measurements: key), or parse with a real YAML library (e.g. PyYAML) inside the Python heredoc so the lint stays correct as the schema grows.
There was a problem hiding this comment.
Fixed in 2a54535. The parser is still a constrained reader for the shipped three-field block-style schema (no PyYAML in this repo's lint CI), but it now fails closed on anything else: flow-style measurements: [...], nested lists, and extra fields such as future assert:. Tests cover flow-style and nested assert:.
b1de714 to
2a54535
Compare
|
Review-agent sticky findings on
|
|
🤖 Finished Review · ❌ Failure · Started 1:54 AM UTC · Completed 1:56 AM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep on head 2a545357 — 5 findings (0 duplicates skipped after checking existing inline comments/reviews on this PR). All findings are inline; each is verified against this head and against the companion engine PR fullsend#6036 @ 61b9bae7.
| continue | ||
| if um and um.group(1) not in FIELD_KEYS: | ||
| raise UnsupportedShape("unsupported field %r" % um.group(1)) | ||
| if re.match(r"^\S", stripped): |
There was a problem hiding this comment.
MEDIUM: Linter fails OPEN on unknown top-level keys after the measurements list, contradicting the "fails closed" claim
The list-exit branch at lines 96-100 (if re.match(r"^\S", stripped): in_measurements = False; items.append(current); current = None; continue) consumes the first non-indented line after the measurements list without validating it, so the unsupported-top-level-field check at lines 102-103 is never reached for it. Once in_measurements is False, every subsequent indented line also falls through all branches silently.
Reproduced twice against head 2a545357 (linter run with MEASUREMENTS_DIR/AGENTS_DIR fixtures):
agent: code+ a valid measurement + trailingnot_a_real_key: true->code.yaml: OK (agent=code, 1 measurement(s)), exit 0. The same key placed beforemeasurements:correctly errors with "unsupported top-level field".- Worse, the realistic typo case: a valid block followed by a typo'd
mesurements:key holdingscorer: totally_bogus_scoreralso reportsOK, exit 0.
This falsifies the header comment at lines 20-21 ("other shapes fail closed as 'unsupported YAML shape'") and the stated goal of commit 2a545357. It is not covered by the existing line-35 thread (whose substance was "hand-rolled parser is not real YAML") — this is a specific verified fail-open bug in the replacement parser that the thread's reply ("Fixed in 2a54535 ... it now fails closed on anything else") introduced.
End-to-end there is no second net: fullsend's LoadRegistry (internal/evalmeasure/registry.go @ 61b9bae7) uses plain yaml.Unmarshal with no KnownFields(true), so yaml.v3 silently drops the unknown key at runtime too — neither lint nor runtime surfaces the typo, which is exactly the silent-zero-score class this linter exists to catch.
Suggested fix: Re-dispatch the terminating line through the top-level-field validation instead of continue-ing past it — factor lines 102-103 into a helper and call it from both the loop fallthrough and the end-of-list branch. Also treat any indented line seen while in_measurements is False as UnsupportedShape rather than ignoring it. Add regression cases to eval/lint-measurements-test.sh for a trailing unknown top-level key and for the mesurements: typo.
There was a problem hiding this comment.
Fixed in 70e7591. Ending the measurements list now re-dispatches the terminating line through top-level validation instead of continue-ing past it. Trailing not_a_real_key: and a typo'd mesurements: after a valid block both fail as unsupported top-level field. Regression cases added in eval/lint-measurements-test.sh.
| MEASUREMENTS_DIR, AGENTS_DIR = sys.argv[1], sys.argv[2] | ||
| KNOWN_SCORERS = frozenset({"trace_fitness"}) | ||
| ID_STYLE = re.compile(r"^[a-z][a-z0-9]*-[0-9]+$") | ||
| FIELD_KEYS = frozenset({"id", "scorer", "version"}) |
There was a problem hiding this comment.
MEDIUM: Linter rejects name:, a field the fullsend engine supports today (not a future field)
FIELD_KEYS = frozenset({"id", "scorer", "version"}) causes any other measurement key to raise UnsupportedShape at line 94. Reproduced against head 2a545357: a manifest with name: Trace Fitness fails with unsupported YAML shape (unsupported field 'name'), exit 1.
Verified against fullsend#6036 head 61b9bae7, internal/evalmeasure/registry.go: MeasurementSpec declares Name string `yaml:"name"` // optional display override; default = Scorer. It is consumed by evalName(), which ScoreTrace passes to ScoreFitnessNamed, and LoadRegistry explicitly validates it (rejects pipe/newline in .name). So name is a functional, shipped engine field — a manifest that is valid and meaningful to the engine hard-fails agents CI with a misleading message.
This is distinguishable from the existing "fail closed on extra fields" reply (comment 3800328729), which justified the strict shape by pointing at a future assert: block. assert: does not exist engine-side; Name does, today.
Related: eval/measurements/README.md:26 ("fullsend's loader only requires a non-empty id") also understates LoadRegistry, which additionally requires non-empty scorer, version >= 1, and no pipe/newline in id/scorer/name.
Suggested fix: Add name to FIELD_KEYS and validate it for pipe/newline the way LoadRegistry does. If excluding it from stock manifests is deliberate, say so explicitly in the script header and README ("stock manifests deliberately omit the engine's optional name: override") rather than reporting it as an unsupported YAML shape. Also correct the README.md:26 parenthetical to match LoadRegistry's real contract.
There was a problem hiding this comment.
Fixed in 70e7591. name is in FIELD_KEYS and rejected for pipe/newline the same way fullsend LoadRegistry does. Optional name: Trace Fitness now lints green. README parenthetical updated to the real contract (non-empty id/scorer, version>=1, no pipe/newline in id/scorer/name).
| print(" ERROR: %s: agent %r does not match filename stem %r (jobs fetch ${AGENT}.yaml)" % (name, agent, stem)) | ||
| errors += 1 | ||
| file_errors += 1 | ||
| if not os.path.isfile(os.path.join(AGENTS_DIR, agent + ".md")): |
There was a problem hiding this comment.
MEDIUM: A scribe.yaml would lint green but can never be fetched, producing the exact silent-zero-score outcome the linter targets
The agent-name check at line 148 accepts any agent: value that has a matching agents/<name>.md. agents/scribe.md exists in this repo, so a scribe.yaml passes cleanly — reproduced against head 2a545357: scribe.yaml: OK (agent=scribe, 1 measurement(s)), exit 0.
But the engine gates the manifest fetch on a narrower list. Verified in fullsend#6036 head 61b9bae7, internal/cli/run.go: defaultAgentsRepoKnownAgents = {triage, code, fix, review, retro, prioritize} (no scribe), and tryAgentsRepoMeasurementManifest returns early for anything not in that map, so eval/measurements/scribe.yaml is never fetched.
The trap is live because eval/measurements/README.md frames scribe's absence as a policy choice ("scribe has no forge work-item identity today"), inviting a contributor to just add the file — it would lint green, merge, and silently score nothing forever. The same trap applies to any future stock agent added here before the engine map is updated.
Suggested fix: Do not hard-code the engine's fetchable set in the linter (that map is explicitly transitional per its own comment — ADR 0058 Phase 5 removes it). Instead add a note to eval/measurements/README.md, and a comment beside the agents/<name>.md check, stating that a manifest only takes effect for agents in fullsend's first-party fetch allow-list (defaultAgentsRepoKnownAgents in internal/cli/run.go), so adding scribe requires a fullsend change first.
There was a problem hiding this comment.
Documented in 70e7591 (did not hard-code the engine allow-list in the linter — that map is transitional per ADR 0058 Phase 5). README First ship + the linter header now say a file only takes effect for agents in fullsend defaultAgentsRepoKnownAgents (the six we ship); agents/<name>.md alone is not enough, so scribe needs a fullsend change first.
| | Change | PR | | ||
| |---|---| | ||
| | New Go scorer or (future) new declarative `assert:` | `fullsend` | | ||
| | New measurement id / enable / disable / thresholds for a stock agent using an existing scorer | **agents** (this repo) | |
There was a problem hiding this comment.
MEDIUM: Change-guide claims "thresholds" are an agents-repo change, but no threshold field exists in the schema
Line 44 states: | New measurement id / enable / disable / thresholds for a stock agent using an existing scorer | **agents** (this repo) |.
Verified against fullsend#6036 head 61b9bae7: MeasurementSpec in internal/evalmeasure/registry.go has only ID, Scorer, Name, Version — there is no threshold field, and ScoreTrace dispatches purely on Scorer. This PR's own linter would reject a thresholds: key as "unsupported YAML shape" (FIELD_KEYS at eval/lint-measurements.sh:42), and nothing in this PR's diff implements or tests such a field. The fullsend guide docs/guides/infrastructure/eval-measurements.md itself lists declarative checks / ratio-threshold bands as "Planned (not in first ship)".
The table is internally inconsistent on this point: the adjacent fullsend row at line 43 correctly marks assert: as "(future)", while the agents row leaves "thresholds" unmarked as if it ships today. A contributor following this table would open an agents PR that cannot pass lint and would have no runtime effect.
Suggested fix: Drop "thresholds" from the agents-side row, or mark it "(future)" the same way the adjacent row marks assert:, mirroring the fullsend guide's "Planned (not in first ship)" wording.
There was a problem hiding this comment.
Fixed in 70e7591. Dropped "thresholds" from the agents-side change-guide row. Thresholds / declarative assert: stay on the fullsend row as "(future)", matching the fullsend guide's "Planned (not in first ship)".
| 1. Local `${FULLSEND_DIR}/eval/measurements/${AGENT}.yaml` if present (override / BYOA) | ||
| 2. Else a SHA-pinned fetch from `fullsend-ai/agents` at the `v0` tag: | ||
| `fullsend eval-measure` resolves `tags/v0` via GitHub `GetRef` (managed jobs | ||
| pass `GH_TOKEN` / `GITHUB_TOKEN`) and then fetches |
There was a problem hiding this comment.
MEDIUM: "managed jobs pass GH_TOKEN / GITHUB_TOKEN" is false for GitLab-managed jobs
Lines 16-20 state that the SHA-pinned tags/v0 GetRef works because "managed jobs pass GH_TOKEN / GITHUB_TOKEN", with no forge qualification. That holds for GitHub Actions but not GitLab.
Verified against fullsend#6036 head 61b9bae7:
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:296-297comments: "This job has no GH_TOKEN; stock-agent manifests skip unless an operator exports GH_TOKEN or GITHUB_TOKEN. Local override still works."internal/cli/evalmeasure.go:204warns: "No GH_TOKEN/GITHUB_TOKEN; agents@v0 GetRef is unauthenticated. GitLab jobs skip stock manifests unless an operator wires a GitHub token."
So on GitLab the GetRef is unauthenticated/rate-limited and these stock manifests generally never load — the README asserts forge-wide default coverage the engine does not provide.
This is not a re-litigation of the resolved HIGH thread on line 16 (floating raw-URL wording, fixed in 2a54535): it is a new inaccuracy in the replacement text that fix introduced.
Suggested fix: Qualify per forge in that sentence: GitHub Actions jobs pass GH_TOKEN/GITHUB_TOKEN; GitLab jobs have no GitHub token by default, so stock manifests skip unless an operator wires one — use a local FULLSEND_DIR override there.
There was a problem hiding this comment.
Fixed in 70e7591. README now qualifies per forge: GitHub Actions jobs pass GH_TOKEN/GITHUB_TOKEN for the SHA-pinned GetRef; GitLab-managed jobs do not by default, so stock manifests skip unless an operator wires a GitHub token — use a local FULLSEND_DIR override there.
2a54535 to
70e7591
Compare
|
🤖 Finished Review · ✅ Success · Started 10:40 PM UTC · Completed 10:55 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep on head 70e7591 — 3 findings (0 duplicates skipped after checking existing inline comments/reviews on this PR). All findings are inline; each is verified against this head and against the companion engine PR fullsend#6036 @ 1b874261.
| current = None | ||
|
|
||
| for raw in text.splitlines(): | ||
| stripped = raw.split("#", 1)[0].rstrip() |
There was a problem hiding this comment.
MEDIUM: Linter's naive # stripping accepts values YAML reads literally — silent zero measurements, contradicting the "fail closed" header
Line 81 is stripped = raw.split("#", 1)[0].rstrip(), which treats every # as a comment start. YAML only starts a comment at line start or after whitespace, so code#x and trace_fitness#typo are plain scalars. This contradicts the header claim at lines 24-26 that non-shipped shapes "fail closed as 'unsupported YAML shape'".
Reproduced against PR head 70e7591 (linter run with fixture MEASUREMENTS_DIR/AGENTS_DIR) and against gopkg.in/yaml.v3 v3.0.1 using fullsend's exact Registry/MeasurementSpec struct tags from fullsend#6036 @ 1b874261 internal/evalmeasure/registry.go:
agent: code#x-> linter printscode.yaml: OK (agent=code, 1 measurement(s)), exit 0. yaml.v3:err=<nil>, Agent="code#x". InScoreTrace(registry.go:74) the guardname != "" && name != UnknownSentinel && !strings.EqualFold(name, reg.Agent)returnsnil— zero measurement rows, and the CLI prints the benign "No new measurements (already scored or no matching traces)" (internal/cli/evalmeasure.goprintMeasurementResults). Silent forever.scorer: trace_fitness#typo-> linter OK, exit 0. yaml.v3:Scorer="trace_fitness#typo"->ScoreTracedefault:branch -> askiprow, never a fitness score.
The pre-commit check-yaml --unsafe hook does not help: both files are valid YAML (verified with PyYAML — yaml.safe_load('agent: code#x') -> {'agent': 'code#x'}).
This is a residual hole in the same hand-rolled parser, not a regression: 70e7591 does fix what it claims (trailing unknown top-level key, mesurements: typo), and eval/lint-measurements-test.sh passes 14/14.
Suggestion: Match YAML's comment rule instead of splitting on any #: strip only a # at column 0 or preceded by whitespace (e.g. re.split(r'(?:^|\s)#', raw, 1)[0]), then raise UnsupportedShape on any residual # left inside an extracted agent/id/scorer/name value. Add regression cases for agent: code#x and scorer: trace_fitness#typo to eval/lint-measurements-test.sh, matching the convention used for the other fail-closed cases.
There was a problem hiding this comment.
Fixed in 3cf6c3d. Comment stripping now matches YAML (# only at column 0 or after whitespace). Residual # inside agent / id / scorer / name values raises UnsupportedShape. Regression cases: agent: code#x, scorer: trace_fitness#typo, and a real trailing comment still passes.
| print(" ERROR: %s: unknown scorer %r (allowed: %s)" % (name, scorer, ", ".join(sorted(KNOWN_SCORERS)))) | ||
| errors += 1 | ||
| file_errors += 1 | ||
| if not re.match(r"^[1-9][0-9]*$", version or ""): |
There was a problem hiding this comment.
MEDIUM: Linter green-lights two shapes LoadRegistry hard-rejects (quoted version:, duplicate top-level key) — fail-open job, no scores
Two shapes lint green at head 70e7591 but hard-fail yaml.Unmarshal in LoadRegistry. Verified by running gopkg.in/yaml.v3 v3.0.1 (the version pinned in fullsend go.mod) against fullsend#6036 @ 1b874261 internal/evalmeasure/registry.go struct tags, and by running the linter with fixture dirs:
version: "1"— values are unquoted by.strip("'\"")at lines 116/123 before the^[1-9][0-9]*$check at line 248, so it lintsOK, exit 0. yaml.v3:yaml: unmarshal errors: line 5: cannot unmarshal !!str `1` into int(MeasurementSpec.Versionisint).- Duplicate top-level key (e.g.
agent:twice) — the parser overwrites and reportsOK, exit 0. yaml.v3:yaml: unmarshal errors: line 2: mapping key "agent" already defined at line 1.
Neither is caught by the check-yaml --unsafe pre-commit hook: both are valid YAML, and PyYAML silently last-wins on duplicate keys (confirmed: yaml.safe_load('agent: code\nagent: code') -> {'agent': 'code'}, no error).
Because MeasureAndExport returns fmt.Errorf("load registry: %w", err) and the managed jobs are explicitly fail-open (.gitlab/ci/fullsend-agent.yml:399-403 fullsend eval-measure ... || true under the comment "Eval measurements (fail-open) ... Never fail the agent job"; continue-on-error in the GitHub action), the manifest merges, CI stays green, and every wild run produces zero measurements — only a warn line in job logs. This directly contradicts the header claims at lines 22-26 ("stricter than fullsend LoadRegistry", "other shapes fail closed") added by the fix commits.
(Tab indentation is NOT part of this finding — check-yaml does reject it in-repo, verified.)
Suggestion: Validate version against the RAW token before quote-stripping so "1" / '1' are rejected with a message naming the engine's int field. Track top-level keys already seen and raise UnsupportedShape on a second agent: / measurements:, since yaml.v3 treats duplicates as a hard error. Add both as regression cases in eval/lint-measurements-test.sh, and narrow the line-22/24-26 header claims to enumerate exactly which LoadRegistry rules are mirrored.
There was a problem hiding this comment.
Fixed in 3cf6c3d.
versionis validated on the raw token before quote-stripping —"1"/'1'fail with an unquoted-integer message (LoadRegistryintfield / yaml.v3).- Duplicate top-level
agent:/measurements:raiseUnsupportedShape(yaml.v3 hard error).
Header comments now enumerate exactly which LoadRegistry rules we mirror vs agents-repo style extras. Regression cases added for both shapes.
| fetches `eval/measurements/${AGENT}.yaml` at that commit. It does **not** | ||
| curl the floating `raw.githubusercontent.com/fullsend-ai/agents/v0/...` URL. | ||
| GitHub Actions jobs pass `GH_TOKEN` / `GITHUB_TOKEN` for that `GetRef`. | ||
| GitLab-managed jobs do not have a GitHub token by default, so stock |
There was a problem hiding this comment.
MEDIUM: README's new GitLab wording is now wrong in the opposite direction — engine moved to 1b874261 and reversed the behaviour
NEW drift, not a re-litigation of thread 3805744921 (README line 18). That thread was correct against fullsend#6036 @ 61b9bae7, and commit 70e7591 fixed it by writing what the engine said at that SHA. The engine has since changed under it: at fullsend#6036 head 1b874261 the behaviour is reversed, so the newly-added lines 20-23 are now false.
Current README lines 20-23: "GitHub Actions jobs pass GH_TOKEN / GITHUB_TOKEN for that GetRef. GitLab-managed jobs do not have a GitHub token by default, so stock manifests skip unless an operator wires one — use a local FULLSEND_DIR override there."
All three primary sources at 1b874261 contradict "stock manifests skip":
docs/ADRs/0087-eval-measurements-online-trace-scoring.md:123-124: "fetch from publicagents@v0even withoutGH_TOKEN(rate-limited); a token is recommended on shared runners."internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:382-384: "agents is public, so GetRef works without GH_TOKEN, but unauthenticated calls share GitHub's ~60 req/hr per-IP limit — export GH_TOKEN / GITHUB_TOKEN on busy shared runners. Local override skips the fetch." The job wires an unconditionalfullsend eval-measure ... || true(lines 399-403).internal/cli/evalmeasure.goevalMeasureFetchContext: on a missing token it does not skip — it emitsStepWarn("No GH_TOKEN/GITHUB_TOKEN; agents@v0 GetRef runs unauthenticated (public repo, ~60 req/hr per IP). Prefer a token on shared runners; local FULLSEND_DIR override skips the fetch.")and returnsgh.New(token)with the empty token.
So GitLab jobs do fetch stock manifests; the real failure mode is per-IP rate limiting on busy shared runners — a different operational instruction than "use a local FULLSEND_DIR override there", which tells GitLab operators to build an override they do not need. This is exactly the drift the PR body's own unchecked "Cross-check with fullsend#6036 / ADR 0087 before merge" item exists to catch.
The rest of the resolution documentation verified accurate at this head: local-first then SHA-pinned fetch, tags/v0 via GetRef (internal/cli/run.go:3867), the defaultAgentsRepoKnownAgents six-agent gate (run.go:3843), and the LoadRegistry contract restated at README lines 28-31.
Suggestion: Replace lines 20-23 with wording quoted from ADR 0087 rather than paraphrased, e.g.: "agents is public, so the GetRef works without a token on both GitHub Actions and GitLab; unauthenticated calls share GitHub's ~60 req/hr per-IP limit, so export GH_TOKEN/GITHUB_TOKEN on busy shared runners (GitHub Actions passes GH_TOKEN automatically). A local FULLSEND_DIR manifest skips the fetch entirely." Re-verify at whatever SHA #6036 finally merges at, then tick the cross-check box.
There was a problem hiding this comment.
Fixed in 3cf6c3d. Re-checked against fullsend#6036 current tip (evalMeasureFetchContext warn + ADR 0087 wording): agents is public, so GetRef works without a token on both forges; the real limit is ~60 req/hr per IP on busy shared runners. README now says that and that a local FULLSEND_DIR override skips the fetch entirely — no longer claims GitLab stock manifests skip by default.
|
After this merges, stock manifests still need a |
Introduce per-agent measurement manifest YAML files that select which eval measurement scorers run after managed agent jobs. Every agent except scribe gets trace_fitness (EM-001) at initial ship. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Lowercase EM-001 to em-001 in README to match manifest YAML files. Add --- document start markers to all measurement manifests for consistency with harness/*.yaml convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Replace ambiguous "every listed agent" phrasing with an explicit list of the six agents that enable trace_fitness at initial ship. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Spell out agents@v0 defaults vs local overrides, engine-vs-policy split with fullsend#6036, and when a change needs fullsend vs agents. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Match eval/measurements/README.md link style for ADR 0087 so the measurement manifest section uses consistent markdown hyperlinks. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Keep the measurement id casing aligned with manifest id fields. Point readers at fullsend#6036 for ADR 0087 until that PR lands on main. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Reject unknown scorers, missing fields, duplicate ids, and agent names that do not match agents/<name>.md so a typo fails CI instead of silently producing zero scores. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Rewrite the pre-commit linter in python3 so it runs on macOS bash 3.2, fails closed on unsupported YAML, and documents em-001 as an agents-repo style rule. Describe the SHA-pinned v0 GetRef fetch instead of a floating raw URL, drop eval/ --- markers, and mention eval/ in README and LOCAL.md. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Fail closed on unknown top-level keys after the measurements list, accept optional name: matching fullsend MeasurementSpec, drop the false thresholds claim, and document GitLab + fetch-allowlist limits. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Match YAML comment rules so code#x / trace_fitness#typo fail closed, reject quoted version and duplicate top-level keys that yaml.v3 rejects, and align the GitLab GetRef docs with the public unauthenticated fetch. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
70e7591 to
3cf6c3d
Compare
|
🤖 Finished Review · ✅ Success · Started 7:17 PM UTC · Completed 7:30 PM UTC Commit: |
| language: script | ||
| files: ^(harness/|docs/|hack/lint-agent-docs) | ||
| pass_filenames: false | ||
| - id: lint-measurements |
There was a problem hiding this comment.
[high] protected-path
This file is under a protected path (.pre-commit-config.yaml). The PR has no linked issue in fullsend-ai/agents providing authorization for modifying governance/infrastructure files. The change adds a lint-measurements pre-commit hook entry, which is consistent with the PR's intent and mirrors the existing lint-agent-docs hook pattern, but human approval is always required for protected-path changes regardless of context.
Suggested fix: Obtain explicit human approval for this infrastructure change, or link to a tracking issue in fullsend-ai/agents that authorizes the .pre-commit-config.yaml modification.
2 participants
Summary
Add default measurement manifests for stock fullsend agents under
eval/measurements/. These are the online-scoring policy that ships with the agents (“here iscode, and here is what we measure on wildcoderuns”).Companion engine PR: fullsend-ai/fullsend#6036 (ADR 0087,
fullsend eval-measure, EM-001trace_fitness).Ownership (please read)
eval/measurements/<agent>.yamlFULLSEND_DIR${FULLSEND_DIR}/eval/measurements/${AGENT}.yamlif present, else fetchagents@v0from this path. Stock-agent users do not copy these files into every repo.run-telemetry.jsonlproduced by fullsend.assert:→ fullsend; new id / enable / disable / thresholds for a stock agent on an existing scorer → this repo; org-specific policy → consumer override.Changes
eval/measurements/{code,fix,prioritize,retro,review,triage}.yaml— each enablestrace_fitness(em-001) @ version 1eval/measurements/README.md— ownership, resolution, when to PR whereeval/README.md— short section + links to fullsend#6036 / ADR 0087Test plan
agent+measurements[].{id,scorer,version})v0(or the pin fullsend uses) includes these paths so wild runs stop skipping measureNotes for reviewers
eval/<agent>/are unchanged and separate.v0), measure steps skip fail-open — that is expected.