Skip to content

fix: new notes skip indexed ids, and fetch records only indexed notes - #160

Open
fduple wants to merge 1 commit into
jordan-gibbs:mainfrom
fduple:pr/fetch-refused-note-url
Open

fduple wants to merge 1 commit into
jordan-gibbs:mainfrom
fduple:pr/fetch-refused-note-url

Conversation

@fduple

@fduple fduple commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

What this changes

write_note takes a taken_ids set and skips those ids as it skips an existing file; the five paths that fetch and save a note record a URL only when sync has indexed the new note under its own id.

Why

note mv keeps a note's id while renaming its file, so after note mv topic topic-moved the filename topic.md is free while the id topic is taken. write_note checked only for an existing file, so the next note titled "Topic" was written to topic.md and sync refused it (id collision: 'topic' already belongs to 'research/notes/topic-moved.md'). The fetch paths then recorded the URL (and with --save-assets the screenshot) under the moved note's id, so the duplicate-url check answered URL already fetched as note 'topic' for the new URL and the fetched text was never indexed. note new, MCP create_note and the research map-of-content reported that id for a file sync never indexes. graph stub and repair --stub: the link resolver never matches a ref by slug, so a stub for [[foo bar]] could take an id another note holds, be refused by sync, and leave the link broken.

On main, with a stub provider that titles every page "Topic":

fetch https://a.example.org/x --save-assets  -> note_id "topic"
note mv topic topic-moved
fetch https://b.example.org/y --save-assets  -> exit 0, "topic"

leaves sources (https://b.example.org/y, topic), the screenshot under topic, topic.md unindexed, and a repeat of the second fetch refused with DUPLICATE_URL naming topic.

How

Prevention: indexed_note_ids(conn) feeds write_note(taken_ids=...) at every caller that derives an id: fetch, fetch-batch, research's _save_result and map-of-content, escalation ingest, fetch_and_save, note new, MCP create_note, graph stub and repair --stub. The second fetch above becomes topic-2, the map-of-content gets a -2 id too (research now reports the id it was written under), and the stub foo-bar-2, which [[foo bar]] resolves to through its title.

Guard: after sync, the five fetch paths call discard_unindexed_note; when another file took the id between write_note and sync, the file and its raw artifact are removed and the error names the file and the id's owner. fetch and escalation ingest exit 1 with NOTE_NOT_INDEXED (the item stays claimed); fetch-batch lists the URL in failed_urls with phase index; _save_result skips it with a stderr warning; fetch_and_save raises RuntimeError.

How it was verified

  • tests/test_cli/test_note_id_taken.py (15 tests) drives each case through each caller and forces the race at each fetch path (a second file indexed under the new id after write_note returns); tests/test_core/test_note.py adds one test for taken_ids. Against main all 16 fail (14 on assertions, the fetch_and_save race test because no RuntimeError is raised, the test_note.py test with a TypeError).
  • Any one changed CLI or MCP file restored to main fails its tests on their assertions; core/note.py or core/fetcher.py restored alone fails on a missing import, so their behaviour was also removed by hand, and the matching tests fail, as does each race test with its guard call disabled.
  • pytest tests/: 1735 passed on 3.11, 3.12, 3.13 and 3.14 (Linux). ruff check src/ tests/ is clean. Windows was not run.

Limits

  • note new --template with an existing template (the default note is one) uses its own filename loop and is not covered: after the note mv above it still returns id topic for a file sync refuses.
  • note new, create_note, the map-of-content and the stubs get no guard after sync; they record nothing under the id, so a race there only misreports it.
  • Unchanged: note mv still leaves the moved note's sources row with note_id NULL and deletes its assets rows, --save-assets still overwrites an existing screenshot.png, and graph stub still reports the link text as each stub's id.

Scope check — the review is much faster when the diff contains only the change being proposed:

  • No unrelated reformatting, whitespace, or prose rewriting outside the lines this change needs
  • CHANGELOG.md touched only to add an entry under [Unreleased]
  • No edits to user-facing output strings, docstrings, or test fixtures unrelated to this fix
  • If you ran a formatter or an automated cleanup pass, its results are in a separate PR

`note mv` keeps a note's id while renaming its file, so after
`note mv topic topic-moved` the filename topic.md is free on disk while
the id `topic` is still taken. write_note's collision loop checks only
for an existing file, so the next new note titled "Topic" was written
to topic.md, and sync refused it ("id collision: 'topic' already
belongs to 'research/notes/topic-moved.md'").

The fetch paths then went on to record the URL, and with --save-assets
its screenshot, under note_id = note_path.stem, which is the moved
note. From then on the duplicate-url check answered "URL already
fetched as note 'topic'" for the new URL, naming the wrong note, and
the fetched text was never indexed. This runs in all five places that
fetch and save a note: `fetch`, `fetch-batch`, research's
_save_result, `escalation ingest` and core.fetcher.fetch_and_save (the
MCP fetch_url tool).

The other callers that create a note under a derived id report an id
that is not the new note's. `note new` and the MCP create_note tool
return id "topic" (the moved note) for a file sync never indexes, and a
second `research` run on a moved map-of-content lists it under the
moved note's id. `graph stub` and `repair --stub` hit the same wall
from a different side: the link resolver matches a ref by id, alias or
title, never by slug, so a broken [[foo bar]] can slugify to "foo-bar"
while a note titled something else holds that id. The stub written for
it is refused by sync and the link stays broken.

Reproduction of the fetch case on main, through the CLI app with a
stub provider that titles every page "Topic", as the CLI tests stub it:

    fetch https://a.example.org/x --save-assets  -> note_id "topic"
    note mv topic topic-moved
    fetch https://b.example.org/y --save-assets  -> exit 0, "topic"

Afterwards the notes table has one row, (topic,
research/notes/topic-moved.md); sources has
(https://b.example.org/y, topic); the screenshot asset is filed under
topic; topic.md sits on disk unindexed and `sync` reports the
collision; and fetching https://b.example.org/y again fails with
DUPLICATE_URL naming 'topic'. `fetch-batch` leaves the same rows.

The fix: write_note takes an optional taken_ids set and skips those ids
through the same fixed-point collision ids it uses for an existing
file. The default is None. indexed_note_ids(conn), next to it in
core/note.py, returns the ids in the notes table, and every write_note
call that creates a note under a derived id passes it: the five fetch
paths (fetch-batch reads them once; the ids it writes itself are on
disk), `note new` (both its write_note calls), MCP create_note, the
research map-of-content, `graph stub` and `repair --stub`. `note new
--template` with a template that exists does not call write_note and is
not covered (see below). In the cases above the new note becomes
topic-2, the map-of-content likewise gets a "-2" id (and `research`
now reports the id it was written under rather than the slug it
started from), and the stub foo-bar-2, which [[foo bar]] resolves to
through its title "Foo Bar".

The fetch paths also get a guard after sync, discard_unindexed_note,
which checks that the notes row for the id is the new file. If it is
not, because another file took the id between write_note and the sync
or sync refused the file for another reason, nothing is recorded: the
file and its raw artifact are removed (and the index re-synced if the
file had a row), as the duplicate-url race path in `fetch` already does
for its own stray file, and the caller reports an error naming the file
and the id's owner, or that the id is not in the index. `fetch` and
`escalation ingest` exit 1 with NOTE_NOT_INDEXED (the
escalation item stays claimed, so it can be ingested again);
`fetch-batch` lists the URL in failed_urls with phase "index";
_save_result skips the result with a warning on stderr; fetch_and_save
raises RuntimeError. fetch-batch runs the check between its sync and
its inserts, because the clean-up's sync cannot begin a transaction
inside the one the inserts have open.

Tests: tests/test_cli/test_note_id_taken.py (15 tests) drives each case
above through each caller, and forces the race at each fetch path by
writing and indexing a second file with the new note's id right after
write_note returns; tests/test_core/test_note.py adds one test for
taken_ids, including an id at the 80-character cap. Against main's src
all 16 fail: 14 on assertions, fetch_and_save's race test because no
RuntimeError is raised, and the test_note.py test with a TypeError.
With any one changed CLI or MCP file restored to main, that file's
tests fail on their assertions, and so does the map-of-content test
with its hunk undone. Restoring core/note.py or core/fetcher.py alone
fails on a missing import or the taken_ids keyword rather than on
behaviour, so for those two the behaviour was also removed by hand
(taken_ids ignored; fetch_and_save's taken_ids argument and guard call
dropped). The matching tests then fail, as does each fetch path's race
test with its guard call disabled: on their assertions, or for
fetch_and_save on a RuntimeError raised or not raised.

`pytest tests/`: 1735 passed on 3.11, 3.12, 3.13 and 3.14 (Linux);
`ruff check src/ tests/` is clean. Windows was not run.

Not changed, and separate: `note mv` still drops the moved note's
database-only rows (its sources row is left with note_id NULL and its
assets rows are deleted, visible in the reproduction above),
--save-assets still writes screenshot.png into the assets directory
without checking for an existing file, a sources row survives a later
correction of its note's `source:` frontmatter, and `graph stub` still
reports the link text as each stub's "id". `note new --template` is
not covered when the template exists (the default `note` template
does): that branch picks its filename with its own loop, which still
checks only for an existing file, so after `note mv topic topic-moved`
a `note new Topic --template note` still returns id "topic" for a file
sync refuses, as on main. `note new`, create_note,
the map-of-content and the stubs get no guard after sync: they record
nothing under the id, so a race there only misreports it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant