[trajectory-grader] Implement exploitation-error - #57152
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.
|
|
✅ 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
- "ab.chatgpt.com"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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
🔵 Needs a closer look
The protected grader and catalog changes explicitly require final human scrutiny before merge.
Pull request overview
Adds the exploitation-error trajectory grader for failures caused by unused evidence.
Changes:
- Implements and documents exploitation-error scoring.
- Adds eight JavaScript test cases.
- Updates catalog status and exploration-error references.
File summaries
| File | Description |
|---|---|
.github/workflows/shared/graders/exploitation-error.md |
Defines the new grader. |
.github/workflows/shared/graders/exploration-error.md |
Updates complementary-grader documentation. |
.github/workflows/shared/graders/README.md |
Marks the grader implemented. |
actions/setup/js/trace_graders.test.cjs |
Adds behavioral coverage. |
pkg/workflow/schemas/github-workflow.json |
Reorders an existing permission definition. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
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.
Verdict
Non-blocking review: I didn't find a changed-line issue here that clearly breaks grader behavior or leaves the new metric under-tested.
Themes checked
- The new grader mirrors the existing trajectory-IR candidate selection used by
exploration-error, so it does not introduce a divergent trace-resolution path. - The mutual-exclusion guard with
exploration-erroris explicit and covered by tests. - The added tests hit the null-applicability branches, the satisfied-objective fast path, the state fallback, and the candidate-preference behavior.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 27.8 AIC · ⌖ 7.33 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
Review: exploitation-error grader
Overall: The implementation is well-structured and the test suite covers all declared branches (not-applicable conditions, all-satisfied, deferred-to-exploration, unused-fraction, fallback to states[]).
One non-blocking correctness issue found:
In exploitation-error.md line 90, unmetDescriptions can emit the string literal "undefined" when an objective has neither a valid string id nor a description property, because objective.description evaluates to undefined and .join() coerces it. A simple three-way fallback to "(unnamed)" would harden this.
Everything else looks correct: the satisfiedAtEventIndex check correctly treats index 0 as satisfied; the Set deduplication for distinctStatesVisited is right; the mutual-exclusivity invariant with exploration-error is properly maintained; and the schema reorder of vulnerability-alerts is cosmetic/ordering only.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 27 AIC · ⌖ 13.8 AIC · ⊞ 6.2K
| return { | ||
| value, | ||
| unit: "ratio", | ||
| details: `objectives=${objectives.length} unmet=${unmet.length} observations=${observations.length} unused=${unused.length} distinctStatesVisited=${distinctStatesVisited} (from ${source})${unmetDescriptions.length === 0 ? "" : `; unmet objectives: ${unmetDescriptions.join(", ")}`}`, |
There was a problem hiding this comment.
unmetDescriptions can produce "undefined" in the details string
If an objective object has neither a truthy string id nor any description property, the expression objective.description is undefined. That value is coerced to the string "undefined" via Array.prototype.join, producing misleading details like "unmet objectives: undefined, ...".
Consider a string-safe fallback:
const unmetDescriptions = unmet.slice(0, 5).map(objective => {
if (typeof objective.id === 'string' && objective.id !== '') return objective.id;
if (typeof objective.description === 'string' && objective.description !== '') return objective.description;
return '(unnamed)';
});@copilot please address this.
There was a problem hiding this comment.
A couple of spots are carrying extra abstraction and test volume without buying much. net: -24 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 #57152 · codex · mai10 · 5.04 AIC · ⌖ 0.579 AIC · ⊞ 13.5K
Comment /ponytail to run again
| min: 0.0 | ||
| max: 1.0 | ||
| script: | | ||
| const isRecord = value => value !== null && typeof value === "object" && !Array.isArray(value); |
There was a problem hiding this comment.
L20-39: yagni: multi-branch trace-shape fallback for trajectoryIR, trajectoryIr, ir, and agentOutput variants. Use one canonical trajectoryIR payload and let the producer emit it.
| @@ -757,6 +775,121 @@ describe("trace_graders", () => { | |||
| }); | |||
There was a problem hiding this comment.
L775-890: shrink: eight near-duplicate cases for one grader. Two smoke tests plus a table-driven case would cover the same branches with less noise.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting on test coverage gaps; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Boundary condition untested: the
observations.length === distinctStatesVisitedequality case is the exact threshold between "defer" and "score", but no test pins it. - Unsafe
undefinedinterpolation:objective.descriptioncan beundefined, producing literal"undefined"in thedetailsoutput string. - Test readability: the duplicate
"repo-root"ref in the scoring test silently relies on Set deduplication; without a comment it reads as a copy-paste bug.
Positive Highlights
- ✅ Full branch coverage across 8 test cases, mirroring the exploration-error suite structure
- ✅ Deferral guard makes the two graders mutually exclusive — clean complementary design
- ✅ Candidate-selection priority ordering is correct (objective-bearing IR over bare
agentOutput) - ✅ Frontmatter comment block is thorough and matches the implementation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 48.2 AIC · ⌖ 15.3 AIC · ⊞ 7.6K
Comment /matt to run again
| objectives: [{ id: "goal", description: "Inspect repo", satisfiedAtEventIndex: null }], | ||
| events: [ | ||
| { kind: "state_change", ref: "repo-root" }, | ||
| { kind: "state_change", ref: "repo-readme" }, |
There was a problem hiding this comment.
[/tdd] Missing boundary test: observations.length === distinctStatesVisited is the exact threshold between deferral and scoring, but no test exercises equality — only < (defers) and > (scores) are covered.
💡 Suggested test
it("scores when observations exactly equal distinctStatesVisited (boundary)", () => {
const result = runExploitationError({
trajectoryIR: {
objectives: [{ id: "goal", description: "Inspect repo", satisfiedAtEventIndex: null }],
events: [{ kind: "state_change", ref: "s1" }],
observations: [{ id: "obs-1", consumedByActionIds: [] }],
},
});
// observations (1) === distinctStatesVisited (1): should score, not defer
expect(result.value).toBe(1);
expect(result.details).toContain("observations=1 unused=1");
});The < guard means equality falls through to scoring — this test pins that semantics and prevents a future <= regression.
@copilot please address this.
| const value = helpers.clamp(unused.length / observations.length, 0, 1); | ||
| const unmetDescriptions = unmet.slice(0, 5).map(objective => (typeof objective.id === "string" && objective.id !== "" ? objective.id : objective.description)); | ||
|
|
||
| return { |
There was a problem hiding this comment.
[/tdd] objective.description may be undefined, causing "undefined" to appear literally in the details string when an objective has neither id nor description.
💡 Suggested fix
const unmetDescriptions = unmet.slice(0, 5).map(objective =>
(typeof objective.id === "string" && objective.id !== "" ? objective.id :
typeof objective.description === "string" && objective.description !== "" ? objective.description :
"(unnamed)")
);No test currently exercises this path. A companion test with { satisfiedAtEventIndex: null } (no id, no description) would lock in the safe fallback.
@copilot please address this.
| }); | ||
|
|
||
| it("scores the unused fraction of observations when exploration was sufficient", () => { | ||
| const result = runExploitationError({ |
There was a problem hiding this comment.
[/tdd] The duplicate "repo-root" ref is intentional (to force distinctStatesVisited=1 via Set deduplication), but looks like a copy-paste error without a comment.
💡 Suggestion
Add an inline comment explaining the intent:
// Two events with the same ref → Set deduplication → distinctStatesVisited=1
// so observations(2) > distinctStatesVisited(1): exploitation-error applies.
events: [
{ kind: "state_change", ref: "repo-root" },
{ kind: "state_change", ref: "repo-root" },
],@copilot please address this.
|
@copilot this PR has completed review activity and would benefit from a final pass. Please address any remaining reviewer feedback, refresh the branch if needed, and run the 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.
|
Implements
exploitation-error(Tier 2, rank 4) from the catalog in.github/workflows/shared/graders/README.md. Whereexploration-errorattributes objective failure to insufficient search,exploitation-errorisolates the opposite case: the run gathered enough evidence but still missed objectives, scoring how much of that evidence no later action ever consumed.Changes
.github/workflows/shared/graders/exploitation-error.md(new) — importablegraders:fragment,unit: ratio,direction: lower_is_better, range[0, 1]. Resolves the Trajectory IR with the same candidate-selection logic asexploration-error(preferring the objective-bearing candidate over unrelatedagentOutput)..github/workflows/shared/graders/README.md— rank 4 Status flipped toImplemented(9 of 25)..github/workflows/shared/graders/exploration-error.md— drops the now-stale "exploitation-error (not yet implemented)" cross-references in the script comment and trailing doc block.actions/setup/js/trace_graders.test.cjs— 8 cases covering every branch, mirroring the existingexploration-errorsuite.Scoring
0observations < distinctStatesVisitedexploration-errorunusedObservations / observations, clampedAn observation counts as used iff
consumedByActionIdsis a non-empty array.distinctStatesVisitedis the distinct-refcount overstate_changeevents, falling back to declaredstates[].The deferral guard is what makes the two graders mutually exclusive on the same trace:
No workflow imports the fragment yet, so no
.lock.ymlregeneration is involved.