fix(skill-evals): do not scavenge JSON out of a prose step's output - #1169
Merged
Conversation
`extract_json_from_output`'s last-resort strategy takes the largest
balanced `{...}` / `[...]` block in the CLI's stdout. Ordinary Markdown
supplies those freely: a task-list `- [ ]` parses as the JSON value `[]`,
and a link's `[text]` as a malformed one. On a step whose output is
prose, that scavenged fragment then replaces the whole body — `actual`
becomes `[]`, `raw_output` no longer exists, and every assertion
addressing it fails against output the model got exactly right.
That is what happened to all four cases of
security-issue-deduplicate/step-3-merge-body. Read back, the model's
merged issue body was correct in each: both credits on separate lines,
both mailing-list threads, severity Unknown, the widened version range,
the CWE agreement noted. The harness threw it away and reported four
failures.
A suite whose expected side addresses a wrap key (`raw_output`,
`stderr`, `exit_code`, via expected.json or an assertions.json `field`)
is declaring that the step emits prose. Skip extraction for those and
pass the body through. This reuses `wrap_is_asserted` from apache#1161 on the
mirror-image problem: that change stopped the harness passing what it
never graded, this one stops it failing what it graded wrong.
All four cases pass after the change. Two tests cover both directions —
prose containing a task-list stays prose, and an ordinary JSON step still
goes through the extractor.
Also bumps the framework version to 0.2.0.dev202609081000 and
regenerates the manifests, so the fix reaches installed copies: the
marketplace is served from `main` and `claude plugin update` compares
version strings, not commit SHAs, so a frozen stamp leaves adopters told
they are already up to date.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01So3JRGXrbqSGrohtZuHWKg
potiuk
added a commit
that referenced
this pull request
Sep 8, 2026
…self-correcting (#1174) * fix(marketplace): re-sync magpie-agent-guard with the dev version stamp `#1169` stamped the framework version to `0.2.0.dev202609081000` across every ecosystem manifest. `#1170` landed right after it, adding the `magpie-agent-guard` plugin — a *new* file, branched before the bump and therefore carrying the previous stamp `0.2.0.dev202609080121`. New files do not conflict, so both merged cleanly and `main` was left with a family plugin one stamp behind the root manifest it inherits from. `check-family-plugins` has failed on `main` ever since, which is why an unrelated dependabot PR (#1167) shows a red `prek` job it did not cause. `python3 tools/dev/check-family-plugins.py --fix` output; no hand edits. Generated-by: Claude Code (Opus 5) * fix(prek): regenerate the family plugins instead of reporting them stale The `check-family-plugins` hook ran check-only, so a contributor whose manifests drifted got an instruction — "run `tools/dev/check-family-plugins.py --fix`" — rather than a fixed tree. The generated files are a pure function of the skills' `family:` frontmatter and `pyproject.toml`'s version, so there is nothing for them to decide: the fixer is the only correct response to every failure the check can report. Run the hook as `--fix`. prek fails a run whose hook modified files, so drift still stops the commit; it now arrives already corrected, needing only a re-stage — the same convention `end-of-file-fixer`, `trailing-whitespace`, and `add-license-headers` already follow here. `--fix`'s exit code is deliberately left alone: the release-bump procedure invokes it directly in plain shell blocks, where returning non-zero on a successful regeneration would read as failure. Scope note: this closes the hand-fix gap for anyone who runs the hooks, but it cannot catch the cross-PR case that broke `main` this morning — two PRs green in isolation, conflict-free on merge, out of sync in the result. Only a check on `main` (or a merge queue) sees that. Docs describing the hook as check-only updated to match. Generated-by: Claude Code (Opus 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Running all 75 eval suites turned up four failures in
security-issue-deduplicate/step-3-merge-body— the whole step, every case.Reading the model's actual output back, it was correct in all four. The
harness threw it away.
extract_json_from_output's last-resort strategy takes the largest balanced{...}/[...]block in the CLI's stdout. Ordinary Markdown supplies thosefreely: a task-list
- [ ]parses as the JSON value[], and a link's[text]as a malformed one. On a step whose output is prose, that scavengedfragment replaces the whole body —
actualbecomes[],raw_outputno longerexists, and every assertion addressing it fails against a correct answer.
What the model actually produced
For
case-1-basic-merge, graded as a failure:Both credits on separate lines, both threads, severity
Unknown, the widenedversion range, the CWE agreement noted — every assertion satisfied. The reported
failure was
field 'raw_output' not present in output.The fix
A suite whose expected side addresses a wrap key (
raw_output,stderr,exit_code— viaexpected.jsonor anassertions.jsonfield) isdeclaring that the step emits prose. Skip extraction for those and pass the
body through untouched.
This reuses
wrap_is_assertedfrom #1161 against the mirror-image problem:Both are the same underlying confusion — treating "no JSON here" as a fact about
the model rather than a fact about the step.
Verification
step-3-merge-bodycases pass after the change (0 → 4)prose; an ordinary JSON step still goes through the extractor
prek run --all-filespasses (exit 0)Version bump
Also bumps the framework to
0.2.0.dev202609081000and regenerates themanifests, per
marketplace-distribution.md. The marketplace is served frommainandclaude plugin updatecompares version strings, not commit SHAs —so without a bump, adopters are told they are already up to date and never
receive this fix.
Notes for reviewers
This came out of a full 1196-case run. That run is not finished: 850 cases
returned
ERRORfrom quota exhaustion, not defects — the CLI was healthy whenre-tested. 29 genuine failures remain, clustered in a way that suggests more
shared causes rather than 29 separate bugs (
issue-reproducer7,issue-fix-workflow4,audit-finding-fix3, and two suites failing cases withmatching
scope-check/drive-by-reformatnames). Those are follow-up work,not in this PR.
Worth stating plainly: this bug and #1161's were both invisible for as long as
they existed, because each produced a plausible-looking result. The eval harness
needs the same scepticism as the things it grades.
🤖 Generated with Claude Code
https://claude.ai/code/session_01So3JRGXrbqSGrohtZuHWKg