Skip to content

fix(save): disambiguate derived slugs to prevent silent overwrites - #133

Open
Karunasagar12 wants to merge 1 commit into
phasespace-labs:mainfrom
Karunasagar12:fix/save-slug-collision
Open

fix(save): disambiguate derived slugs to prevent silent overwrites#133
Karunasagar12 wants to merge 1 commit into
phasespace-labs:mainfrom
Karunasagar12:fix/save-slug-collision

Conversation

@Karunasagar12

Copy link
Copy Markdown

Summary

Fixes #129 — when slug is omitted, save_memory() derives one from the opening words of the content. Two saves whose openings agree resolved to the same file, silently overwriting the first.

What changed

In palinode/core/save.py, after deriving the slug (only when not explicitly passed):

  1. Check if the target path already exists
  2. If it does, compare the existing file's body to the new content
  3. If different → append sha256(full_content)[:8] to the slug
  4. Loop until the candidate filename is either unused or byte-identical (re-save)

Design decisions (per issue guidance)

  • Hash the full content, not a prefix — avoids the 100-char collision trap described in the issue
  • Loop until unused — handles the astronomically unlikely case of hash[:8] collision
  • Byte-identical re-save is idempotent — same content to the same derived slug hits the same file (not a collision, just a re-save)
  • Explicitly-passed slugs still overwrite — the documented escape hatch for idempotent updates is preserved
  • Stays inside core/save.py — does not touch ingest/pipeline.py (separate issue, as noted)
  • Uses frontmatter lib directly to extract body for comparison (already a project dependency)
  • Fail-open on unreadable file — matches existing pattern elsewhere in the function

Tests

Added tests/test_save_slug_collision.py — 5 tests:

Test What it proves
test_two_saves_shared_opening_produce_distinct_files Core fix works
test_content_identical_first_100_chars_still_disambiguates Prefix-hash trap is avoided
test_explicit_slug_still_overwrites Escape hatch preserved
test_identical_content_resave_is_idempotent No spurious disambiguation
test_three_collisions_produce_three_files Full reproduction from issue
============================== 5 passed in 0.40s ===============================

Existing save tests (29) also pass.

When slug is omitted, save_memory() derives one from the opening words
of the content. Two saves whose openings agree resolved to the same file,
silently overwriting the first with no error or warning.

Fix: after deriving the slug, check if the target path already exists
with different content. If so, append a sha256[:8] hash of the full
content to the slug. This produces stable, deterministic filenames that
won't collide unless content is genuinely identical (in which case the
re-save is allowed through as idempotent).

Design decisions per issue phasespace-labs#129:
- Hash the FULL content, not a prefix (avoids the 100-char trap)
- Loop until candidate filename is unused or byte-identical
- Explicitly-passed slugs still overwrite (documented escape hatch)
- Stays inside core/save.py (does not touch ingest/pipeline.py)
- Uses frontmatter lib directly (already a project dependency)

Tests:
- Shared opening line → two distinct files, both retrievable
- Identical first 100 chars, different after → still disambiguates
- Explicit slug → still overwrites (escape hatch preserved)
- Identical content re-save → idempotent (same file, no disambiguation)
- Three collisions → three distinct files

Fixes phasespace-labs#129
@Paul-Kyle

Copy link
Copy Markdown
Member

Thank you for this. I owe you an explanation for the overlap before asking anything else: kratos0718
claimed #129 at 03:15Z and opened #132 ten minutes later, but we failed to assign him, so the issue
still looked free when you arrived. You had no way to see that work was already underway.

I'm keeping this open as the fallback while #132 goes through a review round. Your implementation is
real work and covers the important derived-vs-explicit distinction; there is no reason for you to
spend time on the current red checks while the earlier PR is still being resolved. I'll update you
here as soon as that path is decided.

@Paul-Kyle

Copy link
Copy Markdown
Member

An update on the follow-up I promised you, because "within the day" has come and gone and silence
would be the wrong way to handle that.

It isn't filed yet, and the reason is the same collision that closed this PR: the save-receipt issue
touches palinode/core/save.py, and both #132 and this one are still open against that file. Filing
it now would put a third person in the same module — which is precisely the thing I apologised for
last time, so I'd rather be late than repeat it.

#132 hasn't moved in about a day. If it stays that way I'll come back to you either way — the issue
gets filed the moment that file is clear, and you're first in line for it. Nothing here is
contingent on you chasing me.

@Paul-Kyle

Copy link
Copy Markdown
Member

Correcting myself twice in one thread, which is not a great look but better than leaving either one
standing.

First, small one: my note earlier today called this PR closed. It isn't — it's open, and it was
always meant to stay open as the fallback. My mistake in the writing, not a change of status.

Second, and more worth your time. I went back through this properly today rather than trusting how I
first triaged it. Your implementation handles a case the other PR didn't: re-saving the same content
after it has already been disambiguated. Because your suffix is a hash of the full content, the
re-save computes the same filename, and your loop breaks when it finds matching content rather than
skipping to the next free name. I ran both algorithms against that scenario — yours produces two
files, the other produced four. That's the exact correction I sent to the other PR yesterday, and
you'd already handled it.

Your four failing checks are all mechanical and none is in the logic: an unused import yaml, a
zip() missing strict=, and bare #129 references in save.py comments and the test docstrings,
which trip a guard we have because a bare number means different issues in our two trackers. That
guard has now caught three contributors and that is our problem to fix, not yours.

I'm not asking you to push anything yet. #129 is assigned to kratos0718 and he gets the first move on
it — that's his to finish, and it would be wrong of me to run a race after telling you we don't. But
I wanted the record to say what your PR actually did rather than "arrived second," because the second
thing is the one that was true and the first thing is the one that mattered.

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.

save silently overwrites an existing memory when the slug is derived from content

2 participants