Fix gitops dry-run to catch manual_agent_install + macos_script conflict#44432
Conversation
…ict (#34464) The gitops dry-run was succeeding when `macos_manual_agent_install` was set to `true` and a `macos_script` was configured, but the actual gitops run would fail with a 422 error. This happened because the `manual_agent_install` validation only lived server-side in the script upload handler, which was skipped entirely during dry-run. Add client-side validation in both the team and no-team code paths that checks for this conflict before attempting to validate/upload the script, ensuring dry-run and real runs behave consistently.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44432 +/- ##
==========================================
+ Coverage 66.75% 66.79% +0.03%
==========================================
Files 2633 2639 +6
Lines 211745 212957 +1212
Branches 9424 9424
==========================================
+ Hits 141355 142236 +881
- Misses 57547 57755 +208
- Partials 12843 12966 +123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Covers both team and no-team code paths in the gitops client-side validation, ensuring 100% patch coverage of the new validation logic without requiring MySQL or Redis.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal bug fix that mirrors the existing server-side conflict check at the client-side validation step so dry-run surfaces the same error.
Extended reasoning...
Overview
The PR adds two early-exit conflict checks in server/service/client.go (team path ~L803, no-team path ~L2603) that error out when both macos_manual_agent_install is true and macos_script is set. This mirrors the existing server-side validation at ee/server/service/setup_experience.go:SetSetupExperienceScript(), which previously was unreachable during --dry-run because the script upload was gated by !opts.DryRun. The PR also adds a unit test in gitops_test.go and an enterprise integration test, plus a changes-file entry.
Security risks
None apparent. The change is purely defensive — it returns an error earlier rather than allowing a misconfiguration to slip through dry-run. No new code paths, network calls, file reads, or auth logic are introduced; the script file is no longer read when the conflict is present, which is fine since the operation is going to fail anyway.
Level of scrutiny
Low. This is a one-line conflict guard duplicated in two places with matching tests. The error messages reuse the existing wording (with a minor scope hint added for the team/unassigned case). No behavioral change for valid configurations.
Other factors
- Bug hunters found no issues.
- Codecov flags 50% patch coverage, but the missed lines are the new error-return branches; both are exercised by the new tests in
gitops_test.goand the integration suite, so coverage is effectively present even if the tool didn't pick up the branch counts on this run. - The new error string uses an ASCII apostrophe while the server-side message uses a curly apostrophe — purely cosmetic, no test relies on it.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
There was a problem hiding this comment.
Pull request overview
Fixes GitOps --dry-run validation so it surfaces the macos_manual_agent_install + macos_script setup experience conflict that previously only failed during a real (non-dry-run) GitOps run.
Changes:
- Added client-side conflict validation in the GitOps apply path for both team and no-team (unassigned) macOS setup experience script handling.
- Added/updated regression tests (unit + enterprise integration) to ensure both dry-run and real runs fail on the conflict.
- Added a changelog entry describing the dry-run behavior fix.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/service/client.go | Adds client-side validation to fail early when macos_script is set alongside macos_manual_agent_install. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Adds enterprise integration regression test covering dry-run and real run for team + no-team paths. |
| cmd/fleetctl/fleetctl/gitops_test.go | Adds unit regression test verifying conflict failure in both dry-run and real run. |
| changes/34464-gitops-dry-run-manual-agent-install-script-conflict | Documents the user-visible behavior change for GitOps dry-run conflict detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds validation to GitOps processing that detects when a macOS setup-experience provides a script while 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
…essages Match the server-side Unicode curly apostrophe (U+2019) in the client-side validation messages, addressing PR review feedback about wording consistency between dry-run and real-run error output.
There was a problem hiding this comment.
LGTM — small, well-scoped guard added in two places to mirror the existing server-side validation during gitops dry-run; integration test properly covers the regression and reviewer feedback has been addressed.
Extended reasoning...
Overview
This PR adds a client-side guard in server/service/client.go (team path at line 803-808 and no-team path at line 2603-2608) so that fleetctl gitops --dry-run catches the macos_manual_agent_install: true + macos_script: <path> conflict that previously only surfaced in real runs (because the script upload was gated by !opts.DryRun). It also adds a unit test in gitops_test.go and an enterprise integration test in gitops_enterprise_integration_test.go covering both team and no-team paths.
Security risks
None. This is a CLI-side validation that fails earlier with the same error users would already see on a real run; it widens no permissions, exposes no data, and accepts no new untrusted input. The error string interpolates the team key via %q (safe quoting).
Level of scrutiny
Low. The production change is ~10 lines of guard logic added immediately before existing validateMacOSSetupScript calls, following the same pattern. There is no logic forking, no new dependency, no API surface change, and no impact on real-run behavior (which already errored out via the server). The author has already addressed Copilot's concerns about apostrophe/Unicode consistency and explained the rationale for keeping the contextual suffixes.
Other factors
A bug-hunter inline comment flags that the unit test substring assertion ("macos_manual_agent_install") is too loose because the unit-test team YAML omits macos_bootstrap_package, so a pre-existing server-side validator emits an error containing the same substring — meaning the unit test would still pass even if the new client-side guard were reverted. This is a legitimate test-quality nit, not a production bug. The integration test correctly includes macos_bootstrap_package and properly isolates the regression scenario, so the fix is meaningfully covered. The nit is low-impact and can be addressed at the author's discretion.
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "macos_manual_agent_install") | ||
|
|
||
| // Real run should fail | ||
| _, err = RunAppNoChecks([]string{"gitops", "-f", teamFile.Name()}) |
There was a problem hiding this comment.
🟡 The new unit test TestGitOpsMacOSSetupScriptWithManualAgentInstallConflict asserts on the substring "macos_manual_agent_install", but the team YAML at lines 6516-6528 (and the no-team YAML at 6561-6568) omit macos_bootstrap_package, so the pre-existing server-side validator at server/fleet/app.go:582 returns an error that also contains that substring ("Couldn't enable macos_manual_agent_install. To use this option, first specify a bootstrap package."). As a result, the assertions on lines 6533/6537 (team) and 6573/6577 (no-team) would still pass even if the new client-side guard at server/service/client.go:803-808 were removed — defeating the regression-test purpose. To tighten this, either include macos_bootstrap_package in the YAMLs (matching the integration test) or assert on a more specific substring like "first disable macos_manual_agent_install".
Extended reasoning...
What the bug is
The new unit test TestGitOpsMacOSSetupScriptWithManualAgentInstallConflict is intended to be a regression guard for #34464, which added a client-side check at server/service/client.go:803-808 (team) and :2603-2608 (no-team) so that gitops --dry-run catches the macos_manual_agent_install + macos_script conflict. However, the unit test's assertion is too loose: assert.Contains(t, err.Error(), "macos_manual_agent_install") matches any error mentioning that field, not specifically the new conflict error.
The specific code path that triggers the false-negative
The team YAML at lines 6516-6528 sets:
controls:
setup_experience:
macos_manual_agent_install: true
macos_script: %sIt does not set macos_bootstrap_package. The pre-existing server-side validator at server/fleet/app.go:582-584 is:
if mos.ManualAgentInstall.Valid && mos.ManualAgentInstall.Value && (!mos.BootstrapPackage.Valid || mos.BootstrapPackage.Value == "") {
return NewInvalidArgumentError("setup_experience.macos_manual_agent_install", `Couldn't enable macos_manual_agent_install. To use this option, first specify a bootstrap package.`)
}This validator is invoked from ee/server/service/teams.go:1240 inside ApplyTeamSpecs (before the dry-run branch), so it fires on both dry-run and real runs. The returned error string contains the substring "macos_manual_agent_install".
Step-by-step proof (hypothetically reverting the fix)
- Imagine the new client-side guard at
server/service/client.go:803-808is removed. validateMacOSSetupScriptonly doesos.ReadFileon the script — succeeds (the test creates a real temp file).- The flow proceeds to
ApplyTeams(withDryRun=true). applyTeamSpecsatee/server/service/teams.go:1240callsspec.MDM.MacOSSetup.Validate().Validate()returns the pre-existing"Couldn't enable macos_manual_agent_install..."error becauseBootstrapPackageis empty.- The test reaches
assert.Contains(t, err.Error(), "macos_manual_agent_install")— and it PASSES because the bootstrap-package error string also containsmacos_manual_agent_install.
So the test passes today both because of the new client-side check and because of an unrelated server-side validation. If the new check were ever reverted/reordered, the unit test would still go green and silently lose its regression value.
Why existing code doesn't prevent it
The integration test at gitops_enterprise_integration_test.go:4798 correctly includes macos_bootstrap_package: %s in its team template, so it does properly isolate the new behavior at the integration layer. The unit test simply did not match that pattern.
Note: the no-team subtest is partially mitigated for a different reason — no-team controls are not folded into mdmAppConfig server-side, so MacOSSetup.Validate() isn't invoked for the no-team config. But the team subtest is still genuinely deficient, and tightening both keeps the assertions consistent.
Impact
This is a test-quality / regression-risk issue, not a production bug. The runtime fix in client.go is correct and the integration test does cover end-to-end behavior. But the unit test, despite its name (...WithManualAgentInstallConflict), is not a strict regression test for the new client-side guard.
How to fix
Either of these is sufficient (the integration test does the first):
- Add
macos_bootstrap_package: <some-url>to the team YAML so the bootstrap-package validator does not fire, leaving only the new client-side check as the source of error. - Tighten the assertion to a more discriminating substring that only the new check produces, e.g.
assert.Contains(t, err.Error(), "first disable macos_manual_agent_install")(or"Couldn't add setup experience script").
The same applies to the no-team subtest assertions on lines 6573/6577.
Related issue: Resolves #34464
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
Testing
What
GitOps
--dry-runwas succeeding whenmacos_manual_agent_installwas set totrueand amacos_scriptwas configured undersetup_experience, but the actual GitOps run would fail with:Why
The
manual_agent_installconflict validation only existed server-side inee/server/service/setup_experience.go:SetSetupExperienceScript(). The script upload call (uploadMacOSSetupScript()) was gated by!opts.DryRuninserver/service/client.go, so during dry-run the upload was skipped entirely and the validation never fired.Fix
Added client-side validation in
server/service/client.goat the point where the YAML-parsedMacOSSetupstruct is processed — before the script file is validated and loaded. This check runs for both dry-run and real runs, catching the conflict early. Two code paths were fixed:setup.ManualAgentInstall.Valuewhensetup.Script.Valueis setmacOSSetup.ManualAgentInstall.ValuewhenmacOSSetup.Script.Valueis setHow I reproduced the issue locally
Prerequisites
docker compose up -d mysql_test redisSteps
Wrote an integration test (
TestDryRunMacOSSetupScriptWithManualAgentInstallConflict) that:testdata/signed.pkg.shscript file withecho "setup script"macos_manual_agent_install: true,macos_script: <path>, andmacos_bootstrap_package: <url>fleetctl gitops --dry-runand asserts it failsfleetctl gitops(no dry-run) and asserts it failsRan the test before the fix — confirmed the bug:
Applied the fix and re-ran — both dry-run and real run now fail with the
macos_manual_agent_installconflict error.Test command
MYSQL_TEST=1 REDIS_TEST=1 go test -v \ -run TestIntegrationsEnterpriseGitops/TestDryRunMacOSSetupScriptWithManualAgentInstallConflict \ ./cmd/fleetctl/integrationtest/gitops/... -count=1 -timeout 600sBoth sub-tests (team and no-team paths) pass. All related existing tests continue to pass:
TestMacOSSetup,TestMacOSSetupScriptWithFleetSecret,TestDeletingNoTeamYAML,TestDisallowSoftwareSetupExperienceSummary by CodeRabbit
Bug Fixes
Tests