Simplify operational-value grading to one-shot evaluators - #60682
Conversation
…on, improve metric validation, and add comprehensive metric patterns - Updated the operational-value-designer skill description and metadata for clarity and versioning. - Refined the design procedure to emphasize the importance of semantic translation and evaluator specificity. - Introduced a new references file containing operational value metric patterns to guide metric design. - Enhanced the verification script to enforce stricter output validation and ensure compliance with the new evaluator structure. - Improved test coverage for the evaluator, including checks for invalid outputs and oversized responses.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new evaluator contract is incompatible with current runtime consumers, and stdout validation has correctness gaps.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
Open findings (3)
What changed in this PR
Refactors the operational-value designer around workflow-specific, deterministic per-run metrics.
Changes:
- Replaces the legacy evaluator contract with an ordered metric-array design.
- Adds domain-specific metric patterns.
- Strengthens evaluator validation and negative tests.
| File | Description |
|---|---|
SKILL.md |
Defines the revised design process and evaluator contract. |
references/metric-patterns.md |
Adds metric-design examples. |
scripts/verify-operational-value-evaluator.sh |
Validates the new output format. |
tests/test.sh |
Adds invalid and oversized-output cases. |
This comment has been minimized.
This comment has been minimized.
|
Nice work on the operational-value-designer refactor! 🎯 This PR looks well-structured and focused on improving the skill's documentation, testing, and verification infrastructure. As a core team member, you've followed the appropriate process, and the changes are clearly scoped to a single skill with strong test coverage. Key strengths:
Noted: The PR is currently marked as draft with "Do not merge" label, which is appropriate for WIP status. Ready for team review when you mark it ready.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 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.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
🏗️ ADR Required - draft addedAn ADR-backed design decision is required for this PR because the prefetch summary shows 550 added lines in default business-logic directories, which is above the 100-line enforcement threshold. Evidence used
Action taken
Next action for the author
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs. The architectural simplification (removing ~2,900 lines of historical replay/maturation/baseline/cache machinery in favor of a frozen one-shot evaluator contract) is well executed and consistently threaded through the compiler, CLI runner, JS grader shim, docs, and spec. Requesting changes on two regressions that reintroduce bugs a prior review already flagged on this branch.
📋 Key Themes & Highlights
Key Themes
- Regressed fixes: two inline comments call out spots where earlier Copilot review feedback (unquoted
$skill_dir, NUL-byte-unsafe command substitution inverify-operational-value-evaluator.sh) was not carried into the final diff. - Contract consistency: the new
{schemaVersion, run, event, outputs, config}request shape and[{id, value}]metric array are validated symmetrically in Go (pkg/cli/graders_run.go,pkg/workflow/graders_operational_value.go), JS (operational_value_grader.cjs), and the verification/test scripts — good deep-module boundary with a single, simple interface replacing several prior modes. - Test coverage:
graders_operational_value_test.go,graders_run_test.go, andoperational_value_grader.test.cjswere all updated to match the new contract with solid edge-case coverage (empty arrays, duplicate/empty ids, out-of-range values, timeouts).
Positive Highlights
- ✅
AGENTS.mdrule 10 codifies the new evaluator-change contract (co-locate evaluator + workflow changes,verify-operational-value-contract-change.sh), keeping governance in sync with the code change. - ✅ Docs (
graders-specification.md,cli.md) were updated in the same commit to drop dead concepts (evidence cutoff, opportunity keys, historical regrading) rather than leaving stale references. - ✅ Daily File Diet evaluator rewrite is much simpler and self-documenting (comment block states metric semantics up front).
Note: the pre-fetched diff was capped at 3000 lines, so some Go files (pkg/cli/graders_run.go, pkg/workflow/graders_config.go, pkg/workflow/graders_operational_value.go, associated tests) were reviewed directly from the working tree rather than the diff; no additional issues were found there.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 112.1 AIC · ⌖ 15.1 AIC · ⊞ 10.4K
Comment /matt to run again
There was a problem hiding this comment.
Applied Impeccable distill (large refactor/cleanup removing historical replay machinery) and audit (technical/correctness quality) modes given this is a refactor_cleanup change.
The core simplification is well-executed: the new one-shot evaluator contract, fixture-driven verification, and updated spec/docs are consistent with each other, and the daily-file-diet evaluator + fixtures pass local verification. Two issues found tied to the refactor itself:
- Dead env var (
pkg/workflow/compiler_yaml_graders.go:68) —GH_AW_RUN_CREATED_ATis still emitted into every compiled workflow even though the JS side (trace_graders.cjs,operational_value_grader.cjs) no longer reads or forwardscreatedAtanywhere. Leftover wiring from the removed maturation/evidence-cutoff machinery. - Regression: unquoted variable (
.github/skills/operational-value-designer/tests/test.sh:23) — this PR changed a previously-quoted"$skill_dir"invocation to an unquoted$skill_direxpansion, inconsistent with every other call in the same script and unsafe for paths with spaces/glob characters.
Requesting changes to clean up both before merge; neither is a security risk but both are avoidable defects introduced directly by this diff.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 234.5 AIC · ⌖ 13.9 AIC · ⊞ 8.4K
Comments that could not be inline-anchored
pkg/workflow/compiler_yaml_graders.go:68
This env var is now dead: trace_graders.cjs (this PR) removed process.env.GH_AW_RUN_CREATED_AT / the operationalValueRunMetadata plumbing (main no longer builds or forwards runMetadata), and operational_value_grader.cjs's buildRunSubject(env) no longer accepts or reads metadata.createdAt. Keeping this GH_TOKEN/GH_AW_RUN_CREATED_AT env block still wires needs.activation.outputs.run_created_at into every compiled workflow (e.g. .github/workflows/daily-file-diet.lock.yml) f…
.github/skills/operational-value-designer/tests/test.sh:23
This line was changed by this PR from the quoted form path=$("$skill_dir/scripts/...") to an unquoted $skill_dir expansion: evaluator_path=$($skill_dir/scripts/operational-value-evaluator-path.sh daily-file-diet). Every other invocation in this script quotes "$skill_dir". If the repository checkout path contains whitespace or glob metacharacters, this line will word-split/glob and fail or run the wrong command. Please quote it: `evaluator_path=$("$skill_dir/scripts/operational-value-eva…
Summary
request.outputswhile preserving the distinction between requested and applied mutations.Why
Operational value should measure the strongest effect a run can actually prove at grading time. Workflow-specific semantics now live in frozen evaluators instead of generic historical infrastructure, avoiding hindsight tuning and claims about future or unapplied outcomes.
Validation
file-diet-decision-conformance=1andlargest-file-under-threshold=0under the new evaluator.make agent-report-progresspasses.