fix(cli): completion cleanup on uninstall + init backend selection#484
Conversation
The uninstall command removes containers, data, and the binary, but leaves orphaned completion snippets in shell profiles (.bashrc, .zshrc, PS profile). These error on every terminal open after the binary is gone. Add completion.Uninstall() that reverses Install() — removes marker blocks from profiles and deletes generated completion files (zsh, fish). Call it from runUninstall before binary removal.
Add persistence backend (SQLite) and memory backend (Mem0) selection prompts to `synthorg init`. Choices are persisted in config.json and passed as SYNTHORG_PERSISTENCE_BACKEND and SYNTHORG_MEMORY_BACKEND env vars in the generated compose.yml. Currently only one option per backend (SQLite, Mem0) — the UI is ready for future additions (PostgreSQL, alternative memory backends).
Pre-reviewed by 4 agents (go-reviewer, security-reviewer, conventions-enforcer, docs-consistency), 14 findings addressed: - Preserve original file permissions in removeMarkerBlock (was hardcoding 0o644, could weaken user's profile permissions) - Only remove first marker block to prevent greedy deletion - Add allowlist validation for PersistenceBackend/MemoryBackend in validateParams and State.validate() - Replace single-option Select with Note widget (no false choice) - Move completion removal after data dir confirmation - Extract rejectUnsafeDir and removeDataDir from confirmAndRemoveData (was 67 lines, now under 50) - Add removeMarkerBlock tests (7 cases + Unix permissions) - Add install/uninstall round-trip tests (Bash, Zsh, Fish) - Add new fields to state round-trip and default tests - Update docker/.env.example and docs/user_guide.md with new env vars - Fix removeMarkerBlock doc comment for multi-line blocks
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds PersistenceBackend and MemoryBackend to persisted State and threads them from interactive init through compose Params and template into SYNTHORG_PERSISTENCE_BACKEND / SYNTHORG_MEMORY_BACKEND env vars; implements shell completion uninstall and integrates it into the uninstall command; tests and validation updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CLI as CLI Init
participant State as Config State
participant Compose as Compose Generator
participant Docker as Docker Compose
rect rgba(200,200,255,0.5)
User->>CLI: run init (interactive)
CLI->>State: read DefaultState()
CLI->>User: prompt backend choices
User->>CLI: select persistence & memory
CLI->>State: build & persist State (includes PersistenceBackend, MemoryBackend)
end
rect rgba(200,255,200,0.5)
CLI->>Compose: ParamsFromState(State)
Compose->>Compose: validateParams (persistence/memory)
Compose->>Docker: render compose.yml (inject env vars)
Docker->>User: compose includes SYNTHORG_PERSISTENCE_BACKEND & SYNTHORG_MEMORY_BACKEND
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
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 enhances the CLI's lifecycle management by introducing robust uninstallation for shell completions and laying the groundwork for configurable persistence and memory backends. It improves the user experience by ensuring a cleaner removal process and prepares the application for future extensibility in data storage options, all while maintaining strong validation and code quality. Highlights
Changelog
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
|
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable cleanup functionality for shell completions during uninstallation and prepares the init command for future backend options. The refactoring of the uninstall logic into smaller, more focused functions significantly improves code clarity and maintainability. The addition of comprehensive round-trip tests for installation and uninstallation is also a great enhancement. I've identified a couple of areas for improvement regarding code duplication and validation logic for the new backend configuration settings.
| if s.PersistenceBackend != "" && !validPersistenceBackends[s.PersistenceBackend] { | ||
| return fmt.Errorf("invalid persistence_backend %q: must be one of sqlite", s.PersistenceBackend) | ||
| } | ||
| if s.MemoryBackend != "" && !validMemoryBackends[s.MemoryBackend] { | ||
| return fmt.Errorf("invalid memory_backend %q: must be one of mem0", s.MemoryBackend) | ||
| } |
There was a problem hiding this comment.
The validation logic here allows an empty string ("") for PersistenceBackend and MemoryBackend by checking s.PersistenceBackend != "". However, an empty string is not a valid backend and is rejected later during compose file generation. This check should be removed to enforce that the backend value is always one of the allowed options.
On a related note, the error message hardcodes the list of allowed values (e.g., "sqlite"). This could become outdated. Consider generating this list dynamically from the validation map for better maintainability.
| if s.PersistenceBackend != "" && !validPersistenceBackends[s.PersistenceBackend] { | |
| return fmt.Errorf("invalid persistence_backend %q: must be one of sqlite", s.PersistenceBackend) | |
| } | |
| if s.MemoryBackend != "" && !validMemoryBackends[s.MemoryBackend] { | |
| return fmt.Errorf("invalid memory_backend %q: must be one of mem0", s.MemoryBackend) | |
| } | |
| if !validPersistenceBackends[s.PersistenceBackend] { | |
| return fmt.Errorf("invalid persistence_backend %q: must be one of sqlite", s.PersistenceBackend) | |
| } | |
| if !validMemoryBackends[s.MemoryBackend] { | |
| return fmt.Errorf("invalid memory_backend %q: must be one of mem0", s.MemoryBackend) | |
| } |
| var validPersistenceBackends = map[string]bool{"sqlite": true} | ||
|
|
||
| // Valid memory backend values. | ||
| var validMemoryBackends = map[string]bool{"mem0": true} |
There was a problem hiding this comment.
These validation maps are duplicated in cli/internal/compose/generate.go. To maintain a single source of truth, you should export these maps from the config package (e.g., ValidPersistenceBackends), and then remove the duplicates in compose/generate.go and use the exported versions there.
| var validPersistenceBackends = map[string]bool{"sqlite": true} | |
| // Valid memory backend values. | |
| var validMemoryBackends = map[string]bool{"mem0": true} | |
| // ValidPersistenceBackends are valid persistence backend values. | |
| var ValidPersistenceBackends = map[string]bool{"sqlite": true} | |
| // ValidMemoryBackends are valid memory backend values. | |
| var ValidMemoryBackends = map[string]bool{"mem0": true} |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/internal/config/state_test.go (1)
39-82: 🧹 Nitpick | 🔵 TrivialAdd a negative test for invalid/empty backend values.
Given the new validation paths, please add a case that confirms
Load()rejects invalid backend strings (and empty strings, if treated as invalid) to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/config/state_test.go` around lines 39 - 82, Add a negative test that verifies Load rejects invalid backend values: create a tmp dir, construct a State with invalid PersistenceBackend (e.g., "invalid-backend") and another case with empty MemoryBackend (""), call Save(state) to write the file, then call Load(tmp) and assert it returns a non-nil error (use t.Fatalf/t.Errorf) for each case; reference the State struct and the Load and Save functions and check both PersistenceBackend and MemoryBackend validation paths.cli/internal/compose/generate_test.go (1)
28-39: 🧹 Nitpick | 🔵 TrivialAdd explicit assertions for backend env vars in generated YAML.
Golden files validate this implicitly, but direct
assertContainschecks would make regressions easier to diagnose.✅ Suggested assertions
func TestGenerateDefault(t *testing.T) { @@ assertContains(t, yaml, "synthorg-data:") + assertContains(t, yaml, "SYNTHORG_PERSISTENCE_BACKEND") + assertContains(t, yaml, "sqlite") + assertContains(t, yaml, "SYNTHORG_MEMORY_BACKEND") + assertContains(t, yaml, "mem0") @@ func TestGenerateCustomPorts(t *testing.T) { @@ assertContains(t, yaml, "SYNTHORG_JWT_SECRET") assertContains(t, yaml, "test-secret-value") + assertContains(t, yaml, "SYNTHORG_PERSISTENCE_BACKEND") + assertContains(t, yaml, "sqlite") + assertContains(t, yaml, "SYNTHORG_MEMORY_BACKEND") + assertContains(t, yaml, "mem0")Also applies to: 70-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/compose/generate_test.go` around lines 28 - 39, Add explicit assertContains checks to the test that verify the backend service environment entries are present in the generated YAML: locate the existing assertions around the "yaml" variable and the "synthorg-backend" image and add assertions that the "environment:" block and each backend env var key emitted by the generator are present (use assertContains(t, yaml, "<ENV_NAME>") for each env var key as defined in the generator for synthorg-backend); apply the same pattern for the web service assertions noted around lines 70-75 so both backend and web env keys are explicitly asserted.
🤖 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/init.go`:
- Around line 133-136: Replace the hardcoded backend labels inside the
huh.NewNote().Description call with values drawn from
defaults.PersistenceBackend and defaults.MemoryBackend so the UI always reflects
config defaults; update the huh.NewNote().Title("Backends").Description(...)
invocation to build the description string (e.g. using fmt.Sprintf("Persistence:
%s · Memory: %s", defaults.PersistenceBackend, defaults.MemoryBackend)) and add
the defaults import if missing, keeping the same Title and grouping with
huh.NewGroup.
In `@cli/cmd/uninstall.go`:
- Around line 138-159: The rejectUnsafeDir guard must reject non-absolute and
drive-relative paths: ensure you Clean the input and then return an error if
filepath.IsAbs(dir) is false or dir == "." or dir == "" (to block relative paths
like "."), and on Windows also reject drive-relative paths like "C:" (detect
where filepath.VolumeName(dir) yields a drive with no separator, e.g. length 2
drive strings with no trailing slash). Update the logic in rejectUnsafeDir to
perform the IsAbs check early, add explicit checks for "."/empty, and add a
Windows-specific check for drive-only paths (e.g., VolumeName indicates a drive
but dir does not include a path separator) so that such inputs are refused
before any os.RemoveAll call.
- Around line 63-71: The uninstall currently only removes completions for
completion.DetectShell(), which leaves stale files for other shells; modify the
logic to iterate over all supported shells (e.g., the set used by completion
package) and call completion.Uninstall(ctx, shell) for each non-Unknown shell,
printing per-shell progress/warnings (replace the single DetectShell() call with
a loop through supported shells and keep the existing error handling that
formats warnings to cmd.ErrOrStderr()); ensure you still skip completion.Unknown
and preserve context and messages like "Removing shell completions..." or change
to per-shell wording.
In `@cli/internal/completion/install.go`:
- Around line 334-381: removeMarkerBlock currently discards any contiguous
non-empty lines after the marker which can delete user config; change the
removal logic in removeMarkerBlock so it only removes lines that match the known
injected snippet pattern instead of any non-empty line: build the expected
snippet lines (or a set of recognizable prefixes) that your installer writes
and, when inBlock, only continue skipping while each line equals one of those
expected snippet lines (or matches a prefix/regexp); stop the block removal as
soon as a non-matching non-empty line is seen (but still consume the marker line
itself), preserve the terminating blank line if present, and keep using
info.Mode() when writing the file.
In `@cli/internal/config/state.go`:
- Around line 90-95: The duplicated allowlists validPersistenceBackends and
validMemoryBackends should be centralized: create exported constants or
variables (e.g., ValidPersistenceBackends, ValidMemoryBackends) in the shared
config package and replace the local maps in state.go and compose/generate.go to
reference those identifiers; update imports to use the config package and remove
the duplicate map literals so both state.go's validPersistenceBackends and the
maps in generate.go use the single source of truth.
- Around line 104-109: The validation currently allows empty backend values
causing Load() to succeed but later fail in compose generation; update the
checks for s.PersistenceBackend and s.MemoryBackend in the Load/validation logic
to reject empty strings as invalid (i.e. treat "" as an error) by changing the
conditions from only checking validPersistenceBackends/validMemoryBackends to
also check for empty values and return a clear fmt.Errorf referencing the field
(s.PersistenceBackend and s.MemoryBackend) and the allowed options from
validPersistenceBackends/validMemoryBackends so failures surface immediately.
---
Outside diff comments:
In `@cli/internal/compose/generate_test.go`:
- Around line 28-39: Add explicit assertContains checks to the test that verify
the backend service environment entries are present in the generated YAML:
locate the existing assertions around the "yaml" variable and the
"synthorg-backend" image and add assertions that the "environment:" block and
each backend env var key emitted by the generator are present (use
assertContains(t, yaml, "<ENV_NAME>") for each env var key as defined in the
generator for synthorg-backend); apply the same pattern for the web service
assertions noted around lines 70-75 so both backend and web env keys are
explicitly asserted.
In `@cli/internal/config/state_test.go`:
- Around line 39-82: Add a negative test that verifies Load rejects invalid
backend values: create a tmp dir, construct a State with invalid
PersistenceBackend (e.g., "invalid-backend") and another case with empty
MemoryBackend (""), call Save(state) to write the file, then call Load(tmp) and
assert it returns a non-nil error (use t.Fatalf/t.Errorf) for each case;
reference the State struct and the Load and Save functions and check both
PersistenceBackend and MemoryBackend validation paths.
🪄 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: 77a5970a-716b-4c59-b9a1-4b17be745fc0
⛔ Files ignored due to path filters (1)
cli/coverage.outis excluded by!**/*.out
📒 Files selected for processing (13)
cli/cmd/init.gocli/cmd/uninstall.gocli/internal/completion/install.gocli/internal/completion/install_test.gocli/internal/compose/compose.yml.tmplcli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/state_test.gocli/testdata/compose_custom_ports.ymlcli/testdata/compose_default.ymldocker/.env.exampledocs/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). (4)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Build Web
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/config/state_test.gocli/cmd/uninstall.gocli/internal/compose/generate_test.gocli/internal/completion/install.gocli/internal/config/state.gocli/internal/completion/install_test.gocli/cmd/init.gocli/internal/compose/generate.go
🧠 Learnings (1)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/uninstall.go
🧬 Code graph analysis (5)
cli/internal/config/state_test.go (1)
cli/internal/config/state.go (1)
State(14-25)
cli/cmd/uninstall.go (1)
cli/internal/completion/install.go (3)
DetectShell(50-69)Unknown(23-23)Uninstall(277-290)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (1)
DataDir(18-31)
cli/cmd/init.go (2)
cli/internal/config/state.go (1)
DefaultState(30-41)cli/internal/config/paths.go (1)
DataDir(18-31)
cli/internal/compose/generate.go (2)
cli/internal/config/state.go (1)
State(14-25)cli/internal/version/version.go (1)
Version(9-9)
🔇 Additional comments (13)
docker/.env.example (1)
23-27: Good env example update for new backend knobs.The added variables and defaults are clear and consistent with the new backend selection flow.
cli/internal/config/state.go (1)
30-40: Please attach race/vulnerability check evidence for these CLI Go changes.I only see explicit mention of
go test,go vet, andgolangci-lint; please also confirmgo test -raceandgovulncheckwere run for this PR.As per coding guidelines, "Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck".
cli/testdata/compose_custom_ports.yml (1)
16-17: Golden fixture update looks correct.These environment entries correctly reflect the new compose backend parameters.
cli/testdata/compose_default.yml (1)
16-17: Default compose fixture is aligned with backend defaults.The added variables match the expected default output.
docs/user_guide.md (1)
54-55: Docs update is accurate and well-placed.The new variables are documented in the right section with clear defaults.
cli/internal/compose/compose.yml.tmpl (1)
16-17: Template wiring is correct and safely quoted.Using
yamlStrfor both new backend fields keeps rendering behavior consistent and safe.cli/internal/compose/generate.go (1)
46-47: Backend fields are correctly propagated and validated.The new params mapping and allowlist checks close the loop from config state to compose generation.
Also applies to: 61-62, 113-118
cli/cmd/init.go (2)
84-92: Backend fields are cleanly wired end-to-end.
setupAnswersdefaults andbuildStatepropagation forPersistenceBackend/MemoryBackendare consistent and align with state defaults.Also applies to: 98-106, 184-193
84-193: These tests are already automated in CI and do not require manual verification or attachment.The
.github/workflows/cli.ymlCI workflow already enforces bothgo test -raceandgovulncheckas mandatory checks for all PRs modifyingcli/**files:
- cli-test job (runs on all platforms):
go test -race -coverprofile=coverage.out -covermode=atomic ./...- cli-vuln job: Installs and runs
govulncheck ./...Both jobs are required to pass before a PR can merge (enforced by the
cli-passgate). No manual verification or attached evidence is needed—the CI status checks on the PR confirm these have already run.cli/internal/compose/generate_test.go (1)
14-20: Good test propagation for new backend params.The additions consistently cover direct
Paramsconstruction andParamsFromStatemapping, including explicit equality checks.Also applies to: 55-63, 81-90, 104-110, 134-143, 162-167
cli/internal/completion/install_test.go (1)
205-407: Strong uninstall test coverage added.The new table-driven marker-removal tests and Bash/Zsh/Fish uninstall round-trip tests are solid and cover key edge cases.
cli/internal/completion/install.go (1)
276-332: Uninstall API shape looks good.Per-shell uninstall helpers are clear and keep behavior predictable across shells.
cli/cmd/uninstall.go (1)
29-80: Please provide race/vulnerability check evidence for these CLI changes.The PR summary mentions
go test,go vet, andgolangci-lint, but it doesn’t explicitly showgo test -raceandgovulncheckoutputs forcli/**.As per coding guidelines:
cli/**/*.go: "Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck".
| huh.NewGroup( | ||
| huh.NewNote().Title("Backends"). | ||
| Description("Persistence: SQLite · Memory: Mem0\n(More options coming soon)"), | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid hardcoded backend labels in the Note description.
This text can drift from actual defaults. Build the description from defaults.PersistenceBackend and defaults.MemoryBackend so UI always reflects config defaults.
♻️ Suggested refactor
- huh.NewGroup(
- huh.NewNote().Title("Backends").
- Description("Persistence: SQLite · Memory: Mem0\n(More options coming soon)"),
- ),
+ huh.NewGroup(
+ huh.NewNote().Title("Backends").
+ Description(fmt.Sprintf(
+ "Persistence: %s · Memory: %s\n(More options coming soon)",
+ strings.Title(defaults.PersistenceBackend),
+ strings.Title(defaults.MemoryBackend),
+ )),
+ ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/init.go` around lines 133 - 136, Replace the hardcoded backend labels
inside the huh.NewNote().Description call with values drawn from
defaults.PersistenceBackend and defaults.MemoryBackend so the UI always reflects
config defaults; update the huh.NewNote().Title("Backends").Description(...)
invocation to build the description string (e.g. using fmt.Sprintf("Persistence:
%s · Memory: %s", defaults.PersistenceBackend, defaults.MemoryBackend)) and add
the defaults import if missing, keeping the same Title and grouping with
huh.NewGroup.
- Remove empty-string bypass in backend validation (Gemini, CodeRabbit) - Centralize backend allowlists — export from config, remove duplicates in compose/generate.go (Gemini, CodeRabbit) - Uninstall completions for all shells, not just detected one (CodeRabbit) - Harden rejectUnsafeDir to reject empty, relative, and "." paths (CodeRabbit) - Cap removeMarkerBlock at 5 lines to prevent unbounded deletion (CodeRabbit) - Use dynamic backend names in init Note from defaults (CodeRabbit) - Add negative tests for invalid/empty backend values in Load (CodeRabbit) - Fix pre-existing tests missing new backend fields
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
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/cmd/config_test.go (1)
78-90: 🧹 Nitpick | 🔵 TrivialExtend
config showassertions to include backend fields.Since this fixture now includes
PersistenceBackend/MemoryBackend, add"sqlite"and"mem0"to the expected output checks so regressions in field display are caught.🤖 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 78 - 90, Update the expectation list in the test in cli/cmd/config_test.go (the slice iterated in the loop that checks output) to include the backend strings "sqlite" and "mem0" so the test verifies PersistenceBackend/MemoryBackend are printed; specifically add "sqlite" and "mem0" to the []string literal being checked in that test loop.
♻️ Duplicate comments (1)
cli/internal/completion/install.go (1)
374-386:⚠️ Potential issue | 🟠 Major
removeMarkerBlockcan still delete user profile lines after the marker.On Line 375, any non-empty line after the marker is removed until blank/cap, even if it is not a SynthOrg snippet line. This can still remove user config (bounded, but still data loss).
🔧 Safer removal: skip only known SynthOrg snippet lines
const maxSnippetLines = 5 + +var knownSnippetLines = map[string]struct{}{ + `eval "$(synthorg completion bash)"`: {}, + "fpath=(~/.zsh/completion $fpath)": {}, + "autoload -Uz compinit && compinit": {}, + "synthorg completion powershell | Out-String | Invoke-Expression": {}, +} @@ for _, line := range lines { if !found && strings.TrimSpace(line) == marker { inBlock = true found = true blockLines = 0 continue } if inBlock { - if strings.TrimSpace(line) != "" && blockLines < maxSnippetLines { - blockLines++ - continue - } - // Empty line or cap reached — end the block. - inBlock = false - if strings.TrimSpace(line) == "" { - // Consume the terminating empty line. - continue - } - // Cap reached on a non-empty line — keep it. + trimmed := strings.TrimSpace(line) + if trimmed == "" { + inBlock = false + continue // consume one trailing blank line + } + if blockLines < maxSnippetLines { + if _, ok := knownSnippetLines[trimmed]; ok { + blockLines++ + continue // remove known snippet line + } + // first non-snippet line: stop removal and keep user content + inBlock = false + } else { + // cap reached: stop block removal and keep remaining content + inBlock = false + } } result = append(result, line) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/completion/install.go` around lines 374 - 386, The removal loop in removeMarkerBlock is too greedy: when inBlock it currently deletes any non-empty line until a blank or cap, which can remove user config; change the logic to only consume/skip lines that are known SynthOrg snippet lines (match the snippet marker/pattern used by this tool) and increment blockLines only for those matches, and if a non-empty line does not match the snippet pattern treat that as the end of the block (set inBlock = false) and do not consume/skip that line; use the existing variables (inBlock, blockLines, maxSnippetLines) and add a deterministic check (prefix or regex match against the SynthOrg snippet marker) to identify snippet lines.
🤖 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/config/state.go`:
- Around line 90-95: Replace the exported mutable maps ValidPersistenceBackends
and ValidMemoryBackends with exported immutable slices (e.g.,
PersistenceBackends []string, MemoryBackends []string) and create unexported map
sets (e.g., persistenceBackendSet, memoryBackendSet) initialized once for fast
lookup; add exported helper functions IsValidPersistenceBackend(name string)
bool and IsValidMemoryBackend(name string) bool that consult the unexported sets
so callers cannot mutate validation state at runtime. Ensure any existing code
using the old maps is updated to use the new slices for enumeration or the
IsValid... helpers for validation.
---
Outside diff comments:
In `@cli/cmd/config_test.go`:
- Around line 78-90: Update the expectation list in the test in
cli/cmd/config_test.go (the slice iterated in the loop that checks output) to
include the backend strings "sqlite" and "mem0" so the test verifies
PersistenceBackend/MemoryBackend are printed; specifically add "sqlite" and
"mem0" to the []string literal being checked in that test loop.
---
Duplicate comments:
In `@cli/internal/completion/install.go`:
- Around line 374-386: The removal loop in removeMarkerBlock is too greedy: when
inBlock it currently deletes any non-empty line until a blank or cap, which can
remove user config; change the logic to only consume/skip lines that are known
SynthOrg snippet lines (match the snippet marker/pattern used by this tool) and
increment blockLines only for those matches, and if a non-empty line does not
match the snippet pattern treat that as the end of the block (set inBlock =
false) and do not consume/skip that line; use the existing variables (inBlock,
blockLines, maxSnippetLines) and add a deterministic check (prefix or regex
match against the SynthOrg snippet marker) to identify snippet lines.
🪄 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: 2f364b90-2efc-45ca-aabd-db525808b529
📒 Files selected for processing (7)
cli/cmd/config_test.gocli/cmd/init.gocli/cmd/uninstall.gocli/internal/completion/install.gocli/internal/compose/generate.gocli/internal/config/state.gocli/internal/config/state_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). (4)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/config/state_test.gocli/cmd/config_test.gocli/internal/compose/generate.gocli/cmd/init.gocli/internal/config/state.gocli/internal/completion/install.gocli/cmd/uninstall.go
🧠 Learnings (2)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/uninstall.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/uninstall.go
🧬 Code graph analysis (5)
cli/internal/config/state_test.go (1)
cli/internal/config/state.go (2)
State(14-25)Load(50-88)
cli/internal/compose/generate.go (2)
cli/internal/config/state.go (3)
State(14-25)ValidPersistenceBackends(91-91)ValidMemoryBackends(94-94)cli/internal/version/version.go (1)
Version(9-9)
cli/cmd/init.go (2)
cli/internal/config/state.go (1)
DefaultState(30-41)cli/internal/config/paths.go (1)
DataDir(18-31)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (1)
DataDir(18-31)
cli/cmd/uninstall.go (1)
cli/internal/completion/install.go (6)
ShellType(20-20)Bash(24-24)Zsh(25-25)Fish(26-26)PowerShell(27-27)Uninstall(277-290)
🔇 Additional comments (4)
cli/cmd/uninstall.go (1)
29-81: Please provide race/vulnerability check evidence for this CLI change set.I see
go test,go vet, andgolangci-lintin PR notes, but notgo test -raceandgovulncheck, which are required forcli/**/*.go. Please attach those results for this PR before merge.As per coding guidelines
cli/**/*.go: "Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck".cli/internal/config/state_test.go (1)
165-199: Good negative coverage for backend validation.The table-driven invalid backend cases on Line 165 through Line 199 are solid and directly exercise the new validation path.
cli/internal/compose/generate.go (1)
107-112: Validation wiring looks correct.Using
config.ValidPersistenceBackendsandconfig.ValidMemoryBackendsinvalidateParamskeeps compose validation aligned with config validation.cli/cmd/init.go (1)
186-197: Backend fields are correctly propagated into persisted state.The additions on Line 186 through Line 197 complete the init→state wiring for the new backend settings.
- Replace exported mutable maps with IsValid... helper functions - Add backend fields to config show output + test assertions - Display PersistenceBackend/MemoryBackend in synthorg config show
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/internal/config/state.go`:
- Around line 111-116: The validation currently rejects empty strings but the
error messages in the Load() checks are hardcoded ("must be one of sqlite",
"must be one of mem0"); update the error formatting to derive the allowed option
list from the validation maps instead of hardcoding: create helper functions (or
reuse) that collect keys from validPersistenceBackends and validMemoryBackends
(e.g., validPersistenceBackendNames(), validMemoryBackendNames()) and use them
in the fmt.Errorf calls inside the checks that reference
IsValidPersistenceBackend and IsValidMemoryBackend so the message reads like
`invalid persistence_backend %q: must be one of <generated list>` and similarly
for memory_backend.
🪄 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: 551558bc-6154-49fb-ab3a-61aad3e3c8d9
📒 Files selected for processing (4)
cli/cmd/config.gocli/cmd/config_test.gocli/internal/compose/generate.gocli/internal/config/state.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/compose/generate.gocli/cmd/config.gocli/internal/config/state.gocli/cmd/config_test.go
🧬 Code graph analysis (3)
cli/internal/compose/generate.go (2)
cli/internal/config/state.go (3)
State(14-25)IsValidPersistenceBackend(94-96)IsValidMemoryBackend(99-101)cli/internal/version/version.go (1)
Version(9-9)
cli/cmd/config.go (1)
src/synthorg/persistence/protocol.py (1)
PersistenceBackend(30-188)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (1)
DataDir(18-31)
🔇 Additional comments (5)
cli/cmd/config.go (1)
71-72: LGTM!The new backend fields are displayed consistently with the existing output pattern. Since
stateis already validated duringconfig.Load(), these fields are guaranteed to have valid values by this point.cli/internal/config/state.go (1)
90-101: LGTM! Good refactor addressing previous review feedback.The unexported maps with exported
IsValid*helpers correctly address the mutable map concern from the previous review. This pattern:
- Prevents external mutation of validation state
- Provides a single source of truth for valid backends
- Is reused by
cli/internal/compose/generate.gofor consistent validationcli/internal/compose/generate.go (2)
107-112: LGTM! Validation now uses centralized helpers.Using
config.IsValidPersistenceBackendandconfig.IsValidMemoryBackendensures validation is consistent withstate.goand eliminates the duplication concern from previous reviews.
44-58: LGTM!
ParamsFromStatecorrectly maps the new backend fields fromconfig.StatetoParams. Validation is appropriately deferred tovalidateParamswhich is called byGenerate.cli/cmd/config_test.go (1)
46-92: LGTM! Test coverage for new backend fields.The test correctly:
- Initializes
Statewith valid backend values (sqlite,mem0)- Verifies these values appear in the CLI output
- Maintains the JWT secret leak check
Address CodeRabbit round-3: error messages for invalid backend values now derive the allowed options list from the validation maps instead of hardcoding them. Adds sortedKeys helper and exported name functions.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
🤖 I have created a release *beep* *boop* --- ## [0.2.9](v0.2.8...v0.2.9) (2026-03-16) ### Bug Fixes * **api:** auto-wire persistence backend from SYNTHORG_DB_PATH env var ([#486](#486)) ([7973b07](7973b07)) * **cli:** completion cleanup on uninstall + init backend selection ([#484](#484)) ([97ccb51](97ccb51)) * **docker:** keep sandbox container alive with sleep infinity ([#485](#485)) ([b9f400f](b9f400f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Summary
completion.Uninstall()that reversesInstall()— removes marker blocks from shell profiles and deletes generated completion files (zsh, fish)runUninstallso orphanedsynthorg completion powershell | Out-String | Invoke-Expressionlines don't error after the binary is removedsynthorg initwizard — displayed as a Note for now (single option each), ready for future additionsSYNTHORG_PERSISTENCE_BACKENDandSYNTHORG_MEMORY_BACKENDenv vars in generated compose.ymlvalidateParamsandState.validate()Review coverage
Pre-reviewed by 4 agents (go-reviewer, security-reviewer, conventions-enforcer, docs-consistency), 14 findings addressed:
removeMarkerBlock(was hardcoding 0o644)rejectUnsafeDirandremoveDataDir(function was 67 lines)removeMarkerBlocktests (7 cases + Unix permissions)docker/.env.exampleanddocs/user_guide.mdwith new env varsTest plan
go test ./...— all passinggo vet ./...— cleangolangci-lint run— cleanremoveMarkerBlockunit tests (7 table-driven cases)synthorg initand verify backend Note appearssynthorg uninstalland verify completion removed from PS profile