Add initial deterministic trace grading framework#54678
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new trace-grader framework is not ready to merge yet: the inline-script execution path still has a sandbox-escape bug, and the user-facing config contract is already inconsistent on the maximum script size.
Blocking themes
- Inline grader source is interpolated directly into generated code before
vm.runInContext, which undermines the security model for “trusted” scripts and makes the sandbox easy to bypass. - The compiler and reference docs disagree on the script-length limit, so the feature contract is not deterministic for users.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 25.5 AIC · ⌖ 7.03 AIC · ⊞ 7K
Comment /review to run again
| }); | ||
|
|
||
| // Wrap script as function body | ||
| const wrappedScript = `(function(trace, run, workflow, config, helpers) { "use strict"; ${script} })`; |
There was a problem hiding this comment.
The inline grader sandbox is still trivially escapable because you interpolate the untrusted script directly into a function body before vm.runInContext, so a script containing backticks / ${...} can break out of the intended body and execute arbitrary host-side code before your sandbox restrictions matter.
💡 Why this blocks the feature
wrappedScript := \(function(...) { "use strict"; ${script} })`` treats the grader source as template-literal content, not opaque code. That means payloads such as }; this.constructor.constructor('return process')() // or any backtick-terminated variant can rewrite the wrapper itself, bypass the “no require/process/fetch” guarantees, and run outside the contract you document. The parser only blocks a few substrings, so this is a real sandbox-escape surface, not a theoretical one.
Use a transport that does not re-embed source inside another source string, for example compile the grader as its own script/module and call a known exported function, or JSON-stringify the body and recover it inside the VM instead of concatenating raw text into the wrapper.
| if isBuiltin { | ||
| return fmt.Errorf("graders.%s is a built-in grader and cannot have a custom script", id) | ||
| } | ||
| if len(s) > 4096 { |
There was a problem hiding this comment.
The compiler and docs disagree on the script size limit: the parser accepts 4096 characters while the public contract says custom graders are limited to 2 KB, so valid documented configs will be rejected or oversized configs will slip through depending on which component the user trusts.
💡 Tighten the contract in one place
pkg/workflow/graders_config.go enforces len(s) > 4096, but docs/src/content/docs/reference/trace-graders.md still says “≤2 KB”. That mismatch is guaranteed to create confusing compile-time failures and makes the safety boundary ambiguous for a feature that executes code.
Pick one limit, encode it as a shared constant if possible, and update both validation and docs together so users get a single, deterministic rule.
There was a problem hiding this comment.
Ponytail pass — over-engineering only, 3 findings:
sortStringsreimplementssort.Strings(already imported/used in the same file) with a bubble sort.ParseGradersFromFrontmatterpublic wrapper has zero callers in this diff or repo.runGraderlegacy adapter duplicates the real dispatch logic inrunBuiltinGrader/runCustomGraderand exists only for tests.
net: -25 lines possible.
Generated by ✂️ Ponytail Reviewer for #54678 · auto · 46.4 AIC · ⌖ 3.52 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_yaml_graders.go:227
stdlib: hand-rolled bubble sort for sortStrings. sort.Strings(disabledIDs), 1 line (package already imports sort and uses it elsewhere in this file).
pkg/workflow/graders_config.go:416
yagni: ParseGradersFromFrontmatter public wrapper has no callers anywhere in the diff or repo. Delete until an external caller exists.
actions/setup/js/trace_graders.cjs:512
yagni: runGrader legacy adapter duplicates runBuiltinGrader/runCustomGrader dispatch and is only referenced by tests, not by main(). Drop it and call the two real functions directly from tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,071 new lines) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /developer-security — requesting changes on correctness and security issues before this graduates from draft.
📋 Key Themes & Highlights
Key Themes
- Metric correctness:
gradeToolSuccessRateandgradeToolFailureCountuse asymmetric predicates (comment on line 248);gradeContextGrowthmixes input+output tokens in a way that doesn't match the stated intent (line 288) - Surprising UX: listing one built-in grader silently enables all nine — undocumented behaviour that will confuse users (line 253)
- Security: manifest JSON is single-quote-escaped before embedding in YAML, while the exec spec uses base64 — inconsistency leaves a potential injection surface for unusual JSON values (line 45 of
compiler_yaml_graders.go) - Test quality:
TestBuildGraderManifestcallslen()on a struct rather than its.Gradersslice; the proto-pollution test validates V8 built-in behaviour rather than any gh-aw code - Minor: hand-rolled bubble sort in
sortStringsduplicatessort.Stringsfrom the same package;gradeLoopsdescription vs. implementation mismatch
Positive Highlights
- ✅ Excellent sandbox hardening for custom scripts:
codeGeneration: {strings: false, wasm: false}, frozen trace,Math.randomremoval - ✅ Single preprocessing pass for all graders — clean design that avoids redundant file I/O
- ✅ Base64 encoding of exec spec prevents YAML/JS injection for the script payload
- ✅ Strong test coverage overall — 567 lines of JS tests and 381 lines of Go tests for a new feature
- ✅
continue-on-error: trueon the graders step correctly avoids blocking CI on metric failures
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 102.3 AIC · ⌖ 12.1 AIC · ⊞ 7.6K
Comment /matt to run again
| */ | ||
| function gradeToolSuccessRate(trace) { | ||
| if (trace.toolCalls.length === 0) return 1; | ||
| const successes = trace.toolCalls.filter(t => t.success === true || (t.success !== false && t.status !== "error" && t.status !== "failure" && t.error === undefined)).length; |
There was a problem hiding this comment.
[/codebase-design] gradeToolSuccessRate and gradeToolFailureCount use independent, asymmetric success/failure heuristics — a tool with success !== false and no error field counts as a success, but the same logic doesn't guarantee it counts as a non-failure. This means successRate + failureCount/total can differ from 1 on ambiguous entries, undermining comparability of the two metrics.
💡 Suggestion: extract a shared predicate
function isToolFailure(t) {
return t.success === false || t.status === "error" || t.status === "failure" || t.error !== undefined;
}
function gradeToolSuccessRate(trace) {
if (trace.toolCalls.length === 0) return 1;
return (trace.toolCalls.length - trace.toolCalls.filter(isToolFailure).length) / trace.toolCalls.length;
}
function gradeToolFailureCount(trace) {
return trace.toolCalls.filter(isToolFailure).length;
}This guarantees successRate === 1 - failureCount/total and makes the invariant expressible in a single test.
@copilot please address this.
| } | ||
|
|
||
| /** @param {PreprocessedTrace} trace @returns {number} */ | ||
| function gradeLoops(trace) { |
There was a problem hiding this comment.
[/codebase-design] gradeLoops counts only consecutive identical calls (same name + args), so interleaved repeated calls — which are also a looping symptom — go undetected. The metric name "Loops" implies a broader pattern, but the implementation is narrower than the description suggests.
💡 Suggestion
Either rename to consecutive-identical-calls to match what the code actually measures, or document the intentional constraint in the JSDoc so callers understand why [read /a, write /b, read /a] scores 0 loops. A test for the non-consecutive case would also pin down the contract.
@copilot please address this.
| } | ||
|
|
||
| /** @param {PreprocessedTrace} trace @returns {number} */ | ||
| function gradeTrajectoryEfficiency(trace) { |
There was a problem hiding this comment.
[/codebase-design] gradeTrajectoryEfficiency uses uniqueTools / totalCalls, capped at 1. For any session with only one tool call this returns 1 (trivially "perfect"), but single-call sessions are likely the least interesting for diversity analysis. The metric conflates diversity with efficiency in a way that may not be meaningful at extremes.
💡 Suggestion
Document the known edge-case behaviour in the JSDoc. If the intent is to measure diversity of tool usage rather than efficiency, consider returning null/unavailable when toolCalls.length < 2 (mirroring how gradeContextGrowth handles < 2 entries). A test for the single-call case would help nail down the contract.
@copilot please address this.
| execB64 := base64.StdEncoding.EncodeToString(execJSON) | ||
|
|
||
| // Escape single quotes for embedding in the YAML script block | ||
| escapedManifest := strings.ReplaceAll(string(manifestJSON), "'", "\\'") |
There was a problem hiding this comment.
[/codebase-design] escapedManifest uses strings.ReplaceAll(manifestJSON, "'", "\\'") to embed the manifest JSON directly in the generated YAML script: block as a single-quoted JS argument. JSON values can contain characters like backticks, ${}, or \n that won't be escaped by this single-quote substitution, potentially producing invalid or injectable YAML/JS at compile time.
💡 Suggestion
The exec spec already uses base64 encoding for safety. Apply the same pattern to the manifest — pass manifestB64 to main() and Buffer.from(arg, 'base64').toString() in the JS side — eliminating the need to trust that the manifest JSON round-trips safely through a YAML block scalar.
@copilot please address this.
| }, | ||
| } | ||
| entries := buildGraderManifest(cfg) | ||
| if len(entries) != 3 { |
There was a problem hiding this comment.
[/tdd] The TestBuildGraderManifest test asserts len(entries) != 3 but buildGraderManifest returns a *graderManifest struct — the test is comparing len(entries) where entries is the whole struct, not its .Graders slice. It should be len(entries.Graders).
💡 Fix
manifest := buildGraderManifest(cfg)
if len(manifest.Graders) != 3 {
t.Fatalf("expected 3 entries, got %d", len(manifest.Graders))
}
// and for JSON round-trip:
data, err := json.Marshal(manifest.Graders)As written the test will always pass because len on a struct pointer is a compile error — check whether this compiles and runs today.
@copilot please address this.
| } | ||
|
|
||
| // sortStrings sorts a string slice in place. | ||
| func sortStrings(s []string) { |
There was a problem hiding this comment.
[/codebase-design] sortStrings in compiler_yaml_graders.go is a hand-rolled O(n2) insertion sort. The standard library sort.Strings is already imported in graders_config.go in the same package and does the same job faster and more readably.
💡 Suggestion
Replace the 9-line bubble sort with:
import "sort"
// ...
sort.Strings(disabledIDs)@copilot please address this.
| if len(s) > 4096 { | ||
| return fmt.Errorf("graders.%s.script exceeds maximum length of 4096 characters (%d)", id, len(s)) | ||
| } | ||
| forbiddenPatterns := []string{"require(", "import(", "import ", "fetch(", "eval(", "process.exit", "child_process", "execSync", "spawnSync", "Function("} |
There was a problem hiding this comment.
[/developer-security] The forbidden-pattern list in parseGraderEntryFields uses simple strings.Contains substring matching. Patterns like "require(" would miss require ( "fs" ) (space before paren) or Unicode look-alike characters. At compile time this is a defence-in-depth check (the VM sandbox provides the real enforcement), but misleading error messages ("script contains forbidden pattern") could confuse users who hit a false negative.
💡 Suggestion
Add a comment making the defence-in-depth nature explicit, and consider compiling a single regexp for the patterns so they can be anchored word-boundaries (e.g. \brequire\s*\() to reduce both false negatives and false positives. Alternatively, document that the VM sandbox is the authoritative enforcement layer.
@copilot please address this.
| expect(gradeToolFailureCount(trace)).toBe(0); | ||
| }); | ||
|
|
||
| it("handles nested malicious JSON in JSONL", () => { |
There was a problem hiding this comment.
[/tdd] The hostile data test for proto-pollution (__proto__) asserts the parsed object exists and Object.prototype is unpolluted, but JSON.parse in modern Node already ignores __proto__ keys — the test passes vacuously and doesn't actually validate that the safeParseJsonl layer adds any additional protection. A more meaningful test would use Object.create(null) or check for the constructor key attack vector.
💡 Suggestion
Either remove the test (it's testing V8's built-in behaviour, not gh-aw code) or replace it with a test for a pattern your code does guard against — e.g. {"constructor":{"prototype":{"polluted":true}}} — and assert Object.prototype.polluted remains undefined.
@copilot please address this.
| function gradeExecutionDuration(trace) { return trace.totalDurationMs; } | ||
|
|
||
| /** @param {PreprocessedTrace} trace @returns {number} */ | ||
| function gradeContextGrowth(trace) { |
There was a problem hiding this comment.
[/tdd] gradeContextGrowth adds input_tokens + output_tokens from the first token-usage entry as the denominator — but that includes the first response's output tokens, which aren't part of the initial context. A context-growth metric is typically lastInputTokens / firstInputTokens. The current formula mixes apples and oranges and may underreport growth for long sessions where output is large.
💡 Suggestion
Consider using only input_tokens for both numerator and denominator, or document the current definition explicitly so callers understand what "context growth" means in this metric. A test that shows growth when input tokens increase across steps (not just total tokens) would help verify the intent.
@copilot please address this.
| break | ||
| } | ||
| } | ||
| if hasAnyBuiltin { |
There was a problem hiding this comment.
[/codebase-design] parseGradersFromFrontmatter auto-fills missing built-ins when hasAnyBuiltin is true (lines 253–260), but this logic is silent — the caller gets more graders than they declared without any log or documented behaviour. This is surprising: a user who writes graders:\n retries:\n threshold: 5 will silently get all 9 built-ins, not just retries.
💡 Suggestion
Document this "partial selection enables all built-ins" behaviour in the function comment and in docs/src/content/docs/reference/trace-graders.md. Alternatively, consider only enabling the explicitly listed graders (zero-config {} for all; explicit list for subset) to match the principle of least surprise. If the current behaviour is intentional, add a test that asserts listing one built-in produces all built-ins.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds opt-in deterministic trace grading to compiled agent workflows.
Changes:
- Parses grader configuration and generates post-agent grading steps.
- Adds built-in/custom graders, result persistence, redaction, and detection staging.
- Adds Go/JavaScript tests and reference documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores grader configuration. |
pkg/workflow/graders_config.go |
Defines and parses graders. |
pkg/workflow/graders_config_test.go |
Tests grader compilation behavior. |
pkg/workflow/frontmatter_types.go |
Adds the frontmatter field. |
pkg/workflow/compiler_yaml_post_agent.go |
Integrates post-agent grading. |
pkg/workflow/compiler_yaml_graders.go |
Generates grader and redaction steps. |
pkg/workflow/compiler_yaml_artifacts.go |
Adds grader fallback artifacts. |
pkg/workflow/compiler_orchestrator_workflow.go |
Extracts grader configuration. |
pkg/constants/job_constants.go |
Defines grader paths and filenames. |
docs/src/content/docs/reference/trace-graders.md |
Documents trace graders. |
actions/setup/sh/prepare_threat_detection_files.sh |
Stages grader files for detection. |
actions/setup/js/trace_graders.test.cjs |
Tests grading behavior and isolation. |
actions/setup/js/trace_graders.cjs |
Implements trace preprocessing and grading. |
actions/setup/js/redact_secrets.cjs |
Adds targeted grader-result redaction. |
.github/workflows/weekly-network-domains-audit.lock.yml |
Regenerates workflow runtime settings. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 12
- Review effort level: Balanced
| // Graders configures deterministic trace graders that compute metrics from | ||
| // post-agent trace files. Can be {} for zero-config (all built-ins) or a map | ||
| // of grader IDs with optional enabled/script overrides. | ||
| Graders any `json:"graders,omitempty"` |
| if entryRaw == nil { | ||
| cfg.Graders[id] = def | ||
| continue | ||
| } |
| yaml.WriteString(" const { setupGlobals } = require('" + SetupActionDestination + "/setup_globals.cjs');\n") | ||
| yaml.WriteString(" setupGlobals(core, github, context, exec, io, getOctokit);\n") | ||
| yaml.WriteString(" const { main } = require('" + SetupActionDestination + "/trace_graders.cjs');\n") | ||
| fmt.Fprintf(yaml, " await main('%s', '%s');\n", escapedManifest, execB64) |
| const fn = vm.runInContext(wrappedScript, ctx, { timeout: SCRIPT_TIMEOUT_MS, filename: `grader:${id}` }); | ||
| const rawResult = fn(frozenTrace, runCtx, workflowCtx, config, helpers); |
| const sandbox = { | ||
| Math: Object.freeze({ ...Math, random: undefined }), | ||
| JSON: Object.freeze({ parse: JSON.parse, stringify: JSON.stringify }), | ||
| Array, | ||
| Object, |
| script: "trace.toolCalls.filter(t => t.name === 'bash').length" | ||
| ``` | ||
|
|
||
| Custom scripts must be pure expressions (≤2 KB, no `require`, `import`, `fetch`, `eval`, or `process.exit`). |
| entries := buildGraderManifest(cfg) | ||
| if len(entries) != 3 { | ||
| t.Fatalf("expected 3 entries, got %d", len(entries)) | ||
| } | ||
|
|
||
| // Verify JSON serialization round-trips | ||
| data, err := json.Marshal(entries) | ||
| if err != nil { | ||
| t.Fatalf("json marshal error: %v", err) | ||
| } | ||
| var decoded []graderManifestEntry | ||
| if err := json.Unmarshal(data, &decoded); err != nil { | ||
| t.Fatalf("json unmarshal error: %v", err) | ||
| } | ||
| if len(decoded) != 3 { | ||
| t.Fatalf("expected 3 decoded entries, got %d", len(decoded)) | ||
| } |
| for id, entryRaw := range m { | ||
| id = strings.TrimSpace(id) | ||
| if !graderIDPattern.MatchString(id) { | ||
| return nil, fmt.Errorf("graders has invalid id %q: must match %s. Example:\ngraders:\n my-metric:\n script: \"return { value: trace.toolCalls.length }\"", id, graderIDPattern.String()) | ||
| } |
| const errResults = results.filter(r => r.error); | ||
| if (errResults.length > 0) { | ||
| const errLines = errResults.map(r => `- **${r.id}**: ${r.error}`).join("\n"); | ||
| core.summary.addDetails("Grader Errors", errLines); | ||
| } | ||
| await core.summary.write({ overwrite: false }); |
There was a problem hiding this comment.
Good structural work — the single preprocessing pass, base64-encoded exec spec for custom scripts, and deepFreeze/deepClone sandbox inputs are well-considered. A few blocking issues need addressing before merge.
Blocking issues:
- vm sandbox exposes
Object/Array— prototype-chain traversal can escape the sandbox; remove or document. (see inline) - Incomplete backslash escaping of manifest JSON — grader names/descriptions containing
\before'will produce broken JavaScript at runtime. (see inline)
Non-blocking issues:
3. runCtx.graderCount is always 0 — misleading comment, never populated. (see inline)
4. sortStrings reimplements sort.Strings with O(n2) — use stdlib. (see inline)
5. trajectory-efficiency naming is misleading — measures diversity, not efficiency. (see inline)
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 90.1 AIC · ⌖ 10.6 AIC · ⊞ 6.2K
| fs.mkdirSync(GRADERS_DIR, { recursive: true }); | ||
| fs.writeFileSync(MANIFEST_PATH, JSON.stringify(manifest, null, 2)); | ||
| } catch (err) { | ||
| core.warning(`Graders: failed to write manifest: ${getErrorMessage(err)}`); |
There was a problem hiding this comment.
Security: Object in the vm sandbox allows prototype-chain escape
Exposing bare Object (and Array) in a vm.createContext sandbox lets a script walk the prototype chain to reach outer-context constructors:
// inside grader script
const fn = Object.getPrototypeOf(Math).constructor;
const proc = fn("return process")();
// proc is the real Node.js process objectThe codeGeneration: { strings: false, wasm: false } option prevents new Function/eval from string literals, but not prototype-chain traversal. The forbidden-patterns block in Go (Function(, eval(, ...) doesn't block this vector.
Fix: Remove Object and Array from the sandbox — grader scripts don't need them since the frozen trace argument is already an array/object. If they must be kept, document that the sandbox is defence-in-depth only and that inline grader authors are trusted the same as workflow authors with secrets access.
@copilot please address this.
| * Main entry point. Called from the github-script step with manifest JSON and base64 exec spec. | ||
| * @param {string} manifestJson - JSON string of grader manifest | ||
| * @param {string} [execSpecB64] - Base64-encoded JSON array of {id, script} | ||
| */ |
There was a problem hiding this comment.
Correctness: runCtx.graderCount is always 0 — comment says "filled by caller" but it never is
const runCtx = deepFreeze({
graderCount: 0, // filled by caller ← never populated
});runCtx is created locally in runCustomGrader, frozen immediately, and never updated. Any grader script relying on run.graderCount will always see 0. Either populate it from the actual enabled-grader count before freezing, or remove the field to avoid setting false expectations.
@copilot please address this.
| escapedManifest := strings.ReplaceAll(string(manifestJSON), "'", "\\'") | ||
|
|
||
| yaml.WriteString(" - name: Run trace graders\n") | ||
| yaml.WriteString(" if: always()\n") |
There was a problem hiding this comment.
Correctness: manifest escaping is incomplete — backslash before single-quote breaks the JS string literal
escapedManifest := strings.ReplaceAll(string(manifestJSON), "'", "\\'")
fmt.Fprintf(yaml, " await main('%s', '%s');\n", escapedManifest, execB64)JSON can contain \\ (an escaped backslash in a string value). After only escaping ', the sequence \\' in the original JSON becomes \\\' — the JavaScript parser sees an escaped backslash (\\) followed by an unescaped single quote that terminates the string literal, causing a syntax error or silent truncation.
Fix: escape backslashes first, then single quotes:
escaped := strings.ReplaceAll(string(manifestJSON), `\`, `\\`)
escaped = strings.ReplaceAll(escaped, "'", `\'`)@copilot please address this.
| } | ||
| fmt.Fprintf(yaml, " GH_AW_SECRET_NAMES: '%s'\n", strings.Join(escapedRefs, ",")) | ||
| for _, secretName := range secretReferences { | ||
| escapedSecretName := escapeSingleQuoteBackslash(secretName) |
There was a problem hiding this comment.
Maintainability: sortStrings reimplements sort.Strings with an O(n2) selection sort
// sortStrings sorts a string slice in place.
func sortStrings(s []string) {
for i := 0; i < len(s); i++ {
for j := i + 1; j < len(s); j++ {
if s[j] < s[i] {
s[i], s[j] = s[j], s[i]sort is already imported in graders_config.go (same package). Drop this helper and call sort.Strings(disabledIDs) directly to use the stdlib's O(n log n) implementation and avoid dead code.
@copilot please address this.
| * @param {{name: string, unit: string, direction: string, threshold?: number, source: string, digest?: string}} meta | ||
| * @returns {GraderResult} | ||
| */ | ||
| function normalizeResult(id, rawResult, meta) { |
There was a problem hiding this comment.
Design: trajectory-efficiency measures tool diversity, not efficiency — the naming is misleading
function gradeTrajectoryEfficiency(trace) {
if (trace.toolCalls.length === 0) return 1;
const uniqueTools = new Set(trace.toolCalls.map(t => String(t.name || t.tool || "")));
return Math.min(1, uniqueTools.size / trace.toolCalls.length);
}This computes unique tool types ÷ total calls. An agent that calls 10 different tools once each scores 1.0; an agent that uses one tool efficiently 10 times scores 0.1. "Efficiency" normally implies fewer steps or less waste — diversity of tool types used is orthogonal to that.
The description ("higher = more diverse usage") partially clarifies it, but the ID trajectory-efficiency and name "Trajectory Efficiency" will cause users to misread results and set wrong thresholds.
Consider renaming to tool-diversity / "Tool Diversity", or at minimum updating the description to explicitly say "measures breadth of tool usage, not operational efficiency".
@copilot please address this.
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32544819956
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 0d6a86e. Completed the blocking runtime-safety work:
Validations rerun:
Remaining human follow-up:
|
|
@copilot the grader runs in GitHub Actions, so it should be written in JavaScript and run in a github-script action step. It does not run as part of the Go CLI. |
PR Triage
|
|
@-
|
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32564450205
|
Verified on current HEAD
Validations rerun:
CI log pass:
PR-finisher pass complete for this run. |
|
@copilot failed checks: impacted-go-tests: https://github.com/github/gh-aw/actions/runs/32558583439/job/97003175606; impacted-js-tests: https://github.com/github/gh-aw/actions/runs/32558583397/job/97003173010; lint-go-custom: https://github.com/github/gh-aw/actions/runs/32558583439/job/97003175641 ; please refresh the branch if needed and continue with the pr-finisher skill
|
I refreshed and revalidated on current HEAD
No code changes were required in this pass. Remaining maintainer follow-up: link ADR |
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32569036201
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented in
Validation rerun:
Branch status: refreshed and up to date with base (
|
|
@copilot sous-chef triage: please review this PR, refresh the branch if needed, address any unresolved review feedback, and use the pr-finisher skill before handing back for maintainer review.
|
PR-finisher pass completed on current HEAD
Validation rerun:
Maintainer handoff: CI on this HEAD remains stale after agent operations; please re-trigger workflows for fresh required-check signals. ADR link is still needed in PR body: |
PR TriageCategory: New deterministic trace grading framework, 3157 additions/27 files. CI green but 2x CHANGES_REQUESTED unresolved — needs author follow-up. Automated triage — run 32572524009
|
Summary
Adds the initial opt-in deterministic trace grading framework to the existing agent job, including frontmatter parsing, built-in metrics, inline JavaScript support, persisted grading files, detection staging, tests, and reference documentation.
Status
This draft is incomplete. Remaining work includes schema registration, full canonical manifest/result metadata and thresholds, restricted VM execution for inline scripts, generic detection interpretation, and final review/security validation.
Validation
run: https://github.com/github/gh-aw/actions/runs/32547644097> Generated by 👨🍳 PR Sous Chef · gpt54 · 19.9 AIC · ⌖ 9.76 AIC · ⊞ 9.5K · ◷
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32552825826> Generated by 👨🍳 PR Sous Chef · gpt54 · 24.2 AIC · ⌖ 8.28 AIC · ⊞ 9.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32558278813> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.36 AIC · ⌖ 8.18 AIC · ⊞ 9.5K · ◷
Branch refresh requested by PR Sous Chef.
Run: https://github.com/github/gh-aw/actions/runs/32569036201> Generated by 👨🍳 PR Sous Chef · gpt54 · 28.6 AIC · ⌖ 8.36 AIC · ⊞ 9.5K · ◷