spec-kitty / spec-kitty/spec-kitty
review-cycle: adopted retained artifact with CRLF on-disk bytes can never verify durable under Windows autocrlf
- Dominant language
- Python
- Stars
- 1.6k
- Forks
- 165
- Avg merge
- 14h 52m
- Merged PRs (30d)
- 303
Description
## Finding
`create_rejected_review_cycle`'s adoption path commits a retained `review-cycle-N.md` **as-is** (whatever bytes are on disk), while the durability check (`review/cycle.py::_commit_review_cycle_artifact` → `_read_artifact_at_ref` vs `artifact_path.read_bytes()`) requires the governed-ref blob to be byte-identical to the local file.
On Windows with Git's default `core.autocrlf=true`, a retained artifact that was hand-edited (or written by any default text-mode tool) carries CRLF bytes on disk. `git add`'s clean filter stores LF in the blob, `git show` reads LF back, and the byte comparison fails — so the retry is classified `persistence_failed` / `destination_readback_mismatch` even though the commit itself succeeded and the destination content is semantically identical.
## Reproduction (emulated on Linux, faithful to the Windows failure mode)
Windows text-mode writes (newline translation) + `core.autocrlf=true`:
1. Create a retained review cycle (commit fails/skips).
2. Rewrite the artifact on disk with default text-mode write (CRLF on Windows), e.g. touching `reviewed_at`.
3. Retry with the real commit router.
Result: classification `persistence_failed`, reason `destination_readback_mismatch`; git warns `LF will be replaced by CRLF the next time Git touches it`.
The canonical writer (`review/artifacts.py::ReviewCycleArtifact.write`) already forces LF via `write_bytes`, so only externally-modified retained artifacts are affected.
## Possible direction (not decided here)
Either the adoption path re-canonicalizes the adopted artifact to LF before committing, or the durability comparison normalizes line endings on both sides. The exact-bytes contract is deliberate (see the comment in `review/artifacts.py`), so this is a semantics decision, not a mechanical fix.
## Context
Found while fixing the test-side sensitivity in #3834 (PR: tests only — this finding is the production-side latent case the tests deliberately do not paper over).
Contributor guide
Research direction
Start at create_rejected_review_cycle and review/cycle.py::_commit_review_cycle_artifact, especially _read_artifact_at_ref, then inspect the exact-bytes contract and ReviewCycleArtifact.write in review/artifacts.py. Reproduce the retained-artifact retry with CRLF bytes and core.autocrlf=true. Done means the chosen line-ending semantics are explicit and the retry no longer misclassifies a successful durable commit.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, python
- Domain
- devtools, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100