Repository navigation
feat(repo): record whether the agent used what a patch: arm added (#41 D4) - #153
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 91.40% 91.55% +0.15%
==========================================
Files 216 223 +7
Lines 26741 27465 +724
Branches 4377 4522 +145
==========================================
+ Hits 24442 25146 +704
- Misses 1635 1648 +13
- Partials 664 671 +7
🚀 New features to boost your workflow:
|
vchamarthi
left a comment
There was a problem hiding this comment.
Not approving yet: 2 items to fix before merge.
Two ways the uptake count reports a use that is not a use of the arm's tool, which hides the new "used none" warning; both are in uptake.py/workspace.py and inline. The red self / repo validate is a pip --require-hashes failure on the dokimi @ git+... install, the same failure as on feat/repo-skill-arms, and passes on fix/repo-eval-hashed-install; it is not caused by this change.
CI at send time: 1 failing, 0 pending.
| servers = json.loads((ws / MCP_CONFIG).read_text(encoding="utf-8")).get("mcpServers") | ||
| except (OSError, ValueError, AttributeError): | ||
| return [] | ||
| return sorted(str(k) for k in servers) if isinstance(servers, dict) else [] |
There was a problem hiding this comment.
Should fix: When the patch changes an existing .mcp.json, mcp_servers records every server in the file, including the ones the base already had, so calls to those count as uptake of the arm; record only the servers the patch added (diff the mcpServers keys against the file at before).
_apply_patch passes files (added or changed paths) to mcp_servers(ws, files) at workspace.py:485, and mcp_servers (uptake.py:37-46) returns sorted(mcpServers) of the patched file with no comparison to the base. Scenario: base .mcp.json has {"mcpServers": {"github": ...}}, the patch adds "code.graph". arm_patch.mcp_servers becomes ["code.graph", "github"]; an agent that only calls mcp__github__* gets mcp_calls > 0, used: true, so repo compare prints no warning although the added server was never called. The guidance:raw baseline has the same github server, so this use is not something the arm added. The contract text ("the servers of a .mcp.json ... the patch added or changed") documents the same behavior; it should say servers the patch added. test_repo_uptake.py only covers a new .mcp.json.
There was a problem hiding this comment.
Fixed in 3117e66: the servers the base .mcp.json already declares are subtracted (read from the tree before the patch), so only added servers count. Test: test_a_mcp_json_the_patch_changed_names_only_the_servers_it_added. Contract text updated to say 'added'.
| files = sorted(set(changed) | touched) | ||
| # -z output ends in a NUL, so the split has one trailing "" the pairing drops | ||
| new = {p for k, p in zip(status[::2], status[1::2], strict=False) if k == "A"} | ||
| new |= {p for p in touched - set(changed) if (ws / p).is_file()} |
There was a problem hiding this comment.
Should fix: .mcp.json stays in arm_patch.added, so a Read of it or a command naming it (cat .mcp.json) counts as a use and suppresses the warning without one MCP call; drop MCP_CONFIG from added since its servers are tracked through mcp_calls.
workspace.py:483 filters added by guidance globs only, and test_repo_uptake.py asserts meta["arm_patch"]["added"] == [".mcp.json", "tools/fmt"] and result["uptake"]["files"] == [".mcp.json", "tools/fmt"]. In arm_uptake, wanted = set(files) (uptake.py:58) and a Read event of /eval/run/workspace/.mcp.json is normalized to .mcp.json by events._relative, so reads becomes 1 and used true (uptake.py:63). The command cat .mcp.json also matches _path_pattern (space before, end after). Reading the config is reading text, which is the case the warning exists to catch.
There was a problem hiding this comment.
Fixed in 3117e66: .mcp.json is no longer in arm_patch.added (its servers are counted through mcp_calls). Test: test_prep_records_the_added_files_that_are_not_guidance_and_the_mcp_servers now asserts added == ['tools/fmt'].
| kind = ev.get("kind") | ||
| if kind == "command" and named is not None and named.search(str(ev.get("text", ""))): | ||
| out["commands"] += 1 | ||
| elif kind == "read" and ev.get("path") in wanted: |
There was a problem hiding this comment.
Question: Is a Read of an added script (or cat tools/fmt, git diff tools/fmt) meant to count as uptake? For a tool file that is reading its text, the same signal the warning treats as 'not the tool'.
uptake.py:62-64 counts every read event whose path is an added file, and any command whose text names one as a whole word. The contract (repo-evaluation-contract.md:376-385) lists reads as uptake, so this may be intended for added documents; a split between files meant to be run and files meant to be read, or reporting commands/mcp_calls separately in the warning, would keep the two apart.
There was a problem hiding this comment.
Intended: a Read of an added file counts as uptake, as the contract lists reads (repo-evaluation-contract.md). The signal is 'the agent engaged with what the arm added', not only 'executed it'. 3117e66 states this in the uptake docstring and the contract; I did not split reads from commands, since the warning is about an arm whose additions the agent ignored entirely.
| noted = ("agent_timeouts", "agent_errors", "over_budget", "cost_unknown") | ||
| notes = [f"{r[k]} {k.replace('_', ' ')}" for k in noted if r[k]] | ||
| if "uptake" in r: | ||
| notes.append(f"arm's additions used in {r['uptake']['used']}/{r['uptake']['cells']} cell(s)") |
There was a problem hiding this comment.
Nit: uptake_counts returns unmeasured, but format_report and format_compare print only used/cells, so a treatment whose traces all failed to parse shows 0/0 and no warning; append the unmeasured count when it is non-zero.
There was a problem hiding this comment.
Fixed in 3117e66: format_report and the compare 'uptake:' line append '(warning: N more cell(s) left no trace to measure)' when unmeasured is non-zero. Test: test_the_text_report_and_compare_show_the_cells_that_could_not_be_measured (covers the note helper both use).
4b6f71e to
afdc62c
Compare
…tel#41 D4) A patch: arm that adds a tool can move a score through the guidance line that names it, with the tool never called. Prep now records the files the patch added that are not guidance, and the servers of an added .mcp.json. The runner counts commands that name those files, reads of them and calls of the servers' tools into result.json.uptake. report shows used/cells per arm. compare shows it per treatment and warns when a treatment used none of what its arm added. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A patch: arm can add .mcp.json as a symlink. mcp_servers followed it, so prep read a host file outside the workspace into meta.json, and a link to /dev/zero read without end. Resolve the path first and read it only when it stays in the workspace and is a regular file; a link loop names none. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
arm_uptake counted command, read and tool events whatever their denied flag, so a call a PreToolUse hook or permission_denials refused gave used: true and hid the "used none" warning. Skip denied events. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…added No test failed when the runner's measured gate, report's measured filter, compare's cells guard or --model filter, the A-only added rule, the gitignored added file or the .mcp.json-in-arm-files guard was removed. The fake harness gains a silent and an idle action for the gate. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 416a6ed. The design is sound and the counting rules in the description hold, but two open points from @vchamarthi still reproduce at this head, and I found a bare-file false positive and a test gap inline, plus a doc nit.
I agree with @vchamarthi on the base .mcp.json servers and on .mcp.json staying in added; both still reproduce here (a base {"mcpServers": {"base": {}}} patched to add new gives mcp_servers == ["base", "new"]). Their question on whether a read of an added tool counts as use, and the unmeasured display, wait for the author.
What I ran, on a Linux review host at 416a6ed:
make ci-local: 25 passed, 1 failed, 6 skipped. The failure ispip-audit, which the description says reports avirtualenvadvisory in the locks; this PR changes no lock (fixed onmainby #152).- 6738 tests passed, 1 skipped; changed-line coverage 96%.
- Reproductions of the code findings, with no model or network involved.
- A mutation run on the new tests: 117 mutants, 53 survive; the ones that matter are in the test-gap item below.
Checked, no issues
- Counting rules:
tools/fmt,./tools/fmt,sh tools/fmtand/eval/run/workspace/tools/fmtcount;cd tools && ./fmt,tools/fmt2,mytools/fmtandtools/fmt.bakdo not. - MCP names:
code.graphmatchesmcp__code_graph__*and notmcp__code.graph__*. - Patch parsing: the
--name-status -zpairing holds for paths with spaces and newlines, deletions, mode changes and binary files; renames are an add plus a delete under--no-renames. - Compat: old run dirs and guidance-only arms get no
uptakekey; no schema underschemas/names these files, so no version bump is needed. - Denied calls: the
deniedskip is pinned from both sides by the tests.
| return None | ||
| paths = "|".join(re.escape(f) for f in sorted(files, key=len, reverse=True)) | ||
| root = re.escape(workdir.rstrip("/") + "/") | ||
| return re.compile(rf"(?:^|(?<=[\s'\"`=;&|(<>])|(?<=\./)|(?<={root}))(?:{paths})(?=$|[\s'\"`;&|)<>:])") |
There was a problem hiding this comment.
A root-level added file counts as used when its name appears as any word of an unrelated command. The pattern matches the bare path at any word boundary, so a file without a / matches like a common word.
With files=["lint"], ["run"], ["build"] and workdir /eval/run/workspace, arm_uptake returns commands: 1, used: True for npm run lint, docker run img and git log -- build. The agent never touched the added file, and the "used none" warning is suppressed.
The contract says the command names the file "as a workspace path", which npm run lint does not.
Suggestion: for a file without a /, count it only when it is ./-prefixed, under the workdir, or the first word of the command.
There was a problem hiding this comment.
Fixed in 3117e66: a file with no '/' counts only as ./name, under the workspace path, first word of a command (or after ; && | ( ), or after an interpreter (sh/bash/python...). 'npm run lint', 'docker run img', 'git log -- build' no longer match. Tests: test_a_root_level_added_file_is_not_named_by_a_common_word_of_a_command, test_a_root_level_added_file_is_named_by_a_path_or_as_a_command.
| out["reads"] += 1 | ||
| elif kind == "tool" and prefixes and str(ev.get("name", "")).startswith(prefixes): | ||
| out["mcp_calls"] += 1 | ||
| out["used"] = bool(out["commands"] or out["reads"] or out["mcp_calls"]) |
There was a problem hiding this comment.
The tests do not pin that a read alone, or an MCP call alone, makes used true. Both existing positive cases combine reads=1 or commands=1 with mcp_calls=1.
Mutation check: dropping reads from the used expression, dropping mcp_calls from it, matching the server prefix without the trailing __ (mcp__code_graph2__x counting for code.graph), and gating uptake on only the files or only the servers all pass the full suite (61 passed in test_repo_uptake.py + test_repo_arms.py). Each is a regression that hides the "used none" warning or raises a false one.
Tests to add: arm_uptake([{"kind": "read", "path": "tools/fmt"}], ["tools/fmt"], [], WS)["used"] is true; the same with one mcp__code_graph__x tool event and servers ["code.graph"]; mcp__code_graph2__x gives mcp_calls == 0; and a run_cell case for a patch that adds only an MCP server.
There was a problem hiding this comment.
Added in 3117e66: test_a_read_alone_or_an_mcp_call_alone_is_a_use (read alone, MCP call alone, and mcp__code_graph2__x gives mcp_calls == 0). I did not add a run_cell case for an MCP-only patch or a files-only/servers-only gating test.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 073963e, which fix the nit from my review:
073963e:dokimi/docs/repo-evaluation-contract.md:377: theuptakeparagraph now namesfilesandmcp_servers, and describes theuptakeentry ({cells, used, unmeasured}) and theuptake: ...line ofrepo compare.
make ci-local at 073963e: 25 passed, 1 failed, 6 skipped (the failure is pip-audit, as at the reviewed head).
Not approving yet. Still open: the two risks from my review; @vchamarthi on arm_patch.mcp_servers records every server of a changed .mcp.json, including the base's; calls to a base server count as uptake and hide the warning (reproduced at this head); @vchamarthi on .mcp.json stays in arm_patch.added when the patch adds it, so a Read or cat .mcp.json counts as use (reproduced at this head).
…vel files strictly .mcp.json is no longer an added file, mcp_servers excludes servers the base declared, a slash-less added file matches only as ./name, under the workspace path, first in a command or after an interpreter, and the text report and compare show cells that left no trace to measure. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vchamarthi
left a comment
There was a problem hiding this comment.
No blockers. 1 non-blocking comment.
Both earlier majors are fixed: mcp_servers now drops servers declared in the pre-patch tree, and .mcp.json is out of added. The reads question is answered in the contract, and unmeasured cells now show in report and compare. One open question remains, on bare root-level names in commands; it does not block.
CI at send time: 0 failing, 0 pending.
| parts.append(rf"(?:^|(?<=[\s'\"`=;&|(<>])|(?<=\./)|(?<={root}))(?:{paths}){end}") | ||
| if flat: | ||
| names = "|".join(re.escape(f) for f in flat) | ||
| parts.append(rf"(?:^\s*(?:\./)?|[;&|(]\s*(?:\./)?|(?:^|(?<=\s)){_INTERPRETERS}\s+|(?<=[\s'\"`=;&|(<>])\./|(?<={root}))" |
There was a problem hiding this comment.
Question: Is lint x (and cd d && lint, asserted at test_repo_uptake.py:244) meant to count as running the workspace's lint? A bare command name resolves through PATH, so it does not run ./lint unless the workspace root is on PATH, and after cd d it cannot name the root file at all.
The flat-name branch of _path_pattern (uptake.py:109) accepts ^\s*(?:\./)? and [;&|(]\s*(?:\./)?, so the ./ is optional for a name first in a command. The docstring at uptake.py:17-18 says cd tools && ./fmt is not counted, which treats cd as changing what a path names; cd d && lint is counted. If the profile does not put the workspace on PATH, requiring ./ (or the workspace path, or an interpreter) for the first-word case keeps the count to runs of the arm's file.
There was a problem hiding this comment.
Not meant to: a bare lint runs whatever PATH finds, and the profile does not put the workspace on PATH. Fixed in eeff916: a root-level file counts only as ./lint, under the workspace path, or after an interpreter (sh lint). lint x and cd d && lint moved to the no-match cases in test_repo_uptake.py; the docstring and repo-evaluation-contract.md say why. Not covered: cd d && ./lint still counts, since the matcher does not track cd (same limit as the nested-path rule).
…e in intel#153 intel#184 already took main's growth since the intel#93 seed into the baseline with a generic rationale. Name the PR behind each of those entries instead, and add dokimi/repos/compare.py::_provenance_warnings, which intel#153 took over the complexity cap after intel#184 and which turns main's Tests job red again.
…e in intel#153 intel#184 already took main's growth since the intel#93 seed into the baseline with a generic rationale. Name the PR behind each of those entries instead, and add dokimi/repos/compare.py::_provenance_warnings, which intel#153 took over the complexity cap after intel#184 and which turns main's Tests job red again. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…153 (#183) #184 already took main's growth since the #93 seed into the baseline with a generic rationale. Name the PR behind each of those entries instead, and add dokimi/repos/compare.py::_provenance_warnings, which #153 took over the complexity cap after #184 and which turns main's Tests job red again. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Part of #41 (D4).
Summary
An arm that adds a tool can move a score through the guidance line that names the tool, with the tool never called. Each cell now records whether its agent used what its
patch:arm added.Counting rules (
dokimi/repos/uptake.py):tools/fmt,./tools/fmt,sh tools/fmt,/eval/run/workspace/tools/fmt. It does not countcd tools && ./fmt,tools/fmt2ormytools/fmt.mcp__<server>__…where<server>is a server in the arm's.mcp.json, spelled the way Claude Code spells it (code.graph→code_graph).measured: falsewhen the cell left no trace to count from.Arms with nothing to use (only guidance files,
guidance:*) get nouptakekey.Evidence
result.jsonof apatch:cell had no record of whether the added tool was touched. A compare ofguidance:rawagainstpatch:kitlooked the same whether the agent rantools/fmtor ignored it.tests/test_repo_uptake.py, with no docker and no LLM:added == [".mcp.json", "tools/fmt"], which excludes the patchedsub/AGENTS.mdand the newsrc/AGENTS.md.make ci-local: everything passes exceptpip-audit, which reports avirtualenvadvisory in the locks. That workflow runs only on PRs that change a lock, and this one changes none.Merge Danger
Door: two-way
The new keys are additive:
meta.json.arm_patch.added/.mcp_servers,result.json.uptakeandcompareprovenance.uptake. Old run dirs have nouptakeand report as before.Blast Radius: small
Only
patch:cells change. Acomparethat includes them can gain one warning line.🤖 Generated with Claude Code