fix(skills,tools): stop two tools deleting data while reporting success - #3358

Verdict: Approve with suggestions

Two tools that quietly destroyed data now refuse or edit precisely: gaia skill remove . no longer wipes the whole skills directory (signing keys and trust store included), and the "replace one function" tool stops eating the code that happens to sit after its target. The fixes are the right shape — every place a skill name becomes a directory goes through one guarded helper, and the function edit now takes its boundaries from the parsed file rather than guessing by indentation — and the tests actually check the file on disk afterwards, which is what the old code lied about.

One thing worth a follow-up before or after merge: this PR newly lets you target a method by its full Class.method name, but if the replacement text isn't indented to sit inside the class, the edit can still land as a valid file with the method quietly moved out of its class — success reported, wrong result. A cheap check after the edit would close the last hole of the same kind this PR is about.

Nothing else blocking. No security concerns.

Real-world evidence

No automated evidence bundle was produced for this run (evidence-bundle.md absent), but the PR description carries the matching evidence itself and I did not re-run it:

• CLI — before/after transcripts of gaia skill remove . showing the old run printing ✅ Removed skill with exit 0 and an empty skills directory, and the new run refusing with exit 4 and everything intact, plus gaia skill remove demo-skill still working.
• Tool behaviour — a side-by-side file dump for the same replace_function call showing CONSTANT = 42 and the next function's decorator deleted on base, and only the target changed after.
• Agent UI / MCP / HTTP — marked N/A with a reason; neither surface is exposed there. Agreed.

The author flagged one gap honestly: gaia eval agent could not run here — the eval judge's structured-output call fails with a Claude billing error on that box — so the tool-docstring change has no eval behind it. The description also notes no tool_selection scenario exercises this tool, so the closest category wouldn't have covered it anyway. I'd still want one eval pass on a box with working judge auth before this is considered fully verified; it doesn't change the verdict, since the docstring change is additive guidance and the behavioural evidence above is direct.

I could not execute the test suite in this environment (pytest isn't installed on the runner), so the pass counts in the test plan are taken as reported, not reproduced.

class Alpha:
 x = 1
 def run(self):
 return "alpha"

 if not validation["is_valid"]:
 return {
 "status": "error",
 "error": "Replacement would result in invalid syntax",
 "syntax_errors": validation.get("errors", []),
 }
 # The span is exact, but a mis-indented replacement can still parse
 # cleanly with the definition relocated out of its class.
 try:
 _resolve_function_node(ast.parse(modified_content), function_name)
 except FunctionLookupError as e:
 return {
 "status": "error",
 "error": (
 f"Replacement no longer defines '{function_name}' in the "
 f"same place ({e}). Check the indentation of "
 "new_implementation."
 ),
 }

 raise FunctionLookupError(
 f"'{name}' is defined more than once (lines {where}). Refusing to "
 "guess which definition to replace — for an @overload stack or a "
 "conditional definition, edit the file with edit_python_file instead."
 )

 name = validated_skill_name(args.name, source="create")
 target = skill_directory(parent, name, source="create")

Closed in d58990a.

🟡 De-indented replacement relocating a method — fixed. The rewritten module is re-parsed before the write, and the target must still resolve at the same qualified name. The qualified name encodes the scope, so that check is the "parent scope unchanged" one, not just presence. Your exact case is pinned by test_a_de_indented_method_replacement_is_refused, with test_the_correctly_indented_replacement_of_the_same_method_succeeds beside it so the guard doesn't cost the legitimate edit. Both fail if I disable the check.

I went for a diagnosing message rather than wrapping _resolve_function_node, because reusing it directly gives "Function 'Alpha.run' not found in file" — the nested-name hint only fires for bare names, so it never mentions that run now exists at module level. The version I landed names where it went:

The replacement moves 'Alpha.run' to 'run'. Indent new_implementation to match the definition it replaces — nothing was written.

On the rename caveat: agreed, out of contract — refused. The docstring said "replace one function definition", and a silent rename leaves callers dangling with status: success, which is the failure class this PR is about. test_a_replacement_that_renames_the_function_is_refused pins it, and the message says so explicitly rather than reporting a generic miss. The tool docstring and the spec page now both state it.

🟢 1 — @overload. Taken as suggested. "Rename one" was wrong advice for a stack that is supposed to repeat a name; it now points at edit_python_file.

🟢 2 — _handle_create. Real bug, taken. validated_skill_name once at the top, then name for Skill(...), _scaffold_body, the scaffold-parse source, and both printed lines.

🟢 3 — symlinked skill directories. Correct reading, and I'm leaving it. install --force over a linked dev checkout now fails where it used to succeed. That path did rmtree(target), and on Windows rmtree follows a directory junction, so "succeeded" there meant deleting the checkout's contents — the same data loss this PR is closing, just via a link instead of a name. Refusing with a message that says to remove the link is the honest behaviour. If a real workflow depends on it, the fix is for install to resolve the link and refuse only when the target escapes the root — worth its own issue rather than widening this one.

Verification: pytest hub/agents/gaia/python/tests/ tests/unit/test_skill_loader.py tests/unit/test_agent_lazy_skill_prompt.py tests/unit/test_starter_skills.py tests/unit/test_skill_sets.py tests/unit/test_skills_{install,cli,migrate,marketplace,format,manager}.py tests/unit/test_file_write_guardrails.py tests/unit/test_skills_name_containment.py tests/unit/test_replace_function_spans.py -q → 1110 passed, 3 failed, 16 skipped. The 3 are the test_real_cli_* subprocess tests hitting RuntimeError: Could not determine home directory on Windows; identical on the base commit.

python util/lint.py --all → black, isort, flake8, bandit, conventions all pass. It exits 1 on 9 pre-existing pylint E1101s for os.killpg / os.getpgid / os.geteuid in daemon/sidecars/{ledger,manager}.py and installer/lemonade_installer.py — POSIX-only members flagged because pylint ran on Windows. Same 9, same three files, none in this diff.

Your last open item — "one eval pass on a box with working judge auth" — now has results.

ANTHROPIC_API_KEY= gaia eval agent --category tool_selection (judge claude-opus-5, Lemonade 11.5.0 / Gemma-4-E4B-it-GGUF): 3/5 passed, avg 7.5. The auth failure I reported earlier was not an account limit — load_dotenv() walks up into the parent checkout's .env and picks up an exhausted key, which flips eval/runner.py into --bare and cuts OAuth off. Setting the variable explicitly empty fixes it.

smart_discovery is red, and it is not this change. Reverting every source file in this PR to upstream/main fails it the same way:

upstream/main (reverted) FAIL 2.35 / FAIL 3.77 (empty index)
this branch FAIL 2.72 / 2.40 / 3.40 / 2.30

Six runs, two branches, three index states, never a pass. The judge attributes it to the allowlist — index_document and read_file denied on a directory where list_files succeeds (src/gaia/security.py:375) — filed as #3370, with #3369 and #3371 for two related gaps in the eval gate.

There is also a mechanical reason this PR could not have moved it: smart_discovery runs the doc profile, whose tool_groups=('doc_rag',) never registers file_io, and file/full pop replace_function out (agent.py:1962). The docstring this PR edits is in no chat profile's tool schema.

One change you did not ask for, in 8830646f: the replace_function docstring is trimmed 1110 → 631 chars, back in line with its neighbours. I made it the outlier in the scope-placement fix, and every tool docstring ships in the schema each turn. It did not move the eval — I checked before keeping it — and the rationale it lost is already in docs/spec/file-io-tools-mixin.mdx.

Full numbers, the sibling comparison, and why the committed baseline is not a valid comparator are in the PR's Evidence section.