fix(cli): Windows uninstall, update UX, health check, sigstore#476
fix(cli): Windows uninstall, update UX, health check, sigstore#476
Conversation
When persistence and message_bus are both None (fresh start, no company config), the health endpoint now returns status="ok" instead of "down". This fixes Docker HEALTHCHECK failures that prevented the web container from starting on fresh installations. - Add has_persistence / has_message_bus properties to AppState - Change HealthStatus model fields to bool | None (None = not configured) - Health logic only counts configured services: empty checks list → OK - Docker HEALTHCHECK and compose template unchanged (already check for "ok")
Uninstall — Windows self-delete: - Add removeAllExcept helper: removes dir contents except the running binary (deepest-first walk, ignores non-empty ancestors) - confirmAndRemoveData: on Windows, skip binary during config dir removal - confirmAndRemoveBinary: on Windows, print deferred cmd /C del command - Linux/macOS behavior unchanged (unlink works on running binaries) Update command UX rework: - Separate CLI and container update flows with individual confirmations - Default all confirm prompts to "yes" instead of "no" - Skip container pull if images already match the target version - Cap container image version to CLI version (by construction) - Update config and regenerate compose.yml before pulling Sigstore: - Add WithIntegratedTimestamps(1) to verifier (required by sigstore-go v1.1+)
- removeAllExcept: track file vs dir during walk, only report file removal errors (dir failures are expected for non-empty ancestors) - isInsideDir: case-fold on Windows (NTFS is case-insensitive) - confirmAndRemoveData: handle os.Executable() error explicitly - updateContainerImages: extract confirmUpdate, regenerateCompose, restartIfRunning helpers (was 109 lines, limit 50) - updateContainerImages: save config AFTER successful pull (prevent stale state on failed pull) - updateCLI: extract confirmUpdate helper (was 51 lines) - Tests: use absolute paths, create excluded file, add Windows case-insensitive test case Pre-reviewed by 6 agents, 8 findings addressed
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors CLI uninstall with Windows-aware selective deletion and safer prompts; restructures CLI update into a two-stage, version-aware flow that updates the CLI and container images with tag normalization and optional restart; makes health checks nullable for optional services, adds AppState predicates, and adds corresponding tests and frontend UI adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (update)
participant State as Config/State
participant Compose as Compose generator
participant Docker as Docker Engine
User->>CLI: run update command
CLI->>CLI: determine current & latest CLI versions
CLI->>State: read current state
State-->>CLI: return state
CLI->>CLI: compute effectiveVersion & targetImageTag
CLI->>Compose: regenerateCompose(tag)
Compose->>State: write updated compose.yml
State-->>CLI: persist success
CLI->>Docker: pull images for tag
Docker-->>CLI: images pulled
CLI->>State: persist updated image tag/state
CLI->>Docker: restartIfRunning (prompt & confirm)
Docker->>Docker: stop/start containers
Docker->>Docker: run health checks
Docker-->>CLI: restart + health result
CLI-->>User: report update completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a series of enhancements aimed at improving the overall reliability and user experience of the CLI tool. It addresses specific platform challenges, streamlines critical maintenance operations, and ensures compatibility with updated security standards, making the application more robust and user-friendly across different environments and configurations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
==========================================
+ Coverage 93.76% 93.81% +0.04%
==========================================
Files 476 476
Lines 22616 22632 +16
Branches 2180 2182 +2
==========================================
+ Hits 21207 21233 +26
+ Misses 1098 1091 -7
+ Partials 311 308 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements across the CLI and API. The CLI uninstall process on Windows is made more robust by handling mandatory file locking, allowing the running binary to be skipped and providing deferred cleanup instructions. The CLI update experience is enhanced by separating CLI and container image update flows, adding explicit confirmations, skipping pulls for matching versions, and ensuring configuration persistence only after successful image pulls. The API health check logic is improved to correctly report 'ok' status for fresh installations where persistence or message bus might not be configured, by introducing bool | None types for service health and has_persistence/has_message_bus properties. This prevents Docker HEALTHCHECK failures in such scenarios. Finally, the Sigstore verification in the self-update mechanism is updated to include integrated timestamps, aligning with sigstore-go v1.1+ requirements. The changes are well-tested with new unit tests covering the new functionalities and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/uninstall_test.go`:
- Around line 88-145: Add a Windows-specific unit test to verify removeAllExcept
handles case-insensitive filesystems: create
TestRemoveAllExcept_CaseInsensitiveWindows that skips unless runtime.GOOS ==
"windows", create a file whose name differs only by case from the excluded path
(e.g., write "Keep.txt" then pass "keep.txt" as excluded), call
removeAllExcept(root, excluded) and assert the original file still exists;
include the runtime.GOOS check, TempDir usage, os.WriteFile to create the file,
and os.Stat to verify it remains so the test catches case-sensitivity
regressions on NTFS.
In `@cli/cmd/uninstall.go`:
- Around line 176-179: The printed Windows cleanup command should escape cmd.exe
metacharacters in execPath before formatting into "cmd /C del \"%s\"\n";
implement a small helper (e.g., escapeForCmd or escapeCmdPath) that takes
execPath and returns a string where %, ^, &, |, <, > are escaped for cmd.exe
(e.g., replace "%"->"%%", "^"->"^^", "&"->"^&", "|"->"^|", "<"->"^<",
">"->"^>"), then use that escaped value in the fmt.Fprintf call in the Windows
branch that prints the del command. Ensure you only change the printed text (no
command execution changes).
- Around line 222-231: The current exclusion check inside the filepath.WalkDir
callback uses a case-sensitive equality (path == except) which fails on Windows;
update the anonymous callback in uninstall.go to perform the same case-folded
comparison used by isInsideDir by using a case-insensitive comparison when
running on Windows (e.g., use runtime.GOOS to detect Windows and
strings.EqualFold to compare path and except), so the excluded executable is
correctly skipped regardless of casing.
In `@cli/cmd/update.go`:
- Around line 126-140: regenerateCompose currently writes the new compose.yml
immediately, which can leave compose.yml pointing at the new tag even if
composeRun (pull) or config.Save fails; change the flow so regenerateCompose
writes to a temporary file (or returns the temp path) instead of overwriting the
live compose.yml, run composeRun against that temp compose file (use the same
composeRun call site that currently uses safeDir/cmd), and only replace the live
compose.yml and set updatedState.ImageTag / call config.Save after composeRun
and config.Save succeed; if any step fails, delete the temp file and leave the
existing compose.yml unchanged (or restore it) to ensure atomic swap semantics.
- Around line 64-65: confirmUpdate currently swallows prompt errors and returns
only a bool; change its signature to confirmUpdate(...) (bool, error) (mirroring
confirmRestart), return any error from form.Run() instead of treating it as a
false/decline, and update both call sites that currently do `if
!confirmUpdate(...)` to capture the (ok, err) return values, propagate/return
err when non-nil, and only proceed when ok is true; update the confirmUpdate
function body (the one building/running the form) to forward form.Run() errors
and adjust callers to abort the command on error.
In `@src/synthorg/api/controllers/health.py`:
- Around line 74-108: The health_check() function duplicates the same try/except
warning+fallback for persistence and message bus; extract that logic into a
small helper (e.g., probe_optional_service or probe_component_health) and call
it from health_check() for app_state.persistence.health_check() and
app_state.message_bus.is_running so the warning/exception handling is
centralized; the helper should accept the component identifier (like
"persistence" or "message_bus") and either an awaitable or callable to invoke,
return bool | None (None when the service is not present), and preserve the
existing logger.warning(API_HEALTH_CHECK, component=..., exc_info=True) and
fallback to False on exception.
In `@tests/unit/api/test_health.py`:
- Around line 51-88: Add tests covering the symmetric message_bus-only
permutations and refactor duplication into a parametrized matrix: update the
TestHealthCheckUnconfiguredServices test class to include cases where only
message_bus is provided (healthy and unhealthy) using a FakeMessageBusBackend
(similar to FakePersistenceBackend) and create_app(message_bus=...), and convert
the three existing scenarios (no services, persistence-only healthy,
persistence-only unhealthy) plus the new message_bus-only healthy/unhealthy
scenarios into a pytest.mark.parametrize over inputs (persistence backend or
None, message_bus backend or None, expected status, expected persistence
bool/None, expected message_bus bool/None) so the health endpoint /api/v1/health
logic in the health controller is fully exercised without duplicated test code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6268bc06-10a0-4858-9075-77021e44f36a
📒 Files selected for processing (9)
cli/cmd/uninstall.gocli/cmd/uninstall_test.gocli/cmd/update.gocli/cmd/update_test.gocli/internal/selfupdate/sigstore.gosrc/synthorg/api/controllers/health.pysrc/synthorg/api/state.pytests/unit/api/test_health.pytests/unit/api/test_state.py
📜 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: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Use PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
Type hints required on all public functions; enforce with mypy strict mode
Google style docstrings required on public classes and functions; enforced by ruff D rules
Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement in non-Pydantic internal collections
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Line length: 88 characters (ruff); enforced by linter
Files:
src/synthorg/api/state.pysrc/synthorg/api/controllers/health.pytests/unit/api/test_state.pytests/unit/api/test_health.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Use frozen Pydantic models for config/identity; use separate mutable-via-copy models withmodel_copy(update=...)for runtime state that evolves
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict)
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators
Every module with business logic must have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code; use synthorg'sget_logger()instead
Always useloggeras the variable name, not_loggerorlog
Always use event name constants from the domain-specific module undersynthorg.observability.eventsin logging calls
Log with structured kwargs: alwayslogger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
Log at DEBUG level for object creation, internal flow, entry/exit of key functions
Use Google-style docstrings in all public Python code for mkdocstrings auto-generation
Files:
src/synthorg/api/state.pysrc/synthorg/api/controllers/health.py
{src/synthorg/**/*.py,tests/**/*.py,web/src/**/*.{ts,tsx,vue},cli/**/*.go,**/*.md,**/*.yml,**/*.yaml,**/*.json,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples; use generic names:
example-provider,example-large-001,example-medium-001,example-small-001, or size aliases
Files:
src/synthorg/api/state.pycli/cmd/update_test.gocli/internal/selfupdate/sigstore.gosrc/synthorg/api/controllers/health.pycli/cmd/uninstall.gocli/cmd/uninstall_test.gocli/cmd/update.gotests/unit/api/test_state.pytests/unit/api/test_health.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All Litestar API routes must use RFC 9457 structured errors with ErrorCategory, ErrorCode, ErrorDetail
Files:
src/synthorg/api/state.pysrc/synthorg/api/controllers/health.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Go CLI binary must use Cobra for commands, charmbracelet/huh for prompts, and charmbracelet/lipgloss for styled CLI output
Go CLI must be cross-platform compatible and build for linux/darwin/windows × amd64/arm64 matrix
Go CLI commands must support completion auto-install for bash, zsh, fish, and powershell
Files:
cli/cmd/update_test.gocli/internal/selfupdate/sigstore.gocli/cmd/uninstall.gocli/cmd/uninstall_test.gocli/cmd/update.go
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowmarkers for test categorization
Maintain 80% minimum test coverage; enforced in CI
Useasyncio_mode = 'auto'in pytest config — no manual@pytest.mark.asyncioneeded on individual tests
Set 30-second timeout per test
Always include-n autowhen running pytest with pytest-xdist; never run tests sequentially
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in test files
Use Hypothesis property-based testing with@givenand@settings; control profile viaHYPOTHESIS_PROFILEenv var (ci: 200 examples, dev: 1000 examples)
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; mocktime.monotonic()andasyncio.sleep()for timing-sensitive tests
Files:
tests/unit/api/test_state.pytests/unit/api/test_health.py
🧠 Learnings (4)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/update_test.gocli/cmd/uninstall.gocli/cmd/update.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Config vs runtime state: frozen Pydantic models for config/identity; separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Applied to files:
src/synthorg/api/controllers/health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/uninstall.gocli/cmd/uninstall_test.go
📚 Learning: 2026-03-15T22:42:34.899Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T22:42:34.899Z
Learning: Applies to cli/**/*.go : Go CLI binary must use Cobra for commands, charmbracelet/huh for prompts, and charmbracelet/lipgloss for styled CLI output
Applied to files:
cli/cmd/uninstall.go
🧬 Code graph analysis (5)
src/synthorg/api/state.py (1)
src/synthorg/persistence/protocol.py (1)
PersistenceBackend(30-188)
src/synthorg/api/controllers/health.py (1)
src/synthorg/api/state.py (4)
persistence(115-117)message_bus(125-127)has_persistence(110-112)has_message_bus(120-122)
cli/cmd/update.go (5)
cli/internal/version/version.go (1)
Version(9-9)cli/internal/selfupdate/updater.go (2)
Download(195-230)Replace(233-239)cli/internal/docker/client.go (2)
Detect(34-66)Info(22-29)cli/internal/config/state.go (2)
Save(99-113)State(14-23)cli/internal/compose/generate.go (2)
ParamsFromState(43-54)Generate(58-76)
tests/unit/api/test_state.py (4)
src/synthorg/api/state.py (4)
persistence(115-117)has_persistence(110-112)message_bus(125-127)has_message_bus(120-122)tests/unit/engine/conftest.py (2)
persistence(431-433)message_bus(437-439)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-504)tests/unit/engine/task_engine_helpers.py (2)
FakeMessageBus(64-82)start(71-72)
tests/unit/api/test_health.py (2)
src/synthorg/api/app.py (1)
create_app(426-550)src/synthorg/api/state.py (1)
persistence(115-117)
🪛 GitHub Check: CodeQL
cli/cmd/uninstall.go
[failure] 144-144: 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/update.go
[failure] 172-172: 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 (12)
cli/internal/selfupdate/sigstore.go (1)
49-55: TheWithIntegratedTimestampsAPI is correctly used in sigstore-go v1.1+.The addition of
verify.WithIntegratedTimestamps(1)to the verifier options is correct and properly implements the requirement for integrated-time timestamps from the transparency log. The comment update accurately reflects this change.cli/cmd/uninstall.go (4)
3-16: LGTM!Imports are appropriate for the new cross-platform functionality. Uses Cobra and charmbracelet/huh as per coding guidelines.
192-207: LGTM!The helper correctly handles Windows case-insensitivity and uses
filepath.Relto determine containment. The logic is sound—if the relative path starts with.., the child is outside the parent.
236-246: LGTM!The deepest-first removal order and silent handling of directory errors (expected for ancestors of excluded files) is correct. File errors are properly collected using
errors.Join.
117-148: CodeQL flags potential path traversal; existing guards provide adequate mitigation in this context.The static analysis tool flags line 144 (
os.RemoveAll(dir)) for using user-controlled data in a path expression.SecurePathprovides only basic validation (absolute path check and cleaning), not filesystem containment—the design intentionally trusts user-provided paths.The actual defense against dangerous removals is the explicit guard block at lines 119-126, which prevents removal of root, home, UNC shares, and drive roots. This adequately mitigates the most critical scenarios for an uninstall operation where the user explicitly provides a data directory.
cli/cmd/uninstall_test.go (3)
10-86: LGTM!Comprehensive test coverage with table-driven tests. Windows-specific cases (different drives, case-insensitivity) are properly guarded. Using
t.TempDir()ensures cleanup.
147-175: LGTM!Good edge case coverage ensuring files outside the root are unaffected by the operation.
177-182: LGTM!Simple but important edge case ensuring empty directories are handled gracefully.
src/synthorg/api/state.py (1)
109-112: Nice addition of non-throwing service presence flags.These properties fit the existing
has_*pattern and let callers branch before touching accessors that intentionally raise 503s.Also applies to: 119-122
src/synthorg/api/controllers/health.py (1)
41-45: Good call making the dependency fields nullable.This keeps “not configured” distinct from “configured but unhealthy” in the response contract.
tests/unit/api/test_state.py (1)
195-222: Good coverage for the newAppStateflags.These assertions lock in the exact non-throwing semantics the health controller now depends on.
cli/cmd/update_test.go (1)
5-24: Nice focused coverage fortargetImageTag.The table-driven cases exercise every normalization branch the helper currently exposes.
Go CLI fixes: - removeAllExcept: case-insensitive comparison on Windows (NTFS) - removeAllExcept: skip root directory itself, only remove contents - Guard filepath.EvalSymlinks behind os.Executable() error check - Handle os.UserHomeDir() error (was silently discarded) - Use PowerShell Remove-Item -LiteralPath instead of cmd /C del - confirmUpdate returns (bool, error) matching confirmRestart pattern - targetImageTag validates tag at trust boundary (defense-in-depth) - Extract pullAndPersist helper with compose.yml backup/rollback - updateContainerImages under 50-line limit via extraction - Actionable warning when ComposeExecOutput fails - Add comment about ASCII limitation in isInsideDir - New tests: case-insensitive Windows, root preservation, invalid tags Python API fixes: - HealthStatus docstring updated for bool|None tri-state semantics - Extract _probe_service/_probe_sync_service helpers (< 50 lines) - New tests: exception paths, message_bus-only permutations - Fix dual import paths for fakes (conftest → fakes canonical) Frontend fixes: - TypeScript HealthStatus: boolean → boolean | null - SystemStatus.vue: three-way display (OK/Down/N/A) for null services - New tests: null/not-configured state coverage Docs: - CLAUDE.md: add API_HEALTH_CHECK event constant - operations.md: add config, completion-install commands; update description for update command
… IDs
NotBlankStr strips whitespace on validation, so '0\r' becomes '0'.
The test's self-downgrade and duplicate-source pre-checks must compare
stripped values to correctly predict Pydantic validation outcomes.
Fixes flaky Hypothesis failure with pairs=[('0', '0\r')].
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/unit/api/test_health.py (1)
52-114: 🧹 Nitpick | 🔵 TrivialConsider parametrizing similar test cases.
The five unconfigured-services tests follow a similar pattern and could be consolidated with
@pytest.mark.parametrizeto reduce duplication, though the current explicit approach is also readable.♻️ Example parametrization
`@pytest.mark.parametrize`( "persistence_healthy,bus_healthy,expected_status,expected_persistence,expected_bus", [ (None, None, "ok", None, None), (True, None, "ok", True, None), (False, None, "down", False, None), (None, True, "ok", None, True), (None, False, "down", None, False), ], ids=[ "no_services", "persistence_only_healthy", "persistence_only_unhealthy", "message_bus_only_healthy", "message_bus_only_unhealthy", ], ) async def test_service_configuration_matrix( self, persistence_healthy: bool | None, bus_healthy: bool | None, expected_status: str, expected_persistence: bool | None, expected_bus: bool | None, ) -> None: # Setup based on parameters...As per coding guidelines: "Prefer
@pytest.mark.parametrizefor testing similar cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/test_health.py` around lines 52 - 114, Consolidate the five similar tests in TestHealthCheckUnconfiguredServices into a single parametrized test: add a pytest.mark.parametrize on parameters (persistence_healthy, bus_healthy, expected_status, expected_persistence, expected_bus) and inside the test use the existing helpers FakePersistenceBackend and FakeMessageBus to conditionally create/connect/start them when persistence_healthy or bus_healthy is True or False, simulate unhealthy states by setting backend._connected = False or bus._running = False when the corresponding param is False, call TestClient(create_app(persistence=backend, message_bus=bus)) and perform the same assertions against response.json() (status and the persistence/message_bus fields) as in the original specific tests; keep the existing test names (or replace them with a single descriptive name) and reuse create_app, FakePersistenceBackend, FakeMessageBus, and TestClient to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/uninstall.go`:
- Around line 263-271: The loop in removeAllExcept currently only records file
removal errors and silently ignores all directory removal failures; change the
error handling so that any non-nil error from os.Remove(entries[i].path) is
recorded except for the expected "directory not empty" case for ancestor dirs.
Concretely, after calling os.Remove(...) in the loop over entries, if err != nil
then: if entries[i].isDir { if errors.Is(err, syscall.ENOTEMPTY) { continue } }
append the error to errs for all other cases; add imports for "errors" and
"syscall" as needed and keep using the existing entries and os.Remove call
sites.
- Around line 118-128: The guard that checks whether dir equals the user's home
uses a verbatim comparison (dir == home) and can be bypassed by case differences
or trailing separators; normalize both values before comparing: call
filepath.Clean on home (like filepath.Clean(dataDir) already does for dir), trim
any trailing separators, and on Windows perform a case-insensitive comparison
(e.g. use strings.EqualFold) when comparing dir and home inside the uninstall
logic (the block that calls os.UserHomeDir(), computes vol :=
filepath.VolumeName(dir), sets isUNC/isDriveRoot, and decides to return an error
instead of proceeding to RemoveAll); update that comparison to compare the
normalized/cleaned forms so the safety check cannot be bypassed.
In `@cli/cmd/update.go`:
- Around line 193-218: Read and record the current compose.yml before calling
regenerateCompose and treat a missing read as "no backup" but still ensure
rollback: use composePath and do an os.ReadFile early to set backup and a
backupExists flag; if regenerateCompose, composeRun, or config.Save fails,
restore by writing the saved backup when backupExists is true, otherwise remove
the possibly-truncated/created composePath file (os.Remove) to revert to the
pre-update state; apply this logic around regenerateCompose, composeRun, and
config.Save so failures in regenerateCompose are also rolled back and avoid
attempting to write a nil backup when backup did not exist.
- Around line 223-226: The compose generation uses the in-memory global
version.Version, which is stale after selfupdate.Replace; modify
regenerateCompose to accept an additional effectiveVersion string argument
(regenerateCompose(state config.State, tag, safeDir, effectiveVersion string)
error), set updatedState.CLIVersion = effectiveVersion before calling
compose.ParamsFromState(updatedState), and update the caller
updateContainerImages to pass the effectiveVersion it computed/received through
to regenerateCompose so the generated compose.yml header uses the new CLI
version instead of the old in-memory value.
---
Duplicate comments:
In `@tests/unit/api/test_health.py`:
- Around line 52-114: Consolidate the five similar tests in
TestHealthCheckUnconfiguredServices into a single parametrized test: add a
pytest.mark.parametrize on parameters (persistence_healthy, bus_healthy,
expected_status, expected_persistence, expected_bus) and inside the test use the
existing helpers FakePersistenceBackend and FakeMessageBus to conditionally
create/connect/start them when persistence_healthy or bus_healthy is True or
False, simulate unhealthy states by setting backend._connected = False or
bus._running = False when the corresponding param is False, call
TestClient(create_app(persistence=backend, message_bus=bus)) and perform the
same assertions against response.json() (status and the persistence/message_bus
fields) as in the original specific tests; keep the existing test names (or
replace them with a single descriptive name) and reuse create_app,
FakePersistenceBackend, FakeMessageBus, and TestClient to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64735454-d437-4160-8bd4-a89349a63ccb
📒 Files selected for processing (14)
CLAUDE.mdcli/cmd/uninstall.gocli/cmd/uninstall_test.gocli/cmd/update.gocli/cmd/update_test.godocs/design/operations.mdsrc/synthorg/api/controllers/health.pytests/unit/api/test_health.pytests/unit/api/test_state.pytests/unit/budget/test_config_properties.pyweb/src/__tests__/components/SystemStatus.test.tsweb/src/__tests__/views/DashboardPage.test.tsweb/src/api/types.tsweb/src/components/dashboard/SystemStatus.vue
📜 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: Test (Python 3.14)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (11)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation source is in
docs/and built with Zensical. Design spec is indocs/design/(7 pages: index, agents, organization, communication, engine, memory, operations). All Markdown documentation must follow Zensical syntax.
Files:
docs/design/operations.md
web/src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vue 3 + PrimeVue + Tailwind CSS in the web dashboard. Organize components by feature (agents/, approvals/, budget/, common/, dashboard/, layout/, messages/, org-chart/, tasks/). Use Pinia stores for state management (auth, agents, tasks, budget, messages, meetings, approvals, websocket, analytics, company, providers).
Files:
web/src/__tests__/components/SystemStatus.test.tsweb/src/api/types.tsweb/src/components/dashboard/SystemStatus.vueweb/src/__tests__/views/DashboardPage.test.ts
web/**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest with vue-tsc for type checking and fast-check for property-based testing in the Vue dashboard. Run with
npm --prefix web run test.
Files:
web/src/__tests__/components/SystemStatus.test.tsweb/src/__tests__/views/DashboardPage.test.ts
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript in the web dashboard. Mirror backend Pydantic models in
web/src/api/with TypeScript types.
Files:
web/src/__tests__/components/SystemStatus.test.tsweb/src/api/types.tsweb/src/__tests__/views/DashboardPage.test.ts
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for variable and function names in TypeScript/Vue code.
Files:
web/src/__tests__/components/SystemStatus.test.tsweb/src/api/types.tsweb/src/components/dashboard/SystemStatus.vueweb/src/__tests__/views/DashboardPage.test.ts
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Use Cobra framework for CLI commands. Organize commands incmd/directory. Use charmbracelet/huh for interactive prompts and charmbracelet/lipgloss for styled CLI output.
Run Go linting withgolangci-lint run, vetting withgo vet ./..., testing withgo test ./.... Type-check and lint Go code in CI on changes tocli/**files.
Files:
cli/cmd/uninstall.gocli/cmd/update.gocli/cmd/uninstall_test.gocli/cmd/update_test.go
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Useexcept A, B:syntax (no parentheses) for exception handling — ruff enforces this on Python 3.14 (PEP 758 except syntax).
Include type hints on all public functions and classes. Strict mypy type-checking is enforced.
Use Google-style docstrings on all public classes and functions. Docstrings are enforced by ruff D rules.
Line length is 88 characters (enforced by ruff).
Files:
src/synthorg/api/controllers/health.pytests/unit/api/test_health.pytests/unit/budget/test_config_properties.pytests/unit/api/test_state.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects rather than mutating existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization).
Use frozen Pydantic models for config/identity. Use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions: use@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields including optional and tuple variants instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Keep functions under 50 lines and files under 800 lines.
Handle errors explicitly, never silently swallow exceptions.
Validate at system boundaries (user input, external APIs, config files).
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases. Vendor names may only appear in: (1) Operations design page (docs/design/operations.md), (2).claude/skill/agent files, (3) third-party import paths. Tests must usetest-provider,test-small-001, etc.
Pure data models, enums, and re-exports do NOT need logging.
Files:
src/synthorg/api/controllers/health.py
src/synthorg/!(observability)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/!(observability)/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerfollowed bylogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in application code. Always use variable namelogger(not_logger, notlog).
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Use structured logging with keyword arguments: alwayslogger.info(EVENT, key=value)— neverlogger.info("msg %s", val).
All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG is for object creation, internal flow, entry/exit of key functions.
Files:
src/synthorg/api/controllers/health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark tests with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow. Maintain 80% coverage minimum. Useasyncio_mode = "auto"(no manual@pytest.mark.asyncioneeded). Set 30-second timeout per test. Always include-n autowhen running pytest (never sequential). Use@pytest.mark.parametrizefor testing similar cases.
Use Python Hypothesis for property-based testing with@given+@settings. Profiles:ci(200 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var. Run dev profile:HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties..hypothesis/is gitignored.
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins.
Files:
tests/unit/api/test_health.pytests/unit/budget/test_config_properties.pytests/unit/api/test_state.py
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use native Go
testing.Ffuzz functions for property-based testing in the CLI. Run fuzzing withgo test -fuzz=<FuzzFunction> -fuzztime=<duration> ./....
Files:
cli/cmd/uninstall_test.gocli/cmd/update_test.go
🧠 Learnings (22)
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to cli/**/*.go : Use Cobra framework for CLI commands. Organize commands in `cmd/` directory. Use charmbracelet/huh for interactive prompts and charmbracelet/lipgloss for styled CLI output.
Applied to files:
docs/design/operations.mdcli/cmd/update.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
docs/design/operations.mdcli/cmd/uninstall.gocli/cmd/update.gocli/cmd/update_test.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
docs/design/operations.mdcli/cmd/uninstall.gocli/cmd/uninstall_test.go
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to cli/go.mod : CLI Go dependencies are listed in `cli/go.mod` with pinned versions. Include: Cobra, charmbracelet/huh, charmbracelet/lipgloss.
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Applied to files:
web/src/components/dashboard/SystemStatus.vue
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to src/synthorg/**/*.py : Keep functions under 50 lines and files under 800 lines.
Applied to files:
src/synthorg/api/controllers/health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to src/synthorg/!(observability)/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG is for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to src/synthorg/!(observability)/**/*.py : Use structured logging with keyword arguments: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to src/synthorg/!(observability)/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to src/synthorg/!(observability)/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` followed by `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Always use variable name `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : Mark tests with `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, or `pytest.mark.slow`. Maintain 80% coverage minimum. Use `asyncio_mode = "auto"` (no manual `pytest.mark.asyncio` needed). Set 30-second timeout per test. Always include `-n auto` when running pytest (never sequential). Use `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to cli/**/*_test.go : Use native Go `testing.F` fuzz functions for property-based testing in the CLI. Run fuzzing with `go test -fuzz=<FuzzFunction> -fuzztime=<duration> ./...`.
Applied to files:
cli/cmd/uninstall_test.gocli/cmd/update_test.go
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to cli/**/*.go : Run Go linting with `golangci-lint run`, vetting with `go vet ./...`, testing with `go test ./...`. Type-check and lint Go code in CI on changes to `cli/**` files.
Applied to files:
cli/cmd/uninstall_test.gocli/cmd/update_test.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on cli/**/*.go) (fast gate before push, skipped in pre-commit.ci — dedicated CI jobs already run these).
Applied to files:
cli/cmd/update_test.go
🧬 Code graph analysis (5)
web/src/__tests__/components/SystemStatus.test.ts (2)
src/synthorg/api/controllers/health.py (1)
HealthStatus(31-54)web/src/api/types.ts (1)
HealthStatus(505-511)
web/src/__tests__/views/DashboardPage.test.ts (1)
web/src/api/endpoints/health.ts (1)
getHealth(4-7)
src/synthorg/api/controllers/health.py (1)
src/synthorg/api/state.py (4)
persistence(115-117)message_bus(125-127)has_persistence(110-112)has_message_bus(120-122)
cli/cmd/update.go (5)
cli/internal/version/version.go (1)
Version(9-9)cli/internal/selfupdate/updater.go (2)
Download(195-230)Replace(233-239)cli/internal/docker/client.go (3)
Detect(34-66)Info(22-29)ComposeExecOutput(100-107)cli/internal/config/state.go (2)
State(14-23)Save(99-113)cli/internal/compose/generate.go (2)
ParamsFromState(43-54)Generate(58-76)
tests/unit/api/test_health.py (4)
tests/unit/engine/task_engine_helpers.py (3)
FakeMessageBus(64-82)get(29-31)start(71-72)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-504)tests/unit/memory/backends/mem0/conftest.py (1)
backend(41-49)src/synthorg/api/state.py (2)
persistence(115-117)message_bus(125-127)
🪛 GitHub Check: CodeQL
cli/cmd/uninstall.go
[failure] 149-149: 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/update.go
[failure] 195-195: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 204-204: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 215-215: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 232-232: 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 (12)
tests/unit/budget/test_config_properties.py (1)
119-123: Whitespace-normalized expectation is correct.This update correctly mirrors
NotBlankStrstripping behavior, so the property test now predicts validation outcomes reliably.src/synthorg/api/controllers/health.py (2)
57-86: LGTM! Clean extraction of probe helpers.The
_probe_serviceand_probe_sync_servicehelpers centralize the try/except/log pattern effectively. Both correctly returnNonewhen not configured,Falseon exception (after logging), and the probe result otherwise.
110-127: Status logic handles tri-state correctly.The probe calls and status determination logic properly handle the three states:
None(not configured) → excluded from checksTrue/False→ included in health assessmentOne edge case to verify: when
checksis empty (no services configured), status isOK, which aligns with the PR objective of avoiding Docker HEALTHCHECK failures on fresh starts.CLAUDE.md (1)
194-199: Documentation aligns with implementation patterns.The expanded logging guidance correctly documents:
- Event constant usage (e.g.,
API_HEALTH_CHECK)- Structured kwargs pattern
- Error path logging at WARNING/ERROR
- State transitions at INFO
- DEBUG for internal flow
These match the patterns used in the health controller changes.
web/src/api/types.ts (1)
507-508: Type changes correctly mirror backend model.The
boolean | nulltype accurately reflects the backend'sbool | Nonetri-state semantics for unconfigured services.web/src/components/dashboard/SystemStatus.vue (1)
33-50: Tri-state rendering logic is correct and accessible.The template correctly handles all four display states with appropriate color coding:
nullservice → neutral slate color + "N/A"true→ green + "OK"false→ red + "Down"- No health data → "Unknown"
The deeply nested ternaries are readable given the clear pattern. Consider extracting to a computed or helper if more states are added in the future.
Also applies to: 58-75
tests/unit/api/test_state.py (1)
195-222: Good coverage for new flag properties.The tests thoroughly cover both states of
has_persistenceandhas_message_bus:
Falsewhen the service isNoneTruewhen the service is configured and connected/startedThe async test methods correctly prepare the fakes before assertion.
tests/unit/api/test_health.py (2)
52-114: Comprehensive coverage for unconfigured service scenarios.All five permutations are now covered:
- No services configured →
okwith bothnull- Persistence-only healthy →
ok- Persistence-only unhealthy →
down- Message-bus-only healthy →
ok- Message-bus-only unhealthy →
downThis addresses the previous review comment about missing message-bus-only permutations.
117-154: Exception path tests verify fallback behavior.The tests correctly verify that exceptions during health probes result in
Falsestatus values anddownoverall status. UsingPropertyMockfor theis_runningproperty is the right approach.web/src/__tests__/views/DashboardPage.test.ts (1)
178-190: Good edge case coverage for fresh install scenario.The test correctly validates that the dashboard renders without error when health services report null values, simulating a fresh installation state where persistence and message bus aren't yet configured.
web/src/__tests__/components/SystemStatus.test.ts (1)
107-137: Comprehensive N/A state coverage.The three new tests thoroughly validate the null-state rendering:
- Persistence null → shows "N/A", not "Down"
- Message bus null → shows "N/A"
- Both null → shows exactly two "N/A" instances
The regex match on line 135 (
text.match(/N\/A/g)?.length) is a clean way to count occurrences.cli/cmd/update_test.go (1)
11-17: Good adversarial coverage fortargetImageTag.The newline, shell-metacharacter, and pre-release cases pin down the trust-boundary behavior well.
Go CLI: - removeAllExcept: only suppress ENOTEMPTY-like errors for ancestor dirs of the excluded file; collect permission/other dir errors - Home dir safety check: normalize with filepath.Clean + case-insensitive comparison on Windows (strings.EqualFold) to prevent bypass - pullAndPersist: rollback compose.yml on regenerateCompose failure too; os.Remove compose.yml when no backup existed pre-update - regenerateCompose: accept effectiveVersion param to override stale in-memory version.Version after selfupdate.Replace Python tests: - Consolidate 5 unconfigured-services tests into parametrized matrix - Fix mutation timing: set unhealthy state after app startup, not before
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cli/cmd/uninstall.go (1)
277-281:⚠️ Potential issue | 🟠 MajorOnly suppress the expected non-empty-directory error here.
This branch currently drops every removal failure on ancestors of
except. If one of those directories fails with ACL/EPERM/path-length issues,removeAllExceptstill returnsniland the caller can print a success message after partial cleanup. Ignore only the platform’s “directory not empty” case.💡 Suggested direction
- if err := os.Remove(entries[i].path); err != nil { - if entries[i].isDir && isInsideDir(except, entries[i].path) { + if err := os.Remove(entries[i].path); err != nil { + if entries[i].isDir && isInsideDir(except, entries[i].path) && isExpectedNonEmptyDir(err) { continue // expected: ancestor of excluded file is non-empty } errs = append(errs, err) }
isExpectedNonEmptyDirshould match only ENOTEMPTY-like errors for the current platform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/uninstall.go` around lines 277 - 281, The removal loop currently suppresses all errors for ancestor dirs of `except`; change it to only ignore platform-specific "directory not empty" errors: update the branch in `removeAllExcept` so it continues only when `entries[i].isDir && isInsideDir(except, entries[i].path) && isExpectedNonEmptyDir(err)`, otherwise append `err`. Implement `isExpectedNonEmptyDir(err)` to examine the error (use errors.As to get *os.PathError or syscall.Errno) and return true only for ENOTEMPTY-like errors for the current platform (e.g., syscall.ENOTEMPTY and Windows' ERROR_DIR_NOT_EMPTY), using errors.Is/Errno comparisons rather than broad truthiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/uninstall.go`:
- Around line 133-136: The current check sets isUNC for any UNC-backed path
because filepath.VolumeName(dir) returns the share prefix; change the check to
only treat UNC when the directory is the UNC root itself. Replace the isUNC
computation with something like isUNCRoot := vol != "" && (dir == vol || dir ==
vol+`\` || dir == vol+`/`) and use isUNCRoot in the final guard, referencing the
existing dir variable and filepath.VolumeName result so only the share root (not
arbitrary UNC-backed config paths) is rejected.
In `@tests/unit/api/test_health.py`:
- Around line 126-159: Combine the two tests
test_persistence_exception_returns_false and
test_message_bus_exception_returns_false into a single parametrized test using
`@pytest.mark.parametrize`: supply parameters for the fake service factory (e.g.,
FakePersistenceBackend vs FakeMessageBus), the setup/call methods to make it
ready (connect() vs start()), the attribute to patch (type(backend).health_check
for persistence and type(bus).is_running with new_callable=PropertyMock for
message bus), the side_effect RuntimeError message, and the expected response
key ("persistence" vs "message_bus") and expected status "down"; inside the
test, use the passed factory to create and start/connect the service, patch the
provided target attribute with the specified side_effect, create the
TestClient(create_app(...)) with the appropriate keyword argument
(persistence=... or message_bus=...), call GET /api/v1/health and assert the
status_code and that body["data"][<expected_key>] is False and
body["data"]["status"] == "down".
- Around line 52-159: Add a 30-second timeout marker to both test classes by
annotating TestHealthCheckUnconfiguredServices and TestHealthCheckExceptionPaths
with `@pytest.mark.timeout`(30); locate the class definitions for
TestHealthCheckUnconfiguredServices and TestHealthCheckExceptionPaths in
tests/unit/api/test_health.py and add the decorator directly above each class so
every test in those classes has an explicit 30s timeout.
---
Duplicate comments:
In `@cli/cmd/uninstall.go`:
- Around line 277-281: The removal loop currently suppresses all errors for
ancestor dirs of `except`; change it to only ignore platform-specific "directory
not empty" errors: update the branch in `removeAllExcept` so it continues only
when `entries[i].isDir && isInsideDir(except, entries[i].path) &&
isExpectedNonEmptyDir(err)`, otherwise append `err`. Implement
`isExpectedNonEmptyDir(err)` to examine the error (use errors.As to get
*os.PathError or syscall.Errno) and return true only for ENOTEMPTY-like errors
for the current platform (e.g., syscall.ENOTEMPTY and Windows'
ERROR_DIR_NOT_EMPTY), using errors.Is/Errno comparisons rather than broad
truthiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49af4e79-f848-486e-8a8e-4da36d5fc36b
📒 Files selected for processing (3)
cli/cmd/uninstall.gocli/cmd/update.gotests/unit/api/test_health.py
📜 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: Test (Python 3.14)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Useexcept A, B:syntax (no parentheses) for exception handling — ruff enforces this on Python 3.14 (PEP 758 except syntax).
Include type hints on all public functions and classes. Strict mypy type-checking is enforced.
Use Google-style docstrings on all public classes and functions. Docstrings are enforced by ruff D rules.
Line length is 88 characters (enforced by ruff).
Files:
tests/unit/api/test_health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark tests with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow. Maintain 80% coverage minimum. Useasyncio_mode = "auto"(no manual@pytest.mark.asyncioneeded). Set 30-second timeout per test. Always include-n autowhen running pytest (never sequential). Use@pytest.mark.parametrizefor testing similar cases.
Use Python Hypothesis for property-based testing with@given+@settings. Profiles:ci(200 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var. Run dev profile:HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties..hypothesis/is gitignored.
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins.
Files:
tests/unit/api/test_health.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Use Cobra framework for CLI commands. Organize commands incmd/directory. Use charmbracelet/huh for interactive prompts and charmbracelet/lipgloss for styled CLI output.
Run Go linting withgolangci-lint run, vetting withgo vet ./..., testing withgo test ./.... Type-check and lint Go code in CI on changes tocli/**files.
Files:
cli/cmd/uninstall.gocli/cmd/update.go
🧠 Learnings (6)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : Mark tests with `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, or `pytest.mark.slow`. Maintain 80% coverage minimum. Use `asyncio_mode = "auto"` (no manual `pytest.mark.asyncio` needed). Set 30-second timeout per test. Always include `-n auto` when running pytest (never sequential). Use `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/uninstall.gocli/cmd/update.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/uninstall.go
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to cli/**/*.go : Use Cobra framework for CLI commands. Organize commands in `cmd/` directory. Use charmbracelet/huh for interactive prompts and charmbracelet/lipgloss for styled CLI output.
Applied to files:
cli/cmd/update.go
🧬 Code graph analysis (1)
tests/unit/api/test_health.py (3)
src/synthorg/api/app.py (1)
create_app(426-550)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-504)src/synthorg/api/state.py (2)
persistence(115-117)message_bus(125-127)
🪛 GitHub Check: CodeQL
cli/cmd/update.go
[failure] 195-195: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 200-200: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 202-202: 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 (12)
tests/unit/api/test_health.py (1)
56-120: Good expansion of health-state matrix coverage.The parametrized cases for configured/unconfigured persistence and message bus states are comprehensive and align well with the nullable health contract.
cli/cmd/update.go (10)
21-29: LGTM!Command setup correctly uses Cobra framework with
RunEfor error handling.
31-37: LGTM!Clean orchestration with proper error propagation between the two update stages.
39-84: LGTM!The function correctly propagates errors from
confirmUpdateand returns the effective version appropriately for both success and decline cases.
86-121: LGTM!Good security practice to validate image tags at the trust boundary before use. The fallback to "latest" for invalid/empty tags provides a safe default.
123-169: LGTM!The function correctly threads
effectiveVersionthrough topullAndPersist, skips updates when already at target, and handles Docker unavailability gracefully.
171-185: LGTM!Correctly propagates
form.Run()errors (includinghuh.ErrUserAbortedon Ctrl+C) instead of treating them as declined confirmations.
228-245: LGTM!The function correctly overrides
CLIVersionwitheffectiveVersion(line 235) to ensure the compose.yml metadata reflects the updated CLI version, not the stale in-memory value.
247-292: LGTM!Good error handling and UX: graceful fallback with manual restart instructions on errors, appropriate non-interactive handling, and health check failure as warning rather than error.
294-307: LGTM!Correct implementation with proper error propagation. The non-interactive check is correctly handled by the caller at lines 262-265.
187-226: Path traversal concern is mitigated byconfig.SecurePathvalidation.The
safeDirreturned fromsafeStateDir()is validated byconfig.SecurePath(), which enforces absolute paths viafilepath.IsAbs()and normalizes withfilepath.Clean(). This prevents path traversal attacks, so the CodeQL warnings are false positives in this context.The rollback logic is correctly implemented — it handles both backup exists and non-existent file cases, and is called on all failure paths including
regenerateComposefailures.cli/cmd/uninstall.go (1)
222-226: The containment check is correct as written.
filepath.Rel()always normalizes paths usingfilepath.Clean()and usesfilepath.Join()which enforces proper path separators. It cannot produce malformed relative paths like..bin\app.exewithout a separator. The current check!strings.HasPrefix(rel, "..")correctly identifies paths outside the parent directory for all cases thatfilepath.Rel()can actually return (.,..,..\\sibling,subdir\\file, etc.).The existing test suite in
uninstall_test.gocovers this function comprehensively and all tests pass. The same check pattern is used elsewhere in the codebase (e.g.,cli/internal/completion/install.goline 250) without this additional safeguard.
| @pytest.mark.unit | ||
| class TestHealthCheckUnconfiguredServices: | ||
| """Health endpoint with partially or fully unconfigured services.""" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ( | ||
| "persistence_state", | ||
| "bus_state", | ||
| "expected_status", | ||
| "expected_persistence", | ||
| "expected_bus", | ||
| ), | ||
| [ | ||
| pytest.param(None, None, "ok", None, None, id="no_services"), | ||
| pytest.param( | ||
| "healthy", None, "ok", True, None, id="persistence_only_healthy" | ||
| ), | ||
| pytest.param( | ||
| "unhealthy", | ||
| None, | ||
| "down", | ||
| False, | ||
| None, | ||
| id="persistence_only_unhealthy", | ||
| ), | ||
| pytest.param(None, "healthy", "ok", None, True, id="bus_only_healthy"), | ||
| pytest.param( | ||
| None, | ||
| "unhealthy", | ||
| "down", | ||
| None, | ||
| False, | ||
| id="bus_only_unhealthy", | ||
| ), | ||
| ], | ||
| ) | ||
| async def test_unconfigured_services( | ||
| self, | ||
| persistence_state: str | None, | ||
| bus_state: str | None, | ||
| expected_status: str, | ||
| expected_persistence: bool | None, | ||
| expected_bus: bool | None, | ||
| ) -> None: | ||
| backend = None | ||
| bus = None | ||
| if persistence_state is not None: | ||
| backend = FakePersistenceBackend() | ||
| await backend.connect() | ||
| if bus_state is not None: | ||
| bus = FakeMessageBus() | ||
| await bus.start() | ||
|
|
||
| with TestClient( | ||
| create_app(persistence=backend, message_bus=bus), | ||
| ) as client: | ||
| # Simulate post-startup failures after app lifecycle completes. | ||
| if persistence_state == "unhealthy" and backend is not None: | ||
| backend._connected = False | ||
| if bus_state == "unhealthy" and bus is not None: | ||
| bus._running = False | ||
|
|
||
| response = client.get("/api/v1/health") | ||
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["data"]["status"] == expected_status | ||
| assert body["data"]["persistence"] is expected_persistence | ||
| assert body["data"]["message_bus"] is expected_bus | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| class TestHealthCheckExceptionPaths: | ||
| """Health endpoint when a configured service raises an exception.""" | ||
|
|
||
| async def test_persistence_exception_returns_false(self) -> None: | ||
| backend = FakePersistenceBackend() | ||
| await backend.connect() | ||
| with ( | ||
| TestClient(create_app(persistence=backend)) as client, | ||
| patch.object( | ||
| type(backend), | ||
| "health_check", | ||
| side_effect=RuntimeError("connection lost"), | ||
| ), | ||
| ): | ||
| response = client.get("/api/v1/health") | ||
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["data"]["persistence"] is False | ||
| assert body["data"]["status"] == "down" | ||
|
|
||
| async def test_message_bus_exception_returns_false(self) -> None: | ||
| bus = FakeMessageBus() | ||
| await bus.start() | ||
| with ( | ||
| TestClient(create_app(message_bus=bus)) as client, | ||
| patch.object( | ||
| type(bus), | ||
| "is_running", | ||
| new_callable=PropertyMock, | ||
| side_effect=RuntimeError("internal error"), | ||
| ), | ||
| ): | ||
| response = client.get("/api/v1/health") | ||
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["data"]["message_bus"] is False | ||
| assert body["data"]["status"] == "down" |
There was a problem hiding this comment.
Add a 30-second timeout marker to these new tests.
Line 52 and Line 122 introduce new test classes without a timeout marker. Please add @pytest.mark.timeout(30) so each test has an explicit upper bound.
Proposed patch
`@pytest.mark.unit`
+@pytest.mark.timeout(30)
class TestHealthCheckUnconfiguredServices:
@@
`@pytest.mark.unit`
+@pytest.mark.timeout(30)
class TestHealthCheckExceptionPaths:As per coding guidelines, "Set 30-second timeout per test."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_health.py` around lines 52 - 159, Add a 30-second timeout
marker to both test classes by annotating TestHealthCheckUnconfiguredServices
and TestHealthCheckExceptionPaths with `@pytest.mark.timeout`(30); locate the
class definitions for TestHealthCheckUnconfiguredServices and
TestHealthCheckExceptionPaths in tests/unit/api/test_health.py and add the
decorator directly above each class so every test in those classes has an
explicit 30s timeout.
- UNC safety check: only reject UNC share roots (\server\share), not arbitrary subdirectories under UNC shares - Parametrize exception-path health tests into single test with service_spec dict to stay under PLR0913 arg limit
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/api/test_health.py (1)
52-53:⚠️ Potential issue | 🟠 MajorAdd explicit 30s timeout markers to both new test classes.
TestHealthCheckUnconfiguredServicesandTestHealthCheckExceptionPathsare missing@pytest.mark.timeout(30). Please add it above each class to enforce bounded test runtime in CI.Proposed patch
`@pytest.mark.unit` +@pytest.mark.timeout(30) class TestHealthCheckUnconfiguredServices: @@ `@pytest.mark.unit` +@pytest.mark.timeout(30) class TestHealthCheckExceptionPaths:As per coding guidelines, "Set 30-second timeout per test."
Also applies to: 122-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/test_health.py` around lines 52 - 53, Add an explicit 30s pytest timeout decorator to both test classes by placing `@pytest.mark.timeout`(30) immediately above the class definitions for TestHealthCheckUnconfiguredServices and TestHealthCheckExceptionPaths so each test in those classes is bounded to 30 seconds; ensure you import pytest if not already present and preserve any existing decorators/order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/api/test_health.py`:
- Around line 52-53: Add an explicit 30s pytest timeout decorator to both test
classes by placing `@pytest.mark.timeout`(30) immediately above the class
definitions for TestHealthCheckUnconfiguredServices and
TestHealthCheckExceptionPaths so each test in those classes is bounded to 30
seconds; ensure you import pytest if not already present and preserve any
existing decorators/order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6a1df72-b3d4-47d7-a998-8d8390db64e4
📒 Files selected for processing (2)
cli/cmd/uninstall.gotests/unit/api/test_health.py
📜 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 Vulnerability Check
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Useexcept A, B:syntax (no parentheses) for exception handling — ruff enforces this on Python 3.14 (PEP 758 except syntax).
Include type hints on all public functions and classes. Strict mypy type-checking is enforced.
Use Google-style docstrings on all public classes and functions. Docstrings are enforced by ruff D rules.
Line length is 88 characters (enforced by ruff).
Files:
tests/unit/api/test_health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark tests with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow. Maintain 80% coverage minimum. Useasyncio_mode = "auto"(no manual@pytest.mark.asyncioneeded). Set 30-second timeout per test. Always include-n autowhen running pytest (never sequential). Use@pytest.mark.parametrizefor testing similar cases.
Use Python Hypothesis for property-based testing with@given+@settings. Profiles:ci(200 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var. Run dev profile:HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties..hypothesis/is gitignored.
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins.
Files:
tests/unit/api/test_health.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Use Cobra framework for CLI commands. Organize commands incmd/directory. Use charmbracelet/huh for interactive prompts and charmbracelet/lipgloss for styled CLI output.
Run Go linting withgolangci-lint run, vetting withgo vet ./..., testing withgo test ./.... Type-check and lint Go code in CI on changes tocli/**files.
Files:
cli/cmd/uninstall.go
🧠 Learnings (7)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : Mark tests with `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, or `pytest.mark.slow`. Maintain 80% coverage minimum. Use `asyncio_mode = "auto"` (no manual `pytest.mark.asyncio` needed). Set 30-second timeout per test. Always include `-n auto` when running pytest (never sequential). Use `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-16T06:18:07.373Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:18:07.373Z
Learning: Applies to tests/**/*.py : NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/test_health.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/uninstall.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/uninstall.go
🧬 Code graph analysis (1)
tests/unit/api/test_health.py (3)
src/synthorg/api/app.py (1)
create_app(426-550)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-504)src/synthorg/api/state.py (2)
persistence(115-117)message_bus(125-127)
🔇 Additional comments (7)
tests/unit/api/test_health.py (1)
56-87: Nice coverage expansion with parameterized matrices.The service-state matrix and exception-path parametrization make the health behavior checks more complete and easier to extend.
Also applies to: 126-152
cli/cmd/uninstall.go (6)
3-16: LGTM!The imports are well-organized and necessary for the Windows-aware deletion logic. The file correctly uses Cobra and charmbracelet/huh as per coding guidelines.
117-142: LGTM!The safety guards properly protect against accidental removal of critical directories:
- Case-insensitive home directory comparison on Windows
- UNC share root detection (not nested paths)
- Drive root and Unix root checks
These guards address the static analysis concern about uncontrolled path data at line 162.
143-166: LGTM!The Windows-specific binary skip logic correctly:
- Resolves symlinks to get the actual binary path
- Uses
isInsideDirfor case-insensitive path containment check- Calls
removeAllExceptto preserve the running binary- Provides clear feedback about what was removed/skipped
194-208: LGTM!The PowerShell approach with
-LiteralPathis safer than the previouscmd /C delapproach. The single-quote escaping correctly handles paths containing quotes. This addresses the previous review concern about cmd.exe metacharacters.
213-231: LGTM!The
isInsideDirhelper correctly handles:
- Path normalization via
filepath.Clean- Case-insensitive comparison on Windows
- Proper containment check using
filepath.RelThe Unicode limitation is appropriately documented and acceptable for typical Windows user profile paths.
238-289: LGTM!The
removeAllExceptfunction correctly implements:
- Case-insensitive exclusion matching on Windows
- Deepest-first removal order for proper directory cleanup
- Root directory preservation
- Selective error suppression only for expected "directory not empty" cases on ancestor paths
- Error aggregation with
errors.JoinThis addresses the previous review concerns about case-sensitive comparison and improper error suppression.
🤖 I have created a release *beep* *boop* --- ## [0.2.8](v0.2.7...v0.2.8) (2026-03-16) ### Features * add RRF rank fusion to memory ranking ([#478](#478)) ([42242b5](42242b5)) * collaboration scoring enhancements — LLM sampling and human override ([#477](#477)) ([b3f3330](b3f3330)) ### Bug Fixes * add .gitattributes to enforce LF line endings for Go files ([#483](#483)) ([1b8c7b6](1b8c7b6)) * **cli:** Windows uninstall, update UX, health check, sigstore ([#476](#476)) ([470ca72](470ca72)) ### Refactoring * **web:** extract WebSocket subscription into reusable composable ([#475](#475)) ([96e6c46](96e6c46)), closes [#351](#351) ### Maintenance * bump hypothesis from 6.151.5 to 6.151.9 in the minor-and-patch group ([#482](#482)) ([a7297d5](a7297d5)) * bump nginxinc/nginx-unprivileged from `aec540f` to `ccbac1a` in /docker/web ([#479](#479)) ([176e052](176e052)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
status="ok"when persistence/message_bus are not configured (fresh start). ChangesHealthStatusfields frombooltobool | None—Nonemeans "not configured",Falsemeans "configured but unhealthy". Fixes Docker HEALTHCHECK failures that prevented web container from starting on fresh installations.removeAllExceptskips the running binary during config dir removal,confirmAndRemoveBinaryprints a deferredcmd /C delcommand. Linux/macOS behavior unchanged (unlink works on running binaries). Case-insensitive path comparison for NTFS.WithIntegratedTimestamps(1)to verifier (required by sigstore-go v1.1+).Test plan
go vetcleangolangci-lintcleanruff check+ruff formatcleanmypycleansynthorg uninstallon Windows — verify binary skip + deferred cleanup hintsynthorg update— verify version check, confirmation, skip-if-matchingdocker compose upwith fresh backend (no company config) — verify HEALTHCHECK passesReview coverage
Pre-reviewed by 6 agents (code-reviewer, go-reviewer, go-security-reviewer, go-conventions-enforcer, python-conventions-enforcer, docs-consistency). 8 findings addressed in a dedicated fix commit.
🤖 Generated with Claude Code