Skip to content

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

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

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.

@Paul-Kyle

Copy link
Copy Markdown
Member

Closing this one, and I want to be straight about why rather than letting it drift any longer.

kratos0718 pushed the idempotency fix on #132 and it merged as ae225ee. It's a good fix — the same
property yours had, arrived at independently, with a regression test that fails on the prior head.
So the fallback this PR was holding open is no longer needed, and keeping it open would just be
keeping you on a hook.

What I said earlier stands and I don't want it buried by a close: your implementation had the
re-save property on its first push, before anyone asked for it, and the four red checks were never
in your logic. Being second by forty-five minutes decided this, not the quality of the work.

The save-receipt issue I promised you on 08-15 — the create-vs-replace signal on save — was blocked
this whole time because it touches palinode/core/save.py and this PR and #132 were both open
against that file. That's now clear. I'm filing it within 24 hours and reserving it for you, so it
won't be claimed out from under you the way this one was. You'll get an @-mention on it.

Thanks for the patience, and sorry it took this long to say so.

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