Skip to content

Compare changed skills through the reusable Evals Action - #15

Open
ronniegeraghty wants to merge 29 commits into
mainfrom
ronniegeraghty-skill-eval-workflow
Open

ronniegeraghty wants to merge 29 commits into
mainfrom
ronniegeraghty-skill-eval-workflow

Conversation

@ronniegeraghty

@ronniegeraghty ronniegeraghty commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Use native Vally features and reusable comparison infrastructure instead of maintaining an eval engine or local reporting implementation in the starter.

  • Move generic authentication, paired execution, native projection and publication into github/evals-action#28, addressing github/evals-action#26. All three calls pin verified 750ff67f1e0a98cfa6a042e89efa99cd7825146b; exact Action contract.
  • Remove the custom reporter, reporting-only fixtures and generic comparison machinery. Preserve real behavior/native-invocation regressions in pr-description.test.mjs; catalog/diff/eval selection remains thin repository policy.
  • Fix local catalog spelling support, inventory membership, CI telemetry opt-out, explicit user approval for the paid judge and bounded publisher queueing. Actual selector fixtures preserve authenticated workflowRef, exclude uncatalogued pairs and reject invalid PR-controlled directory names before annotations or outputs. Inventory regressions require malformed names/pinned-source fields to fail before the authenticated tree reader; primitive strings and strict end-of-input prevent coercion/final-line-break cases. A workflow regression preserves the repository-scoped selection credential. Malformed JSON identifies its trusted input kind without exposing its contents.
  • Preserve merged Remove unsupported project Docker default #19's compatible local backend, host launcher and exposure warnings through the reported-conflict-only target merge. No Docker defaults return. Plugin sources, graders, eval fixtures, models and dependency pins/lock remain unchanged.
  • Correct README/guide workflow ownership from the actual YAML: skill-evals.yml is only a read-only notification; the trusted skill-eval-comment.yml controller's evals job owns protected approval/paid execution, and its separate publish job owns commenting. No trigger, permission or approval policy changed.

Current published head: cde7dd88e0e384a2b12b5a55301a18895fb8fd6b. The first migration removed a net 1,037 lines; native-results cleanup 18a2ec9 removed another net 909 lines. No replacement custom dashboard, parser or eval framework was added. The latest commit clarifies actual workflow responsibilities and consumes the verified upstream diagnostic-edge correction.

Native commands

npm alias Native command
eval vally eval --workers 1 --max-retries 1
eval:models vally experiment run evals/standard-models.experiment.yaml --output-dir evals/results/standard-models --require-pass
eval:serve vally serve evals/results
eval:compare vally compare

eval:serve is the model-free loopback dashboard; CLI 0.21.0 includes its server after normal npm ci. eval:compare invokes a judge and spends AI credits, not an automatic model-free file diff or PR-publication step. The approval warning precedes its documented command. Vally stays 0.21.0. Serve and compare were checked against installed help/source.

Behavior and trust

The read-only skill-evals.yml notification has no approval gate or paid execution. Trusted default-branch skill-eval-comment.yml performs selection; its protected evals job runs sequential Before/After with the saved model, one trial/worker and zero retries. Only the target skill changes; measurements and neighbors stay fixed at the verified merge. A native Before verdict failure does not suppress After; integrity/provenance failures still fail closed. Failed/incomplete outcomes keep the paid job failed; unpaired or uncatalogued skills are not run.

Catalog/file pairing does not itself prove a custom eval stages or invokes the changed skill. Before paid approval, maintainers must verify the native environment paths and intended invocation; changing the catalog does not update .vally.yaml automatically. The included eval uses the shared native marketplace environment. This is not a new local-only ownership filter or duplicate environment parser.

The selection step explicitly uses GH_TOKEN: ${{ github.token }} with read-only job permissions. That token is limited to the workflow repository, rather than a stored account-wide CLI credential. Public outside pinned metadata is deliberately supported; unrelated private repositories are not made accessible by a PR-controlled catalog. Broader credentials require separate target-authorization review, not a silent token substitution or same-org regex. Reader finding and source-based resolution.

The protected evals job has billing permission and read-only repository/PR/Actions access, not PR-write access. The controller's fresh publish job has PR-write access, no billing permission and no checkout/candidate execution. Approval permits candidate code to execute with model credentials on the host; narrow environments/workspaces are not filesystem, credential, network or shared-kernel isolation.

The exclusive-App requirement was declined. The default shared github-actions[bot] account allows another permitted PR-write workflow to forge marker/ordering metadata and suppress reports. This MVP accepts that limitation, not a security fix. Authenticated workflowRef namespaces legitimate ordering, not exclusive publisher attestation. No App credentials, alternate provenance framework or repository settings were added.

Acquisition uses the Action's script-disabled job-private generated integrity lock, never candidate manifests. Both arms share one graph and record digests; transitive resolution can differ across jobs. Actual bytes/types/permissions/symlink targets outside the selected skill are checked before After and after each native arm; drift rejects even passing native evidence. Regular-file symlinks include resolved bytes/permissions; directory/special-file targets and initialized/nonempty submodules are explicitly unsupported. Empty uninitialized gitlinks are allowed. Git replace objects are disabled in guard/restore/diff commands, and untracked directories, including empty selected-skill directories, are detected.

Declared Git filter attributes on tracked files are conservatively unsupported, including negative declarations and drivers literally named unset or unspecified. Both working-tree and cached attributes, including local overrides, are checked across tracked paths before and after staging the selected source. The original index is preserved through preflight; index and worktree are restored together so additions/deletions/renames remain supported. Git operations force the effective worktree to the verified candidate rather than trusting core.worktree. Normal text/EOL normalization is retained; diff guards disable external diff and text-conversion commands. This is an explicit integrity/support boundary, not a silent fallback or a change to the caller's approval policy.

Stale-context checks guard each arm and final publication without undoing billed requests. Publishers use the bounded 100-pending-job queue and producing-run/attempt ordering within workflowRef. Partial paid retries and refreshed base/test-merge context require Re-run all jobs, renewed approval and repeating paid arms; publisher-only retry cannot repair changed context.

Only trusted selection and two bounded verdict projections are uploaded. The pinned Action constructs fixed regular JSON files outside the candidate checkout and emits their paths only after exclusive creation; the workflow's existing regression permits exactly those two fields, never recursive private diagnostics. Artifact-path finding and source-based resolution. This is an API guarantee, not host sandboxing or independent attestation of candidate execution.

Approved tables retain grader counts, visible After failures, collapsed Before failures and marketplace coverage. Only oversized unchanged/unmeasured inventory may be summarized; changed rows, recorded results and failures are retained or publication fails explicitly. Legacy missing native verdicts remain unknown, not invented passes. Missing native model identity is explicitly Unspecified, not inferred; the failure legend covers either native arm. Missing optional trial eval names use the native parser's normalized group name. Changed-skill failures render once; inventory retains totals/references and additional non-changed failures. Fixed setup/checkout/symlink/missing-artifact failure codes do not expose raw exception text. The approved fictional sample remains illustrative.

Validation and review boundary

The merged integration passed 34 focused model-free checks and lint; the preceding diagnostic batch passed 30 affected checks and lint. This workflow-ownership/pin batch passed the focused workflow responsibility/permission contract and lint; selector behavior/regressions are unchanged. Two actual workflow-entrypoint cases build verified PR merges with invalid plugin/skill names and require the exact rejection, no annotations/raw names and no selection/output files. The inventory-helper regression covers 12 malformed-field cases with exact errors and zero tree-reader calls, plus incomplete-tree rejection; its null-name case failed before the primitive guard correction.

Syntax regressions cover malformed marketplace JSON and actual workflow-entrypoint context/environment JSON. Catalog/context cases reproduced the old misleading message before the fix; all require the correct input-kind error, no payload/annotations and no output artifacts. Labels cover marketplace catalog, comparison context, evals environment and pinned remote inventory; only syntax exceptions are wrapped. Exact dependency metadata/runtime were verified; runtime blob is 2629849c7bc17e1e5c5c4f03fc35f7c09eac5cd4.

All6 exact-head GitHub checks succeeded as of2026-11-13T11:38:33Z, including the completed Copilot review job. The completed review of current head cde7dd8 reports No open issues / Needs a closer look, not approval. Its summary repeats the notifier/controller guidance concern but publishes no inline finding or specific remaining location. The source-based response checks every remaining tracked Markdown notifier reference: README and guide both explicitly label notification-only behavior and immediately identify the trusted controller's approval/paid/publish roles. No additional incorrect guidance reference was identified; the remaining human judgment is deferred, not claimed cleared. No speculative code change or duplicate same-head review was requested.

Earlier inline findings were addressed and resolved. The concrete JSON ambiguity, workflow-attribution ambiguity and rejection-test gap were corrected with source evidence and focused regressions. Earlier unlocated coverage/setup judgments were not independently cleared beyond these concrete corrections. Artifact-path and reader findings were resolved with exact source/credential-boundary evidence, not scanning suppression. Final human review remains separate from the successful checks and COMMENTED Copilot result.

No paid eval/judge, server launch, real evaluator-authentication test, Docker operation, settings change or merge of this PR was performed. Live workflow_run, token acceptance, approval, runtime parity and billing remain separately qualified. Review the dependency before deployment; both workflows and the trusted selector must reach the default branch before automatic activation. See integration and trust details.

Require protected-environment approval before paid same-repository runs; hold eval measurements fixed while switching only the target skill source and reuse the Action's verified same-job tooling.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty
ronniegeraghty requested a review from a team as a code owner November 11, 2026 19:23
Copilot AI dynamic review requested due to automatic review settings November 11, 2026 19:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Missing working-directory inputs prevent both eval invocations from running, and the selection job lacks the actions: read permission needed for private-repository environment checks.

2 open issues
What changed in this PR

Introduces approval-gated before/after evaluations for changed existing plugin skills, preserving the marketplace’s shared evaluation setup while switching only the selected skill’s source.

Changes:

  • Adds sequential, pinned evals-action runs against exact base and PR-head skill versions.
  • Requires protected-environment approval for same-repository PRs and skips skills without a valid comparison pair.
  • Adds model-free workflow checks and documents setup, billing, and execution boundaries.
File Description
README.md Introduces the approval-gated PR evaluation workflow.
docs/​checks-and-evals.md Documents comparison inputs, approval requirements, and execution limits.
.github/​workflows/​skill-evals.yml Adds skill selection, source switching, and sequential evaluation jobs.
.github/​workflows/​evals.yml Updates comments to distinguish manual smoke evaluations from skill comparisons.
.github/​scripts/​validation.test.mjs Adds checks for workflow policy, selection, source restoration, and measurement drift.

Review effort: Max (auto)

Comment thread .github/workflows/skill-evals.yml Resolved
Comment thread .github/workflows/skill-evals.yml Resolved
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

🧪 Skill eval comparison

Mock-up only: all results are fictional. Example PR changes marketplace-maintainer/release-plugins and pr-description/pr-description.

Changed skills: 📈 2 resolved · 📉 1 new failure. Other skills: 📉 1 new failure. Failure totals count grader/model results.

Sources: Before <base SHA> → After <PR head SHA> · Evidence: <workflow run link>
Trials: One per scenario/model/arm, zero retries. Observed differences do not prove causation.

Read cells as Before → After. A scenario is one test case; graders are its individual checks. Scenario labels below omit their repeated <eval>- prefix; evidence retains the full native IDs.

Changed skill: marketplace-maintainer/release-plugins

Eval : scenario (passing / total graders) Claude Sonnet 5.5 GPT-6.1 Terra
release-plugins : keeps-already-bumped-version ✅ 3/3 → 3/3 ✅ 3/3 → 3/3
release-plugins : retires-approved-plugin ✅ 3/3 → 3/3 ✅ 3/3 → 3/3
release-plugins : ignores-local-copy-edit ✅ 4/4 → 4/4 ✅ 4/4 → 4/4
release-plugins : asks-for-unknown-release-base 📈 3/4 → 4/4 ✅ 4/4 → 4/4
release-plugins : preserves-unreleased-initial-version 📉 4/4 → 3/4 ✅ 4/4 → 4/4

Changed skill: pr-description/pr-description

Eval : scenario (passing / total graders) Claude Sonnet 5.5 GPT-6.1 Terra
pr-description : drafts-structured-pr-body 📈 3/4 → 4/4 ✅ 4/4 → 4/4
pr-description : ignores-bug-and-review-question ✅ 4/4 → 4/4 ✅ 4/4 → 4/4

❌ After failures in changed skills (1 grader/model result)

Skill: marketplace-maintainer/release-plugins

Eval : scenario: release-plugins : preserves-unreleased-initial-version

Change Failed grader Model Why it failed Evidence
📉 New in After skill-invocation (skill routing) Claude Sonnet 5.5 Required release-plugins activation was not recorded. <after grader evidence link>
🕰️ Before failures in changed skills: 2 grader/model results, both resolved After

Skill: marketplace-maintainer/release-plugins

Eval : scenario: release-plugins : asks-for-unknown-release-base

Change Failed grader Model Why it failed Evidence
📈 Resolved in After program (release-evidence request) Claude Sonnet 5.5 Mentioned the missing released base but did not ask for it. <before grader evidence link>

Skill: pr-description/pr-description

Eval : scenario: pr-description : drafts-structured-pr-body

Change Failed grader Model Why it failed Evidence
📈 Resolved in After program (PR-body sections) Claude Sonnet 5.5 Omitted the required Risks / Rollback section. <before grader evidence link>
🌐 All marketplace skills: 6 evaluated · 📉 1 new failure in an unchanged skill · 6 not evaluated

Cells: passing scenarios / total scenarios, Before → After. A scenario passes only when it meets its configured scoring rule; these examples require every grader to pass.

Skill (passing / total scenarios) Claude Sonnet 5.5 GPT-6.1 Terra
marketplace-maintainer/release-plugins 🔄 4/5 → 4/5 (1 resolved, 1 new failure) ✅ 5/5 → 5/5
marketplace-maintainer/update-remote-pins ✅ 3/3 → 3/3 ✅ 3/3 → 3/3
plugin-authoring/add-plugin ✅ 2/2 → 2/2 ✅ 2/2 → 2/2
plugin-authoring/submit-plugin ✅ 2/2 → 2/2 ✅ 2/2 → 2/2
plugin-authoring/try-plugin-locally 📉 2/2 → 1/2 ✅ 2/2 → 2/2
pr-description/pr-description 📈 1/2 → 2/2 ✅ 2/2 → 2/2
eval/compare ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval
eval/create ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval
eval/improve ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval
eval/review ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval
eval/run ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval
eval/setup ⏭️ Not evaluated — no repo eval ⏭️ Not evaluated — no repo eval

The six remote Evals skills have no repository evals. Loading them is not behavioral coverage.

🔎 Marketplace failure details: 2 After · 2 Before (grader/model results)

Skill: marketplace-maintainer/release-plugins

Eval : scenario: release-plugins : asks-for-unknown-release-base

Change Failed grader Model Why it failed Evidence
Before (resolved After) program (release-evidence request) Claude Sonnet 5.5 Mentioned the missing released base but did not ask for it. <before grader evidence link>

Eval : scenario: release-plugins : preserves-unreleased-initial-version

Change Failed grader Model Why it failed Evidence
After (new) skill-invocation (skill routing) Claude Sonnet 5.5 Required release-plugins activation was not recorded. <after grader evidence link>

Skill: pr-description/pr-description

Eval : scenario: pr-description : drafts-structured-pr-body

Change Failed grader Model Why it failed Evidence
Before (resolved After) program (PR-body sections) Claude Sonnet 5.5 Omitted the required Risks / Rollback section. <before grader evidence link>

Skill: plugin-authoring/try-plugin-locally

Eval : scenario: try-plugin-locally : prepares-current-checkout-session

Change Failed grader Model Why it failed Evidence
After (new) program (local-command path) Claude Sonnet 5.5 The command omitted the selected plugin's absolute local path. <after failed assertion/evidence link>

Legend: ✅ All pass in both arms · 📈 More pass · 📉 Fewer pass · 🔄 Same count, different failures · ❌ Failed grader. Change labels distinguish new, resolved and persistent failures.

⚠️ Error · ⏭️ Not run · ❔ Unknown never count as passing. Use 3/4 → ⚠️ Error, not an invented After count; report partial coverage separately.

Same evals, graders and fixtures across arms; only the PR's changed skill sources differ. Links and SHAs are placeholders, not actual run evidence.

Preserve the pending main integration, fix Action inputs and environment-read permissions, and add the approved compact comparison presentation without expanding paid execution scope.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Avoid upload collisions and importing prior-attempt verdicts when a maintainer reruns the same workflow.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

With workflow-token billing enabled, PR-editable workflows can bypass maintainer approval while retaining paid Copilot access, creating a critical authorization gap.

4 open issues
2 resolved since last review

Review effort: Max (auto)

Comment thread .github/workflows/skill-evals.yml Resolved
Comment thread .github/scripts/skill-eval-comment.mjs Resolved
Comment thread .github/scripts/skill-eval-comment.mjs Resolved
Comment thread .github/workflows/skill-evals.yml Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The publishing job can execute PR-controlled code with pull-request write permissions without protected-job approval, creating a privilege-escalation risk.

5 open issues
3 previously missed issues

In code that hasn't changed since last review

High severity Require explicit passing verdicts before declaring resolution

.github/​scripts/​skill-eval-comment.mjs:215

A missing check in the other arm is treated as a passing verdict: Before with completed=true, routing=false and After with only completed=true produces an improving 1/2 → 1/1 cell and labels routing as resolved. Require an explicit passing verdict for the matching check before declaring a resolution; otherwise mark that comparison unknown or incomplete. Apply the same presence check to the headline totals, transition markers, and marketplace comparison counts.

High severity Run the publisher from a trusted revision

.github/​workflows/​skill-evals.yml:237

This job executes the PR's comment helper with pull-requests: write, even when a new, unpaired skill causes the protected eval job to be skipped. A same-repository integration with source-file write access but no workflow-editing or pull-request-write permission can replace that helper and gain the job's broader token without approval. Run the publisher from a trusted revision rather than the PR checkout, bootstrapping this new helper on the trusted branch before enabling the workflow.

Medium severity Replace native JSON parse errors with a fixed diagnostic

.github/​scripts/​skill-eval-comment.mjs:119

If native stdout contains a non-JSON line after the first JSON record, JSON.parse can include a fragment of that line in its exception message. The CLI logs that message verbatim at line 339, exposing raw evaluator content even when the Action has already classified the evidence as malformed-native-jsonl. Catch the parsing exception here and replace it with a fixed diagnostic.

Review effort: Max (auto)

Comment thread .github/workflows/skill-evals.yml Resolved
ronniegeraghty and others added 2 commits November 12, 2026 08:45
Require matching complete grader evidence for transitions, redact parse failures, cancel stale PR runs, and reject partial retries before either paid arm. Run the publisher from the trusted base and document the still-open billing trust decision without changing triggers.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove write permissions from PR publication code; validate the authenticated producer workflow, run, attempt, PR and merge revisions, and treat bounded artifact JSON as untrusted display data. Preserve the existing native paid entrypoint and document its separate approval-boundary gap.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty
ronniegeraghty requested a dynamic review from Copilot November 12, 2026 17:31
Comment thread .github/workflows/skill-evals.yml Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A same-repository PR author can modify the workflow to bypass evaluation approval and obtain a billing-capable token, enabling unauthorized use of the organization’s Copilot credits.

4 open issues
4 resolved since last review

Review effort: Max (auto)

Comment thread .github/scripts/skill-eval-comment.mjs Resolved
Comment thread .github/scripts/skill-eval-comment.mjs Resolved
Keep the PR notifier read-only and use default-branch workflow_run code for selection, environment-approved native CLI execution, and separate fresh-runner publication. Pin Vally 0.21 outside the candidate checkout without spoofing GitHub events. Bind selected revisions and producing attempts, preserve partial grading failures and escape labels once. Track supported Action migration in github/evals-action#26; owner billing policy and approved candidate credential exposure remain explicit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty ronniegeraghty changed the title Add approval-gated before/after skill evals with evals-action Add trusted before/after skill evals with native Vally Nov 12, 2026
@ronniegeraghty
ronniegeraghty requested a dynamic review from Copilot November 12, 2026 18:53
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

The current native migration is pushed in 4430df5. The PR notifier now has read-only contents access; trusted default-branch workflow_run selection leads to the existing protected paid job, followed by a fresh publisher-only runner. The original Action-directory/selection-read, partial-grading/check-presence, source-attempt and trusted-publication findings remain covered; this update also preserves completed checks beside grading errors, escapes labels once and skips cancelled source runs.

github/evals-action#26 tracks returning to the Action once trusted source-event/revision handling is supported. The approved fictional sample remains unchanged, not actual measurement evidence. No paid evals or credential/settings changes were made. Current-head CI and cycle-four review are pending, not presumed clean. This entrypoint split does not constrain other workflows an authorized editor can create; owner billing policies and approved candidate-process exposure remain explicit.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

An invalid runner reference in job-level env prevents the workflow from running, while missing PR-read permission blocks private-repository selection and shared state compromises comparison isolation.

3 open issues
4 resolved since last review

Review effort: Max (auto)

Grant selection PR-read access, initialize the native CLI path in a step rather than unavailable job runner context, and give Before and After separate fresh homes and temporary directories. Model-free command execution checks both state boundaries without running models.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty
ronniegeraghty requested a dynamic review from Copilot November 12, 2026 19:20
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

Addressed the rejection-coverage concern in published 3aa54e61b9723b43587880a938273be48b823133.

The focused production-helper regression now requires malformed catalog names, source kinds, repository/SHA fields and remote paths to throw the existing errors without calling the authenticated tree reader. It covers non-string fields, final line breaks, invalid pins and traversal; incomplete tree responses also fail rather than producing partial inventory. The regression first failed against the old null-name behavior. Primitive-string guards and strict end-of-input matching now prevent the coercion/line-break cases, without adding a parser, org allowlist or broader credential.

The same batch consumes the actual verified 2a30ab7c4c5e057baab6ac6dc4c496e502a9d947 Action: missing model identity is explicitly Unspecified, and the failure legend covers either native arm. All 30 affected model-free checks and lint pass. Vally remains0.21.0; no paid eval/judge or merge was performed. This reply is not a request for review; a fresh actual review request on the new head will be verified separately.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

An unpublished moderate-severity finding about incomplete workflow regression coverage warrants human assessment of the remaining publication risk.

No open issues 🎉

Review effort: Max (dynamic)

Pin the verified published comparison Action after the prior consumer review completed. Preserve the same API and Vally version while accepting native normalized eval names and retaining failure evidence without duplicate changed-skill rows.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

This review reports No open issues but asks for human assessment of an unpublished workflow-coverage concern. I read the complete published review: it contains no inline findings, affected location or failing behavior to reproduce. That judgment remains deferred, not claimed fixed or cleared. I am not inventing a new workflow harness or coverage gate from a category-only summary.

The existing model-free contract checks approval/job permissions, the three exact Action calls and restricted artifact paths; the preceding batch passed30 affected checks plus lint. Real workflow_run, evaluator-token acceptance and paid runtime/billing qualification remain explicitly separate and unperformed. No paid eval/judge was run to bypass that boundary.

After this review completed, published 36d911dbdcdf25cbc296414a75703b378bfb0e6d makes only the already verified upstream pin update to 3fd266ba11370e38aef93876630bcb86166d1739 in the three calls, contract test and docs. That supplies native normalized eval-name fallback and nonduplicated changed-skill failure rows while retaining required evidence; it does not purport to fix the unpublished concern. The focused workflow contract and lint pass; API, Vally0.21.0 and the accepted shared-bot limitation are unchanged. No credentials/settings changes or merge were made.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human review is needed to assess the impact of unpublished findings involving protected-approval workflow guidance and ambiguous JSON error diagnostics.

No open issues 🎉

Review effort: Max (dynamic)

Pin the actual published Action correction distinguishing invalid working directories, Vally resolution and tool acquisition without echoing raw exception details. Preserve the existing command/API, approval policy and Vally version.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

This completed review has No open issues and no published inline findings, location or reproducer for its protected-approval/JSON-diagnostics categories. Those unpublished human-assessment concerns remain deferred, not claimed fixed.

The existing documented contract requires maintainer reviewers on the protected evals environment and fails preflight when they are absent. The selector deliberately emits a fixed JSON-error message instead of raw payload/exception contents. These are source facts, not proof that an unspecified finding has been cleared; I have not guessed a new approval policy, parser or logging framework from the summary. Live approval/token/billing qualification remains separately unperformed, with no paid eval/judge run.

Published 5ec6fa155bbd3c9449d0bd70f7f8ab2bc97c1e5e is only the separately supplied verified Action pin update to e926c2c79744c502fc30867264b546e83684e99c. It distinguishes fixed working-directory, Vally-resolution and tool-acquisition failure codes without raw exception text; it is not presented as a fix for the unpublished JSON/approval concern. The focused workflow contract and lint pass; preceding30 source regressions are unchanged. Same API/Vally0.21.0/accepted shared-bot limitation; no settings, credentials or merge changes.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed to judge the impact of unpublished findings concerning ambiguous input-error diagnostics and inaccurate workflow setup guidance.

No open issues 🎉

Review effort: Max (dynamic)

Pin the verified comparison Action correction that rejects active custom filters before restoring source and disables external diff/textconv. Document that unsupported context while retaining normal text/EOL normalization and unchanged approval/API contracts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

I read the complete review of5ec6fa1: No open issues, with zero published inline findings or a location/reproducer for the unpublished input-diagnostics/setup-guidance categories. Those judgments remain deferred and unassessed, as in the earlier boundary response; I am not claiming they are fixed or inventing a policy/parser change from the summary.

Published 2b1c99886462582707ba20af416dcb07c43d8171 consumes the separately verified upstream 339bc231110c53b47017c8301b351dc438a1c2c7 Git-filter integrity correction. The caller docs now explicitly state that active custom Git filters on tracked files fail before source restoration, while normal text/EOL normalization remains supported. That is a sourced dependency/support-boundary update, not a purported fix for unknown review findings. The focused workflow contract and lint pass; preceding30 source regressions, API, Vally0.21.0 and approval policy remain unchanged. No paid eval/judge, credentials/settings changes or merge.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed to weigh unpublished minor findings about misleading JSON error messages and inaccurate documentation of workflow responsibilities.

No open issues 🎉

Review effort: Max (dynamic)

Reproduce misleading catalog/context diagnostics, add fixed trusted input-kind labels, and cover actual CLI failures without payload echoes or outputs. Consume the verified effective-worktree/index restoration fix and document the conservative rejection of all declared Git filter attributes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

For the completed2b1 review, I independently reproduced a concrete diagnostics issue in the existing selector: malformed marketplace/context JSON both reported Invalid skill selection JSON. Published 73d89b6473148bc2fc87b1853f7e4db5f24c18d3 now labels trusted input kinds separately—marketplace catalog, comparison context, evals environment and pinned remote inventory—without echoing JSON contents or raw syntax exceptions. Non-syntax errors still propagate.

The existing fixture now checks malformed catalog input plus actual workflow-entrypoint context/environment failures: exact role-specific error, status1, no synthetic private marker/annotations and no selection/output files. Catalog/context cases failed before the fix; all30 affected model-free checks and lint now pass. This uses the existing JSON helper, not a new parser/logging framework or approval policy.

The same batch consumes verified 5be803cb5cc6fa05ca8b9269665e1af457949087. Docs accurately describe its conservative unsupported boundary: all declared tracked-file filter attributes, including negative declarations and literal unset/unspecified drivers. Normal text/EOL normalization and added/deleted/renamed source paths remain supported by the upstream index+worktree restoration contract.

The complete published review still provides no inline finding, location or reproducer for inaccurate workflow-responsibility documentation. That unlocated judgment remains deferred, not claimed fixed; the concrete JSON correction does not clear every unpublished concern. Same API/Vally0.21.0/accepted shared-bot limitation. No paid eval/judge, credentials/settings changes or merge.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed for an unpublished documentation nit about which workflow owns the approval gate and paid execution.

No open issues 🎉

Review effort: Max (dynamic)

Correct the README's notification-workflow attribution using the existing YAML job responsibilities: trusted controller evals owns approval/billing, publish owns commenting, and PR notification owns neither. Consume the verified upstream diagnostic-edge fixes without changing permissions or execution policy.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

Addressed the workflow-ownership nit in published cde7dd88e0e384a2b12b5a55301a18895fb8fd6b, grounded in the actual YAML rather than guessing an unpublished code defect.

The README previously linked the read-only skill-evals.yml notification as the before/after workflow and attributed approval/commenting to that workflow. It now explicitly names the trusted skill-eval-comment.yml controller: its evals job owns the protected evals environment and paid Before/After, while its separate publish job owns comment-write access, not billing. The PR notification owns neither. The linked guide states the same split. No approval policy, workflow trigger, job permission or execution behavior changed.

The same batch pins verified 750ff67f1e0a98cfa6a042e89efa99cd7825146b in all3calls/docs/test for the upstream shallow-parent, unsupported-symlink and missing-download diagnostics. The focused workflow responsibility/permission contract and lint pass; the30 source checks from the preceding JSON-diagnostic batch are unchanged. This corrects the concrete ownership ambiguity without claiming unspecified unpublished findings beyond that are cleared. No paid eval/judge, credentials/settings changes or merge.

🤖 AI-authored reply via Agency Finish PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed to judge the significance of workflow guidance pointing readers to the notifier rather than the controller responsible for approving paid evaluation runs.

No open issues 🎉

Review effort: Max (dynamic)

@ronniegeraghty

Copy link
Copy Markdown
Collaborator Author

I checked this exact-head review and all remaining tracked Markdown references to the notifier. The published review has no inline finding or specific remaining location. At cde7dd88e0e384a2b12b5a55301a18895fb8fd6b, the only guidance links to skill-evals.yml are explicitly labeled read-only notification: README and the guide. Both immediately point to the trusted controller and identify its evals approval/paid job and separate publish job.

The actual controller YAML matches that description. Its source-workflow input intentionally names the notifier for authenticated trigger provenance; changing that to the paid controller would not be a documentation fix.

No additional incorrect guidance reference was identified, so I am leaving the remaining human judgment deferred rather than making speculative changes or requesting another same-head review to clear a category-only summary. A concrete remaining location would be needed to distinguish it from the attribution already corrected. All6 exact-head checks, including the completed Copilot review job, succeed; the review reports No open issues / Needs a closer look, not approval. No code changes, new review request, paid eval/judge or merge from this response.

🤖 AI-authored reply via Agency Finish PR.

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.

3 participants