Repository navigation
Fix ambiguous explain-flag test assertion and build failure in interactive mode - #31
Merged
Merged
Conversation
Co-authored-by: kriserickson <325934+kriserickson@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] WIP to address feedback on previous PR for explain flag
Fix ambiguous explain-flag test assertion and build failure in interactive mode
Mar 1, 2026
kriserickson
approved these changes
Mar 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a vacuously-passing --explain test and resolves an interactive-mode build failure by ensuring explanation output is capturable via os.Stdout redirection and by plumbing the explain flag through interactive configuration.
Changes:
- Make executor explanation output write to the current
os.Stdoutso tests can capture it. - Strengthen
TestRunWithExplainTrueby using a non-overlapping command output and asserting on a unique explanation string. - Add a runtime-only
Config.Explainfield and propagate--explaininto interactive mode.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/executor/executor.go | Sends explanation output to os.Stdout (capturable in tests / redirectable). |
| internal/executor/executor_test.go | Removes ambiguous assertion by using unique explanation text and echo hello. |
| internal/config/config.go | Adds Config.Explain as a non-persisted runtime flag carrier. |
| cmd/root.go | Sets cfg.Explain before entering interactive mode so REPL execution respects --explain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
TestRunWithExplainTrueasserted on"explain-test", which also appeared in the command's own output (echo explain-test), making the test vacuously pass even with broken explanation logic. Additionally,fatih/color'sPrintfwrites to a package-levelOutputcaptured at init time, bypassingos.Stdoutredirects in tests entirely.Changes
internal/executor/executor.go: SwitchdimColor.Printf(...)→dimColor.Fprintf(os.Stdout, ...)so explanation output goes to the currentos.Stdout, making it capturable via pipe redirect in tests.internal/executor/executor_test.go: Use a command (echo hello) whose output cannot overlap with the explanation, and assert on a uniqueexplanationTextconstant:internal/config/config.go: AddExplain bool \toml:"-"`toConfig— runtime-only, not persisted — to resolve the build failure whererepl.goreferencedcfg.Explain` without the field existing.cmd/root.go: Setcfg.Explain = explainFlagbefore entering interactive mode so the flag propagates through the REPL path.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.