hans/zettelmesh
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

perf/index-first-lookups #2

Merged
hans merged 7 commits from perf/index-first-lookups into main 2026-09-05 16:26:59 +02:00
Conversation 0 Commits 7 Files changed 39 +2989 -116
hans commented 2026-09-05 16:26:48 +02:00
Owner
Copy link
No description provided. 
Registry input is now validated at the parser (registry.schema), not
just at the link/note-id layer:
- url: reject a leading '-' (git clone argument injection) and '::'
  (remote-helper transports like ext:: = RCE); allowlist
  https/ssh/git/file/abs-path/scp. embedding_cache_url too. (F1)
- shortname: enforce is_valid_shortname so '..' can't become a path
  segment in PathResolver. (F2)
- paths/exclude_paths: reject '..' and absolute patterns; Path.glob
  does not confine itself to the anchor on any CPython through 3.14. (F3)
GitRepository.clone adds '--' before the positionals and
'-c protocol.ext.allow=never'; note_repository.discover() drops any glob
match resolving outside the checkout. GitError redacts '://user:token@'
from urls in its message and stderr. (F1, F3, F7)

.env is loaded from the config dir / $ZETTEL_ENV_FILE only, never an
upward walk from the cwd, so a checkout's committed .env can't repoint
the embedding/chat endpoint or inject a key. (F4)

Prompt-injection framing (not filtering): the chat context wraps each
passage in <<<PASSAGE n>>> markers (body '<<<'/'>>>' defanged) with a
system-prompt rule that marked content is data; MCP note payloads gain a
content_trust field (body stays byte-exact). New always-on
suspicious-unicode LintRule flags bidi-override / zero-width chars
(ERROR). (F9)

Opt-in audit log: zettel.audit.audit() writes one JSON line per MCP tool
call and per answer_question when $ZETTEL_AUDIT_LOG is set. (F10)

Sync purges the index for a repo whose pull now fails with a real
remote's authz rejection, and for repos removed from the registry;
SyncReport.purged / the CLI surface it. The local clone is left on disk
by design. Sidecar chunk offsets are clamped to the body length. (F11, F5)

MCP DNS-rebinding guard: mcp.transport_security.make_transport_security
enables the SDK's Host-header check — derived allowlist for a loopback
bind, --allowed-host for a non-loopback one. (F6 sub-issue)

667 tests pass (was 614). SECURITY-REVIEW-2026-09.md and
docs/extensions/security-and-deployment.md track fixed vs
deployment-posture findings.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RCtojKuWSSfRj3W4Zh291R
get_note() went through resolution.cross_repo.resolve -> NoteRepository.load
-> discover(), which globs, reads and parses EVERY file in the checkout to
find one note — plus a `git log -1` subprocess per file when a frontmatter
adapter is configured. Measured against clones/owasp-cheatsheets (117
notes): 1859 ms per call.

It is the universal accessor, so the cost multiplied: `zettel search` ships
with --snippet on by default and calls search_snippet() per hit (20 hits =
37 s), backlinks() calls it per source, follow_links() per link,
answer_question() per retrieved passage, and the TUI's preview-follows-
cursor calls it on every arrow key.

IndexStore already records each note's path, so the common case needs no
scan: _note_from_index() reads that one file, _resolve_from_disk() keeps
the authoritative scan as the fallback. note_path() gets the same split.

The index is a derivation and can be stale, so _note_from_index re-checks
every property the scan would have guaranteed: the checkout still exists
(the ACL check — a revoked repo has no checkout, so a stale row serves no
content), the recorded path is still confined to it, the file still parses
through the same adapter-aware parse_note_text, and the parsed note still
carries the requested id (a file re-identified since the last sync must not
be returned under the old one). Any failure means "the index is behind",
so it falls back rather than answering.

Measured after: 17 ms per get_note (109x), 20-hit search 37.2 s -> 0.34 s.

tests/integration/test_lookup_scaling.py asserts a bound on WORK rather
than on time — it counts discover() calls, which is machine-independent
and is exactly the metric that regressed. The rest of the suite cannot see
this class of bug: every other fixture holds a handful of notes, where a
full scan is indistinguishable from a single read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
create_note() checked the registry entry and the write mode, then resolved
a checkout path and wrote — without checking that the checkout exists.
NoteRepository.write_new does mkdir(parents=True), so `zettel new` against a
never-synced repo created a plain directory that is not a git repository,
wrote the note into it, and reported success.

The damage compounds: the next sync sees checkout_path.exists() is now
true, so _clone_or_pull takes the *pull* branch, git fails in a non-repo,
and the shortname reports `inaccessible` on every subsequent sync —
permanently, until someone removes the directory by hand. The note is
outside version control, never indexed, and never mentioned again.

Refusing up front (RepoNotCheckedOutError) is the only outcome that does
not silently lose the note. The guard tests `<checkout>/.git` with
.exists() rather than .is_dir(), because a linked worktree stores .git as
a file and writing a note into a worktree is legitimate.

note_path() already took this posture ("the repo has no local checkout
yet" is one of its documented None cases); the two are now consistent.

Wired into both surfaces that create notes — `zettel new` and the TUI's
new-note modal — alongside the NotWritableRepoError they already handle.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
AGENTS.md states it as a rule: "Constructing a NoteRepository: always via
repository.note_repository.build_for_entry. Never construct it directly."
The factory exists because forgetting to wire frontmatter_adapter_config
caused the same bug twice (sync_service, then resolution/cross_repo), each
time making colocated/adopted repos index correctly but fail to resolve;
resolve() was given a required db_path for the same reason, so that no
easier-to-forget construction route was left.

create_note() was the one caller still building it by hand.

Behaviour is identical — write_new only uses checkout_path and the globs,
and adopted repos are refused earlier, so the fields the factory adds are
unused on this path. The point is that it is the third instance of the
pattern the factory was built to eliminate, in the method that decides
where a new note's file lands; a future change to write-side behaviour
should not be able to reintroduce the bug a fourth time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
_rebuild_backlinks() re-reads and re-parses every note of every accessible
repo, then replaces the backlinks table wholesale. It has to work that way
— an edge can cross repos — but it ran at the end of EVERY sync, including
one where every repo reported up_to_date and nothing else was read. A repo
that was reindexed got parsed twice in one pass: once in _reindex_view,
once here. ApplicationServices.reindex_repository (one repo, force=True)
paid it for the whole mesh.

The table is a pure derivation of the notes on disk, so "nothing was
reindexed and nothing was purged" implies "no edge moved" — SyncReport
already carries exactly that. A repo that changed upstream but was not
pulled this pass is not a counter-example: its notes on disk did not
change either.

One case still needs a rebuild when nothing changed: rebuild() DROPs and
recreates the table, which is how an index written by an older zettel (no
link_type column) migrates. That healing used to be free, because the
rebuild was unconditional. Verified that without a guard such a database
stays broken forever — all_edges() raises "no such column: link_type" —
so BacklinkStore.schema_is_current() is checked as the second condition.

Measured on the example workspace: ~2.5 s of re-parsing removed from every
no-op sync (~14 s cold).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
Both handlers called services.sync_repositories() synchronously on the LSP
message loop: a clone/pull against every registered remote, a reindex of
anything that moved, and a full backlink rebuild — all before the first
diagnostic could be published, with no other thread to fall back on. An
unreachable host, or an ssh credential prompt with no tty to answer it,
blocked the editor indefinitely.

Index-backed features (completion, go-to-definition, references) now read
whatever the last `zettel sync` produced. The module already accepted that
completions may be briefly stale; this makes the staleness last until the
user syncs, which is the right trade for never hanging the editor. Syncing
is an explicit user action — `zettel sync`, or the TUI's `s` — both of
which can report progress and failure, which a language server cannot.

Buffer-backed diagnostics are unaffected: they parse the editor's live
buffer and never needed the sync.

The module docstring documented the old behaviour as a resolved design
question, so it is revised rather than silently contradicted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
subprocess.run was called with no timeout anywhere in the adapter, so a
hung transport — an unreachable host, or an ssh credential prompt with no
tty to answer it — blocked the calling process forever with no signal.
`zettel sync` would hang rather than reporting the repo inaccessible, and
the LSP inherited the same stall on every file open.

A timeout now surfaces as an ordinary GitError, which is the outcome
callers already handle for an unreachable remote. It carries no stderr,
deliberately: sync_service._looks_like_access_denied pattern-matches on
that text to decide whether to PURGE a repo's index rows, and a timeout is
not an ACL rejection — a timed-out repo must keep its rows.

Default 600 s: generous enough for a first clone of a large repo on a slow
link, and local operations (rev-parse, log, config) finish in
milliseconds, so it only ever bites on the network ones. $ZETTEL_GIT_TIMEOUT
overrides it; 0 disables the bound for a deliberate long-running clone, and
an unparseable value falls back to the default rather than breaking every
git call.

Applies to the two sites that bypass _run as well (check_ignore,
is_partial_clone — they handle exit 1 as a normal outcome), so the
"every invocation is bounded" claim holds for the whole adapter.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RW72LM22NGTeioFwzFud8X
hans merged commit 5c6ab55890 into main 2026-09-05 16:26:59 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
hans/zettelmesh!2
No description provided.