Merged
Conversation
Reviewer's GuideAdds a new Sequence diagram for lets self doc command executionsequenceDiagram
actor User
participant LetsCLI as lets_binary
participant CobraRoot as RootCommand
participant SelfCmd as SelfCommand
participant DocCmd as DocCommand
participant Util as UtilOpenURL
participant Browser as SystemBrowser
User->>LetsCLI: run lets self doc
LetsCLI->>CobraRoot: execute
CobraRoot->>SelfCmd: dispatch self
SelfCmd->>DocCmd: dispatch doc
DocCmd->>Util: OpenURL(letsDocsURL)
Util->>Browser: exec.Command(browserCommand, url)
Browser-->>Util: process started
Util-->>DocCmd: nil error
DocCmd-->>SelfCmd: nil error
SelfCmd-->>CobraRoot: nil error
CobraRoot-->>LetsCLI: exit code 0
LetsCLI-->>User: documentation opened in browser
Class diagram for new doc command and browser utilityclassDiagram
class CobraCommand {
+string Use
+string~slice~ Aliases
+string Short
+function Args(cmd CobraCommand, args string~slice~) error
+function RunE(cmd CobraCommand, args string~slice~) error
}
class DocCommandFactory {
+const letsDocsURL string
+initDocCommand(openURL func(string) error) *CobraCommand
}
class SelfCommandInitializer {
+openURL func(string) error
+InitSelfCmd(rootCmd *CobraCommand, version string)
}
class UtilBrowser {
+browserCommand(goos string, url string) (string, string~slice~, error)
+OpenURL(url string) error
}
SelfCommandInitializer --> CobraCommand : configures
SelfCommandInitializer --> DocCommandFactory : calls initDocCommand
DocCommandFactory --> CobraCommand : constructs
SelfCommandInitializer --> UtilBrowser : uses OpenURL via openURL var
UtilBrowser --> CobraCommand : used indirectly in command execution
Flow diagram for config error handling including self and doc commandsflowchart TD
A[Start lets CLI] --> B[Parse flags and commands]
B --> C{Current command name}
C --> D[completion/help/self/lsp/doc]
C --> E[Other command]
D --> F{Config error?}
F --> G[Do not fail on config error]
G --> H[Proceed with command]
E --> I{Config error?}
I --> J[Fail on config error]
J --> K[Exit with error]
I --> H
F --> H
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- The
openURLpackage-level variable inself.gois mutated in tests, which can be brittle and race-prone if tests are ever run in parallel; consider wiring the opener via function arguments/command construction (likeinitDocCommand(util.OpenURL)) and injecting a stub from tests instead of relying on a mutable global. - In
failOnConfigError, adding both"self"and"doc"torootCommandsmay not accurately modellets self doc(wherecurrent.Name()is just"doc"), and also pre-emptively disables config errors for a potential top-leveldoccommand; consider basing the decision on the full command path or parent command chain instead of justcurrent.Name().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `openURL` package-level variable in `self.go` is mutated in tests, which can be brittle and race-prone if tests are ever run in parallel; consider wiring the opener via function arguments/command construction (like `initDocCommand(util.OpenURL)`) and injecting a stub from tests instead of relying on a mutable global.
- In `failOnConfigError`, adding both `"self"` and `"doc"` to `rootCommands` may not accurately model `lets self doc` (where `current.Name()` is just `"doc"`), and also pre-emptively disables config errors for a potential top-level `doc` command; consider basing the decision on the full command path or parent command chain instead of just `current.Name()`.
## Individual Comments
### Comment 1
<location path="internal/util/browser_test.go" line_range="41-23" />
<code_context>
+ }
+ })
+
+ t.Run("unsupported", func(t *testing.T) {
+ _, _, err := browserCommand("windows", "https://lets-cli.org")
+ if err == nil {
+ t.Fatal("expected unsupported platform error")
+ }
+ })
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion on the error message for unsupported platforms
This subtest only checks that an error is returned. Please also assert that `err.Error()` mentions the OS (e.g. contains "windows") so the test enforces a descriptive, platform-specific error rather than allowing a generic one.
Suggested implementation:
```golang
t.Run("unsupported", func(t *testing.T) {
_, _, err := browserCommand("windows", "https://lets-cli.org")
if err == nil {
t.Fatal("expected unsupported platform error")
}
if !strings.Contains(err.Error(), "windows") {
t.Fatalf("expected error to mention platform \"windows\", got %q", err.Error())
}
})
}
```
Ensure that `strings` is imported at the top of `internal/util/browser_test.go`:
- Add `import "strings"` to the existing import block if it is not already present.
</issue_to_address>
### Comment 2
<location path="tests/no_lets_file.bats" line_range="55-57" />
<code_context>
+}
+
+@test "no_lets_file: lets self doc opens docs without config" {
+ fake_bin_dir="$(mktemp -d)"
+ opened_url_file="$(mktemp)"
+ rm -f "${opened_url_file}"
+
+ cat > "${fake_bin_dir}/xdg-open" <<'EOF'
</code_context>
<issue_to_address>
**nitpick (testing):** Use Bats teardown or traps to ensure temporary files/directories are cleaned up even when the test fails
`fake_bin_dir` and `opened_url_file` are only removed at the end of the test body, so a failing assertion will skip cleanup and leave temporary files/directories behind. Move this cleanup into a `teardown` function or a `trap` so it always runs, even on failure.
</issue_to_address>
### Comment 3
<location path="internal/util/browser.go" line_range="26" />
<code_context>
cmd := exec.Command(name, args...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/no_lets_file.bats
Outdated
Comment on lines
+55
to
+57
| fake_bin_dir="$(mktemp -d)" | ||
| opened_url_file="$(mktemp)" | ||
| rm -f "${opened_url_file}" |
There was a problem hiding this comment.
nitpick (testing): Use Bats teardown or traps to ensure temporary files/directories are cleaned up even when the test fails
fake_bin_dir and opened_url_file are only removed at the end of the test body, so a failing assertion will skip cleanup and leave temporary files/directories behind. Move this cleanup into a teardown function or a trap so it always runs, even on failure.
Contributor
Greptile SummaryThis PR introduces a Key changes and issues:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant main as cmd/lets/main.go
participant cobra as cobra (rootCmd)
participant self as self command
participant doc as doc command
participant browser as internal/util/browser.go
participant OS as OS process (xdg-open / open)
User->>main: lets self doc
main->>cobra: Traverse(["self","doc"])
cobra-->>main: current=doc command
main->>main: failOnConfigError()<br/>"doc" is in exclusion set → false
Note over main: config error suppressed
main->>cobra: ExecuteContext(ctx)
cobra->>self: route to self
self->>doc: route to doc
doc->>browser: openURL("https://lets-cli.org/docs/quick_start")
browser->>browser: browserCommand(runtime.GOOS, url)
alt darwin
browser->>OS: exec.Command("open", url).Start()
else linux
browser->>OS: exec.Command("xdg-open", url).Start()
else windows / other
browser-->>doc: error: unsupported platform
doc-->>User: "can not open documentation: unsupported platform"
end
OS->>OS: Process.Release() (fire-and-forget)
browser-->>doc: nil
doc-->>User: (silent success)
Last reviewed commit: "Add lets self doc co..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+54
to
+85
| @test "no_lets_file: lets self doc opens docs without config" { | ||
| fake_bin_dir="$(mktemp -d)" | ||
| opened_url_file="$(mktemp)" | ||
| rm -f "${opened_url_file}" | ||
|
|
||
| cat > "${fake_bin_dir}/xdg-open" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| printf "%s" "$1" > "${LETS_TEST_OPENED_URL_FILE}" | ||
| EOF | ||
| chmod +x "${fake_bin_dir}/xdg-open" | ||
|
|
||
| PATH="${fake_bin_dir}:${PATH}" \ | ||
| LETS_CONFIG=${NOT_EXISTED_LETS_FILE} \ | ||
| LETS_TEST_OPENED_URL_FILE="${opened_url_file}" \ | ||
| run lets self doc | ||
|
|
||
| assert_success | ||
|
|
||
| for _ in $(seq 1 20); do | ||
| if [[ -f "${opened_url_file}" ]]; then | ||
| break | ||
| fi | ||
| sleep 0.1 | ||
| done | ||
|
|
||
| run cat "${opened_url_file}" | ||
| assert_success | ||
| assert_output "https://lets-cli.org/docs/quick_start" | ||
|
|
||
| rm -rf "${fake_bin_dir}" | ||
| rm -f "${opened_url_file}" | ||
| } |
Contributor
There was a problem hiding this comment.
The test fakes only xdg-open, which is the binary chosen by OpenURL on Linux. On macOS, OpenURL calls open instead (see internal/util/browser.go). If this test is ever run in a macOS CI environment, the fake binary is never invoked, opened_url_file is never written, and the final cat assertion fails.
To make the test portable, create a fake for both launchers:
cat > "${fake_bin_dir}/xdg-open" <<'EOF'
#!/usr/bin/env bash
printf "%s" "$1" > "${LETS_TEST_OPENED_URL_FILE}"
EOF
chmod +x "${fake_bin_dir}/xdg-open"
# macOS uses `open`
cat > "${fake_bin_dir}/open" <<'EOF'
#!/usr/bin/env bash
printf "%s" "$1" > "${LETS_TEST_OPENED_URL_FILE}"
EOF
chmod +x "${fake_bin_dir}/open"Alternatively, add a platform guard and skip on unsupported platforms.
kindermax
force-pushed
the
codex/add-lets-self-doc-command
branch
from
1aad4c4 to
47eb6d5
Compare
kindermax
force-pushed
the
codex/add-lets-self-doc-command
branch
from
743952f to
efef308
Compare
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.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.
Summary
lets self docand hook it into the root self command so doc browsing is a valid built-in actiondocs/docs/changelog.mdand ensure config errors are suppressed during doc/help/lsp/self invocationsTesting
Summary by Sourcery
Add a new self documentation command that opens the online docs and ensure it works without requiring a config file or failing on config errors for doc-related commands.
New Features:
lets self doc(aliasdocs) command that opens the Lets online documentation in the default browser.Enhancements:
selfanddoccommands to bypass config errors similar to help, completion, and lsp commands.Documentation:
lets self doccommand in the changelog.Tests:
self doccommand behavior, including URL opening and error propagation.lets self docworks and opens the expected docs URL even when no config file is present.