From 954b0628ba8e4111b5bf5fb0333d6f0bdb576842 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 12 Jun 2026 13:26:30 +0000 Subject: [PATCH 1/2] Initial plan From 4c7875eeab855f69150c1e68259ced1e70d749d3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 12 Jun 2026 13:40:00 +0000 Subject: [PATCH 2/2] fix: clear stale repo-hooks-active markers in global dedup --- README.md | 2 + cmd/hookflow/cmd_test.go | 64 +++++++++++++++++++++++ cmd/hookflow/main.go | 84 ++++++++++++++++++++++--------- internal/session/sentinel.go | 21 ++++++++ internal/session/sentinel_test.go | 43 ++++++++++++++++ 5 files changed, 189 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 944bd9d..dfedb15 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,8 @@ Or just ask Copilot to create one — it knows the syntax from the installed ski When both exist, repo hooks run first and personal hooks automatically defer (via the `--global` flag). This means repo-specific workflows always take priority. +To prevent silent bypass after crashes/restarts, stale `repo-hooks-active` session markers are auto-cleared during `--global` execution. The stale threshold defaults to `24h` and can be configured with `HOOKFLOW_REPO_HOOKS_ACTIVE_STALE_THRESHOLD` (for example, `5m` or `1h`). + ### Test and share ```bash diff --git a/cmd/hookflow/cmd_test.go b/cmd/hookflow/cmd_test.go index 42fd2ba..0a3419d 100644 --- a/cmd/hookflow/cmd_test.go +++ b/cmd/hookflow/cmd_test.go @@ -10,6 +10,7 @@ import ( "runtime" "strings" "testing" + "time" eventpkg "github.com/htekdev/gh-hookflow/internal/event" "github.com/htekdev/gh-hookflow/internal/schema" @@ -4197,6 +4198,69 @@ t.Errorf("Expected allow when repo-hooks-active marker exists in global mode, go } } +// TestGlobalFlagClearsStaleRepoHooksMarker verifies stale markers do not keep +// bypassing global mode and are cleaned up automatically. +func TestGlobalFlagClearsStaleRepoHooksMarker(t *testing.T) { +tmpDir, err := os.MkdirTemp("", "hookflow-global-stale-*") +if err != nil { +t.Fatal(err) +} +defer func() { _ = os.RemoveAll(tmpDir) }() + +sessionDir := t.TempDir() +t.Setenv("HOOKFLOW_SESSION_DIR", sessionDir) +t.Setenv("HOOKFLOW_REPO_HOOKS_ACTIVE_STALE_THRESHOLD", "1h") + +// Create hookflows + hooks.json to satisfy global compliance and dedup checks. +workflowDir := filepath.Join(tmpDir, ".github", "hookflows") +if err := os.MkdirAll(workflowDir, 0755); err != nil { +t.Fatal(err) +} +workflow := "name: allow-all\non:\n file:\n paths: [\"**\"]\nsteps:\n - run: echo ok\n" +if err := os.WriteFile(filepath.Join(workflowDir, "allow.yml"), []byte(workflow), 0644); err != nil { +t.Fatal(err) +} +hooksDir := filepath.Join(tmpDir, ".github", "hooks") +if err := os.MkdirAll(hooksDir, 0755); err != nil { +t.Fatal(err) +} +hooksJSON := `{"version":1,"hooks":{"preToolUse":[{"bash":"gh hookflow run --raw --event-type preToolUse"}]}}` +if err := os.WriteFile(filepath.Join(hooksDir, "hooks.json"), []byte(hooksJSON), 0644); err != nil { +t.Fatal(err) +} + +// Create stale marker +markerPath := filepath.Join(sessionDir, "repo-hooks-active") +if err := os.WriteFile(markerPath, []byte(""), 0644); err != nil { +t.Fatal(err) +} +oldTime := time.Now().Add(-2 * time.Hour) +if err := os.Chtimes(markerPath, oldTime, oldTime); err != nil { +t.Fatal(err) +} + +oldStdout := os.Stdout +stdoutR, stdoutW, _ := os.Pipe() +os.Stdout = stdoutW + +escapedDir := strings.ReplaceAll(tmpDir, `\`, `\\`) +_ = runWithRawInput(tmpDir, `{"toolName":"create","toolArgs":{"path":"test.txt","file_text":"hello"},"cwd":"`+escapedDir+`"}`, "pre", true) + +_ = stdoutW.Close() +os.Stdout = oldStdout + +var buf bytes.Buffer +_, _ = buf.ReadFrom(stdoutR) +output := buf.String() + +if !strings.Contains(output, "allow") { +t.Errorf("Expected command to continue after stale marker cleanup, got: %s", output) +} +if _, statErr := os.Stat(markerPath); !os.IsNotExist(statErr) { +t.Error("Expected stale repo-hooks-active marker to be cleared") +} +} + // TestGlobalFlagProcessesWhenNoRepoHooks tests that --global mode processes events // when no repo-hooks-active marker exists (global-only mode). func TestGlobalFlagProcessesWhenNoRepoHooks(t *testing.T) { diff --git a/cmd/hookflow/main.go b/cmd/hookflow/main.go index 4d8c290..4f9588e 100644 --- a/cmd/hookflow/main.go +++ b/cmd/hookflow/main.go @@ -25,6 +25,11 @@ import ( var version = "0.1.0" +const ( + repoHooksActiveStaleThresholdEnv = "HOOKFLOW_REPO_HOOKS_ACTIVE_STALE_THRESHOLD" + defaultRepoHooksActiveStaleThreshold = 24 * time.Hour +) + func main() { // Initialize logging (errors are non-fatal) _ = logging.Init() @@ -429,6 +434,21 @@ func runWithRawInput(dir, inputStr, lifecycle string, global bool) error { if err != nil { log.Warn("failed to check repo-hooks-active: %v", err) } + if repoActive { + staleThreshold, thresholdErr := repoHooksActiveStaleThreshold() + if thresholdErr != nil { + log.Warn("invalid %s=%q, using default %s", repoHooksActiveStaleThresholdEnv, os.Getenv(repoHooksActiveStaleThresholdEnv), defaultRepoHooksActiveStaleThreshold) + staleThreshold = defaultRepoHooksActiveStaleThreshold + } + stale, age, staleErr := session.IsRepoHooksActiveStale(staleThreshold) + if staleErr != nil { + log.Warn("failed to evaluate repo-hooks-active staleness: %v", staleErr) + } else if stale { + log.Info("global mode: clearing stale repo-hooks-active marker for session %q (age=%s, threshold=%s)", raw.SessionID, age.Round(time.Second), staleThreshold) + _ = session.ClearRepoHooksActive() + repoActive = false + } + } if repoActive { // Verify repo hooks still exist — hooks.json may have been deleted mid-session repoHooksFile := filepath.Join(dir, ".github", "hooks", "hooks.json") @@ -570,6 +590,20 @@ func runWithRawInput(dir, inputStr, lifecycle string, global bool) error { return err } +func repoHooksActiveStaleThreshold() (time.Duration, error) { + rawThreshold := strings.TrimSpace(os.Getenv(repoHooksActiveStaleThresholdEnv)) + if rawThreshold == "" { + return defaultRepoHooksActiveStaleThreshold, nil + } + + threshold, err := time.ParseDuration(rawThreshold) + if err != nil || threshold <= 0 { + return 0, fmt.Errorf("invalid duration") + } + + return threshold, nil +} + // primitiveGuards performs critical safety checks on raw hook input before // any event detection or workflow matching. These are non-negotiable. func primitiveGuards(input []byte) *schema.WorkflowResult { @@ -901,7 +935,7 @@ func runMatchingWorkflowsWithEvent(dir string, evt *schema.Event, global bool) e func runMatchingWorkflows(dir, eventStr, lifecycle string) error { // Parse the event var eventData map[string]interface{} - + // Handle stdin input if eventStr == "-" { input, err := io.ReadAll(os.Stdin) @@ -910,34 +944,34 @@ func runMatchingWorkflows(dir, eventStr, lifecycle string) error { } eventStr = string(input) } - + if eventStr == "" { // No event provided, allow by default result := schema.NewAllowResult() return outputWorkflowResult(result) } - + if err := json.Unmarshal([]byte(eventStr), &eventData); err != nil { return fmt.Errorf("failed to parse event JSON: %w", err) } - + // Convert to Event struct event := parseEventData(eventData) - + // Normalize file path to be relative to dir (for matching against workflow patterns) if event.File != nil && event.File.Path != "" { event.File.Path = normalizeFilePath(event.File.Path, dir) } - + // Set lifecycle from CLI flag event.Lifecycle = lifecycle - + // Discover workflows using the discover package discoveredWFs, err := discover.Discover(dir) if err != nil { return fmt.Errorf("failed to discover workflows: %w", err) } - + if len(discoveredWFs) == 0 { // No workflows found, allow by default result := schema.NewAllowResult() @@ -948,7 +982,7 @@ func runMatchingWorkflows(dir, eventStr, lifecycle string) error { for _, wf := range discoveredWFs { workflowFiles = append(workflowFiles, wf.Path) } - + // Load and match workflows var matchingWorkflows []*schema.Workflow var hookifyResults []*schema.WorkflowResult @@ -983,20 +1017,20 @@ func runMatchingWorkflows(dir, eventStr, lifecycle string) error { // Skip invalid workflows continue } - + // Check if workflow matches the event matcher := trigger.NewMatcher(wf) if matcher.Match(event) { matchingWorkflows = append(matchingWorkflows, wf) } } - + if len(matchingWorkflows) == 0 && len(hookifyResults) == 0 { // No matching workflows or hookify rules, allow by default result := schema.NewAllowResult() return outputWorkflowResult(result) } - + // Aggregate results — deny wins var finalResult *schema.WorkflowResult var warnReasons []string @@ -1027,7 +1061,7 @@ func runMatchingWorkflows(dir, eventStr, lifecycle string) error { finalResult = result } } - + if finalResult == nil { finalResult = schema.NewAllowResult() } @@ -1043,7 +1077,7 @@ func runMatchingWorkflows(dir, eventStr, lifecycle string) error { // parseEventData converts raw event data to a schema.Event func parseEventData(data map[string]interface{}) *schema.Event { event := &schema.Event{} - + // Parse hook event if hookData, ok := data["hook"].(map[string]interface{}); ok { event.Hook = &schema.HookEvent{} @@ -1063,7 +1097,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { } } } - + // Parse tool event if toolData, ok := data["tool"].(map[string]interface{}); ok { event.Tool = &schema.ToolEvent{} @@ -1077,7 +1111,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { event.Tool.HookType = hookType } } - + // Parse file event if fileData, ok := data["file"].(map[string]interface{}); ok { event.File = &schema.FileEvent{} @@ -1091,7 +1125,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { event.File.Content = c } } - + // Parse commit event if commitData, ok := data["commit"].(map[string]interface{}); ok { event.Commit = &schema.CommitEvent{} @@ -1119,7 +1153,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { } } } - + // Parse push event if pushData, ok := data["push"].(map[string]interface{}); ok { event.Push = &schema.PushEvent{} @@ -1133,7 +1167,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { event.Push.After = after } } - + // Parse top-level cwd and timestamp if cwd, ok := data["cwd"].(string); ok { event.Cwd = cwd @@ -1141,7 +1175,7 @@ func parseEventData(data map[string]interface{}) *schema.Event { if ts, ok := data["timestamp"].(string); ok { event.Timestamp = ts } - + return event } @@ -1209,7 +1243,7 @@ func extractPushRef(command string, currentBranch string) string { if len(matches) >= 2 { return "refs/tags/" + matches[1] } - + // Default to current branch return "refs/heads/" + currentBranch } @@ -1372,24 +1406,24 @@ func normalizeFilePath(filePath, dir string) string { // Normalize path separators for cross-platform compatibility filePath = strings.ReplaceAll(filePath, "\\", "/") dir = strings.ReplaceAll(dir, "\\", "/") - + // Ensure dir ends with / if !strings.HasSuffix(dir, "/") { dir = dir + "/" } - + // If the file path starts with the dir, make it relative if strings.HasPrefix(filePath, dir) { return strings.TrimPrefix(filePath, dir) } - + // Also try case-insensitive match (Windows paths) lowerFilePath := strings.ToLower(filePath) lowerDir := strings.ToLower(dir) if strings.HasPrefix(lowerFilePath, lowerDir) { return filePath[len(dir):] } - + // Return as-is if not under dir return filePath } diff --git a/internal/session/sentinel.go b/internal/session/sentinel.go index c991097..c3757f4 100644 --- a/internal/session/sentinel.go +++ b/internal/session/sentinel.go @@ -3,6 +3,7 @@ package session import ( "os" "path/filepath" + "time" ) const repoHooksActiveFileName = "repo-hooks-active" @@ -58,3 +59,23 @@ func IsRepoHooksActive() (bool, error) { } return false, err } + +// IsRepoHooksActiveStale returns whether the repo-hooks-active marker is older +// than threshold. If the marker does not exist, stale is false. +func IsRepoHooksActiveStale(threshold time.Duration) (stale bool, age time.Duration, err error) { + path, err := repoHooksActivePath() + if err != nil { + return false, 0, err + } + + info, err := os.Stat(path) + if os.IsNotExist(err) { + return false, 0, nil + } + if err != nil { + return false, 0, err + } + + age = time.Since(info.ModTime()) + return age > threshold, age, nil +} diff --git a/internal/session/sentinel_test.go b/internal/session/sentinel_test.go index da05ca2..6d1d878 100644 --- a/internal/session/sentinel_test.go +++ b/internal/session/sentinel_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" ) func TestMarkRepoHooksActive(t *testing.T) { @@ -113,3 +114,45 @@ func TestClearRepoHooksActive_NoMarker(t *testing.T) { t.Fatalf("ClearRepoHooksActive() should not error when no marker: %v", err) } } + +func TestIsRepoHooksActiveStale(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOOKFLOW_SESSION_DIR", dir) + + if err := MarkRepoHooksActive(); err != nil { + t.Fatalf("MarkRepoHooksActive() error: %v", err) + } + + markerPath := filepath.Join(dir, repoHooksActiveFileName) + oldTime := time.Now().Add(-2 * time.Hour) + if err := os.Chtimes(markerPath, oldTime, oldTime); err != nil { + t.Fatalf("os.Chtimes() error: %v", err) + } + + stale, age, err := IsRepoHooksActiveStale(time.Hour) + if err != nil { + t.Fatalf("IsRepoHooksActiveStale() error: %v", err) + } + if !stale { + t.Fatal("expected marker to be stale") + } + if age <= time.Hour { + t.Fatalf("expected age > 1h, got %v", age) + } +} + +func TestIsRepoHooksActiveStale_NoMarker(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOOKFLOW_SESSION_DIR", dir) + + stale, age, err := IsRepoHooksActiveStale(time.Hour) + if err != nil { + t.Fatalf("IsRepoHooksActiveStale() error: %v", err) + } + if stale { + t.Fatal("expected marker to not be stale when marker does not exist") + } + if age != 0 { + t.Fatalf("expected age 0 when marker does not exist, got %v", age) + } +}