feat: Inject AHOY_CMD and AHOY_COMMAND_NAME environment variables into subprocesses#170
Merged
Conversation
…o subprocesses Sets AHOY_CMD to the resolved path of the running ahoy binary and AHOY_COMMAND_NAME to the name of the invoked command, making both available to every subprocess ahoy runs. Adds a testdata fixture and BATS integration tests to verify the behaviour.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ChangesEnvironment variable injection, error handling, and circular import detection
GitHub Actions hardening and linting configuration
Sequence Diagram(s)sequenceDiagram
participant main
participant osExec as os.Executable()
participant setupApp
participant processImport
participant RunE as cobra RunE handler
participant subprocess as exec.Command.Run()
main->>osExec: capture binary path
osExec-->>main: ahoyExecutable
main->>setupApp: initialise importVisited, seed primary config path
setupApp->>processImport: process each import path
processImport->>processImport: check importVisited for cycle
processImport-->>setupApp: merged command map or "Circular import detected" log
setupApp->>RunE: build command with RunE
RunE->>subprocess: os.Environ() + AHOY_COMMAND_NAME + AHOY_CMD
subprocess-->>RunE: error or nil
RunE-->>setupApp: return error to cobra
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai There should be a configuration change to include you doing code reviews on the v3-candidate branch, please check your configuration |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🛠 Debug Information Gathered
To fix this, add reviews:
auto_review:
base_branches:
- v3-candidateThis field accepts regex patterns too, so if you want to cover all non-default branches you could use Once that change is merged into the default branch, CodeRabbit will automatically review PRs that target |
Member
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/ahoy.go`:
- Line 321: The current assignment to command.Env appends ahoyEnvVars before
envVars so user-provided environment entries can override injected values;
change the append order so envVars are appended first and ahoyEnvVars are
appended last (so injected AHOY_CMD / AHOY_COMMAND_NAME override any
duplicates), i.e., update the expression that builds command.Env which uses
command.Environ(), ahoyEnvVars and envVars to append envVars then ahoyEnvVars.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 850910d1-4739-42ea-8472-a116e8914154
📒 Files selected for processing (3)
v2/ahoy.gov2/testdata/ahoy-self-env.ahoy.ymlv2/tests/environment-variables.bats
Contributor
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
- $AHOY_CMD contains a path to the actual ahoy executable so it can be used for substitution for running ahoy-driven subcommands with the exact same binary. - $AHOY_COMMAND_NAME contains the name of the ahoy command (within the .ahoy.yml file) that is being run, and therefore allows the subprocess to know it is being run by an ahoy parent process. Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
…bles into subprocesses" This reverts commit 1f78216.
… files... ... don't override the injected runtime values. In Go's os/exec, the last occurrence of a duplicate environment variable key takes precedence, and we want that to be the ones set by ahoy itself. Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
yaml.v2 uses case-insensitive field matching as a fallback when no tags are present, but yaml.v3 (the current maintained version) requires exact matches. Without tags, migrating to yaml.v3 would silently leave AhoyAPI empty on every config file, causing spurious "unsupported API version" errors for all users. Add explicit yaml:"..." tags to every field on both Config and Command to make the structs safe for a future yaml.v3 migration.
…os.Exit os.Exit inside rootCmd.Execute() bypasses the pipe-drain cleanup in main(), which can cause in-flight subprocess stderr output to be silently truncated. Switching the command closure from Run to RunE and returning the subprocess error lets main() close the write-end of the pipe and wait for the drain goroutine to flush before exiting.
The block logged the sourcefile path and called os.Exit(0) whenever sourcefile was set, which in normal usage is always. This meant shell completion was printing the config file path instead of command names and exiting without running the pipe-drain cleanup in main(). Removing it lets the correct completion logic below run and avoids the os.Exit inside Execute().
… first
Previously getCommands() called logger("fatal") on the first invalid
command definition, meaning users had to fix errors one at a time with
no indication others existed. Each validation failure now logs as
[error] and continues, with a single [fatal] summary after all commands
are checked. Update BATS assertions to match the new output format.
getSubCommands() and getCommands() mutually recurse when processing imports. A circular reference (A imports B imports A) caused infinite recursion until the Go runtime panicked with a stack overflow. Track visited file paths in a package-level map reset by setupApp() at startup. The root config file is marked visited immediately so imports cannot loop back to it. When a circular reference is detected, a [warn] message is emitted and the file is skipped. Add a BATS test to verify no crash occurs and the warning is present. Also reset the map in the unit test that calls getSubCommands() directly.
os.Pipe() errors were silently discarded with _, leaving w as nil and causing a panic on the subsequent os.Stdout assignment. Add proper error handling that returns early and cleans up any partially created pipes. Add a deferred restore of os.Stdout/os.Stderr so they are recovered even if cmd.Execute() panics, preventing file descriptor leaks between tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ahoy.go (1)
385-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRunE currently suppresses actionable execution errors.
With
SilenceErrors=true, this path returns an error but only prints a blank line. Failures like “executable not found” can become effectively silent. Print the actual error (or at least non-exit errors) before returning.Suggested fix
- if err := command.Run(); err != nil { - fmt.Fprintln(os.Stderr) - return err - } + if err := command.Run(); err != nil { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + fmt.Fprintln(os.Stderr, err.Error()) + } + return err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ahoy.go` around lines 385 - 387, In the RunE function's error handling block where command.Run() is checked, the current code prints only a blank line to stderr instead of the actual error message. Replace the fmt.Fprintln(os.Stderr) call with a proper error print statement that outputs the error content (e.g., using fmt.Fprintf or fmt.Fprintln with the err variable) to ensure actionable execution errors like "executable not found" are visible to users before returning the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/zizmor.yml:
- Around line 14-20: The zizmor job currently grants only security-events: write
permission in its job-level permissions block, but the actions/checkout action
requires contents: read permission to access the repository. Since job-level
permissions override the workflow-level permissions rather than extending them,
add contents: read to the permissions block alongside the existing
security-events: write permission.
In `@ahoy.go`:
- Around line 188-196: The importVisited map persists as monotonic state across
the entire app run, causing files legitimately imported from different recursion
branches to be incorrectly flagged as circular imports. Replace this global
persistent map with stack-style tracking that marks imports as visited only for
the current recursion path: mark them as visited on function entry and unmark
them on function exit. Additionally, normalize all seeded and imported paths
consistently before checking or storing them in the tracking map to ensure
reliable comparison. This approach will prevent false positives for valid
multi-branch imports while still catching genuine circular dependencies within a
single path.
---
Outside diff comments:
In `@ahoy.go`:
- Around line 385-387: In the RunE function's error handling block where
command.Run() is checked, the current code prints only a blank line to stderr
instead of the actual error message. Replace the fmt.Fprintln(os.Stderr) call
with a proper error print statement that outputs the error content (e.g., using
fmt.Fprintf or fmt.Fprintln with the err variable) to ensure actionable
execution errors like "executable not found" are visible to users before
returning the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07aebd8e-3f65-41a9-bbe8-88437928ff27
📒 Files selected for processing (11)
.circleci/config.yml.coderabbit.yaml.github/dependabot.yml.github/workflows/build_and_test.yml.github/workflows/codeql.yml.github/workflows/release.yml.github/workflows/zizmor.yml.golangci.ymlahoy.goahoy_test.gotests/missing-cmd.bats
💤 Files with no reviewable changes (1)
- .circleci/config.yml
…versions and enable Zizmor scanning Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ahoy_test.go (1)
247-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose pipe readers to avoid descriptor leaks in test helper.
randrErrare not closed, so repeatedappRuncalls can leak file descriptors and destabilise long test runs.Suggested patch
r, w, pipeErr := os.Pipe() if pipeErr != nil { return "", fmt.Errorf("failed to create stdout pipe: %w", pipeErr) } +defer r.Close() rErr, wErr, pipeErr := os.Pipe() if pipeErr != nil { w.Close() r.Close() return "", fmt.Errorf("failed to create stderr pipe: %w", pipeErr) } +defer rErr.Close()Also applies to: 292-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ahoy_test.go` around lines 247 - 256, The pipe readers r and rErr created by os.Pipe() calls are never closed, causing file descriptor leaks in the appRun test helper function when called repeatedly. Add cleanup code to close both r and rErr before the function returns successfully. This should typically be done via defer statements immediately after each os.Pipe() call, or by ensuring both readers are closed in all code paths that lead to the function returning (both success and error paths). The defer approach is preferred to guarantee cleanup even if the function exits early through multiple return statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ahoy_test.go`:
- Around line 404-409: This test mutates global state including AhoyConf.srcDir
and the importVisited global variable without restoring them, which causes test
isolation issues. Save the original values of AhoyConf.srcDir, importVisited,
and any global logger output at the beginning of the test, then use defer or
t.Cleanup() to restore all these values after the test completes. Apply this
cleanup pattern to all locations in the test where these globals are modified,
ensuring that both the initial setup around line 404-409 and the modifications
around line 439-445 are properly cleaned up.
---
Outside diff comments:
In `@ahoy_test.go`:
- Around line 247-256: The pipe readers r and rErr created by os.Pipe() calls
are never closed, causing file descriptor leaks in the appRun test helper
function when called repeatedly. Add cleanup code to close both r and rErr
before the function returns successfully. This should typically be done via
defer statements immediately after each os.Pipe() call, or by ensuring both
readers are closed in all code paths that lead to the function returning (both
success and error paths). The defer approach is preferred to guarantee cleanup
even if the function exits early through multiple return statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62ab644e-801d-4cea-9e5f-5e265db1e099
📒 Files selected for processing (7)
.github/workflows/build_and_test.yml.github/workflows/codeql.yml.github/workflows/release.yml.github/workflows/zizmor.ymlahoy.goahoy_test.gov2/.golangci.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/zizmor.yml
- ahoy.go
- .github/workflows/release.yml
…bust Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
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
1 participant
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.
Sets AHOY_CMD to the resolved path of the running ahoy binary and AHOY_COMMAND_NAME to the name of the invoked command, making both available to every subprocess ahoy runs.
Adds a testdata fixture and BATS integration tests to verify the behaviour.
This is a fix for #165
Summary by CodeRabbit
Release Notes
New Features
AHOY_COMMAND_NAME(current command name) andAHOY_CMD(binary path) in the execution environment.Improvements
ahoy.ymlschema.Bug Fixes