Skip to content

fix(devkit): scrub git repo-location env vars in spawned processes - #400

Closed
crs48 wants to merge 1 commit into
mainfrom
claude/vigilant-goldstine-346b8f
Closed

crs48 wants to merge 1 commit into
mainfrom
claude/vigilant-goldstine-346b8f

Conversation

@crs48

@crs48 crs48 commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Problem

The .husky/pre-commit final step pnpm vitest run --changed HEAD --passWithNoTests failed falsely whenever the changed-file set pulled in packages/devkit tests (bridge.test.ts, dev-loop.test.ts, git.test.ts). Those tests create real temp git repos and run git worktree add, which died with:

fatal: .git/index: index file open failed: Not a directory

git commit exports GIT_INDEX_FILE (and related repo-location vars) into the hook environment; NodeCommandRunner spread process.env into every child, so the tests' spawned git processes were silently redirected at the outer repo instead of their temp-repo cwd. The same tests pass outside the hook.

Fix

  • packages/devkit/src/command-runner.ts (the robust fix): NodeCommandRunner drops the git repo-location vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_COMMON_DIR, GIT_PREFIX) from the inherited env before spawning. An explicit value via options.env still wins. This covers both the Git class and the tests' direct runner.run('git', …) setup calls, and protects any devkit consumer that runs inside a hook.
  • .husky/pre-commit: the vitest step runs under env -u GIT_INDEX_FILE -u GIT_DIR -u GIT_WORK_TREE as defense for any other suite that spawns git.
  • New regression test in command-runner.test.ts pinning the scrub + override-wins behavior.
  • Changeset: @xnetjs/devkit patch.

Verification

  • Reproduced first: with GIT_INDEX_FILE/GIT_DIR pointed at the repo, all 5 git.test.ts tests failed with the reported error; after the fix, the full devkit suite (54 tests) passes under the same polluted env.
  • End-to-end: the commit on this branch ran the pre-commit hook itself with devkit in the changed set — vitest ran git.test.ts (incl. worktree add) inside the real hook environment and passed.
  • Full suite green on push (938 files / 10,212 tests via the pre-push hook).

🤖 Generated with Claude Code

Running devkit (or its tests) inside a git hook broke every spawned git:
'git commit' exports GIT_INDEX_FILE into the hook environment, so temp-repo
tests and 'git worktree add' were silently redirected at the hook's repo and
died with '.git/index: index file open failed: Not a directory'.

NodeCommandRunner now drops GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE and friends
from the inherited env (explicit options.env still wins), and the pre-commit
hook scrubs them for the vitest step as defense for other suites.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

This PR needs a changelog entry.

Add a fragment file to the PR (it lands with the merge — nothing is
written to main separately, and the PR number is filled in at deploy):

node scripts/changelog/new.mjs --title "Short, benefit-first headline" \
  --summary "One sentence on what the user can now do." \
  --tags app,ai --highlight "A specific user-visible point"

No user-facing change? Add the skip-changelog label instead.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Preview: https://xnet.fyi/pr/400/app/

github-actions Bot added a commit that referenced this pull request Jul 6, 2026
@crs48

crs48 commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Closing as superseded — and flagging that merging this branch would regress main.

The fix landed independently in packages/devkit/src/command-runner.ts on main, in a strictly better form:

  • 8 scrubbed vars (this branch has 7 — it is missing GIT_NAMESPACE).
  • Scrubbed only for command === 'git', rather than for every spawned command.
  • options.env is spread last, so an explicit override still wins.

The hazard is that this branch predates exploration 0391. Its command-runner.ts contains no NodeLineRunner, LineRunner, StreamRunOptions, or FakeLineRunner — the streaming agent seam that Claude Code's --output-format stream-json path depends on:

$ git diff origin/main pr400 -- packages/devkit/src/command-runner.ts
# ~150 deleted lines, covering the whole 0391 streaming seam

GitHub reports this PR as CONFLICTING, which is the only reason that has not already happened. Do not reopen and resolve conflicts in favour of this branch — it would silently delete the seam.

Rationale and full triage: docs/explorations/0410_[_]_OPEN_PR_TRIAGE_AND_THE_STRANDED_BRANCH_PROBLEM.md.

@crs48 crs48 closed this Jul 30, 2026
crs48 added a commit that referenced this pull request Jul 30, 2026
…(0410) (#658)

Implements exploration
`0410_[_]_OPEN_PR_TRIAGE_AND_THE_STRANDED_BRANCH_PROBLEM.md`, written in
the previous turn.

## What this closes out

Eight PRs were open, the oldest from March. Checking each one's **actual
diff against `main`** (rather than its description) found that four
described work already in `main`, re-derived and landed weeks later by
another route.

| PR | Age | Action taken |
| --- | --- | --- |
| #400 | 24d | **Closed** — fix landed on main in a better form; branch
predates 0391 and would have deleted `NodeLineRunner` |
| #9 | 145d | **Closed** — draft;
`apps/electron/src/renderer/workspace/` no longer exists |
| #595 | 11d | **Closed** — byte-identical to main (`fbc2965a2`) |
| #463 | 20d | **Closed** — main has a longer, `[x]` version of 0298 |
| #13 | 141d | **Closed** — both fixes on main verbatim |
| #505 | 17d | **Merged** — exploration 0318 + scale bench |
| #389 | 25d | **Merged** — exploration 0270 |
| #449 | 20d | **Rebuilt here** — see below |

`#400` was the load-bearing one: `git diff origin/main pr400 --
packages/devkit/src/command-runner.ts` showed ~150 lines of *deletion*
covering the whole 0391 streaming agent seam. Only its `CONFLICTING`
state had prevented that.

`#505`'s `build-and-smoke-test` went green on a rebase, confirming it
was branch staleness rather than the diff.

## The #449 rebuild (0290 bugs #2 and #3)

The original patched `apps/web/src/workbench/SyncStatus.tsx`, which 0406
deleted; the component now lives in `packages/workbench`, so the rebuild
lands on **desktop and web at once**.

- `WorkbenchHost` gains a `hub` capability (`configuredUrl` / `connect`)
so the shell can offer a way out of the disconnected state without
importing either app's `hub-url` module. `connect` returns a reason
string on rejection — never a silent no-op.
- `SyncStatus` exports `openSyncStatusPanel()`; the desktop popover and
mobile sheet both listen. The panel shows an inline hub-URL form while
there is no hub.
- `ShareDialog`'s `!ready` branch explains why a hub is needed and
offers **Connect a hub…**; private-hub links now confirm before copying.
- Adds `normalizeHubUrl` to the electron renderer's `hub-url` mirror (+4
unit tests), so a malformed URL is rejected loudly rather than
persisted.

Verified by driving the real app against a local hub — the CTA closes
the dialog and opens the panel; an invalid URL shows an error and
persists nothing; `https://hub.xnet.fyi/` normalises to
`wss://hub.xnet.fyi`; a `localhost` link's first Copy click asks "Copy
local-only link?" and writes nothing, the second copies. No console
errors. 0290 is now `[-]` at 11/20.

## Stopping the recurrence

Time-to-merge for the last 20 merged PRs was **0 hours median, 1 hour
max** — there is no review queue, so a PR that outlives its session is
abandoned, not pending.

- `.github/workflows/stale.yml` — warns at 14d, closes at 21d,
`keep-open` exempts. Named consumer (the maintainer, weekly) and a
decidable pass condition, per AGENTS.md.
- Fixed the `/explore` next-number command, which read only the working
tree and so handed out numbers already claimed on branches. Seven
explorations were found stranded that way.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

This branch was successfully deployed

1 active deployment
pr-400 — 05b78809 Deployed Jul 6, 2026 by crs48 via deploy-preview #696
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