feat: add cross-platform Go CLI for container lifecycle management#401
feat: add cross-platform Go CLI for container lifecycle management#401
Conversation
Go binary (cli/) with Cobra commands: init wizard (huh forms), start/stop,
status, logs, doctor diagnostics, self-update from GitHub Releases, and
uninstall. Generates hardened Docker Compose from embedded template, polls
health endpoint, collects system diagnostics for bug reports.
Also includes:
- Fix backend Dockerfile healthcheck ('healthy' -> 'ok')
- CI: add cli.yml workflow (lint, test, build matrix, govulncheck, GoReleaser)
- CI: add dorny/paths-filter to ci.yml so Python/dashboard jobs skip on
unrelated changes
- Pre-commit: add conditional go-vet and go-test hooks
- Dependabot: add gomod ecosystem for cli/
- GoReleaser config with Homebrew tap + Scoop bucket
- Install scripts (curl|sh for Linux/macOS, PowerShell for Windows)
- Issue template for CLI installation failures
- Update README with CLI as primary Quick Start path
- Update CLAUDE.md with CLI package structure and commands
- Update PR review skills with Go-specific conditional agents
(go-reviewer, go-security-reviewer, go-conventions-enforcer)
- Update PR review skills to scan conversation context for issue refs
and always ask user when no issue is linked
- Refactor paths.go, docker/client.go, selfupdate/updater.go to expose testable core functions (dataDirForOS, InstallHint/DaemonHint, CheckFromURL, ReplaceAt) enabling cross-platform unit tests without real Docker/GitHub - Add comprehensive tests: selfupdate archive extraction (tar.gz/zip), checksum verification, mock HTTP server for Check/Download, ReplaceAt filesystem test, diagnostics Collect smoke test, config round-trip tests, health check edge cases (initial delay, cancellation, last error) - Target Go 1.26 (matches installed version) in go.mod and .golangci.yml - Add golangci-lint pre-commit hook (pre-push, conditional on cli/**/*.go) - Add .golangci.yml config with errcheck, govet, staticcheck, bodyclose, errorlint, noctx, prealloc, unconvert, unparam - CI: run Go tests on ubuntu/macos/windows matrix for real platform coverage
Security (Critical): - Require checksum verification for self-update (no silent bypass) - Validate user input before compose template rendering (port range, path validation, YAML quoting for DockerSock) - Validate install.sh version string against semver pattern - Validate asset URLs against expected GitHub domain prefix - Add io.LimitReader to all HTTP response reads (prevent memory exhaustion) - Add archive entry size guards (prevent zip bomb / tar bomb) - Remove container logs from GitHub issue URL body (may contain secrets) - Use os.CreateTemp for self-update temp file (prevent TOCTOU race) - Make install.sh checksum verification mandatory (fatal on missing tool) - Fix install.ps1 regex-based hash extraction with exact string match Correctness: - Fix port values silently discarded in init wizard (strPtr binding bug) - Fix fmt.Sscanf truncating paths with spaces — use strings.TrimSpace - Fix Windows Docker socket default (//./pipe/docker_engine) - Fix wmic deprecation — use fsutil on Windows for disk info - Use errors.Is for io.EOF comparison - Add http.Client timeout to health checks and self-update requests Conventions: - Warn on composeRun errors in update/uninstall instead of silent discard - Warn when Docker unavailable during uninstall volume removal - Validate --tail flag in logs command - Validate DataDir as absolute path when loading config Infra/CI: - Add persist-credentials: false to release checkout - Pin golangci-lint to v2.11.3 (not latest) - Add GORELEASER_TAP_TOKEN for homebrew-tap/scoop-bucket external repos - Add changes job result to ci-pass gate - Use local hook for golangci-lint pre-commit (cd into cli/) Docs: - Document cd exception for Go CLI commands in CLAUDE.md - Add Go hooks to pre-push hooks list - Update README Status (CLI no longer "remaining") - Fix scoop install to show bucket add step - Clarify src/ai_company/cli/ as superseded by Go binary - Add Go 1.26+ to Dependencies section Pre-reviewed by 6 agents, 40 findings addressed
…tomation Security/Critical: - Wire imageTag validation in compose.Generate (was dead code in init.go) - Add yamlStr template function to prevent YAML injection via JWTSecret/LogLevel - Replace strings.Fields(ComposePath) with ComposeCmd []string slice - Fix GoReleaser env var mismatch (GORELEASER_KEY → GORELEASER_TAP_TOKEN) - Add version validation + arch detection to PowerShell installer - Validate service names in logs command to prevent compose arg injection - Validate data dir as absolute path and docker sock for unsafe chars Code quality: - Refactor runInit (130→3 functions), runUninstall (98→3), runUpdate (76→3), runStatus (61→4 helpers), CheckFromURL (59→3 helpers) - Remove dead httpGet function from selfupdate - Remove unused --verbose flag - Fix os.IsNotExist → errors.Is(err, os.ErrNotExist) in 4 files - Fix config.Load returning DefaultState ignoring dataDir argument - Fix uninstall running compose down twice - Use http.NewRequestWithContext in diagnostics and status (noctx) - Add health check after restart in update command - Add resource usage display (docker stats) in status command - Add minimum Docker/Compose version checking - Resolve symlinks in --data-dir to prevent traversal - Use random suffix for .old file on Windows self-update - Lowercase Go error strings per convention - Handle discarded error in update command - Add -- separator in logs compose args - Fix ComposeExec misleading comment - Dev build warning in update command Install scripts: - Rewrite with pinned version + checksums (updated by release automation) - Fall back to runtime API + checksum download when SYNTHORG_VERSION overrides - Add post-GoReleaser CI step: pins checksums in scripts, commits to main, appends install instructions + checksum table to GitHub Release notes Container hardening: - Add user: directive (65532:65532 backend/sandbox, 101:101 web) - Add deploy.resources.limits (memory + CPU per service) Tests: - Handle errcheck violations in test handlers (w.Write, Close, WriteHeader) - Fix TestSaveFilePermissions to verify 0600 permissions - Fix TestEnsureDirIdempotent to check MkdirAll error - Rename TestHttpGet → TestHTTPGet - Add TestCheckMinVersions, TestVersionAtLeast, TestIsUpdateAvailable - Update golden files for new template output Docs: - Update roadmap: remove CLI from "Remaining Work" (implemented) - Update tech-stack: CLI is Go (Cobra + huh), not TBD - Update operations: CLI is implemented, not Future/Typer+Rich - Update getting_started: cli/ is Go binary, not future - Update user_guide: add CLI quick start section - Fix CLAUDE.md: remove indirect deps (lipgloss, yaml.v3) from CLI deps Follow-up: get.synthorg.io redirect page → #399
Strip brews + scoops sections from .goreleaser.yml, remove GORELEASER_TAP_TOKEN references from CI, and update all docs (CLAUDE.md, README, tech-stack, operations) to reflect install scripts as the only distribution method alongside GitHub Releases.
… escaping CI: - Add cli-pass gate job to cli.yml (mirrors ci-pass pattern in ci.yml) - Disable Go cache in release job (zizmor cache-poisoning finding) - Add LicenseRef-scancode-google-patent-license-golang to dependency review allow-list (golang.org/x/* false positive) - Remove script pinning/PR step — install scripts always fetch from GitHub API at runtime (simpler, no branch protection issues) - Keep post-release step that appends checksums + install instructions to GitHub Release notes CLI: - Add isInteractive() TTY detection — update skips restart prompt in non-interactive mode, uninstall requires interactive terminal - findAssets returns explicit errors for missing platform asset or checksums.txt (consistent error handling) - yamlStr escapes $ to prevent Docker Compose variable interpolation - docker stats filtered by compose project container IDs (not all host containers) - install.sh usage changed to | bash (pipefail is bash-specific, not POSIX sh) Docs: - All install script references updated from | sh to | bash
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a complete Go-based CLI (synthorg) with interactive commands (init/start/stop/status/logs/update/doctor/uninstall/version), platform-aware config and compose generation, Docker/Compose integration, self-update via GitHub Releases, install scripts, CI/workflow updates, tests, and docs adjustments. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Dependency ReviewThe following issues were found:
License Issuescli/go.mod
OpenSSF ScorecardScorecard details
Scanned Files
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement to the SynthOrg project by introducing a dedicated, cross-platform Go CLI. This new command-line interface provides a streamlined and secure way for users to initialize, manage, and troubleshoot their SynthOrg containerized environment. It simplifies common operational tasks, improves system diagnostics, and establishes a robust foundation for future command-line interactions with the application. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR introduces a full cross-platform Go CLI ( Remaining issues found:
Confidence Score: 4/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: cli/internal/selfupdate/updater.go
Line: 124-132
Comment:
**Pre-release builds never receive stable-release updates**
`compareSemver` uses `strings.FieldsFunc` to extract the leading numeric run from each version segment. For a patch value like `"3-beta"` it returns `["3"]`, and for `"3"` (the stable release) it also returns `["3"]` — so `compareSemver("1.2.3", "1.2.3-beta")` returns `0`. Consequently `isUpdateAvailable("1.2.3-beta", "1.2.3")` evaluates to `compareSemver("1.2.3", "1.2.3-beta") > 0` = `false`, and users on any pre-release build will silently never be offered the corresponding stable release.
A minimal fix is to treat a stable version as strictly greater than a pre-release with the same major.minor.patch when the stable release is the "latest":
```suggestion
func isUpdateAvailable(current, latest string) bool {
cur := strings.TrimPrefix(current, "v")
if cur == "dev" {
return true
}
lat := strings.TrimPrefix(latest, "v")
cmp := compareSemver(lat, cur)
if cmp != 0 {
return cmp > 0
}
// Same numeric version: stable (no pre-release) beats a pre-release.
latHasPre := strings.ContainsRune(lat, '-')
curHasPre := strings.ContainsRune(cur, '-')
return !latHasPre && curHasPre
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/internal/selfupdate/updater.go
Line: 311-331
Comment:
**URL scheme not validated after redirects**
The redirect-host guard checks `resp.Request.URL.Hostname()` but not the scheme. Go's `http.Client` will follow HTTPS → HTTP redirects by default (the `CheckRedirect` policy does not enforce scheme preservation). An attacker who could inject an HTTP redirect from an allowed domain (e.g. `objects.githubusercontent.com`) would bypass TLS for the binary download; the checksum check still runs, but the download is exposed to passive network interception.
Consider adding a scheme check alongside the host check:
```go
finalURL := resp.Request.URL
if finalURL.Scheme != "https" {
return nil, fmt.Errorf("download redirected to non-HTTPS URL %q", finalURL.String())
}
if !AllowedDownloadHosts[finalURL.Hostname()] {
return nil, fmt.Errorf("download redirected to unexpected host %q", finalURL.Hostname())
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/cmd/init.go
Line: 46-48
Comment:
**Duplicate comment**
The same comment appears twice on consecutive lines — a copy-paste artifact:
```suggestion
// Warn if re-initializing over existing config (JWT secret will change).
// isInteractive() is already checked at function entry, so prompt is safe.
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/cmd/start.go
Line: 28
Comment:
**Unused `args` parameter**
`args` is never referenced inside `runStart`. The same pattern appears in `stop.go:24`. Using the blank identifier makes the intent explicit and avoids confusion:
```suggestion
func runStart(cmd *cobra.Command, _ []string) error {
```
And in `cli/cmd/stop.go` line 24:
```go
func runStop(cmd *cobra.Command, _ []string) error {
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 3b62efb |
There was a problem hiding this comment.
Pull request overview
Adds a standalone, cross-platform Go-based synthorg CLI to manage the Dockerized SynthOrg deployment lifecycle (init/start/stop/status/logs/update/doctor/uninstall/version), plus release/distribution automation and documentation updates.
Changes:
- Introduces a new Go module under
cli/(Cobra commands, compose generation, health checks, diagnostics, self-update). - Adds installation scripts + GoReleaser config and a dedicated CLI CI workflow (lint/test/build/vulncheck/release).
- Updates docs/README and tweaks backend Dockerfile healthcheck status to match API (
ok).
Reviewed changes
Copilot reviewed 51 out of 53 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/user_guide.md | Adds CLI-based quick start; keeps manual compose path. |
| docs/roadmap/index.md | Removes “CLI may not be needed” roadmap item. |
| docs/getting_started.md | Updates project layout description (now mentions CLI). |
| docs/design/operations.md | Marks CLI as implemented and documents its scope. |
| docs/architecture/tech-stack.md | Sets CLI tech stack to Go/Cobra/huh. |
| docker/backend/Dockerfile | Updates healthcheck expected status to ok. |
| cli/testdata/compose_default.yml | Golden file for default compose generation. |
| cli/testdata/compose_custom_ports.yml | Golden file for custom port compose generation. |
| cli/scripts/install.sh | Bash installer for Linux/macOS with checksum verification. |
| cli/scripts/install.ps1 | PowerShell installer for Windows with checksum verification + PATH update. |
| cli/main.go | CLI entry point. |
| cli/internal/version/version.go | Build-time version variables (ldflags). |
| cli/internal/selfupdate/updater.go | GitHub Releases check/download/verify/extract/replace implementation. |
| cli/internal/selfupdate/updater_test.go | Self-update unit tests (assets, checksum, extraction, download, replace). |
| cli/internal/health/check.go | Backend health polling helper. |
| cli/internal/health/check_test.go | Health polling tests. |
| cli/internal/docker/client.go | Docker/Compose detection + command exec helpers + min version warnings. |
| cli/internal/docker/client_test.go | Docker helper tests. |
| cli/internal/diagnostics/collect.go | Diagnostic collection + text formatter + disk info. |
| cli/internal/diagnostics/collect_test.go | Diagnostics tests (truncate, formatting, no panic). |
| cli/internal/config/state.go | Persisted CLI state (config.json) load/save. |
| cli/internal/config/state_test.go | State persistence tests + permissions checks. |
| cli/internal/config/paths.go | Platform data-dir resolution + EnsureDir. |
| cli/internal/config/paths_test.go | Data-dir + EnsureDir tests. |
| cli/internal/compose/generate.go | Compose template rendering + input validation + YAML-safe string helper. |
| cli/internal/compose/generate_test.go | Compose generation tests + golden comparisons. |
| cli/internal/compose/compose.yml.tmpl | Embedded compose template (hardening + optional sandbox). |
| cli/cmd/root.go | Root command, --data-dir, TTY detection helper. |
| cli/cmd/init.go | Interactive init wizard; writes compose + config. |
| cli/cmd/start.go | Pull + up + health check. |
| cli/cmd/stop.go | Stack shutdown. |
| cli/cmd/status.go | Shows versions, container state, docker stats, health response. |
| cli/cmd/logs.go | Logs wrapper with input validation. |
| cli/cmd/update.go | Self-update + image pull + optional restart + health check. |
| cli/cmd/doctor.go | Collects diagnostics, writes file, prints prefilled GitHub issue URL. |
| cli/cmd/uninstall.go | Interactive teardown (containers, data dir, optional binary removal). |
| cli/cmd/version.go | Prints CLI version/build info. |
| cli/go.mod | Defines CLI module + dependencies. |
| cli/go.sum | Dependency lock hashes. |
| cli/.goreleaser.yml | GoReleaser build/archive/checksum/release config. |
| cli/.golangci.yml | golangci-lint config for CLI. |
| README.md | Adds CLI install + usage to Quick Start; updates status section. |
| CLAUDE.md | Documents cli/ module tooling and repo layout updates. |
| .pre-commit-config.yaml | Adds pre-push Go lint/vet/test hooks gated to cli/**/*.go. |
| .gitignore | Ignores CLI build artifacts and coverage output. |
| .dockerignore | Excludes cli/ from Docker build context. |
| .github/workflows/dependency-review.yml | Allows additional Go-related license identifier. |
| .github/workflows/cli.yml | New CLI workflow (lint/test/build/vulncheck/release notes append). |
| .github/workflows/ci.yml | Adds domain path filtering for Python/Docker/Web jobs. |
| .github/dependabot.yml | Adds Dependabot updates for cli/ gomod ecosystem. |
| .github/ISSUE_TEMPLATE/installer-failure.yml | New issue template for CLI install failures. |
| .claude/skills/pre-pr-review/SKILL.md | Updates skill to classify/validate CLI changes and run Go checks. |
| .claude/skills/aurelio-review-pr/SKILL.md | Updates skill to classify CLI changes and run Go-focused reviewers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cli/internal/compose/generate.go
Outdated
| if p.Sandbox && p.DockerSock != "" { | ||
| if strings.ContainsAny(p.DockerSock, "\"'`$\n\r{}[]") { | ||
| return fmt.Errorf("docker socket path %q contains unsafe characters", p.DockerSock) | ||
| } | ||
| } |
cli/cmd/init.go
Outdated
| func runInit(cmd *cobra.Command, args []string) error { | ||
| answers, err := runSetupForm() | ||
| if err != nil { | ||
| return err | ||
| } |
| expected, err := os.ReadFile(golden) | ||
| if err != nil { | ||
| // Golden doesn't exist yet — create it. | ||
| if err := os.MkdirAll(filepath.Dir(golden), 0o755); err != nil { | ||
| t.Fatalf("create testdata dir: %v", err) | ||
| } | ||
| if err := os.WriteFile(golden, actual, 0o644); err != nil { | ||
| t.Fatalf("write golden: %v", err) | ||
| } | ||
| t.Logf("created golden file %s", golden) | ||
| return | ||
| } |
| if _, err := tmpFile.Write(binaryData); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return fmt.Errorf("writing new binary: %w", err) | ||
| } | ||
| if err := tmpFile.Chmod(0o755); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return fmt.Errorf("setting permissions: %w", err) | ||
| } | ||
| tmpFile.Close() | ||
|
|
| // On Windows, we can't overwrite the running binary — rename first. | ||
| // Use a random suffix to avoid predictable paths. | ||
| var oldPath string | ||
| if runtime.GOOS == "windows" { | ||
| oldFile, err := os.CreateTemp(dir, binaryName+".old.*.tmp") | ||
| if err != nil { | ||
| os.Remove(tmpPath) | ||
| return fmt.Errorf("creating temp file for old binary: %w", err) | ||
| } | ||
| oldPath = oldFile.Name() | ||
| oldFile.Close() | ||
| os.Remove(oldPath) // Remove so Rename can use the path. | ||
|
|
||
| if err := os.Rename(execPath, oldPath); err != nil { | ||
| os.Remove(tmpPath) | ||
| return fmt.Errorf("renaming current binary: %w", err) | ||
| } | ||
| } |
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() |
| src/ai_company/ # Main package (src layout) | ||
| api/ # Litestar REST + WebSocket routes | ||
| budget/ # Cost tracking and spending controls | ||
| cli/ # CLI interface (future) | ||
| cli/ # Go CLI binary (container lifecycle management) | ||
| communication/ # Inter-agent message bus |
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive pull request that introduces a full-featured Go CLI for managing the application's lifecycle. The code is well-structured, secure, and includes thorough testing and documentation. I've identified a few areas for improvement to enhance robustness and maintainability, including a critical issue regarding an unhandled error that could lead to a panic. My other comments focus on making version comparisons more reliable, improving diagnostic information, and making parts of the code more maintainable.
cli/internal/diagnostics/collect.go
Outdated
| // Health endpoint. | ||
| healthURL := fmt.Sprintf("http://localhost:%d/api/v1/health", state.BackendPort) | ||
| client := &http.Client{Timeout: 5 * time.Second} | ||
| req, _ := http.NewRequestWithContext(ctx, http.MethodGet, healthURL, nil) |
There was a problem hiding this comment.
| issueURL := fmt.Sprintf( | ||
| "https://github.com/Aureliolo/synthorg/issues/new?title=%s&labels=type%%3Abug&body=%s", | ||
| url.QueryEscape(issueTitle), | ||
| encodedBody, | ||
| ) |
There was a problem hiding this comment.
The GitHub repository URL is hardcoded here. If the repository is ever moved or renamed, this link will break. It's more maintainable to define this as a constant, perhaps in the internal/version package, so it can be changed in one place.
You could also consider using the installer-failure.yml issue template you've added in this PR to provide a more structured bug report form for users by adding template=installer-failure.yml to the URL query.
cli/internal/diagnostics/collect.go
Outdated
| func diskInfo(ctx context.Context) string { | ||
| var name string | ||
| var args []string | ||
| switch runtime.GOOS { | ||
| case "windows": | ||
| // Use fsutil (available on all Windows versions) instead of deprecated wmic. | ||
| name = "fsutil" | ||
| args = []string{"volume", "diskfree", "C:"} | ||
| default: | ||
| name = "df" | ||
| args = []string{"-h", "/"} | ||
| } | ||
| cmd := exec.CommandContext(ctx, name, args...) | ||
| var out bytes.Buffer | ||
| cmd.Stdout = &out | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Sprintf("unavailable: %v", err) | ||
| } | ||
| return strings.TrimSpace(out.String()) | ||
| } |
There was a problem hiding this comment.
The diskInfo function currently checks the disk space for the root directory (/) or C:. This might not be where the application data is stored, especially if the user has configured a custom data directory on a different partition.
To provide more relevant diagnostics, consider modifying this function to accept the data directory path and report the disk space for that specific path. You can pass state.DataDir from the Collect function.
| func versionAtLeast(got, min string) bool { | ||
| got = strings.TrimPrefix(got, "v") | ||
| min = strings.TrimPrefix(min, "v") | ||
|
|
||
| gParts := strings.SplitN(got, ".", 3) | ||
| mParts := strings.SplitN(min, ".", 3) | ||
|
|
||
| for i := range 3 { | ||
| var g, m int | ||
| if i < len(gParts) { | ||
| // Strip non-numeric suffixes (e.g. "1-rc1"). | ||
| numStr := strings.FieldsFunc(gParts[i], func(r rune) bool { | ||
| return r < '0' || r > '9' | ||
| }) | ||
| if len(numStr) > 0 { | ||
| g, _ = strconv.Atoi(numStr[0]) | ||
| } | ||
| } | ||
| if i < len(mParts) { | ||
| m, _ = strconv.Atoi(mParts[i]) | ||
| } | ||
| if g > m { | ||
| return true | ||
| } | ||
| if g < m { | ||
| return false | ||
| } | ||
| } | ||
| return true // equal | ||
| } |
There was a problem hiding this comment.
This custom version comparison logic is brittle. It ignores potential errors from strconv.Atoi and might not handle all valid version string formats correctly, especially complex pre-release tags or non-numeric parts.
For improved robustness and clarity, consider using a dedicated semantic versioning library like github.com/Masterminds/semver/v3. This would handle various version formats and comparisons more reliably.
If you prefer to avoid adding a new dependency, you should at least handle the error from strconv.Atoi to avoid silent failures on unexpected version formats.
| os.Remove(tmpPath) | ||
| return fmt.Errorf("writing new binary: %w", err) | ||
| } | ||
| if err := tmpFile.Chmod(0o755); err != nil { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.pre-commit-config.yaml (1)
54-91: 🧹 Nitpick | 🔵 TrivialConsider grouping Go hooks together for maintainability.
The Go hooks are currently split:
golangci-lintappears before the Python hooks (mypy,pytest-unit), whilego-vetandgo-testappear after. Grouping all Go hooks together would improve readability and make future maintenance easier.Additionally, note that
golangci-lintincludesvetchecks by default, so the separatego-vethook is somewhat redundant. This is harmless sincego vetis fast, but you could consider removing it if you want to reduce pre-push time.♻️ Suggested reordering (Python hooks first, then Go hooks)
- repo: local hooks: - id: mypy name: mypy type-check entry: uv run mypy src/ tests/ language: system pass_filenames: false stages: [pre-push] - id: pytest-unit name: pytest unit tests entry: uv run pytest tests/ -m unit -n auto -q language: system pass_filenames: false stages: [pre-push] - id: golangci-lint name: golangci-lint (CLI) entry: bash -c 'cd cli && golangci-lint run' language: system files: ^cli/.*\.go$ pass_filenames: false stages: [pre-push] - id: go-vet name: go vet (CLI) entry: bash -c 'cd cli && go vet ./...' language: system files: ^cli/.*\.go$ pass_filenames: false stages: [pre-push] - id: go-test name: go test (CLI) entry: bash -c 'cd cli && go test ./...' language: system files: ^cli/.*\.go$ pass_filenames: false stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 54 - 91, The Go hooks are split across the file (golangci-lint is separated from go-vet and go-test) which harms readability; reorder the hooks so mypy and pytest-unit are grouped together and golangci-lint, go-vet, go-test are grouped together (move the entries with ids golangci-lint, go-vet, and go-test adjacent to each other) and optionally remove the redundant go-vet hook (id: go-vet) because golangci-lint already runs vet checks if you want to reduce pre-push time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/pre-pr-review/SKILL.md:
- Around line 205-221: The ordered list items for "Vet:", "Test:", and "Build
check:" use explicit numeric prefixes (10., 11., 12.) which triggers
markdownlint MD029; change those numeric prefixes to "1." for each list entry so
the markdown uses the allowed repeating "1." ordered-list style (locate the
three list entries titled "Vet:", "Test:", and "Build check:" in SKILL.md and
update their prefixes accordingly).
- Around line 90-92: The change missed categorizing module files under the cli
rules, so add patterns for cli/go.mod and cli/go.sum to the existing rule set
(either extend the cli_go rule to include "cli/go.mod" and "cli/go.sum" or
create/merge a specific cli_module rule) so Go checks and Go-agent selection run
for dependency-only updates; update the rule names referenced (cli_go,
cli_config) and any file-glob logic that determines Phase 2 vet/test/build
selection to include these module filenames.
In @.github/ISSUE_TEMPLATE/installer-failure.yml:
- Around line 31-36: Update the OS options list under the "options:" key to
include "Windows (arm64)" alongside the existing entries so Windows arm64 users
can select the correct installer; locate the options block that currently lists
"macOS (Apple Silicon / arm64)", "macOS (Intel / amd64)", "Linux (amd64)",
"Linux (arm64)", and "Windows (amd64)" and add a new entry "Windows (arm64)" to
that list.
In @.github/workflows/ci.yml:
- Around line 35-46: The CI matrix currently lists path filters for jobs like
python, dashboard, and docker but omits workflow files, allowing changes under
.github/workflows/** to bypass CI; update the workflow file so the path filters
include '.github/workflows/**' (add it alongside the existing entries under the
same filter grouping that contains 'src/**'/'web/**'/'docker/**') to ensure PRs
that only modify workflow logic trigger the matrix jobs.
In @.github/workflows/cli.yml:
- Around line 55-58: The CI workflows use actions/setup-go with caching which
triggered cache-poisoning warnings; for each job using that action (lint, test,
build, vuln — and confirm release already disables cache) update the setup-go
step (the uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 block)
to include cache: false (or remove cache-related inputs like
cache-dependency-path/go.sum) to disable caching, or alternatively add a concise
justification comment above the step explaining why you accept the risk
(read-only permissions/no artifact publishing) to suppress the warning; apply
the change to the occurrences referenced (around the setup-go blocks at the
other locations).
- Around line 148-149: The "Install govulncheck" step currently uses go install
golang.org/x/vuln/cmd/govulncheck@latest which should be pinned to an explicit
release; update that run command to use a concrete version tag (for example
replace `@latest` with a specific version like `@v0.5.0` or your chosen vetted
release) so the workflow installs a reproducible govulncheck binary, and
optionally document the selected version in a variable or comment for future
updates.
In @.pre-commit-config.yaml:
- Around line 61-62: Remove the extra blank line between the golangci-lint and
mypy hooks in the pre-commit configuration so hooks are separated by a single
blank line consistent with the rest of the file; locate the consecutive blank
lines that appear immediately after the "golangci-lint" hook entry and delete
one so only one empty line remains before the "mypy" hook.
In `@cli/cmd/init.go`:
- Around line 13-17: The init command currently hardcodes created compose image
tags to "latest"; change it to use the CLI build version as the default tag and
only fall back to "latest" for dev builds. In the init command handler (e.g.,
initCmd / NewInitCmd) obtain the build version (the package-level version
variable or equivalent), pass that tag into the compose generator (functions
like compose.NewCompose / compose.Generate or the struct that builds
compose.yml) and update the flag/default logic so if version is empty or
contains "dev" you use "latest", otherwise use the exact build version string as
the image tag; adjust any places around lines ~139-148 that set the tag default
to implement this behavior. Ensure the change propagates to compose output so
generated compose.yml embeds the chosen tag.
In `@cli/cmd/logs.go`:
- Around line 58-64: The validation currently allows zero because it only
rejects n < 0 while the error message says "positive integer"; update the
validation in the tail parsing block (the tail variable derived from logTail,
where strconv.Atoi is called) to reject zero as well by changing the check to
treat n <= 0 as invalid, so the code returns the existing error message `--tail
must be a positive integer or 'all'` when n is 0 or negative.
In `@cli/cmd/root.go`:
- Around line 32-42: The resolveDataDir function currently only resolves
symlinks when a user-supplied dataDir is set; to make behavior consistent,
always run filepath.EvalSymlinks on the final directory (whether from dataDir or
config.DataDir()) and return the resolved path if EvalSymlinks succeeds,
otherwise fall back to the original value; update resolveDataDir to pick the
directory (use dataDir if non-empty else config.DataDir()), then call
filepath.EvalSymlinks on that value and return the resolved path on success, or
the original path on error.
In `@cli/cmd/start.go`:
- Around line 68-75: The health check branch currently prints warnings but still
returns nil; modify the error path after calling health.WaitForHealthy(...) so
the command returns a non-zero exit (e.g., return fmt.Errorf("startup failed:
%w", err)) after printing the diagnostic messages; update the function
containing the call to health.WaitForHealthy (the command Run/RunE handler in
start.go) to propagate the error instead of falling through to a successful
return.
In `@cli/cmd/status.go`:
- Around line 111-129: The status command currently treats any HTTP response
with a JSON body as healthy; update the handling after client.Do(req) to check
resp.StatusCode and treat non-2xx codes as error responses: after reading body
into "body" and unmarshalling into "hr", if resp.StatusCode is not in the
200–299 range print the JSON error plus the HTTP status (e.g., "Backend:
<prettyJSON(hr)> (HTTP <StatusCode>)") and return similarly for non-JSON bodies,
otherwise for 2xx responses keep the existing prettyJSON output; reference
variables/functions resp, resp.StatusCode, body, hr, prettyJSON and ensure the
non-2xx branch is executed before assuming success.
In `@cli/cmd/uninstall.go`:
- Around line 130-140: The uninstall path uses os.Executable() which may return
a symlink so removing that path can leave the real binary behind; update the
removeBinary branch (the code that calls os.Executable() and os.Remove) to
resolve symlinks (use filepath.EvalSymlinks on execPath) before attempting
removal so it matches selfupdate.ReplaceAt behavior, and add "path/filepath" to
imports; preserve the existing error handling/logging but operate on the
evaluated path.
- Around line 107-112: The code calls os.RemoveAll(state.DataDir) without
validating the target; add safety checks in the uninstall logic around
removeData to prevent deleting critical paths: ensure state.DataDir is
non-empty, not "/" or drive root, not the user's home directory, and matches an
expected application data base path or contains an app-specific marker file
before proceeding; if any check fails, return an error and avoid calling
os.RemoveAll(state.DataDir); optionally require an explicit confirmation flag
(e.g., --force) or interactive confirmation via
cmd.OutOrStdout()/cmd.InOrStdin() before performing the removal.
In `@cli/cmd/update.go`:
- Around line 114-120: The restart currently ignores errors from composeRun(...,
"down") and proceeds to composeRun(..., "up", "-d"), risking duplicate
containers; update the restart flow in update.go to abort when composeRun(ctx,
cmd, info, state.DataDir, "down") returns an error by returning a wrapped error
(e.g., return fmt.Errorf("stopping containers: %w", err)) instead of only
logging a warning, or alternatively change the subsequent composeRun up call to
composeRun(ctx, cmd, info, state.DataDir, "up", "-d", "--force-recreate") to
force a clean recreate; locate these calls to composeRun (the "down" and "up"
invocations) and apply one of the two fixes consistently.
In `@cli/internal/compose/compose.yml.tmpl`:
- Around line 65-85: Add a Docker healthcheck to the sandbox service in the
compose template so orchestration can verify it is healthy; update the sandbox
block (service name "sandbox" in compose.yml.tmpl) to include a healthcheck
section (e.g., test, interval, timeout, retries) that runs an appropriate check
for the sandbox container, and if other services depend on it adjust their
depends_on to use condition service_healthy (or compose v3 healthcheck-aware
depends_on) so startup ordering and health gating work correctly.
In `@cli/internal/compose/generate_test.go`:
- Around line 172-182: The current os.ReadFile(golden) error path silently
creates the missing golden file; change it to fail the test when the golden file
is absent unless the UPDATE_GOLDEN env var is set. In the ReadFile error branch
(around variables expected, err, golden, actual) detect os.IsNotExist(err) and
call t.Fatalf("golden file %s missing; run tests with UPDATE_GOLDEN=1 to
regenerate", golden), and only perform the MkdirAll/WriteFile creation when
os.Getenv("UPDATE_GOLDEN") == "1"; keep existing file mode bits and log message
when creating.
In `@cli/internal/compose/generate.go`:
- Around line 86-91: In validateParams, after the existing range checks for
p.BackendPort and p.WebPort, add a check that returns an error if p.BackendPort
== p.WebPort; use a clear error message like "backend and web ports must be
different" and reference p.BackendPort and p.WebPort in the message so callers
know the conflicting values; update validateParams (the function containing the
current range validation) to perform this equality check before successful
return.
In `@cli/internal/config/paths.go`:
- Around line 17-23: DataDir currently falls back to "." when os.UserHomeDir()
fails, which is unsafe; change DataDir() so that on error from os.UserHomeDir()
it logs a warning (use the package logger or log.Printf) including the original
error and then use a safer fallback such as os.TempDir() (or return an error by
changing the signature to DataDir() (string, error) and propagating that)
instead of silently returning a relative "."; update all callers if you change
the signature and ensure you reference DataDir(), os.UserHomeDir(), and
dataDirForOS(...) when making the change.
In `@cli/internal/config/state_test.go`:
- Line 108: The test uses os.Getenv("OS") for platform detection which is
unreliable; replace that condition with runtime.GOOS (e.g., change if
os.Getenv("OS") != "Windows_NT" to if runtime.GOOS != "windows"), update any
casing/expected string accordingly, and add runtime to the imports in
state_test.go so the test reliably detects the OS using the standard library.
In `@cli/internal/config/state.go`:
- Around line 47-50: The missing-file branch (errors.Is(err, os.ErrNotExist))
returns defaults.DataDir = dataDir without normalizing/validating the path,
causing inconsistent behavior versus the later absolute-path validation; update
that branch in state.go to resolve and normalize dataDir (e.g., filepath.Abs +
filepath.Clean) before assigning to DefaultState().DataDir and return an error
if resolution fails so the same absolute-path invariants applied in the later
block (lines ~59-63) are enforced on first-run as well.
- Around line 54-65: The load is unmarshaling into a zero-value State so missing
fields overwrite defaults; instead initialize s with the default configuration
before unmarshaling (e.g., s := DefaultState() or a struct literal with default
ports/log level), then call json.Unmarshal(data, &s) to merge values onto those
defaults; keep the existing DataDir canonicalization/validation (s.DataDir =
filepath.Clean(...), IsAbs check) after the merge.
In `@cli/internal/diagnostics/collect_test.go`:
- Around line 73-80: TestCollectDoesNotPanic uses context.Background() and can
hang if probing stalls; update the test to create a timeout context (e.g., via
context.WithTimeout) and pass that to Collect, ensuring you call the cancel
function with defer to free resources; modify TestCollectDoesNotPanic to
construct ctx, defer cancel, then call Collect(ctx, state) so the test fails
fast instead of hanging.
In `@cli/internal/diagnostics/collect.go`:
- Around line 133-151: The diskInfo function should handle macOS explicitly to
ensure consistent output; add a case for runtime.GOOS == "darwin" inside
diskInfo to run a macOS-friendly command (for example use "df" with flags or
"diskutil info /" depending on desired detail) instead of the default Linux
path, then normalize/trim the output the same way; update the switch to include
case "darwin" and call the chosen command (referencing diskInfo, runtime.GOOS,
name, args, and the existing exec.CommandContext usage) so macOS produces
predictable disk info for later parsing.
- Around line 69-83: The call to http.NewRequestWithContext ignores its error
and may leave req nil; update the health check in collect.go to capture and
handle the error returned by http.NewRequestWithContext(ctx, http.MethodGet,
healthURL, nil) (the local variable req), and on error set r.HealthStatus to
"unreachable" and append a descriptive error to r.Errors (similar to the
client.Do error handling), returning or skipping the request execution if req is
nil so you never call client.Do with a nil request; ensure the logic around
client := &http.Client... and resp handling in the same block remains unchanged.
In `@cli/internal/docker/client_test.go`:
- Around line 39-40: The test expects FreeBSD to return a systemctl hint but
FreeBSD uses rc.d/service; update the DaemonHint behavior to handle "freebsd"
explicitly (function DaemonHint) and return a FreeBSD-appropriate hint such as
"Start the Docker daemon: service docker start" (or adjust the test in
cli/internal/docker/client_test.go to expect that FreeBSD string), ensuring the
test table entry for "freebsd" is updated to match the new string; modify only
DaemonHint and the test expectation so the default case remains unchanged for
other platforms.
In `@cli/internal/docker/client.go`:
- Around line 90-97: ComposeExec currently discards stdout/stderr so callers get
no context on failure; change ComposeExec (and where it calls composeArgs) to
capture command output (use cmd.CombinedOutput() or set cmd.Stdout/Stderr to a
buffer) and when cmd returns an error wrap/return a new error that includes the
original error plus the captured output (or log the output via the existing
logger) so callers receive stdout/stderr context on failures; ensure the
returned error preserves the underlying exit error and includes the output
string for debugging.
In `@cli/internal/health/check.go`:
- Around line 22-45: The WaitForHealthy function can panic if interval <= 0 and
may never run a probe before timing out; validate interval and run the first
probe immediately: inside WaitForHealthy, check that interval > 0 (return an
error if not) before calling time.NewTicker, perform an initial probe by calling
checkOnce(ctx, url) immediately after the initialDelay select (and handle/return
success if nil, or set lastErr if non-nil), then create the ticker and continue
the loop; reference WaitForHealthy, checkOnce, interval, initialDelay and
time.NewTicker when making these changes.
In `@cli/internal/selfupdate/updater.go`:
- Around line 262-276: The httpGetWithClient function lacks URL scheme
validation which can enable SSRF risks if reused; update httpGetWithClient to
parse the provided url (using net/url.Parse or equivalent), verify the scheme is
either "http" or "https" and that the host is non-empty, and return a clear
error when validation fails before creating the request; keep the existing
behavior (using client.Do, status check, and limited read) otherwise so callers
like Download remain compatible.
- Around line 122-126: The current isUpdateAvailable function wrongly treats any
cur != lat as an update; change isUpdateAvailable to parse both versions with a
semantic version parser (handle and strip leading "v"), treat the special "dev"
token as always up-to-date (return false), compare parsed semver objects
(major/minor/patch and pre-release rules) and only return true if latest >
current; if parsing fails for either version, fall back to a safe comparison
(e.g., use semver-like numeric comparison or string equality) but ensure you do
not return true when current is newer than latest; update references to
isUpdateAvailable accordingly.
In `@cli/scripts/install.ps1`:
- Around line 53-63: Normalize and trim the expected checksum before comparing
to the computed one: after extracting $ExpectedHash from the checksums.txt line
(the ($line -split '\s+')[0] assignment) ensure you apply a Trim() and ToLower()
on $ExpectedHash so that it is compared to $ActualHash (which is already
ToLower()) in the if ($ExpectedHash -ne $ActualHash) check; this prevents false
mismatches due to casing or surrounding whitespace when using Get-Content and
Get-FileHash.
In `@cli/scripts/install.sh`:
- Around line 91-98: The install block assumes INSTALL_DIR exists; ensure the
script creates the directory before moving the binary: check and create
INSTALL_DIR (e.g., with mkdir -p "$INSTALL_DIR") prior to invoking mv/chmod for
"${TMP_DIR}/${BINARY_NAME}" -> "${INSTALL_DIR}/${BINARY_NAME}"; keep existing
permission logic and sudo usage but perform the mkdir step in the same
permission context (use sudo when INSTALL_DIR creation requires elevated rights)
so both branches (writable and non-writable) succeed.
---
Outside diff comments:
In @.pre-commit-config.yaml:
- Around line 54-91: The Go hooks are split across the file (golangci-lint is
separated from go-vet and go-test) which harms readability; reorder the hooks so
mypy and pytest-unit are grouped together and golangci-lint, go-vet, go-test are
grouped together (move the entries with ids golangci-lint, go-vet, and go-test
adjacent to each other) and optionally remove the redundant go-vet hook (id:
go-vet) because golangci-lint already runs vet checks if you want to reduce
pre-push time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47a36b0e-ef07-4f85-a89c-75696f6ef256
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
.claude/skills/aurelio-review-pr/SKILL.md.claude/skills/pre-pr-review/SKILL.md.dockerignore.github/ISSUE_TEMPLATE/installer-failure.yml.github/dependabot.yml.github/workflows/ci.yml.github/workflows/cli.yml.github/workflows/dependency-review.yml.gitignore.pre-commit-config.yamlCLAUDE.mdREADME.mdcli/.golangci.ymlcli/.goreleaser.ymlcli/cmd/doctor.gocli/cmd/init.gocli/cmd/logs.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/stop.gocli/cmd/uninstall.gocli/cmd/update.gocli/cmd/version.gocli/go.modcli/internal/compose/compose.yml.tmplcli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/paths.gocli/internal/config/paths_test.gocli/internal/config/state.gocli/internal/config/state_test.gocli/internal/diagnostics/collect.gocli/internal/diagnostics/collect_test.gocli/internal/docker/client.gocli/internal/docker/client_test.gocli/internal/health/check.gocli/internal/health/check_test.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/updater_test.gocli/internal/version/version.gocli/main.gocli/scripts/install.ps1cli/scripts/install.shcli/testdata/compose_custom_ports.ymlcli/testdata/compose_default.ymldocker/backend/Dockerfiledocs/architecture/tech-stack.mddocs/design/operations.mddocs/getting_started.mddocs/roadmap/index.mddocs/user_guide.md
💤 Files with no reviewable changes (1)
- docs/roadmap/index.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: All Docker image builds use a single root `.dockerignore` (both backend and web images build with `context: .`). Images are tagged by CI with version from `pyproject.toml` (`[tool.commitizen].version`), semver, and SHA.
Applied to files:
.dockerignoreCLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting).
Applied to files:
CLAUDE.md.pre-commit-config.yaml.claude/skills/pre-pr-review/SKILL.md.github/workflows/ci.ymlcli/.golangci.yml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: All commits must be GPG/SSH signed on `main` branch—required by branch protection.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Pre-push hooks run mypy type-check and pytest unit tests as a fast gate before push. These are skipped in pre-commit.ci (dedicated CI jobs already run them).
Applied to files:
CLAUDE.md.pre-commit-config.yaml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: For GitHub issue queries, use `gh issue list` via Bash (not MCP tools)—MCP `list_issues` has unreliable field data.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Commit format: `<type>: <description>` with types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: After finishing an issue implementation, always create a feature branch (`<type>/<slug>`), commit, and push—do not create a PR automatically. Do not leave work uncommitted on main.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Applies to docker/Dockerfile* : Lint all Dockerfiles with hadolint—enforced via pre-commit hook locally and in CI via `dockerfile-lint` job.
Applied to files:
CLAUDE.md.pre-commit-config.yaml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Never create a PR directly using `gh pr create`—always use `/pre-pr-review` to run automated checks + review agents + fixes before creating the PR. For trivial/docs-only changes, use `/pre-pr-review quick` to skip agents but run automated checks. After PR exists, use `/aurelio-review-pr` for external reviewer feedback.
Applied to files:
CLAUDE.md.claude/skills/pre-pr-review/SKILL.md.claude/skills/aurelio-review-pr/SKILL.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Images are only pushed to GHCR after Trivy/Grype vulnerability scans pass. CVE triage is via `.github/.trivyignore.yaml` and `.github/.grype.yaml`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Applies to pyproject.toml : Organize dependencies in groups: `test` (pytest + plugins), `dev` (includes test + ruff, mypy, pre-commit, commitizen, pip-audit). Install via `uv sync`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Applies to docker/Dockerfile* : In Dockerfile definitions, follow CIS hardening best practices: use multi-stage builds (builder → setup → distroless runtime), Chainguard Python base image, non-root user (UID 65532), CIS-hardened configuration.
Applied to files:
docker/backend/Dockerfile
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: When review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to PR changes), fix them all—no deferring, no 'out of scope' skipping.
Applied to files:
.claude/skills/pre-pr-review/SKILL.md.claude/skills/aurelio-review-pr/SKILL.md
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Applies to web/src/**/*.{ts,tsx,vue} : Use TypeScript for Vue 3 web dashboard components. Enforce type checking via vue-tsc.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Preserve existing `Closes `#NNN`` references in PR issue references—never remove unless explicitly asked.
Applied to files:
.claude/skills/aurelio-review-pr/SKILL.md
🧬 Code graph analysis (17)
cli/main.go (1)
cli/cmd/root.go (1)
Execute(54-60)
cli/cmd/update.go (6)
cli/internal/version/version.go (1)
Version(6-6)cli/internal/selfupdate/updater.go (3)
Check(64-66)Download(155-179)Replace(182-188)cli/internal/config/state.go (1)
Load(43-66)cli/internal/docker/client.go (2)
Detect(34-66)ComposeExecOutput(100-107)cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/health/check.go (1)
WaitForHealthy(22-52)
cli/internal/docker/client_test.go (1)
cli/internal/docker/client.go (7)
InstallHint(122-131)DaemonHint(134-141)RunCmd(110-119)Info(22-29)ComposeExecOutput(100-107)ComposeExec(91-97)CheckMinVersions(69-78)
cli/cmd/root.go (1)
cli/internal/config/paths.go (1)
DataDir(17-23)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (2)
DataDir(17-23)EnsureDir(44-46)
cli/internal/config/state_test.go (2)
cli/internal/config/state.go (5)
DefaultState(26-34)State(14-23)Save(69-78)Load(43-66)StatePath(37-39)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/stop.go (3)
cli/internal/config/state.go (1)
Load(43-66)cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/docker/client.go (1)
Detect(34-66)
cli/internal/health/check_test.go (1)
cli/internal/health/check.go (1)
WaitForHealthy(22-52)
cli/cmd/uninstall.go (3)
cli/internal/config/state.go (2)
Load(43-66)State(14-23)cli/internal/docker/client.go (2)
Detect(34-66)Info(22-29)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/init.go (3)
cli/internal/config/paths.go (2)
DataDir(17-23)EnsureDir(44-46)cli/internal/config/state.go (4)
StatePath(37-39)DefaultState(26-34)State(14-23)Save(69-78)cli/internal/compose/generate.go (2)
ParamsFromState(43-54)Generate(58-76)
cli/internal/compose/generate_test.go (3)
cli/internal/compose/generate.go (3)
Params(31-40)Generate(58-76)ParamsFromState(43-54)cli/internal/config/state.go (1)
State(14-23)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/internal/diagnostics/collect.go (4)
cli/internal/config/state.go (1)
State(14-23)cli/internal/version/version.go (2)
Version(6-6)Commit(7-7)cli/internal/docker/client.go (2)
Detect(34-66)ComposeExecOutput(100-107)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/internal/selfupdate/updater_test.go (1)
cli/internal/selfupdate/updater.go (5)
Release(42-45)Asset(48-51)Download(155-179)CheckFromURL(70-89)ReplaceAt(192-252)
cli/internal/diagnostics/collect_test.go (3)
cli/internal/diagnostics/collect.go (2)
Report(22-37)Collect(40-98)cli/internal/config/state.go (1)
State(14-23)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/version.go (1)
cli/internal/version/version.go (3)
Version(6-6)Commit(7-7)Date(8-8)
cli/cmd/status.go (4)
cli/internal/config/state.go (2)
Load(43-66)State(14-23)cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/docker/client.go (4)
Detect(34-66)Info(22-29)ComposeExecOutput(100-107)RunCmd(110-119)cli/internal/version/version.go (2)
Version(6-6)Commit(7-7)
cli/internal/config/paths_test.go (1)
cli/internal/config/paths.go (2)
DataDir(17-23)EnsureDir(44-46)
🪛 Checkov (3.2.508)
cli/testdata/compose_custom_ports.yml
[low] 17-18: Base64 High Entropy String
(CKV_SECRET_6)
🪛 GitHub Actions: Workflow Security
.github/workflows/cli.yml
[error] 55-55: cache-poisoning: runtime artifacts potentially vulnerable to a cache poisoning attack.
[error] 86-86: cache-poisoning: runtime artifacts potentially vulnerable to a cache poisoning attack.
[error] 118-118: cache-poisoning: runtime artifacts potentially vulnerable to a cache poisoning attack.
[error] 143-143: cache-poisoning: runtime artifacts potentially vulnerable to a cache poisoning attack.
🪛 LanguageTool
CLAUDE.md
[style] ~235-~235: Since ownership is already implied, this phrasing may be redundant.
Context: ... when their domain is affected. CLI has its own workflow (cli.yml). - Jobs: lint ...
(PRP_OWN)
.claude/skills/pre-pr-review/SKILL.md
[uncategorized] ~138-~138: The official name of this software platform is spelled with a capital “H”.
Context: .../.cssfile changed; anydocker/or.github/workflows/` file changed; config change...
(GITHUB)
🪛 markdownlint-cli2 (0.21.0)
.claude/skills/pre-pr-review/SKILL.md
[warning] 205-205: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/1/1
(MD029, ol-prefix)
[warning] 211-211: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/1/1
(MD029, ol-prefix)
[warning] 217-217: Ordered list item prefix
Expected: 1; Actual: 12; Style: 1/1/1
(MD029, ol-prefix)
🪛 PSScriptAnalyzer (1.24.0)
cli/scripts/install.ps1
[warning] 17-17: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 30-30: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 46-46: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 52-52: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 68-68: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 71-71: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 79-79: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 83-83: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 84-84: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] Missing BOM encoding for non-ASCII encoded file 'install.ps1'
(PSUseBOMForUnicodeEncodedFile)
🔇 Additional comments (59)
.github/dependabot.yml (1)
101-119: Looks good — Dependabot now covers the new Go CLI module.This entry is correctly scoped to
/cliand follows the existing scheduling/grouping/reviewer conventions in this file..dockerignore (1)
60-62: Good context-pruning change.Ignoring
cli/here is a solid optimization for shared root-context Docker builds and keeps unrelated CLI sources out of backend/web image contexts.Based on learnings: All Docker image builds use a single root
.dockerignore(both backend and web images build withcontext: .)..github/workflows/dependency-review.yml (2)
36-37: Clear compliance rationale added for the new Go license reference.The added context is concise and audit-friendly, and it explains why this
LicenseRefis intentionally permitted.
43-43: Allowlist entry is correctly aligned with the new license comment.Adding
LicenseRef-scancode-google-patent-license-golangtoallow-licensesis consistent with the documented exception and should reduce false positives..pre-commit-config.yaml (2)
5-5: LGTM on the CI skip list update.Correctly adding the new Go hooks to the CI skip list aligns with the existing pattern—dedicated CI jobs already cover these checks.
54-60: Hook implementations look correct.The new Go hooks properly:
- Use
bash -c 'cd cli && ...'to run in the correct directory- Set
pass_filenames: falsesince these tools operate on packages, not individual files- Scope to
^cli/.*\.go$to trigger only on CLI Go files- Run at
pre-pushstage, consistent with the existing Python pre-push hooksAlso applies to: 77-91
.gitignore (1)
73-77: Go CLI artifact ignore entries are correct.This prevents accidental commits of local binaries, dist outputs, and coverage artifacts from
cli/.docker/backend/Dockerfile (1)
109-109: Healthcheck status alignment looks good.Line 109 now checks for
status == "ok", matching the expected health payload shape while keeping a distroless-safe exec healthcheck.cli/internal/config/paths_test.go (2)
19-66: OS-specific data directory tests are well covered.The Darwin/Windows/Linux/FreeBSD cases validate both env-driven and fallback path branches clearly.
68-93:EnsureDirbehavior coverage is solid.These tests correctly verify both directory creation and idempotence on existing paths.
cli/.goreleaser.yml (1)
5-42: Release config is coherent and production-ready.Multi-platform builds, ldflags metadata injection, per-OS archive format override, and SHA-256 checksum output are all correctly wired.
cli/testdata/compose_custom_ports.yml (1)
4-65: Custom-port golden fixture looks consistent.The fixture correctly captures custom mappings (
9000:8000,4000:8080), hardening directives, and healthcheck semantics expected from the generator.cli/internal/version/version.go (1)
4-9: Build metadata injection setup is correct.The exported
vardefaults are appropriate for ldflags replacement during release builds.cli/internal/diagnostics/collect_test.go (1)
11-71: Formatting and utility test coverage is strong.The truncate, text-format section checks, error rendering assertions, and disk info contract checks provide good baseline confidence.
Also applies to: 92-98
cli/.golangci.yml (1)
4-4: No action required. golangci-lint v2.11.3 (pinned in the workflow) explicitly supports Go 1.26 (support added in v2.9.0). The configuration is correct and compatible.> Likely an incorrect or invalid review comment.cli/go.mod (2)
3-3: Verify Go version 1.26 exists.Go 1.26 appears to be a future version that may not exist yet. As of early 2025, the latest stable Go release was in the 1.23/1.24 range. This could cause build failures if the Go toolchain doesn't recognize this version.
[raise_major_issue, request_verification]
What is the latest stable version of Go programming language?
5-8: Both direct dependencies are available on the Go proxy.cli/cmd/version.go (2)
10-18: LGTM!The version command implementation follows Cobra best practices: uses
cmd.OutOrStdout()for testability, has clear formatting with aligned labels, and properly consumes build metadata from the internal/version package.
20-22: LGTM!Standard Cobra command registration pattern.
cli/internal/docker/client_test.go (4)
55-70: LGTM!Good test coverage for
RunCmdwith success, failure, and stderr scenarios. Usinggo versionas a reliable cross-platform command is practical.
100-110: Clever mock approach for ComposeExec testing.Using
["go", "version"]as a fakeComposeCmdto test the command parsing logic without requiring Docker is a pragmatic approach.
129-154: LGTM!Good coverage of version comparison edge cases including exact minimum versions and both components being too old.
156-177: LGTM!Good test coverage for semantic version comparison including the
vprefix stripping case..github/workflows/cli.yml (2)
198-212: Release wait loop is a reasonable safeguard.The 5-minute wait with 30 retries for the GitHub Release to exist handles race conditions between release creation and asset upload gracefully. The fallback message correctly notes GoReleaser will create if needed.
223-279: LGTM!The release notes augmentation script is well-constructed:
- Uses temp file and
--notes-fileto avoid shell injection- Extracts checksums safely with awk
- Properly cleans up temp file
- Appends rather than replaces existing notes
.claude/skills/aurelio-review-pr/SKILL.md (2)
148-149: LGTM!Good addition of CLI-specific file categories for the PR review skill, enabling targeted Go code review.
174-177: LGTM!Appropriate addition of Go-specific review agents. The security reviewer triggering on
exec.Command,os/exec,http, file operations, and user-supplied paths aligns well with the CLI's security-sensitive operations (self-update, file handling)..github/ISSUE_TEMPLATE/installer-failure.yml (1)
17-23: Verify if Homebrew/Scoop options are still valid.The PR summary mentions "removal of Homebrew/Scoop distribution in favor of install scripts," but this template still lists them as installation methods. If these distribution channels are removed, consider updating the dropdown to avoid user confusion.
cli/internal/config/paths.go (2)
25-41: LGTM!The
dataDirForOSfunction correctly handles platform-specific paths with appropriate fallbacks. Extracting it as a separate function enables thorough unit testing.
43-46: LGTM!Using
0o700permissions for the data directory is appropriate—it ensures only the owner can read/write/execute, which is correct for user configuration data.cli/internal/health/check_test.go (5)
12-23: LGTM!Clean test setup with httptest.Server and proper cleanup via defer.
53-70: LGTM!Good test for retry behavior—verifies that
WaitForHealthyretries on initial failures and eventually succeeds when the service becomes healthy.
72-89: LGTM!Good validation that the initial delay parameter is respected before polling begins.
91-106: LGTM!Important edge case: verifies that context cancellation during the initial delay period is properly handled.
157-172: LGTM!Good test ensuring the timeout error message includes context about the last polling error, which aids debugging.
cli/internal/selfupdate/updater_test.go (1)
22-732: Comprehensive updater test coverage—nice work.This test suite meaningfully covers cross-platform extraction, checksum enforcement, failure modes, and end-to-end update flow behavior.
cli/internal/selfupdate/updater.go (2)
182-252: LGTM! Atomic binary replacement with proper rollback.The
ReplaceAtfunction correctly handles:
- Symlink resolution before replacement
- Temp file creation in the same directory for atomic rename
- Proper permission setting (0o755)
- Windows-specific handling with rollback on failure
- Cleanup of temp files on error paths
278-294: LGTM! Checksum verification is correctly implemented.The function properly parses the checksums file format, matches by asset name, and compares hex-encoded SHA-256 hashes. The error messages are descriptive for debugging.
cli/internal/docker/client.go (2)
143-173: LGTM! Version comparison logic is sound.The
versionAtLeastfunction correctly:
- Strips
vprefix from both versions- Handles versions with fewer than 3 components
- Strips non-numeric suffixes (e.g.,
-rc1,-beta)- Performs proper numeric comparison
151-172: Thefor i := range 3syntax is supported. The project requires Go 1.26 incli/go.mod, which is well above Go 1.22 where this range-over-integer syntax was introduced.cli/cmd/uninstall.go (1)
23-45: LGTM! Proper interactive guard and graceful Docker handling.The command correctly:
- Enforces interactive terminal for destructive operations
- Loads configuration from the resolved data directory
- Continues gracefully when Docker is unavailable (warning only)
cli/internal/diagnostics/collect.go (1)
85-92: LGTM! Proper secret redaction in diagnostic output.The JWTSecret is correctly redacted before marshaling to JSON, preventing accidental exposure in bug reports.
cli/main.go (1)
1-14: LGTM! Clean minimal entry point.The entry point correctly delegates to
cmd.Execute()and exits with status 1 on error. Based on the relevant snippet fromcli/cmd/root.go, errors are already printed to stderr before being returned, so no double-printing occurs.docs/getting_started.md (1)
105-105: LGTM! Documentation accurately reflects the implemented CLI.The update from "CLI interface (future)" to "Go CLI binary (container lifecycle management)" correctly documents the newly implemented Go CLI.
docs/design/operations.md (1)
945-956: LGTM! Comprehensive CLI documentation.The updated section accurately describes:
- Implementation status (now implemented vs. future)
- Technology stack (Go, Cobra, charmbracelet/huh)
- Available commands
- Distribution method (GoReleaser + install scripts)
docs/architecture/tech-stack.md (1)
65-65: LGTM! CLI entry properly added to tech stack table.The CLI technology entry follows the established table format and provides clear rationale including the command set and distribution method.
cli/internal/compose/compose.yml.tmpl (2)
35-40: Healthcheck implementation looks correct.The inline Python healthcheck parses the JSON response and validates the status field. This approach avoids adding curl/wget dependencies to the container while still performing a meaningful health verification.
1-88: CIS hardening and template structure look solid.The template correctly applies CIS Docker Benchmark v1.6.0 hardening controls (no-new-privileges, cap_drop ALL, read_only, tmpfs mounts, non-root users). The
yamlStrfunction usage forLogLevelandJWTSecretprevents YAML injection. Resource limits are appropriately set for each service type.cli/cmd/stop.go (1)
24-49: Implementation follows established patterns and handles errors appropriately.The stop command correctly validates prerequisites (compose.yml existence, Docker availability) before attempting to stop containers. Error messages include actionable guidance for users.
cli/cmd/doctor.go (2)
61-67: URL construction is correct, but consider potential length limits.The issue URL is properly encoded. However, if
issueBodybecomes lengthy (e.g., very long health status messages), some browsers may truncate URLs exceeding ~2000 characters. The current bounded fields should be safe, but this is worth monitoring if additional diagnostics are added later.
40-47: Diagnostic file handling is secure.File permissions of 0o600 appropriately restrict access to potentially sensitive diagnostic data. The warning-only approach for file write failures allows the command to continue and still display diagnostics to stdout.
cli/cmd/root.go (1)
44-51: TTY detection implementation is correct.The
isInteractivefunction properly checks for terminal mode usingos.ModeCharDevice, which is the standard approach for detecting interactive sessions.README.md (2)
79-98: CLI installation and usage documentation is clear and complete.The Quick Start sections provide straightforward instructions for both installation methods (Linux/macOS and Windows) and cover the essential commands. The progression from
init→start→status→doctormatches the typical user workflow.
13-13: No action needed—the Python version badge is correct.Python 3.14 has been released and 3.14.3 is the current stable version, making the "3.14+" badge accurate.
docs/user_guide.md (2)
9-26: CLI-first documentation structure is well organized.The guide appropriately positions the CLI as the recommended approach while preserving the manual Docker Compose workflow for development or advanced users. Command examples are clear and actionable.
23-23: No action required — documentation is accurate.The default WebPort value in the codebase is correctly set to 3000 (see
cli/internal/config/state.go), which matches the documentation. The web dashboard URL in the user guide is correct.cli/cmd/logs.go (1)
66-79: Good defense-in-depth for command injection prevention.The combination of service name regex validation and the
--separator provides robust protection against command injection. The separator ensures that even if a malicious service name bypassed validation, it wouldn't be interpreted as a flag.cli/cmd/update.go (2)
33-67: CLI self-update flow is well-structured.The update flow appropriately warns on dev builds, handles check failures gracefully, and provides clear progress feedback. The actual download and replacement delegate to
selfupdatepackage which handles checksum verification.
122-129: Health check parameters are reasonable for container startup.The 90-second timeout with 5-second initial delay and 2-second polling interval provides adequate time for container initialization while giving timely feedback on failures.
| - `cli_go`: `.go` files in `cli/` | ||
| - `cli_config`: non-Go files in `cli/` (`.yml`, `.yaml`, `.tmpl`, `.sh`, `.ps1`) | ||
| - `site`: files in `site/` |
There was a problem hiding this comment.
cli/go.mod and cli/go.sum are not categorized, so Go checks can be skipped.
The new cli_go / cli_config rules miss module files. Dependency updates under cli/ can therefore bypass Phase 2 Go vet/test/build and Go-agent selection.
💡 Proposed fix
- - `cli_config`: non-Go files in `cli/` (`.yml`, `.yaml`, `.tmpl`, `.sh`, `.ps1`)
+ - `cli_config`: non-Go files in `cli/` (`.yml`, `.yaml`, `.tmpl`, `.sh`, `.ps1`, `go.mod`, `go.sum`)📝 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.
| - `cli_go`: `.go` files in `cli/` | |
| - `cli_config`: non-Go files in `cli/` (`.yml`, `.yaml`, `.tmpl`, `.sh`, `.ps1`) | |
| - `site`: files in `site/` | |
| - `cli_go`: `.go` files in `cli/` | |
| - `cli_config`: non-Go files in `cli/` (`.yml`, `.yaml`, `.tmpl`, `.sh`, `.ps1`, `go.mod`, `go.sum`) | |
| - `site`: files in `site/` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pre-pr-review/SKILL.md around lines 90 - 92, The change
missed categorizing module files under the cli rules, so add patterns for
cli/go.mod and cli/go.sum to the existing rule set (either extend the cli_go
rule to include "cli/go.mod" and "cli/go.sum" or create/merge a specific
cli_module rule) so Go checks and Go-agent selection run for dependency-only
updates; update the rule names referenced (cli_go, cli_config) and any file-glob
logic that determines Phase 2 vet/test/build selection to include these module
filenames.
| 10. **Vet:** | ||
|
|
||
| ```bash | ||
| cd cli && go vet ./... | ||
| ``` | ||
|
|
||
| 11. **Test:** | ||
|
|
||
| ```bash | ||
| cd cli && go test ./... | ||
| ``` | ||
|
|
||
| 12. **Build check:** | ||
|
|
||
| ```bash | ||
| cd cli && go build ./... | ||
| ``` |
There was a problem hiding this comment.
Fix ordered-list numbering to satisfy markdownlint MD029.
Lines 205/211/217 use 10., 11., 12. in a list style that expects 1. prefixes.
💡 Proposed fix
-10. **Vet:**
+1. **Vet:**
@@
-11. **Test:**
+1. **Test:**
@@
-12. **Build check:**
+1. **Build check:**🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 205-205: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/1/1
(MD029, ol-prefix)
[warning] 211-211: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/1/1
(MD029, ol-prefix)
[warning] 217-217: Ordered list item prefix
Expected: 1; Actual: 12; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pre-pr-review/SKILL.md around lines 205 - 221, The ordered
list items for "Vet:", "Test:", and "Build check:" use explicit numeric prefixes
(10., 11., 12.) which triggers markdownlint MD029; change those numeric prefixes
to "1." for each list entry so the markdown uses the allowed repeating "1."
ordered-list style (locate the three list entries titled "Vet:", "Test:", and
"Build check:" in SKILL.md and update their prefixes accordingly).
| python: | ||
| - 'src/**' | ||
| - 'tests/**' | ||
| - 'pyproject.toml' | ||
| - 'uv.lock' | ||
| - '.github/actions/**' | ||
| dashboard: | ||
| - 'web/**' | ||
| docker: | ||
| - 'docker/**' | ||
| - '.dockerignore' | ||
|
|
There was a problem hiding this comment.
Workflow-only changes currently bypass this CI matrix.
Because .github/workflows/** is missing from filters, a PR that only changes workflow logic can skip all gated jobs and still pass ci-pass.
💡 Proposed fix
python:
- 'src/**'
- 'tests/**'
- 'pyproject.toml'
- 'uv.lock'
- '.github/actions/**'
+ - '.github/workflows/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 35 - 46, The CI matrix currently lists
path filters for jobs like python, dashboard, and docker but omits workflow
files, allowing changes under .github/workflows/** to bypass CI; update the
workflow file so the path filters include '.github/workflows/**' (add it
alongside the existing entries under the same filter grouping that contains
'src/**'/'web/**'/'docker/**') to ensure PRs that only modify workflow logic
trigger the matrix jobs.
| func WaitForHealthy(ctx context.Context, url string, timeout, interval, initialDelay time.Duration) error { | ||
| ctx, cancel := context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
|
|
||
| // Wait for initial delay (container startup). | ||
| select { | ||
| case <-time.After(initialDelay): | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
|
|
||
| ticker := time.NewTicker(interval) | ||
| defer ticker.Stop() | ||
|
|
||
| var lastErr error | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| if lastErr != nil { | ||
| return fmt.Errorf("health check timed out (last error: %w)", lastErr) | ||
| } | ||
| return fmt.Errorf("health check timed out") | ||
| case <-ticker.C: | ||
| if err := checkOnce(ctx, url); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type go '\bWaitForHealthy\s*\(' -C2Repository: Aureliolo/synthorg
Length of output: 4089
🏁 Script executed:
cat -n cli/internal/health/check.goRepository: Aureliolo/synthorg
Length of output: 2641
🌐 Web query:
Go time.NewTicker behavior non-positive duration panic
💡 Result:
In Go, time.NewTicker(d) panics if d <= 0 (zero or negative). The runtime panic message is "non-positive interval for NewTicker". [1], [2]
So you must validate the duration before calling it, e.g. only create the ticker when d > 0 (or choose a default / return an error instead). [1]
Sources:
[1] time package docs (NewTicker: “d must be greater than zero; if not, NewTicker will panic.”) (pkg.go.dev)
[2] Go standard library source (src/time/tick.go: panic string) (go.dev)
Citations:
- 1: https://pkg.go.dev/time%40go1.25.5?utm_source=openai
- 2: https://go.dev/src/time/tick.go?s=1752%3A1785&utm_source=openai
Guard invalid intervals and run the first probe immediately.
Line 33 will panic when interval <= 0 (per Go's time.NewTicker specification which requires positive duration). Additionally, the first health check is delayed until the first ticker tick (line 44), so if timeout <= initialDelay + interval, a timeout can occur without any probe attempt.
💡 Proposed fix
func WaitForHealthy(ctx context.Context, url string, timeout, interval, initialDelay time.Duration) error {
+ if interval <= 0 {
+ return fmt.Errorf("health check interval must be > 0, got %s", interval)
+ }
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
@@ -35,6 +38,14 @@ func WaitForHealthy(ctx context.Context, url string, timeout, interval, initia
ticker := time.NewTicker(interval)
defer ticker.Stop()
var lastErr error
+ if err := checkOnce(ctx, url); err == nil {
+ return nil
+ } else {
+ lastErr = err
+ }
+
for {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/health/check.go` around lines 22 - 45, The WaitForHealthy
function can panic if interval <= 0 and may never run a probe before timing out;
validate interval and run the first probe immediately: inside WaitForHealthy,
check that interval > 0 (return an error if not) before calling time.NewTicker,
perform an initial probe by calling checkOnce(ctx, url) immediately after the
initialDelay select (and handle/return success if nil, or set lastErr if
non-nil), then create the ticker and continue the loop; reference
WaitForHealthy, checkOnce, interval, initialDelay and time.NewTicker when making
these changes.
| func isUpdateAvailable(current, latest string) bool { | ||
| cur := strings.TrimPrefix(current, "v") | ||
| lat := strings.TrimPrefix(latest, "v") | ||
| return cur == "dev" || cur != lat | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Version comparison logic may cause unintended downgrades or false positives.
The isUpdateAvailable function returns true whenever cur != lat, which could trigger updates even when the current version is newer than the latest release (e.g., pre-release builds, or if a release is rolled back). Consider implementing proper semantic version comparison.
♻️ Proposed semantic version comparison
func isUpdateAvailable(current, latest string) bool {
cur := strings.TrimPrefix(current, "v")
lat := strings.TrimPrefix(latest, "v")
- return cur == "dev" || cur != lat
+ if cur == "dev" {
+ return true
+ }
+ // Compare semver: return true only if latest > current
+ return compareSemver(cur, lat) < 0
+}
+
+// compareSemver returns -1 if a < b, 0 if a == b, 1 if a > b.
+func compareSemver(a, b string) int {
+ aParts := strings.SplitN(a, ".", 3)
+ bParts := strings.SplitN(b, ".", 3)
+ for i := 0; i < 3; i++ {
+ var aNum, bNum int
+ if i < len(aParts) {
+ aNum, _ = strconv.Atoi(strings.Split(aParts[i], "-")[0])
+ }
+ if i < len(bParts) {
+ bNum, _ = strconv.Atoi(strings.Split(bParts[i], "-")[0])
+ }
+ if aNum < bNum {
+ return -1
+ }
+ if aNum > bNum {
+ return 1
+ }
+ }
+ return 0
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/selfupdate/updater.go` around lines 122 - 126, The current
isUpdateAvailable function wrongly treats any cur != lat as an update; change
isUpdateAvailable to parse both versions with a semantic version parser (handle
and strip leading "v"), treat the special "dev" token as always up-to-date
(return false), compare parsed semver objects (major/minor/patch and pre-release
rules) and only return true if latest > current; if parsing fails for either
version, fall back to a safe comparison (e.g., use semver-like numeric
comparison or string equality) but ensure you do not return true when current is
newer than latest; update references to isUpdateAvailable accordingly.
cli/internal/selfupdate/updater.go
Outdated
| func httpGetWithClient(ctx context.Context, client *http.Client, url string, maxBytes int64) ([]byte, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("http %d from %s", resp.StatusCode, url) | ||
| } | ||
| return io.ReadAll(io.LimitReader(resp.Body, maxBytes)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating URL prefix in httpGetWithClient for defense in depth.
While findAssets validates URLs before calling Download, httpGetWithClient is a general-purpose function that could be called from other contexts. Adding URL validation here would provide defense in depth against SSRF if the function is reused.
🛡️ Optional: Add URL prefix validation
-func httpGetWithClient(ctx context.Context, client *http.Client, url string, maxBytes int64) ([]byte, error) {
+func httpGetWithClient(ctx context.Context, client *http.Client, url string, maxBytes int64) ([]byte, error) {
+ if !strings.HasPrefix(url, expectedURLPrefix) && !strings.HasPrefix(url, "https://api.github.com/") {
+ return nil, fmt.Errorf("URL %q does not match allowed prefixes", url)
+ }
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/selfupdate/updater.go` around lines 262 - 276, The
httpGetWithClient function lacks URL scheme validation which can enable SSRF
risks if reused; update httpGetWithClient to parse the provided url (using
net/url.Parse or equivalent), verify the scheme is either "http" or "https" and
that the host is non-empty, and return a clear error when validation fails
before creating the request; keep the existing behavior (using client.Do, status
check, and limited read) otherwise so callers like Download remain compatible.
…nline review Security/correctness: - DockerSock required when sandbox enabled (empty path → invalid volume mapping) - Redirect domain validation in httpGetWithClient (AllowedDownloadHosts) - tmpFile.Sync() + checked Close() in self-update binary replacement - Safety checks on os.RemoveAll in uninstall (refuse root/home/empty) - Resolve symlinks before removing binary in uninstall - Compose down failure aborts restart in update (no duplicate containers) - Health check failure in start returns non-zero exit - Same-port validation (backend != web) in compose generation UX/robustness: - init and uninstall require interactive TTY (fail fast in CI/cron) - Confirmation prompt before CLI binary replacement in update - Re-init warns about JWT rotation and prompts to confirm overwrite - CLI build version used as default image tag (not "latest") - Non-2xx HTTP status treated as error in status display - --tail 0 rejected (must be positive integer) - Symlinks resolved for default data dir path too - diskInfo checks partition of data dir (not always root/C:) - doctor.go uses version.RepoURL constant (no hardcoded URL) Install scripts: - install.ps1: Trim() + ToLower() on expected checksum hash - install.sh: mkdir -p INSTALL_DIR before mv CI/config: - cache: false on ALL setup-go steps in cli.yml (zizmor) - govulncheck pinned to v1.1.4 (not @latest) - Windows arm64 added to installer-failure issue template - Pre-commit: removed double blank line, grouped Go hooks together Tests: - Golden file comparison fails when file missing (no silent create) - state_test.go uses runtime.GOOS (not os.Getenv) - state.go normalizes dataDir on missing file, unmarshals onto defaults - TestMain allows localhost in AllowedDownloadHosts for httptest - diskInfo test passes data dir argument
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (4)
cli/internal/diagnostics/collect_test.go (1)
73-79:⚠️ Potential issue | 🟡 MinorBound this test with a timeout context.
Collectprobes Docker and health endpoints; withcontext.Background()this test can hang indefinitely in CI.Suggested fix
import ( "context" "strings" "testing" + "time" "github.com/Aureliolo/synthorg/cli/internal/config" ) @@ - report := Collect(context.Background(), state) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + report := Collect(ctx, state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/diagnostics/collect_test.go` around lines 73 - 79, TestCollectDoesNotPanic uses context.Background() so Collect can hang; change the test to create a timeout context (e.g., via context.WithTimeout) and pass that context into Collect, ensuring the context is cancelled/cleaned up (call cancel) and that the timeout is long enough for expected probes; update the TestCollectDoesNotPanic test to replace context.Background() with the timed context and pass the cancel to defer to avoid indefinite hangs when calling Collect(state).cli/cmd/uninstall.go (1)
108-117:⚠️ Potential issue | 🔴 CriticalValidate the uninstall target as a SynthOrg-owned directory before
RemoveAll.
config.Loadonly requiresDataDirto be absolute. A corrupt or hand-edited state file can still point this at/Users,/etc, or another broad directory, and this branch only blocks exact root/home values before callingos.RemoveAll. Resolve the path, require an app-specific marker inside it, and reject known system directories before deleting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/uninstall.go` around lines 108 - 117, The current uninstall branch uses state.DataDir directly and only blocks a few exact paths before calling os.RemoveAll; instead resolve and canonicalize the target (use filepath.Abs + filepath.EvalSymlinks) and reject deletion unless it contains an app-specific marker (e.g. a known marker file or directory created by this app), and also explicitly refuse well-known system directories ("/", "/etc", "/usr", the user home, root drive letters like "C:\", etc.); update the logic around removeData/state.DataDir and the os.RemoveAll call to first validate the canonical path contains the marker and is not one of the forbidden system paths, and return an error if validation fails so only verified SynthOrg-owned directories are removed.cli/internal/selfupdate/updater.go (1)
123-126:⚠️ Potential issue | 🟠 MajorCompare versions semantically before setting
UpdateAvail.
cur != lattreats any mismatch as an upgrade, sov1.10.0will happily "update" tov1.9.0. Becauseupdateinstalls whenever this flag is true, this opens an unintended downgrade path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/selfupdate/updater.go` around lines 123 - 126, isUpdateAvailable currently treats any string mismatch as an update which allows downgrades; change it to perform a semantic version comparison (use golang.org/x/mod/semver) so only newer versions set UpdateAvail. In isUpdateAvailable, normalize both inputs to include a leading "v" (or use semver.Canonical), then use semver.Compare(curWithV, latWithV) < 0 to return true (or keep cur == "dev" special-case). If either version is not valid semver, fall back to the existing conservative behavior (e.g., cur == "dev" || cur != lat) so you don't break non-semver tags; reference the function isUpdateAvailable when making the change.cli/cmd/status.go (1)
124-133: 🧹 Nitpick | 🔵 TrivialNon-2xx responses are now distinguished, but "unhealthy" label would improve clarity.
The code correctly differentiates 2xx from non-2xx responses by appending the HTTP status code. However, the past review suggested explicitly labeling non-2xx as "unhealthy" for clearer UX during outages.
♻️ Optional: Add explicit "unhealthy" label
var hr map[string]any if json.Unmarshal(body, &hr) == nil { if resp.StatusCode >= 200 && resp.StatusCode < 300 { fmt.Fprintf(out, " Backend: %s\n", prettyJSON(hr)) } else { - fmt.Fprintf(out, " Backend: %s (HTTP %d)\n", prettyJSON(hr), resp.StatusCode) + fmt.Fprintf(out, " Backend: unhealthy (HTTP %d): %s\n", resp.StatusCode, prettyJSON(hr)) } } else { - fmt.Fprintf(out, " Backend: %s (HTTP %d)\n", string(body), resp.StatusCode) + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + fmt.Fprintf(out, " Backend: %s\n", string(body)) + } else { + fmt.Fprintf(out, " Backend: unhealthy (HTTP %d): %s\n", resp.StatusCode, string(body)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/status.go` around lines 124 - 133, The health output currently prints the backend response and HTTP code but doesn't mark non-2xx responses as "unhealthy"; update the resp.StatusCode handling in the json.Unmarshal branch (the block using hr, prettyJSON and resp.StatusCode) and the fallback branch that prints string(body) so that any non-2xx response is labeled "unhealthy" (e.g., "Backend: unhealthy — <payload> (HTTP <code>)") while preserving the existing 2xx behavior; adjust both the json.Unmarshal true/false paths to include this explicit "unhealthy" label for clarity.
🤖 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/cli.yml:
- Around line 6-13: The workflow's push and pull_request path filters only list
"cli/**" so changes to the workflow file itself won't trigger CI; update both
"paths:" arrays (the ones under the push and under pull_request) that currently
contain "cli/**" to also include the workflow file as an additional string entry
so edits to the workflow will trigger the workflow automatically.
- Around line 249-257: Update the installer invocations to pin the fetched
scripts to the workflow TAG and export SYNTHORG_VERSION so installers install
the matching release: replace the raw.githubusercontent.com/.../main/... URLs
for install.sh and install.ps1 with the tagged path that uses the TAG variable
(e.g. raw.githubusercontent.com/.../${{ env.TAG }}/cli/scripts/install.sh and
.../${{ env.TAG }}/cli/scripts/install.ps1), and set SYNTHORG_VERSION to ${{
env.TAG }} before invoking the script (for bash export SYNTHORG_VERSION=${{
env.TAG }} && curl ... | bash; for PowerShell set $env:SYNTHORG_VERSION='${{
env.TAG }}'; irm ... | iex). Ensure you update both occurrences: install.sh and
install.ps1 and reference TAG and SYNTHORG_VERSION accordingly.
In @.pre-commit-config.yaml:
- Around line 84-90: Update the go-test pre-push hook (id: go-test) to run tests
with the race detector by modifying the hook's entry command (entry) so that it
invokes `go test -race` in the cli package (e.g., change the entry from "cd cli
&& go test ./..." to include the -race flag); keep pass_filenames and stages
behavior the same and ensure the command remains wrapped in the existing bash -c
invocation.
- Line 70: The pre-commit hook entries using "entry: bash -c 'cd cli &&
golangci-lint run'" (and the similar go vet/go test entries) require a Bash
environment on Windows; either document this requirement in
docs/getting_started.md (add a note that Git Bash/WSL is required for pre-commit
hooks) or make the hooks cross-platform by introducing platform-specific wrapper
executables and updating .pre-commit-config.yaml to call them (e.g., add
scripts/run_golangci.{sh,ps1,bat} or a small cross-platform Node/Python
launcher, then replace the "entry: bash -c 'cd cli && golangci-lint run'"
references with the wrapper command and do the same for the go vet and go test
hooks).
- Around line 68-82: Remove the redundant go-vet pre-push hook: delete the block
with id "go-vet" (the entry "bash -c 'cd cli && go vet ./...'", name "go vet
(CLI)") since "golangci-lint" already runs govet; keep the existing
golangci-lint hook (id "golangci-lint") and ensure stages/files/pass_filenames
settings remain unchanged.
In `@cli/cmd/doctor.go`:
- Around line 52-69: Detect and handle overly long GitHub issue URLs by
measuring the length of encodedBody (after url.QueryEscape on issueBody) and, if
it exceeds a safe threshold (e.g., 4000 characters), replace issueBody with a
much shorter fallback template (e.g., minimal OS/CLI/Docker/Health lines and an
explicit note to attach the full diagnostics file) before computing encodedBody
and building issueURL; update the logic around issueBody, encodedBody and
issueURL in doctor.go (referencing issueBody, encodedBody, issueTitle, and
issueURL) to perform this length check and use the truncated/fallback body so
the generated URL stays within browser limits.
In `@cli/cmd/init.go`:
- Around line 46-63: The inner isInteractive() guard inside the existing-config
branch is redundant because isInteractive() already caused an early return when
non-interactive; remove the if isInteractive() { ... } wrapper and unindent its
contents so the interactive prompt (the huh.NewForm/huh.NewGroup/huh.NewConfirm
creation and form.Run handling, plus the proceed check) runs directly within the
existing config block; keep the same behavior for proceed == false returning nil
and propagate form.Run errors as before.
In `@cli/cmd/uninstall.go`:
- Around line 88-90: The uninstall flow currently logs a warning when
composeRun(ctx, cmd, info, state.DataDir, downArgs...) fails but continues with
data/binary removal and prints success; instead, stop the uninstall on
composeRun failure by returning or bubbling the error from the current command
handler (the Run/RunE function that calls composeRun) so subsequent removal
steps (data/binary cleanup and success message) do not run. Locate the
composeRun call in uninstall.go, replace the warning-only branch with code that
wraps or returns the error (e.g., fmt.Errorf("compose down failed: %w", err)
returned from the command) so the caller/Cobra command exits non-zero and
prevents further cleanup and the misleading success print.
In `@cli/cmd/update.go`:
- Around line 26-31: runUpdate currently calls updateCLI and discards the
discovered result.LatestVersion, causing updateContainerImages to pull using the
old State.ImageTag; modify updateCLI to return the selected version (or a struct
with LatestVersion), have runUpdate capture that value and then either
regenerate/persist the compose/state to set State.ImageTag to the new version
before calling updateContainerImages (or re-exec the newly-updated binary prior
to pulling), updating references to runUpdate, updateCLI, updateContainerImages
and State.ImageTag so the new version flows from discovery → persist/regenerate
compose → compose pull/restart.
In `@cli/internal/compose/compose.yml.tmpl`:
- Around line 72-73: The volume mount using the short form "-
\"{{.DockerSock}}:/var/run/docker.sock:ro\"" can break YAML parsing on Windows
and with special characters; change the volumes entry in compose.yml.tmpl to the
long/bind syntax: replace the short string with a mapping that sets type:
"bind", source: "{{.DockerSock}}", target: "/var/run/docker.sock", and
read_only: true so the generator emits YAML-safe, cross-platform mounts when
rendering the .DockerSock variable.
In `@cli/internal/compose/generate_test.go`:
- Around line 75-94: TestGenerateWithSandbox only covers a simple Docker socket
and misses characters that require escaping/quoting; update the test
(TestGenerateWithSandbox) to set Params.DockerSock to a path containing
characters that force escaping (e.g., include a dollar sign, backslash or
Windows drive colon) before calling Generate(p), and add an assertContains on
the output YAML (using assertContains) that verifies the rendered mount string
is properly escaped/quoted; this ensures the Generate function’s Compose
hardening output is validated for edge-case socket paths.
In `@cli/internal/selfupdate/updater_test.go`:
- Around line 722-738: Test table in TestIsUpdateAvailable lacks a downgrade
case so a downgrade bug isn't caught; add a test row exercising a lower latest
version (for example current "v1.10.0" and latest "v1.9.0") and expect false.
Update the tests slice in TestIsUpdateAvailable to include the downgrade case so
isUpdateAvailable("v1.10.0", "v1.9.0") returns false and the test fails if
downgrades are incorrectly treated as updates.
In `@cli/scripts/install.ps1`:
- Line 34: The assignment to $WinArch silently defaults any non-Arm64
architecture to "amd64"; change the logic in the install script to explicitly
check for Architecture::X64 and Architecture::Arm64 using
[System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture, set
$WinArch = "amd64" for Architecture::X64 and "arm64" for Architecture::Arm64,
and if the value is anything else (e.g., Architecture::Arm or unexpected enum)
write an error to the console and exit with a non‑zero code so the installer
fails fast; update the conditional around RuntimeInformation::OSArchitecture and
references to $WinArch accordingly.
In `@cli/scripts/install.sh`:
- Around line 92-99: The current install.sh branch incorrectly skips sudo when
the parent directory is writable even if $INSTALL_DIR exists and is not
writable; update the conditional around the mkdir/mv/chmod block so non-root
operations are only used when $INSTALL_DIR itself is writable OR when
$INSTALL_DIR does not exist AND its parent directory is writable (i.e., check
existence of $INSTALL_DIR plus writability of dirname "$INSTALL_DIR"); keep the
same operations on TMP_DIR/$BINARY_NAME and $INSTALL_DIR/$BINARY_NAME but use
sudo for the other case.
---
Duplicate comments:
In `@cli/cmd/status.go`:
- Around line 124-133: The health output currently prints the backend response
and HTTP code but doesn't mark non-2xx responses as "unhealthy"; update the
resp.StatusCode handling in the json.Unmarshal branch (the block using hr,
prettyJSON and resp.StatusCode) and the fallback branch that prints string(body)
so that any non-2xx response is labeled "unhealthy" (e.g., "Backend: unhealthy —
<payload> (HTTP <code>)") while preserving the existing 2xx behavior; adjust
both the json.Unmarshal true/false paths to include this explicit "unhealthy"
label for clarity.
In `@cli/cmd/uninstall.go`:
- Around line 108-117: The current uninstall branch uses state.DataDir directly
and only blocks a few exact paths before calling os.RemoveAll; instead resolve
and canonicalize the target (use filepath.Abs + filepath.EvalSymlinks) and
reject deletion unless it contains an app-specific marker (e.g. a known marker
file or directory created by this app), and also explicitly refuse well-known
system directories ("/", "/etc", "/usr", the user home, root drive letters like
"C:\", etc.); update the logic around removeData/state.DataDir and the
os.RemoveAll call to first validate the canonical path contains the marker and
is not one of the forbidden system paths, and return an error if validation
fails so only verified SynthOrg-owned directories are removed.
In `@cli/internal/diagnostics/collect_test.go`:
- Around line 73-79: TestCollectDoesNotPanic uses context.Background() so
Collect can hang; change the test to create a timeout context (e.g., via
context.WithTimeout) and pass that context into Collect, ensuring the context is
cancelled/cleaned up (call cancel) and that the timeout is long enough for
expected probes; update the TestCollectDoesNotPanic test to replace
context.Background() with the timed context and pass the cancel to defer to
avoid indefinite hangs when calling Collect(state).
In `@cli/internal/selfupdate/updater.go`:
- Around line 123-126: isUpdateAvailable currently treats any string mismatch as
an update which allows downgrades; change it to perform a semantic version
comparison (use golang.org/x/mod/semver) so only newer versions set UpdateAvail.
In isUpdateAvailable, normalize both inputs to include a leading "v" (or use
semver.Canonical), then use semver.Compare(curWithV, latWithV) < 0 to return
true (or keep cur == "dev" special-case). If either version is not valid semver,
fall back to the existing conservative behavior (e.g., cur == "dev" || cur !=
lat) so you don't break non-semver tags; reference the function
isUpdateAvailable when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee1dae7b-e7aa-4259-80f3-8a69486d7aee
📒 Files selected for processing (24)
.github/ISSUE_TEMPLATE/installer-failure.yml.github/workflows/cli.yml.pre-commit-config.yamlcli/cmd/doctor.gocli/cmd/init.gocli/cmd/logs.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/internal/compose/compose.yml.tmplcli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/state_test.gocli/internal/diagnostics/collect.gocli/internal/diagnostics/collect_test.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/updater_test.gocli/internal/version/version.gocli/scripts/install.ps1cli/scripts/install.shdocs/getting_started.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Backend
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting).
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Pre-push hooks run mypy type-check and pytest unit tests as a fast gate before push. These are skipped in pre-commit.ci (dedicated CI jobs already run them).
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2026-03-14T15:34:22.487Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:34:22.487Z
Learning: Applies to docker/Dockerfile* : Lint all Dockerfiles with hadolint—enforced via pre-commit hook locally and in CI via `dockerfile-lint` job.
Applied to files:
.pre-commit-config.yaml
🧬 Code graph analysis (10)
cli/internal/config/state_test.go (2)
cli/internal/config/state.go (5)
DefaultState(26-34)State(14-23)Save(75-84)Load(43-72)StatePath(37-39)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/internal/diagnostics/collect.go (4)
cli/internal/config/state.go (1)
State(14-23)cli/internal/version/version.go (2)
Version(9-9)Commit(10-10)cli/internal/docker/client.go (2)
Detect(34-66)ComposeExecOutput(100-107)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/init.go (4)
cli/internal/config/state.go (4)
StatePath(37-39)DefaultState(26-34)State(14-23)Save(75-84)cli/internal/config/paths.go (2)
DataDir(17-23)EnsureDir(44-46)cli/internal/version/version.go (1)
Version(9-9)cli/internal/compose/generate.go (2)
ParamsFromState(43-54)Generate(58-76)
cli/internal/compose/generate_test.go (2)
cli/internal/compose/generate.go (3)
Params(31-40)Generate(58-76)ParamsFromState(43-54)cli/internal/config/state.go (1)
State(14-23)
cli/internal/selfupdate/updater_test.go (1)
cli/internal/selfupdate/updater.go (6)
AllowedDownloadHosts(274-278)Release(43-46)Asset(49-52)Download(156-180)CheckFromURL(71-90)ReplaceAt(193-261)
cli/cmd/root.go (1)
cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/status.go (4)
cli/internal/config/state.go (2)
Load(43-72)State(14-23)cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/docker/client.go (4)
Detect(34-66)Info(22-29)ComposeExecOutput(100-107)RunCmd(110-119)cli/internal/version/version.go (2)
Version(9-9)Commit(10-10)
cli/cmd/uninstall.go (3)
cli/internal/config/state.go (2)
Load(43-72)State(14-23)cli/internal/docker/client.go (2)
Detect(34-66)Info(22-29)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/logs.go (3)
cli/internal/config/state.go (1)
Load(43-72)cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/docker/client.go (1)
Detect(34-66)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (2)
DataDir(17-23)EnsureDir(44-46)
🪛 PSScriptAnalyzer (1.24.0)
cli/scripts/install.ps1
[warning] 17-17: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 30-30: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 46-46: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 52-52: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 68-68: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 71-71: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 79-79: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 83-83: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 84-84: File 'install.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] Missing BOM encoding for non-ASCII encoded file 'install.ps1'
(PSUseBOMForUnicodeEncodedFile)
🔇 Additional comments (24)
docs/getting_started.md (1)
105-105: Clear and accurate CLI scope clarification.This wording cleanly distinguishes the Python module from the new top-level Go CLI, which aligns with the PR’s architecture changes.
.github/ISSUE_TEMPLATE/installer-failure.yml (1)
1-79: Well-structured issue template for CLI installation failures.The template is comprehensive and covers all necessary information for debugging installation issues:
- Installation method dropdown covers all distribution channels (Homebrew, Scoop, installers, manual, source build)
- OS dropdown includes all platforms from the CI build matrix, including Windows arm64
- Required fields (install-method, os, error, steps) capture essential debugging info
- Optional fields (version, context) allow for additional helpful details without blocking submission
- Shell rendering for error output improves readability
- Additional context guidance appropriately mentions Docker version, which aligns with the CLI's minimum version checks
.pre-commit-config.yaml (1)
5-5: LGTM!The skip list correctly includes the new Go hook IDs (
golangci-lint, go-vet, go-test), ensuring these are skipped in pre-commit.ci since dedicated CI jobs already run them.cli/scripts/install.ps1 (1)
53-57: The original code does not have the issue described. In PowerShell, when$lineis$null, the expression($null -split '\s+')[0].Trim().ToLower()does not throw an error. Instead,$null -split '\s+')returns a string array with a single empty string element, which propagates as an empty string through.Trim()and.ToLower(). The existing checkif (-not $ExpectedHash)on line 56 correctly catches this case and throws the intended error message. The current code handles the missing checksum scenario safely without modification.> Likely an incorrect or invalid review comment.cli/internal/version/version.go (1)
4-12: Centralizing ldflag-backed metadata here looks good.Safe defaults plus a single source of truth keep
versionanddoctorin sync.cli/internal/config/state_test.go (1)
1-182: LGTM!The test suite is comprehensive, covering defaults, round-trip persistence, directory creation, file permissions (with appropriate Windows skip using
runtime.GOOS), missing file defaults, and invalid JSON error handling. The use oft.TempDir()ensures proper test isolation.cli/cmd/root.go (2)
30-42: LGTM!The
resolveDataDirfunction now correctly resolves symlinks for both user-provided and default paths, addressing the asymmetric handling from the past review. The logic flow is clean: determine the directory (flag or default), then resolve symlinks on the final value.
44-51: LGTM!The
isInteractive()implementation correctly detects terminal interactivity by checking foros.ModeCharDevice, which is the standard approach for TTY detection in Go.cli/cmd/doctor.go (1)
27-74: LGTM overall!The doctor command flow is well-structured: load config → collect diagnostics → save to file → print report → generate GitHub issue URL. Error handling is appropriate with graceful degradation when file saving fails.
cli/cmd/logs.go (2)
58-64: LGTM!The tail validation now correctly rejects zero and negative values (
n <= 0), matching the error message requiring "a positive integer." This addresses the previous review concern.
66-79: LGTM!The service name validation and argument construction are secure:
- Regex validation prevents command injection via service names
- The
--separator correctly prevents service names from being interpreted as flagscli/cmd/status.go (1)
31-62: LGTM!The status command flow is well-organized with clear separation of concerns across helper functions. The graceful handling of uninitialized state and Docker unavailability provides a good user experience.
cli/internal/diagnostics/collect.go (3)
68-86: LGTM!The health check now properly handles the error from
http.NewRequestWithContext(lines 71-74), addressing the past review concern. The error handling chain is correct: request creation error → request execution error → successful response with body reading.
88-95: LGTM!The JWTSecret redaction is correctly implemented, ensuring sensitive credentials are not exposed in diagnostic reports.
136-165: LGTM!The
diskInfofunction correctly uses partition-aware disk checks by targeting the data directory rather than just root. The Windows/Unix branching is appropriate, anddf -hworks correctly on both Linux and macOS.cli/cmd/init.go (2)
164-169: LGTM!The image tag derivation now correctly uses the CLI's build version, falling back to "latest" only for dev builds. This addresses the past review concern about maintaining CLI→container version mapping.
222-228: LGTM!The JWT secret generation correctly uses
crypto/randfor cryptographically secure random bytes, and the 48-byte length (384 bits) provides strong entropy.cli/internal/compose/generate.go (3)
92-94: LGTM!The validation now correctly rejects identical backend and web ports, addressing the past review concern about Docker Compose binding failures.
106-122: LGTM!The
yamlStrfunction correctly handles YAML escaping:
- Escapes backslashes first (proper order to avoid double-escaping)
- Escapes quotes and control characters
- Escapes
$to$$to prevent Docker Compose variable interpolationThe always-quoted output approach is safe and consistent.
78-104: LGTM!The
validateParamsfunction provides comprehensive validation:
- Image tag pattern prevents YAML injection
- Log level restricted to known safe values
- Port range and uniqueness validation
- Sandbox mode requires Docker socket with unsafe character rejection
cli/cmd/start.go (2)
68-71: Health check failure now returns a non-zero exit code.The previous review concern about returning
nilon health check failure has been addressed. The command now correctly returns an error after printing diagnostic guidance, ensuring automation and scripts properly detect startup failures.
79-89: No defensive check needed. TheDetect()function guarantees thatComposeCmdis non-empty when it succeeds—it's set to either["docker", "compose"]or["docker-compose"]before returning. If detection fails,Detect()returns an error, socomposeRun()would never be called with an invalidInfo. The error handling is already in place at the caller level.> Likely an incorrect or invalid review comment.cli/internal/config/state.go (2)
47-72: Previous review concerns have been addressed.The
Loadfunction now correctly:
- Validates that
dataDiris an absolute path in the missing-file branch (Lines 50-53)- Unmarshals onto
DefaultState()to preserve defaults for missing fields (Line 60)Both paths now maintain consistent absolute-path invariants.
74-84: Secure file permissions and proper error handling.The
Savefunction correctly:
- Ensures the data directory exists before writing
- Uses
0o600permissions restricting access to owner-only, which is appropriate for config files that may contain sensitive data likeJWTSecret
|
|
||
| - id: golangci-lint | ||
| name: golangci-lint (CLI) | ||
| entry: bash -c 'cd cli && golangci-lint run' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Windows is mentioned in contributing/setup docs
rg -i 'windows|bash|wsl' README.md CONTRIBUTING.md docs/ 2>/dev/null | head -20Repository: Aureliolo/synthorg
Length of output: 837
🏁 Script executed:
# Check the .pre-commit-config.yaml file to see the lines mentioned
cat -n .pre-commit-config.yaml | sed -n '65,90p'Repository: Aureliolo/synthorg
Length of output: 937
🏁 Script executed:
# Check if CONTRIBUTING.md exists and what setup instructions it contains
if [ -f CONTRIBUTING.md ]; then
cat CONTRIBUTING.md
else
echo "CONTRIBUTING.md not found"
fiRepository: Aureliolo/synthorg
Length of output: 87
🏁 Script executed:
# Search for pre-commit specific instructions in documentation
rg -i 'pre-commit' README.md CONTRIBUTING.md docs/ 2>/dev/nullRepository: Aureliolo/synthorg
Length of output: 666
🏁 Script executed:
# View the Pre-commit Hooks section in docs/getting_started.md
rg -A 15 '## Pre-commit Hooks' docs/getting_started.mdRepository: Aureliolo/synthorg
Length of output: 396
🏁 Script executed:
# View the Pre-Commit Hooks section in docs/security.md
rg -A 10 '### Pre-Commit Hooks' docs/security.mdRepository: Aureliolo/synthorg
Length of output: 465
🏁 Script executed:
# Check if there are any Windows-specific instructions in docs/ for setup
rg -B 2 -A 5 '(Windows|Git Bash|WSL|bash)' docs/getting_started.md | head -40Repository: Aureliolo/synthorg
Length of output: 788
Document bash requirement for Go pre-commit hooks on Windows, or refactor to cross-platform approach.
The bash -c pattern used in the golangci-lint, go vet, and go test hooks (lines 70, 78, 86) won't work on vanilla Windows. While docs/getting_started.md includes Windows setup instructions, it doesn't mention that pre-commit hooks require bash (Git Bash, WSL, or equivalent). Either add a note to the pre-commit documentation clarifying this requirement for Windows developers, or refactor to use a cross-platform approach that works natively on Windows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml at line 70, The pre-commit hook entries using
"entry: bash -c 'cd cli && golangci-lint run'" (and the similar go vet/go test
entries) require a Bash environment on Windows; either document this requirement
in docs/getting_started.md (add a note that Git Bash/WSL is required for
pre-commit hooks) or make the hooks cross-platform by introducing
platform-specific wrapper executables and updating .pre-commit-config.yaml to
call them (e.g., add scripts/run_golangci.{sh,ps1,bat} or a small cross-platform
Node/Python launcher, then replace the "entry: bash -c 'cd cli && golangci-lint
run'" references with the wrapper command and do the same for the go vet and go
test hooks).
| volumes: | ||
| - "{{.DockerSock}}:/var/run/docker.sock:ro" |
There was a problem hiding this comment.
Switch the socket mount to long syntax.
Embedding {{.DockerSock}} in the short "host:container:ro" form reintroduces YAML/Compose parsing bugs for $, backslashes, and Windows drive-letter colons. That breaks the cross-platform/YAML-safe guarantees the generator is supposed to provide.
Suggested fix
volumes:
- - "{{.DockerSock}}:/var/run/docker.sock:ro"
+ - type: bind
+ source: {{yamlStr .DockerSock}}
+ target: /var/run/docker.sock
+ read_only: true📝 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.
| volumes: | |
| - "{{.DockerSock}}:/var/run/docker.sock:ro" | |
| volumes: | |
| - type: bind | |
| source: {{yamlStr .DockerSock}} | |
| target: /var/run/docker.sock | |
| read_only: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/compose/compose.yml.tmpl` around lines 72 - 73, The volume mount
using the short form "- \"{{.DockerSock}}:/var/run/docker.sock:ro\"" can break
YAML parsing on Windows and with special characters; change the volumes entry in
compose.yml.tmpl to the long/bind syntax: replace the short string with a
mapping that sets type: "bind", source: "{{.DockerSock}}", target:
"/var/run/docker.sock", and read_only: true so the generator emits YAML-safe,
cross-platform mounts when rendering the .DockerSock variable.
| func TestGenerateWithSandbox(t *testing.T) { | ||
| p := Params{ | ||
| CLIVersion: "dev", | ||
| ImageTag: "latest", | ||
| BackendPort: 8000, | ||
| WebPort: 3000, | ||
| LogLevel: "info", | ||
| Sandbox: true, | ||
| DockerSock: "/var/run/docker.sock", | ||
| } | ||
| out, err := Generate(p) | ||
| if err != nil { | ||
| t.Fatalf("Generate: %v", err) | ||
| } | ||
| yaml := string(out) | ||
|
|
||
| assertContains(t, yaml, "synthorg-sandbox:latest") | ||
| assertContains(t, yaml, "/var/run/docker.sock:/var/run/docker.sock:ro") | ||
| assertContains(t, yaml, "no-new-privileges:true") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exercise a socket path that needs escaping.
This sandbox test only uses /var/run/docker.sock, so it won't catch $, backslashes, or drive-letter colons in the rendered mount. Add one case here that forces escaping/quoting so the Compose hardening can't regress silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/compose/generate_test.go` around lines 75 - 94,
TestGenerateWithSandbox only covers a simple Docker socket and misses characters
that require escaping/quoting; update the test (TestGenerateWithSandbox) to set
Params.DockerSock to a path containing characters that force escaping (e.g.,
include a dollar sign, backslash or Windows drive colon) before calling
Generate(p), and add an assertContains on the output YAML (using assertContains)
that verifies the rendered mount string is properly escaped/quoted; this ensures
the Generate function’s Compose hardening output is validated for edge-case
socket paths.
cli/scripts/install.sh
Outdated
| if [ -w "$INSTALL_DIR" ] || [ -w "$(dirname "$INSTALL_DIR")" ]; then | ||
| mkdir -p "$INSTALL_DIR" | ||
| mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}" | ||
| chmod +x "${INSTALL_DIR}/${BINARY_NAME}" | ||
| else | ||
| sudo mkdir -p "$INSTALL_DIR" | ||
| sudo mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}" | ||
| sudo chmod +x "${INSTALL_DIR}/${BINARY_NAME}" |
There was a problem hiding this comment.
Only skip sudo when the target dir itself is writable.
This branch uses the non-privileged path whenever the parent directory is writable, even if $INSTALL_DIR already exists and is not. In that case mkdir -p succeeds, but mv/chmod still fail.
💡 Proposed fix
-if [ -w "$INSTALL_DIR" ] || [ -w "$(dirname "$INSTALL_DIR")" ]; then
+if [ -d "$INSTALL_DIR" ] && [ -w "$INSTALL_DIR" ]; then
+ mkdir -p "$INSTALL_DIR"
+ mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
+ chmod +x "${INSTALL_DIR}/${BINARY_NAME}"
+elif [ ! -e "$INSTALL_DIR" ] && [ -w "$(dirname "$INSTALL_DIR")" ]; then
mkdir -p "$INSTALL_DIR"
mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
chmod +x "${INSTALL_DIR}/${BINARY_NAME}"
else
sudo mkdir -p "$INSTALL_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/scripts/install.sh` around lines 92 - 99, The current install.sh branch
incorrectly skips sudo when the parent directory is writable even if
$INSTALL_DIR exists and is not writable; update the conditional around the
mkdir/mv/chmod block so non-root operations are only used when $INSTALL_DIR
itself is writable OR when $INSTALL_DIR does not exist AND its parent directory
is writable (i.e., check existence of $INSTALL_DIR plus writability of dirname
"$INSTALL_DIR"); keep the same operations on TMP_DIR/$BINARY_NAME and
$INSTALL_DIR/$BINARY_NAME but use sudo for the other case.
CI: - Add .github/workflows/cli.yml to path triggers (self-trigger on edits) - Release notes pin install URLs to $TAG + set SYNTHORG_VERSION - Remove redundant go-vet pre-push hook (golangci-lint includes govet) - Add -race flag to go-test pre-push hook CLI: - Remove redundant isInteractive() in init re-init block (already guarded) - Truncate doctor issue URL body if >3500 chars (browser limits) - Abort uninstall on compose down failure (was warning-only) - Explicit "unhealthy" label for non-2xx status responses - Non-interactive update prints notice before auto-applying - Proper semver comparison prevents downgrades (compareSemver) Install scripts: - install.ps1: explicit X64/Arm64 arch check, error on unknown - install.sh: fix sudo logic (check INSTALL_DIR vs parent writability) - install.sh: use jq/python3/grep fallback chain for JSON parsing Tests: - collect_test.go: timeout context instead of Background() - updater_test.go: multi-digit minor downgrade case (v1.10→v1.9)
| func isUpdateAvailable(current, latest string) bool { | ||
| cur := strings.TrimPrefix(current, "v") | ||
| if cur == "dev" { | ||
| return true | ||
| } | ||
| lat := strings.TrimPrefix(latest, "v") | ||
| // Only offer update when latest is strictly greater than current. | ||
| return compareSemver(lat, cur) > 0 | ||
| } |
There was a problem hiding this comment.
Pre-release builds never receive stable-release updates
compareSemver uses strings.FieldsFunc to extract the leading numeric run from each version segment. For a patch value like "3-beta" it returns ["3"], and for "3" (the stable release) it also returns ["3"] — so compareSemver("1.2.3", "1.2.3-beta") returns 0. Consequently isUpdateAvailable("1.2.3-beta", "1.2.3") evaluates to compareSemver("1.2.3", "1.2.3-beta") > 0 = false, and users on any pre-release build will silently never be offered the corresponding stable release.
A minimal fix is to treat a stable version as strictly greater than a pre-release with the same major.minor.patch when the stable release is the "latest":
| func isUpdateAvailable(current, latest string) bool { | |
| cur := strings.TrimPrefix(current, "v") | |
| if cur == "dev" { | |
| return true | |
| } | |
| lat := strings.TrimPrefix(latest, "v") | |
| // Only offer update when latest is strictly greater than current. | |
| return compareSemver(lat, cur) > 0 | |
| } | |
| func isUpdateAvailable(current, latest string) bool { | |
| cur := strings.TrimPrefix(current, "v") | |
| if cur == "dev" { | |
| return true | |
| } | |
| lat := strings.TrimPrefix(latest, "v") | |
| cmp := compareSemver(lat, cur) | |
| if cmp != 0 { | |
| return cmp > 0 | |
| } | |
| // Same numeric version: stable (no pre-release) beats a pre-release. | |
| latHasPre := strings.ContainsRune(lat, '-') | |
| curHasPre := strings.ContainsRune(cur, '-') | |
| return !latHasPre && curHasPre | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/internal/selfupdate/updater.go
Line: 124-132
Comment:
**Pre-release builds never receive stable-release updates**
`compareSemver` uses `strings.FieldsFunc` to extract the leading numeric run from each version segment. For a patch value like `"3-beta"` it returns `["3"]`, and for `"3"` (the stable release) it also returns `["3"]` — so `compareSemver("1.2.3", "1.2.3-beta")` returns `0`. Consequently `isUpdateAvailable("1.2.3-beta", "1.2.3")` evaluates to `compareSemver("1.2.3", "1.2.3-beta") > 0` = `false`, and users on any pre-release build will silently never be offered the corresponding stable release.
A minimal fix is to treat a stable version as strictly greater than a pre-release with the same major.minor.patch when the stable release is the "latest":
```suggestion
func isUpdateAvailable(current, latest string) bool {
cur := strings.TrimPrefix(current, "v")
if cur == "dev" {
return true
}
lat := strings.TrimPrefix(latest, "v")
cmp := compareSemver(lat, cur)
if cmp != 0 {
return cmp > 0
}
// Same numeric version: stable (no pre-release) beats a pre-release.
latHasPre := strings.ContainsRune(lat, '-')
curHasPre := strings.ContainsRune(cur, '-')
return !latHasPre && curHasPre
}
```
How can I resolve this? If you propose a fix, please make it concise.| func httpGetWithClient(ctx context.Context, client *http.Client, rawURL string, maxBytes int64) ([]byte, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Validate final URL after redirects stays within GitHub's domain. | ||
| if finalHost := resp.Request.URL.Hostname(); !AllowedDownloadHosts[finalHost] { | ||
| return nil, fmt.Errorf("download redirected to unexpected host %q", finalHost) | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("http %d from %s", resp.StatusCode, rawURL) | ||
| } | ||
| return io.ReadAll(io.LimitReader(resp.Body, maxBytes)) | ||
| } |
There was a problem hiding this comment.
URL scheme not validated after redirects
The redirect-host guard checks resp.Request.URL.Hostname() but not the scheme. Go's http.Client will follow HTTPS → HTTP redirects by default (the CheckRedirect policy does not enforce scheme preservation). An attacker who could inject an HTTP redirect from an allowed domain (e.g. objects.githubusercontent.com) would bypass TLS for the binary download; the checksum check still runs, but the download is exposed to passive network interception.
Consider adding a scheme check alongside the host check:
finalURL := resp.Request.URL
if finalURL.Scheme != "https" {
return nil, fmt.Errorf("download redirected to non-HTTPS URL %q", finalURL.String())
}
if !AllowedDownloadHosts[finalURL.Hostname()] {
return nil, fmt.Errorf("download redirected to unexpected host %q", finalURL.Hostname())
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/internal/selfupdate/updater.go
Line: 311-331
Comment:
**URL scheme not validated after redirects**
The redirect-host guard checks `resp.Request.URL.Hostname()` but not the scheme. Go's `http.Client` will follow HTTPS → HTTP redirects by default (the `CheckRedirect` policy does not enforce scheme preservation). An attacker who could inject an HTTP redirect from an allowed domain (e.g. `objects.githubusercontent.com`) would bypass TLS for the binary download; the checksum check still runs, but the download is exposed to passive network interception.
Consider adding a scheme check alongside the host check:
```go
finalURL := resp.Request.URL
if finalURL.Scheme != "https" {
return nil, fmt.Errorf("download redirected to non-HTTPS URL %q", finalURL.String())
}
if !AllowedDownloadHosts[finalURL.Hostname()] {
return nil, fmt.Errorf("download redirected to unexpected host %q", finalURL.Hostname())
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR introduces a new cross-platform Go-based synthorg CLI (as a separate Go module under cli/) to manage the Docker container lifecycle, self-update from GitHub Releases, and collect diagnostics. It also updates documentation and CI/release automation to support building, testing, and distributing the CLI.
Changes:
- Add Go CLI module with Cobra commands (init/start/stop/status/logs/update/doctor/uninstall/version) plus internal packages for compose generation, Docker detection, health checks, self-update, and diagnostics.
- Add distribution + automation: GoReleaser config, install scripts (bash/PowerShell), CLI GitHub Actions workflow, and Dependabot gomod updates.
- Update docs/README/CLAUDE + minor Docker healthcheck fix (
healthy→ok) to align with backend health status semantics.
Reviewed changes
Copilot reviewed 51 out of 53 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/user_guide.md | Switch quick start to CLI-first flow; keep manual compose path. |
| docs/roadmap/index.md | Remove “CLI future” roadmap item now that CLI is implemented. |
| docs/getting_started.md | Clarify Python CLI module vs top-level Go CLI. |
| docs/design/operations.md | Update operations diagram + note to reflect implemented Go CLI. |
| docs/architecture/tech-stack.md | Document Go CLI tech choice + command set. |
| docker/backend/Dockerfile | Align healthcheck expected status with ok. |
| cli/testdata/compose_default.yml | Golden compose output for default config. |
| cli/testdata/compose_custom_ports.yml | Golden compose output for custom ports/JWT/log level. |
| cli/scripts/install.sh | Linux/macOS installer with checksum verification. |
| cli/scripts/install.ps1 | Windows installer with checksum verification and PATH update. |
| cli/main.go | CLI entry point. |
| cli/internal/version/version.go | Build-time version metadata (ldflags). |
| cli/internal/selfupdate/updater.go | Self-update logic (release check/download/verify/extract/replace). |
| cli/internal/selfupdate/updater_test.go | Tests for self-update behavior and safety checks. |
| cli/internal/health/check.go | Backend health polling helper. |
| cli/internal/health/check_test.go | Health polling tests. |
| cli/internal/docker/client.go | Docker/Compose detection + command execution helpers. |
| cli/internal/docker/client_test.go | Tests for docker helpers and version checks. |
| cli/internal/diagnostics/collect.go | Diagnostic collection + text formatting. |
| cli/internal/diagnostics/collect_test.go | Tests for diagnostics formatting/truncation/disk info. |
| cli/internal/config/state.go | Persisted CLI config (JSON) load/save + validation. |
| cli/internal/config/state_test.go | Tests for config persistence and permissions. |
| cli/internal/config/paths.go | Platform data directory resolution + EnsureDir helper. |
| cli/internal/config/paths_test.go | Tests for platform data-dir resolution. |
| cli/internal/compose/generate.go | Compose generation from embedded template + validation + YAML escaping. |
| cli/internal/compose/generate_test.go | Compose generation tests (including golden comparisons). |
| cli/internal/compose/compose.yml.tmpl | Embedded compose template with hardening and optional sandbox. |
| cli/go.mod | Go module definition + dependencies. |
| cli/go.sum | Go dependency lockfile. |
| cli/cmd/root.go | Root Cobra command + data-dir flag + interactivity detection. |
| cli/cmd/init.go | Interactive setup wizard: config + compose generation. |
| cli/cmd/start.go | Compose pull/up + health wait. |
| cli/cmd/stop.go | Compose down. |
| cli/cmd/status.go | Display container state, stats, and backend health info. |
| cli/cmd/logs.go | Tail/follow logs with input validation. |
| cli/cmd/update.go | Self-update + image pull + optional restart/health check. |
| cli/cmd/doctor.go | Generate diagnostic file + prefilled GitHub issue URL. |
| cli/cmd/uninstall.go | Interactive uninstall (containers/data/binary). |
| cli/cmd/version.go | Print version/build metadata. |
| cli/.goreleaser.yml | GoReleaser build/archive/checksum configuration. |
| cli/.golangci.yml | CLI lint configuration. |
| .github/workflows/cli.yml | CLI CI workflow (lint/test/build/vulncheck + release on tags). |
| .github/workflows/ci.yml | Add domain-based path filtering to skip unrelated jobs. |
| .github/workflows/dependency-review.yml | Allow license metadata for golang.org/x/* packages. |
| .github/dependabot.yml | Add gomod updates for /cli. |
| .github/ISSUE_TEMPLATE/installer-failure.yml | New issue template for CLI installation failures. |
| .pre-commit-config.yaml | Add pre-push Go lint/test hooks (CLI-only). |
| .gitignore | Ignore CLI build artifacts/dist/coverage outputs. |
| .dockerignore | Exclude cli/ from Docker build contexts. |
| README.md | Document CLI installation + CLI-first quick start. |
| CLAUDE.md | Document Go CLI module structure and Go tooling commands. |
| .claude/skills/pre-pr-review/SKILL.md | Extend skill taxonomy and checks to include Go CLI domain. |
| .claude/skills/aurelio-review-pr/SKILL.md | Extend review skill taxonomy to include Go CLI domain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --- Detect architecture --- | ||
|
|
||
| $OsArch = [System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture | ||
| $WinArch = switch ($OsArch) { |
| } | ||
|
|
||
| # --- Extract and install --- | ||
|
|
||
| Write-Host "Extracting..." |
| func DataDir() string { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| home = "." // fallback to current directory |
| - 'tests/**' | ||
| - 'pyproject.toml' | ||
| - 'uv.lock' | ||
| - '.github/actions/**' |
| out := cmd.OutOrStdout() | ||
|
|
||
| dir := resolveDataDir() | ||
| state, err := config.Load(dir) | ||
| if err != nil { |
| paths: | ||
| - "cli/**" | ||
| - ".github/workflows/cli.yml" |
| fi | ||
|
|
||
| # --- Extract and install --- | ||
|
|
||
| echo "Extracting..." |
🤖 I have created a release *beep* *boop* --- ## [0.2.0](v0.1.4...v0.2.0) (2026-03-15) ##First probably usable release? Most likely not no and everything will break ### Features * add /get/ installation page for CLI installer ([#413](#413)) ([6a47e4a](6a47e4a)) * add cross-platform Go CLI for container lifecycle management ([#401](#401)) ([0353d9e](0353d9e)), closes [#392](#392) * add explicit ScanOutcome signal to OutputScanResult ([#394](#394)) ([be33414](be33414)), closes [#284](#284) * add meeting scheduler, event-triggered meetings, and Go CLI lint fixes ([#407](#407)) ([5550fa1](5550fa1)) * wire MultiAgentCoordinator into runtime ([#396](#396)) ([7a9e516](7a9e516)) ### Bug Fixes * CLA signatures branch + declutter repo root ([#409](#409)) ([cabe953](cabe953)) * correct Release Please branch name in release workflow ([#410](#410)) ([515d816](515d816)) * replace slsa-github-generator with attest-build-provenance, fix DAST ([#424](#424)) ([eeaadff](eeaadff)) * resolve CodeQL path-injection alerts in Go CLI ([#412](#412)) ([f41bf16](f41bf16)) ### Refactoring * rename package from ai_company to synthorg ([#422](#422)) ([df27c6e](df27c6e)), closes [#398](#398) ### Tests * add fuzz and property-based testing across all layers ([#421](#421)) ([115a742](115a742)) ### CI/CD * add SLSA L3 provenance for CLI binaries and container images ([#423](#423)) ([d3dc75d](d3dc75d)) * bump the major group with 4 updates ([#405](#405)) ([20c7a04](20c7a04)) ### Maintenance * bump github.com/spf13/cobra from 1.9.1 to 1.10.2 in /cli in the minor-and-patch group ([#402](#402)) ([e31edbb](e31edbb)) * narrow BSL Additional Use Grant and add CLA ([#408](#408)) ([5ab15bd](5ab15bd)), closes [#406](#406) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
cli/) with 9 Cobra commands:init(interactive huh wizard),start,stop,status(resource usage per container),logs,doctor(diagnostics + pre-filled GitHub issue URL),update(self-update from GitHub Releases with health check after restart),uninstall,versionyamlStrtemplate function with$escaping for Docker Compose variable interpolationcli.ymlworkflow — golangci-lint, go vet, multi-platform test matrix (ubuntu/macos/windows), cross-compile build matrix (6 targets), govulncheck,cli-passgate job, GoReleaser release onv*tags with post-release checksum table in release notescurl | bashfor Linux/macOS,irm | iexfor Windows) with checksum verification and arch detection'healthy'→'ok'(matchesServiceStatus.OK)cli/**/*.go)Test plan
go build ./...compiles without errorsgo test ./...passes (6 packages, all platforms)go vet ./...cleansynthorg init→synthorg start→ health check passesCloses #392
Follow-up: get.synthorg.io → #399