[slaskawi]

Description

Prevent normalized CLI_FEATURES values from being reinserted into zarf tools callback arguments. This fixes Zarf action callbacks such as uds zarf tools kubectl ..., which reject the root --features flag.

Related Issue

Fixes CLI-317

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Other (security config, docs update, etc)

Checklist before merging

• Test, docs, adr added or updated as needed
• Contributor Guide Steps followed

[slaskawi]

@greptileai review

[slaskawi]

@codex review

[slaskawi]

@copilot review

[Copilot]

@copilot review

The changes look correct and well-scoped. Here's a brief review:

internal/mode/mode.go

• The early return in addZarfFeatures (line 158–160) correctly prevents any --features injection for zarf tools (and z tools) commands. This is the right place for the guard — before any feature processing.
• The z alias handling added to nestedZarfCommandIndex and rootZarfToolsCommandIndex is consistent with the existing alias support in zarfCommandIndex.
• Edge cases like -- separator and root flags that take values are handled by the existing loop logic, which is preserved.

internal/mode/mode_test.go

• Three new unit test cases cover Legacy, alias (z), and Next mode — good breadth of coverage.

tests/legacy/e2e/zarf_test.go

• The e2e test exercises both a Legacy features string and a NextMode-enabled string, validating the fix end-to-end against a real kubectl invocation.

No issues found. The fix is minimal and correct.

[slaskawi]

@greptileai review

[slaskawi]

@copilot review

[slaskawi]

@codex review

[greptile-apps]

Greptile Summary

The PR prevents normalized CLI feature values from being passed to recognized Zarf vendor-tool callbacks and expands alias/root-flag handling.

• Adds vendor-tool detection before restoring Zarf feature arguments.
• Supports the z alias in nested command parsing.
• Adds unit and legacy end-to-end coverage for vendor-tool invocations with CLI_FEATURES.

Confidence Score: 5/5

The PR appears safe to merge because no eligible blocking failure remains.

No blocking failure remains.

Important Files Changed

Filename	Overview
internal/mode/mode.go	Adds vendor-tool detection and broadens Zarf command parsing so CLI features are omitted from callback arguments.
internal/mode/mode_test.go	Covers feature suppression for vendor tools, aliases, root flags, and non-vendor tools.
tests/legacy/e2e/zarf_test.go	Verifies kubectl callbacks run successfully with legacy and next-mode CLI feature environments.

_{Reviews (3): Last reviewed commit: "fix: keep CLI features out of Zarf tools" | Re-trigger Greptile}

[Copilot]

@copilot review

No code changes since the previous review. The fix remains correct and minimal — no issues found.

[slaskawi]

@codex please review the latest commit b02e306.

[Copilot]

Pull request overview

This PR fixes an argument/feature-handling edge case in the mode/feature bootstrap layer so that normalized CLI_FEATURES values are not re-added as --features=... when invoking uds zarf tools ... (which can cause Zarf’s vendored tool execution callbacks to reject the unexpected root flag).

Changes:

• Skip re-inserting --features=... when the resolved command is zarf tools (including when zarf root flags appear between zarf and tools).
• Extend Zarf command detection to recognize the z alias consistently.
• Add unit and legacy e2e coverage to ensure uds zarf tools kubectl ... works with CLI_FEATURES set (including with NextMode enabled).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
tests/legacy/e2e/zarf_test.go	Adds an e2e regression test ensuring CLI_FEATURES does not break uds zarf tools kubectl ....
internal/mode/mode.go	Prevents re-inserting --features for zarf tools and improves detection across flags/aliases.
internal/mode/mode_test.go	Adds unit test cases covering zarf tools feature-skipping behavior (including flags and alias).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b02e3062a3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[slaskawi]

@copilot review

[slaskawi]

@codex review

[slaskawi]

@greptileai review

[Copilot]

@copilot review

Reviewed commit 259353b. The implementation is correct and well-scoped:

• zarfVendorTool properly guards addZarfFeatures for zarf tools <vendor-tool> (and the z alias), returning false conservatively when no known vendor tool follows tools/t — so bare zarf tools and non-vendor subcommands still receive features.
• rootZarfToolsCommandIndex now correctly skips root flags (including value-consuming ones in both --flag value and --flag=value forms) between zarf/z and tools/t, consistent with nestedZarfCommandIndex.
• Unit tests cover root flags, equals-form flags, the z alias, non-vendor tools, and Next mode.

No issues found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 259353ba17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mjnagel]

In testing this does appear to fix the kubectl regression I experienced. I still think there's a broader concern though that is worth addressing or clearly deferring/opening a new issue for... --features / CLI_FEATURES still mixes UDS-owned features (for example, NextMode) with Zarf-owned features (for example, values) in one set. I'm not sure if there's any particular issues with this today but this could potentially cause problems in the future if:

• CLI or Zarf perform any validation around feature flags (that the selected flag actually exists)
• CLI/Zarf have overlapping feature flags (same name - this might be okay if they are meant to do the same thing)

Even putting these potential issues aside, I think it could be a bit confusing that feature flags meant for the CLI get passed to Zarf and vice-versa?

Edit: also (as I know you flagged) there's some clunkiness around the way this PR filters out features from the tool commands. I wonder whether this could be cleaner if the distinction between Zarf and CLI features was more explicitly designed/accounted for?

[joelmccoy]

In testing this does appear to fix the kubectl regression I experienced. I still think there's a broader concern though that is worth addressing or clearly deferring/opening a new issue for... --features / CLI_FEATURES still mixes UDS-owned features (for example, NextMode) with Zarf-owned features (for example, values) in one set. I'm not sure if there's any particular issues with this today but this could potentially cause problems in the future if:

• CLI or Zarf perform any validation around feature flags (that the selected flag actually exists)
• CLI/Zarf have overlapping feature flags (same name - this might be okay if they are meant to do the same thing)

Even putting these potential issues aside, I think it could be a bit confusing that feature flags meant for the CLI get passed to Zarf and vice-versa?

Edit: also (as I know you flagged) there's some clunkiness around the way this PR filters out features from the tool commands. I wonder whether this could be cleaner if the distinction between Zarf and CLI features was more explicitly designed/accounted for?

After seeing this I wish we would have taken the approach of separating the features. Like using --uds-features for cli features and --features for zarf features. I'd be happy to make this split as it is much more cleaner. This would be a breaking change but I think a worthwhile one for longevity here.

Created a ticket here: https://linear.app/defense-unicorns/issue/CLI-323/separate-ownership-of-uds-cli-and-zarf-feature-flags

[slaskawi]

Implemented position-sensitive ownership in mode.go: root --features/CLI_FEATURES configure UDS, while everything after either Zarf entrypoint passes to Zarf unchanged.

[joelmccoy]

LGTM - converted this to a breaking change in the pr title, but I think this small change is worth while

[chance-coleman]

🚀 looks good