Skip to content

fix(guardrails): skip eval samples in reference-verify - #10686

Closed
kyle-sexton wants to merge 5 commits into
mainfrom
fix/guardrails-eval-samples-verify
Closed

kyle-sexton wants to merge 5 commits into
mainfrom
fix/guardrails-eval-samples-verify

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Closes #10694
Refs: #9936

Summary

The reference-verify pre-push entry blocked the push of branch host-run/9936-eval-scenario-sources, which adds the developer-experience eval scenario sources. It reported 28 findings, every one dotnet build --nologo (not in 'dotnet build --help'), and every one in a file under plugins/developer-experience/evals/sources/labels/*/samples/. Those files are labeled judge samples: recorded model output. A flag, skill reference or path inside them is data, not a claim this repository makes (docs/conventions/untrusted-content/README.md), and editing them to pass the check would corrupt the eval.

Fix

  • verified_type in plugins/guardrails/git-hooks/reference-verify.sh, the one filter that picks which changed files go to the three verifiers, now skips any path with a samples/ folder below an evals/ folder (at the repository root or nested, directly or deeper). Nothing else was added: the three verifiers run only through this entry, so the rule lives in one place.
  • Narrowest rule the layout supports. Plugin eval suites keep recorded answers in evals/<case>/samples/ and the authored parts in prompt.md, graders/ and fixtures/; the 9936 sources follow the same split (labels/*/samples/ vs harness/, judge/, held-out/). Prompts, graders, fixtures, harness scripts and a samples/ folder outside evals/ stay verified.
  • README entry row and the script and test headers say what is skipped.

Verification

  • Repro first: the new stay-quiet case (an unknown flag in an eval sample does not block) failed against the unmodified script (want [0|] got [1|evals/sources/labels/T1/samples/run1.md:1: cli-flag-verify: unknown flag(s) ...]) and passes after the fix. Three must-fire cases pass before and after: the same bad flag in an eval graders/check.md, an eval prompt.md, and docs/samples/x.md still blocks. reference-verify.test.sh: 36 passed, 0 failed.
  • The 9936 branch tip pushed through the hook as a new branch: before the fix, exit 1 with 28 findings in 309 s; after, exit 0 with no findings in 30 s (the sample files are no longer sent to the verifiers).
  • scripts/check-affected.sh --tests on the committed branch: 14 passed, 2 skipped (no changed .py, no changed skill), including the selected guardrails suites.

Left out: #9936's own deliverable. This PR is the hook-precision fix #10694 asks for, which unblocks that push; it commits none of the eval scenario sources. #9936 still needs, after this merges: pushing branch host-run/9936-eval-scenario-sources (520 scrubbed scenario source files, already committed and merged with main) and opening its PR, the identifying-details review record for those files, and the changelog check.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 28, 2026 12:23
Files under an evals/ suite's samples/ folder hold recorded model output.
A CLI flag, skill reference or path inside one is data, not a claim the
repository makes, and editing it to satisfy the check would corrupt the
eval. Prompts, graders and other eval files stay verified.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 28, 2026 16:50
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-28T16:51:24.923432Z e264374 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review needs no new review of fa2ed4b083c442e94160b7cc01c5fc39a1225d67: no file of this pull request changed since the review of f6ca323.

No security issues found in this pull request.

critical-findings: 0

The change makes the pre-push reference-verify entry skip .md, .sh, .bash, .ps1 and .psm1 files under an evals/.../samples/ folder. It adds one case arm to verified_type in plugins/guardrails/git-hooks/reference-verify.sh. The rest is a README row, comments, a changelog entry and tests.

  • What the skipped checks guard: cli-flag-verify, skill-reference-verify and stale-path-verify check for unknown CLI flags, missing skill references and stale paths. They are correctness and precision checks, not security controls. The hook runs locally at push, so it is advisory. It does not gate merge, and a pusher can already bypass it.
  • Pattern scope: the case globs match evals/<...>/samples/* only. * also matches / in a bash case, so deeper paths under samples/ are skipped, as the PR says it intends. A docs/samples/ folder is not skipped, and the new tests cover that.
  • Untrusted content: recorded model output stays out of the verifiers' input, which narrows what untrusted text reaches them. Nothing here executes the file contents or widens a token, trust list, kill switch or merge hold.
  • Trusted paths: none of the changed files match the listed trusted paths. The diff has no manifest changes, so there are no manifest <field> lines.
  • Instruction surface: no rule or guardrail instruction is deleted or softened. The one narrowing is the verifier skip described above.
  • Spend: none.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

claude-review needs no new review of fa2ed4b083c442e94160b7cc01c5fc39a1225d67: no file of this pull request changed since the review of f6ca323.

No findings. I read the diff and the PR context only and ran nothing, so the test results below are the author's claims, not mine.

Change: verified_type in plugins/guardrails/git-hooks/reference-verify.sh now returns "not verified" for any path with a samples/ folder below an evals/ folder. Bash case globs let * match /, so the four patterns cover evals/ at the repository root or nested, with samples/ directly under it or deeper. They need a / or the start of the path before evals, so a name like myevals/samples/x.md is not skipped. A samples/ folder outside evals/, such as docs/samples/x.md, stays verified. That matches the rule in #10694.

Closing issue #10694 criteria:

  • Stay-quiet test for plugins/p/evals/c/samples/answer.md and evals/sources/labels/T1/samples/run1.md: the new test covers both paths.
  • Must-fire tests for graders/check.md, prompt.md and docs/samples/x.md: the new loop covers all three.
  • README entry row names the skipped files: the row is updated.
  • reference-verify.test.sh passing in full: author-claimed (36 passed), not independently run.
  • Pushing the chore(developer-experience): commit the hand-rolled eval scenario sources #9936 branch through the hook with no findings: author-claimed, not verified here.

The changelog fragment is a patch bump for a precision fix, which fits.

Spend: no hold. The change adds no paid or billed resource.

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.

The pr-verify verifier failed head e264374288fabd5042165079a059be8b1df926de. What it wrote, as data:

Summary: PR #10686 links #9936 with `Refs:`, so it has to deliver part of that issue. Issue #9936 asks for the developer-experience eval scenario sources to be committed under plugins/developer-experience/evals/sources/, scrubbed and reviewed for identifying details. This diff commits none of those sources. Instead it changes the guardrails `reference-verify` pre-push hook so it no longer checks files under any evals/**/samples/ folder. That is a prerequisite, not a part of the issue's scope. It also loosens a check the issue never asked to change. The PR body says 'Left out: nothing' and does not name the issue's remaining work. As a refs-scoped delivery it fails.
- PR body: for a `Refs:` link the body has to say which part of #9936 this delivers and what remains. It says 'Left out: nothing' and lists no remaining work: committing the sources, scrubbing the 11 files that contain a home path or email address, recording the identifying-details review, and the changelog check. Name what remains, or link the issue that actually covers this change.
- .changes/guardrails/fix-guardrails-eval-samples-verify-d4c705cd.md: the changelog fragment describes the hook change. Keep it with whichever issue ends up authorizing that change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The spec for this hook change is now #10694, a sub-issue of #9936, linked with Closes #10694; the body names what #9936 still needs after this merges.

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.

The pr-verify verifier failed head e264374288fabd5042165079a059be8b1df926de. What it wrote about this file, as data:

- plugins/guardrails/git-hooks/reference-verify.sh:139: issue #9936 asks only that the scenario sources (definitions, fixture generator, checklists, labeled samples) be committed under plugins/developer-experience/evals/sources/. Changing the guardrails pre-push verifier is outside that scope. Either move this hook change under its own issue that asks for it and link that issue with `Closes:`, or deliver an actual part of #9936 under this Refs.
- plugins/guardrails/git-hooks/reference-verify.sh:139: this weakens a guard beyond what the spec asks. Every path matching evals/samples/*, evals/*/samples/*, */evals/samples/* or */evals/*/samples/* in any repository using the plugin now skips cli-flag-verify, skill-reference-verify and stale-path-verify. A change to what a shared guardrail enforces needs a spec that asks for it. Getting past the 28 findings on the #9936 branch alone does not justify it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The spec for this hook change is now #10694, a sub-issue of #9936, linked with Closes #10694; the body names what #9936 still needs after this merges.

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.

The pr-verify verifier failed head e264374288fabd5042165079a059be8b1df926de. What it wrote about this file, as data:

- plugins/guardrails/git-hooks/reference-verify.test.sh:253: the tests look correct as written. Samples are skipped, while graders/, prompt.md and docs/samples/ still block. The blocking problem is scope and spec, not the test logic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The spec for this hook change is now #10694, a sub-issue of #9936, linked with Closes #10694; the body names what #9936 still needs after this merges.

@kyle-sexton
kyle-sexton enabled auto-merge October 28, 2026 17:03
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

The pr-unblock lane stopped address-feedback on this pull request: the reply job failed before it decided anything (a lane token it could not get, or a script error), so no reply was posted. The lane sets no label: the run ends in a failing pr-unblock/address-feedback status on the head the lane gated.

When a retry on this head can change the outcome, the lane runs one on its own, up to its per-head cap, and edits this comment if it stops again; a run that does not stop clears the status. When no retry can (a review thread the lane does not take or settle, or a failing check after the retries are spent), a person resolves the threads or gets the checks green, then dispatches the workflow below: that run finds the cause gone and clears the status. On a pull request the lanes App opened, the dispatchers start that run once per stop; on any other, a person does. A new commit to the branch starts clean.

To run it again on this head now, dispatch the pr-unblock workflow:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=10686 -f activity=address-feedback

Lane-Activity: pr-unblock/resolve-conflicts
@kyle-sexton

Copy link
Copy Markdown
Member Author

Superseded by #10698 (merged as f41aa45), which makes reference-verify skip recorded eval samples.

auto-merge was automatically disabled October 28, 2026 19:10

Pull request was closed

@github-actions
github-actions Bot deleted the fix/guardrails-eval-samples-verify branch October 29, 2026 06:35
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.

guardrails: reference-verify skips recorded eval samples (evals/**/samples/)

1 participant