fix(skills): do the temp-file cleanup in Python, not shell rm/find (#2790)

[abhay-codes07]

Fixes #2790.

Thanks @cdtatarevich — the diagnosis was already right, including which files a later --update would read back.

First: #2797 covers the same issue

@hudsonwa opened #2797 on this about a minute before I pushed, so please read these as alternatives and take whichever you prefer — I am not attached to mine landing.

The approaches differ, and the difference is mostly about surface area rather than taste:

• fix(cli): clean build intermediates with pathlib instead of rm #2797 adds a graphify clean <dir> subcommand and switches graphify/skill.md to call it. One command, no inline Python, and a reusable entry point.
• This PR folds the cleanup into the Python each block already runs, and goes through tools/skillgen/fragments/ so every host is regenerated.

Two things I would check on #2797 before choosing it, offered as review rather than argument:

1. It edits graphify/skill.md directly without the matching tools/skillgen/fragments/core/core.md change. The generated artifacts cannot be edited in place, and its CI reflects that — skillgen-check: FAILURE, alongside test (3.10): FAILURE. Fixable by moving the edit into the fragment and regenerating.
2. It fixes skill.md only. The other 20 skills and every references/update.md still carry rm -f / find -delete / Remove-Item, so the denial reproduces unchanged on codex, windows, droid and the rest.

Neither is a reason to prefer this PR over the idea in #2797 — a graphify clean subcommand is a cleaner call site than an inline loop, and it matches how the runbook already invokes graphify export obsidian and graphify query. If you want that shape, the right end state is probably #2797's subcommand called from the fragments, regenerated across all hosts. I am happy to close this and send exactly that, or to rebase this onto #2797 once it lands — the fragment and regeneration plumbing here is needed either way.

The change

Cleanup was the only part of the runbook that shelled out. It now runs in Python, so no host is asked to rm anything:

-rm -f graphify-out/.graphify_detect.json graphify-out/.graphify_extract.json ...
-find graphify-out -maxdepth 1 -name '.graphify_chunk_*.json' -delete 2>/dev/null
-rm -f graphify-out/.needs_update 2>/dev/null || true
+_out = Path('graphify-out')
+for _tmp in ['.graphify_detect.json', '.graphify_extract.json', ..., '.needs_update']:
+    (_out / _tmp).unlink(missing_ok=True)
+for _chunk in _out.glob('.graphify_chunk_*.json'):
+    _chunk.unlink(missing_ok=True)

The one design choice worth calling out: the cleanup is folded into the tail of the Python program each block already runs, rather than added as its own $(cat ...) -c step.

I tried the separate-step version first and backed it out. It works, but it adds a fence, an invocation line, a bare ", and a from pathlib import Path to every site — and --monolith-roundtrip then needs those generic lines added to the sanctioned-diff allowlist, which would blunt a guard whose whole job is catching drift in the hand-maintained monoliths. Folding adds only statements that name graphify's own intermediates, so the new predicate stays narrow.

I went through every site rather than just Step 9, since the same denial hits each one: Part B's temp files, Step 9, and the --update old-graph cleanup in references/shared/update.md. After this there is no rm/find -delete/Remove-Item left in any shipped skill or reference.

cp and mkdir -p are deliberately untouched. They are not destructive, so they are not what these policies withhold, and #2790 is about the delete step.

Two consequences I want to flag rather than bury

The PowerShell render no longer contains Remove-Item or Get-ChildItem. There is nothing left for the POSIX→PowerShell translator to rewrite, so test_powershell_hosts_carry_no_bash_only_shell — which asserted those cmdlets were present — now asserts they are absent. Changing an existing assertion deserves a second look, so: that test was pinning the #2528 translation of a line this PR removes, and the surviving assertions (no bash-only tokens, here-string invocation, chunk cleanup still happens) are unchanged.

Cleanup now runs only if the step's own program succeeds, where the shell form ran unconditionally (|| true). I think this is the better behaviour — a failed Step 9 keeps its intermediates for debugging, and the next run overwrites them anyway — but it is a real behavioural change and not strictly required by the issue, so say the word if you would rather it stayed unconditional.

Generated artifacts

Edited tools/skillgen/fragments/{core/core.md,core/devin.md,core/aider.md,references/shared/update.md} and regenerated; the other 60-odd changed files are python -m tools.skillgen output plus a re---bless of expected/. All five validators pass:

check OK: 134 artifact(s) match committed output and expected/.
audit-coverage OK: every per-host v8 heading single-homes in that host's render.
schema-singleton OK: the file_type enum is the six-value superset everywhere.
monolith-roundtrip OK: each monolith matches v8 modulo the enum unification.
always-on-roundtrip OK: each always_on/*.md reproduces its former constant byte for byte.

_is_shell_free_cleanup_line is the new sanctioned change-class for --monolith-roundtrip, sitting next to _is_chunk_cleanup_line, which sanctioned the #1172 rewrite of these same lines.

Tests

tests/test_skill_cleanup_is_shell_free.py (133 tests — mostly the per-file parametrization across every shipped skill and reference) asserts the property that closes the class rather than the specific strings:

• no shipped skill or reference deletes through a shell verb (rm -rf/rm -f, find -delete, Remove-Item, del /q), parametrized per file so a regression names the file;
• every skill that dispatches chunks still cleans them up — otherwise the guard above would pass trivially by deleting cleanup altogether;
• the snippet under test is pinned to what skill.md actually ships, so this file cannot drift into testing something the runbook no longer says;
• the replacement is executed in a temp tree and must remove every intermediate while leaving graph.json and GRAPH_REPORT.md, be idempotent, and tolerate a missing graphify-out/ (a --no-viz or cluster-only run that never wrote a chunk must not crash Step 9 — rm -f tolerated that and unlink(missing_ok=True) has to as well).

Reverting the fragments and keeping the tests fails 32 of them, on any platform — these are file-content assertions, so they have teeth on your Ubuntu CI, not just locally.

Validation

Windows 11, Python 3.12, branched off 4fca621 (0.9.44).

• Full suite: 20 failed, 4469 passed -> 21 failed, 4601 passed. The +132 are the new tests.
• All five skillgen validators pass (output above).
• The one extra failure is test_incremental_mtime_collision.py::test_same_size_rewrite_in_one_tick_is_requeued, which I do not believe this PR causes: it passes 4/4 in isolation and 4/4 running its whole file, and it also surfaced once in a full-suite run on an unrelated branch of mine (a cli.py hook change) before passing in isolation there too. It depends on two writes landing in the same filesystem timestamp tick, so it looks load-sensitive under a full run. Nothing here touches detect, cache or manifest. Flagging it rather than quietly reporting 20.

Note on #2782

That PR edits the same three core fragments and touches the .needs_update line in the same Step 9 block, so expect a textual conflict here and a large one in the regenerated artifacts. No semantic overlap — it is fixing the dot/no-dot flag name, this is removing the shell verbs around it. Happy to rebase on top of it if it lands first; no preference on ordering.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[graphify-labs]

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

---

Graphify review — findings

This PR replaces shell-based temp file cleanup commands (rm -f, find ... -delete) with inline Python cleanup using Path(...).unlink(missing_ok=True) and Path.glob(...) across the various per-platform graphify skill markdown files (agents, aider, amp, claw, codex, copilot, and others) as well as the corresponding skillgen generator/fragment tooling and expected-output test fixtures. The cleanup logic is folded into the existing Python blocks rather than run as separate post-script shell lines. The surface area spans the skill definition docs, the skillgen tool code, and the associated rationale/expected-output tests, keeping the generated skills and their test expectations in sync.

No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1297 functions depend on the 1297 functions this change touches.

Health — this change adds coupling hotspots:

• new: render() — 13 callers, 5 callees
• new: audit_coverage() — 8 callers, 6 callees
• new: main() — 3 callers, 11 callees
• new: monolith_roundtrip() — 3 callers, 5 callees
• new: test_audit_catches_a_dropped_non_allowlisted_heading() — 0 callers, 6 callees

Verification — 1297 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

• verification_scope: 1297 function(s) in the blast radius were not formally verified this run

· 5 more finding(s) on lines outside this diff (see the check run).