refactor: fix issues, and add new features#217
Merged
Conversation
…, fix release-tag URLs - cluster cleanup: pin every helm call to the target cluster's kube-context (was: acted on the kubeconfig's CURRENT context — could uninstall releases from an unrelated/production cluster); parse `helm list -o json` properly instead of splitting on ":" (namespace extraction was broken, every uninstall silently failed) - cluster delete --force: select containers by exact-match label k3d.cluster=<name> instead of the unanchored name=k3d-<name> regex that also removed containers of similarly-named clusters (dev -> dev-2) - wsllauncher: build release asset URLs with the bare tag (release.yml tags "0.4.7", not "v0.4.7") — WSL auto-install 404'd for every published release - selfupdate: ForTag falls back to the alternate v-prefix spelling, so `openframe update v0.4.7` and `0.4.7` both resolve; typed ErrReleaseNotFound Regression tests for all three T0s (cleanup kube-context pinning, label-based force-delete selection, bare-tag URLs + ForTag fallback).
… gates, banner every step - make test-unit/test-race now include the root package (main_test.go — the only exit-code fidelity tests) and tests/testutil, both silently skipped by the old ./cmd/... ./internal/... pattern; integration tests stay excluded (they create real k3d clusters) - new jobs: cross-compile all release platforms (catches GOOS-specific compile breakage early), govulncheck (reachable known vulnerabilities, blocking), go mod tidy -diff in lint - smoke `update check -o json` (machine-clean stdout) alongside the text form - uniform greppable banners (=== TEST:/SETUP:/TEARDOWN:) at the start of every step plus `---` sub-banners inside composite steps, so raw logs can be navigated with a single grep
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates CLI ref and context handling, cluster cleanup targeting, prerequisite failures, secret redaction, self-update installation, environment parsing, CI coverage, and release validation workflows. ChangesApplication, cluster, and security workflows
CI and release validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
…release pipeline - guard: releases can only be dispatched from main — the self-updater's cosign identity pin (release.yml@refs/heads/main) rejects assets signed from any other ref, so a non-main release would fail verification for every user - gates: golangci-lint + make test-unit must pass BEFORE the tag is created and anything is published - smoke after GoReleaser: the built linux/amd64 binary must report the exact release version (guards the ldflags -X injection that self-update depends on), and the bare-tag download URLs (linux_amd64.tar.gz, checksums.txt, checksums.txt.bundle) must resolve — end-to-end guard for the WSL auto-install / `openframe update` download contract - cleanup on failure: delete the pushed tag and the half-published release, so a failed run no longer wedges the next dispatch (version.yml resolves the same version and the tag push collides) and never leaves a broken release visible as latest - greppable === GUARD:/GATE:/SMOKE:/CLEANUP: banners, matching test.yml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/test.yml (1)
190-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate JSON parseability for
update check -o json.The comment on Line 197 states "json mode must keep stdout machine-clean (no spinner) and stay parseable," but the test only tolerates network failure — it never validates that the JSON output is actually parseable when the command succeeds. Consider piping through
jqto verify parseability on success.♻️ Proposed fix to validate JSON parseability
"$OF_BIN" update check || echo "update check skipped (network)" # json mode must keep stdout machine-clean (no spinner) and stay parseable. - "$OF_BIN" update check -o json || echo "update check -o json skipped (network)" + if "$OF_BIN" update check -o json >update-check.json 2>/dev/null; then + jq empty update-check.json || { echo "::error::update check -o json produced invalid JSON"; cat update-check.json; exit 1; } + else + echo "update check -o json skipped (network)" + fi🤖 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/test.yml around lines 190 - 199, Validate the successful JSON output from the “update check -o json” command by piping it through jq, while preserving the existing network-failure tolerance; update the command in the workflow’s update-check section so jq runs only when the command succeeds and causes the test to fail for invalid JSON.internal/shared/selfupdate/release.go (2)
107-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap
ErrReleaseNotFoundin the both-miss error for caller introspection.The
fmt.Errorfon line 109 doesn't wrapErrReleaseNotFound, so callers can't useerrors.Is(err, ErrReleaseNotFound)to distinguish "not found" from transient failures. SinceErrReleaseNotFoundis exported, callers may expect to check for it.♻️ Suggested improvement
- return Release{}, fmt.Errorf("no release found for tag %q (also tried %q)", tag, alt) + return Release{}, fmt.Errorf("no release found for tag %q (also tried %q): %w", tag, alt, ErrReleaseNotFound)🤖 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 `@internal/shared/selfupdate/release.go` around lines 107 - 110, Update the both-tag-miss error in getRelease to wrap ErrReleaseNotFound using a %w format verb while retaining the tag and alternate-tag context, so errors.Is can identify the exported sentinel.
91-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
alt == tagguard is unreachable.
alternateTageither strips a leading"v"or prepends one, so it always returns a string different from the input. Theif alt == tagbranch on lines 104-106 is dead code. It's harmless as a defensive guard, but consider adding a comment noting it's intentional protection against future changes toalternateTag.🤖 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 `@internal/shared/selfupdate/release.go` around lines 91 - 123, Document the defensive purpose of the alt == tag check in Client.ForTag, noting that alternateTag currently always toggles the v prefix but the guard protects against future changes. Add a concise comment immediately before the branch without changing its behavior.internal/cluster/cleanup_helm_test.go (1)
141-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
hasFlagValuefor stricter kube-context verification in the integration test.
TestCleanupCluster_HelmPhasePinsKubeContextusesassert.Containsf(t, argv, "--kube-context", ...)which only checks for the flag's presence. The unit-level test (TestCleanupHelmReleases_PinsKubeContext) correctly useshasFlagValueto verify both flag and value. Consider applying the same strictness here to ensure the correct context value is pinned, not just that the flag exists.♻️ Proposed refactor
for _, argv := range argvs { - assert.Containsf(t, argv, "--kube-context", "helm call without --kube-context: %v", argv) + assert.Truef(t, hasFlagValue(argv, "--kube-context", "k3d-test-cluster"), + "every helm call must pin --kube-context k3d-test-cluster, got: %v", argv) }🤖 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 `@internal/cluster/cleanup_helm_test.go` around lines 141 - 154, Strengthen TestCleanupCluster_HelmPhasePinsKubeContext by replacing the flag-presence assertion with hasFlagValue, verifying every captured Helm argument list includes --kube-context paired with the expected "test-cluster" value, while retaining the existing failure context.
🤖 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 `@internal/cluster/providers/k3d/manager.go`:
- Around line 254-260: Prevent shell injection in the cleanup command by
validating the user-provided cluster name before it reaches command
construction. Add an isValidClusterName helper allowing only non-empty lowercase
letters, digits, and hyphens, and invoke it at the relevant entry point before
constructing cleanupCmd; return an appropriate error for invalid names. Ensure
all shell interpolation of clusterName is covered.
In `@internal/cluster/service.go`:
- Around line 299-304: Remove the unconditional "--wait" flag from the uninstall
arguments in cleanupHelmReleases to match HelmManager.UninstallRelease’s
non-blocking cleanup behavior; alternatively, replace it with a short bounded
"--timeout 30s" if waiting is required.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 190-199: Validate the successful JSON output from the “update
check -o json” command by piping it through jq, while preserving the existing
network-failure tolerance; update the command in the workflow’s update-check
section so jq runs only when the command succeeds and causes the test to fail
for invalid JSON.
In `@internal/cluster/cleanup_helm_test.go`:
- Around line 141-154: Strengthen TestCleanupCluster_HelmPhasePinsKubeContext by
replacing the flag-presence assertion with hasFlagValue, verifying every
captured Helm argument list includes --kube-context paired with the expected
"test-cluster" value, while retaining the existing failure context.
In `@internal/shared/selfupdate/release.go`:
- Around line 107-110: Update the both-tag-miss error in getRelease to wrap
ErrReleaseNotFound using a %w format verb while retaining the tag and
alternate-tag context, so errors.Is can identify the exported sentinel.
- Around line 91-123: Document the defensive purpose of the alt == tag check in
Client.ForTag, noting that alternateTag currently always toggles the v prefix
but the guard protects against future changes. Add a concise comment immediately
before the branch without changing its behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 529fd64c-4bfb-4fd8-acb3-73431ae5f7e5
📒 Files selected for processing (10)
.github/workflows/test.ymlMakefileinternal/cluster/cleanup_helm_test.gointernal/cluster/providers/k3d/forcedelete_test.gointernal/cluster/providers/k3d/manager.gointernal/cluster/service.gointernal/shared/selfupdate/release.gointernal/shared/selfupdate/release_test.gointernal/shared/wsllauncher/install.gointernal/shared/wsllauncher/install_test.go
- new ui.RequireConfirmation: in CI/piped sessions a required confirmation fails fast with the skip-flag hint instead of blocking on a prompt no one can answer - update/rollback: REQUIRE --yes when non-interactive — the old guard silently auto-confirmed replacing the binary in CI (inverted polarity); OPENFRAME_AUTO_UPDATE opt-in path unchanged - app uninstall / cluster delete / cluster cleanup: fail fast with a --yes/--force hint when non-interactive; cluster cleanup --force now actually skips the confirmation (it always prompted, hanging CI) and its help text says so - inotify limits: skip entirely on macOS (no fs.inotify.* there), read current values first, escalate only via `sudo -n` (fails instead of prompting on /dev/tty mid-spinner) — bootstrap --non-interactive can no longer stall on a hidden sudo password prompt; WSL branch prompt-free too - prerequisites installers (cluster + chart): drop every "Continuing anyway" — install/start failures fail fast with the real cause instead of a misleading k3d/helm error minutes later; freshly installed Docker is now started and waited for (30s→120s budget for cold Docker Desktop starts, re-login hint for Linux docker-group membership) - cluster prerequisite gate ORs ui.IsNonInteractive() like the chart gate, so a forgotten --non-interactive in CI can't reach an interactive confirm Tests: RequireConfirmation fail-fast, cleanup/delete force-vs-noninteractive guards, inotify darwin-skip/sufficient-skip/sudo -n/failure-message/WSL, and containsTool.
…values file, and install target - branch resolution reads the FLATTENED schema (top-level repository.branch — the key the pipeline writes and the chart consumes); the legacy nested deployment.oss.repository.branch struct and dead GetCurrentBranch fallback are deleted. A values-file branch is no longer silently ignored, and a stale legacy-schema file can no longer override an explicit --ref - app upgrade requires openframe-helm-values.yaml: upgrading with an empty values map made `helm upgrade` replace release values with chart defaults, wiping registry credentials/ingress settings when run from the wrong directory. Fresh installs/bootstrap keep defaults but announce it loudly (a clean machine legitimately has no values file yet) - upgrade --ref + --sync is rejected as mutually exclusive — --sync used to silently discard --ref and re-sync the CURRENT ref - one kube-target per install: the --context/interactively-selected context is threaded into every helm CLI call (winning over the cluster-derived one via a single helmKubeContext helper), and the ArgoCD readiness wait is built from the SAME rest.Config as the HelmManager (NewArgoCDForTarget) instead of lazily resolving the kubeconfig's current context — previously one install could target three different clusters Tests: flattened-vs-legacy branch resolution (+end-to-end into AppOfApps), upgrade-requires-values vs fresh-install-defaults, ref+sync rejection, helmKubeContext precedence into helm argv.
… git ref The two flags fed the same value (resolvedRef) with confusing precedence rules and duplicated the F5 mutual-exclusion surface. --ref covers both branches and release tags. - drop the flag from install/upgrade (shared addInstallFlags), the InstallFlags field, extraction, and the upgrade --sync guard - resolvedRef(): --ref when set, else the default platform branch (main) - contract tests updated — the flag's absence is now part of the frozen CLI surface; unit tests and docs mention only --ref BREAKING CHANGE: scripts using --github-branch must switch to --ref (fails fast with "unknown flag" instead of being silently ignored).
…rict env flags, verified WSL install - TLS verification is no longer disabled for *.local hosts: mDNS/legacy-AD .local domains are common for real corporate clusters, and the suffix match enabled silent MITM. Only loopback/0.0.0.0/localhost/host.docker.internal keep the local-cluster bypass - redaction actually works now: values-file loading registers the docker registry password and ngrok apiKey/authtoken (chokepoint), prompt sites register collected secrets, and CommandError/WSLError fields are redacted at construction — previously only verbose prints were scrubbed while the returned error carried the raw command line and stderr - HTTP timeouts: download.Downloader defaults to a 5-minute client (was http.DefaultClient — unbounded), and Apply runs under a deadline both in auto-update (10m, runs after the user's command off context.Background) and `openframe update` (15m) - strict OPENFRAME_* boolean parsing via config.EnvBool: `=0`/`=false` no longer toggles INSECURE_SKIP_VERIFY (!), NO_UPDATE_CHECK, NO_WSL_FORWARD - keep the .bak when saving the rollback point fails — deleting it destroyed the only copy of the previous binary while advertising rollback - WSL auto-install goes through the full self-update trust chain: the Linux release is cosign+SHA256-verified on the Windows side (selfupdate.FetchVerifiedLinuxBinary) and streamed into WSL via stdin; the curl-inside-WSL path (integrity-only, same-release checksums) is deleted along with its URL builders Tests: .local-is-remote guard, CommandError/URL-credential redaction, values-file secret registration, EnvBool table, offline unsigned-release refusal + insecure opt-in fetch chain, no-download stdin install script.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/chart/providers/helm/manager_args_test.go (1)
40-58: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
KUBECONFIGisolation toTestArgoCDInstallArgs_ClusterContextAndDryRun.This test relies on
helmKubeContextresolvingClusterName: "demo"tok3d-demo, but unlikeTestHelmKubeContext(which setsKUBECONFIGto a nonexistent path for determinism), it does not isolate the kubeconfig. If the host machine has a real kubeconfig with a context for a cluster named "demo",k8s.ResolveContextForClustermay return a different context name, making the test flaky.🛡️ Proposed fix
func TestArgoCDInstallArgs_ClusterContextAndDryRun(t *testing.T) { + // Isolate from the host kubeconfig so the ClusterName fallback is + // deterministic (k3d-<name>) regardless of the machine running the test. + t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "absent")) + args := argoCDInstallArgs(config.ChartInstallConfig{ClusterName: "demo", DryRun: true}, "v.yaml")🤖 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 `@internal/chart/providers/helm/manager_args_test.go` around lines 40 - 58, Isolate the test from the host kubeconfig so context resolution is deterministic. At the start of TestArgoCDInstallArgs_ClusterContextAndDryRun, set KUBECONFIG to a nonexistent path using t.Setenv, matching the setup in TestHelmKubeContext, before calling argoCDInstallArgs.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
68-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the golangci-lint version instead of using
latest.The action is pinned to a specific commit hash (good), but
version: latestmeans the golangci-lint binary itself floats. A new linter release can introduce rules that fail the release gate unexpectedly, making releases non-reproducible. Pin to a specific version (e.g.,v2.1.6) for consistency.♻️ Proposed fix
- name: 'Gate: lint' uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: - version: latest + version: v2.1.6🤖 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/release.yml around lines 68 - 80, Pin the golangci-lint binary used by the “Gate: lint” step to a fixed version such as v2.1.6, replacing version: latest while keeping the existing action commit pin unchanged.internal/cluster/ui/operations.go (1)
198-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify confirmation handling in
internal/cluster/ui/operations.goconfirmDeletionreimplements the non-interactive guard thatRequireConfirmationalready centralizes, and cleanup/deletion also differ in error wrapping (WrapConfirmationErrorvs raw errors). Consider using the shared helper in both paths, or applying the same wrapping to cleanup for consistent user-facing messages.🤖 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 `@internal/cluster/ui/operations.go` around lines 198 - 211, Unify confirmCleanup and confirmDeletion by routing both through sharedUI.RequireConfirmation, removing the duplicated sharedUI.IsNonInteractive guard in confirmDeletion. Ensure both methods use the same confirmation prompt/options and apply consistent confirmation error wrapping, including WrapConfirmationError where appropriate.
🤖 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 @.github/workflows/test.yml:
- Around line 78-103: Resolve the disabled vulncheck CI job by either
uncommenting the complete vulncheck workflow, including its job definition and
steps, so it runs as a blocking check, or remove the commented-out block and
document deferral in an issue. Keep the existing govulncheck configuration and
explanatory comments only if the job is enabled.
In `@internal/chart/ui/configuration/docker.go`:
- Around line 79-80: Register the trimmed password with the redaction system
instead of the raw input. Update the redaction call near the password handling
in the configuration code to use the same strings.TrimSpace(password) value that
is stored and passed downstream, matching ingress.go behavior.
In `@internal/shared/executor/executor.go`:
- Around line 438-445: Redact the command stderr before returning the result to
prevent sensitive output from reaching user-facing messages. In the executor
logic surrounding the WSLError and CommandError returns, assign `result.Stderr`
to `redact.Redact(result.Stderr)` before each return, including the WSL error
path and the final `CommandError` path.
---
Outside diff comments:
In `@internal/chart/providers/helm/manager_args_test.go`:
- Around line 40-58: Isolate the test from the host kubeconfig so context
resolution is deterministic. At the start of
TestArgoCDInstallArgs_ClusterContextAndDryRun, set KUBECONFIG to a nonexistent
path using t.Setenv, matching the setup in TestHelmKubeContext, before calling
argoCDInstallArgs.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 68-80: Pin the golangci-lint binary used by the “Gate: lint” step
to a fixed version such as v2.1.6, replacing version: latest while keeping the
existing action commit pin unchanged.
In `@internal/cluster/ui/operations.go`:
- Around line 198-211: Unify confirmCleanup and confirmDeletion by routing both
through sharedUI.RequireConfirmation, removing the duplicated
sharedUI.IsNonInteractive guard in confirmDeletion. Ensure both methods use the
same confirmation prompt/options and apply consistent confirmation error
wrapping, including WrapConfirmationError where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65f93e75-72d5-41b0-9aac-19e90240220d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (54)
.github/workflows/release.yml.github/workflows/test.ymlcmd/app/contract_test.gocmd/app/install.gocmd/app/install_test.gocmd/app/uninstall.gocmd/app/upgrade.gocmd/app/upgrade_test.gocmd/update/update.godocs/getting-started/first-steps.mdgo.modinternal/chart/prerequisites/installer.gointernal/chart/providers/argocd/constants.gointernal/chart/providers/helm/kubecontext_test.gointernal/chart/providers/helm/manager.gointernal/chart/providers/helm/manager_args_test.gointernal/chart/services/argocd.gointernal/chart/services/chart_service.gointernal/chart/services/noninteractive_config_test.gointernal/chart/services/regression_0_4_6_test.gointernal/chart/ui/configuration/docker.gointernal/chart/ui/configuration/ingress.gointernal/chart/ui/templates/helm_modifier.gointernal/chart/ui/templates/register_secrets_test.gointernal/chart/utils/config/branch_resolution_test.gointernal/chart/utils/config/builder.gointernal/chart/utils/config/models.gointernal/chart/utils/types/interfaces.gointernal/cluster/models/flags.gointernal/cluster/prerequisites/docker/docker.gointernal/cluster/prerequisites/installer.gointernal/cluster/prerequisites/installer_test.gointernal/cluster/providers/k3d/inotify_test.gointernal/cluster/providers/k3d/manager.gointernal/cluster/service.gointernal/cluster/ui/operations.gointernal/cluster/ui/operations_noninteractive_test.gointernal/shared/config/envbool.gointernal/shared/config/envbool_test.gointernal/shared/config/transport.gointernal/shared/config/transport_test.gointernal/shared/download/verify.gointernal/shared/executor/executor.gointernal/shared/executor/redaction_test.gointernal/shared/selfupdate/auto.gointernal/shared/selfupdate/fetch.gointernal/shared/selfupdate/fetch_test.gointernal/shared/selfupdate/notice.gointernal/shared/selfupdate/update.gointernal/shared/ui/noninteractive_test.gointernal/shared/ui/prompts.gointernal/shared/wsllauncher/install.gointernal/shared/wsllauncher/install_test.gointernal/shared/wsllauncher/launcher.go
✅ Files skipped from review due to trivial changes (6)
- internal/chart/providers/argocd/constants.go
- internal/shared/config/envbool_test.go
- docs/getting-started/first-steps.md
- internal/cluster/prerequisites/installer_test.go
- internal/cluster/models/flags.go
- internal/chart/services/regression_0_4_6_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cluster/service.go
…act, make dry-run visible
- tests/integration/{bootstrap,cluster,root} now require `-tags integration`:
a bare `go test ./...` used to CREATE a real k3d cluster and run a full
37-second bootstrap as a side effect; only the pure helper tests in
tests/integration/common run untagged. make test-integration passes the tag
- contract tests for cmd/update — the one surface that rewrites the running
binary had zero drift protection: freeze `update [version]` (--yes/-y,
--force), `check` (--output/-o, default text), `rollback` (--yes/-y), and
add `update` to the root command's frozen subcommand set
- dry-run prints "Would run: <redacted command>" unconditionally via
pterm.Info (--silent still suppresses it): a non-verbose dry-run used to
produce no output and exit 0 — indistinguishable from a real run
- drop the vacuous TestExecuteWithVersion (asserted only that the symbol
exists); its real coverage is main_test.go, which CI now runs
Tests: dry-run visibility + redaction guards, update contract (root/check/
rollback), root subcommand set includes update.
- delete ~200 unreachable functions (guided by x/tools/cmd/deadcode) with their orphaned tests: the entire shared/ui/progress and shared/ui/messages template layers, the unmasked-input credentials prompter, deprecated UI shims, the parallel dead error system in chart_errors.go, dead retry policies/callbacks, and assorted dead helpers across every domain; where trivial, tests were migrated to the live API instead of deleted. Remaining deadcode findings are three documented test seams (SetTestExecutor, ResetGlobalFlags, redact.ClearSecrets) - merge the ~120-line ExecuteWithContextDeferred copy of the install workflow into ExecuteWithContext: deferred HelmManager resolution is now a nil-check at Step 2.5 instead of a whole forked workflow that kept drifting - break the prod -> tests/testutil import: the integration-test flag helpers compiled testutil (and testify) into the shipped binary - delete the decorative provider.New factory (never called by production — every constructor hard-codes k3d); the Provider interface plus the compile-time assertion remain as the real seam - interfaces.go: 13 declared interfaces -> the 5 actually implemented and consumed; drop the speculative ServiceFactory/Orchestrator/Workflow layer - GetStatusColor moved to shared/ui/status.go (the one live function inside the otherwise-dead tables.go)
…CreateCluster tests The B3 prompt-free inotify change made the path OS-dependent: on Linux it now reads current limits via `sysctl -n` before escalating. The testify mocks only expected the old bash call, so the unexpected call panicked on the Linux CI leg (invisible on darwin, where the whole step is skipped). Add .Maybe() expectations reporting sufficient limits — valid on both OSes.
…t --silent, ref-pinning docs - dry-run ends with an explicit "Dry run complete — nothing was changed."; "Using branch" reworded to "Deploying ref" (it never reflected cluster state) - cluster status JSON gains numeric ready_servers/total_servers alongside the human "1/1" string - --silent now produces zero non-error bytes: the Configuration Summary detail lines and bootstrap's blank spacers printed via raw fmt, bypassing the pterm redirection (the verification report's "silent" log had three blank lines); the CI guard is tightened from "no summary box" to "output file is empty" - docs: note that the root argocd-apps Application keeps its own ref while children are pinned, and how the CLI handles the post-upgrade mixed-ref window e2e: strict empty-output silent assertion with a loud-run positive control.
- cmd/help_matrix_test.go: walk the ENTIRE cobra tree — every current and future command must serve --help (never blocked by gates/cluster/network), carry a Short description, and document every flag; new commands are covered automatically - cli_matrix_test.go: hermetic real-binary matrix (25 cases) in an isolated env (temp HOME, absent kubeconfig, update checks and WSL forwarding off, closed stdin): version/help/completion/rollback happy paths with content asserts; unknown commands/flags; loud failures for the removed --github-branch and legacy --deployment-mode; parse-time flag validation; bootstrap arg-count and RFC1123 name validation; ref+sync exclusivity; uninstall-requires---yes; unknown --context; machine outputs on stdout; --silent stays undecorated - test.yml: e2e validation-matrix step for tool-dependent negatives (invalid -o, machine status without a name, nonexistent cluster) plus a jq contract on JSON outputs (list is an array; status carries name and the numeric ready_servers/total_servers) Interactive prompts are deliberately out of scope (cannot be automated); their non-TTY fail-fast behavior is asserted instead.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/test.yml (1)
78-103: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVulncheck job remains commented out.
This was previously flagged and the change details confirm the
vulncheckjob section is still added but commented out. The inline comment states it should be "Blocking," yet it does not run. Known vulnerabilities in reachable code paths are not being checked in CI.Either uncomment the job to enforce the gate, or remove the block and track enablement in an issue.
Run the following script to verify the current state of the vulncheck job:
#!/bin/bash # Description: Check if the vulncheck job is still commented out sed -n '78,103p' .github/workflows/test.yml🤖 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/test.yml around lines 78 - 103, Enable the vulncheck GitHub Actions job by uncommenting the entire block, including its job definition, checkout and Go setup steps, and govulncheck command, so it runs as a blocking CI check. If enablement is intentionally deferred, remove the commented block instead and track it through an issue.internal/cluster/providers/k3d/manager.go (1)
245-249: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winShell injection risk in WSL cleanup may persist after filter change.
The Docker container discovery filter was changed from
name=k3d-<cluster>tolabel=k3d.cluster=<cluster>, but the past review flagged thatclusterNameis interpolated unescaped into abash -cstring. Changing the filter type does not address the injection vector. Verify whetherclusterNameis still interpolated directly into shell commands.#!/bin/bash # Description: Inspect the WSL cleanup command construction for shell injection sed -n '237,280p' internal/cluster/providers/k3d/manager.go🤖 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 `@internal/cluster/providers/k3d/manager.go` around lines 245 - 249, The WSL cleanup command still interpolates clusterName directly into a shell command, leaving a shell-injection risk. In the cleanup logic around cleanupCmd and the WSL command execution, avoid embedding the value in bash -c text; pass it as a safely quoted argument or use an API/argument-based command invocation, preserving the exact label matching behavior.
🤖 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.
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 78-103: Enable the vulncheck GitHub Actions job by uncommenting
the entire block, including its job definition, checkout and Go setup steps, and
govulncheck command, so it runs as a blocking CI check. If enablement is
intentionally deferred, remove the commented block instead and track it through
an issue.
In `@internal/cluster/providers/k3d/manager.go`:
- Around line 245-249: The WSL cleanup command still interpolates clusterName
directly into a shell command, leaving a shell-injection risk. In the cleanup
logic around cleanupCmd and the WSL command execution, avoid embedding the value
in bash -c text; pass it as a safely quoted argument or use an
API/argument-based command invocation, preserving the exact label matching
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd46ba62-16e7-4484-89c5-45ec8d9d6598
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/test.ymlcmd/app/upgrade.godocs/getting-started/first-steps.mdinternal/bootstrap/service.gointernal/chart/providers/argocd/stall.gointernal/chart/providers/argocd/stall_test.gointernal/chart/providers/argocd/sync.gointernal/chart/providers/argocd/wait.gointernal/chart/services/appofapps.gointernal/chart/services/chart_service.gointernal/chart/services/context_target_test.gointernal/chart/services/noninteractive_config_test.gointernal/chart/utils/config/models.gointernal/chart/utils/types/interfaces.gointernal/cluster/models/cluster.gointernal/cluster/providers/k3d/manager.gointernal/cluster/providers/k3d/manager_test.gointernal/cluster/ui/operations.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/app/upgrade.go
- internal/chart/utils/config/models.go
- internal/cluster/ui/operations.go
- internal/chart/utils/types/interfaces.go
- internal/chart/services/chart_service.go
…installs
The N2 fix made `app install --context <ctx> --non-interactive --dry-run`
reachable, and the new e2e step immediately caught this: the post-install
release verification runs `helm list`, which the dry-run executor suppresses,
so a dry-run failed by construction ("helm list returned empty") — nothing
was ever created to verify. Return early after the install command in dry-run
mode; the cluster-reachability pre-check deliberately stays active (validating
the target is part of a dry-run's value).
Verified end-to-end against a live k3d cluster with the exact e2e invocation
(exit 0 through to "Dry run complete"). Regression unit test drives the full
InstallArgoCDWithProgress flow with a mock executor + fake clientset and
asserts no `helm list` follows the install, which still carries
--dry-run=client.
`go build -o <name>` produces an extensionless file, and os/exec on Windows
resolves executables via PATHEXT — so both the exit-code integration test
(main_test.go) and the CLI invocation matrix built binaries that Windows
refused to execute ("not found in %PATH%"). This surfaced on the first full
Windows CI run of the root package.
The old job built exactly the three GOOS/GOARCH combos the e2e runner legs already compile (and weaker: without test files). The release ships SIX combos, so breakage on the un-runnered three (linux/arm64, windows/arm64, darwin/amd64) would only surface when the release itself failed. Now: - `go build ./...` for all six GoReleaser combos (kept in sync with .goreleaser.yml), failing PRs in minutes instead of releases - `go vet ./...` for the three combos no runner ever builds — vet cross-compiles TEST files too, catching the class of Windows-only test breakage that just hit CI, for platforms where tests never run
cleanupHelmReleases removes every release in the cluster, including argo-cd and app-of-apps — whose Application CRs carry ArgoCD's resources-finalizer. Once the ArgoCD controller itself is being removed, nothing can clear that finalizer, so --wait blocked for helm's default 5 minutes PER release (and the timeout error was verbose-only). UninstallRelease in the chart provider already documents and applies the same fire-and-forget rationale; cleanup now matches it, with a regression test asserting --wait never returns. Known leftover (pre-existing, now documented in the code and backlog): finalizer-pinned Application CRs stay Terminating and can pin the argocd namespace — the proper fix is the finalizer stripping `app uninstall` does.
…e stored password form - executor: result.Stderr is redacted where it is populated (ExitError parse) — the single chokepoint covering every consumer, including the non-verbose "Helm output: %s" user-facing error that embedded raw stderr; a child process echoing a token back can no longer leak it (regression test drives a real subprocess) - helm manager: verbose "Helm stdout:" print is redacted at the PRINT site — helm's rendered dry-run/verbose output includes values (docker registry password); the result struct itself stays raw since callers parse it - docker configurator: register the TRIMMED password — the raw padded input never matched the trimmed value the config actually stores (ingress.go already did this correctly) - selfupdate: binaryVersion --version timeout 5s→30s — exceeding it only blanks the rollback label, while 5s was repeatedly exceeded under a fully parallel test run, flaking TestSavePreviousAndRollback
`cluster delete <name> --force` skipped both the existence check and any command-layer validation (only create/bootstrap validated), and its Docker fallback interpolates the name into a `bash -c` string run with sudo inside WSL — a name like `x'; whoami; '` broke out of the single-quoted filter and executed arbitrary commands. Validate with the existing models.ValidateClusterName (RFC1123: [a-zA-Z0-9-], no shell metacharacters) at two points: K3dManager.DeleteCluster (the domain boundary, covering every delete path and both fallbacks) and forceCleanupDockerContainersWSL (the sink itself, so a future caller cannot reintroduce the injection). No ad-hoc escaping: after validation the alphabet contains no metacharacters, and a second validator would drift from the canonical one. Guard tests: ten injection payloads × force on/off must error with ZERO commands executed; valid names still reach k3d. BREAKING: `cluster delete` now rejects names that were previously passed through to k3d (which would have failed there anyway).
…release - e2e (linux+darwin): a new CI step exercises the LIVE trust chain end to end on a copy of the binary — resolve the latest release via the API, update from a stamped 0.0.1 (dev builds refuse to self-update), verify the cosign signature and checksums for real, assert --version matches the release, roll back, assert the previous version returns, and confirm a second rollback is a clean no-op. Also covers `update <tag>` in both spellings (bare and v-prefixed) and refusal of a nonexistent release. The tag is resolved dynamically, so the test does not rot with the next release. Windows is excluded on purpose: the native launcher forwards into WSL, so the step would silently test the Linux binary instead - offline units: dev builds are flagged and never offered an update (with a release-version control), explicit tags resolve in either spelling, Apply refuses on native Windows with WSL guidance, and Rollback consumes its restore point exactly once
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/chart/providers/helm/manager.go`:
- Around line 399-413: Redact Helm stdout in the error fallback before including
it in the returned error. Locate the error-handling logic immediately before the
verbose logging block, where result.Stdout is used when result.Stderr is empty,
and pass that stdout through redact.Redact while preserving stderr handling and
error context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f3e4bca-4dea-4c70-a1df-3688e16a74d4
📒 Files selected for processing (16)
.github/workflows/test.ymlcli_matrix_test.gocmd/help_matrix_test.gogo.modinternal/chart/providers/helm/dryrun_install_test.gointernal/chart/providers/helm/manager.gointernal/chart/ui/configuration/docker.gointernal/cluster/cleanup_helm_test.gointernal/cluster/providers/k3d/manager.gointernal/cluster/providers/k3d/name_validation_test.gointernal/cluster/service.gointernal/shared/executor/executor.gointernal/shared/executor/redaction_test.gointernal/shared/selfupdate/rollback.gointernal/shared/selfupdate/update_flow_test.gomain_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/chart/ui/configuration/docker.go
- internal/cluster/cleanup_helm_test.go
- go.mod
- internal/shared/executor/executor.go
…t the verbose log The previous change redacted stdout at the verbose print site while the adjacent failure path still embedded it raw: when helm fails with an empty stderr (routine on Windows/WSL, where stderr is folded into stdout via 2>&1), `Helm output: %s` leaked whatever helm rendered — including the docker registry password. Two more raw prints existed in argocd_wait.go (`helm list` and `helm status` verbose output). - error path: redact the stdout fallback before it enters the error string - argocd_wait: redact both verbose stdout prints at the print site (the struct value stays raw — callers parse it)
… can be reaped `cluster cleanup` uninstalled the Helm releases but left the ArgoCD Application CRs behind: their resources-finalizer can only be cleared by the ArgoCD controller, which the same cleanup had just removed. The CRs stayed in Terminating and pinned the argocd namespace forever, so the next `app install` failed on CRD ownership. Cleanup now brackets the Helm uninstall the way `app uninstall` already does: delete the Applications while the controller still runs (it cascades workload cleanup), then strip the finalizers once it is gone. internal/cluster must not import internal/chart, so the phases run through a new ApplicationCleaner interface whose ArgoCD-backed implementation is injected by cmd/cluster — the same inversion used for ClusterAccess. The cleaner is optional: an unreachable cluster, or one without OpenFrame, cleans up as before. Tests trace helm calls and cleaner calls into one ordering log and assert delete -> helm-uninstall -> strip (verified to fail when the order is inverted), plus the no-cleaner and cleaner-error paths.
On Windows the root command forwards the entire CLI into WSL before any
command runs (wsllauncher), so the Linux build there has GOOS=linux and every
`runtime.GOOS == "windows"` branch below it was unreachable — reachable only
via the undocumented OPENFRAME_NO_WSL_FORWARD escape hatch, where it was
broken anyway: it hardcoded the "Ubuntu" distro while the launcher is
distro-agnostic (OPENFRAME_WSL_DISTRO).
- bootstrap: drop initializeWSL — a PowerShell step that installed a hardcoded
Ubuntu, slept fixed intervals, and created a `runner:runner` account with
NOPASSWD sudo. A CI artifact that had no business in a released binary
- docker prerequisites: drop isDockerRunningWSL/startDockerWindows/
startDockerInWSL and the Windows branches of isDockerInstalled/
IsDockerRunning; StartDocker on Windows now says "run openframe inside WSL"
- k3d provider: drop the WSL branches of CreateCluster (WSL-IP TLS SAN,
Windows path conversion, kubeconfig server rewrite), verifyClusterReachable
and cleanupStaleLockFiles, plus convertWindowsPathToWSL, getWSLUser,
getWSLInternalIP, rewriteWSLKubeconfigServerAddress,
getKubeconfigContentFromWSL and forceCleanupDockerContainersWSL. The
force-delete Docker fallback now triggers on --force alone, not "force or
Windows". Two orphaned path_{other,windows}.go helpers go with them (helm
keeps its own independent copies)
- the shell-injection guard from the deleted WSL sink moves to the surviving
direct-Docker path, with its test migrated: all ten payloads still rejected
with zero commands executed
Kept: platform.WSLClusterHint guards and `GOOS != "windows"` checks (defensive,
not dead), and increaseInotifyLimitsFor's parameterized windows case.
Three defects of the same shape: the message was written, but nothing could
ever see it.
executor.CommandError discarded the child's stderr, so a failed
`k3d cluster create` reported "exit code: 1: exit status 1" while the real
reason ("port 6550 already allocated") was thrown away. Carry Stderr on the
error and render its tail in Error() -- bounded, tail-preserving, since the
reason sits at the end.
The handler matched an errors.CommandError type that nothing ever
constructed; real failures fell through to the generic dump. Delete the dead
type and match *executor.CommandError, the one the executor actually
returns. Writing the test for it exposed a second bug: the handler used bare
pterm.Printf, which writes straight to stdout and bypasses --silent; switch
to DefaultBasicText.
--verbose never called pterm.EnableDebugMessages(), so all 36 pterm.Debug
call sites (helm/k3d command lines, ArgoCD wait internals, prerequisite
decisions) printed nothing, ever. Enable it; --silent still wins.
selfupdate's two warnings -- signature verification skipped, rollback point
not saved -- went through the progress callback, which callers wire to
spinner.UpdateText. A security downgrade was announced as spinner text that
the next step overwrote within one frame. Route them to a new Warn hook
(stderr by default, pterm.Warning in `openframe update`) so they outlive the
operation and never touch stdout.
`cluster cleanup` swallows every phase error by design, so a broken cluster stays tearable-down. It paired that with a summary that printed "Removed unused Docker images / Freed up disk space / Optimized cluster performance" verbatim -- a run in which every phase failed looked exactly like a clean one. Phases now return counts, CleanupResult collects the failures, and the summary prints what happened: real counts, "Nothing to remove", or "finished with problems" naming the phases that did not complete. Writing the test exposed the counts going through bare pterm.Printf, which bypasses --silent. `cluster status --detailed` printed fixed CPU/Memory/Storage figures that were never measured -- identical on every machine at every moment. Replace with the nodes the provider actually reported, and say plainly that the CLI collects no metrics (pointing at `kubectl top`). The API endpoint was hardcoded to https://0.0.0.0:6550 in three places. 6550 is only the preferred port: findPort falls back to 6551/6552 when taken, so on a machine with a second cluster the box named another cluster's API server. Read it from the kubeconfig instead. GetChartStatus ran `helm status --output json`, discarded the output, and returned a literal {Status: "deployed", Version: "1.0.0"} -- a failed release reported itself healthy. Note it does NOT switch to parsing `helm status`: that JSON carries no chart version, and its top-level "version" is the release revision (verified against helm v4.2.2 on a live release), so parsing it would swap one wrong answer for another. `helm get metadata` returns status, chart version, and appVersion. `cluster create --help` promised existing clusters "will be recreated"; CreateCluster warns and reuses them.
… progress Two defects found while making the wait loop talk. triggerRepoServerRecovery and checkRepoServerHealth were nested inside `if config.Verbose`, so a corrective action depended on a logging flag: a user who did not pass --verbose never got the recovery, and a wedged repo-server simply burned the whole 60-minute timeout. The loop cannot be driven from a unit test (30s bootstrap phase), so wait_gating_test.go parses wait.go and fails if these calls are re-nested under a verbosity check. spinner.finish printed its final line via pterm.Success.WithWriter(s.out), and WithWriter overrides the io.Discard writer ui.SetSilent installs on the package-level printers -- so every spinner in the CLI printed SUCCESS to stdout under --silent. Consult ui.IsSilent (restored; it now has a real caller) and discard everything except Fail, which --silent still promises. Progress (N/M ready) moves into the spinner text by default, with a textual heartbeat every 30s (10s verbose) for CI logs, where the spinner is suppressed and nothing was printed at all. The periodic outputs no longer gate on `int(elapsed.Seconds())%10 == 0`: the status check runs every 2s, so landing on an exact multiple was luck, and a missed tick skipped an entire diagnostic cycle. The timeout error now names the applications that never became ready and the command to inspect them; the CRD timeout explains that the CRD ships with the ArgoCD release; RepoServerIssue.Message (OOMKilled, CrashLoopBackOff) was computed and discarded at every call site and is now printed once per distinct diagnosis, with the restart announced so it does not read as a new failure. Add the missing spinners: app-of-apps install (--wait --timeout 60m, the longest phase, silent), git clone, and the WSL binary fetch -- whose progress callback existed and was passed nil, so the first Windows run looked frozen.
The retry policy decided "is this transient?" by substring-matching
"network timeout" -- a phrase Go never emits (it says "i/o timeout") -- and
"tiller not ready", which Helm removed in 2019 and this CLI, driving Helm
3/4, could never see. Neither pattern could ever match, so genuinely
transient failures were not retried.
Classify structurally instead: net.Error.Timeout(), syscall.ECONNREFUSED and
friends, and the apiserver's backpressure errors (409/429/503). Order
matters and is now pinned by tests: an http.Client timeout satisfies BOTH
net.Error.Timeout() and errors.Is(context.DeadlineExceeded), while a
cancelled request is a net.Error whose Timeout() is false -- checking the
context sentinels first would have misfiled every network timeout as "the
operation is over". Substrings survive only as a fallback for child
processes, whose stderr now reaches us via executor.CommandError.
friendly.go matches substrings of other projects' error strings, which a
patch release can reword, silently dropping the hint. Pin seven verbatim
upstream messages (helm, client-go, kubectl, Docker) as a canary.
Smaller fixes: name the branch in BranchNotFoundError, name the namespace
and context when `helm uninstall` fails, stop prefixing every ChartError
with "chart" ("chart waiting failed for ArgoCD applications"), drop the
stale "Phase 2 will add cosign" package doc (cosign shipped), and raise a
failed config-service init from Debug to Warning.
Route 35 raw fmt.Print* calls through pterm so --silent holds. Machine
output -- `fmt.Print(string(b))` for json/yaml and the --quiet cluster-name
list -- is deliberately left alone: it must not gain styling or be
suppressible.
… isn't there
`cluster cleanup` ran `docker exec <node> docker image|container|volume|
network|system prune` on every k3d node. The nodes are rancher/k3s
containers running containerd and ship no docker binary, so every prune
exited 127 ("docker": executable file not found in $PATH). The errors were
swallowed, so cleanup reported success and reclaimed nothing, on every run.
Use crictl, which k3s provides as a symlink to its multi-call binary and is
always present in the node: `crictl rmi --prune` removes unused images. Drop
the container/volume/network/builder prunes entirely — those are Docker
concepts with no containerd equivalent inside the node; the node's volumes
and networks belong to the host daemon.
Verified end-to-end against a cluster created by this CLI: a seeded unused
image is removed, the summary reports the real count, and the previous test
(which asserted `docker image prune` WAS executed — pinning the broken
behaviour) is replaced by one that forbids any docker-in-node call.
Darwin — `openframe update` hit HTTP 403 mid-run on shared macOS runners. The updater reads GITHUB_TOKEN, but the "apply the real latest release" step exported only GH_TOKEN (which the step's curl uses), so the CLI ran unauthenticated and got rate-limited. Add a GitHubToken() helper that accepts both conventions — GITHUB_TOKEN (Actions) and GH_TOKEN (the gh CLI) — and route all six call sites through it, which also helps real users who have only `gh auth` set. Export GITHUB_TOKEN in the workflow step too. Linux — the timeout error printed an un-runnable command: `kubectl describe application argocd-apps (Health: Progressing) -n argocd`. assess.notReady entries are decorated "name (Health: X)" labels for display, and timeoutError fed one verbatim into the kubectl hint. Carry bare application names alongside the labels (assess.notReadyNames) and use those for the command, the labels for the human list. (The 15m timeout itself is a platform-convergence issue in CI, not a CLI bug.) Windows — TestInstallArgoCD_DryRunSkipsVerification and _ErrorDoesNotLeakStdout exercise InstallArgoCDWithProgress, which refuses on native Windows via WSLClusterHint before reaching the logic under test. That guard is dead in production (the CLI forwards into WSL first) and the behaviour asserted is OS-independent, covered on Linux/darwin. Skip on Windows, matching the existing precedent in namespace_test.go and manager_local_test.go. Each fix has a regression test: GitHubToken precedence, the timeout hint's bare-name usage, and the two Windows skips.
ivan-flamingo
approved these changes
Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
k3dforced-delete fallback targets containers by exact cluster label.--yes/--forceguidance;openframe updateconfirmations are consistently enforced.app install/upgraderef pinning uses--refonly, with kube-context aligned;--silentnow guarantees no extra output.integrationtag.