Skip to content

fix(skills): skip flat admin files when writing to external output dir#9

Merged
joeVenner merged 2 commits into
mainfrom
fix/skills-skip-flat-files-external-dir
Apr 14, 2026
Merged

fix(skills): skip flat admin files when writing to external output dir#9
joeVenner merged 2 commits into
mainfrom
fix/skills-skip-flat-files-external-dir

Conversation

@joeVenner
Copy link
Copy Markdown
Contributor

@joeVenner joeVenner commented Apr 14, 2026

Summary

  • When --output-dir points outside ~/.iotcli/skills/ (e.g. ~/.claude/skills), system_prompt.md, iotcli.skill.yaml, and iotcli.tools.json were being written there alongside the per-device SKILL.md dirs
  • These files have no OpenClaw frontmatter and are not skills — they're admin artifacts that belong only in the canonical skills dir
  • system_prompt.md in particular could confuse an agent's skill loader if read as raw context
  • Fix: flat admin files are now only written when outputting to the default ~/.iotcli/skills/ dir

Test plan

  • iotcli skills generate --output-dir /tmp/test → only per-device dirs, no flat files
  • iotcli skills generate (no flag) → all files including system_prompt.md, iotcli.tools.json, iotcli.skill.yaml

@github-actions github-actions Bot added bug Something isn't working size: S < 50 lines changed area: skills AI agent skill generator and removed size: S < 50 lines changed labels Apr 14, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 14, 2026

Greptile Summary

This PR guards the three flat admin files (iotcli.tools.json, iotcli.skill.yaml, system_prompt.md) behind an is_default_dir check in generate_all, so they are only written to the canonical ~/.iotcli/skills/ directory and are not emitted when --output-dir points to an external agent skill loader like ~/.claude/skills. The fix is logically correct and well-motivated.

  • is_default_dir is computed via out.resolve() == self.skills_dir.resolve(), which correctly handles the common cases (no flag → default, absolute path → compared verbatim). Shell-expanded paths like /home/user/.iotcli/skills match correctly; only single-quoted ~ would not, which is an unusual edge case.
  • _cleanup_stale still iterates over the external directory and deletes any subdirectory containing a SKILL.md file whose name is not an active iotcli device slug. In a shared dir such as ~/.claude/skills this can silently delete third-party skill directories that iotcli never wrote — a meaningful data-loss risk for the exact use case the PR is enabling.
  • No automated tests are added for the new conditional logic; the test plan is manual only.

Confidence Score: 3/5

  • Safe to merge for the stated fix goal, but the stale-dir cleanup can delete non-iotcli skill files in the shared external dir that this PR is specifically designed to support.
  • The is_default_dir guard for admin files is correct and well-implemented. However, _cleanup_stale still runs against the external directory and can destructively remove third-party skill dirs in ~/.claude/skills — the primary motivating use case. This is a data-loss risk on the user's happy path that should be resolved before shipping.
  • src/iotcli/skills/generator.py — specifically the _cleanup_stale call path in generate_all when is_default_dir is False.

Important Files Changed

Filename Overview
src/iotcli/skills/generator.py Adds is_default_dir guard in generate_all so flat admin files (iotcli.tools.json, iotcli.skill.yaml, system_prompt.md) are only written to the canonical ~/.iotcli/skills/ directory; _cleanup_stale stale-dir removal continues to run for external dirs and may delete non-iotcli skill dirs in shared external paths.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["generate_all(output_dir)"] --> B{"output_dir provided?"}
    B -- "No" --> C["out = self.skills_dir (canonical)"]
    B -- "Yes" --> D["out = Path(output_dir)"]
    C --> E["Write per-device SKILL.md dirs"]
    D --> E
    E --> F{"is_default_dir?\nout.resolve() == skills_dir.resolve()"}
    F -- "True (canonical dir)" --> G["_cleanup_stale(purge_legacy=True)\n removes stale device dirs +\n legacy *.skill.md files"]
    F -- "False (external dir)" --> H["_cleanup_stale(purge_legacy=False)\n removes stale device dirs only\n ⚠️ may delete non-iotcli dirs"]
    G --> I["Write iotcli.tools.json\nWrite iotcli.skill.yaml\nWrite system_prompt.md"]
    H --> J["Skip flat admin files ✓"]
    I --> K["Return results"]
    J --> K
Loading

Comments Outside Diff (1)

  1. src/iotcli/skills/generator.py, line 459-469 (link)

    P1 Stale-dir cleanup deletes non-iotcli skill dirs in shared external paths

    The primary motivation for this PR is to make --output-dir ~/.claude/skills safe for agent skill loaders. However, _cleanup_stale still iterates over the entire external directory and deletes any subdirectory that (a) contains a SKILL.md file and (b) is not named after a current iotcli device slug. In a shared dir like ~/.claude/skills this will destructively remove third-party skill dirs that iotcli never created.

    Concretely: if ~/.claude/skills/my-other-tool/SKILL.md exists and my-other-tool is not an iotcli device, generate_all --output-dir ~/.claude/skills silently deletes it.

    The comment on line 185 acknowledges this ("we still remove orphan per-device SKILL.md dirs (we created those)") but the code has no record of which directories it actually created, so the assumption doesn't hold for a shared external dir.

    A targeted fix would be to skip stale-dir cleanup entirely when is_default_dir is False, or to track created directories and only clean up known ones:

    # Only run stale cleanup when we own the directory
    if is_default_dir:
        self._cleanup_stale(out, live_slugs, purge_legacy=True)

    or alternatively, limit the per-device cleanup to only the slugs that were generated in the current invocation:

    # In external dirs, only remove dirs we just overwrote whose device is gone
    if not is_default_dir:
        self._cleanup_stale(out, live_slugs, purge_legacy=False)

    If the existing conservative approach (remove stale dirs even in external dirs) is intentional, the comment on line 185 should be strengthened to warn users that the external dir will have orphan device dirs cleaned up, so they should not co-locate unrelated skills with the same name as an iotcli device slug.

Reviews (2): Last reviewed commit: "docs(skills): update module and generate..." | Re-trigger Greptile

@joeVenner
Copy link
Copy Markdown
Contributor Author

@greptile review

@joeVenner joeVenner merged commit e37cea5 into main Apr 14, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: skills AI agent skill generator bug Something isn't working size: S < 50 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant