Skip to content

fix: stop duplicate Skill creation from overwriting existing data - #145

Open
harr1sz wants to merge 1 commit into
titanwings:dot-skillfrom
harr1sz:fix/prevent-skill-overwrite
Open

fix: stop duplicate Skill creation from overwriting existing data#145
harr1sz wants to merge 1 commit into
titanwings:dot-skillfrom
harr1sz:fix/prevent-skill-overwrite

Conversation

@harr1sz

@harr1sz harr1sz commented Aug 31, 2026

Copy link
Copy Markdown

Summary

Running --action create twice with the same {character}/{slug} currently reuses the existing directory. The second run can replace generated files such as work.md, persona.md, SKILL.md, and meta.json, then reset the lifecycle metadata to v1. No backup is created first.

This PR makes creation non-destructive. create claims the final directory with one atomic mkdir before writing any artifacts. If that path is already occupied, creation stops and tells the user to use --action update for the existing Skill.

Changes

  • Refuse create when any file, directory, or symlink already occupies the target path.
  • Reserve the target with mkdir(exist_ok=False), so two concurrent creators cannot both claim the same slug.
  • Keep the competing target untouched if another creator wins that reservation race.
  • Leave an incomplete directory in place if artifact generation fails, so cleanup cannot accidentally delete a path that another process replaced. The error tells the user to inspect it and remove it only after confirming it is the failed create they intend to discard.
  • Document the behavior in README.md, the maintained translated READMEs, INSTALL.md, INSTALL_EN.md, and both language sections of SKILL.md.
  • Add CLI and unit regressions for duplicate creation, concurrent reservations, dangling symlinks, write failures, and post-reservation path replacement.

User-visible behavior

Before this change, repeating a create command could silently rewrite an existing Skill.

After this change, the command exits without changing the existing files:

error: create target already exists: <path>; use --action update only if it is the existing Skill you intended to change

The normal update path is unchanged. It still archives the current artifacts and advances the existing Skill through the Evolution workflow.

If generation fails after the directory was reserved, the error names the path and explains the recovery boundary:

error: create failed after reserving <path>; the directory may be incomplete. Inspect it and remove it only if it is the failed create you intend to discard, then retry. Original error: <cause>

Why use an atomic directory reservation

An early exists() check is useful, but it is not enough on its own. Another process can create the same slug after the check and before the directory is created.

mkdir(exist_ok=False) is the ownership check. Directory creation is atomic: one creator gets the new directory, while later or concurrent creators receive FileExistsError before they can write an artifact.

If artifact generation later fails, the incomplete directory is left in place. Automatically deleting it by pathname would create another race: the path could have been replaced by another process before cleanup, causing unrelated data to be removed. Keeping it is the safer failure mode, and a later create still refuses to overwrite it.

The user can inspect the incomplete directory and remove it deliberately before retrying. The CLI and installation guide explain when this is an exception to the normal use update, do not delete rule. The code stays within the Python standard library and does not need a platform-specific filesystem layer.

Scope and compatibility

  • --action update, rollback, installation, and generated artifact formats are unchanged.
  • No new package or runtime dependency is added.
  • The fix applies to future create operations. It cannot restore a Skill that was overwritten by an older version.
  • This is one focused lifecycle fix. It does not redesign Evolution Mode or change how slugs are chosen.

Motivation

create and update represent different user intentions. Creating should claim a new slug once. Changing an existing Skill should go through update, where version history and rollback already exist.

Without this boundary, a repeated command, an automation retry, or two concurrent creators can turn a harmless mistake into data loss. The previous code also gave the user no recovery path because the overwritten files were never archived.

Related issue: none. On 2026-08-31, I checked the current dot-skill branch at 868c293, all open PRs targeting that branch, and the open Issues. I did not find an overlapping implementation or report.

Testing

  • python3 -m compileall tools/
  • python3 -m unittest discover -s tests -p 'test_*.py' -v on Python 3.9.6: 78 tests passed
  • uv run --python 3.11 --with requests python -m compileall tools/
  • uv run --python 3.11 --with requests python -m unittest discover -s tests -p 'test_*.py' -v: 78 tests passed
  • node bin/distilly.mjs --check-package: package payload is valid
  • git diff --check
  • Focused regression run with PYTHONWARNINGS=error
  • Manually exercised duplicate create, failed create recovery, and a clean retry
  • Ruff comparison for tools/skill_writer.py: no new findings; the same 9 existing findings appear on origin/dot-skill

The new tests verify that:

  • a second CLI create leaves the original metadata, work content, and version directory unchanged;
  • a competing target is preserved when it wins the atomic directory reservation;
  • files and dangling symlinks already occupying the target are rejected;
  • a failed create never deletes a directory that replaced its original reservation;
  • filesystem errors are reported without a traceback, and the incomplete directory remains available for inspection;
  • after deliberate removal of the confirmed incomplete directory, a clean retry succeeds.

Checklist

  • I read and followed CONTRIBUTING.md.
  • The PR addresses one concern.
  • Tests were added to the existing tests/test_*.py suite.
  • User-facing behavior is documented in README.md, SKILL.md, INSTALL.md, and INSTALL_EN.md.
  • No secrets, tokens, personal data, or generated private source material are included.
  • No new dependency is required.
  • I checked for overlapping open PRs and Issues before preparing the change.

Screenshots

Not applicable. This PR changes CLI and filesystem behavior.

中文说明

这次修复的是什么

现在如果对同一个 {character}/{slug} 连续执行两次 --action create,第二次会继续使用原目录。它可能直接改写 work.mdpersona.mdSKILL.mdmeta.json,还会把生命周期版本重新写成 v1,而且改写前没有备份。

这次修改把 createupdate 的职责分开:

  • create 只负责创建一个从未存在过的新 Skill;
  • 已有 Skill 必须通过进化模式或 --action update 修改,以便保留版本历史和回滚能力。

用户会看到什么

如果目标路径已经存在,命令会停止,不会碰原来的文件:

error: create target already exists: <path>; use --action update only if it is the existing Skill you intended to change

这个保护不只针对完整的 Skill 目录。只要目标位置已经被文件、目录或符号链接占用,create 都不会强行替换。

如果创建已经占用目录、但生成文件时失败,错误会明确给出路径,并提示这个目录可能不完整。用户需要先检查;只有确认它就是本次失败留下、确实要丢弃的目录时才手动删除,然后重试。普通的已有 Skill 仍然必须走进化模式,不能用这个例外绕过版本管理。

为什么不能只加一个 exists 检查

单纯在开头判断一次路径是否存在,仍然有竞态:检查之后、正式创建目录之前,另一个进程可能先创建同一个 slug。

现在的流程是:

  1. 用一次原子的 mkdir(exist_ok=False) 占用目标目录;
  2. 如果另一个进程已经抢先占用了目标路径,本次创建会在写入任何文件前失败,并保留对方的内容;
  3. 如果本次创建拿到了目录,就在里面生成文件;
  4. 如果生成过程报错,保留不完整目录,不按路径自动递归删除;
  5. 用户检查并明确处理这个目录后,再决定是否重试。

这个方案只使用 Python 标准库,不需要维护 macOS、Linux 和 Windows 各自的底层重命名接口。

这里不做自动删除。原因很具体:创建失败后,另一个进程可能已经替换了这个路径;此时按路径递归删除,反而会删掉别人的内容。下一次 create 仍然会拒绝覆盖,用户需要先检查,再决定如何处理。

这次没有改什么

  • --action update、版本归档和回滚逻辑没有变化;
  • 安装流程和生成文件格式没有变化;
  • 没有新增第三方依赖;
  • 这项修复只保护今后的创建操作,不能恢复旧版本已经覆盖掉的内容;
  • 没有借机重做进化模式或 slug 规则,PR 只处理重复创建可能造成的数据损失。

验证结果

  • Python 3.9.6:78 项测试通过;
  • Python 3.11:78 项测试通过;
  • tools/ 编译检查通过;
  • npm 包内容检查通过;
  • git diff --check 通过;
  • PYTHONWARNINGS=error 下的失败路径回归测试通过;
  • tools/skill_writer.py 没有新增 Ruff 问题,当前 9 项与上游基线一致。

没有关联 Issue。2026-08-31 准备这项修改时,我重新检查了 dot-skill 分支、所有指向该分支的开放 PR 和当前开放 Issue,没有发现重复实现或相同问题。

Reserve the target directory atomically before writing artifacts, and direct existing Skills to the update workflow. Keep failed creates for inspection so recovery never risks deleting another process's data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant