From 432258b8419c9fc4984d702ab38830e89781a46c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:47:12 -0700 Subject: [PATCH 1/9] fix(cli): grant a confined seat its repo hooks when the git probe fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `git rev-parse --git-path hooks` failed, prepareCommitAttribution left COMMONLY_AGENT_ORIGINAL_HOOKS_PATH unset, so the claude and codex jails were not granted the repository's hooks. A hook outside the workspace then did not run: the commit was refused with EPERM where the jail admits the hook's metadata, and the hook was skipped without a word where it does not. On a probe that fails at once, read the hooks directory from the repository's own files instead: core.hooksPath from the repository config (relative paths resolve from the top of the tree), else the hooks folder of the common git directory, so a linked worktree gets its main repository's. Log one warning line when this is used. A probe that timed out reads nothing and logs a warning. The open() that stalled git would stall the listener too, and a synchronous read cannot be timed out, which is the TASK-321 deadlock again. A git that cannot open the repository cannot commit in it either. Tests: four fallback cases (default folder, relative hooksPath from a subdirectory, quoted hooksPath with a comment, linked worktree), no repository means no grant and no warning, and the timed-out probe reads nothing even with a repository present. A real sandbox-exec smoke shows a confined seat with a failed probe runs a hook outside its workspace, against a control jail without the grant where the hook does not run. Written by Rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 (TASK-322) Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 105 ++++++++++++++ .../seatbelt.commit-hooks.smoke.test.mjs | 127 +++++++++++++++++ cli/src/lib/commit-attribution.js | 133 ++++++++++++++++-- 3 files changed, 356 insertions(+), 9 deletions(-) create mode 100644 cli/__tests__/seatbelt.commit-hooks.smoke.test.mjs diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index 7d1cb47bdc..0a39547bd0 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -1,9 +1,11 @@ +import { jest } from '@jest/globals'; import { execFileSync as execFileSyncWithEnv, spawnSync, } from 'node:child_process'; import { accessSync, + appendFileSync, constants, existsSync, mkdirSync, @@ -31,6 +33,9 @@ const makeExecutable = (path, content) => { accessSync(path, constants.X_OK); }; +// A git that refuses the repository at once, as on a "dubious ownership" error. +const FAILING_GIT = '#!/bin/sh\necho "fatal: detected dubious ownership" >&2\nexit 128\n'; + describe('prepareCommitAttribution', () => { test('adds both trailers, preserves Git identity and delegates the repository hooks', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-')); @@ -290,10 +295,14 @@ describe('prepareCommitAttribution', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-hung-git-')); const stubBin = join(root, 'bin'); mkdirSync(stubBin); + // A repository whose hooks a fallback would find, so an absent path below + // shows that a timed-out probe reads nothing. + execFileSync('git', ['init', '--quiet'], { cwd: root }); // Stands in for a git whose open() never returns, as on a seat whose // gitdir sits under a consent-protected folder. It ignores SIGTERM, so // only the SIGKILL the probe sends can end it. makeExecutable(join(stubBin, 'git'), '#!/bin/sh\ntrap "" TERM\nexec sleep 60\n'); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); let attribution; try { const startedAt = Date.now(); @@ -307,12 +316,108 @@ describe('prepareCommitAttribution', () => { expect(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH).toBeUndefined(); expect(attribution.env.GIT_CONFIG_VALUE_0).toBe(attribution.hooksDirectory); expect(existsSync(join(attribution.hooksDirectory, 'prepare-commit-msg'))).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('did not answer in 5s'); } finally { + warn.mockRestore(); attribution?.cleanup(); rmSync(root, { recursive: true, force: true }); } }, 30000); + test.each([ + { + name: 'the default hooks folder', + setup: (repo) => { + mkdirSync(join(repo, '.git', 'hooks'), { recursive: true }); + return { cwd: repo, expected: join(repo, '.git', 'hooks') }; + }, + }, + { + name: 'a relative core.hooksPath, resolved from the top of the tree', + setup: (repo) => { + mkdirSync(join(repo, '.husky', '_'), { recursive: true }); + mkdirSync(join(repo, 'src')); + execFileSync('git', ['config', 'core.hooksPath', '.husky/_'], { cwd: repo }); + return { cwd: join(repo, 'src'), expected: join(repo, '.husky', '_') }; + }, + }, + { + name: 'a quoted core.hooksPath with a trailing comment', + setup: (repo, root) => { + const hooks = join(root, 'shared hooks'); + mkdirSync(hooks); + appendFileSync(join(repo, '.git', 'config'), `[core]\n\thooksPath = "${hooks}" ; set by hand\n`); + return { cwd: repo, expected: hooks }; + }, + }, + { + name: "a linked worktree's main repository hooks", + setup: (repo, root) => { + mkdirSync(join(repo, '.git', 'hooks'), { recursive: true }); + execFileSync('git', ['commit', '--allow-empty', '--quiet', '-m', 'init'], { cwd: repo }); + const worktree = join(root, 'worktree'); + execFileSync('git', ['worktree', 'add', '--quiet', '-b', 'side', worktree], { cwd: repo }); + return { cwd: worktree, expected: join(repo, '.git', 'hooks') }; + }, + }, + ])('a git that fails at once still yields $name', ({ setup }) => { + const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-fallback-')); + const repo = join(root, 'repo'); + const stubBin = join(root, 'bin'); + mkdirSync(repo); + mkdirSync(stubBin); + execFileSync('git', ['init', '--quiet'], { cwd: repo }); + execFileSync('git', ['config', 'user.name', 'Fixture Author'], { cwd: repo }); + execFileSync('git', ['config', 'user.email', 'fixture@example.test'], { cwd: repo }); + const { cwd, expected } = setup(repo, root); + makeExecutable(join(stubBin, 'git'), FAILING_GIT); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + let attribution; + try { + attribution = prepareCommitAttribution({ + cwd, + env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + agentName: 'forge', + adapter: 'claude', + }); + expect(realpathSync(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH)) + .toBe(realpathSync(expected)); + expect(attribution.sandboxEnv.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH) + .toBe(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('(exit 128)'); + expect(warn.mock.calls[0][0]).toContain('read from the repository'); + } finally { + warn.mockRestore(); + attribution?.cleanup(); + rmSync(root, { recursive: true, force: true }); + } + }); + + test('a git that fails at once outside any repository grants nothing and says nothing', () => { + const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-no-repo-')); + const stubBin = join(root, 'bin'); + mkdirSync(stubBin); + makeExecutable(join(stubBin, 'git'), FAILING_GIT); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + let attribution; + try { + attribution = prepareCommitAttribution({ + cwd: root, + env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + agentName: 'forge', + adapter: 'claude', + }); + expect(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + attribution?.cleanup(); + rmSync(root, { recursive: true, force: true }); + } + }); + test('keeps long seat IDs inside the email local-part limit with a stable hash', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-long-seat-')); execFileSync('git', ['init', '--quiet'], { cwd: root }); diff --git a/cli/__tests__/seatbelt.commit-hooks.smoke.test.mjs b/cli/__tests__/seatbelt.commit-hooks.smoke.test.mjs new file mode 100644 index 0000000000..533e9c2c77 --- /dev/null +++ b/cli/__tests__/seatbelt.commit-hooks.smoke.test.mjs @@ -0,0 +1,127 @@ +/** + * seatbelt.commit-hooks.smoke.test.mjs + * + * A confined seat commits under real sandbox-exec (TASK-322). When git cannot + * report the hooks directory before the turn, the seat is still granted the + * repository's hooks from the repository's own files, so a hook that lives + * outside the workspace runs. The control commits in a jail without that + * grant, where the hook does not run, so the test is not vacuous. + */ + +import { jest } from '@jest/globals'; +import { execFileSync, spawnSync } from 'child_process'; +import { + accessSync, constants, existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync, +} from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import { prepareCommitAttribution, supportedGitHooks } from '../src/lib/commit-attribution.js'; +import { wrapArgvWithSeatbelt } from '../src/lib/sandbox/seatbelt.js'; + +const macTest = process.platform === 'darwin' ? test : test.skip; + +const fixtureEnv = Object.fromEntries(Object.entries(process.env).filter(([name]) => ( + !name.startsWith('GIT_') && !name.startsWith('COMMONLY_AGENT_') +))); +const git = (args, cwd) => execFileSync('git', args, { cwd, env: fixtureEnv, encoding: 'utf8' }); +const makeExecutable = (path, content) => { + writeFileSync(path, content, { encoding: 'utf8', mode: 0o700 }); +}; +const isExecutable = (path) => { + try { + accessSync(path, constants.X_OK); + return true; + } catch { + return false; + } +}; + +describe('a confined seat whose git hooks probe failed', () => { + let root; + afterEach(() => { if (root) rmSync(root, { recursive: true, force: true }); }); + + macTest('still runs a repository hook that lives outside the workspace', () => { + root = realpathSync(mkdtempSync(join(tmpdir(), 'commonly-commit-hooks-seatbelt-'))); + const workspace = join(root, 'workspace'); + const state = join(root, 'state'); + const hooks = join(root, 'repo-hooks'); + const stubBin = join(root, 'bin'); + for (const dir of [workspace, state, hooks, stubBin]) mkdirSync(dir); + git(['init', '--quiet'], workspace); + git(['config', 'user.name', 'Fixture Author'], workspace); + git(['config', 'user.email', 'fixture@example.test'], workspace); + git(['config', 'core.hooksPath', hooks], workspace); + // Git runs hooks from the top of the working tree, which the jail can write. + makeExecutable(join(hooks, 'pre-commit'), '#!/bin/sh\nprintf ran > hook-ran\n'); + makeExecutable(join(stubBin, 'git'), '#!/bin/sh\nexit 128\n'); + // The jail runs the binary git itself runs; /usr/bin/git is an xcrun shim. + const realGit = realpathSync(join(git(['--exec-path'], workspace).trim(), 'git')); + + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const attribution = prepareCommitAttribution({ + cwd: workspace, + env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + agentName: 'forge', + displayName: 'Forge', + adapter: 'claude', + }); + warn.mockRestore(); + try { + expect(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH).toBe(hooks); + + // The grant both adapters build from the attribution env + // (claude.js and codex.js): every proxy hook, plus each original hook + // that is executable. + const commitInJail = (originalHooksPath, message) => { + writeFileSync(join(workspace, `${message}.txt`), `${message}\n`); + git(['add', `${message}.txt`], workspace); + const executablePaths = [ + realGit, + ...supportedGitHooks().map((name) => join(attribution.hooksDirectory, name)), + ...(originalHooksPath ? supportedGitHooks() + .map((name) => join(originalHooksPath, name)) + .filter(isExecutable) : []), + ]; + const argv = wrapArgvWithSeatbelt([realGit, 'commit', '--quiet', '-m', message], { + workspacePath: workspace, + workspaceAccess: 'write', + executablePath: realGit, + statePath: state, + executablePaths, + }); + return spawnSync(argv[0], argv.slice(1), { + cwd: workspace, + env: { + ...attribution.env, + PATH: fixtureEnv.PATH, + HOME: state, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + }, + encoding: 'utf8', + timeout: 30_000, + }); + }; + const lastMessage = () => git(['show', '-s', '--format=%B', 'HEAD'], workspace); + + // The jail lets a hook under /private/var pass `[ -x ]`, because the + // baseline admits metadata there, but not run, so this commit is refused. + // A hook whose metadata the jail hides fails `[ -x ]` and is skipped + // instead. The hook runs in neither case. + const control = commitInJail(null, 'without-grant'); + expect(control.status).not.toBe(0); + expect(control.stderr).toContain('Operation not permitted'); + expect(existsSync(join(workspace, 'hook-ran'))).toBe(false); + git(['reset', '--quiet'], workspace); + + const granted = commitInJail(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH, 'with-grant'); + expect(granted.stderr).toBe(''); + expect(granted.status).toBe(0); + expect(lastMessage()).toContain('Co-authored-by: Forge '); + expect(existsSync(join(workspace, 'hook-ran'))).toBe(true); + } finally { + attribution.cleanup(); + } + }, 60_000); +}); diff --git a/cli/src/lib/commit-attribution.js b/cli/src/lib/commit-attribution.js index 3801ca4ed7..2f739e6b0d 100644 --- a/cli/src/lib/commit-attribution.js +++ b/cli/src/lib/commit-attribution.js @@ -4,12 +4,13 @@ import { accessSync, constants, mkdtempSync, + readFileSync, rmSync, statSync, writeFileSync, } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join, resolve } from 'node:path'; +import { homedir, tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; import { spawnSync } from 'node:child_process'; const GIT_HOOKS = [ @@ -77,6 +78,104 @@ const seatAddressId = (agentName, instanceId) => { // resolves the original hooks itself at commit time. const GIT_PROBE_TIMEOUT_MS = 5000; +const directoryOrNull = (path) => { + try { + return statSync(path).isDirectory() ? path : null; + } catch { + return null; + } +}; + +// One git config value: quotes group, a backslash escapes the next character, +// and an unquoted # or ; starts a comment. +const configValue = (raw) => { + let value = ''; + let quoted = false; + for (let index = 0; index < raw.length; index += 1) { + const character = raw[index]; + if (character === '\\' && index + 1 < raw.length) { + index += 1; + value += raw[index]; + } else if (character === '"') { + quoted = !quoted; + } else if (!quoted && (character === '#' || character === ';')) { + break; + } else { + value += character; + } + } + return value.trim(); +}; + +// The last core.hooksPath in one config file, or null. `include` and +// conditional includes are not followed. +const configuredHooksPath = (configPath) => { + let text; + try { + text = readFileSync(configPath, 'utf8'); + } catch { + return null; + } + let inCore = false; + let hooksPath = null; + for (const rawLine of text.split(/\r?\n/)) { + let line = rawLine.trim(); + const section = /^\[\s*([A-Za-z0-9.-]+)\s*("[^"]*")?\s*\]/.exec(line); + if (section) { + inCore = section[1].toLowerCase() === 'core' && !section[2]; + line = line.slice(section[0].length).trim(); + } + const entry = inCore && /^hookspath\s*=(.*)$/i.exec(line); + if (entry) hooksPath = configValue(entry[1]); + } + return hooksPath || null; +}; + +// The hooks directory a commit in `cwd` would use, read from the repository's +// own files without running git: core.hooksPath from the repository config, +// else the hooks folder of the common git directory (a linked worktree shares +// its main repository's). Global config and includes are not read. +const hooksPathFromGitFiles = (cwd) => { + let top = resolve(cwd); + let entry = join(top, '.git'); + let entryStat = null; + for (;;) { + try { + entryStat = statSync(entry); + break; + } catch { + if (dirname(top) === top) return null; + top = dirname(top); + entry = join(top, '.git'); + } + } + let gitDir = entry; + if (entryStat.isFile()) { + let pointer = null; + try { + pointer = /^gitdir:\s*(.+?)\s*$/m.exec(readFileSync(entry, 'utf8')); + } catch { + // Unreadable: the same as no repository. + } + if (!pointer) return null; + gitDir = resolve(top, pointer[1]); + } + let commonDir = gitDir; + try { + commonDir = resolve(gitDir, readFileSync(join(gitDir, 'commondir'), 'utf8').trim()); + } catch { + // A main repository has no commondir file. + } + const configured = configuredHooksPath(join(commonDir, 'config')); + if (!configured) return directoryOrNull(join(commonDir, 'hooks')); + if (configured === '~' || configured.startsWith('~/')) { + return directoryOrNull(join(homedir(), configured.slice(1))); + } + // Git runs hooks from the top of the working tree, so a relative path + // resolves there. + return directoryOrNull(resolve(top, configured)); +}; + const resolveOriginalHooksPath = (cwd, env) => { const result = spawnSync( 'git', @@ -90,13 +189,29 @@ const resolveOriginalHooksPath = (cwd, env) => { killSignal: 'SIGKILL', }, ); - if (result.error || result.status !== 0 || !result.stdout?.trim()) return null; - const hooksPath = resolve(result.stdout.trim()); - try { - return statSync(hooksPath).isDirectory() ? hooksPath : null; - } catch { + if (result.error?.code === 'ETIMEDOUT') { + // The open() that stalled git would stall this process too, and a + // synchronous read cannot be timed out, so nothing is read here. A git + // that cannot open the repository cannot commit in it either. + // eslint-disable-next-line no-console + console.warn(`[commit-attribution] git did not answer in ${GIT_PROBE_TIMEOUT_MS / 1000}s in ${cwd}, ` + + 'so a confined seat is not granted this repository\'s hooks this turn'); return null; } + if (result.error || result.status !== 0 || !result.stdout?.trim()) { + // A confined seat can only run the hooks it is granted, and a hook it is + // not granted is skipped without a word. So read the hooks directory from + // the repository's files instead. + const fallback = hooksPathFromGitFiles(cwd); + if (fallback) { + const reason = result.error?.code || `exit ${result.status}`; + // eslint-disable-next-line no-console + console.warn(`[commit-attribution] git could not report the hooks directory (${reason}), ` + + `so ${fallback} was read from the repository's files`); + } + return fallback; + } + return directoryOrNull(resolve(result.stdout.trim())); }; const resolveGitConfigCount = (env) => { @@ -192,8 +307,8 @@ export const prepareCommitAttribution = ({ COMMONLY_AGENT_GIT_CONFIG_BASE_COUNT: String(configIndex), COMMONLY_AGENT_HOOKS_PATH: hooksDirectory, // The sandbox adapters allow the workspace's existing hook files. The - // proxy resolves the target repo's path at hook time; a confined seat may - // fail closed if that repo's hook lies outside the workspace allowlist. + // proxy resolves the target repo's path at hook time, and a confined seat + // skips, without failing the commit, a hook outside what it was granted. ...(originalHooksPath ? { COMMONLY_AGENT_ORIGINAL_HOOKS_PATH: originalHooksPath } : {}), COMMONLY_AGENT_SEAT_NAME: cleanSingleLine(displayName || agentName, 'agent'), COMMONLY_AGENT_SEAT_ID: seatAddressId(agentName, instanceId), From 53515cfae5f72828bb975c44a1244daeb95b858b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:57:52 -0700 Subject: [PATCH 2/9] fix(cli): expand ~ hooks paths against the probe's HOME MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file fallback expanded `~` against the listener's own HOME, while the probe it stands in for runs git with the seat env. It now reads that env's HOME, and grants nothing when HOME is unset, as git does. Adds fallback cases for a gitdir: pointer relative to the .git file and for a ~/ core.hooksPath. Corrects two comments: an ungranted hook can refuse the commit, not only be skipped. Written by Rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 (TASK-322) Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 26 +++++++++++++++++++++-- cli/src/lib/commit-attribution.js | 20 +++++++++-------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index 0a39547bd0..2a897b435d 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -12,6 +12,7 @@ import { mkdtempSync, readFileSync, realpathSync, + renameSync, rmSync, writeFileSync, } from 'node:fs'; @@ -361,6 +362,27 @@ describe('prepareCommitAttribution', () => { return { cwd: worktree, expected: join(repo, '.git', 'hooks') }; }, }, + { + name: 'the hooks of a gitdir: pointer relative to the .git file', + setup: (repo, root) => { + // Submodules write a relative pointer, and git worktree add does not. + const store = join(root, 'store'); + renameSync(join(repo, '.git'), store); + writeFileSync(join(repo, '.git'), 'gitdir: ../store\n'); + mkdirSync(join(store, 'hooks'), { recursive: true }); + mkdirSync(join(repo, 'src')); + return { cwd: join(repo, 'src'), expected: join(store, 'hooks') }; + }, + }, + { + name: "a ~/ core.hooksPath under the probe's HOME", + setup: (repo, root) => { + const home = join(root, 'home'); + mkdirSync(join(home, 'team-hooks'), { recursive: true }); + execFileSync('git', ['config', 'core.hooksPath', '~/team-hooks'], { cwd: repo }); + return { cwd: repo, expected: join(home, 'team-hooks'), env: { HOME: home } }; + }, + }, ])('a git that fails at once still yields $name', ({ setup }) => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-fallback-')); const repo = join(root, 'repo'); @@ -370,14 +392,14 @@ describe('prepareCommitAttribution', () => { execFileSync('git', ['init', '--quiet'], { cwd: repo }); execFileSync('git', ['config', 'user.name', 'Fixture Author'], { cwd: repo }); execFileSync('git', ['config', 'user.email', 'fixture@example.test'], { cwd: repo }); - const { cwd, expected } = setup(repo, root); + const { cwd, expected, env = {} } = setup(repo, root); makeExecutable(join(stubBin, 'git'), FAILING_GIT); const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); let attribution; try { attribution = prepareCommitAttribution({ cwd, - env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + env: { ...fixtureEnv, ...env, PATH: `${stubBin}:${fixtureEnv.PATH}` }, agentName: 'forge', adapter: 'claude', }); diff --git a/cli/src/lib/commit-attribution.js b/cli/src/lib/commit-attribution.js index 2f739e6b0d..fc562cf5a7 100644 --- a/cli/src/lib/commit-attribution.js +++ b/cli/src/lib/commit-attribution.js @@ -9,7 +9,7 @@ import { statSync, writeFileSync, } from 'node:fs'; -import { homedir, tmpdir } from 'node:os'; +import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { spawnSync } from 'node:child_process'; @@ -134,8 +134,9 @@ const configuredHooksPath = (configPath) => { // The hooks directory a commit in `cwd` would use, read from the repository's // own files without running git: core.hooksPath from the repository config, // else the hooks folder of the common git directory (a linked worktree shares -// its main repository's). Global config and includes are not read. -const hooksPathFromGitFiles = (cwd) => { +// its main repository's). Global config and includes are not read. `~` expands +// against the HOME the probe gave git, as git itself would. +const hooksPathFromGitFiles = (cwd, env) => { let top = resolve(cwd); let entry = join(top, '.git'); let entryStat = null; @@ -169,7 +170,7 @@ const hooksPathFromGitFiles = (cwd) => { const configured = configuredHooksPath(join(commonDir, 'config')); if (!configured) return directoryOrNull(join(commonDir, 'hooks')); if (configured === '~' || configured.startsWith('~/')) { - return directoryOrNull(join(homedir(), configured.slice(1))); + return env.HOME ? directoryOrNull(join(env.HOME, configured.slice(1))) : null; } // Git runs hooks from the top of the working tree, so a relative path // resolves there. @@ -199,10 +200,11 @@ const resolveOriginalHooksPath = (cwd, env) => { return null; } if (result.error || result.status !== 0 || !result.stdout?.trim()) { - // A confined seat can only run the hooks it is granted, and a hook it is - // not granted is skipped without a word. So read the hooks directory from - // the repository's files instead. - const fallback = hooksPathFromGitFiles(cwd); + // A confined seat can only run the hooks it is granted. A hook it is not + // granted is skipped without a word, or refuses the commit where the jail + // can see the file but not run it. So read the hooks directory from the + // repository's files instead. + const fallback = hooksPathFromGitFiles(cwd, env); if (fallback) { const reason = result.error?.code || `exit ${result.status}`; // eslint-disable-next-line no-console @@ -308,7 +310,7 @@ export const prepareCommitAttribution = ({ COMMONLY_AGENT_HOOKS_PATH: hooksDirectory, // The sandbox adapters allow the workspace's existing hook files. The // proxy resolves the target repo's path at hook time, and a confined seat - // skips, without failing the commit, a hook outside what it was granted. + // cannot run a hook outside what it was granted. ...(originalHooksPath ? { COMMONLY_AGENT_ORIGINAL_HOOKS_PATH: originalHooksPath } : {}), COMMONLY_AGENT_SEAT_NAME: cleanSingleLine(displayName || agentName, 'agent'), COMMONLY_AGENT_SEAT_ID: seatAddressId(agentName, instanceId), From 9259b12dfc37f52382cb8c30da896512ef2f9eb2 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:08:35 -0700 Subject: [PATCH 3/9] fix(cli): keep a hooks path that ends in a space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git keeps quoted whitespace in a config value and prints a path verbatim, so a hooks directory may end in a space. The file fallback trimmed the quoted value and the probe trimmed git's output, so each granted a different directory than git uses. Both now drop only unquoted whitespace and git's newline. Each fallback case now also runs the real probe and checks the fixture against git's own --git-path hooks answer. Written by Rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 (TASK-322) Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 15 +++++++++++++-- cli/src/lib/commit-attribution.js | 13 +++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index 2a897b435d..eb0d0703ea 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -344,9 +344,9 @@ describe('prepareCommitAttribution', () => { }, }, { - name: 'a quoted core.hooksPath with a trailing comment', + name: 'a quoted core.hooksPath that ends in a space, with a trailing comment', setup: (repo, root) => { - const hooks = join(root, 'shared hooks'); + const hooks = join(root, 'shared hooks '); mkdirSync(hooks); appendFileSync(join(repo, '.git', 'config'), `[core]\n\thooksPath = "${hooks}" ; set by hand\n`); return { cwd: repo, expected: hooks }; @@ -393,6 +393,17 @@ describe('prepareCommitAttribution', () => { execFileSync('git', ['config', 'user.name', 'Fixture Author'], { cwd: repo }); execFileSync('git', ['config', 'user.email', 'fixture@example.test'], { cwd: repo }); const { cwd, expected, env = {} } = setup(repo, root); + // The fallback stands in for the probe, so real git checks the fixture + // and the probe's reading of git's answer. + const reported = execFileSync('git', ['rev-parse', '--path-format=absolute', '--git-path', 'hooks'], { + cwd, env: { ...fixtureEnv, ...env }, encoding: 'utf8', + }).replace(/\n$/, ''); + expect(realpathSync(reported)).toBe(realpathSync(expected)); + const probed = prepareCommitAttribution({ + cwd, env: { ...fixtureEnv, ...env }, agentName: 'forge', adapter: 'claude', + }); + probed.cleanup(); + expect(realpathSync(probed.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH)).toBe(realpathSync(expected)); makeExecutable(join(stubBin, 'git'), FAILING_GIT); const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); let attribution; diff --git a/cli/src/lib/commit-attribution.js b/cli/src/lib/commit-attribution.js index fc562cf5a7..9a929167bd 100644 --- a/cli/src/lib/commit-attribution.js +++ b/cli/src/lib/commit-attribution.js @@ -87,24 +87,28 @@ const directoryOrNull = (path) => { }; // One git config value: quotes group, a backslash escapes the next character, -// and an unquoted # or ; starts a comment. +// and an unquoted # or ; starts a comment. Whitespace around the value is +// dropped unless it is quoted. const configValue = (raw) => { let value = ''; + let kept = 0; let quoted = false; for (let index = 0; index < raw.length; index += 1) { const character = raw[index]; if (character === '\\' && index + 1 < raw.length) { index += 1; value += raw[index]; + kept = value.length; } else if (character === '"') { quoted = !quoted; } else if (!quoted && (character === '#' || character === ';')) { break; - } else { + } else if (quoted || value || !/\s/.test(character)) { value += character; + if (quoted) kept = value.length; } } - return value.trim(); + return value.slice(0, kept) + value.slice(kept).trimEnd(); }; // The last core.hooksPath in one config file, or null. `include` and @@ -213,7 +217,8 @@ const resolveOriginalHooksPath = (cwd, env) => { } return fallback; } - return directoryOrNull(resolve(result.stdout.trim())); + // Only git's newline ends the line, since a path can end in a space. + return directoryOrNull(resolve(result.stdout.replace(/\r?\n$/, ''))); }; const resolveGitConfigCount = (env) => { From 6f61bc873082df59f957ae6a4a72d371aa06a513 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:28:32 -0700 Subject: [PATCH 4/9] test(cli): read only repository config in commit-attribution fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A developer's global core.hooksPath moved the expected hooks directory and failed five tests, three of them the new real-git checks. The shared fixture env now sets GIT_CONFIG_GLOBAL=/dev/null and GIT_CONFIG_NOSYSTEM=1. Written by Rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 (TASK-322) Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index eb0d0703ea..a63c8db4ba 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -21,9 +21,15 @@ import { join } from 'node:path'; import { prepareCommitAttribution } from '../src/lib/commit-attribution.js'; -const fixtureEnv = Object.fromEntries(Object.entries(process.env).filter(([name]) => ( - !name.startsWith('GIT_CONFIG_') && !name.startsWith('COMMONLY_AGENT_') -))); +// A developer's global or system core.hooksPath would move every expected +// hooks directory, so the fixtures read only repository config. +const fixtureEnv = { + ...Object.fromEntries(Object.entries(process.env).filter(([name]) => ( + !name.startsWith('GIT_CONFIG_') && !name.startsWith('COMMONLY_AGENT_') + ))), + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', +}; const execFileSync = (command, args, options = {}) => execFileSyncWithEnv(command, args, { env: fixtureEnv, ...options, From b8f569416694f20c56ece22f2963c2e064adaeb9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:38:23 -0700 Subject: [PATCH 5/9] test(cli): keep an inherited GIT_CONFIG out of commit-attribution fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GIT_CONFIG redirects `git config` writes, so with it inherited the fixtures wrote core.hooksPath into the developer's file and six tests failed. The fixture env now drops it; a new case fails if the filter lets it through. Written by rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 31 +++++++++++++++++++---- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index a63c8db4ba..d1959c3757 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -22,14 +22,18 @@ import { join } from 'node:path'; import { prepareCommitAttribution } from '../src/lib/commit-attribution.js'; // A developer's global or system core.hooksPath would move every expected -// hooks directory, so the fixtures read only repository config. -const fixtureEnv = { - ...Object.fromEntries(Object.entries(process.env).filter(([name]) => ( - !name.startsWith('GIT_CONFIG_') && !name.startsWith('COMMONLY_AGENT_') +// hooks directory, so the fixtures read only repository config. An inherited +// GIT_CONFIG would also send each fixture `git config` write to that file. +const isolateGitConfig = (env) => ({ + ...Object.fromEntries(Object.entries(env).filter(([name]) => ( + name !== 'GIT_CONFIG' + && !name.startsWith('GIT_CONFIG_') + && !name.startsWith('COMMONLY_AGENT_') ))), GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_NOSYSTEM: '1', -}; +}); +const fixtureEnv = isolateGitConfig(process.env); const execFileSync = (command, args, options = {}) => execFileSyncWithEnv(command, args, { env: fixtureEnv, ...options, @@ -298,6 +302,23 @@ describe('prepareCommitAttribution', () => { } }); + test('fixture git config writes land in the repository, not an inherited GIT_CONFIG file', () => { + const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-git-config-')); + const repo = join(root, 'repo'); + const outside = join(root, 'outside.gitconfig'); + try { + mkdirSync(repo); + writeFileSync(outside, '[user]\n\tname = Outside\n'); + const env = isolateGitConfig({ ...process.env, GIT_CONFIG: outside }); + execFileSync('git', ['init', '--quiet'], { cwd: repo, env }); + execFileSync('git', ['config', 'core.hooksPath', 'team-hooks'], { cwd: repo, env }); + expect(readFileSync(outside, 'utf8')).toBe('[user]\n\tname = Outside\n'); + expect(readFileSync(join(repo, '.git', 'config'), 'utf8')).toContain('hooksPath = team-hooks'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + test('a git child that never exits cannot block the turn', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-hung-git-')); const stubBin = join(root, 'bin'); From 6cd909f43c9ada63751648b6a644449f9d00646a Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:48:49 -0700 Subject: [PATCH 6/9] test(cli): cover inherited GIT_CONFIG_* in the fixture isolation case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GIT_CONFIG case now also inherits GIT_CONFIG_COUNT/KEY/VALUE naming an outside core.hooksPath, and asserts git reads the repository value. Dropping either filter branch fails the case, with no GIT_CONFIG_* variables in the outer environment as well. Written by rivet, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index d1959c3757..123e2ac98b 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -302,18 +302,29 @@ describe('prepareCommitAttribution', () => { } }); - test('fixture git config writes land in the repository, not an inherited GIT_CONFIG file', () => { + test('fixtures read and write only repository config despite inherited GIT_CONFIG variables', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-git-config-')); const repo = join(root, 'repo'); const outside = join(root, 'outside.gitconfig'); try { mkdirSync(repo); writeFileSync(outside, '[user]\n\tname = Outside\n'); - const env = isolateGitConfig({ ...process.env, GIT_CONFIG: outside }); + const env = isolateGitConfig({ + ...process.env, + GIT_CONFIG: outside, + GIT_CONFIG_COUNT: '1', + GIT_CONFIG_KEY_0: 'core.hooksPath', + GIT_CONFIG_VALUE_0: join(root, 'env-hooks'), + }); execFileSync('git', ['init', '--quiet'], { cwd: repo, env }); execFileSync('git', ['config', 'core.hooksPath', 'team-hooks'], { cwd: repo, env }); expect(readFileSync(outside, 'utf8')).toBe('[user]\n\tname = Outside\n'); expect(readFileSync(join(repo, '.git', 'config'), 'utf8')).toContain('hooksPath = team-hooks'); + expect(execFileSync('git', ['config', '--get', 'core.hooksPath'], { + cwd: repo, + env, + encoding: 'utf8', + })).toBe('team-hooks\n'); } finally { rmSync(root, { recursive: true, force: true }); } From 49a48d1ace1dc999be5f3a00301f29256ecca899 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 23:58:46 -0700 Subject: [PATCH 7/9] fix(cli): grant fallback hooks only from a repository git would open Otto's gate on #2916 found that the fallback trusted the first .git it met. A .git holding only config and hooks/ above a non-repository cwd made git answer "not a git repository", and the fallback still granted that config's core.hooksPath. A repository another user owns went the same way. The fallback now returns no hooks unless the nearest .git is a repository git would open: HEAD names a ref or an object id, and the common directory holds objects/ and refs/. The worktree top, the gitdir and any .git file must be owned by this user's effective uid. Global safe.directory is not read, so a foreign repository it would allow yields no hooks, which is git's answer without it. Tests: five non-repository layouts, each checked against real git first, and a foreign-owned repository with the real uid as its control. Each check reddens only its own case when removed. 24/24, and the sandbox-exec smoke still runs the outside hook. Written by Rivet, a Commonly agent https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 Co-Authored-By: Claude Opus 5.5 --- cli/__tests__/commit-attribution.test.mjs | 85 +++++++++++++++++++++++ cli/src/lib/commit-attribution.js | 37 +++++++++- 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index 123e2ac98b..8cd77b325d 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -489,6 +489,91 @@ describe('prepareCommitAttribution', () => { } }); + // Each .git holds a config whose core.hooksPath names a directory of + // executable hooks, and real git refuses each one as a repository. + test.each([ + { name: 'a .git directory holding only config and hooks/', omit: ['HEAD', 'objects', 'refs'] }, + { name: 'a .git directory without objects/', omit: ['objects'] }, + { name: 'a .git directory without refs/', omit: ['refs'] }, + { name: 'a .git directory whose HEAD names no ref', head: 'main\n' }, + { + name: 'a .git file pointing at a directory holding only config and hooks/', + omit: ['HEAD', 'objects', 'refs'], + pointer: true, + }, + ])('a git that fails at once under $name grants nothing and says nothing', ({ omit = [], head, pointer }) => { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-not-a-repo-'))); + const top = join(root, 'a'); + const cwd = join(top, 'sub'); + const granted = join(root, 'granted'); + const gitDir = pointer ? join(root, 'store') : join(top, '.git'); + const stubBin = join(root, 'bin'); + mkdirSync(cwd, { recursive: true }); + mkdirSync(granted); + mkdirSync(stubBin); + makeExecutable(join(granted, 'pre-commit'), '#!/bin/sh\nexit 0\n'); + mkdirSync(join(gitDir, 'hooks'), { recursive: true }); + writeFileSync(join(gitDir, 'config'), `[core]\n\thooksPath = ${granted}\n`); + if (!omit.includes('HEAD')) writeFileSync(join(gitDir, 'HEAD'), head ?? 'ref: refs/heads/main\n'); + if (!omit.includes('objects')) mkdirSync(join(gitDir, 'objects')); + if (!omit.includes('refs')) mkdirSync(join(gitDir, 'refs')); + if (pointer) writeFileSync(join(top, '.git'), `gitdir: ${gitDir}\n`); + const real = spawnSync('git', ['rev-parse', '--git-path', 'hooks'], { + cwd, env: { ...fixtureEnv, GIT_CEILING_DIRECTORIES: root }, encoding: 'utf8', + }); + expect(real.status).not.toBe(0); + expect(real.stderr).toContain('not a git repository'); + makeExecutable(join(stubBin, 'git'), FAILING_GIT); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + let attribution; + try { + attribution = prepareCommitAttribution({ + cwd, + env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + agentName: 'forge', + adapter: 'claude', + }); + expect(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + attribution?.cleanup(); + rmSync(root, { recursive: true, force: true }); + } + }); + + // No second uid is available here, so a different effective uid stands in + // for another user. The same fixture under the real uid is the control. + test('a git that fails at once in a repository another user owns grants nothing and says nothing', () => { + const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-foreign-')); + const stubBin = join(root, 'bin'); + mkdirSync(stubBin); + execFileSync('git', ['init', '--quiet'], { cwd: root }); + mkdirSync(join(root, '.git', 'hooks'), { recursive: true }); + makeExecutable(join(stubBin, 'git'), FAILING_GIT); + const env = { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }; + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const attributions = []; + try { + attributions.push(prepareCommitAttribution({ + cwd: root, env, agentName: 'forge', adapter: 'claude', + })); + expect(realpathSync(attributions[0].env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH)) + .toBe(realpathSync(join(root, '.git', 'hooks'))); + warn.mockClear(); + jest.spyOn(process, 'geteuid').mockReturnValue(process.geteuid() + 1); + attributions.push(prepareCommitAttribution({ + cwd: root, env, agentName: 'forge', adapter: 'claude', + })); + expect(attributions[1].env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + } finally { + jest.restoreAllMocks(); + attributions.forEach((attribution) => attribution.cleanup()); + rmSync(root, { recursive: true, force: true }); + } + }); + test('keeps long seat IDs inside the email local-part limit with a stable hash', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-long-seat-')); execFileSync('git', ['init', '--quiet'], { cwd: root }); diff --git a/cli/src/lib/commit-attribution.js b/cli/src/lib/commit-attribution.js index 9a929167bd..7ddbbee165 100644 --- a/cli/src/lib/commit-attribution.js +++ b/cli/src/lib/commit-attribution.js @@ -3,8 +3,10 @@ import { createHash } from 'node:crypto'; import { accessSync, constants, + lstatSync, mkdtempSync, readFileSync, + readlinkSync, rmSync, statSync, writeFileSync, @@ -135,11 +137,41 @@ const configuredHooksPath = (configPath) => { return hooksPath || null; }; +// Git opens a git directory only if its HEAD names a ref or an object id and +// its common directory holds objects/ and refs/. +const isGitDirectory = (gitDir, commonDir) => { + if (!directoryOrNull(join(commonDir, 'objects')) || !directoryOrNull(join(commonDir, 'refs'))) { + return false; + } + const head = join(gitDir, 'HEAD'); + try { + if (lstatSync(head).isSymbolicLink()) return readlinkSync(head).startsWith('refs/'); + const line = readFileSync(head, 'utf8').split('\n')[0]; + return /^ref:\s*refs\//.test(line) || /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/.test(line); + } catch { + return false; + } +}; + +// Git refuses a repository whose files another user owns, unless global +// safe.directory config allows it. That config is not read here, so such a +// repository yields no hooks, which is git's own answer without it. +const ownedByThisUser = (path) => { + if (typeof process.geteuid !== 'function') return false; + try { + return lstatSync(path).uid === process.geteuid(); + } catch { + return false; + } +}; + // The hooks directory a commit in `cwd` would use, read from the repository's // own files without running git: core.hooksPath from the repository config, // else the hooks folder of the common git directory (a linked worktree shares // its main repository's). Global config and includes are not read. `~` expands -// against the HOME the probe gave git, as git itself would. +// against the HOME the probe gave git, as git itself would. The nearest .git +// must be a repository this user owns, or there are no hooks. Git would walk +// on past a .git directory that is not a repository, and this stops there. const hooksPathFromGitFiles = (cwd, env) => { let top = resolve(cwd); let entry = join(top, '.git'); @@ -171,6 +203,9 @@ const hooksPathFromGitFiles = (cwd, env) => { } catch { // A main repository has no commondir file. } + if (!isGitDirectory(gitDir, commonDir)) return null; + const owned = [top, gitDir, ...(entryStat.isFile() ? [entry] : [])]; + if (!owned.every(ownedByThisUser)) return null; const configured = configuredHooksPath(join(commonDir, 'config')); if (!configured) return directoryOrNull(join(commonDir, 'hooks')); if (configured === '~' || configured.startsWith('~/')) { From 3177a219d381d37245398a65bbd8a04c92637cf8 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 28 Oct 2026 00:03:52 -0700 Subject: [PATCH 8/9] test(cli): accept either wording git uses to refuse a bad gitfile Linux CI's git says "gitfile does not point to a valid repository" where git 2.54 says "not a git repository". Both are refusals, and the fixture only needs to show real git refuses the layout before the fallback runs. Written by Rivet, a Commonly agent https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- cli/__tests__/commit-attribution.test.mjs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index 8cd77b325d..db591ca9d3 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -521,8 +521,9 @@ describe('prepareCommitAttribution', () => { const real = spawnSync('git', ['rev-parse', '--git-path', 'hooks'], { cwd, env: { ...fixtureEnv, GIT_CEILING_DIRECTORIES: root }, encoding: 'utf8', }); + // Git versions word the refusal of a bad gitfile differently. expect(real.status).not.toBe(0); - expect(real.stderr).toContain('not a git repository'); + expect(real.stderr).toMatch(/not a git repository|gitfile does not point to a valid repository/); makeExecutable(join(stubBin, 'git'), FAILING_GIT); const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); let attribution; From a48961ee5796a72cd2e06a7bcfe866c8dbc23bdc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 28 Oct 2026 00:37:03 -0700 Subject: [PATCH 9/9] fix(cli): read the fallback's git files without blocking, after ownership Vera's note at 3177a219d: the fallback opened HEAD before checking who owns the repository, so a FIFO planted as HEAD hung it. Ownership of the top, the .git entry and a gitfile's target is now settled before any repository file is read. Every read (gitfile, commondir, HEAD, config) opens with O_NONBLOCK and returns null unless the target is a regular file, so a FIFO cannot hang even a repository this user owns. Tests: a FIFO HEAD and a FIFO config, each in a child killed at 8s. Both were killed on the old head. Making either read blocking again fails only its own case. Written by Rivet, a Commonly agent https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 Co-Authored-By: Claude Opus 5.5 --- cli/__tests__/commit-attribution.test.mjs | 43 ++++++++++++++++++++ cli/src/lib/commit-attribution.js | 48 +++++++++++++---------- 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/cli/__tests__/commit-attribution.test.mjs b/cli/__tests__/commit-attribution.test.mjs index db591ca9d3..b5d8115b04 100644 --- a/cli/__tests__/commit-attribution.test.mjs +++ b/cli/__tests__/commit-attribution.test.mjs @@ -575,6 +575,49 @@ describe('prepareCommitAttribution', () => { } }); + // A FIFO where git keeps a file would block a plain read forever, so each + // case runs in a child that is killed if it outlives the probe's own bound. + // The FIFO has no writer, and the repository is this user's own, so the + // ownership check cannot stand between the fallback and the read. + test.each([ + { name: 'HEAD', granted: false }, + { name: 'config', granted: true }, + ])('a git that fails at once beside a FIFO $name finishes without reading it', ({ name, granted }) => { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-fifo-'))); + const stubBin = join(root, 'bin'); + mkdirSync(stubBin); + execFileSync('git', ['init', '--quiet'], { cwd: root }); + mkdirSync(join(root, '.git', 'hooks'), { recursive: true }); + rmSync(join(root, '.git', name)); + execFileSync('mkfifo', [join(root, '.git', name)]); + makeExecutable(join(stubBin, 'git'), FAILING_GIT); + const moduleUrl = new URL('../src/lib/commit-attribution.js', import.meta.url).href; + const script = ` + const { prepareCommitAttribution } = await import(${JSON.stringify(moduleUrl)}); + console.warn = () => {}; + const attribution = prepareCommitAttribution({ + cwd: process.argv[1], env: process.env, agentName: 'forge', adapter: 'claude', + }); + process.stdout.write(JSON.stringify(attribution.env.COMMONLY_AGENT_ORIGINAL_HOOKS_PATH ?? null)); + attribution.cleanup(); + `; + try { + const child = spawnSync(process.execPath, ['--input-type=module', '-e', script, root], { + env: { ...fixtureEnv, PATH: `${stubBin}:${fixtureEnv.PATH}` }, + encoding: 'utf8', + timeout: 8000, + killSignal: 'SIGKILL', + }); + expect(child.signal).toBeNull(); + expect(child.status).toBe(0); + const hooksPath = JSON.parse(child.stdout); + if (granted) expect(realpathSync(hooksPath)).toBe(join(root, '.git', 'hooks')); + else expect(hooksPath).toBeNull(); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, 15000); + test('keeps long seat IDs inside the email local-part limit with a stable hash', () => { const root = mkdtempSync(join(tmpdir(), 'commonly-commit-attribution-long-seat-')); execFileSync('git', ['init', '--quiet'], { cwd: root }); diff --git a/cli/src/lib/commit-attribution.js b/cli/src/lib/commit-attribution.js index 7ddbbee165..ac2fde1aca 100644 --- a/cli/src/lib/commit-attribution.js +++ b/cli/src/lib/commit-attribution.js @@ -2,9 +2,12 @@ import { Buffer } from 'node:buffer'; import { createHash } from 'node:crypto'; import { accessSync, + closeSync, constants, + fstatSync, lstatSync, mkdtempSync, + openSync, readFileSync, readlinkSync, rmSync, @@ -88,6 +91,20 @@ const directoryOrNull = (path) => { } }; +// A regular file's text, or null. The open does not block, so a FIFO or a +// device where git keeps a file is refused instead of hanging the caller. +const readRegularFile = (path) => { + let fd = null; + try { + fd = openSync(path, constants.O_RDONLY | constants.O_NONBLOCK); + return fstatSync(fd).isFile() ? readFileSync(fd, 'utf8') : null; + } catch { + return null; + } finally { + if (fd !== null) closeSync(fd); + } +}; + // One git config value: quotes group, a backslash escapes the next character, // and an unquoted # or ; starts a comment. Whitespace around the value is // dropped unless it is quoted. @@ -116,12 +133,8 @@ const configValue = (raw) => { // The last core.hooksPath in one config file, or null. `include` and // conditional includes are not followed. const configuredHooksPath = (configPath) => { - let text; - try { - text = readFileSync(configPath, 'utf8'); - } catch { - return null; - } + const text = readRegularFile(configPath); + if (text === null) return null; let inCore = false; let hooksPath = null; for (const rawLine of text.split(/\r?\n/)) { @@ -146,7 +159,7 @@ const isGitDirectory = (gitDir, commonDir) => { const head = join(gitDir, 'HEAD'); try { if (lstatSync(head).isSymbolicLink()) return readlinkSync(head).startsWith('refs/'); - const line = readFileSync(head, 'utf8').split('\n')[0]; + const line = (readRegularFile(head) ?? '').split('\n')[0]; return /^ref:\s*refs\//.test(line) || /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/.test(line); } catch { return false; @@ -186,26 +199,19 @@ const hooksPathFromGitFiles = (cwd, env) => { entry = join(top, '.git'); } } + // Ownership is settled before any of the repository's files is read. + if (!ownedByThisUser(top) || !ownedByThisUser(entry)) return null; let gitDir = entry; if (entryStat.isFile()) { - let pointer = null; - try { - pointer = /^gitdir:\s*(.+?)\s*$/m.exec(readFileSync(entry, 'utf8')); - } catch { - // Unreadable: the same as no repository. - } + const pointer = /^gitdir:\s*(.+?)\s*$/m.exec(readRegularFile(entry) ?? ''); if (!pointer) return null; gitDir = resolve(top, pointer[1]); + if (!ownedByThisUser(gitDir)) return null; } - let commonDir = gitDir; - try { - commonDir = resolve(gitDir, readFileSync(join(gitDir, 'commondir'), 'utf8').trim()); - } catch { - // A main repository has no commondir file. - } + // A main repository has no commondir file. + const commonDirFile = readRegularFile(join(gitDir, 'commondir')); + const commonDir = commonDirFile === null ? gitDir : resolve(gitDir, commonDirFile.trim()); if (!isGitDirectory(gitDir, commonDir)) return null; - const owned = [top, gitDir, ...(entryStat.isFile() ? [entry] : [])]; - if (!owned.every(ownedByThisUser)) return null; const configured = configuredHooksPath(join(commonDir, 'config')); if (!configured) return directoryOrNull(join(commonDir, 'hooks')); if (configured === '~' || configured.startsWith('~/')) {