Repository navigation
Conversation
…n results `repo run` arms now compose with `+` like the Q&A --arms: a guidance part (none/raw/bridge, raw by default) plus `skill:<path>` (one skill dir) and `skill-catalog:<dir>` (every skill subdir), installed into the workspace at .claude/skills/<name>/ in prep step 5 and listed with digests in meta.json. Relative paths are under eval_dir; `repo run` and `repo tasks check --arm` refuse missing skills and a catalog without a task's `expected_skill`. A cell with skills records which ones the agent used (Skill tool calls or reads of their SKILL.md) as skills.skill_invoked / skill_selected_correct; `repo report` counts them. Each cell also records cache read/creation input tokens, and the report gives per-row token means. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… repo's .mcp.json servers The stock claude-code profile passes --strict-mcp-config with no --mcp-config, so a repo that ships .mcp.json ran with no MCP tools. Passing --mcp-config unconditionally breaks repos without the file (Claude Code exits on a missing config). args_if_present appends arguments only when the named file exists in the run's workspace. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
==========================================
+ Coverage 91.64% 91.67% +0.03%
==========================================
Files 222 223 +1
Lines 27409 27757 +348
Branches 4508 4593 +85
==========================================
+ Hits 25118 25447 +329
- Misses 1620 1633 +13
- Partials 671 677 +6
🚀 New features to boost your workflow:
|
ff1503d to
5c17c68
Compare
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5c17c68. The arm grammar, the guidance-then-skills ordering and the args_if_present design hold up. Four things are worth fixing before merge, three inline and one below.
What I ran, on a Linux review host at 5c17c68:
make ci-local: 20 passed, 0 failed, 4 skipped (docker-only and org-level checks); changed-line coverage gate passes.- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run over the new code: 56 mutants, 23 of which pass the full suite. The one that matters is inline on
runner.py.
Findings that cannot be anchored inline:
- A skill is matched by its directory name, and nothing checks the
namein itsSKILL.mdfrontmatter.Skill.nameisroot.name(arms.py:100), andskill_usageand theexpected_skillcheck compare against it. The Agent Skills spec requires the frontmatternameto equal the directory name, butSKILL.mdis never parsed, so a skill that breaks this is accepted. If the agent reports the frontmatter name, as inskills/dirname/SKILL.mdwithname: pretty-nameand an event{"kind": "tool", "name": "Skill", "skill": "pretty-name"},skill_usagereturnsskills_used: [],skill_invoked: False,skill_selected_correct: False. The tests'_skillhelper always writesnameequal to the directory name. Suggestion: refuse a skill whose frontmatternamediffers from its directory (or read the frontmatter name), and add a test with differing names.
Checked, no issues
- Schema versioning:
args_if_presentis an optional key added toharness_profile.v1.jsonin place; old profiles still validate, andschema_compatagainst the base passes. - Old results:
report.summarizereads the token fields with.get, so result files without cache fields orskillsstill summarize. - Arm ordering: guidance is applied before skills are installed, so
guidance:none+skill:xkeeps the skill...in a top-levelskill:path is refused afterresolve(). - Housekeeping: the docs page is already in
nav:, the changelog fragment is inchangelog.d/, no generated artifacts are committed, and the tests run offline.
…e a result dir A skill or catalog entry with a symlink to a host path was copied with its target's bytes (copytree dereferences), so the agent could read host files. Links that leave a skill, and catalog entries that leave the catalog, are now refused before any cell runs. Two arms whose strings slug to the same cell name (skill:a/b and skill:a_b) shared one result directory, so the second arm silently got no row. check_arms now refuses such arms up front. Regression tests cover both, and the runner's --mcp-config wiring. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| def check_arms(arms: list[str], content_dir: Path, tasks: list[Any]) -> list[RepoArm]: | ||
| """Parse every arm and find its skills before anything runs; a catalog must hold each task's | ||
| ``expected_skill``. Returns the parsed arms.""" | ||
| from .runner import slug # runner imports this module |
There was a problem hiding this comment.
The collision check works, but from .runner import slug here adds a new import cycle (arms -> runner -> grading -> workspace -> arms), and test_import_cycles_do_not_grow fails on a9b74b5.
Moving slug into arms.py (or a small module both import) would fix it.
There was a problem hiding this comment.
Fixed in f7280b0: slug() moved into arms.py, and runner.py imports it from there, so arms no longer imports runner. test_import_cycles_do_not_grow passes.
| dest = ws / skill.target | ||
| if dest.exists(): | ||
| raise PrepError(f"task {task.id}: the workspace already has {skill.target}") | ||
| dest.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
If the repo at the task base tracks .claude (or .claude/skills) as a symlink to a host path, this mkdir/copytree writes through it: with .claude -> /tmp/x in the base, --arm skill:skills/lint created /tmp/x/skills/lint/SKILL.md on the host and the cell still passed. Please refuse when ws/.claude or ws/.claude/skills is a symlink, or when dest.resolve() is not inside ws.resolve().
There was a problem hiding this comment.
Fixed in f7280b0: _install_skills refuses when .claude or .claude/skills is a symlink in the workspace, and when the destination would resolve outside the workspace. Regression test test_a_skill_is_not_written_through_a_repository_link (both link positions) fails on the old code.
| if element == TOOLS_ELEMENT: | ||
| out.extend(self.tools[tools]) | ||
| for name in sorted(present & self.args_if_present.keys()): | ||
| out.extend(self.args_if_present[name]) |
There was a problem hiding this comment.
These args are added for every tool mode, so --tools none / --tools read on a repo with .mcp.json now start its MCP servers, and --tools does not limit MCP tools. The test in test_repo_reach.py even pins --tools "" --mcp-config .mcp.json. Should args_if_present apply only to the all mode (or be keyed per tool mode)?
There was a problem hiding this comment.
Done in 4e4e402 (option a): args_if_present is added only in the all tool mode, so --tools none/read no longer start the repo's MCP servers. tests/test_repo_reach.py now pins that none/read get no --mcp-config and all does; the end-to-end MCP test runs with --tools all.
| record: dict[str, Any] = {"task": task.id, "family": task.family, "arm": arm, "model": opts.model, "rep": rep, | ||
| "harness": opts.profile.name, "harness_version": meta["harness_version"], | ||
| "base_date": meta["base_date"], "score_task": bool(task.score), "agent": agent, "grade": grade} | ||
| if meta.get("skills"): # without a trace the agent used none of them |
There was a problem hiding this comment.
When the trace could not be read, this records skill_invoked=False and skill_selected_correct=False, and repo report counts the cell as "did not use the skill". That is "not measured" read as a result.
Please record null for both (or a measured: false flag) here and skip such cells in the report counts.
There was a problem hiding this comment.
Fixed in f7280b0: a cell whose trace cannot be read now records skill_invoked and skill_selected_correct as null with measured: false. The report leaves those cells out of the counts, and the n/… denominator is the measured cells only. Test test_unmeasured_skill_usage_is_left_out_of_the_counts fails on the old code.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a9b74b5. The three threads from my earlier review are fixed and each has a regression test. make ci-local fails on the import cycle @vlad-perevezentsev already flagged at arms.py:150; I agree, and the test passes on the PR's base. The link check still lets through links that make the copy fail. Two inline points, two more below.
What I ran, on a Linux review host at a9b74b5:
make ci-local: 19 passed, 1 failed, 4 skipped (docker-only and org-level checks). The failure istests/test_import_architecture.py::test_import_cycles_do_not_grow; 5318 tests passed, 1 skipped, and the changed-line coverage gate passes.- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run over the new code: 75 mutants, 30 of which pass the touched test files. The ones that matter are inline.
Findings that cannot be anchored inline:
repo comparecannot select a skill arm, because the selector splits on the last/.parse_selector(compare.py:75, outside this diff, predates the PR) doesrpartition("/"). The PR makes/normal in an arm string. After running the docs' batch (guidance:rawandguidance:raw+skill:skills/lint),--treatment guidance:raw+skill:skills/lintis parsed as batchguidance:raw+skill:skills, armlint, and fails withno finished cell matches the treatment 'guidance:raw+skill:skills/lint'. Suggestion: try the whole string as an arm first, and fall back toBATCH/ARMonly when it matches no cell, or document that skill arms cannot be compared byrepo compare.- Nit:
docs/repo-evaluation-contract.mdis not updated. §3 still says every arm is aguidance:*arm, the §6 profile example omitsargs_if_presentand shows--strict-mcp-configwithout--mcp-config, and the result tables omitskills,expected_skilland the cache-token fields.
Still open from my earlier review: none. The link-out, cell-name collision and runner-wiring threads are fixed.
Checked, no issues
- Cell names:
check_armsbuilds the slug exactly ascell_namedoes (slug(arm.replace(":", "-"))).skill:sandskill:s/are refused, andskill:sandguidance:raw+skill:sare not wrongly refused.check_armsruns on bothrepo runandtasks check --arm. - Links out of a skill: a direct link, a chain
a -> b -> host file, a dangling link to a host path and a catalog entry linking out are all refused. - MCP wiring:
presentis built fromworkspace/<name>for names in the profile's ownargs_if_present. Disablingpresent=or usingrd / nmakes the new runner test fail. - Schema versioning and housekeeping: the optional
args_if_presentkey is added in place,schema_compatpasses, old results still summarize, and no generated artifacts are committed.
- arms: move slug() out of runner so arms no longer imports runner (closes the import cycle test_import_cycles_do_not_grow catches) - arms: refuse skill links that dangle, loop, point at a directory, or are not regular files; they crashed every cell at workspace prep - workspace: refuse to install a skill through a .claude or .claude/skills that the repository ships as a link - runner/report: a cell whose trace could not be read records skill usage as null and is left out of the skill counts - docs: contract §3 covers skill arms; §6 shows args_if_present - tests: link cases, catalog with the expected skill first, guidance:none keeping a skill, tree hash sensitivity, unmeasured cells, write-through Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Skill arms become forms of workspace.Arm (skill:<dir>, skill-catalog:<dir>, joined to a guidance part with +), named by directory so the name holds no '/'. The separate parse_arm grammar of arms.py is gone; guidance:rev: and patch: and pin_arm work with skills. Cache token fields use main's cache_read_tokens/cache_write_tokens next to usage_scope and subagent_calls; skill cells follow main's unsupported.json handling. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
--tools none/read do not limit MCP tools, so a repo's .mcp.json started its servers in arms meant to give the agent no tools. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…tory The agent calls a skill by its frontmatter name, usage matches on the directory name; a mismatch scored skill_invoked False. The merge with main added the check; this adds its regression test and the changelog line. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Skill arms are named by directory (no '/'), so parse_selector needs no whole-string fallback; the test pins that. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Pushed e1dd820 (merge of main in 263da4a, then one commit per point).
|
Arm.skills is a tuple of (kind, path) pairs; the annotation said tuple[str, ...] and mypy strict failed on it. Add a test for the guard, which no test ran: two paths with one directory name in one invocation are refused. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A skill arm is named by its directories, so resuming a batch with another path of that name, or after editing a host skill, pooled two skills in one row. The batch now records each skill arm's tree sha256 in _eval/skills.json and refuses a mismatch; a new arm is added, the same bytes resume. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
4e4e402 limited the arguments to --tools all and updated the changelog and the contract, but the profile schema still said they apply whenever the file exists. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
An agent that exits before its first turn (for example on an MCP config it rejects) leaves an empty trace that reads without error, so the cell was recorded as measured and counted as not using the skill. A trace with no event and no result line is now unmeasured, as an unreadable one is. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
These mutants passed the suite: skill_invoked as bool(used), the Skill event without the command fallback, the expected_skill name check disabled, the report's selection count for tasks without expected_skill, and the frontmatter quote strip removed. Each now fails a test. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
|
On the three items left in the 09:18 run: pushed 0f73fca.
|
… applied harness args check_arms now looks the skill's target up in each task's base, so repo tasks check --arm and repo run refuse a skill the repository already has instead of every cell crashing in prep. meta.json records the args_if_present names a cell ran with. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
@exai-bot /review |
exai-bot
left a comment
exai-bot
left a comment
There was a problem hiding this comment.
Note
AI-generated review of 0f73fcaff8be, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 18 of 96 turns, 1.9 of 48 minutes, 823k in / 13k out tokens.
This adds skill: and skill-catalog: arms to repo run and repo tasks check --arm, a per-cell skills usage record, token means in the report, and an args_if_present profile key. Through it, the claude-code profile passes --mcp-config .mcp.json only when the workspace has that file. I found the design careful: it handles symlinks, name collisions, resume pinning and the trace-unreadable case, and the tests cover the risky paths. I found only minor issues.
4 findings are on the changed lines, as comments below.
For a human reviewer
- Repos that ship
.mcp.jsonnow start those MCP servers inside the agent container, so earlier baselines for those repos are not comparable. Decide whether that needs a harness-version or profile marker.meta.jsonrecordsargs_if_present, but only when the trace was converted. - Absolute host paths are accepted for
skill:andskill-catalog:. Confirm that is intended for the CLI user, since the PR checks symlinks but does not restrict absolute paths.
Not verified
I could not run the tests. I did not confirm how Claude Code names or resolves project skills under .claude/skills in the container. I also did not check --mcp-config behaviour with a dangling .mcp.json symlink, which exists() on the host would follow differently from the container.
| row["skill_measured"] = len(used) | ||
| row["skill_invoked"] = sum(bool(u["skill_invoked"]) for u in used) | ||
| if any(u.get("expected_skill") for u in used): | ||
| row["skill_selected_correct"] = sum(bool(u["skill_selected_correct"]) for u in used) |
There was a problem hiding this comment.
Minor (plausible, correctness): skill_selected_correct rate mixes tasks that have no expected_skill into its denominator
The row gets skill_selected_correct as soon as any measured cell has an expected_skill. The count is sum(bool(u["skill_selected_correct"])), and the printed denominator is skill_measured, which counts every measured cell. A cell whose task names no expected skill has skill_selected_correct = None, so it is counted as a miss. In a batch that mixes tasks with and without expected_skill, the reported "n/m selected correct" is understated.
Suggested fix:
Count only cells where expected_skill is set, and report that count as the denominator for skill_selected_correct.
There was a problem hiding this comment.
Fixed in 256c179. Confirmed: a task naming no expected_skill has skill_selected_correct = None and was counted as a miss. The row now carries skill_expected (cells whose task names a skill) and the rate is skill_selected_correct/skill_expected; skill_invoked keeps skill_measured. Test test_the_selected_correct_rate_counts_only_cells_whose_task_names_a_skill (2 expecting cells, 1 correct, 2 without: 1/2, not 1/4) fails on the old code.
| return full | ||
|
|
||
|
|
||
| def check_arms(arms: list[Any], content_dir: Path, tasks: list[Any], git_dir: Path | None = None) -> None: |
There was a problem hiding this comment.
Minor (confirmed, maintainability): check_arms spawns one git process per (task, skill) pair
For each arm, git cat-file -e runs once for every task and every skill. A catalog of N skills over M tasks costs N×M sequential subprocesses before any cell starts. This runs on every repo run and repo tasks check --arm. The result per task does not depend on the skill, so one git ls-tree -r --name-only <base> -- .claude/skills per task, or per distinct base, would do the same check in M calls.
Suggested fix:
Run one git ls-tree per distinct task base and compare the skill targets against its output.
There was a problem hiding this comment.
Fixed in 256c179. check_arms now runs one git ls-tree -r -z --name-only <base> -- .claude/skills per distinct task base and compares every skill target against that list, so the cost is one process per base, not tasks x skills. test_the_base_check_ignores_a_skill_the_guidance_arm_removes_and_reads_each_base_once counts the git calls (2 tasks, 1 base -> 1 call; the old code made 2).
| def check_arms(arms: list[Any], content_dir: Path, tasks: list[Any], git_dir: Path | None = None) -> None: | ||
| """Find the skills of every parsed arm before anything runs; a catalog must hold each task's | ||
| ``expected_skill``. Two arms that would share a name or a result directory are refused, and so is a skill | ||
| the base of a task (in ``git_dir``) already ships, which preparing the workspace would refuse cell by cell.""" |
There was a problem hiding this comment.
Nit (confirmed, correctness): Pre-flight check of the base's .claude/skills ignores whether the guidance arm removes it
check_arms refuses when the task base already ships .claude/skills/<name>. prepare_workspace runs _install_skills after _apply_arm, which for guidance:none or a rev arm deletes files matching the manifest's guidance globs. If those globs cover that skill, the pre-flight refuses a cell that preparation would have accepted. The docstring says the pre-flight mirrors preparation, but it does not.
There was a problem hiding this comment.
Fixed in 256c179. Confirmed: preparation drops the guidance-glob files for guidance:none and pinned-rev arms before _install_skills. check_arms now takes the manifest's guidance_globs (both callers pass them) and, for those arms, counts only the base files under the skill directory that the globs do not match. guidance:raw/patch arms and a glob set that misses the skill still refuse. Covered by the same test as the ls-tree change; the old code refused the guidance:none case.
| "cost_usd": _mean([r["agent"]["cost_usd"] for r in recs]), | ||
| "total_cost_usd": round(sum(r["agent"]["cost_usd"] or 0 for r in recs), 6), | ||
| "turns": _mean([r["agent"]["num_turns"] for r in recs]), | ||
| **{k: _mean([r["agent"].get(k) for r in recs]) for k in _TOKENS}, |
There was a problem hiding this comment.
Nit (plausible, style): The PR description names cache fields that result.json does not get
The description says result.json gains cache_read_input_tokens and cache_creation_input_tokens. Those names exist only in the raw trace. agent.cache_read_tokens and agent.cache_write_tokens already existed in runner.py, and the new test asserts these names. This PR only averages the existing token fields into the report rows. The description and the changelog fragment should say so.
There was a problem hiding this comment.
Agreed that the names were wrong: the report only averages the token fields result.json already has (agent.input_tokens, output_tokens, cache_read_tokens, cache_write_tokens). The changelog fragment and the report.py docstring now say that (256c179). The PR body line is a maintainer edit and is left for the PR author to reword; no code change.
… removal; selection rate counts only expecting cells Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
@exai-bot /re-review |
exai-bot
left a comment
exai-bot
left a comment
There was a problem hiding this comment.
Note
AI-generated review of 22f98800a536, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 16 of 94 turns, 1.7 of 48 minutes, 643k in / 12k out tokens.
Re-review of 22f98800a536: 2 new findings.
2 findings are on the changed lines, as comments below.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…drop guidance for unpinned rev arms in check Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
On the two 'For a human reviewer' points in the 19:49 review, facts only:
|
A guidance:rev arm drops the base's guidance files and then writes its revision's back. check_arms counted only the drop, so a revision that ships .claude/skills/<name> passed `repo tasks check --arm` and every cell of the arm then failed in prep with "the workspace already has". Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
meta.json listed every args_if_present file in the workspace, so a --tools none or read cell said [".mcp.json"] though its argv had no --mcp-config. The stamp now comes from the rule argv uses, and is in the first meta.json write, so a cell whose trace fails to convert keeps it. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 135b273. The four threads from my last review are fixed, and the new commits (base check for revision arms, leaf-versus-directory refusal, applied_args) hold up. Three test gaps in the new pre-flight code are worth closing before merge, and there are three nits. @xaleryb's question on a malformed .mcp.json waits for the author.
What I ran, on a Linux review host at 135b273:
make ci-local: 25 passed, 1 failed, 6 skipped. The failure ispip-audit: it reportsvirtualenv 21.14.7(CVE-2026-113722, fix 21.14.9) in the CI tooling lock, which this PR does not touch (fixed onmainby #152). 6763 tests passed, 1 skipped; changed-line coverage 96% (278 lines).- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run over the code added since my last review: 33 mutants against the six touched test files, 8 of which pass the suite. Three of the 8 are equivalent or minor; the ones that matter are inline.
Findings that cannot be anchored inline:
- Nit:
docs/repo-evaluation-contract.mddoes not listargs_if_presentamong themeta.jsonfields.grep -rn args_if_present dokimi/docsreturns only the profile-schema snippet.meta.jsonnow records it (runner.py:163-164, asserted intest_repo_skill_arms.py). Suggestion: add it to the paragraph that lists whatmeta.jsonrecords.
Checked, no issues
- Command line vs
meta.json:applied_argsandargvuse the samepresentset.--tools noneand--tools readrecord[]; a dangling.mcp.jsonis omitted from both. - Report rates: old
result.jsonfiles withoutskillsor token fields still summarize;skill_expectedcounts only cells whose task names a skill, and then/mtext matches the denominators. - Schema versioning:
args_if_presentis an optional key added in place toharness_profile.v1.json;schema_compatpasses. - Earlier mutants:
if False:in_frontmatter_name, bothskill_usageguards,arm == "guidance:raw"/"guidance:bridge", andtail = ""/skills=()/base=arm.baseinpin_armeach fail the touched suite now.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…irectory reads; document a malformed .mcp.json Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Review at 135b273, items not on a thread:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ecd26bf. The three test gaps, the patch docstring and the stale comment from my last review are closed, and the new tests kill the mutants they were written for. The base check still disagrees with preparation in two places, and one frontmatter value crashes check_arms. Four things are worth fixing before merge: three inline and one below.
What I ran, on a Linux review host at ecd26bf:
make ci-local: 25 passed, 1 failed, 6 skipped. The failure ispip-audit: it reportsgitpython 3.2.0andvirtualenv 21.14.7in the CI tooling lock, which this PR does not touch (mainfixed the virtualenv one in #152; the gitpython advisory is new). 6770 tests passed, 1 skipped.- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run over the code my threads named: 7 mutants, all killed by the new tests.
Findings that cannot be anchored inline:
docs/preparing-a-repository.md:250-252(outside this diff) is now false. It says theclaude-codeprofile "passes--strict-mcp-configwith no--mcp-config, an MCP server added by a patch is not loaded, so this route cannot test an added MCP tool yet". With this PR,--tools allpasses--mcp-config .mcp.jsonwhenever the prepared workspace has the file, andpresentis computed after the arm and patch are applied (runner.py:163), so apatch:arm that adds.mcp.jsonis loaded. A reader would skip a test that now works. Suggestion: replace the sentence with theargs_if_presentrule (allmode only, file must exist in the workspace).
@xaleryb's question on a malformed .mcp.json waits for the author.
Still open from my earlier review: none. All five threads are fixed.
Checked, no issues
- Thread tests: each of the seven mutants from my threads (
rstrip, unfilteredwritten,arm.rev,utf-8, unstripped fence,if meta,len(parts) == 4 and) fails the touched suite now. args_if_present:meta.jsonand the argv use the same rule; the list is[]outside--tools alland when the workspace has no matching file, as the new contract sentence says.- Schema versioning and housekeeping: the optional key was added in place to
harness_profile.v1.json; both docs pages are innav:; the changelog fragment is inchangelog.d/; no generated artifacts are committed.
| left = [p for p in shipped[task.base] | ||
| if not (drops and any(match_glob(g, p) for g in guidance_globs))] | ||
| written = [p for p in shipped[rev] if any(match_glob(g, p) for g in guidance_globs)] if rev else [] | ||
| for skill in skills: | ||
| for where, files in ((f"the base of task {task.id}", left), (f"guidance revision {rev}", written)): | ||
| if any(p.startswith(skill.target.rstrip("/") + "/") or p == skill.target for p in files): | ||
| raise ArmError(f"arm {arm.spec!r}: {where} already has {skill.target}") |
There was a problem hiding this comment.
check_arms passes a guidance:none skill arm that every cell then refuses in preparation.
The check treats a skill the base ships as gone once the guidance globs match its files (left). Preparation only unlinks those files (workspace.py:434) and leaves the emptied directory, so _install_skills hits dest.exists() (workspace.py:402).
- Base tracks
.claude/skills/lint/SKILL.md, guidance globs["**/AGENTS.md", ".claude/skills/**"], armguidance:none+skill:skills/lint. check_arms(...)returns without error.prepare_workspace(...)for the same arm then raisesPrepError: task fix_add: the workspace already has .claude/skills/lint.
The existing test (test_the_base_check_ignores_a_skill_the_guidance_arm_removes_and_reads_each_base_once) calls only check_arms, so it pins the pre-flight and not what preparation does. The same holds for the pinned and unpinned revision arms.
Suggestion: remove the directories that the guidance drop empties in _apply_arm, or let _install_skills accept a directory with no files in it; add a prepare_workspace assertion to that test.
There was a problem hiding this comment.
Fixed in 36a9448: _install_skills removes a skill directory that holds only empty directories (what dropping the guidance leaves) before the dest.exists() check; a directory with any file or link in it is still refused. Pinned by test_a_guidance_none_arm_installs_a_skill_the_base_ships_under_the_guidance_globs, which runs check_arms and prepare_workspace for the same arm.
| written = [p for p in shipped[rev] if any(match_glob(g, p) for g in guidance_globs)] if rev else [] | ||
| for skill in skills: | ||
| for where, files in ((f"the base of task {task.id}", left), (f"guidance revision {rev}", written)): | ||
| if any(p.startswith(skill.target.rstrip("/") + "/") or p == skill.target for p in files): |
There was a problem hiding this comment.
A base that tracks .claude or .claude/skills as a symlink passes the check, then every cell fails in preparation.
The test is p.startswith(target + "/") or p == target, where target is .claude/skills/<name>. git ls-tree -r ... -- .claude/skills lists a symlink at .claude/skills as that one path, and the .claude link is not listed at all, so neither matches.
- Base with
.claude/skills -> ../shared, armskill:skills/lint:check_armspasses; preparation raisesPrepError: .claude/skills is a symlink in the repository; refusing to install lint through it. - Base with
.claude -> elsewhere: the same, with.claude is a symlink.
This is the refusal @vlad-perevezentsev asked for, but it only fires per cell. Suggestion: also list .claude and .claude/skills with git ls-tree <base> -- .claude .claude/skills and refuse a mode 120000 entry up front, with a test.
There was a problem hiding this comment.
Fixed in 36a9448: _base_skill_files now reads git ls-tree -r <base> -- .claude .claude/skills (still one git call per base) and check_arms refuses a mode 120000 entry at either path up front (the base of task T tracks .claude as a symlink). Test: test_the_base_check_refuses_a_tracked_symlink_at_claude_or_its_skills, both paths.
| try: | ||
| meta = yaml.safe_load("\n".join(block)) | ||
| except yaml.YAMLError: | ||
| return None | ||
| name = meta.get("name") if isinstance(meta, dict) else None | ||
| return str(name).strip() or None if name is not None else None |
There was a problem hiding this comment.
A SKILL.md with name: 2024-13-45 crashes check_arms with a bare ValueError.
yaml.safe_load resolves that scalar as a timestamp and raises ValueError: month must be in 1..12, which is not a yaml.YAMLError. _frontmatter_name catches only the latter, so resolve_skills, check_arms and tasks check --arm show a traceback, not an ArmError, and the batch does not start.
Suggestion: except (yaml.YAMLError, ValueError): return None, and add name: 2024-13-45 to the invalid-YAML case in test_a_valid_frontmatter_name_is_read_from_the_top_level_key_only.
There was a problem hiding this comment.
Fixed in 36a9448: _frontmatter_name catches ValueError as well as yaml.YAMLError, so name: 2024-13-45 reads as no declared name (the directory's). Test: test_a_frontmatter_name_that_yaml_reads_as_an_impossible_date_is_the_directory_name_not_a_crash.
|
On xaleryb's malformed |
…vers links and emptied dirs repo validate and the repo run pre-flight (and a cell whose arm wrote the file) refuse a .mcp.json the agent's CLI would exit on. check_arms refuses a base that tracks .claude or .claude/skills as a symlink, a guidance:none arm installs a skill whose directory the guidance drop emptied, and a SKILL.md name YAML reads as an impossible date is not a crash. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…helpers from the arm, manifest and workspace checks repo validate reports a malformed .mcp.json as a warning and succeeds; repo run --tools all and a patch arm that writes one still refuse it. check_arms, resolve_skills, check_repo and prepare_workspace are back under the complexity cap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Follow-up to the .mcp.json threads: 09f903e changes the earlier 36a9448 behavior. |
…der its complexity baseline Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
This branch has not been deployed
7 participants
Summary
Adds two arm types,
skill:andskill-catalog:, torepo run, and lets the claude-code profile start a repo's MCP servers.repo tasks check --armrejects a skill path that does not exist.--strict-mcp-configwithout--mcp-config, so a repo that ships.mcp.jsonran with no MCP tools. Passing--mcp-configevery time is not a fix either: Claude Code exits withMCP config file not foundwhen the repo has no.mcp.json.Evidence
Before:
--arm skill:...was rejected. A blueprint repo with.mcp.jsonshowed nomcp__*tools.After: 5315 tests passed, 2 skipped. New tests:
tests/test_repo_skill_arms.py: skill arms, check validation, and theskills/ cache fields inresult.json.test_claude_code_profile_passes_mcp_config_only_when_the_workspace_has_one.The change was also used for about 870 real cells across 4 swcatalog blueprint task sets on Haiku 5.5 and Sonnet 5.5. A sample: banking-assistant on Haiku went from 23/25 with no guidance to 25/25 with a skill, and the skill was invoked in 19 of 25 runs.
Merge Danger
Door: two-way. The new profile key and result fields are additive. Existing arms behave as before.
Blast Radius: claude-code-runs. Repos that ship
.mcp.jsonwill now start those MCP servers, which changes the agent's tool set compared with earlier baselines for those repos.🤖 Generated with Claude Code