diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 27e31eb36..894af0a26 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1787,6 +1787,13 @@ teamai init --repo https://github.com/yourorg/yourrepo --scope user --role ({ - default: () => mockGit, + default: (...args: unknown[]) => { + simpleGitCalls.push(args); + return mockGit; + }, })); vi.mock('fs-extra', () => ({ @@ -55,6 +64,79 @@ vi.mock('../utils/logger.js', () => ({ import { generateBranchName, pushRepoBranch, checkoutMaster, pushRepoDirectly, initRepo, configureGitUser, getHeadRev, resetToCleanMaster, isMetadataOnlyDiff, isGitRepo, normalizeRepoUrlForCompare, remotesMatch, redactGitCredentials, pullRepo, pullRepoFastForward, pushLearningToOrigin } from '../utils/git.js'; import fse from 'fs-extra'; +import { createGit, createGitForInitPush, disableGitTerminalPrompt } from '../utils/git.js'; + +describe('createGit', () => { + // The init-hang bug: a push with missing credentials opened an invisible + // prompt (no tty) and blocked forever. The credential-prompt guard is set + // process-wide by disableGitTerminalPrompt (tested separately), NOT per + // simple-git instance — simple-git's .env() replaces the whole child env, + // which would drop PATH/HOME and break git. The spawn-level block timeout + // is NOT global either — it lives in createGitForInitPush so it cannot kill + // slow clones/fetches elsewhere. + beforeEach(() => { + simpleGitCalls.length = 0; + }); + + it('does NOT add a global spawn timeout (would kill legitimate slow clones/fetches)', () => { + createGit('/some/path'); + const options = simpleGitCalls.at(-1)![0] as { timeout?: unknown }; + expect(options.timeout).toBeUndefined(); + }); + + it('forwards basePath as baseDir', () => { + createGit('/some/path'); + const options = simpleGitCalls.at(-1)![0] as { baseDir?: string }; + expect(options.baseDir).toBe('/some/path'); + }); +}); + +describe('createGitForInitPush', () => { + beforeEach(() => { + simpleGitCalls.length = 0; + }); + + it('adds the spawn-level block timeout so a hung init push subprocess is killed', () => { + createGitForInitPush('/some/path'); + const options = simpleGitCalls.at(-1)![0] as { timeout?: { block?: number } }; + // simple-git's timeout.block actually kills the spawned process; a + // Promise.race-style timeout would leave it running. + expect(options.timeout?.block).toBe(30_000); + }); + + it('honors TEAMAI_INIT_PUSH_TIMEOUT_MS for slow links', () => { + const prev = process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS; + process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS = '120000'; + try { + createGitForInitPush('/some/path'); + const options = simpleGitCalls.at(-1)![0] as { timeout?: { block?: number } }; + expect(options.timeout?.block).toBe(120_000); + } finally { + if (prev === undefined) delete process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS; + else process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS = prev; + } + }); +}); + +describe('disableGitTerminalPrompt', () => { + // The credential-prompt guard must be process-wide so every git subprocess + // inherits it — but it must not clobber an explicit user override. + afterEach(() => { + delete process.env.GIT_TERMINAL_PROMPT; + }); + + it('sets GIT_TERMINAL_PROMPT=0 process-wide so git fails fast on missing credentials', () => { + delete process.env.GIT_TERMINAL_PROMPT; + disableGitTerminalPrompt(); + expect(process.env.GIT_TERMINAL_PROMPT).toBe('0'); + }); + + it('is idempotent and preserves an explicit user override', () => { + process.env.GIT_TERMINAL_PROMPT = '1'; // user wants prompts + disableGitTerminalPrompt(); + expect(process.env.GIT_TERMINAL_PROMPT).toBe('1'); + }); +}); describe('generateBranchName', () => { it('should produce teamai/push// format', () => { diff --git a/src/__tests__/init-hang-regression.test.ts b/src/__tests__/init-hang-regression.test.ts new file mode 100644 index 000000000..a10415cc7 --- /dev/null +++ b/src/__tests__/init-hang-regression.test.ts @@ -0,0 +1,235 @@ +/** + * Real-git regression for the init-hang bug (PR #677). + * + * Original failure: with a protected default branch and missing push + * credentials, the member-registration / reviewer-config push hung forever + * (simple-git had no subprocess timeout and no GIT_TERMINAL_PROMPT guard), so + * `teamai init` never reached the local-config step. These tests pin both + * halves of the fix against a real git remote: + * + * 1. createGit must pass a spawn-level block timeout and + * GIT_TERMINAL_PROMPT=0 to every simple-git instance — the timeout kills + * the hung child process instead of merely un-awaiting it. + * 2. The guarded instance behaves normally on a plain file remote (a fast + * push with credentials present must still succeed), so the guards do + * not break the happy path. + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import http from 'node:http'; +import os from 'node:os'; +import path from 'node:path'; +import { simpleGit } from 'simple-git'; + +import { createGit, pushRepoDirectly } from '../utils/git.js'; + +let tmp: string; +let originalHome: string; + +beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-init-hang-')); + originalHome = process.env.HOME ?? ''; + process.env.HOME = path.join(tmp, 'home'); + fs.mkdirSync(process.env.HOME, { recursive: true }); +}); + +afterEach(() => { + process.env.HOME = originalHome; + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +/** A bare origin whose default branch rejects pushes via an update hook. */ +async function seedProtectedOrigin(): Promise { + const seed = path.join(tmp, 'seed'); + fs.mkdirSync(seed, { recursive: true }); + const seedGit = simpleGit(seed); + await seedGit.init(['--initial-branch=main']); + await seedGit.addConfig('user.email', 't@t.com'); + await seedGit.addConfig('user.name', 't'); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), 'team: acme\n'); + fs.mkdirSync(path.join(seed, 'skills'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'skills', '.gitkeep'), ''); + await seedGit.add(['.']); + await seedGit.commit('init knowledge'); + + const origin = path.join(tmp, 'origin.git'); + await simpleGit().clone(seed, origin, ['--bare']); + const hook = path.join(origin, 'hooks', 'update'); + fs.writeFileSync( + hook, + `#!/bin/sh +ref="$1" +if [ "$ref" = "refs/heads/main" ] || [ "$ref" = "refs/heads/master" ]; then + echo "default branch is protected" >&2 + exit 1 +fi +exit 0 +`, + ); + fs.chmodSync(hook, 0o755); + return origin; +} + +describe('createGit hang guards (init-hang regression)', () => { + it('returns a functional git instance (factory-level guards are pinned in git.test.ts)', () => { + const git = createGit(); + expect(typeof git.status).toBe('function'); + expect(typeof git.push).toBe('function'); + }); + + it('a push rejected by a protected default branch fails fast (real git, no hang)', async () => { + const origin = await seedProtectedOrigin(); + const clone = path.join(tmp, 'team-repo'); + await simpleGit().clone(origin, clone); + await simpleGit(clone).addConfig('user.email', 't@t.com'); + await simpleGit(clone).addConfig('user.name', 't'); + + // Push a real change at the protected branch. The server-side hook + // rejects it; the guarded instance must surface that rejection promptly + // (previously a missing credential made this await forever). + fs.mkdirSync(path.join(clone, 'members'), { recursive: true }); + fs.writeFileSync(path.join(clone, 'members', 'alice.yaml'), 'username: alice\n'); + + const start = Date.now(); + await expect( + pushRepoDirectly(clone, '[teamai] Register member: alice', ['members/alice.yaml'], { initPush: true }), + ).rejects.toThrow(); + const elapsed = Date.now() - start; + + // A server-side rejection is immediate; only a regression back to the + // unguarded hang would take the full 30s+ timeout budget. + expect(elapsed).toBeLessThan(15_000); + }); + + it('the guards do not break the happy path: a fast push to an unprotected branch still succeeds', async () => { + const seed = path.join(tmp, 'seed-ok'); + fs.mkdirSync(seed, { recursive: true }); + const seedGit = simpleGit(seed); + await seedGit.init(['--initial-branch=main']); + await seedGit.addConfig('user.email', 't@t.com'); + await seedGit.addConfig('user.name', 't'); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), 'team: acme\n'); + await seedGit.add(['.']); + await seedGit.commit('init'); + + const origin = path.join(tmp, 'origin-ok.git'); + await simpleGit().clone(seed, origin, ['--bare']); + + const clone = path.join(tmp, 'team-repo-ok'); + await simpleGit().clone(origin, clone); + await simpleGit(clone).addConfig('user.email', 't@t.com'); + await simpleGit(clone).addConfig('user.name', 't'); + + fs.mkdirSync(path.join(clone, 'members'), { recursive: true }); + fs.writeFileSync(path.join(clone, 'members', 'bob.yaml'), 'username: bob\n'); + + // No timeout, no throw — the guarded instance completes a normal push. + await expect( + pushRepoDirectly(clone, '[teamai] Register member: bob', ['members/bob.yaml'], { initPush: true }), + ).resolves.toBeUndefined(); + }); +}); + +describe('credential-prompt guard (init-hang regression)', () => { + // Reproduces the original bug precisely: a push to a remote that requires + // credentials, with NO credential helper available. Without + // GIT_TERMINAL_PROMPT=0, git opens an interactive username/password prompt + // on the tty — and since teamai runs git with no tty, the push hangs + // forever. With the guard, git fails immediately with "terminal prompts + // disabled" / "could not read Username". + // + // We stand up a local HTTP git endpoint that always answers 401 Unauthorized + // (demanding Basic auth), then push to it with every credential source + // stripped: empty HOME, no GIT_CONFIG_GLOBAL, no credential helper. The push + // must fail fast rather than block on a prompt. + let server: http.Server; + let remoteUrl: string; + + beforeEach(async () => { + server = http.createServer((_req, res) => { + // Always require authentication — git will look for a credential, find + // none, and (without the guard) try to prompt. + res.writeHead(401, { 'WWW-Authenticate': 'Basic realm="teamai-test"' }); + res.end('Unauthorized'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const addr = server.address(); + if (!addr || typeof addr === 'string') throw new Error('failed to bind test server'); + remoteUrl = `http://127.0.0.1:${addr.port}/team.git`; + }); + + afterEach(() => new Promise((resolve) => server.close(() => resolve()))); + + it('a credential-less push to a 401 HTTP remote fails fast instead of prompting', async () => { + const clone = path.join(tmp, 'credless-clone'); + fs.mkdirSync(clone, { recursive: true }); + const git = simpleGit(clone); + await git.init(['--initial-branch=main']); + await git.addConfig('user.email', 't@t.com'); + await git.addConfig('user.name', 't'); + await git.addRemote('origin', remoteUrl); + // An initial commit so `main` exists; pushRepoDirectly then stages and + // commits the member file itself (otherwise it sees nothing to commit and + // returns without pushing). + fs.writeFileSync(path.join(clone, 'README.md'), 'seed\n'); + await git.add(['README.md']); + await git.commit('seed'); + fs.mkdirSync(path.join(clone, 'members'), { recursive: true }); + fs.writeFileSync(path.join(clone, 'members', 'alice.yaml'), 'username: alice\n'); + + // Isolate credentials so git reaches the terminal-prompt path rather than + // a credential helper: point GIT_CONFIG_GLOBAL at a temp config that clears + // every helper (`[credential]\thelper =`), plus an empty HOME so no + // user-level helper is discovered. This is the state that made the old push + // hang — and the state GIT_TERMINAL_PROMPT=0 exists to rescue. + const prevHome = process.env.HOME; + const prevGlobal = process.env.GIT_CONFIG_GLOBAL; + const prevSystem = process.env.GIT_CONFIG_NOSYSTEM; + const prevPrompt = process.env.GIT_TERMINAL_PROMPT; + const isolatedConfig = path.join(tmp, 'no-helper.gitconfig'); + fs.writeFileSync( + isolatedConfig, + '[credential]\n\thelper =\n[user]\n\tname = t\n\temail = t@t.com\n', + ); + process.env.HOME = path.join(tmp, 'empty-home'); + fs.mkdirSync(process.env.HOME, { recursive: true }); + process.env.GIT_CONFIG_GLOBAL = isolatedConfig; + process.env.GIT_CONFIG_NOSYSTEM = '1'; + // The CLI sets this process-wide at startup (disableGitTerminalPrompt). + // This test imports pushRepoDirectly directly, so simulate that here. + process.env.GIT_TERMINAL_PROMPT = '0'; + // Shrink the init-push block timeout so a hung push is killed in a couple + // seconds instead of the default 30 (keeps the test fast). initPush:true + // routes through createGitForInitPush, whose timeout.block actually + // terminates the spawned git process — the real fix for a push that hangs + // on a credential prompt or helper. + const prevInitTimeout = process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS; + process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS = '3000'; + + const start = Date.now(); + try { + // initPush:true uses createGitForInitPush (spawn-level block timeout), + // mirroring how init actually calls this. The push must reject within + // the timeout budget — a regression to no spawn timeout would hang + // forever (a credential helper/401 can block past any await-only guard). + await expect( + pushRepoDirectly(clone, '[teamai] Register member: alice', ['members/alice.yaml'], { initPush: true }), + ).rejects.toThrow(); + } finally { + process.env.HOME = prevHome; + if (prevGlobal === undefined) delete process.env.GIT_CONFIG_GLOBAL; + else process.env.GIT_CONFIG_GLOBAL = prevGlobal; + if (prevSystem === undefined) delete process.env.GIT_CONFIG_NOSYSTEM; + else process.env.GIT_CONFIG_NOSYSTEM = prevSystem; + if (prevPrompt === undefined) delete process.env.GIT_TERMINAL_PROMPT; + else process.env.GIT_TERMINAL_PROMPT = prevPrompt; + if (prevInitTimeout === undefined) delete process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS; + else process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS = prevInitTimeout; + } + const elapsed = Date.now() - start; + + // The spawn-level block timeout (3s here) kills the hung push; without it + // the push would block indefinitely. Allow headroom for git startup. + expect(elapsed).toBeLessThan(10_000); + }, 30_000); +}); diff --git a/src/__tests__/init.test.ts b/src/__tests__/init.test.ts index abf05853c..d7ab06f31 100644 --- a/src/__tests__/init.test.ts +++ b/src/__tests__/init.test.ts @@ -13,6 +13,8 @@ const mockGit = { push: vi.fn(), revparse: vi.fn().mockResolvedValue('main'), raw: vi.fn(), + // createGit applies GIT_TERMINAL_PROMPT=0 via simple-git's .env() chainable. + env: vi.fn().mockReturnThis(), }; vi.mock('simple-git', () => ({ diff --git a/src/index.ts b/src/index.ts index 0b40234f7..f6a4652ad 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,10 +1,16 @@ import { createRequire } from 'node:module'; import { Command, Option } from 'commander'; import { setVerbose, setSilent, log } from './utils/logger.js'; +import { disableGitTerminalPrompt } from './utils/git.js'; import type { GlobalOptions, LocalConfig } from './types.js'; import { TEAMAI_HOOK_SUBCOMMANDS } from './hooks.js'; import { registerPackagesCommand } from './pkg/register-command.js'; +// Fail fast on a missing git credential instead of hanging on an invisible +// prompt (teamai runs git with no tty). Set once at module load so every +// command's git subprocesses inherit it. +disableGitTerminalPrompt(); + // Commands that migrate a legacy `/.teamai/` into the partition on first // run (issue #374 P1-3). Only write commands trigger it; read-only commands rely // on the double-read fallback, and hook-dispatch is excluded outright (see below). diff --git a/src/init.ts b/src/init.ts index aa685f9ad..21df75666 100644 --- a/src/init.ts +++ b/src/init.ts @@ -3,8 +3,8 @@ import fs from 'node:fs'; import path from 'node:path'; import { saveLocalConfig, loadTeamConfig, saveLocalConfigForScope, loadLocalConfigForScope, loadStateForScope, saveStateForScope, resolveProjectDataHome } from './config.js'; import { reconcileTeamHooksForConfig } from './hooks.js'; -import { configureGitUser, initRepo, isGitRepo, getRemoteUrl, remotesMatch, redactGitCredentials, pullRepoFastForward } from './utils/git.js'; -import { pushRepoDirectly } from './utils/git.js'; +import { configureGitUser, initRepo, isGitRepo, getRemoteUrl, remotesMatch, redactGitCredentials, pullRepoFastForward, pushRepoDirectly, autoPushViaMR, initPushBlockTimeoutMs } from './utils/git.js'; +import { withTimeout } from './utils/async.js'; import { getProvider, detectProviderForInit, RepoNotFoundError, OrganizationNotFoundError, RepoCreatePermissionError } from './providers/index.js'; import { parseGenericGitExistingRemote } from './providers/git/repo-url.js'; import { ensureDir, writeFile, pathExists, expandHome, readFileSafe, remove } from './utils/fs.js'; @@ -975,26 +975,30 @@ export async function initSelfRepo(options: GlobalOptions & { const { updateReports } = await import('./utils/reports-branch.js'); let isNewSelfMember = false; let selfMemberChanged = false; - const pushed = await updateReports(localConfig, async (wt) => { - const memberDir = path.join(wt, 'members'); - await ensureDir(memberDir); - const memberPath = path.join(memberDir, `${username}.yaml`); - isNewSelfMember = !await pathExists(memberPath); - const existingSelfMember = await getMemberConfig(wt, username); - const merged = mergeMemberConfig(existingSelfMember, { - username, - projects: localConfig.projects, - }); - selfMemberChanged = merged.changed; - if (!merged.changed) return null; - await writeFile(memberPath, YAML.stringify(merged.config)); - return { - files: ['members/'], - message: isNewSelfMember - ? `[teamai] Register member: ${username}` - : `[teamai] Update member roster: ${username}`, - }; - }); + const pushed = await withTimeout( + updateReports(localConfig, async (wt) => { + const memberDir = path.join(wt, 'members'); + await ensureDir(memberDir); + const memberPath = path.join(memberDir, `${username}.yaml`); + isNewSelfMember = !await pathExists(memberPath); + const existingSelfMember = await getMemberConfig(wt, username); + const merged = mergeMemberConfig(existingSelfMember, { + username, + projects: localConfig.projects, + }); + selfMemberChanged = merged.changed; + if (!merged.changed) return null; + await writeFile(memberPath, YAML.stringify(merged.config)); + return { + files: ['members/'], + message: isNewSelfMember + ? `[teamai] Register member: ${username}` + : `[teamai] Update member roster: ${username}`, + }; + }, { initPush: true }), + initPushBlockTimeoutMs(), + 'Member registration push', + ); if (selfMemberChanged) { if (pushed) { log.success(isNewSelfMember @@ -1420,14 +1424,18 @@ export async function init(options: GlobalOptions & { // after that go to teamai-reports. if (createdSkeleton && !options.dryRun) { try { - await pushRepoDirectly(localPath, '[teamai] Initialize team repo skeleton', [ - 'teamai.yaml', - 'skills/.gitkeep', - 'rules/.gitkeep', - 'docs/.gitkeep', - 'env/.gitkeep', - 'members/.gitkeep', - ]); + await withTimeout( + pushRepoDirectly(localPath, '[teamai] Initialize team repo skeleton', [ + 'teamai.yaml', + 'skills/.gitkeep', + 'rules/.gitkeep', + 'docs/.gitkeep', + 'env/.gitkeep', + 'members/.gitkeep', + ], { initPush: true }), + initPushBlockTimeoutMs(), + 'Skeleton push', + ); } catch (e) { log.warn(`Push failed (you can push manually later): ${(e as Error).message}`); } @@ -1441,27 +1449,31 @@ export async function init(options: GlobalOptions & { const { updateReports } = await import('./utils/reports-branch.js'); let memberChanged = false; let memberProjects: string[] | undefined; - const pushed = await updateReports(reportsConfig, async (wt) => { - const memberDir = path.join(wt, 'members'); - await ensureDir(memberDir); - const memberPath = path.join(memberDir, `${username}.yaml`); - isNewMember = !await pathExists(memberPath); - const existingMember = await getMemberConfig(wt, username); - const merged = mergeMemberConfig(existingMember, { - username, - projects: resolvedProjects, - }); - memberChanged = merged.changed; - memberProjects = merged.config.projects; - if (!merged.changed) return null; - await writeFile(memberPath, YAML.stringify(merged.config)); - return { - files: ['members/'], - message: isNewMember - ? `[teamai] Register member: ${username}` - : `[teamai] Update member roster: ${username}`, - }; - }); + const pushed = await withTimeout( + updateReports(reportsConfig, async (wt) => { + const memberDir = path.join(wt, 'members'); + await ensureDir(memberDir); + const memberPath = path.join(memberDir, `${username}.yaml`); + isNewMember = !await pathExists(memberPath); + const existingMember = await getMemberConfig(wt, username); + const merged = mergeMemberConfig(existingMember, { + username, + projects: resolvedProjects, + }); + memberChanged = merged.changed; + memberProjects = merged.config.projects; + if (!merged.changed) return null; + await writeFile(memberPath, YAML.stringify(merged.config)); + return { + files: ['members/'], + message: isNewMember + ? `[teamai] Register member: ${username}` + : `[teamai] Update member roster: ${username}`, + }; + }, { initPush: true }), + initPushBlockTimeoutMs(), + 'Member registration push', + ); if (memberChanged) { log.success(isNewMember ? `Registered as team member: ${username}` @@ -1509,10 +1521,36 @@ export async function init(options: GlobalOptions & { if (!options.dryRun) { try { - await pushRepoDirectly(localPath, `[teamai] Configure reviewers: ${reviewers.join(', ')}`, [ - 'teamai.yaml', - ]); - log.success('Reviewer config pushed to team repo'); + // Reviewer config goes via MR: a protected default branch rejects + // a direct push (the original bug), and an MR lands it on a + // feature branch for a maintainer to merge. The push itself uses + // createGitForInitPush (spawn-level timeout); the provider's + // pr-create spawnSync also carries the init-push timeout so a + // stalled gh/gf process cannot block the event loop past it. + const mrTeamConfig = await loadTeamConfig(localPath); + const mrLocalConfig = { + repo: { remote: repoInfo.httpsUrl, localPath }, + username, + }; + if (mrTeamConfig) { + const prUrl = await withTimeout( + autoPushViaMR( + localPath, + `[teamai] Configure reviewers: ${reviewers.join(', ')}`, + ['teamai.yaml'], + mrTeamConfig, + mrLocalConfig, + { initPush: true }, + ), + initPushBlockTimeoutMs(), + 'Reviewer config push', + ); + if (prUrl) { + log.success(`Reviewer config pushed via MR: ${prUrl}`); + } else { + log.warn('Reviewer config MR could not be created (you can push manually later)'); + } + } } catch (e) { log.warn(`Push failed (you can push manually later): ${(e as Error).message}`); } diff --git a/src/providers/github/gh-cli.ts b/src/providers/github/gh-cli.ts index 2d5a5c9ff..3e6a092ce 100644 --- a/src/providers/github/gh-cli.ts +++ b/src/providers/github/gh-cli.ts @@ -47,7 +47,7 @@ export function isGhInstalled(): boolean { */ export function ghExec( args: string[], - options?: { inheritStdio?: boolean; cwd?: string; env?: NodeJS.ProcessEnv }, + options?: { inheritStdio?: boolean; cwd?: string; env?: NodeJS.ProcessEnv; timeoutMs?: number }, ): { stdout: string; stderr: string; status: number } { const ghPath = getGhPath(); if (!ghPath) { @@ -63,6 +63,7 @@ export function ghExec( stdio: 'inherit', env: { ...process.env, ...(options.env ?? {}) }, cwd: options.cwd, + ...(options.timeoutMs ? { timeout: options.timeoutMs } : {}), }); return { stdout: '', stderr: '', status: result.status ?? 1 }; } @@ -72,6 +73,7 @@ export function ghExec( encoding: 'utf-8', maxBuffer: 10 * 1024 * 1024, cwd: options?.cwd, + ...(options?.timeoutMs ? { timeout: options.timeoutMs } : {}), }); return { @@ -338,6 +340,10 @@ export interface GhPrCreateOptions { reviewers?: string[]; /** Working directory (the team repo local path) */ cwd?: string; + /** Hard timeout (ms) for the `gh pr create` subprocess; kills it at the OS + * level once exceeded. spawnSync blocks the event loop, so withTimeout cannot + * interrupt it — this is the only way to bound a stalled PR creation. */ + spawnTimeoutMs?: number; } /** @@ -384,7 +390,7 @@ function ghPrCreateViaCli(opts: GhPrCreateOptions): string { args.push('-r', opts.reviewers.join(',')); } - const result = ghExec(args, { cwd: opts.cwd }); + const result = ghExec(args, { cwd: opts.cwd, ...(opts.spawnTimeoutMs ? { timeoutMs: opts.spawnTimeoutMs } : {}) }); if (result.status !== 0) { const errMsg = result.stderr || result.stdout; throw new Error(`gh pr create failed: ${errMsg}`); diff --git a/src/providers/github/index.ts b/src/providers/github/index.ts index 3f623f771..9d7af0342 100644 --- a/src/providers/github/index.ts +++ b/src/providers/github/index.ts @@ -60,6 +60,7 @@ export class GitHubProvider implements GitProvider { description: opts.description, reviewers: opts.reviewers, cwd: opts.cwd, + spawnTimeoutMs: opts.spawnTimeoutMs, }); } diff --git a/src/providers/tgit/gf-cli.ts b/src/providers/tgit/gf-cli.ts index 964fec165..85c60578d 100644 --- a/src/providers/tgit/gf-cli.ts +++ b/src/providers/tgit/gf-cli.ts @@ -37,7 +37,7 @@ function shellQuote(s: string): string { */ export function gfExec( args: string[], - options?: { inheritStdio?: boolean; cwd?: string }, + options?: { inheritStdio?: boolean; cwd?: string; timeoutMs?: number }, ): { stdout: string; stderr: string; status: number } { const gfPath = getGfPath(); // Shell-quote every token (including the binary path) so values such as repo @@ -52,6 +52,7 @@ export function gfExec( stdio: 'inherit', env: { ...process.env }, cwd: options.cwd, + ...(options.timeoutMs ? { timeout: options.timeoutMs } : {}), }); return { stdout: '', stderr: '', status: result.status ?? 1 }; } @@ -61,6 +62,7 @@ export function gfExec( encoding: 'utf-8', maxBuffer: 10 * 1024 * 1024, cwd: options?.cwd, + ...(options?.timeoutMs ? { timeout: options.timeoutMs } : {}), }); return { @@ -397,6 +399,10 @@ export interface GfMrCreateOptions { reviewers?: string[]; /** Working directory for gf CLI (should be the team repo path) */ cwd?: string; + /** Hard timeout (ms) for the `gf mr create` subprocess; kills it at the OS + * level once exceeded. spawnSync blocks the event loop, so withTimeout cannot + * interrupt it — this bounds a stalled MR creation. */ + spawnTimeoutMs?: number; } /** @@ -420,7 +426,7 @@ export function gfMrCreate(opts: GfMrCreateOptions): string { args.push('-r', opts.reviewers.join(',')); } - const result = gfExec(args, { cwd: opts.cwd }); + const result = gfExec(args, { cwd: opts.cwd, ...(opts.spawnTimeoutMs ? { timeoutMs: opts.spawnTimeoutMs } : {}) }); if (result.status !== 0) { const errMsg = result.stderr || result.stdout; throw new Error(`gf mr create failed: ${errMsg}`); diff --git a/src/providers/tgit/index.ts b/src/providers/tgit/index.ts index 03b8f1452..cab62d4e1 100644 --- a/src/providers/tgit/index.ts +++ b/src/providers/tgit/index.ts @@ -61,6 +61,7 @@ export class TGitProvider implements GitProvider { description: opts.description, reviewers: opts.reviewers, cwd: opts.cwd, + spawnTimeoutMs: opts.spawnTimeoutMs, }); } diff --git a/src/providers/types.ts b/src/providers/types.ts index e67b68adb..ccf95ab8b 100644 --- a/src/providers/types.ts +++ b/src/providers/types.ts @@ -38,6 +38,14 @@ export interface PrCreateOptions { reviewers?: string[]; /** Working directory for CLI operations */ cwd?: string; + /** + * Hard timeout (ms) for any synchronous provider CLI subprocess (e.g. + * `gh pr create`, `gf mr`). When set, the spawned process is killed at the + * OS level once it exceeds this — a spawnSync blocks the event loop, so a + * plain withTimeout cannot interrupt it; this is the only way to guarantee + * a stalled MR creation cannot hold init hostage. + */ + spawnTimeoutMs?: number; } /** diff --git a/src/push.ts b/src/push.ts index a6001f591..d1e0370ef 100644 --- a/src/push.ts +++ b/src/push.ts @@ -92,6 +92,7 @@ async function createPrWithFallback( branchName: string, title: string, description: string, + opts: { spawnTimeoutMs?: number } = {}, ): Promise { const provider = getProvider(teamConfig.provider); const mrSpin = spinner('Creating Pull Request...').start(); @@ -114,6 +115,7 @@ async function createPrWithFallback( description, reviewers: teamConfig.reviewers?.length ? teamConfig.reviewers : undefined, cwd: localConfig.repo.localPath, + spawnTimeoutMs: opts.spawnTimeoutMs, }); mrSpin.succeed(`Pull Request created: ${prUrl}`); return prUrl; diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 3f5e7732e..1d9789e2c 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -27,7 +27,7 @@ import path from 'node:path'; import fse from 'fs-extra'; import type { SimpleGit } from 'simple-git'; -import { createGit, isGitRepo, commitSkippingHooks, isDedicatedRepoRoot } from './git.js'; +import { createGit, createGitForInitPush, isGitRepo, commitSkippingHooks, isDedicatedRepoRoot } from './git.js'; import { acquireLock, releaseLock } from '../update.js'; import { ensureDir, writeFile, pathExists } from './fs.js'; import { log } from './logger.js'; @@ -114,8 +114,8 @@ async function nothingLeftToPush(git: SimpleGit, spec: BranchWorktreeSpec): Prom } } -async function remoteBranchExists(spec: BranchWorktreeSpec, repoRoot: string): Promise { - const git = createGit(repoRoot); +async function remoteBranchExists(spec: BranchWorktreeSpec, repoRoot: string, initPush = false): Promise { + const git = initPush ? createGitForInitPush(repoRoot) : createGit(repoRoot); try { const res = await git.listRemote(['--heads', 'origin', spec.branch]); return typeof res === 'string' && res.trim().length > 0; @@ -143,6 +143,8 @@ export interface EnsureWorktreeOptions { * view without changing origin. */ pushIfCreated?: boolean; + /** Use the spawn-level block-timeout git factory (init pushes). */ + initPush?: boolean; } async function ensureWorktree( @@ -168,9 +170,10 @@ async function ensureWorktree( // worktree gitdir (`/.git/worktrees/`) is gone and git ops // fail with "not a git repository". Probe a real git command and fall through // to remove+recreate when the link is stale. + const makeGit = options.initPush ? createGitForInitPush : createGit; if (await isGitRepo(wt)) { try { - await createGit(wt).revparse(['--is-inside-work-tree']); + await makeGit(wt).revparse(['--is-inside-work-tree']); return wt; } catch { // stale/dangling worktree link (clone was re-cloned/pruned) — recreate below. @@ -183,7 +186,7 @@ async function ensureWorktree( } await ensureDir(path.dirname(wt)); - const git = createGit(repoRoot); + const git = makeGit(repoRoot); // Prune any dangling worktree registration left from a previous removal. try { @@ -192,7 +195,7 @@ async function ensureWorktree( // best effort } - if (await remoteBranchExists(spec, repoRoot)) { + if (await remoteBranchExists(spec, repoRoot, options.initPush)) { // Remote branch exists: fetch and check it out into the worktree. try { await git.fetch(['origin', spec.branch]); @@ -214,15 +217,15 @@ async function ensureWorktree( if (branches.all.includes(spec.branch)) { await git.raw(['worktree', 'add', wt, spec.branch]); } else { - await createOrphanWorktree(spec, repoRoot, wt); + await createOrphanWorktree(spec, repoRoot, wt, options.initPush); await writeWorktreeGitignore(wt); - const wtGit = createGit(wt); + const wtGit = makeGit(wt); await wtGit.add(['.gitignore']); await commitSkippingHooks(wtGit, spec.initCommitMessage); } if (options.pushIfCreated !== false) { try { - await createGit(wt).push(['-u', 'origin', spec.branch]); + await makeGit(wt).push(['-u', 'origin', spec.branch]); } catch (e) { log.debug(`[${spec.logTag}] initial push skipped: ${(e as Error).message}`); } @@ -236,8 +239,8 @@ async function ensureWorktree( * Create an orphan-branch worktree. Uses the modern `--orphan` flag (git 2.42+) * and falls back to the detach + `checkout --orphan` dance for older git. */ -async function createOrphanWorktree(spec: BranchWorktreeSpec, repoRoot: string, wt: string): Promise { - const git = createGit(repoRoot); +async function createOrphanWorktree(spec: BranchWorktreeSpec, repoRoot: string, wt: string, initPush = false): Promise { + const git = initPush ? createGitForInitPush(repoRoot) : createGit(repoRoot); try { // git 2.42+: create a worktree on a fresh orphan branch directly. // The branch name must be given via -b; a positional after is treated @@ -245,7 +248,7 @@ async function createOrphanWorktree(spec: BranchWorktreeSpec, repoRoot: string, await git.raw(['worktree', 'add', '--orphan', '-b', spec.branch, wt]); // The --orphan worktree may inherit the index/files from HEAD in some git // versions; clear tracked entries so the branch starts empty. - const wtGit = createGit(wt); + const wtGit = initPush ? createGitForInitPush(wt) : createGit(wt); try { await wtGit.raw(['rm', '-rf', '--cached', '.']); } catch { @@ -255,7 +258,7 @@ async function createOrphanWorktree(spec: BranchWorktreeSpec, repoRoot: string, } catch { // Older git (<2.42): detach a worktree at HEAD, then orphan-checkout inside it. await git.raw(['worktree', 'add', '--detach', wt, 'HEAD']); - const wtGit = createGit(wt); + const wtGit = initPush ? createGitForInitPush(wt) : createGit(wt); await wtGit.raw(['checkout', '--orphan', spec.branch]); try { await wtGit.raw(['rm', '-rf', '--cached', '.']); @@ -287,21 +290,33 @@ async function writeWorktreeGitignore(wt: string): Promise { } const MAX_PUSH_RETRIES = 5; +// Init pushes are non-blocking (init completes and writes local config + skills +// regardless); a failed push is retried on the next init. Bounding retries to 1 +// during init prevents a stuck remote from holding init for 5× the timeout via +// the push/fetch/rebase retry loop. +const INIT_PUSH_MAX_RETRIES = 1; export interface BranchWrite { files: string[]; message: string; } +/** Options threaded through commitAndPushAt and its callers. */ +export interface BranchPushOptions { + pushIfUnchanged?: boolean; + /** Use the spawn-level block-timeout git factory (init pushes). */ + initPush?: boolean; +} + /** Commit `files` in an already-locked worktree and push them. */ async function commitAndPushAt( spec: BranchWorktreeSpec, wt: string, message: string, files: string[], - options: { pushIfUnchanged?: boolean } = {}, + options: BranchPushOptions = {}, ): Promise { - const git = createGit(wt); + const git = options.initPush ? createGitForInitPush(wt) : createGit(wt); await git.add(files); const status = await git.status(); @@ -321,7 +336,8 @@ async function commitAndPushAt( // Push with fetch+rebase retry. Each member only writes .yaml, so // rebase conflicts are effectively impossible; retries handle the pure // non-fast-forward race. - for (let attempt = 1; attempt <= MAX_PUSH_RETRIES; attempt++) { + const maxRetries = options.initPush ? INIT_PUSH_MAX_RETRIES : MAX_PUSH_RETRIES; + for (let attempt = 1; attempt <= maxRetries; attempt++) { try { // A push that resolves is a push the remote accepted: git exits non-zero // when it refuses one. Do NOT re-check the remote-tracking ref here — it @@ -330,7 +346,7 @@ async function commitAndPushAt( await git.push(['origin', spec.branch]); return { status: 'published' }; } catch (pushErr) { - if (attempt === MAX_PUSH_RETRIES) { + if (attempt === maxRetries) { log.debug(`[${spec.logTag}] push failed after ${attempt} attempts: ${(pushErr as Error).message}`); return { status: 'failed', reason: (pushErr as Error).message }; } @@ -367,7 +383,7 @@ async function commitAndPushImpl( localConfig: LocalConfig, message: string, files: string[], - options: { pushIfUnchanged?: boolean } = {}, + options: BranchPushOptions = {}, ): Promise { const lockPath = lockFilePath(spec, localConfig); const locked = await acquireLock(lockPath); @@ -377,7 +393,7 @@ async function commitAndPushImpl( } try { - const wt = await ensureWorktree(spec, localConfig); + const wt = await ensureWorktree(spec, localConfig, { initPush: options.initPush }); return await commitAndPushAt(spec, wt, message, files, options); } catch (e) { log.debug(`[${spec.logTag}] commitAndPush failed (non-blocking): ${(e as Error).message}`); @@ -398,7 +414,7 @@ async function updateImpl( spec: BranchWorktreeSpec, localConfig: LocalConfig, write: (worktree: string) => Promise, - options: { pushIfUnchanged?: boolean } = {}, + options: BranchPushOptions = {}, ): Promise { if (!usesBranchWorktree(localConfig)) { throw new Error(`update() needs a branch-backed repo, and ${spec.branch} has none for kind: 'http'`); @@ -411,9 +427,9 @@ async function updateImpl( } try { - const wt = await ensureWorktree(spec, localConfig); + const wt = await ensureWorktree(spec, localConfig, { initPush: options.initPush }); try { - await syncWorktree(spec, wt); + await syncWorktree(spec, wt, options.initPush); } catch (e) { log.debug(`[${spec.logTag}] sync before write failed, writing onto the local copy: ${(e as Error).message}`); } @@ -522,8 +538,8 @@ async function applyDirtySnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: * `git stash` in another worktree of this repo is left alone. Stash-apply * conflicts restore the original uncommitted files, never conflict markers. */ -async function syncWorktree(spec: BranchWorktreeSpec, wt: string): Promise { - const git = createGit(wt); +async function syncWorktree(spec: BranchWorktreeSpec, wt: string, initPush = false): Promise { + const git = initPush ? createGitForInitPush(wt) : createGit(wt); const upstream = `origin/${spec.branch}`; try { await git.fetch(['origin', spec.branch]); @@ -631,13 +647,13 @@ export interface BranchWorktree { update( localConfig: LocalConfig, write: (worktree: string) => Promise, - options?: { pushIfUnchanged?: boolean }, + options?: BranchPushOptions, ): Promise; commitAndPush( localConfig: LocalConfig, message: string, files: string[], - options?: { pushIfUnchanged?: boolean }, + options?: BranchPushOptions, ): Promise; refresh(localConfig: LocalConfig, options?: EnsureWorktreeOptions): Promise; } diff --git a/src/utils/git.ts b/src/utils/git.ts index 84a6a49e4..0e87b796f 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -5,17 +5,87 @@ import fse from 'fs-extra'; import simpleGit, { type SimpleGit } from 'simple-git'; import { log } from './logger.js'; +/** + * Disables git's interactive credential prompt for the whole teamai process. + * + * With this unset, a push with missing credentials opens a username/password + * prompt on the tty — and since teamai runs git as a subprocess with no tty, + * the push hangs forever, stalling init before the local config is written. + * `GIT_TERMINAL_PROMPT=0` makes git fail fast with "could not read Username" + * instead. + * + * Set once at process start (see {@link disableGitTerminalPrompt}) so every + * spawned git subprocess inherits it. We do NOT use simple-git's `.env()` + * chainable for this: both its overloads *replace* the child's entire + * environment (`this.env` is used verbatim as the spawn `env`, not merged + * with `process.env`), which would drop PATH/HOME and break git itself. + */ +const NO_PROMPT_VAR = 'GIT_TERMINAL_PROMPT' as const; +const NO_PROMPT_VAL = '0' as const; + +/** + * Set `GIT_TERMINAL_PROMPT=0` process-wide so no git subprocess can block on + * an interactive credential prompt. Idempotent; safe to call multiple times. + * Preserves an explicit user override if one is already set. + */ +export function disableGitTerminalPrompt(): void { + if (process.env[NO_PROMPT_VAR] === undefined) { + process.env[NO_PROMPT_VAR] = NO_PROMPT_VAL; + } +} + +/** + * Block timeout for git subprocesses spawned during `teamai init` pushes. + * simple-git's `timeout.block` kills the spawned process when it produces no + * output for this long (unlike `withTimeout`, a Promise.race that only stops + * awaiting while the child keeps running and holds the Node event loop open). + * + * Scoped to init's network pushes only: a global ceiling here would also kill + * legitimate slow clones/fetches/rebases in unrelated commands. The env var + * lets a slow link or a very large team repo raise the ceiling without a new + * release. Read at call time (not module load) so tests can override it. + * + * Validates the env var: a non-numeric, negative, or non-finite value falls + * back to the default rather than reaching simple-git (which would either + * silently disable the timeout or misbehave). + */ +export function initPushBlockTimeoutMs(): number { + const raw = process.env.TEAMAI_INIT_PUSH_TIMEOUT_MS; + if (raw !== undefined) { + const parsed = Number(raw); + if (Number.isFinite(parsed) && parsed > 0) { + return parsed; + } + } + return 30_000; +} + /** * Create a SimpleGit instance for a given base path. * * Authentication is handled by the provider's remote URL or by normal Git * facilities such as credential helpers, SSH config, and SSH agents. + * + * Every instance inherits the process-wide `GIT_TERMINAL_PROMPT=0` set by + * {@link disableGitTerminalPrompt}, so a missing credential fails fast + * instead of hanging on a prompt that can never be answered. */ export function createGit(basePath?: string): SimpleGit { - if (basePath) { - return simpleGit({ baseDir: basePath }); - } - return simpleGit(); + return basePath ? simpleGit({ baseDir: basePath }) : simpleGit(); +} + +/** + * Like {@link createGit}, but additionally kills any git subprocess that + * produces no output for the configured init-push block timeout. Reserved for + * `teamai init` pushes: a hung push is killed at the process level (the + * `withTimeout` wrapper around the await is only a second layer), while + * unrelated commands keep simple-git's default of no spawn timeout. + */ +export function createGitForInitPush(basePath?: string): SimpleGit { + const block = initPushBlockTimeoutMs(); + return basePath + ? simpleGit({ baseDir: basePath, timeout: { block } }) + : simpleGit({ timeout: { block } }); } /** @@ -54,7 +124,7 @@ export async function isGitRepo(localPath: string): Promise { */ export async function initRepo(remote: string, localPath: string): Promise { await fse.ensureDir(localPath); - const git = simpleGit({ baseDir: localPath }); + const git = createGit(localPath); await git.init(); await git.addRemote('origin', remote); } @@ -337,9 +407,17 @@ export async function getDefaultBranch(localPath: string): Promise { /** * Push directly to whatever branch is checked out, whether that is `main`, * `master` or anything else. Used during init for first-time setup, and by CI. + * + * `opts.initPush` selects the spawn-level block timeout factory used by init + * (see {@link createGitForInitPush}); other callers get the plain factory. */ -export async function pushRepoDirectly(localPath: string, message: string, files: string[]): Promise { - const git = createGit(localPath); +export async function pushRepoDirectly( + localPath: string, + message: string, + files: string[], + opts: { initPush?: boolean } = {}, +): Promise { + const git = opts.initPush ? createGitForInitPush(localPath) : createGit(localPath); const existingFiles = []; for (const f of files) { const fullPath = fs.existsSync(`${localPath}/${f}`); @@ -420,10 +498,11 @@ export async function autoPushViaMR( files: string[], teamConfig: { repo: string; provider?: string; reviewers?: string[] }, localConfig: { repo: { remote: string; localPath: string }; username: string }, + opts: { initPush?: boolean } = {}, ): Promise { try { const branchName = generateBranchName(localConfig.username); - const pushed = await pushRepoBranch(repoPath, message, files, branchName); + const pushed = await pushRepoBranch(repoPath, message, files, branchName, { initPush: opts.initPush }); if (!pushed) { log.debug('[git] autoPushViaMR: nothing to commit'); return null; @@ -432,6 +511,7 @@ export async function autoPushViaMR( const { createPrWithFallback } = await import('../push.js'); const prUrl = await createPrWithFallback( teamConfig, localConfig, branchName, message, message, + opts.initPush ? { spawnTimeoutMs: initPushBlockTimeoutMs() } : {}, ); await checkoutMaster(repoPath); @@ -505,15 +585,18 @@ export async function remoteBranchExists( * force-pushed, which updates that PR in place instead of opening another one. * If the rebuilt tree matches what the remote branch already holds, nothing is * pushed and the function returns false. + * + * `opts.initPush` selects the spawn-level block timeout factory used by init + * (see {@link createGitForInitPush}); other callers get the plain factory. */ export async function pushRepoBranch( localPath: string, message: string, files: string[], branchName: string, - opts: { reuseBranch?: boolean } = {}, + opts: { reuseBranch?: boolean; initPush?: boolean } = {}, ): Promise { - const git = createGit(localPath); + const git = opts.initPush ? createGitForInitPush(localPath) : createGit(localPath); if (opts.reuseBranch) { // Fetch so the tree comparison below can see the remote branch's content. diff --git a/src/utils/reports-branch.ts b/src/utils/reports-branch.ts index 262176f9e..84e1875c9 100644 --- a/src/utils/reports-branch.ts +++ b/src/utils/reports-branch.ts @@ -14,6 +14,7 @@ import { pathExists } from './fs.js'; import { createBranchWorktree, isPublished, + type BranchPushOptions, type BranchWrite, type EnsureWorktreeOptions, } from './branch-worktree.js'; @@ -64,7 +65,7 @@ export async function commitAndPushReports( localConfig: LocalConfig, message: string, files: string[], - options: { pushIfUnchanged?: boolean } = {}, + options: BranchPushOptions = {}, ): Promise { return isPublished(await reportsBranch.commitAndPush(localConfig, message, files, options)); } @@ -73,7 +74,7 @@ export async function commitAndPushReports( export async function updateReports( localConfig: LocalConfig, write: (worktree: string) => Promise, - options: { pushIfUnchanged?: boolean } = {}, + options: BranchPushOptions = {}, ): Promise { return isPublished(await reportsBranch.update(localConfig, write, options)); }