fix: protect local skill edits

[chaim0m]

Summary

• protect locally edited managed files on install and update paths
• recognize known released skill digests when upgrading installations that predate manifests
• save forced-overwrite backups in unique sibling directories outside the managed skill root
• preserve every backup across repeated --force operations
• reject dci skill --force unless --all or a named agent provides a target

Why

Follow-up to Alfredo's review comment on #35. Installs could overwrite local edits, while a naive backup implementation would block legacy upgrades, overwrite earlier backups, and expose backup files as skill content.

Test methods

go test ./...
go vet ./...

Coverage uses temporary directories for legacy no-manifest upgrades, unforced protection, repeated forced backups, backup placement, and invalid flag combinations.

Could this break things?

Risk: medium, intentional safety tightening. Reinstalling over genuine local edits now needs --force. Unmodified files shipped by prior releases are recognized as clean, and forced backups remain recoverable outside the managed skill directory.

Jira

CMP-48650

[apgiorgi]

Extending the local-edit guard to the install paths is right, and content-hashing against the manifest is the correct detection mechanism — a CLI upgrade alone correctly doesn't read as a local edit. Two problems to fix.

1. This breaks upgrades from v1.5.1 and earlier.

skill_management.go:209-215 falls back to current embedded digests when no manifest exists. But v1.5.1 shipped dci skill claude without writing a manifest (git show v1.5.1:main.go:1001), and skills/dci-cli/SKILL.md has changed since (d8fabcd to 143cfc2).

So: a user on v1.5.1 upgrades, runs the documented dci skill claude (README:176), and installSkillSafely (skill_management.go:80-92) fails with installed skill has local changes to SKILL.md for a file they never touched. Their only escape is --force, which then drops a SKILL.md.bak containing the old shipped file.

Pre-#35 installs need either a manifest backfill or a fallback that treats "no manifest, digest matches any known shipped version" as clean. At minimum a missing manifest shouldn't block a fresh install-over.

2. A second --force destroys the first backup.

skill_management.go:102-103 uses a fixed path + ".bak" with os.WriteFile, which truncates. So: edit, --force (backup holds edit #1), edit again, --force (backup now holds edit #2, edit #1 gone). Since the whole point of the backup is recovery, this should either refuse to clobber an existing .bak or use a distinguishing suffix.

Nits

• .bak files are written inside the agent's skill directory (skill_management.go:102), where agents glob references/ and will pick them up as content. Better outside the skill root.
• dci skill --force without --all is accepted and silently ignored (skill_management.go:328,345).
• The error says to "inspect them" but nothing shows the local diff — skill list only lists embedded files.
• Coverage is one happy-path case (skill_management_test.go:150-197). Nothing covers the no-manifest legacy path (issue 1), repeated --force (issue 2), or the claim that .bak files are treated as unmanaged.

fix: is defensible for a data-loss guard, though it also adds flags — feat: would be equally correct.

[chaim0m]

Addressed both blockers and the related nits. Manifest-free installs recognize known released digests, forced backups use unique sibling directories outside the managed skill tree, repeated force operations preserve every backup, and root --force now requires --all or a named agent. Added legacy-upgrade, repeated-backup, placement, and flag-validation coverage. go test ./... and go vet ./... pass.

[chaim0m]

All skill-safety requests remain addressed after merging current main: known manifest-free releases upgrade cleanly, backups are unique and outside the managed skill root, repeated force operations preserve prior backups, and invalid root --force usage is rejected. go test ./... and go vet ./... pass. Re-requesting review on the current head.