Skip to content

fix: improve update channel detection and add config get command#814

Merged
Aureliolo merged 7 commits intomainfrom
feat/improve-update-logic
Mar 24, 2026
Merged

fix: improve update channel detection and add config get command#814
Aureliolo merged 7 commits intomainfrom
feat/improve-update-logic

Conversation

@Aureliolo
Copy link
Copy Markdown
Owner

Summary

  • Fix selectBestRelease ordering bug: GitHub API returns releases out of version order when drafts are published asynchronously. The old code took the first match assuming newest-first; now compares all candidates by version to find the true latest.
  • Add dev channel mismatch warning: When running a dev build (e.g. 0.5.0-dev.8) but the update channel is "stable", synthorg update now prints a note explaining why dev releases won't appear and how to switch.
  • Add synthorg config get <key>: New subcommand to read individual config values (with shell completion). Supports all display-safe keys; secrets are excluded from the allowlist.
  • Refactor for code conventions: Extract compose functions to update_compose.go, split updater_test.go into focused test files, extract helpers to bring all functions under 50 lines and all files under 800 lines.

Test plan

  • go -C cli test ./... -- all tests pass
  • go -C cli vet ./... -- clean
  • golangci-lint run -- 0 issues
  • go -C cli build ./... -- builds successfully
  • Pre-commit hooks pass (golangci-lint, go vet, go test, gitleaks, commitizen)
  • Manual: synthorg config get channel returns current channel
  • Manual: synthorg update on dev build with stable channel shows mismatch note
  • Manual: synthorg update on dev channel finds latest dev release regardless of API order

Review coverage

Pre-reviewed by 4 agents (docs-consistency, go-reviewer, go-security-reviewer, go-conventions-enforcer). 11 findings triaged and addressed.

🤖 Generated with Claude Code

Aureliolo and others added 2 commits March 24, 2026 12:57
selectBestRelease assumed GitHub API returns releases newest-first, but
draft-then-publish releases appear out of version order. Now compares
all candidates by version to find the true latest.

Also warns when running a dev build with the stable update channel
configured, and adds `synthorg config get <key>` for reading individual
config values.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address review findings: extract compose functions to update_compose.go,
split updater_test.go into focused test files, extract helpers to bring
all functions under 50 lines (handleDeclinedCompose, downloadAndApplyCLI,
performRestart, isDevChannelMismatch, writeTempBinary, windowsPreReplace).
Add SecurePath guard to config get for consistency with config show.

Pre-reviewed by 4 agents, 11 findings addressed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA cbff36c.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 304da6e0-b160-4a66-a243-7aef164901a1

📥 Commits

Reviewing files that changed from the base of the PR and between 811fd3c and cbff36c.

📒 Files selected for processing (1)
  • cli/internal/selfupdate/updater_test.go
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: CLI Test (windows-latest)
  • GitHub Check: CLI Test (macos-latest)
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/internal/selfupdate/updater_test.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
🧬 Code graph analysis (1)
cli/internal/selfupdate/updater_test.go (1)
cli/internal/selfupdate/updater.go (2)
  • Asset (76-79)
  • CheckDevFromURL (123-156)
🔇 Additional comments (6)
cli/internal/selfupdate/updater_test.go (6)

169-196: LGTM!

Test correctly validates that stable releases take precedence over dev releases at the same base version. The assertion v0.4.7 > v0.4.7-dev.3 aligns with semantic versioning expectations where prereleases have lower precedence.


198-214: LGTM!

Test correctly validates that selectBestRelease returns an error when all candidate releases are drafts. The Content-Type header is now included as previously suggested.


216-245: LGTM!

Test validates important resilience behavior—malformed tags (e.g., dev.NaN) should not prevent selection of valid releases. This is a valuable edge case covering API response quirks.


247-289: LGTM!

This test directly validates the PR's core fix: selectBestRelease must compare by semantic version rather than relying on GitHub's list order. The arrangement with v0.5.0-dev.11 appearing third but being selected as highest is an excellent test case for the ordering bug fix.


291-318: LGTM!

Test validates that the semantic version comparison fix also applies to stable-only release lists. Good complementary coverage to the dev-focused out-of-order test.


140-167: No action needed. expectedURLPrefix is properly defined as a package-level variable in cli/internal/selfupdate/updater.go (line 36) and is accessible to all tests in the package. The tests will compile and run correctly.


Walkthrough

Adds a new config get <key> CLI subcommand with key completion, directory validation, explicit errors for invalid paths, load failures, unknown keys, and rejection of secret keys. Moves compose refresh, diffing, and image-patching logic into a new cli/cmd/update_compose.go, removing that logic from update.go and centralizing restart and image-capture helpers. Refactors CLI self-update flow (download/apply helpers), tightens redirect-host allowlisting, changes release selection and dev-tag parsing, and refactors ReplaceAt. Expands unit and integration tests across config and self-update paths and updates the user guide with a config get example.

Suggested labels

autorelease: tagged

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.84% which is insufficient. The required threshold is 40.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: fixing update channel detection and adding a config get command, both of which are prominent in the changeset.
Description check ✅ Passed The description is detailed and directly related to the changeset, covering all major changes including the selectBestRelease fix, dev channel mismatch warning, config get command, and refactoring work.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the summary. You can try again by commenting /gemini summary.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/cmd/config_test.go`:
- Around line 456-475: TestConfigGetDefaultChannel currently saves
DefaultState() which writes "channel":"stable" and defeats the test; instead
create and write a minimal config JSON file that omits the "channel" property
(seed config.json directly in the temp dir used by DataDir) so the loader
exercises the fallback that DefaultState provides. In the
TestConfigGetDefaultChannel test replace the call to config.Save(state) with
code that marshals a struct/map containing the required fields except "channel"
and writes it to the config file path in dir (so rootCmd.Execute() reads a
config.json missing channel and triggers the defaulting logic).

In `@cli/cmd/update_compose.go`:
- Around line 228-260: The regex in imageLinePattern is too permissive and
matches repos like synthorg-backend-fips; update the pattern so it only matches
the exact synthorg-{service} repo optionally followed by a tag or digest.
Replace the current value of imageLinePattern with a pattern like
`(\s+image:\s+)ghcr\.io/aureliolo/synthorg-(backend|web|sandbox)(?:[:@][^\s]+)?`
so patchComposeImageRefs still captures the leading "image: " prefix and service
name but will not rewrite custom repo names that extend the base name.

In `@cli/internal/selfupdate/updater_download_test.go`:
- Around line 250-288: The TestReplace currently performs manual os.Rename calls
instead of exercising the actual updater logic; update the test to call the
production Replace (or ReplaceAt) helper so it verifies the real replacement
path (handle Windows special-casing if Replace/ReplaceAt do that) — locate
TestReplace in updater_download_test.go and replace the manual rename block with
a call to Replace or ReplaceAt (using the same fakeBinary/newPath setup and
permissions) and assert the resulting file contents match newContent;
alternatively, if you intentionally don't want to test the production helper,
rename the test to not imply it covers Replace/ReplaceAt.

In `@cli/internal/selfupdate/updater.go`:
- Around line 153-165: Ensure we don't accept a malformed tag as the initial
baseline: before assigning latestDev or latestStable when they are nil, validate
the candidate tag by calling compareWithDev(r.TagName, r.TagName) (or otherwise
parsing/validating the tag) and only set latestDev/latestStable if that call
returns err == nil; keep the existing compare logic for subsequent candidates
(compareWithDev(r.TagName, latestDev.TagName) / compareWithDev(r.TagName,
latestStable.TagName)), and apply the same validation change to the other
occurrence mentioned (lines 178-180) so an invalid initial baseline cannot
poison later comparisons.
- Around line 452-457: When os.Rename(tmpPath, execPath) fails after
windowsPreReplace has moved the original binary (oldPath != ""), attempt the
rollback rename of oldPath -> execPath and if that rollback fails do not delete
tmpPath and return a combined error that includes both the original os.Rename
error and the rollback error; if the rollback succeeds then you may remove
tmpPath and return the original error. Update the error handling around
tmpPath/execPath (referencing tmpPath, execPath, oldPath and the
windowsPreReplace rename context) to capture the rollback error, avoid deleting
tmpPath when rollback failed, and wrap both errors in the returned fmt.Errorf
message.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7ce95995-b62f-4095-abba-360ae3d30e26

📥 Commits

Reviewing files that changed from the base of the PR and between 9b75c1d and 27529f9.

📒 Files selected for processing (8)
  • cli/cmd/config.go
  • cli/cmd/config_test.go
  • cli/cmd/update.go
  • cli/cmd/update_compose.go
  • cli/cmd/update_test.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_download_test.go
  • cli/internal/selfupdate/updater_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: CLI Test (windows-latest)
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/cmd/update_test.go
  • cli/cmd/config.go
  • cli/cmd/config_test.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_test.go
  • cli/cmd/update.go
  • cli/cmd/update_compose.go
  • cli/internal/selfupdate/updater_download_test.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.
📚 Learning: 2026-03-15T18:17:43.675Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:17:43.675Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).

Applied to files:

  • cli/cmd/config.go
  • cli/cmd/update.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-24T11:40:22.911Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.911Z
Learning: Applies to cli/**/*.go : Use Go 1.26+ with Cobra CLI framework; run `go -C cli` from repo root (never `cd cli`), run tests with `-n auto` parallelism, use native `testing.F` for fuzz tests

Applied to files:

  • cli/cmd/config.go
  • cli/cmd/update.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/**/*.go : Go CLI (Go 1.26+) uses Cobra for commands, charmbracelet/huh for interactive CLI, charmbracelet/lipgloss for styled output. Cross-platform builds (linux/darwin/windows × amd64/arm64). GoReleaser for releases with cosign keyless signing of checksums.txt. SLSA L3 provenance attestations via actions/attest-build-provenance.

Applied to files:

  • cli/cmd/config.go
  • cli/internal/selfupdate/updater.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/go.mod : Go CLI dependencies: Go 1.26+, Cobra (commands), charmbracelet/huh (interactive CLI), charmbracelet/lipgloss (styled output).

Applied to files:

  • cli/cmd/config.go
  • cli/cmd/update.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.

Applied to files:

  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_test.go
  • cli/cmd/update.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: Applies to go.mod : Maintain Go 1.26+ requirement. Dependencies: Cobra (CLI framework), charmbracelet/huh and charmbracelet/lipgloss (UI), sigstore-go (code signing), go-containerregistry (container image verification), go-tuf (TUF client for Sigstore).

Applied to files:

  • cli/internal/selfupdate/updater_test.go
  • cli/cmd/update.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: CLI workflow (`.github/workflows/cli.yml`) runs Go lint (golangci-lint + go vet) + test (race, coverage) + build (cross-compile matrix) + vulnerability check (govulncheck) + fuzz testing. Cross-compiles for linux/darwin/windows × amd64/arm64. GoReleaser release on v* tags with cosign keyless signing and SLSA L3 attestations.

Applied to files:

  • cli/internal/selfupdate/updater_test.go
🧬 Code graph analysis (6)
cli/cmd/config.go (2)
cli/internal/config/paths.go (1)
  • SecurePath (58-64)
cli/internal/config/state.go (1)
  • Load (65-103)
cli/cmd/config_test.go (3)
cli/internal/config/state.go (2)
  • DefaultState (36-48)
  • Save (230-244)
cli/internal/config/paths.go (1)
  • DataDir (18-31)
cli/cmd/root.go (1)
  • Execute (101-107)
cli/internal/selfupdate/updater_test.go (1)
cli/internal/selfupdate/updater.go (2)
  • Asset (59-62)
  • CheckDevFromURL (106-139)
cli/cmd/update.go (4)
cli/internal/config/state.go (1)
  • State (16-31)
cli/internal/selfupdate/updater.go (3)
  • CheckResult (65-72)
  • Download (387-422)
  • Replace (425-431)
cli/internal/version/version.go (2)
  • RepoURL (5-5)
  • Version (9-9)
cli/internal/docker/client.go (1)
  • Info (22-29)
cli/cmd/update_compose.go (3)
cli/internal/config/state.go (1)
  • State (16-31)
cli/internal/compose/generate.go (2)
  • ParamsFromState (50-64)
  • Generate (68-87)
cli/internal/images/images.go (1)
  • RepoPrefix (15-15)
cli/internal/selfupdate/updater_download_test.go (1)
cli/internal/selfupdate/updater.go (5)
  • Release (53-56)
  • Asset (59-62)
  • Download (387-422)
  • CheckFromURL (271-295)
  • ReplaceAt (435-465)
🪛 GitHub Check: CodeQL
cli/cmd/update_compose.go

[failure] 112-112: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.


[failure] 241-241: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.

- Fix splitDev returning full string instead of sliced base on Atoi
  failure (malformed dev tags now correctly return base version)
- Fix compareWithDev doc comment notation (.dev3 -> -dev.3)
- Fix runConfigGet discarding validated safeDir from SecurePath
- Extract captureImageIDsForCleanup to keep updateContainerImages
  under 50-line limit
- Add per-hop redirect host validation via CheckRedirect on both
  apiClient and download HTTP client (defense-in-depth)
- Add t.Cleanup to new config get tests to reset shared rootCmd state
- Document config get subcommand in user guide

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 12:38 — with GitHub Actions Inactive
- Tighten imageLinePattern regex to prevent matching extended repo
  names (e.g. synthorg-backend-fips) by requiring [:@] before suffix
- Validate initial baseline tags in selectBestRelease to prevent
  malformed tags from poisoning release selection
- Handle failed Windows rollback in ReplaceAt: report combined error
  and preserve tmpPath for manual recovery
- Fix TestConfigGetDefaultChannel to exercise actual channel fallback
  by seeding config.json without "channel" key
- Fix TestReplace to call ReplaceAt instead of manual os.Rename

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 12:43 — with GitHub Actions Inactive
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
cli/cmd/config_test.go (1)

468-480: ⚠️ Potential issue | 🟡 Minor

This setup still never exercises the default-channel fallback.

config.Save(state) serializes "channel":"stable", so the loader never has to supply the default when channel is missing. Seed config.json without a channel property if you want this test to cover the fallback path.

Possible fix
 	dir := t.TempDir()
-	state := config.DefaultState()
-	state.DataDir = dir
-	if err := config.Save(state); err != nil {
+	raw, err := json.Marshal(map[string]any{
+		"data_dir": dir,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dir, "config.json"), raw, 0o600); err != nil {
 		t.Fatal(err)
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/config_test.go` around lines 468 - 480, TestConfigGetDefaultChannel
currently calls config.Save(state) which writes "channel":"stable" so the
fallback path is never exercised; change the setup to create a config.json
without a channel property instead of using config.Save. Locate
TestConfigGetDefaultChannel and, after setting state.DataDir, write a minimal
JSON file into the state directory (or serialize a struct/map that omits the
Channel field) so the file lacks "channel" (e.g., write
`{"data_dir":"<dir>"}`-style JSON) before invoking the loader; this forces the
loader's default-channel logic to run and validates the fallback.
cli/internal/selfupdate/updater.go (2)

469-474: ⚠️ Potential issue | 🟠 Major

Keep tmpPath if the Windows rollback also fails.

When os.Rename(tmpPath, execPath) fails after windowsPreReplace moved the current binary aside, this path ignores a failed rollback and still deletes tmpPath. That can leave no binary at execPath and no staged replacement to recover from.

Possible fix
 	if err := os.Rename(tmpPath, execPath); err != nil {
 		if runtime.GOOS == "windows" && oldPath != "" {
-			_ = os.Rename(oldPath, execPath)
+			if rollbackErr := os.Rename(oldPath, execPath); rollbackErr != nil {
+				return fmt.Errorf(
+					"replacing binary: %w (rollback failed: %v; old binary left at %s; staged binary left at %s)",
+					err, rollbackErr, oldPath, tmpPath,
+				)
+			}
 		}
 		_ = os.Remove(tmpPath)
 		return fmt.Errorf("replacing binary: %w", err)
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/internal/selfupdate/updater.go` around lines 469 - 474, When
os.Rename(tmpPath, execPath) fails (in the block handling the replace in
updater.go), don't unconditionally remove tmpPath; first attempt the Windows
rollback by calling os.Rename(oldPath, execPath) and capture its error into a
variable (referencing oldPath, execPath and os.Rename). If the rollback returns
an error, preserve tmpPath (do not call os.Remove(tmpPath)) and return an error
that includes both the original replace error and the rollback error; if the
rollback succeeds, then remove tmpPath and return the original replace error.
Ensure the error message clearly includes both failures when rollback fails.

167-179: ⚠️ Potential issue | 🟠 Major

Validate release tags before they become baselines.

These branches still accept the first matching tag as latestDev/latestStable. With the new splitDev fallback, a malformed tag such as v0.5.0-dev.NaN now compares like a stable 0.5.0, so a bad first candidate can suppress later valid releases at the same base and make selectBestRelease pick the wrong tag. Please reject malformed tags before seeding either baseline, and add a regression case for that ordering.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/internal/selfupdate/updater.go` around lines 167 - 179, The code seeds
latestDev/latestStable with the first matching tag even if malformed (e.g.,
v0.5.0-dev.NaN) causing later valid releases to be suppressed; update the
branches that set latestDev and latestStable to validate the tag before seeding
by parsing/checking it (use the same validation used by compareWithDev/splitDev
and reject tags that return an error or an invalid parse) so you only assign a
baseline when the tag is well-formed, and add a regression test exercising a
malformed first tag followed by a valid tag to ensure selectBestRelease chooses
the valid one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/cmd/config_test.go`:
- Around line 445-466: Extend the tests to explicitly assert that secret keys
are rejected: add a new test (or expand TestConfigGetUnknownKey) which creates a
saved state (using config.DefaultState and config.Save), then runs rootCmd with
args ["config","get","jwt_secret", "--data-dir", dir] and
["config","get","settings_key", "--data-dir", dir] and assert that
rootCmd.Execute() returns an error for both cases; reuse the same setup/cleanup
pattern (rootCmd.SetOut/SetErr/SetArgs) so the test fails if these secret keys
become readable by the get command.

---

Duplicate comments:
In `@cli/cmd/config_test.go`:
- Around line 468-480: TestConfigGetDefaultChannel currently calls
config.Save(state) which writes "channel":"stable" so the fallback path is never
exercised; change the setup to create a config.json without a channel property
instead of using config.Save. Locate TestConfigGetDefaultChannel and, after
setting state.DataDir, write a minimal JSON file into the state directory (or
serialize a struct/map that omits the Channel field) so the file lacks "channel"
(e.g., write `{"data_dir":"<dir>"}`-style JSON) before invoking the loader; this
forces the loader's default-channel logic to run and validates the fallback.

In `@cli/internal/selfupdate/updater.go`:
- Around line 469-474: When os.Rename(tmpPath, execPath) fails (in the block
handling the replace in updater.go), don't unconditionally remove tmpPath; first
attempt the Windows rollback by calling os.Rename(oldPath, execPath) and capture
its error into a variable (referencing oldPath, execPath and os.Rename). If the
rollback returns an error, preserve tmpPath (do not call os.Remove(tmpPath)) and
return an error that includes both the original replace error and the rollback
error; if the rollback succeeds, then remove tmpPath and return the original
replace error. Ensure the error message clearly includes both failures when
rollback fails.
- Around line 167-179: The code seeds latestDev/latestStable with the first
matching tag even if malformed (e.g., v0.5.0-dev.NaN) causing later valid
releases to be suppressed; update the branches that set latestDev and
latestStable to validate the tag before seeding by parsing/checking it (use the
same validation used by compareWithDev/splitDev and reject tags that return an
error or an invalid parse) so you only assign a baseline when the tag is
well-formed, and add a regression test exercising a malformed first tag followed
by a valid tag to ensure selectBestRelease chooses the valid one.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a2cf609d-181e-48f9-b6be-80ddd50eef5b

📥 Commits

Reviewing files that changed from the base of the PR and between 27529f9 and c2dd0f9.

📒 Files selected for processing (6)
  • cli/cmd/config.go
  • cli/cmd/config_test.go
  • cli/cmd/update.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_version_test.go
  • docs/user_guide.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: CLI Test (windows-latest)
  • GitHub Check: CLI Test (macos-latest)
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

Documentation built with Zensical (config: mkdocs.yml); API reference auto-generated via mkdocstrings + Griffe (AST-based); design spec in docs/design/ (9 pages) is the starting point for all implementation

Files:

  • docs/user_guide.md
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/cmd/config.go
  • cli/internal/selfupdate/updater_version_test.go
  • cli/cmd/config_test.go
  • cli/internal/selfupdate/updater.go
  • cli/cmd/update.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to docker/Dockerfile.sandbox : Docker sandbox: `synthorg-sandbox` — Python 3.14 + Node.js + git, non-root (UID 10001), agent code execution sandbox

Applied to files:

  • docs/user_guide.md
📚 Learning: 2026-03-24T11:40:22.912Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override

Applied to files:

  • docs/user_guide.md
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...

Applied to files:

  • docs/user_guide.md
📚 Learning: 2026-03-15T18:17:43.675Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:17:43.675Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).

Applied to files:

  • cli/cmd/config.go
  • cli/cmd/update.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/**/*.go : Go CLI (Go 1.26+) uses Cobra for commands, charmbracelet/huh for interactive CLI, charmbracelet/lipgloss for styled output. Cross-platform builds (linux/darwin/windows × amd64/arm64). GoReleaser for releases with cosign keyless signing of checksums.txt. SLSA L3 provenance attestations via actions/attest-build-provenance.

Applied to files:

  • cli/cmd/config.go
📚 Learning: 2026-03-24T11:40:22.911Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.911Z
Learning: Applies to cli/**/*.go : Use Go 1.26+ with Cobra CLI framework; run `go -C cli` from repo root (never `cd cli`), run tests with `-n auto` parallelism, use native `testing.F` for fuzz tests

Applied to files:

  • cli/cmd/config_test.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/go.mod : Go CLI dependencies: Go 1.26+, Cobra (commands), charmbracelet/huh (interactive CLI), charmbracelet/lipgloss (styled output).

Applied to files:

  • cli/cmd/update.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: Applies to go.mod : Maintain Go 1.26+ requirement. Dependencies: Cobra (CLI framework), charmbracelet/huh and charmbracelet/lipgloss (UI), sigstore-go (code signing), go-containerregistry (container image verification), go-tuf (TUF client for Sigstore).

Applied to files:

  • cli/cmd/update.go
🧬 Code graph analysis (3)
cli/cmd/config.go (2)
cli/internal/config/paths.go (1)
  • SecurePath (58-64)
cli/internal/config/state.go (1)
  • Load (65-103)
cli/cmd/config_test.go (3)
cli/internal/config/state.go (2)
  • DefaultState (36-48)
  • Save (230-244)
cli/internal/config/paths.go (1)
  • DataDir (18-31)
cli/cmd/root.go (1)
  • Execute (101-107)
cli/cmd/update.go (3)
cli/internal/config/state.go (1)
  • State (16-31)
cli/internal/selfupdate/updater.go (3)
  • CheckResult (79-86)
  • Download (401-439)
  • Replace (442-448)
cli/internal/version/version.go (2)
  • RepoURL (5-5)
  • Version (9-9)

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/internal/selfupdate/updater.go (1)

272-285: 🧹 Nitpick | 🔵 Trivial

Clarify semantic: malformed -dev. suffix is treated as stable.

When splitDev encounters a tag like "0.4.7-dev." or "0.4.7-dev.NaN", it now returns (-1, "0.4.7"). Since devNum == -1 is the marker for "stable", this means compareWithDev("0.4.7-dev.", "0.4.7") returns 0 (equal).

This is a reasonable fallback, but the comment could clarify this semantic for future maintainers:

📝 Suggested documentation improvement
-// splitDev splits "0.4.7-dev.3" into (3, "0.4.7") or (-1, "0.4.7") if no -dev. suffix.
+// splitDev splits "0.4.7-dev.3" into (3, "0.4.7") or (-1, "0.4.7") if no -dev. suffix
+// or if the dev number is unparseable (e.g., "0.4.7-dev." → (-1, "0.4.7")).
 func splitDev(v string) (devNum int, base string) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/internal/selfupdate/updater.go` around lines 272 - 285, Update the
comment for splitDev to explicitly document that when the "-dev." suffix is
missing or malformed (e.g., "0.4.7-dev." or "0.4.7-dev.NaN"), the function
returns devNum == -1 and the base version (treating the tag as stable), which
makes compareWithDev treat such malformed dev tags as equivalent to the base
stable tag; mention the returned tuple semantics (devNum, base) and that -1
denotes "stable/no-dev".
♻️ Duplicate comments (2)
cli/cmd/config_test.go (1)

445-466: ⚠️ Potential issue | 🟠 Major

Add explicit secret-key rejection assertions.

This currently validates only the generic unknown-key path. Add explicit failures for jwt_secret and settings_key to prevent silent regressions in secret-read protections.

Proposed test tightening
 func TestConfigGetUnknownKey(t *testing.T) {
 	t.Cleanup(func() {
 		rootCmd.SetOut(nil)
 		rootCmd.SetErr(nil)
 		rootCmd.SetArgs(nil)
 	})
 	dir := t.TempDir()
 	state := config.DefaultState()
 	state.DataDir = dir
 	if err := config.Save(state); err != nil {
 		t.Fatal(err)
 	}
 
-	var buf bytes.Buffer
-	rootCmd.SetOut(&buf)
-	rootCmd.SetErr(&buf)
-	rootCmd.SetArgs([]string{"config", "get", "unknown_key", "--data-dir", dir})
-	err := rootCmd.Execute()
-	if err == nil {
-		t.Fatal("expected error for unknown key")
-	}
+	for _, key := range []string{"unknown_key", "jwt_secret", "settings_key"} {
+		t.Run(key, func(t *testing.T) {
+			var buf bytes.Buffer
+			rootCmd.SetOut(&buf)
+			rootCmd.SetErr(&buf)
+			rootCmd.SetArgs([]string{"config", "get", key, "--data-dir", dir})
+			if err := rootCmd.Execute(); err == nil {
+				t.Fatalf("expected error for disallowed key %q", key)
+			}
+		})
+	}
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/config_test.go` around lines 445 - 466, The TestConfigGetUnknownKey
test currently only asserts a generic error for an unknown key; extend it to
explicitly check that requests for protected secrets fail by invoking rootCmd
with args for "config get jwt_secret" and "config get settings_key" (using the
same setup: set rootCmd output/error to buf, set args with "--data-dir", call
rootCmd.Execute()) and assert that each call returns a non-nil error and
contains an appropriate error message; update TestConfigGetUnknownKey (and reuse
the same temp state created via config.DefaultState() and config.Save) to
include these two additional assertions so secret-key reads are explicitly
rejected.
cli/cmd/update_compose.go (1)

99-101: ⚠️ Potential issue | 🟠 Major

Anchor the image regex; it still matches custom repos like synthorg-backend-fips.

Despite the comment above imageLinePattern, (?:[:@]\S+)? is optional, so the regex still matches the synthorg-backend prefix inside ghcr.io/aureliolo/synthorg-backend-fips:.... That means patchComposeImageRefs can rewrite the line into a broken mixed repo/tag, and isUpdateBoilerplateOnly will also treat the custom repo as a stock backend image and auto-apply it without prompting. Require an exact repo match and preserve any trailing comment explicitly.

Suggested fix
 var imageLinePattern = regexp.MustCompile(
-	`(\s+image:\s+)ghcr\.io/aureliolo/synthorg-(backend|web|sandbox)(?:[:@]\S+)?`,
+	`(?m)^([ \t]*image:[ \t]+)` + regexp.QuoteMeta(images.RepoPrefix) +
+		`(backend|web|sandbox)(?:[:@][^[:space:]#]+)?([ \t]+#.*)?[ \t]*$`,
 )
@@
 	patched := imageLinePattern.ReplaceAllStringFunc(string(existing), func(match string) string {
 		sub := imageLinePattern.FindStringSubmatch(match)
-		if len(sub) < 3 {
+		if len(sub) < 4 {
 			return match
 		}
-		prefix := sub[1] // e.g. "    image: "
-		name := sub[2]   // e.g. "backend"
+		prefix := sub[1]
+		name := sub[2]
+		comment := sub[3]
 		repo := images.RepoPrefix + name
 		replaced[name] = true
 
 		if d, ok := digestPins[name]; ok && d != "" {
-			return prefix + repo + "@" + d
+			return prefix + repo + "@" + d + comment
 		}
-		return prefix + repo + ":" + tag
+		return prefix + repo + ":" + tag + comment
 	})

Also applies to: 230-232, 248-262

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/update_compose.go` around lines 99 - 101, The image regex
(imageLinePattern) is too permissive and matches substrings like
"synthorg-backend" inside "ghcr.io/.../synthorg-backend-fips", causing
patchComposeImageRefs and isUpdateBoilerplateOnly to misidentify and rewrite
images; tighten the pattern to require an exact repo name boundary (e.g., match
either start or a separator like [/:] before the repo and enforce end-of-repo
before optional tag/digest), explicitly capture and preserve any trailing inline
comment, and update all usages (the matching logic around oldSub/newSub in
patchComposeImageRefs and related comparisons used by isUpdateBoilerplateOnly)
so they use the new anchored groups instead of the previous optional
(?:[:@]\S+)? group. Ensure the new regex only considers the repo name as a full
token and keeps the tag/digest and comment intact when generating replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/internal/selfupdate/updater_download_test.go`:
- Around line 250-272: Tests TestReplace and TestReplaceAt both duplicate
coverage for ReplaceAt; consolidate by removing one of them or make them
exercise distinct scenarios. Edit updater_download_test.go to either delete
TestReplace (or TestReplaceAt) and keep a single canonical test for ReplaceAt,
or modify one test to cover a different edge case (e.g., symlink resolution,
varying file permissions, Windows .exe behavior, or atomic replace failure path)
so each test name (TestReplace and TestReplaceAt) verifies a unique behavior of
ReplaceAt rather than repeating the same assertions.

---

Outside diff comments:
In `@cli/internal/selfupdate/updater.go`:
- Around line 272-285: Update the comment for splitDev to explicitly document
that when the "-dev." suffix is missing or malformed (e.g., "0.4.7-dev." or
"0.4.7-dev.NaN"), the function returns devNum == -1 and the base version
(treating the tag as stable), which makes compareWithDev treat such malformed
dev tags as equivalent to the base stable tag; mention the returned tuple
semantics (devNum, base) and that -1 denotes "stable/no-dev".

---

Duplicate comments:
In `@cli/cmd/config_test.go`:
- Around line 445-466: The TestConfigGetUnknownKey test currently only asserts a
generic error for an unknown key; extend it to explicitly check that requests
for protected secrets fail by invoking rootCmd with args for "config get
jwt_secret" and "config get settings_key" (using the same setup: set rootCmd
output/error to buf, set args with "--data-dir", call rootCmd.Execute()) and
assert that each call returns a non-nil error and contains an appropriate error
message; update TestConfigGetUnknownKey (and reuse the same temp state created
via config.DefaultState() and config.Save) to include these two additional
assertions so secret-key reads are explicitly rejected.

In `@cli/cmd/update_compose.go`:
- Around line 99-101: The image regex (imageLinePattern) is too permissive and
matches substrings like "synthorg-backend" inside
"ghcr.io/.../synthorg-backend-fips", causing patchComposeImageRefs and
isUpdateBoilerplateOnly to misidentify and rewrite images; tighten the pattern
to require an exact repo name boundary (e.g., match either start or a separator
like [/:] before the repo and enforce end-of-repo before optional tag/digest),
explicitly capture and preserve any trailing inline comment, and update all
usages (the matching logic around oldSub/newSub in patchComposeImageRefs and
related comparisons used by isUpdateBoilerplateOnly) so they use the new
anchored groups instead of the previous optional (?:[:@]\S+)? group. Ensure the
new regex only considers the repo name as a full token and keeps the tag/digest
and comment intact when generating replacements.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 39a6292d-68e6-4b8e-b29b-0656c38d8761

📥 Commits

Reviewing files that changed from the base of the PR and between c2dd0f9 and 9d9e004.

📒 Files selected for processing (4)
  • cli/cmd/config_test.go
  • cli/cmd/update_compose.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_download_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/internal/selfupdate/updater.go
  • cli/cmd/config_test.go
  • cli/cmd/update_compose.go
  • cli/internal/selfupdate/updater_download_test.go
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: Applies to go.mod : Maintain Go 1.26+ requirement. Dependencies: Cobra (CLI framework), charmbracelet/huh and charmbracelet/lipgloss (UI), sigstore-go (code signing), go-containerregistry (container image verification), go-tuf (TUF client for Sigstore).

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/**/*.go : Go CLI (Go 1.26+) uses Cobra for commands, charmbracelet/huh for interactive CLI, charmbracelet/lipgloss for styled output. Cross-platform builds (linux/darwin/windows × amd64/arm64). GoReleaser for releases with cosign keyless signing of checksums.txt. SLSA L3 provenance attestations via actions/attest-build-provenance.

Applied to files:

  • cli/internal/selfupdate/updater.go
  • cli/cmd/config_test.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: CLI workflow (`.github/workflows/cli.yml`) runs Go lint (golangci-lint + go vet) + test (race, coverage) + build (cross-compile matrix) + vulnerability check (govulncheck) + fuzz testing. Cross-compiles for linux/darwin/windows × amd64/arm64. GoReleaser release on v* tags with cosign keyless signing and SLSA L3 attestations.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-24T11:40:22.911Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.911Z
Learning: Applies to cli/**/*.go : Use Go 1.26+ with Cobra CLI framework; run `go -C cli` from repo root (never `cd cli`), run tests with `-n auto` parallelism, use native `testing.F` for fuzz tests

Applied to files:

  • cli/cmd/config_test.go
📚 Learning: 2026-03-15T18:17:43.675Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:17:43.675Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).

Applied to files:

  • cli/cmd/config_test.go
  • cli/cmd/update_compose.go
📚 Learning: 2026-03-24T11:40:22.911Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.911Z
Learning: Applies to cli/**/*.go : Run linting with `go -C cli tool golangci-lint run`, vetting with `go -C cli vet`, and tests with `go -C cli test ./...` (without -fuzz for standard test runs)

Applied to files:

  • cli/cmd/config_test.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to cli/go.mod : Go CLI dependencies: Go 1.26+, Cobra (commands), charmbracelet/huh (interactive CLI), charmbracelet/lipgloss (styled output).

Applied to files:

  • cli/cmd/update_compose.go
🧬 Code graph analysis (3)
cli/cmd/config_test.go (3)
cli/internal/config/state.go (2)
  • DefaultState (36-48)
  • Save (230-244)
cli/internal/config/paths.go (1)
  • DataDir (18-31)
cli/cmd/root.go (1)
  • Execute (101-107)
cli/cmd/update_compose.go (3)
cli/internal/config/state.go (1)
  • State (16-31)
cli/internal/compose/generate.go (2)
  • ParamsFromState (50-64)
  • Generate (68-87)
cli/internal/images/images.go (1)
  • RepoPrefix (15-15)
cli/internal/selfupdate/updater_download_test.go (1)
cli/internal/selfupdate/updater.go (5)
  • ReplaceAt (456-490)
  • Release (67-70)
  • Asset (73-76)
  • Download (405-443)
  • CheckFromURL (289-313)
🪛 GitHub Check: CodeQL
cli/cmd/update_compose.go

[failure] 112-112: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.


[failure] 243-243: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.

🔇 Additional comments (13)
cli/cmd/config_test.go (2)

389-443: Good table-driven coverage and Cobra state isolation.

This exercises all intended non-secret config get keys and correctly isolates shared rootCmd state between subtests.


468-503: Default-channel fallback test is now correctly structured.

Seeding config.json without channel directly validates the real defaulting path and asserts the expected "stable" output.

cli/internal/selfupdate/updater.go (6)

46-64: LGTM! Redirect host validation is well-implemented.

The checkRedirectHost function correctly validates each redirect hop against AllowedDownloadHosts before the connection is established, preventing SSRF via redirect chains. Applying this to both apiClient and the download client provides defense-in-depth alongside the post-response check in httpGetWithClient.


167-184: LGTM! Malformed tag handling properly prevents poisoning.

The self-comparison validation (compareWithDev(r.TagName, r.TagName)) elegantly ensures only parseable tags become candidates. Combined with the version-based comparison instead of API ordering, this correctly addresses the out-of-order release scenario.


410-413: LGTM! Download client properly configured with redirect validation.

Consistent application of checkRedirectHost to the download client ensures redirect-based SSRF is blocked for both API calls and binary downloads.


473-489: LGTM! Windows rollback error handling now preserves recovery artifacts.

Using errors.Join to combine both the original error and rollback error is clean. Preserving tmpPath when rollback fails allows manual recovery, and the error message now includes the location of the old binary.


492-521: LGTM! writeTempBinary extraction is clean with proper cleanup.

The helper correctly cleans up the temp file on any failure path (Write, Chmod, Sync, or Close errors). The explicit Sync before Close ensures data is flushed to disk before the rename.


523-544: LGTM! windowsPreReplace correctly handles the Windows-specific flow.

The pattern of creating a temp file, closing it, removing it, then using Rename to move the current binary to that path is the standard approach for Windows binary replacement. Cleanup of tmpPath on failure prevents orphaned files.

cli/internal/selfupdate/updater_download_test.go (5)

22-248: LGTM! Comprehensive extraction test coverage.

The tests cover both archive formats (tar.gz, zip), nested paths, missing binaries, invalid data, and OS-specific routing. Good use of table-driven patterns for error cases.


274-340: LGTM! Test correctly exercises lower-level JSON parsing.

TestCheckWithMockServer intentionally tests httpGetWithClient and JSON parsing separately from CheckFromURL, which is why it uses example.com URLs that would fail the production findAssets URL prefix validation. The TestCheckFromURL test properly uses expectedURLPrefix for end-to-end validation.


575-609: LGTM! testArchive helper reduces test duplication.

Good use of t.Helper() for proper stack traces on failure. The OS-branching matches the production extractBinary logic.


611-675: LGTM! Bundle error handling tests are appropriate.

Tests correctly verify error messages for bundle download failures and invalid JSON. The same AllowedDownloadHosts concern from TestDownloadWithMockServer applies here.


342-406: AllowedDownloadHosts is properly configured for tests.

Test setup in updater_test.go explicitly adds 127.0.0.1 and localhost to AllowedDownloadHosts (lines 20-21), so the validation in httpGetWithClient (line 576) will not reject test servers. The test failure observed is due to a different issue (connection refused), unrelated to the host allowlist validation.

			> Likely an incorrect or invalid review comment.

- Add TestConfigGetRejectsSecretKeys: verify jwt_secret and
  settings_key are not readable via config get
- Replace duplicate TestReplace with TestReplaceAtViaSymlink to test
  symlink resolution edge case (distinct from TestReplaceAt)
- Update splitDev doc comment to document malformed suffix behavior
  and -1 sentinel semantics

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 13:34 — with GitHub Actions Inactive
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
cli/internal/selfupdate/updater.go (1)

167-183: ⚠️ Potential issue | 🟠 Major

Malformed -dev. tags still bypass the new baseline validation.

splitDev now maps values like v1.2.3-dev.NaN to (-1, "1.2.3"), so compareWithDev(r.TagName, r.TagName) no longer rejects them. In this branch, that tag still counts as a dev candidate, becomes the initial latestDev, and then beats valid v1.2.3-dev.8 tags because the comparator treats the malformed baseline as stable-at-base-version. Skip prereleases whose -dev. suffix does not parse before assigning or comparing latestDev.

🐛 Suggested fix
 		if r.Prerelease && strings.Contains(r.TagName, "-dev.") {
+			if devNum, _ := splitDev(strings.TrimPrefix(r.TagName, "v")); devNum < 0 {
+				continue
+			}
 			if latestDev == nil {
 				latestDev = r
 			} else if cmp, err := compareWithDev(r.TagName, latestDev.TagName); err == nil && cmp > 0 {
 				latestDev = r
 			}

Also applies to: 272-286

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/internal/selfupdate/updater.go` around lines 167 - 183, The prerelease
selection logic accepts malformed "-dev." tags because compareWithDev no longer
rejects them; update the branch that handles r.Prerelease (where latestDev is
set/compared) to first verify the tag's dev suffix parses successfully (use
compareWithDev or splitDev to check/parse and ensure it returns a valid dev
index) and skip the prerelease if parsing fails; apply the same guard in the
other symmetric block that assigns/compares latestDev so only properly parsed
"-dev.N" tags are considered when initializing or comparing latestDev.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/internal/selfupdate/updater_download_test.go`:
- Around line 289-355: The tests use an httptest.Server (srv) and call
httpGetWithClient (and Download elsewhere) which now validate hosts against
AllowedDownloadHosts; to fix, before calling httpGetWithClient or Download in
TestCheckWithMockServer (and the other httptest-backed tests), extract the
hostname from srv.URL and add it to AllowedDownloadHosts (or call the existing
helper that does this) so the test host (127.0.0.1/::1) is allowlisted; update
the blocks in the "parse release JSON" and "asset matching" subtests (and the
other referenced test ranges) to perform this allowlist injection using the same
helper/assertion pattern to ensure calls to httpGetWithClient/Download succeed.

In `@cli/internal/selfupdate/updater.go`:
- Around line 50-53: checkRedirectHost currently only checks the hostname and
allows redirects that downgrade from HTTPS to HTTP; update it to also validate
the URL scheme by rejecting any redirect where req.URL.Scheme != "https". In the
checkRedirectHost function, after verifying
AllowedDownloadHosts[req.URL.Hostname()], add a scheme check that returns a
descriptive error (e.g., "redirect to disallowed scheme %q") when the scheme is
not "https", so only HTTPS redirects are accepted. Ensure you reference
req.URL.Scheme and AllowedDownloadHosts in the new validation.

---

Duplicate comments:
In `@cli/internal/selfupdate/updater.go`:
- Around line 167-183: The prerelease selection logic accepts malformed "-dev."
tags because compareWithDev no longer rejects them; update the branch that
handles r.Prerelease (where latestDev is set/compared) to first verify the tag's
dev suffix parses successfully (use compareWithDev or splitDev to check/parse
and ensure it returns a valid dev index) and skip the prerelease if parsing
fails; apply the same guard in the other symmetric block that assigns/compares
latestDev so only properly parsed "-dev.N" tags are considered when initializing
or comparing latestDev.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d56065e6-d6e2-45a5-bb18-abfe6876868b

📥 Commits

Reviewing files that changed from the base of the PR and between 9d9e004 and ce0208b.

📒 Files selected for processing (3)
  • cli/cmd/config_test.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_download_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: CLI Test (windows-latest)
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/cmd/config_test.go
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_download_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
📚 Learning: 2026-03-24T11:40:22.911Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.911Z
Learning: Applies to cli/**/*.go : Use Go 1.26+ with Cobra CLI framework; run `go -C cli` from repo root (never `cd cli`), run tests with `-n auto` parallelism, use native `testing.F` for fuzz tests

Applied to files:

  • cli/cmd/config_test.go
📚 Learning: 2026-03-15T18:17:43.675Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:17:43.675Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).

Applied to files:

  • cli/cmd/config_test.go
  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: Applies to go.mod : Maintain Go 1.26+ requirement. Dependencies: Cobra (CLI framework), charmbracelet/huh and charmbracelet/lipgloss (UI), sigstore-go (code signing), go-containerregistry (container image verification), go-tuf (TUF client for Sigstore).

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.

Applied to files:

  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_download_test.go
🧬 Code graph analysis (2)
cli/cmd/config_test.go (3)
cli/internal/config/state.go (2)
  • DefaultState (36-48)
  • Save (230-244)
cli/internal/config/paths.go (1)
  • DataDir (18-31)
cli/cmd/root.go (1)
  • Execute (101-107)
cli/internal/selfupdate/updater_download_test.go (1)
cli/internal/selfupdate/updater.go (5)
  • ReplaceAt (460-494)
  • Release (67-70)
  • Asset (73-76)
  • Download (409-447)
  • CheckFromURL (293-317)
🔇 Additional comments (4)
cli/cmd/config_test.go (4)

389-443: Strong table-driven coverage for config get happy paths.

Lines 389-443 add clear key/value assertions across multiple display-safe fields, and the subtest cleanup reduces shared rootCmd state bleed between cases.


445-466: Unknown-key negative path is correctly locked in.

Lines 445-466 correctly assert rootCmd.Execute() fails for unsupported keys, which protects the allowlist contract.


468-493: Explicit secret-key rejection test is a good safety guard.

Lines 468-493 correctly verify jwt_secret and settings_key remain unreadable via config get, preventing accidental exposure regressions.


495-530: Default-channel fallback test now exercises the intended path.

Lines 495-530 properly seed config.json without channel and confirm config get channel returns stable, validating unmarshal-onto-default behavior.

Comment on lines +289 to +355
func TestCheckWithMockServer(t *testing.T) {
release := Release{
TagName: "v1.0.0",
Assets: []Asset{
{Name: assetName(), BrowserDownloadURL: "https://example.com/asset"},
{Name: "checksums.txt", BrowserDownloadURL: "https://example.com/checksums"},
},
}
body, err := json.Marshal(release)
if err != nil {
t.Fatal(err)
}

srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
if _, err := w.Write(body); err != nil {
t.Logf("write error: %v", err)
}
}))
defer srv.Close()

t.Run("parse release JSON", func(t *testing.T) {
client := &http.Client{}
data, err := httpGetWithClient(context.Background(), client, srv.URL, maxAPIResponseBytes)
if err != nil {
t.Fatalf("httpGetWithClient: %v", err)
}
var r Release
if err := json.Unmarshal(data, &r); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if r.TagName != "v1.0.0" {
t.Errorf("TagName = %q, want v1.0.0", r.TagName)
}
if len(r.Assets) != 2 {
t.Errorf("len(Assets) = %d, want 2", len(r.Assets))
}
})

t.Run("asset matching", func(t *testing.T) {
client := &http.Client{}
data, err := httpGetWithClient(context.Background(), client, srv.URL, maxAPIResponseBytes)
if err != nil {
t.Fatal(err)
}
var r Release
if err := json.Unmarshal(data, &r); err != nil {
t.Fatal(err)
}

var assetURL, checksumURL string
for _, a := range r.Assets {
if a.Name == assetName() {
assetURL = a.BrowserDownloadURL
}
if a.Name == "checksums.txt" {
checksumURL = a.BrowserDownloadURL
}
}
if assetURL == "" {
t.Error("asset URL not found")
}
if checksumURL == "" {
t.Error("checksum URL not found")
}
})
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Allowlist each httptest hostname before calling httpGetWithClient or Download.

httpGetWithClient and Download now reject final hosts outside AllowedDownloadHosts. These cases all hit srv.URL, so they will fail against 127.0.0.1/::1 unless the test injects that hostname first; otherwise they only pass if some other test mutates the global allowlist beforehand.

🧪 Suggested fix
+import "net/url"
+
+func allowTestHost(t *testing.T, rawURL string) {
+	t.Helper()
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	host := u.Hostname()
+	prev, had := AllowedDownloadHosts[host]
+	AllowedDownloadHosts[host] = true
+	t.Cleanup(func() {
+		if had {
+			AllowedDownloadHosts[host] = prev
+		} else {
+			delete(AllowedDownloadHosts, host)
+		}
+	})
+}
+
 func TestCheckWithMockServer(t *testing.T) {
 	...
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		...
 	}))
 	defer srv.Close()
+	allowTestHost(t, srv.URL)
// Apply the same helper in the other httptest-backed cases that call
// httpGetWithClient or Download.

Also applies to: 357-444, 626-690

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/internal/selfupdate/updater_download_test.go` around lines 289 - 355, The
tests use an httptest.Server (srv) and call httpGetWithClient (and Download
elsewhere) which now validate hosts against AllowedDownloadHosts; to fix, before
calling httpGetWithClient or Download in TestCheckWithMockServer (and the other
httptest-backed tests), extract the hostname from srv.URL and add it to
AllowedDownloadHosts (or call the existing helper that does this) so the test
host (127.0.0.1/::1) is allowlisted; update the blocks in the "parse release
JSON" and "asset matching" subtests (and the other referenced test ranges) to
perform this allowlist injection using the same helper/assertion pattern to
ensure calls to httpGetWithClient/Download succeed.

- Reject HTTPS-to-HTTP downgrades in checkRedirectHost by validating
  req.URL.Scheme before checking the hostname allowlist
- Fix malformed dev tag poisoning in selectBestRelease: verify splitDev
  returns a valid dev number (>= 0) for prerelease candidates, not
  just compareWithDev self-comparison (which returns nil error for
  malformed suffixes like "dev.NaN")
- Add TestCheckDevFromURLMalformedFirstTag regression test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 14:14 — with GitHub Actions Inactive
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/internal/selfupdate/updater_test.go`:
- Around line 198-213: In TestCheckDevFromURLAllDrafts, the test server response
omits the Content-Type header; update the httptest server handler used by srv so
it sets w.Header().Set("Content-Type", "application/json") before writing the
JSON body, keeping the rest of the test (releases, body, and call to
CheckDevFromURL) unchanged to match other CheckDevFromURL tests and ensure
consistency.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 190d5fae-c95e-4afd-8e92-989ac5a53536

📥 Commits

Reviewing files that changed from the base of the PR and between ce0208b and 811fd3c.

📒 Files selected for processing (2)
  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

cli/**/*.go: Use Go 1.26+ with Cobra CLI framework; run go -C cli from repo root (never cd cli), run tests with -n auto parallelism, use native testing.F for fuzz tests
Run linting with go -C cli tool golangci-lint run, vetting with go -C cli vet, and tests with go -C cli test ./... (without -fuzz for standard test runs)

Files:

  • cli/internal/selfupdate/updater.go
  • cli/internal/selfupdate/updater_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T11:40:22.912Z
Learning: Version management: `pyproject.toml` (`[tool.commitizen].version`), `src/synthorg/__init__.py` (`__version__`); Release Please automates version bumping (patch for fix:/docs:, minor for feat!/BREAKING); `Release-As` trailer in PR body as final paragraph for manual override
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-15T21:32:02.880Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:32:02.880Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow: Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile: linux/darwin/windows × amd64/arm64) + govulncheck + fuzz testing (main-only, 30s/target, continue-on-error, matrix over 4 packages). cli-pass gate includes fuzz as informational. GoReleaser release on v* tags. Cosign keyless signing of checksums.txt. SLSA L3 provenance attestations. Sigstore bundle (.sigstore.json) attached. Post-release appends checksums/verification/provenance to draft release notes.

Applied to files:

  • cli/internal/selfupdate/updater.go
📚 Learning: 2026-03-19T11:19:40.044Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:19:40.044Z
Learning: Applies to go.mod : Maintain Go 1.26+ requirement. Dependencies: Cobra (CLI framework), charmbracelet/huh and charmbracelet/lipgloss (UI), sigstore-go (code signing), go-containerregistry (container image verification), go-tuf (TUF client for Sigstore).

Applied to files:

  • cli/internal/selfupdate/updater_test.go
🧬 Code graph analysis (1)
cli/internal/selfupdate/updater_test.go (1)
cli/internal/selfupdate/updater.go (2)
  • Asset (76-79)
  • CheckDevFromURL (123-156)
🔇 Additional comments (8)
cli/internal/selfupdate/updater.go (5)

46-67: HTTPS downgrade protection and per-hop redirect validation are correctly implemented.

The checkRedirectHost function now validates both the scheme (must be https) and hostname (must be in AllowedDownloadHosts) before each redirect hop. Integrating this into both apiClient and the download client ensures consistent protection across all HTTP operations.


163-196: Tag validation prevents malformed baselines from poisoning release selection.

The two-tier validation approach is sound:

  1. Line 173-175: Self-comparison via compareWithDev catches fundamentally unparseable tags.
  2. Line 182-184: Explicit devNum < 0 check rejects dev-like tags with non-numeric suffixes (e.g., v0.5.0-dev.NaN).

This correctly addresses the previously identified issue where a malformed first tag could become the baseline and suppress valid candidates.


283-300: splitDev now returns clean base for malformed suffixes, enabling safe comparison.

Returning (-1, base) instead of (-1, originalTag) for malformed -dev. suffixes ensures compareSemver receives a valid version string. This prevents parse failures when comparing against well-formed tags.


488-505: Rollback error handling now properly surfaces failures and preserves recovery artifacts.

When os.Rename(tmpPath, execPath) fails on Windows after moving the original aside:

  • If rollback succeeds: cleanup proceeds normally.
  • If rollback fails: tmpPath is preserved and error includes both the original failure and rollback failure with errors.Join, plus the oldPath location for manual recovery.

This correctly addresses the previously identified issue.


507-559: Helper function extraction improves readability while maintaining correct cleanup semantics.

Both writeTempBinary and windowsPreReplace consistently clean up tmpPath on failure paths, and the unique-path pattern in windowsPreReplace (CreateTemp → Close → Remove → Rename) is correct.

cli/internal/selfupdate/updater_test.go (3)

18-23: Appropriate test setup for redirect validation with httptest servers.

Extending AllowedDownloadHosts in TestMain enables the redirect validation to work with local test servers while keeping production hosts restricted.


140-244: Test coverage validates key selection behaviors including the malformed-tag fix.

The tests comprehensively cover:

  • Dev selected when newer than stable (TestCheckDevFromURL)
  • Stable preferred at equal base version (TestCheckDevFromURLPrefersStable)
  • Error when all releases are drafts (TestCheckDevFromURLAllDrafts)
  • Malformed dev tags skipped in favor of valid ones (TestCheckDevFromURLMalformedFirstTag)

The TestCheckDevFromURLMalformedFirstTag test directly validates the regression fix for the splitDev change.


246-332: Out-of-order tests validate the core PR fix: version comparison over API ordering.

TestCheckDevFromURLOutOfOrder and TestCheckDevFromURLOutOfOrderStable directly validate that selectBestRelease compares all candidates by semantic version rather than relying on GitHub API list position—the primary bug this PR addresses.

Consistency fix: all other CheckDevFromURL test handlers set
Content-Type: application/json; this one was missing it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 15:07 — with GitHub Actions Inactive
@Aureliolo Aureliolo merged commit 6b137f0 into main Mar 24, 2026
41 checks passed
@Aureliolo Aureliolo deleted the feat/improve-update-logic branch March 24, 2026 15:22
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 24, 2026 15:22 — with GitHub Actions Inactive
Aureliolo added a commit that referenced this pull request Mar 30, 2026
🤖 I have created a release *beep* *boop*
---
#MAJOR CHANGES; We got a somewhat working webui :)

##
[0.5.0](v0.4.9...v0.5.0)
(2026-03-30)


### Features

* add analytics trends and budget forecast API endpoints
([#798](#798))
([16b61f5](16b61f5))
* add department policies to default templates
([#852](#852))
([7a41548](7a41548))
* add remaining activity event types (task_started, tool_used,
delegation, cost_incurred)
([#832](#832))
([4252fac](4252fac))
* agent performance, activity, and history API endpoints
([#811](#811))
([9b75c1d](9b75c1d))
* Agent Profiles and Detail pages (biography, career, performance)
([#874](#874))
([62d7880](62d7880))
* app shell, Storybook, and CI/CD pipeline
([#819](#819))
([d4dde90](d4dde90))
* Approvals page with risk grouping, urgency indicators, batch actions
([#889](#889))
([4e9673d](4e9673d))
* Budget Panel page (P&L dashboard, breakdown charts, forecast)
([#890](#890))
([b63b0f1](b63b0f1))
* build infrastructure layer (API client, auth, WebSocket)
([#815](#815))
([9f01d3e](9f01d3e))
* CLI global options infrastructure, UI modes, exit codes, env vars
([#891](#891))
([fef4fc5](fef4fc5))
* CodeMirror editor and theme preferences toggle
([#905](#905),
[#807](#807))
([#909](#909))
([41fbedc](41fbedc))
* Company page (department/agent management)
([#888](#888))
([cfb88b0](cfb88b0))
* comprehensive hint coverage across all CLI commands
([#900](#900))
([937974e](937974e))
* config system extensions, per-command flags for
init/start/stop/status/logs
([#895](#895))
([32f83fe](32f83fe))
* configurable currency system replacing hardcoded USD
([#854](#854))
([b372551](b372551))
* Dashboard page (metric cards, activity feed, budget burn)
([#861](#861))
([7d519d5](7d519d5))
* department health, provider status, and activity feed endpoints
([#818](#818))
([6d5f196](6d5f196))
* design tokens and core UI components
([#833](#833))
([ed887f2](ed887f2))
* extend approval, meeting, and budget API responses
([#834](#834))
([31472bf](31472bf))
* frontend polish -- real-time UX, accessibility, responsive,
performance ([#790](#790),
[#792](#792),
[#791](#791),
[#793](#793))
([#917](#917))
([f04a537](f04a537))
* implement human roles and access control levels
([#856](#856))
([d6d8a06](d6d8a06))
* implement semantic conflict detection in workspace merge
([#860](#860))
([d97283b](d97283b))
* interaction components and animation patterns
([#853](#853))
([82d4b01](82d4b01))
* Login page + first-run bootstrap + Company page
([#789](#789),
[#888](#888))
([#896](#896))
([8758e8d](8758e8d))
* Meetings page with timeline viz, token bars, contribution formatting
([#788](#788))
([#904](#904))
([b207f46](b207f46))
* Messages page with threading, channel badges, sender indicators
([#787](#787))
([#903](#903))
([28293ad](28293ad))
* Org Chart force-directed view and drag-drop reassignment
([#872](#872),
[#873](#873))
([#912](#912))
([a68a938](a68a938))
* Org Chart page (living nodes, status, CRUD, department health)
([#870](#870))
([0acbdae](0acbdae))
* per-command flags for remaining commands, auto-behavior wiring,
help/discoverability
([#897](#897))
([3f7afa2](3f7afa2))
* Providers page with backend rework -- health, CRUD, subscription auth
([#893](#893))
([9f8dd98](9f8dd98))
* scaffold React + Vite + TypeScript + Tailwind project
([#799](#799))
([bd151aa](bd151aa))
* Settings page with search, dependency indicators, grouped rendering
([#784](#784))
([#902](#902))
([a7b9870](a7b9870))
* Setup Wizard rebuild with template comparison, cost estimator, theme
customization ([#879](#879))
([ae8b50b](ae8b50b))
* setup wizard UX -- template filters, card metadata, provider form
reuse ([#910](#910))
([7f04676](7f04676))
* setup wizard UX overhaul -- mode choice, step reorder, provider fixes
([#907](#907))
([ee964c4](ee964c4))
* structured ModelRequirement in template agent configs
([#795](#795))
([7433548](7433548))
* Task Board page (rich Kanban, filtering, dependency viz)
([#871](#871))
([04a19b0](04a19b0))


### Bug Fixes

* align frontend types with backend and debounce WS refetches
([#916](#916))
([134c11b](134c11b))
* auto-cleanup targets newly pulled images instead of old ones
([#884](#884))
([50e6591](50e6591))
* correct wipe backup-skip flow and harden error handling
([#808](#808))
([c05860f](c05860f))
* improve provider setup in wizard, subscription auth, dashboard bugs
([#914](#914))
([87bf8e6](87bf8e6))
* improve update channel detection and add config get command
([#814](#814))
([6b137f0](6b137f0))
* resolve all ESLint warnings, add zero-warnings enforcement
([#899](#899))
([079b46a](079b46a))
* subscription auth uses api_key, base URL optional for cloud providers
([#915](#915))
([f0098dd](f0098dd))


### Refactoring

* semantic analyzer cleanup -- shared filtering, concurrency, extraction
([#908](#908))
([81372bf](81372bf))


### Documentation

* brand identity and UX design system from
[#765](#765) exploration
([#804](#804))
([389a9f4](389a9f4))
* page structure and information architecture for v0.5.0 dashboard
([#809](#809))
([f8d6d4a](f8d6d4a))
* write UX design guidelines with WCAG-verified color system
([#816](#816))
([4a4594e](4a4594e))


### Tests

* add unit tests for agent hooks and page components
([#875](#875))
([#901](#901))
([1d81546](1d81546))


### CI/CD

* bump actions/deploy-pages from 4.0.5 to 5.0.0 in the major group
([#831](#831))
([01c19de](01c19de))
* bump astral-sh/setup-uv from 7.6.0 to 8.0.0 in
/.github/actions/setup-python-uv in the all group
([#920](#920))
([5f6ba54](5f6ba54))
* bump codecov/codecov-action from 5.5.3 to 6.0.0 in the major group
([#868](#868))
([f22a181](f22a181))
* bump github/codeql-action from 4.34.1 to 4.35.0 in the all group
([#883](#883))
([87a4890](87a4890))
* bump sigstore/cosign-installer from 4.1.0 to 4.1.1 in the
minor-and-patch group
([#830](#830))
([7a69050](7a69050))
* bump the all group with 3 updates
([#923](#923))
([ff27c8e](ff27c8e))
* bump wrangler from 4.76.0 to 4.77.0 in /.github in the minor-and-patch
group ([#822](#822))
([07d43eb](07d43eb))
* bump wrangler from 4.77.0 to 4.78.0 in /.github in the all group
([#882](#882))
([f84118d](f84118d))


### Maintenance

* add design system enforcement hook and component inventory
([#846](#846))
([15abc43](15abc43))
* add dev-only auth bypass for frontend testing
([#885](#885))
([6cdcd8a](6cdcd8a))
* add pre-push rebase check hook
([#855](#855))
([b637a04](b637a04))
* backend hardening -- eviction/size-caps and model validation
([#911](#911))
([81253d9](81253d9))
* bump axios from 1.13.6 to 1.14.0 in /web in the all group across 1
directory ([#922](#922))
([b1b0232](b1b0232))
* bump brace-expansion from 5.0.4 to 5.0.5 in /web
([#862](#862))
([ba4a565](ba4a565))
* bump eslint-plugin-react-refresh from 0.4.26 to 0.5.2 in /web
([#801](#801))
([7574bb5](7574bb5))
* bump faker from 40.11.0 to 40.11.1 in the minor-and-patch group
([#803](#803))
([14d322e](14d322e))
* bump https://github.com/astral-sh/ruff-pre-commit from v0.15.7 to
0.15.8 ([#864](#864))
([f52901e](f52901e))
* bump nginxinc/nginx-unprivileged from `6582a34` to `f99cc61` in
/docker/web in the all group
([#919](#919))
([df85e4f](df85e4f))
* bump nginxinc/nginx-unprivileged from `ccbac1a` to `6582a34` in
/docker/web ([#800](#800))
([f4e9450](f4e9450))
* bump node from `44bcbf4` to `71be405` in /docker/sandbox
([#827](#827))
([91bec67](91bec67))
* bump node from `5209bca` to `cf38e1f` in /docker/web
([#863](#863))
([66d6043](66d6043))
* bump picomatch in /site
([#842](#842))
([5f20bcc](5f20bcc))
* bump recharts 2-&gt;3 and @types/node 22-&gt;25 in /web
([#802](#802))
([a908800](a908800))
* Bump requests from 2.32.5 to 2.33.0
([#843](#843))
([41daf69](41daf69))
* bump smol-toml from 1.6.0 to 1.6.1 in /site
([#826](#826))
([3e5dbe4](3e5dbe4))
* bump the all group with 3 updates
([#921](#921))
([7bace0b](7bace0b))
* bump the minor-and-patch group across 1 directory with 2 updates
([#829](#829))
([93e611f](93e611f))
* bump the minor-and-patch group across 1 directory with 3 updates
([#841](#841))
([7010c8e](7010c8e))
* bump the minor-and-patch group across 1 directory with 3 updates
([#869](#869))
([548cee5](548cee5))
* bump the minor-and-patch group in /site with 2 updates
([#865](#865))
([9558101](9558101))
* bump the minor-and-patch group with 2 updates
([#867](#867))
([4830706](4830706))
* consolidate Dependabot groups to 1 PR per ecosystem
([06d2556](06d2556))
* consolidate Dependabot groups to 1 PR per ecosystem
([#881](#881))
([06d2556](06d2556))
* improve worktree skill with full dep sync and status enhancements
([#906](#906))
([772c625](772c625))
* remove Vue remnants and document framework decision
([#851](#851))
([bf2adf6](bf2adf6))
* update web dependencies and fix brace-expansion CVE
([#880](#880))
([a7a0ed6](a7a0ed6))
* upgrade to Storybook 10 and TypeScript 6
([#845](#845))
([52d95f2](52d95f2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants