From 260b2560f90b3d347116d2ebcca06e632acba401 Mon Sep 17 00:00:00 2001 From: Ivan Stetsenko Date: Mon, 11 May 2026 22:36:17 -0400 Subject: [PATCH 1/4] feat: add lore hooks install/uninstall + CI integration (closes #41) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add commit-msg hook management for Lore protocol enforcement: - `lore hooks install` — installs commit-msg hook to .git/hooks/ - `lore hooks uninstall` — removes Lore-managed hooks only - Hook validates commits via `lore validate --last 1 --strict` - Skips merge commits, fixup/squash commits, configurable authors - Falls back from `lore` to `npx lore-protocol` when not on PATH - Detects existing non-Lore hooks (requires --force to overwrite, backs up to .bak) - [hooks] config section: enforce, allow_fixup, skip_authors, skip_patterns - .pre-commit-hooks.yaml for pre-commit framework integration - .github/workflows/lore-validate.yml for GitHub Actions CI Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/lore-validate.yml | 23 ++++ .pre-commit-hooks.yaml | 8 ++ src/commands/hooks.ts | 97 +++++++++++++++ src/commands/init.ts | 6 + src/interfaces/hook-installer.ts | 17 +++ src/main.ts | 12 ++ src/services/config-loader.ts | 5 + src/services/hook-installer.ts | 84 +++++++++++++ src/services/hook-script-generator.ts | 74 +++++++++++ src/types/config.ts | 7 ++ tests/unit/services/hook-installer.test.ts | 116 ++++++++++++++++++ .../services/hook-script-generator.test.ts | 95 ++++++++++++++ 12 files changed, 544 insertions(+) create mode 100644 .github/workflows/lore-validate.yml create mode 100644 .pre-commit-hooks.yaml create mode 100644 src/commands/hooks.ts create mode 100644 src/interfaces/hook-installer.ts create mode 100644 src/services/hook-installer.ts create mode 100644 src/services/hook-script-generator.ts create mode 100644 tests/unit/services/hook-installer.test.ts create mode 100644 tests/unit/services/hook-script-generator.test.ts diff --git a/.github/workflows/lore-validate.yml b/.github/workflows/lore-validate.yml new file mode 100644 index 00000000..9512fbbd --- /dev/null +++ b/.github/workflows/lore-validate.yml @@ -0,0 +1,23 @@ +name: Lore Validate + +on: + pull_request: + branches: [main] + +jobs: + validate: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - uses: actions/setup-node@v4 + with: + node-version: '18' + + - run: npm ci + - run: npm run build + + - name: Validate Lore commits + run: node ./bin/lore.js validate --since origin/main --strict diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml new file mode 100644 index 00000000..e4bbc7fe --- /dev/null +++ b/.pre-commit-hooks.yaml @@ -0,0 +1,8 @@ +- id: lore-validate + name: Lore protocol validation + description: Validate commit messages follow the Lore protocol + entry: lore validate --last 1 --strict + language: node + stages: [commit-msg] + always_run: true + additional_dependencies: ['lore-protocol'] diff --git a/src/commands/hooks.ts b/src/commands/hooks.ts new file mode 100644 index 00000000..df5012b0 --- /dev/null +++ b/src/commands/hooks.ts @@ -0,0 +1,97 @@ +import type { Command } from 'commander'; +import type { IGitClient } from '../interfaces/git-client.js'; +import type { IOutputFormatter } from '../interfaces/output-formatter.js'; +import type { IHookInstaller } from '../interfaces/hook-installer.js'; + +/** + * Register the `lore hooks` command group. + * Subcommands: install, uninstall. + */ +export function registerHooksCommand( + program: Command, + deps: { + hookInstaller: IHookInstaller; + gitClient: IGitClient; + getFormatter: () => IOutputFormatter; + }, +): void { + const hooks = program + .command('hooks') + .description('Manage git hooks for Lore protocol enforcement'); + + hooks + .command('install') + .description('Install commit-msg hook for Lore validation') + .option('--force', 'Overwrite existing non-Lore hooks (backs up to .bak)') + .action(async (options: { force?: boolean }) => { + const { hookInstaller, gitClient, getFormatter } = deps; + const formatter = getFormatter(); + + let repoRoot: string; + try { + repoRoot = await gitClient.getRepoRoot(); + } catch { + console.error(formatter.formatError(1, [ + { severity: 'error', message: 'Not inside a git repository' }, + ])); + process.exitCode = 1; + return; + } + + const result = await hookInstaller.install(repoRoot, { force: options.force ?? false }); + + if (!result.installed) { + console.error(formatter.formatError(1, [ + { severity: 'error', message: `Hook already exists at ${result.hookPath}. Use --force to overwrite (existing hook will be backed up to .bak).` }, + ])); + process.exitCode = 1; + return; + } + + if (result.backedUp) { + console.log(formatter.formatSuccess(`Existing hook backed up to ${result.backedUp}`)); + } + + if (result.alreadyInstalled) { + console.log(formatter.formatSuccess('Lore commit-msg hook updated.')); + } else { + console.log(formatter.formatSuccess(`Lore commit-msg hook installed at ${result.hookPath}`)); + } + }); + + hooks + .command('uninstall') + .description('Remove Lore-managed commit-msg hook') + .action(async () => { + const { hookInstaller, gitClient, getFormatter } = deps; + const formatter = getFormatter(); + + let repoRoot: string; + try { + repoRoot = await gitClient.getRepoRoot(); + } catch { + console.error(formatter.formatError(1, [ + { severity: 'error', message: 'Not inside a git repository' }, + ])); + process.exitCode = 1; + return; + } + + const result = await hookInstaller.uninstall(repoRoot); + + if (result.notLoreHook) { + console.error(formatter.formatError(1, [ + { severity: 'error', message: `Hook at ${result.hookPath} was not installed by Lore. Remove it manually if needed.` }, + ])); + process.exitCode = 1; + return; + } + + if (!result.removed) { + console.log(formatter.formatSuccess('No commit-msg hook found. Nothing to remove.')); + return; + } + + console.log(formatter.formatSuccess('Lore commit-msg hook removed.')); + }); +} diff --git a/src/commands/init.ts b/src/commands/init.ts index ec30e840..adceb5af 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -28,6 +28,12 @@ max_depth = 3 [cli] update_check = true + +[hooks] +enforce = true +allow_fixup = true +skip_authors = [] +skip_patterns = ["^Merge "] `; /** diff --git a/src/interfaces/hook-installer.ts b/src/interfaces/hook-installer.ts new file mode 100644 index 00000000..8e30ce35 --- /dev/null +++ b/src/interfaces/hook-installer.ts @@ -0,0 +1,17 @@ +export interface HookInstallResult { + readonly installed: boolean; + readonly hookPath: string; + readonly backedUp?: string; + readonly alreadyInstalled: boolean; +} + +export interface HookUninstallResult { + readonly removed: boolean; + readonly hookPath: string; + readonly notLoreHook: boolean; +} + +export interface IHookInstaller { + install(repoRoot: string, options: { force: boolean }): Promise; + uninstall(repoRoot: string): Promise; +} diff --git a/src/main.ts b/src/main.ts index c027262e..746366cc 100644 --- a/src/main.ts +++ b/src/main.ts @@ -44,6 +44,10 @@ import { registerCommitCommand } from './commands/commit.js'; import { registerValidateCommand } from './commands/validate.js'; import { registerSquashCommand } from './commands/squash.js'; import { registerDoctorCommand } from './commands/doctor.js'; +import { registerHooksCommand } from './commands/hooks.js'; + +import { HookScriptGenerator } from './services/hook-script-generator.js'; +import { HookInstaller } from './services/hook-installer.js'; import { LoreError, ValidationError } from './util/errors.js'; import { shouldCheckForUpdate } from './util/update-check.js'; @@ -197,6 +201,14 @@ async function main(): Promise { getFormatter, }); + const hookScriptGenerator = new HookScriptGenerator(); + const hookInstaller = new HookInstaller(hookScriptGenerator, config.hooks); + registerHooksCommand(program, { + hookInstaller, + gitClient, + getFormatter, + }); + // 7. Parse and run await program.parseAsync(process.argv); } diff --git a/src/services/config-loader.ts b/src/services/config-loader.ts index 3be7aafd..4920d226 100644 --- a/src/services/config-loader.ts +++ b/src/services/config-loader.ts @@ -30,6 +30,11 @@ const CAMEL_TO_SNAKE: Record> = { cli: { updateCheck: 'update_check', }, + hooks: { + allowFixup: 'allow_fixup', + skipAuthors: 'skip_authors', + skipPatterns: 'skip_patterns', + }, }; const VALID_OUTPUT_FORMATS = new Set(['text', 'json']); diff --git a/src/services/hook-installer.ts b/src/services/hook-installer.ts new file mode 100644 index 00000000..5e5cb3a9 --- /dev/null +++ b/src/services/hook-installer.ts @@ -0,0 +1,84 @@ +import { readFile, writeFile, mkdir, rename, unlink, chmod } from 'node:fs/promises'; +import { join } from 'node:path'; +import type { IHookInstaller, HookInstallResult, HookUninstallResult } from '../interfaces/hook-installer.js'; +import type { HookScriptGenerator } from './hook-script-generator.js'; +import type { LoreConfig } from '../types/config.js'; +import { LORE_MARKER } from './hook-script-generator.js'; + +/** + * Installs and uninstalls Lore-managed git hooks. + * + * GRASP: Pure Fabrication — filesystem infrastructure for hook management. + * SOLID: SRP — only manages hook file operations. Script generation is + * delegated to HookScriptGenerator. + */ +export class HookInstaller implements IHookInstaller { + constructor( + private readonly scriptGenerator: HookScriptGenerator, + private readonly hooksConfig: LoreConfig['hooks'], + ) {} + + async install(repoRoot: string, options: { force: boolean }): Promise { + const hooksDir = join(repoRoot, '.git', 'hooks'); + const hookPath = join(hooksDir, 'commit-msg'); + + await mkdir(hooksDir, { recursive: true }); + + let existingContent: string | null = null; + try { + existingContent = await readFile(hookPath, 'utf-8'); + } catch { + // No existing hook + } + + if (existingContent !== null) { + if (this.isLoreHook(existingContent)) { + // Lore-managed hook — overwrite silently (upgrade) + } else if (!options.force) { + // Non-Lore hook exists, no --force — refuse + return { installed: false, hookPath, alreadyInstalled: false }; + } else { + // Non-Lore hook exists with --force — back up then install + const backupPath = `${hookPath}.bak`; + await rename(hookPath, backupPath); + await this.writeHook(hookPath); + return { installed: true, hookPath, backedUp: backupPath, alreadyInstalled: false }; + } + } + + await this.writeHook(hookPath); + return { + installed: true, + hookPath, + alreadyInstalled: existingContent !== null && this.isLoreHook(existingContent), + }; + } + + async uninstall(repoRoot: string): Promise { + const hookPath = join(repoRoot, '.git', 'hooks', 'commit-msg'); + + let content: string; + try { + content = await readFile(hookPath, 'utf-8'); + } catch { + return { removed: false, hookPath, notLoreHook: false }; + } + + if (!this.isLoreHook(content)) { + return { removed: false, hookPath, notLoreHook: true }; + } + + await unlink(hookPath); + return { removed: true, hookPath, notLoreHook: false }; + } + + private async writeHook(hookPath: string): Promise { + const script = this.scriptGenerator.generateCommitMsgHook(this.hooksConfig); + await writeFile(hookPath, script, 'utf-8'); + await chmod(hookPath, 0o755); + } + + private isLoreHook(content: string): boolean { + return content.includes(LORE_MARKER); + } +} diff --git a/src/services/hook-script-generator.ts b/src/services/hook-script-generator.ts new file mode 100644 index 00000000..e6762112 --- /dev/null +++ b/src/services/hook-script-generator.ts @@ -0,0 +1,74 @@ +import type { LoreConfig } from '../types/config.js'; + +const LORE_MARKER = '# LORE-MANAGED-HOOK'; + +/** + * Generates shell script content for git hooks. + * + * GRASP: Information Expert — knows the structure of hook scripts. + * SOLID: SRP — only generates script strings, no filesystem ops. + */ +export class HookScriptGenerator { + generateCommitMsgHook(config: LoreConfig['hooks']): string { + const lines: string[] = [ + '#!/usr/bin/env bash', + `${LORE_MARKER} — do not edit manually. Reinstall with: lore hooks install`, + '', + 'COMMIT_MSG_FILE="$1"', + 'COMMIT_MSG=$(cat "$COMMIT_MSG_FILE")', + '', + ]; + + // Skip fixup/squash commits + if (config.allowFixup) { + lines.push( + '# Skip fixup/squash commits (validated after squash)', + 'if [[ "$COMMIT_MSG" =~ ^(fixup|squash)! ]]; then', + ' exit 0', + 'fi', + '', + ); + } + + // Skip patterns from config + for (const pattern of config.skipPatterns) { + lines.push( + `# Skip pattern: ${pattern}`, + `if [[ "$COMMIT_MSG" =~ ${pattern} ]]; then`, + ' exit 0', + 'fi', + '', + ); + } + + // Skip authors from config + if (config.skipAuthors.length > 0) { + lines.push('# Skip configured authors'); + lines.push('AUTHOR=$(git var GIT_AUTHOR_IDENT 2>/dev/null | sed \'s/>.*/>/\')'); + for (const author of config.skipAuthors) { + lines.push( + `if [[ "$AUTHOR" == *"${author}"* ]]; then`, + ' exit 0', + 'fi', + ); + } + lines.push(''); + } + + // Run validation + const strictFlag = config.enforce ? ' --strict' : ''; + lines.push( + '# Run Lore protocol validation', + 'if command -v lore &> /dev/null; then', + ` lore validate --last 1${strictFlag}`, + 'else', + ` npx --yes lore-protocol validate --last 1${strictFlag}`, + 'fi', + '', + ); + + return lines.join('\n'); + } +} + +export { LORE_MARKER }; diff --git a/src/types/config.ts b/src/types/config.ts index 14850d1d..a8cdadc4 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -24,6 +24,12 @@ export interface LoreConfig { readonly cli: { readonly updateCheck: boolean; }; + readonly hooks: { + readonly enforce: boolean; + readonly allowFixup: boolean; + readonly skipAuthors: readonly string[]; + readonly skipPatterns: readonly string[]; + }; } export const DEFAULT_CONFIG: LoreConfig = { @@ -34,4 +40,5 @@ export const DEFAULT_CONFIG: LoreConfig = { output: { defaultFormat: 'text' }, follow: { maxDepth: 3 }, cli: { updateCheck: true }, + hooks: { enforce: true, allowFixup: true, skipAuthors: [], skipPatterns: ['^Merge '] }, }; diff --git a/tests/unit/services/hook-installer.test.ts b/tests/unit/services/hook-installer.test.ts new file mode 100644 index 00000000..e4c276a6 --- /dev/null +++ b/tests/unit/services/hook-installer.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { HookInstaller } from '../../../src/services/hook-installer.js'; +import { HookScriptGenerator, LORE_MARKER } from '../../../src/services/hook-script-generator.js'; +import { DEFAULT_CONFIG } from '../../../src/types/config.js'; +import * as fs from 'node:fs/promises'; + +vi.mock('node:fs/promises'); + +describe('HookInstaller', () => { + let installer: HookInstaller; + const repoRoot = '/repo'; + const hookPath = '/repo/.git/hooks/commit-msg'; + + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(fs.mkdir).mockResolvedValue(undefined); + vi.mocked(fs.writeFile).mockResolvedValue(undefined); + vi.mocked(fs.chmod).mockResolvedValue(undefined); + vi.mocked(fs.rename).mockResolvedValue(undefined); + vi.mocked(fs.unlink).mockResolvedValue(undefined); + + const generator = new HookScriptGenerator(); + installer = new HookInstaller(generator, DEFAULT_CONFIG.hooks); + }); + + describe('install', () => { + it('installs hook when no existing hook exists', async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); + + const result = await installer.install(repoRoot, { force: false }); + + expect(result.installed).toBe(true); + expect(result.hookPath).toBe(hookPath); + expect(result.alreadyInstalled).toBe(false); + expect(fs.mkdir).toHaveBeenCalledWith('/repo/.git/hooks', { recursive: true }); + expect(fs.writeFile).toHaveBeenCalledWith(hookPath, expect.stringContaining(LORE_MARKER), 'utf-8'); + expect(fs.chmod).toHaveBeenCalledWith(hookPath, 0o755); + }); + + it('overwrites existing Lore-managed hook silently', async () => { + vi.mocked(fs.readFile).mockResolvedValue(`#!/bin/bash\n${LORE_MARKER}\nold content`); + + const result = await installer.install(repoRoot, { force: false }); + + expect(result.installed).toBe(true); + expect(result.alreadyInstalled).toBe(true); + expect(fs.rename).not.toHaveBeenCalled(); + }); + + it('refuses to overwrite non-Lore hook without --force', async () => { + vi.mocked(fs.readFile).mockResolvedValue('#!/bin/bash\nsome other hook'); + + const result = await installer.install(repoRoot, { force: false }); + + expect(result.installed).toBe(false); + expect(fs.writeFile).not.toHaveBeenCalled(); + }); + + it('backs up non-Lore hook and installs with --force', async () => { + vi.mocked(fs.readFile).mockResolvedValue('#!/bin/bash\nsome other hook'); + + const result = await installer.install(repoRoot, { force: true }); + + expect(result.installed).toBe(true); + expect(result.backedUp).toBe(`${hookPath}.bak`); + expect(fs.rename).toHaveBeenCalledWith(hookPath, `${hookPath}.bak`); + expect(fs.writeFile).toHaveBeenCalled(); + }); + + it('creates .git/hooks/ directory if missing', async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); + + await installer.install(repoRoot, { force: false }); + + expect(fs.mkdir).toHaveBeenCalledWith('/repo/.git/hooks', { recursive: true }); + }); + + it('sets executable permission on hook file', async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); + + await installer.install(repoRoot, { force: false }); + + expect(fs.chmod).toHaveBeenCalledWith(hookPath, 0o755); + }); + }); + + describe('uninstall', () => { + it('removes Lore-managed hook', async () => { + vi.mocked(fs.readFile).mockResolvedValue(`#!/bin/bash\n${LORE_MARKER}\ncontent`); + + const result = await installer.uninstall(repoRoot); + + expect(result.removed).toBe(true); + expect(fs.unlink).toHaveBeenCalledWith(hookPath); + }); + + it('refuses to remove non-Lore hook', async () => { + vi.mocked(fs.readFile).mockResolvedValue('#!/bin/bash\nsome other hook'); + + const result = await installer.uninstall(repoRoot); + + expect(result.removed).toBe(false); + expect(result.notLoreHook).toBe(true); + expect(fs.unlink).not.toHaveBeenCalled(); + }); + + it('handles missing hook gracefully', async () => { + vi.mocked(fs.readFile).mockRejectedValue(new Error('ENOENT')); + + const result = await installer.uninstall(repoRoot); + + expect(result.removed).toBe(false); + expect(result.notLoreHook).toBe(false); + }); + }); +}); diff --git a/tests/unit/services/hook-script-generator.test.ts b/tests/unit/services/hook-script-generator.test.ts new file mode 100644 index 00000000..da187999 --- /dev/null +++ b/tests/unit/services/hook-script-generator.test.ts @@ -0,0 +1,95 @@ +import { describe, it, expect } from 'vitest'; +import { HookScriptGenerator } from '../../../src/services/hook-script-generator.js'; +import { DEFAULT_CONFIG } from '../../../src/types/config.js'; + +describe('HookScriptGenerator', () => { + const generator = new HookScriptGenerator(); + + it('generates a valid bash script with shebang and marker', () => { + const script = generator.generateCommitMsgHook(DEFAULT_CONFIG.hooks); + + expect(script).toMatch(/^#!\/usr\/bin\/env bash/); + expect(script).toContain('LORE-MANAGED-HOOK'); + }); + + it('reads commit message from $1 argument', () => { + const script = generator.generateCommitMsgHook(DEFAULT_CONFIG.hooks); + + expect(script).toContain('COMMIT_MSG_FILE="$1"'); + expect(script).toContain('COMMIT_MSG=$(cat "$COMMIT_MSG_FILE")'); + }); + + it('includes lore validate call with --strict when enforce is true', () => { + const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, enforce: true }); + + expect(script).toContain('lore validate --last 1 --strict'); + }); + + it('omits --strict when enforce is false', () => { + const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, enforce: false }); + + expect(script).toContain('lore validate --last 1'); + expect(script).not.toContain('--strict'); + }); + + it('includes fixup/squash skip when allowFixup is true', () => { + const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, allowFixup: true }); + + expect(script).toContain('fixup|squash'); + expect(script).toContain('exit 0'); + }); + + it('omits fixup/squash skip when allowFixup is false', () => { + const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, allowFixup: false }); + + expect(script).not.toContain('fixup|squash'); + }); + + it('includes skip patterns from config', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipPatterns: ['^Merge ', '^Revert '], + }); + + expect(script).toContain('^Merge '); + expect(script).toContain('^Revert '); + }); + + it('includes skip authors from config', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipAuthors: ['dependabot[bot]', 'renovate[bot]'], + }); + + expect(script).toContain('dependabot[bot]'); + expect(script).toContain('renovate[bot]'); + expect(script).toContain('GIT_AUTHOR_IDENT'); + }); + + it('omits author check when skipAuthors is empty', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipAuthors: [], + }); + + expect(script).not.toContain('GIT_AUTHOR_IDENT'); + }); + + it('includes fallback from lore to npx', () => { + const script = generator.generateCommitMsgHook(DEFAULT_CONFIG.hooks); + + expect(script).toContain('command -v lore'); + expect(script).toContain('npx --yes lore-protocol'); + }); + + it('handles empty skipPatterns', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipPatterns: [], + }); + + expect(script).toContain('lore validate'); + // Should not have any skip pattern blocks + expect(script).not.toContain('# Skip pattern:'); + }); +}); From a60a2a4661291183dc57ab3e505687c330a351c6 Mon Sep 17 00:00:00 2001 From: Ivan Stetsenko Date: Mon, 11 May 2026 22:43:02 -0400 Subject: [PATCH 2/4] Fix review findings: shell injection, validate timing, DIP compliance Critical fixes: - Sanitize skipPatterns and skipAuthors before interpolating into bash script (prevents shell injection via shared .lore/config.toml) - Hook uses `lore validate --commit-msg-file` instead of `--last 1` (commit-msg hook fires before commit exists in git history) - Add --commit-msg-file flag to validate command for hook integration Major fixes: - HookInstaller depends on IHookScriptGenerator interface, not concrete class (DIP compliance per CLAUDE.md) - Extract resolveRepoRoot helper to DRY install/uninstall error handling - Pass trailerParser to validate command for commit message file parsing Tests updated for new script content and shell injection filtering. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/commands/hooks.ts | 48 ++++++++--------- src/commands/validate.ts | 51 +++++++++++++------ src/interfaces/hook-installer.ts | 6 +++ src/main.ts | 1 + src/services/hook-installer.ts | 5 +- src/services/hook-script-generator.ts | 25 +++++---- .../services/hook-script-generator.test.ts | 35 +++++++++---- 7 files changed, 106 insertions(+), 65 deletions(-) diff --git a/src/commands/hooks.ts b/src/commands/hooks.ts index df5012b0..4d8bd432 100644 --- a/src/commands/hooks.ts +++ b/src/commands/hooks.ts @@ -19,26 +19,31 @@ export function registerHooksCommand( .command('hooks') .description('Manage git hooks for Lore protocol enforcement'); + async function resolveRepoRoot(): Promise { + const { gitClient, getFormatter } = deps; + try { + return await gitClient.getRepoRoot(); + } catch { + const formatter = getFormatter(); + console.error(formatter.formatError(1, [ + { severity: 'error', message: 'Not inside a git repository' }, + ])); + process.exitCode = 1; + return null; + } + } + hooks .command('install') .description('Install commit-msg hook for Lore validation') .option('--force', 'Overwrite existing non-Lore hooks (backs up to .bak)') .action(async (options: { force?: boolean }) => { - const { hookInstaller, gitClient, getFormatter } = deps; - const formatter = getFormatter(); - - let repoRoot: string; - try { - repoRoot = await gitClient.getRepoRoot(); - } catch { - console.error(formatter.formatError(1, [ - { severity: 'error', message: 'Not inside a git repository' }, - ])); - process.exitCode = 1; - return; - } + const { hookInstaller, getFormatter } = deps; + const repoRoot = await resolveRepoRoot(); + if (!repoRoot) return; const result = await hookInstaller.install(repoRoot, { force: options.force ?? false }); + const formatter = getFormatter(); if (!result.installed) { console.error(formatter.formatError(1, [ @@ -63,21 +68,12 @@ export function registerHooksCommand( .command('uninstall') .description('Remove Lore-managed commit-msg hook') .action(async () => { - const { hookInstaller, gitClient, getFormatter } = deps; - const formatter = getFormatter(); - - let repoRoot: string; - try { - repoRoot = await gitClient.getRepoRoot(); - } catch { - console.error(formatter.formatError(1, [ - { severity: 'error', message: 'Not inside a git repository' }, - ])); - process.exitCode = 1; - return; - } + const { hookInstaller, getFormatter } = deps; + const repoRoot = await resolveRepoRoot(); + if (!repoRoot) return; const result = await hookInstaller.uninstall(repoRoot); + const formatter = getFormatter(); if (result.notLoreHook) { console.error(formatter.formatError(1, [ diff --git a/src/commands/validate.ts b/src/commands/validate.ts index cd369382..df218a05 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -1,13 +1,16 @@ import type { Command } from 'commander'; import type { Validator } from '../services/validator.js'; -import type { IGitClient } from '../interfaces/git-client.js'; +import type { IGitClient, RawCommit } from '../interfaces/git-client.js'; import type { IOutputFormatter } from '../interfaces/output-formatter.js'; +import type { TrailerParser } from '../services/trailer-parser.js'; import type { CommitValidationResult, FormattableValidationResult, ValidationIssue } from '../types/output.js'; +import { readFile } from 'node:fs/promises'; interface ValidateCommandOptions { readonly since?: string; readonly last?: number; readonly strict?: boolean; + readonly commitMsgFile?: string; } /** @@ -21,6 +24,7 @@ export function registerValidateCommand( deps: { validator: Validator; gitClient: IGitClient; + trailerParser: TrailerParser; getFormatter: () => IOutputFormatter; }, ): void { @@ -30,26 +34,41 @@ export function registerValidateCommand( .option('--since ', 'Validate all commits since ref (e.g., main)') .option('--last ', 'Validate the last N commits', parseInt) .option('--strict', 'Treat warnings as errors') + .option('--commit-msg-file ', 'Validate a commit message file (used by git hooks)') .action(async (range: string | undefined, options: ValidateCommandOptions) => { - const { validator, gitClient, getFormatter } = deps; + const { validator, gitClient, trailerParser, getFormatter } = deps; - // Determine the revision range - let logArgs: string[]; + let rawCommits: readonly RawCommit[]; - if (range) { - // Explicit range: e.g., HEAD~5..HEAD or main..feature - logArgs = [range]; - } else if (options.since) { - logArgs = [`${options.since}..HEAD`]; - } else if (options.last !== undefined && options.last > 0) { - logArgs = [`-${options.last}`]; + if (options.commitMsgFile) { + // Hook mode: validate a commit message file before the commit exists + const content = await readFile(options.commitMsgFile, 'utf-8'); + const trailerBlock = trailerParser.extractTrailerBlock(content); + const lines = content.split('\n'); + rawCommits = [{ + hash: '(pending)', + date: new Date().toISOString(), + author: '', + subject: lines[0] ?? '', + body: content, + trailers: trailerBlock, + }]; } else { - // Default: last commit - logArgs = ['-1']; - } + // Normal mode: validate commits from git history + let logArgs: string[]; + + if (range) { + logArgs = [range]; + } else if (options.since) { + logArgs = [`${options.since}..HEAD`]; + } else if (options.last !== undefined && options.last > 0) { + logArgs = [`-${options.last}`]; + } else { + logArgs = ['-1']; + } - // Get raw commits from git - const rawCommits = await gitClient.log(logArgs); + rawCommits = await gitClient.log(logArgs); + } // Validate all commits let results: readonly CommitValidationResult[] = await validator.validate(rawCommits); diff --git a/src/interfaces/hook-installer.ts b/src/interfaces/hook-installer.ts index 8e30ce35..20ae62f9 100644 --- a/src/interfaces/hook-installer.ts +++ b/src/interfaces/hook-installer.ts @@ -1,3 +1,9 @@ +import type { LoreConfig } from '../types/config.js'; + +export interface IHookScriptGenerator { + generateCommitMsgHook(config: LoreConfig['hooks']): string; +} + export interface HookInstallResult { readonly installed: boolean; readonly hookPath: string; diff --git a/src/main.ts b/src/main.ts index 746366cc..52f823d7 100644 --- a/src/main.ts +++ b/src/main.ts @@ -186,6 +186,7 @@ async function main(): Promise { registerValidateCommand(program, { validator, gitClient, + trailerParser, getFormatter, }); diff --git a/src/services/hook-installer.ts b/src/services/hook-installer.ts index 5e5cb3a9..96bbde76 100644 --- a/src/services/hook-installer.ts +++ b/src/services/hook-installer.ts @@ -1,7 +1,6 @@ import { readFile, writeFile, mkdir, rename, unlink, chmod } from 'node:fs/promises'; import { join } from 'node:path'; -import type { IHookInstaller, HookInstallResult, HookUninstallResult } from '../interfaces/hook-installer.js'; -import type { HookScriptGenerator } from './hook-script-generator.js'; +import type { IHookInstaller, IHookScriptGenerator, HookInstallResult, HookUninstallResult } from '../interfaces/hook-installer.js'; import type { LoreConfig } from '../types/config.js'; import { LORE_MARKER } from './hook-script-generator.js'; @@ -14,7 +13,7 @@ import { LORE_MARKER } from './hook-script-generator.js'; */ export class HookInstaller implements IHookInstaller { constructor( - private readonly scriptGenerator: HookScriptGenerator, + private readonly scriptGenerator: IHookScriptGenerator, private readonly hooksConfig: LoreConfig['hooks'], ) {} diff --git a/src/services/hook-script-generator.ts b/src/services/hook-script-generator.ts index e6762112..b491aabf 100644 --- a/src/services/hook-script-generator.ts +++ b/src/services/hook-script-generator.ts @@ -1,6 +1,9 @@ import type { LoreConfig } from '../types/config.js'; -const LORE_MARKER = '# LORE-MANAGED-HOOK'; +export const LORE_MARKER = '# LORE-MANAGED-HOOK'; + +const SAFE_PATTERN = /^[a-zA-Z0-9 ^$.*+?|[\]()\\/_-]+$/; +const SAFE_AUTHOR = /^[^"$`\\\n]+$/; /** * Generates shell script content for git hooks. @@ -30,8 +33,9 @@ export class HookScriptGenerator { ); } - // Skip patterns from config + // Skip patterns from config (validated for shell safety) for (const pattern of config.skipPatterns) { + if (!SAFE_PATTERN.test(pattern)) continue; lines.push( `# Skip pattern: ${pattern}`, `if [[ "$COMMIT_MSG" =~ ${pattern} ]]; then`, @@ -41,11 +45,12 @@ export class HookScriptGenerator { ); } - // Skip authors from config - if (config.skipAuthors.length > 0) { + // Skip authors from config (validated for shell safety) + const safeAuthors = config.skipAuthors.filter((a) => SAFE_AUTHOR.test(a)); + if (safeAuthors.length > 0) { lines.push('# Skip configured authors'); lines.push('AUTHOR=$(git var GIT_AUTHOR_IDENT 2>/dev/null | sed \'s/>.*/>/\')'); - for (const author of config.skipAuthors) { + for (const author of safeAuthors) { lines.push( `if [[ "$AUTHOR" == *"${author}"* ]]; then`, ' exit 0', @@ -55,14 +60,14 @@ export class HookScriptGenerator { lines.push(''); } - // Run validation + // Run validation against the commit message file const strictFlag = config.enforce ? ' --strict' : ''; lines.push( - '# Run Lore protocol validation', + '# Run Lore protocol validation against the commit message file', 'if command -v lore &> /dev/null; then', - ` lore validate --last 1${strictFlag}`, + ` lore validate --commit-msg-file "$COMMIT_MSG_FILE"${strictFlag}`, 'else', - ` npx --yes lore-protocol validate --last 1${strictFlag}`, + ` npx --yes lore-protocol validate --commit-msg-file "$COMMIT_MSG_FILE"${strictFlag}`, 'fi', '', ); @@ -70,5 +75,3 @@ export class HookScriptGenerator { return lines.join('\n'); } } - -export { LORE_MARKER }; diff --git a/tests/unit/services/hook-script-generator.test.ts b/tests/unit/services/hook-script-generator.test.ts index da187999..ee30aa72 100644 --- a/tests/unit/services/hook-script-generator.test.ts +++ b/tests/unit/services/hook-script-generator.test.ts @@ -19,16 +19,16 @@ describe('HookScriptGenerator', () => { expect(script).toContain('COMMIT_MSG=$(cat "$COMMIT_MSG_FILE")'); }); - it('includes lore validate call with --strict when enforce is true', () => { + it('validates via --commit-msg-file with --strict when enforce is true', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, enforce: true }); - expect(script).toContain('lore validate --last 1 --strict'); + expect(script).toContain('lore validate --commit-msg-file "$COMMIT_MSG_FILE" --strict'); }); - it('omits --strict when enforce is false', () => { + it('validates via --commit-msg-file without --strict when enforce is false', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, enforce: false }); - expect(script).toContain('lore validate --last 1'); + expect(script).toContain('lore validate --commit-msg-file "$COMMIT_MSG_FILE"'); expect(script).not.toContain('--strict'); }); @@ -36,7 +36,6 @@ describe('HookScriptGenerator', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, allowFixup: true }); expect(script).toContain('fixup|squash'); - expect(script).toContain('exit 0'); }); it('omits fixup/squash skip when allowFixup is false', () => { @@ -45,7 +44,7 @@ describe('HookScriptGenerator', () => { expect(script).not.toContain('fixup|squash'); }); - it('includes skip patterns from config', () => { + it('includes safe skip patterns from config', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, skipPatterns: ['^Merge ', '^Revert '], @@ -55,7 +54,17 @@ describe('HookScriptGenerator', () => { expect(script).toContain('^Revert '); }); - it('includes skip authors from config', () => { + it('filters out unsafe skip patterns', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipPatterns: ['^Merge ', '"]]; rm -rf / #'], + }); + + expect(script).toContain('^Merge '); + expect(script).not.toContain('rm -rf'); + }); + + it('includes safe skip authors from config', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, skipAuthors: ['dependabot[bot]', 'renovate[bot]'], @@ -66,6 +75,16 @@ describe('HookScriptGenerator', () => { expect(script).toContain('GIT_AUTHOR_IDENT'); }); + it('filters out unsafe skip authors', () => { + const script = generator.generateCommitMsgHook({ + ...DEFAULT_CONFIG.hooks, + skipAuthors: ['good-bot', 'evil"; rm -rf / #'], + }); + + expect(script).toContain('good-bot'); + expect(script).not.toContain('rm -rf'); + }); + it('omits author check when skipAuthors is empty', () => { const script = generator.generateCommitMsgHook({ ...DEFAULT_CONFIG.hooks, @@ -88,8 +107,6 @@ describe('HookScriptGenerator', () => { skipPatterns: [], }); - expect(script).toContain('lore validate'); - // Should not have any skip pattern blocks expect(script).not.toContain('# Skip pattern:'); }); }); From 088f928efb7e4d2d1813fb71403ee4b2bffbb431 Mon Sep 17 00:00:00 2001 From: Ivan Stetsenko Date: Mon, 11 May 2026 22:43:54 -0400 Subject: [PATCH 3/4] Fix .pre-commit-hooks.yaml: use --commit-msg-file instead of --last 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same bug as the generated hook script — commit-msg hook fires before the commit exists, so --last 1 validates the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) --- .pre-commit-hooks.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index e4bbc7fe..ff0734d5 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -1,7 +1,7 @@ - id: lore-validate name: Lore protocol validation description: Validate commit messages follow the Lore protocol - entry: lore validate --last 1 --strict + entry: lore validate --commit-msg-file language: node stages: [commit-msg] always_run: true From a4b791247db3e7732cb4d5a0cadeb80a15ac8e40 Mon Sep 17 00:00:00 2001 From: Ivan Stetsenko Date: Mon, 11 May 2026 22:59:26 -0400 Subject: [PATCH 4/4] Set lore-validate workflow to advisory mode for initial rollout Most existing commits don't have Lore trailers yet. The workflow runs validation but uses continue-on-error so PRs aren't blocked. Remove continue-on-error and add --strict once all contributors use lore commit. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/lore-validate.yml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/lore-validate.yml b/.github/workflows/lore-validate.yml index 9512fbbd..ba0d427a 100644 --- a/.github/workflows/lore-validate.yml +++ b/.github/workflows/lore-validate.yml @@ -7,6 +7,8 @@ on: jobs: validate: runs-on: ubuntu-latest + # Non-blocking initially — remove continue-on-error to enforce + continue-on-error: true steps: - uses: actions/checkout@v4 with: @@ -14,10 +16,14 @@ jobs: - uses: actions/setup-node@v4 with: - node-version: '18' + node-version: '20' - run: npm ci - run: npm run build - - name: Validate Lore commits - run: node ./bin/lore.js validate --since origin/main --strict + - name: Validate Lore commits in PR + run: | + echo "Validating commits since origin/main..." + node ./bin/lore.js validate --since origin/main || true + echo "" + echo "Note: Lore validation is advisory. To enforce, remove continue-on-error from the workflow."