fix: validate the .git marker, and stop serving stale traversal caches - #17
Merged
Merged
Conversation
…traversals Two regressions from #16, found by an independent audit and reproduced against the 2.28.1 bundle before fixing. walk: the nested-repository boundary triggered on the NAME `.git`, so a file named `.git` holding anything else — a truncated write, an unrelated file — silently dropped its entire subtree from the index. Git's own rule is now applied: a DIRECTORY is a git dir, a FILE marks a repo only when it reads "gitdir: <path>" (git rejects the rest with "fatal: invalid gitfile format"). A symlinked `.git` resolves through its target. Valid markers keep behaving exactly as before, and the marker itself is still never indexed as source. traverse: the adjacency cache keyed on the edge array's identity and length, so retargeting an edge IN PLACE — same array, same length — kept answering with the pre-edit graph. A snapshot of the fields a traversal reads is now taken when a view is built and checked on every hit; values are copied by reference, so a check is a few pointer comparisons per edge. That check is not free, and measuring it changed the design: reverseClosure is back to its pre-PR shape, uncached and sorting only the buckets the walk visits — an impact closure reaches a handful of nodes, so both eager sorting and cache validation cost more than the walk (200 impactOf calls over a 9 153-edge graph: 78 ms eager, 70 ms now, 69 ms pre-PR). bfs keeps its checked cache, where two maps, two sorts and a degree distribution per call still pay for it: 161 ms pre-PR, 41 ms now. 19 075 impactOf/neighborsOf answers match the 2.28.1 bundle on a real repo, and another 19 075 match after mutating every 7th edge in place — the case that used to go stale. graph.json/symbols.json unchanged. Claude-Session: https://claude.ai/code/session_01YDMVcJNo7WZh9xFAGWs2F8
Follow-up to the previous commit, from a second review. The gitfile regex accepted `gitdir:` with no space and on any line of the file; real git (read_gitfile_gently) compares the first 8 bytes against exactly `gitdir: ` and rejects the rest as "invalid gitfile format" — verified against git itself for no-space, leading-whitespace and second-line variants. Parsing is now that prefix test plus a trailing whitespace trim, which is both stricter and simpler than the regex. The read is bounded: a 4 KiB cap means a stray archive that merely carries the name `.git` is no longer read whole (200 MB cost 156 ms) just to discover it is not a gitfile. A `.git` directory is still decided on its dirent alone, with no syscall. What git ALSO does and this walk deliberately does not: validate that the target looks like a repository. A stale gitfile from a pruned or moved worktree still sits on a full checkout, so it stays a boundary — now stated as the deviation it is, where the comment previously claimed to follow git's rule outright. Tests: the git-rejected spellings, a gitfile with no trailing newline (git accepts it), a symlinked `.git` resolving through a directory and through a gitfile, a dangling `.git` symlink keeping its subtree, the size cap, and an edge REPLACED in the array rather than edited in place. Claude-Session: https://claude.ai/code/session_01YDMVcJNo7WZh9xFAGWs2F8
Contributor
|
🎉 This PR is included in version 2.28.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two regressions that #16 introduced, found by an independent audit (Codex CLI) and confirmed by a second review (Fable) that also tightened the first fix. Both were reproduced against the published 2.28.1 bundle before being fixed.
1.
walk: the repository boundary triggered on the name.gitA file named
.githolding anything other than agitdir:line — a truncated write, an unrelated file carrying the name, a dangling symlink — made the walker skip its entire subtree, silently. That is precisely the failure the walk's "never a silent truncation" doctrine forbids.Now a
.gitdirectory is a git dir, and a.gitfile is a marker only when it opens with exactlygitdir:. That prefix is git's own rule (read_gitfile_gentlycompares the first 8 bytes), verified against real git:.gitfile bodygitdir: <path>gitdir: <path>(no trailing newline)gitdir:<path>(no space)gitdir: <path>(leading space)junk\ngitdir: <path>(2nd line)A symlinked
.gitresolves through its target (git supports it); a dangling one is not a repository. The read is capped at 4 KiB, so a stray 200 MB file carrying the name is no longer read whole (156 ms) just to be rejected. A.gitdirectory is still decided on its dirent alone, with no syscall.Deliberate deviation, now documented as one: git additionally requires the target to look like a repository and reports "not a git repository" otherwise. This walk stops at a well-formed marker whatever its target — a stale gitfile from a pruned or moved worktree still sits on a full checkout, and indexing it would duplicate the parent's sources, which is what the boundary exists to prevent. The previous comment claimed to follow git's rule outright; it now states the difference.
2.
traverse: the adjacency cache served stale answersThe cache was keyed on the edge array's identity and length. Retargeting an edge in place — same array, same length — left both unchanged, so
impactOf/neighborsOfkept answering with the pre-edit graph.Graph.fileEdgesis public and mutable, and the engine is vendored by consumers, so a cache that silently answers on the old graph is the wrong default.A snapshot of exactly the fields a traversal reads (
from,to,kind,weight,dangling,confidence) is taken when a view is built and checked on every hit; values are copied by reference, so a check is a few pointer comparisons per edge. It also catches an edge object replaced in the array, which the old check missed too.Measuring the check changed the design. It is not free, so
reverseClosureis back to its pre-PR shape — uncached, and sorting only the buckets the walk visits. An impact closure reaches a handful of nodes, so both eager sorting and cache validation cost more than the walk itself.bfskeeps the checked cache, where two maps, two sorts and a degree distribution per call still pay for it.impactOfneighborsOfVerification
pnpm typecheck,pnpm test(1 290 passed, 50 skipped),pnpm run check:buildall pass; the committed bundle matches a fresh rebuild.graph.json/symbols.jsonbyte-identical to perf: walker boundaries, batched worker dispatch, hot-path memos (+ deadcode fix) #16's output ontests/fixtures/mini-repoand on a real 2 924-file repo (still 2 924 files).impactOf/neighborsOfanswers match the 2.28.1 bundle on that repo — and another 19 075 match after retargeting every 7th edge in place, the case that used to go stale.git rev-parserather than assumed, for all eight spellings above.Audit notes
Codex additionally stress-tested #16's worker dispatcher — 280 synthetic runs over 1–513 jobs and 2–64 workers, plus twelve 2 358-job real-repo runs, every record accounted for, fault injection falling back correctly — and found no defect there. The dead-code fix from #16 was confirmed on the real repo: 170 removals, all verified to be genuinely called, no additions.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YDMVcJNo7WZh9xFAGWs2F8