Skip to content

feat: add value grader - #55155

Merged
mnkiefer merged 14 commits into
mainfrom
vadd-value-grader
Aug 24, 2026
Merged

feat: add value grader#55155
mnkiefer merged 14 commits into
mainfrom
vadd-value-grader

Conversation

@mnkiefer

@mnkiefer mnkiefer commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator
  • Adds an experimental, opt-in value grader that measures whether each workflow run achieved its intended repository outcome.
  • Unlike existing graders, it measures operational value rather than execution quality.
graders:
  operational-value:
    run: .github/graders/WORKFLOW-NAME-operational-value.sh
  • Configured workflows automatically record value in [0,1], evidence provenance, maturity, and optional baseline delta.
  • Runs ca be regrdaded as evidence matures:
    gh aw graders value RUN-ID --evidence-at TIMESTAMP --json

@mnkiefer mnkiefer self-assigned this Aug 23, 2026
Copilot AI balanced review requested due to automatic review settings August 23, 2026 19:50
@mnkiefer mnkiefer changed the title Implement Value Grader feat: add value grader Aug 23, 2026
@mnkiefer
mnkiefer marked this pull request as draft August 23, 2026 19:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an experimental operational-value grader with frozen Bash functions, artifact provenance, and historical regrading.

Changes:

  • Adds value-grader compilation and runtime execution.
  • Adds gh aw graders value replay support.
  • Adds schemas, documentation, tests, and authoring skill.
Show a summary per file
File Description
pkg/workflow/graders_value.go Loads and freezes value functions.
pkg/workflow/graders_value_test.go Tests function preparation.
pkg/workflow/graders_config.go Adds value-grader configuration.
pkg/workflow/graders_config_test.go Tests parsing and serialization.
pkg/workflow/compiler.go Integrates value preparation.
pkg/workflow/compiler_yaml_graders.go Generates runtime and artifacts.
pkg/workflow/compiler_yaml_artifacts.go Uploads frozen functions.
pkg/parser/schemas/main_workflow_schema.json Defines function schema.
pkg/parser/schema_test.go Tests schema acceptance.
pkg/constants/job_constants.go Adds archive filename.
pkg/cli/graders_value_regrade.go Implements historical regrading.
pkg/cli/graders_value_regrade_test.go Tests replay behavior.
pkg/cli/graders_command.go Adds graders CLI command.
pkg/cli/experiments_grader_observations.go Expands artifact models.
docs/src/content/docs/specs/graders-specification.md Specifies value grading.
docs/src/content/docs/reference/trace-graders.md Documents usage and replay.
cmd/gh-aw/main.go Registers the command.
actions/setup/js/value_grader.test.cjs Tests runtime validation.
actions/setup/js/value_grader.cjs Executes value functions.
actions/setup/js/trace_graders.test.cjs Tests function archiving.
actions/setup/js/trace_graders.cjs Integrates value grading.
.github/skills/aw-value/tests/test.sh Tests skill utilities.
.github/skills/aw-value/SKILL.md Defines authoring guidance.
.github/skills/aw-value/scripts/verify-value-function.sh Verifies value functions.
.github/skills/aw-value/scripts/value-function-path.sh Generates grader paths.

Review details

Suppressed comments (1)

pkg/cli/graders_value_regrade.go:384

  • Although the original case is passed to --grade-run, the returned case and opportunity key are accepted without comparison to the original observation. A function can therefore change the assigned opportunity as evidenceAt changes, so the emitted regrade does not actually preserve the original case as required. Validate both returned fields against original.Case and original.OpportunityKey before building the replay artifact.
	outputJSON, err := runValueBash(ctx, bashPath, functionPath, []string{functionPath, "--grade-run"}, requestJSON, valueFunctionTimeout)
	if err != nil {
		return nil, fmt.Errorf("value function --grade-run failed: %w", err)
	}
	return parseValueFunctionOutput(outputJSON, original.Subject, evidenceAtText, evidenceAt, baselineValue)
  • Files reviewed: 25/25 changed files
  • Comments generated: 10
  • Review effort level: Balanced

Comment thread pkg/workflow/graders_value.go Outdated
Comment thread pkg/workflow/graders_value.go Outdated
Comment thread pkg/workflow/graders_config.go Outdated
Comment thread pkg/cli/graders_value_regrade.go Outdated
Comment thread docs/src/content/docs/specs/graders-specification.md Outdated
Comment thread docs/src/content/docs/reference/trace-graders.md Outdated
Comment thread .github/skills/aw-value/scripts/verify-value-function.sh Outdated
Comment thread actions/setup/js/trace_graders.cjs Outdated
Comment thread pkg/cli/graders_value_regrade.go Outdated
Comment thread pkg/cli/graders_value_regrade.go Outdated
@pelikhan

Copy link
Copy Markdown
Collaborator

"value" and "function" are generic terms, we should try to find more esoteric/unique terms to describe this feature to help downstream with search/grep

Comment thread docs/src/content/docs/reference/trace-graders.md Outdated
@mnkiefer

Copy link
Copy Markdown
Collaborator Author

"value" and "function" are generic terms, we should try to find more esoteric/unique terms to describe this feature to help downstream with search/grep

I agree, renamed back to "operational value" (as in my original POC) and "evaluator" now.

# Conflicts:
#	cmd/gh-aw/main.go
#	pkg/workflow/graders_config.go
#	pkg/workflow/graders_config_test.go
@mnkiefer
mnkiefer marked this pull request as ready for review August 24, 2026 10:06
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Ponytail Reviewer completed successfully!

Generated by Ponytail Reviewer for #55155

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

reviewed_at: 2026-08-24T00:00:00Z
review_event: REQUEST_CHANGES
top_themes:
  - permissions escalation despite explicit deny-all
  - temp-file handling for frozen evaluator execution
files_reviewed:
  - actions/setup/js/operational_value_grader.cjs
  - actions/setup/js/trace_graders.cjs
  - pkg/cli/graders_operational_value_regrade.go
  - pkg/workflow/compiler_main_job_helpers.go
  - pkg/workflow/compiler_yaml_graders.go
  - pkg/workflow/graders_config.go
  - pkg/workflow/graders_operational_value.go
comment_count: 2

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 26.3 AIC · ⌖ 8.03 AIC · ⊞ 7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

The operational-value plumbing is close, but this version still introduces one real security regression and one unsafe implementation choice.

Blocking themes
  • Enabling operational-value silently overrides an explicit permissions: {} deny-all configuration by injecting actions: read.
  • The evaluator executor writes frozen Bash into runner-global temp instead of the existing gh-aw temp area, which is a bad fit for a feature built around handling archived executable content.

🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 26.3 AIC · ⌖ 8.03 AIC · ⊞ 7K
Comment /review to run again

Comment thread pkg/workflow/compiler_main_job_helpers.go Outdated
Comment thread actions/setup/js/operational_value_grader.cjs Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /codebase-design, /tdd, and /diagnosing-bugs — requesting changes on correctness, security, and coverage gaps.

Note: The diff was capped at 3 000 lines; the highest-impact files were prioritised. Skill selection used the standard heuristic (triage was not invoked separately).

📋 Key Themes & Highlights

Blocking Issues (correctness / security)

  1. Symlink bypass at compile time (graders_operational_value.go line 54) — file.Stat() follows symlinks, allowing an in-repo symlink to satisfy the file-existence check while the spec requires rejection. Use os.Lstat instead.
  2. Bash syntax check ≠ validity (graders_operational_value.go line 68) — bash -n validates syntax but not semantics; a syntactically-valid script with no --grade-run implementation still compiles and silently fails at grading time.
  3. Nil pointer panic on BaselineValue (graders_operational_value_regrade.go line 633) — when value: null and BaselineValue is non-nil, DeltaFromBaseline is nil and the default (non-JSON) rendering path panics. Add a nil guard before dereferencing.
  4. Artifact-driven code execution without trust anchor (graders_operational_value_regrade.go line 392) — digest comparison is performed against the same artifact being executed; a compromised artifact controls both the digest and the script.
  5. Host not propagated in regrade (graders_operational_value_regrade.go line 194) — the [HOST/]OWNER/REPO host component is not set in GH_HOST/API base URL for the replayed function, breaking cross-host reruns.

Non-blocking / Design Issues

  1. graders.value accepts invalid fields (graders_config.go line 267) — the generic parser can overwrite hardcoded bounds/direction; post-parse validation is missing.
  2. actions: read permission not inferred (trace_graders.cjs line 749) — workflows with explicit least-privilege permissions will get a silent 403.
  3. Verifier skips --grade-run (verify-operational-value-evaluator.sh line 54) — the critical path is not exercised; a script with no --grade-run is reported as verified.
  4. "Both files" sentence inaccurate in docsgraders-specification.md line 226 and trace-graders.md line 93 reference "Both files" but there are now three required files.

Positive Highlights

  • ✅ Thorough verifier schema validation covering attained / missed / missing / malformed examples.
  • ✅ Correct WASM stub separation (graders_operational_value_bash_wasm.go).
  • ✅ Solid regrade test coverage for the happy path.
  • ✅ Well-specified SKILL.md evaluator interface.

The blocking issues (nil panic, artifact trust, symlink bypass) should be addressed before merge.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 29.7 AIC · ⌖ 11.1 AIC · ⊞ 7.6K
Comment /matt to run again

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (1,650 new lines) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/55155-operational-value-grader.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next
  1. Review the draft ADR committed to your branch — it was generated from the PR diff
  2. Complete the missing sections — add context the AI couldn't infer, refine the decision rationale, and list real alternatives you considered
  3. Commit the finalized ADR to docs/adr/ on your branch
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-55155: Introduce a Dedicated Operational-Value Grader Type

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

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 Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 0042-use-postgresql.md for PR #42).

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · claude · sonnet46 · 95.4 AIC · ⌖ 31.5 AIC · ⊞ 8.6K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ponytail review: a few small simplifications are possible in the new grader plumbing. net: -30 lines possible.

Generated by ✂️ Ponytail Reviewer for #55155 · codex · cop10 · 16.5 AIC · ⌖ 1.77 AIC · ⊞ 16.7K
Comment /ponytail to run again

Comments that could not be inline-anchored

actions/setup/js/trace_graders.cjs:569

actions/setup/js/trace_graders.cjs:569: yagni: isolated subprocess worker for inline graders. Inline the script execution in-process or use a smaller helper.

pkg/cli/graders_operational_value_regrade.go:136

pkg/cli/graders_operational_value_regrade.go:136: shrink: custom boundedCommandBuffer around bytes.Buffer. Use exec.CommandContext with a simple size check or io.LimitReader instead.

pkg/workflow/graders_config.go:332

pkg/workflow/graders_config.go:332: yagni: bespoke operational-value config parsing and path validation. A plain run string plus one compiler-side check would remove most of this branch.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: add value grader

Modes applied: harden (new feature with external evaluator execution), audit (security/correctness of command execution and environment isolation).

Summary

The PR adds a well-structured operational-value grader with solid security foundations: digest verification, symlink rejection in the compiler path, allowlisted environment, and a buildSafeGitShowObjectArg guard against shell injection. Two issues warrant attention:

  1. Symlink bypass in archive reading (pkg/cli/graders_operational_value_regrade.go:243): file.Stat() follows symlinks, so a malformed artifact containing a symlink to a regular file would pass the IsRegular() check. The compiler side correctly uses os.Lstat(); the regrade/archive reader should too.

  2. Environment inconsistency (actions/setup/js/operational_value_grader.cjs:66): safeFunctionEnv omits GITHUB_GRAPHQL_URL, while the Go regrade equivalent includes it. Evaluators using gh api graphql will behave differently between initial grading and regrading.

Everything else looks correct: actions: read is properly inferred, the --grade-run interface is fully verified, the identity and provenance checks are thorough, and the normalizeResult null-value path is handled safely.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 285.1 AIC · ⌖ 9.27 AIC · ⊞ 6.2K

Comment thread pkg/cli/graders_operational_value_regrade.go Outdated
Comment thread actions/setup/js/operational_value_grader.cjs Outdated
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot This PR has blocking review feedback and should use the pr-finisher skill to drive it to mergeable shape.

Please prioritize these next steps:

  • Address the blocking github-actions review requests, especially the security/correctness issues called out in the latest reviews.
  • Refresh the PR branch after your fixes so maintainers get an up-to-date merge result.
  • Re-run make fmt and any impacted tests before pushing.
  • Update the PR body to link the ADR that the design-decision gate requested.

Failed checks called out in current review state:

  • Design Decision Gate requires a linked ADR in the PR body.

After pushing fixes, reply in the relevant review threads and resolve any addressed threads.

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 14.7 AIC · ⌖ 8.12 AIC · ⊞ 8.7K ·
Comment /souschef to run again

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot This PR has blocking review feedback and should use the pr-finisher skill to drive it to mergeable shape.

Please prioritize these next steps:...

All items in the last review round (symlink rejection via Lstat, compile-time bash -n validation, enforced [0,1] direction/bounds invariants, verifier exercising --grade-run, trusted-checkout enforcement for regrade replay, actions: read permission plumbing, per-host environment propagation, independent baseline/delta guarding) were already addressed in the preceding commits on this branch. Linked the ADR (docs/adr/55155-operational-value-grader.md) in the PR description. go build ./..., gofmt, and the grader-focused test suites are clean.

Copilot AI requested a review from gh-aw-bot August 24, 2026 11:18
@pelikhan

Copy link
Copy Markdown
Collaborator

The fact that it widens the github actions token is somewhat a problem. We might need to move this to another job.

@mnkiefer

mnkiefer commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

The fact that it widens the github actions token is somewhat a problem. We might need to move this to another job.

I moved the workflow-run lookup to the activation job, which already has compiler-controlled permissions. It passes created_at to the grader as an output, so the main agent token is no longer widened with actions: read.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

gh-aw-pr-sous-chef
@copilot This PR still needs forward progress before maintainer investigation is likely to be productive.

Please use the pr-finisher skill and focus on the newest blocking feedback first:

  • pelikhan noted the token-scope concern and asked for the workflow-run lookup to stay in a job that already has compiler-controlled permissions.
  • multiple github-actions reviews are still in CHANGES_REQUESTED; reply in the relevant review threads with the latest fix context and resolve any addressed threads.
  • rerun impacted checks after the follow-up so the current failures (impacted-go-tests, JS Tests (shard 2/4), build, build-wasm, Unit tests (S-Z and other)) reflect the latest branch state.

Also refresh the branch as needed, run make fmt, and push the follow-up changes before asking for another maintainer pass.

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 15.6 AIC · ⌖ 10.6 AIC · ⊞ 8.7K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

gh-aw-pr-sous-chef
@copilot This PR still needs forward progress before maintainer investigation is likely to be productive.
...

The token-scope fix (moving the workflow-run creation-time lookup to the activation job, which already has compiler-controlled actions: read, and passing run_created_at to the grading step as an env var) was already implemented in 83cdb7e. The remaining CI failures were caused by the 292 .lock.yml files being stale relative to the generate_aw_info.cjs signature change (main(core, context, github)) in that same commit; recompiled all workflows in 05b1... to sync them. go build, go vet, gofmt, and the impacted JS/Go grader tests are clean.

@mnkiefer

Copy link
Copy Markdown
Collaborator Author

@copilot Fix PR related tests

…aw_info

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot Fix PR related tests

Fixed in 8fd673c: the wasm golden fixtures (TestWasmGolden_AllEngines/TestWasmGolden_CompileFixtures) were still expecting the pre-rename main(core, context, github) call and were causing build-wasm, impacted-go-tests, and the S-Z unit test shard to fail; regenerated them. Also fixed a js-typecheck/impacted-js-tests TS2339 error caused by using global.github as a default parameter value in generate_aw_info.cjs — switched to the repo's existing githubClient || global.github fallback pattern used elsewhere. go build, make fmt, make test-unit, the full pkg/workflow suite, and JS typecheck/tests all pass now.

@mnkiefer
mnkiefer enabled auto-merge (squash) August 24, 2026 14:50
@mnkiefer
mnkiefer dismissed github-actions[bot]’s stale review August 24, 2026 14:52

All concerns addressed.

@mnkiefer
mnkiefer merged commit f2832fe into main Aug 24, 2026
46 checks passed
@mnkiefer
mnkiefer deleted the vadd-value-grader branch August 24, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants