Skip to content

fix(ingest): treat a cross-drive --dir as outside the repo (#384) - #396

Open
Frankie-Xu wants to merge 2 commits into
trailhq:mainfrom
Frankie-Xu:fix/384-windows-cross-drive-ignore
Open

Frankie-Xu wants to merge 2 commits into
trailhq:mainfrom
Frankie-Xu:fix/384-windows-cross-drive-ignore

Conversation

@Frankie-Xu

Copy link
Copy Markdown
Contributor

Summary

  • ensureGitignored / ensureSearchable treated rel.startsWith(..) as --dir outside the repo. On Windows, path.win32.relative for a different drive returns an absolute path (F:/tmp), so the guard missed and /F:/graft-dir-repro-tmp/ was appended to the repo .gitignore / .ignore.
  • Treat an absolute rel (path.isAbsolute or path.win32.isAbsolute) as outside too. Same-drive in-repo --dir still writes /graft/.

Not in this PR

  • Does not change GRAFT_NO_GITIGNORE / GRAFT_NO_IGNORE.
  • Does not index a cross-drive directory.
  • Does not move graft/.cache/telemetry-repo-id.json (split out in the issue).

Closes #384

Test plan

  • root on this volume + F:/graft-dir-repro-tmp → neither ignore file contains F:
  • Same-drive <root>/graft still gets /graft/
  • node --import tsx --test --test-name-pattern ensureGitignored|ensureSearchable test/context.test.ts

Made with Cursor

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

🌱 graft blast radius

1 area changed → 3 areas can be affected. 7 dependent symbols, depth 2.
Tests: 1 area updated its tests.
Tag: @anirudhkumar-nanonets — 4 of 4 areas · @shhdwi — Graph Building, CLI Engine

flowchart TB
  A0(("Graph Building<br/>3 symbols"))
  A1(("CLI Engine<br/>2 symbols"))
  A2(("Pull Request Review<br/>2 symbols"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Graph Building 3 src/graph/build.ts:L151-L410 buildGraph — calls, depth 1 Node File Context
CLI Engine 2 src/cli.ts:L1-L1407 cli.ts — calls, depth 2 Node File Context
Pull Request Review 2 src/app/brain-build.ts:L251-L358 readRepository — calls, depth 2 Node File Context
Who knows this code — 2 people across 4 areas
Area Who knows it
Node File Context · changed @anirudhkumar-nanonets — 8 commits, last 1mo ago
Graph Building · affected @anirudhkumar-nanonets — 19 commits, last 26d ago · @shhdwi — 7 commits, last 1mo ago
CLI Engine · affected @anirudhkumar-nanonets — 41 commits, last 5d ago · @shhdwi — 24 commits, last 1mo ago
Pull Request Review · affected @anirudhkumar-nanonets — 9 commits, last 5d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 7 dependent symbols, grouped by area

Graph Building — 3 symbols in 3 files

  • src/graph/build.ts:L151-L410 — buildGraph (calls, depth 1)
    371: // (`ensureGitignored` writes `.gitignore`, which a read has no business doing),
  • src/graph/workspace-cli.ts:L48-L86 — runWorkspaceBuild (calls, depth 1)
    83: ensureGitignored(root, contextDirFor(root, opts.override));
  • src/graph/refresh.ts:L150-L227 — ensureFreshGraph (calls, depth 2)

CLI Engine — 2 symbols in 2 files

  • src/cli.ts:L1-L1407 — cli.ts (calls, depth 2)
    25: import { contextDirFor } from "./context/node-file.js";
  • src/engine.ts:L91-L101 — graph (calls, depth 2)

Pull Request Review — 2 symbols in 2 files

  • src/app/brain-build.ts:L251-L358 — readRepository (calls, depth 2)
  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 2)
Test signal per changed area — 1 ✓

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Node File Context — 2 of 2 reached · 1 test file changed here: test/context.test.ts
32 test suites also reference this code

34 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/ask-index.test.ts
  • test/ask.test.ts
  • test/container-extract.test.ts
  • test/context-only-dir.test.ts
  • test/covers.test.ts
  • test/generic-extract.test.ts
  • test/graph-go.test.ts
  • test/graph-incremental.test.ts
  • test/graph-invariants.test.ts
  • test/graph-java.test.ts
  • test/graph-languages.test.ts
  • test/graph-php.test.ts
  • test/graph-posix-paths.test.ts
  • test/graph-python.test.ts
  • test/graph-r-classes.test.ts
  • test/graph-r-phase3.test.ts
  • test/graph-r-phase4.test.ts
  • test/graph-r-phase5.test.ts
  • test/graph-r.test.ts
  • test/graph-references.test.ts
  • …12 more

graft blast · origin/main...HEAD · depth 2 · 2 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

@Frankie-Xu
Frankie-Xu marked this pull request as ready for review September 15, 2026 12:48
Windows checkout rewrote node-file.ts and its test as CRLF, inflating the PR. Behaviour is unchanged.
@Frankie-Xu

Copy link
Copy Markdown
Contributor Author

Follow-up: 919d38e stores node-file.ts and the test as LF to match main. The previous commit was CRLF-only noise. Behaviour is unchanged.

github-actions Bot added a commit that referenced this pull request Sep 15, 2026
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.

--dir on a different Windows drive defeats the outside-the-repo guard: absolute paths accumulate in .gitignore and .ignore

1 participant