Repository navigation
Add trusted native skill comparison subaction - #28
ronniegeraghty wants to merge 26 commits into
Conversation
Move reusable workflow_run provenance, sequential native arms and compact projection reporting into an opt-in comparison adapter. Keep repository selection and approval policy in the caller, preserving the root Action policy. Local model-free checks passed; skip automatic CI to avoid the existing paid Copilot E2E workflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reject oversized projections before retention while preserving an explicit error and attempting the other arm. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues block typechecking, make frozen installs depend on an Azure feed, and allow comparisons to pass with skipped trials or consume files left behind by the earlier revision.
7 open issues
What changed in this PR
Adds an opt-in comparison subaction for approval-gated workflow_run jobs, reusing native Vally execution without changing the root Action’s event or checkout policy.
Changes:
- Separates source authentication, Before/After execution and publisher-only reporting into
context,runandreportcommands. - Adds bounded result projections, artifact validation and sticky-comment publication controls.
- Documents the integration and trust boundaries, adds model-free tests, and packages a separate comparison bundle.
| File | Description |
|---|---|
vitest.config.ts |
Excludes the thin comparison entrypoint from coverage. |
tooling/packaged.test.mjs |
Checks standalone comparison-bundle packaging and command rejection. |
tooling/build.mjs |
Builds both entrypoints and combines license inventories. |
tests/native.test.ts |
Updates the native CLI fixture to Vally 0.21.0. |
tests/comparison-report.test.ts |
Tests projection, rendering and untrusted artifact intake. |
tests/comparison-context.test.ts |
Tests provenance authentication and selection validation. |
tests/comparison-action.test.ts |
Tests execution arms, source restoration and publication controls. |
src/native.ts |
Exposes native process exit codes for comparison validation. |
src/comparison-report.ts |
Adds bounded projections, artifact loading and comparison rendering. |
src/comparison-index.ts |
Adds the comparison entrypoint and sanitized failure handling. |
src/comparison-context.ts |
Authenticates source identity, PR context and selection metadata. |
src/comparison-action.ts |
Implements comparison commands and native execution orchestration. |
README.md |
Links the subaction and updates the documented fixture version. |
package.json |
Pins the development Vally fixture to 0.21.0. |
dist/comparison-index.cjs.LEGAL.txt |
Includes bundled third-party license comments. |
comparison/README.md |
Documents integration, permissions, trust boundaries and qualification limits. |
comparison/action.yml |
Declares subaction inputs, outputs and the Node 24 bundle. |
Review effort: Max (dynamic)
Address the completed Copilot review with ignored-file drift detection, full graded-row gating, authenticated bot identity reuse, dotted workflow selectors, explicit input typing and exact source-run head binding. Model-free pnpm check passes. Lockfile registry portability remains blocked on public availability of the pinned Vally 0.21.0 release; no silent version downgrade or paid workflow dispatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove 217 mirror-specific tarball overrides without changing package versions, integrities or dependency bindings. Preserve published Vally 0.21.0; earlier availability probes returned stale npm cache after TLS failure. Add a packaging regression for non-public tarball URLs. Frozen install, clean offline restore and full model-free pnpm check pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
NUL-terminate the ignored/untracked drift inventory so whitespace-only filenames cannot be trimmed away. Keep all reviewed source and Vally 0.21 behavior unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical defects can falsely report resolved checks, hide native evaluation failures, allow altered measurement inputs into comparisons, and weaken package integrity verification.
5 open issues
7 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
Address five fresh review findings: restore125 original SHA512 bindings, reject hidden index flags, carry native arm/eval verdicts independently of grader counts, bind grader names and IDs, and retain known sanitized error codes. Vally0.21 is unchanged. Full model-free pnpm check passes265tests and8packaging controls, including actual mock-executor native threshold rejection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A same-repository PR workflow with comment-write permission can exploit the shared Actions bot identity to forge a report marker with an inflated run number, causing legitimate reports to be rejected as stale.
7 open issues
5 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
Bind markers to authenticated trusted workflow refs and preserve stable safe validation codes. Clarify context outputs, clean candidate state, base/merge invalidation and flat artifact layout. Document the shared Actions bot's lack of exclusive publisher attestation; the protected credential decision remains open. Model-free pnpm check passes267tests and8packaging controls. No credentials/settings, paid E2E or Vally version change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The final freshness check ignores the merge SHA, allowing invalidated comparison results to be published after the base branch advances even when the PR head is unchanged.
5 open issues
7 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
Preserve observed native errors, reject initialized submodules, and bound unmeasured inventory without dropping recorded failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The digest is not rechecked after each native arm, allowing changes to measurement files to go undetected while a passing projection is accepted.
4 open issues
5 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
Retain model-specific verdicts and contiguous inventory tables when summarizing unchanged skills. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The fixture digest misses symlink-resolved content changes, allowing Before/After results measured against different fixture contents to be accepted.
2 open issues
4 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
There was a problem hiding this comment.
Approved #28 at 9e78eb4. The check-ID credential gap and ordinary-name token matching are fixed in the source and packaged code, with focused regression cases. All five tracked findings are now resolved; no outstanding code findings remain from this review.
PR tests/builds were not run, as requested. Verification used source inspection, isolated synthetic expressions and bundle syntax checking. Hosted model-free CI and live integration remain unverified; this approval does not clear those separate acceptance gates or the documented accepted shared-bot risk.
Generated by 🌳Breza Bot
There was a problem hiding this comment.
🟡 Changes recommended
The license-notice header names only one bundle and its LEGAL file despite covering both bundles, so the generator and distributed notices need updating.
1 open issue
2 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
There was a problem hiding this comment.
Comment — one low-severity correction remains for #28 at 9e78eb4.
Update the license-notice header for both bundles. The build combines dependencies from both Actions, but the generated header names only the root bundle and its license-comment file. Update the generator and regenerate the notices before final approval. This is a scope-description mismatch, not evidence of missing license text.
The five earlier findings remain fixed. This follow-up reuses the existing inline thread without adding a duplicate. PR tests/builds were not run; hosted model-free CI and live integration remain unverified.
Generated by 🌳Breza Bot
Update only the existing notice header and regenerate distributed notices through the build tooling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Approved #28 at a2823c4. The remaining notice-header finding is fixed in both the generator and distributed notices: both bundles and both license-comment files are named. All six tracked findings are fixed; no outstanding code findings remain from this review. The existing discussion thread has been left unchanged.
This revision changes only notice text. PR tests/builds were not run, as requested. Hosted model-free CI and live integration remain unverified; approval does not clear those separate acceptance gates or the documented accepted shared-bot risk.
Generated by 🌳Breza Bot
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because credential-detection false positives can invalidate legitimate comparisons, while missing or malformed run metadata can allow comment updates without freshness checks.
No open issues 🎉
1 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
There was a problem hiding this comment.
Changes requested for #28 at a2823c4, superseding my earlier approval after checking the newly raised ordering concern.
- Medium: Reject unknown run order before overwriting a report. Missing or malformed stored run metadata lets an older publisher replace newer results for the same head and merge. Validate that metadata before writing.
The six earlier fixes remain valid. Current PR-head and merge checks still apply; this finding concerns ordering between runs, not the accepted shared-bot identity risk. PR tests/builds were not run. Verification used source inspection and isolated synthetic expressions; hosted model-free CI and live integration remain unverified.
Generated by 🌳Breza Bot
Require existing report ordering tags to contain positive safe run and attempt values before any overwrite; preserve shared-bot policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Approved #28 at c00931f. The run-order finding is fixed: existing reports with missing, malformed or invalid run numbers are rejected before any write. The source, focused regression cases and packaged guard agree. All seven tracked findings are fixed; no outstanding code findings remain from this review.
PR tests/builds were not run, as requested. Verification used source inspection, isolated synthetic expressions and bundle syntax checking. Hosted model-free CI and live integration remain unverified; approval does not clear those separate acceptance gates or the documented accepted shared-bot risk.
Generated by 🌳Breza Bot
There was a problem hiding this comment.
🟡 Changes recommended
A tracked symlink can reintroduce excluded skill bytes into the digest, causing the restored baseline check to fail and blocking After even when Before made no changes.
1 open issue
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
There was a problem hiding this comment.
Changes requested for #28 at c00931f, superseding my earlier approval after verifying the new symlink case.
- Medium: Detect outside symlinks into the selected skill before execution. When the target differs between revisions, Before can consume credits, then the intended source switch changes the measured bytes and prevents After from running. Reject this unsupported layout before either arm, without weakening mutation detection.
The existing digest correctly detects different bytes; the remaining issue is detecting the incompatible layout too late. The seven earlier fixes remain valid. This review reuses the existing inline thread. PR tests/builds were not run; hosted model-free CI and live integration remain unverified.
Generated by 🌳Breza Bot
Detect outside file symlinks resolving into the selected skill before either native arm, preserving existing other-target digest checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Comment — one low-severity test correction remains for #28 at bcbe1c1.
Make the symlink-target regression independent of direct file hashing. Its new target is inside the scanned directory, so the assertion passes even if following symlink targets breaks. Move the target outside that directory before final approval.
The alias preflight fix is correct in the source and packaged code; all eight earlier findings are fixed. This remaining finding concerns regression coverage, not a newly observed runtime failure. PR tests/builds were not run; hosted model-free CI and live integration remain unverified.
Generated by 🌳Breza Bot
There was a problem hiding this comment.
🟡 Changes recommended
Missing measurement paths and GitHub credentials produce generic or mismatched error codes, while report tests can leave untracked scratch directories in the checkout.
3 open issues
1 resolved since last review
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
| let current = cwd; | ||
| for (const component of path.split('/')) { | ||
| current = join(current, component); | ||
| if ((await lstat(current)).isSymbolicLink()) throw new ComparisonError('comparison-symlinked-measurement-path'); |
| if (command === 'run' && (process.platform !== 'linux' || process.arch !== 'x64')) { | ||
| throw new ComparisonError('comparison-requires-linux-x64'); | ||
| } | ||
| const api = githubClient(token, process.env.RUNNER_TEMP!); |
| import { join, resolve } from 'node:path'; | ||
| import { | ||
| collectNative, loadComparison, renderComparison, | ||
| type ComparisonArm, type ComparisonPair, type ComparisonSelection, | ||
| } from '../src/comparison-report.js'; | ||
| import { parseNativeEvidence } from '../src/native.js'; | ||
|
|
||
| const evalPath = 'evals/local/skills/draft/eval.yaml'; | ||
| const directories: string[] = []; | ||
| afterEach(async () => { | ||
| for (const directory of directories.splice(0)) await rm(directory, { recursive: true, force: true }); | ||
| }); | ||
| async function directory(): Promise<string> { | ||
| const root = await mkdtemp(resolve('.comparison-report-')); |
Place the target outside the measured checkout and mutate equal-length bytes. Use the approved 30-second limit only for the existing real-Git alias fixture. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Comment — one low-severity correction remains for #28 at c029d4d.
Name the actual missing GitHub token input. An empty github-token currently reports report-token-required, although that input does not exist on this subaction. Use a comparison-specific error naming github-token before final approval.
The new head fixes the symlink regression: the target is outside the scanned directory, and the mutation keeps its byte length unchanged. All nine earlier findings are now fixed. The remaining finding reuses an existing thread; no duplicate inline comment was added. PR tests/builds were not run; hosted model-free CI and live integration remain unverified.
Generated by 🌳Breza Bot
There was a problem hiding this comment.
🔵 Needs a closer look
The comparison-report tests create scratch directories in the repository root, so interrupted runs can leave unignored artifacts that may be accidentally committed.
3 open issues
1 file not reviewed
pnpm-lock.yaml: Generated file
Review effort: Max (dynamic)
Summary
Closes #26. Add the separate
github/evals-action/comparisonadapter (context,run,report), keeping native configuration, selection, grading and execution in Vally. Root Action behavior is unchanged.Simplified scope: show Before/After eval results only for skills changed in the repository. Each changed skill has a compact eval/scenario-by-model table with passing/total grader counts and separate native verdicts. Removed all-skills inventory, delta counters, failure-detail sections, compression logic and generated failure reasons. Errors, missing evidence, not-run outcomes and native scoring rejection remain explicit.
Configurable layout: the trusted caller supplies
working-directory, changed-skill keys,include.directoryandinclude.evalPath; no marketplace directory names are hardcoded. The README demonstratescapabilities/writerwithquality/writer.yaml. Native.vally.yamlowns model/backend/fixture/grader configuration. The existingskillsselection field remains compatible and is not rendered.Published pin:
c029d4d62ad4012660956e090ea01b661789d38d. Metadataa5aadd6d0e959bf7eadfafddcb893a68624fe1ad; comparison runtimec036edf263a12117f22072fefcf064d2800eabafis unchanged by the latest test-only correction. New projections omitchecks[].reason; the publisher accepts and discards legacy reasons. All three caller steps must use the same pin.The latest scoped correction changes only three report behaviors: ordinary long hyphenated/underscored names remain valid, retained row results stay visible under error/incomplete arms without becoming comparable, and each changed skill has only its own model/variant columns. Exact execution-credential and explicit token checks remain unchanged. No inventory, extra detail sections or reporting features were reintroduced.
The subsequent two-finding fix applies those existing credential checks to check IDs and adds IDs to the exact execution-secret scan. Classic token prefixes now require a non-alphanumeric boundary and an alphanumeric body, so ordinary underscore-separated names do not match. No new sanitization framework or report changes.
The final notice-only correction updates the existing generator's header to name both bundles and both LEGAL files, then regenerates the distributed notices. Only those two files changed; license texts and runtime behavior are unchanged.
The subsequent ordering correction refuses to overwrite an existing marked report with a missing/malformed run tag or non-positive/unsafe run and attempt values. Eight focused invalid-tag cases verify no comment write. This mirrors the root ordering guard without changing the accepted shared-bot credential policy.
Outside file symlinks resolving into the selected skill are rejected before either native invocation. A real tracked-alias fixture proves zero native calls; without the alias the same fixture observes genuine Before/After source contents. Other resolved file targets retain byte/permission hashing and mutation detection. This unsupported-layout check prevents spending execution credits on an inherently invalid comparison.
The external-target regression now places its target outside the measured checkout and changes equal-length bytes, so it actually depends on reading the symlink target. The user-approved 30-second limit applies only to the existing real-Git alias fixture after its default 5-second limit timed out. No production code, runtime bundles or assertions changed in this test-only follow-up.
Execution/authentication, approval/credential boundaries, source-only restoration, measurement integrity and publication freshness guards remain. Fixed recovery hints now reach publication logs without arbitrary exception text. Candidate-emitted labels containing the exact execution credential or recognizable token patterns are rejected before projection/publication; this is defense in depth, not proof against deliberate encoded/split exfiltration.
Integrated current main and resolved source/lock/bundle conflicts. Retained Vally 0.21.0 and registry-neutral strong integrities while accepting main's pinned development-tool updates.
Verification
Model-free checks and source-to-bundle verification pass.
Native integration evidence is distinguished from synthetic controls.
Credential, process and artifact boundaries were reviewed.
Documentation and packaged
distmatch source.pnpm install --frozen-lockfile --ignore-scripts— passed after integrating main's manifest updates, including 548 supply-chain policy checks.pnpm build— passed; both bundles regenerated, never manually edited.pnpm check— passed: typecheck, lint, 319 model-free tests, coverage, deterministic bundle comparison and 8 packaging controls.git diff --check— passed.Actual Vally mock execution exercises a caller-mapped non-marketplace layout with Before-fail/After-pass and native weighted-score rejection. Focused controls cover changed-only rendering even with a large unrelated inventory, alternative path mappings, safe hints, credential-bearing labels and legacy artifact intake. The saved concise example is generated by the production renderer from an illustrative model-free fixture, not a paid run.
One fresh current-head review round was requested at 2026-11-14T09:48:03Z: Wallace (
wbreza, event 36115112503) and Copilot (event 36115112584). Both requests are confirmed; their results are pending. No watcher or open-ended campaign was started. The external-target test thread was resolved. Current-head scans have started; ordinary PR CI is intentionally skipped with[skip ci]to avoid unauthorized paid E2E.Limitations
No paid E2E, credential/settings changes, GitHub App setup or PR merge. Live workflow_run/protected-environment/billing integration and hosted Linux qualification remain separate acceptance gates. Consumer pin adoption and trusted default-branch activation remain separate from upstream merge/release.
The shared
github-actions[bot]publisher-identity limitation remains explicitly accepted, not fixed; dedicated App credentials are deferred. Ordinary candidate network/credential/shared-kernel exposure is not eliminated by these guards. Uncached public-registry installation remains unqualified on this host; frozen/cached controls do not establish a fresh public download.