fix(skill-evals): a CLI that grades nothing is an ERROR, not a PASS - #1161
Merged
Conversation
When the model CLI produced no usable JSON, the runner wrapped its stdout
as {"raw_output": ...} and handed that to the intersection-only
comparator. The comparator only checks keys present on both sides, so if
expected.json declared no wrap key it compared nothing — and reported
PASS.
That turns a broken CLI into a green run. It is not hypothetical: a
sandboxed `claude -p` returns "Not logged in", and a full suite recently
reported 35/35 passed with no model having graded a single case. Re-run
with credentials, the real result was 28/35.
The wrap still exists, because a step whose output is prose has two
legitimate ways to gate on it: a raw_output / stderr / exit_code key in
expected.json, or a structural assertion whose `field` addresses one
(security-issue-deduplicate/step-3-merge-body uses the latter, and is
unaffected). When neither is present the case now reports ERROR naming
the cause, with the first 120 characters of stdout.
Same guard on the non-zero-exit branch: some auth failures and safety
filters signal via exit code rather than prose.
Two tests asserted the old behaviour and are inverted; three cover the
paths that must keep grading. README documents the guard and adds the
operational note that --cli runs need to be outside any sandbox that
denies the CLI its credentials.
Generated-by: Claude Code (Opus 5)
4 tasks
potiuk
added a commit
that referenced
this pull request
Sep 8, 2026
The marker sat at a1cff44, 17 commits behind main. Bumping it alone would claim those commits are described by the specs, so the drift is closed first. Specs updated for what actually shipped: - meta-and-quality-tooling: skill-evals errors, rather than passing, when a case's CLI produced no gradeable output (#1161). - security-reporting: the tracker dashboard projects the current partial bucket to its end-of-bucket value, splitting RATE series (accumulate from zero) from LEVEL series (carry over), and deliberately not projecting mean-based signals (#1158). - project-agnosticism: <PROJECT> and <project> are two placeholders holding different values, and the lint carries both spellings plus spaced variants (#1154). - adapters: the forwarder relay's contact_handle defaults to an org-level shared inbox rather than a named individual (#1135). The multi-hop coordinator case is designed in RFC-AI-0008 and unimplemented. - issue-management-family: the family's eval suites, and the note that --cli runs belong outside a credential-denying sandbox (#1145). Commits needing no spec change: #1152, #1143 and #1156 updated their own specs in-commit; #1149, #1147, #1151 are behaviour-preserving bug fixes; #1155 and #1141 are CI and dependency chores; #1159's spec edits landed with it; #1144 removes hardcoded literals that no spec asserted. One genuine gap recorded rather than papered over: no spec covers marketplace distribution or the dev-version stamping rule from #1160, which is load-bearing because `claude plugin update` compares version strings, not commit SHAs. Logged in adoption-and-setup as wanting its own spec. Generated-by: Claude Code (Opus 5)
potiuk
added a commit
that referenced
this pull request
Sep 8, 2026
… shared toolchain (#1164) * ci(dev): add check-doc-sync — deterministic gates for doc/tree drift This session fixed a batch of documentation that had drifted from the tree: ten specs missing from both index files, per-family and per-mode skill counts stale in three places, and catalogue totals stale in a fourth. Every one was found by reading, not by a check. Left as is, they come back. check-doc-sync.py makes five of them mechanical: - every tools/spec-loop/specs/*.md is listed in BOTH overview.md and README.md; - README.md's family table "N skills" cells match live family: frontmatter; - docs/modes.md's Modes-at-a-glance Skill-count column matches live mode: frontmatter — the validator's modes-doc rule checks section membership but parses only the mode and status columns, so the counts were unguarded; - bare "N skills" totals in docs/setup/marketplaces.md match the catalogue; - every script in tools/dev/ is named in tools/dev/README.md. Each check was verified to fail on the exact defect it exists for, by reintroducing that defect and confirming the reported number matched what had actually been wrong. A check that cannot go red is worse than no check, since it reads as evidence — the lesson from #1161, where an unauthenticated CLI produced a fully green eval run. The fifth check found three undocumented scripts on its first run (agent-pre-commit.sh, check-placeholders.sh, run-workspace-check.sh), so tools/dev/README.md now carries a table saying what each of the seven guards rather than naming a couple in passing. The counts are deliberately narrow: the total-skills check reads an allowlist of files rather than sweeping the repo, because plenty of docs legitimately count a subset ("Nine skills cover the staged path") and a greedy scan would flag those as drift forever. Generated-by: Claude Code (Opus 5) * test(dev): bring tools/dev under the workspace checks The scripts in tools/dev are the gates that guard everyone else's changes, and they were the one Python tree nothing checked: the workspace checks iterate `[tool.uv.workspace] members`, and tools/dev was not one. So ruff, mypy, and pytest all skipped it. Declares the tree as a workspace member, the same config-carrier pattern `skills/pyproject.toml` uses, and adds 20 tests for check-doc-sync.py. The tests are written around the red case. Each check builds a miniature repo in tmp_path and asserts the drift is reported with both numbers, then that the corrected tree is silent — because a gate that cannot fail is worse than no gate, it reads as evidence while measuring nothing. Also covered: a spec listed in only one of the two indexes still counts as missing, a README table row that names no family is not read as a family with a wrong count, a mode nothing declares must say 0 rather than be skipped, every stale total on a page is reported rather than just the first, and the totals check stays inside its allowlist. Script filenames are hyphenated, so the tests load the module through importlib rather than a plain import. Bringing the tree under the checks surfaced four lint findings in pre-existing scripts; three are fixed (an unused unpacked variable, two loops ruff can express directly) and RUF003 is ignored with a reason — the repo's prose style uses en dashes throughout, and rewriting comments to hyphens to satisfy a homoglyph check would make them worse. Four files were reformatted. Every script was re-run afterwards to confirm behaviour is unchanged. Generated-by: Claude Code (Opus 5) * refactor(workspace): share the dev toolchain via magpie-dev Every workspace member repeated the same three pins in its own `[dependency-groups] dev`, under an instruction in the root pyproject to "keep the version bounds here and in every member in lockstep". They were not in lockstep: mypy >=2.1.0, >=2.3.0, >=1.10 (three floors) pytest >=9.1.1, >=8.0 (two) ruff >=0.16.1, >=0.16.2 (two) tools/skill-reconciler-diff declared only pytest, and tools/fossil had no dev group at all despite being ruff-checked — its checks ran against whatever happened to be in the environment. Nothing detected any of it. The duplication was the defect and the lockstep instruction was the workaround. tools/dev becomes `magpie-dev`, a metadata-only wheel whose dependencies are the toolchain. Every other member's dev group becomes `["magpie-dev"]`, with the workspace source declared once at the root. Members with extras keep them (tools/gmail/oauth-draft retains mcp). Bumping a version is now one edit and the whole workspace moves. Each member's environment stays self-contained: the checks still run `uv run --directory <member> --project . python -m <tool>`, so nothing leaks in from the root. Only the declaration is shared. Two properties checked rather than assumed. Check discovery keys off the config sections (`[tool.ruff]`, `[tool.mypy]`, `[tool.pytest.ini_options]`) and not the dev group, so giving a member more tools does not silently enrol it in more checks. And the whole battery was re-run after deleting every member `.venv`, not against a warm cache: ruff 30/30, ruff-format 30/30, mypy 27/27, pytest 33/33. One real behaviour change: tools/bitbucket moves from mypy>=1.10 / pytest>=8.0 to the current floors. It passes the fresh sweep, but that is a version bump rather than a no-op. Generated-by: Claude Code (Opus 5) * ci(dev): verify every workspace member's tests actually run Both the CI pytest matrix and the workspace sweep are driven by the presence of `[tool.pytest.ini_options]`. A project can therefore carry a full tests/ directory and be executed by nothing: the job is simply never emitted, so there is no red tick for anyone to notice. Nothing checked this. check-workspace-members.py now reports three shapes: - tests on disk, no [tool.pytest.ini_options] → the tests never run; - [tool.pytest.ini_options], no tests on disk → the CI job runs and collects nothing, so its green tick proves nothing; - neither → no tests at all. A project that genuinely should not be tested declares it with `[tool.magpie.checks] skip = ["pytest"]`, the same opt-out the sweep and the CI matrix already honour — so an exemption is visible in the file rather than being a silent absence from a matrix nobody reads. All 33 members pass today, so this locks in the current state rather than fixing a present gap. The hook now also re-fires on test files, since adding or deleting tests changes the answer. Ten tests cover it, including the false negative that would make the whole check worthless: a test file vendored inside a member's .venv or site-packages must not make an untested member look tested. Each of the three failure shapes was also verified by hand against a real member, and the declared opt-out confirmed to silence all three. Generated-by: Claude Code (Opus 5)
4 tasks
potiuk
added a commit
that referenced
this pull request
Sep 8, 2026
…1169) `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 #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. Claude-Session: https://claude.ai/code/session_01So3JRGXrbqSGrohtZuHWKg Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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
When the model CLI produced no usable JSON, the runner wrapped its stdout as
{"raw_output": …}and handed that to the intersection-only comparator. Thecomparator only checks keys present on both sides, so if
expected.jsondeclared no wrap key it compared nothing — and printed
PASS.That turns a broken CLI into a fully green run.
This is not hypothetical. A sandboxed
claude -pcannot reach the keychain or~/.claude.json, so it returnsNot logged in. #1156's test plan claims thethree
security-model-*suites passed 35/35 on exactly that basis; no modelgraded a single case. Re-run with credentials, the real result is 28/35.
The fix
The wrap stays, because a step whose output is prose has two legitimate ways to
gate on it:
raw_output/stderr/exit_codekey inexpected.json, orfieldaddresses one of those inassertions.json— which is whatsecurity-issue-deduplicate/step-3-merge-bodydoes.wrap_is_asserted()checks both. When neither is present, the case reportsERROR naming the cause and printing the first 120 characters of stdout, instead
of a pass that asserted nothing. The same guard covers the non-zero-exit branch,
since some auth failures and safety filters signal via exit code rather than
prose.
--exactmode is unchanged.Blast radius
Narrow by construction: the guard only fires where no JSON was extracted at
all. A working CLI emits JSON, takes the normal path, and is unaffected — the
only runs that change are ones where nothing was being graded.
Verified end-to-end rather than by argument:
security-model-prepare: was9 passed, now0 passed, 9 errored, each naming the cause. Same command, same fixtures.security-issue-deduplicate/step-3-merge-body/case-1— the one suite thatgrades prose via
raw_outputassertions — still reports FAIL, not ERROR, sothe guard does not intercept it. (It fails for an unrelated pre-existing
reason: the model returns
[]rather than the prose body the step specifies.Not addressed here.)
Test plan
prek run --all-filespasses (exit 0)skill-evalssuite: 161 tests passthe reasoning in their docstrings
assertions.jsonfield,expected.jsonkey, and a negative caseNotes for reviewers
The two inverted tests are the interesting part of the diff —
test_cli_mode_non_json_wraps_and_passes_under_field_awareandtest_cli_mode_non_zero_exit_wraps_under_field_awareencoded the vacuous passas intended behaviour, which is why nothing caught this earlier.
The README passage describing the old behaviour is rewritten, and a note is
added to Prerequisites:
--cliruns must happen outside any sandbox thatdenies the CLI its credentials.
Follow-up, not in this PR: the seven real failures the corrected numbers
exposed (
security-model-prepare8/9,verify9/11,update11/15). Five ofthe seven are rejection paths — cases asserting a cluster is not eligible for
a suppression rule, or that a widening suggestion must be refused. Each needs
diagnosing as either an over-specified fixture or a genuine gap in the skill
prose.
🤖 Generated with Claude Code
https://claude.ai/code/session_01So3JRGXrbqSGrohtZuHWKg