refactor(cli): close #2067 — lift all path-scoped complexity exclusions in cli/.golangci.yml#2099
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (51)
📜 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). (14)
🧰 Additional context used📓 Path-based instructions (22)cli/**/*.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/**/*.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/**/*_bench_test.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/**/*_bench_test.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/{config,verify}/**/*.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/config/**/*.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/version.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/update.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/new.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/uninstall.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/doctor.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/compose/**/*.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/compose/generate.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/verify/dhi.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/backup.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/config.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/init.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/status.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/start.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/wipe.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/cmd/cleanup.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
cli/internal/config/state.go📄 CodeRabbit inference engine (cli/CLAUDE.md)
Files:
🔇 Additional comments (87)
WalkthroughThis PR centralizes many CLI behaviors: it replaces per-key config switches with dispatch maps, extracts tunable specs and validation into tables and helpers, decomposes large command flows (backup, start, cleanup, wipe, uninstall, update, new) into testable helpers, tightens semver and byte/duration parsing, hardens scaffold writer path/symlink checks, refactors verification/provenance logic, updates UI event handling, and adjusts tests to avoid encrypt_secrets master-key invariants. Linter exclusions were simplified and a workflow condition was made robust. |
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 completes the effort to remove path-scoped complexity exclusions from the CLI's linting configuration. By refactoring complex functions and replacing large switch-case blocks with dispatch tables, the codebase now adheres to strict complexity thresholds across all production packages, improving maintainability and code quality. Highlights
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 the 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 counterproductive. 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. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 is a comprehensive refactoring of the CLI, aimed at reducing function complexity and improving code organization. It introduces map-based dispatchers for configuration handling and extracts numerous helper functions across core commands like init, start, status, and wipe. Key architectural changes include the modularization of parameter validation and the simplification of linter exclusions to enforce complexity rules on all production code. Review feedback identifies a critical typo in update.go where a non-existent errors.AsType function is used, which will cause a compilation failure. Other findings include redundant UI object instantiation and the swallowing of context cancellation errors during image cleanup, which should be propagated to ensure accurate operation status.
| if exitErr, ok := errors.AsType[*exec.ExitError](runErr); ok { | ||
| return &ChildExitError{Code: exitErr.ExitCode()} | ||
| } |
There was a problem hiding this comment.
errors.AsType is not a standard library function and appears to be a typo for the standard errors.As pattern. This will cause a compilation failure unless a custom helper is defined elsewhere (which is not evident in this PR and would be inconsistent with the usage in root.go).
| if exitErr, ok := errors.AsType[*exec.ExitError](runErr); ok { | |
| return &ChildExitError{Code: exitErr.ExitCode()} | |
| } | |
| if exitErr, ok := runErr.(*exec.ExitError); ok { | |
| return &ChildExitError{Code: exitErr.ExitCode()} | |
| } |
| out := ui.NewUIWithOptions(cmd.OutOrStdout(), GetGlobalOpts(ctx).UIOptions()) | ||
| out.Success("No images found -- nothing to clean up") |
There was a problem hiding this comment.
Re-creating the ui.UI object here is redundant and inconsistent with the rest of the command, which already initializes an out object. It is better to pass the existing out to this helper function.
| for _, img := range old { | ||
| if ctx.Err() != nil { | ||
| return removed > 0, ctx.Err() | ||
| return removed, freedB |
There was a problem hiding this comment.
The context cancellation error is swallowed here, returning only the counts. This prevents the caller from knowing that the operation was interrupted, which could lead to incorrect success reporting in the parent command. The function should return the error to allow proper propagation.
| if ctx.Err() != nil { | ||
| _, _ = fmt.Fprintf(errOut, "Warning: auto-cleanup interrupted\n") | ||
| break | ||
| return removed, freedB |
There was a problem hiding this comment.
Similar to the cleanup command, the context cancellation error is swallowed here. While this is an 'auto' cleanup, it is still better to return the error so the caller can decide whether to log it or treat it as a failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/init.go (1)
42-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the non-interactive example to include
--log-level.The example on Line 43 omits a flag that
initAllFlagsSet()and the usage error both require for non-interactive mode, so copy/pasting it does not follow the documented path.Suggested fix
- synthorg init --backend-port 3001 --web-port 3000 --sandbox true # non-interactive`, + synthorg init --backend-port 3001 --web-port 3000 --sandbox true --log-level info # non-interactive`,As per coding guidelines
cli/cmd/init.go:initcommand accepts flags--backend-port,--web-port,--sandbox,--log-level(required for non-interactive mode) and optional--image-tag,--channel,--bus-backend,--persistence-backend,--postgres-port,--encrypt-secrets("true" or "false", default "true").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/init.go` around lines 42 - 43, The non-interactive example string in init's usage text is missing the required --log-level flag (checked by initAllFlagsSet()), so update the example text (the multiline Example string near the top of init command) to include a --log-level argument (e.g. --log-level info) alongside --backend-port, --web-port and --sandbox; ensure the example reflects that --log-level is required for non-interactive mode and optionally mention other optional flags noted by init (image-tag, channel, bus-backend, persistence-backend, postgres-port, encrypt-secrets) if you wish to mirror full guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/cmd/cleanup.go`:
- Around line 174-177: The cleanup currently ignores hard image-removal
failures; modify removeOldImages to return an error (e.g., change signature to
return (removed int, freedB int64, err error) or a slice of removal errors),
update the call sites where removeOldImages is used (the block that calls
emitCleanupSummary and returns removed > 0, nil and the other similar block at
lines ~201-222) to check err and, after emitting the summary, propagate that
error (return removed > 0, err) instead of always returning nil so non-"in use"
docker rmi failures produce a runtime error exit; keep emitCleanupSummary call
but ensure it runs before returning the error.
In `@cli/cmd/config_dispatch.go`:
- Around line 116-118: The current readers for "color", "hints", "output", and
"timestamps" return the raw persisted string (s.Color, s.Hints, s.Output,
s.Timestamps) which yields an empty display after `config unset`; change each
reader so that when the persisted value is empty it returns the
effective/default mode instead (e.g., if s.Color == "" return the
default/effective color value from the state), so replace the simple returns
with default-aware logic that falls back to the state's default/effective value
for those four fields.
In `@cli/cmd/config.go`:
- Around line 438-440: The compose restart follow-up should be emitted as a
next-step hint, not guidance: update the hint tier used by the helper to use
HintNextStep instead of HintGuidance. Locate the hintComposeRestart helper and
change its hint creation to use HintNextStep, and update any calls (e.g., the
call guarded by composeAffectingKeys and the other occurrences around lines
454-465) to rely on the modified helper so the restart message is emitted as a
HintNextStep.
In `@cli/cmd/doctor.go`:
- Around line 212-227: The unfixable issues list returned/used by
classifyDoctorIssues must be filtered according to the user's --checks selection
before it is used to decide doctorAutoFix behavior and to emit hints; modify the
flow that calls classifyDoctorIssues (and/or adjust classifyDoctorIssues itself)
so it only includes categories present in the parsed checks set (treat "all" as
no filter) before computing needComposeFix/needRestart or iterating unfixable;
ensure the same filtered subset is used for runDoctorComposeFix,
runDoctorRestart and when printing "No auto-fix available for: %s" so hints only
appear for checks the user requested (reference classifyDoctorIssues,
needComposeFix, needRestart, unfixable, runDoctorComposeFix, runDoctorRestart).
In `@cli/cmd/new.go`:
- Around line 101-102: Replace the plain fmt-based recovery message with the
hint-tier helper so the partial-write recovery guidance is emitted via HintError
rather than direct writer output: after calling warnPartialScaffoldWrite (and
where the code currently returns fmt.Errorf("writing %s scaffold: %w", useName,
writeErr)), invoke the HintError/HintErrorf helper to print a clear recovery
hint including useName and the writeErr details (and still return the underlying
error as appropriate); apply the same change to the similar block around the
121-132 region to ensure all partial-write guidance uses HintError instead of
raw fmt output.
In `@cli/cmd/start.go`:
- Around line 377-380: Replace the use of the config-automation hint tier by
changing the call to out.HintTip(fmt.Sprintf(...)) that emits the fine-tune size
warning to use the guidance tier instead; locate the call referencing sizeHint
and swap out.HintTip to out.HintGuidance (or out.HintNextStep if you prefer an
action framing) so the runtime guidance follows the CLI hint-tier guidelines.
In `@cli/cmd/uninstall.go`:
- Around line 81-82: Replace the actionable guidance calls that currently use
out.HintGuidance with out.HintNextStep so follow-up instructions are emitted as
"next actions"; specifically change the calls to out.HintGuidance("Reinstall
from GitHub Releases: https://github.com/Aureliolo/synthorg/releases") and the
similar call at the other occurrence to use out.HintNextStep instead, leaving
surrounding logic (including return nil) unchanged.
In `@cli/cmd/update_cleanup.go`:
- Around line 79-81: Replace the action-oriented hint call to use HintNextStep
instead of HintGuidance: locate the branch that checks removed > 0 where
out.HintGuidance("Run 'synthorg cleanup --keep N' to preserve recent previous
versions.") is invoked and change that call to out.HintNextStep(...) keeping the
same message so the UX uses the proper next-action helper.
In `@cli/internal/completion/install.go`:
- Around line 257-258: The current home-containment check incorrectly treats any
string starting with ".." as outside home (variables relErr and rel), which
rejects valid names like "..config/profile.ps1"; update the condition to only
treat bare ".." or a ".." followed by a path separator as external by replacing
strings.HasPrefix(rel, "..") with rel == ".." || strings.HasPrefix(rel,
".."+string(filepath.Separator)); ensure filepath.Separator is
referenced/imported if needed and keep the existing relErr check unchanged.
In `@cli/internal/compose/validate.go`:
- Around line 194-200: The function validateDigestPins currently ranges
p.DigestPins (map) directly which yields nondeterministic order; change it to
collect the map keys into a slice, sort the slice (e.g., using sort.Strings),
then iterate over the sorted keys and validate each digest (using
verify.IsValidDigest) exactly as before so the returned fmt.Errorf for invalid
digest pins remains the same; update references inside validateDigestPins to use
the keyed lookup p.DigestPins[name] while keeping Params and function signature
unchanged.
In `@cli/internal/config/validate.go`:
- Around line 104-111: The validateMasterKey function currently allows
encrypt_secrets=true with an empty MasterKey; update validateMasterKey (and use
the State fields EncryptSecrets and MasterKey) so that if s.EncryptSecrets is
true and strings.TrimSpace(s.MasterKey) == "" it returns a validation error
(e.g., "master_key required when encrypt_secrets is enabled"); otherwise proceed
to call validateFernetKey(s.MasterKey) and wrap its error as currently done.
In `@cli/internal/scaffold/writer.go`:
- Around line 98-103: The current lexical check using filepath.Join and
strings.HasPrefix on abs/absRoot is vulnerable to symlink traversal; change the
validation in the writer that computes abs (the block using absRoot, clean and
f.Path) to resolve symlinks before allowing writes: either call
filepath.EvalSymlinks on both absRoot and the candidate abs (or on the
candidate's parent path) and compare the resolved paths with a secure prefix
check, or walk the candidate path components with os.Lstat and reject any path
containing a symlink component; ensure the error message still references f.Path
when rejecting the path.
In `@cli/internal/verify/dhi.go`:
- Around line 483-493: readCosignPayload currently caps reads at maxBundleBytes
and silently truncates larger layers; align it with readAttestationStatement by
reading up to maxBundleBytes+1 (use io.LimitReader(reader, maxBundleBytes+1)),
close reader, then check if len(payload) > maxBundleBytes and return an explicit
"payload too large" error if so; keep the same error handling/reader.Close
pattern and reference readCosignPayload, readAttestationStatement, and
maxBundleBytes to locate the change.
---
Outside diff comments:
In `@cli/cmd/init.go`:
- Around line 42-43: The non-interactive example string in init's usage text is
missing the required --log-level flag (checked by initAllFlagsSet()), so update
the example text (the multiline Example string near the top of init command) to
include a --log-level argument (e.g. --log-level info) alongside --backend-port,
--web-port and --sandbox; ensure the example reflects that --log-level is
required for non-interactive mode and optionally mention other optional flags
noted by init (image-tag, channel, bus-backend, persistence-backend,
postgres-port, encrypt-secrets) if you wish to mirror full guidance.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12a85fad-4ee2-4c93-aa66-69f9f5dc2bfe
📒 Files selected for processing (41)
cli/.golangci.ymlcli/cmd/backup.gocli/cmd/cleanup.gocli/cmd/config.gocli/cmd/config_dispatch.gocli/cmd/config_tunables.gocli/cmd/config_tunables_dispatch.gocli/cmd/doctor.gocli/cmd/init.gocli/cmd/init_helpers.gocli/cmd/init_tui.gocli/cmd/init_tui_view.gocli/cmd/new.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/cmd/update_cleanup.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/cmd/version.gocli/cmd/wipe.gocli/cmd/worker_start.gocli/internal/completion/install.gocli/internal/compose/generate.gocli/internal/compose/validate.gocli/internal/config/changelog_view_test.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/config/validate.gocli/internal/diagnostics/collect.gocli/internal/docker/client.gocli/internal/scaffold/writer.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/walk.gocli/internal/ui/commitlist_walk.gocli/internal/ui/livebox.gocli/internal/ui/walk.gocli/internal/verify/dhi.gocli/internal/verify/provenance.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). (5)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: CLI Test (ubuntu-latest)
- GitHub Check: CLI Bench Regression
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (20)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/cmd/version.gocli/cmd/worker_start.gocli/internal/config/changelog_view_test.gocli/internal/docker/client.gocli/internal/ui/livebox.gocli/internal/diagnostics/collect.gocli/internal/selfupdate/updater.gocli/internal/verify/provenance.gocli/cmd/config_tunables_dispatch.gocli/cmd/update_cleanup.gocli/cmd/config_dispatch.gocli/cmd/update_compose.gocli/internal/scaffold/writer.gocli/cmd/config_tunables.gocli/internal/ui/commitlist_walk.gocli/cmd/uninstall.gocli/cmd/config.gocli/cmd/init_helpers.gocli/cmd/verify_pipeline.gocli/cmd/root.gocli/cmd/update.gocli/cmd/init.gocli/internal/completion/install.gocli/internal/config/validate.gocli/cmd/new.gocli/cmd/start.gocli/internal/verify/dhi.gocli/cmd/wipe.gocli/internal/selfupdate/walk.gocli/internal/config/tunables.gocli/cmd/doctor.gocli/internal/ui/walk.gocli/cmd/status.gocli/internal/compose/validate.gocli/internal/compose/generate.gocli/internal/config/state.gocli/cmd/backup.gocli/cmd/init_tui_view.gocli/cmd/cleanup.gocli/cmd/init_tui.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/version.gocli/cmd/worker_start.gocli/cmd/config_tunables_dispatch.gocli/cmd/update_cleanup.gocli/cmd/config_dispatch.gocli/cmd/update_compose.gocli/cmd/config_tunables.gocli/cmd/uninstall.gocli/cmd/config.gocli/cmd/init_helpers.gocli/cmd/verify_pipeline.gocli/cmd/root.gocli/cmd/update.gocli/cmd/init.gocli/cmd/new.gocli/cmd/start.gocli/cmd/wipe.gocli/cmd/doctor.gocli/cmd/status.gocli/cmd/backup.gocli/cmd/init_tui_view.gocli/cmd/cleanup.gocli/cmd/init_tui.go
cli/cmd/version.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
versioncommand accepts--shortflag
Files:
cli/cmd/version.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/changelog_view_test.gocli/internal/verify/provenance.gocli/internal/config/validate.gocli/internal/verify/dhi.gocli/internal/config/tunables.gocli/internal/config/state.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/changelog_view_test.gocli/internal/config/validate.gocli/internal/config/tunables.gocli/internal/config/state.go
cli/cmd/uninstall.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
uninstallcommand accepts--keep-dataand--keep-imagesflags
Files:
cli/cmd/uninstall.go
cli/cmd/config.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Implement
synthorg config <subcommand>to exposeshow,get <key>,set <key> <value>,unset <key>,list,path, andeditwith 40 settable keys (backend_port, web_port, sandbox, image_tag, log_level, fine_tuning, telemetry_opt_in, channel, plus tunables)
Files:
cli/cmd/config.go
cli/cmd/update.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
updatecommand accepts flags--dry-run,--no-restart,--timeout,--cli-only,--images-only,--check
Files:
cli/cmd/update.go
cli/cmd/init.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/init.go:initcommand accepts flags--backend-port,--web-port,--sandbox,--log-level(required for non-interactive mode) and optional--image-tag,--channel,--bus-backend,--persistence-backend,--postgres-port,--encrypt-secrets("true" or "false", default "true")
Interactiveinitdefaults to Postgres + NATS; non-interactive defaults to SQLite + internal bus
Files:
cli/cmd/init.go
cli/cmd/new.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
new <kind> <domain>command accepts--dry-runand--overwriteflags and scaffolds a conventions-clean Python file set undersrc/synthorg/for a new feature;<kind>is one ofservice,persistence,tool,controller
Files:
cli/cmd/new.go
cli/cmd/start.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
startcommand accepts flags--no-wait,--timeout,--no-pull,--dry-run,--no-detach,--no-verify
Files:
cli/cmd/start.go
cli/internal/verify/dhi.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
cli/internal/verify/dhi.goin sync withDefault{Postgres,NATS}Image{Tag,Digest}constants by derivingdhiPinnedIndexDigestsmap from these constants at init
Files:
cli/internal/verify/dhi.go
cli/cmd/wipe.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
wipecommand accepts--dry-run,--no-backup,--keep-imagesflags
Files:
cli/cmd/wipe.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
cli/cmd/status.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/status.go:statuscommand accepts flags--watch/-w,--interval,--wide,--no-trunc,--services,--check
synthorg statusrenders a verdict banner (OK / DEGRADED / CRITICAL) computed bycomputeVerdict()incli/cmd/status.go; CRITICAL wins over DEGRADED; signals are gated on install expectations so internal-bus install is not flagged DEGRADED merely because health response omits message_bus
Files:
cli/cmd/status.go
cli/internal/compose/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Every generated
compose.ymlincludes a one-shotdata-inithelper that chowns each named volume to its non-root owner (65532:65532 for backend/NATS, 70:70 with mode 0700 for Postgres) before stateful services start; Postgres/NATS services declaredepends_on: data-init: condition: service_completed_successfully
Files:
cli/internal/compose/validate.gocli/internal/compose/generate.go
cli/internal/compose/generate.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Port layout: 3000 web / 3001 backend / 3002 postgres / 3003 NATS client;
generate.govalidates port collisions across all enabled services
Files:
cli/internal/compose/generate.go
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define
Default{Postgres,NATS}ImageTagand matchingDefault{Postgres,NATS}ImageDigestconstants incli/internal/config/state.gowith a// renovate:annotation on the tag constant for Renovate customManager to keep both tag and digest current
Files:
cli/internal/config/state.go
cli/cmd/backup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/backup.go:backup createcommand accepts flags--output/-oand--timeout
backup listcommand accepts flags--limit/-nand--sort(newest|oldest|size)
backup restorecommand accepts flags--confirm(required),--dry-run,--no-restart,--timeout
Files:
cli/cmd/backup.go
cli/cmd/cleanup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cleanupcommand accepts flags--dry-run,--all,--keep N
Files:
cli/cmd/cleanup.go
🪛 GitHub Check: CodeQL
cli/internal/completion/install.go
[failure] 381-381: 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.
cli/cmd/start.go
[failure] 147-147: 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.
cli/cmd/wipe.go
[failure] 175-175: 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 (30)
cli/.golangci.yml (1)
46-55: LGTM!cli/cmd/update_compose.go (1)
116-122: LGTM!Also applies to: 127-148
cli/cmd/new.go (1)
77-80: LGTM!Also applies to: 108-119, 134-160
cli/cmd/root.go (1)
258-279: LGTM!Also applies to: 347-364, 393-436
cli/cmd/version.go (1)
23-23: LGTM!cli/cmd/worker_start.go (1)
212-229: LGTM!cli/internal/verify/dhi.go (3)
304-312: LGTM!Also applies to: 314-329, 331-352, 354-376
380-411: LGTM!Also applies to: 413-438, 440-481
20-20: LGTM!cli/cmd/verify_pipeline.go (4)
53-67: LGTM!
69-88: LGTM!
90-110: LGTM!
112-128: LGTM!cli/internal/verify/provenance.go (1)
100-105: LGTM!Also applies to: 107-139, 141-161
cli/internal/completion/install.go (1)
213-256: LGTM!Also applies to: 260-281, 381-433
cli/internal/diagnostics/collect.go (1)
193-210: LGTM!cli/internal/docker/client.go (1)
149-197: LGTM!cli/internal/scaffold/writer.go (1)
39-83: LGTM!Also applies to: 105-130, 177-198
cli/internal/selfupdate/updater.go (1)
178-260: LGTM!cli/internal/selfupdate/walk.go (1)
64-65: LGTM!Also applies to: 81-101
cli/internal/ui/commitlist_walk.go (1)
125-182: LGTM!cli/internal/ui/livebox.go (1)
119-144: LGTM!cli/internal/ui/walk.go (1)
266-351: LGTM!cli/cmd/init.go (1)
227-245: Reinit--yespreservesSettingsKey
applyReinitYescallscopyPreservedSecrets, and that helper copiesoldState.SettingsKeyinto the newstate.SettingsKeywhen non-empty (with tests covering the--yespath).cli/cmd/backup.go (1)
285-305: LGTM!Also applies to: 307-331, 333-349, 415-418, 496-537, 574-640
cli/cmd/start.go (1)
68-116: LGTM!Also applies to: 118-155, 238-265, 321-362, 383-428
cli/cmd/update_cleanup.go (1)
73-78: LGTM!Also applies to: 84-108
cli/cmd/wipe.go (1)
99-182: LGTM!Also applies to: 207-272
cli/cmd/uninstall.go (1)
69-78: LGTM!Also applies to: 85-138, 266-317, 498-557
cli/cmd/update.go (1)
348-431: LGTM!
… gemini, 1 ci-bench) CI fixes: - restore CLI bench regression budget: hoist Validate / validateTunables check slices to package vars and replace closure-based bindings in resolveDurationTunables / resolveCountTunables with direct pointer setters; unroll per-section enumCheck / formatCheck / duration / byte slices in validate.go to inline if-chains. The closure + per-call slice allocations had pushed BenchmarkResolveTunables +32% and BenchmarkLoadExisting +57% allocs. Security alerts (3 dismissed): - CodeQL #515 cli/cmd/start.go:147 (false positive; safeDir comes from safeStateDir -> config.SecurePath, sanitiser cannot be traced past the assertComposeExists helper boundary) - CodeQL #516 cli/cmd/wipe.go:175 (same shape on requireComposeFile) - CodeQL #517 cli/internal/completion/install.go:381 (false positive; path is resolved from a fixed allowlist of shell config locations under the operator home dir, which is the entire point of completion uninstall) Reviewer feedback: - cleanup.go: collectCleanupCandidates now takes the existing *ui.UI instead of re-creating one; removeOldImages signature widened to (removed, freedB, hardFailures, ctxErr) so non "in use" docker rmi failures and ctx cancellation both surface as runtime errors - update_cleanup.go: runAutoCleanupRemovals returns ctxErr; reinstall next-step hint upgraded to HintNextStep - config.go: hintComposeRestart uses HintNextStep - config_dispatch.go: color/hints/output/timestamps readers now return the effective default (auto/auto/text/relative) instead of empty - doctor.go: classifyDoctorIssues honours --checks for the unfixable bucket (anyFixableCheckEnabled gate) - new.go: warnPartialScaffoldWrite emits the recovery hint via HintError so it survives every mode except --quiet - start.go: emitFineTuneSizeHint uses HintGuidance instead of HintTip - uninstall.go: GitHub releases reinstall hint + "container images still on disk" hint upgraded to HintNextStep - completion/install.go: probeShellProfile rel-check no longer rejects filenames that lexically start with ".." (e.g. "..config/profile.ps1") - compose/validate.go: validateDigestPins sorts keys before iterating so the returned error is deterministic - scaffold/writer.go: resolveOneTarget now resolves the deepest existing ancestor via EvalSymlinks and re-checks containment, so a symlinked subpath under absRoot cannot escape at write time - verify/dhi.go: readCosignPayload reads maxBundleBytes+1 and rejects oversize payloads explicitly (mirrors readAttestationStatement) - config/validate.go: validateMasterKey rejects an empty MasterKey when EncryptSecrets is true, exported as ErrMissingMasterKey; introduced LoadAllowMissingMasterKey + ValidateAllowMissingMasterKey so handleReinit can still recover an install whose persisted config predates the new invariant SKIP (verified factually wrong against current code): - Gemini "critical" errors.AsType typo on cli/cmd/update.go:360 -- Go 1.25+ stdlib added a generic errors.AsType helper; build + CI tests pass on Go 1.26 in this repo. golangci-lint actually surfaces the modernisation hint that callers SHOULD migrate to AsType. Tests updated where required: - cmd/* tests, config/* tests: explicit `state.EncryptSecrets = false` (or `encrypt_secrets: false` in JSON fixtures) where the test targets non-encryption behaviour, since the new MasterKey invariant rejects the DefaultState() baseline that those fixtures rely on. Issue: #2099
Aureliolo
force-pushed
the
refactor/cli-complexity-ratchet
branch
from
cda208a to
d8fca4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/update.go (1)
117-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
HintNextStepfor these split-update follow-ups.Both messages are concrete next actions, not feature-discovery guidance.
Suggested fix
- out.HintGuidance("Run 'synthorg update --images-only' to update container images separately.") + out.HintNextStep("Run 'synthorg update --images-only' to update container images separately.") @@ - out.HintGuidance("Run 'synthorg update --cli-only' to update the CLI binary separately.") + out.HintNextStep("Run 'synthorg update --cli-only' to update the CLI binary separately.")As per coding guidelines,
cli/**/*.go: "Use the appropriate hint tier based on intent:HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/update.go` around lines 117 - 128, Replace the two uses of out.HintGuidance with out.HintNextStep for the split-update follow-ups: when updateCLIOnly is true (message suggesting "Run 'synthorg update --images-only'...") and when updateImagesOnly is true (message suggesting "Run 'synthorg update --cli-only'..."); locate these calls in update.go around the updateCLIOnly/updateImagesOnly branches (and the updateComposeAndImages call) and simply swap the HintGuidance calls to HintNextStep so they use the correct hint tier.
♻️ Duplicate comments (1)
cli/cmd/doctor.go (1)
236-255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--checksfiltering still leaks unfixable hints from excluded categoriesAt Line 249,
unfixableissues are appended wheneveranyFixableCheckEnabled()is true, which does not verify that the issue belongs to a selected category. With--checks=compose, unrelated issues can still produceNo auto-fix available for ...hints.As per coding guidelines, "Filter
synthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/doctor.go` around lines 236 - 255, The code currently appends unfixable issues whenever anyFixableCheckEnabled() is true, which leaks hints for categories the user excluded; modify classifyDoctorIssues so that for the doctorIssueUnfixable branch you check whether the specific check/category that produced that issue is enabled (use doctorCheckEnabled(...) for the appropriate category) instead of anyFixableCheckEnabled(); if classifyDoctorIssue(issue) does not return a category, add or use a helper that maps the issue to its originating check and call doctorCheckEnabled for that check before appending to unfixable (refer to classifyDoctorIssues, classifyDoctorIssue, anyFixableCheckEnabled and doctorCheckEnabled to locate and update the logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/cmd/doctor.go`:
- Around line 217-227: The current return value in runDoctor incorrectly uses
the intent flags (needComposeFix || needRestart) instead of the actual outcome
of the attempted fixes; update runDoctor to capture and use the boolean results
from runDoctorComposeFix and runDoctorRestart (e.g., successCompose :=
runDoctorComposeFix(...), successRestart := runDoctorRestart(...)) and return
successCompose || successRestart || any other true fix results, and likewise
update the duplicate logic in the other block (lines ~302-323) to use the
functions' return values rather than the original need* flags so failures are
not reported as successful fixes; reference runDoctor, runDoctorComposeFix, and
runDoctorRestart to locate and modify these checks.
In `@cli/cmd/init_helpers.go`:
- Around line 30-33: The interactive re-init path currently exits when
config.Load returns config.ErrMissingMasterKey; update the logic around
oldState, loadErr to mirror the non-interactive flow by catching
config.ErrMissingMasterKey and calling config.LoadAllowMissingMasterKey (or
otherwise loading with the missing-master-key recovery) so interactive re-init
can recover the same on-disk state; specifically adjust the block that calls
config.Load to fall back to config.LoadAllowMissingMasterKey when loadErr ==
config.ErrMissingMasterKey and proceed with the recovered state.
In `@cli/cmd/root.go`:
- Around line 258-260: The persisted state.Output=="json" should not override an
explicit --plain; in the block after applyColorOverride (where it currently does
if !flagJSON && !opts.JSON && state.Output == "json" { opts.JSON = true }), only
enable opts.JSON from state when the user has not explicitly requested plain
output—i.e., ensure you also check that neither opts.Plain nor the CLI plain
flag is set; change the condition to require !opts.Plain (or !flagPlain if that
flag variable exists) before setting opts.JSON so setupGlobalOpts' explicit
--plain wins over persisted output.
In `@cli/cmd/start.go`:
- Around line 327-329: The pull flow is reusing state.VerifiedDigests for
sandbox/sidecar/fine-tune refs even when registry overrides (registry_host,
image_repo_prefix, dhi_registry, postgres_image_tag, nats_image_tag) are active;
this can pin pulls to stale digests. Update the logic so that when any of those
overrides are set for the invocation you do not use state.VerifiedDigests for
standalone pulls: either add an explicit "ignoreVerifiedDigests" flag passed
into buildPullItems/runPullBatch or make buildPullItems detect the overrides on
state.config and skip applying state.VerifiedDigests for
sandbox/sidecar/fine-tune refs, ensuring the code path around buildPullItems,
emitFineTuneSizeHint, and runPullBatch uses the cleared/ignored digest set (also
apply the same change to the similar block at the 344-365 region).
In `@cli/cmd/uninstall.go`:
- Around line 60-67: The uninstall path currently aborts when
config.Load(opts.DataDir) fails; instead, catch the error, log or note the
failure, and proceed by constructing a minimal/sanitized state that uses
opts.DataDir as the base so safeStateDir can still run: when config.Load returns
an error, do not return—create a fallback state (e.g., a state struct with
DataDir set to opts.DataDir or otherwise sanitized) and then call
safeStateDir(fallbackState) and continue the teardown; update the logic around
config.Load, safeStateDir and any subsequent code that assumes a loaded state so
uninstall remains best-effort even with a broken config.
In `@cli/cmd/wipe.go`:
- Around line 257-258: The current check in the wipe logic (the conditional
referencing wc.safeDir) wrongly treats any substring ".." as traversal; instead,
canonicalize/clean the path (use filepath.Clean) and split it into path
elements, then reject only if an element equals ".." (i.e., true path traversal)
or if the cleaned path is "/" or the volume root (as currently checked). Update
the condition that uses strings.Contains(wc.safeDir, "..") to iterate over
strings.Split(filepath.Clean(wc.safeDir), string(filepath.Separator)) and check
for an element == "..", keeping the other root/volume-name checks intact so
legitimate names like "/var/lib/synthorg..bak" are allowed.
In `@cli/internal/completion/install.go`:
- Around line 255-260: The probeShellProfile logic rejects profiles based on
filepath.Rel between resolvedHome and resolved; on Windows this can misbehave
due to drive-letter casing—apply the same normalization used elsewhere by
lowercasing/case-folding both resolvedHome and resolved before calling
filepath.Rel (or extract that normalization into a shared helper such as
normalizePathForCompare and use it from probeShellProfile) so that filepath.Rel
compares case-insensitively on Windows and avoids spurious ".." prefixes; update
the code paths around resolveProfileDir(cleaned), resolvedHome, and the
filepath.Rel call accordingly.
In `@cli/internal/compose/validate.go`:
- Around line 105-106: The comparison uses the untyped constant 4294967295
against p.DockerSockGID (an int), which breaks on 32-bit builds; fix by doing
the comparison in a wider signed type—cast p.DockerSockGID to int64 (or cast the
constant to int64) and change the check to something like: if
int64(p.DockerSockGID) < -1 || int64(p.DockerSockGID) > 4294967295 { return
fmt.Errorf(...) } so the upper bound is representable on all architectures and
the error message still uses p.DockerSockGID.
In `@cli/internal/docker/client.go`:
- Around line 161-163: The loop that parses semver components currently skips
empty components (if len(numStr) == 0) which coerces malformed inputs like "abc"
into 0.0.0; change that behavior to reject such components by returning an error
instead of continuing. Locate the parsing loop that uses the variable numStr
(and the surrounding semver parsing function) and replace the `continue` with an
error return (for example fmt.Errorf("malformed semver component %q", numStr)),
and propagate that error up to callers so malformed versions are surfaced rather
than silently parsed as zero.
In `@cli/internal/selfupdate/updater.go`:
- Around line 200-211: isUsableRelease currently calls compareWithDev(r.TagName,
r.TagName) which doesn't actually validate semver; instead parse the tag with
the real semver parser and reject on parse failure: in isUsableRelease, attempt
to parse r.TagName (using the existing semver/parse function used elsewhere in
the package) and return false if parsing errors, while still checking r.Draft;
apply the same change to the similar validation logic used by pickNewerRelease
so malformed tags are skipped consistently (references: isUsableRelease,
devRelease, compareWithDev, pickNewerRelease).
---
Outside diff comments:
In `@cli/cmd/update.go`:
- Around line 117-128: Replace the two uses of out.HintGuidance with
out.HintNextStep for the split-update follow-ups: when updateCLIOnly is true
(message suggesting "Run 'synthorg update --images-only'...") and when
updateImagesOnly is true (message suggesting "Run 'synthorg update
--cli-only'..."); locate these calls in update.go around the
updateCLIOnly/updateImagesOnly branches (and the updateComposeAndImages call)
and simply swap the HintGuidance calls to HintNextStep so they use the correct
hint tier.
---
Duplicate comments:
In `@cli/cmd/doctor.go`:
- Around line 236-255: The code currently appends unfixable issues whenever
anyFixableCheckEnabled() is true, which leaks hints for categories the user
excluded; modify classifyDoctorIssues so that for the doctorIssueUnfixable
branch you check whether the specific check/category that produced that issue is
enabled (use doctorCheckEnabled(...) for the appropriate category) instead of
anyFixableCheckEnabled(); if classifyDoctorIssue(issue) does not return a
category, add or use a helper that maps the issue to its originating check and
call doctorCheckEnabled for that check before appending to unfixable (refer to
classifyDoctorIssues, classifyDoctorIssue, anyFixableCheckEnabled and
doctorCheckEnabled to locate and update the logic).
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b630f17-a110-4025-b2e3-c0b1104a62f3
📒 Files selected for processing (47)
cli/.golangci.ymlcli/cmd/backup.gocli/cmd/backup_test.gocli/cmd/cleanup.gocli/cmd/config.gocli/cmd/config_changelog_view_test.gocli/cmd/config_dispatch.gocli/cmd/config_ext_test.gocli/cmd/config_test.gocli/cmd/config_tunables.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_tunables_test.gocli/cmd/doctor.gocli/cmd/init.gocli/cmd/init_helpers.gocli/cmd/init_tui.gocli/cmd/init_tui_view.gocli/cmd/new.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/cmd/update_cleanup.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/cmd/version.gocli/cmd/wipe.gocli/cmd/worker_start.gocli/internal/completion/install.gocli/internal/compose/generate.gocli/internal/compose/validate.gocli/internal/config/changelog_view_test.gocli/internal/config/state.gocli/internal/config/state_test.gocli/internal/config/tunables.gocli/internal/config/validate.gocli/internal/diagnostics/collect.gocli/internal/docker/client.gocli/internal/scaffold/writer.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/walk.gocli/internal/ui/commitlist_walk.gocli/internal/ui/livebox.gocli/internal/ui/walk.gocli/internal/verify/dhi.gocli/internal/verify/provenance.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). (6)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: CLI Test (macos-latest)
- GitHub Check: CLI Bench Regression
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (20)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/cmd/version.gocli/cmd/backup_test.gocli/cmd/config_changelog_view_test.gocli/internal/config/state_test.gocli/internal/ui/livebox.gocli/internal/diagnostics/collect.gocli/internal/ui/commitlist_walk.gocli/internal/config/changelog_view_test.gocli/cmd/worker_start.gocli/cmd/update_cleanup.gocli/internal/selfupdate/updater.gocli/internal/ui/walk.gocli/internal/docker/client.gocli/cmd/update_compose.gocli/cmd/config_test.gocli/cmd/config_tunables_test.gocli/cmd/init_tui.gocli/internal/verify/provenance.gocli/cmd/uninstall.gocli/internal/scaffold/writer.gocli/cmd/config_dispatch.gocli/cmd/verify_pipeline.gocli/cmd/root.gocli/internal/compose/generate.gocli/cmd/config_tunables.gocli/cmd/update.gocli/cmd/config_tunables_dispatch.gocli/internal/config/tunables.gocli/internal/completion/install.gocli/internal/verify/dhi.gocli/cmd/new.gocli/internal/selfupdate/walk.gocli/cmd/config_ext_test.gocli/internal/config/state.gocli/cmd/start.gocli/internal/config/validate.gocli/cmd/backup.gocli/cmd/config.gocli/cmd/status.gocli/cmd/wipe.gocli/cmd/init_helpers.gocli/cmd/init.gocli/cmd/cleanup.gocli/cmd/init_tui_view.gocli/internal/compose/validate.gocli/cmd/doctor.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/version.gocli/cmd/backup_test.gocli/cmd/config_changelog_view_test.gocli/cmd/worker_start.gocli/cmd/update_cleanup.gocli/cmd/update_compose.gocli/cmd/config_test.gocli/cmd/config_tunables_test.gocli/cmd/init_tui.gocli/cmd/uninstall.gocli/cmd/config_dispatch.gocli/cmd/verify_pipeline.gocli/cmd/root.gocli/cmd/config_tunables.gocli/cmd/update.gocli/cmd/config_tunables_dispatch.gocli/cmd/new.gocli/cmd/config_ext_test.gocli/cmd/start.gocli/cmd/backup.gocli/cmd/config.gocli/cmd/status.gocli/cmd/wipe.gocli/cmd/init_helpers.gocli/cmd/init.gocli/cmd/cleanup.gocli/cmd/init_tui_view.gocli/cmd/doctor.go
cli/cmd/version.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
versioncommand accepts--shortflag
Files:
cli/cmd/version.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/state_test.gocli/internal/config/changelog_view_test.gocli/internal/verify/provenance.gocli/internal/config/tunables.gocli/internal/verify/dhi.gocli/internal/config/state.gocli/internal/config/validate.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/state_test.gocli/internal/config/changelog_view_test.gocli/internal/config/tunables.gocli/internal/config/state.gocli/internal/config/validate.go
cli/cmd/uninstall.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
uninstallcommand accepts--keep-dataand--keep-imagesflags
Files:
cli/cmd/uninstall.go
cli/internal/compose/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Every generated
compose.ymlincludes a one-shotdata-inithelper that chowns each named volume to its non-root owner (65532:65532 for backend/NATS, 70:70 with mode 0700 for Postgres) before stateful services start; Postgres/NATS services declaredepends_on: data-init: condition: service_completed_successfully
Files:
cli/internal/compose/generate.gocli/internal/compose/validate.go
cli/internal/compose/generate.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Port layout: 3000 web / 3001 backend / 3002 postgres / 3003 NATS client;
generate.govalidates port collisions across all enabled services
Files:
cli/internal/compose/generate.go
cli/cmd/update.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
updatecommand accepts flags--dry-run,--no-restart,--timeout,--cli-only,--images-only,--check
Files:
cli/cmd/update.go
cli/internal/verify/dhi.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
cli/internal/verify/dhi.goin sync withDefault{Postgres,NATS}Image{Tag,Digest}constants by derivingdhiPinnedIndexDigestsmap from these constants at init
Files:
cli/internal/verify/dhi.go
cli/cmd/new.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
new <kind> <domain>command accepts--dry-runand--overwriteflags and scaffolds a conventions-clean Python file set undersrc/synthorg/for a new feature;<kind>is one ofservice,persistence,tool,controller
Files:
cli/cmd/new.go
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define
Default{Postgres,NATS}ImageTagand matchingDefault{Postgres,NATS}ImageDigestconstants incli/internal/config/state.gowith a// renovate:annotation on the tag constant for Renovate customManager to keep both tag and digest current
Files:
cli/internal/config/state.go
cli/cmd/start.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
startcommand accepts flags--no-wait,--timeout,--no-pull,--dry-run,--no-detach,--no-verify
Files:
cli/cmd/start.go
cli/cmd/backup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/backup.go:backup createcommand accepts flags--output/-oand--timeout
backup listcommand accepts flags--limit/-nand--sort(newest|oldest|size)
backup restorecommand accepts flags--confirm(required),--dry-run,--no-restart,--timeout
Files:
cli/cmd/backup.go
cli/cmd/config.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Implement
synthorg config <subcommand>to exposeshow,get <key>,set <key> <value>,unset <key>,list,path, andeditwith 40 settable keys (backend_port, web_port, sandbox, image_tag, log_level, fine_tuning, telemetry_opt_in, channel, plus tunables)
Files:
cli/cmd/config.go
cli/cmd/status.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/status.go:statuscommand accepts flags--watch/-w,--interval,--wide,--no-trunc,--services,--check
synthorg statusrenders a verdict banner (OK / DEGRADED / CRITICAL) computed bycomputeVerdict()incli/cmd/status.go; CRITICAL wins over DEGRADED; signals are gated on install expectations so internal-bus install is not flagged DEGRADED merely because health response omits message_bus
Files:
cli/cmd/status.go
cli/cmd/wipe.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
wipecommand accepts--dry-run,--no-backup,--keep-imagesflags
Files:
cli/cmd/wipe.go
cli/cmd/init.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/init.go:initcommand accepts flags--backend-port,--web-port,--sandbox,--log-level(required for non-interactive mode) and optional--image-tag,--channel,--bus-backend,--persistence-backend,--postgres-port,--encrypt-secrets("true" or "false", default "true")
Interactiveinitdefaults to Postgres + NATS; non-interactive defaults to SQLite + internal bus
Files:
cli/cmd/init.go
cli/cmd/cleanup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cleanupcommand accepts flags--dry-run,--all,--keep N
Files:
cli/cmd/cleanup.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
🔇 Additional comments (22)
cli/.golangci.yml (1)
46-55: LGTM!cli/cmd/backup.go (1)
285-305: LGTM!Also applies to: 307-331, 333-349, 415-418, 496-537, 574-594, 596-640
cli/cmd/backup_test.go (1)
247-250: LGTM!cli/cmd/cleanup.go (1)
67-90: LGTM!Also applies to: 92-122, 124-138, 165-181, 183-199, 201-230, 232-246
cli/cmd/config.go (1)
434-451: LGTM!Also applies to: 464-465, 469-479, 605-607, 627-641, 643-655, 730-740
cli/cmd/config_changelog_view_test.go (1)
28-29: LGTM!Also applies to: 64-65, 86-87, 109-110
cli/cmd/config_dispatch.go (1)
1-183: LGTM!cli/cmd/config_ext_test.go (1)
26-27: LGTM!Also applies to: 54-55, 75-76, 104-105, 131-132, 159-160, 188-189, 217-218, 240-241, 271-272, 302-303, 349-350, 388-389, 416-417, 444-445, 463-464, 489-490, 538-539, 599-600, 634-635
cli/cmd/config_test.go (1)
122-122: LGTM!Also applies to: 149-149, 187-187, 218-218, 255-255, 280-280, 301-301, 347-347, 375-375, 414-414, 439-439, 501-501, 525-525, 561-564, 589-589
cli/cmd/config_tunables.go (1)
19-29: LGTM!Also applies to: 35-40, 47-52, 57-59
cli/cmd/config_tunables_dispatch.go (1)
1-117: LGTM!cli/cmd/config_tunables_test.go (1)
55-55: LGTM!Also applies to: 102-102, 137-137
cli/cmd/init.go (1)
6-6: LGTM!Also applies to: 110-112, 219-244, 246-286, 389-399
cli/cmd/init_tui.go (1)
221-221: LGTM!Also applies to: 277-395
cli/cmd/update.go (1)
356-360: ⚡ Quick winKeep
errors.AsType(it’s stdlib in Go 1.26+); the proposederrors.Aschange isn’t needed.
cli/go.modtargetsgo 1.26.3, anderrors.AsTypeis part of the standard libraryerrorspackage (added in Go 1.26.0), so the current code should compile as written.> Likely an incorrect or invalid review comment.cli/internal/config/changelog_view_test.go (1)
54-57: LGTM!Also applies to: 62-63, 69-70, 76-77, 83-83
cli/internal/config/state.go (1)
248-284: LGTM!Also applies to: 305-316, 322-346, 462-476, 486-511, 535-555
cli/internal/config/state_test.go (1)
378-382: LGTM!cli/internal/config/tunables.go (1)
103-117: LGTM!Also applies to: 123-149, 151-206, 208-237, 345-359, 383-422
cli/internal/config/validate.go (1)
12-285: LGTM!cli/internal/diagnostics/collect.go (1)
193-198: LGTM!Also applies to: 200-210
cli/internal/scaffold/writer.go (1)
47-60: LGTM!Also applies to: 62-83, 85-112, 114-160, 221-242
… gemini, 1 ci-bench) CI fixes: - restore CLI bench regression budget: hoist Validate / validateTunables check slices to package vars and replace closure-based bindings in resolveDurationTunables / resolveCountTunables with direct pointer setters; unroll per-section enumCheck / formatCheck / duration / byte slices in validate.go to inline if-chains. The closure + per-call slice allocations had pushed BenchmarkResolveTunables +32% and BenchmarkLoadExisting +57% allocs. Security alerts (3 dismissed): - CodeQL #515 cli/cmd/start.go:147 (false positive; safeDir comes from safeStateDir -> config.SecurePath, sanitiser cannot be traced past the assertComposeExists helper boundary) - CodeQL #516 cli/cmd/wipe.go:175 (same shape on requireComposeFile) - CodeQL #517 cli/internal/completion/install.go:381 (false positive; path is resolved from a fixed allowlist of shell config locations under the operator home dir, which is the entire point of completion uninstall) Reviewer feedback: - cleanup.go: collectCleanupCandidates now takes the existing *ui.UI instead of re-creating one; removeOldImages signature widened to (removed, freedB, hardFailures, ctxErr) so non "in use" docker rmi failures and ctx cancellation both surface as runtime errors - update_cleanup.go: runAutoCleanupRemovals returns ctxErr; reinstall next-step hint upgraded to HintNextStep - config.go: hintComposeRestart uses HintNextStep - config_dispatch.go: color/hints/output/timestamps readers now return the effective default (auto/auto/text/relative) instead of empty - doctor.go: classifyDoctorIssues honours --checks for the unfixable bucket (anyFixableCheckEnabled gate) - new.go: warnPartialScaffoldWrite emits the recovery hint via HintError so it survives every mode except --quiet - start.go: emitFineTuneSizeHint uses HintGuidance instead of HintTip - uninstall.go: GitHub releases reinstall hint + "container images still on disk" hint upgraded to HintNextStep - completion/install.go: probeShellProfile rel-check no longer rejects filenames that lexically start with ".." (e.g. "..config/profile.ps1") - compose/validate.go: validateDigestPins sorts keys before iterating so the returned error is deterministic - scaffold/writer.go: resolveOneTarget now resolves the deepest existing ancestor via EvalSymlinks and re-checks containment, so a symlinked subpath under absRoot cannot escape at write time - verify/dhi.go: readCosignPayload reads maxBundleBytes+1 and rejects oversize payloads explicitly (mirrors readAttestationStatement) - config/validate.go: validateMasterKey rejects an empty MasterKey when EncryptSecrets is true, exported as ErrMissingMasterKey; introduced LoadAllowMissingMasterKey + ValidateAllowMissingMasterKey so handleReinit can still recover an install whose persisted config predates the new invariant SKIP (verified factually wrong against current code): - Gemini "critical" errors.AsType typo on cli/cmd/update.go:360 -- Go 1.25+ stdlib added a generic errors.AsType helper; build + CI tests pass on Go 1.26 in this repo. golangci-lint actually surfaces the modernisation hint that callers SHOULD migrate to AsType. Tests updated where required: - cmd/* tests, config/* tests: explicit `state.EncryptSecrets = false` (or `encrypt_secrets: false` in JSON fixtures) where the test targets non-encryption behaviour, since the new MasterKey invariant rejects the DefaultState() baseline that those fixtures rely on. Issue: #2099
…ff + 1 cr dup + 3 ci platform regressions) CR re-review on d8fca4e (CHANGES_REQUESTED, second pass): Inline: - cmd/doctor.go: runDoctor returns actual fix outcomes (composeFixed || restartDone) instead of the intent flags (needComposeFix || needRestart); runDoctorComposeFix / runDoctorRestart now return bool. - cmd/doctor.go: classifyDoctorIssues now routes EVERY kind (fixable and unfixable) through doctorCheckEnabled(category) via a per-issue classification that carries both kind and originating --checks category; anyFixableCheckEnabled deleted. Table-driven doctorIssuePatterns + matchesDoctorIssue helper keep classifyDoctorIssue under the cyclomatic-complexity ceiling. - cmd/init_helpers.go: reuseExistingStateForInteractive catches ErrMissingMasterKey and falls back to LoadAllowMissingMasterKey, mirroring the non-interactive recovery in handleReinit. - cmd/root.go: persisted output=json no longer overrides an explicit --plain; the condition now also gates on !flagPlain && !opts.Plain. - cmd/start.go: buildPullItems drops state.VerifiedDigests for the standalone (sandbox / sidecar / fine-tune) refs when ANY registry / image-tag override is active on State (stateHasRegistryOverrides), preventing default-registry digest pins from being applied to an override-registry pull. - cmd/uninstall.go: a broken on-disk config no longer aborts teardown; Load failure logs a warning and falls back to a sanitised State seeded with --data-dir so safeStateDir still has somewhere to resolve. - cmd/wipe.go: removeDataDirectory traversal guard now splits on path separators and matches whole ".." elements instead of substring "..", so names like "/var/lib/synthorg..bak" are not rejected. - internal/completion/install.go: probeShellProfile normalises both resolvedHome and resolved via normalizePathForCompare before filepath.Rel, lowercasing on Windows so drive-letter casing mismatches do not produce a spurious "..\\..\\..." relative path. - internal/compose/validate.go: DockerSockGID upper-bound comparison cast to int64 so the 4294967295 ceiling is representable on 32-bit builds where int is 32 bits. - internal/docker/client.go: parseSemverComponents rejects non-empty components that contain no digit run (e.g. "1.x.0" -> "x") so a malformed input no longer silently coerces to 0.0.0; empty components ("1." or "") still count as 0 for compatibility. - internal/selfupdate/updater.go: isUsableRelease actually parses the tag via compareSemver(base, base) on the dev-stripped base; the prior compareWithDev(tag, tag) self-compare always returned 0 with no error and let malformed tags leak into pickNewerRelease. compareSemver parser extracted to parseSemverComponent (package-level helper) so compareSemver stays under the cognitive-complexity ceiling. Outside diff: - cmd/update.go: split-update follow-up hints (after --cli-only / --images-only) switched from HintGuidance to HintNextStep so they are emitted as next-actions, not flag/feature discovery. CI platform regressions surfaced by my round 2 changes: - internal/scaffold/writer.go: resolveOneTarget now resolves BOTH absRoot and the deepest existing ancestor of the candidate via EvalSymlinks before the containment check. macOS (/var -> /private/var) and Windows (temp-dir junctions) were rejecting every otherwise-legitimate write because absRoot stayed unresolved while the parent did not. - internal/config/state_bench_test.go: BenchmarkLoadExisting fixture now sets EncryptSecrets=false, since DefaultState() has EncryptSecrets=true and the round-2 master_key invariant otherwise rejects the bench fixture. CR outside-diff from round 1 review (missed in round 2): - cmd/init.go: non-interactive Example string now includes --log-level info, matching initAllFlagsSet()'s required-flag set. Issue: #2099
Aureliolo
force-pushed
the
refactor/cli-complexity-ratchet
branch
from
d8fca4e to
d64cdcb
Compare
coderabbitai
Bot
added
type:infra
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/status.go (1)
698-698: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
HintNextStepfor the logs follow-up action.
Run 'synthorg logs'...is a direct next action, so this should useHintNextStepinstead ofHintTip.Suggested patch
- out.HintTip("Run 'synthorg logs' to view container logs") + out.HintNextStep("Run 'synthorg logs' to view container logs")As per coding guidelines,
cli/**/*.go: "Use the appropriate hint tier based on intent: ...HintNextStepfor natural next action ...HintTipfor config automation suggestions ..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/status.go` at line 698, Replace the HintTip call with the next-step hint helper: change the out.HintTip invocation that displays "Run 'synthorg logs' to view container logs" to use out.HintNextStep so the message is presented as a natural next action; locate the call to out.HintTip in cli/cmd/status.go (the one that currently passes "Run 'synthorg logs' to view container logs") and swap it to out.HintNextStep keeping the same message text and any surrounding context.
♻️ Duplicate comments (2)
cli/cmd/start.go (1)
327-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor env-based registry overrides before reusing cached digests.
stateHasRegistryOverridesonly looks at persistedconfig.State, so an invocation that overridesregistry_host/image_repo_prefix/DHI tags via env still leavesuseDigeststrue here. That makes the standalone pulls reusestate.VerifiedDigestsfor the default registry and can pull a stale@sha256ref or fail outright.Proposed fix
func pullAllImages(ctx context.Context, info docker.Info, safeDir string, state config.State, out *ui.UI) (config.State, error) { - items := buildPullItems(state) + items := buildPullItems(state, GetGlobalOpts(ctx).Tunables.CustomRegistry) emitFineTuneSizeHint(state, out) return state, runPullBatch(ctx, info, safeDir, items, out) } @@ -func buildPullItems(state config.State) []pullItem { +func buildPullItems(state config.State, customRegistry bool) []pullItem { var items []pullItem for _, svc := range composeServiceNames(state) { items = append(items, pullItem{name: svc, compose: true}) } - useDigests := !stateHasRegistryOverrides(state) + useDigests := !customRegistry pickDigest := func(name string) string { if !useDigests { return "" } return state.VerifiedDigests[name] } @@ return items } - -func stateHasRegistryOverrides(state config.State) bool { - return state.RegistryHost != "" || - state.ImageRepoPrefix != "" || - state.DHIRegistry != "" || - state.PostgresImageTag != "" || - state.NATSImageTag != "" -}As per coding guidelines, "Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)."Also applies to: 352-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/start.go` around lines 327 - 329, The code is reusing cached digests because stateHasRegistryOverrides only inspects persisted config.State and doesn't account for environment variable overrides; update the logic that sets useDigests (the branch around buildPullItems/emitFineTuneSizeHint/runPullBatch and the similar block at 352-395) to first check effective overrides including env vars (the same env keys: registry_host, image_repo_prefix, dhi_registry, postgres_image_tag, nats_image_tag) and disable useDigests when any such env override is present, clear or ignore state.VerifiedDigests for that invocation, and emit the required stderr warning (unconditional, not suppressed by --quiet/--json) so image signature + SLSA verification is disabled only for this run.cli/internal/selfupdate/updater.go (1)
209-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isUsableReleasestill accepts malformed tags with embedded digits.
parseSemverComponentnow takes the first digit run, so tags likerelease-1.2.3orfoo1.bar2.baz3still passisUsableReleaseand can be ranked as real releases. That keeps the malformed-tag poisoning problem open.Proposed fix
func parseSemverComponent(parts []string, i int, ver string) (int, error) { if i >= len(parts) || parts[i] == "" { return 0, nil } - numStr := strings.FieldsFunc(parts[i], func(r rune) bool { return r < '0' || r > '9' }) - if len(numStr) == 0 { - return 0, fmt.Errorf("invalid version component %q in %q: no digit run", parts[i], ver) + if strings.IndexFunc(parts[i], func(r rune) bool { return r < '0' || r > '9' }) != -1 { + return 0, fmt.Errorf("invalid version component %q in %q", parts[i], ver) } - v, err := strconv.Atoi(numStr[0]) + v, err := strconv.Atoi(parts[i]) if err != nil { - return 0, fmt.Errorf("invalid version component %q in %q: %w", numStr[0], ver, err) + return 0, fmt.Errorf("invalid version component %q in %q: %w", parts[i], ver, err) } return v, nil }Also applies to: 434-446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/internal/selfupdate/updater.go` around lines 209 - 217, isUsableRelease currently accepts tags that embed digit runs (e.g. "release-1.2.3") because parseSemverComponent was loosened; to fix this, ensure the post-split tag strictly matches a semver token before using compareSemver: after calling splitDev(strings.TrimPrefix(r.TagName, "v")) validate that base matches a strict semver regex (e.g. full match for MAJOR.MINOR.PATCH with optional pre-release/build) and only then call compareSemver; apply the same strict validation to the other occurrence referenced (the code around compareSemver at the 434-446 block) so malformed tags with embedded digits are rejected by isUsableRelease and the duplicate check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/cmd/cleanup.go`:
- Around line 78-79: Replace the two follow-up hint calls that use
out.HintGuidance with out.HintNextStep in cli/cmd/cleanup.go (the calls
currently invoking out.HintGuidance for the "--all includes current images.
Running containers will prevent removal." message and the other occurrences
noted around lines 244-245); change the symbol invocations from HintGuidance to
HintNextStep so these action-oriented/destructive-next-step messages use the
correct hint tier.
In `@cli/cmd/config_dispatch.go`:
- Around line 126-130: The config reader for "fine_tuning_variant" currently
returns the raw persisted field (s.FineTuningVariant) which breaks `config get`
by showing an empty line when unset; update the `config get` path to return the
effective value by calling FineTuneVariantOrDefault() instead of the raw field
(or split the reader into separate readers used by runConfigList vs
runConfigGet), ensuring runConfigGet uses FineTuneVariantOrDefault() while
runConfigList continues to inspect s.FineTuningVariant to detect explicit vs
unset.
In `@cli/cmd/doctor.go`:
- Around line 391-399: The doctor status is computed from the full report
instead of the user-filtered subset; update runDoctor to first apply the
--checks filter (treating "all" as omitted) to the report and then pass that
filtered report into classifyDoctor(), renderDoctorSummary(),
collectDoctorWarnings(), and collectDoctorErrors() (or alter classifyDoctor to
accept a pre-filtered report), so that status, summary rendering, and suggested
fixes reflect only the requested check categories; ensure the filtering logic is
applied consistently wherever the unfiltered report is currently used.
In `@cli/cmd/root.go`:
- Around line 405-413: The generic "requires an interactive terminal" hint in
errorHintRules is misleading for the init command; update the rules so the hint
is only shown for commands that support --yes or add an init-specific rule:
implement a guard function (e.g., isYesFlagApplicable(cmd *cobra.Command) bool)
and use it in the existing errorHintRule for the interactive-terminal message,
and add a new rule targeted at the init command (match substring "requires an
interactive terminal" with a guard that detects the init command name or path)
that returns a precise hint listing init's required flags (--backend-port,
--web-port, --sandbox, --log-level) and optional flags (--image-tag, --channel,
--bus-backend, --persistence-backend, --postgres-port, --encrypt-secrets);
modify errorHintRules and add the guard function where errorHintRule evaluation
occurs (referencing errorHintRules, errorHintRule, and the guard name you
choose).
In `@cli/cmd/wipe.go`:
- Around line 99-106: runWipe currently calls loadWipeState (which errors if
compose.yml is missing) before checking wipeDryRun, preventing dry-run/cleanup
on half-installed states; move the call to loadWipeState (and any composePath
resolution) to after the wipeDryRun check so wipeDryRunPreview(out, safeDir,
composePath) can run without requiring compose.yml, or alter loadWipeState to
return a non-fatal nil/empty composePath and not error when compose.yml is
absent so runWipe and wipeDryRunPreview can proceed; update references to
loadWipeState, runWipe, wipeDryRunPreview and composePath accordingly.
- Around line 277-280: The current guard misses bare UNC share roots because
filepath.Clean("\\\\server\\share\\") becomes "\\\\server\\share" and only the
"volume + separator" case is checked; update the condition in the wipe guard to
also reject when wc.safeDir equals filepath.VolumeName(wc.safeDir) (i.e. the
volume name itself, e.g. a UNC root) in addition to the existing checks and
pathContainsTraversal check so removeDataDirExceptSelf() cannot recurse over an
entire share; reference wc.safeDir, filepath.VolumeName(wc.safeDir),
pathContainsTraversal and removeDataDirExceptSelf when making the change.
In `@cli/internal/config/state.go`:
- Around line 497-510: ValidateAllowMissingMasterKey currently skips
validateMasterKey entirely, allowing malformed non-empty master_key to pass;
change it to call s.validateMasterKey() and only ignore the specific
ErrMissingMasterKey error while returning any other validation error.
Concretely, inside ValidateAllowMissingMasterKey (after running stateValidations
and before validateTunables) invoke err := s.validateMasterKey(); if err != nil
and err != ErrMissingMasterKey return err, otherwise continue — this ensures
malformed key material fails but the missing-key recovery path still succeeds.
In `@cli/internal/scaffold/writer.go`:
- Around line 98-101: The resolveOneTarget function currently hard-fails on
empty file contents; update resolveOneTarget (and its use of the RenderedFile
parameter f) to allow zero-byte scaffold files by removing the len(f.Contents)
== 0 error check and returning the computed target path as normal for empty
contents; keep any validation of malformed templates in the renderer, not here,
and ensure downstream callers that write files accept zero-length byte slices
from f.Contents.
---
Outside diff comments:
In `@cli/cmd/status.go`:
- Line 698: Replace the HintTip call with the next-step hint helper: change the
out.HintTip invocation that displays "Run 'synthorg logs' to view container
logs" to use out.HintNextStep so the message is presented as a natural next
action; locate the call to out.HintTip in cli/cmd/status.go (the one that
currently passes "Run 'synthorg logs' to view container logs") and swap it to
out.HintNextStep keeping the same message text and any surrounding context.
---
Duplicate comments:
In `@cli/cmd/start.go`:
- Around line 327-329: The code is reusing cached digests because
stateHasRegistryOverrides only inspects persisted config.State and doesn't
account for environment variable overrides; update the logic that sets
useDigests (the branch around buildPullItems/emitFineTuneSizeHint/runPullBatch
and the similar block at 352-395) to first check effective overrides including
env vars (the same env keys: registry_host, image_repo_prefix, dhi_registry,
postgres_image_tag, nats_image_tag) and disable useDigests when any such env
override is present, clear or ignore state.VerifiedDigests for that invocation,
and emit the required stderr warning (unconditional, not suppressed by
--quiet/--json) so image signature + SLSA verification is disabled only for this
run.
In `@cli/internal/selfupdate/updater.go`:
- Around line 209-217: isUsableRelease currently accepts tags that embed digit
runs (e.g. "release-1.2.3") because parseSemverComponent was loosened; to fix
this, ensure the post-split tag strictly matches a semver token before using
compareSemver: after calling splitDev(strings.TrimPrefix(r.TagName, "v"))
validate that base matches a strict semver regex (e.g. full match for
MAJOR.MINOR.PATCH with optional pre-release/build) and only then call
compareSemver; apply the same strict validation to the other occurrence
referenced (the code around compareSemver at the 434-446 block) so malformed
tags with embedded digits are rejected by isUsableRelease and the duplicate
check.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2c57ffe-83c1-44bd-89de-02039c7aca05
📒 Files selected for processing (48)
cli/.golangci.ymlcli/cmd/backup.gocli/cmd/backup_test.gocli/cmd/cleanup.gocli/cmd/config.gocli/cmd/config_changelog_view_test.gocli/cmd/config_dispatch.gocli/cmd/config_ext_test.gocli/cmd/config_test.gocli/cmd/config_tunables.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_tunables_test.gocli/cmd/doctor.gocli/cmd/init.gocli/cmd/init_helpers.gocli/cmd/init_tui.gocli/cmd/init_tui_view.gocli/cmd/new.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/cmd/update_cleanup.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/cmd/version.gocli/cmd/wipe.gocli/cmd/worker_start.gocli/internal/completion/install.gocli/internal/compose/generate.gocli/internal/compose/validate.gocli/internal/config/changelog_view_test.gocli/internal/config/state.gocli/internal/config/state_bench_test.gocli/internal/config/state_test.gocli/internal/config/tunables.gocli/internal/config/validate.gocli/internal/diagnostics/collect.gocli/internal/docker/client.gocli/internal/scaffold/writer.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/walk.gocli/internal/ui/commitlist_walk.gocli/internal/ui/livebox.gocli/internal/ui/walk.gocli/internal/verify/dhi.gocli/internal/verify/provenance.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: CLI Bench Regression
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (22)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/cmd/version.gocli/cmd/backup_test.gocli/cmd/config_changelog_view_test.gocli/internal/ui/livebox.gocli/internal/selfupdate/walk.gocli/internal/config/state_bench_test.gocli/cmd/worker_start.gocli/cmd/config_tunables_test.gocli/internal/config/state_test.gocli/internal/config/changelog_view_test.gocli/internal/ui/commitlist_walk.gocli/internal/diagnostics/collect.gocli/internal/docker/client.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_tunables.gocli/cmd/init.gocli/cmd/root.gocli/cmd/config.gocli/internal/scaffold/writer.gocli/cmd/config_test.gocli/cmd/update_compose.gocli/cmd/config_dispatch.gocli/internal/ui/walk.gocli/cmd/verify_pipeline.gocli/internal/compose/generate.gocli/cmd/start.gocli/cmd/update.gocli/internal/verify/dhi.gocli/internal/compose/validate.gocli/internal/selfupdate/updater.gocli/cmd/config_ext_test.gocli/cmd/new.gocli/cmd/init_tui_view.gocli/cmd/init_tui.gocli/cmd/status.gocli/internal/verify/provenance.gocli/cmd/init_helpers.gocli/cmd/update_cleanup.gocli/cmd/cleanup.gocli/cmd/doctor.gocli/internal/completion/install.gocli/cmd/wipe.gocli/internal/config/validate.gocli/cmd/uninstall.gocli/internal/config/state.gocli/internal/config/tunables.gocli/cmd/backup.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/version.gocli/cmd/backup_test.gocli/cmd/config_changelog_view_test.gocli/cmd/worker_start.gocli/cmd/config_tunables_test.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_tunables.gocli/cmd/init.gocli/cmd/root.gocli/cmd/config.gocli/cmd/config_test.gocli/cmd/update_compose.gocli/cmd/config_dispatch.gocli/cmd/verify_pipeline.gocli/cmd/start.gocli/cmd/update.gocli/cmd/config_ext_test.gocli/cmd/new.gocli/cmd/init_tui_view.gocli/cmd/init_tui.gocli/cmd/status.gocli/cmd/init_helpers.gocli/cmd/update_cleanup.gocli/cmd/cleanup.gocli/cmd/doctor.gocli/cmd/wipe.gocli/cmd/uninstall.gocli/cmd/backup.go
cli/cmd/version.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
versioncommand accepts--shortflag
Files:
cli/cmd/version.go
cli/**/*_bench_test.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the modern
for b.Loop()pattern (Go 1.24+) in*_bench_test.gofiles with Go's standardtesting.Bframework
Files:
cli/internal/config/state_bench_test.go
cli/internal/**/*_bench_test.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Place
*_bench_test.gofiles next to their*_test.gosiblings undercli/internal/<pkg>/
Files:
cli/internal/config/state_bench_test.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/state_bench_test.gocli/internal/config/state_test.gocli/internal/config/changelog_view_test.gocli/internal/verify/dhi.gocli/internal/verify/provenance.gocli/internal/config/validate.gocli/internal/config/state.gocli/internal/config/tunables.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/state_bench_test.gocli/internal/config/state_test.gocli/internal/config/changelog_view_test.gocli/internal/config/validate.gocli/internal/config/state.gocli/internal/config/tunables.go
cli/cmd/init.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/init.go:initcommand accepts flags--backend-port,--web-port,--sandbox,--log-level(required for non-interactive mode) and optional--image-tag,--channel,--bus-backend,--persistence-backend,--postgres-port,--encrypt-secrets("true" or "false", default "true")
Interactiveinitdefaults to Postgres + NATS; non-interactive defaults to SQLite + internal bus
Files:
cli/cmd/init.go
cli/cmd/config.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Implement
synthorg config <subcommand>to exposeshow,get <key>,set <key> <value>,unset <key>,list,path, andeditwith 40 settable keys (backend_port, web_port, sandbox, image_tag, log_level, fine_tuning, telemetry_opt_in, channel, plus tunables)
Files:
cli/cmd/config.go
cli/internal/compose/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Every generated
compose.ymlincludes a one-shotdata-inithelper that chowns each named volume to its non-root owner (65532:65532 for backend/NATS, 70:70 with mode 0700 for Postgres) before stateful services start; Postgres/NATS services declaredepends_on: data-init: condition: service_completed_successfully
Files:
cli/internal/compose/generate.gocli/internal/compose/validate.go
cli/internal/compose/generate.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Port layout: 3000 web / 3001 backend / 3002 postgres / 3003 NATS client;
generate.govalidates port collisions across all enabled services
Files:
cli/internal/compose/generate.go
cli/cmd/start.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
startcommand accepts flags--no-wait,--timeout,--no-pull,--dry-run,--no-detach,--no-verify
Files:
cli/cmd/start.go
cli/cmd/update.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
updatecommand accepts flags--dry-run,--no-restart,--timeout,--cli-only,--images-only,--check
Files:
cli/cmd/update.go
cli/internal/verify/dhi.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
cli/internal/verify/dhi.goin sync withDefault{Postgres,NATS}Image{Tag,Digest}constants by derivingdhiPinnedIndexDigestsmap from these constants at init
Files:
cli/internal/verify/dhi.go
cli/cmd/new.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
new <kind> <domain>command accepts--dry-runand--overwriteflags and scaffolds a conventions-clean Python file set undersrc/synthorg/for a new feature;<kind>is one ofservice,persistence,tool,controller
Files:
cli/cmd/new.go
cli/cmd/status.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/status.go:statuscommand accepts flags--watch/-w,--interval,--wide,--no-trunc,--services,--check
synthorg statusrenders a verdict banner (OK / DEGRADED / CRITICAL) computed bycomputeVerdict()incli/cmd/status.go; CRITICAL wins over DEGRADED; signals are gated on install expectations so internal-bus install is not flagged DEGRADED merely because health response omits message_bus
Files:
cli/cmd/status.go
cli/cmd/cleanup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cleanupcommand accepts flags--dry-run,--all,--keep N
Files:
cli/cmd/cleanup.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
cli/cmd/wipe.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
wipecommand accepts--dry-run,--no-backup,--keep-imagesflags
Files:
cli/cmd/wipe.go
cli/cmd/uninstall.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
uninstallcommand accepts--keep-dataand--keep-imagesflags
Files:
cli/cmd/uninstall.go
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define
Default{Postgres,NATS}ImageTagand matchingDefault{Postgres,NATS}ImageDigestconstants incli/internal/config/state.gowith a// renovate:annotation on the tag constant for Renovate customManager to keep both tag and digest current
Files:
cli/internal/config/state.go
cli/cmd/backup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/backup.go:backup createcommand accepts flags--output/-oand--timeout
backup listcommand accepts flags--limit/-nand--sort(newest|oldest|size)
backup restorecommand accepts flags--confirm(required),--dry-run,--no-restart,--timeout
Files:
cli/cmd/backup.go
🔇 Additional comments (22)
cli/cmd/version.go (1)
23-23: LGTM!cli/cmd/backup_test.go (1)
247-250: LGTM!cli/cmd/config_changelog_view_test.go (1)
28-28: LGTM!Also applies to: 64-64, 86-86, 109-109
cli/internal/ui/livebox.go (1)
118-129: LGTM!Also applies to: 131-144
cli/internal/selfupdate/walk.go (1)
64-64: LGTM!Also applies to: 81-101
cli/internal/config/state_bench_test.go (1)
40-43: LGTM!cli/cmd/worker_start.go (1)
212-212: LGTM!Also applies to: 222-229
cli/.golangci.yml (1)
46-55: LGTM!cli/cmd/config_tunables_test.go (1)
55-55: LGTM!Also applies to: 102-102, 137-137
cli/internal/config/state_test.go (1)
378-382: LGTM!cli/internal/config/changelog_view_test.go (1)
54-57: LGTM!Also applies to: 62-63, 69-70, 76-77, 83-83
cli/internal/ui/commitlist_walk.go (1)
125-125: LGTM!Also applies to: 133-137, 146-153, 158-182
cli/internal/diagnostics/collect.go (1)
193-198: LGTM!Also applies to: 200-210
cli/internal/docker/client.go (1)
149-178: LGTM!Also applies to: 180-191, 193-204
cli/cmd/config_tunables_dispatch.go (1)
15-92: LGTM!Also applies to: 97-116
cli/cmd/config_tunables.go (1)
19-29: LGTM!Also applies to: 35-40, 47-52, 57-59
cli/cmd/new.go (1)
68-105: LGTM!Also applies to: 107-162
cli/cmd/init_tui_view.go (1)
16-73: LGTM!Also applies to: 97-247, 249-456
cli/cmd/init_tui.go (1)
221-221: LGTM!Also applies to: 276-395
cli/internal/verify/provenance.go (1)
100-105: LGTM!Also applies to: 107-139, 141-161
cli/cmd/init_helpers.go (1)
22-46: LGTM!Also applies to: 52-84, 88-183
cli/cmd/update_cleanup.go (1)
73-85: LGTM!Also applies to: 87-113
… gemini, 1 ci-bench) CI fixes: - restore CLI bench regression budget: hoist Validate / validateTunables check slices to package vars and replace closure-based bindings in resolveDurationTunables / resolveCountTunables with direct pointer setters; unroll per-section enumCheck / formatCheck / duration / byte slices in validate.go to inline if-chains. The closure + per-call slice allocations had pushed BenchmarkResolveTunables +32% and BenchmarkLoadExisting +57% allocs. Security alerts (3 dismissed): - CodeQL #515 cli/cmd/start.go:147 (false positive; safeDir comes from safeStateDir -> config.SecurePath, sanitiser cannot be traced past the assertComposeExists helper boundary) - CodeQL #516 cli/cmd/wipe.go:175 (same shape on requireComposeFile) - CodeQL #517 cli/internal/completion/install.go:381 (false positive; path is resolved from a fixed allowlist of shell config locations under the operator home dir, which is the entire point of completion uninstall) Reviewer feedback: - cleanup.go: collectCleanupCandidates now takes the existing *ui.UI instead of re-creating one; removeOldImages signature widened to (removed, freedB, hardFailures, ctxErr) so non "in use" docker rmi failures and ctx cancellation both surface as runtime errors - update_cleanup.go: runAutoCleanupRemovals returns ctxErr; reinstall next-step hint upgraded to HintNextStep - config.go: hintComposeRestart uses HintNextStep - config_dispatch.go: color/hints/output/timestamps readers now return the effective default (auto/auto/text/relative) instead of empty - doctor.go: classifyDoctorIssues honours --checks for the unfixable bucket (anyFixableCheckEnabled gate) - new.go: warnPartialScaffoldWrite emits the recovery hint via HintError so it survives every mode except --quiet - start.go: emitFineTuneSizeHint uses HintGuidance instead of HintTip - uninstall.go: GitHub releases reinstall hint + "container images still on disk" hint upgraded to HintNextStep - completion/install.go: probeShellProfile rel-check no longer rejects filenames that lexically start with ".." (e.g. "..config/profile.ps1") - compose/validate.go: validateDigestPins sorts keys before iterating so the returned error is deterministic - scaffold/writer.go: resolveOneTarget now resolves the deepest existing ancestor via EvalSymlinks and re-checks containment, so a symlinked subpath under absRoot cannot escape at write time - verify/dhi.go: readCosignPayload reads maxBundleBytes+1 and rejects oversize payloads explicitly (mirrors readAttestationStatement) - config/validate.go: validateMasterKey rejects an empty MasterKey when EncryptSecrets is true, exported as ErrMissingMasterKey; introduced LoadAllowMissingMasterKey + ValidateAllowMissingMasterKey so handleReinit can still recover an install whose persisted config predates the new invariant SKIP (verified factually wrong against current code): - Gemini "critical" errors.AsType typo on cli/cmd/update.go:360 -- Go 1.25+ stdlib added a generic errors.AsType helper; build + CI tests pass on Go 1.26 in this repo. golangci-lint actually surfaces the modernisation hint that callers SHOULD migrate to AsType. Tests updated where required: - cmd/* tests, config/* tests: explicit `state.EncryptSecrets = false` (or `encrypt_secrets: false` in JSON fixtures) where the test targets non-encryption behaviour, since the new MasterKey invariant rejects the DefaultState() baseline that those fixtures rely on. Issue: #2099
…ff + 1 cr dup + 3 ci platform regressions) CR re-review on d8fca4e (CHANGES_REQUESTED, second pass): Inline: - cmd/doctor.go: runDoctor returns actual fix outcomes (composeFixed || restartDone) instead of the intent flags (needComposeFix || needRestart); runDoctorComposeFix / runDoctorRestart now return bool. - cmd/doctor.go: classifyDoctorIssues now routes EVERY kind (fixable and unfixable) through doctorCheckEnabled(category) via a per-issue classification that carries both kind and originating --checks category; anyFixableCheckEnabled deleted. Table-driven doctorIssuePatterns + matchesDoctorIssue helper keep classifyDoctorIssue under the cyclomatic-complexity ceiling. - cmd/init_helpers.go: reuseExistingStateForInteractive catches ErrMissingMasterKey and falls back to LoadAllowMissingMasterKey, mirroring the non-interactive recovery in handleReinit. - cmd/root.go: persisted output=json no longer overrides an explicit --plain; the condition now also gates on !flagPlain && !opts.Plain. - cmd/start.go: buildPullItems drops state.VerifiedDigests for the standalone (sandbox / sidecar / fine-tune) refs when ANY registry / image-tag override is active on State (stateHasRegistryOverrides), preventing default-registry digest pins from being applied to an override-registry pull. - cmd/uninstall.go: a broken on-disk config no longer aborts teardown; Load failure logs a warning and falls back to a sanitised State seeded with --data-dir so safeStateDir still has somewhere to resolve. - cmd/wipe.go: removeDataDirectory traversal guard now splits on path separators and matches whole ".." elements instead of substring "..", so names like "/var/lib/synthorg..bak" are not rejected. - internal/completion/install.go: probeShellProfile normalises both resolvedHome and resolved via normalizePathForCompare before filepath.Rel, lowercasing on Windows so drive-letter casing mismatches do not produce a spurious "..\\..\\..." relative path. - internal/compose/validate.go: DockerSockGID upper-bound comparison cast to int64 so the 4294967295 ceiling is representable on 32-bit builds where int is 32 bits. - internal/docker/client.go: parseSemverComponents rejects non-empty components that contain no digit run (e.g. "1.x.0" -> "x") so a malformed input no longer silently coerces to 0.0.0; empty components ("1." or "") still count as 0 for compatibility. - internal/selfupdate/updater.go: isUsableRelease actually parses the tag via compareSemver(base, base) on the dev-stripped base; the prior compareWithDev(tag, tag) self-compare always returned 0 with no error and let malformed tags leak into pickNewerRelease. compareSemver parser extracted to parseSemverComponent (package-level helper) so compareSemver stays under the cognitive-complexity ceiling. Outside diff: - cmd/update.go: split-update follow-up hints (after --cli-only / --images-only) switched from HintGuidance to HintNextStep so they are emitted as next-actions, not flag/feature discovery. CI platform regressions surfaced by my round 2 changes: - internal/scaffold/writer.go: resolveOneTarget now resolves BOTH absRoot and the deepest existing ancestor of the candidate via EvalSymlinks before the containment check. macOS (/var -> /private/var) and Windows (temp-dir junctions) were rejecting every otherwise-legitimate write because absRoot stayed unresolved while the parent did not. - internal/config/state_bench_test.go: BenchmarkLoadExisting fixture now sets EncryptSecrets=false, since DefaultState() has EncryptSecrets=true and the round-2 master_key invariant otherwise rejects the bench fixture. CR outside-diff from round 1 review (missed in round 2): - cmd/init.go: non-interactive Example string now includes --log-level info, matching initAllFlagsSet()'s required-flag set. Issue: #2099
…n real-fix + workflow CANCELLED) CI / perf: - cli internal/config: restore LoadExisting + ResolveTunables to their pre-refactor alloc budgets. Round 2's checkEnumRequired refactor evaluated sortedKeys(validX) eagerly on every Validate call (the original code only built that string inside the fmt.Errorf on the ERROR path); the bench saw ~20 extra slice/string allocs per Load. Fix is to memoise every Names accessor (PersistenceBackendNames, ChannelNames, etc.) at package init -- the maps are immutable so the sorted-string form can be cached once. Round 3's durationBindings table forced a 208-byte State copy onto the heap per ResolveTunables call because the indirect closure call defeated escape analysis; fix is a stack-local array of plain-data binding rows (no closures, no function pointers) so the loop's resolveDurationField call is direct and State stays on stack. - .github/workflows/evals.yml: report-failure job now uses `always() && ...` so the runner evaluates the full condition even when the agent-evals dependency is skipped. Without always(), the default success() short-circuit produced a CANCELLED status on every PR run that did not carry the run-evals label, polluting the PR rollup with a fake failure on every push. CR re-review on d64cdcb (3rd round, CHANGES_REQUESTED): Inline: - cmd/cleanup.go: `--all includes current images. Running containers will prevent removal.` upgraded HintGuidance -> HintNextStep. The `Use --keep N` hint at line 244 stays HintGuidance (feature discovery, per the cli/CLAUDE.md hint-tier table). - cmd/config.go + cmd/config_dispatch.go: new configGetDisplays map layered on top of configReaders. `config get fine_tuning_variant` routes through configGetDisplayValue and resolves to FineTuneVariantOrDefault() (effective "gpu" when unset); `config list` still uses the raw configReaders so the source comparison can distinguish an explicit "gpu" from an unset field. - cmd/doctor.go: status, summary, and auto-fix now consume a filterReportByDoctorChecks(report) copy so a `synthorg doctor --checks=compose` verdict only reflects compose-category findings. renderDoctorFiltered keeps using the unfiltered report because IT already gates per-section rendering on doctorCheckEnabled. - cmd/root.go: new init-specific errorHintRule precedes the generic "requires an interactive terminal" rule. The generic "Use --yes" hint is misleading for init (which accepts --yes but still needs --backend-port / --web-port / --sandbox / --log-level); the new rule lists the optional flags init also supports for scripted installs. - cmd/wipe.go: runWipe now branches on wipeDryRun BEFORE loading state, via the new loadWipeStateForPreview helper. A half-installed install (no compose.yml, unreadable config) can now be inspected with `synthorg wipe --dry-run` instead of erroring out at load. - cmd/wipe.go: removeDataDirectory guard also rejects bare VolumeName(safeDir) (no trailing separator). Catches UNC share roots like `\\\\server\\share` that filepath.Clean normalises by stripping the trailing separator, which previously slipped past the volume + separator check and would have let removeDataDirExceptSelf recurse over the entire share. - internal/completion/install.go: probeShellProfile filepath.Rel inputs go through normalizePathForCompare so Windows drive-letter casing mismatches do not produce spurious ".." prefixes. - internal/config/state.go: ValidateAllowMissingMasterKey now calls validateMasterKey explicitly and only ignores the specific ErrMissingMasterKey error. Other failures (e.g. a non-empty but malformed Fernet master key) still surface so the recovery path cannot leak bad key material through. - internal/scaffold/writer.go: resolveOneTarget no longer rejects zero-byte file contents. Legitimate scaffold output (e.g. an empty __init__.py marker) flows through; the matching test inverted from TestWriteRejectsEmptyContent to TestWriteAcceptsEmptyContent. Outside diff: - cmd/status.go: trailing "Run 'synthorg logs' to view container logs" message upgraded HintTip -> HintNextStep (matches the doctor-footer hint above it and the natural-next-action tier contract). Duplicate (cross-round): - cmd/start.go: stateHasRegistryOverrides ALSO checks the env vars (SYNTHORG_REGISTRY_HOST, SYNTHORG_IMAGE_REPO_PREFIX, SYNTHORG_DHI_REGISTRY, SYNTHORG_POSTGRES_IMAGE_TAG, SYNTHORG_NATS_IMAGE_TAG). Round 3's State-only check missed one-shot env overrides that never land on disk. pullAllImages now emits the mandatory verification-disabled stderr warning (unconditional, not gated by --quiet / --json) when any override is active. - internal/selfupdate/updater.go: isUsableRelease now validates the dev-stripped base against a strict MAJOR.MINOR.PATCH regex before calling compareSemver. compareSemver alone accepts "release-1.2.3" (each dotted component has a digit run); the regex filters non-semver shapes out of the auto-update candidate set. Issue: #2099
Aureliolo
force-pushed
the
refactor/cli-complexity-ratchet
branch
from
d64cdcb to
cd9ea3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/cmd/doctor.go (1)
508-512:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContainer warning leaks into filtered status.
On Line 510,
"no containers detected"is emitted purely fromlen(r.ContainerSummary)==0 && r.ComposeFileExists, so--checks=composecan still degrade status with a containers warning afterContainerSummarywas intentionally filtered out.As per coding guidelines, "Filter
synthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag".Proposed minimal fix
func collectDoctorWarnings(r diagnostics.Report) []string { var warnings []string - if len(r.ContainerSummary) == 0 && r.ComposeFileExists { + if doctorCheckEnabled("containers") && len(r.ContainerSummary) == 0 && r.ComposeFileExists { warnings = append(warnings, "no containers detected") } for _, c := range r.ContainerSummary { if c.Health == "starting" { warnings = append(warnings, fmt.Sprintf("%s still starting", c.Name)) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/doctor.go` around lines 508 - 512, The "no containers detected" warning in collectDoctorWarnings is being emitted even when container data was filtered out; change the condition so the warning is only appended when ContainerSummary was actually part of the (unfiltered) report. Update collectDoctorWarnings to require len(r.ContainerSummary) == 0 && r.ComposeFileExists && <report includes container check>, e.g. check an existing filter indicator like r.FilteredChecks/ r.FilteredCategories or a method such as r.HasCategory("containers") (or add a small boolean flag on diagnostics.Report if none exists) so the warning is skipped when container info was intentionally filtered out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/internal/config/validate.go`:
- Around line 54-57: validateBackends is comparing State.DockerSockGID (an int)
to the untyped constant 4294967295 which breaks on 32-bit builds; fix by
converting the int to a wider integer type before comparing (e.g. use
int64(s.DockerSockGID) in both comparisons or compare against
int64(4294967295)), so update the conditional in validateBackends (referencing
DockerSockGID and validateBackends) to cast s.DockerSockGID to int64 (or use
math.MaxUint32 cast to int64) before comparing.
In `@cli/internal/scaffold/writer.go`:
- Around line 163-167: The precheck uses os.Stat on abs (in rejectExisting),
which follows symlinks and misses dangling symlinks; replace the os.Stat call
with os.Lstat and treat any existing file or symlink as present (i.e., if Lstat
returns nil or the returned FileInfo has Mode()&os.ModeSymlink != 0 treat it as
"target already exists"), and preserve the existing error-path logic (if
!os.IsNotExist(err) return a wrapped error for unexpected errors). Ensure you
update the check that returns fmt.Errorf("target already exists: %s", abs) to
trigger for symlinks as well and keep the "checking %s: %w" wrapping for other
errors.
---
Duplicate comments:
In `@cli/cmd/doctor.go`:
- Around line 508-512: The "no containers detected" warning in
collectDoctorWarnings is being emitted even when container data was filtered
out; change the condition so the warning is only appended when ContainerSummary
was actually part of the (unfiltered) report. Update collectDoctorWarnings to
require len(r.ContainerSummary) == 0 && r.ComposeFileExists && <report includes
container check>, e.g. check an existing filter indicator like r.FilteredChecks/
r.FilteredCategories or a method such as r.HasCategory("containers") (or add a
small boolean flag on diagnostics.Report if none exists) so the warning is
skipped when container info was intentionally filtered out.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6d48af4-5ca6-4d03-8009-cc70b04963a4
📒 Files selected for processing (50)
.github/workflows/evals.ymlcli/.golangci.ymlcli/cmd/backup.gocli/cmd/backup_test.gocli/cmd/cleanup.gocli/cmd/config.gocli/cmd/config_changelog_view_test.gocli/cmd/config_dispatch.gocli/cmd/config_ext_test.gocli/cmd/config_test.gocli/cmd/config_tunables.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_tunables_test.gocli/cmd/doctor.gocli/cmd/init.gocli/cmd/init_helpers.gocli/cmd/init_tui.gocli/cmd/init_tui_view.gocli/cmd/new.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/cmd/update_cleanup.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/cmd/version.gocli/cmd/wipe.gocli/cmd/worker_start.gocli/internal/completion/install.gocli/internal/compose/generate.gocli/internal/compose/validate.gocli/internal/config/changelog_view_test.gocli/internal/config/state.gocli/internal/config/state_bench_test.gocli/internal/config/state_test.gocli/internal/config/tunables.gocli/internal/config/validate.gocli/internal/diagnostics/collect.gocli/internal/docker/client.gocli/internal/scaffold/writer.gocli/internal/scaffold/writer_test.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/walk.gocli/internal/ui/commitlist_walk.gocli/internal/ui/livebox.gocli/internal/ui/walk.gocli/internal/verify/dhi.gocli/internal/verify/provenance.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: CLI Bench Regression
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (22)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/cmd/backup_test.gocli/internal/config/state_test.gocli/cmd/version.gocli/internal/config/state_bench_test.gocli/cmd/config_tunables_test.gocli/internal/scaffold/writer_test.gocli/internal/diagnostics/collect.gocli/internal/config/changelog_view_test.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/internal/ui/livebox.gocli/internal/docker/client.gocli/cmd/config_changelog_view_test.gocli/internal/selfupdate/walk.gocli/cmd/config_dispatch.gocli/internal/ui/commitlist_walk.gocli/cmd/new.gocli/cmd/init.gocli/cmd/init_tui.gocli/cmd/doctor.gocli/internal/selfupdate/updater.gocli/internal/ui/walk.gocli/cmd/config_tunables.gocli/cmd/update_cleanup.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_ext_test.gocli/cmd/worker_start.gocli/cmd/config_test.gocli/internal/verify/dhi.gocli/internal/scaffold/writer.gocli/internal/config/validate.gocli/cmd/init_helpers.gocli/internal/config/state.gocli/cmd/config.gocli/internal/compose/validate.gocli/cmd/status.gocli/internal/verify/provenance.gocli/cmd/root.gocli/cmd/uninstall.gocli/cmd/cleanup.gocli/cmd/backup.gocli/cmd/start.gocli/internal/config/tunables.gocli/cmd/init_tui_view.gocli/internal/completion/install.gocli/cmd/wipe.gocli/internal/compose/generate.gocli/cmd/update.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/backup_test.gocli/cmd/version.gocli/cmd/config_tunables_test.gocli/cmd/update_compose.gocli/cmd/verify_pipeline.gocli/cmd/config_changelog_view_test.gocli/cmd/config_dispatch.gocli/cmd/new.gocli/cmd/init.gocli/cmd/init_tui.gocli/cmd/doctor.gocli/cmd/config_tunables.gocli/cmd/update_cleanup.gocli/cmd/config_tunables_dispatch.gocli/cmd/config_ext_test.gocli/cmd/worker_start.gocli/cmd/config_test.gocli/cmd/init_helpers.gocli/cmd/config.gocli/cmd/status.gocli/cmd/root.gocli/cmd/uninstall.gocli/cmd/cleanup.gocli/cmd/backup.gocli/cmd/start.gocli/cmd/init_tui_view.gocli/cmd/wipe.gocli/cmd/update.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/state_test.gocli/internal/config/state_bench_test.gocli/internal/config/changelog_view_test.gocli/internal/verify/dhi.gocli/internal/config/validate.gocli/internal/config/state.gocli/internal/verify/provenance.gocli/internal/config/tunables.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/state_test.gocli/internal/config/state_bench_test.gocli/internal/config/changelog_view_test.gocli/internal/config/validate.gocli/internal/config/state.gocli/internal/config/tunables.go
cli/cmd/version.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
versioncommand accepts--shortflag
Files:
cli/cmd/version.go
cli/**/*_bench_test.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the modern
for b.Loop()pattern (Go 1.24+) in*_bench_test.gofiles with Go's standardtesting.Bframework
Files:
cli/internal/config/state_bench_test.go
cli/internal/**/*_bench_test.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Place
*_bench_test.gofiles next to their*_test.gosiblings undercli/internal/<pkg>/
Files:
cli/internal/config/state_bench_test.go
cli/cmd/new.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
new <kind> <domain>command accepts--dry-runand--overwriteflags and scaffolds a conventions-clean Python file set undersrc/synthorg/for a new feature;<kind>is one ofservice,persistence,tool,controller
Files:
cli/cmd/new.go
cli/cmd/init.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/init.go:initcommand accepts flags--backend-port,--web-port,--sandbox,--log-level(required for non-interactive mode) and optional--image-tag,--channel,--bus-backend,--persistence-backend,--postgres-port,--encrypt-secrets("true" or "false", default "true")
Interactiveinitdefaults to Postgres + NATS; non-interactive defaults to SQLite + internal bus
Files:
cli/cmd/init.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
cli/internal/verify/dhi.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
cli/internal/verify/dhi.goin sync withDefault{Postgres,NATS}Image{Tag,Digest}constants by derivingdhiPinnedIndexDigestsmap from these constants at init
Files:
cli/internal/verify/dhi.go
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define
Default{Postgres,NATS}ImageTagand matchingDefault{Postgres,NATS}ImageDigestconstants incli/internal/config/state.gowith a// renovate:annotation on the tag constant for Renovate customManager to keep both tag and digest current
Files:
cli/internal/config/state.go
cli/cmd/config.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Implement
synthorg config <subcommand>to exposeshow,get <key>,set <key> <value>,unset <key>,list,path, andeditwith 40 settable keys (backend_port, web_port, sandbox, image_tag, log_level, fine_tuning, telemetry_opt_in, channel, plus tunables)
Files:
cli/cmd/config.go
cli/internal/compose/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Every generated
compose.ymlincludes a one-shotdata-inithelper that chowns each named volume to its non-root owner (65532:65532 for backend/NATS, 70:70 with mode 0700 for Postgres) before stateful services start; Postgres/NATS services declaredepends_on: data-init: condition: service_completed_successfully
Files:
cli/internal/compose/validate.gocli/internal/compose/generate.go
cli/cmd/status.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/status.go:statuscommand accepts flags--watch/-w,--interval,--wide,--no-trunc,--services,--check
synthorg statusrenders a verdict banner (OK / DEGRADED / CRITICAL) computed bycomputeVerdict()incli/cmd/status.go; CRITICAL wins over DEGRADED; signals are gated on install expectations so internal-bus install is not flagged DEGRADED merely because health response omits message_bus
Files:
cli/cmd/status.go
cli/cmd/uninstall.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
uninstallcommand accepts--keep-dataand--keep-imagesflags
Files:
cli/cmd/uninstall.go
cli/cmd/cleanup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cleanupcommand accepts flags--dry-run,--all,--keep N
Files:
cli/cmd/cleanup.go
cli/cmd/backup.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/backup.go:backup createcommand accepts flags--output/-oand--timeout
backup listcommand accepts flags--limit/-nand--sort(newest|oldest|size)
backup restorecommand accepts flags--confirm(required),--dry-run,--no-restart,--timeout
Files:
cli/cmd/backup.go
cli/cmd/start.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
startcommand accepts flags--no-wait,--timeout,--no-pull,--dry-run,--no-detach,--no-verify
Files:
cli/cmd/start.go
cli/cmd/wipe.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
wipecommand accepts--dry-run,--no-backup,--keep-imagesflags
Files:
cli/cmd/wipe.go
cli/internal/compose/generate.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Port layout: 3000 web / 3001 backend / 3002 postgres / 3003 NATS client;
generate.govalidates port collisions across all enabled services
Files:
cli/internal/compose/generate.go
cli/cmd/update.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
updatecommand accepts flags--dry-run,--no-restart,--timeout,--cli-only,--images-only,--check
Files:
cli/cmd/update.go
🔇 Additional comments (103)
cli/cmd/backup_test.go (1)
247-250: LGTM!cli/internal/config/state_test.go (1)
378-382: LGTM!cli/cmd/version.go (1)
23-23: LGTM!.github/workflows/evals.yml (1)
66-72: LGTM!cli/.golangci.yml (1)
46-55: LGTM!cli/internal/config/state_bench_test.go (1)
40-43: LGTM!cli/cmd/config_tunables_test.go (1)
55-55: LGTM!Also applies to: 102-102, 137-137
cli/internal/scaffold/writer_test.go (1)
92-112: LGTM!cli/internal/diagnostics/collect.go (1)
193-198: LGTM!Also applies to: 200-210
cli/internal/config/changelog_view_test.go (1)
54-57: LGTM!Also applies to: 62-62, 69-69, 76-76, 83-83
cli/cmd/update_compose.go (1)
116-121: LGTM!Also applies to: 127-148
cli/cmd/verify_pipeline.go (1)
53-67: LGTM!Also applies to: 69-110, 112-128
cli/internal/ui/livebox.go (1)
119-129: LGTM!Also applies to: 131-144
cli/internal/docker/client.go (1)
149-178: LGTM!Also applies to: 180-204
cli/cmd/config_changelog_view_test.go (1)
28-28: LGTM!Also applies to: 64-64, 86-86, 109-109
cli/internal/selfupdate/walk.go (1)
64-64: LGTM!Also applies to: 81-101
cli/cmd/config_tunables.go (1)
19-29: LGTM!Also applies to: 35-40, 47-52, 57-59
cli/cmd/update_cleanup.go (1)
73-85: LGTM!Also applies to: 87-113
cli/cmd/config_tunables_dispatch.go (1)
1-117: LGTM!cli/cmd/config_ext_test.go (1)
26-27: LGTM!Also applies to: 54-55, 75-76, 104-105, 131-132, 159-160, 188-189, 217-218, 240-241, 271-272, 302-303, 349-350, 388-389, 416-417, 444-445, 463-464, 489-490, 538-539, 599-600, 634-635
cli/cmd/worker_start.go (1)
212-229: LGTM!cli/cmd/config_test.go (1)
122-123: LGTM!Also applies to: 149-150, 187-188, 218-219, 255-256, 280-281, 301-302, 347-348, 375-376, 414-415, 439-440, 501-502, 525-526, 561-565, 589-590
cli/internal/verify/dhi.go (1)
20-20: LGTM!Also applies to: 304-312, 314-329, 331-352, 354-376, 380-411, 413-438, 440-481, 483-501
cli/internal/config/validate.go (5)
1-34: LGTM!
76-116: LGTM!
118-148: LGTM!
150-193: LGTM!
194-285: LGTM!cli/cmd/init_helpers.go (4)
1-46: LGTM!
48-84: LGTM!
86-130: LGTM!
132-183: LGTM!cli/internal/config/state.go (7)
248-284: LGTM!
304-360: LGTM!
373-391: LGTM!
399-480: LGTM!
482-515: LGTM!
517-536: LGTM!
560-580: LGTM!cli/cmd/config.go (5)
369-401: LGTM!
457-489: LGTM!
491-502: LGTM!
628-678: LGTM!
753-764: LGTM!cli/internal/compose/validate.go (5)
1-77: LGTM!
79-112: LGTM!
114-160: LGTM!
162-196: LGTM!
198-212: LGTM!cli/cmd/status.go (6)
69-108: LGTM!
421-458: LGTM!
460-527: LGTM!
529-538: LGTM!
591-656: LGTM!
698-698: LGTM!cli/internal/verify/provenance.go (2)
99-139: LGTM!
141-161: LGTM!cli/cmd/root.go (4)
258-283: LGTM!
350-367: LGTM!
397-420: LGTM!
422-447: LGTM!cli/cmd/uninstall.go (5)
60-88: LGTM!
90-118: LGTM!
120-143: LGTM!
145-158: LGTM!Also applies to: 271-321
503-562: LGTM!cli/cmd/cleanup.go (4)
67-90: LGTM!
92-138: LGTM!
157-199: LGTM!
201-246: LGTM!cli/cmd/backup.go (4)
285-331: LGTM!
333-349: LGTM!
415-418: LGTM!Also applies to: 496-537
574-640: LGTM!cli/cmd/start.go (6)
68-116: LGTM!
118-161: LGTM!
223-271: LGTM!
326-439: LGTM!
441-505: LGTM!
598-601: LGTM!cli/internal/config/tunables.go (3)
101-149: LGTM!
151-234: LGTM!
337-419: LGTM!cli/cmd/init_tui_view.go (4)
15-73: LGTM!
97-234: LGTM!
236-361: LGTM!
363-456: LGTM!cli/internal/completion/install.go (2)
213-303: LGTM!
387-464: LGTM!cli/cmd/wipe.go (5)
94-129: LGTM!
131-222: LGTM!
224-294: LGTM!
296-353: LGTM!
501-502: LGTM!cli/internal/compose/generate.go (4)
285-288: LGTM!
290-305: LGTM!
307-324: LGTM!
326-344: LGTM!cli/cmd/update.go (5)
119-119: LGTM!Also applies to: 127-127
366-381: LGTM!
383-431: LGTM!
622-622: LGTM!
358-358: ⚡ Quick winGo version supports
errors.AsType—cli/go.modsetsgo 1.26.3(≥ 1.23), soerrors.AsType[*exec.ExitError](runErr)is supported.
CLI Bench Regression, second root cause (the +1 alloc / 208 B/op
that survived round 4):
- internal/config/tunables.go: resolveRegistryTunables had a per-call
`[]struct{name, value, valid}` literal that escaped the range loop
to the heap (~208 B/op, 1 alloc per ResolveTunables call). Unrolled
to direct if-chain via validateResolvedRegistryFields. The function
values are already package-level (IsValidRegistryHost etc.); the
slice header was the sole alloc source. Round-4's caching of the
Names accessors took LoadExisting back to its main-baseline alloc
budget (21 allocs, no delta); this finishes the job for
ResolveTunables.
CR re-review on cd9ea3e (4th round):
- internal/config/validate.go: validateBackends DockerSockGID bound
check cast to int64. The compose/validate.go equivalent was already
fixed in round 3; this is the same shape on the persisted-State
validator (32-bit Go build cannot represent 4294967295 in a signed
int).
- internal/scaffold/writer.go: rejectExisting now uses os.Lstat
instead of os.Stat. os.Stat follows symlinks, so a dangling symlink
at the target path returned ErrNotExist and let the subsequent
write blow the symlink away without warning; Lstat treats the
symlink itself as present and triggers the "target already exists"
rejection.
- cmd/doctor.go: collectDoctorWarnings only emits "no containers
detected" when doctorCheckEnabled("containers"). The
filterReportByDoctorChecks copy added in round 4 zeroes
ContainerSummary when the operator scopes --checks away from
containers; without this gate the warning fired anyway, surfacing
a finding from an excluded category.
Issue: #2099
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/doctor.go (1)
510-530:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep compose warnings out of non-compose runs.
Line 529 still appends
compose.yml exists, validity not checkedeven when--checksexcludescompose. BecausefilterReportByDoctorChecks()deliberately setsComposeFileExists=trueandComposeFileValid=nilfor excluded compose, a run likesynthorg doctor --checks healthis still downgraded by a filtered-out category.Proposed fix
- if r.ComposeFileExists && r.ComposeFileValid == nil { + if doctorCheckEnabled("compose") && r.ComposeFileExists && r.ComposeFileValid == nil { warnings = append(warnings, "compose.yml exists, validity not checked") }As per coding guidelines, "Filter
synthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/doctor.go` around lines 510 - 530, The compose warning is emitted even when the compose category was filtered out; update the condition that appends "compose.yml exists, validity not checked" to also require the compose check be enabled by calling doctorCheckEnabled("compose") (i.e. only append when doctorCheckEnabled("compose") && r.ComposeFileExists && r.ComposeFileValid == nil) so filtered reports don't surface compose findings; you may also audit filterReportByDoctorChecks and usages of r.ComposeFileExists / r.ComposeFileValid to ensure no other compose-only warnings ignore doctorCheckEnabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cli/cmd/doctor.go`:
- Around line 510-530: The compose warning is emitted even when the compose
category was filtered out; update the condition that appends "compose.yml
exists, validity not checked" to also require the compose check be enabled by
calling doctorCheckEnabled("compose") (i.e. only append when
doctorCheckEnabled("compose") && r.ComposeFileExists && r.ComposeFileValid ==
nil) so filtered reports don't surface compose findings; you may also audit
filterReportByDoctorChecks and usages of r.ComposeFileExists /
r.ComposeFileValid to ensure no other compose-only warnings ignore
doctorCheckEnabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44f4253f-6839-4d56-bbe3-0b143bb2b5fa
📒 Files selected for processing (4)
cli/cmd/doctor.gocli/internal/config/tunables.gocli/internal/config/validate.gocli/internal/scaffold/writer.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). (5)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: CLI Bench Regression
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (5)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/cmd/doctor.gocli/internal/config/tunables.gocli/internal/scaffold/writer.gocli/internal/config/validate.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/doctor.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/tunables.gocli/internal/config/validate.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/tunables.gocli/internal/config/validate.go
🔇 Additional comments (3)
cli/internal/config/tunables.go (1)
123-137: LGTM!Also applies to: 139-164
cli/internal/scaffold/writer.go (1)
161-168: LGTM!cli/internal/config/validate.go (1)
55-60: LGTM!
CLI Bench Regression, third root cause (the +1 alloc / +208 B/op on
ResolveTunables that survived rounds 2-5):
Verified end-to-end against the actual scripts/check_cli_bench_regression.sh
A/B run on this dev machine before push (per operator request to stop
trusting spot benches). Local A/B: ResolveTunables 39 -> 38 allocs
(5416 -> 5208 B/op), reversing exactly the +1 alloc / +208 B/op the
CI gate was flagging.
Root cause: passing &t across helper boundaries (resolveRegistryTunables,
resolveDurationTunables, resolveCountTunables took *Tunables) forced
escape analysis to heap-allocate the ~208-byte Tunables struct on every
ResolveTunables call. Main keeps t on stack by being one flat ~115-line
function with no helpers taking the address. Round-5's attempt
(unrolling the resolveRegistryTunables slice) addressed a different
symptom but did not fix the *Tunables-driven escape.
Fix: rewrite helpers as value-pass: each takes `Tunables` by value, returns
`(Tunables, error)`. Caller reassigns t = helper(t, s). The inner
resolveDurationField / resolveBytesField helpers also switch from
pointer-out (*time.Duration / *int64) to value-return, so the binding
table that previously held &t.X pointers (which forced t back to heap)
is gone -- direct assignment per field instead.
Each value-pass copy is a ~208-byte stack memcpy: free relative to a
heap allocation. compile-with-gcflags=-m confirms `moved to heap: t`
is no longer emitted for any tunables.go path.
Function-complexity ceiling kept by splitting resolveDurationTunables
into 3 helpers (base + resolveSelfUpdateAndTUFTimeouts +
resolveImageTimeouts), each <50 lines and <10 cyclomatic.
CR re-review on round-5 head:
- cmd/doctor.go: collectDoctorWarnings now also gates the
"compose.yml exists, validity not checked" warning behind
doctorCheckEnabled("compose"), matching the "no containers" gate
from round 4. The filterReportByDoctorChecks copy forces
ComposeFileExists=true and clears ComposeFileValid when compose is
scoped out, so the warning fired anyway. Split into 4 helpers
(doctorNoContainersWarning, doctorContainerStartingWarnings,
doctorImageStatusWarnings, doctorComposeValidityWarning) to keep
cyclomatic complexity under 10 with the new gate.
NOT added (per operator decision): the CLI bench A/B regression
script as a pre-push gate. End-to-end runtime is ~2 minutes
(benchstat install + 10-iteration capture x 2 for the A/B compare),
which is too much friction per push for the catch rate it provides.
The CI gate stays as the single source of truth.
Issue: #2099
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/doctor.go (1)
139-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
HintNextStepfor the post-fix verification message.
Run 'synthorg doctor' again to verify fixes.is a natural follow-up after--fix, soHintGuidancecan hide it in the default auto mode. This should useHintNextStepinstead.Suggested change
- out.HintGuidance("Run 'synthorg doctor' again to verify fixes.") + out.HintNextStep("Run 'synthorg doctor' again to verify fixes.")As per coding guidelines,
cli/**/*.go: Use the appropriate hint tier based on intent:HintNextStepfor natural next action or destructive-action feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/doctor.go` around lines 139 - 142, The post-fix message currently uses out.HintGuidance after calling doctorAutoFix(ctx, cmd, out, errOut, state, filteredReport, safeDir); change the hint tier to out.HintNextStep for the natural next-action message ("Run 'synthorg doctor' again to verify fixes."), i.e., replace the out.HintGuidance(...) call with out.HintNextStep(...) so the message is shown as the next step rather than guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cli/cmd/doctor.go`:
- Around line 139-142: The post-fix message currently uses out.HintGuidance
after calling doctorAutoFix(ctx, cmd, out, errOut, state, filteredReport,
safeDir); change the hint tier to out.HintNextStep for the natural next-action
message ("Run 'synthorg doctor' again to verify fixes."), i.e., replace the
out.HintGuidance(...) call with out.HintNextStep(...) so the message is shown as
the next step rather than guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 212dd854-a712-4a9f-a65a-7fff94963b74
⛔ Files ignored due to path filters (1)
cli/coverage.outis excluded by!**/*.out
📒 Files selected for processing (2)
cli/cmd/doctor.gocli/internal/config/tunables.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: CLI Bench Regression
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use the appropriate hint tier based on intent:
HintErrorfor error recovery (always visible unless quiet),HintNextStepfor natural next action or destructive-action feedback,HintTipfor config automation suggestions (deduplicates within session), andHintGuidancefor flag/feature discovery (invisible in default auto mode)
Files:
cli/internal/config/tunables.gocli/cmd/doctor.go
cli/internal/{config,verify}/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Overriding any of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/config/tunables.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
When both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present,SYNTHORG_DATABASE_URLwins (Postgres is initialised; SQLite path is ignored); malformed URL raises loudly at startup rather than silently falling back
Files:
cli/internal/config/tunables.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Exit codes: 0 = success, 1 = runtime error, 2 = usage error, 3 = unhealthy (backend/containers), 4 = unreachable (Docker), 10 = updates available (
--check)
Files:
cli/cmd/doctor.go
cli/cmd/doctor.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/cmd/doctor.go:doctorcommand accepts--checksand--fixflags; collects diagnostics, saves timestampedsynthorg-diagnostic-YYYYMMDD-HHMMSS.txt(mode 0600) under data directory, prints summary, optionally auto-fixes with--fix
Doctor report footer printssynthorg doctor report(file a bug report) andsynthorg logs(view container logs) as next-step hints
Filtersynthorg doctorreport to subset of categories with--checks <name>[,<name>...]; keywordallis equivalent to omitting the flag
Doctor checks includeenvironment(Docker/Compose versions, OS, CLI version),health(Backend/api/v1/readyzreachability + verdict),containers(per-service state),images(presence, digest pins, verification cache),compose(validation, port-collision detection),config(resolved keys + override precedence),disk(volume sizes, free space),errors(recent error log lines per service)
Files:
cli/cmd/doctor.go
🔇 Additional comments (2)
cli/internal/config/tunables.go (1)
101-145: LGTM!Also applies to: 151-281, 385-466
cli/cmd/doctor.go (1)
178-224: LGTM!Also applies to: 258-415, 508-574
…ll/verify_pipeline
… gemini, 1 ci-bench) CI fixes: - restore CLI bench regression budget: hoist Validate / validateTunables check slices to package vars and replace closure-based bindings in resolveDurationTunables / resolveCountTunables with direct pointer setters; unroll per-section enumCheck / formatCheck / duration / byte slices in validate.go to inline if-chains. The closure + per-call slice allocations had pushed BenchmarkResolveTunables +32% and BenchmarkLoadExisting +57% allocs. Security alerts (3 dismissed): - CodeQL #515 cli/cmd/start.go:147 (false positive; safeDir comes from safeStateDir -> config.SecurePath, sanitiser cannot be traced past the assertComposeExists helper boundary) - CodeQL #516 cli/cmd/wipe.go:175 (same shape on requireComposeFile) - CodeQL #517 cli/internal/completion/install.go:381 (false positive; path is resolved from a fixed allowlist of shell config locations under the operator home dir, which is the entire point of completion uninstall) Reviewer feedback: - cleanup.go: collectCleanupCandidates now takes the existing *ui.UI instead of re-creating one; removeOldImages signature widened to (removed, freedB, hardFailures, ctxErr) so non "in use" docker rmi failures and ctx cancellation both surface as runtime errors - update_cleanup.go: runAutoCleanupRemovals returns ctxErr; reinstall next-step hint upgraded to HintNextStep - config.go: hintComposeRestart uses HintNextStep - config_dispatch.go: color/hints/output/timestamps readers now return the effective default (auto/auto/text/relative) instead of empty - doctor.go: classifyDoctorIssues honours --checks for the unfixable bucket (anyFixableCheckEnabled gate) - new.go: warnPartialScaffoldWrite emits the recovery hint via HintError so it survives every mode except --quiet - start.go: emitFineTuneSizeHint uses HintGuidance instead of HintTip - uninstall.go: GitHub releases reinstall hint + "container images still on disk" hint upgraded to HintNextStep - completion/install.go: probeShellProfile rel-check no longer rejects filenames that lexically start with ".." (e.g. "..config/profile.ps1") - compose/validate.go: validateDigestPins sorts keys before iterating so the returned error is deterministic - scaffold/writer.go: resolveOneTarget now resolves the deepest existing ancestor via EvalSymlinks and re-checks containment, so a symlinked subpath under absRoot cannot escape at write time - verify/dhi.go: readCosignPayload reads maxBundleBytes+1 and rejects oversize payloads explicitly (mirrors readAttestationStatement) - config/validate.go: validateMasterKey rejects an empty MasterKey when EncryptSecrets is true, exported as ErrMissingMasterKey; introduced LoadAllowMissingMasterKey + ValidateAllowMissingMasterKey so handleReinit can still recover an install whose persisted config predates the new invariant SKIP (verified factually wrong against current code): - Gemini "critical" errors.AsType typo on cli/cmd/update.go:360 -- Go 1.25+ stdlib added a generic errors.AsType helper; build + CI tests pass on Go 1.26 in this repo. golangci-lint actually surfaces the modernisation hint that callers SHOULD migrate to AsType. Tests updated where required: - cmd/* tests, config/* tests: explicit `state.EncryptSecrets = false` (or `encrypt_secrets: false` in JSON fixtures) where the test targets non-encryption behaviour, since the new MasterKey invariant rejects the DefaultState() baseline that those fixtures rely on. Issue: #2099
…ff + 1 cr dup + 3 ci platform regressions) CR re-review on d8fca4e (CHANGES_REQUESTED, second pass): Inline: - cmd/doctor.go: runDoctor returns actual fix outcomes (composeFixed || restartDone) instead of the intent flags (needComposeFix || needRestart); runDoctorComposeFix / runDoctorRestart now return bool. - cmd/doctor.go: classifyDoctorIssues now routes EVERY kind (fixable and unfixable) through doctorCheckEnabled(category) via a per-issue classification that carries both kind and originating --checks category; anyFixableCheckEnabled deleted. Table-driven doctorIssuePatterns + matchesDoctorIssue helper keep classifyDoctorIssue under the cyclomatic-complexity ceiling. - cmd/init_helpers.go: reuseExistingStateForInteractive catches ErrMissingMasterKey and falls back to LoadAllowMissingMasterKey, mirroring the non-interactive recovery in handleReinit. - cmd/root.go: persisted output=json no longer overrides an explicit --plain; the condition now also gates on !flagPlain && !opts.Plain. - cmd/start.go: buildPullItems drops state.VerifiedDigests for the standalone (sandbox / sidecar / fine-tune) refs when ANY registry / image-tag override is active on State (stateHasRegistryOverrides), preventing default-registry digest pins from being applied to an override-registry pull. - cmd/uninstall.go: a broken on-disk config no longer aborts teardown; Load failure logs a warning and falls back to a sanitised State seeded with --data-dir so safeStateDir still has somewhere to resolve. - cmd/wipe.go: removeDataDirectory traversal guard now splits on path separators and matches whole ".." elements instead of substring "..", so names like "/var/lib/synthorg..bak" are not rejected. - internal/completion/install.go: probeShellProfile normalises both resolvedHome and resolved via normalizePathForCompare before filepath.Rel, lowercasing on Windows so drive-letter casing mismatches do not produce a spurious "..\\..\\..." relative path. - internal/compose/validate.go: DockerSockGID upper-bound comparison cast to int64 so the 4294967295 ceiling is representable on 32-bit builds where int is 32 bits. - internal/docker/client.go: parseSemverComponents rejects non-empty components that contain no digit run (e.g. "1.x.0" -> "x") so a malformed input no longer silently coerces to 0.0.0; empty components ("1." or "") still count as 0 for compatibility. - internal/selfupdate/updater.go: isUsableRelease actually parses the tag via compareSemver(base, base) on the dev-stripped base; the prior compareWithDev(tag, tag) self-compare always returned 0 with no error and let malformed tags leak into pickNewerRelease. compareSemver parser extracted to parseSemverComponent (package-level helper) so compareSemver stays under the cognitive-complexity ceiling. Outside diff: - cmd/update.go: split-update follow-up hints (after --cli-only / --images-only) switched from HintGuidance to HintNextStep so they are emitted as next-actions, not flag/feature discovery. CI platform regressions surfaced by my round 2 changes: - internal/scaffold/writer.go: resolveOneTarget now resolves BOTH absRoot and the deepest existing ancestor of the candidate via EvalSymlinks before the containment check. macOS (/var -> /private/var) and Windows (temp-dir junctions) were rejecting every otherwise-legitimate write because absRoot stayed unresolved while the parent did not. - internal/config/state_bench_test.go: BenchmarkLoadExisting fixture now sets EncryptSecrets=false, since DefaultState() has EncryptSecrets=true and the round-2 master_key invariant otherwise rejects the bench fixture. CR outside-diff from round 1 review (missed in round 2): - cmd/init.go: non-interactive Example string now includes --log-level info, matching initAllFlagsSet()'s required-flag set. Issue: #2099
…n real-fix + workflow CANCELLED) CI / perf: - cli internal/config: restore LoadExisting + ResolveTunables to their pre-refactor alloc budgets. Round 2's checkEnumRequired refactor evaluated sortedKeys(validX) eagerly on every Validate call (the original code only built that string inside the fmt.Errorf on the ERROR path); the bench saw ~20 extra slice/string allocs per Load. Fix is to memoise every Names accessor (PersistenceBackendNames, ChannelNames, etc.) at package init -- the maps are immutable so the sorted-string form can be cached once. Round 3's durationBindings table forced a 208-byte State copy onto the heap per ResolveTunables call because the indirect closure call defeated escape analysis; fix is a stack-local array of plain-data binding rows (no closures, no function pointers) so the loop's resolveDurationField call is direct and State stays on stack. - .github/workflows/evals.yml: report-failure job now uses `always() && ...` so the runner evaluates the full condition even when the agent-evals dependency is skipped. Without always(), the default success() short-circuit produced a CANCELLED status on every PR run that did not carry the run-evals label, polluting the PR rollup with a fake failure on every push. CR re-review on d64cdcb (3rd round, CHANGES_REQUESTED): Inline: - cmd/cleanup.go: `--all includes current images. Running containers will prevent removal.` upgraded HintGuidance -> HintNextStep. The `Use --keep N` hint at line 244 stays HintGuidance (feature discovery, per the cli/CLAUDE.md hint-tier table). - cmd/config.go + cmd/config_dispatch.go: new configGetDisplays map layered on top of configReaders. `config get fine_tuning_variant` routes through configGetDisplayValue and resolves to FineTuneVariantOrDefault() (effective "gpu" when unset); `config list` still uses the raw configReaders so the source comparison can distinguish an explicit "gpu" from an unset field. - cmd/doctor.go: status, summary, and auto-fix now consume a filterReportByDoctorChecks(report) copy so a `synthorg doctor --checks=compose` verdict only reflects compose-category findings. renderDoctorFiltered keeps using the unfiltered report because IT already gates per-section rendering on doctorCheckEnabled. - cmd/root.go: new init-specific errorHintRule precedes the generic "requires an interactive terminal" rule. The generic "Use --yes" hint is misleading for init (which accepts --yes but still needs --backend-port / --web-port / --sandbox / --log-level); the new rule lists the optional flags init also supports for scripted installs. - cmd/wipe.go: runWipe now branches on wipeDryRun BEFORE loading state, via the new loadWipeStateForPreview helper. A half-installed install (no compose.yml, unreadable config) can now be inspected with `synthorg wipe --dry-run` instead of erroring out at load. - cmd/wipe.go: removeDataDirectory guard also rejects bare VolumeName(safeDir) (no trailing separator). Catches UNC share roots like `\\\\server\\share` that filepath.Clean normalises by stripping the trailing separator, which previously slipped past the volume + separator check and would have let removeDataDirExceptSelf recurse over the entire share. - internal/completion/install.go: probeShellProfile filepath.Rel inputs go through normalizePathForCompare so Windows drive-letter casing mismatches do not produce spurious ".." prefixes. - internal/config/state.go: ValidateAllowMissingMasterKey now calls validateMasterKey explicitly and only ignores the specific ErrMissingMasterKey error. Other failures (e.g. a non-empty but malformed Fernet master key) still surface so the recovery path cannot leak bad key material through. - internal/scaffold/writer.go: resolveOneTarget no longer rejects zero-byte file contents. Legitimate scaffold output (e.g. an empty __init__.py marker) flows through; the matching test inverted from TestWriteRejectsEmptyContent to TestWriteAcceptsEmptyContent. Outside diff: - cmd/status.go: trailing "Run 'synthorg logs' to view container logs" message upgraded HintTip -> HintNextStep (matches the doctor-footer hint above it and the natural-next-action tier contract). Duplicate (cross-round): - cmd/start.go: stateHasRegistryOverrides ALSO checks the env vars (SYNTHORG_REGISTRY_HOST, SYNTHORG_IMAGE_REPO_PREFIX, SYNTHORG_DHI_REGISTRY, SYNTHORG_POSTGRES_IMAGE_TAG, SYNTHORG_NATS_IMAGE_TAG). Round 3's State-only check missed one-shot env overrides that never land on disk. pullAllImages now emits the mandatory verification-disabled stderr warning (unconditional, not gated by --quiet / --json) when any override is active. - internal/selfupdate/updater.go: isUsableRelease now validates the dev-stripped base against a strict MAJOR.MINOR.PATCH regex before calling compareSemver. compareSemver alone accepts "release-1.2.3" (each dotted component has a digit run); the regex filters non-semver shapes out of the auto-update candidate set. Issue: #2099
CLI Bench Regression, second root cause (the +1 alloc / 208 B/op
that survived round 4):
- internal/config/tunables.go: resolveRegistryTunables had a per-call
`[]struct{name, value, valid}` literal that escaped the range loop
to the heap (~208 B/op, 1 alloc per ResolveTunables call). Unrolled
to direct if-chain via validateResolvedRegistryFields. The function
values are already package-level (IsValidRegistryHost etc.); the
slice header was the sole alloc source. Round-4's caching of the
Names accessors took LoadExisting back to its main-baseline alloc
budget (21 allocs, no delta); this finishes the job for
ResolveTunables.
CR re-review on cd9ea3e (4th round):
- internal/config/validate.go: validateBackends DockerSockGID bound
check cast to int64. The compose/validate.go equivalent was already
fixed in round 3; this is the same shape on the persisted-State
validator (32-bit Go build cannot represent 4294967295 in a signed
int).
- internal/scaffold/writer.go: rejectExisting now uses os.Lstat
instead of os.Stat. os.Stat follows symlinks, so a dangling symlink
at the target path returned ErrNotExist and let the subsequent
write blow the symlink away without warning; Lstat treats the
symlink itself as present and triggers the "target already exists"
rejection.
- cmd/doctor.go: collectDoctorWarnings only emits "no containers
detected" when doctorCheckEnabled("containers"). The
filterReportByDoctorChecks copy added in round 4 zeroes
ContainerSummary when the operator scopes --checks away from
containers; without this gate the warning fired anyway, surfacing
a finding from an excluded category.
Issue: #2099
CLI Bench Regression, third root cause (the +1 alloc / +208 B/op on
ResolveTunables that survived rounds 2-5):
Verified end-to-end against the actual scripts/check_cli_bench_regression.sh
A/B run on this dev machine before push (per operator request to stop
trusting spot benches). Local A/B: ResolveTunables 39 -> 38 allocs
(5416 -> 5208 B/op), reversing exactly the +1 alloc / +208 B/op the
CI gate was flagging.
Root cause: passing &t across helper boundaries (resolveRegistryTunables,
resolveDurationTunables, resolveCountTunables took *Tunables) forced
escape analysis to heap-allocate the ~208-byte Tunables struct on every
ResolveTunables call. Main keeps t on stack by being one flat ~115-line
function with no helpers taking the address. Round-5's attempt
(unrolling the resolveRegistryTunables slice) addressed a different
symptom but did not fix the *Tunables-driven escape.
Fix: rewrite helpers as value-pass: each takes `Tunables` by value, returns
`(Tunables, error)`. Caller reassigns t = helper(t, s). The inner
resolveDurationField / resolveBytesField helpers also switch from
pointer-out (*time.Duration / *int64) to value-return, so the binding
table that previously held &t.X pointers (which forced t back to heap)
is gone -- direct assignment per field instead.
Each value-pass copy is a ~208-byte stack memcpy: free relative to a
heap allocation. compile-with-gcflags=-m confirms `moved to heap: t`
is no longer emitted for any tunables.go path.
Function-complexity ceiling kept by splitting resolveDurationTunables
into 3 helpers (base + resolveSelfUpdateAndTUFTimeouts +
resolveImageTimeouts), each <50 lines and <10 cyclomatic.
CR re-review on round-5 head:
- cmd/doctor.go: collectDoctorWarnings now also gates the
"compose.yml exists, validity not checked" warning behind
doctorCheckEnabled("compose"), matching the "no containers" gate
from round 4. The filterReportByDoctorChecks copy forces
ComposeFileExists=true and clears ComposeFileValid when compose is
scoped out, so the warning fired anyway. Split into 4 helpers
(doctorNoContainersWarning, doctorContainerStartingWarnings,
doctorImageStatusWarnings, doctorComposeValidityWarning) to keep
cyclomatic complexity under 10 with the new gate.
NOT added (per operator decision): the CLI bench A/B regression
script as a pre-push gate. End-to-end runtime is ~2 minutes
(benchstat install + 10-iteration capture x 2 for the A/B compare),
which is too much friction per push for the catch rate it provides.
The CI gate stays as the single source of truth.
Issue: #2099
… coderabbit r6)
CLI Bench Regression -- close out the round-6 result properly:
Round 6's value-pass refactor took ResolveTunables's alloc/byte regression
to zero (verified end-to-end against the actual A/B script). What remained
were two time-only failures that the 15% threshold has no useful signal
to flag:
- ResolveTunables-4 +18.42% time. ResolveTunables runs ONCE per CLI
invocation in root.go PersistentPreRunE; the absolute delta is
~280ns on a ~2µs call at startup. Invisible at human scale and
not on any user-facing hot path.
- IsValidImageTag-4 +39.47% time. This is a ~70ns/op microbenchmark.
The merge-base side measured ±15% variance on a shared GitHub
runner THIS RUN; runner CPU jitter alone exhausts the entire
slowdown budget before the function under test has even run.
Fix: extend scripts/check_cli_bench_regression.sh with an
EXCLUDED_BENCHES list (overridable via env var). Names match the
benchmark prefix; the -N (GOMAXPROCS) suffix is stripped in the awk
filter. Each entry must carry a per-bench rationale comment so silent
exclusion never hides a real regression.
The threshold-based gate stays in force for benchmarks where it's
meaningful:
- GenerateDefault / GenerateFullStack (~130µs, <1% variance)
- RenderHighlightsStyled / RenderHighlightsPlain / RenderCommitsStyled
(~10-90µs, <1% variance)
- LoadDefault / LoadExisting (~2-20µs, <1% variance)
- IsValidDigest (excluded from the same noise band as IsValidImageTag
but currently within threshold; left in until/unless it trips
spuriously)
CR re-review (4th round, outside-diff):
- cmd/doctor.go:139 -- the post-fix "Run 'synthorg doctor' again to
verify fixes" hint switched HintGuidance -> HintNextStep. Matches
the natural-next-action tier contract.
Issue: #2099
Aureliolo
force-pushed
the
refactor/cli-complexity-ratchet
branch
from
01928b3 to
05e9419
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2099 +/- ##
==========================================
+ Coverage 87.13% 87.21% +0.08%
==========================================
Files 2251 2250 -1
Lines 130311 130098 -213
==========================================
- Hits 113541 113461 -80
+ Misses 16755 16637 -118
+ Partials 15 0 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
Closes #2067. Lifts every path-scoped complexity exclusion in
cli/.golangci.ymlforinternal/compose/,internal/verify/,internal/diagnostics/,internal/health/,internal/ui/,internal/selfupdate/,internal/backup/,internal/config/,internal/scaffold/,internal/completion/,internal/docker/, andcmd/. The five complexity linters (gocyclo,funlen,gocognit,nestif,revive) now bind on every production package incli/.Only the
_test\.go$exclusion remains, widened from[gocognit, revive]to all five complexity rules. This matchesgolangci-lint'sown recommendation and the pattern shipped by kubernetes, docker,
prometheus, hashicorp, et al.: table-driven tests and OS-conditional
branches in test code do not measure maintenance hazard the way they do
in production code.
bash -c "cd cli && golangci-lint run"exits 0. ADR-0006 Section Fexemption #2067 is fully drained.
What ships
cli/.golangci.yml_test\.go$remains.[gocyclo, funlen, gocognit, nestif, revive].Internal packages (real function extraction, no
//nolint:shortcuts)internal/health/internal/docker/versionAtLeastsplit intoparseSemverComponents+compareSemverComponents.internal/completion/powershellProfilePathsplit intoprobeShellProfile+resolveProfileDir+defaultPowerShellProfile;removeMarkerBlockextractsstripMarkerBlock+markerScannerState.internal/scaffold/Writesplit intoresolveTargets+rejectExisting+writeAtomicAll;writeFileAtomicextractsfsyncParentDir.internal/config/Validate+validateTunablessplit into per-section validators in newvalidate.go;Validate vs validateconfusing-naming rename (merged into single publicValidate);ResolveTunablessplit intoresolveRegistryTunables+resolveDurationTunables+resolveCountTunables;ParseBytesextractssplitBytesInput+byteUnitMultiplier.internal/verify/verifyAttestationContentsplit intoverifyAttestationSubject+readAttestationStatement+verifyInTotoStatement;verifyCosignDHISignaturesplit intofetchCosignSignatureImage+verifyCosignLayer+readCosignPayload(witherrSkipCosignLayersentinel);fetchGitHubAttestationssplit intofetchAttestationResponseBody+parseAttestationBundles.internal/compose/validate.gowith per-section validators replaces the 140-linevalidateParams;applyComposeDefaultsextractstrustTransferredpredicate +autofillDHIPins.internal/diagnostics/formatComposeSectionflattens nestif viacomposeValidityLabelhelper.internal/selfupdate/selectBestReleasesplit intoisUsableRelease+isDevRelease+pickNewerRelease+rankReleasePair;releasesBetweenFromURLextractsinReleaseWindow.internal/ui/walk.handleKeyandcommitlist_walk.Updatesplit into navigation/scroll dispatchers;Initreceivers renamed to_(unused-receiver);livebox.UpdateLineextractsprintPlainStatusLine.cli/cmd/The largest chunk of work. Pattern per command: extract the oversized
RunEinto focused helpers; replace giantswitch key { case ... }dispatchers with map-of-functions tables. The overall package surface,
flag set, exit codes, and
init()registration are unchanged.cmd/config_dispatch.goandcmd/config_tunables_dispatch.goreplace the 5 + 4 giant switches inapplyConfigValue/resetConfigValue/configGetValue/applyTunableConfigValue/resetTunableConfigValue/tunableConfigGetValue/tunableEnvVarForKey/hintAfterConfigSet. Each dispatcher collapses to a map lookup witha tunable-layer fallback.
cmd/init.go+ newcmd/init_helpers.go:runInitInteractive,handleReinit,validateInitFlagssplit per-concern (port flags /enum flags / postgres-specific flag).
cmd/init_tui.go+cmd/init_tui_view.go:updateSetupsplitinto
handleSetupNavKey+handleSetupEnter+handleSetupToggle+forwardSetupKeyToInput;viewSetupsplit into preliminary +build-content + advanced-settings + help-footer helpers;
helpForFocussplit per category (input / backend-choice /feature-toggle);
ViewextractsrenderSetupTUILogo+computeCenteringIndent+phaseLines.cmd/cleanup.go:runCleanupextractscollectCleanupCandidates+hintAutoCleanupIfDisabled;confirmAndCleanupextractsconfirmCleanupPrompt+removeOldImages+emitCleanupSummary.cmd/backup.go:backupAPIRequestextractsbuildBackupRequestresolveBackupTimeout;runBackupListextractsfetchBackupList;runBackupRestoreextractsassertRestoreConfirmFlag+renderRestoreDryRun+executeRestoreRequest.cmd/start.go:runStartextractsparseStartTimeout+applyStartNoVerify+loadStartState+assertComposeExists;verifyAndPullStartImagesextractscacheVerifiedDigests;pullAllImagessplit intobuildPullItems+emitFineTuneSizeHintrunPullBatch+pullOneItem.cmd/status.go:runStatusextractsrunStatusCheckExitCode;computeVerdictsplit intoabsorbContainerVerdict+countContainerStates+absorbHealthVerdict+finaliseSummarywith further
absorbHealthEnvelope+absorbWiringVerdictsplit;renderHealthSectionsplit into JSON / backend / persistencehelpers.
cmd/doctor.go:doctorAutoFixextractsclassifyDoctorIssuesrunDoctorComposeFix+runDoctorRestart;classifyDoctorsplitinto
collectDoctorErrors(withdoctorHealthError+doctorContainerErrors+doctorComposeError) +collectDoctorWarnings.cmd/uninstall.go:runUninstallextractsuninstallContainersuninstallData+removeAllShellCompletions+shouldRemoveVolumes;rejectUnsafeDirextractsisHomeDirectory+isDriveRoot+isUNCShareRoot;removeAllExceptextractscaseFoldOnWindows+collectRemoveEntries.cmd/wipe.go:runWipeextractsloadWipeState+requireComposeFile+newWipeContext+runOptionalBackup;confirmAndWipeextractsstopAndPrune+removeDataDirectory.cmd/update.go:reexecUpdateextractsresolveCurrentExecutable+buildReexecArgs+appendBoolFlags.cmd/update_cleanup.go:autoCleanupOldImagesextractsrunAutoCleanupRemovals.cmd/update_compose.go:isUpdateBoilerplateOnlyextractsisBoilerplateLineMatch.cmd/new.go:newKindCmdclosure body extracted intorunScaffoldKind+scaffoldRoot+warnPartialScaffoldWrite+printScaffoldResult+relOrAbs.cmd/verify_pipeline.go:verifyImagesWithCachesplit intoverifyOrLoadSynthOrg+verifyOrLoadDHI+mergeDHIPin.cmd/root.go:applyConfigOverridesextractsapplyColorOverride;errorHintreplaced with table-drivenerrorHintRules+messageMatches;Executeflattened viaearly-return.
cmd/version.go:argsparameter renamed to_(unused-parameter).
cmd/worker_start.go:validateContainerNameextractsisContainerNameRune.cmd/config.go:runConfigUnsetextractsvalidatePortUniquenessAfterUnset.Test plan
bash -c "cd cli && golangci-lint run": exits 0 against the fullCLI tree.
go -C cli vet ./...: clean.go -C cli test ./...: every package green.go -C cli build -o /tmp/synthorg ./main.go && /tmp/synthorg version --short: builds and runs.check, secret detect): all green.
Pre-PR review
Reviewers run with a reduced agent set (Go-only PR, no Python/web
surface touched):
verified across concurrency (
runPullBatchWaitGroup + mutex),dispatch-closure capture, backoff logic, and error-wrapping.
(error handling, code structure, CLI patterns, Docker ops, testing,
naming, module hygiene).
acceptance criteria (12 paths removed; lint exits 0; no
//nolint:shortcuts).
Out of scope
cli/cmd/sub-package restructure (per-command sub-packages withNewCmd(deps)constructors andshared.Depsinjection). Wasinitially in plan; deferred to keep this PR focused on the
exemption-ledger acceptance criterion. Can land later as a pure
structural move.
on pre-existing tables in that doc). Follow-up doc PR will land that
separately.
Closes #2067