fix: align the local runtime contract - #49
Conversation
📝 WalkthroughWalkthroughThe CLI now stores project metadata in local markers, discovers projects from nested directories, accepts existing UI/Core checkouts, and updates command workflows. Compose files use pinned images and revised host/container networking. Documentation and runtime tests cover the new contracts. ChangesProject configuration and runtime workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The PR changes initialization, project-state persistence, and Docker startup behavior. At the current head, an interrupted initialization can corrupt the project marker, Docker startup can wait indefinitely, and validation is blocked by excessive setup complexity, so merge should wait for these concrete issues to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant User
participant InitCommand
participant ProjectMarker
participant ProjectCommand
participant Compose
participant Core
participant UI
User->>InitCommand: provide project or checkout paths
InitCommand->>ProjectMarker: write .kubeorch/project.json
User->>ProjectCommand: run a project-scoped command
ProjectCommand->>ProjectMarker: discover and validate marker
ProjectCommand->>Compose: validate and start configured services
UI->>Core: request /v1/api
ProjectCommand->>Core: perform health check
ProjectCommand->>UI: perform health check
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Mohit Nagaraj <mohitnagaraj20@gmail.com>
0c6f2a4 to
5746d42
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if dockerDesktop == "" { | ||
| continue | ||
| } | ||
| if _, err := os.Stat(dockerDesktop); err != nil { |
| continue | ||
| } | ||
| fmt.Println(" opening docker desktop...") | ||
| if err := exec.Command(dockerDesktop).Start(); err != nil { |
| continue | ||
| } | ||
| fmt.Println(" opening docker desktop...") | ||
| if err := exec.Command(dockerDesktop).Start(); err != nil { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
cmd/init.go (1)
132-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not require Git for existing-checkout initialization.
checkPrerequisitesalways checks or installs Git before this function resolves existing paths. With--ui-pathor--core-pathplus--skip-deps, no later operation needs Git. This documented adoption flow fails on systems without Git and can trigger an unnecessary installation.Check Git only when
cloneUI || cloneCore. Keep Docker Compose validation for every mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f345606-3cc2-4f6e-b85c-a1a21f48f570
📒 Files selected for processing (26)
README.mdcmd/config.gocmd/debug.gocmd/docker/docker-compose.dev.ymlcmd/docker/docker-compose.hybrid-core.ymlcmd/docker/docker-compose.hybrid-ui.ymlcmd/docker/docker-compose.prod.ymlcmd/exec.gocmd/init.gocmd/logs.gocmd/restart.gocmd/root.gocmd/runtime_contract_test.gocmd/start.gocmd/status.gocmd/stop.gocmd/testing.gocmd/utils.godocker/docker-compose.dev.ymldocker/docker-compose.hybrid-core.ymldocker/docker-compose.hybrid-ui.ymldocker/docker-compose.prod.ymldocs/ARCHITECTURE.mddocs/CONCURRENT-OPERATIONS.mddocs/CONFIGURATION.mdtests/unit/cmd_test.go
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| type projectMarker struct { | ||
| Version int `json:"version"` | ||
| UIPath string `json:"ui_path,omitempty"` | ||
| CorePath string `json:"core_path,omitempty"` | ||
| Mode string `json:"mode"` | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reorder projectMarker fields to pass lint.
The lint check reports that this layout has unnecessary GC pointer bytes. Move Version after the string fields. JSON field names and marker compatibility remain unchanged.
🧰 Tools
🪛 GitHub Actions: golangci-lint / 0_lint.txt
[error] 22-22: golangci-lint fieldalignment (govet): struct with 48 pointer bytes could be reduced to 40 by reordering its fields.
🪛 GitHub Actions: golangci-lint / lint
[error] 22-22: golangci-lint fieldalignment (govet): struct with 48 pointer bytes could be reduced to 40 by reordering its fields.
🪛 GitHub Check: lint
[failure] 22-22:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
Source: Linters/SAST tools
| markerPath := filepath.Join(markerDir, projectMarkerFilename) | ||
| if err := os.WriteFile(markerPath, data, configFilePerm); err != nil { | ||
| return fmt.Errorf("failed to write project marker %s: %w", markerPath, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Write the project marker atomically.
os.WriteFile truncates the current marker before it writes replacement data. An interrupted orchcli init can leave .kubeorch/project.json empty or partial. All lifecycle commands then fail marker parsing until the user repairs the project.
Write and sync a temporary file in markerDir, then rename it over markerPath. Add a test for a partial-write failure path.
| } | ||
|
|
||
| func setupDevelopment(cloneUI, cloneCore bool) error { | ||
| func setupDevelopment(cloneUI, cloneCore, useExistingUI, useExistingCore bool) error { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split setupDevelopment to restore the lint threshold.
The lint check reports cyclomatic complexity 53 with a maximum of 15. This prevents the validation pipeline from completing. Extract checkout resolution, clone execution, dependency setup, configuration generation, and completion messaging into focused helpers.
🧰 Tools
🪛 GitHub Check: lint
[failure] 132-132:
cyclomatic complexity 53 of func setupDevelopment is high (> 15) (gocyclo)
Source: Linters/SAST tools
|
|
||
| uiLocal := projectConfig.UIPath != "" && dirExists(projectConfig.UIPath) | ||
| coreLocal := projectConfig.CorePath != "" && dirExists(projectConfig.CorePath) | ||
| if err := validateDockerCompose(); err != nil { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Propagate command cancellation to all health-check requests.
The status paths use context-free HTTP helpers, so a canceled command can continue waiting for health responses and the enabled noctx lint check rejects the code. Build each request with http.NewRequestWithContext(cmd.Context(), ...) and execute it with client.Do.
📍 Affects 1 file
cmd/status.go#L32-L32(this comment)cmd/status.go#L104-L115
Source: Linters/SAST tools
| func waitForDockerDaemon(maxSeconds int) error { | ||
| fmt.Println(" waiting for docker to start...") | ||
| for i := 0; i < maxSeconds; i++ { | ||
| if err := checkCommand("docker", "info"); err == nil { | ||
| return nil | ||
| } | ||
| time.Sleep(time.Second) | ||
| } | ||
| return fmt.Errorf("docker desktop did not start within %d seconds", maxSeconds) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- cmd/utils.go relevant symbols ---'
rg -n -A35 -B15 'func waitForDockerDaemon|func checkCommand|waitForDockerDaemon\(' cmd/utils.go
printf '%s\n' '--- command execution and context usage ---'
rg -n 'exec\.Command(Context)?|checkCommand\(' --glob '*.go' .
printf '%s\n' '--- module/toolchain context ---'
sed -n '1,80p' go.modRepository: KubeOrch/cli
Length of output: 9895
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
source = Path("cmd/utils.go").read_text()
check = re.search(
r'func checkCommand\(name string, args \.\.\.string\) error \{\s*'
r'cmd := exec\.Command\(name, args\.\.\.\)\s*'
r'return cmd\.Run\(\)\s*\}',
source,
)
wait = re.search(
r'func waitForDockerDaemon\(maxSeconds int\) error \{(?P<body>.*?)\n\}',
source,
re.S,
)
assert check, "checkCommand is not an unbounded exec.Command(...).Run() wrapper"
assert wait, "waitForDockerDaemon was not found"
body = wait.group("body")
assert body.count('checkCommand("docker", "info")') == 1
assert body.count("time.Sleep(time.Second)") == 1
assert "exec.CommandContext" not in body
print("checkCommand invokes docker info with exec.Command(...).Run()")
print("waitForDockerDaemon performs one blocking check per iteration")
print("waitForDockerDaemon has no per-command context or deadline")
PY
printf '%s\n' '--- relevant imports ---'
sed -n '1,35p' cmd/utils.goRepository: KubeOrch/cli
Length of output: 1334
Bound the Docker readiness check with a context deadline.
If docker info hangs, checkCommand waits indefinitely and maxSeconds never limits waitForDockerDaemon. Use exec.CommandContext with one context.WithTimeout covering the full polling operation.
| **When:** Only Core repository cloned | ||
| **Purpose:** Backend development without frontend setup | ||
| ```bash | ||
| orchcli init --ui-path ./ui --core-path ./core --skip-deps |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not skip dependency installation in the bootstrap command.
Line 43 passes --skip-deps, but the next step runs npm run dev. On a fresh UI checkout, that command can fail because local packages were not installed. Remove the flag so orchcli init performs the dependency setup described by this PR.
Proposed fix
-orchcli init --ui-path ./ui --core-path ./core --skip-deps
+orchcli init --ui-path ./ui --core-path ./core📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| orchcli init --ui-path ./ui --core-path ./core --skip-deps | |
| orchcli init --ui-path ./ui --core-path ./core |
| ```go | ||
| tasks := []Task{ | ||
| {Name: "Checking PostgreSQL", Action: checkPostgres}, | ||
| {Name: "Checking MongoDB", Action: checkMongoDB}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the health-check example with runStart.
Line 92 presents checkMongoDB as a concurrent orchcli start task. cmd/start.go calls waitForMongoDB only after detached Compose startup. It does not run concurrent Core or UI checks. Replace this example with the current behavior, or add the documented task group to runStart.
| # Start Core (Terminal 1) | ||
| cd core && air | ||
| cd core && go run . | ||
|
|
||
| # Start UI (Terminal 2) | ||
| cd ui && npm run dev |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the Core hot-reload documentation.
go run . does not watch source files. The feature list states that all development modes support hot reload, but this command requires a manual restart after Core changes. State that requirement here, or document and invoke a supported watcher.
2 participants
Summary
.kubeorch/project.jsonmarker with parent-directory discovery and safe legacy migration behaviororchcli initto adopt existing UI/Core checkouts, make initialization idempotent, and improve Windows Docker Desktop handling and service health reportingValidation
go test ./...go vet ./...docker compose ... config --quietfor all four shipped Compose modesstatusfrom nested UI/Core directories201, login200, authenticated dashboard/settings requests200, admin role rendered, and workspace create/list succeeded with no browser console errorsRemaining dependency
The currently published Core and UI
v0.0.3images expose AMD64 manifests only. This PR documents that limitation and pins the available images, but actual macOS ARM64 production/hybrid execution remains blocked on multi-arch component releases. For that reason this PR references rather than closes the runtime issue.Refs #47