spec-kitty / spec-kitty/spec-kitty

review-cycle: adopted retained artifact with CRLF on-disk bytes can never verify durable under Windows autocrlf

Open
#4,280 0 comments 0 reactions 0 assignees View on GitHub
priority:P3 reliability type:finding windows
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.