feat: add cross-platform Go CLI for container lifecycle management#395
Closed
Aureliolo wants to merge 5 commits into
Closed
feat: add cross-platform Go CLI for container lifecycle management#395Aureliolo wants to merge 5 commits into
Aureliolo wants to merge 5 commits into
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
Contributor
Dependency ReviewThe following issues were found:
License Issuescli/go.mod
OpenSSF ScorecardScorecard details
Scanned Files
|
Contributor
|
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 (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a full Go-based SynthOrg CLI (init, start, stop, status, logs, update, doctor, uninstall, version) with supporting modules for compose generation, config persistence, Docker detection/exec, health polling, diagnostics, self-update, CI workflows, installers, linters, and docs. No existing public API signatures outside the new CLI module were modified. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as SynthOrg CLI
participant Docker as Docker/Compose
participant Backend as Backend API
participant GitHub as GitHub Releases
User->>CLI: synthorg init
CLI->>CLI: prompt & build State
CLI->>Docker: Detect & check versions
CLI->>CLI: render compose.yml and save state
User->>CLI: synthorg start
CLI->>CLI: load state
CLI->>Docker: docker compose pull
CLI->>Docker: docker compose up -d
CLI->>Backend: poll /api/v1/health
Backend-->>CLI: {data: {status: "ok"}}
CLI-->>User: Ready (Dashboard/API URLs)
User->>CLI: synthorg update
CLI->>GitHub: Check latest release
GitHub-->>CLI: release metadata + asset URLs
CLI->>GitHub: download asset & checksum
CLI->>CLI: verify checksum & extract binary
CLI->>CLI: Replace self
CLI->>Docker: pull new images / recreate containers
CLI-->>User: Updated & running
User->>CLI: synthorg doctor
CLI->>Docker: Compose ps / logs
CLI->>Backend: health check
CLI->>CLI: collect disk/config/info
CLI->>CLI: write diagnostic file
CLI->>GitHub: generate prefilled issue URL
CLI-->>User: diagnostic file path + issue link
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
Contributor
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 significantly enhances the project's operational capabilities and developer experience by introducing a dedicated, cross-platform Go CLI. This new tool streamlines the setup, management, and troubleshooting of the application's Docker-based services, moving away from manual configuration. Concurrently, it refines the internal PR review process by making issue linking more intelligent and user-centric, ensuring better traceability and communication within the development workflow. Highlights
Changelog
Ignored Files
Activity
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
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an impressive pull request that introduces a comprehensive, cross-platform Go CLI for managing the application's lifecycle. The CLI is well-structured with Cobra, provides an excellent user experience with an interactive init wizard, and includes essential commands like start, stop, status, logs, and doctor. The implementation quality is outstanding, particularly the security and robustness of the self-update mechanism, which includes mandatory checksum verification, URL validation, and atomic binary replacement. The addition of detailed diagnostics, CI/CD for the Go binary (including multi-platform builds and releases via GoReleaser), and pre-commit hooks demonstrates a thorough approach. The codebase is clean, well-tested, and shows great attention to detail. I have a few suggestions to further improve the code.
| $Release = Invoke-RestMethod -Uri "https://api.github.com/repos/$Repo/releases/latest" | ||
| $Version = $Release.tag_name | ||
| } else { | ||
| $Version = $env:SYNTHORG_VERSION |
Contributor
There was a problem hiding this comment.
The SYNTHORG_VERSION environment variable is used without validation. The corresponding install.sh script includes validation for this variable to ensure it matches a semantic versioning pattern. A malicious version string could potentially lead to downloading from an unintended URL or other unexpected behavior. It's important to add similar validation here to ensure the version string is in the expected format (e.g., v1.2.3).
$Version = $env:SYNTHORG_VERSION
if ($Version -notmatch '^v[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.]+)?$') {
throw "Error: invalid version string: $Version"
}
Comment on lines
+13
to
+29
| verbose bool | ||
| ) | ||
|
|
||
| var rootCmd = &cobra.Command{ | ||
| Use: "synthorg", | ||
| Short: "SynthOrg CLI — manage your synthetic organization", | ||
| Long: `SynthOrg CLI manages the lifecycle of your synthetic organization. | ||
|
|
||
| Run 'synthorg init' to set up a new installation, then 'synthorg start' | ||
| to launch the backend and web dashboard containers.`, | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, | ||
| } | ||
|
|
||
| func init() { | ||
| rootCmd.PersistentFlags().StringVar(&dataDir, "data-dir", "", "data directory (default: platform-appropriate)") | ||
| rootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "enable verbose output") |
Contributor
There was a problem hiding this comment.
The persistent flag --verbose (and its corresponding verbose variable) is defined for the root command but it's not used anywhere in the CLI logic. This can be misleading for users who expect more detailed output when using the flag. The flag should either be implemented to provide verbose logging/output or removed if it's not intended to be used yet.
| ) | ||
|
|
||
| // imageTagPattern validates image tags to prevent YAML injection. | ||
| var imageTagPattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) |
Contributor
There was a problem hiding this comment.
The imageTagPattern variable is defined but not used anywhere. This appears to be dead code. While it might have been intended for future validation if users could specify an image tag, in its current state it should be removed to improve code clarity.
Greptile SummaryThis PR introduces a well-structured, cross-platform Go CLI ( Key findings from this review pass:
Confidence Score: 3/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: cli/scripts/install.sh
Line: 3-10
Comment:
**`| sh` usage will fail on non-bash shells (e.g., Ubuntu's dash)**
The documented install invocation pipes through `sh`, but the script relies on two bash-specific features that are not available in POSIX `sh` (e.g., `dash`, the default `/bin/sh` on Ubuntu and Debian):
1. **Line 10: `set -euo pipefail`** — the `pipefail` option is not a POSIX `set` option and causes `dash` to error immediately with `Illegal option -o pipefail`, aborting the install before any useful work is done.
2. **Line 78: `EXPECTED_CHECKSUM="${!CHECKSUM_VAR:-}"`** — the `${!var}` indirect expansion syntax is a bash extension; dash will treat this as a literal string reference and produce the wrong (empty) checksum, triggering the "No checksum found" error on line 91.
On any Debian-family system where `/bin/sh -> dash`, running the documented `curl ... | sh` command silently fails with a cryptic error rather than installing the binary.
The simplest fix is to update the usage comment (and the README/docs install commands) to use `bash` explicitly:
```suggestion
# Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | bash
#
```
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/status.go
Line: 86-93
Comment:
**`docker stats` reports all host containers, not just the synthorg stack**
`docker stats --no-stream` returns CPU/memory data for every running container on the Docker host, not just the ones belonging to the synthorg compose project. The function already has the compose project's container IDs available (from the `ps -q` call on line 81), but doesn't use them to filter the stats output.
On a shared host or a developer machine with many containers running, this will flood the `synthorg status` output with resource rows for unrelated services and exposes their names and metrics.
Pass the specific container IDs from the `ps -q` output to `docker stats` to scope the output correctly:
```go
ids := strings.Fields(strings.TrimSpace(psOut))
statsArgs := append([]string{"stats", "--no-stream", "--format",
"table {{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}"}, ids...)
statsOut, err := docker.RunCmd(ctx, "docker", statsArgs...)
```
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: 92-99
Comment:
**`error` log level is missing from the setup wizard options**
`allowedLogLevels` in `generate.go` includes `"error"` as a valid value, but the form only exposes `debug`, `info`, and `warn`. This means:
- Users cannot select `error` log level through `synthorg init` — they must manually edit `config.json`.
- If a user somehow gets `"error"` into their config (e.g., via a future config migration or manual edit), the value is accepted by the validator but was never surfaced as an option.
Consider adding `error` as a fourth option for consistency:
```suggestion
huh.NewSelect[string]().Title("Log level").Options(
huh.NewOption("Debug", "debug"),
huh.NewOption("Info", "info"),
huh.NewOption("Warning", "warn"),
huh.NewOption("Error", "error"),
).Value(&a.logLevel),
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 1736f88 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new cross-platform Go-based synthorg CLI to manage the project’s container lifecycle (init/start/stop/status/logs/doctor/update/uninstall/version), including compose generation, self-update, diagnostics, and supporting CI/release/distribution scaffolding. Also aligns the backend container healthcheck status string with the API contract.
Changes:
- Introduces a new Go module under
cli/(commands + internal packages for config/compose/docker/health/diagnostics/selfupdate). - Adds distribution and automation assets (install scripts, GoReleaser config, Go lint/test/build/vuln CI workflow, Dependabot gomod).
- Updates docs, pre-commit, and Docker healthcheck behavior to support the new CLI workflow.
Reviewed changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/backend/Dockerfile | Updates backend HEALTHCHECK to treat status ok as healthy. |
| cli/testdata/compose_default.yml | Golden file for default compose generation output. |
| cli/testdata/compose_custom_ports.yml | Golden file for custom ports/secret compose generation output. |
| cli/scripts/install.sh | Bash installer for Linux/macOS with checksum verification. |
| cli/scripts/install.ps1 | PowerShell installer for Windows with checksum verification. |
| cli/main.go | CLI entrypoint calling Cobra root command. |
| cli/internal/version/version.go | Build-time version variables (ldflags-injected). |
| cli/internal/selfupdate/updater_test.go | Tests for self-update asset selection, download, checksum, extraction, replacement. |
| cli/internal/selfupdate/updater.go | Self-update implementation (release check, download+verify, archive extraction, atomic replace). |
| cli/internal/health/check_test.go | Tests for backend health polling behavior. |
| cli/internal/health/check.go | Health polling helper with timeout/interval/initial delay. |
| cli/internal/docker/client_test.go | Tests for Docker/Compose helper functions. |
| cli/internal/docker/client.go | Docker/Compose detection + execution helpers. |
| cli/internal/diagnostics/collect_test.go | Tests for diagnostics formatting, truncation, best-effort collection. |
| cli/internal/diagnostics/collect.go | Diagnostics collection + text report generation. |
| cli/internal/config/state_test.go | Tests for persisted state save/load behavior. |
| cli/internal/config/state.go | Persisted CLI config (JSON) save/load logic. |
| cli/internal/config/paths_test.go | Tests for cross-platform data directory resolution. |
| cli/internal/config/paths.go | Data dir resolution + directory creation helper. |
| cli/internal/compose/generate_test.go | Compose generation tests + golden comparisons. |
| cli/internal/compose/generate.go | Compose template rendering and params mapping. |
| cli/internal/compose/compose.yml.tmpl | Embedded compose template with hardening + optional sandbox/JWT secret. |
| cli/go.sum | Go dependency lockfile for CLI module. |
| cli/go.mod | Go module definition and dependencies for CLI. |
| cli/cmd/version.go | synthorg version Cobra command. |
| cli/cmd/update.go | synthorg update command (self-update + image pull/restart prompt). |
| cli/cmd/uninstall.go | synthorg uninstall command with confirmations and cleanup options. |
| cli/cmd/stop.go | synthorg stop command (compose down). |
| cli/cmd/status.go | synthorg status command (compose ps + backend health fetch). |
| cli/cmd/start.go | synthorg start command (pull/up + health wait). |
| cli/cmd/root.go | Root command, global flags, and executor wiring. |
| cli/cmd/logs.go | synthorg logs command with tail/follow passthrough. |
| cli/cmd/init.go | synthorg init interactive setup wizard writing compose + config. |
| cli/cmd/doctor.go | synthorg doctor diagnostics + prefilled GitHub issue URL. |
| cli/.goreleaser.yml | GoReleaser configuration for multi-platform builds + brew/scoop publishing. |
| cli/.golangci.yml | golangci-lint configuration for the CLI module. |
| README.md | Updates Quick Start to prefer CLI install + init/start workflow. |
| CLAUDE.md | Documents CLI structure, commands, and CI/path-filtering changes. |
| .pre-commit-config.yaml | Adds CLI Go hooks (golangci-lint/go vet/go test) on pre-push. |
| .gitignore | Ignores CLI build outputs and dist/coverage artifacts. |
| .github/workflows/cli.yml | Adds dedicated CLI CI workflow (lint/test/build/govulncheck/release). |
| .github/workflows/ci.yml | Adds path-filter gating to skip unrelated Python/dashboard/docker jobs. |
| .github/dependabot.yml | Adds gomod Dependabot updates for cli/. |
| .github/ISSUE_TEMPLATE/installer-failure.yml | Adds issue template for CLI installation failures. |
| .dockerignore | Excludes cli/ from Docker build context. |
| .claude/skills/pre-pr-review/SKILL.md | Updates pre-PR review skill to include CLI Go domains + issue detection rules. |
| .claude/skills/aurelio-review-pr/SKILL.md | Updates PR review skill to include CLI Go domains + issue detection rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+94
| // Trim whitespace from user input. | ||
| dir = strings.TrimSpace(dir) |
Comment on lines
+108
to
+112
| dockerSock = strings.TrimSpace(dockerSock) | ||
| if !filepath.IsAbs(dockerSock) && !strings.HasPrefix(dockerSock, "//") { | ||
| return fmt.Errorf("docker socket must be an absolute path, got %q", dockerSock) | ||
| } | ||
| } |
Comment on lines
+45
to
+49
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return DefaultState(), nil | ||
| } |
Comment on lines
+69
to
+73
| healthURL := fmt.Sprintf("http://localhost:%d/api/v1/health", state.BackendPort) | ||
| client := &http.Client{Timeout: 5 * time.Second} | ||
| if resp, err := client.Get(healthURL); err != nil { | ||
| r.HealthStatus = "unreachable" | ||
| r.Errors = append(r.Errors, fmt.Sprintf("health: %v", err)) |
Comment on lines
+69
to
+83
| resp, err := client.Get(healthURL) | ||
| if err != nil { | ||
| fmt.Fprintf(out, " Backend: unreachable (%v)\n", err) | ||
| } else { | ||
| defer resp.Body.Close() | ||
| body, readErr := io.ReadAll(io.LimitReader(resp.Body, 64*1024)) | ||
| if readErr != nil { | ||
| fmt.Fprintf(out, " Backend: error reading response (%v)\n", readErr) | ||
| return nil | ||
| } | ||
| var hr map[string]any | ||
| if json.Unmarshal(body, &hr) == nil { | ||
| fmt.Fprintf(out, " Backend: %s\n", prettyJSON(hr)) | ||
| } else { | ||
| fmt.Fprintf(out, " Backend: %s (HTTP %d)\n", string(body), resp.StatusCode) |
Comment on lines
+20
to
+21
| // imageTagPattern validates image tags to prevent YAML injection. | ||
| var imageTagPattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) |
Comment on lines
+57
to
+61
| // ComposeExec runs `docker compose` (or `docker-compose`) with the given | ||
| // arguments, forwarding stdout/stderr to the provided writers. | ||
| func ComposeExec(ctx context.Context, info Info, dir string, args ...string) error { | ||
| parts := strings.Fields(info.ComposePath) | ||
| parts = append(parts, args...) |
| # If GORELEASER_TAP_TOKEN is not set, tap/bucket updates are | ||
| # skipped (GoReleaser handles this gracefully). | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GORELEASER_KEY: ${{ secrets.GORELEASER_TAP_TOKEN }} |
Comment on lines
+16
to
+20
| if (-not $env:SYNTHORG_VERSION) { | ||
| Write-Host "Fetching latest release..." | ||
| $Release = Invoke-RestMethod -Uri "https://api.github.com/repos/$Repo/releases/latest" | ||
| $Version = $Release.tag_name | ||
| } else { |
Comment on lines
+13
to
+16
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(`{"data":{"status":"ok"}}`)) | ||
| })) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 48
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 reordering Go hooks for faster feedback.
The current order runs
golangci-lintbeforego-vet, butgo vetis typically faster and catches fundamental issues. Running it first provides quicker feedback on obvious problems before the more comprehensive lint pass.♻️ Suggested hook order
- repo: local hooks: + - 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: 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: mypy ... - id: pytest-unit ... - - 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]Also, there's an extra blank line at line 62 that could be removed for consistency.
🤖 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, Reorder the pre-commit hooks so the faster, basic Go checks run before the heavier lint: move the hook with id "go-vet" so it appears before the "golangci-lint" hook (keep the same entries and settings for both "go-vet" and "golangci-lint"), and remove the stray extra blank line currently between hooks to keep formatting consistent; reference the hooks by their ids "go-vet" and "golangci-lint" and adjust ordering accordingly.
🤖 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-217: The ordered list in SKILL.md currently uses explicit
numbers "10.", "11.", "12." for items labeled "Vet:", "Test:", and "Build
check:" which violates markdownlint MD029; change those list markers to use "1."
(i.e., make each ordered item use "1.") so the entire ordered list uses the
repeated "1." style, updating the list entries containing "Vet:", "Test:", and
"Build check:" accordingly.
- Around line 203-224: Update the post-fix re-verification phases to include the
existing "Go CLI checks (steps 10-12)" so Go vet, go test, and go build run
after fixes; modify the Phase 8/Phase 9 re-verification sections in SKILL.md to
conditionally re-run steps 10–12 whenever cli_go or cli_config files changed
(the same condition used in the Phase 2 gating), ensuring the docs state "cd cli
&& go vet ./...", "cd cli && go test ./...", and "cd cli && go build ./..." are
executed as part of the post-fix checks to prevent regressions.
In @.github/ISSUE_TEMPLATE/installer-failure.yml:
- Around line 8-12: Insert a clear redaction warning block immediately before
any prompt that requests full terminal output or proxy details: update the
markdown "value" section (the type: markdown block containing "Thanks for
reporting an installation issue...") to prepend a short warning that the form is
public and requesters must redact secrets (API tokens, SSH keys, proxy
credentials, internal hostnames), give one-line examples of what to redact, and
link to a redaction guide or suggest replacing secrets with "<REDACTED>" so
reporters don't accidentally publish sensitive data.
In @.github/workflows/ci.yml:
- Around line 23-26: The workflow filter currently only gates jobs by domain
outputs (outputs.python, outputs.dashboard, outputs.docker) which lets a PR that
only edits the workflow file skip all gated jobs and still mark ci-pass as
successful; update each gated job (e.g., the
lint/mypy/pytest/python-audit/dockerfile-lint and dashboard-* jobs referenced in
the file) to also check the workflow predicate from the filter step (the
outputs.workflow value) in their if: condition so those jobs run when the
workflow itself changed (or conversely ensure they require outputs.workflow ==
'false' only to skip), and keep the ci-pass gate logic consistent with the added
outputs.workflow predicate.
In @.github/workflows/cli.yml:
- Around line 148-149: Replace the non-hermetic installation "go install
golang.org/x/vuln/cmd/govulncheck@latest" in the "Install govulncheck" step with
a pinned release (e.g., "@vX.Y.Z" or a repo-level variable) so the workflow
installs a specific govulncheck version; update the run line to use that pinned
version the same way other actions/GoReleaser are pinned.
- Around line 4-13: The workflow's push and pull_request path filters only
include "cli/**", which prevents changes to the workflow file itself from
triggering runs; update both the push and pull_request blocks (the paths arrays
under push and pull_request) to also include ".github/workflows/cli.yml" so
edits to the workflow file will trigger the workflow immediately.
- Around line 156-163: Add job-level environment protection to the cli-release
job so the GORELEASER_TAP_TOKEN (and any other release secrets) are gated by the
protected release environment; update the cli-release job block (job name:
cli-release) to include an environment: release entry (matching the repo's
protected environment used elsewhere) and ensure the GORELEASER_TAP_TOKEN is
provided via that environment's secrets rather than being injected at workflow
scope.
In `@cli/.golangci.yml`:
- Around line 6-22: Add the gosec security linter to the enabled linters by
updating the linters enable list (the "linters: enable:" block) to include
"gosec" so that static security checks run alongside errcheck/govet/etc.;
optionally, if desired, also add a gosec-specific configuration block to tune
rules and severity.
In `@cli/cmd/doctor.go`:
- Around line 41-44: The write to savePath using os.WriteFile can fail if
state.DataDir doesn't exist; before calling os.WriteFile in the doctor command
(where filename/savePath are computed), ensure the directory exists by calling
os.MkdirAll(state.DataDir, 0o700) (or similar perms), handle and report any
error from MkdirAll to cmd.ErrOrStderr(), and only proceed to os.WriteFile if
the directory creation succeeded so the savePath write no longer returns "no
such file or directory".
In `@cli/cmd/init.go`:
- Around line 20-21: The declared but unused regex imageTagPattern should be
applied to validate image tags in runInit: when reading or assigning
state.ImageTag inside runInit, test it against imageTagPattern and
return/propagate a clear error if it doesn't match (preventing
invalid/YAML-injection tags); if validation is already handled elsewhere, delete
the imageTagPattern declaration to avoid dead code. Reference the symbols
imageTagPattern, runInit, and state.ImageTag when making the change.
- Around line 106-112: The current sandbox validation only checks that
dockerSock is absolute (using filepath.IsAbs and strings.HasPrefix) but does not
guard against YAML-special characters; update the validation inside the sandbox
branch to also reject any dockerSock containing YAML-special characters (e.g.,
':', '#', '\'', '"', '|', '>', '&', '%', '*', '?', '@', '!', newline/tab) by
checking dockerSock with strings.ContainsAny or a simple regexp and returning a
descriptive error (include the dockerSock value) if any are present; keep the
existing absolute-path checks (dockerSock, sandbox, filepath.IsAbs,
strings.HasPrefix) and add the additional conditional that returns fmt.Errorf
when the forbidden chars are found.
- Around line 123-132: The state currently persists DockerSock even when sandbox
is disabled; update the init flow that builds the config.State (the state
variable) so that if the sandbox boolean is false you clear DockerSock (e.g.,
set dockerSock = "" or set DockerSock: "" in the struct) before assigning it to
config.State. Locate the code that constructs state := config.State{...
DockerSock: dockerSock ...} and ensure DockerSock is conditionally cleared when
sandbox == false so saved state does not contain a docker socket path when
sandboxing is disabled.
In `@cli/cmd/logs.go`:
- Around line 55-56: The --tail validation currently accepts 0; update the
numeric check in the parsing block that uses strconv.Atoi on the variable tail
so it rejects zero by changing the condition from n < 0 to n <= 0 (the error
return already uses fmt.Errorf("--tail must be a positive integer or 'all', got
%q", logTail) so leave that message unchanged). Locate the parsing/validation
code that assigns n, err := strconv.Atoi(tail) (in cli/cmd/logs.go) and replace
the n < 0 check with n <= 0 to enforce a strictly positive integer for --tail.
In `@cli/cmd/root.go`:
- Around line 11-14: Remove the dead `verbose` flag: delete the `verbose`
variable declaration and remove its PersistentFlags registration (the
BoolVar/BoolVarP call that wires `verbose` to the CLI) from the `root.go`
init/command setup, ensuring there are no remaining references to the `verbose`
symbol (search for `verbose` usage) before committing; if you prefer to keep a
verbosity flag instead, implement its behavior where commands respect the
`verbose` boolean (e.g., adjust logging verbosity) and wire that logic to the
same `verbose` symbol.
In `@cli/cmd/start.go`:
- Around line 38-40: The os.Stat check for composePath currently only handles
the IsNotExist case; update the compose file existence check (around composePath
and state.DataDir in the function that calls os.Stat) to handle other errors by
returning them immediately with context—i.e., if err != nil and not
os.IsNotExist(err) return a wrapped or formatted error describing that Stat
failed for composePath (include err), and only for os.IsNotExist(err) return the
existing "compose.yml not found..." message.
In `@cli/cmd/status.go`:
- Around line 67-85: The health check hardcodes "localhost" when building
healthURL which breaks checks if the backend binds elsewhere; update the
construction of healthURL in status.go (the healthURL variable used with
client.Get) to use the configured host instead of "localhost" (e.g., read from
state.BackendHost or equivalent config), fall back to localhost only if no host
is configured, and ensure any related documentation/comment reflects this
behavior so the health check uses the actual backend bind address.
- Around line 90-96: The prettyJSON function currently passes a non-empty prefix
(" ") to json.MarshalIndent which inserts two leading spaces on every line;
update prettyJSON to use an empty prefix and keep the indent (e.g., call
json.MarshalIndent with prefix "" and indent " ") so the first line is not
prefixed and nested lines are still indented (modify the prettyJSON function
accordingly).
In `@cli/cmd/stop.go`:
- Around line 37-40: The docker.Detect call returns an error that should be
wrapped consistently with other errors in stop.go; update the error return after
the docker.Detect(ctx) call (the variables info, err from docker.Detect) to wrap
the original err with contextual text (e.g., "detecting docker: %w") before
returning so the error flows include the same context style as other returns
like "loading config: %w" and "stopping containers: %w".
- Around line 25-35: The code in cli/cmd/stop.go uses state.DataDir without
validating it, which can result in a relative path when DataDir is empty; update
the logic after config.Load(dir) to fall back to the resolved dir returned by
resolveDataDir() when state.DataDir == "" (i.e. set composePath to
state.DataDir, and if empty assign dir, then filepath.Join(composePath,
"compose.yml") before os.Stat). Apply the same fix to the identical composePath
handling in cli/cmd/start.go and cli/cmd/logs.go so they also use dir as the
fallback when state.DataDir is empty.
In `@cli/cmd/uninstall.go`:
- Around line 84-89: Before calling os.RemoveAll on state.DataDir (in the
uninstall/removeData block), resolve and validate the path: use filepath.Clean +
filepath.Abs on state.DataDir, reject empty paths or obvious root paths (e.g.
"/" on Unix or "<Drive>:\\" on Windows) and refuse to proceed if the resolved
path equals root or has too shallow depth; ideally also ensure the resolved path
is contained within an expected safe base (e.g. compare prefix against
state.InstallDir or a known app data root) and only then call os.RemoveAll; if
validation fails, return an error instead of deleting.
In `@cli/cmd/update.go`:
- Around line 72-73: The code currently ignores the error returned by
docker.ComposeExecOutput when running "ps -q", which can hide failures and
incorrectly skip the restart prompt; update the call to capture both psOut and
err (e.g., psOut, err := docker.ComposeExecOutput(ctx, info, state.DataDir,
"ps", "-q")), then handle err before using psOut — either return or log the
error and force the restart flow decision (do not treat a command error as an
empty psOut), and ensure any downstream logic that checks psOut (the if psOut !=
"" branch) only runs when err is nil; reference docker.ComposeExecOutput, psOut,
err, ctx, info, and state.DataDir when making the change.
- Around line 90-93: After composeRun succeeds, add a post-restart readiness
probe before printing "Containers restarted with new images.": call a new or
existing health-check helper (e.g., waitForBackendReady or CheckBackendHealth)
from cli/cmd/update.go to poll the backend's health endpoint with a configurable
timeout and backoff, returning an error if the service never becomes ready; only
print the success message after the health check returns healthy and propagate
the health-check error (wrap with context like "waiting for backend readiness")
if it fails; update imports for time/context/http as needed.
In `@cli/go.mod`:
- Line 14: Update the indirect dependency github.com/charmbracelet/bubbles from
the pre-release pseudo-version to the latest stable (v2.0.0): edit the go.mod
entry for github.com/charmbracelet/bubbles to v2.0.0 or run `go get
github.com/charmbracelet/bubbles@v2.0.0`, then run `go mod tidy` to reconcile
transitive requirements (verify charmbracelet/huh compatibility and adjust code
if any API changes surface). Ensure the module name
github.com/charmbracelet/bubbles is the one updated and run tests/build to
confirm no breaking changes.
- Around line 1-39: The dependency-review workflow is rejecting
golang.org/x/sync, golang.org/x/sys, and golang.org/x/text due to the Google
patent license identifier; open .github/workflows/dependency-review.yml and add
LicenseRef-scancode-google-patent-license-golang to the allow-licenses list (or
add the three modules under allow-dependencies-licenses) so the
dependency-review check accepts the Google-patent-licensed BSD-3-Clause variants
referenced in the cli/go.mod dependencies (golang.org/x/sync, golang.org/x/sys,
golang.org/x/text).
In `@cli/internal/compose/compose.yml.tmpl`:
- Around line 29-31: The healthcheck's inline Python command under the
healthcheck:test field is complex but intentional due to a read-only filesystem
and absence of curl in the image; add an inline YAML comment next to the test
entry (or directly above the healthcheck block) explaining that curl/cli tools
are not available in the container image and therefore a small Python one-liner
is used to perform the HTTP GET and JSON check, so future reviewers understand
why the Python approach is required.
- Around line 17-19: The template in compose.yml.tmpl renders JWTSecret
unescaped which can break YAML if the secret contains quotes, colons or
newlines; update the template to escape or quote the value when rendering (e.g.
use the Go template printf "%q" or another YAML-escaping template function) so
AI_COMPANY_JWT_SECRET is always a safe double-quoted YAML string, and optionally
add a validation step near generateSecret in init.go to ensure secrets are
URL-safe/base64 or reject/normalize unsafe values before templating.
In `@cli/internal/compose/generate_test.go`:
- Around line 124-152: TestParamsFromState currently omits asserting the
CLIVersion; update the test to validate that ParamsFromState(s).CLIVersion
equals version.Version by adding an assertion in TestParamsFromState that
compares p.CLIVersion to version.Version (use the existing p variable from
ParamsFromState and the version.Version symbol).
- Around line 161-188: The compareGolden helper currently auto-creates missing
golden files inside compareGolden, which can mask missing/removed goldens in CI;
update compareGolden so that if the golden file is missing it fails the test
(t.Fatalf) unless UPDATE_GOLDEN== "1" is set (or explicitly allowed for local
runs), instead of silently creating the file — change the branch where
os.ReadFile(golden) returns an error: if os.Getenv("UPDATE_GOLDEN") == "1" then
create/update the file as before, otherwise call t.Fatalf("missing golden file
%s; run with UPDATE_GOLDEN=1 to create", name); keep function name compareGolden
and preserve the existing UPDATE_GOLDEN behavior for local updates.
In `@cli/internal/config/paths_test.go`:
- Around line 83-91: The test TestEnsureDirIdempotent ignores the error from
os.MkdirAll; update the setup to check and fail on that error so test setup
cannot silently fail: call os.MkdirAll(target, 0o700) and if it returns a
non-nil error use t.Fatalf (or t.Fatalf-like) to report "failed to create
existing dir" including the error, keeping the rest of the test invoking
EnsureDir(target) unchanged; this touches the TestEnsureDirIdempotent function
and references os.MkdirAll and EnsureDir.
In `@cli/internal/config/state_test.go`:
- Around line 87-104: TestSaveFilePermissions currently only validates JSON;
update it to assert the saved file mode so it fails if permissions regress
(e.g., generated file should be 0600). After calling Save(s) and reading the
file with os.Stat(StatePath(tmp)), check FileMode() & os.FileMode(0777) == 0600
and use t.Fatalf/t.Errorf on mismatch, but skip or adjust the assertion on
Windows (runtime.GOOS == "windows") to avoid false failures. Keep references to
TestSaveFilePermissions, Save, and StatePath when locating the change.
- Around line 106-116: TestLoadMissing only asserts BackendPort but not DataDir,
hiding a first-run path bug; update the test (TestLoadMissing) to also assert
that DataDir equals the expected default from DefaultState(): call
DefaultState() or otherwise obtain the OS default data dir used by DefaultState
and compare s.DataDir to that value, failing the test if they differ so
Load(tmp) returning defaults includes the correct DataDir.
In `@cli/internal/config/state.go`:
- Around line 67-75: Save currently allows empty or relative DataDir values and
can write config.json to the wrong place; validate State.DataDir at the start of
Save and return an error if it's empty, "." or a relative path. Specifically, in
the Save function (and before calling EnsureDir/StatePath), check s.DataDir !=
"" and that filepath.IsAbs(s.DataDir) is true and s.DataDir != "." (mirroring
Load's validation and cli/internal/config/paths.go behavior), and return a clear
error if the check fails so callers cannot persist an invalid state directory.
- Around line 43-63: Load currently returns DefaultState() when the config file
is missing, which discards the caller-provided dataDir; change the
os.ErrNotExist branch so it constructs the default state but preserves and
canonicalizes the requested dataDir: create s := DefaultState(), and if the
incoming dataDir is non-empty set s.DataDir = filepath.Clean(dataDir) and
validate it's absolute (same check as later), then return s, nil; keep the
existing canonicalization/validation logic for loaded states (referencing Load,
DefaultState, State.DataDir, and StatePath).
In `@cli/internal/diagnostics/collect_test.go`:
- Around line 73-90: Update TestCollectDoesNotPanic in collect_test.go to
explicitly assert that the JWT secret is redacted in the produced report: after
calling Collect(context.Background(), state) add an assertion that
report.Secrets.JWTSecret is the expected redacted value (or matches the
redaction pattern used by Collect, e.g. "[REDACTED]" or a constant used for
redaction) and not the raw secret from state; reference the Collect function and
the report.Secrets.JWTSecret field to locate the code to change.
In `@cli/internal/diagnostics/collect.go`:
- Around line 58-65: The code silently ignores errors from
docker.ComposeExecOutput when gathering "ps" and "logs"; update the blocks that
call docker.ComposeExecOutput (for "ps" and for "logs") to append any non-nil
error to the report's Errors slice (Report.Errors / r.Errors) instead of
dropping it: on err != nil push a descriptive string (including the command,
state.DataDir, and err.Error()) into r.Errors, and keep the existing success
behavior (setting r.ContainerPS and r.RecentLogs) when err == nil; reference the
calls to docker.ComposeExecOutput, r.ContainerPS, r.RecentLogs and r.Errors when
making the change.
In `@cli/internal/docker/client.go`:
- Around line 59-79: Both ComposeExec and ComposeExecOutput assume
strings.Fields(info.ComposePath) yields at least one element and index parts[0],
causing a panic for empty/malformed ComposePath; add a guard at the start of
both functions that checks if info.ComposePath is empty or if parts :=
strings.Fields(info.ComposePath); len(parts) == 0, and return a clear error
(e.g., fmt.Errorf("empty compose path: %q", info.ComposePath)) instead of
proceeding; keep rest of logic unchanged so exec.CommandContext is only called
when parts[0] is valid.
In `@cli/internal/health/check_test.go`:
- Around line 53-70: The test TestWaitForHealthyEventualSuccess uses a
non-atomic counter variable calls inside the httptest handler which can be racy;
change calls to an int32/uint32 and update it using
atomic.AddInt32/atomic.AddUint32 (or capture the returned value) and compare
that atomic value to 3 when deciding to return 503 vs 200, ensuring all accesses
use the atomic package (e.g., use atomic.AddInt32(&calls, 1) and compare the
returned value or use atomic.LoadInt32(&calls)).
In `@cli/internal/health/check.go`:
- Around line 33-49: The loop currently waits for the first ticker event before
calling checkOnce, causing a missed or delayed probe; after the startup delay,
perform an immediate probe by calling checkOnce(ctx, url) once before
creating/entering the ticker loop (update handling of lastErr and early return
on success), then proceed to the existing ticker-based retries using ticker,
interval, lastErr, ctx and url; ensure behavior and error wrapping on timeout
remain unchanged.
- Around line 39-43: The ctx.Done() branch in the health check loop incorrectly
maps all cancellations to a timeout message; modify the branch in the function
containing the loop so that if ctx.Err() is non-nil you return
fmt.Errorf("health check canceled: %w", ctx.Err()) (or simply return ctx.Err())
when the context was canceled, otherwise preserve the existing behavior that
returns the wrapped lastErr for timeouts (e.g., if lastErr != nil return
fmt.Errorf("health check timed out (last error: %w)", lastErr) else return
fmt.Errorf("health check timed out")). Ensure you check
ctx.Err()/context.Canceled vs deadline exceeded semantics so Ctrl-C/upstream
cancels are distinguishable from real timeouts.
- Around line 22-34: The WaitForHealthy function must validate its duration
parameters to avoid panics: at the top of WaitForHealthy check timeout, interval
and initialDelay and return a descriptive error if timeout <= 0, if interval <=
0, or if initialDelay < 0; do not call context.WithTimeout, time.After or
time.NewTicker until the parameters are validated. Reference the function name
WaitForHealthy and the parameter names (ctx, url, timeout, interval,
initialDelay) in the error messages so callers can see which argument is
invalid.
In `@cli/internal/selfupdate/updater_test.go`:
- Line 585: Replace the hard-coded Unix-style missing path with a deterministic
path built from t.TempDir(); e.g., in the test call to ReplaceAt([]byte("data"),
"/nonexistent/path/binary") replace the second argument with
filepath.Join(t.TempDir(), "nonexistent", "binary") (assign t.TempDir() to a
variable if needed) and add an import for "path/filepath" if it's not already
present so the test consistently fails across platforms.
In `@cli/internal/selfupdate/updater.go`:
- Around line 110-127: The CheckFromURL path currently may return a result with
empty result.AssetURL or result.ChecksumURL causing downstream Download to fail;
after the loop that sets result.AssetURL/result.ChecksumURL (in the function
handling release.Assets), validate both fields and if either is empty return a
clear error (e.g. "missing asset URL" or "missing checksum URL") instead of
returning result,nil so callers like Download get immediate actionable feedback;
update the check inside the same function (referencing assetName(),
result.AssetURL, result.ChecksumURL, and Download) to fail fast when metadata is
incomplete.
- Around line 229-243: httpGetWithClient currently uses io.LimitReader which
silently truncates large responses; change the read logic to detect oversize by
reading up to maxBytes+1 bytes from resp.Body (e.g., via
io.LimitReader(resp.Body, maxBytes+1)) and if the returned payload length is
greater than maxBytes return a clear error like "response larger than %d bytes"
instead of returning truncated data; keep the existing request/response/error
handling and defer resp.Body.Close() logic in httpGetWithClient so callers
receive an explicit error when the HTTP body exceeds maxBytes.
In `@cli/scripts/install.ps1`:
- Around line 28-30: Detect the runtime architecture instead of hardcoding
"amd64": add a small arch detection and mapping (e.g. read
$env:PROCESSOR_ARCHITECTURE or
[System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture, map "AMD64"
-> "amd64", "ARM64" -> "arm64"), then build $ArchiveName using that mapped arch
(replace the hardcoded "synthorg_windows_amd64.zip" with e.g.
"synthorg_windows_${arch}.zip") and use the resulting $ArchiveName when
composing $DownloadUrl and $ChecksumsUrl so Windows arm64 users download the
correct artifact.
- Around line 16-22: Validate the $env:SYNTHORG_VERSION value before using it to
build GitHub release URLs: after the existing if/else, check that $Version (when
set from $env:SYNTHORG_VERSION) matches an allowed release-tag pattern (e.g.,
semver with optional leading "v" such as ^v?\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$)
and if it does not, Write-Error a clear message and exit with non-zero status
instead of proceeding; update the branch that assigns $Version from
$env:SYNTHORG_VERSION to perform this regex validation and fail fast to avoid
producing confusing 404s.
In `@cli/scripts/install.sh`:
- Around line 1-3: The install script and docs currently instruct users to pipe
the curl installer into sh (`| sh`), which fails because the script relies on
Bash features; update the installer invocation strings to use `| bash`
instead—replace occurrences of `| sh` in the install script (shebang/invocation
line), the README installer example, and the installer examples in the
ISSUE_TEMPLATE to `| bash` so the script runs under Bash as intended.
- Around line 77-83: The install block fails on missing INSTALL_DIR creation and
runs chmod as non-root after a sudo mv, so create the target directory and
perform move+permission changes under the same privilege; update the block that
uses INSTALL_DIR, BINARY_NAME, and TMP_DIR so it runs mkdir -p "${INSTALL_DIR}"
and then either uses a single privileged command (e.g., sudo sh -c 'mv
"${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}" && chmod +x
"${INSTALL_DIR}/${BINARY_NAME}"') or uses an atomic helper like install to move
and set executable bit in one step so ownership and chmod happen together.
In `@README.md`:
- Around line 81-85: The Scoop example uses a single line with "scoop bucket add
aureliolo https://github.com/Aureliolo/scoop-bucket && scoop install synthorg",
which fails on Windows PowerShell 5.1 because && isn't supported; split this
into two separate commands so users run the bucket-add first ("scoop bucket add
aureliolo https://github.com/Aureliolo/scoop-bucket") and then run the install
("scoop install synthorg"), updating the README entry accordingly.
---
Outside diff comments:
In @.pre-commit-config.yaml:
- Around line 54-91: Reorder the pre-commit hooks so the faster, basic Go checks
run before the heavier lint: move the hook with id "go-vet" so it appears before
the "golangci-lint" hook (keep the same entries and settings for both "go-vet"
and "golangci-lint"), and remove the stray extra blank line currently between
hooks to keep formatting consistent; reference the hooks by their ids "go-vet"
and "golangci-lint" and adjust ordering accordingly.
🪄 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: 4079f3d2-dd46-4397-ac90-8fef172ece51
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
.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.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/Dockerfile
Comment on lines
+203
to
+224
| **Go CLI checks (steps 10-12):** Run only if `cli_go` or `cli_config` files changed. | ||
|
|
||
| 10. **Vet:** | ||
|
|
||
| ```bash | ||
| cd cli && go vet ./... | ||
| ``` | ||
|
|
||
| 11. **Test:** | ||
|
|
||
| ```bash | ||
| cd cli && go test ./... | ||
| ``` | ||
|
|
||
| 12. **Build check:** | ||
|
|
||
| ```bash | ||
| cd cli && go build ./... | ||
| ``` | ||
|
|
||
| If steps 10-12 fail, fix the Go code and re-run. | ||
|
|
Contributor
There was a problem hiding this comment.
Add Go re-verification to post-fix phases to prevent regressions.
Go checks were added in Phase 2, but Phase 8/9 re-verification still only covers Python/Web. If Go files change during fixes or simplification, regressions can slip through before push.
Suggested doc patch
## Phase 8: Post-Fix Verification
Run automated checks again (same conditional gating as Phase 2):
@@
**Web dashboard checks (steps 5-7):** Run only if `web_src` or `web_test` files were changed or modified during Phase 7.
5. `npm --prefix web run lint`
6. `npm --prefix web run type-check`
7. `npm --prefix web run test`
+
+**Go CLI checks (steps 8-10):** Run only if `cli_go` or `cli_config` files were changed or modified during Phase 7.
+
+8. `cd cli && go vet ./...`
+9. `cd cli && go test ./...`
+10. `cd cli && go build ./...`
@@
3. Re-run verification (same conditional gating as Phase 8):
- If `src_py` or `test_py` changed: `uv run ruff check src/ tests/` + `uv run ruff format src/ tests/` + `uv run mypy src/ tests/` + `uv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80`
- If `web_src` or `web_test` changed: `npm --prefix web run lint` + `npm --prefix web run type-check` + `npm --prefix web run test`
+ - If `cli_go` or `cli_config` changed: `cd cli && go vet ./...` + `cd cli && go test ./...` + `cd cli && go build ./...`🧰 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 203 - 224, Update the
post-fix re-verification phases to include the existing "Go CLI checks (steps
10-12)" so Go vet, go test, and go build run after fixes; modify the Phase
8/Phase 9 re-verification sections in SKILL.md to conditionally re-run steps
10–12 whenever cli_go or cli_config files changed (the same condition used in
the Phase 2 gating), ensuring the docs state "cd cli && go vet ./...", "cd cli
&& go test ./...", and "cd cli && go build ./..." are executed as part of the
post-fix checks to prevent regressions.
Comment on lines
+205
to
+217
| 10. **Vet:** | ||
|
|
||
| ```bash | ||
| cd cli && go vet ./... | ||
| ``` | ||
|
|
||
| 11. **Test:** | ||
|
|
||
| ```bash | ||
| cd cli && go test ./... | ||
| ``` | ||
|
|
||
| 12. **Build check:** |
Contributor
There was a problem hiding this comment.
Fix ordered-list numbering to satisfy markdownlint MD029.
This list uses 10/11/12; with the configured style, each ordered item should use 1..
Suggested doc patch
-10. **Vet:**
+1. **Vet:**
@@
-11. **Test:**
+1. **Test:**
@@
-12. **Build check:**
+1. **Build check:**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 10. **Vet:** | |
| ```bash | |
| cd cli && go vet ./... | |
| ``` | |
| 11. **Test:** | |
| ```bash | |
| cd cli && go test ./... | |
| ``` | |
| 12. **Build check:** | |
| 1. **Vet:** |
🧰 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 - 217, The ordered
list in SKILL.md currently uses explicit numbers "10.", "11.", "12." for items
labeled "Vet:", "Test:", and "Build check:" which violates markdownlint MD029;
change those list markers to use "1." (i.e., make each ordered item use "1.") so
the entire ordered list uses the repeated "1." style, updating the list entries
containing "Vet:", "Test:", and "Build check:" accordingly.
Comment on lines
+8
to
+12
| - type: markdown | ||
| attributes: | ||
| value: | | ||
| Thanks for reporting an installation issue. Please fill in the details below. | ||
|
|
Contributor
There was a problem hiding this comment.
Add a redaction warning before asking for full logs.
This form is public, but it explicitly asks for full terminal output and proxy context. Without an upfront warning, reporters will paste tokens, proxy credentials, or internal hostnames into a public issue.
💡 Suggested change
value: |
Thanks for reporting an installation issue. Please fill in the details below.
+
+ Before submitting, redact tokens, passwords, proxy credentials, internal hostnames, and any other sensitive data from terminal output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/ISSUE_TEMPLATE/installer-failure.yml around lines 8 - 12, Insert a
clear redaction warning block immediately before any prompt that requests full
terminal output or proxy details: update the markdown "value" section (the type:
markdown block containing "Thanks for reporting an installation issue...") to
prepend a short warning that the form is public and requesters must redact
secrets (API tokens, SSH keys, proxy credentials, internal hostnames), give
one-line examples of what to redact, and link to a redaction guide or suggest
replacing secrets with "<REDACTED>" so reporters don't accidentally publish
sensitive data.
Comment on lines
+23
to
+26
| outputs: | ||
| python: ${{ steps.filter.outputs.python }} | ||
| dashboard: ${{ steps.filter.outputs.dashboard }} | ||
| docker: ${{ steps.filter.outputs.docker }} |
Contributor
There was a problem hiding this comment.
Workflow-only PRs can go green without running any checks.
If a PR only edits .github/workflows/ci.yml, the path filter leaves every domain output false, all gated jobs are skipped, and ci-pass still succeeds because skipped counts as passing. That defeats the CI gate for changes to the workflow itself.
💡 Suggested change
outputs:
python: ${{ steps.filter.outputs.python }}
dashboard: ${{ steps.filter.outputs.dashboard }}
docker: ${{ steps.filter.outputs.docker }}
+ workflow: ${{ steps.filter.outputs.workflow }}
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
persist-credentials: false
@@
with:
filters: |
+ workflow:
+ - '.github/workflows/ci.yml'
+ - '.github/actions/**'
python:
- 'src/**'
- 'tests/**'
- 'pyproject.toml'
- 'uv.lock'
- '.github/actions/**'
@@
- if: needs.changes.outputs.python == 'true' || github.event_name == 'workflow_dispatch'
+ if: needs.changes.outputs.python == 'true' || needs.changes.outputs.workflow == 'true' || github.event_name == 'workflow_dispatch'Apply the same workflow predicate to the other gated jobs as well.
As per coding guidelines, .github/workflows/ci.yml: CI jobs run in parallel: lint (ruff) + type-check (mypy) + test (pytest + coverage) + python-audit (pip-audit) + dockerfile-lint (hadolint) + dashboard-lint/type-check/test/build/audit (npm) → ci-pass gate.
Also applies to: 34-45, 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 23 - 26, The workflow filter currently
only gates jobs by domain outputs (outputs.python, outputs.dashboard,
outputs.docker) which lets a PR that only edits the workflow file skip all gated
jobs and still mark ci-pass as successful; update each gated job (e.g., the
lint/mypy/pytest/python-audit/dockerfile-lint and dashboard-* jobs referenced in
the file) to also check the workflow predicate from the filter step (the
outputs.workflow value) in their if: condition so those jobs run when the
workflow itself changed (or conversely ensure they require outputs.workflow ==
'false' only to skip), and keep the ci-pass gate logic consistent with the added
outputs.workflow predicate.
Comment on lines
+6
to
+22
| linters: | ||
| enable: | ||
| - errcheck | ||
| - govet | ||
| - ineffassign | ||
| - staticcheck | ||
| - unused | ||
| - gosimple | ||
| - gocritic | ||
| - gofmt | ||
| - bodyclose | ||
| - errname | ||
| - errorlint | ||
| - noctx | ||
| - prealloc | ||
| - unconvert | ||
| - unparam |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good linter selection; consider adding gosec for security checks.
The enabled linters provide solid coverage for code quality, error handling, and performance. Given that this CLI handles sensitive operations (Docker management, self-updates with checksum verification), consider enabling gosec to catch common security issues.
♻️ Optional: add gosec linter
linters:
enable:
- errcheck
- govet
- ineffassign
- staticcheck
- unused
- gosimple
- gocritic
- gofmt
- bodyclose
- errname
- errorlint
+ - gosec
- noctx
- prealloc
- unconvert
- unparam📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| linters: | |
| enable: | |
| - errcheck | |
| - govet | |
| - ineffassign | |
| - staticcheck | |
| - unused | |
| - gosimple | |
| - gocritic | |
| - gofmt | |
| - bodyclose | |
| - errname | |
| - errorlint | |
| - noctx | |
| - prealloc | |
| - unconvert | |
| - unparam | |
| linters: | |
| enable: | |
| - errcheck | |
| - govet | |
| - ineffassign | |
| - staticcheck | |
| - unused | |
| - gosimple | |
| - gocritic | |
| - gofmt | |
| - bodyclose | |
| - errname | |
| - errorlint | |
| - gosec | |
| - noctx | |
| - prealloc | |
| - unconvert | |
| - unparam |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/.golangci.yml` around lines 6 - 22, Add the gosec security linter to the
enabled linters by updating the linters enable list (the "linters: enable:"
block) to include "gosec" so that static security checks run alongside
errcheck/govet/etc.; optionally, if desired, also add a gosec-specific
configuration block to tune rules and severity.
Comment on lines
+1
to
+3
| #!/usr/bin/env bash | ||
| # SynthOrg CLI installer for Linux and macOS. | ||
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\|\s*sh|set -euo pipefail' README.md cli/scripts/install.sh .github/ISSUE_TEMPLATE/installer-failure.yml
echo -n "/bin/sh resolves to: "
readlink -f /bin/sh || true
set +e
output=$(printf 'set -euo pipefail\n' | /bin/sh -s 2>&1)
status=$?
set -e
printf '%s\n' "$output"
if [ "$status" -eq 0 ]; then
echo "/bin/sh accepted pipefail on this runner."
else
echo "/bin/sh rejected pipefail (expected on dash/BusyBox-like shells)."
fiRepository: Aureliolo/synthorg
Length of output: 668
Change | sh to | bash for the curl installer invocations.
The script uses set -euo pipefail on line 9, which is not supported by POSIX shells like dash. This causes immediate failure when the documented curl ... | sh command runs on systems where /bin/sh is not Bash. Update the invocations in cli/scripts/install.sh (line 3), README.md (line 84), and .github/ISSUE_TEMPLATE/installer-failure.yml (lines 20 and 67) to pipe into bash instead.
Suggested change for install.sh
-# Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh
+# Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | bash📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #!/usr/bin/env bash | |
| # SynthOrg CLI installer for Linux and macOS. | |
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh | |
| #!/usr/bin/env bash | |
| # SynthOrg CLI installer for Linux and macOS. | |
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | bash |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/scripts/install.sh` around lines 1 - 3, The install script and docs
currently instruct users to pipe the curl installer into sh (`| sh`), which
fails because the script relies on Bash features; update the installer
invocation strings to use `| bash` instead—replace occurrences of `| sh` in the
install script (shebang/invocation line), the README installer example, and the
installer examples in the ISSUE_TEMPLATE to `| bash` so the script runs under
Bash as intended.
Comment on lines
+77
to
+83
| echo "Installing to ${INSTALL_DIR}/${BINARY_NAME}..." | ||
| if [ -w "$INSTALL_DIR" ]; then | ||
| mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}" | ||
| else | ||
| sudo mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}" | ||
| fi | ||
| chmod +x "${INSTALL_DIR}/${BINARY_NAME}" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '77,84p' cli/scripts/install.sh
if grep -nFq 'mkdir -p "$INSTALL_DIR"' cli/scripts/install.sh; then
echo 'INSTALL_DIR creation found.'
else
echo 'INSTALL_DIR is never created before the move.'
fi
grep -nF 'sudo mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"' cli/scripts/install.sh
grep -nF 'chmod +x "${INSTALL_DIR}/${BINARY_NAME}"' cli/scripts/install.shRepository: Aureliolo/synthorg
Length of output: 500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get file size
wc -l cli/scripts/install.sh
# View the script to understand INSTALL_DIR setup
head -100 cli/scripts/install.sh
# Check if INSTALL_DIR is created anywhere in the script
echo "=== Checking for mkdir operations ==="
grep -n "mkdir" cli/scripts/install.sh || echo "No mkdir found"
# Check INSTALL_DIR variable initialization
echo "=== INSTALL_DIR initialization ==="
grep -n "INSTALL_DIR" cli/scripts/install.sh | head -20Repository: Aureliolo/synthorg
Length of output: 3389
Fix missing directory creation and privilege mismatch in install.
The script fails when installing to a fresh custom INSTALL_DIR: the directory is never created, and mkdir -p is absent. Additionally, when using sudo mv (lines 81), the binary becomes root-owned, causing the subsequent non-privileged chmod +x on line 83 to fail. Combine directory creation, file move, and permission setting in a single privileged step to fix both issues.
Suggested change
echo "Installing to ${INSTALL_DIR}/${BINARY_NAME}..."
-if [ -w "$INSTALL_DIR" ]; then
- mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
+if mkdir -p "$INSTALL_DIR" 2>/dev/null && [ -w "$INSTALL_DIR" ]; then
+ install -m 0755 "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
else
- sudo mv "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
+ sudo mkdir -p "$INSTALL_DIR"
+ sudo install -m 0755 "${TMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/${BINARY_NAME}"
fi
-chmod +x "${INSTALL_DIR}/${BINARY_NAME}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/scripts/install.sh` around lines 77 - 83, The install block fails on
missing INSTALL_DIR creation and runs chmod as non-root after a sudo mv, so
create the target directory and perform move+permission changes under the same
privilege; update the block that uses INSTALL_DIR, BINARY_NAME, and TMP_DIR so
it runs mkdir -p "${INSTALL_DIR}" and then either uses a single privileged
command (e.g., sudo sh -c 'mv "${TMP_DIR}/${BINARY_NAME}"
"${INSTALL_DIR}/${BINARY_NAME}" && chmod +x "${INSTALL_DIR}/${BINARY_NAME}"') or
uses an atomic helper like install to move and set executable bit in one step so
ownership and chmod happen together.
Comment on lines
+81
to
+85
| ```bash | ||
| brew install aureliolo/tap/synthorg # macOS | ||
| scoop bucket add aureliolo https://github.com/Aureliolo/scoop-bucket && scoop install synthorg # Windows | ||
| curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh # Linux | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Windows PowerShell 5.1 support the && operator, or was it introduced in PowerShell 7?
💡 Result:
Windows PowerShell 5.1 does not support the && operator.
&& (and ||) are pipeline chain operators that were introduced in PowerShell 7 (available starting with PowerShell 7 previews and documented as “Beginning in PowerShell 7…”). [1], [2]
Split the Scoop example into separate commands.
Line 83 uses &&, which is only available in PowerShell 7+. Many Scoop users still run Windows PowerShell 5.1, causing this quick-start line to fail before scoop install executes.
💡 Suggested doc change
-scoop bucket add aureliolo https://github.com/Aureliolo/scoop-bucket && scoop install synthorg # Windows
+scoop bucket add aureliolo https://github.com/Aureliolo/scoop-bucket
+scoop install synthorg # Windows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 81 - 85, The Scoop example uses a single line with
"scoop bucket add aureliolo https://github.com/Aureliolo/scoop-bucket && scoop
install synthorg", which fails on Windows PowerShell 5.1 because && isn't
supported; split this into two separate commands so users run the bucket-add
first ("scoop bucket add aureliolo https://github.com/Aureliolo/scoop-bucket")
and then run the install ("scoop install synthorg"), updating the README entry
accordingly.
Comment on lines
+4
to
+13
| push: | ||
| branches: [main] | ||
| paths: | ||
| - "cli/**" | ||
| tags: | ||
| - "v*" | ||
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - "cli/**" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the CLI workflow file
find . -name "cli.yml" -path "*/.github/workflows/*"Repository: Aureliolo/synthorg
Length of output: 89
🏁 Script executed:
# Read the CLI workflow file to see the full structure
cat -n .github/workflows/cli.ymlRepository: Aureliolo/synthorg
Length of output: 7785
🏁 Script executed:
# Also check a few other workflows to see how they handle path filters and workflow file inclusion
cat -n .github/workflows/pages.yml | head -30Repository: Aureliolo/synthorg
Length of output: 772
🏁 Script executed:
# Check docker.yml for comparison
cat -n .github/workflows/docker.yml | head -30Repository: Aureliolo/synthorg
Length of output: 829
🏁 Script executed:
# Check ci.yml for comparison
cat -n .github/workflows/ci.yml | head -30Repository: Aureliolo/synthorg
Length of output: 1019
Include workflow file in path triggers to test release plumbing changes.
The outer paths: ["cli/**"] prevents this workflow from running when .github/workflows/cli.yml itself changes. Other workflows in the repository (pages.yml, docker.yml) include their own workflow file path to ensure immediate testing of release plumbing updates. Add .github/workflows/cli.yml to both push and pull_request paths so that modifications to the workflow trigger a test run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli.yml around lines 4 - 13, The workflow's push and
pull_request path filters only include "cli/**", which prevents changes to the
workflow file itself from triggering runs; update both the push and pull_request
blocks (the paths arrays under push and pull_request) to also include
".github/workflows/cli.yml" so edits to the workflow file will trigger the
workflow immediately.
Comment on lines
+148
to
+149
| - name: Install govulncheck | ||
| run: go install golang.org/x/vuln/cmd/govulncheck@latest |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin govulncheck to a specific version.
go install ...@latest makes CI non-hermetic and can break this workflow on upstream releases unrelated to the repo. Pin the tool version the same way the action SHAs and GoReleaser version are pinned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli.yml around lines 148 - 149, Replace the non-hermetic
installation "go install golang.org/x/vuln/cmd/govulncheck@latest" in the
"Install govulncheck" step with a pinned release (e.g., "@vX.Y.Z" or a
repo-level variable) so the workflow installs a specific govulncheck version;
update the run line to use that pinned version the same way other
actions/GoReleaser are pinned.
Comment on lines
+156
to
+163
| cli-release: | ||
| name: CLI Release | ||
| if: startsWith(github.ref, 'refs/tags/v') | ||
| needs: [cli-lint, cli-test, cli-build, cli-vuln] | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| steps: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "cli.yml" --type fRepository: Aureliolo/synthorg
Length of output: 44
🏁 Script executed:
cat -n .github/workflows/cli.yml | head -220Repository: Aureliolo/synthorg
Length of output: 7785
🏁 Script executed:
grep -n "environment:" .github/workflows/cli.ymlRepository: Aureliolo/synthorg
Length of output: 44
🏁 Script executed:
grep -rn "environment:" .github/ --include="*.yml" --include="*.yaml"Repository: Aureliolo/synthorg
Length of output: 367
Add environment protection to the CLI release job.
The GORELEASER_TAP_TOKEN is injected without job-level environment protection. This token is used to publish to external repos (homebrew-tap, scoop-bucket) and should be gated by a release environment, consistent with other sensitive release workflows in the repo (see release.yml:13).
Suggested fix
cli-release:
name: CLI Release
+ environment: release
if: startsWith(github.ref, 'refs/tags/v')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cli-release: | |
| name: CLI Release | |
| if: startsWith(github.ref, 'refs/tags/v') | |
| needs: [cli-lint, cli-test, cli-build, cli-vuln] | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write | |
| steps: | |
| cli-release: | |
| name: CLI Release | |
| environment: release | |
| if: startsWith(github.ref, 'refs/tags/v') | |
| needs: [cli-lint, cli-test, cli-build, cli-vuln] | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write | |
| steps: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cli.yml around lines 156 - 163, Add job-level environment
protection to the cli-release job so the GORELEASER_TAP_TOKEN (and any other
release secrets) are gated by the protected release environment; update the
cli-release job block (job name: cli-release) to include an environment: release
entry (matching the repo's protected environment used elsewhere) and ensure the
GORELEASER_TAP_TOKEN is provided via that environment's secrets rather than
being injected at workflow scope.
Comment on lines
+43
to
+63
| func Load(dataDir string) (State, error) { | ||
| path := StatePath(dataDir) | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return DefaultState(), nil | ||
| } | ||
| return State{}, err | ||
| } | ||
| var s State | ||
| if err := json.Unmarshal(data, &s); err != nil { | ||
| return State{}, err | ||
| } | ||
| // Canonicalize and validate DataDir. | ||
| if s.DataDir != "" { | ||
| s.DataDir = filepath.Clean(s.DataDir) | ||
| if !filepath.IsAbs(s.DataDir) { | ||
| return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", s.DataDir) | ||
| } | ||
| } | ||
| return s, nil |
Contributor
There was a problem hiding this comment.
Preserve the requested dataDir when config.json is missing.
Load(dataDir) falls back to DefaultState(), which resets State.DataDir back to the OS default. First-run flows that passed a custom data directory will silently switch to a different location instead of honoring the caller input.
💡 Minimal fix
func Load(dataDir string) (State, error) {
+ dataDir = filepath.Clean(dataDir)
+ if dataDir != "" && !filepath.IsAbs(dataDir) {
+ return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", dataDir)
+ }
+
path := StatePath(dataDir)
data, err := os.ReadFile(path)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
- return DefaultState(), nil
+ s := DefaultState()
+ if dataDir != "" {
+ s.DataDir = dataDir
+ }
+ return s, nil
}
return State{}, err
}
var s State
if err := json.Unmarshal(data, &s); err != nil {
return State{}, err
}
+ if s.DataDir == "" {
+ s.DataDir = dataDir
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func Load(dataDir string) (State, error) { | |
| path := StatePath(dataDir) | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return DefaultState(), nil | |
| } | |
| return State{}, err | |
| } | |
| var s State | |
| if err := json.Unmarshal(data, &s); err != nil { | |
| return State{}, err | |
| } | |
| // Canonicalize and validate DataDir. | |
| if s.DataDir != "" { | |
| s.DataDir = filepath.Clean(s.DataDir) | |
| if !filepath.IsAbs(s.DataDir) { | |
| return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", s.DataDir) | |
| } | |
| } | |
| return s, nil | |
| func Load(dataDir string) (State, error) { | |
| dataDir = filepath.Clean(dataDir) | |
| if dataDir != "" && !filepath.IsAbs(dataDir) { | |
| return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", dataDir) | |
| } | |
| path := StatePath(dataDir) | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| s := DefaultState() | |
| if dataDir != "" { | |
| s.DataDir = dataDir | |
| } | |
| return s, nil | |
| } | |
| return State{}, err | |
| } | |
| var s State | |
| if err := json.Unmarshal(data, &s); err != nil { | |
| return State{}, err | |
| } | |
| // Canonicalize and validate DataDir. | |
| if s.DataDir != "" { | |
| s.DataDir = filepath.Clean(s.DataDir) | |
| if !filepath.IsAbs(s.DataDir) { | |
| return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", s.DataDir) | |
| } | |
| } | |
| if s.DataDir == "" { | |
| s.DataDir = dataDir | |
| } | |
| return s, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/config/state.go` around lines 43 - 63, Load currently returns
DefaultState() when the config file is missing, which discards the
caller-provided dataDir; change the os.ErrNotExist branch so it constructs the
default state but preserves and canonicalizes the requested dataDir: create s :=
DefaultState(), and if the incoming dataDir is non-empty set s.DataDir =
filepath.Clean(dataDir) and validate it's absolute (same check as later), then
return s, nil; keep the existing canonicalization/validation logic for loaded
states (referencing Load, DefaultState, State.DataDir, and StatePath).
Comment on lines
+67
to
+75
| func Save(s State) error { | ||
| if err := EnsureDir(s.DataDir); err != nil { | ||
| return err | ||
| } | ||
| data, err := json.MarshalIndent(s, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(StatePath(s.DataDir), data, 0o600) |
Contributor
There was a problem hiding this comment.
Validate State.DataDir before writing.
Save accepts "" and relative paths, but Load later rejects them. That can write config.json into the current working directory or persist a state that cannot be loaded back; cli/internal/config/paths.go:16-22 can also produce "." on home-dir lookup failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/config/state.go` around lines 67 - 75, Save currently allows
empty or relative DataDir values and can write config.json to the wrong place;
validate State.DataDir at the start of Save and return an error if it's empty,
"." or a relative path. Specifically, in the Save function (and before calling
EnsureDir/StatePath), check s.DataDir != "" and that filepath.IsAbs(s.DataDir)
is true and s.DataDir != "." (mirroring Load's validation and
cli/internal/config/paths.go behavior), and return a clear error if the check
fails so callers cannot persist an invalid state directory.
Comment on lines
+22
to
+34
| 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() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/internal/health/check.go | head -60Repository: Aureliolo/synthorg
Length of output: 1900
🏁 Script executed:
# Check if there are callers of WaitForHealthy and how parameters are passed
rg -A 5 -B 5 "WaitForHealthy" cli/internal/health/Repository: Aureliolo/synthorg
Length of output: 8477
🏁 Script executed:
# Look for command-line flag definitions that configure these timeout/interval values
rg -r "interval|timeout|initialDelay" cli/ -t go | head -30Repository: Aureliolo/synthorg
Length of output: 3072
🌐 Web query:
Go time.NewTicker panic behavior non-positive duration
💡 Result:
In Go, time.NewTicker(d) panics immediately if d <= 0 (zero or negative duration). The panic value/message is:
non-positive interval for NewTicker(often shown aspanic: non-positive interval for NewTicker). [1][2]
Related: (*time.Ticker).Reset(d) also panics if d <= 0 (non-positive interval for Ticker.Reset). [2]
Sources: [1] [2]
🏁 Script executed:
rg -B 10 -A 5 "WaitForHealthy" cli/cmd/start.goRepository: Aureliolo/synthorg
Length of output: 916
🌐 Web query:
Go context.WithTimeout non-positive duration behavior
💡 Result:
context.WithTimeout(parent, timeout) is just WithDeadline(parent, time.Now().Add(timeout)).[1][2]
So if timeout <= 0, the computed deadline is now or in the past, and WithDeadline will cancel the returned context immediately (its Done() channel is already closed / closes immediately) and ctx.Err() will be context.DeadlineExceeded.[2]
It still returns a cancel func; calling it is harmless and still recommended by the docs to release resources (even though in the timeout <= 0 case there’s no timer to stop because cancellation happens immediately).[1][2]
Sources: [1] pkg.go.dev/context docs, [2] Go context package implementation (WithTimeout + WithDeadline dur <= 0 path).
Add parameter validation to prevent runtime panics from non-positive durations.
time.NewTicker(interval) panics if interval <= 0, and time.After(initialDelay) panics if initialDelay < 0. Additionally, context.WithTimeout(ctx, timeout) with timeout <= 0 causes immediate cancellation. These parameters are function-level inputs without validation; even though current hardcoded usage is safe, the API accepts arbitrary caller-provided values that could trigger crashes.
Suggested fix
func WaitForHealthy(ctx context.Context, url string, timeout, interval, initialDelay time.Duration) error {
+ if timeout <= 0 {
+ return fmt.Errorf("timeout must be > 0")
+ }
+ if interval <= 0 {
+ return fmt.Errorf("interval must be > 0")
+ }
+ if initialDelay < 0 {
+ initialDelay = 0
+ }
+
ctx, cancel := context.WithTimeout(ctx, timeout)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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() | |
| func WaitForHealthy(ctx context.Context, url string, timeout, interval, initialDelay time.Duration) error { | |
| if timeout <= 0 { | |
| return fmt.Errorf("timeout must be > 0") | |
| } | |
| if interval <= 0 { | |
| return fmt.Errorf("interval must be > 0") | |
| } | |
| if initialDelay < 0 { | |
| initialDelay = 0 | |
| } | |
| 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() |
🤖 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 - 34, The WaitForHealthy
function must validate its duration parameters to avoid panics: at the top of
WaitForHealthy check timeout, interval and initialDelay and return a descriptive
error if timeout <= 0, if interval <= 0, or if initialDelay < 0; do not call
context.WithTimeout, time.After or time.NewTicker until the parameters are
validated. Reference the function name WaitForHealthy and the parameter names
(ctx, url, timeout, interval, initialDelay) in the error messages so callers can
see which argument is invalid.
Comment on lines
+33
to
+49
| 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 { | ||
| lastErr = err | ||
| continue | ||
| } | ||
| return nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/internal/health/check.goRepository: Aureliolo/synthorg
Length of output: 2641
Probe once immediately after the startup delay.
The first HTTP request is delayed until the first ticker event. This means timeout <= initialDelay + interval can expire without ever touching the endpoint. Healthy services also pay one unnecessary interval of latency.
Suggested fix
- ticker := time.NewTicker(interval)
- defer ticker.Stop()
-
var lastErr error
+ if err := checkOnce(ctx, url); err != nil {
+ lastErr = err
+ } else {
+ return nil
+ }
+
+ ticker := time.NewTicker(interval)
+ defer ticker.Stop()
+
for {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 { | |
| lastErr = err | |
| continue | |
| } | |
| return nil | |
| var lastErr error | |
| if err := checkOnce(ctx, url); err != nil { | |
| lastErr = err | |
| } else { | |
| return nil | |
| } | |
| ticker := time.NewTicker(interval) | |
| defer ticker.Stop() | |
| 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 { | |
| lastErr = err | |
| continue | |
| } | |
| return nil |
🤖 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 33 - 49, The loop currently waits
for the first ticker event before calling checkOnce, causing a missed or delayed
probe; after the startup delay, perform an immediate probe by calling
checkOnce(ctx, url) once before creating/entering the ticker loop (update
handling of lastErr and early return on success), then proceed to the existing
ticker-based retries using ticker, interval, lastErr, ctx and url; ensure
behavior and error wrapping on timeout remain unchanged.
Comment on lines
+39
to
+43
| case <-ctx.Done(): | ||
| if lastErr != nil { | ||
| return fmt.Errorf("health check timed out (last error: %w)", lastErr) | ||
| } | ||
| return fmt.Errorf("health check timed out") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/internal/health/check.goRepository: Aureliolo/synthorg
Length of output: 2641
🌐 Web query:
Go context.WithTimeout behavior context.Canceled vs context.DeadlineExceeded
💡 Result:
context.WithTimeout(parent, d) creates a child context that is Done when any of these happens first: (1) its timeout/deadline expires, (2) you call the returned cancel(), or (3) the parent context is canceled. It’s equivalent to WithDeadline(parent, time.Now().Add(d)). [1]
Which error you get (ctx.Err())
context.DeadlineExceeded: the context was canceled because its deadline/timeout expired. [2]context.Canceled: the context was canceled for any reason other than deadline expiry—most commonly:- you (or a library) called the returned
cancel()early, or - the parent context was canceled first. [2]
- you (or a library) called the returned
So even with WithTimeout, you can legitimately see either error: it depends on what canceled the context first (explicit/parent cancel vs. the timeout firing). [1] [2]
Typical check
Use errors.Is:
errors.Is(err, context.DeadlineExceeded)→ timed outerrors.Is(err, context.Canceled)→ canceled (explicitly or by parent) [2]
Why defer cancel() is still correct
Calling cancel() releases resources (notably timers) associated with the timeout context, and should be done even if you expect the timeout to fire. [1] [3]
Sources: [1] Go context docs (WithDeadline/WithTimeout behavior) https://pkg.go.dev/context@go1.25.2 • [2] Go context docs (Canceled vs DeadlineExceeded) https://pkg.go.dev/context@go1.25.2 • [3] Go docs on canceling operations and deferring cancel() https://go.dev/doc/database/cancel-operations
Keep cancellation distinct from timeout.
The ctx.Done() branch always returns "health check timed out", even when the parent context was canceled. That makes Ctrl-C and upstream aborts indistinguishable from a real deadline. Note that the initial delay block (line 30) correctly handles this by returning ctx.Err() directly, but the main loop does not.
💡 Minimal fix
case <-ctx.Done():
+ if ctx.Err() == context.Canceled {
+ return ctx.Err()
+ }
if lastErr != nil {
return fmt.Errorf("health check timed out (last error: %w)", lastErr)
}🤖 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 39 - 43, The ctx.Done() branch in
the health check loop incorrectly maps all cancellations to a timeout message;
modify the branch in the function containing the loop so that if ctx.Err() is
non-nil you return fmt.Errorf("health check canceled: %w", ctx.Err()) (or simply
return ctx.Err()) when the context was canceled, otherwise preserve the existing
behavior that returns the wrapped lastErr for timeouts (e.g., if lastErr != nil
return fmt.Errorf("health check timed out (last error: %w)", lastErr) else
return fmt.Errorf("health check timed out")). Ensure you check
ctx.Err()/context.Canceled vs deadline exceeded semantics so Ctrl-C/upstream
cancels are distinguishable from real timeouts.
…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
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Comment on lines
+155
to
+159
| # ── Release (GoReleaser, only on version tags) ── | ||
| cli-release: | ||
| name: CLI Release | ||
| if: startsWith(github.ref, 'refs/tags/v') | ||
| needs: [cli-lint, cli-test, cli-build, cli-vuln] |
There was a problem hiding this comment.
ci-pass gate in ci.yml omits all CLI jobs
The cli-release job (and all other cli-* jobs) live in cli.yml, but the merge gate defined in ci.yml's ci-pass job only waits on Python/dashboard/Docker jobs:
needs: [changes, lint, type-check, test, python-audit, dockerfile-lint,
dashboard-lint, dashboard-type-check, dashboard-test, dashboard-build, dashboard-audit]None of the cli-lint, cli-test, cli-build, or cli-vuln jobs are referenced. If branch protection is configured to require only the CI Pass status check, a PR that breaks CLI compilation or tests can be merged without failing the gate. The two workflows would need to be consolidated, or the branch-protection rules must explicitly require the CLI workflow's jobs as additional required checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/cli.yml
Line: 155-159
Comment:
**`ci-pass` gate in `ci.yml` omits all CLI jobs**
The `cli-release` job (and all other `cli-*` jobs) live in `cli.yml`, but the merge gate defined in `ci.yml`'s `ci-pass` job only waits on Python/dashboard/Docker jobs:
```yaml
needs: [changes, lint, type-check, test, python-audit, dockerfile-lint,
dashboard-lint, dashboard-type-check, dashboard-test, dashboard-build, dashboard-audit]
```
None of the `cli-lint`, `cli-test`, `cli-build`, or `cli-vuln` jobs are referenced. If branch protection is configured to require only the `CI Pass` status check, a PR that breaks CLI compilation or tests can be merged without failing the gate. The two workflows would need to be consolidated, or the branch-protection rules must explicitly require the `CLI` workflow's jobs as additional required checks.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+235
to
+242
| # ── Commit and push pinned scripts to main ── | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" | ||
| git checkout main | ||
| git add cli/scripts/install.sh cli/scripts/install.ps1 | ||
| git commit -m "chore: pin install scripts to ${TAG} checksums [skip ci]" || echo "No changes to commit" | ||
| git push origin main |
There was a problem hiding this comment.
Direct push to main in release workflow bypasses branch protection
After pinning install-script checksums, the job directly pushes to main:
git checkout main
git add cli/scripts/install.sh cli/scripts/install.ps1
git commit -m "chore: pin install scripts to ${TAG} checksums [skip ci]" || echo "No changes to commit"
git push origin mainIf the repository has branch-protection rules that require PR reviews or status checks, this push will either fail silently (the || echo "No changes to commit" masks git errors but not push errors) or bypass review entirely. Additionally, git checkout main after a tag-triggered run may land on an outdated main if new commits arrived between the tag and the release job's execution, potentially creating a commit with a stale base.
Consider pushing to a dedicated branch and auto-merging, or switching to a separate workflow triggered after the release is published that targets the correct commit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/cli.yml
Line: 235-242
Comment:
**Direct push to `main` in release workflow bypasses branch protection**
After pinning install-script checksums, the job directly pushes to `main`:
```bash
git checkout main
git add cli/scripts/install.sh cli/scripts/install.ps1
git commit -m "chore: pin install scripts to ${TAG} checksums [skip ci]" || echo "No changes to commit"
git push origin main
```
If the repository has branch-protection rules that require PR reviews or status checks, this push will either fail silently (the `|| echo "No changes to commit"` masks git errors but not push errors) or bypass review entirely. Additionally, `git checkout main` after a tag-triggered run may land on an outdated `main` if new commits arrived between the tag and the release job's execution, potentially creating a commit with a stale base.
Consider pushing to a dedicated branch and auto-merging, or switching to a separate workflow triggered after the release is published that targets the correct commit.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+106
to
+113
| restart, err := confirmRestart() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !restart { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Interactive TUI blocks in non-TTY environments
confirmRestart() unconditionally calls huh.NewForm(...).Run() when running containers are detected. In non-interactive environments (cron jobs, SSH sessions without a TTY, piped scripts, or CI), huh will either block waiting for input or return an error that propagates out of updateContainerImages as a hard failure, aborting the whole update — even though the image pull succeeded.
Consider checking whether stdin is a terminal before prompting, and either auto-accepting a safe default or exposing a --yes / --non-interactive flag:
// Example guard:
if !isatty.IsTerminal(os.Stdin.Fd()) {
fmt.Fprintln(out, "Non-interactive mode: skipping restart. Run 'synthorg stop && synthorg start' to apply new images.")
return nil
}The same consideration applies to the stopAndRemoveVolumes and confirmAndRemoveBinary prompts in uninstall.go.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/update.go
Line: 106-113
Comment:
**Interactive TUI blocks in non-TTY environments**
`confirmRestart()` unconditionally calls `huh.NewForm(...).Run()` when running containers are detected. In non-interactive environments (cron jobs, SSH sessions without a TTY, piped scripts, or CI), `huh` will either block waiting for input or return an error that propagates out of `updateContainerImages` as a hard failure, aborting the whole update — even though the image pull succeeded.
Consider checking whether stdin is a terminal before prompting, and either auto-accepting a safe default or exposing a `--yes` / `--non-interactive` flag:
```go
// Example guard:
if !isatty.IsTerminal(os.Stdin.Fd()) {
fmt.Fprintln(out, "Non-interactive mode: skipping restart. Run 'synthorg stop && synthorg start' to apply new images.")
return nil
}
```
The same consideration applies to the `stopAndRemoveVolumes` and `confirmAndRemoveBinary` prompts in `uninstall.go`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+128
to
+145
| func findAssets(release Release) (assetURL, checksumURL string, err error) { | ||
| archiveName := assetName() | ||
| for _, a := range release.Assets { | ||
| if a.Name == archiveName { | ||
| if !strings.HasPrefix(a.BrowserDownloadURL, expectedURLPrefix) { | ||
| return "", "", fmt.Errorf("asset URL %q does not match expected prefix", a.BrowserDownloadURL) | ||
| } | ||
| assetURL = a.BrowserDownloadURL | ||
| } | ||
| if a.Name == "checksums.txt" { | ||
| if !strings.HasPrefix(a.BrowserDownloadURL, expectedURLPrefix) { | ||
| return "", "", fmt.Errorf("checksum URL %q does not match expected prefix", a.BrowserDownloadURL) | ||
| } | ||
| checksumURL = a.BrowserDownloadURL | ||
| } | ||
| } | ||
| return assetURL, checksumURL, nil | ||
| } |
There was a problem hiding this comment.
findAssets returns empty strings without error on a platform miss
When neither the matching archive nor checksums.txt is found in the release, findAssets returns "", "", nil. The caller CheckFromURL then stores empty strings in CheckResult, and runUpdate gracefully handles an empty AssetURL by printing "No binary available for your platform." However, a missing checksumURL alone is silently propagated; Download then returns an error ("no checksum file found"), which is surfaced as a hard failure.
The silent success return makes the function harder to reason about and the behaviour differs across the two "not found" cases. Consider returning a distinct error when the primary asset is missing:
if assetURL == "" {
return "", "", fmt.Errorf("no release asset found for platform %s_%s", runtime.GOOS, runtime.GOARCH)
}
if checksumURL == "" {
return "", "", fmt.Errorf("no checksums.txt found in release assets")
}This would give callers a consistent, descriptive failure instead of requiring them to special-case empty strings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/internal/selfupdate/updater.go
Line: 128-145
Comment:
**`findAssets` returns empty strings without error on a platform miss**
When neither the matching archive nor `checksums.txt` is found in the release, `findAssets` returns `"", "", nil`. The caller `CheckFromURL` then stores empty strings in `CheckResult`, and `runUpdate` gracefully handles an empty `AssetURL` by printing "No binary available for your platform." However, a missing `checksumURL` alone is silently propagated; `Download` then returns an error ("no checksum file found"), which is surfaced as a hard failure.
The silent success return makes the function harder to reason about and the behaviour differs across the two "not found" cases. Consider returning a distinct error when the primary asset is missing:
```go
if assetURL == "" {
return "", "", fmt.Errorf("no release asset found for platform %s_%s", runtime.GOOS, runtime.GOARCH)
}
if checksumURL == "" {
return "", "", fmt.Errorf("no checksums.txt found in release assets")
}
```
This would give callers a consistent, descriptive failure instead of requiring them to special-case empty strings.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+101
to
+112
| func yamlStr(s string) string { | ||
| // If the string contains YAML-special characters, double-quote and escape. | ||
| if strings.ContainsAny(s, ":#{}[]|>&*!%@`\"'\\\n\r\t") { | ||
| escaped := strings.ReplaceAll(s, `\`, `\\`) | ||
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | ||
| escaped = strings.ReplaceAll(escaped, "\n", `\n`) | ||
| escaped = strings.ReplaceAll(escaped, "\r", `\r`) | ||
| escaped = strings.ReplaceAll(escaped, "\t", `\t`) | ||
| return `"` + escaped + `"` | ||
| } | ||
| return `"` + s + `"` | ||
| } |
There was a problem hiding this comment.
yamlStr does not escape $ — Docker Compose variable interpolation can silently mangle values
yamlStr never escapes $ characters inside double-quoted YAML strings. Docker Compose performs variable substitution on compose file values before YAML parsing, so a JWT secret or log-level value containing $VAR would be silently replaced at docker compose up time, potentially resulting in an empty or wrong value without any error.
The current code is safe in practice because:
JWTSecretis generated viabase64.URLEncoding(alphabet:[A-Za-z0-9\-_=], no$)LogLevelcomes from an allow-list (debug,info,warn,error)
But the function offers no general safety guarantee. If the compose template is ever extended with a user-supplied string that wasn't generated by this code (e.g., an external JWT_SECRET env var), the absence of $ escaping becomes a real bug. Adding $$ escaping inside the double-quote branch would make the function defensively correct:
escaped = strings.ReplaceAll(escaped, `$`, `$$`)Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/internal/compose/generate.go
Line: 101-112
Comment:
**`yamlStr` does not escape `$` — Docker Compose variable interpolation can silently mangle values**
`yamlStr` never escapes `$` characters inside double-quoted YAML strings. Docker Compose performs variable substitution on compose file values before YAML parsing, so a JWT secret or log-level value containing `$VAR` would be silently replaced at `docker compose up` time, potentially resulting in an empty or wrong value without any error.
The current code is safe in practice because:
- `JWTSecret` is generated via `base64.URLEncoding` (alphabet: `[A-Za-z0-9\-_=]`, no `$`)
- `LogLevel` comes from an allow-list (`debug`, `info`, `warn`, `error`)
But the function offers no general safety guarantee. If the compose template is ever extended with a user-supplied string that wasn't generated by this code (e.g., an external `JWT_SECRET` env var), the absence of `$` escaping becomes a real bug. Adding `$$` escaping inside the double-quote branch would make the function defensively correct:
```go
escaped = strings.ReplaceAll(escaped, `$`, `$$`)
```
How can I resolve this? If you propose a fix, please make it concise.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.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Comment on lines
+3
to
+10
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh | ||
| # | ||
| # Environment variables: | ||
| # SYNTHORG_VERSION — specific version to install (overrides pinned version, | ||
| # falls back to runtime checksum download) | ||
| # INSTALL_DIR — installation directory (default: /usr/local/bin) | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
| sh usage will fail on non-bash shells (e.g., Ubuntu's dash)
The documented install invocation pipes through sh, but the script relies on two bash-specific features that are not available in POSIX sh (e.g., dash, the default /bin/sh on Ubuntu and Debian):
- Line 10:
set -euo pipefail— thepipefailoption is not a POSIXsetoption and causesdashto error immediately withIllegal option -o pipefail, aborting the install before any useful work is done. - Line 78:
EXPECTED_CHECKSUM="${!CHECKSUM_VAR:-}"— the${!var}indirect expansion syntax is a bash extension; dash will treat this as a literal string reference and produce the wrong (empty) checksum, triggering the "No checksum found" error on line 91.
On any Debian-family system where /bin/sh -> dash, running the documented curl ... | sh command silently fails with a cryptic error rather than installing the binary.
The simplest fix is to update the usage comment (and the README/docs install commands) to use bash explicitly:
Suggested change
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | sh | |
| # | |
| # Environment variables: | |
| # SYNTHORG_VERSION — specific version to install (overrides pinned version, | |
| # falls back to runtime checksum download) | |
| # INSTALL_DIR — installation directory (default: /usr/local/bin) | |
| set -euo pipefail | |
| # Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | bash | |
| # |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/scripts/install.sh
Line: 3-10
Comment:
**`| sh` usage will fail on non-bash shells (e.g., Ubuntu's dash)**
The documented install invocation pipes through `sh`, but the script relies on two bash-specific features that are not available in POSIX `sh` (e.g., `dash`, the default `/bin/sh` on Ubuntu and Debian):
1. **Line 10: `set -euo pipefail`** — the `pipefail` option is not a POSIX `set` option and causes `dash` to error immediately with `Illegal option -o pipefail`, aborting the install before any useful work is done.
2. **Line 78: `EXPECTED_CHECKSUM="${!CHECKSUM_VAR:-}"`** — the `${!var}` indirect expansion syntax is a bash extension; dash will treat this as a literal string reference and produce the wrong (empty) checksum, triggering the "No checksum found" error on line 91.
On any Debian-family system where `/bin/sh -> dash`, running the documented `curl ... | sh` command silently fails with a cryptic error rather than installing the binary.
The simplest fix is to update the usage comment (and the README/docs install commands) to use `bash` explicitly:
```suggestion
# Usage: curl -sSfL https://raw.githubusercontent.com/Aureliolo/synthorg/main/cli/scripts/install.sh | bash
#
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+86
to
+93
| statsOut, err := docker.RunCmd(ctx, "docker", "stats", "--no-stream", "--format", | ||
| "table {{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}") | ||
| if err != nil { | ||
| fmt.Fprintf(out, "Could not get resource usage: %v\n", err) | ||
| return | ||
| } | ||
| fmt.Fprintln(out, "Resource usage:") | ||
| fmt.Fprintln(out, statsOut) |
There was a problem hiding this comment.
docker stats reports all host containers, not just the synthorg stack
docker stats --no-stream returns CPU/memory data for every running container on the Docker host, not just the ones belonging to the synthorg compose project. The function already has the compose project's container IDs available (from the ps -q call on line 81), but doesn't use them to filter the stats output.
On a shared host or a developer machine with many containers running, this will flood the synthorg status output with resource rows for unrelated services and exposes their names and metrics.
Pass the specific container IDs from the ps -q output to docker stats to scope the output correctly:
ids := strings.Fields(strings.TrimSpace(psOut))
statsArgs := append([]string{"stats", "--no-stream", "--format",
"table {{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}"}, ids...)
statsOut, err := docker.RunCmd(ctx, "docker", statsArgs...)Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/status.go
Line: 86-93
Comment:
**`docker stats` reports all host containers, not just the synthorg stack**
`docker stats --no-stream` returns CPU/memory data for every running container on the Docker host, not just the ones belonging to the synthorg compose project. The function already has the compose project's container IDs available (from the `ps -q` call on line 81), but doesn't use them to filter the stats output.
On a shared host or a developer machine with many containers running, this will flood the `synthorg status` output with resource rows for unrelated services and exposes their names and metrics.
Pass the specific container IDs from the `ps -q` output to `docker stats` to scope the output correctly:
```go
ids := strings.Fields(strings.TrimSpace(psOut))
statsArgs := append([]string{"stats", "--no-stream", "--format",
"table {{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}"}, ids...)
statsOut, err := docker.RunCmd(ctx, "docker", statsArgs...)
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new cross-platform Go-based synthorg CLI to manage the Docker Compose lifecycle (init/start/stop/status/logs/doctor/update/uninstall/version), plus supporting release tooling and documentation updates to make the CLI the recommended install/run path.
Changes:
- Introduces a standalone Go module under
cli/(commands + internal packages for compose generation, health polling, diagnostics, and self-update). - Adds distribution/release automation (install scripts, GoReleaser config, dedicated GitHub Actions workflow, dependabot gomod, pre-commit hooks).
- Updates docs/README and aligns backend healthcheck status to
"ok".
Reviewed changes
Copilot reviewed 50 out of 52 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/user_guide.md | Adds CLI-focused quick start and retains manual compose path. |
| docs/roadmap/index.md | Removes “CLI may not be needed” roadmap item. |
| docs/getting_started.md | Updates project layout docs (currently mispoints CLI location). |
| docs/design/operations.md | Updates operations doc to reflect implemented Go CLI. |
| docs/architecture/tech-stack.md | Updates tech stack to list Go CLI and its command set. |
| docker/backend/Dockerfile | Healthcheck status updated from healthy to ok. |
| cli/testdata/compose_default.yml | Golden output for default compose generation. |
| cli/testdata/compose_custom_ports.yml | Golden output for custom ports/secret compose generation. |
| cli/scripts/install.sh | Linux/macOS installer with checksum verification and pinned section. |
| cli/scripts/install.ps1 | Windows installer with checksum verification and pinned section. |
| cli/main.go | CLI entrypoint wiring to Cobra command root. |
| cli/internal/version/version.go | Build-time version variables for ldflags injection. |
| cli/internal/selfupdate/updater_test.go | Tests for release checking, downloads, extraction, checksum verification, replacement. |
| cli/internal/selfupdate/updater.go | Self-update logic (GitHub Releases, URL validation, checksum verify, atomic replace). |
| cli/internal/health/check_test.go | Tests for health polling behavior (timeouts, delays, eventual success). |
| cli/internal/health/check.go | Health polling implementation with timeout/interval/initial delay. |
| cli/internal/docker/client_test.go | Tests for docker/compose helpers and version comparisons. |
| cli/internal/docker/client.go | Docker/Compose detection and command execution helpers. |
| cli/internal/diagnostics/collect_test.go | Tests for diagnostics truncation and report formatting. |
| cli/internal/diagnostics/collect.go | Diagnostics collection (docker info, logs, health endpoint, disk info). |
| cli/internal/config/state_test.go | Tests for config persistence and permissions. |
| cli/internal/config/state.go | Persisted CLI state load/save logic. |
| cli/internal/config/paths_test.go | Tests for platform-specific data dir resolution. |
| cli/internal/config/paths.go | Data directory resolution and directory creation helper. |
| cli/internal/compose/generate_test.go | Compose generation tests + golden comparisons. |
| cli/internal/compose/generate.go | Embedded template rendering + parameter validation/YAML quoting. |
| cli/internal/compose/compose.yml.tmpl | Compose template with CIS hardening and optional sandbox service. |
| cli/go.sum | Go dependency lockfile for the CLI module. |
| cli/go.mod | Go module definition and dependencies for the CLI module. |
| cli/cmd/version.go | synthorg version command. |
| cli/cmd/update.go | synthorg update (self-update + image pull + optional restart/health check). |
| cli/cmd/uninstall.go | synthorg uninstall (down/remove data/remove binary with confirmations). |
| cli/cmd/stop.go | synthorg stop (compose down). |
| cli/cmd/status.go | synthorg status (versions, container ps, stats, health endpoint). |
| cli/cmd/start.go | synthorg start (pull/up + health polling). |
| cli/cmd/root.go | Root Cobra command and --data-dir handling. |
| cli/cmd/logs.go | synthorg logs with argument/flag validation. |
| cli/cmd/init.go | Interactive init wizard + compose/config file generation. |
| cli/cmd/doctor.go | synthorg doctor diagnostic file + prefilled issue URL. |
| cli/.goreleaser.yml | GoReleaser build/archive/checksum configuration for CLI releases. |
| cli/.golangci.yml | golangci-lint configuration for CLI module. |
| README.md | Updates Quick Start to prefer CLI installation and usage. |
| CLAUDE.md | Updates repo layout/docs to include CLI and Go commands. |
| .pre-commit-config.yaml | Adds Go CLI pre-push hooks (lint/vet/test) and skips them in pre-commit.ci. |
| .gitignore | Ignores CLI build outputs/dist/coverage artifacts. |
| .github/workflows/cli.yml | New dedicated CLI CI workflow (lint/test/build/vulncheck/release). |
| .github/workflows/ci.yml | Adds path-filtering so Python/docker/dashboard jobs skip when unaffected. |
| .github/dependabot.yml | Adds gomod ecosystem updates for cli/. |
| .github/ISSUE_TEMPLATE/installer-failure.yml | Adds issue template for CLI installation failures. |
| .dockerignore | Excludes cli/ from Docker build context. |
| .claude/skills/pre-pr-review/SKILL.md | Extends pre-PR review skill with Go CLI categorization and checks. |
| .claude/skills/aurelio-review-pr/SKILL.md | Extends PR review skill with Go CLI categorization and agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+69
to
+72
| 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) | ||
| if resp, err := client.Do(req); err != nil { |
Comment on lines
+172
to
+183
| 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 | ||
| } |
Comment on lines
102
to
106
| 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 |
Comment on lines
+3
to
+14
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: | ||
| - "cli/**" | ||
| tags: | ||
| - "v*" | ||
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - "cli/**" | ||
| workflow_dispatch: |
Comment on lines
+122
to
+126
| func isUpdateAvailable(current, latest string) bool { | ||
| cur := strings.TrimPrefix(current, "v") | ||
| lat := strings.TrimPrefix(latest, "v") | ||
| return cur == "dev" || cur != lat | ||
| } |
Comment on lines
+200
to
+211
| 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() | ||
|
|
Comment on lines
+79
to
+94
| func printResourceUsage(ctx context.Context, out io.Writer, info docker.Info, state config.State) { | ||
| // Get container names to query resource usage. | ||
| psOut, err := docker.ComposeExecOutput(ctx, info, state.DataDir, "ps", "-q") | ||
| if err != nil || psOut == "" { | ||
| return | ||
| } | ||
|
|
||
| statsOut, err := docker.RunCmd(ctx, "docker", "stats", "--no-stream", "--format", | ||
| "table {{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}") | ||
| if err != nil { | ||
| fmt.Fprintf(out, "Could not get resource usage: %v\n", err) | ||
| return | ||
| } | ||
| fmt.Fprintln(out, "Resource usage:") | ||
| fmt.Fprintln(out, statsOut) | ||
| } |
| tail := strings.TrimSpace(logTail) | ||
| if tail != "all" { | ||
| if n, err := strconv.Atoi(tail); err != nil || n < 0 { | ||
| return fmt.Errorf("--tail must be a positive integer or 'all', got %q", logTail) |
Comment on lines
+92
to
+95
| if p.Sandbox && p.DockerSock != "" { | ||
| if strings.ContainsAny(p.DockerSock, "\"'`$\n\r{}[]") { | ||
| return fmt.Errorf("docker socket path %q contains unsafe characters", p.DockerSock) | ||
| } |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cli/) with 9 Cobra commands:init(interactive huh wizard),start,stop,status,logs,doctor(diagnostics + pre-filled GitHub issue URL),update(self-update from GitHub Releases),uninstall,versionimage:tags from GHCR, notbuild:'healthy'→'ok'(matchesServiceStatus.OK)cli.ymlworkflow — golangci-lint, go vet, multi-platform test matrix (ubuntu/macos/windows), cross-compile build matrix (6 targets), govulncheck, GoReleaser release onv*tagsci.ymlwithdorny/paths-filter— Python/dashboard/Docker jobs skip on unrelated changescurl|shfor Linux/macOS, PowerShell for Windows), issue template for installer failurescli/**/*.go)gomodecosystem forcli/Security hardening (from pre-PR review)
io.LimitReaderon all HTTP response reads + archive entry size guardsos.CreateTempfor self-update temp file (TOCTOU mitigation)persist-credentials: falseon release checkoutTest plan
go build ./...compiles without errorsgo test ./...passes (6 packages with tests)go vet ./...cleansynthorg versionprints dev/none/unknownsynthorg --helpshows all 9 commandssynthorg init→synthorg start→ health check passesPre-reviewed by 6 agents (docs-consistency, go-reviewer, go-security-reviewer, go-conventions-enforcer, infra-reviewer, issue-resolution-verifier), 40 findings addressed.
Closes #392