fix(gitignore): don't append /graft/ when git already ignores the dir (#582)

[djbclark]

Fixes #582.

The bug

ensureGitignored decides whether the graph dir is ignored by reading the root .gitignore text. A rule in .git/info/exclude, a parent .gitignore or core.excludesFile is invisible to it, so every build re-appends

# graft's local graph cache — regenerable, not committed (run `graft build`).
/graft/

The checkout is dirty again after each refresh, in the repos where the owner deliberately keeps tooling noise out of the tracked .gitignore (an upstream fork is the typical case: the change would ride into a PR branch). GRAFT_NO_GITIGNORE works around it, but it has to be set on every launcher (MCP server env, each hook, interactive shells; see #514).

Repro (main, git 2.x)

$ git init demo && cd demo
$ printf '/graft/\n' >> .git/info/exclude
$ git check-ignore -v graft/INDEX.md        # .git/info/exclude:1:/graft/  graft/INDEX.md
$ graft build .
$ git status --short                        # ?? .gitignore  (or M .gitignore)

The fix

Before appending, ask git: git check-ignore -q -- <dir>/. Exit 0 means git already ignores the dir, so ensureGitignored returns without writing. Any other outcome (not a work tree, git not installed, 5 s timeout) falls through to the existing behaviour, so non-git directories and the existing tests are unaffected. The trailing slash makes git treat the path as a directory, so dir-only rules such as graft/ match.

The guard sits after the existing "entry already present" check and before the write. GRAFT_NO_GITIGNORE and the outside-the-root guard are unchanged.

Two commits:

1. fix(gitignore): don't append /graft/ when git already ignores the dir (#582): the guard and its tests.
2. fix(gitignore): harden the check-ignore guard (review follow-ups): the git call runs without GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_COMMON_DIR, GIT_OBJECT_DIRECTORY and GIT_NAMESPACE, with -c core.fsmonitor=false; the ensureGitignored tests no longer depend on the developer's environment; one comment corrected. See "Security review" below.

Not in this PR

ensureBuildConfigIgnored (/.graft/, #401) has the same blind spot and can reuse the helper in a follow-up.

Merge note: #519

This PR and #519 (legacy-entry migration) both edit the tail of ensureGitignored, so whichever lands second gets a mechanical conflict in src/context/node-file.ts. Resolution: keep #519's migration block and its if (lines.some(entry)) return;, then put if (gitAlreadyIgnores(root, bare)) return; just before const gap. The guard is placed after the presence check so a legacy-migration rewrite still runs first.

Design note for review

The guard applies to every caller of ensureGitignored, including one passing LINK_NOTE (the .graft/ read-token dir). No caller in src does that today (LINK_NOTE has had no caller since b92c22e). If one is added back, it should skip the guard and always write the anchored entry: a rule that exists only in a user-level file or .git/info/exclude protects that clone and that account only, so another account, a container with a different HOME, or CI working in the same checkout would not see it, and a read token could be committed. For the regenerable graph cache the guard is the right behaviour; for a token dir it is not. This PR keeps a single behaviour because no token-dir caller exists, and says so here for the maintainers to decide.

Tests

test/context.test.ts, five new cases, each in a throwaway git init repo:

1. .git/info/exclude ignores /graft/: an existing .gitignore is untouched, and none is created when absent.
2. core.excludesFile ignores graft/: no .gitignore is written.
3. A parent .gitignore ignores graft/ and the graph root is a subdirectory: nothing written.
4. A git repo with an unrelated exclude rule: /graft/ is still appended.
5. An inherited GIT_DIR pointing at another repo that ignores /graft/ does not stop the entry being written (added with the follow-up commit).

The file's tests now run with an empty global git config, no default excludes file, GIT_CONFIG_NOSYSTEM=1, GIT_CEILING_DIRECTORIES set above the temp root, and GRAFT_NO_GITIGNORE/GRAFT_NO_IGNORE/GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE unset, restored after each test.

node --import tsx --test test/context.test.ts: 38 pass, both with GRAFT_NO_GITIGNORE=1 exported and with it unset, and with a hostile environment (global core.excludesFile ignoring graft/, TMPDIR inside an ignoring repo, GIT_DIR exported). npx tsc -p tsconfig.json --noEmit is clean. With the source change reverted, tests 1 to 3 fail; with only the follow-up's source change reverted, test 5 fails.

Security review

Reviewed by a separate security-review agent before opening this PR. Verdict line, verbatim:

VERDICT: findings (4)
Recommendation: APPROVE. All four findings are Low or Info and none blocks the PR. The commit adds no injection path and no new attack surface. It fails safe: when git can't answer, graft writes the entry as before.

#	Severity	Finding	Addressed
F1	Low (tests)	The new and existing tests depended on the developer's git and graft environment (global excludes, TMPDIR inside a repo, GRAFT_NO_GITIGNORE).	Fixed in the follow-up commit: per-test environment isolation (empty global config, no default excludes file, GIT_CONFIG_NOSYSTEM, GIT_CEILING_DIRECTORIES, GRAFT_NO_* unset), restored afterwards.
F2	Low	An inherited GIT_DIR, GIT_WORK_TREE or GIT_INDEX_FILE (for example from a git hook) made check-ignore answer for another repo, so the entry could be skipped.	Fixed: those variables plus GIT_COMMON_DIR, GIT_OBJECT_DIRECTORY and GIT_NAMESPACE are removed from the child's environment; new regression test.
F3	Low (hardening)	git check-ignore runs the repo's core.fsmonitor command.	Fixed: the call passes -c core.fsmonitor=false. The same exposure in the ingest git ls-files calls (src/ingest/fs.ts) is not new and is left for a separate PR.
F4	Info	The helper's comment overstated what the trailing slash does (negation rules can judge a not-yet-created path differently), and a token dir would need to bypass the guard.	Comment corrected (callers create the dir first, as both do today); the token-dir case is in the design note above.

Accepted without change, as the review recommended: when root is not a repo but sits inside an outer repo that ignores it, the write is skipped until the next build after a git init, which then self-corrects. Not covered: Windows (bare git resolution against cwd, the same exposure src/ingest/fs.ts already has).

🤖 Generated with Claude Code

[trailhq-graft]

🌱 graft blast radius

1 area changed → 3 areas can be affected. 6 dependent symbols, depth 2.
Tests: 1 area updated its tests.
Tag: @anirudhkumar-nanonets — 4 of 4 areas · @shhdwi — Graph Building, CLI Entry Point · @Frankie-Xu — Gitignore Management

flowchart TB
  A0(("Graph Building<br/>3 symbols"))
  A1(("CLI Entry Point<br/>2 symbols"))
  A2(("Pull Request Review<br/>1 symbol"))
  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:L204-L472 buildGraph — calls, depth 1	Gitignore Management
CLI Entry Point	2	src/cli.ts:L1-L2368 cli.ts — calls, depth 2	Gitignore Management
Pull Request Review	1	src/app/review.ts:L45-L99 reviewPullRequest — calls, depth 2	Gitignore Management

Who knows this code — 3 people across 4 areas

Area	Who knows it
Gitignore Management · changed	@anirudhkumar-nanonets — 10 commits, last 2d ago · @Frankie-Xu — 3 commits, last 2mo ago
Graph Building · affected	@anirudhkumar-nanonets — 20 commits, last 2d ago · @shhdwi — 9 commits, last yesterday
CLI Entry Point · affected	@anirudhkumar-nanonets — 58 commits, last today · @shhdwi — 26 commits, last yesterday
Pull Request Review · affected	@anirudhkumar-nanonets — 3 commits, last 1mo 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 6 dependent symbols, grouped by area

Graph Building — 3 symbols in 3 files

• src/graph/build.ts:L204-L472 — buildGraph (calls, depth 1)
  430: // (`ensureGitignored` writes `.gitignore`, which a read has no business doing),
• src/graph/workspace-cli.ts:L52-L100 — runWorkspaceBuild (calls, depth 1)
  92: ensureGitignored(root, contextDirFor(root, opts.override));
• src/graph/refresh.ts:L151-L228 — ensureFreshGraph (calls, depth 2)

CLI Entry Point — 2 symbols in 2 files

• src/cli.ts:L1-L2368 — cli.ts (calls, depth 2)
  31: import { contextDirFor } from "./context/node-file.js";
• src/engine.ts:L92-L102 — graph (calls, depth 2)

Pull Request Review — 1 symbol in 1 file

• 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.

• ✓ Gitignore Management — 1 of 2 reached · 1 test file changed here: test/context.test.ts
  • not reached: gitAlreadyIgnores

33 test suites also reference this code

35 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-native-fallback.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
• …13 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.