v4#43
Draft
donatj wants to merge 45 commits into
Draft
Conversation
- Defers cmd.Wait() immediately after cmd.Start() to ensure process reaping - Combines Wait error with any stdin operation errors using multierror - Moves timeout timer setup before stdin operations for proper cleanup Previously early returns from Seek or Copy failures would skip cmd.Wait(), leaving zombie processes.
- Uses exec.CommandContext with context.WithTimeout for process lifecycle - Moves data.Seek before cmd.Start to avoid starting process on seek failure - Removes deferred stdin.Close in favor of explicit cleanup on error paths - Adds timeout exceeded detection with descriptive error message - Maintains multierror combination of stdin and wait errors Context-based cancellation is more idiomatic and ensures proper process cleanup on timeout.
…/copy-error paths Agent-Logs-Url: https://github.com/donatj/hookah/sessions/9c696600-1506-4502-9912-17bc6b4bb88d Co-authored-by: donatj <133747+donatj@users.noreply.github.com>
multierror.Append already handles nil cases
- Sets Setpgid to create process group for spawned scripts - Monitors context deadline and explicitly kills process group on timeout - Uses negative PID with SIGKILL to terminate all children Shell scripts spawn child processes that survive parent termination. CommandContext kills only the parent shell, leaving children like sleep running. Process groups allow killing the entire tree.
- Overrides cmd.Cancel to kill process group on context timeout - Removes manual select/goroutine timeout monitoring - Returns to simple cmd.Wait in defer CommandContext automatically calls cmd.Cancel on timeout. Overriding it to kill the process group is cleaner than manually monitoring ctx.Done.
- Guards cmd.Cancel against nil process - Ignores stdin.Close error after successful copy Child processes may exit early without reading stdin, causing close to fail with a broken pipe error.
- Switches from multierror.Append().ErrorOrNil() to native errors.Join - Completes migration to stdlib error handling
- Changes prefix from static string to function callback - Fixes trailing prefix bug when output ends with newline - Adds dynamic prefix test with incrementing counter - Renames 'started' field to 'needsPrefix' for clarity The prefix function is now called each time a prefix is needed, enabling use cases like timestamps or counters that change per line.
- Adds timestamp prefix to stdout and stderr logs - Aligns prefix and filename columns based on longest seen values - Uses relative paths for cleaner file display - Changes error handler prefix from suffix to prefix format Logs now display as ": TIMESTAMP PREFIX FILENAME (stream) >" with dynamic column width.
- Logs unexpected arguments to stderr - Displays usage help on invalid input - Exits with error code 1 Prevents silent acceptance of malformed command lines.
- Adds DisableLogPrefixes field to HookExec struct - Conditionally applies prefix writers based on flag - Updates TestEnvPopulatedCorrectly to disable prefixes Allows tests to verify raw output without timestamp and path prefixes.
- Moves defer after error check to prevent nil dereference - Removes redundant loop in symlink depth error message Resolves SA5001 and S1011 staticcheck warnings.
- Adds NewHookExec with functional options pattern - Moves exec to internal/exec package - Moves Logger interface to exec package - Fixes global state by moving counters to struct fields - Updates test paths for internal location
- Changes module path from v3 to v4 - Updates all import paths to v4
- Adds doc.go with package description
- Adds exec_unix.go with //go:build unix tag - Moves isExecFile from exec.go to exec_unix.go - Prevents compilation on Windows
- Adds WriterFactory type for custom writer creation per file - Adds internal/logging package with Formatter for log output - Removes formatting logic from exec package - Removes DisableLogPrefixes field and option - Adds WithWriterFactory option to HookExec - Updates server to use logging.Formatter with WriterFactory - Updates tests to use WriterFactory pattern Exec package now handles execution only. Server controls log formatting by providing a WriterFactory that wraps output streams with the logging.Formatter.
- Removes alignment state from Formatter (no mutex, no counters) - Changes WriterFactory signature to wrap existing stdout/stderr - Updates execFile to pass base writers to WriterFactory - Removes alignment test from formatter tests - Updates server to wrap provided writers instead of hardcoding os.Stdout Formatter is now stateless with simple space-separated output. WriterFactory wraps the writers HookExec already has instead of creating new ones, making it a true decorator pattern.
- Removes prefix parameter from execFile signature - Updates call sites to remove delivery and error prefix arguments - Updates tests to match new signature
- Adds owner and repo parameters to Formatter.Wrap - Updates format to include owner/repo after delivery ID - Updates server to pass login and repo to formatter - Updates test to verify owner/repo appears in output
- Removes Formatter abstraction from logging package - Moves format string construction directly into server WriterFactory - Server now builds log prefix inline with PrefixWriter - Removes formatter.go and formatter_test.go Format is now explicit in server code instead of hidden in Formatter. Makes customization trivial without modifying logging package.
- Adds -no-secret flag to explicitly disable signature verification - Requires -secret unless -no-secret is specified - Validates that both flags are not used together - Updates flag description to indicate requirement
- Adds makePrefix helper function that parameterizes stream name - Removes duplicate format string for stdout and stderr - Adds ghEvent to log output format
- Adds DisablePrefix field to HookServer - Adds ServerDisablePrefix option function - Makes WriterFactory application conditional on DisablePrefix flag - Simplifies error collection in NewHookServer using incremental errors.Join The -no-prefix flag allows disabling timestamp and delivery ID prefixes on hook script output for cleaner logs when prefixes are unnecessary.
- Updates badge links from v3 to v4 - Updates go install command to use v4 import path - Updates Go version requirement from 1.20+ to 1.26+
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| defer cancel() | ||
|
|
||
| cmd := exec.CommandContext(ctx, f) |
- Adds ErrPathTraversal error for checking with errors.Is - Validates owner, repo, event are non-empty - Validates all components using filepath.Clean - Rejects components containing path separators - Validates optional action parameter when present - Specifies rejected component name in error messages Prevents directory traversal attacks by ensuring path components cannot escape the intended directory hierarchy.
| files := []string{} | ||
| errHandlers := []string{} | ||
|
|
||
| fs, err := os.Stat(path) |
| } | ||
|
|
||
| if fs.IsDir() { | ||
| d, err := os.Open(path) |
- Tests rejection of slash, dot-dot, and dot in components - Tests empty component validation - Tests valid components with underscores, hyphens, numbers - Verifies ErrPathTraversal is returned via errors.Is - Improves validation to explicitly reject dot and dot-dot
- Extracts validation logic into reusable function - Adds allowEmpty parameter for optional components - Reduces code duplication between owner/repo/event and action validation
- Removes allowEmpty parameter from function signature - Checks action for empty explicitly in caller before validation - Reduces conditional logic in validation function
There was a problem hiding this comment.
Pull request overview
This PR bumps the module to v4 and refactors the core webhook execution/server logic into internal/ packages, while adding a configurable log-prefixing pipeline and tightening CLI security defaults around signature verification.
Changes:
- Moved hook execution and HTTP server code into
internal/execandinternal/server, with a functional-options constructor forHookExec. - Added a
WriterFactory+PrefixWriterto separate output formatting (timestamps, delivery ID, owner/repo) from execution, plus-no-prefixto disable prefixes. - Updated module path/versioning (
/v4) and CLI flags, including making-secretrequired by default with an explicit-no-secretopt-out.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates docs/badges/install instructions for /v4 and Go 1.26+ requirement. |
| go.mod | Bumps module path to github.com/donatj/hookah/v4 and updates Go version directive. |
| doc.go | Adds package-level documentation for hookah. |
| cmd/hookah/main.go | Switches to internal/server + internal/exec, adds -no-secret and -no-prefix, and enforces secret requirements. |
| internal/server/server.go | Uses new internal/exec and internal/logging patterns; adds log prefix wrapping via WriterFactory. |
| internal/logging/writer.go | Introduces PrefixWriter to prefix each output line with a dynamically generated string. |
| internal/logging/writer_test.go | Adds unit tests for PrefixWriter (static and dynamic prefixes). |
| internal/exec/exec.go | New HookExec implementation with functional options, traversal validation, process-group timeout handling, and WriterFactory integration. |
| internal/exec/exec_unix.go | Adds Unix build-tagged isExecFile implementation (executable detection + intended symlink following). |
| internal/exec/exec_test.go | Moves and extends exec-path/env tests; adds timeout and stdin-copy error path tests. |
| internal/exec/exec_traversal_test.go | Adds tests ensuring path traversal attempts are rejected. |
| internal/exec/exec_options_test.go | Adds tests for the NewHookExec functional-options constructor. |
| exec.go (deleted) | Removes old root-level exec implementation (migrated to internal/exec). |
| exec_test.go (deleted) | Removes old root-level exec tests (migrated to internal/exec). |
Comments suppressed due to low confidence (1)
cmd/hookah/main.go:94
getLoggerignores itsfilenameparameter and instead uses the global*errlogflag when opening the file. This will behave incorrectly ifgetLoggeris called with any value other than*errlog. Use thefilenameargument consistently.
func getLogger(filename string) exec.Logger {
if filename != "" {
f, err := os.OpenFile(*errlog, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
if err != nil {
log.Fatal(err)
}
return log.New(f, "", log.LstdFlags)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+42
| fs := fss[len(fss)-1] | ||
| fi, err := os.Stat(fs) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| mode := fi.Mode() | ||
| if mode.IsRegular() && mode|0111 == mode { | ||
| return true, nil | ||
| } | ||
|
|
||
| if mode&os.ModeSymlink != 0 { | ||
| link, err := os.Readlink(fi.Name()) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| fss = append(fss, link) | ||
| return isExecFile(fss...) | ||
| } |
Comment on lines
+198
to
+206
| for _, fi := range fi { | ||
| fpath := filepath.Join(path, fi.Name()) | ||
| is, err := isExecFile(fpath) | ||
| if err != nil { | ||
| return files, errHandlers, err | ||
| } | ||
|
|
||
| if is { | ||
| if strings.HasPrefix(fi.Name(), "@@error.") { |
| // data, | ||
| // WithInfoLog(logger), | ||
| // WithStdout(os.Stdout), | ||
| // WithDisableLogPrefixes(false), |
Comment on lines
+38
to
+72
| if pw.needsPrefix { | ||
| prefix := []byte(pw.prefixFunc()) | ||
| if _, err = pw.w.Write(prefix); err != nil { | ||
| return 0, err | ||
| } | ||
| pw.needsPrefix = false | ||
| } | ||
|
|
||
| n = len(p) | ||
| start := 0 | ||
|
|
||
| for i, b := range p { | ||
| if b == '\n' { | ||
| if _, err = pw.w.Write(p[start : i+1]); err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| start = i + 1 | ||
|
|
||
| if start < len(p) { | ||
| prefix := []byte(pw.prefixFunc()) | ||
| if _, err = pw.w.Write(prefix); err != nil { | ||
| return 0, err | ||
| } | ||
| } else { | ||
| pw.needsPrefix = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if start < len(p) { | ||
| if _, err = pw.w.Write(p[start:]); err != nil { | ||
| return 0, err | ||
| } | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| } | ||
|
|
||
| fs := fss[len(fss)-1] | ||
| fi, err := os.Stat(fs) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Replaces obsolete WithDisableLogPrefixes reference with WithWriterFactory in documentation example.
- Removes delivery from Exec function signature - Updates call sites in server and tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architectural
Logging
Security
Process Management
Code Quality