fix: installer only removes or overwrites skill dirs it created (#8) - #28
Conversation
Co-Authored-By: Subash Natarajan <suboss87@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…gerprint Co-Authored-By: Subash Natarajan <suboss87@gmail.com>
…aude Co-Authored-By: Subash Natarajan <suboss87@gmail.com>
Runtime verification — installer safety fixes (
|
| shape | with --force |
result |
|---|---|---|
| link → directory (genuine fdeops fingerprint) | ✔ tested | refused, target byte-identical |
| link → regular file | ✔ | refused, file unchanged, no stack trace |
| dangling link | ✔ | refused, target never created, exit 0 |
link → target inside ~/.claude/skills |
✔ | refused, target unchanged |
legacy-named link (healthcare-fde) |
✔ | target dir, SKILL.md and extra file all survive; link left in place |
skip 1 skill path(s) that are symlinks - fdeops will not write through them:
~/.claude/skills/fde -> /tmp/r5/h.fNdXuK/outside
Containment asserted directly rather than trusting the message: find $HOME -name .fdeops-managed -not -path '*/.claude/*' → 0, and no references/*.md outside .claude → 0.
The wording nit found here (the message assumed the target was outside ~/.claude/skills, which is not true for an inside-target link) is fixed in 109665b.
Tightened adopt fingerprint — 7 shapes probed
| SKILL.md shape | adopted? | overwritten? |
|---|---|---|
genuine description: Engagement fieldbook for Forward Deployed Engineers |
yes (upgrade works) | yes |
description: MY OWN skill. I use fdeops sometimes. |
no | no |
| phrase in the body only / no frontmatter / mid-value / different phrase | no | no |
| ALL-CAPS variant | yes (/i) |
yes |
In the prose-mention case the user's SKILL.md is byte-identical, their extra file survives, no marker is planted, and the dir is reported as a collision.
Graceful failures + the partial-overwrite answer
Read-only skill dir and read-only parent: 0 stack-trace frames, permission denied at <path>, exit 1, and — the round-4 abort — ~/.claude/hooks still installed.
On staging+rename: a partial state is still reachable. Forcing a real mid-copy failure (delete one reference file and make references/ read-only) replaces SKILL.md before failing, leaving a loadable-but-inconsistent dir. Accepted as-is — it's reported, exits 1, and the retained marker lets a re-run fully repair the tree (diff -rq identical). Only managed/--force content is affected; a failed first install leaves no marker, so a broken dir is never falsely claimed as ours.
Heads-up for anyone re-testing: chmod 500 on a complete references/ does not fail, because copyFileSync rewrites existing 644 files without needing dir write permission.
Regression + four-way merge
User-authored healthcare-fde/fde-audit/gov-fde survive byte-identical and are reported; marked legacy dirs removed; --force takes over; pre-marker adopt upgrades to shipped content; re-run idempotent with no collision noise.
Merged with #29+#30+#31 on Main (all four verified as ancestors): 52 gates, 83/83. Only conflict is a one-hunk union in test/fde-cli.test.js with #30 (both append at the same EOF anchor, no logic overlap). On the merged tree the symlink guard still holds and node bin/install.js demo is not swallowed by the --force argv filtering:
Not covered: dashboard --open (sandbox chrome shim URI-encodes the path).
Full report: test-report.md · recording: /home/ubuntu/screencasts/fdeops-r5-merge/fdeops-r5-merge-edited.mp4
…s demo test Co-Authored-By: Subash Natarajan <suboss87@gmail.com>
Summary
Closes #8.
removeLegacySkills()deleted 16 hardcoded paths under~/.claude/skills/guarded only byexistsSync(p/SKILL.md), andcopyDir(SKILLS_SRC, GLOBAL_SKILLS_DIR)overwrote unconditionally — so a user's ownhealthcare-fde/(orfintech-fde/,gov-fde/, anyfde-*) was destroyed unrecoverably by an install they ran to gain a tool. For a product whose pitch is "nothing unreviewed, your data stays yours", this is the worst possible first impression.Ownership is now explicit, as the issue suggests. Every directory the installer creates gets a
.fdeops-managedmarker, and nothing without one is touched:copyDir(SKILLS_SRC, …)is replaced byinstallSkillDirs(), which applies the same rule per skill directory, so an unrelated~/.claude/skills/fde/is skipped rather than clobbered. Collisions print the paths and the way out:Upgrades keep working. Installs predating the marker have no marker either, so a strict rule would freeze them at their current version.
wasInstalledByUs()adopts a same-named directory whoseSKILL.mdis recognizably fdeops', printsadopt …, and marks it. That fingerprint is consulted only to overwrite, never to delete — a v2 directory without a marker is skipped, because a user'shealthcare-fdebuilt on fdeops would mention fdeops too. The v2→v3 cleanup therefore becomes opt-in (--force) for anyone upgrading from before markers existed; the tradeoff is losing an automatic cleanup rather than losing a user's work.--forceis accepted anywhere in argv and threaded through bothcmdInstallandcmdAdapters.Two regression tests: a user-authored
healthcare-fde/(with an extranotes.md) and an unrelatedfde/survive a full install and are reported,--forcethen takes both over, a marked v2 dir is still auto-removed; and a pre-marker fdeopsfde/is adopted and upgraded. 80/80 tests + 51 gates pass.docs/install.mddocuments the marker and the escape hatch.Link to Devin session: https://app.devin.ai/sessions/f135381c4682413bae73dff38eb6d1a3
Requested by: @suboss87