Add tool output consumption rate trajectory grader#57252
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
There was a problem hiding this comment.
🟡 Changes recommended
Malformed array entries are incorrectly counted as valid consumption, inflating the reported rate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a trajectory grader that measures whether tool-originated observations are subsequently consumed.
Changes:
- Implements the consumption-rate grader and applicability handling.
- Adds grader tests.
- Marks catalog rank 12 implemented.
File summaries
| File | Description |
|---|---|
.github/workflows/shared/graders/tool-output-consumption-rate.md |
Defines the grader. |
actions/setup/js/trace_graders.test.cjs |
Tests grader behavior. |
.github/workflows/shared/graders/README.md |
Updates implementation status. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const consumed = toolObservations.filter( | ||
| observation => Array.isArray(observation.consumedByActionIds) && observation.consumedByActionIds.length > 0 | ||
| ); | ||
| const unconsumedIds = toolObservations | ||
| .filter(observation => !Array.isArray(observation.consumedByActionIds) || observation.consumedByActionIds.length === 0) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This one branch is more general than the current data model seems to need; trimming the fallback scaffolding would make the grader easier to read and maintain. net: -12 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57252 · codex · mai10 · 4.85 AIC · ⌖ 0.499 AIC · ⊞ 13.5K
Comment /ponytail to run again
| max: 1.0 | ||
| script: | | ||
| const isRecord = value => value !== null && typeof value === "object" && !Array.isArray(value); | ||
| const candidates = [ |
There was a problem hiding this comment.
L17-38: yagni: multi-branch trace-shape fallback chain for trajectoryIR, trajectoryIr, ir, and agentOutput variants. Collapse to the canonical trajectoryIR shape plus one nested agentOutput fallback.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting on test-coverage and extraction robustness. Overall a solid, well-tested addition; requesting minor changes.
📋 Key Themes & Highlights
Issues Found
- Missing
passedon success path — The grader's success-pathreturnomitspassed; both null-path returns set it explicitly. If the framework relies on it, this is a silent correctness gap. - Hardcoded
slice(6)for YAML indent extraction — Brittle against reformatting; should derive indent dynamically. - Misleading
it.eachrow label — "no tool calls" row actually tests the unmatched-call path already covered by the next row; the test intent is unclear.
Positive Highlights
- ✅ Comprehensive test suite covers normal scoring, malformed metadata, nested IR structures, and all not-applicable conditions
- ✅ Clear null-handling contract: returns
passed: nullrather than fabricating a value when provenance is absent - ✅ Multi-key IR candidate lookup (
trajectoryIR,trajectoryIr,ir, nested underagentOutput) is thorough and defensive - ✅ Inline HTML comment provides excellent context for future maintainers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 41.4 AIC · ⌖ 15.3 AIC · ⊞ 7.6K
Comment /matt to run again
|
|
||
| return { | ||
| value: helpers.ratio(consumed.length, toolObservations.length), | ||
| unit: "ratio", |
There was a problem hiding this comment.
[/tdd] The success-path return omits passed — if the grader framework expects it (even as null), this could silently yield an "unevaluated" result while both null-path returns explicitly set passed: null.
💡 Suggestion
Add a test that asserts the value of result.passed on the success path to lock in the contract:
it("scores the fraction of matching tool observations that were consumed", () => {
const result = runToolOutputConsumptionRate({ ... });
expect(result.value).toBeCloseTo(0.5);
expect(result.passed).toBeNull(); // or true/false depending on framework threshold rules
...
});If the framework derives passed from a threshold, add an explicit comment inside the grader's return stating that omission is intentional.
@copilot please address this.
| throw new Error("unable to extract tool-output-consumption-rate grader script"); | ||
| } | ||
| const toolOutputConsumptionRateScript = toolOutputConsumptionRateScriptMatch[1] | ||
| .split("\n") |
There was a problem hiding this comment.
[/tdd] The script extraction regex uses slice(6) to strip indentation from the YAML block — this is a hardcoded assumption about the YAML indentation level (6 spaces). If the file is reformatted or the indentation changes, the extracted script will be silently corrupted.
💡 Suggestion
Derive the indent length dynamically instead:
const lines = toolOutputConsumptionRateScriptMatch[1].split("\n");
const indent = lines[0].match(/^(\s*)/)[1].length;
const toolOutputConsumptionRateScript = lines.map(line => line.slice(indent)).join("\n");This makes the extractor robust to reformatting.
@copilot please address this.
| expect(result.value).toBe(1); | ||
| }); | ||
|
|
||
| it.each([ |
There was a problem hiding this comment.
[/tdd] The it.each table test for "no tool calls" sends observations with a sourceToolCallId that points to an unknown call — it ends up testing "no matching tool call" rather than the stated case. A true "no tool calls" case would have no toolCalls array at all and observations whose sourceToolCallId is also absent.
💡 Clarification
Current "no tool calls" fixture:
{ trajectoryIR: { observations: [{ id: "obs-1", sourceToolCallId: "tc-1", consumedByActionIds: ["act-1"] }] } }This observation does have a sourceToolCallId — it just can't be matched because toolCalls is missing. That exercises the "unmatched" branch, which is already covered by the "no matching tool call" row below it. Consider either renaming the row or using an observation with no sourceToolCallId to truly test the "no tool-originated observations" path from a missing-sourceToolCallId angle.
@copilot please address this.
There was a problem hiding this comment.
The grader logic and test coverage are solid overall, but there is one correctness issue.
Blocking: candidates is missing trace as the first entry — every peer grader (skill-constraint-coverage, exploration-error, etc.) probes the root trace object first. Omitting it means a trace where observations / toolCalls live at the top level of the trace (not nested under trajectoryIR) will always return passed: null instead of a real score. See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 48.2 AIC · ⌖ 14.7 AIC · ⊞ 6.2K
| trace.trajectoryIr, | ||
| trace.ir, | ||
| isRecord(trace.agentOutput) ? trace.agentOutput.trajectoryIR : null, | ||
| isRecord(trace.agentOutput) ? trace.agentOutput.trajectoryIr : null, |
There was a problem hiding this comment.
The candidates array omits trace itself as the first probe. Every other grader in this repo (e.g. skill-constraint-coverage) starts with trace before the nested-path candidates. If a caller stores observations/toolCalls directly on the root trace object, the grader will silently return passed: null ("no observations") instead of computing a score.
Suggest:
const candidates = [
trace, // ← add this first, matching skill-constraint-coverage pattern
trace.trajectoryIR,
trace.trajectoryIr,
trace.ir,
...
].filter(isRecord);A test covering observations at the top-level trace shape would confirm the fix.
@copilot please address this.
🧪 Test Quality Sentinel ReportSummary✅ Test Quality Score: 90/100 (Excellent) File: Quality Metrics
AnalysisTest Breakdown1. "scores the fraction of matching tool observations that were consumed"
2. "treats malformed consumption metadata as unconsumed"
3. "reads a complete IR nested in agentOutput"
4-6. "normalizes %s as unavailable" (parametrized 3×)
Strengths
Violations Check
Recommendation✅ APPROVE — Test Quality Sentinel approval.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
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. Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Blocking issue
The new grader still overstates consumption because malformed consumedByActionIds entries are treated as valid as long as the array is non-empty.
Why this blocks
The grader contract in this PR says malformed consumption metadata must count as unconsumed. Right now values like [null] or [42] increase the numerator, so broken trajectory IR can look healthier than it is. That turns the metric into a false-positive generator instead of a reliability signal. The tests only cover the non-array case, so this regression would ship unnoticed.
Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
codeload.github.comgithub.comregistry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 43.6 AIC · ⌖ 7.32 AIC · ⊞ 21.8K
Comment /review to run again
|
@copilot quick triage for this PR:
Key review checkpoints:
PR: #57252 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in 3f4b8b0 (see subsequent push): |
The built-in tool success rate does not reveal outputs fetched but never used. This adds a grader measuring consumed tool-originated observations over all tool-originated observations.
Grader contract
observations[].sourceToolCallIdagainsttoolCalls[].id.consumedByActionIdsas consumed.Applicability
passed: nullwhen observations or matching tool provenance are absent.Catalog