fix(integrations): atomically record generated skill files - #4405
fix(integrations): atomically record generated skill files#4405Gambit-Checkmate wants to merge 1 commit into
Conversation
mnriem
left a comment
There was a problem hiding this comment.
The two modified files need to be normalized back to LF before this can be reviewed. The repository enforces * text=auto eol=lf, but this commit introduces mixed CRLF/LF endings across almost every line: the displayed diff is 2,778 additions and 2,772 deletions, while git diff -w reduces it to roughly 22 changed lines. This makes the substantive change effectively unreadable and creates unnecessary merge risk.
Please also demonstrate that the regression test fails on main before the implementation change. As written, it appears to assert behavior the existing write_file_and_record() path already provides, so it does not yet establish that switching from write-then-record_existing() to record_file() fixes the reported upgrade scenario.
Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol).
|
Thanks for the review. I normalized the local investigation and re-ran the reported upgrade path in an isolated environment: initialize with v0.5.0, then run v1.0.1 init --here --integration claude --force in the same project. The upgraded project contains 10 .claude/skills/speckit-*/SKILL.md files, and claude.manifest.json records all 10, including speckit-converge/SKILL.md. A fresh v1.0.1 install also records all 10.\n\nBecause I cannot make the reported behavior fail before the proposed implementation change, this PR does not demonstrate a valid fix. I am closing it rather than retain a noisy or unproven change. If you can provide the minimal project state or environment condition that reproduces the missing entry, I would be glad to revisit it with a failing regression test first. |
Summary
Fixes #4273
Validation
The full repository test suite is not available in the local environment, so CI should run the integration test.