fix: runner correctness bugs, cleanup, tests, and architecture docs

CLAUDE.md

@@ -22,6 +22,8 @@ Toolchain is pinned in `mise.toml` (go 1.23.8, node 24, pnpm 11). `node`/`pnpm`
 
 ## Architecture
 
+See [docs/architecture.md](docs/architecture.md) for the request-pipeline diagram and package map.
+
 Entry: `cmd/forge/main.go` → `forge.Run(args)` in `internal/forge/app.go`, a flat switch that dispatches every subcommand (`init install uninstall doctor completion cache list ci validate run migrate update`). New subcommand = new case here.
 
 Packages under `internal/forge/`:

docs/architecture.md

@@ -0,0 +1,59 @@
+# Architecture
+
+How a git operation flows through forge, and what each package owns.
+
+## Request pipeline
+
+```mermaid
+flowchart TD
+    git["git commit / push / merge"] --> shim[".forge/hooks/<hook> shim"]
+    shim --> cli["forge run &lt;hook&gt;<br/>(cmd/forge → internal/forge/app.go)"]
+    cli --> run["runner.RunHookWithOptions"]
+
+    run --> root["git.DetectRepoRoot"]
+    run --> envf["load .git-hooks.env / .env"]
+    run --> skip{"SKIP_* env<br/>or merge/rebase<br/>in progress?"}
+    skip -->|yes| stop["skip hook"]
+    skip -->|no| load["config.LoadConfig<br/>(repo forge.toml + global merge)"]
+
+    load --> policy{"commit-msg /<br/>prepare-commit-msg?"}
+    policy -->|yes| pol["apply commit-message policy<br/>(conventional, ticket footer)"]
+    policy -->|no| ws{"workspace<br/>members match<br/>staged files?"}
+    ws -->|yes| member["run hook per member<br/>(member forge.toml)"]
+    ws -->|no| files["gather files<br/>(staged, or all-tracked for --all-files)"]
+
+    files --> mode{"parallel mode?"}
+    mode -->|no| seq["runHookCfg<br/>(sequential)"]
+    mode -->|yes| par["runHookCfgParallel<br/>(waves by depends_on)"]
+
+    seq --> tool
+    par --> tool
+
+    subgraph tool["per tool"]
+        filter["filter files<br/>(extensions, glob patterns)"] --> backend["backend.ResolveBackend<br/>host / ddev / docker"]
+        backend --> cache{"cache hit?"}
+        cache -->|yes| skiptool["skip (cached)"]
+        cache -->|no| exec["execute<br/>(timeout, env, check-mode)"]
+        exec --> after["on success:<br/>restage / stage_outputs / update cache"]
+    end
+```
+
+## Package map
+
+| Package | Responsibility |
+|---------|----------------|
+| `cmd/forge` | Binary entry point; calls `forge.Run(args)` |
+| `internal/forge` (`app.go`) | CLI dispatch for every subcommand; also `install`, `uninstall`, `doctor`, `validate`, `completion` |
+| `internal/forge/config` | Load/parse/merge `forge.toml`, presets, Husky migration, remote presets, global-config merge |
+| `internal/forge/runner` | Core execution: file filtering, sequential + parallel runners, commit-message policy, run cache, workspace routing |
+| `internal/forge/backend` | Execution abstraction — `HostBackend`, `DdevBackend`, `DockerBackend`; DDEV auto-detection |
+| `internal/forge/git` | Repo-root detection, staged/tracked file listing, `git add`, sequencer-state checks |
+| `internal/forge/ui` | Terminal output (colors, spinners) and the plain CI variant |
+| `internal/forge/update` | Self-update from GitHub releases |
+
+## Key invariants
+
+- **Tool order** follows declaration order in `forge.toml`. Go maps don't preserve order, so `config.parseHookToolOrder` regex-scans the raw TOML and feeds `OrderedToolNames()` — never rely on map iteration for ordering.
+- **Backend resolution precedence**: per-tool `backend` → `[execution].default_backend` → DDEV auto-detect (if the container is running) → host.
+- **Mutations happen only on success**: `restage` and `stage_outputs` run after a tool passes (never in `--check` mode); the run cache is only written for passing tools.
+- **Config precedence**: `FORGE_CONFIG` → repo `forge.toml`, with the global user config (`~/.config/forge/config.toml`) merged underneath — repo values always win.

internal/forge/config/config.go

@@ -72,7 +72,6 @@ type ToolConfig struct {
 	Restage           bool              `toml:"restage"`
 	OnFailure         string            `toml:"on_failure"`
 	Group             string            `toml:"group"`
-	When              string            `toml:"when"`
 	Timeout           string            `toml:"timeout"`
 	Cache             bool              `toml:"cache"`
 	CheckArgs         []string          `toml:"check_args"`

internal/forge/runner/review_fixes_test.go

@@ -0,0 +1,107 @@
+package runner
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/TerrorSquad/forge/internal/forge/config"
+	"github.com/TerrorSquad/forge/internal/forge/git"
+)
+
+// TestPrePushHonorsRunOptions is a regression test: the pre-push path used to
+// drop RunOptions (--tool/--skip-tool/--check/... were silently ignored).
+func TestPrePushHonorsRunOptions(t *testing.T) {
+	dir := initBareGitRepo(t)
+
+	tool := config.ToolConfig{
+		Command:   "false", // always exits 1
+		Type:      "system",
+		PassFiles: boolPtr(false),
+	}
+	cfg := config.HookConfig{
+		Enabled: boolPtr(true),
+		Tools:   map[string]config.ToolConfig{"always-fails": tool},
+	}
+
+	// Sanity: without a skip the failing tool must fail the hook.
+	if err := runHookCfgWithPushContext(dir, "pre-push", cfg, config.ExecutionConfig{}, PushContext{}, RunOptions{}); err == nil {
+		t.Fatal("expected failure when the tool runs and fails")
+	}
+
+	// With --skip-tool the opts must be honored, so the hook passes.
+	err := runHookCfgWithPushContext(dir, "pre-push", cfg, config.ExecutionConfig{}, PushContext{}, RunOptions{SkipTools: []string{"always-fails"}})
+	if err != nil {
+		t.Errorf("pre-push must honor RunOptions.SkipTools, got: %v", err)
+	}
+}
+
+// TestStageOutputsOnlyOnSuccess verifies stage_outputs are added to the index
+// when the tool succeeds, but not when it fails (mirrors restage semantics).
+func TestStageOutputsOnlyOnSuccess(t *testing.T) {
+	t.Run("failure does not stage", func(t *testing.T) {
+		dir := initBareGitRepo(t)
+		writeFile(t, filepath.Join(dir, "out.txt"), "generated\n")
+
+		tool := config.ToolConfig{
+			Command:      "false", // fails
+			Type:         "system",
+			PassFiles:    boolPtr(false),
+			StageOutputs: []string{"out.txt"},
+		}
+		cfg := config.HookConfig{Enabled: boolPtr(true), Tools: map[string]config.ToolConfig{"gen": tool}}
+		_ = runHookCfgWithPushContext(dir, "pre-push", cfg, config.ExecutionConfig{}, PushContext{}, RunOptions{})
+
+		if staged := stagedSet(t, dir); staged["out.txt"] {
+			t.Error("out.txt must NOT be staged when the tool fails")
+		}
+	})
+
+	t.Run("success stages", func(t *testing.T) {
+		dir := initBareGitRepo(t)
+		writeFile(t, filepath.Join(dir, "out.txt"), "generated\n")
+
+		tool := config.ToolConfig{
+			Command:      "true", // succeeds
+			Type:         "system",
+			PassFiles:    boolPtr(false),
+			StageOutputs: []string{"out.txt"},
+		}
+		cfg := config.HookConfig{Enabled: boolPtr(true), Tools: map[string]config.ToolConfig{"gen": tool}}
+		if err := runHookCfgWithPushContext(dir, "pre-push", cfg, config.ExecutionConfig{}, PushContext{}, RunOptions{}); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if staged := stagedSet(t, dir); !staged["out.txt"] {
+			t.Error("out.txt must be staged when the tool succeeds")
+		}
+	})
+}
+
+func stagedSet(t *testing.T, dir string) map[string]bool {
+	t.Helper()
+	files, err := git.StagedFiles(dir)
+	if err != nil {
+		t.Fatalf("StagedFiles: %v", err)
+	}
+	set := map[string]bool{}
+	for _, f := range files {
+		set[f] = true
+	}
+	return set
+}
+
+// TestParseAllowedGroups covers the HOOKS_ONLY parsing after the strings.Split
+// simplification (trimming, case-folding, empty entries).
+func TestParseAllowedGroups(t *testing.T) {
+	t.Setenv("HOOKS_ONLY", " Format , ,LINT ")
+	got := parseAllowedGroups()
+	if len(got) != 2 {
+		t.Fatalf("expected 2 groups, got %v", got)
+	}
+	if _, ok := got["format"]; !ok {
+		t.Error("expected lowercased 'format'")
+	}
+	if _, ok := got["lint"]; !ok {
+		t.Error("expected lowercased 'lint'")
+	}
+}

internal/forge/runner/runner.go

@@ -128,7 +128,7 @@ func RunHookWithOptions(hookName string, editFile string, opts RunOptions) error
 
 	if hookName == "pre-push" {
 		pushCtx := parsePushContext(os.Stdin)
-		return runHookCfgWithPushContext(repoRoot, hookName, hookCfg, cfg.Execution, pushCtx)
+		return runHookCfgWithPushContext(repoRoot, hookName, hookCfg, cfg.Execution, pushCtx, opts)
 	}
 
 	if hookName == "post-commit" {
@@ -266,7 +266,7 @@ func runHookCfg(root, hookName, editFile string, hookCfg config.HookConfig, exec
 		dur := time.Since(start)
 		ui.ClearRunning()
 
-		if !checkMode && len(tool.StageOutputs) > 0 {
+		if err == nil && !checkMode && len(tool.StageOutputs) > 0 {
 			_ = git.AddFiles(root, tool.StageOutputs)
 		}
 
@@ -583,29 +583,15 @@ func parseAllowedGroups() map[string]struct{} {
 		return nil
 	}
 	res := map[string]struct{}{}
-	s := bufio.NewScanner(strings.NewReader(raw))
-	s.Split(splitComma)
-	for s.Scan() {
-		v := strings.TrimSpace(strings.ToLower(s.Text()))
+	for _, part := range strings.Split(raw, ",") {
+		v := strings.TrimSpace(strings.ToLower(part))
 		if v != "" {
 			res[v] = struct{}{}
 		}
 	}
 	return res
 }
 
-func splitComma(data []byte, atEOF bool) (advance int, token []byte, err error) {
-	for i, b := range data {
-		if b == ',' {
-			return i + 1, data[:i], nil
-		}
-	}
-	if atEOF && len(data) > 0 {
-		return len(data), data, nil
-	}
-	return 0, nil, nil
-}
-
 func isHookSkippedEnv(hook string) bool {
 	key := "SKIP_" + strings.ToUpper(strings.ReplaceAll(hook, "-", ""))
 	return isTruthy(os.Getenv(key))
@@ -824,7 +810,7 @@ func parsePushContext(r io.Reader) PushContext {
 	return ctx
 }
 
-func runHookCfgWithPushContext(root, hookName string, hookCfg config.HookConfig, execCfg config.ExecutionConfig, ctx PushContext) error {
+func runHookCfgWithPushContext(root, hookName string, hookCfg config.HookConfig, execCfg config.ExecutionConfig, ctx PushContext, opts RunOptions) error {
 	if ctx.Remote != "" {
 		os.Setenv("FORGE_PUSH_REMOTE", ctx.Remote)
 	}
@@ -837,5 +823,5 @@ func runHookCfgWithPushContext(root, hookName string, hookCfg config.HookConfig,
 			os.Setenv("FORGE_PUSH_BRANCH", strings.TrimPrefix(ref, "refs/heads/"))
 		}
 	}
-	return runHookCfg(root, hookName, "", hookCfg, execCfg, nil, RunOptions{})
+	return runHookCfg(root, hookName, "", hookCfg, execCfg, nil, opts)
 }

internal/forge/runner/runner_parallel.go

@@ -133,7 +133,7 @@ func runHookCfgParallel(root, hookName string, hookCfg config.HookConfig, exec c
 
 		if !checkMode {
 			for _, pr := range waveResults {
-				if len(pr.tool.StageOutputs) > 0 {
+				if pr.err == nil && len(pr.tool.StageOutputs) > 0 {
 					_ = git.AddFiles(root, pr.tool.StageOutputs)
 				}
 			}

internal/forge/schema/forge.schema.json

@@ -158,10 +158,6 @@
           "description": "Logical group name shown in output (e.g. 'lint', 'format', 'analysis', 'test', 'artifacts'). No functional effect.",
           "examples": ["lint", "format", "analysis", "test", "refactor", "artifacts"]
         },
-        "when": {
-          "type": "string",
-          "description": "Conditional expression controlling when this tool runs."
-        },
         "timeout": {
           "type": "string",
           "description": "Per-tool timeout, e.g. '120s', '2m'. Overrides [execution].tool_timeout.",

website/reference/env.md

@@ -30,6 +30,12 @@ HOOKS_ONLY=format git commit -m "style: format"
 FORGE_CONFIG=configs/strict.toml git commit -m "feat: stricter checks"
 ```
 
+## Env files
+
+Before running a hook, forge loads `.git-hooks.env` and then `.env` from the repo root (if present). Each `KEY=value` line is applied **only when the variable isn't already set** in the environment, so your shell always wins. Lines starting with `#` are ignored, and surrounding single/double quotes are stripped.
+
+This lets a repo commit shared defaults — e.g. a `.git-hooks.env` with `HOOKS_ONLY=format` or `SKIP_PHPSTAN=1` — without every contributor exporting them by hand.
+
 ## Tool name matching
 
 `SKIP_<TOOL>` matches the **uppercase key** of the tool in `forge.toml`. For example, a tool keyed as `phpcs` is skipped with `SKIP_PHPCS=1`.