Skip to content

fix: normalise indexer path keys on Windows (#18)#19

Merged
dubadub merged 7 commits into
mainfrom
fix/windows-path-separator-18
May 16, 2026
Merged

fix: normalise indexer path keys on Windows (#18)#19
dubadub merged 7 commits into
mainfrom
fix/windows-path-separator-18

Conversation

@dubadub
Copy link
Copy Markdown
Member

@dubadub dubadub commented May 15, 2026

Summary

  • Fixes #18 — on Windows the indexer was keying file paths with backslashes (plats\pates-carbo.cook) while the downloader keys them with forward slashes (plats/pates-carbo.cook). compare_records treated the two as different paths, so every downloaded file produced a spurious DeleteForm (and subsequent re-create) per indexer cycle. End result: data loss on first Windows sync.
  • One-line fix: switch path.strip_prefix(base)?.to_string_lossy() to to_slash_lossy() in client/src/indexer.rs::build_file_record. path-slash was already a runtime dependency and is used the same way at the upload boundary in client/src/remote.rs:259.
  • Adds two regression tests: a unit test on build_file_record, and an integration test simulating the post-download state in client/tests/indexer_tests.rs.
  • Adds .github/workflows/test.yml — multi-platform CI matrix (ubuntu-latest, macos-latest, windows-latest) running cargo test --workspace on push and PR. The repo had no test-running CI before; this gap is the structural reason the bug went unnoticed. --all-features was deliberately avoided because the server's database_postgres feature requires libpq which isn't available on the Windows runner and is unrelated to this fix.

Out of scope

The issue also called out two defense-in-depth follow-ups for the destructive paths in check_download_once (registry guard, first-run sync ordering). Those are deferred to a separate PR — they address the broader "remote tombstone replay on an unindexed local" foot-gun, not the Windows-specific manifestation this PR fixes.

Test plan

The CI Test workflow will fill in the last three boxes automatically.

@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

Code Review

Overview

This PR fixes a real data-loss bug on Windows: the indexer was keying paths with backslashes while the downloader used forward slashes, so compare_records treated every downloaded file as simultaneously "missing" and "new", uploading spurious tombstones and destroying recipes. The one-line fix in build_file_record is correct, minimal, and consistent with how remote.rs:259 already normalises paths at the upload boundary.


What's good

  • Fix is correct and minimal. path-slash is already a runtime dependency used for the same purpose at the upload boundary; reusing it here is exactly right.
  • Test coverage at two levels. The unit test on build_file_record and the integration test on check_index_once together pin both the function contract and the end-to-end indexer/downloader key-agreement invariant.
  • CI addition. Adding a multi-platform matrix closes the structural gap that let this bug exist undetected.
  • PR description. Root cause, scope verification, and deferred follow-ups are clearly explained.

Issues

High: docs/superpowers files should not be in this PR

The two files added under docs/superpowers/ are AI agent working documents (an implementation plan and a spec), not repository documentation. They contain agentic-tooling instructions, step-by-step prompts, and bash commands for an agent executor. They add 582 lines to the diff while the actual fix is 3 lines. The PR description already captures all the context a future reader needs. Please remove these before merging:

  • docs/superpowers/plans/2026-05-15-windows-path-separator-fix.md
  • docs/superpowers/specs/2026-05-15-windows-path-separator-fix-design.md

High: use std::os::unix::fs::symlink will break Windows CI compilation

client/tests/indexer_tests.rs has an unconditional import at the top of the file:

use std::os::unix::fs::symlink;

std::os::unix does not exist on Windows — this is a compile error when test.yml runs on windows-latest. Both the import and the symlink test need to be cfg-gated:

#[cfg(unix)]
use std::os::unix::fs::symlink;

// ...

#[cfg(unix)]
#[test]
fn check_index_once_skips_symlinks() { ... }

Without this, the Windows CI job fails to compile the integration test binary entirely and never reaches the regression test that proves the fix works. This is a pre-existing issue in the file, but this PR is the one adding Windows CI, so it needs to be fixed here.


Minor

&vec![form] -> &[form] (indexer_tests.rs around the registry::create call): &vec![...] allocates a Vec just to immediately borrow it as a slice. &[form] is idiomatic Rust and avoids the allocation.

Overly defensive unwrap_or in the integration test timestamp setup:

m.replace_nanosecond(0).unwrap_or(m)

replace_nanosecond only returns Err if the value is out of the valid range; 0 is always valid. .expect("0 is a valid nanosecond") is clearer intent and avoids silently falling back to an un-truncated timestamp on a path that can never fail.


Summary

The core fix is correct and the test strategy is sound. Two things must be addressed before merging: remove the agentic planning docs from docs/superpowers/, and cfg-gate the std::os::unix import and symlink test so the Windows CI job can compile. Without the latter, the CI matrix added by this PR will fail on Windows for reasons unrelated to the actual bug, which undermines the proof-of-fix goal. Everything else is optional polish.

@dubadub dubadub merged commit e5417df into main May 16, 2026
7 checks passed
@dubadub dubadub deleted the fix/windows-path-separator-18 branch May 16, 2026 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First sync on Windows uploads tombstones for every downloaded file (path-separator mismatch)

1 participant