Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the Bash Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "Cobra CLI\n(cmd/root.go)"
participant Prompts as "Prompts\n(internal/prompts)"
participant Config as "Config\n(internal/config)"
participant Files as "Files\n(internal/files)"
participant Repo as "Repo\n(internal/repo)"
participant GitHub as "GitHub / gh"
User->>CLI: run gitstart [flags]
activate CLI
CLI->>CLI: PreRunE validate flags
alt dry-run
CLI->>Prompts: DryRunPrompt(action)
Prompts-->>CLI: preview output
else normal flow
CLI->>Config: LoadUsername()
Config-->>CLI: username
CLI->>Prompts: PromptUser / PromptSelect
Prompts-->>CLI: inputs (dir, lang, branch, ...)
CLI->>Files: CreateProjectDir(dir)
Files-->>CLI: dir created
CLI->>Files: FetchGitignore(lang, dest)
Files->>GitHub: GET raw gitignore
GitHub-->>Files: template
Files-->>CLI: .gitignore written
CLI->>Files: FetchLicenseText(license, dest)
Files->>GitHub: GET /licenses/<slug>
GitHub-->>Files: license body
Files-->>CLI: LICENSE written
CLI->>Files: CreateReadme(name, desc, dest)
Files-->>CLI: README.md written
CLI->>Repo: InitGitRepo(dir)
Repo-->>CLI: .git created
CLI->>Repo: CommitAndPush(dir, branch, message)
Repo->>GitHub: git push / gh repo create
Repo-->>CLI: pushed / remote configured
end
CLI-->>User: success / exit
deactivate CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
docs/README.md-287-292 (1)
287-292:⚠️ Potential issue | 🟡 MinorReferences to
planning/will be broken — directory is gitignored.Lines 288 and 292 reference
planning/implementation_plan.md, but theplanning/directory is in.gitignoreand won't be available in the repository. Either:
- Remove the
planning/directory from.gitignoreif these docs should be public- Remove or update these references to point to available documentation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 287 - 292, The README contains links to planning/implementation_plan.md that will be broken because the planning/ directory is gitignored; either stop ignoring the planning/ directory so the file is committed (remove the planning/ entry from .gitignore) or update the README references (e.g., in the "Interactive prompts" and "Migration Notes" sections that reference planning/implementation_plan.md) to point to an existing public doc or remove the link entirely; ensure you update the exact string "planning/implementation_plan.md" in README.md (and any other files referencing it) to reflect the chosen fix.docs/README.md-59-66 (1)
59-66:⚠️ Potential issue | 🟡 MinorIncorrect
go installpath.The module path in
go.modisgithub.com/shinichiokada/gitstart, but the documentation referencesgithub.com/shinokada/gitstart. Also, sincemain.gois at the module root (not incmd/gitstart), the correct install command should be:go install github.com/shinichiokada/gitstart@latest📝 Proposed fix
### Install via Go You can install the Go version directly with: ```sh -go install github.com/shinokada/gitstart/cmd/gitstart@latest +go install github.com/shinichiokada/gitstart@latest</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/README.mdaround lines 59 - 66, The README's Go install example uses
the wrong module path; update the command in the "Install via Go" section to
match the module root where main.go lives and the module name in go.mod by
replacing the incorrect "go install
github.com/shinokada/gitstart/cmd/gitstart@latest" entry with "go install
github.com/shinichiokada/gitstart@latest" so the install points to the module
root (refer to the "Install via Go" block and the example command string).</details> </blockquote></details> <details> <summary>cmd/root.go-63-65 (1)</summary><blockquote> `63-65`: _⚠️ Potential issue_ | _🟡 Minor_ **Write execution errors to stderr, not stdout.** CLI errors should go to stderr to keep stdout clean for pipes/scripts. <details> <summary>💡 Proposed fix</summary> ```diff func Execute() { if err := rootCmd.Execute(); err != nil { - fmt.Println(err) + fmt.Fprintln(os.Stderr, err) os.Exit(1) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 63 - 65, The CLI currently prints execution errors to stdout using fmt.Println in the rootCmd.Execute error branch; change this to write to stderr by using fmt.Fprintln with os.Stderr so execution error messages from rootCmd.Execute() go to stderr (locate the rootCmd.Execute error handling in cmd/root.go and replace fmt.Println(err) with writing the error to os.Stderr).internal/repo/repo_github_test.go-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorAvoid empty placeholder tests that always pass.
This currently adds no coverage. Mark it as skipped (or convert to gated integration test) so intent is explicit.
💡 Proposed fix
func TestCreateGitHubRepo(t *testing.T) { - // This test is a placeholder; actual repo creation requires gh CLI and network access. + t.Skip("integration test: requires gh CLI auth and network access") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo_github_test.go` around lines 5 - 7, The TestCreateGitHubRepo test is an empty placeholder that always passes; update the TestCreateGitHubRepo function to explicitly skip the test (e.g., call t.Skip or t.Skipf with a short explanatory message) or convert it into a gated integration test (add a build tag like // +build integration and check for an env var) so intent is explicit and it doesn’t silently add coverage; locate the TestCreateGitHubRepo function and add the skip or gating logic accordingly.internal/prompts/prompts_test.go-7-19 (1)
7-19:⚠️ Potential issue | 🟡 MinorCurrent tests pass without verifying behavior.
Line 9 only executes code, and Lines 13-19 are empty tests. These will stay green even if prompt behavior regresses. Add assertions (with mocked I/O) or explicitly
t.Skip(...)placeholders to avoid false confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts_test.go` around lines 7 - 19, TestDryRunPrompt currently only calls DryRunPrompt and TestPromptUser/TestPromptSelect are empty placeholders, which yields false positives; update TestDryRunPrompt to assert behavior (e.g., capture/mocking stdout/stderr or check returned values) rather than just calling DryRunPrompt, and either implement mocked I/O-based assertions for interactive functions PromptUser and PromptSelect or mark those tests with t.Skip("interactive; requires manual testing or mocking") so they don't silently pass; use the function names DryRunPrompt, TestDryRunPrompt, TestPromptUser, and TestPromptSelect to locate and modify the tests.
🧹 Nitpick comments (10)
internal/utils/utils_test.go (1)
10-13: Assert the returned error is the original error, not just non-nil.At Line 10-Line 13, checking only non-nil is too weak; this should verify that
ErrorHandlerreturns the same error.Proposed test improvement
result := ErrorHandler(err, "test message") - if result == nil { - t.Fatalf("expected error to be returned") + if result != err { + t.Fatalf("expected original error to be returned, got: %v", result) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/utils_test.go` around lines 10 - 13, The test currently only checks that ErrorHandler(err, "test message") returns non-nil; change the assertion to verify the returned error is the original error (err) by comparing result to err (or using errors.Is(result, err)) so the test ensures ErrorHandler returns the same error value; update the assertion around result and err in the Test in internal/utils/utils_test.go accordingly.internal/files/files_test.go (1)
8-27: Consider usingt.TempDir()for test isolation.The hardcoded
"test_tmp_dir"path can cause issues with parallel tests and may leave artifacts if the test panics. Additionally, the cleanupdeferwill fail ifCreateProjectDirnever created the directory.Using
t.TempDir()provides automatic cleanup, unique paths, and better isolation.♻️ Proposed refactor using t.TempDir()
func TestCreateProjectDir(t *testing.T) { - tmpDir := "test_tmp_dir" - defer func() { - if err := os.RemoveAll(tmpDir); err != nil { - t.Fatalf("failed to remove temp dir: %v", err) - } - }() + // Use t.TempDir() for automatic cleanup + baseDir := t.TempDir() + tmpDir := baseDir + "/test_project" err := CreateProjectDir(tmpDir) if err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/files/files_test.go` around lines 8 - 27, Replace the hardcoded tmpDir with t.TempDir() in TestCreateProjectDir: call base := t.TempDir() and pass a unique subpath (e.g. filepath.Join(base, "proj")) to CreateProjectDir so the test still validates directory creation; remove the manual defer os.RemoveAll cleanup and adjust assertions to use the new variable (references: TestCreateProjectDir, CreateProjectDir, tmpDir).internal/files/readme_test.go (1)
8-28: Consider usingt.TempDir()for test isolation and verify content.Similar to
files_test.go, using a hardcoded path in the working directory can cause issues. The cleanup will also fail ifCreateReadmefails.Additionally, verifying only
Size() > 0doesn't confirm correct content generation.♻️ Proposed refactor
func TestCreateReadme(t *testing.T) { - file := "README_test.md" - defer func() { - if err := os.Remove(file); err != nil { - t.Fatalf("failed to remove test README file: %v", err) - } - }() + file := t.TempDir() + "/README_test.md" err := CreateReadme("TestProject", "Test description.", file) if err != nil { t.Fatalf("expected no error, got %v", err) } - info, err := os.Stat(file) + content, err := os.ReadFile(file) if err != nil { - t.Fatalf("expected README file to exist, got error: %v", err) + t.Fatalf("expected README file to exist: %v", err) } - if info.Size() == 0 { - t.Fatalf("expected README file to have content") + expected := "# TestProject\n\nTest description.\n" + if string(content) != expected { + t.Fatalf("unexpected content: got %q, want %q", string(content), expected) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/files/readme_test.go` around lines 8 - 28, TestCreateReadme uses a hardcoded filename and only checks Size()>0; change it to use t.TempDir() for isolation and remove manual os.Remove cleanup, call CreateReadme with a path inside that temp dir (e.g., filepath.Join(tempDir, "README_test.md")), assert CreateReadme returns nil, then read the file contents and verify it contains the project name and description (e.g., check for "TestProject" and "Test description.") instead of just checking size; update references to the CreateReadme call and file variable in readme_test.go accordingly.internal/repo/repo_commit_test.go (1)
9-15: Uset.TempDir()instead of a fixed directory name.Line 9 uses a shared path (
test_commit_repo), which can collide across runs and parallel test execution.♻️ Suggested refactor
- dir := "test_commit_repo" - if err := os.MkdirAll(dir, 0755); err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - defer func() { - _ = os.RemoveAll(dir) - }() + dir := t.TempDir()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo_commit_test.go` around lines 9 - 15, Replace the fixed directory creation with the testing helper: instead of setting dir := "test_commit_repo" and calling os.MkdirAll/deferring os.RemoveAll, use t.TempDir() to obtain an isolated temporary directory for the test; update any uses of the variable dir in this test (in internal/repo/repo_commit_test.go) to rely on the returned t.TempDir() value and remove the explicit cleanup code that calls os.RemoveAll.Makefile (1)
1-1: Consider adding a phonyalltarget alias.Static linting expects an
allphony target. Adding it improves consistency with common Make conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add a phony all target and declare it in the existing .PHONY list so static linting finds it: update the .PHONY line (which currently lists build test clean ...) to include all, and add an "all:" target that forwards to the primary build target (e.g., "all: build" or "all: default-target") so make all behaves as the conventional alias for building the project.internal/prompts/prompts.go (1)
16-39: Avoid swallowing stdin/parse errors in prompt helpers.Line 19 discards
ReadStringerrors, and Lines 31-34 map scan failures to"". This makes I/O failure indistinguishable from user intent. Prefer returning(string, error)so callers can retry or abort explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts.go` around lines 16 - 39, PromptUser and PromptSelect currently swallow I/O and parse errors (discarding ReadString error and returning empty string on Scanln failures), so change both functions (PromptUser and PromptSelect) to return (string, error), propagate and return any error from reader.ReadString (trimmed) in PromptUser, and in PromptSelect capture fmt.Scanln errors and return an explicit error when scanning or when the numeric choice is out of range; update callers to handle the returned error so they can retry or abort instead of treating "" as user input.bash/Makefile (1)
1-1: Expand.PHONYto include all declared targets.Line 1 omits many targets defined later (
check-deps,lint,format,install,uninstall,test-all,watch,coverage,pre-commit,release, etc.), which can cause accidental no-op behavior if same-named files exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bash/Makefile` at line 1, Update the .PHONY declaration so it lists every declared Makefile target to avoid accidental no-op behavior; specifically add targets such as check-deps, lint, format, install, uninstall, test-all, watch, coverage, pre-commit, release (and any other targets present) to the existing .PHONY line so the current ".PHONY: help test test-unit test-integration test-shellcheck install-deps clean" entry includes all target names..github/workflows/release.yml (1)
34-37: Pin GoReleaser version instead oflatest.Line 37 uses a floating version, which makes releases non-reproducible and vulnerable to upstream breaking changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 34 - 37, The workflow currently uses a floating GoReleaser version ("with: version: latest") in the "Install GoReleaser" step (goreleaser/goreleaser-action@v5); replace the floating value with a pinned, explicit release tag or semver (e.g., a tested goreleaser release) to ensure reproducible builds and guard against upstream breakage—update the "with: version" value to that specific version and commit.internal/config/config.go (2)
35-39: Consider trimming whitespace from loaded username.If the config file is edited manually or contains a trailing newline, the returned username will include that whitespace, which could cause subtle issues with API calls or display.
🔧 Proposed fix
+import ( + "os" + "path/filepath" + "strings" +)data, err := os.ReadFile(path) if err != nil { return "", err } - return string(data), nil + return strings.TrimSpace(string(data)), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 35 - 39, The config loader returns the raw file contents (string(data)) which can include trailing newlines or whitespace; modify the return to trim whitespace before returning by applying strings.TrimSpace to the file contents (e.g., trim the result of string(data) in the same function that calls os.ReadFile) so the username has no leading/trailing whitespace while keeping the existing error handling.
18-26: Consider validating empty username.If an empty string is passed, an empty config file is written, which could cause confusing behavior when loaded later.
🔧 Proposed fix
func SaveUsername(username string) error { + if username == "" { + return fmt.Errorf("username cannot be empty") + } path, err := ConfigFilePath()Note: Requires adding
"fmt"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 18 - 26, The SaveUsername function currently writes an empty string to the config file when username == "", so add a validation at the start of SaveUsername to return an error (e.g., fmt.Errorf("username cannot be empty")) if username is empty, before calling ConfigFilePath(), ensuring you also add "fmt" to the imports; keep the rest of the directory creation and os.WriteFile logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 9-12: The release job currently lacks explicit workflow
permissions; add a permissions block on the release job (jobs.release) granting
at minimum contents: write (to create releases via GoReleaser) and packages:
write (to push to GHCR), so the steps that invoke GoReleaser and the GHCR
login/push have the required token scopes; place this permissions mapping at the
same level as runs-on and steps in the release job definition.
In @.gitignore:
- Around line 23-25: The .gitignore currently lists "go.sum" which should not be
ignored; remove the "go.sum" entry from .gitignore and commit the go.sum file to
the repository so dependency checksums are tracked (ensure no other rule later
re-ignores it and then add/commit the go.sum file).
In @.goreleaser.yaml:
- Around line 20-1468: The brews block contains hundreds of repeated duplicate
key blocks for the same formula (the "brews" mapping with "name: gitstart")
causing massive bloat; remove all repeated/duplicated mappings and consolidate
into a single canonical brew entry for name: gitstart that contains only the
unique keys (tap, commit_author, folder, homepage, description, license, test,
install, dependencies, skip_upload, plist/caveats/conflicts/replaces if needed).
Locate the "brews" mapping and the repeated "name: gitstart" entries, keep one
well-formed mapping with the final desired values, delete all subsequent
duplicate blocks, and validate the resulting YAML for uniqueness of keys and
proper indentation so the file reduces to the expected ~30 lines.
In `@bash/Makefile`:
- Around line 47-50: The Makefile uses Bash-specific syntax (the read -p/-n
invocation and the [[ ... =~ ... ]] conditional in the rule that runs bats
tests/integration.bats), so add an explicit SHELL = /bin/bash directive at the
top of the Makefile to force make to use Bash; after adding the SHELL
assignment, keep the existing rule that prompts with read and conditionally runs
bats unchanged.
In `@cmd/root.go`:
- Around line 56-57: Add a pre-run validation on the Cobra command to fail fast
when both visibility flags are set: inspect the boolean flags private and public
(used in rootCmd.PersistentFlags().BoolVarP) in a PreRunE/PersistentPreRunE
handler attached to rootCmd and return an error (e.g., fmt.Errorf("flags
--private and --public are mutually exclusive")) if private && public so the
command exits immediately with a clear message.
In `@internal/config/config_test.go`:
- Around line 8-26: TestSaveAndLoadUsername mutates the real user config by
writing/removing the path returned by ConfigFilePath(); fix by isolating the
test: use t.TempDir() and set an environment override that ConfigFilePath
respects (e.g., t.Setenv("XDG_CONFIG_HOME", tmpDir)) before calling SaveUsername
and LoadUsername so they operate on a temp config, or if ConfigFilePath has no
env override, add a test-only helper ConfigFilePathForDir(dir string) (and/or
allow injecting the config directory) and call that from the test to point
SaveUsername/LoadUsername to tmpDir; ensure the test uses t.TempDir() and
t.Setenv so no real user files are touched.
In `@internal/files/gitignore_test.go`:
- Around line 8-19: The TestFetchGitignore is an integration-style test that
calls FetchGitignore and hits the network using a fixed output file
".gitignore_test", making it flaky and prone to collisions; change it to a
deterministic unit test by either converting it to an integration test (e.g.,
rename/annotate and call t.Skip unless integration flag is set) or mock the
network: refactor FetchGitignore to accept an injectable fetcher (or provide a
helper FetchFunc) and write a unit test that uses httptest.Server to serve a
known .gitignore response and t.TempDir() (or t.Helper-created temp file) for
the output path to avoid filename collisions, updating TestFetchGitignore to use
the injected fetcher/server and temp dir; alternatively, if keeping a
live-network integration test, use t.Skip unless an env/integration flag is
present and switch the output to t.TempDir().
In `@internal/files/gitignore.go`:
- Around line 14-24: The HTTP GET in gitignore.go currently uses http.Get
without timeout and only defers resp.Body.Close() inside the 200 branch, leaking
the body on non-200 responses; replace http.Get with an http.Client having a
timeout (e.g., &http.Client{Timeout: 10 * time.Second}), call client.Get(url),
and as soon as resp != nil defer resp.Body.Close() before checking
resp.StatusCode; if status != 200 return a descriptive error (including status)
so the body is always closed, then proceed to create the file (os.Create) and
io.Copy from resp.Body as before (refer to the current resp, err variables and
the file create/copy logic).
In `@internal/files/license_test.go`:
- Around line 8-19: TestFetchLicenseText is flaky because it calls
FetchLicenseText which hits the network and writes to a fixed file; change
FetchLicenseText to accept an injectable HTTP client or base URL (or a fetch
function) so tests can point to a local server, then update TestFetchLicenseText
to spin up an httptest.Server that returns a deterministic license body and call
FetchLicenseText with the test server/client, and write output into an isolated
path via t.TempDir() + filepath.Join(...) instead of the fixed "LICENSE_test"
filename to avoid parallel test conflicts; update function signatures
(FetchLicenseText) and the test (TestFetchLicenseText) accordingly.
In `@internal/files/license.go`:
- Around line 13-27: The function that fetches the GitHub license (in
internal/files/license.go where the code builds the URL and calls http.Get) must
handle non-200 responses and timeouts: replace the plain http.Get with an
http.Client that has a reasonable Timeout, check resp.StatusCode after the
request and return a clear error when it is not 200 (including status code and
status text), and ensure you do not proceed to decode/write the license if
data.Body is empty—return an error instead; keep the resp.Body.Close defer but
perform status and empty-body checks before allowing the LICENSE file to be
written.
In `@internal/repo/repo_commit_test.go`:
- Around line 31-34: The test in repo_commit_test.go calls CommitAndPush but
doesn't fail when it returns an error; change the assertion so the test fails on
non-nil error and only logs success on nil. Specifically, in the test that calls
CommitAndPush(dir, "main", "test commit"), replace the current unconditional log
with an if/else that calls t.Fatalf or t.Errorf with the error when err != nil
and otherwise t.Logf the success message; reference the CommitAndPush call to
locate where to update the assertion.
In `@internal/repo/repo_test.go`:
- Around line 9-17: Replace the hardcoded tmpDir and manual cleanup with Go's
testing helper: change tmpDir := "test_git_repo" and the os.MkdirAll + defer
os.RemoveAll block to tmpDir := t.TempDir(), remove the explicit mkdir and
deferred removal, and update any subsequent usage of tmpDir; this affects the
tmpDir variable allocation in repo_test.go so tests get isolated temporary
directories automatically.
In `@internal/repo/repo.go`:
- Line 18: The git commit invocation that runs {"git", "commit", "-m", message}
should not abort the whole flow when the repo is clean; either add the
"--allow-empty" flag to the args to create an explicit empty commit, or catch
the git commit error and treat a "nothing to commit" stderr message/exit code as
non-fatal so the flow continues; update the code that constructs/runs the commit
command (the place where {"git","commit","-m", message} is used) to implement
one of these two behaviors.
- Around line 8-44: The git/gh shell-outs (InitGitRepo, CommitAndPush,
CreateGitHubRepo) use exec.Command without timeouts and can hang; update each to
create a context with a bounded timeout (e.g., context.WithTimeout) and execute
the command with exec.CommandContext (or command.WithContext) instead of
exec.Command, ensure you call cancel() after each command, and apply an
appropriate per-command timeout (short for init/commit, longer for push/create)
so each Cmd.Run is bounded and returns on context deadline.
In `@internal/utils/utils.go`:
- Line 10: The current fmt.Printf("ERROR: %s: %v\n", msg, err) writes to stdout;
change it to fmt.Fprintf(os.Stderr, "ERROR: %s: %v\n", msg, err) so errors go to
stderr, and add the os import if missing; update the call site where fmt.Printf
is used (the error-printing invocation shown) to use fmt.Fprintf with os.Stderr.
In `@Makefile`:
- Around line 40-41: The Makefile's install target currently invokes go install
with a file path ("go install main.go"); change the install target (labelled
"install") to invoke go install with a package path such as "go install ." so
the current package is installed into $GOBIN/$GOPATH/bin instead of using a
filename; update the command under the install target accordingly.
---
Minor comments:
In `@cmd/root.go`:
- Around line 63-65: The CLI currently prints execution errors to stdout using
fmt.Println in the rootCmd.Execute error branch; change this to write to stderr
by using fmt.Fprintln with os.Stderr so execution error messages from
rootCmd.Execute() go to stderr (locate the rootCmd.Execute error handling in
cmd/root.go and replace fmt.Println(err) with writing the error to os.Stderr).
In `@docs/README.md`:
- Around line 287-292: The README contains links to
planning/implementation_plan.md that will be broken because the planning/
directory is gitignored; either stop ignoring the planning/ directory so the
file is committed (remove the planning/ entry from .gitignore) or update the
README references (e.g., in the "Interactive prompts" and "Migration Notes"
sections that reference planning/implementation_plan.md) to point to an existing
public doc or remove the link entirely; ensure you update the exact string
"planning/implementation_plan.md" in README.md (and any other files referencing
it) to reflect the chosen fix.
- Around line 59-66: The README's Go install example uses the wrong module path;
update the command in the "Install via Go" section to match the module root
where main.go lives and the module name in go.mod by replacing the incorrect "go
install github.com/shinokada/gitstart/cmd/gitstart@latest" entry with "go
install github.com/shinichiokada/gitstart@latest" so the install points to the
module root (refer to the "Install via Go" block and the example command
string).
In `@internal/prompts/prompts_test.go`:
- Around line 7-19: TestDryRunPrompt currently only calls DryRunPrompt and
TestPromptUser/TestPromptSelect are empty placeholders, which yields false
positives; update TestDryRunPrompt to assert behavior (e.g., capture/mocking
stdout/stderr or check returned values) rather than just calling DryRunPrompt,
and either implement mocked I/O-based assertions for interactive functions
PromptUser and PromptSelect or mark those tests with t.Skip("interactive;
requires manual testing or mocking") so they don't silently pass; use the
function names DryRunPrompt, TestDryRunPrompt, TestPromptUser, and
TestPromptSelect to locate and modify the tests.
In `@internal/repo/repo_github_test.go`:
- Around line 5-7: The TestCreateGitHubRepo test is an empty placeholder that
always passes; update the TestCreateGitHubRepo function to explicitly skip the
test (e.g., call t.Skip or t.Skipf with a short explanatory message) or convert
it into a gated integration test (add a build tag like // +build integration and
check for an env var) so intent is explicit and it doesn’t silently add
coverage; locate the TestCreateGitHubRepo function and add the skip or gating
logic accordingly.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 34-37: The workflow currently uses a floating GoReleaser version
("with: version: latest") in the "Install GoReleaser" step
(goreleaser/goreleaser-action@v5); replace the floating value with a pinned,
explicit release tag or semver (e.g., a tested goreleaser release) to ensure
reproducible builds and guard against upstream breakage—update the "with:
version" value to that specific version and commit.
In `@bash/Makefile`:
- Line 1: Update the .PHONY declaration so it lists every declared Makefile
target to avoid accidental no-op behavior; specifically add targets such as
check-deps, lint, format, install, uninstall, test-all, watch, coverage,
pre-commit, release (and any other targets present) to the existing .PHONY line
so the current ".PHONY: help test test-unit test-integration test-shellcheck
install-deps clean" entry includes all target names.
In `@internal/config/config.go`:
- Around line 35-39: The config loader returns the raw file contents
(string(data)) which can include trailing newlines or whitespace; modify the
return to trim whitespace before returning by applying strings.TrimSpace to the
file contents (e.g., trim the result of string(data) in the same function that
calls os.ReadFile) so the username has no leading/trailing whitespace while
keeping the existing error handling.
- Around line 18-26: The SaveUsername function currently writes an empty string
to the config file when username == "", so add a validation at the start of
SaveUsername to return an error (e.g., fmt.Errorf("username cannot be empty"))
if username is empty, before calling ConfigFilePath(), ensuring you also add
"fmt" to the imports; keep the rest of the directory creation and os.WriteFile
logic unchanged.
In `@internal/files/files_test.go`:
- Around line 8-27: Replace the hardcoded tmpDir with t.TempDir() in
TestCreateProjectDir: call base := t.TempDir() and pass a unique subpath (e.g.
filepath.Join(base, "proj")) to CreateProjectDir so the test still validates
directory creation; remove the manual defer os.RemoveAll cleanup and adjust
assertions to use the new variable (references: TestCreateProjectDir,
CreateProjectDir, tmpDir).
In `@internal/files/readme_test.go`:
- Around line 8-28: TestCreateReadme uses a hardcoded filename and only checks
Size()>0; change it to use t.TempDir() for isolation and remove manual os.Remove
cleanup, call CreateReadme with a path inside that temp dir (e.g.,
filepath.Join(tempDir, "README_test.md")), assert CreateReadme returns nil, then
read the file contents and verify it contains the project name and description
(e.g., check for "TestProject" and "Test description.") instead of just checking
size; update references to the CreateReadme call and file variable in
readme_test.go accordingly.
In `@internal/prompts/prompts.go`:
- Around line 16-39: PromptUser and PromptSelect currently swallow I/O and parse
errors (discarding ReadString error and returning empty string on Scanln
failures), so change both functions (PromptUser and PromptSelect) to return
(string, error), propagate and return any error from reader.ReadString (trimmed)
in PromptUser, and in PromptSelect capture fmt.Scanln errors and return an
explicit error when scanning or when the numeric choice is out of range; update
callers to handle the returned error so they can retry or abort instead of
treating "" as user input.
In `@internal/repo/repo_commit_test.go`:
- Around line 9-15: Replace the fixed directory creation with the testing
helper: instead of setting dir := "test_commit_repo" and calling
os.MkdirAll/deferring os.RemoveAll, use t.TempDir() to obtain an isolated
temporary directory for the test; update any uses of the variable dir in this
test (in internal/repo/repo_commit_test.go) to rely on the returned t.TempDir()
value and remove the explicit cleanup code that calls os.RemoveAll.
In `@internal/utils/utils_test.go`:
- Around line 10-13: The test currently only checks that ErrorHandler(err, "test
message") returns non-nil; change the assertion to verify the returned error is
the original error (err) by comparing result to err (or using errors.Is(result,
err)) so the test ensures ErrorHandler returns the same error value; update the
assertion around result and err in the Test in internal/utils/utils_test.go
accordingly.
In `@Makefile`:
- Line 1: Add a phony all target and declare it in the existing .PHONY list so
static linting finds it: update the .PHONY line (which currently lists build
test clean ...) to include all, and add an "all:" target that forwards to the
primary build target (e.g., "all: build" or "all: default-target") so make all
behaves as the conventional alias for building the project.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
.github/workflows/release.yml.gitignore.goreleaser.yamlMakefilebash/.gitattributesbash/Makefilebash/README.mdbash/tests/README.mdbash/tests/fix-permissions.shbash/tests/gitstart.batsbash/tests/integration.batsbash/tests/quick-test.shbash/tests/run-tests.shbash/tests/shellcheck.shbash/tests/test-dry-run-simple.shbash/tests/test-dry-run.shbash/tests/test-path-handling.shbash/tests/test-validation.shbash/tests/verify-changes.shbash/uninstall.shcmd/root.godocs/README.mdgitstartgo.modinternal/config/config.gointernal/config/config_test.gointernal/files/files.gointernal/files/files_test.gointernal/files/gitignore.gointernal/files/gitignore_test.gointernal/files/license.gointernal/files/license_test.gointernal/files/readme.gointernal/files/readme_test.gointernal/prompts/prompts.gointernal/prompts/prompts_test.gointernal/repo/repo.gointernal/repo/repo_commit_test.gointernal/repo/repo_github_test.gointernal/repo/repo_test.gointernal/utils/utils.gointernal/utils/utils_test.gomain.goupdates/ABOUT_FIX_PERMISSIONS.mdupdates/ABSOLUTE_PATH_BUG.mdupdates/CI_FIXES_AND_CODERABBIT.mdupdates/CI_FIX_SUMMARY.mdupdates/CI_SETUP.mdupdates/CLEANUP_SUMMARY.mdupdates/CODERABBIT_FIXES.mdupdates/CODERABBIT_SECOND_REVIEW.mdupdates/COMPLETE_DIRECTORY_REORG.mdupdates/COMPLETE_FIX_SUMMARY.mdupdates/CROSS_PLATFORM.mdupdates/EXAMPLES.mdupdates/FINAL_PROJECT_STATUS.mdupdates/FINAL_SUMMARY.mdupdates/GPL_BUG_EXPLANATION.mdupdates/LINUX_COMPATIBILITY.mdupdates/MASTER_STATUS.mdupdates/MIGRATION.mdupdates/PRE_COMMIT_CHECKLIST.mdupdates/PROJECT_CLEANUP_AND_FIXES.mdupdates/QUICK_FIX.mdupdates/QUICK_REFERENCE.mdupdates/README.mdupdates/ROUND2_FIXES.mdupdates/ROUND3_FIXES.mdupdates/SHELLCHECK_SC2312_FIXES.mdupdates/TESTING.mdupdates/TESTING_INFRASTRUCTURE.mdupdates/TEST_EXECUTION_GUIDE.mdupdates/TEST_FIXES.mdupdates/TEST_FIXES_FINAL_ROUND.mdupdates/TEST_FIX_EMPTY_MESSAGE.mdupdates/TEST_FIX_SUMMARY.mdupdates/UPDATE_SUMMARY.mdupdates/VERIFICATION_CHECKLIST.md
💤 Files with no reviewable changes (35)
- updates/TESTING.md
- updates/QUICK_REFERENCE.md
- updates/FINAL_SUMMARY.md
- updates/MIGRATION.md
- updates/GPL_BUG_EXPLANATION.md
- updates/TEST_FIX_SUMMARY.md
- updates/TESTING_INFRASTRUCTURE.md
- updates/README.md
- updates/TEST_EXECUTION_GUIDE.md
- updates/CODERABBIT_SECOND_REVIEW.md
- updates/CI_FIX_SUMMARY.md
- updates/ABOUT_FIX_PERMISSIONS.md
- updates/TEST_FIXES_FINAL_ROUND.md
- updates/CI_SETUP.md
- updates/LINUX_COMPATIBILITY.md
- updates/CROSS_PLATFORM.md
- updates/TEST_FIX_EMPTY_MESSAGE.md
- updates/UPDATE_SUMMARY.md
- updates/MASTER_STATUS.md
- updates/COMPLETE_FIX_SUMMARY.md
- updates/ROUND3_FIXES.md
- updates/CODERABBIT_FIXES.md
- updates/PRE_COMMIT_CHECKLIST.md
- updates/CLEANUP_SUMMARY.md
- updates/SHELLCHECK_SC2312_FIXES.md
- updates/EXAMPLES.md
- updates/QUICK_FIX.md
- updates/TEST_FIXES.md
- updates/COMPLETE_DIRECTORY_REORG.md
- updates/PROJECT_CLEANUP_AND_FIXES.md
- updates/FINAL_PROJECT_STATUS.md
- updates/ROUND2_FIXES.md
- updates/ABSOLUTE_PATH_BUG.md
- updates/VERIFICATION_CHECKLIST.md
- updates/CI_FIXES_AND_CODERABBIT.md
bash/Makefile
Outdated
Comment on lines
+47
to
+50
| @read -p "Continue? [y/N] " -n 1 -r; \ | ||
| if [[ $$REPLY =~ ^[Yy]$$ ]]; then \ | ||
| bats tests/integration.bats; \ | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the bash/Makefile
find . -path "*/bash/Makefile" -type fRepository: shinokada/gitstart
Length of output: 77
🏁 Script executed:
# Read the Makefile to check lines 40-55 for context and SHELL directive
head -60 bash/Makefile | cat -nRepository: shinokada/gitstart
Length of output: 2415
🏁 Script executed:
# Check if there are other Makefiles in the project to see if they use SHELL directive
fd -t f -n "Makefile" -x grep -l "SHELL :=" {} \;Repository: shinokada/gitstart
Length of output: 292
🏁 Script executed:
# Search for SHELL directives in Makefiles across the project
fd "Makefile" -type f --exec grep -l "SHELL :=" {} \;Repository: shinokada/gitstart
Length of output: 232
🏁 Script executed:
# Search for SHELL directives in Makefiles across the project
find . -name "Makefile" -exec grep -l "SHELL :=" {} \;Repository: shinokada/gitstart
Length of output: 44
🏁 Script executed:
# Verify that bash-specific syntax would fail under /bin/sh by checking syntax
# Let's also see if there are any other test targets that might have similar issues
grep -n "read \|\\[\\[" bash/MakefileRepository: shinokada/gitstart
Length of output: 144
SHELL must be explicitly set to /bin/bash in this Makefile.
Lines 47-49 use Bash-specific syntax (read -p/-n, [[ ... ]], =~) that will fail when make executes with the default /bin/sh shell. Add the SHELL directive at the top of the file to ensure Bash is used.
Suggested fix
+SHELL := /usr/bin/env bash
.PHONY: help test test-unit test-integration test-shellcheck install-deps clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bash/Makefile` around lines 47 - 50, The Makefile uses Bash-specific syntax
(the read -p/-n invocation and the [[ ... =~ ... ]] conditional in the rule that
runs bats tests/integration.bats), so add an explicit SHELL = /bin/bash
directive at the top of the Makefile to force make to use Bash; after adding the
SHELL assignment, keep the existing rule that prompts with read and
conditionally runs bats unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/repo/repo.go (1)
9-9:⚠️ Potential issue | 🟠 MajorUse longer timeouts for network-bound commands.
Line 9 applies one 30s timeout to everything, but
git push(Line 28) andgh repo create(Line 52) are network-bound and can exceed that under normal conditions, causing avoidable failures.💡 Proposed fix (per-command timeout policy)
-const cmdTimeout = 30 * time.Second +const ( + localCmdTimeout = 30 * time.Second + remoteCmdTimeout = 2 * time.Minute +) func InitGitRepo(dir string) error { - ctx, cancel := context.WithTimeout(context.Background(), cmdTimeout) + ctx, cancel := context.WithTimeout(context.Background(), localCmdTimeout) defer cancel() cmd := exec.CommandContext(ctx, "git", "init") cmd.Dir = dir return cmd.Run() } func CommitAndPush(dir, branch, message string) error { - cmds := [][]string{ - {"git", "add", "."}, - {"git", "commit", "--allow-empty", "-m", message}, - {"git", "branch", "-M", branch}, - {"git", "push", "-u", "origin", branch}, + type commandSpec struct { + args []string + timeout time.Duration + } + cmds := []commandSpec{ + {args: []string{"git", "add", "."}, timeout: localCmdTimeout}, + {args: []string{"git", "commit", "--allow-empty", "-m", message}, timeout: localCmdTimeout}, + {args: []string{"git", "branch", "-M", branch}, timeout: localCmdTimeout}, + {args: []string{"git", "push", "-u", "origin", branch}, timeout: remoteCmdTimeout}, } - for _, args := range cmds { - ctx, cancel := context.WithTimeout(context.Background(), cmdTimeout) - cmd := exec.CommandContext(ctx, args[0], args[1:]...) + for _, c := range cmds { + ctx, cancel := context.WithTimeout(context.Background(), c.timeout) + cmd := exec.CommandContext(ctx, c.args[0], c.args[1:]...) cmd.Dir = dir err := cmd.Run() cancel() if err != nil { return err } } return nil } func CreateGitHubRepo(dir, repoName, visibility, description string) error { @@ - ctx, cancel := context.WithTimeout(context.Background(), cmdTimeout) + ctx, cancel := context.WithTimeout(context.Background(), remoteCmdTimeout) defer cancel() cmd := exec.CommandContext(ctx, "gh", args...)Also applies to: 28-28, 52-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` at line 9, The global cmdTimeout = 30 * time.Second is too short for network-bound operations; update the code to use longer, per-command timeouts for the network calls (specifically the git push invocation and the gh repo create call). Add explicit timeout constants (e.g., gitPushTimeout, ghRepoCreateTimeout) or pass a larger context/timeout when calling the functions that run those commands (the code paths invoking "git push" and "gh repo create") while leaving cmdTimeout for local fast commands; ensure you reference and use these new timeouts where the existing run/exec logic currently runs "git push" and "gh repo create".
🧹 Nitpick comments (7)
internal/utils/utils_test.go (1)
8-19: Assert stderr side effects, not only return values.Current test validates return behavior but doesn’t verify the observable output contract (stderr emission for non-nil errors, silence for nil errors).
Proposed test hardening
import ( "errors" + "io" + "os" + "strings" "testing" ) +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + os.Stderr = w + fn() + _ = w.Close() + os.Stderr = orig + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("failed to read stderr: %v", err) + } + return string(out) +} + func TestErrorHandler(t *testing.T) { err := errors.New("test error") - result := ErrorHandler(err, "test message") + stderrOut := captureStderr(t, func() { + result := ErrorHandler(err, "test message") + if result != err { + t.Fatalf("expected original error to be returned, got: %v", result) + } + }) + if !strings.Contains(stderrOut, "ERROR: test message: test error") { + t.Fatalf("expected stderr to contain formatted error, got: %q", stderrOut) + } - if result != err { - t.Fatalf("expected original error to be returned, got: %v", result) - } - nilResult := ErrorHandler(nil, "should not print") - if nilResult != nil { - t.Fatalf("expected nil to be returned") + nilStderrOut := captureStderr(t, func() { + nilResult := ErrorHandler(nil, "should not print") + if nilResult != nil { + t.Fatalf("expected nil to be returned") + } + }) + if nilStderrOut != "" { + t.Fatalf("expected no stderr output for nil error, got: %q", nilStderrOut) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/utils_test.go` around lines 8 - 19, Update TestErrorHandler to assert stderr side effects in addition to return values: wrap calls to ErrorHandler in a temporary capture of os.Stderr (pipe or buffer) and for the non-nil case assert the captured stderr contains the "test message" (or error text) and for the nil case assert captured stderr is empty; keep the existing assertions that ErrorHandler returns the original error and nil respectively. Use the ErrorHandler symbol and the TestErrorHandler test function to locate where to add the stderr capture/restore logic and ensure you restore os.Stderr after each subtest to avoid leaking the redirection.internal/prompts/prompts_test.go (2)
7-11: AssertDryRunPromptoutput, not just execution.This test currently passes even if the printed message format regresses. Capture stdout and assert the exact output contract.
Proposed test improvement
import ( + "io" + "os" "testing" ) func TestDryRunPrompt(t *testing.T) { - // This test just ensures the function runs without error. - DryRunPrompt("Create project directory") - + old := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create stdout pipe: %v", err) + } + os.Stdout = w + t.Cleanup(func() { os.Stdout = old }) + + DryRunPrompt("Create project directory") + + _ = w.Close() + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("failed to read captured stdout: %v", err) + } + + want := "[DRY RUN] Would perform: Create project directory\n" + if string(out) != want { + t.Fatalf("unexpected output: got %q, want %q", string(out), want) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts_test.go` around lines 7 - 11, The test TestDryRunPrompt currently only calls DryRunPrompt("Create project directory") without verifying output; update it to capture stdout when calling DryRunPrompt and assert the captured string equals the expected message (including newline/formatting) to prevent regressions in printed format. Locate TestDryRunPrompt and the DryRunPrompt function, use a stdout capture approach (e.g., redirect os.Stdout to a pipe/buffer within the test) around the DryRunPrompt call, then compare the buffer contents to the exact expected output and fail the test if they differ.
13-19: Skipped prompt tests leave interactive behavior unverified.Both prompt APIs are currently untested. Consider introducing an input/output seam (e.g., injectable
io.Reader/io.Writer) so these tests can run non-interactively in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts_test.go` around lines 13 - 19, The prompt tests are skipped because prompt functions are tightly coupled to os.Stdin/os.Stdout; refactor the interactive API (e.g., PromptUser and PromptSelect) to accept injectable io.Reader and io.Writer (or add overloaded variants like PromptUserWithIO/PromptSelectWithIO) so tests can supply bytes.Buffer or strings.NewReader for input and a bytes.Buffer for output; update the tests TestPromptUser and TestPromptSelect to use these injectable variants, feed simulated input, capture output, and assert expected behavior instead of skipping.cmd/root.go (1)
70-73: Remove dead code:importPromptsserves no purpose.This function is a no-op. The
promptspackage is already used directly in theRunfunction (lines 43-63), so the import is retained by actual usage. The call toimportPrompts()on line 42 and the function itself are unnecessary.♻️ Proposed fix
Run: func(cmd *cobra.Command, args []string) { if directory == "" { cmd.Help() return } if dryRun { - importPrompts() prompts.DryRunPrompt("[OPTIONS]")Remove the function definition entirely:
-// importPrompts is a hack to ensure prompts is imported for use in Run. -func importPrompts() { - // This function does nothing, but ensures the prompts package is imported. -} - // boolToString returns "true" or "false" for a bool value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 70 - 73, Remove the dead no-op helper: delete the importPrompts() function definition and remove any invocation of importPrompts() (it's unused); the prompts package is already referenced in the Run function (so the import will be retained by actual usage) — search for the symbol importPrompts and remove both its call and its function to eliminate dead code.Makefile (1)
1-1: Consider adding a conventionalalltarget.Static analysis suggests adding an
alltarget. Whilebuildserves as the default (first target), addingallis a common Makefile convention that improves discoverability.♻️ Optional enhancement
-.PHONY: build test clean clean-cache clean-all run lint lint-fix coverage install +.PHONY: all build test clean clean-cache clean-all run lint lint-fix coverage install + +# Default target +all: build # Build the gitstart binary build:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add a conventional top-level "all" target to the Makefile that aliases the existing build target so users and tools can discover the default build action; create a target named all that depends on build (e.g., "all: build") and keep it phony by including it in the .PHONY list (add "all" alongside build/test/clean).internal/repo/repo.go (2)
17-17: Return command/stderr context on failures.Current
cmd.Run()returns opaque errors (often just exit status). Include command + combined output so users can diagnose auth/remote/config problems quickly.Also applies to: 34-38, 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` at line 17, The code currently returns opaque errors from cmd.Run(); change those call sites (where cmd.Run() is returned) to use cmd.CombinedOutput() instead, check the error, and wrap/return a new error that includes the command (cmd.Args or cmd.String()) and the combined output (string(output)) so callers see the stderr/stdout context; update each occurrence (the cmd.Run() returns around the repo functions in internal/repo/repo.go) to follow this pattern so failures surface the exact command invoked and its output for debugging.
46-48: Validatevisibilitybefore converting to a flag.Line 47 appends raw input as
--<value>. Fail fast with an allowlist (public|private|internal) so invalid values are rejected with a clear error instead of a downstream CLI failure.💡 Proposed fix
import ( "context" + "fmt" "os/exec" "time" ) @@ args := []string{"repo", "create", repoName, "--source=.", "--remote=origin", "--push"} - if visibility != "" { - args = append(args, "--"+visibility) + switch visibility { + case "": + case "public", "private", "internal": + args = append(args, "--"+visibility) + default: + return fmt.Errorf("invalid visibility %q: expected public, private, or internal", visibility) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` around lines 46 - 48, Validate the visibility value before appending it as a CLI flag: in internal/repo/repo.go locate the block that uses the visibility variable and args slice (the code that currently does args = append(args, "--"+visibility)) and replace it with an allowlist check that only accepts "public", "private", or "internal"; if visibility is non-empty but not one of those values, return or propagate a clear error (e.g., "invalid visibility: <value>, must be one of public|private|internal") instead of appending, otherwise append "--"+visibility as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/files/gitignore_test.go`:
- Around line 17-28: The test currently only checks that FetchGitignore("Go",
file) creates a non-empty file, which can pass when the function returns its
local fallback; update the assertion to verify the file contains the expected
network-fetched content (e.g., check for a known snippet or marker that appears
in the real GitHub Go .gitignore) and explicitly fail if the content equals the
fallback minimal string used by FetchGitignore; locate the test in
gitignore_test.go and adjust the assertions after os.Stat/file read to read the
file contents and assert they include the network-specific token or do not equal
the fallback constant returned by FetchGitignore.
---
Duplicate comments:
In `@internal/repo/repo.go`:
- Line 9: The global cmdTimeout = 30 * time.Second is too short for
network-bound operations; update the code to use longer, per-command timeouts
for the network calls (specifically the git push invocation and the gh repo
create call). Add explicit timeout constants (e.g., gitPushTimeout,
ghRepoCreateTimeout) or pass a larger context/timeout when calling the functions
that run those commands (the code paths invoking "git push" and "gh repo
create") while leaving cmdTimeout for local fast commands; ensure you reference
and use these new timeouts where the existing run/exec logic currently runs "git
push" and "gh repo create".
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 70-73: Remove the dead no-op helper: delete the importPrompts()
function definition and remove any invocation of importPrompts() (it's unused);
the prompts package is already referenced in the Run function (so the import
will be retained by actual usage) — search for the symbol importPrompts and
remove both its call and its function to eliminate dead code.
In `@internal/prompts/prompts_test.go`:
- Around line 7-11: The test TestDryRunPrompt currently only calls
DryRunPrompt("Create project directory") without verifying output; update it to
capture stdout when calling DryRunPrompt and assert the captured string equals
the expected message (including newline/formatting) to prevent regressions in
printed format. Locate TestDryRunPrompt and the DryRunPrompt function, use a
stdout capture approach (e.g., redirect os.Stdout to a pipe/buffer within the
test) around the DryRunPrompt call, then compare the buffer contents to the
exact expected output and fail the test if they differ.
- Around line 13-19: The prompt tests are skipped because prompt functions are
tightly coupled to os.Stdin/os.Stdout; refactor the interactive API (e.g.,
PromptUser and PromptSelect) to accept injectable io.Reader and io.Writer (or
add overloaded variants like PromptUserWithIO/PromptSelectWithIO) so tests can
supply bytes.Buffer or strings.NewReader for input and a bytes.Buffer for
output; update the tests TestPromptUser and TestPromptSelect to use these
injectable variants, feed simulated input, capture output, and assert expected
behavior instead of skipping.
In `@internal/repo/repo.go`:
- Line 17: The code currently returns opaque errors from cmd.Run(); change those
call sites (where cmd.Run() is returned) to use cmd.CombinedOutput() instead,
check the error, and wrap/return a new error that includes the command (cmd.Args
or cmd.String()) and the combined output (string(output)) so callers see the
stderr/stdout context; update each occurrence (the cmd.Run() returns around the
repo functions in internal/repo/repo.go) to follow this pattern so failures
surface the exact command invoked and its output for debugging.
- Around line 46-48: Validate the visibility value before appending it as a CLI
flag: in internal/repo/repo.go locate the block that uses the visibility
variable and args slice (the code that currently does args = append(args,
"--"+visibility)) and replace it with an allowlist check that only accepts
"public", "private", or "internal"; if visibility is non-empty but not one of
those values, return or propagate a clear error (e.g., "invalid visibility:
<value>, must be one of public|private|internal") instead of appending,
otherwise append "--"+visibility as before.
In `@internal/utils/utils_test.go`:
- Around line 8-19: Update TestErrorHandler to assert stderr side effects in
addition to return values: wrap calls to ErrorHandler in a temporary capture of
os.Stderr (pipe or buffer) and for the non-nil case assert the captured stderr
contains the "test message" (or error text) and for the nil case assert captured
stderr is empty; keep the existing assertions that ErrorHandler returns the
original error and nil respectively. Use the ErrorHandler symbol and the
TestErrorHandler test function to locate where to add the stderr capture/restore
logic and ensure you restore os.Stderr after each subtest to avoid leaking the
redirection.
In `@Makefile`:
- Line 1: Add a conventional top-level "all" target to the Makefile that aliases
the existing build target so users and tools can discover the default build
action; create a target named all that depends on build (e.g., "all: build") and
keep it phony by including it in the .PHONY list (add "all" alongside
build/test/clean).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/release.yml.gitignore.goreleaser.yamlMakefilecmd/root.godocs/README.mdgitstartinternal/config/config.gointernal/config/config_test.gointernal/files/files_test.gointernal/files/gitignore.gointernal/files/gitignore_test.gointernal/files/license.gointernal/files/license_test.gointernal/files/readme_test.gointernal/prompts/prompts_test.gointernal/repo/repo.gointernal/repo/repo_commit_test.gointernal/repo/repo_github_test.gointernal/repo/repo_test.gointernal/utils/utils.gointernal/utils/utils_test.go
💤 Files with no reviewable changes (1)
- gitstart
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/repo/repo_github_test.go
- internal/repo/repo_test.go
- internal/repo/repo_commit_test.go
- internal/files/gitignore.go
- internal/config/config.go
- internal/files/license_test.go
- .gitignore
- docs/README.md
- internal/files/files_test.go
- internal/config/config_test.go
- .github/workflows/release.yml
…d code removal, CI chmod
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
cmd/root.go (3)
69-75: Usestrconv.FormatBoolinstead of a custom helper.Go's standard library provides
strconv.FormatBool(b)which does exactly this. It's idiomatic and reduces custom code.Suggested fix
Add the import:
import ( "fmt" "os" + "strconv" "github.com/shinichiokada/gitstart/internal/prompts" "github.com/spf13/cobra" )Then replace usage:
- prompts.DryRunPrompt(" Private: " + boolToString(private)) - prompts.DryRunPrompt(" Public: " + boolToString(public)) - prompts.DryRunPrompt(" Quiet: " + boolToString(quiet)) + prompts.DryRunPrompt(" Private: " + strconv.FormatBool(private)) + prompts.DryRunPrompt(" Public: " + strconv.FormatBool(public)) + prompts.DryRunPrompt(" Quiet: " + strconv.FormatBool(quiet))And remove the
boolToStringfunction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 69 - 75, Replace the custom helper boolToString with the standard library function: import strconv, replace all calls to boolToString(...) with strconv.FormatBool(...), and remove the boolToString function definition from cmd/root.go; ensure the new import is added to the existing import block so references compile.
37-40: Consider handling the error fromcmd.Help().
cmd.Help()returns an error that is currently ignored. While it rarely fails, handling it provides better diagnostics if something goes wrong.Suggested fix
if directory == "" { - cmd.Help() + _ = cmd.Help() return }Or, if you want to surface the error:
Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if directory == "" { - cmd.Help() - return + return cmd.Help() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 37 - 40, Capture and handle the error returned by cmd.Help() instead of ignoring it: assign err := cmd.Help() and if err != nil, return or log a descriptive error (e.g., wrap and return fmt.Errorf("showing help failed: %w", err) or call a logger) so callers get diagnostics; update the block where directory == "" that currently calls cmd.Help() to check its returned error and propagate or report it accordingly.
77-78: Consider injecting the version at build time.Hardcoding the version requires manual updates on each release. A common Go pattern is to use
-ldflagsto inject the version:var version = "dev" // overridden at build timeThen build with:
go build -ldflags "-X 'github.com/shinichiokada/gitstart/cmd.version=v1.2.3'" .This integrates well with GoReleaser (which is already in the project).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 77 - 78, The version variable is hardcoded; change its default to a build-time override-friendly value and ensure builds inject the real version via ldflags; update the declaration of the package-level variable `version` (in cmd/root.go) to use a placeholder like "dev" so `go build -ldflags "-X 'github.com/shinichiokada/gitstart/cmd.version=vX.Y.Z'"` (or your GoReleaser config) can replace it at build time, and verify any uses of `version` (e.g., in the root command output) still read that variable.internal/prompts/prompts_test.go (1)
9-30: Consider handling thew.Close()error.Line 20 discards the error from
w.Close(). While rare, a close failure could causeio.ReadAll(r)to hang or return incomplete data. Checking the error improves test reliability.Suggested fix
- _ = w.Close() + if err := w.Close(); err != nil { + t.Fatalf("failed to close pipe writer: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts_test.go` around lines 9 - 30, The test TestDryRunPrompt currently discards the error returned by w.Close(); update the test to check the result of w.Close() (call w.Close() and if err != nil { t.Fatalf("failed to close stdout pipe: %v", err) }) before calling io.ReadAll(r) so any close failures fail the test and prevent hangs or incomplete reads when invoking DryRunPrompt; locate the close on the pipe (w.Close()) in TestDryRunPrompt and add the error check there.Makefile (1)
22-25: Consider a CI-friendly coverage option.The
coveragetarget opens an HTML report withgo tool cover -html=..., which works locally but may fail or hang in headless CI environments. Consider adding a separate target or flag for CI that outputs text coverage instead.Suggested addition
# Generate coverage report (CI-friendly, text output) coverage-ci: go test -coverprofile=coverage.out ./... go tool cover -func=coverage.out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 22 - 25, The current Makefile coverage target opens an HTML report which can hang in headless CI; add a new CI-friendly target named coverage-ci that still runs go test -coverprofile=coverage.out but instead prints a text summary via go tool cover -func=coverage.out (leave the existing coverage target for local HTML viewing), and update any CI workflows to call coverage-ci instead of coverage.internal/repo/repo.go (2)
17-27: Consider extracting a shared command runner helper.
context+exec+stderr+wrapis repeated in three places. A small helper would reduce duplication and keep timeout/error behavior consistent.Also applies to: 45-54, 72-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` around lines 17 - 27, Extract a small helper (e.g., runCmdWithTimeout or runCommandCtx) that accepts context timeout, working directory, command args and returns combined error/stderr; replace the duplicated pattern in InitGitRepo (the context.WithTimeout + exec.CommandContext + cmd.Stderr buffer + cmd.Run + fmt.Errorf wrapping) and use the same helper for the other similar blocks referenced (the blocks at lines 45-54 and 72-80) so timeouting, stderr capture and error wrapping are centralized and consistent.
53-53: Include the failing subcommand in error context.At Line 53, the prefix currently reports only
git, which hides the exact failing step (add,commit,push, etc.). Include full args for actionable diagnostics.🔧 Proposed refactor
import ( "bytes" "context" "fmt" "os/exec" + "strings" "time" ) @@ - return fmt.Errorf("%s: %w: %s", c.args[0], err, stderr.String()) + return fmt.Errorf("%s: %w: %s", strings.Join(c.args, " "), err, strings.TrimSpace(stderr.String())) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` at line 53, The error currently prefixes failures with only c.args[0] (e.g., "git") which hides the actual subcommand; update the return that builds the error (the fmt.Errorf(...) call that uses stderr.String()) to include the full invoked args by joining c.args (e.g., strings.Join(c.args, " ")) as the context prefix instead of c.args[0], and ensure you import strings if needed so the final message reads something like "<full command>: <wrapped error>: <stderr>".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/repo/repo.go`:
- Around line 62-68: CreateGitHubRepo currently lets an empty visibility slip
through the switch; change the handling so empty visibility is not left to gh's
defaults — either explicitly default it to "private" or return an error.
Concretely, update the start of the CreateGitHubRepo visibility logic (the
visibility variable and the switch block that appends to args) to detect
visibility=="" and set visibility = "private" (or alternatively return
fmt.Errorf("visibility must be one of: public, private, internal") if you prefer
to reject empty input), then continue with the existing cases that append
"--"+visibility to args.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 69-75: Replace the custom helper boolToString with the standard
library function: import strconv, replace all calls to boolToString(...) with
strconv.FormatBool(...), and remove the boolToString function definition from
cmd/root.go; ensure the new import is added to the existing import block so
references compile.
- Around line 37-40: Capture and handle the error returned by cmd.Help() instead
of ignoring it: assign err := cmd.Help() and if err != nil, return or log a
descriptive error (e.g., wrap and return fmt.Errorf("showing help failed: %w",
err) or call a logger) so callers get diagnostics; update the block where
directory == "" that currently calls cmd.Help() to check its returned error and
propagate or report it accordingly.
- Around line 77-78: The version variable is hardcoded; change its default to a
build-time override-friendly value and ensure builds inject the real version via
ldflags; update the declaration of the package-level variable `version` (in
cmd/root.go) to use a placeholder like "dev" so `go build -ldflags "-X
'github.com/shinichiokada/gitstart/cmd.version=vX.Y.Z'"` (or your GoReleaser
config) can replace it at build time, and verify any uses of `version` (e.g., in
the root command output) still read that variable.
In `@internal/prompts/prompts_test.go`:
- Around line 9-30: The test TestDryRunPrompt currently discards the error
returned by w.Close(); update the test to check the result of w.Close() (call
w.Close() and if err != nil { t.Fatalf("failed to close stdout pipe: %v", err)
}) before calling io.ReadAll(r) so any close failures fail the test and prevent
hangs or incomplete reads when invoking DryRunPrompt; locate the close on the
pipe (w.Close()) in TestDryRunPrompt and add the error check there.
In `@internal/repo/repo.go`:
- Around line 17-27: Extract a small helper (e.g., runCmdWithTimeout or
runCommandCtx) that accepts context timeout, working directory, command args and
returns combined error/stderr; replace the duplicated pattern in InitGitRepo
(the context.WithTimeout + exec.CommandContext + cmd.Stderr buffer + cmd.Run +
fmt.Errorf wrapping) and use the same helper for the other similar blocks
referenced (the blocks at lines 45-54 and 72-80) so timeouting, stderr capture
and error wrapping are centralized and consistent.
- Line 53: The error currently prefixes failures with only c.args[0] (e.g.,
"git") which hides the actual subcommand; update the return that builds the
error (the fmt.Errorf(...) call that uses stderr.String()) to include the full
invoked args by joining c.args (e.g., strings.Join(c.args, " ")) as the context
prefix instead of c.args[0], and ensure you import strings if needed so the
final message reads something like "<full command>: <wrapped error>: <stderr>".
In `@Makefile`:
- Around line 22-25: The current Makefile coverage target opens an HTML report
which can hang in headless CI; add a new CI-friendly target named coverage-ci
that still runs go test -coverprofile=coverage.out but instead prints a text
summary via go tool cover -func=coverage.out (leave the existing coverage target
for local HTML viewing), and update any CI workflows to call coverage-ci instead
of coverage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/tests.ymlMakefilecmd/root.gointernal/files/gitignore_test.gointernal/prompts/prompts_test.gointernal/repo/repo.gointernal/utils/utils_test.go
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/utils/utils_test.go
Comment on lines
+62
to
+68
| switch visibility { | ||
| case "": | ||
| case "public", "private", "internal": | ||
| args = append(args, "--"+visibility) | ||
| default: | ||
| return fmt.Errorf("invalid visibility %q: expected public, private, or internal", visibility) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Locate definition and call sites for CreateGitHubRepo ==="
echo "--- Function definition ---"
ast-grep --lang go --pattern 'func CreateGitHubRepo($_, $_, $_, $_) error { $$$ }' 2>/dev/null || echo "No exact match found with ast-grep, trying rg..."
rg -n "func CreateGitHubRepo" --type=go || echo "No function definition found"
echo
echo "--- Call sites of CreateGitHubRepo ---"
rg -n "CreateGitHubRepo\(" --type=go -A1 -B1 || echo "No call sites found"
echo
echo "=== 2) Check for empty visibility literals at call sites ==="
rg -nP --type=go -C2 'CreateGitHubRepo\([^,\n]+,\s*[^,\n]+,\s*""' || echo "No empty string visibility calls found"
echo
echo "=== 3) Check all CreateGitHubRepo calls with context ==="
rg -n "CreateGitHubRepo" --type=go -A 5 -B 5 | head -100Repository: shinokada/gitstart
Length of output: 2097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding all files that import or reference CreateGitHubRepo ==="
rg -l "CreateGitHubRepo" --type=go
echo
echo "=== Search for actual invocations (not just definition) ==="
rg -n "CreateGitHubRepo\s*\(" --type=go -B3 -A3
echo
echo "=== Check main package for CLI entry points ==="
find . -name "main.go" -o -name "*main*.go" | head -5
rg -n "visibility" --type=go -B2 -A2 | head -50
echo
echo "=== Check for any function that calls CreateGitHubRepo ==="
rg -n "func.*{" --type=go -A20 | rg -B5 -A5 "CreateGitHubRepo"Repository: shinokada/gitstart
Length of output: 2027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check main.go for CreateGitHubRepo calls ==="
cat -n ./main.go | head -100
echo
echo "=== Search for any CreateGitHubRepo invocation ==="
rg "CreateGitHubRepo" --type=go -C5
echo
echo "=== Check for visibility flag handling ==="
rg -n "visibility" --type=go -C3Repository: shinokada/gitstart
Length of output: 2240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find cmd package ==="
find . -path ./vendor -prune -o -name "*.go" -type f | grep cmd | head -10
echo
echo "=== Search for CreateGitHubRepo in cmd package ==="
find . -path ./vendor -prune -o -name "*.go" -type f -exec grep -l "CreateGitHubRepo\|repo.CreateGitHubRepo" {} \; 2>/dev/null
echo
echo "=== Check all go files with full context for CreateGitHubRepo usage ==="
rg -n "repo\." --type=go | grep -i create | head -20
echo
echo "=== Look at directory structure ==="
find . -type d -name cmd | head -5
ls -la ./cmd/ 2>/dev/null || echo "cmd dir not found at root"
echo
echo "=== Search more broadly ==="
rg -n "\.CreateGitHubRepo" --type=go
</parameter>Repository: shinokada/gitstart
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List all files in cmd directory ==="
find ./cmd -type f -name "*.go" 2>/dev/null | head -20
echo
echo "=== Read cmd/root.go ==="
wc -l ./cmd/root.go
cat -n ./cmd/root.go | head -150
echo
echo "=== Search for repo package import and usage ==="
rg -n "import.*repo\|from.*repo\|repo\." --type=go ./cmd/ 2>/dev/null || echo "No repo references in cmd"
</parameter>Repository: shinokada/gitstart
Length of output: 5203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for all imports of repo package ==="
rg -n "import.*repo\|github.com.*repo" --type=go
echo
echo "=== Look for the actual Run function implementation ==="
rg -n "func.*Run\(" --type=go -A 50 | head -150
echo
echo "=== Search for any function that might call CreateGitHubRepo ==="
rg -n "private\|public" --type=go -B5 -A5 | grep -A10 -B10 "visibility\|CreateGitHubRepo"
echo
echo "=== Check if there are other go files ==="
find . -name "*.go" -type f | grep -v vendor | grep -v testRepository: shinokada/gitstart
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List all Go files in the repository ==="
find . -name "*.go" -type f ! -path "./vendor/*" | sort
echo
echo "=== Check if CreateGitHubRepo is actually used anywhere (not just defined) ==="
for file in $(find . -name "*.go" -type f ! -path "./vendor/*"); do
if grep -q "CreateGitHubRepo" "$file" 2>/dev/null; then
echo "=== File: $file ==="
grep -n "CreateGitHubRepo" "$file"
fi
done
echo
echo "=== Look for visibility parameter handling in the codebase ==="
rg -n "visibility\s*=" --type=go -B3 -A3
echo
echo "=== Check for any function that might be the actual entry point for repo creation ==="
rg -n "func\s+\w+.*repo\|repo.*Create" --type=go -iRepository: shinokada/gitstart
Length of output: 1104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the full Run function in cmd/root.go ==="
sed -n '36,66p' ./cmd/root.go
cat ./cmd/root.go | tail -50
echo
echo "=== Search for any imports of the repo package ==="
rg -n "repo\." --type=go
echo
echo "=== Look for where private/public flags might be converted to visibility ==="
rg -n "private\|public" --type=go -B2 -A2 | head -80Repository: shinokada/gitstart
Length of output: 3356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the exact content of cmd/root.go around the Run function ==="
sed -n '36,70p' ./cmd/root.go
echo
echo "=== Search for any other references to private/public flag handling ==="
rg -n "private\|public" ./cmd/root.go
echo
echo "=== Check if repo package is imported anywhere ==="
rg -n "internal/repo\|internal.repo" --type=go
echo
echo "=== Check internal/config to see if it might handle repo creation ==="
cat -n ./internal/config/config.go | head -50Repository: shinokada/gitstart
Length of output: 1818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify: Is CreateGitHubRepo actually called anywhere in non-test code? ==="
for file in $(find . -name "*.go" -type f ! -path "./vendor/*" ! -name "*_test.go"); do
if grep -q "CreateGitHubRepo" "$file" 2>/dev/null; then
echo "Found in: $file"
grep -n "CreateGitHubRepo" "$file"
fi
done
echo
echo "=== List all non-test Go files and check for repo imports ==="
find . -name "*.go" -type f ! -path "./vendor/*" ! -name "*_test.go" -exec grep -l "internal/repo\|repo\." {} \;
echo
echo "=== Double-check cmd/root.go for any actual repo package usage ==="
grep -n "import\|repo\." ./cmd/root.go || echo "No repo package usage in cmd/root.go"
echo
echo "=== Verify CreateGitHubRepo is exported but unused ==="
grep -n "^func CreateGitHubRepo" ./internal/repo/repo.go
</parameter>Repository: shinokada/gitstart
Length of output: 687
Default visibility to "private" or reject empty value before this function is integrated.
CreateGitHubRepo currently accepts an empty string for visibility without explicit handling (line 63), which leaves repository visibility to gh defaults. While the function is not yet called in production code, this design flaw should be fixed now: once the actual implementation in cmd/root.go converts the --private/--public flags to a visibility parameter, passing an empty string would make the automation non-deterministic and privacy-sensitive.
🔧 Proposed fix
switch visibility {
- case "":
- case "public", "private", "internal":
+ case "":
+ visibility = "private"
+ fallthrough
+ case "public", "private", "internal":
args = append(args, "--"+visibility)
default:
return fmt.Errorf("invalid visibility %q: expected public, private, or internal", visibility)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch visibility { | |
| case "": | |
| case "public", "private", "internal": | |
| args = append(args, "--"+visibility) | |
| default: | |
| return fmt.Errorf("invalid visibility %q: expected public, private, or internal", visibility) | |
| } | |
| switch visibility { | |
| case "": | |
| visibility = "private" | |
| fallthrough | |
| case "public", "private", "internal": | |
| args = append(args, "--"+visibility) | |
| default: | |
| return fmt.Errorf("invalid visibility %q: expected public, private, or internal", visibility) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repo/repo.go` around lines 62 - 68, CreateGitHubRepo currently lets
an empty visibility slip through the switch; change the handling so empty
visibility is not left to gh's defaults — either explicitly default it to
"private" or return an error. Concretely, update the start of the
CreateGitHubRepo visibility logic (the visibility variable and the switch block
that appends to args) to detect visibility=="" and set visibility = "private"
(or alternatively return fmt.Errorf("visibility must be one of: public, private,
internal") if you prefer to reject empty input), then continue with the existing
cases that append "--"+visibility to args.
…on, coverage-ci target
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)
.github/workflows/tests.yml (1)
35-41:⚠️ Potential issue | 🟡 MinorConditional chmod without conditional execution creates inconsistency.
The
chmodis now conditional (passes if file is missing), but line 41 still executes./tests/shellcheck.shunconditionally. If the script doesn't exist, the chmod step will pass due to|| true, but the workflow will fail at the execution step.Either the scripts should always exist (making conditional chmod unnecessary), or the execution should also be guarded.
💡 Proposed fix if scripts are optional
- name: Run ShellCheck - run: ./tests/shellcheck.sh + run: | + [ -f tests/shellcheck.sh ] && ./tests/shellcheck.sh || echo "shellcheck.sh not found, skipping"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 35 - 41, The workflow sets executable permissions conditionally for tests/run-tests.sh, tests/shellcheck.sh, and tests/test-dry-run.sh but still unconditionally runs ./tests/shellcheck.sh in the "Run ShellCheck" step, which will fail if the script is absent; update the "Run ShellCheck" step (referencing ./tests/shellcheck.sh and the step name "Run ShellCheck") to guard execution similarly—only invoke ./tests/shellcheck.sh when tests/shellcheck.sh exists (or remove the `|| true` from the chmod lines and ensure the script is always present), and ensure any other run steps for tests/run-tests.sh or tests/test-dry-run.sh follow the same conditional execution pattern.
♻️ Duplicate comments (1)
internal/repo/repo.go (1)
62-68:⚠️ Potential issue | 🟠 MajorEmpty visibility still falls through to
ghdefaults.The empty string case on line 63 is a no-op, leaving repository visibility to
ghCLI defaults. This was flagged in a previous review. Consider defaulting to "private" for safety-by-default behavior, or explicitly requiring a visibility value.💡 Proposed fix to default to private
switch visibility { case "": + visibility = "private" + fallthrough case "public", "private", "internal": args = append(args, "--"+visibility)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repo/repo.go` around lines 62 - 68, The switch on visibility in repo.go currently treats the empty string as a no-op, leaving gh CLI defaults; change the empty-case behavior to default to private by appending "--private" to args when visibility == "" (i.e., in the case "" branch of the switch), so the options become explicit and safe-by-default; adjust the switch that already handles "public","private","internal" and the default error branch accordingly to avoid falling through to gh defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 37-67: The Run function currently handles dry-run but leaves the
non-dry-run path as a placeholder; either implement the actual execution logic
or explicitly notify users that real execution is not yet implemented. Update
the Run handler (the anonymous Run func) so when dryRun is false and directory
is provided it either calls the real implementation (e.g., a function like
performProjectCreation/initializeProject/createProject with the existing flags:
directory, language, branch, message, private, public, description, quiet) or
prints a clear, user-facing message via the cobra command (e.g., cmd.Println or
cmd.Errorf) indicating "actual execution not implemented" and then returns;
replace the "// ...existing code for actual execution..." placeholder with one
of these two actions so the behavior is deterministic.
In `@internal/repo/repo.go`:
- Around line 60-72: The CreateGitHubRepo function builds an args slice starting
with "repo" so runCmd ends up trying to execute a non-existent "repo" binary;
update the args construction so the command executed is "gh" (i.e., ensure the
first element of args is "gh" and the following elements remain "repo",
"create", repoName, ...), leaving visibility handling and description logic
intact and still calling runCmd(dir, remoteCmdTimeout, args...); reference
CreateGitHubRepo, runCmd, and the args slice when making the change.
---
Outside diff comments:
In @.github/workflows/tests.yml:
- Around line 35-41: The workflow sets executable permissions conditionally for
tests/run-tests.sh, tests/shellcheck.sh, and tests/test-dry-run.sh but still
unconditionally runs ./tests/shellcheck.sh in the "Run ShellCheck" step, which
will fail if the script is absent; update the "Run ShellCheck" step (referencing
./tests/shellcheck.sh and the step name "Run ShellCheck") to guard execution
similarly—only invoke ./tests/shellcheck.sh when tests/shellcheck.sh exists (or
remove the `|| true` from the chmod lines and ensure the script is always
present), and ensure any other run steps for tests/run-tests.sh or
tests/test-dry-run.sh follow the same conditional execution pattern.
---
Duplicate comments:
In `@internal/repo/repo.go`:
- Around line 62-68: The switch on visibility in repo.go currently treats the
empty string as a no-op, leaving gh CLI defaults; change the empty-case behavior
to default to private by appending "--private" to args when visibility == ""
(i.e., in the case "" branch of the switch), so the options become explicit and
safe-by-default; adjust the switch that already handles
"public","private","internal" and the default error branch accordingly to avoid
falling through to gh defaults.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/tests.ymlMakefilecmd/root.gointernal/prompts/prompts_test.gointernal/repo/repo.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/prompts/prompts_test.go
…emented execution path
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Line 31: The job-level if currently restricts runs to pushes on main ("if:
github.event_name == 'push' && github.ref == 'refs/heads/main'") which prevents
workflow_dispatch; update that condition to also allow manual runs—for example
modify the if to include workflow_dispatch (e.g. "if: (github.event_name ==
'push' && github.ref == 'refs/heads/main') || github.event_name ==
'workflow_dispatch'") or remove the if entirely if the job should run for all
triggers; edit the if key in the workflow where it currently appears.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.ymlcmd/root.gointernal/repo/repo.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/repo/repo.go
- cmd/root.go
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
19-23: Align CI Go version with declared module minimum (or test both).Line 22 uses Go
1.22, whilego.moddeclaresgo 1.21. This can mask compatibility regressions for contributors running 1.21. Prefer either pinning CI to 1.21 or running a test matrix with both 1.21 and 1.22.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 19 - 23, The CI currently pins actions/setup-go@v5 to go-version '1.22' while go.mod specifies go 1.21; update the workflow to either pin the runner to '1.21' to match the module minimum or replace the single go-version entry with a matrix that tests both '1.21' and '1.22' (modify the actions/setup-go step where go-version: '1.22' is set to use the chosen approach), ensuring the test matrix or single-version value aligns with the go.mod declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 37-41: The "integration-tests" job currently only echoes
instructions instead of running tests; replace the two echo lines under the step
named "Run integration tests" with a real test invocation (for example: set
INTEGRATION=1 and run "go test -v ./..." or whatever project-specific
integration test command you use), ensure the test step fails the job on test
failures by removing or setting "continue-on-error: false", and if GitHub
auth/credentials are required gate the step with an if-condition or check that
required secrets/env vars exist so it only runs when credentials are available.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 19-23: The CI currently pins actions/setup-go@v5 to go-version
'1.22' while go.mod specifies go 1.21; update the workflow to either pin the
runner to '1.21' to match the module minimum or replace the single go-version
entry with a matrix that tests both '1.21' and '1.22' (modify the
actions/setup-go step where go-version: '1.22' is set to use the chosen
approach), ensuring the test matrix or single-version value aligns with the
go.mod declaration.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
40-44:⚠️ Potential issue | 🟠 Major
integration-testsstill behaves like a placeholder, not a real test job.Line 42-Line 43 only print guidance, and
continue-on-error: truekeeps the job green even if execution is later added incorrectly. This can mask regressions in mainline CI.Proposed fix
integration-tests: name: Integration Tests @@ - name: Run integration tests - run: | - echo "Integration tests are currently skipped (require GitHub auth)" - echo "To run locally: INTEGRATION=1 go test -v ./..." - continue-on-error: true + run: INTEGRATION=1 go test -v ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 40 - 44, The "Run integration tests" step currently only echoes messages and sets continue-on-error: true which hides missing or broken integration test runs; update the "Run integration tests" step so it executes real tests (e.g., run integration command like INTEGRATION=1 go test -v ./... or your actual test script) and remove continue-on-error: true so failures fail the job, or alternatively add a guard such as if: github.event.pull_request == null or require an explicit INTEGRATION env/secret check and exit non-zero when not configured; modify the step named "Run integration tests" to either run the real test command or explicitly skip with a non-quiet conditional so CI accurately reflects test status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/tests.yml:
- Around line 40-44: The "Run integration tests" step currently only echoes
messages and sets continue-on-error: true which hides missing or broken
integration test runs; update the "Run integration tests" step so it executes
real tests (e.g., run integration command like INTEGRATION=1 go test -v ./... or
your actual test script) and remove continue-on-error: true so failures fail the
job, or alternatively add a guard such as if: github.event.pull_request == null
or require an explicit INTEGRATION env/secret check and exit non-zero when not
configured; modify the step named "Run integration tests" to either run the real
test command or explicitly skip with a non-quiet conditional so CI accurately
reflects test status.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
36-45: Consider adding Go setup step for future-proofing.When this job is eventually enabled to run actual integration tests, it will need Go installed. Adding the setup step now (even though the tests are skipped) makes future enablement straightforward and keeps the job self-contained.
♻️ Suggested improvement
steps: - name: Checkout code uses: actions/checkout@v4 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.21' + - name: Run integration tests run: | echo "Integration tests require GitHub auth credentials." echo "To run locally: INTEGRATION=1 go test -v ./..." echo "Skipping in CI until credentials are configured as secrets." exit 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 36 - 45, Add a Go setup step before the "Run integration tests" step so the job installs Go even while tests are skipped; insert a step using actions/setup-go (e.g., actions/setup-go@v4) with a pinned go-version (or matrix variable) right after the "Checkout code" step so the "Run integration tests" step runs in a self-contained environment when unskipped in the future; ensure the step id or name is clear (e.g., "Setup Go") and place it above the "Run integration tests" step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 36-45: Add a Go setup step before the "Run integration tests" step
so the job installs Go even while tests are skipped; insert a step using
actions/setup-go (e.g., actions/setup-go@v4) with a pinned go-version (or matrix
variable) right after the "Checkout code" step so the "Run integration tests"
step runs in a self-contained environment when unskipped in the future; ensure
the step id or name is clear (e.g., "Setup Go") and place it above the "Run
integration tests" step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ececca6-8fd4-4ec6-9792-173ec8decba1
📒 Files selected for processing (1)
.github/workflows/tests.yml
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Closes #
📑 Description
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
Chores
Documentation
Tests