feat: local workbench gitops and dory registry host access - #89
feat: local workbench gitops and dory registry host access#89patrickleet wants to merge 15 commits into
Conversation
Move all colima-specific operations behind a Backend enum in src/commands/local/backend/ and rename ColimaSizeArgs to SizeArgs. Registry wiring (hosts sync) now flows through backend::wire_local_registry so provider/config installs stay backend-agnostic. No behavior change: identical command invocations on the colima path. Implements [[tasks/cluster-backend-abstraction]] (phase 1)
Adds --backend <colima|kind> (global on hops local) with resolution order flag > persisted (~/.hops/local/backend) > detected cluster (colima wins for back-compat) > platform default (macOS colima, else kind). kind backend: create with a pinned 127.0.0.1:30500 port mapping for the in-cluster registry NodePort, docker start/stop of the node container for resume, destroy/reset via kind delete + recreate, and containerd certs.d hosts.toml aliases (both registry names -> registry ClusterIP over HTTP) written idempotently after the registry deploys. Sizing flags error on kind. HOPS_KUBE_CONTEXT is auto-set from the backend when --context is absent. Preflight enforces kind >= 0.27 (certs.d config_path default). Implements [[tasks/cluster-backend-abstraction]] (phase 2)
GH Actions workflow exercises the CI acceptance path on ubuntu-latest: hops local start --backend kind, doctor, a registry round-trip through both pull names (Service name and localhost:30500), the stop/start resume path via the persisted backend, and destroy. README documents backend selection, persistence/resolution order, and the dory-via-kind recipe. Implements [[tasks/cluster-backend-abstraction]] (phases 3-4)
Backend::Dory drives dory's built-in k3s headlessly via the dory CLI (k8s enable/disable/status) and the engine docker socket: - start: writes ~/.dory/k8s/registries.yaml (k3s-native trust aliasing both registry pull names to the Service hostname over HTTP; read at boot via dory's bind mount), then 'dory k8s enable --publish 30500:30500'. The Dory app's port forwarder makes the published NodePort host-reachable. - wire_registry: syncs hostname -> ClusterIP in the node's /etc/hosts per start (in-place rewrite — /etc/hosts is a bind mount, sed -i fails). - stop/resume via docker stop/start of dory-k8s; destroy/reset via dory k8s disable (+ enable); sizing flags error (VM sized in the app). - kube_context 'dory' (dory names its kubeconfig context that); hops prepends ~/.kube/dory-config to KUBECONFIG for its kubectl/helm children. - detection order: colima > kind > dory. Also: 'helm repo update crossplane-stable' instead of bare update — a stale unrelated repo in the user's helm config no longer breaks start. Verified end-to-end locally: start (Crossplane 2.3.3 + providers + registry), doctor all green, registry round-trip through BOTH pull names to pod Ready, stop/start resume, doctor green again. Implements [[tasks/dory-native-backend]]
[[tasks/rr-2-dory-contract]]
Current dory merges the dory context into ~/.kube/config at enable time, so hops no longer needs to mutate KUBECONFIG for child processes; the side-file prepend remains as a fallback for pre-merge dory versions. Implements [[tasks/dory-kubeconfig-merge]]
Launching Dory.app re-provisions its engine for ~90s and restarts dockerd in the VM at the end, SIGTERMing every container — a k3s node enabled during that window reports Ready and then dies mid-bootstrap. The engine socket's mtime marks the session start, so when it is younger than 180s, start/reset now watch the node until the window passes and re-enable it (up to 3x) if the engine restart takes it down. Steady-state starts pay one container inspect. Implements [[tasks/rr-2-dory-contract]]
Add hops local up/down/status/open/gitops with workspace registry, helm template apply + labels, env/chart FS watch, host access URL planning, and auto source-delivery selection. Unit tests cover parse/merge/labels/registry/watch/delivery/net. Implements [[tasks/lwb-application-reconcile]] [[tasks/lwb-gitops-watch]] [[tasks/lwb-up-down-workspace]] [[tasks/lwb-status-net]] [[tasks/lwb-source-delivery]] [[tasks/lwb-docs-happy-path]]
- Probe node path visibility via docker/kubectl hostPath (not host is_dir) - Sync fallback: tar|kubectl with default_sync_ignores; mutagen when present - Host access starts kubefwd or falls back to kubectl port-forward; down stops PIDs - Stamp pod-template labels for workspace discovery - e2e-ui cluster-dev waits for .hops-synced when sourceDelivery.mode=sync Implements residual [[tasks/lwb-source-delivery]] [[tasks/lwb-status-net]]
Resolve source host roots per Application (deliveryPath / service root) so UI mounts ui/ (vite package.json) while API mounts the Cargo workspace — not a single monorepo root for every pod. Fix continuous tar watcher to sync_all apps with distinct host paths. Keep infinite wait-for-sync in cluster-dev when mode=sync. Tests cover deliveryPath resolution and multi-app watch script.
Current Dory app exposes the Docker API on hv/engine.sock; hops still looked only at the legacy ~/.dory/engine.sock path and failed preflight even when the app and engine were running.
Modern `dory k8s` is a kubectl wrapper only. Create/start the dory-k8s container on the engine socket with runc (avoid dead dory-runc pins), write ~/.kube/dory-config with a `dory` context, and keep registry ports/yaml mounts. Verified: hops local start --backend dory succeeds.
Add hops local gitops cluster/worktree flows, map-mode host access for package registry (127.0.0.1:30500), TLS-aware registry probe, and dory registries inject without host bind mounts. Supports config install --path dogfood of multi-backend SecretStack on stock dory.
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesBackend-neutral local workflows and GitOps workbench
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant User
participant LocalCLI
participant Backend
participant GitOps
participant Kubernetes
participant HostAccess
User->>LocalCLI: run local up
LocalCLI->>Backend: resolve and start backend
Backend->>Kubernetes: wait for API readiness
LocalCLI->>GitOps: reconcile cluster and application manifests
GitOps->>Kubernetes: apply rendered resources
LocalCLI->>HostAccess: discover services and start forwards
HostAccess->>Kubernetes: create service port-forwards
HostAccess-->>LocalCLI: return workspace URLs and runtime state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
skills/claude/references/local-source-packages.md-35-35 (1)
35-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse clearer wording in the install-mode heading.
Replace
stable dogfoodwithstable dogfoodingorpublished stable packages. The current phrase is ambiguous in user-facing documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/claude/references/local-source-packages.md` at line 35, Update the “Published” install-mode heading to replace the ambiguous “stable dogfood” wording with either “stable dogfooding” or “published stable packages,” while preserving the heading’s meaning.Source: Linters/SAST tools
README.md-9-15 (1)
9-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the backend support matrix consistent.
These new sections use different backend lists. Some mention only Colima and kind, while the later backend section supports Dory. The prerequisites also still require macOS, although kind is documented and tested on Ubuntu.
State Dory’s external prerequisites, list all supported backends consistently, and split platform requirements by backend.
Also applies to: 59-59, 107-107, 238-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 9 - 15, Update the README’s backend support matrix consistently across the overview, prerequisites, local-development, and referenced sections. Document Dory’s external prerequisites, include Colima, kind, and Dory wherever supported backends are listed, and split platform requirements by backend so macOS applies to Colima while Ubuntu support is stated for kind and Dory.skills/claude/references/config-install.md-42-42 (1)
42-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSpecify the IPv4 bind in the documented port-forward.
The README and registry code prefer
127.0.0.1to avoid Docker using[::1]. Add--address 127.0.0.1so the documented command can actually use the IPv4 address it says it uses.Proposed fix
-3. Ensures registry host access ... `kubectl port-forward -n crossplane-system svc/registry 30500:5000` on `127.0.0.1` +3. Ensures registry host access ... `kubectl port-forward --address 127.0.0.1 -n crossplane-system svc/registry 30500:5000`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/claude/references/config-install.md` at line 42, Update the documented registry port-forward command to include kubectl’s explicit IPv4 bind option `--address 127.0.0.1`, while preserving the existing namespace, service, and port mapping.src/commands/local/backend/colima.rs-375-398 (1)
375-398: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAnchor the
seddelete pattern to whole-line host entries.
/{escaped_host}/dis unanchored. It deletes every line that contains the hostname as a substring. A line for a different host that ends with the same suffix, or a comment that names the registry, is also removed. Match the hostname as a distinct field instead.🐛 Proposed fix to restrict the deletion
- let escaped_host = hostname.replace('.', "\\."); run_cmd( "colima", &[ "ssh", "--", "sudo", - "sed", - "-i", - &format!("/{}/d", escaped_host), - "/etc/hosts", + "sh", + "-c", + &format!( + "awk '$2 != \"{}\"' /etc/hosts > /tmp/hosts.hops && cat /tmp/hosts.hops > /etc/hosts && rm -f /tmp/hosts.hops", + hostname + ), ], )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/backend/colima.rs` around lines 375 - 398, Update the sed delete pattern in the hostname cleanup flow to match only complete /etc/hosts entries whose hostname is a distinct field, anchoring the escaped hostname appropriately rather than deleting any line containing it. Preserve the existing run_cmd invocation and subsequent host-entry append behavior.src/commands/local/backend/colima.rs-268-293 (1)
268-293: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSelect the
defaultprofile instead of the first parsed entry.
ColimaInstancedoes not deserialize the profilename, andparse_colima_listreturns the first entry it can parse.colima list --jsonlists every profile. The lifecycle commands in this file runcolima start/colima stopwithout--profile, so they always act on thedefaultprofile. If a user has other profiles, the status, CPU, memory, and disk values can come from the wrong profile. That produces incorrect resize prompts and incorrect running-state detection.Add
nameto the struct and select thedefaultprofile.🐛 Proposed fix to select the default profile
#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] struct ColimaInstance { + #[serde(default)] + name: String, #[serde(default)] status: String,- if let Ok(instances) = serde_json::from_str::<Vec<ColimaInstance>>(trimmed) { - return Ok(instances.into_iter().next()); - } + if let Ok(instances) = serde_json::from_str::<Vec<ColimaInstance>>(trimmed) { + return Ok(pick_default(instances)); + } - for line in trimmed - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - { - if let Ok(instance) = serde_json::from_str::<ColimaInstance>(line) { - return Ok(Some(instance)); - } - } + let instances: Vec<ColimaInstance> = trimmed + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .filter_map(|line| serde_json::from_str::<ColimaInstance>(line).ok()) + .collect(); + if !instances.is_empty() { + return Ok(pick_default(instances)); + }Add the helper:
fn pick_default(instances: Vec<ColimaInstance>) -> Option<ColimaInstance> { instances .iter() .find(|i| i.name == "default") .cloned() .or_else(|| instances.into_iter().next()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/backend/colima.rs` around lines 268 - 293, Update ColimaInstance to deserialize and retain the profile name, then modify parse_colima_list to select the instance whose name is "default" when parsing a collection or line-delimited output. Preserve the existing fallback to the first parsed instance when no default profile exists, and apply the same selection behavior to all parsed list results.src/commands/local/backend/kind.rs-209-214 (1)
209-214: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the kind certificate alias with the registry scheme.
hosts_tomlaliaseshttp://<clusterIP>:5000withskip_verify, but skips only TLS and the service manifest exposes plain HTTP on port5000. Use a matching plain-HTTP configuration for the kind cert alias, or point both the dory and kind config at the same service and scheme.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/backend/kind.rs` around lines 209 - 214, Update hosts_toml so the kind certificate alias uses a plain-HTTP registry configuration matching the service exposed on port 5000, rather than relying on skip_verify for an HTTP endpoint. Keep the alias, capabilities, and registry target consistent with the dory configuration and service scheme.src/commands/local/backend/dory.rs-827-832 (1)
827-832: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestart k3s after
inject_registries_yaml.
wire_registrywritesregistries.yamlinto the running node each start, but k3s only reads it at startup. The injectedregistries.yamlchanges do not apply tocontainerduntil k3s is restarted; otherwise package pulls can fail until the next reset/restart. Restart k3s after the injection, or run this step only when the node is starting up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/backend/dory.rs` around lines 827 - 832, The wire_registry flow must restart k3s after a successful inject_registries_yaml call so containerd reloads the updated registries.yaml. Update the success path around inject_registries_yaml to invoke the existing k3s restart mechanism, while preserving the current warning behavior when injection fails.src/commands/local/workbench/reconcile.rs-164-183 (1)
164-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNote the shallow-merge loss for nested keys, and drop
deep_clone_value.
merge_helm_valuesreplaces a whole top-level key.build_runtime_valuesinsertssourceDeliveryas a complete mapping and merges it only againstopts.runtime_values. If an Application declaresspec.source.helm.values.sourceDelivery.<key>, that key is dropped without warning. Either mergesourceDeliveryrecursively or document that runtime-owned top-level keys must not be set per app.
deep_clone_valueonly callsv.clone(). Callclone()directly and remove the helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/reconcile.rs` around lines 164 - 183, Update merge_helm_values to recursively merge nested mappings so runtime values preserve application-defined keys such as sourceDelivery.<key>, while retaining runtime values on conflicts; replace deep_clone_value calls with direct clone() calls and remove the redundant helper.src/commands/local/workbench/application.rs-175-187 (1)
175-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicated
normalize_pathcarries the same..defect in both modules. Both files contain a character-for-character identical path normalizer.out.pop()returnsfalseon an empty buffer, and the..component is then discarded, so a relative input silently resolves inside the base instead of above it.
src/commands/local/workbench/application.rs#L175-L187: fixnormalize_pathto push..when the buffer has nothing to pop, and export it for reuse.src/commands/local/workbench/watch.rs#L90-L102: delete the localnormalizeand call the exportedapplication::normalize_path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/application.rs` around lines 175 - 187, Update src/commands/local/workbench/application.rs#L175-L187 by exporting normalize_path and preserving a ParentDir component when out.pop() cannot remove anything; update src/commands/local/workbench/watch.rs#L90-L102 by deleting the duplicate normalize function and calling application::normalize_path instead.src/commands/local/workbench/net.rs-189-205 (1)
189-205: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound map-mode allocation to the workspace stride.
host_port = port_base + ihas no upper bound.MAP_PORT_STRIDEis 100, so a namespace with more than 100 services allocates ports that belong to the next workspace's range and the two workspaces fight over the same host ports.The plan also never checks that the host port is free. If another process owns the port,
kubectl port-forwardexits immediately,host_access_needs_healreports unhealthy, andensure_host_accessrestarts it on everystatuscall without ever succeeding. Consider probing withlocalhost_port_listeningbefore assigning, and returning an explicit error when the service count exceeds the stride.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 189 - 205, The HostAccessMode::Map allocation must stay within MAP_PORT_STRIDE and avoid already-occupied host ports. In the map-plan construction, return an explicit error when services exceed the stride, and use localhost_port_listening to skip unavailable ports before assigning entries; preserve the existing URL and port_map population for valid free allocations.src/commands/local/up.rs-599-614 (1)
599-614: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the hardcoded stub ports with the chart's declared ports.
default_service_stubsreturns 5180 when the application name contains "ui" and 8791 otherwise. Those values come from one specific project. Every other repository receives wrong ports in the dry-run URL card and in the fallback path at line 246 when service discovery fails.
ReconcileResult.rendered_yamlalready holds the labeled manifests. Parse theServicedocuments from it and read the realspec.ports[].port, so the stub matches what the chart declares.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/up.rs` around lines 599 - 614, Update default_service_stubs to derive each endpoint port from the Service manifests in ReconcileResult.rendered_yaml, parsing the labeled YAML documents and reading spec.ports[].port instead of using app-name-based constants. Preserve the existing endpoint construction and provide the parsed chart-declared port for dry-run and service-discovery fallback results.src/commands/local/up.rs-58-61 (1)
58-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--watchhas no effect.
wants_watchat line 363 computes!args.once && !args.dry_runand never readsargs.watch. Passing--watchchanges nothing, and passing--watch --oncestill disables the watch. The help text calls the flag "Redundant unless scripting", but it is redundant in all cases.Remove the flag, or make it force the watch on so that
--watch --dry-runis meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/up.rs` around lines 58 - 61, Update the watch-option handling around wants_watch so args.watch actually enables watching, including when combined with --dry-run, while preserving --once as the explicit disable override; alternatively remove the watch field and its CLI declaration if it is not intended to affect behavior, and keep the help text consistent with the chosen behavior.src/commands/local/workbench/net.rs-362-366 (1)
362-366: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequire all DNS port-forwards to survive, not just one.
The check uses
any, so the function reports success when a single forward of many is alive. The user then sees "host access: cluster DNS" while some service URLs never answer. The fallback to map mode is skipped because the error path is not taken.🐛 Proposed fix
std::thread::sleep(Duration::from_millis(500)); - if !rt.pids.iter().any(|p| pid_is_alive(*p)) { - return Err("dns port-forwards exited immediately".into()); - } + let dead = rt.pids.iter().filter(|p| !pid_is_alive(**p)).count(); + if dead > 0 { + return Err(format!( + "{dead} of {} dns port-forwards exited immediately", + rt.pids.len() + ) + .into()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 362 - 366, Update the DNS port-forward liveness check in the runtime setup flow to require every PID in rt.pids to remain alive, replacing the any-based condition with an all-based validation. Return the existing error whenever any forward exits, so the fallback to map mode is triggered for partial failures while preserving success when all forwards survive.src/commands/local/up.rs-453-475 (1)
453-475: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCap the debounce window so a continuous event stream cannot starve the reconcile.
Line 468 resets
deadlineon every received event. While a process writes continuously into a watched chart or env directory, the inner loop keeps extending the deadline andrebuildnever runs. Add a maximum total wait measured from the first event.🐛 Proposed fix
let mut kind = first; // Debounce and merge events + let hard_deadline = Instant::now() + debounce * 10; let mut deadline = Instant::now() + debounce; loop { - let remaining = deadline.saturating_duration_since(Instant::now()); + let remaining = deadline + .min(hard_deadline) + .saturating_duration_since(Instant::now()); if remaining.is_zero() { break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/up.rs` around lines 453 - 475, Cap the event-merge loop in the debounce logic around WatchRebuild so repeated rx.recv_timeout events cannot postpone reconciliation indefinitely. Record the first event’s timestamp or an equivalent fixed maximum deadline before entering the loop, and ensure each subsequent deadline reset is limited by that maximum while preserving event merging and channel-disconnect handling.src/commands/local/workbench/cluster_gitops.rs-155-176 (1)
155-176: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the manifest walk against symlink cycles.
collect_manifests_recfollows symlinked directories, becausePath::is_dirresolves symlinks. A symlink that points to an ancestor inside the cluster tree causes unbounded recursion and a stack overflow. Useent.file_type()so symlinks are not traversed, or track visited canonical paths.🛡️ Proposed fix
for ent in fs::read_dir(dir)? { let ent = ent?; let path = ent.path(); - if path.is_dir() { + let file_type = ent.file_type()?; + if file_type.is_symlink() { + continue; + } + if file_type.is_dir() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/cluster_gitops.rs` around lines 155 - 176, Update collect_manifests_rec to inspect each DirEntry’s file type with ent.file_type() and avoid recursing into symlinked directories, while preserving traversal of real directories and manifest collection for regular files.src/commands/local/workbench/reconcile.rs-429-445 (1)
429-445: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim the release name after truncation.
.take(53)runs aftertrim_matches('-'). If the 53rd character is-, the release name ends with-and Helm rejects it as not DNS-1123 compliant.🐛 Proposed fix
- s.trim_matches('-').chars().take(53).collect() + let truncated: String = s.trim_matches('-').chars().take(53).collect(); + truncated.trim_matches('-').to_string()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/reconcile.rs` around lines 429 - 445, Update sanitize_release_name so the final output is trimmed of leading and trailing hyphens after truncating to 53 characters; preserve the existing normalization, hyphen collapsing, and length limit while ensuring a truncated name cannot end with '-'.src/commands/config/install.rs-712-716 (1)
712-716: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winA fixed masterkey is written into a git-tracked file.
local_xr_scaffoldwrites an inline Zitadel masterkey intoauth/stack.yamlunder the gitops tree. The comment marks it dev-only, but the value is identical for every user and lands in version control. Any local AuthStack created from this scaffold shares one key. Generate a random 32-byte value per scaffold, or reference an existing Secret and leave the field empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/config/install.rs` around lines 712 - 716, Replace the hardcoded masterkey under the firstInstance configuration in local_xr_scaffold with a newly generated random 32-byte value for each scaffold, ensuring auth/stack.yaml never receives the shared literal key. Preserve the Zitadel configuration structure while sourcing the value from the scaffold’s existing secure randomness mechanism.src/commands/local/gitops.rs-50-56 (1)
50-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--watchhas no effect; only--onceis read.
run_clusterandrun_worktreebranch onargs.oncealone.args.watchis never read, so--watchis a no-op and--once --watchsilently runs once. Either remove the flag or mark it as mutually exclusive with--onceand document it as a no-op default.🔧 Proposed change
/// Watch and re-apply on YAML changes (default). Use `--once` to disable. - #[arg(long, default_value_t = false)] + #[arg(long, default_value_t = false, conflicts_with = "once")] pub watch: bool,Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/gitops.rs` around lines 50 - 56, Update the command argument definitions and handling around the once and watch fields so --watch is not exposed as a misleading no-op: either remove the watch flag entirely and retain the default watch behavior, or make --watch and --once mutually exclusive and ensure command execution honors the selected mode. Preserve single-run behavior for --once and prevent conflicting flags from silently running once.src/commands/local/workbench/registry.rs-130-141 (1)
130-141: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle unreadable record files like corrupt ones.
fs::read_to_stringpropagates an error and aborts the whole listing. A single unreadable or transiently missing file then breakshops local down,status, andopen, which all calllist_workspaces. Parse errors are already tolerated, so treat I/O errors the same way.🛡️ Proposed fix
- let text = fs::read_to_string(&path)?; - match serde_json::from_str::<WorkspaceRecord>(&text) { - Ok(r) => records.push(r), - Err(e) => log::warn!("skip corrupt workspace record {}: {e}", path.display()), - } + let text = match fs::read_to_string(&path) { + Ok(t) => t, + Err(e) => { + log::warn!("skip unreadable workspace record {}: {e}", path.display()); + continue; + } + }; + match serde_json::from_str::<WorkspaceRecord>(&text) { + Ok(r) => records.push(r), + Err(e) => log::warn!("skip corrupt workspace record {}: {e}", path.display()), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/registry.rs` around lines 130 - 141, Update the record-reading flow in list_workspaces so fs::read_to_string failures are handled like serde_json parse failures: log a warning identifying the affected path and continue scanning remaining entries instead of propagating the error. Preserve successful parsing and record collection behavior.src/commands/local/gitops_write.rs-37-44 (1)
37-44: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject absolute
rel_pathvalues too.
Path::joindiscards the base when the argument is absolute. Arel_pathof/etc/foo.yamlcontains no.., so it passes the guard and the file is written outside the gitops root. The current callers build the path from sanitized names, so this is a hardening gap rather than an active exploit, but the check is the only guard this writer has.🛡️ Proposed fix
- if f.rel_path.is_empty() || f.rel_path.contains("..") { + let rel = Path::new(&f.rel_path); + if f.rel_path.is_empty() + || f.rel_path.contains("..") + || rel.is_absolute() + || rel + .components() + .any(|c| !matches!(c, std::path::Component::Normal(_))) + { return Err(format!("invalid gitops relative path: {}", f.rel_path).into()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/gitops_write.rs` around lines 37 - 44, Update the relative-path validation in the file-writing loop to reject absolute f.rel_path values before calling root.join, while preserving the existing empty-path and parent-traversal checks and error behavior.src/commands/local/gitops.rs-190-197 (1)
190-197: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake
hops local gitops worktreecomputedelivery_modewith the same probe ashops local up.
hops local upcallsresolve_delivery_for_apps(...)and infershostPathwhen every app host path is node-visible.run_worktree()instead passes the same path map but hardcodesdelivery_mode: Some("sync".into()), so the renderedsourceDelivery.modecan differ from theupreconciliation output even when the node can see the host path. Derivedelivery_modeinrun_worktreeusing the same path-visibility result, or document thatgitops worktreedeliberately overrides delivery mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/gitops.rs` around lines 190 - 197, Update run_worktree’s ReconcileOptions construction to derive delivery_mode using the same resolve_delivery_for_apps(...) path-visibility probe used by local up, so node-visible host paths select hostPath while other cases retain the fallback behavior; remove the hardcoded "sync" value and preserve the existing app path inputs.
🧹 Nitpick comments (22)
src/commands/local/backend/colima.rs (1)
486-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the line-delimited JSON branch.
The tests cover the single-object and array shapes.
parse_colima_listalso has a per-line branch at Lines 282-290.colima list --jsonemits one JSON object per line for multiple profiles, so that branch is the common real-world path. Add a case with two lines to lock the behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/backend/colima.rs` around lines 486 - 508, Add a test alongside parse_colima_list_accepts_single_object and parse_colima_list_accepts_array_output that supplies two newline-delimited JSON objects to parse_colima_list. Assert parsing succeeds and verifies the expected instance selection and key fields, covering the per-line JSON branch used for multiple profiles.src/commands/local/start.rs (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix stale references to Renovate’s config file path.
The repo only contains
renovate.json;cli/renovate.jsonis not present. Update the comments insrc/commands/local/start.rs,bootstrap/providers/provider-helm.yaml,bootstrap/providers/provider-kubernetes.yaml, andskills/claude/references/local-setup.mdto point torenovate.jsonif Renovate runs from this repository root.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/start.rs` around lines 21 - 24, Update the Renovate configuration path references in the comments and documentation associated with CROSSPLANE_CHART_VERSION and the provider manifests/local setup guidance to use the repository-root renovate.json instead of cli/renovate.json; leave the version and surrounding instructions unchanged.renovate.json (1)
46-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTrack
CROSSPLANE_CHART_VERSIONwith the Helm datasource.
hops local startinstallscrossplane-stable/crossplanewith this value as the Helm chart version. Renovate currently reads GitHub release appVersions fromcrossplane/crossplane, which can diverge from the chart versions published athttps://charts.crossplane.io/stable. UsedatasourceTemplate: "helm"and set the Crossplane stable registry in apackageRulesrule withmatchDatasources: ["helm"], then update the grouping rule package name tocrossplane.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@renovate.json` around lines 46 - 58, Update the Renovate regex rule tracking CROSSPLANE_CHART_VERSION to use the Helm datasource, and configure the Crossplane stable chart registry through a packageRules entry matching the helm datasource. Change the grouping rule’s package name from the GitHub repository identifier to crossplane, while preserving the existing extraction and file matching behavior.src/commands/local/mod.rs (1)
224-230: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAvoid the shell in
command_exists.
command_existsbuilds a shell string fromprogram. All current callers pass literals, so this is not exploitable today. A future caller with a dynamic value would introduce command injection. Static analysis also flags this line.Resolve the binary without a shell.
♻️ Proposed refactor
pub(crate) fn command_exists(program: &str) -> bool { - Command::new("sh") - .args(["-c", &format!("command -v {} >/dev/null 2>&1", program)]) - .status() - .map(|s| s.success()) - .unwrap_or(false) + Command::new("/usr/bin/env") + .args(["which", program]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/mod.rs` around lines 224 - 230, Update command_exists to resolve the executable directly without invoking sh or constructing a shell command string; preserve its boolean success behavior by returning true only when the named program can be found, and false on lookup failure.Source: Linters/SAST tools
src/commands/local/package_install.rs (1)
294-302: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport a missing
curlbinary explicitly.
registry_push_http_oktreats any spawn failure as "not reachable". Ifcurlis absent from PATH, the probe never succeeds.ensure_registry_push_reachablethen starts the port-forward, polls for 15 seconds, and fails with an error that points at the registry Deployment. The real cause is the missing tool.Check for
curland return a targeted error, or replace the probe with a direct TCP/HTTP check in Rust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/package_install.rs` around lines 294 - 302, The registry_push_http_ok probe currently hides curl spawn failures as registry unreachability. Update registry_push_http_ok and its caller ensure_registry_push_reachable to detect a missing curl binary and return a targeted error identifying curl as the cause, while preserving the existing HTTP/HTTPS reachability checks for available curl installations.src/commands/local/up.rs (1)
672-676: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate
probe_for_testsbehind#[cfg(test)].The function is public, marked
#[allow(dead_code)], and documented as existing for tests. It ships in release builds as unused public API. Replace the attribute with#[cfg(test)], or move the wrapper into the test module that needs it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/up.rs` around lines 672 - 676, Gate the test-only wrapper probe_for_tests with #[cfg(test)] so it is compiled only in test builds; remove the unnecessary #[allow(dead_code)] while preserving its delegation to probe_node_path_visibility.src/commands/local/workbench/cluster_gitops.rs (2)
185-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable example-file conditions.
Line 185 already returns
falsefor any name that does not end in.yamlor.yml. After that guard,name.ends_with(".example"),name.ends_with(".yaml.example"), andname.ends_with(".yml.example")can never be true. Onlyname.contains(".example.")at line 192 filters anything. The test at line 429 passes becauseaws.yaml.examplefails the extension check, not the example checks.♻️ Proposed cleanup
- if name.ends_with(".example") || name.ends_with(".example.yaml") || name.ends_with(".example.yml") - { - return false; - } if name.contains(".example.") { return false; } - // *.yaml.example pattern: file name ends with .example already handled; - // also skip foo.yaml.example via ends_with .example above when full name is x.yaml.example - if name.ends_with(".yaml.example") || name.ends_with(".yml.example") { - return false; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/cluster_gitops.rs` around lines 185 - 199, Remove the unreachable ends_with(".example"), ends_with(".yaml.example"), and ends_with(".yml.example") checks from the filename filter, including their related comment. Keep the initial YAML-extension guard and the name.contains(".example.") condition unchanged.
245-280: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider a retry pass for manifests that fail on missing CRDs.
Manifests apply once in a single ordered pass. When
packages/installs a Configuration, the CRDs it brings are not established when the following custom resources apply, so those applies fail with "no matches for kind" and land inresult.errors. The caller insrc/commands/local/up.rsonly logs a warning, so the user sees a partial cluster with no automatic recovery. A second pass over failed paths after a short wait, orkubectl wait --for condition=established, would make the firsthops local upconverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/cluster_gitops.rs` around lines 245 - 280, Update the manifest application flow around apply_one so failed manifests with missing-CRD errors are retried after a short wait or CRD-establishment check. Reprocess only eligible paths from result.errors, update applied and error collections based on the retry outcome, and preserve the existing all-failed and partial-result reporting behavior after retries complete.src/commands/local/workbench/watch.rs (2)
90-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
normalize_pathinapplication.rs.This function is character-for-character identical to
normalize_pathinsrc/commands/local/workbench/application.rslines 175-187, including the..-dropping behavior on an empty buffer. Export one implementation and call it from both modules so a fix applies in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/watch.rs` around lines 90 - 102, Remove the duplicate normalize implementation in watch.rs and reuse the existing normalize_path implementation from application.rs. Export normalize_path as needed and update the watch code to call it, preserving the current path-normalization behavior in both modules.
117-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead references keep unused imports alive in two files. Both sites add a no-op statement whose only purpose is to suppress an unused-import warning. Remove the placeholder and the import in each case.
src/commands/local/workbench/watch.rs#L117-L118: delete_use_appand dropApplicationfrom theuse super::application::{...}list at line 3.src/commands/local/up.rs#L526-L533: deletelet _ = select_delivery_strategy;and dropselect_delivery_strategyfrom theuse super::workbench::delivery::{...}list at line 9.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/watch.rs` around lines 117 - 118, The no-op references only suppress unused-import warnings; remove them and their corresponding imports. In src/commands/local/workbench/watch.rs lines 117-118, delete _use_app and remove Application from the super::application import. In src/commands/local/up.rs lines 526-533, delete let _ = select_delivery_strategy and remove select_delivery_strategy from the super::workbench::delivery import.src/commands/local/workbench/net.rs (2)
295-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
_kubefwd_availableparameter.
start_host_access_autoignores this parameter.src/commands/local/up.rsline 258 passesfalse, andensure_host_accessat line 594 also passesfalse.HostAccessMode::Kubefwdis therefore unreachable through this entry point. Remove the parameter, or use it to select the kubefwd branch when the binary is present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 295 - 302, Remove the unused _kubefwd_available parameter from start_host_access_auto and update its callers, including up.rs and ensure_host_access, to match the revised signature. Preserve the existing host-access behavior without passing the hardcoded false value.
282-288: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAvoid the shell in
command_exists.Static analysis flags the interpolation of
programintosh -c. The only current caller passes the literal"kubefwd", so this is not exploitable now. The signature still accepts any&strand a future caller could pass a name derived from configuration. Resolve the binary without a shell instead.♻️ Proposed refactor
fn command_exists(program: &str) -> bool { - Command::new("sh") - .args(["-c", &format!("command -v {program} >/dev/null 2>&1")]) - .status() - .map(|s| s.success()) - .unwrap_or(false) + std::env::var_os("PATH") + .map(|paths| { + std::env::split_paths(&paths).any(|dir| dir.join(program).is_file()) + }) + .unwrap_or(false) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 282 - 288, Update command_exists to resolve the executable directly through Rust’s process/path APIs instead of invoking sh -c or interpolating program into a shell command. Preserve its bool result and false-on-resolution-failure behavior, including for the existing kubefwd caller.Source: Linters/SAST tools
src/commands/local/workbench/reconcile.rs (1)
478-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
sanitize_release_nameandensure_namespace_on_docs.Both functions carry correctness risk and neither has direct coverage. Suggested cases: a release name longer than 53 characters that truncates on a
-; a name with only non-alphanumeric characters; aClusterRoledocument that must keep no namespace; a namespaced document that must receive the override.
Do you want me to generate these unit tests?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/reconcile.rs` around lines 478 - 697, Add focused unit tests in the existing tests module for sanitize_release_name, covering truncation to the maximum length at a hyphen boundary and input containing only non-alphanumeric characters. Add ensure_namespace_on_docs tests covering ClusterRole documents retaining no namespace and namespaced documents receiving the override namespace.src/commands/local/workbench/cluster_dns.rs (1)
347-358: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRun
ifconfig lo0once instead of once per IP.The closure at line 351 spawns
ifconfig lo0for every entry inips. The output does not change between iterations. Capture it once before the filter.♻️ Proposed refactor
+ let lo0 = Command::new("ifconfig") + .arg("lo0") + .output() + .map(|o| String::from_utf8_lossy(&o.stdout).into_owned()) + .unwrap_or_default(); let missing: Vec<String> = ips .iter() .filter(|ip| !ip.is_empty() && *ip != "127.0.0.1") - .filter(|ip| { - !Command::new("ifconfig") - .arg("lo0") - .output() - .map(|o| String::from_utf8_lossy(&o.stdout).contains(ip.as_str())) - .unwrap_or(false) - }) + .filter(|ip| !lo0.contains(ip.as_str())) .cloned() .collect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/cluster_dns.rs` around lines 347 - 358, Capture the `ifconfig lo0` output once before constructing `missing`, then have the filter closure reuse that captured content for each IP instead of spawning `Command::new("ifconfig")` per iteration. Preserve the existing handling for command failures and the exclusions for empty values and `127.0.0.1`.src/commands/local/workbench/application.rs (2)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the manual
Defaultimpl with a derive.
SyncPolicy::default()returns the same value as the derived implementation. Clippy reportsderivable_implsfor this pattern, which fails builds that deny warnings.♻️ Proposed refactor
-#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)] #[serde(rename_all = "camelCase")] pub struct SyncPolicy { #[serde(default)] pub prune: bool, } - -impl Default for SyncPolicy { - fn default() -> Self { - Self { prune: false } - } -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/application.rs` around lines 79 - 83, Replace the manual Default implementation for SyncPolicy with the Default derive on the struct, preserving prune’s existing false default and removing the impl block so Clippy’s derivable_impls warning is resolved.
213-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the comment with the fail-fast behavior.
Line 217 states that non-Application documents are skipped quietly. The code returns an error for any YAML file that does not parse as an Application. A
kustomization.yamlor any unrelated YAML in the env directory then blocks the whole reconcile. Decide on one behavior and document it. If skipping is intended, filter onapiVersion/kindbefore failing.♻️ Proposed change if skipping is intended
match parse_application_yaml(&text) { Ok(app) => apps.push((path, app)), Err(e) => { - // Multi-doc or unrelated YAML: try first document only already failed. - return Err(format!("{}: {e}", path.display()).into()); + if text.contains(APPLICATION_KIND) && text.contains(APPLICATION_API_VERSION) { + return Err(format!("{}: {e}", path.display()).into()); + } + log::debug!("skipping non-Application YAML {}: {e}", path.display()); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/application.rs` around lines 213 - 227, Align the handling and comment in the application-loading loop: either remove the claim that unrelated documents are skipped and document the existing fail-fast behavior, or, if skipping is intended, inspect each parsed document’s apiVersion/kind before returning errors and continue past non-Application YAML while still propagating genuine Application parse errors.src/commands/local/zitadel.rs (1)
116-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the required Secret inside the generated ProviderConfig file.
providerconfigs/zitadel.yamlcontains asecretRefto{namespace}/{secret_name}with keycredentials. That Secret is intentionally never written to the GitOps directory. A user who applies the committed tree to a fresh cluster gets a ProviderConfig that references a Secret which does not exist, and the provider reports an unclear credential error.Prepend a YAML comment header to the written ProviderConfig that names the required Secret and the command that creates it. The log line at line 131 conveys this only at generation time, not to the person who later reads the file in Git.
Consider also adding a write-only mode.
--gitopscurrently writes the manifests and still applies them live, so the command needs a reachable cluster even when the user only wants to generate files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/zitadel.rs` around lines 116 - 135, Before passing provider_config_yaml to write_gitops_files, prepend a YAML comment header identifying the referenced Secret as {namespace}/{secret_name}, its credentials key, and the command used to create it, while preserving valid ProviderConfig YAML. Also add a reachable write-only GitOps mode to the local Zitadel command so --gitops can generate manifests without applying them or requiring a cluster, while retaining current live-apply behavior by default.src/commands/local/gitops.rs (1)
225-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
NAMESPACE_PREFIXinstead of the literal"hops-wt-".
src/commands/local/workbench/registry.rsdefinesNAMESPACE_PREFIXandnamespace_for_namebuilds names from it. This literal will drift if the prefix changes. Import the constant and strip with it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/gitops.rs` around lines 225 - 229, Update strip_ns_prefix to import and use the existing NAMESPACE_PREFIX constant from the workbench registry module instead of the hardcoded "hops-wt-" literal, while preserving the current fallback and String conversion behavior.src/commands/local/status.rs (1)
65-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
healedbinding.The tuple returns
healed, but the value is only discarded at Line 87. The self-heal note is already printed inside the match arm. Drop the second tuple element to simplify the branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/status.rs` around lines 65 - 87, Remove the unused healed tuple element from the status command flow around ensure_host_access and plan_host_access. Return only the plan from each branch, remove the healed binding and discard statement, and preserve the existing self-heal note logging.src/commands/local/workbench/delivery.rs (1)
861-903: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote host paths and app names defensively in the generated script.
host_source_path,app_name,namespace, andmount_pathare interpolated into double-quoted shell strings. A path that contains",$, or a backslash produces a broken or re-interpreted script. Escape these values before interpolation, or pass them tosync_onethrough environment variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/delivery.rs` around lines 861 - 903, Update the generated-script construction around sync_all, needs_marker, and find_parts to safely quote host_source_path, app_name, namespace, and mount_path before interpolation. Escape double quotes, dollar signs, and backslashes for the generated shell context, and apply the same treatment consistently to sync_one arguments, kubectl selectors, test paths, and host-root find arguments.src/commands/local/aws.rs (1)
83-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--refreshsilently discards--gitopsin both credential commands. Each command returns from the--refreshbranch before the gitops write, so the user gets no files and no explanation.
src/commands/local/aws.rs#L83-L112: log a warning in the--refreshbranch whenargs.gitopsis set, or declare the two arguments as conflicting.src/commands/local/github.rs#L69-L87: apply the same handling so both commands behave identically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/aws.rs` around lines 83 - 112, Handle the incompatible --refresh and --gitops combination consistently in both src/commands/local/aws.rs lines 83-112 and src/commands/local/github.rs lines 69-87: either declare the arguments mutually exclusive or, within each command’s refresh branch, warn when args.gitops is set before returning. Ensure users are explicitly informed that refresh does not produce GitOps files.src/commands/local/open.rs (1)
104-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
discover_servicesis copied into two commands. Both files define an identical Kubernetes Service discovery helper, including thekubernetesservice filter and the port-80 default. There is no shared helper next toServiceEndpoint, so the two copies will drift.
src/commands/local/open.rs#L104-L133: delete the localdiscover_servicesand import the shared helper.src/commands/local/status.rs#L203-L232: delete the localdiscover_servicesand import the same shared helper; place the extracted function insrc/commands/local/workbench/net.rsbesideServiceEndpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/open.rs` around lines 104 - 133, Extract the duplicated discover_services helper into src/commands/local/workbench/net.rs beside ServiceEndpoint, preserving its Kubernetes service filtering and port-80 default behavior. Remove the local definitions at src/commands/local/open.rs:104-133 and src/commands/local/status.rs:203-232, and import the shared discover_services helper in both callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b20bd77c-53e7-4d32-b2a5-35126a52d39b
📒 Files selected for processing (49)
.github/workflows/on-pr-kind-smoke.yamlREADME.mdbootstrap/providers/provider-helm.yamlbootstrap/providers/provider-kubernetes.yamlrenovate.jsonskills/claude/SKILL.mdskills/claude/references/config-install.mdskills/claude/references/local-setup.mdskills/claude/references/local-source-packages.mdskills/claude/references/local-workbench.mdskills/claude/references/stacks-and-xrs.mdsrc/commands/auth/bootstrap.rssrc/commands/config/install.rssrc/commands/local/aws.rssrc/commands/local/backend/colima.rssrc/commands/local/backend/dory.rssrc/commands/local/backend/kind.rssrc/commands/local/backend/mod.rssrc/commands/local/destroy.rssrc/commands/local/doctor.rssrc/commands/local/down.rssrc/commands/local/github.rssrc/commands/local/gitops.rssrc/commands/local/gitops_write.rssrc/commands/local/install.rssrc/commands/local/mod.rssrc/commands/local/open.rssrc/commands/local/package_install.rssrc/commands/local/reset.rssrc/commands/local/resize.rssrc/commands/local/start.rssrc/commands/local/status.rssrc/commands/local/stop.rssrc/commands/local/uninstall.rssrc/commands/local/up.rssrc/commands/local/workbench/application.rssrc/commands/local/workbench/cluster_dns.rssrc/commands/local/workbench/cluster_gitops.rssrc/commands/local/workbench/delivery.rssrc/commands/local/workbench/mod.rssrc/commands/local/workbench/net.rssrc/commands/local/workbench/reconcile.rssrc/commands/local/workbench/registry.rssrc/commands/local/workbench/watch.rssrc/commands/local/zitadel.rssrc/commands/provider/install.rssrc/commands/secrets/list.rssrc/commands/vars/mod.rssrc/commands/vars/sync.rs
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== workflow excerpt ==\n'
sed -n '1,120p' .github/workflows/on-pr-kind-smoke.yaml 2>/dev/null || true
printf '\n== action checkout permissions and persist-credentials references ==\n'
rg -n "actions/checkout|persist-credentials|permissions:|cargo build|docker build|docker push" .github/workflows/on-pr-kind-smoke.yaml .github/workflows . 2>/dev/null | head -n 200
printf '\n== README/docs registry alias references ==\n'
rg -n "localhost:30500|127\.0\.0\.1:30500|regist" README.md . 2>/dev/null | head -n 120Repository: hops-ops/hops-cli
Length of output: 16718
🌐 Web query:
actions/checkout default persist-credentials true
💡 Result:
The persist-credentials input for the actions/checkout GitHub Action has a default value of true [1][2][3]. When set to true (the default), the action configures the authentication token or SSH key into the local Git configuration [1][2][4]. This allows subsequent steps in your workflow to execute authenticated Git commands without needing to manually re-configure authentication [1][2]. Important technical context regarding this setting includes: Security Evolution: Historically, the credentials were persisted directly in the repository's .git/config file [2][5]. In more recent versions, this behavior has been improved to store credentials in a separate file located in $RUNNER_TEMP, which is then included conditionally using Git's includeIf mechanism [5]. This change was designed to enhance security by isolating credentials from the repository's main Git configuration [5]. Opt-Out: If you do not want the action to persist credentials, you can explicitly set persist-credentials: false in your workflow YAML [1][2]. Cleanup: The action automatically removes the persisted credentials during the post-job cleanup phase [1][2].
Citations:
- 1: https://www.github.com/actions/checkout
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 5: Persist creds to a separate file actions/checkout#2286
🌐 Web query:
actions/checkout v4 README persist-credentials default
💡 Result:
The default value for the persist-credentials input in actions/checkout (including version 4) is true [1][2][3]. When persist-credentials is set to true (the default), the action configures the Git authentication token or SSH key within the local Git configuration. This allows subsequent steps in your workflow to execute authenticated Git commands [2][4]. The action automatically removes these credentials during the post-job cleanup process [2][4]. If you wish to prevent the action from configuring these credentials, you can explicitly set persist-credentials: false in your workflow step [2][4]. Note: In later versions of actions/checkout (starting from v6), the mechanism for persisting these credentials was updated to store them in a separate file under $RUNNER_TEMP rather than directly within the local .git/config file, enhancing security [5][6][7]. The default behavior remains true [5][6].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4/README.md
- 3: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 4: https://www.github.com/actions/checkout
- 5: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 6: https://github.com/actions/checkout/tree/v6.0.0
- 7: Persist creds to a separate file actions/checkout#2286
Disable checkout credential persistence.
actions/checkout@v4 persists GitHub credentials by default, and this workflow runs repository-controlled build steps afterward. Set persist-credentials: false and add a minimal permissions block unless later Git-authenticated steps require more.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/on-pr-kind-smoke.yaml at line 18, Update the
actions/checkout@v4 step to set persist-credentials to false, and add a minimal
permissions block for the workflow unless subsequent Git-authenticated steps
require additional access.
Source: Linters/SAST tools
| /// Configuration YAML for gitops trees. | ||
| /// `local_source`: use Always pull policy so re-installs pick new dev tags. | ||
| fn build_configuration_gitops_yaml( | ||
| name: &str, | ||
| package_ref: &str, | ||
| skip_dependency_resolution: bool, | ||
| local_source: bool, | ||
| ) -> String { | ||
| let mode = if local_source { | ||
| "source build (local registry)" | ||
| } else { | ||
| "published package" | ||
| }; | ||
| let mut yaml = format!( | ||
| r#"# Generated by hops config install --gitops ({mode}) | ||
| apiVersion: pkg.crossplane.io/v1 | ||
| kind: Configuration | ||
| metadata: | ||
| name: {name} | ||
| spec: | ||
| package: {package_ref} | ||
| "# | ||
| ); | ||
| if local_source { | ||
| yaml.push_str(" packagePullPolicy: Always\n"); | ||
| } | ||
| if skip_dependency_resolution { | ||
| yaml.push_str(" skipDependencyResolution: true\n"); | ||
| } | ||
| yaml | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The gitops Configuration drops packagePullPolicy: Always for published packages.
build_configuration_yaml (Line 846) always emits packagePullPolicy: Always, and that manifest is applied live. build_configuration_gitops_yaml emits it only when local_source is true. Applying the generated tree with hops local gitops cluster therefore removes the field from the live Configuration and changes pull behavior. Keep the two builders in agreement, or derive the gitops YAML from build_configuration_yaml plus a header comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/config/install.rs` around lines 611 - 641, The
build_configuration_gitops_yaml function must always emit packagePullPolicy:
Always, matching build_configuration_yaml for both local and published packages.
Move the YAML field outside the local_source conditional while preserving the
existing skipDependencyResolution behavior and mode-specific header.
| pub fn configure_docker_insecure_registry() -> Result<(), Box<dyn Error>> { | ||
| let config = run_cmd_output("colima", &["ssh", "--", "cat", "/etc/docker/daemon.json"])?; | ||
|
|
||
| if config.contains("insecure-registries") { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| log::info!("Configuring Docker for insecure local registry..."); | ||
|
|
||
| // Insert the insecure-registries key before the final closing brace. | ||
| let new_config = if let Some(pos) = config.rfind('}') { | ||
| let prefix = config[..pos].trim_end(); | ||
| format!( | ||
| "{},\n \"insecure-registries\": [\"{}\"]\n}}\n", | ||
| prefix, REGISTRY_PULL | ||
| ) | ||
| } else { | ||
| return Err("Invalid daemon.json: no closing brace".into()); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge into an existing insecure-registries list instead of skipping.
Line 305 returns early when daemon.json contains the insecure-registries key. The key can already exist for a different registry. In that case REGISTRY_PULL is never added, Docker keeps using HTTPS for it, and later pushes and pulls fail with a TLS error that does not name this cause.
The splice at Line 312 also assumes a well-formed single-object file. serde_json is already a dependency in this file. Parse the document, merge the entry, and re-serialize.
🐛 Proposed fix using serde_json
- if config.contains("insecure-registries") {
- return Ok(());
- }
-
- log::info!("Configuring Docker for insecure local registry...");
-
- // Insert the insecure-registries key before the final closing brace.
- let new_config = if let Some(pos) = config.rfind('}') {
- let prefix = config[..pos].trim_end();
- format!(
- "{},\n \"insecure-registries\": [\"{}\"]\n}}\n",
- prefix, REGISTRY_PULL
- )
- } else {
- return Err("Invalid daemon.json: no closing brace".into());
- };
+ let mut doc: serde_json::Value = if config.trim().is_empty() {
+ serde_json::json!({})
+ } else {
+ serde_json::from_str(&config)
+ .map_err(|e| format!("Invalid daemon.json: {e}"))?
+ };
+ let obj = doc
+ .as_object_mut()
+ .ok_or("Invalid daemon.json: top level is not an object")?;
+ let entries = obj
+ .entry("insecure-registries")
+ .or_insert_with(|| serde_json::Value::Array(Vec::new()))
+ .as_array_mut()
+ .ok_or("Invalid daemon.json: insecure-registries is not an array")?;
+ if entries.iter().any(|v| v.as_str() == Some(REGISTRY_PULL)) {
+ return Ok(());
+ }
+ entries.push(serde_json::Value::String(REGISTRY_PULL.to_string()));
+
+ log::info!("Configuring Docker for insecure local registry...");
+ let new_config = format!("{}\n", serde_json::to_string_pretty(&doc)?);📝 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.
| pub fn configure_docker_insecure_registry() -> Result<(), Box<dyn Error>> { | |
| let config = run_cmd_output("colima", &["ssh", "--", "cat", "/etc/docker/daemon.json"])?; | |
| if config.contains("insecure-registries") { | |
| return Ok(()); | |
| } | |
| log::info!("Configuring Docker for insecure local registry..."); | |
| // Insert the insecure-registries key before the final closing brace. | |
| let new_config = if let Some(pos) = config.rfind('}') { | |
| let prefix = config[..pos].trim_end(); | |
| format!( | |
| "{},\n \"insecure-registries\": [\"{}\"]\n}}\n", | |
| prefix, REGISTRY_PULL | |
| ) | |
| } else { | |
| return Err("Invalid daemon.json: no closing brace".into()); | |
| }; | |
| pub fn configure_docker_insecure_registry() -> Result<(), Box<dyn Error>> { | |
| let config = run_cmd_output("colima", &["ssh", "--", "cat", "/etc/docker/daemon.json"])?; | |
| let mut doc: serde_json::Value = if config.trim().is_empty() { | |
| serde_json::json!({}) | |
| } else { | |
| serde_json::from_str(&config) | |
| .map_err(|e| format!("Invalid daemon.json: {e}"))? | |
| }; | |
| let obj = doc | |
| .as_object_mut() | |
| .ok_or("Invalid daemon.json: top level is not an object")?; | |
| let entries = obj | |
| .entry("insecure-registries") | |
| .or_insert_with(|| serde_json::Value::Array(Vec::new())) | |
| .as_array_mut() | |
| .ok_or("Invalid daemon.json: insecure-registries is not an array")?; | |
| if entries.iter().any(|v| v.as_str() == Some(REGISTRY_PULL)) { | |
| return Ok(()); | |
| } | |
| entries.push(serde_json::Value::String(REGISTRY_PULL.to_string())); | |
| log::info!("Configuring Docker for insecure local registry..."); | |
| let new_config = format!("{}\n", serde_json::to_string_pretty(&doc)?); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/backend/colima.rs` around lines 302 - 320, Update
configure_docker_insecure_registry to parse daemon.json with serde_json instead
of checking the key and splicing text. Ensure the root object contains an
insecure-registries array, merge REGISTRY_PULL without duplicating existing
entries, and serialize the updated document before applying it; return an error
for invalid or incompatible JSON.
| let path = home()?.join(".kube/dory-config"); | ||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| std::fs::write(&path, &cfg)?; | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create the kubeconfig files with restrictive permissions from the start.
std::fs::write creates ~/.kube/dory-config with the process umask, normally 0644. The file contains client-key-data. The set_permissions call runs after the content is on disk, so other local users can read the admin key during that window. The same pattern applies to the merge temp file at Line 639.
Create the file with mode 0600 before writing the content.
🔒 Proposed fix
- std::fs::write(&path, &cfg)?;
- #[cfg(unix)]
- {
- use std::os::unix::fs::PermissionsExt;
- let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600));
- }
+ write_private(&path, cfg.as_bytes())?;Add a shared helper:
fn write_private(path: &std::path::Path, bytes: &[u8]) -> Result<(), Box<dyn Error>> {
use std::io::Write;
let mut opts = std::fs::OpenOptions::new();
opts.write(true).create(true).truncate(true);
#[cfg(unix)]
{
use std::os::unix::fs::OpenOptionsExt;
opts.mode(0o600);
}
opts.open(path)?.write_all(bytes)?;
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/backend/dory.rs` around lines 566 - 575, Replace the
direct std::fs::write and post-write set_permissions pattern in the
kubeconfig-writing flow with a shared private-file helper that opens or creates
files with mode 0600 before writing. Use this helper for both the dory-config
output and the merge temporary file, preserving truncation and complete byte
writes while retaining non-Unix compatibility.
| if let Ok(raw) = fs::read_to_string(&pid_path) { | ||
| if let Ok(pid) = raw.trim().parse::<u32>() { | ||
| if pid_is_alive(pid) && localhost_port_listening(REGISTRY_HOST_PORT) { | ||
| return Ok(()); | ||
| } | ||
| // Stale PID or dead listener — stop and restart. | ||
| let _ = std::process::Command::new("kill") | ||
| .arg(pid.to_string()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .status(); | ||
| let _ = fs::remove_file(&pid_path); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Verify the PID owner before sending kill.
The recovery branch runs when the recorded PID is alive but port 30500 is not listening. Operating systems reuse PIDs. After a reboot or after the recorded kubectl process exits, the PID can belong to an unrelated process of the same user. The code then terminates that process.
Confirm that the PID is a kubectl port-forward process before killing it.
🛡️ Proposed fix
if pid_is_alive(pid) && localhost_port_listening(REGISTRY_HOST_PORT) {
return Ok(());
}
- // Stale PID or dead listener — stop and restart.
- let _ = std::process::Command::new("kill")
- .arg(pid.to_string())
- .stdout(Stdio::null())
- .stderr(Stdio::null())
- .status();
+ // Stale PID or dead listener — stop and restart, but only when the
+ // PID still belongs to our port-forward (PIDs get reused).
+ if pid_is_registry_port_forward(pid) {
+ let _ = std::process::Command::new("kill")
+ .arg(pid.to_string())
+ .stdout(Stdio::null())
+ .stderr(Stdio::null())
+ .status();
+ }
let _ = fs::remove_file(&pid_path);Add the ownership check:
#[cfg(unix)]
fn pid_is_registry_port_forward(pid: u32) -> bool {
std::process::Command::new("ps")
.args(["-p", &pid.to_string(), "-o", "args="])
.output()
.map(|o| {
let cmd = String::from_utf8_lossy(&o.stdout);
cmd.contains("port-forward")
&& cmd.contains(&format!("svc/{REGISTRY_SVC}"))
})
.unwrap_or(false)
}
#[cfg(not(unix))]
fn pid_is_registry_port_forward(_pid: u32) -> bool {
false
}📝 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.
| if let Ok(raw) = fs::read_to_string(&pid_path) { | |
| if let Ok(pid) = raw.trim().parse::<u32>() { | |
| if pid_is_alive(pid) && localhost_port_listening(REGISTRY_HOST_PORT) { | |
| return Ok(()); | |
| } | |
| // Stale PID or dead listener — stop and restart. | |
| let _ = std::process::Command::new("kill") | |
| .arg(pid.to_string()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .status(); | |
| let _ = fs::remove_file(&pid_path); | |
| } | |
| } | |
| if let Ok(raw) = fs::read_to_string(&pid_path) { | |
| if let Ok(pid) = raw.trim().parse::<u32>() { | |
| if pid_is_alive(pid) && localhost_port_listening(REGISTRY_HOST_PORT) { | |
| return Ok(()); | |
| } | |
| // Stale PID or dead listener — stop and restart, but only when the | |
| // PID still belongs to our port-forward (PIDs get reused). | |
| if pid_is_registry_port_forward(pid) { | |
| let _ = std::process::Command::new("kill") | |
| .arg(pid.to_string()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .status(); | |
| } | |
| let _ = fs::remove_file(&pid_path); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/package_install.rs` around lines 366 - 379, Verify
ownership of the recorded PID before terminating it in the recovery branch
around pid_is_alive and localhost_port_listening. Add a platform-appropriate
pid_is_registry_port_forward check that confirms the process is kubectl
port-forward for svc/{REGISTRY_SVC}; only invoke kill and remove the PID file
when that check succeeds, while preserving safe behavior on unsupported
platforms.
| pub fn ensure_host_access( | ||
| namespace: &str, | ||
| services: &[ServiceEndpoint], | ||
| port_base: u16, | ||
| state_dir: &Path, | ||
| workspace: &str, | ||
| ) -> Result<(HostAccessPlan, HostAccessRuntime, bool), Box<dyn Error>> { | ||
| let prior = load_host_access_runtime(state_dir, workspace)?; | ||
|
|
||
| if let Some(rt) = &prior { | ||
| if !host_access_needs_heal(rt) { | ||
| return Ok((plan_from_runtime(rt), rt.clone(), false)); | ||
| } | ||
| log::info!("host access unhealthy; restarting ({})", rt.mode); | ||
| } | ||
|
|
||
| // Rebuild services list from runtime maps if discovery is empty. | ||
| let services = if services.is_empty() { | ||
| prior | ||
| .as_ref() | ||
| .map(services_from_runtime) | ||
| .unwrap_or_else(|| services.to_vec()) | ||
| } else { | ||
| services.to_vec() | ||
| }; | ||
|
|
||
| // Always prefer dns on restart; auto falls back to map if sudo/hosts fails. | ||
| let (plan, rt) = | ||
| start_host_access_auto(namespace, &services, false, port_base, state_dir, workspace)?; | ||
| std::thread::sleep(Duration::from_millis(400)); | ||
| Ok((plan, rt, true)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Honor the recorded mode when healing host access.
ensure_host_access always calls start_host_access_auto, which tries DNS first. DNS setup requires an admin elevation through apply_privileged_dns_config. If the user previously declined elevation and the workspace fell back to map mode, prior.mode is "map", but every subsequent hops local status or hops local up prompts for admin again before falling back.
Pass the recorded mode so a workspace that resolved to map mode stays in map mode until the user asks for DNS explicitly.
🐛 Proposed fix
- // Always prefer dns on restart; auto falls back to map if sudo/hosts fails.
- let (plan, rt) =
- start_host_access_auto(namespace, &services, false, port_base, state_dir, workspace)?;
+ // Reuse the mode that previously succeeded; only try dns when it was chosen before.
+ let prior_mode = prior
+ .as_ref()
+ .and_then(|rt| HostAccessMode::parse(&rt.mode));
+ let (plan, rt) = if matches!(prior_mode, Some(HostAccessMode::Map)) {
+ let plan = plan_host_access_mode(
+ namespace,
+ &services,
+ HostAccessMode::Map,
+ port_base,
+ &BTreeMap::new(),
+ );
+ let rt = start_host_access_with_services(&plan, &services, state_dir, workspace)?;
+ (plan, rt)
+ } else {
+ start_host_access_auto(namespace, &services, false, port_base, state_dir, workspace)?
+ };📝 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.
| pub fn ensure_host_access( | |
| namespace: &str, | |
| services: &[ServiceEndpoint], | |
| port_base: u16, | |
| state_dir: &Path, | |
| workspace: &str, | |
| ) -> Result<(HostAccessPlan, HostAccessRuntime, bool), Box<dyn Error>> { | |
| let prior = load_host_access_runtime(state_dir, workspace)?; | |
| if let Some(rt) = &prior { | |
| if !host_access_needs_heal(rt) { | |
| return Ok((plan_from_runtime(rt), rt.clone(), false)); | |
| } | |
| log::info!("host access unhealthy; restarting ({})", rt.mode); | |
| } | |
| // Rebuild services list from runtime maps if discovery is empty. | |
| let services = if services.is_empty() { | |
| prior | |
| .as_ref() | |
| .map(services_from_runtime) | |
| .unwrap_or_else(|| services.to_vec()) | |
| } else { | |
| services.to_vec() | |
| }; | |
| // Always prefer dns on restart; auto falls back to map if sudo/hosts fails. | |
| let (plan, rt) = | |
| start_host_access_auto(namespace, &services, false, port_base, state_dir, workspace)?; | |
| std::thread::sleep(Duration::from_millis(400)); | |
| Ok((plan, rt, true)) | |
| } | |
| pub fn ensure_host_access( | |
| namespace: &str, | |
| services: &[ServiceEndpoint], | |
| port_base: u16, | |
| state_dir: &Path, | |
| workspace: &str, | |
| ) -> Result<(HostAccessPlan, HostAccessRuntime, bool), Box<dyn Error>> { | |
| let prior = load_host_access_runtime(state_dir, workspace)?; | |
| if let Some(rt) = &prior { | |
| if !host_access_needs_heal(rt) { | |
| return Ok((plan_from_runtime(rt), rt.clone(), false)); | |
| } | |
| log::info!("host access unhealthy; restarting ({})", rt.mode); | |
| } | |
| // Rebuild services list from runtime maps if discovery is empty. | |
| let services = if services.is_empty() { | |
| prior | |
| .as_ref() | |
| .map(services_from_runtime) | |
| .unwrap_or_else(|| services.to_vec()) | |
| } else { | |
| services.to_vec() | |
| }; | |
| // Reuse the mode that previously succeeded; only try dns when it was chosen before. | |
| let prior_mode = prior | |
| .as_ref() | |
| .and_then(|rt| HostAccessMode::parse(&rt.mode)); | |
| let (plan, rt) = if matches!(prior_mode, Some(HostAccessMode::Map)) { | |
| let plan = plan_host_access_mode( | |
| namespace, | |
| &services, | |
| HostAccessMode::Map, | |
| port_base, | |
| &BTreeMap::new(), | |
| ); | |
| let rt = start_host_access_with_services(&plan, &services, state_dir, workspace)?; | |
| (plan, rt) | |
| } else { | |
| start_host_access_auto(namespace, &services, false, port_base, state_dir, workspace)? | |
| }; | |
| std::thread::sleep(Duration::from_millis(400)); | |
| Ok((plan, rt, true)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 566 - 597, Update
ensure_host_access to preserve the previously recorded access mode when healing
an unhealthy runtime: use prior.mode to select map mode instead of always
invoking DNS-first start_host_access_auto. Keep DNS behavior for runtimes
recorded as DNS or when no prior runtime exists, while retaining the existing
service reconstruction and restart flow.
| let values_path = std::env::temp_dir().join(format!( | ||
| "hops-lwb-values-{}-{}.yaml", | ||
| std::process::id(), | ||
| release | ||
| )); | ||
| std::fs::write(&values_path, values_yaml)?; | ||
| let output = Command::new("helm") | ||
| .args([ | ||
| "template", | ||
| release, | ||
| &chart_path.to_string_lossy(), | ||
| "--namespace", | ||
| namespace, | ||
| "--values", | ||
| &values_path.to_string_lossy(), | ||
| ]) | ||
| .output()?; | ||
| let _ = std::fs::remove_file(&values_path); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create the Helm values file with a non-predictable name and exclusive creation.
The path hops-lwb-values-<pid>-<release>.yaml in the shared temp directory is fully predictable. std::fs::write follows symlinks, so another local user can pre-create that path as a symlink and redirect the write. The file also holds merged chart values, which may contain sensitive configuration, and is created with the default umask.
Two additional points: the ? at line 91 returns before line 92, so the temp file leaks when helm fails to spawn. release is not sanitized inside this trait method, only at the reconcile_one call site, so a path separator in the release name escapes the temp directory.
Use tempfile::NamedTempFile (or OpenOptions::new().create_new(true).mode(0o600)) in a per-invocation directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/reconcile.rs` around lines 75 - 92, Update the
temporary values-file handling around the reconcile Helm invocation to use
tempfile::NamedTempFile (or equivalent exclusive 0600 creation) in a
per-invocation temporary directory, rather than constructing a release-based
path with std::env::temp_dir and std::fs::write. Pass the generated file path to
the existing Command::new("helm") arguments, and ensure cleanup is RAII-based so
failures from output() cannot leak the file; do not derive the path from
unsanitized release input.
Source: Linters/SAST tools
| fn ensure_namespace( | ||
| &self, | ||
| namespace: &str, | ||
| labels: &BTreeMap<String, String>, | ||
| ) -> Result<(), Box<dyn Error>> { | ||
| let mut label_lines = String::new(); | ||
| for (k, v) in labels { | ||
| label_lines.push_str(&format!(" {k}: {v}\n")); | ||
| } | ||
| let yaml = format!( | ||
| "apiVersion: v1\nkind: Namespace\nmetadata:\n name: {namespace}\n labels:\n{label_lines}" | ||
| ); | ||
| crate::commands::local::kubectl_apply_stdin(&yaml) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize the Namespace manifest instead of interpolating label strings.
ensure_namespace builds YAML with format!. The label values include opts.workspace_name, which comes from --name or the cwd basename in src/commands/local/up.rs. A name that contains \n, :, #, or a leading { produces an invalid manifest or injects unintended fields into the Namespace object. Build a serde_yaml::Value and serialize it.
🛡️ Proposed fix
- let mut label_lines = String::new();
- for (k, v) in labels {
- label_lines.push_str(&format!(" {k}: {v}\n"));
- }
- let yaml = format!(
- "apiVersion: v1\nkind: Namespace\nmetadata:\n name: {namespace}\n labels:\n{label_lines}"
- );
- crate::commands::local::kubectl_apply_stdin(&yaml)
+ let mut label_map = serde_yaml::Mapping::new();
+ for (k, v) in labels {
+ label_map.insert(Value::String(k.clone()), Value::String(v.clone()));
+ }
+ let mut metadata = serde_yaml::Mapping::new();
+ metadata.insert(Value::String("name".into()), Value::String(namespace.to_string()));
+ metadata.insert(Value::String("labels".into()), Value::Mapping(label_map));
+ let mut root = serde_yaml::Mapping::new();
+ root.insert(Value::String("apiVersion".into()), Value::String("v1".into()));
+ root.insert(Value::String("kind".into()), Value::String("Namespace".into()));
+ root.insert(Value::String("metadata".into()), Value::Mapping(metadata));
+ crate::commands::local::kubectl_apply_stdin(&serde_yaml::to_string(&Value::Mapping(root))?)📝 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.
| fn ensure_namespace( | |
| &self, | |
| namespace: &str, | |
| labels: &BTreeMap<String, String>, | |
| ) -> Result<(), Box<dyn Error>> { | |
| let mut label_lines = String::new(); | |
| for (k, v) in labels { | |
| label_lines.push_str(&format!(" {k}: {v}\n")); | |
| } | |
| let yaml = format!( | |
| "apiVersion: v1\nkind: Namespace\nmetadata:\n name: {namespace}\n labels:\n{label_lines}" | |
| ); | |
| crate::commands::local::kubectl_apply_stdin(&yaml) | |
| } | |
| fn ensure_namespace( | |
| &self, | |
| namespace: &str, | |
| labels: &BTreeMap<String, String>, | |
| ) -> Result<(), Box<dyn Error>> { | |
| let mut label_map = serde_yaml::Mapping::new(); | |
| for (k, v) in labels { | |
| label_map.insert(Value::String(k.clone()), Value::String(v.clone())); | |
| } | |
| let mut metadata = serde_yaml::Mapping::new(); | |
| metadata.insert(Value::String("name".into()), Value::String(namespace.to_string())); | |
| metadata.insert(Value::String("labels".into()), Value::Mapping(label_map)); | |
| let mut root = serde_yaml::Mapping::new(); | |
| root.insert(Value::String("apiVersion".into()), Value::String("v1".into())); | |
| root.insert(Value::String("kind".into()), Value::String("Namespace".into())); | |
| root.insert(Value::String("metadata".into()), Value::Mapping(metadata)); | |
| crate::commands::local::kubectl_apply_stdin(&serde_yaml::to_string(&Value::Mapping(root))?) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/reconcile.rs` around lines 110 - 123, Update
ensure_namespace to construct the Namespace manifest as a serde_yaml::Value
(including apiVersion, kind, metadata.name, and labels) and serialize it with
serde_yaml rather than interpolating namespace or label strings into YAML.
Preserve the existing labels and kubectl_apply_stdin flow while ensuring special
characters in values remain data and cannot alter the manifest structure.
| fn split_yaml_docs_owned(s: &str) -> Vec<String> { | ||
| let mut parts = Vec::new(); | ||
| let mut buf = String::new(); | ||
| for line in s.lines() { | ||
| if line.trim() == "---" { | ||
| if !buf.trim().is_empty() { | ||
| parts.push(std::mem::take(&mut buf)); | ||
| } | ||
| } else { | ||
| buf.push_str(line); | ||
| buf.push('\n'); | ||
| } | ||
| } | ||
| if !buf.trim().is_empty() { | ||
| parts.push(buf); | ||
| } | ||
| parts | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any chart in the repo embeds `---` inside a block scalar,
# which the line-based splitter would corrupt.
set -euo pipefail
fd -e yaml -e yml --full-path 'templates' | while IFS= read -r f; do
rg -nU '(\||>)[-+0-9]*\s*\n(\s+.*\n)*?\s+---\s*$' "$f" && echo "^^ $f"
doneRepository: hops-ops/hops-cli
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate reconcile.rs =="
fd -a 'reconcile\.rs$' .
echo "== outline =="
ast-grep outline 'src/commands/local/workbench/reconcile.rs' --view expanded || true
echo "== relevant section =="
sed -n '140,230p' 'src/commands/local/workbench/reconcile.rs'
echo "== occurrences of split_yaml_docs_owned and render_labels_into_manifests =="
rg -n "split_yaml_docs_owned|render_labels_into_manifests|ensure_namespace_on_docs|parse rendered manifest|parse rendered" 'src/commands/local/workbench/reconcile.rs'Repository: hops-ops/hops-cli
Length of output: 6173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== render_labels_into_manifests and ensure_namespace_on_docs =="
sed -n '260,475p' 'src/commands/local/workbench/reconcile.rs'
echo "== tests around split behavior =="
sed -n '522,560p' 'src/commands/local/workbench/reconcile.rs'
echo "== check serde_yaml deserializer behavior via local docs/source if vendored? =="
rg -n "pub fn from_str|Deserializer::from_str|multi-document|from_reader" -S . | head -80Repository: hops-ops/hops-cli
Length of output: 8505
🌐 Web query:
serde_yaml Deserializer from_str multi-document YAML block scalar separator
💡 Result:
To deserialize a multi-document YAML stream using the serde_yaml crate, you must use the serde_yaml::Deserializer::from_str (or from_slice) method, which returns an iterator over the documents in the stream [1][2][3]. Standard top-level functions like serde_yaml::from_str, serde_yaml::from_reader, and serde_yaml::from_slice are designed to handle exactly one YAML document [4][5][6]. If they encounter a stream containing multiple documents, they will return a MoreThanOneDocument error [3][7]. To correctly process multi-document YAML, use the following pattern: use serde::Deserialize; use serde_yaml::{Deserializer, Value}; fn main { let input = "---\nk: 107\n...\n---\nj: 106\n"; // Create an iterator over the YAML documents for document in Deserializer::from_str(input) { // Deserialize each document individually let value = Value::deserialize(document).expect("failed to deserialize document"); println!("{:?}", value); } } Regarding block scalar separators in multi-document streams: serde_yaml relies on the underlying parser's ability to handle standard YAML block scalars (like | or >) and document separators (--- and...). When you iterate through documents using the Deserializer, the block scalars within each document are parsed according to standard YAML rules without requiring special handling for the separators [1][2]. The separation between documents is handled by the document boundaries (--- or...), which indicate the start or end of a YAML document, allowing the iterator to identify each discrete document in the stream [1][3].
Citations:
- 1: https://docs.rs/serde_yaml/latest/serde_yaml/struct.Deserializer.html
- 2: https://github.com/dtolnay/serde-yaml/blob/2009506d/src/de.rs
- 3: https://github.com/dtolnay/serde-yaml/blob/7a340b295b01e6e80955e9ae3a831882d29e8967/src/de.rs
- 4: https://docs.rs/serde_yaml/latest/src/serde_yaml/lib.rs.html
- 5: https://docs.rs/serde_yaml/latest/serde_yaml/
- 6: https://docs.rs/serde_yaml/latest/serde_yaml/fn.from_reader.html
- 7: https://stackoverflow.com/questions/72207805/deserializing-multiple-documents-with-serde-yaml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find cargo/serde_yaml crate source =="
fd -a 'de\.rs$|lib\.rs$' . | rg 'target/.*serde_yaml|/serde_yaml/' || true
if [ -d target/registry/src/serde_yaml-* ]; then
echo "== serde_yaml source found =="
ls target/registry/src/serde_yaml-*
else
echo "== no target/registry/src/serde_yaml-* =="
fi
echo "== behavior probe without repo code =="
python3 - <<'PY'
from pathlib import Path
root = next(Path("target").glob("registry/src/serde_yaml-*/serde-yaml-*"), None)
# If available, print relevant parser slices rather than running repo code
print("serde_yaml source root:", root)
if root:
for path in [root / "serde-yaml-*/src/de.rs", root / "serde-yaml-*/src/error.rs"]:
files = list(Path(".").glob(str(path.relative_to("."))))
print("candidate files:", [str(f) for f in files])
PYRepository: hops-ops/hops-cli
Length of output: 306
Use the YAML parser for multi-document splitting.
split_yaml_docs_owned treats every trimmed --- line as a record separator while iterating over Helm output. If a Chart renders YAML inside a literal/folded scalar, that embedded --- becomes an invalid fragment. This can make render_labels_into_manifests fail with parse rendered manifest or send corrupted YAML through ensure_namespace_on_docs. Build these paths on serde_yaml::Deserializer::from_str and re-serialize parsed Values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/reconcile.rs` around lines 197 - 214, Replace
the line-based splitting in split_yaml_docs_owned with
serde_yaml::Deserializer::from_str, parsing each YAML document into
serde_yaml::Value and re-serializing the values into document strings. Update
render_labels_into_manifests and ensure_namespace_on_docs to consume these
parser-derived documents so --- inside literal or folded scalars remains part of
its scalar content, while preserving existing document-processing behavior and
parse-error propagation.
| fn ensure_namespace_on_docs(yaml: &str, namespace: &str) -> Result<String, Box<dyn Error>> { | ||
| let mut out = Vec::new(); | ||
| for doc in split_yaml_docs_owned(yaml) { | ||
| if doc.trim().is_empty() { | ||
| continue; | ||
| } | ||
| let mut value: Value = serde_yaml::from_str(&doc)?; | ||
| if let Some(root) = value.as_mapping_mut() { | ||
| let kind = root | ||
| .get(Value::String("kind".into())) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or(""); | ||
| let cluster_scoped = matches!( | ||
| kind, | ||
| "Namespace" | "ClusterRole" | "ClusterRoleBinding" | "CustomResourceDefinition" | ||
| ); | ||
| if !cluster_scoped { | ||
| let meta_key = Value::String("metadata".into()); | ||
| if let Some(meta) = root.get_mut(&meta_key).and_then(|v| v.as_mapping_mut()) { | ||
| meta.insert( | ||
| Value::String("namespace".into()), | ||
| Value::String(namespace.to_string()), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| out.push(serde_yaml::to_string(&value)?); | ||
| } | ||
| Ok(out.join("---\n")) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Extend or invert the cluster-scoped kind check.
ensure_namespace_on_docs stamps metadata.namespace on every kind that is not one of four names. Charts commonly ship other cluster-scoped kinds, for example PersistentVolume, StorageClass, ValidatingWebhookConfiguration, MutatingWebhookConfiguration, APIService, PriorityClass, IngressClass, and Crossplane XRs. kubectl apply rejects a namespace field on those objects, so the whole document fails.
A safer approach is to stop stamping the namespace and pass --namespace <ns> to kubectl apply instead. helm template --namespace already renders the intended namespace, and kubectl applies the flag only to namespaced resources. If stamping must stay, extend the list to cover the kinds above.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/reconcile.rs` around lines 447 - 476, Update
ensure_namespace_on_docs to avoid injecting metadata.namespace based on an
incomplete cluster-scoped kind allowlist, preferably by removing this stamping
and ensuring the kubectl apply invocation receives --namespace <ns> instead.
Preserve helm template’s namespace behavior and let kubectl apply determine
whether each resource is namespaced; if stamping remains, expand the
cluster-scoped handling to cover all supported cluster-scoped kinds, including
the examples in the review.
Collapse host access to one path: workspace Services and related in-cluster FQDNs via loopback IPs, /etc/hosts, macOS stub DNS, and a port-forward supervisor. Prefer dory-config credentials first, and plumb HOPS_KUBE_CONTEXT into long-lived kubectl children.
|
Pushed DNS-only host access + dory kubeconfig fix ( |
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 (2)
src/commands/local/workbench/net.rs (2)
162-175: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA corrupt runtime file blocks every workspace command.
load_host_access_runtimepropagates the deserialization error.stop_host_access_processes_only,stop_host_access, andensure_host_accessall call it with?. One truncated or hand-edited<workspace>.host-access.jsontherefore breakshops local status,hops local down, and the restart path, and the user has no in-tool way to recover.collect_dns_blocks_from_runtimesalready tolerates the same failure.Treat a parse failure as "no runtime", and log a warning.
🛡️ Proposed fix
let path = runtime_path(state_dir, workspace); if !path.exists() { return Ok(None); } - Ok(Some(serde_json::from_str(&fs::read_to_string(path)?)?)) + let text = fs::read_to_string(&path)?; + match serde_json::from_str(&text) { + Ok(rt) => Ok(Some(rt)), + Err(e) => { + log::warn!("ignoring unreadable host access runtime {}: {e}", path.display()); + Ok(None) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 162 - 175, Update load_host_access_runtime to distinguish file-read failures from JSON deserialization failures: preserve propagation of read errors, but treat serde_json parsing errors as no runtime by logging a warning and returning Ok(None). Keep the existing missing-file behavior and ensure callers such as stop_host_access_processes_only, stop_host_access, and ensure_host_access can continue normally when the runtime file is corrupt.
806-846: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOne not-ready endpoint restarts the whole supervisor on every
status.
host_access_needs_healreturns true when any single endpoint is not listening.ensure_host_accessthen callsstart_host_access, which kills the supervisor and all port-forwards, re-applies the DNS configuration, and starts everything again.src/commands/local/status.rscallsensure_host_accessby default. During bring-up, one pod that is not ready yet therefore tears down the working forwards of every other service each time the user runshops local status, and prints "host access restarted (self-heal)".The supervisor already restarts individual forwards every two seconds. Restart it only when the supervisor process is dead or the endpoint set changed.
🐛 Proposed fix
pub fn host_access_needs_heal(rt: &HostAccessRuntime) -> bool { if rt.ip_map.is_empty() { return true; } if !rt.pids.iter().any(|p| pid_is_alive(*p)) { return true; } - for (key, ip) in &rt.ip_map { - let port = rt.service_ports.get(key).copied().unwrap_or(80); - if !ip_port_listening(ip, port) { - return true; - } - } + // Individual endpoints are restarted by the supervisor loop; a pod that is + // not ready yet must not trigger a full restart. false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 806 - 846, Update host_access_needs_heal and ensure_host_access so an individual non-listening endpoint does not trigger supervisor restart; only heal when the supervisor process is no longer alive or the configured endpoint set has changed. Preserve the existing reuse path and per-forward recovery handled by the supervisor, while still restarting when the endpoint configuration differs.
🧹 Nitpick comments (1)
src/commands/local/workbench/net.rs (1)
616-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
stub_dns_respondsalways returns true.The function makes the health check at line 478 equivalent to
pid_is_alive(pid). A hung stub DNS is treated as healthy and is never restarted. Either send one UDP query for a known zone entry and check for a response, or remove the function and callpid_is_alivedirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/local/workbench/net.rs` around lines 616 - 619, Update stub_dns_responds so it performs an actual UDP DNS query for a known zone entry and returns whether a response is received, or remove the helper and use pid_is_alive directly in the health check. Ensure hung stub DNS processes are not reported as healthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/claude/references/secrets.md`:
- Line 6: Remove the unsupported Vault workflows from the secrets documentation:
delete the Vault target references, vault configuration layout, and
synchronization instructions, including mentions of secret-path and
port-forward. Keep the documented AWS and GitHub sync workflows intact until a
Vault implementation is added to SyncTarget and its dispatch path.
In `@src/commands/local/workbench/cluster_dns.rs`:
- Around line 264-272: The loopback alias checks use substring matching against
raw ifconfig output, allowing prefix collisions. Add one shared helper to parse
the inet fields from ifconfig lo0 into whole-address values, then update
dns_os_config_present in src/commands/local/workbench/cluster_dns.rs:264-272 and
verify_loopback_aliases_ready in src/commands/local/workbench/net.rs:625-634 to
compare against that parsed address set; both sites require this change.
In `@src/commands/local/workbench/net.rs`:
- Around line 442-460: The teardown path must reverse both DNS artifacts created
during setup. In src/commands/local/workbench/net.rs:442-460, update
stop_host_access to call write_zone_file with the remaining blocks so
dns-zone.tsv stays synchronized with /etc/hosts; in
src/commands/local/workbench/cluster_dns.rs:338-346, when the last workspace
stops, remove /etc/resolver/svc.cluster.local and stop the stub DNS process, or
ensure the stub DNS is independently kept running.
- Around line 396-409: Serialize DNS state updates with one exclusive lock under
state_dir, covering collect_dns_blocks_from_runtimes, the /etc/hosts
read/merge/write sequence, and apply_privileged_dns_config. Reuse that same lock
in sync_alloc_for_namespace so allocation-file updates cannot race with worktree
DNS synchronization.
- Around line 920-933: Update the PID sweep around the piddir read_dir loop to
remove each processed PID file after attempting the TERM command, then remove
the piddir directory after the sweep completes. Preserve the existing behavior
for unreadable files and invalid PID contents.
- Around line 593-614: Update ensure_macos_stub_dns and its caller
start_host_access so failure to spawn the optional macOS stub DNS logs a warning
and continues successfully instead of propagating the error. Preserve existing
setup and error handling for required operations, while treating only the stub
DNS startup failure as non-fatal.
- Around line 702-718: Update the supervisor loop around the backgrounded
port-forward started by `k port-forward` to reap exited child processes before
checking whether their recorded PID is alive. Reuse the existing `pid_is_alive`
behavior that treats zombie state as dead, and ensure completed children are
waited on so stale zombie PIDs cannot make the `kill -0` check continue
indefinitely.
- Around line 330-349: Update the host-label parsing in
discover_workspace_endpoints so it reads the first label as name and the second
label as namespace, rather than popping the final label and joining the
remainder. Preserve the existing port extraction and endpoint insertion
behavior, including rejecting hosts without two non-empty labels.
---
Outside diff comments:
In `@src/commands/local/workbench/net.rs`:
- Around line 162-175: Update load_host_access_runtime to distinguish file-read
failures from JSON deserialization failures: preserve propagation of read
errors, but treat serde_json parsing errors as no runtime by logging a warning
and returning Ok(None). Keep the existing missing-file behavior and ensure
callers such as stop_host_access_processes_only, stop_host_access, and
ensure_host_access can continue normally when the runtime file is corrupt.
- Around line 806-846: Update host_access_needs_heal and ensure_host_access so
an individual non-listening endpoint does not trigger supervisor restart; only
heal when the supervisor process is no longer alive or the configured endpoint
set has changed. Preserve the existing reuse path and per-forward recovery
handled by the supervisor, while still restarting when the endpoint
configuration differs.
---
Nitpick comments:
In `@src/commands/local/workbench/net.rs`:
- Around line 616-619: Update stub_dns_responds so it performs an actual UDP DNS
query for a known zone entry and returns whether a response is received, or
remove the helper and use pid_is_alive directly in the health check. Ensure hung
stub DNS processes are not reported as healthy.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0888702-787c-4d38-a295-04969fd8f3ab
📒 Files selected for processing (15)
bootstrap/providers/helm-drc.yamlbootstrap/providers/helm.yamlbootstrap/providers/kubernetes-drc.yamlbootstrap/providers/kubernetes.yamlskills/claude/references/secrets.mdsrc/commands/local/backend/dory.rssrc/commands/local/backend/mod.rssrc/commands/local/open.rssrc/commands/local/package_install.rssrc/commands/local/start.rssrc/commands/local/status.rssrc/commands/local/up.rssrc/commands/local/workbench/cluster_dns.rssrc/commands/local/workbench/net.rssrc/commands/local/workbench/registry.rs
💤 Files with no reviewable changes (1)
- src/commands/local/workbench/registry.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/commands/local/open.rs
- src/commands/local/status.rs
- src/commands/local/backend/mod.rs
- src/commands/local/up.rs
- src/commands/local/package_install.rs
|
|
||
| `hops secrets` manages repo-level secrets using SOPS for encryption and syncs | ||
| to AWS Secrets Manager or GitHub repository secrets. | ||
| to AWS Secrets Manager, GitHub repository secrets, or HashiCorp Vault (KV). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the Vault sync target or remove these documented workflows.
src/commands/secrets/sync.rs currently defines only SyncTarget::Aws and SyncTarget::Github at Lines [29-34], and dispatches only those targets at Lines [78-83]. Therefore, hops secrets sync vault, --secret-path, and --port-forward are not valid commands. The documented .hops.yaml vault block also has no consumer in the sync path.
Either add the Vault target and implementation, or remove the Vault layout, configuration, and synchronization sections until the CLI supports them.
Also applies to: 23-27, 49-60, 111-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/claude/references/secrets.md` at line 6, Remove the unsupported Vault
workflows from the secrets documentation: delete the Vault target references,
vault configuration layout, and synchronization instructions, including mentions
of secret-path and port-forward. Keep the documented AWS and GitHub sync
workflows intact until a Vault implementation is added to SyncTarget and its
dispatch path.
| let lo0 = Command::new("ifconfig") | ||
| .arg("lo0") | ||
| .output() | ||
| .map(|o| String::from_utf8_lossy(&o.stdout).into_owned()) | ||
| .unwrap_or_default(); | ||
| loopback_ips.iter().all(|ip| { | ||
| ip.is_empty() || ip == "127.0.0.1" || lo0.contains(ip.as_str()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Loopback alias checks compare an IP against raw ifconfig text. Both sites use contains, so a longer alias such as 127.53.0.25 satisfies a check for 127.53.0.2. The shared root cause is substring matching instead of address matching; extract one helper that parses the inet fields of ifconfig lo0 and compares whole addresses.
src/commands/local/workbench/cluster_dns.rs#L264-L272: indns_os_config_present, compareloopback_ipsagainst the parsed address set so a prefix collision cannot suppress the elevation prompt.src/commands/local/workbench/net.rs#L625-L634: inverify_loopback_aliases_ready, buildmissingfrom the same parsed address set so a missing alias is reported before the supervisor starts.
📍 Affects 2 files
src/commands/local/workbench/cluster_dns.rs#L264-L272(this comment)src/commands/local/workbench/net.rs#L625-L634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/cluster_dns.rs` around lines 264 - 272, The
loopback alias checks use substring matching against raw ifconfig output,
allowing prefix collisions. Add one shared helper to parse the inet fields from
ifconfig lo0 into whole-address values, then update dns_os_config_present in
src/commands/local/workbench/cluster_dns.rs:264-272 and
verify_loopback_aliases_ready in src/commands/local/workbench/net.rs:625-634 to
compare against that parsed address set; both sites require this change.
| let host = &s[start..end]; | ||
| // expect name.namespace | ||
| let mut parts: Vec<&str> = host.split('.').collect(); | ||
| if parts.len() >= 2 { | ||
| let ns = parts.pop().unwrap().to_string(); | ||
| let name = parts.join("."); | ||
| if !name.is_empty() && !ns.is_empty() { | ||
| let after = end + marker.len(); | ||
| let port = if s[after..].starts_with(':') { | ||
| let digits: String = s[after + 1..] | ||
| .chars() | ||
| .take_while(|c| c.is_ascii_digit()) | ||
| .collect(); | ||
| digits.parse().ok() | ||
| } else { | ||
| None | ||
| }; | ||
| out.push((ns, name, port)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Take the first two labels of the host, not the last label as namespace.
The parser pops the last label as the namespace and joins the remaining labels as the service name. A cluster DNS name has the form name.namespace.svc.cluster.local, so the name is a single label. For a host with three or more labels, such as the stateful-set form pod-0.svc-name.ns.svc.cluster.local, this yields name = "pod-0.svc-name" and ns = "ns". discover_workspace_endpoints then adds an endpoint that no Service matches. The supervisor retries kubectl port-forward svc/pod-0.svc-name every two seconds, and the URL stays [down].
Read the first label as the name and the second as the namespace.
🐛 Proposed fix
- let mut parts: Vec<&str> = host.split('.').collect();
- if parts.len() >= 2 {
- let ns = parts.pop().unwrap().to_string();
- let name = parts.join(".");
- if !name.is_empty() && !ns.is_empty() {
+ let parts: Vec<&str> = host.split('.').collect();
+ if parts.len() == 2 {
+ let name = parts[0].to_string();
+ let ns = parts[1].to_string();
+ if !name.is_empty() && !ns.is_empty() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 330 - 349, Update the
host-label parsing in discover_workspace_endpoints so it reads the first label
as name and the second label as namespace, rather than popping the final label
and joining the remainder. Preserve the existing port extraction and endpoint
insertion behavior, including rejecting hosts without two non-empty labels.
| let mut blocks = collect_dns_blocks_from_runtimes(state_dir, Some(workspace))?; | ||
| blocks.extend(ns_blocks); | ||
| let mut merged_by_ns: BTreeMap<String, BTreeMap<String, String>> = BTreeMap::new(); | ||
| for (ns, m) in blocks { | ||
| merged_by_ns.entry(ns).or_default().extend(m); | ||
| } | ||
| let mut host_lines = Vec::new(); | ||
| for (ns, m) in &merged_by_ns { | ||
| host_lines.extend(cluster_dns::hosts_lines_for_workspace(ns, m)); | ||
| } | ||
| let current = fs::read_to_string("/etc/hosts").unwrap_or_default(); | ||
| let hosts_body = cluster_dns::merge_hosts_file(¤t, &host_lines); | ||
|
|
||
| cluster_dns::apply_privileged_dns_config(&hosts_body, &ips)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize the /etc/hosts read-merge-write across worktrees.
This code reads /etc/hosts, merges the managed block for all workspaces, and writes it back through apply_privileged_dns_config. The module documentation states that one local control plane serves many worktrees. Two concurrent hops local up runs both read the file before either writes it. The second write replaces the managed block with a version that omits the first run's entries. The first workspace keeps its loopback aliases and its port-forwards, but its FQDNs no longer resolve, and nothing detects the loss until the next heal cycle.
Hold one exclusive lock, for example a lock file under state_dir, across collect_dns_blocks_from_runtimes, the merge, and apply_privileged_dns_config. The same lock should cover sync_alloc_for_namespace, which has the same race on the allocation file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 396 - 409, Serialize DNS
state updates with one exclusive lock under state_dir, covering
collect_dns_blocks_from_runtimes, the /etc/hosts read/merge/write sequence, and
apply_privileged_dns_config. Reuse that same lock in sync_alloc_for_namespace so
allocation-file updates cannot race with worktree DNS synchronization.
| /// Write zone: fqdn → ip for the macOS stub DNS (and reloads on every start). | ||
| fn write_zone_file( | ||
| state_dir: &Path, | ||
| by_ns: &BTreeMap<String, BTreeMap<String, String>>, | ||
| ) -> Result<(), Box<dyn Error>> { | ||
| let path = state_dir.join(RUNTIME_SUBDIR).join("dns-zone.tsv"); | ||
| fs::create_dir_all(path.parent().unwrap())?; | ||
| let mut body = String::new(); | ||
| for (ns, m) in by_ns { | ||
| for (svc, ip) in m { | ||
| let fqdn = format!("{svc}.{ns}.svc.cluster.local"); | ||
| let twin = format!("{svc}.{ns}.svc.cluster"); | ||
| let short = format!("{svc}.{ns}"); | ||
| body.push_str(&format!("{fqdn}\t{ip}\n{twin}\t{ip}\n{short}\t{ip}\n")); | ||
| } | ||
| } | ||
| fs::write(path, body)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Teardown does not reverse two DNS artifacts. stop_host_access removes the loopback aliases and rebuilds /etc/hosts, but the setup path also writes a zone file and a macOS resolver file. Neither is reverted, so removed FQDNs keep resolving to loopback IPs with nothing listening, and the resolver keeps pointing at a stub DNS that may no longer run.
src/commands/local/workbench/net.rs#L442-L460: callwrite_zone_filefromstop_host_accesswith the blocks that remain, sodns-zone.tsvmatches/etc/hosts.src/commands/local/workbench/cluster_dns.rs#L338-L346: remove/etc/resolver/svc.cluster.localand stop the stub DNS process when the last workspace stops, or keep the stub DNS running independently of workspaces.
📍 Affects 2 files
src/commands/local/workbench/net.rs#L442-L460(this comment)src/commands/local/workbench/cluster_dns.rs#L338-L346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 442 - 460, The teardown
path must reverse both DNS artifacts created during setup. In
src/commands/local/workbench/net.rs:442-460, update stop_host_access to call
write_zone_file with the remaining blocks so dns-zone.tsv stays synchronized
with /etc/hosts; in src/commands/local/workbench/cluster_dns.rs:338-346, when
the last workspace stops, remove /etc/resolver/svc.cluster.local and stop the
stub DNS process, or ensure the stub DNS is independently kept running.
| let log_out = fs::OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(&log_path)?; | ||
| let log_err = log_out.try_clone()?; | ||
| let child = Command::new("python3") | ||
| .arg(&script) | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::from(log_out)) | ||
| .stderr(Stdio::from(log_err)) | ||
| .spawn() | ||
| .map_err(|e| format!("failed to spawn macOS stub DNS: {e}"))?; | ||
| let pid = child.id(); | ||
| std::mem::forget(child); | ||
| fs::write(&pid_path, format!("{pid}\n"))?; | ||
| std::thread::sleep(Duration::from_millis(200)); | ||
| if !pid_is_alive(pid) { | ||
| return Err("macOS stub DNS exited immediately".into()); | ||
| } | ||
| log::info!("macOS stub DNS for *.svc.cluster.local on 127.0.0.1:{MACOS_LOCAL_DNS_PORT} pid={pid}"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not fail hops local up when python3 is absent.
ensure_macos_stub_dns returns an error when the spawn fails, and start_host_access propagates it with ?. macOS 12.3 and later do not ship /usr/bin/python3 unless the Command Line Tools are installed. On such a machine the whole workspace bring-up fails, even though /etc/hosts and the loopback aliases are already in place and provide working resolution. The stub DNS only removes the mDNS delay.
Log a warning and continue when the stub DNS cannot start.
🛡️ Proposed fix
- ensure_macos_stub_dns(state_dir)?;
+ if let Err(e) = ensure_macos_stub_dns(state_dir) {
+ log::warn!("macOS stub DNS unavailable ({e}); falling back to /etc/hosts resolution");
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 593 - 614, Update
ensure_macos_stub_dns and its caller start_host_access so failure to spawn the
optional macOS stub DNS logs a warning and continues successfully instead of
propagating the error. Preserve existing setup and error handling for required
operations, while treating only the stub DNS startup failure as non-fatal.
| while true; do | ||
| while IFS=$'\t' read -r NS SVC IP PORT KEY; do | ||
| [ -z "${{NS:-}}" ] && continue | ||
| safe=$(echo "$KEY" | tr '/:' '__') | ||
| pf="$PIDDIR/$safe.pid" | ||
| pid="" | ||
| if [ -f "$pf" ]; then pid=$(cat "$pf" 2>/dev/null || true); fi | ||
| if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then continue; fi | ||
| if [ -n "$pid" ]; then | ||
| kill "$pid" 2>/dev/null || true | ||
| echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) restart $KEY" >>"$LOG" | ||
| else | ||
| echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) start $KEY $IP:$PORT" >>"$LOG" | ||
| fi | ||
| k port-forward -n "$NS" --address "$IP" "svc/$SVC" "${{PORT}}:${{PORT}}" >>"$LOG" 2>&1 & | ||
| echo $! >"$pf" | ||
| done < "$CONFIG" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The supervisor never reaps exited port-forwards, so kill -0 reports them as alive.
The script backgrounds each kubectl port-forward and never calls wait on it. When a forward exits, it stays a zombie child of the supervisor. kill -0 "$pid" succeeds for a zombie, so the continue at line 709 runs and the forward is never restarted. The self-healing loop is defeated for exactly the failure it exists to handle. pid_is_alive in this file already treats state Z as dead, which confirms the case is expected.
Reap the exited children so the pid disappears.
🐛 Proposed fix
while true; do
+ # Reap exited forwards so `kill -0` does not see zombies.
+ while wait -n 2>/dev/null; do :; done
while IFS=$'\t' read -r NS SVC IP PORT KEY; do
[ -z "${{NS:-}}" ] && continue
safe=$(echo "$KEY" | tr '/:' '__')
pf="$PIDDIR/$safe.pid"
pid=""
if [ -f "$pf" ]; then pid=$(cat "$pf" 2>/dev/null || true); fi
if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then continue; fi
if [ -n "$pid" ]; then
kill "$pid" 2>/dev/null || true
+ wait "$pid" 2>/dev/null || true
echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) restart $KEY" >>"$LOG"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 702 - 718, Update the
supervisor loop around the backgrounded port-forward started by `k port-forward`
to reap exited child processes before checking whether their recorded PID is
alive. Reuse the existing `pid_is_alive` behavior that treats zombie state as
dead, and ensure completed children are waited on so stale zombie PIDs cannot
make the `kill -0` check continue indefinitely.
| let piddir = state_dir | ||
| .join(RUNTIME_SUBDIR) | ||
| .join(format!("{workspace}.dns-pf-pids")); | ||
| if let Ok(entries) = fs::read_dir(piddir) { | ||
| for ent in entries.flatten() { | ||
| if let Ok(s) = fs::read_to_string(ent.path()) { | ||
| if let Ok(pid) = s.trim().parse::<u32>() { | ||
| let _ = Command::new("kill") | ||
| .args(["-TERM", &pid.to_string()]) | ||
| .status(); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Delete the pid files after the sweep.
The loop reads every pid file under <workspace>.dns-pf-pids and sends TERM, but it never removes the files or the directory. The directory persists across runs. On the next teardown the sweep reads pid values from an earlier run. The operating system recycles pids, so the command can terminate an unrelated process that the user owns.
Remove each file after the kill, and remove the directory.
🐛 Proposed fix
- if let Ok(entries) = fs::read_dir(piddir) {
+ if let Ok(entries) = fs::read_dir(&piddir) {
for ent in entries.flatten() {
if let Ok(s) = fs::read_to_string(ent.path()) {
if let Ok(pid) = s.trim().parse::<u32>() {
let _ = Command::new("kill")
.args(["-TERM", &pid.to_string()])
.status();
}
}
+ let _ = fs::remove_file(ent.path());
}
+ let _ = fs::remove_dir(&piddir);
}📝 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.
| let piddir = state_dir | |
| .join(RUNTIME_SUBDIR) | |
| .join(format!("{workspace}.dns-pf-pids")); | |
| if let Ok(entries) = fs::read_dir(piddir) { | |
| for ent in entries.flatten() { | |
| if let Ok(s) = fs::read_to_string(ent.path()) { | |
| if let Ok(pid) = s.trim().parse::<u32>() { | |
| let _ = Command::new("kill") | |
| .args(["-TERM", &pid.to_string()]) | |
| .status(); | |
| } | |
| } | |
| } | |
| } | |
| let piddir = state_dir | |
| .join(RUNTIME_SUBDIR) | |
| .join(format!("{workspace}.dns-pf-pids")); | |
| if let Ok(entries) = fs::read_dir(&piddir) { | |
| for ent in entries.flatten() { | |
| if let Ok(s) = fs::read_to_string(ent.path()) { | |
| if let Ok(pid) = s.trim().parse::<u32>() { | |
| let _ = Command::new("kill") | |
| .args(["-TERM", &pid.to_string()]) | |
| .status(); | |
| } | |
| } | |
| let _ = fs::remove_file(ent.path()); | |
| } | |
| let _ = fs::remove_dir(&piddir); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/local/workbench/net.rs` around lines 920 - 933, Update the PID
sweep around the piddir read_dir loop to remove each processed PID file after
attempting the TERM command, then remove the piddir directory after the sweep
completes. Preserve the existing behavior for unreadable files and invalid PID
contents.
Share one delivery host path across apps in a workspace so monorepo codegen and local packages stay coherent; each worktree keeps its own changes. Explicit deliveryPath remains an override.
1 participant
Summary
hops local gitops cluster|worktreeand start--gitopsintegration127.0.0.1:30500), TLS-aware proberegistries.yamlin-node (no host bind mount), wire hosts for local registry pullsTest plan
hops-cli config install --path xrs/stacks/aws/secreton dory → Healthy Configurationregistry_port_forward_argsand dory registries helpersSummary by CodeRabbit
New Features
Documentation
Tests