Skip to content

fix(workflows): sanitize release-docs git subprocess env - #1351

Merged
lavaman131 merged 2 commits into
mainfrom
fix/release-docs-git-env
Jun 12, 2026
Merged

fix(workflows): sanitize release-docs git subprocess env#1351
lavaman131 merged 2 commits into
mainfrom
fix/release-docs-git-env

Conversation

@lavaman131

@lavaman131 lavaman131 commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Strips repository-local Git environment variables (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, and others) from subprocesses in the release-docs workflow and test fixtures, preventing Git hook runners (e.g. prek) from silently redirecting commands at the wrong repository.

Root Cause

Git honors repository-local env vars over cwd — hook runners like prek export these vars into the hook process environment, so any transitively spawned Git command (including git init on a temp directory) operates on the hook-invoking repo instead of the intended one. In the worst case, git init persists core.worktree into the shared .git/config of the real repo, corrupting it.

Changes

  • .atomic/workflows/lib/release-docs.ts: Pass env: createGitEnvironment() to execFileSync inside runCommand(), stripping all repository-local Git env vars so every Git subprocess always targets its explicit cwd.
  • test/unit/release-docs-workflow.test.ts:
    • Apply the same createGitEnvironment() sanitization to runGit() in test fixtures, preventing fixture-repo git init calls from corrupting the real repo under hook runners.
    • Extract a commitAll() helper to deduplicate repeated commit invocations across tests.
    • Add a regression test that spawns a child Bun process with ambient hook-style env vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE pointing at a detached decoy repo) and asserts currentBranchName() still returns the correct branch of the target repo.

Validation

  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Pre-commit and pre-push hooks passed during commit/push

@claude claude Bot changed the title fix(workflows): sanitize release docs git subprocess env fix(workflows): sanitize release-docs git subprocess env Jun 12, 2026
@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review: fix(workflows): sanitize release docs git subprocess env

Nice, tightly-scoped fix. Routing all subprocesses through the single runCommand chokepoint with createGitEnvironment() is the right call — it covers every git/gh/bun invocation in the file in one place, and stripping the repo-local Git vars from the non-git commands (gh, bun, bunx) is harmless. The explanatory comments are genuinely useful, and the regression test is well-constructed: it injects GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE at child-process startup (the realistic hook-runner scenario), and would actually fail without the sanitizer (the detached decoy repo makes currentBranchName() throw), so it exercises the fix rather than just asserting happy-path behavior. The commitAll extraction is a clean dedup too. ✅

Suggestions (non-blocking)

  1. Prefer the public barrel over a deep src/ import. release-docs.ts reaches into package internals:
    ```ts
    import { createGitEnvironment } from "../../../packages/coding-agent/src/utils/git-env.js";
    ```
    `createGitEnvironment` is a public export of `@bastani/atomic` (`packages/coding-agent/src/index.ts:481`), and the sibling `.atomic/workflows/release-docs.ts` already imports via a package name (`@bastani/workflows`). `packages/workflows/src/runs/shared/worktree.ts` likewise imports it from `@bastani/atomic`. Importing from `@bastani/atomic` here would match the established convention in this directory and avoid coupling to the fork's internal file layout (a `src/utils/` move would silently break this brittle `../../../` path). The test file has the same deep import.

  2. Minor — the regression test relies on `process.execPath` running a `.ts` file. Correct under Bun (and the repo mandates Bun), but it implicitly assumes `process.execPath` is the Bun binary. A one-line comment noting the child is intentionally launched with the Bun runtime would help future readers who might assume Node.

Things I verified

  • `runCommand` is the only subprocess call site in `.atomic/workflows/`, so the sanitization coverage is complete.
  • `createGitEnvironment()` preserves the rest of `process.env` (it only deletes the `GIT_LOCAL_ENV_VARS` list), so `gh` auth, `PATH`, etc. are untouched — no functional regression for the `gh`/`bun` commands.
  • The `.js` import extension follows the repo's TS-ESM convention (CLAUDE.md), correct.

No correctness or security concerns — this is a net security/robustness improvement. LGTM modulo the import-source nit.

🤖 Generated with Claude Code

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — fix(workflows): sanitize release-docs git subprocess env

Solid, well-scoped fix with a clear root-cause writeup. The diagnosis (hook runners exporting GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE, which Git honors over cwd and git -C) is correct, and reusing the already-established createGitEnvironment() helper keeps this consistent with the existing pattern in packages/{workflows,subagents}/src/runs/shared/worktree.ts. 👍

What's good

  • Single chokepoint: runCommand() is the only subprocess entry point in release-docs.ts, so wrapping its env covers every git/gh invocation transitively (runGit, runCommandResult, currentBranchName, etc.). No call sites missed.
  • Genuine regression test: The new test actually guards the fix — with the sanitization removed, the child process would resolve the detached decoy repo (via ambient GIT_DIR), git branch --show-current returns empty, currentBranchName() falls through to the detached-HEAD throw, and the assertion fails. So it's not a tautology.
  • Helpful comments anchoring the "why" to git-env.ts, and the commitAll() extraction is a nice dedup.

Minor suggestions (non-blocking)

  1. Test path resolution via process.cwd() (release-docs-workflow.test.ts:97): join(process.cwd(), ".atomic/workflows/lib/release-docs.ts") assumes the runner's cwd is the repo root. That holds for bun run test:unit, but it's more brittle than the file's other imports which are relative. Consider deriving from import.meta.dir so the test is cwd-independent.
  2. Scratch script location: the generated .ts is written to tmpdir() directly rather than inside one of the mkdtempSync dirs. It's cleaned up in finally with force: true, so this is purely cosmetic — but nesting it under one of the temp repos (or its own mkdtempSync) would keep the cleanup surface tidy and avoid the manual ${pid}-${Date.now()} uniqueness dance.
  3. createGitEnvironment() is invoked per-call, re-cloning process.env on every runCommand. Negligible here (a handful of git calls), just noting it — no change needed.
  4. Scope note: the sanitized env is applied to non-git commands too (e.g. gh). This is harmless and arguably desirable, but the comment frames it purely around git — a half-sentence noting it's safe/intended for all subprocesses would prevent a future reader from "fixing" it.

Coverage / correctness

  • Typecheck/lint/unit validation noted in the PR description; the change is import + one env: option + test, all type-safe against the exported createGitEnvironment signature.
  • No security or performance concerns. If anything this reduces a footgun (stray core.worktree written into a shared .git/config).

LGTM pending the optional nits above.

Reviewed by Claude (Opus 4.8)

@lavaman131
lavaman131 merged commit 4f41b0d into main Jun 12, 2026
10 checks passed
@lavaman131
lavaman131 deleted the fix/release-docs-git-env branch June 12, 2026 19:22
@lavaman131

Copy link
Copy Markdown
Collaborator Author

Addressed the non-blocking feedback in aae1df73 fix(workflows): use public atomic git env export:

  • Switched both release-docs implementation and regression test imports to the public @bastani/atomic barrel for createGitEnvironment.
  • Added an explicit test comment noting process.execPath is intentionally the Bun runtime for the child TypeScript script.

The branch is pushed and clean.

lavaman131 added a commit that referenced this pull request Jun 29, 2026
* fix(workflows): sanitize release docs git subprocess env

Assistant-model: GPT-5.5

* fix(workflows): use public atomic git env export

Assistant-model: GPT-5.5
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